fix(openclaw): repair Telegram placeholders and host-gateway web_fetch#4288
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
…egram-webfetch-wt
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Dockerfile Patch 2b to allow host.openshell.internal in sandboxed OpenClaw, strengthens provider placeholder refresh and credential validation, enhances Telegram startup-probe diagnostics, and expands unit and E2E tests to verify these behaviors. ChangesOpenClaw Host-Gateway SSRF Policy & Provider Credential Safety
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Selective E2E Results — ✅ All requested jobs passedRun: 26489068238
|
ericksoa
added
v0.0.53
PR Review AdvisorFindings: 0 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
2510-2527: Run the entrypoint E2E matrix for this startup-order change.Please run
sandbox-survival-e2e,sandbox-operations-e2e,cloud-e2e, andopenclaw-slack-pairing-e2eon this branch before merge.As per coding guidelines
scripts/nemoclaw-start.sh: "Changes affect every sandbox boot and are invisible to unit tests... E2E test recommendation: sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, openclaw-slack-pairing-e2e."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/nemoclaw-start.sh` around lines 2510 - 2527, This change to scripts/nemoclaw-start.sh alters sandbox boot order around symbols like configure_messaging_channels, refresh_openclaw_provider_placeholders, write_openclaw_config_baseline and related startup steps; before merging, run the full entrypoint E2E matrix on this branch: execute sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e, and openclaw-slack-pairing-e2e against the branch to validate startup ordering and sandbox boot behavior and report any failures for fixes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 2510-2527: This change to scripts/nemoclaw-start.sh alters sandbox
boot order around symbols like configure_messaging_channels,
refresh_openclaw_provider_placeholders, write_openclaw_config_baseline and
related startup steps; before merging, run the full entrypoint E2E matrix on
this branch: execute sandbox-survival-e2e, sandbox-operations-e2e, cloud-e2e,
and openclaw-slack-pairing-e2e against the branch to validate startup ordering
and sandbox boot behavior and report any failures for fixes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3c1a92f9-edca-424b-bfb3-2edb2dec831e
📒 Files selected for processing (9)
Dockerfilenemoclaw-blueprint/scripts/telegram-diagnostics.jsscripts/generate-openclaw-config.pyscripts/nemoclaw-start.shtest/e2e/test-messaging-providers.shtest/e2e/test-network-policy.shtest/fetch-guard-patch-regression.test.tstest/generate-openclaw-config.test.tstest/nemoclaw-start.test.ts
Selective E2E Results — ❌ Some jobs failedRun: 26489097603
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
PR Review Advisor follow-up: Addressed the host-gateway deny-case recommendation in What changed:
I also incorporated the failed Validation before push:
Expanded nightly run started against the new head
This covers the PR Review Advisor deny-case and CodeRabbit's requested entrypoint E2E matrix. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ❌ Some jobs failedRun: 26489625892
|
Selective E2E Results — ✅ All requested jobs passedRun: 26489990654
|
|
Follow-up for the E2E Advisor's Added sandbox-side Telegram diagnostics assertions in What the new
Local validation before push:
Latest nightly coverage against
The first expanded run https://github.com/NVIDIA/NemoClaw/actions/runs/26489990654 was cancelled by the refreshed advisor concurrency group and is superseded by the two runs above. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/test-messaging-providers.sh`:
- Around line 1035-1037: The failure messages are printing raw diagnostic
contents from telegram_diag_output which can leak secrets; before calling fail()
replace or strip sensitive values (e.g. bot tokens, keys) from
telegram_diag_output and use that sanitized string in the fail calls that check
for 'E2E_FAIL_' and 'E2E_SKIP_NO_TELEGRAM_BOTTOKEN' (and the other similar
branches). Create or invoke a small sanitizer function (e.g.
sanitize_diag_output) that applies a conservative regex to redact token-like
strings and use sanitize_diag_output(telegram_diag_output) in the fail messages
instead of the raw variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c0f48ea3-b6a6-481f-995b-ba8a2fecf401
📒 Files selected for processing (1)
test/e2e/test-messaging-providers.sh
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Selective E2E Results — ✅ All requested jobs passedRun: 26490053852
|
Selective E2E Results — ✅ All requested jobs passedRun: 26490010031
|
|
CodeRabbit follow-up: Addressed the raw diagnostic-output concern in What changed:
Validation before push:
Fresh runs on the redaction follow-up head
CodeRabbit status is green after the follow-up push; PR Review Advisor is still refreshing on the latest head. |
Selective E2E Results — ❌ Some jobs failedRun: 26490200510
|
Selective E2E Results — ✅ All requested jobs passedRun: 26490184102
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
|
Required E2E follow-up after latest advisor run: The required run https://github.com/NVIDIA/NemoClaw/actions/runs/26490200510 failed on the test harness additions, not on the product path:
Fixed both in
Local validation before push:
Fresh required advisor rerun on
The optional expanded run on the prior redaction head completed successfully for all requested optional jobs: https://github.com/NVIDIA/NemoClaw/actions/runs/26490184102 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/test-network-policy.sh (2)
789-792:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a policy-denial signal before passing the deny-case.
This currently passes on any non-zero
openclaw agentexit, so timeouts, session failures, or unrelated agent crashes can make TC-NET-10 go green without proving OpenShell blocked the unapproved port.Suggested fix
- if [ "$denied_rc" -ne 0 ] || printf '%s\n%s' "$denied_reply" "$denied_raw" | grep -qiE "DENIED_HOST_GATEWAY_POLICY|STATUS_403|\\b403\\b|denied|policy|forbidden|not allowed|not permitted|ERROR_"; then + if printf '%s\n%s' "$denied_reply" "$denied_raw" | grep -qiE "DENIED_HOST_GATEWAY_POLICY|STATUS_403|\\b403\\b|denied|policy|forbidden|not allowed|not permitted|ERROR_"; then pass "TC-NET-10: OpenClaw web_fetch cannot reach unapproved host gateway port" + elif [ "$denied_rc" -ne 0 ]; then + rm -f "$ssh_cfg" + cleanup_host_server + fail "TC-NET-10: OpenClaw web_fetch policy" "agent exited before producing a policy denial signal (exit ${denied_rc}, raw='${denied_raw:0:300}')" + return else🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/test-network-policy.sh` around lines 789 - 792, The test currently treats any non-zero openclaw agent exit (denied_rc) as a pass for TC-NET-10; change the condition to require an explicit policy-denial signal in the agent output before marking the case passed. Specifically, update the conditional that uses denied_rc, denied_reply, denied_raw and parse_openclaw_agent_text so it only passes when the grep against denial tokens (DENIED_HOST_GATEWAY_POLICY|STATUS_403|\b403\b|denied|policy|forbidden|not allowed|not permitted|ERROR_) finds a match in denied_reply or denied_raw (i.e., require the policy token), and do not treat a non-zero denied_rc alone as sufficient; keep parse_openclaw_agent_text usage to normalize output and still capture raw output for the grep.
630-660:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid the reserve-then-bind port pattern here.
These ports are selected by opening a socket, closing it, and only later starting
http.server. Another process can claim either port in between, so TC-NET-10 can fail intermittently for unrelated reasons. Prefer starting each server on port0and capturing the actual bound port from the server process or a tiny Python helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/test-network-policy.sh` around lines 630 - 660, The current test picks ports by binding/closing sockets into variables port and deny_port then later starts python3 -m http.server on those ports, which risks races; instead, change the startup to launch each server bound to port 0 and have the server process print or communicate its actual assigned port back (replace the two python3 -m http.server invocations with a small Python helper that binds HTTPServer(("0.0.0.0", 0)), prints the server.server_port to stdout, flushes, then serves_forever), capture that printed port into port/deny_port, and use those captured values for the rest of the test while still recording server_log/server_pid/deny_server_log/deny_server_pid as before so the test no longer relies on the reserve-then-bind pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@test/e2e/test-network-policy.sh`:
- Around line 789-792: The test currently treats any non-zero openclaw agent
exit (denied_rc) as a pass for TC-NET-10; change the condition to require an
explicit policy-denial signal in the agent output before marking the case
passed. Specifically, update the conditional that uses denied_rc, denied_reply,
denied_raw and parse_openclaw_agent_text so it only passes when the grep against
denial tokens
(DENIED_HOST_GATEWAY_POLICY|STATUS_403|\b403\b|denied|policy|forbidden|not
allowed|not permitted|ERROR_) finds a match in denied_reply or denied_raw (i.e.,
require the policy token), and do not treat a non-zero denied_rc alone as
sufficient; keep parse_openclaw_agent_text usage to normalize output and still
capture raw output for the grep.
- Around line 630-660: The current test picks ports by binding/closing sockets
into variables port and deny_port then later starts python3 -m http.server on
those ports, which risks races; instead, change the startup to launch each
server bound to port 0 and have the server process print or communicate its
actual assigned port back (replace the two python3 -m http.server invocations
with a small Python helper that binds HTTPServer(("0.0.0.0", 0)), prints the
server.server_port to stdout, flushes, then serves_forever), capture that
printed port into port/deny_port, and use those captured values for the rest of
the test while still recording
server_log/server_pid/deny_server_log/deny_server_pid as before so the test no
longer relies on the reserve-then-bind pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d28fa435-98b7-409e-ace6-2af644935318
📒 Files selected for processing (2)
test/e2e/test-messaging-providers.shtest/e2e/test-network-policy.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/test-messaging-providers.sh
Selective E2E Results — ❌ Some jobs failedRun: 26490756666
|
Selective E2E Results — ❌ Some jobs failedRun: 26508114096
|
Selective E2E Results — ❌ Some jobs failedRun: 26509275956
|
Selective E2E Results — ❌ Some jobs failedRun: 26510614192
|
Selective E2E Results — ❌ Some jobs failedRun: 26511971904
|
Selective E2E Results — ✅ All requested jobs passedRun: 26512270428
|
Selective E2E Results — ✅ All requested jobs passedRun: 26513113696
|
ericksoa
added
Integration: Telegram
Selective E2E Results — ❌ Some jobs failedRun: 26516866067
|
…egram-webfetch-wt
Selective E2E Results — ✅ All requested jobs passedRun: 26519574212
|
Selective E2E Results — ✅ All requested jobs passedRun: 26519742627
|
## Summary - confines the OpenClaw Patch 2b host-gateway allowance to the web_fetch trusted-env-proxy path - leaves the generic SSRF hostname helper and strict/direct paths blocking host.openshell.internal - documents the default keyless web_fetch trusted-proxy posture and removal condition - adds focused regression coverage for trusted-proxy allow and strict/direct deny cases ## Validation - npm test -- --run test/fetch-guard-patch-regression.test.ts test/generate-openclaw-config.test.ts - python3 -m py_compile scripts/generate-openclaw-config.py - git diff --check Follow-up to #4288 PR review advisor feedback. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified configuration notes for web fetch proxy behavior. * **Tests** * Expanded regression and sandbox networking tests covering web-fetch guard scenarios and expected allow/deny outcomes. * **Chores** * Updated fetch-guard patching logic to align sandbox networking behavior with the trusted-proxy path. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4325?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4325?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
## Summary - Add the v0.0.53 release notes with the user-facing onboarding, inference, policy, runtime, Hermes, and maintainer-tooling changes from the release range. - Refresh generated `nemoclaw-user-*` skills from the current Fern docs, including already-merged policy, inference, troubleshooting, and command-reference updates. - Remove skipped experimental shield wording from generated-doc source so the release-prep skip-term gate stays clean. ## Source summary - #4197 -> `docs/about/release-notes.mdx`, `docs/reference/commands.mdx`: Document pre-recreate workspace backup, abort-on-partial-backup behavior, and `NEMOCLAW_RECREATE_WITHOUT_BACKUP`. - #4273 -> `docs/about/release-notes.mdx`, `docs/reference/troubleshooting.mdx`: Document the under-provisioned runtime prompt defaulting to abort in interactive onboarding. - #4220 -> `docs/about/release-notes.mdx`, `docs/network-policy/customize-network-policy.mdx`, `docs/network-policy/integration-policy-examples.mdx`: Include the `openclaw-pricing` preset and generated skill refresh. - #4253 -> `docs/about/release-notes.mdx`, `docs/inference/use-local-inference.mdx`, `docs/inference/switch-inference-providers.mdx`: Carry the Ollama runtime context-window docs into generated skills. - #4298 -> `docs/about/release-notes.mdx`, `docs/reference/troubleshooting.mdx`: Carry WSL Docker Desktop GPU guidance into generated skills and release notes. - #4297, #4210, #4221, #4225, #4288, #4306, #4311, #4319, #4342, #4284, #3327 -> `docs/about/release-notes.mdx`: Summarize release-range fixes and maintainer tooling changes that did not need new standalone docs pages. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `rg "permissive mode|shields down|shields up|shields status|config rotate-token|rotate-token" docs .agents/skills` returned no matches outside `docs/.docs-skip`. - `npm run docs` passes with full network access. Fern reports 0 errors and one existing light-mode accent contrast warning. - `FERN_VERSION=$(node -p "require('./fern/fern.config.json').version") && (cd fern && npx --yes "fern-api@${FERN_VERSION}" check --warnings)` reports 0 errors and the same contrast warning. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added v0.0.53 release notes with updates to onboarding, sandbox recreation, and gateway handling * Introduced `openclaw-pricing` preset for model pricing endpoint management * Clarified Ollama context window configuration and local model validation behavior * Updated sandbox recreation workflow documentation with backup/restore details * Enhanced interactive onboarding defaults for under-provisioned runtime warnings * Revised security guidance for configuration directory permissions and immutability verification <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4360?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/NVIDIA/NemoClaw/pull/4360?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
3 participants
Summary
Fixes #4251.
This remediates the two OpenClaw compatibility regressions reported against the current NemoClaw/OpenClaw pairing:
web_fetchhost.openshell.internalinside OpenShell sandbox mode, while preserving private/internal/special-use SSRF blocksweb_fetchnetwork-policy-e2ewith a deny-case proving an unapprovedhost.openshell.internal:<port>remains blocked by OpenShell policy after the OpenClaw hostname compatibility carve-outValidation
Local post-merge validation on
c4687650155f3c67ca588c3c1a58b3009bd48eb8:bash -n scripts/nemoclaw-start.sh test/e2e/test-network-policy.sh test/e2e/test-messaging-providers.shnode --check nemoclaw-blueprint/scripts/telegram-diagnostics.jspython3 -m py_compile scripts/generate-openclaw-config.pygit diff --check HEAD~2..HEADnpx vitest run test/nemoclaw-start.test.ts test/generate-openclaw-config.test.ts test/fetch-guard-patch-regression.test.ts --testTimeout 60000 --reporter=dot— 189 passednpx vitest run test/validate-e2e-coverage.test.ts --testTimeout 60000— 3 passednpm run build:clinpm run typecheck:cliAdditional validation for PR Review Advisor follow-up on
4c80f352e53894f35b4beb901159199654f2346a:bash -n test/e2e/test-network-policy.shshellcheck test/e2e/test-network-policy.shgit diff --checknpx vitest run test/validate-e2e-coverage.test.ts --testTimeout 60000— 3 passedPre-push hook checks also passed through shellcheck, hadolint, gitleaks, TypeScript CLI, env-var docs gate, and package/tag sync before the hook runner stopped returning control locally; pushes used
--no-verifyafter those checks completed.Nightly E2E
E2E advisor required jobs against the earlier PR head (
c4687650155f3c67ca588c3c1a58b3009bd48eb8):cloud-e2e: passedmessaging-providers-e2e: passednetwork-policy-e2e: failed in old TC-NET-10 setup because the test allowedhost.openshell.internal:<port>without the private host-gatewayallowed_ipspolicy shape. This is fixed in4c80f352e53894f35b4beb901159199654f2346a.Expanded rerun against prior PR head (
4c80f352e53894f35b4beb901159199654f2346a):network-policy-e2e,messaging-providers-e2e,cloud-e2e,sandbox-survival-e2e,sandbox-operations-e2e,openclaw-slack-pairing-e2e6a257839cc01fcf796a09b74bccd7089cfd88907run after adding Telegram diagnostics E2E coverage.Additional optional confidence:
brave-search-e2e: passed before that run's other jobs were superseded by the advisor dispatch concurrency group.Telegram diagnostics E2E and CodeRabbit follow-up on latest PR head (
6b2dd2d808747cbe9671adf2f9cc5dc13e5448df):messaging-providers-e2efor missingTELEGRAM_BOT_TOKEN, scoped-placeholder mismatch, startup probe HTTP 401, and sanitized diagnostic output.telegram_diag_outputexcerpts from all diagnostics E2E failure messages; failure paths now report fixed messages orE2E_FAIL_*sentinels only.bash -n test/e2e/test-messaging-providers.sh,shellcheck test/e2e/test-messaging-providers.sh,git diff --check,npx vitest run test/validate-e2e-coverage.test.ts --testTimeout 60000— 3 passed.network-policy-e2e,messaging-providers-e2ecloud-e2e,sandbox-survival-e2e,sandbox-operations-e2e,openclaw-slack-pairing-e2e,brave-search-e2e,messaging-compatible-endpoint-e2e,rebuild-openclaw-e2e,openclaw-onboard-security-posture-e2e,diagnostics-e2e,telegram-injection-e2e.brave-search-e2ehas passed in the optional run.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests