fix(onboard): resolve moved Ollama lazy-load path - #4140
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 (2)
📝 WalkthroughWalkthroughTwo files were updated to fix a regression where the Ollama tools-capability gate lost access to onboard module state after a directory refactor. The relative require paths are corrected from ChangesOllama tools-capability gate onboard access restoration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Suggested labels
Suggested reviewers
Poem
🚥 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 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 |
|
✨ Thanks for submitting this detailed PR about resolving the moved onboard lazy-load path inside the Ollama tools-capability gate. This proposes a fix for the regression caused by the file move in #3195 and adds regression coverage for onboard-backed non-interactive and prompt-helper paths. Related open PRs: Related open issues: |
|
Thanks for identifying the moved-path regression and establishing the initial regression-test direction here. #5898 is a fresh replacement because this branch now conflicts with main and its unsigned published commit cannot be rewritten under the repository's current verification policy. The replacement preserves the issue intent while removing the Ollama proxy's onboarding dependency entirely through an explicit interaction boundary. Please continue review on #5898. |
<!-- markdownlint-disable MD041 --> ## Summary This PR removes the Ollama tools-capability gate's hidden dependency on the onboarding entrypoint. Onboarding now supplies its interaction state explicitly, fixing `--non-interactive` and `--yes` behavior without a circular lazy import. ## Related Issue Fixes #4139. Replacement for #4140, which identified the moved-path regression and established the initial test direction but cannot be updated without rewriting its unsigned, conflicting history. ## Changes - Add a typed `OllamaToolCapabilityInteraction` boundary for non-interactive, auto-yes, and confirmation decisions. - Pass onboarding's in-memory interaction state into `prepareOllamaModel` explicitly. - Keep environment and credential-prompt defaults for standalone capability checks. - Remove all three lazy onboarding lookups and their swallowed exceptions from the Ollama proxy. - Add focused regressions for caller-supplied non-interactive, auto-yes, and interactive confirmation behavior. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [x] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the proxy and onboarding-selection suites cover the surrounding pull, validation, and selection flows. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this restores the already-documented `--non-interactive`, `--yes`, and Ollama tools-gate semantics without changing commands or output contracts. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: focused diff review verified that only interaction-state ownership changes; capability validation, override requirements, and credential handling are unchanged. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Ollama model tool-capability checks to consistently respect interaction modes (non-interactive, auto-yes, and explicit yes/no confirmation), skipping prompts when appropriate. * Updated onboarding flow so confirmation behavior is delegated cleanly when interactive input is required. * **Tests** * Expanded unit coverage to ensure non-interactive/auto-yes short-circuit correctly and that interactive confirmation receives the expected question and default choice. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
I am glad this issue is fixed. @cv Feel free to close or amend this PR. |
|
Closing as superseded by #5898, which merged as There is no remaining gap on current
That explicit interaction boundary supersedes this PR's path-only change in Thank you @fallintoplace for the original moved-path diagnosis and regression-test direction. #5898 explicitly preserved and credited that contribution, and your follow-up here confirmed that this PR may be closed. |
<!-- markdownlint-disable MD041 --> ## Summary This PR removes the Ollama tools-capability gate's hidden dependency on the onboarding entrypoint. Onboarding now supplies its interaction state explicitly, fixing `--non-interactive` and `--yes` behavior without a circular lazy import. ## Related Issue Fixes NVIDIA#4139. Replacement for NVIDIA#4140, which identified the moved-path regression and established the initial test direction but cannot be updated without rewriting its unsigned, conflicting history. ## Changes - Add a typed `OllamaToolCapabilityInteraction` boundary for non-interactive, auto-yes, and confirmation decisions. - Pass onboarding's in-memory interaction state into `prepareOllamaModel` explicitly. - Keep environment and credential-prompt defaults for standalone capability checks. - Remove all three lazy onboarding lookups and their swallowed exceptions from the Ollama proxy. - Add focused regressions for caller-supplied non-interactive, auto-yes, and interactive confirmation behavior. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check all that apply. For any "covered by existing tests", "not applicable", or waiver entry, add a brief justification on the same line or in the Changes section. --> - [x] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: the proxy and onboarding-selection suites cover the surrounding pull, validation, and selection flows. - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: this restores the already-documented `--non-interactive`, `--yes`, and Ollama tools-gate semantics without changing commands or output contracts. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: focused diff review verified that only interaction-state ownership changes; capability validation, override requirements, and credential handling are unchanged. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each item you ran and confirmed. Leave unchecked items you skipped. Doc-only changes do not require npm test unless you ran it. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved Ollama model tool-capability checks to consistently respect interaction modes (non-interactive, auto-yes, and explicit yes/no confirmation), skipping prompts when appropriate. * Updated onboarding flow so confirmation behavior is delegated cleanly when interactive input is required. * **Tests** * Expanded unit coverage to ensure non-interactive/auto-yes short-circuit correctly and that interactive confirmation receives the expected question and default choice. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Summary
nonInteractivestate and the shared prompt helper after the#3195file moveRoot cause
#3195movedsrc/lib/onboard-ollama-proxy.tstosrc/lib/inference/ollama/proxy.ts, but the lazy-load sites keptrequire("./onboard").That path was correct in the old location, but from
src/lib/inference/ollama/proxy.tsit resolves to a nonexistent sibling module instead ofsrc/lib/onboard.ts. The catch blocks then silently fall back to env vars only.Scope
This change keeps the fix narrow and only repairs the moved-module path regression plus the missing regression coverage.
It does not expand the in-memory
autoYesbehavior, sinceisAutoYesis not currently exported fromsrc/lib/onboard.tsand would be a separate scope decision.Testing
npm run build:clinpm test -- test/ollama-tools-capability.test.tsFixes #4139
Summary by CodeRabbit
Chores
Tests