From e3041f54814a85b3b605d79467ddb1a570c32f56 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 6 Nov 2026 10:07:57 -0800 Subject: [PATCH] fix(ci): let the lockfile bot own the sandbox provider lockfiles (#19522) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The repository has a supply-chain rule: contributors do not commit lockfiles. A bot and verified Dependabot commits write them. > - The rule checks only the root `pnpm-lock.yaml`. > - The sandbox providers are outside the root workspace, so they have two more pnpm lockfiles. Pull requests edited them by hand, for example #12791, #18839 and #19489. > - This pull request makes the same rule and the same bot cover those two lockfiles. > - The benefit is one lockfile rule for every managed pnpm lockfile, and no lockfile edits in contributor pull requests. ## Linked Issues or Issue Description Refs #19489 **What existing behavior does this improve?** The lockfile policy and the lockfile refresh bot. **Subsystem affected** CI: `.github/scripts/lockfile-trust.mjs`, `.github/workflows/refresh-lockfile.yml`, and the sandbox provider installs. **Current behavior** `lockfile-trust.mjs` blocks contributor edits to `pnpm-lock.yaml` only. A contributor can edit `scripts/typecheck-sandbox-providers/pnpm-lock.yaml` or `packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml`, and CI passes. The refresh bot does not update these files. `typecheck:test-programs` fails on a stale provider lockfile and tells the contributor to commit a new one. **Proposed behavior** The policy blocks contributor edits to all three lockfiles. The refresh bot regenerates all three after merge. CI resolves a stale provider lockfile in place for the run, as it does for the root lockfile. **Reason and benefit** Lockfile content comes only from the bot or from verified Dependabot commits. Contributors do not need to run pnpm in special folders. **Breaking changes** None. A pull request that edits a provider lockfile now fails the policy check. Remove the lockfile change from the pull request. The bot commits it after merge. ## What Changed - `lockfile-trust.mjs` now exports `LOCKFILES`: the root lockfile, the shared sandbox provider lockfile, and the Daytona lockfile. The policy blocks contributor edits to each of them. A Dependabot pull request must match a verified Dependabot commit for each lockfile that it changes. The `merged` command and the reporting-lock proof still use only the root lockfile. - `check-pr-lockfile.mjs` (advisory check) uses the same list. - `refresh-lockfile.yml`: - A job-level `LOCKFILES` env lists the same three files. A test checks that it equals the policy list. - A new step regenerates the two provider lockfiles with `--resolution-only --ignore-scripts --no-frozen-lockfile`. - The unexpected-changes check, the change check, and `git add` cover all three lockfiles. - The release-age gate checks the added versions in every lockfile. - A held refresh saves all three lockfiles at their repository paths, with a `source-locks.sha256` file. The next run reuses them only if every master lockfile is unchanged. An artifact in the old format is not reused. - The PR title is now `chore(lockfile): refresh pnpm lockfiles`. - `check-lockfile-release-age.mjs` accepts more than one `--base`/`--head` pair. - `scripts/typecheck-test-programs.mjs`: if the frozen install of the provider workspace fails, the script resolves the lockfile in place and installs frozen again. - `scripts/build-standalone-public-packages.mjs`: the same fallback applies to a package with a committed lockfile (Daytona). `release.sh` already restores tracked files when it exits. - `doc/DEVELOPING.md` and `doc/RELEASE-AUTOMATION-SETUP.md` describe the managed lockfiles. They also say that the Pi and Kimi distribution lockfiles stay hand-managed, because they hold attested provider pins. ## Verification - `node --test .github/scripts/tests/*.test.mjs scripts/__tests__/build-standalone-concurrency.test.mjs`: 731 tests pass. New tests cover: - the policy for provider lockfiles, including a real git history with a Dependabot commit and a later hand edit - the workflow and policy lists match - the unexpected-changes check, run with bash against a fixture - a held refresh round trip, and its invalidation when a provider lockfile changes on master - the release-age gate across lockfile pairs - the inline fallback in both install scripts - `node --test scripts/typecheck-test-programs.test.mjs`: the install tests pass. Two tests fail locally only because this worktree has no root `node_modules` (no `tsc`). They also fail without this change. - I ran the new bot commands on master with pnpm 9.15.4. No package version changed. pnpm 9.15.4 removes `libc:` fields and adds `deprecated:` notes, so the first refresh is a metadata-only change. The release-age gate passes it because it adds no versions. - I ran the release-age and held-lock steps locally with bash against this commit. The gate reported `eligible=true`, and `sha256sum --check` accepted the saved hashes. - The workflow YAML parses, and `bash -n` accepts every `run` block. - Not testable before merge: the policy job loads `lockfile-trust.mjs` from the base branch, so the new rule applies only to pull requests opened after this merges. The refresh workflow runs only on master. ## Risks - The release build now resolves a stale Daytona lockfile in place, from the registry. The root release install already resolves without a frozen lockfile, so this is the same level of trust. - The paid runner e2e job still installs Daytona with `--frozen-lockfile`. After a Daytona manifest change merges, its Daytona cases fail until the bot's refresh merges. This matches the rule that paid jobs refuse a new root lockfile until its refresh merges. - Dependabot pull requests for the provider entries in #19489 change a provider lockfile. They pass only if each lockfile matches a verified Dependabot commit, the same as for the root lockfile. - Low risk otherwise: the bot keeps its existing token, environment, and merge rules. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`) in Claude Code, with tool use (shell, file edits, and git). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 --- .../scripts/check-lockfile-release-age.mjs | 30 +++++-- .github/scripts/check-pr-lockfile.mjs | 10 +-- .github/scripts/lockfile-trust.mjs | 51 ++++++++---- .../tests/check-lockfile-release-age.test.mjs | 29 +++++++ .../scripts/tests/check-pr-lockfile.test.mjs | 13 +++ .../tests/lockfile-refresh-merge.test.mjs | 83 +++++++++++++++++-- .github/scripts/tests/lockfile-trust.test.mjs | 79 ++++++++++++++++-- .github/workflows/refresh-lockfile.yml | 79 +++++++++++++----- doc/DEVELOPING.md | 37 ++++++--- doc/RELEASE-AUTOMATION-SETUP.md | 4 +- .../build-standalone-concurrency.test.mjs | 35 ++++++++ scripts/build-standalone-public-packages.mjs | 41 ++++++--- scripts/typecheck-test-programs.mjs | 18 +++- scripts/typecheck-test-programs.test.mjs | 25 +++++- 14 files changed, 443 insertions(+), 91 deletions(-) diff --git a/.github/scripts/check-lockfile-release-age.mjs b/.github/scripts/check-lockfile-release-age.mjs index f5338250687..ac718d922ea 100644 --- a/.github/scripts/check-lockfile-release-age.mjs +++ b/.github/scripts/check-lockfile-release-age.mjs @@ -1,16 +1,19 @@ #!/usr/bin/env node /** * check-lockfile-release-age.mjs - * Decides whether the lockfile bot may merge a refreshed pnpm-lock.yaml without review. + * Decides whether the lockfile bot may merge refreshed pnpm lockfiles without review. * Every package version that the refresh adds must be published on npm for at least * the minimum age. A version that is too new, or that cannot be verified, holds the * PR for a human. * * Exports: parseLockfilePackages(text), addedPackageVersions(baseText, headText), + * addedPackageVersionsAcross(pairs), * checkReleaseAge(entries, { fetchPublishTime, now, minAgeMs, concurrency }) * - * CLI: node check-lockfile-release-age.mjs --base --head --min-age-days - * Writes `eligible=true|false` to GITHUB_OUTPUT and a Markdown report to --report. + * CLI: node check-lockfile-release-age.mjs --base --head [--base --head ...] + * --min-age-days + * Pass one --base/--head pair per lockfile, in the same order. Writes `eligible=true|false` + * to GITHUB_OUTPUT and a Markdown report to --report. */ import { appendFileSync, readFileSync, writeFileSync } from 'node:fs'; import { fileURLToPath } from 'node:url'; @@ -65,6 +68,15 @@ export function addedPackageVersions(baseText, headText) { .map(([key, resolution]) => ({ key, ...splitPackageKey(key), resolution })); } +/** Added versions across several base/head lockfile pairs, each listed once. */ +export function addedPackageVersionsAcross(pairs) { + const entries = new Map(); + for (const [baseText, headText] of pairs) { + for (const entry of addedPackageVersions(baseText, headText)) entries.set(`${entry.key}\0${entry.resolution}`, entry); + } + return [...entries.values()].sort((a, b) => a.key.localeCompare(b.key)); +} + export async function fetchNpmPublishTime(name, version, fetchImpl = fetch) { const res = await fetchImpl(`https://registry.npmjs.org/${name.replace('/', '%2f')}`, { headers: { Accept: 'application/json' }, @@ -135,19 +147,21 @@ export function formatReport(result, minAgeDays) { async function main() { const { values } = parseArgs({ options: { - base: { type: 'string' }, - head: { type: 'string' }, + base: { type: 'string', multiple: true }, + head: { type: 'string', multiple: true }, 'min-age-days': { type: 'string' }, report: { type: 'string' }, }, }); const minAgeDays = Number(values['min-age-days']); - if (!values.base || !values.head || !Number.isFinite(minAgeDays) || minAgeDays < 0) { - console.error('Usage: check-lockfile-release-age.mjs --base --head --min-age-days [--report ]'); + const bases = values.base ?? []; + const heads = values.head ?? []; + if (bases.length === 0 || bases.length !== heads.length || !Number.isFinite(minAgeDays) || minAgeDays < 0) { + console.error('Usage: check-lockfile-release-age.mjs --base --head [--base --head ...] --min-age-days [--report ]'); process.exit(2); } - const entries = addedPackageVersions(readFileSync(values.base, 'utf8'), readFileSync(values.head, 'utf8')); + const entries = addedPackageVersionsAcross(bases.map((base, index) => [readFileSync(base, 'utf8'), readFileSync(heads[index], 'utf8')])); const result = await checkReleaseAge(entries, { minAgeMs: minAgeDays * DAY_MS }); const report = formatReport(result, minAgeDays); console.log(report); diff --git a/.github/scripts/check-pr-lockfile.mjs b/.github/scripts/check-pr-lockfile.mjs index ea3e80ea0c6..b9e48d18df5 100644 --- a/.github/scripts/check-pr-lockfile.mjs +++ b/.github/scripts/check-pr-lockfile.mjs @@ -6,24 +6,24 @@ * Export: checkLockfile(files, prAuthor, prBranch) → { passed, failures } */ import { fileURLToPath } from 'node:url'; -import { DEPENDABOT_LOGIN, LOCKFILE, REFRESH_BRANCH, REFRESH_PR_AUTHOR, REPORT_LOCK_FILES } from './lockfile-trust.mjs'; +import { DEPENDABOT_LOGIN, LOCKFILES, REFRESH_BRANCH, REFRESH_PR_AUTHOR, REPORT_LOCK_FILES } from './lockfile-trust.mjs'; export function checkLockfile(files, prAuthor, prBranch) { const changed = files.map(f => f.filename); - const lockfileChanged = changed.includes(LOCKFILE); + const lockfilesChanged = changed.filter(f => LOCKFILES.includes(f)); const proofChanged = changed.filter(f => REPORT_LOCK_FILES.includes(f)); - if (!lockfileChanged && proofChanged.length === 0) return { passed: true, failures: [] }; + if (lockfilesChanged.length === 0 && proofChanged.length === 0) return { passed: true, failures: [] }; if (prAuthor === REFRESH_PR_AUTHOR && prBranch === REFRESH_BRANCH) return { passed: true, failures: [] }; if (prAuthor === DEPENDABOT_LOGIN && proofChanged.length === 0) return { passed: true, failures: [] }; - const edited = [lockfileChanged && LOCKFILE, ...proofChanged].filter(Boolean).map(f => `\`${f}\``).join(', '); + const edited = [...lockfilesChanged, ...proofChanged].map(f => `\`${f}\``).join(', '); return { passed: false, failures: [ `You have changes to ${edited} — \`pr.yml\` will hard-fail this PR because only the lockfile refresh workflow and Dependabot may change lock content. ` + 'To fix: run `pnpm install` locally, exclude the lockfile from your commit, push again. ' + - 'The lockfile is regenerated automatically by the refresh bot on a schedule.', + 'The refresh bot regenerates every managed lockfile after your PR merges.', ], }; } diff --git a/.github/scripts/lockfile-trust.mjs b/.github/scripts/lockfile-trust.mjs index 3ac0701ea34..63076d614cb 100644 --- a/.github/scripts/lockfile-trust.mjs +++ b/.github/scripts/lockfile-trust.mjs @@ -2,9 +2,9 @@ /** * lockfile-trust.mjs * - * Decides who may change the dependency lock. Only trusted environments write it: + * Decides who may change the dependency locks (LOCKFILES). Only trusted environments write them: * - the lockfile refresh workflow (`chore/refresh-lockfile`, opened by github-actions[bot]), - * - Dependabot, when the exact lockfile bytes come from a verified Dependabot commit. + * - Dependabot, when the exact bytes of each lockfile it changes come from a verified Dependabot commit. * * The reporting-lock proof (`tests/runner-e2e/report-lock.json` and `.patch`) can rewrite the * lock that paid evaluation jobs install, so it counts as lock content too. Only the refresh @@ -24,7 +24,16 @@ import { appendFileSync, realpathSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; +// The root lockfile. The reporting-lock proof and the `merged` command follow this one only. export const LOCKFILE = 'pnpm-lock.yaml'; +// Sandbox providers stay out of the root workspace (see doc/DEVELOPING.md), so they have their own +// pnpm lockfiles. The refresh workflow regenerates these too, and its LOCKFILES env lists the same +// paths. Hand-managed pins, such as the Pi and Kimi distributions, are not listed. +export const PROVIDER_LOCKFILES = [ + 'scripts/typecheck-sandbox-providers/pnpm-lock.yaml', + 'packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml', +]; +export const LOCKFILES = [LOCKFILE, ...PROVIDER_LOCKFILES]; export const REPORT_LOCK_FILES = ['tests/runner-e2e/report-lock.json', 'tests/runner-e2e/report-lock.patch']; export const DEPENDABOT_LOGIN = 'dependabot[bot]'; export const REFRESH_BRANCH = 'chore/refresh-lockfile'; @@ -61,12 +70,14 @@ export function findDependabotLockCommit({ lockBlob, commits, lockBlobAt }) { /** * Classify the lock-content change in a pull request. + * `dependabotLockCommits` maps each changed lockfile to the verified Dependabot commit that holds + * its exact bytes, or to null. * @returns {{ allowed: boolean, writer: 'none' | 'dependabot' | 'refresh-bot', reason: string }} */ -export function classifyPullRequest({ changedPaths, author, headRepo, baseRepo, headRef, dependabotLockCommit }) { - const lockfileChanged = changedPaths.includes(LOCKFILE); +export function classifyPullRequest({ changedPaths, author, headRepo, baseRepo, headRef, dependabotLockCommits = {} }) { + const changedLockfiles = LOCKFILES.filter((file) => changedPaths.includes(file)); const proofChanged = changedPaths.some((file) => REPORT_LOCK_FILES.includes(file)); - if (!lockfileChanged && !proofChanged) { + if (changedLockfiles.length === 0 && !proofChanged) { return { allowed: true, writer: 'none', reason: 'The pull request does not change lock content.' }; } @@ -80,14 +91,16 @@ export function classifyPullRequest({ changedPaths, author, headRepo, baseRepo, return { allowed: false, writer: 'dependabot', reason: `Dependabot pull requests must not change ${REPORT_LOCK_FILES.join(' or ')}. The lockfile refresh renews the reporting-lock proof after merge.` }; } - if (!dependabotLockCommit) { + const unverified = changedLockfiles.filter((file) => !dependabotLockCommits[file]); + if (unverified.length > 0) { return { allowed: false, writer: 'dependabot', - reason: `${LOCKFILE} does not match any verified Dependabot commit in this pull request. ${RECREATE_HINT}` }; + reason: `${unverified.join(', ')} does not match any verified Dependabot commit in this pull request. ${RECREATE_HINT}` }; } - return { allowed: true, writer: 'dependabot', reason: `${LOCKFILE} matches verified Dependabot commit ${dependabotLockCommit}.` }; + return { allowed: true, writer: 'dependabot', + reason: changedLockfiles.map((file) => `${file} matches verified Dependabot commit ${dependabotLockCommits[file]}.`).join(' ') }; } - const files = [lockfileChanged && LOCKFILE, ...REPORT_LOCK_FILES.filter((file) => changedPaths.includes(file))].filter(Boolean); + const files = [...changedLockfiles, ...REPORT_LOCK_FILES.filter((file) => changedPaths.includes(file))]; return { allowed: false, writer: 'none', reason: `Do not commit ${files.join(', ')} in pull requests. CI owns lockfile updates.` }; } @@ -146,18 +159,22 @@ function checkPullRequest() { // Diff against the merge base so recent base-branch commits are not counted as PR changes. const changedPaths = run('git', ['diff', '--name-only', `${baseSha}...${headSha}`]).split('\n').filter(Boolean); - let dependabotLockCommit = null; - if (author === DEPENDABOT_LOGIN && changedPaths.includes(LOCKFILE)) { - dependabotLockCommit = findDependabotLockCommit({ - lockBlob: tryRun('git', ['rev-parse', `${headSha}:${LOCKFILE}`]), - commits: pullCommits(repository, required('PR_NUMBER')), - lockBlobAt: (sha) => tryRun('git', ['rev-parse', `${sha}:${LOCKFILE}`]), - }); + const dependabotLockCommits = {}; + const changedLockfiles = LOCKFILES.filter((file) => changedPaths.includes(file)); + if (author === DEPENDABOT_LOGIN && changedLockfiles.length > 0) { + const commits = pullCommits(repository, required('PR_NUMBER')); + for (const file of changedLockfiles) { + dependabotLockCommits[file] = findDependabotLockCommit({ + lockBlob: tryRun('git', ['rev-parse', `${headSha}:${file}`]), + commits, + lockBlobAt: (sha) => tryRun('git', ['rev-parse', `${sha}:${file}`]), + }); + } } const result = classifyPullRequest({ changedPaths, author, headRepo: process.env.HEAD_REPO ?? '', baseRepo: repository, - headRef: process.env.HEAD_REF ?? '', dependabotLockCommit, + headRef: process.env.HEAD_REF ?? '', dependabotLockCommits, }); console.log(result.reason); writeOutputs({ writer: result.writer }); diff --git a/.github/scripts/tests/check-lockfile-release-age.test.mjs b/.github/scripts/tests/check-lockfile-release-age.test.mjs index a5cdaebc607..0a79599c237 100644 --- a/.github/scripts/tests/check-lockfile-release-age.test.mjs +++ b/.github/scripts/tests/check-lockfile-release-age.test.mjs @@ -1,7 +1,12 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; import { addedPackageVersions, + addedPackageVersionsAcross, checkReleaseAge, formatReport, parseLockfilePackages, @@ -66,6 +71,30 @@ test('lists only package versions the refresh adds', () => { ]); }); +test('lists the versions each lockfile adds, once', () => { + const pairs = [ + [lockfile(['left-pad@1.3.0']), lockfile(['left-pad@1.3.0', 'shared@2.0.0', 'root-only@1.0.0'])], + [lockfile([]), lockfile(['shared@2.0.0', 'provider-only@3.0.0'])], + ]; + assert.deepEqual(addedPackageVersionsAcross(pairs).map(entry => entry.key), ['provider-only@3.0.0', 'root-only@1.0.0', 'shared@2.0.0']); +}); + +test('the CLI rejects unpaired lockfiles before any registry lookup', () => { + const root = mkdtempSync(path.join(tmpdir(), 'release-age-cli-')); + try { + const file = path.join(root, 'pnpm-lock.yaml'); + writeFileSync(file, lockfile([])); + const result = spawnSync(process.execPath, [ + path.resolve('.github/scripts/check-lockfile-release-age.mjs'), + '--base', file, '--head', file, '--base', file, '--min-age-days', '3', + ], { encoding: 'utf8' }); + assert.equal(result.status, 2, result.stderr); + assert.match(result.stderr, /Usage/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test('lists an existing version whose resolution changed', () => { const swapped = ['left-pad@1.3.0', '{tarball: https://example.com/left-pad-1.3.0.tgz}']; assert.deepEqual(addedPackageVersions(lockfile(['left-pad@1.3.0']), lockfile([swapped])), [ diff --git a/.github/scripts/tests/check-pr-lockfile.test.mjs b/.github/scripts/tests/check-pr-lockfile.test.mjs index 4436ebd176d..aa3fed1a5a7 100644 --- a/.github/scripts/tests/check-pr-lockfile.test.mjs +++ b/.github/scripts/tests/check-pr-lockfile.test.mjs @@ -47,3 +47,16 @@ test('fails when a regular user changes the reporting-lock proof', () => { assert.equal(result.passed, false); assert.ok(result.failures[0].includes('report-lock.patch')); }); + +test('fails when a regular user changes a sandbox provider lockfile', () => { + for (const file of ['scripts/typecheck-sandbox-providers/pnpm-lock.yaml', 'packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml']) { + const result = checkLockfile(makeFiles([file]), 'someuser', 'fix/bug'); + assert.equal(result.passed, false, file); + assert.ok(result.failures[0].includes(file)); + } +}); + +test('passes when Dependabot changes a sandbox provider lockfile', () => { + const files = makeFiles(['packages/plugins/sandbox-providers/daytona/package.json', 'packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml']); + assert.equal(checkLockfile(files, 'dependabot[bot]', 'dependabot/npm_and_yarn/packages/plugins/sandbox-providers/daytona/zod-4.6.6').passed, true); +}); diff --git a/.github/scripts/tests/lockfile-refresh-merge.test.mjs b/.github/scripts/tests/lockfile-refresh-merge.test.mjs index 54406e450b6..3001529c5df 100644 --- a/.github/scripts/tests/lockfile-refresh-merge.test.mjs +++ b/.github/scripts/tests/lockfile-refresh-merge.test.mjs @@ -1,9 +1,10 @@ import test from "node:test"; import assert from "node:assert/strict"; import { execFileSync, spawnSync } from "node:child_process"; -import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; +import { LOCKFILES } from "../lockfile-trust.mjs"; const workflow = readFileSync(new URL("../../workflows/refresh-lockfile.yml", import.meta.url), "utf8"); const stepNames = [...workflow.matchAll(/^ - name: (.+)$/gm)].map((match) => match[1]); @@ -46,17 +47,18 @@ test("a held refresh keeps its versions through the next master run's artifact", assert.match(reuse, /gh run download "\$run_id" --name held-pnpm-lock/); assert.match( reuse, - /if \[ "\$\(sha256sum < pnpm-lock\.yaml\)" != "\$\(cat "\$held\/source-lock\.sha256"\)" \]; then[\s\S]*?exit 0\n fi\n cp "\$held\/pnpm-lock\.yaml" pnpm-lock\.yaml/, - "a held lock must only be reused while master still has the lock it was resolved from", + /if ! sha256sum --check --status "\$held\/source-locks\.sha256"; then[\s\S]*?exit 0\n fi\n for lock in \$LOCKFILES; do\n cp "\$held\/\$lock" "\$lock"/, + "held locks must only be reused while master still has every lock they were resolved from", ); + assert.match(reuse, /if \[ ! -f "\$held\/source-locks\.sha256" \]; then[\s\S]*?exit 0/, "an older held artifact must not be reused"); assert.doesNotMatch(reuse, /chore\/refresh-lockfile/, "the PR branch is writable by others and must not seed resolution"); for (const name of ["Stage held lockfile", "Save held lockfile for the next refresh"]) { assert.equal(condition(name), "steps.release-age.outputs.eligible == 'false'"); } const stage = step("Stage held lockfile"); - assert.match(stage, /cp pnpm-lock\.yaml "\$RUNNER_TEMP\/held-upload\/pnpm-lock\.yaml"/); - assert.match(stage, /sha256sum < "\$RUNNER_TEMP\/base-pnpm-lock\.yaml" > "\$RUNNER_TEMP\/held-upload\/source-lock\.sha256"/); + assert.match(stage, /for lock in \$LOCKFILES; do[\s\S]*?cp "\$lock" "\$held\/\$lock"/); + assert.match(stage, /\(cd "\$RUNNER_TEMP\/base-locks" && sha256sum \$LOCKFILES\) > "\$held\/source-locks\.sha256"/); const save = step("Save held lockfile for the next refresh"); assert.match(save, /name: held-pnpm-lock/); assert.match(save, /path: \$\{\{ runner\.temp \}\}\/held-upload\//); @@ -95,3 +97,74 @@ test("a refresh that renews only the reporting-lock proof still opens a pull req rmSync(root, { recursive: true, force: true }); } }); + +test("the workflow owns the same lockfiles that the trust policy guards", () => { + const env = workflow.match(/^ LOCKFILES: (.+)$/m)?.[1]; + assert.ok(env, "the refresh job must define LOCKFILES"); + assert.deepEqual(env.trim().split(/\s+/), LOCKFILES); + assert.match(step("Refresh sandbox provider lockfiles"), + /pnpm --dir scripts\/typecheck-sandbox-providers install --resolution-only --ignore-scripts --no-frozen-lockfile/); + assert.match(step("Refresh sandbox provider lockfiles"), + /pnpm --dir packages\/plugins\/sandbox-providers\/daytona install --ignore-workspace --resolution-only --ignore-scripts --no-frozen-lockfile/); + assert.ok(stepNames.indexOf("Refresh sandbox provider lockfiles") < stepNames.indexOf("Fail on unexpected file changes")); + assert.match(step("Check release age of added package versions"), /for lock in \$LOCKFILES; do[\s\S]*?pairs\+=\(--base "\$base\/\$lock" --head "\$lock"\)/); +}); + +// Run a workflow step's script with bash, as GitHub would. +const runStep = (name, cwd, env) => { + const lines = step(name).split("\n"); + const start = lines.findIndex((line) => line === " run: |") + 1; + const script = lines.slice(start).map((line) => line.slice(10)).join("\n"); + return spawnSync("bash", ["-e", "-c", script], { cwd, encoding: "utf8", env: { ...process.env, LOCKFILES: LOCKFILES.join(" "), ...env } }); +}; + +test("unexpected-change check admits every managed lockfile and nothing else", () => { + const root = mkdtempSync(path.join(tmpdir(), "refresh-unexpected-")); + try { + const git = (...args) => execFileSync("git", args, { cwd: root }); + for (const file of [...LOCKFILES, "notes.md"]) { + mkdirSync(path.dirname(path.join(root, file)), { recursive: true }); + writeFileSync(path.join(root, file), "old\n"); + } + git("init", "-q"); + git("add", "."); + git("-c", "user.name=Fixture", "-c", "user.email=fixture@example.invalid", "commit", "-qm", "fixture"); + for (const file of LOCKFILES) writeFileSync(path.join(root, file), "new\n"); + const allowed = runStep("Fail on unexpected file changes", root, {}); + assert.equal(allowed.status, 0, allowed.stdout + allowed.stderr); + writeFileSync(path.join(root, "notes.md"), "new\n"); + const blocked = runStep("Fail on unexpected file changes", root, {}); + assert.equal(blocked.status, 1, blocked.stdout + blocked.stderr); + assert.match(blocked.stdout, /notes\.md/); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +test("a held refresh round-trips every managed lockfile", () => { + const root = mkdtempSync(path.join(tmpdir(), "refresh-held-")); + const temp = mkdtempSync(path.join(tmpdir(), "refresh-held-temp-")); + try { + for (const file of LOCKFILES) { + for (const dir of [root, path.join(temp, "base-locks")]) { + mkdirSync(path.dirname(path.join(dir, file)), { recursive: true }); + writeFileSync(path.join(dir, file), `master ${file}\n`); + } + writeFileSync(path.join(root, file), `held ${file}\n`); + } + const staged = runStep("Stage held lockfile", root, { RUNNER_TEMP: temp }); + assert.equal(staged.status, 0, staged.stderr); + const held = path.join(temp, "held-upload"); + for (const file of LOCKFILES) assert.equal(readFileSync(path.join(held, file), "utf8"), `held ${file}\n`); + + // The next run checks the hashes against master's lockfiles, then copies the held ones in. + for (const file of LOCKFILES) writeFileSync(path.join(root, file), `master ${file}\n`); + const check = `sha256sum --check --status "${held}/source-locks.sha256"`; + assert.equal(spawnSync("bash", ["-c", check], { cwd: root }).status, 0, "unchanged master locks must match the held source hashes"); + writeFileSync(path.join(root, LOCKFILES[1]), "changed on master\n"); + assert.notEqual(spawnSync("bash", ["-c", check], { cwd: root }).status, 0, "a changed provider lock must invalidate the held refresh"); + } finally { + rmSync(root, { recursive: true, force: true }); + rmSync(temp, { recursive: true, force: true }); + } +}); diff --git a/.github/scripts/tests/lockfile-trust.test.mjs b/.github/scripts/tests/lockfile-trust.test.mjs index 026c6a7ffc8..f9fba08cbc5 100644 --- a/.github/scripts/tests/lockfile-trust.test.mjs +++ b/.github/scripts/tests/lockfile-trust.test.mjs @@ -1,7 +1,7 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { execFileSync, spawnSync } from 'node:child_process'; -import { chmodSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { @@ -10,10 +10,12 @@ import { findMergedDependabotPull, isDependabotCommit, isGitHubSignedCommit, + LOCKFILES, + PROVIDER_LOCKFILES, } from '../lockfile-trust.mjs'; const repo = 'paperclipai/paperclip'; -const base = { author: 'someuser', headRepo: repo, baseRepo: repo, headRef: 'fix/bug', dependabotLockCommit: null }; +const base = { author: 'someuser', headRepo: repo, baseRepo: repo, headRef: 'fix/bug', dependabotLockCommits: {} }; test('allows a pull request that does not change lock content', () => { assert.deepEqual( @@ -56,7 +58,7 @@ test('allows the refresh workflow to change the lockfile and its proof', () => { test('allows a Dependabot lockfile that matches a verified Dependabot commit', () => { const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', headRef: 'dependabot/npm_and_yarn/zod-4.6.6', - changedPaths: ['package.json', 'pnpm-lock.yaml'], dependabotLockCommit: 'abc123' }); + changedPaths: ['package.json', 'pnpm-lock.yaml'], dependabotLockCommits: { 'pnpm-lock.yaml': 'abc123' } }); assert.equal(result.allowed, true); assert.equal(result.writer, 'dependabot'); }); @@ -68,17 +70,49 @@ test('blocks a Dependabot lockfile that no verified Dependabot commit produced', }); test('blocks a Dependabot pull request that changes the reporting-lock proof', () => { - const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', dependabotLockCommit: 'abc123', + const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', dependabotLockCommits: { 'pnpm-lock.yaml': 'abc123' }, changedPaths: ['pnpm-lock.yaml', 'tests/runner-e2e/report-lock.json'] }); assert.equal(result.allowed, false); }); test('blocks a Dependabot author from a fork', () => { const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', headRepo: 'someone/paperclip', - dependabotLockCommit: 'abc123', changedPaths: ['pnpm-lock.yaml'] }); + dependabotLockCommits: { 'pnpm-lock.yaml': 'abc123' }, changedPaths: ['pnpm-lock.yaml'] }); assert.equal(result.allowed, false); }); +test('guards the sandbox provider lockfiles as well as the root lockfile', () => { + assert.deepEqual(LOCKFILES, ['pnpm-lock.yaml', ...PROVIDER_LOCKFILES]); + for (const file of PROVIDER_LOCKFILES) { + const result = classifyPullRequest({ ...base, changedPaths: [file] }); + assert.equal(result.allowed, false, file); + assert.ok(result.reason.includes(file), result.reason); + } +}); + +test('allows the refresh workflow to change the provider lockfiles', () => { + const result = classifyPullRequest({ ...base, author: 'github-actions[bot]', headRef: 'chore/refresh-lockfile', + changedPaths: LOCKFILES }); + assert.equal(result.allowed, true); + assert.equal(result.writer, 'refresh-bot'); +}); + +test('allows a Dependabot provider lockfile that matches a verified Dependabot commit', () => { + const [file] = PROVIDER_LOCKFILES; + const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', + changedPaths: ['packages/plugins/sandbox-providers/e2b/package.json', file], dependabotLockCommits: { [file]: 'abc123' } }); + assert.equal(result.allowed, true); + assert.equal(result.writer, 'dependabot'); +}); + +test('blocks a Dependabot pull request when any changed lockfile is unverified', () => { + const [file] = PROVIDER_LOCKFILES; + const result = classifyPullRequest({ ...base, author: 'dependabot[bot]', + changedPaths: ['pnpm-lock.yaml', file], dependabotLockCommits: { 'pnpm-lock.yaml': 'abc123', [file]: null } }); + assert.equal(result.allowed, false); + assert.ok(result.reason.startsWith(`${file} does not match`), result.reason); +}); + test('finds only verified Dependabot commits that hold the exact lockfile', () => { const blobs = { a: 'lock-1', b: 'lock-2', c: 'lock-2', d: 'lock-2' }; const lockBlobAt = (sha) => blobs[sha] ?? null; @@ -132,7 +166,10 @@ function runPrCheck(build) { try { const git = (...args) => execFileSync('git', args, { cwd: root, encoding: 'utf8' }).trim(); const commit = (author, files) => { - for (const [file, contents] of Object.entries(files)) writeFileSync(path.join(root, file), contents); + for (const [file, contents] of Object.entries(files)) { + mkdirSync(path.dirname(path.join(root, file)), { recursive: true }); + writeFileSync(path.join(root, file), contents); + } git('add', '.'); git('-c', `user.name=${author}`, '-c', `user.email=${author}@example.invalid`, 'commit', '-qm', author); return git('rev-parse', 'HEAD'); @@ -190,6 +227,36 @@ test('pr command blocks a lockfile commit whose Dependabot author another commit assert.match(result.stdout, /Untrusted lockfile change/); }); +test('pr command checks each lockfile a Dependabot pull request changes', () => { + const [file] = PROVIDER_LOCKFILES; + const admitted = runPrCheck((commit) => { + const sha = commit('dependabot', { [file]: 'lock: dependabot\n', 'package.json': '{"zod":"4.6.6"}\n' }); + return { author: 'dependabot[bot]', commits: [{ sha, login: 'dependabot[bot]', committer: 'web-flow', verified: true }] }; + }); + assert.equal(admitted.status, 0, admitted.stdout); + assert.match(admitted.output, /^writer=dependabot$/m); + + const edited = runPrCheck((commit) => { + const sha = commit('dependabot', { 'pnpm-lock.yaml': 'lock: dependabot\n', [file]: 'lock: dependabot\n' }); + const edit = commit('maintainer', { [file]: 'lock: edited\n' }); + return { author: 'dependabot[bot]', commits: [ + { sha, login: 'dependabot[bot]', committer: 'web-flow', verified: true }, + { sha: edit, login: 'maintainer', committer: 'web-flow', verified: true }, + ] }; + }); + assert.equal(edited.status, 1, edited.stdout); + assert.ok(edited.stdout.includes(`${file} does not match`), edited.stdout); +}); + +test('pr command blocks a contributor change to a provider lockfile', () => { + const result = runPrCheck((commit) => { + commit('someuser', { [PROVIDER_LOCKFILES[1]]: 'lock: contributor\n' }); + return { author: 'someuser', commits: [] }; + }); + assert.equal(result.status, 1); + assert.match(result.output, /^writer=none$/m); +}); + test('pr command blocks a contributor lockfile change', () => { const result = runPrCheck((commit) => { commit('someuser', { 'pnpm-lock.yaml': 'lock: contributor\n' }); diff --git a/.github/workflows/refresh-lockfile.yml b/.github/workflows/refresh-lockfile.yml index 4160536d3a3..87665452ab8 100644 --- a/.github/workflows/refresh-lockfile.yml +++ b/.github/workflows/refresh-lockfile.yml @@ -24,6 +24,12 @@ jobs: actions: read contents: write pull-requests: write + env: + # Every lockfile this workflow owns: the root one, then the sandbox + # providers' own lockfiles. Keep it equal to LOCKFILES in + # .github/scripts/lockfile-trust.mjs, which blocks contributor edits to + # the same files. A test checks that the two lists match. + LOCKFILES: pnpm-lock.yaml scripts/typecheck-sandbox-providers/pnpm-lock.yaml packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml steps: - name: Checkout repository @@ -43,9 +49,9 @@ jobs: # claim the shared cache key with an empty archive before full installs. package-manager-cache: false - # Start from the lockfile the previous run held, so resolution keeps those + # Start from the lockfiles the previous run held, so resolution keeps those # versions while their manifests are unchanged and they can age past the - # gate. Read it from that run's artifact, not the PR branch: anyone with + # gate. Read them from that run's artifact, not the PR branch: anyone with # write access can push to chore/refresh-lockfile. - name: Reuse versions from a held refresh env: @@ -61,19 +67,34 @@ jobs: echo "The previous refresh held nothing. Resolving from master." exit 0 fi - # Reuse the held lock only while master still has the lock it was - # resolved from. A lock change on master, such as a maintainer's - # correction, supersedes it. - if [ "$(sha256sum < pnpm-lock.yaml)" != "$(cat "$held/source-lock.sha256")" ]; then - echo "The lockfile on master changed after run $run_id held its refresh. Resolving from master." + # An artifact saved before the provider lockfiles has no + # source-locks.sha256. Resolve from master instead. + if [ ! -f "$held/source-locks.sha256" ]; then + echo "Run $run_id held its refresh in an older format. Resolving from master." exit 0 fi - cp "$held/pnpm-lock.yaml" pnpm-lock.yaml - echo "Resolving from the lockfile held by run $run_id." + # Reuse the held locks only while master still has every lock they + # were resolved from. A lock change on master, such as a maintainer's + # correction, supersedes them. + if ! sha256sum --check --status "$held/source-locks.sha256"; then + echo "A lockfile on master changed after run $run_id held its refresh. Resolving from master." + exit 0 + fi + for lock in $LOCKFILES; do + cp "$held/$lock" "$lock" + done + echo "Resolving from the lockfiles held by run $run_id." - name: Refresh pnpm lockfile run: pnpm install --resolution-only --ignore-scripts --no-frozen-lockfile + # Sandbox providers stay out of the root workspace. The shared typecheck + # workspace has its own pnpm-workspace.yaml, and Daytona installs alone. + - name: Refresh sandbox provider lockfiles + run: | + pnpm --dir scripts/typecheck-sandbox-providers install --resolution-only --ignore-scripts --no-frozen-lockfile + pnpm --dir packages/plugins/sandbox-providers/daytona install --ignore-workspace --resolution-only --ignore-scripts --no-frozen-lockfile + # A verified Dependabot lockfile merged to master is trusted lock content. # Report its hash so the proof refresh can admit it. - name: Identify a verified Dependabot lockfile on master @@ -99,7 +120,9 @@ jobs: echo "Lockfile is already up to date." exit 0 fi - unexpected="$(printf '%s\n' "$changed" | awk '$2 != "pnpm-lock.yaml" && $2 != "tests/runner-e2e/report-lock.json" && $2 != "tests/runner-e2e/report-lock.patch" { print }')" + unexpected="$(printf '%s\n' "$changed" | awk -v allowed="$LOCKFILES tests/runner-e2e/report-lock.json tests/runner-e2e/report-lock.patch" ' + BEGIN { n = split(allowed, files, " "); for (i = 1; i <= n; i++) ok[files[i]] = 1 } + !($2 in ok) { print }')" if [ -n "$unexpected" ]; then echo "Unexpected files changed during lockfile refresh:" echo "$changed" @@ -114,8 +137,8 @@ jobs: run: | # A verified Dependabot lockfile that already resolves changes only # the reporting-lock proof. Commit that proof too. - if git diff --quiet -- pnpm-lock.yaml tests/runner-e2e/report-lock.json tests/runner-e2e/report-lock.patch; then - echo "Lockfile and reporting-lock proof unchanged, nothing to do." + if git diff --quiet -- $LOCKFILES tests/runner-e2e/report-lock.json tests/runner-e2e/report-lock.patch; then + echo "Lockfiles and reporting-lock proof unchanged, nothing to do." echo "pr_url=" >> "$GITHUB_OUTPUT" echo "head_sha=" >> "$GITHUB_OUTPUT" exit 0 @@ -126,8 +149,8 @@ jobs: git config user.email "lockfile-bot@users.noreply.github.com" git checkout -B "$BRANCH" - git add pnpm-lock.yaml tests/runner-e2e/report-lock.json tests/runner-e2e/report-lock.patch - git commit -m "chore(lockfile): refresh pnpm-lock.yaml" + git add $LOCKFILES tests/runner-e2e/report-lock.json tests/runner-e2e/report-lock.patch + git commit -m "chore(lockfile): refresh pnpm lockfiles" git push --force origin "$BRANCH" # Only reuse an open PR from this repository owner, not a fork with the same branch name. @@ -164,6 +187,7 @@ jobs: ## What Changed - Refresh the dependency lock from the manifests on master. + - Refresh the sandbox provider lockfiles from their manifests on master. - Include the matching offline report correction and hashes. ## Verification @@ -188,7 +212,7 @@ jobs: PR_BODY pr_url="$(gh pr create \ --head "$BRANCH" \ - --title "chore(lockfile): refresh pnpm-lock.yaml" \ + --title "chore(lockfile): refresh pnpm lockfiles" \ --body-file "$pr_body_file")" echo "Created new PR: $pr_url" else @@ -204,10 +228,18 @@ jobs: id: release-age if: steps.upsert-pr.outputs.pr_url != '' run: | - git show "$GITHUB_SHA:pnpm-lock.yaml" > "$RUNNER_TEMP/base-pnpm-lock.yaml" + # Keep each base lockfile at its repository path. "Stage held + # lockfile" hashes them from there. + base="$RUNNER_TEMP/base-locks" + pairs=() + for lock in $LOCKFILES; do + mkdir -p "$base/$(dirname "$lock")" + # A lockfile that master does not have yet has no base, so all of its entries count as added. + git show "$GITHUB_SHA:$lock" > "$base/$lock" 2>/dev/null || : > "$base/$lock" + pairs+=(--base "$base/$lock" --head "$lock") + done node .github/scripts/check-lockfile-release-age.mjs \ - --base "$RUNNER_TEMP/base-pnpm-lock.yaml" \ - --head pnpm-lock.yaml \ + "${pairs[@]}" \ --min-age-days 3 \ --report "$RUNNER_TEMP/release-age-report.md" cat "$RUNNER_TEMP/release-age-report.md" >> "$GITHUB_STEP_SUMMARY" @@ -236,9 +268,14 @@ jobs: - name: Stage held lockfile if: steps.release-age.outputs.eligible == 'false' run: | - mkdir -p "$RUNNER_TEMP/held-upload" - cp pnpm-lock.yaml "$RUNNER_TEMP/held-upload/pnpm-lock.yaml" - sha256sum < "$RUNNER_TEMP/base-pnpm-lock.yaml" > "$RUNNER_TEMP/held-upload/source-lock.sha256" + held="$RUNNER_TEMP/held-upload" + for lock in $LOCKFILES; do + mkdir -p "$held/$(dirname "$lock")" + cp "$lock" "$held/$lock" + done + # Hash the master lockfiles this refresh started from, by repository + # path, so the next run can check them with sha256sum --check. + (cd "$RUNNER_TEMP/base-locks" && sha256sum $LOCKFILES) > "$held/source-locks.sha256" - name: Save held lockfile for the next refresh if: steps.release-age.outputs.eligible == 'false' diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 5a7659c4ee9..a7b69dee07e 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -132,11 +132,26 @@ Current implementation status: ## Dependency Lockfile Policy -GitHub Actions owns `pnpm-lock.yaml`. +GitHub Actions owns every managed pnpm lockfile: -- Do not commit `pnpm-lock.yaml` in pull requests. -- Pull request CI validates dependency resolution when manifests change. -- Pushes to `master` regenerate `pnpm-lock.yaml` with `pnpm install --lockfile-only --no-frozen-lockfile`, commit it back if needed, and then run verification with `--frozen-lockfile`. +- `pnpm-lock.yaml` at the repository root +- `scripts/typecheck-sandbox-providers/pnpm-lock.yaml`, for the sandbox providers +- `packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml` + +`LOCKFILES` in `.github/scripts/lockfile-trust.mjs` lists them. + +- Do not commit any of them in pull requests. Only the lockfile refresh workflow + and verified Dependabot commits may change them. +- Pull request CI validates dependency resolution when manifests change. A stale + lockfile is resolved in place for that run. +- Pushes to `master` run `.github/workflows/refresh-lockfile.yml`. It regenerates + each lockfile with `pnpm install --resolution-only --ignore-scripts --no-frozen-lockfile` + and opens one `chore/refresh-lockfile` pull request for all of them. See + `doc/RELEASE-AUTOMATION-SETUP.md` for when the bot merges it. + +The Pi and Kimi distribution lockfiles under `packages/paperclip-runner/scripts/` +are not on this list. They hold attested provider pins, so change them by hand +(see `rules/acpx-profile-identity.mdc`). ## Trusted PR Workflow @@ -1343,15 +1358,15 @@ To add a test directory, add it to the `include` of the package's Sandbox providers are outside the root workspace, so the root install has none of their SDKs. `scripts/typecheck-sandbox-providers` is a small workspace that lists them and holds one lockfile. Its install replaces any standalone `node_modules` in -those providers. After changing a provider's `package.json`, refresh that lockfile: +those providers. After you change a provider's `package.json`, do not refresh or +commit that lockfile. `typecheck:test-programs` resolves a stale lockfile in place +for the run, and the lockfile refresh workflow commits the new one after merge (see +[Dependency Lockfile Policy](#dependency-lockfile-policy)). -```sh -pnpm -C scripts/typecheck-sandbox-providers install --ignore-scripts --lockfile-only -``` - -Do not commit a lockfile inside a provider. +Do not add a lockfile inside a provider. `scripts/build-standalone-public-packages.mjs` uses a frozen install when one -exists, which changes what a release resolves. +exists, which changes what a release resolves. Daytona already has one; the +release build resolves it in place when it is stale. #### Reused Runner build in the server build diff --git a/doc/RELEASE-AUTOMATION-SETUP.md b/doc/RELEASE-AUTOMATION-SETUP.md index 45607c30e85..1906df8f9ce 100644 --- a/doc/RELEASE-AUTOMATION-SETUP.md +++ b/doc/RELEASE-AUTOMATION-SETUP.md @@ -221,7 +221,7 @@ If `@cryppadotta` is not the right reviewer identity in the public repo, change ## 8.1. Let the Lockfile Bot Merge Lockfile Refreshes -`.github/workflows/refresh-lockfile.yml` regenerates `pnpm-lock.yaml` from the manifests on `master` and merges the result without CI or review. Only trusted environments write lock content, and contributors never commit it. Two writers can change `pnpm-lock.yaml`: +`.github/workflows/refresh-lockfile.yml` regenerates `pnpm-lock.yaml` from the manifests on `master` and merges the result without CI or review. It also regenerates the sandbox provider lockfiles, `scripts/typecheck-sandbox-providers/pnpm-lock.yaml` and `packages/plugins/sandbox-providers/daytona/pnpm-lock.yaml`, in the same pull request. `LOCKFILES` in `.github/scripts/lockfile-trust.mjs` and in the workflow lists all three. Only trusted environments write lock content, and contributors never commit it. Two writers can change these lockfiles: - this workflow, on its `chore/refresh-lockfile` branch - Dependabot, but only when the lockfile in its pull request matches a verified Dependabot commit @@ -232,7 +232,7 @@ The reporting-lock proof (`tests/runner-e2e/report-lock.json` and `report-lock.p After a Dependabot lockfile merges, this workflow renews the proof. It first confirms three things: GitHub signed the merge commit, Dependabot opened the pull request, and the lockfile at the pull request head matches a verified Dependabot commit. It then records the merged lockfile in the proof and merges the refresh like any other. Paid evaluation jobs refuse the new lockfile until that refresh merges. -Resolution keeps locked versions, but it picks the newest matching version for an added or widened dependency. A release-age gate (`.github/scripts/check-lockfile-release-age.mjs`) therefore checks every package version that the refresh adds. The bot merges only when each one has been on npm for at least 3 days. If any version is newer, or the gate cannot verify it, the bot leaves a comment on the PR and does not merge. A maintainer can review and merge it, or the daily scheduled refresh merges it after the versions are old enough. A held run saves its lockfile, and the hash of the `master` lockfile it started from, as an artifact. The next run resolves from that lockfile only while `master` still has the same lockfile. Held versions therefore stay put while their manifests are unchanged, even when newer releases appear, and a lockfile change on `master` replaces them. The gate also holds any added entry whose resolution is not a plain npm registry tarball. Change the age with `--min-age-days` in the workflow. +Resolution keeps locked versions, but it picks the newest matching version for an added or widened dependency. A release-age gate (`.github/scripts/check-lockfile-release-age.mjs`) therefore checks every package version that the refresh adds to any of the lockfiles. The bot merges only when each one has been on npm for at least 3 days. If any version is newer, or the gate cannot verify it, the bot leaves a comment on the PR and does not merge. A maintainer can review and merge it, or the daily scheduled refresh merges it after the versions are old enough. A held run saves its lockfiles, and the hashes of the `master` lockfiles it started from, as an artifact. The next run resolves from those lockfiles only while `master` still has every one of them unchanged. Held versions therefore stay put while their manifests are unchanged, even when newer releases appear, and a lockfile change on `master` replaces them. The gate also holds any added entry whose resolution is not a plain npm registry tarball. Change the age with `--min-age-days` in the workflow. The merge uses a dedicated GitHub App. Do not reuse `commitperclip` or bypass the built-in `github-actions` app. Those identities are available to other workflows, including release jobs that run dependency install scripts, so a bypass for them would let any of those workflows land code on `master`. diff --git a/scripts/__tests__/build-standalone-concurrency.test.mjs b/scripts/__tests__/build-standalone-concurrency.test.mjs index ef22755f57a..7ed862bc6d7 100644 --- a/scripts/__tests__/build-standalone-concurrency.test.mjs +++ b/scripts/__tests__/build-standalone-concurrency.test.mjs @@ -2,6 +2,8 @@ import assert from "node:assert/strict"; import test from "node:test"; import { + installStandalonePackage, + RESOLVE_LOCKFILE_ARGS, resolveConcurrency, runWithConcurrency, } from "../build-standalone-public-packages.mjs"; @@ -129,3 +131,36 @@ test("resolveConcurrency never returns more than the package count", () => { } } }); + +test("a package with a current lockfile installs it frozen once", async () => { + const calls = []; + await installStandalonePackage({ hasLockfile: true, lockfile: "pkg/pnpm-lock.yaml", log: () => {}, run: async (args) => { calls.push(args); } }); + assert.deepEqual(calls, [["install", "--ignore-workspace", "--frozen-lockfile"]]); +}); + +test("a stale committed lockfile is resolved in place, then installed frozen", async () => { + const calls = []; + const logs = []; + await installStandalonePackage({ + hasLockfile: true, + lockfile: "pkg/pnpm-lock.yaml", + log: (line) => logs.push(line), + run: async (args) => { + calls.push(args); + if (calls.length === 1) throw new Error("ERR_PNPM_OUTDATED_LOCKFILE"); + }, + }); + assert.deepEqual(calls, [ + ["install", "--ignore-workspace", "--frozen-lockfile"], + RESOLVE_LOCKFILE_ARGS, + ["install", "--ignore-workspace", "--frozen-lockfile"], + ]); + assert.ok(RESOLVE_LOCKFILE_ARGS.includes("--resolution-only") && RESOLVE_LOCKFILE_ARGS.includes("--ignore-scripts")); + assert.match(logs[0], /pkg\/pnpm-lock\.yaml/); +}); + +test("a package without a lockfile installs without writing one", async () => { + const calls = []; + await installStandalonePackage({ hasLockfile: false, lockfile: "pkg/pnpm-lock.yaml", log: () => {}, run: async (args) => { calls.push(args); } }); + assert.deepEqual(calls, [["install", "--ignore-workspace", "--no-lockfile"]]); +}); diff --git a/scripts/build-standalone-public-packages.mjs b/scripts/build-standalone-public-packages.mjs index db26ccf7564..d3d46ec05b0 100644 --- a/scripts/build-standalone-public-packages.mjs +++ b/scripts/build-standalone-public-packages.mjs @@ -115,6 +115,31 @@ async function runCaptured(command, args, cwd, log) { } } +const FROZEN_INSTALL_ARGS = ["install", "--ignore-workspace", "--frozen-lockfile"]; +export const RESOLVE_LOCKFILE_ARGS = ["install", "--ignore-workspace", "--resolution-only", "--ignore-scripts", "--no-frozen-lockfile"]; + +/** + * Install a standalone package. A package with a committed lockfile installs it frozen. Contributors + * do not commit that lockfile: the lockfile refresh workflow regenerates it after a manifest change + * merges. Until then it can be stale, so resolve it in place and install frozen again, like the + * root install. release.sh restores tracked files when it exits. + */ +export async function installStandalonePackage({ hasLockfile, lockfile, run, log }) { + if (!hasLockfile) { + // Standalone packages intentionally avoid committed lockfile churn in the repo. + await run(["install", "--ignore-workspace", "--no-lockfile"]); + return; + } + try { + await run(FROZEN_INSTALL_ARGS); + return; + } catch { + log(` i ${lockfile} does not match its package.json; resolving it in place`); + } + await run(RESOLVE_LOCKFILE_ARGS); + await run(FROZEN_INSTALL_ARGS); +} + // Each standalone package installs into its own directory (`--ignore-workspace`) // and builds into its own `dist`, so there is no shared mutable state between // packages beyond pnpm's content-addressable global store, which is safe for @@ -135,16 +160,12 @@ async function prepareAndBuildPackage(pkg) { rmSync(nodeModulesDir, { force: true, recursive: true }); } - const installArgs = existsSync(packageLockfilePath) - ? ["install", "--ignore-workspace", "--frozen-lockfile"] - : [ - "install", - "--ignore-workspace", - "--no-lockfile", - // Standalone packages intentionally avoid committed lockfile churn in the repo. - ]; - - await runCaptured("pnpm", installArgs, pkgDir, log); + await installStandalonePackage({ + hasLockfile: existsSync(packageLockfilePath), + lockfile: path.join(pkg.dir, "pnpm-lock.yaml"), + run: (args) => runCaptured("pnpm", args, pkgDir, log), + log, + }); // The fresh install above wipes node_modules and no longer fires a // per-plugin postinstall (removed for supply-chain safety), so link the diff --git a/scripts/typecheck-test-programs.mjs b/scripts/typecheck-test-programs.mjs index 562efef0939..c073bac93c1 100644 --- a/scripts/typecheck-test-programs.mjs +++ b/scripts/typecheck-test-programs.mjs @@ -60,6 +60,10 @@ const PREPARE_STEPS = [ ]; // Replacing a provider's standalone node_modules would otherwise ask for confirmation. const INSTALL_ARGS = ["install", "--frozen-lockfile", "--ignore-scripts", "--prefer-offline", "--config.confirmModulesPurge=false"]; +// Contributors do not commit provider lockfiles: the lockfile refresh workflow regenerates them after +// merge, as it does the root pnpm-lock.yaml. Until then a changed manifest leaves the lockfile stale, +// so resolve it in place, like the root install in CI. +export const RESOLVE_ARGS = ["install", "--resolution-only", "--ignore-scripts", "--no-frozen-lockfile"]; export function findUncheckedTests({ trackedFiles, checkedFiles, checkedElsewhere = CHECKED_ELSEWHERE }) { const checked = new Set(checkedFiles); @@ -164,12 +168,18 @@ export function typecheckTestPrograms({ const status = pnpm(args, { cwd: root, env }); if (status !== 0) return status; } - const installed = pnpm(INSTALL_ARGS, { cwd: path.join(root, PROVIDER_WORKSPACE), env }); + const workspace = path.join(root, PROVIDER_WORKSPACE); + let installed = pnpm(INSTALL_ARGS, { cwd: workspace, env }); if (installed !== 0) { - logError( - `typecheck:test-programs: if a provider's package.json changed, run ` + - `\`pnpm install --ignore-scripts --lockfile-only\` in ${PROVIDER_WORKSPACE} and commit its pnpm-lock.yaml`, + log( + `typecheck:test-programs: ${PROVIDER_WORKSPACE}/pnpm-lock.yaml does not match the provider manifests. ` + + `Resolving it in place. Do not commit it: the lockfile refresh workflow updates it after merge.`, ); + installed = pnpm(RESOLVE_ARGS, { cwd: workspace, env }); + if (installed === 0) installed = pnpm(INSTALL_ARGS, { cwd: workspace, env }); + } + if (installed !== 0) { + logError(`typecheck:test-programs: could not install the sandbox providers in ${PROVIDER_WORKSPACE}`); return installed; } // The install removes the SDK link from each provider's node_modules. diff --git a/scripts/typecheck-test-programs.test.mjs b/scripts/typecheck-test-programs.test.mjs index 52816e4fbdb..9e82a0a929e 100644 --- a/scripts/typecheck-test-programs.test.mjs +++ b/scripts/typecheck-test-programs.test.mjs @@ -10,6 +10,7 @@ import { findUncheckedTests, PROGRAMS, PROVIDER_WORKSPACE, + RESOLVE_ARGS, SANDBOX_PROVIDERS, typecheckTestPrograms, } from "./typecheck-test-programs.mjs"; @@ -104,13 +105,33 @@ test("providers install frozen without scripts, then relink the SDK", () => { assert.equal(calls.linked, 1); }); -test("a stale provider lockfile fails before any program runs and names the fix", () => { +test("a stale provider lockfile is resolved in place, then installed frozen", () => { + const workspace = path.join("/repo", PROVIDER_WORKSPACE); + let frozenAttempts = 0; + const { options, calls } = harness({ + pnpm: (args, { cwd }) => { + calls.pnpm.push({ args, cwd }); + if (cwd !== workspace || !args.includes("--frozen-lockfile")) return 0; + frozenAttempts += 1; + return frozenAttempts === 1 ? 1 : 0; + }, + }); + assert.equal(typecheckTestPrograms(options), 0); + assert.deepEqual( + calls.pnpm.filter(({ cwd }) => cwd === workspace).map(({ args }) => args.includes("--frozen-lockfile") ? "frozen" : args), + ["frozen", RESOLVE_ARGS, "frozen"], + ); + assert.ok(RESOLVE_ARGS.includes("--resolution-only") && RESOLVE_ARGS.includes("--ignore-scripts")); + assert.equal(calls.tsc.length, 2); +}); + +test("a provider install that still fails stops before any program runs", () => { const { options, calls } = harness({ pnpm: (args, { cwd }) => (cwd === path.join("/repo", PROVIDER_WORKSPACE) ? 1 : 0), }); assert.equal(typecheckTestPrograms(options), 1); assert.equal(calls.tsc.length, 0); - assert.match(calls.errors[0], /pnpm-lock\.yaml/); + assert.match(calls.errors[0], /could not install the sandbox providers/); }); test("an uncovered test file fails the run and is named", () => {