Address Holistic Review Workflow Feedback

.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,62 +113,89 @@ 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
 
 ```
 ## Holistic Review
 
-**Motivation**: <1-2 sentences on whether the PR is justified and the problem is real>
+<For a routine LGTM review with no non-obvious evidence to record, emit only `✅ LGTM — <one concise sentence>`; 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>
+<Otherwise, include only the applicable fields below.>
 
-**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.>
+**Motivation**: <one sentence on why the PR is justified, only when it adds non-obvious context>
 
----
+**Approach**: <one sentence on whether the change takes the right approach, only when it adds non-obvious context>
+
+**Summary**: <❌ Needs Changes / ⚠️ Needs Human Review / 💡 Suggestions / ✅ LGTM / ❌ Reject>. <one concise sentence naming the outcome and required action or non-obvious evidence.>
 
 ### Detailed Findings
 
-#### ✅/⚠️/❌ <Category Name> — <Brief description>
+#### ✅/⚠️/💡/❌ <Category Name> — <Brief description>
 
-<Explanation with specifics. Reference code, line numbers, interleavings, etc.>
+<One concise paragraph with specific evidence and, for ⚠️/💡/❌ findings, the requested action.>
 
-(Repeat for each finding category. Group related findings under a single heading.)
+<Include only findings that add non-obvious evidence or actionable feedback. Group related findings under a single heading.>
 
 <!-- AI disclosure note: place any AI-generated content disclosure below this line. -->
 <!-- Example: > [!NOTE] This review was created by GitHub Copilot. -->
 ```
 
 ### 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. For a routine LGTM review with no
+  non-obvious evidence to record, emit only `✅ LGTM — <one concise sentence>` 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 errors (must fix before merge)
   - 💡 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.
-- **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.
+- 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 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
 
 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 +210,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.

.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, assessment-unchanged, or no-actionable-feedback.'
+        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