Skip to content

Make UK build outcomes honest in staging, telemetry and the Logbook - #1147

Merged
juaristi22 merged 8 commits into
mainfrom
uk-build-outcome-gaps
Oct 9, 2026
Merged

juaristi22 merged 8 commits into
mainfrom
uk-build-outcome-gaps

Conversation

@juaristi22

@juaristi22 juaristi22 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

Originally stacked on #1099 (always-on telemetry emitter), which merged at bd886fc; this now targets main.

Readers must accept staging contract version 3 first: PolicyEngine/calibration-diagnostics#206 has to be deployed and its collector qualified (a blocked event returns 2xx) before this merges. An old collector answers blocked with 422; since review round 1 the emitter treats that as settled and makes the run local-only instead of retrying, so a blocked run would be missing from the hosted view rather than wedging delivery. PolicyEngine/calibration-diagnostics#206 still goes first so no blocked run is lost.

Why

After #1115, a review of how UK builds show up in the calibration dashboard found eight gaps:

  1. Every telemetry failure carried error_code: BUILD_FAILED, and failure_class was never written.
  2. Gate-blocked builds closed their staging run as completed; the dashboard inferred "blocked" from counts.
  3. The spine build never recorded its output H5's sha256.
  4. A graph-built dense candidate could not be assembled: its *.local_gates.json held the graph's unsigned 26-gate document, with no release_id or shippable.
  5. Killed builds stayed running with no Logbook row. There was no SIGTERM handler, and the spine build ignored Ctrl-C.
  6. A build refused at the preflight gates returned 1 with no gate event, so the dashboard showed it as passed.
  7. The Logbook disposition and the telemetry status disagreed on every outcome that wasn't a pass.
  8. Resumed builds weren't linked to the attempts whose store they reused, and reuse wasn't recorded.

What changes (one commit per area)

A. Staging contract v3 and one outcome classification (gaps 1, 2, 6, 7).

  • Staging documents move to schema_version 3. A run can now end blocked, with block {phase, blocking_failure_count, blocking_gate_ids}. Failures gain failure_class.
  • The delivery summary keeps contract_version 2, which publish_cli, both assemblers and the dashboard pin.
  • Version 2 documents stay readable. The v2 fixtures are frozen beside a new v3 set (completed, calibration, failed with a class, blocked).
  • microcosm.build.run_outcome classifies a build's end once, and the staging bundle, the hosted emitter and the Logbook disposition all take it from there:
    • a gate block anywhere in the cause chain is blocked;
    • Ctrl-C is INTERRUPTED, SIGTERM TERMINATED, MemoryError OUT_OF_MEMORY, a graph-node failure GRAPH_NODE_FAILED, and anything else BUILD_FAILED.
  • The Logbook keeps its vocabulary:
    • blocked or failed → failed;
    • interrupted, terminated, or a rung abort → discarded.
  • A dense build refused at the preflight gates now records:
    • a preflight_gates stage;
    • Logbook verdicts whose receipts resolve in the preflight gate document;
    • a blocked run at phase preflight.
  • The hosted emitter gains a dedicated blocked run event and error_code on failures.
  • A block whose details the staging content policy refuses (a gate id that reads as a sensitive key, say) closes both destinations failed with GATE_BLOCK_UNRECORDED, so no run is left running. blocking_gate_ids may be empty when the refusal named no gate (never a placeholder), and a raised refusal's blocked event carries the gate statuses when its report could be read.
  • With Add always-on build telemetry delivery #1099's _run_with_telemetry, each attempt's own close-out closes the hosted emitter with this classification first, so the wrapper's generic complete or fail only runs when nothing closed it. An error raised before the attempt takes over (its preflight digest, say) is classified there too. Add always-on build telemetry delivery #1099's test that expected a gate-blocked dense run to read failed now expects blocked.

B. Termination signals (gap 5).

  • raise_on_sigterm() turns the first SIGTERM into BuildTerminatedError. It is a KeyboardInterrupt subclass, so the existing interrupt arms record it and kernels' except Exception can't swallow it.
  • The spine, dense and national entry points install it. After the close-out they exit 143, so supervisors still see a termination, not a crash.
  • The spine build gains an interrupt arm. A Ctrl-C or SIGTERM closes the run failed with its own class and writes a discarded row.

C. Sign the dense local gate report (gap 4).

  • The driver projects the six local outcomes from the graph's terminal document. Nothing is re-evaluated, and the projection checks that the declarations match.
  • It replays them through the gate battery under MARKS_ARTIFACT, grafts the scoped-report trio, and signs the result with the Logbook build id as release_id.
  • The report is written before any refusal, so a blocked candidate leaves one too. Without the key or an attempt it stays unsigned and is never shippable.
  • The full document keeps its evidence name, uk.full.gates.calibrated.gate_report.json. The package binds it, and it is recorded as outputs.full_gate_report.
  • Gate receipts now resolve: the six local gates point into the signed report, and the rest point into the full document.
  • --release-candidate refuses to start without a 32-byte signing key, or with --target-geographies that select no local targets.
  • A filtered build writes no local report: outputs.local_gate_report is null, with the reason recorded. It never fills the missing local gates in as not applicable.
  • The release preflight now requires exactly 32 bytes of key.
  • The assembler and preflight tests now consume the build's own report. No test signs a report by hand.

D. Spine output checksum (gap 3).

  • The spine's H5 digest is measured after the last write, smoke marking included. It is recorded in:
    • the sidecar (output);
    • a <h5>.sha256 file;
    • the spine_h5_creation stage event;
    • the Logbook pipeline verdict.
  • A full build refuses an input H5 whose digest differs from the sidecar's output.sha256. Older sidecars still bind by content identity.

E. Resume lineage (gap 8).

  • Each attempt's request.json names its Logbook build id and its staging run id.
  • The rowwise manifest gains an execution block, outside the run parameters, so the identity is unchanged. It records:
    • the graph store and the attempt directory;
    • the nodes reused versus computed, where the first run to reach a node decides;
    • the earlier attempts of the same request on the store (most recent first, at most twenty), beside counts of every earlier attempt on the store and of the matching ones.
  • The counts also reach the staging run as a graph_execution stage event.

Review round 1

Two commits on top of the five areas (see the round 1 reply for the item-by-item):

  • block() validates before mutating, and a refused block falls back to failed with GATE_BLOCK_UNRECORDED on both destinations; no placeholder gate ids; gate statuses on raised refusals; classify_return in _run_with_telemetry; interrupts found in the cause chain; earlier_attempts bounded to the same request.
  • The hosted emitter makes a settled collector rejection (a 4xx other than 408 and 429, on events or registration) local-only with one warning instead of retrying it forever.

Known limits

  • After the first SIGTERM, settlement re-hashes the sources and can outlast a supervisor's grace period. A second SIGTERM then kills the process mid-settlement.
  • SIGKILL and out-of-memory kills can't be caught. Add always-on build telemetry delivery #1099's heartbeat and unexpected_process_exit event cover them, and they still write no Logbook row. Area E lets the next attempt on the same store name the killed one.

Testing

  • Unit tests:
    • classify_failure, including the cause chain, wrapped spine blocks and every class;
    • logbook_disposition;
    • the SIGTERM handler: fires once, restores the previous handler, does nothing off the main thread.
  • Both close-outs, tested for:
    • a pass;
    • a terminal block;
    • a preflight refusal;
    • a raised error;
    • Ctrl-C;
    • SIGTERM (exit 143, TERMINATED, discarded) on the spine, dense and national lines.
  • The dense replay:
    • signed with a key, and accepted by _check_uk_dense_gate_report;
    • unsigned without a key, and unsigned without an attempt;
    • a filtered build;
    • a posture mismatch.
  • The graph-built dense candidate passes the release preflight and the assembler on its own signed report.
  • The spine smoke run checks its digest everywhere it is recorded. The checkpoint binding refuses a mismatched digest.
  • Resume tests: a cold attempt reports zero reuse, and a --resume require replay reports reuse and names the cold attempt. Request ids and the stage event are tested too.
  • tools/generate_staging_contract_fixtures.py --check passes, the v2 SHA256SUMS is unchanged, and tools/ci_test_plan.py verify passes.
  • At 0e80b03 (rebased onto main after Add always-on build telemetry delivery #1099 merged): test_run_outcome, test_staging_v2, test_telemetry_emitter, test_termination, test_uk_full_build_cli, test_uk_frs_spine, test_uk_rowwise_national_role and test_uk_calibration_run all pass except two pre-existing spool-migration tests in test_telemetry_emitter (test_pre_eligibility_spool_is_not_uploaded_after_upgrade, test_current_pre_alembic_spool_is_adopted_without_losing_events), which fail the same way on a clean checkout of origin/main on this machine while main's CI at 36557ca is green, so they are a local-environment failure and touch nothing this PR changes.
  • Before the rebase, the UK and shared engine-free suites plus the data publish guard and dense release contract tests ran with 6554 passed; the only failures were environmental (the local venv has the UK extra) or an xdist-only flake that passes serially. The UK staging integration tests pass (2), and every commit imports and lints on its own.

🤖 Generated with Claude Code

Base automatically changed from codex/always-on-telemetry-emitter to main October 8, 2026 20:34
@vahid-ahmadi

Copy link
Copy Markdown
Contributor

Automated review pass (Claude Code, high effort) — round 1 at 52732b23

Verdict: the eight gaps are fixed and each has a test that fails without the fix. Nothing in the code blocks this, but three things should change before it merges, and the merge still waits on calibration-diagnostics#206 being built and deployed. #206 is an open issue today, with no PR yet.

#1099 has merged, so I reviewed the five commits on top of 72a969d4 (#1099's head).

What checks out

  • Each gap has a test that bites. I copied this PR's tests onto 72a969d4. The new cases fail there, one or more per gap:

    • gap 1: a raised error classified;
    • gap 2: a gate block closing as blocked;
    • gap 3: the digest binding and the smoke outputs;
    • gap 4: the dense replay signed or unsigned, and the assembler on a graph-built candidate;
    • gap 5: SIGTERM on dense, national and spine, plus the spine's Ctrl-C;
    • gap 6: a preflight refusal reading as blocked;
    • gap 7: the Logbook envelope;
    • gap 8: an attempt naming itself and reporting reuse.

    The three new shared test modules don't import on the base.

  • At the head:

    • the nine changed test files pass, 355 tests;
    • generate_staging_contract_fixtures.py --check passes;
    • ci_test_plan.py verify passes;
    • ruff check on the changed files is clean.
  • The v3 documents match Add guarded US area artifact release path #206's spec:

    • block has its three keys, and the blocked, failed and other status rules hold;
    • failure has exactly five keys, with failure_class matching ^[a-z][a-z0-9_]*$;
    • the blocked terminal event is stage / blocked / blocked;
    • the delivery summary stays at contract version 2, and v2 documents still validate.
  • One classification feeds everything. run_outcome feeds the staging bundle, the hosted emitter and the Logbook disposition. _run_with_telemetry only closes the emitter when the attempt hasn't, since available turns false on close.

  • SIGTERM handling:

    • the handler fires once, puts SIG_DFL back for the escalation, restores the previous handler on exit, and does nothing off the main thread;
    • BuildTerminatedError subclasses KeyboardInterrupt, so kernels' except Exception can't swallow it;
    • the executor re-raises it unwrapped after settling;
    • all three entry points exit 143.
  • Preflight refusals: the gate receipts resolve, because the staged bundle, uk.full.gates.preflight.gate_report.json included, is published even when the build returns 1.

  • Dense replay: it projects outcomes in the scoped manifest's order and refuses a declaration mismatch or a posture mismatch. It writes the report before any refusal, and with no key or no attempt the report stays unsigned and isn't shippable.

Should

  1. A failed block close leaves the staging run running.

    • Where: StagingRunBundleWriterV2.block() (staging_v2.py:1132) sets self.status = "blocked" before _content_policy.validate_payload(details) (:1147). Its callers then swallow a StagingContractError and stop: _close_blocked (rowwise_staging.py:486) and the spine's _close_failed_telemetry (spine_build.py:934).
    • Effect: if the details are ever rejected, the run is never closed. In memory it reads blocked, which also stops fail() from closing it, while on disk it stays running. That's the symptom gaps 2 and 5 set out to remove.
    • How likely: unlikely today, since phases and gate ids are clean identifiers.
    • Fix: validate before mutating any state. On a contract error, fall back to closing the run failed with its own class, so every path reaches a terminal event. A test that passes an unsafe phase would cover it.
  2. Make the 422 hazard safe in the client rather than relying on deploy order.

    • Where: collector.py:191 defers and retries any non-202 status other than 401 and 403, so an old collector's 422 on the new blocked event wedges that run's queue for good.
    • Fix: treat 422, and other 4xx apart from 408 and 429, as permanent for the run, the way 403 is (collector.py:185). Make it local-only with a reason, and warn once.
    • Why: this PR then stops depending on Add guarded US area artifact release path #206's deploy order, and the next contract change can't wedge delivery either.
  3. Two codes aren't in Add guarded US area artifact release path #206's list. Add guarded US area artifact release path #206 lists INTERRUPTED, TERMINATED, OUT_OF_MEMORY, GRAPH_NODE_FAILED and BUILD_FAILED, but this PR also writes:

    • BUILD_REFUSED / refused (run_outcome.py:148, :169);
    • RUNG_ABORTED / aborted (spine_build.py:950).

    Add both to Add guarded US area artifact release path #206 so the dashboard and stop statistics label them.

    Related: _run_with_telemetry's non-zero-return path (full_build_cli.py:2672) still sends failure_class="build_failure" with no error_code. classify_return would keep one vocabulary.

Nits

  1. unidentified_gate placeholder. GateBlock fills in unidentified_gate (run_outcome.py:122, :132; full_build_cli.py:1561, :2373), which the dashboard will show as if it were a gate. Either name that placeholder in Add guarded US area artifact release path #206, or allow an empty id list alongside the count. Separately, _BLOCK_SCHEMA.blocking_gate_ids (staging_v2.py:326) has no minItems: 1, though the writer requires one.
  2. gate_statuses is optional in practice. A raised block passes gate_statuses=None (rowwise_staging.py:430), so its terminal event has no gate_statuses, but Add guarded US area artifact release path #206 lists that key as part of details. Either say it's optional in Add guarded US area artifact release path #206 or send {}.
  3. earlier_attempts isn't bounded. It lists every sibling attempt directory on the store (full_build_cli.py:299-326), whatever its request. On a long-lived store that grows without limit. Cap it, or keep only attempts whose request bindings match.
  4. Interrupts and terminations are found differently. classify_failure checks for KeyboardInterrupt on the top-level error only (run_outcome.py:145), while TERMINATED walks the cause chain. Nothing wraps interrupts today, but walking the chain for both is cheap and consistent.

Before merge: #206 has to be built and deployed, and a blocked event has to return 2xx from the qualification collector. Finding 2 would remove that ordering risk. Then retarget and let CI run, since this branch has had none.

juaristi22 and others added 7 commits October 9, 2026 11:25
…ng contract v3)

A build whose gates refuse its candidate used to close its staging run as
`completed`, and a build refused at the preflight gates read as a pass on the
dashboard. Every failure carried `BUILD_FAILED`, and the Logbook and the
telemetry could disagree on how a run ended.

- Staging documents move to schema version 3: a `blocked` run status with a
  `block` record (gate phase, blocking gate ids, count) and a `failure_class`
  on failures. The delivery summary keeps contract version 2, which
  publication and the assemblers pin. Version 2 documents stay readable; the
  v2 fixtures are frozen beside a new v3 set (adds a blocked run).
- `microcosm.build.run_outcome` classifies a build's end once: a gate block
  anywhere in the cause chain is `blocked`; Ctrl-C, out-of-memory, graph-node
  and other errors get distinct codes and classes; the Logbook disposition is
  derived from the same classification.
- Both UK close-outs close the run `blocked` (terminal battery, national seam
  battery) or `completed` from that classification; the preflight refusal now
  records its gates, a `preflight_gates` stage, Logbook verdicts that resolve
  in the preflight gate document, and a `blocked` run at phase `preflight`.
- The spine build closes gate blocks as `blocked` and classifies other errors.
- The hosted emitter gains a `blocked` run event and `error_code` on failures.

Readers must accept version 3 first: PolicyEngine/calibration-diagnostics#206.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A supervisor's SIGTERM killed a UK build without running its failure path:
the staging run stayed `running` and no Logbook row was written. The spine
build also let Ctrl-C escape without closing its run.

- `microcosm.build.termination.raise_on_sigterm` turns the first SIGTERM
  into `BuildTerminatedError`, a `KeyboardInterrupt` subclass, so the
  existing interrupt arms record it and kernels' `except Exception` cannot
  swallow it. The handler fires once and then restores the default, so a
  second SIGTERM still kills the process at once.
- The spine, dense and national entry points install it and, after the
  close-out, exit with status 143, so supervisors still see a termination.
- The spine build gains an interrupt arm: the run closes `failed`
  (`INTERRUPTED` or `TERMINATED`) and the attempt records a `discarded` row.

Known limits: settlement after the first signal can outlast a supervisor's
grace period; SIGKILL and out-of-memory kills are covered only by the hosted
emitter's heartbeat and write no Logbook row.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A graph-built dense candidate could not be assembled: its
`*.local_gates.json` held the graph's unsigned 26-gate document, with no
`release_id` and no `shippable`, so the dense contract, the release
preflight and the size evaluation all failed to read it.

- `replay_uk_dense_gate_battery` projects the six local outcomes from the
  graph's terminal document (nothing is re-evaluated; the projection checks
  the declarations match), replays them through the gate battery under
  `MARKS_ARTIFACT`, grafts the scoped-report trio and signs the report with
  the Logbook build id as `release_id`. Without the key or an attempt it is
  written unsigned (`signing_error`) and is never shippable.
- The dense build writes it before any refusal, so a blocked candidate
  leaves it too; the graph's own enforcement still sets the exit status.
- The full document keeps its evidence name
  (`uk.full.gates.calibrated.gate_report.json`), is what the package binds,
  and is recorded as `outputs.full_gate_report`. Gate receipts now resolve:
  local gates in the signed report, the rest in the full document.
- A build whose `--target-geographies` select no local targets writes no
  local report (`outputs.local_gate_report: null`, with the reason).
- `--release-candidate` refuses to start without a 32-byte signing key or
  with a filter that drops the local targets; the preflight requires
  exactly 32 bytes.
- The assembler and preflight tests now feed the build's own report; no
  test signs a report by hand.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The spine build never recorded its output file's digest, so nothing tied
a downstream build to the exact H5 a spine run wrote.

- The digest is measured once the H5 is fully written (smoke marking
  included) and recorded in the sidecar (`output {filename, sha256,
  size_bytes}`), a `<h5>.sha256` file in `sha256sum` format, the
  `spine_h5_creation` stage event and the Logbook `pipeline` verdict
  (`artifact_sha256`).
- `load_bound_spine_checkpoint` refuses an input H5 whose measured digest
  differs from the sidecar's `output.sha256`; older sidecars without the
  key still bind by content identity. Both full-build roles pass the
  measured digest.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A resumed build reused stored graph results from earlier attempts, but
nothing linked it to them or said how much it reused.

- Each attempt's `request.json` now names its Logbook build id and staging
  run id.
