refactor(messaging): move build setup to manifest hooks#4949
Draft
sandl99 wants to merge 14 commits into
Draft
Conversation
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:
📝 WalkthroughWalkthroughThis PR migrates NemoClaw's messaging configuration from legacy per-channel script-based logic to a unified base64-encoded manifest-driven plan system. It replaces multiple Docker build arguments with ChangesMessaging Plan System & Docker Build Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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, 5 worth checking, 1 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: 4
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.ts (1)
3383-3387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve Telegram mention policy across
channels stop/start.Line 3383 now gates
telegramConfigonactiveMessagingChannels, which excludes temporarily disabled channels. A rebuild afterchannels stop telegramwill therefore persistcurrent.telegramConfig = null, so a laterchannels start telegramloses the priorTELEGRAM_REQUIRE_MENTIONbehavior unless the operator re-exports it. This should key off the configured channel set (enabledChannels/ persisted messaging channels), not only the currently attached providers.Suggested fix
- if (activeMessagingChannels.includes("telegram")) { + const telegramConfigured = + enabledChannels?.includes("telegram") ?? activeMessagingChannels.includes("telegram"); + if (telegramConfigured) { const telegramRequireMention = computeTelegramRequireMention(); if (telegramRequireMention !== null) { telegramConfig.requireMention = telegramRequireMention; } }Based on learnings, channel state is expected to persist durably across rebuilds, and the PR objective explicitly calls out rebuild-hydration parity for Telegram config.
🤖 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 3383 - 3387, The current guard uses activeMessagingChannels to decide whether to set telegramConfig.requireMention, which drops the persisted Telegram mention policy when providers are temporarily stopped; change the check to use the persisted/configured channel set (e.g., enabledChannels or enabledMessagingChannels / persisted messaging channels) instead of activeMessagingChannels so computeTelegramRequireMention() runs whenever Telegram is configured, not only when a provider is attached; update the block around telegramConfig and computeTelegramRequireMention to key off that persisted/enable set (ensure you reference telegramConfig, computeTelegramRequireMention, and the enabledChannels/enabledMessagingChannels variable) so TELEGRAM_REQUIRE_MENTION survives channels stop/start cycles.Source: Learnings
test/generate-openclaw-config.test.ts (1)
1-4:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the stale legacy test-size budget entry to unblock CI.
The pipeline is failing because this file is now 2105 lines while the recorded legacy budget remains 2106. Please lower the corresponding budget entry to match the current line count so
codebase-growth-guardrailspasses.🤖 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/generate-openclaw-config.test.ts` around lines 1 - 4, The recorded legacy "test-size" budget entry is off by one (recorded 2106 vs actual 2105 lines) causing CI failures; update the legacy test-size budget value to 2105 to match test/generate-openclaw-config.test.ts current line count so codebase-growth-guardrails passes—locate the budget entry named "legacy test-size" (or the test-size entry used by the codebase-growth-guardrails config) and lower its numeric value from 2106 to 2105.Source: Pipeline failures
src/lib/messaging/compiler/manifest-compiler.ts (1)
333-343:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
validValuesis bypassed for config env values.At Line 337, config-resolved values return before the
validValuesgate at Line 342. That lets out-of-range config values pass manifest constraints.Suggested fix
function readInputEnvValue(input: ChannelInputSpec): MessagingSerializableValue | undefined { + const normalize = (raw: string | undefined): string | undefined => { + const normalized = raw?.replace(/\r/g, "").trim(); + if (!normalized || normalized.length === 0) return undefined; + if (input.validValues && !input.validValues.includes(normalized)) return undefined; + return normalized; + }; + if (!input.envKey) return undefined; if (input.kind === "config") { const resolved = resolveMessagingChannelConfigEnvValue(input.envKey, process.env); - if (resolved.value) return resolved.value; + const normalizedResolved = normalize(resolved.value); + if (normalizedResolved !== undefined) return normalizedResolved; } - const value = process.env[input.envKey]; - const normalized = value?.replace(/\r/g, "").trim(); - if (!normalized || normalized.length === 0) return undefined; - if (input.validValues && !input.validValues.includes(normalized)) return undefined; - return normalized; + return normalize(process.env[input.envKey]); }🤖 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/manifest-compiler.ts` around lines 333 - 343, In readInputEnvValue, config-resolved values returned early bypass the validValues check; after calling resolveMessagingChannelConfigEnvValue(input.envKey, process.env) assign its value to a local, normalize it the same way as env lookup (strip \r and trim), reject if empty, and then apply input.validValues.includes(normalized) before returning—i.e., replace the early return of resolved.value with the same normalization and validValues gating used for process.env[input.envKey] so both code paths enforce constraints (refer to readInputEnvValue, resolveMessagingChannelConfigEnvValue, and input.validValues).
🧹 Nitpick comments (3)
src/lib/actions/sandbox/rebuild.ts (1)
273-273: Run the rebuild-specific E2E suite for this behavior shift.Please run the
channels-stop-start-e2eworkflow for this rebuild-path change to validate disabled-channel persistence and post-rebuild messaging reapply behavior.As per coding guidelines, "
src/lib/actions/sandbox/rebuild.ts... 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` at line 273, The rebuildSandbox change needs its specific end-to-end validation: run the channels-stop-start-e2e workflow to exercise rebuild-specific scenarios for disabled-channel persistence and post-rebuild message reapply behavior; trigger that CI job and attach the resulting logs to this PR, ensuring the rebuildSandbox function's behavior around channel disable/persist and message reapply is confirmed by the E2E results.Source: Coding guidelines
Dockerfile (1)
413-550: Run the Dockerfile E2E subset before merge.These changes move critical build behavior into manifest hook execution at image-build time, so unit tests alone won’t fully validate runtime delivery. Please run the recommended selective nightly E2E jobs for this path.
As per coding guidelines,
Dockerfilechanges are only fully testable with a real container build and should use the recommended selective E2E workflows.🤖 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 `@Dockerfile` around lines 413 - 550, The Dockerfile change runs generate-openclaw-config.mts and run-openclaw-build-hooks.mts at build time which requires a real container build to validate; before merging, perform the Dockerfile E2E subset by building the image and running the repository's selective nightly E2E jobs that exercise build-hook and config-generation paths (specifically exercise run-openclaw-build-hooks.mts and generate-openclaw-config.mts execution), verify the produced openclaw.json and gateway token behavior in both root and non-root modes, and surface any failures back to the PR so we can fix build-hook or config generation issues before merge.Source: Coding guidelines
src/lib/messaging/compiler/engines/template.ts (1)
161-197: 🏗️ Heavy liftBreak up
resolveTemplateReferenceto reduce branching complexityThis resolver now centralizes many channel-specific branches, which makes it brittle for future additions. Split into a keyed resolver map (or per-channel resolver modules) and keep this function as a thin dispatcher.
As per coding guidelines,
**/*.{js,ts,tsx,jsx}: "Keep function complexity low in JavaScript and TypeScript code."🤖 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/engines/template.ts` around lines 161 - 197, The resolveTemplateReference function has grown many channel-specific branches; refactor it into a thin dispatcher that routes to per-channel resolver functions or a keyed resolver map (e.g., a map keyed by the top-level token like "discord", "telegramConfig", "wechatConfig", "slackConfig", "allowedIds", "proxyUrl") and move the existing logic into those handlers (extract logic currently in resolveTemplateReference for discord: discordGuilds, discordAllowedUsers, discordRequireMention, for allowedIds: resolveAllowedIdsTemplate usage, for telegramConfig: parseBoolean/stateValue, for wechatConfig: stateValue, for slackConfig: slackAllowedChannels, and proxyUrl) so resolveTemplateReference only picks the handler and returns handler(reference, context) or the default template string; ensure semantics and return types of RenderTemplateValue remain unchanged.Source: Coding guidelines
🤖 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 `@agents/hermes/config/manifest-hooks.ts`:
- Around line 83-90: The code currently treats any non-"json-fragment" render as
env-lines; update the logic in manifest-hooks.ts where render.kind is checked
(around the applyJsonRender/applyEnvRender calls) to explicitly handle allowed
kinds (e.g., "json-fragment" and "env-lines") and fail-closed for anything else:
if render.kind === "json-fragment" call applyJsonRender and push
appliedHooks/appliedTargets, else if render.kind === "env-lines" call
applyEnvRender and push appliedTargets, otherwise throw a descriptive Error (or
call the project’s error/assert utility) including the invalid render.kind and
render.target so schema drift/typos surface immediately.
In `@src/lib/messaging/compiler/engines/agent-render-engine.ts`:
- Around line 31-42: The default parameter creates an empty
MessagingHookRegistry which lacks required hooks (notably common.staticOutputs)
causing run-time failures when runMessagingHook is invoked; fix by ensuring the
passed-or-default registry contains the shared hooks before use — e.g., if hooks
is empty, register common.staticOutputs (and any other shared hooks) into the
MessagingHookRegistry instance used here (look for the hooks parameter,
MessagingHookRegistry, common.staticOutputs, renderHookForManifestEntry and
runMessagingHook) so render planning succeeds when callers omit hooks.
In `@test/e2e/docs/parity-inventory.generated.json`:
- Around line 8805-8807: The JSON entry's normalized_id is out of sync with its
text: update the "normalized_id" value for the object whose "text" is "M-W9:
Real WeChat token spliced into accounts/${WECHAT_ACCOUNT}.json — manifest seed
placeholder regression" so it reflects the manifest-seed regression semantics
(remove or replace the "seed.wechat.accounts.py" fragment and normalize to match
the text), ensuring the normalized_id string corresponds to the same source
semantics as the "text" field.
In `@test/onboard.test.ts`:
- Line 2718: Update the legacyMaxLines entry for the test file in the
test-file-size budget JSON: locate the JSON object key "test/onboard.test.ts" in
the test-file-size budget configuration and change its value from 4887 to 4879
so the legacyMaxLines matches the current file length (4879).
---
Outside diff comments:
In `@src/lib/messaging/compiler/manifest-compiler.ts`:
- Around line 333-343: In readInputEnvValue, config-resolved values returned
early bypass the validValues check; after calling
resolveMessagingChannelConfigEnvValue(input.envKey, process.env) assign its
value to a local, normalize it the same way as env lookup (strip \r and trim),
reject if empty, and then apply input.validValues.includes(normalized) before
returning—i.e., replace the early return of resolved.value with the same
normalization and validValues gating used for process.env[input.envKey] so both
code paths enforce constraints (refer to readInputEnvValue,
resolveMessagingChannelConfigEnvValue, and input.validValues).
In `@src/lib/onboard.ts`:
- Around line 3383-3387: The current guard uses activeMessagingChannels to
decide whether to set telegramConfig.requireMention, which drops the persisted
Telegram mention policy when providers are temporarily stopped; change the check
to use the persisted/configured channel set (e.g., enabledChannels or
enabledMessagingChannels / persisted messaging channels) instead of
activeMessagingChannels so computeTelegramRequireMention() runs whenever
Telegram is configured, not only when a provider is attached; update the block
around telegramConfig and computeTelegramRequireMention to key off that
persisted/enable set (ensure you reference telegramConfig,
computeTelegramRequireMention, and the enabledChannels/enabledMessagingChannels
variable) so TELEGRAM_REQUIRE_MENTION survives channels stop/start cycles.
In `@test/generate-openclaw-config.test.ts`:
- Around line 1-4: The recorded legacy "test-size" budget entry is off by one
(recorded 2106 vs actual 2105 lines) causing CI failures; update the legacy
test-size budget value to 2105 to match test/generate-openclaw-config.test.ts
current line count so codebase-growth-guardrails passes—locate the budget entry
named "legacy test-size" (or the test-size entry used by the
codebase-growth-guardrails config) and lower its numeric value from 2106 to
2105.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 413-550: The Dockerfile change runs generate-openclaw-config.mts
and run-openclaw-build-hooks.mts at build time which requires a real container
build to validate; before merging, perform the Dockerfile E2E subset by building
the image and running the repository's selective nightly E2E jobs that exercise
build-hook and config-generation paths (specifically exercise
run-openclaw-build-hooks.mts and generate-openclaw-config.mts execution), verify
the produced openclaw.json and gateway token behavior in both root and non-root
modes, and surface any failures back to the PR so we can fix build-hook or
config generation issues before merge.
In `@src/lib/actions/sandbox/rebuild.ts`:
- Line 273: The rebuildSandbox change needs its specific end-to-end validation:
run the channels-stop-start-e2e workflow to exercise rebuild-specific scenarios
for disabled-channel persistence and post-rebuild message reapply behavior;
trigger that CI job and attach the resulting logs to this PR, ensuring the
rebuildSandbox function's behavior around channel disable/persist and message
reapply is confirmed by the E2E results.
In `@src/lib/messaging/compiler/engines/template.ts`:
- Around line 161-197: The resolveTemplateReference function has grown many
channel-specific branches; refactor it into a thin dispatcher that routes to
per-channel resolver functions or a keyed resolver map (e.g., a map keyed by the
top-level token like "discord", "telegramConfig", "wechatConfig", "slackConfig",
"allowedIds", "proxyUrl") and move the existing logic into those handlers
(extract logic currently in resolveTemplateReference for discord: discordGuilds,
discordAllowedUsers, discordRequireMention, for allowedIds:
resolveAllowedIdsTemplate usage, for telegramConfig: parseBoolean/stateValue,
for wechatConfig: stateValue, for slackConfig: slackAllowedChannels, and
proxyUrl) so resolveTemplateReference only picks the handler and returns
handler(reference, context) or the default template string; ensure semantics and
return types of RenderTemplateValue remain 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: 4beb4b5f-e93c-4052-9c9a-7af56ccfdd8d
📒 Files selected for processing (56)
Dockerfileagents/hermes/Dockerfileagents/hermes/config/build-env.tsagents/hermes/config/hermes-config.tsagents/hermes/config/hermes-env.tsagents/hermes/config/manifest-hooks.tsagents/hermes/config/messaging-config.tsagents/hermes/generate-config.tsagents/hermes/policy-additions.yamlscripts/generate-openclaw-config.mtsscripts/lib/sandbox-init.shscripts/openclaw-build-messaging-plugins.pyscripts/run-openclaw-build-hooks.mtsscripts/seed-wechat-accounts.pysrc/ext/wechat/qr.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/adapters/openshell/client.tssrc/lib/adapters/openshell/runtime.tssrc/lib/messaging/applier/agent-config.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/manifest.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/compiler/engines/agent-render-engine.tssrc/lib/messaging/compiler/engines/build-step-engine.tssrc/lib/messaging/compiler/engines/template.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/messaging/hooks/common/index.tssrc/lib/messaging/hooks/common/static-outputs.tssrc/lib/messaging/hooks/common/token-paste.test.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/manifest/types.tssrc/lib/onboard.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.tssrc/lib/onboard/messaging-config.test.tssrc/lib/onboard/messaging-config.tssrc/lib/onboard/wechat-config.tssrc/lib/sandbox/build-context.tstest/e2e/docs/parity-inventory.generated.jsontest/e2e/test-messaging-providers.shtest/generate-hermes-config.test.tstest/generate-openclaw-config.test.tstest/messaging-plan-test-helper.tstest/onboard-messaging.test.tstest/onboard.test.tstest/run-openclaw-build-hooks.test.tstest/sandbox-build-context.test.tstest/sandbox-provisioning.test.tstest/seed-wechat-accounts.test.ts
💤 Files with no reviewable changes (6)
- src/lib/onboard/messaging-config.test.ts
- scripts/openclaw-build-messaging-plugins.py
- test/seed-wechat-accounts.test.ts
- scripts/seed-wechat-accounts.py
- agents/hermes/config/messaging-config.ts
- agents/hermes/config/build-env.ts
|
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. |
wscurran
added
area: messaging
Contributor
|
✨ Related open issues: |
…ort, function or class' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile (1)
559-597: Run the Dockerfile E2E subset before merge.This path changes build-time messaging hook orchestration and post-install file generation, so it should be validated with the recommended nightly E2E job subset for Dockerfile-impacting changes.
As per coding guidelines:
Dockerfile ... E2E test recommendation: cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, openclaw-tui-chat-correlation-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 `@Dockerfile` around lines 559 - 597, This change updates build-time messaging hook orchestration (the RUN node --experimental-strip-types /src/lib/messaging/applier/build/messaging-build-applier.mts calls for --phase agent-install and --phase post-agent-install and the openclaw plugins install/enable/inspect steps), so before merging run the Dockerfile-impact E2E subset: cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and openclaw-tui-chat-correlation-e2e; verify the messaging post-agent-install outputs are generated into the sandbox as expected, that openclaw plugins install/enable/inspect succeeds and registers the /nemoclaw runtime command, and that no network resolution occurred during the plugin install (confirm NPM_CONFIG_OFFLINE behavior and pruned plugin-runtime-deps artifacts).Source: Coding guidelines
agents/hermes/Dockerfile (1)
135-147: Run the Hermes E2E subset before merge.These changes affect Hermes onboarding/build hooks and post-install render/build-file application, so they should be validated with the Hermes-focused nightly jobs.
As per coding guidelines:
agents/hermes/** ... E2E test recommendation: hermes-e2e, hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e, hermes-onboard-security-posture-e2e, rebuild-hermes-e2e, rebuild-hermes-stale-base-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 `@agents/hermes/Dockerfile` around lines 135 - 147, These Dockerfile changes touch Hermes onboarding and post-install hooks (see the RUN commands invoking /src/lib/messaging/applier/build/messaging-build-applier.mts with --agent hermes --phase agent-install and --phase post-agent-install and the nemoclaw plugin copy into /sandbox/.hermes/plugins/nemoclaw/), so ensure the Hermes-focused E2E subset is executed before merging by updating CI to include the recommended jobs (hermes-e2e, hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e, hermes-onboard-security-posture-e2e, rebuild-hermes-e2e, rebuild-hermes-stale-base-e2e) in the PR/check pipeline or by scheduling a blocking nightly run that validates these changes; modify the workflow matrix or job list accordingly so these tests run against this Dockerfile change.Source: Coding guidelines
🤖 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/applier/build/messaging-build-applier.mts`:
- Around line 385-396: The current resolveAgentRenderTarget allows path
traversal via inputs like "~/.openclaw/../..." because it only checks prefixes;
fix by normalizing/ resolving the sliced subpath and validating it stays inside
the intended agent root before returning. In resolveAgentRenderTarget (variables
target, agent, home and throws MessagingBuildApplierError), compute the
agentRoot (join(home, ".openclaw") or join(home, ".hermes")), resolve the
candidate path against agentRoot and ensure the resolved path starts with
agentRoot (or that no path segment is ".."/contains upward traversal) and throw
MessagingBuildApplierError if validation fails; only return the resolved safe
path when the check passes.
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 135-147: These Dockerfile changes touch Hermes onboarding and
post-install hooks (see the RUN commands invoking
/src/lib/messaging/applier/build/messaging-build-applier.mts with --agent hermes
--phase agent-install and --phase post-agent-install and the nemoclaw plugin
copy into /sandbox/.hermes/plugins/nemoclaw/), so ensure the Hermes-focused E2E
subset is executed before merging by updating CI to include the recommended jobs
(hermes-e2e, hermes-inference-switch-e2e, hermes-discord-e2e, hermes-slack-e2e,
hermes-onboard-security-posture-e2e, rebuild-hermes-e2e,
rebuild-hermes-stale-base-e2e) in the PR/check pipeline or by scheduling a
blocking nightly run that validates these changes; modify the workflow matrix or
job list accordingly so these tests run against this Dockerfile change.
In `@Dockerfile`:
- Around line 559-597: This change updates build-time messaging hook
orchestration (the RUN node --experimental-strip-types
/src/lib/messaging/applier/build/messaging-build-applier.mts calls for --phase
agent-install and --phase post-agent-install and the openclaw plugins
install/enable/inspect steps), so before merging run the Dockerfile-impact E2E
subset: cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, and
openclaw-tui-chat-correlation-e2e; verify the messaging post-agent-install
outputs are generated into the sandbox as expected, that openclaw plugins
install/enable/inspect succeeds and registers the /nemoclaw runtime command, and
that no network resolution occurred during the plugin install (confirm
NPM_CONFIG_OFFLINE behavior and pruned plugin-runtime-deps artifacts).
🪄 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: 4959dcd2-ec8c-461d-8cd7-1045da67a1c3
📒 Files selected for processing (20)
Dockerfileagents/hermes/Dockerfileagents/hermes/generate-config.tsci/test-file-size-budget.jsonscripts/generate-openclaw-config.mtssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/compiler/engines/agent-render-engine.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/onboard.tssrc/lib/sandbox/build-context.tstest/e2e/docs/parity-inventory.generated.jsontest/fetch-guard-patch-regression.test.tstest/generate-hermes-config.test.tstest/generate-openclaw-config.test.tstest/helpers/messaging-plan-fixtures.tstest/messaging-build-applier.test.tstest/onboard-messaging.test.tstest/sandbox-build-context.test.tstest/sandbox-provisioning.test.tstest/security-c2-dockerfile-injection.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/e2e/docs/parity-inventory.generated.json
🚧 Files skipped from review as they are similar to previous changes (5)
- agents/hermes/generate-config.ts
- test/generate-hermes-config.test.ts
- src/lib/messaging/compiler/manifest-compiler.ts
- test/onboard-messaging.test.ts
- scripts/generate-openclaw-config.mts
…est-hooks # Conflicts: # ci/test-file-size-budget.json # src/lib/messaging/compiler/manifest-compiler.ts # src/lib/onboard.ts # test/onboard-messaging.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/messaging/compiler/workflow-planner.ts (1)
74-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate sandbox/agent before deriving credential availability from sandbox entry plan.
Line 171 reads
sandboxEntry.messaging.plandirectly, bypassing the schema/sandbox/agent checks already enforced byreadSandboxEntryPlan(...). This can import credential availability from a stale/mismatched plan and incorrectly skip credential enrollment during add-channel planning.Suggested fix
- credentialAvailability: mergeAvailability( - credentialAvailabilityFromPlan(existingPlan), - this.credentialAvailabilityFromSandboxEntry( - context.sandboxEntry, - [context.channelId], - ), - context.credentialAvailability, - ), + credentialAvailability: mergeAvailability( + credentialAvailabilityFromPlan(existingPlan), + this.credentialAvailabilityFromSandboxEntry(context, [context.channelId]), + context.credentialAvailability, + ), @@ - private credentialAvailabilityFromSandboxEntry( - sandboxEntry: MessagingWorkflowPlannerSandboxEntry | null | undefined, + private credentialAvailabilityFromSandboxEntry( + context: Pick< + MessagingWorkflowPlannerSandboxContext, + "sandboxName" | "agent" | "sandboxEntry" + >, channelIds: readonly MessagingChannelId[], ): MessagingCompilerCredentialAvailability | undefined { - const plan = sandboxEntry?.messaging?.plan; + const plan = readSandboxEntryPlan(context); if (!plan) return undefined;Also applies to: 167-173
🤖 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 74 - 80, credentialAvailabilityFromSandboxEntry is reading sandboxEntry.messaging.plan directly (e.g., sandboxEntry.messaging.plan) which can pull stale or mismatched plan data and bypass the validation done by readSandboxEntryPlan(...); change the call sites that pass sandboxEntry directly into credentialAvailabilityFromSandboxEntry so they first call readSandboxEntryPlan(context.sandboxEntry) (or otherwise validate/normalize the sandbox/agent via readSandboxEntryPlan) and then derive credential availability from the validated plan object, ensuring mergeAvailability(credentialAvailabilityFromPlan(...), this.credentialAvailabilityFromSandboxEntry(...), context.credentialAvailability) uses the validated plan rather than raw sandboxEntry.messaging.plan.
🧹 Nitpick comments (3)
src/lib/actions/sandbox/policy-channel.ts (1)
199-200: Run the targeted messaging/onboarding E2E suites before merge.Given policy-context refresh and conflict-path changes, run the listed Hermes/onboarding E2Es to validate end-to-end rebuild + state propagation behavior.
As per coding guidelines, onboarding and Hermes-affecting changes should run the recommended nightly E2E jobs for these paths.
Also applies to: 253-254, 1218-1219, 1358-1359, 1595-1596
🤖 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 199 - 200, Run the targeted Hermes and onboarding E2E suites before merging changes that touch policy-context refresh and conflict-path behavior; specifically trigger the onboarding and Hermes nightly E2E jobs to validate end-to-end rebuild and state propagation after changes that call refreshSandboxPolicyContextFile (and related changes around the same call sites referenced), and re-run the listed suites covering the regions around refreshSandboxPolicyContextFile to confirm no regressions in onboarding/Hermes flows.Source: Coding guidelines
Dockerfile (1)
604-669: Run the Dockerfile E2E subset for this hook-migration path.Given the build-time hook sequencing and plan transport changes, run the recommended E2E workflow subset before merge to validate real image behavior (build-time generated artifacts + runtime startup path).
As per coding guidelines, Dockerfile-layer changes should be validated with the recommended E2E jobs for container behavior.
🤖 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 `@Dockerfile` around lines 604 - 669, Run the Dockerfile E2E subset to validate the hook-migration path: exercise the build and runtime steps that involve the OpenClaw agent and NemoClaw plugin (look for RUN steps invoking /src/lib/messaging/applier/build/messaging-build-applier.mts with --phase agent-install and --phase post-agent-install, the patch script /usr/local/lib/nemoclaw/patch-openclaw-slack-deny-feedback.mts, and the openclaw plugins install/enable/inspect sequence), ensuring the build-time generated artifacts and runtime startup succeed under NPM_CONFIG_OFFLINE and the OpenClaw OTEL conditional; if failures occur, capture logs from the image build and container startup, reproduce locally with the same env vars (NEMOCLAW_OPENCLAW_OTEL, OPENCLAW_VERSION, NPM_CONFIG_OFFLINE) and iterate until the E2E subset passes before merging.Source: Coding guidelines
agents/hermes/Dockerfile (1)
144-156: Run the Hermes E2E subset for hook-phase and onboarding safety.This touches Hermes onboarding/config/render timing and post-install behavior, so validating with the Hermes-focused E2E jobs is worthwhile before merge.
As per coding guidelines,
agents/hermes/**changes should be verified with the recommended Hermes E2E suite.🤖 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 `@agents/hermes/Dockerfile` around lines 144 - 156, This change affects Hermes onboarding/render/post-agent-install hooks (see the node invocation of messaging-build-applier.mts with --agent hermes and the nemoclaw plugin copy), so before merging run the Hermes-focused E2E subset that exercises hook-phase and onboarding/post-agent-install flows (i.e., CI job that validates messaging-build-applier.mts --phase agent-install and --phase post-agent-install and plugin install behavior) and ensure those tests pass; if failures appear, reproduce locally against the same Dockerfile steps and fix timing/order or hook scripts accordingly.Source: Coding guidelines
🤖 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 2891-2905: The loop that builds
reusableMessagingProviders/reusableMessagingChannels can add "slack" when only
the SLACK_APP_TOKEN is present, bypassing the bot-token guard later; update the
loop over messagingTokenDefs so that entries with envKey === "SLACK_APP_TOKEN"
are ignored for channel population (e.g., add a guard like if (envKey ===
"SLACK_APP_TOKEN") continue before computing channel), keeping the bot-token
gate in the reusable-provider path; ensure this change references the existing
variables/methods reusableMessagingProviders, reusableMessagingChannels,
messagingTokenDefs, and providerExistsInGateway so only fully-qualified Slack
(bot-token) providers can add the "slack" channel.
---
Outside diff comments:
In `@src/lib/messaging/compiler/workflow-planner.ts`:
- Around line 74-80: credentialAvailabilityFromSandboxEntry is reading
sandboxEntry.messaging.plan directly (e.g., sandboxEntry.messaging.plan) which
can pull stale or mismatched plan data and bypass the validation done by
readSandboxEntryPlan(...); change the call sites that pass sandboxEntry directly
into credentialAvailabilityFromSandboxEntry so they first call
readSandboxEntryPlan(context.sandboxEntry) (or otherwise validate/normalize the
sandbox/agent via readSandboxEntryPlan) and then derive credential availability
from the validated plan object, ensuring
mergeAvailability(credentialAvailabilityFromPlan(...),
this.credentialAvailabilityFromSandboxEntry(...),
context.credentialAvailability) uses the validated plan rather than raw
sandboxEntry.messaging.plan.
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 144-156: This change affects Hermes
onboarding/render/post-agent-install hooks (see the node invocation of
messaging-build-applier.mts with --agent hermes and the nemoclaw plugin copy),
so before merging run the Hermes-focused E2E subset that exercises hook-phase
and onboarding/post-agent-install flows (i.e., CI job that validates
messaging-build-applier.mts --phase agent-install and --phase post-agent-install
and plugin install behavior) and ensure those tests pass; if failures appear,
reproduce locally against the same Dockerfile steps and fix timing/order or hook
scripts accordingly.
In `@Dockerfile`:
- Around line 604-669: Run the Dockerfile E2E subset to validate the
hook-migration path: exercise the build and runtime steps that involve the
OpenClaw agent and NemoClaw plugin (look for RUN steps invoking
/src/lib/messaging/applier/build/messaging-build-applier.mts with --phase
agent-install and --phase post-agent-install, the patch script
/usr/local/lib/nemoclaw/patch-openclaw-slack-deny-feedback.mts, and the openclaw
plugins install/enable/inspect sequence), ensuring the build-time generated
artifacts and runtime startup succeed under NPM_CONFIG_OFFLINE and the OpenClaw
OTEL conditional; if failures occur, capture logs from the image build and
container startup, reproduce locally with the same env vars
(NEMOCLAW_OPENCLAW_OTEL, OPENCLAW_VERSION, NPM_CONFIG_OFFLINE) and iterate until
the E2E subset passes before merging.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 199-200: Run the targeted Hermes and onboarding E2E suites before
merging changes that touch policy-context refresh and conflict-path behavior;
specifically trigger the onboarding and Hermes nightly E2E jobs to validate
end-to-end rebuild and state propagation after changes that call
refreshSandboxPolicyContextFile (and related changes around the same call sites
referenced), and re-run the listed suites covering the regions around
refreshSandboxPolicyContextFile to confirm no regressions in onboarding/Hermes
flows.
🪄 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: 92d22fe6-ab7c-4299-bd60-46c59ef752b9
📒 Files selected for processing (32)
Dockerfileagents/hermes/Dockerfileagents/hermes/config/build-env.tsagents/hermes/config/hermes-config.tsagents/hermes/generate-config.tsci/test-file-size-budget.jsonscripts/generate-openclaw-config.mtsscripts/lib/sandbox-init.shsrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/channels/discord/template-resolver.tssrc/lib/messaging/channels/index.tssrc/lib/messaging/channels/slack/template-resolver.tssrc/lib/messaging/channels/telegram/template-resolver.tssrc/lib/messaging/channels/template-resolver-utils.tssrc/lib/messaging/channels/template-resolver.tssrc/lib/messaging/channels/wechat/hooks/seed-openclaw-account.tssrc/lib/messaging/channels/wechat/template-resolver.tssrc/lib/messaging/channels/whatsapp/template-resolver.tssrc/lib/messaging/compiler/engines/agent-render-engine.tssrc/lib/messaging/compiler/engines/template.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/onboard.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/sandbox/build-context.ts
💤 Files with no reviewable changes (2)
- src/lib/messaging/channels/wechat/hooks/seed-openclaw-account.ts
- src/lib/actions/sandbox/rebuild.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/messaging/channels/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/lib/sandbox-init.sh
- src/lib/onboard/dockerfile-patch.ts
- src/lib/messaging/compiler/engines/agent-render-engine.ts
- src/lib/messaging/compiler/workflow-planner.test.ts
- src/lib/messaging/applier/setup-applier.test.ts
- agents/hermes/config/build-env.ts
- agents/hermes/config/hermes-config.ts
- scripts/generate-openclaw-config.mts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/messaging/compiler/workflow-planner.ts (1)
74-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate sandbox/agent before deriving credential availability from sandbox entry plan.
Line 171 reads
sandboxEntry.messaging.plandirectly, bypassing the schema/sandbox/agent checks already enforced byreadSandboxEntryPlan(...). This can import credential availability from a stale/mismatched plan and incorrectly skip credential enrollment during add-channel planning.Suggested fix
- credentialAvailability: mergeAvailability( - credentialAvailabilityFromPlan(existingPlan), - this.credentialAvailabilityFromSandboxEntry( - context.sandboxEntry, - [context.channelId], - ), - context.credentialAvailability, - ), + credentialAvailability: mergeAvailability( + credentialAvailabilityFromPlan(existingPlan), + this.credentialAvailabilityFromSandboxEntry(context, [context.channelId]), + context.credentialAvailability, + ), @@ - private credentialAvailabilityFromSandboxEntry( - sandboxEntry: MessagingWorkflowPlannerSandboxEntry | null | undefined, + private credentialAvailabilityFromSandboxEntry( + context: Pick< + MessagingWorkflowPlannerSandboxContext, + "sandboxName" | "agent" | "sandboxEntry" + >, channelIds: readonly MessagingChannelId[], ): MessagingCompilerCredentialAvailability | undefined { - const plan = sandboxEntry?.messaging?.plan; + const plan = readSandboxEntryPlan(context); if (!plan) return undefined;Also applies to: 167-173
🤖 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 74 - 80, credentialAvailabilityFromSandboxEntry is reading sandboxEntry.messaging.plan directly (e.g., sandboxEntry.messaging.plan) which can pull stale or mismatched plan data and bypass the validation done by readSandboxEntryPlan(...); change the call sites that pass sandboxEntry directly into credentialAvailabilityFromSandboxEntry so they first call readSandboxEntryPlan(context.sandboxEntry) (or otherwise validate/normalize the sandbox/agent via readSandboxEntryPlan) and then derive credential availability from the validated plan object, ensuring mergeAvailability(credentialAvailabilityFromPlan(...), this.credentialAvailabilityFromSandboxEntry(...), context.credentialAvailability) uses the validated plan rather than raw sandboxEntry.messaging.plan.
🧹 Nitpick comments (3)
src/lib/actions/sandbox/policy-channel.ts (1)
199-200: Run the targeted messaging/onboarding E2E suites before merge.Given policy-context refresh and conflict-path changes, run the listed Hermes/onboarding E2Es to validate end-to-end rebuild + state propagation behavior.
As per coding guidelines, onboarding and Hermes-affecting changes should run the recommended nightly E2E jobs for these paths.
Also applies to: 253-254, 1218-1219, 1358-1359, 1595-1596
🤖 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 199 - 200, Run the targeted Hermes and onboarding E2E suites before merging changes that touch policy-context refresh and conflict-path behavior; specifically trigger the onboarding and Hermes nightly E2E jobs to validate end-to-end rebuild and state propagation after changes that call refreshSandboxPolicyContextFile (and related changes around the same call sites referenced), and re-run the listed suites covering the regions around refreshSandboxPolicyContextFile to confirm no regressions in onboarding/Hermes flows.Source: Coding guidelines
Dockerfile (1)
604-669: Run the Dockerfile E2E subset for this hook-migration path.Given the build-time hook sequencing and plan transport changes, run the recommended E2E workflow subset before merge to validate real image behavior (build-time generated artifacts + runtime startup path).
As per coding guidelines, Dockerfile-layer changes should be validated with the recommended E2E jobs for container behavior.
🤖 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 `@Dockerfile` around lines 604 - 669, Run the Dockerfile E2E subset to validate the hook-migration path: exercise the build and runtime steps that involve the OpenClaw agent and NemoClaw plugin (look for RUN steps invoking /src/lib/messaging/applier/build/messaging-build-applier.mts with --phase agent-install and --phase post-agent-install, the patch script /usr/local/lib/nemoclaw/patch-openclaw-slack-deny-feedback.mts, and the openclaw plugins install/enable/inspect sequence), ensuring the build-time generated artifacts and runtime startup succeed under NPM_CONFIG_OFFLINE and the OpenClaw OTEL conditional; if failures occur, capture logs from the image build and container startup, reproduce locally with the same env vars (NEMOCLAW_OPENCLAW_OTEL, OPENCLAW_VERSION, NPM_CONFIG_OFFLINE) and iterate until the E2E subset passes before merging.Source: Coding guidelines
agents/hermes/Dockerfile (1)
144-156: Run the Hermes E2E subset for hook-phase and onboarding safety.This touches Hermes onboarding/config/render timing and post-install behavior, so validating with the Hermes-focused E2E jobs is worthwhile before merge.
As per coding guidelines,
agents/hermes/**changes should be verified with the recommended Hermes E2E suite.🤖 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 `@agents/hermes/Dockerfile` around lines 144 - 156, This change affects Hermes onboarding/render/post-agent-install hooks (see the node invocation of messaging-build-applier.mts with --agent hermes and the nemoclaw plugin copy), so before merging run the Hermes-focused E2E subset that exercises hook-phase and onboarding/post-agent-install flows (i.e., CI job that validates messaging-build-applier.mts --phase agent-install and --phase post-agent-install and plugin install behavior) and ensure those tests pass; if failures appear, reproduce locally against the same Dockerfile steps and fix timing/order or hook scripts accordingly.Source: Coding guidelines
🤖 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 2891-2905: The loop that builds
reusableMessagingProviders/reusableMessagingChannels can add "slack" when only
the SLACK_APP_TOKEN is present, bypassing the bot-token guard later; update the
loop over messagingTokenDefs so that entries with envKey === "SLACK_APP_TOKEN"
are ignored for channel population (e.g., add a guard like if (envKey ===
"SLACK_APP_TOKEN") continue before computing channel), keeping the bot-token
gate in the reusable-provider path; ensure this change references the existing
variables/methods reusableMessagingProviders, reusableMessagingChannels,
messagingTokenDefs, and providerExistsInGateway so only fully-qualified Slack
(bot-token) providers can add the "slack" channel.
---
Outside diff comments:
In `@src/lib/messaging/compiler/workflow-planner.ts`:
- Around line 74-80: credentialAvailabilityFromSandboxEntry is reading
sandboxEntry.messaging.plan directly (e.g., sandboxEntry.messaging.plan) which
can pull stale or mismatched plan data and bypass the validation done by
readSandboxEntryPlan(...); change the call sites that pass sandboxEntry directly
into credentialAvailabilityFromSandboxEntry so they first call
readSandboxEntryPlan(context.sandboxEntry) (or otherwise validate/normalize the
sandbox/agent via readSandboxEntryPlan) and then derive credential availability
from the validated plan object, ensuring
mergeAvailability(credentialAvailabilityFromPlan(...),
this.credentialAvailabilityFromSandboxEntry(...),
context.credentialAvailability) uses the validated plan rather than raw
sandboxEntry.messaging.plan.
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 144-156: This change affects Hermes
onboarding/render/post-agent-install hooks (see the node invocation of
messaging-build-applier.mts with --agent hermes and the nemoclaw plugin copy),
so before merging run the Hermes-focused E2E subset that exercises hook-phase
and onboarding/post-agent-install flows (i.e., CI job that validates
messaging-build-applier.mts --phase agent-install and --phase post-agent-install
and plugin install behavior) and ensure those tests pass; if failures appear,
reproduce locally against the same Dockerfile steps and fix timing/order or hook
scripts accordingly.
In `@Dockerfile`:
- Around line 604-669: Run the Dockerfile E2E subset to validate the
hook-migration path: exercise the build and runtime steps that involve the
OpenClaw agent and NemoClaw plugin (look for RUN steps invoking
/src/lib/messaging/applier/build/messaging-build-applier.mts with --phase
agent-install and --phase post-agent-install, the patch script
/usr/local/lib/nemoclaw/patch-openclaw-slack-deny-feedback.mts, and the openclaw
plugins install/enable/inspect sequence), ensuring the build-time generated
artifacts and runtime startup succeed under NPM_CONFIG_OFFLINE and the OpenClaw
OTEL conditional; if failures occur, capture logs from the image build and
container startup, reproduce locally with the same env vars
(NEMOCLAW_OPENCLAW_OTEL, OPENCLAW_VERSION, NPM_CONFIG_OFFLINE) and iterate until
the E2E subset passes before merging.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 199-200: Run the targeted Hermes and onboarding E2E suites before
merging changes that touch policy-context refresh and conflict-path behavior;
specifically trigger the onboarding and Hermes nightly E2E jobs to validate
end-to-end rebuild and state propagation after changes that call
refreshSandboxPolicyContextFile (and related changes around the same call sites
referenced), and re-run the listed suites covering the regions around
refreshSandboxPolicyContextFile to confirm no regressions in onboarding/Hermes
flows.
🪄 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: 92d22fe6-ab7c-4299-bd60-46c59ef752b9
📒 Files selected for processing (32)
Dockerfileagents/hermes/Dockerfileagents/hermes/config/build-env.tsagents/hermes/config/hermes-config.tsagents/hermes/generate-config.tsci/test-file-size-budget.jsonscripts/generate-openclaw-config.mtsscripts/lib/sandbox-init.shsrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/channels/discord/template-resolver.tssrc/lib/messaging/channels/index.tssrc/lib/messaging/channels/slack/template-resolver.tssrc/lib/messaging/channels/telegram/template-resolver.tssrc/lib/messaging/channels/template-resolver-utils.tssrc/lib/messaging/channels/template-resolver.tssrc/lib/messaging/channels/wechat/hooks/seed-openclaw-account.tssrc/lib/messaging/channels/wechat/template-resolver.tssrc/lib/messaging/channels/whatsapp/template-resolver.tssrc/lib/messaging/compiler/engines/agent-render-engine.tssrc/lib/messaging/compiler/engines/template.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/onboard.tssrc/lib/onboard/dockerfile-patch.test.tssrc/lib/onboard/dockerfile-patch.tssrc/lib/onboard/messaging-channel-setup.tssrc/lib/sandbox/build-context.ts
💤 Files with no reviewable changes (2)
- src/lib/messaging/channels/wechat/hooks/seed-openclaw-account.ts
- src/lib/actions/sandbox/rebuild.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/messaging/channels/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/lib/sandbox-init.sh
- src/lib/onboard/dockerfile-patch.ts
- src/lib/messaging/compiler/engines/agent-render-engine.ts
- src/lib/messaging/compiler/workflow-planner.test.ts
- src/lib/messaging/applier/setup-applier.test.ts
- agents/hermes/config/build-env.ts
- agents/hermes/config/hermes-config.ts
- scripts/generate-openclaw-config.mts
🛑 Comments failed to post (1)
src/lib/onboard.ts (1)
2891-2905:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep Slack’s bot-token gate in the reusable-provider path.
This loop can add
slacktoreusableMessagingChannelswhen only${sandboxName}-slack-appexists. That bypasses the existing guard at Lines 3311-3314, so the policy/registry can enable Slack andsandbox createwill attach only the app provider. The bridge then starts with an incomplete Slack credential set.Suggested fix
const reusableMessagingProviders: string[] = []; const reusableMessagingChannels: string[] = []; if (enabledChannels != null) { for (const { name, envKey, token } of messagingTokenDefs) { if (token) continue; const channel = envKey === "SLACK_APP_TOKEN" ? "slack" : getMessagingChannelForEnvKey(envKey); if (!channel || !enabledChannels.includes(channel)) continue; if (!providerExistsInGateway(name)) continue; reusableMessagingProviders.push(name); - if (!reusableMessagingChannels.includes(channel)) { + const canReuseChannel = + envKey !== "SLACK_APP_TOKEN" || + messagingTokenDefs.some( + (candidate) => + candidate.envKey === "SLACK_BOT_TOKEN" && + (!!candidate.token || providerExistsInGateway(candidate.name)), + ); + if (canReuseChannel && !reusableMessagingChannels.includes(channel)) { reusableMessagingChannels.push(channel); } } }🤖 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 2891 - 2905, The loop that builds reusableMessagingProviders/reusableMessagingChannels can add "slack" when only the SLACK_APP_TOKEN is present, bypassing the bot-token guard later; update the loop over messagingTokenDefs so that entries with envKey === "SLACK_APP_TOKEN" are ignored for channel population (e.g., add a guard like if (envKey === "SLACK_APP_TOKEN") continue before computing channel), keeping the bot-token gate in the reusable-provider path; ensure this change references the existing variables/methods reusableMessagingProviders, reusableMessagingChannels, messagingTokenDefs, and providerExistsInGateway so only fully-qualified Slack (bot-token) providers can add the "slack" channel.
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
Migrate messaging build setup from legacy Docker build scripts and ad hoc build args into manifest-driven hooks. OpenClaw and Hermes now consume the staged messaging plan for package installs, static render outputs, and WeChat post-agent-install behavior.
Related Issue
Fixes #4395
Changes
scripts/run-openclaw-build-hooks.mtsand wire Docker builds throughNEMOCLAW_MESSAGING_PLAN_B64.agents/hermes/config/messaging-config.ts.Type of Change
Verification
Targeted checks run:
npm run build:cli,npm run typecheck:cli,npx vitest run test/onboard-messaging.test.ts,npx vitest run src/lib/messaging/hooks/common/token-paste.test.ts,git diff --check.npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit