Skip to content

fix(onboard): color preflight WARN/ERROR check lines (#6004) - #6017

Merged
cv merged 18 commits into
mainfrom
fix/6004-preflight-warn-error-color
Jul 4, 2026
Merged

fix(onboard): color preflight WARN/ERROR check lines (#6004)#6017
cv merged 18 commits into
mainfrom
fix/6004-preflight-warn-error-color

Conversation

@jason-ma-nv

@jason-ma-nv jason-ma-nv commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

The nemoclaw onboard preflight printed its // check lines in the default terminal color, so warnings and failures were visually indistinguishable from passing checks in the lengthy preflight output. This colors warnings yellow and failures red (passing /INFO lines stay default), making problems easy to spot.

Related Issue

Fixes #6004

Changes

  • src/lib/cli/terminal-style.ts: add warnLine() / failLine() helpers ( yellow, red). Color is auto-suppressed via the existing useColor gate (!NO_COLOR && stdout.isTTY), so NO_COLOR=1 and non-TTY/CI output stay plain text with no escape sequences — satisfying the issue's NO_COLOR requirement with no new gating.
  • src/lib/onboard/bridge-dns-preflight.ts + src/lib/onboard/sandbox-gpu-preflight.ts: route the dedicated preflight modules' / lines through the helpers (16 sites).
  • src/lib/cli/terminal-style.test.ts: unit tests for the colored TTY path, the NO_COLOR=1 plain path, and the non-TTY plain path.

Scoped to the two dedicated preflight-check modules to keep the diff reviewable and the output consistent. Passing /INFO lines are intentionally left in the default color per the issue. Follow-ups (not in this PR): the few symbol warnings in the onboard.ts [1/8] block and the bare-text hard-failure lines (e.g. "Docker is not reachable") use a different output shape and can be unified separately.

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: terminal color only; no command/flag/behavior surface change and no documented preflight-output contract.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: onboarding preflight; change is presentation-only (ANSI wrapping via the existing useColor gate), no control-flow/exit-code change. The two modules' 24 existing unit tests still pass (non-TTY → plain text, unchanged), plus new color/NO_COLOR/non-TTY tests.
  • 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)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Jason Ma jama@nvidia.com

Summary by CodeRabbit

  • New Features
    • Added stderr-specific warnLine/failLine helpers for consistent ⚠/✗ CLI preflight formatting.
    • Introduced shared onboarding preflight message helpers to centralize runtime, memory/swap, and messaging-provider guidance.
  • Bug Fixes
    • Fixed stream-accurate ANSI coloring so warning/error prefixes follow stderr (stdout won’t leak styling).
    • Ensures ANSI output is suppressed under NO_COLOR/limited-color conditions.
  • Tests
    • Expanded deterministic coverage for severity/color behavior across terminal-style, bridge reachability, onboarding preflight messages, and HTTP proxy preflight output.

Preflight check results printed plain ✓/⚠/✗ lines in the default terminal color, so warnings and failures were visually indistinguishable from passing checks in the lengthy onboard preflight output.

Add warnLine/failLine helpers in terminal-style.ts (⚠ yellow, ✗ red) — color auto-suppressed under NO_COLOR or non-TTY via the existing useColor gate, so CI stays plain text. Apply them to the dedicated preflight-check modules (bridge-dns-preflight, sandbox-gpu-preflight). ✓/INFO lines are left in default color per the issue.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Jason Ma <jama@nvidia.com>
@jason-ma-nv jason-ma-nv self-assigned this Jun 30, 2026
@coderabbitai

coderabbitai Bot commented Jun 30, 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

Adds stderr-aware severity formatting for CLI preflight lines, shared onboarding message helpers, and updates bridge-DNS, sandbox-GPU, gateway reachability, HTTP proxy, and runtime preflight output to use the new colored warning/error rendering.

Changes

Colored preflight output helpers

Layer / File(s) Summary
Severity line helpers
src/lib/cli/terminal-style.ts, src/lib/cli/terminal-style.test.ts
Adds stderr-aware warnLine and failLine formatting and expands tests to cover TTY, redirected, NO_COLOR, and true-color cases.
Shared onboarding preflight messages
src/lib/onboard/preflight-messages.ts, src/lib/onboard/preflight-messages.test.ts, src/lib/onboard.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/http-proxy-preflight.ts, src/lib/onboard/http-proxy-preflight.test.ts
Adds reusable onboarding message printers and routes low-memory, swap creation, missing provider, runtime, and HTTP proxy warning output through them.
Bridge DNS preflight output
src/lib/onboard/bridge-dns-preflight.ts
Reformats bridge container, container DNS, and host DNS warnings and failures through the shared severity line helpers.
Sandbox GPU preflight output
src/lib/onboard/sandbox-gpu-preflight.ts
Reformats sandbox GPU config, runtime, proof, CUDA, and CDI preflight messages through the shared severity line helpers.
Gateway reachability and UFW fallback output
src/lib/onboard/gateway-sandbox-reachability.ts, src/lib/onboard/gateway-sandbox-reachability-severity.test.ts
Formats sandbox bridge reachability warnings and failures with the shared severity helpers and updates the UFW fallback warning path.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#3441: Touches src/lib/onboard/gateway-sandbox-reachability.ts and the same reachability warning/failure paths updated here.
  • #6004: Directly matches the preflight warning/error colorization change addressed by this PR.

Suggested labels: bug-fix

Suggested reviewers: ericksoa, jyaunches, cjagwani

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.67% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly matches the main change: colorizing onboard preflight WARN/ERROR lines.
Linked Issues check ✅ Passed The PR adds stream-aware ANSI coloring for WARN and failed preflight lines, keeps INFO unchanged, and suppresses color in NO_COLOR/plain-text cases.
Out of Scope Changes check ✅ Passed The additional preflight paths and tests all support the same onboard severity-coloring fix, with no unrelated feature work apparent.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/6004-preflight-warn-error-color

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

@github-code-quality

github-code-quality Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/6004-preflight-w... 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/6004-preflight-w... 9e06a53 +/-
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/6004-preflight-w... branch is 69%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/6004-preflight-w... 9e06a53 +/-
src/lib/shields...nsition-lock.ts 87%
src/lib/actions...all/run-plan.ts 81%
src/lib/onboard/preflight.ts 80%
src/lib/state/o...oard-session.ts 78%
src/lib/state/sandbox.ts 74%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/shields/index.ts 68%
src/lib/actions...licy-channel.ts 60%
src/lib/policy/index.ts 60%
src/lib/onboard.ts 22%

Updated July 04, 2026 00:50 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-1: Offset onboarding hotspot growth from the presentation refactor; then add or justify PRA-T1.
Open items: 1 required · 0 warnings · 0 suggestions · 4 test follow-ups
Since last review: 1 prior item resolved · 1 still applies · 1 new item found

Action checklist

  • PRA-1 Fix: Offset onboarding hotspot growth from the presentation refactor in src/lib/onboard.ts:518
  • 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.ts:518 Move or shrink enough touched presentation code so `src/lib/onboard.ts` is net-neutral or net-negative for this PR, and consider moving the new CDI severity tests into a focused severity/output test file or otherwise offsetting that hotspot growth. Keep security and lifecycle control flow in place; only extract or reduce message formatting, imports, and test-only severity coverage.

🚨 Required before merge

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

PRA-1 Required — Offset onboarding hotspot growth from the presentation refactor

  • Location: src/lib/onboard.ts:518
  • Category: architecture
  • Problem: This PR is a terminal-output presentation refactor and already extracts shared severity message helpers, but the top-level onboarding entrypoint still grows in a heavily edited lifecycle file. The synthetic drift context reports `src/lib/onboard.ts` as a blocker-level large-file hotspot with `baseLines: 5385`, `headLines: 5406`, and `delta: 21`. The same drift pass also flags `src/lib/onboard/preflight-cdi.test.ts` growing by 65 lines in an existing large test hotspot. The prior advisor's gateway-cleanup warning finding is resolved, but its `onboard.ts` growth finding still applies.
  • Impact: Leaving presentation-only changes with net growth in `onboard.ts` makes future reviews and merges harder in code that coordinates sandbox, gateway, credential, inference, and runtime lifecycle behavior. Growing the CDI monolith test file for severity-rendering coverage adds similar avoidable review friction when the new tests could live in a focused severity test module.
  • Required action: Move or shrink enough touched presentation code so `src/lib/onboard.ts` is net-neutral or net-negative for this PR, and consider moving the new CDI severity tests into a focused severity/output test file or otherwise offsetting that hotspot growth. Keep security and lifecycle control flow in place; only extract or reduce message formatting, imports, and test-only severity coverage.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Use read-only diff evidence such as `git diff --numstat origin/main...HEAD -- src/lib/onboard.ts src/lib/onboard/preflight-cdi.test.ts` or the synthetic monolith deltas and confirm `src/lib/onboard.ts` no longer grows from this presentation refactor and the CDI severity coverage is not expanding the existing large CDI test hotspot without an offset.
  • Missing regression test: The existing onboarding entrypoint/codebase-growth guard should catch this. If it does not, add or update a size-budget assertion that fails when presentation-only onboarding changes grow `src/lib/onboard.ts` or other current large-file hotspots without an offsetting extraction.
  • Done when: The required change is committed and verification passes: Use read-only diff evidence such as `git diff --numstat origin/main...HEAD -- src/lib/onboard.ts src/lib/onboard/preflight-cdi.test.ts` or the synthetic monolith deltas and confirm `src/lib/onboard.ts` no longer grows from this presentation refactor and the CDI severity coverage is not expanding the existing large CDI test hotspot without an offset.
  • Evidence: Previous advisor review required offsetting `onboard.ts` growth. Current drift evidence still reports `src/lib/onboard.ts` +21 lines as a blocker-level monolith delta and `src/lib/onboard/preflight-cdi.test.ts` +65 lines as a blocker-level hotspot delta.
Simplification opportunities: 1 possible cut, net -21 lines possible

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

  • PRA-1 shrink (src/lib/onboard.ts:518): Remaining presentation/output churn in `src/lib/onboard.ts` and severity-only additions in the large CDI test file.
    • Replacement: Keep the new `preflight-messages.ts` direction, but move enough message formatting/import churn out of `onboard.ts` and place CDI severity-rendering tests in a focused severity test or shared test helper rather than expanding the existing CDI monolith.
    • Net: -21 lines
    • Safety boundary: Do not simplify away preflight gates, exit behavior, credential redaction, sandbox/network policy checks, rollback behavior, runtime remediation guidance, or the required color/NO_COLOR regression tests.
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 — Run or document targeted runtime validation that `nemoclaw onboard >stdout.log` with a warning/failure preflight path keeps WARN/ERROR first lines colored on color-capable stderr even while stdout is redirected.. Unit coverage is strong and close to the changed code, including color-capable stderr, non-color stderr, `NO_COLOR`, fatal paths, warning paths, proxy redaction, and gateway cleanup behavior. Static review still cannot prove real shell redirection and Node stream capability behavior for the packaged CLI in an actual terminal.
  • PRA-T2 Runtime validation — Run or document targeted runtime validation that `NO_COLOR=1 nemoclaw onboard` on a path emitting both a warning and a fatal preflight line writes no `\x1b[` escape sequences to captured stderr.. Unit coverage is strong and close to the changed code, including color-capable stderr, non-color stderr, `NO_COLOR`, fatal paths, warning paths, proxy redaction, and gateway cleanup behavior. Static review still cannot prove real shell redirection and Node stream capability behavior for the packaged CLI in an actual terminal.
  • PRA-T3 Runtime validation — Run or document targeted runtime validation that Docker-unavailable preflight renders the first fatal line red on color-capable stderr and still exits before remediation exactly as before.. Unit coverage is strong and close to the changed code, including color-capable stderr, non-color stderr, `NO_COLOR`, fatal paths, warning paths, proxy redaction, and gateway cleanup behavior. Static review still cannot prove real shell redirection and Node stream capability behavior for the packaged CLI in an actual terminal.
  • PRA-T4 Runtime validation — Run or document targeted runtime validation that the preflight gateway-cleanup defer path emits `Gateway will be recreated...` through the shared warning renderer on stderr, colored with a color-capable stderr and plain under `NO_COLOR=1`.. Unit coverage is strong and close to the changed code, including color-capable stderr, non-color stderr, `NO_COLOR`, fatal paths, warning paths, proxy redaction, and gateway cleanup behavior. Static review still cannot prove real shell redirection and Node stream capability behavior for the packaged CLI in an actual terminal.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Required — Offset onboarding hotspot growth from the presentation refactor

  • Location: src/lib/onboard.ts:518
  • Category: architecture
  • Problem: This PR is a terminal-output presentation refactor and already extracts shared severity message helpers, but the top-level onboarding entrypoint still grows in a heavily edited lifecycle file. The synthetic drift context reports `src/lib/onboard.ts` as a blocker-level large-file hotspot with `baseLines: 5385`, `headLines: 5406`, and `delta: 21`. The same drift pass also flags `src/lib/onboard/preflight-cdi.test.ts` growing by 65 lines in an existing large test hotspot. The prior advisor's gateway-cleanup warning finding is resolved, but its `onboard.ts` growth finding still applies.
  • Impact: Leaving presentation-only changes with net growth in `onboard.ts` makes future reviews and merges harder in code that coordinates sandbox, gateway, credential, inference, and runtime lifecycle behavior. Growing the CDI monolith test file for severity-rendering coverage adds similar avoidable review friction when the new tests could live in a focused severity test module.
  • Required action: Move or shrink enough touched presentation code so `src/lib/onboard.ts` is net-neutral or net-negative for this PR, and consider moving the new CDI severity tests into a focused severity/output test file or otherwise offsetting that hotspot growth. Keep security and lifecycle control flow in place; only extract or reduce message formatting, imports, and test-only severity coverage.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Use read-only diff evidence such as `git diff --numstat origin/main...HEAD -- src/lib/onboard.ts src/lib/onboard/preflight-cdi.test.ts` or the synthetic monolith deltas and confirm `src/lib/onboard.ts` no longer grows from this presentation refactor and the CDI severity coverage is not expanding the existing large CDI test hotspot without an offset.
  • Missing regression test: The existing onboarding entrypoint/codebase-growth guard should catch this. If it does not, add or update a size-budget assertion that fails when presentation-only onboarding changes grow `src/lib/onboard.ts` or other current large-file hotspots without an offsetting extraction.
  • Done when: The required change is committed and verification passes: Use read-only diff evidence such as `git diff --numstat origin/main...HEAD -- src/lib/onboard.ts src/lib/onboard/preflight-cdi.test.ts` or the synthetic monolith deltas and confirm `src/lib/onboard.ts` no longer grows from this presentation refactor and the CDI severity coverage is not expanding the existing large CDI test hotspot without an offset.
  • Evidence: Previous advisor review required offsetting `onboard.ts` growth. Current drift evidence still reports `src/lib/onboard.ts` +21 lines as a blocker-level monolith delta and `src/lib/onboard/preflight-cdi.test.ts` +65 lines as a blocker-level hotspot delta.

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 30, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: cloud-onboard, onboard-negative-paths, gateway-drift-preflight
Optional E2E: onboard-resume, gpu-double-onboard, docs-validation

Dispatch hint: cloud-onboard,onboard-negative-paths,gateway-drift-preflight

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • cloud-onboard (high): Required because src/lib/onboard.ts and shared preflight modules changed in ways that can affect full hosted onboarding, including preflight diagnostics, gateway cleanup warnings, and sandbox creation flow. This job exercises the real installer/OpenShell/hosted-inference onboarding path.
  • onboard-negative-paths (low): Required because fatal and warning preflight output changed, including Docker-not-reachable, unsupported runtime, DNS, proxy, and severity rendering. This job covers CLI negative-path exit/output contracts where regressions would be user-visible before sandbox creation.
  • gateway-drift-preflight (low): Required because preflight gateway cleanup/defer behavior and warnings changed. This job targets gateway drift preflight with the real repo CLI and PATH-resolved OpenShell/Docker shims without mutating live Docker/OpenShell state.

Optional E2E

  • onboard-resume (medium): Useful because bridge/DNS and fatal runtime preflight are reused as resume backstops. The PR does not appear to change resume state-machine orchestration, so this is optional rather than required by the onboarding resume rule.
  • gpu-double-onboard (high): Useful if a GPU runner is available because sandbox GPU/CDI preflight diagnostics now use shared severity helpers. This provides live confidence that GPU onboarding still passes beyond unit-level CDI/GPU message coverage.
  • docs-validation (low): Useful because ci/platform-matrix.json and generated docs were touched. This is not merge-blocking for runtime behavior, but it can catch drift in launch-facing platform/inference documentation.

New E2E recommendations

  • CLI terminal severity output (low): Existing live onboarding E2E captures stdout/stderr but does not appear to exercise a real pseudo-TTY/redirection matrix for colored warn/error lines. The core behavior is unit-tested here, but a small integration/E2E would protect the user-visible contract for onboard >log and onboard 2>log.
    • Suggested test: Add a lightweight CLI output contract test that runs an onboarding preflight negative path under controlled stdout/stderr TTY or pipe conditions and asserts warning/error severity lines are emitted to stderr without leaking ANSI when stderr is redirected.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: cloud-onboard,onboard-negative-paths,gateway-drift-preflight

@github-actions

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Recommendation

Required Vitest E2E scenarios: cloud-onboard-vitest
Optional Vitest E2E scenarios: gpu-e2e-vitest

Dispatch required Vitest E2E scenarios:

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

Workflow run

Full Vitest E2E advisor summary

Vitest E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required Vitest E2E scenarios

  • cloud-onboard-vitest: Changes touch onboarding preflight output helpers and bridge/DNS preflight code used by live non-interactive onboarding; run the focused cloud onboard Vitest job to exercise the real CLI preflight/onboard path.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=cloud-onboard-vitest

Optional Vitest E2E scenarios

  • gpu-e2e-vitest: Changes also touch sandbox GPU preflight warning/failure formatting. This GPU-runner job is the closest live Vitest coverage for GPU onboard/preflight behavior, but it requires a special GPU runner and the primary change is output formatting.
    • Dispatch: gh workflow run e2e-vitest-scenarios.yaml --ref <pr-head-ref> --field jobs=gpu-e2e-vitest

Relevant changed files

  • src/lib/cli/terminal-style.ts
  • src/lib/onboard/bridge-dns-preflight.ts
  • src/lib/onboard/sandbox-gpu-preflight.ts

@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/bridge-dns-preflight.ts`:
- Around line 214-216: The inconclusive DNS warning path is only partially
migrated to warnLine, leaving other branches in bridge-dns-preflight on the old
hardcoded ⚠ formatting. Update the remaining inconclusive warning sites in the
same entrypoint to call warnLine consistently so all DNS preflight warnings
share the TTY/NO_COLOR-aware formatting, and keep any ANSI styling centralized
in terminal-style.ts rather than duplicating it in onboard preflight code.
🪄 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: 33a327ef-aeb8-412e-94c8-20b19a75647b

📥 Commits

Reviewing files that changed from the base of the PR and between 59c8680 and bb53acb.

📒 Files selected for processing (4)
  • src/lib/cli/terminal-style.test.ts
  • src/lib/cli/terminal-style.ts
  • src/lib/onboard/bridge-dns-preflight.ts
  • src/lib/onboard/sandbox-gpu-preflight.ts

Comment thread src/lib/onboard/bridge-dns-preflight.ts
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Fix PRA-7: onboard.ts grew by +21 lines despite preflight-messages.ts extraction — CI blocker; then add or justify PRA-T1.
Open items: 2 required · 13 warnings · 3 suggestions · 8 test follow-ups
Since last review: 1 prior item resolved · 13 still apply · 2 new items found

Action checklist

  • PRA-7 Fix: onboard.ts grew by +21 lines despite preflight-messages.ts extraction — CI blocker in src/lib/onboard.ts:1
  • PRA-8 Fix: stderr-keyed coloring fixes ANSI leakage bug — security-positive change in src/lib/cli/terminal-style.ts:20
  • PRA-1 Resolve or justify: Source-of-truth review needed: src/lib/cli/terminal-style.ts:7-12
  • PRA-2 Resolve or justify: Source-of-truth review needed: src/lib/cli/terminal-style.test.ts:17-42
  • PRA-3 Resolve or justify: Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:57-58
  • PRA-4 Resolve or justify: Source-of-truth review needed: src/lib/onboard/preflight-runtime-resources.test.ts:30
  • PRA-5 Resolve or justify: Source-of-truth review needed: src/lib/onboard/preflight-messages.ts:40-56
  • PRA-6 Resolve or justify: Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:33-45
  • PRA-9 Resolve or justify: Legacy color constants frozen at import time — source-of-truth review needed in src/lib/cli/terminal-style.ts:7
  • PRA-10 Resolve or justify: stubStream/restoreStream helpers at module scope as test footguns in src/lib/cli/terminal-style.test.ts:17
  • PRA-11 Resolve or justify: redactProxyCredentials regex fallback leaks credentials on multi-@ malformed URLs in src/lib/onboard/http-proxy-preflight.ts:57
  • PRA-12 Resolve or justify: resolveSandboxGpuFlagFromOptions calls process.exit directly, limiting testability in src/lib/onboard/sandbox-gpu-preflight.ts:52
  • 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-T6 Add or justify test follow-up: Preflight runtime resources test expects raw '⚠' marker instead of warnLine output
  • PRA-T7 Add or justify test follow-up: src/lib/cli/terminal-style.ts:7-12
  • PRA-T8 Add or justify test follow-up: src/lib/cli/terminal-style.test.ts:17-42
  • PRA-16 In-scope improvement: Duplicate withStderrColorDepth helper across five test files in src/lib/onboard/bridge-dns-preflight.ts:1
  • PRA-17 In-scope improvement: bridge-dns-preflight.ts monolith at 648 lines — track as tech debt in src/lib/onboard/bridge-dns-preflight.ts:1
  • PRA-18 In-scope improvement: preflight-messages extraction only partially offsets onboard.ts growth in src/lib/onboard/preflight-messages.ts:1

Findings index

ID Severity Category Location Required action
PRA-1 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-2 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-3 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-4 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-5 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-6 Resolve/justify architecture Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
PRA-7 Required architecture src/lib/onboard.ts:1 Offset the +21 lines by extracting additional concerns from onboard.ts (e.g., port conflict messages, GPU detection messages, messaging setup) or removing unused code to achieve net-zero or negative growth before merge.
PRA-8 Required security src/lib/cli/terminal-style.ts:20 No fix needed — this is the intended security improvement. Verify the behavior is tested in terminal-style.test.ts lines 50-85.
PRA-9 Resolve/justify architecture src/lib/cli/terminal-style.ts:7 Either: (a) make legacy constants getters that evaluate at access time, or (b) audit all call sites and migrate to warnLine/failLine/styleText, then deprecate the constants. Document the invalid state, source boundary (module import time), why source can't be fixed in this PR, regression test, and removal condition.
PRA-10 Resolve/justify architecture src/lib/cli/terminal-style.test.ts:17 Make stubStream and restoreStream private to the test pattern (e.g., move inside withRestoredStreams or prefix with underscore) rather than removing them, since they are actively used by the test cases. Add a comment warning against direct use.
PRA-11 Resolve/justify security src/lib/onboard/http-proxy-preflight.ts:57 Add a test case for malformed URLs with multiple @ signs to verify redaction behavior. Consider a more robust regex (e.g., iterative replacement) or reject unparseable URLs with a warning instead of best-effort redaction.
PRA-12 Resolve/justify correctness src/lib/onboard/sandbox-gpu-preflight.ts:52 Return an error result (e.g., { ok: false, error: string }) instead of calling process.exit directly, or inject an exit function parameter for testability (pattern used elsewhere in preflight).
PRA-13 Resolve/justify tests src/lib/onboard/preflight-runtime-resources.test.ts:30 Update the test to use withStderrColorDepth(1, ...) to explicitly test plain-text output, or match against the warnLine output pattern (with optional ANSI codes).
PRA-14 Resolve/justify security src/lib/onboard/preflight-messages.ts:40 Document that the warn callback must write to stderr, or have warnLine accept an explicit stream parameter. Current usage is safe but the API is footgun-prone.
PRA-15 Resolve/justify security src/lib/onboard/http-proxy-preflight.ts:33 Either apply warnLine to all warning lines in the block, or apply no color to any (let the caller's warn callback decide). Consistency within a warning block is preferred.
PRA-16 Improvement correctness src/lib/onboard/bridge-dns-preflight.ts:1 Extract a shared test utility (e.g., src/lib/test-utils/stream-mock.ts) with a single withMockedStderrColorDepth helper. Migrate all test files to use it.
PRA-17 Improvement architecture src/lib/onboard/bridge-dns-preflight.ts:1 Track as technical debt. Not a blocker for this PR. Consider extracting: bridge-start-probe, host-dns-probe, container-dns-probe, remediation printers as separate modules.
PRA-18 Improvement scope src/lib/onboard/preflight-messages.ts:1 Consider extracting additional preflight message clusters (e.g., port conflict messages from preflight.ts, GPU detection lines, messaging provider missing) to further reduce onboard.ts line count and resolve PRA-4.

🚨 Required before merge

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

PRA-7 Required — onboard.ts grew by +21 lines despite preflight-messages.ts extraction — CI blocker

  • Location: src/lib/onboard.ts:1
  • Category: architecture
  • Problem: onboard.ts grew from 5385 to 5406 lines (+21) despite extracting 88 lines to preflight-messages.ts. The CI 'onboard-entrypoint-budget' / codebase-growth guardrail flags this as a blocker. Extraction pattern must yield net-negative growth.
  • Impact: Blocks merge per CI growth guardrail. Continued monolith growth increases sandbox escape / host-glue bug surface in privileged onboarding code.
  • Required action: Offset the +21 lines by extracting additional concerns from onboard.ts (e.g., port conflict messages, GPU detection messages, messaging setup) or removing unused code to achieve net-zero or negative growth before merge.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check drift context monolithDeltas for onboard.ts — shows baseLines=5385, headLines=5406, delta=21, severity=blocker. Run CI growth check locally.
  • Missing regression test: None — CI growth guardrail enforces this. Verify by running the growth check and confirming net-negative or zero delta.
  • Done when: The required change is committed and verification passes: Check drift context monolithDeltas for onboard.ts — shows baseLines=5385, headLines=5406, delta=21, severity=blocker. Run CI growth check locally.
  • Evidence: Drift context monolithDeltas: onboard.ts baseLines=5385, headLines=5406, delta=21, severity=blocker. PR adds imports and calls to printLowMemoryWarning, printSwapCreationFailed, printMessagingProviderMissing but net growth remains positive.

PRA-8 Required — stderr-keyed coloring fixes ANSI leakage bug — security-positive change

  • Location: src/lib/cli/terminal-style.ts:20
  • Category: security
  • Problem: stderr-keyed coloring via styleText({ stream: process.stderr }) correctly evaluates color capability on stderr. This fixes the ANSI leakage bug where 'onboard 2>log' leaked ANSI codes and 'onboard >log' dropped color. Implementation uses Node's built-in styleText which respects NO_COLOR, NODE_DISABLE_COLORS, FORCE_COLOR.
  • Impact: Positive — eliminates prior security issue where stderr redirection leaked ANSI escape sequences into log files.
  • Required action: No fix needed — this is the intended security improvement. Verify the behavior is tested in terminal-style.test.ts lines 50-85.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read terminal-style.ts lines 15-37 and terminal-style.test.ts lines 50-85 — tests verify color on stderr TTY, no color when stderr redirected, and NO_COLOR=1 forces plain text.
  • Missing regression test: Existing tests at terminal-style.test.ts lines 50-85 cover the stderr-keyed color behavior. No additional test needed.
  • Done when: The required change is committed and verification passes: Read terminal-style.ts lines 15-37 and terminal-style.test.ts lines 50-85 — tests verify color on stderr TTY, no color when stderr redirected, and NO_COLOR=1 forces plain text.
  • Evidence: terminal-style.ts:22-26 passes { stream: process.stderr } to styleText; tests at terminal-style.test.ts:58-75 verify stderr TTY gets color, stderr redirected gets plain text, NO_COLOR=1 forces plain.
Review findings by urgency: 2 required fixes, 13 items to resolve/justify, 3 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-1 Resolve/justify — Source-of-truth review needed: src/lib/cli/terminal-style.ts:7-12

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: terminal-style.test.ts:95-104 (legacy true-color test) — no test for import-after-env-change
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: terminal-style.ts:7-12 documents the freeze; terminal-style.test.ts:95-104 tests legacy true-color but only when configured before import

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/cli/terminal-style.test.ts:17-42

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None — no isolation verification test
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: terminal-style.test.ts:17-42 defines stubStream/restoreStream at module scope; withRestoredStreams uses them but they are accessible to any test

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:57-58

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Existing test: single-@ case only (//alice:s3cret@host)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: http-proxy-preflight.ts:57-58 single-replacement regex in catch block; http-proxy-preflight.test.ts:33-38 only tests single-@ case

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/preflight-runtime-resources.test.ts:30

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: The test itself
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: preflight-runtime-resources.test.ts:30 expects raw marker; preflight-messages.ts:46 uses warnLine which adds ANSI on color-capable stderr

PRA-5 Resolve/justify — Source-of-truth review needed: src/lib/onboard/preflight-messages.ts:40-56

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: preflight-messages.ts:40-56 warnLine called internally, warn callback passed through; preflight.ts:400 passes console.warn (stderr)

PRA-6 Resolve/justify — Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:33-45

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: http-proxy-preflight.ts:33 uses warnLine; lines 35-45 use plain warn() for proxy URL, explanation, fix hints

PRA-9 Resolve/justify — Legacy color constants frozen at import time — source-of-truth review needed

  • Location: src/lib/cli/terminal-style.ts:7
  • Category: architecture
  • Problem: Legacy color constants (G, B, D, R, RD, YW) are frozen at module import time based on stdout TTY/NO_COLOR state. The comment acknowledges this ('import after NO_COLOR and TTY state are configured') but the constants are evaluated once at load. This is a localized workaround that preserves an invalid state: callers importing after env/TTY changes get stale color decisions.
  • Impact: New code should use warnLine/failLine which evaluate at call time. But existing uses of G/B/D/R/RD/YW in the codebase (onboard.ts, bridge-dns-preflight.ts, etc.) may produce incorrect color output if imported before TTY/env is configured.
  • Recommended action: Either: (a) make legacy constants getters that evaluate at access time, or (b) audit all call sites and migrate to warnLine/failLine/styleText, then deprecate the constants. Document the invalid state, source boundary (module import time), why source can't be fixed in this PR, regression test, and removal condition.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for usages of G, B, D, R, RD, YW in the codebase and verify they are imported after TTY/env setup or migrated to call-time helpers.
  • Missing regression test: terminal-style.test.ts:95-104 (legacy true-color test) — but no test verifies that importing after env change produces correct constants.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for usages of G, B, D, R, RD, YW in the codebase and verify they are imported after TTY/env setup or migrated to call-time helpers.
  • Evidence: terminal-style.ts:7-12 documents the freeze; terminal-style.test.ts:95-104 tests legacy true-color but only when configured before import.

PRA-10 Resolve/justify — stubStream/restoreStream helpers at module scope as test footguns

  • Location: src/lib/cli/terminal-style.test.ts:17
  • Category: architecture
  • Problem: stubStream and restoreStream helpers are defined at module scope and mutate global process.stdout/stderr properties. These are test footguns: if a test forgets to call restoreStream or withRestoredStreams, subsequent tests run with mutated streams.
  • Impact: Test pollution risk — module-scope mutation helpers can leak state between tests, causing flaky or false-passing tests.
  • Recommended action: Make stubStream and restoreStream private to the test pattern (e.g., move inside withRestoredStreams or prefix with underscore) rather than removing them, since they are actively used by the test cases. Add a comment warning against direct use.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read terminal-style.test.ts lines 17-42 — helpers are at module scope and used by withRestoredStreams. Check if any test uses them directly without withRestoredStreams.
  • Missing regression test: No existing test verifies isolation — add a test that runs two test cases sequentially where the first mutates streams and the second verifies clean state.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read terminal-style.test.ts lines 17-42 — helpers are at module scope and used by withRestoredStreams. Check if any test uses them directly without withRestoredStreams.
  • Evidence: terminal-style.test.ts:17-42 defines stubStream/restoreStream at module scope; withRestoredStreams uses them but they are accessible to any test.

PRA-11 Resolve/justify — redactProxyCredentials regex fallback leaks credentials on multi-@ malformed URLs

  • Location: src/lib/onboard/http-proxy-preflight.ts:57
  • Category: security
  • Problem: redactProxyCredentials falls back to regex `raw.replace(/(\/\/)[^/@]+@/, "$1****@")` for unparseable URLs. This only redacts the first userinfo segment. For malformed URLs with multiple @ signs (e.g., `//alice:secret@host@other`), it produces `//****@host@other`, leaking 'host' which could contain credentials.
  • Impact: Credential leakage on malformed proxy URLs. Defense-in-depth gap: the URL parser catches valid URLs, but the regex fallback is incomplete for multi-@ malformed strings.
  • Recommended action: Add a test case for malformed URLs with multiple @ signs to verify redaction behavior. Consider a more robust regex (e.g., iterative replacement) or reject unparseable URLs with a warning instead of best-effort redaction.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read http-proxy-preflight.ts lines 48-58. The catch block at line 57-58 uses the single-replacement regex. Test with `//user:pass@host@other` — current output leaks 'host'.
  • Missing regression test: Add test case: `redactProxyCredentials('//alice:secret@host@other')` should not contain 'alice', 'secret', or 'host' (or should fully redact). Current test only covers single-@ case.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read http-proxy-preflight.ts lines 48-58. The catch block at line 57-58 uses the single-replacement regex. Test with `//user:pass@host@other` — current output leaks 'host'.
  • Evidence: http-proxy-preflight.ts:57-58 single-replacement regex in catch block; http-proxy-preflight.test.ts:33-38 only tests single-@ case.

PRA-12 Resolve/justify — resolveSandboxGpuFlagFromOptions calls process.exit directly, limiting testability

  • Location: src/lib/onboard/sandbox-gpu-preflight.ts:52
  • Category: correctness
  • Problem: resolveSandboxGpuFlagFromOptions calls process.exit(1) directly on conflicting flags (--gpu + --no-gpu, or flag conflicts). This limits testability — tests cannot verify error behavior without exiting the test process.
  • Impact: Reduced test coverage for flag conflict validation. Tests must spawn subprocesses or mock process.exit, adding complexity.
  • Recommended action: Return an error result (e.g., { ok: false, error: string }) instead of calling process.exit directly, or inject an exit function parameter for testability (pattern used elsewhere in preflight).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read sandbox-gpu-preflight.ts lines 52-57 — two process.exit(1) calls for flag conflicts.
  • Missing regression test: Add unit test for flag conflict validation that asserts error return without process exit.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read sandbox-gpu-preflight.ts lines 52-57 — two process.exit(1) calls for flag conflicts.
  • Evidence: sandbox-gpu-preflight.ts:52-57 two process.exit(1) calls for flag conflicts; no tests for this error path.

PRA-13 Resolve/justify — Preflight runtime resources test expects raw '⚠' marker instead of warnLine output

  • Location: src/lib/onboard/preflight-runtime-resources.test.ts:30
  • Category: tests
  • Problem: Test expects raw '⚠' marker in warn callback output, but the implementation now uses warnLine which outputs ANSI codes when stderr supports color. The test doesn't mock color depth (no withStderrColorDepth), making it flaky — passes in CI (no color) but would fail locally with color-capable stderr.
  • Impact: Flaky test that passes in CI but fails for developers with color terminals. Undermines confidence in warning output tests.
  • Recommended action: Update the test to use withStderrColorDepth(1, ...) to explicitly test plain-text output, or match against the warnLine output pattern (with optional ANSI codes).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read preflight-runtime-resources.test.ts line 30 — expects '⚠ Container runtime under-provisioned' but warnLine adds ANSI when colorDepth > 1.
  • Missing regression test: Update existing test to use withStderrColorDepth(1, ...) for deterministic plain-text assertion.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read preflight-runtime-resources.test.ts line 30 — expects '⚠ Container runtime under-provisioned' but warnLine adds ANSI when colorDepth > 1.
  • Evidence: preflight-runtime-resources.test.ts:30 expects raw marker; preflight-messages.ts:46 uses warnLine which adds ANSI on color-capable stderr.

PRA-14 Resolve/justify — printUnderProvisionedRuntimeWarning custom warn callback stream mismatch footgun

  • Location: src/lib/onboard/preflight-messages.ts:40
  • Category: security
  • Problem: printUnderProvisionedRuntimeWarning accepts a custom `warn` callback but uses warnLine internally which keys color off process.stderr. If caller passes a callback that writes to stdout (or a test mock that captures to a buffer), the color decision is based on stderr but output goes elsewhere — ANSI codes may leak or be dropped incorrectly.
  • Impact: API footgun: inconsistent color behavior when custom warn callback is used. Current callers (preflight.ts) pass console.warn (stderr) so it's safe, but the API doesn't enforce this.
  • Recommended action: Document that the warn callback must write to stderr, or have warnLine accept an explicit stream parameter. Current usage is safe but the API is footgun-prone.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read preflight-messages.ts lines 40-56 — warnLine called internally, warn callback passed through. Check preflight.ts line 400 — passes console.warn (stderr).
  • Missing regression test: Add test verifying that printUnderProvisionedRuntimeWarning with a stdout-writing callback produces incorrect color (documents the footgun) or refactor to accept explicit stream.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read preflight-messages.ts lines 40-56 — warnLine called internally, warn callback passed through. Check preflight.ts line 400 — passes console.warn (stderr).
  • Evidence: preflight-messages.ts:40-56 warnLine used internally; preflight.ts:400 passes console.warn (stderr).

PRA-15 Resolve/justify — warnIfHostProxyMissesLoopback inconsistent coloring within warning block

  • Location: src/lib/onboard/http-proxy-preflight.ts:33
  • Category: security
  • Problem: warnIfHostProxyMissesLoopback uses warnLine for the first warning line but plain warn() for subsequent detail lines (proxy URL, explanation, fix hints). This creates inconsistent coloring within a single warning block — first line colored, rest plain.
  • Impact: Visual inconsistency in warning output. Undermines the severity coloring system. If stderr is redirected, first line correctly goes plain but subsequent lines were already plain — works by accident. If stderr is TTY, first line colored, rest not.
  • Recommended action: Either apply warnLine to all warning lines in the block, or apply no color to any (let the caller's warn callback decide). Consistency within a warning block is preferred.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read http-proxy-preflight.ts lines 33-45 — line 33 uses warnLine, lines 35-45 use plain warn().
  • Missing regression test: Add test verifying all lines in the warning block have consistent color treatment (all colored on color-capable stderr, all plain on plain stderr).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read http-proxy-preflight.ts lines 33-45 — line 33 uses warnLine, lines 35-45 use plain warn().
  • Evidence: http-proxy-preflight.ts:33 uses warnLine; lines 35-45 use plain warn() for proxy URL, explanation, fix hints.

💡 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-16 Improvement — Duplicate withStderrColorDepth helper across five test files

  • Location: src/lib/onboard/bridge-dns-preflight.ts:1
  • Category: correctness
  • Problem: Duplicate withStderrColorDepth helper across five test files (bridge-dns-preflight, http-proxy-preflight, preflight-cdi, preflight-messages, gateway-sandbox-reachability-severity). Each reimplements the same stderr mocking pattern.
  • Impact: Code duplication, maintenance burden, risk of inconsistent mocking behavior.
  • Suggested action: Extract a shared test utility (e.g., src/lib/test-utils/stream-mock.ts) with a single withMockedStderrColorDepth helper. Migrate all test files to use it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Search for 'withStderrColorDepth' or 'withColoredStderr' across test files — 5+ implementations.
  • Missing regression test: Shared utility should have its own tests verifying color depth mocking works correctly.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Found in bridge-dns-preflight.ts test, http-proxy-preflight.test.ts, preflight-cdi.test.ts, preflight-messages.test.ts, gateway-sandbox-reachability-severity.test.ts

PRA-17 Improvement — bridge-dns-preflight.ts monolith at 648 lines — track as tech debt

  • Location: src/lib/onboard/bridge-dns-preflight.ts:1
  • Category: architecture
  • Problem: bridge-dns-preflight.ts is a 648-line monolith handling bridge container start, host DNS, container DNS, remediation printing, and daemon.json patching. Track as technical debt for future extraction of preflight sub-modules.
  • Impact: Large file increases cognitive load and bug surface. Harder to test individual preflight components in isolation.
  • Suggested action: Track as technical debt. Not a blocker for this PR. Consider extracting: bridge-start-probe, host-dns-probe, container-dns-probe, remediation printers as separate modules.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: File is 648 lines per drift context. Review for logical boundaries.
  • Missing regression test: None — existing tests cover the integrated behavior. Extraction would require test reorganization.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context shows bridge-dns-preflight.ts at 648 lines.

PRA-18 Improvement — preflight-messages extraction only partially offsets onboard.ts growth

  • Location: src/lib/onboard/preflight-messages.ts:1
  • Category: scope
  • Problem: preflight-messages extraction (88 lines) only partially offsets onboard.ts growth (+21 net). Additional preflight message clusters (port conflict messages, GPU detection messages, messaging setup messages) could be extracted to further reduce onboard.ts line count.
  • Impact: Missed opportunity to achieve net-negative growth. Onboard.ts remains at blocker threshold.
  • Suggested action: Consider extracting additional preflight message clusters (e.g., port conflict messages from preflight.ts, GPU detection lines, messaging provider missing) to further reduce onboard.ts line count and resolve PRA-4.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Search onboard.ts for console.log/console.warn/console.error calls that print preflight-style messages — many are inline rather than extracted.
  • Missing regression test: None — extraction would be verified by existing tests and CI growth check.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard.ts grew +21 despite 88-line extraction. Drift context shows preflight-messages.ts added 88 lines, onboard.ts grew 21 lines.
Simplification opportunities: 3 possible cuts, net -200 lines possible

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

  • PRA-16 stdlib (src/lib/onboard/bridge-dns-preflight.ts:1): Duplicate withStderrColorDepth/withColoredStderr implementations in 5 test files
    • Replacement: Single shared utility at src/lib/test-utils/stream-mock.ts with withMockedStderrColorDepth helper
    • Net: -150 lines
    • Safety boundary: Test utilities only — no production code affected. Must preserve exact mocking behavior for stderr isTTY/getColorDepth.
  • PRA-17 shrink (src/lib/onboard/bridge-dns-preflight.ts:1): Extract bridge-start-probe, host-dns-probe, container-dns-probe, remediation printers as separate modules
    • Replacement: Multiple focused modules under src/lib/onboard/preflight/
    • Net: 0 lines
    • Safety boundary: All extractions must preserve existing test coverage and public API (assertDockerBridgeAndContainerDnsHealthy, assertHostDnsHealthy, printDockerBridgeContainerStartFailure, printHostDnsRemediation, printContainerDnsResolutionFailedRemediation, printDaemonJsonDnsPatch).
  • PRA-18 shrink (src/lib/onboard/preflight-messages.ts:1): Inline preflight message strings in onboard.ts (port conflicts, GPU detection, messaging setup)
    • Replacement: Additional functions in preflight-messages.ts or new focused message modules
    • Net: -50 lines
    • Safety boundary: Must not change control flow or error handling — only extract string formatting and console calls.
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 — redactProxyCredentials multi-@ malformed URL — credential leakage on //user:pass@host@other. Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/cli/terminal-style.ts, src/lib/onboard.ts, src/lib/onboard/bridge-dns-preflight.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/gateway-sandbox-reachability.ts.
  • PRA-T2 Runtime validation — printUnderProvisionedRuntimeWarning stdout callback — ANSI leakage when warn callback writes to stdout. Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/cli/terminal-style.ts, src/lib/onboard.ts, src/lib/onboard/bridge-dns-preflight.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/gateway-sandbox-reachability.ts.
  • PRA-T3 Runtime validation — warnIfHostProxyMissesLoopback consistent coloring — all warning block lines same color treatment. Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/cli/terminal-style.ts, src/lib/onboard.ts, src/lib/onboard/bridge-dns-preflight.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/gateway-sandbox-reachability.ts.
  • PRA-T4 Runtime validation — preflight-runtime-resources withStderrColorDepth(1) — deterministic plain-text assertion. Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/cli/terminal-style.ts, src/lib/onboard.ts, src/lib/onboard/bridge-dns-preflight.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/gateway-sandbox-reachability.ts.
  • PRA-T5 Runtime validation — terminal-style legacy constants import-after-env — verify constants stale after env change. Runtime/sandbox/infrastructure paths need behavioral runtime validation: ci/platform-matrix.json, docs/inference/inference-options.mdx, docs/reference/platform-support.mdx, src/lib/cli/terminal-style.ts, src/lib/onboard.ts, src/lib/onboard/bridge-dns-preflight.ts, src/lib/onboard/fatal-runtime-preflight.ts, src/lib/onboard/gateway-sandbox-reachability.ts.
  • PRA-T6 Preflight runtime resources test expects raw '⚠' marker instead of warnLine output — Update the test to use withStderrColorDepth(1, ...) to explicitly test plain-text output, or match against the warnLine output pattern (with optional ANSI codes).
  • PRA-T7 src/lib/cli/terminal-style.ts:7-12 — terminal-style.test.ts:95-104 (legacy true-color test) — no test for import-after-env-change. terminal-style.ts:7-12 documents the freeze; terminal-style.test.ts:95-104 tests legacy true-color but only when configured before import
  • PRA-T8 src/lib/cli/terminal-style.test.ts:17-42 — None — no isolation verification test. terminal-style.test.ts:17-42 defines stubStream/restoreStream at module scope; withRestoredStreams uses them but they are accessible to any test
Since last review details

Current findings, using the urgency labels above:

PRA-1 Resolve/justify — Source-of-truth review needed: src/lib/cli/terminal-style.ts:7-12

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: terminal-style.test.ts:95-104 (legacy true-color test) — no test for import-after-env-change
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: terminal-style.ts:7-12 documents the freeze; terminal-style.test.ts:95-104 tests legacy true-color but only when configured before import

PRA-2 Resolve/justify — Source-of-truth review needed: src/lib/cli/terminal-style.test.ts:17-42

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None — no isolation verification test
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: terminal-style.test.ts:17-42 defines stubStream/restoreStream at module scope; withRestoredStreams uses them but they are accessible to any test

PRA-3 Resolve/justify — Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:57-58

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: Existing test: single-@ case only (//alice:s3cret@host)
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: http-proxy-preflight.ts:57-58 single-replacement regex in catch block; http-proxy-preflight.test.ts:33-38 only tests single-@ case

PRA-4 Resolve/justify — Source-of-truth review needed: src/lib/onboard/preflight-runtime-resources.test.ts:30

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: The test itself
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: preflight-runtime-resources.test.ts:30 expects raw marker; preflight-messages.ts:46 uses warnLine which adds ANSI on color-capable stderr

PRA-5 Resolve/justify — Source-of-truth review needed: src/lib/onboard/preflight-messages.ts:40-56

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as needs_followup.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: preflight-messages.ts:40-56 warnLine called internally, warn callback passed through; preflight.ts:400 passes console.warn (stderr)

PRA-6 Resolve/justify — Source-of-truth review needed: src/lib/onboard/http-proxy-preflight.ts:33-45

  • Location: not file-specific
  • Category: architecture
  • Problem: The advisor marked localized patch analysis as missing.
  • Impact: A localized workaround can preserve or hide an invalid state when the source boundary is unclear.
  • Recommended action: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Missing regression test: None
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Inspect the localized patch and source-of-truth review fields for a concrete invalid state, source boundary, source-fix constraint, regression test, and removal condition.
  • Evidence: http-proxy-preflight.ts:33 uses warnLine; lines 35-45 use plain warn() for proxy URL, explanation, fix hints

PRA-7 Required — onboard.ts grew by +21 lines despite preflight-messages.ts extraction — CI blocker

  • Location: src/lib/onboard.ts:1
  • Category: architecture
  • Problem: onboard.ts grew from 5385 to 5406 lines (+21) despite extracting 88 lines to preflight-messages.ts. The CI 'onboard-entrypoint-budget' / codebase-growth guardrail flags this as a blocker. Extraction pattern must yield net-negative growth.
  • Impact: Blocks merge per CI growth guardrail. Continued monolith growth increases sandbox escape / host-glue bug surface in privileged onboarding code.
  • Required action: Offset the +21 lines by extracting additional concerns from onboard.ts (e.g., port conflict messages, GPU detection messages, messaging setup) or removing unused code to achieve net-zero or negative growth before merge.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Check drift context monolithDeltas for onboard.ts — shows baseLines=5385, headLines=5406, delta=21, severity=blocker. Run CI growth check locally.
  • Missing regression test: None — CI growth guardrail enforces this. Verify by running the growth check and confirming net-negative or zero delta.
  • Done when: The required change is committed and verification passes: Check drift context monolithDeltas for onboard.ts — shows baseLines=5385, headLines=5406, delta=21, severity=blocker. Run CI growth check locally.
  • Evidence: Drift context monolithDeltas: onboard.ts baseLines=5385, headLines=5406, delta=21, severity=blocker. PR adds imports and calls to printLowMemoryWarning, printSwapCreationFailed, printMessagingProviderMissing but net growth remains positive.

PRA-8 Required — stderr-keyed coloring fixes ANSI leakage bug — security-positive change

  • Location: src/lib/cli/terminal-style.ts:20
  • Category: security
  • Problem: stderr-keyed coloring via styleText({ stream: process.stderr }) correctly evaluates color capability on stderr. This fixes the ANSI leakage bug where 'onboard 2>log' leaked ANSI codes and 'onboard >log' dropped color. Implementation uses Node's built-in styleText which respects NO_COLOR, NODE_DISABLE_COLORS, FORCE_COLOR.
  • Impact: Positive — eliminates prior security issue where stderr redirection leaked ANSI escape sequences into log files.
  • Required action: No fix needed — this is the intended security improvement. Verify the behavior is tested in terminal-style.test.ts lines 50-85.
  • Expected follow-up: Fix before merge or get explicit maintainer override.
  • Verification: Read terminal-style.ts lines 15-37 and terminal-style.test.ts lines 50-85 — tests verify color on stderr TTY, no color when stderr redirected, and NO_COLOR=1 forces plain text.
  • Missing regression test: Existing tests at terminal-style.test.ts lines 50-85 cover the stderr-keyed color behavior. No additional test needed.
  • Done when: The required change is committed and verification passes: Read terminal-style.ts lines 15-37 and terminal-style.test.ts lines 50-85 — tests verify color on stderr TTY, no color when stderr redirected, and NO_COLOR=1 forces plain text.
  • Evidence: terminal-style.ts:22-26 passes { stream: process.stderr } to styleText; tests at terminal-style.test.ts:58-75 verify stderr TTY gets color, stderr redirected gets plain text, NO_COLOR=1 forces plain.

PRA-9 Resolve/justify — Legacy color constants frozen at import time — source-of-truth review needed

  • Location: src/lib/cli/terminal-style.ts:7
  • Category: architecture
  • Problem: Legacy color constants (G, B, D, R, RD, YW) are frozen at module import time based on stdout TTY/NO_COLOR state. The comment acknowledges this ('import after NO_COLOR and TTY state are configured') but the constants are evaluated once at load. This is a localized workaround that preserves an invalid state: callers importing after env/TTY changes get stale color decisions.
  • Impact: New code should use warnLine/failLine which evaluate at call time. But existing uses of G/B/D/R/RD/YW in the codebase (onboard.ts, bridge-dns-preflight.ts, etc.) may produce incorrect color output if imported before TTY/env is configured.
  • Recommended action: Either: (a) make legacy constants getters that evaluate at access time, or (b) audit all call sites and migrate to warnLine/failLine/styleText, then deprecate the constants. Document the invalid state, source boundary (module import time), why source can't be fixed in this PR, regression test, and removal condition.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Search for usages of G, B, D, R, RD, YW in the codebase and verify they are imported after TTY/env setup or migrated to call-time helpers.
  • Missing regression test: terminal-style.test.ts:95-104 (legacy true-color test) — but no test verifies that importing after env change produces correct constants.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Search for usages of G, B, D, R, RD, YW in the codebase and verify they are imported after TTY/env setup or migrated to call-time helpers.
  • Evidence: terminal-style.ts:7-12 documents the freeze; terminal-style.test.ts:95-104 tests legacy true-color but only when configured before import.

PRA-10 Resolve/justify — stubStream/restoreStream helpers at module scope as test footguns

  • Location: src/lib/cli/terminal-style.test.ts:17
  • Category: architecture
  • Problem: stubStream and restoreStream helpers are defined at module scope and mutate global process.stdout/stderr properties. These are test footguns: if a test forgets to call restoreStream or withRestoredStreams, subsequent tests run with mutated streams.
  • Impact: Test pollution risk — module-scope mutation helpers can leak state between tests, causing flaky or false-passing tests.
  • Recommended action: Make stubStream and restoreStream private to the test pattern (e.g., move inside withRestoredStreams or prefix with underscore) rather than removing them, since they are actively used by the test cases. Add a comment warning against direct use.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read terminal-style.test.ts lines 17-42 — helpers are at module scope and used by withRestoredStreams. Check if any test uses them directly without withRestoredStreams.
  • Missing regression test: No existing test verifies isolation — add a test that runs two test cases sequentially where the first mutates streams and the second verifies clean state.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read terminal-style.test.ts lines 17-42 — helpers are at module scope and used by withRestoredStreams. Check if any test uses them directly without withRestoredStreams.
  • Evidence: terminal-style.test.ts:17-42 defines stubStream/restoreStream at module scope; withRestoredStreams uses them but they are accessible to any test.

PRA-11 Resolve/justify — redactProxyCredentials regex fallback leaks credentials on multi-@ malformed URLs

  • Location: src/lib/onboard/http-proxy-preflight.ts:57
  • Category: security
  • Problem: redactProxyCredentials falls back to regex `raw.replace(/(\/\/)[^/@]+@/, "$1****@")` for unparseable URLs. This only redacts the first userinfo segment. For malformed URLs with multiple @ signs (e.g., `//alice:secret@host@other`), it produces `//****@host@other`, leaking 'host' which could contain credentials.
  • Impact: Credential leakage on malformed proxy URLs. Defense-in-depth gap: the URL parser catches valid URLs, but the regex fallback is incomplete for multi-@ malformed strings.
  • Recommended action: Add a test case for malformed URLs with multiple @ signs to verify redaction behavior. Consider a more robust regex (e.g., iterative replacement) or reject unparseable URLs with a warning instead of best-effort redaction.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read http-proxy-preflight.ts lines 48-58. The catch block at line 57-58 uses the single-replacement regex. Test with `//user:pass@host@other` — current output leaks 'host'.
  • Missing regression test: Add test case: `redactProxyCredentials('//alice:secret@host@other')` should not contain 'alice', 'secret', or 'host' (or should fully redact). Current test only covers single-@ case.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read http-proxy-preflight.ts lines 48-58. The catch block at line 57-58 uses the single-replacement regex. Test with `//user:pass@host@other` — current output leaks 'host'.
  • Evidence: http-proxy-preflight.ts:57-58 single-replacement regex in catch block; http-proxy-preflight.test.ts:33-38 only tests single-@ case.

PRA-12 Resolve/justify — resolveSandboxGpuFlagFromOptions calls process.exit directly, limiting testability

  • Location: src/lib/onboard/sandbox-gpu-preflight.ts:52
  • Category: correctness
  • Problem: resolveSandboxGpuFlagFromOptions calls process.exit(1) directly on conflicting flags (--gpu + --no-gpu, or flag conflicts). This limits testability — tests cannot verify error behavior without exiting the test process.
  • Impact: Reduced test coverage for flag conflict validation. Tests must spawn subprocesses or mock process.exit, adding complexity.
  • Recommended action: Return an error result (e.g., { ok: false, error: string }) instead of calling process.exit directly, or inject an exit function parameter for testability (pattern used elsewhere in preflight).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read sandbox-gpu-preflight.ts lines 52-57 — two process.exit(1) calls for flag conflicts.
  • Missing regression test: Add unit test for flag conflict validation that asserts error return without process exit.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read sandbox-gpu-preflight.ts lines 52-57 — two process.exit(1) calls for flag conflicts.
  • Evidence: sandbox-gpu-preflight.ts:52-57 two process.exit(1) calls for flag conflicts; no tests for this error path.

PRA-13 Resolve/justify — Preflight runtime resources test expects raw '⚠' marker instead of warnLine output

  • Location: src/lib/onboard/preflight-runtime-resources.test.ts:30
  • Category: tests
  • Problem: Test expects raw '⚠' marker in warn callback output, but the implementation now uses warnLine which outputs ANSI codes when stderr supports color. The test doesn't mock color depth (no withStderrColorDepth), making it flaky — passes in CI (no color) but would fail locally with color-capable stderr.
  • Impact: Flaky test that passes in CI but fails for developers with color terminals. Undermines confidence in warning output tests.
  • Recommended action: Update the test to use withStderrColorDepth(1, ...) to explicitly test plain-text output, or match against the warnLine output pattern (with optional ANSI codes).
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read preflight-runtime-resources.test.ts line 30 — expects '⚠ Container runtime under-provisioned' but warnLine adds ANSI when colorDepth > 1.
  • Missing regression test: Update existing test to use withStderrColorDepth(1, ...) for deterministic plain-text assertion.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read preflight-runtime-resources.test.ts line 30 — expects '⚠ Container runtime under-provisioned' but warnLine adds ANSI when colorDepth > 1.
  • Evidence: preflight-runtime-resources.test.ts:30 expects raw marker; preflight-messages.ts:46 uses warnLine which adds ANSI on color-capable stderr.

PRA-14 Resolve/justify — printUnderProvisionedRuntimeWarning custom warn callback stream mismatch footgun

  • Location: src/lib/onboard/preflight-messages.ts:40
  • Category: security
  • Problem: printUnderProvisionedRuntimeWarning accepts a custom `warn` callback but uses warnLine internally which keys color off process.stderr. If caller passes a callback that writes to stdout (or a test mock that captures to a buffer), the color decision is based on stderr but output goes elsewhere — ANSI codes may leak or be dropped incorrectly.
  • Impact: API footgun: inconsistent color behavior when custom warn callback is used. Current callers (preflight.ts) pass console.warn (stderr) so it's safe, but the API doesn't enforce this.
  • Recommended action: Document that the warn callback must write to stderr, or have warnLine accept an explicit stream parameter. Current usage is safe but the API is footgun-prone.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read preflight-messages.ts lines 40-56 — warnLine called internally, warn callback passed through. Check preflight.ts line 400 — passes console.warn (stderr).
  • Missing regression test: Add test verifying that printUnderProvisionedRuntimeWarning with a stdout-writing callback produces incorrect color (documents the footgun) or refactor to accept explicit stream.
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read preflight-messages.ts lines 40-56 — warnLine called internally, warn callback passed through. Check preflight.ts line 400 — passes console.warn (stderr).
  • Evidence: preflight-messages.ts:40-56 warnLine used internally; preflight.ts:400 passes console.warn (stderr).

PRA-15 Resolve/justify — warnIfHostProxyMissesLoopback inconsistent coloring within warning block

  • Location: src/lib/onboard/http-proxy-preflight.ts:33
  • Category: security
  • Problem: warnIfHostProxyMissesLoopback uses warnLine for the first warning line but plain warn() for subsequent detail lines (proxy URL, explanation, fix hints). This creates inconsistent coloring within a single warning block — first line colored, rest plain.
  • Impact: Visual inconsistency in warning output. Undermines the severity coloring system. If stderr is redirected, first line correctly goes plain but subsequent lines were already plain — works by accident. If stderr is TTY, first line colored, rest not.
  • Recommended action: Either apply warnLine to all warning lines in the block, or apply no color to any (let the caller's warn callback decide). Consistency within a warning block is preferred.
  • Expected follow-up: Resolve in this PR or explain why the risk is acceptable.
  • Verification: Read http-proxy-preflight.ts lines 33-45 — line 33 uses warnLine, lines 35-45 use plain warn().
  • Missing regression test: Add test verifying all lines in the warning block have consistent color treatment (all colored on color-capable stderr, all plain on plain stderr).
  • Done when: The risk is fixed or explicitly justified in the PR. Verification: Read http-proxy-preflight.ts lines 33-45 — line 33 uses warnLine, lines 35-45 use plain warn().
  • Evidence: http-proxy-preflight.ts:33 uses warnLine; lines 35-45 use plain warn() for proxy URL, explanation, fix hints.

PRA-16 Improvement — Duplicate withStderrColorDepth helper across five test files

  • Location: src/lib/onboard/bridge-dns-preflight.ts:1
  • Category: correctness
  • Problem: Duplicate withStderrColorDepth helper across five test files (bridge-dns-preflight, http-proxy-preflight, preflight-cdi, preflight-messages, gateway-sandbox-reachability-severity). Each reimplements the same stderr mocking pattern.
  • Impact: Code duplication, maintenance burden, risk of inconsistent mocking behavior.
  • Suggested action: Extract a shared test utility (e.g., src/lib/test-utils/stream-mock.ts) with a single withMockedStderrColorDepth helper. Migrate all test files to use it.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Search for 'withStderrColorDepth' or 'withColoredStderr' across test files — 5+ implementations.
  • Missing regression test: Shared utility should have its own tests verifying color depth mocking works correctly.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Found in bridge-dns-preflight.ts test, http-proxy-preflight.test.ts, preflight-cdi.test.ts, preflight-messages.test.ts, gateway-sandbox-reachability-severity.test.ts

PRA-17 Improvement — bridge-dns-preflight.ts monolith at 648 lines — track as tech debt

  • Location: src/lib/onboard/bridge-dns-preflight.ts:1
  • Category: architecture
  • Problem: bridge-dns-preflight.ts is a 648-line monolith handling bridge container start, host DNS, container DNS, remediation printing, and daemon.json patching. Track as technical debt for future extraction of preflight sub-modules.
  • Impact: Large file increases cognitive load and bug surface. Harder to test individual preflight components in isolation.
  • Suggested action: Track as technical debt. Not a blocker for this PR. Consider extracting: bridge-start-probe, host-dns-probe, container-dns-probe, remediation printers as separate modules.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: File is 648 lines per drift context. Review for logical boundaries.
  • Missing regression test: None — existing tests cover the integrated behavior. Extraction would require test reorganization.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context shows bridge-dns-preflight.ts at 648 lines.

PRA-18 Improvement — preflight-messages extraction only partially offsets onboard.ts growth

  • Location: src/lib/onboard/preflight-messages.ts:1
  • Category: scope
  • Problem: preflight-messages extraction (88 lines) only partially offsets onboard.ts growth (+21 net). Additional preflight message clusters (port conflict messages, GPU detection messages, messaging setup messages) could be extracted to further reduce onboard.ts line count.
  • Impact: Missed opportunity to achieve net-negative growth. Onboard.ts remains at blocker threshold.
  • Suggested action: Consider extracting additional preflight message clusters (e.g., port conflict messages from preflight.ts, GPU detection lines, messaging provider missing) to further reduce onboard.ts line count and resolve PRA-4.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Search onboard.ts for console.log/console.warn/console.error calls that print preflight-style messages — many are inline rather than extracted.
  • Missing regression test: None — extraction would be verified by existing tests and CI growth check.
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: onboard.ts grew +21 despite 88-line extraction. Drift context shows preflight-messages.ts added 88 lines, onboard.ts grew 21 lines.

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.

@wscurran wscurran added area: cli Command line interface, flags, terminal UX, or output bug-fix PR fixes a bug or regression labels Jul 1, 2026
@wscurran

wscurran commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

…sive message

PRA-1: console.warn at bridge-dns-preflight.ts:173 was rendering in
default color instead of using warnLine() like the other warn messages
added in this PR.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: ubuntu-repo-cloud-openclaw, onboard-resume, onboard-repair, gpu-e2e
Optional E2E targets: None

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=gpu-e2e

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • ubuntu-repo-cloud-openclaw: Core onboarding preflight and gateway/sandbox reachability code changed; run the live-supported Ubuntu Docker cloud OpenClaw target to exercise the primary onboarding path.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field targets=ubuntu-repo-cloud-openclaw
  • onboard-resume: Onboarding preflight/backstop code used by resume paths changed, including bridge/DNS and preflight messaging; the onboarding resume rule requires the onboard-resume live job.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-resume
  • onboard-repair: The changed preflight/backstop surface can affect repair execution from persisted sessions, so the onboarding resume policy requires the onboard-repair live job as well.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=onboard-repair
  • gpu-e2e: GPU/CDI preflight code changed in sandbox-gpu-preflight and related preflight paths; gpu-e2e is the only live E2E job that exercises this GPU-specific surface.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=gpu-e2e

Optional E2E targets

  • None.

Relevant changed files

  • src/lib/cli/terminal-style.ts
  • src/lib/onboard.ts
  • src/lib/onboard/bridge-dns-preflight.ts
  • src/lib/onboard/fatal-runtime-preflight.ts
  • src/lib/onboard/gateway-sandbox-reachability.ts
  • src/lib/onboard/http-proxy-preflight.ts
  • src/lib/onboard/preflight-gateway-cleanup-decision.ts
  • src/lib/onboard/preflight-messages.ts
  • src/lib/onboard/preflight.ts
  • src/lib/onboard/sandbox-gpu-preflight.ts

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Scenario Results — ❌ Some jobs failed

Run: 28552134018
Workflow ref: fix/6004-preflight-warn-error-color
Requested scenarios: (selector rejected by workflow validation)
Requested jobs: (default — all default-enabled free-standing jobs; explicit-only jobs such as jetson-nvmap-gpu-vitest and sandbox-rlimits-connect-vitest are skipped unless selected)
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
generate-matrix ❌ failure

Failed jobs: generate-matrix. Check run artifacts for logs.

@cv cv added v0.0.73 and removed v0.0.72 labels Jul 1, 2026
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Vitest E2E Target Results — ❌ Some jobs failed

Run: 28553969502
Workflow ref: main
Requested targets: (default — all supported)
Requested jobs: (selector rejected by workflow validation)
Summary: 0 passed, 1 failed, 0 cancelled, 0 skipped

Job Result
generate-matrix ❌ failure

Failed jobs: generate-matrix. Check run artifacts for logs.

Complete the warnLine cutover for inconclusive container-DNS warnings:
the image_pull_failed branch still emitted a hardcoded '⚠' line, bypassing
the helper's TTY/NO_COLOR handling. Route it through warnLine so all
inconclusive DNS warnings share one authoritative formatting path.
Addresses CodeRabbit review comment on #6017.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Signed-off-by: Jason Ma <jama@nvidia.com>
@cv cv added v0.0.74 and removed v0.0.73 labels Jul 2, 2026
@cv

cv commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

I don't think we should merge this as a two-module formatting change. It adds another selectively adopted warnLine() / failLine() path while the same preflight still emits unstyled severity lines: the unsupported-runtime, Docker-not-reachable, and under-provisioned-runtime paths in src/lib/onboard.ts are immediate examples. Elsewhere, the CLI mixes bare / markers, ${YW} / ${RD} interpolation, and local ANSI constants. Formatting therefore still depends on which call site emits the message, and the Docker-not-running reproduction from #6004 remains unfixed.

We should build this on the Node standard-library primitives we already have at our runtime floor (>=22.16): console.info() / console.warn() / console.error() for semantic level and stdout/stderr routing, and node:util.styleText() for color. styleText() is stable there and already handles terminal capability plus NO_COLOR, NODE_DISABLE_COLORS, and FORCE_COLOR. Warning/error styling should pass { stream: process.stderr }, since its default is stdout. That matters here: the proposed helper checks process.stdout.isTTY but its callers write through console.warn() / console.error() to stderr, so redirecting the streams independently can suppress or emit ANSI incorrectly.

Please define one severity renderer backed by those stdlib APIs and apply it consistently—at minimum across the complete onboard preflight surface, with representative call-site tests—instead of landing a scoped helper and leaving the rest as follow-up debt.

…rn-error-color

Signed-off-by: Jason Ma <jama@nvidia.com>

# Conflicts:
#	src/lib/onboard/bridge-dns-preflight.ts
…xt (#6004)

Replace the module-load YW/RD-string preflight helpers with one severity
renderer backed by node:util.styleText. warnLine/failLine now decide color
from process.stderr (where console.warn/error write) and okLine/infoLine from
process.stdout, so styleText's per-stream capability and NO_COLOR /
NODE_DISABLE_COLORS / FORCE_COLOR handling apply to the stream the line
actually lands on. Previously color was computed from process.stdout.isTTY
while WARN/ERROR printed to stderr, so 'onboard >log' dropped their color on a
color-capable terminal and 'onboard 2>log' leaked raw ANSI into the file.

Apply the renderer across the onboard environment-preflight surface that emits
through console.warn/error: unsupported/under-provisioned runtime, low-memory
swap, and missing messaging providers in onboard.ts; Docker bridge / DNS in
bridge-dns-preflight.ts; sandbox GPU in sandbox-gpu-preflight.ts; and
sandbox->gateway bridge reachability in gateway-sandbox-reachability.ts. The
two low-memory warnings move from console.log to console.warn so their stream
matches their severity.

The single-sink log(msg) surfaces (wsl-docker-desktop-gpu and the interactive
provider/messaging verifiers) route all levels to stdout and are left for a
follow-up sink refactor, since stderr-keyed color there would recreate the
mismatch this change removes.

Signed-off-by: Jason Ma <jama@nvidia.com>
@jason-ma-nv

Copy link
Copy Markdown
Collaborator Author

Confirmed the stream-routing defect and reworked the change around it (3c9cb0c).

terminal-style.ts now exposes one severity renderer built on node:util.styleText. warnLine/failLine style against process.stderr (where console.warn/console.error write) and okLine/infoLine against process.stdout, so styleText's per-stream capability plus NO_COLOR / NODE_DISABLE_COLORS / FORCE_COLOR apply to the stream each line actually lands on. That removes the process.stdout.isTTY-vs-stderr mismatch: onboard >log keeps WARN/ERROR colored on the terminal, and onboard 2>log no longer writes raw ANSI into the file. Tests assert both redirect directions.

Applied the renderer across the onboard environment-preflight surface that emits through console.warn/console.error, including the onboard.ts paths you called out — unsupported / under-provisioned runtime and the Docker-not-running repro from #6004 — plus bridge/DNS, sandbox GPU, and sandbox→gateway bridge reachability. The two low-memory warnings moved from console.log to console.warn so their stream matches their severity.

One scope boundary I want your read on: wsl-docker-desktop-gpu.ts and the interactive provider/messaging verifiers (web-search-verify.ts, inference-providers/hermes.ts, nim-selection, messaging-conflict) emit through a single injected log(msg) sink that routes every level to stdout. Coloring their as stderr-keyed would recreate the exact mismatch this change removes, so a correct conversion there means refactoring those sinks to route by severity. Should that land in this PR, or as a tracked follow-up? Happy to do it here if you'd prefer one pass.

Comment thread src/lib/cli/terminal-style.test.ts Fixed
Revert the inline severity-line conversions in src/lib/onboard.ts. The
codebase-growth-guardrails check requires onboard.ts to be net-neutral or
smaller, and wrapping its multi-line WARN/ERROR messages in warnLine/failLine
(plus the new import) grew the file. Converting those inline lines needs a
module extraction, which is deferred to the same follow-up as the log-sink
severity refactor.

The severity renderer and the already-extracted preflight modules
(bridge-dns-preflight, sandbox-gpu-preflight, gateway-sandbox-reachability)
still carry the stream-keyed color fix, covering the Docker/DNS/GPU preflight
failures including the #6004 Docker-not-running repro.

Signed-off-by: Jason Ma <jama@nvidia.com>
Comment thread src/lib/cli/terminal-style.test.ts Fixed
@jason-ma-nv

Copy link
Copy Markdown
Collaborator Author

Correction to my previous note: I've reverted the inline conversions in src/lib/onboard.ts (61a3ecc). The codebase-growth-guardrails check requires onboard.ts to be net-neutral or smaller, and wrapping its multi-line WARN/ERROR messages in warnLine/failLine (plus the import) grows the file. Converting those inline lines the right way means extracting them to a module, so I'm folding that into the same follow-up as the single-sink log() refactor rather than fighting the budget with formatting.

What remains in this PR carries the actual fix: the stream-keyed styleText renderer, and its use across the already-extracted preflight modules — bridge-dns-preflight (Docker bridge / container DNS / host DNS), sandbox-gpu-preflight, and gateway-sandbox-reachability. That covers the #6004 Docker-not-running path, which surfaces through the bridge/daemon-reachability probes rather than the onboard.ts runtime lines.

Proposed follow-up (happy to do it here if you'd rather one pass): extract the onboard.ts preflight severity lines (unsupported / under-provisioned runtime) into a preflight-messages module so they can adopt the renderer without tripping the onboard entrypoint budget, and refactor the log(msg) sinks (wsl-docker-desktop-gpu, provider/messaging verifiers) to route by severity.

… adopt the renderer (#6004)

Move the inline onboard.ts preflight WARN/ERROR emitters — unsupported
runtime, under-provisioned runtime, low-memory, failed swap, and missing
messaging provider — into src/lib/onboard/preflight-messages.ts, where they use
the shared stream-keyed warnLine/failLine renderer. onboard.ts now calls
one-liners, so it shrinks (net -9 lines) and stays within the codebase-growth /
onboard-entrypoint budget instead of growing as the earlier inline attempt did.

WARN lines route through console.warn and ERROR lines through console.error, so
the renderer's stderr-keyed color matches the stream. The two low-memory
warnings therefore move from console.log to console.warn. Adds representative
call-site tests for each emitter, including the colima vs docker-desktop resize
branch.

This closes the onboard.ts side of #6004; the single-sink log() surfaces
(wsl-docker-desktop-gpu, provider/messaging verifiers) remain a separate
follow-up.

Signed-off-by: Jason Ma <jama@nvidia.com>
@jason-ma-nv

Copy link
Copy Markdown
Collaborator Author

Update: the onboard.ts extraction (follow-up #1) has landed (d004a26), so this PR now covers the onboard.ts side too.

The inline preflight severity emitters — unsupported / under-provisioned runtime, low-memory, failed swap, missing messaging provider — moved into src/lib/onboard/preflight-messages.ts and now use the shared renderer. onboard.ts calls one-liners and is net -9 lines, so it's within the growth budget rather than over it. WARN routes through console.warn, ERROR through console.error, so color matches the stream (the two low-memory warnings moved from console.log to console.warn accordingly). Added per-emitter call-site tests, including the colima vs docker-desktop resize branch.

Verified on a Linux Node 22.23.1 host (exact SHA d004a26): build:cli clean, 215 preflight/renderer tests pass, and a compiled-dist check confirms both redirect directions — onboard >log keeps WARN/ERROR colored on the terminal, onboard 2>log writes no ANSI into the log.

Remaining: only the single-sink log() surfaces (wsl-docker-desktop-gpu, provider/messaging verifiers), which need the sink to route by severity. Happy to fold that in here or track it separately — your call.

…6004)

CodeQL / code-quality flagged the unused `beforeEach` named import left
behind when the color tests moved to stream stubbing in afterEach.

Signed-off-by: Jason Ma <jama@nvidia.com>
cv added 7 commits July 3, 2026 13:58
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>

# Conflicts:
#	src/lib/onboard.ts
#	src/lib/onboard/sandbox-gpu-preflight.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv

cv commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Keep NO_COLOR authoritative when FORCE_COLOR is also set.

Make the color-depth fixtures branch-free.

Refresh generated source references after the preflight extraction.

Co-authored-by: Jason Ma <jama@nvidia.com>

Signed-off-by: Jason Ma <jama@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Route the missing or stale CDI failure through the shared stderr renderer.

Cover red terminal output and NO_COLOR plain output.

Preserve remediation and exit behavior.

Co-authored-by: Jason Ma <jama@nvidia.com>

Signed-off-by: Jason Ma <jama@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Route the preflight gateway recreation warning through the stderr severity renderer.

Preserve cleanup behavior and cover color-capable and NO_COLOR output.

Co-authored-by: Jason Ma <jama@nvidia.com>

Signed-off-by: Jason Ma <jama@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria
apurvvkumaria requested a review from cv July 4, 2026 00:53
@cv
cv merged commit 874296d into main Jul 4, 2026
42 checks passed
@cv
cv deleted the fix/6004-preflight-warn-error-color branch July 4, 2026 00:55
@ericksoa ericksoa mentioned this pull request Jul 4, 2026
21 tasks
ericksoa added a commit that referenced this pull request Jul 4, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [#6020](#6020) and
[#5876](#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [#6251](#6251) and
[#5989](#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [#6232](#6232),
[#6082](#6082),
[#6219](#6219),
[#6214](#6214),
[#6215](#6215),
[#6230](#6230), and
[#6260](#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [#6166](#6166),
[#6254](#6254),
[#6265](#6265),
[#6164](#6164), and
[#6017](#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [#6150](#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [#6234](#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [#6129](#6129),
[#5987](#5987),
[#5955](#5955), and
[#6220](#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [#5963](#5963),
[#6050](#6050),
[#6094](#6094),
[#6238](#6238),
[#5988](#5988),
[#6235](#6235),
[#6181](#6181), and
[#5986](#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [#6200](#6200),
[#6248](#6248),
[#6168](#6168),
[#6270](#6270), and
[#5649](#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] 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 — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


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

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary
This PR prepares the user-facing documentation for v0.0.74 before the
release plan is frozen.
It expands the release notes across the 56-commit train and closes
durable documentation gaps found during the pre-tag commit scan.

## Changes
- Expand the `v0.0.74` release notes to cover OpenShell 0.0.72, managed
MCP, progressive tool disclosure, LangChain Deep Agents Code,
onboarding, local inference, messaging, recovery, and contributor
workflows.
- Correct the `destroy` contract for retained per-name volumes,
gateway-unreachable `--force` cleanup, managed MCP ownership, and
same-name recovery.
- Document separate remediation for an unreachable container DNS
resolver versus one that answers with `NXDOMAIN` or `REFUSED`.
- Document the Windows on Arm N1X automatic Ollama safeguard and its
remaining large-model limitations.
- State that messaging conflicts abort rebuild before backup or
deletion, leaving the original sandbox intact.
- Link the agent-runnable value benchmark from the contributor task
index.
- Synchronize generated agent command variants.
- Validate with `npm run docs:sync-agent-variants` and `npm run docs`;
Fern completed with 0 errors and 2 existing warnings.
- Source summary:
- [NVIDIA#6020](NVIDIA#6020) and
[NVIDIA#5876](NVIDIA#5876) ->
`docs/about/release-notes.mdx`: Consolidate the OpenShell 0.0.72 policy
boundary and managed MCP lifecycle.
- [NVIDIA#6251](NVIDIA#6251) and
[NVIDIA#5989](NVIDIA#5989) ->
`docs/about/release-notes.mdx`: Summarize progressive tool disclosure
and sandbox-first inference controls.
- [NVIDIA#6232](NVIDIA#6232),
[NVIDIA#6082](NVIDIA#6082),
[NVIDIA#6219](NVIDIA#6219),
[NVIDIA#6214](NVIDIA#6214),
[NVIDIA#6215](NVIDIA#6215),
[NVIDIA#6230](NVIDIA#6230), and
[NVIDIA#6260](NVIDIA#6260) ->
`docs/about/release-notes.mdx`: Summarize the experimental LangChain
Deep Agents Code status, secret, version, rebuild, snapshot, and MCP
boundaries.
- [NVIDIA#6166](NVIDIA#6166),
[NVIDIA#6254](NVIDIA#6254),
[NVIDIA#6265](NVIDIA#6265),
[NVIDIA#6164](NVIDIA#6164), and
[NVIDIA#6017](NVIDIA#6017) ->
`docs/about/release-notes.mdx`: Summarize BuildKit prebuild, validated
image reuse, bounded readiness, and preflight improvements.
- [NVIDIA#6150](NVIDIA#6150) ->
`docs/about/release-notes.mdx` and `docs/reference/troubleshooting.mdx`:
Separate unreachable-resolver remediation from reachable-but-rejected
DNS responses.
- [NVIDIA#6234](NVIDIA#6234) ->
`docs/about/release-notes.mdx`,
`docs/inference/use-local-inference.mdx`, and
`docs/get-started/windows-preparation.mdx`: Document N1X automatic 9B
selection and the remaining explicit-large-model boundary.
- [NVIDIA#6129](NVIDIA#6129),
[NVIDIA#5987](NVIDIA#5987),
[NVIDIA#5955](NVIDIA#5955), and
[NVIDIA#6220](NVIDIA#6220) ->
`docs/about/release-notes.mdx`,
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Document messaging policy
persistence, status, and the pre-destructive conflict check.
- [NVIDIA#5963](NVIDIA#5963),
[NVIDIA#6050](NVIDIA#6050),
[NVIDIA#6094](NVIDIA#6094),
[NVIDIA#6238](NVIDIA#6238),
[NVIDIA#5988](NVIDIA#5988),
[NVIDIA#6235](NVIDIA#6235),
[NVIDIA#6181](NVIDIA#6181), and
[NVIDIA#5986](NVIDIA#5986) ->
`docs/about/release-notes.mdx`, `docs/reference/commands.mdx`, and
`docs/reference/commands-nemohermes.mdx`: Summarize day-two recovery and
clarify retained-volume and local-only destroy semantics.
- [NVIDIA#6200](NVIDIA#6200),
[NVIDIA#6248](NVIDIA#6248),
[NVIDIA#6168](NVIDIA#6168),
[NVIDIA#6270](NVIDIA#6270), and
[NVIDIA#5649](NVIDIA#5649) ->
`docs/about/release-notes.mdx` and `CONTRIBUTING.md`: Summarize
contributor setup and verification improvements and expose the advisory
value benchmark.

## Type of Change

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

## Quality Gates
<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: documentation-only release
preparation; generated-variant synchronization and the Fern docs build
validate the changed pages and routes.
- [x] 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 — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification
<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration 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: tests
are not applicable to this documentation-only change; `npm run docs`
validates the source and generated routes.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [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)
- [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)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>


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

## Summary by CodeRabbit

* **Documentation**
* Expanded setup guidance for Windows on Arm devices with safer default
local model selection.
* Clarified local inference and sandbox messaging behavior, including
conflict checks before rebuilds and safer recovery steps.
* Updated destroy/rebuild/reference docs with more detailed warnings,
failure handling, and volume-retention guidance.
* Improved troubleshooting instructions for Docker DNS issues with
clearer paths for unreachable vs. blocked resolvers.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output bug-fix PR fixes a bug or regression

Projects

None yet

6 participants