From 1ea7a63598faf276b2353db74905dd663d82c2a3 Mon Sep 17 00:00:00 2001 From: Ali Al Dallal Date: Wed, 12 Aug 2026 03:17:17 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20goal=200017=20=E2=80=94=20direct-UI/serv?= =?UTF-8?q?ice=20mutations=20now=20emit=20mill-data-changed=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (audit, 2026-08-12): only mcpsvc emitted the live-sync event, so an MCP-authored change propagated to every open surface but a plain UI create/edit/delete never did. Gives CompositionService, ConfigureService, and GuardrailService their own emit via a new shared internal/services/dataevent package (the one place EventName/Changed/ Emit are now defined, replacing mcpsvc's locally-owned copies); GuardrailService's rule CRUD introduces a new "guardrail-rule" entity, ConfigureService's decision/execenv CRUD introduce "decision"/ "execenv". Fixes App.tsx's mill-data-changed routing, which misrouted list/mcpserver to refreshRequests()+refreshWorkflows() (neither store holds either) instead of doing nothing useful at all. Frontend: a new shared/configureEntityStore.ts (mirrors store.ts's workflows/requests pattern) backs Configure's Lists/Decisions/MCP Servers/ExecEnv sections, replacing each page's own local-state fetch so a live update from another surface actually reaches an already-mounted tab; ConfigureAttributes switches to the existing shared workflows store. WorkflowRunsPanel, HomeView, and ActivityRunsExplorer subscribe to entity:"run"; CompositionView's armed-badge refresh and useGuardrailBadges subscribe to entity:"workflow"/"guardrail-rule"; ReviewView adds a guardrail-pending-changed subscription alongside its existing 2s poll (kept as the documented DBOS-has-no-per-step-event fallback). Proofs: per-service Go tests using a new dataevent.TestHook seam (application.Get() is always nil under `go test`, so this is the only way to observe an Emit call); a new e2e spec (realtime-cross-surface.spec.ts) proves the flagship two-surface scenario both directions — an MCP-authored import_list reaching an open Configure > Lists tab, and a direct-UI workflow create in one browser window reaching a canvas picker open in a second window — neither page ever reloads. docs/SPEC.md's realtime bullet gets a note on the "including Mill's own UI mutations" gap this closes. Goal file moved to archive/, BACKLOG.md updated. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01FYwojT8GdUbYSoggbvEFft --- docs/SPEC.md | 10 +- docs/goals/BACKLOG.md | 2 +- .../0017-realtime-surfaces-audit.md | 52 +++ frontend/e2e/realtime-cross-surface.spec.ts | 126 ++++++ frontend/package-lock.json | 10 +- frontend/src/app/App.tsx | 20 +- frontend/src/composition/CompositionView.tsx | 15 + .../src/composition/WorkflowRunsPanel.tsx | 17 + .../src/composition/useGuardrailBadges.ts | 14 + .../src/configure/ConfigureAttributes.tsx | 23 +- frontend/src/configure/ConfigureDecisions.tsx | 7 +- frontend/src/configure/ConfigureExecEnv.tsx | 7 +- frontend/src/configure/ConfigureLists.tsx | 10 +- .../src/configure/ConfigureMCPServers.tsx | 7 +- frontend/src/shared/configureEntityStore.ts | 71 ++++ frontend/src/views/ActivityRunsExplorer.tsx | 12 + frontend/src/views/HomeView.tsx | 14 + frontend/src/views/ReviewView.tsx | 12 +- .../compositionsvc/compositionservice.go | 9 + .../compositionservice_dataevent_test.go | 152 ++++++++ .../compositionservice_versioning.go | 6 + .../configuresvc/configuredecision.go | 4 + .../services/configuresvc/configureexecenv.go | 4 + .../services/configuresvc/configurelist.go | 368 ++++++++++++++++++ .../configuresvc/configuremcpserver.go | 4 + .../services/configuresvc/configureservice.go | 345 +--------------- .../configureservice_dataevent_test.go | 211 ++++++++++ .../configureservice_requestauth.go | 4 + internal/services/dataevent/dataevent.go | 60 +++ .../services/guardrailsvc/guardrailservice.go | 4 + .../guardrailservice_dataevent_test.go | 63 +++ .../mcpsvc/millmcpservice_authoring.go | 35 +- .../services/mcpsvc/millmcpservice_debug.go | 9 +- .../services/mcpsvc/millmcpservice_tools.go | 17 +- main.go | 3 +- 35 files changed, 1320 insertions(+), 407 deletions(-) rename docs/goals/{ => archive}/0017-realtime-surfaces-audit.md (56%) create mode 100644 frontend/e2e/realtime-cross-surface.spec.ts create mode 100644 frontend/src/shared/configureEntityStore.ts create mode 100644 internal/services/compositionsvc/compositionservice_dataevent_test.go create mode 100644 internal/services/configuresvc/configurelist.go create mode 100644 internal/services/configuresvc/configureservice_dataevent_test.go create mode 100644 internal/services/dataevent/dataevent.go create mode 100644 internal/services/guardrailsvc/guardrailservice_dataevent_test.go diff --git a/docs/SPEC.md b/docs/SPEC.md index 85e43b4a..d2a7e1fc 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -107,7 +107,15 @@ an implicit `FINAL`. ever show stale state the user would have to manually refresh?" and close that gap (goal 0017). Same family as the §1 thesis (no gap between what you see and what's real) — applied to *time*, not just - structure. + structure. **Including Mill's own UI mutations, not just external + ones**: goal 0017's audit found the event layer's emit side lived in + ONE place (`mcpsvc`, MCP-authored writes only) — a direct-UI create/ + edit/delete through `CompositionService`/`ConfigureService`/ + `GuardrailService` emitted nothing at all, so it only ever reached the + tab that made the change, never another open surface. Fixed by giving + every direct-mutation service its own `dataevent.Emit` call (one + shared package, `internal/services/dataevent`) rather than treating + MCP as the sole live-sync source. - **Scope filter, learned from the screenshot-to-clipboard tangent**: before any capability goes into Mill, check whether the OS (or an existing launcher like Alfred/Raycast) already does it simply and well. If yes, diff --git a/docs/goals/BACKLOG.md b/docs/goals/BACKLOG.md index 5adf55a5..7fc95877 100644 --- a/docs/goals/BACKLOG.md +++ b/docs/goals/BACKLOG.md @@ -133,4 +133,4 @@ live-review material, interleaved during owner reviews, not a lane.** - [x] [0006 — Trigger-aware Workflows list](0006-trigger-aware-workflows-list.md) — 2026-08-10 - [x] [0007 — Resource-inventory redesign](0007-resource-inventory-redesign.md) — 2026-08-10 (owner recognition test passed live: "like an addition") 12. [x] [0016 — Keymap system](archive/0016-keymap-system.md) — delivered 2026-08-10 (command registry, Settings rebinding, ⌘W→tab, Run=⌘↩; 127/127) -13. [ ] [0017 — Real-time surfaces audit](0017-realtime-surfaces-audit.md) (product value locked in SPEC §1: never make the user refresh; audit every surface for stale state, fix via the existing event layer) +- [x] [0017 — Real-time surfaces audit](archive/0017-realtime-surfaces-audit.md) — delivered 2026-08-12 (root cause: only mcpsvc emitted mill-data-changed; gave CompositionService/ConfigureService/GuardrailService their own dataevent.Emit, fixed App.tsx's list/mcpserver misrouting, added a lists/decisions/mcpServers/execEnvs shared store, wired run/workflow/guardrail-rule subscribers across WorkflowRunsPanel/Home/ActivityRunsExplorer/CompositionView/useGuardrailBadges/ReviewView) diff --git a/docs/goals/0017-realtime-surfaces-audit.md b/docs/goals/archive/0017-realtime-surfaces-audit.md similarity index 56% rename from docs/goals/0017-realtime-surfaces-audit.md rename to docs/goals/archive/0017-realtime-surfaces-audit.md index 4e8b06f9..ab086a7b 100644 --- a/docs/goals/0017-realtime-surfaces-audit.md +++ b/docs/goals/archive/0017-realtime-surfaces-audit.md @@ -70,3 +70,55 @@ everywhere live; watch a run complete on its Runs tab without reopening; have an MCP author change something and see it in the open window — with no manual refresh anywhere, and any remaining poll justified in writing. + +## Delivered 2026-08-12 + +All P0/P1/P2 items from the audit's fix list, implemented: + +- **P0-1** `CompositionService` now emits `mill-data-changed + {entity:"workflow"}` after Create/Update/Delete/UpdateAttributes + (compositionservice.go) and, via the shared `mutateWorkflow` choke + point, Publish/PublishExistingVersion/RestoreVersionToDraft/ + SetWorkflowDisabled/SnapshotDraft (compositionservice_versioning.go) + — `ImportWorkflow` inherits it for free (delegates to CreateWorkflow). +- **P0-2** `ConfigureService` now emits for request/list/mcpserver/ + decision (new)/execenv (new) CRUD, including List row mutations + (AddListRow/UpdateListRow/DeleteListRow) — `UpdateWorkflowAttributes` + inherits `workflow`'s emit via its delegation to + `CompositionService.UpdateAttributes`. Lists CRUD split out of + configureservice.go into configurelist.go (500-line limit). +- **P0-3** `GuardrailService` emits a new `guardrail-rule` entity after + rule CRUD. +- **P0-4** App.tsx's `mill-data-changed` handler no longer misroutes + `list`/`mcpserver` to `refreshRequests()`+`refreshWorkflows()` — each + entity now routes to its own store's refresher. +- **Shared emit helper**: `internal/services/dataevent` (new package) + is the ONE place `EventName`/`Changed`/`Emit` are defined — mcpsvc's + old locally-owned `DataChanged`/`DataChangedEventName`/ + `emitDataChanged` were migrated onto it, removing the duplication the + audit's root cause created. +- **P1-1** `frontend/src/shared/configureEntityStore.ts` (new file, + mirrors store.ts's workflows/requests pattern) backs + ConfigureLists/ConfigureDecisions/ConfigureMCPServers/ConfigureExecEnv + — each switched from its own local `useState` + fetch to the shared + store. ConfigureAttributes switched to store.ts's existing shared + `workflows`. +- **P1-2/P1-3/P1-4** WorkflowRunsPanel, HomeView (covers HomeMostUsed), + and ActivityRunsExplorer each subscribe to + `mill-data-changed{entity:"run"}` and refresh their run list/metrics. +- **P2** CompositionView's `refreshArmed`, `useGuardrailBadges`, and + ReviewView (added a `guardrail-pending-changed` subscription + alongside its existing 2s poll, kept as the documented fallback) all + wired. +- **Proofs**: Go unit tests per service + (`*_dataevent_test.go` in compositionsvc/configuresvc/guardrailsvc, + using `dataevent.TestHook` — the seam added since `application.Get()` + is always nil under `go test`); a new e2e spec, + `e2e/realtime-cross-surface.spec.ts`, proves the flagship two-surface + scenario both ways (an MCP-authored `import_list` reaching an open + Configure > Lists tab; a direct-UI workflow create in one browser + window reaching a canvas picker open in a second window) — neither + page ever reloads. +- **Left as-is, per the audit's own verdict** (unchanged): the in-flight + run 1s polls (DBOS has no per-step event), QuickPanel's + refresh-on-summon, hover-preview/EntityRefField mount-fetches. diff --git a/frontend/e2e/realtime-cross-surface.spec.ts b/frontend/e2e/realtime-cross-surface.spec.ts new file mode 100644 index 00000000..5b01bdd3 --- /dev/null +++ b/frontend/e2e/realtime-cross-surface.spec.ts @@ -0,0 +1,126 @@ +import type { Page } from '@playwright/test' +import { test, expect } from './fixtures/server' +import { connectMCPClient } from './mcpTestClient' +import { clickRowAction } from './inventoryRow' + +// Goal 0017's flagship scenario, direct from the audit's own root +// cause: before this goal, ONLY mcpsvc emitted mill-data-changed -- +// ConfigureService/CompositionService/GuardrailService emitted +// NOTHING, so a direct-UI or MCP-authored mutation never reached an +// already-open OTHER surface (only the exact tab that made the change +// ever refreshed itself, via its own local refetch call). This spec +// proves the fix at the two surfaces the audit named as the P0s: +// Configure's inventories (list/mcpserver were actively MISROUTED to +// refreshRequests()+refreshWorkflows(), App.tsx:242-244 before the +// fix) and a canvas entity picker seeing a workflow created elsewhere. + +// Normalizes to write-gate ON + per-write approval OFF (unattended) -- +// same local helper canvas-live-sync.spec.ts already uses (kept local +// there too, not promoted to mcpTestClient.ts, since that module's own +// enableMCPWritesWithApprovalRequired deliberately leaves approval ON +// for specs that want to exercise the approval banner instead). +async function enableUnattendedMCPWrites(page: Page): Promise { + 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).not.toBeChecked() + } +} + +async function restoreMCPWriteDefaults(page: Page): Promise { + await page.goto('/') + await page.getByRole('button', { name: 'Settings' }).click() + const approvalCheckbox = page.getByTestId('mcp-write-approval-checkbox') + if (await approvalCheckbox.count() && !(await approvalCheckbox.isChecked())) { + await approvalCheckbox.click() + await expect(approvalCheckbox).toBeChecked() + } + const writeCheckbox = page.getByTestId('mcp-write-enabled-checkbox') + if (await writeCheckbox.isChecked()) { + await writeCheckbox.click() + await expect(writeCheckbox).not.toBeChecked() + } +} + +test('Configure > Lists open: an MCP-authored import_list appears live, no reload (P0-2/P1-1)', async ({ page }, testInfo) => { + await enableUnattendedMCPWrites(page) + + await page.getByRole('link', { name: 'Configure' }).click() + await page.getByRole('tab', { name: 'Lists' }).click() + await expect(page.getByTestId('configure-lists')).toBeVisible() + + const label = 'E2E cross-surface list' + const row = page.locator('[data-testid="inventory-row"][data-entity="list"]', { has: page.getByText(label, { exact: true }) }) + await expect(row).toHaveCount(0) + + const client = await connectMCPClient(testInfo.parallelIndex) + try { + const result = await client.callTool({ + name: 'import_list', + arguments: { json: JSON.stringify({ label, description: '', columns: [{ Key: 'k', Label: 'K', Type: 'text' }] }) }, + }) + if (result.isError) throw new Error(`import_list failed: ${JSON.stringify(result.content)}`) + + // No page.reload() -- ConfigureLists.tsx now reads the shared + // configureEntityStore (shared/configureEntityStore.ts), which + // App.tsx's mill-data-changed{entity:"list"} handler refreshes. + // Before the fix, 'list' routed to refreshRequests()+ + // refreshWorkflows() -- neither of which touches this page's own + // (then-local) state at all, so this row would never have appeared + // without navigating away and back. + await expect(row).toBeVisible({ timeout: 10_000 }) + } finally { + await client.close() + } + + await clickRowAction(page, row, 'Delete') + await restoreMCPWriteDefaults(page) +}) + +test('a direct-UI workflow create in one window reaches a canvas picker open in another (P0-1)', async ({ page }) => { + // Two real pages against the SAME worker server -- the actual "two + // open surfaces" the goal names, not one page simulating it. Neither + // one drives the other; both independently subscribe to the same + // backend's mill-data-changed broadcast. + const page2 = await page.context().newPage() + try { + await page.goto('/') + await page.getByRole('link', { name: 'Configure' }).click() + await page.getByRole('tab', { name: 'Attributes' }).click() + const select = page.getByTestId('attributes-workflow-select') + await expect(select).toBeVisible() + + const label = 'E2E cross-surface workflow' + await expect(select.locator('option', { hasText: label })).toHaveCount(0) + + // A genuinely direct-UI create (the Workflows page's own "New + // workflow" button + Save, no MCP involved at all) on the SECOND + // page -- proves CompositionService.CreateWorkflow's own new + // dataevent.Emit call (compositionservice.go), not mcpsvc's. + await page2.goto('/') + await page2.getByRole('link', { name: 'Workflows' }).click() + await page2.getByTestId('new-workflow').click() + await page2.locator('[role="tabpanel"]:not([hidden])').last().getByLabel('Label').fill(label) + await page2.locator('[role="tabpanel"]:not([hidden])').last().getByTestId('save-workflow').click() + + // No page.reload() on page (the first page/window) -- ConfigureAttributes.tsx + // now reads shared/store.ts's workflows store, refreshed by + // App.tsx's mill-data-changed{entity:"workflow"} handler. + await expect(select.locator('option', { hasText: label })).toHaveCount(1, { timeout: 10_000 }) + + await page2.getByRole('link', { name: 'Workflows' }).click() + const row = page2.locator('[data-testid="inventory-row"][data-entity="workflow"]', { has: page2.getByText(label, { exact: true }) }) + await clickRowAction(page2, row, 'Delete') + } finally { + await page2.close() + } +}) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 3ae377ba..8bf4ddab 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -13,7 +13,7 @@ "@primer/octicons-react": "^19.33.0", "@primer/primitives": "^11.10.0", "@primer/react": "^38.35.0", - "@wailsio/runtime": "*", + "@wailsio/runtime": "latest", "@xyflow/react": "^12.11.2", "cronstrue": "^3.24.0", "elkjs": "^0.12.0", @@ -1829,14 +1829,14 @@ "version": "15.7.15", "resolved": "https://registry.npmjs.org/@types/prop-types/-/prop-types-15.7.15.tgz", "integrity": "sha512-F6bEyamV9jKGAFBEmlQnesRPGOQqS2+Uwi0Em15xenOxHaf2hv6L8YCVn3rPdPJOiJfPiCnLIRyvwVaqMY3MIw==", - "devOptional": true, + "dev": true, "license": "MIT" }, "node_modules/@types/react": { "version": "18.3.31", "resolved": "https://registry.npmjs.org/@types/react/-/react-18.3.31.tgz", "integrity": "sha512-vfEqpXTvwT91yhmwdfouStN2hSKwTvyRs8qpLfADyrq/kxDw0hZM7Wk9Ug1FELj8hIby+S/+kQCSRFF32nv2Qw==", - "devOptional": true, + "dev": true, "license": "MIT", "dependencies": { "@types/prop-types": "*", @@ -1847,7 +1847,7 @@ "version": "18.3.7", "resolved": "https://registry.npmjs.org/@types/react-dom/-/react-dom-18.3.7.tgz", "integrity": "sha512-MEe3UeoENYVFXzoXEWsvcpg6ZvlrFNlOQ7EOsvhI3CfAXwzPfO8Qwuxd40nepsYKqyyVQnTdEfv68q91yLcKrQ==", - "devOptional": true, + "dev": true, "license": "MIT", "peerDependencies": { "@types/react": "^18.0.0" @@ -2819,7 +2819,7 @@ "version": "3.2.3", "resolved": "https://registry.npmjs.org/csstype/-/csstype-3.2.3.tgz", "integrity": "sha512-z1HGKcYy2xA8AGQfwrn0PAy+PB7X/GSj3UVJW9qKyn43xWa+gl5nXmU4qqLMRzWVLFC8KusUX8T/0kCiOYpAIQ==", - "devOptional": true, + "dev": true, "license": "MIT" }, "node_modules/d3-array": { diff --git a/frontend/src/app/App.tsx b/frontend/src/app/App.tsx index bbcc2885..49686bf0 100644 --- a/frontend/src/app/App.tsx +++ b/frontend/src/app/App.tsx @@ -11,6 +11,7 @@ import PlaceholderView from "../views/PlaceholderView"; import { CapabilitiesService, ExecutionService, SettingsService } from '../shared/bindings' import type { BuildInfo } from '../shared/bindings' import { refreshKeybindings, refreshNodeTypes, refreshRequests, refreshWorkflows, useAppStore } from "../shared/store"; +import { refreshDecisions, refreshExecEnvs, refreshLists, refreshMCPServers } from "../shared/configureEntityStore"; import { dispatchCommandForEvent } from "../shared/commands"; import { WorkTabShell } from "./WorkTabShell"; import { AppSidebar } from "./AppSidebar"; @@ -230,17 +231,24 @@ function App() { // it's the only one of the two sources that fires headlessly; // Composition Run-button clicks push directly from their own handler, // since they already resolve synchronously in the browser. - // Live sync for MCP-driven authoring (docs/adr/0025): when an - // external LLM changes data through Mill's MCP server, the open - // window refreshes it immediately -- §1's what-you-see-is-what-I-see - // thesis running in both directions. One coarse refresh per entity - // kind; the stores are cheap to re-fetch at Mill's scale. + // Live sync (docs/adr/0025 + goal 0017): every direct-mutation + // service now emits this, not just mcpsvc -- one refresher per + // entity kind, each routed to its own store (shared/store.ts's + // workflows/requests, shared/configureEntityStore.ts's lists/ + // decisions/mcpServers/execEnvs). Was previously misrouted for + // 'list'/'mcpserver' (refreshRequests()+refreshWorkflows(), neither + // of which holds either); 'decision'/'execenv' are new entity + // strings. 'guardrail-rule' has no shared-store consumer here -- + // useGuardrailBadges/the Guardrails section subscribe to it directly. useEffect(() => { return Events.On('mill-data-changed', (evt) => { const entity = (evt.data as { entity?: string })?.entity if (entity === 'workflow' || entity === 'run') void refreshWorkflows() if (entity === 'request') void refreshRequests() - if (entity === 'list' || entity === 'mcpserver') { void refreshRequests(); void refreshWorkflows() } + if (entity === 'list') void refreshLists() + if (entity === 'mcpserver') void refreshMCPServers() + if (entity === 'decision') void refreshDecisions() + if (entity === 'execenv') void refreshExecEnvs() }) }, []) diff --git a/frontend/src/composition/CompositionView.tsx b/frontend/src/composition/CompositionView.tsx index 66bd38cc..69f01f43 100644 --- a/frontend/src/composition/CompositionView.tsx +++ b/frontend/src/composition/CompositionView.tsx @@ -1,4 +1,5 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { Events } from '@wailsio/runtime' import { Button, Heading, Label, Stack, Text } from '@primer/react' import { PlusIcon, UploadIcon, WorkflowIcon } from '@primer/octicons-react' import { CompositionService, ExecutionService, TriggerService } from '../shared/bindings' @@ -72,6 +73,20 @@ function CompositionView() { refreshArmed() }, [refreshArmed]) + // goal 0017 P2: a Publish/disable/delete elsewhere (another tab, an + // MCP author) can arm or disarm a workflow's trigger listener -- + // armedWorkflows used to only refresh from THIS page's own Publish + // button/mount, so that badge could silently go stale for a change + // made anywhere else. refreshWorkflows() already runs on the same + // event (App.tsx), but the list's own store update doesn't imply + // TriggerService's separately-tracked armed-set changed too. + useEffect(() => { + return Events.On('mill-data-changed', (evt) => { + const entity = (evt.data as { entity?: string })?.entity + if (entity === 'workflow') refreshArmed() + }) + }, [refreshArmed]) + // The row-level Publish CTA (docs/goals/0006, decision 2): publishing // is what's actually blocking a configured-but-not-live trigger from // arming (TriggerService.Sync's own gate), so this is the same diff --git a/frontend/src/composition/WorkflowRunsPanel.tsx b/frontend/src/composition/WorkflowRunsPanel.tsx index 58a0feaa..c850cec4 100644 --- a/frontend/src/composition/WorkflowRunsPanel.tsx +++ b/frontend/src/composition/WorkflowRunsPanel.tsx @@ -1,4 +1,5 @@ import { useEffect, useRef, useState } from 'react' +import { Events } from '@wailsio/runtime' import { Button, IconButton, Label, type LabelProps, Select, Stack, Text } from '@primer/react' import { DataTable, type Column } from '@primer/react/experimental' import { BugIcon, CheckCircleIcon, XCircleIcon, ClockIcon, XIcon, ShieldIcon, ShieldXIcon, StopIcon } from '@primer/octicons-react' @@ -99,6 +100,22 @@ function WorkflowRunsPanel({ workflowId, attrs, initialRunId, onInitialRunConsum // eslint-disable-next-line react-hooks/exhaustive-deps }, [workflowId]) + // goal 0017 P1-2: this tab's base run list used to update only on + // mount/workflow-switch -- a run started elsewhere (another tab, a + // headless trigger, an MCP author's run_workflow) never appeared here + // without reopening the tab. mill-data-changed{entity:"run"} is + // already emitted for every run kind (executionsvc's own run-start/ + // debug-tool paths); the in-flight-run detail poll above stays -- + // DBOS has no per-step event, so polling an already-open run's own + // step-by-step progress is still the honest only-path. + useEffect(() => { + return Events.On('mill-data-changed', (evt) => { + const entity = (evt.data as { entity?: string })?.entity + if (entity === 'run') refreshRuns() + }) + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [workflowId]) + useEffect(() => { if (!initialRunId) return setSelectedRunID(initialRunId) diff --git a/frontend/src/composition/useGuardrailBadges.ts b/frontend/src/composition/useGuardrailBadges.ts index 7db38763..1b137d97 100644 --- a/frontend/src/composition/useGuardrailBadges.ts +++ b/frontend/src/composition/useGuardrailBadges.ts @@ -1,4 +1,5 @@ import { useCallback, useEffect } from 'react' +import { Events } from '@wailsio/runtime' import { GuardrailService } from '../shared/bindings' import type { CanvasNode, CanvasState } from './canvasStore' @@ -37,5 +38,18 @@ export function useGuardrailBadges(workflowId: string | undefined, nodes: Canvas // eslint-disable-next-line react-hooks/exhaustive-deps }, [workflowId, nodeFingerprint]) + // goal 0017 P2: a policy guardrail rule changed in Configure > + // Guardrails (another tab, or an external MCP author once that + // surface exists) must re-run this canvas's verdicts too -- the + // nodeFingerprint-keyed effect above only notices a NODE edit, never + // a rule edit elsewhere, so a canvas left open could show a stale + // ask/deny badge after its governing rule changed underneath it. + useEffect(() => { + return Events.On('mill-data-changed', (evt) => { + const entity = (evt.data as { entity?: string })?.entity + if (entity === 'guardrail-rule') refresh() + }) + }, [refresh]) + return refresh } diff --git a/frontend/src/configure/ConfigureAttributes.tsx b/frontend/src/configure/ConfigureAttributes.tsx index 2125371e..b1ff033f 100644 --- a/frontend/src/configure/ConfigureAttributes.tsx +++ b/frontend/src/configure/ConfigureAttributes.tsx @@ -1,9 +1,10 @@ -import { useEffect, useState } from 'react' +import { useState } from 'react' import { Button, FormControl, Heading, IconButton, Select, Stack, Text, TextInput } from '@primer/react' import { PlusIcon, TrashIcon } from '@primer/octicons-react' -import { CompositionService, ConfigureService } from '../shared/bindings' -import type { AttributeDef, Workflow } from '../../bindings/github.com/alicoding/mill/internal/domain/composition/models' +import { ConfigureService } from '../shared/bindings' +import type { AttributeDef } from '../../bindings/github.com/alicoding/mill/internal/domain/composition/models' import { Type as ConfigFieldType } from '../../bindings/github.com/alicoding/mill/internal/domain/typedfield/models' +import { refreshWorkflows, useAppStore } from '../shared/store' import styles from '../shared/ListCard.module.css' import PageContainer from '../shared/PageContainer' @@ -22,17 +23,19 @@ const TYPE_LABEL: Record = { // no way to build a choice-set from it; see ruleTranslate.ts's // fieldsFromAttributes for the same exclusion on the read side. export function ConfigureAttributes() { - const [workflows, setWorkflows] = useState(null) + // Store-shared workflows (shared/store.ts's refreshWorkflows/ + // useAppStore) instead of this page's own CompositionService. + // Workflows() fetch (goal 0017 P1-1: it's the same list every other + // surface reads, and App.tsx already fetches it once on mount and + // refreshes it on mill-data-changed{entity:"workflow"} -- a second, + // page-local copy could only ever drift from that). + const workflows = useAppStore((s) => s.workflows) const [selectedID, setSelectedID] = useState('') const [attrs, setAttrs] = useState([]) const [saving, setSaving] = useState(false) const [error, setError] = useState('') const [saved, setSaved] = useState(false) - useEffect(() => { - CompositionService.Workflows().then((list) => setWorkflows(list ?? [])).catch(console.error) - }, []) - const selectWorkflow = (id: string) => { setSelectedID(id) setAttrs(workflows?.find((w) => w.ID === id)?.Attributes ?? []) @@ -49,8 +52,8 @@ export function ConfigureAttributes() { setSaved(false) setSaving(true) try { - const updated = await ConfigureService.UpdateWorkflowAttributes(selectedID, attrs) - setWorkflows((prev) => prev?.map((w) => (w.ID === selectedID ? updated : w)) ?? null) + await ConfigureService.UpdateWorkflowAttributes(selectedID, attrs) + void refreshWorkflows() setSaved(true) } catch (err) { setError(String(err)) diff --git a/frontend/src/configure/ConfigureDecisions.tsx b/frontend/src/configure/ConfigureDecisions.tsx index a6ee46d0..5eac467e 100644 --- a/frontend/src/configure/ConfigureDecisions.tsx +++ b/frontend/src/configure/ConfigureDecisions.tsx @@ -9,6 +9,7 @@ import { Category } from '../../bindings/github.com/alicoding/mill/internal/doma import { Type as ConfigFieldType } from '../../bindings/github.com/alicoding/mill/internal/domain/typedfield/models' import { EntityRefField } from './EntityRefField' import { downloadJSON } from '../shared/downloadJSON' +import { refreshDecisions, useConfigureEntityStore } from '../shared/configureEntityStore' import { ViewModeToggle } from '../shared/ViewModeToggle' import { useViewMode } from '../shared/viewMode' import { InventoryList, type InventoryItem } from '../shared/InventoryList' @@ -53,7 +54,9 @@ function emptyOutput(): OutputField { // as Lists/MCP Servers); Duplicate/Export/Delete move into the // trailing ⋯ menu. export function ConfigureDecisions() { - const [decisions, setDecisions] = useState(null) + // Store-shared (refreshDecisions, shared/configureEntityStore.ts) -- + // see ConfigureLists.tsx's identical comment (goal 0017 P1-1). + const decisions = useConfigureEntityStore((s) => s.decisions) const [editingID, setEditingID] = useState(null) const [label, setLabel] = useState('') const [category, setCategory] = useState(Category.CategoryUncategorized) @@ -66,7 +69,7 @@ export function ConfigureDecisions() { const [viewMode, setViewMode] = useViewMode('mill-decisions-view-mode') const refetch = () => { - ConfigureService.Decisions().then((list) => setDecisions(list ?? [])).catch(console.error) + void refreshDecisions() } useEffect(refetch, []) diff --git a/frontend/src/configure/ConfigureExecEnv.tsx b/frontend/src/configure/ConfigureExecEnv.tsx index 0d56d54c..a0691435 100644 --- a/frontend/src/configure/ConfigureExecEnv.tsx +++ b/frontend/src/configure/ConfigureExecEnv.tsx @@ -7,6 +7,7 @@ import { ConfigureService } from '../shared/bindings' import type { ExecEnv } from '../../bindings/github.com/alicoding/mill/internal/domain/execenv/models' import { Shell, ProfileMode } from '../../bindings/github.com/alicoding/mill/internal/domain/execenv/models' import { downloadJSON } from '../shared/downloadJSON' +import { refreshExecEnvs, useConfigureEntityStore } from '../shared/configureEntityStore' import { envToRows, rowsToEnv, type EnvRow } from './execEnvRows' import { ViewModeToggle } from '../shared/ViewModeToggle' import { useViewMode } from '../shared/viewMode' @@ -55,7 +56,9 @@ const PROFILE_CAPTION: Partial> = { // (the Configure-entity recipe, docs/SPEC.md §9.5) closely: no // secret/auth concept here at all, same as MCP Server. export function ConfigureExecEnv() { - const [envs, setEnvs] = useState(null) + // Store-shared (refreshExecEnvs, shared/configureEntityStore.ts) -- + // see ConfigureLists.tsx's identical comment (goal 0017 P1-1). + const envs = useConfigureEntityStore((s) => s.execEnvs) const [editingID, setEditingID] = useState(null) const [label, setLabel] = useState('') const [shell, setShell] = useState(Shell.ShellZsh) @@ -70,7 +73,7 @@ export function ConfigureExecEnv() { const [viewMode, setViewMode] = useViewMode('mill-execenvs-view-mode') const refetch = () => { - ConfigureService.ExecEnvs().then((list) => setEnvs(list ?? [])).catch(console.error) + void refreshExecEnvs() } const exportEnv = (id: string, label: string) => { diff --git a/frontend/src/configure/ConfigureLists.tsx b/frontend/src/configure/ConfigureLists.tsx index 9d20defb..d9bcb5c9 100644 --- a/frontend/src/configure/ConfigureLists.tsx +++ b/frontend/src/configure/ConfigureLists.tsx @@ -9,6 +9,7 @@ import { RowStatus } from '../../bindings/github.com/alicoding/mill/internal/dom import type { Field } from '../../bindings/github.com/alicoding/mill/internal/domain/typedfield/models' import { Type as ConfigFieldType } from '../../bindings/github.com/alicoding/mill/internal/domain/typedfield/models' import { downloadJSON } from '../shared/downloadJSON' +import { refreshLists, useConfigureEntityStore } from '../shared/configureEntityStore' import { ViewModeToggle } from '../shared/ViewModeToggle' import { useViewMode } from '../shared/viewMode' import { InventoryList, type InventoryItem } from '../shared/InventoryList' @@ -39,7 +40,12 @@ function emptyColumn(): Field { // list-lookup and a list-search workflow node resolve against these // same Columns/Rows. export function ConfigureLists() { - const [lists, setLists] = useState(null) + // Store-shared (refreshLists, shared/configureEntityStore.ts), the + // same one-fetch-many-consumers pattern store.ts's workflows/requests + // already use -- so App.tsx's mill-data-changed handler pushing a + // live update lands here even when this tab is already open, + // mounted, and idle (goal 0017 P1-1). + const lists = useConfigureEntityStore((s) => s.lists) const [editingID, setEditingID] = useState(null) const [label, setLabel] = useState('') const [description, setDescription] = useState('') @@ -52,7 +58,7 @@ export function ConfigureLists() { const [viewMode, setViewMode] = useViewMode('mill-lists-view-mode') const refetch = () => { - ConfigureService.Lists().then((l) => setLists(l ?? [])).catch(console.error) + void refreshLists() } const exportList = (id: string, label: string) => { diff --git a/frontend/src/configure/ConfigureMCPServers.tsx b/frontend/src/configure/ConfigureMCPServers.tsx index 778fe9e3..6c952bc0 100644 --- a/frontend/src/configure/ConfigureMCPServers.tsx +++ b/frontend/src/configure/ConfigureMCPServers.tsx @@ -7,6 +7,7 @@ import { ConfigureService } from '../shared/bindings' import type { MCPServer } from '../../bindings/github.com/alicoding/mill/internal/domain/mcpserver/models' import type { Tool } from '../../bindings/github.com/alicoding/mill/internal/adapters/mcpclient/models' import { downloadJSON } from '../shared/downloadJSON' +import { refreshMCPServers, useConfigureEntityStore } from '../shared/configureEntityStore' import { ViewModeToggle } from '../shared/ViewModeToggle' import { useViewMode } from '../shared/viewMode' import { InventoryList, type InventoryItem } from '../shared/InventoryList' @@ -34,7 +35,9 @@ function argsToRows(args: string[] | null | undefined): string[] { // the list, one panel per server that's been queried), just triggered // from the menu instead of a dedicated button. export function ConfigureMCPServers() { - const [servers, setServers] = useState(null) + // Store-shared (refreshMCPServers, shared/configureEntityStore.ts) -- + // see ConfigureLists.tsx's identical comment (goal 0017 P1-1). + const servers = useConfigureEntityStore((s) => s.mcpServers) const [editingID, setEditingID] = useState(null) const [label, setLabel] = useState('') const [command, setCommand] = useState('') @@ -47,7 +50,7 @@ export function ConfigureMCPServers() { const [viewMode, setViewMode] = useViewMode('mill-mcpservers-view-mode') const refetch = () => { - ConfigureService.MCPServers().then((list) => setServers(list ?? [])).catch(console.error) + void refreshMCPServers() } const exportServer = (id: string, label: string) => { diff --git a/frontend/src/shared/configureEntityStore.ts b/frontend/src/shared/configureEntityStore.ts new file mode 100644 index 00000000..ab87d0e3 --- /dev/null +++ b/frontend/src/shared/configureEntityStore.ts @@ -0,0 +1,71 @@ +import { create } from 'zustand' +import { ConfigureService } from './bindings' +import type { List } from '../../bindings/github.com/alicoding/mill/internal/domain/list/models' +import type { Decision } from '../../bindings/github.com/alicoding/mill/internal/domain/decision/models' +import type { MCPServer } from '../../bindings/github.com/alicoding/mill/internal/domain/mcpserver/models' +import type { ExecEnv } from '../../bindings/github.com/alicoding/mill/internal/domain/execenv/models' + +// The other half of store.ts's "one fetch, many consumers" server-data +// pattern (workflows/nodeTypes/requests) for Configure's remaining +// reusable entity kinds -- goal 0017 P1-1: before this, each of +// ConfigureLists/ConfigureDecisions/ConfigureMCPServers/ConfigureExecEnv +// held its OWN local useState + its own fetch, so App.tsx's +// mill-data-changed handler had nowhere to push a live update into -- +// an already-open Configure tab could never see an entity created +// elsewhere (the UI, another tab, an MCP author) without a full +// remount. A second store file, not more fields on store.ts's +// useAppStore, purely to stay under CLAUDE.md's 500-line-per-file +// convention (useAppStore already carries the work-tab/view state +// machine) -- same shared/ leaf placement, same dependency-cruiser +// boundary (.claude/rules/frontend.md: shared/ has no upward imports), +// just its own `create()` call. +interface ConfigureEntityState { + lists: List[] | null + decisions: Decision[] | null + mcpServers: MCPServer[] | null + execEnvs: ExecEnv[] | null + setLists: (lists: List[]) => void + setDecisions: (decisions: Decision[]) => void + setMCPServers: (mcpServers: MCPServer[]) => void + setExecEnvs: (execEnvs: ExecEnv[]) => void +} + +export const useConfigureEntityStore = create()((set) => ({ + lists: null, + decisions: null, + mcpServers: null, + execEnvs: null, + setLists: (lists) => set({ lists }), + setDecisions: (decisions) => set({ decisions }), + setMCPServers: (mcpServers) => set({ mcpServers }), + setExecEnvs: (execEnvs) => set({ execEnvs }), +})) + +// refreshLists/refreshDecisions/refreshMCPServers/refreshExecEnvs mirror +// store.ts's refreshWorkflows/refreshRequests shape exactly: the one +// refetch path per shared list, callable from any surface (a page's +// mount, a form's onSaved, App.tsx's mill-data-changed router) without +// prop threading. +export function refreshLists(): Promise { + return ConfigureService.Lists() + .then((list) => useConfigureEntityStore.getState().setLists(list ?? [])) + .catch(console.error) +} + +export function refreshDecisions(): Promise { + return ConfigureService.Decisions() + .then((list) => useConfigureEntityStore.getState().setDecisions(list ?? [])) + .catch(console.error) +} + +export function refreshMCPServers(): Promise { + return ConfigureService.MCPServers() + .then((list) => useConfigureEntityStore.getState().setMCPServers(list ?? [])) + .catch(console.error) +} + +export function refreshExecEnvs(): Promise { + return ConfigureService.ExecEnvs() + .then((list) => useConfigureEntityStore.getState().setExecEnvs(list ?? [])) + .catch(console.error) +} diff --git a/frontend/src/views/ActivityRunsExplorer.tsx b/frontend/src/views/ActivityRunsExplorer.tsx index 59d04d61..2cbd8aec 100644 --- a/frontend/src/views/ActivityRunsExplorer.tsx +++ b/frontend/src/views/ActivityRunsExplorer.tsx @@ -1,4 +1,5 @@ import { useEffect, useState } from 'react' +import { Events } from '@wailsio/runtime' import { Button, Label, Stack, Text, TextInput } from '@primer/react' import { DataTable, type Column } from '@primer/react/experimental' import { StopIcon } from '@primer/octicons-react' @@ -40,6 +41,17 @@ export function ActivityRunsExplorer({ workflow }: { workflow: Workflow }) { // eslint-disable-next-line react-hooks/exhaustive-deps }, [workflow.ID]) + // goal 0017 P1-4: this explorer's durable run history used to update + // only on mount/workflow-switch -- scoped to the selected workflow, + // same as WorkflowRunsPanel.tsx's identical subscription. + useEffect(() => { + return Events.On('mill-data-changed', (evt) => { + const entity = (evt.data as { entity?: string })?.entity + if (entity === 'run') 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 diff --git a/frontend/src/views/HomeView.tsx b/frontend/src/views/HomeView.tsx index cb05977c..1e227e1c 100644 --- a/frontend/src/views/HomeView.tsx +++ b/frontend/src/views/HomeView.tsx @@ -1,4 +1,5 @@ import { lazy, Suspense, useCallback, useEffect, useState } from 'react' +import { Events } from '@wailsio/runtime' import { Checkbox, FormControl, Heading, SegmentedControl, Spinner, Stack, Text } from '@primer/react' import { Blankslate } from '@primer/react/experimental' import { GraphIcon } from '@primer/octicons-react' @@ -49,6 +50,19 @@ export default function HomeView() { useEffect(() => { refresh() }, [refresh]) + // goal 0017 P1-3: Home is "the reason to open Mill" (docs/SPEC.md + // §3.2.3) -- it must show a run that happened while the window sat + // open on this page (a headless trigger, an MCP-driven run_workflow) + // without the owner having to switch away and back. metrics (and the + // mostUsed list HomeMostUsed.tsx renders from it) are both derived + // from this one refresh() call, so one subscription covers both. + useEffect(() => { + return Events.On('mill-data-changed', (evt) => { + const entity = (evt.data as { entity?: string })?.entity + if (entity === 'run') refresh() + }) + }, [refresh]) + useEffect(() => { // The generated binding types this `{ [_ in string]?: number }` (an // optional index signature, mirroring Go's map[string]int, which diff --git a/frontend/src/views/ReviewView.tsx b/frontend/src/views/ReviewView.tsx index 5f4ab4ac..fad7af08 100644 --- a/frontend/src/views/ReviewView.tsx +++ b/frontend/src/views/ReviewView.tsx @@ -88,9 +88,19 @@ function ReviewView() { useEffect(() => { refresh() + // goal 0017 P2: guardrail-pending-changed (executionsvc, already + // emitted on every park/resolve -- App.tsx's own pending-badge + // effect already consumes it) gets the queue refreshing on the SAME + // event that already exists, instead of waiting up to 2s for the + // poll below. The poll itself stays: it's the documented fallback + // for anything that changes queue state without a dedicated event + // (a park racing this subscription's mount, a clock-based staleness + // badge aging past its threshold with nothing else to trigger a + // re-render). const timer = setInterval(refresh, 2000) const offMCP = Events.On('mcp-write-approval', refresh) - return () => { clearInterval(timer); offMCP() } + const offGuardrail = Events.On('guardrail-pending-changed', refresh) + return () => { clearInterval(timer); offMCP(); offGuardrail() } }, []) const resolveWrite = (id: string, approve: boolean) => { diff --git a/internal/services/compositionsvc/compositionservice.go b/internal/services/compositionsvc/compositionservice.go index e806571b..4e8daf75 100644 --- a/internal/services/compositionsvc/compositionservice.go +++ b/internal/services/compositionsvc/compositionservice.go @@ -10,6 +10,7 @@ import ( "github.com/alicoding/mill/internal/adapters/settings" "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/seeding" ) @@ -185,6 +186,11 @@ func (c *CompositionService) CreateWorkflow(label, description string, nodes []c return composition.Workflow{}, fmt.Errorf("save workflow: %w", err) } c.notifySyncer() + // Live-sync (goal 0017): a direct UI create must reach every other + // open surface exactly like an MCP-authored one already does + // (docs/adr/0025) -- ImportWorkflow delegates here, so this single + // call covers both entry points. + dataevent.Emit("workflow", wf.ID) return wf, nil } @@ -275,6 +281,7 @@ func (c *CompositionService) UpdateWorkflow(id, label, description string, nodes return composition.Workflow{}, fmt.Errorf("save workflow: %w", err) } c.notifySyncer() + dataevent.Emit("workflow", wf.ID) // goal 0017: live-sync every open surface return wf, nil } @@ -330,6 +337,7 @@ func (c *CompositionService) UpdateAttributes(workflowID string, attrs []composi c.mu.Unlock() return composition.Workflow{}, fmt.Errorf("save workflow attributes: %w", err) } + dataevent.Emit("workflow", wf.ID) // goal 0017: live-sync every open surface return wf, nil } @@ -375,6 +383,7 @@ func (c *CompositionService) DeleteWorkflow(id string) error { return fmt.Errorf("save workflow deletion: %w", err) } c.notifySyncer() + dataevent.Emit("workflow", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/compositionsvc/compositionservice_dataevent_test.go b/internal/services/compositionsvc/compositionservice_dataevent_test.go new file mode 100644 index 00000000..d98dd3e0 --- /dev/null +++ b/internal/services/compositionsvc/compositionservice_dataevent_test.go @@ -0,0 +1,152 @@ +package compositionsvc + +import ( + "testing" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/domain/typedfield" + "github.com/alicoding/mill/internal/services/dataevent" + "github.com/alicoding/mill/internal/services/servicetest" +) + +// captureEmits swaps in dataevent.TestHook (dataevent.go's own doc +// comment has the full reasoning: application.Get() is always nil +// under `go test`, so this is the one seam that can observe an Emit +// call at all) and returns the slice it appends every (entity, id) +// pair to. Restored to nil on test cleanup so a later test in this +// package never sees a stale hook. +func captureEmits(t *testing.T) *[]dataevent.Changed { + t.Helper() + var got []dataevent.Changed + dataevent.TestHook = func(entity, id string) { + got = append(got, dataevent.Changed{Entity: entity, ID: id}) + } + t.Cleanup(func() { dataevent.TestHook = nil }) + return &got +} + +// TestDataEvent_WorkflowMutations proves goal 0017's P0-1: every +// direct-UI workflow mutation emits mill-data-changed{entity:"workflow"} +// -- the root-cause fix (before this, ONLY mcpsvc's MCP-authoring path +// emitted this event, so a plain Composition-page create/edit/publish +// never reached another open tab/picker). +func TestDataEvent_WorkflowMutations(t *testing.T) { + store := servicetest.NewFakeStore() + c := NewCompositionService(store) + nodes := []composition.Node{{ID: "t", NodeTypeID: "trigger-manual"}} + + t.Run("CreateWorkflow", func(t *testing.T) { + got := captureEmits(t) + wf, err := c.CreateWorkflow("Emit test wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + }) + + t.Run("UpdateWorkflow", func(t *testing.T) { + wf, err := c.CreateWorkflow("Update-target wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + got := captureEmits(t) + updated, err := c.UpdateWorkflow(wf.ID, "Update-target wf (edited)", "", nodes, nil) + if err != nil { + t.Fatalf("UpdateWorkflow: %v", err) + } + assertEmittedWorkflow(t, *got, updated.ID) + }) + + t.Run("UpdateAttributes", func(t *testing.T) { + wf, err := c.CreateWorkflow("Attrs-target wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + got := captureEmits(t) + attrs := []composition.AttributeDef{{Key: "note", Type: typedfield.TypeText}} + updated, err := c.UpdateAttributes(wf.ID, attrs) + if err != nil { + t.Fatalf("UpdateAttributes: %v", err) + } + assertEmittedWorkflow(t, *got, updated.ID) + }) + + t.Run("PublishWorkflow_PublishExistingVersion_SetWorkflowDisabled_RestoreVersionToDraft", func(t *testing.T) { + // All four route through the shared mutateWorkflow choke point + // (compositionservice_versioning.go) -- one emit call there + // covers every one of them; exercised together here for that + // reason, one call each. + wf, err := c.CreateWorkflow("Lifecycle-target wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + + got := captureEmits(t) + if _, err := c.PublishWorkflow(wf.ID); err != nil { + t.Fatalf("PublishWorkflow: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + + got = captureEmits(t) + if _, err := c.SetWorkflowDisabled(wf.ID, true); err != nil { + t.Fatalf("SetWorkflowDisabled: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + + got = captureEmits(t) + if _, err := c.PublishExistingVersion(wf.ID, 1); err != nil { + t.Fatalf("PublishExistingVersion: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + + got = captureEmits(t) + if _, err := c.RestoreVersionToDraft(wf.ID, 1); err != nil { + t.Fatalf("RestoreVersionToDraft: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + }) + + t.Run("DeleteWorkflow", func(t *testing.T) { + wf, err := c.CreateWorkflow("Delete-target wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + got := captureEmits(t) + if err := c.DeleteWorkflow(wf.ID); err != nil { + t.Fatalf("DeleteWorkflow: %v", err) + } + assertEmittedWorkflow(t, *got, wf.ID) + }) + + t.Run("ImportWorkflow_delegatesToCreate", func(t *testing.T) { + source, err := c.CreateWorkflow("Export-source wf", "", nodes, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + exported, err := c.ExportWorkflow(source.ID) + if err != nil { + t.Fatalf("ExportWorkflow: %v", err) + } + got := captureEmits(t) + imported, err := c.ImportWorkflow(exported) + if err != nil { + t.Fatalf("ImportWorkflow: %v", err) + } + assertEmittedWorkflow(t, *got, imported.ID) + }) +} + +// assertEmittedWorkflow fails the test unless got contains at least +// one dataevent.Changed{"workflow", id} pair -- every mutation this +// file tests emits the "workflow" entity, so entity itself isn't a +// parameter (unlike configuresvc's/guardrailsvc's own dataevent +// tests, which cover several different entity strings). +func assertEmittedWorkflow(t *testing.T, got []dataevent.Changed, id string) { + t.Helper() + for _, c := range got { + if c.Entity == "workflow" && c.ID == id { + return + } + } + t.Errorf("dataevent.Emit(\"workflow\", %q) was not observed; got %+v", id, got) +} diff --git a/internal/services/compositionsvc/compositionservice_versioning.go b/internal/services/compositionsvc/compositionservice_versioning.go index db697715..ef516bbb 100644 --- a/internal/services/compositionsvc/compositionservice_versioning.go +++ b/internal/services/compositionsvc/compositionservice_versioning.go @@ -6,6 +6,7 @@ import ( "time" "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/dataevent" ) // Workflow lifecycle & versioning RPCs (docs/adr/0021) -- the @@ -57,6 +58,11 @@ func (c *CompositionService) mutateWorkflow(id string, fn func(composition.Workf return composition.Workflow{}, fmt.Errorf("save workflow: %w", err) } c.notifySyncer() + // Live-sync (goal 0017): the ONE choke point every lifecycle + // mutation (Publish/PublishExistingVersion/RestoreVersionToDraft/ + // SetWorkflowDisabled/SnapshotDraft) routes through -- one emit call + // covers all five instead of duplicating it per caller. + dataevent.Emit("workflow", updated.ID) return updated, nil } diff --git a/internal/services/configuresvc/configuredecision.go b/internal/services/configuresvc/configuredecision.go index 6d9ed53c..3de7baaa 100644 --- a/internal/services/configuresvc/configuredecision.go +++ b/internal/services/configuresvc/configuredecision.go @@ -7,6 +7,7 @@ import ( "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/decision" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/seeding" ) @@ -71,6 +72,7 @@ func (c *ConfigureService) CreateDecision(label string, category decision.Catego c.mu.Unlock() return decision.Decision{}, fmt.Errorf("save decision: %w", err) } + dataevent.Emit("decision", d.ID) // goal 0017: live-sync every open surface return d, nil } @@ -143,6 +145,7 @@ func (c *ConfigureService) UpdateDecision(id, label string, category decision.Ca c.mu.Unlock() return decision.Decision{}, fmt.Errorf("save decision: %w", err) } + dataevent.Emit("decision", d.ID) // goal 0017: live-sync every open surface return d, nil } @@ -182,6 +185,7 @@ func (c *ConfigureService) DeleteDecision(id string) error { c.mu.Unlock() return fmt.Errorf("save decision deletion: %w", err) } + dataevent.Emit("decision", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/configuresvc/configureexecenv.go b/internal/services/configuresvc/configureexecenv.go index e1b17867..b520c1ca 100644 --- a/internal/services/configuresvc/configureexecenv.go +++ b/internal/services/configuresvc/configureexecenv.go @@ -8,6 +8,7 @@ import ( "github.com/alicoding/mill/internal/adapters/shellenv" "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/execenv" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/seeding" ) @@ -81,6 +82,7 @@ func (c *ConfigureService) CreateExecEnv(label string, shell execenv.Shell, prof c.mu.Unlock() return execenv.ExecEnv{}, fmt.Errorf("save execution environment: %w", err) } + dataevent.Emit("execenv", e.ID) // goal 0017: live-sync every open surface return e, nil } @@ -125,6 +127,7 @@ func (c *ConfigureService) UpdateExecEnv(id, label string, shell execenv.Shell, c.mu.Unlock() return execenv.ExecEnv{}, fmt.Errorf("save execution environment: %w", err) } + dataevent.Emit("execenv", e.ID) // goal 0017: live-sync every open surface return e, nil } @@ -165,6 +168,7 @@ func (c *ConfigureService) DeleteExecEnv(id string) error { c.mu.Unlock() return fmt.Errorf("save execution environment deletion: %w", err) } + dataevent.Emit("execenv", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/configuresvc/configurelist.go b/internal/services/configuresvc/configurelist.go new file mode 100644 index 00000000..a57af678 --- /dev/null +++ b/internal/services/configuresvc/configurelist.go @@ -0,0 +1,368 @@ +package configuresvc + +import ( + "encoding/json" + "fmt" + "log/slog" + "time" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/domain/list" + "github.com/alicoding/mill/internal/domain/typedfield" + "github.com/alicoding/mill/internal/services/dataevent" + "github.com/alicoding/mill/internal/services/seeding" +) + +// List CRUD/persistence -- split out of configureservice.go (goal +// 0017) once adding the live-sync dataevent.Emit calls below would +// have pushed that file past CLAUDE.md's 500-line convention. Same +// per-entity-file organization configuremcpserver.go/configuredecision.go/ +// configureexecenv.go's own header comments already established -- +// Lists was the one entity type still living in the main file. + +// resolveList implements composition.go's lookupListFn seam. Entries +// is list.DeriveEntries's own computed key/value view (goal 0011) -- +// list-lookup keeps reading a flat map with zero changes to its own +// execution logic; Columns/Rows are the typed additions list-search +// reads. +func (c *ConfigureService) resolveList(id string) (composition.ResolvedList, error) { + c.mu.Lock() + defer c.mu.Unlock() + for _, l := range c.lists { + if l.ID == id { + return composition.ResolvedList{ + Entries: list.DeriveEntries(l), + Columns: l.Columns, + Rows: l.Rows, + }, nil + } + } + return composition.ResolvedList{}, fmt.Errorf("no list with id %q", id) +} + +// --- Lists --- + +func (c *ConfigureService) Lists() []list.List { + c.mu.Lock() + defer c.mu.Unlock() + out := make([]list.List, len(c.lists)) + copy(out, c.lists) + return out +} + +// findListLocked returns the index of the list with id in c.lists, or +// -1 -- caller must hold c.mu. +func (c *ConfigureService) findListLocked(id string) int { + for i, l := range c.lists { + if l.ID == id { + return i + } + } + return -1 +} + +// removeListByIDLocked removes the list with id from c.lists, if +// present -- caller must hold c.mu. +func (c *ConfigureService) removeListByIDLocked(id string) { + if idx := c.findListLocked(id); idx != -1 { + c.lists = append(c.lists[:idx], c.lists[idx+1:]...) + } +} + +// revertListLocked restores previous (keyed by its own ID) into +// c.lists -- caller must hold c.mu. Shared undo path for every List/ +// row mutation below's persist-failure revert (docs/goals/0025 item +// 2's memory-vs-store rule, extended here from CreateList/UpdateList +// to the new row-level mutations for the same reason: a phantom +// in-memory row a restart would silently drop is exactly as wrong as +// a phantom list). +func (c *ConfigureService) revertListLocked(previous list.List) { + if idx := c.findListLocked(previous.ID); idx != -1 { + c.lists[idx] = previous + } +} + +func (c *ConfigureService) CreateList(label, description string, columns []typedfield.Field) (list.List, error) { + now := time.Now() + l := list.List{ + ID: seeding.NewSlugID(label, "list"), Label: label, Description: description, + Columns: columns, CreatedAt: now, UpdatedAt: now, + } + if err := list.Validate(l); err != nil { + return list.List{}, err + } + + c.mu.Lock() + c.lists = append(c.lists, l) + c.mu.Unlock() + + if err := c.persistLists(); err != nil { + // Don't leave a phantom-saved list in memory that a restart + // would drop (docs/goals/0025 item 2's memory-vs-store rule). + c.mu.Lock() + c.removeListByIDLocked(l.ID) + c.mu.Unlock() + return list.List{}, fmt.Errorf("save list: %w", err) + } + dataevent.Emit("list", l.ID) // goal 0017: live-sync every open surface + return l, nil +} + +func (c *ConfigureService) UpdateList(id, label, description string, columns []typedfield.Field) (list.List, error) { + c.mu.Lock() + idx := c.findListLocked(id) + if idx == -1 { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no list with id %q", id) + } + previous := c.lists[idx] + l := previous + // CreatedAt is preserved from the stored entity, never trusted from + // the wire; UpdatedAt always advances on a real update. + l.Label, l.Description, l.Columns = label, description, columns + l.UpdatedAt = time.Now() + if err := list.Validate(l); err != nil { + c.mu.Unlock() + return list.List{}, err + } + c.lists[idx] = l + c.mu.Unlock() + + if err := c.persistLists(); err != nil { + c.mu.Lock() + c.revertListLocked(previous) + c.mu.Unlock() + return list.List{}, fmt.Errorf("save list: %w", err) + } + dataevent.Emit("list", l.ID) // goal 0017: live-sync every open surface + return l, nil +} + +// AddListRow appends a new, Active row to a List, minting its ID here +// (row-ID generation stays a service-layer concern, same as List IDs +// themselves via seeding.NewSlugID -- internal/domain/list stays pure +// per .claude/rules/backend.md). +func (c *ConfigureService) AddListRow(listID string, values map[string]string) (list.List, error) { + c.mu.Lock() + idx := c.findListLocked(listID) + if idx == -1 { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no list with id %q", listID) + } + previous := c.lists[idx] + now := time.Now() + row := list.Row{ + ID: seeding.NewSlugID("", "row"), Values: values, + CreatedAt: now, UpdatedAt: now, Status: list.RowActive, + } + l := previous + l.Rows = append(append([]list.Row{}, l.Rows...), row) + l.UpdatedAt = now + if err := list.Validate(l); err != nil { + c.mu.Unlock() + return list.List{}, err + } + c.lists[idx] = l + c.mu.Unlock() + + if err := c.persistLists(); err != nil { + c.mu.Lock() + c.revertListLocked(previous) + c.mu.Unlock() + return list.List{}, fmt.Errorf("save list: %w", err) + } + dataevent.Emit("list", l.ID) // goal 0017: live-sync every open surface + return l, nil +} + +// UpdateListRow replaces one row's Values/Status (its ID/CreatedAt +// stay put; UpdatedAt is stamped here, not client-supplied). +func (c *ConfigureService) UpdateListRow(listID, rowID string, values map[string]string, status list.RowStatus) (list.List, error) { + c.mu.Lock() + idx := c.findListLocked(listID) + if idx == -1 { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no list with id %q", listID) + } + previous := c.lists[idx] + l := previous + rowIdx := -1 + for i, r := range l.Rows { + if r.ID == rowID { + rowIdx = i + break + } + } + if rowIdx == -1 { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no row with id %q in list %q", rowID, listID) + } + if status == "" { + status = list.RowActive + } + now := time.Now() + rows := append([]list.Row{}, l.Rows...) + rows[rowIdx].Values = values + rows[rowIdx].Status = status + rows[rowIdx].UpdatedAt = now + l.Rows = rows + l.UpdatedAt = now + if err := list.Validate(l); err != nil { + c.mu.Unlock() + return list.List{}, err + } + c.lists[idx] = l + c.mu.Unlock() + + if err := c.persistLists(); err != nil { + c.mu.Lock() + c.revertListLocked(previous) + c.mu.Unlock() + return list.List{}, fmt.Errorf("save list: %w", err) + } + dataevent.Emit("list", l.ID) // goal 0017: live-sync every open surface + return l, nil +} + +func (c *ConfigureService) DeleteListRow(listID, rowID string) (list.List, error) { + c.mu.Lock() + idx := c.findListLocked(listID) + if idx == -1 { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no list with id %q", listID) + } + previous := c.lists[idx] + l := previous + rows := make([]list.Row, 0, len(l.Rows)) + found := false + for _, r := range l.Rows { + if r.ID == rowID { + found = true + continue + } + rows = append(rows, r) + } + if !found { + c.mu.Unlock() + return list.List{}, fmt.Errorf("no row with id %q in list %q", rowID, listID) + } + l.Rows = rows + l.UpdatedAt = time.Now() + c.lists[idx] = l + c.mu.Unlock() + + if err := c.persistLists(); err != nil { + c.mu.Lock() + c.revertListLocked(previous) + c.mu.Unlock() + return list.List{}, fmt.Errorf("save list: %w", err) + } + dataevent.Emit("list", l.ID) // goal 0017: live-sync every open surface + return l, nil +} + +func (c *ConfigureService) DeleteList(id string) error { + c.mu.Lock() + idx := -1 + for i, l := range c.lists { + if l.ID == id { + idx = i + break + } + } + if idx == -1 { + c.mu.Unlock() + return fmt.Errorf("no list with id %q", id) + } + removed := c.lists[idx] + wasBuiltIn := removed.BuiltIn + c.lists = append(c.lists[:idx], c.lists[idx+1:]...) + c.mu.Unlock() + + // A deleted built-in gets a tombstone so top-up seeding never + // resurrects it (topUpBuiltInLists, configureservice_builtin.go) -- + // same discipline DeleteHTTPRequest/DeleteDecision already apply. + // Removal and tombstone must succeed together (docs/goals/0025 item + // 2): an untombstoned removal would silently come back on the next + // restart's top-up seeding. + if wasBuiltIn { + if err := seeding.RecordTombstone(c.store, id); err != nil { + c.mu.Lock() + c.lists = insertListAt(c.lists, idx, removed) + c.mu.Unlock() + return fmt.Errorf("tombstone deleted list %q: %w", id, err) + } + } + if err := c.persistLists(); err != nil { + c.mu.Lock() + c.lists = insertListAt(c.lists, idx, removed) + c.mu.Unlock() + return fmt.Errorf("save list deletion: %w", err) + } + dataevent.Emit("list", id) // goal 0017: live-sync every open surface + return nil +} + +// insertListAt reinserts l at idx (clamped to the current length) -- +// used to undo DeleteList's removal when the tombstone or persist step +// that must accompany it fails. +func insertListAt(lists []list.List, idx int, l list.List) []list.List { + if idx < 0 || idx > len(lists) { + idx = len(lists) + } + lists = append(lists, list.List{}) + copy(lists[idx+1:], lists[idx:]) + lists[idx] = l + return lists +} + +// --- persistence --- + +func (c *ConfigureService) persistLists() error { + c.mu.Lock() + lists := make([]list.List, len(c.lists)) + copy(lists, c.lists) + c.mu.Unlock() + + data, err := json.Marshal(lists) + if err != nil { + return fmt.Errorf("marshal lists: %w", err) + } + if err := c.store.Set(listsKey, string(data)); err != nil { + return fmt.Errorf("persist lists: %w", err) + } + return nil +} + +// migrateLegacyLists converts any pre-0011 flat key/value List +// (Columns/Rows empty, Entries populated) into the typed shape once, +// in place, and re-persists -- goal 0011's decided backward-compat +// approach (list.MigrateLegacyEntries's own doc comment has the full +// reasoning). A list that's already typed, or one that's genuinely +// empty, is left untouched. Runs on every restore() call, but the +// migration is idempotent (a list only ever qualifies once -- after +// migrating, it carries Columns, so the condition never fires again), +// so persisting again on a later restart is a cheap no-op. +func (c *ConfigureService) migrateLegacyLists() { + changed := false + for i, l := range c.lists { + if len(l.Columns) > 0 || len(l.Entries) == 0 { + continue + } + columns, rows := list.MigrateLegacyEntries(l.Entries, func() string { return seeding.NewSlugID("", "row") }) + c.lists[i].Columns = columns + c.lists[i].Rows = rows + changed = true + } + if changed { + // Startup migration, not a user-initiated RPC -- nothing to + // return the error to (this runs from restore()). Logged so a + // failure is diagnosable rather than silently dropped + // (docs/goals/0025 item 1's fire-and-forget bucket); worst + // case the migration simply re-runs identically on the next + // launch, since Entries itself is untouched by this function. + if err := c.persistLists(); err != nil { + slog.Error("failed to persist migrated legacy lists", "error", err) + } + } +} diff --git a/internal/services/configuresvc/configuremcpserver.go b/internal/services/configuresvc/configuremcpserver.go index 1134f1c4..a71b514d 100644 --- a/internal/services/configuresvc/configuremcpserver.go +++ b/internal/services/configuresvc/configuremcpserver.go @@ -8,6 +8,7 @@ import ( "github.com/alicoding/mill/internal/adapters/mcpclient" "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/mcpserver" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/seeding" ) @@ -64,6 +65,7 @@ func (c *ConfigureService) CreateMCPServer(label, command string, args []string) c.mu.Unlock() return mcpserver.MCPServer{}, fmt.Errorf("save MCP server: %w", err) } + dataevent.Emit("mcpserver", s.ID) // goal 0017: live-sync every open surface return s, nil } @@ -104,6 +106,7 @@ func (c *ConfigureService) UpdateMCPServer(id, label, command string, args []str c.mu.Unlock() return mcpserver.MCPServer{}, fmt.Errorf("save MCP server: %w", err) } + dataevent.Emit("mcpserver", s.ID) // goal 0017: live-sync every open surface return s, nil } @@ -144,6 +147,7 @@ func (c *ConfigureService) DeleteMCPServer(id string) error { c.mu.Unlock() return fmt.Errorf("save MCP server deletion: %w", err) } + dataevent.Emit("mcpserver", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/configuresvc/configureservice.go b/internal/services/configuresvc/configureservice.go index 95bf031e..c67dfad5 100644 --- a/internal/services/configuresvc/configureservice.go +++ b/internal/services/configuresvc/configureservice.go @@ -16,9 +16,7 @@ import ( "github.com/alicoding/mill/internal/domain/httprequest" "github.com/alicoding/mill/internal/domain/list" "github.com/alicoding/mill/internal/domain/mcpserver" - "github.com/alicoding/mill/internal/domain/typedfield" "github.com/alicoding/mill/internal/services/compositionsvc" - "github.com/alicoding/mill/internal/services/seeding" ) // validateOpenAPISpec rejects an HTTPRequest save whose OpenAPISpec @@ -95,295 +93,9 @@ func NewConfigureService(store settings.Store, comp *compositionsvc.CompositionS return c } -// resolveList implements composition.go's lookupListFn seam. Entries -// is list.DeriveEntries's own computed key/value view (goal 0011) -- -// list-lookup keeps reading a flat map with zero changes to its own -// execution logic; Columns/Rows are the typed additions list-search -// reads. -func (c *ConfigureService) resolveList(id string) (composition.ResolvedList, error) { - c.mu.Lock() - defer c.mu.Unlock() - for _, l := range c.lists { - if l.ID == id { - return composition.ResolvedList{ - Entries: list.DeriveEntries(l), - Columns: l.Columns, - Rows: l.Rows, - }, nil - } - } - return composition.ResolvedList{}, fmt.Errorf("no list with id %q", id) -} - -// --- Lists --- - -func (c *ConfigureService) Lists() []list.List { - c.mu.Lock() - defer c.mu.Unlock() - out := make([]list.List, len(c.lists)) - copy(out, c.lists) - return out -} - -// findListLocked returns the index of the list with id in c.lists, or -// -1 -- caller must hold c.mu. -func (c *ConfigureService) findListLocked(id string) int { - for i, l := range c.lists { - if l.ID == id { - return i - } - } - return -1 -} - -// removeListByIDLocked removes the list with id from c.lists, if -// present -- caller must hold c.mu. -func (c *ConfigureService) removeListByIDLocked(id string) { - if idx := c.findListLocked(id); idx != -1 { - c.lists = append(c.lists[:idx], c.lists[idx+1:]...) - } -} - -// revertListLocked restores previous (keyed by its own ID) into -// c.lists -- caller must hold c.mu. Shared undo path for every List/ -// row mutation below's persist-failure revert (docs/goals/0025 item -// 2's memory-vs-store rule, extended here from CreateList/UpdateList -// to the new row-level mutations for the same reason: a phantom -// in-memory row a restart would silently drop is exactly as wrong as -// a phantom list). -func (c *ConfigureService) revertListLocked(previous list.List) { - if idx := c.findListLocked(previous.ID); idx != -1 { - c.lists[idx] = previous - } -} - -func (c *ConfigureService) CreateList(label, description string, columns []typedfield.Field) (list.List, error) { - now := time.Now() - l := list.List{ - ID: seeding.NewSlugID(label, "list"), Label: label, Description: description, - Columns: columns, CreatedAt: now, UpdatedAt: now, - } - if err := list.Validate(l); err != nil { - return list.List{}, err - } - - c.mu.Lock() - c.lists = append(c.lists, l) - c.mu.Unlock() - - if err := c.persistLists(); err != nil { - // Don't leave a phantom-saved list in memory that a restart - // would drop (docs/goals/0025 item 2's memory-vs-store rule). - c.mu.Lock() - c.removeListByIDLocked(l.ID) - c.mu.Unlock() - return list.List{}, fmt.Errorf("save list: %w", err) - } - return l, nil -} - -func (c *ConfigureService) UpdateList(id, label, description string, columns []typedfield.Field) (list.List, error) { - c.mu.Lock() - idx := c.findListLocked(id) - if idx == -1 { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no list with id %q", id) - } - previous := c.lists[idx] - l := previous - // CreatedAt is preserved from the stored entity, never trusted from - // the wire; UpdatedAt always advances on a real update. - l.Label, l.Description, l.Columns = label, description, columns - l.UpdatedAt = time.Now() - if err := list.Validate(l); err != nil { - c.mu.Unlock() - return list.List{}, err - } - c.lists[idx] = l - c.mu.Unlock() - - if err := c.persistLists(); err != nil { - c.mu.Lock() - c.revertListLocked(previous) - c.mu.Unlock() - return list.List{}, fmt.Errorf("save list: %w", err) - } - return l, nil -} - -// AddListRow appends a new, Active row to a List, minting its ID here -// (row-ID generation stays a service-layer concern, same as List IDs -// themselves via seeding.NewSlugID -- internal/domain/list stays pure -// per .claude/rules/backend.md). -func (c *ConfigureService) AddListRow(listID string, values map[string]string) (list.List, error) { - c.mu.Lock() - idx := c.findListLocked(listID) - if idx == -1 { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no list with id %q", listID) - } - previous := c.lists[idx] - now := time.Now() - row := list.Row{ - ID: seeding.NewSlugID("", "row"), Values: values, - CreatedAt: now, UpdatedAt: now, Status: list.RowActive, - } - l := previous - l.Rows = append(append([]list.Row{}, l.Rows...), row) - l.UpdatedAt = now - if err := list.Validate(l); err != nil { - c.mu.Unlock() - return list.List{}, err - } - c.lists[idx] = l - c.mu.Unlock() - - if err := c.persistLists(); err != nil { - c.mu.Lock() - c.revertListLocked(previous) - c.mu.Unlock() - return list.List{}, fmt.Errorf("save list: %w", err) - } - return l, nil -} - -// UpdateListRow replaces one row's Values/Status (its ID/CreatedAt -// stay put; UpdatedAt is stamped here, not client-supplied). -func (c *ConfigureService) UpdateListRow(listID, rowID string, values map[string]string, status list.RowStatus) (list.List, error) { - c.mu.Lock() - idx := c.findListLocked(listID) - if idx == -1 { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no list with id %q", listID) - } - previous := c.lists[idx] - l := previous - rowIdx := -1 - for i, r := range l.Rows { - if r.ID == rowID { - rowIdx = i - break - } - } - if rowIdx == -1 { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no row with id %q in list %q", rowID, listID) - } - if status == "" { - status = list.RowActive - } - now := time.Now() - rows := append([]list.Row{}, l.Rows...) - rows[rowIdx].Values = values - rows[rowIdx].Status = status - rows[rowIdx].UpdatedAt = now - l.Rows = rows - l.UpdatedAt = now - if err := list.Validate(l); err != nil { - c.mu.Unlock() - return list.List{}, err - } - c.lists[idx] = l - c.mu.Unlock() - - if err := c.persistLists(); err != nil { - c.mu.Lock() - c.revertListLocked(previous) - c.mu.Unlock() - return list.List{}, fmt.Errorf("save list: %w", err) - } - return l, nil -} - -func (c *ConfigureService) DeleteListRow(listID, rowID string) (list.List, error) { - c.mu.Lock() - idx := c.findListLocked(listID) - if idx == -1 { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no list with id %q", listID) - } - previous := c.lists[idx] - l := previous - rows := make([]list.Row, 0, len(l.Rows)) - found := false - for _, r := range l.Rows { - if r.ID == rowID { - found = true - continue - } - rows = append(rows, r) - } - if !found { - c.mu.Unlock() - return list.List{}, fmt.Errorf("no row with id %q in list %q", rowID, listID) - } - l.Rows = rows - l.UpdatedAt = time.Now() - c.lists[idx] = l - c.mu.Unlock() - - if err := c.persistLists(); err != nil { - c.mu.Lock() - c.revertListLocked(previous) - c.mu.Unlock() - return list.List{}, fmt.Errorf("save list: %w", err) - } - return l, nil -} - -func (c *ConfigureService) DeleteList(id string) error { - c.mu.Lock() - idx := -1 - for i, l := range c.lists { - if l.ID == id { - idx = i - break - } - } - if idx == -1 { - c.mu.Unlock() - return fmt.Errorf("no list with id %q", id) - } - removed := c.lists[idx] - wasBuiltIn := removed.BuiltIn - c.lists = append(c.lists[:idx], c.lists[idx+1:]...) - c.mu.Unlock() - - // A deleted built-in gets a tombstone so top-up seeding never - // resurrects it (topUpBuiltInLists, configureservice_builtin.go) -- - // same discipline DeleteHTTPRequest/DeleteDecision already apply. - // Removal and tombstone must succeed together (docs/goals/0025 item - // 2): an untombstoned removal would silently come back on the next - // restart's top-up seeding. - if wasBuiltIn { - if err := seeding.RecordTombstone(c.store, id); err != nil { - c.mu.Lock() - c.lists = insertListAt(c.lists, idx, removed) - c.mu.Unlock() - return fmt.Errorf("tombstone deleted list %q: %w", id, err) - } - } - if err := c.persistLists(); err != nil { - c.mu.Lock() - c.lists = insertListAt(c.lists, idx, removed) - c.mu.Unlock() - return fmt.Errorf("save list deletion: %w", err) - } - return nil -} - -// insertListAt reinserts l at idx (clamped to the current length) -- -// used to undo DeleteList's removal when the tombstone or persist step -// that must accompany it fails. -func insertListAt(lists []list.List, idx int, l list.List) []list.List { - if idx < 0 || idx > len(lists) { - idx = len(lists) - } - lists = append(lists, list.List{}) - copy(lists[idx+1:], lists[idx:]) - lists[idx] = l - return lists -} +// resolveList/Lists/CreateList/UpdateList/AddListRow/UpdateListRow/ +// DeleteListRow/DeleteList/persistLists/migrateLegacyLists live in +// configurelist.go (goal 0017 split, see that file's header comment). // --- Attributes (delegates to CompositionService -- see SPEC.md §3.5's // "Configure-authored but workflow-scoped" cardinality note) --- @@ -392,24 +104,6 @@ func (c *ConfigureService) UpdateWorkflowAttributes(workflowID string, attrs []c return c.composition.UpdateAttributes(workflowID, attrs) } -// --- persistence --- - -func (c *ConfigureService) persistLists() error { - c.mu.Lock() - lists := make([]list.List, len(c.lists)) - copy(lists, c.lists) - c.mu.Unlock() - - data, err := json.Marshal(lists) - if err != nil { - return fmt.Errorf("marshal lists: %w", err) - } - if err := c.store.Set(listsKey, string(data)); err != nil { - return fmt.Errorf("persist lists: %w", err) - } - return nil -} - // restore loads persisted HTTPRequests/Lists. HTTPRequests has three // cases, checked in order (ADR-0016's migration plan): (1) requestsKey // already has data -- the common case after this migration has run @@ -460,36 +154,3 @@ func (c *ConfigureService) restore() { } } } - -// migrateLegacyLists converts any pre-0011 flat key/value List -// (Columns/Rows empty, Entries populated) into the typed shape once, -// in place, and re-persists -- goal 0011's decided backward-compat -// approach (list.MigrateLegacyEntries's own doc comment has the full -// reasoning). A list that's already typed, or one that's genuinely -// empty, is left untouched. Runs on every restore() call, but the -// migration is idempotent (a list only ever qualifies once -- after -// migrating, it carries Columns, so the condition never fires again), -// so persisting again on a later restart is a cheap no-op. -func (c *ConfigureService) migrateLegacyLists() { - changed := false - for i, l := range c.lists { - if len(l.Columns) > 0 || len(l.Entries) == 0 { - continue - } - columns, rows := list.MigrateLegacyEntries(l.Entries, func() string { return seeding.NewSlugID("", "row") }) - c.lists[i].Columns = columns - c.lists[i].Rows = rows - changed = true - } - if changed { - // Startup migration, not a user-initiated RPC -- nothing to - // return the error to (this runs from restore()). Logged so a - // failure is diagnosable rather than silently dropped - // (docs/goals/0025 item 1's fire-and-forget bucket); worst - // case the migration simply re-runs identically on the next - // launch, since Entries itself is untouched by this function. - if err := c.persistLists(); err != nil { - slog.Error("failed to persist migrated legacy lists", "error", err) - } - } -} diff --git a/internal/services/configuresvc/configureservice_dataevent_test.go b/internal/services/configuresvc/configureservice_dataevent_test.go new file mode 100644 index 00000000..c42ae911 --- /dev/null +++ b/internal/services/configuresvc/configureservice_dataevent_test.go @@ -0,0 +1,211 @@ +package configuresvc + +import ( + "testing" + + "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/domain/decision" + "github.com/alicoding/mill/internal/domain/execenv" + "github.com/alicoding/mill/internal/domain/httprequest" + "github.com/alicoding/mill/internal/services/dataevent" +) + +// captureEmits mirrors compositionsvc's own helper of the same name +// (see that package's compositionservice_dataevent_test.go for the +// full seam reasoning: application.Get() is always nil under `go +// test`, so dataevent.TestHook is the only way to observe an Emit +// call). Package-local since dataevent.TestHook is shared, mutable +// package state -- every test using it must t.Cleanup it back to nil. +func captureEmits(t *testing.T) *[]dataevent.Changed { + t.Helper() + var got []dataevent.Changed + dataevent.TestHook = func(entity, id string) { + got = append(got, dataevent.Changed{Entity: entity, ID: id}) + } + t.Cleanup(func() { dataevent.TestHook = nil }) + return &got +} + +func assertEmitted(t *testing.T, got []dataevent.Changed, entity, id string) { + t.Helper() + for _, c := range got { + if c.Entity == entity && c.ID == id { + return + } + } + t.Errorf("dataevent.Emit(%q, %q) was not observed; got %+v", entity, id, got) +} + +// TestDataEvent_RequestMutations proves goal 0017's P0-2 for +// HTTPRequests: Create/Update/Delete all emit +// mill-data-changed{entity:"request"}. +func TestDataEvent_RequestMutations(t *testing.T) { + cfg, _ := newTestConfigureService(t) + + got := captureEmits(t) + req, err := cfg.CreateHTTPRequest("Emit test request", "https://example.com", "GET", "", httprequest.AuthNone, nil, "", nil, nil, "") + if err != nil { + t.Fatalf("CreateHTTPRequest: %v", err) + } + assertEmitted(t, *got, "request", req.ID) + + got = captureEmits(t) + updated, err := cfg.UpdateHTTPRequest(req.ID, "Emit test request (edited)", "https://example.com", "GET", "", httprequest.AuthNone, nil, "", nil, nil, "") + if err != nil { + t.Fatalf("UpdateHTTPRequest: %v", err) + } + assertEmitted(t, *got, "request", updated.ID) + + got = captureEmits(t) + if err := cfg.DeleteHTTPRequest(req.ID); err != nil { + t.Fatalf("DeleteHTTPRequest: %v", err) + } + assertEmitted(t, *got, "request", req.ID) +} + +// TestDataEvent_ListMutations proves goal 0017's P0-2 for Lists: +// Create/Update/AddRow/UpdateRow/DeleteRow/Delete all emit +// mill-data-changed{entity:"list"}. +func TestDataEvent_ListMutations(t *testing.T) { + cfg, _ := newTestConfigureService(t) + + got := captureEmits(t) + l, err := cfg.CreateList("Emit test list", "", nil) + if err != nil { + t.Fatalf("CreateList: %v", err) + } + assertEmitted(t, *got, "list", l.ID) + + got = captureEmits(t) + l, err = cfg.UpdateList(l.ID, "Emit test list (edited)", "", nil) + if err != nil { + t.Fatalf("UpdateList: %v", err) + } + assertEmitted(t, *got, "list", l.ID) + + got = captureEmits(t) + l, err = cfg.AddListRow(l.ID, map[string]string{"k": "v"}) + if err != nil { + t.Fatalf("AddListRow: %v", err) + } + assertEmitted(t, *got, "list", l.ID) + rowID := l.Rows[0].ID + + got = captureEmits(t) + l, err = cfg.UpdateListRow(l.ID, rowID, map[string]string{"k": "v2"}, "") + if err != nil { + t.Fatalf("UpdateListRow: %v", err) + } + assertEmitted(t, *got, "list", l.ID) + + got = captureEmits(t) + l, err = cfg.DeleteListRow(l.ID, rowID) + if err != nil { + t.Fatalf("DeleteListRow: %v", err) + } + assertEmitted(t, *got, "list", l.ID) + + got = captureEmits(t) + if err := cfg.DeleteList(l.ID); err != nil { + t.Fatalf("DeleteList: %v", err) + } + assertEmitted(t, *got, "list", l.ID) +} + +// TestDataEvent_MCPServerMutations proves goal 0017's P0-2 for +// configured MCP Servers. +func TestDataEvent_MCPServerMutations(t *testing.T) { + cfg, _ := newTestConfigureService(t) + + got := captureEmits(t) + s, err := cfg.CreateMCPServer("Emit test server", "echo", nil) + if err != nil { + t.Fatalf("CreateMCPServer: %v", err) + } + assertEmitted(t, *got, "mcpserver", s.ID) + + got = captureEmits(t) + s, err = cfg.UpdateMCPServer(s.ID, "Emit test server (edited)", "echo", nil) + if err != nil { + t.Fatalf("UpdateMCPServer: %v", err) + } + assertEmitted(t, *got, "mcpserver", s.ID) + + got = captureEmits(t) + if err := cfg.DeleteMCPServer(s.ID); err != nil { + t.Fatalf("DeleteMCPServer: %v", err) + } + assertEmitted(t, *got, "mcpserver", s.ID) +} + +// TestDataEvent_DecisionMutations proves goal 0017's P0-2 for +// Decisions -- "decision" is a NEW entity string on the wire, not +// covered by App.tsx's routing before this goal. +func TestDataEvent_DecisionMutations(t *testing.T) { + cfg, _ := newTestConfigureService(t) + + got := captureEmits(t) + d, err := cfg.CreateDecision("Emit test decision", decision.CategoryApprove, nil, "") + if err != nil { + t.Fatalf("CreateDecision: %v", err) + } + assertEmitted(t, *got, "decision", d.ID) + + got = captureEmits(t) + d, err = cfg.UpdateDecision(d.ID, "Emit test decision (edited)", decision.CategoryApprove, nil, "") + if err != nil { + t.Fatalf("UpdateDecision: %v", err) + } + assertEmitted(t, *got, "decision", d.ID) + + got = captureEmits(t) + if err := cfg.DeleteDecision(d.ID); err != nil { + t.Fatalf("DeleteDecision: %v", err) + } + assertEmitted(t, *got, "decision", d.ID) +} + +// TestDataEvent_ExecEnvMutations proves goal 0017's P0-2 for Execution +// Environments -- "execenv" is a NEW entity string on the wire. +func TestDataEvent_ExecEnvMutations(t *testing.T) { + cfg, _ := newTestConfigureService(t) + + got := captureEmits(t) + e, err := cfg.CreateExecEnv("Emit test execenv", execenv.ShellBash, execenv.ProfileClean, execenv.TempDirSentinel, nil) + if err != nil { + t.Fatalf("CreateExecEnv: %v", err) + } + assertEmitted(t, *got, "execenv", e.ID) + + got = captureEmits(t) + e, err = cfg.UpdateExecEnv(e.ID, "Emit test execenv (edited)", execenv.ShellBash, execenv.ProfileClean, execenv.TempDirSentinel, nil) + if err != nil { + t.Fatalf("UpdateExecEnv: %v", err) + } + assertEmitted(t, *got, "execenv", e.ID) + + got = captureEmits(t) + if err := cfg.DeleteExecEnv(e.ID); err != nil { + t.Fatalf("DeleteExecEnv: %v", err) + } + assertEmitted(t, *got, "execenv", e.ID) +} + +// TestDataEvent_UpdateWorkflowAttributes_DelegatesToComposition proves +// goal 0017's "workflow-attributes changes" item: ConfigureService's +// delegate emits "workflow" via CompositionService.UpdateAttributes, +// not a second, separate emit -- one source of truth for the workflow +// entity's live-sync event. +func TestDataEvent_UpdateWorkflowAttributes_DelegatesToComposition(t *testing.T) { + cfg, comp := newTestConfigureService(t) + wf, err := comp.CreateWorkflow("Attrs-target wf", "", []composition.Node{{ID: "t", NodeTypeID: "trigger-manual"}}, nil) + if err != nil { + t.Fatalf("CreateWorkflow: %v", err) + } + + got := captureEmits(t) + if _, err := cfg.UpdateWorkflowAttributes(wf.ID, nil); err != nil { + t.Fatalf("UpdateWorkflowAttributes: %v", err) + } + assertEmitted(t, *got, "workflow", wf.ID) +} diff --git a/internal/services/configuresvc/configureservice_requestauth.go b/internal/services/configuresvc/configureservice_requestauth.go index 5bcd75db..f16f9c42 100644 --- a/internal/services/configuresvc/configureservice_requestauth.go +++ b/internal/services/configuresvc/configureservice_requestauth.go @@ -9,6 +9,7 @@ import ( "github.com/alicoding/mill/internal/adapters/openapispec" "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/httprequest" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/seeding" ) @@ -134,6 +135,7 @@ func (c *ConfigureService) CreateHTTPRequest(label, baseURL, method, body string c.mu.Unlock() return httprequest.HTTPRequest{}, fmt.Errorf("save request: %w", err) } + dataevent.Emit("request", req.ID) // goal 0017: live-sync every open surface return req, nil } @@ -185,6 +187,7 @@ func (c *ConfigureService) UpdateHTTPRequest(id, label, baseURL, method, body st c.mu.Unlock() return httprequest.HTTPRequest{}, fmt.Errorf("save request: %w", err) } + dataevent.Emit("request", req.ID) // goal 0017: live-sync every open surface return req, nil } @@ -230,6 +233,7 @@ func (c *ConfigureService) DeleteHTTPRequest(id string) error { } _ = c.credentials.Delete(id) _ = c.credentials.Delete(joseKeychainID(id)) + dataevent.Emit("request", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/dataevent/dataevent.go b/internal/services/dataevent/dataevent.go new file mode 100644 index 00000000..a5c18eb1 --- /dev/null +++ b/internal/services/dataevent/dataevent.go @@ -0,0 +1,60 @@ +// Package dataevent is the ONE shared emit point for Mill's live-sync +// event (docs/adr/0025, goal 0017): every direct-mutation service -- +// mcpsvc (external MCP authoring), compositionsvc, configuresvc, +// guardrailsvc -- calls Emit after a successful write so every open +// Mill surface refreshes what just changed, instead of only the +// MCP-authored path doing so (goal 0017's audit root cause: before +// this package existed, mcpsvc alone defined and emitted this event, +// so a plain UI create/edit never reached another open tab/picker). +// A tiny package of its own, not folded into mcpsvc or compositionsvc, +// because mcpsvc already imports compositionsvc/configuresvc +// (millmcpservice.go) -- those two importing back to reuse mcpsvc's +// old DataChanged type would be circular; this sits below all four so +// each can import it (.claude/rules/backend.md's shared-cross-service- +// helper convention, same shape as internal/services/seeding). +package dataevent + +import "github.com/wailsapp/wails/v3/pkg/application" + +// Changed is the live-sync event payload: which kind of entity changed +// (e.g. "workflow", "request", "list", "mcpserver", "decision", +// "execenv", "guardrail-rule", "run") and its ID. +type Changed struct { + Entity string `json:"entity"` + ID string `json:"id"` +} + +// EventName is registered by main.go (application.RegisterEvent) and +// listened for in frontend/src/app/App.tsx via Events.On. Exported as +// the single source of the wire name -- no other file should spell it +// as a string literal. +const EventName = "mill-data-changed" + +// Emit fires EventName with the given entity/id, a no-op when no Wails +// application is running (e.g. a plain `go test` of a service package +// with no application.New ever called) -- application.Get() returns +// nil in that case, same defensive check mcpsvc's original emit +// helper already had. TestHook (below) is invoked unconditionally +// alongside it, so a mutating method's test can prove "this emits" +// without spinning up a real Wails application. +func Emit(entity, id string) { + if app := application.Get(); app != nil { + app.Event.Emit(EventName, Changed{Entity: entity, ID: id}) + } + if TestHook != nil { + TestHook(entity, id) + } +} + +// TestHook, when non-nil, is invoked by every Emit call with the same +// (entity, id) -- the ONE seam every mutating service's test uses +// (compositionsvc/configuresvc/guardrailsvc/mcpsvc) to assert "this +// method emits mill-data-changed" (goal 0017's DoD): application.Get() +// always returns nil under `go test` (no real Wails application is +// ever constructed there), so the production app.Event.Emit path is +// silently unobservable -- this hook is the alternative, same shape as +// servicetest.FakeStore.SetErr being the seam for persist-failure +// tests. Package-level and shared across a test binary, so a test that +// sets it MUST restore it to nil via t.Cleanup before returning, +// otherwise a later, unrelated test in the same package would trip it. +var TestHook func(entity, id string) diff --git a/internal/services/guardrailsvc/guardrailservice.go b/internal/services/guardrailsvc/guardrailservice.go index a6b8bc09..30fdefd8 100644 --- a/internal/services/guardrailsvc/guardrailservice.go +++ b/internal/services/guardrailsvc/guardrailservice.go @@ -9,6 +9,7 @@ import ( "github.com/alicoding/mill/internal/domain/composition" "github.com/alicoding/mill/internal/domain/guardrail" "github.com/alicoding/mill/internal/services/compositionsvc" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/google/uuid" ) @@ -83,6 +84,7 @@ func (g *GuardrailService) CreateRule(rule guardrail.Rule) (guardrail.Rule, erro g.rules = g.rules[:len(g.rules)-1] return guardrail.Rule{}, fmt.Errorf("save guardrail rule: %w", err) } + dataevent.Emit("guardrail-rule", rule.ID) // goal 0017: live-sync every open surface return rule, nil } @@ -102,6 +104,7 @@ func (g *GuardrailService) UpdateRule(rule guardrail.Rule) error { g.rules[i] = previous return fmt.Errorf("save guardrail rule: %w", err) } + dataevent.Emit("guardrail-rule", rule.ID) // goal 0017: live-sync every open surface return nil } } @@ -133,6 +136,7 @@ func (g *GuardrailService) DeleteRule(id string) error { g.rules[idx] = removed return fmt.Errorf("save guardrail rule deletion: %w", err) } + dataevent.Emit("guardrail-rule", id) // goal 0017: live-sync every open surface return nil } diff --git a/internal/services/guardrailsvc/guardrailservice_dataevent_test.go b/internal/services/guardrailsvc/guardrailservice_dataevent_test.go new file mode 100644 index 00000000..99ed27cd --- /dev/null +++ b/internal/services/guardrailsvc/guardrailservice_dataevent_test.go @@ -0,0 +1,63 @@ +package guardrailsvc + +import ( + "testing" + + "github.com/alicoding/mill/internal/domain/guardrail" + "github.com/alicoding/mill/internal/services/compositionsvc" + "github.com/alicoding/mill/internal/services/dataevent" + "github.com/alicoding/mill/internal/services/servicetest" +) + +// captureEmits mirrors compositionsvc's/configuresvc's own helper of +// the same name (see compositionsvc/compositionservice_dataevent_test.go +// for the full seam reasoning). +func captureEmits(t *testing.T) *[]dataevent.Changed { + t.Helper() + var got []dataevent.Changed + dataevent.TestHook = func(entity, id string) { + got = append(got, dataevent.Changed{Entity: entity, ID: id}) + } + t.Cleanup(func() { dataevent.TestHook = nil }) + return &got +} + +func assertEmitted(t *testing.T, got []dataevent.Changed, entity, id string) { + t.Helper() + for _, c := range got { + if c.Entity == entity && c.ID == id { + return + } + } + t.Errorf("dataevent.Emit(%q, %q) was not observed; got %+v", entity, id, got) +} + +// TestDataEvent_RuleMutations proves goal 0017's P0-3: guardrail rule +// CRUD emits a NEW "guardrail-rule" entity on mill-data-changed, so a +// canvas open in another tab re-runs its verdict badges when a rule +// changes elsewhere. +func TestDataEvent_RuleMutations(t *testing.T) { + store := servicetest.NewFakeStore() + comp := compositionsvc.NewCompositionService(store) + g := NewGuardrailService(store, comp) + + got := captureEmits(t) + rule, err := g.CreateRule(guardrail.Rule{Label: "Emit test rule", Effect: guardrail.EffectAllow, NodeTypeID: "list-lookup"}) + if err != nil { + t.Fatalf("CreateRule: %v", err) + } + assertEmitted(t, *got, "guardrail-rule", rule.ID) + + got = captureEmits(t) + rule.Label = "Emit test rule (edited)" + if err := g.UpdateRule(rule); err != nil { + t.Fatalf("UpdateRule: %v", err) + } + assertEmitted(t, *got, "guardrail-rule", rule.ID) + + got = captureEmits(t) + if err := g.DeleteRule(rule.ID); err != nil { + t.Fatalf("DeleteRule: %v", err) + } + assertEmitted(t, *got, "guardrail-rule", rule.ID) +} diff --git a/internal/services/mcpsvc/millmcpservice_authoring.go b/internal/services/mcpsvc/millmcpservice_authoring.go index 25c2b2fc..22785986 100644 --- a/internal/services/mcpsvc/millmcpservice_authoring.go +++ b/internal/services/mcpsvc/millmcpservice_authoring.go @@ -6,9 +6,9 @@ import ( "fmt" "github.com/alicoding/mill/internal/domain/composition" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/executionsvc" "github.com/modelcontextprotocol/go-sdk/mcp" - "github.com/wailsapp/wails/v3/pkg/application" ) // The LLM-authoring tool tier (docs/adr/0025): an external MCP client @@ -24,25 +24,6 @@ import ( // resolve_approval is PERMANENTLY EXCLUDED by design, not omission: an // LLM approving its own guarded runs would collapse the guardrail. -// DataChanged is the live-sync event (docs/adr/0025): emitted after -// any MCP-driven mutation so an open Mill window refreshes what the -// LLM just changed -- §1's what-you-see-is-what-I-see thesis running -// in both directions. -type DataChanged struct { - Entity string `json:"entity"` - ID string `json:"id"` -} - -// DataChangedEventName is registered by main.go (RegisterEvent) and -// listened for in App.tsx. -const DataChangedEventName = "mill-data-changed" - -func emitDataChanged(entity, id string) { - if app := application.Get(); app != nil { - app.Event.Emit(DataChangedEventName, DataChanged{Entity: entity, ID: id}) - } -} - // SetExecutionService late-binds the execution service for the // list_runs/get_run/run_workflow tools -- same late-bound-setter shape // as SettingsService.SetMCPService, for the same construction-order @@ -201,7 +182,10 @@ func (m *MillMCPService) registerAuthoringTools() { if err != nil { return "", err } - emitDataChanged("workflow", wf.ID) + // No manual dataevent.Emit here -- SnapshotDraft/UpdateWorkflow/ + // UpdateAttributes (compositionsvc) already emit "workflow" + // internally now (goal 0017), so an MCP-driven update_workflow + // still lands the exact same live-sync event it always did. return fmt.Sprintf("updated draft of %q (previous draft snapshotted as v%d)", wf.Label, len(wf.Versions)), nil }) mcp.AddTool(m.server, &mcp.Tool{ @@ -228,7 +212,9 @@ func (m *MillMCPService) registerAuthoringTools() { if err != nil { return "", err } - emitDataChanged("workflow", wf.ID) + // PublishWorkflow (compositionsvc, via mutateWorkflow) already + // emits "workflow" -- see the update_workflow executor's comment + // above. return fmt.Sprintf("published %q as v%d (live)", wf.Label, wf.PublishedVersion), nil }) mcp.AddTool(m.server, &mcp.Tool{ @@ -254,7 +240,8 @@ func (m *MillMCPService) registerAuthoringTools() { if err := m.comp.DeleteWorkflow(in.ID); err != nil { return "", err } - emitDataChanged("workflow", in.ID) + // DeleteWorkflow (compositionsvc) already emits "workflow" -- see + // the update_workflow executor's comment above. return "deleted", nil }) mcp.AddTool(m.server, &mcp.Tool{ @@ -290,7 +277,7 @@ func (m *MillMCPService) registerAuthoringTools() { if err != nil { return nil, nil, err } - emitDataChanged("run", summary.RunID) + dataevent.Emit("run", summary.RunID) res, err := jsonResult(summary) return res, nil, err }) diff --git a/internal/services/mcpsvc/millmcpservice_debug.go b/internal/services/mcpsvc/millmcpservice_debug.go index 4998218b..3fcc1b7e 100644 --- a/internal/services/mcpsvc/millmcpservice_debug.go +++ b/internal/services/mcpsvc/millmcpservice_debug.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/alicoding/mill/internal/domain/guardrail" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -74,7 +75,7 @@ func (m *MillMCPService) registerDebugTools() { if err != nil { return nil, nil, err } - emitDataChanged("run", summary.RunID) + dataevent.Emit("run", summary.RunID) res, err := jsonResult(summary) return res, nil, err }) @@ -96,7 +97,7 @@ func (m *MillMCPService) registerDebugTools() { if err := m.exec.ResolveApproval(in.RunID, nodeID, true, nil, false); err != nil { return nil, nil, err } - emitDataChanged("run", in.RunID) + dataevent.Emit("run", in.RunID) return textResult(fmt.Sprintf("stepped past %s -- use get_run to inspect it, or step_run/resume_run/stop_run again if it parked once more", nodeID)), nil, nil }) @@ -116,7 +117,7 @@ func (m *MillMCPService) registerDebugTools() { if err := m.exec.ResolveApproval(in.RunID, nodeID, true, nil, true); err != nil { return nil, nil, err } - emitDataChanged("run", in.RunID) + dataevent.Emit("run", in.RunID) return textResult(fmt.Sprintf("resumed past %s -- use get_run to see the final result (or the next breakpoint, if one is hit)", nodeID)), nil, nil }) @@ -136,7 +137,7 @@ func (m *MillMCPService) registerDebugTools() { if err := m.exec.ResolveApproval(in.RunID, nodeID, false, nil, false); err != nil { return nil, nil, err } - emitDataChanged("run", in.RunID) + dataevent.Emit("run", in.RunID) return textResult(fmt.Sprintf("stopped at %s", nodeID)), nil, nil }) } diff --git a/internal/services/mcpsvc/millmcpservice_tools.go b/internal/services/mcpsvc/millmcpservice_tools.go index 09f86b58..8921375a 100644 --- a/internal/services/mcpsvc/millmcpservice_tools.go +++ b/internal/services/mcpsvc/millmcpservice_tools.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -143,7 +144,9 @@ func (m *MillMCPService) registerTools() { if err != nil { return "", err } - emitDataChanged("workflow", wf.ID) + // ImportWorkflow delegates to CreateWorkflow (compositionsvc), + // which already emits "workflow" (goal 0017) -- no manual emit + // needed here. return jsonText(importToolResult{ID: wf.ID, Label: wf.Label}) }) mcp.AddTool(m.server, &mcp.Tool{ @@ -174,7 +177,8 @@ func (m *MillMCPService) registerTools() { if err != nil { return "", err } - emitDataChanged("request", r.ID) + // ImportHTTPRequest delegates to CreateHTTPRequest (configuresvc), + // which already emits "request" (goal 0017). return jsonText(importToolResult{ID: r.ID, Label: r.Label}) }) mcp.AddTool(m.server, &mcp.Tool{ @@ -204,7 +208,11 @@ func (m *MillMCPService) registerTools() { if err != nil { return "", err } - emitDataChanged("list", l.ID) + // Kept explicit (unlike the other import_* tools above): ImportList + // does a SECOND mutation after CreateList (attaching rows), which + // CreateList's own internal emit can't see -- this is the one that + // actually reflects rows being present. + dataevent.Emit("list", l.ID) return jsonText(importToolResult{ID: l.ID, Label: l.Label}) }) mcp.AddTool(m.server, &mcp.Tool{ @@ -233,7 +241,8 @@ func (m *MillMCPService) registerTools() { if err != nil { return "", err } - emitDataChanged("mcpserver", s.ID) + // ImportMCPServer delegates to CreateMCPServer (configuresvc), + // which already emits "mcpserver" (goal 0017). return jsonText(importToolResult{ID: s.ID, Label: s.Label}) }) mcp.AddTool(m.server, &mcp.Tool{ diff --git a/main.go b/main.go index 9959ec68..00cd3b1c 100644 --- a/main.go +++ b/main.go @@ -15,6 +15,7 @@ import ( "github.com/alicoding/mill/internal/services/capabilitysvc" "github.com/alicoding/mill/internal/services/compositionsvc" "github.com/alicoding/mill/internal/services/configuresvc" + "github.com/alicoding/mill/internal/services/dataevent" "github.com/alicoding/mill/internal/services/executionsvc" "github.com/alicoding/mill/internal/services/guardrailsvc" "github.com/alicoding/mill/internal/services/mcpsvc" @@ -53,7 +54,7 @@ func init() { application.RegisterEvent[triggersvc.HotkeyActivity]("hotkey-activity") application.RegisterEvent[mcpsvc.MCPWriteRequest]("mcp-write-approval") application.RegisterEvent[mcpsvc.MCPWriteActivity]("mcp-write-activity") - application.RegisterEvent[mcpsvc.DataChanged](mcpsvc.DataChangedEventName) + application.RegisterEvent[dataevent.Changed](dataevent.EventName) application.RegisterEvent[executionsvc.GuardrailPendingChanged]("guardrail-pending-changed") // docs/adr/0033: the Quick Panel's "Open Mill"/"Open Settings" rows // (OpenMainWindow) emit this so App.tsx can switch the store's view