refactor(messaging): use plan-backed channel state#5123
Open
sandl99 wants to merge 9 commits into
Open
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMigrates messaging state to structured SandboxMessagingPlan: adds plan validation and registry messaging helpers, converts session/registry schemas and channel flows to plan-backed logic, removes legacy backfill/probe, and updates tests and e2e fixtures to assert messaging.plan usage. ChangesPlan Validation Foundation
Session State Migration
Registry Messaging Architecture
Conflict Detection Simplification
Channel Add/Remove Mutations
Sandbox Lifecycle
Onboard Messaging Integration
Inventory and Status
Channel Status and Doctor
Messaging Host State
Diagnostics
Unit and Integration Test Fixtures
E2E Test Infrastructure
Test Size Budgets
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
✨ Finishing Touches🧪 Generate unit tests (beta)
|
sandl99
added
area: messaging
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 5 needs attention, 10 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/policy-channel.ts (1)
970-977:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce consistent rollback on messaging plan persistence failure.
Line 976 ignores
applyPlanToRegistryfailure, and Lines 1027-1033 exit after side effects have already run. That can leave gateway/policy state changed without durablemessaging.planstate.Suggested fix
- MessagingHostStateApplier.applyPlanToRegistry(sandboxName, plan); + if (!MessagingHostStateApplier.applyPlanToRegistry(sandboxName, plan)) { + removeChannelPresetIfPresent(sandboxName, canonical); + console.error(` ${YW}⚠${R} Could not persist messaging plan for '${sandboxName}'.`); + process.exit(1); + } @@ if (!MessagingHostStateApplier.applyPlanToRegistry(sandboxName, plan)) { + removeChannelPresetIfPresent(sandboxName, canonical); + await rollbackChannelAdd(sandboxName, channelDef, canonical, { + wasAlreadyEnabled, + priorCreds, + }); console.error(` ${YW}⚠${R} Could not persist messaging plan for '${sandboxName}'.`); console.error( " Earlier gateway or policy side effects may already have run, but durable channel state was not saved.", ); process.exit(1); }Also applies to: 1027-1033
🤖 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/actions/sandbox/policy-channel.ts` around lines 970 - 977, The code persists manifest/gateway changes before calling MessagingHostStateApplier.applyPlanToRegistry and currently ignores failures; update the flow so that when applyPlanToRegistry(sandboxName, plan) throws or rejects (both at the shown block and the similar 1027-1033 block) you perform a rollback: undo applyChannelAddToGatewayAndRegistry side effects and revert persistManifestAddState(sandboxName, manifest) (or mark it failed) and then exit with non-zero; locate usages around applyChannelPresetIfAvailable, applyChannelAddToGatewayAndRegistry, persistManifestAddState, and MessagingHostStateApplier.applyPlanToRegistry and wrap the applyPlanToRegistry call in a try/catch that triggers the rollback steps on error and re-throws/exits with a logged error.
🧹 Nitpick comments (2)
src/lib/state/registry-messaging.ts (1)
31-37: 💤 Low valueConsider adding a clarifying comment for the undefined return.
The function returns
undefined(notnull) for invalid input, which is intentional to allow optional field omission when spreading into registry entries. A brief comment explaining this choice would help future maintainers.💡 Suggested comment
+// Returns undefined (not null) for invalid input so spreading the result into +// a registry entry will omit the field rather than set it to null. export function cloneSandboxMessagingState( messaging: SandboxMessagingState | null | undefined, ): SandboxMessagingState | undefined {🤖 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/state/registry-messaging.ts` around lines 31 - 37, Add a brief clarifying comment inside the cloneSandboxMessagingState function explaining that the function intentionally returns undefined (not null) for invalid or absent input so callers can omit the optional field when spreading into registry entries; reference the behavior around the null/undefined check and the final return so future maintainers understand why undefined is used and not null.src/lib/messaging/compiler/workflow-planner.ts (1)
235-240: 💤 Low valueConsider removing the unused
_sandboxEntryparameter.The function
disabledChannelsFromSandboxEntryno longer reads from thesandboxEntryparameter after migrating to plan-only state. While the underscore prefix correctly marks it as unused per coding guidelines, the parameter could be removed entirely to simplify the signature. The only call site (line 104) already has access toexistingPlanand doesn't need the extra parameter.♻️ Proposed simplification
-function disabledChannelsFromSandboxEntry( - _sandboxEntry: MessagingWorkflowPlannerSandboxEntry | null | undefined, - fallbackPlan: SandboxMessagingPlan | null, -): MessagingChannelId[] { +function disabledChannelsFromFallbackPlan( + fallbackPlan: SandboxMessagingPlan | null, +): MessagingChannelId[] { return uniqueChannels(fallbackPlan?.disabledChannels ?? []); }And update the call site:
return setPlanDisabledChannels( existingPlan, - disabledChannelsFromSandboxEntry(context.sandboxEntry, existingPlan), + disabledChannelsFromFallbackPlan(existingPlan), "rebuild", );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/messaging/compiler/workflow-planner.ts` around lines 235 - 240, The function disabledChannelsFromSandboxEntry still accepts an unused _sandboxEntry parameter; remove that parameter from the signature and update its usages so it only takes (fallbackPlan: SandboxMessagingPlan | null) and returns uniqueChannels(fallbackPlan?.disabledChannels ?? []); update the call site(s) that currently pass a sandbox entry (e.g., the caller that has existingPlan) to pass only the fallback plan (existingPlan) and adjust any type annotations/imports accordingly to keep types consistent.
🤖 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 5409-5412: The call to
getRecordedMessagingChannelsForResumeFromState is seeding from only active
channels via getActiveChannelsFromPlan(session?.messagingPlan), which drops
configured-but-stopped channels and loses persisted stop state; change the
argument to use the plan's configured channels (e.g., pass the configured
channels list from session?.messagingPlan or a new helper like
getConfiguredChannelsFromPlan(session?.messagingPlan)) so
configured-but-disabled entries are included, and ensure the disabled/started
state remains tracked separately (do not derive enabled/disabled by filtering
here). Update any related call sites or helpers so
getRecordedMessagingChannelsForResumeFromState receives the full configured
channels list while the resume logic still respects each channel's disabled
flag.
In `@src/lib/onboard/channel-state.ts`:
- Around line 20-28: The env-plan reader passed via deps is never used by
default callers, so resolveDisabledChannels (the function containing
getDisabledChannelsFromPlan, readMessagingPlanFromEnv,
onboardSession.loadSession, and registry.getDisabledChannels) should default to
the real env reader instead of only using deps.readMessagingPlanFromEnv when
provided; fix by importing the actual readMessagingPlanFromEnv implementation
into channel-state.ts and use it when deps?.readMessagingPlanFromEnv is
undefined (i.e., set const envPlan = (deps?.readMessagingPlanFromEnv ??
readMessagingPlanFromEnv)()), so callers that do not pass deps will observe the
in-flight env plan rather than falling back to stale session/registry state.
In `@test/e2e/docs/parity-inventory.generated.json`:
- Around line 7986-8002: The generated JSON parity-inventory.generated.json is
missing SPDX metadata; either add SPDX fields to the JSON root (e.g., top-level
"SPDX-License-Identifier" and "SPDX-FileCopyrightText" properties with the
correct license/copyright text) so the file contains the required license
metadata, or update the repository's licensing exceptions/config to explicitly
exclude/generated JSON artifacts; locate parity-inventory.generated.json and
either insert those top-level SPDX properties or register the file pattern as an
allowed generated artifact.
In `@test/e2e/test-channels-add-remove.sh`:
- Around line 178-183: The test currently requires a telegram entry in
messaging.plan.channels (variables plan and channel) even when asserting a
removed state; update the assertion logic in test-channels-add-remove.sh so that
when testing post-remove you assert that no channel with channelId ===
"telegram" exists in plan.channels (i.e., channel === undefined/null) instead of
failing if it's missing, and ensure any removed-state checks rely on plan as the
single source of truth rather than expecting a removed marker entry; locate the
block that computes const plan = entry.messaging?.plan and the channel lookup
and change the expectation to assert absence of the telegram channel.
In `@test/e2e/test-rebuild-hermes.sh`:
- Line 311: The test fixture seeds the session by setting sess['messagingPlan']
= plan but leaves legacy keys (messagingChannels, messagingChannelConfig,
disabledChannels) from the loaded onboard-session.json, which can mask failures;
update the seeding code that sets sess (the sess object where messagingPlan is
assigned) to explicitly remove/delete those legacy keys before or after
assigning sess['messagingPlan'] so the session only contains messagingPlan
(e.g., delete sess['messagingChannels'], delete sess['messagingChannelConfig'],
delete sess['disabledChannels']).
---
Outside diff comments:
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 970-977: The code persists manifest/gateway changes before calling
MessagingHostStateApplier.applyPlanToRegistry and currently ignores failures;
update the flow so that when applyPlanToRegistry(sandboxName, plan) throws or
rejects (both at the shown block and the similar 1027-1033 block) you perform a
rollback: undo applyChannelAddToGatewayAndRegistry side effects and revert
persistManifestAddState(sandboxName, manifest) (or mark it failed) and then exit
with non-zero; locate usages around applyChannelPresetIfAvailable,
applyChannelAddToGatewayAndRegistry, persistManifestAddState, and
MessagingHostStateApplier.applyPlanToRegistry and wrap the applyPlanToRegistry
call in a try/catch that triggers the rollback steps on error and
re-throws/exits with a logged error.
---
Nitpick comments:
In `@src/lib/messaging/compiler/workflow-planner.ts`:
- Around line 235-240: The function disabledChannelsFromSandboxEntry still
accepts an unused _sandboxEntry parameter; remove that parameter from the
signature and update its usages so it only takes (fallbackPlan:
SandboxMessagingPlan | null) and returns
uniqueChannels(fallbackPlan?.disabledChannels ?? []); update the call site(s)
that currently pass a sandbox entry (e.g., the caller that has existingPlan) to
pass only the fallback plan (existingPlan) and adjust any type
annotations/imports accordingly to keep types consistent.
In `@src/lib/state/registry-messaging.ts`:
- Around line 31-37: Add a brief clarifying comment inside the
cloneSandboxMessagingState function explaining that the function intentionally
returns undefined (not null) for invalid or absent input so callers can omit the
optional field when spreading into registry entries; reference the behavior
around the null/undefined check and the final return so future maintainers
understand why undefined is used and not null.
🪄 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: cf133ce8-7327-4f24-a9dc-7abc3a590c41
📒 Files selected for processing (60)
ci/test-file-size-budget.jsonsrc/lib/actions/inference-route-api.test.tssrc/lib/actions/inference-set.test.tssrc/lib/actions/sandbox/channel-status.test.tssrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging/applier/conflict-detection-legacy.test.tssrc/lib/messaging/applier/conflict-detection/backfill.tssrc/lib/messaging/applier/conflict-detection/entries.tssrc/lib/messaging/applier/conflict-detection/index.tssrc/lib/messaging/applier/conflict-detection/probe.tssrc/lib/messaging/applier/conflict-detection/types.tssrc/lib/messaging/applier/host-state-applier.test.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/onboard.tssrc/lib/onboard/channel-state.test.tssrc/lib/onboard/channel-state.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/events.tssrc/lib/onboard/machine/final-flow-phases.test.tssrc/lib/onboard/machine/handlers/policies.test.tssrc/lib/onboard/machine/handlers/policies.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/messaging-config.tssrc/lib/onboard/messaging-credentials.tssrc/lib/onboard/messaging-plan-session.tssrc/lib/onboard/messaging-reuse.test.tssrc/lib/onboard/messaging-reuse.tssrc/lib/onboard/session-updates.tssrc/lib/sandbox/whatsapp-diagnostics.tssrc/lib/state/onboard-session.test.tssrc/lib/state/onboard-session.tssrc/lib/state/registry-messaging.tssrc/lib/state/registry.tssrc/lib/status-command-deps.tstest/channels-add-preset.test.tstest/channels-remove-full-teardown.test.tstest/cli/logs.test.tstest/cli/status-root-json.test.tstest/e2e/docs/parity-inventory.generated.jsontest/e2e/test-channels-add-remove.shtest/e2e/test-channels-stop-start.shtest/e2e/test-messaging-providers.shtest/e2e/test-rebuild-hermes.shtest/onboard-messaging.test.tstest/onboard.test.tstest/rebuild-credential-preflight.test.tstest/rebuild-shields-auto-unlock.test.tstest/rebuild-stale-recovery.test.tstest/registry.test.tstest/repro-2201.test.ts
💤 Files with no reviewable changes (10)
- src/lib/messaging/applier/conflict-detection-legacy.test.ts
- test/onboard.test.ts
- src/lib/messaging/applier/conflict-detection/probe.ts
- src/lib/onboard/machine/core-flow-phases.test.ts
- src/lib/messaging/compiler/workflow-planner.test.ts
- src/lib/onboard/session-updates.ts
- src/lib/messaging/applier/conflict-detection/index.ts
- src/lib/messaging/applier/conflict-detection/backfill.ts
- src/lib/messaging/applier/conflict-detection/types.ts
- src/lib/messaging/applier/host-state-applier.test.ts
Contributor
Author
|
Responding to the PR Review Advisor “Needs attention” items: I am intentionally not taking additional action on these three advisory items in this migration PR.
This is valid hardening, but it is not required for migration parity. The pre-migration channel lifecycle was not fully atomic either: add/remove paths could already mutate OpenShell providers, policy, credentials, or session state without a full rollback contract. Adding full rollback now would introduce new lifecycle semantics across provider upsert/delete, policy apply/remove, token/session changes, QR state, and registry writes. That is security-sensitive behavior and should be designed/tested as a separate hardening follow-up rather than folded into this source-of-truth migration.
This is also valid validation hardening, but not a blocker for this PR. The migration intentionally makes I agree a follow-up should make registry readers distinguish “absent plan” from “present but malformed plan,” especially for conflict detection and status/doctor diagnostics. For this PR, the acceptance bar is preserving supported normal flows with plan-backed state.
I am not splitting the large test files in this migration PR. The repository’s enforced guardrail is the test-size budget check, and the current branch passes For this PR, I am keeping the scope focused on replacing legacy persisted messaging fields with |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27262121242
|
…messaging-state # Conflicts: # ci/test-file-size-budget.json
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27264726061
|
sandl99
added
v0.0.62
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27266759162
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27267840227
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27271049670
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27321293497
|
…messaging-state # Conflicts: # ci/test-file-size-budget.json # src/lib/onboard.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates onboard session and registry messaging state to the compiled messaging plan so lifecycle, status, and rebuild flows no longer depend on legacy flat channel fields. This removes the old conflict backfill/probe compatibility path and updates coverage for plan-backed channel state across the migration surface.
Related Issue
Fixes #4947
Changes
messaging.planaccessors across onboard, rebuild, channel lifecycle, status, doctor, inventory, and policy flows.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Local checks run:
npm run typecheck:clipassesnpm run build:clipassesnpm run test-size:checkpassesnpx @biomejs/biome check test/onboard-messaging.test.ts test/channels-add-preset.test.ts src/lib/onboard/machine/final-flow-phases.test.tspassesbash -npasses for the four edited E2E shell scriptsnpx prek run --all-fileswas attempted; it fails in this local environment on unrelated full CLI/E2E tests, so it is not checked above.npm testwas attempted earlier; it timed out/fails in this local environment on unrelated full CLI/E2E tests, so it is not checked above.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit