From ed0ee2eddc8791f0183760e3ad2b596bd77e7af5 Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Mon, 20 Jul 2026 03:35:07 -0700 Subject: [PATCH 1/5] Calibrate holistic review complexity guidance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/holistic-review.lock.yml | 2 +- .github/workflows/holistic-review.md | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/holistic-review.lock.yml b/.github/workflows/holistic-review.lock.yml index c391e2c878668d..e2ea00e603116b 100644 --- a/.github/workflows/holistic-review.lock.yml +++ b/.github/workflows/holistic-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7be581e4246d39b0d5fc4bfbbcb93665ed9ab8e27bcdc93d16e05d7003e9a9a6","body_hash":"a7b409e3f0101dbacaff1d418694b134b6b99c53f167c8e2092de111d4c2e9b8","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7be581e4246d39b0d5fc4bfbbcb93665ed9ab8e27bcdc93d16e05d7003e9a9a6","body_hash":"949f297dc94e7de7a7fe4ac47bb09b0c78a70795d0c6fef2e613dd3492b04b48","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"cec6394202d7db187b02310d928812194988eb20","version":"v0.82.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27","digest":"sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27@sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27","digest":"sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27@sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27","digest":"sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27@sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27","digest":"sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27@sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.0","digest":"sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.0@sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw (v0.82.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/.github/workflows/holistic-review.md b/.github/workflows/holistic-review.md index af08066094edcb..fdb06f0d3e64f4 100644 --- a/.github/workflows/holistic-review.md +++ b/.github/workflows/holistic-review.md @@ -391,6 +391,14 @@ This dispatched worker has no sub-agent or task tooling. Skip the skill's `Disco Follow the review skill for the range selected in Step 1. Consult existing PR comments and reviews as directed by the skill, but do not modify, hide, supersede, or otherwise remove prior comments or reviews. +Explicitly assess whether the PR's added complexity is necessary and proportionate to its validated +goal. Do not treat size, low-level code, or specialized algorithms as concerns by themselves when +the problem inherently requires them and the design is well-factored, tested, and consistent with +established direction. Escalate only when a materially simpler approach meets the same requirements, +the complexity is poorly encapsulated or duplicative, or the demonstrated benefit is too narrow to +justify the maintenance burden. When the tradeoff remains unresolved, use `⚠️ Needs Human Review` +and state the specific decision a maintainer should make. + Use the review skill's exact top-level body structure. After `## Holistic Review`, immediately emit `**Motivation**:`, `**Approach**:`, and `**Summary**:` in that order. Do not add a `### Holistic Assessment` subheading, substitute a `Verdict` field, or rename those fields. For each actionable finding that is specific to one changed line or a contiguous changed range, invoke the `create_pull_request_review_comment` safe output before submitting the review. Use the dispatched `pull_request_number`, the changed file path, and the exact right-side line or range. Put the complete actionable explanation in that inline comment. Do not create inline comments for unchanged lines, broad/cross-cutting findings, non-actionable observations, or findings without a precise changed location; include those only in the visible `### Detailed Findings` section of the review body. Do not duplicate a finding's full explanation in both places: identify inline findings briefly in the body and link to the relevant file and line when possible. From 37f8b3c28b0a65f9dba9302c2f063da081427a9b Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Mon, 20 Jul 2026 03:35:07 -0700 Subject: [PATCH 2/5] Refine holistic review output Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/code-review/SKILL.md | 35 +++++++++++++++------- .github/workflows/holistic-review.lock.yml | 2 +- .github/workflows/holistic-review.md | 20 +++++++++---- 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 0c73f1b11efb72..8f5ecbb84697be 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -55,7 +55,7 @@ Based **only** on the code context gathered above (without the PR description or 3. **Is this the right approach?** Would a simpler alternative be more consistent with the codebase? Could the goal be achieved with existing functionality? Are there correctness, performance, or safety concerns? 4. **What problems do you see?** Identify bugs, edge cases, missing validation, thread-safety issues, performance regressions, API design problems, test gaps, and anything else that concerns you. -Write down your independent assessment before proceeding. You must produce a holistic assessment (using the criteria from the applicable `.github/instructions/*.instructions.md` files for the diff) at this stage. +Write down your independent assessment before proceeding. You must produce a holistic assessment (using the criteria from the applicable `.github/instructions/**/*.instructions.md` files for the diff) at this stage. ### Step 4: Incorporate PR Narrative and Reconcile @@ -113,7 +113,7 @@ When the environment supports launching sub-agents with different models (e.g., When presenting the final review (whether as a PR comment or as output to the user), use the following structure. This ensures consistency across reviews and makes the output easy to scan. -> 📝 **AI-generated content disclosure:** When posting review content to GitHub (PR review comments, PR comments) under a user's credentials — i.e., the account is **not** a dedicated "copilot" or "bot" account/app (e.g., `github-actions[bot]`, `copilot`) — you **MUST** include a concise, visible note (e.g. a `> [!NOTE]` alert) at the bottom of the content indicating the content was AI/Copilot-generated. Skip this if the user explicitly asks you to omit it. +> **AI-generated content disclosure:** When posting review content to GitHub (PR review comments, PR comments) under a user's credentials — i.e., the account is **not** a dedicated "copilot" or "bot" account/app (e.g., `github-actions[bot]`, `copilot`) — you **MUST** include a concise, visible note (e.g. a `> [!NOTE]` alert) at the bottom of the content indicating the content was AI/Copilot-generated. Skip this if the user explicitly asks you to omit it. ### Structure @@ -124,13 +124,13 @@ When presenting the final review (whether as a PR comment or as output to the us **Approach**: <1-2 sentences on whether the fix/change takes the right approach> -**Summary**: <✅ LGTM / ⚠️ Needs Human Review / ⚠️ Needs Changes / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points. If "Needs Human Review," explicitly state which findings you are uncertain about and what a human reviewer should focus on.> +**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points. If "Needs Human Review," explicitly state which findings are uncertain and what a human reviewer should focus on.> --- ### Detailed Findings -#### ✅/⚠️/❌ — +#### ✅/⚠️/💡/❌ — @@ -149,26 +149,39 @@ When presenting the final review (whether as a PR comment or as output to the us - **Detailed Findings** uses emoji-prefixed category headers: - ✅ for things that are correct / look good (use to confirm important aspects were verified) - ⚠️ for warnings or impactful suggestions (should fix, or follow-up) - - ❌ for errors (must fix before merge) - 💡 for minor suggestions or observations (nice-to-have) + - ❌ for errors (must fix before merge) - **Cross-cutting analysis** should be included when relevant: check whether related code (sibling types, callers, other platforms) is affected by the same issue or needs a similar fix. - **Test quality** should be assessed as its own finding when tests are part of the PR. -- **Summary** gives a clear verdict: LGTM (no blocking issues — use only when confident), Needs Human Review (code may be correct but you have unresolved concerns or uncertainty that require human judgment), Needs Changes (with blocking issues listed), or Reject (explaining why this should be closed outright). **Never give a blanket LGTM when you are unsure.** When in doubt, use "Needs Human Review" and explain what a human should focus on. +- **Summary** gives a clear verdict: `❌ Needs Changes`, `⚠️ Needs Human Review`, + `💡 Suggestions`, `✅ LGTM`, or `❌ Reject`. Use `✅ LGTM` only when confident and no + suggestions remain. When uncertain, use `⚠️ Needs Human Review` and explain what a human + should focus on. - Keep the review concise but thorough. Every claim should be backed by evidence from the code. ### Verdict Consistency Rules The summary verdict **must** be consistent with the findings in the body. Follow these rules: -1. **The verdict must reflect your most severe finding.** If you have any ⚠️ findings, the verdict cannot be "LGTM." Use "Needs Human Review" or "Needs Changes" instead. Only use "LGTM" when all findings are ✅ or 💡 and you are confident the change is correct and complete. +1. **The verdict must reflect your most severe finding.** If you have any `⚠️` or `❌` + findings, the verdict cannot be `✅ LGTM`. Use `⚠️ Needs Human Review`, `❌ Needs + Changes`, or `❌ Reject` instead. If the only findings are `💡`, use `💡 Suggestions`. + Only use `✅ LGTM` when all findings are ✅ and you are confident the change is correct and complete. -2. **When uncertain, always escalate to human review.** If you are unsure whether a concern is valid, whether the approach is sufficient, or whether you have enough context to judge, the verdict must be "Needs Human Review" — not LGTM. Your job is to surface concerns for human judgment, not to give approval when uncertain. A false LGTM is far worse than an unnecessary escalation. +2. **When uncertain, always escalate to human review.** If you are unsure whether a concern + is valid, whether the approach is sufficient, or whether you have enough context to + judge, the verdict must be `⚠️ Needs Human Review` — not `✅ LGTM`. 3. **Separate code correctness from approach completeness.** A change can be correct code that is an incomplete approach. If you believe the code is right for what it does but the approach is insufficient (e.g., treats symptoms without investigating root cause, silently masks errors that should be diagnosed, fixes one instance but not others), the verdict must reflect the gap — do not let "the code itself looks fine" collapse into LGTM. -4. **Classify each ⚠️ and ❌ finding as merge-blocking or advisory.** Before writing your summary, decide for each finding: "Would I be comfortable if this merged as-is?" If any answer is "no," the verdict must be "Needs Changes." If any answer is "I'm not sure," the verdict must be "Needs Human Review." +4. **Classify each `⚠️` and `❌` finding as merge-blocking or advisory.** Before writing + the summary, decide for each finding whether it would be acceptable to merge as-is. If + any answer is no, the verdict must be `❌ Needs Changes`; if any answer is uncertain, + it must be `⚠️ Needs Human Review`. -5. **Devil's advocate check before finalizing.** Re-read all your ⚠️ findings. For each one, ask: does this represent an unresolved concern about the approach, scope, or risk of masking deeper issues? If so, the verdict must reflect that tension. Do not default to optimism because the diff is small or the code is obviously correct at a syntactic level. +5. **Devil's advocate check before finalizing.** Re-read every `⚠️` finding. If it + represents an unresolved concern about the approach, scope, or risk of masking deeper + issues, the verdict must reflect that tension. --- @@ -183,7 +196,7 @@ review**, in addition to the process above. Load, based on the paths in the diff: -- **`src/**` changed:** `.github/instructions/review-all-src.instructions.md` -- reviewer mindset, the Holistic PR Assessment criteria (Motivation, Evidence, Approach, Cost-Benefit, Scope, Risk, Codebase Fit), correctness philosophy, PR hygiene, consistency, and documentation. Use these criteria to write the Motivation, Approach, and Summary fields in your output. +- **`src/**` changed:** `.github/instructions/review-all-src.instructions.md` -- reviewer mindset, the Holistic PR Assessment criteria (Motivation, Evidence, Approach, Cost-Benefit, Scope, Risk, Codebase Fit), correctness philosophy, PR hygiene, consistency, and documentation. Use these criteria to write the Motivation, Approach, and Summary fields in the output. - **`**/*.cs` changed:** `.github/instructions/review-csharp.instructions.md` -- C# error handling, thread safety, security, correctness, performance/allocation, API design, and style rules. - **Native files (`*.c` / `*.cpp` / `*.h` / `*.inc` / `*.S` / `*.asm`) changed:** `.github/instructions/review-native.instructions.md` -- C++ style, VM/JIT contracts, GC protection, platform defines, and interop/marshalling rules. - **Test files (`**/tests/**`, `src/tests/**`) changed:** `.github/instructions/review-all-tests.instructions.md` -- testing conventions and regression-test requirements. diff --git a/.github/workflows/holistic-review.lock.yml b/.github/workflows/holistic-review.lock.yml index e2ea00e603116b..da272cf4008f13 100644 --- a/.github/workflows/holistic-review.lock.yml +++ b/.github/workflows/holistic-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7be581e4246d39b0d5fc4bfbbcb93665ed9ab8e27bcdc93d16e05d7003e9a9a6","body_hash":"949f297dc94e7de7a7fe4ac47bb09b0c78a70795d0c6fef2e613dd3492b04b48","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7be581e4246d39b0d5fc4bfbbcb93665ed9ab8e27bcdc93d16e05d7003e9a9a6","body_hash":"8f6493cd30dba38a165e7991ee9b6733a55e55a2d7e120637edc702d3cc7c620","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"cec6394202d7db187b02310d928812194988eb20","version":"v0.82.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27","digest":"sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27@sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27","digest":"sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27@sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27","digest":"sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27@sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27","digest":"sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27@sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.0","digest":"sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.0@sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw (v0.82.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/.github/workflows/holistic-review.md b/.github/workflows/holistic-review.md index fdb06f0d3e64f4..62472eb09e5e00 100644 --- a/.github/workflows/holistic-review.md +++ b/.github/workflows/holistic-review.md @@ -364,7 +364,7 @@ actual base-to-head range, not its head compared with the current state of `main For a re-review, use two distinct scopes: -1. Read the complete current PR range `$HOLISTIC_REVIEW_CURRENT_MERGE_BASE_SHA..$HOLISTIC_REVIEW_HEAD_SHA` only to refresh the cumulative assessment. Compare it with the prior review(s) so the summary accurately reflects the current motivation, approach, risk, and overall verdict after the PR has evolved. `HOLISTIC_REVIEW_PREVIOUS_REVIEW_HISTORY` is the authoritative JSON array containing the initial workflow review and the most recent workflow review, with `{ commit, review_id }` entries. Retrieve each listed review by ID; do not try to discover history from the broader bot review list. In the new review body, add one **Assessment History** bullet for each entry. Each bullet must include a Markdown permalink in the form `[review ](${{ github.server_url }}/${{ github.repository }}/pull/${{ github.event.inputs.pr_number }}#pullrequestreview-)`, identify its reviewed commit, and state its verdict, the current verdict, and whether the assessment is unchanged or changed. Only call an assessment unchanged when its verdict, motivation, approach, and risk assessment are all unchanged. For each changed assessment, explain how the PR patch changes identified below caused the change. +1. Read the complete current PR range `$HOLISTIC_REVIEW_CURRENT_MERGE_BASE_SHA..$HOLISTIC_REVIEW_HEAD_SHA` only to refresh the cumulative assessment. Compare it with the prior review(s) so the summary accurately reflects the current motivation, approach, risk, and overall verdict after the PR has evolved. `HOLISTIC_REVIEW_PREVIOUS_REVIEW_HISTORY` is the authoritative JSON array containing the initial workflow review and the most recent workflow review, with `{ commit, review_id }` entries. Retrieve each listed review by ID; do not try to discover history from the broader bot review list. Use the prior reviews only to compare the current assessment. Only call an assessment unchanged when its verdict, motivation, approach, and risk assessment are all unchanged. For each changed assessment, explain how the PR patch changes identified below caused the change. 2. Read `$HOLISTIC_REVIEW_SCOPE_DIR/range-diff.txt` as the primary commit-level explanation of added, removed, or modified PR patches. Read `$HOLISTIC_REVIEW_SCOPE_DIR/patch-diff.txt` to capture merge-conflict resolutions and other changes that `range-diff` cannot represent. These files compare cumulative patches using the historical and current merge bases recorded in `metadata.json`. Restrict all new detailed and actionable findings to changes between those previous and current PR patches. Do not use `git diff "$HOLISTIC_REVIEW_PREVIOUS_HEAD_SHA" HEAD` to determine the incremental scope: after a rebase, that tree comparison includes unrelated upstream changes. Do not introduce a finding about code that was already part of the PR at `$HOLISTIC_REVIEW_PREVIOUS_HEAD_SHA`, even if an earlier review missed it. Inline findings must point to lines in the current base-to-head diff. The refreshed assessment may explain how the cumulative PR changed, but must not turn an issue in unchanged code into a new finding. If the previous and current head commits are identical but the merge base changed, the PR was @@ -391,17 +391,25 @@ This dispatched worker has no sub-agent or task tooling. Skip the skill's `Disco Follow the review skill for the range selected in Step 1. Consult existing PR comments and reviews as directed by the skill, but do not modify, hide, supersede, or otherwise remove prior comments or reviews. -Explicitly assess whether the PR's added complexity is necessary and proportionate to its validated -goal. Do not treat size, low-level code, or specialized algorithms as concerns by themselves when -the problem inherently requires them and the design is well-factored, tested, and consistent with +Explicitly assess whether the PR's added complexity is necessary and proportionate to a validated +goal. A validated goal is supported by at least one concrete evidence source: an approved API, +specification, or accepted design requirement; a reproducible bug or regression; representative +benchmark or performance evidence; or customer, CI, production, or similarly concrete evidence. +Do not treat size, low-level code, or specialized algorithms as concerns by themselves when the +problem inherently requires them and the design is well-factored, tested, and consistent with established direction. Escalate only when a materially simpler approach meets the same requirements, the complexity is poorly encapsulated or duplicative, or the demonstrated benefit is too narrow to justify the maintenance burden. When the tradeoff remains unresolved, use `⚠️ Needs Human Review` and state the specific decision a maintainer should make. -Use the review skill's exact top-level body structure. After `## Holistic Review`, immediately emit `**Motivation**:`, `**Approach**:`, and `**Summary**:` in that order. Do not add a `### Holistic Assessment` subheading, substitute a `Verdict` field, or rename those fields. +Use the review skill's exact top-level body structure. After `## Holistic Review`, immediately emit +`**Motivation**:`, `**Approach**:`, and `**Summary**:` in that order. Then emit `### Detailed +Findings`. Do not restate the PR title, its description, or obvious code behavior unless the code +has a meaningful semantic difference from the stated intent. Do not duplicate findings or +assessments across the summary, detailed findings, or inline comments. Use only `✅`, `⚠️`, `💡`, +and `❌` as review-content status emojis. -For each actionable finding that is specific to one changed line or a contiguous changed range, invoke the `create_pull_request_review_comment` safe output before submitting the review. Use the dispatched `pull_request_number`, the changed file path, and the exact right-side line or range. Put the complete actionable explanation in that inline comment. Do not create inline comments for unchanged lines, broad/cross-cutting findings, non-actionable observations, or findings without a precise changed location; include those only in the visible `### Detailed Findings` section of the review body. Do not duplicate a finding's full explanation in both places: identify inline findings briefly in the body and link to the relevant file and line when possible. +For each actionable finding that is specific to one changed line or a contiguous changed range, invoke the `create_pull_request_review_comment` safe output before submitting the review. Use the dispatched `pull_request_number`, the changed file path, and the exact right-side line or range. Put the complete actionable explanation in that inline comment. Do not create inline comments for unchanged lines, broad/cross-cutting findings, non-actionable observations, or findings without a precise changed location; include those only in the visible `### Detailed Findings` section of the review body. Do not duplicate a finding's full explanation in both places: identify inline findings briefly in the body and link to the relevant file and line when useful. Safe outputs are CLI-mounted by `tools.cli-proxy`. Invoke each safe output as one shell command whose executable is `safeoutputs`, passing exactly one JSON object through a single-quoted here-document: From f9dedac209fdfaf465c55750b4c0ed5b6f3a7ca5 Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Mon, 20 Jul 2026 03:35:07 -0700 Subject: [PATCH 3/5] Move holistic review state to an issue Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/code-review/SKILL.md | 23 +- .../workflows/holistic-review-completed.yml | 89 ++ .../holistic-review-orchestrator.yml | 763 ++---------------- .github/workflows/holistic-review.lock.yml | 84 +- .github/workflows/holistic-review.md | 73 +- .../holistic-review/orchestrator-dispatch.sh | 551 +++++++++++++ .../orchestrator-get-state-issue.sh | 98 +++ .../orchestrator-reconcile-state.sh | 168 ++++ .../orchestrator-upsert-state.sh | 104 +++ 9 files changed, 1199 insertions(+), 754 deletions(-) create mode 100644 .github/workflows/holistic-review-completed.yml create mode 100644 .github/workflows/scripts/holistic-review/orchestrator-dispatch.sh create mode 100644 .github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh create mode 100644 .github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh create mode 100644 .github/workflows/scripts/holistic-review/orchestrator-upsert-state.sh diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 8f5ecbb84697be..201f533c06d6ef 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -120,11 +120,11 @@ When presenting the final review (whether as a PR comment or as output to the us ``` ## Holistic Review -**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real> +**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real, when this adds non-obvious context> -**Approach**: <1-2 sentences on whether the fix/change takes the right approach> +**Approach**: <1-2 sentences on whether the fix/change takes the right approach, when this adds non-obvious context> -**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points. If "Needs Human Review," explicitly state which findings are uncertain and what a human reviewer should focus on.> +**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points, when it adds non-obvious context. If "Needs Human Review," explicitly state which findings are uncertain and what a human reviewer should focus on.> --- @@ -142,10 +142,12 @@ When presenting the final review (whether as a PR comment or as output to the us ### Guidelines -- Begin the review body with `## Holistic Review`, immediately followed by the - `**Motivation**:`, `**Approach**:`, and `**Summary**:` fields in that order. Do not - add a `### Holistic Assessment` subheading, substitute a `Verdict` field, or rename - those fields. +- Begin the review body with `## Holistic Review`. Include `**Motivation**:`, + `**Approach**:`, and `**Summary**:` only when each adds non-obvious assessment beyond + the PR title, description, and code changes; omit any field that would merely restate + the obvious. When present, keep those fields in that order. Do not add a + `### Holistic Assessment` subheading, substitute a `Verdict` field, or rename those + fields. - **Detailed Findings** uses emoji-prefixed category headers: - ✅ for things that are correct / look good (use to confirm important aspects were verified) - ⚠️ for warnings or impactful suggestions (should fix, or follow-up) @@ -153,9 +155,10 @@ When presenting the final review (whether as a PR comment or as output to the us - ❌ for errors (must fix before merge) - **Cross-cutting analysis** should be included when relevant: check whether related code (sibling types, callers, other platforms) is affected by the same issue or needs a similar fix. - **Test quality** should be assessed as its own finding when tests are part of the PR. -- **Summary** gives a clear verdict: `❌ Needs Changes`, `⚠️ Needs Human Review`, - `💡 Suggestions`, `✅ LGTM`, or `❌ Reject`. Use `✅ LGTM` only when confident and no - suggestions remain. When uncertain, use `⚠️ Needs Human Review` and explain what a human +- When included, **Summary** gives a clear verdict: `❌ Needs Changes`, `⚠️ Needs Human + Review`, `💡 Suggestions`, `✅ LGTM`, or `❌ Reject`. Use `✅ LGTM` only when confident + and no suggestions remain. When Summary is omitted, make the verdict clear through the + detailed findings. When uncertain, use `⚠️ Needs Human Review` and explain what a human should focus on. - Keep the review concise but thorough. Every claim should be backed by evidence from the code. diff --git a/.github/workflows/holistic-review-completed.yml b/.github/workflows/holistic-review-completed.yml new file mode 100644 index 00000000000000..e693f556ccbb42 --- /dev/null +++ b/.github/workflows/holistic-review-completed.yml @@ -0,0 +1,89 @@ +name: Holistic Review Completed + +on: + workflow_call: + inputs: + pr_number: + description: 'Pull request number reviewed by the worker.' + required: true + type: string + head_sha: + description: 'Pull request head commit assessed by the worker.' + required: true + type: string + outcome: + description: 'One of: review-submitted or assessment-unchanged.' + required: true + type: string + +permissions: {} + +jobs: + get-state-issue: + if: ${{ !github.event.repository.fork }} + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + state_issue_number: ${{ steps.get.outputs.state_issue_number }} + permissions: + contents: read + issues: read + pull-requests: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Get pull request state issue + id: get + env: + CREATE_STATE_ISSUE: 'false' + GH_TOKEN: ${{ github.token }} + shell: bash + run: bash .github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh + reconcile-state: + needs: get-state-issue + if: ${{ needs.get-state-issue.outputs.state_issue_number != '' }} + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + state_updates: ${{ steps.reconcile.outputs.state_updates }} + permissions: + contents: read + issues: read + pull-requests: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Reconcile completed review + id: reconcile + env: + GH_TOKEN: ${{ github.token }} + # Reusable workflows retain the worker's event context. Bind state to that + # context rather than trusting the agent-controlled callback payload. + PR_NUMBER: ${{ github.event.inputs.pr_number }} + HEAD_SHA: ${{ github.event.inputs.pr_head_sha }} + OUTCOME: ${{ inputs.outcome }} + STATE_ISSUE_NUMBER: ${{ needs.get-state-issue.outputs.state_issue_number }} + shell: bash + run: bash .github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh + upsert-state: + needs: [get-state-issue, reconcile-state] + if: ${{ always() && needs.get-state-issue.result == 'success' && needs.get-state-issue.outputs.state_issue_number != '' && needs.reconcile-state.result == 'success' }} + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + contents: read + issues: write + pull-requests: read + strategy: + fail-fast: false + max-parallel: 1 + matrix: ${{ fromJSON(needs.reconcile-state.outputs.state_updates || '{"include":[]}') }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Upsert pull request state comment + env: + GH_TOKEN: ${{ github.token }} + STATE_ISSUE_NUMBER: ${{ needs.get-state-issue.outputs.state_issue_number }} + PR_NUMBER: ${{ matrix.pr_number }} + PR_TITLE: ${{ matrix.title }} + STATE_JSON: ${{ toJSON(matrix.state) }} + shell: bash + run: bash .github/workflows/scripts/holistic-review/orchestrator-upsert-state.sh diff --git a/.github/workflows/holistic-review-orchestrator.yml b/.github/workflows/holistic-review-orchestrator.yml index 29cf4fbce77cd2..4ba8511326c8bf 100644 --- a/.github/workflows/holistic-review-orchestrator.yml +++ b/.github/workflows/holistic-review-orchestrator.yml @@ -9,7 +9,6 @@ on: description: 'Comma-separated open pull request numbers to consider, including drafts and retry-limited review targets; an unchanged head with a durable review is not reviewed again' required: false type: string - permissions: {} concurrency: @@ -17,720 +16,74 @@ concurrency: cancel-in-progress: false jobs: + get-state-issue: + if: ${{ !github.event.repository.fork }} + runs-on: ubuntu-latest + timeout-minutes: 10 + outputs: + state_issue_number: ${{ steps.get.outputs.state_issue_number }} + permissions: + contents: read + issues: write + pull-requests: read + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Get pull request state issue + id: get + env: + CREATE_STATE_ISSUE: 'true' + GH_TOKEN: ${{ github.token }} + shell: bash + run: bash .github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh dispatch: - if: ${{ github.event_name == 'workflow_dispatch' || !github.event.repository.fork }} + needs: get-state-issue + if: ${{ !github.event.repository.fork }} runs-on: ubuntu-latest timeout-minutes: 15 + outputs: + state_updates: ${{ steps.dispatch.outputs.state_updates }} permissions: - actions: write + actions: read + contents: read + issues: write pull-requests: write steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - name: Dispatch reviews for new pull request heads + id: dispatch env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_TOKEN: ${{ github.token }} MAX_DISPATCH: '20' MAX_REVIEW_ATTEMPTS: '5' - PR_NUMBERS: ${{ inputs.pr_numbers }} + PR_NUMBERS: ${{ github.event.inputs.pr_numbers }} + STATE_ISSUE_NUMBER: ${{ needs.get-state-issue.outputs.state_issue_number }} shell: bash - run: | - set -euo pipefail - - open_prs_file="$(mktemp)" - dispatched_prs_file="$(mktemp)" - retry_limited_prs_file="$(mktemp)" - already_reviewed_prs_file="$(mktemp)" - trap 'rm -f "$open_prs_file" "$dispatched_prs_file" "$retry_limited_prs_file" "$already_reviewed_prs_file"' EXIT - state_comment_intro="Workflow state for the [Holistic Review Orchestrator](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID})." - state_comment_pattern='^Workflow state for the \[(Holistic Review Orchestrator|Code Review Orchestrator)\]\([^)]*/actions/runs/[0-9]+\)\.( Please ignore and do not edit\.)?\n\n```json\n' - previous_state_comment_prefix='Code review workflow state (managed automatically; do not edit).' - - requested_pr_numbers='[]' - if [ -n "$PR_NUMBERS" ]; then - requested_pr_numbers="$(jq -Rn --arg pr_numbers "$PR_NUMBERS" ' - $pr_numbers - | split(",") - | map(gsub("^\\s+|\\s+$"; "")) - | if any(.[]; test("^[1-9][0-9]*$") | not) then - error("pr_numbers must be a comma-separated list of positive pull request numbers") - else - map(tonumber) | unique - end - ')" - fi - - gh pr list --repo "$GITHUB_REPOSITORY" --state open --limit 1000 \ - --json number,baseRefName,baseRefOid,headRefOid,isDraft,updatedAt \ - --jq '[.[]]' > "$open_prs_file" - if ! jq -e --argjson requested_pr_numbers "$requested_pr_numbers" ' - if ($requested_pr_numbers | length) == 0 then - true - else - ([.[] | .number] as $open_pr_numbers - | all($requested_pr_numbers[]; . as $requested | any($open_pr_numbers[]; . == $requested))) - end - ' "$open_prs_file" > /dev/null; then - echo "One or more requested pull requests are not open." >&2 - exit 1 - fi - jq --argjson requested_pr_numbers "$requested_pr_numbers" ' - if ($requested_pr_numbers | length) == 0 then - . - else - [.[] | select(.number as $number | any($requested_pr_numbers[]; . == $number))] - end - ' "$open_prs_file" > "${open_prs_file}.filtered" - if [ "$(jq 'length' <<< "$requested_pr_numbers")" -eq 0 ]; then - jq '[.[] | select(.isDraft == false)]' "${open_prs_file}.filtered" > "$open_prs_file" - else - mv "${open_prs_file}.filtered" "$open_prs_file" - fi - echo "Eligible pull requests: $(jq 'length' "$open_prs_file")" - - worker_runs_since="$(date -u -d '7 days ago' '+%Y-%m-%dT%H:%M:%SZ')" - worker_runs="$( - gh api --method GET --paginate --slurp \ - "repos/${GITHUB_REPOSITORY}/actions/workflows/holistic-review.lock.yml/runs" \ - -f per_page=100 \ - -f "created=>=${worker_runs_since}" | - jq -c '{ workflow_runs: [ .[] | .workflow_runs[] ] }' - )" - - get_review_history() { - local pr_number="$1" - local include_legacy_reviews="$2" - local expected_run_id="${3:-}" - local submitted_after="${4:-}" - gh api --paginate --slurp \ - "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}/reviews?per_page=100" | - jq -c \ - --argjson include_legacy_reviews "$include_legacy_reviews" \ - --arg expected_run_id "$expected_run_id" \ - --arg submitted_after "$submitted_after" ' - [ - .[][] - | select( - .user.login == "github-actions[bot]" - and .state == "COMMENTED" - and ((.body // "") | contains("") | not) - and ( - $submitted_after == "" - or (.submitted_at // "") >= $submitted_after - ) - and ( - $expected_run_id == "" - or ( - (.body // "") - | contains( - ", id: " - + $expected_run_id - + ", workflow_id: holistic-review," - ) - ) - ) - and ( - ( - (.body // "") as $body - | ($body | startswith("## Holistic Review\n\n**Motivation**:")) - and ($body | contains("\n\n**Approach**:")) - and ($body | contains("\n\n**Summary**:")) - and ( - ( - ($body | contains("")) - ) - ) - ] - | last // empty - ' <<< "$comments")" - if [ -n "$state_comment" ]; then - state_comment_is_legacy=true - fi - elif jq -e '(.body // "") | startswith("Workflow state for the [Code Review Orchestrator]")' \ - <<< "$state_comment" > /dev/null; then - state_comment_is_legacy=true - fi - - last_dispatched_commit='' - last_dispatched_base_ref='' - last_dispatched_base_sha='' - last_reviewed_commit='' - last_reviewed_base_ref='' - last_reviewed_base_sha='' - last_recorded_worker_run_id='' - review_history='[]' - review_history_requires_migration=true - review_attempt_commit='' - review_attempt_base_ref='' - review_attempt_count=0 - manual_retry_reset=false - retry_state_requires_migration=false - state_comment_id='' - if [ -n "$state_comment" ]; then - state_comment_id="$(jq -er '.id' <<< "$state_comment")" - last_dispatched_commit="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_dispatched_commit // .last_dispatched_head // "" - ) catch "" - ' <<< "$state_comment")" - last_dispatched_base_sha="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_dispatched_base_sha // "" - ) catch "" - ' <<< "$state_comment")" - last_dispatched_base_ref="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_dispatched_base_ref // "" - ) catch "" - ' <<< "$state_comment")" - last_reviewed_commit="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_reviewed_commit // .last_reviewed_head // "" - ) catch "" - ' <<< "$state_comment")" - last_reviewed_base_sha="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_reviewed_base_sha // "" - ) catch "" - ' <<< "$state_comment")" - last_reviewed_base_ref="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_reviewed_base_ref // "" - ) catch "" - ' <<< "$state_comment")" - last_recorded_worker_run_id="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .last_recorded_worker_run_id // "" - ) catch "" - ' <<< "$state_comment")" - review_history="$(jq -c --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .review_history // [] - ) catch [] - ' <<< "$state_comment")" - review_history_requires_migration="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | ((.review_history | type) != "array" or .review_history_format != "holistic-review-disclosure-v1") - ) catch true - ' <<< "$state_comment")" - review_attempt_commit="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .review_attempt_commit // "" - ) catch "" - ' <<< "$state_comment")" - review_attempt_base_ref="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .review_attempt_base_ref // "" - ) catch "" - ' <<< "$state_comment")" - review_attempt_count="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | .review_attempt_count // 0 - ) catch 0 - ' <<< "$state_comment")" - retry_state_requires_migration="$(jq -r --arg state_comment_pattern "$state_comment_pattern" --arg previous_state_comment_prefix "$previous_state_comment_prefix" --argjson max_review_attempts "$MAX_REVIEW_ATTEMPTS" ' - try ( - .body - | if test($state_comment_pattern) - or startswith($previous_state_comment_prefix + "\n\n```json\n") - then split("```json\n")[1] | split("\n```")[0] - else sub("^\\n"; "") - end - | fromjson - | ( - .version != 5 - or has("last_dispatched_base_ref") == false - or has("last_dispatched_base_sha") == false - or has("last_reviewed_base_ref") == false - or has("last_reviewed_base_sha") == false - or has("review_attempt_commit") == false - or has("review_attempt_base_ref") == false - or has("review_attempt_count") == false - or .max_review_attempts != $max_review_attempts - ) - ) catch true - ' <<< "$state_comment")" - if ! [[ "$review_attempt_count" =~ ^[0-9]+$ ]]; then - review_attempt_count=0 - retry_state_requires_migration=true - fi - if [ -z "$review_attempt_commit" ] && [ -n "$last_dispatched_commit" ]; then - review_attempt_commit="$last_dispatched_commit" - review_attempt_base_ref="$last_dispatched_base_ref" - review_attempt_count=1 - retry_state_requires_migration=true - fi - fi - - # Legacy state does not identify its base branch, so it must not suppress one conservative full review. - - write_state_comment() { - state_json="$( - jq -n \ - --arg last_dispatched_commit "$last_dispatched_commit" \ - --arg last_dispatched_base_ref "$last_dispatched_base_ref" \ - --arg last_dispatched_base_sha "$last_dispatched_base_sha" \ - --arg last_reviewed_commit "$last_reviewed_commit" \ - --arg last_reviewed_base_ref "$last_reviewed_base_ref" \ - --arg last_reviewed_base_sha "$last_reviewed_base_sha" \ - --arg last_recorded_worker_run_id "$last_recorded_worker_run_id" \ - --arg review_attempt_commit "$review_attempt_commit" \ - --arg review_attempt_base_ref "$review_attempt_base_ref" \ - --argjson review_attempt_count "$review_attempt_count" \ - --argjson max_review_attempts "$MAX_REVIEW_ATTEMPTS" \ - --argjson review_history "$review_history" ' - { - version: 5, - last_dispatched_commit: $last_dispatched_commit, - last_dispatched_base_ref: $last_dispatched_base_ref, - last_dispatched_base_sha: $last_dispatched_base_sha, - last_reviewed_commit: $last_reviewed_commit, - last_reviewed_base_ref: $last_reviewed_base_ref, - last_reviewed_base_sha: $last_reviewed_base_sha, - last_recorded_worker_run_id: $last_recorded_worker_run_id, - review_attempt_commit: $review_attempt_commit, - review_attempt_base_ref: $review_attempt_base_ref, - review_attempt_count: $review_attempt_count, - max_review_attempts: $max_review_attempts, - review_history_format: "holistic-review-disclosure-v1", - review_history: $review_history - } - ' - )" - state_body="$( - printf '%s\n\n```json\n%s\n```' "$state_comment_intro" "$state_json" - )" - if [ -n "$state_comment_id" ]; then - gh api --method PATCH \ - "repos/${GITHUB_REPOSITORY}/issues/comments/${state_comment_id}" \ - -f "body=${state_body}" > /dev/null - else - gh api --method POST \ - "repos/${GITHUB_REPOSITORY}/issues/${pr_number}/comments" \ - -f "body=${state_body}" > /dev/null - fi - } - - # A submitted workflow review is authoritative even if the worker later fails. The - # state comment records that commit separately from the most recently dispatched - # commit so a later worker reviews only the commits since this durable review. - state_needs_update="$state_comment_is_legacy" - if [ "$retry_state_requires_migration" = true ]; then - state_needs_update=true - fi - include_legacy_reviews=false - if [ "$state_comment_is_legacy" = true ] || [ "$review_history_requires_migration" = true ]; then - include_legacy_reviews=true - review_history="$(get_review_history "$pr_number" "$include_legacy_reviews")" - state_needs_update=true - fi - if [ -n "$last_dispatched_commit" ]; then - completed_review_run_name="Holistic Review #${pr_number} (${last_dispatched_commit})" - legacy_completed_review_run_name="Code Review Worker #${pr_number} (${last_dispatched_commit})" - completed_review_run="$(jq -c --arg review_run_name "$completed_review_run_name" --arg legacy_review_run_name "$legacy_completed_review_run_name" ' - [ - .workflow_runs[] - | select(.display_title == $review_run_name or .display_title == $legacy_review_run_name) - ] - | sort_by(.created_at) - | last // empty - ' <<< "$worker_runs")" - if [ -n "$completed_review_run" ] && - [ "$(jq -r '.status' <<< "$completed_review_run")" = "completed" ] && - [ "$last_recorded_worker_run_id" != "$(jq -r '.id' <<< "$completed_review_run")" ]; then - completed_review_created_at="$(jq -r '.created_at' <<< "$completed_review_run")" - completed_review_run_id="$(jq -r '.id' <<< "$completed_review_run")" - discovered_review_history="$( - get_review_history \ - "$pr_number" \ - "$include_legacy_reviews" \ - "$completed_review_run_id" \ - "$completed_review_created_at" - )" - if jq -e --arg commit "$last_dispatched_commit" \ - 'any(.[]; .commit == $commit)' <<< "$discovered_review_history" > /dev/null; then - current_review="$(jq -c --arg commit "$last_dispatched_commit" ' - [ .[] | select(.commit == $commit) ] | last - ' <<< "$discovered_review_history")" - review_history="$(jq -cn \ - --argjson review_history "$review_history" \ - --argjson current_review "$current_review" ' - if ($review_history | length) == 0 then - [$current_review] - elif $review_history[0].review_id == $current_review.review_id then - $review_history - else - [$review_history[0], $current_review] - end - ' - )" - last_recorded_worker_run_id="$(jq -r '.id' <<< "$completed_review_run")" - if [ "$last_reviewed_commit" != "$last_dispatched_commit" ]; then - last_reviewed_commit="$last_dispatched_commit" - fi - last_reviewed_base_ref="$last_dispatched_base_ref" - last_reviewed_base_sha="$last_dispatched_base_sha" - review_attempt_commit='' - review_attempt_base_ref='' - review_attempt_count=0 - state_needs_update=true - elif [ "$(jq -r '.conclusion // ""' <<< "$completed_review_run")" = "success" ]; then - last_recorded_worker_run_id="$(jq -r '.id' <<< "$completed_review_run")" - if [ "$review_attempt_commit" = "$last_dispatched_commit" ] && - [ "$review_attempt_base_ref" = "$last_dispatched_base_ref" ] && - [ "$review_attempt_count" -ge "$MAX_REVIEW_ATTEMPTS" ]; then - echo "Completed review run for commit ${last_dispatched_commit} did not submit a review; retry limit reached." - else - echo "Completed review run for commit ${last_dispatched_commit} did not submit a review; retrying." - last_dispatched_commit='' - last_dispatched_base_ref='' - last_dispatched_base_sha='' - fi - state_needs_update=true - fi - fi - fi - - if [ -n "$PR_NUMBERS" ] && - { [ "$last_reviewed_commit" != "$head_sha" ] || - [ "$last_reviewed_base_ref" != "$base_ref" ]; } && - [ "$review_attempt_commit" = "$head_sha" ] && - [ "$review_attempt_base_ref" = "$base_ref" ] && - [ "$review_attempt_count" -ge "$MAX_REVIEW_ATTEMPTS" ]; then - review_attempt_commit='' - review_attempt_base_ref='' - review_attempt_count=0 - manual_retry_reset=true - state_needs_update=true - fi - - if [ "$last_reviewed_commit" = "$head_sha" ] && - [ "$last_reviewed_base_ref" = "$base_ref" ]; then - if [ -n "$review_attempt_commit" ] || - [ -n "$review_attempt_base_ref" ] || - [ "$review_attempt_count" -ne 0 ]; then - review_attempt_commit='' - review_attempt_base_ref='' - review_attempt_count=0 - state_needs_update=true - fi - if [ -n "$PR_NUMBERS" ]; then - printf '| [#%s](%s/%s/pull/%s) | `%s` |\n' \ - "$pr_number" \ - "$GITHUB_SERVER_URL" \ - "$GITHUB_REPOSITORY" \ - "$pr_number" \ - "$head_sha" >> "$already_reviewed_prs_file" - fi - if [ "$state_needs_update" = true ]; then - write_state_comment - fi - continue - fi - - if [ "$review_attempt_commit" = "$head_sha" ] && - [ "$review_attempt_base_ref" = "$base_ref" ] && - [ "$review_attempt_count" -ge "$MAX_REVIEW_ATTEMPTS" ]; then - printf '| [#%s](%s/%s/pull/%s) | `%s` | %s |\n' \ - "$pr_number" \ - "$GITHUB_SERVER_URL" \ - "$GITHUB_REPOSITORY" \ - "$pr_number" \ - "$head_sha" \ - "$review_attempt_count" >> "$retry_limited_prs_file" - if [ "$state_needs_update" = true ]; then - write_state_comment - fi - continue - fi - - review_run_name="Holistic Review #${pr_number} (${head_sha})" - legacy_review_run_name="Code Review Worker #${pr_number} (${head_sha})" - review_run="$(jq -c --arg review_run_name "$review_run_name" --arg legacy_review_run_name "$legacy_review_run_name" ' - [ - .workflow_runs[] - | select(.display_title == $review_run_name or .display_title == $legacy_review_run_name) - ] - | sort_by(.created_at) - | last // empty - ' <<< "$worker_runs")" - if [ "$last_dispatched_commit" = "$head_sha" ] && - [ "$last_dispatched_base_ref" = "$base_ref" ] && - [ -n "$review_run" ]; then - review_status="$(jq -r '.status' <<< "$review_run")" - review_conclusion="$(jq -r '.conclusion // ""' <<< "$review_run")" - if [ "$review_status" != "completed" ] || - { [ "$review_conclusion" = "success" ] && - [ "$manual_retry_reset" != true ]; }; then - if [ "$state_needs_update" = true ]; then - write_state_comment - fi - continue - fi - fi - - if [ "$dispatched" -ge "$MAX_DISPATCH" ]; then - if [ "$manual_retry_reset" = true ]; then - last_dispatched_commit='' - last_dispatched_base_ref='' - last_dispatched_base_sha='' - fi - if [ "$state_needs_update" = true ]; then - write_state_comment - fi - if [ -n "$PR_NUMBERS" ]; then - continue - fi - break - fi - - previous_head_sha="$last_reviewed_commit" - previous_base_sha="$last_reviewed_base_sha" - if [ -z "$last_reviewed_base_ref" ]; then - previous_head_sha='' - previous_base_sha='' - fi - - fetch_sha="$previous_head_sha" - if [ -z "$fetch_sha" ]; then - fetch_sha="$head_sha" - fi - - aw_context="$(jq -cn \ - --arg run_id "$GITHUB_RUN_ID" \ - --arg repo "$GITHUB_REPOSITORY" \ - --arg workflow_id "$GITHUB_WORKFLOW_REF" \ - --argjson item_number "$pr_number" \ - '{ - run_id: $run_id, - repo: $repo, - workflow_id: $workflow_id, - item_type: "pull_request", - item_number: $item_number - }')" - - gh api --method POST \ - "repos/${GITHUB_REPOSITORY}/actions/workflows/holistic-review.lock.yml/dispatches" \ - -f "ref=${DEFAULT_BRANCH}" \ - -f "inputs[pr_number]=${pr_number}" \ - -f "inputs[pr_base_ref]=${base_ref}" \ - -f "inputs[pr_head_sha]=${head_sha}" \ - -f "inputs[previous_head_sha]=${previous_head_sha}" \ - -f "inputs[previous_base_sha]=${previous_base_sha}" \ - -f "inputs[previous_review_history]=${review_history}" \ - -f "inputs[fetch_sha]=${fetch_sha}" \ - -f "inputs[aw_context]=${aw_context}" > /dev/null - - if [ "$review_attempt_commit" != "$head_sha" ] || - [ "$review_attempt_base_ref" != "$base_ref" ]; then - review_attempt_count=0 - fi - review_attempt_commit="$head_sha" - review_attempt_base_ref="$base_ref" - review_attempt_count=$((review_attempt_count + 1)) - last_dispatched_commit="$head_sha" - last_dispatched_base_ref="$base_ref" - last_dispatched_base_sha="$base_sha" - write_state_comment - - previous_commit_display="$previous_head_sha" - if [ -z "$previous_commit_display" ]; then - previous_commit_display='_Initial review_' - else - previous_commit_display="\`$previous_commit_display\`" - fi - printf '| [#%s](%s/%s/pull/%s) | `%s` | %s |\n' \ - "$pr_number" \ - "$GITHUB_SERVER_URL" \ - "$GITHUB_REPOSITORY" \ - "$pr_number" \ - "$head_sha" \ - "$previous_commit_display" >> "$dispatched_prs_file" - - dispatched=$((dispatched + 1)) - done < <(jq -c 'sort_by(.updatedAt)[] | { - pr_number: .number, - base_ref: .baseRefName, - base_sha: .baseRefOid, - head_sha: .headRefOid - }' "$open_prs_file") - - echo "Dispatched ${dispatched} holistic review workflow(s)." - { - echo '## Holistic Review Orchestrator' - echo - if [ -s "$dispatched_prs_file" ]; then - echo "Dispatched ${dispatched} holistic review workflow(s):" - echo - echo '| Pull request | Dispatched commit | Previously reviewed commit |' - echo '| --- | --- | --- |' - cat "$dispatched_prs_file" - else - echo 'No holistic review workflows were dispatched.' - fi - if [ -s "$retry_limited_prs_file" ]; then - echo - echo '### Retry limit reached' - echo - echo '| Pull request | Commit | Attempts |' - echo '| --- | --- | ---: |' - cat "$retry_limited_prs_file" - echo - echo "Scheduled retries stop after ${MAX_REVIEW_ATTEMPTS} attempts for one commit and target branch. A targeted manual dispatch resets that review target's retry budget." - fi - if [ -s "$already_reviewed_prs_file" ]; then - echo - echo '### Already reviewed' - echo - echo 'These targeted pull requests already have a durable review for their current commit and target branch, so no duplicate review was dispatched.' - echo - echo '| Pull request | Commit |' - echo '| --- | --- |' - cat "$already_reviewed_prs_file" - fi - } >> "$GITHUB_STEP_SUMMARY" + run: bash .github/workflows/scripts/holistic-review/orchestrator-dispatch.sh + upsert-state: + needs: [get-state-issue, dispatch] + if: ${{ always() && needs.get-state-issue.result == 'success' && needs.dispatch.result == 'success' && needs.dispatch.outputs.state_updates != '{"include":[]}' }} + runs-on: ubuntu-latest + timeout-minutes: 10 + permissions: + actions: write + contents: read + issues: write + pull-requests: read + strategy: + fail-fast: false + max-parallel: 1 + matrix: ${{ fromJSON(needs.dispatch.outputs.state_updates || '{"include":[]}') }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + - name: Upsert pull request state comment + env: + DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} + GH_TOKEN: ${{ github.token }} + STATE_ISSUE_NUMBER: ${{ needs.get-state-issue.outputs.state_issue_number }} + PR_NUMBER: ${{ matrix.pr_number }} + PR_TITLE: ${{ matrix.title }} + STATE_JSON: ${{ toJSON(matrix.state) }} + WORKER_DISPATCH: ${{ toJSON(matrix.worker_dispatch) }} + shell: bash + run: bash .github/workflows/scripts/holistic-review/orchestrator-upsert-state.sh diff --git a/.github/workflows/holistic-review.lock.yml b/.github/workflows/holistic-review.lock.yml index da272cf4008f13..c212e724afc2a7 100644 --- a/.github/workflows/holistic-review.lock.yml +++ b/.github/workflows/holistic-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7be581e4246d39b0d5fc4bfbbcb93665ed9ab8e27bcdc93d16e05d7003e9a9a6","body_hash":"8f6493cd30dba38a165e7991ee9b6733a55e55a2d7e120637edc702d3cc7c620","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b686905ca0d3a7422d66e496cb390941ca2dd33088435db8af5f7ae6ba67ce88","body_hash":"e76767cbb0ee3f0eaf898c176e68768e17a06364291f73d930d23b4b3457b622","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"cec6394202d7db187b02310d928812194988eb20","version":"v0.82.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27","digest":"sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27@sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27","digest":"sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27@sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27","digest":"sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27@sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27","digest":"sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27@sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.0","digest":"sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.0@sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw (v0.82.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -288,6 +288,7 @@ jobs: GH_AW_EXPR_802A9F6A: ${{ github.event.issue.number || (fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_type == 'issue' && fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number) }} GH_AW_EXPR_FF1D34CE: ${{ github.event.comment.id || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').comment_id }} GH_AW_GITHUB_ACTOR: ${{ github.actor }} + GH_AW_GITHUB_EVENT_INPUTS_PR_HEAD_SHA: ${{ github.event.inputs.pr_head_sha }} GH_AW_GITHUB_EVENT_INPUTS_PR_NUMBER: ${{ github.event.inputs.pr_number }} GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} GH_AW_GITHUB_RUN_ID: ${{ github.run_id }} @@ -297,20 +298,20 @@ jobs: run: | bash "${RUNNER_TEMP}/gh-aw/actions/create_prompt_first.sh" { - cat << 'GH_AW_PROMPT_a6cd7642a51ba102_EOF' + cat << 'GH_AW_PROMPT_696bc85ca0da3ae4_EOF' - GH_AW_PROMPT_a6cd7642a51ba102_EOF + GH_AW_PROMPT_696bc85ca0da3ae4_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/xpia.md" cat "${RUNNER_TEMP}/gh-aw/prompts/temp_folder_prompt.md" cat "${RUNNER_TEMP}/gh-aw/prompts/markdown.md" cat "${RUNNER_TEMP}/gh-aw/prompts/safe_outputs_prompt.md" - cat << 'GH_AW_PROMPT_a6cd7642a51ba102_EOF' + cat << 'GH_AW_PROMPT_696bc85ca0da3ae4_EOF' - Tools: create_pull_request_review_comment(max:10), submit_pull_request_review, missing_tool, missing_data, noop + Tools: create_pull_request_review_comment(max:10), submit_pull_request_review, call_workflow, missing_tool, missing_data, noop - GH_AW_PROMPT_a6cd7642a51ba102_EOF + GH_AW_PROMPT_696bc85ca0da3ae4_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/mcp_cli_tools_prompt.md" - cat << 'GH_AW_PROMPT_a6cd7642a51ba102_EOF' + cat << 'GH_AW_PROMPT_696bc85ca0da3ae4_EOF' The following GitHub context information is available for this workflow: {{#if github.actor}} @@ -352,18 +353,19 @@ jobs: stop immediately and report the limitation rather than spending turns trying to work around it. - GH_AW_PROMPT_a6cd7642a51ba102_EOF + GH_AW_PROMPT_696bc85ca0da3ae4_EOF cat "${RUNNER_TEMP}/gh-aw/prompts/cli_proxy_with_safeoutputs_prompt.md" - cat << 'GH_AW_PROMPT_a6cd7642a51ba102_EOF' + cat << 'GH_AW_PROMPT_696bc85ca0da3ae4_EOF' {{#runtime-import .github/workflows/holistic-review.md}} - GH_AW_PROMPT_a6cd7642a51ba102_EOF + GH_AW_PROMPT_696bc85ca0da3ae4_EOF } > "$GH_AW_PROMPT" - name: Interpolate variables and render templates uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt GH_AW_ENGINE_ID: "copilot" + GH_AW_GITHUB_EVENT_INPUTS_PR_HEAD_SHA: ${{ github.event.inputs.pr_head_sha }} GH_AW_GITHUB_EVENT_INPUTS_PR_NUMBER: ${{ github.event.inputs.pr_number }} GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} GH_AW_GITHUB_SERVER_URL: ${{ github.server_url }} @@ -382,6 +384,7 @@ jobs: GH_AW_EXPR_802A9F6A: ${{ github.event.issue.number || (fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_type == 'issue' && fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').item_number) }} GH_AW_EXPR_FF1D34CE: ${{ github.event.comment.id || fromJSON(github.event.inputs.aw_context || github.event.client_payload.aw_context || '{}').comment_id }} GH_AW_GITHUB_ACTOR: ${{ github.actor }} + GH_AW_GITHUB_EVENT_INPUTS_PR_HEAD_SHA: ${{ github.event.inputs.pr_head_sha }} GH_AW_GITHUB_EVENT_INPUTS_PR_NUMBER: ${{ github.event.inputs.pr_number }} GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} GH_AW_GITHUB_RUN_ID: ${{ github.run_id }} @@ -405,6 +408,7 @@ jobs: GH_AW_EXPR_802A9F6A: process.env.GH_AW_EXPR_802A9F6A, GH_AW_EXPR_FF1D34CE: process.env.GH_AW_EXPR_FF1D34CE, GH_AW_GITHUB_ACTOR: process.env.GH_AW_GITHUB_ACTOR, + GH_AW_GITHUB_EVENT_INPUTS_PR_HEAD_SHA: process.env.GH_AW_GITHUB_EVENT_INPUTS_PR_HEAD_SHA, GH_AW_GITHUB_EVENT_INPUTS_PR_NUMBER: process.env.GH_AW_GITHUB_EVENT_INPUTS_PR_NUMBER, GH_AW_GITHUB_REPOSITORY: process.env.GH_AW_GITHUB_REPOSITORY, GH_AW_GITHUB_RUN_ID: process.env.GH_AW_GITHUB_RUN_ID, @@ -606,9 +610,9 @@ jobs: mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs" mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs - cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_760d276743f4d3f1_EOF' - {"create_pull_request_review_comment":{"max":10,"side":"RIGHT","target":"${{ github.event.inputs.pr_number }}"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"report_incomplete":{},"submit_pull_request_review":{"allowed_events":["COMMENT"],"max":1,"target":"${{ github.event.inputs.pr_number }}"}} - GH_AW_SAFE_OUTPUTS_CONFIG_760d276743f4d3f1_EOF + cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_ed4dbecf896b597e_EOF' + {"call_workflow":{"max":1,"workflow_files":{"holistic-review-completed":"./.github/workflows/holistic-review-completed.yml"},"workflows":["holistic-review-completed"]},"create_pull_request_review_comment":{"max":10,"side":"RIGHT","target":"${{ github.event.inputs.pr_number }}"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"false"},"report_incomplete":{},"submit_pull_request_review":{"allowed_events":["COMMENT"],"max":1,"target":"${{ github.event.inputs.pr_number }}"}} + GH_AW_SAFE_OUTPUTS_CONFIG_ed4dbecf896b597e_EOF - name: Generate Safe Outputs Tools env: GH_AW_TOOLS_META_JSON: | @@ -618,7 +622,36 @@ jobs: "submit_pull_request_review": " CONSTRAINTS: Maximum 1 review(s) can be submitted. Target: ${{ github.event.inputs.pr_number }}." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [ + { + "_call_workflow_name": "holistic-review-completed", + "description": "Call the 'holistic-review-completed' reusable workflow via workflow_call. This workflow must support workflow_call and be in .github/workflows/ directory in the same repository.", + "inputSchema": { + "additionalProperties": false, + "properties": { + "head_sha": { + "description": "Pull request head commit assessed by the worker.", + "type": "string" + }, + "outcome": { + "description": "One of: review-submitted or assessment-unchanged.", + "type": "string" + }, + "pr_number": { + "description": "Pull request number reviewed by the worker.", + "type": "string" + } + }, + "required": [ + "head_sha", + "outcome", + "pr_number" + ], + "type": "object" + }, + "name": "holistic_review_completed" + } + ] } GH_AW_VALIDATION_JSON: | { @@ -1154,10 +1187,27 @@ jobs: /tmp/gh-aw/sandbox/firewall/awf-reflect.json if-no-files-found: ignore + call-holistic-review-completed: + needs: safe_outputs + if: needs.safe_outputs.outputs.call_workflow_name == 'holistic-review-completed' + # Imported from called workflow "holistic-review-completed" because GitHub requires the caller job to grant permissions requested by reusable workflow jobs. + # Review the called workflow's job-level permissions in ./.github/workflows/holistic-review-completed.yml. + permissions: + contents: read + issues: write + pull-requests: read + uses: ./.github/workflows/holistic-review-completed.yml + with: + head_sha: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).head_sha }} + outcome: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).outcome }} + pr_number: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).pr_number }} + secrets: inherit + conclusion: needs: - activation - agent + - call-holistic-review-completed - detection - pat_pool - safe_outputs @@ -1309,7 +1359,7 @@ jobs: GH_AW_WORKFLOW_SOURCE_URL: "${{ github.server_url }}/${{ github.repository }}/blob/${{ github.ref_name }}/.github/workflows/holistic-review.md" GH_AW_RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} GH_AW_AGENT_CONCLUSION: ${{ needs.agent.result }} - GH_AW_NOOP_REPORT_AS_ISSUE: "true" + GH_AW_NOOP_REPORT_AS_ISSUE: "false" GH_AW_AIC: ${{ needs.agent.outputs.aic }} GH_AW_THREAT_DETECTION_AIC: ${{ needs.detection.outputs.aic }} GH_AW_AMBIENT_CONTEXT: ${{ needs.agent.outputs.ambient_context }} @@ -1804,6 +1854,8 @@ jobs: GH_AW_WORKFLOW_NAME: "Holistic Review" GH_AW_WORKFLOW_SOURCE_URL: "${{ github.server_url }}/${{ github.repository }}/blob/${{ github.ref_name }}/.github/workflows/holistic-review.md" outputs: + call_workflow_name: ${{ steps.process_safe_outputs.outputs.call_workflow_name }} + call_workflow_payload: ${{ steps.process_safe_outputs.outputs.call_workflow_payload }} code_push_failure_count: ${{ steps.process_safe_outputs.outputs.code_push_failure_count }} code_push_failure_errors: ${{ steps.process_safe_outputs.outputs.code_push_failure_errors }} create_discussion_error_count: ${{ steps.process_safe_outputs.outputs.create_discussion_error_count }} @@ -1857,7 +1909,7 @@ jobs: GH_AW_ALLOWED_DOMAINS: "api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com,www.googleapis.com" GITHUB_SERVER_URL: ${{ github.server_url }} GITHUB_API_URL: ${{ github.api_url }} - GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"create_pull_request_review_comment\":{\"max\":10,\"side\":\"RIGHT\",\"target\":\"${{ github.event.inputs.pr_number }}\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"report_incomplete\":{},\"submit_pull_request_review\":{\"allowed_events\":[\"COMMENT\"],\"max\":1,\"target\":\"${{ github.event.inputs.pr_number }}\"}}" + GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"call_workflow\":{\"max\":1,\"workflow_files\":{\"holistic-review-completed\":\"./.github/workflows/holistic-review-completed.yml\"},\"workflows\":[\"holistic-review-completed\"]},\"create_pull_request_review_comment\":{\"max\":10,\"side\":\"RIGHT\",\"target\":\"${{ github.event.inputs.pr_number }}\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"false\"},\"report_incomplete\":{},\"submit_pull_request_review\":{\"allowed_events\":[\"COMMENT\"],\"max\":1,\"target\":\"${{ github.event.inputs.pr_number }}\"}}" with: github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} script: | diff --git a/.github/workflows/holistic-review.md b/.github/workflows/holistic-review.md index 62472eb09e5e00..1254d2a4124710 100644 --- a/.github/workflows/holistic-review.md +++ b/.github/workflows/holistic-review.md @@ -222,8 +222,9 @@ pre-agent-steps: echo "HOLISTIC_REVIEW_PREVIOUS_MERGE_BASE_SHA=$previous_base_sha" echo "HOLISTIC_REVIEW_SCOPE_DIR=$scope_dir" } >> "$GITHUB_ENV" - safe-outputs: + noop: + report-as-issue: false create-pull-request-review-comment: max: 10 side: RIGHT @@ -232,6 +233,9 @@ safe-outputs: max: 1 target: ${{ github.event.inputs.pr_number }} allowed-events: [COMMENT] + call-workflow: + workflows: [holistic-review-completed] + max: 1 timeout-minutes: 30 @@ -341,7 +345,6 @@ to launch another process or modify the workspace. Do not use write/edit tools o spawn child processes. In particular, do not configure or invoke Git aliases, hooks, pagers, external helpers, external diff or merge tools, credential helpers, or SSH commands; GitHub CLI extensions, aliases, configuration, or pagers; or an external compression program for `sort`. - ## Step 1: Determine the Review Scope Before the agent started, a trusted deterministic step computed the initial or incremental @@ -374,12 +377,11 @@ not rediscover findings in portions of the PR patch that remained unchanged. These re-review scope rules override any broader review-scope guidance in the review skill. -If `HOLISTIC_REVIEW_HAS_CHANGES` is `false`, do not inspect the source patch for new -findings and do not exit. Still submit a new `COMMENT` review. Its Holistic Review must state -that the PR patch has not changed since the prior review (or that an initial PR has no -base-to-head changes), include the required Assessment History for a re-review, and contain -no actionable findings. This ensures every successful worker review is recorded without -altering prior reviews. +If `HOLISTIC_REVIEW_HAS_CHANGES` is `false`, do not inspect the source patch for new findings. +For an initial review, submit a `COMMENT` review explaining that the PR has no base-to-head +changes. For a re-review, compare the resulting assessment with the most recent previous review +as described in Step 3; if it is unchanged, record the no-op result instead of posting a duplicate +review. ## Step 2: Load Review Guidelines @@ -391,10 +393,10 @@ This dispatched worker has no sub-agent or task tooling. Skip the skill's `Disco Follow the review skill for the range selected in Step 1. Consult existing PR comments and reviews as directed by the skill, but do not modify, hide, supersede, or otherwise remove prior comments or reviews. -Explicitly assess whether the PR's added complexity is necessary and proportionate to a validated -goal. A validated goal is supported by at least one concrete evidence source: an approved API, -specification, or accepted design requirement; a reproducible bug or regression; representative -benchmark or performance evidence; or customer, CI, production, or similarly concrete evidence. +Explicitly assess whether the PR's added complexity is necessary and proportionate to the stated +problem, supported by at least one concrete evidence source: an approved API, specification, or +accepted design requirement; a reproducible bug or regression; representative benchmark or +performance evidence; or customer, CI, production, or similarly concrete evidence. Do not treat size, low-level code, or specialized algorithms as concerns by themselves when the problem inherently requires them and the design is well-factored, tested, and consistent with established direction. Escalate only when a materially simpler approach meets the same requirements, @@ -402,12 +404,38 @@ the complexity is poorly encapsulated or duplicative, or the demonstrated benefi justify the maintenance burden. When the tradeoff remains unresolved, use `⚠️ Needs Human Review` and state the specific decision a maintainer should make. -Use the review skill's exact top-level body structure. After `## Holistic Review`, immediately emit -`**Motivation**:`, `**Approach**:`, and `**Summary**:` in that order. Then emit `### Detailed -Findings`. Do not restate the PR title, its description, or obvious code behavior unless the code -has a meaningful semantic difference from the stated intent. Do not duplicate findings or -assessments across the summary, detailed findings, or inline comments. Use only `✅`, `⚠️`, `💡`, -and `❌` as review-content status emojis. +Use the review skill's top-level body structure. After `## Holistic Review`, include +`**Motivation**:`, `**Approach**:`, and `**Summary**:` only when each adds non-obvious assessment +beyond the PR title, description, and code changes; omit Motivation and/or Summary when they would +merely restate the obvious. When present, keep those fields in that order. Then emit +`### Detailed Findings`. Do not duplicate findings or assessments across the optional top-level +fields, detailed findings, or inline comments. Use only `✅`, `⚠️`, `💡`, and `❌` as +review-content status emojis. + +For an incremental review, compare the complete current assessment with the most recent previous +review. The assessment is unchanged only when its emitted top-level fields, findings, and action +items are all unchanged; an updated commit or wording that restates the same assessment does not +make it different. When the assessment is unchanged: + +1. Do not create inline comments or submit a new pull request review. +2. Invoke `call_workflow` exactly once to record the completion: + + ```bash + safeoutputs call_workflow . <<'EOF' + {"workflow_name":"holistic-review-completed","pr_number":"${{ github.event.inputs.pr_number }}","head_sha":"${{ github.event.inputs.pr_head_sha }}","outcome":"assessment-unchanged"} + EOF + ``` + +For every review that you submit, invoke `call_workflow` exactly once after invoking +`submit_pull_request_review`, with `outcome` set to `review-submitted`: + +```bash +safeoutputs call_workflow . <<'EOF' +{"workflow_name":"holistic-review-completed","pr_number":"${{ github.event.inputs.pr_number }}","head_sha":"${{ github.event.inputs.pr_head_sha }}","outcome":"review-submitted"} +EOF +``` + +Do not invoke `call_workflow` for an incomplete review. For each actionable finding that is specific to one changed line or a contiguous changed range, invoke the `create_pull_request_review_comment` safe output before submitting the review. Use the dispatched `pull_request_number`, the changed file path, and the exact right-side line or range. Put the complete actionable explanation in that inline comment. Do not create inline comments for unchanged lines, broad/cross-cutting findings, non-actionable observations, or findings without a precise changed location; include those only in the visible `### Detailed Findings` section of the review body. Do not duplicate a finding's full explanation in both places: identify inline findings briefly in the body and link to the relevant file and line when useful. @@ -419,20 +447,19 @@ safeoutputs create_pull_request_review_comment . <<'EOF' EOF ``` -Replace the example values with the dispatched PR and finding. Do not pipe from `printf`, use flag-form arguments, chain another command, inspect CLI help, or use `report_incomplete`/`noop` as a substitute for the required review. Those forms can be rejected by the read-only shell policy even though the safe output itself is allowed. +Replace the example values with the dispatched PR and finding. Do not pipe from `printf`, use flag-form arguments, chain another command, or inspect CLI help. Use `report_incomplete` only when the review cannot be completed. When complete, submit the review with the same single-command JSON-input form: ```bash safeoutputs submit_pull_request_review . <<'EOF' -{"pull_request_number": 123, "event": "COMMENT", "body": "## Holistic Review\n\n**Motivation**: ...\n\n**Approach**: ...\n\n**Summary**: ..."} +{"pull_request_number": 123, "event": "COMMENT", "body": "## Holistic Review\n\n### Detailed Findings\n\n..."} EOF ``` -Set `pull_request_number` to `${{ github.event.inputs.pr_number }}` and include the complete review body as a valid JSON string. If the command is rejected, correct the JSON or invocation and retry this exact form once. Always submit a `COMMENT` event, including for an LGTM verdict. Never submit `REQUEST_CHANGES`. Inline comments created above are automatically included in this review. End every review with this disclosure, replacing the generic Copilot disclosure in the review skill: +Set `pull_request_number` to `${{ github.event.inputs.pr_number }}` and include the complete review body as a valid JSON string. If the command is rejected, correct the JSON or invocation and retry this exact form once. Except for the unchanged incremental-assessment case above, always submit a `COMMENT` event, including for an LGTM verdict. Never submit `REQUEST_CHANGES`. Inline comments created above are automatically included in this review. End every submitted review with this disclosure, replacing the generic Copilot disclosure in the review skill: > [!NOTE] > This review was generated by this repository's [Holistic Review](${{ github.server_url }}/${{ github.repository }}/blob/main/.github/workflows/holistic-review.md) agentic workflow to complement the built-in Copilot review. -The deterministic orchestrator separately records each completed worker's reviewed commit. -Do not add workflow provenance markers to the review body. +The orchestrator uses this callback to update the review state issue. diff --git a/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh b/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh new file mode 100644 index 00000000000000..ece39119370b4e --- /dev/null +++ b/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh @@ -0,0 +1,551 @@ +#!/usr/bin/env bash +set -euo pipefail + +open_prs_file="$(mktemp)" +dispatched_prs_file="$(mktemp)" +retry_limited_prs_file="$(mktemp)" +already_reviewed_prs_file="$(mktemp)" +closed_prs_file="$(mktemp)" +state_updates_file="$(mktemp)" +trap 'rm -f "$open_prs_file" "$dispatched_prs_file" "$retry_limited_prs_file" "$already_reviewed_prs_file" "$closed_prs_file" "$state_updates_file"' EXIT +state_issue_comment_prefix='\n' + +requested_pr_numbers='[]' +if [ -n "$PR_NUMBERS" ]; then + requested_pr_numbers="$(jq -Rn --arg pr_numbers "$PR_NUMBERS" ' + $pr_numbers + | split(",") + | map(gsub("^\\s+|\\s+$"; "")) + | if any(.[]; test("^[1-9][0-9]*$") | not) then + error("pr_numbers must be a comma-separated list of positive pull request numbers") + else + map(tonumber) | unique + end + ')" +fi + +gh pr list --repo "$GITHUB_REPOSITORY" --state open --limit 1000 \ + --json number,title,baseRefName,baseRefOid,headRefOid,isDraft,updatedAt \ + --jq '[.[]]' > "$open_prs_file" +# Capture open pull requests before filtering drafts so their state comments are retained. +active_pr_numbers="$(jq -c '[.[] | .number]' "$open_prs_file")" +if ! jq -e --argjson requested_pr_numbers "$requested_pr_numbers" ' + if ($requested_pr_numbers | length) == 0 then + true + else + ([.[] | .number] as $open_pr_numbers + | all($requested_pr_numbers[]; . as $requested | any($open_pr_numbers[]; . == $requested))) + end +' "$open_prs_file" > /dev/null; then + echo "One or more requested pull requests are not open." >&2 + exit 1 +fi +jq --argjson requested_pr_numbers "$requested_pr_numbers" ' + if ($requested_pr_numbers | length) == 0 then + . + else + [.[] | select(.number as $number | any($requested_pr_numbers[]; . == $number))] + end +' "$open_prs_file" > "${open_prs_file}.filtered" +if [ "$(jq 'length' <<< "$requested_pr_numbers")" -eq 0 ]; then + jq '[.[] | select(.isDraft == false)]' "${open_prs_file}.filtered" > "$open_prs_file" +else + mv "${open_prs_file}.filtered" "$open_prs_file" +fi +echo "Eligible pull requests: $(jq 'length' "$open_prs_file")" + +state_issue_number="$STATE_ISSUE_NUMBER" +state_comments="$( + gh api --method GET --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${state_issue_number}/comments?per_page=100" +)" + +html_escape() { + jq -rn --arg text "$1" ' + $text + | gsub("&"; "&") + | gsub("<"; "<") + | gsub(">"; ">") + | gsub("\""; """) + ' +} + +while IFS= read -r resolved_state_comment; do + resolved_state_comment_id="$(jq -er '.id' <<< "$resolved_state_comment")" + closed_pr_number="$(jq -er ' + .body + | capture("^") + | .number + ' <<< "$resolved_state_comment")" + gh api --method DELETE \ + "repos/${GITHUB_REPOSITORY}/issues/comments/${resolved_state_comment_id}" > /dev/null + printf '| [#%s](%s/%s/pull/%s) |\n' \ + "$closed_pr_number" \ + "$GITHUB_SERVER_URL" \ + "$GITHUB_REPOSITORY" \ + "$closed_pr_number" >> "$closed_prs_file" +done < <( + # Remove state for closed PRs so the central issue remains bounded over time. + jq -c \ + --arg state_issue_comment_pattern "$state_issue_comment_pattern" \ + --argjson active_pr_numbers "$active_pr_numbers" ' + .[][] + | select( + .user.login == "github-actions[bot]" + and ((.body // "") | test($state_issue_comment_pattern)) + and ( + .body + | capture("^") + | .number + | tonumber as $pr_number + | any($active_pr_numbers[]; . == $pr_number) + | not + ) + ) + ' <<< "$state_comments" +) + +worker_runs_since="$(date -u -d '7 days ago' '+%Y-%m-%dT%H:%M:%SZ')" +worker_runs="$( + gh api --method GET --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/actions/workflows/holistic-review.lock.yml/runs" \ + -f per_page=100 \ + -f "created=>=${worker_runs_since}" | + jq -c '{ workflow_runs: [ .[] | .workflow_runs[] ] }' +)" + +get_review_history() { + local pr_number="$1" + gh api --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/pulls/${pr_number}/reviews?per_page=100" | + jq -c ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and .state == "COMMENTED" + and ((.body // "") | contains("" + )? + | .sha + ) // $review.commit_id + ) as $reviewed_commit + | { + commit: $reviewed_commit, + review_id: $review.id, + submitted_at: $review.submitted_at + } + ] + | sort_by(.submitted_at) + | reduce .[] as $review ( + []; + if any(.[]; .review_id == $review.review_id) + then . + else . + [$review] + end + ) + # The first review anchors the initial assessment; the latest is the current assessment. + | if length > 1 then [.[0], .[-1]] else . end + | map(del(.submitted_at)) + ' +} + +dispatched=0 +while IFS= read -r entry; do + pr_number="$(jq -er '.pr_number' <<< "$entry")" + pr_title="$(jq -er '.title' <<< "$entry")" + pr_title_html="$(html_escape "$pr_title")" + base_ref="$(jq -er '.base_ref' <<< "$entry")" + base_sha="$(jq -er '.base_sha' <<< "$entry")" + head_sha="$(jq -er '.head_sha' <<< "$entry")" + + state_comment="$(jq -c \ + --arg state_issue_comment_prefix "$state_issue_comment_prefix" \ + --arg pr_number "$pr_number" ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and ( + (.body // "") + | startswith($state_issue_comment_prefix + $pr_number + " -->") + ) + ) + ] + | last // empty + ' <<< "$state_comments")" + last_dispatched_commit='' + last_dispatched_base_ref='' + last_dispatched_base_sha='' + last_reviewed_commit='' + last_reviewed_base_ref='' + last_reviewed_base_sha='' + review_history='[]' + review_attempt_commit='' + review_attempt_base_ref='' + review_attempt_count=0 + worker_dispatch='null' + manual_retry_reset=false + if [ -n "$state_comment" ]; then + state_json="$(jq -er ' + .body + | split("```json\n")[1] + | split("\n```")[0] + | fromjson + ' <<< "$state_comment")" + last_dispatched_commit="$(jq -r '.last_dispatched_commit // ""' <<< "$state_json")" + last_dispatched_base_sha="$(jq -r '.last_dispatched_base_sha // ""' <<< "$state_json")" + last_dispatched_base_ref="$(jq -r '.last_dispatched_base_ref // ""' <<< "$state_json")" + last_reviewed_commit="$(jq -r '.last_reviewed_commit // ""' <<< "$state_json")" + last_reviewed_base_sha="$(jq -r '.last_reviewed_base_sha // ""' <<< "$state_json")" + last_reviewed_base_ref="$(jq -r '.last_reviewed_base_ref // ""' <<< "$state_json")" + review_history="$(jq -c '.review_history // []' <<< "$state_json")" + review_attempt_commit="$(jq -r '.review_attempt_commit // ""' <<< "$state_json")" + review_attempt_base_ref="$(jq -r '.review_attempt_base_ref // ""' <<< "$state_json")" + review_attempt_count="$(jq -r '.review_attempt_count // 0' <<< "$state_json")" + fi + + write_state_comment() { + # Emit normalized state for the shared upsert job rather than writing concurrently here. + state_json="$( + jq -n \ + --arg last_dispatched_commit "$last_dispatched_commit" \ + --arg last_dispatched_base_ref "$last_dispatched_base_ref" \ + --arg last_dispatched_base_sha "$last_dispatched_base_sha" \ + --arg last_reviewed_commit "$last_reviewed_commit" \ + --arg last_reviewed_base_ref "$last_reviewed_base_ref" \ + --arg last_reviewed_base_sha "$last_reviewed_base_sha" \ + --arg pull_request_title "$pr_title" \ + --arg review_attempt_commit "$review_attempt_commit" \ + --arg review_attempt_base_ref "$review_attempt_base_ref" \ + --argjson review_attempt_count "$review_attempt_count" \ + --argjson max_review_attempts "$MAX_REVIEW_ATTEMPTS" \ + --argjson review_history "$review_history" ' + { + version: 8, + last_dispatched_commit: $last_dispatched_commit, + last_dispatched_base_ref: $last_dispatched_base_ref, + last_dispatched_base_sha: $last_dispatched_base_sha, + last_reviewed_commit: $last_reviewed_commit, + last_reviewed_base_ref: $last_reviewed_base_ref, + last_reviewed_base_sha: $last_reviewed_base_sha, + pull_request_title: $pull_request_title, + review_attempt_commit: $review_attempt_commit, + review_attempt_base_ref: $review_attempt_base_ref, + review_attempt_count: $review_attempt_count, + max_review_attempts: $max_review_attempts, + review_history_format: "holistic-review-disclosure-v1", + review_history: $review_history + } + ' + )" + jq -cn \ + --argjson pr_number "$pr_number" \ + --arg title "$pr_title" \ + --argjson state "$state_json" \ + --argjson worker_dispatch "$worker_dispatch" ' + { + pr_number: $pr_number, + title: $title, + state: $state + } + | if $worker_dispatch == null then . else .worker_dispatch = $worker_dispatch end + ' >> "$state_updates_file" + } + + # Pull request reviews are authoritative for review identity. An unchanged-assessment + # callback advances the latest authenticated review's commit in the state comment, so + # retain that commit while the physical review remains the same. + state_needs_update=false + expected_summary_emoji=':eyes:' + if [ "$last_dispatched_commit" = "$last_reviewed_commit" ] && + [ "$last_dispatched_base_ref" = "$last_reviewed_base_ref" ]; then + expected_summary_emoji=':heavy_check_mark:' + fi + if [ -n "$state_comment" ] && + ! jq -e --arg expected_summary_emoji "$expected_summary_emoji" \ + '(.body // "") | contains("" + $expected_summary_emoji + " ")' \ + <<< "$state_comment" > /dev/null; then + state_needs_update=true + fi + refreshed_review_history="$(get_review_history "$pr_number")" + recorded_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$review_history")" + refreshed_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$refreshed_review_history")" + if [ "$recorded_review_id" = "$refreshed_review_id" ] && [ -n "$recorded_review_id" ]; then + refreshed_review_history="$( + jq -cn \ + --argjson review_history "$review_history" \ + --argjson refreshed_review_history "$refreshed_review_history" ' + $refreshed_review_history + | .[-1].commit = $review_history[-1].commit + ' + )" + fi + if [ "$review_history" != "$refreshed_review_history" ]; then + review_history="$refreshed_review_history" + state_needs_update=true + fi + + refreshed_reviewed_commit="$(jq -r 'if length == 0 then "" else .[-1].commit end' <<< "$review_history")" + if [ "$last_reviewed_commit" != "$refreshed_reviewed_commit" ]; then + last_reviewed_commit="$refreshed_reviewed_commit" + state_needs_update=true + fi + if [ -n "$last_reviewed_commit" ]; then + if [ "$last_reviewed_base_ref" != "$base_ref" ] || + [ "$last_reviewed_base_sha" != "$base_sha" ]; then + last_reviewed_base_ref="$base_ref" + last_reviewed_base_sha="$base_sha" + state_needs_update=true + fi + if [ -n "$review_attempt_commit" ] || + [ -n "$review_attempt_base_ref" ] || + [ "$review_attempt_count" -ne 0 ]; then + review_attempt_commit='' + review_attempt_base_ref='' + review_attempt_count=0 + state_needs_update=true + fi + elif [ -n "$last_reviewed_base_ref" ] || [ -n "$last_reviewed_base_sha" ]; then + last_reviewed_base_ref='' + last_reviewed_base_sha='' + state_needs_update=true + fi + + if [ -n "$PR_NUMBERS" ] && + { [ "$last_reviewed_commit" != "$head_sha" ] || + [ "$last_reviewed_base_ref" != "$base_ref" ]; } && + [ "$review_attempt_commit" = "$head_sha" ] && + [ "$review_attempt_base_ref" = "$base_ref" ] && + [ "$review_attempt_count" -ge "$MAX_REVIEW_ATTEMPTS" ]; then + review_attempt_commit='' + review_attempt_base_ref='' + review_attempt_count=0 + manual_retry_reset=true + state_needs_update=true + fi + + if [ "$last_reviewed_commit" = "$head_sha" ] && + [ "$last_reviewed_base_ref" = "$base_ref" ]; then + if [ -n "$review_attempt_commit" ] || + [ -n "$review_attempt_base_ref" ] || + [ "$review_attempt_count" -ne 0 ]; then + review_attempt_commit='' + review_attempt_base_ref='' + review_attempt_count=0 + state_needs_update=true + fi + if [ -n "$PR_NUMBERS" ]; then + printf '| [#%s](%s/%s/pull/%s) | `%s` |\n' \ + "$pr_number" \ + "$GITHUB_SERVER_URL" \ + "$GITHUB_REPOSITORY" \ + "$pr_number" \ + "$head_sha" >> "$already_reviewed_prs_file" + fi + if [ "$state_needs_update" = true ]; then + write_state_comment + fi + continue + fi + + if [ "$review_attempt_commit" = "$head_sha" ] && + [ "$review_attempt_base_ref" = "$base_ref" ] && + [ "$review_attempt_count" -ge "$MAX_REVIEW_ATTEMPTS" ]; then + printf '| [#%s](%s/%s/pull/%s) | `%s` | %s |\n' \ + "$pr_number" \ + "$GITHUB_SERVER_URL" \ + "$GITHUB_REPOSITORY" \ + "$pr_number" \ + "$head_sha" \ + "$review_attempt_count" >> "$retry_limited_prs_file" + if [ "$state_needs_update" = true ]; then + write_state_comment + fi + continue + fi + + review_run_name="Holistic Review #${pr_number} (${head_sha})" + review_run="$(jq -c --arg review_run_name "$review_run_name" ' + [ + .workflow_runs[] + | select(.display_title == $review_run_name) + ] + | sort_by(.created_at) + | last // empty + ' <<< "$worker_runs")" + if [ "$last_dispatched_commit" = "$head_sha" ] && + [ "$last_dispatched_base_ref" = "$base_ref" ] && + [ -n "$review_run" ]; then + review_status="$(jq -r '.status' <<< "$review_run")" + review_conclusion="$(jq -r '.conclusion // ""' <<< "$review_run")" + if [ "$review_status" != "completed" ]; then + if [ "$state_needs_update" = true ]; then + write_state_comment + fi + continue + fi + if [ "$review_conclusion" = "success" ]; then + echo "Completed review run for commit ${head_sha} did not update a Holistic Review; retrying." + last_dispatched_commit='' + last_dispatched_base_ref='' + last_dispatched_base_sha='' + state_needs_update=true + fi + fi + + if [ "$dispatched" -ge "$MAX_DISPATCH" ]; then + if [ "$manual_retry_reset" = true ]; then + last_dispatched_commit='' + last_dispatched_base_ref='' + last_dispatched_base_sha='' + fi + if [ "$state_needs_update" = true ]; then + write_state_comment + fi + if [ -n "$PR_NUMBERS" ]; then + continue + fi + break + fi + + previous_head_sha="$last_reviewed_commit" + previous_base_sha="$last_reviewed_base_sha" + if [ -z "$last_reviewed_base_ref" ]; then + previous_head_sha='' + previous_base_sha='' + fi + + fetch_sha="$previous_head_sha" + if [ -z "$fetch_sha" ]; then + fetch_sha="$head_sha" + fi + + aw_context="$(jq -cn \ + --arg run_id "$GITHUB_RUN_ID" \ + --arg repo "$GITHUB_REPOSITORY" \ + --arg workflow_id "$GITHUB_WORKFLOW_REF" \ + --argjson item_number "$pr_number" \ + '{ + run_id: $run_id, + repo: $repo, + workflow_id: $workflow_id, + item_type: "pull_request", + item_number: $item_number + }')" + + if [ "$review_attempt_commit" != "$head_sha" ] || + [ "$review_attempt_base_ref" != "$base_ref" ]; then + review_attempt_count=0 + fi + review_attempt_commit="$head_sha" + review_attempt_base_ref="$base_ref" + review_attempt_count=$((review_attempt_count + 1)) + last_dispatched_commit="$head_sha" + last_dispatched_base_ref="$base_ref" + last_dispatched_base_sha="$base_sha" + worker_dispatch="$(jq -cn \ + --arg pr_number "$pr_number" \ + --arg base_ref "$base_ref" \ + --arg head_sha "$head_sha" \ + --arg previous_head_sha "$previous_head_sha" \ + --arg previous_base_sha "$previous_base_sha" \ + --arg review_history "$review_history" \ + --arg fetch_sha "$fetch_sha" \ + --arg aw_context "$aw_context" ' + { + pr_number: $pr_number, + base_ref: $base_ref, + head_sha: $head_sha, + previous_head_sha: $previous_head_sha, + previous_base_sha: $previous_base_sha, + review_history: $review_history, + fetch_sha: $fetch_sha, + aw_context: $aw_context + } + ')" + write_state_comment + + previous_commit_display="$previous_head_sha" + if [ -z "$previous_commit_display" ]; then + previous_commit_display='_Initial review_' + else + previous_commit_display="\`$previous_commit_display\`" + fi + printf '| [#%s](%s/%s/pull/%s) | `%s` | %s |\n' \ + "$pr_number" \ + "$GITHUB_SERVER_URL" \ + "$GITHUB_REPOSITORY" \ + "$pr_number" \ + "$head_sha" \ + "$previous_commit_display" >> "$dispatched_prs_file" + + dispatched=$((dispatched + 1)) +done < <(jq -c 'sort_by(.updatedAt)[] | { + pr_number: .number, + title: .title, + base_ref: .baseRefName, + base_sha: .baseRefOid, + head_sha: .headRefOid +}' "$open_prs_file") + +echo "Prepared ${dispatched} holistic review workflow dispatch(es)." +{ + echo '## Holistic Review Orchestrator' + echo + if [ -s "$dispatched_prs_file" ]; then + echo "Prepared ${dispatched} holistic review workflow dispatch(es):" + echo + echo '| Pull request | Dispatched commit | Previously reviewed commit |' + echo '| --- | --- | --- |' + cat "$dispatched_prs_file" + else + echo 'No holistic review workflows were dispatched.' + fi + if [ -s "$closed_prs_file" ]; then + echo + echo '### Closed pull requests' + echo + echo 'Removed centralized review state comments for these closed pull requests:' + echo + echo '| Pull request |' + echo '| --- |' + cat "$closed_prs_file" + fi + if [ -s "$retry_limited_prs_file" ]; then + echo + echo '### Retry limit reached' + echo + echo '| Pull request | Commit | Attempts |' + echo '| --- | --- | ---: |' + cat "$retry_limited_prs_file" + echo + echo "Scheduled retries stop after ${MAX_REVIEW_ATTEMPTS} attempts for one commit and target branch. A targeted manual dispatch resets that review target's retry budget." + fi + if [ -s "$already_reviewed_prs_file" ]; then + echo + echo '### Already reviewed' + echo + echo 'These targeted pull requests already have a durable review for their current commit and target branch, so no duplicate review was dispatched.' + echo + echo '| Pull request | Commit |' + echo '| --- | --- |' + cat "$already_reviewed_prs_file" + fi +} >> "$GITHUB_STEP_SUMMARY" + +{ + jq -sc ' + sort_by(.pr_number) + | group_by(.pr_number) + | map(last) + | { include: . } + ' "$state_updates_file" | tr -d '\n' | sed 's/^/state_updates=/' +} >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh b/.github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh new file mode 100644 index 00000000000000..eee463fd740c31 --- /dev/null +++ b/.github/workflows/scripts/holistic-review/orchestrator-get-state-issue.sh @@ -0,0 +1,98 @@ +#!/usr/bin/env bash +set -euo pipefail + +state_issue_title='[Holistic Review Orchestrator] PR Review State' +state_issue_body=' + +This issue stores state for pull request reviews conducted by the Holistic Review workflows.' +state_issue_comment_pattern='^\n' + +create_state_issue() { + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/issues" \ + -f "title=${state_issue_title}" \ + -f "body=${state_issue_body}" | + jq -er '.number' +} + +state_issue_search="$( + jq -rn \ + --arg repository "$GITHUB_REPOSITORY" \ + --arg state_issue_title "$state_issue_title" ' + "repo:\($repository) is:issue in:title \"\($state_issue_title)\"" + | @uri + ' +)" +state_issue="$( + gh api --method GET --paginate --slurp \ + "search/issues?q=${state_issue_search}&per_page=100" | + jq -c --arg state_issue_title "$state_issue_title" ' + [ + .[] | .items[] + | select(.user.login == "github-actions[bot]") + | select(.title == $state_issue_title) + ] + | sort_by(.number) + | last // empty + ' +)" + +if [ "${CREATE_STATE_ISSUE:-true}" != true ]; then + # Completion callbacks only consume an open store. The globally serialized dispatcher + # creates and rotates it, preventing concurrent callbacks from creating duplicates. + if [ -n "$state_issue" ] && [ "$(jq -r '.state' <<< "$state_issue")" = "open" ]; then + state_issue_number="$(jq -er '.number' <<< "$state_issue")" + else + state_issue_number='' + fi +elif [ -z "$state_issue" ]; then + state_issue_number="$(create_state_issue)" +elif [ "$(jq -r '.state' <<< "$state_issue")" = "open" ]; then + state_issue_number="$(jq -er '.number' <<< "$state_issue")" +else + # A closed store is immutable; seed its replacement with active PR state only. + closed_state_issue_number="$(jq -er '.number' <<< "$state_issue")" + state_issue_number="$(create_state_issue)" + active_pr_numbers="$( + gh pr list --repo "$GITHUB_REPOSITORY" --state open --limit 1000 --json number --jq '[.[].number]' + )" + closed_state_comments="$( + gh api --method GET --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${closed_state_issue_number}/comments?per_page=100" + )" + while IFS= read -r closed_state_comment; do + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/issues/${state_issue_number}/comments" \ + -f "body=$(jq -r '.body' <<< "$closed_state_comment")" > /dev/null + done < <( + jq -c \ + --arg state_issue_comment_pattern "$state_issue_comment_pattern" \ + --argjson active_pr_numbers "$active_pr_numbers" ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and ((.body // "") | test($state_issue_comment_pattern)) + ) + | . + { + pr_number: ( + .body + | capture("^") + | .number + | tonumber + ) + } + | select( + .pr_number as $pr_number + | any($active_pr_numbers[]; . == $pr_number) + ) + ] + | sort_by(.pr_number, .updated_at) + | group_by(.pr_number) + | map(last) + | .[] + ' <<< "$closed_state_comments" + ) +fi + +echo "state_issue_number=$state_issue_number" >> "$GITHUB_OUTPUT" diff --git a/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh b/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh new file mode 100644 index 00000000000000..c2610cfb2a8322 --- /dev/null +++ b/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh @@ -0,0 +1,168 @@ +#!/usr/bin/env bash +set -euo pipefail + +case "$OUTCOME" in + review-submitted|assessment-unchanged|'') + ;; + *) + echo "Unsupported worker callback outcome: ${OUTCOME}" >&2 + exit 1 + ;; +esac + +state_issue_number="$STATE_ISSUE_NUMBER" +state_comments="$( + gh api --method GET --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${state_issue_number}/comments?per_page=100" +)" +state_comment="$( + jq -c --arg pr_number "$PR_NUMBER" ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and ((.body // "") | startswith("")) + ) + ] + | last // empty + ' <<< "$state_comments" +)" +if [ -z "$state_comment" ]; then + echo "No state comment exists for pull request ${PR_NUMBER}." >&2 + exit 1 +fi +state_json="$( + jq -er ' + .body + | split("```json\n")[1] + | split("\n```")[0] + | fromjson + ' <<< "$state_comment" +)" +pr="$( + gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" \ + --json number,title,baseRefName,baseRefOid,headRefOid +)" +if [ "$(jq -r '.headRefOid' <<< "$pr")" != "$HEAD_SHA" ]; then + echo "Callback head SHA does not match the current pull request head." >&2 + exit 1 +fi + +find_current_review() { + gh api --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}/reviews?per_page=100" | + jq -c --arg head_sha "$HEAD_SHA" ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and .state == "COMMENTED" + and .commit_id == $head_sha + and ((.body // "") | contains("\n' + +state_issue_number="$STATE_ISSUE_NUMBER" + +state_comment="$( + gh api --method GET --paginate --slurp \ + "repos/${GITHUB_REPOSITORY}/issues/${state_issue_number}/comments?per_page=100" | + jq -c --arg pr_number "$PR_NUMBER" ' + [ + .[][] + | select( + .user.login == "github-actions[bot]" + and ((.body // "") | startswith("")) + ) + ] + | last // empty + ' +)" +current_pr="$(gh pr view "$PR_NUMBER" --repo "$GITHUB_REPOSITORY" --json headRefOid,baseRefName)" +current_pr_head="$(jq -r '.headRefOid' <<< "$current_pr")" +current_pr_base_ref="$(jq -r '.baseRefName' <<< "$current_pr")" +if [ -n "$state_comment" ]; then + current_state_json="$( + jq -er ' + .body + | split("```json\n")[1] + | split("\n```")[0] + | fromjson + ' <<< "$state_comment" + )" + # Do not let a delayed dispatch overwrite a completed callback for the current PR head/base. + if [ "$(jq -r '.last_reviewed_commit // ""' <<< "$current_state_json")" = "$current_pr_head" ] && + [ "$(jq -r '.last_reviewed_base_ref // ""' <<< "$current_state_json")" = "$current_pr_base_ref" ] && + { [ "$(jq -r '.last_reviewed_commit // ""' <<< "$STATE_JSON")" != "$current_pr_head" ] || + [ "$(jq -r '.last_reviewed_base_ref // ""' <<< "$STATE_JSON")" != "$current_pr_base_ref" ]; }; then + STATE_JSON="$current_state_json" + fi +fi +pr_title_html="$(jq -rn --arg text "$PR_TITLE" ' + $text + | gsub("&"; "&") + | gsub("<"; "<") + | gsub(">"; ">") + | gsub("\""; """) +')" +review_status_emoji="$(jq -r ' + if [ + .last_dispatched_commit // "", + .last_dispatched_base_ref // "" + ] == [ + .last_reviewed_commit // "", + .last_reviewed_base_ref // "" + ] + then ":heavy_check_mark:" + else ":eyes:" + end +' <<< "$STATE_JSON")" +state_body="$( + printf '%s%s -->\n
\n%s #%s - %s\n\n```json\n%s\n```\n
' \ + "$state_issue_comment_prefix" \ + "$PR_NUMBER" \ + "$review_status_emoji" \ + "$GITHUB_REPOSITORY" \ + "$PR_NUMBER" \ + "$PR_NUMBER" \ + "$pr_title_html" \ + "$STATE_JSON" +)" +if [ -n "$state_comment" ]; then + gh api --method PATCH \ + "repos/${GITHUB_REPOSITORY}/issues/comments/$(jq -er '.id' <<< "$state_comment")" \ + -f "body=${state_body}" > /dev/null +else + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/issues/${state_issue_number}/comments" \ + -f "body=${state_body}" > /dev/null +fi + +if [ -n "${WORKER_DISPATCH:-}" ] && [ "$WORKER_DISPATCH" != 'null' ]; then + worker_pr_number="$(jq -er '.pr_number' <<< "$WORKER_DISPATCH")" + worker_base_ref="$(jq -er '.base_ref' <<< "$WORKER_DISPATCH")" + worker_head_sha="$(jq -er '.head_sha' <<< "$WORKER_DISPATCH")" + worker_previous_head_sha="$(jq -er '.previous_head_sha' <<< "$WORKER_DISPATCH")" + worker_previous_base_sha="$(jq -er '.previous_base_sha' <<< "$WORKER_DISPATCH")" + worker_review_history="$(jq -er '.review_history' <<< "$WORKER_DISPATCH")" + worker_fetch_sha="$(jq -er '.fetch_sha' <<< "$WORKER_DISPATCH")" + worker_aw_context="$(jq -er '.aw_context' <<< "$WORKER_DISPATCH")" + + gh api --method POST \ + "repos/${GITHUB_REPOSITORY}/actions/workflows/holistic-review.lock.yml/dispatches" \ + -f "ref=${DEFAULT_BRANCH}" \ + -f "inputs[pr_number]=${worker_pr_number}" \ + -f "inputs[pr_base_ref]=${worker_base_ref}" \ + -f "inputs[pr_head_sha]=${worker_head_sha}" \ + -f "inputs[previous_head_sha]=${worker_previous_head_sha}" \ + -f "inputs[previous_base_sha]=${worker_previous_base_sha}" \ + -f "inputs[previous_review_history]=${worker_review_history}" \ + -f "inputs[fetch_sha]=${worker_fetch_sha}" \ + -f "inputs[aw_context]=${worker_aw_context}" > /dev/null +fi From 029bbeff5c054ced2cdaa5af5636854e221b950e Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Mon, 20 Jul 2026 18:44:49 -0700 Subject: [PATCH 4/5] Reduce holistic review verbosity Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/skills/code-review/SKILL.md | 41 ++++++++++++++-------- .github/workflows/holistic-review.lock.yml | 2 +- .github/workflows/holistic-review.md | 19 ++++++---- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 201f533c06d6ef..2ed6e924db8fec 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -120,21 +120,23 @@ When presenting the final review (whether as a PR comment or as output to the us ``` ## Holistic Review -**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real, when this adds non-obvious context> +`; add at most two closely related sentences when needed. Do not add top-level fields or Detailed Findings.> -**Approach**: <1-2 sentences on whether the fix/change takes the right approach, when this adds non-obvious context> + -**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. <2-3 sentence summary of the overall verdict and key points, when it adds non-obvious context. If "Needs Human Review," explicitly state which findings are uncertain and what a human reviewer should focus on.> +**Motivation**: ---- +**Approach**: + +**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. ### Detailed Findings #### ✅/⚠️/💡/❌ — - + -(Repeat for each finding category. Group related findings under a single heading.) + @@ -144,22 +146,31 @@ When presenting the final review (whether as a PR comment or as output to the us - Begin the review body with `## Holistic Review`. Include `**Motivation**:`, `**Approach**:`, and `**Summary**:` only when each adds non-obvious assessment beyond - the PR title, description, and code changes; omit any field that would merely restate - the obvious. When present, keep those fields in that order. Do not add a - `### Holistic Assessment` subheading, substitute a `Verdict` field, or rename those - fields. + the PR title, description, and code changes. For a routine LGTM review with no + non-obvious evidence to record, emit only `✅ LGTM — ` after + the heading, adding at most two closely related sentences when needed; omit the + fields and Detailed Findings. Otherwise, omit any field that would merely restate the + obvious, and keep present fields in that order. Do not add a `### Holistic Assessment` + subheading, substitute a `Verdict` field, or rename those fields. +- Treat each assertion as a budgeted claim: a routine LGTM review is one to three + sentences; a review with findings has a one-sentence Summary and one concise paragraph + per finding. Do not repeat a verdict, rationale, or requested action between a + top-level field, a detailed finding, and an inline comment. - **Detailed Findings** uses emoji-prefixed category headers: - - ✅ for things that are correct / look good (use to confirm important aspects were verified) + - ✅ only for independently useful, non-obvious evidence that is worth recording - ⚠️ for warnings or impactful suggestions (should fix, or follow-up) - 💡 for minor suggestions or observations (nice-to-have) - ❌ for errors (must fix before merge) +- Do not use a finding merely to list unaffected components, declare that no public API + changed, or restate the PR diff. Omit routine "no concerns" assurances. - **Cross-cutting analysis** should be included when relevant: check whether related code (sibling types, callers, other platforms) is affected by the same issue or needs a similar fix. -- **Test quality** should be assessed as its own finding when tests are part of the PR. +- Assess test quality whenever tests are part of the PR, but report it as its own finding + only when that adds non-obvious evidence or actionable feedback. - When included, **Summary** gives a clear verdict: `❌ Needs Changes`, `⚠️ Needs Human Review`, `💡 Suggestions`, `✅ LGTM`, or `❌ Reject`. Use `✅ LGTM` only when confident - and no suggestions remain. When Summary is omitted, make the verdict clear through the - detailed findings. When uncertain, use `⚠️ Needs Human Review` and explain what a human - should focus on. + and no suggestions remain. When the optional fields are omitted for a routine LGTM, + use the concise `✅ LGTM` form above. When uncertain, use `⚠️ Needs Human Review` and + explain what a human should focus on. - Keep the review concise but thorough. Every claim should be backed by evidence from the code. ### Verdict Consistency Rules diff --git a/.github/workflows/holistic-review.lock.yml b/.github/workflows/holistic-review.lock.yml index c212e724afc2a7..9cfad635db400b 100644 --- a/.github/workflows/holistic-review.lock.yml +++ b/.github/workflows/holistic-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b686905ca0d3a7422d66e496cb390941ca2dd33088435db8af5f7ae6ba67ce88","body_hash":"e76767cbb0ee3f0eaf898c176e68768e17a06364291f73d930d23b4b3457b622","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b686905ca0d3a7422d66e496cb390941ca2dd33088435db8af5f7ae6ba67ce88","body_hash":"063ae637087d9b0c5881da6548005922e293ae82d2eafec54ecfaf98cfa39d6c","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"cec6394202d7db187b02310d928812194988eb20","version":"v0.82.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27","digest":"sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27@sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27","digest":"sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27@sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27","digest":"sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27@sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27","digest":"sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27@sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.0","digest":"sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.0@sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw (v0.82.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # diff --git a/.github/workflows/holistic-review.md b/.github/workflows/holistic-review.md index 1254d2a4124710..a1a2fdaa5a15f1 100644 --- a/.github/workflows/holistic-review.md +++ b/.github/workflows/holistic-review.md @@ -404,13 +404,18 @@ the complexity is poorly encapsulated or duplicative, or the demonstrated benefi justify the maintenance burden. When the tradeoff remains unresolved, use `⚠️ Needs Human Review` and state the specific decision a maintainer should make. -Use the review skill's top-level body structure. After `## Holistic Review`, include -`**Motivation**:`, `**Approach**:`, and `**Summary**:` only when each adds non-obvious assessment -beyond the PR title, description, and code changes; omit Motivation and/or Summary when they would -merely restate the obvious. When present, keep those fields in that order. Then emit -`### Detailed Findings`. Do not duplicate findings or assessments across the optional top-level -fields, detailed findings, or inline comments. Use only `✅`, `⚠️`, `💡`, and `❌` as -review-content status emojis. +Use the review skill's top-level body structure. For a routine LGTM review with no non-obvious +evidence to record, emit only `✅ LGTM — ` after `## Holistic Review`, adding +at most two closely related sentences when needed; omit the top-level fields and +`### Detailed Findings`. Otherwise, include `**Motivation**:`, `**Approach**:`, and `**Summary**:` +only when each adds non-obvious assessment beyond the PR title, description, and code changes. +Omit any field that merely restates the obvious, keeping present fields in that order. Do not +duplicate a verdict, rationale, or requested action across top-level fields, detailed findings, or +inline comments. Treat each assertion as a budgeted claim: use a one-sentence Summary and one +concise paragraph per finding. Do not add findings merely to list unaffected components, say that +no public API changed, or restate the PR diff. Use `✅` only for independently useful, non-obvious +evidence; otherwise, omit positive findings. Use only `✅`, `⚠️`, `💡`, and `❌` as review-content +status emojis. For an incremental review, compare the complete current assessment with the most recent previous review. The assessment is unchanged only when its emitted top-level fields, findings, and action From 98a9f077d13878f23bb5ec74ff7d0a084953266d Mon Sep 17 00:00:00 2001 From: Jeff Handley Date: Mon, 20 Jul 2026 21:22:22 -0700 Subject: [PATCH 5/5] Skip non-actionable holistic reviews Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../workflows/holistic-review-completed.yml | 2 +- .github/workflows/holistic-review.lock.yml | 4 +- .github/workflows/holistic-review.md | 59 ++++----- .../holistic-review/orchestrator-dispatch.sh | 119 +++++++++++------- .../orchestrator-reconcile-state.sh | 36 ++++-- 5 files changed, 135 insertions(+), 85 deletions(-) diff --git a/.github/workflows/holistic-review-completed.yml b/.github/workflows/holistic-review-completed.yml index e693f556ccbb42..2aeff776475568 100644 --- a/.github/workflows/holistic-review-completed.yml +++ b/.github/workflows/holistic-review-completed.yml @@ -12,7 +12,7 @@ on: required: true type: string outcome: - description: 'One of: review-submitted or assessment-unchanged.' + description: 'One of: review-submitted, assessment-unchanged, or no-actionable-feedback.' required: true type: string diff --git a/.github/workflows/holistic-review.lock.yml b/.github/workflows/holistic-review.lock.yml index 9cfad635db400b..a69922eb9406f8 100644 --- a/.github/workflows/holistic-review.lock.yml +++ b/.github/workflows/holistic-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b686905ca0d3a7422d66e496cb390941ca2dd33088435db8af5f7ae6ba67ce88","body_hash":"063ae637087d9b0c5881da6548005922e293ae82d2eafec54ecfaf98cfa39d6c","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"b686905ca0d3a7422d66e496cb390941ca2dd33088435db8af5f7ae6ba67ce88","body_hash":"7c8335c7ffb6c2210a8011b3a2fd9b94a82ec935b0f9e7a2341ace169afb4174","compiler_version":"v0.82.6","strict":true,"agent_id":"copilot","agent_model":"${{ vars.HOLISTIC_REVIEW_MODEL }}","engine_versions":{"copilot":"1.0.68"}} # gh-aw-manifest: {"version":1,"secrets":["COPILOT_PAT_0","COPILOT_PAT_1","COPILOT_PAT_2","COPILOT_PAT_3","COPILOT_PAT_4","COPILOT_PAT_5","COPILOT_PAT_6","COPILOT_PAT_7","COPILOT_PAT_8","COPILOT_PAT_9","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0","version":"v7.0.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e","version":"v6.4.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"cec6394202d7db187b02310d928812194988eb20","version":"v0.82.6"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27","digest":"sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.27@sha256:bb5a0150dcff1cddf9b8045bb411b7759806bace0abcb132fb22158073e155d9"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27","digest":"sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.27@sha256:01e58c4383fa9952abe76e0a134a27c970f81f744d6b7861fc9e08b7964d94c3"},{"image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27","digest":"sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a","pinned_image":"ghcr.io/github/gh-aw-firewall/cli-proxy:0.27.27@sha256:70df326caf73bf5911340dca4620b529a483dd8f42142b0a41d7b9761ab4ab7a"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27","digest":"sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.27@sha256:92d820df47b2eff75d93a5bec4dc183a3ec55ed7ddb4f25cb0fdda5c3e995409"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.0","digest":"sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.0@sha256:9dbdf42842c224a95016df1d2a85a2901e04204c242079343b302a307d2b8031"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b","pinned_image":"ghcr.io/github/gh-aw-node@sha256:529d02eb970b1161aa25c593a9c3df57fdfad5a8add328cb3b6eccef66f3183b"},{"image":"ghcr.io/github/github-mcp-server:v1.5.0","digest":"sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4","pinned_image":"ghcr.io/github/github-mcp-server:v1.5.0@sha256:e25564dccc9110a70a77b9df560cbde11aa392fcb5f08b9abe5c4ebc6d146ea4"}]} # This file was automatically generated by gh-aw (v0.82.6). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -634,7 +634,7 @@ jobs: "type": "string" }, "outcome": { - "description": "One of: review-submitted or assessment-unchanged.", + "description": "One of: review-submitted, assessment-unchanged, or no-actionable-feedback.", "type": "string" }, "pr_number": { diff --git a/.github/workflows/holistic-review.md b/.github/workflows/holistic-review.md index a1a2fdaa5a15f1..c1c83dd526aa0c 100644 --- a/.github/workflows/holistic-review.md +++ b/.github/workflows/holistic-review.md @@ -378,10 +378,9 @@ not rediscover findings in portions of the PR patch that remained unchanged. These re-review scope rules override any broader review-scope guidance in the review skill. If `HOLISTIC_REVIEW_HAS_CHANGES` is `false`, do not inspect the source patch for new findings. -For an initial review, submit a `COMMENT` review explaining that the PR has no base-to-head -changes. For a re-review, compare the resulting assessment with the most recent previous review -as described in Step 3; if it is unchanged, record the no-op result instead of posting a duplicate -review. +Treat an initial review with no base-to-head changes as having no actionable feedback. For a +re-review, compare the resulting assessment with the most recent previous review as described in +Step 3; record an unchanged assessment without posting a duplicate review. ## Step 2: Load Review Guidelines @@ -404,34 +403,38 @@ the complexity is poorly encapsulated or duplicative, or the demonstrated benefi justify the maintenance burden. When the tradeoff remains unresolved, use `⚠️ Needs Human Review` and state the specific decision a maintainer should make. -Use the review skill's top-level body structure. For a routine LGTM review with no non-obvious -evidence to record, emit only `✅ LGTM — ` after `## Holistic Review`, adding -at most two closely related sentences when needed; omit the top-level fields and -`### Detailed Findings`. Otherwise, include `**Motivation**:`, `**Approach**:`, and `**Summary**:` -only when each adds non-obvious assessment beyond the PR title, description, and code changes. -Omit any field that merely restates the obvious, keeping present fields in that order. Do not -duplicate a verdict, rationale, or requested action across top-level fields, detailed findings, or -inline comments. Treat each assertion as a budgeted claim: use a one-sentence Summary and one -concise paragraph per finding. Do not add findings merely to list unaffected components, say that -no public API changed, or restate the PR diff. Use `✅` only for independently useful, non-obvious -evidence; otherwise, omit positive findings. Use only `✅`, `⚠️`, `💡`, and `❌` as review-content -status emojis. +Only submit a pull request review when it contains new actionable feedback. A routine LGTM, +positive evidence, an unchanged assessment, a restatement of the PR, or an assurance that no +issues were found is not actionable and must not create a review. This overrides the review +skill's routine-LGTM emission guidance for this workflow. + +Use the review skill's top-level body structure only for actionable reviews. Include +`**Motivation**:`, `**Approach**:`, and `**Summary**:` only when each adds non-obvious assessment +beyond the PR title, description, and code changes. Omit any field that merely restates the +obvious, keeping present fields in that order. Do not duplicate a verdict, rationale, or requested +action across top-level fields, detailed findings, or inline comments. Treat each assertion as a +budgeted claim: use a one-sentence Summary and one concise paragraph per finding. Do not add +findings merely to list unaffected components, say that no public API changed, or restate the PR +diff. Use only `⚠️`, `💡`, and `❌` as actionable review-content status emojis. + +When the current assessment has no new actionable feedback, whether it is the initial assessment +or an unchanged incremental assessment, do not create inline comments or submit a pull request +review. Invoke `call_workflow` exactly once to record the completion: + +```bash +safeoutputs call_workflow . <<'EOF' +{"workflow_name":"holistic-review-completed","pr_number":"${{ github.event.inputs.pr_number }}","head_sha":"${{ github.event.inputs.pr_head_sha }}","outcome":"no-actionable-feedback"} +EOF +``` For an incremental review, compare the complete current assessment with the most recent previous review. The assessment is unchanged only when its emitted top-level fields, findings, and action items are all unchanged; an updated commit or wording that restates the same assessment does not -make it different. When the assessment is unchanged: - -1. Do not create inline comments or submit a new pull request review. -2. Invoke `call_workflow` exactly once to record the completion: - - ```bash - safeoutputs call_workflow . <<'EOF' - {"workflow_name":"holistic-review-completed","pr_number":"${{ github.event.inputs.pr_number }}","head_sha":"${{ github.event.inputs.pr_head_sha }}","outcome":"assessment-unchanged"} - EOF - ``` +make it different. An unchanged assessment has no new actionable feedback: do not create inline +comments or submit a pull request review, and record it with the `no-actionable-feedback` +completion above. -For every review that you submit, invoke `call_workflow` exactly once after invoking +For every actionable review that you submit, invoke `call_workflow` exactly once after invoking `submit_pull_request_review`, with `outcome` set to `review-submitted`: ```bash @@ -462,7 +465,7 @@ safeoutputs submit_pull_request_review . <<'EOF' EOF ``` -Set `pull_request_number` to `${{ github.event.inputs.pr_number }}` and include the complete review body as a valid JSON string. If the command is rejected, correct the JSON or invocation and retry this exact form once. Except for the unchanged incremental-assessment case above, always submit a `COMMENT` event, including for an LGTM verdict. Never submit `REQUEST_CHANGES`. Inline comments created above are automatically included in this review. End every submitted review with this disclosure, replacing the generic Copilot disclosure in the review skill: +Set `pull_request_number` to `${{ github.event.inputs.pr_number }}` and include the complete review body as a valid JSON string. If the command is rejected, correct the JSON or invocation and retry this exact form once. Submit a `COMMENT` event only for new actionable feedback. Never submit `REQUEST_CHANGES`. Inline comments created above are automatically included in this review. End every submitted review with this disclosure, replacing the generic Copilot disclosure in the review skill: > [!NOTE] > This review was generated by this repository's [Holistic Review](${{ github.server_url }}/${{ github.repository }}/blob/main/.github/workflows/holistic-review.md) agentic workflow to complement the built-in Copilot review. diff --git a/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh b/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh index ece39119370b4e..0bf36f1515a06b 100644 --- a/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh +++ b/.github/workflows/scripts/holistic-review/orchestrator-dispatch.sh @@ -191,6 +191,9 @@ while IFS= read -r entry; do review_attempt_commit='' review_attempt_base_ref='' review_attempt_count=0 + last_no_actionable_commit='' + last_no_actionable_base_ref='' + last_no_actionable_base_sha='' worker_dispatch='null' manual_retry_reset=false if [ -n "$state_comment" ]; then @@ -210,6 +213,9 @@ while IFS= read -r entry; do review_attempt_commit="$(jq -r '.review_attempt_commit // ""' <<< "$state_json")" review_attempt_base_ref="$(jq -r '.review_attempt_base_ref // ""' <<< "$state_json")" review_attempt_count="$(jq -r '.review_attempt_count // 0' <<< "$state_json")" + last_no_actionable_commit="$(jq -r '.last_no_actionable_commit // ""' <<< "$state_json")" + last_no_actionable_base_ref="$(jq -r '.last_no_actionable_base_ref // ""' <<< "$state_json")" + last_no_actionable_base_sha="$(jq -r '.last_no_actionable_base_sha // ""' <<< "$state_json")" fi write_state_comment() { @@ -227,9 +233,12 @@ while IFS= read -r entry; do --arg review_attempt_base_ref "$review_attempt_base_ref" \ --argjson review_attempt_count "$review_attempt_count" \ --argjson max_review_attempts "$MAX_REVIEW_ATTEMPTS" \ + --arg last_no_actionable_commit "$last_no_actionable_commit" \ + --arg last_no_actionable_base_ref "$last_no_actionable_base_ref" \ + --arg last_no_actionable_base_sha "$last_no_actionable_base_sha" \ --argjson review_history "$review_history" ' { - version: 8, + version: 9, last_dispatched_commit: $last_dispatched_commit, last_dispatched_base_ref: $last_dispatched_base_ref, last_dispatched_base_sha: $last_dispatched_base_sha, @@ -242,7 +251,10 @@ while IFS= read -r entry; do review_attempt_count: $review_attempt_count, max_review_attempts: $max_review_attempts, review_history_format: "holistic-review-disclosure-v1", - review_history: $review_history + review_history: $review_history, + last_no_actionable_commit: $last_no_actionable_commit, + last_no_actionable_base_ref: $last_no_actionable_base_ref, + last_no_actionable_base_sha: $last_no_actionable_base_sha } ' )" @@ -260,13 +272,25 @@ while IFS= read -r entry; do ' >> "$state_updates_file" } - # Pull request reviews are authoritative for review identity. An unchanged-assessment - # callback advances the latest authenticated review's commit in the state comment, so - # retain that commit while the physical review remains the same. + # A no-actionable completion has no physical review. Keep its durable completion marker + # authoritative for this head/base pair instead of deriving an empty review history as pending. state_needs_update=false + has_no_actionable_completion=false + if [ "$last_no_actionable_commit" = "$head_sha" ] && + [ "$last_no_actionable_base_ref" = "$base_ref" ]; then + has_no_actionable_completion=true + elif [ -n "$last_no_actionable_commit" ] || + [ -n "$last_no_actionable_base_ref" ] || + [ -n "$last_no_actionable_base_sha" ]; then + last_no_actionable_commit='' + last_no_actionable_base_ref='' + last_no_actionable_base_sha='' + state_needs_update=true + fi expected_summary_emoji=':eyes:' - if [ "$last_dispatched_commit" = "$last_reviewed_commit" ] && - [ "$last_dispatched_base_ref" = "$last_reviewed_base_ref" ]; then + if [ "$has_no_actionable_completion" = true ] || + { [ "$last_dispatched_commit" = "$last_reviewed_commit" ] && + [ "$last_dispatched_base_ref" = "$last_reviewed_base_ref" ]; }; then expected_summary_emoji=':heavy_check_mark:' fi if [ -n "$state_comment" ] && @@ -275,48 +299,50 @@ while IFS= read -r entry; do <<< "$state_comment" > /dev/null; then state_needs_update=true fi - refreshed_review_history="$(get_review_history "$pr_number")" - recorded_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$review_history")" - refreshed_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$refreshed_review_history")" - if [ "$recorded_review_id" = "$refreshed_review_id" ] && [ -n "$recorded_review_id" ]; then - refreshed_review_history="$( - jq -cn \ - --argjson review_history "$review_history" \ - --argjson refreshed_review_history "$refreshed_review_history" ' - $refreshed_review_history - | .[-1].commit = $review_history[-1].commit - ' - )" - fi - if [ "$review_history" != "$refreshed_review_history" ]; then - review_history="$refreshed_review_history" - state_needs_update=true - fi + if [ "$has_no_actionable_completion" = false ]; then + refreshed_review_history="$(get_review_history "$pr_number")" + recorded_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$review_history")" + refreshed_review_id="$(jq -r 'if length == 0 then "" else .[-1].review_id end' <<< "$refreshed_review_history")" + if [ "$recorded_review_id" = "$refreshed_review_id" ] && [ -n "$recorded_review_id" ]; then + refreshed_review_history="$( + jq -cn \ + --argjson review_history "$review_history" \ + --argjson refreshed_review_history "$refreshed_review_history" ' + $refreshed_review_history + | .[-1].commit = $review_history[-1].commit + ' + )" + fi + if [ "$review_history" != "$refreshed_review_history" ]; then + review_history="$refreshed_review_history" + state_needs_update=true + fi - refreshed_reviewed_commit="$(jq -r 'if length == 0 then "" else .[-1].commit end' <<< "$review_history")" - if [ "$last_reviewed_commit" != "$refreshed_reviewed_commit" ]; then - last_reviewed_commit="$refreshed_reviewed_commit" - state_needs_update=true - fi - if [ -n "$last_reviewed_commit" ]; then - if [ "$last_reviewed_base_ref" != "$base_ref" ] || - [ "$last_reviewed_base_sha" != "$base_sha" ]; then - last_reviewed_base_ref="$base_ref" - last_reviewed_base_sha="$base_sha" + refreshed_reviewed_commit="$(jq -r 'if length == 0 then "" else .[-1].commit end' <<< "$review_history")" + if [ "$last_reviewed_commit" != "$refreshed_reviewed_commit" ]; then + last_reviewed_commit="$refreshed_reviewed_commit" state_needs_update=true fi - if [ -n "$review_attempt_commit" ] || - [ -n "$review_attempt_base_ref" ] || - [ "$review_attempt_count" -ne 0 ]; then - review_attempt_commit='' - review_attempt_base_ref='' - review_attempt_count=0 + if [ -n "$last_reviewed_commit" ]; then + if [ "$last_reviewed_base_ref" != "$base_ref" ] || + [ "$last_reviewed_base_sha" != "$base_sha" ]; then + last_reviewed_base_ref="$base_ref" + last_reviewed_base_sha="$base_sha" + state_needs_update=true + fi + if [ -n "$review_attempt_commit" ] || + [ -n "$review_attempt_base_ref" ] || + [ "$review_attempt_count" -ne 0 ]; then + review_attempt_commit='' + review_attempt_base_ref='' + review_attempt_count=0 + state_needs_update=true + fi + elif [ -n "$last_reviewed_base_ref" ] || [ -n "$last_reviewed_base_sha" ]; then + last_reviewed_base_ref='' + last_reviewed_base_sha='' state_needs_update=true fi - elif [ -n "$last_reviewed_base_ref" ] || [ -n "$last_reviewed_base_sha" ]; then - last_reviewed_base_ref='' - last_reviewed_base_sha='' - state_needs_update=true fi if [ -n "$PR_NUMBERS" ] && @@ -332,8 +358,9 @@ while IFS= read -r entry; do state_needs_update=true fi - if [ "$last_reviewed_commit" = "$head_sha" ] && - [ "$last_reviewed_base_ref" = "$base_ref" ]; then + if [ "$has_no_actionable_completion" = true ] || + { [ "$last_reviewed_commit" = "$head_sha" ] && + [ "$last_reviewed_base_ref" = "$base_ref" ]; }; then if [ -n "$review_attempt_commit" ] || [ -n "$review_attempt_base_ref" ] || [ "$review_attempt_count" -ne 0 ]; then diff --git a/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh b/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh index c2610cfb2a8322..c25391b72b8672 100644 --- a/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh +++ b/.github/workflows/scripts/holistic-review/orchestrator-reconcile-state.sh @@ -2,7 +2,7 @@ set -euo pipefail case "$OUTCOME" in - review-submitted|assessment-unchanged|'') + review-submitted|assessment-unchanged|no-actionable-feedback|'') ;; *) echo "Unsupported worker callback outcome: ${OUTCOME}" >&2 @@ -78,11 +78,8 @@ if [ -z "$OUTCOME" ]; then done if [ "$current_review" != "null" ] && [ -n "$current_review" ]; then OUTCOME='review-submitted' - elif [ "$(jq 'length' <<< "$review_history")" -gt 0 ]; then - OUTCOME='assessment-unchanged' else - echo "Worker callback did not identify a submitted review or a prior assessment." >&2 - exit 1 + OUTCOME='no-actionable-feedback' fi fi @@ -128,20 +125,40 @@ case "$OUTCOME" in ' <<< "$review_history" )" ;; + no-actionable-feedback) + review_history='[]' + ;; *) echo "Unsupported worker callback outcome: ${OUTCOME}" >&2 exit 1 ;; esac +base_ref="$(jq -r '.baseRefName' <<< "$pr")" +base_sha="$(jq -r '.baseRefOid' <<< "$pr")" +last_no_actionable_commit='' +last_no_actionable_base_ref='' +last_no_actionable_base_sha='' +case "$OUTCOME" in + no-actionable-feedback) + review_history='[]' + last_no_actionable_commit="$HEAD_SHA" + last_no_actionable_base_ref="$base_ref" + last_no_actionable_base_sha="$base_sha" + ;; +esac + state_json="$( jq -c \ --arg head_sha "$HEAD_SHA" \ - --arg base_ref "$(jq -r '.baseRefName' <<< "$pr")" \ - --arg base_sha "$(jq -r '.baseRefOid' <<< "$pr")" \ + --arg base_ref "$base_ref" \ + --arg base_sha "$base_sha" \ --arg title "$(jq -r '.title' <<< "$pr")" \ + --arg last_no_actionable_commit "$last_no_actionable_commit" \ + --arg last_no_actionable_base_ref "$last_no_actionable_base_ref" \ + --arg last_no_actionable_base_sha "$last_no_actionable_base_sha" \ --argjson review_history "$review_history" ' - .version = 8 + .version = 9 | .last_dispatched_commit = $head_sha | .last_dispatched_base_ref = $base_ref | .last_dispatched_base_sha = $base_sha @@ -154,6 +171,9 @@ state_json="$( | .review_attempt_count = 0 | .review_history_format = "holistic-review-disclosure-v1" | .review_history = $review_history + | .last_no_actionable_commit = $last_no_actionable_commit + | .last_no_actionable_base_ref = $last_no_actionable_base_ref + | .last_no_actionable_base_sha = $last_no_actionable_base_sha | del(.last_recorded_worker_run_id) ' <<< "$state_json" )"