fix(canonical): call Result.is_ok/is_err as properties#1218
Conversation
Result.is_ok and Result.is_err are @Property, and Result has no unwrap_err method — only the .error property. The canonical live-run assertion was calling them as methods, so OUROBOROS_RUN_CANONICAL=1 raised TypeError before any failure evidence could surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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
Metadata
| Field | Value |
|### Recovery Notes
First recoverable review artifact generated from codex analysis log.
---|---|
| PR | #1218 |
| HEAD checked | 1f7dc10993e7119e4fe40e942cf649f5b5fb8eef |
| Request ID | req_1779699687_2 |
| Review record | 99af50e3-805e-4c50-9179-97f01d52b501 |
What Improved
- Fixes the opt-in canonical live-run test to use the project’s property-based
ResultAPI (is_ok,is_err,error) instead of calling removed/nonexistent method-style helpers.
Issue Requirements
| Requirement | Status |
|---|---|
| No linked issue requirement captured | N/A |
Prior Findings Status
No prior human or inline review concerns were present in the supplied comment artifacts.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | None | None | High | None. |
Non-blocking Suggestions
| 1 | None | None | None. |
Test Coverage Notes
- Reviewed
/tmp/pr_diff_1218.patch, changed-files list, canonical test harness,AutoHandler.handle(), andouroboros.core.types.Result. - Could not execute pytest in this container:
pythonis missing,python3is 3.11 while the project requires>=3.12and.python-versionis3.14, andpytestis not installed forpython3.
Design Notes
The change aligns the canonical live-run assertion with the repository-wide Result contract and does not alter runtime behavior.
Design / Roadmap Gate
Affected boundary is the test-to-MCP-handler contract: _invoke_ouroboros_auto() returns AutoHandler.handle()’s Result[MCPToolResult, MCPServerError], whose API exposes is_ok, is_err, and error as properties. The updated assertion matches that contract. No persistence, replay, compatibility, or production runtime boundary changes are introduced.
Directional Notes
Maintainer memory emphasized evidence-based evaluation review. I focused on whether the changed test accurately exercises the MCP handler result boundary and whether it weakens canonical evidence; no current-HEAD blocker was found.
Test Coverage
- Reviewed
/tmp/pr_diff_1218.patch, changed-files list, canonical test harness,AutoHandler.handle(), andouroboros.core.types.Result. - Could not execute pytest in this container:
pythonis missing,python3is 3.11 while the project requires>=3.12and.python-versionis3.14, andpytestis not installed forpython3.
Merge Recommendation
Approve. The patch is a narrow correction to a skipped-by-default live canonical test and matches the current Result API.
Review-Metadata:
verdict: APPROVE
head_sha: 1f7dc10
request_id: req_1779699687_2
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
Coordination note re: #1214PR #1214 (also OPEN, also editing class _Ok:
def is_ok(self) -> bool: # method, not property
return True
def unwrap_err(self) -> str: # nonexistent on Result
return "unexpected"That stub is shape-incompatible with the real If this PR lands first, the cc reviewers of #1214 for awareness. |
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 | #1218 |
| HEAD checked | 1f7dc10993e7119e4fe40e942cf649f5b5fb8eef |
| Request ID | req_1779699995_3 |
| Review record | eeb6a290-67ab-4c9c-bc55-eaacad33f99a |
What Improved
- Fixes the live canonical harness assertion to use the real
ResultAPI:is_ok/is_errproperties anderrorinstead of nonexistent method-style accessors.
Issue Requirements
| Requirement | Status |
|---|---|
| No linked issue requirement captured | N/A |
Prior Findings Status
No inline prior review comments were present. The PR comment about #1214 was treated as coordination context only; it does not create a blocker for this PR.
Blockers
No in-scope blocking findings remained after policy filtering.
Follow-up Findings
| # | File:Line | Priority | Confidence | Suggestion |
|---|---|---|---|---|
| 1 | tests/canonical/test_canonical.py:248 | Low | Medium | Consider adding a tiny unit-level assertion around the live-run result failure formatting if this path gets more logic; for this PR’s two-line contract fix, the current source-level API match is sufficient. |
Non-blocking Suggestions
| 1 | tests/canonical/test_canonical.py:248 | Test environment | I could not execute the focused pytest module locally because pytest is not installed in the review container (python3 -m pytest tests/canonical/test_canonical.py failed with No module named pytest). |
Test Coverage Notes
- Reviewed
tests/canonical/test_canonical.py,tests/canonical/conftest.py, andsrc/ouroboros/core/types.py. - Confirmed
AutoHandler.handle()returnsResult[MCPToolResult, MCPServerError], andResult.is_ok,Result.is_err, andResult.errorare the correct public API. - Attempted focused test run, but local pytest dependency is unavailable in the snapshot environment.
Design Notes
The change preserves the existing canonical harness design and only corrects its Result contract usage.
Design / Roadmap Gate
Affected boundary is the opt-in live canonical test path from _invoke_ouroboros_auto() to AutoHandler.handle() and Result unwrapping. The PR aligns the test with the public Result properties, improves MCP error reporting, and does not alter persistence, runtime state, replay behavior, or handler contracts.
Directional Notes
Maintainer memory emphasized contract correctness and evaluation evidence. I focused review on the Result API boundary and the live canonical invocation path; no blocker is supported by current source evidence.
Test Coverage
- Reviewed
tests/canonical/test_canonical.py,tests/canonical/conftest.py, andsrc/ouroboros/core/types.py. - Confirmed
AutoHandler.handle()returnsResult[MCPToolResult, MCPServerError], andResult.is_ok,Result.is_err, andResult.errorare the correct public API. - Attempted focused test run, but local pytest dependency is unavailable in the snapshot environment.
Merge Recommendation
Merge. The patch fixes a real API mismatch in the live canonical harness, and I found no blocking regression in the affected call path.
Review-Metadata:
verdict: APPROVE
head_sha: 1f7dc10
request_id: req_1779699995_3
review_profile: memory-aware-zero-trust-v2
advisory_memory_only: true
Reviewed by ouroboros-agent[bot] via Codex deep analysis
2 participants
Summary
tests/canonical/test_canonical.py:248-252was callingresult.is_ok()andresult.unwrap_err()as methods, butResult.is_ok/is_errare@propertyaccessors insrc/ouroboros/core/types.py:70-78andunwrap_errdoes not exist (only the.errorproperty at line 102 exposes the err side).OUROBOROS_RUN_CANONICAL=1 pytest tests/canonical/) therefore raisedTypeError: 'bool' object is not callablebefore any production failure evidence could surface — masking real regressions inooo autobehind a test-harness type error.Why this matters
R1 evidence (gathered locally on
265aedb4) shows thatooo auto cli-todois currently BLOCKED at safe-default closure (separate regression; tracked in a follow-up issue). That blocker is only readable after this harness fix lands, otherwise pytest dies at theis_ok()call site before the AutoPipelineState/MCP envelope can be inspected.Test plan
uv run pytest tests/canonical/ -v(canonical live OFF, $0 token cost) — 17 passed, 1 skippedOUROBOROS_RUN_CANONICAL=1 pytest tests/canonical/ -k cli-todo— surfaces the realphase=blockedevidence instead ofTypeError🤖 Generated with Claude Code