Skip to content

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

Merged
apurvvkumaria merged 15 commits into
mainfrom
feat/channels-status-config-summary
Jul 1, 2026
Merged

feat(cli): add channel config status summary#6044
apurvvkumaria merged 15 commits into
mainfrom
feat/channels-status-config-summary

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.

➜  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.
Fixes #5691
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)
  • 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/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).

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

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.

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 self-assigned this Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds rendered-config parsers and sandbox channel-status comparison signals, expands channels status to bulk all-channel reporting, and exports sandbox exec command wrapping with updated recovery tests and docs.

Changes

Channel Status Config Diagnostics & Bulk Reporting

Layer / File(s) Summary
Parser 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 parser types and helpers, registers built-in parsers by manifest id, re-exports parser utilities, and adds manifest coverage for parser presence and visibility-key filtering.
Per-channel rendered config parsers
src/lib/messaging/channels/discord/rendered-config-parser.ts, src/lib/messaging/channels/discord/rendered-config-parser.test.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
Implements Discord, Slack, Teams, Telegram, WeChat, and WhatsApp rendered-config parsers with agent-specific visibility keys and value extraction, plus Discord parser tests covering mention-policy extraction cases.
Config status comparison and signal formatting
src/lib/actions/sandbox/channel-status-config-values.ts, src/lib/actions/sandbox/channel-status-config.ts, src/lib/actions/sandbox/channel-status-config-values.test.ts, src/lib/actions/sandbox/channel-status-config-core.test.ts, src/lib/actions/sandbox/channel-status-config-channels.test.ts, src/lib/actions/sandbox/channel-status-telegram-policy.test.ts, src/lib/actions/sandbox/channel-status-summary.test.ts
Adds config value formatting/equality helpers, sandbox config comparison wiring, and test coverage for formatting, read/parse handling, cross-channel comparisons, Telegram group policy output, and summary rendering.
Bulk channel status rendering and tests
src/lib/actions/sandbox/channel-status.ts, src/lib/actions/sandbox/channel-status.test-helpers.ts, src/lib/actions/sandbox/channel-status.test.ts, src/commands/sandbox/channels/status.ts, src/lib/cli/public-display-defaults.ts, docs/reference/commands.mdx, docs/reference/commands-nemohermes.mdx
Extends channel status reporting to render bulk all-channel results when --channel is omitted, adds unknown-channel handling, updates the test harness and WhatsApp call sites, and refreshes command help, public display defaults, and reference docs.

Exec Command Wrapping and Recovery Test Decoding

Layer / File(s) Summary
Sandbox exec command wrapping
src/lib/actions/sandbox/process-recovery.ts, src/lib/actions/sandbox/process-recovery.test.ts
Exports the sandbox exec command wrapper, changes the fast-path condition to use newline detection and the Hermes validator script, and adds tests for single-line wrapping, multi-line encoding, and raw-path validator handling.
Connect recovery test command decoding
test/cli/connect-recovery.test.ts, test/cli/connect-recovery-settle.test.ts
Adds a shell decode helper and updates openshell mock scripts to decode wrapped sandbox exec commands before matching recovery and probe behavior.

Sequence Diagram(s)

sequenceDiagram
  participant showSandboxChannelStatus
  participant buildConfigStatusSignals
  participant getBuiltInRenderedConfigParser
  participant execSandbox

  showSandboxChannelStatus->>buildConfigStatusSignals: build channel diagnostics
  buildConfigStatusSignals->>getBuiltInRenderedConfigParser: select parser by channel id
  buildConfigStatusSignals->>execSandbox: read rendered config targets
  execSandbox-->>buildConfigStatusSignals: target contents
  buildConfigStatusSignals-->>showSandboxChannelStatus: DiagnosticSignal[]
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~90 minutes

Suggested reviewers

  • ericksoa
  • brandonpelfrey
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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 summarizes the main CLI change: adding a channel config status summary.
Linked Issues check ✅ Passed The Telegram status path now surfaces the effective group policy for open, allowlist, and disabled cases as requested in #5695.
Out of Scope Changes check ✅ Passed The added parser, status-summary, docs, and exec-wrapping changes all support the stated channel-status objective and appear in scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/channels-status-config-summary

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... 401d820 +/-
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... 401d820 +/-
src/lib/shields...nsition-lock.ts 86%
src/lib/actions...dbox/rebuild.ts 80%
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 80%
src/lib/state/sandbox.ts 72%
src/lib/onboard/preflight.ts 69%
src/lib/shields/index.ts 67%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/actions...licy-channel.ts 58%
src/lib/onboard.ts 20%

Updated July 01, 2026 22:18 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-3: Normal sandbox exec marker is no longer emitted on its own line; then add or justify PRA-T1.
Open items: 1 required · 5 warnings · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 3 still apply · 2 new items found

Action checklist

  • PRA-3 Fix: Normal sandbox exec marker is no longer emitted on its own line in src/lib/actions/sandbox/sandbox-exec-output.ts:8
  • PRA-1 Resolve or justify: Source-of-truth review needed: Rendered config parser metadata versus channel manifest render blocks
  • PRA-2 Resolve or justify: Source-of-truth review needed: Sandbox exec stdout marker compatibility
  • PRA-4 Resolve or justify: Rendered-config parsers can still drift from manifest render definitions in src/lib/messaging/channels/rendered-config-parser.ts:14
  • PRA-5 Resolve or justify: Status comparisons read whole rendered config sources that may contain secrets in src/lib/actions/sandbox/channel-status-config.ts:294
  • PRA-6 Resolve or justify: Issue-critical env-onboarding to real status boundary still lacks validation evidence
  • 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: Issue-critical env-onboarding to real status boundary still lacks validation evidence
  • 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 Required correctness src/lib/actions/sandbox/sandbox-exec-output.ts:8 Restore the normal wrapper to emit a real newline, for example the previous `printf '%s\n' '${SANDBOX_EXEC_STARTED_MARKER}'; ${command}` shape, while keeping the base64 newline-safe path only for the Hermes validator case if needed.
PRA-4 Resolve/justify architecture src/lib/messaging/channels/rendered-config-parser.ts:14 Add a manifest-coupled drift guard in this PR. At minimum, assert built-in parser target/path/env metadata against the corresponding manifest render blocks, with explicit documented exceptions for derived projections such as Discord guild maps, Slack object-key projections, and WeChat generated account files. Alternatively derive simple visibility metadata from render specs where possible.
PRA-5 Resolve/justify security src/lib/actions/sandbox/channel-status-config.ts:294 Change the extraction boundary so only allowlisted non-secret keys/paths are returned from the sandbox, or have parser metadata drive a redacted in-sandbox projection. Keep the size cap and timeout, but avoid transporting raw token/secret/password entries to the host at all.
PRA-6 Resolve/justify tests Add or identify narrow runtime/integration evidence in the current PR for the env-onboarding/status boundary. Keep it behavior-focused and use existing CLI/integration style rather than adding a broad new E2E framework layer.

🚨 Required before merge

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

PRA-3 Required — Normal sandbox exec marker is no longer emitted on its own line

  • Location: src/lib/actions/sandbox/sandbox-exec-output.ts:8
  • Category: correctness
  • Problem: The non-Hermes branch changed the marker command to `printf '%s\\\\n' ...`. In a shell printf format, that emits a literal backslash-n after the marker instead of a newline. `extractSandboxExecCommandStdout` only accepts a marker that occupies its own stdout line, so normal `executeSandboxExecCommand` calls can fail to parse child stdout and fall back to Docker or return null.
  • Impact: This can break recovery, status, and other sandbox exec callers that use the normal readable wrapper, including the new rendered-config status reads. In environments without a trusted local Docker fallback, a healthy sandbox command can be treated as unreachable or failed.
  • Required action: Restore the normal wrapper to emit a real newline, for example the previous `printf '%s\n' '${SANDBOX_EXEC_STARTED_MARKER}'; ${command}` shape, while keeping the base64 newline-safe path only for the Hermes validator case if needed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `src/lib/actions/sandbox/sandbox-exec-output.ts:8` and compare it with the exact-line marker check in `extractSandboxExecCommandStdout`; the current printf format leaves the marker and child output on the same physical line.
  • Missing regression test: Add a focused unit test that builds the normal marked command, exercises it through `/bin/sh -c` or an equivalent shell-level fixture, and asserts that `extractSandboxExecCommandStdout` returns child stdout for a normal command without relying on Docker fallback.
  • Done when: The required change is committed and verification passes: Read `src/lib/actions/sandbox/sandbox-exec-output.ts:8` and compare it with the exact-line marker check in `extractSandboxExecCommandStdout`; the current printf format leaves the marker and child output on the same physical line.
  • Evidence: `buildSandboxExecMarkedCommand` now returns `printf '%s\\n' ...` for normal commands, while `extractSandboxExecCommandStdout` searches for a line whose trimmed text equals `__NEMOCLAW_SANDBOX_EXEC_STARTED__`.
Review findings by urgency: 1 required fix, 5 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 parser metadata versus channel manifest render blocks

  • 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: Missing a manifest-coupled drift guard that checks parser target/path/env metadata against manifest render blocks with explicit derived-projection exceptions.
  • 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: `manifests.test.ts` checks parser presence and config/non-secret-like keys, while per-channel parsers such as `telegram/rendered-config-parser.ts` still independently hard-code targets and paths that `telegram/manifest.ts` renders separately.

PRA-2 Resolve/justify — Source-of-truth review needed: Sandbox exec stdout marker compatibility

  • 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 a normal-command marker round-trip test that verifies marker emission and extraction through shell syntax rather than mocked stdout.
  • 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: `sandbox-exec-output.ts:8` changed the normal printf format, while parser tests mock already-correct marker stdout and do not catch the normal wrapper regression.

PRA-4 Resolve/justify — Rendered-config parsers can still drift from manifest render definitions

  • Location: src/lib/messaging/channels/rendered-config-parser.ts:14
  • Category: architecture
  • Problem: The parser registry and per-channel parsers hard-code rendered targets, structured paths, and env keys separately from the channel manifests that actually render OpenClaw and Hermes config. The added manifest tests verify parser presence and that keys are config/non-secret-like inputs, but they still do not prove each parser visibility key is backed by a render entry for the same channel and agent or by a documented derived-file exception.
  • Impact: A future manifest render change can make `channels status` report stale, missing, or false-mismatch values while parser fixture tests continue to pass. For the linked Telegram issue, the status output could again omit or misreport the effective `TELEGRAM_GROUP_POLICY` if `telegram/manifest.ts` moves the rendered field without updating `telegram/rendered-config-parser.ts`.
  • Recommended action: Add a manifest-coupled drift guard in this PR. At minimum, assert built-in parser target/path/env metadata against the corresponding manifest render blocks, with explicit documented exceptions for derived projections such as Discord guild maps, Slack object-key projections, and WeChat generated account files. Alternatively derive simple visibility metadata from render specs where possible.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `src/lib/messaging/channels/manifests.test.ts` around the rendered parser tests and compare `src/lib/messaging/channels/telegram/rendered-config-parser.ts` with `src/lib/messaging/channels/telegram/manifest.ts`; the tests stop at parser presence and config-input/secret-like filtering.
  • Missing regression test: Add a drift test that fails if `telegramRenderedConfigParser` still claims `openclaw.json:channels.telegram.accounts.default.groupPolicy` after the Telegram manifest stops rendering `groupPolicy` there, and extend the same guard or explicit exceptions to Teams, Slack, Discord, WeChat, and WhatsApp parser keys.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `src/lib/messaging/channels/manifests.test.ts` around the rendered parser tests and compare `src/lib/messaging/channels/telegram/rendered-config-parser.ts` with `src/lib/messaging/channels/telegram/manifest.ts`; the tests stop at parser presence and config-input/secret-like filtering.
  • Evidence: `telegram/rendered-config-parser.ts` independently hard-codes `channels.telegram.accounts.default.groupPolicy`; `telegram/manifest.ts` independently renders that field under `telegram-openclaw-channel`; `manifests.test.ts` only checks parser presence and key/input secrecy constraints.

PRA-5 Resolve/justify — Status comparisons read whole rendered config sources that may contain secrets

  • Location: src/lib/actions/sandbox/channel-status-config.ts:294
  • Category: security
  • Problem: The new comparison path reads the entire bounded rendered config source into host-side stdout with `head -c`, then parses full `.env`, JSON, or YAML contents before selecting non-secret keys. Output filtering is good, but the diagnostic still transports and stores secret-bearing files such as Hermes `.env` or OpenClaw channel config in host process memory for a feature documented as comparing non-secret config.
  • Impact: A future log, exception, debug dump, or parser change could expose credentials that should remain inside the sandbox/secret boundary. This weakens NemoClaw's least-exposure credential posture even though the current text output does not print tokens.
  • Recommended action: Change the extraction boundary so only allowlisted non-secret keys/paths are returned from the sandbox, or have parser metadata drive a redacted in-sandbox projection. Keep the size cap and timeout, but avoid transporting raw token/secret/password entries to the host at all.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `readConfigSourceValues` and `parseEnvLines` in `src/lib/actions/sandbox/channel-status-config.ts`; the command is `head -c 65537 <target>` and `parseEnvLines` records every env entry before key-specific filtering.
  • Missing regression test: Add a security regression where rendered `.env`/`openclaw.json` includes `*_TOKEN`, `*_SECRET`, and password-like values, and assert the sandbox exec result or projection returned to host excludes those raw secret keys while status still reports the intended non-secret comparison.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `readConfigSourceValues` and `parseEnvLines` in `src/lib/actions/sandbox/channel-status-config.ts`; the command is `head -c 65537 <target>` and `parseEnvLines` records every env entry before key-specific filtering.
  • Evidence: `readConfigSourceValues` reads whole targets at line 294; `parseEnvLines` at line 335 stores all key/value pairs; parser keys later select non-secret values, but the raw source has already crossed the sandbox-to-host diagnostic boundary.

PRA-6 Resolve/justify — Issue-critical env-onboarding to real status boundary still lacks validation evidence

  • Location: not file-specific
  • Category: tests
  • Problem: The added unit tests cover injected registry inputs and synthetic rendered config for open, allowlist, disabled, mismatch, unreadable, oversized, malformed, and secret-omission cases. The linked issue specifically reproduces `TELEGRAM_GROUP_POLICY` supplied by environment during onboarding and then observed through the real `channels status --channel telegram` command after persistence, rendering, sandbox exec framing, and CLI output formatting.
  • Impact: A unit-level pass can still miss a production-path break where env onboarding does not persist the group policy, rendered OpenClaw config diverges from the registry plan, OpenShell exec framing changes the read result, or the oclif command omits the user-facing line. This is the remaining acceptance-risk for the linked issue.
  • Recommended action: Add or identify narrow runtime/integration evidence in the current PR for the env-onboarding/status boundary. Keep it behavior-focused and use existing CLI/integration style rather than adding a broad new E2E framework layer.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare issue [All Platforms][CLI&UX][GitHub Issue #5695] nemoclaw <sandbox> channels status --channel telegram does not display TELEGRAM_GROUP_POLICY (open/allowlist/disabled) for any value #5695's reproduction blocks with the changed tests: `channel-status-config-core.test.ts` and `channel-status-telegram-policy.test.ts` use `makeDeps` and synthetic registry/rendered JSON, while `channel-status-summary.test.ts` covers no-arg summary with mocked exec.
  • Missing regression test: Add targeted integration coverage for `TELEGRAM_GROUP_POLICY=open`, `allowlist`, and `disabled` flowing through onboarding or the closest existing persisted-plan fixture into `channels status --channel telegram`, asserting the `Telegram group policy (TELEGRAM_GROUP_POLICY): <value>` line appears and `TELEGRAM_BOT_TOKEN` does not.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare issue [All Platforms][CLI&UX][GitHub Issue #5695] nemoclaw <sandbox> channels status --channel telegram does not display TELEGRAM_GROUP_POLICY (open/allowlist/disabled) for any value #5695's reproduction blocks with the changed tests: `channel-status-config-core.test.ts` and `channel-status-telegram-policy.test.ts` use `makeDeps` and synthetic registry/rendered JSON, while `channel-status-summary.test.ts` covers no-arg summary with mocked exec.
  • Evidence: The validation context reports `runtime_validation_recommended` for `channel-status-config.ts`, `channel-status.ts`, and CLI/status surfaces; static test inventory shows extensive unit tests, but all status comparisons use mocked registry and synthetic rendered sources.

💡 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.
Simplification opportunities: 1 possible cut

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

  • PRA-4 shrink (src/lib/messaging/channels/rendered-config-parser.ts:14): Unverified duplicate parser metadata for built-in rendered config targets, paths, and env keys.
    • Replacement: Keep the parser abstraction, but back it with a manifest alignment test or derive straightforward target/key metadata from manifest render specs where possible.
    • Safety boundary: Do not remove secret filtering, bounded sandbox reads, shell/path sanitization, or the allowlisted derived projections that prevent path traversal and credential leakage.
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 — Normal sandbox exec marked command emits `__NEMOCLAW_SANDBOX_EXEC_STARTED__` on its own line and `extractSandboxExecCommandStdout` returns child stdout without Docker fallback.. The PR adds many focused unit tests for parser behavior and status rendering, but the changed behavior crosses CLI, registry persistence, rendered config, and sandbox exec framing boundaries. One blocker also needs a shell-level marker round-trip test because mocked stdout currently misses the regression.
  • PRA-T2 Runtime validation — `TELEGRAM_GROUP_POLICY=open` supplied during onboarding or the closest persisted-plan integration fixture appears in `channels status --channel telegram` as `Telegram group policy (TELEGRAM_GROUP_POLICY): open` and omits `TELEGRAM_BOT_TOKEN`.. The PR adds many focused unit tests for parser behavior and status rendering, but the changed behavior crosses CLI, registry persistence, rendered config, and sandbox exec framing boundaries. One blocker also needs a shell-level marker round-trip test because mocked stdout currently misses the regression.
  • PRA-T3 Runtime validation — `TELEGRAM_GROUP_POLICY=allowlist` supplied during onboarding or the closest persisted-plan integration fixture appears in `channels status --channel telegram` as `Telegram group policy (TELEGRAM_GROUP_POLICY): allowlist`.. The PR adds many focused unit tests for parser behavior and status rendering, but the changed behavior crosses CLI, registry persistence, rendered config, and sandbox exec framing boundaries. One blocker also needs a shell-level marker round-trip test because mocked stdout currently misses the regression.
  • PRA-T4 Runtime validation — `TELEGRAM_GROUP_POLICY=disabled` supplied during onboarding or the closest persisted-plan integration fixture appears in `channels status --channel telegram` as `Telegram group policy (TELEGRAM_GROUP_POLICY): disabled`.. The PR adds many focused unit tests for parser behavior and status rendering, but the changed behavior crosses CLI, registry persistence, rendered config, and sandbox exec framing boundaries. One blocker also needs a shell-level marker round-trip test because mocked stdout currently misses the regression.
  • PRA-T5 Runtime validation — Manifest/parser drift guard fails when a parser visibility key points at a target/path/env key not rendered by the built-in manifest for the same channel and agent, except for documented derived projections.. The PR adds many focused unit tests for parser behavior and status rendering, but the changed behavior crosses CLI, registry persistence, rendered config, and sandbox exec framing boundaries. One blocker also needs a shell-level marker round-trip test because mocked stdout currently misses the regression.
  • PRA-T6 Issue-critical env-onboarding to real status boundary still lacks validation evidence — Add or identify narrow runtime/integration evidence in the current PR for the env-onboarding/status boundary. Keep it behavior-focused and use existing CLI/integration style rather than adding a broad new E2E framework layer.
  • PRA-T7 Acceptance clause — Platform: DGX Spark — add test evidence or identify existing coverage. No platform-specific runtime evidence was reviewed; the changed tests use mocked deps and local fixtures.
  • PRA-T8 Acceptance clause — GPU: any — add test evidence or identify existing coverage. The status logic is not GPU-specific, and no GPU-dependent validation is present.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: Rendered config parser metadata versus channel manifest render blocks

  • 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: Missing a manifest-coupled drift guard that checks parser target/path/env metadata against manifest render blocks with explicit derived-projection exceptions.
  • 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: `manifests.test.ts` checks parser presence and config/non-secret-like keys, while per-channel parsers such as `telegram/rendered-config-parser.ts` still independently hard-code targets and paths that `telegram/manifest.ts` renders separately.

PRA-2 Resolve/justify — Source-of-truth review needed: Sandbox exec stdout marker compatibility

  • 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 a normal-command marker round-trip test that verifies marker emission and extraction through shell syntax rather than mocked stdout.
  • 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: `sandbox-exec-output.ts:8` changed the normal printf format, while parser tests mock already-correct marker stdout and do not catch the normal wrapper regression.

PRA-3 Required — Normal sandbox exec marker is no longer emitted on its own line

  • Location: src/lib/actions/sandbox/sandbox-exec-output.ts:8
  • Category: correctness
  • Problem: The non-Hermes branch changed the marker command to `printf '%s\\\\n' ...`. In a shell printf format, that emits a literal backslash-n after the marker instead of a newline. `extractSandboxExecCommandStdout` only accepts a marker that occupies its own stdout line, so normal `executeSandboxExecCommand` calls can fail to parse child stdout and fall back to Docker or return null.
  • Impact: This can break recovery, status, and other sandbox exec callers that use the normal readable wrapper, including the new rendered-config status reads. In environments without a trusted local Docker fallback, a healthy sandbox command can be treated as unreachable or failed.
  • Required action: Restore the normal wrapper to emit a real newline, for example the previous `printf '%s\n' '${SANDBOX_EXEC_STARTED_MARKER}'; ${command}` shape, while keeping the base64 newline-safe path only for the Hermes validator case if needed.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `src/lib/actions/sandbox/sandbox-exec-output.ts:8` and compare it with the exact-line marker check in `extractSandboxExecCommandStdout`; the current printf format leaves the marker and child output on the same physical line.
  • Missing regression test: Add a focused unit test that builds the normal marked command, exercises it through `/bin/sh -c` or an equivalent shell-level fixture, and asserts that `extractSandboxExecCommandStdout` returns child stdout for a normal command without relying on Docker fallback.
  • Done when: The required change is committed and verification passes: Read `src/lib/actions/sandbox/sandbox-exec-output.ts:8` and compare it with the exact-line marker check in `extractSandboxExecCommandStdout`; the current printf format leaves the marker and child output on the same physical line.
  • Evidence: `buildSandboxExecMarkedCommand` now returns `printf '%s\\n' ...` for normal commands, while `extractSandboxExecCommandStdout` searches for a line whose trimmed text equals `__NEMOCLAW_SANDBOX_EXEC_STARTED__`.

PRA-4 Resolve/justify — Rendered-config parsers can still drift from manifest render definitions

  • Location: src/lib/messaging/channels/rendered-config-parser.ts:14
  • Category: architecture
  • Problem: The parser registry and per-channel parsers hard-code rendered targets, structured paths, and env keys separately from the channel manifests that actually render OpenClaw and Hermes config. The added manifest tests verify parser presence and that keys are config/non-secret-like inputs, but they still do not prove each parser visibility key is backed by a render entry for the same channel and agent or by a documented derived-file exception.
  • Impact: A future manifest render change can make `channels status` report stale, missing, or false-mismatch values while parser fixture tests continue to pass. For the linked Telegram issue, the status output could again omit or misreport the effective `TELEGRAM_GROUP_POLICY` if `telegram/manifest.ts` moves the rendered field without updating `telegram/rendered-config-parser.ts`.
  • Recommended action: Add a manifest-coupled drift guard in this PR. At minimum, assert built-in parser target/path/env metadata against the corresponding manifest render blocks, with explicit documented exceptions for derived projections such as Discord guild maps, Slack object-key projections, and WeChat generated account files. Alternatively derive simple visibility metadata from render specs where possible.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect `src/lib/messaging/channels/manifests.test.ts` around the rendered parser tests and compare `src/lib/messaging/channels/telegram/rendered-config-parser.ts` with `src/lib/messaging/channels/telegram/manifest.ts`; the tests stop at parser presence and config-input/secret-like filtering.
  • Missing regression test: Add a drift test that fails if `telegramRenderedConfigParser` still claims `openclaw.json:channels.telegram.accounts.default.groupPolicy` after the Telegram manifest stops rendering `groupPolicy` there, and extend the same guard or explicit exceptions to Teams, Slack, Discord, WeChat, and WhatsApp parser keys.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect `src/lib/messaging/channels/manifests.test.ts` around the rendered parser tests and compare `src/lib/messaging/channels/telegram/rendered-config-parser.ts` with `src/lib/messaging/channels/telegram/manifest.ts`; the tests stop at parser presence and config-input/secret-like filtering.
  • Evidence: `telegram/rendered-config-parser.ts` independently hard-codes `channels.telegram.accounts.default.groupPolicy`; `telegram/manifest.ts` independently renders that field under `telegram-openclaw-channel`; `manifests.test.ts` only checks parser presence and key/input secrecy constraints.

PRA-5 Resolve/justify — Status comparisons read whole rendered config sources that may contain secrets

  • Location: src/lib/actions/sandbox/channel-status-config.ts:294
  • Category: security
  • Problem: The new comparison path reads the entire bounded rendered config source into host-side stdout with `head -c`, then parses full `.env`, JSON, or YAML contents before selecting non-secret keys. Output filtering is good, but the diagnostic still transports and stores secret-bearing files such as Hermes `.env` or OpenClaw channel config in host process memory for a feature documented as comparing non-secret config.
  • Impact: A future log, exception, debug dump, or parser change could expose credentials that should remain inside the sandbox/secret boundary. This weakens NemoClaw's least-exposure credential posture even though the current text output does not print tokens.
  • Recommended action: Change the extraction boundary so only allowlisted non-secret keys/paths are returned from the sandbox, or have parser metadata drive a redacted in-sandbox projection. Keep the size cap and timeout, but avoid transporting raw token/secret/password entries to the host at all.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `readConfigSourceValues` and `parseEnvLines` in `src/lib/actions/sandbox/channel-status-config.ts`; the command is `head -c 65537 <target>` and `parseEnvLines` records every env entry before key-specific filtering.
  • Missing regression test: Add a security regression where rendered `.env`/`openclaw.json` includes `*_TOKEN`, `*_SECRET`, and password-like values, and assert the sandbox exec result or projection returned to host excludes those raw secret keys while status still reports the intended non-secret comparison.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `readConfigSourceValues` and `parseEnvLines` in `src/lib/actions/sandbox/channel-status-config.ts`; the command is `head -c 65537 <target>` and `parseEnvLines` records every env entry before key-specific filtering.
  • Evidence: `readConfigSourceValues` reads whole targets at line 294; `parseEnvLines` at line 335 stores all key/value pairs; parser keys later select non-secret values, but the raw source has already crossed the sandbox-to-host diagnostic boundary.

PRA-6 Resolve/justify — Issue-critical env-onboarding to real status boundary still lacks validation evidence

  • Location: not file-specific
  • Category: tests
  • Problem: The added unit tests cover injected registry inputs and synthetic rendered config for open, allowlist, disabled, mismatch, unreadable, oversized, malformed, and secret-omission cases. The linked issue specifically reproduces `TELEGRAM_GROUP_POLICY` supplied by environment during onboarding and then observed through the real `channels status --channel telegram` command after persistence, rendering, sandbox exec framing, and CLI output formatting.
  • Impact: A unit-level pass can still miss a production-path break where env onboarding does not persist the group policy, rendered OpenClaw config diverges from the registry plan, OpenShell exec framing changes the read result, or the oclif command omits the user-facing line. This is the remaining acceptance-risk for the linked issue.
  • Recommended action: Add or identify narrow runtime/integration evidence in the current PR for the env-onboarding/status boundary. Keep it behavior-focused and use existing CLI/integration style rather than adding a broad new E2E framework layer.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Compare issue [All Platforms][CLI&UX][GitHub Issue #5695] nemoclaw <sandbox> channels status --channel telegram does not display TELEGRAM_GROUP_POLICY (open/allowlist/disabled) for any value #5695's reproduction blocks with the changed tests: `channel-status-config-core.test.ts` and `channel-status-telegram-policy.test.ts` use `makeDeps` and synthetic registry/rendered JSON, while `channel-status-summary.test.ts` covers no-arg summary with mocked exec.
  • Missing regression test: Add targeted integration coverage for `TELEGRAM_GROUP_POLICY=open`, `allowlist`, and `disabled` flowing through onboarding or the closest existing persisted-plan fixture into `channels status --channel telegram`, asserting the `Telegram group policy (TELEGRAM_GROUP_POLICY): <value>` line appears and `TELEGRAM_BOT_TOKEN` does not.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Compare issue [All Platforms][CLI&UX][GitHub Issue #5695] nemoclaw <sandbox> channels status --channel telegram does not display TELEGRAM_GROUP_POLICY (open/allowlist/disabled) for any value #5695's reproduction blocks with the changed tests: `channel-status-config-core.test.ts` and `channel-status-telegram-policy.test.ts` use `makeDeps` and synthetic registry/rendered JSON, while `channel-status-summary.test.ts` covers no-arg summary with mocked exec.
  • Evidence: The validation context reports `runtime_validation_recommended` for `channel-status-config.ts`, `channel-status.ts`, and CLI/status surfaces; static test inventory shows extensive unit tests, but all status comparisons use mocked registry and synthetic rendered sources.

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.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: messaging-providers, channels-stop-start, sandbox-operations, hermes-sandbox-secret-boundary
Optional E2E: channels-add-remove, diagnostics, docs-validation

Dispatch hint: messaging-providers,channels-stop-start,sandbox-operations,hermes-sandbox-secret-boundary

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • messaging-providers (high): Closest existing real-sandbox coverage for rendered messaging configs and redaction across Telegram, Discord, Slack, WhatsApp, and WeChat. The PR's status command now reads these generated config surfaces and must not expose secret values while reporting non-secret config/policy state.
  • channels-stop-start (high): Exercises real OpenClaw and Hermes messaging channel lifecycle, rebuild, provider reuse, policy list, registry state, and in-sandbox config contracts. This is important because channels status now summarizes configured/disabled channels and compares rendered OpenClaw/Hermes channel config.
  • sandbox-operations (high): Validates real sandbox status/recovery behavior through OpenShell and in-sandbox process boundaries. The process-recovery and sandbox exec output parser changes can affect status, gateway recovery, and internal exec probes.
  • hermes-sandbox-secret-boundary (medium): Required because the sandbox exec command wrapper now has special handling for validate-hermes-env-secret-boundary.py, directly touching the Hermes secret-boundary validation path.

Optional E2E

  • channels-add-remove (high): Useful adjacent confidence for Telegram add/remove, rebuild, gateway credential reuse, policy-list, and cleanup behavior. It does not appear to directly invoke the new channels status summary/detail path, so it is optional rather than the primary required coverage.
  • diagnostics (medium): Adjacent diagnostic CLI coverage for debug/status/credential redaction against a live sandbox. Helpful because this PR is mostly diagnostic output, but it does not specifically cover channels status.
  • docs-validation (low): Optional check for the updated command reference docs and CLI/docs parity after changing channels status help text and public display defaults.

New E2E recommendations

  • channels status command (high): No existing live E2E appears to directly run nemoclaw <sandbox> channels status without --channel, --channel telegram, --channel whatsapp, and --json against a real rebuilt sandbox. The changed code is primarily this command, so current required jobs provide indirect confidence only.
    • Suggested test: Add a focused live E2E target that onboards a sandbox with at least Telegram and WhatsApp test channels, runs channels status summary and per-channel detail/json modes, verifies policy/config comparison lines, verifies WhatsApp deep-probe exit semantics, and asserts tokens/secret placeholders are redacted.
  • Hermes rendered config status comparisons (medium): The parser changes include Hermes .env/YAML paths for multiple channels, but existing live coverage mostly validates generated config directly rather than channels status parser behavior.
    • Suggested test: Extend a Hermes messaging E2E or create a small focused target that runs channels status --channel telegram|discord|slack|wechat on a Hermes sandbox and checks expected non-secret comparison output with no token leakage.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: messaging-providers,channels-stop-start,sandbox-operations,hermes-sandbox-secret-boundary

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: messaging-providers
Optional E2E targets: messaging-compatible-endpoint

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=messaging-providers

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required E2E targets

  • messaging-providers: Changes touch messaging channel status/config comparison and rendered-config parser code for multiple providers. The wired messaging-providers live job is the smallest dispatch in e2e.yaml that exercises real messaging provider onboarding and rendered OpenClaw channel configuration across Telegram, Discord, Slack, WhatsApp, and WeChat surfaces.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=messaging-providers

Optional E2E targets

  • messaging-compatible-endpoint: Adjacent Telegram messaging coverage with a compatible endpoint sandbox can provide extra confidence for rendered OpenClaw Telegram config, but the primary changed surface is broader provider status/config comparison covered by messaging-providers.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=messaging-compatible-endpoint

Relevant changed files

  • src/commands/sandbox/channels/status.ts
  • src/lib/actions/sandbox/channel-status-config-values.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/actions/sandbox/sandbox-exec-output.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 (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-2: Missing runtime integration test for config comparison exec path; then add or justify PRA-T1.
Open items: 4 required · 5 warnings · 4 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 7 still apply · 5 new items found

Action checklist

  • PRA-2 Fix: Missing runtime integration test for config comparison exec path in src/lib/actions/sandbox/channel-status-config.ts:180
  • PRA-3 Fix: Test file exceeds monolith threshold (499 lines) in src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • PRA-4 Fix: Test file exceeds monolith threshold (469 lines) in src/lib/actions/sandbox/channel-status-config-channels.test.ts:1
  • PRA-5 Fix: Source file exceeds monolith threshold (406 lines) in src/lib/actions/sandbox/channel-status-config.ts:1
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/channel-status.test-helpers.ts:16 (vi.mock for policy, registry, agent/defs, process-recovery)
  • PRA-6 Resolve or justify: Source-of-truth review needed: Test helper mocks for policy/registry/agent/process-recovery (CJS interop) in src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • PRA-7 Resolve or justify: resolveConfigTarget accepts arbitrary /sandbox/ paths without allowlist validation in src/lib/actions/sandbox/channel-status-config.ts:224
  • PRA-8 Resolve or justify: Missing path traversal negative test for config comparison exec path in src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • PRA-9 Resolve or justify: Heavy mocking creates mock boundary risk in src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • 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: Missing runtime integration test for config comparison exec path
  • PRA-T7 Add or justify test follow-up: Heavy mocking creates mock boundary risk
  • PRA-T8 Add or justify test follow-up: src/lib/actions/sandbox/channel-status.test-helpers.ts:16 (vi.mock for policy, registry, agent/defs, process-recovery)
  • PRA-10 In-scope improvement: manifests.test.ts parser alignment tests should move to dedicated file in src/lib/messaging/channels/manifests.test.ts:147
  • PRA-11 In-scope improvement: Extract WhatsApp probe helpers to dedicated file in src/lib/actions/sandbox/channel-status.ts:1
  • PRA-12 In-scope improvement: booleanConfigValue silent fallback for non-standard booleans in src/lib/actions/sandbox/channel-status-config-values.ts:54
  • PRA-13 In-scope improvement: safeAccountId pattern not applied to other channels with config-derived paths in src/lib/messaging/channels/wechat/rendered-config-parser.ts:50

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 Required tests src/lib/actions/sandbox/channel-status-config.ts:180 Add an integration test in test/cli/ that creates a real sandbox (or uses test fixture), writes a known rendered config, runs `channels status --channel <name>`, and verifies the config comparison output matches expected values.
PRA-3 Required architecture src/lib/actions/sandbox/channel-status-config-core.test.ts:1 Split into at least two files: channel-status-config-core.test.ts (core comparison logic: match, mismatch, oversized, unreadable, malformed, boolean/list normalization) and channel-status-config-edge-cases.test.ts (default values, Hermes vs OpenClaw differences, shared source warning deduplication).
PRA-4 Required architecture src/lib/actions/sandbox/channel-status-config-channels.test.ts:1 Move Discord tests to src/lib/messaging/channels/discord/rendered-config-parser.test.ts (already exists with 4 tests, add these), Slack tests to slack/rendered-config-parser.test.ts (new), WeChat tests to wechat/rendered-config-parser.test.ts (new). Keep only cross-channel comparison logic here.
PRA-5 Required architecture src/lib/actions/sandbox/channel-status-config.ts:1 Extract at minimum: (1) config-source-reader.ts — `readConfigSourceValues`, `parseRenderedConfigSource`, `resolveConfigTarget`; (2) config-comparator.ts — `compareConfigSource`, `configInputSignal`, `expectedConfigValue`; (3) keep orchestrator `buildConfigStatusSignals` in channel-status-config.ts (~150 lines).
PRA-6 Resolve/justify architecture src/lib/actions/sandbox/channel-status.test-helpers.ts:16 Document the source-of-truth review: the invalid state is vitest's inability to resolve runner.ts CJS require() calls; the source is agent/defs.ts and policy/index.ts transitive deps; source fix requires ESM migration of runner.ts; regression test is existing unit tests passing; removal when runner.ts is ESM. Add at least one integration test that exercises real registry/policy/agent modules without mocks.
PRA-7 Resolve/justify security src/lib/actions/sandbox/channel-status-config.ts:224 Add allowlist validation in resolveConfigTarget: only allow /sandbox/.openclaw/..., /sandbox/.hermes/..., /sandbox/.openclaw-weixin/... paths. Reject other /sandbox/ paths.
PRA-8 Resolve/justify security src/lib/actions/sandbox/channel-status-config-core.test.ts:1 Add a test in channel-status-config-core.test.ts that registers a mock parser returning a traversal target and verifies execSandbox is never called (or resolveConfigTarget returns null).
PRA-9 Resolve/justify tests src/lib/actions/sandbox/channel-status.test-helpers.ts:16 Add at least one integration test that exercises real registry/policy/agent modules without mocks. Consider using the existing integration test pattern from test/cli/connect-recovery-settle.test.ts as a model.
PRA-10 Improvement architecture src/lib/messaging/channels/manifests.test.ts:147 Move the two parser alignment tests to a new src/lib/messaging/channels/rendered-config-parser.test.ts file.
PRA-11 Improvement architecture src/lib/actions/sandbox/channel-status.ts:1 Extract whatsapp-probe.ts with buildProbeScript, parseProbeOutput, buildWhatsappProbeInput, resolveStateDirs. Document as technical debt if scope is tight.
PRA-12 Improvement scope src/lib/actions/sandbox/channel-status-config-values.ts:54 Add test cases for 'yes', 'no', 'on', 'off', 'enabled', 'disabled', 2, -1 to booleanConfigValue test to document current behavior explicitly.
PRA-13 Improvement correctness src/lib/messaging/channels/wechat/rendered-config-parser.ts:50 Audit other channel parsers for config-value-derived file paths and apply similar safe validation. Currently only WeChat has this pattern.

🚨 Required before merge

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

PRA-2 Required — Missing runtime integration test for config comparison exec path

  • Location: src/lib/actions/sandbox/channel-status-config.ts:180
  • Category: tests
  • Problem: All unit tests mock execSandbox; no test exercises the real `openshell sandbox exec head -c 65537` path that reads rendered agent configs from a live sandbox. This is a sandbox/infrastructure boundary that needs behavioral validation.
  • Impact: A regression in the exec command formatting, shell quoting, output parsing, or timeout handling would not be caught by current tests.
  • Required action: Add an integration test in test/cli/ that creates a real sandbox (or uses test fixture), writes a known rendered config, runs `channels status --channel <name>`, and verifies the config comparison output matches expected values.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run `npx vitest run --project integration test/cli/channel-status-config-integration.test.ts` (new test file) and confirm it passes against a real sandbox.
  • Missing regression test: Integration test that executes `showSandboxChannelStatus` with real `execSandbox` (not mocked) against a sandbox with known rendered config, verifying the config comparison signals are correct.
  • Done when: The required change is committed and verification passes: Run `npx vitest run --project integration test/cli/channel-status-config-integration.test.ts` (new test file) and confirm it passes against a real sandbox.
  • Evidence: testDepth.verdict=runtime_validation_recommended; all existing tests in channel-status-config-core.test.ts and channel-status-config-channels.test.ts use mocked execSandbox

PRA-3 Required — Test file exceeds monolith threshold (499 lines)

  • Location: src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • Category: architecture
  • Problem: channel-status-config-core.test.ts grew by 499 lines from zero base. Contains core comparison logic tests mixed with edge cases (default values, Hermes vs OpenClaw differences, shared source warning deduplication).
  • Impact: Large test files are harder to maintain, review, and debug. Related tests should be split by concern.
  • Required action: Split into at least two files: channel-status-config-core.test.ts (core comparison logic: match, mismatch, oversized, unreadable, malformed, boolean/list normalization) and channel-status-config-edge-cases.test.ts (default values, Hermes vs OpenClaw differences, shared source warning deduplication).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check file line count after split: `wc -l src/lib/actions/sandbox/channel-status-config-core.test.ts` should be < 300 lines.
  • Missing regression test: No new test needed — this is a structural reorganization of existing tests.
  • Done when: The required change is committed and verification passes: Check file line count after split: `wc -l src/lib/actions/sandbox/channel-status-config-core.test.ts` should be < 300 lines.
  • Evidence: monolithDeltas shows channel-status-config-core.test.ts delta=499 severity=blocker

PRA-4 Required — Test file exceeds monolith threshold (469 lines)

  • Location: src/lib/actions/sandbox/channel-status-config-channels.test.ts:1
  • Category: architecture
  • Problem: channel-status-config-channels.test.ts grew by 469 lines from zero base. Contains per-channel parser tests for Discord, Slack, WeChat, Hermes WeChat that should be co-located with their parser implementations.
  • Impact: Channel-specific parser tests should be co-located with their parser implementations or split by channel for maintainability.
  • Required action: Move Discord tests to src/lib/messaging/channels/discord/rendered-config-parser.test.ts (already exists with 4 tests, add these), Slack tests to slack/rendered-config-parser.test.ts (new), WeChat tests to wechat/rendered-config-parser.test.ts (new). Keep only cross-channel comparison logic here.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Verify each channel parser test file exists and contains the moved tests; channel-status-config-channels.test.ts should be < 200 lines.
  • Missing regression test: No new test needed — reorganization of existing tests.
  • Done when: The required change is committed and verification passes: Verify each channel parser test file exists and contains the moved tests; channel-status-config-channels.test.ts should be < 200 lines.
  • Evidence: monolithDeltas shows channel-status-config-channels.test.ts delta=469 severity=blocker

PRA-5 Required — Source file exceeds monolith threshold (406 lines)

  • Location: src/lib/actions/sandbox/channel-status-config.ts:1
  • Category: architecture
  • Problem: channel-status-config.ts is a new 406-line file handling config comparison, source reading, parsing, and signal generation. Multiple cohesive responsibilities: config resolution, exec execution, parsing, comparison logic, and signal generation.
  • Impact: Single large file mixes config resolution, exec execution, parsing, comparison logic, and signal generation. Harder to test in isolation and review.
  • Required action: Extract at minimum: (1) config-source-reader.ts — `readConfigSourceValues`, `parseRenderedConfigSource`, `resolveConfigTarget`; (2) config-comparator.ts — `compareConfigSource`, `configInputSignal`, `expectedConfigValue`; (3) keep orchestrator `buildConfigStatusSignals` in channel-status-config.ts (~150 lines).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check file line count after extraction: `wc -l src/lib/actions/sandbox/channel-status-config.ts` should be ~150 lines.
  • Missing regression test: No new test needed — extraction preserves existing test coverage.
  • Done when: The required change is committed and verification passes: Check file line count after extraction: `wc -l src/lib/actions/sandbox/channel-status-config.ts` should be ~150 lines.
  • Evidence: monolithDeltas shows channel-status-config.ts delta=406 severity=blocker
Review findings by urgency: 4 required fixes, 5 items to resolve/justify, 4 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/channel-status.test-helpers.ts:16 (vi.mock for policy, registry, agent/defs, process-recovery)

  • 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: Existing unit tests pass against mocks; but they test against mocks, not real modules. Need integration test with real modules.
  • 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: channel-status.test-helpers.ts lines 6-43; runner.ts uses require() calls that vitest CLI-test project cannot resolve

PRA-6 Resolve/justify — Source-of-truth review needed: Test helper mocks for policy/registry/agent/process-recovery (CJS interop)

  • Location: src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • Category: architecture
  • Problem: Test helpers mock policy, registry, agent/defs, and process-recovery modules as CJS interop workaround. Need to identify: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed.
  • Impact: If the real modules change behavior (e.g., registry.getMessagingPlanFromEntry returns different shape), unit tests pass but integration fails.
  • Recommended action: Document the source-of-truth review: the invalid state is vitest's inability to resolve runner.ts CJS require() calls; the source is agent/defs.ts and policy/index.ts transitive deps; source fix requires ESM migration of runner.ts; regression test is existing unit tests passing; removal when runner.ts is ESM. Add at least one integration test that exercises real registry/policy/agent modules without mocks.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if runner.ts has been migrated to ESM in the codebase; if not, the mock boundary risk persists.
  • Missing regression test: Add at least one integration test (can be the same as the runtime validation test above) that exercises real registry/policy/agent modules without mocks.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if runner.ts has been migrated to ESM in the codebase; if not, the mock boundary risk persists.
  • Evidence: channel-status.test-helpers.ts lines 6-43 mock 4 modules; runner.ts uses CJS require() that vitest cannot resolve from TS test files

PRA-7 Resolve/justify — resolveConfigTarget accepts arbitrary /sandbox/ paths without allowlist validation

  • Location: src/lib/actions/sandbox/channel-status-config.ts:224
  • Category: security
  • Problem: resolveConfigTarget accepts any target starting with `/sandbox/` without restricting to known config directories. While targets currently come from built-in manifests (not user-controlled), defense-in-depth warrants restricting to `/sandbox/.openclaw/`, `/sandbox/.hermes/`, `/sandbox/.openclaw-weixin/` to prevent future manifest tampering from enabling arbitrary file reads.
  • Impact: If a future manifest (or compromised manifest) specifies `/sandbox/etc/passwd`, it would be read via the execSandbox path.
  • Recommended action: Add allowlist validation in resolveConfigTarget: only allow /sandbox/.openclaw/..., /sandbox/.hermes/..., /sandbox/.openclaw-weixin/... paths. Reject other /sandbox/ paths.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Verify that a manifest with target `/sandbox/etc/passwd` would be rejected by resolveConfigTarget returning null.
  • Missing regression test: Test case in channel-status-config-core.test.ts: register a mock parser with target `/sandbox/etc/passwd` and verify resolveConfigTarget returns null and execSandbox is not called.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Verify that a manifest with target `/sandbox/etc/passwd` would be rejected by resolveConfigTarget returning null.
  • Evidence: channel-status-config.ts:224 returns target for any path starting with `/sandbox/`

PRA-8 Resolve/justify — Missing path traversal negative test for config comparison exec path

  • Location: src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • Category: security
  • Problem: Category 8 security testing gap: no explicit path traversal negative test. The WeChat parser has safeAccountId validation but the core config comparison lacks a test that registers a mock parser returning a traversal target (e.g., /etc/passwd) and verifies execSandbox is never called or resolveConfigTarget returns null.
  • Impact: Path traversal via malicious manifest target would not be caught by tests.
  • Recommended action: Add a test in channel-status-config-core.test.ts that registers a mock parser returning a traversal target and verifies execSandbox is never called (or resolveConfigTarget returns null).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the new test and verify it fails before the fix, passes after.
  • Missing regression test: Negative test: mock parser returns target like `../../../etc/passwd` or `/etc/passwd`; verify resolveConfigTarget returns null and execSandbox is not invoked.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the new test and verify it fails before the fix, passes after.
  • Evidence: All existing tests use valid manifest targets; no negative test for traversal

PRA-9 Resolve/justify — Heavy mocking creates mock boundary risk

  • Location: src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • Category: tests
  • Problem: Heavy mocking of policy, registry, agent/defs, and process-recovery creates mock boundary risk. If the real modules change behavior (e.g., registry.getMessagingPlanFromEntry returns different shape), unit tests pass but integration fails.
  • Impact: Unit tests pass against mocks but real integration may fail when modules change.
  • Recommended action: Add at least one integration test that exercises real registry/policy/agent modules without mocks. Consider using the existing integration test pattern from test/cli/connect-recovery-settle.test.ts as a model.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the integration test and verify it exercises real modules.
  • Missing regression test: Integration test in test/cli/ that creates a real sandbox fixture and calls showSandboxChannelStatus with real deps.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the integration test and verify it exercises real modules.
  • Evidence: channel-status.test-helpers.ts mocks 4 modules; all 9 test files use these mocks

💡 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-10 Improvement — manifests.test.ts parser alignment tests should move to dedicated file

  • Location: src/lib/messaging/channels/manifests.test.ts:147
  • Category: architecture
  • Problem: manifests.test.ts grew by 34 lines adding parser alignment tests ('keeps rendered config parsers aligned with built-in manifests' and 'keeps rendered config parser keys limited to manifest config inputs'). These should move to a new src/lib/messaging/channels/rendered-config-parser.test.ts.
  • Impact: Parser alignment tests are mixed with manifest structure tests, making both harder to maintain.
  • Suggested action: Move the two parser alignment tests to a new src/lib/messaging/channels/rendered-config-parser.test.ts file.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check that manifests.test.ts line count decreases and new test file exists with the moved tests.
  • Missing regression test: No new test needed — reorganization of existing tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: manifests.test.ts lines 147-170 contain parser alignment tests; monolithDeltas shows +34 lines

PRA-11 Improvement — Extract WhatsApp probe helpers to dedicated file

  • Location: src/lib/actions/sandbox/channel-status.ts:1
  • Category: architecture
  • Problem: channel-status.ts grew by 60 lines. WhatsApp probe helpers (buildProbeScript, parseProbeOutput, buildWhatsappProbeInput, resolveStateDirs) should be extracted to whatsapp-probe.ts. Keep channel-status.ts as orchestrator.
  • Impact: WhatsApp-specific logic mixed with general channel status orchestration; harder to test in isolation.
  • Suggested action: Extract whatsapp-probe.ts with buildProbeScript, parseProbeOutput, buildWhatsappProbeInput, resolveStateDirs. Document as technical debt if scope is tight.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check file line count after extraction: channel-status.ts should be smaller; whatsapp-probe.ts should exist.
  • Missing regression test: No new test needed — extraction preserves existing test coverage.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: channel-status.ts lines 335-400 contain WhatsApp probe helpers; monolithDeltas shows +60 lines

PRA-12 Improvement — booleanConfigValue silent fallback for non-standard booleans

  • Location: src/lib/actions/sandbox/channel-status-config-values.ts:54
  • Category: scope
  • Problem: booleanConfigValue returns null for non-standard booleans ('yes', 'no', 'on', 'off', 'enabled', 'disabled', 2, -1) which silently falls through to string comparison. This behavior should be explicitly documented with test cases.
  • Impact: Operators may expect 'yes'/'no' to work as booleans; current behavior treats them as strings without warning.
  • Suggested action: Add test cases for 'yes', 'no', 'on', 'off', 'enabled', 'disabled', 2, -1 to booleanConfigValue test to document current behavior explicitly.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run channel-status-config-values.test.ts and verify new test cases pass with current null-return behavior.
  • Missing regression test: Test cases added to channel-status-config-values.test.ts for each non-standard boolean value.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: booleanConfigValue returns null for non-1/0/true/false inputs; configValuesEqual falls back to formatConfigValue string comparison

PRA-13 Improvement — safeAccountId pattern not applied to other channels with config-derived paths

  • Location: src/lib/messaging/channels/wechat/rendered-config-parser.ts:50
  • Category: correctness
  • Problem: WeChat's safeAccountId validates accountId format and blocks path traversal. Other channels (Discord guild IDs, Slack channel IDs, Teams tenant IDs) may derive file paths from config values and should apply similar safe validation.
  • Impact: If other channels ever derive file paths from config values, they would lack the same path traversal protection.
  • Suggested action: Audit other channel parsers for config-value-derived file paths and apply similar safe validation. Currently only WeChat has this pattern.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check discord/rendered-config-parser.ts, slack/rendered-config-parser.ts, teams/rendered-config-parser.ts for config-value-derived paths.
  • Missing regression test: If any other channel derives file paths from config values, add safe validation and corresponding negative tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: wechat/rendered-config-parser.ts:50 safeAccountId blocks `../x`, `a/b`, quotes, newlines; tested in channel-status-config-channels.test.ts
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: config comparison reads real openclaw.json via execSandbox (test/cli/channel-status-config-integration.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: execSandbox command formatting, shell quoting, output parsing, timeout handling, and real module integration. All current tests mock execSandbox and core modules.
  • PRA-T2 Runtime validation — Security: mock parser with traversal target returns null from resolveConfigTarget (channel-status-config-core.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: execSandbox command formatting, shell quoting, output parsing, timeout handling, and real module integration. All current tests mock execSandbox and core modules.
  • PRA-T3 Runtime validation — Integration: real registry/policy/agent modules without mocks (same integration test as above). Runtime/sandbox/infrastructure paths need behavioral runtime validation: execSandbox command formatting, shell quoting, output parsing, timeout handling, and real module integration. All current tests mock execSandbox and core modules.
  • PRA-T4 Runtime validation — BooleanConfigValue: 'yes','no','on','off','enabled','disabled',2,-1 return null (channel-status-config-values.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: execSandbox command formatting, shell quoting, output parsing, timeout handling, and real module integration. All current tests mock execSandbox and core modules.
  • PRA-T5 Runtime validation — resolveConfigTarget rejects /sandbox/etc/passwd (channel-status-config-core.test.ts). Runtime/sandbox/infrastructure paths need behavioral runtime validation: execSandbox command formatting, shell quoting, output parsing, timeout handling, and real module integration. All current tests mock execSandbox and core modules.
  • PRA-T6 Missing runtime integration test for config comparison exec path — Add an integration test in test/cli/ that creates a real sandbox (or uses test fixture), writes a known rendered config, runs `channels status --channel <name>`, and verifies the config comparison output matches expected values.
  • PRA-T7 Heavy mocking creates mock boundary risk — Add at least one integration test that exercises real registry/policy/agent modules without mocks. Consider using the existing integration test pattern from test/cli/connect-recovery-settle.test.ts as a model.
  • PRA-T8 src/lib/actions/sandbox/channel-status.test-helpers.ts:16 (vi.mock for policy, registry, agent/defs, process-recovery) — Existing unit tests pass against mocks; but they test against mocks, not real modules. Need integration test with real modules.. channel-status.test-helpers.ts lines 6-43; runner.ts uses require() calls that vitest CLI-test project cannot resolve
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/channel-status.test-helpers.ts:16 (vi.mock for policy, registry, agent/defs, process-recovery)

  • 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: Existing unit tests pass against mocks; but they test against mocks, not real modules. Need integration test with real modules.
  • 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: channel-status.test-helpers.ts lines 6-43; runner.ts uses require() calls that vitest CLI-test project cannot resolve

PRA-2 Required — Missing runtime integration test for config comparison exec path

  • Location: src/lib/actions/sandbox/channel-status-config.ts:180
  • Category: tests
  • Problem: All unit tests mock execSandbox; no test exercises the real `openshell sandbox exec head -c 65537` path that reads rendered agent configs from a live sandbox. This is a sandbox/infrastructure boundary that needs behavioral validation.
  • Impact: A regression in the exec command formatting, shell quoting, output parsing, or timeout handling would not be caught by current tests.
  • Required action: Add an integration test in test/cli/ that creates a real sandbox (or uses test fixture), writes a known rendered config, runs `channels status --channel <name>`, and verifies the config comparison output matches expected values.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run `npx vitest run --project integration test/cli/channel-status-config-integration.test.ts` (new test file) and confirm it passes against a real sandbox.
  • Missing regression test: Integration test that executes `showSandboxChannelStatus` with real `execSandbox` (not mocked) against a sandbox with known rendered config, verifying the config comparison signals are correct.
  • Done when: The required change is committed and verification passes: Run `npx vitest run --project integration test/cli/channel-status-config-integration.test.ts` (new test file) and confirm it passes against a real sandbox.
  • Evidence: testDepth.verdict=runtime_validation_recommended; all existing tests in channel-status-config-core.test.ts and channel-status-config-channels.test.ts use mocked execSandbox

PRA-3 Required — Test file exceeds monolith threshold (499 lines)

  • Location: src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • Category: architecture
  • Problem: channel-status-config-core.test.ts grew by 499 lines from zero base. Contains core comparison logic tests mixed with edge cases (default values, Hermes vs OpenClaw differences, shared source warning deduplication).
  • Impact: Large test files are harder to maintain, review, and debug. Related tests should be split by concern.
  • Required action: Split into at least two files: channel-status-config-core.test.ts (core comparison logic: match, mismatch, oversized, unreadable, malformed, boolean/list normalization) and channel-status-config-edge-cases.test.ts (default values, Hermes vs OpenClaw differences, shared source warning deduplication).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check file line count after split: `wc -l src/lib/actions/sandbox/channel-status-config-core.test.ts` should be < 300 lines.
  • Missing regression test: No new test needed — this is a structural reorganization of existing tests.
  • Done when: The required change is committed and verification passes: Check file line count after split: `wc -l src/lib/actions/sandbox/channel-status-config-core.test.ts` should be < 300 lines.
  • Evidence: monolithDeltas shows channel-status-config-core.test.ts delta=499 severity=blocker

PRA-4 Required — Test file exceeds monolith threshold (469 lines)

  • Location: src/lib/actions/sandbox/channel-status-config-channels.test.ts:1
  • Category: architecture
  • Problem: channel-status-config-channels.test.ts grew by 469 lines from zero base. Contains per-channel parser tests for Discord, Slack, WeChat, Hermes WeChat that should be co-located with their parser implementations.
  • Impact: Channel-specific parser tests should be co-located with their parser implementations or split by channel for maintainability.
  • Required action: Move Discord tests to src/lib/messaging/channels/discord/rendered-config-parser.test.ts (already exists with 4 tests, add these), Slack tests to slack/rendered-config-parser.test.ts (new), WeChat tests to wechat/rendered-config-parser.test.ts (new). Keep only cross-channel comparison logic here.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Verify each channel parser test file exists and contains the moved tests; channel-status-config-channels.test.ts should be < 200 lines.
  • Missing regression test: No new test needed — reorganization of existing tests.
  • Done when: The required change is committed and verification passes: Verify each channel parser test file exists and contains the moved tests; channel-status-config-channels.test.ts should be < 200 lines.
  • Evidence: monolithDeltas shows channel-status-config-channels.test.ts delta=469 severity=blocker

PRA-5 Required — Source file exceeds monolith threshold (406 lines)

  • Location: src/lib/actions/sandbox/channel-status-config.ts:1
  • Category: architecture
  • Problem: channel-status-config.ts is a new 406-line file handling config comparison, source reading, parsing, and signal generation. Multiple cohesive responsibilities: config resolution, exec execution, parsing, comparison logic, and signal generation.
  • Impact: Single large file mixes config resolution, exec execution, parsing, comparison logic, and signal generation. Harder to test in isolation and review.
  • Required action: Extract at minimum: (1) config-source-reader.ts — `readConfigSourceValues`, `parseRenderedConfigSource`, `resolveConfigTarget`; (2) config-comparator.ts — `compareConfigSource`, `configInputSignal`, `expectedConfigValue`; (3) keep orchestrator `buildConfigStatusSignals` in channel-status-config.ts (~150 lines).
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check file line count after extraction: `wc -l src/lib/actions/sandbox/channel-status-config.ts` should be ~150 lines.
  • Missing regression test: No new test needed — extraction preserves existing test coverage.
  • Done when: The required change is committed and verification passes: Check file line count after extraction: `wc -l src/lib/actions/sandbox/channel-status-config.ts` should be ~150 lines.
  • Evidence: monolithDeltas shows channel-status-config.ts delta=406 severity=blocker

PRA-6 Resolve/justify — Source-of-truth review needed: Test helper mocks for policy/registry/agent/process-recovery (CJS interop)

  • Location: src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • Category: architecture
  • Problem: Test helpers mock policy, registry, agent/defs, and process-recovery modules as CJS interop workaround. Need to identify: what invalid state is handled, where that state is created, why the source cannot be fixed in this PR, what regression test proves the source cannot regress, and when the workaround can be removed.
  • Impact: If the real modules change behavior (e.g., registry.getMessagingPlanFromEntry returns different shape), unit tests pass but integration fails.
  • Recommended action: Document the source-of-truth review: the invalid state is vitest's inability to resolve runner.ts CJS require() calls; the source is agent/defs.ts and policy/index.ts transitive deps; source fix requires ESM migration of runner.ts; regression test is existing unit tests passing; removal when runner.ts is ESM. Add at least one integration test that exercises real registry/policy/agent modules without mocks.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if runner.ts has been migrated to ESM in the codebase; if not, the mock boundary risk persists.
  • Missing regression test: Add at least one integration test (can be the same as the runtime validation test above) that exercises real registry/policy/agent modules without mocks.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if runner.ts has been migrated to ESM in the codebase; if not, the mock boundary risk persists.
  • Evidence: channel-status.test-helpers.ts lines 6-43 mock 4 modules; runner.ts uses CJS require() that vitest cannot resolve from TS test files

PRA-7 Resolve/justify — resolveConfigTarget accepts arbitrary /sandbox/ paths without allowlist validation

  • Location: src/lib/actions/sandbox/channel-status-config.ts:224
  • Category: security
  • Problem: resolveConfigTarget accepts any target starting with `/sandbox/` without restricting to known config directories. While targets currently come from built-in manifests (not user-controlled), defense-in-depth warrants restricting to `/sandbox/.openclaw/`, `/sandbox/.hermes/`, `/sandbox/.openclaw-weixin/` to prevent future manifest tampering from enabling arbitrary file reads.
  • Impact: If a future manifest (or compromised manifest) specifies `/sandbox/etc/passwd`, it would be read via the execSandbox path.
  • Recommended action: Add allowlist validation in resolveConfigTarget: only allow /sandbox/.openclaw/..., /sandbox/.hermes/..., /sandbox/.openclaw-weixin/... paths. Reject other /sandbox/ paths.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Verify that a manifest with target `/sandbox/etc/passwd` would be rejected by resolveConfigTarget returning null.
  • Missing regression test: Test case in channel-status-config-core.test.ts: register a mock parser with target `/sandbox/etc/passwd` and verify resolveConfigTarget returns null and execSandbox is not called.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Verify that a manifest with target `/sandbox/etc/passwd` would be rejected by resolveConfigTarget returning null.
  • Evidence: channel-status-config.ts:224 returns target for any path starting with `/sandbox/`

PRA-8 Resolve/justify — Missing path traversal negative test for config comparison exec path

  • Location: src/lib/actions/sandbox/channel-status-config-core.test.ts:1
  • Category: security
  • Problem: Category 8 security testing gap: no explicit path traversal negative test. The WeChat parser has safeAccountId validation but the core config comparison lacks a test that registers a mock parser returning a traversal target (e.g., /etc/passwd) and verifies execSandbox is never called or resolveConfigTarget returns null.
  • Impact: Path traversal via malicious manifest target would not be caught by tests.
  • Recommended action: Add a test in channel-status-config-core.test.ts that registers a mock parser returning a traversal target and verifies execSandbox is never called (or resolveConfigTarget returns null).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the new test and verify it fails before the fix, passes after.
  • Missing regression test: Negative test: mock parser returns target like `../../../etc/passwd` or `/etc/passwd`; verify resolveConfigTarget returns null and execSandbox is not invoked.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the new test and verify it fails before the fix, passes after.
  • Evidence: All existing tests use valid manifest targets; no negative test for traversal

PRA-9 Resolve/justify — Heavy mocking creates mock boundary risk

  • Location: src/lib/actions/sandbox/channel-status.test-helpers.ts:16
  • Category: tests
  • Problem: Heavy mocking of policy, registry, agent/defs, and process-recovery creates mock boundary risk. If the real modules change behavior (e.g., registry.getMessagingPlanFromEntry returns different shape), unit tests pass but integration fails.
  • Impact: Unit tests pass against mocks but real integration may fail when modules change.
  • Recommended action: Add at least one integration test that exercises real registry/policy/agent modules without mocks. Consider using the existing integration test pattern from test/cli/connect-recovery-settle.test.ts as a model.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Run the integration test and verify it exercises real modules.
  • Missing regression test: Integration test in test/cli/ that creates a real sandbox fixture and calls showSandboxChannelStatus with real deps.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Run the integration test and verify it exercises real modules.
  • Evidence: channel-status.test-helpers.ts mocks 4 modules; all 9 test files use these mocks

PRA-10 Improvement — manifests.test.ts parser alignment tests should move to dedicated file

  • Location: src/lib/messaging/channels/manifests.test.ts:147
  • Category: architecture
  • Problem: manifests.test.ts grew by 34 lines adding parser alignment tests ('keeps rendered config parsers aligned with built-in manifests' and 'keeps rendered config parser keys limited to manifest config inputs'). These should move to a new src/lib/messaging/channels/rendered-config-parser.test.ts.
  • Impact: Parser alignment tests are mixed with manifest structure tests, making both harder to maintain.
  • Suggested action: Move the two parser alignment tests to a new src/lib/messaging/channels/rendered-config-parser.test.ts file.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check that manifests.test.ts line count decreases and new test file exists with the moved tests.
  • Missing regression test: No new test needed — reorganization of existing tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: manifests.test.ts lines 147-170 contain parser alignment tests; monolithDeltas shows +34 lines

PRA-11 Improvement — Extract WhatsApp probe helpers to dedicated file

  • Location: src/lib/actions/sandbox/channel-status.ts:1
  • Category: architecture
  • Problem: channel-status.ts grew by 60 lines. WhatsApp probe helpers (buildProbeScript, parseProbeOutput, buildWhatsappProbeInput, resolveStateDirs) should be extracted to whatsapp-probe.ts. Keep channel-status.ts as orchestrator.
  • Impact: WhatsApp-specific logic mixed with general channel status orchestration; harder to test in isolation.
  • Suggested action: Extract whatsapp-probe.ts with buildProbeScript, parseProbeOutput, buildWhatsappProbeInput, resolveStateDirs. Document as technical debt if scope is tight.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check file line count after extraction: channel-status.ts should be smaller; whatsapp-probe.ts should exist.
  • Missing regression test: No new test needed — extraction preserves existing test coverage.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: channel-status.ts lines 335-400 contain WhatsApp probe helpers; monolithDeltas shows +60 lines

PRA-12 Improvement — booleanConfigValue silent fallback for non-standard booleans

  • Location: src/lib/actions/sandbox/channel-status-config-values.ts:54
  • Category: scope
  • Problem: booleanConfigValue returns null for non-standard booleans ('yes', 'no', 'on', 'off', 'enabled', 'disabled', 2, -1) which silently falls through to string comparison. This behavior should be explicitly documented with test cases.
  • Impact: Operators may expect 'yes'/'no' to work as booleans; current behavior treats them as strings without warning.
  • Suggested action: Add test cases for 'yes', 'no', 'on', 'off', 'enabled', 'disabled', 2, -1 to booleanConfigValue test to document current behavior explicitly.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run channel-status-config-values.test.ts and verify new test cases pass with current null-return behavior.
  • Missing regression test: Test cases added to channel-status-config-values.test.ts for each non-standard boolean value.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: booleanConfigValue returns null for non-1/0/true/false inputs; configValuesEqual falls back to formatConfigValue string comparison

PRA-13 Improvement — safeAccountId pattern not applied to other channels with config-derived paths

  • Location: src/lib/messaging/channels/wechat/rendered-config-parser.ts:50
  • Category: correctness
  • Problem: WeChat's safeAccountId validates accountId format and blocks path traversal. Other channels (Discord guild IDs, Slack channel IDs, Teams tenant IDs) may derive file paths from config values and should apply similar safe validation.
  • Impact: If other channels ever derive file paths from config values, they would lack the same path traversal protection.
  • Suggested action: Audit other channel parsers for config-value-derived file paths and apply similar safe validation. Currently only WeChat has this pattern.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check discord/rendered-config-parser.ts, slack/rendered-config-parser.ts, teams/rendered-config-parser.ts for config-value-derived paths.
  • Missing regression test: If any other channel derives file paths from config values, add safe validation and corresponding negative tests.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: wechat/rendered-config-parser.ts:50 safeAccountId blocks `../x`, `a/b`, quotes, newlines; tested in channel-status-config-channels.test.ts

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: 1

🧹 Nitpick comments (2)
docs/reference/commands.mdx (1)

1274-1278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Split multi-sentence lines into one sentence per line.

Lines 1274 and 1278 each contain multiple sentences on a single line. Break each sentence onto its own line.

As per path instructions: "Keep one sentence per line in Markdown and MDX source files."

🤖 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 `@docs/reference/commands.mdx` around lines 1274 - 1278, The command reference
text in the status checks section contains multiple sentences on the same MDX
line, which violates the one-sentence-per-line convention. Reformat the prose in
the affected block so each sentence in the description of `--channel`, WhatsApp
probing, and registered channel config comparisons appears on its own line,
while keeping the existing wording and references to the status command behavior
intact.

Source: Path instructions

docs/reference/commands-nemohermes.mdx (1)

999-1003: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Split multi-sentence lines into one sentence per line.

Lines 999 and 1003 each contain multiple sentences on a single line. Break each sentence onto its own line.

As per path instructions: "Keep one sentence per line in Markdown and MDX source files."

🤖 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 `@docs/reference/commands-nemohermes.mdx` around lines 999 - 1003, The prose in
the command reference block uses multiple sentences on the same line, which
violates the one-sentence-per-line MDX rule. Update the descriptive text in the
command docs section so each sentence in the `commands-nemohermes` content is
split onto its own line, keeping the wording the same while preserving the
existing descriptions for `--channel`, WhatsApp probing, and registered-channel
config comparisons.

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.

Inline comments:
In `@src/lib/messaging/channels/discord/rendered-config-parser.ts`:
- Around line 71-83: The mixed-value path in discordRequireMention is returning
the stringified projection keys instead of the original requireMention values,
so conflicting guild settings lose their true types. Update the
discordRequireMention function to dedupe by formatProjectionValue for
comparison, but build the returned array from the original
MessagingSerializableValue entries so booleans, numbers, and other values
preserve their types when requireMention differs across guilds.

---

Nitpick comments:
In `@docs/reference/commands-nemohermes.mdx`:
- Around line 999-1003: The prose in the command reference block uses multiple
sentences on the same line, which violates the one-sentence-per-line MDX rule.
Update the descriptive text in the command docs section so each sentence in the
`commands-nemohermes` content is split onto its own line, keeping the wording
the same while preserving the existing descriptions for `--channel`, WhatsApp
probing, and registered-channel config comparisons.

In `@docs/reference/commands.mdx`:
- Around line 1274-1278: The command reference text in the status checks section
contains multiple sentences on the same MDX line, which violates the
one-sentence-per-line convention. Reformat the prose in the affected block so
each sentence in the description of `--channel`, WhatsApp probing, and
registered channel config comparisons appears on its own line, while keeping the
existing wording and references to the status command behavior intact.
🪄 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: 87235fb3-87e4-4fbf-a1a7-4e53b9a8d1c9

📥 Commits

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

📒 Files selected for processing (22)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/commands/sandbox/channels/status.ts
  • 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/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.test.ts
  • src/lib/messaging/channels/discord/rendered-config-parser.ts
  • src/lib/messaging/channels/index.ts
  • src/lib/messaging/channels/manifests.test.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 Outdated
sandl99 added 2 commits June 30, 2026 15:14
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 added area: ci CI workflows, checks, release automation, or GitHub Actions area: messaging Messaging channels, bridges, manifests, or channel lifecycle labels 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/lib/actions/sandbox/channel-status-summary.test.ts (1)

7-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add an unknown-channel regression test.

This suite covers the new no-arg summary path, but it never exercises the explicit unknown-channel branch added in this PR. A public-boundary test for an invalid channel value would lock down that the removed default fallback cannot silently route back into summary or WhatsApp probing. As per path instructions, "Tests must prove that public entrypoints reach the new path and that the old path is deleted or cannot execute."

🤖 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-summary.test.ts` around lines 7 - 90,
Add a regression test in showSandboxChannelStatus that passes an explicit
invalid channel value and asserts the unknown-channel branch is taken. Update
the channel-status-summary.test suite using the public entrypoint and the
existing makeDeps/showSandboxChannelStatus helpers to verify it does not fall
back to the no-arg summary path or trigger WhatsApp probing. Ensure the new test
checks the expected unknown-channel output/verdict so the removed default path
cannot be reached.

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.

Inline comments:
In `@src/lib/actions/sandbox/channel-status-config-values.ts`:
- Around line 49-55: The boolean normalization helper currently only handles
booleans and strings, so numeric 0/1 values from structured config can still
compare as mismatches in configValuesEqual(). Update booleanConfigValue to also
accept numeric 0 and 1 and normalize them to false/true, alongside the existing
boolean-like string handling, so comparisons in channel-status-config-values.ts
treat these inputs consistently.

---

Nitpick comments:
In `@src/lib/actions/sandbox/channel-status-summary.test.ts`:
- Around line 7-90: Add a regression test in showSandboxChannelStatus that
passes an explicit invalid channel value and asserts the unknown-channel branch
is taken. Update the channel-status-summary.test suite using the public
entrypoint and the existing makeDeps/showSandboxChannelStatus helpers to verify
it does not fall back to the no-arg summary path or trigger WhatsApp probing.
Ensure the new test checks the expected unknown-channel output/verdict so the
removed default path cannot be reached.
🪄 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: 62fbd309-214f-41b1-b695-19716004a99b

📥 Commits

Reviewing files that changed from the base of the PR and between 7ab9cf1 and 98d47ad.

📒 Files selected for processing (18)
  • docs/reference/commands-nemohermes.mdx
  • docs/reference/commands.mdx
  • src/commands/sandbox/channels/status.ts
  • src/lib/actions/sandbox/channel-status-config-channels.test.ts
  • src/lib/actions/sandbox/channel-status-config-core.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.ts
  • src/lib/actions/sandbox/channel-status-config.ts
  • src/lib/actions/sandbox/channel-status-summary.test.ts
  • src/lib/actions/sandbox/channel-status-telegram-policy.test.ts
  • src/lib/actions/sandbox/channel-status.test-helpers.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/messaging/channels/discord/rendered-config-parser.test.ts
  • src/lib/messaging/channels/manifests.test.ts
  • src/lib/messaging/channels/wechat/rendered-config-parser.ts
✅ Files skipped from review due to trivial changes (4)
  • src/commands/sandbox/channels/status.ts
  • docs/reference/commands-nemohermes.mdx
  • src/lib/actions/sandbox/channel-status-config-values.test.ts
  • docs/reference/commands.mdx
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/lib/actions/sandbox/process-recovery.test.ts
  • src/lib/messaging/channels/wechat/rendered-config-parser.ts
  • src/lib/messaging/channels/manifests.test.ts
  • src/lib/actions/sandbox/process-recovery.ts
  • src/lib/actions/sandbox/channel-status.ts

Comment thread src/lib/actions/sandbox/channel-status-config-values.ts
Signed-off-by: San Dang <sdang@nvidia.com>

@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)
test/process-recovery.test.ts (1)

293-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid pinning this test to the exact shell snippet.

These assertions lock the test to payload source text (printf '%s\\n' ..., newline formatting) instead of the observable contract that the Hermes validator command is forwarded unwrapped and still executes through OpenShell. A small quoting or wrapper refactor would break the test without changing behavior.

As per path instructions, **/*.test.{ts,js,mts,mjs,cts,cjs} tests should "Prefer observable outcomes through the public boundary over source-text" and should flag "implementation lock-in."

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

In `@test/process-recovery.test.ts` around lines 293 - 296, The test is too
tightly coupled to the exact shell payload text, so update the assertions in
process-recovery.test to verify the observable contract instead of source
formatting. In the shellPayload checks, keep validating that the Hermes
validator command is forwarded and that OpenShell is used, but remove assertions
that pin the payload to specific newline/printf syntax or exact wrapper text;
use higher-level expectations around the command path and execution behavior so
refactors in shell quoting do not break the test.

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 `@test/process-recovery.test.ts`:
- Around line 293-296: The test is too tightly coupled to the exact shell
payload text, so update the assertions in process-recovery.test to verify the
observable contract instead of source formatting. In the shellPayload checks,
keep validating that the Hermes validator command is forwarded and that
OpenShell is used, but remove assertions that pin the payload to specific
newline/printf syntax or exact wrapper text; use higher-level expectations
around the command path and execution behavior so refactors in shell quoting do
not break the test.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 73198d20-4b41-4d5f-bf0f-f40d764dc84d

📥 Commits

Reviewing files that changed from the base of the PR and between 98d47ad and d507a25.

📒 Files selected for processing (1)
  • test/process-recovery.test.ts

Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99 sandl99 added the enhancement New capability or improvement request label 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.

Actionable comments posted: 2

🤖 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/actions/sandbox/channel-status-config.ts`:
- Around line 112-119: The severity logic in the channel status summary is
treating missing rendered values as info because `compareConfigSource()` leaves
absent parsed values unchecked. Update the `channel-status-config` status
calculation so the summary in this branch also warns when `expected.hasValue` is
true but not all configured sources are represented in `comparisons`, using the
existing `checkedComparisons`/`allSourcesChecked` logic in the returned object
to surface that drift instead of downgrading it to info.

In `@test/process-recovery.test.ts`:
- Around line 293-295: The test is overfitting to the current argv framing in
shell execution instead of verifying the newline-safe contract. Update the
assertions in the recovery test around the command builder/execution path so
they focus on observable behavior at the public boundary: the payload must
remain newline-safe and must not contain raw multi-line shell content or the
unsafe decode-and-execute pattern. Keep the check anchored to the relevant test
helpers/flows that produce shellPayload rather than asserting the exact fixed
sandbox exec prefix.
🪄 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: ea9acbfd-ad85-4bfb-8a65-a1d93081a1d4

📥 Commits

Reviewing files that changed from the base of the PR and between d507a25 and 806f3da.

📒 Files selected for processing (9)
  • src/lib/actions/sandbox/channel-status-config-channels.test.ts
  • src/lib/actions/sandbox/channel-status-config-core.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.ts
  • src/lib/actions/sandbox/channel-status-config.ts
  • src/lib/actions/sandbox/channel-status-summary.test.ts
  • src/lib/actions/sandbox/channel-status-telegram-policy.test.ts
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
  • test/process-recovery.test.ts
💤 Files with no reviewable changes (1)
  • src/lib/messaging/channels/telegram/rendered-config-parser.ts
🚧 Files skipped from review as they are similar to previous changes (5)
  • src/lib/actions/sandbox/channel-status-telegram-policy.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.test.ts
  • src/lib/actions/sandbox/channel-status-config-values.ts
  • src/lib/actions/sandbox/channel-status-config-channels.test.ts
  • src/lib/actions/sandbox/channel-status-config-core.test.ts

Comment thread src/lib/actions/sandbox/channel-status-config.ts
Comment thread test/process-recovery.test.ts Outdated
sandl99 added 2 commits June 30, 2026 16:58
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
@sandl99

sandl99 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Maintainer rationale for PR Review Advisor (Nemotron Ultra) required items on current head 2b5225d253b2c1aaef0e0be4c603cf7328dcd275:

  • PRA-3: Justified. The finding is stale/overbroad for the cited current behavior. On the current head, optional configured inputs with parser visibility are rendered as status lines instead of disappearing. For example, channels status shows Slack Channel IDs (SLACK_ALLOWED_CHANNELS): not set and Discord User ID (DISCORD_USER_ID): not set. For inputs that genuinely have no rendered source for a given agent, channels status intentionally compares only values that the agent actually renders; adding generic no rendered config source lines would be metadata coverage/noise rather than a correctness fix for this PR.

  • PRA-4: Justified. Full manifest-input-to-rendered-source coverage is not a valid invariant across all agents. The messaging manifest is cross-agent, while rendered config is agent-specific. Some inputs have no equivalent rendered value for an agent, such as Telegram groupPolicy for Hermes, Teams requireMention for Hermes, WhatsApp allowedIds for OpenClaw, and dynamic WeChat account-file fields that only exist after an account id is known. Treating those as required rendered sources would force invented comparisons. The current behavior keeps channel status focused on actual source-of-truth values that are rendered and comparable.

  • PRA-5: Justified/deferred. The suggested split is a non-functional architecture refactor. channel-status-config.ts is scoped to one feature boundary: resolve rendered config sources, read/parse them, and produce diagnostic signals. Splitting it now would add review churn without changing user-visible behavior or reducing the current PR risk. Existing targeted tests cover the behavior. This can be revisited as follow-up cleanup if maintainers want a stricter file-size policy.

  • PRA-6: Justified/deferred. The large test files are behavior-focused and grouped around the new channel-status comparison surface. Splitting them would be organizational only and would not improve correctness for this PR. The current tests are targeted at the changed behavior; additional reshaping can be handled separately if a repository-wide test-file size policy is adopted.

Conclusion: no further code changes are required for these four required advisor items on the latest head. PRA-3 and PRA-4 are addressed by current behavior plus agent-specific source-of-truth rationale; PRA-5 and PRA-6 are refactor-only and should not block this feature PR.

@sandl99
sandl99 requested a review from cv June 30, 2026 10:25
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ✅ All selected jobs passed

Run: 28437574000
Workflow ref: feat/channels-status-config-summary
Requested targets: channels-stop-start,channels-add-remove
Requested jobs: (default — all default-enabled free-standing jobs; explicit-only jobs such as jetson-nvmap-gpu and sandbox-rlimits-connect are skipped unless selected)
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
channels-add-remove ✅ success
channels-stop-start ✅ success

@prekshivyas prekshivyas left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Status summary logic is safe — secrets filtered at the manifest input level, 64 KB read cap, path-traversal coverage. CodeRabbit finding is a diagnostic accuracy gap, not a safety issue. LGTM.

prekshivyas and others added 2 commits June 30, 2026 11:02
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
…and test budget

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
prekshivyas and others added 5 commits June 30, 2026 11:33
…atisfy file budget

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
…gn, update tests

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Prek Shiv <prekshiv@nvidia.com>
…s-config-summary

# Conflicts:
#	src/lib/actions/sandbox/process-recovery.test.ts
#	src/lib/agent/hermes-recovery-boundary.ts
#	test/cli/connect-recovery.test.ts
#	test/process-recovery.test.ts
@jyaunches jyaunches added v0.0.72 and removed v0.0.71 labels Jun 30, 2026
@apurvvkumaria
apurvvkumaria merged commit 76654f7 into main Jul 1, 2026
54 of 57 checks passed
@apurvvkumaria
apurvvkumaria deleted the feat/channels-status-config-summary branch July 1, 2026 22:40
ericksoa pushed a commit that referenced this pull request Jul 2, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- #6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- #6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- #5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- #6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- #6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- #5980 and #5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- #6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- #6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- #5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- #6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- #6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- #6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- #6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- #5976 and #5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- #5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- #5978 and #6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

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

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
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>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
## Summary
- Add the `v0.0.72` release-note section with links to the deeper docs
pages for installer recovery, command diagnostics, inference, policy,
and sandbox repair changes.
- Document the custom preset `allowed_ips` guard for user-authored
policy files.

## Related Issue
None.

## Source summary
- NVIDIA#6132 -> `docs/about/release-notes.mdx`: Summarizes installer and
upgrade recovery before generic onboarding, with links to quickstart and
lifecycle docs.
- NVIDIA#6087 -> `docs/network-policy/customize-network-policy.mdx`: Documents
that user-authored custom presets reject `allowed_ips` for ordinary
endpoints; also summarized in release notes.
- NVIDIA#5975 -> `docs/about/release-notes.mdx`: Summarizes safer curl-based
inference probes that keep API keys out of process arguments.
- NVIDIA#6044 -> `docs/about/release-notes.mdx`: Summarizes compact `channels
status` configuration reporting.
- NVIDIA#6096 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw EC2
metadata discovery disablement and links to security guidance.
- NVIDIA#5980 and NVIDIA#5991 -> `docs/about/release-notes.mdx`: Summarizes `exec`
multiline argument rejection and recovery guidance.
- NVIDIA#6023 -> `docs/about/release-notes.mdx`: Summarizes
registered-provider diagnostics for `inference set` failures.
- NVIDIA#6074 -> `docs/about/release-notes.mdx`: Summarizes the refreshed
NVIDIA Endpoints featured-model selection behavior.
- NVIDIA#5969 -> `docs/about/release-notes.mdx`: Summarizes `credentials add`
provider credential registration.
- NVIDIA#6060 -> `docs/about/release-notes.mdx`: Summarizes mutable OpenClaw
config permission restoration after `exec`.
- NVIDIA#6134 -> `docs/about/release-notes.mdx`: Summarizes restored Tavily
access for managed Python workflows.
- NVIDIA#6089 -> `docs/about/release-notes.mdx`: Summarizes Hermes runtime
version-scheme comparison during upgrade checks.
- NVIDIA#6131 -> `docs/about/release-notes.mdx`: Summarizes OpenClaw gateway
watchdog recovery behavior.
- NVIDIA#5976 and NVIDIA#5990 -> `docs/about/release-notes.mdx`: Summarizes prompt
stdin EOF cancellation behavior during onboarding.
- NVIDIA#5540 -> `docs/about/release-notes.mdx`: Summarizes clarified
host-level and per-sandbox status command scope.
- NVIDIA#5978 and NVIDIA#6018 -> `docs/about/release-notes.mdx`: Summarizes
policy-denial log breadcrumbs in connect shells.

## Testing
- `npm run docs:sync-agent-variants`
- `npm run docs`
- Commit hooks passed during `git commit`, including commitlint and
gitleaks.
- Pre-push hook passed during `git push`, including TypeScript CLI and
package/tag version sync.

## Checklist
- [x] Documentation updated.
- [x] `npm run docs` completed with 0 errors and 1 existing Fern
warning.
- [x] No source code or generated build artifacts committed.

Signed-off-by: Miyoung Choi <miyoungc@nvidia.com>

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

## Summary by CodeRabbit

* **Documentation**
* Added release notes for v0.0.72 covering improved installer recovery,
clearer CLI diagnostics, safer inference setup and provider switching,
better credential handling, stronger policy boundaries, and more robust
runtime repair behavior.
* Updated network policy guidance to clarify when `allowed_ips` can be
used, including a specific exception for the sandbox-to-host bridge
endpoint.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ci CI workflows, checks, release automation, or GitHub Actions area: messaging Messaging channels, bridges, manifests, or channel lifecycle enhancement New capability or improvement request

Projects

None yet

4 participants