Remove the legacy stack the hexagonal slice replaced, now that nothing
assembles it: app/scheduler/service.py (the old orchestration),
app/gateway/routers/scheduled_tasks.py (the old dict-returning router,
no longer mounted), the deerflow/scheduler package (its cron/timezone
rules live in ScheduleSpec), the dict-returning repositories in
persistence/scheduled_task*/sql.py, and the deps.py providers and
app.state wiring that served them. The ORM rows and the
uq_scheduled_task_run_active partial unique index stay -- the table
definitions live with the shared engine/alembic infrastructure and the
schedule adapters are their only readers and writers.
The legacy test suites go with the code they pinned; every scenario has
a counterpart in the new suites. The one suite that guarded semantics
rather than the old implementation -- the real-database dispatch-race
TOCTOU tests -- is migrated to the new stack as
test_schedule_dispatch_race.py, driving ScheduleService over the real
SQL adapters with the same barrier, natural-timing, and index-semantics
cases.
Docs and comments that named the old classes as the current wiring
(backend/AGENTS.md, reload_boundary.py, channel/service comments) now
name the composition-root wiring instead.
created_at was minted twice for one construction: the aggregate's field
default read the clock, then the adapter's add() read it again and stored
its own value -- so the aggregate a caller held disagreed with the stored
row by however long the insert took, and the model docstring pointed at a
write path that no longer exists.
ScheduledTask.create now stamps created_at and updated_at from the
explicit now= it already receives (one clock reading, the rule input),
and the adapter persists the aggregate's instants verbatim on insert --
matching the feedback reference implementation. updated_at's later life
stays deliberately with the adapter's write paths: the CAS methods
(record_launch / record_completion) write the row without ever holding an
aggregate, so only storage can stamp 'last written'. The model docstring
now states that ownership split instead of citing the deleted legacy
repository. Pinned by a domain test on the factory and a contract case
on add() across both implementations.
SqlScheduledTaskRepository._to_domain raised InvalidScheduleError for a
row whose stored schedule no longer parses -- the same error the
aggregate raises for a client-submitted schedule, which the router maps
to 422. A stored fault therefore told the client its perfectly fine
request was wrong, and made the row unrepairable over HTTP: PATCH reads
the task before writing, so the fix path 422'd too. Enum rebuild
failures were worse -- a raw ValueError crossed the boundary
untranslated.
The rebuild is now translated to a dedicated CorruptStoredScheduleError,
raised only by the persistence adapter and deliberately absent from the
router's status table, so it falls through to the unclassified-500
branch: a server-side fault reported as one. List reads keep skipping
and logging the bad row. Pinned by tests/test_schedule_corrupt_rows.py
against a real sqlite database.
Add domain/schedule/commands.py with one frozen dataclass per HTTP-driven
write use case -- CreateScheduledTask, UpdateScheduledTask, PauseTask,
ResumeTask, DeleteTask, TriggerTask -- and make the service methods their
handlers, keeping the naming chain aligned across all three spellings
(command / handler / <Command>Request). The clock stays an explicit now=
handler parameter: it is a rule input, not part of the client's intent.
UpdateScheduledTask expresses partial updates with an UNSET sentinel, so
absence is unambiguous; the wire keeps its historical None-means-omitted
convention and the request model's to_command owns the translation. The
former ContextChange moves from service.py into commands.py unchanged.
The request models are renamed to <Command>Request and own transformation
① (to_command): identity is injected server-side and pinned by a test to
never appear on the wire models. Clock- and callback-driven writes
(run_once, dispatch_task, handle_run_completion, reconcile_on_startup)
deliberately stay plain methods -- those drivers have no wire shape to
translate, their inputs are already domain vocabulary.
The context package now also exports the commands and the service, which
completes its public API and retires the stale 'service not landed' note.
Move model/errors.py up one level to domain/schedule/exceptions.py, a
sibling of the model, matching the AWS domain layout the spec mandates
(exceptions/ is its own member of the domain folder, not part of the
model) and the feedback reference implementation. Class names keep the
PEP 8 Error suffix. Pure move -- the nine classes are AST-identical to
the originals; imports across the domain, adapters, router, and tests
now take errors from deerflow.domain.schedule.exceptions.
- command-ify the write use cases (RateRun / RetractRunRating; queries
keep plain parameters, commands stay dumb data)
- split the domain errors into exceptions.py, a peer of model.py
(PEP 8 Error suffixes, AWS-style module name)
- unify the aggregate->row mapping as _apply(row, feedback) so one
explicit field list serves both the insert and the update path
- drop the unused feedback.message_id column (migration 0011): feedback
is bound to a run, nothing ever wrote or read the field
- pin remove_for_run's equality semantics for user_id=None in the
contract suite and fix the port docstring that contradicted both
implementations
`spec_column.py` and `spec_wire.py` were the same 60 lines twice, kept in
sync by a parity suite. Both are gone; what they did is now split along the
line that actually separates the two boundaries.
Why two files existed
---------------------
They were one module until the slice split it, and the reason given for the
split holds: a primary adapter must not import a secondary one, and the two
shapes are equal only by coincidence. But that argument only requires the two
*shapes* to be independent -- it does not require the parsing *rule* to be
written twice, and writing it twice is what needed `test_schedule_spec_parity`
to assert the two agreed, down to identical error text.
Splitting it properly
---------------------
`ScheduleSpec.from_primitives(schedule_type, *, cron, run_at, timezone)` takes
four strings, not a `Mapping[str, Any]` -- the mapping was the thing that kept
this out of the domain, and four strings carry no transport or storage format
with them. It owns the whole rule: unknown type, missing or non-string field,
unparseable `run_at`, and (via __post_init__, unchanged) 5-field cron and
resolvable timezone. Values are checked rather than trusted, since both
callers read data a client can influence.
Each adapter keeps only what is genuinely its own -- which two keys its format
uses -- as private methods on the class that owns the boundary, matching how
`SqlFeedbackRepository` and AWS's own ports-and-adapters sample put the
conversion inside the adapter rather than beside it:
SqlScheduledTaskRepository._spec_from_row / _spec_to_column
models.ScheduledTaskCreateRequest.to_schedule / models._spec_to_wire
The emit direction stays duplicated, deliberately: it is three lines per side
with no rule in it, and the two are *allowed* to diverge -- one is an HTTP
contract, the other a storage format. Asserting they stay byte-identical was
a constraint neither side asked for, so that suite is not replaced.
The router stops building value objects
---------------------------------------
`create` passes `body.to_schedule()`. `update` passes
`body.to_schedule(current.schedule)`, replacing eight lines that re-emitted
the current spec to the wire shape purely to read defaults back out of it;
omitted parts now come off the value object directly.
Tests
-----
`test_schedule_spec_parity.py` is deleted (162 lines). Its structural and
value cases moved to `TestFromPrimitives` in the domain suite -- stated once
now instead of parametrized over two implementations. Emit coverage was
already elsewhere: wire in `test_schedule_response_models`, column via the
repository round-trip in `test_schedule_fakes`.
One gap found while removing it: the `once` half of the update fallback had
no coverage on either side (both existing cases use cron), and it is exactly
the branch this commit rewrites -- `spec_to_wire(current)` round-trip to
`current.run_at.isoformat()`. Added
`test_a_timezone_change_on_a_once_task_keeps_the_same_instant`, and confirmed
by mutation that it is the only case that catches that branch breaking.
Behaviour is unchanged: same wire shapes, same normalizations (whitespace in
cron, trailing Z re-emitted as +00:00), same error messages, so the 422
details clients see do not move.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A cron task could become permanently unschedulable after a run that failed
fast. Reproduced against the real SQL adapters, and against the legacy path
for contrast -- the legacy code does not have this bug, so the hexagonal
slice introduced it.
The window
----------
`dispatch_task` already documents it: "a fast-failing run can reach
handle_run_completion before this write lands". The hook therefore reads the
task while the dispatch path has not yet written its bookkeeping, and the
snapshot it holds still carries the elapsed `next_run_at` the claim was made
on. `handle_run_completion` then wrote that whole snapshot back through
`save()`, so whichever landed second undid the other.
For a cron task the result is terminal in the worst way. `record_launch`
writes the fresh fire time, bumps `run_count` and clears the claim; the
completion's whole-aggregate write restores the elapsed fire time, rolls
`run_count` back and leaves `status='running'` with `lease_expires_at IS
NULL`. Neither `claim_due` branch matches that shape (one needs `enabled`,
the other needs an expired claim), and `cancel_stuck_once_tasks` only sweeps
`once` rows -- so nothing can ever reach the task again.
The legacy `app/scheduler/service.py` wrote the same outcome field by field
(`update(..., updates={"last_error": ...})`) and never touched scheduling
state, which is why it survives the same interleaving.
The fix
-------
`record_completion` joins `record_launch` as a second deliberately narrow
port method, for the same stated reason `record_launch` is not expressed as
`save(task)`: the two race, so neither may write through the aggregate. They
now own disjoint fields -- the launch owns the schedule, the completion owns
the verdict (terminal status plus `last_error`, with `None` meaning "do not
move the status", i.e. every cron task).
`save()` keeps its documented purpose, the user-initiated whole-aggregate
updates (`update_task` / `pause` / `resume`). `handle_run_completion` still
reads the task first, but only to ask `status_after_completion`, which reads
nothing but the schedule type -- immutable, so that read carries no
time-of-check risk.
Tests
-----
The contract suite missed this because it groups cases by port method:
`record_launch` appears only among its own, never interleaved with `save`,
and `test_protect_terminal_keeps_a_concurrently_finalized_verdict` pins the
mirror-image direction only. The in-memory double replaces the whole row too,
so both implementations were faithfully wrong -- the defect was in the
contract, not either adapter.
Added: `TestRecordCompletion` (5 cases, both implementations), including the
interleaving itself; and `test_a_cron_task_survives_a_launch_write_landing_mid_completion`,
which drives a real `ScheduleService` over a repo double that commits
`record_launch` between the hook's read and its write. Both were watched
failing first -- the service case failing with the actual symptom, a rolled
back `next_run_at`, not a missing method.
Verified end to end against file-backed sqlite with the real service: the
task keeps `enabled`, the concurrent fire time and run count survive, the
verdict is recorded, and the next poll still claims it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adopts the layout feedback landed in cb49dd67: secondary adapters live
under `app/adapters/<context>/`, one file per port, the file named after
the port in snake_case with the technology carried by the class name.
`app/infra/` is now gone entirely.
The rename also separates two meanings of "run" that shared one filename
space: `run_sql.py` held `ScheduledRun` (an execution record), while the
`run_launcher.py` still to come deals in Gateway runs.
task_sql.py -> scheduled_task_repository.py
run_sql.py -> scheduled_run_repository.py
spec_mapping.py -> spec_mapping.py (implements no port, so no rename)
Both SQL adapters now carry the `Secondary adapter (owned persistence)`
docstring marker -- this context owns both tables and writes its own
queries. `spec_mapping` says instead that it is a boundary mapping and
names its two callers. The package `__init__.py` is empty, so imports go
through the full path and a class's home file stays unambiguous.
Pure move: every top-level symbol was compared against its pre-move
original by AST dump (docstrings excluded), plus a separate per-class
method-name comparison, since a whole-class dump reports a docstring edit
and a renamed method the same way.
Also repoints three docstrings and one diagram that still named the
deleted `app/infra/` path.
A review pass over domain/schedule found three genuinely untested
branches, all of which now have cases:
- ensure_launchable with a naive `now`. next_after already had this
covered; the delay floor did not, so a caller handing over a naive
clock reading could have had it shifted by the local offset unnoticed.
- update_task changing the prompt. Only the title path was exercised.
- _save finding the row gone. get_task saw it and save no longer does,
which is a concurrent delete; the caller must get the same not-found it
would have got a moment earlier rather than a None leaking out.
That takes service.py and every model module to 100%. The one remaining
uncovered line is croniter's naive-return guard, carried over verbatim
from schedules.py and unreachable with an aware input -- it now says so
instead of looking like an untested branch.
Also fills in the documentation the migration skipped: TaskStatus,
ContextMode and RunStatus arrived from the original draft without
docstrings while their newer siblings had them, and ScheduleService plus
four of its use cases were undocumented. Each now records the reasoning a
reader would otherwise have to reconstruct -- why RUNNING is not "the
agent is executing", why SKIPPED never passes through QUEUED, why
INTERRUPTED is not FAILED.
CRON_FIELD_COUNT stops being exported: it has no consumer outside the
module that defines it.
Four changes, all from the same observation -- the service was expressing
domain concepts in language the domain does not use.
DispatchOutcome replaces the Literal. The caller branches on all four
values and the SKIPPED/CONFLICT distinction is itself a business rule, so
it is domain vocabulary like every other enum in this context; a bare
string was the odd one out.
lease_owner leaves the domain entirely. It was a pure pass-through: the
service held it only to hand it to claim_due, which never reads it back.
Which process claimed a task is an identity, not a rule -- an adapter can
record one for diagnostics without the domain carrying it. lease_seconds
stays, because how long a claim survives genuinely changes recovery
behaviour.
The _UNSET sentinel is gone. It existed for one field: thread_id, the only
update parameter with a meaningful None. But thread_id and context_mode
always move together, so packaging them as ContextChange removes the
ambiguity and lets every other field use plain None for "not supplied" --
which also matches what the HTTP layer already does with exclude_none.
The previous `title: str = _UNSET` annotation was simply untrue.
pause/resume no longer route through a _transition helper taking an
unbound method. Two direct bodies plus a _save that raises on a missing
row read better, and _save documents why this stays a read-modify-write:
pushing "not while running" into a storage predicate would put the rule
beyond a zero-IO test and give it a second home. Closing that window
properly needs optimistic locking, which needs a schema change.
Also cleans three lint findings surfaced by a broader rule set than the
project enables: a regex metacharacter in a pytest match=, an unused
override parameter, and two deliberately-naive datetimes that now say so
with a noqa rather than looking accidental.
The input port of the context: every scheduled-task use case, orchestrated
over the four output ports. It holds no business rules -- each decision is
delegated to the aggregate -- and `test_schedule_service.py` runs the
complete lifecycle with no HTTP, no database and no run runtime, which is
the acceptance criterion this migration was for.
`dispatch_task` mirrors the pre-migration structure line for line,
including its comments, and makes exactly three substitutions: bare dicts
become domain objects, the HTTPException-409 sniffing becomes `except
ThreadBusyError`, and the status-derivation static methods become
aggregate methods. Its four exits and their two conflict paths are
unchanged, and the tests pin the collapse: the fast-path rejection and
the active-slot rejection must produce identical results, asserted over
every field of DispatchResult.
One behavioural narrowing, deliberate. The old code wrapped the launch
*and* its follow-up writes in `except Exception`, so a failing bookkeeping
write was recorded as a failed launch -- marking an execution that had
actually started as failed. The port contract admits exactly two escapes
from `launch`, so only the launch is guarded now and a genuine write fault
propagates instead of being misreported.
SchedulePolicy gains max_concurrent_runs and lease_seconds. Both are
operator-tunable thresholds the domain needs but must not read, which is
what that value object is for; the claiming process's identity stays a
constructor argument since it is an identity, not a threshold.
Four Protocols the domain declares and the outer ring will implement,
plus the two DTOs that keep infrastructure types out of the inner ring:
- ScheduledTaskRepository / ScheduledRunRepository, exchanging domain
objects rather than the bare dicts the current repositories return
- RunLauncher, whose contract is that only ThreadBusyError or
LaunchFailedError may escape -- that translation is what keeps the run
runtime and the web framework out of the domain
- ThreadLookup, one method rather than the whole thread store
- LaunchedRun and RunOutcome, so the completion path stops taking a
runtime record the purity test would reject
Two deliberate departures from the earlier sketch. There is no Clock
port: `now` is already an explicit parameter throughout, so the domain
never reads a clock and the tests are already deterministic -- adding
one would only create a second source of truth for the same value. And
`record_launch` is not expressed as `save(task)`, because
`protect_terminal` makes it a compare-and-set against a concurrently
finalizing run; a read-modify-write through the aggregate would
reintroduce the race the flag exists to close.
The in-memory doubles model the active-slot rule rather than skipping
it: a double that never refuses a second active run would let the
service's conflict collapse go untested. Their semantics are pinned by
test_schedule_fakes.py, which becomes the contract suite once the SQL
adapters land and both tiers run the same cases.
Concurrency is out of scope for the doubles and says so in their module
docstring -- it stays covered against a real database in
test_scheduled_task_dispatch_race.py.
First step of the scheduled-task context's hexagonal migration: the
inner-ring model, with zero infrastructure dependencies.
- ScheduleSpec / SchedulePolicy value objects
- ScheduledTask aggregate root
- ScheduledRun aggregate
- 9 domain errors, 5 enums
Rules are migrated verbatim from their current homes, each method's
docstring citing the source line: timezone/cron/next-run calculation
from deerflow/scheduler/schedules.py, context-mode and re-arm rules from
routers/scheduled_tasks.py, and the four status-derivation rules from
app/scheduler/service.py.
Two things previously held by convention are now enforced by
construction. Validation and normalization live in __post_init__, so
building a ScheduleSpec field-by-field cannot bypass them. The skipped
tombstone is a separate factory, so it can never be written as the
transient queued row that would collide with uq_scheduled_task_run_active.
The domain does not serialize itself: mapping the stored schedule_spec
JSON in and out stays with the adapter layer, keeping Mapping[str, Any]
out of every domain signature.
Production code still runs through app/scheduler/service.py -- this
commit adds no call sites and changes no behavior.
Four findings from @willem-bd on #4401:
- FeedbackDialog stays mounted across messages, so selected tags and the
comment survived an ESC/click-outside dismiss and pre-filled the next
thumbs-down. Reset on every close path via a wrapped onOpenChange.
- Neither the dialog's handleSubmit nor handleDialogSubmit caught a failed
enrichment PUT, so a rejection went unhandled and the user got no signal.
Catch in the dialog (where the rejection lands), toast, and keep the input
for a retry.
- rate_run awaited the RunLookup port before Feedback.create validated the
rating, contradicting its own "before any I/O happens" docstring: an
invalid rating on an unknown run surfaced as RunNotFoundError. Validate
first, restoring the legacy router's 400-before-404 ordering. The service
test now uses an unknown run id so it actually pins that order.
- InMemoryFeedbackRepository moved out of test_feedback.py into
tests/feedback_fakes.py (with FakeRunLookup) so two test modules share it
without one importing the other; conftest states the tests-dir sys.path
dependency explicitly, which also makes it work under
--import-mode=importlib.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The single-line `class X: ...` exception stubs tripped
github-code-quality's "Statement has no effect" rule: a bare `...` is a
discarded expression statement. Replace each with a one-line docstring,
matching the empty-error convention used elsewhere (GoalWriteConflict,
ConflictError) and silencing the false positive. No behavior change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hexagonal inner ring for the feedback bounded context: frozen aggregate
with construct-time invariants (rating, reason-tag slugs), technology-
neutral ports (typing.Protocol), and the application service with
explicit user_id. Guarded by an AST purity test that keeps domain/ free
of infrastructure imports.
