Skip to content

fix(01KVWMCK): reviewer installs PR-worktree deps before the AC walk#446

Merged
jackmcintyre merged 1 commit into
mainfrom
fix/01KVWMCK-reviewer-installs-deps
Jun 24, 2026
Merged

fix(01KVWMCK): reviewer installs PR-worktree deps before the AC walk#446
jackmcintyre merged 1 commit into
mainfrom
fix/01KVWMCK-reviewer-installs-deps

Conversation

@jackmcintyre

Copy link
Copy Markdown
Owner

What & why

The run's reviewer materialised the PR head into a fresh git worktree and ran each vitest: acceptance check against it without ever installing dependencies. A fresh git worktree add carries no node_modules, so any dependency-importing suite (or a vitest.config.ts importing vitest/config) failed on UNRESOLVED_IMPORT. The reviewer recorded that as a quality failure, derived NEEDS CHANGES, and bounced the story until it blocked rework-exhausted — with a genuinely-green PR sitting on the branch and nothing shipped. This hit essentially every story whose tests import a dependency, so one false negative on a foundational story stalled the whole backlog.

The dev side never hit this because the dev is an agent that installs as it codes; the reviewer is a pure deterministic tool seam that never did.

The fix

Install the materialised worktree's dependencies once, before the AC walk, in runReviewerSession:

  • New plugins/flow/mcp-server/src/lib/prepare-review-worktree.ts reuses the same structural toolchain resolver the dev pre-PR gate uses (resolveProjectToolchain) and runs a clean frozen install (npm ci / pnpm install --frozen-lockfile / yarn install --immutable / bun install --frozen-lockfile), gated on lockfile presence (--prefer-offline where supported).
  • Runs at the lockfile's workspace root, walked up from the build home — which on a workspace/monorepo target may differ from the resolver's build-home cwd (for the Flow repo they coincide at plugins/flow).
  • Runs exactly once per review, before the per-AC loop — never re-entered per AC (which would reintroduce the slow-review failure mode).
  • If the worktree cannot be prepared, the review surfaces a re-runnable setup-error (never a quality verdict), reusing the existing setup-error result path so the story is not dead-ended in rework. A shared writeSetupErrorResult helper now backs both the standards-missing and the install-failure setup-error paths.

Acceptance criteria

  • AC1 — a dependency-importing suite that would fail in an un-installed worktree passes once deps are installed, and the story reaches an approved verdict instead of bouncing.
  • AC2 — an unpreparable worktree is reported as a re-runnable setup problem, not a quality failure; the AC walk is skipped and the worktree is cleaned up.

Tests & checks

  • New unit suite for the install helper: plan walk (build-home lockfile, monorepo walk-up to repo-root lockfile, no-lockfile skip), per-manager frozen args, and runner skip/success/failure/timeout.
  • New reviewer behavioural tests: install-runs-once-before-the-vitest-call (at the worktree root) and failed-install → setup-error (AC walk skipped, worktree cleaned, result file carries the setup detail).
  • Existing reviewer stubs updated to route install commands.
  • Full suite green (3843 tests), typecheck + lint + knip clean, coverage 86.29% overall (new file 97.14%), dist/ bundles rebuilt and committed (deterministic).

Note for review

This fixes the very reviewer path it touches, so the currently-installed reviewer cannot validate this PR's own dependency-importing suite — it was built and verified by hand (/ship-story-style), not via /flow:run. Merge on green CI, then /reinstall-flow + restart to make the fix live for subsequent autonomous runs.

https://claude.ai/code/session_01JczczroEnDH6k8KMNdMMxE

The run's reviewer materialised the PR head into a fresh git worktree and ran
each vitest: acceptance check against it WITHOUT ever installing dependencies.
A fresh `git worktree add` carries no node_modules, so dependency-importing
suites failed on UNRESOLVED_IMPORT and a genuinely-green PR was mislabelled
NEEDS CHANGES -> rework-exhausted. The dev side never hit this because the dev
is an agent that installs as it codes; the reviewer is a pure tool seam.

Fix: install the worktree's dependencies ONCE, before the AC walk, in
runReviewerSession:
- New lib `prepare-review-worktree.ts` reuses the same structural toolchain
  resolver the dev pre-PR gate uses (resolveProjectToolchain) and runs a clean
  frozen install (npm ci / pnpm install --frozen-lockfile / yarn install
  --immutable / bun install --frozen-lockfile), gated on lockfile presence.
- The install runs at the LOCKFILE's workspace root, walked up from the build
  home, which on a workspace/monorepo target may differ from the resolver's
  build-home cwd (for Flow they coincide at plugins/flow).
- The install runs exactly once per review, before the per-AC loop, never per AC.
- If the worktree cannot be prepared, the review surfaces a re-runnable
  setup-error (NOT a quality verdict), reusing the existing setup-error result
  path so the story is not dead-ended in doomed rework. The shared
  writeSetupErrorResult helper now backs both the standards-missing and the
  install-failure setup-error paths.

Tests: new unit suite for the install helper (plan walk, per-manager frozen
args, skip/success/failure/timeout) plus reviewer behavioural tests for
install-once-before-the-walk and failed-install-is-a-setup-error. Existing
reviewer stubs updated to route install commands. dist bundles rebuilt.

Claude-Session: https://claude.ai/code/session_01JczczroEnDH6k8KMNdMMxE
@jackmcintyre jackmcintyre merged commit 0b67100 into main Jun 24, 2026
1 check passed
@jackmcintyre jackmcintyre deleted the fix/01KVWMCK-reviewer-installs-deps branch June 24, 2026 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant