Repository navigation
fix(onboard): preserve tier-default brave/tavily on re-onboard reuse - #6856
Conversation
On the re-onboard reuse path, a Balanced-tier default preset that doubles as a built-in web-search provider preset (`brave`, `tavily`) was silently dropped even when the policy tier and web-search choice were unchanged. The other tier defaults (`npm`, `pypi`, `huggingface`, `brew`) persisted, so only the search-provider egress (e.g. `api.search.brave.com`) was narrowed out of the reapplied set. Root cause: `isStaleBuiltinWebSearchPolicyPreset` treats `brave`/`tavily` as a stale web-search leftover whenever no matching web-search provider is configured, without distinguishing the case where the same preset is a default egress preset of the tier being applied. `preparePolicyPresetResume- Selection` pruned it, which forced a reconcile and a reapply that dropped it. Fix: add a `tierDefaultPresetNames` exemption to the staleness predicate and thread the recorded tier's defaults through the resume-selection prune and the reuse reapply prune. A preset that is a default of the applied tier is a tier egress default, not a stale web-search leftover, so it is preserved regardless of the web-search provider choice. The Restricted tier lists no such default, so a genuinely stale `brave`/`tavily` there still prunes, and provider-switch replacement (brave -> tavily) still adds the active provider. Fixes #6844 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
|
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOnboarding resume now passes tier and agent context into built-in web-search preset reconciliation. Tier-provided defaults are preserved, while unrelated presets remain eligible for pruning. Resume and reapply tests cover balanced, restricted, and alternate-provider scenarios. ChangesTier-default preset preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant ResumeSelection
participant PresetReconciliation
participant PolicySync
Onboarding->>ResumeSelection: prepare resume preset selection with tier and agent
ResumeSelection->>PresetReconciliation: evaluate built-in web-search preset provenance
PresetReconciliation-->>ResumeSelection: preserve tier defaults or mark stale
Onboarding->>PolicySync: reapply selected presets with recorded tier
PolicySync-->>Onboarding: retain tier defaults and prune stale presets
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
…tiers resolveTierPresets throws on an unknown tier, and the recorded tier on the reuse/resume path can be a non-canonical value (e.g. a display label or an authoritative rebuild tier not yet registered). Guard both new tier-default lookups on getTier (which returns null for unknown tiers) so a non-tier reuse path keeps its prior behavior instead of throwing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
… classifier Replace the parallel `tierDefaultPresetNames` set threaded through the reuse prune sites with the existing `classifyPresetProvenance` classifier: a preset whose provenance is `source: "tier"` (a default of the applied tier) is not a stale web-search leftover. Pruning and the policy-list display now share one notion of why a preset is present, instead of a second, parallel one. - Drops the injected `tiers` dep from preparePolicyPresetResumeSelection and the getTier/resolveTierPresets guard in setupPoliciesWithSelection; the classifier fails safe on an unknown/non-canonical tier (getTier -> null -> not "tier"). - Exemption stays scoped exactly to the applied tier: Restricted lists no such default, so a genuinely stale brave/tavily there is still pruned. Added a boundary test that a non-tier-default tavily on Balanced is still pruned. - Fresh suggested onboard is intentionally unchanged: omitting brave egress without web-search is conservative-egress by design, not part of this bug. Design annotation: - invalidState: on reuse, an already-applied tier-default egress preset (brave/tavily on Balanced/Open) is pruned as a stale web-search leftover and its egress narrowed, on an unchanged tier. - sourceBoundary: isStaleBuiltinWebSearchPolicyPreset, the single predicate all reuse/resume prune sites share. - whyNotSourceFix: preset provenance is intentionally not persisted (preset-provenance.ts is display-only by design); reuse its classifier rather than reverse that decision. - regressionTest: policy-resume-selection.test.ts — preserve on Balanced, still-prune on Restricted, keep-with-switched-provider, prune-non-tier-default. - removalCondition: if preset provenance becomes persisted per-preset, replace the current-tier inference with the stored source. Verified end-to-end on our DGX aarch64 test host (GPU) against a local Ollama sandbox on main: reuse re-onboard preserves brave; the x86_64 path is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
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)
src/lib/onboard/policy-selection.ts (1)
302-302: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
recordedTierNameinto the remaining stale-pruning checks
src/lib/onboard/policy-selection.ts:83, 138-152, 203-205still prunes without the active tier.appliedForPreservationruns beforerecordedTierNameis read,computeSetupPresetSuggestions()still omitstierNamein its stale-web-search filter, and the non-authoritativepruneUnavailablePresets(chosen, ...)call does too. That can still strip a recorded Balancedbravebefore selection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/onboard/policy-selection.ts` at line 302, Update the stale-pruning flow in policy selection to consistently pass the active recordedTierName: read it before constructing appliedForPreservation, provide it to pruneUnavailablePresets(applied), include tierName in computeSetupPresetSuggestions()’s stale-web-search filtering, and pass it to the non-authoritative pruneUnavailablePresets(chosen, ...) call. Preserve recorded Balanced brave entries during selection.Source: Path instructions
🤖 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 `@src/lib/onboard/policy-selection.ts`:
- Line 302: Update the stale-pruning flow in policy selection to consistently
pass the active recordedTierName: read it before constructing
appliedForPreservation, provide it to pruneUnavailablePresets(applied), include
tierName in computeSetupPresetSuggestions()’s stale-web-search filtering, and
pass it to the non-authoritative pruneUnavailablePresets(chosen, ...) call.
Preserve recorded Balanced brave entries during selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 78877dcc-335e-4346-9f0b-f507dddcbc05
📒 Files selected for processing (4)
src/lib/onboard/policy-preset-reconciliation.tssrc/lib/onboard/policy-resume-selection.test.tssrc/lib/onboard/policy-resume-selection.tssrc/lib/onboard/policy-selection.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
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/policy-tiers-onboard.test.ts`:
- Around line 487-491: Update the runPolicySetup fixture in the affected test to
explicitly set tierName to a valid non-Balanced tier such as "restricted", while
preserving recordedPolicyTier as "balanced".
🪄 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: 4c99d9d4-7ba4-4440-aae2-a984903f12c4
📒 Files selected for processing (1)
test/policy-tiers-onboard.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
## Summary Add v0.0.83 release notes to `docs/about/release-notes.mdx` for pre-tag release prep. ## Source Summary - #6773 -> `docs/about/release-notes.mdx`: Shared inference route changes are explicit and fail-safe; status shows recorded route, live route, and drift. - #6875 -> `docs/about/release-notes.mdx`: DGX Station GB300 express setup restored; vLLM storage preflight narrowed. - #6770 -> `docs/about/release-notes.mdx`: Risky Spark vLLM server warning during onboarding. - #6856 -> `docs/about/release-notes.mdx`: Re-onboard reuse preserves tier-default brave/tavily presets. - #6867 -> `docs/about/release-notes.mdx`: Unreachable custom endpoint routed through transport-recovery path. - #6860 -> `docs/about/release-notes.mdx`: Rebuild preflight uses model-aware token field for o-series/GPT-5. - #6845 -> `docs/about/release-notes.mdx`: Corporate CA anchored for image build TLS. - #6833 -> `docs/about/release-notes.mdx`: SSH ControlMaster-delegated forwards recognized in fallback. - #6837 -> `docs/about/release-notes.mdx`: Hermes light skin writes via stdin on macOS. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [x] Tests not applicable — justification: doc-only release notes - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed - [ ] Non-success, skipped, or missing CI check accepted by maintainer ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub - [x] Normal pre-commit, commit-msg, and pre-push hooks passed - [x] `npm run docs` passes with 0 errors Signed-off-by: Jessica Yaunches <jyaunches@nvidia.com> Signed-off-by: Jessica Yaunches <jyaunches@nvidia.com>
… 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. NVIDIA#6856 fixed this contract for NVIDIA#6844, but only threaded the recorded tier 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>
… 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>
Summary
On the re-onboard reuse path, the Balanced-tier default preset
brave(Brave Search API access) was silently dropped even when the policy tier (Balanced) and the web-search choice (No web search) were unchanged. The other four Balanced defaults (npm,pypi,huggingface,brew) persisted — onlybravewas narrowed out of the reapplied set, removing its egress (api.search.brave.com).Fixes #6844.
Root cause
isStaleBuiltinWebSearchPolicyPresettreatsbrave/tavilyas a stale web-search leftover whenever no matching web-search provider is configured. It did not account for the fact that the same preset is also a default egress preset of the tier being applied. On reuse,preparePolicyPresetResumeSelectionprunedbraveunder this rule, flippingrecordedPolicyPresetsNeedReconcileand driving a reapply whose set omittedbrave.Fix
A preset that is a default of the applied tier is a tier egress default, not a stale web-search leftover. The exemption is expressed through the existing
classifyPresetProvenanceclassifier (source: "tier"), so pruning and thepolicy-listdisplay share a single notion of why a preset is present rather than a second, parallel one. It is threaded through both reuse/resume prune sites (preparePolicyPresetResumeSelectionandcreateUnavailablePolicyPresetPruner).Scoped to the reuse/resume path. Fresh-onboard suggestion behaviour is intentionally unchanged: a fresh suggested onboard omits
braveegress unless web-search is chosen (conservative egress by design — seeonboard-policy-suggestions.test.ts), which is not part of this reuse regression.Design annotation
brave/tavilyon Balanced/Open) is pruned as a stale web-search leftover and its egress narrowed, on an unchanged tier.isStaleBuiltinWebSearchPolicyPreset— the single predicate all reuse/resume prune sites share.preset-provenance.tsis display-only by design); we reuse its classifier rather than reverse that decision with a persisted-state refactor.policy-resume-selection.test.ts— preserve on Balanced, still-prune on Restricted, keep-with-switched-provider, prune-non-tier-default.Behaviour preserved (boundaries)
brave/tavilydefault → a genuinely stale preset there is still pruned.tavilyon Balanced with no matching provider) is still pruned — the exemption is scoped to real tier defaults, not "anybrave/tavily".getTier→ null → not"tier"→ not exempt → prior behaviour), so the authoritative-rebuild-tier-pending case does not throw.Testing
policy-resume-selection.test.tscovers the full contract above; the broader onboard policy/tier/preset suites pass (213 tests across 15 files);build:cli+typecheckgreen.main, onboarding a local Ollama sandbox (loopback route, so onboarding is not blocked by the endpoint SSRF preflight):npm, pypi, huggingface, brew→Removed preset: brave→ brave inactive.npm, pypi, huggingface, brew, brave→ brave stays active ([from balanced tier]), no egress narrowing.Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Summary by CodeRabbit