Plan-approval mailbox notification + triage Plan Review convergence (#2237)
## Summary Two related changes around the plan-approval flow, plus a fix for triage's plan-review loop that was stranding tasks at the replan cap. ### 1. Post a mailbox message when a plan needs approval (`feat`) The ntfy push on `awaiting-approval` already existed end-to-end. This adds the missing **durable, in-dashboard record**: a `system`-typed mailbox message linking to the task, written whenever a task enters `awaiting-approval`. - Fires **before** the notifications-enabled gate, so a dashboard-only operator (no ntfy/webhook configured) still gets the record — the whole point of the mailbox channel. - `system` type avoids re-triggering the `message:agent-to-user` ntfy pipeline (no double-notify); idempotent via `sendMessageOnce` (key `plan-approval:<taskId>`). ### 2. Help triage Plan Review converge before the replan cap (`fix`) Investigation of three tasks that burned all 8 plan-review replans without converging (**FN-7996, FN-8105, FN-8108**) found the reviewer surfaced a *new, deeper* issue each cycle instead of confirming its prior ones were fixed (goalpost movement), and reviewed specs at implementation altitude. This addresses the root causes: - **Feed the spec reviewer its own prior REVISE feedback + the 1-based replan attempt** so it verifies prior issues rather than moving goalposts. Gated to `reviewType === "spec"` and `attempt > 1` — **code review and normal plan review are byte-for-byte unaffected** (double-verified). - **Reviewer prompt:** converge-on-re-review rule (don't REVISE for your own earlier miss), severity ratchet (critical-only at attempt ≥ 3), and a **Spec Altitude** guard so exact SQL/lock/CAS protocol design is deferred to code review. - **Planner prompt:** front-load exhaustive surface enumeration before writing File Scope, and a storage-architecture ground-truth note (Postgres-only store, composite PK `(project_id, id)`, `schema-applier` migrations) to stop the repeated stale-fact REVISE rounds. ## Testing - `@fusion/core` + `@fusion/engine` typecheck clean. - Added coverage: reviewer spec-convergence wiring (attempt gating + code/plan exclusion + severity ratchet), triage prior-feedback derivation (incl. empty-output→notes fallback), mailbox decoupling (fires when push disabled) + rejection safety, and assertions for all new prompt sections. - Affected suites green: notification-service, reviewer, triage-plan-review-replan-cap, triage-replan-feedback, agent-prompts. - `pnpm check:changesets` passes (2 changesets: `@runfusion/fusion` minor + patch). ## Review Ran a 6-persona `ce-code-review` (correctness + adversarial on Opus; maintainability, testing, project-standards, api-contract). Guards verified unbreakable; no P0/P1 correctness or security issues. Applied the resulting fixes: decoupled the mailbox write from the push gate (P2), `??`→`||` in the feedback derivation (P3), de-duplicated the `specConvergence` ternary (P3), and closed the test-coverage gap the review flagged. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Plans transitioning to **awaiting approval** now write an idempotent, task-linked **dashboard mailbox** message (approval reason + direct task link), even if push/notifications are disabled. - **Bug Fixes** - Plan Review **replan** behavior now better **converges** on prior REVISE feedback (including notes fallback) and stops looping at the replan cap. - At later attempts (attempt 3+), **REVISE** is applied to **critical** issues while lower-severity items shift to suggestions. - **Tests** - Added/expanded coverage for mailbox messaging and spec-convergence prompt wiring. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/plan-approval-mailbox-message.md
Normal file
7
.changeset/plan-approval-mailbox-message.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": minor
|
||||
---
|
||||
|
||||
summary: Plans that need approval now also post a task-linked message to your dashboard mailbox.
|
||||
category: feature
|
||||
dev: NotificationService.handleTaskUpdated writes a `system`-typed mailbox message via `MessageStore.sendMessageOnce` (idempotency key `plan-approval:<taskId>`) on the awaiting-approval transition, alongside the existing ntfy push. Content links to the task using `buildNtfyClickUrl(ntfyDashboardHost, projectId, taskId)`; `system` type avoids re-triggering the `message:agent-to-user` ntfy pipeline. Covers both the manual plan gate and the plan-review-replan-cap escalation.
|
||||
7
.changeset/triage-plan-review-convergence.md
Normal file
7
.changeset/triage-plan-review-convergence.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Stop triage Plan Review from looping to the replan cap by converging the spec reviewer.
|
||||
category: fix
|
||||
dev: reviewStep/buildReviewRequest now thread the reviewer's own prior Plan Review feedback plus the replan attempt (spec gate only); at attempt 3+ the reviewer gates on critical issues only. Reviewer and planner prompts add spec-altitude, prior-issue-verification, front-loaded surface enumeration, and Postgres-only storage ground-truth rules.
|
||||
@@ -71,6 +71,21 @@ describe("resolveAgentPrompt", () => {
|
||||
expect(result).toContain("concrete PROMPT.md edit");
|
||||
});
|
||||
|
||||
// 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("## 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("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+");
|
||||
});
|
||||
|
||||
it("returns the correct built-in prompt for merger when no config provided", () => {
|
||||
const result = resolveAgentPrompt("merger");
|
||||
expect(result).toBeTruthy();
|
||||
|
||||
@@ -314,6 +314,16 @@ 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.
|
||||
|
||||
## Your Role
|
||||
@@ -527,6 +537,18 @@ tests. Manual verification is NOT a test.
|
||||
- If the project has no test framework, the Testing step must include setting one up
|
||||
as part of this task (not just skipping tests)
|
||||
|
||||
## File Scope — front-load surface enumeration
|
||||
|
||||
Before you write \`## File Scope\`, grep the codebase for ALL call-sites of every function/behavior you change AND every persistence/backend path involved (e.g. every store method, backend adapter, migration, and route that touches the data), and enumerate every one of them in the spec. Do not rely on Plan Review to discover missed surfaces incrementally — a spec that lists only the surface the operator pointed at drives the reviewer to surface a deeper missed surface each cycle, which is the main cause of Plan Review replan loops. List the grep-confirmed surfaces so the reviewer confirms coverage rather than hunting for gaps.
|
||||
|
||||
## Storage architecture (ground truth — do not cite removed/nonexistent things)
|
||||
|
||||
Verified facts about this codebase's storage — cite these correctly so Plan Review does not lose cycles rejecting stale claims:
|
||||
- The task store is **PostgreSQL-only**. The legacy SQLite runtime \`Database\` class was removed (\`packages/core/src/db.ts\`); production data access is the async Drizzle \`AsyncDataLayer\`. Do not spec against a SQLite runtime store.
|
||||
- There is **no** \`packages/core/src/task-store/store.ts\`. Do not cite it.
|
||||
- The \`tasks\` table uses a **composite primary key \`(project_id, id)\`** (\`packages/core/src/postgres/schema/project.ts\`). Task updates/scoping must never key by \`id\` alone — always include \`project_id\`.
|
||||
- New Postgres migrations must be **registered explicitly** in \`packages/core/src/postgres/schema-applier.ts\` (version constant + bookkeeping check); a \`.sql\` file dropped in the migrations dir that is not wired there silently never runs.
|
||||
|
||||
## Duplicate check
|
||||
Before writing a spec, first call \`fn_task_list\` to see active tasks, then call \`fn_task_search\` with 2-4 distinct keyword phrases from the task title and description (for example file paths, error symptoms, and symbol names).
|
||||
For any likely match in \`done\` or \`archived\`, call \`fn_task_show\` to inspect details before deciding.
|
||||
@@ -662,6 +684,17 @@ 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.
|
||||
|
||||
@@ -801,6 +834,11 @@ When you must **REVISE**:
|
||||
- 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.
|
||||
@@ -841,6 +879,12 @@ not whether every function and parameter is listed.
|
||||
Good plan: identifies key behavioral changes, calls out risks, has a testing strategy.
|
||||
Do NOT demand function-level implementation checklists.
|
||||
|
||||
## Spec Altitude
|
||||
|
||||
Review specs at spec altitude — the same right-altitude principle as plan review. A spec must name the invariant/behavior, the affected surfaces, and the acceptance test. It does NOT have to pre-decide implementation.
|
||||
Do NOT REVISE a spec to demand exact SQL, lock/CAS protocol design, composite-PK column mapping, or field-level failure-state values — those are implementation decisions verified at code review, not spec gates.
|
||||
REVISE on those grounds ONLY when the spec's stated approach is provably impossible, or it omits a required surface or behavior (not merely its implementation detail).
|
||||
|
||||
## Test Quality Review
|
||||
|
||||
When reviewing tests, check that they verify observable behavior and regression risk (not only implementation trivia).
|
||||
|
||||
@@ -224,6 +224,125 @@ describe("NotificationService", () => {
|
||||
expect(sendNotification).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:20:
|
||||
Awaiting-approval fires BOTH channels: the ntfy push AND a durable, task-linked mailbox
|
||||
message (system-typed, idempotent per task). These tests lock the invariant across the
|
||||
manual gate and the plan-review-replan-cap escalation, and prove best-effort isolation.
|
||||
*/
|
||||
it("writes a task-linked mailbox message when a plan needs approval", async () => {
|
||||
const store = createStore({
|
||||
ntfyEnabled: true,
|
||||
ntfyTopic: "topic",
|
||||
ntfyDashboardHost: "https://dash.example",
|
||||
});
|
||||
const sendMessageOnce = vi.fn(async (input: any, _key: string) => ({
|
||||
message: { ...input, id: "msg-once-x", read: false, createdAt: "", updatedAt: "" },
|
||||
inserted: true,
|
||||
}));
|
||||
const messageStore = Object.assign(new EventEmitter(), { sendMessageOnce });
|
||||
|
||||
const service = new NotificationService(store as any, {
|
||||
projectId: "p1",
|
||||
messageStore: messageStore as any,
|
||||
});
|
||||
await service.start();
|
||||
|
||||
store.emit("task:updated", task({ status: "awaiting-approval" }));
|
||||
await Promise.resolve();
|
||||
|
||||
expect(sendMessageOnce).toHaveBeenCalledTimes(1);
|
||||
const [input, key] = sendMessageOnce.mock.calls[0];
|
||||
expect(key).toBe("plan-approval:FN-1");
|
||||
expect(input.type).toBe("system");
|
||||
expect(input.toId).toBe("dashboard");
|
||||
expect(input.toType).toBe("user");
|
||||
expect(input.metadata).toMatchObject({ taskId: "FN-1", awaitingApprovalReason: "manual" });
|
||||
// Deep link to the task is present in the message body.
|
||||
expect(input.content).toContain("https://dash.example/?project=p1&task=FN-1");
|
||||
});
|
||||
|
||||
it("labels the replan-cap escalation reason in the approval mailbox message", async () => {
|
||||
const store = createStore({ ntfyEnabled: true, ntfyTopic: "topic" });
|
||||
const sendMessageOnce = vi.fn(async (input: any, _key: string) => ({
|
||||
message: { ...input, id: "msg-once-y", read: false, createdAt: "", updatedAt: "" },
|
||||
inserted: true,
|
||||
}));
|
||||
const messageStore = Object.assign(new EventEmitter(), { sendMessageOnce });
|
||||
|
||||
const service = new NotificationService(store as any, { messageStore: messageStore as any });
|
||||
await service.start();
|
||||
|
||||
store.emit(
|
||||
"task:updated",
|
||||
task({ status: "awaiting-approval", awaitingApprovalReason: "plan-review-replan-cap" }),
|
||||
);
|
||||
await Promise.resolve();
|
||||
|
||||
expect(sendMessageOnce).toHaveBeenCalledTimes(1);
|
||||
const [input] = sendMessageOnce.mock.calls[0];
|
||||
expect(input.metadata).toMatchObject({ awaitingApprovalReason: "plan-review-replan-cap" });
|
||||
expect(input.content).toContain("Plan Review exhausted");
|
||||
// No dashboard host configured -> no absolute link line, but the message still sends.
|
||||
expect(input.content).not.toContain("http");
|
||||
});
|
||||
|
||||
it("does not throw when the message store has no write side", async () => {
|
||||
const store = createStore({ ntfyEnabled: true, ntfyTopic: "topic" });
|
||||
const messageStore = new EventEmitter(); // no sendMessageOnce
|
||||
const service = new NotificationService(store as any, { messageStore: messageStore as any });
|
||||
await service.start();
|
||||
|
||||
expect(() => store.emit("task:updated", task({ status: "awaiting-approval" }))).not.toThrow();
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:40:
|
||||
The durable mailbox message is an in-app channel decoupled from the push-enabled gate: a
|
||||
dashboard-only operator with NO ntfy topic and NO webhook (notifications disabled) must still
|
||||
get the awaiting-approval record. Regression guard for the FIX A decoupling.
|
||||
*/
|
||||
it("writes the approval mailbox message even when push notifications are disabled", async () => {
|
||||
const store = createStore({ ntfyEnabled: false }); // no ntfy, no webhook -> notifications disabled
|
||||
const sendMessageOnce = vi.fn(async (input: any, _key: string) => ({
|
||||
message: { ...input, id: "msg-once-disabled", read: false, createdAt: "", updatedAt: "" },
|
||||
inserted: true,
|
||||
}));
|
||||
const messageStore = Object.assign(new EventEmitter(), { sendMessageOnce });
|
||||
|
||||
const service = new NotificationService(store as any, { messageStore: messageStore as any });
|
||||
await service.start();
|
||||
|
||||
store.emit("task:updated", task({ status: "awaiting-approval" }));
|
||||
await Promise.resolve();
|
||||
|
||||
expect(sendMessageOnce).toHaveBeenCalledTimes(1);
|
||||
const [input, key] = sendMessageOnce.mock.calls[0];
|
||||
expect(key).toBe("plan-approval:FN-1");
|
||||
expect(input.type).toBe("system");
|
||||
});
|
||||
|
||||
it("swallows a sendMessageOnce rejection and logs it without throwing", async () => {
|
||||
const store = createStore({ ntfyEnabled: true, ntfyTopic: "topic" });
|
||||
const sendMessageOnce = vi.fn(async () => {
|
||||
throw new Error("mailbox boom");
|
||||
});
|
||||
const messageStore = Object.assign(new EventEmitter(), { sendMessageOnce });
|
||||
|
||||
const service = new NotificationService(store as any, { messageStore: messageStore as any });
|
||||
await service.start();
|
||||
|
||||
expect(() => store.emit("task:updated", task({ status: "awaiting-approval" }))).not.toThrow();
|
||||
// Allow the fire-and-forget mailbox write (and its catch) to settle.
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
|
||||
expect(sendMessageOnce).toHaveBeenCalledTimes(1);
|
||||
expect(schedulerLog.log).toHaveBeenCalledWith(
|
||||
expect.stringContaining("awaiting-approval mailbox message failed: mailbox boom"),
|
||||
);
|
||||
});
|
||||
|
||||
it("stop unsubscribes listeners", async () => {
|
||||
const store = createStore({ ntfyEnabled: true, ntfyTopic: "topic" });
|
||||
const sendNotification = vi.fn(async () => ({ success: true, providerId: "mock" }));
|
||||
|
||||
@@ -52,7 +52,7 @@ function createMockSession(reviewText: string) {
|
||||
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
mockedPromptWithFallback.mockImplementation(async (session, prompt, options) => {
|
||||
mockedPromptWithFallback.mockImplementation(async (session: any, prompt: any, options: any) => {
|
||||
if (typeof session.prompt === "function") {
|
||||
if (options == null) {
|
||||
await session.prompt(prompt);
|
||||
@@ -493,6 +493,104 @@ describe("reviewStep — spec review type", () => {
|
||||
});
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-19:40:
|
||||
Prove the spec-gate convergence block is wired through reviewStep -> buildReviewRequest. We drive
|
||||
the real (module-private) request builder by capturing the prompt string handed to the mocked
|
||||
session, exactly like the "spec review type" tests above — no test-only export is needed because
|
||||
the request text is observable at the session seam.
|
||||
*/
|
||||
describe("reviewStep — spec convergence wiring", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
function captureReviewPrompt(): { getPrompt: () => string } {
|
||||
const state = { prompt: "" };
|
||||
mockedCreateFnAgent.mockResolvedValue({
|
||||
session: {
|
||||
prompt: vi.fn().mockImplementation(async (prompt: string) => {
|
||||
state.prompt = prompt;
|
||||
}),
|
||||
subscribe: vi.fn().mockImplementation((cb: any) => {
|
||||
cb({
|
||||
type: "message_update",
|
||||
assistantMessageEvent: { type: "text_delta", delta: "### Verdict: APPROVE\n### Summary\nOK" },
|
||||
});
|
||||
}),
|
||||
dispose: vi.fn(),
|
||||
},
|
||||
} as any);
|
||||
return { getPrompt: () => state.prompt };
|
||||
}
|
||||
|
||||
it("omits the convergence block for spec reviews on attempt <= 1 or undefined", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 0, "Spec Review", "spec", "# Task: FN-CONV",
|
||||
undefined,
|
||||
{ priorSpecReviewFeedback: "prior REVISE text", specReviewAttempt: 1 },
|
||||
);
|
||||
expect(cap.getPrompt()).not.toContain("## Convergence — Plan Review attempt");
|
||||
expect(cap.getPrompt()).not.toContain("prior REVISE text");
|
||||
});
|
||||
|
||||
it("omits the convergence block for spec reviews when convergence fields are absent", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 0, "Spec Review", "spec", "# Task: FN-CONV",
|
||||
);
|
||||
expect(cap.getPrompt()).not.toContain("## Convergence — Plan Review attempt");
|
||||
});
|
||||
|
||||
it("includes the convergence block + prior feedback + verify-your-own-miss wording at attempt 2", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 0, "Spec Review", "spec", "# Task: FN-CONV",
|
||||
undefined,
|
||||
{ priorSpecReviewFeedback: "PRIOR-REVISE-MARKER: fix the missing Surface Enumeration", specReviewAttempt: 2 },
|
||||
);
|
||||
const prompt = cap.getPrompt();
|
||||
expect(prompt).toContain("## Convergence — Plan Review attempt 2");
|
||||
expect(prompt).toContain("PRIOR-REVISE-MARKER: fix the missing Surface Enumeration");
|
||||
expect(prompt).toContain("VERIFY each issue you raised previously was addressed");
|
||||
expect(prompt).toContain("that is your own earlier miss");
|
||||
// Attempt 2 must NOT yet ratchet severity.
|
||||
expect(prompt).not.toContain("Severity ratchet (attempt 3+)");
|
||||
});
|
||||
|
||||
it("adds the severity ratchet at attempt >= 3", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 0, "Spec Review", "spec", "# Task: FN-CONV",
|
||||
undefined,
|
||||
{ priorSpecReviewFeedback: "prior text", specReviewAttempt: 3 },
|
||||
);
|
||||
const prompt = cap.getPrompt();
|
||||
expect(prompt).toContain("## Convergence — Plan Review attempt 3");
|
||||
expect(prompt).toContain("Severity ratchet (attempt 3+)");
|
||||
});
|
||||
|
||||
it("never includes the convergence block for code reviews even when convergence fields are passed", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 1, "Code Review", "code", "# prompt", "abc123",
|
||||
{ priorSpecReviewFeedback: "prior text", specReviewAttempt: 3 } as any,
|
||||
);
|
||||
expect(cap.getPrompt()).not.toContain("## Convergence — Plan Review attempt");
|
||||
});
|
||||
|
||||
it("never includes the convergence block for plan reviews even when convergence fields are passed", async () => {
|
||||
const cap = captureReviewPrompt();
|
||||
await reviewStep(
|
||||
"/tmp/worktree", "FN-CONV", 1, "Plan Review", "plan", "# prompt",
|
||||
undefined,
|
||||
{ priorSpecReviewFeedback: "prior text", specReviewAttempt: 3 } as any,
|
||||
);
|
||||
expect(cap.getPrompt()).not.toContain("## Convergence — Plan Review attempt");
|
||||
});
|
||||
});
|
||||
|
||||
describe("FN-5928 surface-enumeration review-gate wording", () => {
|
||||
it("requires spec reviews to block missing or incomplete surface enumeration for bug-fix specs", () => {
|
||||
expect(DEFAULT_REVIEWER_PROMPT).toContain("**Surface enumeration:**");
|
||||
|
||||
@@ -233,6 +233,50 @@ describe("Plan Review replan cap", () => {
|
||||
}));
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-19:40:
|
||||
Prove the spec-gate convergence inputs are derived from the latest Plan Review REVISE and passed
|
||||
to reviewStep: priorSpecReviewFeedback = latest REVISE output with a `|| notes` fallback (FIX B —
|
||||
an empty-string output must fall through to notes, not be treated as present), and
|
||||
specReviewAttempt = planReviewReplanCount + 1. `mockReviewStep.mock.calls[0][7]` is the options bag.
|
||||
*/
|
||||
it("passes prior REVISE feedback (empty-output -> notes fallback) and attempt to reviewStep", async () => {
|
||||
const rootDir = await createFixtureRoot();
|
||||
roots.push(rootDir);
|
||||
const task = createRetryTask({
|
||||
id: "FN-REPLAN-CAP-CONVERGE",
|
||||
planReviewReplanCount: 1,
|
||||
workflowStepResults: [
|
||||
{
|
||||
workflowStepId: "plan-review",
|
||||
workflowStepName: "Plan Review",
|
||||
phase: "pre-merge",
|
||||
status: "failed",
|
||||
verdict: "REVISE",
|
||||
// Empty output must NOT win under `||`; notes is the real feedback.
|
||||
output: "",
|
||||
notes: "NOTES-FALLBACK-MARKER: add the missing Surface Enumeration section.",
|
||||
},
|
||||
],
|
||||
} as Partial<Task>);
|
||||
const prompt = `# Task: ${task.id} - Existing draft\n\n## Mission\n\nOnly rewrite after reviewer feedback.\n`;
|
||||
await writePrompt(rootDir, task.id, prompt);
|
||||
const store = createStore(task);
|
||||
mockReviewStep.mockResolvedValue({ verdict: "APPROVE", review: "Approved.", summary: "Ready." });
|
||||
|
||||
await runGate(rootDir, task, store);
|
||||
|
||||
expect(mockReviewStep).toHaveBeenCalled();
|
||||
const options = mockReviewStep.mock.calls[0][7] as {
|
||||
priorSpecReviewFeedback?: string;
|
||||
specReviewAttempt?: number;
|
||||
};
|
||||
expect(options.priorSpecReviewFeedback).toBe(
|
||||
"NOTES-FALLBACK-MARKER: add the missing Surface Enumeration section.",
|
||||
);
|
||||
expect(options.specReviewAttempt).toBe(2);
|
||||
});
|
||||
|
||||
it("resets the replan counter when Plan Review passes", async () => {
|
||||
const rootDir = await createFixtureRoot();
|
||||
roots.push(rootDir);
|
||||
|
||||
@@ -3,14 +3,15 @@ import type {
|
||||
Column,
|
||||
MergeResult,
|
||||
Message,
|
||||
MessageCreateInput,
|
||||
NotificationEvent,
|
||||
NotificationPayload,
|
||||
NotificationProvider,
|
||||
Settings,
|
||||
Task,
|
||||
} from "@fusion/core";
|
||||
import { NotificationDispatcher } from "@fusion/core";
|
||||
import { DEFAULT_NTFY_EVENTS } from "../notifier.js";
|
||||
import { DASHBOARD_USER_ID, NotificationDispatcher } from "@fusion/core";
|
||||
import { DEFAULT_NTFY_EVENTS, buildNtfyClickUrl, formatTaskIdentifier } from "../notifier.js";
|
||||
import { schedulerLog } from "../logger.js";
|
||||
import { classifyTransientMergeError } from "../transient-merge-error-classifier.js";
|
||||
import { NtfyNotificationProvider } from "./ntfy-provider.js";
|
||||
@@ -57,6 +58,16 @@ interface NotificationServiceStore {
|
||||
interface NotificationMessageStore {
|
||||
on(event: "message:sent", listener: (message: Message) => void): void;
|
||||
off?(event: "message:sent", listener: (message: Message) => void): void;
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:20:
|
||||
The awaiting-approval mailbox message is written via sendMessageOnce so replans that
|
||||
re-enter awaiting-approval reuse the deterministic id and do not spam the operator inbox.
|
||||
Optional so test/light message-store fakes without a write side still satisfy the type.
|
||||
*/
|
||||
sendMessageOnce?(
|
||||
input: MessageCreateInput,
|
||||
idempotencyKey: string,
|
||||
): Promise<{ message: Message; inserted: boolean }>;
|
||||
}
|
||||
|
||||
export interface NotificationChatStore {
|
||||
@@ -71,6 +82,14 @@ export class NotificationService {
|
||||
private started = false;
|
||||
private chatStore: NotificationChatStore | undefined;
|
||||
private notificationsEnabled = false;
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:20:
|
||||
Cache the dashboard host from settings.ntfyDashboardHost so the synchronous task:updated
|
||||
handler can build a task deep-link for the awaiting-approval mailbox message without an
|
||||
async settings read. Undefined when the operator has not configured a dashboard host; the
|
||||
mailbox message then falls back to the task identifier without an absolute URL.
|
||||
*/
|
||||
private dashboardHost?: string;
|
||||
private ntfyProvider?: NtfyNotificationProvider;
|
||||
private webhookProvider?: WebhookNotificationProvider;
|
||||
private refreshInFlight: Promise<void> | null = null;
|
||||
@@ -111,6 +130,7 @@ export class NotificationService {
|
||||
|
||||
const settings = await this.store.getSettings();
|
||||
this.setNotificationsEnabledFromSettings(settings);
|
||||
this.dashboardHost = settings.ntfyDashboardHost;
|
||||
this.refreshFailureNotificationSettings(settings);
|
||||
await this.syncNtfyProvider(settings);
|
||||
await this.syncWebhookProvider(settings);
|
||||
@@ -229,6 +249,19 @@ export class NotificationService {
|
||||
private handleTaskUpdated = (task: Task): void => {
|
||||
void this.maybeSuppressTransientFailedNotification(task, `status=${task.status ?? "undefined"}`);
|
||||
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:40:
|
||||
Write the durable awaiting-approval mailbox message BEFORE the push-enabled gate. The ntfy/webhook
|
||||
push respects `notificationsEnabled` (which requires ntfy or a webhook to be configured), but the
|
||||
mailbox message is an in-app channel that needs no push provider. Firing it here means a
|
||||
dashboard-only operator (no ntfy/webhook) still gets the durable, in-dashboard approval record —
|
||||
the whole point of the mailbox channel. Written as a `system` message (not `agent-to-user`) so it
|
||||
does NOT re-trigger the `message:agent-to-user` ntfy pipeline.
|
||||
*/
|
||||
if (task.status === "awaiting-approval") {
|
||||
void this.writeAwaitingApprovalMailboxMessage(task);
|
||||
}
|
||||
|
||||
if (!this.notificationsEnabled) {
|
||||
return;
|
||||
}
|
||||
@@ -273,6 +306,9 @@ export class NotificationService {
|
||||
awaitingApprovalReason: task.awaitingApprovalReason ?? "manual",
|
||||
}),
|
||||
);
|
||||
// FNXC:PlanApprovalMailbox 2026-07-16-19:40: the durable mailbox message is written
|
||||
// unconditionally at the top of handleTaskUpdated (decoupled from `notificationsEnabled`);
|
||||
// only the ntfy push above stays gated on push configuration.
|
||||
}
|
||||
|
||||
if (task.status === "awaiting-user-review") {
|
||||
@@ -293,6 +329,63 @@ export class NotificationService {
|
||||
}
|
||||
};
|
||||
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-19:20:
|
||||
Durable in-dashboard record that a task's plan needs manual approval, linking to the task.
|
||||
- Fires from the same awaiting-approval transition as the ntfy push, covering both entry
|
||||
points (workflow manual plan gate and the Plan Review replan-cap escalation).
|
||||
- `sendMessageOnce` keyed by task id makes it idempotent: replanning that re-enters
|
||||
awaiting-approval, or NotificationService restarts, do not produce duplicate inbox rows.
|
||||
- Best-effort/fire-and-forget: a message-store write failure must never break the ntfy path
|
||||
or the task:updated handler, so errors are swallowed with a log line only.
|
||||
*/
|
||||
private async writeAwaitingApprovalMailboxMessage(task: Task): Promise<void> {
|
||||
/*
|
||||
FNXC:PlanApprovalMailbox 2026-07-16-20:10:
|
||||
Called fire-and-forget (`void this.writeAwaitingApprovalMailboxMessage(task)`), so the ENTIRE
|
||||
body — not just the store write — must be inside try/catch. A throw in payload construction
|
||||
(e.g. formatTaskIdentifier on a malformed runtime task) would otherwise become an unhandled
|
||||
promise rejection that can crash the process. Best-effort: log and swallow.
|
||||
*/
|
||||
try {
|
||||
const messageStore = this.options.messageStore;
|
||||
if (!messageStore?.sendMessageOnce) {
|
||||
return;
|
||||
}
|
||||
const identifier = formatTaskIdentifier(task);
|
||||
const reason = task.awaitingApprovalReason ?? "manual";
|
||||
const reasonLine =
|
||||
reason === "plan-review-replan-cap"
|
||||
? "Plan Review exhausted its automatic revision attempts and escalated this plan for a human decision."
|
||||
: "The generated plan is ready and needs your approval before execution begins.";
|
||||
const link = buildNtfyClickUrl({
|
||||
dashboardHost: this.dashboardHost,
|
||||
projectId: this.options.projectId,
|
||||
taskId: task.id,
|
||||
});
|
||||
const content = [
|
||||
`**${identifier} needs plan approval**`,
|
||||
"",
|
||||
reasonLine,
|
||||
...(link ? ["", `[Open ${task.id}](${link})`] : []),
|
||||
].join("\n");
|
||||
const input: MessageCreateInput = {
|
||||
fromId: "system",
|
||||
fromType: "system",
|
||||
toId: DASHBOARD_USER_ID,
|
||||
toType: "user",
|
||||
type: "system",
|
||||
content,
|
||||
metadata: { taskId: task.id, awaitingApprovalReason: reason },
|
||||
};
|
||||
await messageStore.sendMessageOnce(input, `plan-approval:${task.id}`);
|
||||
} catch (error) {
|
||||
schedulerLog.log(
|
||||
`[notify] ${task.id} awaiting-approval mailbox message failed: ${error instanceof Error ? error.message : String(error)}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
private handleTaskMerged = (result: MergeResult): void => {
|
||||
void this.handleTaskMergedAsync(result);
|
||||
};
|
||||
@@ -319,6 +412,7 @@ export class NotificationService {
|
||||
private handleSettingsUpdated = async (data: { settings: Settings; previous: Settings }): Promise<void> => {
|
||||
const { settings, previous } = data;
|
||||
this.setNotificationsEnabledFromSettings(settings);
|
||||
this.dashboardHost = settings.ntfyDashboardHost;
|
||||
this.refreshFailureNotificationSettings(settings);
|
||||
|
||||
if (
|
||||
|
||||
@@ -132,6 +132,18 @@ export interface ReviewOptions {
|
||||
pluginRunner?: import("./plugin-runner.js").PluginRunner;
|
||||
/** Allow this reviewer to fix in-scope findings in the same session before returning its final verdict. */
|
||||
allowInlineFixes?: boolean;
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-09:20:
|
||||
Spec-gate-only convergence context. The triage Plan Review gate feeds the reviewer its OWN
|
||||
prior REVISE feedback plus the 1-based replan attempt so a re-review VERIFIES its earlier
|
||||
issues were addressed instead of surfacing a fresh, deeper blocking issue every cycle
|
||||
(whack-a-mole/goalpost movement burned all 8 replans on FN-7996/FN-8105/FN-8108). Only
|
||||
injected when reviewType === "spec" and attempt > 1; CODE review and normal PLAN review are
|
||||
unaffected because these fields stay undefined on those paths.
|
||||
*/
|
||||
priorSpecReviewFeedback?: string;
|
||||
/** 1-based current Plan Review attempt (= (task.planReviewReplanCount ?? 0) + 1). Spec gate only. */
|
||||
specReviewAttempt?: number;
|
||||
/**
|
||||
* Fired immediately after the reviewer's `AgentSession` is created. The
|
||||
* caller can register the session in a per-task subagent map so that the
|
||||
@@ -220,8 +232,13 @@ export async function reviewStep(
|
||||
&& reviewType !== "code"
|
||||
&& Boolean(options.store && options.taskId);
|
||||
|
||||
// FNXC:TriagePlanReviewConvergence 2026-07-16-09:20: spec-gate-only convergence context (see ReviewOptions).
|
||||
const specConvergence: SpecReviewConvergence | undefined =
|
||||
reviewType === "spec"
|
||||
? { priorFeedback: options.priorSpecReviewFeedback, attempt: options.specReviewAttempt }
|
||||
: undefined;
|
||||
let request = buildReviewRequest(
|
||||
taskId, stepNumber, stepName, reviewType, promptContent, cwd, baseline, options.userComments,
|
||||
taskId, stepNumber, stepName, reviewType, promptContent, cwd, baseline, options.userComments, specConvergence,
|
||||
);
|
||||
if (options.allowInlineFixes === true) {
|
||||
/*
|
||||
@@ -561,8 +578,10 @@ export async function reviewStep(
|
||||
}
|
||||
|
||||
reviewText = "";
|
||||
// FNXC:TriagePlanReviewConvergence 2026-07-16-09:20: reuse the single `specConvergence`
|
||||
// computed above so the context-limit retry carries byte-identical spec convergence context.
|
||||
let reducedRequest = buildReducedReviewRequest(
|
||||
taskId, stepNumber, stepName, reviewType, promptContent, cwd, baseline, options.userComments,
|
||||
taskId, stepNumber, stepName, reviewType, promptContent, cwd, baseline, options.userComments, specConvergence,
|
||||
);
|
||||
if (options.allowInlineFixes === true) {
|
||||
reducedRequest = appendSameSessionFixPolicy(reducedRequest, reviewType, canWritePromptInline);
|
||||
@@ -827,6 +846,19 @@ function buildReducedTaskPromptSummary(promptContent: string): string {
|
||||
return sections.join("\n\n").trim();
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-09:20:
|
||||
Spec-gate-only convergence context threaded into the review request. `priorFeedback` is the
|
||||
reviewer's own most recent Plan Review REVISE text; `attempt` is the 1-based replan attempt.
|
||||
Present only for reviewType === "spec"; drives the per-attempt convergence + severity-ratchet
|
||||
block injected into the request so the re-review confirms prior issues rather than moving the
|
||||
goalposts each cycle.
|
||||
*/
|
||||
interface SpecReviewConvergence {
|
||||
priorFeedback?: string;
|
||||
attempt?: number;
|
||||
}
|
||||
|
||||
function buildReducedReviewRequest(
|
||||
taskId: string,
|
||||
stepNumber: number,
|
||||
@@ -836,6 +868,7 @@ function buildReducedReviewRequest(
|
||||
cwd: string,
|
||||
baseline?: string,
|
||||
userComments?: TaskComment[],
|
||||
specConvergence?: SpecReviewConvergence,
|
||||
): string {
|
||||
/*
|
||||
FNXC:AgentSteering 2026-06-30-17:09:
|
||||
@@ -851,6 +884,7 @@ function buildReducedReviewRequest(
|
||||
cwd,
|
||||
baseline,
|
||||
userComments,
|
||||
specConvergence,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -863,6 +897,7 @@ function buildReviewRequest(
|
||||
cwd: string,
|
||||
baseline?: string,
|
||||
userComments?: TaskComment[],
|
||||
specConvergence?: SpecReviewConvergence,
|
||||
): string {
|
||||
const parts = [
|
||||
`Review request for task ${taskId}, Step ${stepNumber}: ${stepName}`,
|
||||
@@ -902,6 +937,43 @@ function buildReviewRequest(
|
||||
"- If same-session PROMPT.md repair is available and a fix is local, apply it and APPROVE rather than bouncing to another replan cycle.",
|
||||
);
|
||||
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-09:20:
|
||||
On replan attempt > 1 the reviewer is re-reviewing a spec IT already rejected. Feed it the
|
||||
prior REVISE text and attempt number so it verifies those issues were addressed instead of
|
||||
surfacing a fresh deeper issue each cycle (the whack-a-mole that burned all 8 replans on
|
||||
FN-7996/FN-8105/FN-8108). At attempt >= 3, ratchet severity: gate only on delivery-blocking
|
||||
`critical` issues so a spec that is executable (executor + code review are later gates) does
|
||||
not loop on wording nits.
|
||||
*/
|
||||
const specAttempt = specConvergence?.attempt ?? 0;
|
||||
if (specAttempt > 1) {
|
||||
parts.push(
|
||||
"",
|
||||
`## Convergence — Plan Review attempt ${specAttempt}`,
|
||||
`You (the reviewer) already reviewed an earlier version of this spec; the planner has since revised the PROMPT.md above. This is attempt ${specAttempt}.`,
|
||||
"- VERIFY each issue you raised previously was addressed. REVISE only if (a) a PRIOR blocking issue is still unresolved, or (b) this revision introduced a GENUINELY NEW problem.",
|
||||
"- 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.",
|
||||
);
|
||||
const priorFeedback = specConvergence?.priorFeedback?.trim();
|
||||
if (priorFeedback) {
|
||||
parts.push(
|
||||
"",
|
||||
"Your prior Plan Review feedback (confirm each item is resolved):",
|
||||
"```",
|
||||
priorFeedback,
|
||||
"```",
|
||||
);
|
||||
}
|
||||
if (specAttempt >= 3) {
|
||||
parts.push(
|
||||
"",
|
||||
"### Severity ratchet (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.",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// Add user comment coverage check for spec reviews
|
||||
if (userComments && userComments.length > 0) {
|
||||
parts.push(
|
||||
|
||||
@@ -2365,6 +2365,34 @@ export class TriageProcessor {
|
||||
instead of step-checkbox language that does not match this gate. Inline PROMPT.md
|
||||
repair remains allowed so the reviewer can fix-and-APPROVE instead of REVISE-looping.
|
||||
*/
|
||||
/*
|
||||
FNXC:TriagePlanReviewConvergence 2026-07-16-09:20:
|
||||
Feed the spec reviewer its OWN latest Plan Review REVISE feedback plus the 1-based replan
|
||||
attempt so a re-review verifies prior issues were addressed instead of surfacing a fresh,
|
||||
deeper blocking issue each cycle. Prior feedback is the authoritative REVISE text stored in
|
||||
workflowStepResults (same source blockAfterPlanReviewRevise / the replan seed use); attempt
|
||||
is (planReviewReplanCount ?? 0) + 1. Both are spec-gate-only reviewer inputs — omitting them
|
||||
on attempt 1 (or a fresh task) leaves the reviewer's cold-review behavior unchanged.
|
||||
*/
|
||||
const priorPlanReviewRevise = [...(latestTaskForReview.workflowStepResults || [])]
|
||||
.reverse()
|
||||
.find(
|
||||
(result) =>
|
||||
result.workflowStepId === PLAN_REVIEW_GROUP_ID
|
||||
&& result.verdict === "REVISE"
|
||||
&& Boolean((result.output || result.notes)?.trim()),
|
||||
);
|
||||
const priorSpecReviewFeedback = (priorPlanReviewRevise?.output || priorPlanReviewRevise?.notes)?.trim() || undefined;
|
||||
// FNXC:TriagePlanReviewConvergence 2026-07-16-21:30: derive the attempt from the MAX replan
|
||||
// count across both the caller's `task` snapshot and the fresh `latestTaskForReview`. The
|
||||
// refresh above falls back to the stale `task` when getTask() fails, so reading either one
|
||||
// alone could under-count the attempt and skip the attempt-3 severity ratchet. Max is
|
||||
// monotonic (fresh count >= stale), so it never sends a lower attempt than any snapshot
|
||||
// knows about. This is best-effort convergence context, so a failed refresh must not defer
|
||||
// or block the Plan Review gate — it just uses the best replan count available.
|
||||
const specReviewAttempt =
|
||||
Math.max(task.planReviewReplanCount ?? 0, latestTaskForReview.planReviewReplanCount ?? 0) + 1;
|
||||
|
||||
let reviewFailure: unknown;
|
||||
const review = await reviewStep(
|
||||
this.rootDir,
|
||||
@@ -2385,6 +2413,9 @@ export class TriageProcessor {
|
||||
agentStore: this.options.agentStore,
|
||||
pluginRunner: this.options.pluginRunner,
|
||||
allowInlineFixes: (settings as Settings & { reviewerInlineFixes?: boolean }).reviewerInlineFixes !== false,
|
||||
// FNXC:TriagePlanReviewConvergence 2026-07-16-09:20: spec-gate-only convergence inputs (see derivation above).
|
||||
priorSpecReviewFeedback,
|
||||
specReviewAttempt,
|
||||
onSessionCreated: (session) => this.registerSubagentSession(task.id, session),
|
||||
onSessionEnded: (session) => this.unregisterSubagentSession(task.id, session),
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user