- Both roles' rowwise manifests gain an `execution` block: the graph store,
  the attempt directory, the nodes reused from the store versus computed
  (the first graph run to reach a node decides; later runs in the same
  attempt see this attempt's own results as hits), and the earlier attempt
  directories on the same store with their ids. It sits outside the run
  parameters, so the candidate identity is unchanged.
- The counts reach the staging run as a `graph_execution` stage event before
  it closes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d the lineage

Review round 1 on #1147 (findings 1, 3 to 7):

- `StagingRunBundleWriterV2.block()` runs every check, the content policy
  included, before it changes any state, so a refused block leaves the run
  `running`. One `close_run_blocked` (shared by the rowwise and spine lines)
  then closes the staging run and the hosted emitter `failed` with
  `GATE_BLOCK_UNRECORDED` / `unrecorded_gate_block`, so no path leaves a run
  `running` and the two destinations cannot disagree.
- `blocking_gate_ids` may be empty when the refusal named no gate; the count
  stays at least 1. No code writes `unidentified_gate` any more.
- A raised refusal carries the gate statuses from the report the battery
  wrote beside its block, or from a spine refusal's phase report, so its
  `blocked` event lists them when the report could be read.
- `_run_with_telemetry` classifies a non-zero return with `classify_return`
  (`BUILD_REFUSED` / `refused`), the attempt close-outs' vocabulary.
- Interrupts are found anywhere in the cause chain, like terminations.
- `execution.earlier_attempts` lists only attempts of the same request, most
  recent first, at most twenty, beside counts of every earlier attempt on
  the store and of the matching ones.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…forever

Review round 1 on #1147 (finding 2): an old collector answers a `blocked`
event with 422, and the delivery queue retried any non-202 status other
than 401 and 403 indefinitely, wedging that run's queue for good.

A 4xx other than a timeout (408) or a rate limit (429) on a run's events
now makes the run local-only (`collector_rejected_events`) with one warning
that names the status; the same rule covers a rejected registration. 401
still refreshes the token; 408 and 429 still retry.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@juaristi22
juaristi22 force-pushed the uk-build-outcome-gaps branch from 52732b2 to 0e80b03 Compare October 9, 2026 10:40
@juaristi22

Copy link
Copy Markdown
Collaborator Author

Round 1 addressed at 0e80b03 (three commits on top of the five areas, so the delta reviews on its own). #1099 merged at bd886fc, so the branch is rebased onto main and the PR targets main; CI is running.

Should

  1. A refused block no longer leaves the run running. StagingRunBundleWriterV2.block() now runs every check, the content policy included, before it changes any state, so a refusal leaves the run running. Both close-outs then fall back the same way: the rowwise line's close_run_blocked (which the spine's _close_failed_telemetry now shares instead of its own copy) closes the staging run and the hosted emitter failed with GATE_BLOCK_UNRECORDED / unrecorded_gate_block, so the two destinations cannot disagree. Tests: a block with a gate id the content policy rejects (token) leaves the writer running and still able to fail(); close_run_blocked on that block closes both destinations failed, and on a recordable block closes both blocked with the gate statuses.
  2. Settled collector rejections are permanent for the run. On a run's events, any 4xx other than 408 and 429 now makes the run local-only (collector_rejected_events) with one warning that names the status; 401 still refreshes the token, 408 and 429 still retry. The same rule applies to a rejected registration (run_registration_rejected), which would wedge the queue the same way. Test parametrised over 422, 404 (local-only, one warning) and 429, 408 (still pending). This softens the deploy-order constraint: an old collector now leaves a blocked run out of the hosted view rather than wedging delivery. Read UK staging contract v3: a real blocked run status, block details and failure classes (dashboard and collector) calibration-diagnostics#206 still goes first so no blocked run is lost.
  3. One vocabulary for a non-zero return. _run_with_telemetry now classifies it with classify_return (BUILD_REFUSED / refused, with the error_code), the same as the attempt's own close-outs; the lifecycle test checks it. The three codes Read UK staging contract v3: a real blocked run status, block details and failure classes (dashboard and collector) calibration-diagnostics#206 did not list (BUILD_REFUSED, RUNG_ABORTED, and the new GATE_BLOCK_UNRECORDED) go to Read UK staging contract v3: a real blocked run status, block details and failure classes (dashboard and collector) calibration-diagnostics#206 with the two contract notes below.

Nits

  1. No placeholder gate. blocking_gate_ids may now be empty when the refusal named no gate; blocking_failure_count stays ≥ 1 (the writer refuses an empty list without a count). The schema keeps no minItems on purpose; the docs and Read UK staging contract v3: a real blocked run status, block details and failure classes (dashboard and collector) calibration-diagnostics#206 say so. The two unidentified_gate sites in the driver are gone (the preflight one was unreachable: artifact_permitted is false only with a non-empty enforced_blocking).
  2. gate_statuses on a raised block. A GateBatteryBlockedError now brings the statuses from the report the battery wrote beside its block (report_path), and a SpineGateBlockedError from its in-memory phase report; close_run_blocked uses them when the caller holds no report. It is absent only when the report could not be read, which Read UK staging contract v3: a real blocked run status, block details and failure classes (dashboard and collector) calibration-diagnostics#206 will state as optional.
  3. earlier_attempts is bounded. It now lists only attempts of the same request (equal bindings), most recent first, at most twenty, beside two counts: every earlier attempt on the store, and the matching ones. The replay test plants an unrelated attempt and checks it is counted, not listed.
  4. Interrupts walk the cause chain like terminations; a KeyboardInterrupt wrapped in a cleanup error is INTERRUPTED.

Verification at 0e80b03: the four shared suites (test_run_outcome, test_staging_v2, test_telemetry_emitter, test_termination) and the four UK suites (test_uk_full_build_cli, test_uk_frs_spine, test_uk_rowwise_national_role, test_uk_calibration_run) — all pass except two pre-existing spool-migration tests in test_telemetry_emitter (test_pre_eligibility_spool_is_not_uploaded_after_upgrade, test_current_pre_alembic_spool_is_adopted_without_losing_events), which fail the same way on a clean checkout of origin/main on this machine while main's CI at 36557ca is green, so they are a local-environment failure and touch nothing this PR changes; generate_staging_contract_fixtures.py --check and ci_test_plan.py verify pass.

@vahid-ahmadi

Copy link
Copy Markdown
Contributor

Automated review pass (Claude Code, high effort) — round 2 at 0e80b036

Verdict: round 1 is fully addressed, and the engine-free CI failure isn't this PR. It's a date-dependent test on main that started failing at 10:00 today. Good to merge once that test is fixed on main (or here) and calibration-diagnostics#206 is deployed.

Round 1

# Item Status
1 Failed block close left the run running Fixed. block() validates everything before changing state (staging_v2.py:1136-1150), and close_run_blocked falls back to fail() with GATE_BLOCK_UNRECORDED on both destinations. The spine now shares it.
2 422 retried forever Fixed. Any 4xx except 408/429 makes the run local-only, for events and registration (collector.py:125-137, :204-209, :267-270), with one warning naming the status.
3 Missing codes / no error_code on a non-zero return Fixed here: classify_return gives BUILD_REFUSED / refused. The three new codes are handed to calibration-diagnostics#206.
4 unidentified_gate placeholder Fixed. Empty ids are allowed with a count ≥ 1. No minItems is deliberate and documented.
5 gate_statuses on a raised block Fixed, read from the battery's written report or the spine's in-memory report.
6 earlier_attempts unbounded Fixed: same-request attempts only, newest first, at most 20, plus two counts.
7 KeyboardInterrupt only checked at the top Fixed, it walks the cause chain (run_outcome.py:199-201).

Each fix has a test that fails without it. I copied the new test files onto d182a1a2 (before the round-1 commits) and they fail there:

  • the refused-block tests in test_staging_v2;
  • the 422/404 cases in test_telemetry_emitter;
  • the lifecycle, replay and unrecorded-block tests in test_uk_full_build_cli.

At the head, the six affected suites pass apart from the two tests below, and ruff check is clean.

The CI failure

Both engine-free jobs fail on one test, test_telemetry_emitter.py::test_pre_eligibility_spool_is_not_uploaded_after_upgrade (assert spool.has_pending()). Locally, test_current_pre_alembic_spool_is_adopted_without_losing_events fails the same way.

  • The cause is the fixtures. They write events with a fixed created_at of 2026-10-02T10:00:00+00:00 (test_telemetry_emitter.py:276, and the pre-Alembic test). EventSpool.__init__ calls prune(), which deletes events older than RETENTION_DAYS = 7 (spool.py:216). From 2026-10-09 10:00 UTC the fixture events are pruned on open, so has_pending() is false.
  • main is affected too. Both tests fail at main's head a469ebe3. main's last green run (36557ca) started at 09:11 today, before the cutoff, and its later runs are still in progress.
  • The PR's emitter changes don't touch spool.py.

Fix (on main, or carried here): date the fixture events relative to now, e.g. (datetime.now(UTC) - timedelta(hours=1)).isoformat(), for both the event and run rows. Optionally add a test that an event older than RETENTION_DAYS is pruned on open, so the retention behaviour is pinned on purpose.

New

  1. Nit: a 404 on run events now makes the run local-only (collector.py:204). That's right for a settled route, but during a collector deploy a briefly missing route would drop the run's hosted view for good. Fine to keep given the deploy order, but worth one line in docs/uk-staging-operations.md next to the 422 note.

@juaristi22
juaristi22 merged commit a224dc1 into main Oct 9, 2026
10 checks passed
MaxGhenis added a commit that referenced this pull request Oct 9, 2026
Resolves the collector.py conflict between #1168 (warnings through
write_warning) and #1147 (a per-reason rejection message): the rejection
message now goes through write_warning.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants