Conversation
There is no CI on this repo, so 11 open PRs and 37 open issues carry no automated signal at all. Add a GitHub Actions workflow that installs, builds (which typechecks) and runs the vitest suite on Node 22 and 24. Uses no secrets and requests only contents:read, so a fork PR run is identical to a run on main. Triggers on plain pull_request rather than pull_request_target, so fork code never executes with write scope. Skips one test that cannot pass without API credentials: repairIndex() shells out to the real Claude Agent SDK summarizer, which has had no usable credentials in tests since 22a82a5 isolated CLAUDE_CONFIG_DIR (#131). The remaining 247 tests pass on both Node versions.
55da31d to
7478e95
Compare
|
Rebased onto current Why the skip is goneThe original finding was correct at the time: That is no longer true. The 13-PR batch merge — specifically f06748a, the summarizer error-handling rework — fixed it. Verified on current main: Skipping it now would silently drop a passing test, so it stays. A caution for anyone verifying this repo: run What the workflow doesTriggers on Runs on fork PRs, which is the point — most inbound here is external. Plain Decisions worth knowingNode 25 excluded on purpose. #100 is a known Node 25 postinstall bug, so that leg would be red on arrival — the exact failure mode this is meant to fix. No dependency caching. Measured rather than assumed: cold cache 56.9s vs warm 58.6s. The cost is node-gyp compiling Typecheck via No lint gate — there is no eslint/biome/prettier config in the repo. e2e excluded ( Follow-ups, not done here
Measured timings
~1-2 min per leg, so no job splitting needed. The Node 24 run used an empty Not verifiable without mergingThat the workflow triggers at all; fork-PR token scoping; concurrency cancellation on a real force-push; and whether GitHub's runner image ships a |
|
Merged the CI workflow to main as — Claude Fable 5.1, Claude Code 2.1.263 |
Read-only, secret-free GitHub Actions workflow so fork PRs get the same build+test signal as main. Merged the workflow only; #164's test/verify.test.ts skip is superseded by the summarizer mock already on main (#117), which keeps the repair test running. Co-authored-by: obra <obra@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_0112vdwZphiWzfCYfaXMes4C
Why
There is no CI on this repo. With 11 open PRs and 37 open issues, every one of them
currently carries zero automated signal — a reviewer has to check out each branch and
build it by hand to learn anything. This adds one workflow so that stops being true.
Almost all open PRs are from outside contributors, so the design constraint that mattered
most was it has to work on fork PRs.
What it runs
One job, matrixed over Node 22 and 24, on
ubuntu-latest:npm installnpm run build— viaprebuildthis generatessrc/version.ts, then runstsc(strict, over
src/), then esbuild. This step is the typecheck.npm test—vitest run, 247 tests.Triggers on
pull_requestand onpushtomain.What it deliberately does not run
test:claude-e2e/test:codex-e2e. These need API credentials. Fork PRs get nosecrets, so these could never be a fair or reliable gate.
Not adding one here; that's a separate opinionated change that would conflict with every
open PR.
dist/is committed, but see thereproducibility caveat below — it would go red from dependency drift alone.
Security posture
permissions: contents: read, nothing else.pull_request, notpull_request_target— the latter runs untrusted fork codewith a writable token and secret access, which is a well-known privilege-escalation footgun.
concurrencyis keyed on${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}with
cancel-in-progress. On apull_requestevent that resolves to the PR number, so aforce-push supersedes the run it replaces; on
pushthepull_requestcontext is absent andit falls back to
github.ref(refs/heads/main). There are noif:conditions anywhere inthe workflow, so there is no expression that can silently evaluate false and skip everything.
fail-fast: falseso one Node version going red still reports the other.Node version decision
The package has no
enginesfield, so the supported range is unstated. I went with22 and 24 — the two active LTS lines. Node 20 is EOL.
I did not add Node 25, even though #100 is specifically a Node 25 bug, because a matrix
leg that is known-red on arrival trains everyone to ignore the whole workflow. Once #100 is
fixed, adding
'25'to the matrix is a one-line change and would be a genuinely usefulregression guard.
Worth flagging: #162 will not be caught by this workflow. I reproduced it locally — npm
11.19.0 and 12.0.2 both block
better-sqlite3,onnxruntime-node,sharpandesbuildinstall scripts by default. But
actions/setup-nodeinstalls the npm bundled with eachNode release, and the install still succeeded on both legs here (the package's own
scripts/postinstall.jscompensates, exactly as it was designed to). So this is real-worldcoverage of the happy path, not of #162. Pinning a specific npm to reproduce it would be a
good follow-up, as a separate non-blocking job.
Caching: I skipped it, and measured why
I did not cache
~/.npm. Three reasons:actions/setup-node'scache: npmrequires a lockfile, and this repo gitignorespackage-lock.json. It would just error.cache: 58.6s. Install time is dominated by node-gyp compiling
better-sqlite3fromsource, not by downloads.
postinstall
npm rebuild better-sqlite3is a silent no-op on Node 25 — binding never builds, recovery hint repeats the dead command #100, Plugin fails to load when host has libvips installed globally: sharp@0.34.5 postinstall picks source build, fails, leaves corrupt node_modules that traps wrapper recovery loop #102, v1.4.2: onnxruntime-common not hoisted on Linux/WSL2 — clean-install repro of #95 Bug 2 (conflicting versions confirmed) #105, v1.4.2 completely broken on Intel Macs: onnxruntime-node@1.24.3 ships no darwin/x64 binary — SessionStart hook error + sync/search dead (MODULE_NOT_FOUND) #125, sharp/libvips dlopen fails (host without system libvips): @img hoisting split crashes SessionStart sync #135, npm 12 blocks better-sqlite3 and onnxruntime-node install scripts; indexing silently never works #162 — genuinely under test rather than cached past.At ~50s, install simply isn't worth optimizing yet.
Reproducibility caveat (no lockfile) — follow-up worth doing
No lockfile of any kind is committed, and
.gitignorelistspackage-lock.json, so thisis a deliberate choice rather than an oversight. Consequences:
npm ciis unavailable;npm installis the only option.^range re-resolves against the registry onevery run, so a run can go red because a transitive dependency published, with no change to
this repo.
npm run buildon a clean checkout produces adist/mcp-server.jsthat differsfrom the committed one by 239 lines — purely esbuild minifier variable-naming churn (
u3vsu) from a newer esbuild resolving under^0.25.11.Committing a lockfile would fix all of this, but I have deliberately not done it here: it's
a maintainer call, and it would conflict with essentially every open PR at once. Flagging it as
the highest-value follow-up.
mainis not green today. Before writing any YAML I ran the suite on a clean clone;test/verify.test.ts > repairIndex > re-indexes outdated files during repairfails,deterministically (5/5 runs), on both Node 22 and Node 24.
I bisected it. It has been failing since 22a82a5 (
fix(test): isolate vitest from the real ~/.config/superpowers (#131)).1075769is the last green commit.Root cause, not a guess:
repairIndex()callssummarizeConversation(), which spawns a realClaude Agent SDK subprocess. 22a82a5 pointed
CLAUDE_CONFIG_DIRat an empty temp dir, so thatsubprocess has no usable credentials and returns
is_error.repairIndexcatches and logs theerror, never re-indexes, and
last_indexednever advances:That makes it, in substance, an e2e test requiring API credentials that happens to live in the
unit suite — the same category as
test:claude-e2e, which this workflow already excludes. SoI marked it
it.skipwith a comment recording the above, rather than weakening the assertion orexcluding the whole file (which would have thrown away 7 good tests alongside it).
This is the one judgment call in the PR and it is easy to reverse. The real fix is for
repairIndexto be drivable with a stub summarizer, or to honorEPISODIC_MEMORY_SKIP_SUMMARIES(which today only affectssync). Happy to drop the skip ifyou'd rather see CI red until that's fixed.
Everything else passes: 247 passed, 1 skipped, 44 files, on both Node versions.
Verification performed
actionlint1.7.7: clean. Action versions checked against the registry —actions/checkout@v7and
actions/setup-node@v7are current (v5 is two majors stale).Every command in the workflow was run locally on a fresh clone of this branch:
The Node 24 run was done with
HOMEset to an empty temp dir andANTHROPIC_API_KEY,CLAUDE_CODE_OAUTH_TOKENandCLAUDE_CONFIG_DIRunset, to approximate a credential-freerunner. Total expected CI wall time is roughly 1–2 minutes per leg.
A useful incidental finding: a bare
tsc --noEmitfails on a fresh checkout —src/version.tsis generated byscripts/generate-version.jsand gitignored, so it only existsafter
prebuild/pretest. That's why the workflow typechecks vianpm run buildinstead of astandalone
tscstep.What I could not verify without merging
secrets. This is standard
pull_requestbehavior and there is noif:gating it, but itcannot be observed until the workflow is on the default branch.
ubuntu-latestimage gets abetter-sqlite3prebuild or compiles from source. Locally Node 24 got a prebuild (16s) andNode 22 compiled (50s). Either works; it only affects timing.
to the matrix later is probably worthwhile — I left them out to keep this first workflow fast
and green.