feat: Add live scan session details - #403
Conversation
ianw-oai
changed the title
ting-hong-shieh
left a comment
ting-hong-shieh
left a comment
There was a problem hiding this comment.
I found one source-attribution bug in the Details filter.
Validation snapshot:
- Exact PR head:
b5a30d0ab865167b23dee0f375417ec2f2b2e034 - Latest
main:216212b70a421b0bf30319d241de47a64050ec0f; the no-commit merge is clean. - Existing focused suites pass: 52/52 tests on the exact head and 59/59 on the latest-main merge. The focused API callback test, TypeScript checking, formatting, build, and whitespace checks also pass on both trees.
validation/codex_security_403_independent_worker_order.test.tssends a parentless independent Deep worker event before the main scan event, then selectsm. On both unmodified trees, the expected main line is absent and the frame instead containsmain · assistant: Independent worker.- A local minimal check that propagates the tracker's known main thread ID makes the reproduction pass while preserving the 59-test focused suite, API test, TypeScript, formatting, and whitespace results.
The reproduction is in-process. It made no model or provider request, used no credentials, and did not scan an external target.
| this.#view === "details" && this.#scrollOffset !== 0 | ||
| ? this.#activityLines(this.#width()).length | ||
| : 0; | ||
| this.#mainThreadId ??= session.parentThreadId ?? session.threadId; |
There was a problem hiding this comment.
Do not infer the main scan from the first event
Independent Deep workers selected from the scan directory can have parentThreadId: null, and sessionFiles() uses filesystem enumeration order. If one of those workers is emitted before the main session, this assignment permanently records the worker as #mainThreadId; selecting m then shows the worker while the real main scan is labeled worker 1.
At exact head b5a30d0a, and again after a clean merge onto main 216212b7, the same two-event fixture expected main · assistant: Main scan. but rendered main · assistant: Independent worker. Propagating the tracker’s known scan thread ID makes the case pass without changing the existing focused results. Please identify the main session explicitly and add an independent-worker-first regression.
mldangelo-oai
left a comment
mldangelo-oai
left a comment
There was a problem hiding this comment.
Nice addition. I tested the current head with the full randomized suite (1,066 passed, 11 skipped), focused dashboard and cost tests, the API callback test, type checks, formatting, and a production build.
I also tried a smaller implementation that uses the cost tracker's existing worker numbers. It removes two dashboard state fields and seven production lines, fixes the independent-worker ordering bug, and keeps the full suite green. My inline notes cover that simplification, startup event loss, avoidable historical-session memory use, inaccessible Details subscriptions, event-ordering overhead, and one regression-test gap.
Leaving this as a comment-only review. Thanks for putting this together.
| readonly #startedAt: number; | ||
| readonly #activities: TimedScanActivity[] = []; | ||
| readonly #details: (ScanSessionEvent & { recordedAt: number })[] = []; | ||
| readonly #workers = new Map<string, number>(); |
There was a problem hiding this comment.
Could we use the worker number ScanCostTracker already assigns instead of keeping a second #workers map and inferring #mainThreadId here? I tried an optional worker?: number on ScanSessionEvent and passed through the tracker's existing number. That removed both dashboard fields and seven production lines, fixed independent workers arriving before the main scan, and still passed the full randomized test suite. It also keeps worker labels consistent between Activity and Details.
| const index = this.#details.findLastIndex( | ||
| (event) => event.recordedAt <= entry.recordedAt, | ||
| ); | ||
| this.#details.splice(index + 1, 0, entry); |
There was a problem hiding this comment.
Could this just append events and defer sorting until the Details view is opened or otherwise needs an updated batch? findLastIndex plus splice scans and shifts the array for every older event, including when the user stays in Activity. In a synthetic benchmark, 12,000 chronological events took 3.1 ms to record; the same events in reverse timestamp order took 395.56 ms. Worker transcripts are read one file at a time, so timestamps from another session can naturally arrive out of order. Deferring the sort should also remove some insertion and cache-invalidation bookkeeping.
| prose: new Set(), | ||
| reasoning: null, | ||
| reasoningCount: 0, | ||
| ...(this.#options.onSessionEvent === undefined ? {} : { events: [] }), |
There was a problem hiding this comment.
One memory concern here: adding onSessionEvent creates an event queue for every saved session before checking whether it belongs to the current scan. With 32 unrelated saved sessions containing 1 MiB of synthetic output each, I measured a 40.13 MiB peak heap versus 8.37 MiB with the observer disabled. Could we classify sessions before retaining raw events, or read and forward only transcripts associated with this scan?
| usage = addTokenUsage(usage, session.usage); | ||
| } | ||
| if (session.threadId === null || !included.has(session.threadId)) { | ||
| session.events?.splice(0); |
There was a problem hiding this comment.
This clears a session's events before a later refresh has a chance to connect it to the current scan. I reproduced missing first messages in two cases: a grandchild worker appears before its parent's log, and an independent worker appears before the main session metadata. Removing the clear makes both cases pass, but it also keeps unrelated history resident: the same synthetic fixture stayed at 40.14 MiB after refresh. Replaying a newly associated session from its saved transcript, or retaining only sessions with unresolved relationships, could preserve those early messages without collecting every old session.
| dashboard.setStage("inspecting repository files"); | ||
| } | ||
| }, | ||
| onSessionEvent: dashboard?.recordDetails.bind(dashboard), |
There was a problem hiding this comment.
Could we skip onSessionEvent when process.stdin.isTTY is false? The dashboard is enabled from stderr's TTY state, while the Details keyboard listener is installed only for TTY stdin. I reproduced the redirected-stdin case: the callback is still present even though the user cannot open Details. Checking stdin here would preserve the regular dashboard while avoiding unnecessary transcript parsing and retention.
| onSessionEvent: (event) => events.push(event), | ||
| }); | ||
| tracker.start("scan-thread"); | ||
| await waitFor(() => events.length === 4); |
There was a problem hiding this comment.
Would you be open to adding a refresh where a grandchild worker already has output but its parent's session file does not exist yet, then creating the parent and checking that the original output is delivered? A second case with an independent worker appearing before the main session metadata would cover the same startup gap. This fixture currently creates all related sessions before tracker.start, so neither missing-history case is exercised.
4 participants
Why
The scan dashboard only showed selected activity. Users could not inspect the prompts, reasoning, tool calls, outputs, and worker events already saved in Codex session logs while a scan was running.
What changed
onSessionEventSDK callback. Exclude unrelated scans and inherited parent history, and keep observer failures from stopping a scan.dto toggle it, then usea,m, or1-9to show all sessions, the main session, or one worker.Verification
pnpm run typespnpm run formatpnpm run build