Normalize verified citation IDs in accepted report copies

docs/contributing.md

@@ -168,8 +168,8 @@ pwsh .\tools\Test-ReviewContract.ps1 -Root .
 The first command checks schema, sections, naming, sample references, and
 skill registration. The second checks that every review leaf has a valid
 positive/clean sample pair. The third checks the cross-surface findings-report
-contract and its bounded range-normalization cases. None proves a model will
-find every defect. See [evaluation](../evaluation/README.md) for optional
+contract and its bounded citation-ID/range-normalization cases. None proves a
+model will find every defect. See [evaluation](../evaluation/README.md) for optional
 model-based scoring.
 
 In the PR description, explain the mistake being prevented, supporting

docs/standalone-runner.md

@@ -59,9 +59,11 @@ only result.
 5. Capture the exact Task return as the immutable raw audit payload and primary
    transport. Preserve it unchanged in private artifacts or host logs. Before
    the full DO acceptance gate, create a normalized candidate only for DO's
-   bounded optional-range case, record that normalization separately in private
-   telemetry, and accept the candidate only if the entire copy passes the
-  unchanged strict gate. Use `tools/Validate-FindingsReport.ps1`, passing the
+   bounded citation-ID and optional-range cases after strict JSON, full
+   structural schema, and reference-integrity checks. Record `normalizedIds`
+   and `removedRanges` separately in private telemetry, and accept the
+   candidate only if the entire copy passes the unchanged strict gate.
+  Use `tools/Validate-FindingsReport.ps1`, passing the
   exact source paths and fully retrieved article paths; pass `-SkillKind super`
   and `-ExpectedCompositionPath` for the final rolled-up report. The accepted report contains no undeclared
   telemetry fields.

microsoft/skills/review/al-code-review.md

@@ -95,7 +95,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene
 
 - **Isolate leaf invocations when the host supports it.** Each sub-skill SHOULD run in a fresh model call or child context containing only its assigned source paths, READ/DO contracts, the leaf instructions, the complete bounded domain catalog per READ, and articles that leaf worklists. Preserve each catalog row's exact `path`; the leaf must copy references from that catalog.
 - **Keep run artifacts private.** Before dispatch, allocate a new GUID-named directory under the current session's artifact directory and a distinct scratch/report child directory for every leaf. Pass a leaf only its own assigned source paths and child directory, never the run root or sibling paths. A leaf MUST NOT discover, enumerate, read, modify, or delete sibling artifacts. Do not reuse a prior run directory, and do not clean up any run artifact until every leaf has finished and consolidation is complete.
-- **Keep raw Task transport distinct from the accepted copy.** Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log. Then apply DO's bounded pre-gate range normalization, when eligible, and its full consumer acceptance gate. The report accepted for rollup is the exact return when no normalization occurred, or the normalized candidate copy when DO permits it; worker-side persistence of another report file is optional and redundant.
+- **Keep raw Task transport distinct from the accepted copy.** Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log. Then apply DO's bounded citation-ID and optional-range normalization, when eligible, and its full consumer acceptance gate. The report accepted for rollup is the exact return when no normalization occurred, or the normalized candidate copy when DO permits it; worker-side persistence of another report file is optional and redundant.
 - **Treat automatic output spills as host-owned.** If the host reports that a Task return was automatically spilled, the coordinator MAY read that file read-only only at the exact path returned by the tool. Never modify, delete, enumerate around, or reuse an automatic spill path. Never bypass a content-exclusion or access denial.
 - Treat each sub-skill in the worklist as its own pass: read the sub-skill's instructions, apply its Source → Relevance → Worklist → Action steps to the orchestrator-supplied inputs, and produce that sub-skill's complete findings-report independently.
 - Do not collapse multiple sub-skills into one shared reasoning step. Each sub-skill has a distinct knowledge subset and a distinct evaluation procedure; sharing one rolled-up scan dilutes per-skill attention and causes leaves to silently underreport (this has been observed in production: leaf skills returned empty `findings[]` while their standalone runs against the same diff produced multiple matches).
@@ -108,7 +108,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene
 For each sub-skill in the worklist:
 
 1. Invoke the sub-skill with the orchestrator's inputs, passing only the subset each sub-skill declares in its `inputs`.
-2. Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log before deriving a candidate. Apply only DO's bounded pre-gate normalization: when the complete raw report has no other defect, a finding has positive-integer `line`, `start-line`, and `end-line`, `start-line <= line <= end-line`, `start-line != line`, and no `suggested-code` field, copy the complete report and remove only that finding's optional `location.range`. Record the normalization separately in private run telemetry or artifacts, never in the findings-report. Validate the entire candidate through DO's existing strict acceptance gate. Accept the exact return when unchanged or the normalized candidate when it passes; otherwise record a separate failed validation result with no findings for rollup. Do not reconstruct JSON, infer fields, alter paths or references, clamp lines, normalize reversed or out-of-bounds ranges, remove a range associated with `suggested-code`, or salvage individual findings.
+2. Capture the exact Task return as the immutable raw audit payload and primary transport. Preserve it unchanged in the leaf's private artifacts or host log before deriving a candidate. Apply only DO's bounded citation-ID and optional-range normalization procedure: strict JSON and the complete structural schema first, all citation safe-path/existence/full-body retrieval checks, then a deep copy with only exact primary-reference ID replacement and eligible range removal. Preserve agent IDs, all original source-bound checks, and immutable accepted nested reports. Record changed IDs and removed ranges privately by finding index, never in the findings-report. Validate the entire candidate through DO's full schema/semantic acceptance gate. Accept the exact return when unchanged or the normalized candidate when it passes; otherwise record a separate failed validation result with no findings for rollup. Do not reconstruct JSON, infer fields, alter paths or references, clamp lines, normalize reversed or out-of-bounds ranges, remove a range associated with `suggested-code`, or salvage individual findings.
 3. Append the accepted findings-report, or the separate failed validation result, to `sub-results`. If its `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 `<from-sub-skill>:` to prevent collisions across sub-skills. Other finding fields are preserved.

schemas/findings-report.schema.json

@@ -122,7 +122,7 @@
       "additionalProperties": false,
       "required": ["id", "severity", "message", "references", "confidence"],
       "properties": {
-        "id": { "type": "string", "minLength": 1, "pattern": "^[^#]+$" },
+        "id": { "type": "string", "minLength": 1 },
         "severity": { "enum": ["blocker", "major", "minor", "info"] },
         "message": { "type": "string", "minLength": 1 },
         "location": { "$ref": "#/definitions/location" },
@@ -143,6 +143,7 @@
       },
       "then": {
         "properties": {
+          "id": { "pattern": "^(?:[a-z0-9]+(?:-[a-z0-9]+)*:)?agent:[a-z0-9]+(?:-[a-z0-9]+)*$" },
           "confidence": { "enum": ["medium", "low"] },
           "severity": { "enum": ["minor", "info"] }
         }

skills/do.md

@@ -182,8 +182,10 @@ Before emitting each leaf report or super-skill rollup:
    `outcome-reason`, not unverified locations.
 
 Validate the complete document against the schema and semantic rules before
-returning it. Consumers MUST NOT strip ID suffixes, downgrade agent findings,
-or clamp locations to make an invalid report pass the acceptance gate.
+returning it. Consumers MUST NOT heuristically strip ID suffixes, downgrade
+agent findings, or clamp locations to make an invalid report pass the acceptance gate. The
+only ID exception is the exact primary-reference copy in the bounded consumer
+procedure below; producers still MUST emit canonical IDs.
 
 ### Consumer acceptance gate
 
@@ -193,36 +195,62 @@ creating any derived value. The accepted findings-report is either that exact
 return or the bounded normalized candidate described below; the raw audit
 payload never changes.
 
-Before the full acceptance gate, a coordinator MAY create a normalized
+Before the full acceptance gate, a coordinator or host MAY create a normalized
 candidate copy only through this deterministic procedure:
 
-1. Parse the exact return as strict JSON and provisionally check the complete
-   report without mutating it. Every acceptance rule below MUST already pass
-   except for one or more findings whose optional `location.range` has
-   `start-line != line`.
-2. Each such finding is eligible only when `location.line`,
-   `location.range.start-line`, and `location.range.end-line` are positive
-   integers, `start-line <= line <= end-line`, and the finding does not contain
-   the `suggested-code` field. Field presence disqualifies normalization even
-   if its value is empty because suggested code may be bound to the reported
-   range.
-3. Deep-copy the complete parsed report. In the candidate copy, remove only
-   `location.range` from every eligible finding. Retain `location.line` and
-   every other value unchanged. Do not add normalization metadata to the
-   findings-report.
-4. Record each removed range separately in private run telemetry or artifacts,
-   associated with the immutable raw audit payload. This record is
-   runner-owned and is not part of the declared report schema.
+1. Validate the exact return as strict JSON and against the complete structural
+   schema before forming a candidate. All required fields, types, enums,
+   conditional requirements, non-empty strings, positive-integer locations,
+   range shape, and additional-property restrictions MUST pass, including
+   nested reports. Do not repair JSON or coerce values. The structural schema
+   deliberately requires only a non-empty string for a cited ID: primary-path
+   equality (including rejecting fragments or scenario suffixes) is semantic,
+   not a global no-`#` pattern that would prevent this bounded procedure.
+   For `references: []`, the schema still enforces agent slug syntax and
+   severity/confidence caps; exact role/producer ownership is semantic.
+2. Verify every cited path through all existing safe-path, live-snapshot
+   existence, and recorded full-body retrieval checks, including every
+   supporting citation. Reject absolute paths, backslashes, dot segments,
+   empty segments, URI escapes, fragments, query strings, and control
+   characters; do not resolve or rewrite them into valid paths. Unknown or
+   unretrieved references disqualify the complete report. An ID with an
+   `(^|:)agent:` marker or `from-sub-skill: "agent"` is an agent encoding,
+   never eligible for ID canonicalization; combining it with citations is
+   invalid.
+3. Deep-copy the complete parsed report. Only for a knowledge-backed finding
+   with non-empty, fully verified `references`, set the candidate `id` exactly
+   to `references[0].path` when it differs (ordinal comparison). Never derive
+   an ID by trimming or parsing the raw ID. Already-canonical IDs are no-ops.
+   Provisionally validate the entire candidate, permitting only optional
+   `location.range` start mismatches. All original range endpoints MUST still
+   be checked against the source snapshot before removing any range.
+4. In the same candidate copy, remove only an eligible `location.range`.
+   Eligibility requires `location.line`, `location.range.start-line`, and
+   `location.range.end-line` to be positive integers,
+   `start-line <= line <= end-line`, `start-line != line`, and the finding
+   does not contain the `suggested-code` field. Field presence disqualifies
+   range removal even if its value is empty. Valid uncited agent findings
+   remain eligible for this range-only operation; their IDs and caps never
+   change. Both operations apply only to the report's own `findings[]`:
+   accepted nested leaf reports are immutable, not re-normalized by rollup.
+   Record each changed ID's zero-based finding index, original ID, and
+   canonical ID, and each removed range with its finding index and original
+   endpoints, separately in private run telemetry or artifacts associated
+   with the exact raw payload. Do not add metadata to the findings-report.
 5. Validate the entire normalized candidate with the existing full consumer
-   acceptance gate below. Only a candidate that passes every rule becomes the
-   accepted copy used for rollup. If any other validation defect exists, or
-   full validation fails, discard the candidate, preserve the raw payload, and
-   fail the complete leaf as before.
+   acceptance gate below, including the complete schema, reference equality,
+   role, source, counts, coverage, and composition checks without exceptions.
+   Only a candidate that passes every rule becomes the accepted copy used for
+   rollup. If any other validation defect exists, or full validation fails,
+   discard the candidate, preserve the raw payload, and fail the complete
+   report as before. Never accept or return a partially normalized subset.
 
 This exception does not infer missing fields, alter references or paths, clamp
 line numbers, repair JSON, normalize a reversed or out-of-bounds range, remove
 a range from a finding containing `suggested-code`, or salvage arbitrary
 individual findings.
+It never canonicalizes agent/uncited IDs or changes messages, reference paths
+or order, `location.line`, severity, confidence, or any other field.
 
 Before accepting either the exact return or an eligible normalized candidate
 as a findings-report, a coordinator or host MUST validate it deterministically:
@@ -275,7 +303,8 @@ returned-and-skipped leaf IDs are invalid even without the artifact. Never
 derive the expected composition from model output.
 Pass
 `-AllowBoundedNormalization` only when the host preserves the immutable raw
-payload and records `removedRanges` in private telemetry as required above.
+payload and records `normalizedIds` and `removedRanges` in private telemetry as
+required above. These fields are returned beside `report`, never inside it.
 
 Validation failure invalidates the complete return; consumers MUST NOT salvage
 individual findings, infer missing fields, reconstruct JSON, clamp ranges,