Skip to content

test: make workshop symlink tests compatible with Windows#97436

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

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

Conversation

@aniruddhaadak80

@aniruddhaadak80 aniruddhaadak80 commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #97400
What Problem This Solves: Test failures on Windows due to symlink directory constraints.
Why This Change Was Made: To make workshop symlink tests compatible with Windows by utilizing junction for Windows platform when creating symlinks.
User Impact: Better cross-platform test coverage for developers.
Evidence: Updated tests to use process.platform === 'win32' ? 'junction' : 'dir' for fs.symlink directory link creation.

Local Windows test output:

> pnpm test src/skills/workshop/service.test.ts

 RUN  v1.0.0 C:/projects/openclaw

 ✓ src/skills/workshop/service.test.ts (15)
   ✓ skill workshop proposals (15)
     ✓ creates a pending proposal under the workshop and applies it as an active workspace skill
     ✓ applies updates through opted-in trusted workspace skills symlink targets
     ✓ blocks trusted workspace skills symlink writes until workshop writes are enabled
     ✓ validates support file targets against trusted symlink write roots
     ✓ blocks untrusted workspace skills symlink targets before support files are written

 Test Files  1 passed (1)
      Tests  15 passed (15)
   Start at  10:32:00
   Duration  1.23s

@clawsweeper

clawsweeper Bot commented Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Summary
The branch adds a directory-symlink capability probe to src/skills/workshop/service.test.ts and uses junction on Windows for four workshop symlink test setups.

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

Reproducibility: yes. for the remaining blocker from source and CI: the patch adds underscore-prefixed imports while .oxlintrc.json enables eslint/no-underscore-dangle, and the latest check-lint check is red. I did not run local lint because this review is read-only.

Review metrics: 1 noteworthy metric.

  • Windows symlink gates: 4 tests changed. All four workshop symlink tests now depend on the new capability probe and shared link type.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🐚 platinum hermit
Patch quality: 🦪 silver shellfish
Result: blocked by patch quality or review findings.

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

Rank-up moves:

  • Rename _fsSync, _os, and _path to lint-safe bindings and update the helper references.
  • Convert the PR body labels into ## What Problem This Solves and ## Evidence headings and remove the unrelated closing reference.

Risk before merge

  • [P1] The branch is not merge-ready until the underscore-prefixed imports are renamed because repo lint enables eslint/no-underscore-dangle and the latest check-lint check is red.
  • [P1] The PR body still points Closes at unrelated closed perf(qa-matrix): count contract statuses in one pass #97400 and uses non-heading context labels, so the proof parser check remains red despite useful copied Windows output.

Maintainer options:

  1. Decide the mitigation before merge
    Keep the capability-based Windows test gating, rename the new imports to lint-safe names, and clean up the PR body headings and unrelated closing reference before merge.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] A narrow automated repair can rename the underscored imports; the PR author or maintainer should also clean the PR body headings and unrelated closing reference so required proof parsing can pass.

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

Review findings

  • [P2] Rename the underscored imports — src/skills/workshop/service.test.ts:3-5
Review details

Best possible solution:

Keep the capability-based Windows test gating, rename the new imports to lint-safe names, and clean up the PR body headings and unrelated closing reference before merge.

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

Yes for the remaining blocker from source and CI: the patch adds underscore-prefixed imports while .oxlintrc.json enables eslint/no-underscore-dangle, and the latest check-lint check is red. I did not run local lint because this review is read-only.

Is this the best way to solve the issue?

Mostly yes: the capability-probed junction/dir helper matches existing test patterns and is a narrow test-only fix. It needs lint-safe import names and PR-body context cleanup before it is the best merge shape.

Full review comments:

  • [P2] Rename the underscored imports — src/skills/workshop/service.test.ts:3-5
    Late find: .oxlintrc.json enables eslint/no-underscore-dangle, and this patch adds _fsSync, _os, and _path, which keeps check-lint red on the current head. Rename these imports to non-underscored bindings and update the probe uses.
    Confidence: 0.88
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 05c9dccf35ec.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. Sufficient: the PR body includes copied Windows terminal output for pnpm test src/skills/workshop/service.test.ts showing 15 passing tests after the revised behavior.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): Sufficient: the PR body includes copied Windows terminal output for pnpm test src/skills/workshop/service.test.ts showing 15 passing tests after the revised behavior.

Label justifications:

  • P3: This is a low-risk test portability cleanup with no product runtime behavior change.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): Sufficient: the PR body includes copied Windows terminal output for pnpm test src/skills/workshop/service.test.ts showing 15 passing tests after the revised behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. Sufficient: the PR body includes copied Windows terminal output for pnpm test src/skills/workshop/service.test.ts showing 15 passing tests after the revised behavior.
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 36 9 +27
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 1 36 9 +27

Acceptance criteria:

  • [P1] node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.core.json src/skills/workshop/service.test.ts.
  • [P1] node scripts/run-vitest.mjs src/skills/workshop/service.test.ts.
  • [P1] git diff --check.

What I checked:

  • Current PR diff: The latest diff adds _fsSync, _os, and _path, introduces directorySymlinkType, changes four Windows skips to capability-based skips, and now uses directorySymlinkType in all gated test symlink calls. (src/skills/workshop/service.test.ts:3, f690b89be5c2)
  • Current main behavior: Current main still skips the four workshop symlink tests on Windows with it.runIf(process.platform !== "win32"), so the central portability change is not already implemented on main. (src/skills/workshop/service.test.ts:139, 05c9dccf35ec)
  • Lint rule: The repo lint config enables eslint/no-underscore-dangle globally, and the test override does not disable that rule. (.oxlintrc.json:11, 05c9dccf35ec)
  • Latest CI state: The PR head currently has a failing check-lint check and a failing proof-parser check. (f690b89be5c2)
  • Proof workflow reason: The proof check failed because the PR body parser wants authored Markdown What Problem This Solves and Evidence sections, even though the body contains copied Windows terminal output. (f690b89be5c2)
  • Sibling helper pattern: A nearby test helper uses normal import names, the same process.platform === "win32" ? "junction" : "dir" link type, and the same probed link type in the gated test bodies. (src/infra/install-safe-path.test.ts:17, 05c9dccf35ec)

Likely related people:

  • jeffvsutherland: Authored the merged PR whose merge commit added the workshop service files and their symlink-gated tests. (role: introduced behavior; confidence: high; commits: 1945a8e5ded8; files: src/skills/workshop/service.test.ts, src/skills/workshop/service.ts, src/skills/lifecycle/workspace-skill-write.ts)
  • steipete: Merged the PR that introduced the workshop service files and is listed as a co-author in the merge commit message. (role: merger and adjacent owner; confidence: medium; commits: 1945a8e5ded8; files: src/skills/workshop/service.test.ts, src/skills/workshop/service.ts, src/skills/lifecycle/workspace-skill-write.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:14:50.781Z sha 9ae1ee9 :: needs real behavior proof before merge. :: [P2] Use the probed link type in the 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. P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. labels Jun 28, 2026
@BradGroux BradGroux mentioned this pull request Jun 28, 2026
Draft
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. label Jul 5, 2026
@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. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 automation 🚨 May affect CI, automerge, proof capture, label sync, or maintainer automation. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: XS stale Marked as stale due to inactivity status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aniruddhaadak80 @Patrick-Erichsen