Skip to content

fix(onboard): skip Ollama loopback override when sudo -n unavailable (#5716) - #5996

Merged
cv merged 8 commits into
mainfrom
fix/5716-ollama-systemd-passwordless-sudo
Jun 30, 2026
Merged

fix(onboard): skip Ollama loopback override when sudo -n unavailable (#5716)#5996
cv merged 8 commits into
mainfrom
fix/5716-ollama-systemd-passwordless-sudo

Conversation

@cjagwani

@cjagwani cjagwani commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Non-interactive Linux onboarding now detects unavailable sudo -n before attempting the Ollama systemd override. It continues only when the active systemd Ollama listener is positively verified as loopback-only; wildcard, non-loopback, missing, or unparseable listener state fails early with actionable sudo guidance.

Related Issue

Fixes #5716.

Changes

  • Probe passwordless sudo before any systemd override command.
  • Inspect the active Ollama listener with non-privileged systemctl and ss evidence when sudo is unavailable.
  • Continue without rewriting the drop-in only when every :11434 listener is IPv4, IPv6, or IPv4-mapped loopback.
  • Fail closed before proxy/model setup when listener exposure cannot be proven safe, and point operators to NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt or passwordless sudo.
  • Cover safe and unsafe listener states, sudo-gate behavior, and adjacent install/onboarding callers.
  • Document the non-interactive systemd behavior in the local-inference guide.

Security posture

A successful HTTP request to 127.0.0.1:11434 is not accepted as proof because a wildcard listener answers there too. The fallback requires an active systemd service plus socket-level listener evidence; otherwise the established fail-closed path remains in force.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — fail-closed wildcard/non-loopback handling and positive listener evidence were reviewed in the signed maintainer follow-up; exact-head automated advisors are also running.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Git hooks passed during commit and push, or npx prek run --from-ref main --to-ref HEAD passes
  • Targeted tests pass for changed behavior
  • Full npm test passes (broad runtime changes only)
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only) — Fern completed with 0 errors and the same 2 existing warnings
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Focused evidence: 16 Ollama systemd/listener tests, 56 adjacent onboarding/install tests, CLI type-check, 23 source-shape/title/conditional budget tests, a normal commit hook rerun with the full CLI suite green, pre-push hooks, and Fern docs validation with 0 errors. The first full hook attempt hit one unrelated 5-second timeout in policy-channel-remove-flow.test.ts; that test passed alone in 170 ms and the normal hook rerun passed.


Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Signed-off-by: Carlos Villela cvillela@nvidia.com

…5716)

`nemoclaw onboard --non-interactive --yes` on a Linux aarch64 DGX
Station (or any Linux host) without passwordless sudo previously
hit step `[3/8] Configuring inference provider`, auto-selected
`Provider: ollama` (recovered from a prior sandbox), then tried the
Ollama systemd loopback override with `sudo -n install / daemon-
reload / restart`. `sudo -n` fails with "sudo: a password is
required" on hosts without passwordless sudo, and the wizard
aborted with `Refusing to continue with a potentially non-loopback
Ollama bind` and exit code 1. The non-interactive contract is
broken: a headless install pipeline cannot recover from an
interactive sudo prompt.

Detect the missing passwordless sudo upfront via a `sudo -n true`
probe and skip the loopback override with an actionable warning
("Skipping Ollama systemd loopback override: passwordless sudo is
not available on this host. Ollama will keep its current bind;
set NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt to allow a password
prompt, or configure passwordless sudo to restore loopback
hardening."). Ollama continues running on its existing bind so
the headless onboard finishes; the host operator can re-run later
to restore the loopback hardening.

The probe is dependency-injected via a new `hasPasswordlessSudoImpl`
test seam, along with `platformImpl` and `hasOllamaSystemdUnitImpl`
seams that let the new fall-through path be exercised
deterministically from non-Linux dev hosts. Two new unit tests
cover the skip-with-warning path and the platform gate.

Closes #5716.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@cjagwani cjagwani added area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow platform: linux Affects non-Ubuntu Linux environments v0.0.69 labels Jun 29, 2026
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

ensureOllamaLoopbackSystemdOverride adds injectable platform and probe hooks, plus a passwordless-sudo skip path for non-interactive runs. Tests cover the new guard, warning output, helper behavior, and non-Linux early return.

Changes

Passwordless sudo guard for Ollama systemd override

Layer / File(s) Summary
Options type and implementation
src/lib/onboard/ollama-systemd.ts
OllamaLoopbackSystemdOverrideOptions adds platformImpl, hasOllamaSystemdUnitImpl, and hasPasswordlessSudoImpl. ensureOllamaLoopbackSystemdOverride uses them for platform gating, systemd unit detection, and an upfront passwordless-sudo check that returns "not-applicable" with a warning when sudo -n cannot continue.
Test coverage
src/lib/onboard/ollama-systemd.test.ts
vi is added to the Vitest imports. New tests verify the non-interactive passwordless-sudo branch on Linux and the non-Linux platform gate.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: skipping the Ollama loopback override when non-interactive sudo is unavailable.
Linked Issues check ✅ Passed The changes implement the required precheck and graceful skip path so non-interactive onboarding no longer fails mid-flow without passwordless sudo.
Out of Scope Changes check ✅ Passed The added test seams, helper export, and env-isolation changes are all directly supporting the Ollama sudo-skip fix.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/5716-ollama-systemd-passwordless-sudo

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/5716-ollama-syst... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5716-ollama-syst... 77f30eb +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/5716-ollama-syst... branch is 67%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/5716-ollama-syst... 77f30eb +/-
src/lib/actions...all/run-plan.ts 80%
src/lib/state/o...oard-session.ts 79%
src/lib/sandbox/config.ts 75%
src/lib/actions...dbox/rebuild.ts 74%
src/lib/state/sandbox.ts 72%
src/lib/onboard/preflight.ts 69%
src/lib/actions...licy-channel.ts 60%
src/lib/onboard...er-gpu-patch.ts 59%
src/lib/policy/index.ts 52%
src/lib/onboard.ts 20%

Updated June 30, 2026 08:38 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: parseListenerEndpoint regex may not handle all ss output formats; then add or justify PRA-T1.
Open items: 3 required · 4 warnings · 2 suggestions · 5 test follow-ups
Since last review: 2 prior items resolved · 0 still apply · 7 new items found

Action checklist

  • PRA-1 Fix: parseListenerEndpoint regex may not handle all ss output formats in src/lib/onboard/ollama-systemd.ts:98
  • PRA-2 Fix: process.exit(1) called directly in library function in src/lib/onboard/ollama-systemd.ts:218
  • PRA-3 Fix: Monolith growth: ollama-systemd.ts +119 lines (310→429, +38%) in src/lib/onboard/ollama-systemd.ts:1
  • PRA-4 Resolve or justify: isActiveOllamaListenerLoopbackOnly swallows ss failures silently in src/lib/onboard/ollama-systemd.ts:158
  • PRA-5 Resolve or justify: TOCTOU window between loopback verification and proxy startup in src/lib/onboard/ollama-systemd.ts:205
  • PRA-6 Resolve or justify: getSudoPrefix returns 'sudo -n' for interactive non-TTY runs in src/lib/onboard/ollama-systemd.ts:76
  • PRA-7 Resolve or justify: shellQuote on constant path inside heredoc passed to bash -c in src/lib/onboard/ollama-systemd.ts:247
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Runtime validation
  • PRA-8 In-scope improvement: Test file grew +146 lines; split to match module extraction in src/lib/onboard/ollama-systemd.test.ts:1
  • PRA-9 In-scope improvement: NEMOCLAW_NON_INTERACTIVE_SUDO_MODE documented in error but not in variable table in docs/inference/use-local-inference.mdx:115

Findings index

ID Severity Category Location Required action
PRA-1 Required security src/lib/onboard/ollama-systemd.ts:98 Harden parseListenerEndpoint against real-world ss output variations. Add test cases for IPv6 zone indices, IPv4-mapped with scope, and verify column index 3 is consistently the local address across ss versions. Consider using a more robust parser or allowlist known formats.
PRA-2 Required security src/lib/onboard/ollama-systemd.ts:218 Change all 4 process.exit(1) calls to return 'failed' and let the caller (onboarding CLI) decide to exit. This matches the declared return type and enables proper unit testing.
PRA-3 Required architecture src/lib/onboard/ollama-systemd.ts:1 Extract pure functions to separate modules: (1) ollama-listener.ts — parseListenerEndpoint, ollamaListenersAreLoopbackOnly, isActiveOllamaListenerLoopbackOnly; (2) ollama-sudo-gate.ts — shouldSkipOllamaLoopbackForMissingSudo, getSudoPrefix; (3) ollama-systemd-override.ts — mergeOllamaLoopbackSystemdOverride, splitSystemdEnvironmentTokens, rewriteEnvironmentLineWithoutManagedAssignments. Keep ensureOllamaLoopbackSystemdOverride as orchestrator.
PRA-4 Resolve/justify security src/lib/onboard/ollama-systemd.ts:158 Add explicit check that ss command succeeded before parsing output. If ss fails, surface a specific error suggesting 'install iproute2 (ss command)'.
PRA-5 Resolve/justify security src/lib/onboard/ollama-systemd.ts:205 Document the TOCTOU window explicitly in comments. Note that the proxy startup that follows enforces authentication regardless of Ollama bind address, limiting exposure to (1) time between check and proxy startup, (2) if proxy fails to start.
PRA-6 Resolve/justify correctness src/lib/onboard/ollama-systemd.ts:76 Clarify intent: if interactive non-TTY should use 'sudo' (allow password prompt), change getSudoPrefix to only return 'sudo -n' when isNonInteractive() is true. If current behavior is intentional, update comment to match.
PRA-7 Resolve/justify correctness src/lib/onboard/ollama-systemd.ts:247 Replace with argv-style runCapture: runCapture([sudoPrefix.split(' ')[0], 'cat', OLLAMA_SYSTEMD_OVERRIDE_PATH], { ignoreError: true, suppressOutput: true, timeout: 30000 }) to avoid shell entirely.
PRA-8 Improvement correctness src/lib/onboard/ollama-systemd.test.ts:1 When extracting modules per ARCH-1, co-locate tests: ollama-listener.test.ts, ollama-sudo-gate.test.ts, ollama-systemd-override.test.ts.
PRA-9 Improvement docs docs/inference/use-local-inference.mdx:115 Add NEMOCLAW_NON_INTERACTIVE_SUDO_MODE to the Non-Interactive Setup variable table with description: 'Set to prompt to allow sudo password prompt in non-interactive runs when a terminal is available.'

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-1 Required — parseListenerEndpoint regex may not handle all ss output formats

  • Location: src/lib/onboard/ollama-systemd.ts:98
  • Category: security
  • Problem: The regex `^(?:\\[([^\\]]+)\\]|(.+)):(\\d+)$` expects the 4th ss token to be exactly 'host:port' but real-world ss -H -ltn output can include IPv6 zone indices (e.g., '[::1%eth0]:11434'), IPv4-mapped with scope ('::ffff:127.0.0.1%lo'), or different column layouts across iproute2 versions. The current .replace(/%[^%]+$/u, '') only strips trailing %... at string end, not mid-token.
  • Impact: False negatives on loopback verification (returns null -> unverified -> fail-closed) on distros where ss includes zone indices. Could block valid non-interactive onboarding on affected systems, or worse, if regex matches incorrectly, could accept non-loopback as loopback.
  • Required action: Harden parseListenerEndpoint against real-world ss output variations. Add test cases for IPv6 zone indices, IPv4-mapped with scope, and verify column index 3 is consistently the local address across ss versions. Consider using a more robust parser or allowlist known formats.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run `ss -H -ltn` on Ubuntu 22.04, 24.04, Debian 12, RHEL 9, and Alpine; verify 4th column format matches regex. Check `man ss` for output format guarantees. Test with IPv6 addresses that have zone indices.
  • Missing regression test: Add test cases in ollama-systemd.test.ts for parseListenerEndpoint with: IPv6 zone index '[::1%eth0]:11434' -> host '::1', IPv4-mapped with scope '::ffff:127.0.0.1%lo' -> host '::ffff:127.0.0.1', malformed lines that should return null.
  • Done when: The required change is committed and verification passes: Run `ss -H -ltn` on Ubuntu 22.04, 24.04, Debian 12, RHEL 9, and Alpine; verify 4th column format matches regex. Check `man ss` for output format guarantees. Test with IPv6 addresses that have zone indices.
  • Evidence: Line 98-107: parseListenerEndpoint function with regex and single .replace at end of string

PRA-2 Required — process.exit(1) called directly in library function

  • Location: src/lib/onboard/ollama-systemd.ts:218
  • Category: security
  • Problem: ensureOllamaLoopbackSystemdOverride calls process.exit(1) on 4 error paths (lines 218, 237, 288, 315) instead of returning 'failed' per its declared return type OllamaLoopbackSystemdOverrideState. This violates library-vs-CLI boundary, makes the function untestable without mocking process.exit, and prevents callers from handling errors gracefully.
  • Impact: Unit tests must mock process.exit to throw (as seen in test line 105). Callers cannot distinguish error types or recover. Violates the function's contract which promises to return 'not-applicable' | 'ready' | 'failed'.
  • Required action: Change all 4 process.exit(1) calls to return 'failed' and let the caller (onboarding CLI) decide to exit. This matches the declared return type and enables proper unit testing.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search for process.exit in ollama-systemd.ts — 4 occurrences at lines 218, 237, 288, 315. Each should return 'failed' instead.
  • Missing regression test: After refactoring, add test that ensureOllamaLoopbackSystemdOverride returns 'failed' (not throws) on all error paths: missing drop-in read, override command failure, listener verification failure, and readiness probe timeout.
  • Done when: The required change is committed and verification passes: Search for process.exit in ollama-systemd.ts — 4 occurrences at lines 218, 237, 288, 315. Each should return 'failed' instead.
  • Evidence: Lines 218, 237, 288, 315: process.exit(1) calls in ensureOllamaLoopbackSystemdOverride

PRA-3 Required — Monolith growth: ollama-systemd.ts +119 lines (310→429, +38%)

  • Location: src/lib/onboard/ollama-systemd.ts:1
  • Category: architecture
  • Problem: File grew from 310 to 429 lines, exceeding the 20-line blocker threshold. Contains multiple responsibilities: systemd drop-in merging (120 lines), loopback listener parsing/verification (40 lines), sudo fallback logic (30 lines), override application with temp files and service restart (80 lines), DGX Spark library override (20 lines).
  • Impact: Reduced maintainability, harder to test in isolation, increased cognitive load. Mixes pure functions (parsing, decision logic) with side-effecting host-boundary operations.
  • Required action: Extract pure functions to separate modules: (1) ollama-listener.ts — parseListenerEndpoint, ollamaListenersAreLoopbackOnly, isActiveOllamaListenerLoopbackOnly; (2) ollama-sudo-gate.ts — shouldSkipOllamaLoopbackForMissingSudo, getSudoPrefix; (3) ollama-systemd-override.ts — mergeOllamaLoopbackSystemdOverride, splitSystemdEnvironmentTokens, rewriteEnvironmentLineWithoutManagedAssignments. Keep ensureOllamaLoopbackSystemdOverride as orchestrator.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: File is 429 lines. Count distinct responsibilities: systemd drop-in parsing (120 lines), listener verification (40 lines), sudo logic (30 lines), override application (80 lines), DGX Spark library override (20 lines).
  • Missing regression test: No new tests needed — existing tests cover extracted functions. Refactor should not change behavior.
  • Done when: The required change is committed and verification passes: File is 429 lines. Count distinct responsibilities: systemd drop-in parsing (120 lines), listener verification (40 lines), sudo logic (30 lines), override application (80 lines), DGX Spark library override (20 lines).
  • Evidence: File length 429 lines vs 310 base. 4 exported functions plus internal helpers.
Review findings by urgency: 3 required fixes, 4 items to resolve/justify, 2 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-4 Resolve/justify — isActiveOllamaListenerLoopbackOnly swallows ss failures silently

  • Location: src/lib/onboard/ollama-systemd.ts:158
  • Category: security
  • Problem: runCapture with ignoreError: true returns empty string on ss failure (permission denied, ss not installed, command not found). This parses as 'no listeners' -> false -> fail-closed, which is safe but produces false negatives on minimal containers where ss is unavailable, blocking valid onboarding without clear error message.
  • Impact: Non-interactive onboarding may fail on minimal containers (e.g., CI images) that lack iproute2/ss package, even when Ollama is correctly bound to loopback. User gets generic 'could not be verified' error instead of actionable 'install iproute2'.
  • Recommended action: Add explicit check that ss command succeeded before parsing output. If ss fails, surface a specific error suggesting 'install iproute2 (ss command)'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check isActiveOllamaListenerLoopbackOnly at line 158-166: runCapture with ignoreError: true on 'systemctl is-active --quiet ollama.service && ss -H -ltn 2>/dev/null'. Simulate ss failure by mocking runCapture to return empty string with error.
  • Missing regression test: Add test for isActiveOllamaListenerLoopbackOnly when ss command fails (simulate via mock returning empty string with error set). Verify specific error message guides user to install ss.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check isActiveOllamaListenerLoopbackOnly at line 158-166: runCapture with ignoreError: true on 'systemctl is-active --quiet ollama.service && ss -H -ltn 2>/dev/null'. Simulate ss failure by mocking runCapture to return empty string with error.
  • Evidence: Line 158-166: runCapture with ignoreError: true, no error/status check before passing to ollamaListenersAreLoopbackOnly

PRA-5 Resolve/justify — TOCTOU window between loopback verification and proxy startup

  • Location: src/lib/onboard/ollama-systemd.ts:205
  • Category: security
  • Problem: Code verifies listener is loopback-only at line 205, then returns 'ready' at line 211. Between this check and when the proxy starts / onboarding completes, the Ollama service could be restarted/reconfigured to bind 0.0.0.0 by another process or admin action.
  • Impact: Theoretical window where Ollama could expose wildcard bind before proxy enforces auth. Risk is low because proxy binds 0.0.0.0:11435 with Bearer token and is the actual network boundary, but non-zero.
  • Recommended action: Document the TOCTOU window explicitly in comments. Note that the proxy startup that follows enforces authentication regardless of Ollama bind address, limiting exposure to (1) time between check and proxy startup, (2) if proxy fails to start.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Trace call chain: ensureOllamaLoopbackSystemdOverride -> returns 'ready' -> caller starts proxy -> proxy validates token. The proxy is the actual network boundary.
  • Missing regression test: Integration test verifying that even if Ollama rebinds to 0.0.0.0 after this check, the proxy still blocks unauthenticated access.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Trace call chain: ensureOllamaLoopbackSystemdOverride -> returns 'ready' -> caller starts proxy -> proxy validates token. The proxy is the actual network boundary.
  • Evidence: Lines 205-211: loopbackOnly check then return 'ready' without re-verification

PRA-6 Resolve/justify — getSudoPrefix returns 'sudo -n' for interactive non-TTY runs

  • Location: src/lib/onboard/ollama-systemd.ts:76
  • Category: correctness
  • Problem: getSudoPrefix returns 'sudo -n' when (!isNonInteractive && !process.stdin.isTTY) — i.e., interactive but piped stdin (e.g., `echo | nemoclaw onboard`). This triggers the sudo-skip gate for interactive non-TTY runs without passwordless sudo, requiring loopback verification. The comment at line 180 says 'non-interactive run that picked the sudo -n prefix' but code includes interactive non-TTY.
  • Impact: Interactive piped runs without passwordless sudo may unexpectedly hit the fail-closed path if Ollama listener isn't verified loopback-only, breaking UX for `echo | nemoclaw onboard` style usage.
  • Recommended action: Clarify intent: if interactive non-TTY should use 'sudo' (allow password prompt), change getSudoPrefix to only return 'sudo -n' when isNonInteractive() is true. If current behavior is intentional, update comment to match.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check getSudoPrefix at line 76: returns 'sudo -n' when (isNonInteractive && rawMode !== 'prompt') OR (!isNonInteractive && !process.stdin.isTTY). Second case is interactive but no TTY.
  • Missing regression test: Add test for interactive non-TTY + no passwordless sudo -> shouldSkipOllamaLoopbackForMissingSudo behavior (should it skip or not?).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check getSudoPrefix at line 76: returns 'sudo -n' when (isNonInteractive && rawMode !== 'prompt') OR (!isNonInteractive && !process.stdin.isTTY). Second case is interactive but no TTY.
  • Evidence: Line 76-84: getSudoPrefix implementation; line 180 comment says 'non-interactive run' but code path includes interactive non-TTY

PRA-7 Resolve/justify — shellQuote on constant path inside heredoc passed to bash -c

  • Location: src/lib/onboard/ollama-systemd.ts:247
  • Category: correctness
  • Problem: runShell heredoc uses `${sudoPrefix} cat ${shellQuote(OLLAMA_SYSTEMD_OVERRIDE_PATH)}` where OLLAMA_SYSTEMD_OVERRIDE_PATH is constant '/etc/systemd/system/ollama.service.d/override.conf'. shellQuote is unnecessary but harmless. Codebase is moving away from shell strings toward argv-style runCapture.
  • Impact: Inconsistent with codebase direction (runner.ts rejects shell strings). Minimal risk since path is constant.
  • Recommended action: Replace with argv-style runCapture: runCapture([sudoPrefix.split(' ')[0], 'cat', OLLAMA_SYSTEMD_OVERRIDE_PATH], { ignoreError: true, suppressOutput: true, timeout: 30000 }) to avoid shell entirely.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if runCapture can replace the shell heredoc for reading the drop-in file. sudoPrefix is 'sudo' or 'sudo -n' — split to get command and args.
  • Missing regression test: None — refactor only.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if runCapture can replace the shell heredoc for reading the drop-in file. sudoPrefix is 'sudo' or 'sudo -n' — split to get command and args.
  • Evidence: Line 247: shellQuote(OLLAMA_SYSTEMD_OVERRIDE_PATH) inside heredoc

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-8 Improvement — Test file grew +146 lines; split to match module extraction

  • Location: src/lib/onboard/ollama-systemd.test.ts:1
  • Category: correctness
  • Problem: Test file grew from 88 to 234 lines. Contains tests for 4 exported functions plus integration-style tests. Should be split when modules are extracted per ARCH-1.
  • Impact: Single large test file harder to navigate and maintain.
  • Suggested action: When extracting modules per ARCH-1, co-locate tests: ollama-listener.test.ts, ollama-sudo-gate.test.ts, ollama-systemd-override.test.ts.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Test file has 4 describe blocks for 4 different functions: mergeOllamaLoopbackSystemdOverride, ollamaListenersAreLoopbackOnly, ensureOllamaLoopbackSystemdOverride non-interactive sudo, shouldSkipOllamaLoopbackForMissingSudo.
  • Missing regression test: None — organizational.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test file 234 lines with 4 describe blocks

PRA-9 Improvement — NEMOCLAW_NON_INTERACTIVE_SUDO_MODE documented in error but not in variable table

  • Location: docs/inference/use-local-inference.mdx:115
  • Category: docs
  • Problem: Documentation at line 115 references NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt as workaround, but the variable table in Non-Interactive Setup section (line ~280) doesn't include it.
  • Impact: Users seeing the error message won't find the variable documented in the reference table.
  • Suggested action: Add NEMOCLAW_NON_INTERACTIVE_SUDO_MODE to the Non-Interactive Setup variable table with description: 'Set to prompt to allow sudo password prompt in non-interactive runs when a terminal is available.'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check the variable table at end of Non-Interactive Setup section in docs/inference/use-local-inference.mdx.
  • Missing regression test: None — documentation only.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 115 mentions variable; variable table at end of section missing it
Simplification opportunities: 3 possible cuts, net -205 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-3 shrink (src/lib/onboard/ollama-systemd.ts:1): Extract parseListenerEndpoint, ollamaListenersAreLoopbackOnly, isActiveOllamaListenerLoopbackOnly to ollama-listener.ts; shouldSkipOllamaLoopbackForMissingSudo, getSudoPrefix to ollama-sudo-gate.ts; mergeOllamaLoopbackSystemdOverride, splitSystemdEnvironmentTokens, rewriteEnvironmentLineWithoutManagedAssignments to ollama-systemd-override.ts
    • Replacement: Three focused modules + thin orchestrator in ollama-systemd.ts
    • Net: -200 lines
    • Safety boundary: All extracted functions must retain exact same signatures and behavior; existing tests must pass without modification
  • PRA-7 stdlib (src/lib/onboard/ollama-systemd.ts:247): runShell heredoc with shellQuote for reading drop-in file
    • Replacement: runCapture([sudoPrefix.split(' ')[0], 'cat', OLLAMA_SYSTEMD_OVERRIDE_PATH], { ignoreError: true, suppressOutput: true, timeout: 30_000 })
    • Net: -5 lines
    • Safety boundary: Must handle sudoPrefix 'sudo' vs 'sudo -n' correctly; preserve ignoreError and suppressOutput behavior
  • PRA-8 shrink (src/lib/onboard/ollama-systemd.test.ts:1): Single test file with all tests
    • Replacement: Three co-located test files matching extracted modules
    • Net: 0 lines
    • Safety boundary: All existing tests must pass; no behavior change
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — parseListenerEndpoint with IPv6 zone index '[::1%eth0]:11434' -> host '::1'. parseListenerEndpoint parses untrusted ss command output from external process boundary. Real-world ss output variations (IPv6 zone indices, scope IDs, column layouts across iproute2 versions) not covered by unit tests. isActiveOllamaListenerLoopbackOnly swallows ss failures silently. Runtime validation on multiple Linux distributions needed.
  • PRA-T2 Runtime validation — parseListenerEndpoint with IPv4-mapped scope '::ffff:127.0.0.1%lo' -> host '::ffff:127.0.0.1'. parseListenerEndpoint parses untrusted ss command output from external process boundary. Real-world ss output variations (IPv6 zone indices, scope IDs, column layouts across iproute2 versions) not covered by unit tests. isActiveOllamaListenerLoopbackOnly swallows ss failures silently. Runtime validation on multiple Linux distributions needed.
  • PRA-T3 Runtime validation — parseListenerEndpoint with malformed/unparseable lines -> null. parseListenerEndpoint parses untrusted ss command output from external process boundary. Real-world ss output variations (IPv6 zone indices, scope IDs, column layouts across iproute2 versions) not covered by unit tests. isActiveOllamaListenerLoopbackOnly swallows ss failures silently. Runtime validation on multiple Linux distributions needed.
  • PRA-T4 Runtime validation — isActiveOllamaListenerLoopbackOnly when ss fails (permission denied, not installed). parseListenerEndpoint parses untrusted ss command output from external process boundary. Real-world ss output variations (IPv6 zone indices, scope IDs, column layouts across iproute2 versions) not covered by unit tests. isActiveOllamaListenerLoopbackOnly swallows ss failures silently. Runtime validation on multiple Linux distributions needed.
  • PRA-T5 Runtime validation — Interactive non-TTY + no passwordless sudo -> shouldSkip behavior. parseListenerEndpoint parses untrusted ss command output from external process boundary. Real-world ss output variations (IPv6 zone indices, scope IDs, column layouts across iproute2 versions) not covered by unit tests. isActiveOllamaListenerLoopbackOnly swallows ss failures silently. Runtime validation on multiple Linux distributions needed.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — parseListenerEndpoint regex may not handle all ss output formats

  • Location: src/lib/onboard/ollama-systemd.ts:98
  • Category: security
  • Problem: The regex `^(?:\\[([^\\]]+)\\]|(.+)):(\\d+)$` expects the 4th ss token to be exactly 'host:port' but real-world ss -H -ltn output can include IPv6 zone indices (e.g., '[::1%eth0]:11434'), IPv4-mapped with scope ('::ffff:127.0.0.1%lo'), or different column layouts across iproute2 versions. The current .replace(/%[^%]+$/u, '') only strips trailing %... at string end, not mid-token.
  • Impact: False negatives on loopback verification (returns null -> unverified -> fail-closed) on distros where ss includes zone indices. Could block valid non-interactive onboarding on affected systems, or worse, if regex matches incorrectly, could accept non-loopback as loopback.
  • Required action: Harden parseListenerEndpoint against real-world ss output variations. Add test cases for IPv6 zone indices, IPv4-mapped with scope, and verify column index 3 is consistently the local address across ss versions. Consider using a more robust parser or allowlist known formats.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Run `ss -H -ltn` on Ubuntu 22.04, 24.04, Debian 12, RHEL 9, and Alpine; verify 4th column format matches regex. Check `man ss` for output format guarantees. Test with IPv6 addresses that have zone indices.
  • Missing regression test: Add test cases in ollama-systemd.test.ts for parseListenerEndpoint with: IPv6 zone index '[::1%eth0]:11434' -> host '::1', IPv4-mapped with scope '::ffff:127.0.0.1%lo' -> host '::ffff:127.0.0.1', malformed lines that should return null.
  • Done when: The required change is committed and verification passes: Run `ss -H -ltn` on Ubuntu 22.04, 24.04, Debian 12, RHEL 9, and Alpine; verify 4th column format matches regex. Check `man ss` for output format guarantees. Test with IPv6 addresses that have zone indices.
  • Evidence: Line 98-107: parseListenerEndpoint function with regex and single .replace at end of string

PRA-2 Required — process.exit(1) called directly in library function

  • Location: src/lib/onboard/ollama-systemd.ts:218
  • Category: security
  • Problem: ensureOllamaLoopbackSystemdOverride calls process.exit(1) on 4 error paths (lines 218, 237, 288, 315) instead of returning 'failed' per its declared return type OllamaLoopbackSystemdOverrideState. This violates library-vs-CLI boundary, makes the function untestable without mocking process.exit, and prevents callers from handling errors gracefully.
  • Impact: Unit tests must mock process.exit to throw (as seen in test line 105). Callers cannot distinguish error types or recover. Violates the function's contract which promises to return 'not-applicable' | 'ready' | 'failed'.
  • Required action: Change all 4 process.exit(1) calls to return 'failed' and let the caller (onboarding CLI) decide to exit. This matches the declared return type and enables proper unit testing.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Search for process.exit in ollama-systemd.ts — 4 occurrences at lines 218, 237, 288, 315. Each should return 'failed' instead.
  • Missing regression test: After refactoring, add test that ensureOllamaLoopbackSystemdOverride returns 'failed' (not throws) on all error paths: missing drop-in read, override command failure, listener verification failure, and readiness probe timeout.
  • Done when: The required change is committed and verification passes: Search for process.exit in ollama-systemd.ts — 4 occurrences at lines 218, 237, 288, 315. Each should return 'failed' instead.
  • Evidence: Lines 218, 237, 288, 315: process.exit(1) calls in ensureOllamaLoopbackSystemdOverride

PRA-3 Required — Monolith growth: ollama-systemd.ts +119 lines (310→429, +38%)

  • Location: src/lib/onboard/ollama-systemd.ts:1
  • Category: architecture
  • Problem: File grew from 310 to 429 lines, exceeding the 20-line blocker threshold. Contains multiple responsibilities: systemd drop-in merging (120 lines), loopback listener parsing/verification (40 lines), sudo fallback logic (30 lines), override application with temp files and service restart (80 lines), DGX Spark library override (20 lines).
  • Impact: Reduced maintainability, harder to test in isolation, increased cognitive load. Mixes pure functions (parsing, decision logic) with side-effecting host-boundary operations.
  • Required action: Extract pure functions to separate modules: (1) ollama-listener.ts — parseListenerEndpoint, ollamaListenersAreLoopbackOnly, isActiveOllamaListenerLoopbackOnly; (2) ollama-sudo-gate.ts — shouldSkipOllamaLoopbackForMissingSudo, getSudoPrefix; (3) ollama-systemd-override.ts — mergeOllamaLoopbackSystemdOverride, splitSystemdEnvironmentTokens, rewriteEnvironmentLineWithoutManagedAssignments. Keep ensureOllamaLoopbackSystemdOverride as orchestrator.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: File is 429 lines. Count distinct responsibilities: systemd drop-in parsing (120 lines), listener verification (40 lines), sudo logic (30 lines), override application (80 lines), DGX Spark library override (20 lines).
  • Missing regression test: No new tests needed — existing tests cover extracted functions. Refactor should not change behavior.
  • Done when: The required change is committed and verification passes: File is 429 lines. Count distinct responsibilities: systemd drop-in parsing (120 lines), listener verification (40 lines), sudo logic (30 lines), override application (80 lines), DGX Spark library override (20 lines).
  • Evidence: File length 429 lines vs 310 base. 4 exported functions plus internal helpers.

PRA-4 Resolve/justify — isActiveOllamaListenerLoopbackOnly swallows ss failures silently

  • Location: src/lib/onboard/ollama-systemd.ts:158
  • Category: security
  • Problem: runCapture with ignoreError: true returns empty string on ss failure (permission denied, ss not installed, command not found). This parses as 'no listeners' -> false -> fail-closed, which is safe but produces false negatives on minimal containers where ss is unavailable, blocking valid onboarding without clear error message.
  • Impact: Non-interactive onboarding may fail on minimal containers (e.g., CI images) that lack iproute2/ss package, even when Ollama is correctly bound to loopback. User gets generic 'could not be verified' error instead of actionable 'install iproute2'.
  • Recommended action: Add explicit check that ss command succeeded before parsing output. If ss fails, surface a specific error suggesting 'install iproute2 (ss command)'.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check isActiveOllamaListenerLoopbackOnly at line 158-166: runCapture with ignoreError: true on 'systemctl is-active --quiet ollama.service && ss -H -ltn 2>/dev/null'. Simulate ss failure by mocking runCapture to return empty string with error.
  • Missing regression test: Add test for isActiveOllamaListenerLoopbackOnly when ss command fails (simulate via mock returning empty string with error set). Verify specific error message guides user to install ss.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check isActiveOllamaListenerLoopbackOnly at line 158-166: runCapture with ignoreError: true on 'systemctl is-active --quiet ollama.service && ss -H -ltn 2>/dev/null'. Simulate ss failure by mocking runCapture to return empty string with error.
  • Evidence: Line 158-166: runCapture with ignoreError: true, no error/status check before passing to ollamaListenersAreLoopbackOnly

PRA-5 Resolve/justify — TOCTOU window between loopback verification and proxy startup

  • Location: src/lib/onboard/ollama-systemd.ts:205
  • Category: security
  • Problem: Code verifies listener is loopback-only at line 205, then returns 'ready' at line 211. Between this check and when the proxy starts / onboarding completes, the Ollama service could be restarted/reconfigured to bind 0.0.0.0 by another process or admin action.
  • Impact: Theoretical window where Ollama could expose wildcard bind before proxy enforces auth. Risk is low because proxy binds 0.0.0.0:11435 with Bearer token and is the actual network boundary, but non-zero.
  • Recommended action: Document the TOCTOU window explicitly in comments. Note that the proxy startup that follows enforces authentication regardless of Ollama bind address, limiting exposure to (1) time between check and proxy startup, (2) if proxy fails to start.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Trace call chain: ensureOllamaLoopbackSystemdOverride -> returns 'ready' -> caller starts proxy -> proxy validates token. The proxy is the actual network boundary.
  • Missing regression test: Integration test verifying that even if Ollama rebinds to 0.0.0.0 after this check, the proxy still blocks unauthenticated access.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Trace call chain: ensureOllamaLoopbackSystemdOverride -> returns 'ready' -> caller starts proxy -> proxy validates token. The proxy is the actual network boundary.
  • Evidence: Lines 205-211: loopbackOnly check then return 'ready' without re-verification

PRA-6 Resolve/justify — getSudoPrefix returns 'sudo -n' for interactive non-TTY runs

  • Location: src/lib/onboard/ollama-systemd.ts:76
  • Category: correctness
  • Problem: getSudoPrefix returns 'sudo -n' when (!isNonInteractive && !process.stdin.isTTY) — i.e., interactive but piped stdin (e.g., `echo | nemoclaw onboard`). This triggers the sudo-skip gate for interactive non-TTY runs without passwordless sudo, requiring loopback verification. The comment at line 180 says 'non-interactive run that picked the sudo -n prefix' but code includes interactive non-TTY.
  • Impact: Interactive piped runs without passwordless sudo may unexpectedly hit the fail-closed path if Ollama listener isn't verified loopback-only, breaking UX for `echo | nemoclaw onboard` style usage.
  • Recommended action: Clarify intent: if interactive non-TTY should use 'sudo' (allow password prompt), change getSudoPrefix to only return 'sudo -n' when isNonInteractive() is true. If current behavior is intentional, update comment to match.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check getSudoPrefix at line 76: returns 'sudo -n' when (isNonInteractive && rawMode !== 'prompt') OR (!isNonInteractive && !process.stdin.isTTY). Second case is interactive but no TTY.
  • Missing regression test: Add test for interactive non-TTY + no passwordless sudo -> shouldSkipOllamaLoopbackForMissingSudo behavior (should it skip or not?).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check getSudoPrefix at line 76: returns 'sudo -n' when (isNonInteractive && rawMode !== 'prompt') OR (!isNonInteractive && !process.stdin.isTTY). Second case is interactive but no TTY.
  • Evidence: Line 76-84: getSudoPrefix implementation; line 180 comment says 'non-interactive run' but code path includes interactive non-TTY

PRA-7 Resolve/justify — shellQuote on constant path inside heredoc passed to bash -c

  • Location: src/lib/onboard/ollama-systemd.ts:247
  • Category: correctness
  • Problem: runShell heredoc uses `${sudoPrefix} cat ${shellQuote(OLLAMA_SYSTEMD_OVERRIDE_PATH)}` where OLLAMA_SYSTEMD_OVERRIDE_PATH is constant '/etc/systemd/system/ollama.service.d/override.conf'. shellQuote is unnecessary but harmless. Codebase is moving away from shell strings toward argv-style runCapture.
  • Impact: Inconsistent with codebase direction (runner.ts rejects shell strings). Minimal risk since path is constant.
  • Recommended action: Replace with argv-style runCapture: runCapture([sudoPrefix.split(' ')[0], 'cat', OLLAMA_SYSTEMD_OVERRIDE_PATH], { ignoreError: true, suppressOutput: true, timeout: 30000 }) to avoid shell entirely.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Check if runCapture can replace the shell heredoc for reading the drop-in file. sudoPrefix is 'sudo' or 'sudo -n' — split to get command and args.
  • Missing regression test: None — refactor only.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Check if runCapture can replace the shell heredoc for reading the drop-in file. sudoPrefix is 'sudo' or 'sudo -n' — split to get command and args.
  • Evidence: Line 247: shellQuote(OLLAMA_SYSTEMD_OVERRIDE_PATH) inside heredoc

PRA-8 Improvement — Test file grew +146 lines; split to match module extraction

  • Location: src/lib/onboard/ollama-systemd.test.ts:1
  • Category: correctness
  • Problem: Test file grew from 88 to 234 lines. Contains tests for 4 exported functions plus integration-style tests. Should be split when modules are extracted per ARCH-1.
  • Impact: Single large test file harder to navigate and maintain.
  • Suggested action: When extracting modules per ARCH-1, co-locate tests: ollama-listener.test.ts, ollama-sudo-gate.test.ts, ollama-systemd-override.test.ts.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Test file has 4 describe blocks for 4 different functions: mergeOllamaLoopbackSystemdOverride, ollamaListenersAreLoopbackOnly, ensureOllamaLoopbackSystemdOverride non-interactive sudo, shouldSkipOllamaLoopbackForMissingSudo.
  • Missing regression test: None — organizational.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test file 234 lines with 4 describe blocks

PRA-9 Improvement — NEMOCLAW_NON_INTERACTIVE_SUDO_MODE documented in error but not in variable table

  • Location: docs/inference/use-local-inference.mdx:115
  • Category: docs
  • Problem: Documentation at line 115 references NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt as workaround, but the variable table in Non-Interactive Setup section (line ~280) doesn't include it.
  • Impact: Users seeing the error message won't find the variable documented in the reference table.
  • Suggested action: Add NEMOCLAW_NON_INTERACTIVE_SUDO_MODE to the Non-Interactive Setup variable table with description: 'Set to prompt to allow sudo password prompt in non-interactive runs when a terminal is available.'
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Check the variable table at end of Non-Interactive Setup section in docs/inference/use-local-inference.mdx.
  • Missing regression test: None — documentation only.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Line 115 mentions variable; variable table at end of section missing it

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: Extract new Ollama listener/sudo helpers to offset monolith growth; then add or justify PRA-T1.
Open items: 1 required · 1 warning · 0 suggestions · 4 test follow-ups
Since last review: 0 prior items resolved · 3 still apply · 2 new items found

Action checklist

  • PRA-1 Fix: Extract new Ollama listener/sudo helpers to offset monolith growth in src/lib/onboard/ollama-systemd.ts:82
  • PRA-2 Resolve or justify: Pin real `ss` wildcard and mixed-listener outputs in src/lib/onboard/ollama-systemd.test.ts:114
  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation

Findings index

ID Severity Category Location Required action
PRA-1 Required architecture src/lib/onboard/ollama-systemd.ts:82 Move the new listener endpoint parsing/loopback proof and the pure missing-sudo decision/probe helpers into a focused helper module or otherwise offset the added size, while preserving the current fail-closed behavior and existing public imports needed by tests.
PRA-2 Resolve/justify security src/lib/onboard/ollama-systemd.test.ts:114 Add focused unit cases to `ollamaListenersAreLoopbackOnly` for `LISTEN ... *:11434 ...` and for mixed output containing both `127.0.0.1:11434` and a wildcard or non-loopback `:11434` listener, asserting both return `false`.

🚨 Required before merge

Address these before merging unless a maintainer explicitly overrides the advisor with rationale.

PRA-1 Required — Extract new Ollama listener/sudo helpers to offset monolith growth

  • Location: src/lib/onboard/ollama-systemd.ts:82
  • Category: architecture
  • Problem: This PR adds the missing-sudo gate, passwordless sudo probe, listener endpoint parser, `ss` proof, and related orchestration directly into `ollama-systemd.ts`, growing an already security-critical host-boundary module by about 119 lines. The logic itself is important and should stay, but keeping all of the new parsing/probing plus privileged systemd orchestration in one file makes future audits of the raw-Ollama exposure boundary harder.
  • Impact: This file controls whether raw Ollama is exposed directly or only through the authenticated proxy. Continued monolith growth in this area increases the chance that future changes miss a fail-closed branch, weaken listener validation, or accidentally mix test seams with privileged host commands.
  • Required action: Move the new listener endpoint parsing/loopback proof and the pure missing-sudo decision/probe helpers into a focused helper module or otherwise offset the added size, while preserving the current fail-closed behavior and existing public imports needed by tests.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `src/lib/onboard/ollama-systemd.ts` around lines 82-135 and 202-222, then compare the PR diff stat showing `src/lib/onboard/ollama-systemd.ts` grew by roughly 119 lines.
  • Missing regression test: Existing `ollama-systemd.test.ts` coverage should be retained after extraction; no new behavior test is required for the extraction itself beyond confirming the current listener and sudo-gate tests still target the moved helpers.
  • Done when: The required change is committed and verification passes: Read `src/lib/onboard/ollama-systemd.ts` around lines 82-135 and 202-222, then compare the PR diff stat showing `src/lib/onboard/ollama-systemd.ts` grew by roughly 119 lines.
  • Evidence: The drift context reports `src/lib/onboard/ollama-systemd.ts` grew from 310 to 429 lines, and the new code is concentrated in the systemd/sudo/listener security boundary.
Review findings by urgency: 1 required fix, 1 item to resolve/justify, 0 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

PRA-2 Resolve/justify — Pin real `ss` wildcard and mixed-listener outputs

  • Location: src/lib/onboard/ollama-systemd.test.ts:114
  • Category: security
  • Problem: The parser currently rejects `0.0.0.0:11434`, `[::]:11434`, non-loopback addresses, missing output, and unrelated ports. It does not explicitly pin real `ss` wildcard output such as `*:11434`, nor the mixed case where one loopback listener and one wildcard/non-loopback listener both exist on `11434`.
  • Impact: The current implementation appears to reject these cases, but this is the security boundary that decides whether onboarding may skip rewriting the systemd drop-in. A future parser change could accidentally treat wildcard or mixed evidence as safe and leave raw Ollama reachable outside the authenticated proxy.
  • Recommended action: Add focused unit cases to `ollamaListenersAreLoopbackOnly` for `LISTEN ... *:11434 ...` and for mixed output containing both `127.0.0.1:11434` and a wildcard or non-loopback `:11434` listener, asserting both return `false`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/onboard/ollama-systemd.test.ts` lines 104-121; the existing negative test covers `0.0.0.0`, `[::]`, and `192.168.1.8`, but not `*:11434` or mixed listener output.
  • Missing regression test: Add tests named like `rejects ss wildcard star listeners` and `rejects mixed loopback and exposed Ollama listeners` under the existing `ollamaListenersAreLoopbackOnly` describe block.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/onboard/ollama-systemd.test.ts` lines 104-121; the existing negative test covers `0.0.0.0`, `[::]`, and `192.168.1.8`, but not `*:11434` or mixed listener output.
  • Evidence: `ollamaListenersAreLoopbackOnly` returns safe only when every collected `OLLAMA_PORT` host is loopback; the tests should explicitly preserve that contract for common `ss` output forms.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

  • None.
Simplification opportunities: 1 possible cut

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-1 shrink (src/lib/onboard/ollama-systemd.ts:82): Cut the new listener parsing/proof helpers and pure missing-sudo gate from the main systemd orchestration file.
    • Replacement: Place them in a small focused helper module, for example an Ollama listener/sudo proof module imported by `ollama-systemd.ts` and its tests.
    • Net: 0 lines
    • Safety boundary: Do not remove or weaken the positive active-service + `ss` listener proof, wildcard/non-loopback rejection, or fail-closed exit before proxy/model setup.
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — On a Linux fixture with an active systemd Ollama listener bound to `127.0.0.1:11434` and no passwordless sudo, run non-interactive onboarding far enough to verify it warns, does not attempt the sudo drop-in rewrite, and proceeds to proxy/model setup.. The hermetic unit tests cover the changed decision logic and parser behavior, but the changed behavior controls real host-boundary systemd, sudo, and TCP listener state.
  • PRA-T2 Runtime validation — On a Linux fixture with an active systemd Ollama listener bound to `0.0.0.0:11434` or `*:11434` and no passwordless sudo, verify onboarding exits before proxy/model validation with the `NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt` or passwordless-sudo remediation hint.. The hermetic unit tests cover the changed decision logic and parser behavior, but the changed behavior controls real host-boundary systemd, sudo, and TCP listener state.
  • PRA-T3 Runtime validation — On a Linux fixture with passwordless sudo available, verify the managed systemd drop-in is still written with `OLLAMA_HOST=127.0.0.1:11434`, the service is restarted, and readiness returns `ready`.. The hermetic unit tests cover the changed decision logic and parser behavior, but the changed behavior controls real host-boundary systemd, sudo, and TCP listener state.
  • PRA-T4 Runtime validation — Add unit coverage showing `ollamaListenersAreLoopbackOnly` rejects mixed `ss` output containing both loopback and wildcard/non-loopback listeners on port `11434`.. The hermetic unit tests cover the changed decision logic and parser behavior, but the changed behavior controls real host-boundary systemd, sudo, and TCP listener state.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — Extract new Ollama listener/sudo helpers to offset monolith growth

  • Location: src/lib/onboard/ollama-systemd.ts:82
  • Category: architecture
  • Problem: This PR adds the missing-sudo gate, passwordless sudo probe, listener endpoint parser, `ss` proof, and related orchestration directly into `ollama-systemd.ts`, growing an already security-critical host-boundary module by about 119 lines. The logic itself is important and should stay, but keeping all of the new parsing/probing plus privileged systemd orchestration in one file makes future audits of the raw-Ollama exposure boundary harder.
  • Impact: This file controls whether raw Ollama is exposed directly or only through the authenticated proxy. Continued monolith growth in this area increases the chance that future changes miss a fail-closed branch, weaken listener validation, or accidentally mix test seams with privileged host commands.
  • Required action: Move the new listener endpoint parsing/loopback proof and the pure missing-sudo decision/probe helpers into a focused helper module or otherwise offset the added size, while preserving the current fail-closed behavior and existing public imports needed by tests.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read `src/lib/onboard/ollama-systemd.ts` around lines 82-135 and 202-222, then compare the PR diff stat showing `src/lib/onboard/ollama-systemd.ts` grew by roughly 119 lines.
  • Missing regression test: Existing `ollama-systemd.test.ts` coverage should be retained after extraction; no new behavior test is required for the extraction itself beyond confirming the current listener and sudo-gate tests still target the moved helpers.
  • Done when: The required change is committed and verification passes: Read `src/lib/onboard/ollama-systemd.ts` around lines 82-135 and 202-222, then compare the PR diff stat showing `src/lib/onboard/ollama-systemd.ts` grew by roughly 119 lines.
  • Evidence: The drift context reports `src/lib/onboard/ollama-systemd.ts` grew from 310 to 429 lines, and the new code is concentrated in the systemd/sudo/listener security boundary.

PRA-2 Resolve/justify — Pin real `ss` wildcard and mixed-listener outputs

  • Location: src/lib/onboard/ollama-systemd.test.ts:114
  • Category: security
  • Problem: The parser currently rejects `0.0.0.0:11434`, `[::]:11434`, non-loopback addresses, missing output, and unrelated ports. It does not explicitly pin real `ss` wildcard output such as `*:11434`, nor the mixed case where one loopback listener and one wildcard/non-loopback listener both exist on `11434`.
  • Impact: The current implementation appears to reject these cases, but this is the security boundary that decides whether onboarding may skip rewriting the systemd drop-in. A future parser change could accidentally treat wildcard or mixed evidence as safe and leave raw Ollama reachable outside the authenticated proxy.
  • Recommended action: Add focused unit cases to `ollamaListenersAreLoopbackOnly` for `LISTEN ... *:11434 ...` and for mixed output containing both `127.0.0.1:11434` and a wildcard or non-loopback `:11434` listener, asserting both return `false`.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read `src/lib/onboard/ollama-systemd.test.ts` lines 104-121; the existing negative test covers `0.0.0.0`, `[::]`, and `192.168.1.8`, but not `*:11434` or mixed listener output.
  • Missing regression test: Add tests named like `rejects ss wildcard star listeners` and `rejects mixed loopback and exposed Ollama listeners` under the existing `ollamaListenersAreLoopbackOnly` describe block.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read `src/lib/onboard/ollama-systemd.test.ts` lines 104-121; the existing negative test covers `0.0.0.0`, `[::]`, and `192.168.1.8`, but not `*:11434` or mixed listener output.
  • Evidence: `ollamaListenersAreLoopbackOnly` returns safe only when every collected `OLLAMA_PORT` host is loopback; the tests should explicitly preserve that contract for common `ss` output forms.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: gpu-double-onboard-vitest, ollama-auth-proxy-vitest
Optional E2E: onboard-negative-paths-vitest, gpu-e2e

Dispatch hint: gpu-double-onboard-vitest,ollama-auth-proxy-vitest

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • gpu-double-onboard-vitest (high): Required because this is the existing live Local Ollama onboarding job that runs install.sh with NEMOCLAW_PROVIDER=ollama, re-runs non-interactive onboard, validates the auth-proxy token, and proves sandbox inference.local still reaches Ollama after re-onboard. It gives the closest end-to-end coverage for changes in the Ollama systemd/proxy onboarding path.
  • ollama-auth-proxy-vitest (medium): Required because the PR modifies the loopback-only enforcement that protects Ollama from direct network exposure while relying on the authenticated proxy. This live job validates real Ollama loopback serving, token enforcement, proxy forwarding, persistence, restart, and container reachability boundaries.

Optional E2E

  • onboard-negative-paths-vitest (low): Useful adjacent confidence for onboarding failure behavior and CLI error surfacing, but the current existing scenario does not appear to specifically model an active systemd Ollama service with missing passwordless sudo.
  • gpu-e2e (high): Useful if GPU capacity is available because it exercises the full local Ollama onboarding user flow, sandbox creation, auth proxy, and inference. gpu-double-onboard-vitest is the stronger required choice for this PR because it also covers re-onboard behavior.

New E2E recommendations

  • local-ollama-onboarding (high): Existing live jobs cover successful Local Ollama onboarding and proxy behavior, but none clearly exercise the new [Linux aarch64][Onboard] nemoclaw onboard --non-interactive --yes aborts with "sudo: a password is required" during Ollama systemd loopback override on hosts without passwordless sudo #5716 branch: non-interactive Linux onboarding with an existing active systemd Ollama service, no passwordless sudo, and loopback-only listener evidence allowing continuation.
    • Suggested test: Add a live or scenario E2E that provisions a fake or isolated systemd/ss boundary for Ollama, disables passwordless sudo for the NemoClaw command, and verifies onboarding continues only when ss reports loopback-only listeners.
  • security-boundary (medium): The fail-closed branch for wildcard or non-loopback Ollama listeners is security-sensitive and currently appears covered mainly by unit tests. A live negative E2E would protect against regressions in shell probing, ss parsing assumptions, and user-facing remediation output.
    • Suggested test: Extend onboard-negative-paths with a Linux Ollama-systemd listener exposure case that confirms non-interactive onboarding aborts before configuring the proxy when listener evidence is missing, unreadable, wildcard, or non-loopback.

Dispatch hint

  • Workflow: .github/workflows/e2e-vitest-scenarios.yaml
  • jobs input: gpu-double-onboard-vitest,ollama-auth-proxy-vitest

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: gpu-e2e-vitest
Optional Vitest E2E scenarios: None

Dispatch required Vitest E2E scenarios:

  • gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=gpu-e2e-vitest

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • gpu-e2e-vitest: The PR changes Ollama systemd loopback override behavior used by local Ollama onboarding, including the non-interactive sudo skip path. The existing GPU Ollama free-standing Vitest job is the live Vitest path that exercises local Ollama onboarding/systemd integration.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=gpu-e2e-vitest

Optional Vitest E2E scenarios

  • None.

Relevant changed files

  • src/lib/onboard/ollama-systemd.test.ts
  • src/lib/onboard/ollama-systemd.ts

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28384559715
Target ref: 550ea04a3ed43c36ca01c1b4d89dce2a8aac1144
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/lib/onboard/ollama-systemd.test.ts`:
- Around line 100-118: This test is still affected by the ambient
NEMOCLAW_NON_INTERACTIVE_SUDO_MODE environment, which can change the behavior of
ensureOllamaLoopbackSystemdOverride via getSudoPrefix and bypass the intended
sudo -n branch. In the ollama-systemd test, isolate the environment by
explicitly setting or stubbing the sudo-mode env for the duration of this case,
and restore it afterward, so the assertions always exercise the non-interactive
passwordless-sudo guard instead of real sudo/systemd behavior.
- Around line 121-128: The non-Linux test for
ensureOllamaLoopbackSystemdOverride is too weak because hasPasswordlessSudoImpl
returning false would still allow the test to pass even if Linux-only checks
were reached. Update the test in ollama-systemd.test.ts so the Linux-only probes
(especially hasOllamaSystemdUnitImpl and hasPasswordlessSudoImpl) throw if
invoked, while platformImpl returns "darwin"; this will prove the platform gate
short-circuits before any Linux-specific logic runs.
🪄 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: b3ab0e6d-408a-4696-afc8-a083f5976abb

📥 Commits

Reviewing files that changed from the base of the PR and between c6113be and 550ea04.

📒 Files selected for processing (2)
  • src/lib/onboard/ollama-systemd.test.ts
  • src/lib/onboard/ollama-systemd.ts

Comment thread src/lib/onboard/ollama-systemd.test.ts Outdated
Comment thread src/lib/onboard/ollama-systemd.test.ts Outdated
@cv cv added v0.0.70 and removed v0.0.69 labels Jun 29, 2026
@cjagwani cjagwani self-assigned this Jun 29, 2026
Multiple targeted fixes for PR #5996.

Advisor PRA-3 (required) + CI repository-checks (no-test-dist-imports):
src/lib/onboard/ollama-systemd.test.ts imported the symbols under
test from `../../../dist/lib/...`. The repository's source-shape
contract requires src/ tests to import from source, with compiled
artifact assertions confined to test/package-contract/. Switch the
imports to the source-relative paths and the contract test fires
on every run rather than against a stale dist build.

Advisor PRA-2 (required): the silent skip-with-warning path accepts
a weaker security posture (Ollama on its existing non-loopback bind)
in exchange for not breaking the headless install contract. Document
the trade-off and the two escape hatches at the call site: the
NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt env var (opts back into a
real sudo prompt when a TTY is attached), and the explicit warning
that names the override in stderr so a CI log capture surfaces the
re-tighten knob without a source dive. Aborting with process.exit(1)
is explicitly rejected as a regression to the original #5716 break.

CodeRabbit (Minor): isolate NEMOCLAW_NON_INTERACTIVE_SUDO_MODE in the
first new test so an outer shell that has set it to `prompt` cannot
change which branch of getSudoPrefix the test exercises. Save and
restore the previous value around the assertion.

CodeRabbit (Minor): prove the platform gate in the non-Linux test by
making the Linux-only systemd-unit and passwordless-sudo probes
throw if reached. With the platform set to darwin, the function
must return before either probe runs; the throwing implementations
make any accidental probe reachability fail the test loudly.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28386750652
Target ref: 1802f77703cdbfa628d0edbacaae3967e8255632
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

…wth guardrail)

The previous round added a try/finally restore of
NEMOCLAW_NON_INTERACTIVE_SUDO_MODE that ended in a small if/else
branch. The codebase-growth-guardrails CI gate rejects any net
new `if` statement in changed test files (tests should stay linear).

Move the save/restore into `beforeEach`/`afterEach` for the
#5716 describe block and extract the assign-or-delete into a
top-level `restoreEnv` helper that uses a ternary statement so
the test bodies remain if-free. Behavior is identical and the
CR isolation contract still holds (the env is forced to its
default at the start of every test in this block and restored
to its original value after).

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28387017215
Target ref: 94b97e75410d848c73fa8bf00b9878d64d9b2d38
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

Ultra advisor PRA-4 (required) called out the missing happy-path
test. Every existing case in the #5716 block exercises either the
new skip-with-warning gate or an early return on a non-Linux
platform. A regression that inverted the new gate's condition
(`!hasPasswordlessSudo()` -> `hasPasswordlessSudo()`) would still
pass every prior test in the block while silently skipping the
loopback override on hosts that have passwordless sudo. Add an
explicit happy-path test that pins the gate's behaviour when
sudo IS available: the function must NOT emit the
"passwordless sudo is not available" warning and must NOT
short-circuit via the new skip path.

Downstream of the gate, the function continues into the live
override path (real runShell against systemd) which then fails
because the test host has no real Ollama systemd unit. The test
tolerates that downstream failure via a process.exit stub that
re-raises a controlled exception; the assertion is scoped to the
gate behaviour, not the downstream side effects, which is the
ONLY thing PRA-4 asks for.

Also rename the describe title from
"ensureOllamaLoopbackSystemdOverride (#5716 non-interactive sudo)"
to "ensureOllamaLoopbackSystemdOverride non-interactive sudo
(#5716)" to satisfy the repository test-title-style check (issue
references must land as a final '(#1234)' suffix). This was the
sole repository-checks failure on the prior commit.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/ollama-systemd.test.ts`:
- Around line 153-176: The happy-path test for
ensureOllamaLoopbackSystemdOverride is still crossing into host-boundary
behavior after the sudo check, so it can mutate the runner environment and rely
on a process.exit stub. Add or use a deterministic seam immediately after the
passwordless-sudo gate (for example, a pure helper or injectable boundary before
the first real systemd/shell action) and update this test to stop at that
boundary while still verifying the “passwordless sudo available” branch.
🪄 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: 46bfb8bb-3ec3-4736-b024-eb84a99e2415

📥 Commits

Reviewing files that changed from the base of the PR and between 94b97e7 and 2a59f5c.

📒 Files selected for processing (1)
  • src/lib/onboard/ollama-systemd.test.ts

Comment thread src/lib/onboard/ollama-systemd.test.ts Outdated
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28396475644
Target ref: 2a59f5c7aaba8e0df84b8ce4ffd16268cae1d0e7
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

…llow-up)

