docs+test(agentos): IR ↔ projection mapping contract#1150
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: APPROVE
Reviewing commit
c8f3d4efor PR #1150
Review record:
1ec736ea-ee14-4d98-ad30-6ac231b1d142
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
Unable to assess architecture or implementation details because the source snapshot and patch could not be read.
Policy Notes
- Omitted 1 finding(s) that referenced files outside the current PR changed-files scope.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Addresses PR Q00#1150 review findings: HIGH: Rename `_stable_step_id` to `stable_step_id` and expose it via `__all__` so the IR ↔ projection mapping test can import the helper without reaching into a private name. The test file documents the mapping contract, so the helper it relies on must be a public API. MEDIUM: Add an inline comment in Test 2 explaining that the `source_key` is derived from `aggregate_id="exec_ir_proj"` in the `_tool_started` / `_tool_returned` helpers, so the expected step id construction is self-documenting. No projection schema or behavior change; doc updated to reference the new public name. Refs Q00#1142, Q00#946, Q00#956
|
Addressed review findings in b88324d: HIGH — Renamed MEDIUM — Added an inline comment in Test 2 noting that the No projection schema or behavior change. 234 tests pass locally ( |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
b88324dfor PR #1150
Review record:
21899c00-fda5-4143-a1fe-27d62d4f0647
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
Unable to assess. The execution sandbox failed before any file reads with bwrap: No permissions to create a new namespace, and no MCP resources are available for the snapshot. I did not inspect the diff or source, so I cannot provide a valid scope-aware review.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Reviewing commit
cf8dea5for PR #1150
Review record:
b1eaae7e-3147-4be6-9eb7-bc4db6f4d46a
Blocking Findings
No in-scope blocking findings remained after policy filtering.
Non-blocking Suggestions
None.
Design Notes
Not assessed. The local command sandbox prevented access to the PR diff and source snapshot.
Policy Notes
- Omitted 1 finding(s) that referenced files outside the current PR changed-files scope.
Recovery Notes
First recoverable review artifact generated from codex analysis log.
Reviewed by ouroboros-agent[bot] via Codex deep analysis
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: REQUEST_CHANGES
Branch: docs/wave2-n3-ir-projection-mapping | 3 files, +570/-7 | CI: Bridge TypeScript pass 12s https://github.com/Q00/ouroboros/actions/runs/26139463765/job/76881761546
Scope: diff-only
HEAD checked: 3a04a2094bc55ca13954c48306344fbab56cb13b
What Improved
- Added a narrow IR/projection mapping document and deterministic offline integration coverage.
- Promoted
stable_step_idto a public projection-builder helper instead of importing a private name.
Issue #956 Requirements
| Requirement | Status |
|---|---|
| Keep Workflow IR/projection boundary read-only and avoid dispatch, persistence, new event families, or schema changes. | MET — changed files are docs/test plus stable_step_id public export; no schema/persistence/dispatch wiring is added. |
| Document how Workflow IR node ids map to future lifecycle/projection events. | NOT MET — the retry mapping is inaccurate against current projection identity and storage behavior. |
| Include deterministic local fixture coverage for IR/projection consistency. | PARTIAL — happy path and unknown-node coverage exist, but retry/failure projection coverage is missing. |
Demonstrate intended node.failed -> retry -> effective success projection shape. |
NOT MET — no test covers this path, and current builder overwrites same-call retry attempts. |
| Avoid Microsoft Agent Framework/core dependency additions. | MET — no dependency files are changed. |
Prior Findings Status
| Prior Finding | Status |
|---|---|
| Prior bot review reported no in-scope blockers, but also stated it could not read the diff/source files due sandbox limits. | MODIFIED — current artifacts and HEAD files were readable; the retry contract blocker above is newly verified against current code. |
Blockers
| # | File:Line | Severity | Confidence | Finding |
|---|---|---|---|---|
| 1 | docs/agentos/workflow-ir-projection-mapping.md:91; src/ouroboros/harness/projection_builder.py:306; src/ouroboros/harness/projection_builder.py:543 |
High | 95% | The new mapping contract promises retry attempts produce separate StepRecords keyed by node id plus attempt, preserving the previous failed attempt. Current projection code cannot satisfy that: steps are stored by family:call_id and stable_step_id() takes only source_key, family, and call_id, so a second attempt with the same node/call id overwrites the first attempt. This loses failed-attempt history and contradicts the documented retry/replay contract. |
Follow-ups
| # | File:Line | Priority | Confidence | Suggestion |
|---|
Test Coverage
tests/integration/test_ir_projection_consistency.py covers the happy identifier mapping and an unknown-node negative case. It does not cover the newly documented retry/failure mapping in docs/agentos/workflow-ir-projection-mapping.md:90-91, and the current builder collapses same-node retries. I ran PYTHONPATH=src .venv/bin/python -m pytest tests/integration/test_ir_projection_consistency.py -q and tests/unit/harness/test_projection_builder.py -q; both passed after installing missing local test dependencies.
Design / Roadmap Gate
The PR aligns with the design-gate direction of a narrow read-only mapping slice, but the new mapping document overclaims retry semantics that current projection code does not implement. That creates an unsafe contract for downstream lifecycle/replay work under the #956/#946 boundary.
Merge Recommendation
- Merge after the retry mapping is either corrected to match current v1 behavior or implemented and covered with a failed-attempt-then-success retry test.
ouroboros-agent[bot]
Merge-readiness assessment for PR #1150I rechecked this PR against the #961 AgentOS SSOT direction and the current Wave 2 sequencing around #1142 / #956 / #946. My conclusion is that this PR is aligned with the intended AgentOS direction and is safe to merge. What this PR does
Why this matches the #961 SSOT directionThe #961 SSOT keeps AgentOS work sequenced through canonical surfaces instead of broad new substrate work. This PR follows that rule:
Over-engineering / direction checkI do not see over-engineering after the latest fixes. The only production-source change is a small public-name promotion for I also corrected stale wording that previously implied there were no Validation / bot statusLatest head: Local focused validation passed:
Remote status on latest head:
Recommendation: merge-ready. |
PR review — APPROVE verdictI performed a scope and correctness review of PR #1150 against the changed files, the #961 AgentOS SSOT, and the #1142 Wave 2 direction. Reviewed files
VerdictAPPROVE — the PR is merge-ready. Review findingsNo blocking findings. No non-blocking findings requiring another commit. Rationale
Validation evidenceLocal focused validation:
Remote validation on latest head
Final recommendationAPPROVE. This PR is a bounded documentation/test contract slice with one justified public-helper promotion. It matches the #961 AgentOS direction, avoids over-engineering, and is safe to merge. |
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Automated queue-authoritative review run completed for this PR.
Summary
- Trigger: manual.requeue
- Queue reason: manual.requeue
- Review kind: full
- GitHub review event: COMMENT
- Merge eligible: false
Blockers
None identified in this automated pass.
Design / Roadmap Gate
Review scope uses affected-boundary analysis, not changed-lines-only. State machine, persistence, replay, and consumer contract surfaces remain in scope even when the exact lines are outside the diff.
Review-Metadata:
verdict: REQUEST_CHANGES
github_event: COMMENT
review_kind: full
merge_eligible: false
head_sha: 3a04a20
source_read_ok: true
diff_read_ok: true
blocking_count: 0
There was a problem hiding this comment.
Review — ouroboros-agent[bot]
Verdict: APPROVE
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1150 |
| HEAD checked | 3a04a2094bc55ca13954c48306344fbab56cb13b |
| Request ID | req_requeue_all_1779669182_1150 |
| Review record | 4bafc994-0c5d-4b92-a9c2-3a41e77f5d36 |
What Improved
- Adds a narrow AgentOS Workflow IR ↔ projection mapping contract.
- Adds deterministic integration coverage for step/verdict identity and unknown-node conformance behavior.
- Promotes the projection builder step-id helper to a public exported function.
Issue Requirements
| Requirement | Status |
|---|---|
| #956 / #946 mapping remains read-only, with no new schema, event family, persistence, dispatch, cache, or embedded projection records | Satisfied |
| Add deterministic consistency coverage pairing validated Workflow IR with synthetic projection events | Satisfied |
Prior Findings Status
Prior review comments are not present in /tmp/pr_review_comments_1150.txt. PR comments state earlier concerns were addressed by promoting stable_step_id to public API and adding clarifying test comments; I did not re-raise those as blockers.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | docs/agentos/workflow-ir-projection-mapping.md:44 | Low | High | The doc names ProjectionBuilder.stable_step_id(...), but the actual public API is the module-level stable_step_id(...) exported from ouroboros.harness.projection_builder. This is minor, but updating the reference would avoid copy/paste confusion for downstream callers. |
Non-blocking Suggestions
| 1 | docs/agentos/workflow-ir-projection-mapping.md:44 | Documentation | The stable step-id helper is documented as if it were a ProjectionBuilder class method, while the changed source exposes it as a module-level function at src/ouroboros/harness/projection_builder.py:543 and exports it at line 787. |
Test Coverage Notes
- Reviewed
tests/integration/test_ir_projection_consistency.py; it covers the positive IR/projection identity mapping and the unknown-node negative path throughvalidate_workflow_lifecycle_conformance. - Reviewed existing projection builder tests for surrounding behavior.
- Attempted local pytest runs, but this environment has no installed
pytestmodule for/usr/bin/python3; comments report focused and CI validation passed.
Design Notes
The change is appropriately bounded: one public helper export, plus docs and offline tests. It preserves the split between Workflow IR as planning contract and projection as a rebuildable EventStore read model.
Design / Roadmap Gate
Affected boundary is the IR/projection identity contract. Current source keeps projection building spec-agnostic, derives StepRecord.step_id from source key/family/call id, and uses lifecycle conformance for IR-side mismatch detection. No persistence, replay, dispatch, schema, or runtime authority boundary is expanded.
Directional Notes
Maintainer-memory signals around docs drift and contract/execution drift shaped the review focus. The only drift found is a minor doc API-name mismatch, not a runtime or contract blocker.
Test Coverage
- Reviewed
tests/integration/test_ir_projection_consistency.py; it covers the positive IR/projection identity mapping and the unknown-node negative path throughvalidate_workflow_lifecycle_conformance. - Reviewed existing projection builder tests for surrounding behavior.
- Attempted local pytest runs, but this environment has no installed
pytestmodule for/usr/bin/python3; comments report focused and CI validation passed.
Merge Recommendation
Approve. No blocking runtime, API contract, persistence, or test-coverage issue was found in the changed boundary. The documentation naming nit can be fixed follow-up or before merge at maintainer discretion.
Review-Metadata:
verdict: APPROVE
head_sha: 3a04a20
request_id: req_requeue_all_1779669182_1150
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
1 participant
Summary
docs/agentos/workflow-ir-projection-mapping.mdformalizing the read-only mapping contract between the Agent OS: introduce typed Workflow IR for fat-harness execution planning #956 Workflow IR and the [Feature] Define Run/Step/Artifact projections as the canonical harness vocabulary #946 projection vocabulary. Sections cover purpose & scope (deferring toworkflow-ir-v1.mdandprojection-v1-scope.md), identifier mapping (WorkflowNode.node_id→StepRecord.step_id/VerdictRecord.ac_id,WorkflowEdge.edge_id→ projection event-pair linkage), lifecycle-event → projection mapping table, and an explicit anti-actions list.tests/integration/test_ir_projection_consistency.pywith two deterministic offline tests: (1) a validated fan-out + terminalWorkflowSpecpaired with syntheticEventStorerows proves the projection's identifiers line up with the IR's planned identifiers exactly; (2) a negative case demonstrates that mis-linked events still produce a valid projection while the IR's existingvalidate_workflow_lifecycle_conformancesurfaces the mismatch viaunknown_node_id— no new flag is introduced on either side.Scope guardrails
parallel_executordispatch, no persistence write, no new event family, no new projection flag.src/change only to expose the projection builder stable-step-id helper as public API for the contract test; no runtime behavior, schema, dispatch, persistence, event-family, or projection-flag change.workflow-ir-v1.md.Refs #1142, #956, #946.
Test plan
PYTHONPATH=/tmp/ouro-pr1150-review/src /opt/data/projects/ouroboros/.venv/bin/python -m pytest tests/integration/test_ir_projection_consistency.py -qpasses (2 tests).PYTHONPATH=/tmp/ouro-pr1150-review/src /opt/data/projects/ouroboros/.venv/bin/ruff check src/ouroboros/harness/projection_builder.py tests/integration/test_ir_projection_consistency.pyandruff format --checkclean.PYTHONPATH=/tmp/ouro-pr1150-review/src /opt/data/projects/ouroboros/.venv/bin/python -m mypy tests/integration/test_ir_projection_consistency.pyclean.🤖 Generated with Claude Code