feat(rigging): custom test commands (#26, increment 2 of 3) - #35
Conversation
Three tasks: config-layer testCommand validation (argv tuple, reject ${{ and
newline), a plan/stacks refactor that resolves the effective test argv through
one render path while keeping the seven existing goldens byte-identical, and
injection-safety tests + skill docs. testCommand is a manual override, so it
does NOT flow through propose_config -- SIGNAL_KEYS and the #33 guard are
untouched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pty test argv (#26) Two Minor findings from the final whole-branch review: - testCommand's line-break check rejected only \n. A bare \r (or U+2028/U+2029) is a line break to a YAML parser and would let the rendered run: command differ from what was written -- non-exploitable (shlex.quote keeps it inert at the shell layer) but an inconsistency with the validator's own intent. Now uses `part.splitlines() != [part]`, which catches \n, \r, \r\n, trailing breaks, and the Unicode separators alike. - render_argv(()) would render `- run: ""` (a silent no-op test step) if a future non-node stack shipped an empty default_test. Unreachable today; now a loud assert so such a stack fails at build time instead of shipping testless CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
submtd
left a comment
There was a problem hiding this comment.
Reviewed against keel's rubric (correctness, tests, scope, changelog) plus a whole-branch review on the most capable model. Verdict: ready to land.
Correctness / security. testCommand is the first user-controlled text reaching a rendered run: block, and it carries two independent guarantees: shlex.quote per element at render, and load-time refusal of any element containing ${{ or any line break (\n, \r, \r\n, U+2028/U+2029). The whole-branch review probed for a bypass — case games, split tokens, Unicode line breaks — and found none: no accepted value produces an unsafe run: line.
The refactor's safety net holds. python's pytest moved from StackSpec.steps into default_test so user and built-in test commands share one render path. The seven pre-existing goldens are byte-identical — structurally, not incidentally: for any config without testCommand, resolution returns exactly the argv the old code hardcoded.
Tests. 1406 passing. Two new goldens, load-bearing injection tests (hostile refused; accepted metacharacters shlex-quoted, no ${{ in any run block), and per-stack validation including the line-break cases.
Scope. testCommand is a manual override, so scaffold.py/SIGNAL_KEYS are untouched — init does not detect it. Clean.
Changelog. Accurate, user-facing, under Unreleased/Added.
One caught-and-fixed note for the record: a final-review hardening edit initially used a greedy regex that silently deleted 26 tests in test_config.py; caught by comparing the suite count against a measured baseline, restored, and the count is now 1406 with nothing lost.
Good to keel:land.
Increment 2 of 3 for #26. Issue #26 stays open (increment 3, service containers, remains).
What
.rigging.jsongains a per-stacktestCommand— an argv array that replaces a stack's (or its node package manager's) default test command:It's the escape hatch for when the default (
pnpm test,bun run test,python -m pytest) guesses wrong — the exact gap the reporting repo in #24 hit withturbo run test.initnever writes it; it's a manual override.Why an argv array, not a shell string
testCommandis the first user-controlled text that reaches a renderedrun:block, so it carries two independent guarantees:shlex.quoteper element at render — shell metacharacters (;, quotes,$) are inert.${{(a GitHub Actions expression opener — GitHub substitutes it at the YAML layer before any shell, so quoting can't help) or any line break (\n,\r,\r\n, U+2028/U+2029).Pipes,
&&, redirects, and subshells are simply not expressible in an argv array — and that's the feature. A repo needing a shell pipeline needs a hand-written workflow. The final whole-branch review (most-capable model) probed for a bypass — case games, split tokens, Unicode line breaks — and found none: no accepted value produces an unsaferun:line.The refactor, and its safety net
To make user commands and built-in defaults share one rendering path, python's
python -m pytestmoved out ofStackSpec.stepsinto a newStackSpec.default_testargv. The effective test argv resolves as: explicittestCommand> node manager'stest> stack'sdefault_test.The seven pre-existing goldens stay byte-identical — that's the proof the refactor changed nothing for repos not using
testCommand, and it's structural, not incidental: for any config withouttestCommand, resolution provably returns exactly the argv the old code hardcoded.Scope
testCommandis a hand-authored override, so it does not flow throughpropose_config—scaffold.py,test_scaffold.py, andSIGNAL_KEYSare untouched.initdoesn't detect a custom command.Tests
Full suite 1406 passing. Two new goldens (
node-testcommand.yml,python-testcommand.yml), load-bearing injection tests (hostile input refused; accepted metacharacters shlex-quoted with no${{in any run block), and per-stack config validation.🤖 Generated with Claude Code