From e4190856b072fb4fcdf2df6a7fe0cdc72e36add9 Mon Sep 17 00:00:00 2001 From: Sev7eNup <79143581+Sev7eNup@users.noreply.github.com> Date: Fri, 23 Oct 2026 00:49:04 +0200 Subject: [PATCH 1/4] Fix nine audit findings in workflow execution and editing Correct step-test snapshots and target validation, cleanup safety, retention recovery, cross-tab authentication, script editing, command selection, scheduler joins, and ancestor traversal. Include regression tests and updated documentation. Refs #575, #587, #588, #589, #590, #591, #592, #593, #594. --- docs/claude-reference.md | 8 +- docs/testing/e2e/21-30.md | 11 +- scripts/README.md | 2 +- .../example-temp-file-cleanup-workflow.json | 44 +-- .../Controllers/WorkflowEditingController.cs | 10 +- src/NodePilot.Api/Dtos/StepTestDtos.cs | 17 +- src/NodePilot.Cli/Api/Dtos/NewSurfaceDtos.cs | 3 +- .../Execution/AncestorIndex.cs | 55 ++-- .../Execution/WorkflowScheduler.cs | 6 +- src/NodePilot.Engine/StepTester.cs | 89 ++++-- src/NodePilot.Engine/WorkflowEngine.cs | 2 +- src/NodePilot.Mcp/Api/Dtos/Dtos.cs | 3 +- .../AuditLogRetentionService.cs | 49 ++-- src/nodepilot-ui/CLAUDE.md | 7 +- src/nodepilot-ui/demo/main.demo.ts | 5 +- src/nodepilot-ui/demo/run/missionScenarios.ts | 5 +- src/nodepilot-ui/demo/ui/additionalTours.ts | 14 +- src/nodepilot-ui/e2e-demo/demo-smoke.spec.ts | 5 +- src/nodepilot-ui/e2e/step-test.spec.ts | 20 +- .../components/ProtectedRoute.test.tsx | 46 ++- .../overlays/CommandPalette.test.tsx | 58 ++++ .../properties/StepTestPanel.test.tsx | 31 ++- .../src/__tests__/demo/isolation.test.ts | 6 +- .../__tests__/demo/missionScenarios.test.ts | 11 +- .../pages/WorkflowEditorPage.test.tsx | 101 ++++++- .../__tests__/security/authBoundary.test.ts | 24 +- .../src/__tests__/stores/authStore.test.ts | 93 +++++++ .../components/designer/EditorOverlays.tsx | 23 +- .../components/designer/PropertiesPanel.tsx | 3 +- .../designer/overlays/CommandPalette.tsx | 38 +-- .../designer/properties/StepTestPanel.tsx | 13 +- .../properties/activities/RunScriptConfig.tsx | 13 +- .../components/designer/properties/shared.tsx | 2 + src/nodepilot-ui/src/security/authBoundary.ts | 21 +- src/nodepilot-ui/src/stores/authStore.ts | 35 ++- .../Rbac/WorkflowStepTestRbacTests.cs | 63 +++-- .../Examples/TempCleanupWorkflowTests.cs | 106 +++++++ .../Execution/AncestorIndexTests.cs | 116 ++++++++ .../Execution/StepTesterTargetTests.cs | 263 ++++++++++++++++++ .../Execution/WorkflowSchedulerTests.cs | 87 ++++++ .../AuditLogRetentionServiceTests.cs | 127 ++++++++- 41 files changed, 1401 insertions(+), 234 deletions(-) create mode 100644 tests/NodePilot.Engine.Tests/Examples/TempCleanupWorkflowTests.cs create mode 100644 tests/NodePilot.Engine.Tests/Execution/AncestorIndexTests.cs create mode 100644 tests/NodePilot.Engine.Tests/Execution/StepTesterTargetTests.cs diff --git a/docs/claude-reference.md b/docs/claude-reference.md index a330b9be..0c15e02a 100644 --- a/docs/claude-reference.md +++ b/docs/claude-reference.md @@ -111,8 +111,12 @@ Bei mehreren Fehlern entscheidet die Reihenfolge aus `StartedAt` und `Id`. `POST /api/workflows/{id}/steps/{stepId}/test` führt eine einzelne Activity aus, ohne eine WorkflowExecution anzulegen. `mockVariables` liefert künstliche -Step-Ergebnisse. `configOverride` ersetzt nur `data.config`, während Maschine, -Credential und Output-Variable aus dem gespeicherten Node stammen. +Step-Ergebnisse. `stepOverride` übergibt den vollständigen aktuellen `node.data`-Stand +einschließlich Activity-Typ, Konfiguration, Maschine, Credential und Output-Variable. +Die drei Testaktionen im Designer verwenden diesen konsistenten Snapshot. +Alternativ ersetzt `configOverride` ausschließlich `data.config` des gespeicherten +Nodes. Beide Overrides sind gegenseitig exklusiv und verlangen eine eigene aktive +Edit-Sperre. Ein nicht mehr vorhandenes ausgewähltes Maschinenziel wird abgelehnt. `test-context` liefert Vorgängerwerte aus einem gewählten oder dem letzten terminalen Lauf. Bei nicht ausgeführten Vorgängern bleibt nur das Schema. Globals kommen aus diff --git a/docs/testing/e2e/21-30.md b/docs/testing/e2e/21-30.md index a494d0e6..d4c601f5 100644 --- a/docs/testing/e2e/21-30.md +++ b/docs/testing/e2e/21-30.md @@ -614,15 +614,16 @@ Pflicht-Lese: CLAUDE.md "Step-Test mit Kontext". --- -### Test 28.3 — configOverride mit Live-Editor-Stand +### Test 28.3 — Vollständiger Step-Snapshot mit Live-Editor-Stand -**Setup:** runScript hat `script: "echo OLD"` in DB. Im Editor (ohne Save) auf `echo NEW` ändern. +**Setup:** runScript hat `script: "echo OLD"`, Maschine A und Credential A in DB. Im Editor auf `echo NEW`, Maschine B und Credential B ändern. -**Schritte:** Step-Test ohne Save. +**Schritte:** Vor dem Autosave den Step-Test auslösen. Für Property-Panel, Skripteditor über „Open Editor“ und Skripteditor per Doppelklick wiederholen. **Prüfpunkte:** -- [ ] Test verwendet `echo NEW` (configOverride). -- [ ] DB-Stand bleibt `echo OLD` unverändert. +- [ ] Test verwendet `echo NEW`, Maschine B und Credential B aus demselben `stepOverride`. +- [ ] Beim Wechsel zu lokaler Ausführung werden gespeichertes Ziel und Credential nicht übernommen. +- [ ] Der Test-Request selbst ändert die gespeicherte Definition nicht; Autosave ist davon unabhängig. --- diff --git a/scripts/README.md b/scripts/README.md index e2646504..87a17979 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -96,7 +96,7 @@ to make the producing trigger and its declared output contract explicit. - `example-windows-update-health-workflow.json` — a Windows Update health check of a host (CBS/WU log tails, service/registry/WMI/file-system probes, `decision` classification). - `example-disk-space-watch-workflow.json` — daily free-space check on C: (`scheduleTrigger` → `runScript` → `decision` on three severities → critical branch collects the ten largest folders and mails ops, `waitAny` → `returnData`). 9 activities. - `example-service-recovery-workflow.json` — Print Spooler watchdog every 15 minutes (`serviceManagement` status → `decision` → start + `delay` + verify → recovery or escalation mail, `waitAny` → `returnData`). The two edges out of the verify step compare the service state rather than step success, because reading a stopped service is a successful step. 11 activities. -- `example-temp-file-cleanup-workflow.json` — `manualTrigger` with `path`/`olderThanDays` parameters → `folderOperation` list → `zipOperation` → `fileHash` and a `runScript` purge in parallel → `waitAll` → summary mail → `returnData`. 8 activities. +- `example-temp-file-cleanup-workflow.json` — `manualTrigger` with `path`/`olderThanDays` parameters → successful `zipOperation` → `folderOperation` list and `fileHash` in parallel → `waitAll` → `runScript` purge only when both checks succeed → summary mail after successful cleanup → `returnData`. 8 activities. - `example-guided-file-workflow.json` — guided browser-demo task: manual inputs → registry destination → service status → PowerShell configuration file → copy → log → return data. 7 activities. The demo simulates registry, service and files; `Test` and `Production` succeed, while `Protected` fails at the copy with access denied. Machine IDs are placeholders remapped by the demo seed. These examples are also the seed of the browser demo (`src/nodepilot-ui/demo/seed/graphs.ts`), which diff --git a/scripts/example-temp-file-cleanup-workflow.json b/scripts/example-temp-file-cleanup-workflow.json index 696136c9..edbc9cc4 100644 --- a/scripts/example-temp-file-cleanup-workflow.json +++ b/scripts/example-temp-file-cleanup-workflow.json @@ -5,18 +5,18 @@ "workflows": [ { "name": "Temp File Cleanup", - "description": "Archives a temp folder into a ZIP, then verifies the archive with a checksum while the aged-out originals are removed. Both branches converge on a waitAll junction before the summary mail goes out.", + "description": "Archives a temp folder into a ZIP, then inventories the folder and computes the archive checksum in parallel. After both checks succeed, aged-out originals are removed and a summary mail is sent.", "definition": { "nodes": [ { "id": "note_input", "type": "stickyNote", "position": { "x": 0, "y": 60 }, - "style": { "width": 940, "height": 110 }, + "style": { "width": 640, "height": 110 }, "data": { "label": "Note", "activityType": "note", "text": "Phase A - Input\nThe two trigger parameters are the only thing to fill in. Both are declared as strings: the run dialog sends strings, and a number would be rejected on execute.", "fontSize": 13, "disabled": true } }, - { "id": "note_finish", "type": "stickyNote", "position": { "x": 1040, "y": 60 }, - "style": { "width": 940, "height": 110 }, + { "id": "note_finish", "type": "stickyNote", "position": { "x": 740, "y": 60 }, + "style": { "width": 1480, "height": 110 }, "data": { "label": "Note", "activityType": "note", - "text": "Phase B - Verify and clean\nChecksum and delete run in parallel and converge on waitAll, so the summary mail only goes out once both are done.", + "text": "Phase B - Verify and clean\nInventory and checksum run in parallel after successful archiving. waitAll joins both checks; only successful checks allow deletion. The summary mail follows successful cleanup.", "fontSize": 13, "disabled": true } }, { "id": "trg", "type": "activity", "position": { "x": 0, "y": 400 }, @@ -32,7 +32,7 @@ } } }, - { "id": "inventory", "type": "activity", "position": { "x": 400, "y": 400 }, + { "id": "inventory", "type": "activity", "position": { "x": 740, "y": 560 }, "data": { "label": "folderOperation: list the temp folder", "activityType": "folderOperation", @@ -41,7 +41,7 @@ "config": { "operation": "list", "path": "{{trg.param.path}}" } } }, - { "id": "archive", "type": "activity", "position": { "x": 700, "y": 400 }, + { "id": "archive", "type": "activity", "position": { "x": 400, "y": 400 }, "data": { "label": "zipOperation: archive the folder", "activityType": "zipOperation", @@ -56,7 +56,7 @@ } } }, - { "id": "checksum", "type": "activity", "position": { "x": 1040, "y": 240 }, + { "id": "checksum", "type": "activity", "position": { "x": 740, "y": 240 }, "data": { "label": "fileHash: checksum the archive", "activityType": "fileHash", @@ -65,7 +65,7 @@ "config": { "path": "C:\\Archive\\temp-cleanup.zip", "algorithm": "SHA256" } } }, - { "id": "purge", "type": "activity", "position": { "x": 1040, "y": 560 }, + { "id": "purge", "type": "activity", "position": { "x": 1380, "y": 400 }, "data": { "label": "runScript: remove the archived files", "activityType": "runScript", @@ -77,7 +77,7 @@ } } }, - { "id": "gather", "type": "activity", "position": { "x": 1380, "y": 400 }, + { "id": "gather", "type": "activity", "position": { "x": 1080, "y": 400 }, "data": { "label": "Junction: waitAll (2)", "activityType": "junction", @@ -109,23 +109,27 @@ } } ], "edges": [ - { "id": "e_trg_inventory", "source": "trg", "target": "inventory", "type": "labeled", + { "id": "e_trg_archive", "source": "trg", "target": "archive", "type": "labeled", "data": { "label": "Always" } }, - { "id": "e_inventory_archive", "source": "inventory", "target": "archive", "type": "labeled", - "data": { "label": "On Success", "condition": "inventory.success" } }, - { "id": "e_archive_checksum", "source": "archive", "target": "checksum", "type": "labeled", - "data": { "label": "Always" } }, - { "id": "e_archive_purge", "source": "archive", "target": "purge", "type": "labeled", - "data": { "label": "Always" } }, + "data": { "label": "On Success", "condition": "archive.success" } }, + { "id": "e_archive_inventory", "source": "archive", "target": "inventory", "type": "labeled", + "data": { "label": "On Success", "condition": "archive.success" } }, { "id": "e_checksum_gather", "source": "checksum", "target": "gather", "type": "labeled", "data": { "label": "Always" } }, - { "id": "e_purge_gather", "source": "purge", "target": "gather", "type": "labeled", + { "id": "e_inventory_gather", "source": "inventory", "target": "gather", "type": "labeled", "data": { "label": "Always" } }, - { "id": "e_gather_mail", "source": "gather", "target": "mail", "type": "labeled", - "data": { "label": "Always" } }, + { "id": "e_gather_purge", "source": "gather", "target": "purge", "type": "labeled", + "data": { "label": "Checks succeeded", "conditionExpression": { + "type": "group", "op": "AND", "children": [ + { "type": "comparison", "left": { "kind": "variable", "stepId": "checksum", "field": "success" }, "op": "isTrue" }, + { "type": "comparison", "left": { "kind": "variable", "stepId": "inventory", "field": "success" }, "op": "isTrue" } + ] + } } }, + { "id": "e_purge_mail", "source": "purge", "target": "mail", "type": "labeled", + "data": { "label": "On Success", "condition": "purge.success" } }, { "id": "e_mail_ret", "source": "mail", "target": "ret", "type": "labeled", "data": { "label": "Always" } } ] diff --git a/src/NodePilot.Api/Controllers/WorkflowEditingController.cs b/src/NodePilot.Api/Controllers/WorkflowEditingController.cs index 73b05d40..0e2b4767 100644 --- a/src/NodePilot.Api/Controllers/WorkflowEditingController.cs +++ b/src/NodePilot.Api/Controllers/WorkflowEditingController.cs @@ -610,7 +610,7 @@ public async Task> ForceUnlock(Guid id, Cancellat /// Useful for testing individual steps during development. Admin/Operator only. /// /// Body fields: MockVariables for upstream variable injection, - /// ConfigOverride for testing unsaved editor state without going through PUT first. + /// ConfigOverride for config-only changes or StepOverride for a complete node-data snapshot. /// Testing the persisted config requires Run; an override additionally requires Edit and /// the caller's active edit-lock because it can replace executable activity config. /// @@ -628,7 +628,10 @@ public async Task> TestStep( if (await RequireWorkflowAccessAsync(workflow, NodePilot.Core.Interfaces.ResourceOp.Run, ct) is { } d8) return d8; var configOverride = request?.ConfigOverride; - if (configOverride.HasValue) + var stepOverride = request?.StepOverride; + if (configOverride.HasValue && stepOverride.HasValue) + return BadRequest(new { message = "Config override and step override cannot be combined" }); + if (configOverride.HasValue || stepOverride.HasValue) { // An override is unsaved executable editor state. Without the Edit + lock gates a // FolderOperator could replace a persisted runScript/HTTP/SQL config and execute it @@ -644,7 +647,8 @@ public async Task> TestStep( StepTestAuthorizationSnapshot.Capture(workflow), request?.MockVariables, configOverride, - ct); + ct, + stepOverride); await _audit.LogAsync(AuditActions.WorkflowStepTested, "Workflow", workflow.Id, AuditDetails.Json(("stepId", stepId), ("success", result.Success), ("durationMs", result.DurationMs)), ct); diff --git a/src/NodePilot.Api/Dtos/StepTestDtos.cs b/src/NodePilot.Api/Dtos/StepTestDtos.cs index 6235de4f..977f35cb 100644 --- a/src/NodePilot.Api/Dtos/StepTestDtos.cs +++ b/src/NodePilot.Api/Dtos/StepTestDtos.cs @@ -9,19 +9,16 @@ namespace NodePilot.Api.Dtos; /// "checkDisk.output": "7", "checkDisk.param.freeGb": "7"). Populates /// upstream {{step.output}} / {{step.param.x}} references. /// -/// ConfigOverride: optional unsaved config JSON for the step under test. When -/// set, this replaces the persisted node config — the test then reflects what the user is -/// editing right now, not the last-saved DB state. Pass null to fall back to the persisted -/// definition. Only the data.config sub-tree is overridden; targetMachine, credential, -/// and outputVariable still come from the persisted node so the user doesn't have to re-enter - /// them on every test click. Because an override can replace executable script/HTTP/SQL - /// config while retaining stored targets and credentials, it requires Folder Edit permission - /// and the caller's active workflow edit-lock. A request without an override only requires - /// Run permission. +/// ConfigOverride replaces only data.config, retaining persisted node metadata. +/// StepOverride replaces the complete node.data object, including activity type, +/// config, target and credential. Omitted target/credential fields clear their saved values. +/// The overrides are mutually exclusive. Either requires Folder Edit permission and the +/// caller's active edit-lock; testing the persisted step requires Run permission. /// public record StepTestRequest( Dictionary? MockVariables = null, - JsonElement? ConfigOverride = null); + JsonElement? ConfigOverride = null, + JsonElement? StepOverride = null); /// /// Variable name + sample value the UI shows in the mock-editor when the user picks diff --git a/src/NodePilot.Cli/Api/Dtos/NewSurfaceDtos.cs b/src/NodePilot.Cli/Api/Dtos/NewSurfaceDtos.cs index b99a33f0..d3679e89 100644 --- a/src/NodePilot.Cli/Api/Dtos/NewSurfaceDtos.cs +++ b/src/NodePilot.Cli/Api/Dtos/NewSurfaceDtos.cs @@ -184,7 +184,8 @@ public sealed record LlmTestProbeRequest(string? ProfileId, JsonElement Settings public sealed record StepTestRequest( Dictionary? MockVariables = null, - JsonElement? ConfigOverride = null); + JsonElement? ConfigOverride = null, + JsonElement? StepOverride = null); public sealed record StepTestResponse( bool Success, diff --git a/src/NodePilot.Engine/Execution/AncestorIndex.cs b/src/NodePilot.Engine/Execution/AncestorIndex.cs index d39dddef..ddea3141 100644 --- a/src/NodePilot.Engine/Execution/AncestorIndex.cs +++ b/src/NodePilot.Engine/Execution/AncestorIndex.cs @@ -2,13 +2,8 @@ namespace NodePilot.Engine.Execution; /// /// Precomputed ancestor sets for every node of a compiled workflow graph. -/// -/// -/// rebuilds its incoming-edge index on every call, so calling it per node would be quadratic in -/// the edge count. This walks the already-compiled ReverseAdjacency once per node instead, -/// and memoizes each node's set so a shared prefix of the graph is traversed once rather than -/// once per descendant. +/// Walks the compiled reverse adjacency, visiting each unresolved ancestor at most once per +/// node and reusing complete ancestor sets from earlier nodes. /// internal static class AncestorIndex { @@ -17,11 +12,13 @@ internal static class AncestorIndex /// node itself). Cycles are tolerated: the visited set stops the walk. /// public static Dictionary> Build( - IReadOnlyDictionary> reverseAdjacency) + IReadOnlyDictionary> reverseAdjacency, + CancellationToken ct = default) { + ct.ThrowIfCancellationRequested(); var result = new Dictionary>(reverseAdjacency.Count, StringComparer.Ordinal); foreach (var nodeId in reverseAdjacency.Keys) - result[nodeId] = Resolve(nodeId, reverseAdjacency, result, []); + result[nodeId] = Resolve(nodeId, reverseAdjacency, result, ct); return result; } @@ -29,32 +26,34 @@ private static IReadOnlySet Resolve( string nodeId, IReadOnlyDictionary> reverseAdjacency, Dictionary> memo, - HashSet inProgress) + CancellationToken ct) { - if (memo.TryGetValue(nodeId, out var cached)) return cached; - - // Re-entering a node means the graph has a cycle. Returning empty here breaks the - // recursion; the outer frame still contributes the parents it reached directly, so a - // cyclic graph degrades to "the ancestors we could reach without looping" rather than - // stack-overflowing. Cyclic graphs produce no roots and never run (see WorkflowEngine), - // but the index must not blow up while validating one. - if (!inProgress.Add(nodeId)) return new HashSet(StringComparer.Ordinal); - - var ancestors = new HashSet(StringComparer.Ordinal); - if (reverseAdjacency.TryGetValue(nodeId, out var parents)) + // Seed the visited set with the node itself so cycles cannot reenter it. + var ancestors = new HashSet(StringComparer.Ordinal) { nodeId }; + var pending = new Stack(); + pending.Push(nodeId); + while (pending.TryPop(out var current)) { + ct.ThrowIfCancellationRequested(); + if (memo.TryGetValue(current, out var cached)) + { + foreach (var ancestor in cached) + { + ct.ThrowIfCancellationRequested(); + ancestors.Add(ancestor); + } + continue; + } + + if (!reverseAdjacency.TryGetValue(current, out var parents)) continue; foreach (var parent in parents) { - if (!ancestors.Add(parent)) continue; - ancestors.UnionWith(Resolve(parent, reverseAdjacency, memo, inProgress)); + ct.ThrowIfCancellationRequested(); + if (ancestors.Add(parent)) pending.Push(parent); } } - inProgress.Remove(nodeId); - - // Only memoize results computed outside a cycle — a set truncated by the guard above is - // not the node's real ancestor set and must not be cached for other descendants. - if (inProgress.Count == 0) memo[nodeId] = ancestors; + ancestors.Remove(nodeId); return ancestors; } } diff --git a/src/NodePilot.Engine/Execution/WorkflowScheduler.cs b/src/NodePilot.Engine/Execution/WorkflowScheduler.cs index e455355a..d3866faa 100644 --- a/src/NodePilot.Engine/Execution/WorkflowScheduler.cs +++ b/src/NodePilot.Engine/Execution/WorkflowScheduler.cs @@ -299,7 +299,11 @@ private static async Task RunLoopAsync( foreach (var predEdge in preds) { if (!completed.Contains(predEdge.Source) && !skipped.Contains(predEdge.Source)) - MarkSubtreeSkipped(predEdge.Source, skipped, adjacency, reverseAdjacency, stopAtNode: successor); + { + foreach (var reopened in MarkSubtreeSkipped( + predEdge.Source, skipped, adjacency, reverseAdjacency, stopAtNode: successor)) + successorsToEvaluate.Enqueue(reopened); + } } foreach (var inFlightStep in inFlight.Values) diff --git a/src/NodePilot.Engine/StepTester.cs b/src/NodePilot.Engine/StepTester.cs index a093fddf..6f499be6 100644 --- a/src/NodePilot.Engine/StepTester.cs +++ b/src/NodePilot.Engine/StepTester.cs @@ -18,7 +18,8 @@ Task TestStepAsync( StepTestAuthorizationSnapshot authorizationSnapshot, Dictionary? mockVariables, JsonElement? configOverride, - CancellationToken ct); + CancellationToken ct, + JsonElement? stepOverride = null); } /// @@ -67,22 +68,26 @@ public StepTester(NodePilotDbContext db, ActivityRegistry registry, IGlobalVaria public async Task TestStepAsync( Guid workflowId, string stepId, StepTestAuthorizationSnapshot authorizationSnapshot, Dictionary? mockVariables, - JsonElement? configOverride, CancellationToken ct) + JsonElement? configOverride, CancellationToken ct, JsonElement? stepOverride = null) { var sw = System.Diagnostics.Stopwatch.StartNew(); - if (configOverride.HasValue + if (configOverride.HasValue && stepOverride.HasValue) + return Fail("Config override and step override cannot be combined", sw); + + var hasOverride = configOverride.HasValue || stepOverride.HasValue; + if (hasOverride && (authorizationSnapshot.CheckedOutByUserId is null || authorizationSnapshot.CheckedOutAt is null)) { - return Fail("Config override requires an active edit-lock authorization snapshot", sw); + return Fail("An override requires an active edit-lock authorization snapshot", sw); } var authorizedWorkflow = _db.Workflows.AsNoTracking() .Where(w => w.Id == workflowId && w.FolderId == authorizationSnapshot.FolderId && w.Version == authorizationSnapshot.Version); - if (configOverride.HasValue) + if (hasOverride) { authorizedWorkflow = authorizedWorkflow.Where(w => w.CheckedOutByUserId == authorizationSnapshot.CheckedOutByUserId @@ -100,26 +105,15 @@ public async Task TestStepAsync( if (node is null) return Fail($"Step '{stepId}' not found in workflow", sw); - if (node.Data.Disabled) - return Fail("Step is disabled — enable it before testing", sw); - - // Sub-workflow activities' runtime RBAC gate only fires when there is a persisted - // parent WorkflowExecution row. Step-test synthesises an execution id that is never - // persisted, so both startWorkflow and forEach would otherwise skip the resolver and - // could invoke a child workflow outside the caller's folder scope. Until step-test - // carries an authenticated persisted call context, refuse both activities. - if (string.Equals(node.Type, "startWorkflow", StringComparison.OrdinalIgnoreCase) - || string.Equals(node.Type, "forEach", StringComparison.OrdinalIgnoreCase)) + if (stepOverride is { } liveData) { - return Fail($"{node.Type} cannot be step-tested — run the full workflow to validate sub-workflow calls.", sw); + if (!TryParseStepOverride(stepId, liveData, out var liveNode) || liveNode is null) + return Fail("Step override requires an activityType string, a config object and valid node fields", sw); + node = liveNode; } - - // Unsaved-config override: when the UI sends the live editor state, swap it onto the - // node we just parsed so the test reflects what the user is editing right now (not the - // last-saved DB config). Other fields (targetMachine, credential, outputVariable) keep - // coming from the persisted definition — those rarely change between test clicks and - // forcing the user to re-include them in every override would be ergonomic noise. - if (configOverride is { ValueKind: JsonValueKind.Object } overrideElement) + // Config overrides change only activity configuration. Target, credential and other + // node metadata come from the authorized persisted definition. + else if (configOverride is { ValueKind: JsonValueKind.Object } overrideElement) { node = new WorkflowNode { @@ -139,10 +133,24 @@ public async Task TestStepAsync( }; } + if (node.Data.Disabled) + return Fail("Step is disabled — enable it before testing", sw); + + // Step tests have no persisted parent execution for the sub-workflow RBAC gate. + if (string.Equals(node.Type, "startWorkflow", StringComparison.OrdinalIgnoreCase) + || string.Equals(node.Type, "forEach", StringComparison.OrdinalIgnoreCase)) + { + return Fail($"{node.Type} cannot be step-tested — run the full workflow to validate sub-workflow calls.", sw); + } + // Resolve machine + credential from node config (raw GUID or literal hostname) ManagedMachine? machine = null; if (Guid.TryParse(node.Data.TargetMachineRaw, out var machineId)) + { machine = await _db.ManagedMachines.AsNoTracking().FirstOrDefaultAsync(m => m.Id == machineId, ct); + if (machine is null) + return Fail($"Target machine '{machineId}' was not found — select an existing machine before testing", sw); + } else if (!string.IsNullOrWhiteSpace(node.Data.TargetMachineRaw)) machine = new ManagedMachine { @@ -231,6 +239,41 @@ public async Task TestStepAsync( private static StepTestResult Fail(string msg, System.Diagnostics.Stopwatch sw) => new(false, null, msg, [], sw.Elapsed.TotalMilliseconds, msg); + private static bool TryParseStepOverride(string stepId, JsonElement data, out WorkflowNode? node) + { + node = null; + if (data.ValueKind != JsonValueKind.Object + || !data.TryGetProperty("activityType", out var type) + || type.ValueKind != JsonValueKind.String || string.IsNullOrWhiteSpace(type.GetString()) + || !data.TryGetProperty("config", out var config) || config.ValueKind != JsonValueKind.Object + || !IsOptionalString(data, "label") || !IsOptionalString(data, "outputVariable") + || !IsOptionalString(data, "targetMachineId") || !IsOptionalString(data, "credentialId") + || !IsOptionalString(data, "breakpointCondition") + || !IsOptionalBoolean(data, "disabled") || !IsOptionalBoolean(data, "breakpoint")) + return false; + + if (data.TryGetProperty("credentialId", out var credential) + && credential.ValueKind == JsonValueKind.String + && !string.IsNullOrWhiteSpace(credential.GetString()) + && !Guid.TryParse(credential.GetString(), out _)) + return false; + + var definition = JsonSerializer.SerializeToElement(new + { + nodes = new[] { new { id = stepId, type = "activity", data } }, + }); + node = WorkflowDefinitionParser.ParseNodes(definition)[0]; + return true; + } + + private static bool IsOptionalString(JsonElement data, string name) + => !data.TryGetProperty(name, out var value) + || value.ValueKind is JsonValueKind.String or JsonValueKind.Null; + + private static bool IsOptionalBoolean(JsonElement data, string name) + => !data.TryGetProperty(name, out var value) + || value.ValueKind is JsonValueKind.True or JsonValueKind.False or JsonValueKind.Null; + private static Dictionary BuildFakeResults(Dictionary mockVars) { var results = new Dictionary(StringComparer.OrdinalIgnoreCase); diff --git a/src/NodePilot.Engine/WorkflowEngine.cs b/src/NodePilot.Engine/WorkflowEngine.cs index 394d4c43..6d38a8ad 100644 --- a/src/NodePilot.Engine/WorkflowEngine.cs +++ b/src/NodePilot.Engine/WorkflowEngine.cs @@ -1056,7 +1056,7 @@ private async Task RunGraphAsync( // resolve a template from an unrelated parallel branch — and only when that branch // happened to finish first, which made the same definition succeed or fail depending on // timing. Computed once per run from the compiled reverse adjacency. - var ancestorsByNode = AncestorIndex.Build(reverseAdjacency); + var ancestorsByNode = AncestorIndex.Build(reverseAdjacency, cts.Token); // The out-of-scope gate asks "is this a node of the graph that I may not read", not "has // it run yet" — the latter made the check depend on which branch finished first. Built diff --git a/src/NodePilot.Mcp/Api/Dtos/Dtos.cs b/src/NodePilot.Mcp/Api/Dtos/Dtos.cs index c2db162e..ece99905 100644 --- a/src/NodePilot.Mcp/Api/Dtos/Dtos.cs +++ b/src/NodePilot.Mcp/Api/Dtos/Dtos.cs @@ -153,7 +153,8 @@ public sealed record NextFiresResponse(List Fires); // ConfigOverride is executable unsaved editor state; the API requires Edit + the caller's lock. public sealed record StepTestRequest( Dictionary? MockVariables = null, - JsonElement? ConfigOverride = null); + JsonElement? ConfigOverride = null, + JsonElement? StepOverride = null); public sealed record StepTestResponse( bool Success, diff --git a/src/NodePilot.Scheduler/AuditLogRetentionService.cs b/src/NodePilot.Scheduler/AuditLogRetentionService.cs index 0d92af30..5012d38e 100644 --- a/src/NodePilot.Scheduler/AuditLogRetentionService.cs +++ b/src/NodePilot.Scheduler/AuditLogRetentionService.cs @@ -171,18 +171,8 @@ internal async Task PurgeOnceAsync(int maxAgeDays, int batchSize, Cancellat } catch { - // F-4: archive succeeded but the DB delete failed (lock, constraint violation, - // connection drop). Without the rollback below, the next retention pass would - // re-select the same rows and append a second archive copy — duplicated audit - // events break SIEM frequency analytics and forensics deduplication. Rolling - // back the per-batch archive file keeps the file/DB invariant clean. If this - // delete itself fails we live with the duplicate — recoverable, vs. silent loss. if (archivedFilePath is not null) - { - var sidecarPath = archivedFilePath + ".sha256"; - TryDeleteFile(archivedFilePath, "archive-rollback-gz"); - TryDeleteFile(sidecarPath, "archive-rollback-sidecar"); - } + await RollbackArchiveIfRowsRemainAsync(archivedFilePath, batchIds); throw; } @@ -192,6 +182,34 @@ internal async Task PurgeOnceAsync(int maxAgeDays, int batchSize, Cancellat return totalDeleted; } + private async Task RollbackArchiveIfRowsRemainAsync(string archivedFilePath, List batchIds) + { + // A failed acknowledgement can follow a committed delete. Only remove the archive + // after an independent read confirms that every archived row is still in the DB. + try + { + using var timeout = new CancellationTokenSource(TimeSpan.FromSeconds(10)); + using var scope = _scopeFactory.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + var remaining = await db.AuditLog.AsNoTracking() + .CountAsync(a => batchIds.Contains(a.Id), timeout.Token); + if (remaining == batchIds.Count) + { + TryDeleteFile(archivedFilePath, "archive-rollback-gz"); + TryDeleteFile(archivedFilePath + ".sha256", "archive-rollback-sidecar"); + return; + } + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Could not verify the audit delete outcome for {Archive}", archivedFilePath); + } + + _logger.LogWarning( + "Preserving audit archive {Archive}: the complete batch was not confirmed in the database. " + + "Retries may produce duplicate entries; deduplicate by audit entry id.", archivedFilePath); + } + // L-16: validation of the audit archive path. Unlike ExecutionRetention, we must NOT // coerce a broken path to "archive-disabled" — if an admin intentionally configured an // archive path, audit rows must be retained in-DB until the archive works. So on probe @@ -252,17 +270,16 @@ private string ValidateArchivePath(string? configured) /// /// Writes the audit batch as gzipped NDJSON to a per-batch file (one file per Purge batch, - /// not one per day) plus a SHA-256 sidecar at {name}.sha256. Per-batch granularity - /// makes the (archive, delete) pair idempotent across retries — the rollback in - /// PurgeOnceAsync simply deletes the per-batch file pair. + /// not one per day) plus a SHA-256 sidecar at {name}.sha256. Retention removes this + /// pair after a failed delete only when every archived row is confirmed in the database. + /// An uncertain delete outcome preserves the archive; entry IDs allow deduplication. /// Critical properties: /// - tmp-write + atomic rename: a crash between write and rename leaves a .tmp file /// that ops can clean up without affecting durability of committed batches. /// - SHA-256 sidecar is written AFTER the gzip rename, so a sidecar without a .gz /// never happens; a .gz without a sidecar only happens on sidecar-write failure /// (logged loudly; the file still archives correctly, just without integrity proof). - /// - returns the final .gz path (or null on failure) so PurgeOnceAsync can roll back - /// both files when the matching DB delete throws — this is what closes F-4. + /// - returns the final .gz path (or null on failure) for verified rollback of the pair. /// internal async Task ArchiveAsync(string archivePath, IEnumerable rows, CancellationToken ct) { diff --git a/src/nodepilot-ui/CLAUDE.md b/src/nodepilot-ui/CLAUDE.md index 81cf9ad4..213c2a69 100644 --- a/src/nodepilot-ui/CLAUDE.md +++ b/src/nodepilot-ui/CLAUDE.md @@ -49,9 +49,10 @@ Server, keine DB — der Zustand lebt pro Tab und ein Reload stellt den Seed wie die Demo-Startdatei aus; Assets nutzen `/demo/`. Alte Hash-Links normalisiert der Demo-Entry. In `vite.config.ts` **und** `vitest.config.ts` auf `'false'` definiert — fehlt das zweite, bricht jeder Test, der `App` importiert, mit einem `ReferenceError`, der nach etwas anderem aussieht. -- **Tab-Isolation ist nicht gratis.** Ein zweiter Tab publiziert ein Identitäts-Ereignis, und jeder - andere Tab beantwortet es mit Cache-Leeren und Remount — ungespeicherte Designer-Änderungen sind - weg. Deshalb legt der Entry den Transport beidseitig still. +- **Tab-Isolation ist nicht gratis.** Echte Anmelde-, Abmelde- und Sitzungswechsel leeren in anderen + Tabs die Caches und bauen geschützte Komponenten neu auf. Reines Beobachten derselben Sitzung + beim Öffnen eines Tabs erhält Entwürfe. Die unabhängigen Demo-Welten dürfen auch echte + Auth-Wechsel nicht teilen; deshalb legt der Entry den Transport beidseitig still. - **Seed:** die Workflow-JSONs aus `demo/seed/fixtures/` und `scripts/` — im Demo-Paket entsteht **kein** Graph. Nodes mit mehreren eingehenden Kanten bekommen beim Laden eine `waitAll`-Junction (wie Designer und SCOrch-Import), sonst blockiert der Pre-Publish-Check. Die Reihenfolge in diff --git a/src/nodepilot-ui/demo/main.demo.ts b/src/nodepilot-ui/demo/main.demo.ts index 255db8c5..30928660 100644 --- a/src/nodepilot-ui/demo/main.demo.ts +++ b/src/nodepilot-ui/demo/main.demo.ts @@ -25,9 +25,8 @@ import { mountDemoBanner } from './ui/banner'; import { mountTour } from './ui/tour'; function bootstrap(): void { - // Several tabs of one browser must not disturb each other. Opening or reloading a tab - // publishes an identity event, and every other tab answers it by clearing caches and - // remounting, which throws away unsaved editor state. + // Demo tabs own independent users and data. Auth changes in one world must not clear + // another world's caches or drafts through the product's shared-session transport. isolateAuthBoundaryTransport(); configureWorld(() => buildWorld()); diff --git a/src/nodepilot-ui/demo/run/missionScenarios.ts b/src/nodepilot-ui/demo/run/missionScenarios.ts index b3c5269b..35c3ce59 100644 --- a/src/nodepilot-ui/demo/run/missionScenarios.ts +++ b/src/nodepilot-ui/demo/run/missionScenarios.ts @@ -15,7 +15,7 @@ export function missionPlan(workflowId: string, nodes: GraphNode[], edges: Graph ? [['trg'], ['probe'], ['dec'], ['log_warning'], ['gather'], ['ret']] : [['trg'], ['probe'], ['dec'], ['log_ok'], ['gather'], ['ret']]; } else if (workflowId === MISSION_WORKFLOW_IDS.parallel) { - groups = [['trg'], ['inventory'], ['archive'], ['checksum', 'purge'], ['gather'], ['mail'], ['ret']]; + groups = [['trg'], ['archive'], ['checksum', 'inventory'], ['gather'], ['purge'], ['mail'], ['ret']]; } else if (workflowId === MISSION_WORKFLOW_IDS.service) { groups = (inputs.serviceState ?? 'Stopped').toLowerCase() === 'running' ? [['trg'], ['status'], ['dec'], ['log_ok'], ['gather'], ['ret']] @@ -27,7 +27,7 @@ export function missionPlan(workflowId: string, nodes: GraphNode[], edges: Graph const required = workflowId === MISSION_WORKFLOW_IDS.decision ? ['trg->probe', 'probe->dec', 'dec->top_dirs', 'top_dirs->mail_critical', 'mail_critical->gather', 'log_warning->gather', 'log_ok->gather', 'gather->ret'] : workflowId === MISSION_WORKFLOW_IDS.parallel - ? ['trg->inventory', 'inventory->archive', 'archive->checksum', 'archive->purge', 'checksum->gather', 'purge->gather', 'gather->mail', 'mail->ret'] + ? ['trg->archive', 'archive->checksum', 'archive->inventory', 'checksum->gather', 'inventory->gather', 'gather->purge', 'purge->mail', 'mail->ret'] : ['trg->status', 'status->dec', 'dec->start', 'start->settle', 'settle->verify', 'verify->mail_recovered', 'mail_recovered->gather', 'gather->ret']; return groups.every(group => group.every(id => available.has(id))) && required.every(connection => connections.has(connection)) ? groups : null; } @@ -57,6 +57,7 @@ export function missionOutcome(workflowId: string, node: GraphNode, inputs: Reco if (id === 'ret') return result(JSON.stringify({ freeSpaceGb: free, severity }), { freeSpaceGb: String(free), severity }); } if (workflowId === MISSION_WORKFLOW_IDS.parallel) { + if (id === 'inventory') return result('Folder inventory complete.', { fileCount: '124' }, 3600); if (id === 'checksum') return result('Archive checksum verified.', { hash: '9f2c4b1e' }, 3200); if (id === 'purge') return result('Archived originals removed.', { removed: '124' }, 3600); if (id === 'gather') return result('Both parallel branches completed.'); diff --git a/src/nodepilot-ui/demo/ui/additionalTours.ts b/src/nodepilot-ui/demo/ui/additionalTours.ts index 2f73007e..cac2e311 100644 --- a/src/nodepilot-ui/demo/ui/additionalTours.ts +++ b/src/nodepilot-ui/demo/ui/additionalTours.ts @@ -19,7 +19,7 @@ const COPY: Record<'de' | 'en', Record> = { de: { build: { title: 'Einen Workflow bauen', work: 'Die drei Schritte (Activities) sind vorkonfiguriert. Die roten Fehlerzahlen zeigen anfangs die noch fehlenden Verbindungen. Sie verschwinden nach dem Verbinden und Speichern. Bewege den Mauszeiger an den rechten Rand von „Manual Trigger“. Ziehe vom Anschluss zum linken Rand von „PowerShell“. Verbinde genauso PowerShell mit Return Data. Klicke oben auf das Diskettensymbol „Zwischen-Speichern“.', publish: 'Beide Verbindungen sind gespeichert. Klicke oben auf das Raketensymbol „Veröffentlichen“. Damit wird der Workflow für Ausführungen freigegeben.', run: 'Klicke oben auf das Play-Symbol „Ausführen“, um deinen Workflow zu starten.', running: 'Die Markierung zeigt, welcher Schritt gerade läuft. Danach kannst du das Ergebnis in Ruhe öffnen.', retry: 'Dieser Lauf war nicht erfolgreich. Öffne seine Ausführung, prüfe die Fehlermeldung und starte nach der Korrektur erneut über „Ausführen“.', done: 'Dein Workflow ist erfolgreich gelaufen. Klicke auf „Ausführung öffnen“ und suche bei „Return Data: greeting“ nach message. Dort sollte „Hello from NodePilot“ stehen.' }, decision: { title: 'Einen Entscheidungspfad verfolgen', work: 'Klicke oben auf das Play-Symbol „Ausführen“. Trage im Dialog bei freeSpaceGb den Wert 8 ein und klicke auf „Ausführen“. Das simuliert 8 GB freien Speicher: unter 10 GB soll der kritische Zweig laufen.', running: 'Verfolge „Decision: severity“ und den Zweig „critical (<10 GB)“. Die anderen Zweige sollen nicht laufen. Das Ergebnis bleibt danach in der Ausführung sichtbar.', retry: 'Klicke erneut auf „Ausführen“ und starte mit freeSpaceGb = 8. Ein anderer Wert nimmt einen anderen Zweig.', done: 'Bei 8 GB lief der kritische Zweig mit Ordneranalyse und Warnmail. Öffne die Ausführung: Bei „Decision: severity“ steht branch = critical, bei „Return“ stehen freeSpaceGb = 8 und severity = critical.' }, - parallel: { title: 'Parallele Arbeit beobachten', work: 'Klicke oben auf „Ausführen“ und im Dialog auf „Ausführen“. Lass path und olderThanDays unverändert. Nach dem Archivieren starten Prüfsumme und Bereinigung gleichzeitig. Alle Dateiaktionen sind simuliert.', running: 'Beobachte die beiden Zweige nach „zipOperation“. „Junction: waitAll“ ist die Zusammenführung: Sie wartet, bis beide Zweige fertig sind, bevor die Abschlussmail folgt.', retry: 'Der Lauf wurde nicht vollständig abgeschlossen. Starte ihn über „Ausführen“ erneut mit den vorgeschlagenen Werten.', done: 'Beide Zweige liefen überlappend. Klicke hier auf „Gantt-Zeitleiste öffnen“. Du kannst die Ergebnisse dort ohne Zeitdruck vergleichen. Die Balken für checksum und remove überlappen; Junction und Email beginnen danach.' }, + parallel: { title: 'Parallele Arbeit beobachten', work: 'Klicke oben auf „Ausführen“ und im Dialog auf „Ausführen“. Lass path und olderThanDays unverändert. Nach dem Archivieren starten Prüfsumme und Inventarisierung gleichzeitig. Alle Dateiaktionen sind simuliert.', running: 'Beobachte die beiden Zweige nach „zipOperation“. „Junction: waitAll“ ist die Zusammenführung: Sie wartet, bis beide Zweige fertig sind, bevor erfolgreiche Prüfungen die Bereinigung freigeben.', retry: 'Der Lauf wurde nicht vollständig abgeschlossen. Starte ihn über „Ausführen“ erneut mit den vorgeschlagenen Werten.', done: 'Beide Zweige liefen überlappend. Klicke hier auf „Gantt-Zeitleiste öffnen“. Du kannst die Ergebnisse dort ohne Zeitdruck vergleichen. Die Balken für checksum und inventory überlappen; Junction, Bereinigung und Email folgen nacheinander.' }, service: { title: 'Einen Dienst wiederherstellen', work: 'Klicke oben auf „Ausführen“. Lass serviceState auf Stopped und bestätige mit „Ausführen“. Das simuliert einen gestoppten Druckwarteschlangendienst (Spooler). Der Workflow soll ihn starten und den Status erneut prüfen.', running: 'Verfolge „start spooler“, die Wartezeit „Delay“ und „verify spooler state“. Erst die erneute Prüfung bestätigt, ob der Dienst läuft.', retry: 'Klicke erneut auf „Ausführen“ und starte mit serviceState = Stopped. Bei Running ist keine Wiederherstellung nötig.', done: 'Der Dienst wurde gestartet und erneut geprüft. Öffne die Ausführung: „verify spooler state“ meldet Running. Erst danach folgt „Email: spooler recovered“. Eine erfolgreiche Startanforderung allein wäre noch keine Bestätigung.' }, live: { title: 'Einen laufenden Workflow abbrechen', work: 'Klicke oben auf das Play-Symbol „Ausführen“. Der simulierte Prüfschritt bleibt für diese Übung aktiv, bis du ihn abbrichst. Du hast Zeit, die nächste Anweisung zu lesen.', running: 'Dein Lauf wartet im Prüfschritt. Klicke hier auf „Live Ops“, um ihn in der Zeitleiste zu finden.', inspect: 'Klicke in der Zeitleiste auf den laufenden Balken von „Live Ops Check“ – nicht auf die ID neben dem Namen. Klicke in den geöffneten Ausführungsdetails auf „Abbrechen“.', retry: 'Starte den Workflow erneut über „Ausführen“ und brich ihn in den Ausführungsdetails von Live Ops ab.', done: 'Der Status lautet „Abgebrochen“. Öffne die Ausführung: Der Prüfschritt wurde gestoppt, Return Data wurde nicht mehr ausgeführt. Der Workflow selbst bleibt aktiviert; nur dieser Lauf wurde beendet.' }, versions: { title: 'Zwei Workflow-Versionen vergleichen', work: 'Klicke oben auf „…“ (Weitere Designer-Aktionen) und dann auf „Diff gegen vorherige Version“. Wähle links „Version 7“. „Hinzugefügt“ zeigt, was der aktuelle Workflow gegenüber dieser älteren Version enthält.', answer: 'Sieh im Vergleich unter „Nodes hinzugefügt“ nach. Welche Activity fehlt in Version 7? Du kannst die Antwort hier neben dem geöffneten Vergleich auswählen.', done: 'In Version 7 fehlt „Return Data: result“. Der Vergleich zählt zwei Ergänzungen: den Schritt und seine Verbindung von Log. Du hast nur verglichen; keine Version wurde wiederhergestellt.' }, @@ -29,7 +29,7 @@ const COPY: Record<'de' | 'en', Record> = { en: { build: { title: 'Build a workflow', work: 'The three steps (activities) are configured for you. Hover over the right edge of Manual Trigger. Drag its connector to the left edge of PowerShell. Connect PowerShell to Return Data the same way. Click the disk icon “Save in place” in the toolbar.', publish: 'Both connections are saved. Click the rocket icon “Publish” in the toolbar to enable the workflow for execution.', run: 'Click the play icon “Run” in the toolbar to start your workflow.', running: 'The highlight shows which step is running. You can open the result afterwards at your own pace.', retry: 'This execution did not succeed. Open it, inspect the error, then correct the workflow and start another Run.', done: 'Your workflow succeeded. Click “Open execution” and find message under “Return Data: greeting”. It should say “Hello from NodePilot”.' }, decision: { title: 'Follow a decision', work: 'Click the play icon “Run” in the toolbar. Enter 8 for freeSpaceGb in the dialog and click “Run”. This simulates 8 GB free: below 10 GB, the critical branch should run.', running: 'Follow “Decision: severity” and the “critical (<10 GB)” branch. The other branches should not run. You can inspect the execution afterwards.', retry: 'Click “Run” again and enter freeSpaceGb = 8. Other values take different branches.', done: 'At 8 GB, the critical branch ran with folder analysis and an alert email. Open the execution: “Decision: severity” reports branch = critical; “Return” reports freeSpaceGb = 8 and severity = critical.' }, - parallel: { title: 'Watch parallel work', work: 'Click “Run”, then “Run” in the dialog. Leave path and olderThanDays unchanged. After archiving, checksum and cleanup start together. All file actions are simulated.', running: 'Watch the two branches after zipOperation. “Junction: waitAll” joins them: it waits for both to finish before the summary email can run.', retry: 'This execution did not finish both branches. Start another run with the suggested values using the play button.', done: 'Both branches overlapped. Click “Open Gantt timeline” here to compare the results at your own pace. The checksum and remove bars overlap; Junction and Email follow them.' }, + parallel: { title: 'Watch parallel work', work: 'Click “Run”, then “Run” in the dialog. Leave path and olderThanDays unchanged. After archiving, checksum and inventory start together. All file actions are simulated.', running: 'Watch the two branches after zipOperation. “Junction: waitAll” joins them: it waits for both checks to succeed before cleanup can run.', retry: 'This execution did not finish both branches. Start another run with the suggested values using the play button.', done: 'Both branches overlapped. Click “Open Gantt timeline” here to compare the results at your own pace. The checksum and inventory bars overlap; Junction, cleanup and Email follow in sequence.' }, service: { title: 'Recover a service', work: 'Click “Run”. Leave serviceState at Stopped and click “Run”. This simulates a stopped Print Spooler service. The workflow should start it and check its status again.', running: 'Follow “start spooler”, the Delay and “verify spooler state”. The second check confirms whether the service is running.', retry: 'Click “Run” again with serviceState = Stopped. Running does not need recovery.', done: 'The service was started and checked again. Open the execution: “verify spooler state” reports Running, followed by “Email: spooler recovered”. A successful start request alone would not confirm recovery.' }, live: { title: 'Control a live run', work: 'Click the play icon “Run”. For this exercise, the simulated check stays active until you cancel it. Take your time reading the next instruction.', running: 'Your execution is waiting in the check step. Click “Live Ops” here to find it in the timeline.', inspect: 'In the timeline, click the running bar for “Live Ops Check”, not the ID beside its name. Click “Cancel” in the execution details that open.', retry: 'Start another run with the play button and cancel it from the execution details in Live Ops.', done: 'The status is “Cancelled”. Open the execution: the check was stopped and Return Data did not run. The workflow remains enabled; only this execution was cancelled.' }, versions: { title: 'Compare two versions', work: 'Click “…” (More designer actions), then “Diff against a previous version”. Select “Version 7” on the left. “Added” shows what the current workflow contains compared with that older version.', answer: 'Look under “Nodes added” in the comparison. Which activity is missing from version 7? Select your answer here beside the open comparison.', done: 'Version 7 is missing “Return Data: result”. The diff counts two additions: the activity and its edge from Log. You compared versions without restoring either one.' }, @@ -78,13 +78,15 @@ function runMatches(mode: NewMissionId, runId: string): boolean { } if (mode === 'parallel') { const checksum = steps.find(step => step.stepId === 'checksum'); + const inventory = steps.find(step => step.stepId === 'inventory'); const purge = steps.find(step => step.stepId === 'purge'); const gather = steps.find(step => step.stepId === 'gather'); const mail = steps.find(step => step.stepId === 'mail'); - return run.status === 'Succeeded' && !!checksum?.startedAt && !!checksum.completedAt && !!purge?.startedAt && !!purge.completedAt && !!gather?.startedAt && !!gather.completedAt && !!mail?.startedAt - && Date.parse(checksum.startedAt) < Date.parse(purge.completedAt) && Date.parse(purge.startedAt) < Date.parse(checksum.completedAt) - && Date.parse(gather.startedAt) >= Math.max(Date.parse(checksum.completedAt), Date.parse(purge.completedAt)) - && Date.parse(mail.startedAt) >= Date.parse(gather.completedAt ?? ''); + return run.status === 'Succeeded' && !!checksum?.startedAt && !!checksum.completedAt && !!inventory?.startedAt && !!inventory.completedAt && !!purge?.startedAt && !!purge.completedAt && !!gather?.startedAt && !!gather.completedAt && !!mail?.startedAt + && Date.parse(checksum.startedAt) < Date.parse(inventory.completedAt) && Date.parse(inventory.startedAt) < Date.parse(checksum.completedAt) + && Date.parse(gather.startedAt) >= Math.max(Date.parse(checksum.completedAt), Date.parse(inventory.completedAt)) + && Date.parse(purge.startedAt) >= Date.parse(gather.completedAt) + && Date.parse(mail.startedAt) >= Date.parse(purge.completedAt); } return mode === 'live' && run.status === 'Cancelled'; } diff --git a/src/nodepilot-ui/e2e-demo/demo-smoke.spec.ts b/src/nodepilot-ui/e2e-demo/demo-smoke.spec.ts index 894bdf33..0906443a 100644 --- a/src/nodepilot-ui/e2e-demo/demo-smoke.spec.ts +++ b/src/nodepilot-ui/e2e-demo/demo-smoke.spec.ts @@ -625,9 +625,8 @@ test.describe('published browser demo', () => { await fetch(`/api/workflows/${id}/publish`, { method: 'POST', body: JSON.stringify({ name: 'Tab A edit' }) }); }, target.id); - // Opening and reloading a second tab publishes an identity event. Without the isolation - // guard every other tab answers it by clearing its caches and remounting, which discards - // unsaved editor state. + // Separate demo worlds keep their own data across tab initialization and reload. + // Their auth transport is isolated as well, including real login/logout boundaries. const tabB = await context.newPage(); await tabB.goto('./workflows'); await tabB.reload(); diff --git a/src/nodepilot-ui/e2e/step-test.spec.ts b/src/nodepilot-ui/e2e/step-test.spec.ts index 65a48197..a2b305f0 100644 --- a/src/nodepilot-ui/e2e/step-test.spec.ts +++ b/src/nodepilot-ui/e2e/step-test.spec.ts @@ -7,8 +7,8 @@ import { installDefaultMocks, MOCK_USER, seedExpertMode } from './fixtures/mockA * StepTestPanel (components/designer/properties/StepTestPanel.tsx) is mounted inside the * PropertiesPanel's collapsible "Test & Debug" section for any non-trigger activity that has a * workflowId. It: - * - POSTs /api/workflows/{id}/steps/{stepId}/test with { mockVariables, configOverride }, where - * configOverride is the live editor config so unsaved edits are tested, + * - POSTs /api/workflows/{id}/steps/{stepId}/test with { mockVariables, stepOverride }, where + * stepOverride contains the complete live step, including target and configuration, * - has four modes (Empty / Last run / Pick a run / Manual mocks); lastRun and pickRun pull from * GET .../test-context and /test-context/runs, * - renders the result block with output, error output, and structured params. @@ -22,7 +22,7 @@ import { installDefaultMocks, MOCK_USER, seedExpertMode } from './fixtures/mockA * 28.1 — Empty mode runs the step and shows live output; /test is the only POST, never /execute. * 28.1b — redacted output ("***") surfaces in the result block. * 28.2 — Manual mocks: key=value lines are parsed into mockVariables on the /test POST. - * 28.3 — configOverride carries the live editor config, not the config saved in the database. + * 28.3 — stepOverride carries the live editor step, not the configuration saved in the database. * 28.6 — Viewer: the Run button is hidden (canRun=false); the API rejects the call as well. */ @@ -99,9 +99,9 @@ test.describe('Step-Test mit Kontext (Teil 28)', () => { await expect(runButton(page)).toBeVisible({ timeout: 10_000 }); await runButton(page).click(); - // The step-test POST fired; its body has configOverride and no mockVariables (Empty mode). + // The step-test POST carries the complete live step and no mockVariables (Empty mode). await expect.poll(() => calls.test, { timeout: 10_000 }).not.toBeNull(); - expect((calls.test as { configOverride?: Record }).configOverride).toMatchObject({ script: 'Get-PSDrive C' }); + expect((calls.test as { stepOverride?: Record }).stepOverride).toMatchObject({ activityType: 'runScript', config: { script: 'Get-PSDrive C' } }); expect((calls.test as { mockVariables?: unknown }).mockVariables).toBeUndefined(); // Live output is shown; no execution was started. @@ -158,7 +158,7 @@ test.describe('Step-Test mit Kontext (Teil 28)', () => { }); }); - test('28.3 — configOverride carries the LIVE (unsaved) editor config, not the DB-saved one', async ({ page }) => { + test('28.3 — stepOverride carries the live unsaved editor configuration', async ({ page }) => { const sink: { body: Record | null } = { body: null }; await page.route(`**/api/workflows/${WF_ID}/steps/step-probe/test`, (route) => { sink.body = route.request().postDataJSON(); @@ -169,7 +169,7 @@ test.describe('Step-Test mit Kontext (Teil 28)', () => { }); // DB-saved script is "echo OLD". Open it, then edit the script in the RunScript config editor - // to "echo NEW" WITHOUT saving — the panel must send the live value as configOverride. + // to "echo NEW" WITHOUT saving — the panel must include it in the live step snapshot. await openStepTest(page, { script: 'echo OLD' }); // The RunScript config exposes a CodeMirror/textarea holding the script. Find the editable @@ -188,9 +188,9 @@ test.describe('Step-Test mit Kontext (Teil 28)', () => { await runButton(page).click(); await expect.poll(() => sink.body, { timeout: 10_000 }).not.toBeNull(); - const override = (sink.body as { configOverride?: { script?: string } }).configOverride; - expect(override?.script).toContain('echo NEW'); - expect(override?.script).not.toContain('echo OLD'); + const override = (sink.body as { stepOverride?: { config?: { script?: string } } }).stepOverride; + expect(override?.config?.script).toContain('echo NEW'); + expect(override?.config?.script).not.toContain('echo OLD'); }); test('28.6 — Viewer cannot run a step-test: the Run button is hidden', async ({ page }) => { diff --git a/src/nodepilot-ui/src/__tests__/components/ProtectedRoute.test.tsx b/src/nodepilot-ui/src/__tests__/components/ProtectedRoute.test.tsx index 7e2e5fdc..58f968dd 100644 --- a/src/nodepilot-ui/src/__tests__/components/ProtectedRoute.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/ProtectedRoute.test.tsx @@ -2,8 +2,10 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import * as React from 'react'; import { act, fireEvent, render, screen } from '@testing-library/react'; import { MemoryRouter, Routes, Route } from 'react-router'; -import { useAuthStore } from '../../stores/authStore'; +import { startAuthBoundarySynchronization, useAuthStore } from '../../stores/authStore'; import { ProtectedRoute } from '../../components/ProtectedRoute'; +import { api } from '../../api/client'; +import { AUTH_BOUNDARY_STORAGE_KEY, type AuthBoundaryEvent } from '../../security/authBoundary'; describe('ProtectedRoute', () => { beforeEach(() => { @@ -99,4 +101,46 @@ describe('ProtectedRoute', () => { expect(unmounted).toHaveBeenCalledOnce(); expect(screen.getByRole('textbox', { name: 'private draft' })).toHaveValue(''); }); + + it('observingTheSameSessionInAnotherTabPreservesProtectedDrafts', async () => { + function Draft() { + const [value, setValue] = React.useState(''); + return setValue(event.target.value)} />; + } + vi.stubGlobal('BroadcastChannel', undefined); + const stop = startAuthBoundarySynchronization(); + const get = vi.spyOn(api, 'get').mockResolvedValue({ id: 'u-a', username: 'alice', role: 'Admin' }); + const storageWrite = vi.spyOn(Storage.prototype, 'setItem'); + try { + useAuthStore.getState().acceptAuthenticatedIdentity({ + userId: 'u-a', username: 'alice', role: 'Admin', + }, { broadcastIdentity: false }); + render(); + fireEvent.change(screen.getByRole('textbox', { name: 'private draft' }), { + target: { value: 'Unsaved work in the first tab' }, + }); + storageWrite.mockClear(); + + // Deliver the real initialization message as if a second tab had sent it. + await act(() => useAuthStore.getState().initialize()); + const messages = storageWrite.mock.calls + .filter(([key]) => key === AUTH_BOUNDARY_STORAGE_KEY) + .map(([, value]) => JSON.parse(value) as AuthBoundaryEvent); + act(() => { + for (const event of messages) { + window.dispatchEvent(new StorageEvent('storage', { + key: AUTH_BOUNDARY_STORAGE_KEY, + newValue: JSON.stringify({ ...event, sourceId: 'second-tab', eventId: 'same-session-probe' }), + })); + } + }); + + expect(screen.getByRole('textbox', { name: 'private draft' })).toHaveValue('Unsaved work in the first tab'); + expect(get).toHaveBeenCalledTimes(1); + } finally { + stop(); + vi.restoreAllMocks(); + vi.unstubAllGlobals(); + } + }); }); diff --git a/src/nodepilot-ui/src/__tests__/components/overlays/CommandPalette.test.tsx b/src/nodepilot-ui/src/__tests__/components/overlays/CommandPalette.test.tsx index 8533d580..5372516d 100644 --- a/src/nodepilot-ui/src/__tests__/components/overlays/CommandPalette.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/overlays/CommandPalette.test.tsx @@ -109,6 +109,24 @@ describe('CommandPalette', () => { expect(onClose).toHaveBeenCalledOnce(); }); + it('enterAfterHoverInvokesTheVisibleCommandAcrossInterleavedGroups', () => { + const publish = vi.fn(); + const exportPng = vi.fn(); + render(); + + const exportButton = screen.getByRole('button', { name: 'Export canvas as PNG' }); + fireEvent.mouseEnter(exportButton); + expect(exportButton).toHaveClass('bg-primary/10'); + fireEvent.keyDown(screen.getByRole('textbox'), { key: 'Enter' }); + + expect(exportPng).toHaveBeenCalledOnce(); + expect(publish).not.toHaveBeenCalled(); + }); + it('escapeKeyClosesWithoutInvoking', () => { const onClose = vi.fn(); const run = vi.fn(); @@ -147,6 +165,46 @@ describe('CommandPalette', () => { expect(runA).toHaveBeenCalledOnce(); }); + it('arrowKeysFollowVisibleGroupedOrderAndKeepSelectionWhenCommandsReorder', () => { + const first = makeCmd('first', 'First edit', { group: 'Edit' }); + const run = makeCmd('run', 'Run workflow', { group: 'Run' }); + const second = makeCmd('second', 'Second edit', { group: 'Edit' }); + const { rerender } = render(); + const input = screen.getByRole('textbox'); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(screen.getByRole('button', { name: 'Second edit' })).toHaveClass('bg-primary/10'); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + expect(screen.getByRole('button', { name: 'Run workflow' })).toHaveClass('bg-primary/10'); + fireEvent.keyDown(input, { key: 'ArrowUp' }); + + rerender(); + expect(screen.getByRole('button', { name: 'Second edit' })).toHaveClass('bg-primary/10'); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(second.run).toHaveBeenCalledOnce(); + expect(first.run).not.toHaveBeenCalled(); + expect(run.run).not.toHaveBeenCalled(); + }); + + it('queryChangesSelectTheFirstVisibleResultAndNeverInvokeDisabledOrMissingResults', () => { + const save = makeCmd('save', 'Save'); + const disabled = makeCmd('publish', 'Publish', { disabled: true }); + render(); + const input = screen.getByRole('textbox'); + + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'Enter' }); + fireEvent.change(input, { target: { value: 'zzz' } }); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(disabled.run).not.toHaveBeenCalled(); + expect(save.run).not.toHaveBeenCalled(); + + fireEvent.change(input, { target: { value: 'save' } }); + fireEvent.keyDown(input, { key: 'Enter' }); + expect(save.run).toHaveBeenCalledOnce(); + }); + it('clickOnBackdropClosesPalette', () => { const onClose = vi.fn(); render(); diff --git a/src/nodepilot-ui/src/__tests__/components/properties/StepTestPanel.test.tsx b/src/nodepilot-ui/src/__tests__/components/properties/StepTestPanel.test.tsx index 2d152830..91c17f94 100644 --- a/src/nodepilot-ui/src/__tests__/components/properties/StepTestPanel.test.tsx +++ b/src/nodepilot-ui/src/__tests__/components/properties/StepTestPanel.test.tsx @@ -5,7 +5,7 @@ import { StepTestPanel } from '../../../components/designer/properties/StepTestP /** * Covers StepTestPanel: rendering and switching all four mode tabs, "Run test" sending the - * configOverride and the selected mocks, "Pick a run" keeping the Run button disabled until a + * complete step snapshot and the selected mocks, "Pick a run" keeping the Run button disabled until a * run is chosen, the manual mocks editor parsing key=value lines while skipping comments and * blank lines, and last-run mode forwarding the context variables without globals. */ @@ -45,7 +45,7 @@ describe('StepTestPanel', () => { fetchMock.mockImplementation((url: string) => url.includes('/test-context') ? pending.promise : Promise.resolve(jsonResponse({ success: true, output: 'OK', outputParameters: {}, durationMs: 1 }))); - wrap(); + wrap(); const run = screen.getByRole('button', { name: /Test activity/i }); expect(run).toBeDisabled(); @@ -65,7 +65,7 @@ describe('StepTestPanel', () => { it('shows a failed context load in standard mode and allows retry before running', async () => { fetchMock.mockRejectedValueOnce(new Error('Context unavailable')); - wrap(); + wrap(); expect(await screen.findByRole('alert')).toHaveTextContent(/context/i); expect(screen.getByRole('button', { name: /Test activity/i })).toBeDisabled(); @@ -83,7 +83,7 @@ describe('StepTestPanel', () => { fetchMock.mockReturnValue(pending.promise); const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); const panel = (workflowId: string, stepId: string) => - + ; const view = render(panel('wf1', 'step1')); fireEvent.click(screen.getByRole('button', { name: /Run test/i })); @@ -105,7 +105,7 @@ describe('StepTestPanel', () => { ])); return pending.promise; }); - wrap(); + wrap(); fireEvent.click(screen.getByRole('button', { name: /^Pick a run$/ })); await waitFor(() => expect(fetchMock.mock.calls.some(([url]) => String(url).endsWith('?executionId=exec1'))).toBe(true)); expect(screen.getByRole('button', { name: /Run test/i })).toBeDisabled(); @@ -119,7 +119,7 @@ describe('StepTestPanel', () => { , @@ -135,7 +135,7 @@ describe('StepTestPanel', () => { , ); @@ -152,7 +152,7 @@ describe('StepTestPanel', () => { , ); @@ -164,7 +164,7 @@ describe('StepTestPanel', () => { success: false, output: '', errorOutput: 'boom', outputParameters: {}, durationMs: 5, errorMessage: 'failed', })); - wrap(); + wrap(); fireEvent.click(screen.getByRole('button', { name: /Run test/i })); const block = (await screen.findByText('boom')).closest('div.rounded-md')!; @@ -173,7 +173,7 @@ describe('StepTestPanel', () => { expect(block.className).not.toMatch(/bg-(red|green)-50/); }); - it('Empty mode test sends configOverride + no mockVariables', async () => { + it('Empty mode sends the complete live step and no mock variables', async () => { fetchMock.mockResolvedValueOnce(jsonResponse({ success: true, output: 'OK', errorOutput: null, outputParameters: {}, durationMs: 12, errorMessage: null, @@ -183,7 +183,7 @@ describe('StepTestPanel', () => { , ); @@ -194,7 +194,8 @@ describe('StepTestPanel', () => { const call = fetchMock.mock.calls[0]; expect(call[0]).toContain('/workflows/wf1/steps/step1/test'); const body = JSON.parse((call[1] as RequestInit).body as string); - expect(body.configOverride).toEqual({ script: 'Get-Process', timeoutSeconds: 30 }); + expect(body.stepOverride).toEqual({ activityType: 'runScript', targetMachineId: 'host-b', credentialId: 'credential-b', config: { script: 'Get-Process', timeoutSeconds: 30 } }); + expect(body.configOverride).toBeUndefined(); expect(body.mockVariables).toBeUndefined(); expect(await screen.findByText(/Succeeded/i)).toBeInTheDocument(); @@ -210,7 +211,7 @@ describe('StepTestPanel', () => { , ); @@ -258,7 +259,7 @@ describe('StepTestPanel', () => { , ); @@ -305,7 +306,7 @@ describe('StepTestPanel', () => { , ); diff --git a/src/nodepilot-ui/src/__tests__/demo/isolation.test.ts b/src/nodepilot-ui/src/__tests__/demo/isolation.test.ts index 8e169d4b..20320e1f 100644 --- a/src/nodepilot-ui/src/__tests__/demo/isolation.test.ts +++ b/src/nodepilot-ui/src/__tests__/demo/isolation.test.ts @@ -1,9 +1,9 @@ /** * Guards for the three boundaries the demo depends on. * - * 1. Tab isolation. The app synchronises identity across tabs; opening or reloading a second - * tab makes every other tab clear its caches and remount, discarding unsaved editor state. - * An in-memory backend does not prevent that, because the channel is purely client-side. + * 1. Tab isolation. The app synchronises real identity/session changes across tabs, clearing + * protected state. Independent demo worlds must not share those boundaries even though + * their client-side transport runs on the same origin. * 2. Dependency direction. `src/` must never import from `demo/`. That is what makes the * product build unable to reach the demo, and the hub and docs seams are injected for * exactly this reason. diff --git a/src/nodepilot-ui/src/__tests__/demo/missionScenarios.test.ts b/src/nodepilot-ui/src/__tests__/demo/missionScenarios.test.ts index ed46d8a3..b6b213fc 100644 --- a/src/nodepilot-ui/src/__tests__/demo/missionScenarios.test.ts +++ b/src/nodepilot-ui/src/__tests__/demo/missionScenarios.test.ts @@ -43,17 +43,18 @@ describe('guided demo executions', () => { expect(getWorld().steps.get(run.id)).toEqual([]); }); - it('overlaps both cleanup branches and waits for both before the email', async () => { + it('overlaps both read-only checks and waits for both before cleanup and email', async () => { const workflow = ensureMissionWorkflow('parallel')!; const run = startRun(workflow.id)!; await vi.runAllTimersAsync(); const steps = getWorld().steps.get(run.id)!; const byId = (id: string) => steps.find(step => step.stepId === id)!; expect(run.status).toBe('Succeeded'); - expect(Date.parse(byId('checksum').startedAt!)).toBeLessThan(Date.parse(byId('purge').completedAt!)); - expect(Date.parse(byId('purge').startedAt!)).toBeLessThan(Date.parse(byId('checksum').completedAt!)); - expect(Date.parse(byId('gather').startedAt!)).toBeGreaterThanOrEqual(Math.max(Date.parse(byId('checksum').completedAt!), Date.parse(byId('purge').completedAt!))); - expect(Date.parse(byId('mail').startedAt!)).toBeGreaterThanOrEqual(Date.parse(byId('gather').completedAt!)); + expect(Date.parse(byId('checksum').startedAt!)).toBeLessThan(Date.parse(byId('inventory').completedAt!)); + expect(Date.parse(byId('inventory').startedAt!)).toBeLessThan(Date.parse(byId('checksum').completedAt!)); + expect(Date.parse(byId('gather').startedAt!)).toBeGreaterThanOrEqual(Math.max(Date.parse(byId('checksum').completedAt!), Date.parse(byId('inventory').completedAt!))); + expect(Date.parse(byId('purge').startedAt!)).toBeGreaterThanOrEqual(Date.parse(byId('gather').completedAt!)); + expect(Date.parse(byId('mail').startedAt!)).toBeGreaterThanOrEqual(Date.parse(byId('purge').completedAt!)); }); it('starts and verifies a stopped service before reporting recovery', async () => { diff --git a/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx b/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx index 5256a915..08b6ba5e 100644 --- a/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx +++ b/src/nodepilot-ui/src/__tests__/pages/WorkflowEditorPage.test.tsx @@ -241,9 +241,80 @@ async function openToolsMenu() { fireEvent.click(await screen.findByTestId('tools-menu-trigger')); } +describe('WorkflowEditorPage — double-click script editing', () => { + it('marks an applied script dirty, saves with Ctrl+S, and restores the prior script with Undo', async () => { + const writes: { definitionJson: string }[] = []; + server.use(http.put(`${BASE}/api/workflows/wf-smoke-1`, async ({ request }) => { + writes.push(await request.json() as { definitionJson: string }); + return new HttpResponse(null, { status: 204 }); + })); + renderPage(); + await waitForCanvasReady(); + fireEvent.doubleClick(document.querySelector('.react-flow__node[data-id="step-b"]')!); + fireEvent.change(await screen.findByTestId('monaco-editor-mock'), { + target: { value: "Write-Output 'updated script'" }, + }); + fireEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: /save.*close/i })); + await waitFor(() => expect(screen.queryByRole('dialog')).not.toBeInTheDocument()); + + const leave = new Event('beforeunload', { cancelable: true }); + window.dispatchEvent(leave); + expect(leave.defaultPrevented).toBe(true); + fireEvent.keyDown(window, { key: 's', ctrlKey: true }); + await waitFor(() => expect(writes).toHaveLength(1)); + const saved = JSON.parse(writes[0].definitionJson); + expect(saved.nodes.find((node: { id: string }) => node.id === 'step-b').data.config.script) + .toBe("Write-Output 'updated script'"); + + fireEvent.keyDown(window, { key: 'z', ctrlKey: true }); + fireEvent.doubleClick(document.querySelector('.react-flow__node[data-id="step-b"]')!); + expect(await screen.findByTestId('monaco-editor-mock')).toHaveValue('Get-PSDrive C'); + }); +}); + +describe('WorkflowEditorPage — step-test target consistency', () => { + it('tests the whole edited step before autosave without mixing in the saved target', async () => { + const oldMachine = '22222222-2222-2222-2222-222222222222'; + const newMachine = '33333333-3333-3333-3333-333333333333'; + const definition = JSON.parse(MOCK_WORKFLOW.definitionJson); + definition.nodes[1].data.targetMachineId = oldMachine; + const puts: { definitionJson: string }[] = []; + const requests: { stepOverride?: Record; configOverride?: unknown }[] = []; + server.use( + http.get(`${BASE}/api/workflows/wf-smoke-1`, () => HttpResponse.json({ ...MOCK_WORKFLOW, definitionJson: JSON.stringify(definition) })), + http.put(`${BASE}/api/workflows/wf-smoke-1`, async ({ request }) => { + const body = await request.json() as { definitionJson: string }; + puts.push(body); + return new HttpResponse(null, { status: 204 }); + }), + http.post(`${BASE}/api/workflows/wf-smoke-1/steps/step-b/test`, async ({ request }) => { + requests.push(await request.json() as typeof requests[number]); + return HttpResponse.json({ success: true, output: 'tested live step', outputParameters: {}, durationMs: 1 }); + }), + ); + const view = renderPage(); + try { + await waitForCanvasReady(); + fireEvent.click(document.querySelector('.react-flow__node[data-id="step-b"]')!); + fireEvent.change(await screen.findByDisplayValue(oldMachine), { target: { value: newMachine } }); + fireEvent.change(screen.getByDisplayValue('Get-PSDrive C'), { target: { value: 'Write-Output "new target test"' } }); + fireEvent.click(screen.getByText('Test & Debug')); + fireEvent.click(await screen.findByRole('button', { name: 'Run test (Empty)' })); + await waitFor(() => expect(requests).toHaveLength(1)); + expect(requests[0].stepOverride).toMatchObject({ + activityType: 'runScript', + targetMachineId: newMachine, + config: { script: 'Write-Output "new target test"' }, + }); + expect(requests[0].configOverride).toBeUndefined(); + expect(puts).toHaveLength(0); + } finally { view.unmount(); } + }); +}); + describe('WorkflowEditorPage — smoke + toolbar', () => { it.each(['double-click', 'properties'] as const)('tests the current unsaved script from the %s editor', async (entry) => { - let sent: { configOverride?: { script?: string } } | undefined; + let sent: { stepOverride?: { config?: { script?: string }; targetMachineId?: string }; configOverride?: unknown } | undefined; server.use(http.post(`${BASE}/api/workflows/wf-smoke-1/steps/step-b/test`, async ({ request }) => { sent = await request.json() as typeof sent; return HttpResponse.json({ success: true, output: 'current script', outputParameters: {}, durationMs: 1 }); @@ -251,6 +322,8 @@ describe('WorkflowEditorPage — smoke + toolbar', () => { renderPage(); await waitForCanvasReady(); const node = document.querySelector('.react-flow__node[data-id="step-b"]')!; + fireEvent.click(node); + fireEvent.change(await screen.findByDisplayValue('machine-test'), { target: { value: 'unsaved-new-target' } }); if (entry === 'double-click') fireEvent.doubleClick(node); else { fireEvent.click(node); @@ -258,7 +331,9 @@ describe('WorkflowEditorPage — smoke + toolbar', () => { } fireEvent.change(await screen.findByTestId('monaco-editor-mock'), { target: { value: "Write-Output 'updated safe script'" } }); fireEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: /^run$/i })); - await waitFor(() => expect(sent?.configOverride?.script).toBe("Write-Output 'updated safe script'")); + await waitFor(() => expect(sent?.stepOverride?.config?.script).toBe("Write-Output 'updated safe script'")); + expect(sent?.stepOverride?.targetMachineId).toBe('unsaved-new-target'); + expect(sent?.configOverride).toBeUndefined(); }); it('loads machine options without requesting operational statistics', async () => { @@ -1804,6 +1879,28 @@ describe('WorkflowEditorPage — Overlays via Buttons & Keys', () => { await waitFor(() => expect(screen.getByPlaceholderText(/Type a command/)).toBeInTheDocument()); }); + it('Enter on the highlighted PNG export runs the export without publishing', async () => { + const publish = vi.fn(); + server.use(http.post(`${BASE}/api/workflows/wf-smoke-1/publish`, () => { + publish(); + return HttpResponse.json(MOCK_PRODUCTIVE); + })); + const anchorClick = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}); + renderPage(); + await waitForCanvasReady(); + fireEvent.keyDown(window, { key: 'P', ctrlKey: true, shiftKey: true }); + const search = await screen.findByPlaceholderText(/Type a command/); + fireEvent.change(search, { target: { value: 'run' } }); + const exportButton = screen.getByRole('button', { name: /Export canvas as PNG/ }); + fireEvent.mouseEnter(exportButton); + expect(exportButton).toHaveClass('bg-primary/10'); + fireEvent.keyDown(search, { key: 'Enter' }); + + await waitFor(() => expect(anchorClick).toHaveBeenCalledOnce()); + expect(toBlobMock).toHaveBeenCalledOnce(); + expect(publish).not.toHaveBeenCalled(); + }); + it('Search opens via Ctrl+F', async () => { renderPage(); await waitFor(() => expect(screen.getByDisplayValue('Smoke Workflow')).toBeInTheDocument()); diff --git a/src/nodepilot-ui/src/__tests__/security/authBoundary.test.ts b/src/nodepilot-ui/src/__tests__/security/authBoundary.test.ts index 66f9b471..41a1236a 100644 --- a/src/nodepilot-ui/src/__tests__/security/authBoundary.test.ts +++ b/src/nodepilot-ui/src/__tests__/security/authBoundary.test.ts @@ -6,6 +6,7 @@ import { handleStaleAuthCookieResponseBoundary, isAuthBoundaryGenerationCurrent, publishAuthenticatedIdentity, + publishObservedSession, registerAuthBoundaryIdentityReprober, registerAuthBoundaryLiveStateClearer, registerAuthBoundaryQueryCacheClearer, @@ -35,7 +36,7 @@ class FakeBroadcastChannel { if (type === 'message') this.messageListeners.delete(listener as (event: MessageEvent) => void); } - emitRemote(data: AuthBoundaryEvent): void { + emitRemote(data: unknown): void { for (const listener of this.messageListeners) listener({ data } as MessageEvent); } } @@ -105,6 +106,27 @@ describe('authBoundary', () => { unsubscribe(); }); + it('transportsSessionObservationsAndRejectsMalformedClaims', () => { + vi.stubGlobal('BroadcastChannel', FakeBroadcastChannel); + const listener = vi.fn(); + const unsubscribe = subscribeToAuthBoundaryEvents(listener); + const channel = FakeBroadcastChannel.instances[0]; + const observed: AuthBoundaryEvent = { + version: 1, type: 'identity', userId: 'u-1', observation: { username: 'alice', role: 'Admin' }, + sourceId: 'another-tab', eventId: 'observed-valid', + }; + channel.emitRemote(observed); + channel.emitRemote({ ...observed, eventId: 'observed-invalid', userId: '' }); + channel.emitRemote({ ...observed, eventId: 'observed-missing-name', observation: { role: 'Admin' } }); + channel.emitRemote({ ...observed, eventId: 'observed-invalid-role', observation: { username: 'alice', role: null } }); + expect(listener).toHaveBeenCalledExactlyOnceWith(observed); + publishObservedSession({ userId: 'u-1', username: 'alice', role: 'Admin' }); + expect(channel.postMessage).toHaveBeenCalledWith(expect.objectContaining({ + type: 'identity', userId: 'u-1', observation: { username: 'alice', role: 'Admin' }, + })); + unsubscribe(); + }); + it('clears live state, sensitive storage and registered query caches in one boundary', () => { const clearLive = vi.fn(); const clearQueries = vi.fn(); diff --git a/src/nodepilot-ui/src/__tests__/stores/authStore.test.ts b/src/nodepilot-ui/src/__tests__/stores/authStore.test.ts index 87018bb3..412852ae 100644 --- a/src/nodepilot-ui/src/__tests__/stores/authStore.test.ts +++ b/src/nodepilot-ui/src/__tests__/stores/authStore.test.ts @@ -50,6 +50,7 @@ describe('authStore (cookie-based, audit H-5)', () => { localStorage.clear(); useAiChatStore.setState({ messagesByThread: {}, threadsByScope: {}, activeThreadByScope: {} }); sessionStorage.clear(); + document.cookie = 'np_csrf=; Max-Age=0; path=/'; queryClient.clear(); // Reset store state to a known "pre-init" shape (matches production bundle load). useAuthStore.setState({ @@ -62,6 +63,7 @@ describe('authStore (cookie-based, audit H-5)', () => { afterEach(() => { while (synchronizationStops.length > 0) synchronizationStops.pop()?.(); + document.cookie = 'np_csrf=; Max-Age=0; path=/'; vi.unstubAllGlobals(); }); @@ -70,6 +72,8 @@ describe('authStore (cookie-based, audit H-5)', () => { // username + role and relies on the httpOnly np_auth cookie for auth. const mockResponse = { userId: 'u-1', username: 'admin', role: 'Admin' }; vi.mocked(api.post).mockResolvedValueOnce(mockResponse); + vi.stubGlobal('BroadcastChannel', undefined); + const storageWrite = vi.spyOn(Storage.prototype, 'setItem'); await useAuthStore.getState().login('admin', 'password'); @@ -77,6 +81,7 @@ describe('authStore (cookie-based, audit H-5)', () => { expect(state.isAuthenticated).toBe(true); expect(state.username).toBe('admin'); expect(state.role).toBe('Admin'); + expect(storageWrite).toHaveBeenCalledWith(AUTH_BOUNDARY_STORAGE_KEY, expect.stringContaining('"type":"identity"')); // No token field in state — the cookie is the source of truth. expect((state as unknown as { token?: string }).token).toBeUndefined(); }); @@ -248,6 +253,62 @@ describe('authStore (cookie-based, audit H-5)', () => { expect(localStorage.getItem(AI_CHAT_STORAGE_KEY)).toBeNull(); }); + it('initialize_publishesAnObservationWithoutTransmittingTheCookie', async () => { + document.cookie = 'np_csrf=session-companion-secret; path=/'; + vi.stubGlobal('BroadcastChannel', undefined); + const storageWrite = vi.spyOn(Storage.prototype, 'setItem'); + storageWrite.mockClear(); + vi.mocked(api.get).mockResolvedValueOnce({ id: 'u-1', username: 'alice', role: 'Admin' }); + + await useAuthStore.getState().initialize(); + + const messages = storageWrite.mock.calls + .filter(([key]) => key === AUTH_BOUNDARY_STORAGE_KEY) + .map(([, value]) => JSON.parse(value)); + expect(messages).toEqual([expect.objectContaining({ + type: 'identity', userId: 'u-1', observation: { username: 'alice', role: 'Admin' }, + })]); + expect(JSON.stringify(messages)).not.toContain('session-companion-secret'); + }); + + it.each(['cookie', 'user', 'role', 'username'] as const)( + 'remoteSessionObservation_%sChange_clearsAndReprobesWithoutEcho', async (change) => { + document.cookie = 'np_csrf=original-session; path=/'; + useAuthStore.getState().acceptAuthenticatedIdentity({ + userId: 'u-1', username: 'alice', role: 'Admin', + }, { broadcastIdentity: false }); + sessionStorage.setItem(DB_ADMIN_QUERY_DRAFT_KEY, 'SELECT previous_session_data'); + queryClient.setQueryData(['private-result'], { secret: true }); + const observed = { + userId: change === 'user' ? 'u-2' : 'u-1', + username: change === 'username' ? 'renamed' : 'alice', + role: change === 'role' ? 'Viewer' : 'Admin', + }; + if (change === 'cookie') document.cookie = 'np_csrf=oidc-new-session; path=/'; + vi.mocked(api.get).mockResolvedValueOnce({ id: observed.userId, username: observed.username, role: observed.role }); + vi.stubGlobal('BroadcastChannel', undefined); + synchronizationStops.push(startAuthBoundarySynchronization()); + const storageWrite = vi.spyOn(Storage.prototype, 'setItem'); + storageWrite.mockClear(); + + window.dispatchEvent(new StorageEvent('storage', { + key: AUTH_BOUNDARY_STORAGE_KEY, + newValue: JSON.stringify({ + version: 1, type: 'identity', userId: observed.userId, + observation: { username: observed.username, role: observed.role }, + sourceId: 'another-tab', eventId: `observed-${change}`, + } satisfies AuthBoundaryEvent), + })); + + expect(useAuthStore.getState().isAuthenticated).toBeNull(); + expect(sessionStorage.getItem(DB_ADMIN_QUERY_DRAFT_KEY)).toBeNull(); + expect(queryClient.getQueryData(['private-result'])).toBeUndefined(); + await vi.waitFor(() => expect(useAuthStore.getState()).toMatchObject({ ...observed, isAuthenticated: true })); + expect(api.get).toHaveBeenCalledWith('/auth/me', { broadcastUnauthorized: false }); + expect(storageWrite).not.toHaveBeenCalledWith(AUTH_BOUNDARY_STORAGE_KEY, expect.any(String)); + }, + ); + it('initialize_secondUser_discardsFirstUsersSqlAndAiState', async () => { vi.mocked(api.get) .mockResolvedValueOnce({ id: 'u-1', username: 'first', role: 'Admin' }) @@ -522,6 +583,38 @@ describe('authStore (cookie-based, audit H-5)', () => { }); }); + it('remoteSessionObservation_newOidcCookieRecoversFromAnAbandonedLogout', async () => { + document.cookie = 'np_csrf=session-before-logout; path=/'; + useAuthStore.getState().acceptAuthenticatedIdentity({ + userId: 'u-1', username: 'alice', role: 'Admin', + }, { broadcastIdentity: false }); + vi.mocked(api.get).mockResolvedValueOnce({ id: 'u-1', username: 'alice', role: 'Admin' }); + vi.stubGlobal('BroadcastChannel', undefined); + synchronizationStops.push(startAuthBoundarySynchronization()); + const dispatch = (event: AuthBoundaryEvent) => window.dispatchEvent(new StorageEvent('storage', { + key: AUTH_BOUNDARY_STORAGE_KEY, + newValue: JSON.stringify(event), + })); + dispatch({ + version: 1, type: 'logout', phase: 'started', + sourceId: 'closed-tab', eventId: 'abandoned-logout-started', + }); + const observation = { + version: 1, type: 'identity', userId: 'u-1', observation: { username: 'alice', role: 'Admin' }, + sourceId: 'oidc-tab', eventId: 'observation-during-logout', + } satisfies AuthBoundaryEvent; + dispatch(observation); + expect(useAuthStore.getState().isAuthenticated).toBe(false); + expect(api.get).not.toHaveBeenCalled(); + + document.cookie = 'np_csrf=new-oidc-session; path=/'; + dispatch({ ...observation, eventId: 'new-oidc-session-after-abandoned-logout' }); + + expect(useAuthStore.getState().isAuthenticated).toBeNull(); + await vi.waitFor(() => expect(useAuthStore.getState()).toMatchObject({ userId: 'u-1', isAuthenticated: true })); + expect(api.get).toHaveBeenCalledWith('/auth/me', { broadcastUnauthorized: false }); + }); + it('remoteUnauthorized_clearsThenReprobesSoAStillValidCookieIsNotLoggedOut', async () => { useAuthStore.getState().acceptAuthenticatedIdentity({ userId: 'u-1', diff --git a/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx b/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx index 787725d8..719c50c2 100644 --- a/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx +++ b/src/nodepilot-ui/src/components/designer/EditorOverlays.tsx @@ -177,8 +177,10 @@ export function EditorOverlays({ nodes={nodes} edges={edges} workflowId={id} - setNodes={setNodes} - onClose={() => { setScriptEditNodeId(null); commitHistory('Edit script'); }} + onSave={(nodeId, script) => { + if (canWrite) handleQuickEditSave(nodeId, { script }); + }} + onClose={() => setScriptEditNodeId(null)} /> )} @@ -386,13 +388,13 @@ function findNodes(nodes: Node[], input: string): Node[] { * including the streaming AI generation. */ function ScriptDoubleClickEditor({ - nodeId, nodes, edges, workflowId, setNodes, onClose, + nodeId, nodes, edges, workflowId, onSave, onClose, }: Readonly<{ nodeId: string; nodes: Node[]; edges: Edge[]; workflowId: string | undefined; - setNodes: (updater: (nds: Node[]) => Node[]) => void; + onSave: (nodeId: string, script: string) => void; onClose: () => void; }>) { const node = nodes.find((n) => n.id === nodeId); @@ -410,21 +412,18 @@ function ScriptDoubleClickEditor({ const upstreamRefs = upstreamVars.map((v) => ({ expression: v.expression, label: v.label })); const outputVariableName = (data.outputVariable as string) || undefined; - // Live (possibly unsaved) config so the step-test reflects the editor, not the last-saved DB - // state. + // Capture the target and configuration together, including the uncommitted script buffer. const runStepTest = async (currentScript: string): Promise => - api.post(`/workflows/${workflowId}/steps/${node.id}/test`, { configOverride: { ...config, script: currentScript } }); + api.post(`/workflows/${workflowId}/steps/${node.id}/test`, { + stepOverride: { ...data, config: { ...config, script: currentScript } }, + }); return ( { - setNodes((nds) => nds.map((n) => { - if (n.id !== nodeId) return n; - const cfg = (n.data as Record).config as Record ?? {}; - return { ...n, data: { ...n.data, config: { ...cfg, script: v } } }; - })); + if (v !== (config.script ?? '')) onSave(nodeId, v); }} onClose={onClose} availableVars={psVars} diff --git a/src/nodepilot-ui/src/components/designer/PropertiesPanel.tsx b/src/nodepilot-ui/src/components/designer/PropertiesPanel.tsx index e0b08dba..c7d84fd2 100644 --- a/src/nodepilot-ui/src/components/designer/PropertiesPanel.tsx +++ b/src/nodepilot-ui/src/components/designer/PropertiesPanel.tsx @@ -294,6 +294,7 @@ function PropertiesPanelImpl({ onOpenWorkflowPicker, workflowId, stepId: node.id, + liveStep: data, outputVariableName: outputVariable || undefined, lastStepsByStepId, isLocalTarget: !(data.targetMachineId), @@ -336,7 +337,7 @@ function PropertiesPanelImpl({ diff --git a/src/nodepilot-ui/src/components/designer/overlays/CommandPalette.tsx b/src/nodepilot-ui/src/components/designer/overlays/CommandPalette.tsx index b203f8e4..00532379 100644 --- a/src/nodepilot-ui/src/components/designer/overlays/CommandPalette.tsx +++ b/src/nodepilot-ui/src/components/designer/overlays/CommandPalette.tsx @@ -91,7 +91,7 @@ function fuzzyScore(query: string, target: string): number { export function CommandPalette({ commands, onClose }: Readonly) { const { t } = useTranslation('editor'); const [query, setQuery] = useState(''); - const [activeIdx, setActiveIdx] = useState(0); + const [activeCommandId, setActiveCommandId] = useState(null); const inputRef = useRef(null); useEffect(() => { inputRef.current?.focus(); }, []); @@ -104,21 +104,12 @@ export function CommandPalette({ commands, onClose }: Readonly) { return scored.map((x) => x.c); }, [commands, query]); - useEffect(() => { setActiveIdx((i) => Math.min(i, Math.max(0, ranked.length - 1))); }, [ranked.length]); - const invoke = (cmd: PaletteCommand) => { if (cmd.disabled) return; cmd.run(); onClose(); }; - const onKeyDown = (e: React.KeyboardEvent) => { - if (e.key === 'ArrowDown') { e.preventDefault(); setActiveIdx((i) => Math.min(ranked.length - 1, i + 1)); } - else if (e.key === 'ArrowUp') { e.preventDefault(); setActiveIdx((i) => Math.max(0, i - 1)); } - else if (e.key === 'Enter' && ranked[activeIdx]) { e.preventDefault(); invoke(ranked[activeIdx]); } - else if (e.key === 'Escape') { e.preventDefault(); onClose(); } - }; - // Group results by their `group` field, preserving rank order within each bucket. const grouped = useMemo(() => { const map = new Map(); @@ -130,8 +121,24 @@ export function CommandPalette({ commands, onClose }: Readonly) { return [...map.entries()]; }, [ranked]); - // Compute a flat-to-absolute index mapping for highlight and click handlers. - let cursor = 0; + const visibleCommands = grouped.flatMap(([, items]) => items); + const activeCommand = visibleCommands.find((cmd) => cmd.id === activeCommandId) ?? visibleCommands[0]; + + const onKeyDown = (e: React.KeyboardEvent) => { + if (e.key === 'ArrowDown' || e.key === 'ArrowUp') { + e.preventDefault(); + const activeIdx = visibleCommands.findIndex((cmd) => cmd.id === activeCommand?.id); + const direction = e.key === 'ArrowDown' ? 1 : -1; + const nextIdx = Math.max(0, Math.min(visibleCommands.length - 1, activeIdx + direction)); + setActiveCommandId(visibleCommands[nextIdx]?.id ?? null); + } else if (e.key === 'Enter' && activeCommand) { + e.preventDefault(); + invoke(activeCommand); + } else if (e.key === 'Escape') { + e.preventDefault(); + onClose(); + } + }; const totalCommands = commands.length; const visibleCount = ranked.length; @@ -152,7 +159,7 @@ export function CommandPalette({ commands, onClose }: Readonly) { ref={inputRef} type="text" value={query} - onChange={(e) => setQuery(e.target.value)} + onChange={(e) => { setQuery(e.target.value); setActiveCommandId(null); }} onKeyDown={onKeyDown} placeholder={t('commandPalette.placeholder')} className="flex-1 bg-transparent border-none outline-none text-[15px] font-label text-on-surface placeholder:text-outline/70" @@ -189,14 +196,13 @@ export function CommandPalette({ commands, onClose }: Readonly) { · {items.length} {items.map((cmd) => { - const idx = cursor++; - const isActive = idx === activeIdx; + const isActive = cmd.id === activeCommand?.id; return (