ci(release): type-check the packed tarball from the consumer side (closes #77) - #78
ci(release): type-check the packed tarball from the consumer side (closes #77)#78yakimoto wants to merge 3 commits into
Conversation
…oses #77) The e2e-smoke asserted every declared `types` target EXISTS in the tarball. It never asserted they RESOLVE — and only the second is a thing a consumer experiences. `dist/sdk-server.d.ts` references a type from @anthropic-ai/claude-agent-sdk, an OPTIONAL peer dependency, so the existence check stays green on a package that fails to type-check for anyone who did not install that peer. Adopts option (a) from #77: requiring the peer to type-check ./sdk-server is honest, because that subpath exists to build a config for the Agent SDK. The gate now enforces both halves of the contract: root must type-check WITHOUT the optional peer ./sdk-server must type-check WITH it It deliberately does NOT assert that ./sdk-server fails without the peer — that would freeze current behaviour into the gate and turn a future switch to self-contained declarations (option (b)) into a spurious release failure. Each arm installs the real packed tarball into its own throwaway project, so its install shape is exactly what it claims to test; re-running `npm install --no-save` in the existing smoke dir would rebuild that tree from its empty package.json and could drop the tarball itself. Compiler, node types and the peer are pinned to the versions package-lock.json already resolves, read from the lockfile rather than `require("<pkg>/package.json")` — the Agent SDK ships an exports map with no "./package.json" entry, so requiring its manifest as a subpath throws ERR_PACKAGE_PATH_NOT_EXPORTED. A package missing from the lockfile fails loudly instead of becoming an empty version string that installs whatever `latest` happens to be. skipLibCheck is off in the probe tsconfig — with it on, tsc never looks inside node_modules declarations, which is the entire class being tested. That also makes tsc visit dependency declarations, so diagnostics that do not name @wave-av/mcp-server are reported as warnings: a gate that fails a release on somebody else is a gate that gets switched off. Proven by negative control before merge, both against a real build+pack: NC1 leak the optional peer into a root-reachable declaration -> `types ok` still green, root arm FAILS (TS2307) NC2 point the subpath declaration at a nonexistent module -> `types ok` still green, ./sdk-server arm FAILS (TS2307) NC3 arm with no compiler installed -> fails loudly, does not pass vacuously happy path on unmodified HEAD -> both arms green Also extends .gitignore: the smoke packs a *.tgz into the repo root, and this public repo had no entries for local-secret files or OS cruft. None has ever been committed here — the entries exist only so a stray `git add -A` cannot be the first time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6a4c3044-db95-4fb8-9e35-6e05af426543) |
|
@coderabbitai review |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 14 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
Comment |
✅ Action performedReview finished.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Co-authored-by: Codesmith <codesmith-bot@users.noreply.github.com>
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 39 minutes. |
The type-resolution arm downgraded a whole class of its OWN mis-wiring to a warning and then exited 0 claiming "type resolution ok", having compiled nothing. Reported by Devin on #78; reproduced end-to-end before changing anything. The excuse bucket was keyed on "the line carries a file location", on the premise -- written into the comment it replaced -- that tsc reports setup errors without one. It does not: $ tsc -p tsconfig.base.json --pretty false tsconfig.base.json(4,5): error TS5023: Unknown compiler option 'foo'. RC=2 That line is not OURS, matches the old DEP_DIAG_RE, so it landed in OTHERS as a ::warning, GLOBAL stayed empty, and the RC!=0 branch printed "type resolution ok" and exited 0. Same for TS5024, TS6046, TS5012. A release could ship advertising consumer-verified types with nothing verified -- the failure mode the arm exists to prevent, arriving through the arm itself. The bucket is now an allowlist: a diagnostic is excused only if its path names a directory under node_modules. Everything else -- located or not -- is GLOBAL and fatal. node_modules/ is anchored to a directory boundary so a sibling that merely ends in the name (my-node_modules/app.ts) buys no excuse. Receipts, real tsc 5.9.3 against a mis-wired arm: old run-arm.sh -> "type resolution ok ... exited 2" EXIT=0 new run-arm.sh -> "the arm verified nothing" EXIT=1 Negative control, a genuine upstream regression in a dependency's own .d.ts, which must stay excused so the gate does not become one that gets switched off: node_modules/fakedep/index.d.ts(1,30): error TS2304 -> warning, EXIT=0 test-classifier.sh pins all eleven cases and is read straight out of run-arm.sh so it cannot drift from what it pins. It fails 5 of 11 against the old pattern, which is the point of adding it. Refs #77.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_28e18d60-2b27-4716-bbfc-734971d0b752) |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
ApprovabilityVerdict: Approved 291cfed CI/CD-only changes adding type-resolution checks to the release workflow. No production code affected. The author owns all changed files. The open review comment describes a scenario the code explicitly handles with test coverage. You can customize Macroscope's approvability policy. Learn more. |
| #!/usr/bin/env bash | ||
| # Pin the diagnostic classification in run-arm.sh. | ||
| # | ||
| # The bug this drill exists for: the excuse bucket used to be keyed on "the line | ||
| # carries a file location", on the belief that tsc reports setup errors without | ||
| # one. It does not -- `tsconfig.base.json(4,5): error TS5023` has a location -- | ||
| # so a mis-wired arm that compiled nothing was downgraded to a warning and the | ||
| # gate exited 0 claiming "type resolution ok". Case 2 is that bug; it fails | ||
| # against the old regex and passes against the current one. | ||
| set -uo pipefail | ||
|
|
||
| HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| # Mirror the two patterns under test, read straight out of run-arm.sh so this | ||
| # drill cannot drift away from the thing it pins. | ||
| eval "$(grep -E '^readonly (OURS_RE|DEP_DIAG_RE)=' "$HERE/run-arm.sh")" |
There was a problem hiding this comment.
🔍 Classifier drill is never executed by any workflow
scripts/smoke/consumer-types/test-classifier.sh pins the two regexes in scripts/smoke/consumer-types/run-arm.sh:28 and scripts/smoke/consumer-types/run-arm.sh:57, but no workflow invokes it: .github/workflows/lint.yml only runs npm run lint / npm run type-check (both scoped to src/), .github/workflows/release.yml calls only run-arm.sh, and package.json declares no test script (the release gate even emits a no unit tests warning for exactly this reason). So the regression drill can silently rot the next time the classification logic is edited. Consider wiring it into the lint workflow or a test script.
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Closes #77.
The gap
The release e2e-smoke asserts every declared
typestarget exists in the tarball. It never asserts they resolve — and only the second is something a consumer experiences.dist/sdk-server.d.tsreferences a type from@anthropic-ai/claude-agent-sdk, an optional peer dependency. So the existence check stays green on a package that fails to type-check for anyone who did not install that peer. That is my own gate from #76, filed against myself.The decision — option (a) from #77
Requiring the peer to type-check
./sdk-serveris honest: that subpath exists to hand a config object to the Agent SDK, so a consumer using it has the SDK by definition. The gate now enforces both halves:./sdk-serverIt deliberately does not assert that
./sdk-serverfails without the peer. That would freeze today's behaviour into the gate and turn a future switch to self-contained declarations (option (b)) into a spurious release failure.The CHANGELOG note from
d2f8b96now says which option was chosen, as #77 asked.Negative controls — run locally against a real build + pack, before this was pushed
A guard that has only ever been observed succeeding is indistinguishable from a guard that always succeeds. Three arms, all recorded:
The
types okline printing immediately above each failure is the point of the PR: that is the old gate being green on a package a consumer cannot use.Also green locally:
npm run lint,npm run type-check,actionlint,shellcheck.Implementation notes worth a reviewer's eye
skipLibCheck: falsein the probe tsconfig is load-bearing. With it on, tsc does not look insidenode_modulesdeclarations at all, and both arms would pass unconditionally.@modelcontextprotocol/sdkand the Agent SDK's own declarations. Diagnostics that do not name@wave-av/mcp-serverare reported as::warning, not failures — a gate that blocks our release on somebody else's.d.tsis a gate that gets switched off. It currently emits 22 such warnings, all real defects in the Agent SDK's bundledsdk.d.ts(TS2304on names it never declares). Worth reporting upstream; not ours to gate on.types: ["node"], not[]. An empty list is not a purer test, just a less realistic consumer — it buried the real signal under ~140 diagnostics about the peer needing@types/node.package-lock.json, notrequire('<pkg>/package.json'). The first local run failed withERR_PACKAGE_PATH_NOT_EXPORTED: the Agent SDK ships anexportsmap with no./package.jsonentry — the same trap thebincheck in this file already documents for our own package. A package missing from the lockfile now fails loudly rather than becoming an empty version string that installs whateverlatesthappens to be.npm install --no-saveinside the existing smoke dir rebuilds that tree from its emptypackage.jsonand can drop the tarball itself.typeof import(...)— a purely type-level reference. The root entry is the executable (#!/usr/bin/env node, callsserver.connect()at top level); a real import would start the MCP server and hang the job.Honest scope limit
dist/index.d.tsis currentlyexport {};— the root entry has no library surface — so the root arm today proves that the roottypestarget resolves and nothing past it. That is thin because the package is thin at root, not because the check is lax; NC1 shows it bites the moment anything is reachable from root. Stated in a comment inroot.tsso nobody reads more into it later.Out of scope but bundled — say if you want it split
.gitignoregains*.tgz(this smoke packs one into the repo root, so anyone reproducing it locally leaves a publishable tarball untracked) plus the usual local-secret and OS-cruft entries, which this public repo had none of. Nothing of that kind has ever been committed here — the entries exist so a straygit add -Acannot be the first time. Same finding as wave-av/adk#71.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Note
Low Risk
Changes are confined to release CI smoke scripts and repo hygiene; they do not alter runtime MCP server behavior or publish credentials beyond adding pre-publish checks.
Overview
Extends the release verify e2e-smoke so it proves published
.d.tsfiles resolve for consumers, not only that they exist in the tarball (#77).After
npm packand the existing tarball checks, CI runs two consumer-sidetscarms in separate throwaway projects: root with only the tarball (no optional@anthropic-ai/claude-agent-sdkpeer), and./sdk-serverwith the tarball plus that peer.typescript,@types/node, and the peer versions are read frompackage-lock.jsonso the gate does not float onlatest.New
scripts/smoke/consumer-typessupplies type-only probes (typeof import(...)), tsconfigs withskipLibCheck: false, andrun-arm.sh, which fails on diagnostics in@wave-av/mcp-serveror probe files while downgrading errors inside othernode_modulesdeclarations to warnings.test-classifier.shpins that classification so mis-wired tsconfig errors cannot pass vacuously.CHANGELOG documents the #77 contract;
.gitignoreadds*.tgz,.env*, and.DS_Storefor local pack/smoke reproduction.Reviewed by Cursor Bugbot for commit 291cfed. Configure here.
Summary by cubic
Adds consumer-side type checking to the release workflow so the packed tarball’s
.d.tsresolve for real consumers. Implements #77 (root resolves without the optional peer;./sdk-serverresolves with@anthropic-ai/claude-agent-sdk) and hardens the gate to fail on misconfiguredtscruns.tscwithskipLibCheck: false.typescript,@types/node, and the peer versions frompackage-lock.json.@wave-av/mcp-serveror probe files; only excuses errors located undernode_modules; fails the arm on unclassifiedtscerrors; addsscripts/smoke/consumer-types/test-classifier.shto lock this behavior.scripts/smoke/consumer-types; updates.gitignoreandCHANGELOG.md.Written for commit 291cfed. Summary will update on new commits.
Note
Add consumer-side TypeScript type-checking of the packed tarball to the release gate
./sdk-serversubpath with@anthropic-ai/claude-agent-sdkinstalled.run-arm.shwhich runstscwithskipLibCheckdisabled and classifies diagnostics: errors from this package's tarball or probe files are fatal, errors undernode_modulesfrom third-party declarations are downgraded to warnings.root.ts,sdk-server.ts) use type-only imports to force declaration resolution without executing the package.tscexits non-zero due solely to third-party declaration errors.Macroscope summarized 291cfed.