Skip to content

test: make fs-safe hardlink tests compatible with Windows#90273

Open
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/fs-safe-hardlink-skip-win32
Open

test: make fs-safe hardlink tests compatible with Windows#90273
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/fs-safe-hardlink-skip-win32

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #7057

What Problem This Solves

The fs-safe hardlink-related test cases were hardcoded to be skipped on Windows, resulting in lost test coverage. This assumption is incorrect, as Windows NTFS local file systems do support hardlinks.

Why This Change Was Made

Replaces the hardcoded Windows skips (process.platform !== "win32") in the fs-safe hardlink-related test cases with a dynamic canCreateHardlinks capability check. If hardlink creation is supported by the partition and environment, the tests will now run on Windows.

User Impact

Improves test accuracy and coverage for Windows environments without affecting production behavior for the end user.

Evidence

The following test execution log shows the test run on Windows. All hardlink tests executed and passed successfully (25 passed, 9 skipped):

 RUN  v4.1.7 C:/Users/ANIRUDDHA/.gemini/antigravity/scratch/openclaw

 ✓  infra  ../../src/infra/fs-safe.test.ts (34 tests | 9 skipped) 1263ms

 Test Files  1 passed (1)
      Tests  25 passed | 9 skipped (34)

Copilot AI review requested due to automatic review settings June 4, 2026 09:28
@openclaw-barnacle openclaw-barnacle Bot added size: XS triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 4, 2026
Copilot AI reviewed Jun 4, 2026
View reviewed changes

Copilot AI 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.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates fs-safe hardlink-related tests to skip based on actual hardlink capability rather than OS platform heuristics.

Changes:

  • Added a runtime hardlink capability probe (canCreateHardlinks) using node:fs + os.tmpdir().
  • Replaced it.runIf(process.platform !== "win32") with it.skipIf(!canCreateHardlinks) for hardlink tests.

Comment thread src/infra/fs-safe.test.ts Outdated
Comment thread src/infra/fs-safe.test.ts Outdated
Comment thread src/infra/fs-safe.test.ts Outdated
Comment thread src/infra/fs-safe.test.ts Outdated
@clawsweeper

clawsweeper Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 5, 2026, 4:22 AM ET / 08:22 UTC.

Summary
The PR replaces three Windows-only skips in src/infra/fs-safe.test.ts with an async hardlink capability probe so hardlink-alias tests can run on Windows filesystems that support them.

PR surface: Tests +31. Total +31 across 1 file.

Reproducibility: yes. at source level: current main and v2026.6.11 still use Windows platform skips for the three hardlink-alias tests. The PR body also includes after-change Windows terminal output for the focused test file.

Review metrics: 2 noteworthy metrics.

  • Hardlink Test Gates: 3 changed. All affected hardlink-alias tests move from OS-name gating to filesystem capability gating, which is the behavior maintainers need to review.
  • Closing Reference Scope: 1 broad issue referenced as closing. The merge would update issue state outside the changed test file, so maintainers should decide whether that is intentional.

Stored data model
Persistent data-model change detected: serialized state: src/infra/fs-safe.test.ts. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Convert the broad closing reference to a non-closing mention unless maintainers intentionally want the linked WSL issue closed by this PR.

Risk before merge

Maintainer options:

  1. Remove The Closing Reference (recommended)
    Change the PR body to reference Flaky tests on Windows/WSL: timeouts and ENOENT in pi-tools workspace-paths & safe-bins #7057 without closing syntax before merge.
  2. Merge With Intentional Auto-Close
    Keep the closing reference only if maintainers intentionally want this narrow test PR to close the broader WSL issue.

Next step before merge

  • [P2] No code repair job is appropriate; maintainers should review the focused test patch and correct or explicitly accept the broad closing reference before merge.

Maintainer decision needed

  • Question: Should the PR body keep closing Flaky tests on Windows/WSL: timeouts and ENOENT in pi-tools workspace-paths & safe-bins #7057, or should that reference be changed before merge?
  • Rationale: The code change is narrow and reviewable, but GitHub closing syntax would close a broader issue that current evidence says remains unresolved.
  • Likely owner: steipete — steipete is the strongest fs-safe history owner and previously provided maintainer-style guidance on the linked WSL issue.
  • Options:
    • Remove Closing Syntax (recommended): Convert the body to a non-closing reference before merge so the broader WSL issue remains open.
    • Accept The Auto-Close: Merge as-is only if maintainers decide this fs-safe test coverage change is enough to close the broader WSL issue.

Security
Cleared: The diff is limited to one test file and adds no dependency, workflow, package-resolution, secrets, or runtime security surface.

Review details

Best possible solution:

Merge the test-only capability gate after the closing reference is narrowed or removed, so the broader WSL issue stays open unless maintainers intentionally close it.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main and v2026.6.11 still use Windows platform skips for the three hardlink-alias tests. The PR body also includes after-change Windows terminal output for the focused test file.

Is this the best way to solve the issue?

Yes for the code: probing actual hardlink capability is narrower and more maintainable than an OS-name skip. The issue-management part should be corrected because the linked closing issue is broader than this patch.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4deb63c9791c.

Label changes

Label changes:

  • add merge-risk: 🚨 automation: The PR body's closing syntax would let GitHub auto-close a broader WSL test-stability issue that this fs-safe-only patch does not fully address.

Label justifications:

  • P3: This is a low-risk test-only Windows coverage improvement with no production runtime, config, provider, session, or security-surface change.
  • merge-risk: 🚨 automation: The PR body's closing syntax would let GitHub auto-close a broader WSL test-stability issue that this fs-safe-only patch does not fully address.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body includes after-change Windows terminal output showing the focused src/infra/fs-safe.test.ts run passing with the hardlink tests exercised.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body includes after-change Windows terminal output showing the focused src/infra/fs-safe.test.ts run passing with the hardlink tests exercised.
Evidence reviewed

PR surface:

Tests +31. Total +31 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 34 3 +31
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 34 3 +31

What I checked:

Likely related people:

  • steipete: The fs-safe wrapper and test history includes major consolidation and extraction work by steipete, including the large fs-safe primitive extraction and repeated fs-safe test updates. (role: feature owner; confidence: high; commits: 538605ff44d2, 34daed1d1eae, e7dfc88bfa34; files: src/infra/fs-safe.test.ts, src/infra/fs-safe.ts, src/test-utils/tracked-temp-dirs.ts)
  • shakkernerd: Recent main history shows shakkernerd touching fs-safe test assertions and guard behavior near the affected test module. (role: recent area contributor; confidence: medium; commits: 986025afe425, 896e2edd59ab; files: src/infra/fs-safe.test.ts)
  • frankekn: Commit history shows frankekn added adjacent fs-safe hardlink/write protection coverage in the same fs-safe surface. (role: adjacent hardlink test contributor; confidence: medium; commits: 96e4975922de; files: src/infra/fs-safe.test.ts, src/infra/fs-safe.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-01T09:44:59.343Z sha 30cea5a :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 4, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label Jun 4, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 4, 2026
@aniruddhaadak80

aniruddhaadak80 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@BradGroux BradGroux mentioned this pull request Jun 4, 2026
Draft
@aniruddhaadak80
aniruddhaadak80 force-pushed the fix/fs-safe-hardlink-skip-win32 branch from feaa297 to 0f7f75f Compare June 28, 2026 17:45
@aniruddhaadak80

aniruddhaadak80 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main. Ready for re-review.

@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jun 28, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 28, 2026
@vincentkoc
vincentkoc force-pushed the fix/fs-safe-hardlink-skip-win32 branch from 0f7f75f to 18e5a8b Compare July 1, 2026 08:50
@vincentkoc
vincentkoc force-pushed the fix/fs-safe-hardlink-skip-win32 branch from 5a9cb72 to 30cea5a Compare July 1, 2026 08:52
@openclaw-barnacle openclaw-barnacle Bot removed the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 5, 2026
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. label Jul 5, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jul 5, 2026
Open
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 20, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. size: XS stale Marked as stale due to inactivity status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flaky tests on Windows/WSL: timeouts and ENOENT in pi-tools workspace-paths & safe-bins

3 participants

@aniruddhaadak80 @Patrick-Erichsen