Repository navigation
fix(OMN-13994): registry-aware ONEX_CC_REPO_PATH fallback in integration_sweep/dod_sweep - #1624
Conversation
…ion_sweep/dod_sweep Both sweep orchestrators resolve their contracts/artifact root from ONEX_CC_REPO_PATH via _resolve_root. A stale/container value (the in-container mount /onex_change_control) leaks into local infra-venv runs where it has no contracts/ dir, so _resolve_root raised and the sweep never ran. Fall back to the canonical registry clone at $OMNI_HOME/onex_change_control when the env path lacks contracts/, before raising. Fail-fast (CLAUDE.md rule #8) is preserved: os.environ["OMNI_HOME"] raises KeyError when unset (never a silent default), and a fallback that itself lacks contracts/ still raises RuntimeError. The 'ONEX_CC_REPO_PATH not set at all' branch is unchanged. Parametrized tests cover both handlers: OMNI_HOME registry fallback (behavioral) + unset-OMNI_HOME fail-fast.
📝 WalkthroughWalkthroughBoth DoD and Integration sweep orchestrator handlers' ChangesResolve Root Fallback Behavior
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ResolveRoot as "_resolve_root"
participant Env as "Environment (ONEX_CC_REPO_PATH, OMNI_HOME)"
participant FS as "Filesystem"
Caller->>ResolveRoot: request root path
ResolveRoot->>Env: read ONEX_CC_REPO_PATH
ResolveRoot->>FS: check contracts/ at resolved path
alt contracts/ found
ResolveRoot-->>Caller: return resolved path
else contracts/ missing
ResolveRoot->>Env: read OMNI_HOME
ResolveRoot->>FS: check contracts/ at OMNI_HOME/onex_change_control
alt fallback contracts/ found
ResolveRoot-->>Caller: return fallback path
else fallback also missing
ResolveRoot-->>Caller: raise RuntimeError (both paths)
end
end
Related Issues: None referenced in the provided context. Related PRs: None referenced in the provided context. Suggested labels: bug, tests Suggested reviewers: None specified in the provided context. 🐰 A path once lost in a container's maze, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.py:58: self._runtime_sha_handler = runtime_sha_handler or HandlerRuntimeShaVerify()\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) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_dod_sweep_orchestrator/handlers/handler_dod_sweep_orchestrator.py (1)
925-951: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicated
_resolve_rootfallback logic across two node handlers.This entire fallback implementation (env-path check, OMNI_HOME fallback, dual
RuntimeError) is byte-for-byte duplicated inhandler_integration_sweep_orchestrator.py(differing only in the final "not set" error message wording). The test file itself acknowledges this: "The two handlers duplicate an identical_resolve_rootstaticmethod." Any future fix/behavior change to this fail-fast contract now requires synchronized edits in two places, risking drift.Consider promoting this to a shared helper (e.g. a small function in a common/shared module accessible to both nodes) that both handlers call, parameterizing only the field name used in the "not set" error message.
♻️ Suggested extraction sketch
- `@staticmethod` - def _resolve_root(configured: str) -> Path: - if configured: - return Path(configured).expanduser().resolve() - env_root = os.environ.get("ONEX_CC_REPO_PATH") # contract-config-ok: config # fmt: skip - if env_root: - resolved = Path(env_root).expanduser().resolve() - if resolved.exists() and (resolved / "contracts").is_dir(): - return resolved - fallback = ( - Path(os.environ["OMNI_HOME"]).expanduser().resolve() - / "onex_change_control" - ) - if fallback.exists() and (fallback / "contracts").is_dir(): - return fallback - raise RuntimeError( - f"ONEX_CC_REPO_PATH={env_root!r} resolves to {resolved} which " - "does not exist or lacks a contracts/ directory, and the " - f"OMNI_HOME fallback {fallback} also lacks a contracts/ directory" - ) - raise RuntimeError( - "ONEX_CC_REPO_PATH is not set and no explicit contract_root/evidence_root was provided. " - "Set ONEX_CC_REPO_PATH to the omni_home repo registry path." - ) + `@staticmethod` + def _resolve_root(configured: str) -> Path: + return resolve_onex_cc_root(configured, unset_hint="contract_root/evidence_root")As per coding guidelines,
src/omnimarket/nodes/node_*/**/*: "Keep node logic insidesrc/omnimarket/nodes/node_*... promote shared types instead" of cross-node 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_dod_sweep_orchestrator/handlers/handler_dod_sweep_orchestrator.py` around lines 925 - 951, The `_resolve_root` fallback logic is duplicated in the two sweep orchestrator handlers, so extract the shared env-path/OMNI_HOME resolution and dual RuntimeError handling into a common helper that both handlers call. Keep the handler-specific “not set” wording parameterized, and update the existing `_resolve_root` staticmethods in `handler_dod_sweep_orchestrator.py` and `handler_integration_sweep_orchestrator.py` to delegate to that shared function.Source: Coding guidelines
🧹 Nitpick comments (1)
src/omnimarket/nodes/node_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.py (1)
325-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_resolve_rootacross two node handlers.This entire
_resolve_rootimplementation (including the new fallback branch, comments, and error messages) is byte-for-byte duplicated inHandlerDodSweepOrchestrator._resolve_rootinnode_dod_sweep_orchestrator. Since node-local logic shouldn't cross-import between nodes, the guideline path forward is to promote this shared resolution logic into a common module (e.g. a shared utility under a non-node-specific package) that both handlers import, rather than maintaining two independently-evolving copies of the same fail-fast/fallback contract.As per coding guidelines,
src/omnimarket/nodes/node_*/**/*: "Do not make one node import another node's private handler or model package; promote shared types instead."♻️ Suggested extraction
+# src/omnimarket/shared/resolve_contract_root.py +import os +from pathlib import Path + + +def resolve_contract_root(configured: str) -> Path: + if configured: + return Path(configured).expanduser().resolve() + env_root = os.environ.get("ONEX_CC_REPO_PATH") # contract-config-ok: config # fmt: skip + if env_root: + resolved = Path(env_root).expanduser().resolve() + if resolved.exists() and (resolved / "contracts").is_dir(): + return resolved + fallback = ( + Path(os.environ["OMNI_HOME"]).expanduser().resolve() + / "onex_change_control" + ) + if fallback.exists() and (fallback / "contracts").is_dir(): + return fallback + raise RuntimeError( + f"ONEX_CC_REPO_PATH={env_root!r} resolves to {resolved} which " + "does not exist or lacks a contracts/ directory, and the " + f"OMNI_HOME fallback {fallback} also lacks a contracts/ directory" + ) + raise RuntimeError( + "ONEX_CC_REPO_PATH is not set and no explicit root was provided. " + "Set ONEX_CC_REPO_PATH to the omni_home repo registry path." + )Both handlers would then call
resolve_contract_root(...)instead of maintaining their own_resolve_rootstaticmethod.🤖 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_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.py` around lines 325 - 356, The `_resolve_root` logic in `HandlerIntegrationSweepOrchestrator` is duplicated in `HandlerDodSweepOrchestrator`, so extract this shared contract-root resolution into a common utility module that both handlers can import instead of keeping two copies. Move the fallback, fail-fast checks, and error messaging into a single shared function such as `resolve_contract_root(...)`, then update each handler to call it from their `_resolve_root`/root-resolution path and remove the duplicated implementation.Source: Coding guidelines
🤖 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.
Outside diff comments:
In
`@src/omnimarket/nodes/node_dod_sweep_orchestrator/handlers/handler_dod_sweep_orchestrator.py`:
- Around line 925-951: The `_resolve_root` fallback logic is duplicated in the
two sweep orchestrator handlers, so extract the shared env-path/OMNI_HOME
resolution and dual RuntimeError handling into a common helper that both
handlers call. Keep the handler-specific “not set” wording parameterized, and
update the existing `_resolve_root` staticmethods in
`handler_dod_sweep_orchestrator.py` and
`handler_integration_sweep_orchestrator.py` to delegate to that shared function.
---
Nitpick comments:
In
`@src/omnimarket/nodes/node_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.py`:
- Around line 325-356: The `_resolve_root` logic in
`HandlerIntegrationSweepOrchestrator` is duplicated in
`HandlerDodSweepOrchestrator`, so extract this shared contract-root resolution
into a common utility module that both handlers can import instead of keeping
two copies. Move the fallback, fail-fast checks, and error messaging into a
single shared function such as `resolve_contract_root(...)`, then update each
handler to call it from their `_resolve_root`/root-resolution path and remove
the duplicated implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98fb346d-adb8-4f2d-8d82-50a59da1af72
📒 Files selected for processing (3)
src/omnimarket/nodes/node_dod_sweep_orchestrator/handlers/handler_dod_sweep_orchestrator.pysrc/omnimarket/nodes/node_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.pytests/test_resolve_root_fail_fast.py
|
| 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
Summary
Makes the
integration_sweepanddod_sweeporchestrator skills runnable from the canonical infra venv. Both resolve their contracts/artifact root fromONEX_CC_REPO_PATHvia_resolve_root. A stale/container value (the in-container mount/onex_change_control) leaks into local runs where it has nocontracts/dir on the host, so_resolve_rootraisedRuntimeErrorand the sweep hard-failed before doing any work.Fix
In
_resolve_root(both handlers), whenONEX_CC_REPO_PATHis set but the resolved dir lacks acontracts/subdir, fall back toPath(os.environ["OMNI_HOME"]) / "onex_change_control"(the canonical registry clone) before raising.src/omnimarket/nodes/node_integration_sweep_orchestrator/handlers/handler_integration_sweep_orchestrator.pysrc/omnimarket/nodes/node_dod_sweep_orchestrator/handlers/handler_dod_sweep_orchestrator.pyFail-fast preserved (CLAUDE.md rule #8)
os.environ["OMNI_HOME"]— raisesKeyErrorwhen unset, never a silent default.OMNI_HOMEis unset (or its registry lackscontracts/) and the env path is invalid, resolution still raises.Validate no silent env fallbackspre-commit hook passes.Proof (behavioral, local)
With
ONEX_CC_REPO_PATH=/onex_change_control(container leak, no hostcontracts/) andOMNI_HOMEset, both handlers now resolve to$OMNI_HOME/onex_change_controlinstead of raising:Tests
tests/test_resolve_root_fail_fast.pyparametrized across BOTH handlers (16 cases, all green):onex_change_control/contracts→ still raisesdod_evidence
topic-naming-lintneedsOMNIBASE_INFRA_PATHdue to worktree nesting and passes when set — this change touches no topics).Closes OMN-13994
Summary by CodeRabbit
Bug Fixes
Tests
Evidence-Source: OCC#3631
Evidence-Ticket: OMN-13994