fix(tui): keep internal runtime events out of the session peek - #5376
fix(tui): keep internal runtime events out of the session peek#5376Lstarsky0 wants to merge 3 commits into
Conversation
Runtime handoffs are persisted with `role = "user"` because strict chat templates reject anything else mid-conversation. `runtime_handoff` says so in its module doc and owns both the envelope and its recognition "so creation and recognition cannot drift" — the TUI restore path uses that recognition, `build_peek` never did, so the web dashboard rendered `waiting_for_subagents` and `background_shell_completion` records as rows labelled USER, contradicting the `visibility="internal"` contract the records carry in their own text. Recognition goes where the module says it belongs rather than becoming a fourth copy in the peek. `is_internal_runtime_handoff` matches the structural shape `runtime_handoff_message_with_meta` builds — two text blocks, no cache markers, a runtime provenance line in the trailing envelope — so someone who pastes an envelope while asking about it submits one block and stays visible. That is the same discriminator already keeping the restore projection off user-authored lookalikes. It covers `shell_completion` provenance too, which `raw_runtime_handoff_text` does not: that one answers the narrower question of what the projection knows how to rewrite. The provenance whitelist stays exact — the two kinds this module emits — rather than filtering everything non-authoritative, since `imported_transcript` and `memory_recall` are content a peek should show. The filter runs before the tail is taken. Filtering afterwards would spend the twelve-entry budget on rows nobody sees, which in a session with busy sub-agents is most of the pane. `message_count` counts the same conversation the entries come from, so the dashboard's "N messages" and "M earlier messages not shown" still account for what it rendered. TUI history is left alone. It renders shell completions as User cells and `apply_loaded_session_never_restores_background_shell_event_as_composer_draft` pins that; whether the terminal should also hide them is a separate question from what the web pane shows. Both directions are tested, per the issue: every handoff shape that can reach a saved session is absent, and real user messages — including one quoting an envelope — survive. Closes Hmbown#5375 Analysis drafted with local tooling; every change and test verified by hand.
|
Thanks @Lstarsky0 for taking the time to contribute. This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered. Please read |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0201aec534
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| text: turn_meta, | ||
| cache_control: meta_cache, | ||
| }, | ||
| ] = message.content.as_slice() |
There was a problem hiding this comment.
When an idle sub-agent completion payload contains an [Attached image: …] reference, handle_idle_subagent_completion routes it through user_content_blocks, which inserts image/tag blocks or an attachment-notice block before the trailing turn_meta. This exact two-block destructure then returns false, so the peek again exposes the raw internal runtime envelope and lets it consume the entry budget. Recognize the provenance block independently of these attachment-expansion blocks while still validating the runtime envelope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and I had the construction path wrong. Fixed in 591440825.
I assumed every handoff came from runtime_handoff_message_with_meta, which does build exactly two blocks. handle_idle_subagent_completion doesn't use it — it takes only subagent_completion_runtime_text / subagent_failure_runtime_text and sends that through handle_send_message with SubAgentHandoff provenance, so the message is assembled by user_content_blocks like any other turn.
Reproduced before changing anything, with a real waiting_for_subagents envelope and the blocks that path inserts:
recognized=false
PEEK User hi
PEEK User <codewhale:runtime_event kind="waiting_for_subagents" visibility="inte…
Worth adding to your description: it isn't only the success case. When attach_image_from_path fails, notice_block still pushes a block, so a payload that merely mentions an attachment breaks the pair whether or not the file loads.
The predicate now anchors on the first and last block with [Text, .., Text]. I did not add an envelope-text check alongside it — recognition keying on payload text is what the module deliberately avoids, and the provenance line already does the separating: a composer turn is ExternalUser, whose authority is implicit, so turn_metadata omits the provenance line for it entirely. Nothing a person sends can carry subagent_handoff or shell_completion, at any block count. The paste test now covers the multi-block shape too, not just the single-block one.
Verified rather than assumed: reverting the .. back to an exact pair makes the new test fail with the envelope showing up as a peek entry, and restoring it passes. Full codewhale-tui --lib is two rounds at 10 failures, the same set main fails on CI plus one local tools::pdf — no change from this branch.
Same hole exists in raw_runtime_handoff_text, which the restore projection uses, so an idle completion with an attachment also replays raw into a resumed thread. Left alone here: that one gates a rewrite rather than a hide, and widening it is a separate change with its own blast radius. Happy to send it if you want it.
|
The macOS and Windows legs here are red, and they aren't mine — filed as #5377. Short version: Worth knowing for reading the checks list here: Nothing in this diff reaches those tests — it touches Happy to rebase once #5377 lands, or if you'd rather see this one green first I can send the test re-pin as its own PR and this can go on top. |
The exact-pair destructure was wrong about how these messages are built. Idle sub-agent completions do not go through `runtime_handoff_message_with_meta` at all: `handle_idle_subagent_completion` takes only the envelope *text* and sends it through the engine's ordinary path, where `user_content_blocks` expands any `[Attached image: …]` line in the payload into image blocks — or a notice block when the file fails to load, so the block count grows either way. Between the envelope and its provenance marker, that leaves a message the predicate did not match, and the peek rendered the raw envelope as the person's own message and spent an entry on it. Anchor on the first and last block instead. The provenance line is what actually separates runtime traffic from a person: a composer turn is `ExternalUser`, whose authority is implicit, so `turn_metadata` omits the provenance line entirely for it. Nothing a person sends is matched by this regardless of how many blocks it has, which the paste test now covers in the multi-block shape as well as the single-block one. Found by the Codex reviewer on Hmbown#5376. Analysis drafted with local tooling; every change and test verified by hand.
Analysis drafted with local tooling; every change and test verified by hand.
0201aec to
9a82ca4
Compare
1 participant
Closes #5375
Repro first, since it turned up something the issue doesn't mention. Building a session from the real constructors and running it through both paths:
Two separate facts. The peek doesn't use the restore projection at all, which is the reported bug. And
background_shell_completionisn't recognised by the projection either —is_subagent_handoff_turn_metamatches onlysubagent_handoffprovenance, and the shell handoff carriesshell_completion.The fix
runtime_handoff's module doc says it owns the envelope and its recognition "so creation and recognition cannot drift". The TUI restore path uses that;build_peekgrew up beside it readingsession.messagesraw. So recognition goes in that module rather than becoming a fourth copy in the peek:Structural, matching what
runtime_handoff_message_with_metabuilds — two text blocks, no cache markers, a runtime provenance line in the trailing envelope. It coversshell_completionas well;raw_runtime_handoff_textstays as it is, because it answers the narrower question of what the projection knows how to rewrite, and I'd rather not widen a function whose callers depend on that.The provenance list stays exact — the two kinds this module emits — rather than "anything non-authoritative".
imported_transcriptandmemory_recallare also non-authoritative and are content a peek should show.Filtering runs before the tail is taken. Afterwards it would spend the twelve-entry budget on rows nobody sees, which in a session with busy sub-agents is most of the pane.
message_countcounts the same conversation the entries come from, soomitted_before + entries.len() == message_countstill holds and the dashboard's two numbers still account for the rows it drew.Deliberately not touched
TUI history renders shell completions as
HistoryCell::User, andapply_loaded_session_never_restores_background_shell_event_as_composer_draftpins it — that test's concern was the composer, not the transcript. Whether the terminal should hide them too is a real question, but it's a different one from what the web pane shows, and I don't want to silently reinterpret a passing test's intent inside a web fix. Happy to open it separately if you want it.Also not touched: the full-transcript
GET /v1/sessions/{id}response. The dashboard always asks?peek=true, and a debug surface returning everything is the "opt-in, clearly labeled" case the issue allows. Say the word if you'd rather it carried explicit provenance too.Tests
Both directions the issue asks for, plus the accounting:
internal_runtime_events_are_absent_from_a_peek— every handoff shape that can reach a saved session, including the restore checkpoints a post-resume save persistsreal_user_messages_survive_the_runtime_filtera_person_who_pastes_a_runtime_envelope_is_still_the_person_talking— one block, so it stays visible; the filter keys on shape, never on the envelope textruntime_traffic_does_not_spend_the_entry_budgetthe_counters_describe_what_the_pane_can_showVerification
cargo fmt --checkclean. Workspace clippy with CI's exact allow-list: exit 0.Full
codewhale-tui --lib, same machine, two rounds on the branch plus a baseline on cleanmain:maina9acd63The failing set is byte-identical in all three — nine
reasoning/thinking-ladder tests plus thetools::pdftiming flake. Not mine, and worth flagging separately: they aren't the parallel-load family from #5355.settings::reasoning_effort_setting_normalizes_and_clearsfails deterministically withleft: Some("xhigh"),right: Some("max"), which reads like the thinking-ladder change and its tests disagreeing rather than a race. I'll dig into that on its own rather than bundle it here.Budget: measured, not carried over —
mainis at 693110 and this branch at 693289, so +179 owned Rust lines, mostly tests. Acknowledged in a separate commit, and re-measured after the rebase rather than reusing the pre-rebase number (upstream shrank 6 lines underneath it). The new ceiling is the exact measurement, not the old one plus a delta.Analysis drafted with local tooling; every change and test verified by hand.