Restructure per review: state what the system promises before how the
code delivers it. The thin product-decision table becomes a full §1
behavioural spec written in zero implementation vocabulary -- user
operations, scheduling semantics, the skip-not-queue overlap decision,
the busy-wait guarantees (delayed, never lost; ordered by due time; no
catch-up of missed cron periods), the failure/recovery table, and an
explicit non-goals list (no queue, no punctuality guarantee, no task
credentials, single scheduler instance) -- each promise cross-referenced
to the section whose mechanism delivers it.
The mechanism chapters absorb what the spec now promises: §6.2 explains
where waiting goes (the table as an implicit priority queue via the
claim query's ordering), §6.3 and a new pitfall entry name the
budget-leak boundary when a completion callback is lost, §8's defence
table gains the race-window column and a per-layer error-translation
table as the journey's unhappy half, and the HTTP endpoint block moves
beside the commands it carries.
The two command-shape tests added with the command refactor are plain
functions in a module whose global pytestmark is asyncio, which pytest
reports as a warning per run. Making them coroutines matches the module
convention.
Bring SCHEDULE_DESIGN_zh.md to the same reference-implementation
walkthrough shape as FEEDBACK_DESIGN_zh.md: it now assumes the spec has
been read, drops the mid-migration 'current state' warnings (the legacy
code is deleted), and covers what the old version predated -- the
commands chapter (UNSET three-state updates, ContextChange, why the
clock- and callback-driven writes stay plain methods), the adapters and
composition-root chapter (CAS field ownership, the corrupt-row error
split, the two anti-corruption layers and the inbound completion
listener), the dispatch journey with the three-driver macro diagram the
spec's §5.3 points at, and the test-layering table for the current
suites.
The spec's §6 marks the schedule slice done and drops the three
completed to-dos; the docs index gains the schedule walkthrough next to
its sibling.
Align the feedback router's dependency injection with the schedule
slice's ScheduleServiceDep shape: a FeedbackServiceDep alias in deps.py,
handlers declaring the service as a parameter instead of resolving it
inline. Swapping the provider is now a one-line change and parameter
order stays unconstrained. The two conditional lookups in thread_runs.py
deliberately keep the imperative call -- they only need the service when
a page actually has an AI message to decorate, and a declared dependency
would 503 the whole listing on a memory backend.
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.
Resolves the alembic head fork: main introduced 0010_run_cancel_request
on 0009 while this branch carried 0010_feedback_tags -> 0011_..._drop.
The unmerged branch revisions are renumbered and rechained after main's
published one (0010_run_cancel_request -> 0011_feedback_tags ->
0012_feedback_drop_message_id), and every test head pin moves to 0012.
- HEXAGONAL_ARCHITECTURE_zh.md becomes the spec: Cockburn/AWS-sourced
standard structure (domain seven-piece layout, file naming rules),
the four-transformation conversion chain with fixed owners and method
names, commands/events design with upgrade triggers, an enforced rule
table, and generic read/write sequence + class diagrams
- FEEDBACK_DESIGN_zh.md is rewritten as the reference-implementation
walkthrough of that spec (commands, exceptions split, _apply mapping,
composition root, updated test map and pitfalls)
- add the definition and dispatch diagrams under docs/assets
Wiring moves out of deps.py::langgraph_runtime into
app/composition.py::build_domain_services, so the rule the assembly owns
-- a memory database backend yields no services and the routes answer
503 -- is an assertion in tests/test_composition.py instead of a comment
inside the lifespan.
- 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
* fix(runs): show run duration once per run and label it as elapsed work
turn_duration is the run's wall-clock lifetime (updated_at - created_at),
but both message endpoints stamped it onto every AI message of the run
and the frontend rendered each stamp as 'Thought for X seconds' — the
same number repeated per message, and tool-wait time presented as model
thinking latency (#4152).
- share one stamping helper between list_messages and list_run_messages:
only the run's final non-middleware AI message carries turn_duration
(list_run_messages already tried to do this but iterated reversed()
without stopping at the first match)
- completed-state trigger copy becomes 'Worked for X seconds' since the
number includes tool execution, not just thinking
Fixes#4152
* fix(threads): stamp turn_duration once per run in /history replay too
get_thread_history stamped turn_duration on every AI message of a run
instead of only the last one, so a run with several AI messages (e.g.
a tool call followed by a final answer) showed the same duration badge
more than once on reload.
Rebasing onto main picked up #4118, which replaced the /history
duration path with a checkpoint-metadata fast path plus an
event-store/run-manager fallback for legacy checkpoints - both of
which stamped every AI message per run_id, reintroducing this same
bug on both paths. This commit folds the once-per-run stamping
(stamp_turn_duration_on_last_ai) into both paths instead of only the
legacy one, and narrows the fast-path/fallback boundary to a per-run
completeness check (turn_run_ids - checkpoint_run_durations) instead
of a per-message one, so a run whose duration is already in checkpoint
metadata never re-triggers the event-store correlation fallback just
because its non-final AI messages correctly have no turn_duration.
* fix(frontend): stop caching a client-measured turn_duration per message
A run can produce several standalone content-only AI messages (e.g. a
subagent handoff), each briefly becoming the newest message and each
mounting its own Reasoning timer via the shared turnStartTime. Wiring
onTurnDurationChange let that per-message timer cache and display a
"Worked for X seconds" badge the moment a later message superseded it
- with a different, premature number, since the timer only measured
that message's own streaming window, not the run's total elapsed
time. That reintroduced the #4152 duplicate-badge bug through a path
the backend fix (once-per-run stamping) doesn't cover.
cachedDuration now only ever mirrors a backend-confirmed
turn_duration (already handled by the other effect here), so a
superseded message can no longer display a stale or wrong duration.
* fix(frontend): use 'Worked for' framing even without a persisted duration
The completed-state copy diverged depending on whether turn_duration
had landed yet: "Thought for a few seconds" (no duration) vs. "Worked
for X seconds" (duration present). Both describe the same completed
run, just with or without a persisted number, so the framing
shouldn't disagree on what happened.
* docs(runs): note why the middleware skip is inert on /history, add test
stamp_turn_duration_on_last_ai's middleware-caller skip only has an
effect on the event-store message shape (metadata.caller); checkpoint
messages replayed on /history never carry that field. Document why
that's not a gap - middleware writes go to thread metadata, not the
messages channel, so no middleware message ever reaches a
checkpoint's messages list.
Also lock the middleware-skip contract on list_thread_messages with
its own regression test, mirroring the existing one for
list_run_messages - the thread feed only gained this skip through the
same shared helper, and previously stamped every AI message,
middleware included.
* chore(frontend): retain current run duration UI
The frontend behavior was superseded by #4348; keep the latest main implementation while retaining this PR's backend fix.
* test(frontend): retain current reasoning coverage
Align the old PR test with the frontend implementation already merged in #4348.
---------
Co-authored-by: fancyboi999 <fancyboi999@users.noreply.github.com>
* fix(sandbox): unwrap Overwrite-wrapped state in ensure_sandbox_initialized
The same fork-restored wrapper that crashed after_agent also reaches the sandbox init path, where sandbox_state.get() on the Overwrite object raises AttributeError. Share the unwrap helper from #4381's follow-up module deerflow/sandbox/overwrite.py and apply it at both init sites.
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
* fix(sandbox): note why discarding fork_restored at the reuse sites is safe
* fix(sandbox): unify the Overwrite unwrap helper and pin the fall-through
- middleware.py now imports unwrap_sandbox from overwrite.py instead of
keeping a second local copy whose docstring had already drifted; the
shared helper covers both crash forms (subscript TypeError and the
.get()-form AttributeError)
- test the acquire fall-through: when the fork-restored id is gone from
the provider, a fresh sandbox is acquired and the stale wrapped state
is replaced by the plain acquired dict
- the reuse-path test now also asserts runtime.state["sandbox"] stays
wrapped, pinning the don't-treat-as-owned contract after_agent relies on
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
* fix(sandbox): unwrap Overwrite state in the sibling sandbox readers
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
---------
Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
A browser client served from a different origin than the Gateway never
learns the id of the run it just created, so a brand-new thread keeps its
placeholder route for the whole session and every action gated on an
established thread — edit and rerun, regenerate, branch — stays hidden
until the page is reloaded.
Run-creating routes return the run's id in `Content-Location`, and the
LangGraph SDK resolves run metadata from that header alone. It is not
CORS-safelisted, so a cross-origin response hides it from JS unless the
server lists it in `Access-Control-Expose-Headers`. `useStream`'s
`onCreated` therefore never fires and the app cannot rewrite its route.
Expose it. `GATEWAY_CORS_ORIGINS` is a supported deployment mode, so the
CORS middleware has to carry everything that mode needs to read. Same-origin
nginx deployments are unaffected because CORS never applies to them.
* feat(checkpoint): make delta snapshot_frequency configurable
* fix(config): carry legacy checkpoint_delta_snapshot_frequency with warning
Addresses review on #4516: the rename from the flat
database.checkpoint_delta_snapshot_frequency key to nested
database.checkpoint_delta.snapshot_frequency silently dropped the old
value (pydantic extra="ignore"). Add a before-validator that maps the
legacy key onto the nested one with a deprecation warning (nested key
wins when both are set), plus a CHANGELOG breaking-change note covering
the rename and the 1000 -> 10 default change.
* fix(checkpoint): validate frozen snapshot frequency
* feat(lark): sidecar credential broker for sandbox lark-cli (Pattern B)
Removes the plaintext Lark credential mounts (appSecret + OAuth tokens)
from the sandbox container. A long-running broker sidecar owns lark-cli
and the per-user config/data dirs and serves the command surface over
Pod loopback; the sandbox gets only a forwarding shim on PATH, so the
raw credential files never exist in the sandbox filesystem.
- lark_broker.py: stdlib-only loopback broker (argv passthrough with
shell=False, server-injected credential env, bounded I/O) + shim
script constant + install-shim mode.
- docker/lark-cli-broker: init(install-shim) + serve image.
- provisioner: LARK_CLI_BROKER_IMAGE + provision_lark_cli_broker →
shim init container + lark-cli-broker sidecar (config/data mounted
sidecar-only); credentials dropped from the sandbox container;
/api/capabilities reports lark_cli_broker_image. Broker supersedes
the Pattern A init-container binary when both are configured.
- gateway: lark_cli_env_overlay(broker=True) omits config/data env;
sandbox_lark_broker_active() TTL-cached mode resolver; broker added
to sandbox_runtime_mode / readiness and the settings UI.
Opt-in and off by default (empty LARK_CLI_BROKER_IMAGE ⇒ no change).
Closes#4338
* fix(lark): address Pattern B broker review findings (#4501)
Follow-up to the sidecar credential broker addressing the PR #4501 review:
- shim: split the on-PATH lark-cli into a /bin/sh launcher + Python shim body
so broker mode fails loudly (exit 127, actionable message) instead of ENOEXEC
when the sandbox image ships no python3; interpreter pinnable via
DEERFLOW_LARK_BROKER_PYTHON. Launcher bakes in the shim's absolute path since
$0 is the bare command name when run off PATH.
- broker: drop the dead cwd payload field (broker can't see the sandbox FS) and
document the command-surface-only / no-file-IO limitation.
- broker: return a structured 500 JSON on unexpected exec errors so the shim
gets a meaningful message, not an opaque transport failure; set a handler
socket timeout to bound slow/stuck connections.
- broker: add an opt-in DEERFLOW_LARK_BROKER_DENY_SUBCOMMANDS denylist that
refuses secret-dumping subcommands before spawning the binary, forwarded from
the provisioner sidecar.
- gateway: tighten the per-bash-call broker probe timeout (1.5s) and cache
negatives longer (300s) so non-broker remote-provisioner users don't pay a
latency hit; guard the mode cache with a lock; drop the dead
_probe_provisioner_lark_cli_init_image wrapper.
- docs: remove the broken design-doc link from the broker README.
Adds tests for launcher python resolution, cwd omission, denylist enforcement,
500-on-error, hot-path probe timeout + negative caching, and provisioner
denylist-env wiring.
Editing the only turn of a thread reran the original prompt: the model
answered the question the edit was replacing while the UI showed the
edited text, and the edit vanished on reload.
The replay-base lookup decided whether a checkpoint predates the target
user message by message id alone. DynamicContextMiddleware re-keys the
first user turn to `{id}__user` mid-run, so every checkpoint written
before it holds the same prompt under an id the lookup cannot match. The
scan walked past those and anchored inside the run that produced the
turn — a checkpoint that still contains the original prompt and owns the
injection node's pending writes, which the replay then re-added after the
edited message.
Require the replay base to be a settled checkpoint (no pending tasks) in
both the lineage walk and the chronological fallback. That rule is
middleware agnostic: the first turn now anchors on the thread's empty
initial checkpoint and later turns on the previous run's tail, which also
drops the existing reliance on LangGraph discarding a stale `__start__`
write.
Edit replay additionally passes `head_checkpoint` so it resolves its base
lineage-first like regenerate does, and a replayed user message is
restored to its pre-swap id: replaying `{id}__user` into a state that has
no reminder yet makes the middleware treat the turn as already injected
and silently drops its date and memory block.
Frontend: a prepared replay masks the turn it supersedes, so the
optimistic-message baseline is taken from the post-mask human count. The
pre-mask count can never be exceeded when the replay puts exactly one
human message back, and on the first turn the runtime re-keys the
replacement message so identity comparison cannot stand in for the count.
Fixes#4531
All 102 domain cases were green, and four of them would have stayed green
through the exact regression they were named after.
Assertions that could not fail
------------------------------
`test_a_claimed_task_is_marked_running_before_dispatch` asserted the lease was
released *after* dispatch -- the opposite of the ordering its name and
docstring describe. The claim is what makes a task uneditable while it is
being dispatched, so the only place that ordering is observable is inside the
launch; the launcher double now reads the repository from there.
`test_active_statuses_are_exactly_queued_and_running` restated the constant it
was checking, so editing the constant edits the assertion with it. Replaced by
`is_active` over all six statuses, which also covers `RUNNING` and the two
terminal statuses that had none.
`test_reuse_thread_with_an_empty_thread_falls_back_to_a_fresh_one` compared
its result against `task.thread_id`, which is `None` on the default task -- it
asserted "not None" against a method whose body is `str(uuid.uuid4())`. Now
asserts the fresh-thread semantics it is named for: a real uuid, distinct per
call.
`test_a_task_deleted_mid_flight_is_not_an_error` had no assert at all. That
path does have observable behaviour: the hook writes the run record before it
reads the task, so a task deleted mid-flight must still leave a finalized
record and a freed active slot.
Contracts stated in a docstring and nowhere else
------------------------------------------------
- a cron overlap must not leave `last_error` behind (service.py:455 branches
on it; only the `once` half was covered, so dropping the branch was free)
- a failed launch replaces the launch bookkeeping instead of carrying it over
the way a skip does -- which is what `last_run_id=None` in `_fail` means for
a task that had already run successfully
- `SchedulePolicy`'s defaults are the permissive ones, so a deployment that
configured no policy cannot have a business constraint invented for it
- transitions leave `updated_at` to the repository, rather than becoming a
second source of truth for the same column
ACTIVE_RUN_STATUSES' promised assertion
---------------------------------------
Its docstring says the check that it stays in lockstep with the partial unique
index's predicate "lives in a separate test module rather than the domain
tests". It did not exist, in that module or any other. `test_scheduled_task_
models.py` now reads both dialect predicates off `__table_args__` and compares
the values it extracts against the constant. The domain suite cannot do this
-- it is deliberately dependency-free and cannot import an ORM model -- so the
new domain case names where the other half of the rule lives.
Removed
-------
Three duplicates: an `INTERRUPTED -> CANCELLED` case the parametrize directly
above it already made, a trailing-Z case identical in path to the aware-run_at
case beside it (the `from_primitives` one is the real one, because it parses a
string), and an `ensure_launchable == next_after` case whose value another
case already asserts outright. Their reasoning moved into comments where it
still applies. The second copy of the tautology, in `test_schedule_fakes.py`,
goes with it.
Verification
------------
A green run is not evidence for this kind of change, so every new or rewritten
assertion was checked by mutation -- break the production rule, confirm the
guarding test fails. All nine caught, re-run after `ruff format` to confirm
the reformat did not soften any of them.
Net +207/-31 across four test files; 425 passed, 3 skipped for the schedule
and composition suites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`run_outcome_mapping.py` called itself "not a port implementation" and sat in
a package of secondary adapters, while the half that actually invoked the use
case lived as a closure in the composition root. It is one thing, and it is a
primary adapter: the run runtime calls it the way HTTP calls the router and
the clock calls the poller.
`ScheduleRunCompletionListener` now holds the whole responsibility -- decide
whether a finished run is ours, translate it, invoke the use case. Those are
not two jobs: "ignore this run" is only meaningful as "do not call the
service", so splitting them is what left the second half in a place where
behaviour is not asserted.
`build_run_completion_hook` drops to `return
ScheduleRunCompletionListener(service)`. The composition root's own docstring
says no adapter logic lives there; that is now true of it as well as of the
routers it was written about.
Placement
---------
Kept in `app/adapters/schedule/` rather than moved beside the other two
primary adapters. The context stays in one package; direction is stated by
the class name and each module's first line, and the package `__init__` --
previously empty -- now lists which of its modules point which way, so a file
added without that line is visibly a file whose direction nobody decided.
A subdirectory for a single inbound module would have made the other four
look like they had been sorted into something.
Tests
-----
This is the part that was not a rename. The conversion had 24 cases; the
invocation had none, because the composition root is not where behaviour is
asserted, so nothing covered "an ordinary chat run must not reach the
service" as opposed to "produces no outcome object".
The cases now drive `__call__` against a recording service, which asserts the
same mappings plus what was done with them, and adds the two that were
unreachable before: the service left entirely alone for a filtered run, and
the completion stamped with a tz-aware current instant.
`test_composition.py` gains `TestRunCompletionHook` for the assembly decision
that remains -- including that the hook is bound to the service it was given,
which a wrong wiring would type-check past. Confirmed by mutation that this
case fails when the binding is broken.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`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>
Switches production onto the hexagonal path. The legacy modules still
compile and still have tests, but nothing assembles them any more; deleting
them is the next commit, kept separate so it stays reviewable.
Composition root
----------------
`app/composition.py::build_domain_services()` is now the only place an
adapter is instantiated. It was extracted from `deps.py::langgraph_runtime`
rather than added to it: wiring there was tangled with engine startup,
orphan recovery and shutdown, so the one rule that governs it -- no SQL
backend means no service and the routes answer 503 -- could not be tested
without booting the whole application, and was held up by a single comment.
It is a pure function of already-built infrastructure, so that rule is now
an assertion. Feedback moved with it; doing this while adding schedule's
five objects costs one change instead of two.
Primary adapter
---------------
The router is protocol translation only. What is gone is the giveaway: cron
normalisation, `next_run_at` arithmetic, the re-arm rule and hand-written
ownership checks all now live in the aggregate. Domain errors map to status
codes through one table, so a new error surfaces as a 500 to be classified
rather than being swallowed by whichever `except` was nearest.
`spec_mapping` split in two (AWS's own layout puts the wire model under the
entrypoint that owns it, and a primary adapter must not import a secondary
one): `adapters/schedule/spec_column.py` for the JSON column,
`routers/schedule/spec_wire.py` for the HTTP body. The two shapes are equal
only by coincidence, so `test_schedule_spec_parity.py` runs every case
against both and compares their outputs and messages directly. Function
names differ per side so an import from the wrong one is visible.
Explicit responses
------------------
Routes returned the ORM row's `to_dict()`, leaking `user_id`,
`assistant_id`, `overlap_policy` and the two lease columns. The response
models publish exactly the field set the frontend declares -- asserted in
both directions, since an extra field is a leak and a missing one breaks a
client.
One wire detail was nearly changed by accident: Pydantic v2 serializes a UTC
datetime as `...Z`, while the legacy `coerce_iso` path emitted `+00:00`.
`UtcTimestamp` pins `isoformat()` so adopting a model does not silently
alter the wire format for every client parsing these.
Tests
-----
73 new cases: router behaviour driven through a real `ScheduleService` over
in-memory fakes (a mocked service would let the error mapping pass without
a domain error ever being raised), response shape, and the composition root.
Router mappings verified by mutation -- a wrong status code or a dropped
timezone fallback turns 9, 2 and 1 cases red respectively.
Two lifespan tests carried a `SimpleNamespace` config that predates this
change; `langgraph_runtime` now reads `config.scheduler`, so they were given
one. Tolerating the gap with `getattr` was rejected: `AppConfig.scheduler`
always exists, so the fallback would be unreachable in production and exist
purely to excuse an incomplete test double.
Full suite is back to its 24 pre-existing failures.
Adds the three remaining adapters plus the poller. Nothing is wired yet --
the composition root is the next commit -- so this is additive and the
legacy `app/scheduler/service.py` still serves production.
`run_launcher.py` is the pivot of the whole slice. The Gateway signals a
busy thread two ways -- `ConflictError` from the run manager, or an
`HTTPException(409)` from the route-level path -- which is why the legacy
scheduler service imported fastapi to tell them apart. Both are one domain
fact, and saying so here is what lets that import disappear without the
busy/failed distinction disappearing with it. Everything else becomes
`LaunchFailedError`, because the port promises the domain that nothing but
its two errors escapes. `CancelledError` is deliberately not caught:
shutdown is control flow, not a launch outcome.
`thread_lookup.py` narrows `ThreadMetaStore` to the one question this
context asks. `require_existing=True` is load-bearing -- the store's
default treats an absent row as accessible, which is right for a thread
not yet written and wrong for binding a task to it.
Both inherit their port explicitly, matching every other adapter in the
codebase including feedback's own anti-corruption layer, and both carry
the TODO naming the published contract that would replace them once the
upstream context has been through a slice of its own.
`run_outcome_mapping.py` implements no port: it is the inbound translation
the composition root will install on the completion hook, and it owns the
filtering the legacy hook did inline. Returning None means "this run is
none of the schedule context's business", so the service is simply never
called and needs no guard clauses.
`poller.py` keeps the two behaviours the legacy loop got right: a failing
poll must not end the loop (one transient "database is locked" used to
stop scheduling for the rest of the process life), and reconciliation must
not block startup.
One deliberate behaviour change: the legacy `start()` swept stale runs and
stuck once-tasks under separate try/excepts, so the first failing did not
stop the second. `reconcile_on_startup` is one call that lets failures
propagate -- the domain's position is that fatality is the caller's policy
-- so the poller's single except means a failed first sweep now skips the
second. Both end up logged and non-fatal, as before.
Tests: 50 new cases across the four modules, each port method called and
asserted on its return value. That is not decoration: inheriting a
Protocol means a misspelled method silently inherits its `...` body and
returns None, so the suite was verified by mutation -- renaming `launch`
and `exists_for_user` turns 16 and 6 cases red respectively.
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.
Renaming a conversation and then editing one of its turns reverts the
title to whatever it was before that turn ran, so the user's own name for
the thread is silently replaced by an older automatically generated one.
Edit replay resumes from the checkpoint before the edited turn, and that
checkpoint predates the rename. Regenerate already guards against exactly
this rollback by replaying the current title as graph input; the edit
replay path was added later and did not carry the guard over.
Replay the title the same way, but only when the replay base already has
one. An untitled base belongs to a thread the title middleware has not
named yet — pinning the current title there would keep a name generated
from the prompt this edit just replaced, and stop the middleware from
naming the rewritten turn.
HEXAGONAL_ARCHITECTURE_zh.md now carries rules only: the two orthogonal
boundaries, the AWS three-folder mapping, the two kinds of secondary
adapter, and how the boundaries are mechanically enforced. The feedback
walkthrough, its known gaps, and its todo list move to a module document,
so the guide stays readable as more modules are migrated.
Two corrections to the guide, both of which would have misled a reader:
- Ports belong inside `domain/`, not beside it. AWS places `ports/` as a
subdirectory of `domain/` and describes the domain folder as "domain and
interfaces"; lifting ports into a third top-level layer would make the
domain depend on an outside package to declare its own needs. The guide
now states this explicitly, since the opposite reading is common.
- The walkthrough had the service calling RunLookup before building the
aggregate. The code does the reverse, and the order matters: validation
runs before any port call, so an invalid rating on a nonexistent run is
reported as InvalidRatingError rather than RunNotFoundError. The same
section also called that check authorization; it is referential
integrity, and authorization is the router's owner_check plus this check
taken together -- which is why the port takes no user_id.
FEEDBACK_DESIGN_zh.md is new and follows the SCHEDULE_DESIGN_zh.md shape:
the aggregate and its invariants, both ports and the conventions that
matter more than their signatures, the four use cases, both adapters, the
walkthrough, the test layering, an extension guide, and a pitfall list.
Three things it records that were not written down anywhere:
- The aggregate reads the system clock in its default factory, which
schedule deliberately avoids. Acceptable while the timestamp is only a
bookkeeping stamp and feeds no rule; noted with the condition that would
force a change.
- A repository that explicitly inherits its Protocol turns a misspelled
method into a silent None, because the inherited body is `...`. Hit for
real during the move. isinstance() cannot detect it, so asserting "the
port is satisfied" is not a substitute for asserting return values.
- RunLookup has no contract test against a real RunStore. A renamed key in
the dict RunStore.get() returns would turn every rating into a 404 with
the suite still green.
README.md indexes both under Quick Links, next to ARCHITECTURE.md.
Group them by bounded context instead of by technology, one file per
port, and align the directory name with the AWS Prescriptive Guidance
layout (entrypoints / domain-with-ports / adapters).
app/infra/persistence/feedback.py
-> app/adapters/feedback/feedback_repository.py owned persistence
-> app/adapters/feedback/run_lookup.py anti-corruption layer
`persistence/` promised a technology-first classification that its own
contents contradicted: RunStoreRunLookup lived there while its docstring
said "no new SQL". Splitting per port makes that distinction structural.
SqlFeedbackRepository and _tz_aware move unchanged -- verified by
comparing their AST against the original rather than by eye. run_lookup.py
additionally gains a RunStore annotation behind TYPE_CHECKING (the module
is imported lazily by the composition root, so this keeps the runtime
import cost at zero), a docstring stating that this context owns no table
and writes no SQL against it, and a TODO recording the condition under
which the body is replaced: when the run context publishes a contract of
its own, the RunLookup port itself does not move.
Each module docstring opens with a fixed marker so the two kinds of
secondary adapter stay greppable:
grep -rl "anti-corruption layer" app/adapters/
Filenames deliberately carry no sql_ / acl_ prefix: a prefix encodes an
implementation property, so switching storage would force a rename even
though the port -- and therefore the import path -- has not changed. The
class name already carries it. A prefix earns its place once one port has
several production implementations, which is not yet the case here.
app/infra/ held nothing else and is removed.
The inner ring is organised per bounded context; the outer ring was not,
so this context's adapters were split across app/infra/persistence/ and
app/infra/schedule/ with no principle separating them — one held the SQL
repositories, the other held the mapping they both consume, purely
because of what technology each touched.
Everything the outer ring provides to the schedule context now lives in
app/infra/schedule/, mirroring domain/schedule/. That also gives the two
remaining adapters an obvious home: the run launcher and the thread
lookup are neither persistence nor mapping, and would have needed a
third rule under the old layout.
Files are moved with git mv so history follows them. app/infra/
persistence/feedback.py stays where it is: it belongs to a separate
migration and moving it here would put that work in this diff. The
package docstring records the asymmetry rather than leaving it to be
rediscovered.
Two secondary adapters plus the spec mapping they share, and the first
real payoff of the port boundary: test_schedule_fakes.py becomes a
contract suite that runs all 31 cases against both the in-memory doubles
and the SQL adapters on a real sqlite file. A rule stated in a port
docstring now has to hold for both, and a divergence is a failure rather
than a surprise in production.
The queries come over unchanged. The claim statement's FOR UPDATE SKIP
LOCKED and both protect_terminal conditional writes are this module's
concurrency contract, not style, and the IntegrityError translation in
add() is what lets the service collapse a lost active-slot race into the
same outcome as its own non-atomic fast path.
Two things the adapters own that the domain deliberately does not. The
claiming process's identity is generated here, because who claimed a
task is an identity rather than a rule and nothing reads it back. And
`Unsupported schedule_type` is raised by the mapping, because
ScheduleSpec only accepts the enum -- structural checks belong to the
boundary, value rules to __post_init__, and both surface as the same
domain error so the router maps one family.
_to_domain introduces a failure mode the legacy repository did not have:
a row whose stored schedule no longer parses. Single-row reads let it
propagate; list reads skip and log, so one corrupt row cannot 500 an
entire listing.
The contract suite reaches past the port in exactly one place -- seeding
a task that already carries a claim, a shape claim_due would never
produce -- and says so where it does.
