AUD-S10..S12: bounded reads, pagination caps, retry/backoff, marker resilience

[konih]

Three serial stories on the same file (internal/forge/gitlab/gitlab.go), one commit each, spec-first (failing test → implementation) per openspec/specs/p5-aud-audit-remediation/spec.md lines 543–654.

Every fail-closed branch below is pinned in both polarities — the guard fires and legitimate traffic is provably unaffected — and every new test was mutation-verified (deliberately break the production code, confirm RED). Evidence is listed per story.

---

AUD-S10 — REL-03 / SEC-08: bounded response reads + pagination caps

Commit 6852ba5.

What changed

• internal/forge/gitlab/gitlab.go: readBounded at the shared do seam caps a single response at maxResponseBytes (8 MiB — MB-order, constant, documented; a 100-item discussions page is KB-order and the largest read is a governed policy/registry file). Over-limit discards the prefix and errors, so truncated bytes never reach a decoder.
• internal/provider/transport.go: the same bound (exported provider.MaxResponseBytes) on CallHTTP. An over-limit provider body classifies as unavailable, never resolved, and keeps auto-merge disarmed.
• ListBotThreads / ListBotNotes: page loops capped at maxListPages (100 pages × 100 per page = 10 000 artifacts). Hitting the cap is an error — reconcile must never run against a partial thread/note list, because a silent partial reads as "that finding has no thread yet" and duplicates it.

Fail-closed, both polarities

Guard	Fires	Positive control
byte bound (gitlab)	TestBoundedReadOverLimitFailsClosed, TestBoundedReadAppliesToRawFileReads	TestBoundedReadAtLimitStillSucceeds — a body of exactly the limit parses
byte bound (provider)	TestBoundedReadOverLimitFailsClosed	TestBoundedReadUnderLimitUnaffected
page cap	TestPaginationCapFailsClosed (discussions + notes)	TestPaginationBelowCapUnchanged — a short-page listing returns every artifact

Scope note (beyond the REQ, deliberate and reported). The REQ names two loops. A third page loop existed in the same class: hasApprovalRulesAPI in snapshot.go, uncapped. Leaving it would have left "every pagination loop capped" untrue, so it is capped too — with its own both-polarity coverage (TestPaginationCapFailsClosedApprovalRules and TestApprovalRulesFailSafeSurvivesCap). The cap is an error, not the probe's pre-existing 404/403 → Free-tier fail-safe: a never-shortening paginator is a forge anomaly, not evidence the instance lacks the API. Erroring aborts the run with zero forge writes, strictly safer than the previous unbounded spin (which literally hung the test suite before the fix — that hang was this story's first RED).

The AUD-S01 diff-enumeration ceiling (ADR-0020-owned) is untouched: it still degrades to REVIEW rather than erroring.

Mutation evidence

Mutation	Result
len(raw) > limit → >=	RED TestBoundedReadAtLimitStillSucceeds
provider over-limit returns the truncated bytes instead of nil	RED TestBoundedReadOverLimitFailsClosed
delete the discussions page-cap guard	RED — panic: test timed out after 20s (the spin the cap exists to stop)
maxListPages 100 → 1	RED TestPaginationBelowCapUnchanged (both sub-tests)

---

AUD-S11 — REL-04: retry/backoff + context deadlines

Commit 3848f39.

What changed

• Client gains a parent context.Context and a RetryPolicy, both settable through a new variadic Option seam on New (WithRetry, WithSleeper, WithJitter, WithContext). The context lives on the struct because forge.Forge is a frozen port with no ctx parameters (ADR-0011 / ADR-0017) and the CLI owns one client per run.
• do retries only GET/HEAD, only on a transport error / 429 / 5xx, up to 3 attempts, with an exponential window (200 ms base, clamped at 2 s) spread over its lower half by an injected jitter source, under a 30 s per-request context deadline. The parent context is re-checked before every attempt, so a deadline that blows during a backoff issues no further request.
• Budget exhaustion returns the last failure unchanged, so every caller's existing fail-closed handling of a non-200 or a transport error applies verbatim. Retries move availability, never a decision.

