Merge branch 'worktree-agent-a5b63f3a0707e4e46' into fix/flow-debugger-arch-review

2026-09-06 14:15:58 -04:00
parent 3f1981865a e79452d2de
commit 05b97b596d
4 changed files with 138 additions and 11 deletions
@@ -1,6 +1,6 @@
 using ZB.MOM.WW.Audit;

-namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
+namespace ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging;

 /// <summary>
 /// Per-run sink for <c>ScriptTrace</c> audit events so a traced routed call can return them
@@ -14,17 +14,37 @@ namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
 /// when the launcher takes the reply snapshot — a lifecycle row the S&amp;F forwarder writes
 /// minutes later, or an un-awaited call finishing after the run returned, must not mutate a
 /// list the reply has already been built from. <see cref="Add"/> is a no-op once completed.
+/// Arch-review F1 (2026-09-06): the inline copy is <b>REDACTED</b> — the collector applies the
+/// host's <see cref="IAuditRedactor"/> itself rather than storing the raw event the emitter also
+/// hands the writer, so Live shows exactly what Replay reads back (per-target SQL-parameter
+/// overrides, header scrubbing and the 8 KB / 64 KB payload caps all apply). The writers keep
+/// their own <c>Apply</c> call — redaction is idempotent, so the stored row is unaffected.
+/// The copy is also capped by BYTES as well as by count, because it rides a gRPC reply whose
+/// default 4 MB receive limit a few hundred large error rows would otherwise breach — and that
+/// same reply carries the script's return value, so an oversized trace would fail a real call.
 /// </summary>
 public sealed class FlowTraceCollector
 {
    /// <summary>Default inline cap per run (design §5.4 <c>MaxTraceEventsPerRun</c>).</summary>
    public const int DefaultMaxEvents = 500;

+    /// <summary>
+    /// Default inline byte budget per run — the summed redacted <see cref="AuditEvent.DetailsJson"/>
+    /// length, well under the 4 MB gRPC default receive limit the reply must fit in.
+    /// </summary>
+    public const int DefaultMaxBytes = 1_048_576;
+
    private readonly List<AuditEvent> _events = new();
    private readonly object _gate = new();
    private readonly int _max;
+    private readonly int _maxBytes;
+    private readonly IAuditRedactor? _redactor;
+    private int _bytes;

-    /// <summary>True once an <see cref="Add"/> was dropped because the cap was reached.</summary>
+    /// <summary>
+    /// True once an <see cref="Add"/> was dropped because the event cap or the byte budget
+    /// was reached.
+    /// </summary>
    public bool Truncated { get; private set; }

    /// <summary>
@@ -33,25 +53,41 @@ public sealed class FlowTraceCollector
    /// </summary>
    public bool Completed { get; private set; }

-    /// <summary>Creates a collector with the given inline cap (must be positive).</summary>
-    public FlowTraceCollector(int maxEvents = DefaultMaxEvents)
+    /// <summary>
+    /// Creates a collector with the given inline caps (both must be positive) and the redactor
+    /// the inline copy is filtered through. A null <paramref name="redactor"/> — a host that
+    /// never called <c>AddAuditLog</c>, or a test — collects raw but is still capped.
+    /// </summary>
+    public FlowTraceCollector(
+        int maxEvents = DefaultMaxEvents,
+        int maxBytes = DefaultMaxBytes,
+        IAuditRedactor? redactor = null)
    {
        if (maxEvents <= 0) throw new ArgumentOutOfRangeException(nameof(maxEvents), "must be positive");
+        if (maxBytes <= 0) throw new ArgumentOutOfRangeException(nameof(maxBytes), "must be positive");
        _max = maxEvents;
+        _maxBytes = maxBytes;
+        _redactor = redactor;
    }

    /// <summary>
-    /// Appends an event, or marks the collector truncated once the cap is reached. A no-op
-    /// after <see cref="Complete"/>.
+    /// Appends the REDACTED copy of an event, or marks the collector truncated once the event
+    /// cap or the byte budget is reached. A no-op after <see cref="Complete"/>.
    /// </summary>
    public void Add(AuditEvent evt)
    {
        ArgumentNullException.ThrowIfNull(evt);
+        // Redaction runs OUTSIDE the lock (arch-review F1): IAuditRedactor is a pure function
+        // but not necessarily a cheap one (regex scrubbing + payload truncation), and the
+        // collector is shared by every emit site of a run.
+        var filtered = _redactor is null ? evt : _redactor.Apply(evt);
+        var size = filtered.DetailsJson?.Length ?? 0;
        lock (_gate)
        {
            if (Completed) return;
-            if (_events.Count >= _max) { Truncated = true; return; }
-            _events.Add(evt);
+            if (_events.Count >= _max || _bytes + size > _maxBytes) { Truncated = true; return; }
+            _events.Add(filtered);
+            _bytes += size;
        }
    }

@@ -247,7 +247,15 @@ internal static class ScriptRunLauncher
        // caller to reply to — a trace-armed trigger run (replyTo == Nobody) writes every row to the
        // audit writer for PullTraceEvents and would only keep a never-read list alive. The
        // ScriptRun row itself is emitted below on every exit path of a traced run.
-        var collector = trace && !replyTo.IsNobody() ? new FlowTraceCollector() : null;
+        // Arch-review F1: the inline copy is redacted at the collector, so the collector carries
+        // the host's IAuditRedactor. That is a SINGLETON (AddAuditLog), resolved here from the
+        // ROOT provider for the same reason siteEventLogger below is — the traced ScriptRun row
+        // is emitted from the catch blocks too, including the shed-at-dequeue path that throws
+        // before the DI scope exists. Null in tests / hosts without AddAuditLog: raw, still capped.
+        var collector = trace && !replyTo.IsNobody()
+            ? new FlowTraceCollector(
+                redactor: serviceProvider?.GetService<ZB.MOM.WW.Audit.IAuditRedactor>())
+            : null;
        var startedUtc = DateTime.UtcNow;
        ScriptRuntimeContext? context = null;
        IAuditWriter? auditWriter = null;
@@ -1,7 +1,7 @@
 using ZB.MOM.WW.Audit;
-using ZB.MOM.WW.ScadaBridge.SiteRuntime.Scripts;
+using ZB.MOM.WW.ScadaBridge.Commons.Types.Debugging;

-namespace ZB.MOM.WW.ScadaBridge.SiteRuntime.Tests.Scripts;
+namespace ZB.MOM.WW.ScadaBridge.Commons.Tests.Debugging;

 /// <summary>Flow Debugger design 2026-09-05 §5.4: the per-run inline trace sink.</summary>
 public class FlowTraceCollectorTests
@@ -85,6 +85,56 @@ public class FlowTraceCollectorTests
        Assert.Equal(snapshot, c.Complete()); // idempotent
    }

