FE-871: monorepo-robust test-dir + workspace runner detection (slice 4)

Amp-Thread-ID: https://ampcode.com/threads/T-019ecb9a-9a08-733b-833d-76885fc8243a
Co-authored-by: Amp <amp@ampcode.com>

Author: kostandinang

memory/PLAN.md

@@ -406,11 +406,11 @@ The May 2026 intent-spec, multi-chat, changeset-ledger, prompt/context, and agen
 - **Name:** Brunch toolchain detection — read the project toolchain from the repo
 - **Linear:** FE-871 · branch `ka/fe-871-brunch-detect` (stacked on FE-867)
 - **Kind:** bounded feature
-- **Status:** done (FE-871). Slice 1 — `detectProfile(repoDir)` / `project-detect.ts`: a pure, evidence-first detector mapping manifests/lockfiles to a registry `ProfileId` (bun lockfile → bun; deno config → deno; `package.json` vitest/jest/none → node-vitest/node-jest/node-test). One clear supported signal resolves; ambiguous evidence (both vitest **and** jest declared) and any repo with no JS/TS evidence return a loud `{detected:false, reason}` via one catch-all rather than silently defaulting to bun — the cheap "which lockfile is present" check, not a language-detection engine (no per-stack Python/Go branches; the catch-all message is already actionable). Slice 2 — `detected` is wired into the `plan-emitter` selection chain as the brownfield front (`flag ≫ detected (brownfield) ≫ spec ≫ architect-classified ≫ bun`) via `resolveEmittedProfile`; a loud detection failure throws rather than silently falling to bun (falling through to an explicit spec/architect choice first). Greenfield (or brownfield without a `repoDir`) keeps the unchanged FE-843 chain — the greenfield no-op. `repoDir` threads CLI launch cwd → `runPlan` → `emitPlanFromSnapshot`; an injectable `detect` seam keeps the emitter tests hermetic. Slice 3 — `detectTestDir(repoDir)` co-locates generated tests where the brownfield repo already keeps its own: detection picks the *runner* (profile), this picks the *path*. A profile's default test directory (`tests/{id}.test.ts`) can fall outside a host repo whose vitest `include` is narrowed (e.g. `src/**`), so the chosen path is unrunnable — vitest reports "No test files found" for an explicitly-named file (observed in a real brownfield cook). Rather than parse the runner's executable-TS config, it samples existing `*.test.*`/`*.spec.*` files (zero-dep bounded `fs` walk, skipping `node_modules`/build dirs) and returns the dominant top-level directory; `withTestDir(toolchain, dir)` relocates the targets while preserving the filename convention. Brownfield-only; `null` (no existing tests) keeps the profile default; greenfield never relocates. Stacked on `agent-extension-host`.
+- **Status:** done (FE-871). Slice 1 — `detectProfile(repoDir)` / `project-detect.ts`: a pure, evidence-first detector mapping manifests/lockfiles to a registry `ProfileId` (bun lockfile → bun; deno config → deno; `package.json` vitest/jest/none → node-vitest/node-jest/node-test). One clear supported signal resolves; ambiguous evidence (both vitest **and** jest declared) and any repo with no JS/TS evidence return a loud `{detected:false, reason}` via one catch-all rather than silently defaulting to bun — the cheap "which lockfile is present" check, not a language-detection engine (no per-stack Python/Go branches; the catch-all message is already actionable). Slice 2 — `detected` is wired into the `plan-emitter` selection chain as the brownfield front (`flag ≫ detected (brownfield) ≫ spec ≫ architect-classified ≫ bun`) via `resolveEmittedProfile`; a loud detection failure throws rather than silently falling to bun (falling through to an explicit spec/architect choice first). Greenfield (or brownfield without a `repoDir`) keeps the unchanged FE-843 chain — the greenfield no-op. `repoDir` threads CLI launch cwd → `runPlan` → `emitPlanFromSnapshot`; an injectable `detect` seam keeps the emitter tests hermetic. Slice 3 — `detectTestDir(repoDir)` co-locates generated tests where the brownfield repo already keeps its own: detection picks the *runner* (profile), this picks the *path*. A profile's default test directory (`tests/{id}.test.ts`) can fall outside a host repo whose vitest `include` is narrowed (e.g. `src/**`), so the chosen path is unrunnable — vitest reports "No test files found" for an explicitly-named file (observed in a real brownfield cook). Rather than parse the runner's executable-TS config, it samples existing `*.test.*`/`*.spec.*` files (zero-dep bounded `fs` walk, skipping `node_modules`/build dirs) and returns the dominant directory; `withTestDir(toolchain, dir)` relocates the targets while preserving the filename convention. Brownfield-only; `null` (no existing tests) keeps the profile default; greenfield never relocates. Slice 4 — monorepo hardening: `detectTestDir` returns the dominant *full* directory (not just the top segment) so a package-rooted include glob still covers the path; `detectProfile` widens runner detection to declared workspace packages (npm/yarn `workspaces`, pnpm `pnpm-workspace.yaml`; literal + single-level `dir/*` globs) **only when the root declares no runner**, scoped to declared workspaces so a stray nested project (docs prototype, example app) can't poison detection — a root runner still wins without scanning, and workspaces collectively declaring both vitest+jest stays loudly ambiguous. Stacked on `agent-extension-host`.
 - **Objective:** Resolve a registry `ProfileId` at **plan time** from the repo's manifest/lockfile evidence — the cheap "which lockfile/manifest is present" check, mapping only to ids already in the FE-843 registry. It is **not** a language-detection engine: anything without a single clear supported signal (ambiguous JS runners, or non-JS stacks like Python/Go) returns a loud `{detected:false}` reason via one actionable catch-all, never a guessed profile. Brownfield-only front of the selection chain (`flag ≫ detected ≫ spec ≫ architect ≫ bun`); the resolved id is stamped into `plan.yaml` so `brunch cook` runs the same toolchain. Greenfield never detects (empty worktree). Resolves toolchain **identity** only — real file paths / existing wiring / `writes` reconciliation is cook-time agent grounding, out of scope here.
 - **Why now / unlocks:** The "no manual steps" goal requires reading the real toolchain rather than inferring from spec prose or a `--profile` flag — and it must happen at plan time, because the deterministic test runner reads the stamped `plan.profile` with **no agent in the loop** (`cook-cli.ts`, `pi-actions.ts`), so a wrong default runs the wrong test command with no diagnostic. The cook agent's `read`/`bash` cannot substitute. FE-843 built the registry but deferred detection; this closes that gap.
 - **Acceptance:** (1) detection maps a real repo to a registry profile id from manifest/lockfile evidence *(slice 1, done)*; (2) brownfield cook/plan resolves toolchain via detection at the front of the FE-843 chain (`--profile` still overrides) *(slice 2)*; (3) greenfield resolution is unchanged (no detection input); (4) ambiguous/unknown repo fails with an actionable message, not a silent default *(slice 1, done)*; (5) the 3 reference fixtures + greenfield smoke score identically before/after.
-- **Verification:** detector unit tests *(slice 1, done — per-stack fixtures + loud `{detected:false}`)*; slice 2: resolution-chain precedence tests (detect vs flag vs spec) + greenfield no-op / before-after-identical test; slice 3: `detectTestDir` clustering/skip/null tests + `withTestDir` relocation tests + emitter tests asserting brownfield targets follow the detected dir while greenfield keeps the profile default.
+- **Verification:** detector unit tests *(slice 1, done — per-stack fixtures + loud `{detected:false}`)*; slice 2: resolution-chain precedence tests (detect vs flag vs spec) + greenfield no-op / before-after-identical test; slice 3: `detectTestDir` clustering/skip/null tests + `withTestDir` relocation tests + emitter tests asserting brownfield targets follow the detected dir while greenfield keeps the profile default; slice 4: full-dir/monorepo `detectTestDir` tests + workspace runner-detection tests (npm/yarn/pnpm, root-wins, literal dir, cross-workspace ambiguity).
 - **Depends on:** `toolchain-profile-expansion` (FE-843).
 - **Traceability:** Requirements 46–50; refines I130-K; greenfield-protecting invariant (new — record in SPEC via ln-sync). **D160-K boundary:** detection is plan-time profile-*id* resolution (an input to authoring), not architect host-introspection — D160-K constrains the architect/authoring stage, not profile resolution, so `brunch-detect` needs no D160-K amendment.
 - **Design docs:** `docs/design/orchestrator.md`.

