mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-08 22:13:59 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Every pull request runs the `ci / Build` check. This check was the slowest check in recent green runs: 432s in run 39254079691, 480s in run 39251304886 and 472s in run 39245233967. > - The slowest step in Build is "Verify built-in Grok from a clean public npm install" (162s and 207s). Its consumer `npm install` took 69s and 75s, even though a background step fills the npm cache first. > - The loopback registry in `scripts/public-install-registry.mjs` sends packages that it does not serve to npmjs with a 302 redirect. It escaped scoped names with `encodeURIComponent` (`%40scope%2Fname`). npm requests and caches the same metadata as `@scope%2fname`, and npm keys cached metadata by URL. > - Thus the consumer install could not use the scoped metadata that the warm-up had just put in the cache. It downloaded and parsed all 291 scoped packuments again (about 285 MB of full packument JSON) inside the 4-vCPU container. > - This pull request makes the redirect use the same escaping as npm. > - The benefit is a shorter Build check on every pull request. The installed bytes do not change. ## Linked Issues or Issue Description **Describe the bug** The consumer install in the public-install verifier does not use cached scoped package metadata. The warm-up step (`scripts/warm-public-install-npm-cache.mjs`) caches this metadata through direct npm requests. **Steps to reproduce** 1. Run `PAPERCLIP_PUBLIC_INSTALL_NPM_CACHE=/tmp/c node scripts/warm-public-install-npm-cache.mjs`. 2. Install the same dependency set through the loopback registry with `npm_config_cache=/tmp/c` and `--loglevel http`. 3. Count the `(cache miss)` lines. **Expected behavior** No cache misses for metadata that the warm-up already fetched. **Actual behavior** 291 cache misses. All of them are scoped packages that are requested as `https://registry.npmjs.org/%40scope%2Fname`. The cache holds them as `https://registry.npmjs.org/@scope%2fname`. Related: #16513 changes when the verifier waits for the warm-up. This pull request does not overlap with it, and the two changes add together. ## What Changed - `scripts/public-install-registry.mjs`: the 302 redirect for packages that the registry does not serve now uses npm's own escaping (`name.replace('/', '%2f')`, the same as npm-package-arg `escapedName`). The `packageName` check already allows only characters that need no other escaping. - `scripts/__tests__/public-install-registry.test.mjs`: the test now expects `@other%2fpkg` for a scoped redirect. A new assertion covers an unscoped redirect. ## Verification - `node --test scripts/__tests__/public-install-registry.test.mjs`: 3 of 3 pass. This includes the real npm install test that compares installed bytes from file installs and registry installs. - Local replay (Linux, npm 11.12.1). I filled a fresh cache with the warm-up script, then installed the same 62-dependency set (627 packages) through the registry: - Before: 291 cache misses, 28.3s. - After: 0 cache misses, 19.8s. - I also ran the real `scripts/public-install-registry.mjs` with a packed root package that depends on the full public dependency set. Result: 2 misses (only the loopback package and its tarball, as expected) and 1438 cache hits. - To compare on CI, look at `consumer_install_ms` in the `paperclip.public-npm-install.timing.v1` line of the Build log. Baseline: 68822 ms (run 39254079691) and 75175 ms (run 39251304886). ## Risks - Low risk. The redirect target is the same npmjs document. Only the URL escaping changes. npm still checks every tarball against the integrity in the packument. - If the npm cache holds stale scoped metadata, npm revalidates it the same way as it does for direct requests (`cache-control: max-age=300`). - The expected time saving comes from a local measurement. The real Build saving depends on the network and CPU of the runner. I expect about 10–20s per Build run. The CI timing line will confirm the number. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, used through Claude Code as a Paperclip agent with tool use (shell, file edit). I did the root-cause analysis, the local replays and the code change with this model. ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
145 lines
9.5 KiB
JavaScript
145 lines
9.5 KiB
JavaScript
import assert from 'node:assert/strict';
|
|
import { execFileSync, spawn } from 'node:child_process';
|
|
import { createHash } from 'node:crypto';
|
|
import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, readlinkSync, rmSync, writeFileSync } from 'node:fs';
|
|
import { tmpdir } from 'node:os';
|
|
import { join } from 'node:path';
|
|
import { fileURLToPath } from 'node:url';
|
|
import test from 'node:test';
|
|
import { grokPublicInstallVersion } from '../grok-public-install-sandbox.mjs';
|
|
import { startPackedRegistry, writePackedRegistryCatalog } from '../public-install-registry.mjs';
|
|
|
|
const script = fileURLToPath(new URL('../public-install-registry.mjs', import.meta.url));
|
|
const scratch = async body => {
|
|
const root = mkdtempSync(join(tmpdir(), 'public-install-registry-'));
|
|
try { await body(root); } finally { rmSync(root, { recursive: true, force: true }); }
|
|
};
|
|
const environment = root => ({ ...process.env, HOME: root, npm_config_cache: join(root, 'cache'),
|
|
npm_config_userconfig: join(root, 'no-npmrc'), npm_config_audit: 'false', npm_config_fund: 'false', npm_config_update_notifier: 'false', npm_config_fetch_retries: '0', npm_config_fetch_timeout: '10000' });
|
|
const run = (command, args, cwd, env = environment(cwd)) => new Promise((resolve, reject) => {
|
|
const child = spawn(command, args, { cwd, env, stdio: ['ignore', 'pipe', 'pipe'] });
|
|
let output = '';
|
|
for (const stream of [child.stdout, child.stderr]) stream.on('data', chunk => { output += chunk; });
|
|
child.once('error', reject);
|
|
child.once('exit', (code, signal) => code === 0 ? resolve(output) : reject(new Error(`${command} exited ${signal ?? code}: ${output}`)));
|
|
});
|
|
const fixture = (root, { publicDependency = false } = {}) => {
|
|
const assets = join(root, 'assets'); mkdirSync(assets);
|
|
const packs = [
|
|
{ name: '@ci/leaf', version: '1.0.0', main: 'index.js' },
|
|
{ name: '@ci/optional', version: '1.0.0' },
|
|
{ name: '@ci/root', version: '1.0.0', dependencies: { '@ci/leaf': '1.0.0', 'bundled-leaf': '1.0.0', ...(publicDependency ? { 'is-number': '7.0.0' } : {}) },
|
|
optionalDependencies: { '@ci/optional': '1.0.0' }, bundleDependencies: ['bundled-leaf'], bin: { 'ci-root': 'index.js' },
|
|
scripts: { postinstall: "node -e \"require('node:fs').writeFileSync('lifecycle-ran', 'ok')\"" } },
|
|
];
|
|
for (const [index, manifest] of packs.entries()) {
|
|
const source = join(root, `source-${index}`); mkdirSync(source);
|
|
writeFileSync(join(source, 'package.json'), JSON.stringify(manifest));
|
|
writeFileSync(join(source, 'index.js'), '#!/usr/bin/env node\nmodule.exports = 42;\n');
|
|
if (manifest.bundleDependencies) {
|
|
const bundle = join(source, 'node_modules/bundled-leaf'); mkdirSync(bundle, { recursive: true });
|
|
writeFileSync(join(bundle, 'package.json'), JSON.stringify({ name: 'bundled-leaf', version: '1.0.0', main: 'index.js' }));
|
|
writeFileSync(join(bundle, 'index.js'), 'module.exports = "bundled";\n');
|
|
}
|
|
execFileSync('npm', ['pack', '--ignore-scripts', '--pack-destination', assets], { cwd: source, env: environment(root), stdio: 'pipe' });
|
|
writeFileSync(join(source, 'package.json'), JSON.stringify({ ...manifest, version: '9.9.9' }));
|
|
}
|
|
return assets;
|
|
};
|
|
const installedFiles = directory => {
|
|
const entries = [];
|
|
const walk = (root, relative = '') => {
|
|
for (const item of readdirSync(root, { withFileTypes: true })) {
|
|
if (item.name === '.package-lock.json') continue;
|
|
const name = join(relative, item.name), path = join(root, item.name);
|
|
if (item.isDirectory()) walk(path, name);
|
|
else entries.push([name, item.isSymbolicLink() ? `link:${readlinkSync(path)}` : createHash('sha256').update(readFileSync(path)).digest('hex')]);
|
|
}
|
|
};
|
|
walk(directory);
|
|
return entries.sort(([a], [b]) => a.localeCompare(b));
|
|
};
|
|
|
|
test('synthetic Grok versions install from packed bytes even when the hash starts with leading-zero digits', { timeout: 30_000 }, async () => scratch(async root => {
|
|
const sourceRevision = '096091501644cf20f2e35d1ab02b28c35579b973';
|
|
const cases = [
|
|
{ version: `0.0.0-grok-verify.${sourceRevision.slice(0, 12)}`, rejected: true },
|
|
{ version: grokPublicInstallVersion(sourceRevision) },
|
|
{ version: grokPublicInstallVersion('8fd48a8d52038fd0d8b286f3742ea13ce40a7ab9') },
|
|
];
|
|
for (const [index, { version, rejected }] of cases.entries()) {
|
|
const cwd = join(root, String(index)), source = join(cwd, 'source'), assets = join(cwd, 'assets'), consumer = join(cwd, 'consumer');
|
|
for (const directory of [join(source, 'package'), assets, consumer]) mkdirSync(directory, { recursive: true });
|
|
writeFileSync(join(source, 'package/package.json'), JSON.stringify({ name: '@ci/grok-version', version }));
|
|
execFileSync('tar', ['-czf', join(assets, 'package.tgz'), '-C', source, 'package']);
|
|
await writePackedRegistryCatalog(assets);
|
|
writeFileSync(join(consumer, 'package.json'), '{"private":true}');
|
|
const userconfig = join(cwd, 'user.npmrc'), globalconfig = join(cwd, 'global.npmrc');
|
|
writeFileSync(userconfig, ''); writeFileSync(globalconfig, '');
|
|
const env = { PATH: process.env.PATH, HOME: cwd, npm_config_cache: join(cwd, 'cache'),
|
|
npm_config_userconfig: userconfig, npm_config_globalconfig: globalconfig,
|
|
npm_config_audit: 'false', npm_config_fund: 'false', npm_config_update_notifier: 'false', npm_config_fetch_retries: '0' };
|
|
const registry = await startPackedRegistry(assets);
|
|
try {
|
|
const install = run('npm', ['install', '--registry', registry.url, '--replace-registry-host=never', '--ignore-scripts', '--omit=dev', ...registry.specs], consumer, env);
|
|
if (rejected) await assert.rejects(install, /ETARGET/);
|
|
else {
|
|
await install;
|
|
assert.equal(JSON.parse(readFileSync(join(consumer, 'node_modules/@ci/grok-version/package.json'))).version, version);
|
|
}
|
|
} finally { await registry.close(); }
|
|
}
|
|
}));
|
|
|
|
test('real npm installs identical packed bytes, bundles, optional dependencies and bins without running scripts early', { timeout: 90_000 }, async () => scratch(async root => {
|
|
const assets = fixture(root, { publicDependency: true }), catalog = await writePackedRegistryCatalog(assets);
|
|
assert.ok(catalog.every(entry => entry.manifest.version === '1.0.0'), 'metadata must come from packed bytes, not changed source');
|
|
const before = join(root, 'file-consumer'), after = join(root, 'registry-consumer');
|
|
for (const cwd of [before, after]) { mkdirSync(cwd); writeFileSync(join(cwd, 'package.json'), '{"private":true}'); }
|
|
await run('npm', ['install', '--ignore-scripts', '--omit=dev', ...catalog.map(entry => join(assets, entry.file))], before);
|
|
await run(process.execPath, [script, assets], after);
|
|
assert.deepEqual(installedFiles(join(after, 'node_modules')), installedFiles(join(before, 'node_modules')));
|
|
for (const cwd of [before, after]) {
|
|
const marker = join(cwd, 'node_modules/@ci/root/lifecycle-ran');
|
|
assert.equal(existsSync(marker), false);
|
|
const lock = readFileSync(join(cwd, 'package-lock.json'), 'utf8');
|
|
const publicPackage = JSON.parse(lock).packages['node_modules/is-number'];
|
|
assert.equal(publicPackage.version, '7.0.0');
|
|
assert.equal(publicPackage.resolved, 'https://registry.npmjs.org/is-number/-/is-number-7.0.0.tgz');
|
|
await run('npm', ['rebuild', '--offline', '--ignore-scripts=false', '--foreground-scripts'], cwd);
|
|
assert.equal(readFileSync(marker, 'utf8'), 'ok');
|
|
assert.equal(readFileSync(join(cwd, 'package-lock.json'), 'utf8'), lock);
|
|
}
|
|
assert.deepEqual(installedFiles(join(after, 'node_modules')), installedFiles(join(before, 'node_modules')));
|
|
}));
|
|
|
|
test('the registry exposes only known tarballs, redirects other packages to npm and closes its listener', async () => scratch(async root => {
|
|
const assets = join(root, 'assets'); mkdirSync(assets);
|
|
writeFileSync(join(assets, 'registry-packages.json'), JSON.stringify([{ file: 'pkg.tgz', manifest: { name: '@ci/pkg', version: '1.0.0' }, integrity: 'sha512-YQ==' }]));
|
|
writeFileSync(join(assets, 'pkg.tgz'), 'packed bytes');
|
|
const registry = await startPackedRegistry(assets);
|
|
try {
|
|
const metadata = await (await fetch(`${registry.url}/@ci%2fpkg`)).json();
|
|
assert.equal(metadata.versions['1.0.0'].dist.integrity, 'sha512-YQ==');
|
|
assert.equal(await (await fetch(metadata.versions['1.0.0'].dist.tarball)).text(), 'packed bytes');
|
|
const external = await fetch(`${registry.url}/@other%2fpkg`, { redirect: 'manual' });
|
|
assert.equal(external.status, 302);
|
|
assert.equal(external.headers.get('location'), 'https://registry.npmjs.org/@other%2fpkg');
|
|
const unscoped = await fetch(`${registry.url}/other-pkg`, { redirect: 'manual' });
|
|
assert.equal(unscoped.headers.get('location'), 'https://registry.npmjs.org/other-pkg');
|
|
assert.equal((await fetch(`${registry.url}/-/900.tgz`)).status, 404);
|
|
assert.equal((await fetch(`${registry.url}/some%2foutside%2ffile`)).status, 404);
|
|
assert.equal((await fetch(`${registry.url}/@ci%2fpkg`, { method: 'POST' })).status, 405);
|
|
} finally { await registry.close(); }
|
|
await assert.rejects(fetch(`${registry.url}/@ci%2fpkg`));
|
|
}));
|
|
|
|
test('npm rejects changed tarball bytes against the catalog integrity and the CLI reports failure', { timeout: 90_000 }, async () => scratch(async root => {
|
|
const assets = fixture(root), catalog = await writePackedRegistryCatalog(assets);
|
|
const archive = join(assets, catalog[0].file);
|
|
const bytes = readFileSync(archive);
|
|
bytes[4] ^= 1; writeFileSync(archive, bytes);
|
|
const consumer = join(root, 'consumer'); mkdirSync(consumer); writeFileSync(join(consumer, 'package.json'), '{"private":true}');
|
|
await assert.rejects(run(process.execPath, [script, assets], consumer), /EINTEGRITY/);
|
|
}));
|