Conversation
![ouroboros-agent[bot]](https://avatars.githubusercontent.com/in/3091302?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
c9f5cc9for PR #483
Review record:
c1d27dc3-de6a-4e7f-a001-f7c7d5c0653c
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/mcp/tools/ac_tree_hud_handler.py:1281 | BLOCKING | This changes ouroboros_ac_tree_hud's default response from the full tree to the new summary view whenever callers omit view. Existing call sites are already documented to use ouroboros_ac_tree_hud(session_id, cursor) without a view argument, so those clients will silently stop receiving the tree output they were built around. That is a public MCP contract break and should stay opt-in unless you version the tool behavior. |
| 2 | src/ouroboros/mcp/tools/job_handlers.py:474 | BLOCKING | ouroboros_job_status and ouroboros_job_wait now default to the compact summary format instead of the previous full markdown snapshot. Callers that omitted view and consumed the existing detailed body (## Job, ### Execution, recent subtasks, etc.) will now get a different unstructured payload, which is another silent API break. The new views are useful, but they need to be opt-in rather than replacing the default response shape. |
Non-blocking Suggestions
None.
Design Notes
The new compact/summary renderers are a sensible addition for low-token monitoring, and the Sub-AC aggregation looks directionally solid. The problem is rollout strategy: the PR introduces those modes by changing default tool behavior instead of adding them as explicit opt-ins, which makes the MCP surface less stable for existing clients.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Q00
force-pushed
the
feat/compact-monitoring-relay
branch
from
c9f5cc9 to
146ec0a
Compare
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
146ec0afor PR #483
Review record:
980b5a7d-91ab-4f28-91f7-4ec6c98329ec
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/mcp/tools/ac_tree_hud_handler.py:66 | BLOCKING | ouroboros_ac_tree_hud now defaults to summary output instead of the full tree (_DEFAULT_HUD_VIEW = "summary" and the tool definition advertises that default). That is a backward-incompatible MCP contract change: any existing caller that omits view and parses the rendered AC tree will silently stop receiving tree rows and max-node behavior, even though the tool name and required arguments are unchanged. |
| 2 | src/ouroboros/mcp/tools/job_handlers.py:36 | BLOCKING | ouroboros_job_status and ouroboros_job_wait now default to compact/summary rendering rather than the previous full job snapshot, and job_wait also swaps its unchanged response to unchanged cursor=.... Existing callers that invoke these tools without a new view argument will no longer get the prior ## Job / ### Execution sections or the old unchanged text, which is another silent API contract break for already-deployed clients. |
Non-blocking Suggestions
| 1 | src/ouroboros/mcp/tools/job_handlers.py:325 | Refactoring | _render_job_snapshot_inner() now replays the full execution event history on every status/wait render. That may be acceptable, but it is a meaningful performance regression risk for long-running jobs with large event streams; bounding or caching the subtask-summary source data would make the new polling path safer. |
Design Notes
The sub-AC progress model is directionally good: it exposes useful structured progress without mutating top-level AC semantics. The main issue is that the PR ships it by changing default text contracts on existing MCP tools instead of adding opt-in views while preserving the old default behavior.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Restore omitted view arguments on ac_tree_hud to render the full AC tree and on job_status/job_wait to render the detailed job snapshot. Keep compact and summary renderers opt-in so existing MCP callers do not silently receive a different text contract. Preserve legacy unchanged text for tree/full defaults while still allowing compact unchanged cursor lines for explicit low-token views. Limit job execution monitor reads to recent events instead of replaying full execution history on each status render. Services: shared Affected files: - skills/run/SKILL.md - src/ouroboros/mcp/tools/ac_tree_hud_handler.py - src/ouroboros/mcp/tools/job_handlers.py - tests/unit/mcp/test_job_manager.py - tests/unit/mcp/tools/test_ac_tree_hud_handler.py - tests/unit/mcp/tools/test_definitions.py
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
3de0eb2for PR #483
Review record:
cb49d17e-75ce-45e2-bf02-89f9c33118f0
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/mcp/tools/job_handlers.py:328 | BLOCKING | ouroboros_job_status / ouroboros_job_wait now compute Sub-AC progress from only the latest 250 execution events (query_events(..., limit=250) at lines 328-338). Once a long-running execution emits more than 250 execution.subtask.updated events, older subtasks fall out of the window and summarize_subtask_events() undercounts sub_ac_total/sub_ac_completed, so the new low-token monitor can report regressing or flatly wrong progress for exactly the large jobs this PR is targeting. |
Non-blocking Suggestions
None.
Design Notes
The new split between full/tree, summary, and compact monitor views is directionally good, and reusing the Sub-AC summarization logic across HUD, job status, and session status keeps the surface area coherent. The main issue is that the job-monitor path derives progress from a truncated execution history, which breaks the correctness of the new recommended polling flow under realistic long-running workloads.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1 participant
Summary
response.metainstead of repeated full-tree polling.Changes
viewmodes forouroboros_ac_tree_hud:compact,summary, andtree.viewmodes forouroboros_job_statusandouroboros_job_wait:compact,summary, andfull.skills/run/SKILL.mdto define a harness-friendly relay loop usingouroboros_job_wait(view="summary").Verification
uv run ruff check src/ouroboros/mcp/tools/ac_tree_hud_handler.py src/ouroboros/mcp/tools/job_handlers.py src/ouroboros/mcp/tools/query_handlers.py tests/unit/mcp/tools/test_ac_tree_hud_handler.py tests/unit/mcp/tools/test_ac_tree_hud_handler_cursor_changed.py tests/unit/mcp/tools/test_ac_tree_hud_handler_completed.py tests/unit/mcp/tools/test_ac_tree_hud_max_nodes.py tests/unit/mcp/tools/test_definitions.py tests/unit/mcp/test_job_manager.pyuv run pytest tests/unit/mcp/tools/test_ac_tree_hud_handler.py tests/unit/mcp/tools/test_ac_tree_hud_handler_cursor_changed.py tests/unit/mcp/tools/test_ac_tree_hud_handler_completed.py tests/unit/mcp/tools/test_ac_tree_hud_max_nodes.py tests/unit/mcp/tools/test_definitions.py tests/unit/mcp/test_job_manager.py