fix: MCP tool identifier aliases + real (non-test) run kind (goal 0021 phase 3) (#54)

Closes the two interop gaps the orchestrator's live Phase-2 MCP probe
found: inconsistent identifier argument names across the tool surface
(id vs. runId vs. workflowId), and run_workflow always landing "test"
kind with no way to opt out, silently excluding real agent-triggered
production runs from Home's automation metrics.

- Every workflow-identifying tool (run_workflow/run_workflow_stepped/
  update_workflow/publish_workflow/delete_workflow/export_workflow)
  now accepts a canonical `workflowId` alongside its original `id`,
  resolved through one shared workflowIDArgs.resolve() helper --
  backward compatible, canonical name listed first in the schema.
- run_workflow gains an optional `test` boolean (default false): false
  lands a new RunKindMCP kind, counted in Home's metrics like a real
  trigger fire; true lands RunKindTest, excluded by default like the
  UI's Test-run button. Both still execute the current draft head --
  only the metrics classification changes. run_workflow_stepped stays
  unconditionally test-kind (a debug surface, never production
  automation).
- Proven by three real-MCP-client Go tests (alias resolution across
  the surface, legacy id backward compat, test-flag controlling both
  RunKind and HomeMetrics visibility).


Claude-Session: https://claude.ai/code/session_018pkViCNAuZp2vBv2K9AbUh

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: alicoding

docs/SPEC.md

@@ -2449,9 +2449,10 @@ turns out to solve this without touching that dispute).
   per-write approval with a diff summary — and every update
   **auto-snapshots the previous draft as a version first**, so any
   LLM change is one load-into-draft from undone), and execution
-  (`run_workflow`, test kind — the guardrail engine is the run's own
-  approval layer; external steps park in the Review queue regardless
-  of who started the run). Every mutation emits a `mill-data-changed`
+  (`run_workflow`, real (`mcp`) kind by default since goal 0021 Phase 3,
+  `test:true` opts into `test` kind — the guardrail engine is the run's
+  own approval layer; external steps park in the Review queue
+  regardless of who started the run). Every mutation emits a `mill-data-changed`
   event the open window's stores refresh on — the §1 thesis running
   in both directions, live. **Extended 2026-08-11 to the open canvas
   itself (goal 0021 phase 2: "see the canvas update while the MCP
@@ -2517,6 +2518,20 @@ 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`).
 
+**Two interop gaps closed 2026-08-13 (goal 0021 Phase 3), found by a
+real MCP client's own live probe, not code review**: every entity/run-
+identifying tool argument now accepts both its original name and an
+explicit canonical one (`workflowId` for a workflow, `runId` for a
+run — listed first in the schema, backward compatible, resolved by one
+shared helper rather than per tool); and `run_workflow` gained a `test`
+argument (default `false`) landing a real `RunKindMCP` run counted in
+Home's automation metrics exactly like a genuine trigger fire, instead
+of always landing the metrics-excluded `test` kind with no way to opt
+out — `run_workflow_stepped` still stays `test` kind unconditionally
+(a debug/inspection surface, never production automation). Both still
+execute the current draft head; only the identifier vocabulary and the
+metrics classification changed.
+
 **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}`

docs/goals/0021-mcp-dogfood-gap-closure.md

@@ -10,9 +10,9 @@ instance, not code reading.
 
 ## Phase 1 — read/introspection surface (2026-08-11 probed, 2026-08-12
 gaps 2-4 closed — Phase 1 now fully complete: every gap either fixed or
-explicitly declined/confirmed-by-design. Phase 2/3 remain open below,
-unblocked but not yet run -- they need live interactive probing, not
-code changes.)
+explicitly declined/confirmed-by-design. Phase 2 probed and its gaps
+closed 2026-08-13 (Phase 3 below); Phase 4 (real-use-case fidelity)
+remains open.)
 
 What already works well, verified live: `list_node_types` is a real
 authoring vocabulary (full typed ConfigFields, defaults, options,
@@ -103,8 +103,10 @@ right terminal Decision.
    writes enabled") so a long-forgotten toggle isn't invisible
    standing authority. Logged as gap, LOW-MED.
 
-## Phase 2 — authoring + step-debug loop (unblocked — the writes
-toggle is already on; per-write approval still gates each import)
+## Phase 2 — authoring + step-debug loop (2026-08-13 live-probed by the
+orchestrator against a real MCP client — the planned author-from-
+scratch + full `run_workflow_stepped` session probes below; now
+complete)
 
 Planned probes: author a workflow from scratch via
 `validate_workflow`→`import_workflow`→`update_workflow`; per-write
@@ -113,7 +115,76 @@ approval UX friction; a full `run_workflow_stepped` session
 verification (the in-flight build) — owner watches the canvas while
 the MCP author works.
 
-## Phase 3 — real-use-case fidelity (partially unblocked)
+**Positive finding, proven live, not assumed:** the full author→run→
+inspect loop works end to end over a real MCP client — validate→
+import→update→run→get_run all round-tripped correctly, per-write
+approval gated each mutation as designed, and a real
+`run_workflow_stepped` session (inspect→step→resume) advanced node by
+node exactly as ADR-0031 describes. No new gap in the loop mechanics
+themselves.
+
+**Gaps found, both closed same day (Phase 3 below):**
+
+6. ~~**[MED] Inconsistent identifier argument names across the tool
+   surface**~~ — **FIXED 2026-08-13.** The live probe burned failed
+   round trips guessing argument names: `import_workflow{json}`,
+   `run_workflow{id,values,payload}`, `update_workflow{id,json}`,
+   `export_workflow{id}` all took a bare `id`, while `get_run{runId}`
+   and `list_runs{workflowId}` already used more explicit names —
+   validation errors were readable, but the inconsistency itself was
+   pure interop friction, not a real design difference across entity
+   kinds. Fixed at the argument-resolution level: a single
+   `workflowIDArgs` struct (`WorkflowID`/`ID`, canonical name first)
+   embedded by every workflow-identifying tool
+   (`run_workflow`/`run_workflow_stepped`/`update_workflow`/
+   `publish_workflow`/`delete_workflow`/`export_workflow`), with one
+   shared `resolve()` deciding which wins if a caller sends both —
+   never per-tool copy-paste. `get_run`/`list_runs`/`step_run`/
+   `resume_run`/`stop_run` already used their own canonical names
+   (`runId`/`workflowId`) before this fix. Fully backward compatible —
+   `id` keeps working unchanged. Proven by
+   `TestMCPIdentifierAliases_CanonicalNamesResolveAcrossTheSurface`
+   (every tool driven end to end using ONLY the new canonical names)
+   and `TestMCPIdentifierAliases_LegacyIdStillResolves` (the original
+   `id` name, real MCP client, real HTTP).
+7. ~~**[MED] `run_workflow` always landed `test` kind, no way to opt
+   out**~~ — **FIXED 2026-08-13.** Externally-triggered production
+   runs were silently excluded from Home's automation metrics by
+   default (`RunKindTest` is the one kind Home's Ambient/TimeSaved/
+   ErrorRate framing always excludes) — wrong for a real agent
+   invoking a workflow for real, not authoring/debugging it.
+   `run_workflow` gained an optional `test` boolean (default `false`):
+   `false` now lands a new `RunKindMCP` run kind, counted in Home's
+   metrics exactly like a genuine trigger fire; `test:true` still lands
+   `RunKindTest`, matching the UI's own Test-run button. Deliberately a
+   DISTINCT kind from `RunKindTriggered` rather than reusing it: no MCP
+   tool offers a "run the published version" choice the way a real
+   trigger fire does (ADR-0021 locks trigger/child-call execution to
+   the published snapshot) — both `test` and the new `mcp` kind still
+   execute the current DRAFT head; only the metrics classification
+   changes (`RunKind.runsDraft`/`RunKind.isTest`,
+   `internal/services/executionsvc/executionservice_runkind.go`).
+   `run_workflow_stepped` intentionally has no `test` argument at all
+   and always stays `RunKindTest` — a debug/inspection surface (pauses
+   before EVERY node), never production automation, documented as such
+   in its own tool description. Proven by
+   `TestMCPRunWorkflow_TestFlagControlsRunKindAndHomeVisibility`: the
+   default run lands `mcp` kind and counts in
+   `HomeMetrics(...).Ambient.TriggeredCount`; `test:true` lands `test`
+   kind and counts in `.Ambient.ManualCount`; `run_workflow_stepped`
+   stays `test` regardless.
+
+## Phase 3 — MCP tool surface gap closure (2026-08-13, delivered)
+
+Both Phase 2 gaps above (6, 7) fixed same day as found, full local
+suite green, PR self-merged. This closes everything actionable that
+Phase 2's live probing surfaced — no other judgment calls came up
+needing the owner (the mandate's own "Phase 3 judgments that need the
+owner surface as found" case never triggered this round).
+
+## Phase 4 — real-use-case fidelity (partially unblocked, still open —
+distinct from Phase 3 above, a different scope: fidelity/breadth work,
+not tool-surface ergonomics)
 
 - **Markdown fidelity on realistic Confluence HTML** — the actual
   daily-pain quality bar: tables, code blocks, panels/macros, nested
@@ -123,14 +194,20 @@ the MCP author works.
 - **§2.1 M365 bridge dry run** — compose capture→code-exec→clipboard
   end-to-end with the pieces that exist; name what's still missing
   (DOM capture, auto-paste target).
-- **AI node** (§3.3 map row, invariant locked) — still unbuilt; the
-  local-Ollama variant is the zero-egress win. Research pass owed
-  before building.
+- ~~**AI node** (§3.3 map row, invariant locked)~~ — **DELIVERED via a
+  separate goal, [0031 — AI node family](archive/0031-ai-node-family.md),
+  2026-08-12**, not this goal's own work; struck here so this list
+  doesn't read as still-outstanding.
 
 ## Acceptance (rolling)
 
 Each phase's gaps either fixed (with the standing proof discipline)
 or explicitly declined with a recorded reason; the phase-2/3 probes
 run and their findings logged here; this file graduates items out as
 they land, and archives when the owner calls the surface
-real-use-ready.
+real-use-ready. **Not archived yet as of Phase 3's 2026-08-13
+delivery**: Phase 4's Confluence-markdown-fidelity and M365-bridge
+items remain genuinely open, substantial, unaddressed work — this
+session's scope was Phase 2/3's tool-surface gap closure only, and the
+file's own archival bar is the owner calling the surface real-use-ready,
+not an agent's unilateral judgment that one phase's items closed.

docs/goals/BACKLOG.md

