fix: address Phase 5 deep PR review findings (3 HIGH, 4 MEDIUM)

- R2-H1: Probe exec errors (EACCES) now return 'unknown' not 'missing',
  preventing trust boundary violation where probe failure → missing →
  auto_approvable brew install. PackageInstallService also catches
  planner throws.
- R2-H2: Failed plan returns 400/409/500 based on structured stage codes
  (file_not_found/validation_failed → 400). No bootstrap.planned emitted
  on failure — emits bootstrap.failed instead.
- R2-H3: Apply route wraps orchestrator in try/catch. On exception:
  updates run to 'failed', emits bootstrap.failed, returns 500.
- R1-F4.6: Route-level concurrency lock via tryAcquire/release on both
  plan and apply routes. Lock key normalized via path.resolve.
- R1-F4.3: Package install failure now skips rig import (status='failed').
- R2-M1: CLI partial result → exit 1 (not 0).
- R2-M2: Step indicator shows REVIEW (3) for planned state. Applying step
  renders stage checklist from plan.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Mike SchwarzandClaude Opus 4.6 committed 2026-03-26 02:22:48 -07:00
1 parent 3e4d1c4228
commit aaa51aeae1
11 files changed
+393 -55

No files matched your search

+3
View File
@@ -80,10 +80,13 @@ export function bootstrapCommand(depsOverride?: StatusDeps): Command {
}
}
const resultStatus = (res.data["status"] as string) ?? "";
if (res.status === 409) {
process.exitCode = 1; // blocked
} else if (res.status >= 500) {
process.exitCode = 2; // failure
} else if (resultStatus === "partial") {
process.exitCode = 1; // partial is not clean success
}
});
+25
View File
@@ -204,4 +204,29 @@ describe("Bootstrap CLI", () => {
const body = JSON.parse(lastReq.body);
expect(body.autoApprove).toBe(false);
});
// T9: partial status -> exit 1 (R2-M1)
it("partial result returns exit code 1", async () => {
const partialServer = http.createServer((_, res) => {
res.writeHead(200, { "Content-Type": "application/json" });
res.end(JSON.stringify({
runId: "run-p", status: "partial", rigId: "rig-p",
stages: [{ stage: "execute_external_installs", status: "failed" }, { stage: "import_rig", status: "ok" }],
errors: ["brew install failed"], warnings: [],
}));
});
await new Promise<void>((resolve) => { partialServer.listen(0, resolve); });
const partialPort = (partialServer.address() as { port: number }).port;
const prog = new Command();
prog.exitOverride();
prog.addCommand(bootstrapCommand(runningDeps(partialPort)));
const { exitCode } = await captureLogs(async () => {
await prog.parseAsync(["node", "rigged", "bootstrap", "/tmp/rig.yaml", "--yes"]);
});
expect(exitCode).toBe(1);
partialServer.close();
});
});
@@ -70,6 +70,20 @@ function actionKey(actionKind: string, subjectType: string | null, subjectName:
*/
export class BootstrapOrchestrator {
private deps: BootstrapOrchestratorDeps;
private activeLocks = new Set<string>();
/** Try to acquire the lock for a sourceRef. Returns false if already locked. */
tryAcquire(sourceRef: string): boolean {
const key = nodePath.resolve(sourceRef);
if (this.activeLocks.has(key)) return false;
this.activeLocks.add(key);
return true;
}
/** Release the lock for a sourceRef. */
release(sourceRef: string): void {
this.activeLocks.delete(nodePath.resolve(sourceRef));
}
constructor(deps: BootstrapOrchestratorDeps) {
// Same-db-handle checks
@@ -101,7 +115,7 @@ export class BootstrapOrchestrator {
const raw = RigSpecCodec.parse(rawYaml);
const validation = RigSpecSchema.validate(raw);
if (!validation.valid) {
stages.push({ stage: "resolve_spec", status: "failed", detail: { errors: validation.errors } });
stages.push({ stage: "resolve_spec", status: "failed", detail: { code: "validation_failed", errors: validation.errors } });
this.deps.bootstrapRepo.updateRunStatus(run.id, "failed");
return { runId: run.id, status: "failed", stages, errors: validation.errors, warnings };
}
@@ -109,7 +123,10 @@ export class BootstrapOrchestrator {
stages.push({ stage: "resolve_spec", status: "ok", detail: { specName: spec.name, specVersion: spec.schemaVersion } });
} catch (err) {
const msg = (err as Error).message;
stages.push({ stage: "resolve_spec", status: "failed", detail: { error: msg } });
const code = (err as NodeJS.ErrnoException).code === "ENOENT" ? "file_not_found"
: msg.includes("YAML") || msg.includes("parse") ? "parse_error"
: "read_error";
stages.push({ stage: "resolve_spec", status: "failed", detail: { code, error: msg } });
errors.push(msg);
this.deps.bootstrapRepo.updateRunStatus(run.id, "failed");
return { runId: run.id, status: "failed", stages, errors, warnings };
@@ -363,6 +380,14 @@ export class BootstrapOrchestrator {
});
// --- Stage 8: IMPORT_RIG ---
// Skip rig import if package installs failed — a rig without its packages is broken
if (packageInstallFailed) {
errors.push("Rig import skipped due to package install failures");
stages.push({ stage: "import_rig", status: "skipped", detail: { reason: "package install failures" } });
this.deps.bootstrapRepo.updateRunStatus(run.id, "failed");
return { runId: run.id, status: "failed", stages, errors, warnings };
}
const instantiateOutcome = await this.deps.rigInstantiator.instantiate(spec);
this.deps.bootstrapRepo.journalAction(run.id, seqCounter++, "rig_import", null, spec.name, instantiateOutcome.ok ? "completed" : "failed", {
@@ -57,9 +57,14 @@ export class PackageInstallService {
const { resolved, targetRoot, runtime, roleName, allowMerge, bootstrapId, fsOps } = opts;
// Plan + detect conflicts
const planner = new InstallPlanner(fsOps);
const plan = planner.plan(resolved, targetRoot, runtime, { roleName });
const refined = detectConflicts(plan, fsOps);
let plan, refined;
try {
const planner = new InstallPlanner(fsOps);
plan = planner.plan(resolved, targetRoot, runtime, { roleName });
refined = detectConflicts(plan, fsOps);
} catch (err) {
return { ok: false, code: "apply_error", message: (err as Error).message };
}
// Check for content-level conflicts
if (refined.conflicts.length > 0) {
@@ -73,17 +73,27 @@ export class RequirementsProbeRegistry {
error: null,
};
} catch (err) {
if ((err as Error).message?.includes("timed out")) {
const msg = (err as Error).message ?? "";
if (msg.includes("timed out")) {
return {
name, kind: "cli_tool", status: "unknown", version: null,
detectedPath: null, provider: null, command: cmd,
installHints: null, error: "probe timed out",
};
}
// Only treat "not found" / exit-code errors as genuinely missing.
// Other errors (EACCES, etc) are unknown — must not become auto_approvable.
if (msg.includes("not found") || msg.includes("No such") || msg.includes("exit code")) {
return {
name, kind: "cli_tool", status: "missing", version: null,
detectedPath: null, provider: null, command: cmd,
installHints: null, error: null,
};
}
return {
name, kind: "cli_tool", status: "missing", version: null,
name, kind: "cli_tool", status: "unknown", version: null,
detectedPath: null, provider: null, command: cmd,
installHints: null, error: null,
installHints: null, error: msg,
};
}
}
@@ -112,17 +122,25 @@ export class RequirementsProbeRegistry {
error: null,
};
} catch (err) {
if ((err as Error).message?.includes("timed out")) {
const msg = (err as Error).message ?? "";
if (msg.includes("timed out")) {
return {
name, kind: "system_package", status: "unknown", version: null,
detectedPath: null, provider: "homebrew", command: cmd,
installHints: null, error: "probe timed out",
};
}
if (msg.includes("No such keg") || msg.includes("not found") || msg.includes("exit code")) {
return {
name, kind: "system_package", status: "missing", version: null,
detectedPath: null, provider: "homebrew", command: cmd,
installHints: null, error: null,
};
}
return {
name, kind: "system_package", status: "missing", version: null,
name, kind: "system_package", status: "unknown", version: null,
detectedPath: null, provider: "homebrew", command: cmd,
installHints: null, error: null,
installHints: null, error: msg,
};
}
}
+88 -39
View File
@@ -1,3 +1,4 @@
import nodePath from "node:path";
import { Hono } from "hono";
import type { BootstrapOrchestrator } from "../domain/bootstrap-orchestrator.js";
import type { BootstrapRepository } from "../domain/bootstrap-repository.js";
@@ -13,6 +14,23 @@ function getDeps(c: { get: (key: string) => unknown }) {
};
}
/** Map failed plan result to HTTP status using structured stage codes */
function planFailureStatus(result: { stages: Array<{ stage: string; status: string; detail: unknown }> }): number {
const failedStage = result.stages.find((s) => s.status === "failed" || s.status === "blocked");
if (!failedStage) return 500;
if (failedStage.stage === "resolve_spec") {
const detail = failedStage.detail as { code?: string } | undefined;
if (detail?.code === "file_not_found" || detail?.code === "parse_error" || detail?.code === "validation_failed") {
return 400;
}
return 500;
}
if (failedStage.status === "blocked") return 409;
return 500;
}
// POST /api/bootstrap/plan
bootstrapRoutes.post("/plan", async (c) => {
const { bootstrapOrchestrator, eventBus } = getDeps(c);
@@ -23,20 +41,34 @@ bootstrapRoutes.post("/plan", async (c) => {
return c.json({ error: "sourceRef is required" }, 400);
}
const result = await bootstrapOrchestrator.bootstrap({
mode: "plan",
sourceRef,
sourceKind: typeof body["sourceKind"] === "string" ? body["sourceKind"] : undefined,
});
// Concurrency lock — consistent state during plan
if (!bootstrapOrchestrator.tryAcquire(sourceRef)) {
return c.json({ error: "Bootstrap already in progress for this spec", code: "conflict" }, 409);
}
eventBus.emit({
type: "bootstrap.planned",
runId: result.runId,
sourceRef,
stages: result.stages.length,
});
try {
const result = await bootstrapOrchestrator.bootstrap({
mode: "plan",
sourceRef,
sourceKind: typeof body["sourceKind"] === "string" ? body["sourceKind"] : undefined,
});
return c.json(result, 200);
if (result.status === "planned") {
eventBus.emit({
type: "bootstrap.planned",
runId: result.runId,
sourceRef,
stages: result.stages.length,
});
return c.json(result, 200);
}
// Plan failed — emit failed event, return appropriate status
eventBus.emit({ type: "bootstrap.failed", runId: result.runId, sourceRef, error: result.errors[0] ?? "plan failed" });
return c.json(result, planFailureStatus(result));
} finally {
bootstrapOrchestrator.release(sourceRef);
}
});
// POST /api/bootstrap/apply
@@ -49,39 +81,56 @@ bootstrapRoutes.post("/apply", async (c) => {
return c.json({ error: "sourceRef is required" }, 400);
}
// Concurrency lock — acquire before creating run
if (!bootstrapOrchestrator.tryAcquire(sourceRef)) {
return c.json({ error: "Bootstrap already in progress for this spec", code: "conflict" }, 409);
}
const sourceKind = typeof body["sourceKind"] === "string" ? body["sourceKind"] : "rig_spec";
const autoApprove = body["autoApprove"] === true;
const approvedActionKeys = Array.isArray(body["approvedActionKeys"]) ? body["approvedActionKeys"] as string[] : undefined;
// Pre-create run + set running + emit started BEFORE orchestrator work
const run = bootstrapRepo.createRun(sourceKind, sourceRef);
bootstrapRepo.updateRunStatus(run.id, "running");
eventBus.emit({ type: "bootstrap.started", runId: run.id, sourceRef });
try {
// Pre-create run + set running + emit started BEFORE orchestrator work
const run = bootstrapRepo.createRun(sourceKind, sourceRef);
bootstrapRepo.updateRunStatus(run.id, "running");
eventBus.emit({ type: "bootstrap.started", runId: run.id, sourceRef });
const result = await bootstrapOrchestrator.bootstrap({
mode: "apply",
sourceRef,
sourceKind,
autoApprove,
approvedActionKeys,
runId: run.id,
});
let result;
try {
result = await bootstrapOrchestrator.bootstrap({
mode: "apply",
sourceRef,
sourceKind,
autoApprove,
approvedActionKeys,
runId: run.id,
});
} catch (err) {
// Exception boundary: update run to failed, emit failed event
bootstrapRepo.updateRunStatus(run.id, "failed");
const errorMsg = (err as Error).message ?? "bootstrap failed";
eventBus.emit({ type: "bootstrap.failed", runId: run.id, sourceRef, error: errorMsg });
return c.json({ runId: run.id, status: "failed", error: errorMsg }, 500);
}
// Emit outcome event
if (result.status === "completed") {
eventBus.emit({ type: "bootstrap.completed", runId: result.runId, rigId: result.rigId!, sourceRef });
return c.json(result, 201);
} else if (result.status === "partial") {
const completedCount = result.stages.filter((s) => s.status === "ok").length;
const failedCount = result.stages.filter((s) => s.status === "failed" || s.status === "blocked").length;
eventBus.emit({ type: "bootstrap.partial", runId: result.runId, sourceRef, rigId: result.rigId, completed: completedCount, failed: failedCount });
return c.json(result, 200);
} else {
const errorMsg = result.errors[0] ?? "bootstrap failed";
eventBus.emit({ type: "bootstrap.failed", runId: result.runId, sourceRef, error: errorMsg });
// 409 if any stage was blocked, 500 otherwise
const hasBlocked = result.stages.some((s) => s.status === "blocked");
return c.json(result, hasBlocked ? 409 : 500);
// Emit outcome event
if (result.status === "completed") {
eventBus.emit({ type: "bootstrap.completed", runId: result.runId, rigId: result.rigId!, sourceRef });
return c.json(result, 201);
} else if (result.status === "partial") {
const completedCount = result.stages.filter((s) => s.status === "ok").length;
const failedCount = result.stages.filter((s) => s.status === "failed" || s.status === "blocked").length;
eventBus.emit({ type: "bootstrap.partial", runId: result.runId, sourceRef, rigId: result.rigId, completed: completedCount, failed: failedCount });
return c.json(result, 200);
} else {
const errorMsg = result.errors[0] ?? "bootstrap failed";
eventBus.emit({ type: "bootstrap.failed", runId: result.runId, sourceRef, error: errorMsg });
const hasBlocked = result.stages.some((s) => s.status === "blocked");
return c.json(result, hasBlocked ? 409 : 500);
}
} finally {
bootstrapOrchestrator.release(sourceRef);
}
});
@@ -947,4 +947,36 @@ requirements:
expect(missingResult).toBeDefined();
expect(missingResult!.status).toBe("missing");
});
// T24: Package install failure skips rig import (R1-F4.3)
it("package install failure skips rig import with status=failed", async () => {
// Create a package that will fail install (incompatible runtime)
const pkgDir = path.join(tmpDir, "test-pkg");
const manifest = `
schema_version: 1
name: codex-only-pkg
version: "1.0.0"
summary: Only works on codex
compatibility:
runtimes:
- codex
exports:
skills:
- source: skills/h
name: h
supported_scopes: [project_shared]
default_scope: project_shared
`.trim();
writePkg(pkgDir, manifest, { "skills/h/SKILL.md": "# H" });
const specPath = writeSpec(SPEC_WITH_PACKAGES_YAML);
const orch = buildOrchestrator();
const result = await orch.bootstrap({ mode: "apply", sourceRef: specPath, autoApprove: true });
expect(result.status).toBe("failed");
// import_rig should be skipped
const importStage = result.stages.find((s) => s.stage === "import_rig");
expect(importStage?.status).toBe("skipped");
expect(result.errors.some((e) => e.includes("skipped"))).toBe(true);
});
});
@@ -268,4 +268,108 @@ describe("Bootstrap API routes", () => {
expect(payload.completed).toBe(3);
expect(payload.failed).toBe(1);
});
// T13: Failed plan returns 400 + no bootstrap.planned event (R2-H2)
it("POST /api/bootstrap/plan with nonexistent spec returns 400, no bootstrap.planned", async () => {
const res = await app.request("/api/bootstrap/plan", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ sourceRef: "/tmp/nonexistent-spec-12345.yaml" }),
});
expect(res.status).toBe(400);
const body = await res.json();
expect(body.status).toBe("failed");
// Should NOT have emitted bootstrap.planned
const plannedEvents = getEvents(db).filter((e) => e.type === "bootstrap.planned");
expect(plannedEvents).toHaveLength(0);
// Should have emitted bootstrap.failed instead
const failedEvents = getEvents(db).filter((e) => e.type === "bootstrap.failed");
expect(failedEvents.length).toBeGreaterThanOrEqual(1);
});
// T14: Apply exception boundary — thrown error gets caught (R2-H3)
it("POST /api/bootstrap/apply catches thrown orchestrator error", async () => {
// Temporarily replace the orchestrator's bootstrap method with one that throws
const origBootstrap = setup.bootstrapOrchestrator.bootstrap.bind(setup.bootstrapOrchestrator);
(setup.bootstrapOrchestrator as unknown as { bootstrap: unknown }).bootstrap = async () => {
throw new Error("unexpected planner crash");
};
const specPath = writeSpec(SIMPLE_SPEC_YAML);
const res = await app.request("/api/bootstrap/apply", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ sourceRef: specPath, autoApprove: true }),
});
expect(res.status).toBe(500);
const body = await res.json();
expect(body.status).toBe("failed");
expect(body.error).toContain("unexpected planner crash");
// bootstrap.started should have been emitted (before the throw)
const startedEvents = getEvents(db).filter((e) => e.type === "bootstrap.started");
expect(startedEvents.length).toBeGreaterThanOrEqual(1);
// bootstrap.failed should also have been emitted (exception boundary)
const failedEvents = getEvents(db).filter((e) => e.type === "bootstrap.failed");
expect(failedEvents.length).toBeGreaterThanOrEqual(1);
// Run should be 'failed', not stuck in 'running'
const run = db.prepare("SELECT status FROM bootstrap_runs WHERE id = ?")
.get(body.runId) as { status: string };
expect(run.status).toBe("failed");
// Restore
(setup.bootstrapOrchestrator as unknown as { bootstrap: unknown }).bootstrap = origBootstrap;
});
// T15: Concurrency lock — second apply returns 409 with no run/started (R1-F4.6)
it("concurrent apply for same spec returns 409 with no run created", async () => {
const specPath = writeSpec(SIMPLE_SPEC_YAML);
// Acquire the lock manually
setup.bootstrapOrchestrator.tryAcquire(specPath);
const res = await app.request("/api/bootstrap/apply", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ sourceRef: specPath, autoApprove: true }),
});
expect(res.status).toBe(409);
const body = await res.json();
expect(body.code).toBe("conflict");
// No bootstrap.started event should have been emitted for this request
// (The lock blocks before run creation)
const startedEvents = getEvents(db).filter((e) => e.type === "bootstrap.started");
expect(startedEvents).toHaveLength(0);
// Release for cleanup
setup.bootstrapOrchestrator.release(specPath);
});
// T16: Concurrent plan for same spec conflicts
it("concurrent plan for same spec returns 409", async () => {
const specPath = writeSpec(SIMPLE_SPEC_YAML);
// Acquire the lock manually (simulating a concurrent plan in progress)
setup.bootstrapOrchestrator.tryAcquire(specPath);
const res = await app.request("/api/bootstrap/plan", {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ sourceRef: specPath }),
});
expect(res.status).toBe(409);
const body = await res.json();
expect(body.code).toBe("conflict");
setup.bootstrapOrchestrator.release(specPath);
});
});
@@ -184,4 +184,31 @@ describe("RequirementsProbeRegistry", () => {
expect(result.installHints).toEqual(hints);
expect(result.status).toBe("installed");
});
// T13: EACCES probe error -> unknown (not missing) — trust boundary fix
it("EACCES probe error returns unknown, not missing", async () => {
const exec = vi.fn(async () => { throw new Error("EACCES: permission denied"); }) as unknown as ExecFn;
const registry = new RequirementsProbeRegistry(exec);
const result = await registry.probeCli("rg");
expect(result.status).toBe("unknown");
expect(result.error).toContain("EACCES");
});
// T14: unknown status does NOT become auto_approvable via planner
it("unknown probe status maps to manual_only in planner, not auto_approvable", async () => {
const { ExternalInstallPlanner } = await import("../src/domain/external-install-planner.js");
const planner = new ExternalInstallPlanner({ platform: "darwin" });
const probeResult = {
name: "rg", kind: "cli_tool" as const, status: "unknown" as const,
version: null, detectedPath: null, provider: null, command: null,
installHints: null, error: "EACCES: permission denied",
};
const plan = planner.planInstalls([probeResult]);
expect(plan.manualOnly).toHaveLength(1);
expect(plan.autoApprovable).toHaveLength(0);
});
});
+27 -5
View File
@@ -31,7 +31,8 @@ function StepIndicator({ current }: { current: number }) {
function currentStepNumber(step: Step): number {
switch (step) {
case "enter": return 1;
case "planning": case "planned": return 2;
case "planning": return 2;
case "planned": return 3;
case "applying": return 4;
case "done": case "error": return 4;
default: return 1;
@@ -223,10 +224,31 @@ export function BootstrapWizard() {
</div>
)}
{/* Step 4: Applying */}
{step === "applying" && (
<div data-testid="step-applying" className="text-body-md text-foreground-muted">
Bootstrapping...
{/* Step 4: Applying — show stage checklist from plan */}
{step === "applying" && planResult && (
<div data-testid="step-applying">
<h3 className="text-headline-md uppercase mb-spacing-3">APPLYING</h3>
<div className="space-y-spacing-1 mb-spacing-4" data-testid="applying-checklist">
{planResult.stages.map((s) => (
<div key={s.stage} className="flex items-center gap-spacing-3 text-label-sm font-mono">
<span className="text-foreground-muted">○</span>
<span>{s.stage}</span>
</div>
))}
<div className="flex items-center gap-spacing-3 text-label-sm font-mono">
<span className="text-foreground-muted">○</span>
<span>execute_external_installs</span>
</div>
<div className="flex items-center gap-spacing-3 text-label-sm font-mono">
<span className="text-foreground-muted">○</span>
<span>install_packages</span>
</div>
<div className="flex items-center gap-spacing-3 text-label-sm font-mono">
<span className="text-foreground-muted">○</span>
<span>import_rig</span>
</div>
</div>
<p className="text-body-sm text-foreground-muted">Bootstrapping...</p>
</div>
)}
@@ -88,6 +88,10 @@ describe("BootstrapWizard", () => {
const rows = screen.getAllByTestId("stage-row");
expect(rows.length).toBeGreaterThanOrEqual(2);
// Step indicator should show REVIEW (step 3), not PLAN (step 2)
const indicator = screen.getByTestId("step-indicator");
expect(indicator.textContent).toContain("3 REVIEW");
});
// T3: requirements panel shows all 4 statuses with correct colors
@@ -177,6 +181,30 @@ describe("BootstrapWizard", () => {
});
});
// T8b: applying step shows stage checklist from plan
it("applying step renders stage checklist from plan", async () => {
fetchMock
.mockResolvedValueOnce({ ok: true, json: async () => PLAN_RESPONSE })
.mockImplementationOnce(() => new Promise(() => {})); // Never resolves — stays in applying
renderWizard();
await waitFor(() => expect(screen.getByTestId("spec-input")).toBeTruthy());
act(() => { fireEvent.change(screen.getByTestId("spec-input"), { target: { value: "/tmp/rig.yaml" } }); });
act(() => { fireEvent.click(screen.getByTestId("plan-btn")); });
await waitFor(() => expect(screen.getByTestId("apply-btn")).toBeTruthy());
act(() => { fireEvent.click(screen.getByTestId("apply-btn")); });
await waitFor(() => {
expect(screen.getByTestId("step-applying")).toBeTruthy();
expect(screen.getByTestId("applying-checklist")).toBeTruthy();
});
// Should show stages from the plan + apply-only stages
const checklist = screen.getByTestId("applying-checklist");
expect(checklist.textContent).toContain("resolve_spec");
expect(checklist.textContent).toContain("import_rig");
});
// T9: apply button disabled when plan is blocked
it("apply button disabled when plan has blocked status", async () => {
const blockedPlan = {