Microsoft/bcquality
1
0
Fork 0
mirror of https://github.com/microsoft/BCQuality.git synced 2026-08-06 09:26:52 +01:00
Code Issues Projects Releases Packages Wiki Activity Actions
bcquality/microsoft/skills/review/al-code-review.md
Jesper Schulz-Wedde 3a07ee82d2 Allow leaf sub-skills to emit agent findings within their domain 
The original DO contract pinned all agent reasoning to the super-skill:
'Agent findings are emitted only by super-skills... Leaf sub-skills MUST
NOT emit agent findings'. This funnels all agent reasoning across all 6
domains through a single super-skill pass, which is the root structural
cause of the attention dilution we have been chasing in BCAppsBCQuality
PRs #28 and #30:

- T1 standalone run showed al-security-review finds rimd-on-read-only
  cleanly when run alone, but emits zero agent findings because the
  contract forbids it. So obvious things like case-without-else (no
  matching KB article yet) get dropped on the floor.
- The al-code-review self-review pass keeps producing 0-1 agent findings
  per PR because it is asked to reason across 6 domains in one pass.

The fix is to move agent reasoning into the leaves, bounded by each
leaf's domain. Each leaf now has both knowledge-backed and agent-finding
permissions within its own scope; the super-skill self-review pass
becomes a smaller, cross-cutting role.

skills/do.md
  - Replace the 'only by super-skills' / 'MUST NOT' clause with a
    two-tier model: leaf sub-skills MAY emit agent findings strictly
    within their declared domain; super-skills MAY emit agent findings
    for cross-cutting concerns that span domains.
  - Update the encoding rules: leaf agent findings have references:[]
    and an agent:-prefixed id, no from-sub-skill (the leaf's own report
    carries the finding under its own skill.id). Super-skills set
    from-sub-skill='agent' for their own self-review findings; when
    rolling up leaf agent findings, they set from-sub-skill=<leaf-id>.
  - Clarify that 'MUST validate against knowledge' applies to super-
    skill self-review candidates only - leaves already validated within
    their domain when they decided to emit.

microsoft/skills/review/al-{security,performance,privacy,style,upgrade,
                                                            ui}-review.md
  - New paragraph after the confidence rules instructing each leaf to
    surface domain-specific agent findings when no knowledge file
    covers a defect the agent recognises from general AL knowledge.
  - Bound the scope: 'The scope is strictly <domain>; defects outside
    this domain belong to other leaves and MUST NOT be emitted here.'
  - Same validation requirement: check the worklist for a matching
    knowledge file first; if one exists, upgrade to a knowledge-backed
    finding instead.

microsoft/skills/review/al-code-review.md
  - Rewrite the 'Agent self-review pass' subsection. The pass is now
    explicitly for cross-cutting concerns that no single leaf could
    have surfaced because they span multiple domains. Domain-specific
    reasoning belongs in the leaves, not duplicated here.
  - Update the rollup behavior to acknowledge leaf-emitted agent
    findings: they are rolled up like any other sub-skill finding, with
    from-sub-skill set to the leaf id, and are not re-validated by the
    super-skill (the leaf already validated within its own domain).
  - Drop the 'Leaf sub-skills MUST NOT emit agent findings' line.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-05-28 11:42:38 +02:00

315 lines
18 KiB
Markdown
Raw Blame History

---
kind: action-skill
id: al-code-review
version: 1
title: AL code review
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI).
inputs: [pr-diff, file-path]
outputs: [findings-report]
bc-version: [all]
technologies: [al]
countries: [w1]
application-area: [all]
sub-skills:
- microsoft/skills/review/al-performance-review.md
- microsoft/skills/review/al-security-review.md
- microsoft/skills/review/al-privacy-review.md
- microsoft/skills/review/al-upgrade-review.md
- microsoft/skills/review/al-style-review.md
- microsoft/skills/review/al-ui-review.md
---
# AL code review
Reviews AL source changes by composing the leaf AL review skills. This is the canonical reference implementation of a **super-skill** — skill authors writing composed reviews should copy its structure.
`al-code-review` does not evaluate knowledge files directly. It invokes each of its sub-skills against the same task input, collects their findings-reports, and then performs its own **self-review pass** over the diff using the agent's built-in BC and AL knowledge. BCQuality knowledge is an additive layer: anything the sub-skills found is cited from BCQuality, and anything the agent finds on its own is validated against BCQuality (cited if matched, suppressed if contradicted, surfaced as an **agent finding** otherwise). The result is a single rolled-up findings-report that mixes knowledge-backed and agent findings, each clearly tagged via `from-sub-skill`.
An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract, extended with `sub-results` and — when applicable — `skipped-sub-skills`.
## Source
The sub-skills invoked by this skill are those listed in frontmatter `sub-skills`:
- `microsoft/skills/review/al-performance-review.md`
- `microsoft/skills/review/al-security-review.md`
- `microsoft/skills/review/al-privacy-review.md`
- `microsoft/skills/review/al-upgrade-review.md`
- `microsoft/skills/review/al-style-review.md`
- `microsoft/skills/review/al-ui-review.md`
Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.
## Relevance
A sub-skill is relevant when both of the following hold:
- The orchestrator has supplied inputs that satisfy the sub-skill's declared `inputs`.
- The orchestrator has not disabled the sub-skill via configuration.
Per the DO contract, the super-skill MUST NOT filter sub-skills by task content. `al-code-review` does not inspect the PR diff to predict whether, for example, there is anything for `al-security-review` to find. Each leaf is responsible for its own task-level applicability decision; leaves signal non-applicability by returning `outcome: "not-applicable"` or `outcome: "no-knowledge"`.
Sub-skills that fail either check are not invoked and are recorded in `skipped-sub-skills`:
- `reason: "configuration"` when the orchestrator disabled the sub-skill.
- `reason: "not-applicable"` when the orchestrator's inputs do not satisfy the sub-skill's declared `inputs`.
## Worklist
The worklist is the list of sub-skills judged relevant by the previous step. Every sub-skill in the worklist will be invoked in the Action step.
## Action
### Execution discipline (mandatory)
The Action step is a sequence of **discrete iterations**, not one combined generation. The contract requires the super-skill to invoke each sub-skill in turn and then perform a self-review pass. Concretely this means:
- 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 before moving on.
- 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).
- The agent self-review pass is its own final iteration. Begin it only after every sub-skill in the worklist has completed and its sub-result is recorded.
- Sub-skills are independent: re-walking the diff once per sub-skill is correct and expected. The output schema accommodates this — `sub-results` carries one entry per sub-skill, each a complete findings-report.
### Roll up sub-skill findings
For each sub-skill in the worklist, executed one at a time per the discipline above:
1. Invoke the sub-skill with the orchestrator's inputs, passing only the subset each sub-skill declares in its `inputs`.
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, append each entry from the sub-skill's `findings[]` to the super-skill's top-level `findings[]`, setting `from-sub-skill` to the sub-skill's `skill.id`. 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.
### Agent self-review pass
Each leaf sub-skill emits both knowledge-backed findings and, per its own contract, agent findings within its own domain. After every sub-skill has produced its sub-result, perform a super-skill self-review pass against the same task input. The goal of this pass is to surface defects the agent recognises on its own that **no single leaf could have surfaced** because they cross domain boundaries — architecture-level issues that touch performance and reliability at once, error-handling gaps that span security and UX, resource-lifecycle patterns that affect both correctness and performance, and similar cross-cutting concerns. BCQuality is an **additive** knowledge layer: it augments the agent's review judgement, it does not replace it.
This pass is mandatory on real-size PRs. An empty agent-findings list at the super-skill level is acceptable only when the diff is small enough that nothing cross-cutting plausibly applies (in practice: PRs of ≤2 files with ≤30 changed lines). For larger diffs, the fact that leaves emitted findings does not exempt the super-skill — the leaves handle within-domain reasoning, the super-skill handles cross-domain reasoning, and a real-size PR with no cross-cutting candidates is rare.
Frame the pass by cross-cutting concerns — architecture, error handling, resource lifecycle — and by the seams between leaf domains. Do not duplicate domain-specific reasoning that belongs in a leaf: a security-only concern is the security leaf's responsibility, not the super-skill's. The super-skill pass adds value where no individual leaf has the right scope.
For every candidate the agent identifies in this pass:
1. **Validate against BCQuality knowledge.** Check the candidate against the knowledge files the sub-skills have already loaded for this task (visible via their `references` and `suppressed` lists in `sub-results`).
- If a BCQuality knowledge file matches the candidate, upgrade it to a knowledge-backed finding: cite the file in `references`, set `id` to the file's path, set `from-sub-skill` to the sub-skill that owns that knowledge domain, and merge with or deduplicate against any sub-skill finding that already covers the same concern at the same location.
- If a BCQuality knowledge file **explicitly contradicts** the candidate (its `## Best Practice` or `## Anti Pattern` says the opposite of what the agent flagged), suppress the candidate and do not surface it.
- Otherwise the candidate has no BCQuality coverage; emit it as a super-skill agent finding.
2. **Emit agent finding.** Per DO's *Agent findings* rules:
- `from-sub-skill: "agent"` (the super-skill itself produced it)
- `references: []`
- `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. Omit it when the appropriate fix depends on context the agent cannot determine.
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: <leaf-id>` 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.
### 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).
`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`.
Derive `outcome` using the DO rollup rules. `outcome-reason` is populated for `partial` and `failed` and SHOULD summarize per-sub-skill state, for example: *"al-security-review failed (tool timeout); al-performance-review completed."*
## 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:
```json
{
"skill": { "id": "al-code-review", "version": 1 },
"outcome": "completed",
"summary": {
"counts": { "blocker": 1, "major": 1, "minor": 3, "info": 0 },
"coverage": { "worklist-size": 4, "items-evaluated": 4 }
},
"findings": [
{
"id": "microsoft/knowledge/performance/filter-before-find.md",
"severity": "major",
"message": "FindSet is called on a record variable without any prior SetRange/SetFilter. This forces a full-table scan.",
"location": {
"file": "src/Sales/PostingRoutines.Codeunit.al",
"line": 140,
"range": { "start-line": 140, "end-line": 144 }
},
"references": [
{ "path": "microsoft/knowledge/performance/filter-before-find.md" }
],
"confidence": "high",
"from-sub-skill": "al-performance-review"
},
{
"id": "community/knowledge/performance/call-setloadfields-before-filters.md",
"severity": "minor",
"message": "SetLoadFields is called after SetRange. Per the referenced guidance the call must come before filters to be folded into the query plan.",
"location": {
"file": "src/Sales/PostingRoutines.Codeunit.al",
"line": 152
},
"references": [
{ "path": "community/knowledge/performance/call-setloadfields-before-filters.md" }
],
"confidence": "high",
"from-sub-skill": "al-performance-review"
},
{
"id": "microsoft/knowledge/security/use-secrettext-for-credentials.md",
"severity": "blocker",
"message": "A bearer token is declared as a Text parameter and passed through the HTTP request path as plain text. The referenced guidance requires credentials to flow as SecretText end-to-end.",
"location": {
"file": "src/Integration/ApiClient.Codeunit.al",
"line": 85,
"range": { "start-line": 85, "end-line": 89 }
},
"references": [
{ "path": "microsoft/knowledge/security/use-secrettext-for-credentials.md" }
],
"confidence": "high",
"from-sub-skill": "al-security-review"
},
{
"id": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md",
"severity": "minor",
"message": "An API key is assigned from a string literal rather than retrieved from IsolatedStorage or Key Vault at runtime.",
"location": {
"file": "src/Integration/ApiClient.Codeunit.al",
"line": 201
},
"references": [
{ "path": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md" }
],
"confidence": "medium",
"from-sub-skill": "al-security-review"
},
{
"id": "agent:missing-error-handling-on-http-client",
"severity": "minor",
"message": "HttpClient.Send is called without inspecting the response status or wrapping the call in a TryFunction. Network or remote-server failures will surface as runtime errors to the user. Recommendation: branch on the HttpResponseMessage.IsSuccessStatusCode and either retry, surface a controlled error, or fall back, depending on the integration's contract.",
"location": {
"file": "src/Integration/ApiClient.Codeunit.al",
"line": 60,
"range": { "start-line": 60, "end-line": 64 }
},
"references": [],
"confidence": "medium",
"from-sub-skill": "agent"
}
],
"suppressed": [],
"sub-results": [
{
"skill": { "id": "al-performance-review", "version": 1 },
"outcome": "completed",
"summary": {
"counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
"coverage": { "worklist-size": 2, "items-evaluated": 2 }
},
"findings": [
{
"id": "microsoft/knowledge/performance/filter-before-find.md",
"severity": "major",
"message": "FindSet is called on a record variable without any prior SetRange/SetFilter. This forces a full-table scan.",
"location": {
"file": "src/Sales/PostingRoutines.Codeunit.al",
"line": 140,
"range": { "start-line": 140, "end-line": 144 }
},
"references": [
{ "path": "microsoft/knowledge/performance/filter-before-find.md" }
],
"confidence": "high"
},
{
"id": "community/knowledge/performance/call-setloadfields-before-filters.md",
"severity": "minor",
"message": "SetLoadFields is called after SetRange. Per the referenced guidance the call must come before filters to be folded into the query plan.",
"location": {
"file": "src/Sales/PostingRoutines.Codeunit.al",
"line": 152
},
"references": [
{ "path": "community/knowledge/performance/call-setloadfields-before-filters.md" }
],
"confidence": "high"
}
],
"suppressed": []
},
{
"skill": { "id": "al-security-review", "version": 1 },
"outcome": "completed",
"summary": {
"counts": { "blocker": 1, "major": 0, "minor": 1, "info": 0 },
"coverage": { "worklist-size": 2, "items-evaluated": 2 }
},
"findings": [
{
"id": "microsoft/knowledge/security/use-secrettext-for-credentials.md",
"severity": "blocker",
"message": "A bearer token is declared as a Text parameter and passed through the HTTP request path as plain text. The referenced guidance requires credentials to flow as SecretText end-to-end.",
"location": {
"file": "src/Integration/ApiClient.Codeunit.al",
"line": 85,
"range": { "start-line": 85, "end-line": 89 }
},
"references": [
{ "path": "microsoft/knowledge/security/use-secrettext-for-credentials.md" }
],
"confidence": "high"
},
{
"id": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md",
"severity": "minor",
"message": "An API key is assigned from a string literal rather than retrieved from IsolatedStorage or Key Vault at runtime.",
"location": {
"file": "src/Integration/ApiClient.Codeunit.al",
"line": 201
},
"references": [
{ "path": "microsoft/knowledge/security/never-hardcode-secrets-in-al.md" }
],
"confidence": "medium"
}
],
"suppressed": []
}
]
}
```
The empty-corpus case — BCQuality's state until knowledge files land — rolls up to `no-knowledge`:
```json
{
"skill": { "id": "al-code-review", "version": 1 },
"outcome": "no-knowledge",
"summary": {
"counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 },
"coverage": { "worklist-size": 0, "items-evaluated": 0 }
},
"findings": [],
"suppressed": [],
"sub-results": [
{
"skill": { "id": "al-performance-review", "version": 1 },
"outcome": "no-knowledge",
"summary": { "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, "coverage": { "worklist-size": 0, "items-evaluated": 0 } },
"findings": [],
"suppressed": []
},
{
"skill": { "id": "al-security-review", "version": 1 },
"outcome": "no-knowledge",
"summary": { "counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 }, "coverage": { "worklist-size": 0, "items-evaluated": 0 } },
"findings": [],
"suppressed": []
}
]
}
```
Reference in a new issue View git blame Copy permalink