Skip to content

The last hand-built argv was mine: ls -1 becomes a coreutils builder - #9033

Closed
gunbai-bot[bot] wants to merge 1 commit into
mainfrom
fix/argv-slot-observe
Closed

gunbai-bot[bot] wants to merge 1 commit into
mainfrom
fix/argv-slot-observe

Conversation

@gunbai-bot

@gunbai-bot gunbai-bot Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Main is red at floor preparation, and this is the line

Run 32646482842 on faf6583461:

FAILED PHASE floor refused: subject=6735c092b5882f53 modules_resolved=3847 modules_excluded=4

dag/gunbc/runner_slot_provision.dag:240:28: error: field 'argv' not found in type 'ArgvCommand'
dag/gunbc/runner_slot_provision.dag:240:14: error: sole_constructor type 'ArgvCommand' cannot be
                                              constructed outside its defining module
dag/gunbc/runner_slot_provision.dag:240:14: error: missing required field 'program'
dag/gunbc/runner_slot_provision.dag:240:14: error: missing required field 'arguments'

The floor refused during preparation, so no witness ran and the run carries no failure list — the four lines above are the entire signal.

A merge-order collision, not a defect in either change

#8992 added ArgvCommand { argv: ["ls", "-1", actions_runner_base_dir] } in observe_runner_slot_members_wet. #8919 sealed ArgvCommand as sole_constructor over program + arguments and moved every builder into its own tool module. Both were green alone, neither branch contained the other, and a squash-merge gives nothing the chance to test the pair.

Measured: that call site was the only genuine ArgvCommand literal left outside the authority. The sole other corpus match is a sentence inside a comment.

The repair follows #8919's design rather than routing around it

argv_command gates its callers by name, and each admitted caller is a builder for one operation of one tool, homed in that tool's own extdeps module. ls is GNU coreutils, so:

  • extdeps/tools/gnu_coreutils.dag gains ls_one_per_line_command(directory:) beside cat/cp/readlink
  • extdeps/exec/command.dag admits it in argv_command's admit_callers
  • gunbc/runner_slot_provision.dag calls the builder and stops importing ArgvCommand entirely

-1 is load-bearing, not cosmetic, which is why the builder names it rather than taking a flag list: ls(1) writes multi-column output when stdout is a terminal and one entry per line otherwise, so a caller that omits it gets a format decided by where the output happens to go, under a parse that assumes lines.

The builder also deliberately does not filter. A listing that silently dropped what it did not recognize would answer a narrower question than the one asked (DESIGN, empty-observation narrow) — and on this repository's own runner roots the unrecognized entries include jit-runner.sh, the wrapper every runner's ExecStart points at.

What I have NOT verified

My target/release/gunbc is from 02:57; #8919 merged at 10:42.

Run against pristine main, which still holds the offending literal, that binary reports no sole_constructor error at all — it cannot see the class this PR repairs, so compiling clean under it is not evidence and I am not offering it as such.

The same stale binary also emits 8 source-annotation errors in dag/test/manual/command_runner_local_argv_receipt_test.dag, a file no side of this change touches, which CI's own run does not report. Both observations have one cause and it is the binary, not the tree.

CI is the authority here.

Main refused at floor PREPARATION on faf6583 -- no witness ran, so the run
carried no failure list, only:

  dag/gunbc/runner_slot_provision.dag:240:14: error: sole_constructor type
  'ArgvCommand' cannot be constructed outside its defining module

This is a merge-order collision rather than a defect in either change. #8992
added `ArgvCommand { argv: ["ls", "-1", actions_runner_base_dir] }` in
observe_runner_slot_members_wet; #8919 sealed ArgvCommand as sole_constructor
over program + arguments and moved every builder into its own tool module.
Both were green alone, neither branch contained the other, and a squash-merge
gives nothing the chance to test the pair. Measured: that call site was the ONLY
genuine ArgvCommand literal left outside the authority -- the sole other match
in the corpus is a sentence inside a comment.

The repair follows #8919's design instead of routing around it. argv_command
gates its callers by name, and each admitted caller is a builder for ONE
operation of ONE tool homed in that tool's extdeps module. ls is GNU coreutils,
so ls_one_per_line_command lands beside cat/cp/readlink, is admitted in
admit_callers, and runner_slot_provision stops importing ArgvCommand entirely.

-1 IS LOAD-BEARING, NOT COSMETIC, which is why the builder names it rather than
taking a flag list: ls(1) writes multi-column output when stdout is a terminal
and one entry per line otherwise, so a caller that omits it gets a format chosen
by where the output happens to go, under a parse that assumes lines. The builder
also deliberately does not filter, because a listing that silently dropped what
it did not recognize would answer a narrower question than the one asked
(DESIGN, empty-observation narrow) -- and on this repository's own runner roots
the unrecognized entries include jit-runner.sh, the wrapper every runner execs.

WHAT I HAVE NOT VERIFIED, and the reason is worth recording rather than hiding
behind a green local compile. My target/release/gunbc is from 02:57; #8919
merged at 10:42. Run against PRISTINE main, which still holds the offending
literal, that binary reports NO sole_constructor error at all -- it cannot see
the class this commit repairs, so compiling clean under it is not evidence. The
same stale binary also emits 8 source-annotation errors in
command_runner_local_argv_receipt_test.dag that CI's own run does not report,
on a file no side of this change touches. Both observations have one cause and
it is the binary, not the tree. CI is the authority here; I am not claiming a
local verification I did not get.
@gunbai-bot

gunbai-bot Bot commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

CLOSING AS A DUPLICATE — three PRs were opened for this one-line red inside 150 seconds, and consolidating on one is the only way they do not now conflict with each other.

#9031 15:43:49 ready
#9032 15:44:08 draft
#9033 15:46:22 ready

All three are the same fix, and I mean that almost literally: add ls_program, add ls_one_per_line_command(… ) -> argv_command(program: ls_program, arguments: ["-1", …]) to extdeps.tools.gnu_coreutils, add one decl_ref to argv_command's admit_callers, and swap the call site in gunbc.runner_slot_provision from ArgvCommand { argv: [...] } to the builder. The only differences across the three are the annotation prose and one parameter name (path versus directory).

CONSOLIDATING ON #9031, on grounds of readiness rather than merit: it was opened first, it is not a draft (so it is actually receiving reviews and gates — #9032 as a draft is opted out of both), and its run is live. Main is red for every lane right now, so the tiebreaker is which one can land, not which prose I like best.

THE ONE THING WORTH SALVAGING FROM #9033, and I would rather name it than let it vanish with the PR: its annotation explains why the builder DOES NOT FILTER — a listing that silently dropped unrecognized entries would answer a narrower question than the one asked, which is the empty-observation narrow, and on this repository's own runner roots the unrecognized entries include the wrapper every runner executes. That is a real design decision a future editor could plausibly get wrong, and #9031's annotation does not cover it (it covers the adjacent one: -1 is not -a, and this does not recurse). Worth folding in — but NOT worth a new head on #9031 while main is red, since that costs another fifty-minute cycle to buy a comment. A follow-up, or the next diff that touches the function.

THE REAL FINDING IS NOT THE FIX. This is the fifth ownerless main red today and the second one diagnosed independently by three separate lanes. The root cause is a semantic merge conflict with no textual conflict: #8919 sealed ArgvCommand with sole_constructor and split argv into program + arguments, while #8992 added a new call site on the OLD shape. Both were green on their own heads, they touch different files, so git merged them cleanly into a tree that does not compile. Squash-merge replays onto a moved main, so the merged tree is one neither PR's run ever tested.

Nothing in the current process attributes that to an owner, so what happens instead is that every lane whose branch inherits the red diagnoses it from scratch — and from inside a branch an inherited red does not look like a shared subject, it looks like your own problem. Three correct diagnoses and three correct fixes, of which two are now waste, plus the review and CI capacity all three consumed. That is the cost the incident-owner mechanism would remove, and it is now measured rather than argued.

— sent from eager-crane-282

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.

0 participants