fix(onboard): preserve slack messaging policy#3862
Open
yimoj wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughDerives required policy presets from messaging channels, merges/prunes them across create, resume, interactive/non-interactive selection, and rebuild restore flows, and adds utilities plus tests validating normalization, merging, resume reconciliation, and restore-time pruning. ChangesMessaging Policy Preset Requirements
sequenceDiagram
participant Onboard as src/lib/onboard.ts
participant PolicySelection as src/lib/onboard/policy-selection.ts
participant MessagingUtil as src/lib/onboard/messaging-policy-presets.ts
participant SetupEngine as setupPoliciesWithSelection
participant Rebuild as src/lib/actions/sandbox/rebuild.ts
Onboard->>MessagingUtil: mergePolicyMessagingChannels(selected, recorded, active, disabled)
MessagingUtil-->>Onboard: policyMessagingChannels
Onboard->>PolicySelection: preparePolicyPresetResumeSelection(..., {recordedPolicyPresets, enabledChannels, disabledChannels})
PolicySelection-->>Onboard: recordedPolicyPresetsForSupport + flags
Onboard->>SetupEngine: setupPoliciesWithSelection(..., { enabledChannels: policyMessagingChannels, disabledChannels })
SetupEngine->>PolicySelection: mergeRequiredSetupPolicyPresets(chosen/applied, {enabledChannels, hermesToolGateways})
PolicySelection->>MessagingUtil: mergeRequiredMessagingChannelPolicyPresets(..., enabledChannels)
MessagingUtil-->>PolicySelection: appended required presets
Rebuild->>MessagingUtil: pruneDisabledMessagingPolicyPresets(backupManifest.policyPresets, rebuildDisabledChannels)
MessagingUtil-->>Rebuild: savedPresets
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![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: 2
🤖 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 10028-10034: The new policy-channel merge (building
policyMessagingChannels from selectedMessagingChannels,
recordedMessagingChannels, and activeMessagingChannels) must be extracted from
onboard.ts into a small helper in src/lib/onboard (e.g.,
createPolicyMessagingChannels or mergePolicyMessagingChannels) and the top-level
onboard.ts should only call that helper with the three inputs and receive the
merged array; move the same extraction for the related block covering lines
~10054-10074 as well, ensure the helper validates
Array.isArray(activeMessagingChannels) and preserves Set de-duplication, and
update onboard.ts to import and call the helper so the entrypoint remains thin.
- Around line 10027-10034: The current merge that builds policyMessagingChannels
uses registry.getSandbox(sandboxName)?.messagingChannels
(activeMessagingChannels) without excluding channels listed in disabledChannels,
which can re-enable stopped channels like "slack"; update the logic that
constructs policyMessagingChannels to first read disabledChannels (e.g.,
registry.getSandbox(sandboxName)?.disabledChannels), ensure it's an array, and
filter out any channel names present in disabledChannels from
activeMessagingChannels (and from the combined set of
selectedMessagingChannels/recordedMessagingChannels) before creating the Set so
disabled channels are never included when deriving required policy presets.
🪄 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: c08f1f3e-cf98-48aa-8de7-23129f0805d4
📒 Files selected for processing (6)
src/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.ts
yimoj
force-pushed
the
fix/3753-slack-policy-preserve
branch
from
687c34a to
4bd022a
Compare
yimoj
force-pushed
the
fix/3753-slack-policy-preserve
branch
2 times, most recently
from
80aebaa to
2371fba
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild.ts (1)
760-763: Run the targeted channels stop/start e2e before merge.Given this branch now prunes restored presets based on disabled channels, please run the recommended
channels-stop-start-e2eworkflow to validate stop/start persistence across destroy/recreate with cached credentials.As per coding guidelines: "
src/lib/actions/sandbox/rebuild.ts: This file controls disabled channel resolution used during onboard and rebuild... E2E test recommendation:channels-stop-start-e2e."🤖 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/rebuild.ts` around lines 760 - 763, The change prunes restored presets using pruneDisabledMessagingPolicyPresets (called into savedPresets with rebuildDisabledChannels), so run the channels-stop-start-e2e workflow before merging to validate stop/start persistence across destroy/recreate with cached credentials; specifically, execute the channels-stop-start-e2e pipeline against this branch, confirm that disabled channel resolution (pruneDisabledMessagingPolicyPresets -> savedPresets) preserves stop/start state after sandbox rebuild, and report any failing scenarios to adjust how rebuildDisabledChannels is computed or applied during restore.
🤖 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 10060-10078: The fast resume path only verifies that the remaining
(pruned) presets are present in the sandbox via arePolicyPresetsApplied, but it
does not detect extra applied presets that were removed because their channels
are now disabled (e.g., Slack still applied). Update the resumePolicies
condition to also ensure there are no additional applied policy presets beyond
recordedPolicyPresetsForSupport: either replace
arePolicyPresetsApplied(sandboxName, recordedPolicyPresetsForSupport) with a
strict equality check (sandboxAppliedPresets ===
recordedPolicyPresetsForSupport) or add a complementary function (e.g.,
arePolicyPresetsExactMatch or hasNoExtraAppliedPolicyPresets) and use that
together with recordedPolicyPresetsNeedReconcile, referencing
recordedPolicyPresetsForSupport, recordedPolicyPresetsNeedReconcile,
arePolicyPresetsApplied and resumePolicies.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild.ts`:
- Around line 760-763: The change prunes restored presets using
pruneDisabledMessagingPolicyPresets (called into savedPresets with
rebuildDisabledChannels), so run the channels-stop-start-e2e workflow before
merging to validate stop/start persistence across destroy/recreate with cached
credentials; specifically, execute the channels-stop-start-e2e pipeline against
this branch, confirm that disabled channel resolution
(pruneDisabledMessagingPolicyPresets -> savedPresets) preserves stop/start state
after sandbox rebuild, and report any failing scenarios to adjust how
rebuildDisabledChannels is computed or applied during restore.
🪄 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: 2333fa41-c915-4b53-934e-fd74904099dd
📒 Files selected for processing (8)
src/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.tstest/rebuild-policy-presets.test.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
10052-10063:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't skip policy reconciliation while a disabled messaging preset is still applied.
Line 10063 only checks that the pruned set is present, not that disabled-channel presets are absent. Because Lines 10076-10080 persist
recordedPolicyPresetsForSupportbeforesetupPoliciesWithSelection()runs, an interrupted run can leave the session withoutslackwhile the sandbox still has it applied. The next--resumewill satisfy this fast path and keep the stale egress policy.💡 Minimal fix
+ const appliedPresetNames = policies.getAppliedPresets(sandboxName); + const hasDisabledMessagingPresetsApplied = + pruneDisabledMessagingPolicyPresets( + appliedPresetNames, + activeSandbox?.disabledChannels, + ).length !== appliedPresetNames.length; const resumePolicies = resume && sandboxName && !recordedPolicyPresetsNeedReconcile && + !hasDisabledMessagingPresetsApplied && arePolicyPresetsApplied(sandboxName, recordedPolicyPresetsForSupport);Also applies to: 10076-10080
🤖 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 10052 - 10063, The fast-path resume incorrectly treats a pruned recordedPolicyPresetsForSupport as valid even when it still contains presets that target disabled messaging channels; update the recordedPolicyPresetsNeedReconcile logic (used to compute resumePolicies) to consider not only length mismatch but also whether any preset in recordedPolicyPresetsForSupport touches activeSandbox?.disabledChannels (i.e., detect preset.channel intersections) so the resume path is blocked when disabled-channel presets remain; also ensure the code that persists recordedPolicyPresetsForSupport (the block around where you save them before calling setupPoliciesWithSelection) is moved or gated so persistence happens only after setupPoliciesWithSelection completes successfully. Reference symbols: recordedPolicyPresetsForSupport, recordedPolicyPresetsNeedReconcile, pruneDisabledMessagingPolicyPresets, mergeRequiredSetupPolicyPresets, arePolicyPresetsApplied, setupPoliciesWithSelection, activeSandbox?.disabledChannels.
🧹 Nitpick comments (1)
src/lib/actions/sandbox/rebuild.ts (1)
760-763: Run the channels stop/start E2E for this path-sensitive change.Given this logic now gates preset restore based on disabled channels, run the targeted lifecycle E2E to confirm
channels stop/channels startbehavior remains stable across rebuild.As per coding guidelines
src/lib/actions/sandbox/rebuild.ts: “This file controls disabled channel resolution used during onboard and rebuild… E2E test recommendation:channels-stop-start-e2e”.🤖 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/rebuild.ts` around lines 760 - 763, This change affects how disabled channels gate preset restoration in src/lib/actions/sandbox/rebuild.ts (see pruneDisabledMessagingPolicyPresets, savedPresets, rebuildDisabledChannels), so run the targeted lifecycle E2E suite named channels-stop-start-e2e against the rebuild/onboard path to validate channels stop/start behavior; if the E2E fails, trace failures back to pruneDisabledMessagingPolicyPresets or the rebuildDisabledChannels resolution logic and update the code or tests to ensure disabled-channel preservation/restore semantics match expected behavior before merging.
🤖 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.
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 10052-10063: The fast-path resume incorrectly treats a pruned
recordedPolicyPresetsForSupport as valid even when it still contains presets
that target disabled messaging channels; update the
recordedPolicyPresetsNeedReconcile logic (used to compute resumePolicies) to
consider not only length mismatch but also whether any preset in
recordedPolicyPresetsForSupport touches activeSandbox?.disabledChannels (i.e.,
detect preset.channel intersections) so the resume path is blocked when
disabled-channel presets remain; also ensure the code that persists
recordedPolicyPresetsForSupport (the block around where you save them before
calling setupPoliciesWithSelection) is moved or gated so persistence happens
only after setupPoliciesWithSelection completes successfully. Reference symbols:
recordedPolicyPresetsForSupport, recordedPolicyPresetsNeedReconcile,
pruneDisabledMessagingPolicyPresets, mergeRequiredSetupPolicyPresets,
arePolicyPresetsApplied, setupPoliciesWithSelection,
activeSandbox?.disabledChannels.
---
Nitpick comments:
In `@src/lib/actions/sandbox/rebuild.ts`:
- Around line 760-763: This change affects how disabled channels gate preset
restoration in src/lib/actions/sandbox/rebuild.ts (see
pruneDisabledMessagingPolicyPresets, savedPresets, rebuildDisabledChannels), so
run the targeted lifecycle E2E suite named channels-stop-start-e2e against the
rebuild/onboard path to validate channels stop/start behavior; if the E2E fails,
trace failures back to pruneDisabledMessagingPolicyPresets or the
rebuildDisabledChannels resolution logic and update the code or tests to ensure
disabled-channel preservation/restore semantics match expected behavior before
merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 65d36433-4682-45c9-a848-1c36a52da176
📒 Files selected for processing (8)
src/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.tstest/rebuild-policy-presets.test.ts
yimoj
force-pushed
the
fix/3753-slack-policy-preserve
branch
2 times, most recently
from
63b2174 to
f12a919
Compare
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 (2)
src/lib/onboard.ts (2)
10072-10110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop disabled-channel cleanup presets when session policy history is absent.
mergeAppliedPolicyPresetsForDisabledMessagingCleanup(...)can synthesize presets that must be shown tosetupPoliciesWithSelection()so the stale disabled-channel policy gets removed. Lines 10105-10108 discard that state wheneverlatestSession.policyPresetsisnull, so resumed/legacy sessions can still leave a disabled preset likeslackapplied.💡 Suggested change
+ const selectedPolicyPresetsForSetup = + Array.isArray(recordedPolicyPresets) || recordedPolicyPresetsForSupport.length > 0 + ? recordedPolicyPresetsForSupport + : null; const setupAppliedPolicyPresets = await setupPoliciesWithSelection(sandboxName, { - selectedPresets: - Array.isArray(recordedPolicyPresets) - ? recordedPolicyPresetsForSupport - : null, + selectedPresets: selectedPolicyPresetsForSetup, enabledChannels: policyMessagingChannels, disabledChannels: activeSandbox?.disabledChannels, webSearchConfig,🤖 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 10072 - 10110, The code discards synthesized cleanup presets from mergeAppliedPolicyPresetsForDisabledMessagingCleanup whenever recordedPolicyPresets is null, so setupPoliciesWithSelection may not receive necessary selectedPresets to remove stale disabled-channel policies; change the selectedPresets argument passed into setupPoliciesWithSelection to use recordedPolicyPresetsForSupport (or at least check its length) instead of gating on Array.isArray(recordedPolicyPresets), ensuring that when recordedPolicyPresetsForSupport contains synthesized presets they are forwarded to setupPoliciesWithSelection; update the ternary at the selectedPresets key (or its surrounding logic) to pass recordedPolicyPresetsForSupport when non-empty so disabled-channel cleanup presets are not dropped.
10027-10124: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract this reconciliation block to clear the entrypoint-budget CI failure.
This chunk is the main new orchestration added to
src/lib/onboard.ts, and CI is currently red because the file grew past the enforced budget. Moving this policy-state assembly behind a small helper insrc/lib/onboard/*should unblock the check while keeping the entrypoint thin.🤖 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 10027 - 10124, Extract the policy-state reconciliation block (starting at the recordedPolicyPresets/recordedMessagingChannels lines through the computation of resumePolicies and prior to the resume handling) into a new helper function (e.g., assemblePolicyState) in a new module under src/lib/onboard/; the helper should accept the minimal inputs (sandboxName, latestSession or recordedPolicyPresets/recordedMessagingChannels, selectedMessagingChannels, registry.getSandbox result or sandboxName+registry, policies, hermesToolGateways, webSearchSupported, activeSandbox?.disabledChannels, webSearchConfig, provider, model, agent, etc.) and return the computed values used afterward (at least recordedPolicyPresetsForSupport, policyMessagingChannels, appliedPolicyPresetsForSupport, disabledMessagingPolicyPresetApplied, resumePolicies); use the existing helper functions referenced in the block (mergePolicyMessagingChannels, pruneDisabledMessagingPolicyPresets, policies.clampSetupPolicyPresetNames, hasDisabledMessagingPolicyPreset, mergeAppliedPolicyPresetsForDisabledMessagingCleanup, mergeRequiredSetupPolicyPresets, arePolicyPresetsApplied) inside the new assemblePolicyState function and export it, then replace the large inline block in src/lib/onboard.ts with a single call to assemblePolicyState and use its returned values unchanged.
🧹 Nitpick comments (2)
src/lib/onboard.ts (1)
10027-10124: Please run the lifecycle E2Es for this path before merge.This touches resume, disabled-channel cleanup, and Slack policy preservation, so I’d specifically want
channels-stop-start-e2e,rebuild-openclaw-e2e, andhermes-slack-e2eon this revision.As per coding guidelines,
channels-stop-start-e2e,rebuild-openclaw-e2e, andhermes-slack-e2eare recommended for onboarding changes insrc/lib/onboard.ts.🤖 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 10027 - 10124, Summary: This change touches resume logic and disabled-channel/policy preset handling; run the relevant E2E suites before merging. Action: execute the lifecycle E2E tests for this onboarding path — channels-stop-start-e2e, rebuild-openclaw-e2e, and hermes-slack-e2e — and verify no regressions around resume, disabled-channel cleanup, and Slack policy preservation; focus validation on flows that exercise resume, mergeRequiredSetupPolicyPresets, pruneDisabledMessagingPolicyPresets, mergeAppliedPolicyPresetsForDisabledMessagingCleanup, hasDisabledMessagingPolicyPreset, arePolicyPresetsApplied, and setupPoliciesWithSelection; if failures occur, capture failures, reproduce locally, and update the code or tests until all three suites pass.test/onboard-preset-diff.test.ts (1)
324-458: ⚡ Quick winHarden these Slack regression tests against inherited messaging env leakage.
These new assertions can flake if parent
DISCORD_*/TELEGRAM_*env vars leak into the spawned helper process and activate extra channels implicitly.Proposed patch (in
buildPreamblescript body)return String.raw` // All stubs MUST be installed before requiring onboard so its module-level // destructuring picks up the patched functions. Object.defineProperty(process, "platform", { value: "darwin" }); + +for (const key of Object.keys(process.env)) { + if (key.startsWith("DISCORD_") || key.startsWith("TELEGRAM_")) { + delete process.env[key]; + } +} const resolver = require(${resolveOpenshellPath}); resolver.resolveOpenshell = () => "/fake/openshell";Based on learnings: For hermetic messaging-channel tests in this repo, spawned helper scripts should remove inherited
DISCORD_*andTELEGRAM_*env vars to avoid unintended channel activation.🤖 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 `@test/onboard-preset-diff.test.ts` around lines 324 - 458, The tests flake because inherited DISCORD_* and TELEGRAM_* env vars can activate messaging channels in the spawned helper; modify the buildPreamble output to explicitly remove any process.env keys that start with "DISCORD_" or "TELEGRAM_" before the helper logic runs. Concretely, inside the buildPreamble-generated script (the preamble string emitted by buildPreamble), add a short sanitization block that iterates Object.keys(process.env) and deletes keys matching /^DISCORD_/ or /^TELEGRAM_/ (e.g., for (const k of Object.keys(process.env)) if (k.startsWith("DISCORD_") || k.startsWith("TELEGRAM_")) delete process.env[k]); place this at the very top of the preamble so setupPoliciesWithSelection and any channel-detection logic sees a hermetic env.
🤖 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.ts`:
- Around line 10072-10110: The code discards synthesized cleanup presets from
mergeAppliedPolicyPresetsForDisabledMessagingCleanup whenever
recordedPolicyPresets is null, so setupPoliciesWithSelection may not receive
necessary selectedPresets to remove stale disabled-channel policies; change the
selectedPresets argument passed into setupPoliciesWithSelection to use
recordedPolicyPresetsForSupport (or at least check its length) instead of gating
on Array.isArray(recordedPolicyPresets), ensuring that when
recordedPolicyPresetsForSupport contains synthesized presets they are forwarded
to setupPoliciesWithSelection; update the ternary at the selectedPresets key (or
its surrounding logic) to pass recordedPolicyPresetsForSupport when non-empty so
disabled-channel cleanup presets are not dropped.
- Around line 10027-10124: Extract the policy-state reconciliation block
(starting at the recordedPolicyPresets/recordedMessagingChannels lines through
the computation of resumePolicies and prior to the resume handling) into a new
helper function (e.g., assemblePolicyState) in a new module under
src/lib/onboard/; the helper should accept the minimal inputs (sandboxName,
latestSession or recordedPolicyPresets/recordedMessagingChannels,
selectedMessagingChannels, registry.getSandbox result or sandboxName+registry,
policies, hermesToolGateways, webSearchSupported,
activeSandbox?.disabledChannels, webSearchConfig, provider, model, agent, etc.)
and return the computed values used afterward (at least
recordedPolicyPresetsForSupport, policyMessagingChannels,
appliedPolicyPresetsForSupport, disabledMessagingPolicyPresetApplied,
resumePolicies); use the existing helper functions referenced in the block
(mergePolicyMessagingChannels, pruneDisabledMessagingPolicyPresets,
policies.clampSetupPolicyPresetNames, hasDisabledMessagingPolicyPreset,
mergeAppliedPolicyPresetsForDisabledMessagingCleanup,
mergeRequiredSetupPolicyPresets, arePolicyPresetsApplied) inside the new
assemblePolicyState function and export it, then replace the large inline block
in src/lib/onboard.ts with a single call to assemblePolicyState and use its
returned values unchanged.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 10027-10124: Summary: This change touches resume logic and
disabled-channel/policy preset handling; run the relevant E2E suites before
merging. Action: execute the lifecycle E2E tests for this onboarding path —
channels-stop-start-e2e, rebuild-openclaw-e2e, and hermes-slack-e2e — and verify
no regressions around resume, disabled-channel cleanup, and Slack policy
preservation; focus validation on flows that exercise resume,
mergeRequiredSetupPolicyPresets, pruneDisabledMessagingPolicyPresets,
mergeAppliedPolicyPresetsForDisabledMessagingCleanup,
hasDisabledMessagingPolicyPreset, arePolicyPresetsApplied, and
setupPoliciesWithSelection; if failures occur, capture failures, reproduce
locally, and update the code or tests until all three suites pass.
In `@test/onboard-preset-diff.test.ts`:
- Around line 324-458: The tests flake because inherited DISCORD_* and
TELEGRAM_* env vars can activate messaging channels in the spawned helper;
modify the buildPreamble output to explicitly remove any process.env keys that
start with "DISCORD_" or "TELEGRAM_" before the helper logic runs. Concretely,
inside the buildPreamble-generated script (the preamble string emitted by
buildPreamble), add a short sanitization block that iterates
Object.keys(process.env) and deletes keys matching /^DISCORD_/ or /^TELEGRAM_/
(e.g., for (const k of Object.keys(process.env)) if (k.startsWith("DISCORD_") ||
k.startsWith("TELEGRAM_")) delete process.env[k]); place this at the very top of
the preamble so setupPoliciesWithSelection and any channel-detection logic sees
a hermetic env.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 401b41ae-3c4f-45d5-b9e8-4fe21ac50f17
📒 Files selected for processing (8)
src/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.tstest/rebuild-policy-presets.test.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
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 (1)
src/lib/onboard.ts (1)
10024-10035:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the effective enabled channel set for the smoke verifier.
activeSandbox.messagingChannelsis the configured set, not the enabled set; this file intentionally preserves disabled channels there sochannels startcan restore them later. Passing that value here meansverifyCompatibleEndpointSandboxSmoke(...)can still treat a stopped channel as active even though the policy flow now usespolicyMessagingChannels.Suggested fix
- const activeMessagingChannels = activeSandbox?.messagingChannels; + const activeMessagingChannels = activeSandbox?.messagingChannels; const policyMessagingChannels = mergePolicyMessagingChannels(selectedMessagingChannels, recordedMessagingChannels, activeMessagingChannels, activeSandbox?.disabledChannels); @@ - messagingChannels: Array.isArray(activeMessagingChannels) ? activeMessagingChannels : [], + messagingChannels: policyMessagingChannels,🤖 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 10024 - 10035, The smoke verifier is being called with the raw configured set (activeMessagingChannels) which can include channels marked disabled; instead pass the effective enabled set (policyMessagingChannels) into verifyCompatibleEndpointSandboxSmoke so the verifier uses the same channel view as the policy flow—update the call site that currently passes Array.isArray(activeMessagingChannels) ? activeMessagingChannels : [] to instead pass Array.isArray(policyMessagingChannels) ? policyMessagingChannels : [] (referencing activeSandbox, activeMessagingChannels, policyMessagingChannels and verifyCompatibleEndpointSandboxSmoke to locate the change).
🧹 Nitpick comments (1)
test/rebuild-policy-presets.test.ts (1)
181-188: ⚡ Quick winConsider adding a clarifying comment to explain the test expectation.
The test expects
telegramto be preserved when disabled, while test 1 expectsslackto be removed when disabled. Without context, this appears contradictory. The test would benefit from a brief comment explaining the distinction (e.g., "telegram channel does not require a 'telegram' policy preset, so a preset named 'telegram' is preserved" or "only channels with known policy mappings are pruned").The phrase "for later start and rebuild" in the test name is also unclear—consider clarifying what "later start" refers to in the test documentation.
📝 Suggested clarifying comment
+ // Unlike Slack (which requires the 'slack' preset), Telegram does not have + // a required policy preset mapping, so a preset named 'telegram' is preserved + // even when the Telegram channel is disabled. it("preserves non-required channel presets for later start and 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 `@test/rebuild-policy-presets.test.ts` around lines 181 - 188, Add a short clarifying comment inside the test "preserves non-required channel presets for later start and rebuild" explaining why "telegram" is expected to be preserved (e.g., that pruneDisabledMessagingPolicyPresets only prunes presets that map to known/required channels, and "telegram" in this case is considered non-required so it should remain), and rename or expand the test name to clarify what "later start" means (for example "preserves non-required channel presets for later start/rebuild when channel mapping is absent"); update references in the test to pruneDisabledMessagingPolicyPresets, manifest.policyPresets and savedPresets to make the intent explicit.
🤖 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.ts`:
- Around line 10024-10035: The smoke verifier is being called with the raw
configured set (activeMessagingChannels) which can include channels marked
disabled; instead pass the effective enabled set (policyMessagingChannels) into
verifyCompatibleEndpointSandboxSmoke so the verifier uses the same channel view
as the policy flow—update the call site that currently passes
Array.isArray(activeMessagingChannels) ? activeMessagingChannels : [] to instead
pass Array.isArray(policyMessagingChannels) ? policyMessagingChannels : []
(referencing activeSandbox, activeMessagingChannels, policyMessagingChannels and
verifyCompatibleEndpointSandboxSmoke to locate the change).
---
Nitpick comments:
In `@test/rebuild-policy-presets.test.ts`:
- Around line 181-188: Add a short clarifying comment inside the test "preserves
non-required channel presets for later start and rebuild" explaining why
"telegram" is expected to be preserved (e.g., that
pruneDisabledMessagingPolicyPresets only prunes presets that map to
known/required channels, and "telegram" in this case is considered non-required
so it should remain), and rename or expand the test name to clarify what "later
start" means (for example "preserves non-required channel presets for later
start/rebuild when channel mapping is absent"); update references in the test to
pruneDisabledMessagingPolicyPresets, manifest.policyPresets and savedPresets to
make the intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1bbda8e1-3f2f-46a5-bb47-3a3b45e30719
📒 Files selected for processing (8)
src/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.tstest/rebuild-policy-presets.test.ts
yimoj
force-pushed
the
fix/3753-slack-policy-preserve
branch
from
f12a919 to
174e2d8
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/policy-selection.ts`:
- Around line 192-198: The current truthy checks (e.g., if
(options.recordedPolicyPresets)) skip empty-array selections and thus drop
required presets on resume; change these guards to explicitly check for
undefined/null (for example: if (options.recordedPolicyPresets !== undefined &&
options.recordedPolicyPresets !== null) or use
Array.isArray(options.recordedPolicyPresets)) so an empty [] is treated as valid
input and still calls mergeRequiredSetupPolicyPresets with the same payload
(include selectablePolicyPresets.map(...) etc.); apply the same explicit-check
fix to the other occurrence around lines 264-272 to ensure resume/replay always
merges required messaging presets when channels are enabled.
In `@test/onboard-preset-diff.test.ts`:
- Around line 324-458: The tests are flaky because the spawned test script
inherits host env vars that can enable messaging channels; update the runScript
helper to sanitize the environment before spawning the child: copy process.env,
remove any keys starting with "DISCORD_" or "TELEGRAM_" (and any other
messaging-related prefixes you see causing leakage), and pass that cleaned env
into the child process spawn/exec call (the runScript function is the unique
symbol to change). Ensure the rest of runScript behavior (stdout/stderr capture,
exit status) remains unchanged.
🪄 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: 3a295cff-be85-4026-9701-4a709a176570
📒 Files selected for processing (8)
src/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/messaging-policy-presets.test.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/policy-selection.tstest/onboard-preset-diff.test.tstest/rebuild-policy-presets.test.ts
Signed-off-by: Yimo Jiang <yimoj@nvidia.com>
yimoj
force-pushed
the
fix/3753-slack-policy-preserve
branch
from
174e2d8 to
fb110d9
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
wscurran
added
enhancement: policy
fix
Integration: Slack
Contributor
|
✨ Related open issues: |
This was referenced May 20, 2026
This was referenced May 21, 2026
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
3 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
Preserves messaging-required policy presets when onboarding resumes or reapplies a recorded policy selection. This prevents Slack messaging rebuilds from keeping the channel configuration while dropping the
slacknetwork policy.Related Issue
Fixes #3753
Changes
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Focused verification run locally:
npx vitest run src/lib/onboard/messaging-policy-presets.test.ts src/lib/onboard/initial-policy.test.ts test/onboard-preset-diff.test.ts test/policy-tiers-onboard.test.tsnpm run typecheck:clinpm run build:clicd nemoclaw && npm install && npm run buildnpx vitest run test/ssrf-parity.test.ts nemoclaw/src/commands/migration-state.test.tscodex review -c sandbox_mode=\"danger-full-access\" --uncommittedFull
npm testwas attempted under isolatedHOME=/tmp/nemoclaw-fix-3753-home/NEMOCLAW_HOME=/tmp/nemoclaw-fix-3753-home/.nemoclaw; it failed in unrelated broad-suite/environment paths (timeouts in CLI/e2e scenario tests and existing gateway-state-sensitive tests). Missing plugin dependency/dist blockers were addressed withcd nemoclaw && npm install && npm run build, and the affectedssrf-parity/migration-statetests then passed.Signed-off-by: Yimo Jiang yimoj@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests