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

Revert branch's suggested-code scoping addition in al-code-review

Browse source 
The three location-span shapes added to al-code-review are output-format mechanics, not domain knowledge: one-click span correctness is the engine's job (Resolve-SuggestionPlacement) and do.md already owns the suggested-code/location contract. The AL concerns the examples illustrate are already covered by existing knowledge (use-isempty-for-existence-check, data-classification-required-on-pii-fields, no-space-before-method-parenthesis). Restores al-code-review to main; the branch now adds only the 6 knowledge files.
This commit is contained in:
wenjiefan 2026-07-17 10:18:02 +02:00
parent 9cc5627f42
commit b9f59c7f09
1 changed files with 0 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

6
microsoft/skills/review/al-code-review.md
View file

@ -112,12 +112,6 @@ Leaf-level agent findings (those with `references: []` inside a sub-skill's repo
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. This is a MUST for small, local, mechanical fixes. 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, adding a missing property, replacing string-concatenated `Error`, changing an over-broad permission token, fixing whitespace or keyword casing. Skip `suggested-code` only when the fix requires choosing between multiple defensible alternatives, when the fix spans non-contiguous code, or when the surrounding context the agent cannot see could change the answer. If a mechanical-looking finding omits `suggested-code`, set `suggested-code-omission-reason`. 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. This is a MUST for small, local, mechanical fixes. 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, adding a missing property, replacing string-concatenated `Error`, changing an over-broad permission token, fixing whitespace or keyword casing. Skip `suggested-code` only when the fix requires choosing between multiple defensible alternatives, when the fix spans non-contiguous code, or when the surrounding context the agent cannot see could change the answer. If a mechanical-looking finding omits `suggested-code`, set `suggested-code-omission-reason`.
Scope `location` and `suggested-code` to exactly the lines that change — no wider, no narrower. `location` MUST cover every original line the replacement rewrites and MUST NOT extend to surrounding lines that stay the same; the delivery surface replaces precisely the lines `location` names with `suggested-code`, so a mismatch lands the fix on the wrong lines. Three shapes recur:
- **Adding a property to an existing member** (for example a missing `DataClassification` on a field): emit the new line together with just one adjacent unchanged line that brackets it — the preceding property or the member's closing `}` — and set `location` to that minimal two-line span. Do not restate the entire field, key, or procedure; reproducing the whole block makes the one-click suggestion replace everything instead of inserting the single line.
- **Rewriting one statement** (for example `exit (X)` → `exit(X)`): point `location.line` at that statement's line, not at the enclosing procedure signature or `begin`/`end`.
- **Collapsing several source lines into fewer** (for example replacing a one-line `Count() > 0` guard and a follow-up existence check on the next line with a single `not IsEmpty()` line): the one-click mechanism replaces only the lines it can match, so a shorter `suggested-code` cannot express the removal of the extra original line and would land on just one of them. Omit `suggested-code`, set `suggested-code-omission-reason` (for example `fix removes lines; manual edit needed`), and describe the collapsed result in `message`.
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. 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 ### Summary and rollup