Improve PR-review parity: suggestion blocks, missing KB articles, privacy cross-ref

Adds the contract field, skill instructions, and two knowledge articles
that BCAppsBCQuality's PR-review agent needs to match (and exceed) the
coverage of the embedded review agent in BCAppsCampAIRHack:

skills/do.md
- New optional findings[].suggested-code field. Documents what it MUST
  contain (a literal line-replacement payload) and when to emit it.

microsoft/skills/review/al-code-review.md
- Instructs both the agent self-review pass and rolled-up sub-skill
  findings to populate suggested-code when the fix is mechanical.
- Lists examples (dead code removal, Count > 0 -> IsEmpty, object-scope
  Label) that map to issues observed in the parity comparison.

microsoft/knowledge/style/telemetry-event-id-stable-unique.{md,bad.al,good.al}
- New knowledge article: telemetry event IDs must be stable, unique,
  and non-placeholder. Closes a gap surfaced by the parity comparison.

microsoft/knowledge/style/labels-declared-at-object-scope.{md,bad.al,good.al}
- New knowledge article: Labels must live in the object-level var
  block, not in procedure-local var blocks. Closes the second gap.

microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md
- Adds an explicit note that changing DataClassification alone does not
  make embedding PII into the message string acceptable, plus links to
  the two adjacent privacy articles. Resolves the privacy advice the
  parity comparison flagged as ambiguous.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde 2026-05-27 15:45:49 +02:00
parent 4d59fb73bc
commit b11f3ec506
9 changed files with 130 additions and 1 deletions

View file

@ -85,9 +85,16 @@ For every candidate the agent identifies in this pass:
- `id` is a skill-defined slug prefixed with `agent:` (for example, `agent:missing-error-handling-on-http-call`).
- `confidence` capped at `medium`.
- `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` SHOULD be set when the fix is small and mechanical (e.g. removing a few unreachable lines, replacing a `Count() > 0` test with `not IsEmpty()`, declaring a missing `Label`). Omit it when the appropriate fix depends on context the agent cannot determine.
Leaf sub-skills MUST NOT emit agent findings: their scope is bounded by the knowledge subset they evaluate. The self-review pass is a super-skill responsibility.
### Suggested-code guidance
For both knowledge-backed findings rolled up from sub-skills and agent findings emitted in the self-review pass, populate `findings[].suggested-code` whenever a concrete code replacement is unambiguous from the diff context. The payload MUST be a literal replacement for the source lines covered by `location` (typically a single line, or the line range in `location.range`) — no diff markers, fences, or commentary. Examples of good candidates: deleting dead code after `exit`, replacing `Count() > 0` with `not IsEmpty()`, moving an inline `Label` declaration to a codeunit-level `var` block, fixing whitespace or keyword casing. Skip `suggested-code` when the fix requires choosing between multiple defensible alternatives or when the surrounding context the agent cannot see could change the answer.
Sub-skills MAY also emit `suggested-code` when their knowledge file unambiguously implies the replacement (the `.good.al` and `.bad.al` companion examples are useful here). The super-skill copies the field through unchanged.
### 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).