diff --git a/.agents/commands/pr-review-ci.md b/.agents/commands/pr-review-ci.md index bb296e7d..082861d1 100644 --- a/.agents/commands/pr-review-ci.md +++ b/.agents/commands/pr-review-ci.md @@ -55,10 +55,10 @@ Steps 3–6 produce: `` (sorted, deduplicated), `` (cou ## Step 7: Post the formal review (atomic) -Build a JSON object with all findings. Write to a PR-specific temp file: +Build a JSON object with all findings. Write it to a per-run temp file — never a shared `/tmp` literal, since concurrent reviews on the same host share `/tmp` and a parallel review could clobber it: ```bash -REVIEW_FILE="/tmp/pr-review-ci--comments.json" +REVIEW_FILE=$(mktemp "${TMPDIR:-/tmp}/pr-review-ci-comments.XXXXXX") || { echo "mktemp failed; cannot allocate the review payload path." >&2; exit 1; } ``` Structure: @@ -79,6 +79,8 @@ Structure: } ``` +Each `comments[].path` is the finding's `file` from the base output — already normalized to a repo-relative path (base Step 6 sub-step 1), which the GitHub reviews API requires (a `./`, `a/`/`b/`, or absolute prefix gets the comment rejected). + ### Verdict | Verdict | When | Event | diff --git a/.agents/commands/pr-review-gh.md b/.agents/commands/pr-review-gh.md index 41a9d9e1..6e69df30 100644 --- a/.agents/commands/pr-review-gh.md +++ b/.agents/commands/pr-review-gh.md @@ -47,7 +47,13 @@ Steps 3–6 produce: ``, ``, ``. ## Step 7: Post the review as `COMMENT` -Build a JSON object at `/tmp/pr-review-gh--comments.json`: +Mint a per-run path for the review payload — never a shared `/tmp` literal, since concurrent reviews on the same host share `/tmp` and a parallel review of the same PR would clobber it: + +```bash +REVIEW_FILE=$(mktemp "${TMPDIR:-/tmp}/pr-review-gh-comments.XXXXXX") || { echo "mktemp failed; cannot allocate the review payload path." >&2; exit 1; } +``` + +Build this JSON object in `$REVIEW_FILE` (each `comments[].path` is the finding's already-normalized repo-relative `file` from the base Step 6 — no `./`, `a/`, `b/`, or absolute prefix, which the GitHub reviews API requires): ```json { @@ -110,7 +116,7 @@ Use `CronCreate` to schedule a recurring job every 2 minutes (`*/2 * * * *`, rec Two kinds of placeholders in the watcher prompt: - **CronCreate-time placeholders** (substitute BEFORE CronCreate): ``, ``, ``, ``, ``, ``, ``. These seven are static. -- **Cycle-derived** (do NOT substitute): `${CYCLE_HEAD_SHA}`, `${CYCLE_HEAD_SHA_SHORT}`, `${CYCLE_PR_STATE}`, `${CYCLE_LAST_REVIEWED_SHA}`, `${CYCLE_MERGE_BASE}`, `${CYCLE_FAILED_AGENTS}`, `${CYCLE_LAST_REVIEWED_RAW}`. Computed inside each cycle. +- **Cycle-derived** (do NOT substitute): `${CYCLE_HEAD_SHA}`, `${CYCLE_HEAD_SHA_SHORT}`, `${CYCLE_PR_STATE}`, `${CYCLE_LAST_REVIEWED_SHA}`, `${CYCLE_LAST_REVIEW_INCOMPLETE}`, `${CYCLE_MERGE_BASE}`, `${CYCLE_FAILED_AGENTS}`, `${CYCLE_LAST_REVIEWED_RAW}`, `${CYCLE_FILE}` (the per-run cycle payload path from `mktemp`). Computed inside each cycle. **Pre-flight before CronCreate**: refuse empty/whitespace-only prompt; refuse if any of the seven static placeholders remain unsubstituted. @@ -154,10 +160,13 @@ CYCLE START: set CYCLE_LAST_REVIEWED_RAW = `gh api repos///pulls//reviews?per_page=100` If gh exit code != 0: abort cycle with WATCH_TRANSIENT_ERROR (do NOT fall through to "review everything"). set CYCLE_LAST_REVIEWED_SHA = `printf '%s' "${CYCLE_LAST_REVIEWED_RAW}" | jq --arg login "" -r '[.[] | select(.user.login == $login or ((.body // "") | test("Parallel PR Review|Code Review Summary")))] | sort_by(.submitted_at) | last | .commit_id // ""'` + set CYCLE_LAST_REVIEW_INCOMPLETE = `printf '%s' "${CYCLE_LAST_REVIEWED_RAW}" | jq --arg login "" -r '[.[] | select(.user.login == $login or ((.body // "") | test("Parallel PR Review|Code Review Summary")))] | sort_by(.submitted_at) | last | if . == null then false else ((.body // "") | test("review may be incomplete|REVIEW_INCOMPLETE")) end'` — "true" when the most recent matching review carried an agent-failure marker: the `review may be incomplete` warning (prepended on a with-findings or watcher-cycle post) OR the `REVIEW_INCOMPLETE` sentinel (the zero-findings-but-failures initial body, which does not carry the warning line). Match both so the zero-findings case — exactly when retry matters most — is not missed. If gh exit was zero AND ${CYCLE_LAST_REVIEWED_SHA} is empty: proceed with empty value (review everything on first sighting). 3. COMPARE SHA: - If ${CYCLE_HEAD_SHA} == ${CYCLE_LAST_REVIEWED_SHA}: say "Sentinel: WATCH_REVIEW_CLEAN — PR # still at ${CYCLE_HEAD_SHA_SHORT}, no new commits since last review." and end this cycle. + If ${CYCLE_HEAD_SHA} == ${CYCLE_LAST_REVIEWED_SHA}: + - If ${CYCLE_LAST_REVIEW_INCOMPLETE} is "true", the last review at this SHA was posted with an agent-failure warning — a failed agent is NOT a clean review (it may now succeed), so do NOT cache-hit. Say "Sentinel: WATCH_RETRY_INCOMPLETE — PR # re-reviewing ${CYCLE_HEAD_SHA_SHORT}: prior review was incomplete (agent failure)." and continue to step 4. + - Otherwise say "Sentinel: WATCH_REVIEW_CLEAN — PR # still at ${CYCLE_HEAD_SHA_SHORT}, no new commits since last review." and end this cycle. 4. NEW COMMIT DETECTED: Say "New commit detected on PR #: ${CYCLE_HEAD_SHA}. Running full review..." @@ -171,10 +180,10 @@ CYCLE START: The base produces: , ${CYCLE_FAILED_AGENTS}, . 6. POST REVIEW to GitHub as a single atomic call: - Build a JSON file at /tmp/pr-review-gh--cycle.json with commit_id=${CYCLE_HEAD_SHA} (NOT a CronCreate-time SHA), event="COMMENT", body (summary table), and comments[] array. + Mint a per-run path — set CYCLE_FILE = `mktemp "${TMPDIR:-/tmp}/pr-review-gh-cycle.XXXXXX"` (never a shared /tmp literal — concurrent watcher cycles or parallel workspaces would clobber it). Build the JSON file in ${CYCLE_FILE} with commit_id=${CYCLE_HEAD_SHA} (NOT a CronCreate-time SHA), event="COMMENT", body (summary table), and comments[] array (each comments[].path is the finding's already-normalized repo-relative file from the base Step 6). If ${CYCLE_FAILED_AGENTS} > 0, prepend "> WARNING: ${CYCLE_FAILED_AGENTS} of agents failed () — review may be incomplete." to the body. - Run: gh api repos///pulls//reviews --method POST --input /tmp/pr-review-gh--cycle.json — abort cycle if non-zero exit. - Clean up: rm -f /tmp/pr-review-gh--cycle.json + Run: gh api repos///pulls//reviews --method POST --input ${CYCLE_FILE} — abort cycle if non-zero exit. + Clean up: rm -f ${CYCLE_FILE} 7. Say "Sentinel: WATCH_REVIEW_DONE — PR # commit ${CYCLE_HEAD_SHA_SHORT}: findings (X critical, Y high, Z medium, W low)." @@ -203,4 +212,5 @@ After CronCreate returns the job ID: | `WATCH_TRANSIENT_ERROR` | Step 9 watcher (any cycle command) | `— step (): ` (any non-zero exit; permanent failures recur every cycle until CronDelete) | | `WATCH_PR_CLOSED` | Step 9 watcher Step 1 | `— PR # state=${CYCLE_PR_STATE}, watcher exiting.` | | `WATCH_REVIEW_CLEAN` | Step 9 watcher Step 3 | `— PR # still at ${CYCLE_HEAD_SHA_SHORT}, no new commits since last review.` | +| `WATCH_RETRY_INCOMPLETE` | Step 9 watcher Step 3 | `— PR # re-reviewing ${CYCLE_HEAD_SHA_SHORT}: prior review was incomplete (agent failure).` (same SHA, but the last review carried an agent-failure marker — re-review instead of cache-hitting) | | `WATCH_REVIEW_DONE` | Step 9 watcher Step 7 | `— PR # commit ${CYCLE_HEAD_SHA_SHORT}: findings (X critical, Y high, Z medium, W low).` | diff --git a/.agents/lib/pr-review-base.md b/.agents/lib/pr-review-base.md index 2d6e1c53..26c005fe 100644 --- a/.agents/lib/pr-review-base.md +++ b/.agents/lib/pr-review-base.md @@ -122,11 +122,11 @@ Persona specs live in `.agents/personas/*.md`. Each file has frontmatter declari Loop: -1. Read every file in `.agents/personas/*.md`. +1. Read **every** file in `.agents/personas/*.md` FIRST and resolve the complete launch set before issuing a single `Agent` call — never fire an agent the moment its file is read. 2. For each persona, decide whether to launch: - `kind: baseline` → always launch. - `kind: conditional` → launch only when the flag named in `trigger:` is true (see Step 4 for flag computation). -3. Launch ALL selected personas **in parallel** using the Agent tool (subagent_type: `"general-purpose"`). +3. Launch ALL selected personas in **one** assistant message — every `Agent` call (subagent_type: `"general-purpose"`) issued together so they run in parallel. Splitting the launches across multiple messages serializes the panel and defeats the parallelism the review is built around. 4. Track `` = count of personas actually launched (baseline + any fired conditionals). Shared per-agent contract (applied uniformly to every launched persona): @@ -167,13 +167,13 @@ Merge all agent results into a single list: - committed: `git diff --name-only $MERGE_BASE..` - plus uncommitted: `git diff --name-only HEAD` (only when `=local`) - For every agent finding, first guard `finding.file`: if it is missing, not a string, or empty, treat the finding as malformed and route it to sub-step 2's partial-failure handling instead of dropping it here. Otherwise, compare `finding.file` against `` after path normalization: + For every agent finding, first guard `finding.file`: if it is missing, not a string, or empty, treat the finding as malformed and route it to sub-step 2's partial-failure handling instead of dropping it here. Otherwise, compute the normalized path once and compare it against ``: - Strip any leading `./`. - Strip diff prefixes `a/` and `b/` if present. - If the agent returned an absolute path, strip the repo-root prefix (`git rev-parse --show-toplevel`) before compare. - Case-sensitive compare (matches git's default). - If `finding.file` is not in ``, **drop the finding** and increment ``. + If the normalized path is not in ``, **drop the finding** and increment ``. Otherwise keep the finding and **write the normalized path back into its `file`** — this is the value the caller passes straight into the GitHub review `comments[].path`, which **must** be repo-relative. A leftover `./`, `a/`/`b/` diff prefix, or absolute path makes the GitHub reviews API reject that comment and collapse the atomic review into a plain PR-level comment. Do NOT filter by line number within a changed file. The Step 5 contract permits flagging adjacent code in a changed file when the diff materially worsens it, so line-level filtering would discard legitimate findings. @@ -207,7 +207,7 @@ Severity labels (used everywhere downstream): The caller (Step 7 of `/pr-review-ci` / `/pr-review-gh` / `/pr-review-local`) consumes: -- `` — sorted, deduplicated array of `{severity, file, line, description}`. +- `` — sorted, deduplicated array of `{severity, file, line, description}`. `file` is normalized to a repo-relative path (Step 6 sub-step 1), so callers can pass it directly into the GitHub review `comments[].path`. - `` — count + names of agents that returned `agent_error` or malformed output. - `` — `{critical, high, medium, low}` totals. - `` — count of personas that actually fired (baseline always-fire count + any conditional personas whose trigger flag was true). Used by the caller's report to phrase " of agents failed" correctly when conditional personas did not fire.