refactor(streaming): split terminal-failure transcript evaluator (#5141) - #5272
Conversation
…quena#5141) Extract _turn_transcript_lacks_final_assistant_answer() so settlement logic can inspect an already-merged transcript. The merge wrapper now delegates to the pure evaluator without changing behavior. Fixes nesquena#5141
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "merged_len": len(list(merged_messages or [])), | ||
| "previous_len": len(list(previous_display or [])), | ||
| "msg_text": msg_text, | ||
| "source": source, | ||
| "drop_replayed_assistant": drop_replayed_assistant, | ||
| } | ||
| ) | ||
| return True | ||
|
|
||
| previous_display = [{"role": "user", "content": "hello"}] | ||
| with mock.patch.object( | ||
| streaming, | ||
| "_turn_transcript_lacks_final_assistant_answer", | ||
| side_effect=_fake_evaluator, | ||
| ): | ||
| result = streaming._merged_transcript_lacks_final_assistant_answer( | ||
| previous_display, | ||
| previous_display, | ||
| previous_display, | ||
| "hello", | ||
| source="cli", | ||
| drop_replayed_assistant=True, | ||
| ) | ||
| assert result is True | ||
| assert len(calls) == 1 | ||
| assert calls[0]["previous_len"] == 1 | ||
| assert calls[0]["msg_text"] == "hello" | ||
| assert calls[0]["source"] == "cli" | ||
| assert calls[0]["drop_replayed_assistant"] is True | ||
| assert calls[0]["merged_len"] >= 1 |
There was a problem hiding this comment.
drop_replayed_assistant=True logic
The delegation test (test_merged_wrapper_delegates_to_turn_evaluator) mocks out _turn_transcript_lacks_final_assistant_answer entirely, so it only verifies that the flag is forwarded — not that the replay-filtering branch inside the extracted function behaves correctly. The three wrapper-parity tests all call with drop_replayed_assistant=False. A case where a replayed assistant message would spuriously satisfy the "final answer" check without the filter is the exact failure mode the terminal-failure path guards against, and it has no direct coverage in this new test file. The existing test_issue5121_provider_auth_terminal_error.py suite presumably covers the regression path, but a direct case exercising the filtering logic in _turn_transcript_lacks_final_assistant_answer would close the gap opened by the extraction.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| assert calls[0]["msg_text"] == "hello" | ||
| assert calls[0]["source"] == "cli" | ||
| assert calls[0]["drop_replayed_assistant"] is True | ||
| assert calls[0]["merged_len"] >= 1 |
There was a problem hiding this comment.
merged_len — >= 1 accepts an empty merge result that happens to contain just the input message. Consider asserting the exact expected post-merge length so a regression in _merge_display_messages_after_agent_result is detected by this test rather than silently passing.
| assert calls[0]["merged_len"] >= 1 | |
| assert calls[0]["merged_len"] == len(previous_display) # merge of identical inputs yields previous_display length |
🔬 Gate certification — GREEN ✅Certified head: What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Codex (reproduce) | SAFE TO SHIP (no findings — extraction truly behavior-preserving, sentinel/_terminal_failure unaltered) |
Full pytest suite (-p no:xdist) |
11188 passed, 0 failed |
PR's tests (test_issue5141, +4) |
4 passed (wrapper matches evaluator: no-replay-filter, with-final-answer, pending-user-after-boundary, delegation) |
Sentinel/settlement-contract tests (non_auth_silent_failure, issue5121, 5224 terminal-transcript) |
17 passed — the error:"" silent-failure sentinel convention is preserved |
Findings (clean)
- ✅ Behavior-preserving extraction —
_merged_transcript_lacks_final_assistant_answeris now a thin wrapper that builds the merged transcript then delegates to the new_turn_transcript_lacks_final_assistant_answer(which operates on an already-merged transcript). Codex confirmed the wrapper produces identical results across every branch, the merge boundary is correct (wrapper merges before delegating; evaluator doesn't double-merge or skip), and theerror-sentinel /_terminal_failuredecision is unchanged. - ✅ Sentinel contract intact — this is the settlement path where
result['error']is a 3-state sentinel (None=success,""=silent-failure→apperror/no_response, non-empty=explicit failure). The 17 sentinel/settlement tests (incl.test_non_auth_silent_failure,test_issue5121, the Bug: Terminal stream failures can hide an existing chat transcript #5224 terminal-transcript preservation) all pass — the refactor doesn't touch the empty-string-sentinel handling. - ✅ No Bug: Terminal stream failures can hide an existing chat transcript #5224/Hide process wakeup prompts from transcripts #5245/Some provider failures bypass WebUI chat error surfacing; confirmed with HTTP 401 #5121 regression. Fresh-based on current master.
Concept: 4/5 — a maintainability refactor (#5141) that makes the terminal-failure evaluator reusable/testable without behavior change; reduces future-bug surface in a notoriously subtle area (good).
Recommendation to the next agent
Ready to merge — cert fresh for sha:c2bb4c65. Behavior-preserving settlement-path refactor; Codex SAFE, full suite green, 4 new evaluator tests + 17 sentinel/settlement-contract tests pass (the error:"" silent-failure sentinel preserved — the exact tripwire in this area). No regression to #5224/#5245/#5121. Concept 4/5. Cert valid only at sha:c2bb4c65.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close. Cert valid only at sha:c2bb4c65.
nesquena-hermes
added
gate-pass
nesquena-hermes
closed this pull request by merging all changes
into
nesquena:master
in
8d5ff4c
|
Shipped in v0.51.765 (Wave 1 batch, via release PR #5277). Thanks @nankingjing! Combined-stage Codex SAFE + full pytest suite green + ESLint/ruff gates clean. 🎉 |
2 participants
Summary
_turn_transcript_lacks_final_assistant_answer()to evaluate an already-merged transcript (pending-user materialization, replayed-assistant filtering, final-answer check)._merged_transcript_lacks_final_assistant_answer()as a thin merge → evaluate wrapper so#5129/#5121settlement semantics stay unchanged.Fixes #5141
Why
The terminal-failure path still needs its own merge inputs (
_all_result_messages, replay filtering), but the evaluation logic is now reusable without repeating the full merge body.Test plan
tests/test_issue5141_terminal_failure_transcript_evaluator.py(4 cases: wrapper parity, final-answer path, pending-user materialization, delegation)#5121regressions (tests/test_issue5121_provider_auth_terminal_error.py)