Skip to content

fix(selfhost): route the last two bare-Number() env knobs through parsePositiveIntEnv + preflight - #10108

Merged
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-env-knobs-parsePositiveIntEnv-10056-v2
Jul 31, 2026
Merged

fix(selfhost): route the last two bare-Number() env knobs through parsePositiveIntEnv + preflight#10108
loopover-orb[bot] merged 2 commits into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-env-knobs-parsePositiveIntEnv-10056-v2

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (src/server.ts) and OLLAMA_NUM_CTX (src/selfhost/ai.ts) were the only two self-host numeric knobs still on the bare-Number(process.env.X ?? "") form (#9157's contract everywhere else). Both fail the two ways #9157 named: a unit-suffixed/separator value ("30s", "30_000") NaN's and silently disables the opt-in with no signal, and a fractional value ("0.5") is accepted — the shutdown deadline loses every race (bulk lock release fires on every shutdown) and ollamaNumCtx floors 0.5 to 0.

Fix

  • src/server.ts reads via parsePositiveIntEnv("LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS", { min: 0, fallback: 0 }), keeping the > 0 gate (unset ⇒ drain-first, byte-identical).
  • ollamaNumCtx reads via parsePositiveIntEnv("OLLAMA_NUM_CTX", { min: 1, fallback: 32_768 }).
  • preflightEnv gains the two paired positiveInteger(...) entries so a malformed value hard-fails boot.
  • Regenerated apps/loopover-ui/src/lib/selfhost-env-reference.ts (npm run selfhost:env-reference), since the structured positiveInteger/parsePositiveIntEnv calls make OLLAMA_NUM_CTX discoverable to the generated reference and re-attribute the shutdown knob's source file.

Defaults, the shutdown drain-first ordering, parsePositiveIntEnv/positiveInteger themselves, the three existing positiveInteger calls, and the ollama provider gate are unchanged. src/server.ts is in codecov.yml's ignore list (entrypoint).

Tests

  • test/unit/selfhost-preflight.test.ts: preflightEnv reports a problem for both knobs on "30s"/"30_000"/"0.5"/"-1" (plus "0" for OLLAMA_NUM_CTX), and ok: true for unset/""/valid integers.
  • test/unit/selfhost-ai.test.ts: ollamaNumCtx() returns 32768 (not 0) for "0.5" and 65536 for "65536".

Closes #10056

…sePositiveIntEnv + preflight

LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS (src/server.ts) and OLLAMA_NUM_CTX
(src/selfhost/ai.ts) were the only self-host numeric knobs still read with a bare
Number(process.env.X ?? ""), the exact form JSONbored#9157 replaced everywhere else. Both fail
the two ways that comment names: a unit-suffixed/separator value ("30s", "30_000")
NaN's and silently disables the opt-in with no signal, and a fractional value ("0.5")
is accepted — the shutdown deadline loses every race (bulk lock release fires on every
shutdown) and ollamaNumCtx floors 0.5 to 0.

Read both via parsePositiveIntEnv (server: { min: 0, fallback: 0 } to keep unset ⇒
wait-for-the-drain; ollama: { min: 1, fallback: 32_768 }), and add the paired
positiveInteger preflight entries so a malformed value hard-fails boot with a clear
message instead of only warning at use time. Defaults, the shutdown drain-first
ordering, and the ollama provider gate are unchanged.

Closes JSONbored#10056
@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 31, 2026 07:30
@loopover-orb

loopover-orb Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-31 07:59:40 UTC

6 files · 1 AI reviewer · no blockers · readiness 83/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This routes the last two bare-Number() env knobs (LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS in src/server.ts, OLLAMA_NUM_CTX in src/selfhost/ai.ts) through parsePositiveIntEnv plus adds matching preflight positiveInteger checks, mirroring the #9157 pattern already used for CRON_INTERVAL_MS/PORT/GITHUB_CACHE_TTL_SECONDS. The min/fallback values chosen (min:0/fallback:0 for the shutdown knob to preserve the drain-first `> 0` gate; min:1/fallback:32_768 for OLLAMA_NUM_CTX) are consistent between the runtime parser call and the new preflight bounds, and the env-reference regen correctly re-attributes both vars' firstReference to src/selfhost/preflight.ts. Tests cover both malformed-value rejection at preflight and the runtime fractional-value regression for ollamaNumCtx().

Nits — 5 non-blocking
  • src/selfhost/preflight.ts:385 — the new `24 * 60 * 60_000` (24h) ceiling for LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS is a fresh magic-number pair; a named constant (or at least a one-line rationale for why 24h is the right ceiling for a shutdown-lock override) would help a future reader the way CRON_INTERVAL_MIN_MS already documents its own bound.
  • src/selfhost/preflight.ts:387 — likewise, `1_000_000` as the OLLAMA_NUM_CTX ceiling has no stated rationale in the diff; worth a short comment on why 1M tokens is the chosen upper bound versus some other value.
  • Confirm `parsePositiveIntEnv`'s own default warn-log behavior on a malformed value doesn't duplicate/conflict with the new preflight hard-fail for the same var — i.e. that preflight running first (boot) means the runtime warn path is effectively unreachable for these two vars in normal self-host boot flow, same as it already is for CRON_INTERVAL_MS/PORT/GITHUB_CACHE_TTL_SECONDS.
  • Consider a shared named constant for the 24h/1_000_000 preflight ceilings if either value has a real-world rationale worth documenting for operators.
  • The PR description claims this closes selfhost(config): route the two remaining bare-Number() env knobs through parsePositiveIntEnv and preflight #10056 with full coverage; the review brief flags partial linked-issue coverage — worth double-checking the issue's full scope is addressed before merge.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #10056
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ❌ 8/20 High review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 239 registered-repo PR(s), 89 merged, 38 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 239 PR(s), 38 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Linked issue satisfaction

Addressed
The diff routes both LOOPOVER_SHUTDOWN_LOCK_RELEASE_AFTER_MS and OLLAMA_NUM_CTX through parsePositiveIntEnv with the exact {min, fallback} pairs specified, preserving the shutdown '>0' gate and default behavior, and adds both positiveInteger(...) preflight checks with matching bounds plus tests covering malformed/valid values. This matches every requirement in the issue, including keeping absence

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 239 PR(s), 38 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Add a concise scope and risk note.
  • Then work through the remaining 2 steps in the Signals table above.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask <question> answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat <question> answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://loopover.ai/docs/loopover-commands

🧪 Experimental — new and may change.

Decision record
  • action: merge · clause: success
  • config: f23f0a68243adbdde53ac593fd85cc9da6be09d85ecf9e54624ee5a49332ff48 · pack: oss-anti-slop · ci: passed
  • record: 03092ae961b82f5b4215fa4a4d4f1828b8e8fbcae245c9de1ef365f30a6de173 (schema v6, head b7b65e5)

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.79%. Comparing base (a7673e2) to head (b7b65e5).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main   #10108       +/-   ##
===========================================
- Coverage   91.88%   79.79%   -12.09%     
===========================================
  Files         930      284      -646     
  Lines      113827    59345    -54482     
  Branches    27466     8882    -18584     
===========================================
- Hits       104588    47354    -57234     
- Misses       7940    11695     +3755     
+ Partials     1299      296     -1003     
Flag Coverage Δ
backend 98.67% <100.00%> (+3.00%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/selfhost/ai.ts 98.38% <100.00%> (-0.01%) ⬇️
src/selfhost/preflight.ts 100.00% <100.00%> (ø)

... and 779 files with indirect coverage changes

@loopover-orb loopover-orb 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.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 45075fd into JSONbored:main Jul 31, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

selfhost(config): route the two remaining bare-Number() env knobs through parsePositiveIntEnv and preflight

1 participant