fix: make the plugin/skill validators work on Windows (6 failing tests -> 0) - #70
Conversation
`pnpm -r test` fails on a clean Windows checkout with six failures, all of them valid files reported as invalid. Three separate causes, none related to the content being validated. 1. Frontmatter delimiters assumed LF. /^---\n([\s\S]*?)\n---\n?/u Git on Windows checks out CRLF by default (core.autocrlf=true) and the repo ships no .gitattributes, so every plugin and skill file arrives as `---\r\n` and the delimiter never matches. Five validators carried the same pattern. 2. Field captures swallowed the carriage return. /^name:\s*([^\n]+)\s*$/mu -> "true\r" instead of "true" so `alwaysApply: true` compared unequal to `true`. 3. Failure messages embedded native path separators. Messages are built from path.join(), so they read `skills\calle` on Windows while tests, CI log greps and docs all expect `skills/calle`. Fixes: accept `\r?\n` in the delimiters, capture `[^\r\n]+` for values, and add a displayPath() helper that renders paths with forward slashes for display only -- filesystem access still uses the native separator. Plus a .gitattributes (`* text=auto eol=lf`) so the CRLF never enters a checkout in the first place. All three are no-ops on POSIX: `\r?` matches nothing against LF, and displayPath() is identity where path.sep is already "/". Adds four regression tests that feed CRLF content directly, so a revert is caught on any platform rather than only on Windows. Before: 6 failing tests on Windows. After: 111 passing, 0 failing.
|
Correcting my own numbers — I re-ran against a pristine clone rather than trusting my working copy, and the count in the description is wrong in a way that understates the problem. What actually happens on a clean Windows checkout: It is 4 failures, not 6, because The practical shape is worse than "some tests fail": a contributor on Windows gets an aborted suite with two thirds of it unexecuted, so the real state of their checkout is invisible to them. After the patch: 111 passing, 0 failing, whole suite runs. Everything else in the description stands — the three causes, the |
|
One follow-up worth naming, because without it this PR fixes the symptom and the cause stays put.
That matters more than it would for an internal tool, because With no Once this lands the suite is 111 passing on Windows, which means the matrix can go in as a required check straight away rather than as a known-broken one: jobs:
test:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest]
runs-on: ${{ matrix.os }}Happy to open that as a small separate PR the moment this one merges — I deliberately have not, because on today's If Windows support is not actually intended, the clean alternative is to say so in metadata: Disclosure: written with AI assistance; the CI, npm and download figures were checked directly and I take responsibility for them. |
Ray-56
left a comment
There was a problem hiding this comment.
Reviewed for repository layout, path handling, supply-chain impact, and cross-platform behavior. The change is limited to repository validation tooling and checkout line-ending policy, does not alter published package contents or runtime behavior, and passes the full local check, test, and pack dry-run suite. Approved.
* fix(core): stop an unreadable token expiry meaning "never expires"
tokenIsUsable treated any expires_at it could not parse as no expiry at all:
const expiresAt = parseIsoDate(cacheDocument.expires_at);
if (!expiresAt) {
return true;
}
parseIsoDate returns null for anything it cannot read, so six different shapes
all resolved to "usable forever": absent, null, "", "not-a-date",
1700000000000, and {}.
The numeric case is the one that makes this more than theoretical. Epoch
milliseconds is a standard way to send an expiry, and broker-client stringifies
whatever the broker sends -- so 1700000000000 arrives here as "1700000000000",
which new Date() reads as Invalid Date. If the broker ever emits a numeric
expiry, the CLI caches that token and never refreshes it again. The failure
surfaces much later as auth errors with no re-login, because the client is
certain the token is fine.
Two changes:
- parseIsoDate accepts epoch seconds and milliseconds as well as ISO strings.
It also matches the sign, so a negative value is rejected rather than handed
to new Date(), which does not fail on it -- V8 reads "-1" as a date and
returns 2001-01-01, a nonsense expiry rather than an error.
- tokenIsUsable distinguishes absent from unreadable. Absent still keeps the
token: the broker never committed to an expiry, and that is the existing
behaviour. Present-but-unreadable now forces a refresh, because one extra
login costs far less than a client that is certain about a token it cannot
reason about.
Worth noting the neighbouring pendingIsExpired assumes the opposite for the same
unparseable input, via Boolean(expiresAt && ...). The two now agree that an
unreadable date is not a reason for confidence.
Tests: 8 cases covering both epoch forms, past and future, unreadable values,
absent values, the minTtlSeconds window, and malformed tokens. core 20/20; type
check clean. The codex-plugin failures in the full suite are pre-existing on
main (they are the CRLF issue in #70) and appear with and without this change.
* chore(core): add the patch changeset for the token expiry fix
Addresses the review on #72. Describes the epoch-expiry parsing and the
unreadable-expiry refresh behaviour, as asked.
Verified: pnpm run check:versions is in sync, pnpm --filter @call-e/core
pack:dry-run builds the tarball, and the core suite is 20/20.
---------
Co-authored-by: EazyHood <rokmc763@hotmail.com>
Co-authored-by: EazyHood <209367218+EazyHood@users.noreply.github.com>
On a clean Windows checkout,
pnpm -r testfails with six failures — all of them valid files reported as invalid, for reasons unrelated to their contents.The file it rejects does start with frontmatter. Three separate causes.
1. Frontmatter delimiters assumed LF
/^---\n([\s\S]*?)\n---\n?/uGit on Windows checks out CRLF by default (
core.autocrlf=true) and the repo ships no.gitattributes, so files arrive as---\r\nand the delimiter never matches. The same pattern appears in five validators:claude-plugin,codex-plugin,cursor-plugin,openclaw-cli-skill,skills-sh-skill.2. Field captures swallowed the carriage return
so
alwaysApply: truecompared unequal totrue.3. Failure messages embedded native path separators
Messages are built from
path.join(), so they readskills\calleon Windows while the tests — and CI log greps, and the docs — expectskills/calle:Fix
\r?\nin the delimiters[^\r\n]+for field valuesdisplayPath(), which renders paths with forward slashes for display only — filesystem access still uses the native separator.gitattributes(* text=auto eol=lf) so CRLF never enters a checkout in the first placeAll three are no-ops on POSIX:
\r?matches nothing against LF, anddisplayPath()is identity wherepath.sepis already/.Tests
Four new regression cases feed CRLF content directly, so a revert is caught on any platform rather than only on Windows.
Why this is worth landing now
The hackathon requires every participant to open a PR against this repo. Any entrant on Windows hits a red test suite on their first
pnpm install && pnpm test, before writing a line of their own code.Disclosure: written with AI assistance. Every failure above was reproduced on my own Windows 11 machine, and I take responsibility for the change.