feat(storage): decide a late tombstone in core, not in a sequence comparison (RECON-P3a) - #2268
feat(storage): decide a late tombstone in core, not in a sequence comparison (RECON-P3a)#2268Jurij89 wants to merge 18 commits into
Conversation
| acceptedTransitionLineage: readonly AgentProfileAppliedTransitionV1[], | ||
| ): SystemRecordAuthorityDecisionV1 { | ||
| const candidateState = validateAgentProfileHeadObjectV1(candidate); | ||
| const evidenceState = snapshotHeadAdvanceEvidenceV1(evidence); |
There was a problem hiding this comment.
🔴 Bug: Late-tombstone helper skips the full evaluator's clock and future-head guards
What's wrong
The new exported helper is described as the same lower-sequence arm, but it does not run the entry-level checks that make that arm safe. Because the lower-sequence rule intentionally accepts the tombstone whenever the retained transition does not accept, an invalid verification clock or too-far-future candidate can be converted into an accepted tombstone instead of a reject.
Example
Call evaluateAgentProfileLateTombstoneAdvanceV1 with a below-sequence tombstone, its exact predecessor, a retained transition whose digest matches the accepted lineage, and evidence.nowMs omitted or Number.NaN. evaluateAuthorityTransitionV1 rejects the transition because the clock is invalid, but the lower-sequence arm treats that non-accept as { decision: 'accept' }. The full evaluateAgentProfileHeadAdvanceV1 path would reject the same evidence before reaching this arm.
Suggested direction
Mirror the full head-advance entry's global validation in the new helper, or otherwise ensure invalid clock/future-head failures are returned as rejects before the lower-sequence tombstone rule interprets transition rejection as tombstone precedence.
For Agents
In packages/core/src/system-record-authority-v1-internal.ts, make evaluateAgentProfileLateTombstoneAdvanceV1 preserve the full evaluator's global evidence checks before delegating, especially isSafeNow(evidenceState.nowMs) and isIssuedTooFarInFuture(candidateState.issuedAt, evidenceState.nowMs). Add a focused case proving invalid/missing nowMs and too-far-future candidate issuedAt cannot become accept through the late-tombstone helper.
There was a problem hiding this comment.
🔴 Bug: Late-tombstone helper skips the full evaluator's clock and head-time gates
What's wrong
This new public helper can return an authority accept for inputs the existing core evaluator would reject before any late-tombstone logic runs. That changes the security contract for callers using the exported seam and can admit or preserve decisions based on invalid verification time or a candidate head issued too far in the future.
Example
Pass a below-sequence tombstone with its exact predecessor, a retained transition whose digest matches acceptedTransitionLineage[candidateSequence] but whose priorHeadDigest does not name the tombstone, and evidence.nowMs = Number.NaN. evaluateAgentProfileLateTombstoneAdvanceV1 returns { decision: 'accept' }, while evaluateAgentProfileHeadAdvanceV1 would reject the same clock at its front door. A future-dated candidate head similarly bypasses the head issuedAt check.
Suggested direction
Add the same isSafeNow and isIssuedTooFarInFuture checks used by evaluateAgentProfileHeadAdvanceV1 before calling evaluateLowerSequenceAgentProfileHeadAdvanceV1, or route through an API shape that cannot bypass those preconditions.
For Agents
In packages/core/src/system-record-authority-v1-internal.ts, make evaluateAgentProfileLateTombstoneAdvanceV1 preserve the full evaluator's required preconditions before delegating: reject unsafe nowMs and candidate issuedAt beyond skew, or otherwise constrain the helper so callers cannot observe decisions without those checks. Add focused core tests proving invalid nowMs and future candidate issuedAt reject instead of becoming accept on the late-tombstone helper.
There was a problem hiding this comment.
🔴 Bug: Late-tombstone helper bypasses the full evaluator's clock and head timestamp checks
What's wrong
This helper is now exported as a public authority decision entry, but it does not preserve the validation contract of evaluateAgentProfileHeadAdvanceV1. Because the delegated lower-sequence arm treats every transition result other than accept as a reason to accept the tombstone, clock failures or a too-future candidate can be converted into an authority acceptance instead of a rejection.
Example
Build the same late tombstone used by system-record-late-tombstone-seam-v1.test.ts, but set the candidate tombstone issuedAt beyond the allowed future skew and provide the exact retained transition that does not name the tombstone as prior head. evaluateAgentProfileHeadAdvanceV1(...) rejects with head issuedAt exceeds the future clock-skew bound; evaluateAgentProfileLateTombstoneAdvanceV1(...) reaches the lower-sequence arm and can return accept. Similarly, an invalid nowMs makes evaluateAuthorityTransitionV1 reject, which this arm interprets as tombstone precedence instead of a clock failure.
Suggested direction
Run the same isSafeNow and candidate issuedAt skew checks in the new helper before delegating, or split the lower-sequence transition result so validation failures are not treated as tombstone precedence.
For Agents
In packages/core/src/system-record-authority-v1-internal.ts, keep the late-tombstone helper aligned with the public head-advance preflight contract. Add coverage showing invalid nowMs and too-future tombstone heads reject through evaluateAgentProfileLateTombstoneAdvanceV1, while valid retained-transition cases still produce the intended stale/accept split.
There was a problem hiding this comment.
Confirmed by construction, and fixed. Thank you — this was real.
I built the state before changing anything. One tombstone below the current sequence, its exact predecessor, and a retained transition that binds the tombstone, so the correct answer is stale:
| clock | entry (before) | full evaluator |
|---|---|---|
| valid | stale |
— |
Number.NaN |
accept |
`reject |
-1 |
accept |
`reject |
So a clock failure did not merely skip a check — it inverted the verdict, because this arm reads a non-accept from the transition verifier as "the tombstone takes precedence". My own docblock claimed the residue of an unusable clock was "a refusal, never an admission". It was the opposite, and nothing tested it.
The fix is the boundary rather than a bolted-on guard, because the same shape would have come back the next time someone built evidence by hand. The retained transition and its clock are now ONE optional field on a purpose-built AgentProfileLateTombstoneEvidenceV1: neither is meaningful alone here — the transition is only ever checked by a clocked verifier, and a clock with no transition is a value nothing reads. "Binding transition plus unusable clock" is now unrepresentable rather than merely refused. On top of that the two global gates you named (isSafeNow, isIssuedTooFarInFuture) are mirrored in the entry rather than delegated, exactly because below that point a refusal MEANS precedence.
Focused cases added as you asked, and they pin more than the reported shape: three clocks that fail isSafeNow for three different reasons (not-a-number, negative, non-integer — a guard written for one is not a guard for the others), the too-far-future candidate head, and the invariant the pairing buys — accept and stale are reachable only when a transition and a valid clock arrived together, asserted rather than described.
Fixed in b2f08c1.
There was a problem hiding this comment.
🟡 Issue: Extract the late-tombstone rule instead of adapting through synthetic head-advance evidence
What's wrong
The new public entry point introduces a narrow evidence type, then immediately translates it back into the broader head-advance evidence shape and uses acceptedTransition/nowMs as overloaded transport fields. That keeps the real invariant in comments and control-flow ordering rather than in the helper signature, which is fragile and harder to maintain.
Example
When retainedTransition is absent, the new API builds { nowMs: Number.NaN } and relies on evaluateLowerSequenceAgentProfileHeadAdvanceV1 rejecting before any clock read. The comment explicitly depends on that ordering instead of making the rule's operands impossible to misuse.
Suggested direction
Make the late-tombstone arm the shared abstraction directly. That would delete the sentinel clock, the broad evidence snapshot adapter, and the duplicated clock guards while preserving behavior.
For Agents
In system-record-authority-v1-internal.ts, extract the lower-sequence tombstone rule into a pure helper that takes candidateState, tombstonePredecessor, retainedTransition?: { transition; nowMs }, lineage, and candidateSequence. Call that helper from both evaluateAgentProfileHeadAdvanceV1 and evaluateAgentProfileLateTombstoneAdvanceV1; preserve all existing decisions and the late-tombstone seam tests.
There was a problem hiding this comment.
🟡 Issue: Decompose the authority evaluator before merging a sub-1k file into a 1201-line module
What's wrong
This PR crosses the explicit 1k-line health boundary for a central production module. The new code is not just a few branches; it adds a separate public subdomain with its own operand models and extensive commentary. Keeping it all in the already-central evaluator makes the authority layer harder to navigate and raises the cost of future changes.
Example
The central authority file now contains the original head-advance evaluator plus late-tombstone evidence snapshots, same-sequence applied-row snapshots, two new public entry points, and their rule implementations. A reader changing ordinary head advancement has to scan storage/receiver-specific tombstone entry points in the same 1201-line module.
Suggested direction
Move the late/same-sequence tombstone evidence types, snapshotters, public entries, and rule helpers behind a dedicated tombstone-authority module. Leave this file as the canonical head-advance evaluator instead of making it the home for every authority subdomain.
For Agents
Split the tombstone-specific rules and operand snapshotters out of packages/core/src/system-record-authority-v1-internal.ts into a focused internal module, keep the barrel exports unchanged, and preserve evaluateAgentProfileHeadAdvanceV1 behavior by importing only the shared rule/adapter it needs. Existing seam tests should continue to pass.
There was a problem hiding this comment.
Same subject as the thread on this file's other hunk, where I have already conceded the measurement — recording it here too rather than answering twice with different words.
You are right about the number and right that the prediction was yours. The file is 1,201 lines; it was 1,095 when the earlier comment said the next rule addition would very likely cross the boundary. The next rule addition was this one. I am not re-running the cohesion argument, and I want to be explicit that this is a concession rather than a standing decline restated — a decline that cites its own earlier answer is the same stale-citation shape I would flag in a reviewer.
The reason it is not in this commit is specific, and it is about a measurement in flight rather than about the file:
An independent adversarial verification of this seam's decision semantics is pinned at a commit and re-runs its own ADR-derived table against my final head, cell by cell, with a declared list of which cells are permitted to move. A module move relocates every line-anchored citation into this file. Seven of them moved in this commit alone — and one had to be re-resolved by SITE rather than by matching its text, because the same predicate call now stands at three places and two of them are textually identical. If the extraction and the semantic change land in one push, that verification cannot distinguish a verdict that moved from a line that moved. This seam has produced five decision-inverting defects that its author did not see; the ability to attribute movement is not something I want to spend on file layout.
So: filed with the lead as its own change, and sized honestly as mechanical — the evidence types, both snapshot helpers, both rules and both entries into a focused internal module, the adapters staying here, the barrel exports unchanged. If the lead sequences it before merge rather than after, it is one commit and I will do it.
One correction to the suggested direction, because it would change what the code proves rather than only where it lives. Moving the narrative rationale out is right for the round-by-round history and wrong for two specific passages: the clock-and-transition pairing, and the "a non-accept is not an affirmative" mapping. Each of those is inversion prevention — each is written where it is because deleting the sentence is how the defect returned the second time. Those stay beside the branch they govern; the history goes with the constructions that prove it.
There was a problem hiding this comment.
🔴 Bug: Same-sequence tombstone evaluation can accept a tombstone for a different authority
What's wrong
The rule being exported and used for storage no longer has the accepted head object, but the replacement row it receives does not carry enough identity to prove the candidate tombstone is competing with that row. Because the active-row branch accepts every unequal-version tombstone after predecessor binding, a valid tombstone from another authority can be read as a revocation of the current authority instead of being rejected or deferred.
Example
Call evaluateAgentProfileSameSequenceTombstoneAdvanceV1 with a tombstone for authority B at sequence 2, bound to B's active predecessor, and an applied row for authority A { status: 'active', authoritySequence: '2', version: '9', headDigest: digestOfA }. The candidate sequence matches and the version is unequal, so the rule returns { decision: 'accept' } even though the tombstone is unrelated to the applied row. In storage, classifySameSequenceTombstoneAdvance passes only these reduced row fields, so this guard is not reintroduced there.
Suggested direction
Include and validate stable authority identity in the applied-row operand, or require the storage adapter to check that the candidate/predecessor/summary belong to the same persisted row before calling the core rule or before honoring accept.
For Agents
Look at evaluateAgentProfileSameSequenceTombstoneAdvanceV1 and the storage adapter in classifySameSequenceTombstoneAdvance. Preserve the intended same-sequence tombstone behavior for the same authority, but add enough applied-row identity to reject unrelated candidates, or have the storage adapter compare current/summary identity before mapping core accept to advance. Add a test where the candidate tombstone is valid for another peer/root at the same sequence and unequal version, and prove it does not advance over the current row.
There was a problem hiding this comment.
🟡 Issue: Split the tombstone authority rules instead of pushing this file past 1k lines
What's wrong
This PR pushes a core implementation file from comfortably under 1000 lines to well over it without a structural split. The added code is not just a few local branches; it introduces new public operands, runtime snapshotters, two standalone entry points, and two rule implementations. Keeping all of that in the existing authority module makes the highest-traffic file harder to scan and raises the cost of future authority-rule changes.
Example
A reader following the same-sequence tombstone path now has to scan the main head-advance evaluator, the new exported persisted-row adapter, and the rule helper all inside one 1226-line module. This is exactly the file-size threshold where decomposition should happen first.
Suggested direction
Keep system-record-authority-v1-internal.ts as the dispatcher/core authority flow and move the new tombstone-specific rule surface behind a dedicated module. This would make the new ADR rules easier to reason about and avoid turning the central authority file into a catch-all for every special-case seam.
For Agents
In packages/core/src/system-record-authority-v1-internal.ts, extract the tombstone-specific operand models, snapshotters, rule helpers, and exported tombstone entry points into a focused internal module such as system-record-authority-tombstone-v1-internal.ts. Preserve the existing public exports through system-record-objects-v1.ts if they remain public, and verify with the existing system-record export/type tests.
There was a problem hiding this comment.
🟡 Issue: The late-tombstone expired-prior branch is untested
What's wrong
This PR changes how the late-tombstone rule handles a retained transition that names the tombstone but is inadmissible as an expired-prior transition. The new code propagates that verifier rejection instead of treating every non-accept as tombstone precedence, but the added tests do not exercise that branch on this public entry.
Example
Build the existing binding retained transition from system-record-late-tombstone-seam-v1.test.ts, change it to mode: 'expired-prior' with priorValidUntil, and call evaluateAgentProfileLateTombstoneAdvanceV1(candidate, { tombstonePredecessor, retainedTransition: { transition, nowMs } }, lineageFor(transition)). The test should assert the propagated reject, such as expired-prior transition cannot resurrect a tombstone.
Suggested direction
Add a focused regression test for a retained expired-prior transition that binds the late tombstone, proving the new inadmissible-expired-prior result is propagated by the late-tombstone entry.
Confidence note
I found tests for co-signed late-tombstone retained transitions and for future/invalid clock refusals, plus existing expired-prior coverage for the standalone transition verifier, but no test that drives an expired-prior retained transition through the new late-tombstone rule where the prior head is the tombstone candidate.
For Agents
Add a late-tombstone seam test near the existing bound/unbound transition cases. Use the same candidate, predecessor, and lineageFor helper, but construct a retained transition that names the tombstone and has mode: 'expired-prior'. Assert the exact reject reason returned through evaluateAgentProfileLateTombstoneAdvanceV1.
| const decision = evaluateAgentProfileLateTombstoneAdvanceV1( | ||
| facts.head, | ||
| Object.freeze({ | ||
| nowMs: NO_VERIFICATION_CLOCK_V1, |
There was a problem hiding this comment.
🟡 Issue: The late-tombstone path is squeezed through generic contracts instead of having its own typed boundary
What's wrong
The new seam spreads special-case knowledge across broad abstractions: tombstone-only reasons are added to the active derivation reason union, the core helper accepts the full generic head-advance evidence object, storage fabricates a clock, and storage must know one core decision arm cannot happen. That makes the implementation harder to reason about than the behavior requires.
Example
Storage currently has no retained transition and no verification clock, but the API still requires a head-advance evidence object with nowMs, so the caller supplies Number.NaN and relies on branch ordering to keep that value unobserved.
Suggested direction
Make late tombstone a first-class typed boundary in core/storage instead of widening generic active/head-advance shapes. A narrower API would let the type system express “no retained transition yet means retry” and would remove the NaN sentinel plus unreachable default handling.
For Agents
Look at evaluateAgentProfileLateTombstoneAdvanceV1 and classifyLateTombstoneAdvance. Preserve the same accept/stale/retry mapping, but introduce a narrow late-tombstone evidence/decision type whose fields match this branch: lineage, tombstonePredecessor, optional retained transition, and a clock only when a retained transition is actually supplied. Keep tombstone-only storage reasons out of the active-only result model or split the replacement derivation result by operation. Existing late-tombstone tests should still pass and should no longer need the fake clock/unreachable-quarantine prose.
There was a problem hiding this comment.
🟡 Issue: The core bridge is shaped around a fake verification clock
What's wrong
This is a maintainability smell rather than a local style issue: production code is satisfying a too-broad API with Number.NaN and relying on knowledge of which downstream branches currently do not read it. The long comment is doing work the type boundary should do.
Example
Current flow builds core evidence as { nowMs: Number.NaN, tombstonePredecessor } just to satisfy AgentProfileHeadAdvanceEvidenceV1. If a retained-transition producer is later added but this call site is not redesigned, the boundary still carries a fake clock and relies on core rejecting through a validation side effect.
Suggested direction
Make the boundary express the actual operands for this rule. A small AgentProfileLateTombstoneEvidenceV1 shape, or a branch that maps missing retained-transition evidence before calling the clocked transition verifier, would remove the sentinel and make future changes harder to misuse.
For Agents
Look at classifyLateTombstoneAdvance in storage and evaluateAgentProfileLateTombstoneAdvanceV1 in core. Preserve the current defer/advance/stale mapping, but replace the generic evidence shape plus NaN sentinel with a dedicated late-tombstone evidence model, or gate the call so core is invoked only when all operands it requires are real. Add/update tests covering the missing-retained-transition path and the retained-transition-present path without using a fake clock.
There was a problem hiding this comment.
🟡 Issue: The late-tombstone boundary is encoded with fake evidence instead of types
What's wrong
This adds a fragile boundary between storage and core: the real invariant lives in a long comment and a sentinel value, not in the function signature. Future retained-transition support will require remembering to replace both the missing acceptedTransition and the fake clock, and the current mapping is harder to reason about because some cases are structurally unreachable from the only production caller.
Example
The call at this line passes Object.freeze({ nowMs: NO_VERIFICATION_CLOCK_V1, ...(predecessor === undefined ? {} : { tombstonePredecessor: predecessor }) }); there is no acceptedTransition, so core is being used through an intentionally incomplete evidence object.
Suggested direction
Introduce an explicit late-tombstone evidence shape or core helper whose signature matches the operands storage actually has. Make retained-transition evidence and a real verification clock a typed pair, and make the current “retry because evidence is absent” path direct instead of relying on a sentinel and unreachable switch arms.
For Agents
Look at evaluateAgentProfileLateTombstoneAdvanceV1 in core and classifyLateTombstoneAdvance in storage. Preserve the current retry mapping for absent retained-transition evidence, but encode absent retained transition and absent verification clock in a purpose-built type or helper rather than NaN plus prose. The late-tombstone seam tests should still prove the same storage outcomes.
There was a problem hiding this comment.
Agreed, and the 🔴 above forced exactly this refactor — the sentinel is gone.
You were right that the long comment was doing work the type boundary should do, and it turned out to be load-bearing rather than cosmetic: the same Number.NaN that "relied on branch ordering to keep that value unobserved" also inverted a stale into an accept when a caller DID supply a transition. The comment was not just carrying the design, it was carrying a false claim about it.
What shipped, in your terms:
- A dedicated late-tombstone evidence type.
AgentProfileLateTombstoneEvidenceV1carries onlytombstonePredecessorandretainedTransition, and the retained transition carries its own clock — so a clock exists only when a transition is actually supplied, which is the shape you proposed. - No fake clock anywhere in storage. The caller now passes only what it holds;
NO_VERIFICATION_CLOCK_V1is deleted. - The unreachable-quarantine prose is now an exhaustive switch with a
neverassignment. Dropping an arm fails the BUILD (TS2322: … is not assignable to type 'never'), so it is enforced rather than described.
One thing I did not take, and I want to be explicit rather than silent about it: the tombstone-only reasons stay on the shared deferral union rather than splitting the replacement derivation result by operation. That split touches the executor's outcome mapping and the verdict-diff codomain, and it is a wider refactor than this slice should carry — I would rather it be its own change with its own before/after than ride a security fix. Filed for the follow-up slice that already owns the active seam.
Fixed in b2f08c1.
There was a problem hiding this comment.
Round 2 finished this one properly — the sentinel is gone from the codebase, not just from storage.
Recording it here because this thread is where the smell was first named: the fake clock no longer exists anywhere. Number.NaN appears zero times in both the core authority file and storage's next-state file. The rule takes its own operands, both entries call it, and the "unreachable when no transition was supplied" comment that this thread correctly identified as load-bearing prose has been deleted along with the thing it was describing.
Landed in 1b23465.
There was a problem hiding this comment.
🟡 Issue: Do not route storage through a core entry with deliberately incomplete evidence
What's wrong
The adapter currently looks like storage delegates the late-tombstone decision to core, but the evidence it passes intentionally omits the one field required for core to decide accept or stale. That creates an abstraction that is wider than the actual storage boundary and makes future readers reason about branches this package cannot reach today.
Example
For the active applied-status path, storage calls core with { tombstonePredecessor } but no retainedTransition; by the new evidence contract, that can only become reject | late tombstone requires the exact retained resurrection transition, which storage maps to deferred|late-tombstone-evidence-incomplete.
Suggested direction
Model retained-transition availability as an explicit boundary instead of manufacturing a partial evidence object just to drive core's missing-evidence reject. That would delete the misleading unreachable accept/stale mapping from today's storage path, or make those arms reachable through a real input.
Confidence note
This is a maintainability concern, not a claim that the current mapping is behaviorally wrong. The comments state storage cannot currently supply a retained transition, which is the premise for the finding.
For Agents
In classifyLateTombstoneAdvance, make the storage evidence boundary explicit. Either add a real retained-transition provider to storage facts and pass it through, or make the current no-transition case a direct named deferral and reserve the core call for the branch where storage actually has the transition. Keep active rows retryable and keep tombstone/dirty rows under undecided-authority-classification.
There was a problem hiding this comment.
Standing decline on the late-tombstone arm, re-measured — and this round moved a premise, in your favour.
Re-measuring my own answer first, since a decline that cites its earlier self is the stale-citation shape I would flag in a reviewer.
Your finding rests on the seam being an abstraction wider than the boundary it serves: storage calls a core entry with evidence that can only ever produce one answer, so the accept and stale arms are decoration and a reader has to trace into core to learn that. That was a fair description of a seam with one routed arm. It is no longer a description of the seam, because this round routes a second one — the same-sequence tombstone rule — and that rule's evidence is evidence storage fully holds. The predecessor is all it reads, storage has it, and all four of its decisions are produced by the entry. The mapping there is not driving a foregone refusal; it is mapping a real answer that varies by input.
So the shape you asked for exists now, arrived at from the other end: a core entry whose operands are exactly what a receiver persists. The late-tombstone arm is the one where that is not achievable today, and the difference between the two arms is now visible in the code rather than only in a comment.
On the late-tombstone arm itself, the decline stands and the reason is unchanged in substance: collapsing it to "no transition, therefore the retry deferral" puts storage in charge of ADR 0002 :132-133, which is an authority rule sitting in the same paragraph as the accept and stale clauses. That is the re-implementation this slice forbids, at the one branch that is live today. Core owns the disposition of missing evidence; storage maps the answer.
Two things this round adds that are new rather than restated:
The unreachability is now measured as structural, not merely asserted. For the same-sequence arm's reject, I attempted every one of the nine binding conjuncts individually, with the bound control minting green in the same run. Every one is refused upstream of the classifier: four by the head codec as malformed heads, five by the verification closure at summary-mint — because the closure runs the same binding predicate over the heads it parses. The producer of the evidence storage requires enforces precisely the conjunction core tests. That is a stronger statement than "storage cannot reach it today", and it ships as a run rather than as a comment.
One arm is written knowing it cannot fire, deliberately. I would rather state that plainly than let it read as an oversight. An unreachable arm written explicitly is the difference between "this cannot happen" being enforced and being believed. When the precondition or the producer changes, the arm is already correct instead of being whatever a default swept it into — and this seam has twice shipped a default that absorbed a case nobody had enumerated.
There was a problem hiding this comment.
🔴 Bug: Same-sequence tombstones can advance without matching the applied authority lineage
What's wrong
The change routes same-sequence tombstones through a new core entry using an applied-row shape that omits authority-continuity data storage still has. That lets a tombstone bound to some active predecessor at the same sequence dominate the current active row solely by sequence/version, even when it belongs to a different same-sequence fork. This materially widens the new advance path and can delete the currently applied projection from the wrong authority branch.
Example
A receiver has an active applied row at authority sequence 2 for root A / transition digest D1. It receives a verified tombstone at sequence 2, version 1 or 3, whose predecessor is a different same-sequence fork for the same peer but root B or transition digest D2. With this adapter, core sees only status: 'active', the same sequence, and an unequal version, returns accept, and storage maps that to advance, deleting root A. The full evaluator with the current head object would reject same-sequence authority changed or quarantine transition-equivocation before reaching the tombstone rule.
Suggested direction
Pass or check enough persisted context before mapping accept to advance: at minimum compare the candidate/predecessor root and retained transition digest/lineage against the applied row, or keep this path deferred when storage cannot prove it is the same same-sequence authority branch.
For Agents
Look at classifySameSequenceTombstoneAdvance and the new AgentProfileSameSequenceAppliedRowV1/evaluateAgentProfileSameSequenceTombstoneAdvanceV1 boundary. Preserve ADR :112-114 for a tombstone that is in the same authority lineage, but prove storage does not advance when the candidate/predecessor root or retained transition digest differs from the applied row. Add a storage-level regression that constructs a same-sequence tombstone from a different fork/root and expects a retry/quarantine/refusal, not ready.
There was a problem hiding this comment.
I am treating this as real, and I am not going to agree with it yet — those are different things and this thread deserves both stated plainly.
What I have confirmed, by reading both layers: the mechanism you describe exists. The full evaluator establishes RECORD IDENTITY before it dispatches to the same-sequence tombstone rule — same-sequence authority changed on issuer/root, then transition-equivocation on an unequal acceptedTransitionDigest. Neither of those checks is in the rule. They are in the adapter above it, and my entry does not carry them.
Worse, my operand shape cannot carry them: AgentProfileSameSequenceAppliedRowV1 is status, sequence, version and head digest. There is nothing in it that lets core ask whether the candidate belongs to the same authority branch as the applied row. So this is not a missing conjunct I forgot to copy — it is a precondition the entry has no way to check and whose absence I did not document as a caller obligation either.
And nothing upstream in storage supplies it. The tombstone-facts assert compares the candidate against ITS OWN predecessor, not against the applied row; the root comparison against the applied row lives only inside the exact-match short-circuit, which is the branch this population is defined to be outside of. Before this PR the arm returned stale or a deferral, so a wrong-branch tombstone was discarded or retried. After it, the mapping is advance, and advance deletes the projection.
This is the same defect as an earlier round of this PR, one rule over. That round found an exported entry reusing a shortcut that was sound inside the full evaluator only because record identity had already been established before dispatch. I fixed that instance, wrote the reason into the docblock, and then built a second entry with the same hole. A correct exclusion did not dispose of the class.
What I have NOT confirmed, and will not claim either way: reachability. Your example needs a same-sequence candidate whose root or accepted-transition digest differs from the applied row's. The head codec binds rootSubject to evmIssuer, so the root half may not be constructible for one record at one sequence. The transition-digest half looks constructible — an equivocation not yet detected has empty conflict slots and an active status, so it passes the upstream screen and the classification reader calls it decided — but looks constructible is exactly the phrase this PR has been burned by in both directions. The population pin that said adjudicated: 0 for the region this whole commit fixes was measuring the instrument, not the system.
So the order is: build the cell first, and let the construction decide whether this is a live bypass, an entry-layer-only hole, or unconstructible-with-cause. Agreeing before measuring would put a fix in on the strength of a reading, on a path whose failure mode is deleting the applied projection from the wrong branch — and I would rather hand the next seat a confirmed defect than a confident one.
The fix I expect, so the shape is on the record either way: give the operand the identity fields storage actually persists — currentRoot, and the accepted-transition digest the applied lineage's last entry carries — and let CORE run the same identity conjuncts the full evaluator runs before dispatch. That keeps the comparison in core rather than re-implementing it at the seam, which is this slice's absolute constraint, and it makes the entry's precondition checkable instead of assumed.
Flagged to my lead as a blocking finding with that shape, and the construction is the gate before anything moves.
There was a problem hiding this comment.
🟡 Issue: Reject mapping is not actually exhaustive because reject reasons are untyped
What's wrong
The new switches are written as if they are exhaustive, but they only exhaust decision, not the reject cases the storage reasons depend on. Because reject.reason is just string, a new or reclassified core reject will be absorbed by the existing case 'reject' and mislabeled as the current storage reason. This weakens the boundary exactly where the PR is trying to make core/storage authority decisions more explicit.
Example
If core later adds a new same-sequence reject reason under the existing decision: 'reject' branch, the switch at line 1226 still compiles and storage silently reports it as tombstone-predecessor-unbound. The default arm never runs because the discriminant did not change.
Suggested direction
Make the boundary explicit before mapping it. The storage adapter should either receive a narrow, storage-specific core result or switch over a typed reject category, not over the broad public authority decision where every reject shares the same discriminant and an unconstrained string reason.
For Agents
In classifyLateTombstoneAdvance and classifySameSequenceTombstoneAdvance, stop mapping all rejects by decision alone. Introduce a narrow core result type for the storage seam, a storage-facing mapper returned by core, or explicit typed reject variants that distinguish missing retained evidence, predecessor-unbound, invalid candidate, and other refusals. Add a focused type assertion that a new reject category forces the storage mapper to change.
| * is a different ADR rule (:126-128) and was measured to contain 3,456 cells of | ||
| * which ZERO are adjudicated, so "nothing else moved" there is structural. | ||
| */ | ||
| export const LATE_TOMBSTONE_SEAM_MOVEMENT_V1: Readonly<Record<string, number>> = { |
There was a problem hiding this comment.
🟡 Issue: The added seam accounting pushes the join-table helper over 1000 lines
What's wrong
This PR grows a test helper from 875 to 1002 lines. The new block is cohesive enough to stand alone, and leaving it in the main join-table file makes an already dense artifact harder to navigate and maintain.
Example
A reviewer now has to scan a 1002-line helper that mixes the main join table, seam-specific movement tables, population accounting, long historical notes, and unrelated impossibility proofs in one file.
Suggested direction
Decompose the seam-specific tables and narrative into a focused helper/module instead of growing the already dense join-table helper. The main table can keep the aggregate map while importing the late-tombstone movement/population facts from a dedicated file.
For Agents
Split the late-tombstone seam accounting out of authority-verdict-diff-join-table-v1.ts, for example into an authority-verdict-diff-late-tombstone-seam-table-v1.ts helper imported by the main join table. Preserve exported constants and table values, and run the affected verdict-diff tests/import checks after moving the constants.
There was a problem hiding this comment.
🟡 Issue: This helper crosses 1,000 lines instead of being decomposed
What's wrong
The PR adds another large, self-contained section to a file that was already broad and pushes it over the 1k-line threshold. That makes the helper harder to scan and turns future count updates into edits across an oversized mixed-purpose file.
Example
LATE_TOMBSTONE_SEAM_MOVEMENT_V1 and LATE_TOMBSTONE_SEAM_POPULATION_V1 are a focused subdomain, but they now live inside the already dense join-table helper alongside global tables, citations, impossibility proofs, mutants, and findings.
Suggested direction
Extract the late-tombstone seam tables and their explanatory proof text into a dedicated module such as authority-verdict-diff-late-tombstone-seam-v1.ts. Keep the main join table as the top-level aggregation surface.
For Agents
Split the late-tombstone seam accounting out of authority-verdict-diff-join-table-v1.ts into a focused helper, then import or re-export the constants needed by existing consumers. Preserve the pinned counts and existing test behavior; the useful proof data should stay, just not push the general join table past the 1k-line boundary.
There was a problem hiding this comment.
🟡 Issue: The late-tombstone tables push this helper past 1k lines
What's wrong
This PR takes a file from below 1,000 lines to above 1,000 lines without a structural reason. The added material is a separable late-tombstone measurement cluster, so keeping it in the monolithic join table increases sprawl and future line-number churn.
Example
LATE_TOMBSTONE_SEAM_MOVEMENT_V1, LATE_TOMBSTONE_SEAM_POPULATION_V1, and LATE_TOMBSTONE_COUNTERFACTUAL_CORE_DECISIONS_V1 are all added to an already dense aggregate table file.
Suggested direction
Decompose the new seam-specific tables and long explanatory block into a dedicated module, leaving the join table as a compact aggregate/index.
For Agents
Split the late-tombstone seam measurements into a focused helper such as authority-verdict-diff-late-tombstone-table-v1.ts, re-export/import the constants needed by the aggregate table, and preserve the existing pinned counts.
There was a problem hiding this comment.
Done — and reviewing that file for the split surfaced a defect the comment was not about.
The seam accounting now lives in authority-verdict-diff-late-tombstone-seam-v1.ts (predicate, movement, population, counterfactual). The join table is back under 1,000 lines and keeps the aggregate surface.
The find: when I went to move them I checked their consumers, and there were none. Three exported tables of pinned numbers that no test asserted — data that reads as evidence and cannot fail, which is the exact species this artifact exists to refuse. They are now asserted against the live join: the after rows are rebuilt from the swept result and compared, both halves are checked to conserve to the same 1,728, and the population is re-derived from the cell axes. I mutated one count (384 → 385) and watched the row go red, so the assertion is not vacuous either.
So the file-size comment was worth more than its face value. Fixed in b2f08c1.
| export { | ||
| assertAgentProfileForkResolutionEvidenceV1, | ||
| evaluateAgentProfileHeadAdvanceV1, | ||
| evaluateAgentProfileLateTombstoneAdvanceV1, |
There was a problem hiding this comment.
🟡 Issue: Public export change is not reflected in the export contract tests
What's wrong
This line makes the late-tombstone helper part of the public package API, but the package’s exact export contract tests were not updated in the diff. That means the validation lane is either red, or if that lane is skipped, there is no pinned evidence that the newly exported API is intentionally part of the surface.
Example
Running the core package export check after build would make Object.keys(api).sort() include evaluateAgentProfileLateTombstoneAdvanceV1, while expectedRuntimeExports does not, triggering system-record-v1 runtime exports changed.
Suggested direction
Extend the existing runtime and type export contract checks to include the new helper, so the PR’s validation evidence covers the public API change instead of leaving the export test stale.
For Agents
Update the core package export contract tests for the new public API: add evaluateAgentProfileLateTombstoneAdvanceV1 to packages/core/test/system-record-package-export-v1.mjs, add an appropriate compile-time import/use in system-record-package-export-v1.types.ts, then run pnpm --filter @origintrail-official/dkg-core test:system-record-export or the repo-equivalent command.
There was a problem hiding this comment.
Already fixed, one commit before this review landed — and CI caught it exactly as you describe.
ea85eb7c4 added evaluateAgentProfileLateTombstoneAdvanceV1 to system-record-package-export-v1.mjs; the gate now reports 275 exact symbols, up from 274. Your predicted failure was the literal CI failure: "System-record protocol evidence" went red on d1082e3af with the runtime-export mismatch.
The second half of your suggestion was still open and is now done too: b2f08c1b3 adds the new types to system-record-package-export-v1.types.ts (AgentProfileLateTombstoneEvidenceV1, AgentProfileLateTombstoneRetainedTransitionV1), so the compile-time half of the contract covers the change as well. Runtime symbol count is unchanged at 275, which is correct — types add no runtime export.
Worth recording why local verification missed it: I ran full package vitest suites and reported them as full package suites. That was true and still missed this gate, because core's test script is three steps and this one is a node script plus a standalone tsc, not vitest at all. Both packages' non-vitest gates are now part of my pre-push run.
There was a problem hiding this comment.
🟡 Issue: Avoid publishing the binding predicate as public API
What's wrong
This turns an internal refactoring helper into a package contract. That makes future cleanup of transition verification harder because callers can start relying on exactly which fields this predicate checks and how validation/digesting is sequenced.
Example
An external caller can now depend on this low-level structural predicate even though the PR only needs it to keep the late-tombstone rule from matching verifier reason text.
Suggested direction
Do not expand the public system-record surface for a helper whose only visible purpose is an internal seam. If this concept is meant to be public, expose it as part of a deliberate verifier API rather than a narrow implementation predicate.
For Agents
Keep isAuthorityTransitionBoundToPriorHeadV1 internal to core authority verification, remove it from system-record-objects-v1.ts and the package export pin, and verify the storage caller still compiles through evaluateAgentProfileLateTombstoneAdvanceV1.
There was a problem hiding this comment.
Agreed, and reverted — the published surface is back to 275 symbols.
You are right that this was an internal seam helper becoming a package contract. It is now classifyAuthorityTransitionBindingV1 inside core's authority-verification module, not exported from system-record-objects-v1.ts and removed from the export pin.
Worth noting the sequence, because your comment and the round's 🔴 met in the middle: the same round found that the predicate's single boolean was too coarse for this seam, so it became a three-way classifier. That made it more obviously an implementation detail of the verifier, which is exactly your point about not freezing "which fields it checks and how validation is sequenced" into a public contract.
Fixed in 40755f6.
There was a problem hiding this comment.
🟡 Issue: Avoid publishing two storage-seam evaluators as the general API
What's wrong
The new public exports bake storage’s current routing seam into the package API. That increases compatibility burden and makes the rule split itself a contract, even though the implementation comments say these entries exist because storage currently holds only persisted row operands.
Example
A consumer now has to choose between the late-tombstone and same-sequence half-rules, provide different evidence shapes, and map their decision unions. That sequence dispatch and mapping is the storage seam’s concern, not an inevitable public domain abstraction.
Suggested direction
Keep the shared authority logic in core, but expose a smaller, more canonical boundary: either an internal module for storage or one tombstone-advance adapter over the persisted applied row. Avoid making two migration-seam rule fragments and their specialized operand types part of the broad public surface.
Confidence note
This assumes system-record-v1 is the externally consumed public barrel, which the package export tests appear to enforce.
For Agents
Check whether storage can consume a core-internal subpath or a narrower package-private export. If this must cross a package boundary, consider replacing the two exported half-rule entries with one storage-facing tombstone advancement adapter that owns sequence dispatch and exposes one stable operand contract. Preserve current storage outcomes and update the export tests accordingly.
There was a problem hiding this comment.
🟡 Issue: Avoid publishing storage-specific tombstone seam adapters as public API
What's wrong
The PR exposes the new late-tombstone and same-sequence tombstone adapters through the public system-record barrel. Those APIs appear to be designed around storage’s persisted-row gap rather than a general consumer model, so they lock a one-off internal coordination problem into the published package surface. That makes future cleanup harder because renaming, merging, or replacing these adapters becomes a compatibility concern.
Example
evaluateAgentProfileSameSequenceTombstoneAdvanceV1 is documented as an entry for “a caller holding a persisted applied row”, and storage is that caller. Publishing it from system-record-v1 makes that internal storage/core coordination shape part of the external API contract.
Suggested direction
Keep the cross-package seam internal if it only exists so storage can avoid reimplementing core authority logic, or reframe it as a general public authority evaluator over persisted authority facts. The current shape exports two very narrow special-case functions and their bespoke operand types, which increases the public API surface without a clear reusable abstraction.
Confidence note
This assumes @origintrail-official/dkg-core/system-record-v1 is intended as a stable public surface; packages/core/package.json exports it and the package-export tests pin the exact runtime/type exports.
For Agents
Review packages/core/src/system-record-objects-v1.ts and storage’s imports from @origintrail-official/dkg-core/system-record-v1. Either keep these tombstone adapters on an internal core subpath used by storage, or replace the two one-off public entries with a more general authority-evaluation model that is worth publishing. Preserve storage behavior and keep the package export test aligned with the chosen boundary.
There was a problem hiding this comment.
🟡 Issue: Avoid publishing storage-specific tombstone seams as first-class API
What's wrong
The new exports turn two very narrow integration seams into public API. That leaks the storage state machine into the core package surface and makes future authority-rule changes harder, because callers can start depending on these partial entry points rather than one canonical model.
Example
External consumers now see evaluateAgentProfileHeadAdvanceV1, evaluateAgentProfileLateTombstoneAdvanceV1, and evaluateAgentProfileSameSequenceTombstoneAdvanceV1, and must know storage-specific sequence/disposition preconditions to choose among them.
Suggested direction
Prefer a single core-owned persisted-state evaluator, or an internal tombstone adapter module, that accepts the receiver row context and dispatches late vs same-sequence internally. That keeps the public surface canonical instead of growing one public function per storage seam.
For Agents
Look at packages/core/src/system-record-objects-v1.ts and the two storage call sites. Preserve current storage behavior, but try to expose one canonical persisted-row authority evaluation API, or keep the tombstone adapters internal to the core/storage integration instead of publishing both narrow seams.
| * predecessor, and the retained transition out of the tombstone's sequence. | ||
| */ | ||
| function tombstoneOfV1(predecessor: AgentProfileHeadObjectV1): AgentProfileHeadObjectV1 { | ||
| const shaped: Record<string, unknown> = { |
There was a problem hiding this comment.
🟡 Issue: The new fixture builds core domain objects through untyped casts
What's wrong
The test is proving a subtle authority seam, but its setup bypasses the type boundary in the exact place where the domain shape matters. That makes the test harder to maintain and makes fixture drift easier to hide behind casts.
Example
A future fixture edit can add or remove a head field inside the Record<string, unknown> object and still satisfy TypeScript because the final line asserts the whole object into AgentProfileHeadObjectV1. The test then depends on runtime failures or unrelated assertions to catch a malformed fixture.
Suggested direction
Move the object-shape work behind a typed builder that returns a validated tombstone head. That would delete most of the Record<string, unknown>, manual delete, as unknown as, and as never noise from the test cases themselves.
Confidence note
The repo already uses casts in some fixture code, but this new test adds a concentrated cluster of them around one domain object builder, so the concern is the new fixture boundary rather than any single cast.
For Agents
Create or reuse a typed tombstone-head fixture builder in the storage test helpers. Keep any unavoidable unsafe cast inside that helper after a codec assertion, and have the test body work with typed AgentProfileTombstoneHeadObjectV1/AgentProfileHeadObjectV1 values. Preserve the same candidate/predecessor/transition relationships and the current assertions.
There was a problem hiding this comment.
🟡 Issue: The new seam fixture bypasses the type model with casts and mutation
What's wrong
The test is about subtle authority-state shapes, but the fixture code constructs those shapes through untyped mutation and broad casts. That makes the tests harder to maintain and weakens compile-time feedback exactly where the setup needs to be clearest.
Example
If the tombstone head shape gains a required field, this helper still compiles because of the final cast; the failure would move to a runtime validator or a later assertion instead of the fixture boundary.
Suggested direction
Replace the ad-hoc clone/delete/cast fixture construction with typed test factories that validate the object shape once and return domain-specific types.
For Agents
Create typed fixture builders near the existing authority-verdict helpers, for example makeTombstoneHeadFromPredecessor, makeLateTombstoneFacts, and makeSnapshotWithHeadVersion. Keep the current scenarios and expected outcomes, but push validation/casting into one small boundary instead of every call site.
There was a problem hiding this comment.
Taken, with the cast confined and validated rather than removed.
tombstoneOfV1 now runs the built object through assertAgentProfileHeadObjectV1 before returning, and returns AgentProfileTombstoneHeadObjectV1 rather than a bare head — so the exact drift you describe (a field added or removed inside the loose record) fails at the fixture boundary with the codec's own message, not at a runtime validator downstream or an unrelated assertion. The reasoning is written at the builder so the next reader knows the cast is deliberate and where its guard is.
I stopped short of the full set of factories (makeLateTombstoneFacts, makeSnapshotWithHeadVersion). Two of those wrap machinery whose shape is currently load-bearing in the test body — the ahead-of-candidate snapshot exists precisely to show a state the default driver cannot build, and hiding that construction behind a helper would bury the point the test is making. I would rather keep that one explicit and revisit when the second seam adds a second caller and the shared shape is actually known.
Fixed in b2f08c1.
There was a problem hiding this comment.
🟡 Issue: Replace the scattered as never fixture plumbing with typed builders
What's wrong
The new 480-line seam test bypasses TypeScript at most of the interesting boundaries. That undercuts the PR's otherwise careful operand modeling: the tests become harder to refactor, and API-shape drift can be hidden behind casts instead of failing where the fixture is built.
Example
If AgentProfileLateTombstoneEvidenceV1 changes shape, calls like line 343 can keep compiling because the object is cast through never; the test will no longer give maintainers a typed fixture boundary for the new public API it is exercising.
Suggested direction
Give the test helpers typed construction APIs and validate mutated fixtures at the boundary. A small number of localized casts is much easier to audit than casts spread across every call site.
For Agents
In system-record-late-tombstone-seam-v1.test.ts, introduce typed fixture builders for storage drive input, late-tombstone evidence, lineage, and mutated heads. Contain any unavoidable unsafe cast inside one builder immediately followed by the existing codec/assertion, then call the public API without as never.
There was a problem hiding this comment.
🟡 Issue: Stop erasing the seam types in the new tests
What's wrong
The new test file repeatedly uses as never at the exact boundaries it is supposed to exercise. That makes the suite harder to maintain because type drift in the public late-tombstone API, storage driver input, or lineage fixture shape will be hidden by the casts instead of surfaced where the fixtures are built.
Example
evaluateAgentProfileLateTombstoneAdvanceV1(..., { retainedTransition: ..., tombstonePredecessor: ... } as never, lineageFor(binding) as never) bypasses the exact exported types this PR is adding. If the evidence or lineage contract changes, these calls can keep compiling while no longer representing a real consumer shape.
Suggested direction
Concentrate unsafe fixture construction behind typed helpers or satisfies checks, and let the test bodies call the public API with real exported types.
For Agents
In packages/storage/test/system-record-late-tombstone-seam-v1.test.ts, replace call-site as never usage with typed fixture builders for AgentProfileLateTombstoneEvidenceV1, AgentProfileAppliedTransitionV1[], and the storage driver request. Keep unavoidable loose object shaping inside validated builders like tombstoneOfV1, then return strongly typed values to test bodies. The existing scenarios and expected outcomes should remain unchanged.
There was a problem hiding this comment.
Re-measured rather than re-declined, since you have added specifics.
The enumeration is accurate: the remaining casts are the driver call, reserved-state quads, the issued candidate, the lane binding, and the evidence/lineage arguments. Two of those are now typed (evidence via satisfies AgentProfileLateTombstoneEvidenceV1, lineage via a declared return type), and the head builder validates through the codec before returning.
What I re-measured, because it decides whether a typed helper buys anything here: packages/storage/tsconfig.typetests.json has an explicit three-file include list, and this test is not in it. I tried adding it — it pulls in pre-existing type errors from two shared fixture helpers this change does not own. So a typed builder in test/helpers/ would give the test bodies nicer shapes while no compiler ever checks them, which is the same category of comfort as the satisfies I already added: real for readers, inert as enforcement.
That is why the enforcement went where a compiler does run — four conditional-type pins on the new evidence types in system-record-package-export-v1.types.ts, each proven by mutation. Drift in the public contract fails there.
The genuinely useful version of your suggestion, which I am filing rather than doing here: put the storage test tree into a typecheck program. That fixes the fixture helpers' existing errors and makes every one of these casts load-bearing at once — a package-level change with its own before/after, not something to attach to a security fix. Recorded with the owner.
There was a problem hiding this comment.
🟡 Issue: Cast-heavy seam fixtures obscure the tested model
What's wrong
These tests are the main executable documentation for the new public late-tombstone entry, but the setup repeatedly bypasses the type boundary. That weakens the clarity of the model the PR is trying to make explicit and makes future fixture changes harder to reason about.
Example
tombstoneOfV1 clones an active head, deletes fields, casts it through unknown, then later tests also cast registry inputs and summaries. A future required fixture field can compile through the casts, leaving the reader to infer the real invariant from comments and runtime validation order.
Suggested direction
Move the object surgery and unavoidable codec-boundary casts into one small, typed fixture factory. The test bodies should read as domain scenarios, not repeated bypasses of the type system.
Confidence note
The test file may be following local fixture conventions, but this PR adds enough new cast-heavy setup that it is worth tightening before this becomes the template for the seam.
For Agents
Extract a typed late-tombstone fixture builder near the existing authority-verdict helpers. It should return validated candidate, predecessor, retained transition, lineage, snapshot, and facts objects so individual tests do not need broad casts. Preserve the scenarios and expected decisions.
There was a problem hiding this comment.
Third raising, so here is the re-measurement rather than the same answer.
Since the last round the fixture surface actually shrank: the evidence arguments carry satisfies AgentProfileLateTombstoneEvidenceV1, the lineage helper declares its return type, and tombstoneOfV1 validates through the codec before returning a typed tombstone head — so a drifted fixture fails at construction with the codec's own message, which was the concrete harm you named.
What has not changed is the thing that decides whether a typed builder buys enforcement, and I re-checked it this round: packages/storage/tsconfig.typetests.json still has an explicit three-file include list and this test is not in it. A typed factory would give the test bodies nicer shapes that no compiler checks. Your phrase "a future required fixture field can compile through the casts" is exactly right — and it stays true with or without the factory, because nothing typechecks this file either way.
So the enforcement went where a compiler runs, and this round added more of it: the seam's decision contract is pinned by conditional types in the export-types fixture (one of the few test files a tsc --noEmit gate compiles), each proven by mutation.
The version of your suggestion that would actually bite — putting the storage test tree into a typecheck program — is filed with the owner. It requires fixing pre-existing errors in two shared fixture helpers this change does not own, so it is its own piece of work rather than a rider on a security fix.
There was a problem hiding this comment.
🟡 Issue: Extract the duplicated cast-heavy tombstone fixture builder
What's wrong
The new tests duplicate a non-trivial domain fixture that relies on manual object surgery and unsafe casts. That is exactly the kind of helper that will drift when the head codec changes: one suite may update the deleted fields, defaults, or validation path while the other keeps the old shape.
Example
Both helpers clone an active predecessor, reshape it into a tombstone, zero projection fields, delete active-only fields, cast through unknown, assert the codec, and check the resulting state.
Suggested direction
Use one parameterized tombstone-head fixture helper for both seam suites instead of copying the object-shaping and validation sequence.
For Agents
Extract a shared test helper, likely under packages/storage/test/helpers/, that builds a validated tombstone head from a predecessor with optional version/overrides. Update both seam suites to use it and keep the existing fixture behavior unchanged.
There was a problem hiding this comment.
Adopted, and this one is squarely mine — I created the duplication in this round.
buildTombstoneHeadFromPredecessorV1 now lives in packages/storage/test/helpers/tombstone-head-fixture-v1.ts and both seam suites call it. The parameterisation you asked for is there: optional version, optional field overrides.
Your drift argument is the right one and it is sharper than "duplication is untidy", so I want to restate it in the terms this branch has been paying for: two copies of a fixture prove that they agree, never that either one is correct. A shared mistake passes both. And the specific rules this builder encodes are exactly the kind that move under a head-codec change — which active-only fields must be deleted, which projection counters must be zeroed, and that the result has to be re-validated rather than trusted. Had the two drifted, the failure would not have been loud: one suite would have kept building a shape the codec no longer accepts, and the codec's refusal reads like a domain refusal rather than a fixture one. Two seats on this branch have already lost a wall to exactly that confusion.
Two things stayed behind deliberately rather than being pulled into the helper:
The version-is-a-parameter reasoning moved WITH the builder, because it is a property of the builder rather than of either caller. isTombstoneBoundToPredecessorV1 requires the tombstone's version to be strictly greater than its predecessor's, not adjacent — so a builder hard-coding predecessor.version + 1 reaches exactly one version relation against any given applied row and makes the other two look unconstructible. That is not hypothetical: it cost a wall before it was parameterized, and the note is now where the next person will hit it.
The load-bearing coincidence stayed in the same-sequence suite, because it is a fact about that suite's chain rather than about the builder: the closure resolves the predecessor through the tombstone's own previousHeadDigest against the ancestry, while the mint graph supplies only the owned-subject table, and those agree only because the current head is a spread of the base. Moving that into the helper would have made it look like a general guarantee.
The five orphaned imports the extraction left behind are gone too. Both suites green, 23 tests, and the six lanes that pin lines into these files still resolve.
| // constant. It caught a neighbouring PR's ordinary-English comment within the | ||
| // hour of being live. Guards COMPOSE with a tripwire and never replace one, | ||
| // and this zero is cited as a premise in shipped artifacts, so replacing it | ||
| // would have silently falsified those citations. The routing keeps the zero by |
There was a problem hiding this comment.
🟡 Issue: The substring tripwire is steering production vocabulary
What's wrong
The new code doubles down on a brittle text scan and explicitly shapes naming around it. That makes the test artifact leak into production terminology, and future maintainers have to reason about forbidden substrings instead of domain boundaries.
Example
A legitimate storage comment like “no disposition is persisted here” would fail the tripwire, while a hand-rolled classifier using synonyms could pass unless it happens to include one of the listed literals.
Suggested direction
Replace the raw source.includes('disposition') guard with an AST/module-boundary assertion or a schema-level check. The test should encode ownership, not reserve ordinary English words across the storage source tree.
For Agents
Update authority-verdict-diff-projection-v1.test.ts to check the actual boundary: parse TS imports/references or inspect schema/codec declarations. Preserve the intent that storage has one core reader consumer and no local disposition producer, but stop making production names and comments dodge a raw substring.
There was a problem hiding this comment.
Half of this is right and is already covered; the other half is a deliberate standing decision, so I am holding the pin and escalating rather than changing it quietly.
Where you are right: a raw substring scan cannot see a hand-rolled classifier written with synonyms. That is true, and it is why this file does not rely on the scan alone. Alongside it are three targeted assertions added in this PR: the core reader is reachable from exactly one file, its total reference count is pinned (not just its call count — an alias survived the call-count version, so that gap is closed), and core's three classification literals are asserted absent from the package. Those are the ownership checks you are asking for.
Where I am not changing it, and why it is a decision rather than an oversight: the raw zero is a tripwire and those three are guards. A guard catches exactly what it names. The tripwire catches what nobody thought to name — and it earned that description this week, catching a neighbouring PR's ordinary-English comment within the hour of being live. Guards compose with a tripwire; they do not replace one. This particular zero is also cited as a premise in shipped artifacts (slice design, PR bodies, module docs), so replacing it silently falsifies those citations.
Your "steering production vocabulary" point is the real cost and I do not dispute it — this PR paid it, renaming a reason member and rewording two comments. The judgement that the cost is worth paying is a standing one on this workstream, made explicitly and recently, with a live incident behind it. It is not mine to reverse in a review reply, so I am recording your objection and raising it with the owner rather than acting unilaterally. If it is reversed, the AST/boundary form you describe is the right replacement and I would implement it against the same three properties.
An AST check would also not have caught the neighbour's case, which was a comment.
There was a problem hiding this comment.
🟡 Issue: Use AST-level source checks instead of counting docblock text
What's wrong
This guard is trying to enforce an architectural boundary, but it does so by counting raw identifier text, including comments. That makes ordinary documentation edits look like boundary violations and keeps adding bespoke regex machinery where a small structural source check would be clearer and less fragile.
Example
Changing only the explanatory docblock that mentions deriveAgentProfileAuthorityDispositionV1 would change the occurrence count and fail this guard, while an AST-equivalent import/call structure would be unchanged. Conversely, raw substring matching cannot distinguish a real call from text in a comment without more ad-hoc filtering.
Suggested direction
Parse the source with the TypeScript compiler API or an existing project parser and assert import/call/string-literal facts structurally, rather than making comments and incidental text part of the contract.
Confidence note
The repository already has source-harvesting tests, but this new assertion goes further by making a docblock mention part of the expected occurrence count, which is the maintainability risk called out here.
For Agents
In packages/storage/test/authority-verdict-diff-projection-v1.test.ts, replace raw substring/reference counts with a small TypeScript AST inspection that resolves imports from @origintrail-official/dkg-core/system-record-v1 and counts actual call expressions or aliases. Preserve the invariant: exactly one storage consumer, no local restatement of the three disposition literals.
There was a problem hiding this comment.
Recorded, and this is now the second round on the same point, so let me be precise about which half I am acting on.
The half I fixed: you are right that making a docblock mention part of an occurrence count is fragile, and that was my addition, not the pre-existing tripwire. The reference count now covers import + docblock + call and is documented as such, but the underlying complaint stands — I would rather it keyed on structure. It is on my list if the surrounding decision changes.
The half I am still not acting on unilaterally: replacing the raw source.includes('disposition') walk. That pin is a standing decision on this workstream, reaffirmed this week after it caught a neighbouring PR's ordinary-English comment within the hour of going live, and it is cited as a premise in shipped artifacts. I have raised your objection with the owner rather than reversing it in a review reply, and I have said the same on the earlier round.
One measured point against the AST proposal as a replacement rather than an addition: an AST check inspects imports, calls and string literals. The case this pin actually caught was a comment. So an AST version would have been green through the exact incident that justifies the pin. That does not make your maintainability concern wrong — it makes AST the right tool for the ownership claim and the wrong tool for the tripwire claim, which is why the three targeted assertions ship alongside rather than instead.
| * clause governs today and the outcome is a RETRY. The accepting and stale arms | ||
| * become reachable unchanged the moment a producer supplies that transition. | ||
| */ | ||
| function classifyLateTombstoneAdvance( |
There was a problem hiding this comment.
🟡 Issue: Pull the late-tombstone adapter out of the 1.5k-line next-state file
What's wrong
This PR adds another 100+ lines to an already sprawling state-machine file. The added code is a distinct storage/core boundary adapter, not general next-state orchestration, so keeping it inline worsens file sprawl and makes future authority seams more likely to accrete in the same place.
Example
The new helper owns three concepts at once: deriving a core authority disposition, building the storage-to-core evidence shape, and translating SystemRecordAuthorityDecisionV1 into TombstoneAdvance. None of that needs to live in the middle of the already-large derivation state machine.
Suggested direction
Extract this cross-package seam into a focused module with a small exported classifier. The central next-state file should dispatch to it, not absorb another specialized adapter and decision mapper.
For Agents
Move classifyLateTombstoneAdvance and the core-decision-to-storage-outcome mapping into a focused internal module, for example system-record-late-tombstone-advance-v1-internal.ts. Keep classifyTombstoneAdvance as a short dispatch and prove equivalence with the existing late-tombstone seam and join tests.
There was a problem hiding this comment.
Agreed in principle, deferred deliberately, and I would rather say why than leave it silent.
The adapter really is three concepts — deriving the authority classification, building the evidence, and mapping SystemRecordAuthorityDecisionV1 onto TombstoneAdvance — and it does not belong in the middle of the derivation state machine long-term.
Two reasons it is not moving in this PR:
It would move again immediately. The second seam (the active/quarantine classifier at classifyAuthorityAdvance) is the next slice and routes through the same core evaluator. Extracting a module shaped around one caller, then reshaping it when the second lands, is worse than extracting once with both in view — the shared surface is not known yet, and I would be guessing at it.
Round 2 already moved a file's worth of code, and every extraction in this package has so far moved line-anchored citations that other suites pin. That has cost three separate correction passes in this PR, one of which CI caught rather than my local run. Doing another structural move in the same change, on top of a security fix, is where the fourth one gets missed.
Recorded for the active-seam slice, which touches this file anyway.
There was a problem hiding this comment.
🟡 Issue: Extract the late-tombstone adapter instead of growing the monolithic state derivation
What's wrong
This PR adds a feature-specific seam, two new deferral reasons, core disposition adaptation, and core verdict mapping directly into a central derivation file that is already over 1.5k lines. Even if the behavior is right, this is spaghetti growth in the highest-traffic module: the late-tombstone policy is not a natural extension of materialization planning, and it makes the storage state machine harder to scan.
Example
A reader trying to understand tombstone replacement now has to hold the storage materialization state machine, core authority-disposition derivation, late-tombstone evidence availability, and core-to-storage verdict mapping all inside the already-large system-record-next-state-v1-internal.ts flow.
Suggested direction
Move classifyLateTombstoneAdvance and the core-decision mapping into a dedicated authority/tombstone adapter with a narrow input/output contract. That lets the central derivation file state the dispatch rule without becoming the home for every ADR seam.
For Agents
Extract the late-tombstone authority adapter from packages/storage/src/system-record-next-state-v1-internal.ts into a focused internal module or pure helper. Preserve the current outcomes for active, dirty, and tombstone applied rows, and keep classifyTombstoneAdvance as a small dispatcher plus same-sequence arithmetic branch.
There was a problem hiding this comment.
Standing answer, re-measured — and the measurement has changed in your favour on one point.
The deferral still holds and for the same reason: the active/quarantine seam is the next slice, routes through the same evaluator, and will reshape whatever module this becomes. Extracting around one caller and reshaping around two is worse than extracting once with both in view.
What I re-measured since the last round: this PR has now moved line-anchored citations four separate times, and every extraction was a cause. One of those escaped my local run and was caught by CI. That is direct evidence for doing the structural move once, deliberately, in a change whose whole subject is the move — rather than as a third structural edit inside a change that also carries a security fix.
Where you have gained ground: the file did grow again this round. The seam accounting has already been extracted to its own helper, and the adapter is the remaining piece. It is filed against the active-seam slice, which touches this file anyway, with your input/output contract sketch attached.
There was a problem hiding this comment.
🟡 Issue: Move the tombstone routing adapter out of the next-state monolith
What's wrong
The PR adds another specialized authority seam directly into an already sprawling storage derivation file. The code is factored into helpers, but the ownership boundary is still wrong: this file now owns tombstone authority routing, authority-disposition gating, core evidence adaptation, and decision mapping. That is structural growth in a hot, general-purpose path rather than an isolated extension point.
Example
The late and same-sequence paths now each encode their own sequence gate, classification precondition, evidence shaping, and authority-decision-to-storage-outcome mapping. A future change to how undecided applied rows are handled would have to rediscover both sites and understand why one maps tombstoned rows to undecided-authority-classification while the other maps them to same-sequence-tombstone-conflict.
Suggested direction
Keep classifyTombstoneAdvance as a small dispatcher and move the new core-routing/mapping logic into a dedicated tombstone authority routing module or policy helper. That would make the new behavior easier to audit and keep storage’s already-large next-state derivation from becoming the place every authority seam accumulates.
For Agents
Extract a storage-side tombstone authority routing adapter from system-record-next-state-v1-internal.ts. Preserve the current sequence dispatch and storage outcomes, but centralize the repeated disposition gate, predecessor evidence construction, applied-row projection, and core-decision mapping in one focused helper/module. Prove the extraction with the existing late/same-sequence seam tests and the verdict-diff join pins.
| * :354. A file-contains pin would have stayed green through all of it; only a | ||
| * line-anchored one says which sites moved and by how much. | ||
| */ | ||
| export const CORE_REJECT_REASON_SITES_V1: Readonly<Record<string, readonly number[]>> = { |
There was a problem hiding this comment.
🟡 Issue: Stop making absolute source line numbers part of the test contract
What's wrong
The PR demonstrates the maintenance problem it doubles down on: inserting one core entry shifts many unrelated citation numbers and forces broad test-data churn. Line positions are incidental implementation details, so pinning them makes refactors noisy and couples documentation movement to test failures.
Example
The new reason at line 63 is pinned to [438]. Moving a docblock above that branch would break this fixture even if the branch and reason literal are unchanged, while an AST/symbol-based pin could still prove the same producer exists without coupling to incidental line position.
Suggested direction
Use stable structural anchors such as function names, exported helper IDs, or harvested literal-producing branches. Generate/display current line numbers for debugging, but avoid storing them as the assertion target.
For Agents
Replace absolute line-number maps in the verdict-diff helpers with structural site IDs: function name plus reason literal, or a harvested source span that is checked by contains/AST position at runtime. Keep line numbers as generated diagnostics if useful, but do not make them hand-maintained test data.
There was a problem hiding this comment.
The cost you describe is real and I paid it four times in this PR. The pin still stays, and the reason is on the record rather than aesthetic.
You are right that inserting one entry shifted many citations and forced churn. That happened three times here, and once it escaped my local run and was caught by CI.
Why line anchoring exists here specifically: the earlier form of this pin was file-contains, and it went green on a citation that pointed at a closing brace. "The file contains this string" is true for any line number, so the pin whose job was keeping citations followable was green on an unfollowable one. Line anchoring is the correction to a defect that actually shipped, not a stylistic preference.
Where your suggestion is already the practice, and where I moved further toward it this round: the entries that matter most pair a line with a contains string, so they are self-correcting — I re-resolved them BY CONTENT rather than by arithmetic twice today, and content resolution is what caught that an arithmetic shift would have landed two citations on real-but-wrong lines (a fork-resolution comparison and a lineage field list) where no assertion would have noticed. That is the same argument you are making, applied inside the current shape.
What I think the right change is, and why it is not this PR: structural site IDs (function name + reason literal) for the harvested maps, with line numbers regenerated as diagnostics. That is a change to a shipped instrument several artifacts cite, so it needs its own change and its own before/after rather than riding a security fix. Filed with the owner.
There was a problem hiding this comment.
🟡 Issue: Committed line-number site maps make ordinary refactors expensive and brittle
What's wrong
The harness is treating physical source layout as part of the maintained contract. That works as a tripwire, but it makes small structural improvements create large fixture churn and discourages decomposing the already-large authority file because every insertion shifts unrelated expected values.
Example
Extracting rejectInvalidHeadClockV1 consolidates two checks, but the fixture still needed broad manual edits such as transition-equivocation moving from [189, 221, ...] to [211, 243, ...]. That is layout churn, not semantic fixture data.
Suggested direction
Replace the manually maintained line arrays with generated harvest metadata, or pin producer identities using stable rule/function labels. The tests can still report current line numbers without making every insertion in the core file cascade through fixture updates.
For Agents
Refactor the verdict-diff fixtures so line numbers are generated by the harvest at test time and used only in failure output. Keep expected reason literals and ambiguity semantics as committed data; identify producers by stable semantic anchors such as enclosing function or explicit rule id instead of committed source line arrays.
There was a problem hiding this comment.
Re-measured, and your example is now the strongest version of this argument — I am carrying it to the owner rather than answering it again.
You are right that extracting rejectInvalidHeadClockV1 was a semantic improvement whose fixture cost was pure layout churn. I can add the count: across this PR the line-anchored citations have now been re-resolved six times, and one of those escaped my local run and was caught by CI.
What I have done inside the current shape: citations are re-resolved BY CONTENT rather than shifted, which is the same idea as your stable-anchor proposal applied at the repair step instead of the storage step. It has twice caught citations that arithmetic would have landed on real-but-wrong lines where no assertion would have noticed.
Why I am still not changing the storage form here: the line anchoring replaced a file-contains pin that went green on a citation pointing at a closing brace, and several shipped artifacts cite the current form as a premise. Swapping it is a change to a shipped instrument, and it belongs in a change whose subject is that instrument — with its own before/after — rather than inside a security fix that has already turned over three times.
Filed with the owner, with your generated-metadata form named as the target.
There was a problem hiding this comment.
🟡 Issue: Stop pinning semantic provenance to physical line numbers
What's wrong
This fixture turns source layout into semantic data. The PR demonstrates the cost by updating a large map of line numbers after adding the tombstone slice, and adding more rows deepens the coupling. That makes structural cleanup noisy and discourages the exact decomposition this PR now needs.
Example
The unchanged reason stable record key changed moved from [137, 553] to [325, 1059] just because this PR inserted code above it. A future extraction of the tombstone rules would force another broad fixture rewrite even if no observable reject codomain changed.
Suggested direction
Replace the line-number arrays with stable semantic site identifiers generated by the harvest. If exact provenance matters, make the producer id stable across insertions instead of using physical line positions.
For Agents
Refactor the authority verdict fixture/harvest to compare stable producer identities instead of raw line numbers: function path plus reason literal, AST path, or explicit test-only site ids. Preserve the current checks for new/reworded reject literals and ambiguous producers, then prove a pure helper extraction does not require changing this data table.
| * which this rule consults, and a caller forced to fill a required `nowMs` it | ||
| * does not have will invent one. | ||
| */ | ||
| export interface AgentProfileLateTombstoneEvidenceV1 { |
There was a problem hiding this comment.
🟡 Issue: The new late-tombstone evidence type contract is only imported, not verified
What's wrong
This change relies on a specific public type boundary: callers must not invent a standalone clock, and the retained transition must carry its verifying clock as one field. The current tests cover runtime decisions, but they do not prove the exported type surface enforces that boundary or that the runtime snapshot allowlist stays aligned with it. That leaves a real API-contract regression able to pass green while downstream consumers are told an unsafe or unusable shape is valid.
Example
A compile-only regression test should fail if AgentProfileLateTombstoneEvidenceV1 ever accepts a top-level nowMs or acceptedTransition, or if AgentProfileLateTombstoneRetainedTransitionV1 stops requiring both transition and nowMs. Today the added tests would still exercise runtime calls because they cast evidence to never.
Suggested direction
Add compile-only assertions for the published evidence shapes and, ideally, a small runtime exactness check for the snapshot allowlists.
For Agents
Look at packages/core/test/system-record-package-export-v1.types.ts and add type-level pins for the new late-tombstone evidence types: exact allowed keys, no top-level clock/transition, and required transition plus nowMs inside retainedTransition. Consider a runtime/harvest assertion that the snapshotLateTombstoneEvidenceV1 allowlist matches the interface, mirroring the existing head-advance evidence optional check.
There was a problem hiding this comment.
Correct, and fixed — this was the sharpest finding of the round because it named a gap my own tests could not have caught.
You were exactly right that the runtime cases could not cover it: they cast their fixtures, so a widened type would have left them green. Four conditional-type pins now live in packages/core/test/system-record-package-export-v1.types.ts, which is one of the very few test files a tsc --noEmit gate actually compiles in this repo — the reason that file was the right home rather than the seam test.
What they pin, in your terms:
AgentProfileLateTombstoneEvidenceV1has no top-levelnowMs- it has no bare
acceptedTransition - its key set is exactly
{ tombstonePredecessor, retainedTransition }, checked in both directions AgentProfileLateTombstoneRetainedTransitionV1requires bothtransitionandnowMs
Each was proven by mutation rather than assumed. I widened the interface three ways — adding a top-level clock, adding a bare transition, making the paired clock optional — and each time the gate failed with error TS2322: Type 'true' is not assignable to type 'never', with a green restore control afterwards. A type pin that has never been seen to fail is indistinguishable from a comment, and in this repository that is not hypothetical: no test directory is in any tsc program, so type-level guards written in ordinary .test.ts files silently pass.
The runtime allowlist half of your suggestion is not done. snapshotLateTombstoneEvidenceV1 builds its optional list inline, and the existing harvest pin that checks that alignment is written for the head-advance snapshot specifically. Extending it to a second snapshot function is a reasonable follow-up; I did not want to widen that harvester in the same change that fixed the pin it already had, which this PR broke and repaired.
Landed in 1b23465.
There was a problem hiding this comment.
🟡 Issue: The new evidence boundary lacks runtime negative tests for rejected shapes
What's wrong
The PR makes the evidence shape part of the safety contract: no top-level clock or bare transition, and a retained transition must carry both the transition and its verifying clock. Type-only tests do not protect JavaScript consumers or casted inputs, and the behavior tests only exercise valid shapes, so a runtime allowlist regression would still have little coverage.
Example
A JS caller, or a TypeScript caller using as never, can call evaluateAgentProfileLateTombstoneAdvanceV1(candidate, { tombstonePredecessor, acceptedTransition: transition, nowMs: NaN } as never, lineage). The contract says those top-level fields are not accepted evidence, but the current tests would not fail if the runtime allowlist accidentally started accepting them.
Suggested direction
Complement the compile-time evidence-shape assertions with runtime boundary tests for extra and incomplete evidence records.
Confidence note
I could not run the targeted suite because pnpm is not installed in this sandbox, so this is based on diff and repository test inspection.
For Agents
Add negative runtime cases near the late-tombstone seam tests for extra top-level evidence fields and malformed retainedTransition records. Assert they fail at the public entry boundary, then keep the existing positive cases proving valid { retainedTransition: { transition, nowMs } } evidence can still reach accept and stale.
There was a problem hiding this comment.
Taken, done, and your finding generalised to the entry this round adds — which is where it earned the most.
For the late-tombstone entry, three runtime cases now assert the refusal at the public boundary rather than at the type level: a bare transition with a top-level clock beside it (the pre-refactor shape that once turned a stale into an accept), a retained transition with no clock, and a clock with no transition. Proven failable rather than assumed live — widening the accepted key list turns all three red, and the type pins alone stay green through it, which is your point exactly.
Your confidence note is worth answering directly: you could not run the suite, and the finding was still correct, because it was derived from what the tests could observe rather than from what they reported. That is the better instrument.
The generalisation is where this got useful. This round adds a second exported entry with its own exact-record evidence and its own operand shape, and the confusable mistake there is not a JavaScript caller inventing a field — it is a TypeScript caller passing the other entry's evidence object. The two shapes differ by one optional key, both compile at every as never boundary, and the wrong one would silently supply an operand the same-sequence rule does not read. Runtime cases now refuse:
- late-tombstone evidence (
{ tombstonePredecessor, retainedTransition }) handed to the same-sequence entry - a top-level clock beside the predecessor
- an applied-row operand carrying an extra key, or missing one
- an applied-row
statusoutside the persisted union
The first is the one I would not have written without this thread. The rest follow from the same principle: an exact-record snapshot is only a boundary if something proves it refuses.
One distinction the new cases hold to, because it is load-bearing at this entry: a malformed operand throws, while a well-formed operand the rule does not answer for — a non-tombstone candidate, a candidate at the wrong sequence, a quarantined or shadow-dirty applied row — returns a reject. Those four are covered by their own row. Collapsing the two categories would either turn a legitimate "not my rule" into a crash, or turn a malformed input into a verdict.
| // one docblock mention, and the single call. A fourth is either a second | ||
| // consumer or an alias, and both are this seam's problem. | ||
| const references = importing.flatMap( | ||
| (f) => f.source.match(/deriveAgentProfileAuthorityDispositionV1/g) ?? [], |
There was a problem hiding this comment.
🟡 Issue: Replace raw source-count pins with semantic checks
What's wrong
The test helpers are becoming a brittle source-code policy engine. Counting substrings and hand-pinning line numbers creates high-churn coupling to comments and file layout, which makes the codebase harder to refactor and obscures the actual invariant the tests are meant to protect.
Example
Adding a harmless explanatory comment mentioning deriveAgentProfileAuthorityDispositionV1 would fail the reference count. Inserting a helper above the core decision sites forces updates to many numeric citation arrays even when behavior and structure are unchanged.
Suggested direction
Keep the architectural guard, but make it track the thing you mean: whether storage derives authority classification locally or calls the canonical core reader. Do not make comments, aliases, or physical line numbers part of the contract.
For Agents
Replace raw substring and line-number pins with a semantic harvest. Prefer an AST-based check for imports/calls and generated source-site metadata keyed by function/reason, or move these observations into a generated snapshot so ordinary refactors do not require hand-maintaining dozens of coordinates.
There was a problem hiding this comment.
Ruled, and I can now give you the disposition rather than only my reasoning.
The raw substring walk is upheld, on four recorded grounds. The one that decides it is the one your own example raises: the incident this pin exists for was an ordinary-English comment in a neighbouring PR, caught the same day the pin went live. An AST or module-boundary assertion is blind to comments by construction, so the proposed replacement is strictly weaker on the exact class that motivates the guard. "Steers production vocabulary" is the design rather than a defect — the zero is cited as a premise in shipped artifacts, and the constraint enforces a package boundary at the cheapest available layer, a cost knowingly paid twice in this PR.
The half you are right about is already covered, and composes with it: synonyms and semantic equivalents pass a substring scan, which is why the three targeted assertions ship alongside — one file may reference the core reader, its total reference count is pinned, and core's three classification literals are asserted absent from the package.
And there is an open door: an AST/module-boundary assertion is welcome as a fourth addition if you want it built. It is not accepted as a replacement. If you would like it, say so on this thread and I will add it against the same three properties.
On the line-number half: I share the churn complaint and have moved as far toward you as this change can carry — citations are re-resolved by content rather than shifted, which caught two that arithmetic would have landed on real-but-wrong lines where no assertion would have noticed. Structural site IDs for the harvested maps are the right end state and are filed with the owner as their own change, since several shipped artifacts cite the current form.
There was a problem hiding this comment.
🟡 Issue: Raw source-text counts make comments part of the design
What's wrong
This couples maintainability to incidental spelling. It also incentivizes awkward production names and comments just to satisfy substring tripwires, which makes ordinary refactors noisy and less direct.
Example
Adding a second explanatory comment mentioning deriveAgentProfileAuthorityDispositionV1, or moving the call behind a local alias while preserving one runtime consumer, would fail this test even though the architecture has not worsened.
Suggested direction
Replace raw source.includes/regex occurrence pins with a more structural check: one runtime import/call site, no storage-side reimplementation of the classification mapping, and behavior covered through the classifier path.
For Agents
Rework authority-verdict-diff-projection-v1.test.ts around lines 163-203. Assert the architectural boundary with an AST/import-level check or a small dedicated storage helper, and avoid counting doc prose or raw substrings as semantic evidence.
There was a problem hiding this comment.
Ruled and upheld, so this is the citation rather than a fresh argument — plus one thing that did change.
The raw walk stands on four recorded grounds, and the decisive one is your own example class: the incident this tripwire exists for was an ordinary-English comment in a neighbouring PR, caught the day it went live. An AST or import-level check is blind to comments by construction, so the proposed replacement is strictly weaker on exactly the class that motivates the guard. The half you are right about — synonyms and aliases pass a substring scan — is what the three targeted assertions alongside it cover.
What changed this round: your local alias example is now covered. The addition pins TOTAL references rather than call sites, precisely because const alias = fn; is a reference and not a call — that gap was real and it is closed.
And the door is open, as offered before: an AST/module-boundary assertion is welcome as a further addition if you want it built. It is not accepted as a replacement. Say so on this thread and I will add it against the same three properties.
On the line-number half: agreed and acted on in the production module this round — six source citations there are now symbol names, several of which were already stale. The fixture-side change to generated metadata is filed with the owner as its own piece of work, since several shipped artifacts cite the current form.
| } | ||
|
|
||
| it('is STALE when the retained transition binds the tombstone, ACCEPT when it does not', () => { | ||
| const bound = evaluateAgentProfileLateTombstoneAdvanceV1( |
There was a problem hiding this comment.
🟡 Issue: The new late-tombstone entry is not tested with a wrong retained transition
What's wrong
The added tests prove the happy stale/accept directions and the missing-transition retry path, but they always pair the supplied transition with a lineage built from that same transition. The changed API contract says the supplied retained transition must exactly match the accepted lineage entry; without a present-but-wrong transition case, a regression that stopped comparing the transition digest or sequence against acceptedTransitionLineage[candidateSequence] could still pass these tests.
Example
A focused regression case could call evaluateAgentProfileLateTombstoneAdvanceV1(candidate, { tombstonePredecessor, retainedTransition: { transition: binding, nowMs } }, lineageFor(retained)) and expect reject | late tombstone requires the exact retained resurrection transition. Without the digest/lineage comparison, that kind of input can fall through to stale or accept.
Suggested direction
Add a negative public-entry test where a well-formed retained transition is supplied with a lineage entry whose digest does not match it, so the new exact-retained-transition contract is proven independently of the missing-transition path.
Confidence note
The new standalone entry shares the lower-sequence rule with the full evaluator, so some aggregate verdict-diff coverage may fail indirectly, but the direct public-entry tests shown here do not exercise this contract.
For Agents
Add a direct test in packages/storage/test/system-record-late-tombstone-seam-v1.test.ts near the bound/unbound late-tombstone entry tests. Keep the candidate, predecessor, and clock valid, but deliberately mismatch evidence.retainedTransition.transition against acceptedTransitionLineage[candidateSequence]; assert the exact retained-transition reject reason.
There was a problem hiding this comment.
You were right, and a mutant proved it before I changed anything.
I deleted the digest comparison outright — computeAgentProfileAuthorityTransitionDigestV1(transition) !== retained.transitionDigest replaced with false — and the whole lane stayed green. Exactly your diagnosis: every row paired the supplied transition with a lineage built from that same transition, so the comparison always matched. The suite was proving the transition was PRESENT while claiming to prove it was the RIGHT one.
The new row is the shape you specified — well-formed transition, real predecessor, real clock, against a lineage retaining a different transition — and it demands reject | late tombstone requires the exact retained resurrection transition specifically, rather than accepting whichever guard fires first. Re-running the same mutant now kills it on exactly that row.
This is the third time in this PR a review comment has found a check of mine that could not fail. Landed in a6cefef.
| .reduce((sum, [, v]) => sum + v, 0)).toBe(total); | ||
| // And the counterfactual is labelled as one: it covers the same population | ||
| // under a design that is NOT shipping. | ||
| expect(Object.values(LATE_TOMBSTONE_COUNTERFACTUAL_CORE_DECISIONS_V1) |
There was a problem hiding this comment.
🟡 Issue: Counterfactual seam decision label is not actually pinned
What's wrong
The new validation test says the counterfactual is labelled as a non-shipping design, but the assertion only verifies that its counts add up to the seam population. That gives false confidence: the label proving core rejects for missing retained-transition evidence can drift while the test stays green.
Example
Changing the counterfactual table key to 'accept': 1728 would still satisfy the sum check at this line, even though it would no longer prove the missing retained-transition evidence rejects for retry.
Suggested direction
Compare the full LATE_TOMBSTONE_COUNTERFACTUAL_CORE_DECISIONS_V1 object, not just the sum of its values, so a reason or decision-label drift fails the test.
For Agents
In packages/storage/test/authority-verdict-diff-join-v1.test.ts, keep the population conservation checks, but also assert the full counterfactual decision map, or recompute the counterfactual decisions from the live seam rows and compare the exact keys and counts. Preserve the 1728 total and prove the exact reject reason remains the only counterfactual decision.
There was a problem hiding this comment.
Correct, and it is the exact species this artifact exists to refuse — in a test I added to close a different instance of it.
The row asserted the sum and nothing else, so your example lands: renaming the key to 'accept': 1728 satisfied the assertion while destroying the statement. The whole content of that row is which decision core returns for missing retained-transition evidence; the total is incidental to it.
The exact map is compared now, key and count. Note the irony for the record: that assertion was itself added two rounds ago to fix pinned tables that had no consumer at all — a check written to close a cannot-fail gap, carrying a smaller one of its own.
Fixed in 5a5ac70.
…parison (RECON-P3a) ADR 0002 :129-133 requires a tombstone learned below the current applied authority sequence to be decided by verifying its exact active predecessor and the exact retained transition out of that sequence, with the tombstone taking precedence unless that transition names it, and missing retained-transition evidence rejecting FOR RETRY rather than treating the tombstone as stale. Storage answered that whole rule with two sequence numbers and returned a flat `stale`, reading neither the predecessor nor any transition. The construction that shows it ships as a test: on a built ADR state -- present row at sequence 2, tombstone candidate at sequence 1, its exact active predecessor present, the retained transition present in the applied lineage -- storage returned `stale` while core returned `accept`, and storage returned the same `stale` with the applied lineage's transition digest perturbed. The seam now routes through core. `evaluateAgentProfileLateTombstoneAdvanceV1` validates its inputs and delegates to core's existing lower-sequence arm; no authority clause is reimplemented in storage. It exists rather than reusing `evaluateAgentProfileHeadAdvanceV1` because that entry needs the accepted head as an OBJECT and a receiver holds a head DIGEST -- synthesising one would invent five committed fields and hand core a fabricated operand. Storage supplies the operands it really holds. The retained transition is not among them: `AgentProfileAuthorityTransitionV1` occurs zero times in this package, the applied row persists transition digests only, and a late tombstone's own verification closure does not cover the rotation out of its sequence. So core's missing-evidence clause governs today and the outcome is a retry. The accepting and stale arms become reachable unchanged once a producer supplies that transition. Phase 2's disposition reader is consumed as an explicit precondition. A tombstoned or shadow-dirty row has no authority disposition in V1, so it defers under its own reason instead of having one invented for it. The verdict-diff artifact is re-pinned. 1,728 comparable cells changed outcome -- 1,152 AGREEMENT, 192 DIVERGENCE, 384 NO-MAPPING -- and NOT ONE BUCKET TOTAL MOVED, so the coverage gate has to be read per row. The movement was derived from the axis arithmetic and pinned before the routing landed; the run matched it exactly. The 192 reverse-direction divergence does not close. It measures the evidence channel, not the classifier: core is handed a retained transition the storage path cannot hold. Its recorded mechanism sentence was also inverted -- core returns `stale` when the transition BINDS the tombstone and `accept` otherwise -- and no fixture cell drives the stale side, so both halves are constructed directly. Refs #2052
…rface The system-record subpath's exact-symbol pin went red in CI on the new export, which is the pin doing its job: the published surface is enumerated, so a symbol added to the barrel has to be admitted deliberately rather than arriving with a feature. WHY LOCAL FULL SUITES DID NOT CONTAIN IT. This gate is not vitest. Core's `test` script is three steps -- baseline, a serial system-record config, and this export check, which runs a plain node script plus a standalone `tsc --noEmit` over a types fixture. Running `vitest run` for the whole package covers the first two and structurally cannot reach the third, so "the full package suite is green" was true and still missed a real gate. Verified here by running the CI script itself: 275 exact symbols, up from 274.
…on it broke The verdict-diff suite walks every file under packages/storage/src and asserts the lowercase noun `disposition` appears ZERO times. Routing the late-tombstone seam through core put six occurrences there -- a reason literal, a local binding and two prose comments -- and the first draft answered that by REPLACING the walk with three targeted claims about the new consumer. That was the wrong trade. The raw substring zero is a TRIPWIRE: it catches what nobody thought to name, including prose and comments and literals that do not exist yet. Targeted assertions are GUARDS: they catch exactly what they name. Guards compose with a tripwire; they do not replace one. The zero is also cited as a premise in shipped artifacts, so replacing it silently falsifies those citations. So the walk is restored verbatim, the reason member is renamed to `undecided-authority-classification`, the local binding follows it, the two comments are reworded, and storage src is back to zero occurrences. The consumer is reached through the CamelCase symbol only, which the walk permits by construction. The three targeted claims stay AS ADDITIONS, and one of them was strengthened: it counted CALL sites, and `const alias = fn;` is a reference rather than a call, so an alias passed a check whose whole job is bounding this consumer. It now pins total references. Proven in both polarities. Planting one lowercase word in a prose comment in the file this slice touches turns the walk red, which is how the tripwire is known to cover these paths rather than merely to exist. The additions stay green there and go red on a copied domain literal and on an aliased reference. Two further changes ride here because they were owed before commit: The verdict mapping is now exhaustive over core's decision union with every branch written out and a `never` assignment closing it, replacing a `default` that would have absorbed an unseen decision -- including a quarantine, which is durable state in core and must not be laundered into a deferral. Dropping an arm fails the BUILD rather than a test, so the guard lives in a real tsc program. The core export carries an explicit operand contract: which transition is required, that the predecessor is required in substance though typed optional, that an absent transition yields a reject and never a stale, and that the clock is read only after a transition has matched.
…accept REVIEW ROUND 1, AND THE RED WAS REAL. The late-tombstone arm reads a NON-ACCEPT from the transition verifier as "the tombstone takes precedence" (:312-315), and that verifier also refuses on an unusable clock. So the entry, which took a bare `nowMs` and ran none of the full evaluator's global gates, turned a clock failure into an admission. Measured before the fix, on one built state with a transition that BINDS the tombstone -- the case whose correct answer is `stale`: valid clock -> stale (correct) NaN clock -> accept (INVERTED) -1 clock -> accept (INVERTED) while the full evaluator rejected the same inputs at its front door. The docblock claimed the residue of an unusable clock was "a refusal, never an admission". It was the opposite, and nothing tested it -- an untested claim in a comment, in the one place it was load-bearing. THE FIX IS THE BOUNDARY, NOT A GUARD BOLTED ON. The retained transition and the clock that verifies it are now ONE optional field on a purpose-built evidence type, because neither is meaningful alone on this path: the transition is only ever checked by a clocked verifier, and a clock with no transition is a value nothing reads. That makes "binding transition plus unusable clock" unrepresentable rather than merely refused, and it deletes the fake clock from storage entirely -- the caller now passes only what it holds. The full evaluator's `isSafeNow` and `isIssuedTooFarInFuture` gates are mirrored in the entry rather than delegated, because below that point a refusal from the verifier MEANS precedence. The invariant this buys is asserted rather than described: accept and stale are reachable only when a retained transition AND a valid clock arrived together, so a caller holding neither -- which is every caller in this repository -- cannot express an admission at all. Storage's reject-for-retry is now a property of the type boundary instead of which branch happens to run first. Three regression rows ride with it: the three clock shapes that fail isSafeNow for different reasons (not-a-number, negative, non-integer), the too-far-future candidate head, and the no-transition invariant. The review's other findings, all valid: The seam accounting moved to its own helper. Reviewing that file surfaced a defect the comment was not about: those three pinned tables had NO CONSUMER -- numbers that read as evidence and could not fail. They are now asserted against the live join, and the assertion was mutation-checked by moving one count. The axis-J grounding pin was harvesting the wrong function. Its regex took the first `const optionals` in the file, and this PR added a second snapshot helper above the one it meant, so the pin silently compared one function's members against another's interface. It is now anchored to the function it names. The ambiguity register grows 6 -> 7. Mirroring the clock gates gives `head issuedAt exceeds the future clock-skew bound` a second producing site, so a caller can no longer tell those branches apart. That is the price of the guard, recorded rather than absorbed. The fixture builder validates through the codec before returning, so a drifted head fails at construction instead of behind a cast. Citations were RE-RESOLVED BY CONTENT this time rather than shifted by arithmetic. Three insertions in one PR is where an off-by-one gets in, and an off-by-one citation stays green in any pin that only asks whether the file contains the string. Seventeen moved; seven ambiguous anchors were left alone rather than guessed at.
…nning CI caught a line-anchored citation this PR moved and I did not sweep: authority-verdict-diff-evidence-binding-v1.test.ts pins `isTombstoneBoundToPredecessorV1(candidateState, predecessor)` at an exact line, and the clock-fix insertions pushed it 49 lines down. WHY LOCAL VERIFICATION MISSED IT, WHICH IS THE PART WORTH RECORDING. After the last two commits I re-ran the lanes I judged affected -- twelve files -- instead of the package. That is the curated-lane failure a third time in one PR, and the third variant of it: first a curated file list, then a vitest run that was not the package's test SCRIPT, now a curated list chosen by judgement about blast radius. Citation pins do not respect blast radius; any file that cites a line in a file this PR touched is affected, and the only reliable way to enumerate those is to run the package. Four citations re-resolved BY CONTENT rather than by arithmetic, each verified against the line it now names: the binding predicate at the evaluating site :254 -> :303 (asserted) the three absent-current branches :111/:531/:678 -> :160/:688/:835 the transition-equivocation decision :171 -> :221 the evidence-snapshot allowlist :701 -> :811 The three absent-current sites are the reason content resolution matters here: the arithmetic shift for the second and third would have landed on a fork-resolution comparison and a lineage field list, both of which are real lines that no assertion covers, so the citations would have been silently wrong while the suite stayed green. Full storage package re-run rather than a lane: 1,173 passed, and every failure is the documented oxigraph-worker contention family, whose membership varied across three runs today (3, 4 and 6 files) on trees that differ only in these comment edits.
…ete the sentinel REVIEW ROUND 2. The rule now lives in one pure helper taking exactly what it reads -- candidate, predecessor, retained transition WITH its clock, lineage, sequence -- and both entries call it. The full evaluator adapts its wider evidence object into those arguments; the late-tombstone entry passes what its caller already holds. WHAT THAT DELETES, which is why it is worth doing rather than tidy. The previous shape made the narrow public type widen back into the head-advance shape, which forced an internal `Number.NaN` clock and made the safety of that value depend on which branch happened to run first inside another function. `Number.NaN` now appears ZERO times in both the core authority file and storage's next-state file. The clock gates moved into the rule beside the reading they protect: below that point a refusal from the transition verifier MEANS "the tombstone takes precedence", so the gates belong where that meaning is applied, not at one of two entries. THE PUBLISHED TYPE CONTRACT IS NOW PINNED WHERE IT COMPILES. The pairing promise -- transition and clock are one field, so nobody can hand the rule a binding transition with an unusable clock -- was carried by prose and by runtime cases that cast their fixtures, so a widened type would have kept them green. Four conditional-type assertions now live in the export types fixture, which is one of the few test files a `tsc --noEmit` gate actually compiles: no top-level clock, no bare transition, exact key set, and both halves of the pair required. Each was proven by mutation -- widening the interface three different ways, each time watching the gate fail with `error TS2322: Type 'true' is not assignable to type 'never'` -- with a green restore control. The seam test drops 14 of its 21 `as never` casts by using the exported evidence type and a typed lineage helper. STATED PLAINLY BECAUSE IT WOULD OTHERWISE READ AS A GUARANTEE: that is readability, not enforcement. No test directory in this package is in a tsc program, and adding this file to the type-contracts program pulls in pre-existing errors in shared fixture helpers this change does not own. The enforcement for the new API is the export-types pins above, which do compile. Citations re-resolved again, and again by CONTENT rather than arithmetic: the harvest map regenerated from source, and the one `contains`-paired citation the regeneration could not reach corrected against the line it now names.
REVIEW ROUND 3, AND THE RED WAS REAL AGAIN. The exported entry reused a shortcut
that returns `stale` for a lower-sequence ACTIVE head, so an entry whose name
promises a tombstone verdict answered for a head its rule says nothing about.
Measured before the fix:
evaluateAgentProfileLateTombstoneAdvanceV1(activeHeadAtSequence1, {}, lineage)
-> { decision: 'stale' }
on EMPTY evidence -- no predecessor, no transition, nothing verified.
THE SHORTCUT IS NOT WRONG; ITS LOCATION WAS. "Below the sequence, therefore
superseded" is sound inside the full evaluator, which has already established
that the candidate and the accepted head are the same record before it
dispatches. This entry has no accepted head to make that comparison against,
which is exactly why the shortcut cannot travel with it. So it moved UP into the
lower-sequence adapter rather than being deleted, the rule below became
tombstone-only, and the entry now narrows to AgentProfileTombstoneHeadObjectV1
with a runtime guard behind it because a caller can always cast.
PROVEN IN THREE PIECES, and the first two did not fail the way I predicted --
recording that rather than smoothing it. Removing the runtime guard and deleting
the relocated shortcut both turn the BUILD red rather than the lane, because the
narrowing they produce is what lets the rule take a tombstone head at all: the
type system had absorbed the guard. Predicting a failing test row and observing a
compile error is an unpredicted red, so it needed its own answer. The third
mutant supplies it: rewording the guard's reason literal compiles and fails
exactly the new row, which is what proves the RUNTIME check executes for a caller
who casts past the type rather than being decorative beside it.
The committed test carries both halves in one row, because either alone is
satisfiable the wrong way: the entry refuses an active candidate, AND the full
evaluator still calls that same head stale through its own path. Delete the
shortcut instead of relocating it and the second assertion goes red while the
first still passes.
Citations re-resolved by content again; the harvest map regenerated (36 reject
sites now -- the new guard adds one literal) and the one contains-paired citation
the regeneration could not reach corrected against the line it now names.
… the clock preflight
REVIEW ROUND 4. Two findings, one of which was a hole in this PR's own tests.
THE EXACTNESS CONTRACT WAS UNTESTED, AND A MUTANT PROVED IT. Every row in the
seam suite paired the supplied transition with a lineage built FROM that same
transition, so the comparison against lineage[candidateSequence] always matched.
Deleting the digest conjunct outright left the WHOLE LANE GREEN. The ADR's word
is "the EXACT retained transition", and a suite that never supplies an inexact
one cannot tell exactness from presence -- it was testing that a transition was
PRESENT while claiming to test that it was the RIGHT one.
The new row supplies a well-formed transition, its real predecessor and a real
clock against a lineage that retains a DIFFERENT transition, and demands the
retained-transition refusal specifically rather than whichever guard fires first.
Re-run with the same mutant: it now dies on exactly that row.
THE CLOCK PREFLIGHT IS ONE HELPER INSTEAD OF TWO INLINE COPIES. Mirroring the
gates at each entry gave both reason literals two producing sites, and that is
not only duplication: these literals are externally harvested, so a caller
observing { decision: 'reject', reason } could no longer tell which branch fired,
and the observationally-ambiguous register grew from six to seven purely as
bookkeeping. Extracting rejectInvalidHeadClockV1 returns it to SIX and drops the
harvest from 36 sites to 34 -- one producer per literal, and the register
measures ambiguity again rather than my duplication.
The ordering the previous round established is unchanged: the preflight still
runs before the transition verifier, because below that point a refusal MEANS
"the tombstone takes precedence" and a clock failure must stop rather than invert.
Citations re-resolved by content, four moved.
REVIEW ROUND 5, AND IT IS THE SAME DEFECT AS ROUND 1 ONE LAYER DOWN. That round fixed the caller-supplied clock with a preflight. The transition verifier also refuses on the TRANSITION'S OWN issuedAt, which the preflight does not cover, and every non-accept was still being read as "the tombstone takes precedence". Measured before the fix, same binding transition, same clock: issued at the fixture time -> stale issued in 2027 -> accept A tombstone admitted because its superseding evidence was dated too far ahead to verify. The first fix treated the instance; this one treats the class. THE MAPPING IS NOW AN ALLOW-LIST, and the inversion is the argument for it. ADR 0002 :131-132 makes the descendant invalid only when the transition does not NAME the tombstone as its predecessor -- so exactly one verifier refusal carries the "otherwise", and it is now named as a constant and matched. Every other refusal propagates verbatim: an unusable clock, a transition beyond the future-skew bound, an expiry window not yet passed all mean "not verifiable now", never "not this tombstone's descendant". A deny-list of temporal reasons would have needed a third edit the next time the verifier learns a refusal; the allow-list fails closed on one it has never been shown. THE VERDICT TABLE IS UNCHANGED, which is the check that matters for the seam: the fixture's accepting cells reach accept through the naming refusal, so the join's 192 and every level-1 row are identical. The suite proves it rather than the commit message asserting it. Also from the same round: the counterfactual table was pinned by its SUM alone, so renaming its key to 'accept' would have satisfied the assertion while destroying the statement -- that core rejects for missing retained-transition evidence is the entire content of that row. The exact map is compared now. Citations re-resolved by content, four moved.
otReviewAgent
left a comment
There was a problem hiding this comment.
Review Agent completed a partial review.
Code quality reviewer failed: retry_exhausted
…at needed it
The allow-list fix treated a class, so the class was enumerated rather than
assumed. Fourteen sites across packages/*/src turn an authority verification
into a branch: five calls to evaluateAuthorityTransitionV1, two to the conflict
evaluator, and seven negated binding predicates.
THIRTEEN map a refusal onto a refusal -- a throw, a fail(...), or the decision
propagated verbatim. The late-tombstone rule is the only site with an
AFFIRMATIVE outcome available to invert into, and the reason is structural: it
is the only place where the ADR gives a refusal a meaning of its own
("otherwise the tombstone takes precedence"). Everywhere else a refusal means
refusal, so there is nothing to invert into and no member to move.
Sites measured 14; moved 1 (this seam, already fixed); justified-unmoved 13. The
note lives beside the mapping so a future site that reads a refusal as an
affirmative is recognisable as joining this class rather than as a new idea.
Citations re-resolved by content after the docblock insertion; three moved.
5a5ac70 to
560188a
Compare
| ); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
🟡 Issue: Move the proof narrative out of the hot authority module
What's wrong
The implementation change is relatively small, but the production authority module now carries a large amount of review narrative and brittle line-number proof text. That makes the core path harder to scan and pushes the file close to the 1k-line threshold without adding executable structure.
Example
A reader has to scan lines 492-560 before reaching a 28-line function, and citations like :121-126 or :303-305 become stale on the next nearby edit unless a separate citation-maintenance pass is done.
Suggested direction
Reduce the production comments to concise invariants and ownership notes. Put the detailed historical proof, review discoveries, measurements, and citation-heavy rationale in a doc/test artifact that is meant to carry that weight.
For Agents
Keep production comments to the invariant and the rule boundary, then move the long defect history, measurements, and line-citation proof into the ADR or the verdict-diff/test artifact. Re-check file size after trimming; consider extracting the late-tombstone entry if the module still trends past 1k lines.
There was a problem hiding this comment.
Agreed and done, with the trim shaped by what each artifact is for.
The production module now carries the invariant and the rule boundary: what the rule decides, why the affirmative rests on a structural fact, and why every other refusal propagates. The defect history, the measurements (the stale-vs-accept clock table, the future-dated construction) and the 14/1/13 sweep have moved to the late-tombstone seam suite, which already holds the constructions that prove them — so the evidence now lives next to the thing that executes it rather than next to the thing it describes.
Your citation-churn point had a second payoff I should name: the trim removed several of the line-anchored references you flagged, and this round's other two changes deleted more (the extracted predicate replaced a literal that the harvest had to track). Fewer citations in the hot module is less to re-resolve on every future edit — which is the same complaint thread 8 raised from the fixture side, answered from the source side.
I did not extract the entry itself. The module is still large, and the adapter extraction is filed against the active-seam slice that touches this file anyway; doing it here would be the fourth structural move in one PR, and every previous one moved citations that other suites pin — once at a cost CI caught rather than my local run.
Fixed in 4fd92bc.
There was a problem hiding this comment.
🟡 Issue: Split the late-tombstone rule out of the central authority evaluator
What's wrong
This is a cohesive new subdomain, but it is embedded directly into an already-large authority file. That makes the main evaluator less scannable and leaves the next rule addition very likely to push this file past the 1k-line boundary. The code-quality move here is decomposition, not polishing the comments in place.
Example
A reader trying to follow ordinary evaluateAgentProfileHeadAdvanceV1 lower-sequence behavior now has to pass through a self-contained late-tombstone subdomain with its own public evidence model, snapshot helper, rule, and history-heavy commentary before getting back to the rest of the authority evaluator.
Suggested direction
Extract a dedicated late-tombstone authority module and leave this file responsible for orchestration across authority advance branches. Move most of the narrative rationale to the seam test or ADR-adjacent docs; keep production comments to the operand contract and non-obvious invariants.
For Agents
Move the late-tombstone evidence types, snapshot helper, evaluateLateTombstoneRuleV1, and exported late-tombstone entry into a focused internal module, then keep evaluateLowerSequenceAgentProfileHeadAdvanceV1 as the thin adapter. Preserve the existing public export from system-record-objects-v1.ts and prove behavior with the current late-tombstone seam tests.
There was a problem hiding this comment.
Your prediction came true inside this PR, and I want to say that before I say why the move is still not in this commit.
Measured just now: system-record-authority-v1-internal.ts is 1,201 lines. It was 1,095 when you wrote that the next rule addition would very likely push it past the 1k boundary. The next rule addition was this one — a second tombstone entry with its own operand types, its own snapshot helper, its own rule and its own commentary. So the decomposition argument is strictly stronger than when I last answered it, and re-running my "cohesion" reply would be the stale-citation shape I have criticised in review comments.
I am not going to argue the point. I am going to give you the reason it is a separate change, because it is specific rather than a general preference for the status quo:
An independent adversarial review of this seam's decision semantics is pinned at a commit and re-runs its own ADR-derived table against my final head. A module move relocates every line-anchored citation into this file. Seven of them moved in this commit alone, and the register that resolves them had to disambiguate one entry by SITE rather than by text, because the same predicate call now appears at three places and two of them are textually identical. If the extraction and the semantic change land together, that verification cannot distinguish a verdict that moved from a line that moved — and the whole point of commissioning it was that this seam has produced five decision-inverting defects that its author did not see.
So: filed with the lead as its own change, sized as mechanical — the evidence types, the snapshot helper, both rules and both entries into a focused internal module, with the adapters staying here and the public exports unchanged from the barrel. If the lead wants it before merge rather than after, it is one commit and I will do it; the ordering is the only thing I am defending, not the file.
One correction to the suggested direction, since it would change what the code proves rather than where it lives: moving the narrative rationale out to the seam tests is right for the history and wrong for two specific passages. The clock-and-transition pairing and the "a non-accept is not an affirmative" mapping are both inversion prevention — each one is there because removing the sentence is how the defect came back the second time. Those stay next to the branch they govern; the round-by-round history goes with the constructions that prove it.
There was a problem hiding this comment.
🟡 Issue: Split the tombstone authority slice before this file crosses 1k lines
What's wrong
This PR pushes a central authority evaluator from under 1k lines to over 1k lines by inlining a new tombstone subdomain and extensive explanatory prose. That makes the module less cohesive and turns future authority changes into edits in an oversized file rather than a focused rule module.
Example
A reader looking for the existing head-advance flow now has to scan through tombstone-only operand models, snapshot adapters, public seam entries, and long rationale blocks before reaching unrelated authority branches. The file also now crosses the explicit 1k-line threshold.
Suggested direction
Move the tombstone-specific public adapters and pure rules into a dedicated module, and move the long design narrative to ADR/test documentation or much shorter comments. The current implementation adds a cohesive subdomain directly into an already central evaluator file.
For Agents
In packages/core/src, extract the late/same-sequence tombstone operand types, snapshot helpers, and rule functions into a focused tombstone-authority module, then re-export the public surface from the barrel. Keep evaluateAgentProfileHeadAdvanceV1 as the dispatcher and preserve all current decisions; run the package export checks plus the storage tombstone seam suites.
There was a problem hiding this comment.
🟡 Issue: Split the tombstone authority rules before landing a 1.2k-line evaluator
What's wrong
This PR pushes a production file from well under 1k lines to well over 1k lines. The added code is not incidental glue; it defines new public operands, snapshots, rule implementations, and exported entries for a separate tombstone authority subdomain. Keeping all of that in the central evaluator makes the file harder to navigate and makes future authority changes more likely to collide in one monolith.
Example
A reader auditing ordinary evaluateAgentProfileHeadAdvanceV1 now has to scan through two new public tombstone APIs, their boundary snapshots, large ADR histories, and storage-row adapter concepts inside the same authority evaluator file. The tombstone rule is a coherent subdomain, but it is folded into the central file instead of becoming a small module with a narrow import surface.
Suggested direction
Move the tombstone authority subdomain behind its own module boundary instead of letting the main authority evaluator absorb another 500+ lines. This is a clearer structural fix than trimming comments inside the same file.
For Agents
Extract the new tombstone-specific public contracts, snapshot helpers, late/same-sequence rules, and exported entry points into a focused core module such as system-record-tombstone-authority-v1-internal.ts. Keep evaluateAgentProfileHeadAdvanceV1 behavior by importing the shared rule/adapters back into system-record-authority-v1-internal.ts, preserve the package barrel exports, and run the package export/type pins plus the late/same-sequence seam suites.
There was a problem hiding this comment.
🟡 Issue: Split this file before landing another 500-line authority slice
What's wrong
This PR pushes a file from well under 1k lines to over 1k lines and mixes a new tombstone subdomain into the central authority evaluator. The implementation may be behaviorally right, but the structure makes the authority model harder to scan, review, and extend.
Example
A reader now has to track tombstone evidence types near line 92, the late rule around line 495, public wrappers around lines 717 and 786, and the same-sequence rule around line 926, all interleaved with absent/active/next-sequence/fork-resolution authority logic.
Suggested direction
Move the late-tombstone and same-sequence tombstone contracts, snapshot helpers, and rule functions into a dedicated system-record-authority-tombstone-v1-internal.ts or equivalent. This PR is exactly the point where decomposition is cheaper than normalizing a 1.2k-line authority module.
For Agents
Extract the tombstone authority slice into a focused internal module, preserving exported names and current decisions. Keep evaluateAgentProfileHeadAdvanceV1 as a thin dispatcher and update barrel exports/tests to import through the new module. Existing seam tests should continue to pass unchanged.
There was a problem hiding this comment.
🟡 Issue: Move design-history prose out of the production hot path
What's wrong
The new implementation relies on large all-caps explanatory blocks and brittle line citations to make the code understandable. That bloats the files, obscures the actual control flow, and creates recurring churn whenever unrelated line numbers move.
Example
A behavior-preserving insertion in system-record-next-state-v1-internal.ts forced comment-only line-number updates across core and test artifacts. That is maintenance work created by the comment structure, not by the behavior.
Suggested direction
Keep source comments at the invariant level and link to symbol names or ADR sections. The longer historical measurements belong in the seam tests or a separate design note, ideally with generated citations instead of hard-coded line numbers.
Confidence note
The domain appears unusually documentation-heavy already, so the concern is not that comments exist; it is that this PR moves historical proof text and brittle line citations into production code at a scale that is now affecting maintainability.
For Agents
Move detailed ADR reasoning, measured-before text, and line-number proofs into ADR/test documentation where they can be asserted or regenerated. Keep production comments short: describe the invariant and name the owning helper/test, not the full review history.
… the rule can decide REVIEW ROUND 6. Two changes that together take prose and unreachable states out of the seam's contract. THE ALLOW-LIST NO LONGER MATCHES AN ENGLISH STRING. It keyed on `transition does not bind the accepted predecessor`, a literal owned by another module, so a reword or a split there would have silently changed this seam's meaning -- and the harvest scaffolding existed partly to police that. The binding conjunction is now a named predicate, isAuthorityTransitionBoundToPriorHeadV1, extracted from the verifier's own check so there is ONE definition, and the seam asks the structural question directly. The failure direction was always the safe one (a reword would have refused a tombstone that should take precedence, never admitted one), but prose was load-bearing for control flow and no longer is. THE RULE'S RESULT IS NARROWED TO WHAT IT CAN PRODUCE. It returned the full authority union, so storage had to carry a `quarantine` arm for a state this rule cannot reach -- and that arm mapped a DURABLE authority state onto "incomplete evidence" while compiling cleanly. The new AgentProfileLateTombstoneDecisionV1 is accept | stale | reject; the storage arm is deleted because it no longer typechecks, and a future widening becomes a compile error at the seam instead of a silent deferral. The narrowing did not typecheck at first, and following the errors was the point: it forced evaluateAuthorityTransitionV1's declared type to match its measured behaviour (one accept, six rejects -- never stale, never quarantine), and the clock preflight's to match its own. Both were wider than anything they produce, which is what made an unreachable arm look obligatory downstream. Also trimmed: the proof narrative in the production module is now the invariant and the boundary, with the defect history, the measurements and the sweep moved to the seam suite that already carries the constructions proving them. The published surface grows by one runtime symbol (276) and one type; both admitted deliberately through the export gate. Citations re-resolved by content, including regenerating the delegated harvest map from source rather than shifting it.
| * `evidence.retainedTransition.transition` MUST be the retained transition OUT | ||
| * of the candidate's sequence -- `lineage[candidateSequence]`, the rotation into | ||
| * the NEXT sequence -- not the rotation into the candidate's own. Supplying the | ||
| * wrong one is refused rather than misread, because :303-305 compares prior |
There was a problem hiding this comment.
🟡 Issue: Line-number citations are already stale
What's wrong
The PR adds a large amount of line-addressed explanation, but some of those addresses are stale in the submitted tree. That is a maintainability regression: the comments look authoritative while sending readers to the wrong code, and every future edit has to update prose across multiple files.
Example
A maintainer following because :303-305 compares prior sequence, next sequence AND digest lands in unrelated code, not the comparison this sentence is describing.
Suggested direction
Stop encoding source line numbers in production prose for this path. Use function names, local variable names, or executable assertions instead; line pins belong in generated tooling, not hand-maintained comments.
For Agents
Replace absolute line citations in the new late-tombstone docblocks with symbol/branch names such as evaluateLateTombstoneRuleV1 and rejectInvalidHeadClockV1, or use stable marker comments if a generated citation system needs anchors. Preserve the invariants, remove the line-number maintenance burden.
There was a problem hiding this comment.
Taken, and you were right that some were already stale in the submitted tree.
Six source line citations in this module's prose are replaced by symbol names — evaluateAgentProfileHeadAdvanceV1, rejectInvalidHeadClockV1, "the rule compares prior sequence, next sequence and digest" — so the explanation survives the next insertion instead of quietly pointing at unrelated code.
ADR citations stay, and the distinction is the useful one: that document is versioned, its line numbers are what the spec itself cites, and it does not move when this file does. What must not be hand-maintained is prose addressing code by position.
This also compounds with the trim from the previous round: the module now carries the invariant and the boundary rather than the proof narrative, and it addresses by name rather than by line — so the citation-maintenance burden you and the fixture-side thread both flagged is materially smaller here.
Fixed in 40755f6.
| * transition may be ACTED on additionally depends on time, and that stays in | ||
| * {@link evaluateAuthorityTransitionV1}. | ||
| */ | ||
| export function isAuthorityTransitionBoundToPriorHeadV1( |
There was a problem hiding this comment.
🟡 Issue: New public transition-binding helper lacks direct behavioral coverage
What's wrong
The PR publishes a helper whose purpose is to distinguish a structurally bound transition from an unbound one, and storage’s late-tombstone decision now depends on that distinction. The tests verify that the name is exported and exercise one digest-mismatch scenario indirectly, but they do not pin the helper’s full public contract across its individual binding fields.
Example
A regression that removes the priorEvmIssuer or peerPublicKey comparison from isAuthorityTransitionBoundToPriorHeadV1 would still satisfy the new export-list check and the bound/unbound late-tombstone tests if those fixtures only differ by priorHeadDigest.
Suggested direction
Add focused runtime tests for the exported helper’s public contract rather than relying on the package export list or on late-tombstone tests that exercise only one mismatch shape.
Confidence note
Static review only; I found indirect coverage through late-tombstone tests, but no direct behavioral test for the newly exported helper itself.
For Agents
Add core package tests for isAuthorityTransitionBoundToPriorHeadV1 near the existing authority transition tests. Prove it returns true for the exact retained transition/prior head pair and false when each structural operand is changed independently, especially priorEvmIssuer, peerPublicKey, and the tuple fields.
There was a problem hiding this comment.
Right about the gap, and the fix went further than adding tests.
Your example was precise: fixtures differing only by priorHeadDigest cannot prove priorEvmIssuer or peerPublicKey are compared. That is the same shape as the digest-conjunct gap you found two rounds ago, and it was live again.
What changed is the thing being tested. The round's 🔴 showed the single boolean was too coarse for this seam — a foreign-issuer transition returned accept. So the helper is now a three-way classifier, it is no longer public API (per the sibling thread), and the coverage lands where the distinction is observable: the seam suite drives the identity fields separately and pins the head-digest case against them, so a regression dropping an identity comparison flips a row from the unrelated-evidence refusal to accept.
That is stronger than a direct unit test of the old predicate would have been, because it pins the classification at the decision that depends on it rather than at a boolean a future refactor could route around.
Fixed in 40755f6.
There was a problem hiding this comment.
🟡 Issue: The new binding classifier is only tested for one unrelated identity field
What's wrong
The late-tombstone rule now depends on distinguishing unrelated retained transitions from same-authority transitions that name another head. The tests cover the important split, but only one of the five identity fields that drive the unrelated branch is exercised, so a regression in the untested comparisons could still pass green while allowing unrelated retained evidence to decide a tombstone.
Example
A retained transition with priorHeadDigest set to the tombstone digest but networkId or peerId changed should reject with late tombstone retained transition belongs to another authority. If the corresponding check were accidentally removed, the current tests would not isolate that regression.
Suggested direction
Add behavior tests that prove every identity-field mismatch is treated as unrelated, not just priorEvmIssuer.
For Agents
In packages/storage/test/system-record-late-tombstone-seam-v1.test.ts, parameterize the existing unrelated-evidence test over each authority identity field checked by classifyAuthorityTransitionBindingV1: networkId, peerId, peerPublicKey, priorAuthoritySequence, and priorEvmIssuer. Keep the existing priorHeadDigest perturbation as the same-authority/different-head control that must still return accept.
There was a problem hiding this comment.
Taken — and measuring it per conjunct showed that something I had already posted on this PR was false.
I replied on another thread that "the seam suite drives the identity fields separately". It drove one of five, in a test named per-identity-field. That is the same shape you are naming here, and you were right to distrust the coverage rather than the claim.
Measured with core rebuilt between each mutation and each run, because storage executes core from dist and a src-only mutation would have reported every one of these as a survivor:
| conjunct | solo removal | what that means |
|---|---|---|
networkId |
dies solo | genuinely load-bearing |
priorEvmIssuer |
dies solo | genuinely load-bearing |
peerId |
survives solo, dies with peerPublicKey |
structural, see below |
peerPublicKey |
survives solo, dies with peerId |
structural, see below |
priorAuthoritySequence |
redundant at this seam | see below |
The two survivors are structural, not gaps. The head codec derives the peer id from the public key, so no fixture can move one without the other — a row that tried would be refused at construction with a binding error rather than reaching the predicate. The pair is what the test drives, and the pairing is written down at the site so the next reader does not file it as missing coverage.
priorAuthoritySequence is forced equal by the lineage validator before this predicate runs, so removing that conjunct changes nothing reachable through this entry. That is recorded in the test as a measured redundancy rather than papered over with a row that could not fail — a per-conjunct assertion that cannot discriminate is worse than an honest absence, because it reads as coverage.
The priorHeadDigest perturbation stays exactly where it was, as the same-authority/different-head control that must still return accept.
…recedence REVIEW ROUND 7. The seam asked ONE structural question -- "does this transition name this head?" -- and read every NO as the ADR's "otherwise the tombstone takes precedence". That is broader than the ADR's clause, and the gap is real: same record, transition names another head -> accept (correct) transition carrying a FOREIGN issuer -> accept (wrong) ADR 0002 :131-132's "otherwise" presupposes evidence from THIS record's rotation out of THIS sequence. A transition naming a different head at that sequence really does prove no valid descendant of the tombstone exists. A transition from another authority proves nothing about this record at all, and reading it as precedence lets an unrelated object decide a tombstone's fate. classifyAuthorityTransitionBindingV1 now answers three ways instead of two -- names-this-head, names-another-head, unrelated -- with the record-and-rotation identity (network, peer, public key, prior sequence, prior issuer) separated from the head-level digest question. The seam maps them: another-head is the ADR's otherwise; unrelated is a refusal with its own reason; names-this-head propagates the verifier's decision, which can only be temporal there. The verifier itself uses the same classifier, so there is one definition rather than two. The regression rows drive each identity field separately, because a fixture that differs by one field cannot prove the others are compared -- the lesson this suite already learned on the digest conjunct -- and the last row perturbs only the head digest and demands `accept`, so the two halves are pinned against each other rather than in isolation. ALSO FROM THIS ROUND: the binding predicate no longer ships as public API. It was exported for one internal seam, which turns an implementation detail into a package contract; the classifier lives in core's verification module and the published surface returns to 275 symbols. And the hand-maintained SOURCE line citations in this module's prose are replaced by symbol names -- six of them, some already stale in the submitted tree, which is the failure mode of prose that addresses code by position. ADR citations stay: that document is versioned and its line numbers are what the spec itself cites.
| priorHead: AgentProfileHeadObjectV1, | ||
| nowMs: number, | ||
| ): SystemRecordAuthorityDecisionV1 { | ||
| ): Extract<SystemRecordAuthorityDecisionV1, { readonly decision: 'accept' | 'reject' }> { |
There was a problem hiding this comment.
🔴 Bug: Public transition evaluator return type was narrowed
What's wrong
This exported API used to promise the full authority-decision union. Narrowing the declared return type is a source-level breaking change for TypeScript consumers that defensively switch on or otherwise type against the old union, even though the runtime values may not have changed.
Example
A downstream caller that currently handles the documented full authority union can stop compiling after this change:
const decision = evaluateAuthorityTransitionV1(t, h, now);
switch (decision.decision) {
case 'quarantine':
// previously valid defensive handling, now TS says this case is impossible
break;
}Suggested direction
Keep the exported return type as SystemRecordAuthorityDecisionV1, or introduce a new narrower helper/type while leaving the existing public function contract intact until a planned breaking release.
For Agents
Look at packages/core/src/system-record-authority-verification-v1-internal.ts and the public barrel in system-record-objects-v1.ts. Preserve runtime behavior if the evaluator truly only returns accept/reject, but avoid narrowing the exported function signature in a patch PR unless this is an intentional breaking API change. Add or update a public API/type compatibility assertion showing existing callers can still treat the result as SystemRecordAuthorityDecisionV1.
There was a problem hiding this comment.
Agreed, and already reverted — this one was mine, and it is the exact mirror of a rule I had written down two rounds earlier.
evaluateAuthorityTransitionV1 declares SystemRecordAuthorityDecisionV1 again. The precise type moved to evaluateAuthorityTransitionInternalV1, which is not on the barrel and is what the late-tombstone rule consumes, so the seam keeps its narrowing without the published signature promising less than it did.
Your framing is the half that was missing from my reasoning. Round 6 narrowed a declared codomain and that was right: an over-wide declaration on an internal boundary forces consumers to write unreachable arms, and those arms then absorb future states silently — the storage quarantine arm that folded a durable authority state into "incomplete evidence" while compiling cleanly. I generalised that to a published surface, where the declaration is not a description of behaviour but a promise about what callers may write, and your defensive case 'quarantine': is exactly the code it breaks with no runtime value moving.
"The declaration is imprecise" and "the declaration is safe to tighten" are different claims. I measured the first and assumed the second.
The compiled export lane now pins it, since a type-level guard in a .test.ts file is decorative in this repo — no package test directory is in any tsc program:
SystemRecordAuthorityDecisionV1 extends ReturnType<typeof evaluateAuthorityTransitionV1>
Proven by mutation rather than by inspection: re-narrowing the public return produces TS2322 at that assignment, and the restore is green. The rule as amended now reads "narrow internal boundaries, never published ones", with both directions pinned in the same file.
| // only site in the package with an affirmative outcome available to invert | ||
| // into, are recorded in the late-tombstone seam suite beside the constructions | ||
| // that prove them. | ||
| switch (classifyAuthorityTransitionBindingV1(transition, candidateState)) { |
There was a problem hiding this comment.
🟡 Issue: Avoid recomputing transition binding after a lossy verifier result
What's wrong
The rule currently calls the verifier, loses the reason it needs to distinguish naming failure from temporal failure, then recomputes binding to reconstruct that distinction. That duplicates validation/digest work and couples this rule to the internal ordering of evaluateAuthorityTransitionV1, which is harder to maintain than an explicit typed boundary.
Example
A future-dated transition that names this tombstone propagates the transition-time reject only because evaluateAuthorityTransitionV1 checks time before binding and the later binding switch falls through to verified. The late-tombstone rule has to know both that ordering and the binding classifier to interpret one transition.
Suggested direction
Refactor the transition verifier boundary so this rule can get a structured result such as { binding, verification }, or split the binding and temporal/expiry checks into clearly ordered helpers. The goal is one classification pass and no dependence on reverse-engineering a generic reject.
For Agents
Look at evaluateLateTombstoneRuleV1 and evaluateAuthorityTransitionV1. Preserve the current accept/stale/reject outcomes, but introduce a structured lower-level transition check or result that carries binding classification and temporal verification together, so the late-tombstone rule computes the relationship once and switches on explicit data. Existing seam tests should cover the binding, unrelated, and temporal-refusal cases.
There was a problem hiding this comment.
This is the sharpest comment on the PR, it converges with an independent finding, and I am treating it as adopted in principle — with the one measurement it was conditional on now done.
You are right about both halves. The rule calls the verifier, discards the reason it needs, and then recomputes the binding relationship to reconstruct the distinction it just threw away. That duplicates the digest work, and worse, it makes the rule's correctness depend on evaluateAuthorityTransitionV1 checking time before binding — an ordering nothing pins and nothing tells you that you broke.
An independent adversarial review of this seam, constructed from the ADR text rather than from my tests, reached the same property from the opposite direction and stated the consequence more precisely than I would have: the seam discards the verifier's refusal reason on two of its three arms, and that is harmless today only by the coincidence that exactly one non-binding refusal is currently reachable and ignoring it does not move a verdict. It stops being harmless the moment the verifier learns a refusal that should mean refuse. A signature check is the obvious candidate — envelope fields sit outside the unsigned codecs today — and on that day a forged transition naming another head would be admitted, the record deleted, and nothing would go red. Two independent readers arriving at one seam property from different directions is the strongest signal this PR has produced about it.
The shape you both describe is the same: establish verifiability first, then consult the binding classifier, so the structural arms are sound by construction rather than by the verifier's internal branch order.
The condition attached to landing it was a measurement I owed — does any cell reachable through the live storage path move? Measured, and the answer is zero. The single storage call site builds its evidence as a frozen { tombstonePredecessor } literal; retainedTransition is never written, not merely left undefined. And AgentProfileAuthorityTransitionV1 occurs in no executable code in that package — two occurrences, both comments, with the identifier itself found by the same scan as the positive control. So core refuses at the retained-transition clause before the verifier is consulted at all, on every cell storage can reach. Exactly one cell moves, and it is a constructed one at the entry: a transition naming another head, dated beyond the future skew, which today returns accept and becomes a refusal.
Two things I want to be accurate about rather than agreeable:
The one-line hoist is not the fix. An earlier read of this called it a one-line reordering of the transition's issuedAt gate. That would fix today's instance and leave the class — the same mistake this seam has now made four times, each one arm over from the last. Your structured-result direction is the one that closes it, and that is why it is worth a change of its own rather than a patch inside this one.
I am not landing it in this commit. This commit reverses a carve-out and moves two cells in the data-destroying direction; the independent verification re-runs its table against my head with exactly one cell declared as permitted to move. Landing a second semantic change in the same push makes that re-run unable to attribute anything. It goes in the next round, with the r11 cell's movement as its own pinned before/after — and if the verifier's re-run shows that one cell moving and nothing else, that doubles as the zero-live-movement check rather than resting on my measurement above.
There was a problem hiding this comment.
🟡 Issue: Dispatch on transition binding before running a verifier whose result is mostly discarded
What's wrong
The late-tombstone rule currently evaluates the full transition verifier for every retained transition, then recomputes the binding and throws away the verifier result on two of the three binding arms. That ordering makes the semantics non-local: readers must understand which verifier refusals are real and which are intentionally ignored. The same behavior can be expressed with fewer moving parts and one binding classification.
Example
Current shape: compute verified; if it accepted return stale; otherwise classify binding; then names-another-head returns accept and unrelated returns its own reject, discarding the verifier reject that was just computed.
Suggested direction
Make classifyAuthorityTransitionBindingV1 the dispatcher for this rule and invoke the full transition verifier only in the one arm where its decision is actually consumed.
For Agents
Refactor evaluateLateTombstoneRuleV1 around lines 533-589: after retained-transition exactness and clock preflight, classify binding once; return reject for unrelated, accept for names-another-head, and only call evaluateAuthorityTransitionInternalV1 in the names-this-head arm, mapping accept to stale and reject through unchanged. Preserve all existing reason strings and seam behavior.
There was a problem hiding this comment.
Half of this is adopted and the other half I have to decline, because as written it would make permanent the exact hazard an independent review of this seam has already flagged. Let me show the measurement rather than assert it.
The half you are right about, unreservedly: the binding relationship is computed twice, the second computation exists only to reconstruct information the first call discarded, and the rule's correctness silently depends on evaluateAuthorityTransitionV1 checking time before binding. One classification, not two. That is a real defect in what I wrote and it should be fixed.
The half I cannot take is the dispatch ORDER, and the reason is a property rather than a preference. Your suggested shape calls the verifier only in the names-this-head arm. That preserves today's verdicts exactly — you are right about that, and I checked it — but it makes a structural commitment: no verifier refusal can ever reach the other two arms again.
Today that is harmless, and harmless by coincidence rather than by design. Exactly one non-binding verifier refusal is reachable, and ignoring it does not move a verdict. It stops being harmless the moment the verifier learns a refusal that should mean refuse. A signature check is the obvious candidate — envelope fields sit outside the unsigned codecs today. On that day, a forged transition naming another head would be classified names-another-head, never presented to the verifier at all, and mapped to accept. The record is deleted and nothing goes red.
An independent adversarial review of this seam, constructed from the ADR text rather than from my tests, found precisely that: the seam discards the verifier's refusal reason on two of three arms, which is harmless today and is a latent admission tomorrow. Your comment and that review agree on the diagnosis — the rule should classify once — and differ on the remedy, because you are optimising the current verdict table and it was reasoning about the verdict table the code will have.
So the shape I am landing takes your simplification and fixes the ordering rather than freezing it:
- retained-transition exactness (unchanged)
- clock preflight (unchanged)
- verifiability — establish that this transition is admissible evidence AT ALL, once, before anything is classified
- one binding classification:
unrelated→ its own reject,names-another-head→accept,names-this-head→stale - no second verifier call, no recomputed binding
That gives you everything you asked for — one classification pass, no discarded verifier result, no dependence on the verifier's internal branch order — while making the structural arms sound by construction instead of by the coincidence that today's only refusal happens not to matter. A future "not verifiable" refusal then refuses on all three arms, which is the property that has to hold.
It is a behaviour change rather than a refactor, and that is the honest label: exactly one constructed cell moves — a transition naming another head, dated beyond the future skew, which currently returns accept and becomes a refusal. I have measured the live blast radius and it is zero: the single storage call site builds its evidence as a frozen { tombstonePredecessor } literal with retainedTransition never written, and AgentProfileAuthorityTransitionV1 occurs in no executable code in that package, so core refuses at the retained-transition clause before the verifier is consulted on any reachable cell.
Not in this commit, and the reason is procedural rather than doubt. This push reverses a carve-out and moves two cells in the data-destroying direction, and the independent verification re-runs its table against my head with exactly one cell declared as permitted to move. A second semantic change in the same push makes that attribution impossible. It lands next, with that cell's before and after pinned as its own row.
There was a problem hiding this comment.
Landed at d47588678, and the movement is measured rather than predicted.
Your simplification is in and the ordering is fixed rather than frozen. The rule now reads: retained-transition exactness, clock preflight, verifiability, then one binding classification. The names-this-head arm asks only the expired-prior question, because re-running the whole verifier there would recompute exactly the classification just performed in order to reconstruct a reason it had already discarded — your point, and it is gone.
The split is deliberately a class rather than a hoist. rejectUnverifiableAuthorityTransitionV1 holds the refusals that are about the transition — can this be checked at all, right now — which are by construction independent of what it binds. rejectInadmissibleExpiredPriorTransitionV1 holds the ones that are questions about this binding, since every one compares the transition against the prior head it names. evaluateAuthorityTransitionInternalV1 is composed from the same two parts in the same order, so the verifier's own decisions are unchanged and the three callers of the published form are untouched.
Two cells moved, both at the entry layer, both toward refusal:
| cell | before | after |
|---|---|---|
names-another-head + future-dated |
accept |
the temporal refusal |
unrelated + future-dated |
belongs to another authority | the temporal refusal |
The first is the one that mattered — an object nothing could verify was being mapped to an admission that deletes the record. The second is reason-only and is the more honest answer: with an unverifiable transition, asserting whose authority it belongs to is a claim derived from unchecked bytes.
No cell any receiver can reach moved, which was the condition on doing this rather than a hope. The single storage call site builds its evidence as a frozen { tombstonePredecessor } literal — retainedTransition is never written, not merely left undefined — and AgentProfileAuthorityTransitionV1 occurs in no executable code in that package, two occurrences and both comments, with the identifier found by the same scan as the positive control.
And the new rows were proven to discriminate rather than trusted for passing. A row that goes green on its first run has shown only the after-state. Removing the verifiability gate — applied in src, rebuilt into the dist that storage actually executes, restored byte-identical — turns the row red with exactly the predicted observation: namesAnotherHead reverts to accept, unrelated reverts to the belongs-to-another-authority literal. The earlier temporal row goes red under the same mutant, which is the gate being load-bearing on all three arms rather than only on the two that visibly moved.
One thing I got wrong in this session and would rather record than quietly fix: an intermediate core run reported three failures and I nearly attributed them. It was void — I had left a full storage suite running in the background, so two suites were competing for the machine. The serial re-run is 107 files and 1,700 tests green. Attribution by A/B, not by re-running until it agreed with me.
…close the binding switch REVIEW ROUND 8, plus two defects found by self-review before the round arrived. 1. THE PUBLISHED RETURN TYPE WAS NARROWED, AND THAT IS A BREAKING CHANGE. `evaluateAuthorityTransitionV1` is exported from the package barrel and pinned in the export list. A previous commit narrowed its declared return type from the full authority union to accept-or-reject, on the grounds that this is its real codomain. It is -- but the declaration is a contract with consumers outside this repository, and a defensive `case 'quarantine':` in their code stops compiling against the narrower union even though no runtime value moved. The published signature returns the full union again. The precise type lives on `evaluateAuthorityTransitionInternalV1`, which is not exported from the barrel and is what the late-tombstone rule consumes, so the seam stays narrow without the public surface moving. If the narrowing is wanted publicly it should be its own change in a release that says so. A type-level pin now holds the public direction: re-narrowing the exported return fails `test:system-record-export` with TS2322 at the pin, with a green restore control. The wider direction is the one nothing else catches, because every in-repo caller keeps compiling happily against a narrowed return. 2. THE IDENTITY COVERAGE CLAIM WAS FALSE, AND THE TEST NAME MADE IT LOOK TRUE. A test called "refuses retained evidence from another authority, per identity field" drove exactly ONE of the five identity fields. The name asserted coverage the body did not have. Measured, per conjunct, with core REBUILT between mutation and run: networkId solo removal -> RED priorEvmIssuer solo removal -> RED peerId solo removal -> GREEN peerPublicKey solo removal -> GREEN peerId + peerPublicKey removed together -> RED priorAuthoritySequence solo removal -> GREEN The two peer survivors are structural, not a gap: the codec asserts that the public key DERIVES the peer id, so no input changes one without the other, and the joint mutant is what shows the pair is covered rather than merely unreachable. The prior-sequence survivor is also structural -- the lineage validator forces entry `i` to carry `priorAuthoritySequence === String(i)` and the rule reads `lineage[candidateSequence]`, so that conjunct is equal by construction before the classifier sees it. It gets its own test asserting the refusal that actually fires, rather than a fabricated row that would trip the lineage check and read as coverage. The foreign identity is a derived constant: the binding check needs only that the public key derives the peer id, so a fixed 32-byte key gives a stable one. 3. RUNTIME NEGATIVE TESTS FOR THE EVIDENCE SHAPE. The type pins protect TypeScript callers and do nothing for a JavaScript one, or for a TypeScript one arriving through `as never`. Three cases now assert the refusal at the boundary: a bare transition with a top-level clock beside it (the pre-refactor shape that once turned a `stale` into an `accept`), a retained transition with no clock, and a clock with no transition. Proven failable by widening the accepted key list, which turns them red. 4. FOUND BY SELF-REVIEW: THREE SENTENCES SHIPPED BROKEN. Replacing hand-maintained line citations with symbol names was right, but three of the six replacements were shorter than the text they matched and dropped the tail of the sentence. The sweep's instrument reported six substitutions applied and zero citations remaining -- both true, and neither can express "the sentence is still a sentence". All three are restored. An automated detector for the class was built and DISCARDED rather than shipped: it caught 1 of the 3 instances it was derived from. 5. FOUND BY SELF-REVIEW: THE BINDING SWITCH HAD A DEFAULT ARM. `default: return verified` absorbed anything that was not the two named bindings. The fall-through happened to be safe, but a fourth binding would have been routed silently at run time. All three are enumerated now, with the repo's established `const unmapped: never` idiom and a throw; adding a fourth member produces TS2322 at that assignment. Refusing with a new reason literal was the other option and was rejected -- it would widen the observable reject codomain by a case no test can construct. TWO EARLIER ATTEMPTS AT THAT PROOF WERE BLIND and are worth recording, because both reported success while executing nothing: `tsc --noEmit -p` skips a `composite` project whose .tsbuildinfo is current, and a mistyped workspace filter matched no projects while pnpm exited 0. 6. THE LINE-ANCHORED PINS, RE-DERIVED RATHER THAN OFFSET. The reject map, the DELEGATED reject map, the quarantine map and two citations all track core by line and moved. Every number comes from the same scan the harvest test runs. The reject codomain is unchanged at 30 literals across 35 sites while 38 entries moved, which is the evidence that these edits are inert on behaviour. Four prose citations were stale, two of them before this branch touched anything, and are replaced by symbol references.
… not two integers
A verified tombstone arriving at the sequence a record already holds was answered
by comparing two version numbers. Below the applied version it was discarded as
`stale`, which is a SETTLED outcome, so the revocation was gone permanently: a
peer that tombstones at one version while still serving a higher active head is
ignored forever by every receiver that missed the revocation. No adversary is
needed beyond that peer, no absent producer is involved, and the path is live
today through the real storage entry.
ADR 0002 :112-114 decides it the other way -- a tombstone "dominates every active
head in that sequence regardless of delivery order or version" -- and :126
resolves active/tombstone conflicts to the tombstone. Core implements that clause
for clause. This routes the disjunct to core and maps the answer.
PROVENANCE, STATED PLAINLY. The discard is PRE-EXISTING; it was flat `stale`
before this branch existed. What this branch did was split the disjunct out and
write a justification declaring it safe, which is a correct exclusion mistaken
for a disposition. The pinned population figure that read "adjudicated: 0" for
this region measured the SWEEP'S REACH, not the branch's correctness, and a
pinned zero that measures the instrument reads exactly like one that measures the
system. Direct construction minted a cell without difficulty.
1. THE RULE IS EXTRACTED ONTO ITS OWN OPERANDS.
`evaluateSameSequenceTombstoneRuleV1` takes the candidate, its predecessor, and
three facts about the applied head -- state, version, head digest. The full
evaluator adapts its accepted head into those arguments; the new entry passes
what a receiver already persists. One implementation, both callers, and core's
behaviour is unchanged: its own suite is green at 1,700 baseline tests and 120
system-record tests.
The adapter keeps the predecessor RESOLUTION, because falling back to the
accepted head when no predecessor evidence was supplied needs the head OBJECT and
only that evaluator holds one. The rule takes the resolved operand, so a caller
with no such object reaches the same decision through the same code.
2. THE ENTRY, WITH BINDING-BEFORE-VERSION AS A PINNED CONTRACT.
`evaluateAgentProfileSameSequenceTombstoneAdvanceV1` is exported; the gate moves
from 275 to 276 exact symbols, admitted deliberately. Its operands are a
persisted ROW -- status, authority sequence, version, head digest -- not a head
object. Asking for the object would repeat the operand gap that made the
late-tombstone entry necessary, and a caller manufacturing one would be inventing
the issuer, clock and schema fields its digest is taken over.
The rule decides bindingness BEFORE it reads any version, and that ordering is a
CONTRACT rather than an accident of branch layout: an unbound candidate answers
with the same reject at every version relation, so a caller mapping these answers
never has to ask the binding question itself. That matters because the predicate
answering it is deliberately not part of the package surface -- un-exported one
round ago on purpose. Asserted, not stated: three unbound cells, one answer, and
moving the version branch above the predecessor check turns that row red.
The status union is core's own, so the step from a persisted status to the head
state the rule branches on is taken in core, where both vocabularies are defined.
A receiver making that step itself would be modelling core's authority reading in
order to call core.
The declared return is the FULL authority union, and here that is measured rather
than conceded: the rule produces all four decisions. Narrowing it would be false
about the body -- the opposite direction from the published transition evaluator,
whose union is a promise to consumers. A compiled-lane pin fails on either error.
3. THE ROUTING, WITH THE PRECONDITION FIRST AND NO DEFAULT.
The applied row's authority classification is consulted BEFORE core, because
core's rule reads no classification and running it first on a row V1 has not
classified would produce an answer carrying a clearance nobody granted. Both
undecided branches defer, under different reasons because the gaps differ:
`undecided-authority-classification` -- a shadow-dirty row is an honest unknown.
`same-sequence-tombstone-conflict` -- a tombstoned row is one ADR :127-128
DOES decide, on operands this package holds, and this path cannot yet express
the clearance. That is a choice with a named cost, not a limit.
Core's answer is then mapped exhaustively over the full union with no default:
accept advances; reject defers as `tombstone-predecessor-unbound`; `quarantine |
head-fork` DECLINES to adopt and keeps today's `authority-history-mismatch`,
keyed on the literal verdict, pending the filed ADR question on whether an
equal-version active/tombstone conflict is a fork at all; stale maps to stale.
The stale arm is written out although the precondition makes it unreachable. An
unreachable arm written explicitly is the difference between "this cannot happen"
being enforced and being believed; if the precondition is ever relaxed the arm is
already correct rather than whatever a default swept it into.
Keying the decline on the VERDICT rather than pre-filtering the cell is what
makes it self-maintaining: when the ADR answer moves core's verdict, the cell
follows with no change here.
4. THE MOVEMENT, PINNED BEFORE THE ROUTING WAS WRITTEN.
Nine cells, three applied statuses by three version relations, measured green
against the pre-routing tree asserting the BEFORE column and changed only by the
routing landing:
cell BEFORE AFTER
lower/active stale ready (advance)
lower/tombstone stale deferred|same-sequence-tombstone-conflict
lower/dirty stale deferred|undecided-authority-classification
equal/active deferred|authority-history-mismatch UNCHANGED
equal/tombstone deferred|authority-history-mismatch deferred|same-sequence-tombstone-conflict
equal/dirty deferred|authority-history-mismatch deferred|undecided-authority-classification
higher/active deferred|authority-history-mismatch ready (advance)
higher/tombstone deferred|authority-history-mismatch deferred|same-sequence-tombstone-conflict
higher/dirty deferred|authority-history-mismatch deferred|undecided-authority-classification
= 1 stale->advance, 1 deferred->advance, 2 stale->deferred, 4 reason-only,
1 unchanged. `ready` rather than `advance` is a layer difference: the classifier
returns `advance` and the derivation the driver observes turns it into a plan.
TWO CELLS DELETE A PROJECTION, and both are ADR :112-114 compliance rather than a
protocol change: each candidate arrives verified and bound to its exact
predecessor, and only the arithmetic was refusing it. Both are scoped to the
applied-head-is-not-the-predecessor shape; when the applied head IS the exact
predecessor the derivation short-circuits upstream, which is the ordinary apply.
THE UNCHANGED CELL IS PINNED AS A PAIR -- core's verdict beside storage's --
because `authority-history-mismatch` is also the general fall-through and has
several producers, so a single-value pin would read "unchanged, mechanism intact"
in the two cases that matter most: the cell no longer being routed, or falling
through for an unrelated reason.
THE CONVERGENCE COST, NAMED. Two receivers learning two competing tombstones in
different orders keep deferring instead of converging on the lower one, until the
follow-up that gives ADR :127-128 a clearance of its own lands. It is bounded and
recoverable: no row is discarded and every one retries.
5. THE REJECT ARM CANNOT FIRE THROUGH STORAGE, AS A RUN RATHER THAN AN ARGUMENT.
All nine conjuncts of the binding predicate were attempted individually and every
one is refused upstream of the classifier, with the bound control minting in the
same run. Four are refused by the head codec as malformed heads; five by the
verification closure at summary-mint -- because the closure runs THE SAME binding
predicate over the heads it parses. The producer of the evidence storage requires
enforces exactly the conjunction core's rule tests, so no unbound tombstone can
carry a summary. Constructible at the entry, unconstructible-with-cause at the
seam.
One conjunct is marked reasoned-not-measured: version strictly-greater, where the
only non-greater version against a version-zero predecessor is zero, and the
codec refuses a version-zero head carrying a previousHeadDigest. A different rule
pre-empts the one under test, and reachability is not strictness.
6. THE ROUND-3 CARVE-OUT IS REVERSED AT THE ROW THAT PINNED IT.
The late-tombstone suite's row asserting `stale` for this disjunct now asserts
the advance, on the identical construction, so the movement is legible where the
old answer was pinned rather than only in the new suite.
7. GUARDS, EACH PROVEN WITH A PREDICTED FAILURE.
Twelve source mutants run serially, each proven applied in src and -- for core, which
storage executes from dist -- rebuilt so the executed code carries it, then
restored byte-identical with a green control. Ten killed with the predicted
observation, two declared survivors:
swap the two precondition reasons -> KILLED, exactly six cells swap
remove the precondition -> KILLED, the six non-active cells move
accept -> stale -> KILLED, exactly the two advancing cells
quarantine decline -> advance -> KILLED, cell row and pair-pin
version before binding (core) -> KILLED, only the contract row
drop the entry's sequence guard -> KILLED, only the refusal row
quarantined row reads as active -> KILLED, only the refusal row
equal-version fork -> accept -> KILLED, cell row and pair-pin
stale arm -> advance -> SURVIVES, declared: precondition-unreachable
reject reason reuse -> SURVIVES, declared: arm unreachable, see 5
widen the evidence key list -> KILLED, only the confusable-evidence case
drop the applied-row status check -> KILLED, only that refusal, message and all
Three compiled-lane type pins likewise proven by mutation: adding a clock to the
evidence shape, adding a head object to the applied row, and narrowing the entry's
return each produce the predicted TS2322, with a green restore at 276 symbols.
A first pass reported five mutants as ANCHOR MISSED and none as surviving, which
is the harness working: the tree is CRLF and the anchors were LF, and a
multi-line anchor that matches nothing reads exactly like a surviving mutant.
8. THE PINS THIS MOVED, RE-DERIVED RATHER THAN OFFSET.
The reject harvest is re-derived from source with the same scan the test runs:
33 distinct literals across 38 sites, up from 30 across 35, the three new ones
being the entry's own preconditions. The ambiguity register is unchanged at six,
so the new literals each have one producing site. The quarantine map moved with
the file. Seven line-anchored citations were re-resolved by a tool that DISCOVERS
registers rather than being told which exist, and refuses on zero discovery. One
was ambiguous by text and was followed by SITE, as its own register instructs.
The authority-classification reader now has two call sites, one per routed
tombstone arm, and the consumer pin says so with the reason: each arm decides
what an unclassified row means for ITS rule, and one arm must not answer for the
other. The vocabulary tripwire is untouched -- the lowercase noun remains at zero
occurrences in storage's source, comments included.
REVIEW ROUND 9. The duplication is this branch's own, created one round ago: the same-sequence seam suite got its tombstone builder by copying the late-tombstone suite's and adding two parameters. WHY IT IS WORTH A COMMIT RATHER THAN A NOTE. Two copies of a fixture prove that they agree, never that either one is correct, and a shared mistake passes both. The rules this builder encodes are exactly the ones a head-codec change moves: which active-only fields must be deleted, which projection counters must be zeroed, and that the result has to be re-validated rather than trusted. A drift would not have failed loudly -- one suite would have kept building a shape the codec no longer accepts, and the codec's refusal is phrased like a domain refusal rather than a fixture one. Two seats on this branch have already lost a wall to that confusion. `buildTombstoneHeadFromPredecessorV1` takes the predecessor, an optional version and optional field overrides. Both suites call it; five imports the extraction orphaned are removed. TWO THINGS DELIBERATELY NOT MERGED INTO IT. The version-is-a-parameter reasoning travels WITH the builder, because it is a property of the builder rather than of either caller: the binding predicate wants a version strictly GREATER than the predecessor's, not adjacent, so hard-coding `predecessor.version + 1` reaches exactly one version relation and makes the other two look unconstructible. That cost a wall before it was parameterized. The load-bearing coincidence stays in the same-sequence suite, because it is a fact about that suite's chain and not a general guarantee: the closure resolves the predecessor through the tombstone's own `previousHeadDigest` against the ancestry while the mint graph supplies only the owned-subject table, and those agree only because the current head is a spread of the base. Promoting it into the helper would have made a coincidence look like a contract. Both seam suites green at 23 tests; the four verdict-diff lanes that pin lines into these files still resolve, re-checked by the discovery tool rather than by assuming a test-only change cannot move a citation.
| // what the enumerated-not-collected discipline above says should happen. A | ||
| // reason added to the source union moves this list and takes the images with | ||
| // it, instead of being absorbed silently. | ||
| 'deferred|late-tombstone-evidence-incomplete', |
There was a problem hiding this comment.
🟡 Issue: The storage codomain pin omits two newly added deferred reasons
What's wrong
The test helper claims to enumerate the complete storage outcome codomain from the closed reason unions, but this PR adds new production reasons that are not included here. That weakens the verification artifact: omitted labels are not pinned as reachable or intentionally unreachable, so future regressions in those mappings can slip past the aggregate evidence.
Example
A change from reason: 'tombstone-predecessor-unbound' to reason: 'authority-history-mismatch' at the new same-sequence reject mapping would not be caught by the codomain/reach pin, because tombstone-predecessor-unbound is not listed as an expected storage outcome at all.
Suggested direction
Add deferred|same-sequence-tombstone-conflict and deferred|tombstone-predecessor-unbound to STORAGE_OUTCOME_CODOMAIN_V1, and add corresponding expected reach rows, likely zero for the currently unreachable aggregate paths if that is the intended evidence.
For Agents
Update the verdict-diff codomain/reach fixtures in packages/storage/test/helpers/authority-verdict-diff-join-v1.ts and authority-verdict-diff-join-table-v1.ts to include all new SystemRecordActiveDerivationDeferredReasonV1 labels, including zero-reach labels. Preserve the existing direct seam tests, but make the aggregate codomain test fail if any new reason is omitted.
There was a problem hiding this comment.
🟡 Issue: The join codomain pin omits two new storage outcomes
What's wrong
The verification harness says it enumerates storage's complete outcome codomain, but this PR adds storage deferral reasons that are not listed there. That makes the join's zero-reach evidence incomplete: an unexercised new outcome can be absent from the codomain and absent from the unreached-outcome register, while the codomain test still passes because it only checks labels it already knows about.
Example
A future join fixture that still does not exercise same-sequence tombstone conflict would keep reporting a "complete" codomain without a zero row for deferred|same-sequence-tombstone-conflict; the current codomain-reach test builds observed only by mapping over STORAGE_OUTCOME_CODOMAIN_V1, so it cannot fail for that missing outcome.
Suggested direction
Keep the enumerated codomain synchronized with the source union, and pin the missing outcomes as reached counts or explained zeroes.
For Agents
Update packages/storage/test/helpers/authority-verdict-diff-join-v1.ts and packages/storage/test/helpers/authority-verdict-diff-join-table-v1.ts so the codomain includes every member added to SystemRecordActiveDerivationDeferredReasonV1, including deferred|same-sequence-tombstone-conflict and deferred|tombstone-predecessor-unbound. Add reach counts, likely zero where the join fixture cannot reach them, and add JOIN_UNREACHED_OUTCOMES_V1 explanations for any zero rows.
There was a problem hiding this comment.
🔴 Bug: The storage codomain pin omits two new deferred outcomes
What's wrong
The aggregate verification claims to enumerate storage's complete outcome codomain, but this PR adds four deferred reasons in production and only two are added to the codomain list. That makes the codomain/reach tests give false confidence: they can stay green while new storage outcomes are not represented in the aggregate verdict-diff evidence.
Example
A later change could make the tombstone-predecessor-unbound arm reachable through the join sweep; until this list includes deferred|tombstone-predecessor-unbound, the codomain/reach pins cannot assert its expected zero or nonzero count. Likewise same-sequence-tombstone-conflict is observed in the dedicated seam suite but absent from this aggregate codomain.
Suggested direction
Add the missing labels to the codomain pin and assert their expected reach, rather than letting the aggregate verification surface silently ignore them.
For Agents
Update packages/storage/test/helpers/authority-verdict-diff-join-v1.ts so STORAGE_OUTCOME_CODOMAIN_V1 enumerates every label created by the changed deferred-reason union, including deferred|same-sequence-tombstone-conflict and deferred|tombstone-predecessor-unbound. Then update the reach/unreached pins to prove whether each is expected to be reached by the join sweep or held at zero.
There was a problem hiding this comment.
🔴 Bug: The storage codomain pin omits two newly added deferred outcomes
What's wrong
The join helper says it enumerates storage's complete outcome codomain, and downstream tests use that list to prove unreached outcomes are still pinned. This PR adds four storage deferred reasons, but the codomain list only adds two. That gives false confidence: omitted zero-reach outcomes are not checked, and one omitted outcome is already exercised by the new same-sequence seam test.
Example
A failing-test sketch for the codomain pin: assert that every SystemRecordActiveDerivationDeferredReasonV1 member appears as deferred|<reason> in STORAGE_OUTCOME_CODOMAIN_V1. It would fail today for same-sequence-tombstone-conflict and tombstone-predecessor-unbound.
Suggested direction
Add deferred|same-sequence-tombstone-conflict and deferred|tombstone-predecessor-unbound to the codomain table, and pin their current reach explicitly if the join does not exercise them.
For Agents
Update packages/storage/test/helpers/authority-verdict-diff-join-v1.ts so STORAGE_OUTCOME_CODOMAIN_V1 includes all newly added deferred reasons, with zero reach/inventory pins where appropriate. Add a source-union-to-codomain assertion so future reason additions cannot silently fall out of this verification table.
…reading what it names
REVIEW ROUND 9, and it closes a class this rule has now reproduced four times --
each time one arm over from the last fix.
THE SHAPE. The late-tombstone rule ran the transition verifier, then discarded
its result on two of three binding arms and re-derived the relationship
structurally. So a transition that could not be verified AT ALL was still read
structurally, and on the `names-another-head` arm that reading is an ADMISSION
which deletes the record. It was harmless only by the coincidence that exactly
one non-binding refusal is reachable today and ignoring it moves no verdict. It
stops being harmless the day the verifier learns a refusal that ought to mean
refuse -- a signature check being the obvious candidate, since envelope fields
sit outside the unsigned codecs.
TWO INDEPENDENT SOURCES, OPPOSITE DIRECTIONS, ONE PROPERTY. An adversarial
verification of this seam built from the ADR text found the discarded-refusal
structure and named the forged-transition consequence. The review round asked for
one binding classification instead of computing it twice. They agreed on the
diagnosis and differed on the remedy: the review's proposed order -- classify
first, verify only on the naming arm -- preserves today's verdicts exactly and
makes the hazard STRUCTURAL, because no verifier refusal could ever reach the
other two arms again. This takes the simplification and fixes the order instead
of freezing it.
THE SPLIT IS A CLASS, NOT A HOIST. `rejectUnverifiableAuthorityTransitionV1`
names the refusals that are about the TRANSITION -- whether it can be checked at
all, right now -- and are therefore independent of what it binds.
`rejectInadmissibleExpiredPriorTransitionV1` names the ones that are questions
about THIS binding, since every one compares the transition against the prior
head it names. Hoisting a single condition would have closed the instance and
left the next binding-independent refusal free to repeat it; now every future
refusal of that kind joins a named group and its callers get the ordering
without noticing.
`evaluateAuthorityTransitionInternalV1` is composed from the same two parts in
the same order, so the verifier's own decisions are unchanged -- and the three
callers of the published form are untouched.
THE RULE now reads: retained-transition exactness, clock preflight,
VERIFIABILITY, then ONE binding classification. The `names-this-head` arm asks
only the expired-prior question, because re-running the whole verifier there
would recompute exactly the classification just performed in order to
reconstruct a reason it had already thrown away.
TWO CELLS MOVE, BOTH AT THE ENTRY LAYER, BOTH TOWARD REFUSAL:
cell BEFORE AFTER
names-another-head + future-dated accept the temporal refusal
unrelated + future-dated belongs to another … the temporal refusal
The first is the one that mattered. The second is reason-only and is the honest
answer: with an object nothing can verify, asserting whose authority it belongs
to is a claim derived from unchecked bytes.
NO CELL REACHABLE THROUGH A RECEIVER MOVES, and that was the condition on making
this change rather than a hope. The single storage call site constructs its
evidence as a frozen `{ tombstonePredecessor }` literal -- `retainedTransition`
is never written, not merely left undefined -- and `AgentProfileAuthorityTransitionV1`
occurs in NO executable code in that package, two occurrences and both comments,
with the identifier itself found by the same scan as the positive control. Core
therefore refuses at the retained-transition clause before the verifier is
consulted on any reachable cell.
PROVEN BY COUNTERFACTUAL, NOT BY THE ROWS PASSING. A new row that passes on
first run has shown only the after-state. Removing the verifiability gate --
applied in src, rebuilt into the dist storage executes, restored byte-identical
-- turns the row RED with exactly the predicted observation: `namesAnotherHead`
reverts to `accept` and `unrelated` reverts to the belongs-to-another-authority
literal. The round-5 row goes red in the same mutant, which is the gate being
load-bearing on all three arms rather than only on the two that moved.
GATES, RUN SERIALLY. Core baseline 107 files / 1,700 tests, system-record 120
passing with the base-confirmed golden-v1 Windows red, export 276 exact symbols
-- unchanged, because both new helpers stay off the barrel. Storage 1,207
passing with only the recorded worker-adapter contention family failing;
type-contracts clean; package-exports all mutants killed. The reject harvest and
both citation registers were re-derived from source rather than offset: 33
literals across 38 sites, the ambiguity register unchanged at six, the delegated
map re-ordered by its new line positions.
RECORDED AGAINST MYSELF: an earlier core run in this session reported three
failures and was VOID -- I had left a full storage suite running in the
background, so two suites were competing. The serial re-run is the measurement;
the concurrent one measured my own scheduling. Attribution by A/B, not by
re-running until it was green.
…has ten The unbound-construction enumeration shipped nine conjuncts measured and the tenth argued out. Both halves of that were wrong, and the second one is the more useful correction. THE ARGUMENT THAT REMOVED THE ROW WAS SOUND AND OUT OF SCOPE. It ran: against a version-zero predecessor the only non-greater version is zero, and the codec refuses a version-zero head carrying a previousHeadDigest, so a different rule pre-empts the one under test. Every clause is true. It is scoped to the PREDECESSOR THAT WAS CHOSEN rather than to the conjunct. Pick a predecessor above zero -- the version-2 active head -- and a version-1 tombstone violates strictly-greater with neither head at zero. MEASURED, control minting green in the same run: the head codec ACCEPTS it and the closure refuses the mint with the same predecessor-not-exact message as the other five closure refusals. It belongs in the closure-refused bucket as measured. Reachability is not strictness -- and neither is a sound argument about one construction a statement about the population. A reasoned omission leaves the row uncovered, and this row is the only assertion watching that conjunct: the mutant it kills is relaxing `>` to `>=`, a one-character edit that would let a tombstone bind a predecessor at its OWN version, which is the equal-version confusion the open ADR question turns on. THE PREDICATE HAS TEN CONJUNCTS AND THIS SUITE SAID NINE FOR SEVERAL ROUNDS. The count came from listing the field comparisons and not noticing that the version relation is a conjunct too -- the same omission that then made it arguable out of the enumeration. Nine `&&` operators means ten conjuncts. The count is now derived at the site rather than asserted in prose, because a number in a comment is not asserted by anything, and this one was wrong in the direction that made the enumeration look complete. A lever is now a thunk rather than a field override, because this conjunct is violated by the PREDECESSOR CHOICE and cannot be reached by perturbing a field. AND THE TEST IS A TRIPWIRE FOR ANOTHER MODULE'S GATE, now stated as such. The unconstructibility it proves rests on the verification closure applying the binding predicate before it mints -- a property of a different module. If that gate is relaxed, these heads begin to mint and every refusal here turns red at once. That is the dependency being self-protecting rather than assumed. Enumeration green at ten rows; no production code changes.
| : Object.freeze({ outcome: 'deferred', reason: 'undecided-authority-classification' }); | ||
| } | ||
| const predecessor = facts.verifiedAuthoritySummary.tombstonePredecessor; | ||
| const decision = evaluateAgentProfileSameSequenceTombstoneAdvanceV1( |
There was a problem hiding this comment.
🔴 Bug: Same-sequence tombstones can advance across a different authority root
What's wrong
The new storage route asks a reduced core rule to decide same-sequence tombstones, but that reduced rule has no access to the applied row's current root, issuer, or retained history. That drops the same-sequence authority-change guard that exists in the full evaluator. A self-consistent tombstone for an alternate branch at the same sequence can therefore be treated as a valid revocation of the currently applied active row, corrupting root/projection state.
Example
Current applied row is active at authoritySequence "2" with currentRoot A. A verified tombstone candidate at sequence "2" is built from an alternate predecessor/root B and has version "1" or "3". The new call at line 1200 gives core no currentRoot/evmIssuer/history, so evaluateAgentProfileSameSequenceTombstoneAdvanceV1 returns accept for an active applied row with unequal version, and line 1225 advances the tombstone. The full evaluator would have rejected/deferred this as a same-sequence authority change instead of deleting A's projection using B's tombstone evidence.
Suggested direction
Bind the same-sequence tombstone decision to the persisted authority/root lineage before mapping core accept to advance. The public applied-row shape likely needs the current root/issuer and history, or storage needs to reject/defer mismatches before calling the narrowed rule.
For Agents
Look at classifySameSequenceTombstoneAdvance and AgentProfileSameSequenceAppliedRowV1. Preserve the intended lower/higher-version tombstone acceptance only when the candidate is on the same authority/root lineage as the applied row; add currentRoot/evmIssuer and/or lineage/history operands, or keep a storage-side guard equivalent to the full evaluator's same-sequence authority check. Add a test with a same-sequence tombstone whose verified predecessor/root differs from the current applied root and prove storage does not advance it.
There was a problem hiding this comment.
🔴 Bug: Same-sequence tombstones can advance over a different authority branch
What's wrong
The new same-sequence storage route asks core to decide with a row shape that omits the applied authority branch identity. For an active applied row, the core rule returns accept for any unequal version once the candidate binds its own predecessor, but it never checks that the tombstone belongs to the same same-sequence branch as the row being replaced. That can turn what should be an authority mismatch/equivocation into an accepted tombstone.
Example
Current applied row is active at authoritySequence 2/version 2 on transition digest T1/root R1. A verified tombstone for the same peer and sequence, version 3, binds its own predecessor on transition digest T2/root R2. The full evaluator would stop before the tombstone rule (same-sequence authority changed or transition-equivocation), but this storage path passes only {status, authoritySequence, version, headDigest} and core returns accept, so storage advances and deletes the current projection.
Suggested direction
Include and check the same preconditions the full evaluator applies before the tombstone rule, such as current root/issuer and accepted transition lineage/digest, or widen the core same-sequence persisted-row operand so it can reject/quarantine branch mismatches before any accept.
For Agents
Look at classifySameSequenceTombstoneAdvance and evaluateAgentProfileSameSequenceTombstoneAdvanceV1. Preserve the new lower/higher-version tombstone behavior only after proving the candidate is on the same authority branch as the applied row. Add a storage seam test where the tombstone predecessor/summary lineage differs from current.transitionLineage or current.currentRoot, and prove it defers/quarantines instead of advancing.
…ator The tenth-conjunct row shipped justified by a mutant it does not kill. Relaxing `>` to `>=` in isTombstoneBoundToPredecessorV1 changes the verdict only at EQUAL versions, and that row violates strictly-greater FROM BELOW -- a version-1 tombstone under the version-2 predecessor, where `1 > 2` and `1 >= 2` are both false. Measured rather than argued: with the mutant applied in src and in the rebuilt dist, the enumeration stayed GREEN on all ten rows. Adds the equal-version construction, which is the discriminator and is proven to be one. Under the same mutant it alone moves -- `closure-mint` to REACHED THE CLASSIFIER -- while every other row holds, and the restore control is green in src and in the rebuilt dist. Also corrects three claims the docblock could no longer carry: the row-count arithmetic (eleven rows covering ten conjuncts), an `&&` span that had been eaten out of the derived-count sentence, and the mutant claim itself. The general form is worth more than the instance: a row justified by a mutant has to be RUN against that mutant, because a construction chosen to violate a conjunct is not automatically a construction that separates the operator expressing it.
What this changes
ADR 0002
:129-133freezes the late-tombstone rule:Storage answered all of that with a comparison of two sequence numbers and returned a flat
stale. This routes that one decision through core.The headline: late tombstones now reject-for-retry per the ADR instead of silently going stale.
And a second, heavier one found by independent verification after the rounds had gone quiet: a tombstone at the SAME sequence was discarded as
staleon a version comparison, which permanently drops a verified revocation. That is live today, it is the region this PR had carved out as a different rule, and it is fixed here. See "The revocation bypass this PR's own exclusion was hiding" below. The routing is CONSERVATIVE — it admits nothing new.acceptbecomes reachable at zero storage cost the day a producer supplies the retained transition.The fail-before, built rather than argued
The compliance claim previously rested on reading two documents side by side. It is now a construction, run against the real exported entry
deriveSystemRecordReplacementV1:priorHeadDigestis the ACTIVE head at sequence 1, not the tombstone — so it does not name the tombstone, and the ADR's "otherwise" applies.Measured on that one built state, before any routing existed:
acceptlate tombstone requires the exact retained resurrection transitionstaleAnd storage returned the same
stalewith the applied row'stransitionLineage[1].transitionDigestperturbed through the real codec round-trip — the comparison reads.length, never the contents. That converts "checks neither" from a reading into a behavioural fact.The routing
evaluateAgentProfileLateTombstoneAdvanceV1is a new core export that validates its inputs the way the full evaluator does and delegates to the existing private lower-sequence arm, so every clause of the rule stays in that arm. It ships with an explicit operand contract: which transition is required (the one OUT of the candidate's sequence), that the predecessor is required in substance though typed optional, that an absent transition yields a REJECT and never a stale, and that the clock is read only after a transition has matched.It exists rather than reusing
evaluateAgentProfileHeadAdvanceV1because that entry requires the accepted head as an object and a receiver holds a head digest. Synthesising one would inventpeerPublicKey,evmIssuer,issuedAt,projectionSchemaDigestandversion, producing a head whose digest does not match the persisted one — a fabricated operand decided as though it were evidence. That operand gap is the sole basis for the entry design.Storage contributes a mapping and no authority logic. The mapping is exhaustive over core's decision union with every branch written out — including
quarantine, which this arm cannot produce today but which must never be laundered into a deferral by falling off the end of a switch — and aneverassignment makes a widened union a compile error rather than a silent default.The retained transition is not available to storage, and that is a property of the inputs.
AgentProfileAuthorityTransitionV1occurs zero times inpackages/storage/src(positive control:AgentProfileAppliedTransitionV1, the digest summary, found at three sites by the same grep); the applied row persists transition digests only; and a late tombstone's own verification closure covers its lineage, not the rotation out of its sequence. The tombstone issue object has exactly 12 keys, none a transition.Phase 2's reader is consumed as an explicit precondition. A tombstoned or shadow-dirty row has no authority classification in V1 and core's accepted state has no member for "unknown", so those rows defer under their own reason rather than having
discoverableinvented for them.assertTrustedTombstoneReplacementstill runs first and still throws. Its conjuncts overlap rather than nest with core's: storage checkspreviousHeadDigest,peerId,authoritySequence,rootSubjectplus the deletion table, owned-subject count and epoch; core additionally checkspeerPublicKey,acceptedTransitionDigest,evmIssuer,projectionSchemaDigestand version-strictly-greater. Routing adds five checks and drops none.The revocation bypass this PR's own exclusion was hiding
An independent adversarial review of this seam — run from the ADR text rather than from these tests, and commissioned precisely because the seam had already produced four decision-inverting defects its author did not see — found a live, high-severity bypass in the region this PR carved OUT of the routing.
A verified tombstone arriving at the sequence a record already holds was decided by comparing two version numbers. Below the applied version it was discarded as
stale, andstaleis SETTLED, so the revocation was gone permanently: a peer that tombstones at one version while continuing to serve a higher active head is ignored forever by every receiver that missed the revocation. No adversary beyond that peer, no absent producer, live today through the real storage entry.ADR 0002
:112-114decides it the other way — a tombstone "dominates every active head in that sequence regardless of delivery order or version" — and:126resolves active/tombstone conflicts to the tombstone.The provenance, stated plainly, because it is the more useful half. The discard is PRE-EXISTING; it was flat
stalebefore this branch existed. What this branch did was split the disjunct out and write a justification declaring it a different rule and therefore safe. The split was a correct EXCLUSION and it was not a disposition. And the instrument said nothing: the population pin recordedadjudicated: 0for that region, which measured the sweep's reach — its axes cannot mint summaries for that combination — not the branch's correctness. A pinned zero that measures the instrument reads exactly like a pinned zero that measures the system. Direct construction minted a cell without difficulty.The fail-before, again as a construction
Nine cells — three applied statuses by three version relations — driven through the real storage entry before any routing existed, asserting the pre-routing column. The arrangement is forced rather than chosen: the predecessor must NOT be the applied row, because when it is, the derivation short-circuits to
advanceupstream at the exact-match conjunction, which is the ordinary tombstone apply and a different population.The unlock was one word.
isTombstoneBoundToPredecessorV1requires the tombstone's version to be strictly greater than its predecessor's, not adjacent — so minting atpredecessor.version + 1reaches exactly one relation and makes the other two look unconstructible. From the version-0 chain base, all three relations against a version-2 applied row are available.The routing, and what moved
The entry is
evaluateAgentProfileSameSequenceTombstoneAdvanceV1; the export gate goes from 275 to 276 exact symbols, admitted deliberately. Its operands are the persisted row — status, authority sequence, version, head digest — never a head object, so the operand gap that made the late-tombstone entry necessary does not recur.Bindingness is decided before any version is read, and that ordering is a pinned contract rather than an accident of branch layout. An unbound candidate answers with the same reject at every version relation, so a caller mapping these answers never has to ask the binding question itself — which matters because the predicate that answers it is deliberately NOT on the package surface, un-exported one round ago on purpose. Asserted, not stated: three unbound cells, one answer, and moving the version branch above the predecessor check turns that row red.
The applied row's authority classification is consulted first, because core's rule reads no classification and running it first on a row V1 has not classified would produce an answer carrying a clearance nobody granted. The two undecided branches defer under different reasons, because the gaps differ:
undecided-authority-classificationfor a shadow-dirty row (an honest unknown), andsame-sequence-tombstone-conflictfor a tombstoned row — a case ADR:127-128DOES decide, on operands this package holds, which this path cannot yet express. That one is a choice with a named cost, not a limitation.Core's answer is then mapped exhaustively with no default.
acceptadvances;rejectdefers astombstone-predecessor-unbound;quarantine | head-forkdeclines to adopt and keeps today'sauthority-history-mismatch, keyed on the literal verdict, pending the filed ADR question on whether an equal-version active/tombstone conflict is a fork at all. Keying the decline on the VERDICT rather than pre-filtering the cell is what makes it self-maintaining: when the ADR answer moves core's verdict, the cell follows with no change here.1 stale→advance, 1 deferred→advance, 2 stale→deferred, 4 reason-only, 1 unchanged.
readyrather thanadvanceis a layer difference and not a disagreement: the classifier returnsadvance, and the derivation the driver observes turns that into a plan.Two cells delete a projection, and that is the compliance
Both advancing cells are the first movements this PR makes in the data-destroying direction that are reachable today, and both are licensed by ADR
:112-114"regardless of delivery order or version". Each candidate arrives verified and bound to its exact predecessor; only the arithmetic was refusing it. Both are scoped to the applied-head-is-not-the-predecessor shape — when the applied head IS the exact predecessor, the derivation short-circuits upstream into the ordinary apply.The unchanged cell is pinned as a PAIR — core's verdict beside storage's — because
authority-history-mismatchis also the classifier's general fall-through with several producers, so a single-value pin would read "unchanged, mechanism intact" in exactly the two cases that matter most: the cell no longer being routed at all, or falling through for an unrelated reason.The convergence cost, named. Two receivers learning two competing tombstones in different orders keep deferring instead of converging on the lower one, until the follow-up giving ADR
:127-128a clearance of its own lands. It is bounded and recoverable: no row is discarded and every one retries.The reject arm cannot fire through storage, and that ships as a run
All nine conjuncts of the binding predicate were attempted individually with the bound control minting green in the same run. Every one is refused upstream of the classifier: four by the head codec as malformed heads, five by the verification closure at summary-mint — because the closure runs THE SAME binding predicate over the heads it parses. The producer of the evidence storage requires enforces exactly the conjunction core's rule tests, so no unbound tombstone can carry a summary. Constructible at the entry, unconstructible-with-cause at the seam.
One conjunct is marked reasoned-not-measured rather than counted: version strictly-greater, where the only non-greater version against a version-zero predecessor is zero, and the codec refuses a version-zero head carrying a
previousHeadDigest. A different rule pre-empts the one under test, and reachability is not strictness.The round-3 carve-out is reversed where it was pinned
The late-tombstone suite's row asserting
stalefor this disjunct now asserts the advance on the identical construction, so the movement is legible at the place that pinned the old answer rather than only in the new suite.The operational change, sized
This is the one behaviour change the slice ships, and it is head-of-line blocking, not extra retries. Measured at
packages/agent/src/system-records/reconcile-v1.ts:697-702:isSettledOutcomecountsapplied | already-applied | **stale**as SETTLED. A settled row isshift()ed offpendingRowsand the loop advances.deferred— returnsblockedwithapplyBlockReasonmapping it toapply-deferred, and the row stays onpendingRows.So a late tombstone that used to be discarded and skipped would now hold the slice at that row. And because storage can never supply the retained transition today, such a row would block on every pass rather than draining. That is the blast radius for all 1,728 cells, both the 576 core-routed and the 1,152 stopped at the precondition.
It is not reachable today, measured at this head rather than inherited:
createAgentProfileReconcilerV1is defined atreconcile-v1.ts:376and has zero other occurrences anywhere inpackages/*/src— no production constructor (positive control:runDurableSync, found five times in the lifecycle file by the same method). This is a third independent confirmation that the lane is inert at merge; the risk lands at D-12's activation gate, which is where this sizing belongs.Reason-string persistence (measured): neither new reason is persisted anywhere, and in fact neither escapes the derivation.
mapZeroWriteatsystem-record-atomic-apply-executor-v1-internal.ts:1282-1283collapses every deferred reason tovalidation-mismatchbefore the outcome leaves the executor. The two members are observable only throughderiveSystemRecordReplacementV1's direct return — which is where the verdict-diff harness drives them, and why they are worth distinguishing — but no operator-facing surface, durable record, or wire format carries them.The coverage proof, and the gate that could not fail
The seam's population is declared from the axes and the cited gates, never from the answer. 1,728 comparable cells reach the routed disjunct (576 per applied status), plus 1,728 with no mintable summary. The movement was derived from the axis arithmetic and pinned before the routing landed; the run matched it exactly, per applied status:
reject -> staleaccept -> stale-> staleAGREEMENT 1,152 unchanged. DIVERGENCE 192 unchanged. NO-MAPPING 384 unchanged. Every cell in the seam changed behaviour and not one bucket total moved — the suite's own four-bucket partition test passed unchanged across the routing. A coverage gate read at bucket level is green through the entire change, which is why the movement ships as row-level pinned data carrying the two-reason split. A reason-agnostic pin would have been satisfied by any deferral label.
The uniform single-reason table (core over every comparable cell with storage's operands) is kept, labelled explicitly as the counterfactual — it is not what ships, because only the 576 decided-classification cells reach core at all.
The second disjunct (same sequence, lower version) is a different ADR rule (
:126-128) and keeps its comparison. It holds 3,456 cells of which zero are adjudicated, so "nothing else moved" there is structural.Those moved agreement rows were nominal agreements: the two sides agreed on the label
stale, not on the semantics — core was refusing for missing evidence while storage was discarding on arithmetic.The 192 does not close, by design. It measures the evidence channel, not the classifier: the join hands core a retained transition the storage path cannot hold.
Corrections this carries
acceptonce the retained resurrection transition validates". Measured insideevaluateLateTombstoneRuleV1, that is inverted: it returnsstalewhen the transition verifier ACCEPTS (a valid descendant exists), and reachesacceptonly through the binding classifier's another-head arm. The flat-stale claim is anchored to the classifier site. This bullet carried two source line numbers until a later insert made them point at an unrelated reject; it addresses the code by symbol now, for the same reason the module's own prose does.quarantined; the classification read makes axis B live. The duplicate-verdict observation survives; the duplicate-behaviour one does not.undecided-authority-classification. The three assertions ship as additions.Verification
stale, and dropping the core entry's precondition all go red in the lane. Dropping the exhaustiveness arm fails the BUILD withTS2322: Type '{ decision: "quarantine"; … }' is not assignable to type 'never'— the guard is live in a realtscprogram rather than decorative in a test file.const alias = fn;is not a call — so it now pins total references, not call sites.testSCRIPT, not just its vitest run. Core:test:baseline107 files / 1700 tests all pass;test:system-record8 passed / 1 failed —system-record-golden-v1, which fails identically at the base commit (a doubled Windows drive letter in the test's own path construction; Windows-local, not reproduced on Linux CI, which was fully green at 58/58).test:system-record-exportpasses at 275 exact symbols, up from 274 — that pin fired on the new export in CI and the symbol was admitted deliberately. Storage's two non-vitest gates pass (test:package-exportsreports all mutants killed;typecheck:type-contractsclean).What the review rounds changed
Round 1 found a real 🔴 in the new core entry, and it was a verdict INVERSION rather than a missing check. The late-tombstone arm reads a non-accept from the transition verifier as "the tombstone takes precedence", and that verifier also refuses on an unusable clock. Measured on one built state with a transition that BINDS the tombstone, where the correct answer is
stale:staleNumber.NaNacceptverification clock is invalid-1acceptverification clock is invalidMy own docblock had claimed the residue of an unusable clock was "a refusal, never an admission". It was the opposite, and nothing tested it — an untested claim in a comment, in the one place it was load-bearing.
The fix is the boundary. The retained transition and its clock are now ONE optional field on a purpose-built
AgentProfileLateTombstoneEvidenceV1, so "binding transition plus unusable clock" is unrepresentable rather than merely refused. Round 2 finished the shape: the rule is a single pure helper over exactly its own operands, called by both entries, and the clock gates live beside the reading they protect.Number.NaNnow appears zero times in both the core authority file and storage next-state.The published type contract is pinned where it compiles. Four conditional-type assertions in the export-types fixture — no top-level clock, no bare transition, exact key set, both halves of the pair required — each proven by widening the interface and watching
error TS2322: Type 'true' is not assignable to type 'never', with a green restore control. That file is one of the few test files atsc --noEmitgate actually compiles, which is the whole reason the pins are worth writing rather than decorative.Two defects surfaced by comments that were about something else. A file-size remark made me open the three pinned seam tables, and they had zero consumers — numbers that read as evidence and could not fail; they are now asserted against the live join and mutation-checked. And this PR had corrupted the axis-J grounding pin: its regex took the first
const optionalsin the core file, and the new snapshot helper landed above the one it meant, so it silently compared one function's members against another's interface.The sibling zero-noun tripwire was restored verbatim after a first draft replaced it with three targeted claims. A guard catches what it names; the tripwire catches what nobody thought to name. They compose rather than substitute. The reason member is named
undecided-authority-classificationso storage source stays at zero occurrences, and the tripwire is proven in both polarities against the file this slice touches.Citations were re-resolved by content, not arithmetic, after the second shift. That decision caught two citations an arithmetic shift would have landed on real-but-wrong lines — a fork-resolution comparison and a lineage field list — where no assertion would have noticed.
Known, and deliberately not fixed here
acceptas reachable today; no invented value stands in for the missing object. Filed as a follow-up, with the note that a producer changing what peers send is a protocol question.Refs #2052