Skip to content

refactor(onboard): persist messagingPlan in session for resume#4901

Merged
cv merged 7 commits into
mainfrom
feat/persist-messaging-plan-in-session
Jun 7, 2026
Merged

refactor(onboard): persist messagingPlan in session for resume#4901
cv merged 7 commits into
mainfrom
feat/persist-messaging-plan-in-session

Conversation

@sandl99
Copy link
Copy Markdown
Contributor

@sandl99 sandl99 commented Jun 7, 2026

Summary

Persists the compiled SandboxMessagingPlan in the onboard session so that --resume runs carry the full channel plan across process restarts without re-reading from env or registry. Establishes messagingPlan as the canonical session field for channel state, with helpers to derive messagingChannels, disabledChannels, and messagingChannelConfig from it.

Related Issue

Closes #4902

Changes

  • messaging-channel-setup.ts — move plan env helpers (writePlanToEnv, readMessagingPlanFromEnv) here from onboard.ts
  • messaging-plan-session.ts — add getChannelsFromPlan, getDisabledChannelsFromPlan, getMessagingChannelConfigFromPlan derivation helpers and parseSandboxMessagingPlan for env deserialization
  • handlers/sandbox.ts — persist messagingPlan to session after sandbox step; restore plan from env or registry on non-interactive resume with plan identity guard
  • onboard-session.ts — add messagingPlan: SandboxMessagingPlan | null to Session and SessionUpdates

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: San Dang sdang@nvidia.com

Stores the compiled SandboxMessagingPlan in the onboard session so that
resume runs can restore the plan to env without re-running enrollment
hooks (token paste, QR pairing). Fixes the gap where the registry entry
lost its `messaging` field on rebuild because the plan was only held in
a process env var that didn't survive across process restarts.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sandl99 sandl99 self-assigned this Jun 7, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 7, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Persist compiled SandboxMessagingPlan across sandbox runs: add parsing and session fields, propagate plan through update inputs, expose env read/write and registry accessor, wire helpers into handleSandboxState, stage plans during rebuild, and add tests for restore and persistence behavior.

Changes

Messaging Plan Persistence

Layer / File(s) Summary
Session types, parsing, and persistence
src/lib/onboard/messaging-plan-session.ts, src/lib/state/onboard-session.ts
Add parseSandboxMessagingPlan; extend Session/SessionUpdates with messagingPlan; parse in createSession/normalizeSession and persist via filterSafeUpdates.
Session update input and conversion
src/lib/onboard/session-updates.ts
Add `messagingPlan?: SandboxMessagingPlan
Messaging-plan helpers and config derivation
src/lib/onboard/messaging-plan-session.ts
Add getChannelsFromPlan, getDisabledChannelsFromPlan, and getMessagingChannelConfigFromPlan plus existing parseSandboxMessagingPlan.
Env helpers and registry accessor
src/lib/onboard/messaging-channel-setup.ts
Export readMessagingPlanFromEnv() and writePlanToEnv(plan) delegating to MessagingSetupApplier, and add getRegistrySandboxMessagingPlan(sandboxName).
Onboard wiring of deps
src/lib/onboard.ts
Import new env/registry helpers and inject readMessagingPlanFromEnv, writePlanToEnv, getRegistrySandboxMessagingPlan into handleSandboxState deps.
Sandbox handler messaging plan behavior
src/lib/onboard/machine/handlers/sandbox.ts
Thread messagingPlan through resume/setup: prefer env plan, fallback to registry and write to env, or read env after channel setup; persist current.messagingPlan on session updates.
Stage and persist rebuild messaging plan
src/lib/actions/sandbox/rebuild.ts
stageMessagingManifestPlanForRebuild returns the staged plan (or null); rebuildSandbox captures it and stores it on the session for resume.
Tests for messaging-plan persistence and restore
src/lib/onboard/machine/handlers/sandbox.test.ts
Add makeMinimalPlan() helper, extend test deps with env hooks, and add tests covering env intake, registry fallback/write, env precedence, and missing-registry behavior.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as Onboard CLI
  participant Handler as handleSandboxState
  participant Env as MessagingSetupApplier
  participant Registry as registry.getSandbox
  participant Session as SessionStore

  CLI->>Handler: start or resume sandbox
  Handler->>Session: read current.messagingPlan
  alt resume with recorded channels
    Handler->>Env: readMessagingPlanFromEnv()
    Env-->>Handler: env plan|null
    alt env plan present
      Handler->>Session: persist current.messagingPlan = env plan
    else env plan missing
      Handler->>Registry: getRegistrySandboxMessagingPlan(sandboxName)
      Registry-->>Handler: registry plan|null
      alt registry plan present
        Handler->>Env: writePlanToEnv(registry plan)
        Handler->>Session: persist current.messagingPlan = registry plan
      else
        Handler->>Session: persist current.messagingPlan = null
      end
    end
  else no recorded channels
    Handler->>Env: setupMessagingChannels()
    Handler->>Env: readMessagingPlanFromEnv()
    Env-->>Handler: env plan|null
    Handler->>Session: persist current.messagingPlan = env plan|null
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • prekshivyas
  • ericksoa
  • jyaunches

"I nibbled at plans and stitched them tight,
Saved them in sessions for the resume's light.
Registry hummed, env shelved the map,
No more re-asking after a nap.
🥕🐰"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed All coding objectives from #4902 are met: messagingPlan field added to Session/SessionUpdates, plan persisted after setupMessagingChannels, restored on non-interactive resume, and helpers exported from messaging-channel-setup.ts.
Out of Scope Changes check ✅ Passed All changes are in-scope and directly support the stated objectives. No extraneous refactoring, hook result storage, mid-hook resume, or legacy field removal is present.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective: persisting the messagingPlan in the session to enable resume runs to restore the plan without re-executing enrollment hooks.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/persist-messaging-plan-in-session

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

E2E Advisor Recommendation

Required E2E: channels-stop-start-e2e, onboard-resume-e2e
Optional E2E: channels-add-remove-e2e, rebuild-openclaw-e2e, rebuild-hermes-e2e

Dispatch hint: channels-stop-start-e2e,onboard-resume-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • channels-stop-start-e2e (high; timeout 120 minutes): Directly covers the changed behavior: OpenClaw and Hermes messaging channels stop/start across rebuild, including disabledChannels, registry messaging.plan state, credential reuse, and channel config after rebuild.
  • onboard-resume-e2e (medium; timeout 60 minutes): Covers the core non-interactive onboard --resume flow touched by handleSandboxState, onboard dependency wiring, and onboard-session persistence changes.

Optional E2E

  • channels-add-remove-e2e (medium-high; timeout 75 minutes): Useful adjacent coverage for messaging plan/credential state when channels are added or removed and a rebuild relies on gateway-held credentials.
  • rebuild-openclaw-e2e (medium; timeout 60 minutes): General OpenClaw rebuild smoke for the modified rebuildSandbox path outside the full multi-channel matrix.
  • rebuild-hermes-e2e (medium; timeout 60 minutes): General Hermes rebuild smoke for the same rebuild/session handoff path with the Hermes agent image and runtime.

New E2E recommendations

  • messaging resume plan restore (medium): Existing channels-stop/start covers rebuild persistence, but there is no narrow E2E that simulates a process restart where onboard --resume has recorded messagingChannels, no env-staged plan, and must restore messaging.plan from the registry before recreating the sandbox.
    • Suggested test: Add a targeted messaging onboard-resume E2E that onboards with a fake Telegram/Discord channel, clears the plan env, resumes non-interactively from a completed sandbox step, and verifies registry messaging.plan plus rendered sandbox config are preserved.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: channels-stop-start-e2e,onboard-resume-e2e

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

E2E Scenario Advisor Recommendation

Required scenario E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-cloud-openclaw-discord, ubuntu-repo-cloud-openclaw-resume
Optional scenario E2E: ubuntu-repo-cloud-hermes-discord, ubuntu-repo-cloud-openclaw-telegram, ubuntu-repo-cloud-openclaw-token-rotation

Dispatch required scenario E2E:

  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-discord
  • gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-resume

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required scenario E2E

  • ubuntu-repo-cloud-openclaw: Core rebuild/onboard/session changes should be validated by the Ubuntu OpenClaw scenario, whose supplemental coverage includes the rebuild lifecycle assertions.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
  • ubuntu-repo-cloud-openclaw-discord: Messaging plan staging, env restoration, registry plan lookup, and session persistence changes affect channel onboarding; Discord gives a standard Ubuntu OpenClaw messaging path without special runners.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-discord
  • ubuntu-repo-cloud-openclaw-resume: handleSandboxState and onboard-session persistence changed in the non-interactive resume path; run the dedicated OpenClaw resume scenario to catch resume regressions.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-resume

Optional scenario E2E

  • ubuntu-repo-cloud-hermes-discord: Optional adjacent coverage for the same messaging-plan/session path with the Hermes agent, whose supported messaging platforms and setup differ from OpenClaw.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes-discord
  • ubuntu-repo-cloud-openclaw-telegram: Optional adjacent coverage for another OpenClaw messaging manifest and its channel-specific validation, exercising the shared plan persistence path with a different provider.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-telegram
  • ubuntu-repo-cloud-openclaw-token-rotation: Optional lifecycle-adjacent messaging coverage for provider rotation/isolation, useful because the PR changes persisted messaging plan/session state.
    • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-token-rotation

Relevant changed files

  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/onboard.ts
  • src/lib/onboard/machine/handlers/sandbox.ts
  • src/lib/onboard/messaging-channel-setup.ts
  • src/lib/onboard/messaging-plan-session.ts
  • src/lib/onboard/session-updates.ts
  • src/lib/state/onboard-session.ts

…el-setup

Exports readMessagingPlanFromEnv and writePlanToEnv from
messaging-channel-setup.ts (which already owns MessagingSetupApplier)
to keep src/lib/onboard.ts from growing. Collapses the one-name
messaging-channel-setup require into a single line to free headroom.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sandl99 sandl99 changed the title feat(onboard): persist messaging plan in session for resume refactor(onboard): persist messaging plan in session for resume Jun 7, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/onboard.ts`:
- Around line 6502-6503: The file exceeded growth budget by +2 lines; keep the
MessagingSetupApplier wiring but reduce lines: replace the two separate
arrow-wrapper properties (readMessagingPlanFromEnv: () =>
MessagingSetupApplier.readPlanFromEnv(), writePlanToEnv: (plan) =>
MessagingSetupApplier.writePlanToEnv(plan)) by a more compact form and remove
one additional non-functional line nearby (e.g., an extra blank line or
redundant comment) so you eliminate at least two lines total; ensure you
reference MessagingSetupApplier.readPlanFromEnv and
MessagingSetupApplier.writePlanToEnv exactly so behavior is preserved.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ff77f7f1-f35a-4224-938a-c5962d173b7a

📥 Commits

Reviewing files that changed from the base of the PR and between d7aa5e0 and 3e41d49.

📒 Files selected for processing (5)
  • src/lib/onboard.ts
  • src/lib/onboard/machine/handlers/sandbox.test.ts
  • src/lib/onboard/machine/handlers/sandbox.ts
  • src/lib/onboard/session-updates.ts
  • src/lib/state/onboard-session.ts

Comment thread src/lib/onboard.ts Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

PR Review Advisor

Findings: 2 needs attention, 9 worth checking, 0 nice ideas
Since last review: 1 prior item resolved, 7 still apply, 1 new item found

Review findings

🛠️ Needs attention

  • Non-interactive resume still does not restore the persisted session plan (src/lib/onboard/machine/handlers/sandbox.ts:270): The linked issue explicitly requires restoring the persisted plan to NEMOCLAW_MESSAGING_PLAN_B64 before createSandbox() so MessagingHostStateApplier.readPlanStateFromEnv() can repopulate registry.messaging. The resume path still only checks the env plan and then the registry plan. If both are empty, it leaves messagingPlan null and writes current.messagingPlan = messagingPlan, clearing the persisted fallback before createSandbox() runs.
    • Recommendation: After env and registry lookup fail, restore a validated and reconciled session.messagingPlan to env before createSandbox(), or recompile an equivalent plan from trusted manifests plus persisted channel/config state. Do not clear an existing session plan unless the current context proves it is invalid.
    • Evidence: Issue clause: “On the non-interactive resume path, restore the persisted plan to NEMOCLAW_MESSAGING_PLAN_B64 before createSandbox is called, so MessagingHostStateApplier.readPlanStateFromEnv() sees it and the registry entry retains its messaging field across rebuild.” Current code initializes messagingPlan = null, reads deps.readMessagingPlanFromEnv(), then deps.getRegistrySandboxMessagingPlan(sandboxName), but never consults session?.messagingPlan; the new test “does not restore plan to env when registry has no entry” codifies the missing fallback.
  • Sandbox handler test hotspot grew without offsetting extraction (src/lib/onboard/machine/handlers/sandbox.test.ts:1): The changed test file is already a large-file hotspot and grew by 84 lines. The deterministic monolith guard flags growth of 20 or more lines in a current monolith as needing extraction or offset before merge.
    • Recommendation: Move the new messaging-plan resume fixtures/tests into a focused test module or shared fixture helper, or otherwise offset the hotspot growth so sandbox.test.ts does not continue accumulating unrelated scenario coverage.
    • Evidence: Monolith delta: src/lib/onboard/machine/handlers/sandbox.test.ts grew from 319 to 403 lines, +84.

🔎 Worth checking

  • Source-of-truth review needed: Persisted compiled messaging plan restored to process env on non-interactive resume: The advisor marked localized patch analysis as missing.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: sandbox.ts checks env then registry only; session?.messagingPlan is never consulted and current.messagingPlan is assigned null when both are absent.
  • Source-of-truth review needed: Tolerant parsing of persisted messaging plans: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: messaging-plan-session.ts returns null for top-level mismatch and otherwise returns value as SandboxMessagingPlan.
  • Source-of-truth review needed: Durable compiled-plan source order across env, registry, session, and manifests: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: rebuild.ts stages rebuildMessagingPlan into env/session, sandbox.ts prefers env then registry, and createSandbox() registers only env-derived messaging state gated by sandboxName.
  • Persisted messaging plans are shallow-validated before becoming durable blueprint state (src/lib/onboard/messaging-plan-session.ts:7): parseSandboxMessagingPlan() validates only top-level plan fields and then casts the value to SandboxMessagingPlan. Nested fields can later influence registry messaging state, OpenShell provider bindings, network policy presets, hook requests, agent config rendering, build metadata, and state updates after being loaded from the durable session file or restored to env.
    • Recommendation: Prefer recompiling plans from trusted manifests and persisted selections on resume. If compiled plans must be durable, deep-validate and reconcile nested fields against the current sandbox name, effective agent, supported channel manifests, policy/render/build schemas, and selected channel set before storing or restoring them.
    • Evidence: parseSandboxMessagingPlan() checks schemaVersion, sandboxName, agent, workflow, and whether nested fields are arrays/objects, then returns value as SandboxMessagingPlan. MessagingHostStateApplier.readPlanStateFromEnv() can convert env plans into registry.messaging.plan, and downstream applyPolicyAtOpenShell(), applyCredentialsAtOpenShell(), and applyAgentConfigAtOpenShell() consume nested plan fields.
  • Env and registry plans can be replayed without agent or channel reconciliation (src/lib/onboard/machine/handlers/sandbox.ts:282): The resume path accepts any env-staged plan and otherwise restores a registry plan for the sandbox name without verifying that the plan's agent, workflow, active/selected channels, disabled-channel state, or supported channel set match the current resume context. The later registry registration path gates only on plan.sandboxName, so stale blueprint state can diverge from the current session and effective agent.
    • Recommendation: Before using an env, registry, or session plan, compute the effective agent and expected selected/active/disabled channel sets, require the plan to match them, and drop or recompile plans that mismatch sandbox name, agent, workflow, selected channels, active channels, or disabled channels.
    • Evidence: handleSandboxState() uses deps.readMessagingPlanFromEnv() or deps.getRegistrySandboxMessagingPlan(sandboxName) and writes the registry plan to env. In onboard.ts, messaging state registration checks only plannedMessagingState?.plan.sandboxName === sandboxName before registry.registerSandbox({ messaging }).
  • Compiled plan persistence broadens durable session contents without minimization tests (src/lib/state/onboard-session.ts:109): The session file now stores the full compiled SandboxMessagingPlan. Secret inputs are intended to be represented as availability metadata, but config inputs can carry serialized values and the plan includes channel configuration, policy, render, hook, build, state-update, and health-check metadata. This broadens durable contents in ~/.nemoclaw/onboard-session.json without explicit minimization or tests proving raw secrets are excluded.
    • Recommendation: Document and enforce which plan fields are safe to persist. Exclude secret-like or PII-adjacent config values where possible, and add tests that token-paste, host-QR, and in-sandbox enrollment paths never serialize raw secret inputs into session.messagingPlan.
    • Evidence: Session now includes messagingPlan: SandboxMessagingPlan | null. getMessagingChannelConfigFromPlan() reconstructs config values from plan.channels[].inputs[].value, and SandboxMessagingInputReference allows value?: MessagingSerializableValue for config inputs.
  • Runtime registry preservation remains unproven (src/lib/onboard/machine/handlers/sandbox.test.ts:351): The new tests verify handler-level calls to mocked read/write plan helpers, but they do not exercise the caller/callee boundary named by the issue: createSandbox() reading NEMOCLAW_MESSAGING_PLAN_B64 through MessagingHostStateApplier.readPlanStateFromEnv() and registering registry.messaging.plan.
    • Recommendation: Add a focused integration-style test that runs the non-interactive resume or rebuild-style recreate path through the create/registration boundary and asserts the recreated registry entry contains the expected messaging.plan.
    • Evidence: Current tests assert writePlanToEnv() calls and session updates. The runtime boundary is in onboard.ts where createSandbox() calls MessagingHostStateApplier.readPlanStateFromEnv() immediately before registry.registerSandbox().
  • Durable compiled-plan restore lacks a clear source-of-truth contract (src/lib/onboard/machine/handlers/sandbox.ts:274): This change introduces several possible sources for the same compiled messaging plan: env-staged rebuild state, registry messaging state, session.messagingPlan, and trusted manifests plus channel/config state. Comments explain env-over-registry preference, but the invalid states, canonical source order, reconciliation rules, source-fix constraint, regression test, and removal condition are not fully defined.
    • Recommendation: Document and enforce the canonical source order and reconciliation rules. Prefer making missing-env/missing-registry states impossible by recompiling from trusted manifests where feasible; otherwise add focused regression tests and a removal condition for the workaround.
    • Evidence: rebuild.ts stages a plan and writes s.messagingPlan, sandbox.ts prefers env then registry, onboard-session.ts persists session.messagingPlan, and messaging-plan-session.ts tolerantly returns null for invalid top-level shapes.
  • PR description is stale relative to the current diff: The PR body describes a policy migration with new/deleted files and helpers that are not present in the current patch after the revert. This makes the intended review scope harder to verify against the actual code changes.
    • Recommendation: Update the PR description to match the current diff: session persistence, env/registry plan restore plumbing, rebuild session staging, and the new messaging-plan-session helper. Remove claims about policy-selection and policy-presets files that are not changed here.
    • Evidence: The PR body references getPolicyPresetsFromPlan, messaging/applier/policy-presets.ts, policy-selection.ts, handlers/policies.ts, and initial-policy.ts changes, but the changedFiles list contains only rebuild/onboard/session/sandbox/messaging-channel setup files.

🌱 Nice ideas

  • None.
Consider writing more tests for
  • **Runtime validation** — Non-interactive resume restores session.messagingPlan to NEMOCLAW_MESSAGING_PLAN_B64 when env is empty and registry has no messaging plan before createSandbox().. The changed behavior crosses session persistence, process env transport, registry state, sandbox creation, policy/provider/config blueprint application, and rebuild recovery. Current coverage is primarily mock-level handler testing.
  • **Runtime validation** — Non-interactive resume preserves existing session.messagingPlan instead of overwriting it with null when no replacement plan is available.. The changed behavior crosses session persistence, process env transport, registry state, sandbox creation, policy/provider/config blueprint application, and rebuild recovery. Current coverage is primarily mock-level handler testing.
  • **Runtime validation** — Rebuild resume after old registry removal registers registry.messaging.plan through createSandbox() and MessagingHostStateApplier.readPlanStateFromEnv().. The changed behavior crosses session persistence, process env transport, registry state, sandbox creation, policy/provider/config blueprint application, and rebuild recovery. Current coverage is primarily mock-level handler testing.
  • **Runtime validation** — Resume rejects or recompiles an env, registry, or session plan whose sandboxName, agent, workflow, selected channels, active channels, or disabled channels do not match current session context.. The changed behavior crosses session persistence, process env transport, registry state, sandbox creation, policy/provider/config blueprint application, and rebuild recovery. Current coverage is primarily mock-level handler testing.
  • **Runtime validation** — Persisted messaging plan excludes raw secret, token, and QR enrollment values for token-paste, host-QR, and in-sandbox QR channels.. The changed behavior crosses session persistence, process env transport, registry state, sandbox creation, policy/provider/config blueprint application, and rebuild recovery. Current coverage is primarily mock-level handler testing.
  • **Runtime registry preservation remains unproven** — Add a focused integration-style test that runs the non-interactive resume or rebuild-style recreate path through the create/registration boundary and asserts the recreated registry entry contains the expected messaging.plan.
  • **Acceptance clause:** Persist the compiled plan to session after `setupMessagingChannels()` completes during the sandbox step. — add test evidence or identify existing coverage. handleSandboxState() reads deps.readMessagingPlanFromEnv() after setupMessagingChannels() and writes current.messagingPlan = messagingPlan; the new test “persists plan from env into session after fresh messaging setup” covers the mock path. recordStepComplete() does not include messagingPlan, so the behavior relies on the preceding updateSession mutation.
  • **Acceptance clause:** On the non-interactive resume path, restore the persisted plan to `NEMOCLAW_MESSAGING_PLAN_B64` before `createSandbox` is called, so `MessagingHostStateApplier.readPlanStateFromEnv()` sees it and the registry entry retains its `messaging` field across rebuild. — add test evidence or identify existing coverage. handleSandboxState() reads an env plan, then a registry plan, but never falls back to session?.messagingPlan. When both env and registry are empty it sets current.messagingPlan = null before createSandbox().
Since last review details

Current findings:

  • Source-of-truth review needed: Persisted compiled messaging plan restored to process env on non-interactive resume: The advisor marked localized patch analysis as missing.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: sandbox.ts checks env then registry only; session?.messagingPlan is never consulted and current.messagingPlan is assigned null when both are absent.
  • Source-of-truth review needed: Tolerant parsing of persisted messaging plans: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: messaging-plan-session.ts returns null for top-level mismatch and otherwise returns value as SandboxMessagingPlan.
  • Source-of-truth review needed: Durable compiled-plan source order across env, registry, session, and manifests: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: rebuild.ts stages rebuildMessagingPlan into env/session, sandbox.ts prefers env then registry, and createSandbox() registers only env-derived messaging state gated by sandboxName.
  • Non-interactive resume still does not restore the persisted session plan (src/lib/onboard/machine/handlers/sandbox.ts:270): The linked issue explicitly requires restoring the persisted plan to NEMOCLAW_MESSAGING_PLAN_B64 before createSandbox() so MessagingHostStateApplier.readPlanStateFromEnv() can repopulate registry.messaging. The resume path still only checks the env plan and then the registry plan. If both are empty, it leaves messagingPlan null and writes current.messagingPlan = messagingPlan, clearing the persisted fallback before createSandbox() runs.
    • Recommendation: After env and registry lookup fail, restore a validated and reconciled session.messagingPlan to env before createSandbox(), or recompile an equivalent plan from trusted manifests plus persisted channel/config state. Do not clear an existing session plan unless the current context proves it is invalid.
    • Evidence: Issue clause: “On the non-interactive resume path, restore the persisted plan to NEMOCLAW_MESSAGING_PLAN_B64 before createSandbox is called, so MessagingHostStateApplier.readPlanStateFromEnv() sees it and the registry entry retains its messaging field across rebuild.” Current code initializes messagingPlan = null, reads deps.readMessagingPlanFromEnv(), then deps.getRegistrySandboxMessagingPlan(sandboxName), but never consults session?.messagingPlan; the new test “does not restore plan to env when registry has no entry” codifies the missing fallback.
  • Sandbox handler test hotspot grew without offsetting extraction (src/lib/onboard/machine/handlers/sandbox.test.ts:1): The changed test file is already a large-file hotspot and grew by 84 lines. The deterministic monolith guard flags growth of 20 or more lines in a current monolith as needing extraction or offset before merge.
    • Recommendation: Move the new messaging-plan resume fixtures/tests into a focused test module or shared fixture helper, or otherwise offset the hotspot growth so sandbox.test.ts does not continue accumulating unrelated scenario coverage.
    • Evidence: Monolith delta: src/lib/onboard/machine/handlers/sandbox.test.ts grew from 319 to 403 lines, +84.
  • Persisted messaging plans are shallow-validated before becoming durable blueprint state (src/lib/onboard/messaging-plan-session.ts:7): parseSandboxMessagingPlan() validates only top-level plan fields and then casts the value to SandboxMessagingPlan. Nested fields can later influence registry messaging state, OpenShell provider bindings, network policy presets, hook requests, agent config rendering, build metadata, and state updates after being loaded from the durable session file or restored to env.
    • Recommendation: Prefer recompiling plans from trusted manifests and persisted selections on resume. If compiled plans must be durable, deep-validate and reconcile nested fields against the current sandbox name, effective agent, supported channel manifests, policy/render/build schemas, and selected channel set before storing or restoring them.
    • Evidence: parseSandboxMessagingPlan() checks schemaVersion, sandboxName, agent, workflow, and whether nested fields are arrays/objects, then returns value as SandboxMessagingPlan. MessagingHostStateApplier.readPlanStateFromEnv() can convert env plans into registry.messaging.plan, and downstream applyPolicyAtOpenShell(), applyCredentialsAtOpenShell(), and applyAgentConfigAtOpenShell() consume nested plan fields.
  • Env and registry plans can be replayed without agent or channel reconciliation (src/lib/onboard/machine/handlers/sandbox.ts:282): The resume path accepts any env-staged plan and otherwise restores a registry plan for the sandbox name without verifying that the plan's agent, workflow, active/selected channels, disabled-channel state, or supported channel set match the current resume context. The later registry registration path gates only on plan.sandboxName, so stale blueprint state can diverge from the current session and effective agent.
    • Recommendation: Before using an env, registry, or session plan, compute the effective agent and expected selected/active/disabled channel sets, require the plan to match them, and drop or recompile plans that mismatch sandbox name, agent, workflow, selected channels, active channels, or disabled channels.
    • Evidence: handleSandboxState() uses deps.readMessagingPlanFromEnv() or deps.getRegistrySandboxMessagingPlan(sandboxName) and writes the registry plan to env. In onboard.ts, messaging state registration checks only plannedMessagingState?.plan.sandboxName === sandboxName before registry.registerSandbox({ messaging }).
  • Compiled plan persistence broadens durable session contents without minimization tests (src/lib/state/onboard-session.ts:109): The session file now stores the full compiled SandboxMessagingPlan. Secret inputs are intended to be represented as availability metadata, but config inputs can carry serialized values and the plan includes channel configuration, policy, render, hook, build, state-update, and health-check metadata. This broadens durable contents in ~/.nemoclaw/onboard-session.json without explicit minimization or tests proving raw secrets are excluded.
    • Recommendation: Document and enforce which plan fields are safe to persist. Exclude secret-like or PII-adjacent config values where possible, and add tests that token-paste, host-QR, and in-sandbox enrollment paths never serialize raw secret inputs into session.messagingPlan.
    • Evidence: Session now includes messagingPlan: SandboxMessagingPlan | null. getMessagingChannelConfigFromPlan() reconstructs config values from plan.channels[].inputs[].value, and SandboxMessagingInputReference allows value?: MessagingSerializableValue for config inputs.
  • Runtime registry preservation remains unproven (src/lib/onboard/machine/handlers/sandbox.test.ts:351): The new tests verify handler-level calls to mocked read/write plan helpers, but they do not exercise the caller/callee boundary named by the issue: createSandbox() reading NEMOCLAW_MESSAGING_PLAN_B64 through MessagingHostStateApplier.readPlanStateFromEnv() and registering registry.messaging.plan.
    • Recommendation: Add a focused integration-style test that runs the non-interactive resume or rebuild-style recreate path through the create/registration boundary and asserts the recreated registry entry contains the expected messaging.plan.
    • Evidence: Current tests assert writePlanToEnv() calls and session updates. The runtime boundary is in onboard.ts where createSandbox() calls MessagingHostStateApplier.readPlanStateFromEnv() immediately before registry.registerSandbox().
  • Durable compiled-plan restore lacks a clear source-of-truth contract (src/lib/onboard/machine/handlers/sandbox.ts:274): This change introduces several possible sources for the same compiled messaging plan: env-staged rebuild state, registry messaging state, session.messagingPlan, and trusted manifests plus channel/config state. Comments explain env-over-registry preference, but the invalid states, canonical source order, reconciliation rules, source-fix constraint, regression test, and removal condition are not fully defined.
    • Recommendation: Document and enforce the canonical source order and reconciliation rules. Prefer making missing-env/missing-registry states impossible by recompiling from trusted manifests where feasible; otherwise add focused regression tests and a removal condition for the workaround.
    • Evidence: rebuild.ts stages a plan and writes s.messagingPlan, sandbox.ts prefers env then registry, onboard-session.ts persists session.messagingPlan, and messaging-plan-session.ts tolerantly returns null for invalid top-level shapes.
  • PR description is stale relative to the current diff: The PR body describes a policy migration with new/deleted files and helpers that are not present in the current patch after the revert. This makes the intended review scope harder to verify against the actual code changes.
    • Recommendation: Update the PR description to match the current diff: session persistence, env/registry plan restore plumbing, rebuild session staging, and the new messaging-plan-session helper. Remove claims about policy-selection and policy-presets files that are not changed here.
    • Evidence: The PR body references getPolicyPresetsFromPlan, messaging/applier/policy-presets.ts, policy-selection.ts, handlers/policies.ts, and initial-policy.ts changes, but the changedFiles list contains only rebuild/onboard/session/sandbox/messaging-channel setup files.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

…an persistence

- Extract parseSandboxMessagingPlan to messaging-plan-session.ts to
  keep onboard-session.ts growth under the monolith threshold
- Guard plan restoration with sandbox-name + agent identity check so
  stale plans from renamed sandboxes or agent switches are not reused
- Add three behavior assertions in sandbox.test.ts: fresh setup
  persists env plan to session; matching plan is restored to env on
  non-interactive resume; mismatched sandbox name skips restoration

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sandl99
Copy link
Copy Markdown
Contributor Author

sandl99 commented Jun 7, 2026

Addressed the PR advisor findings in 9a4ed8e:

Needs attention → fixed

  • Tests missing: Added 3 behavior assertions to sandbox.test.ts — fresh setup persists env plan into session; matching plan restored to env on non-interactive resume; mismatched sandbox name skips restoration.
  • Monolith growth: Extracted parseSandboxMessagingPlan to src/lib/onboard/messaging-plan-session.ts. onboard-session.ts is now +12 lines vs main (was +32).

Worth checking → fixed

  • Stale plan identity: Added sandbox-name + agent guard before writePlanToEnv. A plan from a renamed sandbox or switched agent is dropped and setup runs fresh.

E2E triggered: https://github.com/NVIDIA/NemoClaw/actions/runs/27084551984

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/onboard/machine/handlers/sandbox.ts (1)

270-298: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard mismatch path skips plan recompilation.

At Line 278-280, the comment says stale plans should go through the normal setup path, but when planMatchesCurrent is false the code just continues with recordedMessagingChannels and leaves messagingPlan null. This can recreate the “missing messaging state on createSandbox” path for stale-plan resumes.

Suggested fix
-    if (recordedMessagingChannels) {
+    let shouldSetupMessaging = !recordedMessagingChannels;
+    if (recordedMessagingChannels) {
       selectedMessagingChannels = recordedMessagingChannels;
       if (selectedMessagingChannels.length > 0) {
         deps.note(`  [non-interactive] Reusing messaging channel configuration: ${selectedMessagingChannels.join(", ")}`);
@@
         if (planMatchesCurrent && storedPlan != null) {
           deps.writePlanToEnv(storedPlan);
           messagingPlan = storedPlan;
+        } else {
+          shouldSetupMessaging = true;
         }
       }
-    } else {
+    }
+    if (shouldSetupMessaging) {
       const existing = sandboxName
         ? deps.getSandboxMessagingChannels(sandboxName) ?? session?.messagingChannels ?? null
         : session?.messagingChannels ?? null;
       selectedMessagingChannels = await deps.setupMessagingChannels(agent, existing, sandboxName);
       messagingPlan = deps.readMessagingPlanFromEnv();
     }
🤖 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 270 - 298, The
current branch reuses recordedMessagingChannels but if planMatchesCurrent is
false it leaves messagingPlan null and skips the normal setup path; instead when
recordedMessagingChannels exists but planMatchesCurrent is false you should
treat it like the "no recorded" case: call deps.setupMessagingChannels(agent,
existing, sandboxName) to obtain selectedMessagingChannels and then set
messagingPlan = deps.readMessagingPlanFromEnv(); only call
deps.writePlanToEnv(storedPlan) when planMatchesCurrent is true. Update the
logic around recordedMessagingChannels / planMatchesCurrent (using variables
recordedMessagingChannels, selectedMessagingChannels, messagingPlan, storedPlan,
planMatchesCurrent and methods deps.setupMessagingChannels,
deps.readMessagingPlanFromEnv, deps.writePlanToEnv) so stale plans are
recompiled via the normal setup path.
🧹 Nitpick comments (1)
src/lib/onboard/machine/handlers/sandbox.test.ts (1)

351-373: ⚡ Quick win

Add coverage for the agent-identity mismatch guard.

The new handler guard checks both sandboxName and agent identity, but the new tests only assert sandbox-name mismatch. Add an agent-mismatch case so the second branch is protected from regressions.

🤖 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.test.ts` around lines 351 - 373, The
tests cover sandbox-name mismatch but not the agent-identity mismatch guard in
handleSandboxState; add a new test similar to the "does not restore plan to env
when sandbox name does not match" case that keeps sandboxName equal but sets the
plan's agent identity (via makeMinimalPlan or the session.messagingPlan) to a
different agent than the provided session or resume input, mock
getRecordedMessagingChannelsForResume and writePlanToEnv as in the other tests,
call handleSandboxState with resume: true and the matching sandboxName, and
assert writePlanToEnv was not called to ensure the agent-identity guard is
exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/lib/onboard/machine/handlers/sandbox.ts`:
- Around line 270-298: The current branch reuses recordedMessagingChannels but
if planMatchesCurrent is false it leaves messagingPlan null and skips the normal
setup path; instead when recordedMessagingChannels exists but planMatchesCurrent
is false you should treat it like the "no recorded" case: call
deps.setupMessagingChannels(agent, existing, sandboxName) to obtain
selectedMessagingChannels and then set messagingPlan =
deps.readMessagingPlanFromEnv(); only call deps.writePlanToEnv(storedPlan) when
planMatchesCurrent is true. Update the logic around recordedMessagingChannels /
planMatchesCurrent (using variables recordedMessagingChannels,
selectedMessagingChannels, messagingPlan, storedPlan, planMatchesCurrent and
methods deps.setupMessagingChannels, deps.readMessagingPlanFromEnv,
deps.writePlanToEnv) so stale plans are recompiled via the normal setup path.

---

Nitpick comments:
In `@src/lib/onboard/machine/handlers/sandbox.test.ts`:
- Around line 351-373: The tests cover sandbox-name mismatch but not the
agent-identity mismatch guard in handleSandboxState; add a new test similar to
the "does not restore plan to env when sandbox name does not match" case that
keeps sandboxName equal but sets the plan's agent identity (via makeMinimalPlan
or the session.messagingPlan) to a different agent than the provided session or
resume input, mock getRecordedMessagingChannelsForResume and writePlanToEnv as
in the other tests, call handleSandboxState with resume: true and the matching
sandboxName, and assert writePlanToEnv was not called to ensure the
agent-identity guard is exercised.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: d4dbd24a-3cb7-4bd4-ad7a-facb49a8af1b

📥 Commits

Reviewing files that changed from the base of the PR and between 461c399 and 9a4ed8e.

📒 Files selected for processing (4)
  • src/lib/onboard/machine/handlers/sandbox.test.ts
  • src/lib/onboard/machine/handlers/sandbox.ts
  • src/lib/onboard/messaging-plan-session.ts
  • src/lib/state/onboard-session.ts
✅ Files skipped from review due to trivial changes (1)
  • src/lib/onboard/messaging-plan-session.ts

@sandl99
Copy link
Copy Markdown
Contributor Author

sandl99 commented Jun 7, 2026

Waiting for E2E confirm

@sandl99 sandl99 added area: messaging Messaging channels, bridges, manifests, or channel lifecycle refactor PR restructures code without intended behavior change area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow labels Jun 7, 2026
…tive resume

rebuild.ts stages an authoritative plan from the registry (which reflects
post-stop/-start channel mutations) before calling onboard --resume.
Previously, the session plan restoration was unconditionally overwriting
that staged plan, causing stopped channels to reappear as active after
rebuild.

Now the handler checks the env first: if a plan is already staged
(rebuild path), it is used as-is. The session plan is only restored when
the env is empty, covering the plain process-restart resume case this PR
was originally targeting.

Also adds a test asserting the rebuild-path preference.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Selective E2E Results — ❌ Some jobs failed

Run: 27085364196
Target ref: feat/persist-messaging-plan-in-session
Requested jobs: channels-add-remove-e2e, onboard-resume-e2e, messaging-providers-e2e, channels-stop-start-e2e, rebuild-openclaw-e2e, ubuntu-repo-cloud-openclaw-telegram
Summary: 0 passed, 0 failed, 4 skipped

Job Result
channels-add-remove-e2e ⚠️ cancelled
channels-stop-start-e2e ⏭️ skipped
messaging-providers-e2e ⏭️ skipped
onboard-resume-e2e ⏭️ skipped
rebuild-openclaw-e2e ⏭️ skipped
ubuntu-repo-cloud-openclaw-telegram ❓ not reported

Missing requested jobs: ubuntu-repo-cloud-openclaw-telegram. The reporting workflow needs to include these jobs.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Selective E2E Results — ❌ Some jobs failed

Run: 27085645605
Target ref: feat/persist-messaging-plan-in-session
Requested jobs: channels-add-remove-e2e, onboard-resume-e2e, messaging-providers-e2e, channels-stop-start-e2e, rebuild-openclaw-e2e, ubuntu-repo-cloud-openclaw-telegram
Summary: 1 passed, 0 failed, 4 skipped

Job Result
channels-add-remove-e2e ✅ success
channels-stop-start-e2e ⏭️ skipped
messaging-providers-e2e ⏭️ skipped
onboard-resume-e2e ⏭️ skipped
rebuild-openclaw-e2e ⏭️ skipped
ubuntu-repo-cloud-openclaw-telegram ❓ not reported

Missing requested jobs: ubuntu-repo-cloud-openclaw-telegram. The reporting workflow needs to include these jobs.

- On non-interactive resume, restore plan from registry (always current
  after stop/start/add/remove) instead of stale session snapshot;
  env-first priority preserved so rebuild.ts staging still wins

- In rebuild.ts, persist the staged plan to the session alongside
  messagingChannels/disabledChannels/messagingChannelConfig so the
  session is fully consistent during the rebuild window

- Add getChannelsFromPlan, getDisabledChannelsFromPlan, and
  getMessagingChannelConfigFromPlan helpers in messaging-plan-session.ts
  so the next PR can replace the three individual session fields with
  plan-derived reads

- Move MessagingHostStateApplier re-export to messaging-channel-setup
  and getRegistrySandboxMessagingPlan helper to keep onboard.ts net-neutral

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@sandl99 sandl99 added v0.0.61 Release target VRDC Issues and PRs submitted by NVIDIA VRDC test team. labels Jun 7, 2026
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/lib/onboard/messaging-plan-session.ts (1)

7-26: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Harden parseSandboxMessagingPlan element-level validation at the boundary.

Line 14–21 only verifies arrays exist, but not element shapes. A malformed persisted session payload can pass this parser and later break helpers (e.g., missing channelId or non-array inputs), causing incorrect restored state or runtime failure.

Suggested fix (validate minimal nested shape before accepting)
 export function parseSandboxMessagingPlan(value: unknown): SandboxMessagingPlan | null {
   if (
     !isObject(value) ||
     value.schemaVersion !== 1 ||
     typeof value.sandboxName !== "string" ||
     typeof value.agent !== "string" ||
     typeof value.workflow !== "string" ||
     !Array.isArray(value.channels) ||
     !Array.isArray(value.disabledChannels) ||
     !Array.isArray(value.credentialBindings) ||
     !isObject(value.networkPolicy) ||
     !Array.isArray(value.agentRender) ||
     !Array.isArray(value.buildSteps) ||
     !Array.isArray(value.stateUpdates) ||
-    !Array.isArray(value.healthChecks)
+    !Array.isArray(value.healthChecks) ||
+    !value.channels.every(
+      (c) =>
+        isObject(c) &&
+        typeof c.channelId === "string" &&
+        Array.isArray(c.inputs),
+    ) ||
+    !value.disabledChannels.every((c) => typeof c === "string")
   ) {
     return null;
   }
   return value as unknown as SandboxMessagingPlan;
 }
🤖 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-plan-session.ts` around lines 7 - 26, The parser
parseSandboxMessagingPlan currently only checks that several properties are
arrays/objects but not the shape of their elements; update the function to
validate minimal nested shapes for each collection before returning the cast:
ensure channels entries contain a string channelId and an array inputs (and that
each input has required fields like id/type as appropriate), disabledChannels
entries are strings (or objects with channelId), credentialBindings entries
include string keys and credentialId, agentRender and buildSteps are arrays of
the expected primitive/object shape (e.g., renderId/string or object with
name/type), stateUpdates entries have required fields (e.g., key and value
types), and healthChecks entries include required fields (e.g., id/string and
enabled boolean); if any element fails its shape check, return null. Locate and
update parseSandboxMessagingPlan to add these per-item guards (using
isObject/type checks and Array.isArray on nested arrays) so the final return
value can safely be cast to SandboxMessagingPlan.
🧹 Nitpick comments (3)
src/lib/onboard.ts (1)

6499-6501: Run the onboarding E2E matrix for this dependency wiring change.

Since this updates handleSandboxState dependencies in onboarding core, run the recommended src/lib/onboard.ts E2E jobs before merge.

As per coding guidelines, changes in src/lib/onboard.ts should be validated with the listed onboarding E2E workflows (e.g., cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e, and related jobs).

🤖 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.ts` around lines 6499 - 6501, This change updates
dependencies used by handleSandboxState in src/lib/onboard.ts (notably
readMessagingPlanFromEnv, writePlanToEnv, and getRegistrySandboxMessagingPlan);
before merging, run the onboarding E2E matrix (cloud-e2e,
sandbox-operations-e2e, rebuild-openclaw-e2e, channels-stop-start-e2e and
related onboarding jobs) to validate the new wiring and ensure no regressions in
sandbox onboarding flows.

Source: Coding guidelines

src/lib/onboard/machine/handlers/sandbox.test.ts (2)

369-386: 💤 Low value

Consider making the two plans distinguishable for test clarity.

Both registryPlan and rebuiltPlan are created with identical arguments (makeMinimalPlan("my-assistant")), making their content indistinguishable. While the test is functionally correct—the assertion on line 384 (expect(writePlanToEnv).not.toHaveBeenCalled()) definitively proves the env path was taken—using different sandbox names or agents would make the test intent clearer and guard against logic changes that might accidentally swap the plans.

📋 Suggested improvement
-    const registryPlan = makeMinimalPlan("my-assistant");
-    const rebuiltPlan = makeMinimalPlan("my-assistant");
+    const registryPlan = makeMinimalPlan("my-assistant", "openclaw");
+    const rebuiltPlan = makeMinimalPlan("my-assistant", "hermes");
🤖 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.test.ts` around lines 369 - 386, The
two plans used in the test are identical which reduces clarity; change how
registryPlan and rebuiltPlan are constructed so they are distinguishable (e.g.,
call makeMinimalPlan with different sandbox names or different agent
identifiers) and keep the rest of the test intact; update the variables
registryPlan and rebuiltPlan (used by readMessagingPlanFromEnv and
getRegistrySandboxMessagingPlan in createDeps) so that when handleSandboxState
is invoked you can clearly assert getSession().messagingPlan equals rebuiltPlan
and that writePlanToEnv was not called.

352-367: 💤 Low value

Consider adding session state assertions for completeness.

Test "restores registry plan to env..." (lines 352-367) verifies that writePlanToEnv is called but doesn't assert that the plan is also persisted to session.messagingPlan. Similarly, test "does not restore plan to env..." (lines 388-402) could verify that session.messagingPlan remains null when no registry entry exists. The runtime code (context snippet 2, line 305) always updates current.messagingPlan, so adding these assertions would strengthen coverage and make the tests more complete.

📋 Suggested additions

For test starting at line 352:

   await handleSandboxState({ ...baseOptions(deps, session), resume: true, sandboxName: "my-assistant" });

   expect(writePlanToEnv).toHaveBeenCalledWith(registryPlan);
+  expect(getSession().messagingPlan).toEqual(registryPlan);
 });

For test starting at line 388:

   await handleSandboxState({ ...baseOptions(deps, session), resume: true, sandboxName: "my-assistant" });

   expect(writePlanToEnv).not.toHaveBeenCalled();
+  expect(getSession().messagingPlan).toBeNull();
 });

Note: You'll need to capture getSession from createDeps in the test at line 388 (similar to line 375).

Also applies to: 388-402

🤖 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.test.ts` around lines 352 - 367, Add
assertions to these tests to verify session.messagingPlan is updated or left
null: after calling handleSandboxState in the "restores registry plan to env on
non-interactive resume..." test assert that the session object passed into
handleSandboxState has its messagingPlan set to registryPlan (in addition to
expecting writePlanToEnv). In the "does not restore plan to env..." test capture
the same session (use the session returned from createDeps/getSession or the
local session variable) and assert session.messagingPlan remains null after
handleSandboxState when readMessagingPlanFromEnv returns null and there is no
registry entry. Reference handleSandboxState, writePlanToEnv,
session.messagingPlan, createDeps/getSession when locating where to add the
assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/lib/onboard/messaging-plan-session.ts`:
- Around line 7-26: The parser parseSandboxMessagingPlan currently only checks
that several properties are arrays/objects but not the shape of their elements;
update the function to validate minimal nested shapes for each collection before
returning the cast: ensure channels entries contain a string channelId and an
array inputs (and that each input has required fields like id/type as
appropriate), disabledChannels entries are strings (or objects with channelId),
credentialBindings entries include string keys and credentialId, agentRender and
buildSteps are arrays of the expected primitive/object shape (e.g.,
renderId/string or object with name/type), stateUpdates entries have required
fields (e.g., key and value types), and healthChecks entries include required
fields (e.g., id/string and enabled boolean); if any element fails its shape
check, return null. Locate and update parseSandboxMessagingPlan to add these
per-item guards (using isObject/type checks and Array.isArray on nested arrays)
so the final return value can safely be cast to SandboxMessagingPlan.

---

Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 6499-6501: This change updates dependencies used by
handleSandboxState in src/lib/onboard.ts (notably readMessagingPlanFromEnv,
writePlanToEnv, and getRegistrySandboxMessagingPlan); before merging, run the
onboarding E2E matrix (cloud-e2e, sandbox-operations-e2e, rebuild-openclaw-e2e,
channels-stop-start-e2e and related onboarding jobs) to validate the new wiring
and ensure no regressions in sandbox onboarding flows.

In `@src/lib/onboard/machine/handlers/sandbox.test.ts`:
- Around line 369-386: The two plans used in the test are identical which
reduces clarity; change how registryPlan and rebuiltPlan are constructed so they
are distinguishable (e.g., call makeMinimalPlan with different sandbox names or
different agent identifiers) and keep the rest of the test intact; update the
variables registryPlan and rebuiltPlan (used by readMessagingPlanFromEnv and
getRegistrySandboxMessagingPlan in createDeps) so that when handleSandboxState
is invoked you can clearly assert getSession().messagingPlan equals rebuiltPlan
and that writePlanToEnv was not called.
- Around line 352-367: Add assertions to these tests to verify
session.messagingPlan is updated or left null: after calling handleSandboxState
in the "restores registry plan to env on non-interactive resume..." test assert
that the session object passed into handleSandboxState has its messagingPlan set
to registryPlan (in addition to expecting writePlanToEnv). In the "does not
restore plan to env..." test capture the same session (use the session returned
from createDeps/getSession or the local session variable) and assert
session.messagingPlan remains null after handleSandboxState when
readMessagingPlanFromEnv returns null and there is no registry entry. Reference
handleSandboxState, writePlanToEnv, session.messagingPlan, createDeps/getSession
when locating where to add the assertions.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 1e37ed83-b2a8-43e8-8ac6-d3b86d8a7dee

📥 Commits

Reviewing files that changed from the base of the PR and between 2d05201 and 673d441.

📒 Files selected for processing (6)
  • src/lib/actions/sandbox/rebuild.ts
  • src/lib/onboard.ts
  • src/lib/onboard/machine/handlers/sandbox.test.ts
  • src/lib/onboard/machine/handlers/sandbox.ts
  • src/lib/onboard/messaging-channel-setup.ts
  • src/lib/onboard/messaging-plan-session.ts

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Selective E2E Results — ✅ All requested jobs passed

Run: 27088189131
Target ref: feat/persist-messaging-plan-in-session
Requested jobs: channels-add-remove-e2e,onboard-resume-e2e,messaging-providers-e2e,channels-stop-start-e2e,rebuild-openclaw-e2e
Summary: 5 passed, 0 failed, 0 skipped

Job Result
channels-add-remove-e2e ✅ success
channels-stop-start-e2e ✅ success
messaging-providers-e2e ✅ success
onboard-resume-e2e ✅ success
rebuild-openclaw-e2e ✅ success

@sandl99 sandl99 requested a review from cv June 7, 2026 09:43
Move messaging policy application into src/lib/messaging/applier/ and
replace channel-list-based preset computation with plan.networkPolicy.presets
throughout the policy selection, resume, and sandbox create flows.

- Add getPolicyPresetsFromPlan helper to messaging-plan-session
- Move pruneDisabledMessagingPolicyPresets to messaging/applier/policy-presets;
  delete merged/computed functions superseded by the compiled plan
- Replace enabledChannels/disabledChannels with messagingPolicyPresets in
  SetupPolicySelectionOptions, SetupPresetSuggestionOptions, and
  preparePolicyPresetResumeSelection; stale-preset detection now uses
  ALL_MESSAGING_POLICY_PRESET_NAMES diff against plan presets
- Remove selectedMessagingChannels, mergePolicyMessagingChannels, and
  getActiveSandbox from handlePoliciesState; handler now accepts a single
  messagingPolicyPresets: string[] derived from the session plan
- Wire getPolicyPresetsFromPlan into onboard.ts handlePoliciesState call and
  prepareInitialSandboxCreatePolicy sandbox create policy
- Update rebuild.ts import to new applier location

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
@sandl99 sandl99 changed the title refactor(onboard): persist messaging plan in session for resume feat(onboard): persist messagingPlan in session and migrate messaging policy to plan-derived presets Jun 7, 2026
@sandl99 sandl99 changed the title feat(onboard): persist messagingPlan in session and migrate messaging policy to plan-derived presets refactor(onboard): persist messagingPlan in session and migrate messaging policy to plan-derived presets Jun 7, 2026
@sandl99 sandl99 changed the title refactor(onboard): persist messagingPlan in session and migrate messaging policy to plan-derived presets refactor(onboard): persist messagingPlan in session for resume Jun 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 7, 2026

Selective E2E Results — ✅ All requested jobs passed

Run: 27091050083
Target ref: feat/persist-messaging-plan-in-session
Requested jobs: channels-add-remove-e2e,onboard-resume-e2e,messaging-providers-e2e,channels-stop-start-e2e,rebuild-openclaw-e2e
Summary: 5 passed, 0 failed, 0 skipped

Job Result
channels-add-remove-e2e ✅ success
channels-stop-start-e2e ✅ success
messaging-providers-e2e ✅ success
onboard-resume-e2e ✅ success
rebuild-openclaw-e2e ✅ success

@cv cv merged commit 633e59b into main Jun 7, 2026
105 of 106 checks passed
@cv cv deleted the feat/persist-messaging-plan-in-session branch June 7, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: messaging Messaging channels, bridges, manifests, or channel lifecycle area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow refactor PR restructures code without intended behavior change v0.0.61 Release target VRDC Issues and PRs submitted by NVIDIA VRDC test team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 2c: persist messaging plan in session for resume

2 participants