Skip to content

feat(messaging): add Google Chat channel for OpenClaw#6120

Draft
hunglp6d wants to merge 75 commits into
mainfrom
feat/messaging-channel-googlechat
Draft

feat(messaging): add Google Chat channel for OpenClaw#6120
hunglp6d wants to merge 75 commits into
mainfrom
feat/messaging-channel-googlechat

Conversation

@hunglp6d

@hunglp6d hunglp6d commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds Google Chat as a first-class OpenClaw messaging channel, served on the existing gateway webhook via the cloudflared tunnel. Outbound auth is minted gateway-side (OpenShell google-service-account-jwt provider) and injected by the L7 proxy, so the service-account private key never enters the sandbox. Inbound cert verification and outbound replies both route by hostname through the trusted L7 proxy (the SSRF guard's trusted-proxy modes), so the channel works in the DNS-less proxy-only sandbox — no local DNS resolve, no sentinel IP.

Scope: OpenClaw only. Hermes uses a different inbound model (Cloud Pub/Sub pull, no webhook) and a different credential path, so Hermes Google Chat is a planned follow-up rather than part of this PR.

Merge Prerequisite

PR #6726 must land first. It upgrades NemoClaw to OpenShell 0.0.82, whose provider refresh secret-material environment support keeps the Google Chat service-account private key off process arguments. Current main pins OpenShell 0.0.72 and cannot safely configure the bridge.

Related Issue

Parts of #5492

Result

image

Changes

Channel

  • New googlechat manifest (inputs, enrollment notes, render) + built-in registration; served on the shared gateway dashboard port at /googlechat (no host forward — reuses the tunnel).
  • Enroll hooks: tunnel/audience gate (derives the public webhook URL from the cloudflared tunnel), token-paste for the SA JSON, config prompts (audience, appPrincipal, DM allowlist); channel template resolver + nemoclaw tunnel service wiring.
  • Always skips in non-interactive mode (mirrors WeChat host-QR): enrollment needs manual, out-of-band Google Cloud Console + appPrincipal steps that no environment variable can satisfy, so the tunnel/audience gate skips rather than enroll a half-configured channel that silently 404s on inbound webhooks.
  • appPrincipal discovery sentinel: a blank appPrincipal renders an all-zeros placeholder so the first DM logs unexpected add-on principal: <N> (the real value to copy) instead of a numberless missing add-on principal binding; inert for Google Workspace accounts. Enrollment notes are rewritten into an always-printed capture guide with the exact logs --follow command and the persist-and-rebuild steps.

Gateway reliability

  • Renders gateway.reload.mode=off. OpenClaw rewrites its own config ~60s after boot (auto-enabling default provider plugins); with reactive hot-reload on, that self-write rebuilds the gateway's HTTP route table and drops the Google Chat inbound webhook route, so /googlechat starts returning 404 and the bot goes silent ~60s after every start. The sandbox's openclaw.json is sealed at build time (0600 + integrity hash), so disabling reactive reload is safe; NemoClaw still restarts the gateway explicitly on rebuild / gateway restart.

Inbound + outbound routing (proxy-only sandbox)

  • googlechat-trusted-proxy-fetch boot preload: rewrites the plugin's three googleapis fetch sites (inbound cert verify + all outbound sends/edits) to the SSRF guard's trusted_env_proxy/trusted_explicit_proxy modes, so they skip the local getaddrinfo and route by hostname through the L7 proxy — fixes EAI_AGAIN in the DNS-less netns, no sentinel IP. Interim; clean fix is upstream in OpenClaw (like web_fetch #50650).
  • googlechat network-policy preset scoped to the Chat spaces tree, including PATCH for streaming message edits.

Outbound auth (key out of sandbox)

  • google-chat-bridge OpenShell provider profile + googlechat-bridge-provider onboard wiring: import the profile pre-create, run provider refresh configure --strategy google-service-account-jwt post-create from the pasted SA JSON — the gateway mints/rotates the token and the L7 proxy injects Authorization: Bearer on chat.googleapis.com.
  • googlechat-outbound-auth boot preload: load-time rewrite of the plugin's single token producer to emit the OpenShell credential placeholder (revision-less alias, so it resolves to the latest re-minted token and survives rotation) instead of signing in-process.
  • Drops in-sandbox SA-key file delivery for outbound; serviceAccountFile is kept only as a channel-start-gate marker (never delivered or read); keeps the BEGIN PRIVATE KEY secret scan.

Documentation

  • Adds OpenClaw-only setup, onboarding, lifecycle, and security guidance. The docs build completed with zero errors; two pre-existing warnings remain (redirect authentication and light-accent contrast).

Type of Change

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

Quality Gates

  • Tests added or updated for changed behavior — unit tests for the two boot preloads (anchor rewrite, idempotency, drift-throw, runtime short-circuit), the bridge provider (env resolution, fail-closed refresh), the tunnel/audience gate (unconditional non-interactive skip, audience does not bypass), and the template resolver (appPrincipal discovery sentinel when blank)
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — nine-category review
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

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

Signed-off-by: Hung Le hple@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Added experimental Google Chat as a built-in inbound-webhook channel under the shared gateway path.
    • Included interactive enrollment with token-paste service-account auth, optional audience/webhook-path controls, and DM allowlist setup.
    • Added a tunnel audience gate that can derive audience from a public tunnel URL with operator confirmation.
    • Added Google Chat config template reference resolution, including allowlist normalization.
  • Bug Fixes
    • Strengthened sandbox safety to prevent accidental service-account key exposure and added guards to refuse unsafe configs.
  • Documentation
    • Added and updated setup and channel-management docs for Google Chat.
  • Tests
    • Added/expanded coverage for the Google Chat manifest, hook behavior, and template resolver semantics.

@copy-pr-bot

copy-pr-bot Bot commented Jul 1, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7ea98351-308f-4b9b-86f3-9fc8bc7eb68d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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 an experimental Google Chat channel for OpenClaw with manifest-driven enrollment, service-account token handling, public webhook audience setup, runtime safeguards, template resolution, documentation, and comprehensive tests.

Changes

Google Chat channel manifest

Layer / File(s) Summary
Manifest contract and inputs
src/lib/messaging/channels/googlechat/manifest.ts
Declares Google Chat metadata, enrollment inputs, policy configuration, and the OpenClaw package pin.
Render wiring and runtime safeguards
src/lib/messaging/channels/googlechat/manifest.ts
Maps settings into OpenClaw configuration, adds runtime preloads and secret scanning, and wires enrollment hooks.
Template reference resolver
src/lib/messaging/channels/googlechat/template-resolver.ts, src/lib/messaging/channels/googlechat/template-resolver.test.ts
Resolves configuration references with defaults, sentinels, and DM allowlist normalization, with test coverage.
Tunnel audience gate implementation
src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts, src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.test.ts
Derives or accepts webhook audiences, manages cloudflared tunnel lifecycle, requires interactive confirmation, and tests these flows.
Registry and manifest validation
src/lib/messaging/channels/manifests.test.ts
Adds registry, import-guard, rendered-config, enrollment, runtime, and package assertions.
Setup and lifecycle documentation
docs/index.yml, docs/manage-sandboxes/*.mdx
Documents setup, interactive enrollment, endpoint requirements, access configuration, credential rotation, removal, pause, and resume behavior.
Package review allowlist
ci/reviewed-npm-lifecycle-allowlist.json
Adds the reviewed Google Chat package entry.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Operator
  participant Enrollment
  participant Tunnel
  participant GoogleCloud
  participant OpenClaw
  Operator->>Enrollment: Add googlechat interactively
  Enrollment->>Tunnel: Start or reuse public tunnel
  Tunnel-->>Enrollment: Return webhook URL
  Enrollment->>GoogleCloud: Confirm endpoint configuration
  Operator->>Enrollment: Paste service-account JSON
  Enrollment->>OpenClaw: Render channel and runtime configuration
Loading

Possibly related PRs

  • NVIDIA/NemoClaw#5777: Adds manifest-driven supported-agent and channel-support gating that this Google Chat manifest integrates with.

Suggested labels: feature, area: messaging, area: integrations, area: docs

Suggested reviewers: cv, ericksoa

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: adding the Google Chat channel for OpenClaw.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/messaging-channel-googlechat

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

@github-code-quality

github-code-quality Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage remains at 96%, unchanged from the branch.

TypeScript / code-coverage/cli

The overall coverage in the branch remains at 79%, unchanged from the branch.

Show a code coverage summary of the most impacted files.
File 33d55b0 439331d +/-
src/lib/sandbox...vileged-exec.ts 82% 71% -11%
src/lib/policy/index.ts 65% 58% -7%
src/lib/messagi...nnel-runtime.ts 0% 61% +61%
src/lib/messagi...-proxy-fetch.ts 0% 72% +72%
src/lib/messagi...utbound-auth.ts 0% 76% +76%
src/lib/messagi...ate-resolver.ts 0% 80% +80%
src/lib/tunnel/...ok-lifecycle.ts 0% 86% +86%
src/lib/messagi...udience-gate.ts 0% 89% +89%
src/lib/tunnel/...ebhook-proxy.ts 0% 91% +91%
src/lib/onboard...dge-provider.ts 0% 99% +99%

Updated July 14, 2026 13:16 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-4: 455-line monolith for generic bridge provider wiring; then add or justify PRA-T1.
Open items: 7 required · 11 warnings · 5 suggestions · 8 test follow-ups
Since last review: 0 prior items resolved · 24 still apply · 0 new items found

Action checklist

  • PRA-4 Fix: 455-line monolith for generic bridge provider wiring in src/lib/onboard/messaging-bridge-provider.ts:1
  • PRA-5 Fix: Test monolith grew to 1041 lines with Google Chat tests in src/lib/messaging/channels/manifests.test.ts:1
  • PRA-6 Fix: providers.ts monolith grew to 556 lines with bridge orchestration in src/lib/onboard/providers.ts:1
  • PRA-7 Fix: tunnel/services.ts monolith grew to 771 lines with lazy-require cycle in src/lib/tunnel/services.ts:640
  • PRA-8 Fix: Non-interactive mode unconditionally skips Google Chat even with pre-supplied audience in src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70
  • PRA-9 Fix: Missing boot-time health check for googlechat-outbound-auth patch in src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:60
  • PRA-10 Fix: Missing boot-time health check for googlechat-trusted-proxy-fetch patch in src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:85
  • PRA-1 Resolve or justify: Source-of-truth review needed: googlechat-outbound-auth.ts (outbound auth rewrite preload)
  • PRA-2 Resolve or justify: Source-of-truth review needed: googlechat-trusted-proxy-fetch.ts (trusted proxy fetch rewrite preload)
  • PRA-3 Resolve or justify: Source-of-truth review needed: messaging-bridge-provider.ts:268 (argv secret transit)
  • PRA-11 Resolve or justify: Service account private key passes through argv to provider refresh configure in src/lib/onboard/messaging-bridge-provider.ts:268
  • PRA-12 Resolve or justify: @ts-nocheck on security-critical auth rewrite preload in src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:1
  • PRA-13 Resolve or justify: @ts-nocheck on security-critical SSRF guard rewrite preload in src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:1
  • PRA-14 Resolve or justify: Synthetic serviceAccountFile sentinel for channel start gate in src/lib/messaging/channels/googlechat/manifest.ts:115
  • PRA-15 Resolve or justify: Two nodePreloads target same bundle with independent Module._extensions overrides in src/lib/messaging/channels/googlechat/manifest.ts:200
  • PRA-16 Resolve or justify: Lazy-require cycle: tunnel-runtime.ts duplicates resolveServicePidDir from services.ts in src/lib/messaging/channels/googlechat/hooks/tunnel-runtime.ts:22
  • PRA-17 Resolve or justify: AppPrincipal discovery sentinel collision assumption unverified in src/lib/messaging/channels/googlechat/template-resolver.ts:20
  • 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: No integration flow test for bridge provider lifecycle with fake OpenShell gateway
  • PRA-T7 Add or justify test follow-up: Missing health-check hook entirely for Google Chat
  • PRA-T8 Add or justify test follow-up: Non-interactive workaround path test missing
  • PRA-19 In-scope improvement: Per-channel rendered-config-parser boilerplate could be unified in src/lib/messaging/channels/googlechat/rendered-config-parser.ts:1
  • PRA-20 In-scope improvement: No integration flow test for bridge provider lifecycle with fake OpenShell gateway in src/lib/onboard/messaging-bridge-provider.test.ts:1
  • PRA-21 In-scope improvement: Missing health-check hook entirely for Google Chat in src/lib/messaging/channels/googlechat/manifest.ts:1
  • PRA-22 In-scope improvement: Non-interactive workaround path test missing in src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70
  • PRA-23 In-scope improvement: DEFAULT_WEBHOOK_PATH constant used only once in src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:15

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Required correctness src/lib/onboard/messaging-bridge-provider.ts:1 Extract into focused modules: list-bridge-profiles.ts, collect-bridge-token-defs.ts, ensure-bridge-profiles.ts, configure-bridge-refreshes.ts, bridge-secret-resolution.ts, plus a thin barrel export. Each <150 lines.
PRA-5 Required correctness src/lib/messaging/channels/manifests.test.ts:1 Split into per-channel test files (e.g., googlechat.manifest.test.ts, teams.manifest.test.ts) or by concern (hooks, render, runtime, packages). Target <500 lines per file.
PRA-6 Required correctness src/lib/onboard/providers.ts:1 Extract bridge provider orchestration into messaging-bridge-upsert.ts that upsertMessagingProviders delegates to. Keep providers.ts focused on inference/provider selection.
PRA-7 Required correctness src/lib/tunnel/services.ts:640 Extract PID directory resolution into tunnel/pid-dir.ts used by both services.ts and tunnel-runtime.ts. Consider splitting services.ts into cloudflared-lifecycle.ts, gateway-stop.ts, pid-dir.ts.
PRA-8 Required correctness src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70 Document in onboarding docs: Google Chat requires interactive enrollment (Cloud Console endpoint URL + appPrincipal). If automation needed later, hook must be updated to support pre-captured appPrincipal + audience.
PRA-9 Required security src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:60 Add boot-time health check hook for googlechat that verifies process.__nemoclawGooglechatOutboundAuthInstalled and patch applied. Fail gateway startup if patch missing and channel active.
PRA-10 Required security src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:85 Add boot-time health check hook verifying process.__nemoclawGooglechatTrustedProxyFetchInstalled and all three anchors patched. Fail gateway startup if patch missing and channel active.
PRA-11 Resolve/justify security src/lib/onboard/messaging-bridge-provider.ts:268 Add TODO comment with upstream OpenShell issue reference for --secret-material-file/stdin transport. Document that key never enters sandbox (key-out-of-sandbox boundary holds).
PRA-12 Resolve/justify security src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:1 Migrate to ESM with proper types, or add JSDoc @typedef for Module._extensions and Module.registerHooks shapes. File follow-up to remove @ts-nocheck.
PRA-13 Resolve/justify security src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:1 Same as outbound-auth: migrate to typed ESM or add JSDoc types for Module hooks. File follow-up to remove @ts-nocheck.
PRA-14 Resolve/justify security src/lib/messaging/channels/googlechat/manifest.ts:115 Keep workaround analysis block current (lines 143-158). When upstream adds native accessToken auth mode, remove sentinel and gateway-reload-off fragment. Add test that sentinel path never accessed.
PRA-15 Resolve/justify architecture src/lib/messaging/channels/googlechat/manifest.ts:200 Document preload order independence. Consider shared loader-hook installer utility to avoid duplicate Module._extensions wrapping.
PRA-16 Resolve/justify architecture src/lib/messaging/channels/googlechat/hooks/tunnel-runtime.ts:22 Extract pure resolveServicePidDir and validateSandboxName to tunnel/pid-dir.ts (shared module) so both services.ts and tunnel-runtime.ts import eagerly without cycle.
PRA-17 Resolve/justify correctness src/lib/messaging/channels/googlechat/template-resolver.ts:20 Add test or doc reference confirming Google add-on principal is 21-digit non-zero numeric ID. If confirmed, note in test comment.
PRA-18 Resolve/justify architecture src/lib/messaging/channels/googlechat/manifest.ts:98 Keep workaround analysis block current. Remove when upstream reload re-mounts channels (not just plugins) on config reload.
PRA-19 Improvement architecture src/lib/messaging/channels/googlechat/rendered-config-parser.ts:1 Evaluate generic RenderedChannelConfigParser factory that takes manifest and agentId, auto-discovers config inputs with statePath. If not feasible, document why Google Chat needs custom parser.
PRA-20 Improvement tests src/lib/onboard/messaging-bridge-provider.test.ts:1 Add integration test using fake OpenShell gateway (or in-process gateway mock) that verifies: profile imported, provider created with sentinel, refresh configured with secret material, token minted and rotated.

🚨 Required before merge

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

PRA-4 Required — 455-line monolith for generic bridge provider wiring

  • Location: src/lib/onboard/messaging-bridge-provider.ts:1
  • Category: correctness
  • Problem: Single file handles profile discovery, token def collection, profile import, refresh configuration, and secret resolution mixed together. Exceeds reviewable size and mixes multiple responsibilities.
  • Impact: Hard to review security boundaries; changes to one concern risk breaking another; no clear module boundaries for testing or future channels.
  • Required action: Extract into focused modules: list-bridge-profiles.ts, collect-bridge-token-defs.ts, ensure-bridge-profiles.ts, configure-bridge-refreshes.ts, bridge-secret-resolution.ts, plus a thin barrel export. Each <150 lines.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -r 'export.*MessagingBridge' src/lib/onboard/ --include="*.ts" | wc -l should show multiple focused files after extraction
  • Missing regression test: Each extracted module should have its own test file mirroring current messaging-bridge-provider.test.ts coverage
  • Done when: The required change is committed and verification passes: grep -r 'export.*MessagingBridge' src/lib/onboard/ --include="*.ts" | wc -l should show multiple focused files after extraction.
  • Evidence: File is 455 lines with 5 exported functions handling distinct phases; prior review PRA-2

PRA-5 Required — Test monolith grew to 1041 lines with Google Chat tests

  • Location: src/lib/messaging/channels/manifests.test.ts:1
  • Category: correctness
  • Problem: manifests.test.ts now contains all channel manifest tests including 163 lines of Google Chat specific tests.
  • Impact: Test file too large for effective review; changes to one channel's tests risk breaking others; slows test iteration.
  • Required action: Split into per-channel test files (e.g., googlechat.manifest.test.ts, teams.manifest.test.ts) or by concern (hooks, render, runtime, packages). Target <500 lines per file.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/messaging/channels/*.test.ts should show no file >500 lines
  • Missing regression test: Each split test file must preserve all current test coverage for its channel
  • Done when: The required change is committed and verification passes: wc -l src/lib/messaging/channels/*.test.ts should show no file >500 lines.
  • Evidence: File grew from 918 to 1041 lines (+123) with Google Chat additions; prior review PRA-3

PRA-6 Required — providers.ts monolith grew to 556 lines with bridge orchestration

  • Location: src/lib/onboard/providers.ts:1
  • Category: correctness
  • Problem: providers.ts mixes inference provider selection, hosted inference staging, gateway provider CRUD, and now bridge provider orchestration via upsertMessagingProviders.
  • Impact: Bridge provider logic coupled to inference provider logic; changes to one domain risk the other; exceeds single-responsibility.
  • Required action: Extract bridge provider orchestration into messaging-bridge-upsert.ts that upsertMessagingProviders delegates to. Keep providers.ts focused on inference/provider selection.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -n 'upsertMessagingProviders' src/lib/onboard/providers.ts should show only a thin delegation call after extraction
  • Missing regression test: New messaging-bridge-upsert.test.ts covering the delegation and error paths
  • Done when: The required change is committed and verification passes: grep -n 'upsertMessagingProviders' src/lib/onboard/providers.ts should show only a thin delegation call after extraction.
  • Evidence: File grew from 507 to 556 lines (+49) with bridge wiring; prior review PRA-4

PRA-7 Required — tunnel/services.ts monolith grew to 771 lines with lazy-require cycle

  • Location: src/lib/tunnel/services.ts:640
  • Category: correctness
  • Problem: services.ts handles cloudflared lifecycle, gateway stop, PID dir resolution. tunnel-runtime.ts lazy-requires services.ts creating import cycle risk; resolveServicePidDir duplicated.
  • Impact: Import cycle risk; duplicated PID dir resolution; hard to test PID logic in isolation.
  • Required action: Extract PID directory resolution into tunnel/pid-dir.ts used by both services.ts and tunnel-runtime.ts. Consider splitting services.ts into cloudflared-lifecycle.ts, gateway-stop.ts, pid-dir.ts.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts imported by both
  • Missing regression test: tunnel/pid-dir.test.ts covering resolution logic with various env/sandbox combinations
  • Done when: The required change is committed and verification passes: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts imported by both.
  • Evidence: File grew from 749 to 771 lines (+22); tunnel-runtime.ts:22-30 duplicates resolveServicePidDir via lazy require; prior review PRA-5, PRA-17

PRA-8 Required — Non-interactive mode unconditionally skips Google Chat even with pre-supplied audience

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70
  • Category: correctness
  • Problem: Hook throws at line 50-62 when context.isInteractive===false, even if GOOGLECHAT_AUDIENCE is pre-configured. Documented as intentional (Cloud Console + appPrincipal steps need human) but not in onboarding docs.
  • Impact: Automation/CI cannot enable Google Chat non-interactively even with all config pre-supplied; operators surprised by skip.
  • Required action: Document in onboarding docs: Google Chat requires interactive enrollment (Cloud Console endpoint URL + appPrincipal). If automation needed later, hook must be updated to support pre-captured appPrincipal + audience.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check docs/onboarding.md or similar for 'Google Chat' and 'non-interactive' sections
  • Missing regression test: Doc update verified; or if automation path added later, test that pre-supplied audience + appPrincipal bypasses skip
  • Done when: The required change is committed and verification passes: Check docs/onboarding.md or similar for 'Google Chat' and 'non-interactive' sections.
  • Evidence: tunnel-audience-gate.ts:50-62 throws unconditionally; test at tunnel-audience-gate.test.ts:55-66 confirms behavior; prior review PRA-6

PRA-9 Required — Missing boot-time health check for googlechat-outbound-auth patch

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:60
  • Category: security
  • Problem: Patch drift logs but continues gateway boot; fail-closed only at send time. Channel can start with unpatched plugin and fail only on first outbound reply.
  • Impact: Silent degradation: bot receives messages but cannot reply until first send attempt; operator unaware until user reports broken replies.
  • Required action: Add boot-time health check hook for googlechat that verifies process.__nemoclawGooglechatOutboundAuthInstalled and patch applied. Fail gateway startup if patch missing and channel active.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check manifest.ts hooks array for a health-check phase hook that validates the outbound-auth sentinel
  • Missing regression test: Integration test: start gateway with googlechat channel active but outbound-auth preload disabled; verify gateway fails to start
  • Done when: The required change is committed and verification passes: Check manifest.ts hooks array for a health-check phase hook that validates the outbound-auth sentinel.
  • Evidence: googlechat-outbound-auth.ts:225-236 catches drift, logs, continues; prior review PRA-14, PRA-T7, PRA-22

PRA-10 Required — Missing boot-time health check for googlechat-trusted-proxy-fetch patch

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:85
  • Category: security
  • Problem: Patch drift logs but continues gateway boot; fail-closed only at fetch time (cert verify or outbound send). Channel can start with unpatched plugin and fail on first inbound cert verify or outbound call.
  • Impact: Silent degradation: bot starts but inbound verification fails (401 on webhook) or outbound fails; operator unaware until first message.
  • Required action: Add boot-time health check hook verifying process.__nemoclawGooglechatTrustedProxyFetchInstalled and all three anchors patched. Fail gateway startup if patch missing and channel active.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check manifest.ts hooks array for a health-check phase hook that validates the trusted-proxy-fetch sentinel
  • Missing regression test: Integration test: start gateway with googlechat channel active but trusted-proxy-fetch preload disabled; verify gateway fails to start
  • Done when: The required change is committed and verification passes: Check manifest.ts hooks array for a health-check phase hook that validates the trusted-proxy-fetch sentinel.
  • Evidence: googlechat-trusted-proxy-fetch.ts:240-250 catches drift, logs, continues; prior review PRA-15, PRA-T8, PRA-23
Review findings by urgency: 7 required fixes, 11 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: googlechat-outbound-auth.ts (outbound auth rewrite preload)

  • 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: googlechat-outbound-auth.test.ts: anchor matching, drift throw, idempotency, short-circuit throws when env unset
  • 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: Workaround Analysis block at lines 10-55; prior review PRA-7

PRA-2 Resolve/justify — Source-of-truth review needed: googlechat-trusted-proxy-fetch.ts (trusted proxy fetch rewrite preload)

  • 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: googlechat-trusted-proxy-fetch.test.ts: anchor matching (3 sites), drift throw, idempotency, site mapping
  • 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: Workaround Analysis block at lines 11-55; prior review PRA-8

PRA-3 Resolve/justify — Source-of-truth review needed: messaging-bridge-provider.ts:268 (argv secret transit)

  • 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: messaging-bridge-provider.test.ts: configureMessagingBridgeRefreshes success/fail-closed tests
  • 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: SECURITY comment at lines 243-274; prior review PRA-11, PRA-12

PRA-11 Resolve/justify — Service account private key passes through argv to provider refresh configure

  • Location: src/lib/onboard/messaging-bridge-provider.ts:268
  • Category: security
  • Problem: OpenShell `provider refresh configure` ingests refresh material only via `--material KEY=VALUE` argv — no stdin, file, or env-ref transport. Private key transits host-local argv (ps/procfs visible).
  • Impact: Host-local secret exposure on trusted host that already holds the key; transient (one configure call); key never enters sandbox. Tracked upstream for --secret-material-file/stdin.
  • Recommended action: Add TODO comment with upstream OpenShell issue reference for --secret-material-file/stdin transport. Document that key never enters sandbox (key-out-of-sandbox boundary holds).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'TODO.*secret-material-file' src/lib/onboard/messaging-bridge-provider.ts should show tracking issue reference
  • Missing regression test: None needed for current behavior; when upstream adds file/stdin transport, test that argv path is no longer used
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'TODO.*secret-material-file' src/lib/onboard/messaging-bridge-provider.ts should show tracking issue reference.
  • Evidence: messaging-bridge-provider.ts:243-274 comment documents risk; prior review PRA-11, PRA-12

PRA-12 Resolve/justify — @ts-nocheck on security-critical auth rewrite preload

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:1
  • Category: security
  • Problem: File uses @ts-nocheck because it manipulates Module._extensions and Module.registerHooks with untyped shapes. Type safety gaps in security-critical patch logic.
  • Impact: Type errors in patch logic (anchor regex, short-circuit source) would only surface at runtime in gateway boot.
  • Recommended action: Migrate to ESM with proper types, or add JSDoc @typedef for Module._extensions and Module.registerHooks shapes. File follow-up to remove @ts-nocheck.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode
  • Missing regression test: TypeScript compile check in CI for this file
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode.
  • Evidence: First line of googlechat-outbound-auth.ts is @ts-nocheck; prior review PRA-9

PRA-13 Resolve/justify — @ts-nocheck on security-critical SSRF guard rewrite preload

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:1
  • Category: security
  • Problem: File uses @ts-nocheck for same Module hook manipulation reasons as outbound-auth. Type safety gaps in SSRF guard rewrite.
  • Impact: Type errors in anchor regexes (ANCHOR_A/B/C) or replacement logic would only surface at runtime.
  • Recommended action: Same as outbound-auth: migrate to typed ESM or add JSDoc types for Module hooks. File follow-up to remove @ts-nocheck.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode
  • Missing regression test: TypeScript compile check in CI for this file
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode.
  • Evidence: First line of googlechat-trusted-proxy-fetch.ts is @ts-nocheck; prior review PRA-10

PRA-14 Resolve/justify — Synthetic serviceAccountFile sentinel for channel start gate

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:115
  • Category: security
  • Problem: Renders non-existent path `/nonexistent/googlechat-gateway-minted-no-service-account-file` as serviceAccountFile to satisfy OpenClaw start gate (requires non-empty serviceAccount*). File never delivered or read; outbound-auth preload short-circuits before read.
  • Impact: Workaround for upstream missing 'configured'/accessToken credential source. If preload fails to load, sentinel path could be read (but secret scan would catch private key leakage).
  • Recommended action: Keep workaround analysis block current (lines 143-158). When upstream adds native accessToken auth mode, remove sentinel and gateway-reload-off fragment. Add test that sentinel path never accessed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check manifest.ts workaround analysis block lines 143-158 for removal condition tracking
  • Missing regression test: Test that verifies serviceAccountFile path is never read at runtime (e.g., fs.readFile spy on sentinel path never called)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check manifest.ts workaround analysis block lines 143-158 for removal condition tracking.
  • Evidence: manifest.ts:98-105 sentinel with comment; workaround analysis at lines 115-130; prior review PRA-13

PRA-15 Resolve/justify — Two nodePreloads target same bundle with independent Module._extensions overrides

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:200
  • Category: architecture
  • Problem: Both googlechat-trusted-proxy-fetch and googlechat-outbound-auth preloads wrap Module._extensions['.js'] and Module.registerHooks independently, targeting the same @openclaw/googlechat dist bundle.
  • Impact: Duplicate loader wrapping; potential order-dependent behavior; harder to reason about patch composition.
  • Recommended action: Document preload order independence. Consider shared loader-hook installer utility to avoid duplicate Module._extensions wrapping.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if both preloads install hooks in same IIFE or if order matters; look for shared utility in src/lib/messaging/channels/googlechat/runtime/
  • Missing regression test: Test that both preloads can be loaded in either order and both patches apply correctly
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if both preloads install hooks in same IIFE or if order matters; look for shared utility in src/lib/messaging/channels/googlechat/runtime/.
  • Evidence: manifest.ts:200-218 two nodePreloads entries; prior review PRA-19

PRA-16 Resolve/justify — Lazy-require cycle: tunnel-runtime.ts duplicates resolveServicePidDir from services.ts

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-runtime.ts:22
  • Category: architecture
  • Problem: tunnel-runtime.ts lazy-requires tunnel/services to access resolveServicePidDir and readCloudflaredState, creating import cycle risk and duplicating PID dir logic.
  • Impact: Cycle risk if services.ts ever imports tunnel-runtime; duplicated logic; harder to test PID resolution in isolation.
  • Recommended action: Extract pure resolveServicePidDir and validateSandboxName to tunnel/pid-dir.ts (shared module) so both services.ts and tunnel-runtime.ts import eagerly without cycle.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts
  • Missing regression test: tunnel/pid-dir.test.ts covering resolution logic; both services.ts and tunnel-runtime.ts tests import from shared module
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts.
  • Evidence: tunnel-runtime.ts:22-30 lazy-requires services.ts; prior review PRA-17

PRA-17 Resolve/justify — AppPrincipal discovery sentinel collision assumption unverified

  • Location: src/lib/messaging/channels/googlechat/template-resolver.ts:20
  • Category: correctness
  • Problem: All-zeros sentinel `000000000000000000000` assumed to never collide with real Google-assigned add-on principal (~21 digits). No test or doc reference confirming Google assigns non-zero numeric IDs.
  • Impact: If Google ever assigns all-zeros principal, discovery log would not surface real value; operator would see 'missing add-on principal binding' instead of the number.
  • Recommended action: Add test or doc reference confirming Google add-on principal is 21-digit non-zero numeric ID. If confirmed, note in test comment.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check template-resolver.test.ts for comment referencing Google documentation or test confirming non-zero assignment
  • Missing regression test: Test comment or doc link verifying Google principal format; or test that sentinel != any known real principal format
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check template-resolver.test.ts for comment referencing Google documentation or test confirming non-zero assignment.
  • Evidence: template-resolver.ts:19-22 sentinel with comment; test uses 21-digit value but no source reference; prior review PRA-16, PRA-25

PRA-18 Resolve/justify — Gateway reload off fragment is a localized workaround

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:98
  • Category: architecture
  • Problem: Renders gateway.reload.mode=off to prevent OpenClaw's ~60s post-boot self-write from dropping webhook route. Workaround analysis block present (lines 143-158).
  • Impact: Disables reactive config reload entirely for sandbox; NemoClaw still restarts gateway explicitly on rebuild/restart. Low risk but upstream fix preferred.
  • Recommended action: Keep workaround analysis block current. Remove when upstream reload re-mounts channels (not just plugins) on config reload.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check manifest.ts lines 143-158 for workaround analysis with removal condition
  • Missing regression test: Test that gateway reload mode is 'off' in rendered openclaw.json when googlechat channel active
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check manifest.ts lines 143-158 for workaround analysis with removal condition.
  • Evidence: manifest.ts:115-130 render fragment + workaround analysis lines 143-158; prior review PRA-18

💡 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-19 Improvement — Per-channel rendered-config-parser boilerplate could be unified

  • Location: src/lib/messaging/channels/googlechat/rendered-config-parser.ts:1
  • Category: architecture
  • Problem: Google Chat has custom RenderedChannelConfigParser (41 lines) that mirrors pattern of other channels but with channel-specific keys.
  • Impact: Boilerplate duplication across channels; new channels must copy-paste parser structure.
  • Suggested action: Evaluate generic RenderedChannelConfigParser factory that takes manifest and agentId, auto-discovers config inputs with statePath. If not feasible, document why Google Chat needs custom parser.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if other channels (slack, teams, etc.) have similar parser files with same structure
  • Missing regression test: If factory created, test that it produces correct visibility keys for all existing channels
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: rendered-config-parser.ts:41 lines; prior review PRA-20

PRA-20 Improvement — No integration flow test for bridge provider lifecycle with fake OpenShell gateway

  • Location: src/lib/onboard/messaging-bridge-provider.test.ts:1
  • Category: tests
  • Problem: Unit tests mock runOpenshell; no test exercises full bridge provider lifecycle (profile import → provider create → refresh configure) against a fake OpenShell gateway.
  • Impact: Integration gaps between bridge wiring and gateway behavior undetected; argv secret transit, refresh timing, provider detachment not validated end-to-end.
  • Suggested action: Add integration test using fake OpenShell gateway (or in-process gateway mock) that verifies: profile imported, provider created with sentinel, refresh configured with secret material, token minted and rotated.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Look for test file using fake gateway or in-process openshell mock for bridge provider flow
  • Missing regression test: Integration test: googlechat-bridge-provider-lifecycle-with-fake-openshell-gateway
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: messaging-bridge-provider.test.ts only unit tests with mocked runOpenshell; prior review PRA-T6, PRA-T21, PRA-T27

PRA-21 Improvement — Missing health-check hook entirely for Google Chat

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:1
  • Category: tests
  • Problem: Manifest declares no health-check phase hooks. Other channels (Slack, Teams, WeChat) have openclaw-bridge-health hooks. Google Chat needs health checks for both runtime preloads.
  • Impact: No automated verification that channel is healthy after rebuild/start; operator must manually check logs.
  • Suggested action: Add health-check hook(s) that verify both preloads are installed and patches applied. Can reuse openclawBridgeHealth helper from hook registry.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check manifest.ts hooks array for phase: 'health-check' entries
  • Missing regression test: Health check test verifying both preload sentinels present after channel enable + rebuild
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: manifest.ts hooks array has only enroll hooks; prior review PRA-24

PRA-22 Improvement — Non-interactive workaround path test missing

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70
  • Category: tests
  • Problem: Unit test covers non-interactive throw behavior but no test documents the workaround rationale or verifies the skip message matches onboarding docs.
  • Impact: Behavior is tested but not linked to documented operator guidance; future changes could diverge from docs.
  • Suggested action: Add test comment or separate doc test linking the non-interactive skip to onboarding documentation once PRA-6 doc update lands.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check tunnel-audience-gate.test.ts for comment referencing onboarding doc section
  • Missing regression test: Doc-linked test or documentation test verifying skip message matches onboarding guide
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: tunnel-audience-gate.test.ts:55-66 tests throw; prior review PRA-26

PRA-23 Improvement — DEFAULT_WEBHOOK_PATH constant used only once

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:15
  • Category: architecture
  • Problem: Constant `const DEFAULT_WEBHOOK_PATH = "/googlechat";` defined at line 15, used only at line 130. Manifest render also hardcodes webhookPath: "/googlechat".
  • Impact: Drift risk between constant and manifest hardcode; unnecessary abstraction adds cognitive load without reuse benefit.
  • Suggested action: Inline the constant or derive from manifest's webhookPath render value to avoid drift. If kept as constant, ensure single source of truth.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: grep -n 'DEFAULT_WEBHOOK_PATH' src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts should show 1 definition + 1 usage, or 0 if inlined
  • Missing regression test: None needed for simplification
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: simplificationSignals shows single_use_config at line 15 and 130
Simplification opportunities: 1 possible cut, net -1 lines possible

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

  • PRA-23 delete (src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:15): const DEFAULT_WEBHOOK_PATH = "/googlechat"; at line 15
    • Replacement: Use literal "/googlechat" at line 130, or import from manifest's webhookPath render value
    • Net: -1 lines
    • Safety boundary: webhookPath in manifest render (line 103) must remain "/googlechat" — this is the contract with plugin inbound route
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 — googlechat-boot-health-check-verifies-outbound-auth-patch-applied. Runtime/sandbox/infrastructure paths need behavioral runtime validation: boot-time health checks for preloads, bridge provider integration flow with fake gateway, full channel enable→rebuild→send/receive cycle.
  • PRA-T2 Runtime validation — googlechat-boot-health-check-verifies-trusted-proxy-fetch-patch-applied. Runtime/sandbox/infrastructure paths need behavioral runtime validation: boot-time health checks for preloads, bridge provider integration flow with fake gateway, full channel enable→rebuild→send/receive cycle.
  • PRA-T3 Runtime validation — googlechat-bridge-provider-lifecycle-with-fake-openshell-gateway. Runtime/sandbox/infrastructure paths need behavioral runtime validation: boot-time health checks for preloads, bridge provider integration flow with fake gateway, full channel enable→rebuild→send/receive cycle.
  • PRA-T4 Runtime validation — googlechat-non-interactive-skip-documented-in-onboarding-guide. Runtime/sandbox/infrastructure paths need behavioral runtime validation: boot-time health checks for preloads, bridge provider integration flow with fake gateway, full channel enable→rebuild→send/receive cycle.
  • PRA-T5 Runtime validation — googlechat-appPrincipal-sentinel-collision-assumption-verified-vs-google-docs. Runtime/sandbox/infrastructure paths need behavioral runtime validation: boot-time health checks for preloads, bridge provider integration flow with fake gateway, full channel enable→rebuild→send/receive cycle.
  • PRA-T6 No integration flow test for bridge provider lifecycle with fake OpenShell gateway — Add integration test using fake OpenShell gateway (or in-process gateway mock) that verifies: profile imported, provider created with sentinel, refresh configured with secret material, token minted and rotated.
  • PRA-T7 Missing health-check hook entirely for Google Chat — Add health-check hook(s) that verify both preloads are installed and patches applied. Can reuse openclawBridgeHealth helper from hook registry.
  • PRA-T8 Non-interactive workaround path test missing — Add test comment or separate doc test linking the non-interactive skip to onboarding documentation once PRA-6 doc update lands.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: googlechat-outbound-auth.ts (outbound auth rewrite preload)

  • 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: googlechat-outbound-auth.test.ts: anchor matching, drift throw, idempotency, short-circuit throws when env unset
  • 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: Workaround Analysis block at lines 10-55; prior review PRA-7

PRA-2 Resolve/justify — Source-of-truth review needed: googlechat-trusted-proxy-fetch.ts (trusted proxy fetch rewrite preload)

  • 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: googlechat-trusted-proxy-fetch.test.ts: anchor matching (3 sites), drift throw, idempotency, site mapping
  • 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: Workaround Analysis block at lines 11-55; prior review PRA-8

PRA-3 Resolve/justify — Source-of-truth review needed: messaging-bridge-provider.ts:268 (argv secret transit)

  • 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: messaging-bridge-provider.test.ts: configureMessagingBridgeRefreshes success/fail-closed tests
  • 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: SECURITY comment at lines 243-274; prior review PRA-11, PRA-12

PRA-4 Required — 455-line monolith for generic bridge provider wiring

  • Location: src/lib/onboard/messaging-bridge-provider.ts:1
  • Category: correctness
  • Problem: Single file handles profile discovery, token def collection, profile import, refresh configuration, and secret resolution mixed together. Exceeds reviewable size and mixes multiple responsibilities.
  • Impact: Hard to review security boundaries; changes to one concern risk breaking another; no clear module boundaries for testing or future channels.
  • Required action: Extract into focused modules: list-bridge-profiles.ts, collect-bridge-token-defs.ts, ensure-bridge-profiles.ts, configure-bridge-refreshes.ts, bridge-secret-resolution.ts, plus a thin barrel export. Each <150 lines.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -r 'export.*MessagingBridge' src/lib/onboard/ --include="*.ts" | wc -l should show multiple focused files after extraction
  • Missing regression test: Each extracted module should have its own test file mirroring current messaging-bridge-provider.test.ts coverage
  • Done when: The required change is committed and verification passes: grep -r 'export.*MessagingBridge' src/lib/onboard/ --include="*.ts" | wc -l should show multiple focused files after extraction.
  • Evidence: File is 455 lines with 5 exported functions handling distinct phases; prior review PRA-2

PRA-5 Required — Test monolith grew to 1041 lines with Google Chat tests

  • Location: src/lib/messaging/channels/manifests.test.ts:1
  • Category: correctness
  • Problem: manifests.test.ts now contains all channel manifest tests including 163 lines of Google Chat specific tests.
  • Impact: Test file too large for effective review; changes to one channel's tests risk breaking others; slows test iteration.
  • Required action: Split into per-channel test files (e.g., googlechat.manifest.test.ts, teams.manifest.test.ts) or by concern (hooks, render, runtime, packages). Target <500 lines per file.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: wc -l src/lib/messaging/channels/*.test.ts should show no file >500 lines
  • Missing regression test: Each split test file must preserve all current test coverage for its channel
  • Done when: The required change is committed and verification passes: wc -l src/lib/messaging/channels/*.test.ts should show no file >500 lines.
  • Evidence: File grew from 918 to 1041 lines (+123) with Google Chat additions; prior review PRA-3

PRA-6 Required — providers.ts monolith grew to 556 lines with bridge orchestration

  • Location: src/lib/onboard/providers.ts:1
  • Category: correctness
  • Problem: providers.ts mixes inference provider selection, hosted inference staging, gateway provider CRUD, and now bridge provider orchestration via upsertMessagingProviders.
  • Impact: Bridge provider logic coupled to inference provider logic; changes to one domain risk the other; exceeds single-responsibility.
  • Required action: Extract bridge provider orchestration into messaging-bridge-upsert.ts that upsertMessagingProviders delegates to. Keep providers.ts focused on inference/provider selection.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -n 'upsertMessagingProviders' src/lib/onboard/providers.ts should show only a thin delegation call after extraction
  • Missing regression test: New messaging-bridge-upsert.test.ts covering the delegation and error paths
  • Done when: The required change is committed and verification passes: grep -n 'upsertMessagingProviders' src/lib/onboard/providers.ts should show only a thin delegation call after extraction.
  • Evidence: File grew from 507 to 556 lines (+49) with bridge wiring; prior review PRA-4

PRA-7 Required — tunnel/services.ts monolith grew to 771 lines with lazy-require cycle

  • Location: src/lib/tunnel/services.ts:640
  • Category: correctness
  • Problem: services.ts handles cloudflared lifecycle, gateway stop, PID dir resolution. tunnel-runtime.ts lazy-requires services.ts creating import cycle risk; resolveServicePidDir duplicated.
  • Impact: Import cycle risk; duplicated PID dir resolution; hard to test PID logic in isolation.
  • Required action: Extract PID directory resolution into tunnel/pid-dir.ts used by both services.ts and tunnel-runtime.ts. Consider splitting services.ts into cloudflared-lifecycle.ts, gateway-stop.ts, pid-dir.ts.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts imported by both
  • Missing regression test: tunnel/pid-dir.test.ts covering resolution logic with various env/sandbox combinations
  • Done when: The required change is committed and verification passes: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts imported by both.
  • Evidence: File grew from 749 to 771 lines (+22); tunnel-runtime.ts:22-30 duplicates resolveServicePidDir via lazy require; prior review PRA-5, PRA-17

PRA-8 Required — Non-interactive mode unconditionally skips Google Chat even with pre-supplied audience

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-audience-gate.ts:70
  • Category: correctness
  • Problem: Hook throws at line 50-62 when context.isInteractive===false, even if GOOGLECHAT_AUDIENCE is pre-configured. Documented as intentional (Cloud Console + appPrincipal steps need human) but not in onboarding docs.
  • Impact: Automation/CI cannot enable Google Chat non-interactively even with all config pre-supplied; operators surprised by skip.
  • Required action: Document in onboarding docs: Google Chat requires interactive enrollment (Cloud Console endpoint URL + appPrincipal). If automation needed later, hook must be updated to support pre-captured appPrincipal + audience.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check docs/onboarding.md or similar for 'Google Chat' and 'non-interactive' sections
  • Missing regression test: Doc update verified; or if automation path added later, test that pre-supplied audience + appPrincipal bypasses skip
  • Done when: The required change is committed and verification passes: Check docs/onboarding.md or similar for 'Google Chat' and 'non-interactive' sections.
  • Evidence: tunnel-audience-gate.ts:50-62 throws unconditionally; test at tunnel-audience-gate.test.ts:55-66 confirms behavior; prior review PRA-6

PRA-9 Required — Missing boot-time health check for googlechat-outbound-auth patch

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:60
  • Category: security
  • Problem: Patch drift logs but continues gateway boot; fail-closed only at send time. Channel can start with unpatched plugin and fail only on first outbound reply.
  • Impact: Silent degradation: bot receives messages but cannot reply until first send attempt; operator unaware until user reports broken replies.
  • Required action: Add boot-time health check hook for googlechat that verifies process.__nemoclawGooglechatOutboundAuthInstalled and patch applied. Fail gateway startup if patch missing and channel active.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check manifest.ts hooks array for a health-check phase hook that validates the outbound-auth sentinel
  • Missing regression test: Integration test: start gateway with googlechat channel active but outbound-auth preload disabled; verify gateway fails to start
  • Done when: The required change is committed and verification passes: Check manifest.ts hooks array for a health-check phase hook that validates the outbound-auth sentinel.
  • Evidence: googlechat-outbound-auth.ts:225-236 catches drift, logs, continues; prior review PRA-14, PRA-T7, PRA-22

PRA-10 Required — Missing boot-time health check for googlechat-trusted-proxy-fetch patch

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:85
  • Category: security
  • Problem: Patch drift logs but continues gateway boot; fail-closed only at fetch time (cert verify or outbound send). Channel can start with unpatched plugin and fail on first inbound cert verify or outbound call.
  • Impact: Silent degradation: bot starts but inbound verification fails (401 on webhook) or outbound fails; operator unaware until first message.
  • Required action: Add boot-time health check hook verifying process.__nemoclawGooglechatTrustedProxyFetchInstalled and all three anchors patched. Fail gateway startup if patch missing and channel active.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check manifest.ts hooks array for a health-check phase hook that validates the trusted-proxy-fetch sentinel
  • Missing regression test: Integration test: start gateway with googlechat channel active but trusted-proxy-fetch preload disabled; verify gateway fails to start
  • Done when: The required change is committed and verification passes: Check manifest.ts hooks array for a health-check phase hook that validates the trusted-proxy-fetch sentinel.
  • Evidence: googlechat-trusted-proxy-fetch.ts:240-250 catches drift, logs, continues; prior review PRA-15, PRA-T8, PRA-23

PRA-11 Resolve/justify — Service account private key passes through argv to provider refresh configure

  • Location: src/lib/onboard/messaging-bridge-provider.ts:268
  • Category: security
  • Problem: OpenShell `provider refresh configure` ingests refresh material only via `--material KEY=VALUE` argv — no stdin, file, or env-ref transport. Private key transits host-local argv (ps/procfs visible).
  • Impact: Host-local secret exposure on trusted host that already holds the key; transient (one configure call); key never enters sandbox. Tracked upstream for --secret-material-file/stdin.
  • Recommended action: Add TODO comment with upstream OpenShell issue reference for --secret-material-file/stdin transport. Document that key never enters sandbox (key-out-of-sandbox boundary holds).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -n 'TODO.*secret-material-file' src/lib/onboard/messaging-bridge-provider.ts should show tracking issue reference
  • Missing regression test: None needed for current behavior; when upstream adds file/stdin transport, test that argv path is no longer used
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -n 'TODO.*secret-material-file' src/lib/onboard/messaging-bridge-provider.ts should show tracking issue reference.
  • Evidence: messaging-bridge-provider.ts:243-274 comment documents risk; prior review PRA-11, PRA-12

PRA-12 Resolve/justify — @ts-nocheck on security-critical auth rewrite preload

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-outbound-auth.ts:1
  • Category: security
  • Problem: File uses @ts-nocheck because it manipulates Module._extensions and Module.registerHooks with untyped shapes. Type safety gaps in security-critical patch logic.
  • Impact: Type errors in patch logic (anchor regex, short-circuit source) would only surface at runtime in gateway boot.
  • Recommended action: Migrate to ESM with proper types, or add JSDoc @typedef for Module._extensions and Module.registerHooks shapes. File follow-up to remove @ts-nocheck.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode
  • Missing regression test: TypeScript compile check in CI for this file
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode.
  • Evidence: First line of googlechat-outbound-auth.ts is @ts-nocheck; prior review PRA-9

PRA-13 Resolve/justify — @ts-nocheck on security-critical SSRF guard rewrite preload

  • Location: src/lib/messaging/channels/googlechat/runtime/googlechat-trusted-proxy-fetch.ts:1
  • Category: security
  • Problem: File uses @ts-nocheck for same Module hook manipulation reasons as outbound-auth. Type safety gaps in SSRF guard rewrite.
  • Impact: Type errors in anchor regexes (ANCHOR_A/B/C) or replacement logic would only surface at runtime.
  • Recommended action: Same as outbound-auth: migrate to typed ESM or add JSDoc types for Module hooks. File follow-up to remove @ts-nocheck.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode
  • Missing regression test: TypeScript compile check in CI for this file
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: File should not have @ts-nocheck; TypeScript compile should pass with strict mode.
  • Evidence: First line of googlechat-trusted-proxy-fetch.ts is @ts-nocheck; prior review PRA-10

PRA-14 Resolve/justify — Synthetic serviceAccountFile sentinel for channel start gate

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:115
  • Category: security
  • Problem: Renders non-existent path `/nonexistent/googlechat-gateway-minted-no-service-account-file` as serviceAccountFile to satisfy OpenClaw start gate (requires non-empty serviceAccount*). File never delivered or read; outbound-auth preload short-circuits before read.
  • Impact: Workaround for upstream missing 'configured'/accessToken credential source. If preload fails to load, sentinel path could be read (but secret scan would catch private key leakage).
  • Recommended action: Keep workaround analysis block current (lines 143-158). When upstream adds native accessToken auth mode, remove sentinel and gateway-reload-off fragment. Add test that sentinel path never accessed.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check manifest.ts workaround analysis block lines 143-158 for removal condition tracking
  • Missing regression test: Test that verifies serviceAccountFile path is never read at runtime (e.g., fs.readFile spy on sentinel path never called)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check manifest.ts workaround analysis block lines 143-158 for removal condition tracking.
  • Evidence: manifest.ts:98-105 sentinel with comment; workaround analysis at lines 115-130; prior review PRA-13

PRA-15 Resolve/justify — Two nodePreloads target same bundle with independent Module._extensions overrides

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:200
  • Category: architecture
  • Problem: Both googlechat-trusted-proxy-fetch and googlechat-outbound-auth preloads wrap Module._extensions['.js'] and Module.registerHooks independently, targeting the same @openclaw/googlechat dist bundle.
  • Impact: Duplicate loader wrapping; potential order-dependent behavior; harder to reason about patch composition.
  • Recommended action: Document preload order independence. Consider shared loader-hook installer utility to avoid duplicate Module._extensions wrapping.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if both preloads install hooks in same IIFE or if order matters; look for shared utility in src/lib/messaging/channels/googlechat/runtime/
  • Missing regression test: Test that both preloads can be loaded in either order and both patches apply correctly
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if both preloads install hooks in same IIFE or if order matters; look for shared utility in src/lib/messaging/channels/googlechat/runtime/.
  • Evidence: manifest.ts:200-218 two nodePreloads entries; prior review PRA-19

PRA-16 Resolve/justify — Lazy-require cycle: tunnel-runtime.ts duplicates resolveServicePidDir from services.ts

  • Location: src/lib/messaging/channels/googlechat/hooks/tunnel-runtime.ts:22
  • Category: architecture
  • Problem: tunnel-runtime.ts lazy-requires tunnel/services to access resolveServicePidDir and readCloudflaredState, creating import cycle risk and duplicating PID dir logic.
  • Impact: Cycle risk if services.ts ever imports tunnel-runtime; duplicated logic; harder to test PID resolution in isolation.
  • Recommended action: Extract pure resolveServicePidDir and validateSandboxName to tunnel/pid-dir.ts (shared module) so both services.ts and tunnel-runtime.ts import eagerly without cycle.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts
  • Missing regression test: tunnel/pid-dir.test.ts covering resolution logic; both services.ts and tunnel-runtime.ts tests import from shared module
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: grep -r 'resolveServicePidDir' src/lib/tunnel/ should show single definition in pid-dir.ts.
  • Evidence: tunnel-runtime.ts:22-30 lazy-requires services.ts; prior review PRA-17

PRA-17 Resolve/justify — AppPrincipal discovery sentinel collision assumption unverified

  • Location: src/lib/messaging/channels/googlechat/template-resolver.ts:20
  • Category: correctness
  • Problem: All-zeros sentinel `000000000000000000000` assumed to never collide with real Google-assigned add-on principal (~21 digits). No test or doc reference confirming Google assigns non-zero numeric IDs.
  • Impact: If Google ever assigns all-zeros principal, discovery log would not surface real value; operator would see 'missing add-on principal binding' instead of the number.
  • Recommended action: Add test or doc reference confirming Google add-on principal is 21-digit non-zero numeric ID. If confirmed, note in test comment.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check template-resolver.test.ts for comment referencing Google documentation or test confirming non-zero assignment
  • Missing regression test: Test comment or doc link verifying Google principal format; or test that sentinel != any known real principal format
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check template-resolver.test.ts for comment referencing Google documentation or test confirming non-zero assignment.
  • Evidence: template-resolver.ts:19-22 sentinel with comment; test uses 21-digit value but no source reference; prior review PRA-16, PRA-25

PRA-18 Resolve/justify — Gateway reload off fragment is a localized workaround

  • Location: src/lib/messaging/channels/googlechat/manifest.ts:98
  • Category: architecture
  • Problem: Renders gateway.reload.mode=off to prevent OpenClaw's ~60s post-boot self-write from dropping webhook route. Workaround analysis block present (lines 143-158).
  • Impact: Disables reactive config reload entirely for sandbox; NemoClaw still restarts gateway explicitly on rebuild/restart. Low risk but upstream fix preferred.
  • Recommended action: Keep workaround analysis block current. Remove when upstream reload re-mounts channels (not just plugins) on config reload.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check manifest.ts lines 143-158 for workaround analysis with removal condition
  • Missing regression test: Test that gateway reload mode is 'off' in rendered openclaw.json when googlechat channel active
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check manifest.ts lines 143-158 for workaround analysis with removal condition.
  • Evidence: manifest.ts:115-130 render fragment + workaround analysis lines 143-158; prior review PRA-18

PRA-19 Improvement — Per-channel rendered-config-parser boilerplate could be unified

  • Location: src/lib/messaging/channels/googlechat/rendered-config-parser.ts:1
  • Category: architecture
  • Problem: Google Chat has custom RenderedChannelConfigParser (41 lines) that mirrors pattern of other channels but with channel-specific keys.
  • Impact: Boilerplate duplication across channels; new channels must copy-paste parser structure.
  • Suggested action: Evaluate generic RenderedChannelConfigParser factory that takes manifest and agentId, auto-discovers config inputs with statePath. If not feasible, document why Google Chat needs custom parser.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check if other channels (slack, teams, etc.) have similar parser files with same structure
  • Missing regression test: If factory created, test that it produces correct visibility keys for all existing channels
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: rendered-config-parser.ts:41 lines; prior review PRA-20

PRA-20 Improvement — No integration flow test for bridge provider lifecycle with fake OpenShell gateway

  • Location: src/lib/onboard/messaging-bridge-provider.test.ts:1
  • Category: tests
  • Problem: Unit tests mock runOpenshell; no test exercises full bridge provider lifecycle (profile import → provider create → refresh configure) against a fake OpenShell gateway.
  • Impact: Integration gaps between bridge wiring and gateway behavior undetected; argv secret transit, refresh timing, provider detachment not validated end-to-end.
  • Suggested action: Add integration test using fake OpenShell gateway (or in-process gateway mock) that verifies: profile imported, provider created with sentinel, refresh configured with secret material, token minted and rotated.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Look for test file using fake gateway or in-process openshell mock for bridge provider flow
  • Missing regression test: Integration test: googlechat-bridge-provider-lifecycle-with-fake-openshell-gateway
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: messaging-bridge-provider.test.ts only unit tests with mocked runOpenshell; prior review PRA-T6, PRA-T21, PRA-T27

Workflow run details

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

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions
Status: No actionable findings remain in the canonical review ledger.

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 2 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported the same number of blockers, 2 more warnings, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard, credential-sanitization, security-posture, channels-add-remove, channels-stop-start, inference-routing, network-policy, onboard-repair, onboard-resume

1 optional E2E recommendation
  • tunnel-lifecycle

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@wscurran wscurran added area: messaging Messaging channels, bridges, manifests, or channel lifecycle area: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: policy Network policy, egress rules, presets, or sandbox policy area: security Security controls, permissions, secrets, or hardening feature PR adds or expands user-visible functionality integration: openclaw OpenClaw integration behavior labels Jul 1, 2026
@sandl99 sandl99 removed the v0.0.77 Release target label Jul 8, 2026
@hunglp6d

hunglp6d commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

Current blocked due to security issue in OpenShell: NVIDIA/OpenShell#2104

@cv cv added the v0.0.82 Release target label Jul 12, 2026
@cjagwani cjagwani added v0.0.83 Release target and removed v0.0.82 Release target labels Jul 13, 2026
@github-actions

Copy link
Copy Markdown
Contributor

@cv

cv commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Current-head update (a621c3c):

  • Resolved the branch conflicts against current main; GitHub now reports the PR as mergeable with no content conflicts.
  • The direct channels add googlechat bridge lifecycle, rollback, and cleanup coverage is present and passes.
  • The service-account private key now uses OpenShell --secret-material-env; the key value is absent from child argv and the parent environment remains unchanged.
  • Targeted verification: 19 files / 205 tests passed; commit and push hooks passed.
  • Added canonical Google Chat setup and lifecycle docs; npm run docs completed with 0 errors (2 pre-existing warnings).

Merge prerequisite: #6726 must land first because current main still pins OpenShell 0.0.72; the required --secret-material-env support is available in OpenShell 0.0.82.

The pushed commits are signed and GitHub-verified. Please re-review the current head once CI completes.

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

The PR description image shows a Google Chat conversation screenshot with a bot named "NemoClaw-GGChat". The user sends "Hello" and the bot replies "Hello Hung! How can I help you today?" (marked as edited). The user then sends "Reply exactly: PONG" and the bot responds "PONG" (marked as "App" and "Edited"). This appears to be a live end-to-end verification that the Google Chat channel is functioning correctly — inbound messages are received and outbound replies are delivered through the webhook.

@cv Thanks for the update — the Google Chat E2E screenshot (Hello → PONG round trip) is a good confirmation the webhook/reply path works end-to-end. Noted on:

  • Bridge lifecycle/rollback/cleanup coverage for the direct add path passing.
  • Service-account key handling now going through OpenShell with the key absent from child argv and parent env unaffected.
  • 19 files / 205 targeted tests passing, docs:strict clean.
  • Merge prerequisite: #6726 needs to land first since this PR currently pins OpenShell 0.0.72 but requires the support shipped in 0.0.82 — I'll keep that dependency in mind when assessing mergeability.

Kicking off a fresh review of the current head now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

The PR description image shows a mobile chat interface titled "NemoClaw-GGChat" with a bot avatar. It displays a conversation: the user sent "Hello" 35 minutes ago, and "NemoClaw-GGChat" replied "Hello Hung! How can I help you today?" (marked as edited). Then the user sent "Reply exactly: PONG" just now, and "NemoClaw-GGChat" (labeled "App") replied "PONG" (also marked as edited). This appears to be a live end-to-end demonstration of the Google Chat integration working — the bot is successfully receiving and responding to messages through the Google Chat channel, confirming the inbound/outbound webhook flow is functional.

@cv Thanks for the detailed update. Noted the key points:

  • Branch conflicts resolved against main, mergeable.
  • Direct channels add googlechat bridge lifecycle/rollback/cleanup path is covered and passing — good, that closes the loop on the earlier lifecycle concern.
  • Private key material now goes through OpenShell's --secret-material-env instead of --material KEY=VALUE on argv — this resolves the security blocker (PRA-6/PRA-7) that was the main point of contention in this thread.
  • 205 tests across 19 files passing, docs regenerated cleanly.

One thing worth flagging explicitly for anyone tracking merge status: this PR is not yet mergeable in practice even with conflicts resolved, since --secret-material-env requires OpenShell 0.0.82 and main is still pinned to 0.0.72 until #6726 lands. It'd be good to mark this PR as blocked/draft (if not already) so it isn't merged prematurely ahead of the OpenShell version bump.

The image showing the live Hello/PONG exchange through "NemoClaw-GGChat" is a nice concrete confirmation that the inbound/outbound webhook flow works end-to-end.

I'll kick off a fresh review of the current head now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@cv cv marked this pull request as draft July 14, 2026 10:12
@cv

cv commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Security review for final head 439331dac6306486707a07c4f9cb44f9f8000e19:

Overall verdict: WARNING (merge prerequisite), with no code-level security blocker found. This PR is ready for maintainer review after #6726 lands OpenShell 0.0.82; it should remain draft until that prerequisite is mergeable and merged.

  1. Secrets and credentials — PASS. The Google service-account private key is passed only through the refresh child process environment via --secret-material-env; regression coverage proves it is absent from argv, registry state, and onboarding session state. PEM redaction and repository secret scans pass.
  2. Input validation — WARNING (non-blocking). Service-account JSON fails closed unless client_email and private_key exist, and channel/audience/principal inputs use the manifest validation path. These are operator-local inputs rather than an untrusted network parsing boundary; stricter semantic validation can be considered separately.
  3. Authentication and authorization — PASS. Inbound Google Chat identity remains JWT/app-principal/allowlist guarded. The outbound sentinel is non-authorizing; the gateway mints the real bearer credential.
  4. Dependency security — PASS. The channel package is exact-version pinned with reviewed tarball/SRI and repository lifecycle controls.
  5. Error handling and logging — PASS. Refresh configuration fails closed, diagnostics are redacted, and secret material is not logged or serialized.
  6. Cryptography — PASS. Token minting uses the OpenShell Google service-account JWT strategy; no custom sandbox-side cryptography was introduced.
  7. Configuration and network boundaries — PASS for the automatic endpoint. Automatic enrollment starts a dedicated loopback proxy and quick tunnel that accept only POST /googlechat; dashboard, health, WebSocket, other routes, and bodies larger than 1 MiB are denied before reaching OpenClaw. The full-dashboard nemoclaw tunnel service remains separate. Operator-managed audience URLs remain responsible for equivalent route restriction.
  8. Security testing — PASS. Real HTTP coverage proves the allowed webhook path, representative denied control paths, the 1 MiB limit, and private state-file modes. Runtime and lifecycle coverage proves both processes are required for readiness, failed startup cleans up, channel removal stops them, and sandbox destroy cleans them up. Direct add/remove/rollback coverage protects the gateway bridge-provider lifecycle.
  9. Holistic architecture — PASS, conditional on chore(openshell): upgrade supported version to 0.0.82 #6726. The service-account key stays host/gateway-side, the sandbox receives only a gateway-minted token, and both CommonJS source-rewrite preloads compose regardless of preload order.
  10. E2E liveness instrumentation — PASS. The stop/start target now emits only a fixed, secret-free agent identifier every 60 seconds; it never prints captured command output. Tests cover timer cleanup, idempotent stop, unref, and best-effort logging.

Final verification: the signed commit is GitHub Verified and includes DCO. The main CI rollup has 42 passing and 5 skipped checks with no failures; all 11 authorized exact-diff live E2E jobs passed, including the 16-minute Hermes channel stop/start target: https://github.com/NVIDIA/NemoClaw/actions/runs/29335937019. Focused lifecycle tests, npm run test:changed, project-membership validation, Biome, CLI type-check/build, docs build, source-shape/file-size budgets, gitleaks, normal commit hooks, and pre-push checks passed.

The canonical review-advisor ledger reports 0 blockers, 0 warnings, and 0 suggestions, with both Terra and Nemotron lanes completed successfully: #6120 (comment). No advisor follow-up is required.

cv added 3 commits July 14, 2026 03:45
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Comment on lines +5 to +16
import {
chmodSync,
closeSync,
constants,
existsSync,
fchmodSync,
mkdirSync,
openSync,
readFileSync,
rmSync,
writeFileSync,
} from "node:fs";
Comment on lines +5 to +16
import {
chmodSync,
closeSync,
constants,
existsSync,
fchmodSync,
mkdirSync,
openSync,
readFileSync,
rmSync,
writeFileSync,
} from "node:fs";
cv added 3 commits July 14, 2026 05:08
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv dismissed their stale review July 14, 2026 13:37

Both requested blockers are addressed on final Verified head 439331d: Google Chat provider lifecycle now covers add/remove/rollback, and service-account key transport uses --secret-material-env instead of argv. The canonical advisor rerun reports 0 blockers, 0 warnings, and 0 suggestions; exact-diff E2E is green.

@cv cv removed the v0.0.83 Release target label Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: messaging Messaging channels, bridges, manifests, or channel lifecycle area: networking DNS, proxy, TLS, ports, host aliases, or connectivity area: policy Network policy, egress rules, presets, or sandbox policy area: security Security controls, permissions, secrets, or hardening feature PR adds or expands user-visible functionality integration: openclaw OpenClaw integration behavior VRDC Issues and PRs submitted by NVIDIA VRDC test team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants