Skip to content

test(ci): point server-scope qualification fixtures at SSH after relay deletion - #27392

Merged
OrcaWin merged 1 commit into
mainfrom
fix/node-server-change-scope-after-relay-delete
Oct 11, 2026
Merged

OrcaWin merged 1 commit into
mainfrom
fix/node-server-change-scope-after-relay-delete

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​16 $\color{#cf222e}{\Huge{\mathbf{−}}}$​16 0
Prod 0 0 0 0

ELI5

main is failing one CI test file. Two PRs merged within minutes of each other: one added tests that use the SSH relay server's folder as their example of "code shared across machines", and the other deleted that folder and removed it from the list of such code. Each passed on its own, but together the tests point at a rule that no longer exists. This PR points the tests at a folder the rule still covers.

What Changed

  • Before: on main, config/scripts/node-server-change-scope.test.mjs fails 7 tests ("keeps every host when the modified relay production module is an actual consumer" and the 6 "retains complete-path qualification..." cases). Each expects the full test matrix (qualification: true, every runner) and gets Linux only.
  • Cause: [CI] Reduce test teardown and scope server qualification #27380 added these tests using src/relay/... paths as their cross-host example. refactor(relay): delete the SSH relay server, its build assets and tests #27215 then removed 'src/relay/' from CROSS_HOST_PREFIXES in config/scripts/node-server-qualification.mjs (the relay server it covered is gone), so those paths no longer turn on the full matrix.
  • After: the test fixtures use src/main/ssh/... paths. src/main/ssh/ is still in CROSS_HOST_PREFIXES. The fixtures are files made up inside a temporary folder, so only the path prefix matters. Test-only change; the scope logic is untouched.

Why

The logic change in #27215 is correct: with the relay server deleted, src/relay/ has nothing left to cover. The tests only needed a cross-host folder as an example, and src/main/ssh/ is the closest one that still exists. Putting src/relay/ back in the list would bring back a rule for a deleted folder just so the tests pass.

Each case still tests the same rule. As a check, I removed 'src/main/ssh/' from CROSS_HOST_PREFIXES and exactly the same 7 tests failed again.

Linked Issue

N/A (internal fix for red main, caused by #27380 and #27215 merging together)

Visual Proof

N/A: CI test fixtures only, nothing visible changes.

Testing

  • I manually tested these changes locally

  • Automated tests added/updated, or explained why not below

  • Before (on main 92fb883): npx vitest run --config config/vitest.config.ts config/scripts/node-server-change-scope.test.mjs gives Tests 7 failed | 91 passed (98)

  • After: that file plus node-server-qualification.test.mjs and check-process-host-imports.test.mjs gives Test Files 3 passed (3), Tests 184 passed (184)

  • Check: removing src/main/ssh/ from the cross-host list makes the same 7 fail again

  • oxlint, oxfmt --check, and the changed-code quality gate pass. Tested on macOS; the change is a path string in a test, nothing differs by platform.

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

…y deletion

The relay server deletion removed src/relay/ from the cross-host qualification
prefixes, but the scope tests merged alongside it still used src/relay/ paths
as their cross-host example. Use src/main/ssh/, which still carries that rule.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
@coderabbitai

coderabbitai Bot commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

Walkthrough

The qualification-scope tests replace unrelated relay test paths and scenarios with SSH test paths. Updated cases cover platform selection, production-module consumption, modification evidence, protected tests, and broken imports.


Priority: ⬇️ Low

Merge Risk: 🔵 Low · up to d8e73

The change is test-only, but six cases may pass through the SSH full-qualification rule without proving malformed modification evidence is rejected. Direct scope-state assertions would close this bounded coverage gap; merge is reasonable with that follow-up.

Pre-merge checks | Passed 4 | Failed 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly identifies the test fixture update and the replacement of the deleted relay path with SSH coverage.
Description check Passed The description is complete and explains the failure, cause, change, rationale, testing, and scope. It correctly marks visual proof as not applicable and identifies the change as an internal test-only…
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.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR

🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

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

🧹 Nitpick comments (1)
config/scripts/node-server-change-scope.test.mjs (1)

413-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the modification-evidence state directly.

The six cases only assert the final qualification. Because changed contains src/main/ssh/pinned-runtime-materializer-context.test.ts, the SSH prefix independently selects every runner whenever the selector scans changedFiles. Add assertions that malformed evidence does not produce graphResolved or matchedFiles; otherwise these cases can still pass without proving the status-specific evidence handling.

Suggested fix
     )
     expect(scope.shouldRun).toBe(true)
+    expect(scope.graphResolved).toBeUndefined()
+    expect(scope.matchedFiles).toBeUndefined()
     expect(nodeServerQualification(changed, scope)).toEqual({

A generic non-cross-host path alone would not preserve the current qualification: true expectation. Directly asserting the scope state tests the intended contract without relying on a cross-host fixture.


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 793d5cac-b1d7-4ecd-bf6c-6ce1f097e8b3
📥 Commits

Reviewing files that changed from the base of the PR and between 92fb883 and d8e7376.

📒 Files selected for processing (1)
  • config/scripts/node-server-change-scope.test.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@OrcaWin
OrcaWin merged commit b241ba5 into main Oct 11, 2026
56 of 58 checks passed
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.

2 participants