Skip to content

test: make refresh symlink tests compatible with Windows#97438

Open
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/refresh-symlink-skip-win32-final
Open

test: make refresh symlink tests compatible with Windows#97438
aniruddhaadak80 wants to merge 2 commits into
openclaw:mainfrom
aniruddhaadak80:fix/refresh-symlink-skip-win32-final

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

The automated tests for the refresh symlink feature were failing on Windows. The test capability probe canCreateDirectorySymlink() correctly uses directorySymlinkType (which uses junction on Windows and dir on POSIX systems), but the actual test cases were hardcoded to use the dir type. Windows requires administrator privileges for dir symlinks, which means that while the capability probe passed using junction, the tests still attempted to create dir symlinks and crashed.

Why This Change Was Made

This change ensures that the tests themselves use the same symlink type (directorySymlinkType) that the capability probe checked. This makes the tests behave consistently with the probe and allows them to execute successfully on Windows runners that support junction but not dir.

User Impact

Improves cross-platform development and test reliability on Windows without impacting actual production behaviour.

Evidence

Modified src/skills/runtime/refresh.test.ts to replace hardcoded dir with directorySymlinkType in the s.symlink calls. Tests now pass on Windows platforms.

Related: #97438

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 5, 2026, 4:15 AM ET / 08:15 UTC.

Summary
The PR replaces four win32-only skips in refresh watcher symlink tests with a directory-symlink capability probe and uses junction on Windows for the test symlink setup.

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

Reproducibility: yes. for the source-level test-portability path: current main skips these symlink watcher tests on Windows, while PR head enables them through a directory-link capability probe. I did not reproduce the after-fix Windows run on a real host.

Review metrics: 1 noteworthy metric.

  • Windows Symlink Gates: 4 tests changed. All affected tests now depend on the new capability probe, so one Windows proof run can validate the full changed test surface.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P1] Add redacted Windows terminal output, a terminal screenshot, or a linked artifact showing node scripts/run-vitest.mjs src/skills/runtime/refresh.test.ts or an equivalent focused refresh test command passing after this patch.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR has a Windows pass claim but no redacted terminal output, logs, screenshot, recording, or linked artifact showing the changed refresh test passing after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

  • [P1] The intended Windows junction-backed path is not proven by visible after-fix output, and Linux CI cannot demonstrate that Windows-specific behavior.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the test-only direction and merge after redacted Windows terminal output or an artifact shows the refresh test passing on a junction-capable Windows host and required checks are green.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] No automated repair is appropriate because the code blocker appears fixed; the contributor needs to add real Windows behavior proof before merge readiness can be cleared.

Security
Cleared: Cleared: the diff only changes a colocated test file and does not alter dependencies, workflows, secrets, package metadata, or runtime code.

Review details

Best possible solution:

Keep the test-only direction and merge after redacted Windows terminal output or an artifact shows the refresh test passing on a junction-capable Windows host and required checks are green.

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

Yes for the source-level test-portability path: current main skips these symlink watcher tests on Windows, while PR head enables them through a directory-link capability probe. I did not reproduce the after-fix Windows run on a real host.

Is this the best way to solve the issue?

Yes, the implementation shape is the narrowest maintainable solution: it stays in the colocated tests and reuses the established Windows junction pattern. The missing piece is contributor-supplied real behavior proof, not another code change.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 07bf384a8b3f.

Label changes

Label changes:

  • add P3: This is a low-risk test portability cleanup with no production runtime change and a remaining proof gap.
  • remove P2: Current review triage priority is P3, so this older priority label is no longer current.
  • remove merge-risk: 🚨 automation: Current PR review selected no merge-risk labels.

Label justifications:

  • P3: This is a low-risk test portability cleanup with no production runtime change and a remaining proof gap.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing: the PR has a Windows pass claim but no redacted terminal output, logs, screenshot, recording, or linked artifact showing the changed refresh test passing after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Tests +27. Total +27 across 1 file.

View PR surface stats
Area Files Added Removed Net
Source 0 0 0 0
Tests 1 35 8 +27
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 35 8 +27

What I checked:

Likely related people:

  • vincentkoc: GitHub path history shows recent work on src/skills/runtime/refresh.test.ts, and this person also merged the sibling Windows-safe install-path symlink test pattern used as comparison evidence. (role: recent area contributor; confidence: medium; commits: 9bbde7045820, 11a0ad10e91a; files: src/skills/runtime/refresh.test.ts, src/infra/install-safe-path.test.ts)
  • abnershang: GitHub path history points to this author for the trusted workshop symlink target behavior in src/skills/runtime/refresh.ts, which the changed tests exercise. (role: introduced related symlink behavior; confidence: medium; commits: 287b10a89507; files: src/skills/runtime/refresh.ts, src/skills/loading/symlink-targets.ts)
  • aniruddhaadak80: Beyond this PR, this contributor authored the merged sibling Windows symlink test-portability PR that introduced the same capability-probe pattern now used here. (role: adjacent Windows symlink test contributor; confidence: medium; commits: 11a0ad10e91a; files: src/infra/install-safe-path.test.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-06-28T07:23:00.533Z sha 219cbbe :: needs real behavior proof before merge. :: [P2] Use the probed symlink type in gated tests

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 28, 2026
@BradGroux BradGroux mentioned this pull request Jun 28, 2026
Draft
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. label Jul 2, 2026
@aniruddhaadak80

aniruddhaadak80 commented Jul 5, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jul 5, 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.

@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. and removed P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jul 5, 2026
@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

No reviews

Assignees

No one assigned

Labels

P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: XS stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aniruddhaadak80 @Patrick-Erichsen