test(db/schedules): edge-case + property coverage for CAS status writers and analytics aggregation - #1827
Conversation
e5ef4e9 to
a692726
Compare
|
Follow-up tracking: the negative- The issue records the point this PR body makes — that canary G-03 detects the skew at severity |
|
Resolve by running |
… writers (#1771) /edge-cases target 3, sub-area A: the #1082 status-as-projection CAS writers in db/schedules/executions.py and db/schedules/queue.py. - test_1771c_schedules_cas_edges.py: matrix rows A1b/A2/A3/A5/A5b/A6/A7/A8/ A12/A13/A14/A15/A16 as parametrized cases on the db_harness (#300) full production schema — SQLite always, PostgreSQL when TEST_POSTGRES_URL is set. - test_1771c_schedules_cas_properties.py: P-A1 bounded RuleBasedStateMachine ("terminal is absorbing" — canary E-02 at the DB layer) plus a meta-test that injects a phantom reversal and proves the machine reports it; P-A2/P-A3 no-crash + idempotence asserted PER RETURN CLASS (bool / int / Optional[Dict] are three different contracts); P-A4 lexicographic-ISO oracle. - hypothesis==6.161.5 added to tests/requirements-test.txt (exact pin so the three concurrent #1771 slices produce an identical, trivially-mergeable line). One strict xfail: A6 — a clock-skewed started_at persists a NEGATIVE duration_ms. No product-code fix (#1771 AC#4). Test-only. No src/ changes. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…aggregation (#1771) /edge-cases target 3, sub-area B: db/schedules/analytics.py — get_agent_analytics (#1107), get_schedule_analytics (#868), get_agent_schedules_summary (#1115) and the pure leaves _bucket_for_trigger / _schedule_command_label. - test_1771c_schedules_analytics_edges.py: matrix rows B1–B12. Highlights: B1 pins _TRIGGER_BUCKETS.values() subset of _BUCKET_ORDER (the existing literal assertion in test_agent_analytics.py omits "Reminders", so it was never a completeness check); B6b mechanises the locked "headline avg is full-set, never the capped pool" discipline; B10 freezes iso_cutoff so the strict-'>' boundary is deterministic rather than a clock race. - test_1771c_schedules_analytics_properties.py: P-B1 conservation (sum of by_type == total_executions, over arbitrary unicode triggers), P-B2 rate/count bounds incl. the zero-terminal-day-is-None rule, P-B3 contiguous UTC-day timeline over arbitrary windows, P-B4 no-crash-total on the pure leaves. Hypothesis `event()` markers make non-vacuity provable via --hypothesis-show-statistics rather than assumed. Five first-run failures were all WRONG TESTS (my expectations/mechanisms), not product bugs — reflection notes recorded in the docstrings. Test-only. No src/ changes. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
/update-tests tail step. Appended with ensure_ascii=True so the existing \uXXXX escaping of 24 unrelated entries is preserved — the diff is +54/-0. The companion .claude/agents/test-runner.md catalog sync is deliberately NOT done: that path is inside the pinned private .claude submodule, and this wave must not move the submodule gitlink. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review fix. B9 hard-coded a 2026-03-01/02 pair, which forced a 10-year window to keep the rows in range — so the gap-fill loop built ~3650 day-dicts per call, and the test would have aged out of any tighter window. Derive the straddling instant from today instead and use a 168h window. Same assertion, cannot age out; the file's runtime drops 8.3s -> 2.4s. Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…m P-B2b (#1771) /review findings on the #1771 target-3 edge-case slice, fixed in place. Test-only: `git diff f7aff46..HEAD -- src/` stays empty (AC#4). R1 REQUIREMENTS MISSING — `get_schedule_analytics` was named a subject-under-test by the plan (§3) and by the edges file's own docstring, yet no test invoked it: under a new-tests-only coverage run its whole body (analytics.py:113-259) was unexecuted, and the reported 92% came from bundling nine pre-existing neighbour files. Added rows B13/B13b/B13c/B15. R2 Closed the residual gap the matrix itself flagged: the `tool_calls` JSON-SHAPE guards. The column is agent-written, so valid-JSON-not-a-list, list-of-scalars, dict-without-`name` and non-numeric `duration_ms` are all reachable input, and a regressed guard turns an analytics read into a 500. Neighbours cover only the `json.loads` raise. Now pinned on BOTH surfaces (`get_schedule_analytics` + `get_agent_schedules_summary`, 20 cases). R3 P-B2b was half-vacuous: it asserted `sampled is (eligible > cap)` with the cap at its production 5000 and at most 12 seeded rows, so the `True` arm was unreachable for every example. The cap is now drawn (1-6) and monkeypatched, with over/at/empty `@example` pins. Verified load-bearing — a probe forcing `sampled is False` fails on the explicit example. R7 Renamed the `st` loop variable that shadowed the conventional `hypothesis.strategies as st` alias used in the sibling properties file. Also corrected in `.plan/edge-cases-1771c-matrix.md` (untracked by convention): `analytics.py:234`/`241` were mis-described as tool_calls guards (234 is the empty-day guard, 241 the FAILED timeline arm, now covered); `542->550` is defensive-dead, not malformed-`started_at`-reachable (`'' > cutoff` is false in SQL, so an empty `started_at` never reaches the loop); both coverage numbers are now reported instead of only the bundled one; and the Hypothesis `event()` percentages are labelled as one observed run rather than a stable contract. Declined: ruff F811 on the `db_backend` fixture import (pre-existing repo-wide pattern in every db_harness consumer; no CI workflow runs ruff). Refs #1771 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three concurrent #1771 slices each added `hypothesis==6.161.5` to tests/requirements-test.txt at a different position with a different comment, so they conflicted with each other rather than merging cleanly. Rebuild the file from dev's copy and insert one byte-identical canonical block at one fixed position (after pytest-cov). All three slices now carry the same bytes in the same place, so they merge in any order and the file keeps exactly ONE hypothesis line. No product code, no test logic change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The first CI run this PR was ever able to produce (it was CONFLICTING until now, so GitHub could not build refs/pull/1827/merge and reported zero checks) failed `lint (sys.modules pollution check)`: 12 bare `sys.modules.pop(...)` calls, 3 per new file. Fixed by satisfying the guard, NOT by widening its baseline (`lint_sys_modules_baseline.txt` is untouched) — this guard has already caught this same class in #783, #606 and #875, and retiring it on a test-rigour PR would be backwards. Both eviction sites are out of monkeypatch's reach, or actively wrong for it: * the `utils*` shadow clear is IMPORT-time, before any fixture exists, so monkeypatch structurally cannot reach it; * the `ops` fixture must evict `db.*` so `from db.schedules import ...` re-imports against the harness-bound engine. `monkeypatch.delitem` records NO undo for a key absent on entry (verified against _pytest.monkeypatch), so the freshly imported harness-bound module would stay resident for later files — strictly worse isolation than the explicit pop it would replace. So both take the documented escape hatch instead: a top-level `_STUBBED_MODULE_NAMES` list plus an autouse `_restore_sys_modules` fixture, matching the shape of tests/unit/test_telegram_webhook_backfill.py. It is a teardown-time guarantee only — the snapshot is taken before the test body, so no in-test behaviour and no assertion changes. Verified: lint reports "203 violation(s) in 60 file(s); baseline allows 240 — no new violations"; the four files still report exactly 129 passed, 1 xfailed (same strict xfail); `git diff origin/dev...HEAD -- src/` still empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4451b2e to
615182d
Compare
Summary
Test-only PR. Applies
/edge-cases(understand → enumerate → generate → reflect → verify) totarget 3 of 7 of #1771: the precondition-guarded CAS terminal transitions in
db/schedules/executions.py+queue.py(the #1082 status-as-projection contract) and theNULL-skip / UTC-bucketing aggregation in
db/schedules/analytics.py.tests/.git diff f7aff466..HEAD -- src/is empty — no product code touched (Run edge-case analysis & property testing (P-38) on the highest-risk core areas #1771AC#4).strict=Truexfail rather than a silent fix.RuleBasedStateMachine); 28 newly covered.Part of #1771 (target 3 of 7). Refs #1771.
Changes
tests/unit/test_1771c_schedules_cas_edges.pytests/unit/test_1771c_schedules_cas_properties.pytests/unit/test_1771c_schedules_analytics_edges.pytests/unit/test_1771c_schedules_analytics_properties.pytests/requirements-test.txthypothesis==6.161.5tests/registry.jsonNo
docs/delta, and that is deliberate per Trinity Rule #4: no new capability, no API change,no schema change, no behaviour change. The tests encode requirements that already exist
(
architecture.mdalready documents "Status-as-projection (#1082)" and the #1107/#868/#1115analytics entries accurately). Adding "and it has tests" would be changelog narration, which that
file's editorial rules forbid.
✅ Merge state — rebased, conflict-free
Rebased onto current
origin/dev— merge-base is now thedevtip, 0 commits behind. The oneexpected conflict,
tests/registry.json, is resolved.It was mechanical, not semantic:
devappended a registry entry fortest_1809_image_drift_recreate.py(and stripped the file's trailing newline) while this branchappends four
test_1771c_*entries at the same array tail. Both sides' entries are kept,comma-separated, in the one array — nothing dropped, nothing rewritten:
93(dev)+ 4(this branch) = 97, and the file parses as JSON;devis a pure insertion — 54 lines added, 0 deleted;dev's no-trailing-newline form is preserved, so there is no gratuitous diff line.Because the branch was conflicting from the start, GitHub could never build a
refs/pull/1827/mergeand therefore reported zero CI checks. That is now unblocked — this isthe first time CI can actually run on this PR.
Sibling collision — resolved. All three #1771 slices now carry a byte-identical
hypothesis==6.161.5block at one fixed position (immediately afterpytest-cov>=6.0.0) intests/requirements-test.txt— verified by hashing the block itself (sha256bf4d409f…b549acd, equal across 1771a / 1771b / 1771c). The slices therefore merge cleanly inany order and leave the file with exactly one
hypothesisline: no manual dedupe, nocomment-block arbitration at merge time.
The real bug this found — negative
duration_msreaches the Overview chart (A6 / B8)Shipped as
@pytest.mark.xfail(strict=True)plus a companion characterization test pinningtoday's behaviour. No product-code fix — that is out of scope per #1771
AC#4.started_atin the future relative to the finalizing process's clock makesupdate_execution_statuspersist a negativeduration_ms(unguardedcompleted_at − started_at), which then flows unguarded intoget_agent_analytics→observed
{'avg': -299997, 'p95': -299997}.runningwithstarted_at = now + 300 s→update_execution_status(id, "success")→duration_ms = -299997.started_atandcompleted_atare written by differentprocesses. The standalone
src/scheduler/container repeats the same unguarded subtractionat
src/scheduler/database.py:514-516, and the backend runs--workers 2.minor).Nothing prevents the poisoned metric, and the analytics consumption path is entirely
unguarded.
--runxfail, it fails on its own asserted contract(
assert duration >= 0,negative duration_ms persisted: -299996) — not on an unrelated error.Coverage — reported as TWO numbers, because one alone misleads
A pre-review draft of this verdict quoted a single "92%" that came from bundling nine
pre-existing neighbour test files. That number is real but not attributable to this PR.
Both are given:
A. This PR's tests alone (attribution)
B. Bundled with the nine neighbour files (the fleet's real position)
Residual misses — attributed, not hand-waved
queue.py:123with_for_update(skip_locked=True)— structurally unreachable withoutTEST_POSTGRES_URL.[SQLITE-ONLY].analytics.py:658-678get_all_agents_schedule_counts— out of declared scope (plan §3).analytics.py:234and542->550if not dayguards oversubstr(started_at,1,10). The only falsydayis an emptystarted_at, and the query filtersstarted_at > cutoff; SQL evaluates'' > '2026-…'as false, so it never reaches the loop. A malformed non-empty value is> cutoffbut yields a truthysubstr, taking the live arm. Reported, deliberately not covered.executions.py:417claim_tokenCAS arm — covered bytest_1081_lease_reaper.py(row A9), not by this PR.Honest scope limits
TEST_POSTGRES_URLwas never set, so every DB-touching case andproperty here ran on SQLite. Two spots are dialect-sensitive and labelled: B9 (
substrday-bucketing) and
queue.py:123(FOR UPDATE SKIP LOCKED, the sole uncovered line,structurally unreachable here). Everything else rests on standard SQL
(
COUNT/AVGNULL-skipping/CASE/integer truncation) or ASCII ISO-8601 collation, equivalenton both backends.
--mutateis scoped to target 1 by the plan, andmutmutis unusable in this repo'ssys.pathlayout. So "90% branch" is a coverage claim,not a mutation-kill claim.
test_1081_pull_endpoints.py::TestClaimConcurrencyC1.CANCELLEDblocks a SUCCESSoverwrite; immutability is an upstream refactor: fire-and-forget dispatch — a hung turn holds zero backend resource #1083 property, not a DB-layer one.
Traps deliberately avoided
bool ⊂ int. A globalisinstance(result, int)would pass for the wrong reason on everybool-returning writer. Assertions are therefore per return class: identity
(
is True/is False) for the 7 bool writers,isinstance(int) and not isinstance(bool)forthe 4 rowcount writers,
dict-or-Noneforclaim_next_queued.sampled is (eligible > _PERCENTILE_ROWSET_CAP)with the cap left at its production5000against ≤12 seeded rows — so the
Truearm was unreachable for every example and half thebiconditional passed vacuously. The cap is now drawn (1–6) and monkeypatched, with
over/at/empty
@examplepins; a probe forcingsampled is Falsenow fails, proving the arm islive. The constant verifiably restores to 5000 at teardown.
(
run_state_machine_as_testprints no statistics, so a pass is weak evidence alone):DID NOT RAISE AssertionError(itdetects the injected reversal, not incidental noise);
machines: 33, reaching_terminal: 30, terminal_observations: 152— the phantom-reversal arm is genuinely reachable.
A requirements gap the review caught
get_schedule_analyticswas named a subject-under-test by both the plan and the edges file'sown docstring — yet no test invoked it.
analytics.py:113-259was entirely unexecuted undernew-tests-only coverage. Closed with 16 added cases (rows B13/B13b/B13c/B15), which is what
moved analytics attribution from 54% → 96%. Those cases also close the
tool_callsJSON-shapeguards on both analytics surfaces — that column is agent-written, so every malformed shape is
reachable input and a regressed guard turns an analytics read into a 500.
Order-independence verified for real
pytest-randomlyis absent locally but installed out-of-band by CI(
backend-unit-test.yml:99) — so a locally-green suite proves nothing about CI ordering. Verifiedexplicitly: 4 solo seeds + 2 interleaved runs with 9 neighbour files + a CI-shaped invocation,
all green, with the venv restored afterwards.
Security
/cso --diff: zero findings.hypothesis→sortedcontainers, whichfakeredisalreadyrequired.
monkeypatchfixture — zerobare module-level
setattr.hypothesis==6.161.5exact pin (rather than a floor) is deliberate: it freezes areproducible derandomized input set, and is the safer supply-chain posture.
Test Plan
cd tests && python -m pytest unit/test_1771c_*.py -q→ 129 passed, 1 xfailed (17.7s); re-run on the rebased branch → 129 passed, 1 xfailed (14.4s, Python 3.12, hypothesis 6.161.5)tests/unittier → 1 failed, 5032 passed, 17 skipped, 1 xfailed. The single red isa pre-existing aged-out fixture (see follow-ups), reproduced on a pristine detached
worktree at the cut base
f7aff466with none of these files present.git diff origin/dev...HEAD --stat -- src/empty (re-verified after the rebase).python3 tests/lint_sys_modules.py→OK: 203 violation(s) in 60 file(s); baseline allows 240 — no new violations, andtests/lint_sys_modules_baseline.txtis untouched (seebelow).
/verify-localdeliberately skipped (explicit call, and justified): zerosrc/changes, noimage-reachable inputs, and the unit tier ran in full.
/verify-localexists to catchsource→image packaging gaps (#1033 class); a test-only diff has no image surface to break.
sys.moduleslint — satisfied, not baselinedThis PR was CONFLICTING from the moment it opened, so GitHub could never build
refs/pull/1827/mergeand reported zero checks. The rebase produced this PR's first ever CIrun — which immediately failed
lint (sys.modules pollution check)with 12 baresys.modules.pop(...)calls (3 per new file). Now green.Fixed by satisfying the guard, not by running
--regenerate-baseline: this guard has alreadycaught this same class in #783 / #606 / #875, and widening it on a PR whose entire point is test
rigour would be backwards.
tests/lint_sys_modules_baseline.txtis unchanged.Both eviction sites take the documented escape hatch (top-level
_STUBBED_MODULE_NAMES+autouse
_restore_sys_modules, shape copied fromtests/unit/test_telegram_webhook_backfill.py)rather than
monkeypatch, for a substantive reason in each case:utils*shadow clear is import-time, before any fixture exists — monkeypatchstructurally cannot reach it;
opsfixture'sdb.*eviction must not usemonkeypatch.delitem: verified directlyagainst
_pytest.monkeypatchthat it records no undo for a key absent on entry, so thefreshly imported harness-bound module would stay resident for later files — strictly worse
isolation than the explicit pop it would replace.
The fixture is a teardown-time guarantee only — the snapshot is taken before the test body, so
no in-test behaviour and no assertion changes. Confirmed empirically: the four files still report
129 passed, 1 xfailed, the same strict xfail, unchanged.
Follow-ups — flagged, NOT filed as issues
duration_msclamp (A6/B8 above). Thestrict=Truexfail flips to XPASS themoment a clamp lands — that is the signal to remove the marker. Both call sites need it:
db/schedules/executions.pyandsrc/scheduler/database.py:514-516.tests/unit/test_1474_read_boundary_z.py::test_schedules_summary_last_run_at_normalizedisaged out — belongs to no slice's fence, so it is nobody's by default. Root cause proven,
not guessed: the fixture hard-codes
NAIVE = "2026-07-06T11:00:00.207634"whileget_agent_schedules_summaryfiltersstarted_at > iso_cutoff(168), so the seeded row nowfalls outside its own 7-day window and
last_run_atisNone. Re-running the identical callwith a 5-year window returns the correctly
_norm_ts-normalized value — the behaviour undertest is fine; the test aged out (it would have gone red around 2026-07-13). One-line fix:
make the constant relative to
now— the same defect class this PR already fixed in its ownB9 case (commit
7740a4e1).analytics.py:234and542->550— either delete the guards oraccept them as defensive; they cannot be covered from this layer.
get_all_agents_schedule_counts(analytics.py:658-678) has zero coverage and is outsidethis slice's declared scope.
success_rateis0.0on a zero-terminal window while per-day isNone, so "never ran" and "failed everything" both render 0% at the headline. No requirementis violated (the discipline is silent on the headline) and changing it is a frontend-visible
contract decision. Characterized, not xfailed.
success → successwins the CAS twice. The DB-layer SUCCESS predicate isstatus != CANCELLED, which an existingsuccessrow satisfies. Not a bug at this layer — thereplay short-circuit is the refactor: fire-and-forget dispatch — a hung turn holds zero backend resource #1083 callback's job, and this looseness is exactly what the
documented "a late SUCCESS overwrites a reaper
LEASE_EXPIRED" guarantee depends on.Characterized so a future tightening is deliberate.
started_atraises before the CAS.update_execution_statusparses
started_atbefore building the CASWHERE, so a malformed non-NULL value raisesValueErroreven for a write that would have lost the CAS. Reachable at this layer(
update_execution_to_queuedcopies a caller-suppliedqueued_atin unvalidated) but notfrom any current production caller (
services/backlog_service.pypassesutc_now_iso()).The
Nonevariant is unreachable — the live schema enforcesNOT NULLon both backends,guarded by its own test since
db/tables.pydeclares the column nullable and a Collapse schema.py + migrations.py into single source of truth (follow-up to #713) #746metadata-driven-DDL migration would open the
AttributeErrorpath.ruff F811on thedb_backendfixture idiom, repo-wide (~10 files). Pre-existing patternin every
db_harnessconsumer; no CI job runs ruff today. Fixing it here would diverge from10+ neighbours. Reported, not changed.
Full edge-case matrix (38 rows + 12 properties) — click to expand
Status legend:
COValready covered ·NEWcovered by this PR ·UNSPECspec gap (reported,characterized, not xfailed) ·
BUGreal bug (strict xfail) ·GAPaccepted gap with written reason.Totals
Noneunreachable, A5b-malformed latent)analytics.py:234,542->550)Sub-area A — CAS status writers
running/failedqueued/pending_retry/skippedcancelledsuccessTruetwice)False, no raisestarted_atNULLstarted_at''/ garbage / out-of-rangeValueErrorbefore the CASqueued_atstarted_atunvalidatedstarted_at300 s in the futureduration_ms = -299997persistedstarted_atZ·+00:00·+05:00aware − naiveTypeErrorretry_countNonepreserves prior;0/1/3write throughNoneclaim_tokenupdate_execution_to_queuedover ×4 terminalsmark_execution_dispatched×2False0max_age_hoursqueued_atNULLqueued_atisnot(None)get_queued_countisolation;""⇒ 0find_expired_leasesexcludes future / NULL / terminal; ordering +limitProperties (sub-area A)
max_examples=30,stateful_step_count=8)bool, never raise, over the full status domainint(notbool)claim_next_queued→dict(only fromqueued) orNoneFalse, status unchanged0claim_next_queuedserves each queued row exactly once, FIFO, thenNoneutc_now_iso()strings, lexicographic order ≡ chronological orderInvariant #16existsSub-area B — analytics aggregation
_TRIGGER_BUCKETS.values() ⊆ _BUCKET_ORDER;Otherlast; no dupestriggered_by""·None· uppercase · paddedOthersuccess_rate is None0.0while per-day isNoneerrorfolded intofailedcontext_usedNULLNonecap−1/cap/cap+1sampledflips only above capavgfull-set whilep95is sampledNone/ value / 195 (inclusive interpolation)duration_msNULLint(None)never reachedduration_msavgandp95{'avg': -299997, 'p95': -299997}started_atoffset+05:00value →substr(...,1,10)[SQLITE-ONLY]cutoff±1 µs>is strict ⇒ exact row excludedhours∈ {24,168,336,720}; zero-daysmessagetool_callsget_schedule_analytics: empty · non-JSON · valid-JSON-not-a-list · list-of-scalars · dict-without-name· falsy name · non-numericduration_ms/review)success/failedday counters;cancelled/runningadd cost + total only/review)tool_callsget_agent_schedules_summary; NULL filtered in SQL not Pythontool_call_totalexact/review)get_schedule_analytics0 / 1 / 2 / 3 success rows/review)None(router → 404)analytics.py:115Properties (sub-area B)
sum(by_type[*].total) == total_executionsfor any multiset of arbitrary unicode triggers — the mechanised "a new trigger never silently vanishes"Other, 54% multi-bucket0 ≤ success_rate ≤ 1(orNoneper-day); sub-counts ≤ total; zero-terminal day ⇒Nonesample_size ≤ capand≤ eligible;samplediff pool exceeds cap/reviewto de-vacuum (see "Traps deliberately avoided")[now−hours, now]for anyhours∈ [1,1000]_bucket_for_triggertotal, result always ∈_BUCKET_ORDER_schedule_command_labeltotal,str, ≤ 80 chars, newline-free🤖 Generated with Claude Code