docs(plans): remediation plan for the Flow Debugger arch-review findings F1–F13
Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29
This commit is contained in:
@@ -0,0 +1,621 @@
|
||||
# Flow Debugger Arch-Review Remediation Implementation Plan
|
||||
|
||||
> **For Claude:** REQUIRED SUB-SKILL: Use superpowers-extended-cc:executing-plans to implement this plan task-by-task.
|
||||
|
||||
**Goal:** Resolve every finding F1–F13 of `docs/plans/2026-09-06-flow-debugger-arch-review.md` so the Flow Debugger's stated security and operator-facing properties hold, and the design docs agree with the code.
|
||||
|
||||
**Architecture:** No redesign. The inline trace copy is redacted and byte-capped at the collector boundary (site and central), the method-entry Live run gets the same site-scope gate the value-change entry has, Replay opens to `OperationalAudit` roles while Live stays Deployer-only, and client waits derive from the server's bound. The Low findings are local fixes in the site runtime, the audit repository, the graph builder and the SVG renderer, plus a documentation pass that records the switches the implementation made.
|
||||
|
||||
**Tech Stack:** .NET 10 / C# 13, Akka.NET, Blazor Server (bUnit tests), System.CommandLine (CLI), xUnit. Build with `dotnet build ZB.MOM.WW.ScadaBridge.slnx`; test one project with `dotnet test tests/<Project>/<Project>.csproj`.
|
||||
|
||||
**Branch:** `fix/flow-debugger-arch-review` (from `main` @ `0ccbe140`). Commit trailer on every commit:
|
||||
`Claude-Session: https://claude.ai/code/session_01NcMG6xXdsJbmD2wYAgkK29`
|
||||
|
||||
**Execution notes for implementers**
|
||||
- Work ONLY in the files listed under `Files:`. If you need another file, stop and report it as a plan defect.
|
||||
- TDD: write the failing test first, run it, implement, run again. Run the whole test project you touched before reporting.
|
||||
- Do not edit `CLAUDE.md`, the arch-review doc, or the design docs unless your task lists them (Task 13 owns docs).
|
||||
- Keep every contract change additive (trailing optional parameters, new proto field numbers never reused).
|
||||
|
||||
---
|
||||
|
||||
## Wave plan (parallelism)
|
||||
|
||||
| Wave | Tasks (mutually non-overlapping files) |
|
||||
|---|---|
|
||||
| 1 | 1 (F1 site), 3 (F2), 4 (F3), 5 (F4), 7 (F6), 10 (F9), 11 (F10) |
|
||||
| 2 | 2 (F1 central; after 1, 3), 8 (F7; after 1), 12 (F11; after 3) |
|
||||
| 3 | 6 (F5; after 2), 9 (F8; after 1, 3, 8, 12), 13 (docs; after everything) |
|
||||
| 4 | 14 (full build, full test run, rig rebuild + `FLOW_ONLY=1 verify-delmia-notifier.sh`) |
|
||||
|
||||
---
|
||||
|
||||
### Task 1: F1 (site) — `FlowTraceCollector` redacts and byte-caps, moves to Commons
|
||||
|
||||
**Classification:** standard
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 3, 4, 5, 7, 10, 11
|
||||
|
||||
**Files:**
|
||||
- Create: `src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowTraceCollector.cs` (moved from SiteRuntime, namespace `ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging`)
|
||||
- Delete: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/FlowTraceCollector.cs`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs:250` (construct with redactor), `:335` region (resolve `IAuditRedactor` from the scope beside `IAuditWriter`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs` (add `using ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging;`)
|
||||
- Move test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/FlowTraceCollectorTests.cs` → `tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging/FlowTraceCollectorTests.cs` (namespace `ZB.MOM.WW.ScadaBridge.Commons.Tests.Debugging`), add the two tests below
|
||||
- Modify test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Scripts/ScriptRuntimeContextTraceEmissionTests.cs` (using only; add one redaction test)
|
||||
|
||||
**Why Commons:** the central `FlowDebugRunner` (Task 2) needs the same collector; Commons already references the `ZB.MOM.WW.Audit` package (`IAuditRedactor`, `AuditEvent`) and hosts `Types/Debugging/`.
|
||||
|
||||
**Step 1: Failing tests** (in the moved `FlowTraceCollectorTests`)
|
||||
|
||||
```csharp
|
||||
private sealed class UpperRedactor : IAuditRedactor
|
||||
{
|
||||
public AuditEvent Apply(AuditEvent evt) => new()
|
||||
{
|
||||
EventId = evt.EventId, OccurredAtUtc = evt.OccurredAtUtc, Action = evt.Action,
|
||||
DetailsJson = evt.DetailsJson?.ToUpperInvariant(),
|
||||
// copy every other property the AuditEvent record carries
|
||||
};
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Add_AppliesTheRedactor_SoTheInlineCopyIsTheFilteredEvent()
|
||||
{
|
||||
var c = new FlowTraceCollector(redactor: new UpperRedactor());
|
||||
var evt = Evt(1); evt.DetailsJson = "{\"secret\":\"abc\"}";
|
||||
c.Add(evt);
|
||||
Assert.Equal("{\"SECRET\":\"ABC\"}", c.Complete()[0].DetailsJson);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void Add_OverByteBudget_Truncates_KeepsEarlierEvents()
|
||||
{
|
||||
var c = new FlowTraceCollector(maxEvents: 500, maxBytes: 100);
|
||||
var big = Evt(1); big.DetailsJson = new string('x', 80);
|
||||
var big2 = Evt(2); big2.DetailsJson = new string('y', 80);
|
||||
c.Add(big); c.Add(big2);
|
||||
Assert.True(c.Truncated);
|
||||
Assert.Single(c.Complete());
|
||||
}
|
||||
```
|
||||
|
||||
**Step 2:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.Commons.Tests --filter FlowTraceCollectorTests` → FAIL (no ctor overload).
|
||||
|
||||
**Step 3: Implementation** — the collector gains:
|
||||
|
||||
```csharp
|
||||
/// <summary>Default inline byte budget per run (sum of redacted DetailsJson lengths).</summary>
|
||||
public const int DefaultMaxBytes = 1_048_576;
|
||||
|
||||
private readonly IAuditRedactor? _redactor;
|
||||
private readonly int _maxBytes;
|
||||
private int _bytes;
|
||||
|
||||
public FlowTraceCollector(int maxEvents = DefaultMaxEvents, int maxBytes = DefaultMaxBytes, IAuditRedactor? redactor = null)
|
||||
{ /* validate both > 0 */ }
|
||||
|
||||
public void Add(AuditEvent evt)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(evt);
|
||||
// Redaction OUTSIDE the lock (pure function, may be non-trivial) — arch-review F1:
|
||||
// the inline copy must be the same filtered record the writer persists.
|
||||
var filtered = _redactor is null ? evt : _redactor.Apply(evt);
|
||||
var size = filtered.DetailsJson?.Length ?? 0;
|
||||
lock (_gate)
|
||||
{
|
||||
if (Completed) return;
|
||||
if (_events.Count >= _max || _bytes + size > _maxBytes) { Truncated = true; return; }
|
||||
_events.Add(filtered);
|
||||
_bytes += size;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Update the class doc: "the inline copy is REDACTED and capped by count AND bytes; the audit rows written through `IAuditWriter` are redacted again by the writer (idempotent)". Keep `Snapshot`, `Count`, `Complete` as they are.
|
||||
|
||||
`ScriptRunLauncher.LaunchScript`: resolve `var redactor = serviceScope.ServiceProvider.GetService<IAuditRedactor>();` beside `auditWriter` (line ~335) and construct the collector AFTER the scope exists: `collector = trace && !replyTo.IsNobody() ? new FlowTraceCollector(redactor: redactor) : null;` — move the `collector` creation from line 250 to just after the scope block (it is only handed to `ScriptRuntimeContext` and `EmitScriptRun`, both later). `IAuditRedactor` is registered by `AddAuditLog` on every site host; null in tests = no redaction, still capped.
|
||||
|
||||
`ScriptRuntimeContext`: only the `using`. `ScriptRuntimeContextTraceEmissionTests`: add one test that builds the context with `new FlowTraceCollector(redactor: new UpperRedactor())`, triggers an `AttributeWrite` emission, and asserts the collected `DetailsJson` is the redacted form while the `IAuditWriter` fake still receives the raw event (the writer redacts itself).
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.Commons.Tests` and `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(site-runtime): Flow Debugger inline trace copy is redacted and byte-capped at the collector (arch-review F1, site half)`
|
||||
|
||||
---
|
||||
|
||||
### Task 2: F1 (central) — `FlowDebugRunner` collects through the same redacting collector
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 8, 12
|
||||
**Blocked by:** Task 1, Task 3
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs:103-146` (`RunMethodAsync`)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs`
|
||||
|
||||
**Step 1: Failing tests**
|
||||
- `RunMethod_SinkEvents_AreRedacted_WhenARedactorIsRegistered`: register an `IAuditRedactor` fake in the test `IServiceProvider` that upper-cases `DetailsJson`; the fake executor pushes one event through `route.TraceSink`; assert the result's event `DetailsJson` is upper-cased.
|
||||
- `RunMethod_OverCap_ReportsTruncated`: fake executor pushes 501 events; assert `result.Truncated == true` and `result.Events.Count == 501` (inbound row + 500).
|
||||
|
||||
**Step 2:** run → FAIL.
|
||||
|
||||
**Step 3: Implementation** — replace the `ConcurrentQueue<AuditEvent>` with
|
||||
|
||||
```csharp
|
||||
var collector = new FlowTraceCollector(redactor: sp.GetService<IAuditRedactor>());
|
||||
var route = sp.GetRequiredService<RouteHelper>().WithTraceSink(collector.Add);
|
||||
...
|
||||
var events = collector.Complete();
|
||||
var ordered = new List<AuditEvent>(events.Count + 1) { inboundRow };
|
||||
ordered.AddRange(events.OrderBy(e => e.OccurredAtUtc).ThenBy(e => e.EventId)); // Task 6 changes this line
|
||||
return new FlowDebugRunResult(root, ..., ordered, Truncated: collector.Truncated, startedAt, finishedAt);
|
||||
```
|
||||
|
||||
`ICentralAuditWriter.WriteAsync` already redacts the stored inbound row; leave `WriteCentralAsync` alone. Note in the method doc that the redactor is `AddAuditLog`'s singleton (`Program.cs:333`) and that a host without one collects raw but still capped.
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests --filter FlowDebugRunnerTests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(management): Flow Debugger Live result is the redacted, capped trace copy (arch-review F1, central half)`
|
||||
|
||||
---
|
||||
|
||||
### Task 3: F2 — site scope on the method-entry Live run (`RouteHelper.WithSiteScope`)
|
||||
|
||||
**Classification:** high-risk (security gate)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 1, 4, 5, 7, 10, 11
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs` (new `RouteSiteScopeException`, `WithSiteScope`, check in `RouteTarget.ResolveSiteAsync:540`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs:509-521` (map the new exception to error code `SITE_SCOPE_DENIED`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs:79-146` (`RunMethodAsync` sets the scope; rethrows as `SiteScopeViolationException` after writing the inbound row)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs`
|
||||
|
||||
**Design.** `IInstanceLocator.GetSiteIdForInstanceAsync` returns the site **identifier** (string, e.g. `site-a`), while `AuthenticatedUser.PermittedSiteIds` holds numeric ids as strings. The runner therefore resolves the user's permitted numeric ids to identifiers through `ISiteRepository.GetSiteByIdAsync` once per run and hands `RouteHelper` an identifier set. Administrators and unscoped users (`PermittedSiteIds.Length == 0`) get no scope (null), exactly like `EnforceSiteScope`. `SiteScopeViolationException` lives in ManagementService, which InboundAPI cannot reference, so InboundAPI throws its own `RouteSiteScopeException : InvalidOperationException`; the executor maps it to `SITE_SCOPE_DENIED` (the script never sees a routed reply — the check runs before any site traffic); the runner converts that code to `SiteScopeViolationException` → `ManagementUnauthorized`, AFTER writing the inbound audit row so the denied attempt is recorded with the operator's name. The HTTP endpoint passes no scope — unchanged.
|
||||
|
||||
**Step 1: Failing tests**
|
||||
|
||||
RouteHelperTests:
|
||||
```csharp
|
||||
[Fact]
|
||||
public async Task Call_OutsideSiteScope_ThrowsRouteSiteScopeException_BeforeRouting()
|
||||
{
|
||||
// locator resolves "site-b"; scope = {"site-a"}; router must never be called
|
||||
var helper = new RouteHelper(locator, router).WithSiteScope(new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "site-a" });
|
||||
await Assert.ThrowsAsync<RouteSiteScopeException>(() => helper.To("Inst1").Call("S"));
|
||||
Assert.Equal(0, router.CallCount);
|
||||
}
|
||||
[Fact] public async Task Call_InsideSiteScope_Routes() { /* scope {"site-b"}, locator "site-b" → routes */ }
|
||||
[Fact] public async Task Call_NullScope_IsUnscoped() { /* WithSiteScope(null) routes */ }
|
||||
```
|
||||
|
||||
FlowDebugRunnerTests:
|
||||
```csharp
|
||||
[Fact]
|
||||
public async Task RunMethod_ScopedDeployer_RouteToOtherSite_IsUnauthorized_AndInboundRowIsWritten()
|
||||
{
|
||||
// user PermittedSiteIds = ["1"]; ISiteRepository fake: id 1 → "site-a"; fake executor returns
|
||||
// new InboundScriptResult(false, null, "Access denied ...", "SITE_SCOPE_DENIED") when route scope excludes "site-b"
|
||||
await Assert.ThrowsAsync<SiteScopeViolationException>(() => runner.RunMethodAsync(sp, cmd, scopedUser, ct));
|
||||
Assert.Single(centralWriter.Written, e => AuditDetailsCodec.Deserialize(e.DetailsJson).Kind == nameof(AuditKind.InboundRequest));
|
||||
}
|
||||
[Fact] public async Task RunMethod_Administrator_IsNeverScoped() { /* Roles contains Administrator, PermittedSiteIds ["1"], route to site-b succeeds */ }
|
||||
```
|
||||
|
||||
**Step 2:** run both projects' filters → FAIL.
|
||||
|
||||
**Step 3: Implementation**
|
||||
|
||||
RouteHelper.cs — beside `SiteUnreachableException`:
|
||||
```csharp
|
||||
/// <summary>Thrown by a routed call whose target site is outside the caller's site scope (arch-review F2). Raised BEFORE any site traffic.</summary>
|
||||
public sealed class RouteSiteScopeException : InvalidOperationException
|
||||
{
|
||||
public RouteSiteScopeException(string message) : base(message) { }
|
||||
}
|
||||
```
|
||||
Add `IReadOnlySet<string>? permittedSiteIdentifiers` as a trailing field/ctor parameter on both `RouteHelper` and `RouteTarget`, thread it through every `With*` copy and `To(...)`, add:
|
||||
```csharp
|
||||
/// <summary>Restricts routed calls to the given site identifiers (case-insensitive); null = unscoped. Flow Debugger Live runs set it from the operator's site-scoped Deployer role.</summary>
|
||||
public RouteHelper WithSiteScope(IReadOnlySet<string>? permittedSiteIdentifiers) => new(..., permittedSiteIdentifiers);
|
||||
```
|
||||
In `RouteTarget.ResolveSiteAsync`, after the null check:
|
||||
```csharp
|
||||
if (_permittedSiteIdentifiers is not null && !_permittedSiteIdentifiers.Contains(siteId))
|
||||
throw new RouteSiteScopeException(
|
||||
$"Access denied: instance '{_instanceCode}' is at site '{siteId}', outside your site scope.");
|
||||
```
|
||||
(`ResolveSiteAsync` is the single choke point — all four routed operations at lines 256/395/460/510 call it.)
|
||||
|
||||
InboundScriptExecutor.cs — add before the `SiteUnreachableException` catch (line ~509):
|
||||
```csharp
|
||||
catch (RouteSiteScopeException ex)
|
||||
{
|
||||
_logger.LogWarning("API method {Method} refused a routed call outside the caller's site scope: {Message}", method.Name, ex.Message);
|
||||
return new InboundScriptResult(false, null, ex.Message, "SITE_SCOPE_DENIED");
|
||||
}
|
||||
```
|
||||
Check whether the script body wraps exceptions in `TargetInvocationException`/`AggregateException` on this path — mirror whatever `SiteUnreachableException` needs (`ex.GetBaseException()` unwrapping) so the mapping is symmetrical.
|
||||
|
||||
FlowDebugRunner.RunMethodAsync — after `EnsureActiveNode`:
|
||||
```csharp
|
||||
var scope = await ResolvePermittedSiteIdentifiersAsync(sp, user, ct).ConfigureAwait(false);
|
||||
var route = sp.GetRequiredService<RouteHelper>().WithTraceSink(...).WithSiteScope(scope);
|
||||
...
|
||||
await WriteCentralAsync(sp, inboundRow, ct).ConfigureAwait(false);
|
||||
if (result.ErrorCode == "SITE_SCOPE_DENIED")
|
||||
throw new SiteScopeViolationException(result.ErrorMessage ?? "Access denied: routed call outside your site scope.");
|
||||
```
|
||||
```csharp
|
||||
/// <summary>Arch-review F2: the user's PermittedSiteIds (numeric) → site identifiers; null = unscoped (empty list or Administrator).</summary>
|
||||
private static async Task<IReadOnlySet<string>?> ResolvePermittedSiteIdentifiersAsync(IServiceProvider sp, AuthenticatedUser user, CancellationToken ct)
|
||||
{
|
||||
if (user.PermittedSiteIds.Length == 0) return null;
|
||||
if (user.Roles.Contains(Roles.Administrator, StringComparer.OrdinalIgnoreCase)) return null;
|
||||
var sites = sp.GetRequiredService<ISiteRepository>();
|
||||
var set = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
|
||||
foreach (var raw in user.PermittedSiteIds)
|
||||
{
|
||||
if (!int.TryParse(raw, out var id)) continue;
|
||||
var site = await sites.GetSiteByIdAsync(id, ct).ConfigureAwait(false);
|
||||
if (site is not null) set.Add(site.SiteIdentifier);
|
||||
}
|
||||
return set; // possibly empty: a scoped user whose sites no longer exist routes nowhere
|
||||
}
|
||||
```
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` and `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(inbound-api): Flow Debugger method-entry Live run enforces the operator's site scope at route time (arch-review F2)`
|
||||
|
||||
---
|
||||
|
||||
### Task 4: F3 — Replay opens to OperationalAudit; Live stays Deployer-only
|
||||
|
||||
**Classification:** standard (authorization)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 1, 3, 5, 7, 10, 11
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Security/AuthorizationPolicies.cs` (new policy `RequireFlowDebugger` + `FlowDebuggerAccessRoles`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementActor.cs:192-310` (`FlowDebugReplayCommand` → new `FlowDebuggerReplayRoles`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor` (page attribute, gate the Live controls) and `FlowDebugger.razor.cs` (`CanRun` also requires the Deployer role; `_canLive` from `AuthenticationStateProvider`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Deployment/FlowDebugger.razor:238` (Retry/Discard delegates only wired when `_canLive`)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/RequiredRoleMatrixTests.cs:144`, `tests/ZB.MOM.WW.ScadaBridge.Security.Tests/` (find the existing policy test class and add a row), `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Deployment/FlowDebuggerPageTests.cs`
|
||||
|
||||
**Step 1: Failing tests**
|
||||
- `RequiredRoleMatrixTests`: `["FlowDebugReplay"] = [Roles.Deployer, Roles.Administrator, Roles.Viewer]` (the two run commands stay `[Roles.Deployer]`).
|
||||
- Security policy test: a Viewer principal satisfies `RequireFlowDebugger`; a Designer does not; a Deployer does.
|
||||
- `FlowDebuggerPageTests`: rendering the page with a Viewer-only principal shows the Replay controls (`data-test="flow-entry-replay"` or the execution-id input — use whatever the page's replay `data-test` markers are) and NOT `data-test="flow-run"`; with Deployer it shows both. Follow the existing test's auth setup (bUnit `TestAuthorizationContext`).
|
||||
|
||||
**Step 2:** run → FAIL.
|
||||
|
||||
**Step 3: Implementation**
|
||||
|
||||
AuthorizationPolicies.cs:
|
||||
```csharp
|
||||
/// <summary>Flow Debugger page access (arch-review F3 / design D6): Replay is an audit read, so
|
||||
/// OperationalAudit roles may open the page; Live runs are additionally gated by
|
||||
/// <see cref="RequireDeployment"/> on the Run controls and by the ManagementActor.</summary>
|
||||
public const string RequireFlowDebugger = "RequireFlowDebugger";
|
||||
public static readonly string[] FlowDebuggerAccessRoles = { Roles.Deployer, Roles.Administrator, Roles.Viewer };
|
||||
// in AddScadaBridgeAuthorization:
|
||||
options.AddPolicy(RequireFlowDebugger, policy => policy.RequireClaim(JwtTokenService.RoleClaimType, FlowDebuggerAccessRoles));
|
||||
```
|
||||
ManagementActor.cs:
|
||||
```csharp
|
||||
private static readonly string[] FlowDebuggerReplayRoles = AuthorizationPolicies.FlowDebuggerAccessRoles;
|
||||
...
|
||||
FlowDebugRunMethodCommand or FlowDebugRunValueChangeCommand => DeployerOnly,
|
||||
// Replay reads the audit chain — OperationalAudit roles + Deployer (design D6, arch-review F3).
|
||||
FlowDebugReplayCommand => FlowDebuggerReplayRoles,
|
||||
```
|
||||
Page: `@attribute [Authorize(Policy = AuthorizationPolicies.RequireFlowDebugger)]`; in `OnInitializedAsync` compute `_canLive = (await Auth.GetAuthenticationStateAsync()).User.HasClaim(JwtTokenService.RoleClaimType, Roles.Deployer)` (inject `AuthenticationStateProvider` if the page does not already); hide/disable the Live entry buttons, the Run button and the value input when `!_canLive` (default the mode to Replay for such a user; if the page opens with `?executionId=` it already goes to Replay); pass `OnRetry`/`OnDiscard` to `FlowStepDetail` only when `_canLive`. Keep the NavMenu entry where it is (inside the Deployment section) — auditors arrive through the Audit Log / Execution Chain drill-ins, which is the point of the finding.
|
||||
|
||||
**Step 4:** `dotnet test` on ManagementService.Tests, Security.Tests, CentralUI.Tests → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(central-ui): Flow Debugger Replay opens to OperationalAudit roles; Live run and Retry/Discard stay Deployer-only (arch-review F3)`
|
||||
|
||||
---
|
||||
|
||||
### Task 5: F4 — client waits derive from the server's bound
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 1, 3, 4, 7, 10, 11
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Services/FlowDebugService.cs:26-40, 178-185`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/FlowDebugCommands.cs:17-18, 37-97`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CLI/README.md` (the `flow-debug run-method` / `run-value-change` option tables)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Services/FlowDebugServiceTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.CLI.Tests/FlowDebugCommandsTests.cs`
|
||||
|
||||
**Step 1: Failing tests**
|
||||
- `FlowDebugServiceTests`: `RunAskTimeoutFor(ApiMethod)` (make it a `public static TimeSpan RunAskTimeoutFor(int timeoutSeconds)`) returns `timeoutSeconds + RunGrace(10 s) + ClientSlack(15 s)`, never below `MinRunAskTimeout` (30 s); `ValueChangeAskTimeoutFor(int waitSeconds)` returns `waitSeconds + ClientSlack`. Assert `RunAskTimeoutFor(180) == 205 s`, `RunAskTimeoutFor(1) == 30 s`, `ValueChangeAskTimeoutFor(45) == 60 s`.
|
||||
- `FlowDebugCommandsTests`: `run-method` accepts `--timeout-seconds` (default 120); `run-value-change` has no such option and derives `waitSeconds + 25`; `--timeout-seconds 0` is rejected with `INVALID_ARGUMENT`.
|
||||
|
||||
**Step 2:** run → FAIL.
|
||||
|
||||
**Step 3: Implementation**
|
||||
|
||||
FlowDebugService: CentralUI references ManagementService, so use `FlowDebugRunner.RunGrace` directly.
|
||||
```csharp
|
||||
/// <summary>Slack the client adds over the server's own bound (Ask hop + serialization).</summary>
|
||||
public static readonly TimeSpan ClientSlack = TimeSpan.FromSeconds(15);
|
||||
public static readonly TimeSpan MinRunAskTimeout = TimeSpan.FromSeconds(30);
|
||||
public static TimeSpan RunAskTimeoutFor(int timeoutSeconds) { var t = TimeSpan.FromSeconds(Math.Max(1, timeoutSeconds)) + FlowDebugRunner.RunGrace + ClientSlack; return t < MinRunAskTimeout ? MinRunAskTimeout : t; }
|
||||
public static TimeSpan ValueChangeAskTimeoutFor(int waitSeconds) => TimeSpan.FromSeconds(Math.Max(1, waitSeconds)) + ClientSlack;
|
||||
```
|
||||
`RunMethodAsync` reads the method (`_inboundApi.GetApiMethodByIdAsync`) and uses `RunAskTimeoutFor(method?.TimeoutSeconds ?? 30)`; `RunValueChangeAsync` uses `ValueChangeAskTimeoutFor(45)` (the command record default — read it from `new FlowDebugRunValueChangeCommand(...).WaitSeconds` so the two never drift). Delete the fixed `RunAskTimeout` and fix the class doc ("both fit inside two minutes" is gone).
|
||||
|
||||
CLI: `run-method` gains `var timeoutOption = new Option<int>("--timeout-seconds") { Description = "Client-side wait for the run; set to the method's TimeoutSeconds + 25 for long methods", DefaultValueFactory = _ => 120 };` validated `> 0`; `run-value-change` passes `TimeSpan.FromSeconds(waitSeconds + 25)`. Remove `RunTimeout`. README: add the option row and the derivation sentence.
|
||||
|
||||
**Step 4:** both test projects → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(central-ui,cli): Flow Debugger client waits derive from the method timeout / wait window instead of a fixed 2 minutes (arch-review F4)`
|
||||
|
||||
---
|
||||
|
||||
### Task 6: F5 — emission order wins over cross-node timestamps
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 9, 13
|
||||
**Blocked by:** Task 2
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Commons/Types/Debugging/FlowGraphBuilder.cs:38-50, 95-97`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ManagementService/FlowDebug/FlowDebugRunner.cs` (`RunMethodAsync` ordered list: keep collector order, no `OrderBy`)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.Commons.Tests/Debugging/FlowGraphBuilderTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/FlowDebugRunnerTests.cs:89` (`..._Ordered_...` test: assert emission order is preserved)
|
||||
|
||||
**Step 1: Failing test** (`FlowGraphBuilderTests`): `SiteClockAhead_SiteStepsStillFollowTheirRouteCall_AndPrecedeResponse` — inbound (central t0), RouteCall (central t0+50 ms, `ChildExecutionId` = child), site ScriptRun (child, t0 + 2 s — clock ahead), site AttributeWrite (child, t0 + 2.01 s), given in that emission order; assert `Steps` order is inbound, RouteCall, ScriptRun, AttributeWrite, Response and the Response's `OccurredAtUtc` is derived from the inbound row as today.
|
||||
|
||||
**Step 2:** run → FAIL (site steps sort after the Response).
|
||||
|
||||
**Step 3: Implementation** — sort per execution, order executions by first appearance:
|
||||
```csharp
|
||||
var seq = new Dictionary<Guid, int>(); // EventId → input index
|
||||
for (var i = 0; i < events.Count; i++) seq.TryAdd(events[i].EventId, i);
|
||||
int Seq(FlowStep s) => seq.TryGetValue(s.EventId, out var i) ? i : int.MaxValue;
|
||||
var firstSeqByExecution = folded.GroupBy(s => s.ExecutionId ?? Guid.Empty).ToDictionary(g => g.Key, g => g.Min(Seq));
|
||||
var steps = folded
|
||||
.OrderBy(s => firstSeqByExecution[s.ExecutionId ?? Guid.Empty]) // executions in emission order
|
||||
.ThenBy(s => s.OccurredAtUtc) // one clock inside an execution
|
||||
.ThenBy(s => KindRank(s.Kind))
|
||||
.ThenBy(Seq)
|
||||
.ToList();
|
||||
```
|
||||
Replay input arrives timestamp-sorted from the repository, so its order is unchanged. The spawner search (line 96): prefer the parent step whose `Seq` is the greatest below the child's first step's `Seq` (non-async, non-Response), falling back to the timestamp rule, then `parentChain[0]`. Runner: `ordered.AddRange(collector.Complete())` — no re-sort (the value-change path keeps its `OrderBy`, all rows there share the site clock).
|
||||
|
||||
Doc comment on `Build`: "Order = emission order between executions, timestamp within one (arch-review F5: central and site clocks differ)".
|
||||
|
||||
**Step 4:** Commons.Tests + ManagementService.Tests → PASS (fix any existing ordering test that pinned the old global sort; the Delmia-shape test passes unchanged because its input is already in timestamp order).
|
||||
|
||||
**Step 5: Commit** `fix(commons): Flow graph orders executions by emission sequence, timestamps only within one clock (arch-review F5)`
|
||||
|
||||
---
|
||||
|
||||
### Task 7: F6 — trace arm cleared when the DCL write fails
|
||||
|
||||
**Classification:** standard (actor)
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 1, 3, 4, 5, 10, 11
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/InstanceActor.cs:183, 339 (Receive registration), 757-779`
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/InstanceActorTraceArmTests.cs`
|
||||
|
||||
**Step 1: Failing test** — `TracedDataWrite_ThatFails_DisarmsTheAttribute_SoTheNextEchoIsUnstamped`: DCL fake answers `WriteTagResponse(Success:false, "rejected")`; then a `TagValueUpdate` on the attribute; assert the published `AttributeValueChanged` has `SourceExecutionId == null && Trace == false`. Mirror the existing `TracedDataWrite_ThenEcho_...` fixture.
|
||||
|
||||
**Step 2:** run → FAIL.
|
||||
|
||||
**Step 3: Implementation**
|
||||
```csharp
|
||||
/// <summary>Self-message: a traced data write failed, drop its trace arm if it is still the armed one (arch-review F6).</summary>
|
||||
private sealed record DisarmTrace(string AttributeName, Guid SourceExecutionId);
|
||||
// ctor: Receive<DisarmTrace>(m => { if (_traceArms.TryGetValue(m.AttributeName, out var a) && a.SourceExecutionId == m.SourceExecutionId) _traceArms.Remove(m.AttributeName); });
|
||||
```
|
||||
In `HandleSetDataAttribute` capture `var self = Self; Guid? armed = command.Trace ? command.SourceExecutionId : null;` and in the `ContinueWith` body, when `!t.IsCompletedSuccessfully || !t.Result.Success`, `if (armed is { } a) self.Tell(new DisarmTrace(attributeName, a));` before returning the response. Update the arm comment: "armed at issue, disarmed on a failed/timed-out write; a successful write's echo consumes it".
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests --filter InstanceActorTraceArmTests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(site-runtime): trace arm is cleared when the traced DCL write fails (arch-review F6)`
|
||||
|
||||
---
|
||||
|
||||
### Task 8: F7 — a shed or rate-limited traced run leaves a Failed `ScriptRun` row
|
||||
|
||||
**Classification:** standard (actor + launcher)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 2, 12
|
||||
**Blocked by:** Task 1
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs` (new `internal static void EmitSuppressedRun(...)`, reusing `EmitScriptRun`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Actors/ScriptActor.cs:519-541 (TrySpawnExecution), 566-580 (SpawnExecution shed branch), 653 (ShedRun signature gains parentExecutionId/trace/triggerExtra)`
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests/Actors/ScriptRunShedTests.cs`
|
||||
|
||||
**Step 1: Failing tests**
|
||||
- `TracedRun_ShedAtTheCap_EmitsAFailedScriptRunRow_ParentedToTheWriter`: fill the cap with untraced runs, then a traced `AttributeValueChanged` (`Trace:true, SourceExecutionId: parent`); assert the `IAuditWriter` fake received one `ScriptTrace/ScriptRun` `Failed` row with `ParentExecutionId == parent` and `ErrorMessage` starting `shed: MaxConcurrentRunsPerScript`.
|
||||
- `TracedRun_SuppressedByMinTimeBetweenRuns_EmitsAFailedScriptRunRow`: `MinTimeBetweenRuns` = 10 min, two traced changes back to back; assert the second yields a `Failed` row with `suppressed: MinTimeBetweenRuns`.
|
||||
|
||||
**Step 2:** run → FAIL.
|
||||
|
||||
**Step 3: Implementation** — in the launcher:
|
||||
```csharp
|
||||
/// <summary>Arch-review F7: a traced run that never launched (shed / rate-limited) still leaves a
|
||||
/// Failed ScriptRun row so the Flow Debugger explains the gap instead of NO_TRIGGER_WITHIN_WINDOW.
|
||||
/// No collector (there is no reply to ride on); writer + identity from a fresh scope.</summary>
|
||||
internal static void EmitSuppressedRun(IServiceProvider? serviceProvider, ILogger logger, string scriptName, string instanceName,
|
||||
Guid? parentExecutionId, string reason, ScriptTraceExtra? triggerExtra)
|
||||
{
|
||||
if (serviceProvider is null) return;
|
||||
using var scope = serviceProvider.CreateScope();
|
||||
var writer = scope.ServiceProvider.GetService<IAuditWriter>();
|
||||
if (writer is null) return;
|
||||
var siteId = scope.ServiceProvider.GetService<ISiteIdentityProvider>()?.SiteId ?? string.Empty;
|
||||
var sourceNode = scope.ServiceProvider.GetService<INodeIdentityProvider>()?.NodeName;
|
||||
EmitScriptRun(trace: true, collector: null, writer, logger, scriptName, instanceName, siteId, sourceNode,
|
||||
executionId: Guid.NewGuid(), parentExecutionId, DateTime.UtcNow, elapsedMs: 0,
|
||||
parameters: null, returnValue: null, error: reason, triggerExtra);
|
||||
}
|
||||
```
|
||||
ScriptActor: in the `MinTimeBetweenRuns` branch `if (trace) ScriptRunLauncher.EmitSuppressedRun(_serviceProvider, _logger, _scriptName, _instanceName, parentExecutionId, "suppressed: MinTimeBetweenRuns", triggerExtra);` and in the shed branch the same with `"shed: MaxConcurrentRunsPerScript"` (pass `parentExecutionId, trace, triggerExtra` into `ShedRun`).
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(site-runtime): shed or rate-limited traced runs emit a Failed ScriptRun row (arch-review F7)`
|
||||
|
||||
---
|
||||
|
||||
### Task 9: F8 — fault continuations on `TaskScheduler.Default`, central emitters log
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Task 6, 13
|
||||
**Blocked by:** Task 1, 3, 8, 12
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRuntimeContext.cs:446-448`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.SiteRuntime/Scripts/ScriptRunLauncher.cs:544-548`
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs:339` (+ optional `ILogger?` threaded through `RouteHelper`/`RouteTarget` ctors and `With*` copies; DI ctor takes `ILogger<RouteHelper>? logger = null`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundDatabaseHelper.cs:271` (+ optional `ILogger? logger = null` ctor parameter; `InboundScriptExecutor.cs:391-400` passes `_logger`)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs:391-400` (pass the logger)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundDatabaseHelperAuditTests.cs` (one test: a faulting writer produces a Warning log entry and the call still succeeds)
|
||||
|
||||
**Step 3:** every site is
|
||||
```csharp
|
||||
_ = t.ContinueWith(
|
||||
x => logger?.LogWarning(x.Exception, "... write failed for {X}", ...),
|
||||
CancellationToken.None,
|
||||
TaskContinuationOptions.OnlyOnFaulted | TaskContinuationOptions.ExecuteSynchronously,
|
||||
TaskScheduler.Default);
|
||||
```
|
||||
(the two site emitters keep their existing messages, just add the token/options/scheduler arguments; the two central ones replace `static _ => { }` with a Warning log).
|
||||
|
||||
**Step 4:** SiteRuntime.Tests + InboundAPI.Tests → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(audit): trace/outbound fault continuations run on the default scheduler and central emitters log (arch-review F8)`
|
||||
|
||||
---
|
||||
|
||||
### Task 10: F9 — one execution-chain walker in `AuditLogRepository`
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 1, 3, 4, 5, 7, 11
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase/Repositories/AuditLogRepository.cs:922-1241`
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests/Repositories/AuditLogRepositoryTraceEventsTests.cs` (+ the existing execution-tree tests in that folder — find them with `grep -rl GetExecutionTreeAsync tests/`)
|
||||
|
||||
**Step 3: Implementation** — pure refactor, behaviour pinned by the existing tests (they need the rig's MSSQL; `SkippableFact` skips without it — run with the container up):
|
||||
```csharp
|
||||
private sealed record ChainAnchor(Guid RootExecutionId, DateTime? RootFirstOccurred);
|
||||
|
||||
/// <summary>Phase 1 + window of the execution-chain walk, shared by GetExecutionTreeAsync and GetTraceEventsAsync (arch-review F9).</summary>
|
||||
private static async Task<ChainAnchor> ResolveChainAnchorAsync(DbConnection conn, Guid executionId, CancellationToken ct) { /* the up-walk loop + MIN(OccurredAtUtc) */ }
|
||||
|
||||
/// <summary>The Edges/Chain CTE prefix (identical in both walkers); callers append their projection.</summary>
|
||||
private static string ChainCtePrefix(string edgesWindowPredicate) => $@"WITH Edges AS (...), Chain AS (...)";
|
||||
|
||||
private static void AddChainParameters(DbCommand cmd, ChainAnchor anchor) { /* @root, @winStart/@winEnd when windowed */ }
|
||||
```
|
||||
`GetExecutionTreeAsync` keeps its `ChainIds` + aggregate projection appended to the prefix; `GetTraceEventsAsync` appends the id projection. Both keep `OPTION (MAXRECURSION …)`. Delete the duplicated blocks. No SQL text change other than the split.
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.ConfigurationDatabase.Tests --filter "AuditLogRepository"` → PASS, none skipped (MSSQL up).
|
||||
|
||||
**Step 5: Commit** `refactor(config-db): GetTraceEventsAsync and GetExecutionTreeAsync share one chain walker (arch-review F9)`
|
||||
|
||||
---
|
||||
|
||||
### Task 11: F10 — prediction overlay matches by (Kind, Title) before Kind
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~3 min
|
||||
**Parallelizable with:** Task 1, 3, 4, 5, 7, 10
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Shared/FlowGraphView.razor.cs:411-423`
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests/Shared/FlowGraphViewTests.cs`
|
||||
|
||||
**Step 1: Failing test** — `Overlay_TwoPredictedRouteCalls_RealSecondCallMatchesByTitle_FirstIsTheGhost`: predicted nodes `RouteCall "A.X"`, `RouteCall "B.Y"`; real graph has one RouteCall step titled `B.Y`; assert the ghost key set contains the predicted `A.X` node and the real step maps to `B.Y`.
|
||||
|
||||
**Step 3:**
|
||||
```csharp
|
||||
var m = unmatched.FirstOrDefault(p => p.Kind == s.Kind && string.Equals(p.Title, s.Title, StringComparison.OrdinalIgnoreCase))
|
||||
?? unmatched.FirstOrDefault(p => p.Kind == s.Kind);
|
||||
```
|
||||
(check the predicted node's title property name — `PredictedNode.Title` or `Label` — and the real step's `FlowStep.Title`).
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.CentralUI.Tests --filter FlowGraphViewTests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(central-ui): Flow Debugger prediction overlay matches by kind and title (arch-review F10)`
|
||||
|
||||
---
|
||||
|
||||
### Task 12: F11 — `RouteCall` row honours the method's `SkipBodyCapture`
|
||||
|
||||
**Classification:** small
|
||||
**Estimated implement time:** ~4 min
|
||||
**Parallelizable with:** Task 2, 8
|
||||
**Blocked by:** Task 3
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/RouteHelper.cs` (`WithBodyCapture(bool)`, field threaded through `RouteHelper`/`RouteTarget`; `EmitRouteCall` passes null summaries when off)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.InboundAPI/InboundScriptExecutor.cs:415-420` (resolve `IOptionsMonitor<AuditLogOptions>` from the scope; `PerTargetOverrides.TryGetValue(method.Name, …)?.SkipBodyCapture` exactly as `AuditWriteMiddleware.cs:183-187`; `.WithBodyCapture(!skip)`)
|
||||
- Test: `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/RouteHelperTests.cs`, `tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests/InboundScriptExecutorTests.cs`
|
||||
|
||||
**Step 1: Failing tests** — `RouteCallRow_WithBodyCaptureOff_HasNoRequestOrResponseSummary` (RouteHelperTests, using the trace sink to read the row) and `Executor_MethodWithSkipBodyCapture_RoutesWithBodyCaptureOff` (executor test with an options monitor fake carrying the override for the method name).
|
||||
|
||||
**Step 3:** `EmitRouteCall(...)`: `requestSummary: _captureBodies ? TraceJson(args) : null, responseSummary: _captureBodies ? TraceJson(returnValue) : null, extra: …` and add `"bodyCapture":"skipped"`-style marker only if `ScriptTraceExtra` already has a free-form field; otherwise leave `extra` unchanged.
|
||||
|
||||
**Step 4:** `dotnet test tests/ZB.MOM.WW.ScadaBridge.InboundAPI.Tests` → PASS.
|
||||
|
||||
**Step 5: Commit** `fix(inbound-api): RouteCall audit row honours the method's SkipBodyCapture override (arch-review F11)`
|
||||
|
||||
---
|
||||
|
||||
### Task 13: F12 + F13 — documentation reconciled with the implementation
|
||||
|
||||
**Classification:** trivial (docs only)
|
||||
**Estimated implement time:** ~5 min
|
||||
**Parallelizable with:** Task 6, 9
|
||||
**Blocked by:** Tasks 1–12 (so the text describes the final code)
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/plans/2026-09-05-flow-debugger-design.md` (D6 row; §5.4 value-change paragraph; §9 role + redaction bullets; §10 volume + inbound-row-after-run note)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.Communication/Protos/sitestream.proto:102-106` (comment only — the vendored C# needs no regeneration for a comment)
|
||||
- Modify: `src/ZB.MOM.WW.ScadaBridge.CentralUI/Components/Pages/Design/ApiMethodForm.razor:50`, `src/ZB.MOM.WW.ScadaBridge.CLI/Commands/ApiMethodCommands.cs:65,101`, `src/ZB.MOM.WW.ScadaBridge.CLI/README.md:2122,2140`, `docs/requirements/Component-InboundAPI.md:73` ("~7" → "7–10 audit rows per call (3 of them trace-only)")
|
||||
- Modify: `docs/requirements/Component-InboundAPI.md` (new sentences: `SchemaRefResolver` is public for the Flow Debugger runner; `RouteHelper.WithSiteScope` + `SITE_SCOPE_DENIED`; `RouteCall` honours `SkipBodyCapture`)
|
||||
- Modify: `docs/requirements/Component-CentralUI.md` (Flow Debugger: page policy `RequireFlowDebugger`, Live controls Deployer-only, client wait derivation)
|
||||
- Modify: `CLAUDE.md` (Flow Debugger bullet: "~7 rows/call" → "7–10", one sentence on F1/F2/F3 outcomes with the review doc path)
|
||||
- Modify: `docs/plans/2026-09-06-flow-debugger-arch-review.md` (append `## 7. Resolution` listing each finding → commit)
|
||||
|
||||
Text to write (verbatim where possible):
|
||||
- D6: "**Live run is a Deployer act.** `RequireDeployment` gates the Run controls and the two run commands; the page and Replay are open to `OperationalAudit` roles + Deployer (`RequireFlowDebugger`, arch-review F3)."
|
||||
- §5.4: replace the debug-stream subscription paragraph with: "For the value-change entry there is no routed reply to ride on. The runner polls the site's `PullTraceEvents` RPC by `ParentExecutionId` (1 s cadence, 2 s settle window, `WaitSeconds` bound) — simpler than the debug-stream subscription originally planned. `AttributeValueUpdate.source_execution_id`/`trace` (fields 7/8) are populated by the site and **consumed by nothing**; they are reserved for a future debug-stream path and must not be reused."
|
||||
- §9 redaction bullet: "Trace payloads pass through the existing redaction policy on BOTH copies: the stored row (writer) and the inline Live copy (`FlowTraceCollector` applies `IAuditRedactor` and a 1 MB byte budget beside the 500-event cap — arch-review F1)." Add: "The method entry enforces the operator's site scope at route time (`RouteHelper.WithSiteScope`, `SITE_SCOPE_DENIED` → `ManagementUnauthorized`, arch-review F2)."
|
||||
- §10: "≈ 7–10 rows (a cached DB write and a notification add 3 trace-only rows)"; "Every routed call adds one central `RouteCall` insert — at inbound-driven rates that doubles the per-call central audit write (accepted, D5)"; "The Live run's inbound row is written AFTER the run completes, so a Replay taken during a long Live run shows only the `RouteCall`."
|
||||
|
||||
**Step 4:** `grep -rn "~7 audit\|~7 rows\|roughly seven" docs src CLAUDE.md` → no hits.
|
||||
|
||||
**Step 5: Commit** `docs: Flow Debugger design reconciled with the implementation (arch-review F12, F13)`
|
||||
|
||||
---
|
||||
|
||||
### Task 14: Full gate — build, full test run, rig rebuild, live verification
|
||||
|
||||
**Classification:** high-risk (rig)
|
||||
**Estimated implement time:** ~15 min wall (mostly waiting)
|
||||
**Parallelizable with:** none — last
|
||||
|
||||
**Steps**
|
||||
1. `dotnet build ZB.MOM.WW.ScadaBridge.slnx` → 0 errors.
|
||||
2. `dotnet test ZB.MOM.WW.ScadaBridge.slnx` → every project green except the 11 known Playwright role-negative tests (`.claude/skills/scadabridge-cluster-ops/SKILL.md`).
|
||||
3. `bash docker/deploy.sh` (site runtime, inbound API, management, central UI all changed).
|
||||
4. Mint a fresh key (see memory `delmia-rig-standin-state`), `FLOW_ONLY=1 DELMIA_API_KEY=… bash docker/verify-delmia-notifier.sh` → `H live+replay YES`, `I outbound live+replay YES`.
|
||||
5. Browser: open `http://localhost:9000/deployment/flow-debugger`, run the `FlowOutboundDemo` method Live, confirm the External lane still draws and the run completes.
|
||||
6. Report; do not merge or push without the user.
|
||||
@@ -0,0 +1,20 @@
|
||||
{
|
||||
"planPath": "docs/plans/2026-09-06-flow-debugger-arch-review-remediation.md",
|
||||
"tasks": [
|
||||
{"id": 1, "subject": "Task 1: F1 site — FlowTraceCollector redacts + byte-caps, moves to Commons", "status": "pending"},
|
||||
{"id": 2, "subject": "Task 2: F1 central — FlowDebugRunner uses the redacting collector", "status": "pending", "blockedBy": [1, 3]},
|
||||
{"id": 3, "subject": "Task 3: F2 — site scope on method-entry Live run", "status": "pending"},
|
||||
{"id": 4, "subject": "Task 4: F3 — Replay opens to OperationalAudit", "status": "pending"},
|
||||
{"id": 5, "subject": "Task 5: F4 — client waits derive from server bound", "status": "pending"},
|
||||
{"id": 6, "subject": "Task 6: F5 — emission order over cross-node timestamps", "status": "pending", "blockedBy": [2]},
|
||||
{"id": 7, "subject": "Task 7: F6 — trace arm cleared on failed write", "status": "pending"},
|
||||
{"id": 8, "subject": "Task 8: F7 — shed/suppressed traced run row", "status": "pending", "blockedBy": [1]},
|
||||
{"id": 9, "subject": "Task 9: F8 — fault continuations scheduler + logging", "status": "pending", "blockedBy": [1, 3, 8, 12]},
|
||||
{"id": 10, "subject": "Task 10: F9 — shared chain walker", "status": "pending"},
|
||||
{"id": 11, "subject": "Task 11: F10 — overlay matches by kind+title", "status": "pending"},
|
||||
{"id": 12, "subject": "Task 12: F11 — RouteCall honours SkipBodyCapture", "status": "pending", "blockedBy": [3]},
|
||||
{"id": 13, "subject": "Task 13: F12+F13 — docs reconciled", "status": "pending", "blockedBy": [1,2,3,4,5,6,7,8,9,10,11,12]},
|
||||
{"id": 14, "subject": "Task 14: full gate — build, tests, rig, verify", "status": "pending", "blockedBy": [13]}
|
||||
],
|
||||
"lastUpdated": "2026-09-06T00:00:00Z"
|
||||
}
|
||||
Reference in New Issue
Block a user