feat(review): flag command substitution in the tool usage artifact

[zfarrell]

Re-lands the has_subst flag from #26. It was reverted in 8b04393 as collateral — #26 pushed the context step to 22,016 characters, past the 21,000 expression limit, and #29 has since moved that step out to scripts/, so the budget that forced the revert is gone.

Adds a run-block budget guard to the lint test, since nothing in the suite could see either the breach or the 20,545-character near-miss that preceded it. Closes #33 once a week of artifacts confirms the cause.

[claude]

Blocking: the scanner cannot see a run block written as the first key of a step, so - run: | is skipped entirely and never measured.

stripped.lstrip() on - run: | yields - run: |, and .startswith("run:") is false. The line falls through to i += 1 and each of its body lines is then scanned individually — none of which start with run: either, so the whole block is invisible.

The failure is silent, which is what makes it worth fixing before merge rather than after: found is still non-zero from the other blocks, so the NO-RUN-BLOCKS-FOUND sentinel does not fire, and the suite prints ok every run: block is under the ${RUN_BUDGET}-character budget — a claim that is now false. - run: | is the idiomatic form for a step with no name:, so the next person who writes one gets the ok line and none of the protection. That is the same shape as the two outages the comment above cites: the check exists, the check passes, the check was not looking.

Dropping the sequence dash also lands key_indent on the column of run rather than on the dash, which is what the block-body indent has to be compared against:

Suggested change

	stripped = lines[i].rstrip("\n")
	key = stripped.lstrip()
	if key.startswith("run:"):
	rest = key[4:].strip()
	key_indent = len(stripped) - len(key)
	stripped = lines[i].rstrip("\n")
	key = stripped.lstrip()
	# `- run: |` is a step whose first key is run:. Drop the sequence dash, both so the
	# block is measured at all and so key_indent lands on the column of `run` -- the
	# block's content is indented relative to the key, not to the dash.
	if key.startswith("- "):
	key = key[2:]
	if key.startswith("run:"):
	rest = key[4:].strip()
	key_indent = len(stripped) - len(key)

Neither workflow file uses that form today, which is why CI is green either way.

[zfarrell]

Confirmed and fixed in 522adae. Reproduced first: on a two-step file the scanner reported only the named step (35 chars) and missed the - run: | block entirely (79 chars, per PyYAML). The list marker is now stripped off the key, and key_indent is measured off the stripped key so - run: yields 8 — the column run actually sits at — rather than 6.

[claude]

nit: the sentinel proves the scanner found run blocks, but nothing proves it can measure them — so the budget check can only ever report a false negative. (not blocking)

found is incremented independently of size, so if the size arithmetic broke — block_indent slicing off real characters, the plain-scalar branch swallowing a multi-line value, the >= inverted — every block would report 0, found would still be 13, and the suite would print ok forever. The largest block in the repo is now ~6k against a 20,000 budget, so no real input exercises the comparison either; the guard has no case where it is known to fire.

A positive control closes it in a couple of lines, since the script already takes its paths from argv: point a second invocation at a fixture holding one over-budget run: | block (and, per the other comment, one over-budget - run: |) and assert it reports both. That turns "we measured nothing over budget" into "we can tell over from under", which is the property the comment above is actually claiming.

Same reasoning as the wc -l < 100 guard on $CONTEXT_SCRIPT at line 140 and the empty-vocabulary check in tests/tool-usage-test.sh — both exist because a check that silently degrades to a no-op is worse than no check.

[zfarrell]

Taken, in 522adae. The scanner now self-tests against a fixture with known sizes before it runs on the real files: two steps (both shapes), asserting exactly 11 20. Sizes are countable by eye — two 9-char lines plus newlines is 20, one 10-char line plus its newline is 11 — so the assertion needs no YAML parser to justify. That closes the false-negative path you named: a scanner that measured every block as zero now fails here instead of reporting ok forever.

[claude]

Review

The has_subst flag itself is right, and measured the right way: raw command for has_subst, unquoted for compound, with tests/tool-usage-test.sh:180 asserting the two do not collapse into each other, plus expect_absent on both the file name and the quoted code so the flag stays a boolean. The new fixture is the production shape rather than a synthetic one. No concerns there.

The finding is in the new run-block budget guard.

Blocking Issues

• tests/workflow-lint-test.sh:281 — the scanner misses run blocks written as the first key of a step. stripped.lstrip() on a line like " - run: |" yields "- run: |", which does not match .startswith("run:"), so the block is never measured and its body lines are then scanned individually as ordinary lines. The miss is silent: found stays non-zero from the other blocks, the NO-RUN-BLOCKS-FOUND sentinel does not fire, and the suite prints "ok every run: block is under the budget" — which is false. A step with no name: key is the idiomatic case for that form, so the next person who writes one is unprotected by a check that reports success. Fix and reasoning are in the inline comment.

Neither workflow file uses that form today, so this is a hole in the new net rather than a live regression — but the net is what the change is for.

Action Required

Strip the sequence dash before the run: test, and take key_indent from the column of run rather than from the dash, since block content is indented relative to the key. Suggestion is in the inline comment.

Non-blocking, in a second inline comment: nothing in the suite proves the scanner can measure a block, only that it finds one, so the budget comparison has no case where it is known to fire. A fixture holding one over-budget block of each form would cover that and the blocking item together.

I ran nothing: Tests / test was still queued when this review started, so I have no result for the new assertions.

[claude]

super nit: the control covers both block-scalar shapes but not the plain-scalar branch, which is the majority of the blocks it will actually meet. (not blocking)

size = len(rest) at line 299 is the arm taken by 8 of the 13 run: lines in the two workflow files (run: tests/*.sh, run: bash .github-workflows/..., the gh pr comment fallback at claude-pr-review.yml:342 — the longest plain scalar here, and the one most likely to grow). If that arm broke — key[4:] off by one, rest measured before .strip(), the branch reading 0 — this control still prints 11 20 and reports ok, which is the same shape of gap the found sentinel had.

One line in the fixture and one number in the expectation close it:

      - run: echo hi

and "7 11 20 " instead of "11 20 " (echo hi is 7, no trailing newline on a plain scalar). Countable by eye like the other two.

[claude]

Both prior findings are resolved.

The blocking one is fixed correctly: stripping the sequence dash before the run: test makes - run: | visible, and computing key_indent off the stripped key lands it on the column of run ( - run: | → 8), so the body-indent comparison at line 311 is against the right column and a sibling key at indent 8 (env:, the next - name: at 6) still terminates the block. No run: line in either workflow file sits inside another block scalar, so the dash-stripping adds no false positives.

The positive control answers the nit: budget 1 over a known fixture asserts sizes 11 and 20, so a scanner that measured every block as zero, or inverted the comparison, now fails before the real files are scanned. One remaining gap in it, non-blocking, noted inline.

CI was queued at review time; I did not run anything.

[claude]

Both prior blocking findings are fixed in code, and the scanner's positive control now covers all three branches it dispatches on. Verified the expected sizes 10 11 20 by hand against the fixture and the branch logic. CI was still queued when this review started, so I am not asserting the suite's result.