CodeRabbit flagged the prior happy-path test as non-hermetic. With
hasPasswordlessSudoImpl returning true, the test fell through into
the real systemd override path. On a Linux CI runner that has
passwordless sudo, the test would write a real /etc/systemd/system
drop-in and restart Ollama -- a host-boundary side effect under
what is meant to be a pure unit test of the new gate.

Extract the gate's decision into a pure exported helper
`shouldSkipOllamaLoopbackForMissingSudo(sudoPrefix, hasPasswordlessSudo)`
that returns true exactly when the override must be skipped (the
sudoPrefix is "sudo -n" AND the passwordless-sudo probe is false).
The call site in `ensureOllamaLoopbackSystemdOverride` now reads as
a single boolean predicate, the probe creation goes through a small
named `defaultHasPasswordlessSudo` helper, and the prior bigger
inline lambda is gone.

Replace the one happy-path test (which had the process.exit stub
gymnastics) with four small predicate tests over the pure helper:
the skip branch, the happy-path branch with sudo available, both
"sudo" (interactive) cases, and a call-counting test that proves
the probe is short-circuited away when sudoPrefix is "sudo". Every
branch the production code can take is now covered by an assertion
that does not touch the filesystem, systemd, or `runShell`.

12/12 tests pass, Biome clean, no if statements added, typecheck
clean.

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28396641757
Target ref: 5aa85ea0ae31171094e3322bc71ba98ce590e015
Workflow ref: main
Requested jobs: gpu-e2e,gpu-double-onboard-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 2 skipped

Job Result
gpu-double-onboard-e2e ⏭️ skipped
gpu-e2e ⏭️ skipped

@github-actions

Copy link
Copy Markdown
Contributor

Selective E2E Results — ⚠️ No requested jobs ran

Run: 28396768670
Target ref: 2ba3fd5abe9632de5a6b288369fe66fd5763e4a8
Workflow ref: main
Requested jobs: gpu-e2e
Summary: 0 passed, 0 failed, 0 cancelled, 1 skipped

Job Result
gpu-e2e ⏭️ skipped

@github-actions

Copy link
Copy Markdown
Contributor

@cv
cv requested a review from prekshivyas June 30, 2026 08:28

@cv cv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer security follow-up on exact head 890c887:

The earlier missing-sudo implementation was not acceptable because it returned generic not-applicable and could preserve a wildcard Ollama listener. The current head resolves that blocker without weakening the boundary:

  • sudo -n true is checked before any privileged override command.
  • Missing sudo may continue only when systemctl is-active ollama.service succeeds and ss -H -ltn shows at least one :11434 listener with every matching address in IPv4/IPv6 loopback.
  • Wildcard, LAN, missing, unavailable-ss, inactive-service, and unparseable evidence all fail closed with the prompt/passwordless-sudo recovery guidance.
  • A successful request to 127.0.0.1 is deliberately not used as proof because wildcard binds also answer there.
  • The safe branch returns ready; the unsafe branch exits inside the override boundary, so no caller can treat skipped hardening as ordinary non-applicability.

Validation: 16 focused listener/override tests, 56 adjacent install/onboarding tests, CLI type-check, growth/source-shape checks, normal full CLI commit hook, pre-push checks, and Fern docs validation. Exact-head GPU Vitest E2E is run 28430942420.

This is a security-resolution review, not an approval or waiver of required CI, automated advisor, GPU E2E, or independent human approval gates.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: None
Optional E2E targets: None

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: medium

Required E2E targets

  • None. Advisor reported no E2E target impact.

Optional E2E targets

  • None.

Relevant changed files

  • src/lib/onboard/ollama-systemd.ts

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ✅ All requested jobs passed

Run: 28430942420
Workflow ref: fix/5716-ollama-systemd-passwordless-sudo
Requested scenarios: (default — all supported)
Requested jobs: gpu-e2e-vitest
Summary: 1 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
gpu-e2e-vitest ✅ success

@cv
cv merged commit d310396 into main Jun 30, 2026
42 checks passed
@cv
cv deleted the fix/5716-ollama-systemd-passwordless-sudo branch June 30, 2026 08:53
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…VIDIA#5716) (NVIDIA#5996)

<!-- markdownlint-disable MD041 -->
## Summary

Non-interactive Linux onboarding now detects unavailable `sudo -n`
before attempting the Ollama systemd override. It continues only when
the active systemd Ollama listener is positively verified as
loopback-only; wildcard, non-loopback, missing, or unparseable listener
state fails early with actionable sudo guidance.

## Related Issue

Fixes NVIDIA#5716.

## Changes

- Probe passwordless sudo before any systemd override command.
- Inspect the active Ollama listener with non-privileged `systemctl` and
`ss` evidence when sudo is unavailable.
- Continue without rewriting the drop-in only when every `:11434`
listener is IPv4, IPv6, or IPv4-mapped loopback.
- Fail closed before proxy/model setup when listener exposure cannot be
proven safe, and point operators to
`NEMOCLAW_NON_INTERACTIVE_SUDO_MODE=prompt` or passwordless sudo.
- Cover safe and unsafe listener states, sudo-gate behavior, and
adjacent install/onboarding callers.
- Document the non-interactive systemd behavior in the local-inference
guide.

## Security posture

A successful HTTP request to `127.0.0.1:11434` is not accepted as proof
because a wildcard listener answers there too. The fallback requires an
active systemd service plus socket-level listener evidence; otherwise
the established fail-closed path remains in force.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — fail-closed wildcard/non-loopback handling and positive
listener evidence were reviewed in the signed maintainer follow-up;
exact-head automated advisors are also running.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [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) — Fern
completed with 0 errors and the same 2 existing warnings
- [x] 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)

Focused evidence: 16 Ollama systemd/listener tests, 56 adjacent
onboarding/install tests, CLI type-check, 23
source-shape/title/conditional budget tests, a normal commit hook rerun
with the full CLI suite green, pre-push hooks, and Fern docs validation
with 0 errors. The first full hook attempt hit one unrelated 5-second
timeout in `policy-channel-remove-flow.test.ts`; that test passed alone
in 170 ms and the normal hook rerun passed.

---
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

---------

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
prekshivyas added a commit that referenced this pull request Aug 8, 2026
)

## Summary

First incremental step on #6014. The Ollama auth proxy now independently
verifies that the Ollama backend is listening only on loopback before
declaring itself ready, with a structured exit signal the host CLI
renders as a specific actionable remediation. Leaves the existing
root-level systemd loopback override (#5996, #5716) in place for this
PR.

## Why an independent probe in the proxy

The proxy currently trusts the systemd drop-in to keep Ollama bound to
`127.0.0.1`. If a user manually edits `OLLAMA_HOST` to `0.0.0.0`, the
proxy still forwards to `127.0.0.1:11434` successfully (Ollama listens
there too) but Ollama is ALSO publicly reachable on `0.0.0.0:11434`,
bypassing the proxy's bearer-token check entirely.

The new probe runs before `server.listen` and refuses to start with exit
code 2 if it sees any non-loopback listener on the backend port. This
moves bind-policy enforcement off the root-coupled systemd path and onto
the proxy itself; the systemd drop-in becomes pure defense-in-depth that
subsequent PRs can retire.

## Changes

- `scripts/ollama-auth-proxy.mts`:
- Independently enumerates backend listeners through `/proc/net/tcp{,6}`
with an `lsof` fallback and refuses any non-loopback listener before
`server.listen`.
- Recognizes the full `127.0.0.0/8`, `::1`, and IPv4-mapped IPv6
loopback shapes both in listener classification and in deciding whether
a local backend URL requires the probe.
- Writes structured startup failure status for host-side remediation and
retains the explicit audited operator override.
- Is fully checked by `tsconfig.cli.json`; no `@ts-nocheck` suppression
remains.
- Keeps side effects inside `main()`, gated by `import.meta.main`, while
exporting typed helpers for focused tests.

- `src/lib/inference/ollama/proxy.ts`:
- Persist a sentinel path (`~/.nemoclaw/ollama-auth-proxy.status`) and
pass it to the spawned proxy via env
- On proxy spawn, unlink any stale status file so a later read sees the
new proxy's reason
- When the readiness loop observes the proxy gone, read the status file
via `readProxyExitStatus` and render specific remediation via
`printProxyStartupReason` for the `backend-not-loopback` reason; fall
back to existing port-conflict or generic message when no status file is
present

- `test/ollama-auth-proxy-bind-probe.test.ts`: 40 Vitest cases cover
listener parsing, all supported loopback encodings, explicit rejects,
local-versus-remote backend trigger selection, the exit-code contract,
and Linux `/proc` integration.

## What this does NOT do (follow-up PRs per #6014)

- Does not delete `ensureOllamaLoopbackSystemdOverride`. The systemd
drop-in still runs on Linux and stays the authority for Ollama's bind on
a fresh install. The probe is independent enforcement on top, not a
replacement.
- Does not relocate `OLLAMA_CONTEXT_LENGTH` or the Spark
`OLLAMA_LLM_LIBRARY=cuda_v13` overrides off the systemd drop-in. Those
are load-bearing for non-security reasons and belong in a follow-up that
moves them to a config-only path before the drop-in writer can be
deleted.
- Does not add periodic re-probing during proxy lifetime; the current PR
only checks at startup. A follow-up could probe periodically to catch
mid-run bind changes.
- Does not cover Docker-Desktop topologies (WSL + Windows-host Ollama,
WSL + WSL-local Ollama). Those bypass the proxy entirely via
`containerCanReachHostLoopback()` and are out of scope per #6014.

## Verification

- `npx vitest run test/ollama-auth-proxy-bind-probe.test.ts` — 38
passed, 2 platform skips on macOS
- Eight focused Ollama proxy suites — 110 passed, 2 platform skips
- `npm run typecheck:cli` — passed with the proxy script fully
type-checked
- `npm run checks:repository` — repository architecture and source-shape
checks passed
- `npm run docs` — 0 errors, 2 existing warnings
- `src/lib/shields/policy-transition.test.ts` carries the exact one-line
setup-hook stabilization from upstream PR #8572 (commit `78f681e72`)
after current-main CI reproduced the 10-second hook timeout three times
on this PR.


## Related

- Issue #6014 (architectural follow-up)
- PR #5996 (the symptom fix that opened #6014)
- Issue #5716 (the user-visible bug)

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Documentation Writer Review
<!-- Required for code and documentation changes after the changes and
applicable validation are complete. Keep one review checkbox and one
instance of each visible or hidden field. For Evidence, list changed
documentation paths. For documentation-only changes, also state that the
writing rules and documentation style were reviewed. For other results,
explain why no documentation change is needed or why the review is
blocked. For Agent, use a consistent product and surface name, such as
Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all
review changes, put `git rev-parse --short HEAD` and `git rev-parse
--short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review
and refresh that metadata after any new commit. This receipt is advisory
during the data-collection pilot. -->
- [x] Documentation writer subagent reviewed the completed changes
- Result: `docs-updated`
- Evidence: `SECURITY.md`. Independent Codex Desktop review passed for
exact head `74431d39a`. The threat model accurately documents the Ollama
auth proxy loopback bind probe, its full `127.0.0.0/8`, `::1`, and
IPv4-mapped IPv6 loopback coverage, non-loopback refusal, operator
override, unavailable-probe fallback, startup-only enforcement,
regression coverage, and scope limits. Removing `@ts-nocheck` preserves
behavior, the broadened trigger aligns all recognized loopback backend
hostnames with that documented guarantee, and the Vitest setup-hook
timeout change requires no additional documentation.
- Agent: Codex Desktop
<!-- docs-review-head-sha: 74431d3 -->
<!-- docs-review-agents-blob-sha: 12ad395 -->

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Added structured proxy startup failure “status file” and clearer
readiness-loop diagnostics.
* Enhanced the Ollama auth proxy with Bearer-token authentication and
loopback-only backend enforcement.
* **Bug Fixes**
* Improved startup failure reporting by surfacing a specific
“backend-not-loopback” reason and remediation guidance when
misconfigured.
* Improved proxy forwarding error responses with consistent HTTP status
handling.
* **Tests**
* Added Vitest coverage for loopback bind/probe detection, address
classification (proc/net and lsof), and contract constant assertions.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

---------

Signed-off-by: Charan Jagwani <cjagwani@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 <34834085+prekshivyas@users.noreply.github.com>
Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: onboarding Onboarding FSM, provider setup, sandbox launch, or first-run flow platform: linux Affects non-Ubuntu Linux environments

Projects

None yet

3 participants