Skip to content

fix(tests): make CLI sync tests pass on Windows - #3869

Open
Irish-Joseph wants to merge 2 commits into
openclaw:mainfrom
Irish-Joseph:fix/sync-windows-tests
Open

Irish-Joseph wants to merge 2 commits into
openclaw:mainfrom
Irish-Joseph:fix/sync-windows-tests

Conversation

@Irish-Joseph

@Irish-Joseph Irish-Joseph commented Sep 30, 2026 •

Copy link
Copy Markdown

What Problem This Solves

Fixes: all 11 tests in packages/clawhub/src/cli/commands/sync.test.ts fail on Windows ("No skills found (checked configured roots)", wrong findSkillFolders arguments), 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 sync command tests.

Why This Change Was Made

The tests drive cmdSync with POSIX sentinel roots (/scan, /workspace/scan, /fallback/skills) and findSkillFolders stubs that recognize them by exact string match. On Windows resolve() renders those paths as drive-prefixed backslash paths, so the stubs never matched. Test-only fixes; implementation code untouched, same behaviors verified:

  • the shared stub now matches roots ending in scan after separator normalization;
  • the relative-roots assertion compares against resolve("/workspace", "scan") — the same platform-rendered absolute path the implementation produces — computed inside the process.cwd() spy window because on Windows path.resolve borrows the drive letter from cwd;
  • the fallback-roots stub matches by separator-normalized suffix (exact equality with a POSIX literal could never hold on Windows).

Evidence

  • Real behavior (built CLI on Windows): node dist/cli.js sync --all --dry-run against 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); --json mode and the registry request log included in <details> below.
  • Unit suite (Windows host, Bun 1.4.2 / Node 22): before (unmodified main): sync.test.ts → 0 passed / 11 failed. After: Test Files 1 passed (1) · Tests 11 passed (11). Full packages/clawhub source suite: 98 failed / 363 passed vs 109 failed / 352 passed on main — the remaining failures are pre-existing and unrelated (real npm/bun spawns or other POSIX fixtures; they pass where the binaries are on PATH, as in the replicated Linux run).
  • CI: bun run ci:static passes. tsc -p packages/clawhub --noEmit, oxlint --type-aware, oxfmt --check clean.
  • CI workflow replication (Linux, ubuntu:24.04 + Bun 1.4.2 + Node 24, matching the CI workflow): static, packages, types-build, e2e-http all pass; unit (7,311 tests) has only 4 pre-existing timing flakes that fail identically on unmodified main in the same environment. (The PR's queued GitHub CI run is awaiting first-time-contributor approval.)
  • Linux behavior is unchanged by construction: separator normalization is a no-op on POSIX paths, resolve("/workspace", "scan") is exactly /workspace/scan on 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 a SKILL.md), single-port registry emulator, Node runtime (real fetch transport). All paths are scratch paths, so no redaction was needed.

Terminal output (human mode):

┌  ClawHub sync
Roots with skills: D:\sync-evidence\skills

To sync
- alpha-tool  NEW (publish 1.0.0)  (1 files)
- demo-skill  NEW (publish 1.0.0)  (1 files)
│
└  Dry run: would publish 2 skill(s).

--json mode:

{
  "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):

[registry] GET /api/v1/resolve slug=alpha-tool
[registry] GET /api/v1/resolve slug=demo-skill

@vercel

vercel Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

@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.

@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 30, 2026
@clawsweeper

clawsweeper Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This 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.

  • Before: The /scan fixture does not match the backslash path, causing false test failures.
  • After: Separator normalization recognizes the fixture; the contributor reports all 11 tests passing.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, correct test repair with Windows results and shipped-CLI output; the CLI transcript supports the path contract while the suite results demonstrate the changed fixtures.
Proof confidence 🐚 platinum hermit (4/6) Sufficient (terminal): For this test-only repair, the supplied Windows before/after suite results exercise the changed fixtures, supplemented by built-CLI human and JSON dry runs discovering two real skill folders through real registry transport.
Patch quality 🦞 diamond lobster (5/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: Windows contributors see false failures in all 11 sync tests.
Reason: The bounded repair restores useful regression coverage without changing designed sync behavior.

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
Reviewed head: 55cdcc1edde3b7d3be3762317ede5849a4ac08ad

Before merge

None.

Findings

None.

Agent review details

How this fits together

The 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
Loading

Technical review

Best 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

  • Sync sentinel roots, relative-root assertion, and fallback-root fixture keeps the original intent (bbe887f: feat(cli): restore skill sync command #2786 restored bulk publishing with owner-scoped planning, relative source provenance, and unattended fallback-root safeguards.)

Testing

Proof path: shipped entry point.

Security

None.

Evidence

What I checked:

  • Verified introduced scope: The pinned introduction changes only sync.test.ts (+15/-4) and CHANGELOG.md (+1); no production implementation changes. (packages/clawhub/src/cli/commands/sync.test.ts:24, 55cdcc1edde3)
  • Current main still needs the repair: Main still matches /scan without separator normalization, asserts the literal /workspace/scan, and compares fallback roots against exact POSIX literals. (packages/clawhub/src/cli/commands/sync.test.ts:22, 0bf64843c697)
  • Root-resolution contract: buildScanRoots and dedupeRoots use platform-native resolve(); normalization belongs in the test fixtures rather than changing runtime paths. (packages/clawhub/src/cli/commands/syncHelpers.ts:14, 55cdcc1edde3)
  • Intent and provenance: Blame attributes the affected fixture and assertion lines to the restored sync implementation in feat(cli): restore skill sync command #2786; its stated contract includes source provenance and refusing unattended fallback-root publishing, both preserved here. (packages/clawhub/src/cli/commands/sync.test.ts:289, bbe887faacd7)
  • Contributor evidence inspected: Irish-Joseph supplies Windows before/after results of 0/11 versus 11/11 and a built-CLI transcript scanning alpha-tool and demo-skill under D:\sync-evidence\skills through real fetch transport to a loopback registry emulator. (55cdcc1edde3)
  • Checks distinguish validation from authorization: The supplied check summary and inspected head checks do not establish completed CI test gates; Vercel explicitly reports deployment authorization required, and a cancelled auto-response check provides no diff-caused failure. (55cdcc1edde3)

Likely related people:

  • Patrick Erichsen: Raw commit bbe887f adds packages/clawhub/src/cli/commands/sync.test.ts:21 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: bbe887faacd7; files: packages/clawhub/src/cli/commands/sync.test.ts)
  • Vincent Koc: Raw commit a63ab20 adds packages/clawhub/src/cli/commands/sync.test.ts:383 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: a63ab20752e1; files: packages/clawhub/src/cli/commands/sync.test.ts)

Review metrics

Metric Value Why it matters
Change scope production +0/-0; tests +15/-4; changelog +1/-0 The added test code handles platform path rendering without expanding runtime behavior.

Labels

Label changes:

No label changes.

Label justifications:

  • P3: This repairs contributor test ergonomics without changing shipped user behavior.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • proof: sufficient: Contributor real behavior proof is sufficient.

Rating scale

6/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.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (59 earlier review cycles; latest 8 shown)
  • reviewed 2026-10-13T18:53:56.216Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-13T23:33:02.170Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T01:52:46.643Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T04:29:15.107Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T07:52:32.179Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T09:18:25.801Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T10:23:00.111Z sha 55cdcc1 :: needs maintainer review before merge. :: none
  • reviewed 2026-10-14T13:35:53.317Z sha 55cdcc1 :: needs maintainer review before merge. :: none

Reviewed October 14, 2026, 11:26 AM ET / 15:26 UTC (Revision 60).

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. 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 merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Oct 1, 2026
@Irish-Joseph

Copy link
Copy Markdown
Author

Verification summary for maintainers (run against the current head):

  • sync.test.ts: 11/11 pass on Windows (0/11 on unmodified main here).
  • bun run ci:static passes; tsc --noEmit, oxlint --type-aware, oxfmt --check clean.
  • Full root suite (7,207 tests): no new failures versus unmodified main on this host (the one extra failure observed in a single loaded run was in convex/lib/openClawPublishAuthorization.test.ts — untouched by this PR, flaky under load, and passing 36/36 in isolation on both branch and main).
  • Test-only change: sentinel-root matching normalized for separators, relative-root expectation computed with the platform's resolve() inside the cwd-spy window, fallback-root matching by normalized suffix.

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.

@Irish-Joseph

Copy link
Copy Markdown
Author

CI workflow replication (Linux)

The queued CI / CodeQL Light / Security Gate / Deploy Test runs on this PR are awaiting first-time-contributor approval. To de-risk approval, I replicated the CI workflow jobs in a clean ubuntu:24.04 container (Bun 1.4.2, Node 24.21, matching the workflow's toolchain), running the exact workflow commands (bun run ci:static, ci:unit, ci:packages, ci:types-build, ci:e2e-http) against this branch's head:

  • static, packages, types-build, e2e-http: pass.
  • unit (7,311 tests): only 4 failures in this run, all timing-sensitive tests unrelated to this diff (convex/githubImport.test.ts gzip-byte race, scripts/ui-proof-backend.test.mjs process-tree reap, scripts/clawsweeper-dispatch-workflow.test.ts, packages/clawhub-admin/.../searchInsights.test.ts spawn timeout) — the identical 4 tests fail on unmodified main in the same environment, i.e. pre-existing under-provisioned-runner flakes (the container has 7 GB RAM vs the 16-vCI runner). No new failures versus main; this branch's regression tests pass.

Could a maintainer approve the queued workflow runs on this PR (GitHub Actions → Approve and run)? Everything above should reproduce green on the provisioned runner.

@Irish-Joseph

Copy link
Copy Markdown
Author

CI replication now covers the remaining two workflow jobs as well, in the same ubuntu:24.04 container against this branch head: claws-openclaw-contract — 9/9 pass (including the real package dry-run against the pinned OpenClaw source); playwright-smoke — 18/18 pass (production build + live-site browser smoke). All seven CI jobs are now verified green (unit flakes identical to unmodified main). Still just waiting on the first-time-contributor "Approve and run" for the queued runs.

@clawsweeper clawsweeper Bot added P3 Low-priority cleanup, docs, polish, ergonomics, or speculative work. and removed P2 Normal backlog priority with limited blast radius. labels Oct 5, 2026
@clawsweeper clawsweeper Bot added rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. labels Oct 7, 2026
@github-actions

Copy link
Copy Markdown
Contributor

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

@github-actions github-actions Bot added the stale label Oct 22, 2026

This branch has not been deployed

No deployments
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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. stale status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant