fix(policy): allowlist wttr.in in weather egress preset#5842
Merged
Conversation
|
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. |
hunglp6d
marked this pull request as ready for review
June 26, 2026 08:34
hunglp6d
added
VRDC
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the branch is 96%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the branch is 67%. Coverage data for the branch is not yet available. Show a code coverage summary of the most covered files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe weather preset now allows outbound HTTPS REST access to ChangesWeather network policy update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review Advisor — InformationalMerge posture: Informational / low confidence Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@nemoclaw-blueprint/policies/presets/weather.yaml`:
- Around line 12-28: This preset change widens the egress allowlist for the
weather policy, so validate it with the network-policy-e2e suite before release.
Re-run the relevant e2e coverage against the weather preset update, focusing on
the policy rules added for wttr.in and wttr.is in the weather.yaml preset, and
verify deny-by-default, whitelist behavior, hot-reload, and SSRF protections
still behave correctly.
🪄 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: cfcbe139-ab98-4e0e-bb8b-d06cd6a22695
📒 Files selected for processing (1)
nemoclaw-blueprint/policies/presets/weather.yaml
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 28228366566
|
Contributor
|
🌿 Preview your docs: https://nvidia-preview-pr-5842.docs.buildwithfern.com/nemoclaw |
Collaborator
Author
|
Update — all PRA items addressed; keeping
|
Contributor
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 (1)
docs/network-policy/integration-policy-examples.mdx (1)
254-254: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the em dash to match the docs style guide.
This sentence uses an em dash, but the docs guidelines for
docs/**/*.{md,mdx}say to avoid them. A comma-separated rewrite keeps the meaning and stays compliant.♻️ Proposed fix
- The preset covers read-only public weather and geocoding lookups — current conditions, forecasts, geocoding, and weather alerts — without enabling messaging or productivity APIs. + The preset covers read-only public weather and geocoding lookups, including current conditions, forecasts, geocoding, and weather alerts, without enabling messaging or productivity APIs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/network-policy/integration-policy-examples.mdx` at line 254, The sentence in the docs copy uses an em dash, which conflicts with the docs style guide for docs/**/*.{md,mdx}. Update the wording in the affected paragraph to use a comma-separated phrasing instead, keeping the same meaning while removing the em dash from the integration-policy-examples content.Source: Coding guidelines
🤖 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 `@docs/network-policy/integration-policy-examples.mdx`:
- Line 254: The sentence in the docs copy uses an em dash, which conflicts with
the docs style guide for docs/**/*.{md,mdx}. Update the wording in the affected
paragraph to use a comma-separated phrasing instead, keeping the same meaning
while removing the em dash from the integration-policy-examples content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 763d4c9f-adbe-4385-928c-4da70ff5cd02
📒 Files selected for processing (1)
docs/network-policy/integration-policy-examples.mdx
Contributor
PR Review Advisor (Nemotron Ultra) — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Collaborator
Author
|
Addressed all six (0 required); keeping
Coverage: required |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| common-egress-agent-e2e | |
| network-policy-e2e |
Contributor
Vitest E2E Scenario Results —
|
| Job | Result |
|---|---|
| common-egress-agent-vitest | |
| network-policy-vitest |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-scenario/live/common-egress-agent.test.ts`:
- Around line 741-744: The e2e check around runOpenClawAgentAssertion is
currently satisfied by the model emitting WEATHER_AGENT_OK without proving the
curl-based weather fetch actually ran. Update the prompt and assertion flow so
the agent must return the exact curl output from the installed weather skill,
then compare that output against a direct sandbox curl result or another
unguessable fetch-derived value, and apply the same tightening to the mirrored
shell scenario. Ensure the success condition depends on the live fetch path
rather than instruction-following alone.
In `@test/weather-policy.test.ts`:
- Around line 34-49: The weather policy contract test only verifies wttr.in, but
the preset is expected to document wttr.is as a fallback too. Update the
assertions in weather-policy.test to include wttr.is in the expected host list
and add a matching endpoint check for wttr.is with the same REST policy and
GET/HEAD-only rules, alongside the existing wttr.in expectation.
🪄 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: 0af42823-74cf-416b-a948-0bf45a7ad07d
📒 Files selected for processing (6)
docs/network-policy/integration-policy-examples.mdxnemoclaw-blueprint/policies/presets/weather.yamltest/e2e-scenario/live/common-egress-agent.test.tstest/e2e-scenario/live/network-policy.test.tstest/e2e/test-common-egress-agent-e2e.shtest/weather-policy.test.ts
✅ Files skipped from review due to trivial changes (1)
- docs/network-policy/integration-policy-examples.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- nemoclaw-blueprint/policies/presets/weather.yaml
…egress # Conflicts: # test/e2e/live/common-egress-agent.test.ts # test/e2e/test-common-egress-agent-e2e.sh
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| common-egress-agent | |
| network-policy | ✅ success |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| common-egress-agent | |
| network-policy |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| common-egress-agent | |
| network-policy |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results —
|
| Job | Result |
|---|---|
| common-egress-agent | |
| network-policy |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Target Results — ✅ All selected jobs passedRun: 28424896912
|
Collaborator
|
Maintainer disposition for the remaining exact-head advisor warnings: the PR body now records the accepted |
cv
approved these changes
Jun 30, 2026
cv
left a comment
cv
left a comment
Collaborator
There was a problem hiding this comment.
Maintainer review: exact-head unit/integration checks and required network-policy/common-egress-agent E2E passed; both PR Review Advisor models now report merge_as_is, DCO/verification is clean, and the bounded GET/HEAD-only wttr.in risk is documented.
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary The bundled OpenClaw weather skill calls `wttr.in`, but the NemoClaw `weather` egress preset only allowed Open-Meteo and weather.gov. Under deny-by-default egress, the current host was blocked. This change closes the NemoClaw-owned policy gap while keeping access read-only and least-privileged. ## Related Issue Addresses the NemoClaw-owned `wttr.in` egress portion of NVIDIA#1417. This PR does not claim to change or fully validate OpenClaw skill readiness, browser fallback, pairing behavior, or the entire upstream weather-skill workflow. ## Changes - Allow `wttr.in:443` for GET and HEAD across its location-path namespace. - Do not pre-authorize `wttr.is`; the reviewed OpenClaw 2026.5.27 pin does not require it, and a future pin change must supply new runtime evidence before widening policy. - Record the source boundary next to the policy and enforce it through `loadAgent("openclaw").expectedVersion`: any OpenClaw pin change fails until maintainers revalidate the bundled skill's egress and update the reviewed contract. - Lock the exact weather host set and full `wttr.in` policy shape in `test/weather-policy.test.ts`. - Add live GET, HEAD, denied POST, and unrelated-host probes to the network-policy target. - Make the common-egress target perform one agent-executed `wttr.in/:help` curl, atomically retain the fetched body, and independently validate that proof from the host test. - Keep user documentation capability-based so it does not drift with providers. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only ## Quality Gates - [x] Focused structural coverage locks the exact policy boundary and forces review on OpenClaw pin drift. - [x] Live targets cover allowed GET/HEAD, denied POST, unrelated-host denial, and an agent-executed single-fetch proof. - [x] The broad `/**` path risk is accepted because wttr location identifiers are themselves paths and no stable narrower prefix exists; policy remains limited to one public host, GET/HEAD only, with no credentials sent. - [x] `wttr.is` exclusion is intentional least privilege; there is no reviewed evidence that the pinned runtime needs it. - [x] The C1 test is deliberately an egress/curl-path test, as its name and contract state; full upstream skill readiness is explicitly outside this PR's scope. - [x] The alleged NVIDIA#5645 conflict is not current: GitHub reports this PR mergeable. Any later cross-PR overlap will be resolved by the branch that lands second. - [x] The shell proof remains an array of fixed fragments plus a shell-quoted generated path, and passed exact-head live E2E; moving it to a separate script would add surface without changing the tested boundary. - [x] Docs describe user-facing capability without promising an upstream skill implementation. ## Verification - [x] PR description includes the DCO declaration and every commit appears Verified in GitHub. - [x] Commit and push hooks passed. - [x] `npx vitest run test/weather-policy.test.ts` passed (2/2). - [x] `npm run typecheck:cli` passed. - [x] `npm run source-shape:check` passed with zero source-shape tests. - [x] Full root CLI/integration hook suite passed during commit. - [x] No secrets, API keys, or credentials committed. - [x] `npm run docs` completed with zero errors and two pre-existing warnings. - [x] Exact-head E2E passed `network-policy` and `common-egress-agent`: https://github.com/NVIDIA/NemoClaw/actions/runs/28424896912 --- Signed-off-by: Hung Le <hple@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The bundled OpenClaw weather skill calls
wttr.in, but the NemoClawweatheregress preset only allowed Open-Meteo and weather.gov. Under deny-by-default egress, the current host was blocked. This change closes the NemoClaw-owned policy gap while keeping access read-only and least-privileged.Related Issue
Addresses the NemoClaw-owned
wttr.inegress portion of #1417.This PR does not claim to change or fully validate OpenClaw skill readiness, browser fallback, pairing behavior, or the entire upstream weather-skill workflow.
Changes
wttr.in:443for GET and HEAD across its location-path namespace.wttr.is; the reviewed OpenClaw 2026.5.27 pin does not require it, and a future pin change must supply new runtime evidence before widening policy.loadAgent("openclaw").expectedVersion: any OpenClaw pin change fails until maintainers revalidate the bundled skill's egress and update the reviewed contract.wttr.inpolicy shape intest/weather-policy.test.ts.wttr.in/:helpcurl, atomically retain the fetched body, and independently validate that proof from the host test.Type of Change
Quality Gates
/**path risk is accepted because wttr location identifiers are themselves paths and no stable narrower prefix exists; policy remains limited to one public host, GET/HEAD only, with no credentials sent.wttr.isexclusion is intentional least privilege; there is no reviewed evidence that the pinned runtime needs it.Verification
npx vitest run test/weather-policy.test.tspassed (2/2).npm run typecheck:clipassed.npm run source-shape:checkpassed with zero source-shape tests.npm run docscompleted with zero errors and two pre-existing warnings.network-policyandcommon-egress-agent: https://github.com/NVIDIA/NemoClaw/actions/runs/28424896912Signed-off-by: Hung Le hple@nvidia.com