Repository navigation
OMN-9273: wire overseer-verify to gh pr checks against agent self-reports - #354
Merged
Merged
Conversation
…orts Adds a new `pr_checks_live` check dimension to HandlerOverseerVerifier that shells out to `gh pr checks <num> --repo <owner>/<name> --json bucket,state, conclusion,name,completedAt` for each PR an agent self-reports as green. Previously the verifier ran five pure-Python checks and rubber-stamped PR completion claims without ever contacting GitHub — the #1 leverage failure mode in overnight dispatch ("agents lie"). Changes: - ModelClaimedPr(pr_number, repo) model for (PR, repo) claim tuples - ModelVerifierRequest gains optional `claimed_prs: list[ModelClaimedPr]` - _check_pr_checks_live: per-PR subprocess shell-out with 30s timeout, per-PR error isolation, `bucket=="pass"` + conclusion whitelist gating; any red check surfaces PR + failing check name in the failure reason - Follows canonical shell-out pattern from node_pr_snapshot_effect (subprocess .run, capture_output=True, text=True, isolated try/except for timeout / OSError / JSONDecodeError) - `pr_checks_live` inserted at priority slot 4 (above allowed_action_scope): a broken PR is stronger evidence of agent lying than a schema quibble - Skipped with a note when `claimed_prs` is empty, preserving zero-I/O semantics for non-PR verifications Tests (6 new + 19 existing overseer tests pass): - pr_checks_live skipped when no PRs claimed (subprocess never invoked) - two green PRs → verdict PASS, pr_checks_live passed - one green + one red → verdict FAIL, failing PR + check name in message - subprocess timeout → verdict FAIL, descriptive timeout message - gh non-zero exit → verdict FAIL with exit code + stderr - malformed JSON → verdict FAIL with parse-error message - existing consumer golden-chain test updated for 6-dim expectation `uv run ruff format/check` clean, `uv run mypy src/ --strict` clean (878 files).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a sixth verification dimension, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant Subprocess as "gh subprocess"
participant GitHubAPI as "GitHub API"
Client->>Handler: Submit ModelVerifierRequest (claimed_prs)
Handler->>Handler: Validate claimed_prs count against _MAX_CLAIMED_PRS
alt no claimed_prs
Handler-->>Client: pr_checks_live = PASS (skipped)
else has claimed_prs
loop per claimed_pr
Handler->>Subprocess: run `gh pr checks --repo <repo> <pr>` (--json fields, timeout)
Subprocess->>GitHubAPI: request checks
GitHubAPI-->>Subprocess: checks JSON
Subprocess-->>Handler: stdout / exit code / stderr (or timeout)
Handler->>Handler: parse JSON, call _check_row_passes() per row, collect failures
end
Handler-->>Client: Aggregate pr_checks_live result (PASS/FAIL) with message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
| Verdict | Meaning | Blocks merge? |
|---|---|---|
passed |
No critical/error findings | No |
blocked |
CRITICAL or ERROR findings found | Yes |
degraded |
All models unavailable (infra) | No (pilot) |
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_handler_overseer_verifier_pr_checks.py (1)
231-247: Add a regression for valid-but-wrong JSON shape.Please add a case where
stdoutis valid JSON but not alist[dict](e.g.{}or["x"]) so per-PR isolation stays protected against schema drift from CLI output.🧪 Suggested additional test
+@pytest.mark.unit +def test_pr_checks_live_fails_on_unexpected_json_shape() -> None: + """Valid JSON with wrong shape should fail gracefully, not raise.""" + handler = HandlerOverseerVerifier() + claimed = [ModelClaimedPr(pr_number=1, repo="OmniNode-ai/omnimarket")] + + with patch( + "omnimarket.nodes.node_overseer_verifier.handlers." + "handler_overseer_verifier.subprocess.run", + return_value=_FakeCompleted(stdout='{"not":"a-list"}', returncode=0), + ): + result = handler.verify(_make_request(claimed_prs=claimed)) + + assert result["verdict"] == "FAIL" + checks = {c["name"]: c for c in result["checks"]} # type: ignore[union-attr] + assert checks["pr_checks_live"]["passed"] is False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_handler_overseer_verifier_pr_checks.py` around lines 231 - 247, Add a new unit test alongside test_pr_checks_live_fails_on_malformed_json that exercises HandlerOverseerVerifier.verify with subprocess.run returning stdout that is valid JSON but of the wrong shape (e.g., "{}" or '["x"]') to ensure pr_checks_live still fails and the pr_checks_live check message indicates a schema/format problem; reference the existing test name test_pr_checks_live_fails_on_malformed_json, the HandlerOverseerVerifier class and its verify method, and assert the resulting result["verdict"] == "FAIL" and that checks["pr_checks_live"]["passed"] is False and its message mentions schema/format or unexpected JSON shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/omnimarket/nodes/node_overseer_verifier/handlers/handler_overseer_verifier.py`:
- Around line 449-456: After parsing JSON into raw, validate its shape before
iterating: ensure raw is a list and each element is a dict (e.g.,
isinstance(raw, list) and all(isinstance(item, dict) for item in raw)); if the
check fails, return a clear error like "{claim.repo}#{claim.pr_number}: invalid
JSON payload shape, expected list of objects" instead of proceeding to build
red_checks or calling _check_row_passes, so you avoid AttributeError when items
are strings or other types.
---
Nitpick comments:
In `@tests/test_handler_overseer_verifier_pr_checks.py`:
- Around line 231-247: Add a new unit test alongside
test_pr_checks_live_fails_on_malformed_json that exercises
HandlerOverseerVerifier.verify with subprocess.run returning stdout that is
valid JSON but of the wrong shape (e.g., "{}" or '["x"]') to ensure
pr_checks_live still fails and the pr_checks_live check message indicates a
schema/format problem; reference the existing test name
test_pr_checks_live_fails_on_malformed_json, the HandlerOverseerVerifier class
and its verify method, and assert the resulting result["verdict"] == "FAIL" and
that checks["pr_checks_live"]["passed"] is False and its message mentions
schema/format or unexpected JSON shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45df5e16-aa00-4566-bf4e-b6989188f45a
📒 Files selected for processing (5)
src/omnimarket/nodes/node_overseer_verifier/handlers/handler_overseer_verifier.pysrc/omnimarket/nodes/node_overseer_verifier/models/model_claimed_pr.pysrc/omnimarket/nodes/node_overseer_verifier/models/model_verifier_request.pytests/test_golden_chain_overseer_verifier_consumer.pytests/test_handler_overseer_verifier_pr_checks.py
Two mechanical hardening fixes raised by CodeRabbit: 1. **Fan-out cap** — Add `_MAX_CLAIMED_PRS = 20` constant. `_check_pr_checks_live` fails fast with a descriptive message when `claimed_prs` exceeds the cap, preventing pathological N-PR invocations from stalling verification up to N×30s. 20 keeps the worst-case at ~10min, well above any realistic agent self-report size. 2. **JSON shape guard** — Previously `raw: list[dict[str, Any]] = json.loads(...)` trusted gh's output shape. A valid non-list JSON (e.g. top-level object) would iterate over dict keys (strings) and `AttributeError` on `.get()`, breaking per-PR error isolation and crashing the verifier. Now explicitly assert top-level list + per-item dict, returning a `JSON shape error` message matching the existing parse-error envelope shape. Tests added (3): - too many claimed PRs → fail fast, subprocess.run never invoked - non-list JSON (top-level object) → JSON shape error, no crash - list with non-dict items → JSON shape error, no crash Full suite: 2228 passed (3 new, 2225 prior). ruff + mypy --strict clean.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/omnimarket/nodes/node_overseer_verifier/handlers/handler_overseer_verifier.py`:
- Around line 481-488: red_checks is built from untyped JSON and may contain
non-string values (e.g., None or ints), which will cause TypeError when
sorted/joined; before collecting names for failing rows in the red_checks list
comprehension (the expression using raw and _check_row_passes), coerce each
item's "name" to a string (e.g., via str(item.get("name", "<unnamed>")) or
conditional fallback) so sorted(set(red_checks)) and ', '.join(...) always
operate on strings; keep the same message construction that uses claim.repo and
claim.pr_number.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7cb04301-78ff-4a54-b903-bc39cf1f664b
📒 Files selected for processing (2)
src/omnimarket/nodes/node_overseer_verifier/handlers/handler_overseer_verifier.pytests/test_handler_overseer_verifier_pr_checks.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_handler_overseer_verifier_pr_checks.py
CodeRabbit flagged that red_checks sourced from untyped JSON could contain
None or int values for `name`, causing TypeError when sort/join runs. Coerce
via `str(item.get('name') or '<unnamed>')`. New test covers None + int + str
mixed rows, asserts sorted/joined output contains each coerced value.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OMN-9273: wire overseer-verify to gh pr checks.
[skip-deploy-gate: pure in-process verifier logic shelling out to gh pr checks via subprocess; no Dockerfile, runtime image, or topic wiring changes.]
Summary
Test plan
Related
Linear: OMN-9273
Summary by CodeRabbit
New Features
Tests