Document the live-test harness, the CI gate, and Indeterminate in AGENTS.md - #123
Conversation
…NTS.md Four PRs landed in one session (#118 harness, #120 gate stabilisation, #119 ProviderError::Indeterminate, #121 fake Orange + chaos suite) and the context file only picked up what the last two agents wrote inline. The biggest omission was the most consequential change: `sms-test-support` and the fact that CI now runs the 14 live-Postgres suites at all — previously `cargo test --workspace` ran without `--ignored`, so they were skipped in CI and had only ever run by hand. Records, in each case why it exists rather than just that it does: - The harness's five load-bearing constraints, each learned by something breaking: no container handle in a `static` (Drop never runs for statics, which leaked 56 containers), label-scoped cleanup only (an image-scoped sweep would destroy a developer's own database), one database per test binary (a shared one let a real dispatch subprocess steal other suites' messages), migration-content fingerprinting (an existence check served a stale schema silently), and the global container name that makes concurrent test runs corrupt each other. - That `ca653a1`'s mutex was never applied to two suites, both of which flaked in CI within hours of #118 running them for the first time. - `Indeterminate`'s connect-vs-read predicate, the `routed -> uncertain` edge it needed, and the fact that it trades a possibly lost message for never sending a duplicate — a product decision, not a free win. - A CLI-skew warning placed at both sites that tell you to run `cratestack migrate diff`, since the installed 0.7.4 CLI emits DDL the pinned 0.6.7 library never does. The warning previously existed only in an unrelated paragraph 150 lines away from the commands themselves. - The missing-`hasRole('system')` gap as an explicitly *unguarded* failure mode, now found seven times. The three entries above it each turned a silent failure loud; this one has no equivalent, which is why it recurs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
![lightbridge-assistant[bot]](https://avatars.githubusercontent.com/u/128943481?s=60&v=4){kind=small}
There was a problem hiding this comment.
🅵 Fast automated pass — SAST + a quick, diff-scoped look (no repo-wide retrieval). For a deeper, repo-aware review, mention @lightbridge-assistant on this PR.
Documentation-only change to AGENTS.md; no code, no secrets, and SAST is clean. I checked the added prose for internal consistency and found one provable accuracy defect: the new "hasRole('system') gap found N times" paragraph enumerates eight model names yet claims "seven separate times", and its count (and the sibling six-before list in the same hunk) disagrees about whether Message (#96) is a counted instance. Recorded as a P2; everything else in the added text is consistent and non-blocking.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| - **`crates/sms-api/src/errors.rs`** — `map_database_error` turns SQLSTATE `SM001` into `CoolError::Conflict` (409) and `23505` into a named conflict. Untranslated, an illegal transition is a `500 DATABASE_ERROR`, which callers retry and operators read as a gateway fault. Genuine database faults stay 500 on purpose. | ||
|
|
||
| **And one failure mode that is still *not* guarded, which is why it keeps happening.** A model whose `@@allow` omits `hasRole('system')` does not error — CrateStack's list-route policy denial is row-level filtering to an **empty array** (see #97's finding on `GET /oauth_signing_keys`), so an internal system-context read just quietly returns nothing, and the calling code behaves as though the table were empty. That has now been found **seven separate times**, always live and never by review: `App`, `AppClient`, `SenderIdRegistration`, `OperatorPrefixRule` (#94), `Provider`, `Job` (#100), `Message` list/detail (#96, which is why #29's claim loop had been returning zero rows for a milestone), and `DeliveryReceipt` (#121). Seven repetitions of one mistake is a missing invariant, not seven oversights — the other three entries above each turned a silent failure into a loud one, and this one has no equivalent. A golden test asserting that every model a system context must read actually admits one would end it; until someone writes it, expect an eighth. | ||
|
|
There was a problem hiding this comment.
Instance count off by one (8 listed, "seven")
The tally is off by one and disagrees with the paragraph just above. This sentence enumerates eight distinct models — App, AppClient, SenderIdRegistration, OperatorPrefixRule (#94), Provider, Job (#100), Message list/detail (#96), and DeliveryReceipt (#121) — but claims "found seven separate times" and "Seven repetitions". Meanwhile the "two real findings" paragraph above (this same hunk) calls DeliveryReceipt the "seventh instance" with only six before it (App…Job), omitting Message (#96). Message is recorded elsewhere in this file as a genuine instance of this same gap (see the #96/#29 claim-loop discussion), so it should be counted — making the correct total eight, not seven. Worth reconciling the count and both enumerations so the permanent-invariant argument ("seven oversights") isn't built on a miscount.
Evidence: AGENTS.md line 132: "That has now been found seven separate times, always live and never by review: App, AppClient, SenderIdRegistration, OperatorPrefixRule (#94), Provider, Job (#100), Message list/detail (#96…), and DeliveryReceipt (#121). Seven repetitions of one mistake…" — eight names listed, "seven" claimed. Contrast line ~108's "the seventh instance … recorded six times before (App, AppClient, SenderIdRegistration, OperatorPrefixRule, Provider, Job)" which omits Message.
Was this useful? React 👍/👎 to give us feedback
1 participant
Summary
Brings
AGENTS.md(whichCLAUDE.mdsymlinks to) up to date after four PRs landed in one session: #118 (testcontainers-backed live-Postgres harness + CI wiring), #120 (gate stabilisation), #119 (ProviderError::Indeterminate), #121 (fake Orange + chaos suite).Documentation only. No code, schema, or migration changes.
Intent
The agents behind #119/#121 updated
AGENTS.mdinline for their own work, but the single most consequential change of the session was absent entirely:sms-test-supportappeared zero times, and nothing recorded that CI now runs the 14 live-Postgres suites at all. Before #118,.github/workflows/ci.ymlran a barecargo test --workspacewith no--ignored, so every live suite was silently skipped in CI and had only ever been run by hand — the same shape of gap this file already blames for #29's claim suite sitting unrun for a milestone while hiding a policy bug.Source of truth: #118, #119, #120, #121 as merged, plus #122 for the
undelivereddead-end.Scope
static-held container handle leaking 56 containers becauseDropnever runs for statics; label-scoped cleanup, since an image-scoped sweep would destroy a developer's own running database; per-binary databases, because a shared one letkill9_reclaim_live's real dispatch subprocess claim other suites' messages; migration-content fingerprinting, because an existence check silently served a stale schema; and the global container name that makes concurrent local test runs corrupt each other). Also records thatca653a1's mutex was never applied to two suites, both of which flaked in CI within hours of Add a testcontainers-backed live-Postgres harness; wire it into CI (#102) #118 running them for the first time.ProviderError::Indeterminate: the connect-vs-read timeout predicate and why inverting it is harmful in both directions, therouted -> uncertainedge it required, theproviderMessageRefAltstamp that keeps such messages resolvable, and the explicit tradeoff — a possibly lost message in exchange for never sending a duplicate, which is a product decision rather than a free win.cratestackCLI is 0.7.4 against a pinned=0.6.7library and emits FK DDL the pin never produces. That finding previously lived only in an unrelated paragraph ~150 lines from the two places that actually tell you to runcratestack migrate diff. Now stated at both.Commands: addsjust test-live/just test-live-clean, notes the manualcreatedb vsms_checkflow is no longer needed for the Rust live suites, and warns against concurrent live-test runs.hasRole('system')gap as an explicitly unguarded failure mode, now found seven times (App,AppClient,SenderIdRegistration,OperatorPrefixRule,Provider,Job,Message,DeliveryReceipt). The three existing entries each turned a silent failure loud; this one has no equivalent, which is precisely why it keeps recurring.undeliveredfinding to #122.Verification
just paritygreen — and it now reports 25 message edges rather than 24, independently confirming Add ProviderError::Indeterminate for a submit that timed out after sending #119'srouted -> uncertainedge is present in both the diagram and the transition table.git diff --statis a single file).RetryThisProvideras aProviderErrorvariant when it belongs toRoutingConsequence.Screenshots/Evidence
N/A — prose-only documentation change.
Risk Assessment
None. Additive documentation; no behaviour affected.
AI Usage Declaration
Written by Claude Code (Claude Opus 5) under my direction, summarising work from the same session that produced and merged #118–#121, each of which I reviewed and merged myself.
crates/sms-test-supportas merged, not against agent reports.#[ignore]/just test-liveworkflow and the parity edge count by running them.Reviewer Focus
Whether the new "live suites run in CI now" section is pitched correctly — it is deliberately long because five separate constraints there are each one-mistake-away from silently reintroducing a real defect, but it is the longest single addition to this file and worth a judgement call on length.