fix(onboard): keep tier-default Brave on reuse in the fresh-selection branch - #10443
fix(onboard): keep tier-default Brave on reuse in the fresh-selection branch#10443harjothkhara wants to merge 1 commit into
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe change preserves tier-provided ChangesPolicy tier preset handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change preserves tier-default policy presets during sandbox reuse while retaining existing pruning behavior for non-default cases; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
466c94f to
02ddd80
Compare
… branch
Re-onboarding a Balanced sandbox and choosing reuse pruned the `brave`
preset and narrowed `api.search.brave.com` egress whenever web search was
declined, then failed finalization with an OpenClaw runtime identity error.
into the resume branch of `setupPoliciesWithSelectionInner`. Reuse normally
seeds presets through that guarded branch via `seedReusedSandboxPolicyPresets`,
which is skipped for a non-interactive env override (NEMOCLAW_POLICY_PRESETS
or an explicit NEMOCLAW_POLICY_MODE) and for a sandbox whose prior policy step
never set `policyPresetsFinalized`. Those runs fall into the fresh-selection
branch instead. The issue's own log line ("Applying policy presets: npm,
pypi, huggingface, brew, openclaw-pricing", no "Preserving previously-applied
presets" note) matches only this branch's non-interactive log format — the
resume branch logs "Reapplying" — so the reported failure is consistent with
an automated non-interactive validation run hitting this code path, not the
interactively-narrated repro steps taking the already-guarded resume branch.
The fresh-selection branch has five `pruneUnavailablePresets` call sites.
Three passed no `tierName` and are fixed here: the preservation prune that
seeds both the non-interactive carry-forward and the interactive pre-checked
set, the NEMOCLAW_POLICY_MODE=skip retention prune, and the non-interactive
env-preset prune. Two are deliberately left alone: the suggestion-list prune
(NVIDIA#6856 intentionally kept fresh-onboard suggestions conservative), and the
final interactive prune, which already passes `preserveExplicitWebSearch`
and so never reaches the tier-blind fallback this bug depends on. Without
`tierName`, `isStaleBuiltinWebSearchPolicyPreset` cannot see that `brave` is
a Balanced tier egress default and drops it as an ordinary web-search
leftover, so the reapplied set no longer matches the applied set and a
policy push narrows live egress.
Thread the recorded tier into the three affected call sites, hoisting
`recordedTierName` above the first of them. The recorded tier is used
rather than a freshly resolved one so a genuinely fresh onboard keeps its
conservative suggestion behaviour: with no recorded tier the value is
null and the pruner still fails safe — verified by mutation (widening this
to the resolved tier breaks the pre-existing null-recorded-tier regression
test for NVIDIA#6844).
The same tier-blind gap exists in the sandbox rebuild target-preset filter
(src/lib/actions/sandbox/rebuild-backup-phase.ts:91-94, also calling
isStaleBuiltinWebSearchPolicyPreset with no tierName despite the caller
already having sandboxEntry.policyTier available one level up) — same
predicate, different command, pre-dating this fix. Not fixed here since
`nemoclaw sandbox rebuild` is a separate entry point from onboard reuse;
flagging for a maintainer or follow-up issue.
A first review (the automated PR Review Advisor) found two more real
issues, fixed here:
- Code/Reduction: the interactive and non-interactive fresh-selection
regression tests differed only in `nonInteractive`. Merged them into one
`it.each` case over both values, matching this file's existing
parameterized-test convention, with no loss of per-branch coverage.
- Documentation: docs/reference/network-policies.mdx said NemoClaw
"removes stale web search presets during resume reconciliation... when
you switch providers or disable web search" with no qualification. That
statement is now incomplete for a reused sandbox with a recorded
Balanced or Open tier, where `brave` (never `tavily` — confirmed against
nemoclaw-blueprint/policies/tiers.yaml, only `brave` is a literal tier
preset default) survives as a tier default rather than being pruned as a
leftover. Qualified the statement and named the cases that still prune
(tavily is never a tier default; Restricted has no web-search default).
Refs NVIDIA#10404
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: harjoth <harjoth.khara@gmail.com>
02ddd80 to
08955e9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto
The conflict was mechanical: #10457 also fixes #10404 and is broader — it covers the One difference is worth a deliberate call, because it is not just scope. This PR passes the recorded tier ( |
|
PR Review Advisor finished for commit |
|
Maintainer decision: superseded by #10457. We are selecting #10457 because it covers both the existing-sandbox reuse path and the rebuild path, includes the field-regression coverage needed for #10404, and is being qualified against the current runtime-identity baseline. The #10457 implementation is independent; no code or tests were transferred from this PR. Thank you for the focused investigation and fix. |
) <!-- markdownlint-disable MD041 --> ## Summary Re-onboarding a ready Balanced or Open sandbox with web search disabled incorrectly removed the already-applied `brave` tier preset, narrowing `api.search.brave.com` egress. The same tier-blind pruning also affected rebuild/restore. This change preserves a built-in web-search preset only when the canonical recorded tier contains it and the preset applies to the active agent. Restricted and unknown tiers, unsupported agents, custom ownership, and provider presets outside the recorded tier continue through normal stale-provider reconciliation. ## Related issues and competing PR Fixes #10404. Supersedes #10443. This implementation was authored independently; no code or tests were transferred from #10443. ## Changes - Pass the resolved policy tier through reuse, non-interactive, skip-mode, and rebuild pruning paths. - Keep one shared stale built-in web-search predicate, using canonical tier membership and existing agent applicability. - Preserve custom preset ownership and deny-by-default behavior for unknown tiers and unsupported agents. - Cover Balanced and Open retention plus Restricted, unknown-tier, provider-switch, unsupported-agent, custom-policy, and rebuild boundaries. - Document that disabling web search or selecting Tavily does not remove an applied `brave` preset owned by a recorded Balanced or Open tier. - Extend the credential-bearing `brave-search` live E2E target to reproduce the complete field transition: - onboard a Brave-enabled Balanced sandbox; - re-onboard the same ready sandbox non-interactively with web search disabled and reuse selected; - require onboarding exit 0 and a ready runtime after the identity check; - require OpenClaw config to record web search disabled; - require live policy to retain `api.search.brave.com`; and - require an in-sandbox request to reach the Brave endpoint. - Register the Brave live test and helper with their focused fast-test owners in the mock/live parity manifest. ### Deliberately unchanged - Fresh suggested onboarding still omits `brave` unless web search is selected. This change retains existing tier egress; it does not add new egress. - Custom preset ownership remains authoritative. - Operators can remove `brave` explicitly or select a tier that does not contain it. ## Type of Change - [x] Code change with doc updates - [ ] Doc only ## Quality Gates - [x] Tests added or updated for changed behavior - [x] Sensitive paths changed (network policy, onboarding, rebuild, live E2E) - [ ] Sensitive-path maintainer approval recorded - [x] Exact-SHA required CI is green - [x] PR Review Advisor has no blockers - [x] CodeRabbit has no unresolved threads - [ ] Full manual PR E2E is green for the latest SHA - [x] No secrets, API keys, or credentials committed ## Verification Candidate SHA: `d55281c2afc66a6e4e1f510c9066431802ae091c` PR base / trusted workflow SHA: `83fb5d93f9c4b4a4f6a32bef8024eb4e91271ea0` - [CI / Pull Request run 33216222212](https://github.com/NVIDIA/NemoClaw/actions/runs/33216222212) passed on the candidate SHA, including all 12 test shards, aggregate coverage, and mock/live parity. - [PR Review Advisor run 33216220335](https://github.com/NVIDIA/NemoClaw/actions/runs/33216220335) passed on the candidate SHA. All nine specialist artifacts report no required change. - All current PR checks are complete: 51 pass, 17 intentionally skipped, 0 pending, 0 failing. - CodeRabbit reports 0 unresolved review threads. - `npm run lint` passed after the final `main` merge. - `npm run typecheck:cli` passed after the final `main` merge and again in the pre-push hook. - Focused reuse/rebuild/tier/preset/E2E-support suites passed 235/235 tests after the final `main` merge. - `npx tsx scripts/checks/e2e-mock-parity.mts --base origin/main --head HEAD` passed. - All added commits are SSH-verified and DCO-signed off. ## Required field evidence before approval The full manual NVIDIA-owned PR E2E is intentionally still pending. Its trusted workflow prerequisite is the stack #10566 then #10565; both must land on `main` before dispatch so the run uses the reviewed OpenShell development MUSL sandbox artifact path. The successful run must remain bound to the candidate SHA, PR base SHA, source repository, and trusted workflow SHA, and must prove: - the second fresh non-interactive re-onboard reuses the same sandbox with `NEMOCLAW_RECREATE_SANDBOX=0`; - the re-onboard exits 0 and the source CLI status command exits 0 with the sandbox ready; - OpenClaw config records `tools.web.search.enabled == false`; - the live policy retains `api.search.brave.com`; and - an in-sandbox request reaches that endpoint with a non-`000` HTTP status. Issue #10404 must remain open and this PR must remain unapproved until that field evidence verifies the separate runtime-identity exit failure. --- Signed-off-by: Hai Nguyen <haingu@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Summary
Re-onboarding a Balanced-tier sandbox and choosing reuse pruned the
bravepreset and narrowedapi.search.brave.comegress whenever web search was declined, then failed finalization with an OpenClaw runtime identity error.#6856fixed this exact preservation contract for#6844, but only threaded the recorded policy tier into the resume branch of the shared policy-preset selection function — a fresh-selection branch that reuse can also reach (non-interactive env overrides, or a sandbox whose prior policy step never finalized) still pruned tier-default presets as if they were ordinary web-search leftovers. This threads the recorded tier into that branch's three affected prune sites too.Related Issue
Fixes #10404
Changes
src/lib/onboard/policy-selection.ts:setupPoliciesWithSelectionInner's fresh-selection branch has fivepruneUnavailablePresetscall sites. Three passed notierNameand are fixed here: the preservation prune (feeds both the non-interactive carry-forward and the interactive pre-checked set), theNEMOCLAW_POLICY_MODE=skipretention prune, and the non-interactive env-preset prune. Two are deliberately left alone: the suggestion-list prune (#6856intentionally kept fresh-onboard suggestions conservative — unrelated to this reuse contract), and the final interactive prune, which already passespreserveExplicitWebSearchand never reaches the tier-blind fallback this bug depends on.recordedTierNameis hoisted above the first affected call site so it's available there. The recorded tier is used rather than a freshly resolved one so a genuinely fresh onboard keeps its existing conservative suggestion behavior: with no recorded tier the value isnull, andnullstill fails safe (prunes) — verified by mutation: widening this to the resolved tier breaks the pre-existing null-recorded-tier regression test for#6844.test/runtime/policy/policy-tiers-onboard.test.ts: 7 new tests covering the repro (interactive and non-interactive fresh-selection branches with a recorded Balanced tier), both reuse-seeding-bypass triggers (NEMOCLAW_POLICY_MODE=skip,NEMOCLAW_POLICY_PRESETSin suggested mode), and three boundary cases that must still prune (a recorded Restricted tier,tavily— not a Balanced default, andwebSearchSupported: false). Retitled the pre-existing#6844fresh-branch test to"...with no recorded tier"— its assertions are unchanged, but its title read as a contradiction of the new recorded-tier tests sitting next to it once both exist.Why the reported failure hits the fresh branch, not the already-guarded resume branch. The issue's repro steps narrate an interactive reuse, which for a normally-completed prior onboard would route through the resume branch
#6856already fixed. But the issue's own log shows"Applying policy presets: npm, pypi, huggingface, brew, openclaw-pricing"with no"Preserving previously-applied presets"note — that message format matches only the fresh-selection branch's non-interactive log line (the resume branch logs"Reapplying"). So the reported failure is consistent with an automated non-interactive validation run (the issue carriesNV QA/UATlabels) that hit a reuse-seeding bypass, not with the narrated interactive steps taking a still-broken resume branch.A related, out-of-scope observation, disclosed rather than folded in: the same tier-blind gap exists in the sandbox rebuild target-preset filter (
src/lib/actions/sandbox/rebuild-backup-phase.ts:91-94, also callingisStaleBuiltinWebSearchPolicyPresetwith notierName, despite the caller one level up already havingsandboxEntry.policyTieravailable). Same predicate, different command (nemoclaw sandbox rebuild, not onboard reuse), pre-dating this fix. Not fixed here since it's a separate entry point this issue doesn't report against; flagging for a maintainer or a follow-up issue.A first correction, made after the automated PR Review Advisor's first pass:
nonInteractive. Merged them into oneit.eachcase over both values, matching this file's existing parameterized-test convention.docs/reference/network-policies.mdxsaid NemoClaw "removes stale web search presets during resume reconciliation... when you switch providers or disable web search" with no qualification — now incomplete for a reused sandbox with a recorded Balanced or Open tier. Qualified the statement: a tier-default preset (brave— confirmed againstnemoclaw-blueprint/policies/tiers.yamlthat onlybrave, nevertavily, is a literal tier preset default) now survives reuse instead of being pruned as a leftover, and named the cases that still prune (tavilyis never a tier default; Restricted has no web-search default).Type of Change
Quality Gates
rebuild-backup-phase.tssibling gap) are all addressed above. A maintainer pass is still expected before merge; this checkbox records contributor-side review completion, not a waiver of maintainer review.DGX Station Hardware Evidence
Not applicable — this PR does not change
scripts/prepare-dgx-station-host.sh. (The issue was reproduced in automated release validation on WSL x86, Ubuntu 24 GPU, and DGX Station, but this fix targets shared onboard/policy-preset logic common to all of them, not a DGX-specific path.)Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpx vitest run --project integration test/runtime/policy/policy-tiers-onboard.test.ts— 100 tests passed (7 new for [Linux][Onboard] Brave Balanced-tier preset is removed on re-onboard reuse when web search is disabled #10404, 4 of which were confirmed RED against unmodifiedorigin/mainand each new/changed production line was independently confirmed load-bearing by per-line mutation).npx vitest run --project cli src/lib/onboard/policy-resume-selection.test.ts— 19 tests passed, confirming no regression in the sibling resume branch#6856already guards.npm run typecheck:cliandnpm run checks:repositoryboth clean.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only) — 0 errors; the 2 warnings reported (missingFERN_TOKENfor a redirects check, and a pre-existing brand accent-color contrast note) are unrelated to this content change and reproduce identically with the doc edit reverted.Signed-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit
Bug Fixes
Tests