forked from farhoodlabs/paperclip
b02e67cea5
## Thinking Path > - Paperclip’s PR workflow is part of the control-plane safety surface because it decides whether a branch is allowed to merge. > - This issue started in that workflow: the lockfile and manifest policy checks were diffing `base.sha..head.sha`, which incorrectly treated unrelated `master` commits as if they belonged to the PR branch. > - The right fix there is to diff from the PR merge base (`base...head`) so policy checks only evaluate files introduced by the branch itself. > - Once that workflow fix was in place, `/checkpr` exposed a second blocker on the PR merge ref: `verify` was failing in newer `master`-side tests that were not part of the original branch diff. > - The actionable repeated failure came from the ACPX local adapter test suite, where a test hard-coded the managed Codex home under `instances/default` even though the stable Vitest runner sets a non-default `PAPERCLIP_INSTANCE_ID`. > - This pull request now includes both the original CI diff-scope fix and the targeted ACPX test fix so the PR’s actual checks align with current base-branch execution. > - The benefit is that the original false-positive lockfile failure is removed, and the merge-ref verify path is hardened against the instance-id isolation used in CI. ## What Changed - Updated `.github/workflows/pr.yml` so the lockfile policy and manifest policy steps diff `pull_request.base.sha...pull_request.head.sha` from the merge base instead of using a two-dot base/head diff. - Added an inline workflow comment explaining why the three-dot diff is required for PR-scoped file detection. - Updated `packages/adapters/acpx-local/src/server/execute.test.ts` so the managed Codex home assertion uses a test-specific `PAPERCLIP_INSTANCE_ID` instead of hard-coding `default`. - Restored `PAPERCLIP_INSTANCE_ID` after that ACPX test finishes so the test remains isolated and does not leak process env changes. ## Verification - Reproduced the original false positive locally by comparing PR heads `#4901` and `#4902` with the old `base..head` logic; both incorrectly included `pnpm-lock.yaml` from unrelated `master` commits. - Verified the new `base...head` logic reduces those PRs to only their actual changed files and excludes `pnpm-lock.yaml`. - Verified a real manifest-changing PR (`#4893`) still reports `package.json` changes under the new logic. - Ran `pnpm -r typecheck` successfully. - Ran `pnpm vitest run packages/adapters/acpx-local/src/server/execute.test.ts` successfully after the ACPX test fix. - Ran `pnpm vitest run packages/db/src/backup-lib.test.ts` successfully against the merge-ref-related DB failure path observed during `/checkpr`. - Pushed commit `9520a976` and allowed PR `#4903` checks to rerun on the updated branch. ## Risks - Low risk: the workflow change only affects how PR policy checks determine the changed file set. - Low risk: the ACPX change is test-only and aligns the test with the instance-isolation behavior already used by `scripts/run-vitest-stable.mjs` in CI. - The remaining operational risk is limited to other unrelated merge-ref-only failures that were not reproduced in the targeted local verification above. > 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 - OpenAI Codex, `gpt-5-codex`, via the Codex local adapter in Paperclip. - Tool-using coding model with shell execution, git, GitHub CLI, and repository inspection in a local worktree. - Context included the current repo, the Paperclip task thread, PR check output, and the isolated execution workspace. ## 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 - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots - [ ] 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
155 lines
4.2 KiB
YAML
155 lines
4.2 KiB
YAML
name: PR
|
|
|
|
on:
|
|
pull_request:
|
|
branches:
|
|
- master
|
|
|
|
concurrency:
|
|
group: pr-${{ github.event.pull_request.number }}
|
|
cancel-in-progress: true
|
|
|
|
jobs:
|
|
policy:
|
|
runs-on: ubuntu-latest
|
|
timeout-minutes: 5
|
|
|
|
steps:
|
|
- name: Checkout repository
|
|
uses: actions/checkout@v4
|
|
with:
|
|
fetch-depth: 0
|
|
|
|
- name: Block manual lockfile edits
|
|
if: github.head_ref != 'chore/refresh-lockfile'
|
|
run: |
|
|
# Diff the PR branch against its merge base so recent base-branch commits
|
|
# do not masquerade as changes made by the PR itself.
|
|
changed="$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}")"
|
|
if printf '%s\n' "$changed" | grep -qx 'pnpm-lock.yaml'; then
|
|
echo "Do not commit pnpm-lock.yaml in pull requests. CI owns lockfile updates."
|
|
exit 1
|
|
fi
|
|
|
|
- name: Setup pnpm
|
|
uses: pnpm/action-setup@v4
|
|
with:
|
|
version: 9.15.4
|
|
run_install: false
|
|
|
|
- name: Setup Node.js
|
|
uses: actions/setup-node@v4
|
|
with:
|
|
node-version: 24
|
|
|
|
- name: Validate Dockerfile deps stage
|
|
run: node ./scripts/check-docker-deps-stage.mjs
|
|
|
|
- name: Validate dependency resolution when manifests change
|
|
run: |
|
|
changed="$(git diff --name-only "${{ github.event.pull_request.base.sha }}...${{ github.event.pull_request.head.sha }}")"
|
|
manifest_pattern='(^|/)package\.json$|^pnpm-workspace\.yaml$|^\.npmrc$|^pnpmfile\.(cjs|js|mjs)$'
|
|
if printf '%s\n' "$changed" | grep -Eq "$manifest_pattern"; then
|
|
pnpm install --lockfile-only --ignore-scripts --no-frozen-lockfile
|
|
fi
|
|
|
|
verify:
|
|
needs: [policy]
|
|
runs-on: ubuntu-latest
|
|
timeout-minutes: 20
|
|
|
|
steps:
|
|
- name: Checkout repository
|
|
uses: actions/checkout@v4
|
|
|
|
- name: Setup pnpm
|
|
uses: pnpm/action-setup@v4
|
|
with:
|
|
version: 9.15.4
|
|
|
|
- name: Setup Node.js
|
|
uses: actions/setup-node@v4
|
|
with:
|
|
node-version: 24
|
|
cache: pnpm
|
|
|
|
- name: Install dependencies
|
|
run: pnpm install --frozen-lockfile
|
|
|
|
- name: Typecheck
|
|
run: pnpm -r typecheck
|
|
|
|
- name: Run tests
|
|
run: pnpm test:run
|
|
|
|
- name: Verify release registry test coverage
|
|
run: pnpm run test:release-registry
|
|
|
|
- name: Build
|
|
run: pnpm build
|
|
|
|
- name: Release canary dry run
|
|
run: |
|
|
git checkout -B master HEAD
|
|
git checkout -- pnpm-lock.yaml
|
|
./scripts/release.sh canary --skip-verify --dry-run
|
|
|
|
e2e:
|
|
needs: [policy]
|
|
runs-on: ubuntu-latest
|
|
timeout-minutes: 30
|
|
|
|
steps:
|
|
- name: Checkout repository
|
|
uses: actions/checkout@v4
|
|
|
|
- name: Setup pnpm
|
|
uses: pnpm/action-setup@v4
|
|
with:
|
|
version: 9.15.4
|
|
|
|
- name: Setup Node.js
|
|
uses: actions/setup-node@v4
|
|
with:
|
|
node-version: 24
|
|
cache: pnpm
|
|
|
|
- name: Install dependencies
|
|
run: pnpm install --frozen-lockfile
|
|
|
|
- name: Build
|
|
run: pnpm build
|
|
|
|
- name: Install Playwright
|
|
run: npx playwright install --with-deps chromium
|
|
|
|
- name: Generate Paperclip config
|
|
run: |
|
|
mkdir -p ~/.paperclip/instances/default
|
|
cat > ~/.paperclip/instances/default/config.json << 'CONF'
|
|
{
|
|
"$meta": { "version": 1, "updatedAt": "2026-01-01T00:00:00.000Z", "source": "onboard" },
|
|
"database": { "mode": "embedded-postgres" },
|
|
"logging": { "mode": "file" },
|
|
"server": { "deploymentMode": "local_trusted", "host": "127.0.0.1", "port": 3100 },
|
|
"auth": { "baseUrlMode": "auto" },
|
|
"storage": { "provider": "local_disk" },
|
|
"secrets": { "provider": "local_encrypted", "strictMode": false }
|
|
}
|
|
CONF
|
|
|
|
- name: Run e2e tests
|
|
env:
|
|
PAPERCLIP_E2E_SKIP_LLM: "true"
|
|
run: pnpm run test:e2e
|
|
|
|
- name: Upload Playwright report
|
|
uses: actions/upload-artifact@v4
|
|
if: always()
|
|
with:
|
|
name: playwright-report
|
|
path: |
|
|
tests/e2e/playwright-report/
|
|
tests/e2e/test-results/
|
|
retention-days: 14
|