fix(ci): make the test suite run truthfully and let CI report on main

[yaron-thenvoi]

Closes INT-1312.

main carried a permanently failing test, and nothing required CI to pass before merging — which is how it landed there. This makes the suite run truthfully on every platform and makes CI report on the trunk, then documents the one piece that is external GitHub state.

⚠️ Action required by an admin — merging this PR does not enforce it

Verified against the live API on 2026-08-31 (read-only; nothing was changed):

$ gh api repos/band-ai/band-sdk-typescript/rulesets/14198025 \
    --jq '{rules: [.rules[].type], bypass: .bypass_actors}'

name:          main-branch-protection
enforcement:   active
applies to:    refs/heads/main
bypass_actors: none
rules:         deletion, non_fast_forward, pull_request
required_status_checks: ABSENT

Review is already enforced — the pull_request rule sets required_approving_review_count: 1, dismiss_stale_reviews_on_push: true, required_review_thread_resolution: true. The missing control is orthogonal: a PR and an approval are mandatory; a passing one is not. An approver can merge a PR whose CI is red. That is exactly what happened on #159 — it merged with both test and the aggregate ci-status concluding failure, and because CI never re-ran on main, the failure was invisible from that moment until a manual review found it.

An administrator needs to add one rule to ruleset 14198025 (Settings → Rules → main-branch-protection):

• Rule: required_status_checks
• Required check: ci-status — and only that one (integration: GitHub Actions)
• Strict: ✅ enabled — "Require branches to be up to date before merging"

No workflow change is needed for either. The ci-status gate job already exists, already depends on every other job, and a test asserts it keeps doing so. docs/ci-cd-workflows.md records both settings and the reasoning behind them.

Why ci-status and only ci-status

GitHub reports a job skipped by a job-level condition as successful. lint and test are intentionally conditional; changes and packaging are mandatory. ci-status encodes that distinction, always runs, and exposes one stable context — so adding or renaming a CI job never requires a ruleset edit.

Why strict, and what it costs

Strict targets semantic conflicts: two PRs that each pass alone and break combined, with no textual conflict for git to catch. Not hypothetical here — the C3–C7 guards assert over global state (export-surface snapshots, whole-tree text scans, byte-identical doc regeneration), so one PR tightening a guard while another adds text the old guard allowed is green twice and red on the trunk.

Four things made the case:

1. CI takes ~2 minutes (measured across the last 25 runs), so bringing a branch up to date is one button and a short wait.
2. main is the release branch, not just the trunk — Release Please cuts releases from it, so a bad interaction between two PRs is a publish candidate, not merely a broken trunk.
3. There is no integration branch. dev is a legacy compatibility lane, not a buffer, so nothing catches a bad interaction before main.
4. Long-lived PRs are the real risk — several open PRs are months behind main. A stale branch merging on a long-outdated green run is the riskiest merge this repo makes.

Costs, accepted: there is no merge queue, so on a busy day a branch can go stale between updating and merging and the update is retried; and Dependabot branches go stale on every merge, though Dependabot rebases them itself. docs/ci-cd-workflows.md records what adding a queue would require if that race ever becomes a real cost.

What changed

1. Compile proofs did not run off POSIX — and half of them could not fail

First, what C3/C4/C5 mean (they are defined nowhere but the filenames)

C1–C7 are the seven slices of the Thenvoi→Band rename, shipped as #150 (3173431). The labels live on only in test filenames, so for reference:

Slice	Renamed	Proof test
C1	Release hold, package-content assertions, pnpm overrides	(release-hardening)
C2	@thenvoi/rest-client → @band-ai/rest-client, ThenvoiClient → BandClient	band-client-conformance
C3	Linear public types/config — LinearThenvoiBridgeConfig → LinearBandBridgeConfig, plus Band-first env fallback	linear-c3-compatibility
C4	Public room-id field thenvoiRoomId → bandRoomId, while deliberately retaining the physical thenvoi_room_id SQLite schema	c4-room-metadata-storage
C5	The package itself — @thenvoi/sdk → @band-ai/sdk and every remaining public symbol	c5-package-symbols
C6	Default env prefix BAND_, default host app.band.ai	c6-no-stale-live-thenvoi, c6-urls
C7	Platform tools → band_*, MCP → mcp__band__	c7-tools-mcp

P-C5-3, P-C3-1, P-STO-01 are individual numbered proofs within a slice.

C3, C4 and C5 are the three slices that changed public API surface, so they are the ones with compile proofs — the only way to show a consumer's code still typechecks across a breaking rename. They are the evidence that the 1.0 rename was safe, which is why silently broken proofs mattered.

Those three slices' proofs compile a real consumer against the built package to show Band symbols resolve and Thenvoi ones do not. Each spawned node_modules/.bin/tsc — a POSIX shell script whose Windows counterparts are tsc.CMD/tsc.ps1. Reproduced directly:

spawnSync("packages/sdk/node_modules/.bin/tsc", ["--version"])
// → { status: null, error: { code: 'ENOENT' } }

status ?? 1 then laundered "never ran" into a compiler verdict. Proofs asserting new names compile failed; proofs asserting old names are rejected were one weakened assertion away from passing while proving nothing.

Now resolved through the compiler's own JS entry under process.execPath, with a throw on spawn failure — a compiler that did not run must never be reportable as "compilation failed as expected". Three copies of the helper became one in tests/support/compileProof.ts, which also raises a clear error when dist/ is missing instead of failing as a puzzling compile error.

Verified non-vacuous: pointing a negative case at a symbol that does exist makes both assertions fail, as they should.

The same assumptions ran through P-C5-1, so that proof also gets a Windows-safe npm pack, a tar invocation that does not read D: as a remote host, a junction instead of an elevation-requiring symlink, and a Node-native scan replacing grep -rl — that last one was silently vacuous anywhere grep is absent, since missing stdout satisfied "no packed file leaks the legacy scope" without opening a single file.

2. CI now validates the trunk

ci.yml triggered on pull_request only, so a PR run only ever tested a merge preview, never main itself. Added push: [main], plus three changes that stop the trunk run being fake-green:

• both package filters forced true on push — otherwise ci-status could pass having skipped lint and test;
• cancel-in-progress limited to PRs — a cancelled trunk run leaves that commit with no verdict;
• release-intent kept PR-only — it has no baseline on a push and would fail every trunk build.

The hardening suite asserts all four, so the trigger cannot be quietly removed or hollowed out. This is detection, not prevention — it complements the ruleset rule above rather than replacing it.

3. Line endings were a test input

core.autocrlf with no .gitattributes meant Windows checkouts arrived CRLF, so guards reading the repo's own files compared against bytes CI never sees: the C6 brand-guard heading, plus seven \n-anchored regexes in the hardening suite. Fixed at the root with * text=auto eol=lf. Nothing tracked needs CRLF; .sh and the Dockerfiles positively require LF.

4. Honest skips instead of permanent red

• SQLite store tests skip when node:sqlite is unavailable (unflagged only from Node 22.13; the pinned toolchain and CI are both well past it).
• Four release-publisher tests skip off POSIX — they drive a #!/bin/sh npm stub for a script that spawns a bare npm.

Both are inert on CI and verified so. Running the SQLite-gated tests with the module available gives 71 passed, 0 skipped. A permanently red suite is one nobody reads, which is how the original failure stayed invisible.

Also: vitest timeout headroom — the 5s default sat below the ~4.5s cost of the import-boundary proofs, making them a coin flip on a loaded runner.

Finding 1 needed no code change

c5-package-symbols.test.ts's .release-hold assertion was already removed by #162 before this branch. Verified with git log 1eb7bc9..origin/main -- packages/sdk/tests/c5-package-symbols.test.ts. Only a stale header comment remained, deleted here.

Verification (Windows, Node 22.12)

Check	Before	After
SDK suite	29 failed / 903 passed	0 failed / 922 passed / 14 skipped
Release hardening	7 failed / 42 passed	0 failed / 46 passed / 4 skipped
pnpm -r lint	0 errors	0 errors
pnpm -r typecheck	clean	clean
OpenClaw suite	202 passed	202 passed

Every remaining skip is environment-gated and runs on CI.

Reviewer note

Everything in "What changed" is code and is meant to be reviewed normally.

The ruleset section is different: merging this PR does not apply it. Both settings have to be made by hand in the GitHub UI. Until they are, the trunk is still unprotected — and this PR itself could be merged red.

Follow-up, not addressed here

packages/sdk/package.json declares engines.node >=22.12, but node:sqlite — which the session-room store needs — is unflagged only from 22.13, and CONTRIBUTING states 22.14. A consumer on exactly 22.12 gets a runtime error from a documented feature. Tightening published engine metadata is a product call, so the tests were made honest instead of changing it silently.

🤖 Generated with Claude Code

[linear-code]

INT-1312

[AlexanderZ-Band]

issue: this spawnSync call sets no timeout, so a hung tsc blocks the Node event loop directly — Vitest's testTimeout/hookTimeout (the whole point of COMPILE_PROOF_OPTS/COMPILE_PROOF_TIMEOUT_MS) can't fire while a synchronous spawn is blocking the same thread. Same gap in runNpm's spawnSync call at c5-package-symbols.test.ts:338. A hang here runs past the documented 120s budget until GitHub Actions' own job-level timeout eventually kills it, not Vitest's.

[yaron-thenvoi]

Agreed — added an explicit timeout: COMPILE_PROOF_TIMEOUT_MS to this spawnSync call (and to runNpm's in c5-package-symbols.test.ts:338, same gap). A hang is now caught by the test's own budget via the existing result.status === null → throw branch, instead of running until the job-level timeout.

[AlexanderZ-Band]

nit: wrapping each arg in bare double quotes doesn't stop cmd.exe from expanding %VAR% sequences inside a quoted segment — quoting neutralizes spaces/&/^ but not %. If os.tmpdir() ever produced a path containing a literal %...% run, cmd.exe would substitute or drop it before npm sees the argument, and npm pack would silently write the tarball to an unexpected path.

[yaron-thenvoi]

Agreed — quoting doesn't stop cmd.exe's %VAR% expansion, and correctly escaping it (doubling % behaves differently in a batch file vs. a /c command line) is more machinery than this edge case is worth. Rather than ship a half-working escape, runNpm now throws if any argument contains % before it ever reaches the shell, so a path like that fails loudly instead of silently packing to the wrong destination.

[AlexanderZ-Band]

nit: this checkout step still runs unconditionally on push, but the only step it feeds — dorny/paths-filter right below — is now gated if: github.event_name == 'pull_request' and never runs on push. Every trunk push does a full checkout for a step that's skipped.

[yaron-thenvoi]

Agreed — added if: github.event_name == 'pull_request' to the checkout step to match paths-filter below it, so a trunk push no longer pays for a checkout nothing downstream uses.

[AlexanderZ-Band]

Reviewed the full diff (compile-proof helper, CI trunk trigger, line-ending fix, honest skips). Solid, well-reasoned change overall — three inline nits/issues found, none blocking:

• issue compileProof.ts:79 — spawnSync has no timeout, so COMPILE_PROOF_TIMEOUT_MS can't actually rescue a hung tsc/npm (same gap in runNpm at c5-package-symbols.test.ts:338).
• nit c5-package-symbols.test.ts:340 — Windows arg quoting doesn't neutralize cmd.exe's %VAR% expansion.
• nit ci.yml:42 — checkout step runs unconditionally on push even though its only consumer (paths-filter) is now PR-only.

(The three comments above landed as separate auto-submitted reviews rather than one batch — a GitHub API quirk of posting comments without an explicit pending-review id. This review just wraps them up.)

[AlexanderZ-Band]

suggestion: unrelated to this diff, but while verifying — root README.md's Development section (line 367-372) lists pnpm test before pnpm build, while CONTRIBUTING.md's new note (added in this PR) says some proofs need a build first (compileProof.ts's linkBuiltSdk throws if dist/ is missing). The dependency isn't new, just newly documented in one place and not the other. Worth a follow-up to reorder or cross-reference.

[yaron-thenvoi]

Agreed and fixed here rather than deferred — reordered README's Development section to pnpm build before pnpm test, with a cross-reference to CONTRIBUTING.md's note on which tests need that build first.

[AlexanderZ-Band]

Both remaining inline nits and the timeout issue addressed as described — verified in the diff (spawnSync timeouts added, % guard in runNpm, checkout gated to pull_request). Approving.