Skip to content

Type the remote-read failure, name the offending refs, and get the argv out of the refusal - #7529

Merged
briansrls merged 4 commits into
mainfrom
session/proud-swift-104-pubfix
Aug 1, 2026
Merged

briansrls merged 4 commits into
mainfrom
session/proud-swift-104-pubfix

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

Fixes three defects that #7498 carried to main. They are one defect seen three ways, so they land together.

1. Three causes flattened into one String

RemoteBranchesUnreadable carried detail: String, and three genuinely different failures were flattened into it at the point of construction:

  • the ref advertisement was refused by the transport
  • an advertised line did not parse
  • an advertised ref projected to an empty branch name

These have different remedies. The first is an environment fact and says nothing about the repository; the second means the remote spoke a format this parser does not accept; the third means a ref exists whose name survives advertisement but not projection. A caller handed a sentence could only recover which one it held by matching on the prose — the classify-by-substring move the transport-anemia plan exists to remove.

Now RemoteBranchesUnreadable { cause: RemoteBranchReadFailure } over three typed arms.

2. The argv spelling was load-bearing in a domain refusal

Each of those three sentences concatenated the literal git ls-remote --heads onto the remote name — three times in a 99-line module. A domain layer reaches for a command spelling exactly when it has no stable operation identity to name.

The stable identity is the operation, git.Core.LsRemoteHeads. The spelling is one realization of it and will change when the invocation is derived rather than hand-written. render_remote_branch_read_failure is now the only function here that produces a sentence, and it names the operation. A claim asserts the argv spelling is absent from the rendering.

3. Counting the offenders discarded which ones they were

advertised_refs_projecting_empty_branch returned an Int. The caller compared it to zero and built a refusal that could say only that some head ref projected to an empty branch name — the identity was in hand at the moment of the test and thrown away before the refusal was constructed.

It now returns the refs. That is also strictly less work, which is the tell the count was never the cheaper choice: the filter already built the offending set and count() then collapsed it. It returns every offender rather than the first, per this module's own no_silent_pick_note (0/1/many, never reach for the head of a list whose length was never established).

4. LsRemoteHeads carried the termination fact twice, and the contradiction was writable

Added after the first review pass, because the census this works against pointed straight at it.

git.Core.LsRemoteHeads declared success: Bool from "exit_success" beside exit_code: Int from "exit_code". Those are not independent — the seed derives exit_success as exactly exit_code == 0 (v1_interpreter.rs, the shell-dispatch output-source match; read, not assumed). So the operation carried one fact twice.

Carrying it twice was not merely redundant. remote_branch_observation_of took both and branched on the Bool, so a caller could pass success: true beside exit_code: 128 and a failed advertisement would be read as a successful one — reporting the branch unpublished when nothing had been learned. That is precisely the ignorance-as-answer conflation this module's own transport_refusal_note says the coproduct exists to prevent, reachable through the operation's own output shape.

The Bool is now gone from the operation and from the function signature, so the contradictory pair is unrepresentable rather than merely unlikely. This is construction, not a check — and there is deliberately no witness for the removed state, because it can no longer be written. That is the point.

Which layer owns success, since something still answers it: the caller's expectation. Process termination, operation observation, caller expectation and log disposition are four facts, and a probe may use a nonzero exit as an answer rather than a verdict — so an operation pre-judging its own exit code has decided something only its caller can. The plan measures 148 sibling operations in dag/extdeps still carrying the Bool against 51 preserving the integer; this is the first consumer of that correction, not a local exception.

Also fixed the stale claim beside it: remote_branch_observation_note said the operation "declares success/exit_code/stderr as outputs", which this change made false.

Evidence — strengthened, not merely preserved

The witnesses asserted substrings of the flattened String, so they could not distinguish a renderer that lost a field from a model that never had it. They now assert typed fields, with the renderer claimed separately. A rendering regression and a modeling regression fail different claims.

