perf(prepare-pr): skip the test suite a change cannot affect, using CI's own selector

[chenmingwei23]

1. What is the problem?

The prepare-pr gate floor ran both test suites in full, locally, on every iteration of its inner loop:

• python -m pytest -q -- 62,108 collected tests. Collection alone takes ~100s before a single test executes.
• npm --prefix website test -- ~1,444 frontend spec files, plus jscpd and the Electron specs via pretest.

Phase 2 is a bounded loop capped at 10 rounds, and CI runs the full suite on refs/pull/<N>/merge regardless of what ran locally. Most PRs touch one surface, so half of that work could not tell the author anything -- a frontend-only PR was running all 62k backend tests to learn nothing.

2. Why this issue matters to the user

It is the largest cost in the loop and it is paid on a workstation rather than on eight sharded CI runners. Every review round pays minutes to tens of minutes of wall clock before any reviewer runs. On a busy machine the aggregate agent memory crosses the cgroup MemoryHigh ceiling and the kernel throttles the whole agent subtree, which surfaces as unrelated 30s initialize timeouts in other sessions.

3. How our fix solves it

scripts/run_scoped_tests.py performs exactly one reduction, and it is CI's own: when a diff touches only one surface, the other surface runs the cross-surface set instead of its full suite. Measured on this checkout, 350 backend files for a frontend-only diff and 146 frontend specs for a backend-only one, via scripts/ci-surface-tests.py -- the same script and the same post-processing ci.yml uses, so there is no second selector here that can silently disagree with CI. A plain skip would have been unsafe: a frontend-only change really can break a backend test that reads a frontend module.

Four verdicts, and everything that is not a reduction runs everything:

condition	verdict
base ref absent or unresolvable	exit 2, fail closed, run nothing
broad-impact file changed (fixtures, collection config, workflows, lockfiles, the vitest setup graph, the runner itself)	full suite
the diff touches THIS surface	full suite
the diff touches only the OTHER surface	cross-surface set

What this deliberately does not do, and why that is the headline of this PR. It does not narrow within the surface a change touches. That was implemented here first, by scanning tests for textual references to the changed module, and it was removed after six review rounds produced nine findings -- all real, all one impossibility: a text scan cannot enumerate the ways a test can reach a module. The spellings found were absolute import, relative import, barrel re-export, in-package fixture, global vitest setup, data-file read, cross-surface parity comparison, and documentation contract, and every remedy shrank the allowlist further toward "escalate everything", which is the full suite again. Doing it soundly needs a real import graph; that is now #5303.

The nine measured traps from the removed attempt are recorded in references/gate-floor.md rather than deleted, because #5303 will meet the same ones: bare-stem matching selecting 621 of ~700 test files, a barrel module whose 128 real consumers are invisible next to 235 incidental index mentions, vitest run -- <paths> silently running all 22,939 tests while reporting a narrow scope, and six more.

Also in this PR, independent of any reduction:

• Argument-injection hardening. Targets come from a selector's stdout, so a file committed as --config=evil.ini would reach pytest as an OPTION. validated_targets() requires a plain relative path resolving to a real file inside the runner's root; -- is added for pytest and deliberately NOT for vitest.
• jscpd and test:electron become explicit floor entries. npm test ran them transitively via pretest; the cross-surface path runs only vitest, so without their own entries they would have vanished from the floor as a side effect of a speed change.
• Every hardcoded path is asserted to exist. A prefix that resolved to nothing (website/src/test/setup) hid the real vitest setup graph from broad-impact classification for four review rounds, because a dead path is indistinguishable from a working one.

4. What tests we did

• The reduction, on real diffs: a backend-only change makes the frontend gate run 146 cross-surface specs; a frontend-only change makes the backend gate run 350 cross-surface files; each surface runs its full suite when the diff touches it. All four combinations verified by probe on this checkout.
• Fail-closed: empty and unresolvable SCOPED_TESTS_BASE_REF both exit 2, never a silent reduction.
• Self-test (--test, wired into the floor ahead of its scans): asserts every hardcoded broad-impact path resolves on disk, that documentation is backend-owned rather than inert, that the cross-surface list arrives in each runner's own path space, that hostile targets (--config=evil.ini, -p no:randomly, ../outside.py, /etc/passwd, a non-existent file) cannot reach argv, and that the vitest argv carries no --.
• Parity suite: test/test_prepare_pr_profiles.py 33 passed (was 30). The two new floor assertions were mutation-verified red-then-green in an earlier revision: stripping SCOPED_TESTS_BASE_REF reddens the base-ref test, deleting the jscpd lane reddens the dropped-lane test.
• Floor gates on this diff, each exit code captured directly rather than through a pipe: self-test, flake8, black, isort, profiles suite, docs-lint, scrub-lint, brand-name -- all rc=0.

Not run locally, stated plainly: the full 62k backend suite. This PR's own diff touches the runner, which is a broad-impact path, so the gate correctly escalates itself to the full suite -- and running that serially is the cost this PR exists to reduce. CI runs it on the merge ref. Separately, test/test_acp_backend_kas.py::TestNoImportCycle fails on a clean checkout of this branch's base on this host (unshare(CLONE_NEWUSER) returns EPERM), unrelated to this diff.

5. Any other suggestions on the work

• The interesting half of the cost is still unaddressed and is now prepare-pr: import-graph test selector, to narrow within a surface #5303. This PR is the part that can be made sound without an import graph.
• gate-floor.md already documented an accepted asymmetry for check_per_file_coverage.py (its enforcement form needs a full coverage run, so it surfaces in CI rather than locally). The surface split is the same trade made explicit; it may be worth naming that pattern once rather than per-gate.
• Splitting the floor into setup[] and gates[] (prepare-pr profile: split setup[] from gates[] so provisioning is not a verdict #2599) would let npx playwright install chromium stop masquerading as a gate. Unrelated to this PR but adjacent.

Follow-up: #5303

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 0f99744dd863bf7cff8662a1287b1c7c7a47325c — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Coarse, fail-closed, CI-parity reduction with honest scoping — the unsound within-surface narrowing was removed rather than patched, which is the right call.

Suggestions

• surface_bucket() and the meta veto are hand-transcribed from ci.yml's paths-filter block; a later filter edit silently strands the local copy (broad-impact only escalates the PR editing ci.yml, not the ones after it merges). Pin the transcription with a parity assertion in test_prepare_pr_profiles.py — the same pattern that already pins gates[] to ci.yml — so a filter change reddens the copy instead of surfacing as a wasted review round. Harm is bounded (CI on the merge ref remains the authority), so this is a hardening, not a defect.

[DESIGN-REVIEWED] 0f99744

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 0f99744dd863bf7cff8662a1287b1c7c7a47325c and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 0f99744

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 0f99744dd863bf7cff8662a1287b1c7c7a47325c: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 0f99744dd863bf7cff8662a1287b1c7c7a47325c — this comment is updated in place on each push.

Review details

The candidate list contains no candidates, and my independent trace of run_scoped_tests.py confirms every decision branch fails open to the full suite: resolve_base → exit 2, changed_files/cross_surface_targets/validated_targets → SelectionUntrustworthy → full suite, and the reduction in plan fires only when the diff touches solely the other surface with no meta/broad-impact path. Referenced files (ci-surface-tests.py, integration/setup.ts, integration/mocks/server.ts, the setupFiles line) all exist. No wrong-skip path, no rule violation grounded.

No findings.

[OPUS-REVIEWED] 0f99744

Verdict parsed from the review's SHA-scoped output markers for commit 0f99744dd863bf7cff8662a1287b1c7c7a47325c.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 0f99744dd863bf7cff8662a1287b1c7c7a47325c: <one-sentence reason>

[chenmingwei23]

Addressed on 82943beecfb7170492e10ba8744f138fe0d9a080. Dispositions, one per finding:

GPT BLOCKING -- "scoped selector skips configured test roots" -- FIXED, and it was worse than reported.

setup.cfg declares testpaths = test src/kiro_crew/apps/builtins. Hardcoding REPO_ROOT / "test" was blind to 115 test_*.py files under the builtin-app testpath, and the frontend's web / "src" was blind to website/integration/*.integration.test.tsx (the suffixes matched; the scan never looked there). Roots are now read from configuration rather than transcribed, so a new testpath widens the scan without anyone remembering to, and an unparseable or missing root escalates instead of silently narrowing.

Proof the fix has an effect, not just a passing test: a change to src/kiro_crew/apps/builtins/auto_improvement/backend/clone_setup.py now selects 5 targets, 3 of them under src/kiro_crew/apps/builtins/auto_improvement/tests/ -- files the previous revision could not see at all.

Chasing that finding surfaced two more real defects in my own code, both now pinned by self-test assertions:

• Broad-impact matching had no boundaries. It was a bare substring test, so clone_setup.py matched the marker setup.py and escalated an ordinary module to the full suite. Now matched on file NAME (exact, or prefix for tsconfig*/vite.config*/requirements*) or on a path prefix. Same boundary bug as the bare-stem one, one layer up.
• Files were classified by location instead of role. src/kiro_crew/apps/builtins is a source tree AND a testpath, so "inside a configured root" was taken to mean "is a test file" -- which made every production module there look like a test helper. Classification is now by name.
• A deleted source module was silently skipped (it is absent from disk, so the "does it exist" guard dropped it). Deleting a module breaks every importer and is the widest blast radius there is, and an absent file cannot be scanned for, so it now escalates. A deleted test file is still just dropped -- there is nothing to run for it.

Design CONCERNS #1 -- "undocumented happy-dom revert" -- was a stale-base artifact, now gone. The branch was cut at fdd5b7c05, before #5252 landed the ^20.11.6 bump on main, so a diff against current main read the older pin as a deliberate revert. Nothing in this PR ever touched website/package.json. Rebased onto db34afc53; the diff is 5 files and no lockfile hunks.

Design CONCERNS #2 -- "the invariant holds only for zero-reference files" -- correct, and the docs were overclaiming. Docs fixed rather than the claim defended.

You are right that the trigger is zero references, not incomplete ones: when some test names a changed module the run narrows to those, so a test that breaks through a transitive import (test -> consumer -> changed module, never spelling the changed module) is not selected. gate-floor.md said "every doubt runs everything", which a text scan does not deliver. The heading and body now state the ceiling explicitly -- transitive edges and dynamic imports are both named as real limits, with the CI full suite on the merge ref as the backstop -- and a reverse-import closure is recorded as the obvious next step rather than smuggled into this PR.

I did not widen the escalation rule to cover it, because "escalate whenever references might be incomplete" is unfalsifiable from a text scan and collapses back into always running everything, which is the gate this replaces.

Verification on this revision: scoped self-test green (now covering all six escalation paths, the roots fix, the boundary fix, the role-vs-location fix and the deletion rule); test/test_prepare_pr_profiles.py 33 passed; black, isort, flake8, docs-lint, scrub-lint, brand-name all rc=0. mypy reports 4 errors in ops_mission_control/backend/providers/cloudwatch.py and transcribe.py -- zero overlap with this diff's 5 files, pre-existing stub drift.

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 0f99744dd863bf7cff8662a1287b1c7c7a47325c — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Verification done. Findings grounded; writing the review.

First-Principles-Verdict: CONCERNS

scripts/local-gate.py already classifies diffs into ci.yml's buckets and runs the cross-surface set — this ships a second transcription of the same mechanism without mentioning it.

What this change ships

Intent: stop paying the full 62k-test local suite on every prepare-pr iteration when the diff cannot affect it — a FIX for a measured cost.

1. Backend gate runs 350 cross-surface files for a frontend-only diff — justified
2. Frontend gate runs 146 cross-surface specs for a backend-only diff — justified
3. Gate now fails (exit 2) without a resolvable base ref — declared, justified
4. Runner self-test becomes its own floor entry — justified
5. jscpd becomes an explicit floor entry — declared, justified
6. Electron specs become an explicit floor entry — declared, justified
7. jscpd + Electron now run TWICE on any frontend-touching diff — undeclared, rides along
8. New run_scoped_tests.py — duplicate of scripts/local-gate.py's bucket/selector mechanism
9. Argument-injection hardening applied only to the new runner — symptom-level, sibling unfixed
10. ~100-line trap archive in gate-floor.md — justified

Watch

• Duplication, counted: scripts/local-gate.py (at base) transcribes the same three ci.yml buckets, calls the same ci-surface-tests.py, does the same website/ stripping and electron/ filtering — grep ci-surface-tests finds both under scripts/. Its bucket rules are pinned to ci.yml by test/test_local_gate.py; the new script's are pinned only to its own hardcoded self-test. Two local selectors can now drift from each other. The description's "no second selector here that can silently disagree with CI" is true against CI, false against the repo. The meaningful differences (fail-closed, per-surface invocation) are policy flags, not a second mechanism — judgment, hence CONCERNS not BLOCK.
• Unfixed siblings of the hardening: local-gate.py:201 and local-gate.py:223 splice the same selector stdout into pytest/vitest argv unvalidated — 2 sites carrying the exact exposure validated_targets() closes; its non--z porcelain parse (local-gate.py:115, strip('"')) is the quoting bug the new script documents.
• kirocrew-worktree-dev/SKILL.md:184-189 still says the pre-push floor is the full suite, "never skipped, never replaced" — this PR makes that same Rule 2 floor diff-scoped and updates only prepare-pr's copy; two shipped skills now disagree.

Subtractions

• Fold run_scoped_tests.py into scripts/local-gate.py (add --surface/fail-closed flags there) — one transcription of ci.yml's buckets instead of two, and test_local_gate.py's ci.yml drift-pin covers the gate for free.
• Shrink frontend_argv(None) from npm --prefix website test to npm --prefix website run test:website: package.json:19,42 make npm test = jscpd + vitest + electron, and jscpd/electron are now standalone gates, so the full-suite path runs both twice per iteration — in a perf PR.

[FIRST-PRINCIPLES-REVIEWED] 0f99744

[chenmingwei23]

Both blocking findings fixed on 1ec749d1e9096db2787cb97db05e5220924e9791, structurally rather than one branch each.

Root cause of this round AND the last one: the selector was a denylist. It asked "is this path one of the doubts I know about?" and silently skipped everything else. That is why each round surfaced a new spelling -- round 1 a second configured testpath, round 2 a .css file and an in-package fixture. Those shapes are not enumerable, so patching two more branches would just buy a round 3.

The question is now inverted into an allowlist: "is this a path I can positively prove I know how to map?" Four verdicts, and escalate is the default, so a file kind nobody anticipated runs the full suite instead of being ignored:

verdict	what qualifies
target	a test module by NAME (test_*.py, *_test.py, *.test.ts[x])
map	a .py under src//scripts/, or a .ts[x] under website/ -- things with an import spelling to scan for
ignore	prose and images, plus the other surface's files (the same split ci.yml's only_frontend/only_backend selectors use)
escalate	everything else

Per finding:

• website/src/index.css -> full suite. Now escalates, as you asked. Verified: the probe prints full suite: ... not a shape this selector can map and runs npm --prefix website test.
• code_review_sage/tests/fixtures.py -> full suite. The helper rule was keyed on a test/ prefix, so it saw the pure-test root and nothing else. It now triggers on any test/tests directory segment, which covers in-package fixtures anywhere in the tree.

A third defect your finding exposed, which you did not name and which was the more dangerous one: ALWAYS_ON was used to SEED the frontend target set, so the set was unconditionally non-empty and the "nothing in the diff maps to this surface" escalation could never fire on the frontend. A changed index.css mapped to nothing and the gate still reported a narrow, green, two-spec run. ALWAYS_ON is now unioned in LAST and emptiness is judged on the mapped set alone.

One thing I tried, measured, and reverted, recorded so it is not re-proposed. The obvious way to keep data files narrow is to match them by BASENAME. It is unsound: scoping on prepare-pr/profiles/kirocrew.json selected 38 test files, all of which genuinely say kirocrew.json -- meaning Kiro Crew's own config file, in a completely different directory. Tightening to parent/name does not rescue it either, because the one test that really reads that profile builds its path from a separate PROFILES_DIR constant and never spells the two segments together. Too loose to narrow on, too indirect to tighten, so data files escalate. This was the same bare-token mistake as the earlier bare-stem (621 of ~700 files) and bare-substring (clone_setup.py matching setup.py) ones; all three are now in gate-floor.md as a named trap class with their measured numbers.

Verification on this revision: scoped self-test green with new assertions pinning every classifier verdict, the in-package-helper rule, the css rule, the ALWAYS_ON masking bug and the bare-basename trap; test/test_prepare_pr_profiles.py 33 passed; black, isort, flake8, docs-lint, scrub-lint, brand-name all rc=0. Behaviour probes on this head: index.css -> full suite, profile JSON -> full suite, acp/runtime.py -> 22 targets, one component -> 4 targets.

On the SAST (Semgrep) red: its own log shows the failure is in the Resolve diff base (merge-base) step -- ::error::Could not resolve the merge-base of $BASE_TIP and $HEAD_SHA (this job needs fetch-depth: 0) -- with the rule tests themselves scanning cleanly. That is infra racing a force-push, not a finding, so I am re-running it rather than changing code for it.

[chenmingwei23]

Both blocking findings fixed on c32092d08e71bb2f497da6abdbb5825f0592e89e.

"Packaged text changes can bypass their contract tests" -- correct, and this PR is its own counterexample. The inert rule keyed on the extension (.md/.txt/.rst anywhere), but inertness is a claim about the PATH: .md under docs/ is prose, while .md under src/ is packaged skill content with contract tests behind it. This repo's own test_gate_rationale_reference_exists_and_is_pointed_at() reads gate-floor.md and asserts the skill points at it -- so on the previous revision a changed SKILL.md was ignored and the test guarding it was dropped, on a PR whose entire purpose is changing those two files.

Inertness is now a narrow path allowlist (docs/, website/docs/, .github/ISSUE_TEMPLATE/) intersected with a prose/image suffix list. Everything else escalates, including packaged SKILL.md, a root README.md, and an importable website/src/assets/*.svg (inert in docs/, importable under src/).

"Rename handling loses the deleted source path" -- correct, and it defeated a fallback that already existed. Keeping only the -> right-hand side hid the old path's disappearance, so a module rename with a stale importer mapped cleanly to the new name and never reached the deletion escalation -- the scoped run passed while the full suite would fail at collection. Both endpoints are now collected: git diff --no-renames reports a rename as delete plus add, and the porcelain old -> new form keeps both sides for older gits that still emit it.

Verified with a real rename rather than an assertion alone: git mv src/kiro_crew/acp/runtime.py runtime_renamed.py now prints full suite: src/kiro_crew/acp/runtime.py was deleted; every importer of it is in scope .... On the previous revision that narrowed.

Two mistakes of my own from this round, recorded because one of them is in this repo's own gate rules.

The Brand Name Gate red was mine: a comment I added said the brand in joined form. It went out because I verified it with python3 scripts/check_brand_name.py | grep ... ; echo rc=$?, which captures grep's status, not the script's -- exactly the "Check exit codes, never piped output" rule in gate-floor.md, which I then broke while editing that same file. Every gate on this revision was re-run redirecting to a file and testing $? directly: self-test, profiles suite (33 passed), black, isort, flake8, docs-lint, scrub-lint, brand, harness-parity, focus-cue, changelog-history -- all rc=0.

SAST (Semgrep) failed again on the same infra step (Resolve diff base (merge-base) needing fetch-depth: 0), not on a rule. The rule tests themselves scan clean in the log. Re-running rather than changing code.

Standing note on the shape of this review, since this is round 3 of the same class. Rounds 1-3 were all one root cause -- the classifier enumerated the doubts it knew about and skipped the rest -- and round 2 inverted it into an allowlist for exactly that reason. Rounds 1 and 2 were genuine holes in the old denylist. This round's two are different in kind: both are inside the allowlist, in the two places where a positive claim was too generous (an extension standing in for a path, a rename standing in for one path instead of two). That is the expected convergence shape rather than a new family, and each is now pinned by an assertion, but if a round 4 lands another "the positive claim was too generous" finding I would rather split the remaining tightening into a follow-up than keep growing this diff.

[chenmingwei23]

Correction: I was wrong about the Semgrep red, twice. I called it infra in two earlier comments on the basis of a ::error::Could not resolve the merge-base ... line in the log. That line is the command text of the step's own script, echoed by the runner, not an error the step emitted. The step that actually failed is Semgrep scan (diff only), exit 1, with a genuine finding. Fixed on c32092d08's successor below.

The finding: python.lang.security.audit.dangerous-subprocess-use-tainted-env-args on scripts/run_scoped_tests.py -- subprocess.run(cmd, ...) reached with data that traces back to an environment variable.

It is not command injection: argv is always a list and shell=True is never used. It is argument injection, which is still real. Targets are derived from git diff output, so a file committed as --config=evil.ini would arrive at pytest as an OPTION rather than a path, and a test runner's own flags are quite enough to do damage.

Fix: every target now passes validated_targets() before it can reach argv -- it must start with an alphanumeric, contain no .. traversal, stay relative, and resolve to a real file inside the runner's root. A target that fails any of those raises, and the caller turns that into a full-suite run like any other doubt rather than an error. Hostile inputs (--config=evil.ini, -p no:randomly, ../outside.py, /etc/passwd, and a non-existent file) are pinned as self-test assertions on both builders.

One thing that fix broke, caught by running it rather than trusting it. I also added a -- separator to both runners. For pytest that is correct. For vitest it is actively harmful: vitest run -- <paths> silently stops treating the positionals as filters and runs the WHOLE suite. Measured: 1474 spec files / 22,939 tests executed while the gate still printed scoped: 4 target(s), so the report disagreed with the run -- safe, but a total loss of the PR's purpose and a lying status line. -- is now on pytest only, absent from vitest, and a self-test assertion pins its absence with that measurement as the reason. validated_targets is the real protection on both surfaces anyway, and is strictly stronger than --: it rejects a leading - outright instead of asking the runner to stop parsing.

Re-verified after the fix: frontend scoped run back to 4 files, backend scoped run 22 files / 1012 tests with -- present and filtering correctly.

One base-owned red worth flagging, not mine: test/test_acp_backend_kas.py::TestNoImportCycle::{test_config_loader_imports_alone,test_the_backend_normalizer_works_in_that_interpreter} fail on a completely clean checkout of this branch's base with an empty git status and no probe applied -- they spawn a subinterpreter and import, on a host where unshare(CLONE_NEWUSER) returns EPERM. They are unrelated to this diff; I am not chasing them here.

Gates on this revision, each with its exit code captured directly rather than through a pipe: self-test, profiles suite (33 passed), black, isort, flake8, docs-lint, scrub-lint, brand-name -- all rc=0.

[chenmingwei23]

All three round-6 findings are real. I verified each against the tree rather than accepting the description, and the numbers are worth stating because they are the argument:

1. Relative imports. src/kiro_crew/apps/builtins/auto_improvement/backend/{commit,ledger_admin,profile_normalize}.py import a sibling with from .store import .... My token set is absolute-only, so when a test exercises the sibling rather than the changed module, nothing matches it.
2. Barrel modules. website/src/store/index.ts exists. 128 specs import ../store or @/store; 235 specs merely contain the word index. So a barrel change maps to an arbitrary 235-file set that is simultaneously too wide and missing the 128 that actually consume it.
3. Tested documentation. test/test_build_target_parity.py exists and reads docs/. My claim that docs/** is inert is simply false.

I am pausing rather than fixing these, because the pattern is now a design verdict rather than a defect list. Six rounds, nine findings, zero false positives -- and every one is the same impossibility: a text reference scan cannot enumerate the ways a test can reach a module. The spellings found so far are absolute import, relative import, barrel re-export, in-package fixture, global vitest setup, data-file read, cross-surface parity comparison, and documentation contract. There is no reason to believe that list is finished.

Each suggested remedy also shrinks the allowlist -- "or fall back to the full suite", "escalate index.ts[x]", "remove tested documentation trees from inert". Extrapolate and the allowlist converges on "escalate everything", at which point the gate IS the full suite and the change has bought nothing. Grinding a seventh round makes the diff bigger while walking toward that fixed point.

What in this PR is sound independently of reference scanning, and worth keeping:

• The surface split, using CI's own selector. A frontend-only diff runs 350 cross-surface backend files instead of the full backend suite; a backend-only diff runs 146 cross-surface specs instead of all 1444. This is sound by construction because it is scripts/ci-surface-tests.py -- the same script and the same post-processing ci.yml uses -- not a second answer invented here.
• Broad-impact escalation and the fail-closed base handling, which can only ever make a run wider.
• The argv hardening (validated_targets, -- on pytest only), which is a real security fix and independent of any narrowing.

What is not sound and generated all nine findings: the per-file reference scan that narrows within a surface. Making that sound needs a real import graph -- Python AST plus a TS resolver that follows barrel re-exports -- which is a different and much larger piece of work than this PR.

So the proposal is to reduce this PR to the sound subset (surface split + escalation + hardening, dropping the within-surface reference narrowing), and file the import-graph selector as a follow-up. That keeps a real, measured win for the common single-surface PR with no soundness argument left, and it costs the reduction on mixed-surface diffs. Holding for a maintainer decision before I touch it further.

[chenmingwei23]

Fixed on 1fede7f2b98e77c9e88c9a88ea90c69000d38f44. This one is in scope -- it is a surface-classification bug, not another instance of the reference-scan impossibility that #5303 now owns -- and chasing it produced the right structural answer.

"CI meta paths incorrectly reduce the frontend suite" -- correct, and broader than the two paths named. I verified rather than assumed: website/src/test/frontendBlobReconcile.wireFormat.test.ts exists and reads .github/, renderVerdict.test.ts does too, and several i18n and settings specs (catalogParity, settingsRegistry, contextSidecar, ...) read docs/ or scripts/. So "everything outside website/ is backend" was wrong as a general rule, not just for those two files.

Rather than broaden two prefixes by hand, I transcribed ci.yml's changes job, which is the authority for this question and already answers it. Its three buckets are frontend: website/**, meta: .github/** scripts/**, and backend: ** minus the other two -- and it disables BOTH reductions whenever meta is touched. My classifier had folded meta into backend, which is exactly the bug. The veto is now implemented the same way and for the same reason, so this class cannot recur without CI's own logic being wrong too.

That also settles the docs/ question in the other direction: ci.yml puts docs/ in the backend catch-all, so a docs-only diff is only_backend for CI as well. Parity with CI is the standard this PR set, so it is parity here too rather than a private judgement.

Verified by probe on all four combinations: a .github/scripts/*.mjs change now reports full suite: the diff touches CI meta paths ... on both surfaces, while a src/ change still reduces the frontend gate to 146 specs and a website/ change still reduces the backend gate to 350 files. Nine bucket assertions pin each classification, including the corollary that this runner lives under scripts/ and therefore disables its own reduction.

On the Semgrep red, which was real again and was my own regression. When I rewrote the runner I split the subprocess.run(...) call across lines, which moved Semgrep's reported line onto the ARGUMENT line -- so the # nosemgrep annotation, still sitting on the preceding line, no longer suppressed anything. The substantive protection never changed (validated_targets()); only the annotation's placement was wrong. It is back on one line following this repo's own precedent (narrate.py:237 uses the same # noqa: E501 # nosemgrep: <rule> form), with the justification as a comment block above and a note that the annotation must sit on the line Semgrep reports.

Gates on this revision, each exit code captured by redirecting to a file and testing $? rather than through a pipe: self-test, flake8, black, isort, profiles suite (33 passed), docs-lint, scrub-lint, brand-name -- all rc=0.

[chenmingwei23]

All three review lanes went clean on 1fede7f2b (Design PASS, GPT no blocking, Opus no blocking) with zero red checks. I have taken the one non-blocking GPT finding anyway, on 36862843c1bb8ee5e8ffa98edccfcdb296d54b6a, because surface classification is now the entire mechanism of this PR and a misclassification there silently skips a full suite.

"Git C-quotes hostile filenames" -- correct. Without -z, git renders a path carrying a non-ASCII byte, an embedded quote or a newline as "website/src/f\303\251e.tsx", leading double-quote included. That fails a startswith("website/") test, so a frontend-only change would have been bucketed backend and the frontend full suite skipped -- the exact failure direction this gate must never take. The previous revision also stripped quotes by hand while splitting the porcelain old -> new form, which is the kind of ad-hoc unquoting that works until a filename contains a space or an arrow.

Both git reads are now NUL-delimited (git diff --name-only --no-renames -z and git status --porcelain --untracked-files=all --no-renames -z), and parsing is split out of the git call into _parse_diff_z() / _parse_status_z() so the hostile shapes can be asserted without creating such files in the tree. Eight assertions cover it: a non-ASCII path and a newline-bearing path survive both parsers verbatim, both classify as frontend, the porcelain parser strips only the 3-character prefix, and no parsed path ever begins with a quote the -z form never added.

Behaviour re-verified unchanged after the rewrite: a src/ change still reduces the frontend gate to 146 specs, a website/ change still reduces the backend gate to 350 files, and a .github/workflows/ change still escalates both.

Gates on this revision, each exit code captured by redirecting to a file and testing $?: self-test, flake8, black, isort, profiles suite (33 passed), docs-lint, scrub-lint, brand-name -- all rc=0.

Note for the lanes: Opus's comment is still stamped d229c52b, i.e. it last ran before the scope reduction, so its clean verdict predates the current mechanism. This push re-triggers it.

[chenmingwei23]

Fixed on 0f99744dd863bf7cff8662a1287b1c7c7a47325c.

"Native Windows cannot launch the npm command shims" -- correct, and it is specific to this PR rather than inherited. The floor's other npm entries (npm --prefix website run build, npx tsc -b) are run by the loop through a shell, which resolves npm.cmd for you. This runner is the first thing in the floor to exec npm from Python with shell=False, where a bare npm does not exist on native Windows and the gate would die with FileNotFoundError before a single spec ran.

I did not take the suggested remedy literally. Hardcoding npm.cmd/npx.cmd under os.name == "nt" fixes that one case; shutil.which() is what this repo already does (mcp_gateway/resolve_once.py:641 resolves npm exactly that way) and additionally covers npm.exe, nvm and Volta shims, and the genuinely-missing case. That last one matters: a missing npm now produces npm is not on PATH, so the frontend suite cannot be launched and exit 2, rather than an exception from inside subprocess. The fallback path needed hardening for the same reason -- if the reduced argv cannot be built and the full-suite argv cannot be built either, that is now a named exit 2 instead of a second raise escaping main().

Verified end to end rather than by assertion alone: a backend-only diff drove the frontend gate through the resolved launcher and the cross-surface run completed green -- 147 passed (147) test files, rc=0. The self-test compares the launcher by BASENAME and additionally asserts it is an absolute path, so a regression back to a bare name fails locally.

Gates on this revision, each exit code captured by redirecting to a file and testing $?: self-test, flake8, black, isort, profiles suite (33 passed), docs-lint, scrub-lint, brand-name -- all rc=0.

Opus is still stamped d229c52b, from before the scope reduction; this push re-triggers it.