src/orchestrator/src/project-detect.test.ts

@@ -88,13 +88,24 @@ describe('detectProfile fails loudly rather than defaulting silently', () => {
 });
 
 describe('detectTestDir learns the test directory from existing test files', () => {
-  it('returns the top-level directory tests cluster in (src/**)', () => {
+  it('returns the full directory tests cluster in, not just the top segment', () => {
     const dir = repo({
-      'src/foo.test.ts': '',
       'src/lib/bar.test.ts': '',
+      'src/lib/qux.test.ts': '',
+      'src/foo.test.ts': '',
       'src/lib/baz.ts': '',
     });
-    expect(detectTestDir(dir)).toBe('src');
+    // src/lib has 2 test files, src has 1 → the deeper, dominant dir wins.
+    expect(detectTestDir(dir)).toBe('src/lib');
+  });
+
+  it('returns a deep monorepo test root so a package-rooted include still covers it', () => {
+    const dir = repo({
+      'packages/app/src/a.test.ts': '',
+      'packages/app/src/b.test.ts': '',
+      'packages/lib/src/c.test.ts': '',
+    });
+    expect(detectTestDir(dir)).toBe('packages/app/src');
   });
 
   it('picks the dominant directory when tests are split across several', () => {
@@ -129,3 +140,50 @@ describe('detectTestDir learns the test directory from existing test files', ()
     expect(detectTestDir(repo({ 'root.test.ts': '' }))).toBeNull();
   });
 });
+
+describe('detectProfile resolves the runner from workspace packages in a monorepo', () => {
+  it('finds vitest in a workspace package when the root declares no runner', () => {
+    const dir = repo({
+      'package.json': JSON.stringify({ workspaces: ['packages/*'] }),
+      'packages/app/package.json': pkg({ vitest: '^2.0.0' }),
+      'packages/lib/package.json': pkg({ typescript: '^5.0.0' }),
+    });
+    expect(detectProfile(dir)).toMatchObject({ detected: true, profile: 'node-vitest' });
+  });
+
+  it('finds the runner via a pnpm-workspace.yaml package list', () => {
+    const dir = repo({
+      'package.json': JSON.stringify({ name: 'root' }),
+      'pnpm-workspace.yaml': "packages:\n  - 'packages/*'\n",
+      'packages/web/package.json': pkg({ jest: '^29.0.0' }),
+    });
+    expect(detectProfile(dir)).toMatchObject({ detected: true, profile: 'node-jest' });
+  });
+
+  it('a root runner wins without scanning (and a workspace cannot make it ambiguous)', () => {
+    const dir = repo({
+      'package.json': JSON.stringify({ workspaces: ['packages/*'], devDependencies: { vitest: '^2.0.0' } }),
+      'packages/legacy/package.json': pkg({ jest: '^29.0.0' }),
+    });
+    expect(detectProfile(dir)).toMatchObject({ detected: true, profile: 'node-vitest' });
+  });
+
+  it('workspaces collectively declaring both runners is ambiguous, not silently picked', () => {
+    const dir = repo({
+      'package.json': JSON.stringify({ workspaces: ['packages/*'] }),
+      'packages/a/package.json': pkg({ vitest: '^2.0.0' }),
+      'packages/b/package.json': pkg({ jest: '^29.0.0' }),
+    });
+    const result = detectProfile(dir);
+    expect(result.detected).toBe(false);
+    expect(!result.detected && result.reason).toMatch(/ambiguous/i);
+  });
+
+  it('a literal (non-wildcard) workspace directory is resolved', () => {
+    const dir = repo({
+      'package.json': JSON.stringify({ workspaces: ['apps/web'] }),
+      'apps/web/package.json': pkg({ vitest: '^2.0.0' }),
+    });
+    expect(detectProfile(dir)).toMatchObject({ detected: true, profile: 'node-vitest' });
+  });
+});

src/orchestrator/src/project-detect.ts

@@ -42,6 +42,76 @@ function readPackageJsonDeps(dir: string): Set<string> | null {
   }
 }
 
+/**
+ * Workspace globs declared by a monorepo root — npm/yarn `workspaces` (array or
+ * `{ packages }`) or pnpm `pnpm-workspace.yaml`. Empty when the repo is not a
+ * declared monorepo. Scoping to *declared* workspaces (not every package.json on
+ * disk) keeps a stray nested project — a docs prototype, an example app — from
+ * poisoning runner detection.
+ */
+function readWorkspaceGlobs(repoDir: string): string[] {
+  const pkgPath = join(repoDir, 'package.json');
+  if (existsSync(pkgPath)) {
+    try {
+      const pkg = JSON.parse(readFileSync(pkgPath, 'utf8')) as {
+        workspaces?: string[] | { packages?: string[] };
+      };
+      const ws = pkg.workspaces;
+      if (Array.isArray(ws)) return ws;
+      if (ws && Array.isArray(ws.packages)) return ws.packages;
+    } catch {
+      // Malformed root package.json: fall through to the pnpm manifest.
+    }
+  }
+  const pnpmPath = join(repoDir, 'pnpm-workspace.yaml');
+  if (existsSync(pnpmPath)) {
+    try {
+      const globs: string[] = [];
+      for (const line of readFileSync(pnpmPath, 'utf8').split('\n')) {
+        const match = /^\s*-\s*['"]?([^'"#]+?)['"]?\s*$/.exec(line);
+        if (match) globs.push(match[1].trim());
+      }
+      return globs;
+    } catch {
+      return [];
+    }
+  }
+  return [];
+}
+
+/**
+ * Resolve a workspace glob to concrete package directories. Handles the two
+ * forms that cover virtually all real monorepos — a literal directory (`apps/web`)
+ * and a single-level wildcard (`packages/*`). Deeper/exotic globs are skipped:
+ * this is the cheap evidence check, not a glob engine.
+ */
+function resolveWorkspaceDirs(repoDir: string, glob: string): string[] {
+  const trimmed = glob.replace(/\/+$/, '');
+  if (trimmed.endsWith('/*')) {
+    const base = trimmed.slice(0, -2);
+    try {
+      return readdirSync(join(repoDir, base), { withFileTypes: true })
+        .filter((entry) => entry.isDirectory())
+        .map((entry) => join(base, entry.name));
+    } catch {
+      return [];
+    }
+  }
+  return trimmed.includes('*') ? [] : [trimmed];
+}
+
+/** Union of dependency names declared across a monorepo root's workspace packages. */
+function collectWorkspaceDeps(repoDir: string): Set<string> {
+  const deps = new Set<string>();
+  for (const glob of readWorkspaceGlobs(repoDir)) {
+    for (const wsDir of resolveWorkspaceDirs(repoDir, glob)) {
+      const wsDeps = readPackageJsonDeps(join(repoDir, wsDir));
+      if (wsDeps) for (const dep of wsDeps) deps.add(dep);
+    }
+  }
+  return deps;
+}
+
 /**
  * Detect the toolchain `ProfileId` for a repo by introspecting its manifests and
  * lockfiles. Precedence is lockfile/config evidence first (most authoritative),
@@ -60,23 +130,38 @@ export function detectProfile(repoDir: string): ProfileDetection {
   }
 
   // Node/TypeScript: pick the runner from declared dependencies.
-  const deps = readPackageJsonDeps(repoDir);
-  if (deps !== null) {
+  const rootDeps = readPackageJsonDeps(repoDir);
+  if (rootDeps !== null) {
+    // Root deps are most authoritative. Only when the root declares no runner do
+    // we widen to the monorepo's workspace packages — a monorepo root often holds
+    // just tooling while the runner lives in each package. A repo that already
+    // resolves at the root never pays the workspace scan and can't be made
+    // ambiguous by a workspace.
+    let deps = rootDeps;
+    let source = 'package.json';
+    if (!rootDeps.has('vitest') && !rootDeps.has('jest')) {
+      const wsDeps = collectWorkspaceDeps(repoDir);
+      if (wsDeps.has('vitest') || wsDeps.has('jest')) {
+        deps = wsDeps;
+        source = 'workspace package.json';
+      }
+    }
+
     const hasVitest = deps.has('vitest');
     const hasJest = deps.has('jest');
     // Two declared runners is genuinely ambiguous — picking one by check-order
     // would silently run the wrong command. Fail loud and let `--profile` decide.
     if (hasVitest && hasJest) {
       return {
         detected: false,
-        reason: `package.json declares both vitest and jest — ambiguous test runner. Pass --profile to pick node-vitest or node-jest.`,
+        reason: `${source} declares both vitest and jest — ambiguous test runner. Pass --profile to pick node-vitest or node-jest.`,
       };
     }
     if (hasVitest) {
-      return { detected: true, profile: 'node-vitest', evidence: 'package.json devDependency vitest' };
+      return { detected: true, profile: 'node-vitest', evidence: `${source} devDependency vitest` };
     }
     if (hasJest) {
-      return { detected: true, profile: 'node-jest', evidence: 'package.json devDependency jest' };
+      return { detected: true, profile: 'node-jest', evidence: `${source} devDependency jest` };
     }
     // No third-party runner declared → the built-in node:test runner needs none.
     return {
@@ -118,17 +203,21 @@ const MAX_WALK_DEPTH = 8;
  * path falls outside a repo whose vitest `include` is narrowed to `src/**`
  * (vitest then reports "No test files found" for an explicitly-named file).
  *
- * Returns the dominant top-level segment (e.g. `'src'`), or `null` when the repo
- * has no test files to learn from — cook then keeps the profile's default path.
+ * Returns the POSIX-relative directory tests cluster in (e.g. `'src'`, or
+ * `'packages/app/src'` in a monorepo), or `null` when the repo has no test files
+ * to learn from — cook then keeps the profile's default path. The *full*
+ * directory (not just the top segment) is returned so a monorepo whose runner
+ * include is rooted deep (e.g. a per-package `src` glob) still gets a covered
+ * path.
  */
 export function detectTestDir(repoDir: string): string | null {
-  // Tally test files by their top-level directory segment relative to the repo
-  // root, because runner include globs are conventionally rooted there
-  // (`src/**`, `tests/**`). Files directly at the root (segment '') don't teach
-  // us a directory, so they're ignored.
+  // Tally test files by their full directory relative to the repo root. Files
+  // directly at the root (relDir '') don't teach us a directory, so they're
+  // ignored. Keys are POSIX paths so the emitted target matches profile
+  // conventions regardless of host separator.
   const counts = new Map<string, number>();
 
-  const walk = (dir: string, depth: number, topSegment: string | null): void => {
+  const walk = (dir: string, depth: number, relDir: string): void => {
     if (depth > MAX_WALK_DEPTH) return;
     let entries: Dirent[];
     try {
@@ -139,15 +228,16 @@ export function detectTestDir(repoDir: string): string | null {
     for (const entry of entries) {
       if (entry.isDirectory()) {
         if (SKIP_DIRS.has(entry.name) || entry.name.startsWith('.')) continue;
-        walk(join(dir, entry.name), depth + 1, topSegment ?? entry.name);
-      } else if (topSegment !== null && entry.isFile() && TEST_FILE_RE.test(entry.name)) {
-        counts.set(topSegment, (counts.get(topSegment) ?? 0) + 1);
+        walk(join(dir, entry.name), depth + 1, relDir === '' ? entry.name : `${relDir}/${entry.name}`);
+      } else if (relDir !== '' && entry.isFile() && TEST_FILE_RE.test(entry.name)) {
+        counts.set(relDir, (counts.get(relDir) ?? 0) + 1);
       }
     }
   };
-  walk(repoDir, 0, null);
+  walk(repoDir, 0, '');
 
   if (counts.size === 0) return null;
-  // Dominant directory wins; ties broken by name for determinism.
+  // Dominant directory wins; ties broken by name (shallower/earlier first) for
+  // determinism.
   return [...counts].sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0]))[0][0];
 }