mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-09 02:41:37 +01:00
3a07ee82d2
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>
103 lines
6.8 KiB
Markdown
103 lines
6.8 KiB
Markdown
---
|
|
kind: action-skill
|
|
id: al-style-review
|
|
version: 1
|
|
title: AL style review
|
|
description: Reviews AL source changes against naming, labelling, and code-convention guidance from BCQuality.
|
|
inputs: [pr-diff, file-path]
|
|
outputs: [findings-report]
|
|
bc-version: [all]
|
|
technologies: [al]
|
|
countries: [w1]
|
|
application-area: [all]
|
|
---
|
|
|
|
# AL style review
|
|
|
|
Reviews AL source changes against the `style` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
|
|
|
|
Style findings cover AL conventions that CodeCop and similar analyzers partially enforce — label suffixes, API page naming, temporary-variable prefixes, label properties, named invocations, `FieldCaption`/`TableCaption` in user messages, `OptionCaption` pairing, Error-parameter passing, `this` keyword, required parentheses, file-naming. Use together with a formal analyzer; this skill adds BCQuality's remedial-knowledge explanations of why each rule exists.
|
|
|
|
An orchestrator invokes this skill with either a `pr-diff` or a `file-path`. The skill produces a single JSON document conforming to the DO output contract.
|
|
|
|
## Source
|
|
|
|
Collect all knowledge files under `*/knowledge/style/**/*.md`, across every enabled layer.
|
|
|
|
## Relevance
|
|
|
|
Apply the frontmatter matching rules defined in READ against the task context:
|
|
|
|
- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
|
|
- `technologies` — `[al]`.
|
|
- `countries` — the countries declared in the consuming app's `app.json`. If absent, `unknown`.
|
|
- `application-area` — pass the actual set declared by the changed objects; do not substitute `[all]`.
|
|
|
|
Discard files that are not applicable. Retain conditionally applicable files only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium` and MUST name the unknown dimensions in `message`.
|
|
|
|
## Worklist
|
|
|
|
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
|
|
|
- Changed AL objects — especially API pages (`PageType = API`), tables and pages declaring Labels/TextConsts, codeunits issuing `Error`/`Message`/`Confirm`, and any file whose name violates the `<ObjectName>.<ObjectType>.al` convention.
|
|
- Changed declarations, weighted toward `: Label '...'`, `: TextConst '...'`, temporary record variables, option fields, error-handling call sites, and codeunit-internal method calls.
|
|
- Tokens extracted from the diff (`Label`, `TextConst`, `Locked`, `Comment`, `MaxLength`, `temporary`, `OptionMembers`, `OptionCaption`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `DelayedInsert`, `FieldCaption`, `TableCaption`, `FieldName`, `TableName`, `Page.RunModal`, `Report.Run`, `this.`, `StrSubstNo`).
|
|
|
|
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic matches a changed object or declaration.
|
|
|
|
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
|
|
|
When the post-conflict worklist is empty because no applicable style knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable style knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
|
|
|
## Action
|
|
|
|
For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`.
|
|
|
|
Set `confidence` to:
|
|
|
|
- `high` when the detection is based on an unambiguous pattern match.
|
|
- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
|
|
- `low` when the finding is an advisory derived only from applicability.
|
|
|
|
After evaluating each worklist entry, also consider whether the diff exhibits a style defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). The scope is strictly style; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract.
|
|
|
|
When the knowledge file ships an unambiguous `.good.al` companion that names exactly the correction the finding requires (and the diff context makes the substitution mechanical), set `findings[].suggested-code` to the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. Skip the field when the appropriate fix depends on context the skill cannot determine, or when more than one defensible replacement exists. See `skills/do.md` for the full contract.
|
|
|
|
Outcome selection:
|
|
|
|
- `completed` — the skill evaluated every worklist item.
|
|
- `no-knowledge` — no applicable style knowledge survived filtering.
|
|
- `not-applicable` — no AL changes in the diff.
|
|
- `partial` — a budget was hit before the worklist was exhausted.
|
|
- `failed` — an unrecoverable error occurred.
|
|
|
|
## Output
|
|
|
|
Output conforms to the DO output contract. A populated example:
|
|
|
|
```json
|
|
{
|
|
"skill": { "id": "al-style-review", "version": 1 },
|
|
"outcome": "completed",
|
|
"summary": {
|
|
"counts": { "blocker": 0, "major": 0, "minor": 1, "info": 0 },
|
|
"coverage": { "worklist-size": 1, "items-evaluated": 1 }
|
|
},
|
|
"findings": [
|
|
{
|
|
"id": "microsoft/knowledge/style/apply-approved-label-suffixes.md",
|
|
"severity": "minor",
|
|
"message": "A Label named Text000 has no approved suffix (Msg/Err/Qst/Tok/Lbl/Txt). Per the referenced CodeCop AA0074 guidance, every Label and TextConst carries a suffix indicating its consuming call.",
|
|
"location": {
|
|
"file": "src/Sales/PostingRoutines.Codeunit.al",
|
|
"line": 42
|
|
},
|
|
"references": [
|
|
{ "path": "microsoft/knowledge/style/apply-approved-label-suffixes.md" }
|
|
],
|
|
"confidence": "high"
|
|
}
|
|
],
|
|
"suppressed": []
|
|
}
|
|
```
|