Fail-safe on writes, both polarities

• TestWritesNeverRetried drives all five write endpoints (create thread, create summary note, resolve thread, approve, merge CAS) through the same 503 that the GET cases retry, and asserts exactly one attempt and zero backoff spent for each.
• TestWriteMethodsAreNotRetryable is the table half: GET/HEAD must be retryable, POST/PUT/PATCH/DELETE must not — so neither "retry everything" nor "retry nothing" passes.
• TestIdempotentRetry covers recovery after 5xx, 429, transport error, budget exhaustion → hard error, expired context → hard error with zero requests, and mid-budget cancellation → stops retrying. 4xx_is_not_retried is the negative control (a 404 gets one attempt and no backoff).

Determinism. The sleeper and the jitter source are injected; no assertion reads the wall clock or math/rand. jitter_widens_the_window asserts the upper half of each window at jitter = 1 while get_succeeds_after_transient_5xx asserts the lower half at jitter = 0 — a constant backoff cannot satisfy both. internal/core is untouched, so purity_test.go is unaffected; task determinism is green.

Pre-existing test constructors (newServer, badClient, the conformance gitlab harness, the two cmd/assent factories) take a no-op sleeper, so the shipped retry budget still runs in every one of them at zero wall-clock cost. TestRetryDefaults pins the shipped policy so the defaults are not left unasserted behind the injection seam.

Mutation evidence

Mutation	Result
retryableMethod → true for all methods	RED all 5 TestWritesNeverRetried cases + TestWriteMethodsAreNotRetryable
defaultMaxAttempts 3 → 1	RED TestRetryDefaults + 3 TestIdempotentRetry cases
drop the jitter term (constant half-window)	RED jitter_widens_the_window
transient() stops treating 5xx/429 as retryable	RED 4 TestIdempotentRetry cases
delete the pre-attempt c.ctx.Err() check	RED expired_context_is_a_hard_error_with_no_request

---

AUD-S12 — ⚠️ REL-06: malformed BOT-marker skip-with-warning

Commit 31c1646. This is a reconcile-protocol behaviour change against ADR-0019, pre-logged as judgment call (d) in the spec — no new D-row needed, and I do not disagree with it at build time.

The behaviour change, stated plainly. Before: one bot note whose marker JSON had been corrupted made ListBotThreads/ListBotNotes return a hard error, so every later reconcile on that MR failed until a human deleted the note. That failed closed, but it bricked the MR. After: a bot-authored artifact with an undecodable marker payload is skipped — treated as not-a-slot-note — and reconcile proceeds. A wrongly-parsed marker still cannot approve anything, because ADR-0019 markers are correlation metadata and never decision input or authorization evidence.

The pre-existing TestListBotThreadsMalformedMarker, which asserted the old hard error, is updated to the new contract with a pointer to the deep coverage.

Author-identity filtering is untouched — and pinned. This is the part that must not weaken. The author check still runs before the marker is looked at, so a contributor note is invisible whether its marker is perfect or garbage, and it never reaches the warning channel (otherwise anyone could spam the receipt, and the warning would leak that the filter had reached the marker at all). TestSpoofedMarkerStillIgnored (conformance) asserts both contributor polarities — well-formed and malformed — against a bot-authored positive control with an identical corrupt body that DOES warn, so the contributor assertions cannot be vacuous. TestContributorMarkersAreInvisible mirrors this at the adapter level. Mutating the ordering reds both.

Where the warning goes (a receipt field nobody prints is a struct field, not a behaviour):

gitlab.Client keeps a deduplicated warning set — the step-9 rescan sees the same artifact more than once per run — exposed sorted via the new optional forge.Warner interface. forge.Reconcile copies it onto PublicationReceipt.warnings on the success path (omitempty, riding the schema's existing top-level additionalProperties: true, exactly like repairs — no schema change, no golden moves). cmd/assent's summarize() appends a suffix only when there are warnings, so the operator sees which artifact to repair and every existing summary line stays byte-identical.

Convergence — a spec deviation, reported rather than hidden. The spec says the duplicate slot post is "repaired by the existing duplicate-repair path (TestConformanceDuplicateRepair)". It is not — for either artifact kind. (My first write-up said this held for threads and failed only for summary notes; review finding F8 corrected that, and commit 2b239a3 fixes the text everywhere I had repeated it.)

Step 8 never fires: a corrupt artifact is filtered out of the listing, so it can never present as a visible duplicate for repair to act on, and PublicationReceipt.repairs stays empty. Convergence is real but comes from ordinary idempotent reuse: run 1 posts one healthy artifact for the slot, and every later run finds and reuses it — step 4's matching-occurrence no-op for a thread, step 3's edit-in-place for a summary note. The corrupt artifact lingers, warning, until an operator deletes it (write minimization — it is deliberately never auto-deleted).

Pinned by TestMalformedBotThreadConvergesWithoutDuplicateRepair (run1: discussionPosts=1 repairs=[] | run2: discussionPosts=1 repairs=[]) and TestMalformedBotMarkerDoubleRunConverges (run 1 → exactly 1 note POST, run 2 → 0 POSTs, receipts byte-identical). TestConformanceDuplicateRepair remains green and unchanged, which is what makes "repairs stays empty" a finding rather than an artefact of repair being unreachable in general.

Mutation evidence

Mutation	Result
restore the old return nil, err on parseMarker failure	RED TestListBotThreadsMalformedMarker, TestMalformedBotMarkerSkipsWithWarning, TestSpoofedMarkerStillIgnored/bot/malformed-marker-does-warn
move the whole parse+warn block above the author-identity check	RED TestSpoofedMarkerStillIgnored/contributor/malformed-marker + TestContributorMarkersAreInvisible/malformed
Reconcile drops the warnings (field never populated)	RED 3 tests across gitlab + conformance
remove the summary suffix (warning never reaches the operator)	RED TestSummarySurfacesReconcileWarnings

TestHealthyReconcileEmitsNoWarning is the positive control for the whole channel: a clean reconcile carries no warnings, so the field cannot be a constant and no golden receipt gains a spurious entry.

---

AUD-S10 × S11 interaction fix

Commit 15b7a33, found in review after the three stories landed.

The two guards interacted badly: readBounded returns an error, and transient() retried on any error, so an over-limit response was fetched three times before failing. Memory stayed bounded and the run still failed closed, but an oversized document is a deterministic failure like a 4xx — the same endpoint sends the same document again — and this lane's own 4xx_is_not_retried test encodes exactly that principle. The bound now wraps a sentinel (errBodyTooLarge) that transient() excludes.

Both polarities pinned: TestBoundedReadOverLimitIsNotRetried (exactly one attempt, zero backoff) and TestTransientFailuresAreStillRetriedAfterTheBound (a 5xx is still retried — so "nothing is transient" does not pass).

Mutation	Result
remove the errBodyTooLarge exclusion from transient()	RED TestBoundedReadOverLimitIsNotRetried
make all errors non-transient	RED TestIdempotentRetry/transport_error_is_retried_then_fails_hard

---

Verification notes

• parseMarker call sites: grep -rn "parseMarker" internal/forge/gitlab/ confirms exactly two production sites (ListBotThreads, ListBotNotes) — both changed. resolve.go has none, so REL-06 is fully closed, not half-closed.
• Warning-set lifetime: gitlab.Client.warnings accumulates for the life of the client and is never drained. That is correct here because there is no serve command — the CLI's only forge commands (run, doctor) build a client inside a per-invocation factory closure, one client per one-shot run, one MR. If a long-lived serve mode (ADR-0019 §3) is ever added and reuses one client across MRs, this set must become per-MR or drained per reconcile.

---

Gates

Gate	Result
task check (full, incl. changelog-verify)	green, exit 0
task determinism	green
coverage	90.3% (gate 90%, D-010)
golangci-lint run ./...	0 issues

Final commit 1d32fa3 is task changelog-write + CHANGELOG.md, as required. (Re-run after the interaction fix; task check exit 0 and task determinism exit 0 at that tip.)

One flake seen, not caused by this lane: internal/provider TestExecDigestPin/match_allows_exec and hack/spikes/provider intermittently fail with signal: killed when the whole suite runs under load — both spawn a compiled child binary under a hardcoded Timeout: time.Second. They pass in isolation (go test -race ./internal/provider/...) and on a clean tree, and this lane touches CallHTTP, not CallExec. Several other lanes were building concurrently on this machine. Worth a separate fix (raise or make the child timeout load-tolerant); flagging rather than silently retrying it away.

Spec conflicts reported

1. AUD-S12 convergence claim — the spec attributes convergence of the post-skip duplicate to TestConformanceDuplicateRepair. True for threads, not for summary notes (see above). Implemented and pinned via write-minimization convergence instead; nothing was skipped.
2. AUD-S10 loop count — the REQ names two page loops; a third of the same class existed and was capped too, with its own tests (see above).

---

Review round 2 — P2 fixes (F1, F2, F3) + P3 (F5)

Reviewer returned APPROVE with no P0/P1. All three P2s and the optional P3 are fixed on this branch. Each fix was mutation-verified the same way as the original lane: apply the mutation, git diff-confirm it landed, watch RED, restore.

F1 — warnings were dropped on every fail-closed refusal (f457c78)

Reconcile attached receipt.Warnings only at its single success return, so ErrArmingRefused / ErrIncompletePreconditions / ErrSHAMoved returned a bare PublicationReceipt{} and summarize printed no suffix. Those refusals are exit-0 advisory outcomes and unarmed is the default adopter posture — so an APPROVE run the forge will not arm skipped a corrupt marker completely silently. Every Reconcile return now goes through withWarnings; the receipt is otherwise untouched (no operations invented on a refusal).

Pinned at the real entry point — runRun → real gitlab.Client → forge.Reconcile → summarize → stdout:

• TestRunSurfacesForgeWarningOnUnarmedRefusal seeds a corrupt bot marker, asserts the run still exits 0 advisory with zero writes, and that the summary names disc-corrupt.
• TestRunEmitsNoWarningSuffixOnCleanRefusal is the positive control: the same refusal without corruption prints the same line as before.

Mutation	Result
revert refusal returns to bare PublicationReceipt{} (pre-fix behaviour)	RED — got "decision=APPROVE arm=true → advisory-only (arming precondition unmet, no approve/merge)"

F2 — the surviving provider mutant is dead (f4c0a5a)

The provider side had an under-limit control only. TestBoundedReadAtLimitStillSucceeds now serves a real, schema-valid FactResponse padded to exactly MaxResponseBytes with insignificant trailing whitespace — not a giant string value, which is schema-invalid and would have masked the read assertion behind a decode failure (my first two attempts did exactly that and failed honestly). It asserts the at-limit body both reads and resolves through ResolveFacts, so a bound that errored at the boundary is also caught as the auto-merge disarm it would cause.

Confirmed with the reviewer's own mutation, len(raw) > limit → >= at both sites simultaneously, diff-verified:

Package	Before this fix	After
internal/forge/gitlab	RED	RED
internal/provider	GREEN (mutant survived)	RED (mutant dead)

No production change — what shipped was already correct; this closes a test gap.

F3 — ADR-0019 + state table now record the change (c0b8165)

Convention used: a dated ## Amendment section appended to ADR-0019, matching the repo's established pattern (ADR-0003, 0004, 0007, 0008, 0009, 0010, 0011, 0012, 0014 all use it). ADR-0019 had no prior amendment, so it is unnumbered — matching every ADR's first amendment; Amendment 2/3 are only used for subsequent ones. The accepted Decision text is left untouched, which is the point of the convention.

The amendment states what changed in step 2 and the three properties that make this an amendment rather than a supersession: decision 1 (markers are correlation metadata only) unchanged; the author-identity filter unchanged and still first; and the worst case converges.

reconciliation-state-table.md gets the same content in two places, mirroring repairs exactly: inline in step 2, and a paragraph after the table. One deliberate judgment call to flag: repairs is explicitly not a table row there ("Pre-existing duplicates ... are not a sixth row of this table"), so warnings isn't either — a malformed marker is filtered during step 2's listing before any slot is classified, so its slot presents as row 1 and takes the ordinary create action. That is exactly why the worst case is a duplicate post rather than a wrong decision. Adding a literal row would have contradicted the mutual-exclusivity paragraph the table depends on, so I mirrored how repairs is handled rather than the literal word "row".

task docs-gates green (incl. the ADR-index status pin) and task docs-build (mkdocs --strict) exit 0.

F5 (optional P3) — retry-body safety made structural (bf72c9b)

Cheap and low-risk, so done. The retry budget now requires body == nil as well as an idempotent method, so a retryable request carrying a once-consumable io.Reader gets one attempt instead of a replayed empty request. Fails in the safe direction. TestRetryableRequestWithBodyIsNotReplayed pins both polarities (GET with body → 1 attempt; identical GET with nil body → full budget), so the guard cannot be mistaken for a disabled retry path.

Mutation	Result
drop the body == nil guard	RED — attempts = 3, want 1

Rebase

gh pr update-branch --rebase 26 returned RebaseConflictError (the generated CHANGELOG.md, which both this lane and AUD-S16/S17 rewrite). Resolved with a merge of origin/main (2748aad) rather than a local rebase — that keeps the no-force-push constraint, and main already uses merge commits. The conflict was CHANGELOG.md only; since it is fully generated, task changelog-write was re-run afterwards so the tree ends changelog-clean (7ba316a).

Gates at 7ba316a

Gate	Result
task check (full)	exit 0
task determinism	exit 0
task docs-gates / task docs-build --strict	green / exit 0
coverage	90.3% (gate 90%)
golangci-lint run ./...	0 issues

The TestExecDigestPin / hack/spikes/provider signal: killed flake appeared on two of four task check runs and passes in isolation under -race; per coordinator instruction it is owned by its own lane and untouched here.

---

Review round 3 — F8 (P2)

Reviewer returned APPROVE on the round-2 delta, with one new P2.

F8 — the ADR amendment and state table named the wrong convergence mechanism (2b239a3)

Both asserted that a skipped thread converges because "step 8's deterministic duplicate-repair resolves the duplicate on the next run". False, in a frozen normative contract. Step 8 never fires: a corrupt thread is filtered out of ListBotThreads, so it is never a visible duplicate for repair to act on. This is the same error I caught in the spec for the summary-note case and then wrote into the amendment for the thread case.

I verified it empirically rather than on trust — seeded a corrupt-only bot thread, ran Reconcile twice, and reproduced the reviewer's numbers exactly:

run1: discussionPosts=1 repairs=[] | run2: discussionPosts=1 repairs=[]

Now pinned by TestMalformedBotThreadConvergesWithoutDuplicateRepair, which asserts repairs stays empty on both runs and that run 2 reuses rather than re-posts. Non-vacuity: mutating the listing so healthy threads are also dropped reds the reuse assertion. That repairs can be non-empty stays pinned by TestConformanceDuplicateRepair, so "empty" here is a finding, not an artefact of repair being unreachable.

Corrected in all five places I had repeated the claim: ADR-0019 Amendment bullet 3, the state table's step-2 paragraph, the state table's post-table paragraph, the ListBotThreads skip comment, and two test doc-comments. Behaviour is unchanged — only the explanation was wrong.

Gates at ea7ade0 — completed run, no early abort

task check exit 0 on the first attempt, with all 13 stages confirmed executed (fmt, vet, lint, test, coverage, build, dogfood-comparison, compare-exitgate-test, changelog-verify, release-changelog-gate-test, release-verify-tag-gate-test, docs-gates, lint-depguard-test) — verified by grepping the stage banners, since an early flake abort would silently skip the later ones. task determinism exit 0 · task docs-build --strict exit 0 · coverage 90.3% · working tree clean, changelog included.

Not rebased, per instruction — merge with --merge.