fix: substitute {pr} in posting-format.md and log html_url

[derekmisler]

What

Fixes the false '⚠️ Review did not complete' double-posts seen in docker/sandboxes (evidence: run 34488043917, PR 5929), and eliminates the guaranteed 404 on every feedback reply.

Root cause: {pr} in gh api URLs is not a gh CLI template variable — only {owner} and {repo} are. The literal string {pr} produced a 404 on first attempt. For the review path, the agent retried but the retry only logged {id, state} (no html_url), so the pullrequestreview-[0-9]+ grep in action.yml missed it and posted a spurious '⚠️ Review did not complete' on top of a completed review. For the reply path, every reply paid a guaranteed 404 + retry.

Changes

review-pr/agents/refs/posting-format.md

• Keep {pr} as the placeholder (baked in at render time by sed)
• Add set -o pipefail before the posting pipeline so gh api failures propagate
• Pipe through | jq '{id, state, html_url}' so html_url (containing pullrequestreview-XXXXXXX) is always logged

review-pr/action.yml — 'Copy reference files' step

• Add PR_NUMBER to env block; validate it is numeric before substitution (mirrors the SHA guard)
• Substitute {pr} → actual PR number via sed alongside __PR_HEAD_SHA__
• Broaden the {pr} guard to grep -rq '{pr}' /tmp/refs/ (catches future placeholders in any staged ref file)
• Broaden the validation error message to cover all failure conditions

review-pr/reply/action.yml (new 'Stage reply agent' step)

• Render pr-review-reply.yaml to /tmp/pr-review-reply.yaml with {pr} substituted; fail-open with a warning if pr-number is missing/non-numeric
• Point the run-reply step at the rendered copy; add pr-number input

.github/workflows/review-pr.yml

• Pass pr-number: ${{ steps.feedback.outputs.pr-number }} at the reply call site

src/resolve-trigger-context/__tests__/workflow-security.test.ts

• Extend runCopyReference() with prNumber param + 3 new cases (empty, non-numeric, shell metacharacters)
• Add runStageReplyAgent() helper + 4 new cases for the reply staging step

Testing

pnpm lint (biome + tsc + actionlint) and pnpm test (1050 tests) pass clean.

Closes #112

[docker-agent]

Assessment: 🔴 CRITICAL

[aheritier]

Assessment: 🟡 NEEDS ATTENTION

Root cause is correct and well evidenced — I reproduced it from the verbose-log artifact of docker/sandboxes run 34488043917: the {pr} URL returned gh: Not Found (HTTP 404), the retry piped through jq '{id, state}' so no pullrequestreview- token ever reached the verbose log, and the ⚠️ Review did not complete notice (review 5168391083) landed 5s after the real review (5168390196). gh api really does only expand {owner}, {repo}, {branch}. Rendering the number at staging time and logging html_url is the right fix, and CI is green at 1417968.

Four things worth addressing before merge.

[should-fix] PR_NUMBER is substituted into an agent-executed snippet without validation — review-pr/action.yml:876,884-885,888

The adjacent PR_HEAD_SHA is regex-guarded before substitution; PR_NUMBER is not, and the post-render guard only detects a surviving {pr}, never a bad substitution. Executing the step body with mocked env:

• PR_NUMBER="" → gh api repos/{owner}/{repo}/pulls//reviews, step exits 0 — a silently invalid URL, i.e. the exact failure mode this PR fixes. (Not reachable today: resolve-context hard-fails on an empty number — defense-in-depth only.)
• PR_NUMBER='111 ; echo INJECTED' → pulls/111 ; echo INJECTED/reviews, step exits 0 — arbitrary shell in a snippet the agent executes. pr-number is a public composite-action input.
• PR_NUMBER='111/reviews' → sed aborts, exit 1 (fails closed, good).

if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then
  echo "::error::Resolved PR number is invalid; refusing to stage review posting"
  exit 1
fi

[should-fix] No test for the new substitution/guard — src/resolve-trigger-context/__tests__/workflow-security.test.ts:1315-1333

runCopyReference() already runs this exact step body, and the it.each table covers the SHA guard exhaustively (valid/empty/non-hex/short/long/unresolved). The helper never sets PR_NUMBER and no {pr} case was added — which is why the empty-value hole above is invisible to CI. Please extend the table (empty / non-numeric / valid) alongside the numeric guard.

[should-fix] The added | jq masks gh api failure — review-pr/agents/refs/posting-format.md:121-122

A pipeline exits with the last command's status and jq exits 0 on empty input (false | jq '{id, state, html_url}' → exit 0). The wild evidence shows this already biting: the 404'd attempt printed {"id": null, "state": null} and the agent moved on. Extending that pipe keeps the silent-failure class alive — consider set -o pipefail before the pipeline, or asserting .id != null.

[should-fix] Same defect remains in the reply path — review-pr/agents/pr-review-reply.yaml:76

gh api repos/{owner}/{repo}/pulls/{pr}/comments --input - has the identical root cause, and that file is not sed-rendered (only posting-format.md is), so every feedback reply still pays a guaranteed 404 + retry. Fine to defer, but worth a tracked follow-up — otherwise "eliminate the false double-posts" is only half done.

[optional] The PR description still describes commit 6c2d97c ("replace {pr} with $PR_NUMBER"), which 1417968 reverted — the final diff correctly keeps {pr} as the placeholder. Worth updating so the merged record isn't misleading.

[optional] The render guard covers only posting-format.md; other refs/*.md are staged verbatim, so a future {pr} elsewhere would ship unsubstituted. A grep -rq '{pr}' /tmp/refs/ guard would close that.

[derekmisler]

addressed all four should-fix items from the review in commit b513bd7:

[should-fix 1] PR_NUMBER numeric validation — added [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] guard before the sed substitution, mirroring the adjacent SHA guard. Empty, non-numeric, and shell-metacharacter values all exit 1.

[should-fix 2] Tests for the new substitution/guard — extended runCopyReference() to accept a prNumber parameter (default '5929') and added three new it.each rows: empty PR number, non-numeric, and PR number with shell metacharacters ('111; echo INJECTED'). All three correctly exit 1.

[should-fix 3] | jq masking gh api failure — added set -o pipefail before the posting pipeline in posting-format.md so a gh api 404 propagates as a non-zero exit instead of being swallowed by jq.

[should-fix 4] Same defect in reply path — filed issue #112 as a tracked follow-up. The reply path (pr-review-reply.yaml) is not sed-rendered, so fixing it requires a separate change. Keeping it out of this PR to keep the scope tight.

[optional] PR description — updated below to reflect the final diff (keeping {pr} as the placeholder).

[optional] Broader {pr} guard — changed grep -q '{pr}' /tmp/refs/posting-format.md to grep -rq '{pr}' /tmp/refs/ so any future {pr} in other staged ref files is also caught.