Record every UK build attempt in Logbook (#666) - #670
Conversation
The three UK drivers now append exactly one validated, hash-chained Logbook row per non-dry invocation, so discarded intermediate builds stay visible before the P1 calibration work starts generating them. Rows spool beside the build outputs; receipts live under build-scoped logbook-receipts/<build_id>/. Successes record iterating, gate-battery blocks and calibrated-candidate refusals record failed, unexpected exceptions record failed after a pipeline-error receipt, and the named dev-rung SPI singleton abort records discarded while keeping its existing uk_rung_abort_receipt payload. Dry runs are excluded on both success and failure paths. Exit codes, exception types, and stdout payloads are unchanged — the spool notice goes to stderr. Terminal verdicts anchor into durable local receipts rather than host-absolute paths: the national rows carry the schema-4 battery statuses uncollapsed, rung aborts point at the named-edge receipt, and the rowwise and candidate rows point into their manifests. The national pin surface hoists the source digests out of the checkpoint-only path so failure rows carry real input pins, and names the future ledger_facts slot for #622/#623 without inventing a placeholder. Adds microcosm.build.logbook_adoption, a driver-agnostic extraction of the generic US stacked-driver helpers (attempt state, predecessor resolution, git code pins, exportable artifact references, normalized role-pin digests, atomic receipts, record-once terminal write). The US driver keeps its local copy here because its contract suite pins those seams; migrating it is the follow-up. Repoints the Logbook docstring from the merged #616 at #665/#666. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…te setup failures Two findings from the #666 adversarial review, both about failure-path ordering and observability: Chain configuration now resolves before any side effect in all three UK drivers. The national driver used to resolve --logbook-prev-row-digest / POPULACE_LOGBOOK_PREV_ROW_DIGEST after unlinking the prior attempt's evidence, replay, build-record, and rung-abort sidecars, so a malformed or conflicting head destroyed local evidence and then crashed; the rowwise dataset driver had the same ordering against its stale coverage/crosswalk unlinks. A config refusal now aborts with no row, no receipt, and untouched sidecars — the same posture as an argparse refusal — and every recorded row carries the resolved head rather than the raw CLI value, so a failure row can no longer chain to the wrong predecessor. The candidate driver's recording envelope now opens before input verification. Input hashing, frame and ladder loading, cloning, and target binding used to run before the try that records failed attempts, so their failures escaped with no Logbook row despite being real terminal outcomes. main() now seeds the attempt state right after CLI validation and routes dry-runs past recording explicitly; pins promote as soon as both inputs are hashed, and the identity digest waits only on the frame-derived source year. Both regression tests were verified to fail against the unfixed drivers: a malformed env head must leave pre-existing national sidecars intact and spool nothing, and a ladder-load failure must spool exactly one failed candidate row with real input pins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
An adversarial review (Codex, run locally) surfaced two failure-path findings; both are fixed in 206ff6e. [high] Invalid Logbook predecessor could erase national sidecars before aborting. The national driver resolved [medium] Rowwise candidate setup failures escaped unrecorded. Input hashing, frame/ladder loading, cloning, and target binding ran before the candidate's recording envelope opened, so those failures spooled no row — contradicting the one-row-per-non-dry-invocation contract. The envelope now opens right after CLI validation; pins promote as soon as both inputs hash, the identity digest waits only on the frame-derived source year, and dry-runs are routed past recording explicitly. Both regression tests were verified to fail against the unfixed drivers before the fix landed:
Full driver + adoption suites (172), logbook contract/backfill/archive/CLI + untouched US driver suites (204), and ruff all pass locally after the fix. 🤖 Generated with Claude Code |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
The exception structure is the part I most expected to be subtly wrong, and it isn't. I verified the two claims rather than taking them:
GateBatteryBlockedError(RuntimeError)— confirmed atgate_battery.py:718. So the earlierexcept ValueErrorarm genuinely cannot shadow it, and arm order between the two is irrelevant.- Both
ValueErrorsub-paths record exactly once: the named-edge rung abort recordsdiscardedthen returns, the unrecognized case records via_record_failed_exceptionthen re-raises. A re-raise can't fall through to a later arm on the sametry, so sibling arms are the right shape andlen(rows) == 1holds on every path.
Seeding a valid placeholder input_pins_digest/identity_digest before anything can fail is what makes preflight failures recordable at all — that's the detail that makes "discarded intermediate builds stay visible" true rather than aspirational. Hoisting the source digests out of the checkpoint-only path so failure rows carry real input pins is the same instinct.
Three things.
1. local_artifact_reference can still emit a host-absolute path.
The docstring says "without host-absolute paths" and the PR body says "never host-absolute paths (rows are exported publicly)", but the final fallback is:
return f"local://{resolved.as_posix().lstrip('/')}"which is the absolute path with the leading slash removed. The repo-root and home branches cover a dev laptop and a CI runner. A build machine writing to a dedicated mount — /mnt/scratch/uk-builds/..., /data/populace/... — falls through to the third branch and exports local://mnt/scratch/uk-builds/..., which is exactly the host layout the contract says it won't carry. And that machine is the one producing the rows worth exporting.
Either the fallback should refuse (a reference outside both roots is arguably a configuration error for an exportable row), or the docstring and changelog should say "repo- and home-relative where possible" instead of "never".
2. _json_ready silently drops None-valued keys from receipts.
if isinstance(value, Mapping):
return {
_json_ready(key): _json_ready(item)
for key, item in value.items()
if item is not None
}write_error_receipt puts gate_verdicts through this, so a verdict carrying an explicit null loses the key entirely — and in an immutable audit receipt, "explicitly null" and "never set" stop being distinguishable. For compaction in a general JSON writer that's fine; for the artifact that is the build-attempt memory, it quietly erases a distinction someone reading a failure six months later would want.
Worth either keeping nulls in receipts or saying in the docstring that null-valued keys are dropped by design, so the next person doesn't read an absent key as absent evidence.
3. Two copies of the adoption helpers, and the shared one is the untested copy.
logbook_adoption.py is a clean extraction, but the US driver keeps its local copy because "its contract suite pins those seams by monkeypatch". So the same logic now exists twice, and the copy with the contract suite holding it in place is the one that isn't shared. A fix or hardening applied to the UK path won't reach the US path, and vice versa — with the drift running in the direction of the untested module.
Naming it as a follow-up is right, and I'm not asking you to migrate the US driver here. But the follow-up is load-bearing rather than tidying, and it'd be worth an issue with that framing rather than a sentence in this PR body.
On the two judgment calls you flagged
Rung abort as discarded rather than failed reads correct to me — a named-edge abort is a deliberate non-result, not a failure, and the receipt keeps its own aborted_with_receipt disposition regardless.
Dry runs recording nothing on success is obviously right. On failure I'd think about it once more: a dry run that fails is a real signal about configuration, and it's the cheapest possible build attempt to have a record of. Not a strong view — a dry run isn't a build attempt, so excluding it is defensible — but of the two calls it's the one where I could imagine wanting the row later.
…filed Three findings from the human review of PR #670: _json_ready now matches the US driver's writer: explicit nulls survive into receipts and non-string mapping keys are rejected. The extraction had added a silent None-drop, which made "explicitly null" indistinguishable from "never set" in immutable audit receipts — exactly the drift the shared module exists to prevent, so the divergence is also cited as evidence in the migration issue. local_artifact_reference's docstring (and the changelog) now state the real contract: checkout- or home-relative where possible, with paths under neither root falling back to the absolute path minus its leading slash. The fallback stays a fallback rather than a refusal because recording must never turn a completed build into a failure over reference formatting; the "never host-absolute" claim was inherited from the US original's docstring and was wrong there too (left for #689, which reconciles the copies). The US-driver migration follow-up is now tracked as #689 with load-bearing framing: two copies of the adoption helpers exist, the deep contract suite pins the unshared one, and the drift direction is toward the shared module. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks @vahid-ahmadi — all three addressed in df5b8fa, and your verification of the exception structure is appreciated. Point by point: 1. 2. 3. Follow-up as an issue. Filed as #689 with the load-bearing framing: two copies, the deep contract suite pinning the unshared one, drift running toward the shared module — with the 4. Dry-run failures. Keeping them unrecorded for now, with your point noted as real. The tie-breakers: recording dry-run failures would make broken-config dry-runs require valid chain configuration ( 🤖 Generated with Claude Code |
vahid-ahmadi
left a comment
There was a problem hiding this comment.
df5b8fa6 closes all three. Verified:
_json_readyno longer drops null-valued keys, and now refuses non-string mapping keys — matching the US writer rather than diverging from it. The regression test asserting an explicitly-nullreasonsurvives into the receipt is the right pin.- The reference ladder is stated honestly now (checkout → home → absolute-minus-leading-slash), with the operator guidance to keep exportable outputs under the checkout or home.
- #689 filed with the load-bearing framing.
On the fallback: I agree with the call, and your reason is better than my suggestion. Refusing would turn a completed build into a failure over reference formatting, on exactly the dedicated-mount host most likely to be doing real work — that trade is clearly wrong, and the honest docstring is the right resolution. Good catch that the username-leak case is the one already covered and the residual is non-home server layout.
The _json_ready provenance is the part worth recording: not inherited, introduced during extraction, in the direction of the shared copy. That's the two-copy risk producing a concrete divergence within days of the split, which makes #689 an argument from evidence rather than from principle. Worth keeping that sentence in the issue.
On dry-run failures — your tie-breaker settles it for me. Requiring valid chain configuration in exactly the cheap planning loops dry-run exists for would defeat the point, and the failure signal genuinely does land in front of an operator running an interactive command. I don't hold the view more strongly than that; leave it.
Nothing further — clear from my side.
Re-applies the CGT family wiring onto the Logbook-era driver: the stage still runs after hmrc_spi_income, _source_pins gains cgt_ods_path at both call sites, and the checkpointed run config records the cgt_ods pin. Driver tests create the CGT ODS wherever main's eager source hashing now requires it, and the new logbook-threading test passes --cgt-ods.
Closes #666. Workstream A of the UK-migration master epic #665 — this lands before P1 calibration starts generating intermediate builds, since Logbook is the migration's build-attempt memory.
What changed
#628 delivered the append-only Logbook, but adoption is per-tool and only
tools/build_us_multispine_pool.pycalled therecord_build_attemptseam. The three UK drivers now append exactly one validated, hash-chained row per non-dry invocation, so discarded intermediate builds stay visible. Rows spool beside the build outputs; receipts live under build-scopedlogbook-receipts/<build_id>/.Dispositions
iteratingfaileddiscardedThe rung abort keeps its existing
uk_rung_abort_receiptpayload, including its owndisposition: "aborted_with_receipt"field; only the Logbook row saysdiscarded. Exit codes, exception types, and stdout payloads are unchanged — the spool notice goes to stderr.Verdicts and pins.
gate_verdictsanchor into durable local receipts, referenced checkout- or home-relative where possible (rows are exported publicly; paths under neither root fall back to the absolute path without its leading slash — see thelocal_artifact_referencedocstring). The national rows carry the schema-4 battery statuses uncollapsed (all five ofpassed/failed/not_applicable/evidence_absent/unreached), read fromerror.report_pathon a block; rung aborts point at the named-edge receipt; rowwise and candidate rows point into their manifests. The national pin surface hoists the source digests out of the checkpoint-only path so failure rows carry real input pins, and names the futureledger_factsslot for #622/#623 without inventing a placeholder.New module.
microcosm.build.logbook_adoptionis a driver-agnostic extraction of the generic US helpers: attempt state, predecessor resolution, git code pins, exportable artifact references, normalized role-pin digests, atomic receipts, and a record-once terminal write. The US driver deliberately keeps its local copy in this PR — its contract suite pins those seams by monkeypatch — so migrating it is load-bearing convergence work tracked as #689. Also repoints the Logbook docstring from the merged #616 at #665/#666.Design notes for review
record_build_attemptcall in one run produces a different row (wall_seconds differs), and two rows chaining the same predecessor make a fork the archive refuses. The national driver's handlers are therefore siblingexceptarms — each records once and then returns or re-raises.GateBatteryBlockedErrorsubclassesRuntimeError, so theexcept ValueErrorarm cannot shadow it; the unrecognized-ValueErrorcase records inline before re-raising, since a re-raise never falls through to a later arm on the sametry. Every driver test assertslen(rows) == 1.input_pins_digestandidentity_digestare required 64-hex, so a placeholder preflight digest is seeded before anything can fail;code_pinkeeps theunresolved-local-git-code-pinsentinel until the real pin resolves. That is what lets a preflight failure still record a valid row.discardedrather thanfailed.Verification
test_logbook_adoption.py+ the four UK driver suites: 169 passedruff check .clean; all 10 changed filesruff formatclean. Full-reporuff format --checkshows 29 pre-existing unrelated diffs, confirmed identical at the base commit and left alone.validates as one 29-row chain, andrenders in the same table beside the US rows —uk-national-staging | f001 | iterating.🤖 Generated with Claude Code