Skip to content

Commit 4b6b2f8

Browse files
waleedkadouswaleedkadous
authored
Merge pull request #679 from cluesmith/builder/bugfix-676-porch-approve-plan-approval-fa
[Bugfix #676] Scope porch check artifact resolution to the worktree
2 parents 54e9815 + 0e70c85 commit 4b6b2f8

4 files changed

Lines changed: 273 additions & 10 deletions

File tree

‎packages/codev/src/commands/porch/__tests__/bugfix-676-approve-worktree-artifacts.test.ts‎

Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
/**
2+
* Regression test for bugfix #676: `porch approve plan-approval` fails with
3+
* "Plan not found" when the plan lives in a builder worktree.
4+
*
5+
* Reproduction:
6+
* 1. Architect runs `porch approve NNN plan-approval ...` from the main
7+
* workspace root (cwd = repo root).
8+
* 2. `findStatusPath` (fixed in PR #674) correctly locates the project's
9+
* status.yaml inside `.builders/<slug>/codev/projects/...`.
10+
* 3. BUT the artifact resolver and the cwd passed to `runPhaseChecks` are
11+
* still scoped to the main workspace, so the `plan_exists` check looks
12+
* at `<main>/codev/plans/` — where the plan does not exist yet —
13+
* and fails.
14+
*
15+
* Fix: `check`, `done`, and `approve` derive the artifact root from the
16+
* resolved status path (via `getArtifactRoot`) and rebuild the resolver +
17+
* check cwd to point at that worktree.
18+
*/
19+
20+
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
21+
import * as fs from 'node:fs';
22+
import * as path from 'node:path';
23+
import { tmpdir } from 'node:os';
24+
import { approve, check, done } from '../index.js';
25+
import { writeState, readState, getStatusPath } from '../state.js';
26+
import type { ProjectState } from '../types.js';
27+
28+
// ---------------------------------------------------------------------------
29+
// Helpers
30+
// ---------------------------------------------------------------------------
31+
32+
/** Mimic the on-disk layout: `<mainRoot>/.builders/<slug>/...` with its own codev/. */
33+
function makeWorkspace(suffix: string): { mainRoot: string; worktreeRoot: string; worktreeSlug: string } {
34+
const mainRoot = path.join(tmpdir(), `porch-676-${suffix}-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`);
35+
fs.mkdirSync(mainRoot, { recursive: true });
36+
const worktreeSlug = 'spir-42-test-feature';
37+
const worktreeRoot = path.join(mainRoot, '.builders', worktreeSlug);
38+
fs.mkdirSync(worktreeRoot, { recursive: true });
39+
return { mainRoot, worktreeRoot, worktreeSlug };
40+
}
41+
42+
function writeProtocol(root: string, protocol: { name: string; [k: string]: unknown }): void {
43+
const dir = path.join(root, 'codev', 'protocols', protocol.name);
44+
fs.mkdirSync(dir, { recursive: true });
45+
fs.writeFileSync(path.join(dir, 'protocol.json'), JSON.stringify(protocol, null, 2));
46+
}
47+
48+
function writeWorktreeStatus(worktreeRoot: string, state: ProjectState): string {
49+
const statusPath = getStatusPath(worktreeRoot, state.id, state.title);
50+
fs.mkdirSync(path.dirname(statusPath), { recursive: true });
51+
writeState(statusPath, state);
52+
return statusPath;
53+
}
54+
55+
function makeState(overrides: Partial<ProjectState> = {}): ProjectState {
56+
return {
57+
id: '0042',
58+
title: 'test-feature',
59+
protocol: 'spir-676-test',
60+
phase: 'plan',
61+
plan_phases: [],
62+
current_plan_phase: null,
63+
gates: {
64+
'plan-approval': { status: 'pending', requested_at: new Date().toISOString() },
65+
},
66+
iteration: 1,
67+
build_complete: false,
68+
history: [],
69+
started_at: new Date().toISOString(),
70+
updated_at: new Date().toISOString(),
71+
...overrides,
72+
};
73+
}
74+
75+
// Protocol is saved to disk as JSON and normalized by `loadProtocol`.
76+
// Phase `checks` is an object of name → command (not an array).
77+
const testProtocolJson = {
78+
name: 'spir-676-test',
79+
version: '1.0.0',
80+
phases: [
81+
{
82+
id: 'plan',
83+
name: 'Plan',
84+
gate: 'plan-approval',
85+
checks: {
86+
plan_exists: 'test -f codev/plans/${PROJECT_TITLE}.md',
87+
},
88+
next: 'implement',
89+
},
90+
{ id: 'implement', name: 'Implement', next: null },
91+
],
92+
};
93+
94+
// Suppress noisy CLI output; keep process.exit swallowable so we can inspect state.
95+
let logSpy: ReturnType<typeof vi.spyOn>;
96+
let exitSpy: ReturnType<typeof vi.spyOn>;
97+
98+
beforeEach(() => {
99+
logSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
100+
exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => {
101+
throw new Error(`process.exit(${code ?? 0})`);
102+
}) as never);
103+
});
104+
105+
afterEach(() => {
106+
logSpy.mockRestore();
107+
exitSpy.mockRestore();
108+
});
109+
110+
// ---------------------------------------------------------------------------
111+
// Tests
112+
// ---------------------------------------------------------------------------
113+
114+
describe('bugfix #676 — artifact resolver follows worktree status path', () => {
115+
it('approve resolves plan_exists against the worktree, not the main workspace', async () => {
116+
const { mainRoot, worktreeRoot } = makeWorkspace('approve');
117+
writeProtocol(mainRoot, testProtocolJson);
118+
// Protocol files are read from the main workspace — but artifacts live in the worktree.
119+
120+
const statusPath = writeWorktreeStatus(worktreeRoot, makeState());
121+
122+
// Plan exists ONLY in the worktree; the main workspace has NO codev/plans/ directory.
123+
const worktreePlansDir = path.join(worktreeRoot, 'codev', 'plans');
124+
fs.mkdirSync(worktreePlansDir, { recursive: true });
125+
fs.writeFileSync(path.join(worktreePlansDir, '0042-test-feature.md'), '# Plan\n');
126+
127+
expect(fs.existsSync(path.join(mainRoot, 'codev', 'plans'))).toBe(false);
128+
129+
// Architect runs approve from main workspace root
130+
await approve(mainRoot, '0042', 'plan-approval', true);
131+
132+
// Gate should be approved (checks passed because resolver found the plan in the worktree)
133+
const updated = readState(statusPath);
134+
expect(updated.gates['plan-approval'].status).toBe('approved');
135+
expect(updated.gates['plan-approval'].approved_at).toBeDefined();
136+
});
137+
138+
it('approve fails cleanly when the plan is missing from the worktree', async () => {
139+
const { mainRoot, worktreeRoot } = makeWorkspace('approve-missing');
140+
writeProtocol(mainRoot, testProtocolJson);
141+
const statusPath = writeWorktreeStatus(worktreeRoot, makeState());
142+
143+
// Writing a plan in the MAIN workspace must NOT trick the check into
144+
// approving the gate — only the worktree is authoritative.
145+
const mainPlansDir = path.join(mainRoot, 'codev', 'plans');
146+
fs.mkdirSync(mainPlansDir, { recursive: true });
147+
fs.writeFileSync(path.join(mainPlansDir, '0042-test-feature.md'), '# Decoy\n');
148+
149+
await expect(approve(mainRoot, '0042', 'plan-approval', true)).rejects.toThrow(/process\.exit/);
150+
151+
// Gate must NOT be approved when the worktree lacks the plan
152+
const updated = readState(statusPath);
153+
expect(updated.gates['plan-approval'].status).toBe('pending');
154+
expect(updated.gates['plan-approval'].approved_at).toBeUndefined();
155+
});
156+
157+
it('check resolves plan_exists against the worktree', async () => {
158+
const { mainRoot, worktreeRoot } = makeWorkspace('check');
159+
writeProtocol(mainRoot, testProtocolJson);
160+
writeWorktreeStatus(worktreeRoot, makeState());
161+
162+
const worktreePlansDir = path.join(worktreeRoot, 'codev', 'plans');
163+
fs.mkdirSync(worktreePlansDir, { recursive: true });
164+
fs.writeFileSync(path.join(worktreePlansDir, '0042-test-feature.md'), '# Plan\n');
165+
166+
// check() prints results but must not throw when all checks pass
167+
await expect(check(mainRoot, '0042')).resolves.toBeUndefined();
168+
});
169+
170+
it('done resolves plan_exists against the worktree', async () => {
171+
const { mainRoot, worktreeRoot } = makeWorkspace('done');
172+
// Protocol where the plan phase has no gate, so done() will attempt to advance
173+
const protocolNoGate = {
174+
name: 'spir-676-test-done',
175+
version: '1.0.0',
176+
phases: [
177+
{
178+
id: 'plan',
179+
name: 'Plan',
180+
checks: { plan_exists: 'test -f codev/plans/${PROJECT_TITLE}.md' },
181+
next: 'review',
182+
},
183+
{ id: 'review', name: 'Review', next: null },
184+
],
185+
};
186+
writeProtocol(mainRoot, protocolNoGate);
187+
188+
const state = makeState({ protocol: 'spir-676-test-done', gates: {} });
189+
const statusPath = writeWorktreeStatus(worktreeRoot, state);
190+
191+
const worktreePlansDir = path.join(worktreeRoot, 'codev', 'plans');
192+
fs.mkdirSync(worktreePlansDir, { recursive: true });
193+
fs.writeFileSync(path.join(worktreePlansDir, '0042-test-feature.md'), '# Plan\n');
194+
195+
await done(mainRoot, '0042');
196+
197+
const updated = readState(statusPath);
198+
expect(updated.phase).toBe('review');
199+
});
200+
});

