ci: bring back a published-package smoke for the surviving packages - #166
Conversation
PR #162 removed deploy-package-smoke together with @1c-odata/mcp-server, and with it the only CI coverage of the published-artifact path: nothing installed the pnpm-pack tarballs (the exact artifact release.yml publishes) or booted the bins from a consumer node_modules layout. package:lint (publint + attw) reads the manifest and types but never installs anything, so a dropped files entry, a broken bin mapping, an unrewritten workspace:* range, or a runtime dep misfiled as a devDependency would all reach npm green. The new .ci/package-smoke.sh packs the four workspace tarballs, installs them into a scratch npm project pinned to the local tarballs (overrides + --install-links), imports every exports entrypoint from the installed tree, and runs both bins (1c-odata, 1c-odata-mcp) through their bin mapping. Wired as a secret-free, fork-safe package-smoke job in ci.yml. Unlike the removed job, this one also covers @1c-odata/cli, which the old smoke never packed.
There was a problem hiding this comment.
🟡 Changes recommended
The smoke script uses npx to run bins, which can fall back to downloading/executing from the registry if local shims are missing, undermining the intent and adding an avoidable CI code-exec/network path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restores CI coverage for the published-artifact (tarball) install path of the remaining @1c-odata/* packages by adding a package-smoke script and a dedicated workflow job, plus documenting the new gate.
Changes:
- Added
.ci/package-smoke.shtopnpm packthe four workspace packages, install each tarball into isolated scratch projects, import allexportsentrypoints, and run both bins. - Added a
package-smokejob to.github/workflows/ci.yml, wired afterlint-and-typecheck, with turbo creds blanked and installs running with--ignore-scripts. - Documented the new
package-smokegate inCLAUDE.md.
File summaries
| File | Description |
|---|---|
CLAUDE.md |
Documents the package-smoke CI gate and what it validates. |
.github/workflows/ci.yml |
Adds a fork-safe package-smoke job that runs the new published-tarball smoke script. |
.ci/package-smoke.sh |
Implements tarball packing, isolated consumer installs, exports import checks, and bin boot checks. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
With a broken bin mapping — the exact failure this smoke exists to catch — npx falls back to fetching a same-named package from the registry and executing it, masking the breakage and adding a network/code-exec path. node_modules/.bin invocation fails fast instead.
There was a problem hiding this comment.
🟢 Approval recommended
The CI job and smoke script are coherent and fork-safe; the only feedback is minor documentation accuracy around what “all exports” includes.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
.ci/package-smoke.sh:83
- The comment says the smoke imports every
exportsentrypoint, but the script does not exercise the./package.jsonexport that each package publishes. Either test that entrypoint too or clarify the wording so it matches what the script actually checks.
# Import every `exports` entrypoint from each installed tree and touch one known
# symbol per entrypoint — a dropped dist file or a phantom dependency fails HERE,
# at consumer-side module resolution, instead of passing green in the workspace.
.github/workflows/ci.yml:162
- This job-level comment claims it imports every
exportsentrypoint, but the smoke script does not currently exercise the./package.jsonexport. Please clarify the comment (or expand the smoke) so CI documentation matches reality.
# (the same `pnpm pack` artifact release.yml publishes), install each into its
# own isolated scratch project (pinned to the local tarballs so npm never hits
# the registry for the unpublished siblings), import every `exports` entrypoint
# from the installed tree and run both bins through their `bin` mapping.
# Catches a dropped `files` entry / broken `bin` / broken `exports` /
CLAUDE.md:67
- The new paragraph states the smoke imports every
exportsentrypoint, but the script doesn’t currently exercise the./package.jsonexport that each package publishes. Consider clarifying this sentence to avoid overstating coverage.
**package-smoke** ([`.ci/package-smoke.sh`](./.ci/package-smoke.sh)) owns the published-artifact path: pack the four workspace tarballs → install each into its own isolated scratch npm project → import every `exports` entrypoint + run both bins (`1c-odata`, `1c-odata-mcp`). Catches `files`/`bin`/`exports`/workspace-rewrite mistakes that `package:lint` (types + exports only) cannot. Secret-free, fork-safe.
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
#162 removed
deploy-package-smoketogether with@1c-odata/mcp-server— and with it the only CI coverage of the published-artifact path. Nothing has installed thepnpm packtarballs (the exact artifactrelease.ymlpublishes to npm) or booted the bins from a consumernode_moduleslayout since:package:lint(publint + attw) reads the manifest and types statically but never installs anything, so a droppedfilesentry, a brokenbinmapping, a brokenexportsmap, or an unrewrittenworkspace:*range would all reach npm green.What this adds
.ci/package-smoke.sh— pack the four workspace tarballs, then install each into its own isolated scratch project (overridespin every@1c-odata/*sibling to its local tarball so npm never reaches the registry for unpublished versions), import everyexportsentrypoint from the installed tree, and run both bins (1c-odata,1c-odata-mcp) through theirbinmapping with Commands-section-anchored assertions.package-smokejob inci.yml— secret-free, fork-safe,needs: lint-and-typecheck. The step blanks the workflow-levelTURBO_TOKEN/TURBO_TEAM, and the scratch installs run--ignore-scripts: this is the one install in CI with no lockfile pinning the third-party graph, so unpinned lifecycle scripts stay off and no write-capable token is in the environment.CLAUDE.mddocumenting the gate.Unlike the removed job, this one also covers
@1c-odata/cli, which the old smoke never packed.Scope note (verified empirically)
A runtime dep dropped from a manifest is deliberately not chased:
pnpm packrebuildsdistviaprepack, and tsdown externalizes exactly whatdependencieslists — an undeclared import gets bundled at build time and the shipped tarball keeps working (checked by deletingc12from the cli manifest: the tarball comes out self-contained and green). What breaks consumers is the manifest/layout layer, and that is what this smoke owns.Note for the parked restore PR #164
When #164 un-parks, it re-adds mcp-server's
deploy-package-smokenext to this job — keep both (this one owns the four library/stdio packages incl.cli; that one owns the mcp-server auth/migrations path) and resolve the smallci.ymlcontext conflict accordingly.Local review: Codex (high) — 1 finding, addressed (per-package isolated installs); Claude reviewer (medium) — 6 findings, 5 addressed, 1 refuted by a live run; security pass — 1 medium (turbo token exposure to unpinned lifecycle scripts), addressed with
--ignore-scripts+ blanked env.