Skip to content

fix(ci): release smoke resolves every workspace package - #28709

Open
only21mil wants to merge 3 commits into
pingdotgg:mainfrom
only21mil:fix/audit-m079
Open

only21mil wants to merge 3 commits into
pingdotgg:mainfrom
only21mil:fix/audit-m079

Conversation

@only21mil

@only21mil only21mil commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Release Smoke checks lockfile resolution by copying the repo's manifests into a temp folder, deleting the lockfile and running vp install --lockfile-only. It copied a hand-kept list of manifests, and that list has drifted. evals and infra/task-cache are workspace packages with importers in pnpm-lock.yaml, but the smoke never copied them, so it never resolved them, and it still passed. Every new package also needs a second edit to that list, which is why #14693, #18694 and #24180 each add a line to it.

Change

The smoke now builds its fixture the way check:changed already does for its unused-patch check. Workspace manifests come from readWorkspace(), and mobile's file: module manifests come from apps/mobile/modules. The root manifest, workspace config, lockfile, apps/mobile/deps and patches are copied as before.

The smoke also compares the regenerated lockfile with the committed one and fails if any committed importer is missing. That uses pnpm's own output as the reference, so it also catches a workspace pattern readWorkspace() can't read, such as **.

Release behavior doesn't change. Only server, desktop, web and contracts get version bumps, and the updater manifest steps are untouched.

Scope and approval

This is a small, focused fix to a CI script, so it qualifies without a prior issue. #23988 lists it as an out-of-scope follow-up: "release-smoke's hard-coded workspace file list can drift from readWorkspace()."

#14693 and #33093 landed with a line each in the list. The merges with main dropped those lines, because discovery already includes both packages. #18694 and #24180 would drop their line after this lands. #23700 edits a different part of the same file.

Verification

Before, on main 0a9abddaee: the smoke passes but resolves only 40 of the lockfile's 42 importers. evals and infra/task-cache are missing.

Release smoke on main: Scope: all 40 workspace projects, checks passed

After, on this branch merged with the same main: all 42 resolve.

Release smoke on this branch: Scope: all 42 workspace projects, checks passed

All runs used Node 26.12.1 and pnpm 12.14.0, the versions package.json pins, on Linux.

  • Base plus only the new importer check, node scripts/release-smoke.ts: fails with Release smoke did not resolve workspace packages: evals, infra/task-cache.
  • This branch: Release smoke checks passed. All 42 committed importers resolve on main 0a9abddaee, and the four release version targets are still bumped to 9.9.9-smoke.0.
  • New package: a temporary packages/release-smoke-probe with its lockfile importer, then removed. This branch passes and resolves it with no list edit. Base plus the check fails, naming evals, infra/task-cache, packages/release-smoke-probe.
  • vp run check:changed 82b10d546c1041d8eff40425f076d61f608d518f: format, lint, knip and typecheck pass.

Not checked:

  • The CI runner. Locally, pnpm resolved from cached registry metadata.
  • .pnpmfile.cjs. The smoke didn't copy it before and still doesn't; check:changed does. I left it out to keep this to one problem.
  • Native Windows or macOS. The smoke's updater-manifest steps only use YAML fixtures, and this change doesn't touch them.
  • UI. This is a CI script with no screen.

Prior PRs

Generated with Claude Opus 5.5 and GPT-6.1 Terra and GPT-6.1 Sol in T3 Code Nightly

Release smoke copied a hand-kept list of manifests into its lockfile
fixture. The list had drifted: evals and infra/task-cache were never
resolved, and each new package needed a second edit.

Build the fixture the way check:changed does: workspace manifests from
readWorkspace() and mobile's file: modules from their directory. The
smoke now fails when the regenerated lockfile lacks an importer the
committed lockfile has.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 1, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 39f99d5

Macroscope's review found this PR approvable — The release smoke now discovers workspace manifests automatically and checks that no committed lockfile importer is omitted. Changes are confined to temporary test fixtures and validation, without altering product defaults or release publishing.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

Walkthrough

The release smoke script now discovers workspace manifests dynamically. After it regenerates a temporary lockfile, it checks that the lockfile contains every importer from the repository lockfile.

Changes: Release smoke checks

Layer / File(s) Summary
Discover workspace manifests
scripts/release-smoke.ts
The fixture manifest list now comes from workspace package directories and existing mobile module manifests instead of a fixed list.
Check regenerated lockfile importers
scripts/release-smoke.ts
A helper reads importer names from the lockfile’s top-level importers section. The smoke test reports repository importers missing from the regenerated lockfile.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: maria-rcks

Merge Risk: Minimal · up to f0126

The mobile modules directory is required by the release-smoke fixture. No actionable merge-blocking risk is established; normal checks remain appropriate.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly identifies the CI fix: the release smoke check now resolves every workspace package.
Description check Passed The description includes all required sections. It explains the problem and change, justifies the scope, and provides focused verification results and limitations.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

@maria-rcks

maria-rcks commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Merged current main (71646e031a) into the existing contributor branch without rebasing. Resolved the release-smoke manifest-list conflict by keeping automatic workspace discovery, which includes the packages added on main. The diff remains one file, with 33 additions and 49 deletions.

Verified merged head 536df8eb222b46c6f7c3c4f4c929ce02833f06ee through blacksmith-check: vp run check:changed github/main passed format, lint, both knip checks, and the affected scripts typecheck. vp run release:smoke passed, including all six relay-state tests and lockfile importer coverage. Two independent read-only reviews approved this exact head with no actionable findings.

GitHub reports no conflicts. Fresh CI is running. This is a CI-script-only change; native updater execution and release publication remain unverified.

Main added packages/source-control-cursor-origin to the hand-kept
workspace list in scripts/release-smoke.ts. This branch replaces that
list with workspace discovery, which already includes the new package,
so the merge keeps discovery and drops the added line.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@only21mil

Copy link
Copy Markdown
Contributor Author

Note

🤖 Opus 5.5 replying on behalf of Victor.

Merged main to clear the conflict in f012686e2b. Main #33093 added packages/source-control-cursor-origin/package.json to the hand-kept list this PR replaces. The merge drops that line, and readWorkspace() already includes the package through packages/*. Nothing else changed: the diff against main is still scripts/release-smoke.ts only.

On the merged head, vp run release:smoke passes and resolves all 42 committed importers. vp run check:changed passes. The description now has before and after captures of the smoke output. With main's list and only the importer check, the smoke still fails on evals, infra/task-cache. No UI: this is a CI script.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments: 1

scripts/release-smoke.ts: 1

16-18: Trivial · Maintainability & Code Quality · Low value

Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Guard the mobile modules directory read.

NodeFS.readdirSync throws ENOENT if apps/mobile/modules does not exist. This includes a sparse checkout or a future rename. The fixture setup then fails with an unrelated error. The other optional paths in this function use existsSync guards, for example apps/mobile/deps and patches. Use the same guard here.

Review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 536df8e and f012686.

Files selected for processing: 1

  • scripts/release-smoke.ts

Limit details: You’ve used all 10 included reviews currently available.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants