refactor(onboard): extract initial flow composition - #8265
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>
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. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesInitial onboarding now uses Initial onboarding composition
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Onboard
participant InitialFlowComposition
participant InitialFlowPhases
participant GatewayRuntime
Onboard->>InitialFlowComposition: createInitialOnboardFlowPhases(options)
InitialFlowComposition->>GatewayRuntime: resolve gateway cleanup and container verification
InitialFlowComposition->>InitialFlowPhases: create phases with merged dependencies
InitialFlowPhases-->>Onboard: return initial onboarding phases
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit d40b64f in the TypeScript / code-coverage/cliThe overall coverage in commit d40b64f in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.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: 2
🧹 Nitpick comments (9)
src/lib/onboard/machine/handlers/sandbox-messaging.test.ts (1)
150-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
slackPlanfixture instead of copying it.
slackPlanhere is identical to the helper atsrc/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.tslines 150-167, including thealpha-slack-bridgeprovider name and theSLACK_BOT_TOKENplaceholder. Both files sit in the same directory, and this change set already addssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tsfor shared messaging fixtures. MoveslackPlanthere and import it in both suites, so a future change to the plan shape updates one definition.🤖 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.test.ts` around lines 150 - 167, Move the duplicated slackPlan fixture into the shared sandbox-test-fixtures module, then import and reuse it in both sandbox-messaging.test.ts and sandbox-provider-effect-replay.test.ts. Preserve the existing Slack channel, provider, credential, and placeholder values while removing each suite-local definition.src/lib/onboard/channel-state.test.ts (1)
93-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the session sandbox-name filter.
resolveDisabledChannelspassessession.messagingPlanonly whensession.sandboxName === sandboxName. No test covers a session that targets a different sandbox, so a regression that drops that guard would pass the suite and leak another sandbox's disabled channels into the current run.💚 Proposed test addition
+ it("ignores a saved session that targets another sandbox", () => { + expect( + resolveDisabledChannels("alpha", { + loadSession: () => sessionWithPlan("beta", ["telegram"]), + readMessagingPlanFromEnv: () => null, + getRegistryMessagingAuthority: () => ({ authoritative: false, plan: null }), + }), + ).toEqual([]); + });🤖 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/channel-state.test.ts` around lines 93 - 100, Add a test adjacent to the existing resolveDisabledChannels session-plan test that supplies a session plan for a different sandbox name and verifies resolveDisabledChannels returns no disabled channels when registry and staged plans are absent. This should specifically cover the session.messagingPlan guard against session.sandboxName mismatching the requested sandboxName.src/lib/onboard/messaging-reuse.test.ts (1)
19-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the
disabledChannelIdsparameter, or drop it.
authoritativeRegistryacceptsdisabledChannelIdsand wires it intodisabled,active, anddisabledChannels, but every call site passes onlychannelIds. The disabled-channel filter atsrc/lib/onboard/messaging-reuse.tsline 57 therefore has no test. That filter is the guard that keeps an operator-disabled channel out of non-interactive reuse, so it is worth pinning while the fixture already supports it.💚 Proposed test addition
+ it("does not reuse a channel the plan marks disabled", () => { + expect( + getNonInteractiveStoredMessagingChannels( + false, + null, + "assistant", + messagingChannels, + () => false, + () => authoritativeRegistry(["slack"], ["slack"]), + () => true, + true, + ), + ).toBeNull(); + });🤖 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/messaging-reuse.test.ts` around lines 19 - 49, Update the tests using authoritativeRegistry to pass at least one disabled channel through disabledChannelIds, and add assertions covering non-interactive reuse in messaging reuse so that the disabled channel is excluded while an enabled channel remains reusable. Preserve the fixture’s existing authoritative plan setup and exercise the disabled-channel filter in messaging-reuse.ts.src/lib/onboard/machine/handlers/sandbox-messaging.ts (1)
385-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated resume-completion gate.
Line 386 tests
(!agentName || agentName === "openclaw") && options.resume && messagingDecisionCompleted. Lines 407-409 test the exact De Morgan negation of the same predicate. The two spellings agree today. They are in adjacent functions that receive the same three inputs, so a later edit to one is likely to miss the other.Extract one helper and call it from both.
♻️ Proposed refactor
+function reusesCompletedOpenclawCheckpoint<Agent>( + messagingDecisionCompleted: boolean, + options: ReconcileSandboxMessagingOptions<Agent>, +): boolean { + const agentName = (options.agent as MessagingAgentLike | null)?.name; + if (agentName && agentName !== "openclaw") return false; + return options.resume && messagingDecisionCompleted; +} + async function selectionFromRegistryAuthority<Agent>( authority: ReturnType<typeof resolveMessagingPlanAuthority>, envPlan: SandboxMessagingPlan | null, messagingDecisionCompleted: boolean, options: ReconcileSandboxMessagingOptions<Agent>, ): Promise<SandboxMessagingSelection | null> { if (authority.source !== "registry") return null; - const agentName = (options.agent as MessagingAgentLike | null)?.name; - if ((!agentName || agentName === "openclaw") && options.resume && messagingDecisionCompleted) { + if (reusesCompletedOpenclawCheckpoint(messagingDecisionCompleted, options)) { return selectionFromCompletedMessagingCheckpoint(envPlan, options, authority.plan, false); } @@ ): Promise<SandboxMessagingSelection | null> { - const agentName = (options.agent as MessagingAgentLike | null)?.name; - if ((agentName && agentName !== "openclaw") || !options.resume || !messagingDecisionCompleted) { - return null; - } + if (!reusesCompletedOpenclawCheckpoint(messagingDecisionCompleted, options)) return null; const stagedPlan = stagedPlanFromAuthority(authority);Also applies to: 406-409
🤖 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 385 - 386, Extract the shared resume-completion gate from the adjacent handlers into a helper, using the existing agent-name, resume, and messagingDecisionCompleted inputs. Replace both the predicate in the current condition and its De Morgan negation around the later branch with calls to that helper, preserving their existing positive and negative control flow.src/lib/messaging/plan-authority.test.ts (1)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
nonesource.The suite covers
registry,staged, andsession, but not the final{ source: "none", plan: null }return. That value is load-bearing:getStoredMessagingChannelConfiginsrc/lib/onboard/messaging-config.tsgates the legacy session fallback onauthority.source === "none". A test pins that contract.💚 Proposed test addition
it("rejects a session plan that targets another sandbox", () => { expect(() => resolveMessagingPlanAuthority({ sandboxName: "alpha", registry: { authoritative: false, plan: null }, stagedPlan: null, sessionPlan: plan("beta", "onboard"), }), ).toThrow("Session messaging plan targets 'beta', not 'alpha'."); }); + + it("reports no source when no plan is available", () => { + expect( + resolveMessagingPlanAuthority({ + sandboxName: "alpha", + registry: { authoritative: false, plan: null }, + stagedPlan: null, + sessionPlan: null, + }), + ).toEqual({ source: "none", plan: null }); + }); });🤖 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/messaging/plan-authority.test.ts` around lines 95 - 105, Add a test in the resolveMessagingPlanAuthority suite covering the no-authority case: provide no registry, staged, or session plan and assert the result is exactly { source: "none", plan: null }. Keep the existing registry, staged, and session coverage unchanged, and ensure the assertion pins the source value used by getStoredMessagingChannelConfig.src/lib/onboard/machine/handlers/sandbox.ts (3)
492-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting why this helper keeps empty receipt names.
checkpointProviderReceiptNamesdoes not filter empty entries, whilecanonicalCheckpointProviderReceiptNamesrejects them. The asymmetry is safe today.checkpointProviderReceiptNamesfeeds only theSetmembership tests invalidateProviderBindingsForRegistration, andisCanonicalCheckpointProviderBindingalready rejects an empty binding name, so an empty entry can never match. A short comment would prevent a later change from reusing this helper where an empty name matters.♻️ Proposed comment
+// Membership-only helper for cross-group ownership checks. Empty entries are +// harmless here because canonical bindings never carry an empty name. Use +// canonicalCheckpointProviderReceiptNames when receipt shape must be proven. function checkpointProviderReceiptNames( checkpoint: OnboardCheckpoint, group: ProviderEffectGroupName, ): string[] { return checkpoint.effectGroups[group]?.fingerprint.split(",") ?? []; }🤖 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.ts` around lines 492 - 497, Add a brief comment above checkpointProviderReceiptNames explaining that it intentionally preserves empty receipt-name entries because its current consumers only perform Set membership checks, while isCanonicalCheckpointProviderBinding rejects empty binding names. Do not change the helper’s filtering behavior.
1951-1960: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider explaining why the combined receipt write follows the per-group writes.
Lines 1933 and 1946 already record each provider effect group through
recordCheckpointProviderEffectGroup. This block then callsrecordCheckpointProviderEffectGroupsfor both groups. The sequence reads as duplicated work, but it is not. The per-group form merges incrementally and preserves the other group's bindings, while the combined form rewritesbindings.registeredProvidersto exactly the two current groups and drops receipts that neither group owns. That pruning is what clears an obsolete receipt when the current run disabled web search or messaging.Add a short comment so a later change does not remove this call as redundant.
♻️ Proposed comment
if (this.resumesSandboxPrompts) { + // Converge the ledger on exactly the two current groups. The per-group + // writes above merge incrementally and preserve unrelated bindings, so + // this call is what prunes receipts that neither current group owns. const session = this.deps.updateSession((current) => { recordCheckpointProviderEffectGroups(current, { webSearch: webSearchProviderBindings, messaging: messagingProviderBindings, }); return current; }); nextState = { ...nextState, session }; }🤖 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.ts` around lines 1951 - 1960, In the resumesSandboxPrompts block, add a short comment immediately before recordCheckpointProviderEffectGroups explaining that the combined write intentionally prunes obsolete receipts by replacing registeredProviders with the current webSearch and messaging groups, following the incremental per-group writes.
2001-2003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit assertion wrapper for this validation gate.
replayableCheckpointProviderBindingsreturns a value, and this call site discards it. The call exists only for its fail-closed side effect: it exits the process when the checkpoint holds duplicate, non-canonical, or unmatched provider bindings. A reader can mistake the discarded result for dead code.Extract a named guard so the intent is visible at the call site. The ordering is correct as written, because this gate runs before any prompt, resume decision, or durable write.
♻️ Proposed refactor
+ private assertCheckpointProviderBindingsValid(checkpoint: OnboardCheckpoint): void { + this.replayableCheckpointProviderBindings(checkpoint); + } + async run(): Promise<SandboxStateResult<WebSearchConfig>> { if (this.options.session?.checkpoint) { - this.replayableCheckpointProviderBindings(this.options.session.checkpoint); + this.assertCheckpointProviderBindingsValid(this.options.session.checkpoint); }🤖 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.ts` around lines 2001 - 2003, In the checkpoint validation block, replace the discarded result from replayableCheckpointProviderBindings with a named assertion guard that clearly expresses the fail-closed validation intent. Keep the guard before any prompt, resume decision, or durable write, and preserve the existing process-exit behavior for invalid checkpoint bindings.src/lib/onboard/checkpoint-record.ts (1)
29-44: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject provider names that contain the fingerprint delimiter.
assertValidProviderBindingsis the single gate for names that later become a comma-joined fingerprint at Lines 169, 177, and 238. It does not reject a name that contains,. If such a name is recorded,checkpointProviderEffectGroupNamessplits the fingerprint into names that do not exist inregisteredProviders, and the next call throws "provider effect group receipt does not match registered credential bindings". The session then cannot resume without discarding the checkpoint.Add the delimiter check in the same validator so the encode/decode invariant holds at the write boundary.
♻️ Proposed guard for the fingerprint delimiter
(binding) => !binding.name || !binding.type || !binding.credentialEnv || + binding.name.includes(",") || binding.name.trim() !== binding.name ||🤖 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-record.ts` around lines 29 - 44, Update assertValidProviderBindings to reject any binding.name containing the comma fingerprint delimiter, alongside its existing name validation. Keep the check at this write-boundary validator so names encoded by checkpointProviderEffectGroupNames remain safely reversible.
🤖 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/final-flow-composition.test.ts`:
- Around line 10-13: Update the vi.mock factory for "./final-flow-phases" to
override the createFinalFlowPhases export, matching the factory invoked by
createFinalOnboardFlowPhases. Keep the original exports spread intact so the
test exercises the mocked phase factory and allows the finalizationDeps
assertion to observe the merged dependencies.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 450-457: Update the module-local selectionFromRecordedChannels
helper and its sole call site to remove the registryPlan parameter, including
the literal null argument passed from the recordedChannels branch. Delete the
obsolete registryPlan conditional branch and any forwarding glue or now-unused
support code, while preserving stagedPlanFromAuthority-based recorded-channel
selection.
---
Nitpick comments:
In `@src/lib/messaging/plan-authority.test.ts`:
- Around line 95-105: Add a test in the resolveMessagingPlanAuthority suite
covering the no-authority case: provide no registry, staged, or session plan and
assert the result is exactly { source: "none", plan: null }. Keep the existing
registry, staged, and session coverage unchanged, and ensure the assertion pins
the source value used by getStoredMessagingChannelConfig.
In `@src/lib/onboard/channel-state.test.ts`:
- Around line 93-100: Add a test adjacent to the existing
resolveDisabledChannels session-plan test that supplies a session plan for a
different sandbox name and verifies resolveDisabledChannels returns no disabled
channels when registry and staged plans are absent. This should specifically
cover the session.messagingPlan guard against session.sandboxName mismatching
the requested sandboxName.
In `@src/lib/onboard/checkpoint-record.ts`:
- Around line 29-44: Update assertValidProviderBindings to reject any
binding.name containing the comma fingerprint delimiter, alongside its existing
name validation. Keep the check at this write-boundary validator so names
encoded by checkpointProviderEffectGroupNames remain safely reversible.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.test.ts`:
- Around line 150-167: Move the duplicated slackPlan fixture into the shared
sandbox-test-fixtures module, then import and reuse it in both
sandbox-messaging.test.ts and sandbox-provider-effect-replay.test.ts. Preserve
the existing Slack channel, provider, credential, and placeholder values while
removing each suite-local definition.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 385-386: Extract the shared resume-completion gate from the
adjacent handlers into a helper, using the existing agent-name, resume, and
messagingDecisionCompleted inputs. Replace both the predicate in the current
condition and its De Morgan negation around the later branch with calls to that
helper, preserving their existing positive and negative control flow.
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 492-497: Add a brief comment above checkpointProviderReceiptNames
explaining that it intentionally preserves empty receipt-name entries because
its current consumers only perform Set membership checks, while
isCanonicalCheckpointProviderBinding rejects empty binding names. Do not change
the helper’s filtering behavior.
- Around line 1951-1960: In the resumesSandboxPrompts block, add a short comment
immediately before recordCheckpointProviderEffectGroups explaining that the
combined write intentionally prunes obsolete receipts by replacing
registeredProviders with the current webSearch and messaging groups, following
the incremental per-group writes.
- Around line 2001-2003: In the checkpoint validation block, replace the
discarded result from replayableCheckpointProviderBindings with a named
assertion guard that clearly expresses the fail-closed validation intent. Keep
the guard before any prompt, resume decision, or durable write, and preserve the
existing process-exit behavior for invalid checkpoint bindings.
In `@src/lib/onboard/messaging-reuse.test.ts`:
- Around line 19-49: Update the tests using authoritativeRegistry to pass at
least one disabled channel through disabledChannelIds, and add assertions
covering non-interactive reuse in messaging reuse so that the disabled channel
is excluded while an enabled channel remains reusable. Preserve the fixture’s
existing authoritative plan setup and exercise the disabled-channel filter in
messaging-reuse.ts.
🪄 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: 6c9248a0-a4c6-4e91-a862-b22f7fea036a
📒 Files selected for processing (47)
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/gateway-stale-port-reuse.test.tssrc/lib/onboard/machine/gateway-stale-port-reuse.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/initial-flow-composition.test.tssrc/lib/onboard/machine/initial-flow-composition.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: 2
🧹 Nitpick comments (9)
src/lib/onboard/machine/handlers/sandbox-messaging.test.ts (1)
150-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
slackPlanfixture instead of copying it.
slackPlanhere is identical to the helper atsrc/lib/onboard/machine/handlers/sandbox-provider-effect-replay.test.tslines 150-167, including thealpha-slack-bridgeprovider name and theSLACK_BOT_TOKENplaceholder. Both files sit in the same directory, and this change set already addssrc/lib/onboard/machine/handlers/sandbox-test-fixtures.tsfor shared messaging fixtures. MoveslackPlanthere and import it in both suites, so a future change to the plan shape updates one definition.🤖 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.test.ts` around lines 150 - 167, Move the duplicated slackPlan fixture into the shared sandbox-test-fixtures module, then import and reuse it in both sandbox-messaging.test.ts and sandbox-provider-effect-replay.test.ts. Preserve the existing Slack channel, provider, credential, and placeholder values while removing each suite-local definition.src/lib/onboard/channel-state.test.ts (1)
93-100: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPin the session sandbox-name filter.
resolveDisabledChannelspassessession.messagingPlanonly whensession.sandboxName === sandboxName. No test covers a session that targets a different sandbox, so a regression that drops that guard would pass the suite and leak another sandbox's disabled channels into the current run.💚 Proposed test addition
+ it("ignores a saved session that targets another sandbox", () => { + expect( + resolveDisabledChannels("alpha", { + loadSession: () => sessionWithPlan("beta", ["telegram"]), + readMessagingPlanFromEnv: () => null, + getRegistryMessagingAuthority: () => ({ authoritative: false, plan: null }), + }), + ).toEqual([]); + });🤖 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/channel-state.test.ts` around lines 93 - 100, Add a test adjacent to the existing resolveDisabledChannels session-plan test that supplies a session plan for a different sandbox name and verifies resolveDisabledChannels returns no disabled channels when registry and staged plans are absent. This should specifically cover the session.messagingPlan guard against session.sandboxName mismatching the requested sandboxName.src/lib/onboard/messaging-reuse.test.ts (1)
19-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the
disabledChannelIdsparameter, or drop it.
authoritativeRegistryacceptsdisabledChannelIdsand wires it intodisabled,active, anddisabledChannels, but every call site passes onlychannelIds. The disabled-channel filter atsrc/lib/onboard/messaging-reuse.tsline 57 therefore has no test. That filter is the guard that keeps an operator-disabled channel out of non-interactive reuse, so it is worth pinning while the fixture already supports it.💚 Proposed test addition
+ it("does not reuse a channel the plan marks disabled", () => { + expect( + getNonInteractiveStoredMessagingChannels( + false, + null, + "assistant", + messagingChannels, + () => false, + () => authoritativeRegistry(["slack"], ["slack"]), + () => true, + true, + ), + ).toBeNull(); + });🤖 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/messaging-reuse.test.ts` around lines 19 - 49, Update the tests using authoritativeRegistry to pass at least one disabled channel through disabledChannelIds, and add assertions covering non-interactive reuse in messaging reuse so that the disabled channel is excluded while an enabled channel remains reusable. Preserve the fixture’s existing authoritative plan setup and exercise the disabled-channel filter in messaging-reuse.ts.src/lib/onboard/machine/handlers/sandbox-messaging.ts (1)
385-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated resume-completion gate.
Line 386 tests
(!agentName || agentName === "openclaw") && options.resume && messagingDecisionCompleted. Lines 407-409 test the exact De Morgan negation of the same predicate. The two spellings agree today. They are in adjacent functions that receive the same three inputs, so a later edit to one is likely to miss the other.Extract one helper and call it from both.
♻️ Proposed refactor
+function reusesCompletedOpenclawCheckpoint<Agent>( + messagingDecisionCompleted: boolean, + options: ReconcileSandboxMessagingOptions<Agent>, +): boolean { + const agentName = (options.agent as MessagingAgentLike | null)?.name; + if (agentName && agentName !== "openclaw") return false; + return options.resume && messagingDecisionCompleted; +} + async function selectionFromRegistryAuthority<Agent>( authority: ReturnType<typeof resolveMessagingPlanAuthority>, envPlan: SandboxMessagingPlan | null, messagingDecisionCompleted: boolean, options: ReconcileSandboxMessagingOptions<Agent>, ): Promise<SandboxMessagingSelection | null> { if (authority.source !== "registry") return null; - const agentName = (options.agent as MessagingAgentLike | null)?.name; - if ((!agentName || agentName === "openclaw") && options.resume && messagingDecisionCompleted) { + if (reusesCompletedOpenclawCheckpoint(messagingDecisionCompleted, options)) { return selectionFromCompletedMessagingCheckpoint(envPlan, options, authority.plan, false); } @@ ): Promise<SandboxMessagingSelection | null> { - const agentName = (options.agent as MessagingAgentLike | null)?.name; - if ((agentName && agentName !== "openclaw") || !options.resume || !messagingDecisionCompleted) { - return null; - } + if (!reusesCompletedOpenclawCheckpoint(messagingDecisionCompleted, options)) return null; const stagedPlan = stagedPlanFromAuthority(authority);Also applies to: 406-409
🤖 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 385 - 386, Extract the shared resume-completion gate from the adjacent handlers into a helper, using the existing agent-name, resume, and messagingDecisionCompleted inputs. Replace both the predicate in the current condition and its De Morgan negation around the later branch with calls to that helper, preserving their existing positive and negative control flow.src/lib/messaging/plan-authority.test.ts (1)
95-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
nonesource.The suite covers
registry,staged, andsession, but not the final{ source: "none", plan: null }return. That value is load-bearing:getStoredMessagingChannelConfiginsrc/lib/onboard/messaging-config.tsgates the legacy session fallback onauthority.source === "none". A test pins that contract.💚 Proposed test addition
it("rejects a session plan that targets another sandbox", () => { expect(() => resolveMessagingPlanAuthority({ sandboxName: "alpha", registry: { authoritative: false, plan: null }, stagedPlan: null, sessionPlan: plan("beta", "onboard"), }), ).toThrow("Session messaging plan targets 'beta', not 'alpha'."); }); + + it("reports no source when no plan is available", () => { + expect( + resolveMessagingPlanAuthority({ + sandboxName: "alpha", + registry: { authoritative: false, plan: null }, + stagedPlan: null, + sessionPlan: null, + }), + ).toEqual({ source: "none", plan: null }); + }); });🤖 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/messaging/plan-authority.test.ts` around lines 95 - 105, Add a test in the resolveMessagingPlanAuthority suite covering the no-authority case: provide no registry, staged, or session plan and assert the result is exactly { source: "none", plan: null }. Keep the existing registry, staged, and session coverage unchanged, and ensure the assertion pins the source value used by getStoredMessagingChannelConfig.src/lib/onboard/machine/handlers/sandbox.ts (3)
492-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting why this helper keeps empty receipt names.
checkpointProviderReceiptNamesdoes not filter empty entries, whilecanonicalCheckpointProviderReceiptNamesrejects them. The asymmetry is safe today.checkpointProviderReceiptNamesfeeds only theSetmembership tests invalidateProviderBindingsForRegistration, andisCanonicalCheckpointProviderBindingalready rejects an empty binding name, so an empty entry can never match. A short comment would prevent a later change from reusing this helper where an empty name matters.♻️ Proposed comment
+// Membership-only helper for cross-group ownership checks. Empty entries are +// harmless here because canonical bindings never carry an empty name. Use +// canonicalCheckpointProviderReceiptNames when receipt shape must be proven. function checkpointProviderReceiptNames( checkpoint: OnboardCheckpoint, group: ProviderEffectGroupName, ): string[] { return checkpoint.effectGroups[group]?.fingerprint.split(",") ?? []; }🤖 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.ts` around lines 492 - 497, Add a brief comment above checkpointProviderReceiptNames explaining that it intentionally preserves empty receipt-name entries because its current consumers only perform Set membership checks, while isCanonicalCheckpointProviderBinding rejects empty binding names. Do not change the helper’s filtering behavior.
1951-1960: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider explaining why the combined receipt write follows the per-group writes.
Lines 1933 and 1946 already record each provider effect group through
recordCheckpointProviderEffectGroup. This block then callsrecordCheckpointProviderEffectGroupsfor both groups. The sequence reads as duplicated work, but it is not. The per-group form merges incrementally and preserves the other group's bindings, while the combined form rewritesbindings.registeredProvidersto exactly the two current groups and drops receipts that neither group owns. That pruning is what clears an obsolete receipt when the current run disabled web search or messaging.Add a short comment so a later change does not remove this call as redundant.
♻️ Proposed comment
if (this.resumesSandboxPrompts) { + // Converge the ledger on exactly the two current groups. The per-group + // writes above merge incrementally and preserve unrelated bindings, so + // this call is what prunes receipts that neither current group owns. const session = this.deps.updateSession((current) => { recordCheckpointProviderEffectGroups(current, { webSearch: webSearchProviderBindings, messaging: messagingProviderBindings, }); return current; }); nextState = { ...nextState, session }; }🤖 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.ts` around lines 1951 - 1960, In the resumesSandboxPrompts block, add a short comment immediately before recordCheckpointProviderEffectGroups explaining that the combined write intentionally prunes obsolete receipts by replacing registeredProviders with the current webSearch and messaging groups, following the incremental per-group writes.
2001-2003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider an explicit assertion wrapper for this validation gate.
replayableCheckpointProviderBindingsreturns a value, and this call site discards it. The call exists only for its fail-closed side effect: it exits the process when the checkpoint holds duplicate, non-canonical, or unmatched provider bindings. A reader can mistake the discarded result for dead code.Extract a named guard so the intent is visible at the call site. The ordering is correct as written, because this gate runs before any prompt, resume decision, or durable write.
♻️ Proposed refactor
+ private assertCheckpointProviderBindingsValid(checkpoint: OnboardCheckpoint): void { + this.replayableCheckpointProviderBindings(checkpoint); + } + async run(): Promise<SandboxStateResult<WebSearchConfig>> { if (this.options.session?.checkpoint) { - this.replayableCheckpointProviderBindings(this.options.session.checkpoint); + this.assertCheckpointProviderBindingsValid(this.options.session.checkpoint); }🤖 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.ts` around lines 2001 - 2003, In the checkpoint validation block, replace the discarded result from replayableCheckpointProviderBindings with a named assertion guard that clearly expresses the fail-closed validation intent. Keep the guard before any prompt, resume decision, or durable write, and preserve the existing process-exit behavior for invalid checkpoint bindings.src/lib/onboard/checkpoint-record.ts (1)
29-44: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject provider names that contain the fingerprint delimiter.
assertValidProviderBindingsis the single gate for names that later become a comma-joined fingerprint at Lines 169, 177, and 238. It does not reject a name that contains,. If such a name is recorded,checkpointProviderEffectGroupNamessplits the fingerprint into names that do not exist inregisteredProviders, and the next call throws "provider effect group receipt does not match registered credential bindings". The session then cannot resume without discarding the checkpoint.Add the delimiter check in the same validator so the encode/decode invariant holds at the write boundary.
♻️ Proposed guard for the fingerprint delimiter
(binding) => !binding.name || !binding.type || !binding.credentialEnv || + binding.name.includes(",") || binding.name.trim() !== binding.name ||🤖 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-record.ts` around lines 29 - 44, Update assertValidProviderBindings to reject any binding.name containing the comma fingerprint delimiter, alongside its existing name validation. Keep the check at this write-boundary validator so names encoded by checkpointProviderEffectGroupNames remain safely reversible.
🤖 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/final-flow-composition.test.ts`:
- Around line 10-13: Update the vi.mock factory for "./final-flow-phases" to
override the createFinalFlowPhases export, matching the factory invoked by
createFinalOnboardFlowPhases. Keep the original exports spread intact so the
test exercises the mocked phase factory and allows the finalizationDeps
assertion to observe the merged dependencies.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 450-457: Update the module-local selectionFromRecordedChannels
helper and its sole call site to remove the registryPlan parameter, including
the literal null argument passed from the recordedChannels branch. Delete the
obsolete registryPlan conditional branch and any forwarding glue or now-unused
support code, while preserving stagedPlanFromAuthority-based recorded-channel
selection.
---
Nitpick comments:
In `@src/lib/messaging/plan-authority.test.ts`:
- Around line 95-105: Add a test in the resolveMessagingPlanAuthority suite
covering the no-authority case: provide no registry, staged, or session plan and
assert the result is exactly { source: "none", plan: null }. Keep the existing
registry, staged, and session coverage unchanged, and ensure the assertion pins
the source value used by getStoredMessagingChannelConfig.
In `@src/lib/onboard/channel-state.test.ts`:
- Around line 93-100: Add a test adjacent to the existing
resolveDisabledChannels session-plan test that supplies a session plan for a
different sandbox name and verifies resolveDisabledChannels returns no disabled
channels when registry and staged plans are absent. This should specifically
cover the session.messagingPlan guard against session.sandboxName mismatching
the requested sandboxName.
In `@src/lib/onboard/checkpoint-record.ts`:
- Around line 29-44: Update assertValidProviderBindings to reject any
binding.name containing the comma fingerprint delimiter, alongside its existing
name validation. Keep the check at this write-boundary validator so names
encoded by checkpointProviderEffectGroupNames remain safely reversible.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.test.ts`:
- Around line 150-167: Move the duplicated slackPlan fixture into the shared
sandbox-test-fixtures module, then import and reuse it in both
sandbox-messaging.test.ts and sandbox-provider-effect-replay.test.ts. Preserve
the existing Slack channel, provider, credential, and placeholder values while
removing each suite-local definition.
In `@src/lib/onboard/machine/handlers/sandbox-messaging.ts`:
- Around line 385-386: Extract the shared resume-completion gate from the
adjacent handlers into a helper, using the existing agent-name, resume, and
messagingDecisionCompleted inputs. Replace both the predicate in the current
condition and its De Morgan negation around the later branch with calls to that
helper, preserving their existing positive and negative control flow.
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 492-497: Add a brief comment above checkpointProviderReceiptNames
explaining that it intentionally preserves empty receipt-name entries because
its current consumers only perform Set membership checks, while
isCanonicalCheckpointProviderBinding rejects empty binding names. Do not change
the helper’s filtering behavior.
- Around line 1951-1960: In the resumesSandboxPrompts block, add a short comment
immediately before recordCheckpointProviderEffectGroups explaining that the
combined write intentionally prunes obsolete receipts by replacing
registeredProviders with the current webSearch and messaging groups, following
the incremental per-group writes.
- Around line 2001-2003: In the checkpoint validation block, replace the
discarded result from replayableCheckpointProviderBindings with a named
assertion guard that clearly expresses the fail-closed validation intent. Keep
the guard before any prompt, resume decision, or durable write, and preserve the
existing process-exit behavior for invalid checkpoint bindings.
In `@src/lib/onboard/messaging-reuse.test.ts`:
- Around line 19-49: Update the tests using authoritativeRegistry to pass at
least one disabled channel through disabledChannelIds, and add assertions
covering non-interactive reuse in messaging reuse so that the disabled channel
is excluded while an enabled channel remains reusable. Preserve the fixture’s
existing authoritative plan setup and exercise the disabled-channel filter in
messaging-reuse.ts.
🪄 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: 6c9248a0-a4c6-4e91-a862-b22f7fea036a
📒 Files selected for processing (47)
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/gateway-stale-port-reuse.test.tssrc/lib/onboard/machine/gateway-stale-port-reuse.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/initial-flow-composition.test.tssrc/lib/onboard/machine/initial-flow-composition.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 (2)
src/lib/onboard/machine/final-flow-composition.test.ts (1)
10-13: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
The mock overrides the wrong export, so the test does not exercise its claim.
createFinalOnboardFlowPhasesinsrc/lib/onboard/machine/final-flow-composition.tscallscreateFinalFlowPhasesfrom./final-flow-phases. This factory overrides a key namedcreateFinalOnboardFlowPhaseson that module instead. The realcreateFinalFlowPhasestherefore still runs,mocks.createFinalFlowPhasesis never called, and the assertion at Line 34 cannot observe the mergedfinalizationDeps. Theas neveroptions object is also passed into the real phase factory.Override
createFinalFlowPhasesin the mock factory.🐛 Proposed fix for the mocked export name
vi.mock("./final-flow-phases", async (importOriginal) => ({ ...(await importOriginal<typeof import("./final-flow-phases")>()), - createFinalOnboardFlowPhases: mocks.createFinalFlowPhases, + createFinalFlowPhases: mocks.createFinalFlowPhases, }));Run the following script to confirm the exported factory name and the re-export used at Line 15:
#!/bin/bash # Description: Confirm the export names in final-flow-phases.ts and final-flow-composition.ts. set -euo pipefail ast-grep outline src/lib/onboard/machine/final-flow-phases.ts --items all ast-grep outline src/lib/onboard/machine/final-flow-composition.ts --items all rg -n 'export (const|function|\{)' src/lib/onboard/machine/final-flow-phases.ts src/lib/onboard/machine/final-flow-composition.ts rg -n 'finalizationHandlerDeps' src/lib/onboard/machine/final-flow-composition.tsBased on the path instruction "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/final-flow-composition.test.ts` around lines 10 - 13, Update the vi.mock factory for "./final-flow-phases" to override the createFinalFlowPhases export, matching the factory invoked by createFinalOnboardFlowPhases. Keep the original exports spread intact so the test exercises the mocked phase factory and allows the finalizationDeps assertion to observe the merged dependencies.src/lib/onboard/machine/handlers/sandbox-messaging.ts (1)
450-457: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove the now-dead
registryPlanparameter fromselectionFromRecordedChannels.Line 454 passes a literal
nullforregistryPlan.selectionFromRecordedChannelsis module-local and this is its only call site, so theelse if (registryPlan)branch at lines 220-221 can no longer execute. The registry-authoritative case already returned at line 437.Leaving the parameter keeps a superseded selection path alive in the signature and hides the fact that recorded-channel reuse now considers only staged plans. Delete the parameter and the branch.
As per path instructions: "Require in-scope callers to use one authoritative path and delete the superseded runtime path, forwarding glue, support helpers, and tests in the same PR."
♻️ Proposed refactor
Update the helper:
function selectionFromRecordedChannels<Agent>( recordedChannels: string[], envPlan: SandboxMessagingPlan | null, - registryPlan: SandboxMessagingPlan | null, options: ReconcileSandboxMessagingOptions<Agent>, ): SandboxMessagingSelection { let selection: SandboxMessagingSelection = { plan: null, selectedChannels: filterChannelNamesForCurrentAgent(recordedChannels, options.agent), }; if (envPlan) selection = selectionFromReusablePlan(envPlan, options.agent, false, options.deps); - else if (registryPlan) - selection = selectionFromReusablePlan(registryPlan, options.agent, true, options.deps); if (selection.selectedChannels.length > 0) {Update the call site:
if (recordedChannels) { return selectionFromRecordedChannels( recordedChannels, stagedPlanFromAuthority(authority), - null, options, ); }🤖 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 the module-local selectionFromRecordedChannels helper and its sole call site to remove the registryPlan parameter, including the literal null argument passed from the recordedChannels branch. Delete the obsolete registryPlan conditional branch and any forwarding glue or now-unused support code, while preserving stagedPlanFromAuthority-based recorded-channel selection.Source: Path instructions
<!-- markdownlint-disable MD041 --> ## Summary Correct changes that conflict resolution altered in #8265. The source architecture limits now match the measured line counts, the lifecycle contract states the validated ordering directly, and a comment that repeated the file role is removed. ## Changes - Set the `src/lib/state/registry.ts`, `src/lib/onboard.ts`, and `src/lib/onboard` source architecture limits to their measured values: 98, 219, and 307. - State that resume registry removal and `repair-and-recreate` occur only after complete intent validation. - Remove the generic `resume-provider-shim.ts` file-role comment. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: repository architecture checks enforce the three limits, and the sandbox-create-intent boundary tests verify complete intent validation before repair or removal. GitHub Actions is the validation authority. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this does not change a command, configuration, output, default, or supported behavior. `lifecycle-contracts.md` is internal architecture documentation. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: independent review of `edf3652b48b3179a9950e25c3ec51f4be4ac52ed` against `3a39ff352f98c4630ab8e18b83f4ec43ac56a2cc` passed all nine security categories. The change does not alter runtime, trust-boundary, dependency, credential, or policy behavior. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: `src/lib/onboard/lifecycle-contracts.md` states the ordering accurately and directly; the repeated file-role comment was removed; the source architecture limits match the measured values. No user-facing documentation page needs an update. - Agent: Codex Desktop <!-- docs-review-head-sha: edf3652 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable — `pre-commit` and `commit-msg` passed. The linked worktree does not contain the generated `dist/` files required by `pre-push`; GitHub Actions is the validation authority. - [ ] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — pending GitHub Actions repository architecture and sandbox-create-intent checks. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — not applicable to this bounded limit and internal wording correction. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified onboarding recovery guidance so registry cleanup and recreation occur only after complete intent validation. * **Chores** * Updated internal architecture quality thresholds and streamlined implementation notes. * No changes to exported functionality or end-user behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
2 participants
Summary
Extract initial onboarding flow composition from
src/lib/onboard.tsinto the machine layer. Runtime behavior stays unchanged, while gateway verification and cleanup dependencies gain one composition owner and the architecture guardrails ratchet downward.Related Issue
Refs #7695
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededee1290b34...d40b64f37confirmed that the change only affects internal initial-flow composition, file placement, and architecture budgets. No user-visible surface changes, and no documentation build is required.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 unavailablenpm run test:changedpassed (1,067 tests);npm run typecheck:cli,npm run checks:repository, and the source architecture check passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Improvements
Tests