Repository navigation
feat(bep): give bep serve a chain-preserving way out of an unreadable audit line (T75) - #2608
Conversation
bep serve --quarantine-unreadable moves each audit segment that holds a line bep cannot read into <audit dir>/quarantine/ and starts. For each moved segment the log gets a quarantine record that carries the hash the chain runs from and the segment's final hash. verify_chain bridges the gap with it, so the chain still verifies. The revocations read in a moved segment are appended again, so they stay in force. A record of the schema with a kind this bep does not know is now told apart from a corrupt line. It no longer refuses the start. The refusal names the flag in place of a hand edit.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Approved: every review thread resolved at @norrietaylor resolved the last thread, which releases the approval this review had withheld. Nothing to do. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The quarantine keeps the chain verifiable and keeps readable revocations in force. The default still refuses, and the code is in the helper's crate. Two items remain. The quarantine appends each restated revocation as a plain revocation record, so the log shows a second destroy event. Gatehouse §6.10 line 839 says the destroy is the mode's one revocation event. C1 adds a quarantine_restated marker to that record. Spec 24 BEP-041 forbids this removal, and the spec does not state the quarantine. C2 amends the spec in this PR.
Ruled at a8fe4bf against gominimal/arch@109541a48c31 by claude-opus-5-5.
quarantine_unreadable now plans every move without changing the log, then writes each segment's quarantine record and revocations before it renames that segment. A failure part wa 8000 y no longer leaves a moved segment with no record, so its revocations stay in force.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Revise round 1. quarantine_unreadable now runs in two phases. The first phase reads every segment, builds the records and the revocation lists, and checks each quarantine target. It changes nothing on disk. The second phase opens the next segment if the active segment is bad. Then, for each segment, it writes the quarantine record and the revocations, and after that it renames the segment. If a failure or a crash happens between the write and the rename, the segment stays in place and the next quarantine moves it again. In that case the log holds a duplicate quarantine record with the same hashes. verify_segments matches gaps by from_hash, so a duplicate does not break verification. A duplicate revocation does no harm. clippy -p bep --all-targets is clean. |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The change conforms to Gatehouse §6.10 lines 836 and 839. The compare fixes the order from the last head. Each segment's quarantine record and its revocations now go to the log before the segment moves. So a failure or a crash cannot leave a moved segment without its revocations. Without the flag, bep still refuses the start. With the flag, every readable revocation stays in the retained log, and the chain verifies across the gap.
Ruled at 7dbfcf7 against gominimal/arch@109541a48c31 by claude-opus-5-5.
A quarantine's restated revocation carries the quarantine_restated marker, so it does not read as a second destroy. Prune keeps a segment with a record of a kind this bep does not know, since it can be a newer bep's revocation. The quarantine warn line says what is lost. Refs T75
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Round 2. I also ran |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare from 7dbfcf7 conforms to Gatehouse §6.10 line 839. A restated revocation now carries the quarantine_restated marker, so the destroy stays the one revocation event. Restore at start still reads the restated record, because it matches on kind only. Prune now keeps a segment with an unknown kind until the ceiling. This is the fail-closed reading of the compaction rule. The earlier PASS stands for the rest of the change.
Ruled at 4259c1d against gominimal/arch@109541a48c31 by claude-opus-5-5.
A line of a kind this bep does not know can hold a newer bep's revocation. The quarantine cannot record it again, so it now counts those lines per moved segment and names the count in its warn line.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Round 3: one thread. The quarantine now counts Line::OtherKind lines per moved segment in a new Quarantined::other_kinds field and names the count in the warn line. The QuarantineRecord schema did not change. The test quarantine_of_the_active_segment_opens_the_next now writes an unknown-kind line into the moved segment and asserts the count. I also ran |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The compare counts the lines of an unknown kind in each quarantined segment. The warn line reports the count as other_kinds. Section 6.10 line 839 says a revocation stands, and the move cannot restate a line whose kind it does not know. The warn line now states that loss, as it states the loss on a corrupt line. The chain and the quarantine record do not change. BEP-068 conforms, so the PASS on 4259c1d1 stands.
Ruled at bd17d34 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 5 unresolved threads — minimal-foundry[bot] crates/bep/src/audit.rs, minimal-arch[bot] crates/bep/src/audit.rs, minimal-arch[bot] crates/bep/src/audit.rs:761, minimal-foundry[bot] crates/bep/src/audit.rs:341, minimal-foundry[bot] crates/bep/src/audit.rs:248. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of bd17d34276d0 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
There was a problem hiding this comment.
Own review at bd17d34. --quarantine-unreadable plans before it writes. It re-records each moved segment's revocations with the quarantine_restated marker and a quarantine record before the rename, and keeps the chain. Prune keeps a segment that holds a corrupt or unknown-kind line. The warn line counts the lines that cannot be restated. The spec gap (BEP-041, BEP-068) is filed as #2613.
Generated by Claude Code
There was a problem hiding this comment.
Babysitter approval at bd17d34: every merge gate passes at this head (arch PASS at bd17d34, zero open threads, every lane aggregator green). Squash title feat(bep): give bep serve a chain-preserving way out of an unreadable audit line (T75) and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
|
/revise Round on #2608 after a merge-queue failure at bd17d34. No round is in flight. #2596 (T68) landed on main and gave
Generated by Claude Code |
…erve-a-chain-preserving-way-out-of-an
T68 added the jti field to audit::Decision. The quarantine's restated revocation and the quarantine test did not set it.
|
Round 4 of 10 on a /revise note, pushed as
Verify
Notes from the agent: The merge of origin/main (with #2596 T68 and the zstd bump #2627) was already on the branch as bae8cd7, with no conflict markers. This round added one commit. It sets |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The ruling on bd17d34 stands. This head merges main and sets an empty jti on the restated revocation in quarantine_unreadable. That is the value main gives a revocation in control.rs. Older audit lines still parse, because Record::jti defaults to empty. The record shape of Gatehouse §6.10 line 836 and the revocation rule of line 839 still hold.
Ruled at 798c9d2 against gominimal/arch@109541a48c31 by claude-opus-5-5.
verify_chain now accepts a quarantine record across a gap only before the first line of a segment, after the gap, for a segment numbered between the two, and when quarantine/ holds that segment and it agrees with the record.
…erve-a-chain-preserving-way-out-of-an
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
Notes from the agent: Revise round 5. verify_segments now takes a second argument: the segments under quarantine/, by file name. verify_chain reads them. A bridge is accepted only at the first line of a segment, from a record at or after the gap, for a moved segment numbered between the two retained segments. The moved file must hold exactly |
|
The runtime will not revise this pull request again on its own: the diff is +842/-51 (893 lines), past 3x the plan's estimate of 260 lines (est: 120 code, 140 tests, as #2588 reads now), at round 5 (the last trigger was the runtime's review findings). A person decides now. Remove |
There was a problem hiding this comment.
arch-review: PASS · the full ruling
The new commit makes the audit chain check stricter. A quarantine record now bridges a gap only when its moved segment exists under quarantine/ and agrees with the record. This fails closed, and §6.10 line 836 asks for a whole-log chain check. The quarantine writer still produces records that verify. The ruling on 798c9d2 stands for the rest, and residual #2613 stays open.
Ruled at 667d71c against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 1 of 10 on a failed check suite: the coder changed nothing. every commit this round put on the branch above 667d71c is a merge of the base branch, so it carries no work of its own |
There was a problem hiding this comment.
Own review at 667d71c. The merge of main sets jti on the restated revocation, which fixes the E0063 the queue hit. f637f6a9 tightens verify_chain: a quarantine record bridges a gap only before the first line of a segment, for a segment numbered between the two, and only when quarantine/ holds that segment and it agrees with the record. That fails closed, and arch passes it. The epic owner ruled the security threads on 2026-10-26. Spec residual #2613.
Generated by Claude Code
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of 667d71c0ebf1 withheld this approval on the threads open then. Posted as a comment: the App cannot approve its own pull request; the check and the label carry it.
There was a problem hiding this comment.
Babysitter approval at 667d71c: every merge gate passes at this head (arch PASS at 667d71c, zero open threads, every lane aggregator green). Squash title feat(bep): give bep serve a chain-preserving way out of an unreadable audit line (T75) and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
T75 — Give bep serve a chain-preserving way out of an unreadable audit line
slice: S10
kind: task
covers: BEP-068
size: M
est: 120 code, 140 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: the refusal names
bep serve --quarantine-unreadablein place of a hand editobservability: one warn line per quarantined segment, and a
quarantineaudit recordlayers:
- recover:
- crates/bep/src/audit.rs: a quarantine moves each segment holding an unreadable line into
<audit dir>/quarantine/and appends aquarantinerecord that carries the moved segment's final hash, so the chain stays verifiable; a record of a kind this bep does not know is told apart from a corrupt line- crates/bep/src/main.rs:
bep serve --quarantine-unreadableruns the quarantine and starts;refuse_unreadablenames that flagtest: cargo nextest run -p bep quarantine_keeps_the_chain_and_starts
produces: none
consumes: none
proof:
bep serve --quarantine-unreadablestarts past an unreadable audit line, and the chain still verifies across the quarantine.depends on:
- blocked by #2290
Proof
A person can run
bep serve --quarantine-unreadableagainst an audit log that holds an unreadable line. bep moves each segment with such a line into /quarantine/ and starts.audit::verify_chainstill verifies the log across the gap through the appendedquarantinerecord. Without the flag, the refusal names--quarantine-unreadablein place of a hand edit.Notes from the agent: Design: no
Kind::Quarantinevariant. Exhaustive matches onKindexist in crates/bep/src/control.rs and crates/minimal/src/cmd/box_audit.rs, and both are outside the layers. The quarantine record is a separateQuarantineRecordline (kind "quarantine", segment, from_hash, final_hash, unreadable, previous_hash). The reader classifies each line. A schema record with an unknown kind, or a quarantine record, is counted in the newReadBatch::other_kinds, not inskipped, so it does not refuse the start.holds_revocation(prune) no longer count 8000 s these lines as possible revocations. A line with a known kind and a malformed field is still corrupt.Chain:
final_hashis theprevious_hashof the first record of the next segment, when that record is readable. Otherwise it is the file's last-line hash. So a quarantine still bridges when the unreadable line is the segment's last line.verify_chain/verify_segmentsare new. They accept any hash on the first retained record, because older segments can be pruned, and they follow quarantine gaps from the head to the found hash. When the active segment is moved, the log opens segment n+1, chained to the moved head.The quarantine also appends again each revocation it can read in a moved segment, so those revocations stay in force at later starts. Without this, the move drops them from the log. A revocation that only an unreadable line holds is lost. The
--helptext and the refusal say so.The refusal text changed: 'Repair or remove those lines' is replaced by the flag instruction. The existing test serve_refuses…[truncated]
Tests
cargo nextest run -p bep quarantine_keeps_the_chain_and_startsVerify
min task run agent-verifypassed ([tasks.verify]).Residuals
quarantinerecord and thequarantine_restatedmarker.Closes #2588
Generated by Claude Code