Quote the interpreter path in the deliverables check command - #96
Open
robreyreynolds wants to merge 1 commit into
Open
Quote the interpreter path in the deliverables check command#96robreyreynolds wants to merge 1 commit into
robreyreynolds wants to merge 1 commit into
Conversation
test_runner_harvests_when_task_passes fails on any machine whose Python lives
under a path containing a space. The check is built by interpolating
sys.executable into a shell string:
check=f"{sys.executable!s} -c {json.dumps(check_code)}"
The check then runs through the shell, which splits the unquoted path. On this
machine sys.executable is
/Users/.../Library/Application Support/.../.venv/bin/python3
so the check dies with
/bin/sh: /Users/.../Library/Application: No such file or directory
The worker itself is fine -- it writes site-final.html as intended, and the
taskdir proves it -- but the check can never exit 0, so the task fails, both
attempts are spent, and run() returns 1 against an expected 0. The failure
reads as a harvesting bug and is not one.
CI cannot see this: hosted runners put Python at /opt/hostedtoolcache or
/usr/bin, neither of which contains a space. It reproduces for contributors
whose interpreter sits under "Application Support", "Program Files", or any
venv beneath a directory with a space in the name.
shlex.quote fixes it. Every other sys.executable use in tests/ is an argv list
handed to subprocess without a shell, so this is the only affected site.
Before: Ran 8 tests, FAILED (failures=1).
After: Ran 8 tests, OK. Full suite 253 tests, OK -- no failures.
robreyreynolds
added a commit
to robreyreynolds/ringer
that referenced
this pull request
Aug 5, 2026
A fix-swarm task commissioned to close four scoped action-plan items came
back with those four fixes plus roughly 340 lines of changes nobody
requested -- dark code: an embeddings rewrite, a streaming-envelope
rework, and a new mixed-tool-call-ownership path that touched a live,
deliberately preserved disagreement.
Every existing check passed. The verify command was green, the summary was
well-formed, and every file was inside the ownership list, because the
extra work landed in files the task legitimately owned. The kit had no way
to distinguish "the fix I asked for" from "that fix plus opinions", so the
diff exported clean and the scope creep was only caught by reading all 340
lines by hand.
Ownership lists bound WHERE a worker may write. They say nothing about WHY
a given hunk exists. This adds that second question.
New optional --item-ids flag: the task declares the item IDs it was
commissioned to close, and any diff hunk citing none of them fails as
dark_code. The ID may appear anywhere in the hunk, including a context
line, so a worker's inline comment naming the item it is closing satisfies
it -- no new syntax, no annotation format to learn. The failure names each
offending file and hunk header so the operator can go straight to it.
Opt-in by construction: an empty list disables the check, so kits used by
projects with no item-ID convention behave exactly as before. The kit
manifest passes the flag and the worker spec states the convention -- a
check the worker is not told about is a trap rather than a guardrail.
Deliberately NOT in this PR, to keep it to one concern:
- a --max-diff-lines ceiling, the natural companion, is a separate change;
- templates/repo-feature has the same gap but produces no diff today, so
wiring it there means adding diff export first -- also separate.
Proof: tests/test_fix_swarm_item_ids.py covers a hunk citing a declared
ID, a dark-code hunk being reported with its file and header, the opt-in
empty case, several declared IDs, and the ID appearing in a context line.
All five fail against unfixed main (the function does not exist) and pass
with this change.
End to end against a real repo, one commissioned fix plus one unrequested
rewrite staged together:
upstream today PASS [fix_contract]: exported ... 2 file(s) exit 0
with --item-ids R17-P0.1 FAIL [dark_code]: ... embeddings.py @@ exit 1
Full suite: 258 tests, one failure -- test_deliverables
.test_runner_harvests_when_task_passes -- which fails identically on
pristine main and is fixed separately in NateBJones-Projects#96.
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
1 participant
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.
test_runner_harvests_when_task_passesfails on any machine whose Python lives under a path containing a space.The check command is built by interpolating
sys.executableinto a shell string:That check is then run through the shell, which splits the unquoted path. On my machine
sys.executableisso the check dies before it starts:
Why it reads as a harvesting bug and isn't
The worker is fine — it writes
site-final.htmlexactly as the spec asks, and the taskdir contains it. But a check that cannot run can never exit 0, so the task fails, both attempts are spent, andrun()returns 1 against the expected 0. The assertion that fires isassertEqual(0, exit_code), which points at harvesting rather than at the check's own command line.Why CI is green
Hosted runners put Python at
/opt/hostedtoolcache/...or/usr/bin, neither of which contains a space, so the suite passes there. It reproduces for contributors whose interpreter sits underApplication Support,Program Files, or any venv beneath a directory with a space in the name.The fix
shlex.quote(sys.executable), plus the import. Two insertions, one deletion.I checked every other
sys.executableintests/: they are all argv lists handed tosubprocesswithout a shell, so this is the only affected site — no other call needs quoting.Proof
tests.test_deliverablesbeforeRan 8 tests—FAILED (failures=1)tests.test_deliverablesafterRan 8 tests—OKRan 253 tests—OK, no failuresIndependent of #95 — both branches are cut from
upstream/main, neither is stacked on the other.A possible follow-up, deliberately not in this PR: a CI matrix entry that installs Python under a path with a space would keep this class from recurring.