[jonahgabriel]

What

Replaces all gh subprocess calls in node_merge_sweep with a proper HTTP adapter behind a protocol. This is the same pattern we used for node_linear_triage (PR #368).

Problem

node_merge_sweep had gh CLI subprocess calls baked directly into consumer.py, __main__.py, and branch_protection.py. This couples the node to a specific CLI tool and makes testing impossible without shelling out.

Solution

New files:

• protocols.py — GitHubPrFetchProtocol for DI
• adapter_github_http.py — real HTTP client using urllib + GitHub GraphQL/REST API, reads GH_PAT (fail-fast, no fallback)
• tests/test_golden_chain_merge_sweep.py — 20 golden chain tests

Rewritten:

• branch_protection.py — accepts protocol, no longer shells out (was: gh api repos/.../branches/main/protection)
• consumer.py — uses GitHubHttpClient, no gh subprocess calls (was: gh pr list --json ...)
• __main__.py — same treatment

Architecture — two invocation paths

Path	Command	Transport
RuntimeLocal	uv run onex node node_merge_sweep	EventBusInmemory, handler is pure compute
Kafka consumer	python -m ...consumer	aiokafka + GitHubHttpClient via GH_PAT

The handler (NodeMergeSweep) is pure compute — zero network calls, zero subprocess calls. All network I/O is isolated in the adapter behind GitHubPrFetchProtocol.

Test coverage (20/20)

• Track classification: A-merge, A-update, A-resolve, B-polish, Skip
• Draft handling, review_bot_gate_passed, solo-dev approval bypass
• Failure history escalation: CHRONIC, RECIDIVIST, stuck thresholds
• max_total_merges cap, skip_polish flag
• Status: queued vs nothing_to_merge

Zero gh subprocess calls

$ grep -r "subprocess" src/omnimarket/nodes/node_merge_sweep/
# (only comments/docstrings remain)

[skip-deploy-gate: OMN-9393 deploy-agent RED — handler untouched, adapter/tests only]

Summary by CodeRabbit

• Refactor
  • Reimplemented the merge sweep node as a compute-based component with improved performance characteristics.
  • Upgraded node contract version to 2.0.0 and reduced operation timeout from 300s to 5s.
  • Changed node archetype from orchestrator to compute, marking as pure (read-only operations).
  • Migrated from legacy node_merge_sweep to node_merge_sweep_compute with updated entry points and metadata.

[coderabbitai]

Warning

Rate limit exceeded

@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 37 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 3 minutes and 37 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 036a868c-81f0-422c-9b80-62a6d429d78c

📥 Commits

Reviewing files that changed from the base of the PR and between 4e47480 and c55c57e.

📒 Files selected for processing (12)

• pyproject.toml
• src/omnimarket/nodes/node_merge_sweep_compute/adapter_github_http.py
• src/omnimarket/nodes/node_merge_sweep_compute/branch_protection.py
• src/omnimarket/nodes/node_merge_sweep_compute/consumer.py
• src/omnimarket/nodes/node_merge_sweep_compute/protocols.py
• src/omnimarket/nodes/node_pr_health_monitor/handlers/handler_pr_health_monitor.py
• src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_input.py
• src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_result.py
• tests/test_golden_chain_merge_sweep_executor.py
• tests/test_golden_chain_pr_health_monitor.py
• tests/test_merge_sweep_consumer.py
• tests/test_stall_detector.py

📝 Walkthrough

Walkthrough

This pull request refactors the merge sweep node from an event-driven orchestrator to a pure compute node, replacing subprocess-based GitHub CLI calls with HTTP-based API requests via a new GitHubHttpClient adapter. The node moves from node_merge_sweep to node_merge_sweep_compute, with updated protocol-driven architecture, simplified test coverage, and coordinated import updates across 15+ downstream consumers.

Changes

Cohort / File(s)	Summary
New Compute Package Structure src/omnimarket/nodes/node_merge_sweep_compute/__init__.py, src/omnimarket/nodes/node_merge_sweep_compute/__main__.py, src/omnimarket/nodes/node_merge_sweep_compute/contract.yaml, src/omnimarket/nodes/node_merge_sweep_compute/metadata.yaml	Added new node_merge_sweep_compute package with init export, refactored CLI entry point using GitHubHttpClient, updated contract metadata (orchestrator→compute, impure→pure, 300s→5s timeout), and new manifest defining node capabilities.
GitHub Adapter & Protocol src/omnimarket/nodes/node_merge_sweep_compute/protocols.py, src/omnimarket/nodes/node_merge_sweep_compute/adapter_github_http.py	Introduced GitHubPrFetchProtocol runtime-checkable protocol with fetch_open_prs and fetch_branch_protection methods. Implemented GitHubHttpClient concrete adapter using GraphQL for PR pagination and REST for branch protection, with defensive JSON parsing and fail-safe error handling.
Branch Protection Cache Refactoring src/omnimarket/nodes/node_merge_sweep/branch_protection.py, src/omnimarket/nodes/node_merge_sweep_compute/branch_protection.py	Removed subprocess-based gh api and JSON parsing from old cache; added new cache implementation accepting injected GitHubPrFetchProtocol. Both versions now delegate fetching to protocol, maintain per-repo lazy caching, and add debug logging.
Consumer Wiring Update src/omnimarket/nodes/node_merge_sweep_compute/consumer.py	Replaced _PR_FIELDS and _fetch_prs subprocess logic with GitHubHttpClient dependency injection. Updated _build_request to accept client parameter and instantiate cache with injected client.
Deleted Legacy Node Files src/omnimarket/nodes/node_merge_sweep/__init__.py, src/omnimarket/nodes/node_merge_sweep/metadata.yaml, omnibase_compat, omnibase_core	Removed old orchestrator node package exports, deprecated metadata manifest, and stale local path reference files.
Entry Point & Configuration Updates pyproject.toml	Updated onex.nodes entry point from node_merge_sweep to node_merge_sweep_compute targeting new package path.
Downstream Consumer Import Updates src/omnimarket/nodes/node_merge_sweep_triage_orchestrator/handlers/handler_triage.py, src/omnimarket/nodes/node_merge_sweep_triage_orchestrator/models/model_triage_request.py, src/omnimarket/nodes/node_overnight/handlers/handler_overnight.py, src/omnimarket/nodes/node_pr_health_monitor/handlers/handler_pr_health_monitor.py, src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_input.py, src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_result.py, src/omnimarket/nodes/node_pr_snapshot_effect/handlers/handler_pr_snapshot.py, src/omnimarket/nodes/node_pr_snapshot_effect/models/model_pr_snapshot_result.py, src/omnimarket/nodes/node_pr_snapshot_effect/models/model_repo_scan_result.py	Updated all import paths for ModelPRInfo, ModelMergeSweepRequest, ModelMergeSweepResult, and EnumPRTrack from old node_merge_sweep.handlers to new node_merge_sweep_compute.handlers module.
Test Suite Refactoring tests/test_golden_chain_merge_sweep.py, tests/test_golden_chain_merge_sweep_executor.py, tests/test_golden_chain_pr_health_monitor.py, tests/test_golden_chain_pr_snapshot_effect.py, tests/test_merge_sweep_consumer.py, tests/test_merge_sweep_review_bot_gate.py, tests/test_metadata_schema.py, tests/test_plugin_compat_handshake.py, tests/test_proof_runtime_local_merge_sweep.py, tests/test_stall_detector.py, tests/test_triage_orchestrator.py, tests/test_triage_phase2_emit_rules.py	Converted async golden-chain tests to synchronous direct handler verification. Replaced subprocess mocking with MagicMock stub GitHub client. Updated imports and contract paths to reference node_merge_sweep_compute. Updated metadata schema tests to assert compute role and non-deprecated status for new node.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

---

🐰 A compute node hops into view,
HTTP-bound, with protocol true,
No more subprocess dance, just pure,
The sweep now reaches every PR—
Five seconds swift, clean, and sure! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 42.03% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main refactoring work: replacing subprocess calls with an HTTP adapter and adding 20 golden chain tests.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jonah/omn-merge-sweep-http-client

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

⚠️ Hostile Reviewer — DEGRADED (informational)

Blocking findings (critical/error): 0
Total findings: 0
Models succeeded: none

Note: All reviewer models failed or were unavailable. Degraded results are informational during the pilot phase (OMN-8524) and do not block merge. Error: cli_review exited non-zero (all models failed or binary error)

---

Gate semantics (pilot phase)

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]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/omnimarket/nodes/node_merge_sweep/consumer.py (1)

197-221: ⚠️ Potential issue | 🟠 Major

Don't swallow GitHub fetch/build failures while offsets auto-commit.

_build_request() now does live GitHub I/O, but this branch still only logs exceptions and keeps going. With enable_auto_commit=True, that can acknowledge the command without emitting merge-sweep-completed, so transient GitHub failures turn into dropped sweeps instead of retries/DLQ handling.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/omnimarket/nodes/node_merge_sweep/consumer.py` around lines 197 - 221,
The current try/except around _build_request, handler.handle and
producer.send_and_wait swallows exceptions from _build_request (which now does
live GitHub I/O) and thus allows offsets to be auto-committed without emitting
merge-sweep-completed; change the flow so that failures in _build_request are
not caught/ignored: call _build_request outside the try/except (or if you must
catch it, log and re-raise) so any exception from _build_request propagates and
prevents offset commit/retry logic from being bypassed; keep the try/except only
around handler.handle and producer.send_and_wait (or ensure caught exceptions
are re-raised) and preserve correlation_id and TOPIC_MERGE_SWEEP_COMPLETED in
logs for diagnosis.

🤖 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_merge_sweep/adapter_github_http.py`:
- Around line 153-156: fetch_open_prs currently calls _split_repo(repo) which
can raise ValueError and escape the adapter's error handling; wrap the
_split_repo call in a try/except ValueError block at the top of fetch_open_prs
to catch malformed repo strings, log the error (including the offending repo
value) using the adapter's existing logging/failure pattern, and return [] so
the malformed input is skipped rather than aborting the sweep; reference the
fetch_open_prs method and the _split_repo function when making this change.
- Around line 64-70: The GraphQL document in adapter_github_http.py references
an undeclared variable "$unused" in the isRequired(...) calls (seen in the
CheckSuite and CheckRun fragments), which causes validation to fail and
_graphql() to return empty results; remove the "pullRequestNumber: $unused"
argument from those isRequired(...) calls (or declare and pass a real variable
if intended) so the operation uses only the declared variables ($owner, $name,
$after) and validates correctly.

In `@src/omnimarket/nodes/node_merge_sweep/protocols.py`:
- Around line 19-33: The current contracts for fetch_open_prs(repo) and
fetch_branch_protection(repo) swallow transport/HTTP errors by returning [] or
None, making outages indistinguishable from valid empty/none states; change
these functions to propagate transport failures (raise a specific exception or
return an error result) instead of returning []/None on network/403/timeout
errors, and only return [] for an authenticated empty PR list or return None
when branch protection genuinely reports "no protection" (explicit GitHub
200/404 semantics). Update fetch_open_prs and fetch_branch_protection to detect
HTTP/transport error codes/timeouts and surface them (throw or return an error
type) so callers can fail closed; ensure existing callers handle the
exception/error result path instead of assuming empty list/None mean
safe-to-proceed.

---

Outside diff comments:
In `@src/omnimarket/nodes/node_merge_sweep/consumer.py`:
- Around line 197-221: The current try/except around _build_request,
handler.handle and producer.send_and_wait swallows exceptions from
_build_request (which now does live GitHub I/O) and thus allows offsets to be
auto-committed without emitting merge-sweep-completed; change the flow so that
failures in _build_request are not caught/ignored: call _build_request outside
the try/except (or if you must catch it, log and re-raise) so any exception from
_build_request propagates and prevents offset commit/retry logic from being
bypassed; keep the try/except only around handler.handle and
producer.send_and_wait (or ensure caught exceptions are re-raised) and preserve
correlation_id and TOPIC_MERGE_SWEEP_COMPLETED in logs for diagnosis.

🪄 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: 013cef20-03ee-4bfd-bcb0-589acd5b9d61

📥 Commits

Reviewing files that changed from the base of the PR and between 86dc374 and 05904ca.

📒 Files selected for processing (6)

• src/omnimarket/nodes/node_merge_sweep/__main__.py
• src/omnimarket/nodes/node_merge_sweep/adapter_github_http.py
• src/omnimarket/nodes/node_merge_sweep/branch_protection.py
• src/omnimarket/nodes/node_merge_sweep/consumer.py
• src/omnimarket/nodes/node_merge_sweep/protocols.py
• tests/test_golden_chain_merge_sweep.py

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

tests/test_metadata_schema.py (1)

64-70: Prefer a neutral fixture node name in this deprecation parser test.

Using an active node identifier (node_merge_sweep_compute) in a deprecated=True fixture can confuse future readers. A synthetic name (e.g., node_legacy_example) keeps intent clearer.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_metadata_schema.py` around lines 64 - 70, Update the fixture dict
used in the deprecation parser test that currently sets "name":
"node_merge_sweep_compute" to a neutral synthetic identifier such as
"node_legacy_example" to avoid implying an active node; locate the dict entry
with keys "name", "version", "description", "deprecated", "deprecated_by", and
"deprecated_reason" (the record that includes "deprecated": True) and replace
only the name string while leaving the other keys and values unchanged so the
test intent remains the same.

src/omnimarket/nodes/node_merge_sweep_compute/__main__.py (1)

62-90: Consider extracting shared helpers to reduce duplication.

The _is_green() and _to_pr_info() functions are duplicated between __main__.py and consumer.py. Both files contain identical logic for normalizing PR data. Consider extracting these to a shared module (e.g., pr_normalization.py or within the handler module) to maintain a single source of truth.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/omnimarket/nodes/node_merge_sweep_compute/__main__.py` around lines 62 -
90, Extract the duplicated PR normalization logic from __main__.py and
consumer.py into a new shared module (e.g., pr_normalization.py) that exposes
the helper functions _is_green and _to_pr_info (and export a ModelPRInfo
import/alias if needed); replace the local implementations in both files with
imports from that module, keep the function signatures (including parameters
like pr: dict[str, Any], repo: str, required_approving: int | None) and behavior
identical, update any imports/usages (e.g., references to ModelPRInfo and
_is_green) in both files, and run/adjust type imports so mypy/typing and tests
still pass.

🤖 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_merge_sweep_compute/adapter_github_http.py`:
- Around line 171-174: The flattening loop in adapter_github_http.py currently
uses ln["name"] which can raise KeyError for malformed label nodes; update the
comprehension inside the for node in nodes loop to safely access the label name
(e.g., use ln.get("name")) and filter out missing/None/empty names so the code
never raises—modify the expression that builds node["labels"] (the list
comprehension over label_nodes) to use ln.get("name") and only include entries
with a truthy name.

---

Nitpick comments:
In `@src/omnimarket/nodes/node_merge_sweep_compute/__main__.py`:
- Around line 62-90: Extract the duplicated PR normalization logic from
__main__.py and consumer.py into a new shared module (e.g., pr_normalization.py)
that exposes the helper functions _is_green and _to_pr_info (and export a
ModelPRInfo import/alias if needed); replace the local implementations in both
files with imports from that module, keep the function signatures (including
parameters like pr: dict[str, Any], repo: str, required_approving: int | None)
and behavior identical, update any imports/usages (e.g., references to
ModelPRInfo and _is_green) in both files, and run/adjust type imports so
mypy/typing and tests still pass.

In `@tests/test_metadata_schema.py`:
- Around line 64-70: Update the fixture dict used in the deprecation parser test
that currently sets "name": "node_merge_sweep_compute" to a neutral synthetic
identifier such as "node_legacy_example" to avoid implying an active node;
locate the dict entry with keys "name", "version", "description", "deprecated",
"deprecated_by", and "deprecated_reason" (the record that includes "deprecated":
True) and replace only the name string while leaving the other keys and values
unchanged so the test intent remains the same.

🪄 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: 135651bb-f134-46da-9e39-7a2136eb09f0

📥 Commits

Reviewing files that changed from the base of the PR and between 05904ca and 4e47480.

📒 Files selected for processing (37)

• omnibase_compat
• omnibase_core
• pyproject.toml
• src/omnimarket/nodes/node_merge_sweep/__init__.py
• src/omnimarket/nodes/node_merge_sweep/metadata.yaml
• src/omnimarket/nodes/node_merge_sweep_compute/__init__.py
• src/omnimarket/nodes/node_merge_sweep_compute/__main__.py
• src/omnimarket/nodes/node_merge_sweep_compute/adapter_github_http.py
• src/omnimarket/nodes/node_merge_sweep_compute/branch_protection.py
• src/omnimarket/nodes/node_merge_sweep_compute/consumer.py
• src/omnimarket/nodes/node_merge_sweep_compute/contract.yaml
• src/omnimarket/nodes/node_merge_sweep_compute/handlers/__init__.py
• src/omnimarket/nodes/node_merge_sweep_compute/handlers/handler_merge_sweep.py
• src/omnimarket/nodes/node_merge_sweep_compute/metadata.yaml
• src/omnimarket/nodes/node_merge_sweep_compute/protocols.py
• src/omnimarket/nodes/node_merge_sweep_compute/tests/__init__.py
• src/omnimarket/nodes/node_merge_sweep_triage_orchestrator/handlers/handler_triage.py
• src/omnimarket/nodes/node_merge_sweep_triage_orchestrator/models/model_triage_request.py
• src/omnimarket/nodes/node_overnight/handlers/handler_overnight.py
• src/omnimarket/nodes/node_pr_health_monitor/handlers/handler_pr_health_monitor.py
• src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_input.py
• src/omnimarket/nodes/node_pr_health_monitor/models/model_pr_health_result.py
• src/omnimarket/nodes/node_pr_snapshot_effect/handlers/handler_pr_snapshot.py
• src/omnimarket/nodes/node_pr_snapshot_effect/models/model_pr_snapshot_result.py
• src/omnimarket/nodes/node_pr_snapshot_effect/models/model_repo_scan_result.py
• tests/test_golden_chain_merge_sweep.py
• tests/test_golden_chain_merge_sweep_executor.py
• tests/test_golden_chain_pr_health_monitor.py
• tests/test_golden_chain_pr_snapshot_effect.py
• tests/test_merge_sweep_consumer.py
• tests/test_merge_sweep_review_bot_gate.py
• tests/test_metadata_schema.py
• tests/test_plugin_compat_handshake.py
• tests/test_proof_runtime_local_merge_sweep.py
• tests/test_stall_detector.py
• tests/test_triage_orchestrator.py
• tests/test_triage_phase2_emit_rules.py

💤 Files with no reviewable changes (4)

• omnibase_core
• src/omnimarket/nodes/node_merge_sweep/metadata.yaml
• omnibase_compat
• src/omnimarket/nodes/node_merge_sweep/init.py

✅ Files skipped from review due to trivial changes (13)

• tests/test_plugin_compat_handshake.py
• tests/test_golden_chain_pr_snapshot_effect.py
• src/omnimarket/nodes/node_merge_sweep_triage_orchestrator/handlers/handler_triage.py
• tests/test_golden_chain_pr_health_monitor.py
• tests/test_triage_orchestrator.py
• src/omnimarket/nodes/node_pr_snapshot_effect/handlers/handler_pr_snapshot.py
• tests/test_stall_detector.py
• src/omnimarket/nodes/node_pr_health_monitor/handlers/handler_pr_health_monitor.py
• src/omnimarket/nodes/node_merge_sweep_compute/init.py
• src/omnimarket/nodes/node_pr_snapshot_effect/models/model_pr_snapshot_result.py
• tests/test_golden_chain_merge_sweep_executor.py
• src/omnimarket/nodes/node_pr_snapshot_effect/models/model_repo_scan_result.py
• src/omnimarket/nodes/node_merge_sweep_compute/metadata.yaml

🚧 Files skipped from review as they are similar to previous changes (1)

• tests/test_golden_chain_merge_sweep.py

[jonahgabriel]

Re-running CodeRabbit Thread Check — all 4 threads resolved, fixes pushed.