ci: comprehensive overhaul — integration tier + secret scan + dep review + lockfile portability + tarball validation - #137
Merged
Conversation
…w, lockfile portability, tarball validation Per Dylan's "overhaul the CI and make it much more comprehensive and robust." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution fix the new tarball checker uncovered. ## What's new ### `integration` — MCP-functional tier in CI (Ubuntu/macOS/Windows × Node 22) The new MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Catches SDK→MCP→bridge→handler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service. Builds the stub-claude SEA binary on Windows (Node 20.19+ blocks spawning .cmd shims via child_process.spawn — CVE-2024-27980 mitigation). ### `secrets` — gitleaks scan on every push + PR `gitleaks/gitleaks-action@v2` with `fetch-depth: 0` so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files committed by accident, OAuth secrets, private keys. ### `dep-review` — github/dependency-review-action on PRs Diffs lockfile changes against the GitHub Advisory Database. `fail-on-severity: moderate` — moderate or higher CVE in any newly introduced dep blocks the PR. Comments summary on PR on failure. PR-only (no main-branch run); `ci-ok` treats `skipped` as ok. ### `lockfile` — portability check in fresh tmpdir Copies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci --ignore-scripts --no-audit` there. Same check as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with `--include=optional`" classes the heartbeat journal documented (hb #48 / #103). The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run. ### `tarball` — validates `npm pack` contents New `.github/scripts/tarball-check.mjs` — runs `npm pack`, inspects the resulting tarball for forbidden patterns: - `.env*`, `credentials.json`, `*.key`, `*.pem`, `id_rsa`, etc. - `__tests__/`, `*.test.ts`, `*.spec.ts` - `node_modules`, `.git`, `coverage`, sourcemaps, `.vscode`, `.idea`, `.DS_Store` - Tarball size > 5MB (catches accidental large-file commits) - `package.json` / `README` missing Also prints a sorted top-10 largest files for the CI step summary. ## Real bug uncovered The tarball checker's first run flagged **270+ test files were being shipped in the npm tarball**. `package.json#files` listed `"src/"` which swept in the entire test suite. Fixed by replacing the broad `"src/"` entry with explicit subdirs: - "src/backend/", "src/core/", "src/frontend/", "src/plugins/", "src/storage/", "src/util/" - "src/bootstrap.ts", "src/cli.ts", "src/index.ts", "src/login.ts" Tarball size dropped from including 270+ test files to a clean 99 source-only files (180KB → still 180KB but with no test scaffolding). Future versions of `talon-agent` on npm won't ship test fixtures. ## Coverage config `vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% for now — ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). ## ci-ok gate Updated to require all 10 jobs (was 6). Branch protection in `.github/branch-protection.json` already gates merges on `CI Status`, so adding a new required job means adding it to the `needs:` list + the `check` calls in the gate script — no separate config change. ## Test plan - [x] `tsc --noEmit` clean - [x] `prettier --check` clean on touched files - [x] `npm run test:integration` passes locally (4 tests) - [x] `npm run tarball:check` passes locally — clean tarball - [x] Lockfile portability passes (replicated tmpdir check, 7s npm ci) - [x] YAML syntax valid - [x] Existing test suite unaffected (1711 tests pass + 1 pre-existing main flake)
…n there yet
The MCP-functional tier passes on Ubuntu + macOS but fails on Windows:
the stub's SEA-bundled MCP client + StdioClientTransport spawn-and-fetch
chain to the recording handler doesn't complete under SEA-on-Windows.
Three of four cases see zero captures at the recording handler, even
though `PostToolBatch: terminating SDK loop on mcp__telegram-tools__end_turn`
fires (so the SDK loop sees the tool, but MCP dispatch never lands).
Likely culprits to investigate (out of scope for this PR):
- esbuild SEA bundle handling of the dynamic `await import("@modelcontextprotocol/sdk/...")`
- StdioClientTransport.spawn behavior under SEA-on-Windows
- Localhost loopback / fetch from spawned subprocess on Windows
The other Windows test paths still run — `Tests (windows-latest, Node 22)`
and `Functional (windows-latest)` cover the platform without MCP dispatch
(sdk-stub, talon-functional). Reinstating Windows for `integration` is
gated on root-causing the failure.
…esn't run there yet" This reverts commit 6410446.
…ndows debugging
Two changes targeting the Windows integration failure:
1. fake-claude.mjs: convert dynamic `await import("@modelcontextprotocol/sdk/...")`
to static top-level imports. Likely root cause of the Windows failure —
esbuild's CJS bundling preserves `await import(literal)` as `Promise.resolve(require(literal))`,
which works locally where node_modules exists, but the SEA binary on
Windows ships only the bundled .cjs without node_modules. Static
imports force esbuild to inline the SDK module bodies into the bundle.
Verified: rebuilt SEA bundle locally, `grep "var StdioClientTransport"
fake-claude.cjs` confirms the SDK is now inlined.
2. talon-mcp-functional.test.ts: add `withProtocolLogOnFailure()` helper.
The MCP-functional tier has subprocess hops (SDK loop → MCP server
spawn → bridge HTTP) that produce no Vitest output when something
goes wrong upstream of the actual `expect(...)`. This wrapper dumps
the last 60 lines of the stub's protocol log on assertion failure
so future Windows regressions are diagnosable from the CI log
directly. Wraps the canary first test only (the others' assertions
are all downstream of the same MCP path).
Local: all 11 MCP-functional tests still pass on Linux. Windows
verification has to come from the CI run on this branch.
dylanneve1
force-pushed
the
feat/ci-overhaul
branch
from
May 10, 2026 00:47
6410446 to
9507333
Compare
…sx, not npx tsx
Real root cause of the Windows integration failure.
`buildMcpServers` was spawning `npx tsx <path>` on Windows and
`node --import <tsx-loader> <path>` on POSIX. The Windows path goes
through the MCP launcher (which proxies stdio), so the launcher then
runs `child_process.spawn("npx", [...])` to start the actual MCP server.
Node 20.19+ refuses to execute `.cmd` shims via `child_process.spawn`
without `shell: true` (CVE-2024-27980 mitigation). `npx` on Windows IS
a .cmd shim. Result: the launcher's spawn fails immediately, the MCP
server never starts, the stub's MCP client tries to connect over a
dead pipe and gets `MCP error -32000: Connection closed`.
The earlier hypothesis (SEA bundle missing dynamic imports) was wrong
— the static-imports change in 9507333 is still the right fix
(removes a dynamic-import gotcha entirely), but the actual blocker
was this command shape.
Fix: use `node --import <tsx-loader> <path>` on every platform. tsx as
a Node loader is platform-identical, and we never spawn `npx.cmd`. The
launcher's spawn now hits a plain `node` binary, which works under
Node's CVE mitigation.
Verified locally: 11 / 11 MCP-functional tests still pass on Linux.
Windows verification: this branch's CI run.
Diagnostic from the previous failed Windows CI run that pinned this:
MCP_CONFIG servers: ["telegram-tools"]
MCP connect error for telegram-tools: MCP error -32000: Connection closed
The launcher's spawned child died before completing initialize.
Second Windows iteration. Same `MCP error -32000: Connection closed` after the previous fix, even with `node --import <path>` everywhere. The remaining issue: `--import` accepts URLs or paths, but on Windows a raw backslash path like `D:\a\talon\talon\node_modules\tsx\dist\esm\index.mjs` is ambiguous between path and URL. Node's loader-hook resolution silently fails to register tsx, every subsequent `.ts` import in `mcp-server.ts` throws, the MCP server crashes, the stub's MCP client gets "Connection closed" on its initialize handshake. Fix: convert via `pathToFileURL(resolve(...)).href` so the loader URL is always a clean `file:///D:/.../index.mjs` regardless of platform. Linux already worked because `/path/to/foo.mjs` is unambiguous as both a path and a URL. Local: 11 / 11 still pass on Linux. Windows: this commit's CI run is the truth-or-not test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Per Dylan's "overhaul the CI and make it much more comprehensive and robust, don't hold back." Five new jobs gated through the existing `CI Status` aggregate, plus a real package-distribution bug the new tarball checker uncovered.
The CI matrix had:
quality,test(4-cell matrix),functional-cross-platform(3 OS),fuzz,security(npm audit),docker. Now it also hasintegration,secrets,dep-review,lockfile,tarball.New jobs
integration— MCP-functional tier in CI (Ubuntu/macOS/Windows × Node 22)The MCP-functional tier from #131 (`talon-mcp-functional.test.ts`) now runs on every PR + push across the full OS matrix. Drives a full Talon turn through stub claude → real handler.ts → real MCP server subprocess → real bridge → real Gateway → recording handler. Catches SDK→MCP→bridge→handler integration bugs (the PR #122 prefix-mismatch class) without needing a live external service.
Builds the stub-claude SEA binary on Windows (Node 20.19+ CVE-2024-27980 mitigation blocks .cmd shim spawning).
secrets— gitleaks scangitleaks/gitleaks-action@v2withfetch-depth: 0so it sees full git history, not just the latest commit. Catches API keys, bot tokens, .env files, OAuth secrets, private keys.dep-review—github/dependency-review-actionon PRsDiffs lockfile changes against the GitHub Advisory Database.
fail-on-severity: moderateblocks PRs introducing moderate-or-higher CVEs. Comments summary on failure.PR-only;
ci-oktreats the resultingskippedon push runs as ok.lockfile— portability check in fresh tmpdirCopies ONLY `package.json` + `package-lock.json` to a fresh tmpdir and runs `npm ci` there. Same logic as `tools/verify-lockfile.sh` locally. Catches the "lockfile drift" / "lockfile only validates with --include=optional" classes the heartbeat journal documented (hb #48 / #103).
The other test jobs run `npm ci` in the source tree where `node_modules` state could mask portability bugs. This one starts from zero, every run.
tarball— validatesnpm packcontentsNew `.github/scripts/tarball-check.mjs` — runs `npm pack`, inspects the tarball for forbidden patterns:
Prints a sorted top-10 largest files for the CI step summary.
Real bug found by the tarball checker
First run flagged 270+ test files were being shipped in the npm tarball. `package.json#files` listed `"src/"` which swept in the entire test suite.
Fixed by replacing the broad `"src/"` entry with explicit subdirs (
src/backend/,src/core/,src/frontend/,src/plugins/,src/storage/,src/util/, plus the entry-point files). Tarball is now 99 source-only files. Future versions of `talon-agent` on npm won't ship test fixtures.Coverage config
`vitest.config.ts`: added an exclude list (test files, integration scaffolding, entry points) and `lcov` reporter for tooling compatibility (codecov, IDE plugins). Thresholds left at 60% — ratcheting up is out of scope for this PR (would risk breaking CI on unrelated paths). Future PRs can ratchet in 5% increments.
ci-ok gate updated
Now requires all 10 jobs (was 6). Branch protection (
branch-protection.json) gates merges onCI Status, so adding a new required job means adding it to theneeds:list + thecheckcalls in the gate script.Test plan
What I deliberately didn't add
Considered, scope-cut for follow-up:
🤖 Generated with Claude Code