refactor(messaging): move build setup to manifest hooks#4949
Draft
sandl99 wants to merge 8 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: 7 needs attention, 7 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
Comment on lines
+83
to
+90
| if (render.kind === "json-fragment") { | ||
| applyJsonRender(config, render); | ||
| appliedTargets.push(render.target); | ||
| if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`); | ||
| continue; | ||
| } | ||
| applyEnvRender(envLines, render); | ||
| appliedTargets.push(render.target); |
Contributor
There was a problem hiding this comment.
Reject unknown manifest render kinds explicitly (fail-closed).
At Line 83, any non-json-fragment kind is treated as env-lines. An unexpected kind (schema drift/typo) should hard-fail instead of silently entering the env path.
Suggested fix
- if (render.kind === "json-fragment") {
+ if (render.kind === "json-fragment") {
applyJsonRender(config, render);
appliedTargets.push(render.target);
if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`);
continue;
}
- applyEnvRender(envLines, render);
- appliedTargets.push(render.target);
- if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`);
+ if (render.kind === "env-lines") {
+ applyEnvRender(envLines, render);
+ appliedTargets.push(render.target);
+ if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`);
+ continue;
+ }
+ throw new Error(
+ `Hermes manifest hook render '${render.renderId ?? render.channelId}' has unsupported kind '${String(render.kind)}'.`,
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (render.kind === "json-fragment") { | |
| applyJsonRender(config, render); | |
| appliedTargets.push(render.target); | |
| if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`); | |
| continue; | |
| } | |
| applyEnvRender(envLines, render); | |
| appliedTargets.push(render.target); | |
| if (render.kind === "json-fragment") { | |
| applyJsonRender(config, render); | |
| appliedTargets.push(render.target); | |
| if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`); | |
| continue; | |
| } | |
| if (render.kind === "env-lines") { | |
| applyEnvRender(envLines, render); | |
| appliedTargets.push(render.target); | |
| if (render.hookId) appliedHooks.push(`${render.channelId}:${render.hookId}`); | |
| continue; | |
| } | |
| throw new Error( | |
| `Hermes manifest hook render '${render.renderId ?? render.channelId}' has unsupported kind '${String(render.kind)}'.`, | |
| ); |
🤖 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/config/manifest-hooks.ts` around lines 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.
|
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
Comment on lines
+385
to
+396
| if (target.startsWith("~/.openclaw/")) { | ||
| if (agent !== "openclaw") { | ||
| throw new MessagingBuildApplierError(`Messaging render target ${target} does not match ${agent}.`); | ||
| } | ||
| return join(home, ".openclaw", target.slice("~/.openclaw/".length)); | ||
| } | ||
| if (target.startsWith("~/.hermes/")) { | ||
| if (agent !== "hermes") { | ||
| throw new MessagingBuildApplierError(`Messaging render target ${target} does not match ${agent}.`); | ||
| } | ||
| return join(home, ".hermes", target.slice("~/.hermes/".length)); | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent render-target path traversal in resolveAgentRenderTarget.
At Line [385] and Line [391], prefix-only checks allow ~/.openclaw/../... or ~/.hermes/../... to escape the agent root and write outside intended config trees during render application.
Suggested fix
function resolveAgentRenderTarget(
agent: MessagingAgentId,
target: string,
options: { readonly homeDir?: string } = {},
): string {
const home = options.homeDir ?? homedir();
+ const agentRoot = agent === "hermes" ? join(home, ".hermes") : join(home, ".openclaw");
+ const normalizedRoot = resolve(agentRoot);
if (agent === "openclaw" && target === "openclaw.json") {
- return join(home, ".openclaw", "openclaw.json");
+ return join(agentRoot, "openclaw.json");
}
+ let relativePath: string | null = null;
if (target.startsWith("~/.openclaw/")) {
if (agent !== "openclaw") {
throw new MessagingBuildApplierError(`Messaging render target ${target} does not match ${agent}.`);
}
- return join(home, ".openclaw", target.slice("~/.openclaw/".length));
+ relativePath = target.slice("~/.openclaw/".length);
}
- if (target.startsWith("~/.hermes/")) {
+ if (target.startsWith("~/.hermes/")) {
if (agent !== "hermes") {
throw new MessagingBuildApplierError(`Messaging render target ${target} does not match ${agent}.`);
}
- return join(home, ".hermes", target.slice("~/.hermes/".length));
+ relativePath = target.slice("~/.hermes/".length);
+ }
+ if (relativePath !== null) {
+ const resolvedTarget = resolve(agentRoot, relativePath);
+ if (resolvedTarget !== normalizedRoot && !resolvedTarget.startsWith(`${normalizedRoot}${sep}`)) {
+ throw new MessagingBuildApplierError(
+ `Messaging render target ${target} must stay inside ${agentRoot}.`,
+ );
+ }
+ return resolvedTarget;
}
throw new MessagingBuildApplierError(`Unsupported messaging render target ${target}.`);
}🤖 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/build/messaging-build-applier.mts` around lines 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.
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