perf: speed up test suite across inner loop, full suite, and CI#1385
Merged
Conversation
…shot - ci-test-shard.mjs: --write-timings aggregation into scripts/test-timings.json (bucketed, newer-snapshot-protected, corrupt-shard tolerant) and --cold-start-probe; CI shard invocations emit vitest json timings - test-changed.mjs: structured mode/reason telemetry line (+ --print-mode) - pr-checks.yml: upload per-shard timing artifacts - baseline: docs/test-speed-baseline-2026-06-03.md (core 41s, engine 179s, cli 49s; cold-start ~1.3-1.8s/process => U8 gate: worthwhile-not-urgent)
…d inventory harness - 427 orphaned dashboard test files ran in NO gate; 395 now gated via self-maintaining backfill lanes (glob minus curated minus skip-list), 31 pre-existing failures + build-output skip-listed with reasons - settings -t name-filter lanes replaced by one unfiltered lane (describe blocks can no longer fall through filters) - scripts/check-test-inventory.mjs: --capture/--diff superset harness + --dashboard-curated completeness guard - pr-checks.yml: engine-slow CI gate (non-empty assertion) + inventory guard job - docs/testing.md: guard, skip-list policy, harness usage
…h runs - skill-sync check conditioned on content hash of its inputs (skips ~0.3s spawn) - ensure-test-artifacts: git-blob content-hash staleness; branch switches no longer trigger spurious ~2.6s tsc rebuilds (mtime fallback when dirty) - isolation guard: cheap --before-fast reusing prior post-run baseline (~2.1s -> ~0.07s); detection proven preserved via injected-leak failure test - vitest-setup: CI skips 4040-4045 discovery probe unless FUSION_RESERVED_PORTS set; kill-guard wrapper untouched, asymmetry pinned by port-probe-policy tests - cache-fresh fast path skips sync/artifacts/HOME-prune entirely (mode line: fast-path=cache-fresh)
- cache key v2 = own hash + sorted transitive workspace dep hashes + shared inputs (lockfile, tsconfig.base, core __test-utils__ tree) - working-tree-dirty files hashed by content (fixes false cache HIT on unstaged edits) - core __test-utils__ folded globally: 16+ packages import it without a workspace dep on core - dep folding adds ~5ms to the inner loop (memoized own-hashes) - docs: cache semantics, --no-cache / FUSION_TEST_NO_CACHE, TTL rationale
…t4 deprecation delta zero
…tion - shards weighted by scripts/test-timings.json durations (median-duration fallback + warning for untimed) - dashboard no longer --shard-sliced (broken across its script chain): 14 lanes enumerated from package.json and scheduled as separately-weighted units, each exactly once - engine/core keep --shard slicing, duration-weighted - estimated critical-path shard: ~235s -> ~159s (spread 96.7% -> 0.06%) - 30-day staleness warning + --check-timings-staleness for a scheduled refresh job; --dry-run mode
…ord U7 triage findings - routes-planning-tracking: replace vi.waitFor polling (1s real-timer race vs fire-and-forget promise chain) with signalOnCall deferred resolution; assertions unchanged, mutate-to-prove verified - top-time offenders documented keep-as-is: all are real-SQLite/real-git/spawned-process integration where the slowness IS the subject (FN-5048 keep class); demotion rejected (would churn inventory testIds)
…PR (evidence recorded)
- content-hash: createRepoContentSnapshot — 2 repo-wide git spawns shared across all hash computations (was ~2 spawns x N packages x 2 passes, ~0.6-1.6s per cache-miss run); snapshot-equivalence test pins zero-spawn path - test-changed: one hash memo + snapshot shared between applyCacheToPlan and recordCachePass (record pass now re-hashes nothing) - ci-test-shard: listPackageTestFiles single source of truth for the test-file glob (was triplicated) - check-test-inventory: curatedProjects defaults to projects (removes duplicated 11-entry list in spec) - ensure-test-artifacts: drop detectMissingArtifacts passthrough alias - drop dead statSync re-export; cross-reference comments on the two shared-input path lists Skipped deliberately: --cold-start-probe/--check-timings-staleness removal (plan artifacts for U8 re-eval + scheduled refresh), best-fit dedup + threshold² (behavior-preserving refactor of verified shard math — follow-up), worker-budget duplication (pre-existing on main)
…tignore the pattern
- laneShardFraction: chained --shard invocations sum to full project weight (api backfill was half-weighted) - exclude *.slow.test.* from duration weighting (engine carried 75 phantom untimed files) - remove dead ENGINE_PACKAGE_NAME export - docs: --cold-start-probe, --inputs-dir (snapshot refresh now self-contained), --print-mode
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR implements a measurement-first test speedup: adds git-backed content hashing and per-package artifact cache, duration-weighted CI sharding with dashboard lane units and per-shard .timings reporter outputs, a dashboard inventory curated-gate (with skip-list and guard), engine-slow nonempty CI gate, test sync/timeout consolidations, and supporting docs/tests/workflow changes. ChangesTest Suite Speedup and CI Gating
Sequence DiagramsequenceDiagram
participant CI as CI Shard Job
participant Cache as Cache Layer
participant Timing as Timing Snapshot
participant Shard as Shard Planner
participant Test as Vitest Execution
participant Report as Reporter Output
CI->>Cache: Pre-build dist hash
Cache-->>CI: Cache hit/miss
CI->>Cache: Seed artifact cache (if hit)
CI->>Timing: Load planning timings
Timing-->>CI: Per-file durations
CI->>Shard: buildScheduleUnits (packages + lanes)
Shard->>Shard: computePackageDurationWeight (untimed fallback)
Shard->>Shard: buildDashboardLaneUnits (lane distribution)
Shard-->>CI: Schedule units (weight, splittable, lane)
CI->>Shard: planShardAssignments
Shard-->>CI: Shard entries (preserve lane)
CI->>Test: Run per-command with --reporter=json
Test->>Report: Write .timings/timings-shard-N.json
Test-->>CI: Test results
CI->>Timing: buildTimingsSnapshot (aggregate, bucket)
Timing->>Timing: writeTimings (staleness check)
CI->>Report: Upload timing artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Contributor
Greptile SummaryThis PR overhauls the test infrastructure for speed and correctness: it adds duration-based shard balancing (replacing file-count weights), a dep-aware/dirty-aware content hash cache for artifact builds, a committed timing snapshot, and closes the dashboard curated-gate coverage hole by adding backfill vitest projects and a guard CI job. The flaky
Confidence Score: 4/5Safe to merge with one open question: two non-slow engine reliability tests are deleted without a clear record of where their coverage went in this diff. The infrastructure changes (content-hash cache, shard planner, backfill gate, timing telemetry) are well-designed and internally consistent. The deletion of branch-group-promotion.test.ts and shared-group-member-integration.test.ts — both non-slow, both using real-git operations — without a visible absorption target in this diff is the one open question. If those scenarios were failing on main or are demonstrably covered by existing slow files, the PR is clean. The remaining known residuals are all acknowledged and accepted by the author. packages/engine/src/tests/reliability-interactions/branch-group-promotion.test.ts and shared-group-member-integration.test.ts (deleted) — confirm coverage disposition before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[pnpm test:ci:shard --shard N --total 4] --> B[ci-test-shard.mjs main]
B --> C[buildScheduleUnits]
C --> D{pkg == dashboard?}
D -- Yes --> E[buildDashboardLaneUnits]
E --> F[enumerateDashboardLanes: leaf script walk]
F --> G[resolveDashboardProjectFiles: tsx --eval]
G --> H[laneProjectNames + laneShardFraction]
H --> I[Per-lane weight units, splittable=false]
D -- No --> J[computePackageDurationWeight]
J --> K[listPackageTestFiles + sumFileDurations]
K --> L[weight = timed + untimed x median]
I --> M[planShardAssignments]
L --> M
M --> N[computeSplitPlan: duration weights]
N --> O[buildShardCommands]
O --> P1[plain: pnpm --filter pkgs test timingFlags]
O --> P2[virtual: pnpm --filter pkg test --shard=i/n timingFlags]
O --> P3[dashboard-lane: pnpm --filter dashboard run lane timingFlags]
P1 --> Q[vitest JSON reporter → .timings/]
P2 --> Q
P3 --> Q
Q --> R[upload artifact: test-timings-shard-N]
R --> S[--write-timings: merge into scripts/test-timings.json]
S --> C
Reviews (7): Last reviewed commit: "test(dashboard): align orphaned pr-push-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+631
to
+660
| } | ||
| process.stdout.write(JSON.stringify(out)); | ||
| `; | ||
| const tsxBin = path.join(projectRoot, "node_modules/.bin/tsx"); | ||
| const result = spawnSync( | ||
| tsxBin, | ||
| ["--eval", script], | ||
| { cwd: dashboardAbs, encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }, | ||
| ); | ||
| if (result.status !== 0 || !result.stdout) { | ||
| return null; | ||
| } | ||
| let parsed; | ||
| try { | ||
| parsed = JSON.parse(result.stdout); | ||
| } catch { | ||
| return null; | ||
| } | ||
| const out = {}; | ||
| for (const [name, files] of Object.entries(parsed)) { | ||
| out[name] = (Array.isArray(files) ? files : []).map((f) => `${dashboardDir}/${f}`.split(path.sep).join("/")); | ||
| } | ||
| return out; | ||
| } | ||
|
|
||
| /** | ||
| * Build the dashboard lane schedulable units. Each enumerated leaf lane becomes | ||
| * one unit weighted by the durations of the files its `--project`s execute | ||
| * (a lane carrying `--shard=i/n` runs 1/n of those files). When the config | ||
| * cannot be imported, the package duration is apportioned evenly across lanes. |
Contributor
There was a problem hiding this comment.
&&-chained lane scripts
buildShardCommands appends timingFlags() to the pnpm run <lane> invocation. For test:quality:api:backfill — which is the shell string node cmd1 ... --shard=1/2 && node cmd2 ... --shard=2/2 — pnpm appends extra args to the tail of the script string, so --reporter=json --outputFile.json=<path> is only received by the --shard=2/2 invocation. The first half's per-file timings go unrecorded, and those files fall back to the median-weight estimate in every subsequent shard-planning run.
The app-backfill avoids this because test:quality:app:backfill delegates to four separate leaf scripts (backfill-1 … backfill-4) via pnpm run, each of which receives the timing flags individually. Structuring the API backfill the same way (two leaf scripts + a delegating parent) would fix this, as would running the two halves in the leaf script with explicit --reporter/--outputFile flags baked in rather than relying on pnpm arg-forwarding.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/ci-test-shard.mjs (1)
24-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound these subprocesses with async
exec+ timeout.All three call sites are synchronous and unbounded today. A hung
pnpm/tsxchild will stall shard planning or the whole CI shard indefinitely instead of failing fast.As per coding guidelines, "Run user-configured commands (test/build/workflow scripts) via async
execwith timeout, notexecSync.execSyncis only acceptable for short deterministic git plumbing."Also applies to: 635-639, 1046-1060
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci-test-shard.mjs` around lines 24 - 34, The run function currently uses synchronous spawnSync (spawnSync) which can hang; replace it with an async implementation using child_process.exec or spawn wrapped in a Promise with a configurable timeout (use the provided options to accept a timeout ms), ensure stdout/stderr are inherited or streamed, and on timeout or non-zero exit resolve/reject so the caller exits with the correct code (use process.exit(code ?? 1) on failure); update the run function signature to return a Promise and update all call sites that invoke run (and the other synchronous blocks that use spawnSync/execSync at the other locations noted) to await run so CI shards fail fast on hung pnpm/tsx processes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-checks.yml:
- Around line 108-119: The new job test-inventory-guard currently checks out the
repo and runs a PR-controlled script (node scripts/check-test-inventory.mjs)
with default token permissions; update the actions/checkout@v4 step(s) to set
persist-credentials: false and scope the workflow job's GITHUB_TOKEN to the
minimum read-only permissions (e.g., permissions: contents: read) at the job
level, and apply the same changes to the other similar job introduced later (the
job covering lines 126-139) so both checkout steps do not leave write-capable
credentials in the workspace.
In `@packages/dashboard/src/__tests__/dashboard-test-config-guard.test.ts`:
- Around line 60-68: The test dashboard-test-config-guard.test.ts currently only
asserts that scripts["test:quality:app:settings"] does not contain "-t ", which
misses Vitest long-form flags; update the assertion for the test in the "runs
the settings lane..." it block (using readDashboardPackageJson and
scripts["test:quality:app:settings"]) to reject all Vitest name-filter flags
instead of just "-t" — e.g. assert the script string does not contain "-t" and
also does not contain "--testNamePattern" or "--test-name-pattern" (or
alternatively split the script into argv tokens and assert none of the tokens
equal any of those flags) so any long-form or short-form name filters fail the
test.
In `@packages/dashboard/src/__tests__/routes-planning-tracking.test.ts`:
- Around line 216-218: The test currently uses await Promise.resolve() which
only flushes a single microtask and can miss asynchronous work after
createIssueSpy; replace that with an explicit signal that the background hook
path has completed (e.g., add a Deferred or Promise that the hook resolves at
the end of its post-createIssue work) and await that in the test before
asserting expect(createIssueSpy).toHaveBeenCalledTimes(1); locate the async
post-dispatch logic tied to createIssueSpy and modify the test harness to
expose/await the completion promise (or use an in-memory fake/deferred hook
completion) so the assertion runs only after all awaiting in the hook finishes;
apply the same pattern to the other similar assertions referencing
createIssueSpy in this test file.
In `@scripts/__tests__/test-changed.test.mjs`:
- Around line 946-1000: The subprocess helpers currently use spawnSync with no
timeout and allow user git hooks/signing to interfere; in git() and runInRepo()
add a bounded timeout (e.g. options.timeout = 5000) to the spawnSync calls and
pass a sanitized env for git/child processes (inherit process.env but set
GIT_CONFIG_NOSYSTEM=1 and any other needed keys) and, for commits, add git
config overrides to disable signing/hooks (e.g. include "-c",
"commit.gpgSign=false", "-c", "core.hooksPath=/dev/null" in the git commit args
in function git) so commits cannot hang and node subprocesses are time-bounded;
update all spawnSync usages referenced (git(...) and the spawnSync in runInRepo)
accordingly.
In `@scripts/assert-engine-slow-nonempty.mjs`:
- Around line 25-38: The current synchronous spawnSync call using spawnSync to
run "pnpm exec vitest" (building the args array that includes
"--outputFile=${outputFile}" and uses { cwd: engineDir, stdio: "inherit", env: {
...process.env } }) must be replaced with an asynchronous exec/execFile-based
invocation that supports a timeout; call a promisified
child_process.exec/execFile (or util.promisify) to run the same command/args,
pass cwd and env as before, set the timeout option to a sensible CI limit,
capture stdout/stderr and write to the same outputFile when the promise
resolves, and reject/kill the child and fail the task if the timeout is reached
so the CI job cannot hang (update any callers that expect the previous return
value to await the new async function).
In `@scripts/check-test-inventory.mjs`:
- Around line 240-250: The function listExecutedDashboardQualityFiles currently
always calls loadSpec() with no args so --spec is ignored; update
listExecutedDashboardQualityFiles to accept a spec path (e.g., add parameter {
spec } or { specPath } to its signature) and pass that through to loadSpec(spec)
instead of calling loadSpec() with no args, then update all callers (the other
curated-guard path noted around the 307-315 block and any invocation sites that
get parseArgs() output) to thread the parsed spec value from parseArgs() into
listExecutedDashboardQualityFiles so the requested spec file is actually used.
In `@scripts/check-test-isolation.mjs`:
- Around line 191-215: recordBaselineFast currently carries forward every path
from readPreviousBaseline().unstableProtectedDirs without revalidating them,
allowing a formerly-active .fusion root to be permanently ignored; update
recordBaselineFast to re-check each previous dir with isFusionEngineActive (and
optionally confirm it still appears in snapshotProtectedFusion()) and only
include it in unstableProtectedDirs if the check returns true, otherwise drop it
so subsequent checkAgainstBaseline() will validate real writes; use the existing
latestProtected and isFusionEngineActive(entry.dir) logic to perform this
revalidation before writing BASELINE_FILE.
In `@scripts/ci-test-shard.mjs`:
- Around line 1044-1074: The probe ignores child exit status and missing/invalid
JSON, allowing a failed Vitest run to yield testDurationMs=0; update the
runColdStartProbe flow to check the spawnSync result and the JSON
parse/contents: after calling spawnSync (the pnpm ... vitest invocation),
inspect the returned object for non-zero status or signal and, if it failed,
throw or return an error so the cold-start probe fails fast; also validate that
readFileSync(outputFile) exists and that extractFileDurations(JSON.parse(...),
packages, projectRoot) returns non-empty timings (use outputFile and the
extractFileDurations call as the check) and treat missing/empty timings as a
failure rather than silently using testDurationMs=0, propagating the error so
the baseline is not poisoned.
In `@scripts/ensure-test-artifacts.mjs`:
- Around line 320-330: The boolean useContentCache currently only checks runFn
=== run or runOptions.artifactCache, so providing only runOptions.gitFn doesn't
enable the content-hash cache; update the condition that defines useContentCache
to also consider runOptions.gitFn (e.g. check runOptions.gitFn !==
undefined/null) so injected runners that supply just gitFn will take the new
cache path; keep the existing cacheOptions construction (artifactCache:
runOptions.artifactCache ?? readArtifactCache(resolvedRootDir), gitFn:
runOptions.gitFn ?? defaultGitRunner) unchanged.
In `@scripts/lib/content-hash.mjs`:
- Around line 111-117: The snapshot currently treats gitFn returning null as an
empty repo; update createRepoContentSnapshot to detect when gitFn(...) returns
null for either ls-files or status calls and return a clearly unavailable result
(e.g., null or an object like { unavailable: true }) instead of constructing
trackedByPath/dirtyPaths/untrackedPaths; specifically check the results of gitFn
used by parseLsFiles and parseStatus and bail out early, and ensure callers
(e.g., scripts/test-changed.mjs and scripts/sync-fusion-skill-tools.mjs) treat
this unavailable return as a cache miss.
In `@scripts/test-changed.mjs`:
- Around line 173-193: pruneFusionTestHomes currently deletes the first matching
fusion-test-home-root-* directories it finds which can remove another concurrent
test's live HOME; update pruneFusionTestHomes to only remove directories that
are proven stale by (1) resolving realpathSync(rawPath) to avoid symlink tricks,
(2) statSync(rawPath) and require the directory mtime (or ctime) be older than a
defined STALE_THRESHOLD_MS constant, and (3) optionally check for a run-specific
marker (e.g., a PID or lock file inside the dir) and verify that PID is not
running before rmSync; apply these checks in the loop (using tmpEntries,
rawPath, removed, maxEntries) and skip deletion unless all stale/ownership
checks pass.
---
Outside diff comments:
In `@scripts/ci-test-shard.mjs`:
- Around line 24-34: The run function currently uses synchronous spawnSync
(spawnSync) which can hang; replace it with an async implementation using
child_process.exec or spawn wrapped in a Promise with a configurable timeout
(use the provided options to accept a timeout ms), ensure stdout/stderr are
inherited or streamed, and on timeout or non-zero exit resolve/reject so the
caller exits with the correct code (use process.exit(code ?? 1) on failure);
update the run function signature to return a Promise and update all call sites
that invoke run (and the other synchronous blocks that use spawnSync/execSync at
the other locations noted) to await run so CI shards fail fast on hung pnpm/tsx
processes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15384f6c-242d-45b3-ad98-bf97fb91ef04
📒 Files selected for processing (32)
.github/workflows/pr-checks.yml.gitignoredocs/plans/2026-06-03-001-perf-test-suite-speedup-plan.mddocs/test-speed-audit-FN-5048.mddocs/test-speed-baseline-2026-06-03.mddocs/testing.mdpackages/core/src/__test-utils__/port-probe-policy.tspackages/core/src/__test-utils__/vitest-setup.tspackages/core/src/__tests__/port-probe-policy.test.tspackages/dashboard/package.jsonpackages/dashboard/src/__tests__/dashboard-test-config-guard.test.tspackages/dashboard/src/__tests__/routes-planning-tracking.test.tspackages/dashboard/vitest.config.tsscripts/__tests__/check-test-inventory.test.mjsscripts/__tests__/check-test-isolation.test.mjsscripts/__tests__/ci-test-shard-timings.test.mjsscripts/__tests__/ci-test-shard.test.mjsscripts/__tests__/content-hash.test.mjsscripts/__tests__/ensure-test-artifacts.test.mjsscripts/__tests__/skill-sync-cache.test.mjsscripts/__tests__/test-changed.test.mjsscripts/assert-engine-slow-nonempty.mjsscripts/check-test-inventory.mjsscripts/check-test-isolation.mjsscripts/ci-test-shard.mjsscripts/ensure-test-artifacts.mjsscripts/lib/content-hash.mjsscripts/lib/dashboard-curated-skiplist.jsonscripts/lib/test-inventory-spec.jsonscripts/sync-fusion-skill-tools.mjsscripts/test-changed.mjsscripts/test-timings.json
Comment on lines
+108
to
+119
| test-inventory-guard: | ||
| name: Dashboard curated-gate guard | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js and pnpm | ||
| uses: ./.github/actions/setup-node-pnpm | ||
|
|
||
| - name: Assert every dashboard test file is gated or skip-listed | ||
| run: node scripts/check-test-inventory.mjs --dashboard-curated |
Contributor
There was a problem hiding this comment.
Harden the new jobs' GITHUB_TOKEN handling.
Both new jobs check out the repo and then execute PR-controlled Node scripts, but they inherit default token permissions and keep checkout credentials in the workspace. Please scope these jobs to the minimum read permissions and set persist-credentials: false on the new checkout steps.
🔒 Suggested hardening
test-inventory-guard:
name: Dashboard curated-gate guard
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- name: Checkout
uses: actions/checkout@v4
+ with:
+ persist-credentials: false
@@
test-slow:
name: Engine slow tier
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- name: Checkout
uses: actions/checkout@v4
with:
fetch-depth: 0
+ persist-credentials: falseAlso applies to: 126-139
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 112-113: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 108-125: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[error] 113-113: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-checks.yml around lines 108 - 119, The new job
test-inventory-guard currently checks out the repo and runs a PR-controlled
script (node scripts/check-test-inventory.mjs) with default token permissions;
update the actions/checkout@v4 step(s) to set persist-credentials: false and
scope the workflow job's GITHUB_TOKEN to the minimum read-only permissions
(e.g., permissions: contents: read) at the job level, and apply the same changes
to the other similar job introduced later (the job covering lines 126-139) so
both checkout steps do not leave write-capable credentials in the workspace.
Comment on lines
+60
to
+68
| it("runs the settings lane unfiltered so no describe block can fall through a -t name filter", () => { | ||
| // Plan U2 / R7 structural fix: the settings lane used to be split into six | ||
| // `-t` name-filtered sub-runs, which meant a SettingsModal describe block | ||
| // matching none of the substrings ran in NO project. The whole | ||
| // SettingsModal.test.tsx file fits one heap-6144 lane, so the lane now runs | ||
| // the project unfiltered. Guard against a regression back to `-t` filters. | ||
| const { scripts } = readDashboardPackageJson(); | ||
| expect(scripts["test:quality:app:settings"]).toContain("--project dashboard-app-quality-settings"); | ||
| expect(scripts["test:quality:app:settings"]).not.toContain("-t "); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Vitest 3.1.0 support a long-form CLI flag equivalent to -t for filtering tests by name, and what spellings are accepted?
💡 Result:
Yes, Vitest 3.1.0 supports a long-form CLI flag equivalent to -t for filtering tests by name [1][2]. The accepted spellings for this flag are: - --testNamePattern [2][3] - --test-name-pattern [1] Vitest supports both camelCase and kebab-case for CLI arguments, meaning both variations are functionally identical [2][4]. The -t flag and its long-form equivalents accept a regular expression (regex) pattern and filter tests based on their full names, including any enclosing describe blocks [5][2].
Citations:
- 1: https://raw.githubusercontent.com/vitest-dev/vitest/refs/heads/v3.1/docs/config/index.md
- 2: https://vitest.dev/guide/cli
- 3: https://github.com/vitest-dev/vitest/blob/6fdb2ba6/docs/guide/cli-generated.md
- 4: https://github.com/vitest-dev/vitest/blob/main/docs/guide/cli.md
- 5: https://vitest.dev/guide/filtering
Guard against all Vitest test-name filter flags, not just -t.
Line 68 only rejects -t , but Vitest 3.1.0 also supports long-form name-filter flags (--testNamePattern / --test-name-pattern). If the settings script regresses to those spellings, the test would still pass—assert the settings script doesn’t include any supported name-filter options (or parse argv tokens instead of substring-matching a single alias).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dashboard/src/__tests__/dashboard-test-config-guard.test.ts` around
lines 60 - 68, The test dashboard-test-config-guard.test.ts currently only
asserts that scripts["test:quality:app:settings"] does not contain "-t ", which
misses Vitest long-form flags; update the assertion for the test in the "runs
the settings lane..." it block (using readDashboardPackageJson and
scripts["test:quality:app:settings"]) to reject all Vitest name-filter flags
instead of just "-t" — e.g. assert the script string does not contain "-t" and
also does not contain "--testNamePattern" or "--test-name-pattern" (or
alternatively split the script into argv tokens and assert none of the tokens
equal any of those flags) so any long-form or short-form name filters fail the
test.
Comment on lines
+216
to
+218
| // No further dispatch should occur after the single createIssue resolves. | ||
| await Promise.resolve(); | ||
| expect(createIssueSpy).toHaveBeenCalledTimes(1); |
Contributor
There was a problem hiding this comment.
Await hook completion instead of a single microtask.
await Promise.resolve() only flushes one checkpoint. If the post-createIssue path picks up another await before an accidental second dispatch, these assertions still pass and miss the regression. Signal the end of the background hook path (for example via a deferred reached after the post-issue updates) and assert the call count after that.
Based on learnings: Prefer narrow seams, in-memory fakes, shared harnesses, and targeted assertions in tests. Prefer fake timers over real polling/time waits.
Also applies to: 327-329
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dashboard/src/__tests__/routes-planning-tracking.test.ts` around
lines 216 - 218, The test currently uses await Promise.resolve() which only
flushes a single microtask and can miss asynchronous work after createIssueSpy;
replace that with an explicit signal that the background hook path has completed
(e.g., add a Deferred or Promise that the hook resolves at the end of its
post-createIssue work) and await that in the test before asserting
expect(createIssueSpy).toHaveBeenCalledTimes(1); locate the async post-dispatch
logic tied to createIssueSpy and modify the test harness to expose/await the
completion promise (or use an in-memory fake/deferred hook completion) so the
assertion runs only after all awaiting in the hook finishes; apply the same
pattern to the other similar assertions referencing createIssueSpy in this test
file.
Comment on lines
+946
to
+1000
| function git(cwd, args) { | ||
| const r = spawnSync("git", args, { cwd, encoding: "utf8" }); | ||
| if (r.status !== 0) throw new Error(`git ${args.join(" ")} failed: ${r.stderr}`); | ||
| return r.stdout; | ||
| } | ||
|
|
||
| /** Build a tiny 3-package chain repo: a <- b <- c, plus unrelated d. */ | ||
| function makeChainRepo(dir) { | ||
| git(dir, ["init", "-q"]); | ||
| git(dir, ["config", "user.email", "t@t.t"]); | ||
| git(dir, ["config", "user.name", "t"]); | ||
| writeFileSync(path.join(dir, "pnpm-lock.yaml"), "lockfileVersion: '9.0'\n"); | ||
| writeFileSync(path.join(dir, "tsconfig.base.json"), "{}\n"); | ||
| writeFileSync(path.join(dir, "pnpm-workspace.yaml"), "packages:\n - 'packages/*'\n"); | ||
| // Shared __test-utils__ tree consumed by every package. | ||
| mkdirSync(path.join(dir, "packages", "core", "src", "__test-utils__"), { recursive: true }); | ||
| writeFileSync(path.join(dir, "packages", "core", "src", "__test-utils__", "vitest-setup.ts"), "export const setup = 1;\n"); | ||
| const pkgs = [ | ||
| ["a", "@x/a", {}], | ||
| ["b", "@x/b", { "@x/a": "workspace:*" }], | ||
| ["c", "@x/c", { "@x/b": "workspace:*" }], | ||
| ["d", "@x/d", {}], | ||
| ]; | ||
| for (const [folder, name, deps] of pkgs) { | ||
| const pkgDir = path.join(dir, "packages", folder, "src"); | ||
| mkdirSync(pkgDir, { recursive: true }); | ||
| writeFileSync(path.join(pkgDir, "index.ts"), `export const x = "${folder}-orig";\n`); | ||
| writeFileSync( | ||
| path.join(dir, "packages", folder, "package.json"), | ||
| JSON.stringify({ name, version: "1.0.0", scripts: { test: "true" }, dependencies: deps }, null, 2), | ||
| ); | ||
| } | ||
| git(dir, ["add", "-A"]); | ||
| git(dir, ["commit", "-q", "-m", "init"]); | ||
| } | ||
|
|
||
| /** | ||
| * Run a snippet inside a subprocess with FUSION_PROJECT_DIR set, importing the | ||
| * real test-changed module. The snippet receives `mod` and must console.log a | ||
| * single JSON line, which we parse and return. | ||
| */ | ||
| function runInRepo(repoDir, snippet) { | ||
| const code = ` | ||
| import * as mod from ${JSON.stringify(scriptModulePath)}; | ||
| const out = (${snippet})(mod); | ||
| console.log(JSON.stringify(out)); | ||
| `; | ||
| const r = spawnSync(process.execPath, ["--input-type=module", "-e", code], { | ||
| cwd: repoDir, | ||
| encoding: "utf8", | ||
| env: { ...process.env, FUSION_PROJECT_DIR: repoDir }, | ||
| }); | ||
| if (r.status !== 0) throw new Error(`subprocess failed: ${r.stderr}\n${r.stdout}`); | ||
| const lastLine = r.stdout.trim().split("\n").filter(Boolean).pop(); | ||
| return JSON.parse(lastLine); |
Contributor
There was a problem hiding this comment.
Make the subprocess fixtures hermetic and time-bounded.
These helpers inherit the developer's git config/env and run git commit / node -e with no timeout. A global commit.gpgSign=true, custom hooks path, or wedged child process can hang or fail the suite for reasons unrelated to the code under test. Please add a bounded timeout to both child launches and disable signing/hooks on the commit calls so these integration tests stay deterministic. As per coding guidelines, Run user-configured commands (test/build/workflow scripts) via async exec with timeout, not execSync. execSync is only acceptable for short deterministic git plumbing.
Also applies to: 1023-1025, 1064-1064, 1106-1107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/__tests__/test-changed.test.mjs` around lines 946 - 1000, The
subprocess helpers currently use spawnSync with no timeout and allow user git
hooks/signing to interfere; in git() and runInRepo() add a bounded timeout (e.g.
options.timeout = 5000) to the spawnSync calls and pass a sanitized env for
git/child processes (inherit process.env but set GIT_CONFIG_NOSYSTEM=1 and any
other needed keys) and, for commits, add git config overrides to disable
signing/hooks (e.g. include "-c", "commit.gpgSign=false", "-c",
"core.hooksPath=/dev/null" in the git commit args in function git) so commits
cannot hang and node subprocesses are time-bounded; update all spawnSync usages
referenced (git(...) and the spawnSync in runInRepo) accordingly.
Comment on lines
+25
to
+38
| const result = spawnSync( | ||
| "pnpm", | ||
| [ | ||
| "exec", | ||
| "vitest", | ||
| "run", | ||
| "--project=engine-slow", | ||
| "--silent=passed-only", | ||
| "--reporter=dot", | ||
| "--reporter=json", | ||
| `--outputFile=${outputFile}`, | ||
| ], | ||
| { cwd: engineDir, stdio: "inherit", env: { ...process.env } }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Run this gate with async exec and a timeout.
Right now a hung pnpm exec vitest leaves the CI job stuck forever because the wrapper is fully synchronous and unbounded.
As per coding guidelines, "Run user-configured commands (test/build/workflow scripts) via async exec with timeout, not execSync. execSync is only acceptable for short deterministic git plumbing."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/assert-engine-slow-nonempty.mjs` around lines 25 - 38, The current
synchronous spawnSync call using spawnSync to run "pnpm exec vitest" (building
the args array that includes "--outputFile=${outputFile}" and uses { cwd:
engineDir, stdio: "inherit", env: { ...process.env } }) must be replaced with an
asynchronous exec/execFile-based invocation that supports a timeout; call a
promisified child_process.exec/execFile (or util.promisify) to run the same
command/args, pass cwd and env as before, set the timeout option to a sensible
CI limit, capture stdout/stderr and write to the same outputFile when the
promise resolves, and reject/kill the child and fail the task if the timeout is
reached so the CI job cannot hang (update any callers that expect the previous
return value to await the new async function).
Comment on lines
+191
to
+215
| function recordBaselineFast() { | ||
| const previous = readPreviousBaseline(); | ||
| if (!previous || !Array.isArray(previous.unstableProtectedDirs)) { | ||
| recordBaseline(); | ||
| return; | ||
| } | ||
|
|
||
| const latestProtected = snapshotProtectedFusion(); | ||
| // Re-confirm engine-lock-active dirs cheaply (no sleep) so a dashboard that | ||
| // started since the previous run is still classified unstable up front. | ||
| const unstableProtectedDirs = new Set(previous.unstableProtectedDirs); | ||
| for (const entry of latestProtected) { | ||
| if (isFusionEngineActive(entry.dir)) unstableProtectedDirs.add(entry.dir); | ||
| } | ||
|
|
||
| const payload = { | ||
| tmpNames: snapshotTmp().map((e) => e.name), | ||
| protectedFusion: latestProtected, | ||
| unstableProtectedDirs: [...unstableProtectedDirs], | ||
| }; | ||
| writeFileSync(BASELINE_FILE, JSON.stringify(payload)); | ||
| console.log(`[test-isolation] Baseline recorded (fast): ${payload.tmpNames.length} temp dir(s), ${payload.protectedFusion.length} protected .fusion root(s).`); | ||
| if (unstableProtectedDirs.size > 0) { | ||
| console.log(`[test-isolation] Reusing ${unstableProtectedDirs.size} externally-active protected dir(s) from prior run.`); | ||
| } |
Contributor
There was a problem hiding this comment.
--before-fast can permanently suppress real .fusion mutations.
recordBaselineFast() carries forward every entry from previous.unstableProtectedDirs without revalidating it. If a live dashboard/external writer was active during one full baseline and then stops, the next fast baseline still writes that dir as unstable, and checkAgainstBaseline() skips it completely. From that point on, real test writes to the now-stable .fusion root pass unnoticed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-test-isolation.mjs` around lines 191 - 215, recordBaselineFast
currently carries forward every path from
readPreviousBaseline().unstableProtectedDirs without revalidating them, allowing
a formerly-active .fusion root to be permanently ignored; update
recordBaselineFast to re-check each previous dir with isFusionEngineActive (and
optionally confirm it still appears in snapshotProtectedFusion()) and only
include it in unstableProtectedDirs if the check returns true, otherwise drop it
so subsequent checkAgainstBaseline() will validate real writes; use the existing
latestProtected and isFusionEngineActive(entry.dir) logic to perform this
revalidation before writing BASELINE_FILE.
Comment on lines
+1044
to
+1074
| const start = Date.now(); | ||
| // NB: no `--` before flags (cac mis-parse); mirror the virtual-shard pattern. | ||
| spawnSync( | ||
| "pnpm", | ||
| [ | ||
| "--filter", | ||
| packageName, | ||
| "exec", | ||
| "vitest", | ||
| "run", | ||
| testFile, | ||
| "--reporter=dot", | ||
| "--reporter=json", | ||
| `--outputFile.json=${outputFile}`, | ||
| ], | ||
| { cwd: projectRoot, stdio: "inherit", env }, | ||
| ); | ||
| const wallClockMs = Date.now() - start; | ||
|
|
||
| let testDurationMs = 0; | ||
| const perFile = (() => { | ||
| try { | ||
| return extractFileDurations(JSON.parse(readFileSync(outputFile, "utf8")), packages, projectRoot); | ||
| } catch { | ||
| return new Map(); | ||
| } | ||
| })(); | ||
| for (const files of perFile.values()) { | ||
| for (const duration of files.values()) testDurationMs += duration; | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Fail the cold-start probe when Vitest fails or never writes timings.
runColdStartProbe() ignores the child exit status and happily returns testDurationMs: 0 / overheadMs: wallClockMs when the probe command fails or produces no JSON. That silently poisons the cold-start baseline with a failed run.
Proposed fix
const start = Date.now();
// NB: no `--` before flags (cac mis-parse); mirror the virtual-shard pattern.
- spawnSync(
+ const result = spawnSync(
"pnpm",
[
"--filter",
packageName,
"exec",
@@
],
{ cwd: projectRoot, stdio: "inherit", env },
);
const wallClockMs = Date.now() - start;
+
+ if (result.error) {
+ throw result.error;
+ }
+ if (result.status !== 0) {
+ throw new Error(`[ci-test-shard] cold-start-probe failed for ${packageName} (exit ${result.status})`);
+ }
+ if (!readFileSync || !outputFile) {
+ throw new Error(`[ci-test-shard] cold-start-probe produced no timing output for ${packageName}`);
+ }
let testDurationMs = 0;
const perFile = (() => {
try {
return extractFileDurations(JSON.parse(readFileSync(outputFile, "utf8")), packages, projectRoot);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ci-test-shard.mjs` around lines 1044 - 1074, The probe ignores child
exit status and missing/invalid JSON, allowing a failed Vitest run to yield
testDurationMs=0; update the runColdStartProbe flow to check the spawnSync
result and the JSON parse/contents: after calling spawnSync (the pnpm ... vitest
invocation), inspect the returned object for non-zero status or signal and, if
it failed, throw or return an error so the cold-start probe fails fast; also
validate that readFileSync(outputFile) exists and that
extractFileDurations(JSON.parse(...), packages, projectRoot) returns non-empty
timings (use outputFile and the extractFileDurations call as the check) and
treat missing/empty timings as a failure rather than silently using
testDurationMs=0, propagating the error so the baseline is not poisoned.
Comment on lines
+320
to
+330
| // U3: load the content-hash cache so branch-switch mtime churn doesn't force a | ||
| // rebuild. The default-runner (real CLI) path uses it; injected test runners | ||
| // can opt in via runOptions.artifactCache / runOptions.gitFn but default to | ||
| // disabled so existing mtime-based tests keep exercising the mtime path. | ||
| const useContentCache = runFn === run || runOptions.artifactCache !== undefined; | ||
| const cacheOptions = useContentCache | ||
| ? { | ||
| artifactCache: runOptions.artifactCache ?? readArtifactCache(resolvedRootDir), | ||
| gitFn: runOptions.gitFn ?? defaultGitRunner, | ||
| } | ||
| : {}; |
Contributor
There was a problem hiding this comment.
gitFn alone never enables the new cache path.
The comment says injected runners can opt in via runOptions.artifactCache / runOptions.gitFn, but useContentCache only checks artifactCache. Supplying just a custom gitFn silently falls back to the old mtime path.
Suggested fix
- const useContentCache = runFn === run || runOptions.artifactCache !== undefined;
+ const useContentCache =
+ runFn === run ||
+ runOptions.artifactCache !== undefined ||
+ runOptions.gitFn !== undefined;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ensure-test-artifacts.mjs` around lines 320 - 330, The boolean
useContentCache currently only checks runFn === run or runOptions.artifactCache,
so providing only runOptions.gitFn doesn't enable the content-hash cache; update
the condition that defines useContentCache to also consider runOptions.gitFn
(e.g. check runOptions.gitFn !== undefined/null) so injected runners that supply
just gitFn will take the new cache path; keep the existing cacheOptions
construction (artifactCache: runOptions.artifactCache ??
readArtifactCache(resolvedRootDir), gitFn: runOptions.gitFn ?? defaultGitRunner)
unchanged.
Comment on lines
+111
to
+117
| export function createRepoContentSnapshot({ rootDir, gitFn = defaultGitRunner }) { | ||
| const tracked = parseLsFiles(gitFn(["ls-files", "-s"], rootDir)); | ||
| const trackedByPath = new Map(tracked.map((entry) => [entry.filePath, entry.blobSha])); | ||
| const { dirtyPaths, untrackedPaths } = parseStatus( | ||
| gitFn(["status", "--porcelain", "-uall"], rootDir), | ||
| ); | ||
| return { trackedByPath, dirtyPaths, untrackedPaths }; |
Contributor
There was a problem hiding this comment.
Fail closed when git metadata is unavailable.
If gitFn(...) returns null, both the snapshot path and the direct path currently hash an empty file set instead of treating the hash as unavailable. Downstream, scripts/test-changed.mjs and scripts/sync-fusion-skill-tools.mjs use this digest for cache-hit decisions, so a transient git failure can incorrectly skip tests or skill-sync work. Please surface a “hash unavailable” result here and have callers treat it as a cache miss instead of emitting a stable digest from missing data.
Also applies to: 180-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/content-hash.mjs` around lines 111 - 117, The snapshot currently
treats gitFn returning null as an empty repo; update createRepoContentSnapshot
to detect when gitFn(...) returns null for either ls-files or status calls and
return a clearly unavailable result (e.g., null or an object like { unavailable:
true }) instead of constructing trackedByPath/dirtyPaths/untrackedPaths;
specifically check the results of gitFn used by parseLsFiles and parseStatus and
bail out early, and ensure callers (e.g., scripts/test-changed.mjs and
scripts/sync-fusion-skill-tools.mjs) treat this unavailable return as a cache
miss.
Comment on lines
+173
to
+193
| export function pruneFusionTestHomes(maxEntries = PRUNE_MAX_ENTRIES) { | ||
| let tmpEntries = []; | ||
| try { | ||
| tmpEntries = readdirSync(tmpdir(), { withFileTypes: true }); | ||
| } catch { | ||
| return; | ||
| } | ||
|
|
||
| let removed = 0; | ||
| for (const entry of tmpEntries) { | ||
| if (removed >= maxEntries) break; | ||
| if (!entry.isDirectory() || !entry.name.startsWith("fusion-test-home-root-")) continue; | ||
| const rawPath = path.join(tmpdir(), entry.name); | ||
| let resolvedPath = rawPath; | ||
| try { | ||
| resolvedPath = realpathSync(rawPath); | ||
| realpathSync(rawPath); | ||
| } catch { | ||
| // Keep raw path fallback. | ||
| } | ||
| try { | ||
| rmSync(rawPath, { recursive: true, force: true }); | ||
| removed++; |
Contributor
There was a problem hiding this comment.
Prune can still delete another live test run’s HOME.
The new cap helps scan cost, but this loop still removes the first matching fusion-test-home-root-* dirs it sees without checking age or ownership. A concurrent pnpm test that created its HOME seconds earlier is still in scope here, so one run can rm-rf another run’s active HOME mid-test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-changed.mjs` around lines 173 - 193, pruneFusionTestHomes
currently deletes the first matching fusion-test-home-root-* directories it
finds which can remove another concurrent test's live HOME; update
pruneFusionTestHomes to only remove directories that are proven stale by (1)
resolving realpathSync(rawPath) to avoid symlink tricks, (2) statSync(rawPath)
and require the directory mtime (or ctime) be older than a defined
STALE_THRESHOLD_MS constant, and (3) optionally check for a run-specific marker
(e.g., a PID or lock file inside the dir) and verify that PID is not running
before rmSync; apply these checks in the loop (using tmpEntries, rawPath,
removed, maxEntries) and skip deletion unless all stale/ownership checks pass.
…ions (~34s) Five reliability-interactions files exercised the same aiMergeTask -> resolveBranchGroupMergeRouting -> evaluateBranchGroupPromotion triangle, each paying its own real-git fixture. Removals verified duplicate-by- duplicate against shared-branch-group-lifecycle (master integration) and group-merge-coordinator (fast unit coverage of all gate reasons): - delete branch-group-promotion-gate (all 5 gate scenarios covered elsewhere) - delete branch-group-promotion (promote-once = lifecycle CASE 3) - delete shared-group-member-integration; its unique runMaintenance assertion absorbed into lifecycle CASE 4 - automerge-precedence: drop pause/engine/settings loop (unit-covered); keep group-level autoMerge override tests (unique) - merge-routing: drop 2-member + ungrouped routing dups (lifecycle CASEs 2/6); keep worktreePath + dual-audit-event tests (unique) Inventory diff: 11 removed IDs, all mapping to approved deletions (12th is the known secrets-crypto randomized-title artifact). Engine default+reliability: 6516/6516 pass. Affected-file wall-clock 52.9s -> 19.0s. Skipped: in-process-runtime source-sniff deletion — the audit claim did not match the actual file (fully-mocked suite, no readFileSync sniffs); left untouched.
…t test The curated-lane contract still holds — default lane never runs broad dashboard-app/dashboard-api — but test:quality:api now chains curated + backfill sub-lanes, so the --project arg lives one level down. Contract strengthened to also pin the backfill completeness net.
…overage preserved) Value-audit verdict: the file is already well-factored (~6% genuine bloat). Merged: 7 full-modal renders for experimental-feature labels into 1; read-only default-render clusters (Global General defaults, version display, header actions); export-filename negative folded into positive. All original assertions survive verbatim; 2 mutate-to-prove checks confirm merged tests still bite; lane green 3x with no flakes. Honest note: wall-clock neutral (~47s lane) — per-test cost is dominated by the ~60-stub beforeEach + full-modal render, not duplicate assertions. A 30-40% lane reduction requires structural work (shared mock fixture, subtree renders) deliberately not attempted here (correctness risk).
- ci-test-shard: timing outputFile is now RELATIVE — one pnpm invocation fans out to several packages whose vitests all received the same absolute path, so every package overwrote the same timings file (last writer wins). Each package now writes <pkgDir>/.timings/; discovery (discoverWorkspaceTimingFiles) and the CI artifact globs scan the tree - acp event-bridge-bounds: 20s timeout on the CPU-bound plan-flood test (timed out at default 5s under loaded CI shard, passes in isolation) - acp process-manager: port-4040-allowlist marker for its doc comments (main-side; local guard flagged it after merging main)
The newly-gated backfill file flaked on a loaded CI shard: waitFor's private 1s default (independent of the 15s vitest testTimeout) raced the click->respondToPlanning state-update chain. Passes deterministically in isolation; 5s bound absorbs shard CPU starvation without masking real regressions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts (1)
206-223: ⚡ Quick winTrim the plan-only flood payload instead of relying on a 20s timeout.
This test can prove the same cap-crossing invariant with far less CPU work by using a minimal
entriesset per plan event. That keeps shard runtime stable and avoids normalizing a much larger timeout.⚡ Proposed change
- // Generous timeout: this test does CPU-bound string flooding (~25 plan - // events x 100 entries x 2k chars) and has timed out at the default 5s - // under loaded CI shards while passing easily in isolation. - it("a plan-ONLY stream stops emitting once the per-turn cap is crossed", { timeout: 20_000 }, async () => { - const { createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES } = + it("a plan-ONLY stream stops emitting once the per-turn cap is crossed", async () => { + const { createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS } = await import("../event-bridge.js"); @@ - const entries = Array.from({ length: MAX_PLAN_ENTRIES }, () => ({ + const entries = Array.from({ length: 1 }, () => ({ content: bigEntry, priority: "low", status: "pending", }));Based on learnings: “Do not add slow tests. Prefer narrow seams, in-memory fakes, shared harnesses, and targeted assertions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts` around lines 206 - 223, The test currently floods each plan event with MAX_PLAN_ENTRIES large entries (heavy CPU); change it to use a minimal entries array (e.g., a single entry whose content length equals PER_CHUNK_CAP_CHARS) so each plan event is lightweight, keep bigEntry = "p".repeat(PER_CHUNK_CAP_CHARS) but build entries as Array.from({ length: 1 }, ...) or simply [{ content: bigEntry, priority: "low", status: "pending" }], then compute floods = Math.ceil(PER_TURN_OUTPUT_CAP_CHARS / PER_CHUNK_CAP_CHARS) + 3 (as before) so the number of plan events still crosses the per-turn cap, and remove the explicit 20_000 timeout from the it(...) call; this preserves the cap-crossing assertion while dramatically reducing CPU work (refer to createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/fusion-plugin-acp-runtime/src/process-manager.ts`:
- Line 15: Replace the raw child_process spawn usage in spawnAgent with the repo
supervised helper: remove or stop using spawn from "node:child_process", import
superviseSpawn from "`@fusion/core`", call superviseSpawn(...) instead of
spawn(...) inside spawnAgent, and ensure the returned supervised child handle is
registered with the existing registry (wire the registry to the supervised
handle the same way other supervised processes are registered) so the agent
launch follows the repository’s process-supervision lifecycle contract.
---
Nitpick comments:
In `@plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts`:
- Around line 206-223: The test currently floods each plan event with
MAX_PLAN_ENTRIES large entries (heavy CPU); change it to use a minimal entries
array (e.g., a single entry whose content length equals PER_CHUNK_CAP_CHARS) so
each plan event is lightweight, keep bigEntry = "p".repeat(PER_CHUNK_CAP_CHARS)
but build entries as Array.from({ length: 1 }, ...) or simply [{ content:
bigEntry, priority: "low", status: "pending" }], then compute floods =
Math.ceil(PER_TURN_OUTPUT_CAP_CHARS / PER_CHUNK_CAP_CHARS) + 3 (as before) so
the number of plan events still crosses the per-turn cap, and remove the
explicit 20_000 timeout from the it(...) call; this preserves the cap-crossing
assertion while dramatically reducing CPU work (refer to createEventBridge,
PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edae9328-7058-4b03-961f-fe7af9ca5fdd
📒 Files selected for processing (15)
.github/workflows/pr-checks.ymlpackages/cli/src/__tests__/package-config.test.tspackages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.test.tsxpackages/dashboard/vitest.config.tspackages/engine/src/__tests__/reliability-interactions/branch-group-automerge-precedence.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-promotion-gate.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-promotion.test.tspackages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.tspackages/engine/src/__tests__/reliability-interactions/shared-group-member-integration.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.tsplugins/fusion-plugin-acp-runtime/src/process-manager.tsscripts/__tests__/ci-test-shard-timings.test.mjsscripts/ci-test-shard.mjs
💤 Files with no reviewable changes (4)
- packages/engine/src/tests/reliability-interactions/branch-group-promotion-gate.test.ts
- packages/engine/src/tests/reliability-interactions/shared-group-member-integration.test.ts
- packages/engine/src/tests/reliability-interactions/branch-group-promotion.test.ts
- packages/engine/src/tests/reliability-interactions/branch-group-automerge-precedence.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/pr-checks.yml
- scripts/tests/ci-test-shard-timings.test.mjs
- packages/dashboard/vitest.config.ts
- scripts/ci-test-shard.mjs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts (1)
206-223: ⚡ Quick winTrim the plan-only flood payload instead of relying on a 20s timeout.
This test can prove the same cap-crossing invariant with far less CPU work by using a minimal
entriesset per plan event. That keeps shard runtime stable and avoids normalizing a much larger timeout.⚡ Proposed change
- // Generous timeout: this test does CPU-bound string flooding (~25 plan - // events x 100 entries x 2k chars) and has timed out at the default 5s - // under loaded CI shards while passing easily in isolation. - it("a plan-ONLY stream stops emitting once the per-turn cap is crossed", { timeout: 20_000 }, async () => { - const { createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES } = + it("a plan-ONLY stream stops emitting once the per-turn cap is crossed", async () => { + const { createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS } = await import("../event-bridge.js"); @@ - const entries = Array.from({ length: MAX_PLAN_ENTRIES }, () => ({ + const entries = Array.from({ length: 1 }, () => ({ content: bigEntry, priority: "low", status: "pending", }));Based on learnings: “Do not add slow tests. Prefer narrow seams, in-memory fakes, shared harnesses, and targeted assertions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts` around lines 206 - 223, The test currently floods each plan event with MAX_PLAN_ENTRIES large entries (heavy CPU); change it to use a minimal entries array (e.g., a single entry whose content length equals PER_CHUNK_CAP_CHARS) so each plan event is lightweight, keep bigEntry = "p".repeat(PER_CHUNK_CAP_CHARS) but build entries as Array.from({ length: 1 }, ...) or simply [{ content: bigEntry, priority: "low", status: "pending" }], then compute floods = Math.ceil(PER_TURN_OUTPUT_CAP_CHARS / PER_CHUNK_CAP_CHARS) + 3 (as before) so the number of plan events still crosses the per-turn cap, and remove the explicit 20_000 timeout from the it(...) call; this preserves the cap-crossing assertion while dramatically reducing CPU work (refer to createEventBridge, PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/fusion-plugin-acp-runtime/src/process-manager.ts`:
- Line 15: Replace the raw child_process spawn usage in spawnAgent with the repo
supervised helper: remove or stop using spawn from "node:child_process", import
superviseSpawn from "`@fusion/core`", call superviseSpawn(...) instead of
spawn(...) inside spawnAgent, and ensure the returned supervised child handle is
registered with the existing registry (wire the registry to the supervised
handle the same way other supervised processes are registered) so the agent
launch follows the repository’s process-supervision lifecycle contract.
---
Nitpick comments:
In `@plugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.ts`:
- Around line 206-223: The test currently floods each plan event with
MAX_PLAN_ENTRIES large entries (heavy CPU); change it to use a minimal entries
array (e.g., a single entry whose content length equals PER_CHUNK_CAP_CHARS) so
each plan event is lightweight, keep bigEntry = "p".repeat(PER_CHUNK_CAP_CHARS)
but build entries as Array.from({ length: 1 }, ...) or simply [{ content:
bigEntry, priority: "low", status: "pending" }], then compute floods =
Math.ceil(PER_TURN_OUTPUT_CAP_CHARS / PER_CHUNK_CAP_CHARS) + 3 (as before) so
the number of plan events still crosses the per-turn cap, and remove the
explicit 20_000 timeout from the it(...) call; this preserves the cap-crossing
assertion while dramatically reducing CPU work (refer to createEventBridge,
PER_CHUNK_CAP_CHARS, PER_TURN_OUTPUT_CAP_CHARS, MAX_PLAN_ENTRIES).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edae9328-7058-4b03-961f-fe7af9ca5fdd
📒 Files selected for processing (15)
.github/workflows/pr-checks.ymlpackages/cli/src/__tests__/package-config.test.tspackages/dashboard/app/components/__tests__/PlanningModeModal.planning-flow.test.tsxpackages/dashboard/app/components/__tests__/SettingsModal.test.tsxpackages/dashboard/vitest.config.tspackages/engine/src/__tests__/reliability-interactions/branch-group-automerge-precedence.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-promotion-gate.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-promotion.test.tspackages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.tspackages/engine/src/__tests__/reliability-interactions/shared-group-member-integration.test.tsplugins/fusion-plugin-acp-runtime/src/__tests__/event-bridge-bounds.test.tsplugins/fusion-plugin-acp-runtime/src/process-manager.tsscripts/__tests__/ci-test-shard-timings.test.mjsscripts/ci-test-shard.mjs
💤 Files with no reviewable changes (4)
- packages/engine/src/tests/reliability-interactions/branch-group-promotion-gate.test.ts
- packages/engine/src/tests/reliability-interactions/shared-group-member-integration.test.ts
- packages/engine/src/tests/reliability-interactions/branch-group-promotion.test.ts
- packages/engine/src/tests/reliability-interactions/branch-group-automerge-precedence.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/pr-checks.yml
- scripts/tests/ci-test-shard-timings.test.mjs
- packages/dashboard/vitest.config.ts
- scripts/ci-test-shard.mjs
🛑 Comments failed to post (1)
plugins/fusion-plugin-acp-runtime/src/process-manager.ts (1)
15-15:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Verify current usage of managed spawn wrapper and raw spawn patterns. echo "== superviseSpawn usages ==" rg -n -C2 '\bsuperviseSpawn\s*\(' --type=ts --type=tsx --type=js --type=mjs echo echo "== raw spawn usage in ACP process manager ==" rg -n -C2 '\bspawn\s*\(' plugins/fusion-plugin-acp-runtime/src/process-manager.ts echo echo "== guardrail references for subprocess policy ==" rg -n -C2 'check-no-nohup|superviseSpawn' eslint.config.mjs scripts/check-no-nohup.mjsRepository: Runfusion/Fusion
Length of output: 134
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== Target file: spawn import + spawnAgent usage ==" rg -n -C3 '\bspawn\s*\(' plugins/fusion-plugin-acp-runtime/src/process-manager.ts rg -n -C3 'from "node:child_process"' plugins/fusion-plugin-acp-runtime/src/process-manager.ts || true echo echo "== Repo: superviseSpawn usages (core managed subprocess wrapper) ==" rg -n -C2 '\bsuperviseSpawn\s*\(' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.mjs' echo echo "== Guardrails references (no-nohup / superviseSpawn) ==" ls -la eslint.config.mjs scripts/check-no-nohup.mjs 2>/dev/null || true rg -n -C2 'check-no-nohup|superviseSpawn|nohup|detached\s*:\s*true' eslint.config.mjs scripts/check-no-nohup.mjs 2>/dev/null || trueRepository: Runfusion/Fusion
Length of output: 10847
Replace raw
spawn(...)withsuperviseSpawn(...)in ACP process manager
plugins/fusion-plugin-acp-runtime/src/process-manager.tsimportsspawnfromnode:child_processandspawnAgent()starts the agent via rawspawn(...), bypassing the repo’s supervised subprocess lifecycle contract. UpdatespawnAgentto usesuperviseSpawn(...)from@fusion/core(and wire the registry to the supervised child handle) to align with the process-supervision guardrails ineslint.config.mjs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/fusion-plugin-acp-runtime/src/process-manager.ts` at line 15, Replace the raw child_process spawn usage in spawnAgent with the repo supervised helper: remove or stop using spawn from "node:child_process", import superviseSpawn from "`@fusion/core`", call superviseSpawn(...) instead of spawn(...) inside spawnAgent, and ensure the returned supervised child handle is registered with the existing registry (wire the registry to the supervised handle the same way other supervised processes are registered) so the agent launch follows the repository’s process-supervision lifecycle contract.
Every shard + the curated-guard job paid ~71s rebuilding 8 packages' dist from scratch. actions/cache now restores dist on exact content-hash match (--print-source-hash; branch-switch stable, pure git-based), with a --seed-artifact-cache step on cache-hit that defeats the mtime trap (restored dist looks older than checkout-time src mtimes). No restore-keys partial fallback: stale dist is a known failure mode here. node_modules is never cached (Windows junction policy). ensure-test-artifacts still runs as the authority and rebuilds anything genuinely missing or changed.
vi.waitFor polls, so the bound adds zero time to green runs; ink frame scheduling has flaked past 3s under shard CPU contention while passing instantly in isolation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pr-checks.yml:
- Line 100: The workflow uses the movable tag "uses: actions/cache@v4" which is
vulnerable to tag-retarget supply-chain risk; update both occurrences of the
symbol "uses: actions/cache@v4" to the corresponding immutable commit reference
"uses: actions/cache@<40-hex-commit-sha>" (replace <40-hex-commit-sha> with the
actual full commit SHA from the actions/cache repository) so the workflow pins
to a specific commit instead of a floating tag.
In `@scripts/__tests__/ensure-test-artifacts.test.mjs`:
- Around line 509-540: The tests currently derive ALL_SOURCE_INPUTS from
packageSourceInputs, which couples the fixture to the implementation; replace
that by pinning an explicit array of expected source-input paths (use a const
like PINNED_ALL_SOURCE_INPUTS) and use that pinned list in fakeGitForAllSources
and any assertions; keep the existing test that packageSourceInputs(pkg) returns
non-empty but also add an assertion that packageSourceInputs for each
REQUIRED_BUILD_PACKAGES package is a subset of (or equals) the pinned
PINNED_ALL_SOURCE_INPUTS to catch missing surfaces, and update references to
ALL_SOURCE_INPUTS in fakeGitForAllSources to use the new pinned constant.
In `@scripts/ensure-test-artifacts.mjs`:
- Around line 111-135: computeCombinedSourceHash currently only collects
packageSourceInputs, so config-only build changes can produce identical cache
keys and cause false cache hits; update computeCombinedSourceHash to include
non-source build inputs by merging packageBuildInputs(pkg) (or similar function
that returns build-config files for a package) and any repo-level build config
files into the inputPaths Set before sorting, ensuring
REQUIRED_BUILD_PACKAGES.flatMap(...) includes both packageSourceInputs(pkg) and
packageBuildInputs(pkg) and that computeContentHash is fed the combined list;
also ensure any new helper (e.g., packageBuildInputs) is deterministic and read
by computeContentHash via existing gitFn so the dist-cache key changes when
build-config files change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fc76492-f115-4a75-90cf-30c5f362c144
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
.github/workflows/pr-checks.ymldocs/test-speed-baseline-2026-06-03.mdpackages/cli/src/commands/dashboard-tui/__tests__/app.test.tsxpackages/dashboard/package.jsonscripts/__tests__/ensure-test-artifacts.test.mjsscripts/ensure-test-artifacts.mjs
✅ Files skipped from review due to trivial changes (1)
- docs/test-speed-baseline-2026-06-03.md
|
|
||
| - name: Cache built dist artifacts | ||
| id: dist-cache | ||
| uses: actions/cache@v4 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify floating vs pinned cache action refs in this workflow.
rg -nP 'uses:\s*actions/cache@' .github/workflows/pr-checks.yml
rg -nP 'uses:\s*actions/cache@[0-9a-f]{40}$' .github/workflows/pr-checks.yml || trueRepository: Runfusion/Fusion
Length of output: 130
Pin actions/cache to an immutable commit SHA
.github/workflows/pr-checks.yml still uses the movable tag uses: actions/cache@v4 at lines 100 and 171 (no actions/cache@<40-hex> commit SHA references found), leaving the workflow exposed to tag-retarget supply-chain risk. Pin these to full commit SHAs.
🧰 Tools
🪛 zizmor (1.25.2)
[error] 100-100: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-checks.yml at line 100, The workflow uses the movable
tag "uses: actions/cache@v4" which is vulnerable to tag-retarget supply-chain
risk; update both occurrences of the symbol "uses: actions/cache@v4" to the
corresponding immutable commit reference "uses:
actions/cache@<40-hex-commit-sha>" (replace <40-hex-commit-sha> with the actual
full commit SHA from the actions/cache repository) so the workflow pins to a
specific commit instead of a floating tag.
Comment on lines
+509
to
+540
| const ALL_SOURCE_INPUTS = [ | ||
| ...new Set(REQUIRED_BUILD_PACKAGES.flatMap((pkg) => packageSourceInputs(pkg))), | ||
| ]; | ||
|
|
||
| test("packageSourceInputs covers every build package (no empty source sets)", () => { | ||
| for (const pkg of REQUIRED_BUILD_PACKAGES) { | ||
| assert.ok( | ||
| packageSourceInputs(pkg).length > 0, | ||
| `${pkg.name} must contribute at least one source input to the combined hash`, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * A whole-repo git stub: every source input dir reports a single tracked file | ||
| * with the given per-path blob sha (defaults to a stable derived value). Lets us | ||
| * drive the combined hash deterministically without a real work tree. | ||
| */ | ||
| function fakeGitForAllSources(blobFor = (filePath) => `blob:${filePath}`) { | ||
| return (args) => { | ||
| if (args[0] === "rev-parse") return "true"; | ||
| if (args[0] === "ls-files") { | ||
| const lines = ALL_SOURCE_INPUTS.map((dir) => { | ||
| const filePath = `${dir}/index.ts`; | ||
| return `100644 ${blobFor(filePath)} 0\t${filePath}`; | ||
| }); | ||
| return lines.join("\n"); | ||
| } | ||
| if (args[0] === "status") return ""; // clean | ||
| return null; | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Make the combined-hash fixture independent from packageSourceInputs().
These tests build ALL_SOURCE_INPUTS from the same helper the production code uses, so they won't fail if a future edit drops one hash surface from packageSourceInputs(). The current non-empty assertion also won't catch a partially missing surface like one directory disappearing from a multi-path package. Please pin the expected inputs in the test instead of deriving them from the implementation under test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/__tests__/ensure-test-artifacts.test.mjs` around lines 509 - 540, The
tests currently derive ALL_SOURCE_INPUTS from packageSourceInputs, which couples
the fixture to the implementation; replace that by pinning an explicit array of
expected source-input paths (use a const like PINNED_ALL_SOURCE_INPUTS) and use
that pinned list in fakeGitForAllSources and any assertions; keep the existing
test that packageSourceInputs(pkg) returns non-empty but also add an assertion
that packageSourceInputs for each REQUIRED_BUILD_PACKAGES package is a subset of
(or equals) the pinned PINNED_ALL_SOURCE_INPUTS to catch missing surfaces, and
update references to ALL_SOURCE_INPUTS in fakeGitForAllSources to use the new
pinned constant.
Comment on lines
+111
to
+135
| * Stable, git-based combined source hash over ALL build packages' source | ||
| * inputs. Computable BEFORE any build (it only reads git blob SHAs / working | ||
| * tree bytes, never dist), and branch-switch stable because it defers to git | ||
| * content rather than file mtimes. Used as the CI dist-cache key. | ||
| * | ||
| * Returns null when git is unavailable (no stable key → caller must not cache). | ||
| * | ||
| * @param {string} rootDir | ||
| * @param {(args: string[], cwd: string) => string|null} [gitFn] | ||
| * @returns {string|null} | ||
| */ | ||
| export function computeCombinedSourceHash(rootDir = process.cwd(), gitFn = defaultGitRunner) { | ||
| const probe = gitFn(["rev-parse", "--is-inside-work-tree"], rootDir); | ||
| if (probe !== "true") return null; | ||
| // Sorted, de-duplicated union of every package's source inputs so the order in | ||
| // REQUIRED_BUILD_PACKAGES can't perturb the hash. | ||
| const inputPaths = [ | ||
| ...new Set(REQUIRED_BUILD_PACKAGES.flatMap((pkg) => packageSourceInputs(pkg))), | ||
| ].sort((a, b) => a.localeCompare(b)); | ||
| return computeContentHash({ | ||
| rootDir, | ||
| inputPaths, | ||
| versionPrefix: `artifact-combined-v${ARTIFACT_CACHE_VERSION}`, | ||
| gitFn, | ||
| }); |
Contributor
There was a problem hiding this comment.
Include non-source build inputs in the dist-cache key.
computeCombinedSourceHash() only hashes package source directories. A build-config-only change can keep the same key, restore an older dist, and then seedArtifactCache() will mark that restored output as fresh. That turns config-only build changes into false cache hits instead of forcing a rebuild.
Also applies to: 439-465
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ensure-test-artifacts.mjs` around lines 111 - 135,
computeCombinedSourceHash currently only collects packageSourceInputs, so
config-only build changes can produce identical cache keys and cause false cache
hits; update computeCombinedSourceHash to include non-source build inputs by
merging packageBuildInputs(pkg) (or similar function that returns build-config
files for a package) and any repo-level build config files into the inputPaths
Set before sorting, ensuring REQUIRED_BUILD_PACKAGES.flatMap(...) includes both
packageSourceInputs(pkg) and packageBuildInputs(pkg) and that computeContentHash
is fed the combined list; also ensure any new helper (e.g., packageBuildInputs)
is deterministic and read by computeContentHash via existing gitFn so the
dist-cache key changes when build-config files change.
…motion-gate test deletion over main's .slow rename
…churn The trial's pnpm add+remove cycle left peer-suffix entries inconsistent (vitest@3.2.4 combo missing); local warm installs validated shallowly while CI's fresh resolution failed every job. Re-resolved with no dependency changes (package.json untouched).
…oute flow The merge of main added resolvePrBaseRef (consumes tryRun mocks before the push sequence) and classifyGhError mapping (network errors -> structured 502 with githubError payload). The long-orphaned test pinned the old shape. Re-mapped mock queues to the real sequence, asserted the stronger current error contract (status + githubError.code + retryable), and made the accidentally-passing no-commits test deterministic. Mutate-to-prove verified the push assertion still bites.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/src/__tests__/register-git-github.pr-push-branch.test.ts`:
- Line 186: The test currently masks a missing stderr by using `stderr ?? ""`,
making the assertion too weak; change the assertion on
`response.body.details.githubError.cause.stderr` to assert the property actually
exists and is non-empty (e.g. remove the nullish coalescing and add checks like
expect(response.body.details.githubError.cause.stderr).toBeDefined() and
expect(response.body.details.githubError.cause.stderr).not.toBe("") or
expect(...).toMatch(/\S/)) so the regression (missing or empty stderr) will fail
the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e7bc4f0-5074-4a79-928b-c70415e3741c
📒 Files selected for processing (1)
packages/dashboard/src/__tests__/register-git-github.pr-push-branch.test.ts
| expect(response.body.error).toContain("Network error while talking to GitHub"); | ||
| expect(response.body.details.githubError.code).toBe("network"); | ||
| expect(response.body.details.githubError.retryable).toBe(true); | ||
| expect(response.body.details.githubError.cause.stderr ?? "").toBeDefined(); |
Contributor
There was a problem hiding this comment.
Fix weak stderr assertion in dashboard GitHub PR push-branch regression test
stderr ?? "" turns a missing/undefined stderr into "", so toBeDefined() will pass even when the regression (no stderr) occurs.
Suggested fix
- expect(response.body.details.githubError.cause.stderr ?? "").toBeDefined();
+ expect(response.body.details.githubError.cause).toEqual(
+ expect.objectContaining({ stderr: expect.any(String) }),
+ );As per coding guidelines, regression tests must assert the general invariant across ALL known surfaces, not only the single reported reproduction (FN-5893).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(response.body.details.githubError.cause.stderr ?? "").toBeDefined(); | |
| expect(response.body.details.githubError.cause).toEqual( | |
| expect.objectContaining({ stderr: expect.any(String) }), | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/dashboard/src/__tests__/register-git-github.pr-push-branch.test.ts`
at line 186, The test currently masks a missing stderr by using `stderr ?? ""`,
making the assertion too weak; change the assertion on
`response.body.details.githubError.cause.stderr` to assert the property actually
exists and is non-empty (e.g. remove the nullish coalescing and add checks like
expect(response.body.details.githubError.cause.stderr).toBeDefined() and
expect(response.body.details.githubError.cause.stderr).not.toBe("") or
expect(...).toMatch(/\S/)) so the regression (missing or empty stderr) will fail
the test.
gsxdsm
added a commit
that referenced
this pull request
Jun 5, 2026
…erge race Main went red when the fast-tests quality-backfill projects (PR #1385) landed alongside the workflow-columns stream (PR #1424) — the new test projects were written against pre-stream code: - TaskFieldsSection.css toggle knob used background: #fff, violating the theme-token assertion in AgentListModal's styling-parity test; use var(--card) per the SkillsView toggle convention - ListView.test.tsx api mock lacked fetchBoardWorkflows (TaskDetailModal now calls it on mount) - chat.test.ts and routes-agent-import.test.ts @fusion/core mocks lacked registerTraitHookImpl (engine merge-trait registers hooks at import) - auto-merge-toggle-blank.mobile and board-mobile-initial-render used vi.runAllTimers(), which never terminates now that sse-bus starts a keepalive setInterval; use vi.runOnlyPendingTimers() Both quality-backfill projects now pass fully: 7151/7151 across 414 files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pnpm testnow costs near-zero when nothing changed, never silently skips a package whose dependency changed, and CI's slowest shard drops from ~235s to ~161s — while 782 dashboard tests that previously ran nowhere are now gated. The work is measurement-first: a committed per-file timing snapshot now drives shard balancing, and every optimization had to prove no coverage was lost via a new inventory-diff harness.Plan:
docs/plans/2026-06-03-001-perf-test-suite-speedup-plan.md(all 8 units executed; U8 vitest-4 upgrade gated out on evidence).scripts/test-timings.json), mode-decision line, cold-start probeKey decisions
scripts/lib/dashboard-curated-skiplist.json) and need a follow-up fix-up ticket.isolate: falserejected everywhere by canary. Engine died with SIGTERM-143 twice; dashboard lanes ran 5.8× slower or showed cross-file contamination. The shared vitest-setup's module-level fs/child_process wrapping makes isolation load-bearing — evidence recorded indocs/test-speed-baseline-2026-06-03.mdso nobody re-trials this blind.fsModuleCacheis worth a dedicated follow-up PR; its deprecation migration delta is already zero.vi.waitForrace inroutes-planning-tracking.test.ts, fixed with deterministic awaits (mutate-to-prove verified).Test plan
scripts/__tests__/), incl. snapshot-equivalence (zero-spawn hash path), dep-invalidation chains in real git fixtures, guard failure-paths (injected leak still detected), shard-coverage invariants, and regression tests for the two review-found balance bugs.dockerfile-workspace-manifests,agents-md-invariants(recently-added plugin missing from manifest lists).Post-Deploy Monitoring & Validation
pr-checks.yml— shard duration spread (expect ≤ ~15% once real durations land),test-inventory-guardandtest-slowjobs green,.timings/artifacts uploading.even-apportionmentwarnings in shard logs (would mean the dashboard config import failed and weights degraded).pnpm test --no-cacheis the escape hatch; revertscripts/test-changed.mjsif systemic), shard timeouts from imbalance (snapshot stale — refresh via--write-timings).Known Residuals
Accepted from the Tier-2 code review (run
20260603-191251); none block merge.scripts/ci-test-shard.mjs:631— tsx spawn inresolveDashboardProjectFilesfires per shard invocation, uncached (~0.8–2s/job); cache by config content hash or plan once per run (performance, manual)scripts/ci-test-shard.mjs:1— file crossed 1,200 lines; split intolib/shard-plan,lib/timing-snapshot,lib/dashboard-lanes(maintainability, manual)scripts/test-changed.mjs:1164— thread the repo hash snapshot intoisSkillSyncCheckCachedto cut 2 git spawns (performance, gated_auto)scripts/check-test-isolation.mjs:191—--before-fastcarries forward staleunstableProtectedDirs; demote no-longer-engine-active dirs to re-probe (correctness+adversarial, manual)scripts/test-changed.mjs:605—adaptGitFnForContentHashdrops the cwd arg (latent wrong-dir hazard) (maintainability, manual)scripts/ci-test-shard.mjs:609— inlinetsx --evalscript string should be a real importable helper file (maintainability, manual)scripts/ensure-test-artifacts.mjs:324—runFn === runidentity comparison gates the artifact cache; replace with explicit option (maintainability, manual)scripts/test-changed.mjs:120— deriveshouldForceFullSuiteoverlap fromSHARED_HASH_INPUT_PATHSinstead of comment-linked parallel lists (maintainability, manual)scripts/test-changed.mjs:1— cache layer extractable tolib/test-cache.mjs; file >1,200 lines (maintainability, manual)FN-XXXXtask-ID prefix required by AGENTS.md Git Conventions (project-standards, manual)scripts/ci-test-shard.mjs/scripts/lib/content-hash.mjs— unit tests for: even-apportionment fallback,writeTimingsempty branch,dirty:missingbranch,--check-timings-stalenessabsent-snapshot CLI path,partiallyUntimedbranch,buildScheduleUnitsisolation (testing, manual)Follow-ups beyond review findings: fix-up ticket for the 31 skip-listed dashboard tests; vitest 4.x upgrade PR (
fsModuleCache); optional scheduled snapshot-refresh job wired to--check-timings-staleness.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests