Skip to content

perf(ci): run each test lane only on the suites the change selects - #6129

Merged
kyle-sexton merged 7 commits into
mainfrom
perf/selection-driven-lanes
Oct 3, 2026
Merged

kyle-sexton merged 7 commits into
mainfrom
perf/selection-driven-lanes

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

No related issue: wave 3 PR 3 of the approved CI design

Summary

Each test lane now runs only the suites the change selects, and a lane the change gives no work does not start. A C# fixture or docs change starts no Python or Node runner, a shell change starts no Node runner, and test-windows runs only the Windows steps whose suites the change reaches.

  • scope plans the test lanes once with the new scripts/plan-test-lanes.sh. The planner runs scripts/affected-tests.sh --unmapped-corpus over the diff and assigns each selected suite to the lane of its ecosystem. *.test.sh goes to test-bash. test_*.py goes to the new test-python. A Node suite runs through its sibling .test.sh in test-bash when it has one, otherwise in its package in the new test-node. Pester suites go to test-windows.
  • test-bash gets 1 to 6 legs and test-python 1 to 4. Legs hold about 120 and 180 suite-seconds and are packed longest first, using the median wall of each suite over four whole-tree CI runs (scripts/suite-seconds.txt; suites on the serial list count three times). Each leg installs only the optional toolchains its own suites need (animation wheels, inventory parser packages, DuckDB).
  • Ordered skips: a test lane with no work skips as a job. ci-status (new step Check each skipped lane against scope) counts that skip as success only where scope succeeded and the lane's run_<x> row is false. Any other skip stays red. scripts/check-docs-only-gate.sh property 12 pins this, and scripts/ci-ordered-skips.test.sh runs the step.
  • test-windows.yml has a new Linux job, scope-windows, which runs the same planner. Each of the six Windows jobs and each of their steps run only when a selected suite starts them (scripts/test-windows-plan.txt; the planner test checks that this file and the workflow name the same keys). A schedule run at the design's off-peak cron (17 9,16 * * *, the same as ci.yml's whole-tree run) runs every step.
  • The planner goes wider than the selection only where the design's S8 rule says to. A Python pin runs every Python suite, a Node pin runs every Node package, and a change to ci.yml or .github/actions/ runs every suite of every lane. An UNMAPPED file adds the corpus of its own language, not the whole shell corpus.

Fix

  • .github/workflows/ci.yml: the plan step replaces the four-leg fan-out step. New scope rows are run_bash, run_python and run_node, plus the plan data. test-bash is rebuilt, and test-python and test-node are new. The Node chains and the disk-hygiene step leave test-bash; the disk-hygiene module now runs as a selected Python suite. The redundant resolver test leaves check-plugins, because its .test.sh wrapper runs it when it is selected. Two jobs join ci-status.needs, and the ordered-skip check is added there. The unread node filter group is removed, and the planner is added to the push run's whole-tree trigger list.
  • .github/workflows/test-windows.yml: scope-windows, per-job and per-step gates, and the schedule.
  • New: scripts/plan-test-lanes.sh with its test, scripts/suite-seconds.txt, scripts/test-windows-plan.txt, and scripts/ci-ordered-skips.test.sh.
  • scripts/check-docs-only-gate.sh and its test: the new rows, the plan data, two matrix reads, and the ordered-skip job condition, which is allowed only on test-<x> with its own row and only when the aggregate reads the checked results.
  • scripts/selection-audit.sh and its test: in a tree whose test lanes are planned, the selector's --unmapped-corpus result is the unmapped fallback.
  • scripts/affected-tests.test.sh live case, docs/ci-runner-routing.md, and three comments.

Verification

Local runs on Windows Git Bash, plus the suites again in WSL Ubuntu on an ext4 clone:

  • scripts/plan-test-lanes.test.sh 28/28, scripts/check-docs-only-gate.test.sh 85/85, scripts/ci-ordered-skips.test.sh 12/12, scripts/affected-tests.test.sh 134/134, scripts/selection-audit.test.sh 18/18 in WSL (with strace), scripts/check-lane-coverage.test.sh, scripts/ci-fail-a-draft.test.sh, scripts/lib/* suites: all pass. scripts/check-script-contract.test.sh failed 2 cases only because htmlhint was absent in the uninstalled WSL clone.
  • actionlint (3 workflows), zizmor --offline, shellcheck/shfmt on every changed script, check-docs-only-gate.sh --check, check-lane-coverage.sh --check, and the lint-shell and lint-repo gate scripts (killswitch hoist, slow shapes, silent skips, discriminating skips, fixture isolation, drive-root, shell portability, pipefail grep, tmp cleanup, conformance registry, docs naming, hook wiring, purged em dashes, contract clauses): all clean.

Two stacked probe PRs, opened against this branch, labelled do-not-merge, and now closed, ran the new ci.yml:

Replay of the 20 most recent squash-merged PRs (#5997..#6121). Each commit's changed files went through this branch's selector, planner and lists. "Before" is today's ci.yml: 1 to 4 legs at 25 suites a leg, the four Node chains on the node filter group, the disk-hygiene step on the python group, and six Windows jobs on the test-windows paths.

PR files before: bash legs (sh suites) / Node chains / disk-hygiene / Windows jobs after: bash legs (sh) / python legs (py) / Node packages / Windows jobs selected via wrapper all run
#6121 1 1 (4) / 4 / yes / 6 1 (4) / 1 (4) / 0 / 1 8 0 yes
#6117 23 2 (28) / 4 / yes / 6 3 (28) / 1 (2) / 0 / 1 34 4 yes
#6115 1 1 (4) / 4 / yes / 6 6 (587) / 4 (139) / 6 / 0 4 0 yes
#6106 24 2 (26) / 4 / yes / 6 4 (26) / 1 (4) / 0 / 2 32 2 yes
#6105 2 1 (4) / 4 / yes / 6 1 (4) / 0 (0) / 0 / 1 4 0 yes
#6104 185 1 (23) / 4 / no / 6 2 (23) / 0 (0) / 0 / 1 25 2 yes
#6102 12 1 (3) / 4 / no / 0 1 (3) / 0 (0) / 0 / 0 4 1 yes
#6094 70 2 (34) / 4 / yes / 6 4 (34) / 1 (6) / 0 / 2 42 2 yes
#6083 15 1 (9) / 0 / yes / 6 1 (9) / 1 (3) / 0 / 0 12 0 yes
#6082 15 1 (9) / 0 / yes / 6 1 (9) / 1 (2) / 0 / 0 11 0 yes
#6081 10 1 (11) / 4 / yes / 6 1 (11) / 0 (0) / 0 / 0 11 0 yes
#6065 54 2 (33) / 4 / yes / 6 4 (33) / 1 (12) / 2 / 1 50 0 yes
#6064 80 4 (111) / 4 / yes / 6 6 (586) / 4 (136) / 6 / 5 121 2 yes
#6063 6 1 (1) / 0 / yes / 6 1 (1) / 1 (1) / 0 / 1 2 0 yes
#6062 32 2 (33) / 4 / yes / 6 3 (33) / 1 (2) / 0 / 1 36 1 yes
#6059 151 4 (88) / 4 / yes / 6 6 (88) / 3 (17) / 0 / 2 105 0 yes
#6018 21 1 (16) / 4 / no / 6 1 (16) / 1 (1) / 0 / 1 18 1 yes
#6013 35 2 (36) / 4 / yes / 6 2 (36) / 0 (0) / 0 / 1 37 1 yes
#6012 22 1 (24) / 4 / yes / 6 1 (24) / 0 (0) / 0 / 1 25 1 yes
#5997 359 4 (118) / 4 / yes / 6 6 (118) / 1 (5) / 0 / 5 126 3 yes

This PR touches ci.yml and test-windows.yml, so its own run is the whole tree: 6 test-bash legs, 4 test-python legs, test-node, and every Windows step.

  • ci run 37151857816 (head ca049a9) was green: 591 shell suites over 6 legs (3m10s to 3m52s each), 139 Python modules over 4 legs (3m11s to 3m52s), and test-node (52s). The whole run took 5m53s, set by lint-shell's whole-repository ShellCheck. The last whole-tree push run on main took 10m46s.
  • The 139 pytest summary lines (passed, skipped, failed per module) are identical as a multiset to those of the last whole-tree run on main (push run 37144196526, Python on the test-bash legs). The narrower test-python toolchain therefore loses no test and adds no skip.
  • The shell corpus showed one new optional skip, plugins/speech/scripts/speech.test.sh (numpy missing). Old whole-tree legs installed the animation wheels everywhere. Fixed in 2918049: speech suites get those wheels, and SPEECH_REQUIRE_DEPS=1 makes a missing numpy fail. Run 37152642136 (head 9b5a894) was green, with the same 26 optional skips over the same suites as main's last whole-tree run.
  • test-windows run 37152642139 was green: all six jobs ran, and no step was skipped.
  • Codex P2 (.python-version skipped every Windows job, although each sets up Python from it): fixed in 52370e0, with tests.
  • github-iac's contract-only verifier (main), run over this ci.yml, reports only the two step-level findings it already reports on main (scope step 0 and the re-run step; today's 07:49 governance-verify run failed on them). The new jobs carry the predicate negated once, so ci-status.needs stays fully gated.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits October 3, 2026 15:56
`scope` now plans the test lanes once with scripts/plan-test-lanes.sh: it
runs the affected-suite selector over the diff and gives each suite to the
lane of its ecosystem. test-bash gets one to six legs packed from measured
suite-seconds (scripts/suite-seconds.txt), new test-python and test-node
jobs run the selected Python modules and the Node packages the change
reaches, and each leg installs only the optional toolchains its suites need.
A lane with no work skips as a job; ci-status counts that skip as success
only where scope succeeded and its run_<lane> row says false, which
scripts/check-docs-only-gate.sh now pins (property 12).

test-windows.yml gets a Linux scope-windows job that runs the same planner,
and each Windows step runs only when the change selects its suite
(scripts/test-windows-plan.txt); a schedule run at 09:17 and 16:17 UTC runs
every step. selection-audit.sh reads the new unmapped fallback (the file's
language corpus) from the selector itself.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 3, 2026 20:31
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-03T20:37:16.053489Z ca049a9 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 6m 9s —— View job


Code review: perf(ci): run each test lane only on the suites the change selects

  • Gather context (PR body, changed files, repo instructions)
  • Invoke /review:code-review and follow its instructions
  • Review the planner, workflow wiring, and gate-script changes
  • Post summary

Summary

Ran the CI code-review lane (correctness + maintainability; security is scoped out since .github/workflows/claude-security-review-hosted.yml runs a dedicated security lane on this repo). I read the full diff against origin/main...HEAD and traced the new logic end to end:

  • scripts/plan-test-lanes.sh: suite classification (*.test.sh, test_*.py, *.test.{js,mjs,cjs} → sibling wrapper or Node package, *.Tests.ps1), the leg-packing awk (ceil-division sizing, cap/force logic, longest-first greedy assignment, the serial-suite 3x weighting), and the whole-tree/pin widening rules (whole, python_pin, node_pin).
  • .github/workflows/ci.yml: the scope job's new plan step and its run_bash/run_python/run_node rows, the new test-python/test-node jobs, the PLAN/LEG_PLAN wiring per leg, and the ci-status "Check each skipped lane against scope" step (traced the jq filter by hand against the new scripts/ci-ordered-skips.test.sh fixtures).
  • .github/workflows/test-windows.yml: the new scope-windows job's base-resolution case and the per-job/per-step contains(fromJSON(...)) gates against scripts/test-windows-plan.txt.
  • scripts/check-docs-only-gate.sh: the new job_gate_skip/SKIP_CHECK_STEP_ID/check 12 logic, confirmed it pins the exact literal text that's actually in ci.yml (job conditions, results: read) and that MATRIX_READS fully replaced the old singular MATRIX_READ with no stale references.
  • scripts/selection-audit.sh: the --unmapped-corpus/--allow-unmapped flag switch and the exit-code normalization (rc=4 → 0 under --unmapped-corpus) in both select_one and replay_rows.

I also checked one latent coupling: the shared need() helper inside pack() flags "duckdb" for any suite path under plugins/harness-ops/, including Python suites outside inventory/, but test-python's "Install locked plugin test toolchains" step has no DuckDB-CLI install (only test-bash does). Today this is inert — none of the Python suites under plugins/harness-ops/ actually touch DuckDB (only the .test.sh suites under observability/ do, which run in test-bash) — so I'm not raising it as a finding, but it's worth a note if a DuckDB-dependent Python suite is ever added there.

I did not find a correctness bug I could pin to a concrete failing input in the new planner, the workflow wiring, or the gate-script updates. The author's own verification (local test runs, actionlint/zizmor/shellcheck, two real probe-PR workflow runs, and a 20-PR replay) lines up with what the code does on inspection; I did not re-run those myself (no shell execution available in this environment) and am not claiming to have independently reproduced them.

No blocking or high-confidence findings to post inline.

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 51s —— View job


Security review — /review:security-review

  • Gather context (PR diff, changed files)
  • Review workflow/script changes for security-relevant surface (injection, trust boundaries, secrets)
  • Check instruction-surface deletions against protected classes
  • Post findings or clean bill

No security issues found.

This PR only touches CI orchestration (.github/workflows/ci.yml, .github/workflows/test-windows.yml, scripts/plan-test-lanes.sh and the related test/data files). No CLAUDE.md/AGENTS.md/rules file is touched, so the instruction-surface-deletion check doesn't apply here.

What I specifically checked, since this is the kind of logic/trust-boundary surface this lane owns (GitHub Actions hardening itself — pull_request_target, permission widening, unpinned actions — is zizmor's lane, not re-reported here):

  • Expression → shell injection. Every needs.scope.outputs.* / steps.plan.outputs.* value that reaches a run: block does so through an env: entry (PLAN, LEG_PLAN, PACKAGES, etc.) and is read back with $VAR / <<<"$VAR", never interpolated with ${{ }} directly inside script text. That's the correct pattern — I grepped every ${{ }} use in both workflow files and confirmed none appear inside a run: body. See ci.yml#L2014-L2015.
  • GITHUB_OUTPUT injection via attacker-controlled suite paths. scripts/plan-test-lanes.sh builds bash_plan/python_plan JSON by string concatenation from selected suite paths (which, on a PR, are attacker-influenceable filenames) and appends the result straight into $GITHUB_OUTPUT without the multi-line <<DELIMITER form. A path containing a raw newline could normally forge extra key=value output lines, but the planner explicitly rejects any suite path containing a quote or backslash (plan-test-lanes.sh#L299), and git itself C-quotes (backslash-escapes) any path containing a raw control character such as \n before it ever reaches this script — so the one case that could inject a line is already caught and fails the job instead of succeeding quietly.
  • Ordered-skip bypass (the PR's own stated risk). The new Check each skipped lane against scope step in ci-status (ci.yml#L2498-L2528) only promotes a test-<x> lane's skipped result to success when scope itself succeeded and that lane's own run_<x> row is exactly 'false'. Any other skip (including a scope failure, or a row that isn't exactly 'false') passes through unchanged and reds the required aggregate. I couldn't find a path where a crafted diff forces a run_<x> row to 'false' while real work still exists without that also being a pre-existing selector-accuracy question already covered by scripts/selection-audit.sh (wave 3 PR 4) rather than something newly introduced here.
  • Trust boundary / token scope. ci.yml keeps permissions: contents: read at the top level and runs on pull_request (not pull_request_target), so a fork PR's token stays read-only regardless of any lane-selection logic bug — this bounds the blast radius of any planner mistake to "wrong/missing test coverage," not credential or secret exposure.

Nothing here rises to a reportable finding under this lane's high-signal bar. GitHub Actions hardening concerns (pinning, permissions:, trigger choice) are zizmor's lane per the PR's own verification section and are out of scope for this review.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through f597b7d; a later push is reviewed from there.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ca049a971f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/plan-test-lanes.sh Outdated
@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through f597b7d; a later push is reviewed from there.

…ithout it

The whole-tree run of the planned lanes showed plugins/speech/scripts/speech.test.sh
skipping its narrate timing suite for a missing numpy: the old whole-tree legs
installed the animation wheels everywhere, and the per-leg plan gave them only to
plugins/animation suites. The plan now gives them to the speech suites too, and
test-bash sets SPEECH_REQUIRE_DEPS=1 so an under-install fails instead of passing.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

Claude Code is working…

I'll analyze this and get back to you.

View job run

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 44s —— View job


Security review — /review:security-review (incremental)

  • Gather context (PR body, custom instructions, prior reviews)
  • Read .claude-lane/incremental.diff (changes since ca049a9 in ci.yml, plan-test-lanes.sh, plan-test-lanes.test.sh)
  • Invoke /review:security-review and follow its instructions for this incremental scope
  • Post findings or clean bill

No security issues found.

This incremental review covers only what changed since ca049a9 (the prior-reviewed commit), per the lane's scope: .claude-lane/incremental.diff touches three files, and every other file in this PR was already reviewed at ca049a9.

What changed and what I checked:

  • .github/workflows/ci.yml: adds a SPEECH_REQUIRE_DEPS: '1' env var alongside the existing ANIMATION_REQUIRE_DEPS, and updates two adjacent comments. Same pattern as the existing flag — a literal '1' value, no interpolation of any PR-controlled data, used only to make the speech suites fail loud instead of skip green when numpy is missing. No new injection surface.
  • scripts/plan-test-lanes.sh: widens the need() classifier's regex from ^plugins\/animation\/ to ^plugins\/(animation|speech)\/, so a changed-file path under plugins/speech/ now also pulls in the animation toolchain tag (where the speech suites get their numpy dependency). This is a static regex alternation over the already-sanitized suite-path strings reviewed in the full-PR pass (quote/backslash-rejected, git-C-quoted); it doesn't change how those strings reach $GITHUB_OUTPUT or run:, so the injection analysis from the prior full review still holds unchanged.
  • scripts/plan-test-lanes.test.sh: adds fixture suites (plugins/speech/sp.sh, plugins/speech/sp.test.sh) and assertions covering the new mapping, plus two line-count updates in unrelated pre-existing test cases now that a plugins/speech/ fixture exists in the tree. Test-only, no production code path.

No new trust-boundary, injection, or credential-handling surface in this increment — it's a toolchain-routing fix (give the speech suites the same numpy wheels the animation suites already get) riding the same mechanisms the full-PR review already cleared. GitHub Actions hardening concerns remain zizmor's lane and are unaffected by this increment.

kyle-sexton and others added 2 commits October 3, 2026 16:44
Every test-windows job sets up its Python from .python-version, so a change to
it is a change to every Windows step, as the old path filter treated it. The
planner now plans the whole Windows lane for it; the other Python pins still run
only the Python suites.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 36s —— View job


Security review — /review:security-review (incremental)

  • Gather context (PR body, custom instructions, prior reviews)
  • Read .claude-lane/incremental.diff (changes since 291804928f2d38fc8199d562539666847f5f5dcb in scripts/plan-test-lanes.sh, scripts/plan-test-lanes.test.sh)
  • Invoke /review:security-review and follow its instructions for this incremental scope
  • Post findings or clean bill

No security issues found.

This increment is the fix for Codex's P2 finding on this PR (a .python-version change previously left whole_windows unset, so a Python-version bump could skip all Windows suites). Both changed files are in scope:

  • scripts/plan-test-lanes.sh#L143-L146: the .python-version case now sets both python_pin=1 and whole_windows=1, split out of the shared pyproject.toml | uv.lock | requirements*.txt | .github/requirements-ci*.txt arm, which keeps python_pin only. This is a pure classification change on the already-sanitized list of changed file paths — it widens which lane runs more work, in the safe direction, and touches none of the quoting/escaping or $GITHUB_OUTPUT-writing logic the full-PR review already cleared (plan-test-lanes.sh lines ~290–300, unchanged in this increment). No new injection surface, no new trust boundary, no change to how PR-controlled data reaches a shell command or workflow output.
  • scripts/plan-test-lanes.test.sh: adds a pyproject.toml fixture and two assertions (.python-version → windows_jobs populated; pyproject.toml alone → windows_jobs=[], python true) pinning the new behavior. Test-only, no production code path, no security relevance.

Nothing else changed in this increment. GitHub Actions hardening concerns remain zizmor's lane and are unaffected.

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 52s —— View job


Code review: incremental (since ca049a9)

  • Gather context (PR body, custom instructions, prior reviews)
  • Read .claude-lane/incremental.diff (changes since ca049a9 in ci.yml, plan-test-lanes.sh, plan-test-lanes.test.sh)
  • Invoke /review:code-review and follow its instructions for this incremental scope
  • Post findings

Summary

This increment is two small fixes, both addressing feedback already on this PR:

  1. fix(ci): run every Windows step when the Python version pin changes — scripts/plan-test-lanes.sh#L143-L147 now sets whole_windows=1 on a .python-version change, in addition to python_pin=1. I traced this against the Codex P2 finding on the prior commit (a Python-version-only PR left scope-windows emitting windows_jobs=[], skipping all six Windows jobs even though each one resolves its interpreter via actions/setup-python with python-version-file: .python-version). I confirmed all six test-windows.yml jobs use exactly that (grep -n "python-version-file" .github/workflows/test-windows.yml → six matches), and that .python-version is in both the push and pull_request paths: filters that gate the workflow run itself, so the fix is live, not inert. The new test case (plan-test-lanes.test.sh lines ~136-143) asserts windows_jobs becomes ["win-a","win-b"] (all fixture jobs) on .python-version, and a second new case confirms the other Python pins (pyproject.toml, etc.) correctly do not widen windows_jobs to []→still-empty, since nothing else in test-windows.yml reads those files for interpreter setup. Both assertions check out by hand-tracing the script logic (scope-windows's on=$whole_windows short-circuit at plan-test-lanes.sh#L370).

  2. fix(ci): give the speech suites the numpy wheels and fail them loud without it — the need() classifier's regex widens from ^plugins\/animation\/ to ^plugins\/(animation|speech)\/ (plan-test-lanes.sh#L301), and ci.yml adds SPEECH_REQUIRE_DEPS: '1' next to ANIMATION_REQUIRE_DEPS in test-bash's "Run this leg's shell suites" step. I verified the env var lands in the right job: plugins/speech/scripts/speech.test.sh is a *.test.sh suite (→ test-bash, not test-python), it's the one file that actually reads SPEECH_REQUIRE_DEPS (confirmed by grep — no Python suite under plugins/speech/ checks that var), and it only imports numpy (not opencv), so tagging it with the shared animation toolchain key is correct: that leg's "Install locked plugin test toolchains" step installs .github/requirements-ci-animation.txt (which bundles both numpy and opencv as one locked file) whenever leg_needs contains animation, giving the speech suite the numpy it needs. The regex alternation syntax ((animation|speech)) is consistent with existing group/alternation usage elsewhere in this same file (plan-test-lanes.sh#L292) and other scripts in the repo, so no awk-portability concern.

Both fixes are covered by new assertions in plan-test-lanes.test.sh, and I traced the control flow by hand for each (no shell execution available in this environment, consistent with the note on the prior full-PR review). I did not find a correctness bug in either change.

One purely cosmetic nit, not worth blocking on: the new ci.yml comment at ci.yml#L2079-L2080 — "The animation wheels (numpy and opencv, which the speech suites use too)" — reads at first glance as if speech needs opencv too; it actually only needs numpy and gets opencv as an incidental side effect of the two wheels being locked in one shared requirements file. The parenthetical is defensible as written (describing the shared install, not a per-suite dependency claim), so I'm not filing it as a finding.

No blocking or high-confidence findings to post inline.

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 24s —— View job


Code review: incremental (since 9b5a894)

  • Gather context (PR body, custom instructions, prior reviews)
  • Read .claude-lane/incremental.diff (changes since 9b5a894 in .github/workflows/ci.yml)
  • Invoke /review:code-review and follow its instructions for this incremental scope
  • Post findings

Summary

This increment is comment-only: it rewords the "Run this leg's shell suites" step's comment in ci.yml#L2079-L2081, from

The animation wheels (numpy and opencv, which the speech suites use too) ...

to

The animation wheels (numpy and opencv; the speech suites need their numpy) ...

This directly addresses the cosmetic nit from the prior incremental review on this PR (that the old wording could be misread as "speech needs opencv too"). The new phrasing makes clear speech only needs numpy and gets opencv as an incidental side effect of the shared locked-requirements file. No executable line changed — SPEECH_REQUIRE_DEPS, the need()/toolchain-tag logic in scripts/plan-test-lanes.sh, and the install step gating are all unchanged in this increment.

No findings. Nothing in this diff has a correctness, maintainability, or review-criteria issue to raise.

@claude

claude Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 38s —— View job


Security review — /review:security-review (incremental)

  • Gather context (PR body, custom instructions, prior reviews)
  • Read .claude-lane/incremental.diff (changes since 9b5a894 in .github/workflows/ci.yml)
  • Invoke /review:security-review and follow its instructions for this incremental scope
  • Post findings or clean bill

No security issues found.

This increment is a single comment-text edit in ci.yml#L2079-L2080, rewording the parenthetical about which wheels the speech suites need:

-      # The animation wheels (numpy and opencv, which the speech suites use
-      # too) and the inventory's parser packages go only on a leg whose suites
+      # The animation wheels (numpy and opencv; the speech suites need their
+      # numpy) and the inventory's parser packages go only on a leg whose suites

This is the fix for the cosmetic nit raised in the prior code-review pass on this PR (the old wording read as if speech needed opencv too). It's a comment only — no code, no run: block, no expression, no env var, no control-flow change. I confirmed via git diff 9b5a894..f597b7d -- .github/workflows/ci.yml that this is the entirety of the diff for this file in this increment, matching .claude-lane/incremental.diff exactly.

No injection surface, no trust-boundary change, no permission or secret handling touched. This is not an instruction file (CLAUDE.md/AGENTS.md/a rules file/a skill or agent body/a hook allowlist), so the instruction-surface-deletion lens doesn't apply. Skip-gate criterion 2 (no security-relevant surface after reading the diff) is met — nothing further to review in this increment.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 3, 2026
Merged via the queue into main with commit 9b00165 Oct 3, 2026
45 of 46 checks passed
@kyle-sexton
kyle-sexton deleted the perf/selection-driven-lanes branch October 3, 2026 21:07
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.

1 participant