GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Fix Windows nightly test failures and absolute --patch-dir bound by mikolalysenko · Pull Request #2049 · SocketDev/socket-patch

Conversation

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

Problem

Scheduled CI on main has failed 5 nights running (11-06 → 11-10). On the latest run, 42763778797 (main @ c751724), the only failing jobs are test (windows-latest, 1) and test (windows-latest, 2). Those legs run only at night (see #1720), so PR and merge-queue CI never catch these failures. Both legs use --no-fail-fast, so this is the full set:

Test	Cause
`patch::user_patches::tests::an_absolute_patch_dir_inside_the_repository_is_read`	package.json built with `format!`, so `C:\...` became invalid JSON escapes. The fix below also found a real bug behind this test.
`patch::user_patches::tests::the_repository_bound_holds_for_workspace_runs_only`	`ends_with("fixes/left-pad+1.3.0.patch")` on a `\`-separated path
`utils::cargo_build_cache::tests::a_config_above_the_repository_is_named_when_its_target_dir_is_unread`	`contains(".cargo/config.toml")` checked against `.cargo\config.toml`
`utils::repo_root::upward_tests::pnpm_workspace_file_above_the_repository_fails_closed`	expected the canonical `\\?\C:\...` path, but `governing_workspace_file` deliberately returns it through `without_verbatim_prefix`
`vex::discover::user_patched::tests::patch_package_patches_mark_yarn_classic_vlt_and_npm_pins`	`/` substring checked against `patches\left-pad+1.3.0.patch`
`vex::discover::user_patched::tests::workspace_member_patches_agree_with_the_consent_gate`	`packages/a` checked against `packages\a\package.json`
`hosted_memory_engine::user_patched_npm_packages_are_never_pinned` (cli)	same separator mismatch in the skip detail

Root cause

Six of the failures are test assertions that assume Unix paths: / separators, unescaped JSON, and \\?\-prefixed expectations.
One is a real production bug. Bound::outside (user_patches.rs) compares the path against the canonical repo root, which on Windows is \\?\C:\proj. A script's --patch-dir C:\proj\fixes is written the way users type it, and Path::starts_with treats the VerbatimDisk and Disk prefixes as different. So on Windows every absolute in-repo --patch-dir was reported as outside the repository, and every npm package was protected. The old test passed a \\?\ path, which no real package.json contains.

Fix

Production: Bound::outside now compares both sides through pnpm_workspace::without_verbatim_prefix. This changes nothing on Unix.
Tests:
- The absolute --patch-dir test now spells the directory the way a user does: unprefixed, quoted, and built through serde_json so the backslashes are escaped.
- Separator-sensitive substring checks run on the text after mapping \ to /.
- The pnpm test expects without_verbatim_prefix(file).

No assertion is weaker.

Proof

Linux:
- cargo test -p socket-patch-core --lib -- patch::user_patches vex::discover::user_patched utils::repo_root utils::cargo_build_cache: 73 passed.
- cargo test -p socket-patch-cli --test hosted_memory_engine --test user_patches: 57 passed.
rustfmt --check on the touched files is clean.
Clippy adds no new findings. Local 1.93.1 reports the same pre-existing sites on unmodified main.
Windows: verified. A CI run dispatched with os=windows on this head, 42772455462, passed test (windows-latest, 1) and test (windows-latest, 2). Its only failures are the mode_migration_vlt e2e legs, which fail the same way on main's nightly on every OS and are not this PR's (see the comment below).

Review findings not fixed

/code-review high:

Hosted engine detail text uses OS separators. In the hosted in-memory engine, the user_patch_conflict detail text uses OS separators (Path::display). Production servers run Linux, so changing how in-memory sources are rendered is a larger change and out of scope here. Worth a follow-up.
No shared slashed helper. The .replace('\\', "/") call is repeated inline, and there is no shared helper for it. Core has no test-util module that all of these tests share. That refactor is left out of a CI fix.

Where tests run

No tests were removed or moved.

🤖 Generated with Claude Code

https://claude.ai/code/session_011f9aeRDbxjatib28cpxPqQ

Note

Medium Risk
Changes workspace-bound patch discovery on Windows (consent gate / user-patch conflicts); incorrect comparison previously over-protected npm packages.

Overview
Fixes Windows nightly CI failures and a production bug where in-repo absolute patch-package --patch-dir paths were treated as outside the git repository.

Production: Bound::outside in user_patches.rs now strips Windows \\?\ verbatim prefixes on both sides before starts_with, so user-typed C:\repo\fixes matches canonical \\?\C:\repo. Unix behavior is unchanged.

Tests: Assertions that assumed / separators or \\?\ paths now normalize with .replace('\\', "/") or expect without_verbatim_prefix. The absolute --patch-dir test builds package.json via serde_json with a quoted, user-style path so JSON escaping is valid on Windows.

^{Reviewed by Cursor Bugbot for commit c40cda6. Configure here.}

Generated by Claude Code

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Fix 7 Windows-only test failures in nightly CI

The scheduled CI run has been red on main for five nights. Its
`test (windows-latest, 1|2)` legs (nightly-only) fail seven tests
that pass on Linux and macOS, all from test-side assumptions about
Unix paths:

- an_absolute_patch_dir_inside_the_repository_is_read built its
  package.json with format!, so the Windows path's backslashes were
  invalid JSON escapes; build it with serde_json instead.
- five assertions matched `/`-separated substrings in user-facing
  detail text that Windows prints with `\`; compare after mapping
  `\` to `/`.
- pnpm_workspace_file_above_the_repository_fails_closed expected the
  canonical `\\?\` path, but governing_workspace_file deliberately
  strips that prefix; expect without_verbatim_prefix(file).

No production code changes; every assertion still checks the same
path on every OS.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_011f9aeRDbxjatib28cpxPqQ

Mikola Lysenko (mikolalysenko) added the ci-janitor Opened by the CI janitor routine (flakes, redundant tests, CI perf) label

Nov 10, 2026

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator Author

bugbot run

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Nov 10, 2026

View reviewed changes

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Read a user-typed absolute --patch-dir as in-repo

c40cda6

On Windows the bound's repository root is canonical (`\\?\C:\...`)
while a script names its patch directory as users type it
(`C:\...`), and Path::starts_with treats the verbatim and plain disk
prefixes as different. Every absolute in-repo `--patch-dir` was
reported as outside the repository, protecting every npm package.
Compare both sides without the verbatim prefix, and make the test
spell the directory the way a user does (quoted, unprefixed).

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_011f9aeRDbxjatib28cpxPqQ

Mikola Lysenko (mikolalysenko) changed the title ~~Fix 7 Windows-only test failures keeping nightly CI red~~ Fix Windows nightly test failures and absolute --patch-dir bound

Nov 10, 2026

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator Author

bugbot run

Generated by Claude Code

cursor[bot] reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot reviewed

Nov 10, 2026

View reviewed changes

cursor Bot left a comment

Copy link

Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

^{Reviewed by Cursor Bugbot for commit c40cda6. Configure here.}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator Author

In the Windows dispatch run 42772455462, e2e-windows (mode_migration_vlt, 1.0.0-rc.14 | 1.2.0) fail. This failure isn't caused by this PR.

The same legs failed on main in last night's scheduled run 42763778797 (main @ c751724). They failed on all three OSes there: e2e-extended on ubuntu (3 legs), e2e-macos (1.2.0) and e2e-windows (2 legs).

The failing assertion is vlt_pinned_matrix_migration_hosted_scan_keeps_agent_patched_tree (assert_advisory). The redirect_vlt_reinstall_required detail came back as the "could not check 1 installed copies … run vlt ci" variant. The test expected the "fresh checkouts install them with vlt ci" variant. This PR's diff doesn't touch vlt redirect or install checks, and no fix exists yet.

I'm leaving it out of this PR. It's the next CI-janitor target. This PR's own target is the test (windows-latest, *) legs, which are still running.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) mentioned this pull request

Nov 10, 2026

Fix nightly vlt migration e2e after #2021's heal change #2050

Merged

Mikola Lysenko (mikolalysenko) added the Ready for review Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review label

Nov 10, 2026

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator Author

Labeled Ready for review by the burn-down agent.

Head: c40cda63fb7bd7a6517d726b26836d14809e1821
CI: 95/98 checks on this head passed or were skipped. The 3 failures are main-wide: e2e-windows (mode_migration_vlt, 1.0.0-rc.14 | 1.2.0) and the ci-ok (windows only) gate that rolls them up. The same vlt legs fail on main's latest scheduled CI run 42763778797 on all three OSes. Fix nightly vlt migration e2e after #2021's heal change #2050 is fixing them.
That main run's test (windows-latest, 1|2) failures are the ones this PR fixes, and they pass here.
Bugbot: reviewed this head, no findings; no open review threads.
Mergeable, no conflicts, doesn't touch CHANGELOG.md.
For reviewers: the one production change is Bound::outside in crates/socket-patch-core/src/patch/user_patches.rs. It strips the Windows \\?\ prefix on both sides before comparing paths. The other changes are test-only.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) enabled auto-merge

November 10, 2026 08:54

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Copy link

Copy Markdown

Collaborator Author

Final review brief

What it does. Fixes the 7 tests that fail only on the nightly Windows test legs (#1720). Six were test assertions that assumed / separators or \\?\ paths. The seventh exposed a real Windows bug: Bound::outside compared a user-typed C:\proj\fixes --patch-dir against the canonical \\?\C:\proj repo root, so every absolute in-repo patch dir counted as outside the repository. Both sides are now compared without the verbatim prefix.

Risk: low. One production line, a no-op on Unix. On Windows it makes an in-repo absolute --patch-dir count as inside the repository; .. is resolved lexically before the prefix is stripped, so nothing outside the repo becomes readable. Everything else is test-only, and no assertion is weaker.

Look here

user_patches.rs:1235-1240: the production fix.
user_patches.rs:2119-2134: the test now writes the path the way a user does (unprefixed, quoted, JSON-escaped). The old test passed a \\?\ path, which no real package.json contains, so it hid the bug.
repo_root_upward_tests.rs:139: expects the unprefixed path that governing_workspace_file deliberately returns.
The other 4 test files only map \ to / before substring checks.

Verified

Read the full diff (5 files, +35/−18). CHANGELOG.md is untouched. No debug leftovers, nothing out of scope.
Checked without_verbatim_prefix (\\?\C: and \\?\UNC\ cases) and the containment logic for traversal.
Ran cargo test -p socket-patch-core --lib -- patch::user_patches locally: 35 passed.
Required checks ci-ok and clippy are green on this head. The Windows dispatch run 42772455462 passes test (windows-latest, 1|2).
Its only red checks are e2e-windows (mode_migration_vlt, …) and the non-required ci-ok (windows only) rollup. Those fail on main's nightly on every OS too, and Fix nightly vlt migration e2e after #2021's heal change #2050 fixes them.
Bugbot reviewed this head with no findings. No open review threads.

Changes I made. None.

Open questions. None blocking. The description lists two follow-ups it chose not to do: OS separators in the hosted engine's detail text, and a shared slashed test helper.

Auto-merge (squash) is armed, so approving sends this straight to the merge queue.

Generated by Claude Code

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Wenxin Jiang (Wenxin-Jiang) approved these changes

Nov 10, 2026

View reviewed changes

Mikola Lysenko (mikolalysenko) added this pull request to the merge queue

Nov 10, 2026

Merged via the queue into main with commit a8ceeb7

Nov 10, 2026

95 of 98 checks passed

Uh oh!

There was an error while loading. Please reload this page.

Mikola Lysenko (mikolalysenko) deleted the ci-janitor/windows-nightly-paths branch

November 10, 2026 14:39

This was referenced Nov 10, 2026

CI perf (coverage tradeoff): Windows-only path bugs reach main because test (windows-latest, *) runs only nightly #1720

Open

Keep the ruby crawl test off the host's Bundler env #2056

Open

Fix Windows nightly: expect /-separated scan headings #2061

Open

Reviewers

cursor[bot]

cursor[bot] left review comments

Wenxin Jiang (Wenxin-Jiang)

Wenxin-Jiang approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

ci-janitor

Opened by the CI janitor routine (flakes, redundant tests, CI perf)

Ready for review

Agent-verified: mergeable, CI green, Bugbot clean — awaiting human review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Root cause

Fix

Proof

Review findings not fixed

Where tests run

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Uh oh!

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026

Final review brief

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mikola Lysenko (mikolalysenko) commented Nov 10, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

cursor Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.