fix(codex): skip native activity rows in handoff extraction - #545
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves Codex handoff message extraction by structurally detecting and skipping Codex’s native TUI “activity” rows (bullet status lines and their optional tree continuations) so that only the actual model reply is returned, without relying on an English-verb allowlist.
Changes:
- Added
_find_response_marker()to locate the first real reply marker after a compact, blank-separated native activity prelude while still skipping MCP tool-call markers. - Updated
CodexProvider.extract_last_message_from_script()to use_find_response_marker()on the primary “last user message” extraction path. - Added unit tests covering compact activity preludes, activity with tree-continuation lines, and preserving legitimate blank-separated reply bullets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/cli_agent_orchestrator/providers/codex.py | Adds structural detection for native activity preludes and uses it to anchor extraction at the actual reply marker. |
| test/providers/test_codex_provider_unit.py | Adds regression tests ensuring activity rows are skipped while legitimate reply bullets remain preserved. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #545 +/- ##
=======================================
Coverage ? 90.99%
=======================================
Files ? 179
Lines ? 23302
Branches ? 0
=======================================
Hits ? 21203
Misses ? 2099
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@w3lld1 thanks for your contribution. the PR looks good except that we needs some more test coverage based on the comments. Can you please help to address it ? |
|
Thanks — added focused coverage for the response-marker edge cases in
Fresh validation on the pushed commit:
|
|
Thanks @w3lld1 for adding the requested coverage. I confirmed the latest commit now has 100% patch coverage, and the focused suite, formatting, and mypy checks pass. There is still one behavioral P2 blocker in The helper returns the first bullet after either two preceding bullet-only rows or any preceding First, normal Codex activity still leaks when multiple activity cells are blank-separated, which is the layout used by upstream Codex snapshots: PR #545 extracts from Second, the same heuristic introduces answer truncation. This valid response: is reduced to only Please identify complete native activity cells and choose the boundary after the last relevant activity cell, rather than returning at the first ambiguous blank-line boundary. If a compact bullet group cannot be distinguished from a real answer using retained structure, preserving it is safer than silently deleting answer content. Please add tests for multiple blank-separated activity cells, interleaved model commentary/activity, two consecutive legitimate answer bullets, and a tree-formatted legitimate answer. |
|
Thanks — addressed the behavioral blocker in The response boundary now advances only after at least two complete native activity cells ( Added coverage for:
Fresh validation on the pushed commit:
|
call-me-ram
left a comment
There was a problem hiding this comment.
Thanks for taking #541, and welcome — this is a well-constructed first PR. The structural approach is exactly the discipline the issue asked for: no verb allowlist, the • Called attention to the flaky test… guard case survives (I verified), MCP-marker handling is untouched, and the tree-continuation form is genuinely fixed — I ran the two-cell case at this head and got the clean '• The bug is in the poll loop.'. The test coverage of the preserve-side cases is thoughtful.
Requesting changes on one thing, and it's about claims rather than code quality:
Must-fix — the PR says it fixes what the code deliberately preserves. The summary's first bullet says "I detect compact, blank-separated Codex activity rows by their layout," and the body says "Fixes #541." But the compact-row case — the ❌ row in #541's own repro table — still reproduces byte-for-byte at this head:
› fix the failing test\n\n• Explored src/providers\n• Ran pytest -q\n\n• The bug is in the poll loop.
→ '• Explored src/providers\n• Ran pytest -q\n\n• The bug is in the poll loop.' (noise still leaks)
And that's not an oversight — your own docstring and test_extract_preserves_ambiguous_compact_bullet_group pin the preservation as intended, on the grounds that a compact group is indistinguishable from a legitimate answer. That's a defensible engineering call (preferring noise over truncation is the right bias here, and • First finding\n• Second finding\n\n• Conclusion shows why pure layout can't split the compact case). But then the PR must say so: rewrite the summary to claim only the tree-continuation form, change "Fixes #541" to "Refs #541" so the issue stays open for the compact residual, and put the ambiguity rationale on the issue. Unless you have evidence the real TUI always renders a └ continuation under activity rows — in which case the compact case is synthetic-only, and a live capture demonstrating that would justify closing #541 outright. Either resolution is fine; a merged PR that auto-closes the issue while its primary repro still leaks is not.
Question — why two complete cells? The single-complete-cell shape is surely the most common real capture (one tool call, then the answer), and it still leaks at this head:
› quick check\n\n• Ran pytest -q\n└ 3 passed\n\n• All green.
→ '• Ran pytest -q\n└ 3 passed\n\n• All green.'
test_extract_preserves_single_tree_formatted_bullet asserts this is intended because "one tree-formatted bullet can be a legitimate answer" — but is there any real capture of a model answer rendering as • bullet + └ continuation? └ is TUI chrome for tool output; models don't normally emit it. If you have no such capture, ≥1 complete cell would fix the dominant real-world case at very low risk. If you keep ≥2, the docstring needs the why — "indistinguishable" is asserted, not argued.
Worth documenting — the trade cuts the other way at ≥2. The preserve-ambiguity principle inverts once two └-cells exist: a legitimate answer that itself contains two tree-formatted bullets (say, echoing a directory layout) followed by a closing bullet gets its first two bullets cut. Constructed case, verified at head: • src/\n└ providers/\n• test/\n└ providers/\n\n• That's the layout. → extracts only '• That's the layout.'. Rare, and I think acceptable — but it belongs in the docstring as the accepted false-positive surface, not undocumented.
Nits: the interleaved-commentary semantics (mid-turn narration dropped from final-message extraction) match main's contract — good; _find_assistant_marker is still live at three status-detection sites, so no dead code — also good, though a one-line comment noting that status detection and extraction now deliberately use different anchors would save the next reader a puzzled minute.
I've approved the CI run for this branch. Fix the claims (or extend the detection) and this lands quickly — the hard part, the shape-based discipline, is already right.
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #545 — fix(codex): skip native activity rows in handoff extraction
Summary
Third iteration of the #541 fix: _find_response_marker now advances the extraction boundary only past ≥2 complete native activity cells (a • summary plus a └ continuation), deliberately preserving ambiguous compact groups. The tree-continuation case is genuinely fixed (verified: the two-cell shape extracts exactly • The bug is fixed.), and all four maintainer-requested tests were added (174 passed, 3 skipped at this head). However, a maintainer's CHANGES_REQUESTED review is live at this head with both must-fix items still open, and our verification found a new, unreported defect: because the boundary can only land on a bullet marker, the skip never fires when the model's reply is prose — the most common reply shape — so activity cells still leak in exactly the scenario the PR targets. Recommend request changes.
Blocking (must fix before merge)
- [correctness] src/cli_agent_orchestrator/providers/codex.py:325-334 — The ≥2-complete-cells skip only works when the reply is itself a
•bullet; a prose reply defeats it entirely.complete_cellsis computed overmatches[:-1]and the returned boundary is alwaysmatches[complete_cells[-1] + 1]— an element ofmatches— so the boundary can only ever land on a bullet. Verified at this head: two complete activity cells followed by a prose (non-bullet) reply → only one cell is counted (matches[:-1]drops the last bullet), the function falls back tomatches[0], and both activity cells leak into the handoff. With three cells and a prose reply, the boundary lands on the last activity bullet, so one full• …\n └ …cell still leaks. Every new test uses a bullet-shaped reply, so the suite is green while the common prose-reply shape misbehaves. Fix: when ≥2 complete cells are found, advance to the first content after the last complete cell's tail (bullet or prose), not to the next element ofmatches; add regression tests for prose replies after 2 and 3 activity cells.
Important (should fix)
- [correctness/tests] src/cli_agent_orchestrator/providers/codex.py:326-330 — An MCP tool-call's tree output can falsely "complete" a neighboring cell. MCP
• Called server.tool(...)markers are excluded frommatches, but their└output lines land in thecell_tailof the preceding non-MCP bullet, marking it complete even though it has no continuation of its own. Two such blocks can reach the ≥2 threshold and advance the boundary past genuine reply content. The intersection of the MCP filter and cell-completeness counting is subtle and currently untested — add an explicit test pinning the intended behavior. - [consistency] src/cli_agent_orchestrator/providers/codex.py:823-825 — The updated call-site comment says "Skip MCP calls and any compact, blank-separated native activity prelude before the model's actual reply" — but the code deliberately preserves compact preludes (and the
_find_response_markerdocstring a few lines above says so). The comment contradicts both the code and the docstring; reword it to describe the ≥2-complete-cell rule. - [conventions] CHANGELOG.md — No
### Fixedentry under[Unreleased]. The repo actively maintains Keep-a-Changelog entries with PR references (e.g. #543, #417); a user-facing Codex extraction fix should have a one-line entry.
Nits (optional)
- [conventions] src/cli_agent_orchestrator/providers/codex.py:329 — The
└continuation regexr"^[^\S\n]*└"is inlined; the file's uniform convention is a documented module-level*_PATTERNconstant (~20 precedents). Promote it to e.g.TREE_CONTINUATION_PATTERN. - [consistency] src/cli_agent_orchestrator/providers/codex.py:302-334 —
_find_response_markerduplicates_find_assistant_marker's finditer + MCP-skip loop and line-end logic verbatim; its no-cell path returns exactly what_find_assistant_markerreturns. A shared helper would keep the two from drifting. If kept separate, a one-line note that the two functions are deliberately None-equivalent (the three status-detection sites rely only onis not None) would protect against a future unification breaking either contract. - [conventions] src/cli_agent_orchestrator/providers/codex.py:303-311 — Docstring uses double spaces after periods; the sibling
_find_assistant_markerdirectly above uses single spacing.
Tests
The 8 new tests are well-constructed, follow suite conventions, cover every branch of _find_response_marker, and include all four maintainer-requested cases (verified: TestCodexBulletFormatExtraction → 18 passed). Gaps: (a) no test covers a prose reply after ≥2 activity cells — the blocking defect above; (b) the single-complete-cell preserve behavior and the two-tree-bullet truncation are intentional tradeoffs but unpinned by any test; (c) the MCP-└/cell-completeness interaction is untested. Recommend regressions for (a) at minimum, plus pinning tests documenting (b) so the tradeoffs are explicit.
Verification
Verifier ran in the PR worktree (PYTHONPATH=$PWD/src, PR code confirmed imported):
- ✓ VERIFIED — focused suite: 174 passed, 3 skipped — matches the author's claim exactly.
- ✓ VERIFIED — new
TestCodexBulletFormatExtraction: 18 passed. - ✓ VERIFIED — tree-continuation fix: two
•+└cells then a bullet reply extracts exactly• The bug is fixed. - ✓ VERIFIED — multi-bullet genuine reply preserved in full.
- ✗ REFUTED — "Fixes #541": the issue's primary compact-row repro still leaks byte-for-byte (preserved by design).
- ✗ REFUTED — single complete activity cell still leaks (both indented and unindented
└forms). - ✗ REFUTED — genuine answer with two tree-formatted bullets is truncated to only its final bullet.
- ✗ REFUTED (net-new) — prose reply after two complete activity cells: both cells leak; after three cells, one cell still leaks (boundary can only land on a bullet).
Security review found no concerns: all new regexes are backtracking-safe (empirically timed on 200k–500k char adversarial inputs, 2–5 ms), the match loop is O(n), and extraction output is pure data with no injection sink.
Verdict
Request changes — a maintainer's CHANGES_REQUESTED with two open must-fix items (claims vs. code; threshold rationale) is live at this head, and verification surfaced a new defect: the activity-cell skip never fires for prose replies, so both targeted leak shapes and the most common reply shape still misbehave. The structural approach and test discipline are right; fix the prose-reply boundary, align the claims, and this is close.
|
Thanks — I addressed the remaining correctness and documentation points in
The PR description now limits the claim to complete tree-continuation cells, preserves the compact-row ambiguity rationale, and uses Fresh validation on the pushed commit:
|
gutosantos82
left a comment
There was a problem hiding this comment.
PR Review: #545 — fix(codex): skip native activity rows in handoff extraction
Summary
Fourth iteration of the #541 fix. _find_response_marker now advances the extraction boundary past ≥2 complete native activity cells even when the model's reply is prose (the blocking defect from the previous round), and MCP tree output no longer falsely "completes" neighboring reply bullets. Both fixes verify at this head, the four previously requested regression suites are in place, the claims are corrected (Refs #541, tree-continuation-only scope), and the changelog entry is added. Two items remain: the branch does not merge cleanly with main (CONFLICTING — almost certainly the fast-moving CHANGELOG.md Unreleased section), and the new prose-boundary logic anchors on the first └ row of the last cell, so a final activity cell whose output spans multiple lines still part-leaks into the handoff. Recommend a rebase plus one small boundary fix.
Important (should fix)
-
[correctness] src/cli_agent_orchestrator/providers/codex.py:330-337 — The prose-reply boundary is computed from the first
└continuation row of the last complete cell (continuation = re.search(...), thenfollowing = re.search(r"\S", cell_tail[continuation.end():])). When the last activity cell's output spans more than one line,prose_startlands inside the residual tool output rather than at the reply. Verified at this head:- two
└rows (• Ran tests\n └ pytest -q\n └ 170 passed\n\nAll green.) → extracts'└ 170 passed\n\nAll green.' - indented continuation lines (
└ 170 passed\n 3 skipped\n\nAll green.) → extracts'3 skipped\n\nAll green.'
This is a strict improvement over
main(which leaks the entire prelude in these shapes), and the repo's existing fixtures all use single-line└tails, so real-capture frequency is unconfirmed — but the fix is cheap: skip the full contiguous continuation block (last└/indented line) instead of the first└match, and add regressions for a multi-line tail. Path-weighted (providers/), but kept at Important because the failure direction is bounded noise, never truncation. - two
-
[process] branch state —
mergeable: CONFLICTINGat this head. The PR cannot merge as-is regardless of code quality; rebase onto currentmain(the addedCHANGELOG.mdUnreleased line is the likely conflict site).
Nits (optional)
- [conventions] src/cli_agent_orchestrator/providers/codex.py:346 —
re.compile("").match(text, prose_start)(a zero-width match used purely to carry an offset) is clever but opaque; a one-line comment saying "synthesize a zero-width match so the caller's.start()contract holds for a prose boundary" would save the next reader a puzzled minute. - [correctness, edge] src/cli_agent_orchestrator/providers/codex.py:334 — the
text[candidate] != "›"composer guard checks a single character; if the tail after the last cell is only the idle composer,prose_startstaysNoneand the function falls back tomatches[0], preserving the whole prelude (verified). That is the right conservative behavior — worth pinning with a test so it survives refactoring.
Tests
Eleven new tests at this head, all following suite conventions. They pin every previously contested behavior: prose replies after two- and three-cell preludes, MCP-tree/cell-completeness interaction, compact-group preservation, single tree-formatted answers, consecutive legitimate answer bullets, interleaved commentary, and the no-marker/no-newline edges. Focused suite: 177 passed, 3 skipped (verified, matches the author's claim). Remaining gap: no test covers a final activity cell with a multi-line or multi-└ tail before a prose reply — the Important finding above — nor the composer-only fallback.
Verification
Ran in the PR worktree at 2174cdb (PYTHONPATH pointing at the PR's src):
- ✓ VERIFIED — focused Codex provider suite: 177 passed, 3 skipped — matches the author's claim exactly.
- ✓ VERIFIED — prose reply after two complete activity cells extracts exactly
'The bug is fixed.'(previous round's blocking defect is fixed). - ✓ VERIFIED — MCP tree output no longer completes neighboring reply bullets; the three-finding capture is preserved in full.
- ✓ VERIFIED — #541's compact-row repro still leaks byte-for-byte — now correctly documented as preserved-by-design, and the PR no longer claims to fix it.
- ✓ VERIFIED — single complete cell + prose reply still leaks (below the documented ≥2 threshold, by design).
- ✗ REFUTED (net-new) — "prose replies now start after the last complete activity cell" does not hold when that cell's output spans multiple lines: two-
└and indented-continuation tails leave residual tool-output lines in the extraction (probes above). - ✓ VERIFIED — two complete cells with no reply yet (composer only) fall back to full preservation rather than returning tool output.
Verdict
Request changes — the substance of both prior human reviews is genuinely addressed at this head and verification confirms the headline fixes, but the branch is CONFLICTING with main (it cannot merge as-is) and the new prose-boundary logic still part-leaks the last activity cell when its tool output spans multiple lines. A rebase plus anchoring the boundary after the full continuation block (with a multi-line-tail regression) and this lands.
2174cdb to
28b4418
Compare
|
Thanks — addressed the follow-up in The prose boundary now advances past the complete blank-separated activity output block, so additional Fresh validation on the pushed commit:
The branch is mergeable again. |
call-me-ram
left a comment
There was a problem hiding this comment.
Re-reviewed at head 28b4418. Approving — you resolved my must-fix both of the permitted ways at once, which is better than I asked for: the body now says "Refs #541" with the compact-ambiguity rationale stated plainly (so the issue correctly stays open for the residual), and the detection was extended — prose replies after activity cells and multi-line/indented └ tails now extract clean. I re-ran the battery myself at this head: the prose case gives 'The bug is fixed.', the multi-line tail gives 'All green.', and every preserve-side guard holds — • Called attention… verbatim, the paragraph-break multi-bullet answer intact, MCP-tree interleaved with reply bullets fully preserved. The deeper pass behind this also mutation-checked the new regressions (they fail against the pre-fix commits) and confirmed the ≥2-cell threshold and the truncation trade are now documented in the docstring with pinning tests, plus the anchor-divergence comment I asked for. gutosantos82's items from both of their rounds verify fixed at this head as well (prose-reply defect, MCP-└ cell miscounting, the multi-line tail part-leak).
Two small asks to fold into a final push, neither blocking:
- The CHANGELOG line landed inside the released
[2.4.0]section — the rebase happened after 2.4.1 absorbed[Unreleased], so your entry now retro-edits published release notes. Add a fresh## [Unreleased]/### Fixedat the top and move it there. - Optionally pin the two accepted trade-offs with tests so they're decisions rather than accidents: the two-tree-bullet legit answer that truncates (documented but unpinned), and the composer-only-tail conservative fallback. And a note for the record: an internal blank line inside the last cell's tool output still lets a bounded slice of that output leak — same family as what you fixed, strictly better than main, fine to leave with a comment.
Thanks for the way you handled this round — extending the mechanism instead of just relabeling the claims, with mutation-verified regressions, is exactly what this extractor needed. I've approved the CI runs for this head; once they're green this is mergeable from my side.
|
@gutosantos82 — your round-4 Item 1 — mergeable. Item 2 — the prose boundary anchoring on the first
One residual I found, non-blocking and I am not asking for it here. The advance is anchored on a blank-line separator between the activity block and the reply. If a frame has no blank line — CI: 22 green, 1 red — So from my side this is approved (08-05) and I would merge it. Could you clear or refresh your review when you get a chance? Four rounds is a lot for a first-time-ish contributor to carry and the author has turned every round around inside a day. |
Summary
Compact bullet-only groups remain intentionally preserved because their layout is indistinguishable from a legitimate multi-bullet answer; preferring the extra activity text avoids truncating a real response.
Testing
uv run pytest test/providers/test_codex_provider_unit.py --no-cov -q(174 passed, 3 skipped)uv run black --check src/cli_agent_orchestrator/providers/codex.py test/providers/test_codex_provider_unit.pyuv run isort --check-only src/cli_agent_orchestrator/providers/codex.py test/providers/test_codex_provider_unit.pyuv run mypy src/cli_agent_orchestrator/providers/codex.pygit diff --check origin/main...HEADThe full repository suite is deferred to CI.
Refs #541