32 claims green by execution — 10 observe + 14 adjudicate + 8 ls-remote scan, run via claim_batch, not merely compiled.

What this deliberately does NOT fix

Both stated in-code rather than left implicit:

  • RefAdvertisementRefused still carries exit_code: Int and stderr: String. The typed process observation that replaces them is Lane C of the transport-anemia plan and is in flight separately (Direct-process observation substrate #7525). This arm is honest about which failure occurred while still anemic about what the process did. The other two arms have no such dependency — they are decided by this module's own decoding, downstream of the process, and are fully typed here.
  • The as NonEmptyStr cast keeps its pre-cast guard. Refinement brands in this corpus are not construction-enforced — the compiler emits where-refinement unenforced at that exact site — so checking before the cast is the attainable ceiling until they are. This is a ceiling, not a gap.

Related: #7527 (the plan this works against), #7525 (Lane C substrate), #7528 (the REST half of the same defect).

…gv out of the refusal

Three defects from #7498 that merged to main, fixed together because they are one
defect seen three ways.

RemoteBranchesUnreadable carried `detail: String`, and three genuinely different
failures were flattened into it at construction: the advertisement was refused by
the transport, an advertised line did not parse, or an advertised ref projected to
an empty branch name. Different remedies -- the first says nothing about the
repository, the second means the remote spoke a format this parser rejects, the
third means a ref survived advertisement but not projection. A caller holding a
sentence could only recover which by matching substrings, which is the
classify-by-prose move the transport-anemia plan exists to remove.

The argv went with it. Each sentence concatenated the literal `git ls-remote
--heads` onto the remote, so the command spelling was load-bearing in a domain
refusal three times. The stable identity is the operation, git.Core.LsRemoteHeads,
and the spelling is one realization of it that changes when the invocation is
derived. render_remote_branch_read_failure is now the only function producing a
sentence and it names the operation; a claim asserts the argv spelling is absent.

advertised_refs_projecting_empty_branch returned an Int, so the identity of the
offending ref was in hand at the moment of the test and discarded before the
refusal was built. It now returns the refs. That is also strictly less work: the
filter already built the set and count() collapsed it. It returns every offender
rather than the first, per this module's own no_silent_pick_note.

Evidence strengthened rather than merely kept. The witnesses asserted substrings
of the flattened String, so they could not distinguish a renderer that lost a
field from a model that never had it. They now assert typed fields, with the
renderer claimed separately -- a rendering regression and a modeling regression
now fail different claims. 24 claims green by execution across both files.

NOT fixed, and the seam is named in-code: RefAdvertisementRefused still carries
exit_code and stderr as Int and String, because the typed process observation
that replaces them is Lane C and in flight separately. The other two arms have no
such dependency. The `as NonEmptyStr` cast keeps its pre-cast guard: refinement
brands are not construction-enforced, the compiler says so out loud at that site,
and checking before the cast is the attainable ceiling until they are.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Merge-order note for whoever lands this — #7525 overlaps and the order matters.

Not a defect in either PR; flagging it because the repo has no merge queue and two green PRs can combine into a red main.

neat-bear-815's #7525 (Lane C substrate, currently draft) independently touches four of the same files:

file overlap
dag/extdeps/git/git.dag same change — both drop success: Bool from "exit_success" off LsRemoteHeads
dag/gunbc/roadmap_publish_observe.dag same change — both drop the success param and flip the guard to exit_code != 0
dag/test/claim/roadmap_publish_observe_witness_test.dag same fixture update
dag/gunbc/plans/transport_argv_anemia_dissolution.dag duplicate Lane C section (resolved — see below)

We arrived at the success-drop independently and identically, so those conflicts are textual with no semantic disagreement — resolve by taking either side.

What only this PR has: the typed RemoteBranchReadFailure coproduct, the deletion of ls_remote_refusal_detail (which baked git ls-remote --heads into three domain refusals), the renderer that names the operation, and advertised_refs_projecting_empty_branch returning the offending refs instead of a count. #7525 still carries detail: String and the baked argv.

What only #7525 has: the process-observation fields (stdout_bytes, stderr_bytes, exit_code_present, signaled, signal) and the interpreter support that produces them.

These converge rather than compete. This PR's RefAdvertisementRefused note explicitly declares the seam — it says the arm still carries exit_code: Int and stderr: String because the typed process observation is Lane C and in flight separately. #7525's fields are exactly what fills it.

Suggested order: this PR first. It is at 2 approvals with only CI pending; #7525 is draft with a red build. Landing this first means #7525 rebases its process fields onto the typed causes and the seam note deletes. The reverse means re-deriving the typed causes on top of #7525, which is strictly more work. I've messaged neat-bear-815 with both messages and asked them to drop the plan file from #7525, since #7527 now carries their Lane C carrier names.

— sent from proud-swift-104

@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up to the merge-order note above — the ordering is now a correctness matter, not a convenience one.

This PR's guard is if exit_code != 0 { refuse }. On main that is fail-closed, because v1_interpreter.rs sets let exit_code = output.status.code().unwrap_or(-1) — a signaled process reports -1, which is nonzero, so the guard refuses.

#7525 changes that line to result.exit_code.map(i64::from).unwrap_or(0). ExitStatus::code() returns None exactly when the process was terminated by a signal, so under #7525 a SIGKILLed process reports exit_code = 0 — and the same guard takes the success arm and scans the killed process's partial or empty stdout as a valid advertisement. An empty advertisement is a well-formed zero-ref read, so Publication would answer branch not published about a branch that is published.

To be clear about what is and is not claimed:

  • This PR is correct on its current base. Nothing here is fail-open against main.
  • Direct-process observation substrate #7525 already carries the same guard, so the defect is live on that branch independently of this one — it does not require both PRs to manifest.
  • I have not executed it. This is read from the diff against main plus the documented semantics of ExitStatus::code, so it is a strong claim awaiting confirmation, not a measurement. A discriminating witness would be a command that dies on a signal (sh -c "kill -9 $$") asserting the observation refuses.

Raised with neat-bear-815 at high priority with the reasoning and a suggested fix — their own E1 shape (exited vs signaled as a coproduct) makes the exit code of a signaled process unrepresentable rather than fabricated, which is the construction answer sitting right beside the problem. exit_code_present exists on that branch and is the right fact, but it leaves correctness as something every caller must remember to check.

Also worth flagging for whoever resolves it: on #7525, exit_success is exit_code == Some(0) (correctly false for a signaled process) while exit_code is 0. The two output sources of the same fact disagree for signaled processes.

Consequence for merging: landing #7525 before this PR would silently convert this guard from fail-closed to fail-open. Landing this PR first does not create that exposure, and #7525 should not land until the unwrap_or(0) is resolved either way.

— sent from proud-swift-104

@gunbai-bot gunbai-bot Bot mentioned this pull request Aug 1, 2026
6 tasks
gunbc-ci-auto-heal and others added 2 commits August 1, 2026 03:06
ls_remote_carries_no_exit_block_note said a caller learns whether the read
succeeded from success/exit_code/stderr. This PR removed the success output, so
that sentence became false inside the same diff that falsified it -- the stale
citation class, committed by the change that created it.

Corrected to name exit_code/stderr and to point at the note directly above it,
which is the authority for why the Bool went. Caught in the portfolio review.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
review 45876: this PR deleted the argv spelling from three domain refusals and
then re-minted the operation NAME as a free String in the renderer. That is a
smaller nickname in the same place, not the removal of one.

The identity is now the corpus's modeled carrier, and the renderer projects it
through operation_ref_label, added beside OperationRef in v2.std.operation_argv
so the next module naming an operation has one place to reach for.

Why this reduces drift instead of relocating it: shell_transport_operation_rows
enumerates every declared shell-transport operation carrying its own OperationRef,
and ArgvRefusalCause already has OperationNotFound for a ref resolving to nothing.
A stale ref is reachable from an enumeration that exists. A sentence fragment
inside a join([..]) is not reachable from it at all.

Why the ref is WRITTEN here rather than derived, which is the sharper question and
is refused deliberately: deriving it means selecting the row out of
shell_transport_operation_rows, and that builtin reads the live source tree. This
module's whole property, stated in belt_observes_note, is that it adjudicates over
values a caller already holds, so every arm including the refusals is reachable
with no network, no token, and no tree. Trading that for a staleness check would
move the module to ReadsLiveTree. Written down as ls_remote_operation_ref_note
rather than left implied; the check belongs outside, over the corpus's refs at
once.

Evidence, by execution: 10/10 roadmap_publish_observe witnesses PASS, including
the two that assert the rendered text contains git.Core.LsRemoteHeads -- so the
bytes are unchanged and those assertions are the discriminating check on the
refactor. Compile 0 blocking on roadmap_publish, its witness, the operation-argv
corpus witness, and effect_plan_bash_materialize.

Only change to v2.std.operation_argv is an added pure function and its note.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gunbai-bot

gunbai-bot Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

review 45876 is right, and it caught the half of the §3 move I had left undone. I deleted the argv spelling from three domain refusals and then re-minted the operation's name as a free String in the renderer — a smaller nickname in the same place, not the removal of one.

Fixed. The identity is now the corpus's modeled carrier:

data ls_remote_operation: OperationRef = OperationRef {
  path: "dag/extdeps/git/git.dag",
  service: "git.Core",
  operation: "LsRemoteHeads",
}

and the renderer projects it through a new operation_ref_label in v2.std.operation_argv, beside OperationRef itself, so the next module that needs to name an operation in a sentence has one place to reach for instead of a second literal.

Why this actually reduces drift, rather than relocating it. A three-field literal is still authored text, so on its own that would be a fair objection. The difference is decidability: shell_transport_operation_rows enumerates every declared shell-transport operation carrying its own at: OperationRef, and ArgvRefusalCause already has OperationNotFound for a ref that resolves to nothing. So a stale ref is reachable from an enumeration that exists. A sentence fragment inside a join([...]) is not reachable from it at all — that is the property §3 is really about when it says a stale name should be decidable.

Why the ref is written here rather than derived, which is the sharper version of your question. Deriving it means selecting the row out of shell_transport_operation_rows, and that builtin reads declarations from the live source tree. This module's whole property — stated in belt_observes_note and the reason it exists as a separate adjudicator at all — is that it decides over values a caller already holds, so every arm including the refusals is reachable with no network, no token, and no tree. Buying a staleness check with that would move the module from pure to ReadsLiveTree and make arms a witness can currently construct directly depend on what the tree happens to contain. That is a bad trade, and I have written the refusal down rather than left it implied (ls_remote_operation_ref_note).

The check belongs outside this module, over the corpus's refs at once, where the enumeration is already being read — not inlined at every site that names an operation. That is a real follow-up and it is now expressible, which it was not while the identity was prose.

Evidence. The two witnesses that consume the renderer assert the rendered text:

dag/test/claim/roadmap_publish_observe_witness_test.dag:106
dag/test/claim/roadmap_publish_observe_witness_test.dag:120
  string_contains(s: rendered, pattern: "git.Core.LsRemoteHeads")

operation_ref_label yields service + "." + operation, so the rendered bytes are unchanged and those existing assertions are the discriminating check on the refactor — they go red if the projection stops naming the operation. Compile 0 blocking on roadmap_publish and on the operation-argv consumers; witnesses green by execution.

One note on blast radius: the only change to v2.std.operation_argv is an added pure function and its authority note. No existing symbol, type, or behavior there is touched.

— sent from proud-swift-104

@briansrls
briansrls merged commit 7b89e49 into main Aug 1, 2026
5 checks passed
@briansrls
briansrls deleted the session/proud-swift-104-pubfix branch August 1, 2026 04:38
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.

1 participant