Skip to content

test: the replay-branch Irreversible table has no completeness check, unlike gateExempt for components #198

Description

@amiddavid

The gap

TestNoStateIsRecordedBeforeTheCommitGate has gateExempt: every registered Offload must be either driven by the table or listed with the test that pins it, so a new component cannot silently escape the commit-gate invariant. That mechanism exists because the convention it replaced had already failed once.

TestADegradedModeReplayDeclaresItselfIrreversible has no equivalent. It covers three replay branches by hand, and there are five commitRefresh call sites:

Site eff argument Covered by
extract_llm.go (apply, replay) computed table subtest
extract_sweep_drop.go (sweepDrop, replay) computed table subtest
state.go (reapplyFrozen) hardcoded markerFull mask subtest, via the len(keys) == 0 block
summarize.go (tryReuse) hardcoded markerFull summarize.go's own len(reusedKeys) == 0 check
summarize.go (refuse) hardcoded markerFull same

Why it is worth a mechanism rather than a note

The coverage above is correct today — established by reading all five sites. Only the two that pass a computed eff can reach commitRefresh's eff != markerFull branch, which is the one that sets rep.Irreversible; the other three pass markerFull and handle the degraded case separately and correctly.

But that conclusion rests on reading five call sites, and nothing fails if a sixth is added that passes a computed eff and is not added to the table. The consequence of missing one is not hypothetical: a replay branch that fails to set rep.Irreversible has components/pipeline.go revert the whole component on every replay turn under marker_mode: summary/off, sending the transcript verbatim and re-writing the provider's cached suffix at ~11.5x the read price. Two instances of exactly that were found during the #188 review — one introduced, one pre-existing (#193) — which is what suggests a third is a matter of time.

Suggested shape

The same mechanism as gateExempt, applied to commitRefresh callers rather than to components: a test asserting that every call site passing a non-constant eff is either driven by TestADegradedModeReplayDeclaresItselfIrreversible or listed with the test that pins it, with a reason.

Implementation options, in increasing robustness:

  1. A source scan over components/offload/*.go for commitRefresh( and a hand-maintained allowlist keyed by file:function. Cheap; brittle in the usual ways a regex over source is.
  2. go/ast over the package, classifying each call by whether its eff argument is the markerFull identifier or an expression. Precise, and the classification is exactly the property that matters.
  3. Restructure so the distinction is in the type system rather than in a convention — e.g. separate entry points for "replay of a decision whose mode is known full" and "replay of a decision whose mode is computed", so the covered set is enumerable from the signature. Largest change; removes the need for a test.

Option 2 looks like the right cost/benefit, mirroring what gateExempt does for components.

How it was found

Raised by the reviewer on #188 as an explicit non-finding — it traced all five sites, confirmed the coverage is genuinely complete, and observed that establishing that took reading five call sites while nothing mechanical enforces it. Filed rather than fixed in #188 because it is test infrastructure rather than a defect, and the branch is already long.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions