Repository navigation
feat(OMN-10784): wire interactive executor into onboarding handler - #1556
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR adds an interactive dispatch path to the onboarding handler: models gain routing and provenance fields, the public handler accepts an injected input adapter and dispatches to InteractiveExecutor when ChangesInteractive Onboarding Executor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Comment |
jonahgabriel
enabled auto-merge (squash)
May 10, 2026 05:34
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/omnibase_infra/nodes/node_onboarding_orchestrator/handlers/handler_onboarding.py`:
- Around line 93-101: The current conversion copies sr.response straight into
handler_step_results which can leak sensitive values; update the transformation
used for ModelStepResult in handler_onboarding.py (the handler_step_results list
comprehension that iterates over result.step_results and constructs
ModelStepResult) to sanitize or redact sr.response before including it in
message (e.g., detect and mask tokens/passwords/emails/connection-strings or
replace with a fixed placeholder like "<REDACTED_RESPONSE>"), or only include a
non-sensitive summary instead of the raw sr.response, ensuring
ModelOnboardingOutput never contains raw interactive answers.
- Around line 51-71: The _load_interactive_policy function currently calls
ModelInteractivePolicy.model_validate(raw) which can raise
pydantic.ValidationError; catch that exception around the model_validate call
and re-raise it as an OnboardingHandlerError with a clear message (include
policy_name and the original validation error details) so the function honors
its declared Raises contract; reference the _load_interactive_policy function,
ModelInteractivePolicy.model_validate call, and OnboardingHandlerError when
making this change.
In
`@src/omnibase_infra/nodes/node_onboarding_orchestrator/models/model_onboarding_input.py`:
- Around line 9-44: Add a Pydantic ConfigDict to ModelOnboardingInput to enforce
strict validation and immutability: import ConfigDict from pydantic and add a
class attribute (e.g. model_config = ConfigDict(frozen=True, extra="forbid",
from_attributes=True)) inside the ModelOnboardingInput class so unexpected
fields are forbidden and the model is immutable/compatible with
ORM/pytest-xdist.
🪄 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: 831979e5-9c56-423c-8a3c-236566be703d
📒 Files selected for processing (5)
contracts/OMN-10784.yamlsrc/omnibase_infra/nodes/node_onboarding_orchestrator/handlers/handler_onboarding.pysrc/omnibase_infra/nodes/node_onboarding_orchestrator/models/model_onboarding_input.pysrc/omnibase_infra/nodes/node_onboarding_orchestrator/models/model_onboarding_output.pytests/unit/nodes/node_onboarding_orchestrator/test_handler_onboarding_interactive.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/omnibase_infra/nodes/node_onboarding_orchestrator/models/model_onboarding_input.py`:
- Around line 39-46: The model currently allows dry_run=False with
env_output_path=None; add a pydantic validator to enforce the documented
invariant by ensuring that when the dry_run field is False, env_output_path is
not None (and optionally non-empty). In the model class containing the dry_run
and env_output_path fields, add a `@root_validator` (or `@validator` with
always=True) that checks values["dry_run"] and values["env_output_path"] and
raises a ValueError with a clear message if dry_run is False and env_output_path
is None/empty so invalid requests are rejected during validation.
🪄 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: 34621a89-4234-4245-9b7e-cc50ecac53e6
📒 Files selected for processing (2)
src/omnibase_infra/nodes/node_onboarding_orchestrator/handlers/handler_onboarding.pysrc/omnibase_infra/nodes/node_onboarding_orchestrator/models/model_onboarding_input.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnibase_infra/nodes/node_onboarding_orchestrator/handlers/handler_onboarding.py
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)
tests/integration/onboarding/test_handler_onboarding_interactive_integration.py (1)
125-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMark the home-directory write-safety test as serial to avoid flaky parallel interference.
The assertion compares real
~/.omnibase/.envmtime, which can be affected by concurrent tests/processes. Add a serial marker so this check runs non-parallel.Suggested patch
`@pytest.mark.asyncio` +@pytest.mark.serial async def test_no_real_home_writes(tmp_path: Path) -> None:As per coding guidelines: "Integration tests requiring specific services must use markers:
@pytest.mark.consul,@pytest.mark.postgres,@pytest.mark.kafka,@pytest.mark.serialfor non-parallel tests".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/onboarding/test_handler_onboarding_interactive_integration.py` around lines 125 - 149, The test test_no_real_home_writes should be marked as non-parallel to avoid flakiness; add the `@pytest.mark.serial` decorator above the async test (alongside the existing `@pytest.mark.asyncio`) so pytest runs it serially, e.g., decorate the test function test_no_real_home_writes with `@pytest.mark.serial` to ensure the mtime assertion on ~/.omnibase/.env isn't affected by concurrent tests or processes.
🤖 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
`@tests/integration/onboarding/test_handler_onboarding_interactive_integration.py`:
- Around line 125-149: The test test_no_real_home_writes should be marked as
non-parallel to avoid flakiness; add the `@pytest.mark.serial` decorator above the
async test (alongside the existing `@pytest.mark.asyncio`) so pytest runs it
serially, e.g., decorate the test function test_no_real_home_writes with
`@pytest.mark.serial` to ensure the mtime assertion on ~/.omnibase/.env isn't
affected by concurrent tests or processes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3876f1c5-b78b-4249-9e9a-d226698e555d
📒 Files selected for processing (3)
contracts/OMN-10784.yamlsrc/omnibase_infra/nodes/node_onboarding_orchestrator/contract.yamltests/integration/onboarding/test_handler_onboarding_interactive_integration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/OMN-10784.yaml
Collaborator
Author
|
Addressed CodeRabbit finding: added |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Extend handle_onboarding to detect policy_type: interactive and dispatch to InteractiveExecutor. Add policy_name, dry_run, env_output_path to ModelOnboardingInput and provenance fields to ModelOnboardingOutput. Adapter injected via function parameter (DI outside models). - DAG path unchanged: policy_name=None routes to existing resolve_policy + verification loop - Interactive path: loads policy by name, drives InteractiveExecutor, optionally writes env via ConfigWriter when dry_run=False - dry_run=True is the default — no accidental writes - 12 new tests: 8 interactive path + 4 DAG regression - All 237 existing onboarding tests pass unchanged
- Wrap model_validate() in try/except to honor OnboardingHandlerError contract - Remove raw response echo from step_results to prevent secret leakage - Add ConfigDict(frozen=True, extra="forbid") to ModelOnboardingInput
- Add tests/integration/onboarding/test_handler_onboarding_interactive_integration.py satisfying Integration Test Coverage gate (5 parametrized cases: local dry_run, cloud dry_run, dry_run=False writes to tmp, dry_run=False without path raises, no real ~/.omnibase writes). - Update node contract.yaml description + bump to 1.1.0 to reflect interactive path addition (Contract Sync Gate). - Fix interfaces_touched enum values (must be one of: events/topics/protocols/envelopes/public_api). - Add dod-006 evidence with 'deploy' keyword satisfying deploy-gate for runtime-path-touching PR (handler is library code; verified via in-process integration suite, no Docker rebuild required). OMN-10784 Evidence-Ticket: OMN-10784
Address CodeRabbit finding: ModelOnboardingInput previously accepted dry_run=False with env_output_path=None, deferring the failure to handler runtime. Now reject the invalid combination at model validation time via @model_validator(mode='after'), raising ValidationError with the same message the handler used. Update unit + integration tests to expect ValidationError at construction instead of OnboardingHandlerError at execution. OMN-10784
…owlist The OMN-10771 PR added ProtocolInputAdapter for the InteractiveExecutor DI boundary. Update the protocol-ownership allowlist so test_no_unknown_protocols and test_protocol_count_within_bounds pass. Pre-existing condition_evaluator failures (literal LHS in 'not in' clauses) filed as OMN-10798 — out of scope for OMN-10784 handler integration. OMN-10784
jonahgabriel
force-pushed
the
jonah/omn-10784-handler-integration
branch
from
May 10, 2026 07:02
ce20190 to
dae02b0
Compare
Pre-existing condition_evaluator failures from OMN-10769 are resolved on main by OMN-10797 (quoted-literal LHS support). Need a new CI run on top of latest main. OMN-10784
jonahgabriel
disabled auto-merge
May 10, 2026 07:51
jonahgabriel
enabled auto-merge (squash)
May 10, 2026 07:51
The earlier commit dae02b0 added ProtocolInputAdapter to KNOWN_INFRA_PROTOCOLS. After OMN-10797 merged the same allowlist entry to main, the GitHub merge ref for this PR contains both — F601 dictionary-key duplicate. Drop this PR's copy since main is now authoritative.
CodeQL flagged the import as unused; resolves the unresolved review thread that was blocking the required-conversation-resolution gate.
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
2 participants
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
handle_onboardinghandler: whenpolicy_nameis set and the policy haspolicy_type == "interactive", dispatch toInteractiveExecutorwith an injectedProtocolInputAdapterpolicy_name,dry_run(defaultTrue), andenv_output_pathtoModelOnboardingInput; addprovenance,policy_name,policy_type,visited_steps,terminal_step,dry_run,env_output_path_writtentoModelOnboardingOutputdry_run=Trueis the safe default --ConfigWriter.writeonly called whendry_run=Falsewith explicitenv_output_pathpolicy_name=NoneChanges
models/model_onboarding_input.pypolicy_name,dry_run,env_output_pathfieldsmodels/model_onboarding_output.pyhandlers/handler_onboarding.py_handle_dag+_handle_interactive, route onpolicy_namecontracts/OMN-10784.yamltest_handler_onboarding_interactive.pyTest plan
dry_run=Truedoes not callConfigWriterdry_run=FalsecallsConfigWriter.writewith correct argsdry_run=Falsewithoutenv_output_pathraisesOnboardingHandlerErrorinput_adapterwith interactive policy raisesOnboardingHandlerErrorpolicy_nameraisesOnboardingHandlerErrorHandlerOnboardingclass wrapper delegates interactive calls correctlypolicy_name=Noneroutes to existing behavior (output format unchanged)Evidence-Source: OCC#902
Evidence-Ticket: OMN-10784
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes / Validation
Tests