Repository navigation
fix(OMN-13940): stop merge sweep queue spin - #1621
Conversation
…erministic ruff-fix path Builds the delegation harness end-to-end with zero LLM risk, proving the plumbing before any model-authored diff lands in Slice 1: - Splits RECEIPT_FAILURE out of the CODE_FAILURE/CHANGES_REQUESTED branch in handler_pr_lifecycle_fix._route (safety bar #5) — it stays on the Claude agent path unconditionally. - Adds a delegation eligibility gate (path/keyword denylist + blast-radius size gate + persistent two-strike escalation) that only routes small, non-sensitive CODE_FAILURE/CHANGES_REQUESTED fixes to a delegated path. - New node node_pr_delegated_fix_effect: resolves a worktree, runs deterministic `ruff format` + `ruff check --fix` (no LLM), re-checks the denylist/size gate against the ACTUAL diff produced, commits with a `delegated-by: ruff-deterministic` trailer, then shells out to the existing node_pr_polish CLI (--skip-repair-dispatch --no-automerge) so the established gate/verify/push/coderabbit-triage flow runs unchanged and a delegated fix never self-arms auto-merge (safety bar #6). - Fixes a pre-existing bug: node_pr_polish always compiled/dispatched a repair-worker (Claude) agent spec even when an already-fixed worktree_path was supplied. New `skip_repair_dispatch` command flag closes it. - Wires delegation counters (delegated/outcome/cost) through ModelPrLifecycleFixResult, the orchestrator's FixResult aggregate, and the sweep-level ModelPrLifecycleResult. - Shared command/result models live in omnimarket.events.pr_delegated_fix (not node-local) so the fix-effect bridge adapter and the new node avoid a cross-node model reach-in; the pr_polish re-entry shells to its CLI rather than importing its models in-process for the same reason. Safety-bar tests: eligible fix accepted with agent never called, denylisted path/content refused before any attempt, two-strike permanently escalates after a second failure, and a gate failure never force-pushes.
…ew dep-health findings - node_pr_polish/contract.yaml was missing the skip_repair_dispatch input declaration added to ModelPrPolishStartCommand/CLI in the previous commit, causing market-skill-baseline's input-drift check to report pr_polish as degraded. Declaring it restores contract/model parity (0 degraded, was 1). - Baseline the two dep-health findings intrinsic to any new node (UNTESTED_HANDLER — the sweep only scans src/, never tests/, so it can never see this PR's real test coverage; and the terminal-event MISSING_TOPIC_EDGE — node_pr_delegated_fix_effect's completed event has no bus subscriber in this in-process Slice 0). Same pattern as node_env_parity_collect_effect (OMN-13925) and node_pr_lifecycle_worktree_prune_effect.
… tests CI's "Tests (Split N/20)" shard reliably failed test_local_dispatch_evidence.py after pytest-split reshuffled shard boundaries because of this PR's new test files. Root cause: pytest-split merely changed which pre-existing test now lands in the same worker process ahead of these tests; handler_llm_delegation_call caches probe_health results in a module-level dict keyed by endpoint_url with a TTL, and the cache lookup happens before the monkeypatched fake_probe_health in this file is ever called — a cached negative result from an earlier test in the same worker silently short-circuits these tests' own transport patch. Clearing the cache before patching makes each test's own probe result authoritative regardless of shard/run order. Confirmed locally: full suite and this file in isolation both pass; the fix is scoped to a pre-existing cross-test global-state hazard, unrelated to this PR's actual delegation-harness scope.
…was missed The previous commit only cleared _health_cache inside _patch_transport and _patch_transport_with_content. test_local_dispatch_reaches_lan_endpoint_via_curl_on_macos_profile patches subprocess.run directly (the curl transport path) and never calls either helper, so it still failed under the same shard-ordering hazard. Converts the fix to an autouse fixture so every test in the file clears the cache regardless of which transport-patching path it uses.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 40 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 (5)
📝 WalkthroughWalkthroughThis PR adds flaky-CI-failure evidence collection from GitHub Actions job logs, propagates that evidence through inventory/triage/orchestrator models to refine CI-failure routing, fixes merge-queue "already queued" handling and fanout failure counting, normalizes repo filter inputs, adjusts sweep pass/sleep defaults and cron cadence, and hardens state-directory resolution, alongside corresponding tests. ChangesFlaky-check evidence pipeline
Merge-sweep behavior and cadence fixes
Postgres integration test gating
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Orchestrator as HandlerPrLifecycleOrchestrator
participant Inventory as HandlerPrLifecycleInventory
participant Gh as gh CLI
participant Triage as HandlerPrLifecycleTriage
participant Fix as FixHandler
Orchestrator->>Inventory: request PR check runs
Inventory->>Gh: gh pr checks --json (link)
Gh-->>Inventory: check run data
alt failed check
Inventory->>Gh: gh api job logs
Gh-->>Inventory: log text or error
Inventory->>Inventory: match network signatures
end
Inventory-->>Orchestrator: PrRecord with failed_check_flaky_evidence
Orchestrator->>Triage: ModelPrInventoryItem with flaky_evidence
Triage-->>Orchestrator: TriageResult with flaky_evidence
Orchestrator->>Orchestrator: _block_reason_for_fix
alt RED and flaky/infra-only or evidence present
Orchestrator->>Fix: block_reason = CI_FAILURE
end
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
| 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:1122: inv_handler = HandlerPrLifecycleInventory()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1140: triage_handler = HandlerPrLifecycleTriage()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1158: reducer_handler = HandlerPrLifecycleStateReducer()
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1180: merge_handler = HandlerPrLifecycleMerge(
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1223: fix_handler = HandlerPrLifecycleFix(
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:1253: prune_handler = HandlerWorktreePrune(git_adapter=GitWorktreeAdapter())
src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py:2450: 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) |
❓ Hostile Reviewer — UNKNOWNBlocking findings (critical/error): 0 Gate semantics (pilot phase)
Powered by omniintelligence.review_pairing.cli_review — node-based adversarial review via HandlerLlmCliSubprocess (OMN-8524) |
|
Auto-merged by merge-sweep | correlation_id: |
…tion-harness-slice0
…tion-harness-slice0
![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 (9)
src/omnimarket/nodes/node_pr_lifecycle_merge_effect/handlers/handler_pr_lifecycle_merge.py (2)
54-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocstring doesn't mention the "already queued" sentinel semantics.
Since
mergedstatus now depends on parsing this return string for a specific substring, the protocol docstring should call out that convention so future adapter implementations don't unknowingly break it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_pr_lifecycle_merge_effect/handlers/handler_pr_lifecycle_merge.py` around lines 54 - 69, Update the merge_pr docstring in HandlerPrLifecycleMerge to document the sentinel return-string convention for the “already queued” case, since merged status is parsed from that substring. Make it explicit in the method contract that adapter implementations must preserve this exact text/meaning when use_merge_queue is true, so callers relying on the substring check do not break.
203-213: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffAvoid parsing merge state from free-form action text
The handler still derivesmergedfrom the adapter’s wording. Return a structured merge result (already_queued: boolor a status enum) instead of relying on message text, so a wording change or alternate adapter can’t silently flip the merge outcome.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_pr_lifecycle_merge_effect/handlers/handler_pr_lifecycle_merge.py` around lines 203 - 213, The merge outcome in handler_pr_lifecycle_merge.py is still inferred from the free-form string returned by _github.merge_pr, which can break if the wording changes. Update merge_pr to return a structured result such as an object with already_queued/status, then have the handler use that field instead of checking merge_action.lower() for text. Keep the merge logic in the handler and the adapter method names (_github.merge_pr, merged, merge_action) as the main touchpoints when refactoring.tests/test_datasource_postgres.py (2)
330-333: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueWrap
conn.close()in try/finally for defense-in-depth.Minor: if any future code is inserted between
connectandclose, an exception would leak the connection sinceclose()isn't guarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_datasource_postgres.py` around lines 330 - 333, The connection cleanup in the Postgres connectivity check is not protected against future exceptions, so update the connect/close flow in the test helper around psycopg2.connect and conn.close to use a try/finally pattern. Keep the existing psycopg2.Error handling for failed connects, but ensure conn.close() is always called in the cleanup path even if additional logic is added between connect and close.
325-337: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winModule-level DB probe runs on every test collection, not just when these tests execute.
_has_reachable_db()executes at import time (line 337), so anypytestinvocation that collects this file — even with-kfilters targeting unrelated tests — pays up to aconnect_timeout=2network round-trip. In CI environments where the DB is routinely unreachable, this adds latency to every run that discovers this module.Consider deferring the reachability check to a session-scoped fixture or
pytest.mark.skipifevaluated lazily, so unrelated test runs aren't penalized by collection-time I/O.♻️ Example: lazy skip via fixture
-def _has_reachable_db() -> bool: - dsn = os.environ.get("OMNIDASH_ANALYTICS_DB_URL") - if not dsn: - return False - try: - conn = psycopg2.connect(dsn, connect_timeout=2) - except psycopg2.Error: - return False - conn.close() - return True - - -_HAS_DB = _has_reachable_db() +@pytest.fixture(scope="session") +def _has_reachable_db() -> bool: + dsn = os.environ.get("OMNIDASH_ANALYTICS_DB_URL") + if not dsn: + return False + try: + conn = psycopg2.connect(dsn, connect_timeout=2) + except psycopg2.Error: + return False + conn.close() + return True(Requires converting the
skipif(not _HAS_DB, ...)markers to a fixture-basedpytest.mark.usefixtures/autouse skip, or callingpytest.importorskip-style logic inside a fixture consumed only by the integration test class.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_datasource_postgres.py` around lines 325 - 337, The module-level reachability probe in _has_reachable_db() is running during test collection, which causes unnecessary connection attempts for unrelated pytest runs. Move the database availability check out of import-time initialization of _HAS_DB and into a lazily evaluated session-scoped fixture or a skip mechanism used only by the Postgres integration tests, so only tests that actually need the database pay the connect_timeout cost. Keep the existing _has_reachable_db() helper, but ensure it is consumed through a fixture or class-level setup tied to the integration test cases rather than at module import.src/omnimarket/nodes/node_session_bootstrap/handlers/handler_session_bootstrap.py (1)
173-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSchedule literal duplicated between
contract.yamland handler's_REQUIRED_CRONS.
cron_expression="*/30 * * * *"here must stay manually in sync withrequired_crons.merge_sweep.scheduleincontract.yaml. This dual source of truth is exactly the kind of drift risk this PR set out to fix (stale merge-sweep cadence). Consider deriving_REQUIRED_CRONSschedule values fromcontract.yamlat load time to eliminate the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_session_bootstrap/handlers/handler_session_bootstrap.py` around lines 173 - 184, The merge-sweep cron schedule is duplicated between the handler’s _REQUIRED_CRONS definition and contract.yaml, creating drift risk. Update handler_session_bootstrap so ModelCronSpec for merge-sweep does not hardcode cron_expression; instead load or derive the schedule from contract.yaml at initialization/load time and use that single source of truth in _REQUIRED_CRONS, keeping the existing ModelCronSpec and merge_sweep naming intact.tests/test_golden_chain_pr_lifecycle_inventory_compute.py (1)
193-241: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a test for the log-fetch timeout path.
Given the fail-soft timeout handling recommended in the handler review (self._run_gh vs. bare subprocess.run), a golden-chain test simulating a
subprocess.TimeoutExpiredon the job-log fetch would guard against regressions in that fail-soft behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_golden_chain_pr_lifecycle_inventory_compute.py` around lines 193 - 241, The current golden-chain coverage for HandlerPrLifecycleInventory only verifies the successful log-fetch fallback path; add a test around _collect_check_runs that simulates subprocess.TimeoutExpired when fetching job logs via the gh api call. Patch the same subprocess entrypoint used by the handler and assert the method still returns the check run plus graceful flaky_failure_evidence instead of raising, so the fail-soft timeout behavior stays covered.src/omnimarket/nodes/node_pr_lifecycle_inventory_compute/handlers/handler_pr_lifecycle_inventory.py (1)
585-598: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify: pass the already-known
repoinstead of re-deriving it from the link with a hardcoded org.
_collect_check_runs(repo, pr_number)already knows the target repo, but_collect_flaky_failure_evidence/_repo_name_from_linkre-derive it by locating the hardcoded_ORG_WIDE_OPEN_PR_ORGconstant inside the check'slinkURL and taking the next path segment. If a check's link ever belongs to a different owner (e.g., a reusable workflow hosted in another org),_repo_name_from_linksilently returns""and evidence collection is skipped — not incorrect, but an unnecessary and fragile indirection whenrepois already available and trusted.♻️ Proposed simplification
- flaky_failure_evidence=self._collect_flaky_failure_evidence(item), + flaky_failure_evidence=self._collect_flaky_failure_evidence( + item, repo=repo + ), ... - def _collect_flaky_failure_evidence( - self, item: dict[str, object] - ) -> tuple[str, ...]: + def _collect_flaky_failure_evidence( + self, item: dict[str, object], *, repo: str + ) -> tuple[str, ...]: """Return network/clone evidence from a failed check's linked job log.""" conclusion = self._normalize_check_conclusion(item) if conclusion not in {"failure", "cancelled", "timed_out"}: return () link = str(item.get("link", "") or "") if "/actions/runs/" not in link or "/job/" not in link: return () job_id = link.rsplit("/job/", 1)[1].split("?", 1)[0].strip() - repo_name = self._repo_name_from_link(link) - if not job_id.isdigit() or not repo_name: + if not job_id.isdigit(): return () result = self._run_gh( [ "gh", "api", - f"repos/{_ORG_WIDE_OPEN_PR_ORG}/{repo_name}/actions/jobs/{job_id}/logs", + f"repos/{repo}/actions/jobs/{job_id}/logs", "--header", "Accept: application/vnd.github+json", ], timeout=30, )
_repo_name_from_linkcan then be removed entirely.Also applies to: 625-636
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_pr_lifecycle_inventory_compute/handlers/handler_pr_lifecycle_inventory.py` around lines 585 - 598, Use the already available repo value from _collect_check_runs(repo, pr_number) instead of re-deriving it from the check link in _collect_flaky_failure_evidence. Update the evidence collection flow to accept the trusted repo directly, remove the dependency on _repo_name_from_link, and eliminate the hardcoded _ORG_WIDE_OPEN_PR_ORG parsing logic so this path no longer depends on the link’s owner segment.tests/test_golden_chain_pr_lifecycle_orchestrator.py (1)
1479-1517: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for mixed genuine+flaky failure PRs.
This test only covers a PR with a single failed check carrying flaky evidence. Given the routing concern raised on
_block_reason_for_fix(OR-based on aggregate evidence), consider adding a test where one failed check has evidence and another does not, to pin down whether such a PR should still route toCI_FAILURE.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_golden_chain_pr_lifecycle_orchestrator.py` around lines 1479 - 1517, The current test only covers a PR where every failed check has flaky evidence; add coverage in test_network_flake_evidence_routes_to_ci_rerun for a mixed case with one failed check in failed_check_names having evidence in failed_check_flaky_evidence and another failed check without evidence. Use _make_orchestrator, PrRecord, TriageRecord, and the existing MockFix assertion to verify _block_reason_for_fix still resolves to EnumPrBlockReason.CI_FAILURE for the mixed evidence scenario.src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py (1)
2718-2732: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNarrow root-path check may not generalize the "unsafe root-anchored state dir" fix.
_resolved_state_dironly special-cases the exact valuesPath("/")andPath("/.onex_state"). Any other root-anchoredONEX_STATE_DIR(e.g./foo,/var/onex) would still pass through and risk the same class of write-permission failure this PR is meant to fix.♻️ Suggested generalization
if raw_state_dir: candidate = Path(raw_state_dir).expanduser() resolved = candidate.resolve() - if resolved not in {Path("/"), Path("/.onex_state")}: + if resolved != Path("/") and resolved.parent != Path("/"): return resolved🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py` around lines 2718 - 2732, The _resolved_state_dir method only rejects Path("/") and Path("/.onex_state"), so other root-anchored ONEX_STATE_DIR values can still slip through and fail later. Update the validation in _resolved_state_dir to generically detect any absolute path rooted at "/" and fall back to OMNI_HOME or the home-based default instead of accepting it. Keep the behavior localized to the existing _resolved_state_dir resolution flow so all unsafe root-anchored state directories are handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/omnimarket/nodes/node_pr_lifecycle_inventory_compute/handlers/handler_pr_lifecycle_inventory.py`:
- Around line 576-584: The flaky evidence lookup in
_collect_flaky_failure_evidence is using bare subprocess.run with a timeout, so
a TimeoutExpired can escape _collect_check_runs and fail the whole PR
collection. Update that path to go through self._run_gh instead of calling
subprocess.run directly, and keep the existing returncode/error handling branch
so timeouts degrade to a non-fatal empty result. Make sure the change is applied
wherever the gh api log fetch is performed in this helper so the fail-soft
behavior matches the rest of handler_pr_lifecycle_inventory.py.
In
`@src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py`:
- Around line 612-626: The routing in _failed_check_flaky_evidence and the
surrounding _has_flaky_failure_evidence branch is too broad because it treats
any flaky signal as enough to move the whole PR into CI rerun handling. Update
the PR lifecycle logic so only checks that are actually marked flaky can
influence CI_FAILURE, and mixed failures with non-flaky lint/test/build errors
continue to route to CODE_FAILURE. Keep the fix localized to the CI routing
decision in handler_pr_lifecycle_orchestrator, using the existing helpers like
_failed_check_flaky_evidence and _has_flaky_failure_evidence to scope the
decision per failed check.
---
Nitpick comments:
In
`@src/omnimarket/nodes/node_pr_lifecycle_inventory_compute/handlers/handler_pr_lifecycle_inventory.py`:
- Around line 585-598: Use the already available repo value from
_collect_check_runs(repo, pr_number) instead of re-deriving it from the check
link in _collect_flaky_failure_evidence. Update the evidence collection flow to
accept the trusted repo directly, remove the dependency on _repo_name_from_link,
and eliminate the hardcoded _ORG_WIDE_OPEN_PR_ORG parsing logic so this path no
longer depends on the link’s owner segment.
In
`@src/omnimarket/nodes/node_pr_lifecycle_merge_effect/handlers/handler_pr_lifecycle_merge.py`:
- Around line 54-69: Update the merge_pr docstring in HandlerPrLifecycleMerge to
document the sentinel return-string convention for the “already queued” case,
since merged status is parsed from that substring. Make it explicit in the
method contract that adapter implementations must preserve this exact
text/meaning when use_merge_queue is true, so callers relying on the substring
check do not break.
- Around line 203-213: The merge outcome in handler_pr_lifecycle_merge.py is
still inferred from the free-form string returned by _github.merge_pr, which can
break if the wording changes. Update merge_pr to return a structured result such
as an object with already_queued/status, then have the handler use that field
instead of checking merge_action.lower() for text. Keep the merge logic in the
handler and the adapter method names (_github.merge_pr, merged, merge_action) as
the main touchpoints when refactoring.
In
`@src/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.py`:
- Around line 2718-2732: The _resolved_state_dir method only rejects Path("/")
and Path("/.onex_state"), so other root-anchored ONEX_STATE_DIR values can still
slip through and fail later. Update the validation in _resolved_state_dir to
generically detect any absolute path rooted at "/" and fall back to OMNI_HOME or
the home-based default instead of accepting it. Keep the behavior localized to
the existing _resolved_state_dir resolution flow so all unsafe root-anchored
state directories are handled consistently.
In
`@src/omnimarket/nodes/node_session_bootstrap/handlers/handler_session_bootstrap.py`:
- Around line 173-184: The merge-sweep cron schedule is duplicated between the
handler’s _REQUIRED_CRONS definition and contract.yaml, creating drift risk.
Update handler_session_bootstrap so ModelCronSpec for merge-sweep does not
hardcode cron_expression; instead load or derive the schedule from contract.yaml
at initialization/load time and use that single source of truth in
_REQUIRED_CRONS, keeping the existing ModelCronSpec and merge_sweep naming
intact.
In `@tests/test_datasource_postgres.py`:
- Around line 330-333: The connection cleanup in the Postgres connectivity check
is not protected against future exceptions, so update the connect/close flow in
the test helper around psycopg2.connect and conn.close to use a try/finally
pattern. Keep the existing psycopg2.Error handling for failed connects, but
ensure conn.close() is always called in the cleanup path even if additional
logic is added between connect and close.
- Around line 325-337: The module-level reachability probe in
_has_reachable_db() is running during test collection, which causes unnecessary
connection attempts for unrelated pytest runs. Move the database availability
check out of import-time initialization of _HAS_DB and into a lazily evaluated
session-scoped fixture or a skip mechanism used only by the Postgres integration
tests, so only tests that actually need the database pay the connect_timeout
cost. Keep the existing _has_reachable_db() helper, but ensure it is consumed
through a fixture or class-level setup tied to the integration test cases rather
than at module import.
In `@tests/test_golden_chain_pr_lifecycle_inventory_compute.py`:
- Around line 193-241: The current golden-chain coverage for
HandlerPrLifecycleInventory only verifies the successful log-fetch fallback
path; add a test around _collect_check_runs that simulates
subprocess.TimeoutExpired when fetching job logs via the gh api call. Patch the
same subprocess entrypoint used by the handler and assert the method still
returns the check run plus graceful flaky_failure_evidence instead of raising,
so the fail-soft timeout behavior stays covered.
In `@tests/test_golden_chain_pr_lifecycle_orchestrator.py`:
- Around line 1479-1517: The current test only covers a PR where every failed
check has flaky evidence; add coverage in
test_network_flake_evidence_routes_to_ci_rerun for a mixed case with one failed
check in failed_check_names having evidence in failed_check_flaky_evidence and
another failed check without evidence. Use _make_orchestrator, PrRecord,
TriageRecord, and the existing MockFix assertion to verify _block_reason_for_fix
still resolves to EnumPrBlockReason.CI_FAILURE for the mixed evidence scenario.
🪄 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: 3e5817ff-fb13-40a0-b782-483f315690a2
📒 Files selected for processing (18)
src/omnimarket/nodes/node_pr_lifecycle_inventory_compute/handlers/handler_pr_lifecycle_inventory.pysrc/omnimarket/nodes/node_pr_lifecycle_inventory_compute/models/model_pr_lifecycle_inventory.pysrc/omnimarket/nodes/node_pr_lifecycle_merge_effect/handlers/handler_pr_lifecycle_merge.pysrc/omnimarket/nodes/node_pr_lifecycle_orchestrator/contract.yamlsrc/omnimarket/nodes/node_pr_lifecycle_orchestrator/handlers/handler_pr_lifecycle_orchestrator.pysrc/omnimarket/nodes/node_pr_lifecycle_orchestrator/protocols/protocol_sub_handlers.pysrc/omnimarket/nodes/node_pr_lifecycle_triage_compute/handlers/handler_pr_lifecycle_triage.pysrc/omnimarket/nodes/node_pr_lifecycle_triage_compute/models/model_pr_inventory_item.pysrc/omnimarket/nodes/node_pr_lifecycle_triage_compute/models/model_pr_triage_result.pysrc/omnimarket/nodes/node_session_bootstrap/contract.yamlsrc/omnimarket/nodes/node_session_bootstrap/handlers/handler_session_bootstrap.pytests/test_codex_runtime_client.pytests/test_datasource_postgres.pytests/test_golden_chain_pr_lifecycle_inventory_compute.pytests/test_golden_chain_pr_lifecycle_merge_effect.pytests/test_golden_chain_pr_lifecycle_orchestrator.pytests/test_golden_chain_session_bootstrap.pytests/unit/nodes/node_pr_lifecycle_orchestrator/test_orchestrator_phase_transitions.py
Summary
Verification
Notes
Observed during live merge-sweep run: the skill enqueued green PRs but repeatedly counted already queued PRs as merged and failed to persist result.json because ONEX_STATE_DIR resolved to /.onex_state.
Evidence-Source: OCC#3692
Evidence-Ticket: OMN-13940
Summary by CodeRabbit
New Features
Bug Fixes