Bound publish-handoff's five network calls and its post-write exit class - #395
Merged
Merged
Conversation
Applies ADR 0068 and references/network-bounds.md to the one caller whose bounded write owns the post-write indeterminacy class. Records the spec, its failure and threat models, and the two-task implementation plan. Refs #384
Both passes' findings applied: the fixture root stubs the public-safety gate rather than symlinking into a shim that cannot resolve there, the stderr pass-through avoids ADR 0047's colon-discard gate, the re-run rule is qualified at its own sentence, the tasks put their failing cases before the implementation, and the bound and hang margins are stated. Refs #384
Records why the fixture rewrites a bound constant in a copy rather than taking either mechanism #384 names, routes the sleep/require_commands interaction to the campaign orchestrator now that #382 is closed, and stops the SKILL.md paragraph claiming every timed-out call is two-sided when only the write is. Refs #384
Adds the bounded_call idiom recorded in references/network-bounds.md, a bounded_network_call wrapper that names a capture slot in the existing 0700 scratch workspace and relays captured stderr, and timed_out, which reports an exceeded bound as this script's own fault -- exit 2, the class the reference's table assigns this caller. gh pr view, gh api on the destination, and git ls-remote each stop capturing into a command substitution and read their value back out of the stdout capture file, which is what the bound requires: a command substitution blocks in the parent and bounds nothing. Refs #384
The create and the readback are the two calls a bound cannot make atomic, so each reports what it actually knows: the write may or may not have landed, and the readback names the comment that was created and whose stored copy went unverified. Neither retries, and both take exit 2 rather than the finding status, because a timeout checked no condition. SKILL.md's re-run rule is qualified at its own sentence and the paragraph below it now says which call reports which state, replacing the paragraph that said these calls carry no bound. Refs #384
The re-run rule no longer prohibits re-running and then instruct a re-run four sentences later: the prohibition is on re-running from the exit status alone, and each call's diagnostic now carries its own next action. The spec's Classification paragraph names both new exit-2 causes rather than one -- the awk scan at the write and the cat read at the ls-remote site are each a scan that could not run. Refs #384
POSIX awk reads an operand shaped name=value as a variable assignment and takes stdin instead, so a relative TMPDIR whose first component is an identifier followed by = would have made the URL scan read this script's own stdin and report a scan that could not run as one that found nothing. The redirect still reads the value back out of the capture file, which is what the bound requires. cat at the ls-remote site takes -- for the option-shaped case, and the spec's threat model now records that the captures reach jq, awk and cat as operands. Refs #384
The redirect pushed the statement one character past the 100-column preference; the file's own prior form for this scan was already a continuation. Refs #384
Owner
Author
|
verdict: approve Forge whole-branch reviewReview exit payloadsFollow-up candidates
Disclosed suppressionsSettled by this change's frozen failure model (
Settled by
Not settled by either — residuals of the recorded mechanism, disclosed rather than closed, and shared with every other applier of this convention:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Applies the network-bound convention recorded in
ADR 0068 and
references/network-bounds.md to all five network calls in
skills/return-to-town/scripts/publish-handoff. Writes no new record: the mechanism, the bound,the write rule and this caller's classification were all already decided.
What changed
bounded_callis transcribed from the reference unchanged, so the repository has one copy of theidiom and no second place for it to drift. Two local wrappers sit on top of it:
bounded_network_callnames a capture slot inside themktemp -d0700 workspace the scriptalready allocates and its EXIT trap already removes, and relays the captured stderr on every
status but 124 —
ghwrites non-fatal material there while exiting 0, and that reached theoperator before these calls were bounded. On 124 it withholds it, because the writer was killed
mid-stream and a partial diagnostic reads as an answer.
timed_outreports an exceeded bound as the script's ownfault— exit 2, its "could not run"class, which is the row the reference's classification table assigns this caller.
124staysinternal and is never an exit status.
All five sites captured into a command substitution, which blocks in the parent and bounds
nothing, so all five are restructurings rather than wrapper swaps: each reads its value back out
of the stdout capture file.
jqtakes the file as an operand in place of a here-string, thegit ls-remoteoutput is re-read withcat, and the readback response becomes a path thatassert_stored_bodyreads with--rawfileunchanged.Each call issues one request, so each takes the 30-second bound. The bound is one script-level
constant and is documented as approximate rather than as a deadline.
What a timeout reports
Every timed-out call exits 2 and names itself. The three calls before the write add that nothing
was posted. The write reports that it may or may not have landed — a bound does not make a write
atomic, and a call killed after GitHub created the comment but before its URL came back leaves a
published block the script cannot see. The readback reports that the comment was created, names
its URL, and reports its stored copy unverified. None is retried.
skills/return-to-town/SKILL.mdno longer says these calls carry no bound. The re-run rule isqualified at its own sentence, and the paragraph below it gives each call's diagnostic its own
next action rather than sending the reader to GitHub in every case.
Two further causes join exit 2, both from moving a captured value into a file:
awkfailing toscan the create response for a URL, and
catfailing to read back the refsgit ls-remotereported. Each is a scan that could not run, so each faults rather than collapsing into the
emptiness check below it — which would report a scan that could not run as one that found
nothing.
The create-response scan reads through a redirect rather than an operand. POSIX
awkreads anoperand shaped
name=valueas a variable assignment and takes stdin instead, so a relativeTMPDIRwhose first component is an identifier followed by=would have made that scan read thescript's own stdin. The redirect still reads the value back out of the capture file.
Test determinism, and why this mechanism
Issue #384 names two ways to reach the bound without waiting 30 seconds — a FIFO the test blocks
on, or an injectable bound — and asks for the choice to be stated. This takes a third:
sedrewrites the one bound constant in a copy of the script, run through the fixture's existing
HELPER_PATH.A FIFO is a file each case must create, open and tear down, and leaves a blocked writer to reason
about when a case aborts. An injectable bound puts a permanent environment key in a shipped
executable for a test's benefit, which an operator could set and collect false timeouts from. The
copy adds no shipped surface, and the fixture fails the case by name if the constant is ever
renamed. Its cost is that five cases exercise a copy whose only difference is the bound.
The margins are stated rather than left implicit: the rewritten bound is 2 seconds against a call
that sleeps 30 — 15x on the bound firing — and that same constant governs the calls which must
succeed in the case, four of them in the readback case at roughly 30 ms each, which is 60x on
the other side.
Five cases cover all five sites.
gitstays real everywhere except the one case that boundsgit ls-remote, which installs a shim that hangs onls-remoteand execs the realgitforeverything else.
Not in this change
No retry or backoff, no bound on the local
git rev-parse --local-env-vars, and no new requiredcommand —
require_commandsis unchanged andscripts/setup.shdoes not gaintimeoutorgtimeout, per ADR 0068.Verification
just verify— one full local run, green, owned by the managed pre-push hook against the exactpushed object. An earlier full run established the clean baseline before implementation, and a
third ran as the assembled-branch integration check after the last task. Three runs, roughly 10
minutes each.
just test publish-handoff— 45 passed, 0 failed. It was 40 before this change.test file kept:
red-confirmed, command exit 1.BASE_SHAset: green at 5.10.1, and red at 5.10.0with the message that the tree differs from the base ref but the version did not increase.
Closes #384
Review exit payloads
Follow-up candidates
references/network-bounds.md— thepublish-handoff:169citation in the private-allocation property, the "all five ofpublish-handoff's" clause in the command-substitution property, and "no executable is bounded yet" above the classification tableDisclosed suppressions
Settled by this change's frozen failure model (
docs/workflow/specs/2026-09-15-bound-publish-handoff-design.md,## Failure model):sleepis not caught — every site calls the wrapper in a|| rc=$?list, which suppressesset -einside it, so the poll loop busy-spins and reports a false 124 on a call that was answering.sleepis not onrequire_commands, and adding it is excluded by this change's charter (owner: Record the timeout convention for git and gh calls in shipped executables #382).kill -0is the only Bash 3.2 idiom available.git ls-remotesitting on an ssh passphrase or host-key prompt is contained by the bound and reported as an exceeded bound rather than named as a prompt.Settled by
references/network-bounds.mdrather than by this change:Not settled by either — residuals of the recorded mechanism, disclosed rather than closed, and shared with every other applier of this convention:
124is in-band with the callee's own exit status, so aghorgitthat genuinely exits 124 would be reported as an exceeded bound. Neither ADR 0068 nor the reference reserves it.