Skip to content

fix(codex): skip native activity rows in handoff extraction - #545

Open
w3lld1 wants to merge 6 commits into
awslabs:mainfrom
w3lld1:fix/codex-native-activity-extraction
Open

fix(codex): skip native activity rows in handoff extraction#545
w3lld1 wants to merge 6 commits into
awslabs:mainfrom
w3lld1:fix/codex-native-activity-extraction

Conversation

@w3lld1

@w3lld1 w3lld1 commented Aug 3, 2026

Copy link
Copy Markdown

Summary

  • I detect blank-separated Codex activity cells that include native tree-continuation rows, without relying on an English verb allowlist.
  • I start handoff extraction at the following model reply while preserving ambiguous compact bullet groups and legitimate reply bullets.
  • I added regressions for tree-continuation activity, compact ambiguous groups, and blank-separated answer bullets.

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)
  • focused response-marker selectors (8 passed)
  • uv run black --check src/cli_agent_orchestrator/providers/codex.py test/providers/test_codex_provider_unit.py
  • uv run isort --check-only src/cli_agent_orchestrator/providers/codex.py test/providers/test_codex_provider_unit.py
  • uv run mypy src/cli_agent_orchestrator/providers/codex.py
  • git diff --check origin/main...HEAD

The full repository suite is deferred to CI.

Refs #541

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-commenter

codecov-commenter commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@a2c5afe). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #545   +/-   ##
=======================================
  Coverage        ?   90.99%           
=======================================
  Files           ?      179           
  Lines           ?    23302           
  Branches        ?        0           
=======================================
  Hits            ?    21203           
  Misses          ?     2099           
  Partials        ?        0           
Flag Coverage Δ
unittests 90.99% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@haofeif

haofeif commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

@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 ?

@w3lld1

w3lld1 commented Aug 3, 2026

Copy link
Copy Markdown
Author

Thanks — added focused coverage for the response-marker edge cases in aa2b9024:

  • no assistant marker after a user prompt;
  • a final response marker without a trailing newline;
  • consolidated line-boundary handling so the previously unreachable duplicate branch is removed.

Fresh validation on the pushed commit:

  • uv run pytest -q test/providers/test_codex_provider_unit.py --cov=cli_agent_orchestrator.providers.codex --cov-report=term-missing — 170 passed, 3 skipped; the three previously uncovered lines in _find_response_marker are now covered;
  • focused response-marker selectors — 4 passed;
  • Black check and git diff HEAD^ --check.

@haofeif

haofeif commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

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 _find_response_marker().

The helper returns the first bullet after either two preceding bullet-only rows or any preceding row. That boundary is not unique to an activity prelude.

First, normal Codex activity still leaks when multiple activity cells are blank-separated, which is the layout used by upstream Codex snapshots:

› inspect
• Explored
  └ Read codex.py

• Ran pytest -q
  └ 170 passed

• The bug is fixed.

PR #545 extracts from • Ran pytest -q, so the second activity cell remains in the handoff result. With model commentary interleaved between activity cells, it starts at that intermediate commentary and includes all later activity too.

Second, the same heuristic introduces answer truncation. This valid response:

• Fixed parser
• Added regression tests

• Verification: all tests pass

is reduced to only • Verification: all tests pass. A valid tree-formatted answer such as • Files changed / └ src/provider.py is similarly dropped when another bullet follows. The base branch preserves both complete answers.

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.


## What Is Correct

- The PR avoids an English-verb allowlist, so replies such as `Called attention
  to the bug` remain valid.
- MCP tool-call markers continue to be excluded.
- The latest commit covers all modified lines.
- The exact compact two-row issue repro passes.

## Validation

```text
Focused Codex provider suite:
170 passed, 3 skipped

Black:
2 files would be left unchanged

isort:
passed

mypy:
Success: no issues found in 1 source file

git diff --check:
passed

GitHub:
All checks passed at reviewed head
```

@haofeif haofeif self-assigned this Aug 3, 2026
@w3lld1

w3lld1 commented Aug 3, 2026

Copy link
Copy Markdown
Author

Thanks — addressed the behavioral blocker in 3feb5aa.

The response boundary now advances only after at least two complete native activity cells ( summary plus continuation). Ambiguous compact bullet groups and single tree-formatted groups are preserved rather than risking answer truncation.

Added coverage for:

  • multiple blank-separated activity cells;
  • interleaved commentary and activity;
  • consecutive legitimate answer bullets;
  • a legitimate tree-formatted answer.

Fresh validation on the pushed commit:

  • focused Codex provider suite: 174 passed, 3 skipped;
  • focused boundary selectors: 8 passed;
  • Black and isort checks;
  • mypy on codex.py;
  • git diff --check.

@call-me-ram call-me-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 gutosantos82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_cells is computed over matches[:-1] and the returned boundary is always matches[complete_cells[-1] + 1] — an element of matches — 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 to matches[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 of matches; 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 from matches, but their output lines land in the cell_tail of 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_marker docstring 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 ### Fixed entry 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 regex r"^[^\S\n]*└" is inlined; the file's uniform convention is a documented module-level *_PATTERN constant (~20 precedents). Promote it to e.g. TREE_CONTINUATION_PATTERN.
  • [consistency] src/cli_agent_orchestrator/providers/codex.py:302-334_find_response_marker duplicates _find_assistant_marker's finditer + MCP-skip loop and line-end logic verbatim; its no-cell path returns exactly what _find_assistant_marker returns. 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 on is 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_marker directly 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.

@w3lld1

w3lld1 commented Aug 4, 2026

Copy link
Copy Markdown
Author

Thanks — I addressed the remaining correctness and documentation points in 2174cdb.

  • prose replies now start after the last complete activity cell, with regressions for two- and three-cell preludes;
  • MCP tree output no longer makes neighboring reply bullets look like complete activity cells;
  • the docstring now explains the two-cell threshold and accepted false-positive tradeoff;
  • the extraction/status-anchor distinction is documented at the call site;
  • added the Unreleased changelog entry.

The PR description now limits the claim to complete tree-continuation cells, preserves the compact-row ambiguity rationale, and uses Refs #541.

Fresh validation on the pushed commit:

  • focused Codex provider suite: 177 passed, 3 skipped;
  • the three new regression selectors: 3 passed;
  • Black and isort checks;
  • mypy on codex.py;
  • git diff --check.

@gutosantos82 gutosantos82 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(...), then following = re.search(r"\S", cell_tail[continuation.end():])). When the last activity cell's output spans more than one line, prose_start lands 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.

  • [process] branch statemergeable: CONFLICTING at this head. The PR cannot merge as-is regardless of code quality; rebase onto current main (the added CHANGELOG.md Unreleased line is the likely conflict site).

Nits (optional)

  • [conventions] src/cli_agent_orchestrator/providers/codex.py:346re.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_start stays None and the function falls back to matches[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.

@w3lld1
w3lld1 force-pushed the fix/codex-native-activity-extraction branch from 2174cdb to 28b4418 Compare August 5, 2026 14:39
@w3lld1

w3lld1 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Thanks — addressed the follow-up in 28b4418 after rebasing onto current main.

The prose boundary now advances past the complete blank-separated activity output block, so additional rows and indented tool-output lines no longer leak into the handoff. I added regressions for both shapes.

Fresh validation on the pushed commit:

  • focused Codex provider module: 179 passed, 3 skipped;
  • the two new regression selectors: 2 passed;
  • Black and isort checks;
  • mypy on codex.py;
  • git diff --check.

The branch is mergeable again.

@call-me-ram call-me-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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] / ### Fixed at the top and move it there.
  2. 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.

@w3lld1

w3lld1 commented Aug 5, 2026

Copy link
Copy Markdown
Author

Thanks — moved the #545 entry out of the released 2.4.0 notes and into a new Unreleased / Fixed section in f9a66f4. git diff HEAD^ --check passes.

@call-me-ram

Copy link
Copy Markdown
Collaborator

@gutosantos82 — your round-4 CHANGES_REQUESTED is the only thing holding this now, and I believe both of your items are resolved. Rather than just asserting that, here is what I verified independently at head (f9a66f4), since the second item was subtle enough to deserve a real check.

Item 1 — mergeable. MERGEABLE at head. @w3lld1 rebased in 28b4418 and the CHANGELOG collision you predicted is gone (the entry moved out of the released 2.4.0 notes into a new Unreleased/Fixed section in f9a66f4).

Item 2 — the prose boundary anchoring on the first row, so a final activity cell with multi-line output part-leaks. This was a real defect and it is now genuinely fixed, not narrowed. The boundary logic advances past the complete blank-separated output block rather than the first continuation row. I probed _find_response_marker directly at head:

frame boundary lands on
two cells, final cell has 3 indented output rows, blank line, prose 'The bug is fixed in the parser.'
two cells, final cell has multi-line output, bullet reply '• The bug is fixed.'
single ambiguous tree group (must NOT truncate) preserved whole ✅

test_extract_skips_indented_tree_output_before_prose_reply covers exactly your shape (└ 170 passed + 3 skipped → extracts "All green."). Focused extraction/handoff suite: 34 passed at head.

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 — └ 170 passed / 3 skipped / The bug is fixed. with nothing between — the function returns matches[0] and the whole activity block leaks, i.e. the original bug. I confirmed that behaviour by probe, but I have no evidence real Codex frames ever omit that blank line, and every fixture and every test frame in the repo includes it. It also fails in the safe direction relative to the alternative (leaking beats silently truncating a real reply). If @w3lld1 wants to close it off, a one-line relaxation of the separator requirement plus a regression would do it — but I would not hold a fifth round for a shape nobody has observed.

CI: 22 green, 1 red — Security Scan, which is the repo-wide js-yaml@4.3.0 Trivy alert on main, not this branch. #569 fixes it. Same red is currently on #564.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants