refactor(policy): migrate messaging policy to plan-derived presets#4909
Draft
sandl99 wants to merge 11 commits into
Draft
refactor(policy): migrate messaging policy to plan-derived presets#4909sandl99 wants to merge 11 commits into
sandl99 wants to merge 11 commits into
Conversation
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>
…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>
…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>
…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>
- 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>
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>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review AdvisorFindings: 3 needs attention, 5 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. |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
…policy Three CI failures in the messaging policy migration: 1. onboard-messaging.test.ts — prepareInitialSandboxCreatePolicy received empty messagingPolicyPresets because createSandbox was called with messagingPlan=null when invoked directly (bypassing the handler). Fix: thread messagingPlan through the createSandbox dep signature; fall back to readMessagingPlanFromEnv() when caller passes null (direct call path). 2. onboard-policy-suggestions.test.ts — computeSetupPresetSuggestions stopped adding channel names (telegram, discord) as preset suggestions because the old enabledChannels loop was removed. Fix: add messagingChannelIds option that adds channel IDs as suggested preset names, mirroring the old behavior. Also add inactive-preset filtering: when messagingPolicyPresets is explicitly provided (even as []), remove messaging presets not in the active set so disabled-channel presets don't appear via open-tier defaults. 3. onboard-preset-diff.test.ts — tests used enabledChannels/disabledChannels which are no longer on SetupPolicySelectionOptions. Updated to messagingPolicyPresets (for required/merge) and messagingPolicyPresets: [] (for disabled-channel pruning). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…resets Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
The source-shape budget blocks net growth in src/lib/onboard.ts. Move the messagingChannelIds derivation (session plan channel ID extraction) into handlers/policies.ts where latestSession is already loaded, removing the two extra lines from the onboard.ts setupPoliciesWithSelection wrapper. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/onboard/policy-selection.ts (3)
294-296:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffSimilar null-handling issue: all messaging presets filtered out when plan is
null.Line 294 constructs
appliedToPreserveby filtering messaging presets that are inplanMessagingPresets, which defaults to an empty set whenoptions.messagingPolicyPresetsisnull(line 286).The filter condition
!ALL_MESSAGING_POLICY_PRESET_NAMES.has(name) || planMessagingPresets.has(name)keeps presets that are either:
- Not a messaging preset, OR
- Present in the plan's messaging presets
When
planMessagingPresetsis empty (fromnull), all messaging presets are filtered out.Impact:
For legacy sandboxes or scenarios where the plan is unavailable, previously-applied messaging presets would be removed from the preserved set, even though there's no evidence they're stale.Recommendation:
Guard the messaging-preset filter:- const appliedToPreserve = appliedPolicyPresetsForSupport.filter( - (name) => !ALL_MESSAGING_POLICY_PRESET_NAMES.has(name) || planMessagingPresets.has(name), - ); + const appliedToPreserve = options.messagingPolicyPresets !== null + ? appliedPolicyPresetsForSupport.filter( + (name) => { + const planMessagingPresets = new Set(options.messagingPolicyPresets); + return !ALL_MESSAGING_POLICY_PRESET_NAMES.has(name) || planMessagingPresets.has(name); + }, + ) + : appliedPolicyPresetsForSupport;Or reuse the
planMessagingPresetscheck from the earlier suggestion and only filter whenoptions.messagingPolicyPresets !== null.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/policy-selection.ts` around lines 294 - 296, The filter that builds appliedToPreserve removes all messaging presets when options.messagingPolicyPresets is null because planMessagingPresets defaults to an empty set; update the logic in the appliedToPreserve computation (which reads appliedPolicyPresetsForSupport and uses ALL_MESSAGING_POLICY_PRESET_NAMES and planMessagingPresets) to only require membership in planMessagingPresets when options.messagingPolicyPresets !== null (i.e., if options.messagingPolicyPresets is null treat messaging presets as allowed to preserve), so that messaging presets are not dropped for legacy/plan-unavailable cases.
377-382:⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffThird instance of null-handling inconsistency in preset preservation logic.
Lines 377–382 compute
appliedForPreservationusingmessagingPolicyPresets ?? [](line 377), which filters out all messaging presets when the plan isnull.This is the same pattern as the issue flagged in
preparePolicyPresetResumeSelection(lines 294–296). For consistency and correctness, the guard should be applied here as well:- const planMessagingPresets = new Set(messagingPolicyPresets ?? []); const appliedForPreservation = applied.filter( (name) => !isStaleBuiltinBrave(name) && - (!ALL_MESSAGING_POLICY_PRESET_NAMES.has(name) || planMessagingPresets.has(name)), + (!ALL_MESSAGING_POLICY_PRESET_NAMES.has(name) || + (messagingPolicyPresets !== null && new Set(messagingPolicyPresets).has(name))), );Or refactor to compute
planMessagingPresetsconditionally once and reuse.🤖 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/policy-selection.ts` around lines 377 - 382, The preservation filter currently treats a null messagingPolicyPresets as an empty array (using messagingPolicyPresets ?? []) which incorrectly drops all plan messaging presets; update the logic around planMessagingPresets and appliedForPreservation so you only consult planMessagingPresets when the plan exists (e.g., compute planMessagingPresets conditionally or check messagingPolicyPresets != null before using planMessagingPresets.has), and reuse this guarded value rather than always defaulting to []—adjust the code around the planMessagingPresets, messagingPolicyPresets, and appliedForPreservation symbols (same approach used in preparePolicyPresetResumeSelection) to ensure consistent null-handling.
286-290:⚠️ Potential issue | 🟡 MinorFix misleading null-handling concern for
disabledMessagingPolicyPresetApplied: atsrc/lib/onboard/machine/handlers/policies.ts,PoliciesStateOptions.messagingPolicyPresetsis typed asstring[](notnull/undefined) and that exact value is passed through topreparePolicyPresetResumeSelectionasmessagingPolicyPresets, sooptions.messagingPolicyPresets ?? []insrc/lib/onboard/policy-selection.tswill effectively behave like “plan presets provided”, not as a “null plan” case. The proposed guard againstoptions.messagingPolicyPresets !== nullis therefore unnecessary for the actual resume-selection call path.🤖 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/policy-selection.ts` around lines 286 - 290, The null-coalescing fallback is misleading because PoliciesStateOptions.messagingPolicyPresets is always a string[] in the resume-selection call path; update the logic to treat options.messagingPolicyPresets as the authoritative plan presets. Replace the creation of planMessagingPresets to use options.messagingPolicyPresets directly (no ?? []), and keep the disabledMessagingPolicyPresetApplied computation using appliedPolicyPresetsForSupport, ALL_MESSAGING_POLICY_PRESET_NAMES, and planMessagingPresets unchanged otherwise; also remove any related guard or comment that suggests messagingPolicyPresets can be null in preparePolicyPresetResumeSelection.
🤖 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/policy-selection.ts`:
- Around line 294-296: The filter that builds appliedToPreserve removes all
messaging presets when options.messagingPolicyPresets is null because
planMessagingPresets defaults to an empty set; update the logic in the
appliedToPreserve computation (which reads appliedPolicyPresetsForSupport and
uses ALL_MESSAGING_POLICY_PRESET_NAMES and planMessagingPresets) to only require
membership in planMessagingPresets when options.messagingPolicyPresets !== null
(i.e., if options.messagingPolicyPresets is null treat messaging presets as
allowed to preserve), so that messaging presets are not dropped for
legacy/plan-unavailable cases.
- Around line 377-382: The preservation filter currently treats a null
messagingPolicyPresets as an empty array (using messagingPolicyPresets ?? [])
which incorrectly drops all plan messaging presets; update the logic around
planMessagingPresets and appliedForPreservation so you only consult
planMessagingPresets when the plan exists (e.g., compute planMessagingPresets
conditionally or check messagingPolicyPresets != null before using
planMessagingPresets.has), and reuse this guarded value rather than always
defaulting to []—adjust the code around the planMessagingPresets,
messagingPolicyPresets, and appliedForPreservation symbols (same approach used
in preparePolicyPresetResumeSelection) to ensure consistent null-handling.
- Around line 286-290: The null-coalescing fallback is misleading because
PoliciesStateOptions.messagingPolicyPresets is always a string[] in the
resume-selection call path; update the logic to treat
options.messagingPolicyPresets as the authoritative plan presets. Replace the
creation of planMessagingPresets to use options.messagingPolicyPresets directly
(no ?? []), and keep the disabledMessagingPolicyPresetApplied computation using
appliedPolicyPresetsForSupport, ALL_MESSAGING_POLICY_PRESET_NAMES, and
planMessagingPresets unchanged otherwise; also remove any related guard or
comment that suggests messagingPolicyPresets can be null in
preparePolicyPresetResumeSelection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 834e28b6-e6f0-4f89-a777-81a073eaf644
📒 Files selected for processing (8)
src/lib/onboard.tssrc/lib/onboard/machine/handlers/policies.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/messaging-plan-session.tssrc/lib/onboard/policy-selection.tstest/onboard-policy-suggestions.test.tstest/onboard-preset-diff.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/onboard/messaging-plan-session.ts
- src/lib/onboard.ts
- src/lib/onboard/machine/handlers/policies.ts
…s only Disabled messaging channels could still drive sandbox network policy because getPolicyPresetsFromPlan spread raw networkPolicy.presets (which retains disabled-channel entries after compilation) and handlePoliciesState passed all plan channels as messagingChannelIds regardless of enabled state. - getPolicyPresetsFromPlan now mirrors applyPolicyAtOpenShell: derives presets from filterEnabledPlanEntries(plan, plan.networkPolicy.entries) - Add getEnabledChannelIdsFromPlan helper; use it in handlePoliciesState so disabled same-name channels (e.g. Telegram) are not suggested - Expand REQUIRED_POLICY_PRESETS_BY_MESSAGING_CHANNEL to cover all five messaging channels (discord, telegram, wechat, whatsapp) so ALL_MESSAGING_POLICY_PRESET_NAMES enables stale detection for non-Slack stopped channels and pruneDisabledMessagingPolicyPresets works in the rebuild backup-manifest path for all channels - Extract hasDisabledMessagingPreset and filterActiveMessagingPresets into policy-presets.ts; replace inline occurrences in policy-selection.ts to reduce duplication - Tighten parseSandboxMessagingPlan to validate element types in channels[], disabledChannels[], networkPolicy.presets[], and networkPolicy.entries[] before accepting a persisted plan - Add messaging-plan-session.test.ts covering disabled-channel preset exclusion, enabled channel ID derivation, and parse validation - Update rebuild-policy-presets tests to reflect correct behavior for all messaging channel presets (not just Slack) Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
wscurran
added
area: messaging
Contributor
|
✨ |
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
2 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 all messaging policy preset computation away from raw channel string lists and onto
messagingPlan.networkPolicy.presets. The compiled plan is now the single source of truth for which network policy presets active messaging channels require, including disabled-channel exclusion and stale-preset cleanup detection.Related Issue
Closes #4394
Changes
messaging-plan-session.ts— addgetPolicyPresetsFromPlanhelper that readsplan.networkPolicy.presetsdirectlymessaging/applier/policy-presets.ts(new) —pruneDisabledMessagingPolicyPresets(retained for backup-manifest paths without a plan) andALL_MESSAGING_POLICY_PRESET_NAMES; moves and slims downonboard/messaging-policy-presets.ts(deleted)policy-selection.ts— replaceenabledChannels/disabledChannelswithmessagingPolicyPresets: string[]acrossSetupPolicySelectionOptions,SetupPresetSuggestionOptions,mergeRequiredSetupPolicyPresets, andpreparePolicyPresetResumeSelection; stale-preset detection usesALL_MESSAGING_POLICY_PRESET_NAMESdiff against plan presetshandlers/policies.ts— replaceselectedMessagingChannels,mergePolicyMessagingChannels, andgetActiveSandboxdeps with a singlemessagingPolicyPresets: string[]; removerecordedMessagingChannelsfrom result typeinitial-policy.ts— addmessagingPolicyPresetsoption; removerequiredMessagingChannelPolicyPresetscallonboard.ts— wiregetPolicyPresetsFromPlan(session.messagingPlan)intohandlePoliciesStateandprepareInitialSandboxCreatePolicyrebuild.ts— update import to newmessaging/applier/policy-presetslocationmessaging-policy-presets.test.tsdeleted,messaging/applier/policy-presets.test.tsaddedType of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Release Notes
Refactor
Tests