feat(messaging): migrate enrollment to manifest hooks#4248
Open
sandl99 wants to merge 28 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
🚥 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)
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: 3
🧹 Nitpick comments (1)
src/lib/onboard.ts (1)
5940-5945: Please run the onboarding/messaging E2Es for this delegation.This moves the step-5 messaging flow behind a shared implementation in the core onboarding entrypoint, so I'd still smoke the onboarding-focused E2Es before merge — especially
cloud-e2e,sandbox-operations-e2e,channels-stop-start-e2e, andmessaging-compatible-endpoint-e2e.As per coding guidelines,
src/lib/onboard.ts: "This file contains core onboarding logic. Changes here affect the full sandbox creation and configuration flow."🤖 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 5940 - 5945, The change routes step-5 messaging through setupMessagingChannelsImpl in src/lib/onboard.ts; run the onboarding/messaging E2E suites for this delegation to validate behavior: execute cloud-e2e, sandbox-operations-e2e, channels-stop-start-e2e, and messaging-compatible-endpoint-e2e against the updated onboarding flow (particularly exercising setupMessagingChannelsImpl and the caller setupMessagingChannels path) and fix any failures uncovered 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.
Inline comments:
In `@src/lib/messaging/channels/slack/manifest.ts`:
- Line 22: The formatHint strings in the Slack channel manifest contain
token-like sample literals that trigger secret scanning; update the formatHint
values (both occurrences referenced by the formatHint property in the Slack
manifest object) to use redacted placeholders instead of realistic token
patterns (e.g., replace "xoxb-1234-5678-abcdef" style examples with something
like "xoxb-REDACTED" or "<redacted>") so the manifest still documents expected
format without including token-like samples; ensure you update both formatHint
occurrences (the formatHint property on the exported Slack manifest)
consistently.
In `@src/lib/messaging/channels/wechat/hooks/index.ts`:
- Around line 28-31: The current spread merge into ilinkLoginOptions lets
properties in options.ilinkLogin that are explicitly undefined overwrite
defaults from createDefaultWechatHostQrLoginOptions(), which can remove required
flags like runLogin or saveCredential; before merging, create a cleaned override
object by filtering out keys with undefined values from options.ilinkLogin and
then spread that cleaned object into ilinkLoginOptions (referencing
ilinkLoginOptions, createDefaultWechatHostQrLoginOptions, options.ilinkLogin,
runLogin, saveCredential to locate the code).
In `@src/lib/onboard/messaging-channel-setup.ts`:
- Around line 53-55: The current helper hasManifestCredentials uses
hasMessagingManifestPrimaryCredential(manifest, getMessagingToken), which only
checks the primary secret and allows non-interactive seeding to include
partially configured channels; update hasManifestCredentials so it validates
that all required secrets for a ChannelManifest are present (not just the
primary) and use that stricter predicate inside seedFromState and the
non-interactive branch starting around seedFromState (lines 63-81) so
non-interactive seeding only includes manifests where every required credential
is available; locate and modify the functions hasManifestCredentials and any
call sites in seedFromState to call the new “all required secrets present” check
instead of hasMessagingManifestPrimaryCredential.
---
Nitpick comments:
In `@src/lib/onboard.ts`:
- Around line 5940-5945: The change routes step-5 messaging through
setupMessagingChannelsImpl in src/lib/onboard.ts; run the onboarding/messaging
E2E suites for this delegation to validate behavior: execute cloud-e2e,
sandbox-operations-e2e, channels-stop-start-e2e, and
messaging-compatible-endpoint-e2e against the updated onboarding flow
(particularly exercising setupMessagingChannelsImpl and the caller
setupMessagingChannels path) and fix any failures uncovered before merging.
🪄 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: 09da88a8-cae8-4ea1-b37a-29bf2ebfdd2d
📒 Files selected for processing (31)
src/lib/actions/sandbox/policy-channel.tssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/channels/discord/manifest.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/channels/telegram/hooks/get-me-reachability.test.tssrc/lib/messaging/channels/telegram/manifest.tssrc/lib/messaging/channels/wechat/hooks/host-qr-login-runtime.tssrc/lib/messaging/channels/wechat/hooks/ilink-login.tssrc/lib/messaging/channels/wechat/hooks/implementations.test.tssrc/lib/messaging/channels/wechat/hooks/index.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/hooks/builtins.tssrc/lib/messaging/hooks/common/config-prompt.test.tssrc/lib/messaging/hooks/common/config-prompt.tssrc/lib/messaging/hooks/common/index.tssrc/lib/messaging/hooks/common/token-paste.test.tssrc/lib/messaging/hooks/common/token-paste.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/utils.tssrc/lib/onboard.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-channel-setup.tstest/onboard-messaging.test.ts
| required: true, | ||
| envKey: "SLACK_BOT_TOKEN", | ||
| formatPattern: "^xoxb-[A-Za-z0-9_-]+$", | ||
| formatHint: "Slack bot tokens start with 'xoxb-' (e.g. xoxb-1234-5678-abcdef).", |
Contributor
There was a problem hiding this comment.
Avoid token-like sample literals in formatHint text.
Line 22 and Line 35 examples resemble live tokens and are already triggering secret-scan findings. Use redacted placeholder shapes instead.
Proposed wording change
- formatHint: "Slack bot tokens start with 'xoxb-' (e.g. xoxb-1234-5678-abcdef).",
+ formatHint: "Slack bot tokens start with 'xoxb-' (e.g. xoxb-<workspace>-<bot>-<redacted>).",
...
- formatHint: "Slack app tokens start with 'xapp-' (e.g. xapp-1-A0000-12345-abcdef).",
+ formatHint: "Slack app tokens start with 'xapp-' (e.g. xapp-<version>-<app-id>-<team-id>-<redacted>).",Also applies to: 35-35
🤖 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/channels/slack/manifest.ts` at line 22, The formatHint
strings in the Slack channel manifest contain token-like sample literals that
trigger secret scanning; update the formatHint values (both occurrences
referenced by the formatHint property in the Slack manifest object) to use
redacted placeholders instead of realistic token patterns (e.g., replace
"xoxb-1234-5678-abcdef" style examples with something like "xoxb-REDACTED" or
"<redacted>") so the manifest still documents expected format without including
token-like samples; ensure you update both formatHint occurrences (the
formatHint property on the exported Slack manifest) consistently.
Comment on lines
+28
to
+31
| const ilinkLoginOptions = { | ||
| ...createDefaultWechatHostQrLoginOptions(), | ||
| ...options.ilinkLogin, | ||
| }; |
Contributor
There was a problem hiding this comment.
Preserve defaults when overrides contain undefined values.
On Line 28-31, spread-merging allows options.ilinkLogin fields set to undefined to clobber defaults (notably runLogin/saveCredential), which can trigger runtime failures later. Filter undefined overrides before merging.
Suggested hardening
- const ilinkLoginOptions = {
- ...createDefaultWechatHostQrLoginOptions(),
- ...options.ilinkLogin,
- };
+ const defaults = createDefaultWechatHostQrLoginOptions();
+ const overrides = Object.fromEntries(
+ Object.entries(options.ilinkLogin ?? {}).filter(([, value]) => value !== undefined),
+ ) as WechatIlinkLoginHookOptions;
+ const ilinkLoginOptions = { ...defaults, ...overrides };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const ilinkLoginOptions = { | |
| ...createDefaultWechatHostQrLoginOptions(), | |
| ...options.ilinkLogin, | |
| }; | |
| const defaults = createDefaultWechatHostQrLoginOptions(); | |
| const overrides = Object.fromEntries( | |
| Object.entries(options.ilinkLogin ?? {}).filter(([, value]) => value !== undefined), | |
| ) as WechatIlinkLoginHookOptions; | |
| const ilinkLoginOptions = { ...defaults, ...overrides }; |
🤖 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/channels/wechat/hooks/index.ts` around lines 28 - 31, The
current spread merge into ilinkLoginOptions lets properties in
options.ilinkLogin that are explicitly undefined overwrite defaults from
createDefaultWechatHostQrLoginOptions(), which can remove required flags like
runLogin or saveCredential; before merging, create a cleaned override object by
filtering out keys with undefined values from options.ilinkLogin and then spread
that cleaned object into ilinkLoginOptions (referencing ilinkLoginOptions,
createDefaultWechatHostQrLoginOptions, options.ilinkLogin, runLogin,
saveCredential to locate the code).
Comment on lines
+53
to
+55
| const hasManifestCredentials = (manifest: ChannelManifest) => | ||
| hasMessagingManifestPrimaryCredential(manifest, getMessagingToken); | ||
| const seedFromState = (includeAllExisting = false): string[] => |
Contributor
There was a problem hiding this comment.
Non-interactive seeding should require all required secrets, not just the primary one.
Line 53 currently seeds channels from hasMessagingManifestPrimaryCredential(...). In non-interactive mode (Line 63 onward), this can enable partially configured channels (e.g., primary token exists but another required secret is missing).
Suggested fix
import {
createBuiltInChannelManifestRegistry,
+ hasMessagingManifestCredentials,
getMessagingManifestAvailabilityContext,
hasMessagingManifestPrimaryCredential,
MessagingWorkflowPlanner,
resolveMessagingManifestSeed,
toMessagingAgentId,
@@
const hasManifestCredentials = (manifest: ChannelManifest) =>
hasMessagingManifestPrimaryCredential(manifest, getMessagingToken);
+ const hasAllRequiredManifestCredentials = (manifest: ChannelManifest) =>
+ hasMessagingManifestCredentials(manifest, getMessagingToken);
@@
if (isNonInteractive() || process.env.NEMOCLAW_NON_INTERACTIVE === "1") {
- const enabled = new Set(seedFromState(false));
+ const enabled = new Set(
+ resolveMessagingManifestSeed(
+ availableChannels,
+ existingChannels,
+ hasAllRequiredManifestCredentials,
+ { includeAllExisting: false },
+ ),
+ );Also applies to: 63-81
🤖 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/messaging-channel-setup.ts` around lines 53 - 55, The current
helper hasManifestCredentials uses
hasMessagingManifestPrimaryCredential(manifest, getMessagingToken), which only
checks the primary secret and allows non-interactive seeding to include
partially configured channels; update hasManifestCredentials so it validates
that all required secrets for a ChannelManifest are present (not just the
primary) and use that stricter predicate inside seedFromState and the
non-interactive branch starting around seedFromState (lines 63-81) so
non-interactive seeding only includes manifests where every required credential
is available; locate and modify the functions hasManifestCredentials and any
call sites in seedFromState to call the new “all required secrets present” check
instead of hasMessagingManifestPrimaryCredential.
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
|
Contributor
PR Review AdvisorFindings: 2 needs attention, 13 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
…nrollment' into u/sdang/3896-phase-2-messaging-enrollment
wscurran
added
enhancement: integration
Contributor
|
✨ |
10 tasks
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26458565211
|
Contributor
|
❌ Brev E2E (messaging-providers): FAILED on branch |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26497114878
|
…-3993-3994' into u/sdang/3896-phase-2-messaging-enrollment # Conflicts: # src/lib/onboard.ts # src/lib/onboard/machine/handlers/sandbox.ts # src/lib/onboard/messaging-channel-setup.test.ts # src/lib/onboard/messaging-channel-setup.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 26497114878
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26497114878
|
…g/3896-phase-2-messaging-enrollment
…g/3896-phase-2-messaging-enrollment
…nrollment' into u/sdang/3896-phase-2-messaging-enrollment
…g/3896-phase-2-messaging-enrollment
…-3993-3994' into u/sdang/3896-phase-2-messaging-enrollment # Conflicts: # src/lib/messaging/applier/setup-applier.test.ts # src/lib/messaging/compiler/workflow-planner.test.ts
…nrollment' into u/sdang/3896-phase-2-messaging-enrollment
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26632897939
|
10 tasks
…om:NVIDIA/NemoClaw into u/sdang/3896-phase-2-messaging-enrollment # Conflicts: # src/lib/actions/sandbox/policy-channel.ts # src/lib/onboard.ts # src/lib/onboard/messaging-channel-setup.test.ts # src/lib/onboard/messaging-channel-setup.ts # test/e2e/test-channels-stop-start.sh # test/e2e/test-messaging-providers.sh
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 26752754084
|
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
Migrates messaging enrollment from the legacy ad hoc onboard implementation to the manifest-driven workflow compiler path.
The PR keeps the existing operator-facing enrollment UX, but moves the source of truth for channel setup into channel manifests and registered messaging hooks. Telegram,
Discord, Slack, WeChat, and WhatsApp now declare their enrollment prompts, config inputs, credential bindings, policy needs, render targets, and setup hooks through the
manifest system.
3 key changes
messaging-channel-setup.ts, with shared manifest-style channelsTest results
OpenClaw
Discord
Telegram
WeChat
Hermes
N/A
Related Issue
Closes #4247
Changes
messaging-channel-setup.tsand shared helpers intomessaging/utils.ts.MessagingWorkflowPlannerresolves hooks from the manifest registry.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements