diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index 11342283c6..43345deb33 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -1,6 +1,6 @@ --- name: pr-fixup -description: Wait for CI and automated reviews on a PR, fix valid failures and comments in the primary conversation, verify, and push. +description: Wait for CI and automated reviews on a PR, fix valid failures and comments, disposition every review thread, verify, and push in the primary conversation. --- # PR Fixup @@ -24,8 +24,8 @@ Create a visible checklist: 1. Gather PR state 2. Resolve an authorized PR merge conflict 3. Fix failing CI checks -4. Triage review comments -5. Address valid comments +4. Triage every review thread +5. Apply valid fixes and record every disposition 6. Commit, rerun affected checks, and push 7. Re-check the new head 8. Report @@ -114,9 +114,9 @@ approve` is not a valid command. Treat the state as clean only when the current head has no failed or pending checks, no merge conflict, no blocking review (an active `CHANGES_REQUESTED` -or a review blocked at the exact current head), no actionable review thread or -issue comment, and qualifying exact-head semantic evidence where PR delivery -requires it. +or a review blocked at the exact current head), no unresolved review thread, no +actionable issue comment, and qualifying exact-head semantic evidence where PR +delivery requires it. ## 2. Fix CI Failures @@ -178,33 +178,49 @@ when the behavior remains internal. ## 3. Triage And Address Reviews Use `scripts/pr-resolve list ` to obtain unresolved threads. Its previews -can be truncated, so expand each listed review thread with -`scripts/pr-resolve show ` before deciding whether it is valid, -already addressed, a preference, or wrong for this codebase. Use -`scripts/pr-state --comment ` only for a flat comment view when a -thread context is not available. Validate against the current head, the spec, +can be truncated, so expand every listed review thread with +`scripts/pr-resolve show ` before deciding how to handle it. +Use `scripts/pr-state --comment ` only for a flat comment view when +a thread context is not available. Validate against the current head, the spec, and existing architecture before editing or replying. -Make only valid changes. GitHub replies and thread resolution are external -writes. A direction to "address" valid review comments explicitly authorizes a -concise reply and resolution after the fix is pushed and targeted verification -passes; a review-only request does not. For an invalid comment, reply with -concrete reasoning only when that authorization includes a response. When -writes are not authorized, report valid comments as addressed in code but still -unresolved; do not declare the PR clean solely from the code change. Resolve an -authorized thread only when the change or response genuinely addresses it. -An explicit request to "address them" authorizes replies and resolution only -for the selected current actionable threads. If new comments appear during -remediation, report them or obtain separate confirmation before replying or -resolving them. +Every unresolved thread requires an explicit disposition, regardless of its +author, bot identity, visibility, or apparent severity. Record exactly one of +these dispositions: -After an authorized fix is pushed, use the atomic helper path +- `actionable`: make the valid code or test change, verify it, cite the + resulting commit in the reply, and resolve the thread when writes are + authorized. +- `already addressed`: verify the current implementation or an existing commit, + explain where it is addressed, and resolve the thread when writes are + authorized. +- `informational` or `optional`: make no code change, acknowledge the information + or suggestion in a reply, and resolve it when complete cleanup is requested. +- `invalid`: do not make an invalid code change or silently ignore the finding; + reply with concrete reasoning grounded in the code, spec, or architecture, + then resolve it only after that pushback is posted and writes are authorized. + +A classification is not a disposition. A thread is not complete merely because +it was called optional, informational, already addressed, or invalid. GitHub +replies and thread resolution are external writes. A request such as "complete +cleanup", "clean up all review threads", or "leave no threads unresolved" +explicitly authorizes a concise reply and resolution for every unresolved +thread, including informational, optional, and invalid findings. A request to +address selected comments authorizes replies and resolution only for those +selected threads. A review-only request authorizes no writes. When writes are +not authorized, record every disposition, report the still-unresolved thread, +and do not declare the PR clean. + +For code changes, push the fix and pass targeted verification before replying +and resolving. For non-code dispositions, verify the current head first, then +use the atomic helper path `scripts/pr-resolve reply --body-file ` to reply, resolve, and react in one operation when the body contains Markdown or shell metacharacters. For short plain-text bodies, a safely quoted argument is acceptable. Never interpolate review text into an unquoted shell command or use backticks in the command itself. After the helper returns, re-fetch the -thread and verify the posted reply body before treating the write as complete. +thread and verify the posted reply body and resolved state before treating the +disposition as complete. Then rerun `scripts/pr-resolve list ` and the exact-head `scripts/pr-state --summary ` check before reporting. @@ -270,22 +286,25 @@ non-empty body in `review_evidence.exact_current_head_reviews[]`, even when Classify current-head review bodies and top-level bot/issue comments before declaring the PR clean; empty thread and issue-comment counts are insufficient. Treat a non-empty `hidden_unresolved_threads` value in that fresh snapshot as a -mandatory hidden-thread gate: run `scripts/pr-resolve list ` again after -the refresh and immediately before reporting. +mandatory hidden-thread gate: expand and disposition each hidden thread, then +run `scripts/pr-resolve list ` again after the refresh and immediately +before reporting. Require `checks_head_sha` to match that head, report pending checks separately from failures, and rerun `scripts/pr-resolve list ` before declaring the PR clean. The final predicate must also require `hidden_unresolved_threads=[]`; do not require filtered and unresolved counts to be equal because the filtered count includes resolved threads. Treat prior review -evidence as stale. When the user authorized thread -writes, a duplicate or stale bot thread still needs an explicit reply and -resolution once current source proves the finding is already fixed, including a -thread surfaced only in `hidden_unresolved_threads`; only current-head -actionable threads drive code changes. Declare the PR clean only when the +evidence as stale. When the user authorized thread writes, every unresolved +thread still needs an explicit reply and resolution matching its disposition. +A duplicate or stale bot thread needs the same treatment once current source +proves the finding is already fixed, including a thread surfaced only in +`hidden_unresolved_threads`; only current-head actionable threads drive code +changes. Declare the PR clean only when the exact-current-head review classification reports no unaddressed findings, `checks_snapshot_complete=true`, `failed_checks=[]`, `pending_checks=[]`, `approval_required_runs=[]`, `actionable_issue_comment_count=0`, -`hidden_unresolved_threads=[]`, there is no merge conflict, and +`unresolved_review_thread_count=0`, `hidden_unresolved_threads=[]`, there is no +merge conflict, and `scripts/pr-resolve list ` is empty. Within the user's monitoring limit, continue checking after resolutions until automated review jobs are terminal; otherwise report the exact pending check names. @@ -313,7 +332,7 @@ differs. The phrase "ready to merge" is reserved for a fresh current-head snapshot with every required check successful or explicitly skipped, no pending or -failed leaf or aggregate check, no blocking review/thread, no merge conflict, +failed leaf or aggregate check, no unresolved review thread, no merge conflict, and the local, upstream, and PR head OIDs aligned. A clean local reproduction does not waive a failed remote check; push the remediation and re-check the new head first. @@ -334,4 +353,7 @@ requested and through a worktree-safe cleanup flow. - Do not use native delegation or a full-history context fork to poll CI. - Do not push, post comments, or resolve threads when the user asked for review only. +- Do not silently dismiss an invalid finding; every invalid thread needs a + concrete pushback disposition, and an authorized complete cleanup must post + that explanation before resolving it. - Do not proceed with an unverified PR when mandatory verification is blocked. diff --git a/.agents/skills/pr-fixup/references/review-evidence.md b/.agents/skills/pr-fixup/references/review-evidence.md index fb01036eb3..e0060b3f87 100644 --- a/.agents/skills/pr-fixup/references/review-evidence.md +++ b/.agents/skills/pr-fixup/references/review-evidence.md @@ -14,7 +14,8 @@ historical audit. The summary fields are: - `failed_checks` and `pending_checks`: actionable check state and run/job URLs. -- `unresolved_review_thread_count`: all unresolved threads; use +- `unresolved_review_thread_count`: all unresolved threads, regardless of + classification; every one is a completion gate. Use `hidden_unresolved_threads` as blockers even outside the current-head filter. - `review_evidence`, `checks_head_sha`, and `checks_snapshot_complete`: exact head evidence. A head race or incomplete fetch is unknown, never inferred @@ -32,9 +33,27 @@ for forks, security, or architecture. Inspect every non-empty body in `review_evidence.exact_current_head_reviews[]`, including `COMMENTED` aggregate-bot reviews, and classify it as actionable, already addressed, -optional, or invalid before declaring reviews clear. `trusted_producer=true` -qualifies only for the dedicated OpenCode App, never merely because a reviewer -name matches. +informational, optional, or invalid before declaring reviews clear. +`trusted_producer=true` qualifies only for the dedicated OpenCode App, never +merely because a reviewer name matches. + +For review threads, classification and disposition are separate. Expand every +unresolved thread with `scripts/pr-resolve show ` and record one +of these outcomes: implement and verify an actionable finding; explain where an +already-addressed, duplicate, or stale finding is handled; acknowledge an +informational or optional suggestion; or give concrete code/spec/architecture +reasoning for an invalid finding. Do not treat a label, internal note, or lack +of code change as a completed disposition. + +When the user requests complete cleanup, including wording such as "clean up +all review threads" or "leave no threads unresolved", reply to and resolve +every unresolved thread after its disposition. This includes informational and +optional threads, which need an acknowledgement, and invalid threads, which +need the concrete pushback reply before resolution. A request limited to +selected actionable comments does not authorize writes to other threads. If +thread writes are not authorized, report each disposition and keep the thread +unresolved; never report the review state as clean. An invalid finding must +never be silently ignored. If `gh`, `scripts/pr-state`, or `scripts/pr-resolve` fails with an authentication or transport error (including a broker 401), do not treat empty @@ -94,10 +113,10 @@ scripts/pr-resolve list Transport/collection failure leaves checks unknown. Parseable pending/failing rows remain usable when `gh pr checks` exits 8; never hide diagnostics in a pipe. -When `hidden_unresolved_threads` is non-empty, fetch each thread body with -`scripts/pr-resolve show `; use the flat comment command only -for a comment without thread context. A listed thread that is already resolved -is stale summary state: re-poll and do not reply again. +When `hidden_unresolved_threads` is non-empty, fetch and disposition each thread +body with `scripts/pr-resolve show `; use the flat comment command +only for a comment without thread context. A listed thread that is already +resolved is stale summary state: re-poll and do not reply again. Poll at 30-second cadence with a 20-minute cap using bounded one-shot commands; avoid long inline loops and `gh pr checks --watch`. In default monitoring, @@ -108,7 +127,9 @@ Queued/in-progress jobs are pending, not speculative-fix triggers. On an explicit wait-through-CI request without a fixed deadline, use the same 20-minute absolute cap: repeat bounded checks until failures, pending checks, and unresolved-thread count are all empty/zero, or stop at the deadline and -report remaining pending checks or unresolved threads. Preserve early stopping +report remaining pending checks or unresolved threads. A nonzero unresolved +count is a blocker even when every remaining thread is informational, optional, +or invalid. Preserve early stopping for failures and merged/closed or access-revoked conditions. For E2E-only pending work, summarize a saved snapshot before printing shards: