refactor(onboard): extract final flow composition - #8264
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
📝 WalkthroughWalkthroughThe change adds a final onboarding flow composition module, updates onboarding imports, relocates finalization dependency paths, updates related tests and probes, and lowers architecture budget thresholds. ChangesFinal onboarding flow composition
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Onboard as onboard.ts
participant Composition as final-flow-composition
participant Phases as final-flow-phases
Onboard->>Composition: createFinalOnboardFlowPhases(options)
Composition->>Composition: merge finalizationHandlerDeps
Composition->>Phases: create final flow phases
Phases-->>Composition: return phases
Composition-->>Onboard: return phases
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7b97b01 in the TypeScript / code-coverage/cliThe overall coverage in commit 7b97b01 in the Show a code coverage summary of the most impacted files.
Updated |
wscurran
added
area: architecture
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/onboard/machine/handlers/sandbox-messaging.ts (2)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a single import path for
resolveMessagingPlanAuthority.This module re-exports
resolveMessagingPlanAuthorityfrom../../../messaging/plan-authority.src/lib/onboard/channel-state.tsimports the same symbol directly from the messaging module, so the codebase now has two import paths for one owner. Import it directly insrc/lib/onboard/machine/handlers/sandbox.tsand drop the re-export, unless the re-export exists for a bounded compatibility window.♻️ Proposed change
import { type RegistryMessagingAuthority, resolveMessagingPlanAuthority, } from "../../../messaging/plan-authority"; import { getActiveChannelsFromPlan, getChannelsFromPlan } from "../../messaging-plan-session"; - -export { resolveMessagingPlanAuthority };Then in
src/lib/onboard/machine/handlers/sandbox.ts, importresolveMessagingPlanAuthorityfrom../../../messaging/plan-authorityinstead of./sandbox-messaging.As per path instructions: "Flag cross-layer cycles, duplicate sources of truth, and forwarding wrappers that add a new layer without retiring the old owner and its callers."
🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts` around lines 15 - 22, Remove the resolveMessagingPlanAuthority re-export from sandbox-messaging.ts and update sandbox.ts to import it directly from ../../../messaging/plan-authority. Preserve the existing RegistryMessagingAuthority import and other sandbox-messaging usages, leaving only the messaging module as the symbol’s import path.Source: Path instructions
450-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
registryPlanpath fromselectionFromRecordedChannels. This is the only caller, and it always passesnullforregistryPlan; remove the parameter and itselse if (registryPlan)branch.🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts` around lines 450 - 457, Update selectionFromRecordedChannels and its sole caller in the recordedChannels flow to remove the unused registryPlan parameter, including the null argument, and delete the corresponding else if (registryPlan) branch while preserving the remaining selection behavior.src/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts (2)
330-331: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe ordering assertion passes when
setupMessagingChannelsnever runs.
invocationCallOrder[0] ?? Number.NEGATIVE_INFINITYmakes the comparison succeed if messaging setup was never called. Assert the setup call first, then compare the orders.💚 Proposed test fix
+ expect(setupMessagingChannels).toHaveBeenCalled(); expect(stageSandboxCredentialProviders.mock.invocationCallOrder[0]).toBeGreaterThan( - setupMessagingChannels.mock.invocationCallOrder[0] ?? Number.NEGATIVE_INFINITY, + setupMessagingChannels.mock.invocationCallOrder[0]!, );As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts` around lines 330 - 331, Update the ordering assertion in the sandbox creation test to first require that setupMessagingChannels was called, then compare its invocation order with stageSandboxCredentialProviders. Remove the Number.NEGATIVE_INFINITY fallback so the test cannot pass when messaging setup is skipped.Source: Path instructions
202-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the plan-authority expectation explicit instead of calling the resolver inside the mock.
getStoredMessagingChannelConfigcallsresolveMessagingPlanAuthorityand discards the result. The call only affects the test if it throws, so the intent is hidden. Lines 229-232 already assert the sandbox name and the cleared plan. Either assert the resolver explicitly or remove the call.💚 Proposed test fix
const getStoredMessagingChannelConfig = vi.fn((sandboxName: string | null) => { - expect(sandboxName).toBe("new-name"); - resolveMessagingPlanAuthority({ - sandboxName: sandboxName ?? "", - registry: { authoritative: false, plan: null }, - stagedPlan, - sessionPlan: durableSession.messagingPlan, - }); + expect(() => + resolveMessagingPlanAuthority({ + sandboxName: sandboxName ?? "", + registry: { authoritative: false, plan: null }, + stagedPlan, + sessionPlan: durableSession.messagingPlan, + }), + ).not.toThrow(); return null; });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts` around lines 202 - 211, Remove the discarded resolveMessagingPlanAuthority call from the getStoredMessagingChannelConfig mock, since the mock should only provide its stored-config behavior. Keep the existing assertions around the resolver’s expected plan authority, sandbox name, and cleared plan explicit in the test rather than relying on an exception from the mock.Source: Path instructions
src/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts (1)
352-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInitialize
currentBindingLivetofalseso staging proves the binding became live.
currentBindingLivestarts astrue, andstageSandboxCredentialProvidersonly sets it totrueagain. The gateway match therefore already succeeds before staging runs, so the test cannot show that staging made the current binding live. The sibling replay test insandbox-checkpoint-crash-recovery.test.tsstarts its flag atfalse.💚 Proposed test fix
- let currentBindingLive = true; + let currentBindingLive = false; const stageSandboxCredentialProviders = vi.fn(async () => { currentBindingLive = true; return [currentBinding]; });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts` around lines 352 - 366, Initialize currentBindingLive to false in the test setup, while keeping stageSandboxCredentialProviders responsible for setting it to true. This ensures providerMatchesGatewayCredential only succeeds after staging and the replay test verifies the intended behavior.Source: Path instructions
src/lib/onboard/checkpoint-revalidate.ts (1)
30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the canonical-binding predicate into one shared helper.
The same canonical-binding rule now exists in three modules:
- Lines 34-40 here.
assertValidProviderBindingsinsrc/lib/onboard/checkpoint-record.ts(Lines 29-44).isCanonicalBindinginsrc/lib/onboard/credential-provider-registration.ts(Lines 87-91).All three check non-empty and trimmed
name,type, andcredentialEnv. The three copies must stay in sync. If one copy gains a rule and the others do not, a binding accepted at registration can be rejected at replay, or the reverse.Move the predicate next to
CheckpointProviderBindinginsrc/lib/state/onboard-checkpoint-types.tsand call it from all three sites. Keep each module's own failure mode: throw, stale report, or plan error.🤖 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/lib/onboard/checkpoint-revalidate.ts` around lines 30 - 46, Extract the shared canonical-binding predicate next to CheckpointProviderBinding in onboard-checkpoint-types.ts, covering non-empty and trimmed name, type, and credentialEnv values. Replace the duplicated checks in the missingProviders filter, assertValidProviderBindings, and isCanonicalBinding with this helper while preserving each caller’s existing failure behavior.
🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 378-415: Add a resume-path test for registry authority using the
existing staged-plan divergence test as a template, with checkpoint channels
differing from the authoritative registry plan. Verify setupMessagingChannels is
not called and the registry plan is returned unchanged.
---
Nitpick comments:
In `@src/lib/onboard/checkpoint-revalidate.ts`:
- Around line 30-46: Extract the shared canonical-binding predicate next to
CheckpointProviderBinding in onboard-checkpoint-types.ts, covering non-empty and
trimmed name, type, and credentialEnv values. Replace the duplicated checks in
the missingProviders filter, assertValidProviderBindings, and isCanonicalBinding
with this helper while preserving each caller’s existing failure behavior.
In `@src/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts`:
- Around line 330-331: Update the ordering assertion in the sandbox creation
test to first require that setupMessagingChannels was called, then compare its
invocation order with stageSandboxCredentialProviders. Remove the
Number.NEGATIVE_INFINITY fallback so the test cannot pass when messaging setup
is skipped.
- Around line 202-211: Remove the discarded resolveMessagingPlanAuthority call
from the getStoredMessagingChannelConfig mock, since the mock should only
provide its stored-config behavior. Keep the existing assertions around the
resolver’s expected plan authority, sandbox name, and cleared plan explicit in
the test rather than relying on an exception from the mock.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 15-22: Remove the resolveMessagingPlanAuthority re-export from
sandbox-messaging.ts and update sandbox.ts to import it directly from
../../../messaging/plan-authority. Preserve the existing
RegistryMessagingAuthority import and other sandbox-messaging usages, leaving
only the messaging module as the symbol’s import path.
- Around line 450-457: Update selectionFromRecordedChannels and its sole caller
in the recordedChannels flow to remove the unused registryPlan parameter,
including the null argument, and delete the corresponding else if (registryPlan)
branch while preserving the remaining selection behavior.
In `@src/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts`:
- Around line 352-366: Initialize currentBindingLive to false in the test setup,
while keeping stageSandboxCredentialProviders responsible for setting it to
true. This ensures providerMatchesGatewayCredential only succeeds after staging
and the replay test verifies the intended behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f95d641-9df3-4861-823f-fb56d53212f3
📒 Files selected for processing (43)
ci/source-architecture-budget.jsondocs/security/credential-storage.mdxsrc/lib/actions/sandbox/rebuild-target-staging.test.tssrc/lib/messaging/plan-authority.test.tssrc/lib/messaging/plan-authority.tssrc/lib/onboard.tssrc/lib/onboard/channel-state.test.tssrc/lib/onboard/channel-state.tssrc/lib/onboard/checkpoint-record.test.tssrc/lib/onboard/checkpoint-record.tssrc/lib/onboard/checkpoint-replay.test.tssrc/lib/onboard/checkpoint-replay.tssrc/lib/onboard/checkpoint-revalidate.tssrc/lib/onboard/credential-provider-registration.test.tssrc/lib/onboard/credential-provider-registration.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/machine/core-flow-composition.test.tssrc/lib/onboard/machine/core-flow-composition.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/final-flow-composition.test.tssrc/lib/onboard/machine/final-flow-composition.tssrc/lib/onboard/machine/finalization-deps.test.tssrc/lib/onboard/machine/finalization-deps.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.tssrc/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.tssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/machine/resume-provider-shim.test.tssrc/lib/onboard/machine/resume-provider-shim.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/onboard/messaging-config.test.tssrc/lib/onboard/messaging-config.tssrc/lib/onboard/messaging-credentials.tssrc/lib/onboard/messaging-reuse.test.tssrc/lib/onboard/messaging-reuse.tstest/channels-add-preset.test.tstest/credential-migration-reconciliation.test.tstest/onboard-fsm-live-slices.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/lib/onboard/machine/handlers/sandbox-messaging.ts (2)
15-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a single import path for
resolveMessagingPlanAuthority.This module re-exports
resolveMessagingPlanAuthorityfrom../../../messaging/plan-authority.src/lib/onboard/channel-state.tsimports the same symbol directly from the messaging module, so the codebase now has two import paths for one owner. Import it directly insrc/lib/onboard/machine/handlers/sandbox.tsand drop the re-export, unless the re-export exists for a bounded compatibility window.♻️ Proposed change
import { type RegistryMessagingAuthority, resolveMessagingPlanAuthority, } from "../../../messaging/plan-authority"; import { getActiveChannelsFromPlan, getChannelsFromPlan } from "../../messaging-plan-session"; - -export { resolveMessagingPlanAuthority };Then in
src/lib/onboard/machine/handlers/sandbox.ts, importresolveMessagingPlanAuthorityfrom../../../messaging/plan-authorityinstead of./sandbox-messaging.As per path instructions: "Flag cross-layer cycles, duplicate sources of truth, and forwarding wrappers that add a new layer without retiring the old owner and its callers."
🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts` around lines 15 - 22, Remove the resolveMessagingPlanAuthority re-export from sandbox-messaging.ts and update sandbox.ts to import it directly from ../../../messaging/plan-authority. Preserve the existing RegistryMessagingAuthority import and other sandbox-messaging usages, leaving only the messaging module as the symbol’s import path.Source: Path instructions
450-457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable
registryPlanpath fromselectionFromRecordedChannels. This is the only caller, and it always passesnullforregistryPlan; remove the parameter and itselse if (registryPlan)branch.🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts` around lines 450 - 457, Update selectionFromRecordedChannels and its sole caller in the recordedChannels flow to remove the unused registryPlan parameter, including the null argument, and delete the corresponding else if (registryPlan) branch while preserving the remaining selection behavior.src/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts (2)
330-331: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe ordering assertion passes when
setupMessagingChannelsnever runs.
invocationCallOrder[0] ?? Number.NEGATIVE_INFINITYmakes the comparison succeed if messaging setup was never called. Assert the setup call first, then compare the orders.💚 Proposed test fix
+ expect(setupMessagingChannels).toHaveBeenCalled(); expect(stageSandboxCredentialProviders.mock.invocationCallOrder[0]).toBeGreaterThan( - setupMessagingChannels.mock.invocationCallOrder[0] ?? Number.NEGATIVE_INFINITY, + setupMessagingChannels.mock.invocationCallOrder[0]!, );As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts` around lines 330 - 331, Update the ordering assertion in the sandbox creation test to first require that setupMessagingChannels was called, then compare its invocation order with stageSandboxCredentialProviders. Remove the Number.NEGATIVE_INFINITY fallback so the test cannot pass when messaging setup is skipped.Source: Path instructions
202-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the plan-authority expectation explicit instead of calling the resolver inside the mock.
getStoredMessagingChannelConfigcallsresolveMessagingPlanAuthorityand discards the result. The call only affects the test if it throws, so the intent is hidden. Lines 229-232 already assert the sandbox name and the cleared plan. Either assert the resolver explicitly or remove the call.💚 Proposed test fix
const getStoredMessagingChannelConfig = vi.fn((sandboxName: string | null) => { - expect(sandboxName).toBe("new-name"); - resolveMessagingPlanAuthority({ - sandboxName: sandboxName ?? "", - registry: { authoritative: false, plan: null }, - stagedPlan, - sessionPlan: durableSession.messagingPlan, - }); + expect(() => + resolveMessagingPlanAuthority({ + sandboxName: sandboxName ?? "", + registry: { authoritative: false, plan: null }, + stagedPlan, + sessionPlan: durableSession.messagingPlan, + }), + ).not.toThrow(); return null; });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts` around lines 202 - 211, Remove the discarded resolveMessagingPlanAuthority call from the getStoredMessagingChannelConfig mock, since the mock should only provide its stored-config behavior. Keep the existing assertions around the resolver’s expected plan authority, sandbox name, and cleared plan explicit in the test rather than relying on an exception from the mock.Source: Path instructions
src/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts (1)
352-366: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInitialize
currentBindingLivetofalseso staging proves the binding became live.
currentBindingLivestarts astrue, andstageSandboxCredentialProvidersonly sets it totrueagain. The gateway match therefore already succeeds before staging runs, so the test cannot show that staging made the current binding live. The sibling replay test insandbox-checkpoint-crash-recovery.test.tsstarts its flag atfalse.💚 Proposed test fix
- let currentBindingLive = true; + let currentBindingLive = false; const stageSandboxCredentialProviders = vi.fn(async () => { currentBindingLive = true; return [currentBinding]; });As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 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/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts` around lines 352 - 366, Initialize currentBindingLive to false in the test setup, while keeping stageSandboxCredentialProviders responsible for setting it to true. This ensures providerMatchesGatewayCredential only succeeds after staging and the replay test verifies the intended behavior.Source: Path instructions
src/lib/onboard/checkpoint-revalidate.ts (1)
30-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the canonical-binding predicate into one shared helper.
The same canonical-binding rule now exists in three modules:
- Lines 34-40 here.
assertValidProviderBindingsinsrc/lib/onboard/checkpoint-record.ts(Lines 29-44).isCanonicalBindinginsrc/lib/onboard/credential-provider-registration.ts(Lines 87-91).All three check non-empty and trimmed
name,type, andcredentialEnv. The three copies must stay in sync. If one copy gains a rule and the others do not, a binding accepted at registration can be rejected at replay, or the reverse.Move the predicate next to
CheckpointProviderBindinginsrc/lib/state/onboard-checkpoint-types.tsand call it from all three sites. Keep each module's own failure mode: throw, stale report, or plan error.🤖 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/lib/onboard/checkpoint-revalidate.ts` around lines 30 - 46, Extract the shared canonical-binding predicate next to CheckpointProviderBinding in onboard-checkpoint-types.ts, covering non-empty and trimmed name, type, and credentialEnv values. Replace the duplicated checks in the missingProviders filter, assertValidProviderBindings, and isCanonicalBinding with this helper while preserving each caller’s existing failure behavior.
🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 378-415: Add a resume-path test for registry authority using the
existing staged-plan divergence test as a template, with checkpoint channels
differing from the authoritative registry plan. Verify setupMessagingChannels is
not called and the registry plan is returned unchanged.
---
Nitpick comments:
In `@src/lib/onboard/checkpoint-revalidate.ts`:
- Around line 30-46: Extract the shared canonical-binding predicate next to
CheckpointProviderBinding in onboard-checkpoint-types.ts, covering non-empty and
trimmed name, type, and credentialEnv values. Replace the duplicated checks in
the missingProviders filter, assertValidProviderBindings, and isCanonicalBinding
with this helper while preserving each caller’s existing failure behavior.
In `@src/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.ts`:
- Around line 330-331: Update the ordering assertion in the sandbox creation
test to first require that setupMessagingChannels was called, then compare its
invocation order with stageSandboxCredentialProviders. Remove the
Number.NEGATIVE_INFINITY fallback so the test cannot pass when messaging setup
is skipped.
- Around line 202-211: Remove the discarded resolveMessagingPlanAuthority call
from the getStoredMessagingChannelConfig mock, since the mock should only
provide its stored-config behavior. Keep the existing assertions around the
resolver’s expected plan authority, sandbox name, and cleared plan explicit in
the test rather than relying on an exception from the mock.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 15-22: Remove the resolveMessagingPlanAuthority re-export from
sandbox-messaging.ts and update sandbox.ts to import it directly from
../../../messaging/plan-authority. Preserve the existing
RegistryMessagingAuthority import and other sandbox-messaging usages, leaving
only the messaging module as the symbol’s import path.
- Around line 450-457: Update selectionFromRecordedChannels and its sole caller
in the recordedChannels flow to remove the unused registryPlan parameter,
including the null argument, and delete the corresponding else if (registryPlan)
branch while preserving the remaining selection behavior.
In `@src/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.ts`:
- Around line 352-366: Initialize currentBindingLive to false in the test setup,
while keeping stageSandboxCredentialProviders responsible for setting it to
true. This ensures providerMatchesGatewayCredential only succeeds after staging
and the replay test verifies the intended behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0f95d641-9df3-4861-823f-fb56d53212f3
📒 Files selected for processing (43)
ci/source-architecture-budget.jsondocs/security/credential-storage.mdxsrc/lib/actions/sandbox/rebuild-target-staging.test.tssrc/lib/messaging/plan-authority.test.tssrc/lib/messaging/plan-authority.tssrc/lib/onboard.tssrc/lib/onboard/channel-state.test.tssrc/lib/onboard/channel-state.tssrc/lib/onboard/checkpoint-record.test.tssrc/lib/onboard/checkpoint-record.tssrc/lib/onboard/checkpoint-replay.test.tssrc/lib/onboard/checkpoint-replay.tssrc/lib/onboard/checkpoint-revalidate.tssrc/lib/onboard/credential-provider-registration.test.tssrc/lib/onboard/credential-provider-registration.tssrc/lib/onboard/lifecycle-contracts.mdsrc/lib/onboard/machine/core-flow-composition.test.tssrc/lib/onboard/machine/core-flow-composition.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/final-flow-composition.test.tssrc/lib/onboard/machine/final-flow-composition.tssrc/lib/onboard/machine/finalization-deps.test.tssrc/lib/onboard/machine/finalization-deps.tssrc/lib/onboard/machine/handlers/sandbox-checkpoint-crash-recovery.test.tssrc/lib/onboard/machine/handlers/sandbox-create-intent-boundary.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.test.tssrc/lib/onboard/machine/handlers/sandbox-messaging.tssrc/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.tssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/machine/resume-provider-shim.test.tssrc/lib/onboard/machine/resume-provider-shim.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/onboard/messaging-config.test.tssrc/lib/onboard/messaging-config.tssrc/lib/onboard/messaging-credentials.tssrc/lib/onboard/messaging-reuse.test.tssrc/lib/onboard/messaging-reuse.tstest/channels-add-preset.test.tstest/credential-migration-reconciliation.test.tstest/onboard-fsm-live-slices.test.ts
🛑 Comments failed to post (1)
src/lib/onboard/machine/handlers/sandbox-messaging.ts (1)
378-415: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Description: Locate existing divergence coverage for messaging checkpoints. rg -nP --type=ts -C5 'divergedCheckpointChannels|Reconciling messaging selection'Repository: NVIDIA/NemoClaw
Length of output: 153
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- candidate files ---' git ls-files | rg '(^|/)(sandbox-messaging\.ts|.*messaging.*test.*|.*sandbox.*test.*)$' || true printf '%s\n' '--- helper definitions and callers ---' rg -n -C4 'selectionFromRegistryAuthority|selectionFromCompletedMessagingAuthority|reconcileCheckpoint|resolveMessagingPlanAuthority|selectionFromCompletedMessagingCheckpoint|messagingDecisionCompleted' src --glob '*.ts' || true printf '%s\n' '--- relevant file outline ---' ast-grep outline src/lib/onboard/machine/handlers/sandbox-messaging.ts --match 'selectionFromRegistryAuthority' --view expanded || trueRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- sandbox-messaging test symbols ---' ast-grep outline src/lib/onboard/machine/handlers/sandbox-messaging.test.ts | sed -n '1,220p' printf '%s\n' '--- authority/checkpoint test references ---' rg -n -C6 'registry|staged|checkpoint|resume|diverg|Reconciling messaging selection|selectedChannels|clearPlanEnv' \ src/lib/onboard/machine/handlers/sandbox-messaging.test.ts printf '%s\n' '--- source control flow ---' sed -n '250,470p' src/lib/onboard/machine/handlers/sandbox-messaging.tsRepository: NVIDIA/NemoClaw
Length of output: 24642
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- authoritative resume coverage across onboarding tests ---' rg -n -C8 'authoritative:\s*true|withMessagingCheckpoint|completedCheckpointSession|Reconciling messaging selection' \ src/lib/onboard src/lib/actions/sandbox --glob '*.test.ts' | rg -B8 -A8 \ 'authoritative:\s*true|withMessagingCheckpoint|completedCheckpointSession|Reconciling messaging selection' || true printf '%s\n' '--- focused test blocks ---' sed -n '286,405p' src/lib/onboard/machine/handlers/sandbox-messaging.test.ts sed -n '531,585p' src/lib/onboard/machine/handlers/sandbox-messaging.test.ts printf '%s\n' '--- deterministic divergence check for the staged-authority fixture ---' python3 - <<'PY' from pathlib import Path p = Path("src/lib/onboard/machine/handlers/sandbox-messaging.test.ts").read_text() start = p.index('it("uses the staged plan before a matching session plan during resume for a pending target"') end = p.index('\n it("rejects a staged plan', start) block = p[start:end] print("has resume:", "resume: true" in block) print("has completed checkpoint:", "withMessagingCheckpoint(" in block) print("checkpoint channels:", '["telegram"]' in block) print("staged plan channels:", "stagedPlan = slackPlan" in block) print("setup skipped:", "setupMessagingChannels).not.toHaveBeenCalled()" in block) PYRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -e printf '%s\n' '--- registry resume tests in the public sandbox handler ---' sed -n '540,620p' src/lib/onboard/machine/handlers/sandbox.test.ts sed -n '1185,1240p' src/lib/onboard/machine/handlers/sandbox.test.ts sed -n '1280,1310p' src/lib/onboard/machine/handlers/sandbox.test.ts printf '%s\n' '--- checkpoint references in registry-authority test regions ---' python3 - <<'PY' from pathlib import Path for name in [ "src/lib/onboard/machine/handlers/sandbox-messaging.test.ts", "src/lib/onboard/machine/handlers/sandbox.test.ts", ]: text = Path(name).read_text().splitlines() print(name) for i, line in enumerate(text): if "getRegistrySandboxMessagingAuthority" in line and "authoritative: true" in "\n".join(text[i:i+12]): lo, hi = max(0, i-8), min(len(text), i+24) block = "\n".join(text[lo:hi]) print(f" lines {lo+1}-{hi}: checkpoint={('checkpoint' in block)}, divergent-marker={('discord' in block and 'telegram' in block)}") PYRepository: NVIDIA/NemoClaw
Length of output: 7557
Add registry-authority resume divergence coverage.
The staged-plan test already covers divergent checkpoint channels and skips setup. Add the equivalent resume case for an authoritative registry plan. Assert that
setupMessagingChannelsis not called and that the registry plan is returned.🤖 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/lib/onboard/machine/handlers/sandbox-messaging.ts` around lines 378 - 415, Add a resume-path test for registry authority using the existing staged-plan divergence test as a template, with checkpoint channels differing from the authoritative registry plan. Verify setupMessagingChannels is not called and the registry plan is returned unchanged.Source: Path instructions
2 participants
Summary
Extract final onboarding flow composition from
src/lib/onboard.tsinto the machine layer. Runtime behavior stays unchanged, while finalization dependencies gain one owner and the architecture guardrails ratchet downward.Related Issue
Refs #7695
Changes
Type of Change
Quality Gates
7b97b0158dd1dfb16f33e4cee2663ab3df353ad1against base commit89e17bfe9a22ffc6b16c8407ce83a2c0a493ba74; no findings.Security Review
PASS; no findings.7b97b0158dd1dfb16f33e4cee2663ab3df353ad1.89e17bfe9a22ffc6b16c8407ce83a2c0a493ba74.PASS; no secret storage, credential values, environment forwarding, or logging changed.PASS; no input boundary changed.PASS; finalization still invokes the same scoped auto-pair approval and warm-up handlers.PASS; no dependency or artifact changed.PASS; readiness, recovery, and deployment-exit behavior are unchanged.PASS; no cryptographic or persisted-data behavior changed.PASS; no network policy, service exposure, permission, image, or HTTP configuration changed.PASS; the composition test pins the recovery and readiness dependency set, relocated handler tests remain intact, the live-slice probe follows the moved module, and exact-head GitHub checks pass for build/typecheck, static checks, CLI tests and all eight shards, installer integration, plugin tests, WSL/macOS tests, CodeQL, architecture guardrails, DCO, commit lint, and both automated advisors. The authoritative live E2E gate is still running and is tracked separately as a merge gate.PASS; the composition layer injects the same finalization handlers previously supplied bysrc/lib/onboard.ts, with the option type preventing callers from replacing those owned handlers.Documentation Writer Review
no-docs-needed7b97b0158dd1dfb16f33e4cee2663ab3df353ad1against base commit89e17bfe9a22ffc6b16c8407ce83a2c0a493ba74. The change only relocates finalization dependencies into the machine layer, adds a composition wrapper that injects the same recovery and readiness dependencies, updates imports and tests, and ratchets architecture budgets. Commands, configuration, defaults, output, errors, workflows, and supported behavior remain unchanged. The composition regression test verifies the dependency invariant and uses a behavior-oriented title; the shortened import-cycle comment follows the writing rules. Nodocs/orfern/files changed. The prior and refreshed patches have the same stable patch ID, every substantive commit is unchanged ingit range-diff, andgit diff --checkpassed. Exact-head GitHub checks pass for build/typecheck, static checks, CLI tests and all eight shards, installer integration, plugin tests, WSL/macOS tests, CodeQL, architecture guardrails, DCO, commit lint, and automated reviews. The live E2E gate remains pending; that does not change the documentation-impact verdict.DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable —pre-commitandcommit-msgpassed. The linked worktree lacks generateddist/artifacts required bypre-push; GitHub Actions is the validation authority.test/onboard-fsm-live-slices.test.tspassed 13 integration tests.npm run typecheck:cliand normal pre-commit, commit-msg, and pre-push hooks passed for PR commit558415d50555844e539b2d101791b1827199ff0a; the clean merge-main refresh to7b97b0158dd1dfb16f33e4cee2663ab3df353ad1preserves every substantive commit, and exact-head CI repeats the applicable source checks successfully.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Core GitHub checks passed for commit7b97b0158dd1dfb16f33e4cee2663ab3df353ad1; the required E2E gate is still running.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit