fix(harness): harden projection query provenance after #990#1016
Conversation
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Reviewing commit
9871cd0for PR #1016
Review record:
4b5f63fb-ab68-4c9a-903c-b790e0b57e82
Blocking Findings
| # | File:Line | Severity | Finding |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|-----------|----------|---------|
| 1 | src/ouroboros/mcp/tools/projection_handlers.py:248 | BLOCKING | In the new len(payload_execution_ids) == 1 branch, the handler keeps filtering the original query_session_related_events(..., execution_id=None) result instead of re-querying with the resolved execution ID. That original query only matches aggregate_id == session_id or payload.session_id == session_id, so it never loads child/runtime events linked solely by parent_execution_id. For metadata-less sessions with subagent/child execution activity, the projection will silently omit part of the run even though the code now treats the session as unambiguous. |
| 2 | src/ouroboros/mcp/tools/projection_handlers.py:250 | BLOCKING | That same metadata-less single-execution branch drops all non-terminal session aggregate events. If a session/<session_id> start event exists but lacks execution_id while still carrying seed_id, seed_goal, or the earliest timestamp, the projection now silently falls back to the session/execution ID and later timestamps instead of using the persisted run metadata. This produces wrong seed_id_source, goal, and started_at for the exact legacy/metadata-less histories this change is trying to support. |
Non-blocking Suggestions
None.
Design Notes
The seed provenance addition is straightforward, but the new ambiguity handling is implemented as post-filtering over an already under-scoped session query. For the metadata-less fallback path to be correct, it needs to resolve the execution ID and then load the full execution-related event set for that execution rather than trimming the initial session-only result.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Keep the Q00#946/Q00#990 MCP projection surface evidence-shaped by making caller-provided seed IDs explicit and rejecting session-only queries that would splice metadata-less multi-execution payloads. Constraint: Q00#961 keeps AgentOS substrate changes narrow and folded into canonical Q00#946 instead of opening new roadmap surfaces. Rejected: Folding this into Q00#1006 verifier PASS work | projection read-model hardening and atomic AC acceptance are separate review surfaces. Confidence: high Scope-risk: narrow Directive: Keep projection query handlers read-only and fail-closed on ambiguous run boundaries. Tested: uv run pytest tests/unit/mcp/tools/test_definitions.py::TestProjectionQueryHandler -q; uv run pytest tests/unit/persistence/test_event_store.py::TestSessionRelatedEvents -q; uv run mypy src/ouroboros/mcp/tools/projection_handlers.py tests/unit/mcp/tools/test_definitions.py; uv run ruff check src/ouroboros/mcp/tools/projection_handlers.py tests/unit/mcp/tools/test_definitions.py Not-tested: Full repository test suite.
9871cd0 to
bf73ebb
Compare
|
Addressed the review blockers in |
|
@ouroboros-agent Please re-review current head |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
bf73ebbfor PR #1016
Review record:
2090ddc2-b68e-42ad-abd6-aa456bffd78a
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
| 1 | tests/unit/mcp/tools/test_definitions.py:887 | Nice-to-have tests | The new seed provenance branch only exercises "event" and "argument". A small unit test for the "fallback" path would make the new seed_id_source contract complete and guard against future regressions in the no-seed-in-events case. |
Design Notes
The change is narrowly scoped and the fail-closed behavior for metadata-less session queries is a reasonable hardening of the projection surface. Re-querying once a single payload execution candidate is found also matches the event-store contract better than filtering the initial broad session query in memory.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Summary
Follow-up to #990 / #946 PR-2.
This PR hardens the read-only projection MCP query surface by:
seed_id_sourceto distinguish event-derived, caller-provided, and fallback seed IDsScope
Validation
uv run pytest tests/unit/mcp/tools/test_definitions.py::TestProjectionQueryHandler -q→ 14 passeduv run pytest tests/unit/persistence/test_event_store.py::TestSessionRelatedEvents -q→ 5 passeduv run mypy src/ouroboros/mcp/tools/projection_handlers.py tests/unit/mcp/tools/test_definitions.py→ passeduv run ruff check src/ouroboros/mcp/tools/projection_handlers.py tests/unit/mcp/tools/test_definitions.py→ passed