rbrooks/Quest-Board

Watch

Fork 0

An @llm_task decorator for the duplicated Celery task preamble (follow-up to #287) #294

New issue

Closed

opened 2026-08-07 06:00:09 +00:00 by claude-bot · 2 comments

claude-bot commented

2026-08-07 06:00:09 +00:00

Collaborator

Fix direction 4 from #287, split out so it does not ride along with that issue's semantic changes. #287 is closed; this is the structural half it deliberately deferred.

Why it was deferred, and why it still matters

#287 made the "no LLM configured" policy documented — settings_service now names the four legitimate policies and require_llm_config implements the default one. But documentation is a convention, and conventions drift. The reason eight behaviours appeared in the first place is that each task hand-writes its own preamble, so there is nothing structural stopping a ninth.

The decorator is what would make the policy uniform by construction rather than by discipline.

The duplication, measured against current `main` (`0a8910e`)

reminder_tasks.py is 4,676 lines. Within it:

28 asyncio.run(...) sync→async wrappers
30 async with task_session() as db: blocks
8 copies of the uuid.UUID(x) / except ValueError / log / return parse guard
12 sites resolving an LLM config across reminder_tasks.py and planning_tasks.py

Six tasks share essentially one skeleton end to end:

task	line
`_generate_lore_entry_summary_async`	3361
`run_lore_entry_draft_generation`	3468
`run_workbench_generation_core`	3848
`_propose_lore_relationships_async`	3927
`_generate_journal_entry_async`	4254
`_generate_session_title_suggestions_async`	4411

Every one of them does, in order:

asyncio.run wrapper around an inner _async
parse the id, log and return on ValueError
async with task_session() as db
function-local imports (to dodge circulars)
db.get(Model, id), log and return if None
resolve the LLM config and branch on None
wrap the real work in try/except, log, swallow

Steps 1–6 are boilerplate. Only step 7's body differs.

Shape

@llm_task(
    name="app.tasks.reminder_tasks.generate_journal_entry",
    loads=Session,                    # parse id + fetch + 404-equivalent guard
    llm=LlmPolicy.SKIP_WITH_WARNING,  # or REQUIRE / RECORD_ON_ROW
)
async def generate_journal_entry(db, session, llm_cfg):
    ...

LlmPolicy maps onto the four policies already documented in settings_service's module docstring — so the decorator becomes the enforcement point for a taxonomy that currently exists only in prose.

Constraints worth stating up front

process_audio is not a candidate. max_retries=0, its own try/except records audio_processing_status, and it resolves Whisper and VAD config alongside the LLM. Leave it alone.
The lore pipeline tasks (lore_chunk_extract, lore_deduplicate_extracts, lore_match_category) have a bespoke on_failure recording lore_generation_status and a retry handler that special-cases LlmNotConfiguredError. The decorator must not flatten that — either compose with it or exclude them.
Function-local imports exist to avoid circular imports. Hoisting them is a separate question; don't assume the decorator can.
Behaviour must not change. This is a refactor. Every existing test should pass untouched — if one needs editing, that is a signal the refactor changed semantics.

Acceptance

@llm_task exists, with the policy enum tied to settings_service's documented four
At least the six skeleton tasks above adopt it
No behaviour change: the full backend suite passes with no test edits
Net line reduction in reminder_tasks.py, and adding a new LLM task no longer requires copying a preamble

Not in scope

Splitting reminder_tasks.py (4,676 lines, and no longer only about reminders). Worth doing eventually; a decorator that shrinks it first makes that split easier to reason about.

Fix direction 4 from #287, split out so it does not ride along with that issue's semantic changes. #287 is closed; this is the structural half it deliberately deferred. ## Why it was deferred, and why it still matters #287 made the "no LLM configured" policy *documented* — `settings_service` now names the four legitimate policies and `require_llm_config` implements the default one. But documentation is a convention, and conventions drift. The reason eight behaviours appeared in the first place is that each task hand-writes its own preamble, so there is nothing structural stopping a ninth. The decorator is what would make the policy uniform **by construction** rather than by discipline. ## The duplication, measured against current `main` (`0a8910e`) `reminder_tasks.py` is **4,676 lines**. Within it: - 28 `asyncio.run(...)` sync→async wrappers - 30 `async with task_session() as db:` blocks - 8 copies of the `uuid.UUID(x)` / `except ValueError` / log / `return` parse guard - 12 sites resolving an LLM config across `reminder_tasks.py` and `planning_tasks.py` Six tasks share essentially one skeleton end to end: | task | line | |---|---| | `_generate_lore_entry_summary_async` | 3361 | | `run_lore_entry_draft_generation` | 3468 | | `run_workbench_generation_core` | 3848 | | `_propose_lore_relationships_async` | 3927 | | `_generate_journal_entry_async` | 4254 | | `_generate_session_title_suggestions_async` | 4411 | Every one of them does, in order: 1. `asyncio.run` wrapper around an inner `_async` 2. parse the id, log and return on `ValueError` 3. `async with task_session() as db` 4. function-local imports (to dodge circulars) 5. `db.get(Model, id)`, log and return if `None` 6. resolve the LLM config and branch on `None` 7. wrap the real work in `try/except`, log, swallow Steps 1–6 are boilerplate. Only step 7's body differs. ## Shape ```python @llm_task( name="app.tasks.reminder_tasks.generate_journal_entry", loads=Session, # parse id + fetch + 404-equivalent guard llm=LlmPolicy.SKIP_WITH_WARNING, # or REQUIRE / RECORD_ON_ROW ) async def generate_journal_entry(db, session, llm_cfg): ... ``` `LlmPolicy` maps onto the four policies already documented in `settings_service`'s module docstring — so the decorator becomes the enforcement point for a taxonomy that currently exists only in prose. ## Constraints worth stating up front - **`process_audio` is not a candidate.** `max_retries=0`, its own `try/except` records `audio_processing_status`, and it resolves Whisper and VAD config alongside the LLM. Leave it alone. - **The lore pipeline tasks** (`lore_chunk_extract`, `lore_deduplicate_extracts`, `lore_match_category`) have a bespoke `on_failure` recording `lore_generation_status` and a retry handler that special-cases `LlmNotConfiguredError`. The decorator must not flatten that — either compose with it or exclude them. - **Function-local imports exist to avoid circular imports.** Hoisting them is a separate question; don't assume the decorator can. - **Behaviour must not change.** This is a refactor. Every existing test should pass untouched — if one needs editing, that is a signal the refactor changed semantics. ## Acceptance - [ ] `@llm_task` exists, with the policy enum tied to `settings_service`'s documented four - [ ] At least the six skeleton tasks above adopt it - [ ] No behaviour change: the full backend suite passes with no test edits - [ ] Net line reduction in `reminder_tasks.py`, and adding a new LLM task no longer requires copying a preamble ## Not in scope Splitting `reminder_tasks.py` (4,676 lines, and no longer only about reminders). Worth doing eventually; a decorator that shrinks it first makes that split easier to reason about.

claude-bot added the

backend

label

2026-08-07 06:00:12 +00:00

claude-bot referenced this issue

2026-08-07 06:00:18 +00:00

"LLM not configured" behaves 8 different ways across 27 call sites, and one of them strands a session in extracting forever #287

claude-bot referenced this issue

2026-08-25 19:32:48 +00:00

[Review] Full audit of the session lifecycle: prep, recording, transcription, summarization, lore — scoping v4.0.0 #319

claude-bot added this to the v4.0.0 - Session Pipeline Accuracy milestone

2026-08-25 20:39:54 +00:00

claude-bot referenced this issue

2026-08-25 20:45:22 +00:00

[Review] Full audit of the session lifecycle: prep, recording, transcription, summarization, lore — scoping v4.0.0 #319

claude-bot commented

2026-08-26 15:14:15 +00:00

Author

Collaborator

Picking this up on feat/v4-deterministic-attribution.

Before implementing I read all six tasks as they stand now (reminder_tasks.py is 4,930 lines on the branch, not 4,676 — the v4 work grew it). One premise in the issue is wrong in a way that changes the design, so recording that here rather than silently building something else.

The six tasks are not uniform in ordering, and that is load-bearing

The issue says every one of them does steps 1–7 "in order". They don't. The LLM resolution sits at a different point in each:

task	what runs before the LLM check
`_generate_lore_entry_summary_async`	load entry, then `if not entry.body: return`
`_generate_journal_entry_async`	load session, `if not session.summary: return`, "public note already exists" guard
`_generate_session_title_suggestions_async`	load campaign only
`_propose_lore_relationships_async`	nothing — the LLM check is first, entries load after
`run_workbench_generation_core`	load row, `if tool is None: mark_failed("Unknown workbench tool")`
`run_lore_entry_draft_generation`	load draft only

A decorator that resolves the config at the top and hands the body an llm_cfg reorders those guards. That is a real behaviour change, not a cosmetic one:

An entry with an empty body on an install with no LLM currently returns quietly; hoisted, it would log LLM not configured, skipping entry … at WARNING. Same for every session with no summary and every session that already has a journal note. On a no-LLM install that inverts the log from "nothing to do" to a standing warning — the exact noise problem #287 set out to fix.
Worse, in the workbench core an unknown tool_id would start reporting "LLM endpoint not configured" to the GM instead of "Unknown workbench tool". That one is user-visible and simply wrong.

Design: the body decides when, the decorator decides what happens on None

So the config is passed as a zero-argument async resolver, not a value:

@llm_task(task_name="generate_journal_entry", subject="session",
          loads=Session, llm=LlmPolicy.SKIP)
async def _generate_journal_entry_async(db, session, llm) -> None:
    if not session.summary:
        return
    ...
    llm_cfg = await llm()      # resolves here, exactly where it does today

llm() never returns None — it either yields a config or executes the declared policy (log-and-return for SKIP, LlmNotConfiguredError for REQUIRE, the row recorder for RECORD_ON_ROW). That is stronger than the original sketch, not weaker: with llm_cfg handed in, a body could still forget to branch on None. Here there is no None branch to get wrong, and ordering is preserved exactly. Uniform by construction, which was the point.

Two corrections to scope

Policy 2 (RECORD_ON_ROW) does not generalise. "The row" and the recording differ per task — draft.status/draft.last_error versus generation_result_service.mark_failed(...) with a task-specific message. And both of those tasks' cores (run_lore_entry_draft_generation, run_workbench_generation_core) are called directly by 20-odd tests as f(db, id), so their signatures are pinned and a decorator cannot reach inside them. The enum keeps the member and takes an on_missing callback, but for these two the decorator only absorbs the wrapper preamble (parse + session); their policy-2 branch stays where it is. Adding this to the "constraints" list alongside process_audio and the lore pipeline.

The net-line-reduction acceptance criterion is unlikely to be met by six tasks alone. The preamble is ~20 lines per task, so six adopters save ~120 lines against a decorator that costs about the same once documented. I'm adopting a seventh (_update_campaign_storyline_async — same preamble, and it makes the kwargs path real) which helps, but the honest value here is the enforcement point, not the line count. reminder_tasks.py gets meaningfully shorter only when the other ~20 task_session blocks adopt the preamble half, and most of those take no id at all, so their preamble is a single line and a decorator would not pay. I'd suggest striking that criterion rather than chasing it.

planning_tasks.py is excluded: both of its LLM sites sit inside a Redis lock and return a dict, a different skeleton entirely.

Picking this up on `feat/v4-deterministic-attribution`. Before implementing I read all six tasks as they stand now (`reminder_tasks.py` is **4,930** lines on the branch, not 4,676 — the v4 work grew it). One premise in the issue is wrong in a way that changes the design, so recording that here rather than silently building something else. ## The six tasks are not uniform in *ordering*, and that is load-bearing The issue says every one of them does steps 1–7 "in order". They don't. The LLM resolution sits at a different point in each: | task | what runs *before* the LLM check | |---|---| | `_generate_lore_entry_summary_async` | load entry, **then `if not entry.body: return`** | | `_generate_journal_entry_async` | load session, **`if not session.summary: return`**, **"public note already exists" guard** | | `_generate_session_title_suggestions_async` | load campaign only | | `_propose_lore_relationships_async` | nothing — the LLM check is *first*, entries load after | | `run_workbench_generation_core` | load row, **`if tool is None: mark_failed("Unknown workbench tool")`** | | `run_lore_entry_draft_generation` | load draft only | A decorator that resolves the config at the top and hands the body an `llm_cfg` reorders those guards. That is a real behaviour change, not a cosmetic one: - An entry with an empty body on an install with no LLM currently returns quietly; hoisted, it would log `LLM not configured, skipping entry …` at WARNING. Same for every session with no summary and every session that already has a journal note. On a no-LLM install that inverts the log from "nothing to do" to a standing warning — the exact noise problem #287 set out to fix. - Worse, in the workbench core an unknown `tool_id` would start reporting *"LLM endpoint not configured"* to the GM instead of *"Unknown workbench tool"*. That one is user-visible and simply wrong. ## Design: the body decides *when*, the decorator decides *what happens on None* So the config is passed as a **zero-argument async resolver**, not a value: ```python @llm_task(task_name="generate_journal_entry", subject="session", loads=Session, llm=LlmPolicy.SKIP) async def _generate_journal_entry_async(db, session, llm) -> None: if not session.summary: return ... llm_cfg = await llm() # resolves here, exactly where it does today ``` `llm()` never returns `None` — it either yields a config or executes the declared policy (log-and-return for `SKIP`, `LlmNotConfiguredError` for `REQUIRE`, the row recorder for `RECORD_ON_ROW`). That is stronger than the original sketch, not weaker: with `llm_cfg` handed in, a body could still forget to branch on `None`. Here there is no `None` branch to get wrong, and ordering is preserved exactly. Uniform by construction, which was the point. ## Two corrections to scope **Policy 2 (`RECORD_ON_ROW`) does not generalise.** "The row" and the recording differ per task — `draft.status`/`draft.last_error` versus `generation_result_service.mark_failed(...)` with a task-specific message. And both of those tasks' cores (`run_lore_entry_draft_generation`, `run_workbench_generation_core`) are called directly by 20-odd tests as `f(db, id)`, so their signatures are pinned and a decorator cannot reach inside them. The enum keeps the member and takes an `on_missing` callback, but for these two the decorator only absorbs the *wrapper* preamble (parse + session); their policy-2 branch stays where it is. Adding this to the "constraints" list alongside `process_audio` and the lore pipeline. **The net-line-reduction acceptance criterion is unlikely to be met by six tasks alone.** The preamble is ~20 lines per task, so six adopters save ~120 lines against a decorator that costs about the same once documented. I'm adopting a seventh (`_update_campaign_storyline_async` — same preamble, and it makes the kwargs path real) which helps, but the honest value here is the enforcement point, not the line count. `reminder_tasks.py` gets meaningfully shorter only when the other ~20 `task_session` blocks adopt the preamble half, and most of those take no id at all, so their preamble is a single line and a decorator would not pay. I'd suggest striking that criterion rather than chasing it. `planning_tasks.py` is excluded: both of its LLM sites sit inside a Redis lock and return a `dict`, a different skeleton entirely.

rbrooks referenced this issue from a commit

2026-08-26 15:34:26 +00:00

refactor(backend): make the LLM-missing policy structural, not a convention (#294)

claude-bot commented

2026-08-26 15:34:43 +00:00

Author

Collaborator

Done in bcd5187 on feat/v4-deterministic-attribution.

What shipped

app/tasks/llm_task.py (212 lines) — LlmPolicy (REQUIRE / RECORD_ON_ROW / SKIP, with policy 4 deliberately absent and a comment saying why: treating None as data to describe is an endpoint concern, never a task one), plus two layered decorators. task_body absorbs parse-id → task_session() → optional row load. llm_task adds the deferred resolver.

Seven tasks adopted it — the six from the issue plus _update_campaign_storyline_async, which has the identical preamble and exercises the **kwargs path (full_regenerate).

Three things that would have broken this quietly, recorded for whoever touches it next

task_session is imported inside the wrapper, not at module level. Seventeen tests patch app.database.task_session to reuse their transactional session; a module-level import binds the original at import time and every one of them fails somewhere confusing.
Logging goes through logging.getLogger(func.__module__), not the decorator's own logger, so records still originate from app.tasks.reminder_tasks.
The sentinel is caught by exact type, never except Exception. A body wrapping its LLM work in a broad except would otherwise swallow the policy and never run it — the one failure mode that would make this silently worse than the duplicated code it replaces. There's a regression test for it. None of the seven converted bodies actually had that shape, but the next one might.

One design change beyond the issue: llm is a required keyword with no default. Defaulting it to SKIP would hand out policy 3 to any call site that omitted the argument, which rebuilds #287's drift one level up with fewer places to look for it. Pinned by test_omitting_the_policy_is_an_error_rather_than_a_silent_default.

Verification: 1,130 passing before → 1,142 after, with 12 new decorator tests and zero edits to existing tests. ruff format + ruff check clean. Also imported app.tasks.reminder_tasks standalone in Celery's include order — 42 tasks register — to confirm the loads= model imports hoisted to module scope don't create a cycle. They can't: app.models.* import only app.database and sqlalchemy. The issue's warning about function-local imports holds for the service imports, which stayed local.

Acceptance, honestly

@llm_task exists, policy enum tied to settings_service's documented four
Six (seven) skeleton tasks adopt it
No behaviour change; full suite passes with no test edits
Net line reduction — not met. reminder_tasks.py 4,930 → 4,816 (−114), against 212 lines for llm_task.py. As flagged above, I don't think it's worth chasing: six adopters were never going to pay for a documented module, and the remaining task_session blocks mostly take no id, so their preamble is one line a decorator wouldn't improve. The value delivered is the second half of that criterion — adding a new LLM task no longer means copying a preamble, and the policy has one implementation instead of seven. Ticking the first half would mean either inlining the documentation or converting tasks that don't benefit.

Closing. The file split the issue puts out of scope is now easier to reason about, if you want an issue for it.

Done in `bcd5187` on `feat/v4-deterministic-attribution`. **What shipped** `app/tasks/llm_task.py` (212 lines) — `LlmPolicy` (`REQUIRE` / `RECORD_ON_ROW` / `SKIP`, with policy 4 deliberately absent and a comment saying why: treating `None` as data to describe is an endpoint concern, never a task one), plus two layered decorators. `task_body` absorbs parse-id → `task_session()` → optional row load. `llm_task` adds the deferred resolver. Seven tasks adopted it — the six from the issue plus `_update_campaign_storyline_async`, which has the identical preamble and exercises the `**kwargs` path (`full_regenerate`). **Three things that would have broken this quietly, recorded for whoever touches it next** - `task_session` is imported *inside* the wrapper, not at module level. Seventeen tests patch `app.database.task_session` to reuse their transactional session; a module-level import binds the original at import time and every one of them fails somewhere confusing. - Logging goes through `logging.getLogger(func.__module__)`, not the decorator's own logger, so records still originate from `app.tasks.reminder_tasks`. - The sentinel is caught by exact type, never `except Exception`. A body wrapping its LLM work in a broad except would otherwise swallow the policy and never run it — the one failure mode that would make this silently worse than the duplicated code it replaces. There's a regression test for it. None of the seven converted bodies actually had that shape, but the next one might. **One design change beyond the issue:** `llm` is a required keyword with no default. Defaulting it to `SKIP` would hand out policy 3 to any call site that omitted the argument, which rebuilds #287's drift one level up with fewer places to look for it. Pinned by `test_omitting_the_policy_is_an_error_rather_than_a_silent_default`. **Verification:** 1,130 passing before → **1,142 after**, with 12 new decorator tests and **zero edits to existing tests**. `ruff format` + `ruff check` clean. Also imported `app.tasks.reminder_tasks` standalone in Celery's include order — 42 tasks register — to confirm the `loads=` model imports hoisted to module scope don't create a cycle. They can't: `app.models.*` import only `app.database` and sqlalchemy. The issue's warning about function-local imports holds for the *service* imports, which stayed local. **Acceptance, honestly** - [x] `@llm_task` exists, policy enum tied to `settings_service`'s documented four - [x] Six (seven) skeleton tasks adopt it - [x] No behaviour change; full suite passes with no test edits - [ ] **Net line reduction — not met.** `reminder_tasks.py` 4,930 → 4,816 (−114), against 212 lines for `llm_task.py`. As flagged above, I don't think it's worth chasing: six adopters were never going to pay for a documented module, and the remaining `task_session` blocks mostly take no id, so their preamble is one line a decorator wouldn't improve. The value delivered is the second half of that criterion — adding a new LLM task no longer means copying a preamble, and the policy has one implementation instead of seven. Ticking the first half would mean either inlining the documentation or converting tasks that don't benefit. Closing. The file split the issue puts out of scope is now easier to reason about, if you want an issue for it.

claude-bot closed this issue

2026-08-26 15:34:47 +00:00