Skip to content

Investigation requests: a viewer panel for open, claimed and reported requests (#5904) - #5907

Merged
erikdarlingdata merged 9 commits into
devfrom
fix/5904-investigation-panel
Oct 16, 2026
Merged

erikdarlingdata merged 9 commits into
devfrom
fix/5904-investigation-panel

Conversation

@erikdarlingdata

@erikdarlingdata erikdarlingdata commented Oct 16, 2026 •

Copy link
Copy Markdown
Owner

Claude posting for Erik Darling.

Fixes #5904.

Both halves are on this branch: the Darling web page (sections below, unchanged) and the Lite WPF panel (## Lite half).

What changed

  • A new Investigations page in the web viewer (#/investigations, nav entry "Investigations" under Alert History). It lists investigation requests over /api/read/list_investigation_requests (limit 100, in the MCP list order, opened_at DESC, request_id): opened, state, server, metric, family, alert count, agent, lease until, reported, route. The State filter (all, open, claimed, reported, closed, expired) and the Family filter go to the read, so the read applies them before its limit. Picking a row reads that one request over /api/read/get_investigation_reports?request_id= and shows its report(s) (verdict, summary, root cause, evidence, agent, time), the stored brief as served, and the journal in the read's order (event_time DESC, journal_id DESC) with the claim time taken from the journal's claimed event. Reports carry the note that they are an agent's unreviewed, self-attributed claim. Empty, error and truncation lines (list truncated, journal truncated) are shown. No write action. The 60 s poll rebuild keeps the filters and the open request.
  • Text only. Every value, the brief and report text included, is drawn through el()'s text path or a <pre>; nothing is built as HTML.
  • Field tables. list_investigation_requests and get_investigation_reports (journal rows) entries in wwwroot/js/read-fields.js; the page reads its columns from there.

Masking

One rule for both halves: mcp.dataDetail masks agents over MCP only; an operator's own window reads at Full. The Darling web page and the Darling Viewer show the full report, brief and journal (the web get_investigation_reports read passes McpDataDetail.Full, as on dev), and the Lite panel now does the same: it builds its pane from InvestigationToolCore.ReportsAnswer at Full and takes no data-detail input at all, so the MCP host's setting and its start/stop no longer change what the operator sees. The MCP tools themselves still mask for agents at the configured level.

Existing panel left in place

pages/alerts.js already has a collapsed read-only "Investigations" disclosure under the alert table (#5632). I left it untouched (its tests pin it); the new page is the full viewer. Say if you want the disclosure removed in favor of a link.

Tests (behavior only; no new source-text pins)

  • InvestigationsPageTests (Node harness investigations-harness.mjs runs the shipped page against a recording fetch): list columns and order, the state and family params, a picked request's report/brief/journal, markup in the data stays text (no img or script element is created), the rebuild keeps the open request, Close, no-report line, empty, error and truncation lines.
  • InvestigationToolsLiveTests.TheWebReads_CarryTheFieldsThePageDraws_InTheMcpOrder_AndTheReportIsServedInFull (live store): both web reads carry every field the page uses, the list is opened_at DESC, the journal event_time DESC, and the web report and brief equal the MCP tool's full-level answer.
  • AlertNotebookTableCatalogTests: its "every catalog key is a key the read's payload builds" scan looked only in the Mcp folder; the two reads build their payload in PerformanceMonitor.Common/Investigations/InvestigationToolCore.cs, so I added that file as the source for those two reads (the rule still holds).
  • The brief's "nav and census pins" are not added: the nav entry sits after Alert History so the existing last-entry (Job History) and Admin-follows-Alert-Rules pins still hold, and a new pin would assert spelling.

Verification

  • Gate (pre-push.sh, Git Bash): guard-darling Total: 4181, Errors: 0, Failed: 0, Skipped: 0 and guard-lite Total: 1147, Errors: 0, Failed: 0, Skipped: 0. The selection stage then stopped ("selection is the whole suite ... run the brief's classes; CI decides"), so no diff-selected classes ran from the script.
  • Brief's classes by full name, plus Census classes, the web page classes (JobHistory, Admin, FleetSweepWebFeed, ManageTags, MuteRules, FinOps shell, AlertHistoryRange), the notebook and view-template field-table classes, WebReadCancellationPinTests, EffectiveReadWindowTests, the investigation tool and contract live classes: Darling.Tests Total: 845, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0 on my own rig (port 55904, stopped).
  • Gate guard stage ran green (guard-darling Total: 4181, Failed: 0, guard-lite Total: 1147, Failed: 0) before the follow-up that put the report read back to Full; after it I rebuilt and reran the class list above (Total: 845, Errors: 0, Failed: 0), not the gate. The only product files that changed are back to dev's text.
  • No browser check with playwright-cli (the page needs a running service with a store; the Node harness runs the shipped page script instead).

Leftovers

  • None for the Lite half (see ## Lite half).
  • Nothing in deprecated/ touched; no CHANGELOG edit.

Lite half

Read-only WPF panel over the DuckDB investigation tables #5902 added. No MCP tool, no action on a request (claim, release and approve are not offered), nothing new for agents.

  • Lite/Investigations/InvestigationPanelReader.cs (new): the read side. ListAsync calls the store's existing ListAsync (limit 100 + 1 for the truncated flag), which already returns opened_at DESC, request_id, so no new store read was needed. ReadDetailAsync calls ReadReportsAsync for one request (its own connection, no write lock) and builds the pane from the same InvestigationToolCore.ReportsAnswer the get_investigation_reports tool serves, so the report, journal and stored brief come from the one builder the tool uses, at Full (see ## Masking). Times and event names come from the read row for row. Claimed time is the newest claimed journal event (the list carries agent and lease end), the same split the Darling page uses.
  • Lite/Windows/InvestigationsWindow.xaml(.cs) (new): list columns state, server, metric, family, alerts, opened, agent, lease until, reported, route (the web page's ten columns; State leads here and Opened leads there) (State and Family filters as on the Darling page, Refresh button); picking a row shows the reports (agent, time, verdict, summary, root cause, evidence), the stored brief and the journal grid. Reads run in Task.Run; a stale pick is dropped by a sequence number. Time columns sort by their UTC DateTime (GridTimeColumnSortMemberTests requires it).
  • Lite/MainWindow.xaml(.cs): an "Investigations" button in the sidebar under Manage Tags opens the window modeless. The reader is built with the store only; MainWindow._mcpDataDetail (the MCP host's agent-facing level) is not consulted.
  • Lite.Tests/InvestigationPanelReaderTests.cs (new, 6 tests, throwaway DuckDB): the list order (ties on opened_at break by request_id, equal to the store's list); the alert count each request carries; state and family filters; the summary, verdict, root cause and stored brief show in full while the tool's answer at metadataOnly for the same request withholds them; a submitted report and its journal render under the request with the report text in full; a request with two reports and three journal rows renders all of them in order with the claimed time from the journal.

Notes for the reviewer:

  • investigation_reports.request_id is UNIQUE, so the store never holds two reports for one request; the two-report case is tested at the mapping level (InvestigationPanelReader.Detail over a constructed InvestigationReportsRead), because the MCP read shape allows a list.
  • No browser or screenshot check: this is WPF, not the web UI. The window was not opened by hand on this machine; XAML compiles and the WPF-reading classes below pass.

Verified (Lite.Tests.exe, one run with every class filter): *Census*, *Ratchet*, *Investigation* (includes DuckDbInvestigationStoreTests, the new reader tests and the MCP investigation tools classes), *GridTimeColumnSort*, *ThemeColorOverride*, *CrossAppGuard*, *TabLoads*, *McpWritePolicy*, *McpPageContract*, *AlertReadFailureSurface*, *DocCommentHygiene*, *McpPayloadContract*, *Inventory*, *AlertLegendParity*, *SettingsSaveReport*, *Wpf*, *Smoke*, *Xaml*, *Window*: Total: 950, Errors: 0, Failed: 0. Build Build succeeded. The first run failed GridTimeColumnSortMemberTests on the journal Time column; fixed with SortMemberPath to UTC members. Full suite not run (laptop rule; CI decides).

CHANGELOG entry

SECTION: Added
ENTRY:

Round 1 review

CI run 38845482264 and the bot's review (CHANGES REQUESTED on 63fddd1). Fixed in the push after it; the last round for this PR.

Red tests

  • InvestigationsPageTests.PickingARequest_ReadsThatOneRequest_AndDrawsItsReportBriefAndJournal_AsText: the claimed .*6:05 line pinned the UTC-4 rendering of 10:05Z. Dropped; the claim-time mapping stays pinned zone-independently by claimedAt in ARequestWithNoReport_SaysSo_AndTheEmptyErrorAndTruncationLinesAreDrawn. The class passes under the laptop's zone and with TZ=UTC.
  • QueryStoreComposeStampWriterGuardLiveTests.AnUnrelatedLongWriter_ATempTableAndAWriteToAnotherTable_NoLongerDefersTheHour (shard 5, not in this PR's diff): not reproduced. The whole class ran three times on a throwaway UTC PostgreSQL rig and passed every time (5 of 5 tests, Skipped 0, three runs). Left to CI's rerun; no change.

Findings

  1. Lite masking (MainWindow.xaml.cs:1789). The panel no longer reads _mcpDataDetail; InvestigationPanelReader has no data-detail parameter and builds the pane at Full. InvestigationPanelReaderTests.ABriefStoredFullIsServedMaskedAtTheLevelTheToolsServeAndStaysMasked is deleted and replaced by ThePanelShowsSummaryVerdictRootCauseAndBriefInFull_WhileTheMcpHostRunsAtMetadataOnly (the same stored report through ReportsAnswer at metadataOnly withholds the literal; the panel shows summary, verdict, root cause and brief in full). AReportAndItsJournalComeBackUnderTheRequestWithTheReportTextInFull is the old masked-report test, flipped. The blank Verdict/Root cause at metadataOnly the review describes can no longer happen: nothing is masked in the panel. The ## Masking rule above states it for both halves. The window's header comments (xaml, .xaml.cs, reader, test class) are updated to say the same.
  2. Lite grid parity (InvestigationsWindow.xaml:59). Added the Alerts column (InvestigationPanelRequest.AlertCount, right-aligned like Route); the grid now has the web page's ten columns, and the comment at :11 says so, including that State leads here and Opened leads there. Pinned by TheListCarriesTheAlertCountOfEachRequest (a joined request shows 2, a single one 1). ClaimCount stays on the record, unbound (harmless, no column wants it).
  3. Web page CSS (investigations.js:132). I reused the defined classes: the detail slot is jh-detail (border, background, :empty { display: none }), its head is jh-detail-head (flex, Close at the right edge), and the summary, root cause, evidence and stored brief <pre>s are jh-detail-message (pre-wrap, max-height, scroll). The two remaining hooks, inv-section and inv-report, now have small spacing rules in app.css beside the jh- block. The Node harness cannot pin CSS, so this is by construction, not by a test; the harness now finds the slot by jh-detail.
  4. rowsUnder copy (investigations.js:63). One definition now: rowsUnder is exported from wwwroot/js/util.js (the array-reply branch from the alerts.js version kept) and both alerts.js and investigations.js import it; neither keeps a copy. The existing AlertHistory and Investigations page classes pass.
  5. Duplicate alerts.js disclosure (non-blocking in the review, and the question this body raised): kept as is. Replacing it with a link to #/investigations is still Erik's call; it needs its tests rewritten and is not part of this fix round.
  6. Traps the review marked "does not apply" (truncation limit + 1, total_* from a .Count, 2^53 ids, Tests that assert spelling, not behavior: cap them, stop the review asking for pins, delete the shape ones #5833 source-text pins): no change; no source-text test was added this round.
  7. Unverified items: the XAML window has still not been opened by hand (it builds; NumericCell resolves as in ProcedureHistoryWindow.xaml); no browser check of the page's wrapping/empty-pane CSS (reuse of the existing classes is the answer). The earlier gate counts are superseded by the runs below.

Verified this round

  • Lite.Tests.exe, builds Build succeeded. (0 warnings): *Investigation*, *Census*, *Ratchet*: Total: 154, Failed: 0; *InvestigationPanelReader* alone: Total: 6, Failed: 0; *Census* + *Ratchet* alone: Total: 32, Failed: 0.
  • Darling.Tests.exe (build Build succeeded., 0 warnings): *InvestigationsPageTests* Total: 5, Failed: 0 (also with TZ=UTC); *PageTests*, *Census*, *Ratchet*, *InvestigationsPage*, *WebAssets*, *Alerts*, *Javascript*: Total: 1045, Failed: 0; *AlertHistory*, *AlertSampleAge*, *WebClickthrough*, *Alert*: Total: 2661, Failed: 0, Skipped: 150; QueryStoreComposeStampWriterGuardLiveTests three runs, 5 of 5 each.
  • Not run: the full suites (laptop rule; CI decides).

@erikdarlingdata
erikdarlingdata marked this pull request as ready for review October 16, 2026 03:25
// The journal keeps the read's order (newest first) and the claim time comes from its claimed event.
var journal = picked.GetProperty("journalRows").EnumerateArray().Select(Strings).ToArray();
Assert.Equal(new[] { "reported", "claimed", "opened" }, journal.Select(r => r[1]));
Assert.Matches("claimed .*6:05", text);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assertion is pinned to your machine's timezone, not to behaviour.

The claimed event in the harness is 2026-10-14T10:05:00Z (investigations-harness.mjs, the journal fixture). The page renders it through localTime → util.js:504, d.toLocaleString(), which formats in the Node process's local zone. Neither Run() here nor the harness sets TZ.

  • On your rig (US Eastern, UTC−4 in October) 10:05Z renders 6:05:00 AM, so claimed .*6:05 matches.
  • On a GitHub-hosted runner, which is UTC, it renders 10/14/2026, 10:05:00 AM. There is no 6:05 anywhere in the pane text — the journal grid's format: "time" cells are localized the same way, the brief carries no times, and the evidence is head blocker 412 s — so Regex.IsMatch fails and PickingARequest_ReadsThatOneRequest_AndDrawsItsReportBriefAndJournal_AsText goes red.

.github/workflows/build.yml sets no runner timezone (the only log_timezone lines are PostgreSQL server settings), so this is a hard red on CI rather than a flake. The surrounding assertions in this same fact are zone-independent by construction ("Verdict: needs_person", "agent-a", the createdImgOrScript count, the journal's event order), which is the shape to follow.

The behaviour actually worth pinning here — "the claim time comes from the claimed event, not from the row" — is already pinned zone-independently by out.claimedAt/out.claimedAtNever, asserted at ARequestWithNoReport_SaysSo_... (Assert.Equal("2026-10-14T10:05:00Z", run.GetProperty("claimedAt").GetString())). Either drop this line as redundant, or have the harness emit the rendered fact string and compare it against localTime applied to the same instant, so the test states the mapping rather than one zone's output of it.

Comment thread Lite/MainWindow.xaml.cs Outdated
store and masks at the level the MCP investigation tools serve now (_mcpDataDetail), read when each request is picked. */
private void InvestigationsButton_Click(object sender, RoutedEventArgs e)
{
var reader = new PerformanceMonitorLite.Investigations.InvestigationPanelReader(InvestigationStore, () => _mcpDataDetail);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two halves of this PR apply opposite masking rules to the same operator-facing surface, and Lite's half makes the panel's readability depend on whether the MCP host happens to be running.

The PR body's "## Masking" section states the rule: "The web viewer shows operators the full report and brief, like the WPF viewer … mcp.dataDetail masks agents over MCP only, so no web read is masked." That is what the Darling half does — DarlingWebEndpoints.cs:6161 hard-codes McpDataDetail.Full, and the Darling Viewer's existing InvestigationsWindow (Darling/PerformanceMonitor.Darling.Viewer/InvestigationsWindow.xaml.cs, ViewerDataService.Investigations.cs) contains no masker at all. The new page's own header comment says it too (pages/investigations.js:14).

Lite's half does the opposite. _mcpDataDetail is the MCP host's agent-facing level: it is Full at construction, set to mcpSettings.DataDetail when the host starts (MainWindow.xaml.cs:729) and reset to Full when it stops (:752). Every other consumer of that field is agent-facing — it is handed to EmailAlertService for the webhook token's brief (:168), i.e. the brief an agent fetches.

Concretely, for an operator who has hardened mcp_data_detail to metadataOnly — the reason the setting exists:

  • Start the MCP host. Open Investigations, pick a reported request.
  • InvestigationToolCore.ReportsAnswer runs McpDataDetailMasker.ForAnswer(MetadataOnly); InvestigationFieldMasking.Mask replaces the whole report node with {"withheld": …} (InvestigationFieldMasking.cs:43). InvestigationPanelReader.Detail then yields Summary = "(withheld) …" and — because only summary and reason go through Withheld() — Verdict and Root cause come back as empty strings (InvestigationPanelReader.cs:1118-1119, Text(node["verdict"]) / Text(node["root_cause"]) on a node that has neither key). The operator's own desktop app will not show them their own agent's findings.
  • Stop the MCP host. The same request now renders in full, because the field flipped back to Full.

So the level is governed by an unrelated service's lifecycle, and a Darling operator in the identical configuration sees everything. InvestigationPanelReaderTests.ABriefStoredFullIsServedMaskedAtTheLevelTheToolsServeAndStaysMasked pins this behaviour, so it is intentional — but it pins the mechanism, not the question of whether the local operator is an agent. Per CONTRIBUTING's two-store rule and the PR's own stated rule, this panel should read at Full like its Darling twins; if the masking is genuinely wanted, it needs its own setting rather than the MCP host's, and the Verdict/Root cause blanking needs the Withheld() treatment Summary already gets.

.filter(Boolean).join(" · ");
const parts = [el("div", { class: "muted", text: head })];
if (rep.verdict != null) parts.push(el("div", {}, [el("strong", { text: "Verdict: " }), el("span", { text: valueText(rep.verdict) })]));
if (rep.summary != null) parts.push(el("div", {}, [el("strong", { text: "Summary: " }), el("pre", { class: "inv-text", text: valueText(rep.summary) })]));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every inv-* class this page invents exists in no stylesheet, and the one on these <pre>s is load-bearing.

grep -rn "inv-" Darling/PerformanceMonitor.Darling.Service/wwwroot/css/ returns nothing: inv-text, inv-brief, inv-report, inv-section, inv-detail (:203) and inv-detail-head (:153) are all undefined across app.css, theme.css, editor.css and time-range.css.

For most of them that is only missing chrome. For inv-text it is a layout defect, because a bare <pre> is white-space: pre. The page this one is modelled on spells the same thing with a class that is defined — job-history.js:366 uses jh-detail-message, and app.css:1325 gives it exactly what a <pre> holding agent free text needs:

.jh-detail-message { margin: 8px 0 0; max-height: 24rem; overflow: auto; white-space: pre-wrap; overflow-wrap: anywhere; }

summary is capped at InvestigationToolCore.MaxSummaryChars = 4000 and root_cause at MaxRootCauseChars = 8000, and neither cap requires a newline — an agent that writes one paragraph produces a single 4,000-character line. With white-space: pre that line does not wrap, so the <pre> overflows and the page gets a horizontal scrollbar; with no max-height/overflow the stored brief at :169 pushes the Journal section off-screen instead of scrolling in place.

Two related misses from the same copy: inv-detail lacks the .jh-detail pane's border/background and its app.css:1322 :empty { display: none; }, so after Close the empty slot still occupies layout; and inv-detail-head lacks .jh-detail-head's display: flex; justify-content: space-between, so the Close button does not sit at the right edge.

The Node harness cannot see any of this — it renders into a node tree with no styling, which the test class says outright ("Behavior only; the spelling of the page's source is not pinned"). Simplest fix is to reuse jh-detail/jh-detail-head/jh-detail-message (the page already reuses jh-detail-close at :155), or to add the inv-* rules to app.css beside them.

Comment on lines +51 to +59
<DataGridTextColumn Header="State" Binding="{Binding State}" Width="80"/>
<DataGridTextColumn Header="Server" Binding="{Binding ServerName}" Width="160"/>
<DataGridTextColumn Header="Metric" Binding="{Binding FirstMetric}" Width="180"/>
<DataGridTextColumn Header="Family" Binding="{Binding Family}" Width="130"/>
<DataGridTextColumn Header="Opened" Binding="{Binding Opened}" SortMemberPath="OpenedAtUtc" Width="140"/>
<DataGridTextColumn Header="Agent" Binding="{Binding Agent}" Width="130"/>
<DataGridTextColumn Header="Lease until" Binding="{Binding LeaseUntil}" SortMemberPath="LeaseUntilUtc" Width="140"/>
<DataGridTextColumn Header="Reported" Binding="{Binding Reported}" SortMemberPath="ReportedAtUtc" Width="140"/>
<DataGridTextColumn Header="Route" Binding="{Binding RouteId}" ElementStyle="{StaticResource NumericCell}" Width="60"/>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alert count is missing, and the comment eight lines up says these columns match the web page's.

:11 claims "The same columns and grouping as the Investigations page in the Darling web UI." The web page's columns are REQUEST_COLUMNS = READ_FIELDS.list_investigation_requests.table.columns (read-fields.js:783-794), ten of them, and the PR body's web bullet lists them including alert count. This grid has nine: alert_count is the one that is gone.

It is not a cosmetic omission — it is how many alerts joined the request, which is what distinguishes a one-off from a server that has fired the same family eleven times in the grouping window. The list tool's own guide text names it as a row field ("Each row: id, server, family, first metric, alert count, state, lease and expiry", DarlingMcpInvestigationTools.cs:73), the web page shows it, and the Darling Viewer's existing Investigations window shows it too (Darling/PerformanceMonitor.Darling.Viewer/InvestigationsWindow.xaml:70, Header="Alerts") — so Lite is the only one of the three surfaces without it.

The reader has the value but does not surface it: InvestigationPanelRequest exposes ClaimCount (InvestigationPanelReader.cs:33), which no column in this file binds, and exposes no AlertCount at all, while Row.AlertCount is right there on the record. Adding public int AlertCount => Row.AlertCount; and an Alerts column (and dropping or binding ClaimCount) closes both halves.

Either way the comment at :11 needs to stop asserting parity it does not have — the order also differs (the web leads with Opened, this leads with State), which is fine on its own but makes the sentence false twice over.

export const JOURNAL_COLUMNS = [...READ_FIELDS.get_investigation_reports.table.columns, { key: "detail_text", label: "Detail", wrap: true }];

/* The rows of a reply under `key`; [] for anything else. */
function rowsUnder(data, key) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: rowsUnder is a copy of alerts.js:730, minus its "or the reply itself when it is an array" branch.

The traps file's line on this is "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." These two are not even pinned equal, so a fix to one (the array branch is exactly the kind of thing that gets added) silently leaves the other behind. alerts.js already exports investigationRows, so exporting rowsUnder beside it and importing it here costs one line.

The same applies one level up, and the PR body already raises it under "Existing panel left in place": the collapsed disclosure at alerts.js:786-820 and this page are now two Investigations surfaces over the same two reads, with different columns (INVESTIGATION_COLUMNS carries verdict/summary and a per-row journal disclosure; REQUEST_COLUMNS carries lease_until/reported_at/route_id), different journal rendering (journalLine joined with · vs a VIZ.table) and different truncation copy. Since you asked: yes, I'd replace the disclosure with a link to #/investigations — but that is your call and it is disclosed, so it is not what the verdict rests on.

@claude

claude Bot commented Oct 16, 2026

Copy link
Copy Markdown

Review posted inline. Four findings; the verdict follows in a formal review.

claude[bot]
claude Bot previously requested changes Oct 16, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: CHANGES REQUESTED

Four findings, inline. The one that stops CI is a timezone-pinned assertion in InvestigationsPageTests; the one that matters most is that the two halves apply opposite masking rules to the same operator-facing surface.

Claims

  • VERIFIED — New page at #/investigations, nav under Alert History, routed and rendered — index.html:39, app.js:48, :91, :186.
  • VERIFIED — Lists over list_investigation_requests at limit 100 in the MCP list order with those ten columns, State and Family sent to the read so it filters above its LIMIT — investigations.js:56, :103, read-fields.js:783-794, DarlingMcpInvestigationTools.cs:108.
  • VERIFIED — A picked row reads one request by request_id and draws reports, stored brief and journal in the read's order, claim time from the journal's claimed event; the unreviewed/self-attributed note is on the reports — investigations.js:75, :142, :160, :181.
  • VERIFIED — Empty, error, list-truncated and journal-truncated lines; no write action (only readTool with the two read names) — :171, :186, :223, :226, :231, :239.
  • VERIFIED — The poll rebuild keeps the filters and the open request — module-scope choices (:104); app.js:479 does not list the route as no-poll.
  • VERIFIED — Text only — every value takes el()'s text: path or a pre, and util.js:36 throws on an html prop; pinned by createdImgOrScript == 0.
  • VERIFIED — The web report read passes McpDataDetail.Full, so no web read is masked — DarlingWebEndpoints.cs:6161; the Darling Viewer's window holds no masker either (ViewerDataService.Investigations.cs names McpDataDetail nowhere).
  • VERIFIED — The alerts.js disclosure is untouched (absent from the diff, still at alerts.js:709-820), and the nav/router pins hold with no new ones: job-history is still last, and both AdminPageTests pins scan spans the new lines sit above.
  • VERIFIED — The catalog scan looked only in the Mcp folder and these reads build their payload in InvestigationToolCore.cs — PayloadSourceFor (AlertNotebookTableCatalogTests.cs:284-312); every catalog key resolves there (ReportsAnswer:321-332, RequestPayload:336-354).
  • VERIFIED — Lite reuses the store's list at limit + 1, already ordered opened_at DESC, request_id, and reads one request on its own connection with no write lock — InvestigationPanelReader.cs:69-76, ListSql:167, ReadReportsAsync:661-667 (passing utcNow as sinceUtc is harmless; ReportsRequestsSql:223 gates that predicate on a non-null request id).
  • VERIFIED — The pane comes from the same ReportsAnswer the tool serves, times and events row for row — InvestigationPanelReader.cs:88-104; the positional node mapping is sound because ReportsAnswer appends one node per row in order (InvestigationToolCore.cs:266, :304).
  • VERIFIED — Lite's nine list columns and its UTC SortMemberPaths — InvestigationsWindow.xaml:51-59, :115. The body's list is accurate; the comment at :11 claiming "the same columns" as the web page is not.
  • VERIFIED — Lite masks at MainWindow._mcpDataDetail, the MCP host's setting, Full while it is not running — MainWindow.xaml.cs:125, :729, :752, :1789. Finding 2 rests on this.
  • VERIFIED — investigation_reports.request_id is UNIQUE, so the two-report case must be tested at the mapping level — Lite/Database/Schema.cs:374.
  • UNVERIFIED — The gate and class-filter counts (4181 / 1147 / 845 / 950, all Failed: 0) — rig results; finding 1 says the UTC outcome differs for at least one class.
  • UNVERIFIED — InvestigationToolsLiveTests.TheWebReads_... passes — needs DARLING_TEST_PG and a live store. It compiles against real symbols; its reports/brief comparison is near-tautological (dispatch and tool called with identical arguments), so what it pins is that the dispatch forwards Full and request_id.
  • UNVERIFIED — "XAML compiles", window never opened by hand — needs a Windows build and a run. NumericCell at :59 does resolve as ProcedureHistoryWindow.xaml resolves it.

Riskiest lines

  1. Darling/Darling.Tests/InvestigationsPageTests.cs:95 — Assert.Matches("claimed .*6:05", text) compares against localTime's toLocaleString() of 2026-10-14T10:05:00Z, encoding a UTC-4 rendering; a UTC runner renders 10:05 and no 6:05 exists in the pane. It IS the pin, pinned to the wrong thing.
  2. Lite/MainWindow.xaml.cs:1789 — the _mcpDataDetail closure makes an operator panel's readability a function of the MCP host's lifecycle and its agent-facing hardening level — pinned by ABriefStoredFullIsServedMaskedAtTheLevelTheToolsServeAndStaysMasked, which pins the mechanism, not whether the local operator is an agent.
  3. wwwroot/js/pages/investigations.js:132 — a pre on class inv-text, defined in no stylesheet, so a 4,000-character newline-free summary renders without white-space: pre-wrap — UNPINNED; the harness renders into an unstyled node tree.

Traps near this diff

  • "Truncation inferred from a full page ... Fetch limit + 1; the flag is fetched.Count > limit." — does not apply: InvestigationPanelReader.cs:71-73 fetches ListLimit + 1 and flags rows.Count > ListLimit; the page reads truncated off the tool, same shape at DarlingMcpInvestigationTools.cs:108-110.
  • "A total_* assigned from a .Count is a count of the page unless the set is provably whole." — does not apply: investigations.js:240 says "N requests shown" and the notice says "the newest N requests; older ones are not listed".
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies: rowsUnder at investigations.js:63 is a copy of alerts.js:730 with a branch dropped, and the surface itself duplicates the alerts.js:786 disclosure. Non-blocking; the body raises the second half and asks for a decision.
  • "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — applies twice: the masking policy (Lite masks; the Darling page and Viewer do not) and the column set (alert_count is at read-fields.js:789 and at PerformanceMonitor.Darling.Viewer/InvestigationsWindow.xaml:70, on neither Lite list).
  • "Ruling (#5833): tests are load-bearing ... never answer a trap with a test that reads source text or asserts how code is spelled." — does not apply: the three new test files are behaviour tests, and the only source-text read added is one entry in the pre-existing census at AlertNotebookTableCatalogTests.cs:259-265.
  • "A JSON number above 2^53 is rounded by every JavaScript client." — does not apply: request_id is a GUID serialized as a string (InvestigationToolCore.cs:289, :338); the other ids are small ints.

Divergence (blind read vs PR body)

  1. The masking rule is stated for one half and inverted in the other. "## Masking" states the product rule — mcp.dataDetail masks agents over MCP only, so no web read is masked — and the Darling half implements it. The Lite half routes the same operator-facing panel through the MCP host's mcp_data_detail, described accurately but never reconciled with that rule. The consequence the body omits: at metadataOnly the operator's own panel shows (withheld) for the summary and empty Verdict and Root cause, because only summary and reason pass through Withheld() (InvestigationPanelReader.cs:1118-1119 against :1146-1147), and it becomes readable again when the MCP host stops.
  2. "Verified ... Failed: 0" against a timezone-pinned assertion. The body reports both suites green on the author's rig and leaves the rest to CI. InvestigationsPageTests.cs:95 can only pass at UTC-4 or UTC+8, and CI runs at UTC with no TZ set anywhere in build.yml.
  3. "The same columns" is asserted in the code, not the body. The body's two column lists differ honestly, so nothing in the body is refuted — but InvestigationsWindow.xaml:11 claims parity with the web page's columns, and the missing one is the column both other surfaces carry.

Reviewer checklist

  • Run the harness under a UTC clock and then InvestigationsPageTests on a UTC machine, and confirm the 6:05 assertion fails as read — Node could not be executed here, so finding 1 is confirmed by reading localTime (util.js:502-505) and the fixture instant, not by a run.
  • Load #/investigations against a store holding a report whose summary is one long newline-free paragraph: does the pre wrap or push a horizontal scrollbar, and does the empty detail slot still take layout after Close?
  • On Lite, start the MCP host at mcp_data_detail = metadataOnly, open Investigations, pick a reported request and see what the operator can read; then stop the host and reopen it.
  • Build Lite on Windows and open the window by hand — the report template, the NumericCell lookup and the row split are unexercised.
  • Decide the duplicate-surface question the body asks: keep the alerts.js disclosure, or replace it with a link.

The Lite panel reads at Full like the web page and the Viewer (mcp.dataDetail masks agents over MCP only), gains the Alerts column, the web page reuses the jh-detail classes and shares rowsUnder from util.js, and the zone-pinned 6:05 assertion is dropped.
@claude

claude Bot commented Oct 16, 2026

Copy link
Copy Markdown

Follow-up review (range 63fddd15c..0f3fc9cd2, the "round-1 review fixes" commit).

Verified independently, before reading the PR body:

  • InvestigationPanelReader drops its Func<McpDataDetail> parameter and always reads at Full (Lite/Investigations/InvestigationPanelReader.cs:77,99). Confirmed this isn't a parity regression: DarlingWebEndpoints.cs:6161 already passed McpDataDetail.Full to ReportsAsync for the web panel before this range, so Lite is catching up to an existing Darling behavior, not diverging from it. The only caller (MainWindow.xaml.cs:1789) was updated to match; no other callers exist.
  • AlertCount surfaced on InvestigationPanelRequest (InvestigationPanelReader.cs:34) is a passthrough of a pre-existing InvestigationRequestRow.AlertCount field; the new Alerts column binds it and uses {StaticResource NumericCell}, which resolves app-wide via App.xaml's merged theme dictionary — same pattern as ProcedureHistoryWindow.xaml.
  • CSS: .inv-section/.inv-report had no rules at all at the base of this range (git show 63fddd15c:...app.css | grep inv- is empty) — the panel was unstyled before. This commit gives it the jh-detail/jh-detail-head/jh-detail-message classes (shared with Job History) plus two new small rules for the two classes that don't map onto that shared set. The Node harness (investigations-harness.mjs:166) was updated to find the slot by its new class, matching the rename.
  • rowsUnder dedup: the merged definition (now only in util.js) takes the superset behavior (bare-array branch from alerts.js's copy). Checked both MCP tool replies (list_investigation_requests, get_investigation_reports) always return an object, never a bare array, so the extra branch is dead code for investigations.js's three call sites, not a behavior change.
  • The dropped Assert.Matches("claimed .*6:05", text) in InvestigationsPageTests.cs was zone-dependent; the claim-time mapping it was redundant with is still pinned zone-independently via claimedAt assertions later in the same file (:130-131).

All of this matches the PR body's "Round 1 review" section point for point — no divergence found.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: LGTM

Claims

  • VERIFIED — "The panel no longer reads _mcpDataDetail; InvestigationPanelReader has no data-detail parameter and builds the pane at Full" — Lite/Investigations/InvestigationPanelReader.cs:77,99 and the single caller update at Lite/MainWindow.xaml.cs:1789
  • VERIFIED — "Added the Alerts column (InvestigationPanelRequest.AlertCount)... the grid now has the web page's ten columns" — InvestigationsWindow.xaml:52-61 (10 DataGridTextColumns), matching the web harness's 10-header string in InvestigationsPageTests.cs:66
  • VERIFIED — "ClaimCount stays on the record, unbound (harmless, no column wants it)" — grep confirms no ClaimCount binding in InvestigationsWindow.xaml
  • VERIFIED — "I reused the defined classes: the detail slot is jh-detail... the harness now finds the slot by jh-detail" — investigations.js:148,150,198, app.css:1326-1328, investigations-harness.mjs:166
  • VERIFIED — "One definition now: rowsUnder is exported from util.js... both alerts.js and investigations.js import it" — util.js:502, alerts.js:27, investigations.js:29; confirmed the merged superset behavior is dead code for investigations.js's callers since both MCP reads it consumes always reply with an object, never a bare array
  • VERIFIED — "Dropped [the claimed .*6:05 assertion]; the claim-time mapping stays pinned zone-independently by claimedAt" — InvestigationsPageTests.cs:92 (comment) and :130-131 (the claimedAt assertions)
  • UNVERIFIED — the cited local test-run counts (Total: 154, Total: 6, Total: 1045, etc.) and the laptop gate runs — these are live build/test executions a read-only tree can't reproduce

Riskiest lines

  1. Lite/Investigations/InvestigationPanelReader.cs:99 — panel now always serves Full regardless of mcp.dataDetail, a deliberate parity change (Darling's web panel already did this via DarlingWebEndpoints.cs:6161, unchanged in this range) — pinned by ThePanelShowsSummaryVerdictRootCauseAndBriefInFull_WhileTheMcpHostRunsAtMetadataOnly (Lite.Tests/InvestigationPanelReaderTests.cs:111)
  2. Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/investigations.js:198 — detail slot renamed to jh-detail, which carries .jh-detail:empty { display: none; } from the Job History rules — pinned by investigations-harness.mjs:166's slotText() lookup and InvestigationsPageTests.TheRebuild_KeepsTheOpenRequest_AndCloseEmptiesThePane
  3. Darling/PerformanceMonitor.Darling.Service/wwwroot/js/util.js:502 — shared rowsUnder now used by both alerts.js and investigations.js — UNPINNED directly (no test imports/calls rowsUnder by name), but exercised transitively through both pages' existing render tests

Traps near this diff

  • "Lite and Darling ship in parity... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: this commit is the fix that brings Lite's InvestigationPanelReader into parity with Darling's pre-existing McpDataDetail.Full panel read (DarlingWebEndpoints.cs:6161, unchanged in this range), not a new one-sided change
  • "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted" — does not apply: rowsUnder had two copies with slightly different behavior (bare-array branch), and this commit collapses them to one definition in util.js rather than pinning two copies equal

Divergence (blind read vs PR body)

None — the body's "Round 1 review" section matches the diff point for point on every claim graded above.

Reviewer checklist

  • Open the Lite InvestigationsWindow by hand once (never opened by hand per the PR body) to confirm the new Alerts column renders and the Full-detail report/brief/journal actually display with real data

@erikdarlingdata
erikdarlingdata dismissed claude[bot]’s stale review October 16, 2026 04:32

Claude posting for Erik Darling. Round 1's findings were fixed in the next push and round 2 answered LGTM on the current head; this stale changes-requested verdict is dismissed so the branch rule reads the current one.

@erikdarlingdata erikdarlingdata added the merge-train In a merge train: its CI ran on the train PR label Oct 16, 2026
@erikdarlingdata
erikdarlingdata merged commit fabbc64 into dev Oct 16, 2026
29 of 33 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/5904-investigation-panel branch October 16, 2026 05:40
erikdarlingdata added a commit that referenced this pull request Oct 16, 2026
Fixes #5929. Web UI smoke: refresh the four screenshot baselines (#5929) - The four screenshot baselines were out of date after the Investigations sidebar link (#5907) and the Admin "Pending Notes" tab (#5908), so every pull request failed the check. The baselines are refreshed from a CI render.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-train In a merge train: its CI ran on the train PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant