Revert "refactor(messaging): persist session registry state as plans"

[sandl99]

Reverts #4945

Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

• New Features

  • Added automatic detection and backfilling of messaging channels for legacy sandbox entries
  • Added messaging channel configuration persistence and reuse across rebuilds
  • Enhanced conflict detection with gateway liveness checks for improved reliability when adding channels

• Bug Fixes

  • Improved handling of channel enable/disable state persistence across sandbox rebuilds
  • Fixed channel configuration loss during onboarding resume flows

• Refactor

  • Simplified messaging channel state storage by using direct field persistence instead of nested plan structures
  • Streamlined plan validation logic to reduce validation complexity

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 2a365832-8a86-4099-870f-4b98642692e1

📥 Commits

Reviewing files that changed from the base of the PR and between 5276e36 and fb06a4c.

📒 Files selected for processing (82)

• ci/test-file-size-budget.json
• src/lib/actions/inference-route-api.test.ts
• src/lib/actions/inference-set.test.ts
• src/lib/actions/sandbox/channel-status.test.ts
• src/lib/actions/sandbox/channel-status.ts
• src/lib/actions/sandbox/doctor.ts
• src/lib/actions/sandbox/policy-channel-conflict.test.ts
• src/lib/actions/sandbox/policy-channel.ts
• src/lib/actions/sandbox/rebuild.ts
• src/lib/inventory/index.test.ts
• src/lib/inventory/index.ts
• src/lib/messaging/applier/conflict-detection-legacy.test.ts
• src/lib/messaging/applier/conflict-detection/backfill.ts
• src/lib/messaging/applier/conflict-detection/entries.ts
• src/lib/messaging/applier/conflict-detection/index.ts
• src/lib/messaging/applier/conflict-detection/probe.ts
• src/lib/messaging/applier/conflict-detection/types.ts
• src/lib/messaging/applier/host-state-applier.test.ts
• src/lib/messaging/applier/host-state-applier.ts
• src/lib/messaging/applier/setup-applier.test.ts
• src/lib/messaging/applier/setup-applier.ts
• src/lib/messaging/compiler/workflow-planner.test.ts
• src/lib/messaging/compiler/workflow-planner.ts
• src/lib/messaging/plan-validation/assertions.ts
• src/lib/messaging/plan-validation/credentials-policy.ts
• src/lib/messaging/plan-validation/envelope-channel.ts
• src/lib/messaging/plan-validation/hooks-health.ts
• src/lib/messaging/plan-validation/index.test.ts
• src/lib/messaging/plan-validation/index.ts
• src/lib/messaging/plan-validation/manifest-matchers.ts
• src/lib/messaging/plan-validation/registered-hooks.ts
• src/lib/messaging/plan-validation/render-build-state.ts
• src/lib/messaging/plan-validation/types.ts
• src/lib/onboard.ts
• src/lib/onboard/channel-state.test.ts
• src/lib/onboard/channel-state.ts
• src/lib/onboard/machine/core-flow-phases.test.ts
• src/lib/onboard/machine/events.ts
• src/lib/onboard/machine/final-flow-phases.test.ts
• src/lib/onboard/machine/handlers/policies.test.ts
• src/lib/onboard/machine/handlers/policies.ts
• src/lib/onboard/machine/handlers/sandbox-messaging-plan.test.ts
• src/lib/onboard/machine/handlers/sandbox.test.ts
• src/lib/onboard/machine/handlers/sandbox.ts
• src/lib/onboard/messaging-config.ts
• src/lib/onboard/messaging-credentials.ts
• src/lib/onboard/messaging-plan-session.test.ts
• src/lib/onboard/messaging-plan-session.ts
• src/lib/onboard/messaging-reuse.test.ts
• src/lib/onboard/messaging-reuse.ts
• src/lib/onboard/session-updates.ts
• src/lib/policy/index.ts
• src/lib/sandbox/whatsapp-diagnostics.ts
• src/lib/state/onboard-session.test.ts
• src/lib/state/onboard-session.ts
• src/lib/state/registry-messaging.ts
• src/lib/state/registry.ts
• src/lib/status-command-deps.ts
• test/channels-add-preset.test.ts
• test/channels-remove-full-teardown.test.ts
• test/cli/connect-recovery.test.ts
• test/cli/doctor-gateway-token.test.ts
• test/cli/list-share-live-inference.test.ts
• test/cli/logs.test.ts
• test/cli/sandbox-mutations.test.ts
• test/cli/sandbox-status-json.test.ts
• test/e2e/docs/parity-inventory.generated.json
• test/e2e/test-channels-add-remove.sh
• test/e2e/test-channels-stop-start.sh
• test/e2e/test-messaging-providers.sh
• test/e2e/test-rebuild-hermes.sh
• test/helpers/messaging-plan-fixtures.ts
• test/helpers/sandbox-handler-fixtures.ts
• test/nemoclaw-start.test.ts
• test/onboard-messaging.test.ts
• test/onboard.test.ts
• test/policies.test.ts
• test/rebuild-credential-preflight.test.ts
• test/rebuild-shields-auto-unlock.test.ts
• test/registry.test.ts
• test/repro-2201.test.ts
• test/secret-redaction.test.ts

---

📝 Walkthrough

Walkthrough

Messaging channel state is now stored and read through flat registry/session fields instead of the older nested messaging plan shape. Onboarding, sandbox actions, status/doctor, conflict detection, and rebuild flows were updated to use those fields, and related tests and fixtures were rewritten to match.

Changes

Messaging state and registry shape

Layer / File(s)	Summary
Session and registry contracts src/lib/state/onboard-session.ts, src/lib/state/registry.ts, src/lib/onboard/session-updates.ts, src/lib/onboard/messaging-config.ts, src/lib/onboard/messaging-reuse.ts, src/lib/onboard/messaging-plan-session.ts, src/lib/onboard/channel-state.ts, src/lib/inventory/index.ts, src/lib/sandbox/whatsapp-diagnostics.ts	Session, registry, and helper code now store and normalize messagingChannels, messagingChannelConfig, and disabledChannels directly.
Onboarding and resume wiring src/lib/onboard.ts, src/lib/onboard/machine/..., src/lib/onboard/messaging-credentials.ts, src/lib/onboard/channel-state.test.ts, src/lib/onboard/messaging-reuse.test.ts, src/lib/onboard/machine/*, src/lib/onboard/messaging-config.ts	Onboarding and resume paths now persist, restore, and pass through the flat messaging fields, including session updates and machine context.
Sandbox actions and status src/lib/actions/sandbox/..., src/lib/inventory/index.ts, src/lib/status-command-deps.ts, src/lib/actions/inference-*.test.ts	Channel add/remove/status/doctor/rebuild logic now reads registry messaging fields directly, including backfill, conflict checks, rollback, and overlap reporting.
Conflict detection and applier internals src/lib/messaging/applier/..., src/lib/messaging/compiler/workflow-planner.ts, src/lib/messaging/plan-validation/*	Messaging conflict detection, workflow planning, setup applier validation, and related plan-validation modules were updated around the new messaging field model and removed validation paths.
Test and fixture updates test/*, test/e2e/*, ci/test-file-size-budget.json	Tests, E2E scripts, and one size-budget entry were updated to match the new registry/session shapes and revised assertions.

Sequence Diagram(s)

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90+ minutes

Possibly related PRs

• NVIDIA/NemoClaw#4050: Refactors the same messaging channel state model and related onboarding/status plumbing that this PR continues to update.
• NVIDIA/NemoClaw#4908: Touches the same conflict-detection and messaging applier flow that this PR expands with backfill and probe support.
• NVIDIA/NemoClaw#4945: Modifies the same channel status and policy-related code paths, with overlapping messaging-channel state handling.

Suggested labels

refactor, area: messaging, area: providers

Suggested reviewers

• cv
• ericksoa

Poem

A bunny hops through state anew,
With channels flat and configs true.
🐰 Carrots of logic, crisp and bright,
The registry hums both day and night.
Hop, hop — the old plan fades from view.

✨ 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 revert-4945-feat/messaging-plan-session-registry

[github-actions]

E2E Advisor Recommendation

Required E2E: cloud-e2e, messaging-providers-e2e, channels-add-remove-e2e, channels-stop-start-e2e, rebuild-openclaw-e2e, rebuild-hermes-e2e, diagnostics-e2e, network-policy-e2e, inference-routing-e2e
Optional E2E: openclaw-inference-switch-e2e, token-rotation-e2e, state-backup-restore-e2e, credential-migration-e2e

Dispatch hint: cloud-e2e,messaging-providers-e2e,channels-add-remove-e2e,channels-stop-start-e2e,rebuild-openclaw-e2e,rebuild-hermes-e2e,diagnostics-e2e,network-policy-e2e,inference-routing-e2e

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

• cloud-e2e (high): Broad install → onboard → sandbox verify → live inference smoke is required because core onboarding, session/registry persistence, policy, and sandbox lifecycle code changed.
• messaging-providers-e2e (high): Validates provider/placeholder/L7 credential flow and QR-only WhatsApp path touched by messaging credentials, setup applier, plan validation, policy-channel, and the modified E2E script.
• channels-add-remove-e2e (high): Directly covers changed channel add/remove persistence, registry messagingChannels fields, gateway provider reuse, rebuild-after-add/remove, and the touched test script.
• channels-stop-start-e2e (high): Directly covers changed disabledChannels handling, paused channel reporting, stop/start lifecycle, multi-channel planner/applier behavior, and the touched test script.
• rebuild-openclaw-e2e (medium): OpenClaw rebuild must be exercised because rebuild logic and messaging registry state are changed and deferred channel mutations are now expected to survive through rebuild.
• rebuild-hermes-e2e (medium): Hermes rebuild coverage is required because rebuild.ts and the Hermes rebuild E2E script changed, and messaging/policy state is shared across agent-specific rebuild flows.
• diagnostics-e2e (medium): Doctor/status/channel diagnostics changed to consume flat registry channel fields and to alter WhatsApp paused/deep-probe behavior; validate live operator diagnostics.
• network-policy-e2e (medium): Policy-channel and plan-validation changes can affect network policy application for messaging presets and security boundaries; run the real policy enforcement E2E.
• inference-routing-e2e (medium): Session schema and onboarding state changes are adjacent to persisted inference route selection; run routing/credential-isolation E2E to ensure no regression in real inference routing.

Optional E2E

• openclaw-inference-switch-e2e (medium): Useful extra confidence for inference set/session persistence after session shape changes, but less directly touched than onboarding and messaging channel lifecycle.
• token-rotation-e2e (high): Useful confidence for messaging credential reuse/rotation paths changed by messaging-credentials, messaging-reuse, provider setup, and conflict detection updates.
• state-backup-restore-e2e (medium): Registry/session schema changes make backup/restore a useful non-blocking compatibility check for persisted messagingChannels, disabledChannels, and messagingChannelConfig fields.
• credential-migration-e2e (medium): Optional security confidence because credential storage and redaction-adjacent messaging paths changed, though the main legacy credential migration implementation is not the core diff.

New E2E recommendations

• messaging-conflict-detection-backfill (high): The PR adds/changes gateway-backed backfill before cross-sandbox conflict detection, but existing E2E jobs do not appear to simulate legacy registry entries with absent messagingChannels and a live/down gateway to prove add-channel conflict behavior is correct end to end.
  • Suggested test: Add an E2E that seeds two sandbox registry entries, one legacy without messagingChannels, runs channels add telegram with a staged token, and verifies matching-token conflicts abort non-interactively while gateway probe/backfill failures are non-fatal and never print secrets.
• legacy-messaging-plan-upgrade (high): The diff moves live commands from entry.messaging.plan helpers to flat messagingChannels, disabledChannels, and messagingChannelConfig; existing E2E coverage primarily exercises freshly-created state.
  • Suggested test: Add an upgrade-style E2E that starts from a fixture or old install containing only messaging.plan, then runs doctor, channels status, channels stop/start, and rebuild to verify compatibility/migration and no channel loss.
• whatsapp-paused-diagnostics (medium): Unit tests cover paused WhatsApp skipping the deep probe, but a real sandbox E2E would catch command wiring, registry persistence, and shell/probe regressions together.
  • Suggested test: Extend the channels stop/start or diagnostics E2E with a WhatsApp paused-state assertion that channels status --channel whatsapp reports paused/registered without running the deep probe and returns the intended exit behavior.

Dispatch hint

• Workflow: .github/workflows/nightly-e2e.yaml
• jobs input: cloud-e2e,messaging-providers-e2e,channels-add-remove-e2e,channels-stop-start-e2e,rebuild-openclaw-e2e,rebuild-hermes-e2e,diagnostics-e2e,network-policy-e2e,inference-routing-e2e

[github-actions]

E2E Scenario Advisor Recommendation

Required scenario E2E: ubuntu-repo-cloud-openclaw, ubuntu-repo-cloud-openclaw-telegram, ubuntu-repo-cloud-openclaw-slack, ubuntu-rebuild-openclaw, ubuntu-repo-cloud-openclaw-token-rotation
Optional scenario E2E: ubuntu-repo-cloud-openclaw-discord, ubuntu-repo-cloud-hermes-slack, ubuntu-repo-cloud-hermes, ubuntu-repo-openai-compatible-openclaw

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-telegram
• gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-slack
• gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-rebuild-openclaw
• gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-token-rotation

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

• ubuntu-repo-cloud-openclaw: Baseline Ubuntu repo OpenClaw onboarding covers the core onboarding/session/registry changes plus smoke, cloud inference routing, and credential-state assertions affected by the changed src/lib/onboard, src/lib/state, src/lib/messaging, and sandbox doctor/status code.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw
• ubuntu-repo-cloud-openclaw-telegram: Exercises the primary OpenClaw messaging onboarding path and common messaging provider/placeholder/secret-leak/bridge checks after changes to messaging channel registry state, workflow planning, host-state appliers, conflict detection, and channel status/doctor logic.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-telegram
• ubuntu-repo-cloud-openclaw-slack: Covers an adjacent OpenClaw messaging channel with Slack-specific provider state and multi-credential behavior, which is a distinct surface touched by the channel conflict/backfill and messaging credential/config changes.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-slack
• ubuntu-rebuild-openclaw: Directly targets the rebuild lifecycle touched by src/lib/actions/sandbox/rebuild.ts and the messaging/registry state persistence changes that must survive rebuild.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-rebuild-openclaw
• ubuntu-repo-cloud-openclaw-token-rotation: Targets provider credential rotation/isolation behavior, which is relevant to the changed messaging credential handling, registry backfill, and conflict-detection code.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-token-rotation

Optional scenario E2E

• ubuntu-repo-cloud-openclaw-discord: Adjacent OpenClaw messaging channel coverage for the same generic messaging registry/planner/applier changes; useful if secrets and CI capacity allow, but Telegram and Slack are the primary targets.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-openclaw-discord
• ubuntu-repo-cloud-hermes-slack: Exercises the same Slack messaging surface on Hermes instead of OpenClaw, providing agent-variant coverage for generic messaging and registry changes.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes-slack
• ubuntu-repo-cloud-hermes: Agent-variant baseline for onboarding/session/registry changes; optional because the primary changed surfaces are covered by Ubuntu OpenClaw and messaging-targeted scenarios.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-cloud-hermes
• ubuntu-repo-openai-compatible-openclaw: Adjacent provider-path coverage for inference routing/provider compatibility changes; optional because the required OpenClaw baseline already covers the main cloud inference route.
  • Dispatch: gh workflow run e2e-scenarios.yaml --ref <pr-head-ref> --field scenarios=ubuntu-repo-openai-compatible-openclaw

Relevant changed files

• src/lib/actions/sandbox/channel-status.ts
• src/lib/actions/sandbox/doctor.ts
• src/lib/actions/sandbox/policy-channel.ts
• src/lib/actions/sandbox/rebuild.ts
• src/lib/inventory/index.ts
• src/lib/messaging/applier/conflict-detection/backfill.ts
• src/lib/messaging/applier/conflict-detection/entries.ts
• src/lib/messaging/applier/conflict-detection/index.ts
• src/lib/messaging/applier/conflict-detection/probe.ts
• src/lib/messaging/applier/conflict-detection/types.ts
• src/lib/messaging/applier/host-state-applier.ts
• src/lib/messaging/applier/setup-applier.ts
• src/lib/messaging/compiler/workflow-planner.ts
• src/lib/messaging/plan-validation/assertions.ts
• src/lib/messaging/plan-validation/credentials-policy.ts
• src/lib/messaging/plan-validation/envelope-channel.ts
• src/lib/messaging/plan-validation/hooks-health.ts
• src/lib/messaging/plan-validation/index.ts
• src/lib/messaging/plan-validation/manifest-matchers.ts
• src/lib/messaging/plan-validation/registered-hooks.ts
• src/lib/messaging/plan-validation/render-build-state.ts
• src/lib/messaging/plan-validation/types.ts
• src/lib/onboard.ts
• src/lib/onboard/channel-state.ts
• src/lib/onboard/machine/events.ts
• src/lib/onboard/machine/handlers/policies.ts
• src/lib/onboard/machine/handlers/sandbox.ts
• src/lib/onboard/messaging-config.ts
• src/lib/onboard/messaging-credentials.ts
• src/lib/onboard/messaging-plan-session.ts
• src/lib/onboard/messaging-reuse.ts
• src/lib/onboard/session-updates.ts
• src/lib/policy/index.ts
• src/lib/sandbox/whatsapp-diagnostics.ts
• src/lib/state/onboard-session.ts
• src/lib/state/registry-messaging.ts
• src/lib/state/registry.ts
• src/lib/status-command-deps.ts

[github-actions]

PR Review Advisor

Findings: 3 needs attention, 7 worth checking, 0 nice ideas
Top item: Channel add/remove can leave gateway, policy, and registry state out of sync

Review findings

🛠️ Needs attention

• Channel mutations ignore durable-state write failures after gateway/policy side effects (src/lib/actions/sandbox/policy-channel.ts:517): The add/remove paths now mutate OpenShell providers and policies, then ignore the boolean result from registry and plan persistence. For example, `applyChannelAddToGatewayAndRegistry()` upserts providers and calls `registry.updateSandbox(...)` without checking the result; the add path later ignores `MessagingHostStateApplier.applyPlanToRegistry(...)`. Remove similarly drops providers, prints success, then performs best-effort policy/plan persistence. If a registry lock/write fails or the sandbox disappears, NemoClaw can leave a live bridge/provider or widened policy with local state saying the channel is absent, weakening rebuild/status/doctor and credential-conflict detection.
  • Recommendation: Make channel add/remove fail closed at the mutation boundary: check every registry/plan persistence return, roll back earlier OpenShell/policy side effects where possible, and do not print success or prompt rebuild until durable state is consistent. Restore negative tests for registry/plan persistence failures on token-backed add, QR add, and remove.
  • Evidence: `policy-channel.ts` calls `registry.updateSandbox(sandboxName, { messagingChannels: ... })` without checking the return; add paths call `MessagingHostStateApplier.applyPlanToRegistry(sandboxName, plan)` without checking; the previous persistence-failure tests were removed from `policy-channel-conflict.test.ts`.
• Partial revert leaves messaging plan and flat registry fields as competing sources of truth (src/lib/actions/sandbox/rebuild.ts:834): The PR claims to revert plan-backed session/registry state, but the code still writes and consumes both flat fields (`messagingChannels`, `disabledChannels`, `messagingChannelConfig`) and plan fields (`Session.messagingPlan`, `SandboxEntry.messaging`, staged env plans). Existing rows created by refactor(messaging): persist session registry state as plans #4945 can contain only `messaging.plan`, while doctor/status/remove/rebuild now often read only flat fields. This makes plan-only channels invisible unless a later conflict-check backfill happens, and it creates unresolved precedence when flat fields and plan state disagree.
  • Recommendation: Choose and document one authoritative messaging state source for this revert. Either migrate/backfill plan-only rows before all lifecycle/status paths read them, or keep plan validation/plan-derived helpers until flat fields are guaranteed present. Add tests for doctor/status/remove/rebuild on a plan-only registry row created by refactor(messaging): persist session registry state as plans #4945 and for disagreement between flat fields and `messaging.plan`.
  • Evidence: The diff deletes `src/lib/messaging/plan-validation/*`, rewrites status/doctor to read `entry.messagingChannels`, but `rebuild.ts` still sets `s.messagingPlan = rebuildMessagingPlan` and `onboard.ts` still registers `messaging: messagingState`.
• The revert grows several existing monolith hotspots (src/lib/onboard.ts:1): This PR is already a broad revert across 82 files and it increases multiple large lifecycle files instead of extracting new compatibility logic. The largest hotspot growth is in onboarding, rebuild, policy-channel, registry, and state/session tests, which are already high-risk surfaces for sandbox lifecycle, credentials, and policy behavior.
  • Recommendation: Offset the monolith growth before merge by extracting the revert compatibility/reconciliation logic into focused helpers with targeted tests, or reduce the patch to the minimum required revert. Keep lifecycle and policy mutation files from absorbing more fallback behavior.
  • Evidence: Deterministic drift analysis flagged blocker-level monolith growth in `src/lib/onboard/machine/handlers/sandbox.test.ts` (+245), `src/lib/state/onboard-session.ts` (+39), `src/lib/actions/sandbox/rebuild.ts` (+35), `src/lib/actions/sandbox/policy-channel.ts` (+28), `src/lib/state/registry.ts` (+22), and `src/lib/onboard.ts` (+20).

🔎 Worth checking

• Source-of-truth review needed: messaging plan versus flat registry/session fields: 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: `rebuild.ts` writes both `s.messagingChannels` and `s.messagingPlan`; `onboard.ts` registers `messaging: messagingState`; status/doctor paths now read flat fields directly.
• Source-of-truth review needed: legacy channel backfill in conflict detection: 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: `src/lib/messaging/applier/conflict-detection/backfill.ts` probes provider suffixes and writes `messagingChannels`; it is invoked only from `checkChannelAddConflict()`.
• Source-of-truth review needed: swallowed backfill/probe failures before channel add: 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: `policy-channel.ts` catches and ignores `backfillMessagingChannels(...)` errors before running `findChannelConflicts(...)`.
• Source-of-truth review needed: best-effort messaging channel config session persistence: 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: `policy-channel.ts` catches `onboardSession.updateSession(...)` and comments `Best-effort: registry state still carries the config when available`, while `registry.updateSandbox(...)` is unchecked.
• Legacy channel backfill failures are swallowed before conflict detection (src/lib/actions/sandbox/policy-channel.ts:418): The new `backfillMessagingChannels(...)` compatibility path is wrapped in an empty catch and the tests explicitly assert that a probe/backfill failure with no pre-recorded conflict lets `channels add` proceed. For single-consumer credentials such as Telegram polling or Slack Socket Mode, a transient gateway/probe/update failure can therefore hide an existing legacy sandbox and allow a second sandbox to claim the same credential without `--force`.
  • Recommendation: Treat backfill/probe failures as an incomplete conflict check and fail closed unless the operator explicitly uses `--force`, matching the existing behavior for `findChannelConflicts` exceptions. At minimum, surface a warning that verification was incomplete and add a non-interactive test that aborts when the only possible conflict source cannot be verified.
  • Evidence: `checkChannelAddConflict()` catches `backfillMessagingChannels(registry, makeChannelsConflictProbe())` and ignores the error; `policy-channel-conflict.test.ts` contains `probe + backfill failure with no pre-recorded conflict lets the add proceed`.
• Removing plan validation weakens the persisted messaging blueprint boundary (src/lib/messaging/plan-validation/index.ts:1): The PR deletes the plan-validation module that checked assertions, credentials policy, envelope/channel matching, registered hooks, manifest matchers, and rendered build state. Remaining plan decoding is mostly shallow shape validation. Because serialized plans can outlive the manifest/hook code that produced them and are later used for sandbox config, policy, hooks, and provider setup, removing validation increases blueprint tampering and drift risk.
  • Recommendation: Keep or replace the validation checks until plan state is no longer read from registry/session/env. If the revert intends to stop trusting persisted plans, remove all plan consumers or gate them behind validation and add tamper/invalid-plan tests.
  • Evidence: All files under `src/lib/messaging/plan-validation/` are deleted, while `MessagingSetupApplier.readPlanFromEnv()`, `MessagingHostStateApplier`, and rebuild/onboard still consume serialized plans.
• channels stop/start can write disabled state for a channel that is not configured (src/lib/actions/sandbox/policy-channel.ts:1476): The stop/start implementation no longer validates that the requested channel is present in the sandbox messaging plan or flat configured-channel list. It only checks that the sandbox exists and the channel name is known, then writes `disabledChannels`. A never-configured channel can be marked disabled, which pollutes registry state and can confuse status/rebuild behavior.
  • Recommendation: Before mutating `disabledChannels`, verify that the channel is configured in the authoritative channel source. If compatibility requires supporting legacy rows, derive membership from both flat fields and valid plan state. Add a negative test for stopping a known but unconfigured channel.
  • Evidence: `sandboxChannelsSetEnabled()` removed the prior `readValidatedSandboxMessagingChannelPlan(...)` guard and now calls `registry.setChannelDisabled(sandboxName, normalized, disabled)` after only `registry.getSandbox(sandboxName)` and `getChannelDef(channelArg)` checks.

🌱 Nice ideas

• None.

Consider writing more tests for

• **Runtime validation** — channels add aborts or rolls back when registry.updateSandbox fails after OpenShell provider upsert, and does not print Registered or Change queued. The diff changes OpenShell provider mutation, registry/session persistence, policy preset application, rebuild/resume, doctor/status, and legacy backfill behavior. Unit tests are heavily mocked and several prior negative persistence-failure tests were removed, so runtime validation is needed to prove host/gateway/sandbox state remains aligned.
• **Runtime validation** — in-sandbox QR channels add exits and un-applies the channel preset when MessagingHostStateApplier.applyPlanToRegistry returns false. The diff changes OpenShell provider mutation, registry/session persistence, policy preset application, rebuild/resume, doctor/status, and legacy backfill behavior. Unit tests are heavily mocked and several prior negative persistence-failure tests were removed, so runtime validation is needed to prove host/gateway/sandbox state remains aligned.
• **Runtime validation** — token-backed channels add rolls back provider and policy side effects when MessagingHostStateApplier.applyPlanToRegistry returns false. The diff changes OpenShell provider mutation, registry/session persistence, policy preset application, rebuild/resume, doctor/status, and legacy backfill behavior. Unit tests are heavily mocked and several prior negative persistence-failure tests were removed, so runtime validation is needed to prove host/gateway/sandbox state remains aligned.
• **Runtime validation** — channels remove does not print success or prompt rebuild when registry removal persistence fails. The diff changes OpenShell provider mutation, registry/session persistence, policy preset application, rebuild/resume, doctor/status, and legacy backfill behavior. Unit tests are heavily mocked and several prior negative persistence-failure tests were removed, so runtime validation is needed to prove host/gateway/sandbox state remains aligned.
• **Runtime validation** — channels stop rejects a known channel that is not currently configured for the sandbox. The diff changes OpenShell provider mutation, registry/session persistence, policy preset application, rebuild/resume, doctor/status, and legacy backfill behavior. Unit tests are heavily mocked and several prior negative persistence-failure tests were removed, so runtime validation is needed to prove host/gateway/sandbox state remains aligned.
• **Acceptance clause:** Reverts refactor(messaging): persist session registry state as plans #4945 — add test evidence or identify existing coverage. The diff removes the plan-validation module and restores many flat messaging fields, but it still retains and consumes `Session.messagingPlan`, `SandboxEntry.messaging`, `MessagingWorkflowPlanner`, `MessagingHostStateApplier`, and env-staged plans. The result is a partial revert with mixed source-of-truth behavior rather than a clean return to pre-refactor(messaging): persist session registry state as plans #4945 state.
• **messaging plan versus flat registry/session fields** — Missing: plan-only registry rows should be exercised through doctor/status/remove/rebuild, and flat-versus-plan disagreement should have deterministic precedence tests.. `rebuild.ts` writes both `s.messagingChannels` and `s.messagingPlan`; `onboard.ts` registers `messaging: messagingState`; status/doctor paths now read flat fields directly.
• **legacy channel backfill in conflict detection** — Partial: tests cover unknown-token legacy entries and thrown backfill writes, but not all lifecycle consumers.. `src/lib/messaging/applier/conflict-detection/backfill.ts` probes provider suffixes and writes `messagingChannels`; it is invoked only from `checkChannelAddConflict()`.

Workflow run details

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