Skip to content

test: make marketplace symlink tests compatible with Windows#97439

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

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

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

��Closes #95534

What Problem This Solves

Fixes an issue where several marketplace plugin symlink boundary tests were unconditionally skipped on Windows environments, even if the environment supports creating directory symlinks (like with Developer Mode enabled).

Why This Change Was Made

Replaces the hardcoded Windows skips with a dynamic capability check (canCreateDirectorySymlinks) and explicitly passes directorySymlinkType to s.symlink when creating the test fixture, so that it creates a junction on Windows and matches the canCreateDirectorySymlinks probe logic.

User Impact

No user-visible product impact. This improves test portability and preserves regression coverage on capable Windows environments.

Evidence

  • Fixed the mockRemoteMarketplaceCloneWithOutsideSymlink test fixture to use directorySymlinkType matching the probe.
  • Ran
    ode scripts/run-vitest.mjs src/plugins/marketplace.test.ts locally and it exited successfully without errors.
  • Pushed the commit so that CI will provide remote test evidence as required.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026

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 two Windows-only marketplace symlink test skips with a directory-symlink capability probe and uses the same platform-specific symlink type in the fixture.

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

Reproducibility: yes. Current main shows the two marketplace symlink tests are skipped on every Windows host via it.runIf(process.platform !== "win32"); I verified this from source rather than a Windows run.

Review metrics: none identified.

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, copied live output, or a linked CI artifact showing node scripts/run-vitest.mjs src/plugins/marketplace.test.ts after the fixture update.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR body asserts a local test run but includes no redacted terminal output, copied live output, or artifact for the Windows after-fix path. 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] No redacted Windows after-fix output or artifact is posted yet, so the Windows-specific test-portability claim remains unproven by real behavior proof.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the test-local capability probe and typed symlink fixture, then require a PR-body evidence update with redacted Windows run output before merge.
  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 needed; the remaining blocker is contributor-supplied Windows/test proof and ordinary merge gating.

Security
Cleared: Cleared: the diff only changes a marketplace test file and does not touch dependencies, workflows, secrets, or runtime security boundaries.

Review details

Best possible solution:

Keep the test-local capability probe and typed symlink fixture, then require a PR-body evidence update with redacted Windows run output before merge.

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

Yes. Current main shows the two marketplace symlink tests are skipped on every Windows host via it.runIf(process.platform !== "win32"); I verified this from source rather than a Windows run.

Is this the best way to solve the issue?

Yes, for the code shape. After the second commit, the fixture uses the same platform-specific directory link type as the probe and matches existing sibling test patterns; the remaining gap is 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 justifications:

  • P3: This is a low-risk test-portability cleanup with no user-visible runtime behavior change.
  • 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 body asserts a local test run but includes no redacted terminal output, copied live output, or artifact for the Windows after-fix path. 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 30 3 +27
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 30 3 +27

What I checked:

Likely related people:

  • Agustin Rivera: Authored the commit that added remote marketplace canonicalization and the symlink escape tests this PR changes. (role: remote marketplace path hardening author; confidence: high; commits: b1dd3ded3589; files: src/plugins/marketplace.ts, src/plugins/marketplace.test.ts)
  • vincentkoc: Authored and merged the PR that introduced the marketplace registry install surface and its initial tests. (role: original marketplace feature author and merger; confidence: medium; commits: ff2e864c98cb; files: src/plugins/marketplace.ts, src/plugins/marketplace.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:20:57.623Z sha 86678f0 :: needs real behavior proof before merge. :: [P2] Use the probed link type in the fixture

@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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. labels Jun 28, 2026
@clawsweeper clawsweeper Bot removed 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. labels Jul 5, 2026
@openclaw-barnacle

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

Labels

P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. size: XS stale Marked as stale due to inactivity

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants