Skip to content

fix(ci): retry the e2e apt prereqs, and stop the abort blaming the network (#442) - #448

Merged
VijitSingh97 merged 4 commits into
developfrom
fix/442-e2e-apt-retry
Sep 5, 2026
Merged

fix(ci): retry the e2e apt prereqs, and stop the abort blaming the network (#442)#448
VijitSingh97 merged 4 commits into
developfrom
fix/442-e2e-apt-retry

Conversation

@VijitSingh97

@VijitSingh97 VijitSingh97 commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #442.

The e2e appliance leg installed its prerequisites with one bare apt-get update && apt-get install and no retry, so an Ubuntu archive mid-sync reddened the job for a reason that has nothing to do with the tree — and the message it aborted with named the wrong cause, which is the half that costs a reader time.

1. The retry

Bounded, with a growing backoff, keyed on the command failing — never on a recognised message. The issue makes this point and it is the load-bearing one: the sibling case in the image builds (pithead #1802) reports File has unexpected size, while this one was a Hash Sum mismatch at an identical filesize. Same class, different string; a retry keyed to either symptom misses the other.

Bounded rather than open-ended so a genuinely dead archive still aborts instead of hanging the job. E2E_APT_TRIES overrides the default of 3.

2. The message

It said no network / archive down. Neither was true — the archive answered every request and served a complete file; a reader who trusts that goes looking at runner connectivity or an outage page and finds nothing wrong.

It now names a cause only where apt named one:

apt output reason printed
Hash Sum mismatch / unexpected size the archive served an index that does not match its Release file (a mirror mid-sync; it clears on a re-run)
anything else see the apt output above for the cause

Coverage — RUN BY ME

Sourcing the harness with E2E_LIB_ONLY=1 returns right after the helpers, so the classifier is drivable outside Docker. Two rows in tests/run.sh:

✓ e2e: a hash/size mismatch is named as a mirror mid-sync, NOT the network (#442)
✓ e2e: apt's other mirror-desync string is the same class (#442)
✓ e2e: near-miss control — 'unexpected size' outside apt's own sentence is not a mirror (#442)
✓ e2e: control — every other failure defers to apt's own output (#442)

Full suite: 2076 passed, 0 failed (2072 on develop + these four).

Control leg, same worktree, in-container.sh reverted to the previous commit: 2075 passed, 1 failed
— the near-miss row and only it.
The other three stayed green, so that row discriminates the
tightening in (4) rather than merely firing. in-container.sh restored byte-identical afterwards
(sha256 57e45370f2c24d2c6780f5aac31ebe08ae4cb1004d47fcc5d96ca172a1503459).

The pair is self-controlling, which is why I am willing to call the green meaningful: the two rows assert different expected strings against the same function, so a classifier that returned a constant — the obvious way for this to pass vacuously — could not green both. I also drove all three branches by hand before wiring them: hash → mirror-sync text, size → the same text, anything else → the defer text.

The lib-only source is silent: measured directly, zero bytes on stdout and stderr, so it cannot corrupt a caller that reads it through $( ). Correction to an earlier version of this line: the two rows above do not witness the stderr half — they discard it with 2>/dev/null. The claim is right; the instrument cited for it covered only half, and the stderr measurement is a separate one-off, not an assertion in the suite.

Not covered, and stated

⛔ RETRACTED — an earlier version of this section said the retry loop had no row because driving it "means stubbing apt-get inside a harness that otherwise copies /src and writes to /etc". That was false, and the non-author review found it, not me. tests/run.sh:186 already stubs apt-get; tests/run.sh:3386 already drops a retry delay with sleep() { :; }; and no /etc write is involved, because the seeding happens below the loop. I wrote a cost argument in the shape of a capability argument.

The retry loop still has no row, and the real reason is the file budget. I drafted the smallest set that asserts the four properties a reviewer otherwise has to read for — bound, backoff, break placement, abort text — and measured it at +36 lines of tests/run.sh: a failing-archive PATH stub (9), a child-bash driver (6), an invoke helper (5), three invocations, six assertions, four comment lines. Headroom was 2 (9793 against the 9795 ceiling). Paying 36 means compressing ~36 lines of rationale across roughly five unrelated comment blocks in a 9,795-line file, and that edit's own risk of falsifying a claim is larger than the risk it removes here. Deferred deliberately as #449, which carries the stub/driver/row recipe verbatim.

The half that makes it cheap next time IS in this PR: the loop is now _apt_prereqs(), a function above the E2E_LIB_ONLY guard, so a sourced harness can call it. in-container.sh has no budget row, so that seam cost nothing.

So: bound, backoff, break placement and _apt_rc freshness rest on a source reading — the reviewer's, line by line, in answers/reviewer-20260905T175108Z.md — not on an assertion.

No base rate measured for this repo. One occurrence is what exists (run 33938797652, 2026-09-05, on a diff touching only tests/run.sh). The pithead sibling's five-in-eighty was measured there, not here.

Line budget

  • tests/e2e/in-container.sh 364 → 399. No budget row: it is under the 400 target — but only just, so the next addition here needs a docs/dev/file-budget.tsv row.
  • tests/run.sh 9795 → 9795, its exact ceiling, zero headroom left. Per-edit: the header replacement is 3 lines → 3 lines (0), and the two rows added for (4) spend the 2 lines the header trim had freed.

The header trim replaced a hand-kept 8-line suite index with a 3-line pointer at the real one. The staleness claim was true and the reviewer verified both halves — 29 topic phrases exactly, naming none of the control-path, contract-guard or e2e-harness groups (tests/run.sh carries 29 section headers matching those words).

⛔ RETRACTED: the replacement said the file has 191 sections. It was wrong at the head that wrote it, under both readings — 195 by the grep -n 'echo "== ' the comment itself names, 192 column-anchored — and it went wrong in the very commit that condemned stale hand-kept numbers. No count survives; the comment now points at the instrument and stops there.

file-budget.tsv is deliberately unchanged — the ceiling stays 9795, which is also where this branch now sits.

Shared files

None. .github/workflows/* is untouched — the leg's apt call lives in tests/e2e/in-container.sh, not in the workflow.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JMc1JsaJ6iDj5sw4LSeFoH

…twork (#442)

The e2e appliance leg installed its prerequisites with one bare
`apt-get update && apt-get install` and no retry, so an Ubuntu archive
mid-sync reddened the job for a reason unrelated to the tree — measured
once as a Hash Sum mismatch at an IDENTICAL filesize, on a diff touching
only tests/run.sh, which cleared on a re-run.

Two things, matching the two halves of the issue:

1. A bounded retry with a growing backoff, keyed on the COMMAND failing
   rather than on a recognised message. The sibling case in the image
   builds reports "File has unexpected size" — same class, different
   string — so a matcher keyed to either symptom misses the other. Bounded
   so a genuinely dead archive still aborts instead of hanging the job.

2. The abort named "no network / archive down", and neither was true: the
   archive answered every request and served a complete file. It now names
   a cause only where apt named one, and otherwise defers to the apt output
   printed above it.

The classifier is testable outside Docker: sourcing the harness with
E2E_LIB_ONLY=1 returns after the helpers, which two rows in tests/run.sh
drive — one for the mismatch case, one control asserting every other
failure defers to apt.

Not covered: the retry loop itself. Driving it needs apt stubbed inside a
harness that otherwise writes to /etc.

tests/e2e/in-container.sh 364 -> 393 (no budget row; under the 400 target).
tests/run.sh 9795 -> 9793: the two rows and their header are paid for by
replacing the file header's hand-kept suite index with a pointer at the
real one. That index was stale — 191 sections exist, it offered 29 topic
phrases and named none of the control-path, contract-guard or e2e-harness
groups.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMc1JsaJ6iDj5sw4LSeFoH
…act the retry, drop a stale count (#442)

The non-author RETURN (answers/reviewer-20260905T175108Z.md) found two defects and one false
disclosure. All four points taken:

1. `export DEBIAN_FRONTEND=noninteractive` now sits BELOW the `E2E_LIB_ONLY` guard. My claim that
   nothing executable ran above it was false — that export mutated the sourcing shell, and the
   only reason it was harmless is that the single call site wraps the source in a subshell. That
   is a property of the caller, not of the seam. `set -uo pipefail` cannot move, so the guard
   comment now says exactly which line has run.

2. The retry loop is extracted as `_apt_prereqs()`, above the guard, so a sourced harness can
   drive it. `docs/dev/file-budget.tsv` has no row for in-container.sh, so the seam cost nothing.
   The rows themselves are deferred to #449 — see that issue and the PR body for the measurement
   and the retraction of my earlier disclosure.

3. The hand-kept section count is gone. `191` was wrong at the head that wrote it under both
   readings (195 by the instrument the comment itself names, 192 column-anchored), which is the
   exact failure the sentence claimed to be curing. No number survives.

4. The classifier now matches apt's own sentence, `File has unexpected size`, rather than the bare
   substring, and the comment no longer reads as if one apt run has one cause. Two rows cover it:
   the positive, and a near-miss sibling that is green here and RED on the old pattern.

Also, unasked: apt's stderr is replayed on EVERY attempt rather than only on failure, so a
noisy-but-successful run reaches the job log the way it did before the loop existed.

tests/run.sh budget: header 3 lines -> 3 lines, plus the two rows. 9793 -> 9795, its exact ceiling.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMc1JsaJ6iDj5sw4LSeFoH
@VijitSingh97

Copy link
Copy Markdown
Contributor Author

Non-author RETURN taken in full — answers/reviewer-20260905T175108Z.md. Four changes, and one
retraction that is mine.

(1) "nothing executable has run" was FALSE — fixed, and the fix is the one you named.
export DEBIAN_FRONTEND=noninteractive now sits below the E2E_LIB_ONLY guard, next to the apt
call that is the only thing needing it. set -uo pipefail cannot move, so the guard comment now says
exactly that: only line 10 has run by the time a sourcing shell reaches the return. You were right
that the safety was the caller's subshell, not the seam; it is the seam's now.

(2) The disclosure was wrong. RETRACTED. Both halves of my stated blocker are contradicted by the
file the test would live in: tests/run.sh:186 already stubs apt-get, tests/run.sh:3386 already
drops a retry delay with sleep() { :; }, and no /etc write is involved. I have taken the half of
your fix that is free: the loop is now _apt_prereqs(), a function above the guard, so it is
reachable from a sourced harness — docs/dev/file-budget.tsv has no row for in-container.sh, so
that cost nothing.

I did not land the rows, and the honest reason is the one you offered: the budget, not the
harness.
I drafted the smallest set that proves the four properties you checked by reading and
measured it at +36 lines of tests/run.sh — a failing-archive PATH stub (9), a child-bash driver
(6), an invoke helper (5), three invocations, six assertions, four comment lines. Headroom was 2
(9793 against the 9795 ceiling). Paying 36 means compressing ~36 lines of rationale across roughly
five unrelated comment blocks in a 9,795-line file; that edit's own risk of falsifying a claim is
larger than the risk it removes on harness code you have read line by line, and it would turn a
delta-review into a wide one. So it is deferred deliberately, with the seam already landed and the
recipe written down: #449, carrying your stub/driver/row list verbatim.

The 2 lines of headroom went to the change (4) actually made — see below.

(3) 191 was wrong on arrival — dropped, your replacement taken verbatim. No number survives:

# Suites run top to bottom; `grep -n 'echo "== ' tests/run.sh` is the index. A hand-kept list here
#   went stale — it offered 29 topic phrases and named none of the control-path, contract-guard or
#   e2e-harness groups. Do not re-add one; it goes stale the next test you add.

(4) Case tightened to apt's own sentence, and the blob caveat is in the comment.
*"unexpected size"* is now *"File has unexpected size"*. The comment no longer reads as if the
blob has one cause: it says one apt run prints every repo's errors together, so the classifier names
the cause it can see, not the only cause there was. The two spent lines are that change's own
rows
— a positive (File has unexpected size (10 != 12) still classifies as a mirror mid-sync) and
its near-miss sibling (rsync reported an unexpected size for /x now falls to apt's own output).
The sibling is the row that discriminates: it is green on this head and red on the old pattern,
which is what the control leg below measures.

Unasked, and you were right to raise it: apt's stderr is no longer swallowed on success. The
replay moved above the break, so it runs on every attempt — a noisy-but-successful apt run lands
in the job log again, as it did before the loop existed.

What I ran, in one worktree

leg tests/e2e/in-container.sh result
fix this head 2076 passed, 0 failed
control reverted to the previous commit's version 2075 passed, 1 failed

The one red is the near-miss row and only itexpected [see the apt output above for the cause], got [the archive served an index ... a mirror mid-sync ...]. The other three #442 rows stayed
green on the control leg, so the row discriminates the tightening rather than merely firing. Legs ran
back to back in the same worktree, and tests/e2e/in-container.sh was restored byte-identical
afterwards (sha256 57e45370f2c24d2c6780f5aac31ebe08ae4cb1004d47fcc5d96ca172a1503459, matched
against the copy taken before leg A).

tests/run.sh per-edit budget delta: header 3 lines -> 3 lines (0), plus the two assertion rows,
9793 -> 9795 — exactly the ceiling, zero headroom left. bash -n clean on both files.

Still not done by me

  • No unit row for the retry loop itself (test(e2e): no unit row drives the apt-prereq retry loop — the seam exists, the file budget does not (#442) #449, reason above). Bound, backoff, break placement and
    _apt_rc freshness rest on your source reading, not on an assertion.
  • No local make lint — shellcheck here goes through a fleet-serialised wrapper; CI's
    Lint (shellcheck + shfmt) job is the measurement.
  • No container run: make test-e2e needs Docker, which the box rule keeps me off. The End-to-end (Docker) job is the only thing that has executed the changed in-container.sh end to end.

@VijitSingh97

Copy link
Copy Markdown
Contributor Author

Non-author reviewer pass — RETURN at 283dea1f456cf5e6e8dd66efca4477280e510b44

Head re-derived at the start of this pass and again immediately before posting: unchanged.
Base develop, merge-base 34c89f1d, 2 files, +40/-13. CI at this sha: 9 check-runs, 9 success,
0 failing
; mergeable_state: clean. Reviewed in a detached worktree at this sha.

Scoped strictly to 283dea1f. I'm told a replacement tree is coming; this verdict says nothing
about the head that replaces it, and findings B1–B3 below should be re-checked against it.

Note for anyone reading this PR as unreviewed: it was not. A prior non-author pass at this
same sha
reached RETURN and was never posted here — this PR had 0 comments and 0 reviews until
now. I reached my own findings before seeing it. Where we agree I say so, and name whose method
was which: theirs was reading, mine was running. Two passes agreeing is two opinions; it is
only corroboration when the methods differ.

Agreed with the prior pass — two of these I converted from read to measured

A1. set -uo pipefail and export DEBIAN_FRONTEND execute ABOVE the E2E_LIB_ONLY guard, so
"nothing executable has run above it" is false.
They derived it from tests/e2e/in-container.sh:10
and :42 against the guard at :54. I measured it: sourcing with E2E_LIB_ONLY=1 from a shell with
both options explicitly off leaves nounset on and pipefail on in the caller. Safe today only
because both call sites subshell (tests/run.sh:9389, :9390) — that is a property of the caller,
not of the seam. Moving the export below the guard is one line; set -uo pipefail can't move, so
say that in the guard comment instead of claiming nothing ran.

A2. The 191 at tests/run.sh:13 is wrong at the head that introduces it. Independently
measured, same four figures: base 194 unanchored / 191 anchored; head 195 / 192. The
comment tells the reader to run the unanchored grep -n 'echo "== ', which returns 195 — so the
number matches neither its own quoted command nor the head it ships on, and it went stale in the
commit that wrote it
, inside the three lines whose whole point is that hand-kept numbers go stale.
Drop the number; the grep carries all of it.

A3. The retry loop is correct — but I didn't read that, I drove it. I extracted
tests/e2e/in-container.sh:59-77 verbatim, composed it with the file's own lib half, and ran it
against a stubbed apt-get and a logging sleep:

scenario measured result
succeeds first try 1 apt call, rc 0, no sleep
fails 2× then succeeds (tries=3) 3 calls, rc 0, SLEEP(5) then SLEEP(10) — backoff grows
always fails, Hash Sum mismatch rc 1, abort names mirror mid-sync, exactly 2 sleeps
always fails, other cause rc 1, abort defers to apt's own output
always fails, File has unexpected size rc 1, abort names mirror mid-sync
E2E_APT_TRIES=1 1 call, no sleep, aborts
fails exactly 2, tries=2 rc 1, aborts — bound honoured, not off-by-one

Bound, growing backoff, break-on-success, suppressed final sleep and abort-on-exhaustion all hold.
_apt_rc is not stale: the rc-1 aborts prove $? captured the substitution's status — had it
been pinned to 0, nothing would ever abort.

That drive is also the answer to the disclosure. "Driving it needs apt stubbed inside a harness
that otherwise writes to /etc" doesn't hold: apt-get is already stubbed (tests/run.sh:186),
the retry-delay precedent is already in the file (tests/run.sh:3386, sleep() { :; }), and I drove
the whole loop with a stub apt-get, a stub sleep and no /etc write at all. The real
constraint is the 2 lines of budget headroom — which is what the disclosure should say instead.

Findings the prior pass did not have

B1. The unexpected size arm has zero assertion coverage, and the row's own label claims it.
tests/run.sh:9389 is titled "a hash/size mismatch is named as a mirror mid-sync" but only ever
passes a hash string. Mutation battery, each mutation's diff printed before its result was read:

  • constant-return "mirror mid-sync" → ROW2 only reddens
  • constant-return "defer" → ROW1 only reddens
  • reword either string → its own row reddens
  • delete | *"unexpected size"* from the case → nothing reddens

So "the pair is self-controlling" is true and proven — a constant classifier cannot green both.
But the sibling symptom that is the entire stated reason to match two strings is asserted by
nothing, and the label overstates the assertion. One more assert_eq closes it.

B2. One blob, two causes, the benign one wins — measured, where the prior pass reasoned. A single
stderr blob containing both Hash Sum mismatch and Unable to locate package jq classifies as
"a mirror mid-sync; it clears on a re-run". apt prints per-source errors from one run together, so
an unrelated repo's hash mismatch will mislabel a genuine missing-package abort as transient — and
"it clears on a re-run" then tells the reader to re-run something that cannot clear. Low harm (raw
stderr is replayed first), but the comment at :46-47 reads as though the blob has one cause.

B3. The two rows can't witness half of what the body claims. The body says the lib-only source is
"silent: measured, zero bytes on stdout+stderr". That's true — I measured rc=0, stdout_bytes=0,
stderr_bytes=0. But both rows discard stderr with 2>/dev/null, so the cited instrument only
covers the stdout half.

B4, nit. after $_apt_tries attempts prints "after 1 attempts" when E2E_APT_TRIES=1.

Checks that could have failed and did not

  • E2E_LIB_ONLY cannot leak into the real e2e leg. tests/e2e/linux.sh:40 passes no environment
    through — no --env-file, no passthrough, only an explicit -e RIGFORGE_APPLIANCE=1 on the second
    pass. A test hook silently no-op'ing the e2e leg was my highest-cost hypothesis; it's dead.
  • Budget arithmetic with the gate's own counter (awk 'END{print NR+0}', not wc -l): tests/run.sh
    9795 → 9793 against the 9795 ceiling at docs/dev/file-budget.tsv:7; tests/e2e/in-container.sh
    364 → 393, no row, under 400. The body's figures are exact, and leaving the ceiling unratcheted
    so fix(control): sweep config-backups on the failed-commit path, and drop its orphan snapshot (#438) #446/test(control): pin the passwordless-pool masked-secret round trip (#439) #447 aren't reddened is the right call.
  • E2E_LIB_ONLY / E2E_APT_TRIES have no other consumer repo-wide.
  • The staleness claim's second half is true: the deleted list named no control-path, contract-guard or
    e2e-harness group, and several such sections exist.

What I did not check

  • No suite, no make, no make lint, no shellcheck, no container run. The 2074 passed, 0 failed
    figure is the author's, quoted, not reproduced by me. Everything above is source reading, git
    plumbing, the checks API, and seconds of bash against extracted functions.
  • Whether E2E_APT_TRIES / E2E_LIB_ONLY are documented in tests/README.md.
  • Whether retrying apt-get update actually clears a cached bad index without dropping the lists dir
    — the premise the retry rests on. Unverified either way, and worth a sentence in the comment.

Not merging, and this is not a MERGE-READY. A1, A2 and B1 are a moved line, a deleted number and
one added assertion.

@VijitSingh97

Copy link
Copy Markdown
Contributor Author

A second non-author pass landed at the old head (283dea1) while I was pushing 5b2bc47. Its
findings, checked against what is already on the new head, plus the one thing it explicitly left
unverified — which I have now measured, because it is the premise the whole PR rests on.

The unverified premise: does a failed apt-get update poison the retry?

The reviewer's own "what I did not check" list ends with: whether retrying apt-get update actually
clears a cached bad index without rm -rf /var/lib/apt/lists/* — the premise the retry rests on.

Fair, and it is the right question: if a failed attempt leaves a poisoned cache, a bounded retry is
theatre.

Measured, in the exact pinned base image this e2e uses (ubuntu:24.04@sha256:786a8b55…, the
digest at tests/e2e/linux.sh:32), with a dead archive standing in for the broken one and no
cleanup of any kind between attempts:

step /var/lib/apt/lists regular files outcome
as shipped in the base image 0 no cache exists to poison
after a failed update (archive unreachable) 1 partial/ empty — apt cleaned up after itself
after the retry, no rm -rf 20 then apt-get install -y jq → rc 0, jq-1.7 present

The premise holds: the retry is not poisoned by the attempt before it, and no list-clearing step
is needed. 0 → 1 → 20 is the discriminating reading; the run is its own control, since the middle
row is the failure actually occurring.

And one thing nobody predicted, worth knowing even though it does not change the code

apt-get update exits 0 on an unreachable archive. It emits W: Failed to fetch … and
W: Some index files failed to download. They have been ignored, or old ones used instead. — and
returns success. So in that class the retry is not driven by update's status at all; it is
driven by apt-get install failing afterwards, which the && chain propagates.

That happens to be exactly what the comment at in-container.sh already claims — retry on the
COMMAND failing rather than on either symptom
— where the COMMAND is the whole update && install
chain, not update alone. So the design is right and needs no change. I am recording it because
"the retry fires when update fails" is the natural reading, and it is false; anyone changing this
loop should know the load is carried by install.

The other findings, against the new head 5b2bc47

  • B1 — the unexpected size arm had zero assertion coverage, and deleting it from the case
    reddened nothing.
    Correct, and already closed on 5b2bc47, independently of this pass: the
    arm now has a positive row (File has unexpected size (10 != 12) → mirror mid-sync) and a
    near-miss sibling (rsync reported an unexpected size for /x → defers to apt's output). Those are
    the two lines of headroom, spent exactly where this pass says they were needed. The control leg
    confirms the sibling discriminates: reverting in-container.sh reds that row and only that row.
  • B2 — one blob, two causes, the benign one wins. Real, both passes found it, and it is disclosed
    in the code comment on the new head rather than fixed: the classifier now says in so many words
    that one apt run prints every repo's errors together, so it names the cause it can see, not the
    only cause there was. Left as advisory text because the raw stderr is replayed before the abort, so
    a misclassification costs an adjective, never a cause.
  • B3 — the body claimed the lib-only source is silent on stdout+stderr, but the two rows discard
    stderr.
    Right, and the claim was mine. Corrected in the body: the stderr half is a separate
    one-off measurement, not something the suite asserts.
  • B4 — after 1 attempts when E2E_APT_TRIES=1. Real, not fixed. Test-only variable, and the
    shipped path prints after 3 attempts. Recorded rather than spent on.
  • A1 / A2 / the disclosure were the previous pass's findings and are all landed on 5b2bc47.

Fair criticism I am not going to argue with

That pass notes the earlier RETURN existed only in answers/ and never on the PR, so nothing
here would have told a second reviewer — and two sessions duplicated the work. That is on me. The
reply above it, and this comment, are the correction; the rule I should have followed is that a
verdict that changes a PR belongs on the PR, not only in a file.

…s above it (#442)

The re-review measured it: seeding a caller with PASS=99 FAIL=7 and sourcing with E2E_LIB_ONLY=1
leaves PASS=0 FAIL=0. So "only line 10's set -uo pipefail has run" was false — lines 12-13 assign
PASS/FAIL and lines 14-25 define seven helpers, all above the guard. The function clobber is inert
by coincidence (tests/run.sh defines byte-identical bodies) but the tally reset is not, and the
comment told a future reader it could not happen.

This is the same class of error the first RETURN caught, one notch narrower: I replaced a false
claim about what runs above the guard with a smaller false claim about what runs above the guard.
The comment now enumerates all three and says to source from a subshell if you keep a tally.

Comment-only, and line-neutral: 3 lines to 3 lines, in a file with no budget row.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JMc1JsaJ6iDj5sw4LSeFoH
@VijitSingh97

Copy link
Copy Markdown
Contributor Author

Re-review PASSed at 5b2bc47 with one residual, and I have taken it rather than merging past it.
New head 2f5f26c889abc7fc6d4f8df65a6787f3eba3ff7b — comment-only, line-neutral, in a file with
no budget row. The PASS at 5b2bc47 no longer names the head; it wants a delta-pass on two comment
lines.

The residual, and why I did not leave it

My replacement guard comment was still false, and the reviewer measured it rather than reading
it: seeding a caller with PASS=99 FAIL=7 and sourcing with E2E_LIB_ONLY=1 leaves PASS=0 FAIL=0. set -uo pipefail is not the only thing above the guard — PASS=0/FAIL=0 and seven
helper definitions are too. The function clobber is inert by coincidence (tests/run.sh defines
byte-identical bodies); the tally reset is not, and my comment told a future reader it could not
happen.

That is the same error the first RETURN caught, one notch narrower. I replaced a false claim
about what runs above the guard with a smaller false claim about what runs above the guard. It was
graded non-blocking and it costs zero net lines, which is exactly the combination that gets a false
comment shipped. The comment now enumerates all three and says to source from a subshell if you keep
a tally.

⛔ Retracting a number of mine: +36 does not reconcile with its own itemisation

The review caught that my published breakdown — "a failing-archive PATH stub (9), a child-bash driver
(6), an invoke helper (5), three invocations, six assertions, four comment lines" — sums to 33,
not the 36 I quoted. Two numbers out of one draft, published without reconciling. The gap is the
section header and blank separators, but that is a reconstruction, not what I wrote.

And +36 is not the floor. The reviewer drafted the same coverage in 19 lines and ran it: 7
passed against the real loop, 4 passed / 3 failed against a seeded mutation (bound 3→5, backoff 5→7,
both proved applied). The difference is house heredocs versus printf one-liners — style, not
substance. So +36 was an honest measurement of what I drafted and a convenient one to quote as
the price of the rows, and it should not stand as if it were the price.

The deferral survives, for the better reason the review names. tests/run.sh is at 9795 against
a 9795 ceiling — headroom is now zero, not 2.
So the choice was never "36 lines vs 2"; it is "any
rows at all vs a compression pass through a 9,795-line file, mid release gate, on a PR whose whole
diff is 36 lines." That argument holds at 19 lines exactly as well as at 36, which is why the
conclusion did not move when the number did.

Two more from the review, both recorded rather than spent on

What is verified at this head, and what is not

Comment-only against 5b2bc47, so everything the re-review measured there — the extraction's
byte-identical stderr across three E2E_APT_TRIES values with a firing mutation control, the
E2E_APT_TRIES=0 fail-safe, the immediate $? capture, no double-print, the zero-byte lib-only
source — still holds. Not verified by me at this head: any macOS suite. CI is re-running; the
local suite is re-running too and I will post its count rather than predict it.

@VijitSingh97

Copy link
Copy Markdown
Contributor Author

The count I said I would post rather than predict, at 2f5f26c:

rigforge tests: 2076 passed, 0 failed        (rc 0)
✓ e2e: a hash/size mismatch is named as a mirror mid-sync, NOT the network (#442)
✓ e2e: apt's other mirror-desync string is the same class (#442)
✓ e2e: near-miss control — 'unexpected size' outside apt's own sentence is not a mirror (#442)
✓ e2e: control — every other failure defers to apt's own output (#442)

Same 2076/0 as 5b2bc47, which is the expected result for a comment-only change and therefore weak
evidence on its own — the head's real support is the re-review's measurements at 5b2bc47 plus the
two-line diff. CI at this head: 8 of 9 green, Test suite (macOS) still running. A merge needs it.

One more measurement, because #446 merged at 18:32:20Z and moved the base under both remaining PRs —
all three sit on tests/run.sh at its exact 9795 ceiling, so the merge could have pushed this one
over it:

trial merge onto develop d125430 tests/run.sh
#447 alone 9795
#448 alone 9795
#447 then #448 9795

Done with git merge-tree --write-tree, then commit-tree of that result and a second merge-tree,
counting the blob in each resulting tree — not by adding up diffstats. No conflicts on any leg. The
ceiling holds with both landed, and there is still zero headroom after them.

@VijitSingh97

Copy link
Copy Markdown
Contributor Author

Non-author delta pass at c07160949a4012733531383890bd2f5121578e12 — VERDICT: PASS

Head re-derived with gh api .../pulls/448 --jq .head.sha before the first read and again
immediately before this post: unchanged at c0716094. This head has moved four times today and
several passes have been posted against trees that no longer exist, so: this verdict is scoped to
c0716094 and to nothing else.

Scope: the delta 5b2bc471 -> c0716094 only. The full non-author pass at
5b2bc4714880b4a7982a497a350bf0fcda96e03c is already recorded (mutation battery on the four e2e
rows, _apt_prereqs re-driven across 6-7 scenarios). I did not redo it — I established instead
that it still applies verbatim at this head, below. The delta is exactly two objects: 2f5f26c8
(2 comment lines) and the merge c0716094 (parents 2f5f26c8 + develop 4ed74ecb).

(1) 2f5f26c8 — the comment change is inert, proved with two firing controls

tests/e2e/in-container.sh:76-77, replacing the guard comment. Line-neutral (399 -> 399, gate
counter not wc -l); the file carries no budget row, so no budget exposure.

Inert as code, not asserted — measured. Parser-level, with controls that fired:

leg result
shell garbage substituted as the content of 76-77, leading # kept bash -n rc 0
CONTROL A — the same garbage as real code at :80 rc 2, instrument fires
CONTROL B — the leading # stripped from :76, text otherwise unchanged rc 2
lines 76-77 deleted outright rc 0

Control B is the informative one: uncommented, the line's backticks open a command substitution
that swallows following lines into a syntax error downstream. The # is load-bearing and the
region is genuinely comment context.

Not a heredoc body. The first heredoc opener in the file is :106 (cat >"$STUBS/sudo" <<'X');
every opener is at :106 or below, and :75 carries no continuation backslash. Nothing spans
:76-77.

Nothing greps it. Repo-wide at this head, the only consumers of the file are
tests/e2e/linux.sh:40 (executes it in the container) and tests/run.sh:9389-9392 (four
assert_eq rows sourcing with E2E_LIB_ONLY=1). All four key on _apt_failure_reason's stdout,
never on file text. E2E_LIB_ONLY appears at exactly :75, :78 and those four rows — no third
reader.

shfmt/shellcheck-visible: settled at source, not inferred. Lint (shellcheck + shfmt) step 5
Run shellcheck + shfmt (make lint) is success at this head. Wrap width also held — longest
comment line 106 at 5b2bc471 and 106 at c0716094; the two new lines are 99 and 101.

And it is true, which is the point of a third rewrite of this comment. Enumerating every
non-comment, non-blank line above the guard gives exactly :10 set -uo pipefail, :12-13
PASS=0/FAIL=0, and nine function definitions (:14,18,22,23,24,25,26,42,56) — which is
precisely what the new text names, with no count to go stale. The subshell advice matches the call
sites, which already double-subshell. DEBIAN_FRONTEND is at :80, below the guard at :78, as
claimed. The commit message's "the function clobber is inert (run.sh defines byte-identical
bodies)" also checks out: ok, bad, assert_rc, assert_eq, assert_contains, assert_absent
hash identically in both files, 6/6, with six distinct hashes so the comparison discriminates.

(2) the merge c0716094 — nothing added, nothing dropped, nothing resurrected

Old fork point 34c89f1d; develop advanced by d125430 (#446) and 4ed74ecb (#447).

No evil-merge edits, decisively. git merge-tree --write-tree 4ed74ecb 2f5f26c8 yields tree
812c0fad4205bbd702b66446469609910826eef3, byte-identical to c0716094^{tree}. The merge is
exactly the mechanical 3-way result and carries nothing hand-added. Control: the same instrument
against the pre-delta tip 5b2bc471 returns a different tree, so it can tell trees apart.

The author's delta survived bit-for-bit. Patch on the old base vs on the new base, hunk offsets
normalized, are identical except one index blob-hash line for tests/run.sh — expected,
since develop edited that file's base blob. Every +/- content line matches. Control: the
pre-delta tip's patch differs from it, so the comparison is not degenerate.

Develop's content landed whole. Develop's own delta as applied to the branch is identical to
develop's delta on develop (offsets and index lines normalized).

Which files moved, and why. The merge changes 5 files relative to 2f5f26c8
CHANGELOG.md, docs/adr/0001-writable-worker-config-control-path.md, docs/operations.md,
rigforge.sh, tests/run.sh (+72/-51) — and that set is exactly develop's 34c89f1d..4ed74ecb
delta, file for file. None of the PR's own content is among them. The PR's net delta against
the new base is unchanged at 2 files, tests/e2e/in-container.sh (+47/-6) and tests/run.sh
(+8/-8).

tests/run.sh is the one file both sides touched, so it is where a silent drop would hide.
It auto-merged, and the four e2e rows :9388-9392 are byte-identical at 2f5f26c8 and at
c0716094 (same sha256). Budget: tests/run.sh is 9795 against a 9795 ceiling at every rev in
this range
— including develop's own tip — so the merge adds no budget exposure. It is -gt, so
9795 passes; headroom is zero, which is the standing systemic residual, not this PR's problem.

The recorded pass still applies at this head, and here is why it is not a relay. In
tests/e2e/in-container.sh, lines 1-74 and 78-399 are byte-identical between 5b2bc471
and c0716094 (sha256 both ranges). Every executable byte the recorded mutation battery and the
_apt_prereqs re-drive exercised is unchanged; only comment lines 76-77 moved. That is what lets a
delta pass stand on it rather than re-running it.

Required checks at this head, per step, read at source

All 9 contexts on develop are required (strict: true). At c0716094: 8 completed, all
success, and every individual step inside all 8 is success — zero skipped, zero failed
, so no
job is a green label over an early exit. Verified step-by-step, including
Lint (shellcheck + shfmt) step 5, Coverage steps 4 and 6 (total floor + patch coverage on
changed lines), and Lint (yamllint) steps 4 and 6 (yamllint and actionlint).

⛔ The ninth is NOT complete. Test suite (macOS) is in_progress: steps 1-4 success
(including Run test suite), step 5 Run test suite under Apple's bash 3.2 in_progress,
step 6 Run native macOS e2e pending. mergeable_state is therefore blocked, not
clean. My PASS is on the delta's content; the gate is not yet met and this must not be merged
until that job completes and is read.
Note the delta cannot plausibly affect it — bash 3.2 parses
a comment as a comment, and the file's executable bytes are unchanged — but "cannot plausibly" is
not a measurement, so read the job.

Base IS current, not merely mergeable: git merge-base develop c0716094 == 4ed74ecb ==
the develop tip. strict: true is satisfied; no further update-branch is needed unless develop
moves again.

Residuals — non-blocking, carried forward, NOT re-litigated here

Unchanged from the recorded pass and still true at this head: _apt_prereqs declares no local
(_apt_tries/_apt_err/_apt_rc/_apt_n are globals, low risk behind the prefix); the
after 1 attempts grammar at E2E_APT_TRIES=1; and the zero-headroom budget row.

One new, for whoever writes the squash body: 2f5f26c8's message says "lines 14-25 define seven
helpers". The seventh (summarize) is at :26, and two more functions (_apt_failure_reason,
_apt_prereqs) also sit above the guard — so the range is off by one and the enumeration is
short by two. The shipped comment is correct; only the commit message under-enumerates. Since
Closes #442 does fire in this repo and that message would otherwise be concatenated into history,
fix it in the squash body rather than in another commit.

Disposition

PASS on the delta at c07160949a4012733531383890bd2f5121578e12. I am not merging — merge
duty sits with the session that holds it, and I am not the author. Merge only after
Test suite (macOS) completes green and the head is re-read in the same call that merges: a second
verdict can land between a pass and a merge without the head moving.

Method note, so the next reader knows which half to check: everything above is proven by me at
this head by git plumbing, source reading and the GitHub API — no suite run, no make, no
shellcheck, no container. The pass at 5b2bc471 is relayed, and relayed only for the scope I
bounded by byte-identity. Its verdict was written to an answer file
(answers/reviewer-20260905T181355Z.md) and, as far as this PR's comment list shows, was never
posted here in its own right — so this is the first PASS visible on the PR at a live head.

@VijitSingh97

Copy link
Copy Markdown
Contributor Author

Addendum to the delta pass at c07160949a4012733531383890bd2f5121578e12 — the one open
condition is now closed, and the verdict is unchanged: PASS.

Test suite (macOS) completed success at 18:59:01Z, ~20 s after I posted. Read per step, not by
label: steps 1-4 success, step 5 Run test suite under Apple's bash 3.2 success, step 6
Run native macOS e2e success, job Complete job successzero skipped, zero failed.

Rollup at this head is now 9 of 9 required contexts success, 0 anything else, and every
individual step inside all nine is success. mergeable_state has moved blocked -> clean.

Base re-checked after a fresh fetch: git merge-base develop c0716094 == 4ed74ecb == the develop
tip. Still current, so strict: true is satisfied and no update-branch is needed.

Head re-derived immediately before this post: still c0716094.

So the merge gate is met. My caveat in the previous comment ("must not be merged until that job
completes and is read") is discharged — I read it. I am still not the one merging; that duty sits
with the session holding it. Two things for whoever does: re-read this PR's comments in the same
call that merges, since a later verdict can land without the head moving; and write the squash body
by hand to fix 2f5f26c8's "lines 14-25 define seven helpers" (summarize is at :26, and
_apt_failure_reason/_apt_prereqs are also above the guard), because Closes #442 does fire in
this repo and the default concatenation would put that under-enumeration into history as fact.

@VijitSingh97
VijitSingh97 merged commit 5c783bb into develop Sep 5, 2026
9 checks passed
@VijitSingh97
VijitSingh97 deleted the fix/442-e2e-apt-retry branch September 5, 2026 19:00
@VijitSingh97 VijitSingh97 mentioned this pull request Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: the e2e appliance leg has no apt retry, and its abort message blames the network for a mirror mid-sync

1 participant