fix: harden recording, rendering & subagents — verified deep-review findings#488
Merged
Conversation
… the post-merge deep review A 50-finding deep review landed against the just-merged rendering rewrite (PR #442). Every finding was independently re-verified against the real code by a fan-out of agents before any fix — that killed 6 false positives (including two the review rated High: C1 "background-Bash notification dropped", which no longer exists post-cutover, and #17 Codex MCP orchestration, which is a separate mechanism). This lands the confirmed, low-risk fixes. Recording (data-loss / security — all dev-debug-gated): - #1 sanitize renderer sessionId before it becomes a recordingId path segment (path-traversal escape of SESSION_RECORDING_DIR) - #2 protect in-memory live recordings from retention pruning (live-dir provider) - #3 flush()/close() now quiesce an in-flight drain so the session:exit tail + trailing notes can't be lost or rescheduled after close - #4 serialize meta.json writes so a fire-and-forget write can't clobber final stats - #5 notes written after the size cap now reach disk (they bypass the cap by design) - #6 cap recording-note IPC text; C8 record dropped-event count at the cap; C10 note-id collisions Rendering correctness: - #7 (High) thinking/reasoning-only turns no longer vanish — collector reads block.thinking/reasoningSummary/reasoningText, not just text - #8 classify Codex web_search/image_generation/local_shell/tool_search as tool-use - #9 emit 'empty' AFTER ownership rejection so an all-rejected feed isn't blank - #20 suppress a committed task-notification joined to a spawn tool_use (it renders inside TaskSubagentRow) — fixes the duplicate subagent rows the corpus captured - #21 lowercase-bash git-widget result suppression; C2 suppress the double AskUserQuestion answer - #23 suppress Codex orchestration_create_agent result JSON below the task card - #24 headline unknown Codex tool rows on arguments/raw; #25 greedy opencode <content> parse - #22 defer JsonResultSlab CodeBlock highlight until first expand - C6 dedupe semantic turn rows by turnId (not just consecutive) in the view bridge - C3 pruned Task card reads terminal state from the committed tool_result; C11 'other' glyph Subagents / replay / scripts: - #16 tombstone pruned subagents so rescan can't re-tail their transcript from byte 0 - #26/#27/#28 block-grain replay coverage + collector-decision threading + unrenderable-drop invariant - #15 triage script gates skew-ingestion-lag on the timestamp window - #34/#35/#13/#14 extract-script CLI validation, mid-stream-corruption warnings, failure exit codes - #11/#31/#C7/#C9 comment/doc corrections Deliberately NOT included (need more design, tracked for follow-up): - #10 session-global text ownership — the proposed gate broke Codex's current-turn exact-text dedup (ledger #170: double-render across the resp_*/rollout id split). Reverted; needs codex/claude-aware timestamp scoping. - C4 broadening prune to stale children changed the prune trigger and broke the fold-buffering test. Kept #16's tombstone; dropped C4. Dismissed false positives: #12, #17, #19, #29, C1, C13. Accepted existing debt (no change): #30 (untriaged corpus), C12 (queue remove), C5 (tests). One corpus fixture updated (#20 suppression) with a real 'fixed' verdict, not a blind bless. Gates: tsc node+web clean; unit 435, renderer 16, integration 30, bundle corpus 48 — all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the confirmed defects from the 50-finding deep review of the just-merged rendering rewrite (#442). Every finding was independently re-verified against the real code by a fan-out of agents before any fix — that killed 6 false positives, including two the review rated High (C1 "background-Bash notification dropped" — no longer exists post-cutover; #17 Codex MCP orchestration — a separate mechanism).
Recording (data-loss / security — all dev-debug-gated)
#1sessionId path-traversal sanitize ·#2protect live recordings from retention prune ·#3flush/close quiesce (no lost tail) ·#4serialize meta writes ·#5notes reach disk after cap ·#6/C8/C10note cap, dropped-count, id collisionsRendering correctness
#7(High) thinking/reasoning-only turns no longer vanish ·#8classify Codex server-tool kinds ·#9emitemptyafter ownership rejection ·#20suppress joined task-notification duplicate (the "horrible subagents UI" bundle) ·#21/C2git-widget + AskUserQuestion double-render ·#23Codex orchestration result ·#24/#25/#22row polish ·C6turn-dedupe in the view bridge ·C3/C11Task card terminal stateSubagents / replay / scripts
#16tombstone pruned subagents (kills per-tick transcript re-tail) ·#26/#27/#28block-grain replay coverage ·#15triage timestamp gate ·#34/#35/#13/#14extract-script validation & failure signalling · doc fixes#11/#31/C7/C9Deliberately deferred (need more design — tracked as follow-up)
#170double-render). Reverted — needs codex/claude-aware timestamp scoping.Verification
One corpus fixture updated (#20 suppression) with a real
fixedverdict — not a blind bless. Gates: tsc node+web clean · unit 435 · renderer 16 · integration 30 · bundle corpus 48 — all green.🤖 Generated with Claude Code