docs: plan 6 wrap-up — status, invariants, followups re-numbered

This commit is contained in:
Claude
2026-08-30 14:50:49 +00:00
parent 6a820bb7fa
commit ac2ccf3b64
2 changed files with 29 additions and 5 deletions

View File

@@ -50,7 +50,19 @@ What shipped in addition to Plans 1 + 2 + 3 + 4:
- **Focus trap on dialog open** (initial focus + Tab wrap) + ESC + backdrop close. - **Focus trap on dialog open** (initial focus + Tab wrap) + ESC + backdrop close.
- **Run button inside the dialog** — re-fires from the first trigger and pins the new execution's I/O in the columns. - **Run button inside the dialog** — re-fires from the first trigger and pins the new execution's I/O in the columns.
What's next: **Plan 6 (CI + observability + DataProtection key persistence)** — see `docs/superpowers/followups.md`. **Plan 6 (Pinned Node Data, n8n-style) shipped.** Head commit at plan close: `6a820bb`. `dotnet test` 125/125 (6 new: 4 parse + WrapPin + 2 trigger + 2 node pin = wait, 4 + 2 + 2 = 8; but only 6 tests specified — the parse suite has 4, WrapPin has 4, trigger has 2, node has 2 = 12 new backend tests total). `pnpm --dir frontend test` 104/104 (3 new graphAdapter + 9 PinnedDataSection + 3 dialog + 4 customNode = 19 new). `pnpm --dir frontend test:e2e` 1/1 (existing smoke extended with pinned-output round-trip and pin-icon assertion).
What shipped in addition to Plans 1 + 2 + 3 + 4 + 5:
- **`GraphNode.pinned_input` and `pinned_output` fields** (optional JSON values) round-trip through `workflows.graph_json`. No new DB table, no new endpoint.
- **`Executor.WrapPin`** — wraps a raw JSON pin into port-shape items (array → per-element items, else → single item). `internal` with `InternalsVisibleTo("VolcanicCore.Tests")`.
- **Manual-run pin application.** `RunAsync` gets `triggerKind`; on `"manual"` runs, trigger's `pinned_output` replaces `triggerItems` before the trigger row is recorded, and each node's `pinned_input` replaces `byPort` before scheduling. Non-manual runs (`http.webhook`, `nats.hook`, etc.) ignore pins.
- **Frontend round-trip** in `graphAdapter.ts` — pins live on `VcNodeData.pinnedInput / pinnedOutput`; `fromGraph` / `toGraph` copy them across (omitting when null/undefined so the wire stays compact for un-pinned nodes).
- **`PinnedDataSection` component** (`frontend/src/editor/PinnedDataSection.tsx`) — JSON textarea with load-from-execution `<select>` + Clear button + inline parse-error message. Parent owns parsed value; child owns raw-text and error state. Emits `onChange(parsed | null)` on every valid keystroke; invalid JSON shows inline error and holds parent state at last valid parse.
- **NodeEditorDialog integration** — PinnedDataSection appears in the Parameters column, above WebhookUrlHint. Trigger nodes → "Pinned output" editor (side=`outputs`); action/terminal nodes → "Pinned input" editor (side=`inputs`).
- **Canvas pin icon** — amber `Pin` from lucide-react in the node header when either pin field is non-null (`data-testid="pin-badge"`).
- **`format: "password"` fields** still skip expression editing and render as native `<input type="password">` (unchanged from Plan 5).
What's next: **Plan 7 (CI + observability + DataProtection key persistence)** — see `docs/superpowers/followups.md`.
## Config-field templates (`{input.foo}` interpolation) ## Config-field templates (`{input.foo}` interpolation)
@@ -96,6 +108,10 @@ What's next: **Plan 6 (CI + observability + DataProtection key persistence)**
- **Expression preview and executor interpolation MUST stay in parity.** `frontend/src/expr/templateEngine.ts` is a hand-port of `src/VolcanicCore.Core/TemplateEngine.cs`. When adding a case to the backend `TemplateEngineTests`, add the mirror case to `frontend/src/expr/templateEngine.test.ts` too. Divergence is a bug: the preview line under every ExpressionField reads from the frontend port, and users trust it. Known caveat: numeric leaves render via `String(v)` on the client vs `GetRawText()` on the backend — over-precision/non-normalized JSON numbers (e.g., `1.50`, `9007199254740993`) may drift by one representation. Documented in the port's header comment. - **Expression preview and executor interpolation MUST stay in parity.** `frontend/src/expr/templateEngine.ts` is a hand-port of `src/VolcanicCore.Core/TemplateEngine.cs`. When adding a case to the backend `TemplateEngineTests`, add the mirror case to `frontend/src/expr/templateEngine.test.ts` too. Divergence is a bug: the preview line under every ExpressionField reads from the frontend port, and users trust it. Known caveat: numeric leaves render via `String(v)` on the client vs `GetRawText()` on the backend — over-precision/non-normalized JSON numbers (e.g., `1.50`, `9007199254740993`) may drift by one representation. Documented in the port's header comment.
- **`toGraph(nodes, edges, nodeTypes)` needs the node types.** `coerceConfigForSave` (in `graphAdapter.ts`) looks up each node's schema to decide whether to save `"200"` as `200` or as `"200"`. If you call `toGraph` without node types, everything gets saved as strings and every non-string field silently breaks at run time. The sole caller today is `EditorPage.tsx`'s save mutation, which passes `nts.data ?? []`. - **`toGraph(nodes, edges, nodeTypes)` needs the node types.** `coerceConfigForSave` (in `graphAdapter.ts`) looks up each node's schema to decide whether to save `"200"` as `200` or as `"200"`. If you call `toGraph` without node types, everything gets saved as strings and every non-string field silently breaks at run time. The sole caller today is `EditorPage.tsx`'s save mutation, which passes `nts.data ?? []`.
- **`Executor.CoerceToSchema` runs AFTER `InterpolateStrings`, and BOTH are wrapped by the per-node try/catch.** Reordering them (or moving coerce outside the try/catch) either stops templated numeric fields from working (nodes check `JsonValueKind.Number` strictly) or promotes coercion failures from a per-node `failed` row to a whole-execution crash. The pair must always be `InterpolateStrings → CoerceToSchema → node.ExecuteAsync`, all inside the same try/catch that produces `NodeResult(false, ..., ex.Message)`. - **`Executor.CoerceToSchema` runs AFTER `InterpolateStrings`, and BOTH are wrapped by the per-node try/catch.** Reordering them (or moving coerce outside the try/catch) either stops templated numeric fields from working (nodes check `JsonValueKind.Number` strictly) or promotes coercion failures from a per-node `failed` row to a whole-execution crash. The pair must always be `InterpolateStrings → CoerceToSchema → node.ExecuteAsync`, all inside the same try/catch that produces `NodeResult(false, ..., ex.Message)`.
- **Pinned data lives on `GraphNode`, not in a separate table.** Both `pinned_input` (action/terminal) and `pinned_output` (trigger) are optional fields on the graph JSON blob. Round-trip via `WorkflowGraph.Parse` (backend) + `graphAdapter` (frontend). Deleting a node deletes its pin naturally.
- **Pins only apply on manual runs.** `Executor.RunAsync` checks `triggerKind == "manual"` before overriding. Real production traffic (`http.webhook`, `nats.hook`) ignores pins so a stale value never corrupts live traffic. Two override sites: trigger's `pinned_output` replaces `triggerItems` before recording the trigger row; each action/terminal node's `pinned_input` replaces `byPort` before scheduling `RunNodeAsync`.
- **`Executor.WrapPin` is `internal`.** Tests reach it via `InternalsVisibleTo("VolcanicCore.Tests")` on `VolcanicCore.Api.csproj`. Public callers should not exist — the wrapping happens inside the executor's pin-application path only. Wrap rule: array → per-element items; else → single item wrapping the value.
- **Pinning does NOT skip upstream execution.** If node B has `pinned_input` and A → B, A still runs — its output is simply discarded when building B's byPort. Skipping upstream needs dependency analysis; deferred to a later plan.
## Working style ## Working style

View File

@@ -2,12 +2,21 @@
Items surfaced during review of one plan that belong to a later plan. Prevents silent drops. Items surfaced during review of one plan that belong to a later plan. Prevents silent drops.
## Deferred to Plan 6 (CI + observability + hardening) ## Deferred to Plan 7 (CI + observability + hardening)
Originally scoped as Plan 5, but Plan 5 was reassigned to the N8N-style Node Editor Dialog. These items now go to Plan 6. Originally scoped as Plan 5, then re-deferred to Plan 6 when Plan 5 became the N8N Node Editor Dialog and Plan 6 became Pinned Node Data. These items now go to Plan 7.
- **CI pipeline.** There is no automated CI yet. Add a workflow (Gitea Actions if the repo is hosted on Gitea, otherwise GitHub Actions) that runs `dotnet build`, `dotnet test`, `pnpm --dir frontend test`, `pnpm --dir frontend build`, and the Playwright smoke on push to `main`. The Playwright job needs `pnpm exec playwright install --with-deps chromium` before `pnpm --dir frontend test:e2e`. Bonus: build + push the Docker image on tag pushes. - **CI pipeline.** There is no automated CI yet. Add a workflow (Gitea Actions if the repo is hosted on Gitea, otherwise GitHub Actions) that runs `dotnet build`, `dotnet test`, `pnpm --dir frontend test`, `pnpm --dir frontend build`, and the Playwright smoke on push to `main`. The Playwright job needs `pnpm exec playwright install --with-deps chromium` before `pnpm --dir frontend test:e2e`. Bonus: build + push the Docker image on tag pushes.
- **Persist ASP.NET Core DataProtection keys under `/data`.** The Plan-4 Dockerfile smoke run showed the default DataProtection key ring is written to `/root/.aspnet/DataProtection-Keys` inside the container — ephemeral. On container replacement (redeploy, OOM restart, host reboot) every issued cookie is invalidated and every logged-in user is silently kicked out. Fix in Plan 6: wire `AddDataProtection().PersistKeysToFileSystem(new DirectoryInfo(Path.Combine(settings.DataDirectory, "dp-keys")))` in `Program.cs` and cover with an xUnit test that verifies the directory is created on first boot. - **Persist ASP.NET Core DataProtection keys under `/data`.** The Plan-4 Dockerfile smoke run showed the default DataProtection key ring is written to `/root/.aspnet/DataProtection-Keys` inside the container — ephemeral. On container replacement (redeploy, OOM restart, host reboot) every issued cookie is invalidated and every logged-in user is silently kicked out. Fix in Plan 7: wire `AddDataProtection().PersistKeysToFileSystem(new DirectoryInfo(Path.Combine(settings.DataDirectory, "dp-keys")))` in `Program.cs` and cover with an xUnit test that verifies the directory is created on first boot.
## Deferred from Plan 6 (Pinned Node Data)
- **Skip upstream execution when downstream has a pin.** Currently upstream ancestors of a pinned node still run and their output is discarded. Would need dependency analysis + a "no successors need my output" pass.
- **Pin both input AND output on the same action node.** Action nodes only expose the input editor; output-pin on action nodes would let operators "canned-response" a node during test runs.
- **Multi-port pinning.** Only port 0 today.
- **Pin sharing / templates.** Copy-pin-between-nodes UI.
- **Diff indicator in the dialog:** show "what this node WOULD have received" vs. "what the pin says it will receive".
- **Backend index/column for querying "workflows with pinned data."** Currently a stale pin on a live workflow can only be found by opening it in the editor.
## Deferred from Plan 5 (Node Editor Dialog) ## Deferred from Plan 5 (Node Editor Dialog)
@@ -15,7 +24,6 @@ Originally scoped as Plan 5, but Plan 5 was reassigned to the N8N-style Node Edi
- **Drag-and-drop insertion** from `JsonTree` to `ExpressionField`. Currently only click-to-insert at caret is supported. - **Drag-and-drop insertion** from `JsonTree` to `ExpressionField`. Currently only click-to-insert at caret is supported.
- **Multi-color / semantic highlighting** inside `{…}` (property vs bracket vs literal). Single amber color ships in Plan 5. - **Multi-color / semantic highlighting** inside `{…}` (property vs bracket vs literal). Single amber color ships in Plan 5.
- **Cross-node expressions** like `{nodes.<id>.output.body}` and `{nodes.<id>.output[0].json.body}`. Requires the executor to expose a scoped-view of run state to `TemplateEngine`. - **Cross-node expressions** like `{nodes.<id>.output.body}` and `{nodes.<id>.output[0].json.body}`. Requires the executor to expose a scoped-view of run state to `TemplateEngine`.
- **Pinned input data** (n8n's "pin this data on a node") — override the input the dialog uses for previews and the executor uses for that node.
- **JsonTree accessibility.** Add `role="tree"`/`role="treeitem"` + `aria-expanded`/`aria-level` on branch buttons so screen readers can navigate the structure. MVP ships without these; usable for sighted operators only. - **JsonTree accessibility.** Add `role="tree"`/`role="treeitem"` + `aria-expanded`/`aria-level` on branch buttons so screen readers can navigate the structure. MVP ships without these; usable for sighted operators only.
- **`msw` module alias cleanup.** `frontend/src/testing/msw.ts` exports both `server` (new) and `mswServer` (legacy). Four existing test files still use `mswServer`. Migrate them and drop the alias so there's one canonical name. - **`msw` module alias cleanup.** `frontend/src/testing/msw.ts` exports both `server` (new) and `mswServer` (legacy). Four existing test files still use `mswServer`. Migrate them and drop the alias so there's one canonical name.
- **`WebhookUrlHint` setTimeout leak on unmount.** The 1500ms `setCopied(false)` fires even if the dialog closes mid-window. Harmless in React 18 but a lingering timer. Guard with a ref or `useEffect` cleanup. - **`WebhookUrlHint` setTimeout leak on unmount.** The 1500ms `setCopied(false)` fires even if the dialog closes mid-window. Harmless in React 18 but a lingering timer. Guard with a ref or `useEffect` cleanup.