Hide process wakeup prompts from transcripts - #5245
santastabber wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| api/models.py | Adds shared is_hidden_process_wakeup_message / filter_hidden_internal_messages predicates; gates _append_recovered_pending_turn against process_wakeup sources; adds _source to the column selection and a Python-loop counting path in get_state_db_session_summary so state.db metadata correctly excludes hidden rows. |
| api/routes.py | Imports shared helpers; gates eager checkpointing against process_wakeup; adds full-sidecar scan in _metadata_only_message_summary to correct inflated counts; filters hidden rows in _message_summary and before _message_window_for_display. Sidecar text scan has a false-positive risk on literal user text. |
| api/streaming.py | Filters wakeup rows throughout _merge_display_messages_after_agent_result; adds a dedicated process_wakeup branch in _merged_transcript_lacks_final_assistant_answer. The tail-slice offset len(previous_visible) is inconsistent with the _partial deduplication that the merge function applies, which can cause settlement to misreport a missing answer. |
| tests/test_process_wakeup_synthetic.py | Inverts or renames pre-existing tests to match the new hide-not-stamp contract; adds thorough new regressions for renderable-count, filter, windowing, settlement, replay-drop, and backfill suppression. |
| tests/test_webui_state_db_reconciliation.py | Adds _source column to test schema helpers and two new metadata-only summary tests covering state.db and sidecar legacy-wakeup-row scenarios. |
Reviews (3): Last reviewed commit: "Hide process wakeup prompts from transcr..." | Re-trigger Greptile
|
Reading the diff at HEAD ( I want to confirm Greptile's P1 because I think it's a genuine, persistent divergence (not just a transient poll artifact), and ground it in the code. Code referenceThe full-load summary now filters: # api/routes.py:7007-7008
def _message_summary(messages) -> dict:
messages = _filter_hidden_internal_messages(messages)But the metadata-only path derives its count from stored JSON, not from a filtered message list: # api/routes.py:7037-7040 (_metadata_only_message_summary)
sidecar_count = _numeric_count(getattr(sidecar_session, "_metadata_message_count", None))
if sidecar_count <= 0:
sidecar_count = _numeric_count(sidecar_session.compact().get("message_count"))And the stored count is written from the raw, unpruned message list: # api/models.py:882
meta['message_count'] = len(self.messages or [])DiagnosisThe key point:
On RecommendationTwo viable approaches:
I'd lean toward option 2 — it keeps a single source of truth for the count and avoids teaching the cheap metadata path about hidden rows. Test planA regression for the metadata path would catch this: build a session whose stored |
dfe6dd5 to
a525c3b
Compare
|
Thanks — addressed in the updated head What changed:
Verification:
|
|
Gated this fresh (Codex reproduce + full suite) — the display-hiding is on the right track, but it introduces a CORE settlement regression I reproduced directly, so one fix is needed before it ships. The bug (reproduced in-process): hiding the Direct repro against your branch: _merged_transcript_lacks_final_assistant_answer(
[], [], [{"role":"assistant","content":"done"}], "wakeup prompt", source="process_wakeup"
) # returns True -> should be False (a kept assistant answer IS a final answer)Fix-spec (Codex, verified):
Note this is exactly the streaming-settlement-contract area where the 3-state |
🔬 Gate certification — RED ⛔ (wakeup-hiding mostly right, but 2 reproduced streaming-settlement edge bugs)Certified head: What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Codex (reproduce) | SHIP ONLY WITH FIXES — 1 CORE (success→apperror) + 1 SILENT (tail-backfill re-leak), both verified |
Full pytest suite (-p no:xdist, rebased) |
11155 passed, 0 failed (green — but no test covers the assistant-only-wakeup-completion settlement or the tail-leak; that's why they slip) |
| PR's tests + reconciliation | 56 passed |
| error-sentinel/settlement-contract tests (issue5121 / non_auth_silent_failure) | 12 passed — change does NOT disturb the 3-state error sentinel ✓ |
⛔ Blocking #1 (CORE, Codex-reproduced + I confirmed in source) — successful wakeup turn settled as a terminal FAILURE
_merged_transcript_lacks_final_assistant_answer() (streaming.py:~5162) unconditionally builds a pending_user row and appends it to merged_messages (pending_user['_source']=source, ~lines 23-29). For a hidden process_wakeup turn, the synthetic user row was correctly removed by the merge, so there's no current-user boundary — and this re-appends the hidden wakeup prompt after the real assistant reply. Then _session_lacks_final_assistant_answer() sees the transcript ending in a user row → _terminal_failure (streaming.py:8143) → the settlement path can emit an apperror despite a real assistant reply. Codex verified the helper returns True for an assistant-only process-wakeup completion. (I confirmed the unconditional append in source.)
Fix (Codex): in _merged_transcript_lacks_final_assistant_answer(), when _is_process_wakeup_source(source) and there's no visible current user row, do NOT append pending_user — evaluate the merged transcript as-is (e.g. return _session_lacks_final_assistant_answer(merged_messages)). Completed process-wakeup turns must not trip _terminal_failure. Add a regression for an assistant-only wakeup completion.
⛔ Blocking #2 (SILENT, Codex-reproduced) — legacy hidden wakeup row re-persisted via the tail-backfill loop
The new not is_hidden_process_wakeup_message(_cmsg) guard was added to two context-backfill branches but NOT the final tail-backfill loop (streaming.py:~4927). So a legacy hidden wakeup row that is the final context-only tail row gets appended into merged → persisted back into s.messages (re-leaks into the durable visible transcript). Codex verified with a direct _merge_display_messages_after_agent_result() call.
Fix (Codex): add and not is_hidden_process_wakeup_message(_cmsg) to the condition at streaming.py:~4927; add a regression where the hidden row is the final context-only tail.
✅ Right (keep it)
- Identification (
role==user && _source==process_wakeup) is precise — a real user turn is never mis-hidden. Filtering applied at the merge, candidate-dedup, 2 backfill branches, pending-turn materialization guard, message-window/count exclusion, and legacy state.db rows. Does NOT touch theerror3-state sentinel (verified). 8 new tests + reconciliation tests pass.
Recommendation to the next agent
Bounce for the 2 streaming-settlement fixes (both Codex-reproduced, precise), then re-gate. The wakeup-hiding concept + identification + the bulk of the filtering are correct, and it correctly leaves the settlement error sentinel alone — but (#1) a successful wakeup turn can be mis-settled as an apperror (re-appended pending row trips _terminal_failure), and (#2) the tail-backfill loop misses the hidden-row guard so a legacy row can re-leak. Both fixes are one-liners + a regression each. crit=3 transcript/settlement correctness → fix before merge. Cert valid only at sha:a525c3b2feca.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close. Cert valid only at sha:a525c3b2feca; a new push invalidates it → re-gate.
|
Addressed the process-wakeup settlement regression in the updated head. What changed:
Verification:
|
a525c3b to
1f0c0ca
Compare
|
Follow-up pushed for the second gate finding as well. What changed:
This is in addition to the settlement fix from the previous push. Verification:
|
1f0c0ca to
1dfb762
Compare
🔬 Gate certification — RED ⛔ (converged on my 2 prior blockers, but a new defect + a structural rebase remain)Certified head: What I ran (worktree on the PR head
|
| Gate | Result |
|---|---|
| Prior-blocker verification (mine, in-process) | ✅ BOTH fixed (see below) |
| Codex (reproduce) | SHIP-WITH-FIXES — 1 reproduced SILENT (provider leak, confirmed by me) + 1 CORE + 1 SILENT (see below) |
| Full pytest suite | 11161 passed, 0 failed |
| process_wakeup + reconciliation tests | ✅ 64/64 |
| error-sentinel / issue5121 contract | ✅ 46/46 (the error:"" 3-state sentinel is untouched) |
Findings
✅ CONVERGED — both my prior blockers are fixed (verified):
- (was CORE) apperror-on-success: the
_is_process_wakeup_source(source)branch now returns early (return _session_lacks_final_assistant_answer(tail_messages)) before thepending_userappend, so a successful wakeup turn no longer re-appends the hidden row → no false_terminal_failure. Verified in-process (lacks_final=True correctly for a no-fresh-answer wakeup). - (was SILENT) tail-backfill:
is_hidden_process_wakeup_messageguard is now present at all three backfill sites (streaming.py:4893/4914/4927). Verified.
The error-sentinel contract (issue5121) is intact (46/46). Nice work closing both.
⛔ SILENT (I REPRODUCED) — hidden wakeup rows are still sent to the provider (api/streaming.py _sanitize_messages_for_api): the row is hidden from display but _sanitize_messages_for_api() strips _source and never DROPS is_hidden_process_wakeup_message(msg), so a legacy hidden {'role':'user','content':'[IMPORTANT: hidden wakeup ping]','_source':'process_wakeup'} row is passed to the model as normal user history. Confirmed by direct call — the hidden row survives in the sanitized output. Fix: drop is_hidden_process_wakeup_message(msg) rows in _sanitize_messages_for_api() before sanitizing; add a regression covering legacy hidden sidecar/state.db rows. (The point of hiding the wakeup prompt is that the model shouldn't see it either.)
- CORE (5168): the wakeup tail slice
merged_messages[len(previous_visible):]may include a backfilled older context-only assistant row if_merge_display_messages_after_agent_resultbackfills before that boundary → could settle success without a fresh answer. My repro of the simplest case returned the correctTrue, so I could not confirm it as stated — but the tail-boundary math is worth hardening (compute the tail after the same backfill boundary / exclude pre-existingprevious_contextrows). - SILENT (9852):
cancel_stream()recovery path may still persist the hidden wakeup prompt as a user row (bypasses the new skip guards). Verify when re-integrating.
⛔ BLOCKED — structural rebase onto current master (this is the contributor's, not a self-rebase): master's #5272 refactor (which I gate-certified GREEN earlier) split the evaluator into _turn_transcript_lacks_final_assistant_answer(merged_messages, previous_display, msg_text, ...) [receives merged_messages PRE-merged] + a _merged_transcript_... wrapper. This PR was written against the OLD single function — its process-wakeup branch calls _merge_display_messages_after_agent_result(previous_display, previous_context, result_messages, ...) using previous_context/result_messages which are no longer parameters of the extracted function. A git apply onto master conflicts in api/streaming.py. I did NOT self-resolve — placing the wakeup branch in the wrong layer risks a subtle settlement bug on a path that already had edge defects. The contributor should re-integrate the process-wakeup logic into the post-#5272 two-function shape (the wakeup-aware merge + tail-filter belongs in the wrapper that still has those vars).
Recommendation to the next agent
RED — gate-fail/changes-requested. Credit that both original blockers are genuinely fixed + the sentinel contract holds. Remaining: (1) drop hidden wakeup rows in _sanitize_messages_for_api (reproduced provider leak) + regression; (2) re-integrate onto master's #5272-refactored _turn_/_merged_ structure (the structural rebase); (3) while there, harden the tail-boundary (Codex CORE) and the cancel-path persistence (Codex SILENT). Converges fast — the hard settlement logic is mostly right. Author @santastabber. crit=3.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy. Gated on the PR's own head (1dfb7622) because it structurally collides with post-#5272 master; the rebase is the contributor's (not a self-resolvable CHANGELOG/i18n rebase).
Summary
process_wakeupuser rows from display windows, message summaries, and renderable-message countsTests
git diff --check origin/master...HEADpython3 -m py_compile api/models.py api/routes.py api/streaming.py./scripts/test.sh tests/test_process_wakeup_synthetic.py -qtrufflehog filesystem --no-update --fail --only-verified api/models.py api/routes.py api/streaming.py tests/test_process_wakeup_synthetic.pygitleaks detect --no-git --redact --verbose --source /tmp/webui-process-wakeup-committed.diff --report-path /tmp/webui-process-wakeup-gitleaks-committed.json --report-format json