‎packages/codev/src/commands/porch/artifacts.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -362,8 +362,18 @@ export class CliResolver implements ArtifactResolver {
362362
/**
363363
* Create the appropriate artifact resolver for this workspace.
364364
* Reads config via loadConfig() (v3.0.0 unified config — .codev/config.json).
365+
*
366+
* @param workspaceRoot - The top-level workspace (where .codev/config.json lives).
367+
* Always used to load config; also used as the .env source
368+
* for the CLI backend.
369+
* @param artifactRoot - Optional override for where the local backend reads
370+
* specs/plans/reviews from. When the project lives in a
371+
* builder worktree (`.builders/<slug>/`), pass that
372+
* worktree's root so artifact lookups find files there
373+
* instead of the top-level `codev/` directory (bugfix #676).
374+
* Defaults to workspaceRoot.
365375
*/
366-
export function getResolver(workspaceRoot: string): ArtifactResolver {
376+
export function getResolver(workspaceRoot: string, artifactRoot?: string): ArtifactResolver {
367377
const config = loadConfig(workspaceRoot);
368378
const artifacts = config.artifacts;
369379

@@ -390,5 +400,5 @@ export function getResolver(workspaceRoot: string): ArtifactResolver {
390400
);
391401
}
392402

393-
return new LocalResolver(workspaceRoot);
403+
return new LocalResolver(artifactRoot ?? workspaceRoot);
394404
}

‎packages/codev/src/commands/porch/index.ts‎

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import {
1515
writeStateAndCommit,
1616
createInitialState,
1717
findStatusPath,
18+
getArtifactRoot,
1819
getProjectDir,
1920
getStatusPath,
2021
detectProjectId,
@@ -39,7 +40,7 @@ import {
3940
allPlanPhasesComplete,
4041
isPlanPhaseComplete,
4142
} from './plan.js';
42-
import { getResolver, type ArtifactResolver } from './artifacts.js';
43+
import { getResolver, LocalResolver, type ArtifactResolver } from './artifacts.js';
4344
import {
4445
runPhaseChecks,
4546
formatCheckResults,
@@ -62,6 +63,29 @@ function section(title: string, content: string): string {
6263
return `\n${chalk.bold(title)}:\n${content}`;
6364
}
6465

66+
/**
67+
* Return a resolver scoped to `artifactRoot` when it differs from the caller's
68+
* cwd-rooted resolver. The incoming `resolver` is typically built from
69+
* `process.cwd()` (the main workspace), but `findStatusPath` may resolve a
70+
* project to a builder worktree under `.builders/`. Checks like `plan_exists`
71+
* must read from that worktree, not from the main tree (bugfix #676).
72+
*
73+
* For the LOCAL backend we rebuild the resolver against `artifactRoot` so file
74+
* lookups point at `<artifactRoot>/codev/...`. For other backends (e.g. CLI)
75+
* artifact location is already independent of the filesystem, so we keep the
76+
* caller's resolver.
77+
*/
78+
function scopeResolver(
79+
workspaceRoot: string,
80+
artifactRoot: string,
81+
resolver?: ArtifactResolver,
82+
): ArtifactResolver | undefined {
83+
if (artifactRoot === workspaceRoot) return resolver;
84+
// Only rebuild if the existing resolver is a LocalResolver (path-dependent).
85+
if (resolver && !(resolver instanceof LocalResolver)) return resolver;
86+
return getResolver(workspaceRoot, artifactRoot);
87+
}
88+
6589
/**
6690
* Log override/skip notices before running checks.
6791
* Only emits output when overrides are actually in use.
@@ -206,6 +230,12 @@ export async function check(workspaceRoot: string, projectId: string, resolver?:
206230
throw new Error(`Project ${projectId} not found.`);
207231
}
208232

233+
// Scope artifact reads + check cwd to the worktree that owns this status.yaml.
234+
// `findStatusPath` searches `.builders/*` first; resolver/cwd must match so
235+
// checks like `plan_exists` see files in the same worktree (bugfix #676).
236+
const artifactRoot = getArtifactRoot(statusPath);
237+
const scopedResolver = scopeResolver(workspaceRoot, artifactRoot, resolver);
238+
209239
const state = readState(statusPath);
210240
const protocol = loadProtocol(workspaceRoot, state.protocol);
211241
const overrides = loadCheckOverrides(workspaceRoot);
@@ -218,7 +248,7 @@ export async function check(workspaceRoot: string, projectId: string, resolver?:
218248
return;
219249
}
220250

221-
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(workspaceRoot, state.id, state.title, resolver) };
251+
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(artifactRoot, state.id, state.title, scopedResolver) };
222252

223253
console.log('');
224254
console.log(chalk.bold('RUNNING CHECKS...'));
@@ -234,7 +264,7 @@ export async function check(workspaceRoot: string, projectId: string, resolver?:
234264
return;
235265
}
236266

237-
const results = await runPhaseChecks(checks, workspaceRoot, checkEnv, undefined, resolver);
267+
const results = await runPhaseChecks(checks, artifactRoot, checkEnv, undefined, scopedResolver);
238268
console.log(formatCheckResults(results));
239269

240270
console.log('');
@@ -291,6 +321,11 @@ export async function done(workspaceRoot: string, projectId: string, resolver?:
291321
const phaseCheckNames = phaseConfig?.checks ?? [];
292322
const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined);
293323

324+
// Scope artifact reads + check cwd to the worktree that owns this status.yaml
325+
// (bugfix #676 — see check() for rationale).
326+
const artifactRoot = getArtifactRoot(statusPath);
327+
const scopedResolver = scopeResolver(workspaceRoot, artifactRoot, resolver);
328+
294329
// Run checks first — but skip if the gate was just approved (approve already ran them)
295330
if (phaseCheckNames.length > 0) {
296331
const gate = getPhaseGate(protocol, state.phase);
@@ -302,14 +337,14 @@ export async function done(workspaceRoot: string, projectId: string, resolver?:
302337
console.log('');
303338
console.log(chalk.dim('Checks skipped (gate approved <60s ago).'));
304339
} else {
305-
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(workspaceRoot, state.id, state.title, resolver) };
340+
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(artifactRoot, state.id, state.title, scopedResolver) };
306341

307342
console.log('');
308343
console.log(chalk.bold('RUNNING CHECKS...'));
309344
logCheckOverrides(phaseCheckNames, checks, overrides);
310345

311346
if (Object.keys(checks).length > 0) {
312-
const results = await runPhaseChecks(checks, workspaceRoot, checkEnv, undefined, resolver);
347+
const results = await runPhaseChecks(checks, artifactRoot, checkEnv, undefined, scopedResolver);
313348
console.log(formatCheckResults(results));
314349

315350
if (!allChecksPassed(results)) {
@@ -422,7 +457,7 @@ export async function done(workspaceRoot: string, projectId: string, resolver?:
422457
}
423458

424459
// Advance to next protocol phase
425-
await advanceProtocolPhase(workspaceRoot, state, protocol, statusPath, resolver);
460+
await advanceProtocolPhase(workspaceRoot, state, protocol, statusPath, scopedResolver);
426461
}
427462

428463
async function advanceProtocolPhase(workspaceRoot: string, state: ProjectState, protocol: Protocol, statusPath: string, resolver?: ArtifactResolver): Promise<void> {
@@ -569,6 +604,11 @@ export async function approve(
569604
throw new Error(`Project ${projectId} not found.`);
570605
}
571606

607+
// Scope artifact reads + check cwd to the worktree that owns this status.yaml
608+
// (bugfix #676 — see check() for rationale).
609+
const artifactRoot = getArtifactRoot(statusPath);
610+
const scopedResolver = scopeResolver(workspaceRoot, artifactRoot, resolver);
611+
572612
const state = readState(statusPath);
573613

574614
// Convenience: for verify-approval, auto-complete porch done if build_complete is false
@@ -616,14 +656,14 @@ export async function approve(
616656
const checks = getPhaseChecks(protocol, state.phase, overrides ?? undefined);
617657

618658
if (phaseCheckNames.length > 0) {
619-
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(workspaceRoot, state.id, state.title, resolver) };
659+
const checkEnv: CheckEnv = { PROJECT_ID: state.id, PROJECT_TITLE: resolveArtifactBaseName(artifactRoot, state.id, state.title, scopedResolver) };
620660

621661
console.log('');
622662
console.log(chalk.bold('RUNNING CHECKS...'));
623663
logCheckOverrides(phaseCheckNames, checks, overrides);
624664

625665
if (Object.keys(checks).length > 0) {
626-
const results = await runPhaseChecks(checks, workspaceRoot, checkEnv, undefined, resolver);
666+
const results = await runPhaseChecks(checks, artifactRoot, checkEnv, undefined, scopedResolver);
627667
console.log(formatCheckResults(results));
628668

629669
if (!allChecksPassed(results)) {

‎packages/codev/src/commands/porch/state.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,19 @@ export function getStatusPath(workspaceRoot: string, projectId: string, name: st
9494
return path.join(getProjectDir(workspaceRoot, projectId, name), 'status.yaml');
9595
}
9696

97+
/**
98+
* Derive the artifact root (worktree root) from a status.yaml path.
99+
* Status paths are always <artifactRoot>/codev/projects/<id>-<name>/status.yaml,
100+
* so the artifact root is three levels up from the containing directory.
101+
*
102+
* Used by commands that must resolve specs/plans/reviews relative to the
103+
* worktree that owns the status file (bugfix #676). `findStatusPath` already
104+
* searches `.builders/*` first, so this matches its resolution.
105+
*/
106+
export function getArtifactRoot(statusPath: string): string {
107+
return path.resolve(path.dirname(statusPath), '..', '..', '..');
108+
}
109+
97110
// ============================================================================
98111
// State Operations
99112
// ============================================================================

0 commit comments

Comments
 (0)