From efebae4cff5b69f3293957dc92abf4aad517b780 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 13:50:28 -0700 Subject: [PATCH 1/7] Add revision-bound plan apply controls --- docs/architecture.md | 10 +- docs/implementation/planning-screen.md | 10 +- test/browser/plans-authoring.spec.ts | 128 ++++++++++++++++++++++++ web/public/app.js | 130 +++++++++++++++++++++++-- web/public/style.css | 12 +++ 5 files changed, 273 insertions(+), 17 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 248f7b69..c68645a4 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -44,7 +44,7 @@ Writing standard: plain language, ISO 24495-1:2023 - **In use for a non-demo review with a `github` block:** planning. `/api/plan/suggestions` and `/api/plan/drafts` ask Claude, through lane D, for suggestion cards or a whole next plan revision (#117, #124). Nothing becomes a revision until you apply it. - **In use with an opt-in `runner` block in `review.json`:** the whole loop after planning. `start` and `resume` on `/api/runner` run a task's plan item by item with a Claude agent (#91). They require current plan-item approvals, bind both the task state and review version, recheck approvals before each later item, and refuse when a completed prefix no longer ends at the task's current head (#107). After an out-of-scope pause, an approved amended plan can reconcile the completed prefix and resume only the unfinished suffix (#88, PR #134). When a run ends, codeboost publishes the task's pull request, a draft if the task needs a person (#103). Approve & merge merges that pull request (#121). Cancelling a task closes its pull requests (#111). These are API actions; the screen has no buttons for them yet. - **Partly built for pre-merge automation:** the trusted local rebase engine records one-to-one commit mappings, verifies the resulting checkout byte for byte, owns its subprocesses durably, and recovers interrupted work (#22, PR #136). Its production conflict path gives a sandboxed agent only the bounded conflict files and imports only audited results for both foreign and plan-owned commits (PRs #138, #140 and #144). No production request invokes the rebaser yet. - - **Not built:** applying draft or suggestion results from the screen, screens for the runner actions, production rebase admission, post-rebase attribution and approval refresh, head-bound `cmd:` checks, remote push/check refresh, review rounds, and the Queue and Learning screens. + - **Not built:** screens for the runner actions, production rebase admission, post-rebase attribution and approval refresh, head-bound `cmd:` checks, remote push/check refresh, review rounds, and the Queue and Learning screens. ## Terms used @@ -164,7 +164,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out |---|---|---|---| | Command line | `web/cli.ts` | Parses `--demo` or `--config`, takes the single-runner lock, starts the server, releases the lock on Ctrl+C. | In use | | HTTP server | `web/server.ts` | Binds `127.0.0.1` only. Checks the token, Host and Origin. Bounded JSON bodies. Routes `/api/review`, `/api/action`, `/api/questions`, `/api/merge`, `/api/issues`, `/api/settings`, plus `/api/runner` and `/api/plan/*` when a runner or planning provider is supplied. Ordered shutdown. | In use; `/api/plan/suggestions` and `/api/plan/drafts` run Claude through lane D for a review with a `github` block (#117, #124) | -| Browser screen | `web/public/*` | Review, Issues and Plans screens. Plans displays and imports revisions, and starts, polls, renders and dismisses draft or suggestion requests through the planning API. Applying those results remains future work. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | In use | +| Browser screen | `web/public/*` | Review, Issues and Plans screens. Plans displays and imports revisions, starts and observes draft or suggestion requests, and applies a ready result with revision-bound idempotency. Plain JavaScript with IBM Plex fonts served locally. Sends item and segment IDs and a state token, never fingerprints or ownership. | In use | | Review service | `runner/review.ts` | Builds the review view: reads history, links it to the plan, computes approval states and merge blockers. Applies review commands through the store. | In use | | Ask | `runner/questions.ts`, `runner/question-*.ts` | Answers a question about one plan item with a read-only Claude agent (Codex is refused in every phase, #93). Runs lane D's setup in a worker thread so the server stays responsive. Ask and planning share one read-only container runner (`runReadOnlyAgent`, #117) but keep separate workers, owners and leftovers. Labels its Docker objects with a per-database Ask owner and recovers only its own leftovers, so reviews sharing one Docker daemon do not block each other (#95). | In use | | Merge coordinator | `runner/merge.ts` | Runs the full merge gate twice, re-reads the local generation, then merges the exact reviewed head. Tracks merge-queue attempts. For a runner task it inspects and merges the task's own published pull request; a configured `github.pullRequest` that differs is refused (#121). | In use | @@ -180,7 +180,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | In use | | Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | In use | | Issue ranking | `core/issue-ranking.ts`, `github/issues.ts`, `web/issues.ts` | Deterministic, explainable issue score. Repository-scoped trust of issue authors. | In use | -| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); Plans starts and displays authoring requests, while applying their results remains future work | +| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); Plans starts, displays and revision-bound applies authoring results | | Execution prompt and audit | `core/execution-prompt.ts`, `core/run-audit.ts`, `prompts/execute.md` | Builds the execute and fix prompt with untrusted text only in escaped data blocks. Audits the change manifest after a run: safety violations first (Git metadata, links that leave the repository, agent commits), then scope against the declared files. | In use with the opt-in `runner` block | | Pull request text | `core/pull-request-body.ts` | Title and description with the plan. Plan and agent text go in fenced blocks, and issue references and @-mentions are neutralized, so the text cannot close other issues or notify people. | In use with the opt-in `runner` block | | Git adapter | `git/history.ts`, `git/clone.ts` | Bounded, hardened history reads. Standalone task clones with no links back to your repository. Every Git command runs with an allowlisted environment. | In use | @@ -413,7 +413,7 @@ These are the decisions that shape the structure. The full list, with the reason | F | Runner | Merged: F1 (lifecycle store, coordinator, shutdown wiring, startup recovery and the lock, planning API, feedback events, and stops that wait for their commit: #53, #74, #57, #59, #60, #96); F2a–F2b (execute prompts, audit, per-item execution, real workspace, durable safety findings: #67, #68, #92, #94, #128); F2d (already-fixed check, PR opening, branch pusher, production publishing, closing PRs on cancel: #84, #101, #110, #115, #116, #119, #123); production wiring and `start`/`resume` (#91, PRs #102, #105); planning through lane D (#117, #124, PRs #118, #120, #125); merging the task's own PR (#121, PR #126); start/resume approval, review-version, recovery and completed-prefix guards (#107, PR #130); continuation after a scope pause (#88, PR #134); the F3 durable trusted local-rebase foundation (#22, PR #136); the F4 foreign-conflict engine plus production sandbox adapter (#22, PRs #138 and #140); and sandboxed owned-commit conflict resolution (#22, PR #144). Next in #22: F5 owns current base/head refresh, rebase admission, post-rebase attribution and approval refresh, and head-bound `cmd:` execution. F6 owns rewritten-head push, fresh required-check refresh, the already-fixed rerun, and exact-pair guarded-merge handoff. The integrated flow pushes a rewritten head before it recomputes review state and reruns every plan item's head-bound `cmd:` checks; fresh required GitHub checks follow. | | Hardening | Allowlisted subprocess environments | Done: every Git command (#82, #83) and every `gh` runner (#84, #86). | | Tests | Test suite reliability | Done. The full suite runs ordinary tests first and Docker-backed files serially; CI and the real-Docker gate passed on PR #132 (#129). | -| G | Planning screen | G1 and G2 built (#145, PRs #148 and #150): display/import the current plan, request a next-revision draft or suggestion cards, and render or dismiss the result. G3 revision-bound Apply and G4 real-provider integration acceptance remain. | +| G | Planning screen | G1 through G3 built (#145, PRs #148 and #150): display/import the current plan, request a next-revision draft or suggestion cards, render or dismiss the result, and revision-bound Apply with exact ambiguous replay. G4 real-provider integration acceptance remains. | | H | Issue ranking and Issues screen | Done. H4b (#108, PR #139) records author-bound trust, guards runner and publishing boundaries, controls which comments reach prompts, records prompt comment evidence, and provides trust/untrust controls on the Issues screen. | | I, J | Queue and run windows; learning from feedback | Not started (after F) | @@ -424,7 +424,7 @@ These are the decisions that shape the structure. The full list, with the reason - The container's guarantees are those of Docker. The agent can always reach its own vendor account. - `cmd:` acceptance checks do not run yet, so any plan item with a `cmd:` check blocks merging. - One user, one machine, one runner per database. The lock refuses a second runner, and it refuses network filesystems, where OS locks are unreliable. -- The runner is opt-in: it needs a `runner` block and a `github` block in `review.json`, and it is always off in the demo. It runs through the API only; the screen has no runner start, resume, publish or plan-result Apply controls yet. Planning authoring and suggestions are available when the planning provider is configured. Publishing also needs `github.baseBranch`. +- The runner is opt-in: it needs a `runner` block and a `github` block in `review.json`, and it is always off in the demo. It runs through the API only; the screen has no runner start, resume or publish controls yet. Planning authoring, suggestions and revision-bound Apply are available when the planning provider is configured. Publishing also needs `github.baseBranch`. - codeboost is Claude-only. Codex is refused in every phase (#93, PR #106), because it can read files only through its shell, which every phase turns off. `agent-isolation.md` says when to revisit this. ## Test this document with a reader diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 244826fa..0d43e367 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -1,10 +1,12 @@ # Planning screen -**Lane and steps:** lane G, G1 and G2. Tracking issue: #145. +**Lane and steps:** lane G, G1 through G3. Tracking issue: #145. G1 adds a Plans screen over the planning contracts already owned by lanes E and F. It does not add another plan writer: the browser imports through `POST /api/plan/import`, then reloads `GET /api/review` so the current Store revision remains the only displayed authority. -G2 adds browser controls for the existing draft and suggestion request lifecycles. The screen can ask for a complete next-revision draft or independent edit cards, poll the durable request, render the result and dismiss it. It still cannot apply either result; Apply remains G3. +G2 adds browser controls for the existing draft and suggestion request lifecycles. The screen can ask for a complete next-revision draft or independent edit cards, poll the durable request, render the result and dismiss it. + +G3 applies a ready draft or one suggestion card through the Store-owned revision-bound endpoint. The browser never constructs a new plan. It retains one action ID across an ambiguous Apply retry, accepts only the expected next revision, then reloads the authoritative review. Once Apply commits, a reload failure is reported separately and cannot re-enable the action. Applying one card consumes the response; sibling cards stay disabled and **Refresh suggestions** sends their bounded operation, item and summary descriptions in a new revision-bound request without replacing newer guidance in the textarea. ## Display contract @@ -28,6 +30,6 @@ After start, the screen polls only the durable request resource with bounded bac Suggestion replies render as plain text followed by one line-separated row per typed edit. Drafts render the proposed revision summary and ordered items. Before rendering a ready result, the browser validates the complete v1 plan or edit-reply structure, including exact object fields, bounded nested file/check/item data and each edit operation's used and null payloads. The browser escapes every agent-provided field. A completed, non-cancelled result remains visible as history but is marked stale whenever its lifecycle is invalidated or consumed, or its captured revision or snapshot no longer matches the current review response. Dismissed output is removed, and a poll started before dismissal cannot restore it. A delayed ready poll likewise cannot present an old result as current after an import or refresh advances the plan. -`test/browser/plans-authoring.spec.ts` covers duplicate activation, focus and disabled styling, request-time guidance preservation, exact ambiguous-start replay, a start refused after an intervening import, malformed-status ownership, failed states with impossible output, consumed-result stale labeling, rejection of mismatched bindings, wrong-operation payloads, incomplete edit cards and incomplete nested draft data, hostile markup and attribute text in both result types, retained completed history and invalidated-history labeling, suggestion rows, whole-draft preview without mutation, dismissal and ambiguous-cancellation reconciliation without a revision change, an older poll returning after dismissal, and the interleaving where an import advances the plan before an older ready poll returns and removes the stale dismissal action. +`test/browser/plans-authoring.spec.ts` covers duplicate activation, focus and disabled styling, request-time guidance preservation, exact ambiguous-start replay, a start refused after an intervening import, malformed-status ownership, failed states with impossible output, consumed-result stale labeling, rejection of mismatched bindings, wrong-operation payloads, incomplete edit cards and incomplete nested draft data, hostile markup and attribute text in both result types, retained completed history and invalidated-history labeling, suggestion rows, whole-draft preview without mutation, dismissal and ambiguous-cancellation reconciliation without a revision change, an older poll returning after dismissal, the interleaving where an import advances the plan before an older ready poll returns, single-flight draft Apply with Review text and snippet preservation, committed-Apply reporting when the authoritative reload fails, and exact ambiguous Apply replay followed by sibling refresh and a second revision-bound Apply. -G3 adds revision-bound Apply and request-time draft/attachment preservation. G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. +G4 completes real-provider/store integration and the remaining T18 browser/adapter gate. diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 2eda984f..f9d56936 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -163,6 +163,132 @@ test('renders a whole next-revision draft without changing the current plan', as expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); }); +test('applies a draft once while preserving newer review text and its attachment', async ({ page }) => { + const { deps, invocations } = planning(); + app = await startServer(createDemo(join(root, 'demo')), 0, undefined, undefined, undefined, undefined, undefined, deps); + await page.goto(app.url); + await page.locator('.added [data-line]').first().click(); + await page.getByRole('button', { name: 'Ask about selection', exact: true }).click(); + await page.getByLabel('Question about this item').fill('Before Apply'); + await expect(page.locator('#attachment')).toContainText('retry.ts'); + await page.getByRole('link', { name: 'Plans', exact: true }).click(); + const current = app.service.store.getPlan(app.service.config.identity); + await page.getByRole('button', { name: 'Draft next revision' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(draft(current))); + const apply = page.getByRole('button', { name: 'Apply draft', exact: true }); + await expect(apply).toBeVisible(); + + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + let applies = 0; + await page.route('**/api/plan/drafts/*/apply', async route => { applies++; arrived(); await held; await route.continue(); }); + await apply.focus(); + await apply.evaluate((button: HTMLButtonElement) => { button.click(); button.click(); }); + await captured; + expect(applies).toBe(1); + await expect(page.getByRole('button', { name: 'Applying…', exact: true })).toBeFocused(); + await expect(page.getByRole('button', { name: 'Applying…', exact: true })).toHaveAttribute('aria-disabled', 'true'); + await page.getByRole('link', { name: 'Review', exact: true }).click(); + await page.getByLabel('Question about this item').fill('Edited while Apply was in flight'); + release(); + + await expect(page.locator('#issue')).toContainText('r2'); + await expect(page.getByLabel('Question about this item')).toHaveValue('Edited while Apply was in flight'); + await expect(page.locator('#attachment')).toContainText('retry.ts'); + await expect(page.locator('#attachment')).not.toContainText('Outdated'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + +test('keeps a committed draft applied when the authoritative reload fails', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const current = app.service.store.getPlan(app.service.config.identity); + await page.getByRole('button', { name: 'Draft next revision' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(draft(current))); + let reloads = 0; + await page.route('**/api/review', async route => { + reloads++; + if (reloads === 1) await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Temporary review read failure' }) }); + else await route.continue(); + }); + await page.getByRole('button', { name: 'Apply draft', exact: true }).click(); + + await expect(page.locator('#plans-status')).toContainText('Revision r2 was applied, but the current plan could not reload'); + await expect(page.getByRole('button', { name: 'Apply draft', exact: true })).toHaveCount(0); + await expect(page.getByRole('button', { name: 'Retry Apply', exact: true })).toHaveCount(0); + await expect(page.locator('#plans-revision')).toHaveText('r1'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); + await page.locator('#plans-refresh').click(); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + expect(reloads).toBe(2); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); +}); + +test('keeps Apply retryable when success names the wrong revision', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const current = app.service.store.getPlan(app.service.config.identity); + await page.getByRole('button', { name: 'Draft next revision' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(draft(current))); + await page.route('**/api/plan/drafts/*/apply', route => route.fulfill({ + status: 200, contentType: 'application/json', body: JSON.stringify({ result: { revision: 3 } }), + })); + await page.getByRole('button', { name: 'Apply draft', exact: true }).click(); + + await expect(page.getByRole('button', { name: 'Retry Apply', exact: true })).toBeVisible(); + await expect(page.locator('#plans-status')).toContainText('Apply returned success without the expected revision'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); +}); + +test('replays an ambiguous suggestion Apply, stales siblings, and refreshes before the next edit', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const first = suggestions(); + first.edits.push({ ...first.edits[0]!, summary: 'Clarify retry intent', reason: 'The intent should name the visible limit.', + field: 'intent', value: 'Explain the operator-visible retry limit.' }); + await page.getByLabel('Guidance').fill('Improve the retry item.'); + await page.getByRole('button', { name: 'Suggest edits' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(first)); + await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(2); + await page.getByLabel('Guidance').fill('Keep this newer guidance.'); + + const sent: unknown[] = []; + await page.route('**/api/plan/suggestions/*/apply', async route => { + sent.push(route.request().postDataJSON()); + if (sent.length === 1) { await route.fetch(); await route.abort('failed'); } + else await route.continue(); + }); + await page.getByRole('button', { name: 'Apply this edit' }).first().click(); + await expect(page.getByRole('button', { name: 'Retry Apply', exact: true })).toBeVisible(); + await expect(page.locator('#plan-suggest')).toHaveAttribute('aria-disabled', 'true'); + await page.locator('#plan-suggest').evaluate((button: HTMLButtonElement) => { button.click(); button.click(); }); + expect(invocations).toHaveLength(1); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); + await page.getByRole('button', { name: 'Retry Apply', exact: true }).click(); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + expect(sent).toHaveLength(2); + expect(sent[1]).toEqual(sent[0]); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); + await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(0); + await expect(page.getByText('Plan changed — refresh suggestions', { exact: false })).toBeVisible(); + + await page.getByRole('button', { name: 'Refresh suggestions', exact: true }).click(); + await expect.poll(() => invocations.length).toBe(2); + expect(invocations[1]!.request).toMatchObject({ mode: 'suggest', revision: 2 }); + expect(invocations[1]!.request.prompt).toContain('Clarify retry intent'); + await expect(page.getByLabel('Guidance')).toHaveValue('Keep this newer guidance.'); + invocations[1]!.resolve(JSON.stringify(suggestions(2))); + await expect(page.getByRole('button', { name: 'Apply this edit', exact: true })).toBeVisible(); + await page.getByRole('button', { name: 'Apply this edit', exact: true }).click(); + await expect(page.locator('#plans-revision')).toHaveText('r3'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(3); +}); + test('retries an ambiguous suggestion start with the exact request', async ({ page }) => { const { deps, invocations } = planning(); await openPlans(page, deps); @@ -264,6 +390,7 @@ test('rejects failed output and marks consumed output stale', async ({ page }) = await expect(page.getByRole('status').filter({ hasText: 'Stale suggestions · generated for r1' })).toBeVisible(); await expect(page.getByRole('article', { name: 'Name the retry ceiling' })).toBeVisible(); await expect(page.getByRole('button', { name: 'Dismiss suggestions' })).toHaveCount(0); + await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(0); await expect(suggest).not.toHaveAttribute('aria-disabled', 'true'); }); @@ -586,5 +713,6 @@ test('marks a delayed suggestion poll stale after an import advances the plan', await expect(page.getByRole('status').filter({ hasText: 'Stale suggestions · generated for r1' })).toBeVisible(); await expect(page.getByRole('article', { name: 'Name the retry ceiling' })).toBeVisible(); await expect(page.getByRole('button', { name: 'Dismiss suggestions' })).toHaveCount(0); + await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(0); expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); diff --git a/web/public/app.js b/web/public/app.js index a00675f7..0b3398ea 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -930,7 +930,7 @@ function supersedePlanRefresh() { claimPlanStatus("neutral", ""); } function planAuthorBusy() { - return ["starting", "pending", "cancelling"].includes(planAuthorRequest?.state); + return ["starting", "pending", "cancelling", "applying"].includes(planAuthorRequest?.state); } function planAuthorStale(request = planAuthorRequest) { return !!request && (["invalidated", "consumed"].includes(request.state) || !data?.plan || @@ -1040,6 +1040,18 @@ function validPlanAuthorStatus(status, request) { if (["ready", "consumed"].includes(status.state)) return !!validResult; return result === null || !!validResult; } +function planApplyButton(request, index, stale) { + const selectedApply = request.applyingIndex === index; + const uncertain = request.state === "apply-uncertain"; + const visible = (request.state === "ready" && !stale) || request.state === "applying" || uncertain; + if (!visible) return ""; + const disabled = request.state === "applying" || (uncertain && !selectedApply); + const label = request.state === "applying" && selectedApply ? "Applying…" + : uncertain && selectedApply ? "Retry Apply" + : index === null ? "Apply draft" : "Apply this edit"; + const suffix = index === null ? "draft" : String(index); + return ``; +} function planAuthorResultMarkup(request, historical = false) { if (request.state === "cancelled") return ""; const stale = planAuthorStale(request); @@ -1048,24 +1060,29 @@ function planAuthorResultMarkup(request, historical = false) { if (request.mode === "suggest" && request.reply) { result = `
${historical ? "Earlier suggested edits" : "Suggested edits"} r${esc(request.revision)}${historyMark}
${request.reply.reply ? `

${esc(request.reply.reply)}

` : ""} - ${request.reply.edits.map((edit) => `
${esc(edit.item)}

${esc(edit.summary)}

${esc(edit.reason)}

${esc(describePlanEdit(edit))}

`).join("") || '

No edits were suggested.

'}`; + ${request.reply.edits.map((edit, index) => `
${esc(edit.item)}

${esc(edit.summary)}

${esc(edit.reason)}

${esc(describePlanEdit(edit))}

${historical ? "" : `
${planApplyButton(request, index, stale)}
`}
`).join("") || '

No edits were suggested.

'}`; } else if (request.mode === "draft" && request.plan) { result = `
${historical ? "Earlier draft revision" : "Draft revision"} r${esc(request.plan.revision)}${historyMark}

${esc(request.plan.summary)}

-
    ${request.plan.items.map((item) => `
  • ${esc(item.id)} ${esc(item.title)}
    • `).join("")}
`; +
    ${request.plan.items.map((item) => `
  • ${esc(item.id)} ${esc(item.title)}
    • `).join("")}
${historical ? "" : `
${planApplyButton(request, null, stale)}
`}`; } const dismissible = request.state === "cancelling" || (request.state === "ready" && !stale); if (!historical && result && request.requestId && dismissible) result += `
`; + const remaining = request.mode === "suggest" && request.state === "consumed" && Number.isSafeInteger(request.appliedIndex) + ? request.reply.edits.filter((_, index) => index !== request.appliedIndex) : []; + if (!historical && remaining.length) + result += '
! Plan changed — refresh suggestions
'; return result; } function renderPlanAuthor() { const restoreDismissFocus = document.activeElement?.id === "plan-author-dismiss"; + const restoreApplyFocus = document.activeElement?.id?.startsWith("plan-author-apply-") ? document.activeElement.id : null; const request = planAuthorRequest, unavailable = !data?.plan; - const busyAuthor = planAuthorBusy(), uncertain = request?.state === "uncertain"; + const busyAuthor = planAuthorBusy(), uncertain = request?.state === "uncertain", applyUncertain = request?.state === "apply-uncertain"; for (const [id, mode, label] of [["plan-draft", "draft", "Draft next revision"], ["plan-suggest", "suggest", "Suggest edits"]]) { const button = $(id); - const blocked = unavailable || busyAuthor || (uncertain && request.mode !== mode); + const blocked = unavailable || busyAuthor || applyUncertain || (uncertain && request.mode !== mode); if (blocked) button.setAttribute("aria-disabled", "true"); else button.removeAttribute("aria-disabled"); button.textContent = request?.state === "starting" && request.mode === mode ? "Starting…" @@ -1085,6 +1102,8 @@ function renderPlanAuthor() { let tone = "neutral", message = ""; if (request.state === "starting") message = `Starting ${noun}…`; else if (request.state === "uncertain") { tone = "warn"; message = `! The ${noun} request outcome is unknown. Retry the exact request.`; } + else if (request.state === "applying") message = `Applying ${request.mode === "draft" ? "draft" : "suggestion"} to r${request.revision}…`; + else if (request.state === "apply-uncertain") { tone = "warn"; message = `! The Apply outcome is unknown. Retry the exact Apply action.`; } else if (request.state === "failed") { tone = "bad"; message = `✕ ${request.mode === "draft" ? "Draft" : "Suggestion"} failed. ${request.reason || "Try again."}`; } else if (request.state === "cancelling") message = `Cancelling ${noun}…`; else if (request.state === "cancelled") message = `– ${request.mode === "draft" ? "Draft" : "Suggestions"} dismissed. ${request.reason || ""}`.trim(); @@ -1097,7 +1116,11 @@ function renderPlanAuthor() { $("plan-author-status").textContent = message; $("plan-author-result").innerHTML = [planAuthorResultMarkup(request), ...planAuthorHistory.map(entry => planAuthorResultMarkup(entry, true))].join(""); $("plan-author-dismiss")?.addEventListener("click", dismissPlanAuthor); + document.querySelectorAll("[data-plan-apply]").forEach(button => button.addEventListener("click", () => + applyPlanAuthor(button.dataset.planApply === "draft" ? null : Number(button.dataset.planApply)))); + $("plan-author-refresh-suggestions")?.addEventListener("click", refreshPlanSuggestions); if (restoreDismissFocus) $("plan-author-dismiss")?.focus(); + else if (restoreApplyFocus) $(restoreApplyFocus)?.focus(); } function renderPlans() { if (!data?.plan) { @@ -1261,8 +1284,8 @@ async function pollPlanAuthor(generation) { schedulePlanAuthorPoll(generation); } } -async function startPlanAuthor(mode) { - if (!data?.plan || planAuthorBusy()) return; +async function startPlanAuthor(mode, feedbackOverride = null) { + if (!data?.plan || planAuthorBusy() || planAuthorRequest?.state === "apply-uncertain") return; if (planImportPending || planRefreshPending || busy) { $("plan-author-status").className = "warn"; $("plan-author-status").textContent = "! Wait for the current plan or review action before asking the agent."; @@ -1273,7 +1296,7 @@ async function startPlanAuthor(mode) { const requestBody = retry ? planAuthorRequest.request : { expectedRevision: data.plan.revision, snapshotId: data.snapshot.id, - feedback: $("plan-feedback").value, + feedback: feedbackOverride ?? $("plan-feedback").value, actionId: crypto.randomUUID(), }; if (planAuthorRequest?.reply || planAuthorRequest?.plan) @@ -1294,6 +1317,9 @@ async function startPlanAuthor(mode) { pollDelay: planAuthorPollInitial, pollTimer: null, cancelActionId: null, + applyAction: null, + applyingIndex: null, + appliedIndex: null, operationGeneration: 0, }; planAuthorRequest = request; @@ -1316,6 +1342,94 @@ async function startPlanAuthor(mode) { renderPlanAuthor(); } } +function refreshPlanSuggestions() { + const request = planAuthorRequest; + if (!request || request.mode !== "suggest" || request.state !== "consumed" || !Number.isSafeInteger(request.appliedIndex)) return; + const remaining = request.reply.edits.filter((_, index) => index !== request.appliedIndex); + if (!remaining.length) return; + const feedback = `Re-evaluate these unapplied suggestions against the current plan. Do not assume their old indexes or payloads are still valid:\n${remaining.map(edit => `- ${edit.op} ${edit.item}: ${edit.summary}`).join("\n")}`; + startPlanAuthor("suggest", feedback); +} +async function applyPlanAuthor(index) { + const request = planAuthorRequest; + const retry = request?.state === "apply-uncertain" && request.applyingIndex === index; + if (!request?.requestId || (!retry && (request.state !== "ready" || planAuthorStale(request))) || + (request.mode === "draft" ? index !== null : !Number.isSafeInteger(index) || !request.reply.edits[index]) || + planImportPending || planRefreshPending || busy) return; + supersedePlanRefresh(); + const generation = ++planOperationGeneration; + const sharedGeneration = ++reviewGeneration; + const mergeObservationOwner = mergeObservationGeneration; + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; + const busyOwner = beginBusy("write"); + request.operationGeneration++; + if (request.pollTimer) clearTimeout(request.pollTimer); + request.pollTimer = null; + request.applyAction ??= { actionId: crypto.randomUUID(), index }; + request.applyingIndex = index; + request.state = "applying"; + request.reason = null; + request.observationError = null; + const statusOwner = claimPlanStatus("neutral", `Applying ${request.mode === "draft" ? "draft" : "suggestion"}…`); + rememberDraft(); + renderPlanAuthor(); + try { + const response = await api(`/api/plan/${request.mode === "draft" ? "drafts" : "suggestions"}/${request.requestId}/apply`, { + actionId: request.applyAction.actionId, + ...(request.mode === "suggest" ? { index } : {}), + }); + const revision = response.result?.revision; + if (!Number.isSafeInteger(revision) || revision !== request.revision + 1) + throw Object.assign(new Error("Apply returned success without the expected revision."), { outcomeUnknown: true }); + request.state = "consumed"; + request.appliedIndex = index; + request.applyAction = null; + request.reason = null; + if (data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, + blockers: [{ code: "plan-changed", message: "The plan changed. Refresh before merging." }] } }; + renderMerge(); + setPlanStatus(statusOwner, "good", `✓ Applied revision r${revision}. Reloading…`); + try { + const updated = await api("/api/review"); + if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration || planAuthorRequest !== request) return; + if (!updated?.plan || updated.plan.revision !== revision) + throw new Error(`The authoritative review did not return applied revision r${revision}.`); + rememberDraft(); + mergeGeneration++; + if (mergePollTimer) clearTimeout(mergePollTimer); + mergePollTimer = null; + mergePollState = null; + mergePollDelay = 2000; + const newerQueue = newerMergeQueue(updated, mergeObservationOwner); + data = preserveSettledQuestionAnswers(newerQueue ? withMergeQueue(updated, newerQueue) : updated); + render(); + if (newerQueue) showMergeQueueStatus(newerQueue); + setPlanStatus(statusOwner, "good", `✓ Revision r${data.plan.revision} is current.`); + } catch (error) { + request.observationError = `Revision r${revision} was applied, but the current plan could not reload. ${error.message}`; + renderPlanAuthor(); + setPlanStatus(statusOwner, "warn", `! Revision r${revision} was applied, but the current plan could not reload. ${error.message}`); + renderMerge(); + } + } catch (error) { + if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration || planAuthorRequest !== request) return; + const ambiguous = !Number.isSafeInteger(error?.status) || error.status === 503 || error.outcomeUnknown === true; + request.state = ambiguous ? "apply-uncertain" : "invalidated"; + request.reason = error.message; + if (!ambiguous) request.applyAction = null; + if (data?.merge?.available) data = { ...data, merge: { ...data.merge, ready: false, action: null, + blockers: [{ code: ambiguous ? "plan-apply-unknown" : "plan-apply-refused", message: `${error.message} Refresh before merging.` }] } }; + renderPlanAuthor(); + renderMerge(); + setPlanStatus(statusOwner, ambiguous ? "warn" : "bad", `${ambiguous ? "!" : "✕"} Could not confirm Apply. ${error.message}`); + } finally { + endBusy(busyOwner); + } +} async function dismissPlanAuthor() { const request = planAuthorRequest; if (!request?.requestId || planAuthorBusy()) return; diff --git a/web/public/style.css b/web/public/style.css index 5bf44d78..f767008a 100644 --- a/web/public/style.css +++ b/web/public/style.css @@ -893,6 +893,14 @@ body.resizing-conversation { cursor: col-resize; user-select: none; } font-family: "IBM Plex Mono", Menlo, Consolas, monospace; font-size: 12px; } +.suggestion-actions { + display: flex; + justify-content: flex-end; + margin-top: 12px; +} +.suggestion-actions button { + min-width: 104px; +} .draft-items { list-style: none; margin: 8px 0 0; @@ -903,6 +911,10 @@ body.resizing-conversation { cursor: col-resize; user-select: none; } border-top: 1px solid var(--line); } .plan-author-result-actions { + display: flex; + align-items: center; + justify-content: space-between; + gap: 12px; padding: 12px 16px; border-top: 1px solid var(--line); } From 7dcb312b70a38eca3cda3d4e9a3a8de8f412fce7 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 13:59:33 -0700 Subject: [PATCH 2/7] Keep plan apply recovery retryable --- docs/implementation/planning-screen.md | 2 +- test/browser/plans-authoring.spec.ts | 33 ++++++++++++++++++++++++++ test/planning-drafts-api.test.ts | 16 ++++++++++++- web/public/app.js | 9 ++++--- web/server.ts | 3 +++ 5 files changed, 58 insertions(+), 5 deletions(-) diff --git a/docs/implementation/planning-screen.md b/docs/implementation/planning-screen.md index 0d43e367..682927a9 100644 --- a/docs/implementation/planning-screen.md +++ b/docs/implementation/planning-screen.md @@ -6,7 +6,7 @@ G1 adds a Plans screen over the planning contracts already owned by lanes E and G2 adds browser controls for the existing draft and suggestion request lifecycles. The screen can ask for a complete next-revision draft or independent edit cards, poll the durable request, render the result and dismiss it. -G3 applies a ready draft or one suggestion card through the Store-owned revision-bound endpoint. The browser never constructs a new plan. It retains one action ID across an ambiguous Apply retry, accepts only the expected next revision, then reloads the authoritative review. Once Apply commits, a reload failure is reported separately and cannot re-enable the action. Applying one card consumes the response; sibling cards stay disabled and **Refresh suggestions** sends their bounded operation, item and summary descriptions in a new revision-bound request without replacing newer guidance in the textarea. +G3 applies a ready draft or one suggestion card through the Store-owned revision-bound endpoint. The browser never constructs a new plan. It retains one action ID across an ambiguous Apply retry, accepts only the expected next revision, then reloads the authoritative review. Once Apply commits, a reload failure is reported separately and cannot re-enable the action or start authoring against the old revision; a manual refresh must observe the applied revision first. Applying one card consumes the response; sibling cards stay disabled and **Refresh suggestions** sends their bounded operation, item and summary descriptions in a new revision-bound request without replacing newer guidance in the textarea. ## Display contract diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index f9d56936..87fb303e 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -227,6 +227,38 @@ test('keeps a committed draft applied when the authoritative reload fails', asyn expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('blocks stale authoring after a suggestion commits but its authoritative reload fails', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const reply = suggestions(); + reply.edits.push({ ...reply.edits[0]!, summary: 'Clarify retry intent', reason: 'The intent should name the visible limit.', + field: 'intent', value: 'Explain the operator-visible retry limit.' }); + await page.getByRole('button', { name: 'Suggest edits' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(reply)); + let reloads = 0; + await page.route('**/api/review', async route => { + reloads++; + if (reloads === 1) await route.fulfill({ status: 500, contentType: 'application/json', body: JSON.stringify({ error: 'Temporary review read failure' }) }); + else await route.continue(); + }); + await page.getByRole('button', { name: 'Apply this edit' }).first().click(); + + await expect(page.locator('#plans-status')).toContainText('Revision r2 was applied, but the current plan could not reload'); + await expect(page.getByRole('button', { name: 'Refresh suggestions', exact: true })).toHaveCount(0); + await expect(page.locator('#plan-draft')).toHaveAttribute('aria-disabled', 'true'); + await expect(page.locator('#plan-suggest')).toHaveAttribute('aria-disabled', 'true'); + await page.locator('#plan-suggest').evaluate((button: HTMLButtonElement) => button.click()); + expect(invocations).toHaveLength(1); + await expect(page.locator('#plans-refresh')).toBeFocused(); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); + + await page.locator('#plans-refresh').click(); + await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.getByRole('button', { name: 'Refresh suggestions', exact: true })).toBeVisible(); + await expect(page.locator('#plan-suggest')).not.toHaveAttribute('aria-disabled', 'true'); +}); + test('keeps Apply retryable when success names the wrong revision', async ({ page }) => { const { deps, invocations } = planning(); await openPlans(page, deps); @@ -271,6 +303,7 @@ test('replays an ambiguous suggestion Apply, stales siblings, and refreshes befo expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); await page.getByRole('button', { name: 'Retry Apply', exact: true }).click(); await expect(page.locator('#plans-revision')).toHaveText('r2'); + await expect(page.getByRole('button', { name: 'Refresh suggestions', exact: true })).toBeFocused(); expect(sent).toHaveLength(2); expect(sent[1]).toEqual(sent[0]); expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); diff --git a/test/planning-drafts-api.test.ts b/test/planning-drafts-api.test.ts index 7444365a..2597f580 100644 --- a/test/planning-drafts-api.test.ts +++ b/test/planning-drafts-api.test.ts @@ -252,11 +252,25 @@ it('does not record a storage error on a cancel as a refusal, so the same action const failing = vi.spyOn(Store.prototype, 'requestMode').mockImplementationOnce(() => { throw Object.assign(new Error('disk I/O error'), { code: 'ERR_SQLITE_ERROR' }); }); const cancel = { actionId: randomUUID() }; - expect((await served.api('POST', `/api/plan/drafts/${id}/cancel`, cancel)).body.error).toBe('disk I/O error'); + expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, cancel)) + .toEqual({ status: 503, body: { error: 'disk I/O error', outcomeUnknown: true } }); failing.mockRestore(); expect(await served.api('POST', `/api/plan/drafts/${id}/cancel`, cancel)).toEqual({ status: 200, body: { result: { state: 'cancelled' } } }); }); +it('reports an unrecorded Apply storage error as retryable and replays the same action ID', async () => { + const served = await serve(redraft), id = (await served.start('drafts')).body.result.requestId as string; + await served.settled('drafts', id); + const failing = vi.spyOn(Store.prototype, 'applyDraft').mockImplementationOnce(() => { + throw Object.assign(new Error('database is locked'), { code: 'ERR_SQLITE_ERROR' }); }); + const apply = { actionId: randomUUID() }; + expect(await served.api('POST', `/api/plan/drafts/${id}/apply`, apply)) + .toEqual({ status: 503, body: { error: 'database is locked', outcomeUnknown: true } }); + failing.mockRestore(); + expect(await served.api('POST', `/api/plan/drafts/${id}/apply`, apply)) + .toEqual({ status: 200, body: { result: { revision: served.view.plan.revision + 1 } } }); +}); + it('dismisses a ready draft, and cancels a pending one when the server shuts down', async () => { const dismissed = await serve(redraft), ready = (await dismissed.start('drafts')).body.result.requestId as string; await dismissed.settled('drafts', ready); diff --git a/web/public/app.js b/web/public/app.js index 0b3398ea..298fa60b 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -1070,6 +1070,7 @@ function planAuthorResultMarkup(request, historical = false) { if (!historical && result && request.requestId && dismissible) result += `
`; const remaining = request.mode === "suggest" && request.state === "consumed" && Number.isSafeInteger(request.appliedIndex) + && data?.plan?.revision === request.revision + 1 ? request.reply.edits.filter((_, index) => index !== request.appliedIndex) : []; if (!historical && remaining.length) result += '
! Plan changed — refresh suggestions
'; @@ -1080,9 +1081,10 @@ function renderPlanAuthor() { const restoreApplyFocus = document.activeElement?.id?.startsWith("plan-author-apply-") ? document.activeElement.id : null; const request = planAuthorRequest, unavailable = !data?.plan; const busyAuthor = planAuthorBusy(), uncertain = request?.state === "uncertain", applyUncertain = request?.state === "apply-uncertain"; + const appliedReloadPending = request?.state === "consumed" && data?.plan?.revision !== request.revision + 1; for (const [id, mode, label] of [["plan-draft", "draft", "Draft next revision"], ["plan-suggest", "suggest", "Suggest edits"]]) { const button = $(id); - const blocked = unavailable || busyAuthor || applyUncertain || (uncertain && request.mode !== mode); + const blocked = unavailable || busyAuthor || applyUncertain || appliedReloadPending || (uncertain && request.mode !== mode); if (blocked) button.setAttribute("aria-disabled", "true"); else button.removeAttribute("aria-disabled"); button.textContent = request?.state === "starting" && request.mode === mode ? "Starting…" @@ -1120,7 +1122,7 @@ function renderPlanAuthor() { applyPlanAuthor(button.dataset.planApply === "draft" ? null : Number(button.dataset.planApply)))); $("plan-author-refresh-suggestions")?.addEventListener("click", refreshPlanSuggestions); if (restoreDismissFocus) $("plan-author-dismiss")?.focus(); - else if (restoreApplyFocus) $(restoreApplyFocus)?.focus(); + else if (restoreApplyFocus) ($(restoreApplyFocus) ?? $("plan-author-refresh-suggestions") ?? $("plans-refresh"))?.focus(); } function renderPlans() { if (!data?.plan) { @@ -1285,7 +1287,8 @@ async function pollPlanAuthor(generation) { } } async function startPlanAuthor(mode, feedbackOverride = null) { - if (!data?.plan || planAuthorBusy() || planAuthorRequest?.state === "apply-uncertain") return; + if (!data?.plan || planAuthorBusy() || planAuthorRequest?.state === "apply-uncertain" || + (planAuthorRequest?.state === "consumed" && data.plan.revision !== planAuthorRequest.revision + 1)) return; if (planImportPending || planRefreshPending || busy) { $("plan-author-status").className = "warn"; $("plan-author-status").textContent = "! Wait for the current plan or review action before asking the agent."; diff --git a/web/server.ts b/web/server.ts index f844d054..44127bf6 100644 --- a/web/server.ts +++ b/web/server.ts @@ -646,6 +646,9 @@ export async function startServer(config: ReviewConfig, port = 4318, questionAge json(404, { error: 'Not found.' }); } catch (error) { if (error instanceof ShuttingDownError) { json(503, { error: error.message }); return; } + if ((error as { code?: string })?.code === 'ERR_SQLITE_ERROR') { + json(503, { error: error instanceof Error ? error.message : 'Storage failed.', outcomeUnknown: true }); return; + } if (error instanceof BadRequest) { json(400, { error: error.message }); return; } if (error instanceof UpstreamFailure) { json(502, { error: error.message }); return; } json(409, { error: error instanceof Error ? error.message : 'Review failed.' }); From a30fe1cf68bd6b4824b72acbd598c5cc596fe95b Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 14:06:02 -0700 Subject: [PATCH 3/7] Keep consumed suggestions blocked until refresh --- test/browser/plans-authoring.spec.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 87fb303e..12791749 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -397,14 +397,15 @@ test('rejects failed output and marks consumed output stale', async ({ page }) = const { deps } = planning(); await openPlans(page, deps); const snapshotId = app.service.load().snapshot.id; - let reads = 0; + let reads = 0, starts = 0; let releaseConsumed!: () => void; const consumedHeld = new Promise(resolve => { releaseConsumed = resolve; }); - await page.route('**/api/plan/suggestions', route => route.fulfill({ - status: 200, contentType: 'application/json', body: JSON.stringify({ + await page.route('**/api/plan/suggestions', route => { + starts++; + return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ result: { requestId: '12345678-1234-4123-8123-123456789abc' }, - }), - })); + }) }); + }); await page.route('**/api/plan/suggestions/*', async route => { reads++; if (reads > 1) await consumedHeld; @@ -424,7 +425,9 @@ test('rejects failed output and marks consumed output stale', async ({ page }) = await expect(page.getByRole('article', { name: 'Name the retry ceiling' })).toBeVisible(); await expect(page.getByRole('button', { name: 'Dismiss suggestions' })).toHaveCount(0); await expect(page.getByRole('button', { name: 'Apply this edit' })).toHaveCount(0); - await expect(suggest).not.toHaveAttribute('aria-disabled', 'true'); + await expect(suggest).toHaveAttribute('aria-disabled', 'true'); + await suggest.evaluate((button: HTMLButtonElement) => button.click()); + expect(starts).toBe(1); }); test('rejects a status response for the wrong planning operation', async ({ page }) => { From 662970d2b7ce23812e9d488b6cf4dd55fd97ba9a Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 14:13:13 -0700 Subject: [PATCH 4/7] Allow authoring after later plan revisions --- test/browser/plans-authoring.spec.ts | 12 ++++++++++++ web/public/app.js | 6 +++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 12791749..0878f806 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -257,6 +257,18 @@ test('blocks stale authoring after a suggestion commits but its authoritative re await expect(page.locator('#plans-revision')).toHaveText('r2'); await expect(page.getByRole('button', { name: 'Refresh suggestions', exact: true })).toBeVisible(); await expect(page.locator('#plan-suggest')).not.toHaveAttribute('aria-disabled', 'true'); + + const next = app.service.store.getPlan(app.service.config.identity); + next.revision++; + next.summary = 'A later authoritative revision'; + app.service.store.importRevision(JSON.stringify(next), 'json', app.service.planContextForAmendment(), 2); + await page.locator('#plans-refresh').click(); + await expect(page.locator('#plans-revision')).toHaveText('r3'); + await page.locator('#plan-suggest').click(); + await expect.poll(() => invocations.length).toBe(2); + expect(invocations[1]!.request).toMatchObject({ mode: 'suggest', revision: 3 }); + invocations[1]!.resolve(JSON.stringify(suggestions(3))); + await expect(page.getByRole('status').filter({ hasText: 'Suggestions ready for r3.' })).toBeVisible(); }); test('keeps Apply retryable when success names the wrong revision', async ({ page }) => { diff --git a/web/public/app.js b/web/public/app.js index 298fa60b..da25ab4e 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -1070,7 +1070,7 @@ function planAuthorResultMarkup(request, historical = false) { if (!historical && result && request.requestId && dismissible) result += `
`; const remaining = request.mode === "suggest" && request.state === "consumed" && Number.isSafeInteger(request.appliedIndex) - && data?.plan?.revision === request.revision + 1 + && data?.plan?.revision >= request.revision + 1 ? request.reply.edits.filter((_, index) => index !== request.appliedIndex) : []; if (!historical && remaining.length) result += '
! Plan changed — refresh suggestions
'; @@ -1081,7 +1081,7 @@ function renderPlanAuthor() { const restoreApplyFocus = document.activeElement?.id?.startsWith("plan-author-apply-") ? document.activeElement.id : null; const request = planAuthorRequest, unavailable = !data?.plan; const busyAuthor = planAuthorBusy(), uncertain = request?.state === "uncertain", applyUncertain = request?.state === "apply-uncertain"; - const appliedReloadPending = request?.state === "consumed" && data?.plan?.revision !== request.revision + 1; + const appliedReloadPending = request?.state === "consumed" && !(data?.plan?.revision >= request.revision + 1); for (const [id, mode, label] of [["plan-draft", "draft", "Draft next revision"], ["plan-suggest", "suggest", "Suggest edits"]]) { const button = $(id); const blocked = unavailable || busyAuthor || applyUncertain || appliedReloadPending || (uncertain && request.mode !== mode); @@ -1288,7 +1288,7 @@ async function pollPlanAuthor(generation) { } async function startPlanAuthor(mode, feedbackOverride = null) { if (!data?.plan || planAuthorBusy() || planAuthorRequest?.state === "apply-uncertain" || - (planAuthorRequest?.state === "consumed" && data.plan.revision !== planAuthorRequest.revision + 1)) return; + (planAuthorRequest?.state === "consumed" && data.plan.revision < planAuthorRequest.revision + 1)) return; if (planImportPending || planRefreshPending || busy) { $("plan-author-status").className = "warn"; $("plan-author-status").textContent = "! Wait for the current plan or review action before asking the agent."; From 784a0837e554952af269dbae7a01ac3f906d5842 Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 14:25:18 -0700 Subject: [PATCH 5/7] Accept later revisions after plan apply --- test/browser/plans-authoring.spec.ts | 23 +++++++++++++++++++++++ web/public/app.js | 4 ++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 0878f806..98fc4c8d 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -227,6 +227,29 @@ test('keeps a committed draft applied when the authoritative reload fails', asyn expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('accepts a later authoritative revision when reloading a committed Apply', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const current = app.service.store.getPlan(app.service.config.identity); + await page.getByRole('button', { name: 'Draft next revision' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(draft(current))); + await page.route('**/api/review', async route => { + const later = app.service.store.getPlan(app.service.config.identity); + expect(later.revision).toBe(2); + later.revision++; + later.summary = 'A concurrent authoritative revision'; + app.service.store.importRevision(JSON.stringify(later), 'json', app.service.planContextForAmendment(), 2); + await route.continue(); + }); + await page.getByRole('button', { name: 'Apply draft', exact: true }).click(); + + await expect(page.locator('#plans-revision')).toHaveText('r3'); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r3 is current.'); + await expect(page.locator('#plan-suggest')).not.toHaveAttribute('aria-disabled', 'true'); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(3); +}); + test('blocks stale authoring after a suggestion commits but its authoritative reload fails', async ({ page }) => { const { deps, invocations } = planning(); await openPlans(page, deps); diff --git a/web/public/app.js b/web/public/app.js index da25ab4e..650c43e4 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -1399,8 +1399,8 @@ async function applyPlanAuthor(index) { try { const updated = await api("/api/review"); if (generation !== planOperationGeneration || sharedGeneration !== reviewGeneration || planAuthorRequest !== request) return; - if (!updated?.plan || updated.plan.revision !== revision) - throw new Error(`The authoritative review did not return applied revision r${revision}.`); + if (!updated?.plan || updated.plan.revision < revision) + throw new Error(`The authoritative review did not include applied revision r${revision}.`); rememberDraft(); mergeGeneration++; if (mergePollTimer) clearTimeout(mergePollTimer); From 3462902a7109a598710033c449cbe78f1a3b85ff Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 14:37:55 -0700 Subject: [PATCH 6/7] Clarify plan apply architecture wording --- docs/architecture.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture.md b/docs/architecture.md index c68645a4..ab937d7e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -180,7 +180,7 @@ Dependencies point downward only. `core/` imports nothing that does input or out | Linking engine | `core/linking.ts` | Replays each commit's line changes and assigns every final line to an owner from the ledger. Produces segments and the Unplanned and Ambiguous rows. | In use | | Approvals | `core/approvals.ts` | Approval fingerprints, dependency staleness, assignments, and accept-as-is choices. | In use | | Issue ranking | `core/issue-ranking.ts`, `github/issues.ts`, `web/issues.ts` | Deterministic, explainable issue score. Repository-scoped trust of issue authors. | In use | -| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); Plans starts, displays and revision-bound applies authoring results | +| Planning library | `core/planning-author.ts`, `core/planning-suggestions.ts`, `prompts/plan-author.md` | Builds bounded planning prompts and validates plan drafts and suggestion cards. Does not run an agent itself; the planning agent above does. Recorded Claude output replays through it in tests (E4, #45). | In use through `/api/plan/*` (#117, #124); Plans starts and displays authoring requests, then applies results through revision-bound endpoints | | Execution prompt and audit | `core/execution-prompt.ts`, `core/run-audit.ts`, `prompts/execute.md` | Builds the execute and fix prompt with untrusted text only in escaped data blocks. Audits the change manifest after a run: safety violations first (Git metadata, links that leave the repository, agent commits), then scope against the declared files. | In use with the opt-in `runner` block | | Pull request text | `core/pull-request-body.ts` | Title and description with the plan. Plan and agent text go in fenced blocks, and issue references and @-mentions are neutralized, so the text cannot close other issues or notify people. | In use with the opt-in `runner` block | | Git adapter | `git/history.ts`, `git/clone.ts` | Bounded, hardened history reads. Standalone task clones with no links back to your repository. Every Git command runs with an allowlisted environment. | In use | From e3697dd532643de0f831b678856b2d677338231d Mon Sep 17 00:00:00 2001 From: mchwang Date: Thu, 8 Oct 2026 14:49:09 -0700 Subject: [PATCH 7/7] Explain blocked plan apply actions --- test/browser/plans-authoring.spec.ts | 29 ++++++++++++++++++++++++++++ web/public/app.js | 8 ++++++-- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/test/browser/plans-authoring.spec.ts b/test/browser/plans-authoring.spec.ts index 98fc4c8d..37c367f3 100644 --- a/test/browser/plans-authoring.spec.ts +++ b/test/browser/plans-authoring.spec.ts @@ -201,6 +201,35 @@ test('applies a draft once while preserving newer review text and its attachment expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(2); }); +test('explains why Apply waits for an in-flight plan refresh', async ({ page }) => { + const { deps, invocations } = planning(); + await openPlans(page, deps); + const current = app.service.store.getPlan(app.service.config.identity); + await page.getByRole('button', { name: 'Draft next revision' }).click(); + await expect.poll(() => invocations.length).toBe(1); + invocations[0]!.resolve(JSON.stringify(draft(current))); + const apply = page.getByRole('button', { name: 'Apply draft', exact: true }); + await expect(apply).toBeVisible(); + + let release!: () => void, arrived!: () => void; + const held = new Promise(resolve => { release = resolve; }); + const captured = new Promise(resolve => { arrived = resolve; }); + await page.route('**/api/review', async route => { arrived(); await held; await route.continue(); }); + let applies = 0; + await page.route('**/api/plan/drafts/*/apply', async route => { applies++; await route.continue(); }); + await page.locator('#plans-refresh').click(); + await captured; + await apply.click(); + + await expect(page.locator('#plan-author-status')).toHaveText('! Wait for the current plan or review action before applying this result.'); + await expect(apply).toBeFocused(); + expect(applies).toBe(0); + expect(app.service.store.getPlan(app.service.config.identity).revision).toBe(1); + release(); + await expect(page.locator('#plans-status')).toHaveText('✓ Revision r1 is current.'); + await expect(apply).toBeVisible(); +}); + test('keeps a committed draft applied when the authoritative reload fails', async ({ page }) => { const { deps, invocations } = planning(); await openPlans(page, deps); diff --git a/web/public/app.js b/web/public/app.js index 650c43e4..a79f8c54 100644 --- a/web/public/app.js +++ b/web/public/app.js @@ -1357,8 +1357,12 @@ async function applyPlanAuthor(index) { const request = planAuthorRequest; const retry = request?.state === "apply-uncertain" && request.applyingIndex === index; if (!request?.requestId || (!retry && (request.state !== "ready" || planAuthorStale(request))) || - (request.mode === "draft" ? index !== null : !Number.isSafeInteger(index) || !request.reply.edits[index]) || - planImportPending || planRefreshPending || busy) return; + (request.mode === "draft" ? index !== null : !Number.isSafeInteger(index) || !request.reply.edits[index])) return; + if (planImportPending || planRefreshPending || busy) { + $("plan-author-status").className = "warn"; + $("plan-author-status").textContent = "! Wait for the current plan or review action before applying this result."; + return; + } supersedePlanRefresh(); const generation = ++planOperationGeneration; const sharedGeneration = ++reviewGeneration;