7 Commits

Author	SHA1	Message	Date
rayhpeng	a6388bc967	fix(schedule): give each bookkeeping timestamp a single owner ... 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.	2026-07-29 19:49:48 +08:00
rayhpeng	f88c8e61bc	refactor(schedule): promote the domain errors to exceptions.py ... 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.	2026-07-29 19:33:12 +08:00
rayhpeng	152e82e25b	test(schedule): make the domain suite fail when the domain breaks ... 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>	2026-07-28 20:29:24 +08:00
rayhpeng	680f044e58	refactor(schedule): move schedule_spec translation onto its owners ... `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>	2026-07-28 20:03:15 +08:00
rayhpeng	2ded78fdad	test(schedule): close the domain's coverage and documentation gaps ... 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.	2026-07-28 12:08:17 +08:00
rayhpeng	d8527dd0f7	refactor(schedule): tighten the service API after review ... 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.	2026-07-28 11:59:39 +08:00
rayhpeng	9d0b09558e	feat(schedule): add the schedule domain model ... 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.	2026-07-28 11:16:16 +08:00