Skip to content

feat(cli): add channel config status summary - #6033

Closed
sandl99 wants to merge 2 commits into
mainfrom
feat/channels-status-config-values
Closed

feat(cli): add channel config status summary#6033
sandl99 wants to merge 2 commits into
mainfrom
feat/channels-status-config-values

Conversation

@sandl99

@sandl99 sandl99 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds a manifest-backed channels status configuration summary that compares sandbox registry inputs against the rendered OpenClaw/Hermes channel config. This replaces the narrower Telegram-only approach in #5705 with per-channel parsers for rendered config sources and keeps doctor unchanged.

Related Issue

Fixes #5695.
Supersedes #5705.

Changes

  • Add a central rendered-config parser registry plus per-channel parsers for Telegram, Teams, Slack, Discord, WeChat, and WhatsApp.
  • Show configured, non-secret channel values as Label (ENV_KEY): value and mark mismatches against rendered agent config as warnings.
  • Make no-arg channels status print a compact summary for configured channels instead of silently defaulting to one channel.
  • Keep WhatsApp on its existing deep diagnostics path and keep doctor behavior unchanged.
  • Make sandbox exec wrapping newline-safe while keeping single-line commands readable for existing recovery/fallback behavior.
  • Update generated command reference docs for the channel status help text.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: scoped to read-only channel status rendering plus the sandbox exec newline wrapper; no credential values are persisted or newly exposed beyond existing non-secret status visibility.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Verification notes:

  • Passed: npx vitest run --project cli src/lib/actions/sandbox/channel-status.test.ts src/lib/actions/sandbox/process-recovery.test.ts
  • Passed: npx vitest run --project integration test/process-recovery.test.ts test/cli/connect-recovery.test.ts test/cli/connect-recovery-settle.test.ts
  • Passed: npx vitest run --project cli src/lib/actions/sandbox/auto-pair-approval.test.ts
  • Passed: npx vitest run --project integration test/sandbox-connect-inference/auto-pair-approval.test.ts
  • Passed: npm run typecheck:cli
  • Passed: npm run build:cli
  • Passed: targeted npx biome check on modified source/test files
  • npm run docs completed with 0 errors and 2 existing warnings.
  • npx prek run --from-ref main --to-ref HEAD did not pass locally; remaining failures were unrelated local broad-lane issues (langchain-deepagents-code-image ${1^^} under sh, sandbox rlimit expectations, and two CLI timeout flakes that passed isolated as listed above).

Signed-off-by: San Dang sdang@nvidia.com

Summary by CodeRabbit

  • New Features
    • channels status now shows a compact all-channel summary by default, with detailed reporting when a specific channel is selected.
    • WhatsApp deep status checks were expanded (including pairing/session and delivery/coverage), with improved verdict outcomes.
    • Enhanced rendered-config parsing for messaging channels (Discord, Slack, Teams, Telegram, WeChat, WhatsApp) to support more accurate status comparisons.
  • Bug Fixes
    • Improved verdict handling and configuration matching behavior, including better treatment of missing/unknown and boolean-like values.
  • Documentation
    • Updated command help and reference docs for output/flag behavior and WhatsApp-specific probing.
  • Tests
    • Expanded status/config and exec-command coverage with new scenarios.

Signed-off-by: San Dang <sdang@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds rendered-config parsers for messaging channels, expands channels status to support all-channels output and config comparisons, exports sandbox exec command marking with CR/LF-based branching, and updates related tests and docs.

Changes

Rendered config, channel status, and exec wrapping

Layer / File(s) Summary
Rendered config contracts and registry
src/lib/messaging/channels/rendered-config-parser-utils.ts, src/lib/messaging/channels/rendered-config-parser.ts, src/lib/messaging/channels/index.ts, src/lib/messaging/channels/manifests.test.ts
Defines rendered-config source and parser contracts, adds shared lookup helpers, builds the built-in parser registry, and verifies every built-in manifest has a parser.
Per-channel rendered config parsers
src/lib/messaging/channels/telegram/rendered-config-parser.ts, src/lib/messaging/channels/teams/rendered-config-parser.ts, src/lib/messaging/channels/discord/rendered-config-parser.ts, src/lib/messaging/channels/slack/rendered-config-parser.ts, src/lib/messaging/channels/wechat/rendered-config-parser.ts, src/lib/messaging/channels/whatsapp/rendered-config-parser.ts, src/lib/messaging/channels/discord/rendered-config-parser.test.ts
Implements channel-specific rendered-config visibility and value extraction for Telegram, Teams, Discord, Slack, WeChat, and WhatsApp, plus a Discord parser test.
Sandbox exec command marking
src/lib/actions/sandbox/process-recovery.ts, src/lib/actions/sandbox/process-recovery.test.ts, test/cli/connect-recovery.test.ts, test/cli/connect-recovery-settle.test.ts
Exports buildSandboxExecMarkedCommand, switches its branching to CR/LF detection, and updates tests and shell stubs to decode wrapped sandbox exec commands before matching them.
Rendered config comparison diagnostics
src/lib/actions/sandbox/channel-status-config.ts
Reads rendered config files inside the sandbox, parses env/YAML/JSON content, and compares rendered values against expected channel inputs with normalization for arrays and booleans.
Channel status control flow
src/lib/actions/sandbox/channel-status.ts, src/lib/actions/sandbox/channel-status.test.ts
Expands channel status reports to include a sandbox-level all-channels variant, threads deep-diagnostics options through basic reports, and updates tests for channel selection and per-channel config output.
Docs and command metadata
src/commands/sandbox/channels/status.ts, src/lib/cli/public-display-defaults.ts, docs/reference/commands.mdx, docs/reference/commands-nemohermes.mdx
Updates command metadata and reference docs to describe messaging channel status, two reporting modes, WhatsApp probe behavior, and rendered config comparison output.

Sequence Diagram(s)

sequenceDiagram
  participant ChannelStatus as buildConfigStatusSignals
  participant Sandbox as execSandbox
  participant Parser as RenderedChannelConfigParser
  participant Renderer as compareConfigSource
  ChannelStatus->>Parser: listConfigVisibilityKeys(context)
  ChannelStatus->>Sandbox: cat resolved config targets
  Sandbox-->>ChannelStatus: file contents
  ChannelStatus->>Renderer: compare expected vs rendered values
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The PR includes extensive unrelated work—new parsers for Slack, Discord, Teams, WeChat, WhatsApp, docs, and exec wrapping—beyond [#5695]. Split the non-Telegram parser additions, docs, and sandbox exec recovery changes into separate PRs or link them to additional issues.
Docstring Coverage ⚠️ Warning Docstring coverage is 5.77% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Linked Issues check ❓ Inconclusive The summaries point to a generalized status/config comparison flow, but they never explicitly show TELEGRAM_GROUP_POLICY in the telegram output. Add a Telegram-specific output or test case proving channels status --channel telegram shows open, allowlist, and disabled policy values.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately reflects the main CLI change to channel status reporting.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/channels-status-config-values

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

@github-code-quality

github-code-quality Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the feat/channels-status... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main feat/channels-status... 1ef6b52 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the feat/channels-status... branch is 68%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main feat/channels-status... 1ef6b52 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/sandbox/config.ts 75%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 60%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 30, 2026 07:53 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@sandl99 sandl99 added area: cli Command line interface, flags, terminal UX, or output area: messaging Messaging channels, bridges, manifests, or channel lifecycle enhancement New capability or improvement request labels Jun 30, 2026
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: channels-stop-start, channels-add-remove, gateway-guard-recovery, sandbox-operations
Optional E2E: messaging-providers, hermes-discord, hermes-slack

Dispatch hint: channels-stop-start,channels-add-remove,gateway-guard-recovery,sandbox-operations

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • channels-stop-start (high): Exercises real OpenClaw and Hermes messaging channel lifecycle for Telegram, Discord, WeChat, Slack, and WhatsApp, including rebuilds and rendered sandbox config probes. This is the closest existing live coverage for the channel config surfaces touched by the new rendered-config status comparisons.
  • channels-add-remove (high): Covers the real channel add/remove path, registry messaging plan persistence, gateway credential reuse, policy preset application, rebuild, and rendered OpenClaw channel config for Telegram. Required because the PR changes channel status/config interpretation around the same registry/rendered-config boundary.
  • gateway-guard-recovery (medium): Directly exercises the production connect --probe-only gateway recovery route that runs a multi-line OpenClaw recovery script through executeSandboxExecCommand. Required because the PR changes multi-line sandbox exec wrapping and marker extraction behavior.
  • sandbox-operations (medium): Exercises real sandbox list/status/logs, sandbox exec after process recovery, and gateway recovery behavior. Required as broad live coverage for shared sandbox exec/process-recovery changes that can affect normal sandbox lifecycle operations.

Optional E2E

  • messaging-providers (high): Useful adjacent confidence for Telegram, Discord, Slack, and WhatsApp provider registration, credential isolation, OpenClaw config patching, and messaging proxy behavior. It is broader and more expensive than the required channel lifecycle/status-adjacent coverage.
  • hermes-discord (high): Optional targeted confidence for Hermes Discord rendered config and rebuild behavior, relevant because the PR changes Discord rendered-config parsing.
  • hermes-slack (high): Optional targeted confidence for Hermes Slack policy, placeholder, provider, secret-boundary, and egress contracts, relevant because the PR changes Slack rendered-config parsing.

New E2E recommendations

  • messaging channel status diagnostics (high): No existing live E2E appears to explicitly run nemoclaw <sandbox> channels status without --channel, --channel <non-whatsapp>, and --json after real channel add/rebuild. Unit tests cover the parser/action, but the new user-facing status behavior depends on a live sandbox, OpenShell exec output framing, registry state, and rendered agent config files.
    • Suggested test: Add a channels-status live E2E or extend channels-add-remove to run channels status, channels status --channel telegram, and JSON status after rebuild, asserting non-secret rendered config comparisons are present and secrets are absent.
  • WhatsApp channel status exec probe (medium): The WhatsApp detailed status probe uses a multi-line in-sandbox shell script through the changed sandbox exec wrapper. Existing WhatsApp QR/provider tests cover pairing/provider behavior, but not the host channels status --channel whatsapp diagnostic verdict path.
    • Suggested test: Add a live WhatsApp diagnostic status check that runs channels status --channel whatsapp --json against an unpaired/QR-only sandbox and asserts bounded execution, expected verdict fields, and no message-body/secret leakage.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: channels-stop-start,channels-add-remove,gateway-guard-recovery,sandbox-operations

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: channels-stop-start, sandbox-operations
Optional E2E targets: messaging-providers

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=channels-stop-start
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-operations

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required E2E targets

  • channels-stop-start: Messaging channel status/config comparison changes touch rendered config parsing for Telegram, Discord, Slack, WeChat, WhatsApp, and Hermes/OpenClaw config paths. The channels-stop-start E2E job exercises live OpenClaw and Hermes channel registry state, rebuilds, policy coverage, and in-sandbox rendered channel config for the affected messaging surfaces.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=channels-stop-start
  • sandbox-operations: process-recovery.ts changes the sandbox exec command wrapper used by recovery/status paths. The sandbox-operations E2E job includes a process recovery assertion after killing the OpenClaw gateway and verifies the sandbox remains executable afterward.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=sandbox-operations

Optional E2E targets

  • messaging-providers: Adjacent coverage for provider placeholder redaction and OpenClaw rendered messaging config across multiple providers; useful because this PR changes shared rendered-config parser utilities and per-channel parser files.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=messaging-providers

Relevant changed files

  • src/commands/sandbox/channels/status.ts
  • src/lib/actions/sandbox/channel-status-config.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/actions/sandbox/process-recovery.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.ts
  • src/lib/messaging/channels/index.ts
  • src/lib/messaging/channels/rendered-config-parser-utils.ts
  • src/lib/messaging/channels/rendered-config-parser.ts
  • src/lib/messaging/channels/slack/rendered-config-parser.ts
  • src/lib/messaging/channels/teams/rendered-config-parser.ts
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
  • src/lib/messaging/channels/wechat/rendered-config-parser.ts
  • src/lib/messaging/channels/whatsapp/rendered-config-parser.ts

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Source-of-truth review needed: Rendered config visibility keys.
Open items: 0 required · 7 warnings · 0 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 7 still apply · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Source-of-truth review needed: Rendered config visibility keys
  • PRA-2 Resolve or justify: Source-of-truth review needed: Malformed rendered config parsing
  • PRA-3 Resolve or justify: Source-of-truth review needed: Newline-safe sandbox exec wrapping
  • PRA-4 Resolve or justify: Bound rendered-config reads and surface parse failures in src/lib/actions/sandbox/channel-status-config.ts:298
  • PRA-5 Resolve or justify: Enforce config-only visibility before resolving or reading parser sources in src/lib/actions/sandbox/channel-status-config.ts:65
  • PRA-6 Resolve or justify: Detailed WhatsApp status bypasses the rendered config comparisons documented for registered channels in src/lib/actions/sandbox/channel-status.ts:629
  • PRA-7 Resolve or justify: Add caller-level regression for multiline Hermes boundary commands in src/lib/actions/sandbox/process-recovery.test.ts:13
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-T6 Add or justify test follow-up: Add caller-level regression for multiline Hermes boundary commands
  • PRA-T7 Add or justify test follow-up: Acceptance clause
  • PRA-T8 Add or justify test follow-up: Acceptance clause

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify security src/lib/actions/sandbox/channel-status-config.ts:298 Read a bounded number of bytes from each rendered config source, for example `head -c <limit> -- <quoted-target>` or an equivalent safe shell snippet, and include parse failures in source-level diagnostics such as `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked`.
PRA-5 Resolve/justify security src/lib/actions/sandbox/channel-status-config.ts:65 Build a manifest input map first and filter `parser.listConfigVisibilityKeys(...)` to keys whose `inputId` maps to `kind === "config"` before `resolveRenderedConfigSources()` and `readConfigSourceValues()` run. Consider warning on invalid parser keys so parser regressions are visible during tests.
PRA-6 Resolve/justify correctness src/lib/actions/sandbox/channel-status.ts:629 Either append non-secret rendered config signals to the detailed WhatsApp report, or narrow the docs and remove or explicitly justify the WhatsApp parser if detailed WhatsApp status is intentionally deep-probe-only.
PRA-7 Resolve/justify tests src/lib/actions/sandbox/process-recovery.test.ts:13 Add a focused test at `executeSandboxExecCommand` or the nearest injectable boundary that runs a multiline Hermes boundary/recovery script through the marked command path and verifies the same `SECRET_BOUNDARY_REFUSED` marker and exit behavior after wrapping.
Review findings by urgency: 0 required fixes, 7 items to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: Rendered config visibility keys

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Inject or stub a parser key for `botToken` and assert no sandbox file read occurs for that key and no token label/env/value appears in status output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildConfigStatusSignals()` currently resolves and reads parser-emitted sources before creating `configInputs` from `kind === "config"` inputs.

PRA-2 Resolve/justify — Source-of-truth review needed: Malformed rendered config parsing

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Malformed `openclaw.json` produces one `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked` warning.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `parseRenderedConfigSource()` catches parse errors, but source warnings are currently generated only from read failures in `targetReads`.

PRA-3 Resolve/justify — Source-of-truth review needed: Newline-safe sandbox exec wrapping

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: `executeSandboxExecCommand` preserves `SECRET_BOUNDARY_REFUSED` behavior for a multiline Hermes boundary/recovery script.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildSandboxExecMarkedCommand()` now base64-wraps multiline commands, and current tests verify wrapper shape rather than the high-risk Hermes boundary caller behavior.

PRA-4 Resolve/justify — Bound rendered-config reads and surface parse failures

  • Location: src/lib/actions/sandbox/channel-status-config.ts:298
  • Category: security
  • Problem: The rendered config status path reads sandbox files with an unbounded `cat` and only records read failures in `targetReads`. If JSON/YAML parsing fails, `parseRenderedConfigSource()` returns `could not parse <target>`, but `configSourceReadSignals()` never sees that parse failure, so operators only get per-input `(not checked)` details rather than a source-level warning.
  • Impact: A sandbox-controlled or corrupted config file can cause host-side memory/CPU pressure during status, and malformed/tampered rendered config can be silently downgraded to an ambiguous not-checked state. That weakens a security-sensitive diagnostic at the sandbox-host boundary and can give operators false confidence that config drift was checked.
  • Recommended action: Read a bounded number of bytes from each rendered config source, for example `head -c <limit> -- <quoted-target>` or an equivalent safe shell snippet, and include parse failures in source-level diagnostics such as `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/channel-status-config.ts` and confirm `readConfigSourceValues()` no longer builds `cat ${quotePath(target)}` and that parse failures are included in the diagnostics rendered by `configSourceReadSignals()` or an equivalent source-warning path.
  • Missing regression test: Add a `showSandboxChannelStatus` test where `/sandbox/.openclaw/openclaw.json` returns malformed JSON and assert exactly one `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked` warning; add a test that captures the exec command and asserts the rendered-config read is byte-bounded.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/channel-status-config.ts` and confirm `readConfigSourceValues()` no longer builds `cat ${quotePath(target)}` and that parse failures are included in the diagnostics rendered by `configSourceReadSignals()` or an equivalent source-warning path.
  • Evidence: `readConfigSourceValues()` invokes `deps.execSandbox(sandboxName, `cat ${quotePath(target)}`, CONFIG_STATUS_TIMEOUT_MS)`. `parseRenderedConfigSource()` catches JSON/YAML parse errors, but `configSourceReadSignals()` iterates only `targetReads`, which contain `{ ok: true, contents }` for successfully read malformed files.

PRA-5 Resolve/justify — Enforce config-only visibility before resolving or reading parser sources

  • Location: src/lib/actions/sandbox/channel-status-config.ts:65
  • Category: security
  • Problem: The code asks a channel parser for visibility keys, resolves their targets, and reads those targets before centrally checking that each `inputId` maps to a manifest input with `kind === "config"`. The final signal loop renders only config inputs, but the read/parse boundary still trusts every parser-emitted key.
  • Impact: A future parser mistake could cause status to read or parse secret-adjacent files for a secret/nonexistent input before the display layer filters it out. Even if values are not printed, this expands credential exposure in host memory and makes the parser contract fragile.
  • Recommended action: Build a manifest input map first and filter `parser.listConfigVisibilityKeys(...)` to keys whose `inputId` maps to `kind === "config"` before `resolveRenderedConfigSources()` and `readConfigSourceValues()` run. Consider warning on invalid parser keys so parser regressions are visible during tests.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `buildConfigStatusSignals()` and confirm parser visibility keys are validated against `manifest.inputs` before any target resolution or `execSandbox` read occurs.
  • Missing regression test: Add a parser-level or status-level test that injects a visibility key for `botToken` and asserts no sandbox read is attempted for that key and no token label, env key, or rendered value appears in text or JSON output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `buildConfigStatusSignals()` and confirm parser visibility keys are validated against `manifest.inputs` before any target resolution or `execSandbox` read occurs.
  • Evidence: `buildConfigStatusSignals()` currently calls `parser.listConfigVisibilityKeys(...)`, `resolveRenderedConfigSources(...)`, and `readConfigSourceValues(...)` before creating `configInputs` from `channelPlan.inputs.filter((input) => input.kind === "config")` and before iterating manifest config inputs.

PRA-6 Resolve/justify — Detailed WhatsApp status bypasses the rendered config comparisons documented for registered channels

  • Location: src/lib/actions/sandbox/channel-status.ts:629
  • Category: correctness
  • Problem: The docs now say registered channels compare non-secret config inputs from the sandbox registry against rendered agent config, and a WhatsApp rendered-config parser was added. However, `--channel whatsapp` takes the deep-probe branch and returns only the `WhatsappDiagnosticReport`; it does not append `buildConfigStatusSignals()` results.
  • Impact: Operators using the detailed WhatsApp status path will not see rendered config drift or allowlist comparisons even though the user-facing docs describe that behavior for registered channels. The new WhatsApp parser is only useful in the no-arg summary path, which makes detailed and summary status disagree.
  • Recommended action: Either append non-secret rendered config signals to the detailed WhatsApp report, or narrow the docs and remove or explicitly justify the WhatsApp parser if detailed WhatsApp status is intentionally deep-probe-only.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the `showSandboxChannelStatus()` branch for `diagnostic.deepProbe === "in-sandbox-qr"` and confirm whether `buildConfigStatusSignals()` is included in the returned detailed report for WhatsApp.
  • Missing regression test: Add a `--channel whatsapp` test with Hermes `WHATSAPP_ALLOWED_USERS` in the rendered `.env` and a matching registry config value, then assert the detailed report includes a non-secret WhatsApp allowlist config signal; or add a test/documentation assertion that detailed WhatsApp deliberately omits rendered config comparisons.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the `showSandboxChannelStatus()` branch for `diagnostic.deepProbe === "in-sandbox-qr"` and confirm whether `buildConfigStatusSignals()` is included in the returned detailed report for WhatsApp.
  • Evidence: The deep-probe branch constructs `{ schemaVersion, sandbox, channel, report: whatsappReport }` directly, while `buildBasicChannelReport()` is the only path that calls `buildConfigStatusSignals()`.

PRA-7 Resolve/justify — Add caller-level regression for multiline Hermes boundary commands

  • Location: src/lib/actions/sandbox/process-recovery.test.ts:13
  • Category: tests
  • Problem: The new sandbox exec wrapper now base64-wraps every multiline command. Existing tests verify wrapper shape and some CLI stubs decode the payload, but they do not exercise the high-risk Hermes secret-boundary/recovery caller semantics after wrapping.
  • Impact: A wrapper regression could preserve syntactic shape while changing exit behavior or stdout framing for the Hermes secret-boundary validator, weakening the credential boundary recovery path without a targeted test catching it.
  • Recommended action: Add a focused test at `executeSandboxExecCommand` or the nearest injectable boundary that runs a multiline Hermes boundary/recovery script through the marked command path and verifies the same `SECRET_BOUNDARY_REFUSED` marker and exit behavior after wrapping.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `src/lib/actions/sandbox/process-recovery.test.ts` and confirm there is a test that exercises a multiline command containing the Hermes boundary validator path through the actual command execution/framing boundary, not only `buildSandboxExecMarkedCommand()` string contents.
  • Missing regression test: Add a test named like `executeSandboxExecCommand preserves SECRET_BOUNDARY_REFUSED for multiline Hermes boundary scripts` that simulates OpenShell stdout framing and asserts the parsed result still exposes the refusal marker.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `src/lib/actions/sandbox/process-recovery.test.ts` and confirm there is a test that exercises a multiline command containing the Hermes boundary validator path through the actual command execution/framing boundary, not only `buildSandboxExecMarkedCommand()` string contents.
  • Evidence: The changed tests assert `buildSandboxExecMarkedCommand()` is newline-free, contains `base64 -d | sh`, and decodes to the original payload, but the payload is `printf 'hello\n'\ncat '/sandbox/.openclaw/openclaw.json'`, not a Hermes boundary script.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Runtime validation: non-interactive onboard with `TELEGRAM_GROUP_POLICY=open`, then `channels status --channel telegram` shows `Telegram group policy (TELEGRAM_GROUP_POLICY): open`.. The PR changes sandbox exec wrapping, status behavior, and host-side parsing of sandbox-rendered config. Unit coverage is broad and relevant, but runtime validation is still advisable for the linked issue and sandbox transport paths.
  • PRA-T2 Runtime validation — Runtime validation: non-interactive onboard with `TELEGRAM_GROUP_POLICY=allowlist`, then `channels status --channel telegram` shows `Telegram group policy (TELEGRAM_GROUP_POLICY): allowlist`.. The PR changes sandbox exec wrapping, status behavior, and host-side parsing of sandbox-rendered config. Unit coverage is broad and relevant, but runtime validation is still advisable for the linked issue and sandbox transport paths.
  • PRA-T3 Runtime validation — Runtime validation: non-interactive onboard with `TELEGRAM_GROUP_POLICY=disabled`, then `channels status --channel telegram` shows `Telegram group policy (TELEGRAM_GROUP_POLICY): disabled`.. The PR changes sandbox exec wrapping, status behavior, and host-side parsing of sandbox-rendered config. Unit coverage is broad and relevant, but runtime validation is still advisable for the linked issue and sandbox transport paths.
  • PRA-T4 Runtime validation — Runtime validation: no-arg `channels status` for a sandbox with Telegram and WhatsApp does not run the WhatsApp deep probe and still shows Telegram group policy in the compact summary.. The PR changes sandbox exec wrapping, status behavior, and host-side parsing of sandbox-rendered config. Unit coverage is broad and relevant, but runtime validation is still advisable for the linked issue and sandbox transport paths.
  • PRA-T5 Runtime validation — Unit regression: malformed rendered `openclaw.json` emits one source-level parse warning instead of only `(not checked)` details.. The PR changes sandbox exec wrapping, status behavior, and host-side parsing of sandbox-rendered config. Unit coverage is broad and relevant, but runtime validation is still advisable for the linked issue and sandbox transport paths.
  • PRA-T6 Add caller-level regression for multiline Hermes boundary commands — Add a focused test at `executeSandboxExecCommand` or the nearest injectable boundary that runs a multiline Hermes boundary/recovery script through the marked command path and verifies the same `SECRET_BOUNDARY_REFUSED` marker and exit behavior after wrapping.
  • PRA-T7 Acceptance clause — Onboarding runs without prompting for the Telegram group policy; `TELEGRAM_GROUP_POLICY` supplies the value. — add test evidence or identify existing coverage. This PR does not modify onboarding prompt ingestion. The changed tests cover status rendering from registry/rendered config values, but not a non-interactive onboarding run with `TELEGRAM_GROUP_POLICY` supplied by env.
  • PRA-T8 Acceptance clause — The sandbox reaches Ready. — add test evidence or identify existing coverage. Sandbox lifecycle readiness is not changed by this diff and is not proven by the changed unit tests. Runtime validation is recommended for the linked issue scenario.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Rendered config visibility keys

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Inject or stub a parser key for `botToken` and assert no sandbox file read occurs for that key and no token label/env/value appears in status output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildConfigStatusSignals()` currently resolves and reads parser-emitted sources before creating `configInputs` from `kind === "config"` inputs.

PRA-2 Resolve/justify — Source-of-truth review needed: Malformed rendered config parsing

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Malformed `openclaw.json` produces one `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked` warning.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `parseRenderedConfigSource()` catches parse errors, but source warnings are currently generated only from read failures in `targetReads`.

PRA-3 Resolve/justify — Source-of-truth review needed: Newline-safe sandbox exec wrapping

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: `executeSandboxExecCommand` preserves `SECRET_BOUNDARY_REFUSED` behavior for a multiline Hermes boundary/recovery script.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: `buildSandboxExecMarkedCommand()` now base64-wraps multiline commands, and current tests verify wrapper shape rather than the high-risk Hermes boundary caller behavior.

PRA-4 Resolve/justify — Bound rendered-config reads and surface parse failures

  • Location: src/lib/actions/sandbox/channel-status-config.ts:298
  • Category: security
  • Problem: The rendered config status path reads sandbox files with an unbounded `cat` and only records read failures in `targetReads`. If JSON/YAML parsing fails, `parseRenderedConfigSource()` returns `could not parse <target>`, but `configSourceReadSignals()` never sees that parse failure, so operators only get per-input `(not checked)` details rather than a source-level warning.
  • Impact: A sandbox-controlled or corrupted config file can cause host-side memory/CPU pressure during status, and malformed/tampered rendered config can be silently downgraded to an ambiguous not-checked state. That weakens a security-sensitive diagnostic at the sandbox-host boundary and can give operators false confidence that config drift was checked.
  • Recommended action: Read a bounded number of bytes from each rendered config source, for example `head -c <limit> -- <quoted-target>` or an equivalent safe shell snippet, and include parse failures in source-level diagnostics such as `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/actions/sandbox/channel-status-config.ts` and confirm `readConfigSourceValues()` no longer builds `cat ${quotePath(target)}` and that parse failures are included in the diagnostics rendered by `configSourceReadSignals()` or an equivalent source-warning path.
  • Missing regression test: Add a `showSandboxChannelStatus` test where `/sandbox/.openclaw/openclaw.json` returns malformed JSON and assert exactly one `Rendered config source: could not parse /sandbox/.openclaw/openclaw.json; config comparisons not checked` warning; add a test that captures the exec command and asserts the rendered-config read is byte-bounded.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/actions/sandbox/channel-status-config.ts` and confirm `readConfigSourceValues()` no longer builds `cat ${quotePath(target)}` and that parse failures are included in the diagnostics rendered by `configSourceReadSignals()` or an equivalent source-warning path.
  • Evidence: `readConfigSourceValues()` invokes `deps.execSandbox(sandboxName, `cat ${quotePath(target)}`, CONFIG_STATUS_TIMEOUT_MS)`. `parseRenderedConfigSource()` catches JSON/YAML parse errors, but `configSourceReadSignals()` iterates only `targetReads`, which contain `{ ok: true, contents }` for successfully read malformed files.

PRA-5 Resolve/justify — Enforce config-only visibility before resolving or reading parser sources

  • Location: src/lib/actions/sandbox/channel-status-config.ts:65
  • Category: security
  • Problem: The code asks a channel parser for visibility keys, resolves their targets, and reads those targets before centrally checking that each `inputId` maps to a manifest input with `kind === "config"`. The final signal loop renders only config inputs, but the read/parse boundary still trusts every parser-emitted key.
  • Impact: A future parser mistake could cause status to read or parse secret-adjacent files for a secret/nonexistent input before the display layer filters it out. Even if values are not printed, this expands credential exposure in host memory and makes the parser contract fragile.
  • Recommended action: Build a manifest input map first and filter `parser.listConfigVisibilityKeys(...)` to keys whose `inputId` maps to `kind === "config"` before `resolveRenderedConfigSources()` and `readConfigSourceValues()` run. Consider warning on invalid parser keys so parser regressions are visible during tests.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `buildConfigStatusSignals()` and confirm parser visibility keys are validated against `manifest.inputs` before any target resolution or `execSandbox` read occurs.
  • Missing regression test: Add a parser-level or status-level test that injects a visibility key for `botToken` and asserts no sandbox read is attempted for that key and no token label, env key, or rendered value appears in text or JSON output.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `buildConfigStatusSignals()` and confirm parser visibility keys are validated against `manifest.inputs` before any target resolution or `execSandbox` read occurs.
  • Evidence: `buildConfigStatusSignals()` currently calls `parser.listConfigVisibilityKeys(...)`, `resolveRenderedConfigSources(...)`, and `readConfigSourceValues(...)` before creating `configInputs` from `channelPlan.inputs.filter((input) => input.kind === "config")` and before iterating manifest config inputs.

PRA-6 Resolve/justify — Detailed WhatsApp status bypasses the rendered config comparisons documented for registered channels

  • Location: src/lib/actions/sandbox/channel-status.ts:629
  • Category: correctness
  • Problem: The docs now say registered channels compare non-secret config inputs from the sandbox registry against rendered agent config, and a WhatsApp rendered-config parser was added. However, `--channel whatsapp` takes the deep-probe branch and returns only the `WhatsappDiagnosticReport`; it does not append `buildConfigStatusSignals()` results.
  • Impact: Operators using the detailed WhatsApp status path will not see rendered config drift or allowlist comparisons even though the user-facing docs describe that behavior for registered channels. The new WhatsApp parser is only useful in the no-arg summary path, which makes detailed and summary status disagree.
  • Recommended action: Either append non-secret rendered config signals to the detailed WhatsApp report, or narrow the docs and remove or explicitly justify the WhatsApp parser if detailed WhatsApp status is intentionally deep-probe-only.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read the `showSandboxChannelStatus()` branch for `diagnostic.deepProbe === "in-sandbox-qr"` and confirm whether `buildConfigStatusSignals()` is included in the returned detailed report for WhatsApp.
  • Missing regression test: Add a `--channel whatsapp` test with Hermes `WHATSAPP_ALLOWED_USERS` in the rendered `.env` and a matching registry config value, then assert the detailed report includes a non-secret WhatsApp allowlist config signal; or add a test/documentation assertion that detailed WhatsApp deliberately omits rendered config comparisons.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read the `showSandboxChannelStatus()` branch for `diagnostic.deepProbe === "in-sandbox-qr"` and confirm whether `buildConfigStatusSignals()` is included in the returned detailed report for WhatsApp.
  • Evidence: The deep-probe branch constructs `{ schemaVersion, sandbox, channel, report: whatsappReport }` directly, while `buildBasicChannelReport()` is the only path that calls `buildConfigStatusSignals()`.

PRA-7 Resolve/justify — Add caller-level regression for multiline Hermes boundary commands

  • Location: src/lib/actions/sandbox/process-recovery.test.ts:13
  • Category: tests
  • Problem: The new sandbox exec wrapper now base64-wraps every multiline command. Existing tests verify wrapper shape and some CLI stubs decode the payload, but they do not exercise the high-risk Hermes secret-boundary/recovery caller semantics after wrapping.
  • Impact: A wrapper regression could preserve syntactic shape while changing exit behavior or stdout framing for the Hermes secret-boundary validator, weakening the credential boundary recovery path without a targeted test catching it.
  • Recommended action: Add a focused test at `executeSandboxExecCommand` or the nearest injectable boundary that runs a multiline Hermes boundary/recovery script through the marked command path and verifies the same `SECRET_BOUNDARY_REFUSED` marker and exit behavior after wrapping.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `src/lib/actions/sandbox/process-recovery.test.ts` and confirm there is a test that exercises a multiline command containing the Hermes boundary validator path through the actual command execution/framing boundary, not only `buildSandboxExecMarkedCommand()` string contents.
  • Missing regression test: Add a test named like `executeSandboxExecCommand preserves SECRET_BOUNDARY_REFUSED for multiline Hermes boundary scripts` that simulates OpenShell stdout framing and asserts the parsed result still exposes the refusal marker.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `src/lib/actions/sandbox/process-recovery.test.ts` and confirm there is a test that exercises a multiline command containing the Hermes boundary validator path through the actual command execution/framing boundary, not only `buildSandboxExecMarkedCommand()` string contents.
  • Evidence: The changed tests assert `buildSandboxExecMarkedCommand()` is newline-free, contains `base64 -d | sh`, and decodes to the original payload, but the payload is `printf 'hello\n'\ncat '/sandbox/.openclaw/openclaw.json'`, not a Hermes boundary script.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (2)
src/lib/actions/sandbox/channel-status.ts (2)

874-982: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Rendered-config parsing/comparison logic is placed in the actions layer.

parseEnvLines, unquoteEnvValue, parseRenderedConfigSource, compareConfigSource, configValuesEqual, listConfigValues, booleanConfigValue, and formatConfigValue are pure rendered-config parsing/comparison primitives. Per the layer ownership for this cohort, this logic belongs under src/lib/messaging/** (side-effect free), leaving channel-status.ts to orchestrate. Consider relocating these to the messaging package (e.g., alongside the per-channel parser utilities) and importing them here.

As per path instructions: "ensure the new 'channels status' orchestration stays in src/lib/actions/sandbox/**, while the 'rendered config' parsing/comparison logic stays in src/lib/messaging/** and remains side-effect free."

🤖 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/channel-status.ts` around lines 874 - 982, The
rendered-config parsing and comparison helpers are currently living in the
actions layer, but they should be moved into the side-effect-free messaging
layer. Relocate parseEnvLines, unquoteEnvValue, parseRenderedConfigSource,
compareConfigSource, configValuesEqual, listConfigValues, booleanConfigValue,
and any related config formatting helper such as formatConfigValue into
src/lib/messaging/**, keeping channel-status.ts focused on orchestration only.
Then update channel-status.ts to import and use those helpers from their new
messaging location, preserving existing behavior while aligning with the layer
ownership rules.

Source: Path instructions


829-848: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff

All-channels mode re-reads the same config file per channel.

In the no---channel path, buildBasicChannelReport invokes buildConfigStatusSignals for every enabled channel, and readConfigSourceValues dedupes targets only within a single channel. Channels that share the same rendered target (e.g. openclaw.json) each trigger a separate cat sandbox exec, each bounded by CONFIG_STATUS_TIMEOUT_MS (5s). With several configured channels this multiplies sandbox round-trips for the same file on a common command. Consider a per-invocation read cache keyed by resolved target so each file is read at most once.

🤖 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/channel-status.ts` around lines 829 - 848, The
all-channels path is repeatedly reading the same rendered config target across
channels, causing duplicate sandbox calls and avoidable timeout overhead. Add a
per-invocation cache in the channel-status flow so shared reads are reused
across calls, using the resolved target as the key; update
readConfigSourceValues and the buildBasicChannelReport/buildConfigStatusSignals
path to consult that cache before calling deps.execSandbox. Keep the existing
per-source dedupe, but ensure each target is read at most once for the entire
status run.
🤖 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/discord/rendered-config-parser.ts`:
- Around line 71-83: The discordRequireMention helper currently returns an empty
array-like rendered value when no guild has requireMention configured, which
causes channel-status to treat it as a mismatch instead of unset. Update
discordRequireMention to return undefined when the filtered values list is
empty, while keeping the existing unique-value handling for populated cases so
it matches discordGuildUsers behavior.

In `@src/lib/messaging/channels/rendered-config-parser.ts`:
- Around line 14-26: The rendered-config parser lookup is using a separate
hand-maintained channel map, which can drift from the manifest-backed channel
registry. Update getBuiltInRenderedConfigParser and the
BUILT_IN_RENDERED_CONFIG_PARSERS logic so it derives the parser from the
manifest registry or is attached to the manifest definition itself, ensuring
channel-status uses one authoritative source for channel support and
diagnostics.

In `@src/lib/messaging/channels/telegram/rendered-config-parser.ts`:
- Around line 32-39: Hermes’ Telegram config parsing is missing the group-policy
source, so `channels status --channel telegram` cannot surface
`TELEGRAM_GROUP_POLICY` for Hermes sandboxes. Update `rendered-config-parser.ts`
in the `context.agentId === "hermes"` branch of the parser so it also returns a
config entry for `TELEGRAM_GROUP_POLICY`, alongside the existing `allowedIds`
and `requireMention` entries, and make sure it can resolve the value from
environment-based configuration as well as any structured config source used by
the Telegram channel status flow.

---

Nitpick comments:
In `@src/lib/actions/sandbox/channel-status.ts`:
- Around line 874-982: The rendered-config parsing and comparison helpers are
currently living in the actions layer, but they should be moved into the
side-effect-free messaging layer. Relocate parseEnvLines, unquoteEnvValue,
parseRenderedConfigSource, compareConfigSource, configValuesEqual,
listConfigValues, booleanConfigValue, and any related config formatting helper
such as formatConfigValue into src/lib/messaging/**, keeping channel-status.ts
focused on orchestration only. Then update channel-status.ts to import and use
those helpers from their new messaging location, preserving existing behavior
while aligning with the layer ownership rules.
- Around line 829-848: The all-channels path is repeatedly reading the same
rendered config target across channels, causing duplicate sandbox calls and
avoidable timeout overhead. Add a per-invocation cache in the channel-status
flow so shared reads are reused across calls, using the resolved target as the
key; update readConfigSourceValues and the
buildBasicChannelReport/buildConfigStatusSignals path to consult that cache
before calling deps.execSandbox. Keep the existing per-source dedupe, but ensure
each target is read at most once for the entire status run.
🪄 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: 81f19417-b7c2-4944-a2f6-f14eb255f979

📥 Commits

Reviewing files that changed from the base of the PR and between 09bef70 and c6f7b6c.

📒 Files selected for processing (19)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/commands/sandbox/channels/status.ts
  • src/lib/actions/sandbox/channel-status.test.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/actions/sandbox/process-recovery.test.ts
  • src/lib/actions/sandbox/process-recovery.ts
  • src/lib/cli/public-display-defaults.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.ts
  • src/lib/messaging/channels/index.ts
  • src/lib/messaging/channels/rendered-config-parser-utils.ts
  • src/lib/messaging/channels/rendered-config-parser.ts
  • src/lib/messaging/channels/slack/rendered-config-parser.ts
  • src/lib/messaging/channels/teams/rendered-config-parser.ts
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
  • src/lib/messaging/channels/wechat/rendered-config-parser.ts
  • src/lib/messaging/channels/whatsapp/rendered-config-parser.ts
  • test/cli/connect-recovery-settle.test.ts
  • test/cli/connect-recovery.test.ts

Comment thread src/lib/messaging/channels/discord/rendered-config-parser.ts
Comment thread src/lib/messaging/channels/rendered-config-parser.ts Outdated
Comment thread src/lib/messaging/channels/telegram/rendered-config-parser.ts
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-4: Test file not split per PRA-5 — 1389 lines in single describe block; then add or justify PRA-T1.
Open items: 3 required · 8 warnings · 2 suggestions · 8 test follow-ups
Since last review: 2 prior items resolved · 6 still apply · 4 new items found

Action checklist

  • PRA-4 Fix: Test file not split per PRA-5 — 1389 lines in single describe block in src/lib/actions/sandbox/channel-status.test.ts:1
  • PRA-5 Fix: Path traversal in resolveConfigTarget — /sandbox/ paths accepted without validation in src/lib/actions/sandbox/channel-status-config.ts:238
  • PRA-6 Fix: safeAccountId fails silently on invalid WeChat accountId — no diagnostic signal emitted in src/lib/messaging/channels/wechat/rendered-config-parser.ts:77
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/process-recovery.ts:133 buildSandboxExecMarkedCommand
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/channel-status-config.ts:238 resolveConfigTarget
  • PRA-3 Resolve or justify: Source-of-truth review needed: src/lib/messaging/channels/wechat/rendered-config-parser.ts:77 safeAccountId
  • PRA-7 Resolve or justify: booleanConfigValue normalizes string/boolean mismatches silently in src/lib/actions/sandbox/channel-status-config.ts:397
  • PRA-8 Resolve or justify: Documentation doesn't explicitly list channels supporting config comparison in docs/reference/commands.mdx:1200
  • PRA-9 Resolve or justify: buildSandboxExecMarkedCommand workaround lacks source-of-truth documentation in src/lib/actions/sandbox/process-recovery.ts:133
  • PRA-10 Resolve or justify: Parser framework adds 7 files of per-channel boilerplate; data-driven approach could eliminate most in src/lib/messaging/channels/rendered-config-parser-utils.ts:1
  • PRA-11 Resolve or justify: Missing negative-path tests for config comparison robustness in src/lib/actions/sandbox/channel-status.test.ts:808
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Missing negative-path tests for config comparison robustness
  • PRA-T6 Add or justify test follow-up: buildSandboxExecMarkedCommand tests only cover happy path
  • PRA-T7 Add or justify test follow-up: src/lib/actions/sandbox/process-recovery.ts:133 buildSandboxExecMarkedCommand
  • PRA-T8 Add or justify test follow-up: src/lib/actions/sandbox/channel-status-config.ts:238 resolveConfigTarget
  • PRA-12 In-scope improvement: quotePath uses inline single-quote escaping — trust assumption undocumented in src/lib/actions/sandbox/channel-status-config.ts:276
  • PRA-13 In-scope improvement: buildSandboxExecMarkedCommand tests only cover happy path in src/lib/actions/sandbox/process-recovery.test.ts:1

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Required architecture src/lib/actions/sandbox/channel-status.test.ts:1 Split the test file into four focused test files by feature area before merge. Preserve all existing tests.
PRA-5 Required security src/lib/actions/sandbox/channel-status-config.ts:238 Add path normalization and validation: resolve the path against the sandbox root (/sandbox) and ensure it stays within that directory using path.resolve and startsWith('/sandbox').
PRA-6 Required security src/lib/messaging/channels/wechat/rendered-config-parser.ts:77 Emit a DiagnosticSignal with severity 'warn' when safeAccountId returns undefined, e.g. 'WeChat account ID contains invalid characters; account-specific config not verified'.
PRA-7 Resolve/justify correctness src/lib/actions/sandbox/channel-status-config.ts:397 Document the normalization behavior explicitly in a comment. Consider adding a strict mode for security-sensitive configs, or at minimum log a debug note when normalization occurs.
PRA-8 Resolve/justify docs docs/reference/commands.mdx:1200 Update documentation to clarify: 'Config comparison currently supported for: telegram, teams, discord, slack, wechat. WhatsApp shows Hermes allowed-users only. Other channels show registration and policy status.'
PRA-9 Resolve/justify architecture src/lib/actions/sandbox/process-recovery.ts:133 Add a comment at the function explaining the five source-of-truth elements. The test 'keeps the OpenShell command argument newline-free while preserving multi-line payloads' serves as regression test.
PRA-10 Resolve/justify architecture src/lib/messaging/channels/rendered-config-parser-utils.ts:1 Consider extending ChannelManifest with a renderVisibility field that declaratively maps inputId to config paths per agent. If not feasible now, document why the parser abstraction is necessary.
PRA-11 Resolve/justify tests src/lib/actions/sandbox/channel-status.test.ts:808 Add the six missing negative-path tests in the new split test files (once PRA-5 is done).
PRA-12 Improvement correctness src/lib/actions/sandbox/channel-status-config.ts:276 Verify no user-controlled input reaches quotePath. If manifests are fully trusted (built-in only), this is acceptable. Document the trust assumption in a comment.
PRA-13 Improvement tests src/lib/actions/sandbox/process-recovery.test.ts:1 Add edge-case tests for buildSandboxExecMarkedCommand: base64 not available, empty string, single quotes in payload, large payloads.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-4 Required — Test file not split per PRA-5 — 1389 lines in single describe block

  • Location: src/lib/actions/sandbox/channel-status.test.ts:1
  • Category: architecture
  • Problem: channel-status.test.ts grew from 504 to 1389 lines (+885) and remains a single describe('showSandboxChannelStatus (whatsapp)', ...) block covering WhatsApp deep probe, all-channels summary, config comparison for 6 channels, and rendered parser behavior. Previous review PRA-5 required splitting into: channel-status-whatsapp.test.ts, channel-status-summary.test.ts, channel-status-config-comparison.test.ts, channel-status-rendered-parsers.test.ts.
  • Impact: Single large test file reduces readability, makes it harder to run targeted tests, obscures coverage gaps, and violates project pattern of focused test files.
  • Required action: Split the test file into four focused test files by feature area before merge. Preserve all existing tests.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/channel-status.test.ts shows 1389 lines; grep '^describe' shows only one describe block
  • Missing regression test: No new test needed — this is test organization. All existing tests must be preserved and redistributed.
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/channel-status.test.ts shows 1389 lines; grep '^describe' shows only one describe block.
  • Evidence: File grew from 504 to 1389 lines per drift context. Test blocks cover WhatsApp probe, summary mode, Telegram/Teams/Discord/Slack/WeChat config comparison, Hermes vs OpenClaw paths.

PRA-5 Required — Path traversal in resolveConfigTarget — /sandbox/ paths accepted without validation

  • Location: src/lib/actions/sandbox/channel-status-config.ts:238
  • Category: security
  • Problem: resolveConfigTarget returns target paths starting with '/sandbox/' directly without path normalization or traversal validation. A malicious or corrupted manifest could specify '/sandbox/../../etc/passwd' and escape the sandbox root when cat is executed via execSandbox.
  • Impact: Sandbox escape via path traversal in rendered config target paths. The execSandbox call at line 288 runs `cat ${quotePath(target)}` with the unvalidated path.
  • Required action: Add path normalization and validation: resolve the path against the sandbox root (/sandbox) and ensure it stays within that directory using path.resolve and startsWith('/sandbox').
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read resolveConfigTarget at line 238; observe the early return for '/sandbox/' prefix without validation
  • Missing regression test: Add test in channel-status-config-comparison.test.ts: manifest target '/sandbox/../../etc/passwd' is rejected/normalized and does not escape sandbox root
  • Done when: The required change is committed and verification passes: Read resolveConfigTarget at line 238; observe the early return for '/sandbox/' prefix without validation.
  • Evidence: Function returns target directly at line 245: `if (target.startsWith('/sandbox/')) return target;` — no path.resolve or containment check

PRA-6 Required — safeAccountId fails silently on invalid WeChat accountId — no diagnostic signal emitted

  • Location: src/lib/messaging/channels/wechat/rendered-config-parser.ts:77
  • Category: security
  • Problem: safeAccountId returns undefined for invalid account IDs (containing '..', path separators, control chars) without emitting any DiagnosticSignal. Operators won't know their WeChat account config cannot be verified, leading to silent config gaps.
  • Impact: Misconfiguration hidden from operators; WeChat account-specific config (baseUrl, userId) not compared without any warning.
  • Required action: Emit a DiagnosticSignal with severity 'warn' when safeAccountId returns undefined, e.g. 'WeChat account ID contains invalid characters; account-specific config not verified'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read safeAccountId at line 77; observe it returns undefined without any side effect or signal
  • Missing regression test: Add test in channel-status-config-comparison.test.ts: WeChat malicious accountId (e.g. '../../etc') emits warning signal and does not crash
  • Done when: The required change is committed and verification passes: Read safeAccountId at line 77; observe it returns undefined without any side effect or signal.
  • Evidence: Function at line 77-85 validates but returns undefined silently; caller at line 49-51 only pushes keys when accountId is truthy
Review findings by urgency: 3 required fixes, 8 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/process-recovery.ts:133 buildSandboxExecMarkedCommand

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: process-recovery.test.ts: 'keeps the OpenShell command argument newline-free while preserving multi-line payloads'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Function at line 133-145 base64-encodes multi-line commands; extractSandboxExecCommandStdout at line 90 parses framed output

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/channel-status-config.ts:238 resolveConfigTarget

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — add test in channel-status-config-comparison.test.ts for traversal rejection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 245: `if (target.startsWith('/sandbox/')) return target;` — no path.resolve or containment check

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/messaging/channels/wechat/rendered-config-parser.ts:77 safeAccountId

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — add test for malicious accountId emitting warning signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 77-85: returns undefined silently; line 49-51 only uses accountId if truthy, no warning emitted

PRA-7 Resolve/justify — booleanConfigValue normalizes string/boolean mismatches silently

  • Location: src/lib/actions/sandbox/channel-status-config.ts:397
  • Category: correctness
  • Problem: booleanConfigValue normalizes string '1'/'true' to boolean true and '0'/'false' to boolean false, causing registry string values to match rendered boolean values silently. This masks type mismatches that could indicate config drift.
  • Impact: Config drift between registry (strings) and rendered agent config (booleans) goes undetected. Operators see 'ok' when types actually differ.
  • Recommended action: Document the normalization behavior explicitly in a comment. Consider adding a strict mode for security-sensitive configs, or at minimum log a debug note when normalization occurs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read booleanConfigValue at line 397; observe string-to-boolean normalization
  • Missing regression test: Test already exists: 'treats 0/1 registry config as matching boolean rendered config' — add test for strict mode or debug logging when normalization occurs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read booleanConfigValue at line 397; observe string-to-boolean normalization.
  • Evidence: Lines 397-408: typeof value === 'string' branch normalizes '1'/'true'→true, '0'/'false'→false; returns null for other strings causing fallback to string comparison

PRA-8 Resolve/justify — Documentation doesn't explicitly list channels supporting config comparison

  • Location: docs/reference/commands.mdx:1200
  • Category: docs
  • Problem: Documentation describes config comparison generically but doesn't explicitly list which channels support it. Users cannot know Telegram/Teams/Discord/Slack/WeChat support config comparison while WhatsApp only shows Hermes allowed-users.
  • Impact: Operators cannot predict which channels will show config comparison vs only registration/policy status.
  • Recommended action: Update documentation to clarify: 'Config comparison currently supported for: telegram, teams, discord, slack, wechat. WhatsApp shows Hermes allowed-users only. Other channels show registration and policy status.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/reference/commands.mdx around line 1200; verify explicit channel list for config comparison
  • Missing regression test: No automated test — documentation accuracy check
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/reference/commands.mdx around line 1200; verify explicit channel list for config comparison.
  • Evidence: Current docs at line 1197-1205 describe feature generically without channel enumeration

PRA-9 Resolve/justify — buildSandboxExecMarkedCommand workaround lacks source-of-truth documentation

  • Location: src/lib/actions/sandbox/process-recovery.ts:133
  • Category: architecture
  • Problem: buildSandboxExecMarkedCommand workaround for multi-line commands lacks source-of-truth documentation explaining: (1) invalid state handled, (2) source boundary, (3) why not fixed at source, (4) regression test, (5) removal condition. Previous review PRA-1/PRA-6.
  • Impact: Future maintainers cannot evaluate whether the workaround is still needed or can be removed. Risk of cargo-culting the base64 encoding pattern.
  • Recommended action: Add a comment at the function explaining the five source-of-truth elements. The test 'keeps the OpenShell command argument newline-free while preserving multi-line payloads' serves as regression test.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read buildSandboxExecMarkedCommand at line 133 and extractSandboxExecCommandStdout; check for source-of-truth comment
  • Missing regression test: Test exists in process-recovery.test.ts: 'keeps the OpenShell command argument newline-free while preserving multi-line payloads'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read buildSandboxExecMarkedCommand at line 133 and extractSandboxExecCommandStdout; check for source-of-truth comment.
  • Evidence: Function at line 133-145 handles multi-line via base64; no comment explaining why OpenShell framing instability requires this

PRA-10 Resolve/justify — Parser framework adds 7 files of per-channel boilerplate; data-driven approach could eliminate most

  • Location: src/lib/messaging/channels/rendered-config-parser-utils.ts:1
  • Category: architecture
  • Problem: Rendered config parser framework adds 7 new files (utils, index, discord, slack, teams, telegram, wechat, whatsapp) of per-channel boilerplate. A data-driven approach extending ChannelManifest with renderVisibility could eliminate most parser files.
  • Impact: Increased maintenance burden; each new channel requires new parser file. Duplication of path/envKey mapping logic across parsers.
  • Recommended action: Consider extending ChannelManifest with a renderVisibility field that declaratively maps inputId to config paths per agent. If not feasible now, document why the parser abstraction is necessary.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Count parser files: ls src/lib/messaging/channels/*/rendered-config-parser.ts shows 6 channel parsers + utils + index
  • Missing regression test: No new test — architecture improvement
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Count parser files: ls src/lib/messaging/channels/*/rendered-config-parser.ts shows 6 channel parsers + utils + index.
  • Evidence: Each parser implements listConfigVisibilityKeys with hardcoded paths; structure is identical across channels

PRA-11 Resolve/justify — Missing negative-path tests for config comparison robustness

  • Location: src/lib/actions/sandbox/channel-status.test.ts:808
  • Category: tests
  • Problem: Negative-path tests for config comparison robustness are incomplete. Missing: (1) malformed JSON/YAML in rendered config produces warning not crash, (2) secret input never appears in any output format, (3) type mismatch string vs number in rendered config, (4) path traversal in manifest target rejected, (5) WeChat malicious accountId emits warning, (6) channel without parser shows registration/policy only.
  • Impact: Security and correctness gaps not caught by tests. Path traversal (PRA-7) and silent WeChat failure (PRA-8) have no test coverage.
  • Recommended action: Add the six missing negative-path tests in the new split test files (once PRA-5 is done).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search channel-status.test.ts for malformed, secret, type mismatch, traversal, WeChat warning, no-parser tests
  • Missing regression test: Six specific negative-path tests: (1) malformed JSON/YAML → warn not crash, (2) secret inputs absent from output, (3) string vs number mismatch detected, (4) '/sandbox/../../etc/passwd' rejected, (5) WeChat '../../etc' accountId emits warn, (6) unknown channel shows registration/policy only
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search channel-status.test.ts for malformed, secret, type mismatch, traversal, WeChat warning, no-parser tests.
  • Evidence: Current tests cover happy paths: match, mismatch, unreadable, defaults, boolean normalization. No negative-path coverage.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-12 Improvement — quotePath uses inline single-quote escaping — trust assumption undocumented

  • Location: src/lib/actions/sandbox/channel-status-config.ts:276
  • Category: correctness
  • Problem: quotePath uses inline single-quote escaping for shell commands. While paths come from trusted manifests, this is a shell-injection-adjacent pattern. Consider using a centralized shell quoting utility if one exists.
  • Impact: If manifest paths ever become user-influenced, shell injection risk. Current trust boundary (built-in manifests only) should be documented.
  • Suggested action: Verify no user-controlled input reaches quotePath. If manifests are fully trusted (built-in only), this is acceptable. Document the trust assumption in a comment.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Trace quotePath callers: only called with target paths from manifests via resolveRenderedConfigSources
  • Missing regression test: No new test — trust boundary verification
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: quotePath at line 276 called only from readConfigSourceValues line 288 with target from resolveRenderedConfigSources which sources from manifest

PRA-13 Improvement — buildSandboxExecMarkedCommand tests only cover happy path

  • Location: src/lib/actions/sandbox/process-recovery.test.ts:1
  • Category: tests
  • Problem: New tests for buildSandboxExecMarkedCommand are good but only cover happy path. Missing: base64 missing handling, empty payload, payload with single quotes, very large payloads.
  • Impact: Edge cases in command wrapping could cause probe failures in production (e.g., base64 not available in minimal containers).
  • Suggested action: Add edge-case tests for buildSandboxExecMarkedCommand: base64 not available, empty string, single quotes in payload, large payloads.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read process-recovery.test.ts describe('sandbox exec command wrapping'); observe only two tests
  • Missing regression test: Four additional edge-case tests for buildSandboxExecMarkedCommand: (1) base64 missing → NEMOCLAW_BASE64_MISSING, (2) empty payload, (3) single quotes in payload, (4) large payload (>8KB)
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Only two tests: single-line readable, multi-line base64'd. No error-path coverage.
Simplification opportunities: 8 possible cuts, net -387 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-4 shrink (src/lib/actions/sandbox/channel-status.test.ts:1): Single 1389-line test file with one describe block
    • Replacement: Four focused test files: channel-status-whatsapp.test.ts, channel-status-summary.test.ts, channel-status-config-comparison.test.ts, channel-status-rendered-parsers.test.ts
    • Net: 0 lines
    • Safety boundary: All existing test behavior must be preserved; no test coverage reduction
  • PRA-5 stdlib (src/lib/actions/sandbox/channel-status-config.ts:238): Direct return of '/sandbox/' prefixed paths
    • Replacement: path.resolve(target) with startsWith('/sandbox') containment check using Node.js path module
    • Net: 3 lines
    • Safety boundary: Must not break legitimate /sandbox/ paths from built-in manifests
  • PRA-6 native (src/lib/messaging/channels/wechat/rendered-config-parser.ts:77): Silent undefined return
    • Replacement: Return object { accountId: string | null, warning?: DiagnosticSignal } or emit signal via callback
    • Net: 5 lines
    • Safety boundary: Must not break existing parser interface; DiagnosticSignal type already imported
  • PRA-7 shrink (src/lib/actions/sandbox/channel-status-config.ts:397): Implicit normalization without visibility
    • Replacement: Explicit comment documenting normalization; optional debug log when normalization occurs
    • Net: 2 lines
    • Safety boundary: Must not change comparison semantics — only add observability
  • PRA-8 shrink (docs/reference/commands.mdx:1200): Generic 'For registered channels, the status output also compares...'
    • Replacement: Explicit channel list: 'Config comparison currently supported for: telegram, teams, discord, slack, wechat. WhatsApp shows Hermes allowed-users only. Other channels show registration and policy status.'
    • Net: 0 lines
    • Safety boundary: Documentation only — no code behavior change
  • PRA-9 native (src/lib/actions/sandbox/process-recovery.ts:133): Undocumented workaround
    • Replacement: Comment block with: invalid state, source boundary, source-fix constraint, regression test reference, removal condition
    • Net: 8 lines
    • Safety boundary: Documentation only — no behavior change
  • PRA-10 yagni (src/lib/messaging/channels/rendered-config-parser-utils.ts:1): 6 per-channel parser files with repeated listConfigVisibilityKeys/getValue implementations
    • Replacement: ChannelManifest.renderVisibility declarative mapping + single generic parser
    • Net: -400 lines
    • Safety boundary: Must preserve all current parsing behavior for Telegram, Teams, Slack, Discord, WeChat, WhatsApp
  • PRA-12 stdlib (src/lib/actions/sandbox/channel-status-config.ts:276): Inline single-quote escaping implementation
    • Replacement: Use existing runner.shellQuote if available, or document trust boundary explicitly
    • Net: -5 lines
    • Safety boundary: Must only be called with paths derived from built-in channel manifests
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Integration test: channel status config comparison against real sandbox with rendered OpenClaw/Hermes config. Runtime/sandbox/infrastructure paths need behavioral runtime validation: config comparison reads sandbox files via execSandbox (cat), WhatsApp probe runs in-sandbox script, process-recovery uses SSH fallback. Unit tests mock execSandbox but integration tests with real sandbox would validate timeout bounds, path resolution, and shell quoting behavior.
  • PRA-T2 Runtime validation — Integration test: WhatsApp deep probe against real sandbox with paired/unpaired states. Runtime/sandbox/infrastructure paths need behavioral runtime validation: config comparison reads sandbox files via execSandbox (cat), WhatsApp probe runs in-sandbox script, process-recovery uses SSH fallback. Unit tests mock execSandbox but integration tests with real sandbox would validate timeout bounds, path resolution, and shell quoting behavior.
  • PRA-T3 Runtime validation — Integration test: buildSandboxExecMarkedCommand with real openshell sandbox exec (multi-line, base64 missing, large payload). Runtime/sandbox/infrastructure paths need behavioral runtime validation: config comparison reads sandbox files via execSandbox (cat), WhatsApp probe runs in-sandbox script, process-recovery uses SSH fallback. Unit tests mock execSandbox but integration tests with real sandbox would validate timeout bounds, path resolution, and shell quoting behavior.
  • PRA-T4 Runtime validation — Integration test: path traversal rejection in resolveConfigTarget with real sandbox exec. Runtime/sandbox/infrastructure paths need behavioral runtime validation: config comparison reads sandbox files via execSandbox (cat), WhatsApp probe runs in-sandbox script, process-recovery uses SSH fallback. Unit tests mock execSandbox but integration tests with real sandbox would validate timeout bounds, path resolution, and shell quoting behavior.
  • PRA-T5 Missing negative-path tests for config comparison robustness — Add the six missing negative-path tests in the new split test files (once PRA-5 is done).
  • PRA-T6 buildSandboxExecMarkedCommand tests only cover happy path — Add edge-case tests for buildSandboxExecMarkedCommand: base64 not available, empty string, single quotes in payload, large payloads.
  • PRA-T7 src/lib/actions/sandbox/process-recovery.ts:133 buildSandboxExecMarkedCommand — process-recovery.test.ts: 'keeps the OpenShell command argument newline-free while preserving multi-line payloads'. Function at line 133-145 base64-encodes multi-line commands; extractSandboxExecCommandStdout at line 90 parses framed output
  • PRA-T8 src/lib/actions/sandbox/channel-status-config.ts:238 resolveConfigTarget — Missing — add test in channel-status-config-comparison.test.ts for traversal rejection. Line 245: `if (target.startsWith('/sandbox/')) return target;` — no path.resolve or containment check
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/process-recovery.ts:133 buildSandboxExecMarkedCommand

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: process-recovery.test.ts: 'keeps the OpenShell command argument newline-free while preserving multi-line payloads'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Function at line 133-145 base64-encodes multi-line commands; extractSandboxExecCommandStdout at line 90 parses framed output

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/channel-status-config.ts:238 resolveConfigTarget

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — add test in channel-status-config-comparison.test.ts for traversal rejection
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 245: `if (target.startsWith('/sandbox/')) return target;` — no path.resolve or containment check

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/messaging/channels/wechat/rendered-config-parser.ts:77 safeAccountId

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Missing — add test for malicious accountId emitting warning signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: Line 77-85: returns undefined silently; line 49-51 only uses accountId if truthy, no warning emitted

PRA-4 Required — Test file not split per PRA-5 — 1389 lines in single describe block

  • Location: src/lib/actions/sandbox/channel-status.test.ts:1
  • Category: architecture
  • Problem: channel-status.test.ts grew from 504 to 1389 lines (+885) and remains a single describe('showSandboxChannelStatus (whatsapp)', ...) block covering WhatsApp deep probe, all-channels summary, config comparison for 6 channels, and rendered parser behavior. Previous review PRA-5 required splitting into: channel-status-whatsapp.test.ts, channel-status-summary.test.ts, channel-status-config-comparison.test.ts, channel-status-rendered-parsers.test.ts.
  • Impact: Single large test file reduces readability, makes it harder to run targeted tests, obscures coverage gaps, and violates project pattern of focused test files.
  • Required action: Split the test file into four focused test files by feature area before merge. Preserve all existing tests.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/channel-status.test.ts shows 1389 lines; grep '^describe' shows only one describe block
  • Missing regression test: No new test needed — this is test organization. All existing tests must be preserved and redistributed.
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/channel-status.test.ts shows 1389 lines; grep '^describe' shows only one describe block.
  • Evidence: File grew from 504 to 1389 lines per drift context. Test blocks cover WhatsApp probe, summary mode, Telegram/Teams/Discord/Slack/WeChat config comparison, Hermes vs OpenClaw paths.

PRA-5 Required — Path traversal in resolveConfigTarget — /sandbox/ paths accepted without validation

  • Location: src/lib/actions/sandbox/channel-status-config.ts:238
  • Category: security
  • Problem: resolveConfigTarget returns target paths starting with '/sandbox/' directly without path normalization or traversal validation. A malicious or corrupted manifest could specify '/sandbox/../../etc/passwd' and escape the sandbox root when cat is executed via execSandbox.
  • Impact: Sandbox escape via path traversal in rendered config target paths. The execSandbox call at line 288 runs `cat ${quotePath(target)}` with the unvalidated path.
  • Required action: Add path normalization and validation: resolve the path against the sandbox root (/sandbox) and ensure it stays within that directory using path.resolve and startsWith('/sandbox').
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read resolveConfigTarget at line 238; observe the early return for '/sandbox/' prefix without validation
  • Missing regression test: Add test in channel-status-config-comparison.test.ts: manifest target '/sandbox/../../etc/passwd' is rejected/normalized and does not escape sandbox root
  • Done when: The required change is committed and verification passes: Read resolveConfigTarget at line 238; observe the early return for '/sandbox/' prefix without validation.
  • Evidence: Function returns target directly at line 245: `if (target.startsWith('/sandbox/')) return target;` — no path.resolve or containment check

PRA-6 Required — safeAccountId fails silently on invalid WeChat accountId — no diagnostic signal emitted

  • Location: src/lib/messaging/channels/wechat/rendered-config-parser.ts:77
  • Category: security
  • Problem: safeAccountId returns undefined for invalid account IDs (containing '..', path separators, control chars) without emitting any DiagnosticSignal. Operators won't know their WeChat account config cannot be verified, leading to silent config gaps.
  • Impact: Misconfiguration hidden from operators; WeChat account-specific config (baseUrl, userId) not compared without any warning.
  • Required action: Emit a DiagnosticSignal with severity 'warn' when safeAccountId returns undefined, e.g. 'WeChat account ID contains invalid characters; account-specific config not verified'.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read safeAccountId at line 77; observe it returns undefined without any side effect or signal
  • Missing regression test: Add test in channel-status-config-comparison.test.ts: WeChat malicious accountId (e.g. '../../etc') emits warning signal and does not crash
  • Done when: The required change is committed and verification passes: Read safeAccountId at line 77; observe it returns undefined without any side effect or signal.
  • Evidence: Function at line 77-85 validates but returns undefined silently; caller at line 49-51 only pushes keys when accountId is truthy

PRA-7 Resolve/justify — booleanConfigValue normalizes string/boolean mismatches silently

  • Location: src/lib/actions/sandbox/channel-status-config.ts:397
  • Category: correctness
  • Problem: booleanConfigValue normalizes string '1'/'true' to boolean true and '0'/'false' to boolean false, causing registry string values to match rendered boolean values silently. This masks type mismatches that could indicate config drift.
  • Impact: Config drift between registry (strings) and rendered agent config (booleans) goes undetected. Operators see 'ok' when types actually differ.
  • Recommended action: Document the normalization behavior explicitly in a comment. Consider adding a strict mode for security-sensitive configs, or at minimum log a debug note when normalization occurs.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read booleanConfigValue at line 397; observe string-to-boolean normalization
  • Missing regression test: Test already exists: 'treats 0/1 registry config as matching boolean rendered config' — add test for strict mode or debug logging when normalization occurs
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read booleanConfigValue at line 397; observe string-to-boolean normalization.
  • Evidence: Lines 397-408: typeof value === 'string' branch normalizes '1'/'true'→true, '0'/'false'→false; returns null for other strings causing fallback to string comparison

PRA-8 Resolve/justify — Documentation doesn't explicitly list channels supporting config comparison

  • Location: docs/reference/commands.mdx:1200
  • Category: docs
  • Problem: Documentation describes config comparison generically but doesn't explicitly list which channels support it. Users cannot know Telegram/Teams/Discord/Slack/WeChat support config comparison while WhatsApp only shows Hermes allowed-users.
  • Impact: Operators cannot predict which channels will show config comparison vs only registration/policy status.
  • Recommended action: Update documentation to clarify: 'Config comparison currently supported for: telegram, teams, discord, slack, wechat. WhatsApp shows Hermes allowed-users only. Other channels show registration and policy status.'
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read docs/reference/commands.mdx around line 1200; verify explicit channel list for config comparison
  • Missing regression test: No automated test — documentation accuracy check
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read docs/reference/commands.mdx around line 1200; verify explicit channel list for config comparison.
  • Evidence: Current docs at line 1197-1205 describe feature generically without channel enumeration

PRA-9 Resolve/justify — buildSandboxExecMarkedCommand workaround lacks source-of-truth documentation

  • Location: src/lib/actions/sandbox/process-recovery.ts:133
  • Category: architecture
  • Problem: buildSandboxExecMarkedCommand workaround for multi-line commands lacks source-of-truth documentation explaining: (1) invalid state handled, (2) source boundary, (3) why not fixed at source, (4) regression test, (5) removal condition. Previous review PRA-1/PRA-6.
  • Impact: Future maintainers cannot evaluate whether the workaround is still needed or can be removed. Risk of cargo-culting the base64 encoding pattern.
  • Recommended action: Add a comment at the function explaining the five source-of-truth elements. The test 'keeps the OpenShell command argument newline-free while preserving multi-line payloads' serves as regression test.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read buildSandboxExecMarkedCommand at line 133 and extractSandboxExecCommandStdout; check for source-of-truth comment
  • Missing regression test: Test exists in process-recovery.test.ts: 'keeps the OpenShell command argument newline-free while preserving multi-line payloads'
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read buildSandboxExecMarkedCommand at line 133 and extractSandboxExecCommandStdout; check for source-of-truth comment.
  • Evidence: Function at line 133-145 handles multi-line via base64; no comment explaining why OpenShell framing instability requires this

PRA-10 Resolve/justify — Parser framework adds 7 files of per-channel boilerplate; data-driven approach could eliminate most

  • Location: src/lib/messaging/channels/rendered-config-parser-utils.ts:1
  • Category: architecture
  • Problem: Rendered config parser framework adds 7 new files (utils, index, discord, slack, teams, telegram, wechat, whatsapp) of per-channel boilerplate. A data-driven approach extending ChannelManifest with renderVisibility could eliminate most parser files.
  • Impact: Increased maintenance burden; each new channel requires new parser file. Duplication of path/envKey mapping logic across parsers.
  • Recommended action: Consider extending ChannelManifest with a renderVisibility field that declaratively maps inputId to config paths per agent. If not feasible now, document why the parser abstraction is necessary.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Count parser files: ls src/lib/messaging/channels/*/rendered-config-parser.ts shows 6 channel parsers + utils + index
  • Missing regression test: No new test — architecture improvement
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Count parser files: ls src/lib/messaging/channels/*/rendered-config-parser.ts shows 6 channel parsers + utils + index.
  • Evidence: Each parser implements listConfigVisibilityKeys with hardcoded paths; structure is identical across channels

PRA-11 Resolve/justify — Missing negative-path tests for config comparison robustness

  • Location: src/lib/actions/sandbox/channel-status.test.ts:808
  • Category: tests
  • Problem: Negative-path tests for config comparison robustness are incomplete. Missing: (1) malformed JSON/YAML in rendered config produces warning not crash, (2) secret input never appears in any output format, (3) type mismatch string vs number in rendered config, (4) path traversal in manifest target rejected, (5) WeChat malicious accountId emits warning, (6) channel without parser shows registration/policy only.
  • Impact: Security and correctness gaps not caught by tests. Path traversal (PRA-7) and silent WeChat failure (PRA-8) have no test coverage.
  • Recommended action: Add the six missing negative-path tests in the new split test files (once PRA-5 is done).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search channel-status.test.ts for malformed, secret, type mismatch, traversal, WeChat warning, no-parser tests
  • Missing regression test: Six specific negative-path tests: (1) malformed JSON/YAML → warn not crash, (2) secret inputs absent from output, (3) string vs number mismatch detected, (4) '/sandbox/../../etc/passwd' rejected, (5) WeChat '../../etc' accountId emits warn, (6) unknown channel shows registration/policy only
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search channel-status.test.ts for malformed, secret, type mismatch, traversal, WeChat warning, no-parser tests.
  • Evidence: Current tests cover happy paths: match, mismatch, unreadable, defaults, boolean normalization. No negative-path coverage.

PRA-12 Improvement — quotePath uses inline single-quote escaping — trust assumption undocumented

  • Location: src/lib/actions/sandbox/channel-status-config.ts:276
  • Category: correctness
  • Problem: quotePath uses inline single-quote escaping for shell commands. While paths come from trusted manifests, this is a shell-injection-adjacent pattern. Consider using a centralized shell quoting utility if one exists.
  • Impact: If manifest paths ever become user-influenced, shell injection risk. Current trust boundary (built-in manifests only) should be documented.
  • Suggested action: Verify no user-controlled input reaches quotePath. If manifests are fully trusted (built-in only), this is acceptable. Document the trust assumption in a comment.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Trace quotePath callers: only called with target paths from manifests via resolveRenderedConfigSources
  • Missing regression test: No new test — trust boundary verification
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: quotePath at line 276 called only from readConfigSourceValues line 288 with target from resolveRenderedConfigSources which sources from manifest

PRA-13 Improvement — buildSandboxExecMarkedCommand tests only cover happy path

  • Location: src/lib/actions/sandbox/process-recovery.test.ts:1
  • Category: tests
  • Problem: New tests for buildSandboxExecMarkedCommand are good but only cover happy path. Missing: base64 missing handling, empty payload, payload with single quotes, very large payloads.
  • Impact: Edge cases in command wrapping could cause probe failures in production (e.g., base64 not available in minimal containers).
  • Suggested action: Add edge-case tests for buildSandboxExecMarkedCommand: base64 not available, empty string, single quotes in payload, large payloads.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Read process-recovery.test.ts describe('sandbox exec command wrapping'); observe only two tests
  • Missing regression test: Four additional edge-case tests for buildSandboxExecMarkedCommand: (1) base64 missing → NEMOCLAW_BASE64_MISSING, (2) empty payload, (3) single quotes in payload, (4) large payload (>8KB)
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Only two tests: single-line readable, multi-line base64'd. No error-path coverage.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

Signed-off-by: San Dang <sdang@nvidia.com>
@copy-pr-bot

copy-pr-bot Bot commented Jun 30, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@sandl99

sandl99 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #6044. The replacement branch has the same final changes committed as a single GitHub-verified commit; this PR included an unsigned low-level follow-up commit after force-push was blocked.

@sandl99 sandl99 closed this Jun 30, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/lib/messaging/channels/manifests.test.ts (1)

225-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

This alignment test is self-referential.

Both the expected value and getBuiltInRenderedConfigParser() are driven by BUILT_IN_CHANNEL_MANIFESTS, so the assertion still passes if a built-in channel silently drops out of that list. Please anchor this to an external contract instead—e.g. assert the known built-in ids, or exercise a public channel-status path that proves each built-in manifest reaches a parser.

As per path instructions, "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."

🤖 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/manifests.test.ts` around lines 225 - 232, The
alignment test in manifests.test.ts is self-referential because both sides
derive from BUILT_IN_CHANNEL_MANIFESTS, so it cannot detect a missing built-in
manifest. Update the test around getBuiltInRenderedConfigParser to anchor on an
external contract by asserting the known built-in channel ids or by exercising
the public channel-status path that proves each built-in manifest is wired to a
parser, rather than comparing BUILT_IN_CHANNEL_MANIFESTS to itself.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/lib/messaging/channels/manifests.test.ts`:
- Around line 225-232: The alignment test in manifests.test.ts is
self-referential because both sides derive from BUILT_IN_CHANNEL_MANIFESTS, so
it cannot detect a missing built-in manifest. Update the test around
getBuiltInRenderedConfigParser to anchor on an external contract by asserting
the known built-in channel ids or by exercising the public channel-status path
that proves each built-in manifest is wired to a parser, rather than comparing
BUILT_IN_CHANNEL_MANIFESTS to itself.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8238ec44-d790-419c-a255-7a7245b52e9c

📥 Commits

Reviewing files that changed from the base of the PR and between c6f7b6c and 1ef6b52.

📒 Files selected for processing (8)
  • src/lib/actions/sandbox/channel-status-config.ts
  • src/lib/actions/sandbox/channel-status.test.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.test.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.ts
  • src/lib/messaging/channels/manifests.test.ts
  • src/lib/messaging/channels/rendered-config-parser.ts
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
🚧 Files skipped from review as they are similar to previous changes (4)
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
  • src/lib/messaging/channels/rendered-config-parser.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.ts
  • src/lib/actions/sandbox/channel-status.test.ts

apurvvkumaria pushed a commit that referenced this pull request Jul 1, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a manifest-backed `channels status` configuration summary that
compares sandbox registry inputs against the rendered OpenClaw/Hermes
channel config. This replaces the narrower Telegram-only approach in
#5705 with per-channel parsers for rendered config sources and keeps
`doctor` unchanged.

```
➜  NemoClaw git:(feat/channels-status-config-values) ✗ nemoclaw tm channels status                 

  NemoClaw channels status: tm / telegram
    [ok] Channel registration: telegram registered
    [ok] Policy coverage: telegram preset applied
    [ok] Telegram User ID (for DM access) (TELEGRAM_ALLOWED_IDS): 7895072570
    [ok] Telegram group policy (TELEGRAM_GROUP_POLICY): open
    [info] Deep diagnostics: not implemented for telegram; see `nemoclaw tm doctor` and `nemoclaw tm logs --follow`
```
## Related Issue
Fixes #5695.
Supersedes #5705 and #6033.

## Changes
- Add a central rendered-config parser registry plus per-channel parsers
for Telegram, Teams, Slack, Discord, WeChat, and WhatsApp.
- Split rendered-config status comparison into a dedicated channel
status config module.
- Show configured, non-secret channel values as `Label (ENV_KEY): value`
and mark mismatches against rendered agent config as warnings.
- Make no-arg `channels status` print a compact summary for configured
channels instead of silently defaulting to one channel.
- Keep WhatsApp on its existing deep diagnostics path and keep `doctor`
behavior unchanged.
- Make sandbox exec wrapping newline-safe while keeping single-line
commands readable for existing recovery/fallback behavior.
- Update generated command reference docs for the channel status help
text.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: scoped to read-only
channel status rendering plus the sandbox exec newline wrapper; no
credential values are persisted or newly exposed beyond existing
non-secret status visibility.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Verification notes:
- Passed: `npx vitest run --project cli
src/lib/actions/sandbox/channel-status.test.ts
src/lib/messaging/channels/discord/rendered-config-parser.test.ts
src/lib/messaging/channels/manifests.test.ts
src/lib/actions/sandbox/process-recovery.test.ts`
- Passed: `npx vitest run --project integration
test/process-recovery.test.ts test/cli/connect-recovery.test.ts
test/cli/connect-recovery-settle.test.ts`
- Passed: `npx vitest run --project cli
src/lib/actions/sandbox/auto-pair-approval.test.ts`
- Passed: `npx vitest run --project integration
test/sandbox-connect-inference/auto-pair-approval.test.ts`
- Passed: `npm run typecheck:cli`
- Passed: `npm run build:cli`
- Passed: `npm run test-conditionals:scan -- --top 25`
- Passed: targeted `npx biome check` on modified source/test files
- `npm run docs` completed with 0 errors and 2 existing warnings.
- `npx prek run --from-ref main --to-ref HEAD` did not pass locally;
remaining failures were unrelated local broad-lane issues
(`langchain-deepagents-code-image` `${1^^}` under `sh`, sandbox rlimit
expectations, and two CLI timeout flakes that passed isolated as listed
above).

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: San Dang <sdang@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* `channels status` now shows a compact status summary for all
configured messaging channels by default, with detailed diagnostics when
`--channel` is specified.
* Status reporting includes messaging policy coverage and non-secret
rendered-config comparisons; WhatsApp detailed output expands runtime
probing (QR/session, Noise connectivity, inbound delivery) plus policy
coverage.
* **Bug Fixes**
* Providing an unknown `--channel` now returns a clear “unknown channel”
error and exits non-zero.
* WhatsApp detailed checks now correctly yield a non-zero verdict for
the idle (no inbound delivery observed) case.
* **Documentation**
* Updated command help and reference docs to clarify compact vs detailed
output, `--channel`/`--json` behavior, and WhatsApp probe scope.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
@sandl99
sandl99 deleted the feat/channels-status-config-values branch July 8, 2026 03:19
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
Adds a manifest-backed `channels status` configuration summary that
compares sandbox registry inputs against the rendered OpenClaw/Hermes
channel config. This replaces the narrower Telegram-only approach in
NVIDIA#5705 with per-channel parsers for rendered config sources and keeps
`doctor` unchanged.

```
➜  NemoClaw git:(feat/channels-status-config-values) ✗ nemoclaw tm channels status                 

  NemoClaw channels status: tm / telegram
    [ok] Channel registration: telegram registered
    [ok] Policy coverage: telegram preset applied
    [ok] Telegram User ID (for DM access) (TELEGRAM_ALLOWED_IDS): 7895072570
    [ok] Telegram group policy (TELEGRAM_GROUP_POLICY): open
    [info] Deep diagnostics: not implemented for telegram; see `nemoclaw tm doctor` and `nemoclaw tm logs --follow`
```
## Related Issue
Fixes NVIDIA#5695.
Supersedes NVIDIA#5705 and NVIDIA#6033.

## Changes
- Add a central rendered-config parser registry plus per-channel parsers
for Telegram, Teams, Slack, Discord, WeChat, and WhatsApp.
- Split rendered-config status comparison into a dedicated channel
status config module.
- Show configured, non-secret channel values as `Label (ENV_KEY): value`
and mark mismatches against rendered agent config as warnings.
- Make no-arg `channels status` print a compact summary for configured
channels instead of silently defaulting to one channel.
- Keep WhatsApp on its existing deep diagnostics path and keep `doctor`
behavior unchanged.
- Make sandbox exec wrapping newline-safe while keeping single-line
commands readable for existing recovery/fallback behavior.
- Update generated command reference docs for the channel status help
text.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates
<!-- Check all that apply. For any "covered by existing tests", "not
applicable", or waiver entry, add a brief justification on the same line
or in the Changes section. -->
- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification: scoped to read-only
channel status rendering plus the sandbox exec newline wrapper; no
credential values are persisted or newly exposed beyond existing
non-secret status visibility.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each item you ran and confirmed. Leave unchecked items you
skipped. Doc-only changes do not require npm test unless you ran it. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [ ] Git hooks passed during commit and push, or `npx prek run
--from-ref main --to-ref HEAD` passes
- [x] Targeted tests pass for changed behavior
- [ ] Full `npm test` passes (broad runtime changes only)
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only)
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

Verification notes:
- Passed: `npx vitest run --project cli
src/lib/actions/sandbox/channel-status.test.ts
src/lib/messaging/channels/discord/rendered-config-parser.test.ts
src/lib/messaging/channels/manifests.test.ts
src/lib/actions/sandbox/process-recovery.test.ts`
- Passed: `npx vitest run --project integration
test/process-recovery.test.ts test/cli/connect-recovery.test.ts
test/cli/connect-recovery-settle.test.ts`
- Passed: `npx vitest run --project cli
src/lib/actions/sandbox/auto-pair-approval.test.ts`
- Passed: `npx vitest run --project integration
test/sandbox-connect-inference/auto-pair-approval.test.ts`
- Passed: `npm run typecheck:cli`
- Passed: `npm run build:cli`
- Passed: `npm run test-conditionals:scan -- --top 25`
- Passed: targeted `npx biome check` on modified source/test files
- `npm run docs` completed with 0 errors and 2 existing warnings.
- `npx prek run --from-ref main --to-ref HEAD` did not pass locally;
remaining failures were unrelated local broad-lane issues
(`langchain-deepagents-code-image` `${1^^}` under `sh`, sandbox rlimit
expectations, and two CLI timeout flakes that passed isolated as listed
above).

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: San Dang <sdang@nvidia.com>


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* `channels status` now shows a compact status summary for all
configured messaging channels by default, with detailed diagnostics when
`--channel` is specified.
* Status reporting includes messaging policy coverage and non-secret
rendered-config comparisons; WhatsApp detailed output expands runtime
probing (QR/session, Noise connectivity, inbound delivery) plus policy
coverage.
* **Bug Fixes**
* Providing an unknown `--channel` now returns a clear “unknown channel”
error and exits non-zero.
* WhatsApp detailed checks now correctly yield a non-zero verdict for
the idle (no inbound delivery observed) case.
* **Documentation**
* Updated command help and reference docs to clarify compact vs detailed
output, `--channel`/`--json` behavior, and WhatsApp probe scope.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: messaging Messaging channels, bridges, manifests, or channel lifecycle enhancement New capability or improvement request

Projects

None yet

1 participant