feat(inference): add Ollama loopback bind probe (step 1 of #6014) - #6054
Merged
Conversation
…#6014) First incremental step on #6014. The 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 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 the 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. Implementation: - scripts/ollama-auth-proxy.js: add `parseProcNetTcpListeners`, `isLoopbackProcAddress`, `probeLinuxLoopbackBind` (parses /proc/net/tcp + /proc/net/tcp6 for LISTEN-state rows on the backend port), and `probeLsofLoopbackBind` (cross-platform fallback via `lsof -nP -iTCP:<port> -sTCP:LISTEN -F n`). The proxy now calls `assertBackendBoundToLoopback` before `server.listen`. On a non-loopback bind it writes a JSON status file (path via `NEMOCLAW_OLLAMA_PROXY_STATUS_FILE`) with reason `backend-not-loopback`, prints an actionable error, and exits with code 2 (`EXIT_BACKEND_NOT_LOOPBACK`). On an unavailable probe (no /proc and no lsof) it falls through with a warning rather than failing closed. `NEMOCLAW_OLLAMA_PROXY_SKIP_BIND_PROBE=1` is an explicit operator override. - The script is restructured so all the side-effect logic (env reads, server.listen, exit) is inside `main()` and gated on `require.main === module`. The pure helpers and constants are exported via `module.exports` so vitest can cover them without spawning the proxy. - src/lib/inference/ollama/proxy.ts: persist a sentinel path (`~/.nemoclaw/ollama-auth-proxy.status`) and pass it to the spawned proxy via `NEMOCLAW_OLLAMA_PROXY_STATUS_FILE`. On proxy spawn, unlink any stale status file so a later read sees the new proxy's reason (or no file when the new proxy starts cleanly). When the readiness loop observes the proxy gone, read the status file via `readProxyExitStatus` and render a specific remediation via `printProxyStartupReason` for the `backend-not-loopback` reason; fall back to the existing port-conflict or generic message when no status file is present. - test/ollama-auth-proxy-bind-probe.test.js: 14 vitest tests on the pure helpers covering LISTEN-state filtering, port matching, address canonicalization, blank-line tolerance, multi-listener output, the three loopback encodings (IPv4, IPv6, IPv4-mapped IPv6), the explicit rejects (wildcard, non-loopback IPv4, IPv6 wildcard), and the EXIT_BACKEND_NOT_LOOPBACK contract value. One Linux-gated integration test reads the host's real /proc/net/tcp at an unused high port to exercise the probe end-to-end. 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 path -- 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. Verification: - `npx vitest run test/ollama-auth-proxy-bind-probe.test.js` -- 14/14 pass, 1 Linux-gated skip on macOS. - `npm run checks` -- Layer import boundaries, source/package boundary, vitest disjoint projects, test title style: all pass. - `npm run typecheck:cli` and `npm run build:cli` clean. - `npx biome check` clean. Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Ollama auth proxy now probes backend loopback binding before startup, writes structured exit status on startup failures, and updates host-side startup handling to read that status. New tests cover the bind-probe helpers and loopback classification. ChangesOllama Auth Proxy Loopback and Startup Hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 74431d3 in the TypeScript / code-coverage/cliThe overall coverage in commit 74431d3 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor (Nemotron Ultra) — BlockedMerge posture: Do not merge until addressed Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 optional E2E recommendation
Blockers
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/inference/ollama/proxy.ts (1)
144-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffStatus-file filesystem I/O should be owned by
state/adapters, not this inference module.This PR adds new filesystem interactions directly in
proxy.ts: the stale-filefs.unlinkSyncinspawnOllamaAuthProxy(Lines 146-150) and thefs.readFileSyncinreadProxyExitStatus(Lines 173-178). Per the layering guidance, persisted/local status-file read/write belongs insrc/lib/state/**(with the path constant), keeping this module focused on orchestration. Consider extracting a smallclearProxyExitStatus()/readProxyExitStatus()(and thePROXY_STATUS_PATHconstant) into the state layer and importing them here.As per path instructions: "any new process/filesystem interactions (e.g., status file read/write) should be owned by state/adapters rather than domain/pure helpers."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/ollama/proxy.ts` around lines 144 - 192, The inference module is owning status-file filesystem I/O, which should live in the state/adapters layer instead. Move the PROXY_STATUS_PATH constant plus the stale-file cleanup and read logic out of proxy.ts by extracting clearProxyExitStatus() and readProxyExitStatus() into src/lib/state/**, then import and use those helpers from spawnOllamaAuthProxy and any exit-status handling paths. Keep proxy.ts focused on orchestration and child-process spawning, with no direct fs.unlinkSync or fs.readFileSync calls.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ollama-auth-proxy.js`:
- Line 70: The loopback gate in `scripts/ollama-auth-proxy.js` is too strict and
the IPv6-mapped constant is wrong: update the `IPV6_MAPPED_IPV4_LOOPBACK_PROC`
value so it matches `::ffff:127.0.0.1` in `/proc/net/tcp6`, and adjust the
classifier logic used by the loopback checks so `isLoopbackAddress`/the related
bind validation accept the full IPv4 loopback range `127.0.0.0/8` instead of
only `127.0.0.1`; make the corresponding changes wherever the same checks are
repeated so valid loopback binds like `127.0.0.2` are allowed.
In `@test/ollama-auth-proxy-bind-probe.test.js`:
- Around line 74-75: The test for isLoopbackProcAddress is currently validating
the IPv6-mapped loopback case with the production constant itself, which makes
the assertion self-referential. Update the fixture in the
ollama-auth-proxy-bind-probe test to use an independently written /proc/net/tcp6
literal for ::ffff:127.0.0.1 instead of importing
IPV6_MAPPED_IPV4_LOOPBACK_PROC, so the test actually verifies the parsing
behavior through the public boundary.
- Around line 98-106: Update the Linux-only probe test in the ollama auth proxy
bind probe suite so it no longer assumes port 64321 is free. In the test that
calls probeLinuxLoopbackBind, first create or discover an actually unused
ephemeral port under test control (for example by binding a temporary server and
reading its assigned port, then probing that port after it is released), and
keep the existing assertions on the returned result. This should make the test
deterministic and independent of incidental host state.
---
Nitpick comments:
In `@src/lib/inference/ollama/proxy.ts`:
- Around line 144-192: The inference module is owning status-file filesystem
I/O, which should live in the state/adapters layer instead. Move the
PROXY_STATUS_PATH constant plus the stale-file cleanup and read logic out of
proxy.ts by extracting clearProxyExitStatus() and readProxyExitStatus() into
src/lib/state/**, then import and use those helpers from spawnOllamaAuthProxy
and any exit-status handling paths. Keep proxy.ts focused on orchestration and
child-process spawning, with no direct fs.unlinkSync or fs.readFileSync calls.
🪄 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: 7d712570-c33c-429e-9201-2c6e19525de2
📒 Files selected for processing (3)
scripts/ollama-auth-proxy.jssrc/lib/inference/ollama/proxy.tstest/ollama-auth-proxy-bind-probe.test.js
…0.0.0/8 (#6014) Address CI reds, CodeRabbit feedback, and advisor PRA-4 on #6054. Concrete fixes: 1. Missing `fs` import in `src/lib/inference/ollama/proxy.ts`. The status-file helpers I added in the prior commit called `fs.unlinkSync` and `fs.readFileSync` but there was no `require("fs")` at the top of the file. This blew up at runtime (cli-test-shards: `ReferenceError: fs is not defined` from the spawn path) AND on Biome lint (`noUndeclaredVariables`). Import fs alongside the existing path/child_process requires. 2. Rename `test/ollama-auth-proxy-bind-probe.test.js` to `.ts` (codebase-growth-guardrails: "this PR adds JavaScript source files. NemoClaw is standardizing on TypeScript for new Node.js code"). Keep the require to `scripts/ollama-auth-proxy.js` intact -- the script file is pre-existing CJS -- but type the exports at the module edge so the test bodies stay strongly typed and pass tsc. 3. CR (Security, Major): "Correct loopback classification before relying on this as the security gate." Two real bugs the reviewer named: a. `IPV6_MAPPED_IPV4_LOOPBACK_PROC` was encoded wrong. /proc/net/tcp6 emits four 32-bit groups, each group's bytes in little-endian order. `::ffff:127.0.0.1` bytes are 00..00 (10) + FF FF + 7F 00 00 01, which grouped-and-reversed is `00000000 00000000 FFFF0000 0100007F`. The earlier constant `0000000000000000FFFF00007F000001` would never appear in real /proc/net/tcp6 output and would silently classify a genuinely-loopback IPv4-mapped IPv6 socket as non-loopback. Correct the constant with an inline derivation comment. b. `isLoopbackProcAddress` accepted only the exact 127.0.0.1 encoding, but IPv4 loopback is the entire 127.0.0.0/8 block. Refactor the classifier to match on the leading (post-reverse) byte being 0x7F rather than a full-string equality on 127.0.0.1. IPv4-mapped IPv6 gets the same widening: any `::ffff:127.x.y.z` is now accepted. 4. CR (Functional, Minor): "Avoid self-referential coverage for the mapped IPv6 fixture." The test imported the production constants and asserted them against the classifier that also consumed them, so an incorrectly encoded fixture would still pass. Rewrite the test's fixtures as independently derived literals (with the byte-derivation shown in comments), and verify the production constants indirectly through the real-world scenarios rather than by identity comparison. 5. CR (Stability, Minor): "Avoid assuming port 64321 is unused." The prior test hardcoded a "probably unused" high port, which would fail environmentally on hosts that actually run something there. Replace with a `beforeEach` that binds a real ephemeral loopback listener via `net.createServer` with port 0, exercises the probe against the port the OS assigned, and tears down in `afterEach`. Adds a second Linux-only case that closes the listener before probing to pin the "no listener at all" branch too. 6. Add module-scope helpers `bindEphemeralLoopback` and `closeServer` so the test bodies remain `if`-free per the repository growth guardrail on conditional branching in changed test files. Verification: - 16/16 tests pass (+2 platform-gated skips) - `npm run checks` clean (layer boundaries, source/package, vitest disjoint projects, test title style) - `npm run typecheck:cli`, `npm run build:cli`, `npx biome check` all clean - Smoke: `isLoopbackProcAddress` correctly classifies 127.0.0.1, 127.0.0.42, ::1, ::ffff:127.0.0.1 as loopback; 0.0.0.0 and 10.0.0.1 as non-loopback Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
area: cli
Collaborator
Author
|
Consolidating advisor justifications after the Concrete items fixed this round:
Recurring justifications:
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/ollama-auth-proxy-bind-probe.test.ts (2)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ProbeResulttype omits the optionalnonLoopbackfield.The actual JS return shape includes an optional
nonLoopbackarray when non-loopback listeners exist, but the localProbeResulttype here only models{ ok, listeners }. Purely cosmetic since no test currently readsnonLoopback, but worth aligning if the suggested negative-path test above is added.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ollama-auth-proxy-bind-probe.test.ts` around lines 13 - 22, The local ProbeResult type is out of sync with probeLinuxLoopbackBind’s actual return shape because it omits the optional nonLoopback field. Update the ProbeResult definition in the ollama-auth-proxy-bind-probe test setup to include the optional nonLoopback array so it matches the runtime shape exposed by probeLinuxLoopbackBind and stays aligned with ProxyExports.
167-216: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider adding a non-loopback negative-path case.
The integration suite covers the "listening on loopback" and "no listener" cases, but not the
ok: false/nonLoopbackpath that this PR's security guarantee actually hinges on (a backend bound to0.0.0.0or similar). Binding an ephemeral server to0.0.0.0and assertingok === falsewith a populatednonLoopbackarray would close the last gap in behavioral confidence for the refusal path. Given PR comments indicate this is an accepted scope tradeoff (unit-level classification + one integration case), this is optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ollama-auth-proxy-bind-probe.test.ts` around lines 167 - 216, The probeLinuxLoopbackBind test suite is missing the negative-path coverage for a non-loopback bind. Add an integration test in describe("probeLinuxLoopbackBind bind probe (`#6014`)") that binds an ephemeral server on 0.0.0.0 (or equivalent non-loopback), then asserts probeLinuxLoopbackBind returns a non-null result with ok set to false and a populated nonLoopback array. Reuse the existing bindEphemeralLoopback/closeServer pattern as a guide, but add a new helper or setup for the non-loopback listener so the refusal path is exercised directly.
🤖 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.
Nitpick comments:
In `@test/ollama-auth-proxy-bind-probe.test.ts`:
- Around line 13-22: The local ProbeResult type is out of sync with
probeLinuxLoopbackBind’s actual return shape because it omits the optional
nonLoopback field. Update the ProbeResult definition in the
ollama-auth-proxy-bind-probe test setup to include the optional nonLoopback
array so it matches the runtime shape exposed by probeLinuxLoopbackBind and
stays aligned with ProxyExports.
- Around line 167-216: The probeLinuxLoopbackBind test suite is missing the
negative-path coverage for a non-loopback bind. Add an integration test in
describe("probeLinuxLoopbackBind bind probe (`#6014`)") that binds an ephemeral
server on 0.0.0.0 (or equivalent non-loopback), then asserts
probeLinuxLoopbackBind returns a non-null result with ok set to false and a
populated nonLoopback array. Reuse the existing
bindEphemeralLoopback/closeServer pattern as a guide, but add a new helper or
setup for the non-loopback listener so the refusal path is exercised directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc87f27d-5be0-4ffd-9bda-5f4c53f7478c
📒 Files selected for processing (3)
scripts/ollama-auth-proxy.jssrc/lib/inference/ollama/proxy.tstest/ollama-auth-proxy-bind-probe.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/ollama/proxy.ts
- scripts/ollama-auth-proxy.js
… classifier (#6014) Ultra advisor PRA-4 flagged the IPv4-mapped IPv6 loopback constant as incorrectly encoded. Traced through the kernel format: /proc/net/tcp6 groups the 16 address bytes into four 32-bit ints, each printed with %08X on native (little-endian) byte order. For ::ffff:127.0.0.1 the address bytes 00..00 (10) + FF FF + 7F 00 00 01 group into u32 numeric values 0x00000000 0x00000000 0xFFFF0000 0x0100007F -- concatenated "0000000000000000FFFF00000100007F". The prior commit had "00000000000000000000FFFF0100007F" which misplaces the FF FF bytes into the wrong 32-bit group. Rather than just patching the constant, harden the whole classifier to reason about semantic address bytes rather than hex string patterns: - Add `decodeProcAddress` that parses a proc-hex column (8 chars for IPv4, 32 chars for IPv6) back into the address byte array in network (IP) order, accounting for the per-group little-endian encoding. Returns null for any input of unexpected length. - Rewrite `isLoopbackProcAddress` to run three explicit shape checks against the decoded bytes: 1. IPv4: first byte == 0x7F (127.0.0.0/8) 2. IPv6: first 15 bytes zero, last byte 0x01 (::1) 3. IPv4-mapped IPv6: 10 zero bytes, then 0xFF 0xFF, then a 127-prefixed IPv4 in bytes 12-15 (::ffff:127.0.0.0/8) All three cases are independent semantic checks, not hex string comparisons, so a subtle encoding bug in one production constant cannot silently break the security-critical classifier. - Fix a related bug the same round found: `probeLinuxLoopbackBind` used to `throw` on any /proc read error other than ENOENT. Under a strict sandbox (EACCES/EPERM) or on a rootless container that hides /proc/net, the throw would crash the proxy instead of falling back to the lsof probe. Return null on any read failure so the caller's fallback chain runs normally. Advisor PRA-9. - Add an explicit audit-trail warning when NEMOCLAW_OLLAMA_PROXY_SKIP_BIND_PROBE=1 disables the probe. The operator-supplied override MUST leave a durable record in stderr so an incident investigator scanning proxy logs can see that enforcement was skipped, when, and how to restore it. Advisor PRA-5. - Add negative test coverage for IPv4-mapped IPv6 with a non-loopback embedded IPv4 (::ffff:10.0.0.1), and for malformed input lengths, so the classifier's shape checks cannot silently accept a wildcard. Verification: - 18/20 tests pass (2 platform-gated skips) - Smoke test covers 10 cases (5 positive loopback shapes, 5 negative including 0.0.0.0, 10.0.0.1, IPv6 wildcard, mapped non-loopback, malformed input) -- all correct - `npm run checks`, `npm run typecheck:cli`, `npx biome check` clean; 0 new if statements in the test file Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
#6014) Address two required advisor items on #6054. Ultra PRA-4 (required): the lsof fallback classifier had the same narrow-127.0.0.1 bug the /proc classifier already fixed. Any host where lsof was the primary loopback probe (macOS, WSL native dockerd without /proc/net/tcp readable) would incorrectly refuse a genuinely-loopback bind on 127.0.0.2 or ::ffff:127.0.0.1 and, worse, mask a mistake as "not-loopback" only-by-accident. Add `isLoopbackLsofAddress` that mirrors the same three semantic loopback shapes as `isLoopbackProcAddress`: - IPv4 dotted quad in 127.0.0.0/8 (accepts every octet after 127) - IPv6 canonical `::1` and bracketed `[::1]` - IPv4-mapped IPv6 in dotted-quad form (`::ffff:127.x.y.z`), bracketed or unbracketed - The literal string `localhost` (lsof may render DNS-resolved hostnames when the operator did not pass `-n`) And explicitly refuse the lsof wildcard token `*`, LAN-scope IPv4 (10.x, 192.168.x), IPv6 wildcard `::`, and any global IPv6. Regular advisor PRA-5 (required): extract the status-file IPC out of `src/lib/inference/ollama/proxy.ts` into `src/lib/inference/ollama/proxy-status.ts`. The protocol (readProxyExitStatus, clearStaleProxyStatus, printProxyStartupReason, ProxyExitStatus type) is self-contained IPC between the auth proxy script and its host caller, and belongs alongside the proxy script's own contract rather than co-mingled with the token, PID, and process-lifecycle logic that proxy.ts otherwise owns. proxy.ts now imports the three helpers from the new module and the monolith shrinks by ~65 lines. Ultra PRA-5 (required): add an explicit THREAT MODEL block to the top of the bind-probe test file naming what the probe is meant to prevent (an on-host bypass of the proxy's token check by an attacker connecting directly to Ollama on a non-loopback bind), which topologies the probe protects (native Linux + macOS + WSL native dockerd; Docker-Desktop paths carved out of scope in the issue body), and what branches the test suite is required to cover for the probe to be sound. Tests cover every branch of both classifiers now (10 lsof cases including the wildcard token reject, on top of the existing 12 /proc cases + 3 probe cases). Verification: - 28/30 tests pass (2 platform-gated skips) - `npm run checks`, `npm run typecheck:cli`, `npx biome check` clean - Test file remains if-free per the growth guardrail Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/inference/ollama/proxy-status.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
@ts-nocheck+require()in a.tsfile undermines type safety.This file is written with
export(ESM) syntax but imports via CommonJSrequire(), and disables type checking entirely with@ts-nocheckon Line 1. This masks any type errors on this module's contract, including theas NodeJS.ErrnoExceptioncasts on Lines 73 and 79, and theparsed.reason/parsed.details/parsed.exitedAtnarrowing on Lines 49-54. Given the surrounding code otherwise uses proper TS types (ProxyExitStatus), consider using standardimportsyntax and removing@ts-nocheckunless there's a project-wide reason (e.g., a legacy JS-interop pattern) for this combination in theinference/ollamacluster.♻️ Suggested fix
-// `@ts-nocheck` // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 @@ -const fs = require("fs"); -const { OLLAMA_PORT } = require("../../core/ports"); +import fs from "fs"; +import { OLLAMA_PORT } from "../../core/ports";Please confirm whether this
require()/@ts-nocheckpattern is intentional/consistent withproxy.tsin this cluster (since that file was noted to have had a missingfsimport bug), or if it should be aligned to standard ESM imports.Also applies to: 19-20
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/ollama/proxy-status.ts` at line 1, Remove the `@ts-nocheck` suppression and align `proxy-status.ts` with the cluster’s normal TypeScript style by replacing CommonJS `require()` usage with standard `import` syntax where applicable. Review the `ProxyExitStatus` parsing and the `NodeJS.ErrnoException` casts in the status handling logic so the types are expressed explicitly instead of being hidden by nocheck. Keep the module’s `export`/ESM contract consistent with `proxy.ts` and only retain any CommonJS interop if there is a documented, project-wide reason for it.
🤖 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.
Nitpick comments:
In `@src/lib/inference/ollama/proxy-status.ts`:
- Line 1: Remove the `@ts-nocheck` suppression and align `proxy-status.ts` with
the cluster’s normal TypeScript style by replacing CommonJS `require()` usage
with standard `import` syntax where applicable. Review the `ProxyExitStatus`
parsing and the `NodeJS.ErrnoException` casts in the status handling logic so
the types are expressed explicitly instead of being hidden by nocheck. Keep the
module’s `export`/ESM contract consistent with `proxy.ts` and only retain any
CommonJS interop if there is a documented, project-wide reason for it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55dc445c-8d28-4b6a-8597-e23b099deecd
📒 Files selected for processing (4)
scripts/ollama-auth-proxy.jssrc/lib/inference/ollama/proxy-status.tssrc/lib/inference/ollama/proxy.tstest/ollama-auth-proxy-bind-probe.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/ollama-auth-proxy-bind-probe.test.ts
- src/lib/inference/ollama/proxy.ts
- scripts/ollama-auth-proxy.js
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ts-nocheck (#6014) Address the concrete blockers on #6054 that CodeRabbit and Ultra flagged this round: CodeQL / CodeRabbit (both): unused fs import in proxy.ts. After the prior commit extracted the status-file IPC into proxy-status.ts, nothing in proxy.ts referenced fs anymore. Remove the require line; keeps the file dependency-minimal and clears the "unused import" alert. Ultra PRA-6 (required): the new proxy-status.ts file had @ts-nocheck at the top, inherited from proxy.ts's convention. That was wrong for a fresh file -- new code should not import proxy.ts's opt-out. Drop @ts-nocheck; typecheck stays clean without it. Any future addition to proxy-status.ts is now under full tsc. Ultra PRA-7 (required): shrink proxy.ts further by relocating the status-file wire protocol constants into proxy-status.ts: - New export PROXY_STATUS_ENV = "NEMOCLAW_OLLAMA_PROXY_STATUS_FILE" so a future rename of the wire env var only touches one file. - New export defaultProxyStatusPath(stateDir) that constructs the status file path under a caller-supplied adapter state dir; the path segment "ollama-auth-proxy.status" no longer lives in proxy.ts. proxy.ts now: - imports PROXY_STATUS_ENV and defaultProxyStatusPath from ./proxy-status - Uses [PROXY_STATUS_ENV]: PROXY_STATUS_PATH in the spawn env (computed keys keep the wire contract single-sourced) - Drops the multi-line comment block that explained the wire protocol (it's now co-located with the exports in proxy-status.ts, next to the actual code) Net line delta for proxy.ts vs main: +20 (was +25 last commit, +18 before that). proxy-status.ts is 126 lines (was 106; +20 for the two new exports and a short header comment). The status IPC contract now has one owner file that a future refactor (proxy retire, socket-based IPC, whatever) can rewrite without touching the token/PID/process lifecycle logic in proxy.ts. Verification: - 28/30 tests pass (2 platform-gated skips) - `npm run checks` clean (layer boundaries, source/package, vitest disjoint, test title style) - `npm run typecheck:cli`, `npm run build:cli`, `npx biome check` clean Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/inference/ollama/proxy-status.ts (1)
19-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftStatus-file I/O lives outside
src/lib/state/**.
readProxyExitStatusandclearStaleProxyStatusperform directfsreads/unlinks for the persisted status file from withinsrc/lib/inference/ollama/, rather than through asrc/lib/state/**module. This makes it harder to inject fakes in tests and mixes protocol/status logic with raw filesystem access.Consider extracting the actual file I/O (
readFileSync,unlinkSync) into a smallsrc/lib/state/**helper that this module calls, keepingproxy-status.tsfocused on the wire-protocol shape/parsing.As per path instructions,
src/**: "keep product logic insrc/lib/domain/**, host/OS/process + fs interactions insrc/lib/adapters/**, and persisted status-file I/O insrc/lib/state/**; avoid direct process/filesystem calls in domain helpers and prefer adapters/state modules so tests can inject fakes (relevant to the proxy startup probe + status-file IPC/status rendering touched undersrc/lib/inference/**)."Also applies to: 51-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/inference/ollama/proxy-status.ts` at line 19, `proxy-status.ts` is mixing status parsing with direct persisted-file I/O, which should live in a `src/lib/state/**` helper instead. Move the `readFileSync` and `unlinkSync` behavior used by `readProxyExitStatus` and `clearStaleProxyStatus` into a small state module, then have `proxy-status.ts` call that helper so the module stays focused on proxy-status shape/parsing and tests can inject fakes more easily.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/lib/inference/ollama/proxy-status.ts`:
- Line 19: `proxy-status.ts` is mixing status parsing with direct persisted-file
I/O, which should live in a `src/lib/state/**` helper instead. Move the
`readFileSync` and `unlinkSync` behavior used by `readProxyExitStatus` and
`clearStaleProxyStatus` into a small state module, then have `proxy-status.ts`
call that helper so the module stays focused on proxy-status shape/parsing and
tests can inject fakes more easily.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dbc3c605-e6a9-4a23-ae43-c81a0fd5f796
📒 Files selected for processing (3)
scripts/ollama-auth-proxy.jssrc/lib/inference/ollama/proxy-status.tssrc/lib/inference/ollama/proxy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/inference/ollama/proxy.ts
- scripts/ollama-auth-proxy.js
Address Ultra advisor PRA-2 (required) and justify PRA-3.
Ultra PRA-2 (required): the threat model for the Ollama auth
proxy bind probe was documented only inline at the top of the
test file. Ultra wanted a durable, project-level location so a
reviewer or auditor scanning the repo can find it without a
grep. Add a "Threat Models" section to SECURITY.md with a
component-level threat model for the bind probe covering:
- Summary of the auth proxy's role and Ollama's own lack of auth
- The threat the probe blocks (on-host bypass of the proxy's
token check by an attacker connecting directly to Ollama on a
non-loopback interface)
- The guarantee the probe adds (walks /proc/net/tcp{,6} or falls
back to lsof, refuses to start with EXIT_BACKEND_NOT_LOOPBACK
if any listener is non-loopback, loopback = full 127.0.0.0/8
+ ::1 + ::ffff:127.0.0.0/8)
- Where the guarantee ends (Docker-Desktop topologies out of
scope, operator override with audit warning, probe-unavailable
fail-open path with the systemd loopback override as defense in
depth, startup-only probe, non-Ollama providers uncovered)
- Where the guarantee is enforced (the test file that pins each
branch of both classifiers)
Ultra PRA-3 + Regular PRA-6 (proxy.ts monolith growth): keeps
@ts-nocheck at the top of proxy.ts with a code comment
explaining why the removal is out of scope for this PR. tsc
without @ts-nocheck emits ~14 noImplicitAny errors on
pre-existing callback parameters (sleep, model, err, code,
bytes, pct, line, tag, ...) scattered through the 992-line file.
Typing each one is a separate refactor that belongs in its own
PR, not co-mingled with the bind-probe security change. This PR
only touches the status-file IPC seam and the spawn env; the
net +20 lines do NOT extend the untyped surface with new
implicit-any callbacks, so the @ts-nocheck-suppressed area's
per-line risk is unchanged.
Verification:
- 28/30 tests pass (2 platform-gated skips)
- `npm run checks`, `npm run typecheck:cli`, `npm run build:cli`
clean
- `npx biome check` on all 4 code files + SECURITY.md clean
- `npx markdownlint-cli2 SECURITY.md` clean
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
wscurran
added
area: providers
6 tasks
Collaborator
|
Release sweep: deferring this PR from The implementation remains dependent on the daemon-ownership decision in #6014, conflicts with current Once #6014's design is settled, the safest path is a fresh, narrow, compliant PR against current |
This was referenced Jul 26, 2026
cv
added a commit
that referenced
this pull request
Jul 26, 2026
<!-- markdownlint-disable MD041 --> ## Summary Conflict publication now constructs validated GitHub trees from the recorded `main` tree instead of the stale PR head. Production requests for #7253 and #6054 shrink from 1,180 and 3,754 entries to 13 and 6 entries. ## Related Issue Follow-up to #7542. ## Changes - Build each GitHub tree from the recorded base SHA and the final tree delta. - Read deletion entries from the base tree. - Keep parent blob reuse, final-tree equality, verified commits, and atomic ref updates unchanged. - Model 100 `main`-only files and a PR-side deletion in the publisher regression. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal GitHub publication mechanics and no user-facing contract. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Maintainer approved this repair after [production run 30193861556](https://github.com/NVIDIA/NemoClaw/actions/runs/30193861556) demonstrated the failure. Trust boundaries do not change. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The change repairs internal GitHub tree publication. It does not change commands, configuration, output, or supported behavior. - Agent: Codex Desktop <!-- docs-review-head-sha: 424209c --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: `npx vitest run --project integration test/pr-merge-conflict-fixer.test.ts` passed 12/12 after formatting. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this focused publisher fix. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved pull request conflict resolution publishing to correctly reflect files added, modified, or deleted on each branch. * Ensured resolved updates are built from the correct base revision, preventing unrelated or outdated files from being included. * Improved publication accuracy when the target branch has advanced with additional changes. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
apurvvkumaria
requested changes
Aug 3, 2026
apurvvkumaria
left a comment
apurvvkumaria
left a comment
Collaborator
There was a problem hiding this comment.
Request changes — reviewed exact head 22c293f. The refactored scripts/ollama-auth-proxy.mts invokes main only under if (import.meta.main), but NemoClaw supports Node >=22.16.0 and Node added import.meta.main in 22.18.0. On supported Node 22.16 and 22.17 that property is undefined, so executing the script exits successfully without calling main or opening the proxy listener. Reproduction on either supported version: set OLLAMA_PROXY_TOKEN, run node scripts/ollama-auth-proxy.mts, and observe exit 0 with no process listening on OLLAMA_PROXY_PORT; the parent readiness path then fails every proxy-fronted Ollama workflow. Replace the guard with Node-22.16-compatible entrypoint detection using import.meta.url and process.argv[1], and add a minimum-version execution test. Raising the repository minimum plus all documented/runtime constraints would also resolve it, but is substantially broader. This is attributable to the PR and makes a supported workflow unusable.
apurvvkumaria
approved these changes
Aug 3, 2026
apurvvkumaria
left a comment
apurvvkumaria
left a comment
Collaborator
There was a problem hiding this comment.
Re-review correction for exact head 22c293f: I independently reproduced that import.meta.main is absent on Node 22.16, but the exact base and head now require and consistently document Node 22.19.0 or newer, where the entry guard works. The earlier compatibility concern therefore is not attributable to supported behavior on this revision and is withdrawn. I found no blocking correctness, security, compatibility, or regression defect. The failing DCO check still needs the PR-body declaration required by repository policy, but that is not a code-review blocker.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 editsOLLAMA_HOSTto0.0.0.0, the proxy still forwards to127.0.0.1:11434successfully (Ollama listens there too) but Ollama is ALSO publicly reachable on0.0.0.0:11434, bypassing the proxy's bearer-token check entirely.The new probe runs before
server.listenand 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:/proc/net/tcp{,6}with anlsoffallback and refuses any non-loopback listener beforeserver.listen.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.tsconfig.cli.json; no@ts-nochecksuppression remains.main(), gated byimport.meta.main, while exporting typed helpers for focused tests.src/lib/inference/ollama/proxy.ts:~/.nemoclaw/ollama-auth-proxy.status) and pass it to the spawned proxy via envreadProxyExitStatusand render specific remediation viaprintProxyStartupReasonfor thebackend-not-loopbackreason; fall back to existing port-conflict or generic message when no status file is presenttest/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/procintegration.What this does NOT do (follow-up PRs per #6014)
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.OLLAMA_CONTEXT_LENGTHor the SparkOLLAMA_LLM_LIBRARY=cuda_v13overrides 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.containerCanReachHostLoopback()and are out of scope per refactor(onboard): decouple Ollama loopback hardening from root-level systemd surgery #6014.Verification
npx vitest run test/ollama-auth-proxy-bind-probe.test.ts— 38 passed, 2 platform skips on macOSnpm run typecheck:cli— passed with the proxy script fully type-checkednpm run checks:repository— repository architecture and source-shape checks passednpm run docs— 0 errors, 2 existing warningssrc/lib/shields/policy-transition.test.tscarries the exact one-line setup-hook stabilization from upstream PR fix(status): wait for inference after gateway recovery #8572 (commit78f681e72) after current-main CI reproduced the 10-second hook timeout three times on this PR.Related
Type of Change
Documentation Writer Review
docs-updatedSECURITY.md. Independent Codex Desktop review passed for exact head74431d39a. The threat model accurately documents the Ollama auth proxy loopback bind probe, its full127.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-nocheckpreserves 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.Summary by CodeRabbit
Signed-off-by: Charan Jagwani cjagwani@nvidia.com