Repository navigation
refactor(onboard): centralize review recovery wiring - #8906
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
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. |
📝 WalkthroughWalkthroughThe change moves default onboarding option and provider review dependency wiring into dedicated modules. It adds reserved sandbox-name validation for resumed sessions, removes obsolete imports, preserves sandbox creation arguments, and lowers the onboarding fan-out budget. ChangesOnboarding wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Sensitive-path security review (exact head bb7e037): PASS with no findings. Reviewed the complete diff across all nine required categories: (1) input validation and trust boundaries—existing option, name, and recovery validation is unchanged; (2) credentials and secrets—Ollama token retrieval/persistence/probing is only rewired to the same functions, with no new logging or storage; (3) network exposure and SSRF—the same proxy topology and probe functions are invoked with unchanged arguments; (4) command execution and injection—no commands or interpolation changed; (5) permissions and privilege boundaries—no permission behavior changed; (6) sandbox isolation—the same onboarding and resumed-sandbox paths remain; (7) injection classes—no parser, shell, URL, HTML, SQL, or template sink changed; (8) dependencies and supply chain—no dependency or artifact changed; (9) data exposure and logging—the same console.error and process.exit callbacks preserve existing messages and failure behavior. Import-time dependencies were also checked: the moved modules were already loaded by onboard.ts, and the new adapters add no side effects or public configuration surface. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/lib/onboard.ts`:
- Around line 3732-3738: Add public-entrypoint tests covering fresh, resumed,
and provider-review onboarding flows. In src/lib/onboard.ts:3732-3738, verify
public flows use resolveDefaultRunEntryOptions; in src/lib/onboard.ts:4074-4077,
verify provider-review uses createDefaultProviderReviewDeps and preserves
expected failure behavior. Add corresponding boundary coverage around
setup-inference.ts:70-86 and entry-options.ts:104-118, including default
dependency behavior and failures without changing the lower-level resolver
tests.
🪄 Autofix
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: e16c0070-9c32-4308-9be7-7d9fbde7fbf6
📒 Files selected for processing (4)
ci/source-architecture-budget.jsonsrc/lib/onboard.tssrc/lib/onboard/entry-options.tssrc/lib/onboard/setup-inference.ts
| const entryOptions = onboardEntryOptions.resolveDefaultRunEntryOptions( | ||
| opts, | ||
| onboardSession.loadSession()?.status ?? null, | ||
| validateName, | ||
| ); | ||
| const { fresh, nonInteractive, cannotPrompt, resume } = entryOptions; | ||
| const { requestedFromDockerfile, requestedSandboxName } = entryOptions; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/lib/onboard.ts --match runOnboard --view expanded
rg -n -C 3 \
--glob '*.{test,spec}.ts' \
'runOnboard|resolveDefaultRunEntryOptions|createDefaultProviderReviewDeps|resolveOnboardRunEntryOptions|createProviderReviewDeps' \
.
rg -n -C 3 \
'resolveDefaultRunEntryOptions|createDefaultProviderReviewDeps|resolveOnboardRunEntryOptions|createProviderReviewDeps' \
srcRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant files ---'
git ls-files \
'src/lib/onboard*' \
'test/*onboard*' \
'src/lib/actions/*onboard*' \
'src/commands/*onboard*' |
rg '(entry-options|setup-inference|onboard(\.test)?|lifecycle|resume|inference|provider)' |
head -200
printf '%s\n' '--- exact symbol references in source and tests ---'
rg -n --glob '*.ts' \
'resolveDefaultRunEntryOptions|createDefaultProviderReviewDeps|resolveOnboardRunEntryOptions|createProviderReviewDeps' \
src test
printf '%s\n' '--- runOnboard definitions and calls ---'
rg -n -C 8 --glob '*.ts' \
'function runOnboard|const runOnboard|wrapOnboard\(runOnboard|createDefaultProviderReviewDeps' \
src/lib/onboard.ts src/lib/onboard test src/commands
printf '%s\n' '--- focused test filenames ---'
git ls-files '*test.ts' |
while read -r f; do
if rg -q 'resolveDefaultRunEntryOptions|createDefaultProviderReviewDeps|resolveOnboardRunEntryOptions|createProviderReviewDeps' "$f"; then
printf '%s\n' "$f"
fi
doneRepository: NVIDIA/NemoClaw
Length of output: 9447
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- entry-options implementation ---'
cat -n src/lib/onboard/entry-options.ts | sed -n '1,180p'
printf '%s\n' '--- setup-inference implementation ---'
cat -n src/lib/onboard/setup-inference.ts | sed -n '1,130p'
printf '%s\n' '--- runOnboard entry wiring ---'
cat -n src/lib/onboard.ts | sed -n '3690,3765p'
cat -n src/lib/onboard.ts | sed -n '4035,4105p'
printf '%s\n' '--- exact symbol references ---'
rg -n --glob '*.ts' \
'resolveDefaultRunEntryOptions|createDefaultProviderReviewDeps|resolveOnboardRunEntryOptions|createProviderReviewDeps' \
src test || true
printf '%s\n' '--- candidate direct onboarding tests ---'
for f in \
src/lib/onboard/entry-options.test.ts \
src/lib/onboard/setup-inference.test.ts \
src/lib/onboard/command.test.ts \
src/lib/onboard/command-support.test.ts \
src/commands/onboard.test.ts \
src/lib/actions/onboard.test.ts \
test/onboard-lifecycle.test.ts
do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
rg -n -C 5 \
'fresh|resume|provider|inference|entry|factory|dependency|runOnboard|onboard' \
"$f" | head -240 || true
fi
doneRepository: NVIDIA/NemoClaw
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- lifecycle test structure and assertions ---'
rg -n -C 4 \
'describe|it\(|runOnboardEntrypoint|resume|fresh|provider|inference|sandboxName|mode|calls|events' \
test/onboard-lifecycle.test.ts | head -320 || true
printf '%s\n' '--- entry-options test imports and test list ---'
sed -n '1,330p' src/lib/onboard/entry-options.test.ts |
rg -n '^(import|describe| it| it\.each| \}\)|export)|resolveDefault|resolveOnboard' || true
printf '%s\n' '--- setup-inference test imports and test list ---'
cat -n src/lib/onboard/setup-inference.test.ts | sed -n '1,190p'
printf '%s\n' '--- provider-review recovery test references ---'
rg -n -C 8 \
'createProviderReviewDeps|providerReviewDeps|prepareLocalProviderForInference|checkpointSandboxIdentity' \
src/lib/onboard/machine/handlers/provider-review-recovery.test.ts || true
printf '%s\n' '--- targeted diff summary ---'
git diff --stat -- src/lib/onboard.ts src/lib/onboard/entry-options.ts src/lib/onboard/setup-inference.ts \
src/lib/onboard/entry-options.test.ts src/lib/onboard/setup-inference.test.ts || trueRepository: NVIDIA/NemoClaw
Length of output: 27071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
symbols = (
"resolveDefaultRunEntryOptions",
"createDefaultProviderReviewDeps",
"resolveOnboardRunEntryOptions",
"createProviderReviewDeps",
)
for symbol in symbols:
refs = []
for path in Path("src").rglob("*.ts"):
text = path.read_text(encoding="utf-8")
for line_no, line in enumerate(text.splitlines(), 1):
if re.search(rf"\b{re.escape(symbol)}\b", line):
refs.append(f"{path}:{line_no}")
print(f"{symbol}:")
for ref in refs:
print(f" {ref}")
if not refs:
print(" <none>")
PYRepository: NVIDIA/NemoClaw
Length of output: 1098
Add public-entrypoint coverage for the default onboarding factories.
Existing tests cover lower-level resolvers and lifecycle behavior, but they do not prove that public fresh, resumed, and provider-review flows use resolveDefaultRunEntryOptions and createDefaultProviderReviewDeps. Add boundary assertions for their default dependencies and failure behavior.
📍 Affects 3 files
src/lib/onboard.ts#L3732-L3738(this comment)src/lib/onboard/entry-options.ts#L104-L118src/lib/onboard/setup-inference.ts#L70-L86src/lib/onboard.ts#L4074-L4077
🤖 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.ts` around lines 3732 - 3738, Add public-entrypoint tests
covering fresh, resumed, and provider-review onboarding flows. In
src/lib/onboard.ts:3732-3738, verify public flows use
resolveDefaultRunEntryOptions; in src/lib/onboard.ts:4074-4077, verify
provider-review uses createDefaultProviderReviewDeps and preserves expected
failure behavior. Add corresponding boundary coverage around
setup-inference.ts:70-86 and entry-options.ts:104-118, including default
dependency behavior and failures without changing the lower-level resolver
tests.
Source: Path instructions
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
Final-head security receipt for dcb13c3: PASS with no findings. The signed merge only brings the published branch to current main; its tree is byte-identical to the already reviewed rebased head a4e2cf3, and the PR delta remains the same four-file wiring refactor reviewed across all nine security categories above. |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: Manual-only E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
wscurran
added
the
refactor
wscurran
added
area: onboarding
Summary
This refactor removes four formatter exemptions that survived the line-budget work in #8724. It moves the default entry-option, reserved-name, and Ollama provider-review dependency wiring into the existing modules that own those contracts.
The resume, sandbox-name validation, provider-review recovery, process-exit, logging, and Ollama proxy behaviors are unchanged.
src/lib/onboard.tsbecomes two lines smaller, and its measured dependency fan-out falls from 211 to 210.Changes
onboard/entry-options.ts.onboard/setup-inference.ts.onboard.tscall sites with formatted calls and remove four growth-guardrail formatter suppressions.The current consumers are the onboarding entry, truthful review recovery, and resumed-session reserved-name paths. Existing focused tests protect those contracts; no new abstraction or configuration surface is introduced.
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
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 unavailablenpm run typecheck:cli,npm run build:cli, source architecture, source shape, and test-size checks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this focused internal dependency-wiring refactor.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com