mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-13 13:38:01 +02:00
2223b7de9e
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Every pull request runs the `Build` check. Its last step is the public npm install verifier, `scripts/verify-grok-npm-install.mjs`, which stages the eighteen public packages reachable from the server and the CLI, packs them, and installs them in a clean consumer. > - Before it stages anything, the verifier runs `scripts/build-standalone-public-packages.mjs`. That script installs and compiles the seven sandbox-provider plugins that live outside the pnpm workspace: one network `pnpm install --ignore-workspace` and one `tsc` per plugin. The line was copied from the build block of `release.sh` when the verifier was written in #14024. > - release.sh needs that build because it publishes those plugins. The verifier does not for anything it stages. Its package closure follows `workspace:` specs from the server and the CLI, and a `workspace:` spec can only name a workspace package, so none of the eighteen packages it packs is a standalone package and no tarball reads that build's output. > - One optional path at the end of the verifier does read it: qualification-package retention, which `docker-runner-check.yml` enables with `PAPERCLIP_RUNNER_QUALIFICATION_PACKAGES_DIR` after running only `pnpm build`, packs the Daytona plugin from its `dist`. The PR `Build` job never sets that variable. > - The timing line from #15868 and #15888 puts the verifier's staging phase at 75.8s on the latest run, of which the staged sources, the bundled installs and the packs account for 46.5s. The rest, about 29s, is the work before staging, and the standalone build is almost all of it. `Build` at 433-530s is the slowest check in most PR runs once the server shards drop, so that time sits on the run's critical path. > - This pull request runs the standalone build only when retention is requested, keeps the `ui-dist` copy the server package needs, and times both on the timing line so what is left before staging is visible. > - The benefit is a `Build` job about 21-28s shorter, with no change to what the verifier installs or checks, no loss of coverage for the standalone build because Canary Dry Run still runs it through `release.sh`, and no change to the retention path that reads it. ## Linked Issues or Issue Description Replaces #15892, which GitHub closed automatically when the branch of #15888 (its base at the time) was deleted on merge. Same two commits, rebased onto `master` on top of #15888 (1ac48aff), #15868 (026bdcd0) and #15861 (85f4205d), which are all merged. #15892 reached Greptile 5/5 with every check green on the same content; the review history is there. **What existing behavior does this improve?** The `Build` PR check. Its public npm install verifier builds the seven standalone sandbox-provider plugins before it stages the eighteen packages it installs, and none of those packages uses that build. **Subsystem affected** CI and release scripts (`scripts/verify-grok-npm-install.mjs`, `scripts/build-standalone-public-packages.mjs`). **Current behavior** The verifier runs `node scripts/build-standalone-public-packages.mjs` and then `bash scripts/prepare-server-ui-dist.sh` before it stages any package, on every run. The first script removes each sandbox-provider plugin's `node_modules`, runs `pnpm install --ignore-workspace` against the registry, links the in-repo plugin SDK, and runs the plugin's `tsc` build, four plugins at a time. On the last `Build` run before this change ([38146108750](https://github.com/paperclipai/paperclip/actions/runs/38146108750)) `stage_packages_ms` was 75.8s and the staged sources, bundled installs and packs inside it took 0.9s, 15.7s and 29.9s, which leaves about 29s before staging. The verifier's package closure, computed in the same way as the script computes it, contains eighteen workspace packages and no sandbox-provider plugin. **Proposed behavior** Run the standalone build only when `PAPERCLIP_RUNNER_QUALIFICATION_PACKAGES_DIR` is set, because that is the one path that reads it, and report it as `stage_standalone_build_ms` when it runs. Always run the `ui-dist` copy before staging. With `PAPERCLIP_RELEASE_REUSE_UI_DIST=1`, which the verifier already sets, that copies the `ui/dist` that `pnpm build` produced into `server/ui-dist`, which the server package publishes. Report that copy as `stage_ui_dist_ms` on the timing line. Leave `release.sh` unchanged, so the release flow and Canary Dry Run still build the standalone packages where they are published. **Reason and benefit** The verifier proves that the eighteen public packages install cleanly from the public registry. The standalone build adds seven network installs and compiles whose output is never packed. Removing it from the PR path shortens the staging phase by about 21-28s. Measured on this PR's first commit ([38150787810](https://github.com/paperclipai/paperclip/actions/runs/38150787810)): `stage_packages_ms` 54.2s against 75.8s, with `stage_ui_dist_ms` 21ms, and the time before staging, `stage_packages_ms` minus the four staging keys, down from about 29s to under 0.1s. **Breaking changes** None. The same eighteen packages are staged, packed, checked and installed from the same sources. The retention path builds exactly what it built before. `scripts/build-standalone-public-packages.mjs` exports two existing helpers for the new guard test and otherwise does not change. ## What Changed - `scripts/verify-grok-npm-install.mjs`: the `build-standalone-public-packages.mjs` call before staging runs only when `PAPERCLIP_RUNNER_QUALIFICATION_PACKAGES_DIR` is set, in the same position as before, and reports `stage_standalone_build_ms`. The flag is read once into `retainQualificationPackages`, and the retention call at the end is gated by the same variable, so the build and the retention that reads it cannot diverge. The `prepare-server-ui-dist.sh` call stays, is timed, and reports `stage_ui_dist_ms`. The comment explains why release.sh and the retention path need the standalone build and the staged packages do not. The timing-line comment lists both new keys. - `scripts/build-standalone-public-packages.mjs`: `parseWorkspaceEntries` and `isWorkspacePackage` are exported. No behavior change. - `scripts/__tests__/grok-public-install-sandbox.test.mjs`: one new guard test. It asserts the verifier references the standalone build exactly once, inside the retention gate, timed under its own key, and that no second reading of the retention variable exists; that `retain-runner-qualification-packages.mjs` still packs the Daytona plugin from its `dist`, and that the Daytona plugin is a standalone package, so the gate cannot pass vacuously; recomputes the verifier's package closure from `release-package-map.mjs list` and the manifests through `workspace:` specs; checks that the public package map still carries packages outside the pnpm workspace; checks that every package in the closure is inside the workspace globs of `pnpm-workspace.yaml`; checks that the server package still publishes `ui-dist`; and checks the order: closure, gated standalone build, `ui-dist` copy, then staging, with the reuse flag set and the timing keys written. ### Review fix, second commit Greptile's P1 on the first commit was valid: `docker-runner-check.yml` runs `pnpm build` and then the verifier with `PAPERCLIP_RUNNER_QUALIFICATION_PACKAGES_DIR` set, and `retainRunnerQualificationPackages` copies `packages/plugins/sandbox-providers/daytona/dist`. The Daytona plugin is excluded from the pnpm workspace, so `pnpm build` never produces that directory and the first commit would have failed that job with `ENOENT` after every install check had passed. The second commit restores the build on exactly that path: the verifier builds the standalone packages when, and only when, retention is requested, and the guard test pins the gate to the reason for it. ## Verification - `node --test scripts/__tests__/grok-public-install-sandbox.test.mjs scripts/__tests__/pack-staged-packages.test.mjs scripts/__tests__/stage-bundled-packages.test.mjs scripts/__tests__/prefetch-child.test.mjs scripts/__tests__/retain-runner-qualification-packages.test.mjs`: 37 tests pass. - The updated guard test fails against the first commit's verifier (no retention gate around the build) and passes with the second commit. The first commit's test failed against #15888's verifier the same way. - `node --check` passes on the verifier. - The verifier itself needs Docker and a network, which this workspace does not have. The first commit's `Build` job ([38150787810](https://github.com/paperclipai/paperclip/actions/runs/38150787810)) ran the branch's verifier and measured the saving: `stage_packages_ms` 54.2s (was 75.8s), `stage_ui_dist_ms` 21ms, `stage_sources_ms` 0.8s, `stage_bundled_ms` 21.0s, `stage_pack_ms` 32.4s, so under 0.1s remains before staging where about 29s was. The second commit does not change the PR path: the gate is false on every PR run, so that job's timing line should show the same shape with no `stage_standalone_build_ms` key. - The retention path was not run here: it needs Docker and the `docker-runner-check` workflow's inputs. The code on that path is the same `run(process.execPath, [...build-standalone-public-packages.mjs])` call the verifier made before this PR, in the same position. - `scripts/__tests__/release-dry-run-notes.test.mjs` exercises the standalone build inside `release.sh` and is unchanged; 11 of its 12 tests spawn `pnpm`, which is not installed in this workspace, and fail the same way on #15888's head. They pass in CI, where `pnpm` is installed. - The first commit's run failed only on `General tests (server (11/12))`: all 74 files and 1257 tests passed, then Vitest caught a teardown `EACCES` removing the per-worker `PAPERCLIP_HOME` under `native-session-resumption.test.ts`, the same error as the first attempt of #15888's run 38146108750. Neither PR touches server code; the failure is noted on the tracking issue. ## Risks - If a future change makes a package the verifier installs depend on a standalone package, the verifier would stage an unbuilt `dist`. That cannot happen through a `workspace:` spec, which is the only kind the closure follows, and the guard test fails if the closure ever contains a package outside the workspace globs. - If a future path in the verifier reads a standalone build without setting the retention variable, it would see no `dist`. The guard test pins the single reference to the gate; a new reader has to extend the gate and the test together. - The sandbox-provider plugins' builds are no longer exercised on every PR by the `Build` job. They are still built by `release.sh` in Canary Dry Run on every PR, by the verifier whenever retention is requested, and the `build-standalone-concurrency` and `release-dry-run-notes` tests still cover the script. - Low risk otherwise. The bundled installs, the packs, the consumer install, and every probe are unchanged. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Fable 5.1 (Anthropic), model ID `claude-fable-5-1`, run as a Paperclip agent through the Claude Code local adapter with tool use (shell, file edits, GitHub CLI). Reasoning mode: extended thinking. ## 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 (CI is running on the second commit) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (the P1 on the first commit is fixed in the second; re-review pending) - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Bender (Fable) <bender-fable@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com>