diff --git a/CHANGELOG.md b/CHANGELOG.md index 5eeeb65bc..368f48803 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,11 +4,13 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + ## [Unreleased] ### Fixed - tmux listing parse failures are retried once and reported as a distinct condition instead of surfacing as a bare `ValueError` that reads like "session not found" one layer up. libtmux 0.53.1+ zips `parse_output`'s fields with `strict=True`, so any short row (a pane or session vanishing mid-listing, or trailing fields tmux omits) raised `ValueError: zip() argument 2 is shorter than argument 1` — which propagated through `server.sessions`/`window.panes`, blocked launches outright, and left the pipe-liveness watchdog unable to tell a genuinely-gone session from a transient parse failure. Adds `TmuxLookupError` and routes the listing reads in `clients/tmux.py` through a single retry-and-classify wrapper; a failed `create_session` no longer leaves an orphaned tmux session that blocks relaunching the same name. Also caps `libtmux<0.53.1`, the last release that zips non-strict (caom-anv) +- Codex handoff extraction now skips native TUI activity cells without relying on an English verb allowlist, including when the model's reply starts with prose (#545) ## [2.4.1] - 2026-08-04 @@ -129,7 +131,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - workflow: the background drive's FAILED backstop no longer overwrites an already-settled run (#505). It fired unconditionally on any exception, so a drive that raised *after* the engine journaled COMPLETED/CANCELLED — during post-settlement bookkeeping — rewrote that terminal state to FAILED, making the durable record misreport the run's outcome. The write is now a conditional `UPDATE ... WHERE state = 'running'` in the journal DAL (atomic, so no concurrent settle can interleave), covering both the `Exception` and `CancelledError` arms; a run that raises *before* settling still lands FAILED as before, so no run is left orphaned in `running` - workflow: `cao workflow events` closes its streamed SSE response on every exit path (#505). The follower breaks out of its loop on a terminal frame and abandons the generator on each reconnect, so without an explicit close the socket survived until garbage collection and a long follow with repeated reconnects accumulated live file descriptors. The equivalent MCP tool was already hardened - workflow: a caller-supplied `run_id` that loses a concurrent-submit race now returns `409` instead of `500` (#505). The uniqueness pre-check and the durable insert are not one atomic operation, so both submits can pass the pre-check; the loser's `IntegrityError` is now mapped to the same `409` the serialized case reports - - profile store writes are now atomic and inter-process safe. Both store writes were previously bare `write_text` calls, so a concurrent `cao profile` write and a server-side write could interleave or leave a partial file. Adds `locked_atomic_write` to `utils/atomic_file.py` as the blind-write sibling of `locked_atomic_rewrite` (#492): it shares the same lock, temp file, fsync, mode preservation and `os.replace`, but skips the read, so a corrupt or non-UTF-8 file in the agent store can still be replaced by the install that would have repaired it instead of failing with `UnicodeDecodeError` (#543) - self-healing pipe-pane liveness watchdog for silently-stalled FIFO forwarding (fixes #388) (#397), including detection of a stall that settles into a new static frame before the next poll and of a pipe that never delivers a single byte from terminal creation (cold start, harness-control#93) — see `CAO_PIPE_LIVENESS_COLD_START_GRACE_S` / `CAO_PIPE_LIVENESS_MAX_COLD_START_ATTEMPTS` in `docs/configuration.md` - web: attach web terminals through the configured backend so herdr-backed terminals no longer fail to attach (#417) diff --git a/src/cli_agent_orchestrator/providers/codex.py b/src/cli_agent_orchestrator/providers/codex.py index 819afa5d2..8d14c84c8 100644 --- a/src/cli_agent_orchestrator/providers/codex.py +++ b/src/cli_agent_orchestrator/providers/codex.py @@ -325,6 +325,63 @@ def _find_assistant_marker(text: str) -> Optional[re.Match[str]]: return None +def _find_response_marker(text: str) -> Optional[re.Match[str]]: + """Find the first model-reply marker after a structural activity prelude. + + Native Codex activity cells have a ``•`` summary followed by a ``└`` tree + continuation. Require at least two complete cells before advancing the + response boundary: a single tree-formatted group may be a legitimate + answer, while two consecutive cells are strong evidence of TUI activity. + Compact bullet groups remain ambiguous and are preserved. This trades a + rare false positive for avoiding silent truncation of ordinary replies and + deliberately avoids matching English verbs such as ``Read`` or ``Called``. + """ + + def line_end(start: int) -> int: + newline = text.find("\n", start) + return len(text) if newline == -1 else newline + + matches = [] + for match in re.finditer(ASSISTANT_PREFIX_PATTERN, text, re.IGNORECASE | re.MULTILINE): + if not re.match(MCP_TOOL_CALL_PATTERN, text[match.start() : line_end(match.start())]): + matches.append(match) + + if not matches: + return None + + complete_cells = [] + prose_start = None + for index, match in enumerate(matches): + next_start = matches[index + 1].start() if index + 1 < len(matches) else len(text) + cell_tail = text[line_end(match.start()) : next_start] + continuation = re.search(r"^[^\S\n]*└[^\n]*(?:\n|$)", cell_tail, re.MULTILINE) + contains_mcp_call = re.search(MCP_TOOL_CALL_PATTERN, cell_tail, re.MULTILINE) + if continuation and not contains_mcp_call: + complete_cells.append(index) + if index == len(matches) - 1: + remaining = cell_tail[continuation.end() :] + separator = re.search(r"^[^\S\n]*\n", remaining, re.MULTILINE) + following = re.search(r"\S", remaining[separator.end() :]) if separator else None + if separator and following: + candidate = ( + line_end(match.start()) + + continuation.end() + + separator.end() + + following.start() + ) + if text[candidate] != "›": + prose_start = candidate + + if len(complete_cells) >= 2: + last_cell = complete_cells[-1] + if last_cell + 1 < len(matches): + return matches[last_cell + 1] + if prose_start is not None: + return re.compile("").match(text, prose_start) + + return matches[0] + + class ProviderError(Exception): """Exception raised for provider-specific errors.""" @@ -956,12 +1013,10 @@ def extract_last_message_from_script(self, script_output: str) -> str: if user_matches: last_user = user_matches[-1] - # Find the first assistant response marker (• or assistant:) after - # the user message, skipping "• Called .(...)" MCP - # tool call markers — those are followed by tool output, not the - # model's reply. Anchoring on a tool call marker would pull tool - # output (e.g. skill body text) into the extracted response. - asst_after_user = _find_assistant_marker(clean_output[last_user.start() :]) + # Extraction uses a stricter anchor than status detection: skip MCP + # calls and at least two complete native activity cells before the + # model's actual reply, while preserving ambiguous compact groups. + asst_after_user = _find_response_marker(clean_output[last_user.start() :]) if asst_after_user: response_start = last_user.start() + asst_after_user.start() diff --git a/test/providers/test_codex_provider_unit.py b/test/providers/test_codex_provider_unit.py index eda429044..2cbdb5e0f 100644 --- a/test/providers/test_codex_provider_unit.py +++ b/test/providers/test_codex_provider_unit.py @@ -13,6 +13,7 @@ from cli_agent_orchestrator.providers.codex import ( CodexProvider, ProviderError, + _find_response_marker, _has_startup_idle_composer, _toml_override, _toml_scalar, @@ -1775,6 +1776,261 @@ def test_extract_does_not_filter_called_as_english_word(self): assert "Called attention to the import bug" in message + def test_extract_preserves_ambiguous_compact_bullet_group(self): + """Compact bullet groups are indistinguishable from a legitimate answer.""" + output = ( + "› fix the failing test\n" + "\n" + "• Explored src/providers\n" + "• Ran pytest -q\n" + "\n" + "• The bug is in the poll loop.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "Explored src/providers" in message + assert "Ran pytest -q" in message + assert "The bug is in the poll loop" in message + + def test_response_marker_returns_none_without_assistant_output(self): + """A user prompt without a response has no response marker.""" + assert _find_response_marker("› still waiting") is None + + def test_response_marker_handles_final_line_without_newline(self): + """A marker on the final line is detected without a trailing newline.""" + marker = _find_response_marker("• Complete") + + assert marker is not None + assert marker.group() == "•" + + def test_extract_preserves_single_tree_formatted_bullet(self): + """One tree-formatted bullet can be a legitimate answer, so retain it.""" + output = ( + "› inspect the provider\n" + "• Explored src/providers\n" + " └ Read codex.py\n" + "\n" + "• The extraction starts at the wrong marker.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "Explored src/providers" in message + assert "Read codex.py" in message + assert "The extraction starts at the wrong marker" in message + + def test_extract_skips_multiple_blank_separated_activity_cells(self): + """The response starts after the last complete native activity cell.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "\n" + "• Ran pytest -q\n" + " └ 170 passed\n" + "\n" + "• The bug is fixed.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "• The bug is fixed." + + def test_extract_skips_activity_cells_before_prose_reply(self): + """A prose reply starts after the last complete native activity cell.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "\n" + "• Ran pytest -q\n" + " └ 170 passed\n" + "\n" + "The bug is fixed.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "The bug is fixed." + + def test_extract_skips_multiple_tree_rows_before_prose_reply(self): + """All tree rows in the final activity cell stay before the reply.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "\n" + "• Ran pytest -q\n" + " └ pytest -q\n" + " └ 170 passed\n" + "\n" + "All green.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "All green." + + def test_extract_skips_indented_tree_output_before_prose_reply(self): + """Indented output belonging to the final tree row is not returned.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "\n" + "• Ran pytest -q\n" + " └ 170 passed\n" + " 3 skipped\n" + "\n" + "All green.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "All green." + + def test_extract_skips_three_activity_cells_before_prose_reply(self): + """All complete activity cells are removed before a prose reply.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "\n" + "• Edited\n" + " └ Updated codex.py\n" + "\n" + "• Ran pytest -q\n" + " └ 170 passed\n" + "\n" + "The bug is fixed.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "The bug is fixed." + + def test_extract_skips_interleaved_commentary_and_activity(self): + """Commentary between complete activity cells stays before the reply boundary.""" + output = ( + "› inspect the provider\n" + "• Explored\n" + " └ Read codex.py\n" + "I will verify the focused behavior next.\n" + "\n" + "• Ran pytest -q\n" + " └ 170 passed\n" + "\n" + "• The bug is fixed.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert message == "• The bug is fixed." + + def test_extract_preserves_two_consecutive_legitimate_answer_bullets(self): + """An ambiguous compact answer is preserved rather than truncated.""" + output = ( + "› summarize the fix\n" + "• Fixed parser\n" + "• Added regression tests\n" + "\n" + "• Verification: all tests pass\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "Fixed parser" in message + assert "Added regression tests" in message + assert "Verification: all tests pass" in message + + def test_extract_preserves_tree_formatted_legitimate_answer(self): + """One tree-formatted answer followed by another bullet is not activity.""" + output = ( + "› summarize the fix\n" + "• Files changed\n" + " └ src/provider.py\n" + "\n" + "• Tests pass\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "Files changed" in message + assert "src/provider.py" in message + assert "Tests pass" in message + + def test_extract_does_not_count_mcp_tree_output_as_activity_cells(self): + """MCP output must not complete neighboring model-reply bullets.""" + output = ( + "› summarize the work\n" + "• First finding\n" + '• Called tools.inspect({"path":"src"})\n' + " └ inspection result\n" + "\n" + "• Second finding\n" + '• Called tools.verify({"path":"test"})\n' + " └ verification result\n" + "\n" + "• Conclusion\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "First finding" in message + assert "Second finding" in message + assert "Conclusion" in message + + def test_extract_preserves_blank_separated_reply_bullets(self): + """A single reply bullet before a blank line is not an activity prelude.""" + output = ( + "› summarize the fix\n" + "• The parser now uses structural layout.\n" + "\n" + "• English verbs remain valid answer text.\n" + "\n" + "› \n" + ) + + provider = CodexProvider("test1234", "test-session", "window-0") + message = provider.extract_last_message_from_script(output) + + assert "parser now uses structural layout" in message + assert "English verbs remain valid" in message + class TestCodexV0111Extraction: """Extraction tests for Codex v0.111.0+ footer format."""