Repository navigation
feat(OMN-2136): RRH nodes — emit effect, validate compute, storage effect - #309
Merged
jonahgabriel merged 20 commits intoFeb 12, 2026
Conversation
…fect Implement the three core Release Readiness Handshake nodes: - node_rrh_emit_effect (EFFECT): Collects repo state, runtime targets, and toolchain versions via 3 handlers - node_rrh_validate_compute (COMPUTE): Pure 13-rule validation engine with 4 profiles (default, ticket-pipeline, ci-repair, seam-ticket) and contract tightening enforcement - node_rrh_storage_effect (EFFECT): Writes JSON artifacts with latest_by_ticket and latest_by_repo symlinks Profiles are defined as Python constants (not YAML) to keep the COMPUTE handler pure and avoid ONEX contract validator collisions. Shared RRH models (ModelRRHResult, ModelRRHEnvironmentData, etc.) compose existing primitives (ModelRuleCheckResult, EnumVerdict) for uniform dashboard consumption. 46 tests covering all rules, profiles, tightening, verdicts, storage, and symlinks. 11,332 tests pass with zero regressions.
Use Callable[..., ModelRuleCheckResult] for the rule dispatcher dict so mypy knows the return type without referencing TYPE_CHECKING-only imports at runtime.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Release Readiness Handshake (RRH): new immutable Pydantic RRH models, three ONEX nodes (emit, validate, storage) with handlers, DI registries, contracts, validation exemptions, and extensive unit & integration tests for collection, validation (13 rules), and artifact persistence. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Emit as NodeRRHEmitEffect
participant Validate as NodeRRHValidateCompute
participant Storage as NodeRRHStorageEffect
participant Disk as Filesystem
Client->>Emit: ModelRRHEmitRequest (repo_path, env, kafka, k8s)
Emit->>Emit: HandlerRepoStateCollect (git)
Emit->>Emit: HandlerRuntimeTargetCollect (env/overrides)
Emit->>Emit: HandlerToolchainCollect (tool --version)
Emit-->>Validate: ModelRRHEnvironmentData (repo_state, runtime, toolchain, timestamp)
Client->>Validate: ModelRRHValidateRequest (env_data, profile_name, governance)
Validate->>Validate: Load profile, apply governance tightening
Validate->>Validate: Execute 13 RRH rules -> derive verdict
Validate-->>Storage: ModelRRHResult (checks[], verdict, metadata)
Client->>Storage: ModelRRHStorageRequest (result, output_dir)
Storage->>Disk: Write artifact JSON (artifacts/...)
Storage->>Disk: Update symlinks (latest_by_ticket, latest_by_repo)
Storage-->>Client: ModelRRHStorageResult (artifact_path, symlinks, success)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_rrh_emit_effect/handlers/handler_repo_state_collect.py`:
- Around line 84-93: Sanitize any git error output before logging: replace the
direct stderr.decode(...) and exc passed to logger.debug in
handler_repo_state_collect (the two logger.debug calls that log "git %s
failed..." and "git %s error: %s") with values run through the sanitization
utilities from omnibase_infra.utils.util_error_sanitization (import the
appropriate sanitize function and call it on
stderr.decode(errors="replace").strip() and on exc or str(exc) before logging)
so no passwords, API keys, PII, or credential-bearing URLs are emitted.
- Around line 53-62: Remote URLs returned by self._git(...) may contain embedded
credentials; sanitize remote_url before storing it in ModelRRHRepoState. Update
handler_repo_state_collect to strip userinfo from remote_url (reuse existing
_sanitize_url_for_logging() from util_pydantic_validators.py or add a private
helper _sanitize_remote_url(remote_url) that removes
urlparse().password/userinfo) and use the sanitized value in the
ModelRRHRepoState(remote_url=...) construction; keep other fields the same and
ensure trimming (.strip()) is applied after sanitization.
- Around line 74-93: The git subprocess started into variable proc in
handler_repo_state_collect can be left running when asyncio.wait_for times out;
modify the try/except/finally so that on TimeoutError (and in the cancellation
path) you explicitly terminate the subprocess (call proc.kill() or
proc.terminate()), then await proc.wait() to reap it before returning/raising;
ensure this cleanup references the existing proc variable created by
asyncio.create_subprocess_exec and runs whether the wait_for timed out or an
OSError/FileNotFoundError occurred so no orphaned git processes remain.
🧹 Nitpick comments (8)
src/omnibase_infra/models/rrh/model_rrh_result.py (1)
17-17: Remove unused import.The
datetimeimport is unused—AwareDatetimeis imported frompydanticon line 20.🧹 Proposed fix
-from datetime import datetime from uuid import UUID, uuid4src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py (1)
132-135: Non-atomic symlink replacement has a small race window.The unlink-then-symlink sequence creates a brief window where the symlink doesn't exist. For concurrent readers, this could cause a
FileNotFoundError. Consider using a temporary symlink with atomic rename for true atomicity:♻️ Optional: Atomic symlink replacement pattern
- # Atomic replace: unlink then symlink. - if link_path.is_symlink() or link_path.exists(): - link_path.unlink() - link_path.symlink_to(rel_target) + # Atomic replace via temporary symlink + rename. + import tempfile + tmp_link = Path(tempfile.mktemp(dir=symlink_dir)) + tmp_link.symlink_to(rel_target) + tmp_link.rename(link_path)For local RRH artifact storage this is likely acceptable as-is, but worth noting if concurrent access becomes a concern.
tests/unit/nodes/test_node_rrh_emit_effect.py (4)
11-11: Unused imports detected.
AsyncMockandMagicMockare imported but not used in this test module.🧹 Remove unused imports
-from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import patch
89-99: Integration-style test in unit test file.This test hits the real filesystem and git repository. While the docstring acknowledges this ("Integration-style test"), consider moving it to an integration test file or marking it appropriately to clarify its nature, as it depends on external state.
101-106: Unnecessary async marker on synchronous property test.
test_handler_typeonly accesses synchronous properties (handler_type,handler_category) but is marked with@pytest.mark.anyio. The marker is unnecessary here.🧹 Remove unnecessary async marker
- `@pytest.mark.anyio` - async def test_handler_type(self, handler: HandlerRepoStateCollect) -> None: + def test_handler_type(self, handler: HandlerRepoStateCollect) -> None: from omnibase_infra.enums import EnumHandlerType, EnumHandlerTypeCategory assert handler.handler_type == EnumHandlerType.INFRA_HANDLER assert handler.handler_category == EnumHandlerTypeCategory.EFFECT
166-171: Unnecessary async marker on synchronous property test.Same as above —
test_handler_typeforHandlerToolchainCollectonly checks synchronous properties.🧹 Remove unnecessary async marker
- `@pytest.mark.anyio` - async def test_handler_type(self, handler: HandlerToolchainCollect) -> None: + def test_handler_type(self, handler: HandlerToolchainCollect) -> None: from omnibase_infra.enums import EnumHandlerType, EnumHandlerTypeCategory assert handler.handler_type == EnumHandlerType.INFRA_HANDLER assert handler.handler_category == EnumHandlerTypeCategory.EFFECTsrc/omnibase_infra/nodes/node_rrh_validate_compute/handlers/handler_rrh_validate.py (2)
348-355: Kafka broker format validation may be overly restrictive.The regex
[a-zA-Z0-9.\-]+:\d+doesn't allow underscores in hostnames or IPv6 addresses (e.g.,[::1]:9092). If IPv6 or underscore-containing hostnames are valid use cases, consider expanding the pattern.
459-496: RRH-1701 repo boundary check has edge cases.The check compares the local directory name with the remote URL's repo name. This can produce false positives when:
- The local clone was renamed (common for forks or multiple clones)
- The remote URL uses a different casing
The current behavior is documented and may be intentional for strict boundary enforcement. Consider documenting these limitations in the rule description if not already covered elsewhere.
…, unused import - Add ReDoS heuristic guard (nested quantifier detection + length cap) to RRH-1002 branch pattern matching in handler_rrh_validate.py - Fix 3 emit-effect handlers to import from canonical modules instead of model_rrh_environment_data (which doesn't export sub-models in __all__) - Remove unused `from datetime import datetime` in model_rrh_result.py - Export ModelRRHRuleSeverity from rrh/__init__.py and models/__init__.py - Fix test import to use canonical model_rrh_contract_governance module Review iteration: 1/10
… [minor/nitpick] atomic symlinks, Kafka regex, RRH-1701 SSH, unused imports, test markers
…tests + [minor] canonical imports, Kafka port range, IPv6 comment, test marker
- Fixed: handler_rrh_storage_write.py:106 - use sanitize_error_message() instead of raw str(exc)
- Fixed: test_node_rrh_validate_compute.py - add fail-path tests for RRH-1102, 1301, 1401, 1402, 1404, 1701
- Fixed: test_node_rrh_emit_effect.py:16 - import from canonical omnibase_infra.models.rrh package
- Fixed: test_node_rrh_validate_compute.py:23 - import from canonical omnibase_infra.models.rrh package
- Fixed: handler_rrh_validate.py:366 - validate TCP port range 1-65535 instead of regex-only \d{1,5}
- Fixed: handler_rrh_validate.py:364 - soften comment to not overpromise IPv6 bracket notation support
- Fixed: test_node_rrh_emit_effect.py:89 - remove misleading @pytest.mark.integration from unit test
Review iteration: 1/10
- Fixed: handler_toolchain_collect.py:85 - separate TimeoutError handling to kill orphaned subprocess with proc.kill()/await proc.wait(), matching the pattern already used in handler_repo_state_collect.py Review iteration: 2/10
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py`:
- Around line 102-108: The catch block currently logs the raw exception with
exc_info=True; replace that logger.warning call to log the sanitized message by
passing sanitize_error_message(exc) (e.g., logger.warning("RRH storage write
failed: %s", sanitize_error_message(exc))) and remove exc_info=True so
tracebacks aren't emitted; keep returning ModelRRHStorageResult with
error=sanitize_error_message(exc) and preserving
correlation_id=request.correlation_id.
- Around line 50-100: The handler uses request.result.correlation_id when
composing the artifact filename but returns ModelRRHStorageResult with
request.correlation_id; to make correlation IDs consistent, change the filename
construction in handle to use request.correlation_id (instead of
request.result.correlation_id) so the same correlation ID is propagated; update
the reference used when building filename (variable filename / ts / cid
assembly) to derive cid from request.correlation_id and leave all other uses of
request.correlation_id in ModelRRHStorageResult and error paths unchanged.
In
`@src/omnibase_infra/nodes/node_rrh_validate_compute/handlers/handler_rrh_validate.py`:
- Around line 166-180: The _load_profile function currently calls
get_profile(name) and lets any exception bubble; modify _load_profile to capture
exceptions from get_profile and re-raise a
ModelInfraErrorContext.with_correlation(...) wrapped exception that includes the
request correlation_id, transport_type and operation metadata. Locate the static
method _load_profile and either add a correlation_id parameter (or fetch the
current request correlation ID from the request/context object used elsewhere),
then wrap the get_profile call in a try/except and on error call
ModelInfraErrorContext.with_correlation(correlation_id, transport_type=...,
operation=...) to produce the error context before raising; keep the return type
as ModelRRHProfile.
- Around line 348-379: The failure message in _check_1201_kafka_reachable
currently injects env.runtime_target.kafka_broker verbatim which may contain
credentials; update the code to call the sanitization utility from
omnibase_infra.utils.util_error_sanitization (e.g., the provided
sanitize/sanitise function) on the broker string before including it in the
ModelRuleCheckResult message so secrets are redacted while preserving a
human-readable identifier.
Comment on lines
+102
to
+108
| except Exception as exc: | ||
| logger.warning("RRH storage write failed: %s", exc, exc_info=True) | ||
| return ModelRRHStorageResult( | ||
| artifact_path="", | ||
| success=False, | ||
| error=sanitize_error_message(exc), | ||
| correlation_id=request.correlation_id, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py | sed -n '1,20p'Repository: OmniNode-ai/omnibase_infra
Length of output: 703
🏁 Script executed:
cat -n src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py | sed -n '95,115p'Repository: OmniNode-ai/omnibase_infra
Length of output: 907
🏁 Script executed:
head -50 src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py | grep -E "^[0-9]+\s+(import|from)"Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
head -40 src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 1194
🏁 Script executed:
rg "from.*sanitize_error_message|import.*sanitize_error_message" src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.pyRepository: OmniNode-ai/omnibase_infra
Length of output: 149
🏁 Script executed:
rg "sanitize_error_message" src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/ -A 2 -B 2Repository: OmniNode-ai/omnibase_infra
Length of output: 1274
🏁 Script executed:
rg "ModelInfraErrorContext|with_correlation" src/omnibase_infra/nodes/node_rrh_storage_effect/ -A 2 -B 2Repository: OmniNode-ai/omnibase_infra
Length of output: 52
🏁 Script executed:
rg "ModelInfraErrorContext|with_correlation" src/omnibase_infra/nodes/ --max-count=5 -lRepository: OmniNode-ai/omnibase_infra
Length of output: 1677
🏁 Script executed:
cat -n src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py | sed -n '60,130p'Repository: OmniNode-ai/omnibase_infra
Length of output: 3120
Sanitize the logged error message.
Line 103 logs the raw exception with exc_info=True, which exposes the full exception details and traceback in logs—risking exposure of passwords, connection strings, or other sensitive data. Apply sanitization consistent with line 107 by using sanitize_error_message(exc) in the log statement and omit exc_info.
🔧 Proposed fix
- logger.warning("RRH storage write failed: %s", exc, exc_info=True)
+ logger.warning(
+ "RRH storage write failed: %s",
+ sanitize_error_message(exc),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except Exception as exc: | |
| logger.warning("RRH storage write failed: %s", exc, exc_info=True) | |
| return ModelRRHStorageResult( | |
| artifact_path="", | |
| success=False, | |
| error=sanitize_error_message(exc), | |
| correlation_id=request.correlation_id, | |
| except Exception as exc: | |
| logger.warning( | |
| "RRH storage write failed: %s", | |
| sanitize_error_message(exc), | |
| ) | |
| return ModelRRHStorageResult( | |
| artifact_path="", | |
| success=False, | |
| error=sanitize_error_message(exc), | |
| correlation_id=request.correlation_id, |
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py`
around lines 102 - 108, The catch block currently logs the raw exception with
exc_info=True; replace that logger.warning call to log the sanitized message by
passing sanitize_error_message(exc) (e.g., logger.warning("RRH storage write
failed: %s", sanitize_error_message(exc))) and remove exc_info=True so
tracebacks aren't emitted; keep returning ModelRRHStorageResult with
error=sanitize_error_message(exc) and preserving
correlation_id=request.correlation_id.
Comment on lines
+166
to
+180
| @staticmethod | ||
| def _load_profile(name: str) -> ModelRRHProfile: | ||
| """Retrieve a built-in RRH profile by name. | ||
|
|
||
| Args: | ||
| name: Profile name (``default``, ``ticket-pipeline``, | ||
| ``ci-repair``, ``seam-ticket``). | ||
|
|
||
| Returns: | ||
| The matching ``ModelRRHProfile``. | ||
|
|
||
| Raises: | ||
| KeyError: If the profile name is not recognized. | ||
| """ | ||
| return get_profile(name) |
There was a problem hiding this comment.
Wrap unknown profile errors with correlation-aware error context.
If get_profile() raises, the error currently bubbles without ModelInfraErrorContext.with_correlation() and transport/operation metadata. Please wrap that failure path using the request correlation_id.
As per coding guidelines, Use MANDATORY error context factory ModelInfraErrorContext.with_correlation() for all exception raises with transport_type and operation parameters.
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_validate_compute/handlers/handler_rrh_validate.py`
around lines 166 - 180, The _load_profile function currently calls
get_profile(name) and lets any exception bubble; modify _load_profile to capture
exceptions from get_profile and re-raise a
ModelInfraErrorContext.with_correlation(...) wrapped exception that includes the
request correlation_id, transport_type and operation metadata. Locate the static
method _load_profile and either add a correlation_id parameter (or fetch the
current request correlation ID from the request/context object used elsewhere),
then wrap the get_profile call in a try/except and on error call
ModelInfraErrorContext.with_correlation(correlation_id, transport_type=...,
operation=...) to produce the error context before raising; keep the return type
as ModelRRHProfile.
…errors, document sync handle - Replace weak path-traversal guard with Path.name + allowlist regex - Apply sanitize_error_string to toolchain handler error logging - Document intentional sync handle() on pure COMPUTE handler
…econd filenames, catch unknown profile, test no-remote, integration marker - Cache _rule_dispatcher as cached_property (avoid 13x rebuild) - Truncate displayed regex patterns to 80 chars in RRH-1002 errors - Add microseconds to artifact filenames to prevent collision - Catch KeyError for unknown profile names with clean FAIL result - Add test for git repo with no remote configured - Mark test_collects_from_real_repo as integration
…t fallback - Reject '.' and '..' after symlink name sanitization - Return empty string when version regex has no match (was leaking raw stdout)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/unit/nodes/test_node_rrh_emit_effect.py`:
- Around line 90-101: The test function test_collects_from_real_repo (using
HandlerRepoStateCollect and asserting ModelRRHRepoState) is an integration test
but lives under tests/unit; move the entire test function to the integration
test suite by relocating it into
tests/integration/nodes/test_node_rrh_emit_effect.py (preserving its async
signature, pytest.mark.integration and pytest.mark.anyio decorators and imports
for HandlerRepoStateCollect and ModelRRHRepoState), and remove it from the unit
file so it no longer inherits the module-level unit marker.
🧹 Nitpick comments (1)
tests/unit/nodes/test_node_rrh_emit_effect.py (1)
210-215: Remove unnecessary async fromtest_handler_type.This test only accesses synchronous properties (
handler_type,handler_category) but is marked as async. The equivalent test inTestHandlerRepoStateCollect(lines 103-107) is correctly synchronous. Consider making this consistent:♻️ Suggested fix
- `@pytest.mark.anyio` - async def test_handler_type(self, handler: HandlerToolchainCollect) -> None: + def test_handler_type(self, handler: HandlerToolchainCollect) -> None: from omnibase_infra.enums import EnumHandlerType, EnumHandlerTypeCategory assert handler.handler_type == EnumHandlerType.INFRA_HANDLER assert handler.handler_category == EnumHandlerTypeCategory.EFFECT
Comment on lines
+90
to
+101
| @pytest.mark.integration | ||
| @pytest.mark.anyio | ||
| async def test_collects_from_real_repo( | ||
| self, handler: HandlerRepoStateCollect | ||
| ) -> None: | ||
| """Integration-style test: collect state from the actual repo.""" | ||
| repo_path = str(Path(__file__).resolve().parents[3]) | ||
| result = await handler.handle(repo_path) | ||
| assert isinstance(result, ModelRRHRepoState) | ||
| assert result.branch # Should have a branch | ||
| assert result.head_sha # Should have a SHA | ||
| assert result.repo_root # Should have a root path |
There was a problem hiding this comment.
Move integration test to tests/integration/ directory.
This test is marked @pytest.mark.integration but resides in tests/unit/nodes/. It will also inherit the module-level @pytest.mark.unit marker, creating conflicting test classification. As per coding guidelines, "Test directory structure: conftest.py at root, separate unit/, integration/, chaos/, replay/, performance/ directories with auto-applied pytest markers."
Consider moving this test to tests/integration/nodes/test_node_rrh_emit_effect.py to maintain proper separation of test types.
🤖 Prompt for AI Agents
In `@tests/unit/nodes/test_node_rrh_emit_effect.py` around lines 90 - 101, The
test function test_collects_from_real_repo (using HandlerRepoStateCollect and
asserting ModelRRHRepoState) is an integration test but lives under tests/unit;
move the entire test function to the integration test suite by relocating it
into tests/integration/nodes/test_node_rrh_emit_effect.py (preserving its async
signature, pytest.mark.integration and pytest.mark.anyio decorators and imports
for HandlerRepoStateCollect and ModelRRHRepoState), and remove it from the unit
file so it no longer inherits the module-level unit marker.
… errors, relocate integration test - Use request.correlation_id consistently in storage write handler (was request.result.correlation_id) - Sanitize logged error with sanitize_error_message(), drop exc_info=True to prevent credential leakage - Include correlation_id in unknown profile error messages for traceability (preserves COMPUTE purity) - Sanitize broker strings with sanitize_error_string() before embedding in validation failure messages - Move integration test from tests/unit/ to tests/integration/nodes/ - Remove unnecessary async marker from synchronous test_handler_type
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_rrh_validate_compute/handlers/handler_rrh_validate.py`:
- Around line 223-265: The _apply_tightening function currently only enables
missing/disabled rules but does not raise severity for rules already enabled at
a lower level; update each tightening branch (the checks for "RRH-1403",
"RRH-1201", "RRH-1301" inside _apply_tightening) to also promote an existing
enabled ModelRRHRuleSeverity from WARN (or any lower severity) to
EnumVerdict.FAIL when the governance requires tightening, preserving
enabled=True and never disabling a rule; use the existing variable existing and
assign a new ModelRRHRuleSeverity with severity=EnumVerdict.FAIL (or modify
existing.severity) when promotion is needed.
Comment on lines
+223
to
+265
| @staticmethod | ||
| def _apply_tightening( | ||
| profile: ModelRRHProfile, | ||
| governance: ModelRRHContractGovernance, | ||
| ) -> dict[str, ModelRRHRuleSeverity]: | ||
| """Apply contract governance tightening to profile rules. | ||
|
|
||
| Tightening rules: | ||
| - ``evidence_requirements: ["tests"]`` -> enable RRH-1403 | ||
| - ``interfaces_touched: ["topics"]`` -> enable RRH-1201 | ||
| - ``deployment_targets: ["k8s"]`` -> enable RRH-1301 | ||
|
|
||
| CRITICAL: Contract can only ENABLE rules or RAISE severity. | ||
| It can NEVER disable a rule that the profile enables, nor | ||
| lower severity from FAIL to WARN. | ||
| """ | ||
| rules: dict[str, ModelRRHRuleSeverity] = {r.rule_id: r for r in profile.rules} | ||
|
|
||
| # Tighten: evidence_requirements includes "tests" -> RRH-1403 | ||
| if "tests" in governance.evidence_requirements: | ||
| existing = rules.get("RRH-1403") | ||
| if existing is None or not existing.enabled: | ||
| rules["RRH-1403"] = ModelRRHRuleSeverity( | ||
| rule_id="RRH-1403", enabled=True, severity=EnumVerdict.FAIL | ||
| ) | ||
|
|
||
| # Tighten: interfaces_touched includes "topics" -> RRH-1201 | ||
| if "topics" in governance.interfaces_touched: | ||
| existing = rules.get("RRH-1201") | ||
| if existing is None or not existing.enabled: | ||
| rules["RRH-1201"] = ModelRRHRuleSeverity( | ||
| rule_id="RRH-1201", enabled=True, severity=EnumVerdict.FAIL | ||
| ) | ||
|
|
||
| # Tighten: deployment_targets includes "k8s" -> RRH-1301 | ||
| if "k8s" in governance.deployment_targets: | ||
| existing = rules.get("RRH-1301") | ||
| if existing is None or not existing.enabled: | ||
| rules["RRH-1301"] = ModelRRHRuleSeverity( | ||
| rule_id="RRH-1301", enabled=True, severity=EnumVerdict.FAIL | ||
| ) | ||
|
|
||
| return rules |
There was a problem hiding this comment.
Contract tightening doesn’t raise severity when rule is already enabled.
If a profile enables a rule at WARN and the contract demands strictness, the current logic won’t promote it to FAIL, contradicting the “raise severity” invariant. Tightening should also elevate severity when needed.
🔧 Suggested fix (raise severity to FAIL when tightening)
@@
if "tests" in governance.evidence_requirements:
existing = rules.get("RRH-1403")
- if existing is None or not existing.enabled:
+ if (
+ existing is None
+ or not existing.enabled
+ or existing.severity != EnumVerdict.FAIL
+ ):
rules["RRH-1403"] = ModelRRHRuleSeverity(
rule_id="RRH-1403", enabled=True, severity=EnumVerdict.FAIL
)
@@
if "topics" in governance.interfaces_touched:
existing = rules.get("RRH-1201")
- if existing is None or not existing.enabled:
+ if (
+ existing is None
+ or not existing.enabled
+ or existing.severity != EnumVerdict.FAIL
+ ):
rules["RRH-1201"] = ModelRRHRuleSeverity(
rule_id="RRH-1201", enabled=True, severity=EnumVerdict.FAIL
)
@@
if "k8s" in governance.deployment_targets:
existing = rules.get("RRH-1301")
- if existing is None or not existing.enabled:
+ if (
+ existing is None
+ or not existing.enabled
+ or existing.severity != EnumVerdict.FAIL
+ ):
rules["RRH-1301"] = ModelRRHRuleSeverity(
rule_id="RRH-1301", enabled=True, severity=EnumVerdict.FAIL
)🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_validate_compute/handlers/handler_rrh_validate.py`
around lines 223 - 265, The _apply_tightening function currently only enables
missing/disabled rules but does not raise severity for rules already enabled at
a lower level; update each tightening branch (the checks for "RRH-1403",
"RRH-1201", "RRH-1301" inside _apply_tightening) to also promote an existing
enabled ModelRRHRuleSeverity from WARN (or any lower severity) to
EnumVerdict.FAIL when the governance requires tightening, preserving
enabled=True and never disabling a rule; use the existing variable existing and
assign a new ModelRRHRuleSeverity with severity=EnumVerdict.FAIL (or modify
existing.severity) when promotion is needed.
Comment on lines
+323
to
+359
| @staticmethod | ||
| def _check_1002_expected_branch( | ||
| env: ModelRRHEnvironmentData, gov: ModelRRHContractGovernance | ||
| ) -> ModelRuleCheckResult: | ||
| """RRH-1002: Branch matches expected pattern from governance.""" | ||
| if not gov.expected_branch_pattern: | ||
| return ModelRuleCheckResult( | ||
| passed=True, | ||
| skipped=True, | ||
| rule_id="RRH-1002", | ||
| reason="No expected_branch_pattern in governance.", | ||
| ) | ||
| pattern = gov.expected_branch_pattern | ||
| # Guard against ReDoS: reject overly long or structurally unsafe patterns. | ||
| if len(pattern) > _MAX_BRANCH_PATTERN_LEN or _NESTED_QUANTIFIER_RE.search( | ||
| pattern | ||
| ): | ||
| return ModelRuleCheckResult( | ||
| passed=False, | ||
| rule_id="RRH-1002", | ||
| message=f"Unsafe branch pattern (possible ReDoS): {_truncate_pattern(pattern)!r}", | ||
| ) | ||
| branch = env.repo_state.branch | ||
| try: | ||
| if re.fullmatch(pattern, branch): | ||
| return ModelRuleCheckResult(passed=True, rule_id="RRH-1002") | ||
| except re.error: | ||
| return ModelRuleCheckResult( | ||
| passed=False, | ||
| rule_id="RRH-1002", | ||
| message=f"Invalid branch pattern: {_truncate_pattern(gov.expected_branch_pattern)}", | ||
| ) | ||
| return ModelRuleCheckResult( | ||
| passed=False, | ||
| rule_id="RRH-1002", | ||
| message=f"Branch '{branch}' does not match pattern '{_truncate_pattern(gov.expected_branch_pattern)}'.", | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ModelRRHContractGovernance definition and its expected_branch_pattern field
rg -n "class ModelRRHContractGovernance" -A 30Repository: OmniNode-ai/omnibase_infra
Length of output: 4482
🏁 Script executed:
# Search for expected_branch_pattern across the codebase
rg -n "expected_branch_pattern" -C 3Repository: OmniNode-ai/omnibase_infra
Length of output: 6715
🏁 Script executed:
# Check if there's any glob handling utilities in the codebase
rg -n "fnmatch|glob" --type py | head -30Repository: OmniNode-ai/omnibase_infra
Length of output: 3536
Fix contract violation: expected_branch_pattern documents "regex or glob" but only handles regex.
ModelRRHContractGovernance docstring (line 28) declares the field as "Regex or glob for expected branch name," but the implementation uses only re.fullmatch() and will fail silently or throw re.error for glob patterns like feature/*. Either implement glob support or update the documentation to specify regex-only. All current tests use regex patterns, so glob support may have been a planned but unimplemented feature.
🔧 Possible approach: explicit glob prefix
@@
-import re
+import fnmatch
+import re
@@
- pattern = gov.expected_branch_pattern
+ pattern = gov.expected_branch_pattern
+ is_glob = pattern.startswith("glob:")
+ raw_pattern = pattern[5:] if is_glob else pattern
@@
- if len(pattern) > _MAX_BRANCH_PATTERN_LEN or _NESTED_QUANTIFIER_RE.search(
- pattern
- ):
+ if (not is_glob) and (
+ len(raw_pattern) > _MAX_BRANCH_PATTERN_LEN
+ or _NESTED_QUANTIFIER_RE.search(raw_pattern)
+ ):
return ModelRuleCheckResult(
passed=False,
rule_id="RRH-1002",
- message=f"Unsafe branch pattern (possible ReDoS): {_truncate_pattern(pattern)!r}",
+ message=f"Unsafe branch pattern (possible ReDoS): {_truncate_pattern(raw_pattern)!r}",
)
branch = env.repo_state.branch
- try:
- if re.fullmatch(pattern, branch):
- return ModelRuleCheckResult(passed=True, rule_id="RRH-1002")
- except re.error:
- return ModelRuleCheckResult(
- passed=False,
- rule_id="RRH-1002",
- message=f"Invalid branch pattern: {_truncate_pattern(gov.expected_branch_pattern)}",
- )
+ if is_glob:
+ if fnmatch.fnmatch(branch, raw_pattern):
+ return ModelRuleCheckResult(passed=True, rule_id="RRH-1002")
+ else:
+ try:
+ if re.fullmatch(raw_pattern, branch):
+ return ModelRuleCheckResult(passed=True, rule_id="RRH-1002")
+ except re.error:
+ return ModelRuleCheckResult(
+ passed=False,
+ rule_id="RRH-1002",
+ message=f"Invalid branch pattern: {_truncate_pattern(raw_pattern)}",
+ )
return ModelRuleCheckResult(
passed=False,
rule_id="RRH-1002",
- message=f"Branch '{branch}' does not match pattern '{_truncate_pattern(gov.expected_branch_pattern)}'.",
+ message=f"Branch '{branch}' does not match pattern '{_truncate_pattern(raw_pattern)}'.",
)…-aware result filtering
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@src/omnibase_infra/nodes/node_rrh_emit_effect/handlers/handler_repo_state_collect.py`:
- Around line 91-123: The _git() helper currently cleans up the subprocess on
TimeoutError but doesn’t handle asyncio.CancelledError, risking orphaned git
processes; modify the try/except in handler_repo_state_collect.py (the _git()
function, Proc variable proc) to add an except asyncio.CancelledError handler
that, if proc is not None, calls proc.kill() and await proc.wait(), then
re-raises the CancelledError to propagate cancellation; keep the existing
TimeoutError branch behavior unchanged.
In
`@src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py`:
- Around line 60-69: Replace the raw ValueError raises in the request.output_dir
validation inside handler_rrh_storage_write (the try block that sets base =
Path(request.output_dir) and checks base.is_absolute() and ".." in base.parts)
with calls that produce the mandated error context: use
ModelInfraErrorContext.with_correlation(..., transport_type=..., operation=...)
to create the error context and either raise that contexted exception or return
a failure result per project convention; ensure you include the original
request.output_dir value in the message and supply the appropriate
transport_type and operation values for this handler so the error follows the
required pattern.
- Around line 160-161: The OSError debug log in handler_rrh_storage_write.py
currently logs the raw exception (logger.debug("Failed to update symlink %s:
%s", name, exc)), which can leak sensitive data; change this to pass the
sanitized message from
omnibase_infra.utils.util_error_sanitization.sanitize_error_message (e.g., call
sanitize_error_message(exc) or sanitize_error_message(str(exc))) so the
logger.debug call uses the sanitized error text instead of the raw exception;
update the import if missing and ensure the log keeps the same context (name)
while substituting the sanitized error.
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_rrh_emit_effect/handlers/handler_repo_state_collect.py (1)
42-58: Consider adding correlation_id support for traceability.The
handle()method doesn't accept or propagate acorrelation_id, and theValueErrorat line 56-58 doesn't useModelInfraErrorContext.with_correlation(). This limits observability when tracing errors through the system.Consider adding an optional
correlation_idparameter:♻️ Suggested enhancement
- async def handle(self, repo_path: str) -> ModelRRHRepoState: + async def handle( + self, repo_path: str, correlation_id: str | None = None + ) -> ModelRRHRepoState: """Collect git state from the given repository path. Args: repo_path: Absolute path to the repository root. + correlation_id: Optional correlation ID for tracing.As per coding guidelines, "Propagate correlation_id from incoming requests; auto-generate with uuid4() if missing; include in all error context."
Comment on lines
+91
to
+123
| proc: asyncio.subprocess.Process | None = None | ||
| try: | ||
| proc = await asyncio.create_subprocess_exec( | ||
| "git", | ||
| "-C", | ||
| repo_path, | ||
| *args, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| ) | ||
| stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=10.0) | ||
| if proc.returncode != 0: | ||
| logger.debug( | ||
| "git %s failed (rc=%d): %s", | ||
| " ".join(args), | ||
| proc.returncode, | ||
| sanitize_error_string(stderr.decode(errors="replace").strip()), | ||
| ) | ||
| return "" | ||
| return stdout.decode(errors="replace") | ||
| except TimeoutError: | ||
| if proc is not None: | ||
| proc.kill() | ||
| await proc.wait() | ||
| logger.debug("git %s error: timed out after 10s", " ".join(args)) | ||
| return "" | ||
| except (FileNotFoundError, OSError) as exc: | ||
| logger.debug( | ||
| "git %s error: %s", | ||
| " ".join(args), | ||
| sanitize_error_string(str(exc)), | ||
| ) | ||
| return "" |
There was a problem hiding this comment.
Handle asyncio.CancelledError to prevent orphaned processes.
The _git() method catches TimeoutError and cleans up the subprocess, but asyncio.CancelledError (raised when the task is cancelled externally, e.g., during shutdown) is not handled. This could leave orphaned git processes.
🛠️ Suggested fix
try:
stdout, stderr = await asyncio.wait_for(proc.communicate(), timeout=10.0)
- except TimeoutError:
+ except (TimeoutError, asyncio.CancelledError):
if proc is not None:
proc.kill()
await proc.wait()
- logger.debug("git %s error: timed out after 10s", " ".join(args))
+ logger.debug("git %s error: cancelled or timed out", " ".join(args))
- return ""
+ raiseNote: For CancelledError, the exception should be re-raised after cleanup to properly propagate cancellation, while TimeoutError can continue returning an empty string as the current graceful degradation behavior.
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_emit_effect/handlers/handler_repo_state_collect.py`
around lines 91 - 123, The _git() helper currently cleans up the subprocess on
TimeoutError but doesn’t handle asyncio.CancelledError, risking orphaned git
processes; modify the try/except in handler_repo_state_collect.py (the _git()
function, Proc variable proc) to add an except asyncio.CancelledError handler
that, if proc is not None, calls proc.kill() and await proc.wait(), then
re-raises the CancelledError to propagate cancellation; keep the existing
TimeoutError branch behavior unchanged.
Comment on lines
+60
to
+69
| try: | ||
| base = Path(request.output_dir) | ||
| if not base.is_absolute(): | ||
| raise ValueError( | ||
| f"output_dir must be absolute, got: {request.output_dir!r}" | ||
| ) | ||
| if ".." in base.parts: | ||
| raise ValueError( | ||
| f"output_dir must not contain '..' components, got: {request.output_dir!r}" | ||
| ) |
There was a problem hiding this comment.
Use ModelInfraErrorContext on raised exceptions (or avoid raises).
The output_dir validation raises raw ValueError without the mandated error context. Consider wrapping these with ModelInfraErrorContext.with_correlation(..., transport_type=..., operation=...), or return a failure result without raising.
As per coding guidelines, Use MANDATORY error context factory ModelInfraErrorContext.with_correlation() for all exception raises with transport_type and operation parameters.
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py`
around lines 60 - 69, Replace the raw ValueError raises in the
request.output_dir validation inside handler_rrh_storage_write (the try block
that sets base = Path(request.output_dir) and checks base.is_absolute() and ".."
in base.parts) with calls that produce the mandated error context: use
ModelInfraErrorContext.with_correlation(..., transport_type=..., operation=...)
to create the error context and either raise that contexted exception or return
a failure result per project convention; ensure you include the original
request.output_dir value in the message and supply the appropriate
transport_type and operation values for this handler so the error follows the
required pattern.
Comment on lines
+160
to
+161
| except OSError as exc: | ||
| logger.debug("Failed to update symlink %s: %s", name, exc) |
There was a problem hiding this comment.
Sanitize the symlink error log message.
The debug log emits the raw exception, which can leak sensitive details. Use sanitize_error_message here as well.
🔧 Proposed fix
- logger.debug("Failed to update symlink %s: %s", name, exc)
+ logger.debug(
+ "Failed to update symlink %s: %s",
+ name,
+ sanitize_error_message(exc),
+ )As per coding guidelines, Sanitize error messages - NEVER include passwords, API keys, PII, connection strings with credentials; use utility functions from omnibase_infra.utils.util_error_sanitization.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except OSError as exc: | |
| logger.debug("Failed to update symlink %s: %s", name, exc) | |
| except OSError as exc: | |
| logger.debug( | |
| "Failed to update symlink %s: %s", | |
| name, | |
| sanitize_error_message(exc), | |
| ) |
🤖 Prompt for AI Agents
In
`@src/omnibase_infra/nodes/node_rrh_storage_effect/handlers/handler_rrh_storage_write.py`
around lines 160 - 161, The OSError debug log in handler_rrh_storage_write.py
currently logs the raw exception (logger.debug("Failed to update symlink %s:
%s", name, exc)), which can leak sensitive data; change this to pass the
sanitized message from
omnibase_infra.utils.util_error_sanitization.sanitize_error_message (e.g., call
sanitize_error_message(exc) or sanitize_error_message(str(exc))) so the
logger.debug call uses the sanitized error text instead of the raw exception;
update the import if missing and ensure the log keeps the same context (name)
while substituting the sanitized error.
Include both RRH exemptions (OMN-2136) and checkpoint exemptions (OMN-2143).
- Add asyncio.CancelledError handler in _git() to prevent orphaned subprocesses - Replace raw ValueError with ProtocolConfigurationError + ModelInfraErrorContext for path validation - Sanitize symlink error log with sanitize_error_message() - Log correlation_id on unknown RRH profile lookup failure - Promote WARN to FAIL severity in contract tightening - Fix docstring: expected_branch_pattern is regex only, not glob - Use atomic symlink replacement via Path.replace() - Add 9 new tests covering all behavioral changes
…n, GC false-positive - handler_rrh_validate: change executor.shutdown(wait=False) to wait=True in finally block to prevent daemon thread accumulation on non-timeout paths - checkpoint handlers (list/read/write): add base_dir is_absolute() and '..' component validation to prevent path traversal via envelope input - handler_stale_run_gc: stop adding malformed file stems to deleted_ids to prevent false-positive removal of active runs from session index
…ard tests - checkpoint handlers: use RuntimeHostError (not ValueError) for base_dir validation, consistent with existing error context pattern - test_gc_removes_malformed: fix assertion to match iteration 1 behavior (malformed stems excluded from deleted_ids) - checkpoint tests: add 6 guard tests for base_dir validation (relative path rejection + traversal component rejection) - fix ruff S108/RUF043: use /var instead of /tmp, raw string regex match
- Checkpoint handlers: add try/except for UUID coercion and isinstance guards for base_dir_raw across read/write/list handlers - Checkpoint list: cap unbounded directory scan at 1000 entries - RRH validate: track timeout state so finally block uses wait=False after ReDoS timeout instead of blocking indefinitely - Stale run GC: treat documents with updated_at >1 hour in the future as stale to prevent indefinite accumulation from clock skew
- Add file:// to SSH URL exclusion heuristic in repo boundary check - Log warning when Unix-socket DSN silently falls back to TCP localhost
jonahgabriel
deleted the
jonah/omn-2136-omnibase_infra-p3-rrh-nodes-emit-effect-validate-compute
branch
February 12, 2026 15:24
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.
Summary
Implements the three core Release Readiness Handshake (RRH) nodes for OMN-2136:
node_rrh_emit_effect(EFFECT): Collects environment data — git repo state, runtime targets, toolchain versions — via 3 handlers (HandlerRepoStateCollect,HandlerRuntimeTargetCollect,HandlerToolchainCollect)node_rrh_validate_compute(COMPUTE): Pure 13-rule validation engine with 4 profiles (default, ticket-pipeline, ci-repair, seam-ticket) and contract tightening enforcement (contracts can only enable rules or raise severity, never loosen)node_rrh_storage_effect(EFFECT): Writes JSON result artifacts withlatest_by_ticket/andlatest_by_repo/symlinks for easy accessKey Design Decisions
rglobs all.yamlundernodes/)ModelRRHResultusesModelRuleCheckResult+EnumVerdictfrom architecture_validator, so dashboards consume both RRH and arch-validation results uniformlyFiles
src/omnibase_infra/models/rrh/— 7 shared Pydantic modelssrc/omnibase_infra/nodes/node_rrh_emit_effect/— EFFECT node + 3 handlerssrc/omnibase_infra/nodes/node_rrh_validate_compute/— COMPUTE node + handler + profilessrc/omnibase_infra/nodes/node_rrh_storage_effect/— EFFECT node + handlertests/unit/nodes/test_node_rrh_*.py— 46 testsvalidation_exemptions.yaml— RRH-specific exemptions for domain string IDsTest plan
Summary by CodeRabbit
New Features
Tests