fix(onboard): exclude inactive OpenClaw messaging presets - #10359
fix(onboard): exclude inactive OpenClaw messaging presets#10359HOYALIM wants to merge 3 commits into
Conversation
Signed-off-by: Ho Lim <subhoya@gmail.com>
|
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 (10)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change generalizes inactive messaging preset pruning from Hermes to recognized messaging agents. Onboarding reconciliation and sandbox rebuilds now use typed channel plans and preserve active or operator-owned custom presets. ChangesMessaging preset pruning
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change narrows built-in messaging preset selection to configured active channels and preserves custom policies; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
rsliter
left a comment
rsliter
left a comment
There was a problem hiding this comment.
Reviewed latest PR commit 903a3bb0848ca4b9d2140bbb8c0af8a6ab1fc4d2 against base SHA 896760b7cd407606050ce609dbcce429b06cc3f1. I found no critical security or correctness defect in the isolated diff. The policy-selection tests passed 63 focused tests, and the CLI build passed. The nine-category security review has no FAIL result; configuration, testing, and integration remain WARNING conditions.
This is a conditional assessment, not an approval. The branch conflicts with current main, and fresh required checks have not run. Please resolve the conflict without dropping #10314 behavior, confirm the product scope for pruning every inactive OpenClaw messaging preset, align the canonical Open-tier documentation, and obtain fresh passing required checks, including the focused Slack pairing E2E. I will re-review the resulting latest PR commit.
Resolve the conflict against NVIDIA#10314 and NVIDIA#10281 by keeping main's suggestion gate and its shared inactive-preset helper. The original wrapper-based prune is dropped here and replaced by an agent-set change in the next commit. Signed-off-by: Hung Le <hple@nvidia.com>
NVIDIA#10281 stopped Hermes from carrying a repository messaging preset for a channel the sandbox does not configure. OpenClaw reaches the same selection paths, so it kept applying a preset for an unconfigured channel, including the Discord preset that names a sandbox-scoped credential provider create-plan validation rejects. Resolve the agent set from supportedAgents in the channel manifests instead of naming Hermes in the guard, and rename the helper to match what it now covers. An agent that appears in no manifest keeps its selection untouched, a missing messaging plan stays no authority to call a channel inactive, and a custom preset that shadows the name is still preserved. Read the rebuild agent from the messaging plan rather than the registry entry. getSandboxAgentRegistryFields stores agent as null for OpenClaw, so the registry field made every default OpenClaw rebuild skip the guard. Build rebuild's enabled-channel set with getActiveChannelIdsFromPlan. Filtering on `disabled` alone counted a configured channel the plan cannot start, which kept its egress preset and left the same dangling credential binding behind. Give the rebuild fixtures production-shaped plans. The existing custom-policy fixture passes even when the shared helper is a no-op, and the lifecycle fixture omitted `active` and its credential hashes, so it read as fully inactive once rebuild resolved the canonical active set. Refs NVIDIA#10153 Signed-off-by: Hung Le <hple@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR Review Advisor finished for commit |
|
Thanks @HOYALIM — this found a real gap, and the diagnosis in the description was right: OpenClaw was keeping a messaging preset for a channel the sandbox does not configure. Closing in favour of #10490, which carries the same fix from a branch in this repository. Two reasons:
Your commit is credited as co-author on #10490. A baseline run of the six OpenClaw targets on |
wscurran
added
area: messaging
Outcome
OpenClaw no longer carries a repository messaging preset for a channel the sandbox does not configure. The onboard suggestion path, a recorded or resumed selection, and
nemoclaw sandbox rebuildnow follow one rule instead of leaving OpenClaw out of it.Reason
#10314 gated the Discord suggestion for every agent, and #10281 added the shared inactive-preset guard but scoped it to Hermes. OpenClaw ships the same channel policies and reaches the same selection paths, so it kept applying a preset whose policy names a sandbox-scoped credential provider that create-plan validation then rejects.
This PR was opened against root-cause group 5 of #10153. That symptom is already resolved on
main: a baseline run of the six OpenClaw targets onmainat4c5840777, which contains #10314 and #10281, passed all six. The scope here is the remaining OpenClaw half of the guard, not that root cause.Related issues
Refs #10153
Changes
src/lib/onboard/messaging-policy-presets.ts: renamepruneInactiveHermesMessagingPolicyPresetstopruneInactiveMessagingPolicyPresetsand resolve its agent set fromsupportedAgentsin the channel manifests instead of naming Hermes. An agent that appears in no manifest keeps its selection untouched, a missing messaging plan is still no authority to call a channel inactive, and a custom preset that shadows the name is still preserved.src/lib/onboard/policy-preset-reconciliation.ts,src/lib/onboard/policy-selection.ts,src/lib/onboard/openclaw-otel-policy-presets.test.ts: rename call sites only.src/lib/actions/sandbox/rebuild-backup-phase.ts: read the rebuild agent from the messaging plan instead of the registry entry.getSandboxAgentRegistryFieldsstoresagent: nullfor OpenClaw, so the registry field made every default OpenClaw rebuild skip the guard.src/lib/actions/sandbox/rebuild-backup-phase.ts: build the enabled-channel set withgetActiveChannelIdsFromPlan. Filtering ondisabledalone counted a configured channel the plan cannot start, which kept its egress preset and left the same dangling credential binding behind.policy-resume-selection.test.ts, a downstream application-boundary guard inpolicy-selection-application.test.ts, and built-in-only rebuild fixtures inrebuild-backup-phase.test.ts.rebuild-flow-lifecycle.test.tsnow builds production-shaped channels and manifest-resolved credential bindings; its previous partial literals read as inactive once rebuild resolved the canonical active set.The guard is not a new mechanism. Its current consumers are the three selection paths named above, and each has a test that fails when the guard is turned into a no-op.
Verification
npm run test:changed— 134 test files, 2166 tests, 0 failures.npm run typecheck:cli— clean.npm run checks:repository— passed, including the source architecture budget.npx vitest run --project integration test/automation/pull-requests/growth-guardrails.test.ts— 32 passed.git diff --check— clean.rebuild-backup-phase.test.ts,messaging-policy-presets.test.ts,policy-selection-application.test.tsand both OpenClaw rows inpolicy-resume-selection.test.ts; restoringsandboxEntry.agentin rebuild fails only the OpenClaw rows; restoring the!channel.disabledfilter fails only the cannot-start row.mainat4c5840777without this change — run 33047892284, targetscommon-egress-agent-openclaw-open-reference,openclaw-slack-pairing,telegram-injection,sessions-agents-cli,tunnel-lifecycle,inference-routing,inference_mode=mock: all six passed.Review notes
Sensitive paths changed: onboarding, policy selection, messaging, and sandbox rebuild. The change narrows which presets reach a policy and does not widen any credential authority.
This PR's scope changed after it was opened. The original diff added an OpenClaw-specific helper alongside #10281's Hermes one and wrapped the shared unavailable-preset pruner, which dropped
preserveExplicitWebSearchand pruned an operator-requestedbravepreset;cli-test-shards (4)failed onpolicy-tiers-onboard.test.ts:679at the previous head while the PR base was green. That approach is replaced here by widening the existing helper, so one concept keeps one name.Live E2E at this head cannot run the credentialed targets. This is a fork PR, and
references/manual-pr.mdin the maintainer E2E skill records that an external PR keeps the credential-free controller selection. Of the six OpenClaw targets, onlyinference-routingcarries thestandardprofile; the rest arenvidia-inferenceorbrave-nvidia-inferenceand are excluded byisPrCandidateCatalogueTarget. Maintainer direction is requested on whether the post-mergemainE2E is the intended evidence for a fork PR.Two adjacent gaps surfaced during review. Both predate this change, both currently affect Hermes, and neither is introduced or worsened by widening the agent set:
getAppliedPresetsreturns[...builtinPolicies, ...customPolicyNames]as a flat list (src/lib/policy/index.ts), and the helper exempts a preset whenever any custom policy shares its name. A sandbox recorded with both a built-indiscordpolicy and a custom policy nameddiscordkeeps the built-in credential-bound policy live. Distinguishing ownership belongs in reconciliation, not in this guard.preparedRecoveryManifest. They remain sufficient for the focused pruning assertions and should not be cited as provider or full messaging-restore coverage.Signed-off-by: Ho Lim subhoya@gmail.com
Signed-off-by: Hung Le hple@nvidia.com