Merge pull request #8 from alicoding/goal/0023-attention-escalation

Attention escalation: idle-aware presence, floating approval prompt, cross-device forward (goal 0023)

Author: alicoding

frontend/bindings/github.com/alicoding/mill/internal/services/guardrailsvc/guardrailservice.ts

@@ -24,15 +24,23 @@ import * as guardrail$0 from "../../domain/guardrail/models.js";
 import * as $models from "./models.js";
 
 /**
- * CreateRule validates and stores a new rule, minting its ID.
+ * CreateRule validates and stores a new rule, minting its ID. On a
+ * persist failure the appended rule is rolled back rather than left
+ * live in memory only -- a rule that silently failed to save must not
+ * appear to be gating anything (docs/goals/0025 item 2's memory-vs-
+ * store consistency rule, applied to guardrail rules too since a
+ * phantom-saved rule here is worse than most: it's the thing deciding
+ * whether a step needs approval).
  */
 export function CreateRule(rule: guardrail$0.Rule): $CancellablePromise<guardrail$0.Rule> {
     return $Call.ByID(3694735496, rule);
 }
 
 /**
  * DeleteRule removes a rule by ID; deleting an absent rule is a no-op,
- * matching every other Configure entity's delete semantics.
+ * matching every other Configure entity's delete semantics. Returns the
+ * persist error (rather than swallowing it, docs/goals/0025 item 1) and
+ * restores the deleted rule if the store write fails.
  */
 export function DeleteRule(id: string): $CancellablePromise<void> {
     return $Call.ByID(1475597571, id);
@@ -58,7 +66,7 @@ export function TestRules(workflowID: string, nodeID: string): $CancellablePromi
 
 /**
  * UpdateRule replaces an existing rule in place, same validation as
- * create.
+ * create; rolls back to the previous rule value if the persist fails.
  */
 export function UpdateRule(rule: guardrail$0.Rule): $CancellablePromise<void> {
     return $Call.ByID(2711428157, rule);

frontend/bindings/github.com/alicoding/mill/internal/services/settingssvc/settingsservice.ts

@@ -54,12 +54,27 @@ export function CheckForUpdates(): $CancellablePromise<$models.UpdateCheckResult
 
 /**
  * ClearKeybinding removes commandID's override, if any, reverting it to
- * its frontend-declared default binding.
+ * its frontend-declared default binding. Returns the persist error
+ * (docs/goals/0025 item 1) rather than swallowing it, and restores the
+ * removed override in memory if the save failed, so a user who thinks
+ * they cleared a binding doesn't have it silently come back after a
+ * restart while the UI shows it as already cleared.
  */
 export function ClearKeybinding(commandID: string): $CancellablePromise<void> {
     return $Call.ByID(3036020909, commandID);
 }
 
+/**
+ * DismissApprovalPrompt hides the floating approval prompt and applies
+ * the same focus-yield mitigation DismissPanel already uses -- called
+ * by the prompt's own frontend once the last pending item resolves (or
+ * there's nothing left to show on mount), the DismissPanel-equivalent
+ * RPC for this window.
+ */
+export function DismissApprovalPrompt(): $CancellablePromise<void> {
+    return $Call.ByID(2270399157);
+}
+
 /**
  * DismissPanel hides the Quick Panel and applies the focus-yield
  * mitigation -- the bound RPC the panel's own frontend calls after a
@@ -71,6 +86,30 @@ export function DismissPanel(): $CancellablePromise<void> {
     return $Call.ByID(3877156882);
 }
 
+/**
+ * ForwardPendingApproval fires the configured cross-device forward for
+ * one new pending item -- fire-and-forget: it returns immediately, and
+ * a delivery failure is only slog-logged, never surfaced back to the
+ * caller or allowed to block anything. App.tsx's own per-new-item loop
+ * calls this alongside NotifyPendingApproval, unconditionally --
+ * unlike the presence gate, forwarding doesn't depend on local
+ * focus/idle at all, since the whole point is reaching the owner when
+ * there may be no local Mac attention to gate on in the first place.
+ */
+export function ForwardPendingApproval(id: string, description: string, kind: string): $CancellablePromise<void> {
+    return $Call.ByID(4270514639, id, description, kind);
+}
+
+/**
+ * GetAttentionIdleThreshold returns the configured idle-seconds
+ * threshold: the presence gate below treats the user as away once
+ * idletime.Seconds() reaches this, even while the window is focused --
+ * the fix for the focused-but-idle suppression case.
+ */
+export function GetAttentionIdleThreshold(): $CancellablePromise<number> {
+    return $Call.ByID(420254759);
+}
+
 /**
  * GetBuildInfo reports which commit this running instance was actually
  * built from (settingsservice_buildinfo.go) -- surfaced in the footer
@@ -82,6 +121,21 @@ export function GetBuildInfo(): $CancellablePromise<$models.BuildInfo> {
     return $Call.ByID(2673585232);
 }
 
+/**
+ * GetForwardApprovalsEnabled reports whether the forward is armed.
+ */
+export function GetForwardApprovalsEnabled(): $CancellablePromise<boolean> {
+    return $Call.ByID(3422319504);
+}
+
+/**
+ * GetForwardApprovalsRequestID returns the configured HTTPRequest's ID
+ * (empty means unconfigured).
+ */
+export function GetForwardApprovalsRequestID(): $CancellablePromise<string> {
+    return $Call.ByID(1205123449);
+}
+
 /**
  * GetLaunchAtLogin queries the real OS state (System Events' login
  * items list) rather than a persisted preference -- authoritative even
@@ -169,16 +223,23 @@ export function ListWorkflowMinutesSaved(): $CancellablePromise<{ [_ in string]?
 }
 
 /**
- * NotifyPendingApproval sends an actionable OS notification for a new
- * pending item (docs/adr/0032 §3). kind "mcp-write" gets Approve/Deny
- * action buttons resolving directly via ResolveMCPWrite; any other kind
- * (a guardrail/human-review park) gets a plain notification whose
- * default click shows+focuses the window instead -- typed input may be
- * required to resolve those, so blind approval from a notification
- * isn't offered.
+ * NotifyPendingApproval sends an actionable OS notification AND shows
+ * the floating approval prompt (docs/goals/0023 item 1) for a new
+ * pending item (docs/adr/0032 §3), but ONLY when isAway(focused) says
+ * the user is away -- the single decision point both surfaces share, so
+ * "notify" and "show the floating prompt" can never disagree about
+ * presence. focused is the caller's own document.hasFocus() reading
+ * (App.tsx) -- only the browser context knows that; everything else
+ * about presence (idle time, the threshold) is resolved in isAway.
+ * 
+ * kind "mcp-write" gets Approve/Deny action buttons resolving directly
+ * via ResolveMCPWrite; any other kind (a guardrail/human-review park)
+ * gets a plain notification whose default click shows+focuses the main
+ * window instead -- typed input may be required to resolve those, so
+ * blind approval from a notification isn't offered.
  */
-export function NotifyPendingApproval(id: string, description: string, kind: string): $CancellablePromise<void> {
-    return $Call.ByID(99139683, id, description, kind);
+export function NotifyPendingApproval(id: string, description: string, kind: string, focused: boolean): $CancellablePromise<void> {
+    return $Call.ByID(99139683, id, description, kind, focused);
 }
 
 /**
@@ -243,6 +304,36 @@ export function RestoreSummonHotkey(): $CancellablePromise<void> {
     return $Call.ByID(442997791);
 }
 
+/**
+ * SetAttentionIdleThreshold persists seconds (a non-positive value
+ * resets to the default, mirroring a cleared Settings field rather than
+ * persisting a nonsensical threshold), returning the persist error --
+ * same reasoning as every other settings toggle in this package: a
+ * save that silently didn't take effect leaves the user believing a
+ * threshold applies when it doesn't.
+ */
+export function SetAttentionIdleThreshold(seconds: number): $CancellablePromise<void> {
+    return $Call.ByID(454955395, seconds);
+}
+
+/**
+ * SetForwardApprovalsEnabled persists the toggle, returning the persist
+ * error (same reasoning as every other settings toggle here: a save
+ * that silently didn't take effect leaves the user believing the
+ * forward is armed when it isn't, or vice versa).
+ */
+export function SetForwardApprovalsEnabled(enabled: boolean): $CancellablePromise<void> {
+    return $Call.ByID(1952175932, enabled);
+}
+
+/**
+ * SetForwardApprovalsRequestID persists which Configure-authored
+ * HTTPRequest to forward pending-approval events through.
+ */
+export function SetForwardApprovalsRequestID(id: string): $CancellablePromise<void> {
+    return $Call.ByID(60838469, id);
+}
+
 /**
  * SetKeybinding overrides commandID's binding to mods+key, rejecting a
  * combo already claimed by another command's override, or by a
@@ -265,10 +356,24 @@ export function SetLaunchAtLogin(enabled: boolean): $CancellablePromise<void> {
     return $Call.ByID(2080722787, enabled);
 }
 
+/**
+ * SetMCPWriteApprovalRequired returns the persist error, same reasoning
+ * as SetMCPWriteEnabled -- relaxing this to "unattended" (required =
+ * false) failing to save silently is a security-relevant gap;
+ * re-tightening it (required = true) failing to save silently would be
+ * worse (the user believes approval is required again when it isn't).
+ */
 export function SetMCPWriteApprovalRequired(required: boolean): $CancellablePromise<void> {
     return $Call.ByID(4059084081, required);
 }
 
+/**
+ * SetMCPWriteEnabled returns the persist error (docs/goals/0025 item 1)
+ * rather than swallowing it -- this is a security-relevant toggle
+ * (whether an external MCP client may write to this instance at all);
+ * a click that silently failed to take effect is exactly the kind of
+ * gap §8's fail-safe posture exists to prevent.
+ */
 export function SetMCPWriteEnabled(enabled: boolean): $CancellablePromise<void> {
     return $Call.ByID(213255560, enabled);
 }
@@ -361,6 +466,9 @@ export function SuspendMenuAccelerators(): $CancellablePromise<void> {
 
 /**
  * UnassignSummonHotkey removes the app-level summon hotkey, if any.
+ * Returns the persist error (docs/goals/0025 item 1) rather than
+ * swallowing it, restoring the in-memory record on failure so it
+ * matches what's still on disk.
  */
 export function UnassignSummonHotkey(): $CancellablePromise<void> {
     return $Call.ByID(2578647287);

frontend/e2e/approval-prompt.spec.ts

@@ -0,0 +1,102 @@
+import type { Page } from '@playwright/test'
+import type { Client } from '@modelcontextprotocol/sdk/client/index.js'
+import { test, expect } from './fixtures/server'
+import { clickRowAction } from './inventoryRow'
+import {
+  connectMCPClient, exportWorkflowViaMCP, findWorkflowIdByLabel,
+  enableMCPWritesWithApprovalRequired, restoreMCPWriteDefaults,
+} from './mcpTestClient'
+
+// Exercises the floating approval prompt's frontend at its hash route
+// (/#/approvalprompt, docs/goals/0023-attention-escalation.md item 1,
+// app/ApprovalPrompt.tsx) over real Go bindings (Wails3 server mode) --
+// same "the route itself is fully e2e-able headlessly" reasoning
+// quick-panel.spec.ts already established for the sibling second
+// window (ADR-0033's mechanism, reused here). What's NOT headlessly
+// verifiable is the WINDOW-LEVEL behavior around it (floating window
+// level, the backend-triggered Show() on an away verdict, Escape via
+// the native HideOnEscape option, the focus-yield mitigation) -- those
+// belong in the manual-only registry (.claude/skills/run-mill/SKILL.md)
+// per .claude/rules/testing.md's own "manual-only registry... never
+// silently absent" requirement.
+
+function workflowRow(page: Page, label: string) {
+  return page.locator('[data-testid="inventory-row"][data-entity="workflow"]', { has: page.getByText(label, { exact: true }) })
+}
+
+test('the approval prompt route renders standalone and shows nothing pending', async ({ page }) => {
+  await page.goto('/#/approvalprompt')
+
+  const prompt = page.getByTestId('approval-prompt')
+  await expect(prompt).toBeVisible()
+  await expect(page.getByTestId('approval-prompt-empty')).toBeVisible()
+
+  // No sidebar/PageLayout chrome -- this is the dedicated minimal shell
+  // (ApprovalPromptApp), not <App/>'s tree.
+  await expect(page.getByRole('link', { name: 'Workflows' })).toHaveCount(0)
+})
+
+test('a parked MCP write shows in the approval prompt, and Approve resolves it', async ({ page }, testInfo) => {
+  await enableMCPWritesWithApprovalRequired(page)
+
+  // A source workflow to export/re-import (import always mints a new
+  // ID -- the exact gated write this lifecycle protects).
+  await page.getByRole('link', { name: 'Workflows' }).click()
+  await page.getByTestId('new-workflow').click()
+  await page.locator('[role="tabpanel"]:not([hidden])').last().getByLabel('Label').fill('E2E approval prompt source')
+  await page.locator('[role="tabpanel"]:not([hidden])').last().getByTestId('save-workflow').click()
+  await expect(workflowRow(page, 'E2E approval prompt source')).toBeVisible()
+
+  const client = await connectMCPClient(testInfo.parallelIndex)
+  let importResultPromise: ReturnType<Client['callTool']>
+  try {
+    const sourceId = await findWorkflowIdByLabel(client, 'E2E approval prompt source')
+    const exported = await exportWorkflowViaMCP(client, sourceId)
+
+    // Fire the gated import -- parks as a durable pending record
+    // (docs/adr/0032), independent of whether this specific browser
+    // page happens to be watching for it.
+    importResultPromise = client.callTool({ name: 'import_workflow', arguments: { json: exported } })
+
+    // A fresh cross-document navigation to the approval prompt's own
+    // route (not a same-document hash change from an already-loaded
+    // page, same discipline quick-panel.spec.ts's own run-target test
+    // uses) picks up the already-parked write on its first fetch.
+    await page.goto('about:blank')
+    await page.goto('/#/approvalprompt')
+
+    const description = page.getByTestId('approval-prompt-description')
+    await expect(description).toBeVisible({ timeout: 15_000 })
+    await expect(description).toContainText('import a workflow')
+
+    await page.getByTestId('approval-prompt-approve').click()
+
+    // Resolved -- the prompt goes back to its empty state (no more
+    // pending items), which is also what drives the real window's own
+    // auto-hide (DismissApprovalPrompt).
+    await expect(page.getByTestId('approval-prompt-empty')).toBeVisible({ timeout: 10_000 })
+
+    // The write actually executed: a second workflow with the same
+    // label now exists (import always mints a new ID, never overwrites).
+    await page.goto('/')
+    await page.getByRole('link', { name: 'Workflows' }).click()
+    await expect(workflowRow(page, 'E2E approval prompt source')).toHaveCount(2, { timeout: 10_000 })
+
+    const result = await importResultPromise
+    if (result.isError) {
+      throw new Error(`import_workflow ultimately errored after approval: ${JSON.stringify(result.content)}`)
+    }
+  } finally {
+    await client.close()
+  }
+
+  // Cleanup: both minted workflows, one at a time (the DOM shifts as
+  // each row disappears), and the settings toggle.
+  let remaining = await workflowRow(page, 'E2E approval prompt source').count()
+  while (remaining > 0) {
+    await clickRowAction(page, workflowRow(page, 'E2E approval prompt source').first(), 'Delete')
+    remaining -= 1
+    await expect(workflowRow(page, 'E2E approval prompt source')).toHaveCount(remaining)
+  }
+  await restoreMCPWriteDefaults(page)
+})

frontend/e2e/mcpTestClient.ts

@@ -0,0 +1,74 @@
+import type { Page } from '@playwright/test'
+import { Client } from '@modelcontextprotocol/sdk/client/index.js'
+import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'
+import { MCP_BASE_PORT, expect } from './fixtures/server'
+
+// Shared e2e helpers for driving Mill's real MCP server over HTTP
+// (docs/adr/0025/0032) -- factored out of mcp-write-approval.spec.ts so
+// a second spec needing a parked MCP write (the Review kind filter,
+// docs/goals/0002 item 4) doesn't hand-roll a second copy. Not a
+// *.spec.ts file itself -- Playwright only picks up files matching its
+// own testMatch glob, same "plain helper module" shape as
+// inventoryRow.ts.
+
+export async function connectMCPClient(workerIndex: number): Promise<Client> {
+  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}`))
+  await client.connect(transport)
+  return client
+}
+
+interface WorkflowIndexEntry {
+  id: string
+  label: string
+}
+
+export async function findWorkflowIdByLabel(client: Client, label: string): Promise<string> {
+  const result = await client.readResource({ uri: 'mill://workflows' })
+  const first = result.contents[0]
+  const text = 'text' in first ? first.text : ''
+  const list = JSON.parse(text as string) as WorkflowIndexEntry[]
+  const found = list.find((w) => w.label === label)
+  if (!found) throw new Error(`workflow "${label}" not found in mill://workflows: ${text}`)
+  return found.id
+}
+
+export async function exportWorkflowViaMCP(client: Client, id: string): Promise<string> {
+  const result = await client.callTool({ name: 'export_workflow', arguments: { id } })
+  if (result.isError) throw new Error(`export_workflow failed: ${JSON.stringify(result.content)}`)
+  const content = result.content as Array<{ type: string; text?: string }>
+  return content[0]?.text ?? ''
+}
+
+// Writes ON, per-write approval left ON (the default) -- the shape most
+// callers need, distinct from canvas-live-sync.spec.ts's own
+// "unattended" helper which deliberately relaxes approval.
+export async function enableMCPWritesWithApprovalRequired(page: Page): Promise<void> {
+  await page.goto('/')
+  await page.getByRole('button', { name: 'Settings' }).click()
+  const writeCheckbox = page.getByTestId('mcp-write-enabled-checkbox')
+  await expect(writeCheckbox).toBeEnabled()
+  if (!(await writeCheckbox.isChecked())) {
+    await writeCheckbox.click()
+    await expect(writeCheckbox).toBeChecked()
+  }
+  const approvalCheckbox = page.getByTestId('mcp-write-approval-checkbox')
+  await expect(approvalCheckbox).toBeEnabled()
+  if (!(await approvalCheckbox.isChecked())) {
+    await approvalCheckbox.click()
+    await expect(approvalCheckbox).toBeChecked()
+  }
+}
+
+// Leaves the shared e2e settings file the way a fresh install would
+// look (.claude/rules/testing.md's cleanup discipline): write gate off,
+// approval required stays on either way.
+export async function restoreMCPWriteDefaults(page: Page): Promise<void> {
+  await page.goto('/')
+  await page.getByRole('button', { name: 'Settings' }).click()
+  const writeCheckbox = page.getByTestId('mcp-write-enabled-checkbox')
+  if (await writeCheckbox.isChecked()) {
+    await writeCheckbox.click()
+    await expect(writeCheckbox).not.toBeChecked()
+  }
+}