Skip to content

fix(messaging): surface telegram config inputs in channels status - #5705

Closed
laitingsheng wants to merge 37 commits into
mainfrom
fix/channels-status-telegram-config-visibility
Closed

fix(messaging): surface telegram config inputs in channels status#5705
laitingsheng wants to merge 37 commits into
mainfrom
fix/channels-status-telegram-config-visibility

Conversation

@laitingsheng

@laitingsheng laitingsheng commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

nemoclaw <sandbox> channels status --channel telegram and nemoclaw <sandbox> doctor now read each channel's persisted plan inputs and render one signal/check per visible config input declared by the manifest. Operators can confirm the active Telegram group policy and mention-mode behavior directly from those commands instead of digging into logs or config files.

The diagnostics renderer is opt-in (safeToPrintInDiagnostics on the manifest input), agent-scoped (agentApplicability skips OpenClaw-only inputs on Hermes), and validates persisted values against the manifest validValues allowlist (out-of-allowlist or present-but-empty values render bounded invalid persisted value (...) text rather than echoing the raw value).

Related Issue

Fixes #5695
Fixes #5691

Changes

  • Extend MessagingChannelDiagnosticSpec with visibleConfigInputs. Inputs opt in via the new safeToPrintInDiagnostics: true manifest property; prompt-labeled config inputs without this flag (for example Telegram allowedIds) no longer reach diagnostics. Secrets are still excluded by kind.
  • Add valueDisplay per-input map for human-readable behaviour text. Telegram requireMention renders mention-only (TELEGRAM_REQUIRE_MENTION=1) / all group messages (TELEGRAM_REQUIRE_MENTION=0) rather than raw 1/0.
  • Add agentApplicability per-input filter. Telegram groupPolicy is marked ["openclaw"], so a Hermes sandbox no longer reports the OpenClaw-only setting.
  • Extract resolveVisibleConfigDisplay and collectVisibleConfigRecords shared helpers in src/lib/messaging/diagnostics.ts. Both channels status (src/lib/actions/sandbox/channel-status.ts) and doctor (src/lib/actions/sandbox/doctor.ts) consume normalised { input, display } records.
  • channels status now only renders visible-config signals when the channel is registered and not paused for the sandbox; doctor only inspects active channels. Defaults no longer surface for unregistered or paused channels.
  • Doctor falls back to the OpenClaw agent for legacy SandboxEntry rows whose agent field predates the runtime split, matching the channels status convention.
  • Persisted values that fall outside the manifest validValues allowlist, are present-but-empty, or are not scalar render [warn] invalid persisted value (...) rather than echoing the raw value. This keeps the diagnostic boundary from leaking corrupted plan state.

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)

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)
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Tinson Lai tinsonl@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added channel diagnostics and doctor checks that display selected, human-readable configuration values (persisted/default/invalid) for supported inputs.
    • Telegram status/doctor now presents clearer labels for group policy and mention mode, including default indicators when applicable.
  • Bug Fixes
    • Improved rendering for missing, empty, or invalid saved values (with normalized “expected” messaging).
    • Automatically hides unsupported or irrelevant Telegram configuration details for the wrong agent, and omits config when a channel is unregistered/paused.
    • Further reduces the chance of exposing sensitive-looking configuration content.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jun 24, 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 visible-config diagnostics infrastructure to surface Telegram channel configuration in channels status and doctor output. It introduces new diagnostics types and resolution logic, annotates Telegram manifest inputs with display metadata, wires plan-based signals into both flows, and adds Telegram-focused tests for the rendered values.

Changes

Telegram Config Visibility in Channel Status and Doctor

Layer / File(s) Summary
Visible config input contract and resolution logic
src/lib/messaging/manifest/types.ts, src/lib/messaging/channels/telegram/manifest.ts, src/lib/messaging/diagnostics.ts, src/lib/messaging/diagnostics.test.ts
ChannelConfigInputSpec gains safeToPrintInDiagnostics and valueDisplay; Telegram requireMention and groupPolicy are annotated with these fields. diagnostics.ts introduces VisibleChannelConfigInput, VisibleConfigDisplay, VisibleConfigRecord, and the collectVisibleConfigInputs / resolveVisibleConfigDisplay / collectVisibleConfigRecords helpers. Integration tests compile a Telegram plan and assert collected visible config records.
Channel status config visibility signals
src/lib/actions/sandbox/channel-status.ts, src/lib/actions/sandbox/channel-status.test.ts
StatusDeps gains getMessagingPlan, wired to registry.getMessagingPlanFromEntry. The basic channel report appends DiagnosticSignals from visible config records when the channel is enabled. Tests cover groupPolicy and requireMention display, omission when unregistered or paused, agent differences, invalid persisted value redaction, and env-based plan compilation.
Doctor messaging config checks
src/lib/actions/sandbox/doctor-messaging.ts, src/lib/actions/sandbox/doctor-flow.test.ts
messagingChannelConfigDoctorChecks iterates enabled channels, collects visible config records, and maps display.source to DoctorStatus with conditional hints. collectMessagingDoctorChecks appends these checks. Doctor tests cover mention-mode, agent variants, legacy no-agent, invalid groupPolicy, empty persisted value, and an env-compiled plan case.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#5777: Both PRs modify src/lib/actions/sandbox/channel-status.ts to determine messaging-channel behavior from the channel manifest registry.
  • NVIDIA/NemoClaw#5910: Both PRs touch the sandbox doctor messaging pipeline and collectMessagingDoctorChecks orchestration.

Suggested labels

area: messaging, area: cli

Suggested reviewers

  • cjagwani
  • cv
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 5.00% 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 clearly matches the main change: surfacing Telegram config inputs in sandbox channels status.
Linked Issues check ✅ Passed The PR satisfies #5695 and #5691 by surfacing Telegram group policy and mention-mode values in status and doctor output.
Out of Scope Changes check ✅ Passed The changes stay focused on messaging diagnostics, Telegram manifest metadata, and supporting tests with no clear unrelated scope.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/channels-status-telegram-config-visibility

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

@laitingsheng laitingsheng added the bug-fix PR fixes a bug or regression label Jun 24, 2026
@github-code-quality

github-code-quality Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/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 fix/channels-status-... d67a997 +/-
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 fix/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 fix/channels-status-... d67a997 +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/shields/index.ts 70%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 59%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 29, 2026 06:14 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@laitingsheng laitingsheng added the integration: telegram Telegram integration or channel behavior label Jun 24, 2026
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: messaging-providers-vitest, channels-add-remove-vitest, diagnostics-vitest, sandbox-operations-vitest
Optional E2E: rebuild-openclaw-vitest, channels-stop-start-vitest

Dispatch hint: messaging-providers-vitest,channels-add-remove-vitest,diagnostics-vitest,sandbox-operations-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • messaging-providers-vitest (high): Most directly covers the touched messaging-provider runtime: install/onboard with messaging, provider placeholder/redaction contracts, Telegram/Slack/Discord/WhatsApp channel registration, policy coverage, and the newly relevant channels status --channel telegram --json plus doctor --json visible-config assertions.
  • channels-add-remove-vitest (high): Required because the PR changes messaging plan compilation, credential binding, OpenShell provider application, registry persistence, and policy-channel behavior. This lane exercises the real CLI/OpenShell rebuild path for adding and removing Telegram, including gateway credentials, policy-list, registry, and in-sandbox config state.
  • diagnostics-vitest (high): Required because doctor/status/diagnostics code paths changed, including doctor-messaging.ts, status rendering, gateway fallback, and credential-related diagnostics. This lane validates real debug/doctor/status and credential diagnostic behavior across the repo CLI, Docker, OpenShell, sandbox exec, and hosted inference boundaries.
  • sandbox-operations-vitest (high): Required because the PR touches sandbox lifecycle commands and supporting state (destroy, wipe-state, session export/delete, gateway state, host aliases, status text). This lane validates real sandbox list/status/exec/logs/process/gateway operations with Docker and OpenShell.

Optional E2E

  • rebuild-openclaw-vitest (high): Good adjacent confidence for changes in rebuild-flow-helpers.ts and rebuild-shields.ts. channels-add-remove-vitest exercises rebuild through the messaging flow, but this lane gives broader OpenClaw rebuild/state-preservation coverage if CI capacity allows.
  • channels-stop-start-vitest (high): Useful optional coverage for changed messaging registry/applier behavior around disabled channel state, provider reuse, policy-list, and rebuilds across OpenClaw/Hermes. Not strictly required if add/remove and messaging-providers pass, but relevant to the same channel lifecycle surface.

New E2E recommendations

  • messaging visible-config tamper resistance (medium): The unit tests cover tampered compact registry values and non-scalar persisted visible config, but existing live coverage appears to assert the valid Telegram visible-config path only. A live boundary check would catch serialization/registry/CLI regressions that accidentally echo unsafe persisted values in channels status or doctor.
    • Suggested test: Extend messaging-providers-vitest or add a focused live scenario that tampers the persisted Telegram visible-config values in the sandbox registry, then asserts channels status --json and doctor --json report bounded invalid-value messages without leaking raw values.
  • Hermes messaging visible-config agent scoping (low): The PR intentionally hides OpenClaw-only Telegram group policy for Hermes in unit tests, but the live messaging-providers lane is OpenClaw-focused. A Hermes live assertion would protect the agent-scoped visibility contract at the real CLI/registry boundary.
    • Suggested test: Add Hermes coverage to an existing messaging live scenario or a focused new scenario that enables Telegram-compatible config for Hermes and asserts OpenClaw-only visible config is omitted while shared visible config remains shown.

Dispatch hint

  • Workflow: e2e-vitest-scenarios.yaml
  • jobs input: messaging-providers-vitest,channels-add-remove-vitest,diagnostics-vitest,sandbox-operations-vitest

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: messaging-providers-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

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

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • messaging-providers-vitest: Focused free-standing Vitest job wired for changed live test test/e2e-scenario/live/messaging-providers.test.ts.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=messaging-providers-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/actions/sandbox/doctor-gateway-fallback.ts
  • src/lib/actions/sandbox/doctor-messaging.ts
  • src/lib/actions/sandbox/policy-channel.ts
  • src/lib/actions/sandbox/policy-context-refresh.ts
  • src/lib/actions/sandbox/rebuild-flow-helpers.ts
  • src/lib/actions/sandbox/status-text.ts
  • src/lib/messaging/applier/index.ts
  • src/lib/messaging/applier/openshell-provider.ts
  • src/lib/messaging/applier/types.ts
  • src/lib/messaging/channels/built-ins.ts
  • src/lib/messaging/channels/slack/hooks/credential-validation.ts
  • src/lib/messaging/channels/slack/hooks/validate-credentials.ts
  • src/lib/messaging/channels/teams/hooks/host-forward-port-conflict.ts
  • src/lib/messaging/channels/telegram/manifest.ts
  • src/lib/messaging/channels/template-resolver.ts
  • src/lib/messaging/channels/wechat/login.ts
  • src/lib/messaging/channels/wechat/template-resolver.ts
  • src/lib/messaging/compiler/engines/credential-binding-engine.ts
  • src/lib/messaging/compiler/index.ts
  • src/lib/messaging/compiler/manifest-compiler.ts
  • src/lib/messaging/diagnostics.ts
  • src/lib/messaging/hooks/common/token-paste.ts
  • src/lib/messaging/hooks/index.ts
  • src/lib/messaging/manifest/registry.ts
  • src/lib/messaging/manifest/types.ts
  • src/lib/messaging/plan-validation.ts
  • src/lib/messaging/visible-config-output.ts
  • src/lib/state/config-io.ts
  • src/lib/state/registry.ts
  • test/e2e-scenario/live/messaging-providers.test.ts

@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Resolve or justify PRA-1: Runtime Telegram onboarding and group-behavior clauses remain partially unvalidated.
Open items: 0 required · 1 warning · 0 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 1 still applies · 0 new items found

Action checklist

  • PRA-1 Resolve or justify: Runtime Telegram onboarding and group-behavior clauses remain partially unvalidated
  • 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: Acceptance clause
  • 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 acceptance Add targeted runtime/integration evidence for the still-uncovered linked clauses, or explicitly identify existing behavior-specific coverage and justify why it is sufficient for this PR. Keep the evidence in changed code/tests or local fixtures rather than relying on external job status.
Review findings by urgency: 0 required fixes, 1 item 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 — Runtime Telegram onboarding and group-behavior clauses remain partially unvalidated

  • Location: not file-specific
  • Category: acceptance
  • Problem: The changed code and tests now provide strong static/unit evidence for manifest opt-in, validValues bounding, compact registry readback, default rendering, agent scoping, and status/doctor output, plus one live installed OpenClaw/default-open status+doctor path. The linked issues also describe runtime operator flows that are still not fully represented by changed test evidence: host-shell-unset TELEGRAM_REQUIRE_MENTION during live onboarding, live allowlist/disabled TELEGRAM_GROUP_POLICY variants, interactive default onboarding, Hermes runtime visibility, and real Telegram group behavior where a plain group message is ignored but an @mention receives a reply.
  • Impact: A regression in one of the linked live onboarding/runtime paths could still leave operators unable to verify the effective Telegram settings from channels status or doctor, even though the renderer/planner unit tests pass for synthetic, compiled, or compact registry plans.
  • Recommended action: Add targeted runtime/integration evidence for the still-uncovered linked clauses, or explicitly identify existing behavior-specific coverage and justify why it is sufficient for this PR. Keep the evidence in changed code/tests or local fixtures rather than relying on external job status.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e-scenario/live/messaging-providers.test.ts around the Telegram visible status/doctor assertions; it covers one installed OpenClaw/default-open path. Compare with src/lib/actions/sandbox/channel-status-telegram-visibility.test.ts, src/lib/actions/sandbox/doctor-messaging-visibility.test.ts, src/lib/messaging/diagnostics.test.ts, and src/lib/messaging/compiler/planner-empty-env-normalization.test.ts, which cover the policy matrix, defaults, invalid/tampered values, compact registry readback, and Hermes hiding at unit level.
  • Missing regression test: Add or identify behavior-specific runtime evidence for: non-interactive live Telegram onboarding with TELEGRAM_REQUIRE_MENTION absent from the host env showing mention-only in both channels status and doctor; live TELEGRAM_GROUP_POLICY=allowlist and TELEGRAM_GROUP_POLICY=disabled onboarding/status assertions; interactive Telegram onboarding default mention-mode persistence and status/doctor display; a Hermes Telegram sandbox where status/doctor hide TELEGRAM_GROUP_POLICY but show mention mode; and an isolated credentialed Telegram group smoke proving a plain group message is ignored while an @mention receives a bot response.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e-scenario/live/messaging-providers.test.ts around the Telegram visible status/doctor assertions; it covers one installed OpenClaw/default-open path. Compare with src/lib/actions/sandbox/channel-status-telegram-visibility.test.ts, src/lib/actions/sandbox/doctor-messaging-visibility.test.ts, src/lib/messaging/diagnostics.test.ts, and src/lib/messaging/compiler/planner-empty-env-normalization.test.ts, which cover the policy matrix, defaults, invalid/tampered values, compact registry readback, and Hermes hiding at unit level.
  • Evidence: Issue [All Platforms][CLI&UX][GitHub Issue #5691] Telegram mention-only configuration not surfaced in NemoClaw status/doctor output for tg-default-sb #5691 requires an installed CLI, Docker, valid BotFather token, unset TELEGRAM_REQUIRE_MENTION, interactive or non-interactive onboarding, Ready status, status/doctor visibility, and practical mention-only group behavior. 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 requires live non-interactive open/allowlist/disabled TELEGRAM_GROUP_POLICY onboarding and channels status visibility. The diff covers the rendering matrix in unit tests and adds a live OpenClaw/default-open status+doctor assertion, but not the remaining live operator flows.

💡 In-scope improvements

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

  • None.
Test follow-ups to resolve or justify

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

  • PRA-T1 Runtime validation — Non-interactive live Telegram onboard with TELEGRAM_REQUIRE_MENTION absent from the host env, then assert both channels status --channel telegram and doctor show mention-only.. Unit coverage is strong for the security-sensitive rendering boundary, manifest contract, planner source normalization, compact registry readback, status/doctor output, and negative disclosure cases. The linked issues, however, include live Telegram onboarding/operator flows and real group-message behavior that unit tests and one OpenClaw/default live path do not fully prove.
  • PRA-T2 Runtime validation — Live Telegram onboarding/status assertions for TELEGRAM_GROUP_POLICY=allowlist and TELEGRAM_GROUP_POLICY=disabled, verifying allowlisted groups only and groups disabled in channels status.. Unit coverage is strong for the security-sensitive rendering boundary, manifest contract, planner source normalization, compact registry readback, status/doctor output, and negative disclosure cases. The linked issues, however, include live Telegram onboarding/operator flows and real group-message behavior that unit tests and one OpenClaw/default live path do not fully prove.
  • PRA-T3 Runtime validation — Interactive Telegram onboarding default mention-mode flow, verifying the persisted plan and status/doctor output render mention-only.. Unit coverage is strong for the security-sensitive rendering boundary, manifest contract, planner source normalization, compact registry readback, status/doctor output, and negative disclosure cases. The linked issues, however, include live Telegram onboarding/operator flows and real group-message behavior that unit tests and one OpenClaw/default live path do not fully prove.
  • PRA-T4 Runtime validation — Hermes Telegram sandbox status/doctor visibility check that hides TELEGRAM_GROUP_POLICY while still rendering Telegram mention mode.. Unit coverage is strong for the security-sensitive rendering boundary, manifest contract, planner source normalization, compact registry readback, status/doctor output, and negative disclosure cases. The linked issues, however, include live Telegram onboarding/operator flows and real group-message behavior that unit tests and one OpenClaw/default live path do not fully prove.
  • PRA-T5 Runtime validation — Credentialed isolated Telegram group smoke that sends one plain group message and one @mention, then verifies only the @mention receives a bot response.. Unit coverage is strong for the security-sensitive rendering boundary, manifest contract, planner source normalization, compact registry readback, status/doctor output, and negative disclosure cases. The linked issues, however, include live Telegram onboarding/operator flows and real group-message behavior that unit tests and one OpenClaw/default live path do not fully prove.
  • PRA-T6 Acceptance clause[All Platforms][CLI&UX][GitHub Issue #5691] Telegram mention-only configuration not surfaced in NemoClaw status/doctor output for tg-default-sb #5691 Environment: "NemoClaw version: v0.0.66" — add test evidence or identify existing coverage. This is historical repro context. The patch changes current source/tests and does not encode a version-specific compatibility assertion.
  • PRA-T7 Acceptance clause[All Platforms][CLI&UX][GitHub Issue #5691] Telegram mention-only configuration not surfaced in NemoClaw status/doctor output for tg-default-sb #5691 Environment: "Host: DGX Spark" — add test evidence or identify existing coverage. The changed live Vitest scenario is platform-gated by existing live fixtures, but the diff does not add a DGX Spark-specific assertion.
  • PRA-T8 Acceptance clause[All Platforms][CLI&UX][GitHub Issue #5691] Telegram mention-only configuration not surfaced in NemoClaw status/doctor output for tg-default-sb #5691 Environment: "Sandbox name: tg-default-sb" — add test evidence or identify existing coverage. Behavior is tested with synthetic alpha entries and the shared live SANDBOX_NAME; the changed code is name-independent but does not use the exact repro name.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Runtime Telegram onboarding and group-behavior clauses remain partially unvalidated

  • Location: not file-specific
  • Category: acceptance
  • Problem: The changed code and tests now provide strong static/unit evidence for manifest opt-in, validValues bounding, compact registry readback, default rendering, agent scoping, and status/doctor output, plus one live installed OpenClaw/default-open status+doctor path. The linked issues also describe runtime operator flows that are still not fully represented by changed test evidence: host-shell-unset TELEGRAM_REQUIRE_MENTION during live onboarding, live allowlist/disabled TELEGRAM_GROUP_POLICY variants, interactive default onboarding, Hermes runtime visibility, and real Telegram group behavior where a plain group message is ignored but an @mention receives a reply.
  • Impact: A regression in one of the linked live onboarding/runtime paths could still leave operators unable to verify the effective Telegram settings from channels status or doctor, even though the renderer/planner unit tests pass for synthetic, compiled, or compact registry plans.
  • Recommended action: Add targeted runtime/integration evidence for the still-uncovered linked clauses, or explicitly identify existing behavior-specific coverage and justify why it is sufficient for this PR. Keep the evidence in changed code/tests or local fixtures rather than relying on external job status.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read test/e2e-scenario/live/messaging-providers.test.ts around the Telegram visible status/doctor assertions; it covers one installed OpenClaw/default-open path. Compare with src/lib/actions/sandbox/channel-status-telegram-visibility.test.ts, src/lib/actions/sandbox/doctor-messaging-visibility.test.ts, src/lib/messaging/diagnostics.test.ts, and src/lib/messaging/compiler/planner-empty-env-normalization.test.ts, which cover the policy matrix, defaults, invalid/tampered values, compact registry readback, and Hermes hiding at unit level.
  • Missing regression test: Add or identify behavior-specific runtime evidence for: non-interactive live Telegram onboarding with TELEGRAM_REQUIRE_MENTION absent from the host env showing mention-only in both channels status and doctor; live TELEGRAM_GROUP_POLICY=allowlist and TELEGRAM_GROUP_POLICY=disabled onboarding/status assertions; interactive Telegram onboarding default mention-mode persistence and status/doctor display; a Hermes Telegram sandbox where status/doctor hide TELEGRAM_GROUP_POLICY but show mention mode; and an isolated credentialed Telegram group smoke proving a plain group message is ignored while an @mention receives a bot response.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read test/e2e-scenario/live/messaging-providers.test.ts around the Telegram visible status/doctor assertions; it covers one installed OpenClaw/default-open path. Compare with src/lib/actions/sandbox/channel-status-telegram-visibility.test.ts, src/lib/actions/sandbox/doctor-messaging-visibility.test.ts, src/lib/messaging/diagnostics.test.ts, and src/lib/messaging/compiler/planner-empty-env-normalization.test.ts, which cover the policy matrix, defaults, invalid/tampered values, compact registry readback, and Hermes hiding at unit level.
  • Evidence: Issue [All Platforms][CLI&UX][GitHub Issue #5691] Telegram mention-only configuration not surfaced in NemoClaw status/doctor output for tg-default-sb #5691 requires an installed CLI, Docker, valid BotFather token, unset TELEGRAM_REQUIRE_MENTION, interactive or non-interactive onboarding, Ready status, status/doctor visibility, and practical mention-only group behavior. 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 requires live non-interactive open/allowlist/disabled TELEGRAM_GROUP_POLICY onboarding and channels status visibility. The diff covers the rendering matrix in unit tests and adds a live OpenClaw/default-open status+doctor assertion, but not the remaining live operator flows.

Workflow run details

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

Signed-off-by: Tinson Lai <tinsonl@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.

Actionable comments posted: 1

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

Inline comments:
In `@src/lib/actions/sandbox/doctor.ts`:
- Around line 569-570: In the formatVisibleConfigValue function, empty arrays
are being rendered as blank strings because join(", ") on an empty array
produces an empty string. This hides the fact that a configuration value was
explicitly set. Modify the array handling logic to check if the array is empty
and return a visible representation (such as "[]") to indicate an empty array
was configured, before applying the map and join operations for non-empty
arrays.
🪄 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: bf64f189-7818-4a42-a2dc-3433a31c4c97

📥 Commits

Reviewing files that changed from the base of the PR and between a20b198 and 53a053c.

📒 Files selected for processing (3)
  • src/lib/actions/sandbox/channel-status.test.ts
  • src/lib/actions/sandbox/doctor-flow.test.ts
  • src/lib/actions/sandbox/doctor.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/lib/actions/sandbox/channel-status.test.ts

Comment thread src/lib/actions/sandbox/doctor.ts Outdated
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…tics

Signed-off-by: Tinson Lai <tinsonl@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.

♻️ Duplicate comments (1)
src/lib/messaging/diagnostics.ts (1)

96-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Empty array persists as a blank ok detail.

An empty array [] passes the rawValue !== "" guard on Line 96 (it's not the empty string), so it's treated as "persisted", but stringifyValue([]) on Line 121 returns "". The signal then renders a blank detail with ok severity, hiding that a value was explicitly configured. This is the same root cause previously flagged in doctor.ts, now centralized here in resolveVisibleConfigDisplay/stringifyValue.

Proposed fix
 function stringifyValue(value: MessagingSerializableValue): string {
   if (typeof value === "string") return value;
   if (typeof value === "boolean") return value ? "true" : "false";
   if (typeof value === "number") return String(value);
-  if (Array.isArray(value)) return value.map(stringifyValue).join(", ");
+  if (Array.isArray(value)) {
+    if (value.length === 0) return "[]";
+    return value.map(stringifyValue).join(", ");
+  }
   return JSON.stringify(value);
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/lib/messaging/diagnostics.ts` around lines 96 - 122, The empty array case
in resolveVisibleConfigDisplay/stringifyValue is being treated as a persisted
value but renders as a blank detail. Update the guard in
resolveVisibleConfigDisplay so values that stringify to an empty string
(especially []) are not returned as persisted, or make stringifyValue return a
non-empty representation for empty arrays. Ensure the fix keeps explicit empty
array configuration visible instead of producing an empty ok detail.
🤖 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.

Duplicate comments:
In `@src/lib/messaging/diagnostics.ts`:
- Around line 96-122: The empty array case in
resolveVisibleConfigDisplay/stringifyValue is being treated as a persisted value
but renders as a blank detail. Update the guard in resolveVisibleConfigDisplay
so values that stringify to an empty string (especially []) are not returned as
persisted, or make stringifyValue return a non-empty representation for empty
arrays. Ensure the fix keeps explicit empty array configuration visible instead
of producing an empty ok detail.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 26a568c2-d18a-43ad-88e4-e89f8536a8d0

📥 Commits

Reviewing files that changed from the base of the PR and between 581ad20 and 371c38d.

📒 Files selected for processing (7)
  • src/lib/actions/sandbox/channel-status.test.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/actions/sandbox/doctor-flow.test.ts
  • src/lib/actions/sandbox/doctor.ts
  • src/lib/messaging/channels/telegram/manifest.ts
  • src/lib/messaging/diagnostics.ts
  • src/lib/messaging/manifest/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/actions/sandbox/doctor-flow.test.ts
  • src/lib/actions/sandbox/channel-status.ts

@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

🤖 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/doctor.ts`:
- Around line 529-535: Default the doctor agent the same way as channels status
by updating showSandboxDoctor’s agent selection before calling
collectVisibleConfigRecords. The current sb.agent ?? null path can leave older
sandboxes with a null agent, which causes OpenClaw-only config inputs to be
skipped; resolve the agent using the same fallback behavior as
showSandboxChannelStatus (entry.agent || "openclaw") and then pass that through
asMessagingAgent so doctor and channels status stay consistent.
🪄 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: d975e440-410d-4325-9064-63be07fcf0a5

📥 Commits

Reviewing files that changed from the base of the PR and between 371c38d and 120a479.

📒 Files selected for processing (6)
  • src/lib/actions/sandbox/channel-status.test.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/actions/sandbox/doctor.ts
  • src/lib/messaging/channels/telegram/manifest.ts
  • src/lib/messaging/diagnostics.ts
  • src/lib/messaging/manifest/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/lib/messaging/channels/telegram/manifest.ts
  • src/lib/actions/sandbox/channel-status.ts
  • src/lib/messaging/manifest/types.ts

Comment thread src/lib/actions/sandbox/doctor.ts Outdated
…d-plan path

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…tor paths

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@jyaunches jyaunches added v0.0.69 and removed v0.0.68 labels Jun 25, 2026
@wscurran wscurran added NV QA Bugs found by the NVIDIA QA Team VDR Linked to VDR finding labels Jun 26, 2026
…elegram-config-visibility

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

# Conflicts:
#	src/lib/actions/sandbox/channel-status.ts
#	src/lib/actions/sandbox/doctor.ts
@github-actions

github-actions Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Blocked

Merge posture: Do not merge until addressed
Primary next action: Fix PRA-3: Test monolith not split after helper extraction (carried from previous review); then add or justify PRA-T1.
Open items: 4 required · 10 warnings · 5 suggestions · 8 test follow-ups
Since last review: 3 prior items resolved · 7 still apply · 8 new items found

Action checklist

  • PRA-3 Fix: Test monolith not split after helper extraction (carried from previous review) in src/lib/actions/sandbox/channel-status.test.ts:1
  • PRA-4 Fix: Test monolith not split after helper extraction (carried from previous review) in src/lib/actions/sandbox/doctor-flow.test.ts:1
  • PRA-5 Fix: Legacy agent fallback lacks migration plan in src/lib/actions/sandbox/doctor-messaging.ts:300
  • PRA-6 Fix: Documentation duplicated across two .mdx files in docs/reference/commands.mdx:831
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/actions/sandbox/doctor-messaging.ts:300 / channel-status.ts:555
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/messaging/diagnostics.ts:198
  • PRA-7 Resolve or justify: Source-of-truth review needed: Legacy agent fallback in src/lib/actions/sandbox/doctor-messaging.ts:300
  • PRA-8 Resolve or justify: Source-of-truth review needed: inputAppliesToAgent null agent handling in src/lib/messaging/diagnostics.ts:198
  • PRA-9 Resolve or justify: Source-of-truth review needed: fakePlanFromInputs silent null return in src/lib/actions/sandbox/test-utils/index.ts:222
  • PRA-10 Resolve or justify: Authorization bypass via legacy agent fallback in agent applicability filter in src/lib/messaging/diagnostics.ts:198
  • PRA-11 Resolve or justify: Test monolith growth exceeds 20-line threshold without validation test expansion in src/lib/messaging/manifest/types.ts:486
  • PRA-12 Resolve or justify: Test monolith growth exceeds 20-line threshold in manifest-compiler.test.ts in src/lib/messaging/compiler/manifest-compiler.test.ts:1424
  • PRA-13 Resolve or justify: Fixture exclusion self-audit scope change needs verification in scripts/checks/no-test-dist-imports.ts:1
  • PRA-14 Resolve or justify: Missing registry validation test for agentApplicability with unsupported agent in src/lib/messaging/diagnostics.ts:70
  • 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: Test monolith not split after helper extraction (carried from previous review)
  • PRA-T7 Add or justify test follow-up: Test monolith not split after helper extraction (carried from previous review)
  • PRA-T8 Add or justify test follow-up: Test monolith growth exceeds 20-line threshold without validation test expansion
  • PRA-15 In-scope improvement: Defensive bounding for tampered plans — empty string treated as invalid in src/lib/messaging/diagnostics.ts:110
  • PRA-16 In-scope improvement: Verify normalizeInputValue empty string handling test runs in CI in src/lib/messaging/compiler/manifest-compiler.ts:382
  • PRA-17 In-scope improvement: Test helper extraction could reduce scaffold in compact registry tests in src/lib/actions/sandbox/channel-status.test.ts:729
  • PRA-18 In-scope improvement: Integration test scaffold uses requireDist + spies for unit-level visible-config tests in src/lib/actions/sandbox/doctor-flow.test.ts:213
  • PRA-19 In-scope improvement: fakePlanFromInputs returns null silently for missing base plan in src/lib/actions/sandbox/test-utils/index.ts:222

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 tests src/lib/actions/sandbox/channel-status.test.ts:1 Split channel-status.test.ts into feature-area test files before merge. Shared helpers in __test-utils__/index.ts and telegram-plan.ts already exist.
PRA-4 Required tests src/lib/actions/sandbox/doctor-flow.test.ts:1 Split doctor-flow.test.ts into feature-area test files before merge. Shared doctor-harness.ts helper already exists.
PRA-5 Required correctness src/lib/actions/sandbox/doctor-messaging.ts:300 Define concrete migration in this PR: add registry upgrade step that backfills agent field on legacy entries based on imageTag or config paths, then remove fallback. Or make agent field required at registry read time with explicit error for legacy entries. Document chosen path and removal timeline.
PRA-6 Required docs docs/reference/commands.mdx:831 Extract shared content to a common include (e.g., docs/reference/_visible-config-rules.mdx) or single-source via cross-reference. Use MDX import or shared snippet.
PRA-7 Resolve/justify architecture src/lib/actions/sandbox/doctor-messaging.ts:300 Document the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add explicit warning signal when agent detection fails rather than silently returning null.
PRA-8 Resolve/justify architecture src/lib/messaging/diagnostics.ts:198 Add explicit agent context requirement at registry read boundary or emit warning diagnostic when agent-scoped input skipped due to missing agent. Current test only covers doctor path, not registry read path.
PRA-9 Resolve/justify architecture src/lib/actions/sandbox/test-utils/index.ts:222 Change to throw descriptive error or return Result type. Callers already provide valid base plans, so safe to tighten.
PRA-10 Resolve/justify security src/lib/messaging/diagnostics.ts:198 Add explicit warning diagnostic when agent field missing and agent-scoped inputs exist. Or require agent field at registry read with explicit error.
PRA-11 Resolve/justify tests src/lib/messaging/manifest/types.ts:486 Add registry.test.ts cases: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
PRA-12 Resolve/justify tests src/lib/messaging/compiler/manifest-compiler.test.ts:1424 Extract Telegram planner boundary tests to dedicated test file (e.g., telegram-planner-boundary.test.ts) to offset growth.
PRA-13 Resolve/justify correctness scripts/checks/no-test-dist-imports.ts:1 Run the check locally: `node scripts/checks/no-test-dist-imports.ts`. If any excluded fixture fails, rewrite to use path construction only or move to test/package-contract/.
PRA-14 Resolve/justify security src/lib/messaging/diagnostics.ts:70 Add test in registry.test.ts verifying registration throws when agentApplicability contains agent not in supportedAgents.
PRA-15 Improvement architecture src/lib/messaging/diagnostics.ts:110 Keep the defensive bounding. Add test in workflow-planner.test.ts for empty string vs undefined distinction at planner boundary (complements existing test at line 753).
PRA-16 Improvement tests src/lib/messaging/compiler/manifest-compiler.ts:382 Verify existing test at manifest-compiler.test.ts:1441-1444 runs in CI (already covered).
PRA-17 Improvement architecture src/lib/actions/sandbox/channel-status.test.ts:729 Refactor channel-status.test.ts to use shared test utilities from __test-utils__/index.ts. Not required for merge.
PRA-18 Improvement architecture src/lib/actions/sandbox/doctor-flow.test.ts:213 For pure unit tests of doctor-messaging logic, consider direct imports. Keep requireDist for integration boundary tests. Not required for merge.
PRA-19 Improvement architecture src/lib/actions/sandbox/test-utils/index.ts:222 Change to throw descriptive error or return Result type.

🚨 Required before merge

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

PRA-3 Required — Test monolith not split after helper extraction (carried from previous review)

  • Location: src/lib/actions/sandbox/channel-status.test.ts:1
  • Category: tests
  • Problem: channel-status.test.ts remains ~259 lines with 13 test cases covering WhatsApp deep probe, basic per-channel reports, Telegram visibility (partial), and paused channel handling. Previous review explicitly required split into: whatsapp-deep-probe.test.ts, telegram-visibility.test.ts, basic-channel-status.test.ts, paused-channel-status.test.ts. New telegram-visibility tests extracted to separate file but original monolith not split.
  • Impact: Maintenance burden, risk of divergent helper implementations, harder to update test infrastructure. Exceeds 20-line growth threshold from baseline.
  • Required action: Split channel-status.test.ts into feature-area test files before merge. Shared helpers in __test-utils__/index.ts and telegram-plan.ts already exist.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/channel-status.test.ts shows ~259 lines; grep -c 'describe\|it' shows ~13 test cases across 4 feature areas
  • Missing regression test: Structural cleanup verified by file size and grep absence of inline helper definitions
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/channel-status.test.ts shows ~259 lines; grep -c 'describe\|it' shows ~13 test cases across 4 feature areas.
  • Evidence: File imports compileTelegramPlanForTests, fakePlanFromInputs, tamperCompactRegistryTelegramInputs from shared utils but contains all test suites inline

PRA-4 Required — Test monolith not split after helper extraction (carried from previous review)

  • Location: src/lib/actions/sandbox/doctor-flow.test.ts:1
  • Category: tests
  • Problem: doctor-flow.test.ts remains ~272 lines with 12 test cases covering doctor JSON report, --fix rejection, OpenShell unavailable, gateway disconnected, read-only diagnostics, live probes after recovery, no repairs, agent-specific tool-scope, local gateway result, agent definition failures. Previous review explicitly required split into: doctor-core-flow.test.ts, doctor-messaging-visibility.test.ts, doctor-runtime-probes.test.ts, doctor-gateway-inference.test.ts. New messaging-visibility tests extracted but original monolith not split.
  • Impact: Maintenance burden, risk of divergent helper implementations, harder to update test infrastructure. Exceeds 20-line growth threshold from baseline.
  • Required action: Split doctor-flow.test.ts into feature-area test files before merge. Shared doctor-harness.ts helper already exists.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/doctor-flow.test.ts shows ~272 lines; grep -c 'describe\|it' shows ~12 test cases across 4 feature areas
  • Missing regression test: Structural cleanup verified by file size and grep absence of inline helper definitions
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/doctor-flow.test.ts shows ~272 lines; grep -c 'describe\|it' shows ~12 test cases across 4 feature areas.
  • Evidence: File uses requireDist to load compiled doctor.js and spies on internal modules; doctor-harness.ts already provides shared harness

PRA-5 Required — Legacy agent fallback lacks migration plan

  • Location: src/lib/actions/sandbox/doctor-messaging.ts:300
  • Category: correctness
  • Problem: Line 300 calls `asMessagingAgent(sb.agent)` where `sb.agent` can be undefined for legacy registry entries. The `asMessagingAgent` function returns null for undefined input, which then causes `inputAppliesToAgent` to return false for agent-scoped visible config inputs (e.g., Telegram group policy hidden on Hermes). This is a silent behavior change for legacy sandboxes with no migration path documented.
  • Impact: Legacy sandboxes without agent field will silently hide agent-applicable visible config instead of showing a warning or migrating. No regression test covers legacy entry handling.
  • Required action: Define concrete migration in this PR: add registry upgrade step that backfills agent field on legacy entries based on imageTag or config paths, then remove fallback. Or make agent field required at registry read time with explicit error for legacy entries. Document chosen path and removal timeline.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check src/lib/actions/sandbox/doctor-messaging.ts:300 and src/lib/messaging/diagnostics.ts:198 for agent field handling
  • Missing regression test: Test in doctor-messaging-visibility.test.ts verifying legacy entry (no agent field) behavior for agent-scoped visible config
  • Done when: The required change is committed and verification passes: Check src/lib/actions/sandbox/doctor-messaging.ts:300 and src/lib/messaging/diagnostics.ts:198 for agent field handling.
  • Evidence: doctor-messaging.ts:300 and channel-status.ts:555 both use asMessagingAgent(sb.agent) without null check or warning

PRA-6 Required — Documentation duplicated across two .mdx files

  • Location: docs/reference/commands.mdx:831
  • Category: docs
  • Problem: The visible config rendering rules note appears identically in both commands.mdx and commands-nemohermes.mdx under 'channels status' and 'doctor' sections. The note explains: 'The Messaging section also surfaces one check per opted-in visible config input so operators can confirm the active policy without inspecting logs. The rendering rules (manifest opt-in via safeToPrintInDiagnostics, agent-scoped hiding, manifest allowlist validation) are shared with channels status and documented under channels status.'
  • Impact: Documentation drift risk — updates to one file won't propagate to the other. Violates single-source-of-truth principle.
  • Required action: Extract shared content to a common include (e.g., docs/reference/_visible-config-rules.mdx) or single-source via cross-reference. Use MDX import or shared snippet.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: diff the 'channels status' and 'doctor' sections in both .mdx files for the visible config note
  • Missing regression test: Documentation lint/check that shared content is not duplicated
  • Done when: The required change is committed and verification passes: diff the 'channels status' and 'doctor' sections in both .mdx files for the visible config note.
  • Evidence: Both docs/reference/commands.mdx and docs/reference/commands-nemohermes.mdx contain identical 4-line note about visible config rendering rules
Review findings by urgency: 4 required fixes, 10 items to resolve/justify, 5 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/doctor-messaging.ts:300 / channel-status.ts:555

  • 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: Doctor test exists for legacy entry hiding agent-scoped config; need channel-status test and explicit warning signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: asMessagingAgent(undefined) returns null; inputAppliesToAgent returns false for agent-scoped inputs

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/messaging/diagnostics.ts:198

  • 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: Doctor test covers legacy entry; need registry.test.ts validation for agentApplicability
  • 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: inputAppliesToAgent returns false for null agent when agentApplicability non-empty

PRA-7 Resolve/justify — Source-of-truth review needed: Legacy agent fallback

  • Location: src/lib/actions/sandbox/doctor-messaging.ts:300
  • Category: architecture
  • Problem: Legacy agent fallback at doctor-messaging.ts:300 and channel-status.ts:555. The invalid state is legacy SandboxEntry missing agent field. Source boundary is registry read. Source cannot be fixed in this PR because migration requires registry upgrade step. Regression test needed: verify legacy entry behavior. Workaround can be removed when all legacy entries migrated.
  • Impact: Localized workaround behavior without clear removal condition. Silent hiding of agent-scoped config for legacy entries.
  • Recommended action: Document the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add explicit warning signal when agent detection fails rather than silently returning null.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'asMessagingAgent' src/lib/actions/sandbox/doctor-messaging.ts src/lib/actions/sandbox/channel-status.ts
  • Missing regression test: Test verifying legacy entry (missing agent) produces explicit warning or error signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'asMessagingAgent' src/lib/actions/sandbox/doctor-messaging.ts src/lib/actions/sandbox/channel-status.ts.
  • Evidence: asMessagingAgent(undefined) returns null; inputAppliesToAgent returns false for agent-scoped inputs when agent is null

PRA-8 Resolve/justify — Source-of-truth review needed: inputAppliesToAgent null agent handling

  • Location: src/lib/messaging/diagnostics.ts:198
  • Category: architecture
  • Problem: inputAppliesToAgent at diagnostics.ts:198 returns false when agent is null for agent-scoped inputs. The invalid state is plan/compiled entry without agent context. Source boundary is diagnostics collection. Source cannot be fixed without propagating agent context through all callers. Regression test exists in doctor-messaging-visibility.test.ts:59-68 but should be in registry.test.ts. Workaround can be removed when agent field is required at registry read.
  • Impact: Agent-scoped visible config silently hidden when agent context unavailable. Could mask misconfiguration.
  • Recommended action: Add explicit agent context requirement at registry read boundary or emit warning diagnostic when agent-scoped input skipped due to missing agent. Current test only covers doctor path, not registry read path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'inputAppliesToAgent' src/lib/messaging/diagnostics.ts
  • Missing regression test: Test in registry.test.ts verifying registration throws when agentApplicability contains agent not in supportedAgents (also PRA-12)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'inputAppliesToAgent' src/lib/messaging/diagnostics.ts.
  • Evidence: inputAppliesToAgent returns false for null agent when agentApplicability is non-empty

PRA-9 Resolve/justify — Source-of-truth review needed: fakePlanFromInputs silent null return

  • Location: src/lib/actions/sandbox/test-utils/index.ts:222
  • Category: architecture
  • Problem: fakePlanFromInputs returns null silently for missing base plan. The invalid state is sandbox entry without messaging plan. Source boundary is test utility. Source cannot be fixed because callers control sandbox fixture. Regression test: callers in channel-status-telegram-visibility.test.ts and doctor-messaging-visibility.test.ts already provide valid base plans. Workaround can be removed when callers guaranteed valid plans.
  • Impact: Test helper silently returns null instead of throwing, masking test setup errors.
  • Recommended action: Change to throw descriptive error or return Result type. Callers already provide valid base plans, so safe to tighten.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts
  • Missing regression test: Unit test for fakePlanFromInputs throwing on null base plan
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts.
  • Evidence: Function returns `base && channelInputs ? mergePlanInputs(base, channelInputs) : base` where base can be null

PRA-10 Resolve/justify — Authorization bypass via legacy agent fallback in agent applicability filter

  • Location: src/lib/messaging/diagnostics.ts:198
  • Category: security
  • Problem: When agent field is missing from SandboxEntry, asMessagingAgent returns null, causing inputAppliesToAgent to return false for agent-scoped inputs. This silently hides OpenClaw-only config (Telegram group policy) for legacy entries instead of treating as unknown agent. Fix together with PRA-6. When agent field is missing, either error explicitly or treat as unknown agent that matches no agentApplicability (current behavior) but add a warning signal that agent detection failed.
  • Impact: Legacy sandboxes without agent field silently lose visibility into agent-scoped config. No audit trail that agent detection failed.
  • Recommended action: Add explicit warning diagnostic when agent field missing and agent-scoped inputs exist. Or require agent field at registry read with explicit error.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check doctor-messaging-visibility.test.ts line 59-68 for 'hides agent-applicability-restricted visible config when a legacy SandboxEntry omits the agent field'
  • Missing regression test: Test verifying warning signal emitted when agent field missing and agent-scoped inputs present
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check doctor-messaging-visibility.test.ts line 59-68 for 'hides agent-applicability-restricted visible config when a legacy SandboxEntry omits the agent field'.
  • Evidence: inputAppliesToAgent returns false for null agent; no warning diagnostic emitted

PRA-11 Resolve/justify — Test monolith growth exceeds 20-line threshold without validation test expansion

  • Location: src/lib/messaging/manifest/types.ts:486
  • Category: tests
  • Problem: manifest/types.ts grew by 22 lines. The growth adds visibleConfigInputs fields to ChannelManifest types but validation tests in registry.test.ts only cover valueDisplay key validation and agentApplicability validation. Missing: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
  • Impact: Type expansion without corresponding validation test coverage increase.
  • Recommended action: Add registry.test.ts cases: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/messaging/manifest/types.ts shows 508 lines (base 486); check registry.test.ts for corresponding validation tests
  • Missing regression test: Three validation test cases in registry.test.ts for the new visibleConfigInputs contract
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/messaging/manifest/types.ts shows 508 lines (base 486); check registry.test.ts for corresponding validation tests.
  • Evidence: manifest/types.ts added visibleConfigInputs to ChannelInputSpec; registry.test.ts validates valueDisplay keys and agentApplicability but not the three missing cases

PRA-12 Resolve/justify — Test monolith growth exceeds 20-line threshold in manifest-compiler.test.ts

  • Location: src/lib/messaging/compiler/manifest-compiler.test.ts:1424
  • Category: tests
  • Problem: File grew by 44 lines (1424→1468). New tests cover Telegram planner boundary and normalizeInputValue. These should be extracted to dedicated test file (e.g., telegram-planner-boundary.test.ts) to offset growth. normalizeInputValue tests could stay as they're compiler-unit focused.
  • Impact: Test file bloat makes maintenance harder. Telegram planner tests are integration-style and belong in separate file.
  • Recommended action: Extract Telegram planner boundary tests to dedicated test file (e.g., telegram-planner-boundary.test.ts) to offset growth.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/messaging/compiler/manifest-compiler.test.ts shows 1468 lines; grep -c 'describe\|it' for telegram planner tests
  • Missing regression test: Extracted test file with telegram planner boundary tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/messaging/compiler/manifest-compiler.test.ts shows 1468 lines; grep -c 'describe\|it' for telegram planner tests.
  • Evidence: manifest-compiler.test.ts is a known large-file hotspot; growth not offset by extraction

PRA-13 Resolve/justify — Fixture exclusion self-audit scope change needs verification

  • Location: scripts/checks/no-test-dist-imports.ts:1
  • Category: correctness
  • Problem: The check now distinguishes dynamic-require path construction (allowed in FIXTURE_EXCLUSIONS) from bare import specifiers (not allowed). Self-audit verifies excluded fixtures still construct compiled-internal paths through path.join/require/template. Need to run locally to confirm no excluded fixture fails.
  • Impact: If any excluded fixture now contains bare imports, the check will fail and block CI. The change is correct but needs local verification.
  • Recommended action: Run the check locally: `node scripts/checks/no-test-dist-imports.ts`. If any excluded fixture fails, rewrite to use path construction only or move to test/package-contract/.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: node scripts/checks/no-test-dist-imports.ts
  • Missing regression test: CI run of no-test-dist-imports check passes
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: node scripts/checks/no-test-dist-imports.ts.
  • Evidence: FIXTURE_EXCLUSIONS still contains test/dist-sourcemaps.test.ts, test/install-preflight.test.ts, test/stale-dist-check.test.ts; new logic distinguishes path-construction vs bare imports

PRA-14 Resolve/justify — Missing registry validation test for agentApplicability with unsupported agent

  • Location: src/lib/messaging/diagnostics.ts:70
  • Category: security
  • Problem: registry.test.ts line 70 area has validation for valueDisplay keys and secret safeToPrintInDiagnostics but no test for agentApplicability referencing agent not in supportedAgents. This is a manifest contract validation gap.
  • Impact: Manifest could declare agentApplicability for unsupported agent, causing silent exclusion at runtime without registration-time error.
  • Recommended action: Add test in registry.test.ts verifying registration throws when agentApplicability contains agent not in supportedAgents.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -A20 'agentApplicability' src/lib/messaging/manifest/registry.test.ts
  • Missing regression test: Test case in registry.test.ts: expect(() => createChannelManifestRegistry([manifestWithInvalidAgentApplicability])).toThrow('agentApplicability.*not in supportedAgents')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -A20 'agentApplicability' src/lib/messaging/manifest/registry.test.ts.
  • Evidence: registry.test.ts validates valueDisplay keys and secret safeToPrintInDiagnostics but not agentApplicability against supportedAgents

💡 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-15 Improvement — Defensive bounding for tampered plans — empty string treated as invalid

  • Location: src/lib/messaging/diagnostics.ts:110
  • Category: architecture
  • Problem: resolveVisibleConfigDisplay correctly treats present-but-empty persisted value as invalid rather than falling back to default. This prevents masking corrupted plans. Good defensive behavior.
  • Impact: Positive security posture — tampered empty strings surface as invalid.
  • Suggested action: Keep the defensive bounding. Add test in workflow-planner.test.ts for empty string vs undefined distinction at planner boundary (complements existing test at line 753).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'invalid persisted value' src/lib/messaging/diagnostics.ts
  • Missing regression test: Test in workflow-planner.test.ts for empty string normalization at planner boundary
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: resolveVisibleConfigDisplay returns invalidPersistedDisplay for planInputPresent with empty string value

PRA-16 Improvement — Verify normalizeInputValue empty string handling test runs in CI

  • Location: src/lib/messaging/compiler/manifest-compiler.ts:382
  • Category: tests
  • Problem: The normalizeInputValue tests at manifest-compiler.test.ts:1441-1444 cover empty/whitespace/null/undefined → undefined. Already covered but should confirm CI execution.
  • Impact: Low — tests exist but CI verification needed.
  • Suggested action: Verify existing test at manifest-compiler.test.ts:1441-1444 runs in CI (already covered).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check CI logs for 'normalizeInputValue' test execution
  • Missing regression test: Already covered by existing tests
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: normalizeInputValue tests exist and cover empty string → undefined

PRA-17 Improvement — Test helper extraction could reduce scaffold in compact registry tests

  • Location: src/lib/actions/sandbox/channel-status.test.ts:729
  • Category: architecture
  • Problem: channel-status.test.ts duplicates fakeAgent and entry helpers that exist in __test-utils__/index.ts. Not required for merge but would improve consistency.
  • Impact: Code duplication in test helpers.
  • Suggested action: Refactor channel-status.test.ts to use shared test utilities from __test-utils__/index.ts. Not required for merge.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare fakeAgent/entry in channel-status.test.ts with fakeChannelStatusAgent/channelStatusEntry in __test-utils__/index.ts
  • Missing regression test: None — structural improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: channel-status.test.ts defines its own fakeAgent and entry functions duplicating __test-utils__/index.ts

PRA-18 Improvement — Integration test scaffold uses requireDist + spies for unit-level visible-config tests

  • Location: src/lib/actions/sandbox/doctor-flow.test.ts:213
  • Category: architecture
  • Problem: doctor-flow.test.ts uses requireDist to load compiled doctor.js and spies on internal modules. For pure unit tests of doctor-messaging logic, consider direct imports. Keep requireDist for integration boundary tests.
  • Impact: Test architecture mixes unit and integration styles.
  • Suggested action: For pure unit tests of doctor-messaging logic, consider direct imports. Keep requireDist for integration boundary tests. Not required for merge.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check doctor-flow.test.ts imports and requireDist usage
  • Missing regression test: None — architectural improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: doctor-flow.test.ts uses requireDist and spies on internal modules for visible-config tests

PRA-19 Improvement — fakePlanFromInputs returns null silently for missing base plan

  • Location: src/lib/actions/sandbox/test-utils/index.ts:222
  • Category: architecture
  • Problem: fakePlanFromInputs returns null silently for missing base plan. Change to throw descriptive error or return Result type. Callers in channel-status-telegram-visibility.test.ts and doctor-messaging-visibility.test.ts already provide valid base plans, so safe to tighten.
  • Impact: Test helper masks setup errors by returning null.
  • Suggested action: Change to throw descriptive error or return Result type.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts
  • Missing regression test: Unit test for fakePlanFromInputs throwing on null base plan
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function returns null when base is null/undefined
Simplification opportunities: 8 possible cuts, net -674 lines possible

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

  • PRA-3 shrink (src/lib/actions/sandbox/channel-status.test.ts:1): channel-status.test.ts monolith (513 lines)
    • Replacement: Four feature-area test files using shared __test-utils__/index.ts helpers
    • Net: -400 lines
    • Safety boundary: Shared test utilities in __test-utils__/index.ts and telegram-plan.ts must remain unchanged
  • PRA-4 shrink (src/lib/actions/sandbox/doctor-flow.test.ts:1): doctor-flow.test.ts monolith (272 lines)
    • Replacement: Four feature-area test files using shared doctor-harness.ts
    • Net: -200 lines
    • Safety boundary: doctor-harness.ts shared utilities must remain unchanged
  • PRA-5 yagni (src/lib/actions/sandbox/doctor-messaging.ts:300): Silent fallback to OpenClaw agent for legacy entries
    • Replacement: Explicit migration step or required agent field with error
    • Net: 10 lines
    • Safety boundary: Must not break existing legacy sandbox operations; migration must be documented
  • PRA-6 stdlib (docs/reference/commands.mdx:831): Duplicated visible config note in both .mdx files (4 lines each)
    • Replacement: Single shared include file imported in both locations
    • Net: -4 lines
    • Safety boundary: Documentation content must remain accurate and accessible
  • PRA-9 yagni (src/lib/actions/sandbox/__test-utils__/index.ts:222): Silent null return in fakePlanFromInputs
    • Replacement: Throw descriptive error when base plan is null/undefined
    • Net: 5 lines
    • Safety boundary: Callers in channel-status-telegram-visibility.test.ts and doctor-messaging-visibility.test.ts already provide valid base plans
  • PRA-12 shrink (src/lib/messaging/compiler/manifest-compiler.test.ts:1424): Telegram planner boundary tests from manifest-compiler.test.ts
    • Replacement: New telegram-planner-boundary.test.ts file
    • Net: -40 lines
    • Safety boundary: normalizeInputValue tests should remain in manifest-compiler.test.ts
  • PRA-17 shrink (src/lib/actions/sandbox/channel-status.test.ts:729): Duplicate fakeAgent and entry helper functions in channel-status.test.ts (~50 lines)
    • Replacement: Import fakeChannelStatusAgent and channelStatusEntry from __test-utils__/index.ts
    • Net: -50 lines
    • Safety boundary: Shared helpers must maintain identical behavior
  • PRA-19 yagni (src/lib/actions/sandbox/__test-utils__/index.ts:222): Silent null return in fakePlanFromInputs
    • Replacement: Throw descriptive error when base plan is null/undefined
    • Net: 5 lines
    • Safety boundary: Callers already provide valid base plans
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 — Add targeted integration test for 'channels status --channel telegram' visible config rendering in live sandbox. Runtime/sandbox/infrastructure paths need behavioral runtime validation for docs, check script, test utilities, channel-status, destroy, passthrough-json. New unit tests are comprehensive for visible config logic but integration validation recommended for CLI command behavior.
  • PRA-T2 Runtime validation — Add targeted integration test for 'doctor' visible config checks in live sandbox. Runtime/sandbox/infrastructure paths need behavioral runtime validation for docs, check script, test utilities, channel-status, destroy, passthrough-json. New unit tests are comprehensive for visible config logic but integration validation recommended for CLI command behavior.
  • PRA-T3 Runtime validation — Verify no-test-dist-imports check passes locally with updated fixture logic. Runtime/sandbox/infrastructure paths need behavioral runtime validation for docs, check script, test utilities, channel-status, destroy, passthrough-json. New unit tests are comprehensive for visible config logic but integration validation recommended for CLI command behavior.
  • PRA-T4 Runtime validation — Add registry.test.ts validation for agentApplicability with unsupported agent. Runtime/sandbox/infrastructure paths need behavioral runtime validation for docs, check script, test utilities, channel-status, destroy, passthrough-json. New unit tests are comprehensive for visible config logic but integration validation recommended for CLI command behavior.
  • PRA-T5 Runtime validation — Add workflow-planner.test.ts test for empty string vs undefined at planner boundary. Runtime/sandbox/infrastructure paths need behavioral runtime validation for docs, check script, test utilities, channel-status, destroy, passthrough-json. New unit tests are comprehensive for visible config logic but integration validation recommended for CLI command behavior.
  • PRA-T6 Test monolith not split after helper extraction (carried from previous review) — Split channel-status.test.ts into feature-area test files before merge. Shared helpers in __test-utils__/index.ts and telegram-plan.ts already exist.
  • PRA-T7 Test monolith not split after helper extraction (carried from previous review) — Split doctor-flow.test.ts into feature-area test files before merge. Shared doctor-harness.ts helper already exists.
  • PRA-T8 Test monolith growth exceeds 20-line threshold without validation test expansion — Add registry.test.ts cases: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/actions/sandbox/doctor-messaging.ts:300 / channel-status.ts:555

  • 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: Doctor test exists for legacy entry hiding agent-scoped config; need channel-status test and explicit warning signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: asMessagingAgent(undefined) returns null; inputAppliesToAgent returns false for agent-scoped inputs

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/messaging/diagnostics.ts:198

  • 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: Doctor test covers legacy entry; need registry.test.ts validation for agentApplicability
  • 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: inputAppliesToAgent returns false for null agent when agentApplicability non-empty

PRA-3 Required — Test monolith not split after helper extraction (carried from previous review)

  • Location: src/lib/actions/sandbox/channel-status.test.ts:1
  • Category: tests
  • Problem: channel-status.test.ts remains ~259 lines with 13 test cases covering WhatsApp deep probe, basic per-channel reports, Telegram visibility (partial), and paused channel handling. Previous review explicitly required split into: whatsapp-deep-probe.test.ts, telegram-visibility.test.ts, basic-channel-status.test.ts, paused-channel-status.test.ts. New telegram-visibility tests extracted to separate file but original monolith not split.
  • Impact: Maintenance burden, risk of divergent helper implementations, harder to update test infrastructure. Exceeds 20-line growth threshold from baseline.
  • Required action: Split channel-status.test.ts into feature-area test files before merge. Shared helpers in __test-utils__/index.ts and telegram-plan.ts already exist.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/channel-status.test.ts shows ~259 lines; grep -c 'describe\|it' shows ~13 test cases across 4 feature areas
  • Missing regression test: Structural cleanup verified by file size and grep absence of inline helper definitions
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/channel-status.test.ts shows ~259 lines; grep -c 'describe\|it' shows ~13 test cases across 4 feature areas.
  • Evidence: File imports compileTelegramPlanForTests, fakePlanFromInputs, tamperCompactRegistryTelegramInputs from shared utils but contains all test suites inline

PRA-4 Required — Test monolith not split after helper extraction (carried from previous review)

  • Location: src/lib/actions/sandbox/doctor-flow.test.ts:1
  • Category: tests
  • Problem: doctor-flow.test.ts remains ~272 lines with 12 test cases covering doctor JSON report, --fix rejection, OpenShell unavailable, gateway disconnected, read-only diagnostics, live probes after recovery, no repairs, agent-specific tool-scope, local gateway result, agent definition failures. Previous review explicitly required split into: doctor-core-flow.test.ts, doctor-messaging-visibility.test.ts, doctor-runtime-probes.test.ts, doctor-gateway-inference.test.ts. New messaging-visibility tests extracted but original monolith not split.
  • Impact: Maintenance burden, risk of divergent helper implementations, harder to update test infrastructure. Exceeds 20-line growth threshold from baseline.
  • Required action: Split doctor-flow.test.ts into feature-area test files before merge. Shared doctor-harness.ts helper already exists.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/actions/sandbox/doctor-flow.test.ts shows ~272 lines; grep -c 'describe\|it' shows ~12 test cases across 4 feature areas
  • Missing regression test: Structural cleanup verified by file size and grep absence of inline helper definitions
  • Done when: The required change is committed and verification passes: wc -l src/lib/actions/sandbox/doctor-flow.test.ts shows ~272 lines; grep -c 'describe\|it' shows ~12 test cases across 4 feature areas.
  • Evidence: File uses requireDist to load compiled doctor.js and spies on internal modules; doctor-harness.ts already provides shared harness

PRA-5 Required — Legacy agent fallback lacks migration plan

  • Location: src/lib/actions/sandbox/doctor-messaging.ts:300
  • Category: correctness
  • Problem: Line 300 calls `asMessagingAgent(sb.agent)` where `sb.agent` can be undefined for legacy registry entries. The `asMessagingAgent` function returns null for undefined input, which then causes `inputAppliesToAgent` to return false for agent-scoped visible config inputs (e.g., Telegram group policy hidden on Hermes). This is a silent behavior change for legacy sandboxes with no migration path documented.
  • Impact: Legacy sandboxes without agent field will silently hide agent-applicable visible config instead of showing a warning or migrating. No regression test covers legacy entry handling.
  • Required action: Define concrete migration in this PR: add registry upgrade step that backfills agent field on legacy entries based on imageTag or config paths, then remove fallback. Or make agent field required at registry read time with explicit error for legacy entries. Document chosen path and removal timeline.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check src/lib/actions/sandbox/doctor-messaging.ts:300 and src/lib/messaging/diagnostics.ts:198 for agent field handling
  • Missing regression test: Test in doctor-messaging-visibility.test.ts verifying legacy entry (no agent field) behavior for agent-scoped visible config
  • Done when: The required change is committed and verification passes: Check src/lib/actions/sandbox/doctor-messaging.ts:300 and src/lib/messaging/diagnostics.ts:198 for agent field handling.
  • Evidence: doctor-messaging.ts:300 and channel-status.ts:555 both use asMessagingAgent(sb.agent) without null check or warning

PRA-6 Required — Documentation duplicated across two .mdx files

  • Location: docs/reference/commands.mdx:831
  • Category: docs
  • Problem: The visible config rendering rules note appears identically in both commands.mdx and commands-nemohermes.mdx under 'channels status' and 'doctor' sections. The note explains: 'The Messaging section also surfaces one check per opted-in visible config input so operators can confirm the active policy without inspecting logs. The rendering rules (manifest opt-in via safeToPrintInDiagnostics, agent-scoped hiding, manifest allowlist validation) are shared with channels status and documented under channels status.'
  • Impact: Documentation drift risk — updates to one file won't propagate to the other. Violates single-source-of-truth principle.
  • Required action: Extract shared content to a common include (e.g., docs/reference/_visible-config-rules.mdx) or single-source via cross-reference. Use MDX import or shared snippet.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: diff the 'channels status' and 'doctor' sections in both .mdx files for the visible config note
  • Missing regression test: Documentation lint/check that shared content is not duplicated
  • Done when: The required change is committed and verification passes: diff the 'channels status' and 'doctor' sections in both .mdx files for the visible config note.
  • Evidence: Both docs/reference/commands.mdx and docs/reference/commands-nemohermes.mdx contain identical 4-line note about visible config rendering rules

PRA-7 Resolve/justify — Source-of-truth review needed: Legacy agent fallback

  • Location: src/lib/actions/sandbox/doctor-messaging.ts:300
  • Category: architecture
  • Problem: Legacy agent fallback at doctor-messaging.ts:300 and channel-status.ts:555. The invalid state is legacy SandboxEntry missing agent field. Source boundary is registry read. Source cannot be fixed in this PR because migration requires registry upgrade step. Regression test needed: verify legacy entry behavior. Workaround can be removed when all legacy entries migrated.
  • Impact: Localized workaround behavior without clear removal condition. Silent hiding of agent-scoped config for legacy entries.
  • Recommended action: Document the invalid state, source boundary, source-fix constraint, regression test, and removal condition. Add explicit warning signal when agent detection fails rather than silently returning null.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'asMessagingAgent' src/lib/actions/sandbox/doctor-messaging.ts src/lib/actions/sandbox/channel-status.ts
  • Missing regression test: Test verifying legacy entry (missing agent) produces explicit warning or error signal
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'asMessagingAgent' src/lib/actions/sandbox/doctor-messaging.ts src/lib/actions/sandbox/channel-status.ts.
  • Evidence: asMessagingAgent(undefined) returns null; inputAppliesToAgent returns false for agent-scoped inputs when agent is null

PRA-8 Resolve/justify — Source-of-truth review needed: inputAppliesToAgent null agent handling

  • Location: src/lib/messaging/diagnostics.ts:198
  • Category: architecture
  • Problem: inputAppliesToAgent at diagnostics.ts:198 returns false when agent is null for agent-scoped inputs. The invalid state is plan/compiled entry without agent context. Source boundary is diagnostics collection. Source cannot be fixed without propagating agent context through all callers. Regression test exists in doctor-messaging-visibility.test.ts:59-68 but should be in registry.test.ts. Workaround can be removed when agent field is required at registry read.
  • Impact: Agent-scoped visible config silently hidden when agent context unavailable. Could mask misconfiguration.
  • Recommended action: Add explicit agent context requirement at registry read boundary or emit warning diagnostic when agent-scoped input skipped due to missing agent. Current test only covers doctor path, not registry read path.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'inputAppliesToAgent' src/lib/messaging/diagnostics.ts
  • Missing regression test: Test in registry.test.ts verifying registration throws when agentApplicability contains agent not in supportedAgents (also PRA-12)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'inputAppliesToAgent' src/lib/messaging/diagnostics.ts.
  • Evidence: inputAppliesToAgent returns false for null agent when agentApplicability is non-empty

PRA-9 Resolve/justify — Source-of-truth review needed: fakePlanFromInputs silent null return

  • Location: src/lib/actions/sandbox/test-utils/index.ts:222
  • Category: architecture
  • Problem: fakePlanFromInputs returns null silently for missing base plan. The invalid state is sandbox entry without messaging plan. Source boundary is test utility. Source cannot be fixed because callers control sandbox fixture. Regression test: callers in channel-status-telegram-visibility.test.ts and doctor-messaging-visibility.test.ts already provide valid base plans. Workaround can be removed when callers guaranteed valid plans.
  • Impact: Test helper silently returns null instead of throwing, masking test setup errors.
  • Recommended action: Change to throw descriptive error or return Result type. Callers already provide valid base plans, so safe to tighten.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts
  • Missing regression test: Unit test for fakePlanFromInputs throwing on null base plan
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts.
  • Evidence: Function returns `base && channelInputs ? mergePlanInputs(base, channelInputs) : base` where base can be null

PRA-10 Resolve/justify — Authorization bypass via legacy agent fallback in agent applicability filter

  • Location: src/lib/messaging/diagnostics.ts:198
  • Category: security
  • Problem: When agent field is missing from SandboxEntry, asMessagingAgent returns null, causing inputAppliesToAgent to return false for agent-scoped inputs. This silently hides OpenClaw-only config (Telegram group policy) for legacy entries instead of treating as unknown agent. Fix together with PRA-6. When agent field is missing, either error explicitly or treat as unknown agent that matches no agentApplicability (current behavior) but add a warning signal that agent detection failed.
  • Impact: Legacy sandboxes without agent field silently lose visibility into agent-scoped config. No audit trail that agent detection failed.
  • Recommended action: Add explicit warning diagnostic when agent field missing and agent-scoped inputs exist. Or require agent field at registry read with explicit error.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check doctor-messaging-visibility.test.ts line 59-68 for 'hides agent-applicability-restricted visible config when a legacy SandboxEntry omits the agent field'
  • Missing regression test: Test verifying warning signal emitted when agent field missing and agent-scoped inputs present
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check doctor-messaging-visibility.test.ts line 59-68 for 'hides agent-applicability-restricted visible config when a legacy SandboxEntry omits the agent field'.
  • Evidence: inputAppliesToAgent returns false for null agent; no warning diagnostic emitted

PRA-11 Resolve/justify — Test monolith growth exceeds 20-line threshold without validation test expansion

  • Location: src/lib/messaging/manifest/types.ts:486
  • Category: tests
  • Problem: manifest/types.ts grew by 22 lines. The growth adds visibleConfigInputs fields to ChannelManifest types but validation tests in registry.test.ts only cover valueDisplay key validation and agentApplicability validation. Missing: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
  • Impact: Type expansion without corresponding validation test coverage increase.
  • Recommended action: Add registry.test.ts cases: config input with safeToPrintInDiagnostics but no validValues should be rejected; valueDisplay with key not in validValues should throw; agentApplicability with unsupported agent should throw.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/messaging/manifest/types.ts shows 508 lines (base 486); check registry.test.ts for corresponding validation tests
  • Missing regression test: Three validation test cases in registry.test.ts for the new visibleConfigInputs contract
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/messaging/manifest/types.ts shows 508 lines (base 486); check registry.test.ts for corresponding validation tests.
  • Evidence: manifest/types.ts added visibleConfigInputs to ChannelInputSpec; registry.test.ts validates valueDisplay keys and agentApplicability but not the three missing cases

PRA-12 Resolve/justify — Test monolith growth exceeds 20-line threshold in manifest-compiler.test.ts

  • Location: src/lib/messaging/compiler/manifest-compiler.test.ts:1424
  • Category: tests
  • Problem: File grew by 44 lines (1424→1468). New tests cover Telegram planner boundary and normalizeInputValue. These should be extracted to dedicated test file (e.g., telegram-planner-boundary.test.ts) to offset growth. normalizeInputValue tests could stay as they're compiler-unit focused.
  • Impact: Test file bloat makes maintenance harder. Telegram planner tests are integration-style and belong in separate file.
  • Recommended action: Extract Telegram planner boundary tests to dedicated test file (e.g., telegram-planner-boundary.test.ts) to offset growth.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: wc -l src/lib/messaging/compiler/manifest-compiler.test.ts shows 1468 lines; grep -c 'describe\|it' for telegram planner tests
  • Missing regression test: Extracted test file with telegram planner boundary tests
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: wc -l src/lib/messaging/compiler/manifest-compiler.test.ts shows 1468 lines; grep -c 'describe\|it' for telegram planner tests.
  • Evidence: manifest-compiler.test.ts is a known large-file hotspot; growth not offset by extraction

PRA-13 Resolve/justify — Fixture exclusion self-audit scope change needs verification

  • Location: scripts/checks/no-test-dist-imports.ts:1
  • Category: correctness
  • Problem: The check now distinguishes dynamic-require path construction (allowed in FIXTURE_EXCLUSIONS) from bare import specifiers (not allowed). Self-audit verifies excluded fixtures still construct compiled-internal paths through path.join/require/template. Need to run locally to confirm no excluded fixture fails.
  • Impact: If any excluded fixture now contains bare imports, the check will fail and block CI. The change is correct but needs local verification.
  • Recommended action: Run the check locally: `node scripts/checks/no-test-dist-imports.ts`. If any excluded fixture fails, rewrite to use path construction only or move to test/package-contract/.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: node scripts/checks/no-test-dist-imports.ts
  • Missing regression test: CI run of no-test-dist-imports check passes
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: node scripts/checks/no-test-dist-imports.ts.
  • Evidence: FIXTURE_EXCLUSIONS still contains test/dist-sourcemaps.test.ts, test/install-preflight.test.ts, test/stale-dist-check.test.ts; new logic distinguishes path-construction vs bare imports

PRA-14 Resolve/justify — Missing registry validation test for agentApplicability with unsupported agent

  • Location: src/lib/messaging/diagnostics.ts:70
  • Category: security
  • Problem: registry.test.ts line 70 area has validation for valueDisplay keys and secret safeToPrintInDiagnostics but no test for agentApplicability referencing agent not in supportedAgents. This is a manifest contract validation gap.
  • Impact: Manifest could declare agentApplicability for unsupported agent, causing silent exclusion at runtime without registration-time error.
  • Recommended action: Add test in registry.test.ts verifying registration throws when agentApplicability contains agent not in supportedAgents.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -A20 'agentApplicability' src/lib/messaging/manifest/registry.test.ts
  • Missing regression test: Test case in registry.test.ts: expect(() => createChannelManifestRegistry([manifestWithInvalidAgentApplicability])).toThrow('agentApplicability.*not in supportedAgents')
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -A20 'agentApplicability' src/lib/messaging/manifest/registry.test.ts.
  • Evidence: registry.test.ts validates valueDisplay keys and secret safeToPrintInDiagnostics but not agentApplicability against supportedAgents

PRA-15 Improvement — Defensive bounding for tampered plans — empty string treated as invalid

  • Location: src/lib/messaging/diagnostics.ts:110
  • Category: architecture
  • Problem: resolveVisibleConfigDisplay correctly treats present-but-empty persisted value as invalid rather than falling back to default. This prevents masking corrupted plans. Good defensive behavior.
  • Impact: Positive security posture — tampered empty strings surface as invalid.
  • Suggested action: Keep the defensive bounding. Add test in workflow-planner.test.ts for empty string vs undefined distinction at planner boundary (complements existing test at line 753).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'invalid persisted value' src/lib/messaging/diagnostics.ts
  • Missing regression test: Test in workflow-planner.test.ts for empty string normalization at planner boundary
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: resolveVisibleConfigDisplay returns invalidPersistedDisplay for planInputPresent with empty string value

PRA-16 Improvement — Verify normalizeInputValue empty string handling test runs in CI

  • Location: src/lib/messaging/compiler/manifest-compiler.ts:382
  • Category: tests
  • Problem: The normalizeInputValue tests at manifest-compiler.test.ts:1441-1444 cover empty/whitespace/null/undefined → undefined. Already covered but should confirm CI execution.
  • Impact: Low — tests exist but CI verification needed.
  • Suggested action: Verify existing test at manifest-compiler.test.ts:1441-1444 runs in CI (already covered).
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check CI logs for 'normalizeInputValue' test execution
  • Missing regression test: Already covered by existing tests
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: normalizeInputValue tests exist and cover empty string → undefined

PRA-17 Improvement — Test helper extraction could reduce scaffold in compact registry tests

  • Location: src/lib/actions/sandbox/channel-status.test.ts:729
  • Category: architecture
  • Problem: channel-status.test.ts duplicates fakeAgent and entry helpers that exist in __test-utils__/index.ts. Not required for merge but would improve consistency.
  • Impact: Code duplication in test helpers.
  • Suggested action: Refactor channel-status.test.ts to use shared test utilities from __test-utils__/index.ts. Not required for merge.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Compare fakeAgent/entry in channel-status.test.ts with fakeChannelStatusAgent/channelStatusEntry in __test-utils__/index.ts
  • Missing regression test: None — structural improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: channel-status.test.ts defines its own fakeAgent and entry functions duplicating __test-utils__/index.ts

PRA-18 Improvement — Integration test scaffold uses requireDist + spies for unit-level visible-config tests

  • Location: src/lib/actions/sandbox/doctor-flow.test.ts:213
  • Category: architecture
  • Problem: doctor-flow.test.ts uses requireDist to load compiled doctor.js and spies on internal modules. For pure unit tests of doctor-messaging logic, consider direct imports. Keep requireDist for integration boundary tests.
  • Impact: Test architecture mixes unit and integration styles.
  • Suggested action: For pure unit tests of doctor-messaging logic, consider direct imports. Keep requireDist for integration boundary tests. Not required for merge.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check doctor-flow.test.ts imports and requireDist usage
  • Missing regression test: None — architectural improvement
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: doctor-flow.test.ts uses requireDist and spies on internal modules for visible-config tests

PRA-19 Improvement — fakePlanFromInputs returns null silently for missing base plan

  • Location: src/lib/actions/sandbox/test-utils/index.ts:222
  • Category: architecture
  • Problem: fakePlanFromInputs returns null silently for missing base plan. Change to throw descriptive error or return Result type. Callers in channel-status-telegram-visibility.test.ts and doctor-messaging-visibility.test.ts already provide valid base plans, so safe to tighten.
  • Impact: Test helper masks setup errors by returning null.
  • Suggested action: Change to throw descriptive error or return Result type.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'fakePlanFromInputs' src/lib/actions/sandbox/__test-utils__/index.ts
  • Missing regression test: Unit test for fakePlanFromInputs throwing on null base plan
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Function returns null when base is null/undefined

Workflow run details

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

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

cv and others added 8 commits June 28, 2026 10:03
Cherry-pick of 23a9b6e from origin/codex/fix-onboard-test-src-import (PR #5927) so this branch can rebuild dist without tripping the no-test-dist-imports guard. Refactors the validation test to drive createInferenceSelectionValidationHelpers from src instead of spawning a child node process against dist artifacts.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…status + doctor

Add an isInteractive flag to compileTelegramPlanForTests and drive both command paths through serializeSandboxMessagingStateForDisk + the real getMessagingPlanFromEntry so the visible-config diagnostics now have command-renderer coverage matching the linked onboard-to-registry-to-diagnostics flow.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…elegram-config-visibility

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

# Conflicts:
#	test/package-contract/inference-selection-validation.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…t fixtures

Stricter manifest registration validation, telegram groupPolicy human-readable display, and shared sandbox test utilities address the open advisor findings on PR 5705.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…gn default display

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ❌ Some jobs failed

Run: 28330560251
Target ref: f6979dfd8
Workflow ref: main
Requested jobs: messaging-providers-e2e
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
messaging-providers-e2e ❌ failure

Failed jobs: messaging-providers-e2e. Check run artifacts for logs.

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ❌ Some jobs failed

Run: 28330968837
Target ref: 373fd97175a277e3b6baf91571b79daead1c19db
Workflow ref: main
Requested jobs: messaging-providers-e2e
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
messaging-providers-e2e ❌ failure

Failed jobs: messaging-providers-e2e. Check run artifacts for logs.

… imports

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…e scanner

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…h tests

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
…viders

Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>

@sandl99 sandl99 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.

Reviewed this PR and the issue #5695. Issue #5695 looks like a feature request rather than a Telegram bug.
Currently, there is no channel exposing its configuration via channel status or doctor.

I'd like to reframe the PR and issue into a feature request, exposing config value via channel status, not focusing for Telegram only.
cc: @cv @ericksoa for comments

➜  NemoClaw git:(main) nemoclaw tm channels status                

  NemoClaw channels status: tm / telegram
    [ok] Channel registration: telegram registered
    [ok] Policy coverage: telegram preset applied
    [info] Deep diagnostics: not implemented for telegram; see `nemoclaw tm doctor` and `nemoclaw tm logs --follow`

➜  NemoClaw git:(main) nemoclaw tm channels status --channel teams

  NemoClaw channels status: tm / teams
    [ok] Channel registration: teams registered
    [ok] Policy coverage: teams preset applied
    [info] Deep diagnostics: not implemented for teams; see `nemoclaw tm doctor` and `nemoclaw tm logs --follow`

@sandl99

sandl99 commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Superseded by #6033, which implements the manifest-backed channel config status summary across configured channels and fixes #5695.

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

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

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

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

## Type of Change

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

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

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

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

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


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

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

---------

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

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

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

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

## Type of Change

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

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

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

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

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


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

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

---------

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

Labels

bug-fix PR fixes a bug or regression integration: telegram Telegram integration or channel behavior NV QA Bugs found by the NVIDIA QA Team VDR Linked to VDR finding

Projects

None yet

5 participants