refactor(messaging): move build setup to manifest hooks#4949
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSwitch messaging to a manifest-driven build plan ( ChangesManifest-driven messaging migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ 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
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 3 needs attention, 9 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![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.
🧹 Nitpick comments (1)
src/lib/messaging/applier/agent-config.ts (1)
232-310: 💤 Low valueConsider extracting shared placeholder logic to reduce duplication.
The credential placeholder preservation logic here (lines 232-310) is nearly identical to the implementation in
messaging-build-applier.mts(lines 634-712). While duplication may be intentional due to different execution contexts (standalone.mtsmodule vs. library code), consider extracting this to a shared utility if both contexts can import from a common source.Minor difference:
isProviderPlaceholderForEnvKeyusesindexOfhere vs. regex in the.mtsfile. Both work correctly for valid placeholders.🤖 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/applier/agent-config.ts` around lines 232 - 310, This code duplicates placeholder-preservation logic found in messaging-build-applier.mts; extract the shared logic into a new utility module (e.g., messaging-placeholder-utils) and import it from both places: move credentialPlaceholderRules, preserveCredentialPlaceholders, isProviderPlaceholderForEnvKey, placeholderSuffixMatchesEnvKey (and getJsonPath if used by both) into the new module and export them; update both src/lib/messaging/applier/agent-config.ts and messaging-build-applier.mts to import and use those exported functions, and unify the slight behavioral difference by adopting the regex-based check for alias matching (make isProviderPlaceholderForEnvKey use the same regex approach as the .mts version) so both callers behave identically.
🤖 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.
Nitpick comments:
In `@src/lib/messaging/applier/agent-config.ts`:
- Around line 232-310: This code duplicates placeholder-preservation logic found
in messaging-build-applier.mts; extract the shared logic into a new utility
module (e.g., messaging-placeholder-utils) and import it from both places: move
credentialPlaceholderRules, preserveCredentialPlaceholders,
isProviderPlaceholderForEnvKey, placeholderSuffixMatchesEnvKey (and getJsonPath
if used by both) into the new module and export them; update both
src/lib/messaging/applier/agent-config.ts and messaging-build-applier.mts to
import and use those exported functions, and unify the slight behavioral
difference by adopting the regex-based check for alias matching (make
isProviderPlaceholderForEnvKey use the same regex approach as the .mts version)
so both callers behave identically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c637cb18-ba0c-45d0-993c-cceff0740ee3
📒 Files selected for processing (4)
src/lib/messaging/applier/agent-config.tssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/applier/setup-applier.test.tstest/messaging-build-applier.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27261238223
|
…est-hooks # Conflicts: # ci/test-file-size-budget.json
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-4949.docs.buildwithfern.com/nemoclaw |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/wechat/hooks/ilink-login.ts`:
- Around line 160-165: The function normalizeCredentialValue currently trims the
input before checking for line breaks so leading/trailing CR/LF are allowed;
change it to test the raw input string for /[\r\n]/ and throw if found, then
perform value.trim() and return the trimmed result; update the implementation in
normalizeCredentialValue to check the original parameter (not the trimmed
variable) for line breaks before trimming.
🪄 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: c0b63581-bfb2-45f4-bdba-676cf7119a63
📒 Files selected for processing (28)
Dockerfile.baseci/test-file-size-budget.jsondocs/manage-sandboxes/messaging-channels.mdxscripts/generate-openclaw-config.mtssrc/ext/wechat/qr.tssrc/lib/messaging-channel-config.tssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/template-resolver-utils.tssrc/lib/messaging/channels/wechat/hooks/ilink-login.tssrc/lib/messaging/channels/wechat/hooks/implementations.test.tssrc/lib/messaging/channels/wechat/hooks/seed-openclaw-account.tssrc/lib/messaging/channels/wechat/ilink-base-url.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/wechat/template-resolver.tssrc/lib/messaging/compiler/engines/agent-render-engine.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.test.tssrc/lib/onboard.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/state/sandbox.tstest/e2e/test-channels-stop-start.shtest/e2e/test-messaging-providers.shtest/generate-openclaw-config.test.tstest/messaging-build-applier.test.tstest/onboard.test.ts
💤 Files with no reviewable changes (7)
- test/e2e/test-channels-stop-start.sh
- Dockerfile.base
- test/e2e/test-messaging-providers.sh
- test/onboard.test.ts
- test/messaging-build-applier.test.ts
- scripts/generate-openclaw-config.mts
- test/generate-openclaw-config.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/lib/onboard/messaging-channel-setup.test.ts
- src/ext/wechat/qr.ts
- src/lib/state/sandbox.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- ci/test-file-size-budget.json
- src/lib/messaging/compiler/workflow-planner.test.ts
- src/lib/messaging/applier/setup-applier.test.ts
- src/lib/messaging/channels/manifests.test.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27268567856
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27268567856
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27270967967
|
cv
approved these changes
Jun 11, 2026
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27326674298
|
12 tasks
cv
pushed a commit
that referenced
this pull request
Jun 11, 2026
## Summary This PR narrows the `openclaw devices approve` compatibility recovery so a failed scope-upgrade approval can recover when OpenClaw replaces the original pending request with a same-device request that includes `operator.admin`. PR #4949 is not treated as the root cause: it changed the build/startup path enough to expose this latent approval replacement behavior, while the actual bug is the approval wrapper returning failure after OpenClaw leaves only the replacement request pending. ## Changes - Pass the failed approve command output into the existing post-failure recovery path. - Detect only same-device replacement requests reported by the CLI as `scope upgrade pending approval` or `pairing required`. - Match replacement request IDs with token boundaries instead of substring matching. - Persist `pending.json` and `paired.json` through same-directory atomic replaces. - Merge existing `approvedScopes` and `scopes`, approve only the expected operator scope closure, strip `operator.admin`, and keep token scopes aligned. - Keep the focused guard regression in `test/nemoclaw-start.test.ts` and ratchet its legacy size budget down after removing comment-only scaffolding. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Verification - [ ] `npx prek run --all-files` passes - [ ] `npm test` passes - [x] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [ ] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Additional verification run locally: - `bash -n scripts/nemoclaw-start.sh` - `npx @biomejs/biome check test/nemoclaw-start.test.ts ci/test-file-size-budget.json` - `npm run test-size:check` - `npx vitest --run test/nemoclaw-start.test.ts` Note: `npx prek run --files ci/test-file-size-budget.json scripts/nemoclaw-start.sh test/nemoclaw-start.test.ts` passed formatting, shellcheck, repository checks, and test-size budget, but its CLI hook also launched broader live E2E tests and failed on unrelated live sandbox/onboard cleanup/timeouts. It is not marked as passing above. --- Signed-off-by: San Dang <sdang@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * More reliable device approval handling with deterministic recovery of pairing/pending state, stricter scope allowlist enforcement, and safer replacement-selection logic. * **Tests** * Updated tests to simulate approval failure and verify guarded recovery behavior; strengthened startup/entrypoint assertions (tokens, file locks, stderr). * **Chores** * Reduced test file-size budget for a specific test. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This was referenced Jun 13, 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
5 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