diff --git a/.changeset/fn-8768-review-followups.md b/.changeset/fn-8768-review-followups.md new file mode 100644 index 0000000000..d824e45b50 --- /dev/null +++ b/.changeset/fn-8768-review-followups.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Prevent stale plan approvals and strengthen planning and review completeness. +category: fix +dev: Adds bounded lifecycle locks, planning-episode evidence, approval serialization, and convergent planning/review ledgers. diff --git a/packages/core/src/__test-utils__/pg-test-harness.ts b/packages/core/src/__test-utils__/pg-test-harness.ts index f461486fdb..162fa76ff9 100644 --- a/packages/core/src/__test-utils__/pg-test-harness.ts +++ b/packages/core/src/__test-utils__/pg-test-harness.ts @@ -909,6 +909,8 @@ export async function usePgTaskStore( export interface SharedPgTaskStoreHarness { readonly rootDir: () => string; readonly globalDir: () => string; + /** Direct connection URL for session-level PostgreSQL primitive tests. */ + readonly testUrl: () => string; readonly store: () => TaskStore; readonly layer: () => AsyncDataLayer; readonly adminDb: () => PostgresJsDatabase; @@ -978,6 +980,7 @@ export function createSharedPgTaskStoreTestHarness(options?: { return { rootDir: () => harness?.rootDir ?? "", globalDir: () => harness?.rootDir ?? "", + testUrl: () => harness?.testUrl ?? "", store: () => { if (!store) throw new Error("SharedPgTaskStoreHarness: beforeAll not called yet"); return store; @@ -1103,4 +1106,3 @@ export function createSharedPgTaskStoreTestHarness(options?: { }, }; } - diff --git a/packages/core/src/__tests__/agent-prompts.test.ts b/packages/core/src/__tests__/agent-prompts.test.ts index 790ae4c8b9..cf65dd89a3 100644 --- a/packages/core/src/__tests__/agent-prompts.test.ts +++ b/packages/core/src/__tests__/agent-prompts.test.ts @@ -62,28 +62,40 @@ describe("resolveAgentPrompt", () => { expect(result).not.toContain("turn it into short, actionable task specs or follow-up tickets"); }); + /* + FNXC:ReviewPromptBoundaries 2026-08-04-06:35: + The base reviewer remains role-neutral. Planning and code-review completeness + policies are injected only at their workflow seams, avoiding mixed contracts. + */ it("returns the correct built-in prompt for reviewer when no config provided", () => { const result = resolveAgentPrompt("reviewer"); expect(result).toBeTruthy(); expect(result).toContain("independent code and plan reviewer"); - // FNXC:PlanReviewReplan 2026-07-15-11:15: convergence guidance for Plan Review REVISE thrash. - expect(result).toContain("Spec / Plan Review Convergence"); - expect(result).toContain("concrete PROMPT.md edit"); + expect(result).not.toContain("## Mandatory Plan Review Procedure"); + expect(result).not.toContain("Mandatory Code Review Procedure"); }); - // FNXC:TriagePlanReviewConvergence 2026-07-16-19:40: lock the new triage-side planner sections. it("includes the front-loaded File Scope and Storage architecture sections in the triage prompt", () => { const result = resolveAgentPrompt("triage"); + expect(result).toContain("## Mandatory Planning Completeness Procedure"); + expect(result).toContain("planning ledger"); + expect(result).toContain("participant graph and both relevant orderings"); + expect(result).toContain("fresh holistic completeness pass"); expect(result).toContain("## File Scope — front-load surface enumeration"); expect(result).toContain("## Storage architecture"); }); - // FNXC:TriagePlanReviewConvergence 2026-07-16-19:40: lock the new reviewer-side spec convergence sections. + it("keeps the fast planning seam lean while preserving completeness invariants", () => { + const result = builtinSeamPrompt("planning-fast"); + expect(result).toContain("## Fast Planning Completeness Check"); + expect(result).toContain("both relevant orderings and failure cleanup"); + expect(result).toContain("complete cumulative ledger"); + }); + it("includes Spec Altitude and re-review convergence sections in the reviewer prompt", () => { const result = resolveAgentPrompt("reviewer"); expect(result).toContain("## Spec Altitude"); - expect(result).toContain("Converging on re-review"); - expect(result).toContain("Severity ratchet at attempt 3+"); + expect(result).not.toContain("prior-review ledger as a decision primer"); }); it("returns the correct built-in prompt for merger when no config provided", () => { @@ -339,9 +351,12 @@ describe("resolveAgentPrompt", () => { expect(fastPrompt).toContain("Do not write bare `### Preflight` / `### Implementation` headings"); expect(fastPrompt).not.toContain("## Review Level"); expect(fastPrompt.length).toBeLessThan(standardPrompt.length / 3); - // FNXC:OriginalDescriptionInPrompt 2026-07-14-23:35: Original Description contract - // adds a few lines to fast planning; keep lean but allow the new mandatory section. - expect(fastPrompt.length).toBeLessThan(6500); + /* + * FNXC:FastPlanningPrompt 2026-08-04-06:35: + * The compact ledger may add one bounded paragraph, while both relative and + * absolute caps keep fast planning materially smaller than the full prompt. + */ + expect(fastPrompt.length).toBeLessThan(7500); expect(fastPrompt.split("\n").length).toBeLessThan(120); }); diff --git a/packages/core/src/__tests__/builtin-code-review-group.test.ts b/packages/core/src/__tests__/builtin-code-review-group.test.ts index 03b4eeb56a..86b5cd0b1e 100644 --- a/packages/core/src/__tests__/builtin-code-review-group.test.ts +++ b/packages/core/src/__tests__/builtin-code-review-group.test.ts @@ -43,6 +43,18 @@ describe("codeReviewOptionalGroupNode", () => { expect(prompt).not.toContain('"verdict":"FAIL"'); expect(prompt).toMatch(/git diff/); expect(prompt).toMatch(/out of scope/i); + /* + * FNXC:CodeReviewSurfaceCoverage 2026-08-04-06:35: + * The built-in gate must trace requirements through production entry points, + * temporal state, and every UI/API/CLI/agent consumer rather than diff only. + */ + expect(prompt).toContain("requirements ledger"); + expect(prompt).toContain("real production entry point"); + expect(prompt).toContain("## Symptom Verification"); + expect(prompt).toContain("## Surface Enumeration"); + expect(prompt).toContain("current state, version, or planning episode"); + expect(prompt).toContain("bounded"); + expect(prompt).toContain("UI, API, CLI, and agent consumers"); }); it("builds a DEFAULT-ON optional-group with the stable group id and distinct inner id", () => { diff --git a/packages/core/src/__tests__/builtin-workflows.test.ts b/packages/core/src/__tests__/builtin-workflows.test.ts index 43fac63fd5..a495828fb7 100644 --- a/packages/core/src/__tests__/builtin-workflows.test.ts +++ b/packages/core/src/__tests__/builtin-workflows.test.ts @@ -659,6 +659,12 @@ describe("built-in workflows", () => { toolMode: "readonly", gateMode: "gate", }); + const planReviewPrompt = String(planReviewInnerConfig(ir).prompt); + expect(planReviewPrompt).toContain("## Mandatory Plan Review Procedure"); + expect(planReviewPrompt).toContain("all independently discoverable blocking findings"); + expect(planReviewPrompt).toContain("prior-review ledger as a decision primer"); + expect(planReviewPrompt).toContain("never demote a critical defect merely because it was missed before"); + expect(planReviewPrompt).toContain("verdict notes must contain the complete blocking checklist"); expect(byId.get("parse")?.column).toBe("in-progress"); expect(byId.get("steps")?.column).toBe("in-progress"); /* diff --git a/packages/core/src/__tests__/plan-approval.test.ts b/packages/core/src/__tests__/plan-approval.test.ts index bca43e624b..87d770729b 100644 --- a/packages/core/src/__tests__/plan-approval.test.ts +++ b/packages/core/src/__tests__/plan-approval.test.ts @@ -68,6 +68,28 @@ describe("isPlanReviewSatisfied", () => { bypassedFromVerdict: "REVISE", })).toBe(false); }); + + it("rejects a historical pass or bypass superseded by a later planning episode", () => { + expect(isPlanReviewSatisfied({ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed", + supersededAt: "2026-08-04T02:00:00.000Z", + supersededReason: "dependency-change", + })).toBe(false); + expect(isPlanReviewSatisfied({ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "skipped", + bypassedBy: "operator", + bypassedAt: "2026-08-04T01:00:00.000Z", + bypassReason: "Approved at the old revision cap", + bypassedFromStatus: "failed", + bypassedFromVerdict: "REVISE", + supersededAt: "2026-08-04T02:00:00.000Z", + supersededReason: "dependency-change", + })).toBe(false); + }); }); /* diff --git a/packages/core/src/__tests__/postgres/planning-lifecycle-advisory-lock.pg.test.ts b/packages/core/src/__tests__/postgres/planning-lifecycle-advisory-lock.pg.test.ts new file mode 100644 index 0000000000..a545985803 --- /dev/null +++ b/packages/core/src/__tests__/postgres/planning-lifecycle-advisory-lock.pg.test.ts @@ -0,0 +1,165 @@ +import { afterAll, afterEach, beforeAll, expect, it } from "vitest"; + +import { + createSharedPgTaskStoreTestHarness, + pgDescribe, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import { + PlanningLifecycleLockTransportError, + withPlanningLifecycleAdvisoryLock, +} from "../../postgres/advisory-locks.js"; + +const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_planning_lock", + poolMax: 1, +}); + +function lock( + projectId: string, + taskId: string, + callback: () => Promise, + timeoutMs = 1_000, + onLockAcquisitionAttempt?: () => void, +): Promise { + return withPlanningLifecycleAdvisoryLock({ + projectId, + taskId, + directSessionUrl: h.testUrl(), + provenance: "migration-override", + runtimeUrl: h.testUrl(), + migrationUrl: h.testUrl(), + timeoutMs, + onLockAcquisitionAttempt, + }, callback); +} + +pgDescribe("planning lifecycle advisory lock", () => { + beforeAll(h.beforeAll); + afterAll(h.afterAll); + afterEach(h.afterEach); + + it("serializes the same project/task key on dedicated sessions", async () => { + let releaseFirst!: () => void; + const firstCanFinish = new Promise((resolve) => { releaseFirst = resolve; }); + let firstEntered!: () => void; + const firstIsHolding = new Promise((resolve) => { firstEntered = resolve; }); + const order: string[] = []; + + const first = lock("project-a", "FN-1", async () => { + order.push("first-enter"); + firstEntered(); + await firstCanFinish; + order.push("first-exit"); + }); + await firstIsHolding; + + let secondAttempted!: () => void; + const secondAttemptIsDispatched = new Promise((resolve) => { secondAttempted = resolve; }); + const second = lock("project-a", "FN-1", async () => { + order.push("second-enter"); + }, 1_000, secondAttempted); + + await secondAttemptIsDispatched; + expect(order).toEqual(["first-enter"]); + + releaseFirst(); + await Promise.all([first, second]); + expect(order).toEqual(["first-enter", "first-exit", "second-enter"]); + }); + + it("does not contend across project or task keys", async () => { + let releaseFirst!: () => void; + const firstCanFinish = new Promise((resolve) => { releaseFirst = resolve; }); + let firstEntered!: () => void; + const firstIsHolding = new Promise((resolve) => { firstEntered = resolve; }); + + const first = lock("project-a", "FN-1", async () => { + firstEntered(); + await firstCanFinish; + }); + await firstIsHolding; + + await Promise.all([ + lock("project-b", "FN-1", async () => {}), + lock("project-a", "FN-2", async () => {}), + ]); + releaseFirst(); + await first; + }); + + it("unlocks and closes the dedicated session when the callback throws", async () => { + await expect(lock("project-a", "FN-1", async () => { + throw new Error("callback failed"); + })).rejects.toThrow("callback failed"); + + await expect(lock("project-a", "FN-1", async () => {})).resolves.toBeUndefined(); + }); + + it.each(["embedded-lifecycle", "migration-override"] as const)( + "accepts a verified %s direct-session provenance", + async (provenance) => { + await expect(withPlanningLifecycleAdvisoryLock({ + projectId: "project-a", + taskId: "FN-1", + directSessionUrl: h.testUrl(), + provenance, + runtimeUrl: h.testUrl(), + migrationUrl: h.testUrl(), + timeoutMs: 1_000, + }, async () => {})).resolves.toBeUndefined(); + }, + ); + + it("bounds lock contention with a typed transport error", async () => { + let releaseFirst!: () => void; + const firstCanFinish = new Promise((resolve) => { releaseFirst = resolve; }); + let firstEntered!: () => void; + const firstIsHolding = new Promise((resolve) => { firstEntered = resolve; }); + const first = lock("project-a", "FN-1", async () => { + firstEntered(); + await firstCanFinish; + }); + await firstIsHolding; + + await expect(lock("project-a", "FN-1", async () => {}, 50)) + .rejects.toBeInstanceOf(PlanningLifecycleLockTransportError); + releaseFirst(); + await first; + }); + + it("fails closed for unavailable, pooled, missing, and mismatched endpoints", async () => { + const base = { + projectId: "project-a", + taskId: "FN-1", + provenance: "migration-override" as const, + timeoutMs: 50, + }; + const callback = async () => {}; + + await expect(withPlanningLifecycleAdvisoryLock({ + ...base, + directSessionUrl: null, + runtimeUrl: h.testUrl(), + migrationUrl: h.testUrl(), + }, callback)).rejects.toBeInstanceOf(PlanningLifecycleLockTransportError); + await expect(withPlanningLifecycleAdvisoryLock({ + ...base, + directSessionUrl: "postgresql://localhost:5432/db?pgbouncer=true", + runtimeUrl: "postgresql://localhost:5432/db?pgbouncer=true", + migrationUrl: "postgresql://localhost:5432/db?pgbouncer=true", + }, callback)).rejects.toBeInstanceOf(PlanningLifecycleLockTransportError); + await expect(withPlanningLifecycleAdvisoryLock({ + ...base, + directSessionUrl: h.testUrl(), + runtimeUrl: h.testUrl(), + migrationUrl: `${h.testUrl()}_other`, + }, callback)).rejects.toBeInstanceOf(PlanningLifecycleLockTransportError); + await expect(withPlanningLifecycleAdvisoryLock({ + ...base, + directSessionUrl: "postgresql://127.0.0.1:1/unavailable", + runtimeUrl: "postgresql://127.0.0.1:1/unavailable", + migrationUrl: "postgresql://127.0.0.1:1/unavailable", + }, callback)).rejects.toBeInstanceOf(PlanningLifecycleLockTransportError); + }); +}); diff --git a/packages/core/src/__tests__/postgres/task-dependency-mutation.pg.test.ts b/packages/core/src/__tests__/postgres/task-dependency-mutation.pg.test.ts index b765250005..f95ece8550 100644 --- a/packages/core/src/__tests__/postgres/task-dependency-mutation.pg.test.ts +++ b/packages/core/src/__tests__/postgres/task-dependency-mutation.pg.test.ts @@ -17,6 +17,7 @@ import { type SharedPgTaskStoreHarness, } from "../../__test-utils__/pg-test-harness.js"; import type { TaskStore } from "../../store.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../../workflows/builtin-coding-workflow-ir.js"; const pgTest = pgDescribe; @@ -126,6 +127,144 @@ pgTest("TaskStore dependency mutations (PostgreSQL)", () => { expect((await store.getWorkflowWorkItem(pending.id))?.state).toBe("cancelled"); }); + it("keeps invalidation and continuation cancellation authoritative in a combined updateTask patch", async () => { + const prerequisite = await store.createTask({ description: "combined prerequisite", column: "done" }); + const dependent = await store.createTask({ description: "combined dependent", column: "todo" }); + const pending = await store.replaceActiveTaskWorkflowContinuation({ + runId: `${dependent.id}:continuation:0`, taskId: dependent.id, nodeId: "plan-review", + kind: "task", state: "runnable", stableWorkflowRunId: `${dependent.id}:workflow`, + continuationSequence: 0, waitReason: "planning", sourceColumn: "todo", targetColumn: "todo", irHash: "ir-v1", + }); + + await store.updateTask(dependent.id, { + dependencies: [prerequisite.id], + status: null, + approvedPlanFingerprint: "sha256:current", + awaitingApprovalReason: "plan-review-replan-cap", + workflowStepResults: [{ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed", + completedAt: "2026-08-04T02:00:00.000Z", + }], + }); + + const updated = await store.getTask(dependent.id); + expect(updated.status).toBe("needs-replan"); + expect(updated.approvedPlanFingerprint).toBeUndefined(); + expect(updated.awaitingApprovalReason).toBeUndefined(); + expect(updated.workflowStepResults).toEqual([ + expect.objectContaining({ + workflowStepId: "plan-review", + status: "passed", + supersededAt: expect.any(String), + supersededReason: "dependency-change", + }), + ]); + expect((await store.getWorkflowWorkItem(pending.id))?.state).toBe("cancelled"); + }); + + it.each(["dedicated", "generic"] as const)( + "preserves but supersedes Plan Review approval through the %s dependency API", + async (api) => { + const prerequisite = await store.createTask({ description: `${api} prerequisite`, column: "done" }); + const dependent = await store.createTask({ description: `${api} dependent`, column: "todo" }); + await store.updateTask(dependent.id, { + workflowStepResults: [{ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed", + completedAt: "2026-08-04T01:00:00.000Z", + }], + }); + const pending = await store.replaceActiveTaskWorkflowContinuation({ + runId: `${dependent.id}:continuation:0`, taskId: dependent.id, nodeId: "plan-review", + kind: "task", state: "runnable", stableWorkflowRunId: `${dependent.id}:workflow`, + continuationSequence: 0, waitReason: "planning", sourceColumn: "todo", targetColumn: "todo", irHash: "ir-v1", + }); + + if (api === "dedicated") { + await store.updateTaskDependencies(dependent.id, { operation: "add", dependency: prerequisite.id }); + } else { + await store.updateTask(dependent.id, { dependencies: [prerequisite.id] }); + } + + const updated = await store.getTask(dependent.id); + expect(updated.status).toBe("needs-replan"); + expect(updated.workflowStepResults).toEqual([ + expect.objectContaining({ + workflowStepId: "plan-review", + status: "passed", + supersededAt: expect.any(String), + supersededReason: "dependency-change", + }), + ]); + expect((await store.getWorkflowWorkItem(pending.id))?.state).toBe("cancelled"); + }, + ); + + it.each(["dedicated", "generic"] as const)( + "invalidates and rehomes an exhausted split-column Plan Review through the %s dependency API", + async (api) => { + const definition = await store.createWorkflowDefinition({ + name: `split review dependency ${api}`, + ir: { + ...BUILTIN_CODING_WORKFLOW_IR, + id: `split-review-dependency-${api}`, + nodes: BUILTIN_CODING_WORKFLOW_IR.nodes.map((node) => + node.id === "plan-review" ? { ...node, column: "in-review" } : node + ), + }, + }); + const prerequisite = await store.createTask({ + description: `${api} prerequisite`, + column: "done", + workflowId: definition.id, + } as never); + const dependent = await store.createTask({ + description: `${api} dependent`, + workflowId: definition.id, + } as never); + const intakeColumn = dependent.column; + await store.moveTask(dependent.id, "in-review", { + moveSource: "engine", + recoveryRehome: true, + bypassGuards: true, + }); + await store.updateTask(dependent.id, { + status: "awaiting-approval", + awaitingApprovalReason: "plan-review-replan-cap", + approvedPlanFingerprint: "sha256:stale", + workflowStepResults: [{ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "failed", + verdict: "REVISE", + completedAt: "2026-08-04T05:00:00.000Z", + }], + } as never); + + if (api === "dedicated") { + await store.updateTaskDependencies(dependent.id, { + operation: "add", + dependency: prerequisite.id, + }); + } else { + await store.updateTask(dependent.id, { dependencies: [prerequisite.id] }); + } + + const updated = await store.getTask(dependent.id); + expect(updated.column).toBe(intakeColumn); + expect(updated.status).toBe("needs-replan"); + expect(updated.awaitingApprovalReason).toBeUndefined(); + expect(updated.approvedPlanFingerprint).toBeUndefined(); + expect(updated.workflowStepResults).toContainEqual(expect.objectContaining({ + workflowStepId: "plan-review", + supersededReason: "dependency-change", + })); + }, + ); + /* FNXC:WorkflowLifecycleColumns 2026-07-31-02:05 (PR #2720 review — greptile): DISTINCT HOLD AND INTAKE LANES, the configuration the default lineage does not exercise. diff --git a/packages/core/src/__tests__/postgres/unplanned-execution-block.pg.test.ts b/packages/core/src/__tests__/postgres/unplanned-execution-block.pg.test.ts new file mode 100644 index 0000000000..6f468977b0 --- /dev/null +++ b/packages/core/src/__tests__/postgres/unplanned-execution-block.pg.test.ts @@ -0,0 +1,126 @@ +import { afterAll, afterEach, beforeAll, expect, it } from "vitest"; +import { eq } from "drizzle-orm"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { + createSharedPgTaskStoreTestHarness, + pgDescribe, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import * as schema from "../../postgres/schema/index.js"; +import type { ResolvedBackend } from "../../postgres/backend-resolver.js"; +import { createConnectionSetFromUrl } from "../../postgres/connection.js"; +import { createAsyncDataLayer } from "../../postgres/data-layer.js"; +import { TaskStore } from "../../store.js"; +import { insertTaskRow } from "../../task-store/async/async-persistence.js"; + +const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_unplanned_block", +}); + +pgDescribe("unplanned execution refusal dedupe", () => { + beforeAll(h.beforeAll); + afterAll(h.afterAll); + afterEach(h.afterEach); + + it("atomically records one durable log entry for concurrent repeat calls", async () => { + const task = await h.store().createTask({ description: "unplanned task" }); + const before = (await h.store().getTask(task.id)).updatedAt; + + const results = await Promise.all(Array.from( + { length: 8 }, + () => h.store().checkAndRecordUnplannedExecutionBlock(task.id, "episode-a"), + )); + + expect(results.filter(Boolean)).toHaveLength(1); + const updated = await h.store().getTask(task.id); + expect(updated.updatedAt).toBe(before); + expect(updated.log?.filter((entry) => entry.action.includes("Execution dispatch refused"))).toHaveLength(1); + const markers = await h.adminDb().select().from(schema.project.unplannedExecutionBlocks); + expect(markers).toHaveLength(1); + }); + + it("records a new refusal when the planning episode changes", async () => { + const task = await h.store().createTask({ description: "replanned task" }); + + await expect(h.store().checkAndRecordUnplannedExecutionBlock(task.id, "episode-a")).resolves.toBe(true); + await expect(h.store().checkAndRecordUnplannedExecutionBlock(task.id, "episode-b")).resolves.toBe(true); + + const updated = await h.store().getTask(task.id); + expect(updated.log?.filter((entry) => entry.action.includes("Execution dispatch refused"))).toHaveLength(2); + }); + + it("keeps the same task id and episode independent across projects", async () => { + const backend: ResolvedBackend = { + mode: "external", + runtimeUrl: h.testUrl(), + migrationUrl: h.testUrl(), + migrationUrlOverridden: true, + directSessionUrl: h.testUrl(), + directSessionProvenance: "migration-override", + }; + const [connectionsA, connectionsB, rootA, rootB] = await Promise.all([ + createConnectionSetFromUrl(backend, { projectId: "project-a", useRuntimeRole: true }), + createConnectionSetFromUrl(backend, { projectId: "project-b", useRuntimeRole: true }), + mkdtemp(join(tmpdir(), "fusion-refusal-a-")), + mkdtemp(join(tmpdir(), "fusion-refusal-b-")), + ]); + try { + const layerA = createAsyncDataLayer(connectionsA, { projectId: "project-a" }); + const layerB = createAsyncDataLayer(connectionsB, { projectId: "project-b" }); + const storeA = new TaskStore(rootA, undefined, { asyncLayer: layerA }); + const storeB = new TaskStore(rootB, undefined, { asyncLayer: layerB }); + const now = new Date().toISOString(); + const row = { + id: "FN-SAME", + description: "same id", + column: "todo", + currentStep: 0, + createdAt: now, + updatedAt: now, + }; + await Promise.all([ + insertTaskRow(layerA, row, { lineageId: null }), + insertTaskRow(layerB, row, { lineageId: null }), + ]); + + await expect(Promise.all([ + storeA.checkAndRecordUnplannedExecutionBlock(row.id, "episode-a"), + storeB.checkAndRecordUnplannedExecutionBlock(row.id, "episode-a"), + ])).resolves.toEqual([true, true]); + + const markers = await h.adminDb().select().from(schema.project.unplannedExecutionBlocks) + .where(eq(schema.project.unplannedExecutionBlocks.taskId, row.id)); + expect(markers.map((marker) => marker.projectId).sort()).toEqual(["project-a", "project-b"]); + } finally { + await Promise.allSettled([ + connectionsA.close(), + connectionsB.close(), + rm(rootA, { recursive: true, force: true }), + rm(rootB, { recursive: true, force: true }), + ]); + } + }); + + it("rolls back the marker when the live task guard fails", async () => { + const archived = await h.store().createTask({ description: "archived task" }); + await h.store().archiveTask(archived.id, { cleanup: false }); + + await expect(h.store().checkAndRecordUnplannedExecutionBlock(archived.id, "episode-a")) + .rejects.toThrow(/not found or archived/); + + const archivedMarkers = await h.adminDb().select().from(schema.project.unplannedExecutionBlocks) + .where(eq(schema.project.unplannedExecutionBlocks.taskId, archived.id)); + expect(archivedMarkers).toHaveLength(0); + + const deleted = await h.store().createTask({ description: "deleted task" }); + await h.store().deleteTask(deleted.id); + await expect(h.store().checkAndRecordUnplannedExecutionBlock(deleted.id, "episode-a")) + .rejects.toThrow(/not found or archived/); + const deletedMarkers = await h.adminDb().select().from(schema.project.unplannedExecutionBlocks) + .where(eq(schema.project.unplannedExecutionBlocks.taskId, deleted.id)); + expect(deletedMarkers).toHaveLength(0); + }); +}); diff --git a/packages/core/src/__tests__/task-update-lanes-resolved.test.ts b/packages/core/src/__tests__/task-update-lanes-resolved.test.ts index 15e8224b9d..05cb6ef180 100644 --- a/packages/core/src/__tests__/task-update-lanes-resolved.test.ts +++ b/packages/core/src/__tests__/task-update-lanes-resolved.test.ts @@ -143,6 +143,120 @@ describe("adding a dependency never parks a card in a deleted column", () => { expect(row.awaitingApprovalReason).toBeUndefined(); }); + it("keeps dependency invalidation authoritative over a combined status clear", async () => { + const { store, row } = harness({ + column: "todo", + dependencies: [], + status: "awaiting-approval", + }, DEFAULT_IR); + + await run(store, { dependencies: ["FN-2"], status: null }); + + expect(row.status).toBe("needs-replan"); + }); + + it("keeps dependency invalidation authoritative over combined current approval evidence", async () => { + const currentResult = { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed", + completedAt: "2026-08-04T02:00:00.000Z", + }; + const { store, row } = harness({ + column: "todo", + dependencies: [], + }, DEFAULT_IR); + + await run(store, { + dependencies: ["FN-2"], + status: "awaiting-approval", + approvedPlanFingerprint: "sha256:current", + awaitingApprovalReason: "plan-review-replan-cap", + workflowStepResults: [currentResult], + }); + + expect(row.status).toBe("needs-replan"); + expect(row.approvedPlanFingerprint).toBeUndefined(); + expect(row.awaitingApprovalReason).toBeUndefined(); + expect(row.workflowStepResults).toEqual([ + expect.objectContaining({ + ...currentResult, + supersededAt: expect.any(String), + supersededReason: "dependency-change", + }), + ]); + }); + + it.each(["passed", "pending"] as const)( + "preserves but supersedes an old %s Plan Review projection when a dependency starts a new planning episode", + async (status) => { + const oldResult = { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status, + ...(status === "pending" + ? { startedAt: "2026-08-04T01:00:00.000Z", leaseOwner: "planner:old-episode" } + : { completedAt: "2026-08-04T01:00:00.000Z" }), + }; + const { store, row } = harness({ + column: "todo", + dependencies: [], + workflowStepResults: [oldResult], + }, DEFAULT_IR); + + await run(store, { dependencies: ["FN-2"] }); + + expect(row.workflowStepResults).toEqual([ + expect.objectContaining({ + ...oldResult, + supersededAt: expect.any(String), + supersededReason: "dependency-change", + }), + ]); + }, + ); + + it.each([ + ["null", null], + ["empty", []], + ["unrelated replacement", [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "passed", + completedAt: "2026-08-04T02:00:00.000Z", + }]], + ] as const)( + "retains the superseded prior Plan Review when a combined patch supplies %s workflow results", + async (label, workflowStepResults) => { + const oldPlanReview = { + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + status: "passed" as const, + completedAt: "2026-08-04T01:00:00.000Z", + }; + const { store, row } = harness({ + column: "todo", + dependencies: [], + workflowStepResults: [oldPlanReview], + }, DEFAULT_IR); + + await run(store, { dependencies: ["FN-2"], workflowStepResults }); + + expect(row.workflowStepResults).toEqual(expect.arrayContaining([ + expect.objectContaining({ + ...oldPlanReview, + supersededAt: expect.any(String), + supersededReason: "dependency-change", + }), + ])); + if (label === "unrelated replacement") { + expect(row.workflowStepResults).toEqual(expect.arrayContaining([ + expect.objectContaining({ workflowStepId: "code-review", status: "passed" }), + ])); + } + }, + ); + it("moves a RENAMED board's hold card to its own intake lane", async () => { // Here intake and hold ARE different columns, so the move is real — and it goes to `inbox`, // a column this board actually declares. diff --git a/packages/core/src/__tests__/workflow-step-results.test.ts b/packages/core/src/__tests__/workflow-step-results.test.ts index 702840079a..880b85d78c 100644 --- a/packages/core/src/__tests__/workflow-step-results.test.ts +++ b/packages/core/src/__tests__/workflow-step-results.test.ts @@ -51,6 +51,28 @@ describe("upsertWorkflowStepResult", () => { expect(next[0].priorAttempts?.[0].output).toBe("advisory-1"); }); + it("preserves superseded Plan Review evidence when the new planning episode starts", () => { + const oldPass = makeResult({ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + startedAt: "T1", + status: "passed", + supersededAt: "T2", + supersededReason: "dependency-change", + }); + const nextEpisode = makeResult({ + workflowStepId: "plan-review", + workflowStepName: "Plan Review", + startedAt: "T3", + status: "pending", + }); + + const next = upsertWorkflowStepResult([oldPass], nextEpisode); + + expect(next[0].status).toBe("pending"); + expect(next[0].priorAttempts).toEqual([oldPass]); + }); + it("does NOT snapshot when the replaced entry was passed/skipped/pending", () => { for (const status of ["passed", "skipped", "pending"] as const) { const attempt1 = makeResult({ startedAt: "T1", status, output: "attempt-1" }); diff --git a/packages/core/src/agents/agent-prompts.ts b/packages/core/src/agents/agent-prompts.ts index 31ac088f85..8a0234ee64 100644 --- a/packages/core/src/agents/agent-prompts.ts +++ b/packages/core/src/agents/agent-prompts.ts @@ -16,6 +16,7 @@ */ import type { AgentCapability, AgentPromptTemplate, AgentPromptsConfig } from "../types.js"; +import { FAST_PLANNING_COMPLETENESS_POLICY, PLANNING_COMPLETENESS_POLICY } from "./planning-review-policy.js"; // FNXC:WorkflowLifecycleColumns 2026-07-30-11:00: these are ROLE comparisons, not column // guards — the planner LANE is named `triage` and stays named that. See PLANNER_AGENT_ROLE. import { PLANNER_AGENT_ROLE } from "../types/task/task-log.js"; @@ -268,6 +269,8 @@ Fast planning also requires \`## Original Description\` (verbatim operator text) */ const FAST_TRIAGE_PROMPT_TEXT = `You are a task specification agent for "fn". This task is running in **fast mode**. +${FAST_PLANNING_COMPLETENESS_POLICY} + Write a lean, executable PROMPT.md quickly. Preserve safety gates, but skip heavyweight ceremony, review scoring, and proactive subtask analysis. ## Fast-mode priorities @@ -317,18 +320,10 @@ If the requested outcome is only to decide, route, or coordinate work, include \ ## Output Write PROMPT.md directly and stop. Do not call \`fn_review_spec()\`; workflow Plan Review is the single optional plan review gate before execution.`; -/* -FNXC:TriagePlanReviewConvergence 2026-07-16-09:20: -Planner-side convergence rules. `## File Scope — front-load surface enumeration` makes the -planner grep ALL call-sites + persistence/backend paths BEFORE writing File Scope so Plan Review -confirms coverage instead of surfacing a deeper missed surface each cycle (a top replan-loop -cause). `## Storage architecture (ground truth)` encodes verified facts (Postgres-only store, -no task-store/store.ts, tasks composite PK (project_id, id), migrations registered in -schema-applier.ts) so the planner stops citing removed/nonexistent things and losing rounds to -stale-codebase-fact rejections (FN-7996/FN-8105/FN-8108). -*/ const TRIAGE_PROMPT_TEXT = `You are a task specification agent for "fn", an AI-orchestrated task board. +${PLANNING_COMPLETENESS_POLICY} + ## Your Role You are the specification quality gate for implementation success. Your job: take a rough task description and produce a fully specified PROMPT.md that another AI agent can execute autonomously in a fresh context with zero memory of this conversation. @@ -686,23 +681,6 @@ If the task targets a different task ID (audit, forensic walk, historical reconc `;; // FN-6235: single source for the built-in reviewer policy; the engine REVIEWER_SYSTEM_PROMPT duplicate was removed. -/* -FNXC:PlanReviewReplan 2026-07-15-11:15: -Built-in reviewer prompt includes Spec/Plan Review Convergence rules so REVISE stays -blocking-only with surgical fix lists, reducing planner↔Plan-Review thrash (paired with -triage seeding existing PROMPT.md on needs-replan and reviewType "spec" for the triage gate). - -FNXC:TriagePlanReviewConvergence 2026-07-16-09:20: -Spec gates looped to the 8-replan cap (FN-7996/FN-8105/FN-8108) because each cycle re-reviewed -cold and surfaced a NEW, deeper blocking issue instead of confirming prior ones were fixed -(goalpost movement/whack-a-mole), and reviewed at implementation altitude (demanding exact SQL, -lock/CAS design, column mapping, field-level failure values against a *spec*). Three prompt -rules address this: (1) "Converging on re-review" in `## Spec / Plan Review Convergence` — -verify prior issues, don't REVISE for the reviewer's own earlier miss, and at attempt 3+ ratchet -to critical-only; (2) `## Spec Altitude` extends the plan-altitude principle to the spec gate so -implementation decisions are deferred to code review; the per-attempt prior-feedback + attempt -number are threaded from triage via reviewStep/buildReviewRequest. -*/ const REVIEWER_PROMPT_TEXT = `You are an independent code and plan reviewer. ## Your Role @@ -832,20 +810,6 @@ Concrete examples: - [Optional improvements, not blocking] \`\`\` -## Spec / Plan Review Convergence - -Specs and pre-execution Plan Review share this gate. Prefer **APPROVE** / **APPROVE_WITH_NOTES** when the plan is executable enough for an agent to implement. Put optional polish only under **Suggestions**. - -When you must **REVISE**: -- List each blocking issue as a concrete PROMPT.md edit (which section, what to add/change/remove). -- Do not demand a full rewrite unless the approach is fundamentally wrong (**RETHINK**). -- Prefer fixing local PROMPT.md defects in-session when you have write tools, then **APPROVE**, instead of bouncing the task through another full replan cycle. - -**Converging on re-review (when the request includes your prior feedback + a Plan Review attempt number):** -- This is a spec you already reviewed. VERIFY each issue you previously raised was addressed. REVISE only for (a) a PRIOR blocking issue still unresolved, or (b) a genuinely NEW problem THIS revision introduced. -- Do NOT introduce a new blocking issue that ALSO applied to the version you previously reviewed — that is your own earlier miss. Record it under **Suggestions**, not REVISE. -- **Severity ratchet at attempt 3+:** gate ONLY on \`critical\` (delivery-blocking) issues. Downgrade lone \`important\`/\`minor\` spec-wording nits to **Suggestions** and APPROVE. Rationale: the executor and downstream code review are later gates — a spec need not be perfect to be executable, only executable. - ## Spec Review — Undersplit Task Detection When reviewing specs, assess whether the task should have been broken into subtasks. The bar for splitting is high — most tasks should remain whole. Coordination overhead (worktrees, dependency wiring, merge sequencing) is real, so splitting must clearly pay for itself. diff --git a/packages/core/src/agents/code-review-policy.ts b/packages/core/src/agents/code-review-policy.ts new file mode 100644 index 0000000000..7d2f80f136 --- /dev/null +++ b/packages/core/src/agents/code-review-policy.ts @@ -0,0 +1,20 @@ +/* + * FNXC:CodeReviewCompleteness 2026-08-04-06:35: + * Code Review must prove every contract row through production reachability and + * exact symptom coverage, reason about temporal state, and make a fresh + * adversarial pass before approval. + */ +export const CODE_REVIEW_COMPLETENESS_POLICY = `## Mandatory Code Review Procedure + +Apply this procedure to code reviews only. Plan/spec reviews use their dedicated criteria below. + +Before choosing a verdict: + +1. Build a **requirements ledger** from PROMPT.md. For every Mission outcome, Completion Criterion, relevant \`## Surface Enumeration\` item, and \`## Symptom Verification\` assertion, identify both implementation evidence and test evidence. A missing or partial required row is REVISE. +2. Trace each changed behavior from its **real production entry point** and selector through the changed helper to its consumers. A helper-only unit test does not prove that production can reach the new branch. +3. For bug fixes, locate an automated test that reproduces the exact reported failure and asserts it is gone across the enumerated surfaces. Green builds, nearby unit tests, and synthetic state injection alone are insufficient. +4. For persistence, lifecycle, and concurrency changes, verify that evidence belongs to the **current state, version, or planning episode**; every read-check-write path is atomic, locked, or CAS-protected; all mutation surfaces preserve the invariant; waits and locks are bounded and cleanup-safe; and a production-shaped integration test covers both relevant orderings. +5. For route or state changes, inspect all affected UI, API, CLI, and agent consumers plus paired success/failure or approve/reject paths for contract parity. +6. Before APPROVE, perform a fresh adversarial validation pass: try to disprove each completed ledger row by reading the real caller, guard, consumer, and test. Cite \`file:line\` for every blocker and name the exact missing proof. + +APPROVE only when every required ledger row has implementation and test evidence.`; diff --git a/packages/core/src/agents/planning-review-policy.ts b/packages/core/src/agents/planning-review-policy.ts new file mode 100644 index 0000000000..99a64ef142 --- /dev/null +++ b/packages/core/src/agents/planning-review-policy.ts @@ -0,0 +1,37 @@ +/* + * FNXC:PlanningReviewCompleteness 2026-08-04-06:35: + * Planning and Plan Review share one completeness contract: planning researches + * and maps the full requirement ledger up front, while review evaluates the whole + * artifact and batches every independently discoverable blocker in one round. + */ +export const PLANNING_COMPLETENESS_POLICY = `## Mandatory Planning Completeness Procedure + +Before writing the final PROMPT.md: + +1. Build an internal **planning ledger** from the Original Description, user comments, project instructions, cited issue/report, and any saved planning document. Preserve every requirement, settled decision, explicit non-goal, and acceptance outcome; do not silently replace the reporter's contract with a narrower reproduction. +2. Research before structuring the plan. Trace the affected behavior through its real production entry points, callers, writers/readers, shared consumers, persistence boundaries, recovery paths, and existing tests. Resolve planning-time facts from the repository now; defer only details that genuinely require implementation-time discovery, and label those explicitly. +3. For stateful, lifecycle, persistence, or concurrency work, enumerate the participant graph and both relevant orderings: stale-work cancellation/fencing, lock order and reentrancy, transaction boundaries, pool/transport limits, competing recovery paths, configuration/deployment identities, bypass/force paths, and failure cleanup. Every stated invariant must name the surfaces that preserve it. +4. Map every ledger row to concrete File Scope entries, dependency-ordered implementation steps, and verification. Each feature-bearing step must name specific automated test scenarios with the input/state, action or ordering, and expected observable result; helper-only tests do not prove production reachability. +5. Keep scope disciplined. Reuse current architecture and documented patterns unless the reporter contract requires a redesign. Put optional cleanup and adjacent ideas outside the active steps. +6. Before persisting PROMPT.md, perform a fresh holistic completeness pass across the whole ledger. Try to disprove the plan by checking referenced paths and patterns, missing consumers, contradictory steps, uncovered failure orderings, and acceptance criteria without tests. Fix all gaps in one pass. + +On a revision, treat the cumulative revision ledger as durable decisions unless a later entry explicitly supersedes one: preserve resolved items, address every unresolved item surgically, and rerun the complete procedure instead of checking only the latest comment.`; + +export const FAST_PLANNING_COMPLETENESS_POLICY = `## Fast Planning Completeness Check + +Before writing PROMPT.md, build a compact internal ledger from the Original Description, user comments, project rules, and acceptance outcomes. Inspect the real production callers/consumers, persistence or recovery paths, and nearby tests before choosing File Scope. For stateful or concurrent work, enumerate every participant plus both relevant orderings and failure cleanup. Map each requirement to a concrete step, file, and automated test scenario, then reread the whole plan once for missing surfaces, contradictions, and unproved outcomes. On revision, preserve prior decisions and address the complete cumulative ledger, not only the latest note.`; + +export const PLAN_REVIEW_COMPLETENESS_POLICY = `## Mandatory Plan Review Procedure + +Apply this procedure to plan/spec reviews only. Code reviews use their dedicated procedure. + +Before choosing a verdict: + +1. Build one **review ledger** for the entire PROMPT.md: Original Description and user comments; Mission and Completion Criteria; Surface Enumeration and Symptom Verification when required; every implementation step, File Scope entry, dependency, risk, and test/verification promise. +2. Complete the full review before reporting. Check coherence and requirement traceability, feasibility against the current repository, scope discipline, execution ordering, and verification quality. When relevant, also inspect security/data integrity, state transitions, concurrency orderings, deployment/configuration boundaries, recovery competitors, and force/bypass paths. +3. Review at specification altitude. Block when a required behavior, surface, ordering, safety constraint, or proof is missing or the stated approach cannot work. Keep optional implementation detail, wording polish, and nonessential improvements advisory. +4. If REVISE is necessary, batch **all independently discoverable blocking findings** into this one verdict; do not stop after the first defect. Give each blocker a stable ID, cite the affected section or repository evidence, and state the concrete PROMPT.md correction. Put advisory observations in a separate list. +5. On re-review, use the supplied prior-review ledger as a decision primer. Verify every prior blocker, do not re-raise resolved or rejected semantic duplicates, and preserve accepted decisions. A newly blocking finding must say whether the revision introduced it, which prior blocker genuinely masked it, or why it is independently delivery-blocking for correctness, security, data safety, or executability. Record an earlier reviewer miss explicitly; never demote a critical defect merely because it was missed before. +6. After any same-session PROMPT.md edit, distrust the edit: reread the complete artifact, rebuild the ledger, and perform a fresh holistic pass before APPROVE. + +APPROVE when the plan is executable and verifiable, not when it is cosmetically perfect. If returning REVISE, the verdict notes must contain the complete blocking checklist because those notes are the durable input to the next planning round.`; diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index adf30a68d0..2b3b93d94b 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -2252,6 +2252,7 @@ export { isTerminalStepResult, type ReviewLeaseDisposition, } from "./workflows/workflow-step-results.js"; +export { PLAN_REVIEW_COMPLETENESS_POLICY } from "./agents/planning-review-policy.js"; /* FNXC:GateBarrelSync 2026-07-19-01:10: Cutover (IR-driven lifecycle) barrel sync — same failure class as the classifyReviewLease incident above, found again when U4's resolveWipBudgetColumns threw "is not a function" in the engine-core hold/release sweep and silently zeroed all scheduler releases (scheduler-workflow-cutover gate suite 10/28 red). RULE: every runtime export the cutover adds to index.ts MUST be mirrored here; the engine-core gate bundle builds @fusion/core from THIS barrel. This block mirrors the cutover's lifecycle modules (transition policy, lifecycle traits, capacity budget, IR pin/drift, review-level preset, legacy adoption, creation column). diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8e78559c0c..2fdf7221d8 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2648,6 +2648,7 @@ export { isTerminalStepResult, type ReviewLeaseDisposition, } from "./workflows/workflow-step-results.js"; +export { PLAN_REVIEW_COMPLETENESS_POLICY } from "./agents/planning-review-policy.js"; // FNXC:SqliteRemoval 2026-07-14: Export async audit reader so engine tests can // query run-audit events in backend mode (sync getRunAuditEvents returns [] in PG mode). export { queryRunAuditEvents } from "./task-store/async/async-audit.js"; diff --git a/packages/core/src/planner/plan-approval.ts b/packages/core/src/planner/plan-approval.ts index 17b709b61f..a5a6e25dcd 100644 --- a/packages/core/src/planner/plan-approval.ts +++ b/packages/core/src/planner/plan-approval.ts @@ -19,6 +19,7 @@ cannot silently open the execution gate. */ export function isPlanReviewSatisfied(result: WorkflowStepResult): boolean { if (result.workflowStepId !== PLAN_REVIEW_GROUP_ID) return false; + if (result.supersededAt != null) return false; if (result.status === "passed") return true; return result.status === "skipped" && (result.bypassedFromStatus === "failed" || result.bypassedFromStatus === "advisory_failure") @@ -31,6 +32,24 @@ export function isPlanReviewSatisfied(result: WorkflowStepResult): boolean { && result.bypassReason.trim().length > 0; } +/* + * FNXC:PlanReviewSupersession 2026-08-04-06:35: + * A dependency change preserves Plan Review history for audit while retiring + * every current gate projection, including an in-flight lease. Superseded + * evidence belongs to the old planning episode and cannot satisfy the new gate. + */ +export function supersedePlanReviewResults( + results: WorkflowStepResult[] | undefined, + supersededAt: string, +): WorkflowStepResult[] | undefined { + if (!results?.some((result) => result.workflowStepId === PLAN_REVIEW_GROUP_ID && result.supersededAt == null)) { + return results; + } + return results.map((result) => result.workflowStepId === PLAN_REVIEW_GROUP_ID && result.supersededAt == null + ? { ...result, supersededAt, supersededReason: "dependency-change" } + : result); +} + /** * FNXC:PlanApproval 2026-07-04-22:41: * FN-7569 — manual plan approval was not idempotent against unchanged plan content: an diff --git a/packages/core/src/postgres/advisory-locks.ts b/packages/core/src/postgres/advisory-locks.ts index 2f1bb9cdce..7a20bec673 100644 --- a/packages/core/src/postgres/advisory-locks.ts +++ b/packages/core/src/postgres/advisory-locks.ts @@ -30,6 +30,53 @@ export class PlanningLifecycleLockTransportError extends Error { } } +const DEFAULT_PLANNING_LIFECYCLE_LOCK_TIMEOUT_MS = 5_000; + +type DedicatedPostgresClient = ReturnType; +type PostgresBackendIdentity = { + database: string; + host: string | null; + port: number | null; + cluster: string | null; +}; + +async function readPostgresBackendIdentity(client: DedicatedPostgresClient): Promise { + return await client` + SELECT current_database() AS database, + inet_server_addr()::text AS host, + inet_server_port() AS port, + current_setting('cluster_name', true) AS cluster + `; +} + +async function runBoundedTransportPhase( + client: DedicatedPostgresClient, + timeoutMs: number, + timeoutMessage: string, + failureMessage: string, + operation: () => Promise, +): Promise { + let timeoutHandle: ReturnType | undefined; + try { + return await Promise.race([ + operation(), + new Promise((_, reject) => { + timeoutHandle = setTimeout(() => { + void client.end({ timeout: 0 }).catch(() => undefined); + reject(new PlanningLifecycleLockTransportError(timeoutMessage)); + }, timeoutMs); + }), + ]); + } catch (error) { + if (error instanceof PlanningLifecycleLockTransportError) throw error; + // Do not retain the driver error as `cause`: connection failures can carry + // endpoint credentials, while this error is operator-visible. + throw new PlanningLifecycleLockTransportError(failureMessage); + } finally { + if (timeoutHandle) clearTimeout(timeoutHandle); + } +} + /** * FNXC:PlanningDependencyReseed 2026-08-04-00:43: * Planning handoff and dependency re-seed cross processes, so their outer lock @@ -45,10 +92,15 @@ export async function withPlanningLifecycleAdvisoryLock( provenance: "embedded-lifecycle" | "migration-override" | null; runtimeUrl?: string | null; migrationUrl?: string | null; + /** Bounds dedicated-session setup and lock acquisition, not callback work. */ + timeoutMs?: number; + /** @internal Test seam fired when the driver dispatches the lock query. */ + onLockAcquisitionAttempt?: () => void; }, callback: () => Promise, ): Promise { const directUrl = input.directSessionUrl; + const timeoutMs = Math.max(1, input.timeoutMs ?? DEFAULT_PLANNING_LIFECYCLE_LOCK_TIMEOUT_MS); if (!directUrl || !input.provenance || looksLikePoolerUrl(directUrl)) { throw new PlanningLifecycleLockTransportError("Planning lifecycle lock requires a direct PostgreSQL session endpoint"); } @@ -81,7 +133,17 @@ export async function withPlanningLifecycleAdvisoryLock( } } - const client = postgres(directUrl, { max: 1, prepare: false, onnotice: () => {} }); + const client = postgres(directUrl, { + max: 1, + connect_timeout: Math.max(1, Math.ceil(timeoutMs / 1_000)), + prepare: false, + onnotice: () => {}, + debug: input.onLockAcquisitionAttempt + ? (_connection, query) => { + if (query.includes("pg_advisory_lock(")) input.onLockAcquisitionAttempt?.(); + } + : undefined, + }); const key = `fusion:planning-lifecycle:${input.projectId}:${input.taskId}`; let acquired = false; try { @@ -91,12 +153,17 @@ export async function withPlanningLifecycleAdvisoryLock( database before locking so an accidental migration endpoint cannot serialize one database while task writes target another. */ - const identity = await client<{ database: string; host: string | null; port: number | null; cluster: string | null }[]>` - SELECT current_database() AS database, - inet_server_addr()::text AS host, - inet_server_port() AS port, - current_setting('cluster_name', true) AS cluster - `; + const identity = await runBoundedTransportPhase( + client, + timeoutMs, + `Planning lifecycle lock session setup timed out after ${timeoutMs}ms`, + "Planning lifecycle lock could not establish its dedicated session", + async () => { + const rows = await readPostgresBackendIdentity(client); + await client`SELECT set_config('lock_timeout', ${`${timeoutMs}ms`}, false)`; + return rows; + }, + ); if (identity[0]?.database !== directDatabase) { throw new PlanningLifecycleLockTransportError("Planning lifecycle lock session selected an unexpected database"); } @@ -109,14 +176,20 @@ export async function withPlanningLifecycleAdvisoryLock( cluster with the same database cannot serialize the wrong task lifecycle. */ if (operationalUrl && operationalUrl !== directUrl) { - const operationalClient = postgres(operationalUrl, { max: 1, prepare: false, onnotice: () => {} }); + const operationalClient = postgres(operationalUrl, { + max: 1, + connect_timeout: Math.max(1, Math.ceil(timeoutMs / 1_000)), + prepare: false, + onnotice: () => {}, + }); try { - const operationalIdentity = await operationalClient<{ database: string; host: string | null; port: number | null; cluster: string | null }[]>` - SELECT current_database() AS database, - inet_server_addr()::text AS host, - inet_server_port() AS port, - current_setting('cluster_name', true) AS cluster - `; + const operationalIdentity = await runBoundedTransportPhase( + operationalClient, + timeoutMs, + `Planning lifecycle lock backend identity check timed out after ${timeoutMs}ms`, + "Planning lifecycle lock could not verify the resolved backend server identity", + () => readPostgresBackendIdentity(operationalClient), + ); const expected = operationalIdentity[0]; const actual = identity[0]; if (!expected || !actual @@ -133,11 +206,28 @@ export async function withPlanningLifecycleAdvisoryLock( await operationalClient.end({ timeout: 5 }).catch(() => undefined); } } - await client`SELECT pg_advisory_lock(hashtext(${key}))`; + await runBoundedTransportPhase( + client, + timeoutMs, + `Planning lifecycle lock acquisition timed out after ${timeoutMs}ms`, + "Planning lifecycle lock acquisition failed", + () => client`SELECT pg_advisory_lock(hashtext(${key}))`, + ); acquired = true; return await callback(); } finally { - if (acquired) await client`SELECT pg_advisory_unlock(hashtext(${key}))`.catch(() => undefined); - await client.end({ timeout: 5 }).catch(() => undefined); + try { + if (acquired) { + await runBoundedTransportPhase( + client, + timeoutMs, + `Planning lifecycle lock cleanup timed out after ${timeoutMs}ms`, + "Planning lifecycle lock cleanup failed", + () => client`SELECT pg_advisory_unlock(hashtext(${key}))`, + ).catch(() => undefined); + } + } finally { + await client.end({ timeout: 5 }).catch(() => undefined); + } } } diff --git a/packages/core/src/task-store/audit-ops.ts b/packages/core/src/task-store/audit-ops.ts index 563220be45..89965abeb1 100644 --- a/packages/core/src/task-store/audit-ops.ts +++ b/packages/core/src/task-store/audit-ops.ts @@ -156,7 +156,12 @@ export async function checkAndRecordUnplannedExecutionBlockImpl( const limit = getTaskActivityLogEntryLimit(); if (log.length > limit) log.splice(0, log.length - limit); await tx.update(schema.project.tasks) - .set({ log, updatedAt: entry.timestamp }) + /* + * FNXC:PlanningHandoffRecovery 2026-08-04-06:35: + * This diagnostic must not make an old planning handoff look fresh to + * recovery grace windows. The marker timestamp records audit recency. + */ + .set({ log }) .where(and(eq(schema.project.tasks.projectId, projectId), eq(schema.project.tasks.id, id))); return true; }); diff --git a/packages/core/src/task-store/task-update.ts b/packages/core/src/task-store/task-update.ts index 63d1f4a629..76f873e1c6 100644 --- a/packages/core/src/task-store/task-update.ts +++ b/packages/core/src/task-store/task-update.ts @@ -32,6 +32,8 @@ import {applyOriginalDescription} from "../tasks/original-description-policy.js" import {normalizeTaskReviewState} from "../task-store/review-state.js"; import {hasOwnDeclaredSymbols, normalizeDeclaredSymbols, extractDeclaredSymbolsFromPrompt, resolveTaskSymbolsForTask} from "../tasks/task-symbol-resolution.js"; import {assertValidProviderInstanceId} from "../provider-instance.js"; +import {supersedePlanReviewResults} from "../planner/plan-approval.js"; +import {PLAN_REVIEW_GROUP_ID} from "../workflows/builtin-plan-review-group.js"; export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updates: Parameters[1], runContext?: RunMutationContext,): Promise { /* FNXC:CredentialInstanceSelection 2026-08-01-05:43: validate task authoring input before persistence; ids are stored but runtime credential resolution remains unchanged. */ @@ -53,6 +55,9 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat const dir = store.taskDir(id); const task = await store.readTaskJson(dir); const wasFailed = task.status === "failed"; + const preUpdatePlanReviewResults = task.workflowStepResults?.filter( + (result) => result.workflowStepId === PLAN_REVIEW_GROUP_ID, + ); // Capture title/description before mutation so the PROMPT.md stub // detector below can compare against the exact wrapper bytes that the @@ -162,11 +167,12 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat if (updates.workspaceWorktrees !== undefined) { task.workspaceWorktrees = updates.workspaceWorktrees; } - // Detect new dependencies being added to a hold-lane task → re-seed for re-specification + // New dependencies re-seed hold-lane tasks and exhausted Plan Review cap parks. let movedToTriage = false; let respecifyFromColumn: string | undefined; let respecifyMoveLanes: TaskMoveLanes | undefined; let previousDependencies: string[] | undefined; + let planningInvalidatedAt: string | undefined; if (updates.dependencies !== undefined) { previousDependencies = (task.dependencies ?? []).map((dependency) => dependency.trim()).filter(Boolean); const oldDeps = new Set(previousDependencies); @@ -212,7 +218,11 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat /* DELIBERATE-LITERAL — the unresolvable-workflow default for the SOURCE lane only; the destination below never falls back to a literal. Reviewed 2026-07-31-02:40. */ const holdLane = depLanes === undefined ? "todo" : depLanes.hold; - if (hasNewDeps && holdLane !== undefined && task.column === holdLane) { + const isPlanReviewCapPark = task.status === "awaiting-approval" + && task.awaitingApprovalReason === "plan-review-replan-cap"; + const shouldRespecify = hasNewDeps + && ((holdLane !== undefined && task.column === holdLane) || isPlanReviewCapPark); + if (shouldRespecify) { const intakeLane = depLanes?.intake; respecifyFromColumn = task.column; const relocating = intakeLane !== undefined && intakeLane !== task.column; @@ -221,23 +231,16 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat task.columnMovedAt = new Date().toISOString(); } /* - FNXC:PlanningDependencyReseed 2026-08-04-00:30: - A new dependency invalidates a plan that is still in the hold lane. The + FNXC:PlanningDependencyReseed 2026-08-04-06:35: + A new dependency invalidates a plan that is still in the hold lane or parked after + exhausting Plan Review in a distinct review column. The prior null reset raced an in-flight planner after it wrote PROMPT.md but before its final handoff, leaving a real specification that neither planning discovery nor release could claim. `needs-replan` is the graph-owned durable re-entry signal: it preserves prompt authority and makes the interrupted planner's stale finalizer harmless. */ - task.status = "needs-replan"; - /* - FNXC:PlanningDependencyReseed 2026-08-04-00:54: - Both dependency mutation APIs invalidate the same pre-execution plan - handoff. Clearing manual-approval evidence here prevents a newly added - blocker from inheriting approval for the superseded specification. - */ - task.approvedPlanFingerprint = undefined; - task.awaitingApprovalReason = undefined; + planningInvalidatedAt = new Date().toISOString(); const depLogEntry: TaskLogEntry = { timestamp: new Date().toISOString(), action: relocating @@ -834,6 +837,37 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat } else if (updates.workflowStepResults !== undefined) { task.workflowStepResults = updates.workflowStepResults; } + if (planningInvalidatedAt !== undefined) { + /* + FNXC:PlanningDependencyReseed 2026-08-04-06:35: + Dependency invalidation is authoritative over every field in the same + generic updateTask patch. Apply it after the ordinary status, approval, + and workflow-result merge so a dashboard PATCH containing dependencies + plus stale current-episode fields cannot undo the replan fence. The + persistence transaction below also retires the pending continuation. + + Preserve the pre-patch Plan Review projection when that same patch clears + or replaces workflowStepResults without a Plan Review row. Dropping that + audit projection lets the graph reconstruct the old pass from its durable + completion log and incorrectly release the newly invalidated plan. + */ + task.status = "needs-replan"; + task.approvedPlanFingerprint = undefined; + task.awaitingApprovalReason = undefined; + const patchedResultsRetainPlanReview = task.workflowStepResults?.some( + (result) => result.workflowStepId === PLAN_REVIEW_GROUP_ID, + ) === true; + const resultsWithPriorPlanReview = patchedResultsRetainPlanReview + ? (task.workflowStepResults ?? []) + : [ + ...(task.workflowStepResults ?? []), + ...(preUpdatePlanReviewResults ?? []), + ]; + task.workflowStepResults = supersedePlanReviewResults( + resultsWithPriorPlanReview.length > 0 ? resultsWithPriorPlanReview : undefined, + planningInvalidatedAt, + ); + } if (updates.mergeDetails === null) { task.mergeDetails = undefined; } else if (updates.mergeDetails !== undefined) { diff --git a/packages/core/src/task-store/update-task-deps.ts b/packages/core/src/task-store/update-task-deps.ts index 5d59bbf654..fc4699d15d 100644 --- a/packages/core/src/task-store/update-task-deps.ts +++ b/packages/core/src/task-store/update-task-deps.ts @@ -25,6 +25,7 @@ import {generateTaskLineageId} from "../tasks/task-lineage.js"; import {deriveFallbackTaskTitle} from "../ai/ai-summarize.js"; import {sanitizeFileScopeInPromptContent} from "../task-store/file-scope.js"; import {__setTaskActivityLogLimitsForTesting} from "../task-store/comments.js"; +import {supersedePlanReviewResults} from "../planner/plan-approval.js"; export async function refineTaskImpl(store: TaskStore, id: string, feedback: string): Promise { const sourceTask = await store.getTask(id); @@ -440,9 +441,10 @@ async function updateTaskDependenciesWithTaskLockImpl(store: TaskStore, id: stri task.log ??= []; let movedToTriage = false; /* - FNXC:WorkflowLifecycleColumns 2026-08-02-02:30 (fleet — GUARD AND DESTINATION together): - A new dependency on a card still resting in the HOLD lane sends it back to INTAKE for - re-specification. Both ends were literals, so this never fired on a renamed board — and converting + FNXC:WorkflowLifecycleColumns 2026-08-04-06:35 (FN-8768 — GUARD AND DESTINATION together): + A new dependency on a card still resting in the HOLD lane, or parked after exhausting Plan + Review in a distinct review column, sends it back to INTAKE for re-specification. Both ends + were literals, so this never fired on a renamed board — and converting only the guard would have written an `intake` column the board may not declare directly into the row, which is worse than not firing: the store would hold a card in a column that does not exist. @@ -453,18 +455,26 @@ async function updateTaskDependenciesWithTaskLockImpl(store: TaskStore, id: stri const holdColumn = respecifyLifecycle?.hold ?? "todo"; const intakeColumn = respecifyLifecycle?.intake; const respecifyFromColumn = task.column; + const isPlanReviewCapPark = task.status === "awaiting-approval" + && task.awaitingApprovalReason === "plan-review-replan-cap"; + const shouldRespecify = hasNewDependencies + && (task.column === holdColumn || isPlanReviewCapPark); /* - FNXC:PlanningDependencyReseed 2026-08-04-00:43: + FNXC:PlanningDependencyReseed 2026-08-04-06:35: A new dependency invalidates every pre-execution approval artifact even when merged intake/hold lanes make this a same-column transition. Leaving the old fingerprint would let an unchanged prompt bypass manual approval. */ - if (hasNewDependencies && task.column === holdColumn) { + if (shouldRespecify) { task.status = "needs-replan"; task.approvedPlanFingerprint = undefined; task.awaitingApprovalReason = undefined; + task.workflowStepResults = supersedePlanReviewResults( + task.workflowStepResults, + task.updatedAt, + ); } - if (hasNewDependencies && task.column === holdColumn && intakeColumn !== undefined) { + if (shouldRespecify && intakeColumn !== undefined) { task.column = intakeColumn; movedToTriage = true; /* @@ -551,4 +561,3 @@ async function updateTaskDependenciesWithTaskLockImpl(store: TaskStore, id: stri return task; }); } - diff --git a/packages/core/src/types/workflow/workflow-steps.ts b/packages/core/src/types/workflow/workflow-steps.ts index 178444d0fe..8d3f2fed8c 100644 --- a/packages/core/src/types/workflow/workflow-steps.ts +++ b/packages/core/src/types/workflow/workflow-steps.ts @@ -313,6 +313,23 @@ export interface WorkflowStepResult { bypassedFromStatus?: WorkflowStepResult["status"]; /** The `verdict` (if any) this result carried immediately before the bypass, preserved for audit only — never promoted to `verdict`. */ bypassedFromVerdict?: WorkflowStepResult["verdict"]; + /* + * FNXC:PlanReviewSupersession 2026-08-04-06:35: + * Persist the boundary that invalidated this planning episode. Superseded + * results remain auditable but cannot satisfy or repair the current gate and + * a superseded pending result cannot be adopted as a live lease. + */ + supersededAt?: string; + /** Machine-readable reason the result stopped being current. */ + supersededReason?: "dependency-change"; + /* + * FNXC:PlanReviewConvergence 2026-08-04-06:35 (FN-8768): + * Number of terminal REVISE results recorded in the current Plan Review + * episode. Unlike `priorAttempts`, this scalar is not capped and is the + * durable authority for revision-budget accounting and attempt numbering. + * Reset when a superseded planning episode is replaced. + */ + planReviewAttemptCount?: number; /* * FNXC:WorkflowStepResults 2026-07-09-00:10: * FN-7727: self-healing recovery re-runs a failed pre-merge review node diff --git a/packages/core/src/workflows/builtin-code-review-group.ts b/packages/core/src/workflows/builtin-code-review-group.ts index ed374af928..2baaf1cb50 100644 --- a/packages/core/src/workflows/builtin-code-review-group.ts +++ b/packages/core/src/workflows/builtin-code-review-group.ts @@ -1,4 +1,5 @@ import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { CODE_REVIEW_COMPLETENESS_POLICY } from "../agents/code-review-policy.js"; /* FNXC:CodeReviewStep 2026-06-25-15:00: @@ -65,6 +66,8 @@ const CODE_REVIEW_PROMPT = `You are a senior code reviewer. Review the task's di 5. **Error handling** — swallowed errors, unhandled rejections/exceptions, missing validation at trust boundaries, misleading error messages. 6. **Contract / signature changes** — changed function/exported-type signatures, API request/response shapes, or serialization that breaks existing callers. +${CODE_REVIEW_COMPLETENESS_POLICY} + Be specific: cite \`file:line\` for every finding and explain the concrete failure it causes. ## Output Requirements diff --git a/packages/core/src/workflows/builtin-plan-review-group.ts b/packages/core/src/workflows/builtin-plan-review-group.ts index 8c1104ac47..91cdec13c8 100644 --- a/packages/core/src/workflows/builtin-plan-review-group.ts +++ b/packages/core/src/workflows/builtin-plan-review-group.ts @@ -1,4 +1,5 @@ import type { WorkflowIrNode } from "./workflow-ir-types.js"; +import { PLAN_REVIEW_COMPLETENESS_POLICY } from "../agents/planning-review-policy.js"; /* FNXC:PlanReviewStep 2026-06-28-23:29: @@ -29,12 +30,14 @@ const PLAN_REVIEW_PROMPT = `You are a senior plan reviewer. Review the task's PR 4. **Verification quality** — absent or weak tests/checks for the behavior being changed. 5. **Risk callouts** — migrations, data-loss paths, external integrations, secrets, or plugin/runtime dependencies that need explicit handling. +${PLAN_REVIEW_COMPLETENESS_POLICY} + Be specific: cite the plan section or file path for every finding and explain the concrete correction. ## Output Requirements - APPROVE: the plan is ready for execution. - APPROVE_WITH_NOTES: execution may proceed, but include non-blocking advisory notes. -- REVISE: the plan should be corrected before execution; include the missing or wrong requirement and the needed change. +- REVISE: the plan should be corrected before execution; include every blocking finding and needed change in the JSON notes, not only in preceding prose. - Final output: output exactly one trailing JSON object on the final line (no markdown fences, no surrounding prose): {"verdict":"APPROVE|APPROVE_WITH_NOTES|REVISE","notes":"..."}`; diff --git a/packages/core/src/workflows/default-workflow-hooks.ts b/packages/core/src/workflows/default-workflow-hooks.ts index ad93cc7b23..b070f4171d 100644 --- a/packages/core/src/workflows/default-workflow-hooks.ts +++ b/packages/core/src/workflows/default-workflow-hooks.ts @@ -377,7 +377,16 @@ export function applyReopenFieldClears(ctx: DefaultWorkflowMoveContext): void { const leftReviewForPlanningOrWip = fromColumn === reviewLane && (planning.includes(toColumn) || toColumn === wipLane); const leftCompleteForPlanning = fromColumn === completeLane && planning.includes(toColumn); - if (!graphOwnedReviewToWip && (leftReviewForPlanningOrWip || leftCompleteForPlanning)) { + /* + FNXC:PlanReviewApproval 2026-08-04-05:35: + A split-column manual approval first persists its audited Plan Review bypass, + then rebounds while the awaiting-approval hold is deliberately preserved. The + move must not erase that evidence before the final hold clear. This provenance + is set only by the approve-plan route; ordinary operator reopens still clear + prior review results exactly as before. + */ + const preservesApprovedPlanReview = ctx.workflowMoveSource === "plan-approval"; + if (!preservesApprovedPlanReview && !graphOwnedReviewToWip && (leftReviewForPlanningOrWip || leftCompleteForPlanning)) { task.workflowStepResults = undefined; } if (fromColumn === reviewLane && planning.includes(toColumn)) { diff --git a/packages/core/src/workflows/workflow-step-results.ts b/packages/core/src/workflows/workflow-step-results.ts index e6385a6342..0cf7611402 100644 --- a/packages/core/src/workflows/workflow-step-results.ts +++ b/packages/core/src/workflows/workflow-step-results.ts @@ -31,6 +31,10 @@ function isTerminalFailure(result: WorkflowStepResult): boolean { return TERMINAL_FAILURE_STATUSES.has(result.status); } +function isSupersededPlanningEvidence(result: WorkflowStepResult): boolean { + return result.supersededAt != null; +} + /** * Strip a result down to a single-level history snapshot: its own * `priorAttempts` are dropped so nesting never grows beyond one level deep. @@ -51,8 +55,9 @@ function toSnapshot(result: WorkflowStepResult): WorkflowStepResult { * carried forward onto the incoming result. If the existing entry represents * a DIFFERENT attempt (deduped by `startedAt` — a same-run `pending`→`failed` * transition of the same attempt is not a new attempt) and its status is a - * terminal failure (`failed` | `advisory_failure`), a single-level snapshot - * of it is pushed onto the incoming result's `priorAttempts`. + * terminal failure (`failed` | `advisory_failure`) or superseded planning + * projection, a single-level snapshot of it is pushed onto the incoming + * result's `priorAttempts`. * - `priorAttempts` is bounded to `opts.maxPriorAttempts` (default * `MAX_WORKFLOW_STEP_PRIOR_ATTEMPTS`), newest-first, oldest dropped. * @@ -79,7 +84,7 @@ export function upsertWorkflowStepResult( && previous.startedAt === incoming.startedAt; let priorAttempts = previous.priorAttempts ? [...previous.priorAttempts] : []; - if (!isSameAttempt && isTerminalFailure(previous)) { + if (!isSameAttempt && (isTerminalFailure(previous) || isSupersededPlanningEvidence(previous))) { priorAttempts = [toSnapshot(previous), ...priorAttempts]; } if (priorAttempts.length > maxPriorAttempts) { diff --git a/packages/dashboard/app/__tests__/browser-layout-smoke-fixture.test.ts b/packages/dashboard/app/__tests__/browser-layout-smoke-fixture.test.ts index 6d727a6623..c379c64b55 100644 --- a/packages/dashboard/app/__tests__/browser-layout-smoke-fixture.test.ts +++ b/packages/dashboard/app/__tests__/browser-layout-smoke-fixture.test.ts @@ -42,6 +42,19 @@ describe("browser layout smoke fixture", () => { expect(html).toContain("floating-window--github-import-detail"); }); + /* + FNXC:PlanReviewReplan 2026-08-04-06:35 FN-8768: + Keep both production approval surfaces in the real-browser fixture; the executable smoke checks + their shared responsive containment rather than treating the presence of markup as layout proof. + */ + it("includes the Plan Review replan-cap approval card and detail surfaces", () => { + const html = createSmokeHtml(); + expect(html).toContain('data-smoke="plan-review-replan-cap-approval"'); + expect(html).toContain("awaiting-approval--plan-review-replan-cap"); + expect(html).toContain("detail-plan-approval-banner--replan-cap"); + expect(html).toContain("Plan Review needs approval"); + }); + it("includes PR flow fixture sections and class hooks", () => { const html = createSmokeHtml(); expect(html).toContain('data-smoke="pr-create-modal"'); diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index 80522be784..43d38a7aee 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -52,7 +52,7 @@ import { ACTIVE_STATUSES, isTaskAgentActive } from "../utils/taskActivity"; import { getPrBadgeModifierClass } from "../utils/prBadgeClass"; import { getTotalAgentActiveMs, getEndToEndDurationMs, getTimedDurationMs, getWorkflowRuntimeMs, parseTimestampToMs } from "../utils/taskTiming"; import { getTaskStatusBadgeLabel, type TaskStatusBadgeContext, hasTaskStatusBadge } from "../utils/taskStatusBadgeLabel"; -import { isReviewBudgetExhaustedApproval } from "../utils/reviewBudgetApproval"; +import { isReviewBudgetExhaustedApproval, isTaskAwaitingPlanApproval } from "../utils/reviewBudgetApproval"; import { canStartPrFeedbackAddressing, getTaskPrimaryPrInfo } from "../utils/prFeedback"; import type { ToastType } from "../hooks/useToast"; import { useConfirm } from "../hooks/useConfirm"; @@ -1541,8 +1541,8 @@ function TaskCardComponent({ know approval is required because Plan Review exhausted automatic REVISE replans without converging — Approve keeps the current PROMPT.md; Reject regenerates. */ - const isAwaitingApproval = isIntakeColumn && task.status === "awaiting-approval"; const isPlanReviewReplanCapApproval = isReviewBudgetExhaustedApproval(task); + const isAwaitingApproval = isTaskAwaitingPlanApproval(task, isIntakeColumn); const isAwaitingInput = task.status === "awaiting-user-input"; const isArchived = isArchivedColumn; /* diff --git a/packages/dashboard/app/components/TaskDetailModal.tsx b/packages/dashboard/app/components/TaskDetailModal.tsx index 8a622a5ddb..f75dbd590f 100644 --- a/packages/dashboard/app/components/TaskDetailModal.tsx +++ b/packages/dashboard/app/components/TaskDetailModal.tsx @@ -85,7 +85,7 @@ import { hasPendingAutomaticRecovery, isTaskManuallyRetryable } from "../utils/t import { findInReviewStallLogEntry, IN_REVIEW_STALL_LOG_REGEX } from "../utils/findInReviewStallLogEntry"; import { getTaskLogEntryAction, getTaskLogEntryOutcome } from "../utils/taskLogEntryDisplay"; import { getRelativeTimeBucket } from "../utils/relativeTimeAgo"; -import { isReviewBudgetExhaustedApproval } from "../utils/reviewBudgetApproval"; +import { isReviewBudgetExhaustedApproval, isTaskAwaitingPlanApproval } from "../utils/reviewBudgetApproval"; import { ACTIVE_STATUSES, resolveEffectiveExecutor, resolveEffectivePlanning, resolveEffectiveValidator, type ModelSelection } from "./effective-model-resolution"; import { TaskContextMenu, buildTaskActionMenuModel, getTaskPrAutomationLabel } from "./TaskContextMenu"; import type { TaskContextMenuColumnFlags, TaskContextMenuColumnMetadata } from "./TaskContextMenu"; @@ -3408,8 +3408,8 @@ export function TaskDetailContent({ const isIntakeColumn = detailColumnFlags ? detailColumnFlags.intake === true : task.column === "triage"; - const isAwaitingApproval = isIntakeColumn && task.status === "awaiting-approval"; const isPlanReviewReplanCapApproval = isReviewBudgetExhaustedApproval(task); + const isAwaitingApproval = isTaskAwaitingPlanApproval(task, isIntakeColumn); const handleTogglePause = useCallback(async () => { try { diff --git a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx index e7b83ff7f7..ff2e2cc6b0 100644 --- a/packages/dashboard/app/components/__tests__/TaskCard.test.tsx +++ b/packages/dashboard/app/components/__tests__/TaskCard.test.tsx @@ -3070,11 +3070,11 @@ describe("TaskCard", () => { expect(container.querySelector(".awaiting-approval--plan-review-replan-cap")).toBeNull(); }); - it("renders a distinct review-budget-exhausted badge when awaitingApprovalReason is plan-review-replan-cap", () => { + it("renders review-budget approval metadata outside the intake column", () => { const { container } = render( { * Replan-cap escalations must explain that Plan Review did not converge so the * operator knows why approval is required (not a generic require-all gate). */ - it("explains Plan Review non-convergence when awaitingApprovalReason is plan-review-replan-cap", () => { + it("offers both decisions in a split Plan Review column after the replan cap", () => { render( { expect(banner.contains(screen.getByTestId("detail-plan-approval-banner-actions"))).toBe(true); expect(banner.contains(screen.getByTestId("detail-plan-approval-banner-approve"))).toBe(true); expect(banner.contains(screen.getByTestId("detail-plan-approval-banner-reject"))).toBe(true); + expect(screen.getByTestId("detail-plan-approval-footer-approve")).toBeTruthy(); + expect(screen.getByTestId("detail-plan-approval-footer-reject")).toBeTruthy(); expect(screen.getByText("Approval needed: Plan Review did not converge")).toBeTruthy(); expect(screen.getByText(/exhausted|without approving|stopped the replan loop/i)).toBeTruthy(); }); diff --git a/packages/dashboard/app/utils/reviewBudgetApproval.ts b/packages/dashboard/app/utils/reviewBudgetApproval.ts index 1eb914ffbe..8696a0e4ce 100644 --- a/packages/dashboard/app/utils/reviewBudgetApproval.ts +++ b/packages/dashboard/app/utils/reviewBudgetApproval.ts @@ -9,3 +9,14 @@ import type { Task } from "../../../core/src/types"; export function isReviewBudgetExhaustedApproval(task: Task): boolean { return task.status === "awaiting-approval" && task.awaitingApprovalReason === "plan-review-replan-cap"; } + +/** + * FNXC:PlanReviewReplan 2026-08-04-06:35 FN-8768: + * Approval controls normally belong to intake, but an exhausted Plan Review remains in the + * graph node's review column. Keep that persisted-reason exception shared by card and detail + * surfaces so a split-column workflow never renders a hold without its operator controls. + */ +export function isTaskAwaitingPlanApproval(task: Task, isIntakeColumn: boolean): boolean { + return task.status === "awaiting-approval" + && (isIntakeColumn || task.awaitingApprovalReason === "plan-review-replan-cap"); +} diff --git a/packages/dashboard/scripts/browser-layout-smoke.mjs b/packages/dashboard/scripts/browser-layout-smoke.mjs index 6646ca001a..271ec3459d 100644 --- a/packages/dashboard/scripts/browser-layout-smoke.mjs +++ b/packages/dashboard/scripts/browser-layout-smoke.mjs @@ -26,6 +26,8 @@ const gitHubImportAfterMobileScreenshotPath = process.env.FUSION_GITHUB_IMPORT_A const gitHubImportAfterShortScreenshotPath = process.env.FUSION_GITHUB_IMPORT_AFTER_SHORT_SCREENSHOT; const resolvedGithubDesktopScreenshotPath = process.env.FUSION_RESOLVED_GITHUB_DESKTOP_SCREENSHOT; const resolvedGithubMobileScreenshotPath = process.env.FUSION_RESOLVED_GITHUB_MOBILE_SCREENSHOT; +const planApprovalDesktopScreenshotPath = process.env.FUSION_PLAN_APPROVAL_DESKTOP_SCREENSHOT; +const planApprovalMobileScreenshotPath = process.env.FUSION_PLAN_APPROVAL_MOBILE_SCREENSHOT; const smokeTheme = process.env.FUSION_BROWSER_SMOKE_THEME === "light" ? "light" : "dark"; function log(message) { @@ -243,6 +245,23 @@ export function createSmokeHtml() { `; + /* + FNXC:PlanReviewReplan 2026-08-04-06:35 FN-8768: + Mirror both exhausted-review operator surfaces so Blink can prove the card badge and detail banner + remain visible and contained at mobile and desktop widths, not merely that fixture HTML loaded. + */ + const planApprovalFixture = ` +
+
+
FN-8768

Dependency changed during planning

+
Plan Review needs approval
+
+
+ Plan Review needs approval + Automatic revisions reached their limit. Review the latest plan, then approve it or send it back for replanning. +
+
`; + const githubImportMobileActionFixture = `
@@ -415,6 +434,7 @@ export function createSmokeHtml() { ${githubImportMobileActionFixture} ${resolvedGithubTableFixture} + ${planApprovalFixture}