mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-09 07:10:34 +01:00
c66b4a55efdc5675b5a5146197078b94de6407da
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
|
|
e3041f5481 |
fix(ci): let the lockfile bot own the sandbox provider lockfiles (#19522)
## 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 <noreply@anthropic.com> |
||
|
|
3a8f95d928 |
fix(ci): let verified Dependabot lockfiles merge through the reporting-lock gate (#19356)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Only trusted environments may change `pnpm-lock.yaml`: the lockfile refresh workflow and Dependabot. Contributors never commit it. > - Paid evaluation jobs also install a lock only when the reporting-lock proof (`tests/runner-e2e/report-lock.json` and `.patch`) admits it. > - #19283 added a CI test that checks the committed lock against that proof. Only the refresh workflow writes the proof, so every Dependabot lockfile pull request now fails CI. Examples are #17364, #17172, and #17911. > - A merged Dependabot lockfile also breaks the refresh workflow. `prepare-report-lock.mjs --refresh` refuses to renew the proof for a lock that the proof does not already admit. > - This pull request lets a verified Dependabot lockfile pass, and it keeps both lockfile rules strict. It also closes a gap in the contributor rule. > - The benefit is that Dependabot security updates can merge again, and only trusted environments write lock content. ## Linked Issues or Issue Description Refs #19283, #17364, #17172, #17911, #19303. **What happened?** `server/scripts/reporting-lock-policy.test.mjs` fails on each Dependabot pull request that changes `pnpm-lock.yaml`. The error is "Reporting lock changed; review the dependency correction before install". A Dependabot pull request cannot change the proof files, so it can never pass. After a merge, the lockfile refresh also stops at "Committed report lock is not reviewed; refusing to refresh its proof". **Expected behavior** - Contributors cannot change `pnpm-lock.yaml` or the reporting-lock proof. - Dependabot can change `pnpm-lock.yaml`. - The lockfile refresh workflow can change `pnpm-lock.yaml` and the proof. - After a Dependabot lockfile merges, the refresh workflow admits it and renews the proof. **Steps to reproduce** 1. Open a Dependabot pull request that changes `pnpm-lock.yaml`, such as #17364. 2. Let CI run. The `server (9/12)` shard fails on the reporting-lock policy test. **Security gap found during this work** The old rule skipped the lockfile check when `github.head_ref` was `chore/refresh-lockfile`. A contributor could open a pull request from a fork branch with that name and commit a lockfile change. The old rule also exempted any pull request that Dependabot opened, including commits that other people pushed to its branch. ## What Changed - Add `.github/scripts/lockfile-trust.mjs`. It decides which writers may change lock content: - The refresh workflow: the head is `chore/refresh-lockfile` in this repository, and `github-actions[bot]` opened the pull request. - Dependabot: Dependabot opened the pull request from this repository, and the head `pnpm-lock.yaml` matches a Dependabot commit. That commit must have `dependabot[bot]` as its author, GitHub (`web-flow`) as its committer, and a verified signature. The script reads these fields from the GitHub API. - The reporting-lock proof files count as lock content. Only the refresh workflow may change them. - `pr-trusted.yml` policy job: replace the inline `head_ref` and author exemption with `lockfile-trust.mjs pr`. The job runs the script copy from the base commit, so a pull request cannot change the check that judges it. If the base predates the script, the job uses the earlier rule. - `refresh-lockfile.yml`: add `lockfile-trust.mjs merged`. It checks that the last lockfile change on `master` is a GitHub-signed merge of a Dependabot pull request, and that the lockfile at the pull request head matches a verified Dependabot commit. If both checks pass, it gives the lockfile SHA-256 to `prepare-report-lock.mjs --refresh --trusted-lock-sha256`. - `refresh-lockfile.yml` PR step: check `pnpm-lock.yaml` and both proof files before it exits as unchanged. A refresh that renews only the proof now opens a pull request. - `prepare-report-lock.mjs` and its `.d.mts` declaration: `refreshReportLock` accepts a trusted lock hash. If the trusted lock already resolves, the proof admits it exactly with an empty patch. If not, the proof records the resolution as the reviewed correction, as before. - `reporting-lock-policy.test.mjs`: skip on Dependabot pull requests. The policy job verifies those lockfiles. The general test job gets `PAPERCLIP_CI_PR_AUTHOR` for this purpose. This keeps the test, so it still merges cleanly with #19303. - `check-pr-lockfile.mjs` (advisory gate): allow Dependabot lockfile changes, and flag changes to the proof files. - `CODEOWNERS`: add the two proof files. - `doc/RELEASE-AUTOMATION-SETUP.md`: describe the writers, the `@dependabot recreate` path, and the proof renewal after merge. ## Verification - `node --test '.github/scripts/tests/*.test.mjs'`: 707 passed. This includes 17 new tests in `lockfile-trust.test.mjs` and 3 new tests in `check-pr-lockfile.test.mjs`. The new tests run the `pr` command against real git histories with a stub `gh`. - `tests/runner-e2e/prepare-report-lock.test.ts`: 10 passed, including 3 new trusted-lock refresh tests. I ran it with a minimal Vitest config, because the runner-e2e config builds a provider bundle first. - `server/scripts/reporting-lock-policy.test.mjs`: passes normally, and skips with `PAPERCLIP_CI_PR_AUTHOR=dependabot[bot]`. - Live check of the `pr` mode against #17364 with the real GitHub API: - With the real author, it reports "pnpm-lock.yaml matches verified Dependabot commit 83c328c15c…" and `writer=dependabot`. - With a contributor as the author and `chore/refresh-lockfile` as the head ref, it fails. - Live check of the `merged` mode at the real merge commits of #16018, #16020, and #15361. All three report a trusted lock hash. For #16018 and #15361, `master` changed after Dependabot's commit, so GitHub's merge produced different lockfile bytes. An exact-bytes rule would refuse them. That is why the rule trusts GitHub-signed merges of a verified Dependabot head. - Greptile reviewed `513ca2def8` at 4/5 with one P1: a proof-only refresh was never committed, because the PR step checked only `pnpm-lock.yaml`. `edbeb1ea0d` fixes it. A new test in `lockfile-refresh-merge.test.mjs` runs the step's change check on a proof-only change. The test fails without the fix. - CI `Typecheck + Release Registry` failed at `edbeb1ea0d`: `prepare-report-lock.d.mts` did not declare the new options argument (TS2554 twice in `prepare-report-lock.test.ts`). `9139aaad85` updates the declaration. A local `tsc -p tests/runner-e2e/tsconfig.json` shows the 2 errors with the old declaration and none with the new one. - CI at `9139aaad85` failed in six general test shards. None of them run code that this pull request changes, and PR CI loads `pr-trusted.yml` from `master`. The failures: - `heartbeat-plugin-environment.test.ts`: "expected 'running' to be 'succeeded'" - `status-card-evaluation-work.test.ts`: a deadline comparison - `isolated-repository-rehome.test.ts`: timed out after 15 seconds - `chat-channels.integration.test.ts`: GitHub webhook recovery scheduling - `company-import-export-e2e.test.ts`: child server start timeout, which also fails on unrelated branches - `workspaces-b`: every test passed, but an `EPIPE` error came after `acpx-engine/execute.test.ts` I started a rerun of the failed jobs. - Superagent found a P1 at `9139aaad85`: a verified signature proves who committed, not who authored. A collaborator could sign a commit with their own key and set Dependabot as the author. `4fe180d6c1` also requires GitHub (`web-flow`) as the committer. New tests cover the spoofed-author case. #17364, #16018, #16020 and #15361 still verify against the GitHub API. - `git merge-file` merges this test change with #19303 without conflicts. - Not run: the full repository test suite and typecheck. These changes touch only CI scripts, workflows, one helper, and tests. GitHub CI covers full regression. ## Risks - `pr-trusted.yml` loads from `master`, so the new policy takes effect only after merge. This pull request's own CI uses the earlier rule. - A Dependabot pull request that is behind `master` now fails if someone merges `master` into it and the lockfile changes. Use `@dependabot recreate`. The policy job error says this. - Paid evaluation jobs refuse a merged Dependabot lockfile until the refresh pull request merges. The refresh merges without review when it adds no new package versions. If the release-age gate holds it, the wait lasts until a maintainer merges it or the daily refresh runs. During that time, the reporting-lock policy test also fails on other pull requests, which is the current behavior for any unrenewed lock. - The rule trusts GitHub as the signer of Dependabot commits and of merge commits (`web-flow`). Superagent uses the same trust root. - People with write access can still push to `chore/refresh-lockfile`. This was already true. The bot merges only its own head commit (`--match-head-commit`), and code-owner review covers anything else. - A future manual proof renewal, such as #19283, now needs the refresh workflow or an admin merge. A contributor pull request that changes the proof files fails. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), with tool use and code execution, in Claude Code. ## 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 (focused tests; see Verification) - [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 <noreply@anthropic.com> |
||
|
|
96feaa331a |
feat(commitperclip): add automated PR quality and security gates (#6469)
Fixes #6470 ## Thinking Path > - Paperclip is an open-source AI agent platform receiving a high volume of community PRs — currently 2,398 open > - The contributor experience is broken: PRs sit for months with no feedback, contributors don't know why they're stuck, and maintainers spend review time on PRs that are missing basics > - Common problems: no linked issue, no test coverage, incomplete PR template, manually-edited lockfile — all catchable before human review > - At the same time, accepting untrusted PRs from unknown contributors is a real attack surface: malicious packages, secret injection, tampering with CI scripts, and code touching the sensitive paths from the April security advisories > - This PR adds automated gates that run on every PR: quality failures get a clear comment telling contributors exactly what to fix, security concerns are silently flagged as draft advisories and block merge via a pending check run > - The benefit is a dramatically faster feedback loop for good-faith contributors and a meaningful security layer for the maintainers reviewing them ## What Changed - **`.github/workflows/commitperclip-review.yml`** — new workflow using `pull_request_target` (runs in base branch context, has secrets, never executes PR code). Runs quality gates + security gates on every PR open/update. - **`.github/dependabot.yml`** — weekly automated dependency vulnerability PRs for npm and GitHub Actions. - **`.github/scripts/get-bot-token.mjs`** — generates a short-lived commitperclip installation token from `COMMITPERCLIP_KEY` secret. - **`.github/scripts/run-quality-gates.mjs`** — orchestrates 5 quality gates, posts/updates a single consolidated comment on the PR. - **`.github/scripts/check-pr-template.mjs`** — validates all 5 required template sections, Thinking Path depth (≥3 sentences), Model Used not placeholder. - **`.github/scripts/check-pr-linked-issue.mjs`** — requires `Fixes #NNN` or issue URL in PR body. - **`.github/scripts/check-pr-test-coverage.mjs`** — requires at least one test file in the diff. - **`.github/scripts/check-pr-lockfile.mjs`** — blocks manual `pnpm-lock.yaml` edits (only the refresh bot may change it). - **`.github/scripts/check-pr-dependencies.mjs`** — informational comment when new npm packages are added. - **`.github/scripts/check-pr-security.mjs`** — 6 silent security checks: secret patterns, CI workflow tampering, build script changes, supply chain (new packages in lockfile), suspicious test patterns (outbound network/shell exec/env var reads), and changes to the 9 sensitive path prefixes from the April advisories. When any fire: creates a draft security advisory + sets `security-review` check to `in_progress` (blocks merge). When clean: sets `security-review` to `success`. - **`actions/dependency-review-action@v4`** — per-PR dependency vulnerability check (fails if new dep has known CVE). - **44 unit tests** across all gate modules (`node:test`, no external deps). ## Verification Run all unit tests locally: ```bash node --test .github/scripts/tests/*.test.mjs ``` Expected: 44 pass, 0 fail. End-to-end: open a PR missing the template, linked issue, and test files → commitperclip posts a consolidated comment listing all failures. Open a PR with all gates satisfied → `✅ All checks passing` comment posted, all check runs green. ## Risks **`pull_request_target` security model:** This workflow runs in base branch context and has access to secrets. It explicitly checks out `ref: master` (never PR code) and reads the PR diff via GitHub API only — no PR code is ever executed. This is the correct pattern for running secret-bearing checks on fork PRs; deviating from it (e.g. checking out the PR branch) would be a security vulnerability. **False positives on security gates:** The sensitive-path gate flags any PR touching the 9 path prefixes from the April advisories. Legitimate fixes to those paths will trigger draft advisories. This is intentional — those paths warrant a human look regardless. The `security-review` check can be manually resolved by a maintainer once reviewed. **commitperclip not yet installed:** Until the app is installed on this repo and the `COMMITPERCLIP_KEY` secret is added, the workflow will fail on the token generation step. The quality gate comment won't post, but Dependency Review will still run independently. ## Model Used Claude Sonnet 4.5, 200k context window, extended thinking enabled, tool use: read/edit files, bash execution, GitHub API calls ## 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 run tests locally and they pass (44/44) - [x] I have added or updated tests where applicable (44 unit tests across all gate modules) - [ ] If this change affects the UI, I have included before/after screenshots (N/A — CI only) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --- ## One-time setup needed from you, Dotta 1. **Install commitperclip app** on this repo: https://github.com/apps/commitperclip/installations/new 2. **Add `COMMITPERCLIP_KEY`** as a repository secret (Actions → Secrets) — ask @brandonburr for the key 3. **Add `security_advisories: write` and `checks: write`** to the commitperclip app permissions (commit-capital org → Settings → Apps → commitperclip → Permissions) 4. **Install Socket.dev** from GitHub Marketplace for supply chain scanning 5. **Branch protection** (optional but recommended): require `commitperclip-review` and `security-review` checks to pass before merge ## Dashboard integration note The `commitperclip-review` check run result maps cleanly to your PR triage dashboard. A single filter on your Worker: ```javascript const gatesCheck = checkRuns.find(r => r.name === 'commitperclip-review'); if (gatesCheck?.conclusion === 'failure') return null; // filter from queue ``` For security flags: `GET /repos/paperclipai/paperclip/security-advisories?state=draft` — advisory titles include `PR #NNN` for cross-referencing. PRs with a matching draft advisory have `security-review` in `in_progress` state (grey spinner, can't merge via branch protection). --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Devin Foley <devin@devinfoley.com> Co-authored-by: Paperclip <noreply@paperclip.ing> |