docs: slim PR review rules into tools/pr

[oxwen11]

Requirement

The review-and-merge workflow mixed gate decisions with command-level steps. Reviewers repeated worktree setup, branch updates, stack merges, session lookups, and evidence redaction by hand.

Expected behavior

pull-requests.md is the judgment checklist: CI, review, independent verification, record, then merge. A gate with no evidence stops the review and is explained on the pull request. The author of a version does not review that version. Merge is a squash of the reviewed SHA with --match-head-commit, and protections stay on. Host writes, shared Pi settings, production operations, and other irreversible actions still wait for the Developer.

Changes and risks

New host writes, or a dependency on Pi's physical files? No. pr-session only reads existing session JSON when an operator runs it.

This is a rules change, not a CI-only exception.

Security rule (disclosed)

security.md gains one rule, worded like CONTEXT.md and matching packages/server/src/pi/project-resource-policy.ts:

Registering a Project trusts it: Pie-owned Pi children for that Project (session and short-lived listing children) start with --approve and run its extensions. The daemon does not load extensions.

This records existing behavior. It does not change code.

What each script replaces

Script	Replaces this step in the old pull-requests.md	Already covered by pie-verify?
pr-status	§1 gh pr checks --required plus the MERGEABLE / SHA recheck before merging	No
pr-sync	Restarting other candidates after main moves: update-branch with expected_head_sha	No
pr-worktree	§3 "a dedicated, clean worktree pinned to its recorded head"	No. pie-verify needs a checkout but does not create one
pr-merge	§4 gh pr merge --squash --match-head-commit, the merge-commit record, and merge-async for a stacked pull request (plain merge returns 403)	No
pr-session	Looking up data.pullRequests[].ref.number under ~/.pie*/storage/sessions by hand	No
redact-evidence	§3 / acceptance "crop public evidence…" (done by hand with magick/ffmpeg)	Partly. pie-verify redacts only the daemon token record, not screenshots or video

Isolated launch, doctor, cleanup, and daemon-token redaction stay in pie-verify. Parallel isolation and cleanup semantics stay in tools/verify/README.md.

Moved, removed, or awaiting sign-off

• Moved to tools/verify/README.md: the screenshot, note, recording, and attach commands from acceptance.md's Capture and delivery section.
• Removed; the reviewer judged these safe: the CI-only exception (the workflow is now stricter), "unchanged conclusions are not repeated", and the trusted-rules commit in the record.
• Removed but undisclosed until review, awaiting Developer sign-off, not changed in this push: the ban on retrying until green and the single infrastructure retry, the default author scope oxwen11, the verification-surface rules (Web + Desktop, installed package versus source, fake Pi versus a real model), the strict base-up-to-date rationale, recording shared Pi settings changes without guess-restoring, the unresolved-review-threads check, and the smaller record (base SHA, checks, what warrants re-review, sanitising).
• Still to do from the earlier feedback: the pstack items (patch-id reuse, one-at-a-time and verified-prefix merging, two lines in design.md, two in AGENTS.md) and the "fall back to a comment when Request changes is not allowed" rule.

Review fixes in 41d5735

• pr-merge exits 2 as soon as gh pr merge fails and records nothing. poll_merged accepts MERGED only when headRefOid is the reviewed --sha. Otherwise it exits 1 without commenting. This closes the false "Merged as X" race.
• pr-session skips a malformed session file instead of aborting the scan.
• redact-evidence prefixes a leading-dash file name with ./. The README says to attach only *.redacted.*.
• CI (Code check) installs jq if it is missing and runs bash tools/pr/test.sh.

Verification

Tested at 41d5735:

• bash tools/pr/test.sh passes (fake gh, no GitHub writes). New cases: CONFLICTING, failed checks, behind base, a refused merge while another head merges, a merge that lands at a different head, a malformed session file, and a leading-dash file name.
• The race case fails against the previous pr-merge (exit 0, false record) and passes now.
• pnpm build && pnpm check passes. A bare pnpm check without a build fails only on typed lint in untouched TS files.
• actionlint .github/workflows/quality.yml passes.
• Not run against the real repository: pr-merge and pr-sync.

[pkg-pr-new]

npx https://pkg.pr.new/oxwen11/pie/@getpie/cli@477

commit: f5ff097

[oxwen11]

Pie Review record: Request changes (blocking), not merged

• Reviewed head: d3bd97464df5806d88b4a290ac7cbabd78550410
• Base: main @ a617dceaa10184fdb19e9072e7a2f2807a719554
• Trusted rules: main @ 2c10c91b9a86d186451423d1f97d8d3ecb624435 (AGENTS.md and .agents/rules/). This review follows the rules on main, not the versions this PR proposes.
• Conclusion: blocked. Two blocking findings, listed below. Not merged.
• CI-only exception: not used. This PR changes agent instructions and verification policy and adds executable scripts, so review and independent verification were both required.

Gate 1: CI

All 3 required checks passed on this head: Check, react-doctor and Publish @getpie/cli preview. The PR was MERGEABLE/CLEAN and 0 commits behind main.

Independent verification (clean detached worktree at the reviewed head)

• bash tools/pr/test.sh passed. It uses a fake gh and runs all 22 steps.
• pr-status against live GitHub, read-only: docs: slim PR review rules into tools/pr #477 reports ready and exits 0. docs: redesign Hub as a single-deployment event broker #466 (conflicting) and fix(desktop): store saved SSH hosts in the versioned JSON document #454 (behind with a failed Check) both report blocked and exit 1, with the correct reasons.
• pr-worktree against live GitHub: it created a detached worktree pinned to the head and left the primary checkout's branch unchanged. It refused an existing dest and a wrong --sha, creating nothing.
• pr-sync against live GitHub: with the head already up to date it reported up to date and made no write. With a wrong --sha it refused before calling update-branch.
• pr-session with an isolated --home: it matched the stored PR number, did not print the snapshot (which contained a token-shaped string), and rejected 0.
• redact-evidence with real ffmpeg: webm (vp8) and mp4 (h264) came out cropped to the requested size with the metadata title removed. It refused to overwrite an existing output, rejected bad geometry, and exited cleanly when magick was missing.
• Stack path: GET repos/oxwen11/pie/stacks?pull_request=477 returns [], so the plain squash path applies. merge-async with sha matches GitHub's documented stacked-PR merge API. pr-merge never passes --admin, --auto or bypass_rules, and it binds the merge to the head SHA.
• Safety: the only git operations are fetch and worktree add (nothing destructive), and gh stderr goes through a token scrub.

Blocking findings

1. pr-status / pr-merge count a PR as ready when required checks are missing. pr_ready only checks that the reported list is non-empty and that every reported check passed. It never compares against the base's required checks, and it ignores mergeStateStatus. I reproduced this with a fixture that reports only Publish @getpie/cli preview, with mergeable: MERGEABLE, behind: 0 and mergeStateStatus: BLOCKED. The result was result: ready, exit 0. Live docs: redesign Hub as a single-deployment event broker #466 has exactly this shape: its current head reports only 1 of the 3 required checks. This contradicts the proposed rule ("Missing … checks stop the workflow"), and it replaces main's explicit "Every required check must be present". The ruleset still blocks the actual merge, so this does not bypass protection. But the documented gate-1 tool gives the wrong answer. Needed: compare against the required contexts (repos/{repo}/rules/branches/{base}) and/or require a clean mergeStateStatus, and add a test.sh case for a missing required check.

2. Verification policy was removed without saying so. The PR body lists what it removes. These main rules are also gone, with no replacement in acceptance.md, tools/verify/README.md or the skills:

   • "shared SPA/UI paths need both Web and Desktop"
   • "source startup cannot prove an installed package"
   • "fake Pi cannot prove real model/tool execution"
   • "never reuse the user's app, development instance or real data"

   A weaker gate can be a valid choice by the owner, but here it doesn't look intentional. Needed: restore these (for example in acceptance.md), or list them explicitly under "Removed from the workflow" so the decision is on record.

Non-blocking notes

• The new security.md line ("A joined Project … launched with --approve") matches the code (PI_PROJECT_PROCESS_ARGS in the session and discovery spawns) and CONTEXT.md. However, the PR body doesn't mention it, and CONTEXT.md calls these Projects registered/imported, not "joined".
• pr-session matches on the number only, ignoring host/owner/repository, so a PR with the same number in another repo will match. Owner and repo are printed, so a reader can filter. By default it scans the operator's real ~/.pie* and prints local paths, which must not be pasted onto a PR.
• The proposed record rule drops the base SHA as well as the rules commit. Only the rules commit is listed as removed.
• CI does not run tools/pr/test.sh.
• When gh pr merge refuses, pr-merge reports it as unverified after polling instead of as a definite refusal.

Gaps

• Image redaction was not tested with a real magick (it isn't installed in the verification environment). It is covered only by the fake in test.sh.
• pr-merge and a real update-branch were not run against GitHub. Their logic was exercised through test.sh fixtures.

What warrants re-review

A new head that addresses findings 1 and 2, or a decision recorded on this PR by the Developer that finding 2's removals are intentional.

[oxwen11]

Review of head d3bd9746: changes needed

Base	a617dcea
Rules judged against	the trusted base (main's rules)
Required CI	green
Reviewer	independent, not the author

Blocking

1. tools/pr/pr-merge can post a false "Merged as X" record (:71-100).
   • When gh pr merge is refused, plain_merge prints the error and still calls confirm_and_comment.
   • poll_merged reads headRefOid but never compares it with --sha, so any state == MERGED counts as success.
   • Scenario: the author pushes after the gates, and --match-head-commit correctly refuses. Someone else then merges the new head inside the 30 s poll. The script reports success and posts a durable record claiming the reviewed SHA was merged.
   • Fix: fail non-zero as soon as the merge is refused, and require headRefOid == EXPECTED in poll_merged.
   • Add a harness case for this, and for merging when the PR is conflicting, has failed checks, or is behind base.
2. Undisclosed security-rule edit (security.md:19-20).
   • The new line "A joined Project is treated as approved: every Pi process … is launched with --approve" matches current code (project-resource-policy.ts:7). However, it is a security-rule change and the PR body does not mention it.
   • "Every Pi process" is also broader than CONTEXT.md, which covers only session children and Pie-owned children.
   • Fix: disclose it in the PR body or split it into its own PR, and match the wording to CONTEXT.md.

Rule changes the PR body does not disclose

These need explicit Developer sign-off.

• "Do not retry until green" is gone. The body says only that the single infrastructure-failure retry was removed. With the retry ban gone too, flaky checks can now be re-run until they pass.

• The default scope "author oxwen11" is gone. Autonomous merge now covers PRs from any same-repo author. The ruleset requires 0 approving reviews, so nothing on GitHub backs this up.

• The verification-surface rules are gone. These were:

  • shared SPA/UI paths need both Web and Desktop;
  • starting from source cannot prove an installed package;
  • a fake Pi cannot prove real model execution.

  They do not appear in acceptance.md or tools/verify/README.md.

• "Keep strict base-up-to-date protection enabled" is gone. Strict is on today, but the rule explained why the head guard alone is not enough.

• "Record that a run changed shared Pi settings; do not guess-restore" is gone.

• The unresolved-review-threads check is gone. pr-merge only blocks on CHANGES_REQUESTED.

• The PR record shrank. It lost the base SHA, the checks and their results, "what change warrants re-review", and the line about sanitising evidence.

These removals are safe: the CI-only exception (stricter now), "unchanged conclusions are not repeated", and the trusted-rules commit.

Non-blocking

• CI coverage. tools/pr/test.sh is not run by CI or by pnpm test, and nothing lints the shell files.
• detect_stack fails open. It falls back to a plain merge on any error containing "404". --match-head-commit still guards that merge.
• pr-session aborts on one malformed session file. It stops the whole scan with a jq error and exit 5 (reproduced).
• redact-evidence file handling.
  • A file named -x.png after -- reaches magick/ffmpeg as an option.
  • .redacted copies sit next to the originals, so a glob attach would upload both.

Checks run

Check	Result
bash tools/pr/test.sh	22/22, exit 0
shellcheck	false positives only
pr-status 477 (read-only)	ready
pr-session against real sessions	read-only (directory mtime unchanged); correct matches for 458/460
redact-evidence on PNG/webm	cropped or blurred, metadata stripped, inputs unchanged, refuses to overwrite and refuses blur on video

Gaps

• pr-merge and pr-sync were not run against the real repo (by design).
• The false-record race in finding 1 comes from reading the code; it was not executed.

Re-review starts from CI on the next push.

[iamdin]

Review record — not merged

• Head: 41d5735b9c97c9b601d5d50d143dde0a047fde68 (new since the review of d3bd9746)
• Base: current main is 34dbcc16bcccf0fe8d352b2ba6989bc351975b59. GitHub reports this PR BEHIND.
• Trusted rules: main @ 34dbcc16bcccf0fe8d352b2ba6989bc351975b59
• Conclusion: stopped at CI. The new commit was not reviewed. Not merged.

Required Check failed on this head: https://github.com/oxwen11/pie/actions/runs/37641743772/job/112862209991

Observed failure: @getpie/app browser -draft-loading.test.tsx “preserves the real draft editor while a newly connected Environment loads and resolves” timed out waiting for fill. Node tests and typecheck in that run passed. react-doctor and both preview publishes succeeded.

Findings recorded on d3bd9746 were not rechecked.

What warrants another review: a new head that contains current main and has every required check executed and successful. Review starts again at CI.

[iamdin]

Review record — not merged yet

• Previous head ea5152c0: required Check did not finish. The job annotation is “The self-hosted runner lost communication with the server.” The build/test step was cancelled. PR tools harness in that run had already succeeded. react-doctor and both preview publishes succeeded.
• Current head: f5ff097d695faf4080fbe179dee6f98d8fb3fc4b, which merges main @ 4a6bcee2 (fix(desktop): store saved SSH hosts in the versioned JSON document #454). tools/pr/test.sh passed locally on this head, including the cases that refuse to record a merge when gh pr merge fails or the merged head is not the reviewed SHA.

Waiting on required CI for f5ff097d. Review of this head restarts at CI.

[iamdin]

CI on f5ff097d — not merged

Required Check failed because the self-hosted runner lost communication. The build/test step did not finish. Run 38153192765.

react-doctor and both preview publishes are still queued. No retry yet: the last Code check to finish is this disconnect, at 16:18 UTC. One retry waits until a later Code check completes, so the runner has recovered.

[iamdin]

One Check retry

main @ 4a6bcee2 finished a Code check at 17:04 UTC (run 38153151649), so the runner is back. The failed Check on f5ff097d (run 38153192765) is being rerun once. react-doctor and both preview publishes on this head already succeeded.