+    /// <summary>
+    /// Arch-review F1: the inline copy must be the SAME filtered record the writer persists —
+    /// the collector applies the redactor itself rather than handing on the raw event.
+    /// </summary>
+    [Fact]
+    public void Add_AppliesTheRedactor_SoTheInlineCopyIsTheFilteredEvent()
+    {
+        var c = new FlowTraceCollector(redactor: new UpperRedactor());
+        var evt = Evt(1) with { DetailsJson = "{\"secret\":\"abc\"}" };
+
+        c.Add(evt);
+
+        var stored = Assert.Single(c.Complete());
+        Assert.Equal("{\"SECRET\":\"ABC\"}", stored.DetailsJson);
+        Assert.NotSame(evt, stored);
+    }
+
+    /// <summary>
+    /// Arch-review F1: the 500-event cap alone does not bound the gRPC reply the inline copy
+    /// rides on — a byte budget over the redacted <c>DetailsJson</c> truncates first.
+    /// </summary>
+    [Fact]
+    public void Add_OverByteBudget_Truncates_KeepsEarlierEvents()
+    {
+        var c = new FlowTraceCollector(maxEvents: 500, maxBytes: 100);
+        var big = Evt(1) with { DetailsJson = new string('x', 80) };
+        var big2 = Evt(2) with { DetailsJson = new string('y', 80) };
+
+        c.Add(big);
+        c.Add(big2);
+
+        Assert.True(c.Truncated);
+        Assert.Single(c.Complete());
+    }
+
+    /// <summary>The byte budget has a documented default and is validated like the event cap.</summary>
+    [Fact]
+    public void DefaultByteBudget_Is1Mb_AndNonPositiveBudgetIsRejected()
+    {
+        Assert.Equal(1_048_576, FlowTraceCollector.DefaultMaxBytes);
+        Assert.Throws<ArgumentOutOfRangeException>(() => new FlowTraceCollector(maxBytes: 0));
+    }
+
+    /// <summary>Upper-cases <c>DetailsJson</c> so a test can see the filtered copy land.</summary>
+    private sealed class UpperRedactor : IAuditRedactor
+    {
+        public AuditEvent Apply(AuditEvent rawEvent)
+            => rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() };
+    }
+
    private static AuditEvent NewEvent() => new()
    {
        EventId = Guid.NewGuid(),
@@ -209,6 +209,39 @@ public class ScriptRuntimeContextTraceEmissionTests : TestKit
        Assert.False(ctx.IsTraced);
    }

+    /// <summary>
+    /// Arch-review F1: the collector applies the host redactor, so the inline copy the reply
+    /// carries is the FILTERED record — while the writer still receives the raw event and
+    /// redacts it itself (redaction is idempotent, so the stored row is identical).
+    /// </summary>
+    [Fact]
+    public async Task SetAttribute_WhenTraced_InlineCopyIsRedacted_WriterStillSeesTheRawEvent()
+    {
+        var probe = CreateTestProbe();
+        var writer = new CapturingAuditWriter();
+        var collector = new FlowTraceCollector(redactor: new UpperRedactor());
+        var ctx = CreateContext(probe.Ref, writer, traced: true, collector);
+
+        var call = ctx.SetAttribute("Setpoint", "42");
+        var cmd = probe.ExpectMsg<SetStaticAttributeCommand>(TimeSpan.FromSeconds(5));
+        probe.Reply(new SetStaticAttributeResponse(cmd.CorrelationId, InstanceName, "Setpoint", true, null, DateTimeOffset.UtcNow));
+        await call;
+
+        var written = Assert.Single(writer.Events);
+        var collected = Assert.Single(collector.Complete());
+        Assert.NotSame(written, collected);
+        Assert.Contains("\"channel\":\"ScriptTrace\"", written.DetailsJson);
+        Assert.NotEqual(written.DetailsJson, collected.DetailsJson);
+        Assert.Equal(written.DetailsJson!.ToUpperInvariant(), collected.DetailsJson);
+    }
+
+    /// <summary>Upper-cases <c>DetailsJson</c> so a test can see the filtered copy land.</summary>
+    private sealed class UpperRedactor : IAuditRedactor
+    {
+        public AuditEvent Apply(AuditEvent rawEvent)
+            => rawEvent with { DetailsJson = rawEvent.DetailsJson?.ToUpperInvariant() };
+    }
+
    [Fact]
    public async Task SetAttribute_WhenTraced_FailedWrite_EmitsFailedRow_ThenThrows()
    {