Repository navigation
fix(OMN-14173): fail-closed prs_fixed accounting for OCC autobind arm — verify pushed companion, not the call - #1653
Conversation
… — verify pushed companion, not the call merge_sweep --fix-only reported prs_fixed=2 on omnimarket #1651/#1652 while authoring ZERO OCC companions (a false-success of the exact class the tooling targets). Root cause = accounting, not routing. Trace: - Routing (STEP 2A): _block_reason_for_fix already routes occ-preflight/eligibility (with a ticket) to RECEIPT_EVIDENCE_SOURCE_AUTOBIND via the OCC-evidence subset match (handler_pr_lifecycle_orchestrator.py) — the OMN-13990 follow-up already landed this. The pre-supplied "{verify/verify}-only" diagnosis was stale vs HEAD. Re-asserted with a fresh witness test. - Accounting (STEP 2B, the real defect): HandlerPrLifecycleFix.handle set fix_applied=True whenever _route() returned without raising — including the no-op / idempotent / short-circuit paths that push NO companion. _fix_one then counted prs_dispatched=1 off fix_applied. Fix (fail-closed, verify the effect not the call): - New OccCompanionVerifier reads GitHub back after the autobind adapter runs and confirms all three: product PR body carries Evidence-Source: OCC#<n>, that OCC PR is OPEN, and the auto/* companion branch exists on the OCC remote. Any missing evidence / resolution error fails CLOSED (verified=False). - HandlerPrLifecycleFix gains occ_companion_verified on its result, set from the verifier for the autobind arm. Default verifier is fail-closed (proves nothing → counts nothing); the merge-sweep orchestrator injects the live verifier. - _fix_one gates prs_fixed for RECEIPT_EVIDENCE_SOURCE_AUTOBIND on occ_companion_verified, never on fix_applied. All other block reasons keep their existing dispatch semantics (no regression to merge arm, OccContractAdapter, CI/code/coderabbit arms). Tests (STEP 3, executed): new test_occ_companion_fail_closed_accounting.py proves (i) occ-preflight/eligibility routes to the autobind arm, (ii) an unverified / unpushed companion is NOT counted in prs_fixed (fail-closed), and the happy path counts exactly once. Live-adapter wiring test asserts the orchestrator injects the real OccCompanionVerifier. dod_evidence: - env -u PYTHONPATH uv run pytest tests/ -m "not kafka": 12271 passed, 60 skipped - new test_occ_companion_fail_closed_accounting.py: 7 passed - ruff format + ruff check --fix: clean - mypy --strict on both nodes: Success, no issues in 27 files - pre-commit (scoped to changed files, OMNIBASE_INFRA_PATH set): all hooks pass
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Temporary close/reopen to trigger fresh pull_request workflows after OCC#3744 Evidence-Source landed; head SHA unchanged. |
|
| Rule | Description |
|---|---|
| ARCH-TOPIC-001 | No hardcoded Kafka topic strings in handler code |
| ARCH-DI-001 | No event_bus=None bypass |
| ARCH-DI-002 | No direct Handler instantiation outside workflow_runner/adapters |
| ARCH-DI-003 | No reinvented DI containers |
| ARCH-TOPIC-002 | contract.yaml topics follow `onex.{cmd |
ARCH-DI-002: Direct Handler instantiation (warning)\n\n```\nsrc/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1163: inv_handler = HandlerPrLifecycleInventory()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1181: triage_handler = HandlerPrLifecycleTriage()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1199: reducer_handler = HandlerPrLifecycleStateReducer()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1221: merge_handler = HandlerPrLifecycleMerge(
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1267: fix_handler = HandlerPrLifecycleFix(
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1302: prune_handler = HandlerWorktreePrune(git_adapter=GitWorktreeAdapter())
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:2499: verify_handler = HandlerVerify()\n```\n\nHandler classes should be wired only in workflow_runner.py or adapter modules. Direct instantiation elsewhere bypasses the contract system.\n\n
Powered by node_pr_review_bot
✅ PR Review Bot — CLEANFindings: 0
Verdict semantics
Powered by node_pr_review_bot — Qwen3-Coder-30B reviewer + DeepSeek-R1 judge (OMN-7963) |
…fresh-ticket companion) Residual follow-up to #1653. The residual hypothesis ("OccAutobindAdapter only SYNCS an existing OCC contract and no-ops when a fresh ticket has none — the contract-CREATION path may be required to mint for a first-time ticket") is REFUTED against live code: - The create-if-absent branch already exists and is wired live: adapter_occ_autobind.py:285-294 writes contracts/<ticket>.yaml via _CONTRACT_TEMPLATE when `not contract_path.is_file()`, and the live OccAutobindAdapter has been injected in _ensure_sub_handlers since facb4e8 (2026-06-24, #1392) — never a noop on the merge_sweep path. - run_git uses check=True and re-raises CalledProcessError (occ_git_transport.py), so a failed OCC push RAISES → fix_applied=False → not counted. A mint failure can never surface as "fixed but no companion". The only clean-return-no-mint path is the idempotency guard (already Evidence-Source: OCC#-bound), which #1653 now gates via the OccCompanionVerifier read-back. The genuine gap was COVERAGE: the sole end-to-end test (TestAutobindForcePush) forced Path.is_file=True (the SYNC path), so nothing locked the CREATE path and the residual could not be falsified empirically. This adds TestAutobindCreatesContractForFreshTicket (2 tests), verifying the EFFECT (not the call), mirroring the #1653 mutation-test style: - fresh ticket (is_file=False) → a net-new contracts/<ticket>.yaml is WRITTEN with the contract template, the auto/* companion branch is force-pushed, and Evidence-Source: OCC#<n> is patched back onto the product PR. - existing contract (is_file=True) → SYNC binds receipts + pushes + patches WITHOUT rewriting the contract (OMN-13888 whole-file-hash: never modify an existing contract). Mutation-verified: reverting the `if not contract_path.is_file()` create guard (→ `if False`) makes test_fresh_ticket_mints_net_new_contract_and_pushes fail (only receipts written, no contract) — the test genuinely guards the mint. dod_evidence: - env -u PYTHONPATH uv run pytest tests/unit/nodes/node_pr_lifecycle_fix_effect/test_adapter_occ_autobind.py: 26 passed - node_pr_lifecycle_fix_effect + node_pr_lifecycle_orchestrator test dirs: 274 passed - mutation (revert create guard): fresh-ticket test FAILS as designed, source restored clean - ruff format + ruff check: clean; mypy --strict fix_effect node: Success (19 files) - pre-commit (changed file, OMNIBASE_INFRA_PATH set): all hooks pass - Live fresh-mint end-to-end proof: NOT run — no safe target (only open omnimarket PRs are #1653 [self-authoring prohibited] and #1651 [already bound OCC#3740]); merge_sweep has no single-PR scope. proof_class=code-only for the live link; create path is code-verified + mutation-locked. OCC companion for OMN-14173 is owed (Codex authors it per feedback_no_self_authored_evidence).
|
| 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)
1 participant
Evidence-Source: OCC#3744
Evidence-Ticket: OMN-14173
OMN-14173 — merge_sweep --fix-only reports prs_fixed but authors ZERO OCC companions
Closes OMN-14173.
Problem (live-verified false-success)
uv run onex skill merge_sweep --repos omnimarket --fix-onlyreturnedprs_fixed=2on omnimarket #1651/#1652 while authoring zero OCC companions — a false-green of the exact class the tooling is meant to catch.Trace (STEP 1 — completed before editing)
_block_reason_for_fix(handler_pr_lifecycle_orchestrator.py:704) already routesocc-preflight / eligibilityfailures (with a ticket) toRECEIPT_EVIDENCE_SOURCE_AUTOBINDvia the OCC-evidence subset match at lines 743-760 (_OCC_EVIDENCE_CHECK_NAMES={verify / verify, occ-preflight / eligibility}, cosmeticEnable Auto-Mergeexcluded). This landed in the OMN-13990 follow-up rounds. The pre-supplied{verify / verify}-only routing diagnosis was stale relative to HEADad3ef3c6— verified by the existingtest_occ_preflight_only_with_ticket_routes_to_autobind.HandlerPrLifecycleFix.handlesetfix_applied = Truewhenever_route()returned without raising (handler_pr_lifecycle_fix.py), including the no-op / idempotent / short-circuit paths that push no companion._dispatch_fix_parallel._fix_onethen setprs_dispatched = 1 if fix_applied(handler_pr_lifecycle_orchestrator.py:~2603). Nothing verified the effect.Fix (STEP 2 — verify the effect, do not trust the call)
OccCompanionVerifier(occ_companion_verifier.py): after the autobind adapter runs, re-reads GitHub and confirms all three of — product PR body carriesEvidence-Source: OCC#<n>, that OCC PR is open, and theauto/<repo>-pr-<n>-occ-autobindbranch exists on the OCC remote. Any missing evidence / token / network error fails closed (verified=False).ModelPrLifecycleFixResult.occ_companion_verified— set from the verifier for the autobind arm. Default verifier (_UnverifiedOccCompanionVerifier) proves nothing → counts nothing; the merge-sweep orchestrator injects the liveOccCompanionVerifierin_ensure_sub_handlers._fix_onegatesprs_fixedforRECEIPT_EVIDENCE_SOURCE_AUTOBINDonocc_companion_verified is True, never onfix_applied. A classified-but-not-minted / short-circuited PR is now reported skipped, not fixed. Every other block reason (merge arm,OccContractAdapterdeploy-gate, CI/code/coderabbit) keeps its existing semantics — no regression.Scope note
The strict companion gate is scoped to the
RECEIPT_EVIDENCE_SOURCE_AUTOBINDarm (the exact reproduced defect — occ-preflight PRs).DEPLOY_GATE_CONTRACT_NOT_FOUNDretains existing behavior because its trivial-infra fast-path legitimately resolves without authoring a companion; extending read-back verification there is a clean follow-up.dod_evidence (executed)
env -u PYTHONPATH uv run pytest tests/ -m "not kafka"→ 12271 passed, 60 skipped (312s)test_occ_companion_fail_closed_accounting.py→ 7 passed (routing witness + fail-closed no-count + verified-counts-once + handler field)ruff format+ruff check --fix→ cleanmypy --stricton both nodes →Success: no issues found in 27 source filespre-commit run(changed files,OMNIBASE_INFRA_PATHset) → all hooks passStaged for Codex
Merge queue untouched (Codex owns merges). omnimarket is heavy:
occ-preflight / eligibility+verify / verifywill be RED until a net-new OCC companion lands for OMN-14173 — not self-authored per policy (occ_companion_owed = true).