Repository navigation
fix(tests): make CLI sync tests pass on Windows - #3869
Irish-Joseph wants to merge 2 commits into
Conversation
|
@Irish-Joseph is attempting to deploy a commit to the OpenClaw Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. What this changesThis PR makes CLI sync test roots and path expectations portable on Windows and adds a changelog entry. Example: A Windows contributor runs sync.test.ts with a resolved sentinel root such as D:\scan.
Review scores
ProductKind: Test only · Worth it: Yes · Fix scope: Complete Merge readiness✅ Ready for maintainer review Keep this PR open: current main retains the Windows-incompatible test fixtures, and this focused repair preserves the existing assertions with no blocking findings. Priority: P3 Before mergeNone. FindingsNone. Agent review detailsHow this fits togetherThe CLI sync command resolves scan roots, discovers skill folders, compares registry fingerprints, and plans or publishes changes; this PR changes its mocked test inputs and expectations. flowchart LR
A[CLI options] --> B[Sync command]
B --> C[Resolve scan roots]
C --> D[Find skill folders]
D --> E[Registry comparison]
E --> F[Plan or publish]
G[Portable test fixtures] --> B
Technical reviewBest possible solution: Land the focused test portability repair while preserving platform-native runtime paths and existing sync safety assertions. Do we have a high-confidence way to reproduce the issue? The source shows platform-resolved roots reaching POSIX-only mocks and expectations; the contributor reports 11 Windows failures before and 11 passes after, but this read-only review ran no tests. Is this the best way to solve the issue? Best-fix verdict: best; local fixture normalization and a platform-resolved expectation preserve the tested contract, whereas changing runtime resolution or the shared test kit would expand scope unnecessarily. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 0bf64843c697. Provenance checked
TestingProof path: shipped entry point. SecurityNone. EvidenceWhat I checked:
Likely related people:
Review metrics
LabelsLabel changes: No label changes. Label justifications:
Rating scale6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior. WorkflowClawSweeper edits this one comment on every review. Comment HistoryReview history (59 earlier review cycles; latest 8 shown)
Reviewed October 14, 2026, 11:26 AM ET / 15:26 UTC (Revision 60). |
|
Verification summary for maintainers (run against the current head):
The only red badge is the Vercel preview-deploy status ("Authorization required to deploy"), which needs a team-member authorization click; no test or CI job is failing. Happy to make any further adjustments. |
CI workflow replication (Linux)The queued
Could a maintainer approve the queued workflow runs on this PR (GitHub Actions → |
|
CI replication now covers the remaining two workflow jobs as well, in the same ubuntu:24.04 container against this branch head: |
|
This pull request has been automatically marked as stale due to inactivity. |
What Problem This Solves
Fixes: all 11 tests in
packages/clawhub/src/cli/commands/sync.test.tsfail on Windows ("No skills found (checked configured roots)", wrongfindSkillFoldersarguments), which blocks Windows-based contributors from getting a green CLI test baseline.User Impact
No user-visible change. Contributors on Windows can run the CLI test suite and see the real pass/fail state for the
synccommand tests.Why This Change Was Made
The tests drive
cmdSyncwith POSIX sentinel roots (/scan,/workspace/scan,/fallback/skills) andfindSkillFoldersstubs that recognize them by exact string match. On Windowsresolve()renders those paths as drive-prefixed backslash paths, so the stubs never matched. Test-only fixes; implementation code untouched, same behaviors verified:scanafter separator normalization;resolve("/workspace", "scan")— the same platform-rendered absolute path the implementation produces — computed inside theprocess.cwd()spy window because on Windowspath.resolveborrows the drive letter from cwd;Evidence
node dist/cli.js sync --all --dry-runagainst a local registry emulator scans two real skill folders and reports the platform-rendered roots —Roots with skills: D:\sync-evidence\skills→Dry run: would publish 2 skill(s);--jsonmode and the registry request log included in<details>below.main):sync.test.ts→ 0 passed / 11 failed. After: Test Files 1 passed (1) · Tests 11 passed (11). Fullpackages/clawhubsource suite: 98 failed / 363 passed vs 109 failed / 352 passed onmain— the remaining failures are pre-existing and unrelated (realnpm/bun spawns or other POSIX fixtures; they pass where the binaries are on PATH, as in the replicated Linux run).bun run ci:staticpasses.tsc -p packages/clawhub --noEmit,oxlint --type-aware,oxfmt --checkclean.CIworkflow):static,packages,types-build,e2e-httpall pass;unit(7,311 tests) has only 4 pre-existing timing flakes that fail identically on unmodifiedmainin the same environment. (The PR's queued GitHubCIrun is awaiting first-time-contributor approval.)resolve("/workspace", "scan")is exactly/workspace/scanon POSIX, and the suffix matches still hold.Full built-CLI transcript (Windows, scratch drive, local registry emulator)
Two real skill folders (
D:\sync-evidence\skills\alpha-tool,D:\sync-evidence\skills\demo-skill, each aSKILL.md), single-port registry emulator, Node runtime (realfetchtransport). All paths are scratch paths, so no redaction was needed.Terminal output (human mode):
--jsonmode:{ "ok": true, "dryRun": true, "registry": "http://127.0.0.1:38690", "roots": ["D:\sync-evidence", "D:\sync-evidence\skills"], "summary": { "wouldPublish": 2, "published": 0, "submitted": 0, "alreadySynced": 0, "skipped": 0, "failed": 0 }, "wouldPublish": [ { "slug": "alpha-tool", "status": "new", "version": "1.0.0", "latestVersion": null, "fileCount": 1, "fingerprint": "282346601a53af…" }, { "slug": "demo-skill", "status": "new", "version": "1.0.0", "latestVersion": null, "fileCount": 1, "fingerprint": "be5abf52572989…" } ] }Registry-side request log (real registry resolution path):