Repository navigation
feat(OMN-13472): ARCH-004 imperative-orchestrator ratchet — cross-file rule + baseline + gate - #2065
Merged
Merged
Conversation
…e rule + baseline + gate Adds ARCH-004 'Contract-Declared Orchestrator Workflow Must Be Bound To An Executor' to node_architecture_validator. Cross-file node-directory rule that joins contract.yaml (fsm/workflow_coordination), handler_routing.routing_strategy, handler source, and node type/name — catching the delegation-shaped anti-pattern ARCH-003 structurally misses (handler-owned _transition( in a non-*Orchestrator class; declared-but-unbound fsm). - RuleContractDeclaredOrchestratorWorkflow registered in validators/__init__.py - scanner_imperative_orchestrator_ratchet: --check-all/--report, --check-changed/--ratchet, --strict; baseline can only shrink - architecture-handshakes/imperative-orchestrator-baseline.yaml (9 current hard-fails, owner OMN-13471; delegation = sole P0 risk 10) - Wired via OMN-12550 path: scripts/validate.py imperative_orchestrators subcommand + pre-commit hook (changed-node ratchet, blocking) + CI full report (non-blocking initially). Cites OMN-12550 + OMN-13325 in configs. - Tests: ARCH-003-passes / ARCH-004-fails delegation-shape proof + 14 more. Refs OMN-13472 (epic OMN-13471), OMN-12550, OMN-13325.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughIntroduces ARCH-004, a new architecture validation rule that flags orchestrator nodes whose ChangesARCH-004 Imperative-Orchestrator Ratchet
Incidental Updates
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over scripts/validate.py,scanner_imperative_orchestrator_ratchet: Pre-commit / CI changed-files ratchet mode
end
participant precommit as Pre-commit / CI
participant validate_py as scripts/validate.py
participant run_imporch as run_imperative_orchestrators
participant scanner as scanner_imperative_orchestrator_ratchet
participant validator as validator_contract_declared_orchestrator_workflow
precommit->>validate_py: imperative_orchestrators --files [...changed files...]
validate_py->>run_imporch: run_imperative_orchestrators(files=[...])
run_imporch->>scanner: load_baseline(baseline_path)
run_imporch->>scanner: node_dirs_for_changed_files(repo_root, files)
run_imporch->>scanner: scan_node_dirs(repo, node_dirs)
scanner->>validator: analyze_node_directory(node_dir)
validator-->>scanner: OrchestratorNodeAnalysis (hard_fail=True/False)
scanner-->>run_imporch: ScanResult(hard_fails=[...])
run_imporch->>scanner: ratchet_violations(scanned, baseline)
scanner-->>run_imporch: violations list
run_imporch-->>validate_py: True (pass) or False (fail)
validate_py-->>precommit: exit 0 or exit 1
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (1)
src/omnibase_infra/nodes/node_architecture_validator/contract.yaml (1)
191-191: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUpdate rule list to include ARCH-004.
The description still references only
ARCH-001, ARCH-002, ARCH-003. Consider updating to includeARCH-004for consistency with the new rule. Same applies to line 240.🤖 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/omnibase_infra/nodes/node_architecture_validator/contract.yaml` at line 191, Update the description string that lists the specific architecture rules to include the newly added ARCH-004 rule. Change the text from "ARCH-001, ARCH-002, ARCH-003" to "ARCH-001, ARCH-002, ARCH-003, ARCH-004" in the description field. Apply this same update in two locations: at line 191 and at line 240 in the contract.yaml file to maintain consistency throughout the documentation.
🤖 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 `@scripts/validate.py`:
- Around line 587-607: The scan_node_dirs function calls on both the error path
(around line 587) and the full report path (around line 606) hardcode
"omnibase_infra" as the repo key, but the baseline file contains entries from
multiple repositories. Replace the hardcoded "omnibase_infra" strings with the
actual repo name derived from each node directory path by parsing the
src/<repo>/... pattern. Group the node directories by their respective
repositories and call scan_node_dirs with the correct repo name for each group
to match what is stored in the baseline.
In
`@src/omnibase_infra/nodes/node_architecture_validator/validators/scanner_imperative_orchestrator_ratchet.py`:
- Around line 439-443: The write_baseline function is being called without
verifying that the `--check-all` flag is set, which can overwrite the canonical
baseline with incomplete node data from a changed-files-only scan. Add a guard
condition that checks if args.check_all is True before allowing the baseline
write to proceed in the code block starting with if args.write_baseline. If
check_all is not set, either skip the baseline write with an appropriate log
message or raise an error to prevent partial baseline corruption.
---
Nitpick comments:
In `@src/omnibase_infra/nodes/node_architecture_validator/contract.yaml`:
- Line 191: Update the description string that lists the specific architecture
rules to include the newly added ARCH-004 rule. Change the text from "ARCH-001,
ARCH-002, ARCH-003" to "ARCH-001, ARCH-002, ARCH-003, ARCH-004" in the
description field. Apply this same update in two locations: at line 191 and at
line 240 in the contract.yaml file to maintain consistency throughout the
documentation.
🪄 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: b86b3cb8-31f7-41fe-a7b2-6b5bc05cf256
📒 Files selected for processing (10)
.github/workflows/ci.yml.pre-commit-config.yamlarchitecture-handshakes/imperative-orchestrator-baseline.yamlpyproject.tomlscripts/validate.pysrc/omnibase_infra/nodes/node_architecture_validator/contract.yamlsrc/omnibase_infra/nodes/node_architecture_validator/validators/__init__.pysrc/omnibase_infra/nodes/node_architecture_validator/validators/scanner_imperative_orchestrator_ratchet.pysrc/omnibase_infra/nodes/node_architecture_validator/validators/validator_contract_declared_orchestrator_workflow.pytests/unit/nodes/node_architecture_validator/test_validator_contract_declared_orchestrator_workflow.py
…terns validator: no leading-underscore class names)
…uard --write-baseline behind --check-all - scripts/validate.py: derive repo_name from repo_root.name instead of hardcoding 'omnibase_infra' (correct repo::node baseline keying). - scanner: --write-baseline now requires --check-all (a baseline from a --check-changed scan would silently shrink the ratchet below true state); add test_write_baseline_requires_check_all.
Collaborator
Author
CI status (OMN-13472)All gates this change is responsible for are GREEN on head
The 3 remaining red checks are pre-existing dev-state failures, not introduced by this PR (this changeset touches zero runner-image / migration / deploy files — see
These three require dev-state remediation outside OMN-13472's scope and should not block review of the ARCH-004 ratchet. |
jonahgabriel
enabled auto-merge
June 22, 2026 14:10
…lock Commit 4269756 refreshed docker/runners/runner-image.lock.json (identity_digest 8c3208f1->0f337da6, shared_env_digest a796970a->efb9011b) to clear runner-image-build-smoke, but left test_release_backmerge_preserves_runner_identity_lock asserting the stale digests, failing Tests (Split 1/15). Update the assertions to the committed lock values; runner-image-build-smoke validates the lock vs real image identity.
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jun 22, 2026
This was referenced Jun 23, 2026
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.
OMN-13472 — Workstream B: ARCH-004 imperative-orchestrator ratchet
Adds ARCH-004 — "Contract-Declared Orchestrator Workflow Must Be Bound To An Executor" to
node_architecture_validator, plus a ratchet baseline and a gate wired through the existing validator-gating path.Implements Workstream B of the verified plan
docs/plans/2026-06-22-imperative-orchestrator-ratchet-and-recovery-plan-verified.md§4, backed by the census indocs/audits/2026-06-22-imperative-orchestrator-audit.md.Tickets: OMN-13472 (this work), epic OMN-13471 (delegation decomposition — baseline owner), OMN-12550 (wire architecture validators as blocking gates — ARCH-004 rides this, not a fresh hook), OMN-13325 (ratchet-enforcement epic).
Why ARCH-003 cannot do this
ARCH-003 (
validator_no_orchestrator_fsm.py) is a single-file AST visitor that only inspects classes whose name contains"Orchestrator", with a method match-set of{transition, can_transition, apply_transition, get_current_state, set_state}. The real defect lives in classHandlerDelegationWorkflow(no "Orchestrator" in the name) and is driven viaself._transition(...)(leading underscore, a call not a def). ARCH-003 never joins contract + routing + handler. ARCH-004 is a cross-file, node-directory rule that joinscontract.yaml(fsm:/workflow_coordination),handler_routing.routing_strategy, handler source, and node type/name.What ARCH-004 detects
HARD-fail (ERROR) for changed/new orchestrator-like nodes:
fsm:(no executor binds it;ModelContractOrchestratorhas no typedfsm/state_machinefield, no traverser consumes orchestratorfsm.transitions) WHILE a handler drives transitions itself (self._transition(,set_state,current_state=, … boundary-anchored to avoid therecord_phase_transition(/"current_state=%s"substring traps; OR anEnum*Statewhose members ≈ the contractfsm.states).routing_strategy: payload_type_matchfunneling 3+ payload/event types into one workflow handler.WARN / baseline score: handler >750 lines (W1); >125 branch/control markers (W2, regex
�(if|elif|for|while|except|case|and|or)�); ≥10 publish/event-construction markers (W3, regex\.publish\(|ModelEventEnvelope|\.emit\(|[A-Za-z_]+Event\(); declared-but-unbound states (W4). Reducers (node_*_fsm_reducer/ contractstate_machine:+ pure transition executor) are EXEMPT.Ratchet
architecture-handshakes/imperative-orchestrator-baseline.yaml(mirrors the repo's existing handshake-baseline pattern; can only shrink). Modes on the scanner CLI /scripts/validate.py imperative_orchestrators:--check-all --report— full report, never hides debt (CI, non-blocking initially).--check-changed --ratchet— fail on a new/worsened/untracked finding for a touched node (pre-commit, blocking).--strict— drop-baseline: a baselined node that still hard-fails fails (ratchet the baseline down).Baseline records 9 current hard-fails (owner OMN-13471):
node_delegation_orchestrator(omnimarket) is the sole P0 — risk 10, codes H1/H2/H3/W1/W2/W3/W4, 1542-line handler. (paths repo-relative — no machine-absolute paths.)Gate wiring (through OMN-12550, not a parallel hook)
onex-imperative-orchestrator-ratchet— changed-node ratchet, blocking.continue-on-error), promote to required once the baseline is below threshold.dod_evidence
test_arch003_passes_but_arch004_fails_delegation_shape: a synthetic node (contractfsm:table + monolithic handler usingself._transition(whose class is NOT*Orchestrator) → ARCH-003 PASSES it (valid=True, 0 violations), ARCH-004 FAILS it (H1+H3). Verified against the realnode_delegation_orchestrator:validate_no_orchestrator_fsm(handler)→ valid=True/0 violations; ARCH-004 → H1/H2/H3 hard-fail, risk 10 (matches the audit).@pytest.mark.unit, synthetic vendored fixtures, no sibling-repo path dependency):test_arch003_passes_but_arch004_fails_delegation_shapetest_payload_type_match_three_plus_payloads_failstest_reducer_with_state_machine_passes(reducer exempt)test_executor_bound_orchestrator_passestest_full_audit_detects_delegation_shaped_fixturetest_substring_trap_not_a_false_positive(record_phase_transition(/"current_state=%s"do NOT match),test_enum_state_match_drives_h1, ratchet/strict/baseline-roundtrip tests, protocol-surface + regex-reporting tests.uv run pytest tests/unit/nodes/node_architecture_validator/→ 220 passed (15 new ARCH-004 + 203 existing, no regression). Union-count regression guard passes (narrowedanalyze_node_directorytoPathto avoid adding a counted union).uv run mypy src/ --strict→ Success: no issues in 2488 source files.uv run ruff format --check/ruff checkon the changeset → clean. Scopedpre-commit run --files <changeset>→ all applicable hooks pass; no--no-verify, no skip tokens.Summary by CodeRabbit
Release Notes
New Features
Tests
Chores
context_pack_hashon delegation events (with an index).Evidence-Source: 33c69fc5f212a42a731f182fbd309bfe9c9b0b59
Evidence-Ticket: OMN-13472