Skip to content

fix(test): remove the duplicate WSL spy from the connect harness - #9019

Closed
Dreamstick9 wants to merge 1 commit into
NVIDIA:mainfrom
Dreamstick9:fix/harness-duplicate-wsl-spy
Closed

fix(test): remove the duplicate WSL spy from the connect harness#9019
Dreamstick9 wants to merge 1 commit into
NVIDIA:mainfrom
Dreamstick9:fix/harness-duplicate-wsl-spy

Conversation

@Dreamstick9

@Dreamstick9 Dreamstick9 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Summary

test/support/connect-flow-test-harness.ts installs two spies on platform.isWsl. The explicit options.isWsl spy added in #8951 runs first, then the platform pin added in #8984 replaces it and delegates to the binding captured just before, so a case that passes isWsl: true resolves against the environment instead of the option. On main today, repairs a WSL Ollama route without requiring an auth proxy token takes the non-WSL branch, calls the auth-proxy probe, and exits 1. This change removes the pin and keeps the explicit option.

Changes

  • test/support/connect-flow-test-harness.ts: remove the platform.isWsl pin and its now-unused WslDetectionOptions import. The explicit options.isWsl supersedes it — it states the WSL decision per case instead of inferring one from the host, and it already delivers the host independence the pin was added for.

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: the case this restores, repairs a WSL Ollama route without requiring an auth proxy token, is the regression test; it fails on main and passes here.
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: test-support change only; no user-facing surface.
  • 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:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: one test-support file; the removed lines were an internal comment and a spy, and no documentation page references the connect harness or WSL detection in tests.
  • Agent: Claude Code

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run validate:pr passed after refreshing origin/main when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result: npx vitest run --project cli over all seven suites that use the harness → 92 passed; the same command on unmodified main → 1 failed, 91 passed. npm run typecheck:cli clean.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: not applicable; the change removes one spy from one test-support file, validated by every suite that consumes it.
  • 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)

Note on an unrelated red check: static-checks fails on this branch through source-architecture, reporting src/lib/onboard: root files fell from 309 to 308. Lower the limit. That reproduces on unmodified main with no changes applied, so it is a stale ratchet in ci/source-architecture-budget.json rather than anything in this diff. I left it alone to keep this change to one concern; happy to send the one-line ratchet update separately if that helps.

Signed-off-by: Kushagar Garg dreamstick909@gmail.com

Summary by CodeRabbit

  • Tests
    • Updated connection-flow test behavior to use the platform’s native WSL detection.
    • Added support for explicitly configured WSL detection overrides in test scenarios.

The connect harness installs two spies on platform.isWsl. The explicit
options.isWsl spy from NVIDIA#8951 runs first, then the platform pin from
NVIDIA#8984 replaces it and delegates to the captured binding, so a case that
passes isWsl: true resolves to the environment instead of the option.
"repairs a WSL Ollama route without requiring an auth proxy token" then
takes the non-WSL branch and exits 1.

Remove the pin. The explicit option supersedes it: it states the WSL
decision per case instead of inferring one from the host, and it already
keeps the case host-independent, which is what the pin was for. All
seven suites that use the harness pass.

Signed-off-by: Kushagar Garg <dreamstick909@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Aug 13, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 8ea12333-1f4d-458a-8f4c-da1a83106669

📥 Commits

Reviewing files that changed from the base of the PR and between 7c4d2f0 and 292fdfb.

📒 Files selected for processing (1)
  • test/support/connect-flow-test-harness.ts
💤 Files with no reviewable changes (1)
  • test/support/connect-flow-test-harness.ts

📝 Walkthrough

Walkthrough

The test harness removes the WslDetectionOptions import and forced Linux override for platform.isWsl. It now uses platform detection and explicit mocks from options.

Changes

Cohort / File(s) Summary
WSL harness behavior
test/support/connect-flow-test-harness.ts
Removes the forced Linux WSL detection path and its type import.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Mergeability Score: ⚪ Minimal · up to 292fd

This localized test-harness change preserves the explicit per-case WSL behavior, and no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Suggested labels: platform: wsl, provider: ollama, bug-fix

Suggested reviewers: prekshivyas

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removing the duplicate WSL spy from the connect test harness.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@Dreamstick9

Copy link
Copy Markdown
Contributor Author

PR #9013 supersedes this change. It keeps the platform pin and puts it behind the explicit options.isWsl. A case that sets the option gets the option. A case that does not set the option keeps the host-independent pin. This shape is better than removal. I ran the seven suites that use the harness on current main. All 92 tests passed. I close this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant