From 0e0e2c51d65d7fa33d6ded0e48459e9e21a4f323 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Wed, 2 Sep 2026 14:28:19 +0200 Subject: [PATCH] experiment: give the super-skill authority to reject leaf findings (DO NOT MERGE) Today the super-skill is a pass-through: Roll up step 5 appends every non-duplicate leaf finding unconditionally, and the only rejection path is step 3 (mechanical sub-skill failure). The self-review pass validates the super-skill's own candidates against knowledge, and an explicit clause exempts leaf findings from that check. No role in the pipeline can veto a leaf. That shows up in the BC-Bench code-review numbers: on the parallel-leaf baseline the agent generates ~630 comments to match ~100 expected ones (precision 0.15-0.16), and a single leaf (al-appsource) accounts for 27-29% of everything emitted while the gold set expects zero findings from it. This arm adds an adjudication gate at roll-up time. The super-skill re-reads the cited code and rejects findings whose precondition does not hold, that the code does not bear out, that loaded knowledge explicitly permits, or that name no actionable change. Rejections are recorded in a new audit-only 'adjudicated-out[]' field; the engine ignores unknown report keys, so nothing downstream changes shape. The gate is deliberately conservative ('when ambiguous, keep the finding') so the measured result is a lower bound on what arbitration can buy. Branched from 712dee9 (the commit the BC-Bench baseline pins) so the only delta against that baseline is this contract change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e33a7b96-4821-4797-9f72-96880f3c6412 --- microsoft/skills/review/al-code-review.md | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index 9b3f934..72825cd 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -80,7 +80,14 @@ For each sub-skill in the worklist, executed one at a time per the discipline ab 2. Capture the sub-skill's complete findings-report verbatim and append it to `sub-results`. 3. If the sub-skill's `outcome` is `failed`, stop here for this sub-skill: its findings are not reliable per the DO contract and MUST NOT be copied into the super-skill's top-level `findings[]` or counted in `summary.counts`. 4. Otherwise, compare each entry from the sub-skill's `findings[]` with findings already rolled up. Two findings are duplicates when they point to the same file and overlapping line/range and prescribe materially the same correction, even when their knowledge-file IDs differ. Merge duplicates instead of appending both: keep the more specific domain owner, preserve that finding's optional `domain` field verbatim (including its absence), use its reference as `references[0]` and therefore as `id`, append the other references as supporting references, keep the highest severity and confidence justified by either report, and preserve one self-contained message. Article and leaf ownership notes decide specificity; do not choose by execution order. -5. Append each non-duplicate finding, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving its optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `:` to prevent collisions across sub-skills. Other finding fields are preserved. +5. **Adjudicate each non-duplicate finding before appending it.** The super-skill is the review's final quality gate. A leaf reports what its own domain knowledge matched, in isolation; only the super-skill sees that finding beside the full diff and the other leaves' reports. Re-read the cited lines and decide whether the finding survives. **Reject** it — do not append it, and record it in `adjudicated-out[]` as `{ id, from-sub-skill, location, reason }` — when any of the following holds: + - **Precondition not met.** The applicability condition behind the finding does not actually hold here: the construct the knowledge file targets is absent, or this file is outside the article's declared scope. + - **Unsupported by the code.** The finding asserts something the cited lines do not bear out — a symbol, property, call, or control-flow claim that is not there. Re-read the code before accepting; do not take the leaf's description on trust. + - **Contradicted by knowledge.** A knowledge file loaded for this task explicitly permits what the leaf flagged (its `## Best Practice` or `## Anti Pattern` says the opposite). + - **Not actionable.** The finding names no concrete, correct change the author could make at that location. + + Reject on evidence, never on taste. A correct, in-scope, actionable finding stays even when it is low severity, repeats a theme already reported elsewhere in the diff, or is not one you would have raised yourself. When the evidence is genuinely ambiguous, keep the finding. +6. Append each surviving finding, setting `from-sub-skill` to the sub-skill's `skill.id` and preserving its optional `domain` field verbatim, including its absence. For non-citation findings (those whose `id` is a skill-defined slug rather than a reference path), prefix `id` with `:` to prevent collisions across sub-skills. Other finding fields are preserved. ### Agent self-review pass @@ -106,7 +113,7 @@ For every candidate the agent identifies in this pass: - `message` is non-empty and self-contained, describing both the issue and a concrete recommendation. A consumer rendering the finding has no knowledge-file footer to fall back on. - `suggested-code` MUST be set when the fix is small, local, and mechanical. If a mechanical-looking finding omits it, set `suggested-code-omission-reason` with the reason (for example, the fix spans non-contiguous code or requires choosing a real production value). -Leaf-level agent findings (those with `references: []` inside a sub-skill's report) are rolled up into the super-skill's top-level `findings[]` like any other sub-skill finding — they keep their `from-sub-skill: ` attribution and are not rewritten. They are not subject to the "MUST validate against knowledge" step above, because each leaf has already validated within its own domain. +Leaf-level agent findings (those with `references: []` inside a sub-skill's report) are rolled up into the super-skill's top-level `findings[]` like any other sub-skill finding — they keep their `from-sub-skill: ` attribution and are not rewritten. They are not subject to the "MUST validate against knowledge" step above, which governs the super-skill's *own* self-review candidates. They **are** subject to the adjudication gate in *Roll up sub-skill findings* step 5, which applies to every finding a leaf reports, knowledge-backed or agent. ### Suggested-code guidance @@ -116,7 +123,7 @@ Sub-skills MAY also emit `suggested-code` when their knowledge file unambiguousl ### Summary and rollup -Aggregate `summary.counts` and `summary.coverage` as the sums across invoked sub-skills whose `outcome` is not `failed`. Agent findings emitted by the super-skill itself contribute to `summary.counts` but not to `summary.coverage` (coverage is a sub-skill worklist metric and is undefined for self-review). +Aggregate `summary.counts` and `summary.coverage` as the sums across invoked sub-skills whose `outcome` is not `failed`. `summary.counts` reflects the findings that **survived adjudication**, not the raw sub-skill totals; `summary.coverage` is a worklist metric and is unaffected by adjudication. Agent findings emitted by the super-skill itself contribute to `summary.counts` but not to `summary.coverage` (coverage is a sub-skill worklist metric and is undefined for self-review). `suppressed[]` at the super-skill level remains empty. Knowledge-file-level suppression is reported by each sub-skill within its own entry in `sub-results`. @@ -126,7 +133,7 @@ Before emitting the rollup, apply DO's reference-integrity gate to every nested ## Output -Output conforms to the DO output contract, extended with `sub-results` and `skipped-sub-skills`. A populated example — both leaves ran, each produced findings: +Output conforms to the DO output contract, extended with `sub-results`, `skipped-sub-skills`, and `adjudicated-out`. `adjudicated-out[]` records the sub-skill findings the super-skill rejected in *Roll up sub-skill findings* step 5 — each entry is `{ id, from-sub-skill, location, reason }`, where `reason` names which rejection criterion applied. It is an audit trail only; rejected findings do not appear in `findings[]` and are not counted in `summary.counts`. An empty array is valid and expected when every finding survived. A populated example — both leaves ran, each produced findings: ```json {