fix(release): make private workspace package boundary packer-independent - #26
Merged
qmarcelle merged 1 commit intoAug 19, 2026
Merged
Conversation
…ETA-297)
`@workspacejson/cli@0.6.0` was tagged but never published. Its publish run
failed at the tarball gate, two steps before `npm publish`:
package.devDependencies.@workspacejson/mining-core leaks "workspace:*"
into the packed manifest.
The gate was right. Three things behind it were not.
1. The invariant was false. `publish-cli.yml` justified publishing with npm on
the grounds that @workspacejson/cli declares no `workspace:` dependencies.
META-297 added the private @workspacejson/mining-core as a devDependency and
that stopped being true. The last release predates that commit, so 0.6.0 was
the first publish attempt since. The standard authority migration did not
cause this; it touched only `dependencies` and `version`.
2. The gate's verdict depended on how it was invoked. The packer was inferred
from `npm_execpath`, so `pnpm run release:verify-packs` packed with pnpm
while CI's `pnpm --filter ... exec ...` packed with npm. The same commit
verified green locally and red in CI, and the green run measured bytes
nobody publishes. The packer is now npm unconditionally, because that is
what `npm publish` ships.
3. The invariant was the wrong shape. It tested for the literal `workspace:`
string, which is syntactic and packer-dependent:
npm pack -> "@workspacejson/mining-core": "workspace:*" caught
pnpm pack -> "@workspacejson/mining-core": "0.0.0" waved through
`0.0.0` is a dangling reference to a package that exists nowhere, wearing a
version that reads as legitimate. Switching packers would have published it
with a green gate. The rule is now identity-based and packer-independent: a
public package's packed manifest must not reference a private workspace
package at all, under any spelling. Private packages are discovered by name
from the workspace, so it needs no maintenance and cannot be evaded by a
rewrite.
Also fixed, found while proving the above:
* Moving the declaration to the private root workspace removed the edge pnpm
used to order `pnpm -r build`, and a clean checkout then built the CLI and
mining-core in parallel — the CLI failing to resolve its own bundle input.
The CLI's build script now builds that input first, so the guarantee travels
with the package that needs it rather than depending on invocation order.
* The deterministic packer surfaced the same class of defect in `agents-audit`,
which depends on its sibling by `workspace:*`. That package is frozen and no
workflow here publishes it, so it is verified with pnpm — the packer matching
its actual (non-)publisher — and the reasoning, plus what must change if
META-243 makes this repository its publisher, is recorded at the override.
Red tests cover both spellings under both packers, and that `npm_execpath` no
longer changes the packer. The publish workflow's invariant comment is replaced
with the one the gate now actually asserts.
Version bumped to 0.6.1. No mining, retrieval, provenance or artifact semantics
change; the 0.5.0 standard authority migration 0.6.0 carried ships unchanged.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
qmarcelle has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Reviewer's GuideEnforces a packer-independent release boundary so public packages cannot reference private workspace packages, makes npm the deterministic packer for release verification, relocates the private mining-core dependency to the root workspace while ensuring the CLI builds it explicitly, and extends CI plus new red tests to cover npm vs pnpm behavior and the 0.6.1 documentation changes. Sequence diagram for verify-package-tarball release guard with packer-independent private boundarysequenceDiagram
actor CI
participant cli_package as @workspacejson/cli
participant verify_script as verify-package-tarball.mjs
participant npm
participant pnpm
participant workspace as workspace_packages
CI->>cli_package: pnpm --filter @workspacejson/cli exec node verify-package-tarball.mjs
cli_package->>verify_script: load package.json (sourceManifest)
verify_script->>verify_script: packer = WORKSPACEJSON_PACKER ?? "npm"
alt packer is npm
verify_script->>npm: npm pack
else packer is pnpm
verify_script->>pnpm: pnpm pack
end
verify_script->>workspace: privateWorkspacePackageNames()
workspace-->>verify_script: Set(private_workspace_names)
verify_script->>verify_script: assertNoPrivateWorkspacePackages(manifest)
verify_script->>verify_script: assertNoWorkspaceProtocol(manifest, "package")
verify_script->>verify_script: assertStandardDependenciesArePinned(manifest)
verify_script->>verify_script: assertRuntimeFiles(manifest, files)
verify_script-->>CI: exit 0 on success / error on private workspace reference
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
verify-package-tarball.test.mjs, thecpSyncfilter relies on string checks likesrc.includes("/node_modules")andsrc.includes("/.git/"), which will break on Windows path separators; consider usingpath.sep,path.basename, or a more robust directory check to keep the tests cross-platform. - The
privateWorkspacePackageNameshelper inverify-package-tarball.mjsassumes all entries under the parent directory ofpackageDirectoryare packages; if non-package folders are expected there, you might want to guard with an additional check (e.g., skip directories without apackage.jsonat the expected depth or restrict to a known workspace root) to avoid accidental misreads.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `verify-package-tarball.test.mjs`, the `cpSync` filter relies on string checks like `src.includes("/node_modules")` and `src.includes("/.git/")`, which will break on Windows path separators; consider using `path.sep`, `path.basename`, or a more robust directory check to keep the tests cross-platform.
- The `privateWorkspacePackageNames` helper in `verify-package-tarball.mjs` assumes all entries under the parent directory of `packageDirectory` are packages; if non-package folders are expected there, you might want to guard with an additional check (e.g., skip directories without a `package.json` at the expected depth or restrict to a known workspace root) to avoid accidental misreads.
## Individual Comments
### Comment 1
<location path="scripts/verify-package-tarball.test.mjs" line_range="96-99" />
<code_context>
+];
+
+function runVerifier(root, env) {
+ return spawnSync(process.execPath, [join(root, "scripts", "verify-package-tarball.mjs")], {
+ cwd: join(root, "packages", "cli"),
+ encoding: "utf8",
+ env: { ...process.env, WORKSPACEJSON_PACKER: undefined, npm_execpath: undefined, ...env },
+ });
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Unset env vars using deletion instead of assigning `undefined` to avoid leaking a string `'undefined'` into the child process.
In `spawnSync`, env vars must be strings; setting them to `undefined` won’t unset them and can instead pass the literal `'undefined'` into the child process. That means `WORKSPACEJSON_PACKER` or `npm_execpath` may be set to `'undefined'`, affecting packer selection. To truly clear them, build the env object without those keys (e.g. copy `process.env`, delete those keys, then apply `env` overrides) or only include them when you explicitly want to set them.
</issue_to_address>
### Comment 2
<location path="scripts/verify-package-tarball.test.mjs" line_range="106-107" />
<code_context>
+function scratchCopy() {
+ const directory = mkdtempSync(join(tmpdir(), "wjson-pack-guard-"));
+ const root = join(directory, "repo");
+ cpSync(repoRoot, root, {
+ recursive: true,
+ filter: (src) => !src.includes("/node_modules") && !src.includes("/.git/") && !src.endsWith("/.git"),
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Filter used in `cpSync` is POSIX-path-specific and may not exclude `node_modules` or `.git` on Windows.
These checks rely on POSIX-style separators (`/`), but on Windows `src` will have `\`, so the excludes won’t trigger and `node_modules` / `.git` may be copied into the scratch directory, slowing tests and potentially changing behavior. Please normalize `src` (e.g., replace `\` with `/` or split on `path.sep`) before applying these checks, or use a path-aware predicate instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+96
to
+99
| return spawnSync(process.execPath, [join(root, "scripts", "verify-package-tarball.mjs")], { | ||
| cwd: join(root, "packages", "cli"), | ||
| encoding: "utf8", | ||
| env: { ...process.env, WORKSPACEJSON_PACKER: undefined, npm_execpath: undefined, ...env }, |
There was a problem hiding this comment.
issue (bug_risk): Unset env vars using deletion instead of assigning undefined to avoid leaking a string 'undefined' into the child process.
In spawnSync, env vars must be strings; setting them to undefined won’t unset them and can instead pass the literal 'undefined' into the child process. That means WORKSPACEJSON_PACKER or npm_execpath may be set to 'undefined', affecting packer selection. To truly clear them, build the env object without those keys (e.g. copy process.env, delete those keys, then apply env overrides) or only include them when you explicitly want to set them.
Comment on lines
+106
to
+107
| cpSync(repoRoot, root, { | ||
| recursive: true, |
There was a problem hiding this comment.
issue (bug_risk): Filter used in cpSync is POSIX-path-specific and may not exclude node_modules or .git on Windows.
These checks rely on POSIX-style separators (/), but on Windows src will have \, so the excludes won’t trigger and node_modules / .git may be copied into the scratch directory, slowing tests and potentially changing behavior. Please normalize src (e.g., replace \ with / or split on path.sep) before applying these checks, or use a path-aware predicate instead.
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
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
Repairs the release-integrity defect exposed by the failed
cli-v0.6.0publish.0.6.0never reached npm; its tarball gate correctly stopped a private@workspacejson/mining-coreworkspace dependency from leaking into the public CLI manifest.This PR cuts the corrected patch release as
@workspacejson/cli@0.6.1without changing mining, retrieval, provenance, or artifact semantics.What changes
publish-cli.ymlpublishes with npmagents-auditpackage that this repository does not publishworkspace:*, pnpm-style0.0.0, concrete-version disguise, ambientnpm_execpath, and a clean baseline0.6.0as tagged-but-never-published and prepares0.6.1Judgement calls under review
Build ordering
Moving
@workspacejson/mining-coreto the private root removed the dependency edge pnpm used for recursive build ordering. A cleanpnpm -r buildexposed this immediately. The CLI build script now builds its own bundled input first, so the requirement travels with the package that needs it instead of depending on workflow ordering or staledist/output.Frozen agents-audit packer
The deterministic npm default also exposed
agents-audit's existing siblingworkspace:*reference. That package is frozen and not publishable from this repository underOWNERSHIP.md, so this PR does not force an npm-publishability property it does not claim. It is explicitly verified with pnpm, while the packer-independent private-workspace identity guard still applies. If META-243 later transfers publish authority here, this override must be removed and the sibling dependency made release-safe.Verification reported on the branch
dist/release:verify-packsgreen: CLI via npm, frozen agents-audit via pnpmrelease:verify-packs:pnpmgreen for both packagesMerge only after GitHub CI confirms the same state from a clean checkout.
Summary by Sourcery
Harden release packaging so private workspace dependencies cannot leak into public manifests and publish verification consistently matches the npm release path.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: