Skip to content

fix: refresh platform binary optionalDependencies on every release#46

Merged
byapparov merged 3 commits intomainfrom
fix/publish-optional-deps-all-platforms
Apr 11, 2026
Merged

fix: refresh platform binary optionalDependencies on every release#46
byapparov merged 3 commits intomainfrom
fix/publish-optional-deps-all-platforms

Conversation

@byapparov
Copy link
Copy Markdown
Contributor

Summary

  • The committed @aictrl/cli optionalDependencies hard-pinned @aictrl/cli-linux-x64 to 0.2.0, so @aictrl/cli@0.3.0 and @0.3.1 shipped pointing at the old 0.2.0 native binary — vanilla upgrades silently kept running the stale build.
  • Only @aictrl/cli-linux-x64 was even listed; linux-arm64, darwin-*, windows-*, and the musl/baseline variants were built by script/build.ts but neither published to npm nor referenced from the top-level optionalDependencies.
  • The publish workflow now (1) loops over every platform binary the build produced under packages/cli/dist/@aictrl/ and publishes each, and (2) regenerates @aictrl/cli's optionalDependencies from those manifests at publish time, preserving genuine runtime optionals (@parcel/watcher, bun-pty) that the compiled binary still resolves at install time.
  • Removed the stale @aictrl/cli-linux-x64 entry from the committed packages/cli/package.json so the source-of-truth no longer lies about what ships.

Why @parcel/watcher and bun-pty are preserved

The compiled Bun binary can't bundle these — src/file/watcher.ts:38 does require(\@parcel/watcher-${process.platform}-${process.arch}-${libc}`)andsrc/pty/index.ts:37doesawait import("bun-pty"). Both are resolved against the host platform at runtime, so they must remain in the published optionalDependencies`.

Why the rewrite happens at publish time, not in source

script/build.ts already stamps each dist/@aictrl/cli-<target>/package.json with Script.version (the release tag), so it's the authoritative source for what binaries actually exist for this release. Reading from there guarantees the top-level package and the platform packages always agree on a version, with no manual bump step to forget.

If dist/@aictrl/ is empty when the strip step runs, the script throws — fail loud rather than silently ship @aictrl/cli with no platform binaries.

Test plan

  • Dry-ran the inline node rewrite script against a mocked dist/@aictrl/ tree containing cli-linux-x64, cli-linux-arm64, cli-darwin-arm64, cli-windows-x64-baseline. Confirmed: stale @aictrl/cli-linux-x64: 0.2.0 is replaced, @parcel/watcher and bun-pty are preserved, all 4 platform packages are added at the correct version, and stripped fields (dependencies/devDependencies/peerDependencies/peerDependenciesMeta/overrides/exports/type) are gone.
  • python3 -c "yaml.safe_load(...)" parses the updated workflow.
  • bun turbo typecheck clean (pre-push hook).
  • Real validation only happens on the next release — the workflow only triggers on release: published. Recommend cutting a test tag (e.g. v0.3.2-test.0) against a scratch package first if you want belt-and-suspenders before the next user-facing release.
  • After merge + next release: npm view @aictrl/cli@<new-version> should show all platform binaries in optionalDependencies at the matching version, and a fresh npm install -g @aictrl/cli on linux-x64 should pull @aictrl/cli-linux-x64@<new-version> (not 0.2.0).

Out of scope (worth separate cleanup)

  • packages/cli/script/postinstall.mjs:52 uses the unscoped name aictrl-${platform}-${arch} while published packages are scoped @aictrl/cli-*. It silently no-ops on failure (process.exit(0) on line 130) and bin/aictrl does the real resolution correctly using the scoped name, so this is latent rather than user-facing — but should be fixed.
  • packages/cli/script/publish.ts appears to be dead code (publishes as @aictrl/cli-ai, not invoked by any workflow). Could probably be deleted.

🤖 Generated with Claude Code

The committed @aictrl/cli optionalDependencies hard-pinned
@aictrl/cli-linux-x64 to 0.2.0, so 0.3.0 and 0.3.1 both shipped pointing
at the old native binary — upgrades silently kept running the 0.2.0
build. Only @aictrl/cli-linux-x64 was even listed; other platforms
(linux-arm64, darwin, windows, musl/baseline variants) were built but
neither published nor referenced.

The publish workflow now (1) loops over every platform binary the build
produced under packages/cli/dist/@aictrl/ and publishes each, and
(2) regenerates @aictrl/cli's optionalDependencies from those manifests
at publish time, preserving genuine runtime optionals (@parcel/watcher,
bun-pty) that the compiled binary still resolves at install time.

Removed the stale @aictrl/cli-linux-x64 entry from the committed
package.json so source-of-truth no longer lies about what ships.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Review: fix: refresh platform binary optionalDependencies on every release

Overall: LGTM — this is a well-targeted fix for a real version-drift bug. The approach of regenerating optionalDependencies from the build output at publish time is the right design, and the fail-loud guard on empty dist is good.


Verified correct

  • Platform binary loop (for dir in */): The glob */ only matches directories, so it won't try to publish stray files. The || true per-directory correctly tolerates already-published versions on reruns, matching the existing convention.
  • optionalDependencies regeneration: require(./package.json) → mutate → writeFileSync is fine in a one-shot CI script. The spread order { ...preserved, ...platformDeps } correctly lets dist-produced platform versions win over any stale source pins.
  • Fail-loud guard: Throwing when platformDeps is empty prevents silently shipping @aictrl/cli with no platform binaries. Good.
  • @parcel/watcher and bun-pty preservation: The filter !name.startsWith('@aictrl/cli-') correctly preserves genuine runtime optionals while dropping stale platform pins. Confirmed this matches what bin/aictrl and the watcher/pty imports expect.
  • package.json source change: Removing the hardcoded @aictrl/cli-linux-x64: "0.2.0" is correct; the workflow is now the sole source of truth for platform binary versions.

Minor observations (not blocking)

  1. fs.readdirSync(distDir) is not filtered to directories only (publish.yml:95). If a non-directory entry (e.g., a log file) were present in dist/@aictrl/, the readFileSync(entry + "/package.json") would throw and fail the workflow. In practice, build.ts only creates subdirectories there, so this won't happen — but adding a .isDirectory() filter or try/catch per entry would make it more resilient if the build pipeline ever changes. Low risk.

  2. || true on platform binary publishes means a total npm outage would not block the main @aictrl/cli publish (publish.yml:60). The main package would ship referencing platform binaries that don't exist on npm yet. This is a pre-existing convention shared by the other publish steps, not introduced by this PR, so it's consistent — just worth noting.

  3. The PR description correctly identifies the latent postinstall.mjs issue (unscoped aictrl-${platform}-${arch} vs scoped @aictrl/cli-*). Not in scope here, but good it's tracked.


No bugs, security issues, or logic errors found. The change is correct and ready to merge.

Reviewed SHA: 417f0bc

CI's bun install --frozen-lockfile rejected the previous commit because
the lockfile still referenced @aictrl/cli-linux-x64@0.2.0 and the stale
0.2.0 version stamp on the cli workspace. Regenerated.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

Review: fix: refresh platform binary optionalDependencies on every release

Verdict: Approve with minor suggestions. This is a solid fix for a real version-drift bug. The approach of regenerating optionalDependencies from the built artifacts at publish time is the right call.


Good

  • The empty-directory guard (throw new Error('No platform binary packages found...')) is a great safety net — fail loud instead of silently shipping a broken package.
  • Preserving non-platform optionals (@parcel/watcher, bun-pty) via the !name.startsWith('@aictrl/cli-') filter is correct and avoids accidentally dropping real runtime deps.
  • Removing the stale hardcoded @aictrl/cli-linux-x64: 0.2.0 from source package.json eliminates the root cause.

Issue: Silent publish failures leave dangling references in optionalDependencies

The publish loop uses || true per iteration, matching the pre-existing convention. However, this means if npm publish fails for a specific platform binary (network error, npm 403, etc.), the binary won't be on npm, but the strip step will still add it to @aictrl/cli's optionalDependencies. Users on that platform would get install errors.

Consider tracking which publishes succeeded and only including those in optionalDependencies, e.g.:

run: |
  published=""
  for dir in */; do
    echo "Publishing ${dir%/}"
    if (cd "$dir" && npm publish --access public --provenance); then
      published="$published ${dir%/}"
    else
      echo "::warning::Failed to publish ${dir%/}"
    fi
  done
  echo "PUBLISHED=$published" >> "$GITHUB_ENV"

Then have the strip step only include platform packages in $PUBLISHED. This is an improvement, not a blocker — the current behavior is no worse than before.


Minor: readdirSync doesn't filter for directories

The inline Node script at line ~92 of the diff does:

for (const entry of fs.readdirSync(distDir)) {
  const manifest = JSON.parse(
    fs.readFileSync(path.join(distDir, entry, 'package.json'), 'utf8')
  );

If any non-directory entry exists in dist/@aictrl/ (a stray file, .gitkeep, etc.), this will throw with a confusing ENOTDIR. Consider using { withFileTypes: true } and filtering entry.isDirectory(). Low severity since this is a build-artifact-only directory.


Confirmed out-of-scope (per PR description)

  • postinstall.mjs:50 uses unscoped aictrl-${platform}-${arch} while published packages are @aictrl/cli-*. This means findBinary() always fails and the outer catch at line 117 calls process.exit(0), so the postinstall is effectively a no-op. Not user-facing today since bin/aictrl handles resolution, but should be fixed in a follow-up.

Reviewed SHA: 08cf7d3

Address review feedback: the dist/@aictrl/ scan should not blindly trust
every entry. Filter to actual directories, skip ones without a
package.json, and only accept packages whose name matches the platform
binary shape (@aictrl/cli-<linux|darwin|windows>-...).

Catches a real failure mode I verified locally: the dead-but-still-in-tree
script/publish.ts creates dist/@aictrl/cli/package.json with name
@aictrl/cli-ai. Without the name filter, that wrapper would silently
get added to @aictrl/cli's optionalDependencies on the next release.

Verified the regex against all 11 name shapes build.ts can produce
(linux/darwin/windows × x64/arm64 × musl/baseline variants) and a
representative set of negative cases.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@byapparov
Copy link
Copy Markdown
Contributor Author

Addressed in dfb6eda.

Observation 1 (fs.readdirSync filter) — implemented. Verified there's a real failure mode here, not just a theoretical one: the dead-but-still-checked-in script/publish.ts creates dist/@aictrl/cli/package.json with name: "@aictrl/cli-ai" (publish.ts:26), which my original loop would have silently injected into @aictrl/cli's optionalDependencies if anything ever invoked that script before the strip step. The new scan filters to actual directories, skips entries without a package.json, and only accepts packages whose name matches ^@aictrl/cli-(linux|darwin|windows)(-[a-z0-9]+)+$. Tested the regex against all 11 name shapes build.ts produces (linux/darwin/windows × x64/arm64 × musl/baseline variants) and confirmed @aictrl/cli-ai, bare @aictrl/cli, and similar near-misses are correctly rejected.

Observation 2 (|| true could mask npm outages) — left as-is intentionally. It's a pre-existing convention shared by every other publish step in this workflow (@aictrl/util, @aictrl/plugin, @aictrl/sdk all use || true). Deviating just for the platform binary loop would be inconsistent without solving the underlying concern, which is really "should this whole workflow be more strict about partial-failure detection?" — that's a separate, larger conversation. Happy to follow up in a dedicated PR if the team wants to move that direction.

Observation 3 (postinstall.mjs) — agreed it's out of scope here, kept tracked in the PR description.

@byapparov byapparov merged commit 22a1226 into main Apr 11, 2026
5 checks passed
@byapparov byapparov deleted the fix/publish-optional-deps-all-platforms branch April 11, 2026 14:41
byapparov pushed a commit that referenced this pull request Apr 11, 2026
First release that picks up the platform binary optionalDependencies
fix from #46 — 0.3.0 and 0.3.1 both shipped pointing at the stale
0.2.0 native binary and need to be superseded.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@byapparov byapparov mentioned this pull request Apr 11, 2026
5 tasks
byapparov added a commit that referenced this pull request Apr 11, 2026
First release that picks up the platform binary optionalDependencies
fix from #46 — 0.3.0 and 0.3.1 both shipped pointing at the stale
@aictrl/cli-linux-x64@0.2.0 native binary and need to be superseded.

Refreshes bun.lock and adds CHANGELOG entry.
byapparov pushed a commit that referenced this pull request Apr 11, 2026
The publish workflow has been silently failing since v0.3.1 (2026-04-03)
with MODULE_NOT_FOUND: promise-retry during `npm install -g npm@latest`.
Root cause: the self-upgrade path on the runner's prebuilt npm tree
leaves @npmcli/arborist's dependency graph half-replaced and the new
binary crashes on first invocation.

Symptoms confirmed today while trying to publish v0.3.2 — both v0.3.1
and v0.3.2 tags exist on GitHub but neither version is actually on npm;
`npm view @aictrl/cli versions` tops out at 0.3.0. This is why the
optionalDependencies pinning fix from #46 hadn't reached users.

Switching to corepack sidesteps the failure mechanism entirely: corepack
ships with Node 22, installs to its own shim directory, and doesn't
touch the bundled npm tree. Pinning to 11.5.2 (minimum OIDC trusted
publishing support + 2 patch releases to shake out regressions) instead
of tracking a moving `@latest` target.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
byapparov added a commit that referenced this pull request Apr 11, 2026
The publish workflow has been silently failing since v0.3.1 with
MODULE_NOT_FOUND: promise-retry during `npm install -g npm@latest`.
Root cause: the self-upgrade path on the runner's prebuilt npm tree
leaves @npmcli/arborist's dependency graph half-replaced.

Discovered while releasing v0.3.2 — both v0.3.1 and v0.3.2 tags exist
on GitHub but neither version is actually on npm; `npm view @aictrl/cli
versions` tops out at 0.3.0. This is why the optionalDependencies fix
from #46 hadn't reached users.

Switch to `corepack prepare npm@11.5.2 --activate`: corepack installs
to its own shim directory and never touches the bundled npm tree, so
the self-upgrade corruption path is unreachable. Pinning to 11.5.2
(minimum OIDC trusted publishing support + 2 patch releases) instead
of tracking a moving `@latest` target.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant