diff --git a/.claude/rules/delivery-discipline.md b/.claude/rules/delivery-discipline.md new file mode 100644 index 00000000..2c0f3a9d --- /dev/null +++ b/.claude/rules/delivery-discipline.md @@ -0,0 +1,49 @@ +# Delivery discipline — Definition of Ready / Definition of Done + +No `paths` frontmatter — applies to every goal, every language. Terms +are Scrum's (the 2020 Scrum Guide formalized DoD as an artifact +commitment) and Kanban's (Kanban University's own "ready for delivery" +pull criteria) — adopted as the converged NAMES for gates this repo +already runs, consolidated here from CLAUDE.md/testing.md/ADR-0034 so +an agent checks ONE list, not four documents (researched 2026-08-12; +owner-mandated: "DoR should be industry standard"). + +## Definition of Ready — before a BACKLOG.md item enters a session +- Precedent checked, not assumed (CLAUDE.md Research→Plan→Implement). +- A capability map exists for any schema/adopt-vs-build call with more + than one real future use (CLAUDE.md Plan step, SPEC §3.3's worked + example). +- A goal file exists: Goal/Plan/**Acceptance stated as a checkable + predicate**, not a vibe. +- No SPEC.md `OPEN` dependency is silently resolved by starting — + surface the choice, or name it an explicit blocker and don't start. +- No estimate/story-point step — deliberately excluded: estimation + negotiates shared *team* capacity, which a solo-owner + + agent-execution loop doesn't have. + +## Definition of Done — before the checkbox flips and the file moves to archive/ +- Local lefthook suite green, never bypassed. +- CI's `ci-gate` required check green on the **merged** PR (ADR-0034). +- Every new capability carries a seeded example + a proof at the right + layer (testing.md's layering) — the seed is part of DoD, never a + follow-up. +- Any bug fixed via live/manual repro is now a committed test + (testing.md). +- SPEC.md updated in the same change, for anything that shifts what it + describes (mechanical-only changes exempted). +- The goal file's own Acceptance criteria checked against what + SHIPPED, not what was planned. +- Nothing secret-shaped staged; a real commit message. + +## Build-health visibility +The ruleset (ADR-0034) already makes main unmergeable-red; confirm, +don't re-enforce: +- `gh pr checks --watch` right after opening a goal's PR. +- `gh run list -b main -L 1` once, when picking up the next goal. +No richer signal is worth building — a webhook receiver is a second +deployable, already forbidden (SPEC §1.1). + +## Tech debt +A BACKLOG.md entry with the same DoR/DoD as any goal — never a second +register, never a bare TODO as the record (a comment may point at a +goal/ADR id, never stand alone). diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 24851607..0e3d9e3a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,7 +15,14 @@ permissions: concurrency: group: ci-${{ github.workflow }}-${{ github.ref }} - cancel-in-progress: true + # Conditional, per GitHub's own docs pattern: a superseded PR run + # SHOULD die (per-PR synthetic ref), but main's post-merge runs must + # each complete — with `true` here, two merges landing close together + # cancelled the earlier merge commit's own verification run, leaving + # that main SHA's CI status permanently incomplete (observed live + # 2026-08-12: merges #1/#10 showed cancelled; the mirror image of + # ADR-0034's bisect-blind-spot concern). + cancel-in-progress: ${{ github.ref != 'refs/heads/main' }} jobs: # goal 0024 / ADR-0034's un-deferred path-filtering: skip the heavy @@ -269,6 +276,19 @@ jobs: runs-on: ubuntu-latest timeout-minutes: 15 strategy: + # fail-fast: false -- real incident, not a hypothetical (PR 11, + # run 31557343422, 2026-08-12): shard 3 failed on a real e2e + # regression, and GitHub Actions' own fail-fast DEFAULT (true when + # unset) immediately cancelled shards 1 and 2 mid-run rather than + # letting them finish -- both were killed ~2.5 minutes into a + # passing-so-far run, so their own genuine pass/fail signal was + # simply never collected ("shard-2-cancelled-while-green": it + # wasn't red, it never got the chance to report). A 3-way sharded + # suite needs every shard's own verdict to know what's actually + # broken, not just "at least one shard is red" -- the same + # reasoning build-go's own 2-platform matrix comment already + # states for its own fail-fast: false, applied here too. + fail-fast: false matrix: shardIndex: [1, 2, 3] shardTotal: [3] diff --git a/docs/SPEC.md b/docs/SPEC.md index 0389e2dc..8e9418ac 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -2375,6 +2375,44 @@ turns out to solve this without touching that dispute). all Go-tested; a Review-queue row appearing and its Approve executing the write is e2e-tested (`mcp-write-approval.spec.ts`). +**A fourth verb — `cancel_write` — and a requester-liveness heartbeat +are now built (docs/goals/archive/0026-request-lifecycle-honesty.md), +completing park/poll/resolve/**WITHDRAW**.** `cancel_write {id}` +(`mcpsvc`, ungated — cancelling your own request only ever reduces +pending work) lets the requesting client withdraw its own still-pending +write; `cancelled` is a distinct `MCPWriteStatus`, never conflated with +`denied`, sharing `ResolveMCPWrite`'s at-most-once locking discipline. +`check_write_status` now also records `LastPolledAt` on every real poll +— the requester's own heartbeat — surfaced to Review as a muted +"requester last checked Nm ago" hint, shown only past a 5-minute +staleness bar (never on fresh polling, no noise). **A real BUG found +live in the same pass**: `ResolveMCPWrite` and the lazy expiry sweep +never fired the `mcp-write-approval` pending-count signal at all — a +resolved write's Activity/Review traces were correct, but the sidebar +badge and any poll-less surface (the banner, the floating prompt) could +hold a phantom pending count against an already-empty queue. Root +cause, once traced: `main.go`'s `application.RegisterEvent[MCPWriteRequest] +("mcp-write-approval")` binds that event name to an exact Go type +(Wails3's own documented behavior — "data types are matched exactly and +no conversion is performed") — emitting a bare `struct{}{}` silently +failed that check and the event was dropped before ever reaching a +browser client. Fixed by emitting a zero-value `MCPWriteRequest` +instead; every resolution path (approve/deny/cancel/expiry) now pings +it unconditionally. **Resolved MCP writes are now durable in Review's +own Recently-resolved section** (`MCPWriteResolved`/`ResolvedMCPWrites`, +merged newest-first alongside resolved runs, distinct `PlugIcon` +identity, not clickable) — previously the only trace of a +denied/expired write was session-only Activity, gone on restart, even +though the 24h outcome record was already persisted. **Activity's own +MCP-write rows are no longer action-dead**: expandable (the existing +`canExpand`/`result` mechanism) with a jump-to-target-workflow +`WorkflowHoverPreview` icon when the gated tool named an existing +workflow (`update_workflow`/`publish_workflow`/`delete_workflow`'s own +`id` argument — empty for `import_*` tools, which mint a new entity). +Proven end-to-end against a real MCP client, including the exact +phantom-badge regression (deny from Review, assert the sidebar badge +clears with no other page event) — `mcp-write-cancel.spec.ts`. + ### 3.7 Global app settings `SettingsService` (`settingsservice.go`) owns Mill's global settings @@ -2640,6 +2678,28 @@ the integration-http/decision-outcome transport tail) + HTTPRequest, independent of the presence gate — the layer that reaches the owner with no local Mac to notify on at all. +**Staleness presentation — `LOCKED` and built +(docs/goals/archive/0026-request-lifecycle-honesty.md), applying the §1 +thesis to time-honesty of a pending ask.** Owner-observed 2026-08-11: a +4-hour-old test write sat in Review looking exactly as urgent as a +fresh one ("feels like I missed something"). Age-tiered, not binary: +fresh (<15 minutes) renders as-is; older gets visible emphasis (an +attention-colored age badge) plus an "expires in Nh" caption counting +down the shared 24h clock (`frontend/src/shared/staleness.ts`'s +`ageTier`/`formatExpiresIn`, one implementation reused by ReviewView's +pending rows — both guardrail/human-review/debug parks and MCP write +requests — the `MCPWriteApprovals` banner, and the floating approval +prompt, `shared/StalenessBadge.tsx`). **No auto-dismiss** — the VS Code +severity-rule precedent (§9.1-adjacent) holds; expiry is the only +terminal timer, staleness is presentation only. The same age-tier +mechanism, at its own 5-minute bar, now also flags a **stuck-ENQUEUED +run** (a real zombie run — queued forever, never dequeued — found in +production data) in `WorkflowRunsPanel` and Activity's runs explorer, +alongside the Stop/`CancelRun` affordance DBOS's own `CancelWorkflow` +already supports for that status ("cancels a running or enqueued +workflow," confirmed against a real constructed ENQUEUED run, not just +trusted from the doc comment — `TestListRuns_EnqueuedRun_PresentationFieldsAndCancelPath`). + **Still `OPEN`, real named gaps:** a menu-bar/dock presence toggle (see above); appearance settings beyond light/dark; a default working directory/scope (blocked on §6); fullscreen window-state tracking diff --git a/docs/adr/0034-git-ci-operating-model.md b/docs/adr/0034-git-ci-operating-model.md index 177db524..52a13a56 100644 --- a/docs/adr/0034-git-ci-operating-model.md +++ b/docs/adr/0034-git-ci-operating-model.md @@ -164,3 +164,10 @@ Both are resolved now, the second one reversed. build now that the mechanism is understood and the felt cost (a 15-minute-plus e2e matrix run on a pure `docs/goals/*.md` edit) is real. +- **`e2e` job's shard matrix gained `fail-fast: false`** (goal 0026), + after a real incident (PR 11, run 31557343422): shard 3 failed + legitimately, and GitHub Actions' own fail-fast default immediately + cancelled shards 1 and 2 mid-run rather than letting them report + their own verdicts — signal lost, not just noise, the same class of + problem `build-go`'s own 2-platform matrix already carried this flag + for. diff --git a/docs/goals/BACKLOG.md b/docs/goals/BACKLOG.md index bac965fa..0e8007bf 100644 --- a/docs/goals/BACKLOG.md +++ b/docs/goals/BACKLOG.md @@ -44,10 +44,19 @@ this pipeline and on this code)** backend-side `isAway`), alert-style authorization request (notify.Start), cross-device forward (`composition.SendJSONWebhook`, `ForwardPendingApproval`) — see ADR-0032's Update note -4. [ ] [0026 — Request lifecycle honesty](0026-request-lifecycle-honesty.md) - — withdrawal verb (`cancel_write`, the MCP Tasks `tasks/cancel` - precedent ADR-0032 already mirrors) + staleness/expiry presentation - (owner-observed 2026-08-11: a 4h-old stale ask reads as breakage) +4. [x] [0026 — Request lifecycle honesty](archive/0026-request-lifecycle-honesty.md) + — delivered 2026-08-12: `cancel_write` MCP tool (a distinct + outcome from denied, ungated, at-most-once); age-tiered staleness + presentation (Review/banner/floating prompt) + "expires in Nh"; + requester-liveness hint (`lastPolledAt`, >5m-stale gate); the + phantom-badge BUG fixed (every resolution path — approve/deny/ + cancel/expiry — now pings the pending-changed signal, found live: + an empty-struct payload silently failed Wails3's own registered- + event type check); resolved MCP writes now durable in Review's + Recently-resolved; Activity MCP-write rows are expandable with a + jump-to-workflow preview; stuck-ENQUEUED runs get age emphasis + + Stop in WorkflowRunsPanel/Activity's runs explorer. Item 4 + (session-side hygiene) intentionally not a Mill code change. **Ratified 2026-08-10 (owner): three groups, A→B→C. 0001 stays standing live-review material, interleaved during owner reviews, not a lane.** diff --git a/docs/goals/archive/0026-request-lifecycle-honesty.md b/docs/goals/archive/0026-request-lifecycle-honesty.md new file mode 100644 index 00000000..f95afc55 --- /dev/null +++ b/docs/goals/archive/0026-request-lifecycle-honesty.md @@ -0,0 +1,56 @@ +# 0026 — Request lifecycle honesty: withdrawal + staleness + +## Goal +A pending decision never lies about its relevance (owner-observed +2026-08-11: a 4-hour-old test write sat in Review looking as urgent as +a fresh ask — "feels like I missed something / it's not working"). +An actionable item must carry its freshness, and the requester must be +able to take back a request that stopped mattering. + +## Plan +1. [x] **`cancel_write` MCP tool** (the missing fourth verb — + park/poll/resolve/WITHDRAW; `tasks/cancel` in the MCP Tasks spec is + the direct precedent ADR-0032 already mirrors): the requesting + client cancels its own still-pending write by id; cancelled ≠ + denied (a distinct outcome, recorded in Activity like + denied/expired — never traceless). Ungated (cancelling your own + request needs no human approval; it only ever REDUCES pending + work). At-most-once semantics shared with resolve. +2. [x] **Staleness presentation** in Review + banner + floating + prompt: age-tiered treatment (fresh <15m renders as-is; older gets + a visible age emphasis + "expires in Nh" from the 24h clock) — the + §1 thesis applied to time-honesty of asks. No auto-dismiss of + actionable items (the VS Code severity rule holds; expiry is the + only terminal timer). +3. [x] **Requester liveness hint** (design question, research first): + should a pending write surface "requester last polled Nm ago" — + check_write_status calls are the natural heartbeat — so an + abandoned request is visibly abandoned? Cheap to record; decide + presentation against the no-noise bar. +4. [ ] Session-side hygiene rule for THIS workflow (rides the memory, + not Mill): a test write parked for demonstration gets cancelled by + its requester when the demonstration ends. (Session-side, not a + Mill code change — outside this delivery's own scope.) +5. [x] **Badge staleness on write resolution (BUG, diagnosed live + 2026-08-11)**: ResolveMCPWrite (and the expiry sweep) never emits on + the pending-changed channel, so the sidebar badge held a phantom 1 + against an empty queue — resolution paths must ping the same ONE + signal parks do (goal 0005's model, missing emit). +6. [x] **Resolved writes appear in Review's Recently-resolved** — today + a denied/approved/cancelled write vanishes from Review entirely + (only trace: session-only Activity, gone on restart). The queue's + own history must include write resolutions, durably (the persisted + 24h outcome records already exist — surface them). +7. [x] **Activity MCP-write rows get actions**: expandable detail + + jump-to-the-target-workflow (runs already drill down; writes are + action-dead — owner: "so what I can do and nothing I can do"). +8. [x] **Stuck-ENQUEUED runs surface honestly** (a run enqueued-forever + reads as live; found: a zombie ENQUEUED run from a morning error) — + age-visible like item 2, plus a Stop affordance where cancel is + legal. + +## Acceptance +A parked-then-obsolete request can be withdrawn by its requester and +shows as cancelled in Activity; a lingering pending item visibly +communicates its age and time-to-expiry; the owner never again reads +staleness as breakage. diff --git a/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/index.ts b/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/index.ts index 78d1cff6..86b42909 100644 --- a/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/index.ts +++ b/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/index.ts @@ -4,5 +4,6 @@ export type { DataChanged, MCPWriteActivity, - MCPWriteRequest + MCPWriteRequest, + MCPWriteResolved } from "./models.js"; diff --git a/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/models.ts b/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/models.ts index 025a7de7..d26df445 100644 --- a/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/models.ts +++ b/frontend/bindings/github.com/alicoding/mill/internal/services/mcpsvc/models.ts @@ -13,21 +13,37 @@ export interface DataChanged { } /** - * MCPWriteActivity is pushed for a missed (expired) or denied MCP - * write so it's no longer traceless (docs/goals/0005-pending-attention- - * model.md item 3). Reuses the same activity-push shape App.tsx's - * hotkey-activity handler already established, under a distinct - * "mcp-write" ActivitySource so it's filterable, not conflated with a - * workflow trigger. + * MCPWriteActivity is pushed for a resolved (denied/cancelled/expired, + * or approved-but-failed) MCP write so it's no longer traceless + * (docs/goals/0005-pending-attention-model.md item 3). Reuses the same + * activity-push shape App.tsx's hotkey-activity handler already + * established, under a distinct "mcp-write" ActivitySource so it's + * filterable, not conflated with a workflow trigger. */ export interface MCPWriteActivity { "description": string; /** - * Outcome is "denied" or "expired" -- an approved write never - * reaches here, there's nothing traceless about it. + * Outcome is "denied", "cancelled", or "expired" -- an + * approved-and-succeeded write never reaches here, there's nothing + * traceless about it. */ "outcome": string; + + /** + * ToolName/WorkflowID/Result back Activity's own MCP-write row + * actions (docs/goals/0026 item 7: "so what I can do and nothing I + * can do") -- ToolName is the gated tool this record was for; + * WorkflowID is the workflow it targeted, when the tool names one + * (update_workflow/publish_workflow/delete_workflow's own "id" + * argument -- empty for import_* tools, which mint a NEW entity + * rather than referencing an existing one, so there's nothing to + * jump to); Result is what the Activity row's expandable detail + * panel shows. + */ + "toolName"?: string; + "workflowID"?: string; + "result"?: string; } /** @@ -41,4 +57,33 @@ export interface MCPWriteRequest { "id": string; "description": string; "createdAt": string; + + /** + * LastPolledAt mirrors MCPWriteRecord's own field (docs/goals/0026 + * item 3) -- nil when the requester has never called + * check_write_status on this id yet. + */ + "lastPolledAt"?: string | null; +} + +/** + * MCPWriteResolved is the frontend-facing shape for an already-resolved + * write (docs/goals/0026 item 6) -- Review's Recently-resolved section + * reads this alongside RunSummary's own resolved rows, merged + * newest-first. Retained for the same 24h window check_write_status + * already promises (sweepLocked's own retention) -- "durable across a + * restart" and "still visible for the same window an MCP client can + * still poll" are the same guarantee, not two. + */ +export interface MCPWriteResolved { + "id": string; + "description": string; + + /** + * approved / denied / cancelled / expired + */ + "status": string; + "error"?: string; + "createdAt": string; + "resolvedAt": string; } diff --git a/frontend/bindings/github.com/alicoding/mill/internal/services/settingssvc/settingsservice.ts b/frontend/bindings/github.com/alicoding/mill/internal/services/settingssvc/settingsservice.ts index 920f3f79..97195e51 100644 --- a/frontend/bindings/github.com/alicoding/mill/internal/services/settingssvc/settingsservice.ts +++ b/frontend/bindings/github.com/alicoding/mill/internal/services/settingssvc/settingsservice.ts @@ -64,6 +64,19 @@ export function ClearKeybinding(commandID: string): $CancellablePromise { return $Call.ByID(3036020909, commandID); } +/** + * DebugBackdatePendingMCPWrite is an e2e-only test knob (docs/goals/0026 + * item 2's staleness presentation) -- see MillMCPService. + * DebugBackdatePendingWrite's own doc comment for why this has to be an + * in-process call rather than an external settings-file edit. Refuses + * outside isolated test data (the same IsIsolatedData signal every + * e2e run already sets via MILL_SETTINGS_PATH) -- never reachable + * against a real production instance. + */ +export function DebugBackdatePendingMCPWrite(id: string, ageMinutes: number): $CancellablePromise { + return $Call.ByID(561097732, id, ageMinutes); +} + /** * DismissApprovalPrompt hides the floating approval prompt and applies * the same focus-yield mitigation DismissPanel already uses -- called @@ -283,6 +296,15 @@ export function ResolveMCPWrite(id: string, approve: boolean): $CancellablePromi return $Call.ByID(2304068895, id, approve); } +/** + * ResolvedMCPWrites lists every already-resolved MCP write still in its + * 24h retention window (docs/goals/0026 item 6) -- Review's + * Recently-resolved section reads this alongside its own resolved runs. + */ +export function ResolvedMCPWrites(): $CancellablePromise { + return $Call.ByID(3844643736); +} + /** * RestoreMenuAccelerators reverses SuspendMenuAccelerators -- see its * doc comment for the full reasoning. Safe to call more times than diff --git a/frontend/e2e/mcp-write-cancel.spec.ts b/frontend/e2e/mcp-write-cancel.spec.ts new file mode 100644 index 00000000..689b3ab5 --- /dev/null +++ b/frontend/e2e/mcp-write-cancel.spec.ts @@ -0,0 +1,195 @@ +import type { Page } from '@playwright/test' +import { test, expect } from './fixtures/server' +import { clickRowAction } from './inventoryRow' +import { + connectMCPClient, exportWorkflowViaMCP, findWorkflowIdByLabel, + enableMCPWritesWithApprovalRequired, restoreMCPWriteDefaults, +} from './mcpTestClient' + +// docs/goals/0026 items 1 and 5: the requester's own withdrawal of a +// pending MCP write (cancel_write, a distinct outcome from denied), and +// the badge-staleness bug fix (every resolution path -- approve, deny, +// cancel, expiry -- must ping the pending-changed signal, not just a +// new park). The parked write's own id is read off the Review row's +// `data-mcp-write-id` attribute (ReviewView.tsx) rather than awaited +// from the original import_workflow call's own eventual result text -- +// that call only resolves once the production 10s courtesy window +// elapses (or a decision lands first), which would make id-extraction +// itself race the window; the row appears immediately on park, +// independent of the courtesy window. + +function workflowRow(page: Page, label: string) { + return page.locator('[data-testid="inventory-row"][data-entity="workflow"]', { has: page.getByText(label, { exact: true }) }) +} + +async function createSourceWorkflow(page: Page, label: string) { + await page.getByRole('link', { name: 'Workflows' }).click() + await page.getByTestId('new-workflow').click() + await page.locator('[role="tabpanel"]:not([hidden])').last().getByLabel('Label').fill(label) + await page.locator('[role="tabpanel"]:not([hidden])').last().getByTestId('save-workflow').click() + await expect(workflowRow(page, label)).toBeVisible() +} + +async function cleanupWorkflow(page: Page, label: string) { + const row = workflowRow(page, label) + if (await row.count() > 0) { + await clickRowAction(page, row.first(), 'Delete') + await expect(workflowRow(page, label)).toHaveCount(0) + } +} + +// The full withdraw lifecycle over a real MCP client: park -> cancel -> +// status cancelled -> nothing written -> the Review row and the sidebar +// badge both drop, driven purely by the event cancel_write now fires +// (never a manual page action). +test('cancel_write withdraws a pending MCP write: parked -> cancelled -> nothing written -> banner/queue count drops', async ({ page }, testInfo) => { + const label = 'E2E MCP write cancel source' + await enableMCPWritesWithApprovalRequired(page) + await createSourceWorkflow(page, label) + + const client = await connectMCPClient(testInfo.parallelIndex) + try { + const sourceId = await findWorkflowIdByLabel(client, label) + const exported = await exportWorkflowViaMCP(client, sourceId) + + // Fire the gated import in the background -- with approval required + // this parks rather than resolving immediately; the promise itself + // isn't awaited until after cancel_write below, since it only + // settles once the courtesy window elapses or a decision lands. + const importResultPromise = client.callTool({ name: 'import_workflow', arguments: { json: exported } }) + + await page.getByRole('link', { name: 'Review' }).click() + const item = page.getByTestId('review-mcp-write-item').first() + await expect(item).toBeVisible({ timeout: 15_000 }) + await expect(item).toContainText('import a workflow') + // The sidebar badge counts this pending write too (docs/goals/0026 + // item 5's own signal, shared with the guardrail badge test in + // guardrail.spec.ts). + await expect(page.getByTestId('review-pending-count')).toHaveText('1', { timeout: 10_000 }) + + const id = await item.getAttribute('data-mcp-write-id') + expect(id).toBeTruthy() + + const cancelRes = await client.callTool({ name: 'cancel_write', arguments: { id } }) + expect(cancelRes.isError, `cancel_write failed: ${JSON.stringify(cancelRes.content)}`).toBeFalsy() + + // Review row disappears, and the badge (the same phantom-1 bug this + // item exists to fix) drops back to zero -- purely event-driven, + // no manual reload/navigation in between. + await expect(page.getByTestId('review-mcp-write-item')).toHaveCount(0, { timeout: 10_000 }) + await expect(page.getByTestId('review-pending-count')).toHaveCount(0, { timeout: 10_000 }) + + // Nothing was written: only the one original workflow exists. + await page.getByRole('link', { name: 'Workflows' }).click() + await expect(workflowRow(page, label)).toHaveCount(1, { timeout: 10_000 }) + + // status is "cancelled", a distinct outcome from "denied". + const statusRes = await client.callTool({ name: 'check_write_status', arguments: { id } }) + const statusText = (statusRes.content?.[0] as { type: string; text?: string } | undefined)?.text ?? '' + expect(JSON.parse(statusText).status).toBe('cancelled') + + // The original import call itself must eventually settle as an + // error (it never got the write it asked for) -- awaited last since + // it may still be inside its own courtesy window. + const finalImport = await importResultPromise + expect(finalImport.isError, 'a cancelled import must return an error result to the original caller').toBeTruthy() + } finally { + await client.close() + await cleanupWorkflow(page, label) + await restoreMCPWriteDefaults(page) + } +}) + +// The phantom-1 repro (docs/goals/0026 item 5's own bug report): before +// this fix, ResolveMCPWrite never fired the pending-changed signal at +// all, so denying a parked write from Review left the sidebar badge +// stuck on a phantom count against an already-empty queue. Denying (not +// cancelling) is the exact regression case. +test('badge count drops on deny without any other event (phantom-1 repro)', async ({ page }, testInfo) => { + const label = 'E2E MCP write deny badge source' + await enableMCPWritesWithApprovalRequired(page) + await createSourceWorkflow(page, label) + + const client = await connectMCPClient(testInfo.parallelIndex) + try { + const sourceId = await findWorkflowIdByLabel(client, label) + const exported = await exportWorkflowViaMCP(client, sourceId) + const importResultPromise = client.callTool({ name: 'import_workflow', arguments: { json: exported } }) + + await page.getByRole('link', { name: 'Review' }).click() + const item = page.getByTestId('review-mcp-write-item').first() + await expect(item).toBeVisible({ timeout: 15_000 }) + await expect(page.getByTestId('review-pending-count')).toHaveText('1', { timeout: 10_000 }) + + await item.getByTestId('review-mcp-write-deny').click() + await expect(page.getByTestId('review-mcp-write-item')).toHaveCount(0, { timeout: 10_000 }) + // The regression this test locks in: the badge must clear on its + // own, driven only by the deny's own emitted event -- no page + // reload, no navigation away and back. + await expect(page.getByTestId('review-pending-count')).toHaveCount(0, { timeout: 10_000 }) + + // docs/goals/0026 item 6: the denied write now surfaces in Review's + // own Recently-resolved section (the durable 24h outcome record), + // not just session-only Activity -- distinct PlugIcon identity, not + // clickable like a run row. + const resolved = page.getByTestId('review-resolved-mcp-write-item').first() + await expect(resolved).toBeVisible({ timeout: 10_000 }) + await expect(resolved.getByTestId('review-resolved-mcp-write-status')).toHaveText('denied') + + const out = await importResultPromise + expect(out.isError).toBeTruthy() + } finally { + await client.close() + await cleanupWorkflow(page, label) + await restoreMCPWriteDefaults(page) + } +}) + +// docs/goals/0026 item 7: an MCP-write Activity row is no longer +// action-dead -- it's expandable (the existing canExpand/result +// mechanism, ActivityView.tsx) and, when the gated tool named an +// existing workflow (update_workflow does; import_workflow doesn't -- +// it mints a new one), carries a jump-to-workflow hover preview, the +// same WorkflowHoverPreview icon a run row already gets. +test('a denied MCP write appears in Activity, expandable, with a jump-to-workflow preview', async ({ page }, testInfo) => { + const label = 'E2E MCP write activity source' + await enableMCPWritesWithApprovalRequired(page) + await createSourceWorkflow(page, label) + + const client = await connectMCPClient(testInfo.parallelIndex) + try { + const sourceId = await findWorkflowIdByLabel(client, label) + const exported = await exportWorkflowViaMCP(client, sourceId) + // update_workflow (unlike import_workflow) names an EXISTING target + // workflow in its own args -- the case this affordance exists for. + const updateResultPromise = client.callTool({ name: 'update_workflow', arguments: { id: sourceId, json: exported } }) + + await page.getByRole('link', { name: 'Review' }).click() + const item = page.getByTestId('review-mcp-write-item').first() + await expect(item).toBeVisible({ timeout: 15_000 }) + await item.getByTestId('review-mcp-write-deny').click() + await expect(page.getByTestId('review-mcp-write-item')).toHaveCount(0, { timeout: 10_000 }) + + await page.getByRole('link', { name: 'Activity' }).click() + const row = page.getByTestId('activity-row').filter({ hasText: 'UPDATE workflow' }).first() + await expect(row).toBeVisible({ timeout: 10_000 }) + + // Jump-to-workflow: the hover-preview anchor only renders when + // entry.workflowID is set (WorkflowHoverPreview, same mechanism a + // run row already uses) -- update_workflow named sourceId, so it's + // present here. + await expect(row.getByTestId('workflow-hover-anchor')).toBeVisible() + + // Expandable: clicking the row opens its detail panel with a real + // result (the denial reason), the existing canExpand mechanism. + await row.click() + await expect(page.getByTestId('activity-detail')).toBeVisible({ timeout: 5_000 }) + + const out = await updateResultPromise + expect(out.isError).toBeTruthy() + } finally { + await client.close() + await cleanupWorkflow(page, label) + await restoreMCPWriteDefaults(page) + } +}) diff --git a/frontend/e2e/mcp-write-staleness.spec.ts b/frontend/e2e/mcp-write-staleness.spec.ts new file mode 100644 index 00000000..cd5bffb0 --- /dev/null +++ b/frontend/e2e/mcp-write-staleness.spec.ts @@ -0,0 +1,93 @@ +import type { Page } from '@playwright/test' +import { test, expect } from './fixtures/server' +import { clickRowAction } from './inventoryRow' +import { + backdatePendingMCPWrite, connectMCPClient, exportWorkflowViaMCP, findWorkflowIdByLabel, + enableMCPWritesWithApprovalRequired, restoreMCPWriteDefaults, +} from './mcpTestClient' + +// docs/goals/0026 item 2: a stale pending item must visibly communicate +// its age -- fresh (<15m) renders as-is, older gets emphasis + "expires +// in Nh". Exercised on a real parked MCP write, backdated via +// SettingsService.DebugBackdatePendingMCPWrite (an e2e-only knob, +// millmcpservice_approval_query.go's own doc comment explains why an +// external settings-file edit can't do this against a LIVE server) +// rather than sleeping 20 real minutes. + +function workflowRow(page: Page, label: string) { + return page.locator('[data-testid="inventory-row"][data-entity="workflow"]', { has: page.getByText(label, { exact: true }) }) +} + +test('a backdated pending MCP write renders age emphasis + expiry text in Review, the banner, and the approval prompt', async ({ page }, testInfo) => { + const label = 'E2E MCP write staleness source' + await enableMCPWritesWithApprovalRequired(page) + + await page.getByRole('link', { name: 'Workflows' }).click() + await page.getByTestId('new-workflow').click() + await page.locator('[role="tabpanel"]:not([hidden])').last().getByLabel('Label').fill(label) + await page.locator('[role="tabpanel"]:not([hidden])').last().getByTestId('save-workflow').click() + await expect(workflowRow(page, label)).toBeVisible() + + const client = await connectMCPClient(testInfo.parallelIndex) + try { + const sourceId = await findWorkflowIdByLabel(client, label) + const exported = await exportWorkflowViaMCP(client, sourceId) + const importResultPromise = client.callTool({ name: 'import_workflow', arguments: { json: exported } }) + + await page.getByRole('link', { name: 'Review' }).click() + const item = page.getByTestId('review-mcp-write-item').first() + await expect(item).toBeVisible({ timeout: 15_000 }) + + // Fresh: no age emphasis yet, just the plain relative-time render + // (StalenessBadge's own 'fresh' branch -- no data-age-tier attribute). + await expect(item.getByTestId('review-mcp-write-age')).not.toHaveAttribute('data-age-tier', 'aging') + + const id = await item.getAttribute('data-mcp-write-id') + expect(id).toBeTruthy() + + // Backdate 20 minutes -- past item 2's 15-minute bar, well short of + // the 24h expiry (renders "expires in 23h"). + await backdatePendingMCPWrite(page, id as string, 20) + + // Review's own row: age emphasis + "expires in Nh". + await expect(item.getByTestId('review-mcp-write-age')).toHaveAttribute('data-age-tier', 'aging', { timeout: 10_000 }) + await expect(item).toContainText(/expires in \d+h/) + + // MCPWriteApprovals' banner (App.tsx's global chrome, visible on + // every page) shows the same emphasis for the same write. + const bannerAge = page.getByTestId('mcp-write-approval-age') + await expect(bannerAge).toHaveAttribute('data-age-tier', 'aging', { timeout: 10_000 }) + + // The floating approval prompt's own route (ADR-0033's mechanism, + // headlessly reachable per approval-prompt.spec.ts's own precedent) + // shows it too. A fresh cross-document navigation (not a + // same-document hash change from the already-loaded page) is + // required to actually mount ApprovalPromptApp's own tree -- + // approval-prompt.spec.ts's own precedent/comment for this. + await page.goto('about:blank') + await page.goto('/#/approvalprompt') + const promptAge = page.getByTestId('approval-prompt-age') + await expect(promptAge).toHaveAttribute('data-age-tier', 'aging', { timeout: 10_000 }) + + // No auto-dismiss (docs/goals/0026 item 2's own "no auto-dismiss" + // rule): the item is still there, still actionable, staleness is + // presentation only. + await expect(page.getByTestId('approval-prompt-description')).toBeVisible() + + await page.goto('/') + await page.getByRole('link', { name: 'Review' }).click() + await page.getByTestId('review-mcp-write-item').first().getByTestId('review-mcp-write-deny').click() + await expect(page.getByTestId('review-mcp-write-item')).toHaveCount(0, { timeout: 10_000 }) + + const out = await importResultPromise + expect(out.isError).toBeTruthy() + } finally { + await client.close() + const row = workflowRow(page, label) + if (await row.count() > 0) { + await clickRowAction(page, row.first(), 'Delete') + await expect(workflowRow(page, label)).toHaveCount(0) + } + await restoreMCPWriteDefaults(page) + } +}) diff --git a/frontend/e2e/mcpTestClient.ts b/frontend/e2e/mcpTestClient.ts index f62e427c..b8a22f29 100644 --- a/frontend/e2e/mcpTestClient.ts +++ b/frontend/e2e/mcpTestClient.ts @@ -11,6 +11,37 @@ import { MCP_BASE_PORT, expect } from './fixtures/server' // own testMatch glob, same "plain helper module" shape as // inventoryRow.ts. +// backdatePendingMCPWrite drives the e2e-only debug knob +// (SettingsService.DebugBackdatePendingMCPWrite, docs/goals/0026 item 2) +// directly over Wails3's own runtime call wire protocol, bypassing the +// generated bindings entirely -- page.evaluate can't `import` a +// production-bundled chunk by a stable path, and the generated +// bindings' own numeric method IDs (Call.ByID) are regenerated per +// `wails3 generate bindings` run, too fragile to hardcode in a test. +// The METHOD NAME form (Call.ByName, "package.Struct.Method") is the +// stable identifier Wails3 itself resolves bound methods by +// server-side (confirmed directly against @wailsio/runtime's own +// dist/calls.js + dist/runtime.js, and wails3's own bindings_test.go +// MethodName format) -- this replicates that exact wire shape via a +// plain POST, the same request the generated binding itself would send. +export async function backdatePendingMCPWrite(page: Page, id: string, ageMinutes: number): Promise { + const res = await page.request.post('/wails/runtime', { + headers: { 'x-wails-client-id': 'e2e-test-knob', 'Content-Type': 'application/json' }, + data: { + object: 0, // objectNames.Call + method: 0, // CallBinding + args: { + 'call-id': `e2e-backdate-${Date.now()}`, + methodName: 'github.com/alicoding/mill/internal/services/settingssvc.SettingsService.DebugBackdatePendingMCPWrite', + args: [id, ageMinutes], + }, + }, + }) + if (!res.ok()) { + throw new Error(`backdatePendingMCPWrite failed: ${res.status()} ${await res.text()}`) + } +} + export async function connectMCPClient(workerIndex: number): Promise { const client = new Client({ name: 'mill-e2e', version: '0.0.0' }) const transport = new StreamableHTTPClientTransport(new URL(`http://127.0.0.1:${MCP_BASE_PORT + workerIndex}`)) diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx index 9c965c74..90c6dd09 100644 --- a/frontend/src/app/App.tsx +++ b/frontend/src/app/App.tsx @@ -278,11 +278,17 @@ function App() { }); }, [pushActivity, workflows]); - // A missed (timed-out) or denied MCP write is no longer traceless - // (docs/goals/0005-pending-attention-model.md item 3): pushed into - // the same Activity feed under the 'mcp-write' source, same push - // shape as the hotkey-activity handler above -- no workflowID/binding - // of its own, just what was denied/missed and why. + // A missed (timed-out), denied, or cancelled MCP write is no longer + // traceless (docs/goals/0005-pending-attention-model.md item 3): + // pushed into the same Activity feed under the 'mcp-write' source, + // same push shape as the hotkey-activity handler above. workflowID/ + // result now carry real content (docs/goals/0026 item 7 -- "so what I + // can do and nothing I can do"): workflowID is set only when the + // gated tool named an existing target (update/publish/delete_workflow; + // empty for import_* tools, which mint a new entity), which is what + // makes ActivityView's existing WorkflowHoverPreview jump-to-workflow + // icon appear for this row, same as any run row; result is what the + // row's existing canExpand/expand-to-detail mechanism shows. useEffect(() => { return Events.On('mcp-write-activity', (evt) => { pushActivity({ @@ -290,11 +296,11 @@ function App() { time: new Date().toLocaleTimeString(), timestamp: Date.now(), source: 'mcp-write', - workflowID: '', + workflowID: evt.data.workflowID ?? '', label: evt.data.description, success: false, detail: evt.data.outcome, - result: '', + result: evt.data.result ?? '', }); }); }, [pushActivity]); diff --git a/frontend/src/app/ApprovalPrompt.tsx b/frontend/src/app/ApprovalPrompt.tsx index d768b3eb..9d15c92c 100644 --- a/frontend/src/app/ApprovalPrompt.tsx +++ b/frontend/src/app/ApprovalPrompt.tsx @@ -4,6 +4,7 @@ import { ShieldIcon } from '@primer/octicons-react' import { Events } from '@wailsio/runtime' import { ExecutionService, SettingsService } from '../shared/bindings' import type { RunSummary, MCPWriteRequest } from '../shared/bindings' +import { StalenessBadge } from '../shared/StalenessBadge' import styles from './ApprovalPrompt.module.css' // docs/goals/0023-attention-escalation.md item 1: the floating @@ -117,6 +118,7 @@ export function ApprovalPrompt() { {oldest.description} + {items.length > 1 && ( +{items.length - 1} more waiting )} diff --git a/frontend/src/app/MCPWriteApprovals.tsx b/frontend/src/app/MCPWriteApprovals.tsx index 703a2db4..f3017b50 100644 --- a/frontend/src/app/MCPWriteApprovals.tsx +++ b/frontend/src/app/MCPWriteApprovals.tsx @@ -4,6 +4,7 @@ import { ShieldIcon } from '@primer/octicons-react' import { Events } from '@wailsio/runtime' import { SettingsService } from '../shared/bindings' import type { MCPWriteRequest } from '../shared/bindings' +import { StalenessBadge } from '../shared/StalenessBadge' import styles from './App.module.css' // The per-write MCP approval surface (docs/adr/0032's park-and-poll @@ -48,6 +49,7 @@ export function MCPWriteApprovals() { {p.description} + diff --git a/frontend/src/composition/WorkflowRunsPanel.tsx b/frontend/src/composition/WorkflowRunsPanel.tsx index fdfd26f9..58a0feaa 100644 --- a/frontend/src/composition/WorkflowRunsPanel.tsx +++ b/frontend/src/composition/WorkflowRunsPanel.tsx @@ -7,6 +7,8 @@ import { RunKind, type RunDetail, type RunSummary } from '../shared/bindings' import type { AttributeDef } from '../../bindings/github.com/alicoding/mill/internal/domain/composition/models' import { ApprovalValuesForm, attrsForPending } from '../shared/ApprovalValuesForm' import { formatRunStartedAt } from '../shared/runTime' +import { StalenessBadge } from '../shared/StalenessBadge' +import { ENQUEUED_STALE_THRESHOLD_MS, isStuckEnqueued } from '../shared/enqueuedStale' import styles from '../shared/ListCard.module.css' import PageContainer from '../shared/PageContainer' @@ -201,11 +203,27 @@ function WorkflowRunsPanel({ workflowId, attrs, initialRunId, onInitialRunConsum // tr:has([data-selected='true']) (ListCard.module.css). renderCell: (run) => ( - {run.pending ? ( - - ) : ( - - )} + + {run.pending ? ( + + ) : ( + + )} + {/* Stuck-ENQUEUED presentation (docs/goals/0026 item 8): a + run that queued forever without ever starting reads as + live otherwise -- age emphasis, same tier language as + the pending-approval staleness treatment (item 2), at + its own 5-minute bar. No "expires in" caption -- an + ENQUEUED run has no 24h clock of its own. */} + {isStuckEnqueued(run) && ( + + )} + ), }, @@ -273,6 +291,14 @@ function WorkflowRunsPanel({ workflowId, attrs, initialRunId, onInitialRunConsum + {isStuckEnqueued(detail) && ( + + )} {/* The run's own identity, so two identical-outcome runs are still tellable apart in the detail header -- the other half of the selected-row fix above. */} diff --git a/frontend/src/shared/StalenessBadge.tsx b/frontend/src/shared/StalenessBadge.tsx new file mode 100644 index 00000000..b28cae44 --- /dev/null +++ b/frontend/src/shared/StalenessBadge.tsx @@ -0,0 +1,47 @@ +import { Label, Stack, Text } from '@primer/react' +import { formatUpdated } from './inventorySort' +import { ageTier, formatExpiresIn, MCP_WRITE_EXPIRY_MS } from './staleness' +import styles from './ListCard.module.css' + +// Shared age-tiered presentation for a pending actionable item +// (docs/goals/0026 item 2): fresh (<15m) renders as plain relative time, +// exactly as every other row already did; aging (15m+) gets visible +// emphasis (an attention-colored Label instead of plain muted text) plus +// an "expires in Nh" caption counting down the shared 24h clock. Reused +// verbatim by ReviewView's pending rows (both guardrail/human-review/ +// debug parks and MCP write requests), MCPWriteApprovals' banner, and +// ApprovalPrompt's floating prompt -- one component, not three +// near-identical age renderings. +export function StalenessBadge({ + createdAt, + expiryMs = MCP_WRITE_EXPIRY_MS, + thresholdMs, + showExpiry = true, + testId, +}: { + createdAt: string | Date + expiryMs?: number + // thresholdMs overrides the default 15-minute age-tier bar -- item + // 8's stuck-ENQUEUED presentation passes its own 5-minute bar + // ("same tier language" at a different cutoff, not a second + // mechanism). + thresholdMs?: number + // showExpiry hides the "expires in Nh" caption -- a stuck-ENQUEUED + // run (item 8) has no 24h expiry concept of its own, only the age + // emphasis; every 24h-clocked pending item (item 2) keeps it. + showExpiry?: boolean + testId?: string +}) { + const tier = ageTier(createdAt, Date.now(), thresholdMs) + const relative = formatUpdated(typeof createdAt === 'string' ? createdAt : createdAt.toISOString()) + if (tier === 'fresh') { + return {relative} + } + const expires = showExpiry ? formatExpiresIn(createdAt, expiryMs) : '' + return ( + + + {expires && {expires}} + + ) +} diff --git a/frontend/src/shared/bindings.ts b/frontend/src/shared/bindings.ts index da8ffcb4..4112ba9f 100644 --- a/frontend/src/shared/bindings.ts +++ b/frontend/src/shared/bindings.ts @@ -32,7 +32,7 @@ export type { } from '../../bindings/github.com/alicoding/mill/internal/services/executionsvc' export { GuardrailService } from '../../bindings/github.com/alicoding/mill/internal/services/guardrailsvc' export type { RuleTestResult } from '../../bindings/github.com/alicoding/mill/internal/services/guardrailsvc' -export type { MCPWriteActivity, MCPWriteRequest } from '../../bindings/github.com/alicoding/mill/internal/services/mcpsvc' +export type { MCPWriteActivity, MCPWriteRequest, MCPWriteResolved } from '../../bindings/github.com/alicoding/mill/internal/services/mcpsvc' export { SettingsService } from '../../bindings/github.com/alicoding/mill/internal/services/settingssvc' export type { BuildInfo, diff --git a/frontend/src/shared/enqueuedStale.test.ts b/frontend/src/shared/enqueuedStale.test.ts new file mode 100644 index 00000000..ead8dd0b --- /dev/null +++ b/frontend/src/shared/enqueuedStale.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from 'vitest' +import { isStuckEnqueued } from './enqueuedStale' + +const NOW = Date.parse('2026-08-11T12:00:00Z') +const minutesAgo = (m: number) => new Date(NOW - m * 60 * 1000).toISOString() + +describe('isStuckEnqueued', () => { + it('is false for a fresh ENQUEUED run', () => { + expect(isStuckEnqueued({ status: 'ENQUEUED', startedAt: minutesAgo(1) } as never, NOW)).toBe(false) + }) + + it('is true for an ENQUEUED run older than 5 minutes', () => { + expect(isStuckEnqueued({ status: 'ENQUEUED', startedAt: minutesAgo(6) } as never, NOW)).toBe(true) + }) + + it('is false for a non-ENQUEUED run regardless of age', () => { + expect(isStuckEnqueued({ status: 'RUNNING', startedAt: minutesAgo(30) } as never, NOW)).toBe(false) + }) + + it('is false when the run has a pending approval, even if ENQUEUED', () => { + expect(isStuckEnqueued({ status: 'ENQUEUED', startedAt: minutesAgo(30), pending: {} } as never, NOW)).toBe(false) + }) +}) diff --git a/frontend/src/shared/enqueuedStale.ts b/frontend/src/shared/enqueuedStale.ts new file mode 100644 index 00000000..1c2df705 --- /dev/null +++ b/frontend/src/shared/enqueuedStale.ts @@ -0,0 +1,20 @@ +import type { RunSummary } from './bindings' +import { ageTier } from './staleness' + +// Stuck-ENQUEUED presentation (docs/goals/0026 item 8): a real +// zombie ENQUEUED run (a queued-forever workflow that never got +// dequeued) was found in production data, reading as live when it was +// never going to progress. Shared by WorkflowRunsPanel and Activity's +// runs explorer -- one definition of "stuck," not two. +export const ENQUEUED_STALE_THRESHOLD_MS = 5 * 60 * 1000 + +// isStuckEnqueued is true only for a run that's BOTH still ENQUEUED +// (never even started) AND older than the 5-minute bar -- a run that's +// pending on a guardrail approval already has its own distinct +// presentation (isDebugPark/isHumanReview et al.), so this +// deliberately excludes anything with `pending` set even if DBOS +// happens to report ENQUEUED underneath it. +export function isStuckEnqueued(run: Pick, now: number = Date.now()): boolean { + if (run.status !== 'ENQUEUED' || run.pending) return false + return ageTier(run.startedAt, now, ENQUEUED_STALE_THRESHOLD_MS) === 'aging' +} diff --git a/frontend/src/shared/staleness.test.ts b/frontend/src/shared/staleness.test.ts new file mode 100644 index 00000000..09527742 --- /dev/null +++ b/frontend/src/shared/staleness.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it } from 'vitest' +import { + ageTier, formatExpiresIn, formatLastChecked, isPollStale, + MCP_WRITE_EXPIRY_MS, STALE_AGE_THRESHOLD_MS, +} from './staleness' + +const NOW = Date.parse('2026-08-11T12:00:00Z') +const minutesAgo = (m: number) => new Date(NOW - m * 60 * 1000).toISOString() +const hoursAgo = (h: number) => new Date(NOW - h * 60 * 60 * 1000).toISOString() + +describe('ageTier', () => { + it('reads fresh under the 15-minute bar', () => { + expect(ageTier(minutesAgo(5), NOW)).toBe('fresh') + expect(ageTier(minutesAgo(14), NOW)).toBe('fresh') + }) + + it('reads aging at and past the 15-minute bar', () => { + expect(ageTier(minutesAgo(15), NOW)).toBe('aging') + expect(ageTier(hoursAgo(4), NOW)).toBe('aging') + }) + + it('treats a missing/unparseable timestamp as fresh, never inventing emphasis', () => { + expect(ageTier(undefined, NOW)).toBe('fresh') + expect(ageTier(null, NOW)).toBe('fresh') + expect(ageTier('not-a-date', NOW)).toBe('fresh') + }) + + it('exposes the threshold as the shared constant', () => { + expect(STALE_AGE_THRESHOLD_MS).toBe(15 * 60 * 1000) + }) +}) + +describe('formatExpiresIn', () => { + it('counts down in hours from the 24h clock by default', () => { + expect(formatExpiresIn(hoursAgo(1), MCP_WRITE_EXPIRY_MS, NOW)).toBe('expires in 23h') + expect(formatExpiresIn(hoursAgo(20), MCP_WRITE_EXPIRY_MS, NOW)).toBe('expires in 4h') + }) + + it('switches to minutes under an hour remaining', () => { + expect(formatExpiresIn(hoursAgo(23.5), MCP_WRITE_EXPIRY_MS, NOW)).toBe('expires in 30m') + }) + + it('renders "expires imminently" once the deadline has passed', () => { + expect(formatExpiresIn(hoursAgo(25), MCP_WRITE_EXPIRY_MS, NOW)).toBe('expires imminently') + }) + + it('returns empty for a missing timestamp', () => { + expect(formatExpiresIn(undefined, MCP_WRITE_EXPIRY_MS, NOW)).toBe('') + }) + + it('honors a custom expiry window', () => { + expect(formatExpiresIn(minutesAgo(30), 60 * 60 * 1000, NOW)).toBe('expires in 30m') + }) +}) + +describe('isPollStale', () => { + it('is false for a recent poll', () => { + expect(isPollStale(minutesAgo(2), NOW)).toBe(false) + }) + + it('is true past the 5-minute bar', () => { + expect(isPollStale(minutesAgo(6), NOW)).toBe(true) + }) + + it('is false when never polled (undefined) -- no noise', () => { + expect(isPollStale(undefined, NOW)).toBe(false) + expect(isPollStale(null, NOW)).toBe(false) + }) +}) + +describe('formatLastChecked', () => { + it('is empty when not stale (fresh polling, no noise)', () => { + expect(formatLastChecked(minutesAgo(1), NOW)).toBe('') + }) + + it('is empty when never polled', () => { + expect(formatLastChecked(undefined, NOW)).toBe('') + }) + + it('renders "requester last checked Nm ago" once stale', () => { + expect(formatLastChecked(minutesAgo(9), NOW)).toMatch(/^requester last checked .*ago$/) + }) +}) diff --git a/frontend/src/shared/staleness.ts b/frontend/src/shared/staleness.ts new file mode 100644 index 00000000..6660eef6 --- /dev/null +++ b/frontend/src/shared/staleness.ts @@ -0,0 +1,111 @@ +import { formatUpdated } from './inventorySort' + +// Shared age-tier/expiry/liveness presentation for every "someone is +// waiting on a human" surface (docs/goals/0026 items 2/3/8): a pending +// MCP write, a guardrail/human-review park, and a stuck-ENQUEUED run +// all share the same 24h clock (ADR-0032's expiry, mirrored by the +// guardrail park's own timeout, §8) and the same owner-observed +// problem -- a stale item reading exactly as urgent as a fresh one +// ("feels like I missed something / it's not working"). One +// implementation, reused by ReviewView/MCPWriteApprovals/ApprovalPrompt +// (item 2), the requester-liveness hint (item 3), and +// WorkflowRunsPanel/ActivityRunsExplorer's stuck-ENQUEUED presentation +// (item 8) -- not five near-identical age calculations. + +// STALE_AGE_THRESHOLD_MS: the age-tier cutoff (item 2's "<15m as-is; +// older gets emphasis"). Deliberately not configurable -- a single +// fixed bar, same as formatUpdated's own fixed relative-time bucket. +export const STALE_AGE_THRESHOLD_MS = 15 * 60 * 1000 + +// MCP_WRITE_EXPIRY_MS mirrors mcpWriteExpiry (millmcpservice_approval.go) +// and the guardrail park's own timeout (§8) -- the one 24h clock every +// "expires in Nh" caption reads from. +export const MCP_WRITE_EXPIRY_MS = 24 * 60 * 60 * 1000 + +// POLL_STALE_THRESHOLD_MS: item 3's own ">5m stale" bar for the +// requester-liveness hint -- deliberately different from (smaller than) +// the age-tier threshold, since polling is a much higher-frequency +// signal than "how old is this ask." +export const POLL_STALE_THRESHOLD_MS = 5 * 60 * 1000 + +export type AgeTier = 'fresh' | 'aging' + +// toMillis parses a Go-emitted RFC3339 timestamp (or a Date, or +// null/undefined -- every optional *time.Time field arrives as +// undefined when unset, not a zero-value string) into epoch +// milliseconds, or null when there's nothing real to compute from. +// Deliberately simpler than inventorySort.ts's parseUpdated: every +// timestamp this module reads (CreatedAt on a pending write, StartedAt +// on a run) is always real once present -- there's no "legacy +// zero-value Go time" case to filter out here, unlike an entity's +// UpdatedAt. +function toMillis(ts: string | Date | null | undefined): number | null { + if (ts == null || ts === '') return null + const ms = typeof ts === 'string' ? Date.parse(ts) : ts.getTime() + return Number.isNaN(ms) ? null : ms +} + +// ageTier classifies how long ago createdAt was, against thresholdMs +// (the shared 15-minute bar, item 2, by default -- item 8's +// stuck-ENQUEUED presentation passes its own 5-minute bar instead, +// "same tier language" at a different cutoff, not a second mechanism). +// An unparseable/missing timestamp reads as 'fresh' -- never invent +// emphasis over data that isn't really there. +export function ageTier( + createdAt: string | Date | null | undefined, + now: number = Date.now(), + thresholdMs: number = STALE_AGE_THRESHOLD_MS, +): AgeTier { + const ms = toMillis(createdAt) + if (ms === null) return 'fresh' + return now - ms >= thresholdMs ? 'aging' : 'fresh' +} + +// formatExpiresIn renders "expires in Nh" / "expires in Nm" counting +// down from createdAt + expiryMs (default: the shared 24h clock). +// '' when createdAt is missing/unparseable (nothing to compute); once +// the deadline has actually passed (the lazy sweep just hasn't caught +// up to relabel the record yet) renders 'expires imminently' rather +// than a confusing negative duration. +export function formatExpiresIn( + createdAt: string | Date | null | undefined, + expiryMs: number = MCP_WRITE_EXPIRY_MS, + now: number = Date.now(), +): string { + const ms = toMillis(createdAt) + if (ms === null) return '' + const remaining = ms + expiryMs - now + if (remaining <= 0) return 'expires imminently' + const hours = remaining / (60 * 60 * 1000) + if (hours >= 1) return `expires in ${Math.ceil(hours)}h` + const minutes = Math.max(1, Math.ceil(remaining / (60 * 1000))) + return `expires in ${minutes}m` +} + +// isPollStale answers item 3's own gate: has it been long enough since +// the requester last polled (check_write_status) that surfacing it is +// worth the pixels, rather than noise on every fresh poll. A never- +// polled write (lastPolledAt undefined) deliberately reads as NOT +// stale here -- there's nothing to say yet, and the age-tier emphasis +// above already covers "this is old," so this stays specifically about +// requester liveness, not a second staleness signal wearing the same +// clothes. +export function isPollStale( + lastPolledAt: string | Date | null | undefined, + now: number = Date.now(), + thresholdMs: number = POLL_STALE_THRESHOLD_MS, +): boolean { + const ms = toMillis(lastPolledAt) + if (ms === null) return false + return now - ms >= thresholdMs +} + +// formatLastChecked renders "requester last checked Nm ago" -- '' when +// there's nothing to say (never polled, or not stale enough yet per +// isPollStale). Reuses formatUpdated for the relative-time phrase +// rather than a second Intl.RelativeTimeFormat instance. +export function formatLastChecked(lastPolledAt: string | Date | null | undefined, now: number = Date.now()): string { + if (!isPollStale(lastPolledAt, now)) return '' + const rel = formatUpdated(lastPolledAt as string) + return rel ? `requester last checked ${rel}` : '' +} diff --git a/frontend/src/views/ActivityRunsExplorer.tsx b/frontend/src/views/ActivityRunsExplorer.tsx index d67d891c..59d04d61 100644 --- a/frontend/src/views/ActivityRunsExplorer.tsx +++ b/frontend/src/views/ActivityRunsExplorer.tsx @@ -1,11 +1,14 @@ import { useEffect, useState } from 'react' -import { Label, Stack, Text, TextInput } from '@primer/react' +import { Button, Label, Stack, Text, TextInput } from '@primer/react' import { DataTable, type Column } from '@primer/react/experimental' +import { StopIcon } from '@primer/octicons-react' import { ResizableTableContainer, TruncatedCell } from '../shared/ResizableTable' import { ExecutionService } from '../shared/bindings' import type { RunSummary } from '../shared/bindings' import type { Workflow } from '../../bindings/github.com/alicoding/mill/internal/domain/composition/models' import { formatRunStartedAt } from '../shared/runTime' +import { StalenessBadge } from '../shared/StalenessBadge' +import { ENQUEUED_STALE_THRESHOLD_MS, isStuckEnqueued } from '../shared/enqueuedStale' import styles from '../shared/ListCard.module.css' // The source-first half of the reference analytics pattern @@ -22,15 +25,35 @@ export function ActivityRunsExplorer({ workflow }: { workflow: Workflow }) { const [runs, setRuns] = useState(null) const [error, setError] = useState('') const [search, setSearch] = useState('') + const [cancellingID, setCancellingID] = useState('') - useEffect(() => { - setRuns(null) - setError('') + const refresh = () => { ExecutionService.ListRunsForWorkflow(workflow.ID) .then((list) => setRuns(list ?? [])) .catch((err) => setError(String(err))) + } + + useEffect(() => { + setRuns(null) + setError('') + refresh() + // eslint-disable-next-line react-hooks/exhaustive-deps }, [workflow.ID]) + // Stuck-ENQUEUED runs get a Stop affordance right in this table + // (docs/goals/0026 item 8) -- this view has no per-row detail/click- + // through of its own (unlike WorkflowRunsPanel's own Stop, which + // lives behind opening a run), so the age-emphasized status alone + // would be "so what I can do and nothing I can do" without one. + const cancelRun = (runID: string) => { + setCancellingID(runID) + setError('') + ExecutionService.CancelRun(runID) + .then(refresh) + .catch((err) => setError(String(err))) + .finally(() => setCancellingID('')) + } + const attrs = workflow.Attributes ?? [] const query = search.trim().toLowerCase() const filtered = (runs ?? []).filter((run) => { @@ -55,11 +78,37 @@ export function ActivityRunsExplorer({ workflow }: { workflow: Workflow }) { { id: 'status', header: 'Status', width: 'auto', renderCell: (run) => ( - + + + {/* Stuck-ENQUEUED presentation (docs/goals/0026 item 8) -- + same age-tier language as item 2's pending-approval + treatment, at ENQUEUED's own 5-minute bar. */} + {isStuckEnqueued(run) && ( + + )} + ), }, + { + id: 'actions', header: '', width: 'auto', + renderCell: (run) => isStuckEnqueued(run) ? ( + + ) : null, + }, // One column per declared attribute -- the workflow's own typed // input schema drives the table's shape, exactly the reference // pattern's point. diff --git a/frontend/src/views/ReviewView.tsx b/frontend/src/views/ReviewView.tsx index 343d3bdf..5f4ab4ac 100644 --- a/frontend/src/views/ReviewView.tsx +++ b/frontend/src/views/ReviewView.tsx @@ -4,11 +4,12 @@ import { Blankslate } from '@primer/react/experimental' import { BugIcon, InboxIcon, PersonIcon, PlugIcon, ShieldIcon } from '@primer/octicons-react' import { Events } from '@wailsio/runtime' import { ExecutionService, SettingsService } from '../shared/bindings' -import type { MCPWriteRequest, RunSummary } from '../shared/bindings' +import type { MCPWriteRequest, MCPWriteResolved, RunSummary } from '../shared/bindings' import { ApprovalValuesForm, attrsForPending } from '../shared/ApprovalValuesForm' import { useAppStore } from '../shared/store' import { formatRunStartedAt } from '../shared/runTime' -import { formatUpdated } from '../shared/inventorySort' +import { StalenessBadge } from '../shared/StalenessBadge' +import { formatLastChecked } from '../shared/staleness' import styles from '../shared/ListCard.module.css' import PageContainer from '../shared/PageContainer' @@ -59,6 +60,13 @@ function ReviewView() { // actionable rows -- one store, multiple surfaces, never a second // competing pending list (goal 0005). const [pendingWrites, setPendingWrites] = useState([]) + // Resolved MCP writes (docs/goals/0026 item 6) -- the durable 24h + // outcome records (approved/denied/cancelled/expired), merged into + // Recently-resolved alongside run resolutions rather than living in a + // second, separate list; today only session-only Activity showed + // these at all, gone on restart even though the record itself was + // already durable. + const [resolvedWrites, setResolvedWrites] = useState([]) const [inputs, setInputs] = useState>>({}) const [workflowFilter, setWorkflowFilter] = useState('') const [kindFilter, setKindFilter] = useState('') @@ -75,6 +83,7 @@ function ReviewView() { }) .catch((err) => setError(String(err))) SettingsService.PendingMCPWrites().then((p) => setPendingWrites(p ?? [])).catch(() => {}) + SettingsService.ResolvedMCPWrites().then((p) => setResolvedWrites(p ?? [])).catch(() => {}) } useEffect(() => { @@ -138,6 +147,22 @@ function ReviewView() { const kindMatches = (run: RunSummary) => !kindFilter || kindFilter === pendingKind(run) const showPendingWrites = kindFilter === '' || kindFilter === 'mcp-write' + // Recently-resolved is a merged, newest-first list of run resolutions + // AND resolved MCP writes (docs/goals/0026 item 6) -- one section, not + // two adjacent lists, same "recognition, not confirmation" discipline + // the pending rows already use (PlugIcon distinguishes a write row). + // MCP writes aren't scoped to a workflow, so they're excluded whenever + // a workflow filter is active (nothing for them to match). + type ResolvedEntry = + | { kind: 'run'; key: string; time: number; run: RunSummary } + | { kind: 'mcp-write'; key: string; time: number; write: MCPWriteResolved } + const resolvedEntries: ResolvedEntry[] = [ + ...resolved + .filter((r) => !workflowFilter || r.workflowID === workflowFilter) + .map((run): ResolvedEntry => ({ kind: 'run', key: run.runID, time: Date.parse(run.completedAt || run.startedAt), run })), + ...(workflowFilter ? [] : resolvedWrites.map((w): ResolvedEntry => ({ kind: 'mcp-write', key: w.id, time: Date.parse(w.resolvedAt), write: w }))), + ].sort((a, b) => b.time - a.time) + // Loading: the first ListRuns() round trip hasn't resolved yet -- // Home.tsx's own centered-Spinner-under-the-Heading treatment is the // suite's standard for this (no dedicated skeleton component exists). @@ -202,27 +227,37 @@ function ReviewView() { filterable by workflow (they aren't scoped to one), but the kind filter does hide them like any other kind. */} - {showPendingWrites && pendingWrites.map((w) => ( -
- - - - MCP write request - - {formatUpdated(w.createdAt)} - - {w.description} - - - + {showPendingWrites && pendingWrites.map((w) => { + const lastChecked = formatLastChecked(w.lastPolledAt) + return ( +
+ + + + MCP write request + + + + {w.description} + {/* Requester-liveness hint (docs/goals/0026 item 3): only + rendered once check_write_status hasn't been called in + >5m -- an abandoned request visibly reads as abandoned, + a fresh-polling one stays silent (no noise). */} + {lastChecked && ( + {lastChecked} + )} + + + + - -
- ))} +
+ ) + })} @@ -240,7 +275,7 @@ function ReviewView() { - {formatRunStartedAt(run.startedAt)} + Step {run.pending?.nodeTypeLabel || run.pending?.nodeTypeID} @@ -270,30 +305,53 @@ function ReviewView() { ))} - {resolved.filter((r) => !workflowFilter || r.workflowID === workflowFilter).length > 0 && ( + {resolvedEntries.length > 0 && ( <> Recently resolved - {resolved.filter((r) => !workflowFilter || r.workflowID === workflowFilter).slice(0, 10).map((run) => ( + {resolvedEntries.slice(0, 10).map((entry) => entry.kind === 'run' ? (
openRun(run)} + onClick={() => openRun(entry.run)} > - {run.workflowLabel} + {entry.run.workflowLabel} + + + {formatRunStartedAt(entry.run.startedAt)} + +
+ ) : ( + // A resolved MCP write (docs/goals/0026 item 6) -- the same + // durable 24h outcome record check_write_status reads, + // surfaced here so it isn't only visible via session-only + // Activity (gone on restart). PlugIcon matches the pending + // row's own identity cue ("recognition, not confirmation"); + // not clickable -- there's no run to drill into. +
+ + + + {entry.write.description} + - - {formatRunStartedAt(run.startedAt)} + {formatRunStartedAt(entry.write.resolvedAt)}
))} diff --git a/internal/adapters/execution/aliases.go b/internal/adapters/execution/aliases.go index 919c2d00..a1f7cfdd 100644 --- a/internal/adapters/execution/aliases.go +++ b/internal/adapters/execution/aliases.go @@ -64,6 +64,19 @@ type ( // ListWorkflowsOption configures one ListWorkflows call. ListWorkflowsOption = dbos.ListWorkflowsOption + // Queue is a registered DBOS queue handle -- docs/goals/0026 item 8's + // regression coverage needs a real, reproducible way to construct a + // still-ENQUEUED run (a zero-worker-concurrency queue never dequeues + // anything submitted to it); Mill's own production run path doesn't + // use a queue today (RunWorkflow starts every run directly), so this + // is currently a test-only alias, kept here rather than in the test + // file itself since it's still a real re-export of the underlying + // SDK type, same shape as every other alias in this file. + Queue = dbos.Queue + + // QueueOption configures one RegisterQueue call. + QueueOption = dbos.QueueOption + // Client is DBOS's narrower, non-launched handle -- every Context // is also a Client (Context embeds Client, confirmed directly // against the installed dbos.Context/dbos.Client interface @@ -147,6 +160,14 @@ func CancelWorkflow(ctx Client, workflowID string, opts ...CancelWorkflowOption) return dbos.CancelWorkflow(ctx, workflowID, opts...) } +// RegisterQueue registers a named DBOS queue -- test-only today (see +// the Queue alias's own doc comment); confirmed directly against the +// installed SDK that this is safe to call even after Launch (its own +// queues_test.go registers queues post-launch throughout). +func RegisterQueue(ctx Client, name string, opts ...QueueOption) (Queue, error) { + return dbos.RegisterQueue(ctx, name, opts...) +} + var ( // GetWorkflowSteps returns one run's recorded step history, in // order -- the data behind the execution-visibility UI. @@ -164,6 +185,18 @@ var ( // one -- Mill always supplies its own (a fresh UUID per run). WithWorkflowID = dbos.WithWorkflowID + // WithQueue submits a run through a registered Queue instead of + // starting it directly -- test-only today (see the Queue alias's own + // doc comment). + WithQueue = dbos.WithQueue + + // WithWorkerConcurrency caps how many queued workflows this executor + // dequeues concurrently -- test-only today; a queue registered with + // 0 never dequeues anything submitted to it, the real DBOS mechanism + // docs/goals/0026 item 8's regression test uses to construct a + // reproducibly-stuck ENQUEUED run. + WithWorkerConcurrency = dbos.WithWorkerConcurrency + // WithWorkflowName pins a RegisterWorkflow call's recorded name // instead of deriving it from the Go function's own runtime name -- // Mill registers a bound method value, whose reflect-derived name diff --git a/internal/services/executionsvc/executionservice_enqueued_test.go b/internal/services/executionsvc/executionservice_enqueued_test.go new file mode 100644 index 00000000..49fd6c22 --- /dev/null +++ b/internal/services/executionsvc/executionservice_enqueued_test.go @@ -0,0 +1,124 @@ +package executionsvc + +import ( + "path/filepath" + "testing" + "time" + + "github.com/alicoding/mill/internal/adapters/execution" + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/compositionsvc" + "github.com/alicoding/mill/internal/services/guardrailsvc" + "github.com/alicoding/mill/internal/services/servicetest" + "github.com/google/uuid" +) + +// docs/goals/0026 item 8: a real, stuck-forever ENQUEUED run was found +// in production data (a zombie run from a morning error) -- reading as +// live when it was never going to progress. This is Mill's own +// regression coverage for the two things that ask depends on: (1) a +// RunSummary for an ENQUEUED run carries real presentation fields (a +// non-zero StartedAt in particular -- the frontend's age-tier +// presentation, docs/goals/0026 items 2/8, has nothing to render an age +// from otherwise) and (2) CancelRun actually cancels a still-ENQUEUED +// run, not just a running/pending one. +// +// Mill's own production path never enqueues a run onto a DBOS Queue +// (executionservice_test.go's TestRunWorkflow_SummaryHasNonZeroStartedAt +// already documents this directly), so there's no way to reach ENQUEUED +// through RunWorkflow/RunWorkflowWithPayload as written. A queue +// registered with WithWorkerConcurrency(0) never dequeues anything +// submitted to it -- the real, SDK-documented mechanism this test uses +// to construct a reproducible, indefinitely-stuck ENQUEUED run, calling +// through to the exact same registered workflow function +// (exec.runWorkflow) production code uses, via execution.RunWorkflow + +// execution.WithQueue directly (white-box, same package). +func TestListRuns_EnqueuedRun_PresentationFieldsAndCancelPath(t *testing.T) { + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + workflowID := findBuiltInWorkflowID(t, comp, "Load sample HTML") + + dbPath := filepath.Join(t.TempDir(), "enqueued.db") + exec, err := NewExecutionService("sqlite:"+dbPath, comp, guardrailsvc.NewGuardrailService(store, comp)) + if err != nil { + t.Fatalf("NewExecutionService: %v", err) + } + t.Cleanup(func() { _ = exec.Shutdown(2 * time.Second) }) + + wf, ok := exec.findWorkflow(workflowID) + if !ok { + t.Fatalf("findWorkflow(%q): not found", workflowID) + } + nodes, edges, attrs, version, err := composition.ResolveRunnable(wf, true, 0) + if err != nil { + t.Fatalf("ResolveRunnable: %v", err) + } + + queue, err := execution.RegisterQueue(exec.ctx, "test-stuck-enqueued", execution.WithWorkerConcurrency(0)) + if err != nil { + t.Fatalf("RegisterQueue: %v", err) + } + + runID := uuid.NewString() + before := time.Now().Add(-time.Minute) + if _, err := execution.RunWorkflow(exec.ctx, exec.runWorkflow, runInput{ + WorkflowID: wf.ID, Nodes: nodes, Edges: edges, Attributes: attrs, + Kind: RunKindTest, Version: version, + }, execution.WithWorkflowID(runID), execution.WithQueue(queue)); err != nil { + t.Fatalf("RunWorkflow (enqueue): %v", err) + } + + // Poll briefly: the row lands via an async DB write relative to + // RunWorkflow's own return, but with WorkerConcurrency(0) it will + // NEVER be dequeued, so once observed as ENQUEUED it stays that way + // for the rest of this test -- no race to worry about after this + // loop exits. + var summary RunSummary + deadline := time.Now().Add(5 * time.Second) + for { + summary, err = exec.summaryFor(runID) + if err == nil && summary.Status == "ENQUEUED" { + break + } + if time.Now().After(deadline) { + t.Fatalf("run never reached ENQUEUED status (last status %q, err %v)", summary.Status, err) + } + time.Sleep(20 * time.Millisecond) + } + + // Presentation fields (item 8's "run summary presentation fields"): + // StartedAt must be populated (falls back to CreatedAt for a run + // DBOS never dequeued -- executionservice_summary.go's own + // documented fallback) so the frontend's age-tier presentation has + // something real to compute "expires in Nh" from; a zero StartedAt + // would silently break it. + if summary.StartedAt.IsZero() { + t.Error("ENQUEUED run summary.StartedAt is zero, want the CreatedAt fallback (see executionservice_summary.go)") + } + if summary.StartedAt.Before(before) { + t.Errorf("summary.StartedAt = %v, want no earlier than %v", summary.StartedAt, before) + } + if summary.WorkflowLabel != "Load sample HTML" { + t.Errorf("summary.WorkflowLabel = %q, want the real workflow label even while enqueued", summary.WorkflowLabel) + } + + // Cancel path: CancelRun must handle a still-ENQUEUED run. DBOS's + // own CancelWorkflow is documented to cancel "a running or enqueued + // workflow" -- confirmed here against a real ENQUEUED row, not just + // trusted from the doc comment. + if err := exec.CancelRun(runID); err != nil { + t.Fatalf("CancelRun on a still-ENQUEUED run: %v", err) + } + + deadline = time.Now().Add(5 * time.Second) + for { + summary, err = exec.summaryFor(runID) + if err == nil && summary.Status == "CANCELLED" { + break + } + if time.Now().After(deadline) { + t.Fatalf("run never reached CANCELLED status after CancelRun (last status %q)", summary.Status) + } + time.Sleep(20 * time.Millisecond) + } +} diff --git a/internal/services/mcpsvc/millmcpservice_approval.go b/internal/services/mcpsvc/millmcpservice_approval.go index 840bb626..30e83e59 100644 --- a/internal/services/mcpsvc/millmcpservice_approval.go +++ b/internal/services/mcpsvc/millmcpservice_approval.go @@ -3,8 +3,6 @@ package mcpsvc import ( "encoding/json" "fmt" - "log/slog" - "sort" "time" "github.com/google/uuid" @@ -59,6 +57,13 @@ const ( MCPWriteStatusApproved MCPWriteStatus = "approved" MCPWriteStatusDenied MCPWriteStatus = "denied" MCPWriteStatusExpired MCPWriteStatus = "expired" + // MCPWriteStatusCancelled is the requester's own withdrawal of a + // still-pending write (docs/goals/0026 item 1) -- the missing fourth + // verb alongside park/poll/resolve (tasks/cancel in the MCP Tasks + // spec is the direct precedent ADR-0032 already mirrors for the rest + // of this lifecycle). Deliberately distinct from denied: nobody + // weighed in and said no, the requester simply stopped needing it. + MCPWriteStatusCancelled MCPWriteStatus = "cancelled" ) // MCPWriteRecord is one gated write's full durable lifecycle record -- @@ -84,6 +89,13 @@ type MCPWriteRecord struct { // approve it; Error then carries the write's own failure). Error string `json:"error,omitempty"` ResolvedAt *time.Time `json:"resolvedAt,omitempty"` + // LastPolledAt records the most recent check_write_status call for + // this write (docs/goals/0026 item 3, decided without further + // research) -- polling IS the natural requester heartbeat, so a + // pending write nobody has checked on in a while visibly reads as + // abandoned rather than merely old. Persisted like every other + // lifecycle field so it survives a restart. + LastPolledAt *time.Time `json:"lastPolledAt,omitempty"` // decision signals the courtesy-window select in gateWrite once // ResolveMCPWrite finalizes this record -- unexported, so @@ -104,31 +116,26 @@ type MCPWriteRequest struct { ID string `json:"id"` Description string `json:"description"` CreatedAt time.Time `json:"createdAt"` + // LastPolledAt mirrors MCPWriteRecord's own field (docs/goals/0026 + // item 3) -- nil when the requester has never called + // check_write_status on this id yet. + LastPolledAt *time.Time `json:"lastPolledAt,omitempty"` } -// MCPWriteActivity is pushed for a missed (expired) or denied MCP -// write so it's no longer traceless (docs/goals/0005-pending-attention- -// model.md item 3). Reuses the same activity-push shape App.tsx's -// hotkey-activity handler already established, under a distinct -// "mcp-write" ActivitySource so it's filterable, not conflated with a -// workflow trigger. -type MCPWriteActivity struct { - Description string `json:"description"` - // Outcome is "denied" or "expired" -- an approved write never - // reaches here, there's nothing traceless about it. - Outcome string `json:"outcome"` -} - -// emitMCPWriteActivity pushes MCPWriteActivity to the frontend. -// application.Get() is nil in a headless Go test process -- a no-op -// there, same guard executionservice_guardrail.go's -// emitGuardrailPendingChanged uses. -func emitMCPWriteActivity(description, outcome string) { - app := application.Get() - if app == nil { - return - } - app.Event.Emit("mcp-write-activity", MCPWriteActivity{Description: description, Outcome: outcome}) +// MCPWriteResolved is the frontend-facing shape for an already-resolved +// write (docs/goals/0026 item 6) -- Review's Recently-resolved section +// reads this alongside RunSummary's own resolved rows, merged +// newest-first. Retained for the same 24h window check_write_status +// already promises (sweepLocked's own retention) -- "durable across a +// restart" and "still visible for the same window an MCP client can +// still poll" are the same guarantee, not two. +type MCPWriteResolved struct { + ID string `json:"id"` + Description string `json:"description"` + Status string `json:"status"` // approved / denied / cancelled / expired + Error string `json:"error,omitempty"` + CreatedAt time.Time `json:"createdAt"` + ResolvedAt time.Time `json:"resolvedAt"` } func (m *MillMCPService) approvalRequired() bool { @@ -221,7 +228,11 @@ func (m *MillMCPService) gateWrite(toolName, description, argsJSON string) (*mcp m.writesMu.Lock() status, resultText, errText := rec.Status, rec.ResultText, rec.Error m.writesMu.Unlock() - if status == MCPWriteStatusDenied || (status == MCPWriteStatusApproved && errText != "") { + // Denied and cancelled both mean the original call never gets + // the write it asked for -- an error either way, distinguished + // only in the persisted record/Activity row, not in this + // in-flight caller's own result (docs/goals/0026 item 1). + if status == MCPWriteStatusDenied || status == MCPWriteStatusCancelled || (status == MCPWriteStatusApproved && errText != "") { return nil, fmt.Errorf("%s", errText) } return textResult(resultText), nil @@ -312,19 +323,47 @@ func (m *MillMCPService) ResolveMCPWrite(id string, approve bool) error { } else { finalizeErr = m.finalizeLocked(rec, MCPWriteStatusApproved, resultText, "") } + activityOutcome = string(MCPWriteStatusApproved) } // Signal regardless of finalizeErr -- the decision (and, for // approve, the real side effect) already happened; a courtesy-window // caller still waiting must not hang just because the durable record // of it lagged. m.signalLocked(rec) - description := rec.Description + // Every field emitMCPWriteActivity needs, captured while the lock is + // still held (docs/goals/0026 item 7) -- reading rec's fields after + // Unlock below would be the exact data race the pre-existing + // `description := rec.Description` line already avoided for + // Description alone; extended here to the new fields. + description, toolName, argsJSON, resultText, errText := rec.Description, rec.ToolName, rec.ArgsJSON, rec.ResultText, rec.Error m.writesMu.Unlock() m.emitExpired(expiredDuringSweep) - if activityOutcome != "" { - emitMCPWriteActivity(description, activityOutcome) + result := resultText + if result == "" { + result = errText + } + if result == "" { + result = description + } + // An approved write whose own execution then failed is still an + // "approved" Status (the human's decision was to approve it -- see + // finalizeLocked's own doc comment), but Activity's own outcome + // vocabulary only ever showed "denied"/"expired" before this -- + // only push a real Activity row for the two genuinely traceless + // outcomes (deny, and an approval whose write itself failed); + // approved-and-succeeded already has a visible trace (the new + // entity itself, plus check_write_status/Review's own resolved + // list, docs/goals/0026 item 6). + if activityOutcome == string(MCPWriteStatusDenied) || (activityOutcome == string(MCPWriteStatusApproved) && errText != "") { + emitMCPWriteActivity(description, activityOutcome, toolName, argsJSON, result) } + // The pending-count signal fires on EVERY resolution outcome, + // unconditionally -- unlike the Activity push above, a resolved + // write must always stop counting as pending regardless of whether + // it also gets an Activity row (docs/goals/0026 item 5, the BUG this + // whole function previously never emitted this for at all). + emitMCPWriteApprovalChanged() if finalizeErr != nil { // The decision (and any real side effect) is final either way -- // this error means it failed to durably RECORD, which the human @@ -336,6 +375,54 @@ func (m *MillMCPService) ResolveMCPWrite(id string, approve bool) error { return nil } +// CancelMCPWrite lets the requester withdraw its own still-pending +// write (docs/goals/0026 item 1 -- the missing fourth verb, park/poll/ +// resolve/WITHDRAW; MCP Tasks' own tasks/cancel is the direct precedent +// ADR-0032 already mirrors for the rest of this lifecycle). Cancelled +// is a DISTINCT terminal outcome from denied: nobody weighed in and +// said no, the requester simply stopped needing it -- recorded and +// surfaced identically to denied/expired (never traceless). Ungated: +// cancelling your own request only ever REDUCES pending work, so no +// approval/write-toggle gate applies here (same "ungated" shape as +// check_write_status, not gateWrite's). At-most-once, same locking +// shape as ResolveMCPWrite -- two concurrent cancel/resolve calls on +// the same id can't both observe Status==pending. +func (m *MillMCPService) CancelMCPWrite(id string) error { + m.writesMu.Lock() + expiredDuringSweep := m.sweepLocked(time.Now()) + + rec, ok := m.writes[id] + if !ok { + m.writesMu.Unlock() + m.emitExpired(expiredDuringSweep) + return fmt.Errorf("no MCP write with id %s (it may have already been swept, 24h after resolution)", id) + } + if rec.Status != MCPWriteStatusPending { + m.writesMu.Unlock() + m.emitExpired(expiredDuringSweep) + return fmt.Errorf("MCP write %s was already resolved (%s)", id, rec.Status) + } + + finalizeErr := m.finalizeLocked(rec, MCPWriteStatusCancelled, "", "cancelled by the requester") + // Signal regardless of finalizeErr, same reasoning as + // ResolveMCPWrite's own decision-already-happened comment -- a + // courtesy-window caller (unlikely for a self-cancel, since the + // requester is usually the one who'd be blocked in it, but not + // impossible if a second client polls) must not hang on a + // durability lag. + m.signalLocked(rec) + description, toolName, argsJSON := rec.Description, rec.ToolName, rec.ArgsJSON + m.writesMu.Unlock() + + m.emitExpired(expiredDuringSweep) + emitMCPWriteActivity(description, string(MCPWriteStatusCancelled), toolName, argsJSON, description) + emitMCPWriteApprovalChanged() + if finalizeErr != nil { + return finalizeErr + } + return nil +} + // persistWritesLocked marshals m.writes to the settings store -- caller // must hold writesMu. Returns the marshal/store error rather than // swallowing it (docs/goals/0025 item 1) -- callers decide whether to @@ -383,90 +470,3 @@ func (m *MillMCPService) loadWrites() { _ = m.sweepLocked(time.Now()) } -// sweepLocked lazily transitions any pending record whose 24h window -// has elapsed to expired, and deletes any terminal record whose own -// 24h-since-resolution retention has elapsed -- caller must hold -// writesMu. Returns the descriptions of records that just expired, for -// the caller to push an Activity row for once unlocked. -func (m *MillMCPService) sweepLocked(now time.Time) []string { - var expired []string - changed := false - for id, rec := range m.writes { - if rec.Status == MCPWriteStatusPending && now.Sub(rec.CreatedAt) > mcpWriteExpiry { - rec.Status = MCPWriteStatusExpired - rec.Error = "no human decision within 24h" - rec.ResolvedAt = &now - expired = append(expired, rec.Description) - changed = true - continue - } - if rec.Status != MCPWriteStatusPending && rec.ResolvedAt != nil && now.Sub(*rec.ResolvedAt) > mcpWriteExpiry { - delete(m.writes, id) - changed = true - } - } - if changed { - // Incidental bookkeeping riding along a read call (PendingMCPWrites/ - // writeStatus/ResolveMCPWrite/loadWrites all call this) -- log-only, - // same fire-and-forget treatment as the compositionsvc/configuresvc - // top-up-seeding sweeps (docs/goals/0025 item 1). A failure here - // just means the same records get swept again on the next call. - if err := m.persistWritesLocked(); err != nil { - slog.Error("failed to persist MCP write sweep", "error", err) - } - } - return expired -} - -func (m *MillMCPService) emitExpired(descriptions []string) { - for _, d := range descriptions { - emitMCPWriteActivity(d, string(MCPWriteStatusExpired)) - } -} - -// PendingMCPWrites lists writes currently awaiting a human decision, -// oldest first -- sweeps lazily first so a since-expired record never -// shows up as still pending. -func (m *MillMCPService) PendingMCPWrites() []MCPWriteRequest { - m.writesMu.Lock() - expired := m.sweepLocked(time.Now()) - out := make([]MCPWriteRequest, 0, len(m.writes)) - for _, rec := range m.writes { - if rec.Status == MCPWriteStatusPending { - out = append(out, MCPWriteRequest{ID: rec.ID, Description: rec.Description, CreatedAt: rec.CreatedAt}) - } - } - m.writesMu.Unlock() - m.emitExpired(expired) - - sort.Slice(out, func(i, j int) bool { return out[i].CreatedAt.Before(out[j].CreatedAt) }) - return out -} - -// checkWriteStatusArgs/Result back the check_write_status tool -// (registered in millmcpservice_tools.go's registerTools). -type checkWriteStatusArgs struct { - ID string `json:"id" jsonschema:"the pending write's id, from a gated write tool's 'parked pending human approval' response text"` -} - -type checkWriteStatusResult struct { - Status string `json:"status"` - Description string `json:"description,omitempty"` - Result string `json:"result,omitempty"` - Error string `json:"error,omitempty"` -} - -// writeStatus sweeps lazily, then returns a snapshot of rec's outcome -// fields -- used by check_write_status. -func (m *MillMCPService) writeStatus(id string) (checkWriteStatusResult, bool) { - m.writesMu.Lock() - expired := m.sweepLocked(time.Now()) - rec, ok := m.writes[id] - var res checkWriteStatusResult - if ok { - res = checkWriteStatusResult{Status: string(rec.Status), Description: rec.Description, Result: rec.ResultText, Error: rec.Error} - } - m.writesMu.Unlock() - m.emitExpired(expired) - return res, ok -} diff --git a/internal/services/mcpsvc/millmcpservice_approval_query.go b/internal/services/mcpsvc/millmcpservice_approval_query.go new file mode 100644 index 00000000..ef20c5f4 --- /dev/null +++ b/internal/services/mcpsvc/millmcpservice_approval_query.go @@ -0,0 +1,301 @@ +package mcpsvc + +import ( + "encoding/json" + "fmt" + "log/slog" + "sort" + "time" + + "github.com/wailsapp/wails/v3/pkg/application" +) + +// The read/query half of the park-and-poll lifecycle +// (millmcpservice_approval.go): the Activity-event shape + emit +// helpers, the lazy expiry sweep, and every read-only surface +// (PendingMCPWrites/ResolvedMCPWrites/check_write_status) -- split out +// once the lifecycle file crossed the 500-line limit (CLAUDE.md/§1.3), +// the same "split along a real seam" discipline this codebase already +// applies elsewhere. gateWrite/ResolveMCPWrite/CancelMCPWrite stay in +// millmcpservice_approval.go; everything here is either called BY them +// (sweepLocked, emitExpired, emitMCPWriteActivity/ApprovalChanged) or +// reads the same m.writes map without ever mutating a pending decision. + +// MCPWriteActivity is pushed for a resolved (denied/cancelled/expired, +// or approved-but-failed) MCP write so it's no longer traceless +// (docs/goals/0005-pending-attention-model.md item 3). Reuses the same +// activity-push shape App.tsx's hotkey-activity handler already +// established, under a distinct "mcp-write" ActivitySource so it's +// filterable, not conflated with a workflow trigger. +type MCPWriteActivity struct { + Description string `json:"description"` + // Outcome is "denied", "cancelled", or "expired" -- an + // approved-and-succeeded write never reaches here, there's nothing + // traceless about it. + Outcome string `json:"outcome"` + // ToolName/WorkflowID/Result back Activity's own MCP-write row + // actions (docs/goals/0026 item 7: "so what I can do and nothing I + // can do") -- ToolName is the gated tool this record was for; + // WorkflowID is the workflow it targeted, when the tool names one + // (update_workflow/publish_workflow/delete_workflow's own "id" + // argument -- empty for import_* tools, which mint a NEW entity + // rather than referencing an existing one, so there's nothing to + // jump to); Result is what the Activity row's expandable detail + // panel shows. + ToolName string `json:"toolName,omitempty"` + WorkflowID string `json:"workflowID,omitempty"` + Result string `json:"result,omitempty"` +} + +// targetWorkflowID extracts the workflow a mutation tool's own args +// named, when it names one at all -- update_workflow/publish_workflow/ +// delete_workflow all take a top-level "id" (see updateWorkflowArgs/ +// idArgs in millmcpservice_authoring.go); import_* tools have no +// existing target (they mint a new ID), so this deliberately returns "" +// for anything else rather than guessing. +func targetWorkflowID(toolName, argsJSON string) string { + switch toolName { + case "update_workflow", "publish_workflow", "delete_workflow": + default: + return "" + } + var in struct { + ID string `json:"id"` + } + if err := json.Unmarshal([]byte(argsJSON), &in); err != nil { + return "" + } + return in.ID +} + +// emitMCPWriteActivity pushes MCPWriteActivity to the frontend. +// application.Get() is nil in a headless Go test process -- a no-op +// there, same guard executionservice_guardrail.go's +// emitGuardrailPendingChanged uses. Takes plain fields rather than a +// *MCPWriteRecord deliberately -- every call site reads the record's +// fields it needs while still holding writesMu (the +// `description := rec.Description` pattern in millmcpservice_approval.go +// established this discipline first), so this only ever sees an +// already-safe snapshot. +func emitMCPWriteActivity(description, outcome, toolName, argsJSON, result string) { + app := application.Get() + if app == nil { + return + } + app.Event.Emit("mcp-write-activity", MCPWriteActivity{ + Description: description, + Outcome: outcome, + ToolName: toolName, + WorkflowID: targetWorkflowID(toolName, argsJSON), + Result: result, + }) +} + +// emitMCPWriteApprovalChanged pings the SAME event a new park already +// emits (docs/goals/0026 item 5, BUG fix) -- every surface that shows a +// pending-MCP-write count (the sidebar badge, ReviewView, +// MCPWriteApprovals, ApprovalPrompt) already listens for +// 'mcp-write-approval' and unconditionally refetches on receipt (goal +// 0005's "one signal, many refresh calls" model) -- so a resolution +// path only needs to fire the SAME event, never a second channel. The +// bug this fixes: ResolveMCPWrite (every outcome) and the lazy expiry +// sweep never fired it at all, so the sidebar badge could hold a +// phantom count against an already-empty queue until something else +// happened to trigger a refresh. +// +// Emits a zero-value MCPWriteRequest, NOT an empty struct{} -- found +// live, not assumed: main.go's application.RegisterEvent[MCPWriteRequest] +// ("mcp-write-approval") documents that Wails3 "matches data types +// exactly and no conversion is performed," so an struct{}{} payload +// silently failed this event's own type check and was dropped before +// ever reaching a browser client -- a real bug an e2e run caught (the +// sidebar badge, which has no polling fallback unlike ReviewView's own +// 2s interval, stayed stuck on a stale count). Every listener still +// ignores evt.data and just refetches; this only has to satisfy the +// registered type, not carry real content. +func emitMCPWriteApprovalChanged() { + app := application.Get() + if app == nil { + return + } + app.Event.Emit("mcp-write-approval", MCPWriteRequest{}) +} + +// expiredWrite snapshots the fields emitMCPWriteActivity needs for one +// record sweepLocked just expired -- captured while writesMu is still +// held (same read-before-unlock discipline as ResolveMCPWrite/ +// CancelMCPWrite's own local variables), since the record itself may be +// deleted by a later sweep before the caller gets around to emitting. +type expiredWrite struct { + Description string + ToolName string + ArgsJSON string + Error string +} + +// sweepLocked lazily transitions any pending record whose 24h window +// has elapsed to expired, and deletes any terminal record whose own +// 24h-since-resolution retention has elapsed -- caller must hold +// writesMu. Returns a snapshot of every record that just expired, for +// the caller to push an Activity row (and the pending-changed signal, +// docs/goals/0026 item 5) for once unlocked. +func (m *MillMCPService) sweepLocked(now time.Time) []expiredWrite { + var expired []expiredWrite + changed := false + for id, rec := range m.writes { + if rec.Status == MCPWriteStatusPending && now.Sub(rec.CreatedAt) > mcpWriteExpiry { + rec.Status = MCPWriteStatusExpired + rec.Error = "no human decision within 24h" + rec.ResolvedAt = &now + expired = append(expired, expiredWrite{ + Description: rec.Description, ToolName: rec.ToolName, ArgsJSON: rec.ArgsJSON, Error: rec.Error, + }) + changed = true + continue + } + if rec.Status != MCPWriteStatusPending && rec.ResolvedAt != nil && now.Sub(*rec.ResolvedAt) > mcpWriteExpiry { + delete(m.writes, id) + changed = true + } + } + if changed { + // Incidental bookkeeping riding along a read call (PendingMCPWrites/ + // writeStatus/ResolveMCPWrite/loadWrites all call this) -- log-only, + // same fire-and-forget treatment as the compositionsvc/configuresvc + // top-up-seeding sweeps (docs/goals/0025 item 1). A failure here + // just means the same records get swept again on the next call. + if err := m.persistWritesLocked(); err != nil { + slog.Error("failed to persist MCP write sweep", "error", err) + } + } + return expired +} + +// emitExpired pushes an Activity row for every record sweepLocked just +// expired, and -- the docs/goals/0026 item 5 bug fix's other half -- +// pings the pending-changed signal exactly once if anything expired, so +// the sidebar badge/queue counts drop even when nobody happened to call +// ResolveMCPWrite/CancelMCPWrite around the same sweep. +func (m *MillMCPService) emitExpired(expired []expiredWrite) { + for _, e := range expired { + emitMCPWriteActivity(e.Description, string(MCPWriteStatusExpired), e.ToolName, e.ArgsJSON, e.Error) + } + if len(expired) > 0 { + emitMCPWriteApprovalChanged() + } +} + +// PendingMCPWrites lists writes currently awaiting a human decision, +// oldest first -- sweeps lazily first so a since-expired record never +// shows up as still pending. +func (m *MillMCPService) PendingMCPWrites() []MCPWriteRequest { + m.writesMu.Lock() + expired := m.sweepLocked(time.Now()) + out := make([]MCPWriteRequest, 0, len(m.writes)) + for _, rec := range m.writes { + if rec.Status == MCPWriteStatusPending { + out = append(out, MCPWriteRequest{ID: rec.ID, Description: rec.Description, CreatedAt: rec.CreatedAt, LastPolledAt: rec.LastPolledAt}) + } + } + m.writesMu.Unlock() + m.emitExpired(expired) + + sort.Slice(out, func(i, j int) bool { return out[i].CreatedAt.Before(out[j].CreatedAt) }) + return out +} + +// ResolvedMCPWrites lists every already-resolved write still in the 24h +// retention window (docs/goals/0026 item 6) -- newest-resolved first, +// the same "queue's own history must include write resolutions" +// Review's Recently-resolved section merges alongside RunSummary's own +// resolved runs. Sweeps lazily first, same as every other read here. +func (m *MillMCPService) ResolvedMCPWrites() []MCPWriteResolved { + m.writesMu.Lock() + expired := m.sweepLocked(time.Now()) + out := make([]MCPWriteResolved, 0) + for _, rec := range m.writes { + if rec.Status == MCPWriteStatusPending || rec.ResolvedAt == nil { + continue + } + out = append(out, MCPWriteResolved{ + ID: rec.ID, Description: rec.Description, Status: string(rec.Status), + Error: rec.Error, CreatedAt: rec.CreatedAt, ResolvedAt: *rec.ResolvedAt, + }) + } + m.writesMu.Unlock() + m.emitExpired(expired) + + sort.Slice(out, func(i, j int) bool { return out[i].ResolvedAt.After(out[j].ResolvedAt) }) + return out +} + +// checkWriteStatusArgs/Result back the check_write_status tool +// (registered in millmcpservice_tools.go's registerTools). +type checkWriteStatusArgs struct { + ID string `json:"id" jsonschema:"the pending write's id, from a gated write tool's 'parked pending human approval' response text"` +} + +type checkWriteStatusResult struct { + Status string `json:"status"` + Description string `json:"description,omitempty"` + Result string `json:"result,omitempty"` + Error string `json:"error,omitempty"` +} + +// DebugBackdatePendingWrite is an e2e-only test knob (docs/goals/0026 +// item 2's staleness presentation: "inject via an internal test knob or +// backdate CreatedAt through the store fixture"). Editing the settings +// file directly from outside a running server doesn't work here -- the +// server only ever reads it once at startup (loadWrites) and treats +// m.writes as the live source of truth from then on, so an external +// file edit would simply be overwritten by the next persist -- this is +// the in-process equivalent, shifting an already-parked record's +// CreatedAt back so a real 15m/24h age-tier render can be exercised +// without sleeping real minutes/hours. Gated by the caller +// (SettingsService.DebugBackdatePendingMCPWrite) to isolated test data +// only -- never reachable against a real production instance. +func (m *MillMCPService) DebugBackdatePendingWrite(id string, ageMinutes int) error { + m.writesMu.Lock() + rec, ok := m.writes[id] + if !ok { + m.writesMu.Unlock() + return fmt.Errorf("no MCP write with id %s", id) + } + if rec.Status != MCPWriteStatusPending { + m.writesMu.Unlock() + return fmt.Errorf("MCP write %s is not pending", id) + } + rec.CreatedAt = time.Now().Add(-time.Duration(ageMinutes) * time.Minute) + err := m.persistWritesLocked() + m.writesMu.Unlock() + // Every poll-less surface (the banner, the floating approval prompt) + // only refreshes on this event -- without it, a backdated write + // would render stale until something else happened to trigger a + // refetch (ReviewView's own 2s poll would mask that, same class of + // masking item 5's bug fix already caught once). + emitMCPWriteApprovalChanged() + return err +} + +// writeStatus sweeps lazily, then returns a snapshot of rec's outcome +// fields -- used by check_write_status. Also records LastPolledAt +// (docs/goals/0026 item 3) -- check_write_status IS the requester's own +// heartbeat, so every real poll (a hit, not a miss) updates it and +// persists, same as every other lifecycle mutation in this file. +func (m *MillMCPService) writeStatus(id string) (checkWriteStatusResult, bool) { + m.writesMu.Lock() + expired := m.sweepLocked(time.Now()) + rec, ok := m.writes[id] + var res checkWriteStatusResult + if ok { + now := time.Now() + rec.LastPolledAt = &now + if err := m.persistWritesLocked(); err != nil { + slog.Error("failed to persist MCP write poll timestamp", "error", err) + } + res = checkWriteStatusResult{Status: string(rec.Status), Description: rec.Description, Result: rec.ResultText, Error: rec.Error} + } + m.writesMu.Unlock() + m.emitExpired(expired) + return res, ok +} diff --git a/internal/services/mcpsvc/millmcpservice_cancel_test.go b/internal/services/mcpsvc/millmcpservice_cancel_test.go new file mode 100644 index 00000000..587f35ae --- /dev/null +++ b/internal/services/mcpsvc/millmcpservice_cancel_test.go @@ -0,0 +1,91 @@ +package mcpsvc + +// docs/goals/0026 item 1: cancel_write is the missing fourth verb +// (park/poll/resolve/WITHDRAW) -- the requesting client withdraws its +// OWN still-pending write, ungated, a DISTINCT outcome from denied. In +// its own file (not millmcpservice_approval_test.go, which is already +// at the 500-line convention) but reusing that file's +// mcpApprovalHarness/awaitPending helpers verbatim -- same package, +// same shared setup every park-and-poll test needs. + +import ( + "encoding/json" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" +) + +// Proven end to end via a real MCP client, the same shape +// TestMCPWriteTools_PerWriteApproval already uses: park an import, +// cancel it via cancel_write, confirm the original call errors out +// (never gets the write it asked for), nothing was written, +// check_write_status reports "cancelled" (not "denied"), the pending +// queue (the same data ReviewView/MCPWriteApprovals/the sidebar badge +// all read) drops to zero, and a second cancel of the same id errors +// (at-most-once). +func TestMCPWriteTools_CancelWrite_WithdrawsOwnPendingWrite(t *testing.T) { + h := newMCPApprovalHarness(t, "127.0.0.1:18095", "Cancel write test workflow") + + done := h.callImport(t) + pending := h.awaitPending(t) + + cancelRes, err := h.session.CallTool(h.ctx, &mcp.CallToolParams{Name: "cancel_write", Arguments: map[string]any{"id": pending.ID}}) + if err != nil || cancelRes.IsError { + t.Fatalf("cancel_write: err=%v res=%+v", err, cancelRes) + } + + // The original import call must resolve as an error result (it + // never gets the write it asked for), not hang or silently succeed. + out := <-done + if out.err != nil { + t.Fatalf("CallTool transport error: %v", out.err) + } + if !out.res.IsError { + t.Fatal("a cancelled import must return an error result to the original caller") + } + if len(h.comp.Workflows()) != h.before { + t.Fatal("a cancelled write must write nothing") + } + + statusRes, err := h.session.CallTool(h.ctx, &mcp.CallToolParams{Name: "check_write_status", Arguments: map[string]any{"id": pending.ID}}) + if err != nil || statusRes.IsError { + t.Fatalf("check_write_status: err=%v res=%+v", err, statusRes) + } + var status checkWriteStatusResult + if err := json.Unmarshal([]byte(statusRes.Content[0].(*mcp.TextContent).Text), &status); err != nil { + t.Fatalf("check_write_status result is not the typed JSON: %v", err) + } + if status.Status != string(MCPWriteStatusCancelled) { + t.Fatalf("status = %+v, want cancelled (distinct from denied)", status) + } + + if got := h.svc.PendingMCPWrites(); len(got) != 0 { + t.Fatalf("PendingMCPWrites after cancel = %+v, want empty (the same data Review/the sidebar badge read)", got) + } + + // At-most-once: cancelling an already-resolved write a second time + // must error, not silently no-op or re-signal anything. + secondCancel, err := h.session.CallTool(h.ctx, &mcp.CallToolParams{Name: "cancel_write", Arguments: map[string]any{"id": pending.ID}}) + if err != nil { + t.Fatalf("second cancel_write transport error: %v", err) + } + if !secondCancel.IsError { + t.Fatal("cancelling an already-resolved write a second time must error") + } +} + +// Cancelling something that was never pending in the first place (a +// bogus/expired id) errors cleanly rather than panicking or +// succeeding -- the "no such write" branch CancelMCPWrite shares with +// ResolveMCPWrite. +func TestMCPWriteTools_CancelWrite_UnknownID_Errors(t *testing.T) { + h := newMCPApprovalHarness(t, "127.0.0.1:18096", "Cancel write unknown-id workflow") + + res, err := h.session.CallTool(h.ctx, &mcp.CallToolParams{Name: "cancel_write", Arguments: map[string]any{"id": "does-not-exist"}}) + if err != nil { + t.Fatalf("cancel_write transport error: %v", err) + } + if !res.IsError { + t.Fatal("cancel_write on an unknown id must return an error result") + } +} diff --git a/internal/services/mcpsvc/millmcpservice_tools.go b/internal/services/mcpsvc/millmcpservice_tools.go index 89c148f8..86c7b05b 100644 --- a/internal/services/mcpsvc/millmcpservice_tools.go +++ b/internal/services/mcpsvc/millmcpservice_tools.go @@ -270,4 +270,17 @@ func (m *MillMCPService) registerTools() { } return textResult(text), nil, nil }) + + mcp.AddTool(m.server, &mcp.Tool{ + Name: "cancel_write", + Description: "Withdraw your OWN still-pending gated write by id (the id from a 'parked pending human " + + "approval' response). Cancelled is a distinct outcome from denied -- use this when the write no " + + "longer matters, not when you expect a human to say no. Ungated: cancelling never needs approval, " + + "it only ever reduces pending work. Errors if the write was already resolved or doesn't exist.", + }, func(_ context.Context, _ *mcp.CallToolRequest, in checkWriteStatusArgs) (*mcp.CallToolResult, any, error) { + if err := m.CancelMCPWrite(in.ID); err != nil { + return nil, nil, err + } + return textResult(fmt.Sprintf("cancelled write %s -- nothing was written", in.ID)), nil, nil + }) } diff --git a/internal/services/settingssvc/settingsservice.go b/internal/services/settingssvc/settingsservice.go index f71cdba0..fe6ac782 100644 --- a/internal/services/settingssvc/settingsservice.go +++ b/internal/services/settingssvc/settingsservice.go @@ -416,3 +416,30 @@ func (s *SettingsService) ResolveMCPWrite(id string, approve bool) error { } return s.mcpService.ResolveMCPWrite(id, approve) } + +// ResolvedMCPWrites lists every already-resolved MCP write still in its +// 24h retention window (docs/goals/0026 item 6) -- Review's +// Recently-resolved section reads this alongside its own resolved runs. +func (s *SettingsService) ResolvedMCPWrites() []mcpsvc.MCPWriteResolved { + if s.mcpService == nil { + return nil + } + return s.mcpService.ResolvedMCPWrites() +} + +// DebugBackdatePendingMCPWrite is an e2e-only test knob (docs/goals/0026 +// item 2's staleness presentation) -- see MillMCPService. +// DebugBackdatePendingWrite's own doc comment for why this has to be an +// in-process call rather than an external settings-file edit. Refuses +// outside isolated test data (the same IsIsolatedData signal every +// e2e run already sets via MILL_SETTINGS_PATH) -- never reachable +// against a real production instance. +func (s *SettingsService) DebugBackdatePendingMCPWrite(id string, ageMinutes int) error { + if !s.isolatedData { + return fmt.Errorf("debug test knobs are only available against isolated test data") + } + if s.mcpService == nil { + return fmt.Errorf("MCP service not running") + } + return s.mcpService.DebugBackdatePendingWrite(id, ageMinutes) +}