refactor(messaging): finish manifest channel migration#5338
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:
📝 WalkthroughWalkthroughMessaging setup, hooks, status checks, persistence, and startup flows now derive channel behavior from manifests and runtime plans. The PR also updates Docker/build staging, onboarding helpers, and tests to use the new messaging runtime metadata and preload pipeline. ChangesMessaging manifest migration
Sequence Diagram(s)sequenceDiagram
participant Dockerfile
participant BuildApplier as messaging-build-applier.mts
participant StartScript as scripts/nemoclaw-start.sh
participant Sandbox
Dockerfile->>BuildApplier: run --phase runtime-setup
BuildApplier-->>Dockerfile: write runtime plan artifact
StartScript->>StartScript: read plan from env or artifact
StartScript->>StartScript: apply env aliases and install preloads
StartScript->>Sandbox: start with secret scans and NODE_OPTIONS
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
sandl99
added
area: messaging
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: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 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. |
sandl99
changed the base branch from
main
to
refactor/compact-messaging-persistence
sandl99
changed the base branch from
refactor/compact-messaging-persistence
to
main
|
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. |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/nemoclaw-start.sh (1)
3567-3585:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNon-root one-shot commands still bypass the preload/security pipeline.
In the non-root path,
exec "${NEMOCLAW_CMD[@]}"runs beforeinstall_messaging_runtime_preloadsandverify_messaging_runtime_secret_scans. Root mode installs both before its one-shot command branch, so the two launch modes are no longer equivalent.Suggested fix
export_gateway_token write_messaging_runtime_preload_plan write_runtime_shell_env ensure_runtime_shell_env_shim lock_rc_files "$_SANDBOX_HOME" || true # Apply manifest-declared runtime env aliases before any child inherits the # env. This covers both one-shot commands and the gateway launch. apply_messaging_runtime_env_aliases + install_messaging_runtime_preloads + verify_messaging_runtime_secret_scans if [ ${`#NEMOCLAW_CMD`[@]} -gt 0 ]; then exec "${NEMOCLAW_CMD[@]}" fi @@ - install_messaging_runtime_preloads - verify_messaging_runtime_secret_scansAs per coding guidelines,
scripts/nemoclaw-start.shchanges affect every sandbox boot and are invisible to unit tests.🤖 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 `@scripts/nemoclaw-start.sh` around lines 3567 - 3585, The non-root one-shot branch executes exec "${NEMOCLAW_CMD[@]}" before running install_messaging_runtime_preloads and verify_messaging_runtime_secret_scans, so non-root one-shot commands skip the preload/security pipeline; move the calls to install_messaging_runtime_preloads and verify_messaging_runtime_secret_scans to run before the conditional that checks NEMOCLAW_CMD (i.e., ensure install_messaging_runtime_preloads and verify_messaging_runtime_secret_scans are invoked prior to the exec "${NEMOCLAW_CMD[@]}" branch) so both root and non-root one-shot launches run the same preload and secret-scan steps while preserving the immediate exec behavior for one-shot commands.Source: Coding guidelines
🧹 Nitpick comments (6)
src/lib/channel-runtime-status.test.ts (1)
153-156: ⚡ Quick winKeep the token assertions tied to the
greppattern.These checks now pass as long as each token appears anywhere in the generated script, so a regression in the actual
grep -iwoEalternation could still slip through. Please bind the expectation to the grep expression itself instead of checking the substrings independently.Possible tightening
- expect(script).toContain("grep -iwoE '"); - for (const token of ["telegram", "discord", "slack", "whatsapp", "wechat", "openclaw-weixin"]) { - expect(script).toContain(token); - } + expect(script).toMatch( + /grep -iwoE '.*telegram.*discord.*slack.*whatsapp.*wechat.*openclaw-weixin.*'/s, + );🤖 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/channel-runtime-status.test.ts` around lines 153 - 156, The test currently checks tokens separately, which allows them to appear anywhere; instead assert that the exact grep alternation is present by verifying the generated script contains the full "grep -iwoE '...'" pattern with tokens joined by | (e.g., check that script includes "grep -iwoE 'telegram|discord|slack|whatsapp|wechat|openclaw-weixin'"); update the assertions around the variable script (and any helper that builds the pattern) to compare against that single grep expression rather than separate token contains checks.src/lib/inventory/index.test.ts (1)
488-495: ⚡ Quick winAssert that the custom overlap message is actually used.
This mock now adds a manifest-supplied
message, but the assertion still passes on generic Slack conflict wording.showStatusCommand()could ignoremessageentirely and this test would still stay green. Please assert the rendered line comes from the templated message path.Possible tightening
- expect( - lines.some((l) => - l.includes("'alice' and 'bob' both have Slack Socket Mode enabled on the same gateway"), - ), - ).toBe(true); + expect(lines).toContain( + " ⚠ 'alice' and 'bob' both have Slack Socket Mode enabled on the same gateway; only one sandbox can receive Slack Socket Mode events unless the gateway supports multiplexing.", + );Also applies to: 511-515
🤖 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/inventory/index.test.ts` around lines 488 - 495, Update the test so it verifies that showStatusCommand actually uses the manifest-supplied templated message from the findMessagingOverlaps mock instead of falling back to the generic wording: locate the mocked function findMessagingOverlaps in src/lib/inventory/index.test.ts and change the assertion(s) that currently match generic Slack conflict text to assert the rendered output contains the exact templated string you added (e.g., the "'{first}' and '{second}' both have Slack Socket Mode enabled..." message) so the test fails if showStatusCommand ignores the message field; apply the same tighter assertion for the second case mentioned around lines 511-515.src/lib/messaging/channels/metadata.ts (1)
66-69: ⚡ Quick winRename
listBuiltInMessagingChannelManifestsor stop honoringoptions.manifests.This helper returns arbitrary caller-supplied manifests when
options.manifestsis set, so the exported name is no longer accurate. That makes the public API misleading for callers reading the barrel surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/messaging/channels/metadata.ts` around lines 66 - 69, The exported function listBuiltInMessagingChannelManifests is misleading because it returns caller-supplied manifests when options.manifests is provided; either stop honoring options.manifests (so it truly only returns built-ins) or rename the function to reflect that it honors caller manifests. Fix by either: 1) change the implementation of listBuiltInMessagingChannelManifests to ignore options.manifests and call selectManifests with an options object that does not forward manifests (ensuring only built-in manifests are returned), or 2) rename the function (and update all references/exports) to something like listMessagingChannelManifests to indicate it may include supplied manifests; update any barrel exports and docs accordingly. Ensure you update or preserve all call-sites that reference listBuiltInMessagingChannelManifests or its replacement.test/sandbox-provider-cleanup.test.ts (1)
56-64: ⚡ Quick winMake this detach-call assertion order-insensitive.
The test name only cares that every suffix is covered, but this comparison also locks in the current
Setinsertion order. A harmless reorder ofSANDBOX_PROVIDER_SUFFIXESwill fail the test without changing behavior.Proposed fix
- expect(detachCalls).toEqual( - SANDBOX_PROVIDER_SUFFIXES.map((suffix) => [ - "sandbox", - "provider", - "detach", - "spark-nemo", - `spark-nemo-${suffix}`, - ]), - ); + expect(detachCalls.map((argv) => argv.join(" ")).sort()).toEqual( + [...SANDBOX_PROVIDER_SUFFIXES] + .map((suffix) => + [ + "sandbox", + "provider", + "detach", + "spark-nemo", + `spark-nemo-${suffix}`, + ].join(" "), + ) + .sort(), + );🤖 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/sandbox-provider-cleanup.test.ts` around lines 56 - 64, The detach-call assertion is order-sensitive; change it to compare the collections without relying on insertion order by converting both actual detachCalls and the expected SANDBOX_PROVIDER_SUFFIXES-derived arrays into order-insensitive sets (e.g., map each sub-array to a stable string representation or sort by the suffix and then compare as Sets) so the test verifies presence of every expected ["sandbox","provider","detach","spark-nemo",`spark-nemo-${suffix}`] tuple regardless of order; update the assertion that references detachCalls and SANDBOX_PROVIDER_SUFFIXES accordingly.scripts/lib/sandbox-init.sh (1)
779-817: Please run the sandbox entrypoint E2Es for this change.This path executes on every sandbox boot and is not meaningfully covered by unit tests, so I’d validate it with the entrypoint-focused jobs before merge:
gh workflow run nightly-e2e.yaml --ref <branch> -f jobs=sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e,openclaw-slack-pairing-e2eAs per coding guidelines, changes to
scripts/lib/sandbox-init.shshould be verified withsandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e, andopenclaw-slack-pairing-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 `@scripts/lib/sandbox-init.sh` around lines 779 - 817, This change touches the sandbox boot path in scripts/lib/sandbox-init.sh (functions configure_messaging_channels and read_messaging_plan_channels which read NEMOCLAW_MESSAGING_PLAN_B64), so run the sandbox entrypoint E2E suites before merging: trigger the nightly E2E workflow for the branch and validate jobs sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, and openclaw-slack-pairing-e2e to ensure the messaging-channel parsing and startup behavior are exercised end-to-end.Source: Coding guidelines
src/lib/onboard/sandbox-messaging-preflight.test.ts (1)
41-52: ⚡ Quick winUse the exported Slack hook handler constant in this fixture.
This test hardcodes
"slack.socketModeGatewayConflict"even though the same contract is already imported viaSLACK_SOCKET_MODE_GATEWAY_CONFLICT_HOOK_HANDLER_IDinsrc/lib/onboard/messaging-conflict-guard.test.ts. Reusing the shared symbol here keeps the fixture aligned with the production hook registry and avoids stale literals on future renames.🤖 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/sandbox-messaging-preflight.test.ts` around lines 41 - 52, Replace the hardcoded hook handler string "slack.socketModeGatewayConflict" in the test fixture with the shared exported constant SLACK_SOCKET_MODE_GATEWAY_CONFLICT_HOOK_HANDLER_ID; add an import for SLACK_SOCKET_MODE_GATEWAY_CONFLICT_HOOK_HANDLER_ID at the top of src/lib/onboard/sandbox-messaging-preflight.test.ts and use that symbol where the handler value is currently set so the fixture stays in sync with the production hook registry.
🤖 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 `@scripts/nemoclaw-start.sh`:
- Around line 1498-1514: clean_env_alias currently validates envAliases[*].match
with Python re but apply_messaging_runtime_env_aliases uses Bash ERE, so change
validation in clean_env_alias to verify the pattern is valid under Bash ERE
(e.g., run a lightweight Bash test via subprocess that attempts a dummy [[ "" =~
"<pattern>" ]] or otherwise validate against POSIX/Bash ERE rules) and fail with
a clear message if Bash rejects it; and reorder the runtime pipeline so non-root
execution runs install_messaging_runtime_preloads and
verify_messaging_runtime_secret_scans before doing the exec in
apply_messaging_runtime_env_aliases/where non-root exec is triggered (move the
preload + secret-scan calls to precede the non-root exec path) to match
root-mode ordering.
In `@src/lib/actions/sandbox/policy-channel.ts`:
- Around line 491-495: The code calls createMessagingPreEnableHookInputs with
currentGatewayName always set to BASE_GATEWAY_NAME; instead, read the recorded
gateway name from the current sandbox registry entry (use the registry entry for
sandboxName from registryEntries) and pass that as currentGatewayName, falling
back to BASE_GATEWAY_NAME only if the sandbox entry has no gateway field; update
the additionalInputs call to use the sandbox's gateway value (from the registry
entry for sandboxName) rather than the hardcoded BASE_GATEWAY_NAME.
In `@src/lib/messaging/applier/hook-phases.ts`:
- Around line 66-77: The loop currently only skips the failing request when
requestWithInputs.onFailure === "skip-channel"; change it to stop all later
hooks for that channel by introducing a Set (e.g., skippedChannels) outside the
loop, check at the top of the for-loop and continue if
skippedChannels.has(request.channelId), and when catching an error for a request
with onFailure === "skip-channel" add requestWithInputs.channelId to
skippedChannels and record the skipped hook via
skippedHooks.push(formatHookKey(requestWithInputs)) before continuing; ensure
you reference hookRequests, requestWithInputs, requestWithInputs.onFailure,
skippedHooks, formatHookKey, and request.channelId in your changes.
In `@src/lib/messaging/channels/metadata.ts`:
- Around line 299-304: The code currently normalizes an omitted hook.agents to
an empty array, losing the original “all agents” semantics; update the object
returned in listMessagingPackageInstallSpecs() (the creation of
MessagingPackageInstallMetadata) to preserve that meaning by setting agents:
hook.agents ?? manifest.supportedAgents (or alternatively keep agents undefined
in the shape so downstream can treat undefined as “all agents”); ensure callers
that use hookTargetsAgent() continue to see undefined for universal installs or
that manifest.supportedAgents is supplied so semantics remain correct (reference
hook.agents, manifest.supportedAgents, packageInstallValue(value),
hookTargetsAgent(), and MessagingPackageInstallMetadata).
- Around line 240-260: Both helpers currently use Object.fromEntries on
listMessagingPolicyPresetMetadata() so later presets with the same
preset.presetName overwrite earlier ones; change getMessagingPolicyKeyAliases to
accumulate/merge entries by preset.presetName: for each preset combine
preset.policyKeys and all values from preset.agentPolicyKeys (flatten), append
to any existing list for that name, then run uniqueStrings once to dedupe before
returning the final map. Similarly change
getMessagingPolicyPresetValidationWarnings to concatenate
preset.validationWarningLines for duplicate preset.presetName (preserve order),
dedupe if needed, and return the merged map; use
listMessagingPolicyPresetMetadata() as the source and reference uniqueStrings,
preset.presetName, preset.policyKeys, preset.agentPolicyKeys, and
preset.validationWarningLines when implementing the merge.
In `@src/lib/messaging/status-outputs.ts`:
- Around line 38-55: The manifest list needs to be filtered by
manifest.supportedAgents when an agent is provided so manifests that don't
support the agent cannot contribute outputs; update
collectBuiltInMessagingStatusOutputs and/or collectMessagingStatusOutputs to
check manifest.supportedAgents (in addition to hook.agents) and skip manifests
where options.agent is set and not included in manifest.supportedAgents before
iterating manifest.hooks and hook.outputs; ensure you keep the existing per-hook
hook.agents check but apply the manifest-level filter first using the
createBuiltInChannelManifestRegistry().list() result (or inside
collectMessagingStatusOutputs) to locate the correct manifests to process.
In `@src/lib/onboard/docker-gpu-patch.ts`:
- Around line 531-535: The default for dockerDriverGateway is wrong: change the
fallback so dockerDriverGateway is derived from either linux platform or the WSL
flag; update the assignment of dockerDriverGateway (currently: const
dockerDriverGateway = options.dockerDriverGateway ?? platform === "linux") to
use options.dockerDriverGateway ?? (platform === "linux" || dockerDesktopWsl) so
that the dockerDesktopWsl true path enables the gateway when callers omit
dockerDriverGateway; keep the rest of the conditional using
config.sandboxGpuEnabled, platform/dockerDesktopWsl and dockerDriverGateway
unchanged.
In `@src/lib/onboard/messaging-conflict-guard.ts`:
- Around line 133-167: The catch currently treats every failure from
MessagingSetupApplier.applyPreEnableChecks as a user-overridable conflict;
change it to only handle the expected conflict/error shape and rethrow any other
errors so real infrastructure/programming failures surface. Concretely, inspect
the caught error from the applyPreEnableChecks call (the catch around
MessagingSetupApplier.applyPreEnableChecks) for the conflict-specific
fingerprint your hooks use (e.g., a property like error.type ===
"MessagingConflict" or error.isOverridableConflict or error.code ===
"CONFLICT"); only when that predicate is true run the existing logging,
deps.promptContinue flow and the abort/deps.error branches; otherwise
immediately rethrow the error so callers (or CI) see the original exception.
Ensure references to runMessagingHook, deps.promptContinue, deps.error, and
abort remain unchanged and only the catch behavior is gated by the conflict
check.
In `@src/lib/onboard/policy-presets.ts`:
- Around line 46-76: The code collapses multiple messaging credentials per
channel into a single env key via credentialsByChannel, causing preset
auto-detection to miss additional credentials; change credentialsByChannel to
collect all providerEnvKey values (e.g., Map<string, string[]> populated from
listMessagingCredentialMetadata()), and then when iterating presets from
listMessagingPolicyPresetMetadata() call maybeSuggestMessagingPreset for each
env key for that channel (or adjust maybeSuggestMessagingPreset to accept and
check an array of env keys), ensuring you still call getCredential(envKey) /
process.env[envKey] for each entry and preserve the existing TTY logging
behavior.
In `@src/lib/onboard/sandbox-build-patch-config.ts`:
- Around line 23-35: prepareSandboxBuildPatchConfig currently returns
messagingChannelConfig directly from readMessagingChannelConfigFromEnv, which
allows stale/unconfigured env keys to be baked in; fix this by enforcing the
manifest-backed allowlist: obtain the configuredMessagingChannels (either from a
new input param or via a provided dependency like
deps.readConfiguredMessagingChannelsFromManifest) and then filter the returned
messagingChannelConfig so only channels present in configuredMessagingChannels
are included before returning the SandboxBuildPatchConfig; update
PrepareSandboxBuildPatchConfigInput and call sites or add the deps helper as
needed and ensure you reference prepareSandboxBuildPatchConfig,
readMessagingChannelConfigFromEnv (or deps.readMessagingChannelConfigFromEnv),
and configuredMessagingChannels in your changes.
In `@src/lib/onboard/sandbox-create-plan.ts`:
- Around line 111-120: getPrimaryCredentialEnvKeys currently picks the first
providerEnvKey per channel by iterating listMessagingCredentialMetadata(), which
relies on unstable iteration order and can drop valid channels in
resolveActiveMessagingChannels/activeMessagingChannels; change
getPrimaryCredentialEnvKeys to select the primary credential deterministically:
prefer an explicit marker (e.g., metadata.isPrimary) on each credential and
choose that providerEnvKey for the channel, and if no marker exists fall back to
a stable tie-breaker (e.g., sort credentials for the channel by a stable field
like providerEnvKey or credentialId and pick the first), ensuring you reference
listMessagingCredentialMetadata(), getPrimaryCredentialEnvKeys(), and
resolveActiveMessagingChannels when making the change so the returned envKeys
are deterministic and stable.
In `@src/lib/sandbox/channels.ts`:
- Around line 118-143: The metadata from configFieldMetadata(manifest) is being
omitted for in-sandbox-qr channels; update the construction so
configFieldMetadata(manifest) is always merged into the ChannelBase (or at least
included in the in-sandbox-qr return) instead of conditionally excluding it when
manifest.auth.mode === "in-sandbox-qr". Specifically, change the spread that
currently uses ...(manifest.auth.mode === "in-sandbox-qr" ? {} :
configFieldMetadata(manifest)) so that configFieldMetadata(manifest) is applied
unconditionally (or added to the object returned inside the in-sandbox-qr
branch), ensuring getChannelDef/ChannelBase still expose userIdEnvKey/label/help
for in-sandbox-qr channels.
In `@src/lib/status-command-deps.ts`:
- Around line 29-30: STATUS_OUTPUTS is currently unfiltered and causes
duplicate/incorrect warnings; modify the code so the sandbox/entry agent is
threaded into checkMessagingBridgeHealth() and findMessagingOverlaps() and use
that agent to filter outputs returned by collectBuiltInMessagingStatusOutputs()
(or replace the global STATUS_OUTPUTS with a per-agent filtered set) before
passing them into those helpers; specifically locate STATUS_OUTPUTS and replace
its global usage with a call that accepts an agent identifier (from the
sandbox/entry context) and returns only outputs where output.agent === agent (or
matching agent predicate), then update checkMessagingBridgeHealth() and
findMessagingOverlaps() to accept the agent param and consume the filtered
outputs instead of the unscoped STATUS_OUTPUTS.
In `@test/sandbox-init.test.ts`:
- Around line 866-869: The test for "no messaging plan" can inherit
NEMOCLAW_MESSAGING_PLAN_B64 from the parent environment via runWithLib; update
the test so the env passed into runWithLib explicitly unsets that variable
(e.g., include NEMOCLAW_MESSAGING_PLAN_B64: undefined or remove it) before
invoking configure_messaging_channels so the path is hermetic and always
exercises the "no messaging plan" branch in
runWithLib/configure_messaging_channels.
---
Outside diff comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 3567-3585: The non-root one-shot branch executes exec
"${NEMOCLAW_CMD[@]}" before running install_messaging_runtime_preloads and
verify_messaging_runtime_secret_scans, so non-root one-shot commands skip the
preload/security pipeline; move the calls to install_messaging_runtime_preloads
and verify_messaging_runtime_secret_scans to run before the conditional that
checks NEMOCLAW_CMD (i.e., ensure install_messaging_runtime_preloads and
verify_messaging_runtime_secret_scans are invoked prior to the exec
"${NEMOCLAW_CMD[@]}" branch) so both root and non-root one-shot launches run the
same preload and secret-scan steps while preserving the immediate exec behavior
for one-shot commands.
---
Nitpick comments:
In `@scripts/lib/sandbox-init.sh`:
- Around line 779-817: This change touches the sandbox boot path in
scripts/lib/sandbox-init.sh (functions configure_messaging_channels and
read_messaging_plan_channels which read NEMOCLAW_MESSAGING_PLAN_B64), so run the
sandbox entrypoint E2E suites before merging: trigger the nightly E2E workflow
for the branch and validate jobs sandbox-survival-e2e, sandbox-operations-e2e,
cloud-e2e, and openclaw-slack-pairing-e2e to ensure the messaging-channel
parsing and startup behavior are exercised end-to-end.
In `@src/lib/channel-runtime-status.test.ts`:
- Around line 153-156: The test currently checks tokens separately, which allows
them to appear anywhere; instead assert that the exact grep alternation is
present by verifying the generated script contains the full "grep -iwoE '...'"
pattern with tokens joined by | (e.g., check that script includes "grep -iwoE
'telegram|discord|slack|whatsapp|wechat|openclaw-weixin'"); update the
assertions around the variable script (and any helper that builds the pattern)
to compare against that single grep expression rather than separate token
contains checks.
In `@src/lib/inventory/index.test.ts`:
- Around line 488-495: Update the test so it verifies that showStatusCommand
actually uses the manifest-supplied templated message from the
findMessagingOverlaps mock instead of falling back to the generic wording:
locate the mocked function findMessagingOverlaps in
src/lib/inventory/index.test.ts and change the assertion(s) that currently match
generic Slack conflict text to assert the rendered output contains the exact
templated string you added (e.g., the "'{first}' and '{second}' both have Slack
Socket Mode enabled..." message) so the test fails if showStatusCommand ignores
the message field; apply the same tighter assertion for the second case
mentioned around lines 511-515.
In `@src/lib/messaging/channels/metadata.ts`:
- Around line 66-69: The exported function listBuiltInMessagingChannelManifests
is misleading because it returns caller-supplied manifests when
options.manifests is provided; either stop honoring options.manifests (so it
truly only returns built-ins) or rename the function to reflect that it honors
caller manifests. Fix by either: 1) change the implementation of
listBuiltInMessagingChannelManifests to ignore options.manifests and call
selectManifests with an options object that does not forward manifests (ensuring
only built-in manifests are returned), or 2) rename the function (and update all
references/exports) to something like listMessagingChannelManifests to indicate
it may include supplied manifests; update any barrel exports and docs
accordingly. Ensure you update or preserve all call-sites that reference
listBuiltInMessagingChannelManifests or its replacement.
In `@src/lib/onboard/sandbox-messaging-preflight.test.ts`:
- Around line 41-52: Replace the hardcoded hook handler string
"slack.socketModeGatewayConflict" in the test fixture with the shared exported
constant SLACK_SOCKET_MODE_GATEWAY_CONFLICT_HOOK_HANDLER_ID; add an import for
SLACK_SOCKET_MODE_GATEWAY_CONFLICT_HOOK_HANDLER_ID at the top of
src/lib/onboard/sandbox-messaging-preflight.test.ts and use that symbol where
the handler value is currently set so the fixture stays in sync with the
production hook registry.
In `@test/sandbox-provider-cleanup.test.ts`:
- Around line 56-64: The detach-call assertion is order-sensitive; change it to
compare the collections without relying on insertion order by converting both
actual detachCalls and the expected SANDBOX_PROVIDER_SUFFIXES-derived arrays
into order-insensitive sets (e.g., map each sub-array to a stable string
representation or sort by the suffix and then compare as Sets) so the test
verifies presence of every expected
["sandbox","provider","detach","spark-nemo",`spark-nemo-${suffix}`] tuple
regardless of order; update the assertion that references detachCalls and
SANDBOX_PROVIDER_SUFFIXES accordingly.
🪄 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: e6091b90-6d48-4a69-abd2-9a3023b24d94
📒 Files selected for processing (94)
ci/test-file-size-budget.jsonscripts/install.shscripts/lib/sandbox-init.shscripts/nemoclaw-start.shsrc/lib/actions/sandbox/channel-status.tssrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/actions/sandbox/telegram-channel-bridge-verification.test.tssrc/lib/actions/sandbox/telegram-channel-bridge-verification.tssrc/lib/agent/runtime-hermes-secret-boundary-behavioural.test.tssrc/lib/channel-runtime-status.test.tssrc/lib/channel-runtime-status.tssrc/lib/credentials/command-support.tssrc/lib/credentials/store.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging-channel-config.test.tssrc/lib/messaging-channel-config.tssrc/lib/messaging/MIGRATION.mdsrc/lib/messaging/applier/conflict-detection/manifest-metadata.tssrc/lib/messaging/applier/hook-phases.test.tssrc/lib/messaging/applier/hook-phases.tssrc/lib/messaging/applier/index.tssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/applier/setup-applier.tssrc/lib/messaging/channels/built-ins.tssrc/lib/messaging/channels/discord/manifest.tssrc/lib/messaging/channels/index.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/metadata.test.tssrc/lib/messaging/channels/metadata.tssrc/lib/messaging/channels/slack/hooks/index.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.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/manifest-compiler.test.tssrc/lib/messaging/diagnostics.test.tssrc/lib/messaging/diagnostics.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/messaging/status-outputs.tssrc/lib/onboard.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/initial-policy.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/machine/handlers/sandbox.test.tssrc/lib/onboard/machine/handlers/sandbox.tssrc/lib/onboard/messaging-config.test.tssrc/lib/onboard/messaging-config.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/onboard/messaging-credentials.tssrc/lib/onboard/messaging-policy-presets.tssrc/lib/onboard/messaging-prep.test.tssrc/lib/onboard/messaging-prep.tssrc/lib/onboard/messaging-reuse.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/sandbox-build-patch-config.test.tssrc/lib/onboard/sandbox-build-patch-config.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/onboard/sandbox-messaging-preflight.test.tssrc/lib/onboard/sandbox-provider-cleanup.tssrc/lib/onboard/wechat-config.tssrc/lib/policy/index.tssrc/lib/sandbox/channels.test.tssrc/lib/sandbox/channels.tssrc/lib/security/redact.tssrc/lib/status-command-deps.test.tssrc/lib/status-command-deps.tstest/channels-add-preset.test.tstest/cli/connect-recovery.test.tstest/cli/destroy-detach-order.test.tstest/cli/doctor-gateway-token.test.tstest/cli/logs.test.tstest/cli/maintenance-command.test.tstest/cli/snapshot-shields.test.tstest/destroy-cleanup-sandbox-services.test.tstest/e2e-scenario/support-tests/e2e-fixture-context.test.tstest/e2e-scenario/support-tests/e2e-shell-supervisor.test.tstest/nemoclaw-start-slack-runtime.test.tstest/nemoclaw-start.test.tstest/onboard-policy-suggestions.test.tstest/sandbox-init.test.tstest/sandbox-provider-cleanup.test.tstest/whatsapp-qr-compact.test.ts
💤 Files with no reviewable changes (6)
- src/lib/actions/sandbox/telegram-channel-bridge-verification.test.ts
- test/e2e-scenario/support-tests/e2e-shell-supervisor.test.ts
- src/lib/actions/sandbox/telegram-channel-bridge-verification.ts
- src/lib/onboard/wechat-config.ts
- src/lib/onboard/machine/core-flow-phases.test.ts
- src/lib/onboard/machine/handlers/sandbox.ts
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/metadata.test.ts`:
- Around line 184-187: The helper manifestWithPreset currently types its preset
parameter as NonNullable<ChannelManifest["policyPresets"]>[number], which
prevents callers passing a string literal; change the signature of
manifestWithPreset to accept both forms by declaring the second parameter as
ChannelPolicyPresetReference (or explicitly string | ChannelPolicyPresetSpec)
and update any imports/types used so the function can accept either a preset
name string or a full ChannelPolicyPresetSpec object; keep the function body
unchanged so it works with both variants.
🪄 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: f7b5b8d9-2876-4cd0-be5f-af7c011bc457
📒 Files selected for processing (35)
scripts/nemoclaw-start.shsrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging-channel-config.test.tssrc/lib/messaging/applier/hook-phases.test.tssrc/lib/messaging/applier/hook-phases.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/metadata.test.tssrc/lib/messaging/channels/metadata.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/hooks/errors.tssrc/lib/messaging/hooks/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/status-outputs.test.tssrc/lib/messaging/status-outputs.tssrc/lib/onboard/docker-gpu-patch-wsl.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/sandbox-build-patch-config.test.tssrc/lib/onboard/sandbox-build-patch-config.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/sandbox/channels.test.tssrc/lib/sandbox/channels.tssrc/lib/status-command-deps.tstest/nemoclaw-start-runtime-env-alias.test.tstest/nemoclaw-start.test.tstest/onboard-policy-suggestions.test.tstest/sandbox-init.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/messaging/hooks/index.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- src/lib/messaging-channel-config.test.ts
- src/lib/inventory/index.test.ts
- src/lib/onboard/sandbox-create-plan.ts
- src/lib/sandbox/channels.test.ts
- src/lib/onboard/policy-presets.ts
- src/lib/inventory/index.ts
- src/lib/actions/sandbox/policy-channel-conflict.test.ts
- src/lib/onboard/sandbox-build-patch-config.ts
- src/lib/messaging/applier/hook-phases.test.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.ts
- src/lib/actions/sandbox/doctor.ts
- src/lib/messaging/manifest/types.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.ts
- src/lib/messaging/status-outputs.ts
- src/lib/onboard/sandbox-build-patch-config.test.ts
- src/lib/sandbox/channels.ts
- src/lib/messaging/channels/manifests.test.ts
- src/lib/messaging/applier/hook-phases.ts
- src/lib/actions/sandbox/policy-channel.ts
- src/lib/onboard/messaging-conflict-guard.ts
- test/nemoclaw-start.test.ts
- scripts/nemoclaw-start.sh
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…annel-manifest-stragglers # Conflicts: # src/lib/actions/sandbox/doctor.ts # src/lib/actions/sandbox/policy-channel-conflict.test.ts # src/lib/actions/sandbox/policy-channel.ts # src/lib/actions/sandbox/snapshot.ts # src/lib/messaging/applier/host-state-applier.ts # src/lib/messaging/plan-validation.ts # src/lib/sandbox/channels.ts # src/lib/security/redact.ts # test/cli/doctor-gateway-token.test.ts
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 44%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile (1)
529-533: Run the Docker-focused E2E subset before merge.This updates baked runtime behavior (preload packaging + healthcheck liveness fallback), so validate it with the recommended
nightly-e2ejob subset on this branch.As per coding guidelines, Dockerfile changes are only reliably validated with real container build/runtime and the listed E2E suites.
Also applies to: 1091-1106
🤖 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 529 - 533, This change touches Dockerfile runtime packaging (the COPY of preload JS files) and liveness behavior; before merging, run the Docker-focused E2E subset (the nightly-e2e job subset) against this branch to validate container build and runtime behavior—specifically confirm the preloads are present in the container and the healthcheck liveness fallback works—then only merge once those E2E runs pass.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/compiler/workflow-planner.ts`:
- Around line 107-111: The three builders buildChannelStopPlanFromSandboxEntry,
buildChannelStartPlanFromSandboxEntry, and buildRebuildPlanFromSandboxEntry
currently call setPlanChannelDisabled / setPlanDisabledChannels but do not
filter plan.runtimeSetup, so hydrateDerivedSandboxMessagingPlanFields can keep
stale runtimeSetup entries; update those three functions to explicitly filter
plan.runtimeSetup after modifying disabledChannels (same approach used in
removePlanChannel) so runtimeSetup only contains entries for channels where
channel.active && !channel.disabled, or alternatively ensure the downstream
applier that consumes runtimeSetup.nodePreloads / envAliases / secretScans
always re-gates by channel.active && !channel.disabled; pick one fix and
implement it consistently (prefer adding the runtimeSetup filter in the three
builder functions to mirror removePlanChannel and avoid relying on downstream
behavior).
---
Nitpick comments:
In `@Dockerfile`:
- Around line 529-533: This change touches Dockerfile runtime packaging (the
COPY of preload JS files) and liveness behavior; before merging, run the
Docker-focused E2E subset (the nightly-e2e job subset) against this branch to
validate container build and runtime behavior—specifically confirm the preloads
are present in the container and the healthcheck liveness fallback works—then
only merge once those E2E runs pass.
🪄 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: f207dbbc-a632-4f30-ab2c-a5f222ac1fe4
📒 Files selected for processing (91)
.github/workflows/e2e-vitest-scenarios.yamlDockerfileci/env-var-doc-allowlist.jsonci/test-file-size-budget.jsonnemoclaw-blueprint/policies/presets/wechat.yamlscripts/install.shscripts/nemoclaw-start.shsrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/agent/runtime-hermes-secret-boundary-behavioural.test.tssrc/lib/channel-runtime-status.tssrc/lib/credentials/store.tssrc/lib/host-qr-handlers.tssrc/lib/inference/nim.test.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging-channel-config.test.tssrc/lib/messaging/applier/hook-phases.test.tssrc/lib/messaging/applier/hook-phases.tssrc/lib/messaging/applier/host-state-applier.tssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/applier/setup-applier.tssrc/lib/messaging/channels/discord/hooks/index.tssrc/lib/messaging/channels/discord/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/discord/manifest.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/metadata.test.tssrc/lib/messaging/channels/metadata.tssrc/lib/messaging/channels/openclaw-bridge-health.test.tssrc/lib/messaging/channels/openclaw-bridge-health.tssrc/lib/messaging/channels/slack/hooks/index.tssrc/lib/messaging/channels/slack/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-status.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-status.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/channels/slack/runtime/slack-channel-guard.jssrc/lib/messaging/channels/telegram/hooks/gateway-conflict-status.test.tssrc/lib/messaging/channels/telegram/hooks/gateway-conflict-status.tssrc/lib/messaging/channels/telegram/hooks/get-me-reachability.tssrc/lib/messaging/channels/telegram/hooks/index.tssrc/lib/messaging/channels/telegram/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/telegram/manifest.tssrc/lib/messaging/channels/telegram/runtime/telegram-diagnostics.jssrc/lib/messaging/channels/wechat/hooks/host-qr-login-runtime.tssrc/lib/messaging/channels/wechat/login.test.tssrc/lib/messaging/channels/wechat/login.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/wechat/qr.test.tssrc/lib/messaging/channels/wechat/qr.tssrc/lib/messaging/channels/wechat/runtime/wechat-diagnostics.jssrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.jssrc/lib/messaging/compiler/engines/build-step-engine.tssrc/lib/messaging/compiler/engines/health-check-engine.tssrc/lib/messaging/compiler/engines/runtime-setup-engine.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/hooks/builtins.tssrc/lib/messaging/hooks/errors.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/hooks/hook-runner.tssrc/lib/messaging/hooks/index.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/persistence.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/onboard.tssrc/lib/onboard/docker-gpu-patch-wsl.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/sandbox-build-patch-config.test.tssrc/lib/onboard/sandbox-build-patch-config.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/policy/index.tssrc/lib/sandbox/channels.test.tssrc/lib/sandbox/channels.tssrc/lib/security/redact.tssrc/lib/state/onboard-session.test.tssrc/lib/status-command-deps.test.tssrc/lib/status-command-deps.ts
💤 Files with no reviewable changes (12)
- .github/workflows/e2e-vitest-scenarios.yaml
- src/lib/host-qr-handlers.ts
- src/lib/sandbox/channels.test.ts
- src/lib/state/onboard-session.test.ts
- src/lib/onboard/sandbox-create-plan.ts
- src/lib/onboard/sandbox-build-patch-config.ts
- src/lib/security/redact.ts
- src/lib/status-command-deps.ts
- src/lib/sandbox/channels.ts
- src/lib/status-command-deps.test.ts
- src/lib/policy/index.ts
- src/lib/onboard/sandbox-build-patch-config.test.ts
✅ Files skipped from review due to trivial changes (9)
- ci/env-var-doc-allowlist.json
- nemoclaw-blueprint/policies/presets/wechat.yaml
- src/lib/messaging/channels/telegram/hooks/index.ts
- src/lib/messaging/hooks/index.ts
- src/lib/messaging/channels/slack/hooks/openclaw-bridge-health.ts
- src/lib/messaging/channels/wechat/login.test.ts
- src/lib/messaging/channels/wechat/qr.ts
- scripts/install.sh
- src/lib/messaging/channels/wechat/qr.test.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/lib/credentials/store.ts
- src/lib/onboard/messaging-conflict-guard.test.ts
- src/lib/messaging/index.ts
- src/lib/onboard/docker-gpu-patch.ts
- src/lib/actions/sandbox/snapshot.ts
- src/lib/inventory/index.ts
- src/lib/actions/sandbox/rebuild.ts
- src/lib/onboard.ts
- src/lib/messaging/plan-validation.test.ts
- src/lib/actions/sandbox/policy-channel-conflict.test.ts
- src/lib/onboard/messaging-conflict-guard.ts
- src/lib/messaging/applier/setup-applier.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.ts
- src/lib/actions/sandbox/doctor.ts
- src/lib/inventory/index.test.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.ts
- src/lib/onboard/policy-presets.ts
- scripts/nemoclaw-start.sh
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
🧹 Nitpick comments (1)
Dockerfile (1)
529-533: Run the Docker-focused E2E subset before merge.This updates baked runtime behavior (preload packaging + healthcheck liveness fallback), so validate it with the recommended
nightly-e2ejob subset on this branch.As per coding guidelines, Dockerfile changes are only reliably validated with real container build/runtime and the listed E2E suites.
Also applies to: 1091-1106
🤖 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 529 - 533, This change touches Dockerfile runtime packaging (the COPY of preload JS files) and liveness behavior; before merging, run the Docker-focused E2E subset (the nightly-e2e job subset) against this branch to validate container build and runtime behavior—specifically confirm the preloads are present in the container and the healthcheck liveness fallback works—then only merge once those E2E runs pass.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/compiler/workflow-planner.ts`:
- Around line 107-111: The three builders buildChannelStopPlanFromSandboxEntry,
buildChannelStartPlanFromSandboxEntry, and buildRebuildPlanFromSandboxEntry
currently call setPlanChannelDisabled / setPlanDisabledChannels but do not
filter plan.runtimeSetup, so hydrateDerivedSandboxMessagingPlanFields can keep
stale runtimeSetup entries; update those three functions to explicitly filter
plan.runtimeSetup after modifying disabledChannels (same approach used in
removePlanChannel) so runtimeSetup only contains entries for channels where
channel.active && !channel.disabled, or alternatively ensure the downstream
applier that consumes runtimeSetup.nodePreloads / envAliases / secretScans
always re-gates by channel.active && !channel.disabled; pick one fix and
implement it consistently (prefer adding the runtimeSetup filter in the three
builder functions to mirror removePlanChannel and avoid relying on downstream
behavior).
---
Nitpick comments:
In `@Dockerfile`:
- Around line 529-533: This change touches Dockerfile runtime packaging (the
COPY of preload JS files) and liveness behavior; before merging, run the
Docker-focused E2E subset (the nightly-e2e job subset) against this branch to
validate container build and runtime behavior—specifically confirm the preloads
are present in the container and the healthcheck liveness fallback works—then
only merge once those E2E runs pass.
🪄 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: f207dbbc-a632-4f30-ab2c-a5f222ac1fe4
📒 Files selected for processing (91)
.github/workflows/e2e-vitest-scenarios.yamlDockerfileci/env-var-doc-allowlist.jsonci/test-file-size-budget.jsonnemoclaw-blueprint/policies/presets/wechat.yamlscripts/install.shscripts/nemoclaw-start.shsrc/lib/actions/sandbox/doctor.tssrc/lib/actions/sandbox/policy-channel-conflict.test.tssrc/lib/actions/sandbox/policy-channel.tssrc/lib/actions/sandbox/rebuild.tssrc/lib/actions/sandbox/snapshot.tssrc/lib/agent/runtime-hermes-secret-boundary-behavioural.test.tssrc/lib/channel-runtime-status.tssrc/lib/credentials/store.tssrc/lib/host-qr-handlers.tssrc/lib/inference/nim.test.tssrc/lib/inventory/index.test.tssrc/lib/inventory/index.tssrc/lib/messaging-channel-config.test.tssrc/lib/messaging/applier/hook-phases.test.tssrc/lib/messaging/applier/hook-phases.tssrc/lib/messaging/applier/host-state-applier.tssrc/lib/messaging/applier/setup-applier.test.tssrc/lib/messaging/applier/setup-applier.tssrc/lib/messaging/channels/discord/hooks/index.tssrc/lib/messaging/channels/discord/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/discord/manifest.tssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/metadata.test.tssrc/lib/messaging/channels/metadata.tssrc/lib/messaging/channels/openclaw-bridge-health.test.tssrc/lib/messaging/channels/openclaw-bridge-health.tssrc/lib/messaging/channels/slack/hooks/index.tssrc/lib/messaging/channels/slack/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-status.test.tssrc/lib/messaging/channels/slack/hooks/socket-mode-gateway-status.tssrc/lib/messaging/channels/slack/manifest.tssrc/lib/messaging/channels/slack/runtime/slack-channel-guard.jssrc/lib/messaging/channels/telegram/hooks/gateway-conflict-status.test.tssrc/lib/messaging/channels/telegram/hooks/gateway-conflict-status.tssrc/lib/messaging/channels/telegram/hooks/get-me-reachability.tssrc/lib/messaging/channels/telegram/hooks/index.tssrc/lib/messaging/channels/telegram/hooks/openclaw-bridge-health.tssrc/lib/messaging/channels/telegram/manifest.tssrc/lib/messaging/channels/telegram/runtime/telegram-diagnostics.jssrc/lib/messaging/channels/wechat/hooks/host-qr-login-runtime.tssrc/lib/messaging/channels/wechat/login.test.tssrc/lib/messaging/channels/wechat/login.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/wechat/qr.test.tssrc/lib/messaging/channels/wechat/qr.tssrc/lib/messaging/channels/wechat/runtime/wechat-diagnostics.jssrc/lib/messaging/channels/whatsapp/manifest.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.jssrc/lib/messaging/compiler/engines/build-step-engine.tssrc/lib/messaging/compiler/engines/health-check-engine.tssrc/lib/messaging/compiler/engines/runtime-setup-engine.tssrc/lib/messaging/compiler/manifest-compiler.test.tssrc/lib/messaging/compiler/manifest-compiler.tssrc/lib/messaging/compiler/workflow-planner.tssrc/lib/messaging/hooks/builtins.tssrc/lib/messaging/hooks/errors.tssrc/lib/messaging/hooks/hook-runner.test.tssrc/lib/messaging/hooks/hook-runner.tssrc/lib/messaging/hooks/index.tssrc/lib/messaging/index.tssrc/lib/messaging/manifest/types.tssrc/lib/messaging/persistence.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/onboard.tssrc/lib/onboard/docker-gpu-patch-wsl.test.tssrc/lib/onboard/docker-gpu-patch.tssrc/lib/onboard/machine/core-flow-phases.test.tssrc/lib/onboard/messaging-channel-setup.test.tssrc/lib/onboard/messaging-conflict-guard.test.tssrc/lib/onboard/messaging-conflict-guard.tssrc/lib/onboard/policy-presets.tssrc/lib/onboard/sandbox-build-patch-config.test.tssrc/lib/onboard/sandbox-build-patch-config.tssrc/lib/onboard/sandbox-create-plan.tssrc/lib/policy/index.tssrc/lib/sandbox/channels.test.tssrc/lib/sandbox/channels.tssrc/lib/security/redact.tssrc/lib/state/onboard-session.test.tssrc/lib/status-command-deps.test.tssrc/lib/status-command-deps.ts
💤 Files with no reviewable changes (12)
- .github/workflows/e2e-vitest-scenarios.yaml
- src/lib/host-qr-handlers.ts
- src/lib/sandbox/channels.test.ts
- src/lib/state/onboard-session.test.ts
- src/lib/onboard/sandbox-create-plan.ts
- src/lib/onboard/sandbox-build-patch-config.ts
- src/lib/security/redact.ts
- src/lib/status-command-deps.ts
- src/lib/sandbox/channels.ts
- src/lib/status-command-deps.test.ts
- src/lib/policy/index.ts
- src/lib/onboard/sandbox-build-patch-config.test.ts
✅ Files skipped from review due to trivial changes (9)
- ci/env-var-doc-allowlist.json
- nemoclaw-blueprint/policies/presets/wechat.yaml
- src/lib/messaging/channels/telegram/hooks/index.ts
- src/lib/messaging/hooks/index.ts
- src/lib/messaging/channels/slack/hooks/openclaw-bridge-health.ts
- src/lib/messaging/channels/wechat/login.test.ts
- src/lib/messaging/channels/wechat/qr.ts
- scripts/install.sh
- src/lib/messaging/channels/wechat/qr.test.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/lib/credentials/store.ts
- src/lib/onboard/messaging-conflict-guard.test.ts
- src/lib/messaging/index.ts
- src/lib/onboard/docker-gpu-patch.ts
- src/lib/actions/sandbox/snapshot.ts
- src/lib/inventory/index.ts
- src/lib/actions/sandbox/rebuild.ts
- src/lib/onboard.ts
- src/lib/messaging/plan-validation.test.ts
- src/lib/actions/sandbox/policy-channel-conflict.test.ts
- src/lib/onboard/messaging-conflict-guard.ts
- src/lib/messaging/applier/setup-applier.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.test.ts
- src/lib/actions/sandbox/doctor.ts
- src/lib/inventory/index.test.ts
- src/lib/messaging/channels/slack/hooks/socket-mode-gateway-conflict.ts
- src/lib/onboard/policy-presets.ts
- scripts/nemoclaw-start.sh
🛑 Comments failed to post (1)
src/lib/messaging/compiler/workflow-planner.ts (1)
107-111:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect runtimeSetup handling across planner/applier paths rg -n --type=ts -C4 'runtimeSetup|nodePreloads|envAliases|secretScans|setPlanChannelDisabled|setPlanDisabledChannels|buildChannelStopPlanFromSandboxEntry|buildRebuildPlanFromSandboxEntry' src/lib # Inspect where runtime setup is actually applied/executed rg -n --type=ts -C4 'apply.*runtime|runtime.*apply|nodePreload|envAlias|secretScan' src/lib/messaging src/lib/onboard src/lib/actionsRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash set -euo pipefail # Show the exact implementations around setPlanChannelDisabled / setPlanDisabledChannels sed -n '70,160p' src/lib/messaging/compiler/workflow-planner.ts sed -n '260,460p' src/lib/messaging/compiler/workflow-planner.ts # Show where runtimeSetup is consumed in the applier rg -n --type=ts 'runtimeSetup' src/lib/messaging/applier -S sed -n '1,260p' src/lib/messaging/applier/setup-applier.ts sed -n '260,520p' src/lib/messaging/applier/setup-applier.ts # Show runtime-setup engine gating logic sed -n '1,120p' src/lib/messaging/compiler/engines/runtime-setup-engine.ts # Show how plans are applied on stop/start/rebuild actions sed -n '700,820p' src/lib/actions/sandbox/policy-channel.ts sed -n '200,320p' src/lib/actions/sandbox/rebuild.ts # Show host-state applier merge behavior (may still not filter) sed -n '1,200p' src/lib/messaging/applier/host-state-applier.tsRepository: NVIDIA/NemoClaw
Length of output: 34283
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find direct consumption of runtimeSetup fields (not just validation/merge) rg -n --type=ts 'runtimeSetup\.|nodePreloads|envAliases|secretScans' src/lib/messaging/applier src/lib/messaging/compiler/engines src/lib/messaging/compiler src/lib/actions | head -n 200 # Find where runtimeSetup is fed into something that writes to host / applies hooks rg -n --type=ts 'nodePreloads|envAliases|secretScans' src/lib/messaging/applier src/lib/messaging/compiler | head -n 200 # Find the runtime-setup-engine entrypoint usage rg -n --type=ts 'runtime-setup-engine' src/lib/messaging src/lib/actions # Show runtime-setup-engine integration file(s) rg -n --type=ts 'planRuntimeSetup\\(|runtimeSetup' src/lib/messaging/compiler | head -n 200Repository: NVIDIA/NemoClaw
Length of output: 8093
🏁 Script executed:
#!/bin/bash set -euo pipefail # Fix: find all actual reads of runtimeSetup in messaging code rg -n --type=ts 'runtimeSetup' src/lib/messaging | head -n 200 # Find concrete consumption of runtimeSetup arrays (reads, not just validation types) rg -n --type=ts '\.runtimeSetup\.(nodePreloads|envAliases|secretScans)' src/lib/messaging src/lib/actions | head -n 200 # Search for direct iteration/consumption without dot access rg -n --type=ts '(for \\(.*nodePreloads|map\\(.*nodePreloads|envAliases|secretScans)' src/lib/messaging src/lib/actions | head -n 200 # Inspect persistence: where derived fields might be hydrated/applied rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields|hydrateDerivedSandboxMessagingPlanFields' src/lib/messaging/persistence.ts sed -n '1,240p' src/lib/messaging/persistence.ts sed -n '240,520p' src/lib/messaging/persistence.tsRepository: NVIDIA/NemoClaw
Length of output: 2321
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate hydration function and planRuntimeSetup usage in persistence.ts rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields|hydrateDerivedSandboxMessagingPlanFields|planRuntimeSetup' src/lib/messaging/persistence.ts # Print relevant sections around those lines # (Use sed with line ranges estimated by the found line numbers) # We'll capture the line numbers first, then sed accordingly. LINES=$(rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields|hydrateDerivedSandboxMessagingPlanFields|planRuntimeSetup' src/lib/messaging/persistence.ts | cut -d: -f1 | tr '\n' ' ') echo "LineNumbers: $LINES" # Get around the first hydration function occurrence line=$(rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields' src/lib/messaging/persistence.ts | head -n1 | cut -d: -f1) start=$((line-80)); end=$((line+200)) sed -n "${start},${end}p" src/lib/messaging/persistence.ts # Also inspect around normalize/derive runtime setup (look for runtimeSetup: planRuntimeSetup or similar) rg -n --type=ts 'runtimeSetup:\s*planRuntimeSetup|runtimeSetup\s*=\s*planRuntimeSetup' src/lib/messaging/persistence.ts || trueRepository: NVIDIA/NemoClaw
Length of output: 11095
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate hydration function and any planRuntimeSetup usage in persistence.ts rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields|hydrateDerivedSandboxMessagingPlanFields|planRuntimeSetup' src/lib/messaging/persistence.ts # Dump the region around hydrateDerivedSandboxMessagingPlanFields line=$(rg -n --type=ts 'function hydrateDerivedSandboxMessagingPlanFields' src/lib/messaging/persistence.ts | head -n1 | cut -d: -f1) start=$((line-80)); end=$((line+220)) sed -n "${start},${end}p" src/lib/messaging/persistence.ts # Dump any inline runtimeSetup derivation patterns if present rg -n --type=ts 'runtimeSetup:\s*planRuntimeSetup|runtimeSetup\s*=\s*planRuntimeSetup' src/lib/messaging/persistence.ts || trueRepository: NVIDIA/NemoClaw
Length of output: 11841
Fix runtimeSetup filtering for stop/start/rebuild (or ensure applier recomputes/gates).
buildChannelStopPlanFromSandboxEntry,buildChannelStartPlanFromSandboxEntry, andbuildRebuildPlanFromSandboxEntryonly updatechannels/disabledChannels(viasetPlanChannelDisabled/setPlanDisabledChannels) and do not filterplan.runtimeSetup. Meanwhile,hydrateDerivedSandboxMessagingPlanFields()keeps an existingplan.runtimeSetupwhen it has any entries (runtimeSetupHasEntries(plan.runtimeSetup)), so it will not recompute runtimeSetup fromchannel.active && !channel.disabledin that case. If host-side setup/policy application consumesplan.runtimeSetupentries without re-gating by channel state, disabled channels may still contribute node preloads/env aliases/secret scans.Add explicit runtimeSetup filtering for stop/start/rebuild (like
removePlanChanneldoes), or ensure the code path that appliesruntimeSetup.nodePreloads/envAliases/secretScansfilters them to only channels wherechannel.active && !channel.disabled.🤖 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 107 - 111, The three builders buildChannelStopPlanFromSandboxEntry, buildChannelStartPlanFromSandboxEntry, and buildRebuildPlanFromSandboxEntry currently call setPlanChannelDisabled / setPlanDisabledChannels but do not filter plan.runtimeSetup, so hydrateDerivedSandboxMessagingPlanFields can keep stale runtimeSetup entries; update those three functions to explicitly filter plan.runtimeSetup after modifying disabledChannels (same approach used in removePlanChannel) so runtimeSetup only contains entries for channels where channel.active && !channel.disabled, or alternatively ensure the downstream applier that consumes runtimeSetup.nodePreloads / envAliases / secretScans always re-gates by channel.active && !channel.disabled; pick one fix and implement it consistently (prefer adding the runtimeSetup filter in the three builder functions to mirror removePlanChannel and avoid relying on downstream behavior).
…annel-manifest-stragglers # Conflicts: # ci/test-file-size-budget.json
…est-stragglers' into refactor/messaging-channel-manifest-stragglers
Contributor
There was a problem hiding this comment.
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/channels/slack/runtime/slack-channel-guard.js (1)
1-247:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPipeline failure: CI blocks
.jssource files.The CI reports this PR adds a JavaScript source file, which violates the repo policy requiring TypeScript for new Node.js code.
However, this file is a runtime preload (
nodePreloadsin the Slack manifest) that gets injected viaNODE_OPTIONS --requirebefore the TypeScript runtime is available. Runtime preloads often need to be plain JavaScript.Consider one of:
- Add this path to the CI allowlist if runtime preloads are a legitimate exception
- Convert to TypeScript with a build step that emits this
.jsfile to a dist location, then update the manifest'smodulereference🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/messaging/channels/slack/runtime/slack-channel-guard.js` around lines 1 - 247, The CI pipeline is rejecting the slack-channel-guard.js file because it violates the TypeScript requirement for Node.js code. However, this file is a legitimate exception since it is a runtime preload injected via NODE_OPTIONS --require before the TypeScript runtime is available. Update the CI allowlist or policy configuration to exclude runtime preload files (specifically the slack-channel-guard.js preload and similar preload patterns) from the TypeScript requirement, or document this path as an approved exception for Node.js preload scripts that must remain as plain JavaScript.Source: Pipeline failures
🧹 Nitpick comments (1)
Dockerfile (1)
651-651: Validate the runtime plan artifact bakes correctly with E2E tests.The runtime-setup phase now bakes messaging metadata (nodePreloads, envAliases, secretScans) into the container image for startup consumption, and the Slack deny-feedback behavior has migrated from a build-time patch to a runtime preload. Per coding guidelines, Dockerfile layer ordering and baked config changes require validation with a real container build.
Run the recommended E2E tests before merge, with particular attention to
openclaw-tui-chat-correlation-e2esince it validates that the baked OpenClaw chat send patch preserves user/assistant turn correlation—and the Slack deny-feedback behavior is now implemented as a runtime preload rather than a build-time patch.To execute:
gh workflow run nightly-e2e.yaml --ref <your-branch> -f jobs=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` at line 651, The Dockerfile runtime-setup phase now bakes messaging metadata into the container and has migrated the Slack deny-feedback behavior from a build-time patch to a runtime preload. Before merging, this configuration change must be validated with E2E tests. Execute the nightly E2E workflow with the specified job list (cloud-e2e, sandbox-survival-e2e, hermes-e2e, rebuild-openclaw-e2e, openclaw-tui-chat-correlation-e2e) on your branch to confirm the baked OpenClaw chat send patch preserves user/assistant turn correlation and the runtime preload implementation of Slack deny-feedback behavior functions correctly in a real container build.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.
Outside diff comments:
In `@src/lib/messaging/channels/slack/runtime/slack-channel-guard.js`:
- Around line 1-247: The CI pipeline is rejecting the slack-channel-guard.js
file because it violates the TypeScript requirement for Node.js code. However,
this file is a legitimate exception since it is a runtime preload injected via
NODE_OPTIONS --require before the TypeScript runtime is available. Update the CI
allowlist or policy configuration to exclude runtime preload files (specifically
the slack-channel-guard.js preload and similar preload patterns) from the
TypeScript requirement, or document this path as an approved exception for
Node.js preload scripts that must remain as plain JavaScript.
---
Nitpick comments:
In `@Dockerfile`:
- Line 651: The Dockerfile runtime-setup phase now bakes messaging metadata into
the container and has migrated the Slack deny-feedback behavior from a
build-time patch to a runtime preload. Before merging, this configuration change
must be validated with E2E tests. Execute the nightly E2E workflow with the
specified job list (cloud-e2e, sandbox-survival-e2e, hermes-e2e,
rebuild-openclaw-e2e, openclaw-tui-chat-correlation-e2e) on your branch to
confirm the baked OpenClaw chat send patch preserves user/assistant turn
correlation and the runtime preload implementation of Slack deny-feedback
behavior functions correctly in a real container build.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 401a45fa-0fd9-481b-a730-0fdc883cf6f5
📒 Files selected for processing (16)
Dockerfilescripts/patch-openclaw-slack-deny-feedback.mtssrc/lib/messaging/applier/build/messaging-build-applier.mtssrc/lib/messaging/channels/manifests.test.tssrc/lib/messaging/channels/slack/runtime/slack-channel-guard.jssrc/lib/messaging/channels/wechat/hooks/host-qr-login-runtime.tssrc/lib/messaging/channels/wechat/hooks/implementations.test.tssrc/lib/messaging/channels/wechat/hooks/seed-openclaw-account.tssrc/lib/messaging/channels/wechat/manifest.tssrc/lib/messaging/channels/wechat/qr.tssrc/lib/messaging/plan-validation.test.tssrc/lib/messaging/plan-validation.tssrc/lib/sandbox/build-context.tstest/messaging-build-applier.test.tstest/openclaw-slack-deny-feedback-patch.test.tstest/sandbox-build-context.test.ts
💤 Files with no reviewable changes (3)
- scripts/patch-openclaw-slack-deny-feedback.mts
- test/sandbox-build-context.test.ts
- src/lib/sandbox/build-context.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/messaging/channels/wechat/qr.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/messaging/channels/wechat/manifest.ts
- src/lib/messaging/plan-validation.test.ts
- src/lib/messaging/plan-validation.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)
test/e2e-scenario/live/whatsapp-qr-compact.test.ts (1)
188-191:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing
--experimental-strip-typesfor TypeScript preload.The test invokes
node --require ${PRELOAD}wherePRELOADis a.tsfile. Node.js will not load a.tsextension file without--experimental-strip-types, even if the file content is valid JavaScript. The test will fail with:TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts"Proposed fix
const patched = await runCommand("node", ["probe.mjs"], { cwd: workdir, - env: { NODE_OPTIONS: `--require ${PRELOAD}` }, + env: { NODE_OPTIONS: `--experimental-strip-types --require ${PRELOAD}` }, timeoutMs: PROBE_TIMEOUT_MS, });🤖 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/e2e-scenario/live/whatsapp-qr-compact.test.ts` around lines 188 - 191, The runCommand invocation for probe.mjs is attempting to require a TypeScript file via NODE_OPTIONS without the --experimental-strip-types flag, which will cause Node.js to fail with an unknown file extension error. Add the --experimental-strip-types flag to the NODE_OPTIONS environment variable in the runCommand call so that Node.js can properly load and parse the TypeScript preload file specified by PRELOAD.
🤖 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/telegram/runtime/telegram-diagnostics.ts`:
- Line 1: Remove the `// `@ts-nocheck`` directive from the top of both diagnostic
runtime modules to restore TypeScript type-checking compliance. In
src/lib/messaging/channels/telegram/runtime/telegram-diagnostics.ts at line 1,
delete the `@ts-nocheck` comment and ensure all process augmentation and wrapper
code has proper type annotations. Similarly, in
src/lib/messaging/channels/wechat/runtime/wechat-diagnostics.ts at line 1,
remove the `@ts-nocheck` comment and add proper TypeScript typing for any
process augmentation or wrappers. This ensures both files comply with repository
rules requiring typed validation via tsconfig-backed compilation for code under
src/lib.
---
Outside diff comments:
In `@test/e2e-scenario/live/whatsapp-qr-compact.test.ts`:
- Around line 188-191: The runCommand invocation for probe.mjs is attempting to
require a TypeScript file via NODE_OPTIONS without the
--experimental-strip-types flag, which will cause Node.js to fail with an
unknown file extension error. Add the --experimental-strip-types flag to the
NODE_OPTIONS environment variable in the runCommand call so that Node.js can
properly load and parse the TypeScript preload file specified by PRELOAD.
🪄 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: 5ba6d266-eb75-4d2d-98c3-d5d21041cb98
📒 Files selected for processing (13)
Dockerfilesrc/lib/messaging/channels/slack/runtime/slack-channel-guard.tssrc/lib/messaging/channels/telegram/runtime/telegram-diagnostics.tssrc/lib/messaging/channels/wechat/runtime/wechat-diagnostics.tssrc/lib/messaging/channels/whatsapp/runtime/whatsapp-qr-compact.tstest/e2e-scenario/live/whatsapp-qr-compact.test.tstest/local-slack-auth-test.shtest/nemoclaw-start-telegram-runtime.test.tstest/nemoclaw-start.test.tstest/openclaw-slack-deny-feedback-patch.test.tstest/telegram-diagnostics.test.tstest/wechat-diagnostics.test.tstest/whatsapp-qr-compact.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/openclaw-slack-deny-feedback-patch.test.ts
- test/nemoclaw-start.test.ts
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 27524152636
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27525173397
|
sandl99
added
v0.0.65
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27525651361
|
This was referenced Jun 15, 2026
13 tasks
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
4 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
This PR completes the messaging channel decoupling by routing channel-specific lifecycle behavior through manifests, hooks, and shared messaging helpers. It removes remaining core assumptions about concrete Telegram, WeChat, Slack, Discord, and WhatsApp implementations so core flows can operate from the manifest registry.
Close
Closes #4397
Changes
src/lib/messaging/.Verification
OpenClaw
Telegram
Slack
Hermes
Telegram
Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes