Skip to content

ci: build and test every PR, including from forks - #164

Closed
obra wants to merge 1 commit into
mainfrom
ci/add-github-actions
Closed

obra wants to merge 1 commit into
mainfrom
ci/add-github-actions

Conversation

@obra

@obra obra commented Sep 8, 2026

Copy link
Copy Markdown
Owner

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:

  1. npm install
  2. npm run build — via prebuild this generates src/version.ts, then runs tsc
    (strict, over src/), then esbuild. This step is the typecheck.
  3. npm testvitest run, 247 tests.

Triggers on pull_request and on push to main.

What it deliberately does not run

  • test:claude-e2e / test:codex-e2e. These need API credentials. Fork PRs get no
    secrets, so these could never be a fair or reliable gate.
  • Lint. There is none to run — no eslint, biome, or prettier config exists in the repo.
    Not adding one here; that's a separate opinionated change that would conflict with every
    open PR.
  • A "dist is up to date" check. Tempting, since dist/ is committed, but see the
    reproducibility caveat below — it would go red from dependency drift alone.

Security posture

  • permissions: contents: read, nothing else.
  • No secrets referenced at all, so a fork run and an internal run do exactly the same thing.
  • Plain pull_request, not pull_request_target — the latter runs untrusted fork code
    with a writable token and secret access, which is a well-known privilege-escalation footgun.
  • No same-repo guard, so fork PRs actually run.

concurrency is keyed on ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
with cancel-in-progress. On a pull_request event that resolves to the PR number, so a
force-push supersedes the run it replaces; on push the pull_request context is absent and
it falls back to github.ref (refs/heads/main). There are no if: conditions anywhere in
the workflow, so there is no expression that can silently evaluate false and skip everything.

fail-fast: false so one Node version going red still reports the other.

Node version decision

The package has no engines field, so the supported range is unstated. I went with
22 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 useful
regression 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, sharp and esbuild
install scripts by default.
But actions/setup-node installs the npm bundled with each
Node release, and the install still succeeded on both legs here (the package's own
scripts/postinstall.js compensates, exactly as it was designed to). So this is real-world
coverage 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:

  1. actions/setup-node's cache: npm requires a lockfile, and this repo gitignores
    package-lock.json. It would just error.
  2. A hand-rolled cache buys nothing measurable here. Cold npm cache: 56.9s. Warm npm
    cache: 58.6s. Install time is dominated by node-gyp compiling better-sqlite3 from
    source, not by downloads.
  3. Installing cold every run keeps the native install/build path — the subject of
    postinstall npm rebuild better-sqlite3 is 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 .gitignore lists package-lock.json, so this
is a deliberate choice rather than an oversight.
Consequences:

  • npm ci is unavailable; npm install is the only option.
  • CI is not reproducible across runs. Every ^ range re-resolves against the registry on
    every run, so a run can go red because a transitive dependency published, with no change to
    this repo.
  • Concretely: npm run build on a clean checkout produces a dist/mcp-server.js that differs
    from the committed one by 239 lines — purely esbuild minifier variable-naming churn (u3 vs
    u) 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.

⚠️ One test is skipped — please review this specifically

main is 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 repair fails,
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)). 1075769 is the last green commit.

Root cause, not a guess: repairIndex() calls summarizeConversation(), which spawns a real
Claude Agent SDK subprocess
. 22a82a5 pointed CLAUDE_CONFIG_DIR at an empty temp dir, so that
subprocess has no usable credentials and returns is_error. repairIndex catches and logs the
error, never re-indexes, and last_indexed never advances:

Failed to re-index .../outdated-repair.jsonl: SummarizerSdkError: Summarizer SDK error: success
    at callClaude (src/summarizer.ts:201:15)
    at summarizeConversation (src/summarizer.ts:537:22)
AssertionError: expected 1788892790102 to be greater than 1788892790102

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. So
I marked it it.skip with a comment recording the above, rather than weakening the assertion or
excluding 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
repairIndex to be drivable with a stub summarizer, or to honor
EPISODIC_MEMORY_SKIP_SUMMARIES (which today only affects sync). Happy to drop the skip if
you'd rather see CI red until that's fixed.

Everything else passes: 247 passed, 1 skipped, 44 files, on both Node versions.

Verification performed

actionlint 1.7.7: clean. Action versions checked against the registry — actions/checkout@v7
and actions/setup-node@v7 are current (v5 is two majors stale).

Every command in the workflow was run locally on a fresh clone of this branch:

Node npm install build test result
22.22.2 12.0.2 50s 2s 8s 247 passed, 1 skipped
24.20.0 11.19.0 16s 2s 8s 247 passed, 1 skipped

The Node 24 run was done with HOME set to an empty temp dir and ANTHROPIC_API_KEY,
CLAUDE_CODE_OAUTH_TOKEN and CLAUDE_CONFIG_DIR unset, to approximate a credential-free
runner. Total expected CI wall time is roughly 1–2 minutes per leg.

A useful incidental finding: a bare tsc --noEmit fails on a fresh checkout
src/version.ts is generated by scripts/generate-version.js and gitignored, so it only exists
after prebuild/pretest. That's why the workflow typechecks via npm run build instead of a
standalone tsc step.

What I could not verify without merging

  • That the workflow actually triggers, and that fork PRs run with a read-only token and no
    secrets. This is standard pull_request behavior and there is no if: gating it, but it
    cannot be observed until the workflow is on the default branch.
  • Concurrency cancellation behavior on a real force-push.
  • Runner-specific install behavior: whether GitHub's ubuntu-latest image gets a
    better-sqlite3 prebuild or compiles from source. Locally Node 24 got a prebuild (16s) and
    Node 22 compiled (50s). Either works; it only affects timing.
  • Anything about macOS or Windows runners. Given Plugin fails to load on Windows: two upstream dep-resolution bugs slip past mcp-server-wrapper.js #95 was a Windows install bug, adding those
    to the matrix later is probably worthwhile — I left them out to keep this first workflow fast
    and green.

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.
@obra
obra force-pushed the ci/add-github-actions branch from 55da31d to 7478e95 Compare September 8, 2026 18:52
@obra

obra commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

Rebased onto current main and dropped the it.skip — this is now workflow-only, one file, 62 lines, no test changes.

Why the skip is gone

The original finding was correct at the time: test/verify.test.ts > repairIndex > re-indexes outdated files during repair failed deterministically, bisected to 22a82a5 (#131), because repairIndex() spawns a real Claude Agent SDK subprocess and #131 pointed CLAUDE_CONFIG_DIR at an empty temp dir.

That is no longer true. The 13-PR batch merge — specifically f06748a, the summarizer error-handling rework — fixed it. Verified on current main:

$ npm test
Test Files  54 passed (54)
     Tests  314 passed (314)

Skipping it now would silently drop a passing test, so it stays.

A caution for anyone verifying this repo: run npm test, not npx vitest run. pretest generates src/version.ts, which is gitignored, so bypassing it produces 13 failed suites all reporting Cannot find module './version.js' — a convincing-looking red that is entirely an artifact of the wrong command. I did exactly that before catching it.

What the workflow does

Triggers on pull_request and push to main. Node 22 and 24 (active LTS), fail-fast: false, timeout-minutes: 20, concurrency keyed on ref with cancel-in-progress. Runs npm installnpm run build (which typechecks) → npm test.

Runs on fork PRs, which is the point — most inbound here is external. Plain pull_request, so fork runs get a read-only token and no secrets. Deliberately not pull_request_target, which executes untrusted fork code with full secrets. No same-repo guard, no if: conditions anywhere, no secrets used at all — so a fork run behaves identically to an internal one.

Decisions worth knowing

Node 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 better-sqlite3, not downloads. setup-node's cache: npm needs a lockfile that doesn't exist, and cold installs keep the native-install path (#100/#102/#105/#125/#135/#162) genuinely under test.

Typecheck via npm run build, not a standalone tsc --noEmit — the latter fails on a fresh checkout for the same generated-version.ts reason.

No lint gate — there is no eslint/biome/prettier config in the repo.

e2e excluded (test:claude-e2e, test:codex-e2e) — they need API credentials and can never run on a fork PR.

Follow-ups, not done here

  • No lockfile, so npm ci is impossible and runs aren't reproducible. .gitignore lists package-lock.json explicitly, so this is a deliberate choice, not an oversight — committing one is a maintainer call with real consequences for the open PRs.
  • npm 12 blocks better-sqlite3 and onnxruntime-node install scripts; indexing silently never works #162 won't be caught by this. Reproduced locally: npm 11.19.0 and 12.0.2 both block those install scripts by default (not just npm 12), but postinstall.js compensates and the install succeeds. Covering it needs a separate non-blocking job pinning a specific npm.
  • npm run build on a clean checkout produces a dist/mcp-server.js differing from the committed one by ~239 lines of esbuild minifier churn, which is why there's no "dist is up to date" check.

Measured timings

Node npm install build test
22.22.2 12.0.2 50s 2s 8s
24.20.0 11.19.0 16s 2s 8s

~1-2 min per leg, so no job splitting needed. The Node 24 run used an empty HOME with all Claude/Anthropic credentials unset, to mimic a runner.

Not verifiable without merging

That the workflow triggers at all; fork-PR token scoping; concurrency cancellation on a real force-push; and whether GitHub's runner image ships a better-sqlite3 prebuild or compiles from source (affects timing only).

@obra

obra commented Sep 8, 2026

Copy link
Copy Markdown
Owner Author

Merged the CI workflow to main as 06d8213. Took .github/workflows/ci.yml as-is; left out this PR's test/verify.test.ts skip because the repair test is already fixed on main by a summarizer mock (from #117) rather than a skip, which keeps its coverage. The full suite is green on main now (314 tests), including the previously-flaky single-instance lock test, which I made deterministic in a follow-up. CI will run build + test on Node 22 and 24 for every PR and push.

— Claude Fable 5.1, Claude Code 2.1.263

obra added a commit that referenced this pull request Sep 8, 2026
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
@obra obra closed this Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant