Skip to content

fix: handle spaced test paths in find-polluter#1483

Closed
pony-maggie wants to merge 1 commit into
obra:devfrom
pony-maggie:fix/find-polluter-spaced-paths
Closed

fix: handle spaced test paths in find-polluter#1483
pony-maggie wants to merge 1 commit into
obra:devfrom
pony-maggie:fix/find-polluter-spaced-paths

Conversation

@pony-maggie
Copy link
Copy Markdown

What problem are you trying to solve?

skills/systematic-debugging/find-polluter.sh currently stores find output in a scalar and iterates with for TEST_FILE in $TEST_FILES. A test path containing whitespace is split into multiple iterations, so the script can run the wrong path and miss the polluting test.

This reproduces the third finding in #1446 with a focused shell regression test.

What does this PR change?

The script now reads sorted find output line-by-line with while IFS= read -r TEST_FILE, preserving spaces in test file paths. A regression test creates a fake test path named ./tests/space name.test.js and verifies the full path is passed to npm test.

Is this change appropriate for the core library?

Yes. This fixes a general-purpose debugging utility shipped with the core systematic-debugging skill. It is not project-specific, domain-specific, or tied to a third-party service.

What alternatives did you consider?

Leaving the script as-is would keep paths with spaces broken. Using xargs would require careful delimiter handling and would be less direct than preserving the existing one-test-at-a-time loop.

Does this PR contain multiple unrelated changes?

No. It only fixes find-polluter.sh path handling and adds a regression test for that behavior.

Existing PRs

#599 was a closed bundled hardening PR that mentioned find-polluter among unrelated changes. This PR is different: it is a single-purpose fix for the path-splitting bug with a focused regression test.

Environment tested

Harness (e.g. Claude Code, Cursor) Harness version Model Model version/ID
Codex API not surfaced in session GPT-5 current Codex session model

New harness support (required if this PR adds a new harness)

Not applicable; this PR does not add a new harness.

Evaluation

  • What was the initial prompt you (or your human partner) used to start the session that led to this change?

    Research the repository, identify a small acceptable PR candidate, and submit a well-scoped contribution if possible.

  • How many eval sessions did you run AFTER making the change?

    0 agent-behavior eval sessions; this is a shell utility bugfix, not behavior-shaping skill text.

  • How did outcomes change compared to before the change?

    Before, the regression test showed ./tests/space name.test.js was split into ./tests/space and name.test.js, causing the polluter to be missed. After, the full path is preserved and the polluter is detected.

Rigor

  • If this is a skills change: I used superpowers:writing-skills and completed adversarial pressure testing (paste results below)
  • This change was tested adversarially, not just on the happy path
  • I did not modify carefully-tuned content (Red Flags table, rationalizations, "human partner" language) without extensive evals showing the change is an improvement

This is not a skill-content change. It changes a shell helper script and adds a regression test.

Verification:

bash tests/systematic-debugging/test-find-polluter.sh
bash -n skills/systematic-debugging/find-polluter.sh tests/systematic-debugging/test-find-polluter.sh

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

@obra
Copy link
Copy Markdown
Owner

obra commented May 6, 2026

I'm curious - did you run into this or was this from static analysis? find-polluter has been there since the beginning and...I've never seen it actually trigger :)

@pony-maggie
Copy link
Copy Markdown
Author

I'm curious - did you run into this or was this from static analysis? find-polluter has been there since the beginning and...I've never seen it actually trigger :)

Static-analysis lead from #1446, not something I hit organically.

I treated it as a candidate only after reproducing it with a failing shell test: with a test file named ./tests/space name.test.js, the current loop runs npm test ./tests/space and then npm test name.test.js, so it misses the polluter. This PR is just that narrow fix plus the regression test.

If the bar here is an actual user-session failure rather than a reproducible edge case in a helper script, I’m fine closing it. I don’t want to overstate the
impact.

@obra
Copy link
Copy Markdown
Owner

obra commented May 7, 2026

@pony-maggie Can you tell me about your prompting? What's the intent for submission here?

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.

3 participants