@@ -201,7 +201,7 @@ live-review material, interleaved during owner reviews, not a lane.**
 4. [x] Copy-management migration ×4 (`app/` → `composition/` → `configure/` → `views/`) then the `eslint-plugin-i18next` revisit — DELIVERED 2026-08-12, see item 10's own writeup above ([0032](archive/0032-copy-management.md)).
 5. [x] Workflow pins/favorites (tech debt, split from goal 0015's remainder 2026-08-12) — DELIVERED 2026-08-13: `pinnedWorkflowIds: string[]` + `togglePinnedWorkflow` on `shared/store.ts`'s existing zustand `persist` (same localStorage tier as `activeWorkTabKey`, goal 0033's precedent — no new Go surface). `app/workflowFrecency.ts`'s new `sortWorkflowsByPinnedAndFrecency` partitions pinned (in pin-order) above the existing frecency-sorted unpinned tail, reusing `sortWorkflowsByFrecency` rather than a second algorithm. A Primer `PinIcon` `IconButton` trailing-visual pin toggle on both the Quick Panel's and ⌘K palette's workflow rows (muted outline unpinned, accent-colored "filled" once pinned) — found and fixed a real Primer interaction bug along the way: `ActionList.Item`'s own `TrailingVisual` wraps children in a `VisualWrap` span with `pointer-events: none` (trailing visuals are decorative-only by the library's own convention), which silently ate every click on the toggle until `pointer-events: auto` was added back on the button itself. Vitest covers the pinned-above-frecency/pin-order/unpinned-id-dropped/no-mutation cases; `quick-panel.spec.ts` gained a full pin→sort→unpin→revert→reload-persists e2e case.
 6. [x] ⌘?/⌘/ multi-binding keybinding alias (tech debt, split from goal 0015's remainder 2026-08-12) — DELIVERED 2026-08-13: `Command` grew an optional `extraBindings: KeyCombo[]` alongside `defaultBinding` (`shared/commands.ts`, backward-compatible); `shared/keybinding.ts`'s `keyFromEventCode` gained `/` support (shift-independent, same as every other key — the Shift mod is what distinguishes ⌘/ from ⌘?, both on the physical Slash key). `palette.open` carries both as `extraBindings`, checked against the full registry + `RESERVED_COMBOS` first (no collision — nothing else uses `/`). `dispatchCommandForEvent` checks a command's effective (override-aware) primary plus its extras every dispatch; extras themselves are deliberately NOT override-checked this pass (Settings' recorder-based rebinding UI still edits only the primary). `views/KeyboardShortcutsSection.tsx` renders extras as read-only secondary `KeyComboChip`s next to the primary's click-to-rebind button. Vitest covers dispatch-matches-either-binding + override-doesn't-disable-extras + no-extraBindings-backward-compat; `keymap.spec.ts` gained both a live ⌘//⌘⇧+/ → palette-opens case and a Settings-renders-the-two-read-only-chips case.
-7. [ ] [0021 — MCP dogfood gap closure](0021-mcp-dogfood-gap-closure.md) Phase 2: orchestrator-driven live MCP probing against the locked-down-enterprise use cases (the mandate names the orchestrator as the prober — self-driveable, exploratory; produces the next ranked gap list). Phase 3 judgments that need the owner surface as they're found.
+7. [ ] [0021 — MCP dogfood gap closure](0021-mcp-dogfood-gap-closure.md) Phase 2 (orchestrator-driven live MCP probing) + Phase 3 (that probe's gap closure) DELIVERED 2026-08-13: the live author-from-scratch + full `run_workflow_stepped` probe found the loop mechanics themselves sound, plus two interop gaps — inconsistent identifier argument names across tools (`id` vs. `runId` vs. `workflowId`) and `run_workflow` always landing `test` kind with no way to opt out (silently excluding real agent-triggered production runs from Home's metrics). Both fixed same day: every workflow-identifying tool now accepts a canonical `workflowId` alongside its original `id` (one shared `resolve()` helper, backward compatible); `run_workflow` gained an optional `test` boolean (default `false` → new `RunKindMCP`, counted in Home's automation metrics like a real trigger fire; `test:true` → `RunKindTest`, excluded, matching the UI's Test button) — `run_workflow_stepped` stays unconditionally `test` kind (debug surface). Real-MCP-client Go tests prove both. Left UNCHECKED / not archived: the goal file's own separate Phase 4 (Confluence-markdown-fidelity fixture corpus + M365 bridge dry run) remains genuinely open, substantial, unaddressed work outside this round's scope — the file's own acceptance bar is the owner calling the whole surface real-use-ready, not one phase closing.
 8. [x] Dev-loop instance guards (tech debt, owner-hit 2026-08-12 evening: THREE concurrent `mill.dev.app` instances in the dock, real crash risk on the 16GB machine) — DELIVERED 2026-08-13, both mechanical fixes landed: (a) per-rebuild reap — `build/config.yml`'s `dev_mode.executes` gained a `type: blocking` `pkill -f "bin/mill.dev.app/Contents/MacOS/mill" || true` step right before the `primary` `wails3 task run` step (blocking steps re-run every reload cycle, confirmed directly against the vendored `github.com/atterpac/refresh` engine source — its own `Primary` case already SHOULD kill-then-restart via a process-group SIGKILL, but a live `task dev` session running during this item's own investigation was caught red-handed with two concurrent `mill.dev.app` processes, one orphaned into a foreign process group refresh's own tracking never reaped; root cause not fully pinned to one line since it's in a vendored third-party dependency, so this reap is an independent, pattern-based backstop rather than a patch to code this repo doesn't own — same shape Taskfile.yml's own start-of-session sweep already used); (b) `task dev` now refuses a second concurrent start — `internal/devguard` (a real Go package, `package main`, unit-tested: `guard_test.go` covers process-list parsing, the wails3-dev-process matcher incl. a real false-positive it caught and fixed against a differently-pathed sibling project, port-PID parsing, and message formatting) runs as the FIRST step of `Taskfile.yml`'s `dev:` task, checks for an already-running `wails3 dev` process for this exact repo, and exits non-zero naming the conflicting PID (+ any port occupancy as corroborating detail) before the destructive sweep steps can run — verified live against a genuinely running session (correctly detected + refused, naming the real PID). Manual-only registry entry added (`.claude/skills/run-mill/SKILL.md`) for what CI structurally can't prove: real per-rebuild-orphan-prevention across several live Go-triggered rebuilds, and a genuine second-terminal `task dev` invocation actually refusing to start.
 9. [ ] Dock-bounce on parked approvals (small, unlocked by wails beta.6's Flash() gaining macOS support via NSApp requestUserAttention — PR #44's changelog finding; Mill calls Flash nowhere today) — the attention stack (goal 0023/ADR-0032's away-user layer) gains a one-shot dock bounce when an approval parks while the user is away; kernel attention-layer surface per ADR-0035 (same class as the dock badge), NOT a new composition path. Tiny: one call site in the existing NotifyPendingApproval flow + manual-only registry entry (real dock behavior isn't CI-testable).
 10. [x] resizable-table.spec.ts drag-timing flake, PROPER fix (3 confirmed recurrences POST-hardening: PR #24 original, #43's run, #44's run — the expect.poll hardening from PR #33's wave was insufficient) — DELIVERED 2026-08-13. A keyboard-based resize alternative was checked and ruled out (`shared/ResizableTable.tsx`'s drag handle is built entirely on pointerdown/pointermove/pointerup, no keyboard path at all — building one would be a real feature addition, not a test fix). Landed instead: `test.describe.configure({ mode: 'serial' })` makes the file's own never-interleave requirement explicit rather than an incidental side effect of global config; `waitForStableBoundingBox` replaces the old non-null-only `expect.poll` with a poll for the box being IDENTICAL across two consecutive reads (a non-null box mid-reflow was always possible the old check couldn't see); the synthesized drag itself moved from one batched `page.mouse.move(..., { steps: N })` call to discrete, individually-awaited moves (a browser can coalesce rapid pointermove events within one CDP command — a real, documented behavior via the `PointerEvent.getCoalescedEvents()` API, not Playwright-specific); and two more `expect.poll`s wait for the drag's actual DOM effect and the `localStorage` persist to land before the next step depends on them, instead of assuming the previous Playwright command's own resolution implies the page's JS listener already ran. Verified via 5 separate fresh `npx playwright test` invocations (10/10 passed) — an in-process `--repeat-each` stress loop on the same worker turned out to be a self-confounding methodology (accumulating browser/worker degradation unrelated to the fix, not representative of a real CI run) once cross-checked against genuinely fresh runs.