diff --git a/microsoft/skills/review/al-breaking-changes-review.md b/microsoft/skills/review/al-breaking-changes-review.md index 82aeda0..f76bdd4 100644 --- a/microsoft/skills/review/al-breaking-changes-review.md +++ b/microsoft/skills/review/al-breaking-changes-review.md @@ -61,6 +61,8 @@ When the post-conflict worklist is empty because no applicable breaking-changes ## Action +**Released-baseline precondition.** Breaking-change rules protect contracts that have already shipped to customers or are exposed to external extensions. Before emitting any finding — knowledge-backed or agent — establish that the affected object, field, key, enum value, or procedure was present in a released baseline. A symbol that is new in this app, was introduced and then changed within the same still-unreleased development cycle, or belongs to an app that has no released version yet can be renamed, renumbered, or removed freely and is not a breaking change. When release status cannot be established from the diff, `app.json`, or a released baseline, do not assert a break; omit the finding rather than flag it. + For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: - When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 879e788..138c876 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -52,6 +52,8 @@ When the post-conflict worklist is empty because no applicable upgrade knowledge ## Action +**Released-baseline precondition.** Upgrade and migration findings protect data and schema that have already shipped to customers. Before emitting any finding — knowledge-backed or agent — establish that the affected table, field, key, or enum existed in a released version. A schema element that is new in this app, or was added and then changed within the same still-unreleased development cycle, needs no upgrade code or migration path and is not an obsoletion, data-loss, or breaking-migration defect. When release status cannot be established from the diff, `app.json`, or a released baseline, do not assert an upgrade defect; omit the finding rather than flag it. + For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows: - When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` for irreversible data corruption (enum-ordinal shift, unguarded reads that abort the upgrade) and for changes that would ship to customers without a migration path (new InitValue on an existing table without upgrade code). diff --git a/skills/do.md b/skills/do.md index 9de11a4..75ba4d0 100644 --- a/skills/do.md +++ b/skills/do.md @@ -158,6 +158,13 @@ When a super-skill rolls up a non-citation finding from a sub-skill (an `id` tha - **Never emit** as agent findings: stylistic or formatting preferences (outside a dedicated style skill's own domain); speculative or hypothetical concerns — anything you would phrase with "could", "might", or "consider"; issues that depend on code not visible in the diff; valid alternative approaches; or generic software-engineering advice a competent model already applies without prompting (the same exclusion the knowledge-file admission test enforces). - **When in doubt, omit.** Recall is the knowledge files' responsibility; the agent channel exists only for the high-confidence, concrete defect the corpus has not captured yet. A missed low-severity observation is cheaper than a false positive. +**Known false-positive patterns — never emit these.** Recurring agent findings that BC reviewers consistently reject. Each depends on context the diff does not show or on an incorrect model of AL semantics; do not emit them as agent findings from any leaf or super-skill: + +- **Missing `ToolTip` on a bound page field.** A page field bound to a table field inherits that field's `ToolTip`; the genuinely-missing case is already covered by analyzer AA0218 (calibrated to `info`). Do not raise a missing-`ToolTip` agent finding for a field with a source-table binding. +- **Page record trigger "missing `exit(true)` blocks the operation".** The Boolean page triggers `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, and `OnQueryClosePage` return `true` by default; omitting an explicit return value lets the operation proceed. Only an explicit `exit(false)` (or a path that returns `false`) cancels it. Do not claim a missing `exit(true)` prevents inserts, modifies, or deletes. +- **"Drill-down / lookup / list is unfiltered".** The effective filter is frequently defined outside the changed hunk — on the table via `SourceTableView`, a `TableRelation`, or `SetRange`/`SetFilter` in unchanged code. Absence of a filter in the diff is not evidence that none applies; do not assert an unfiltered result set. +- **`CalcFields` in both `OnAfterGetRecord` and `OnAfterGetCurrRecord`.** These triggers fire at different points in the page lifecycle (per row as records load vs. when a record becomes current) and serve different purposes; the pair is not duplicate or redundant work. + Agent findings may be emitted by both leaf sub-skills and super-skills, with different scope boundaries: - A **leaf sub-skill** MAY emit agent findings strictly within its declared `domain`. al-security-review MAY surface an agent security finding that no knowledge file covers (for example, a `case` over a security-relevant enum with no `else` arm), but MUST NOT emit a style or performance agent finding — those are out of scope for the leaf and belong to other leaves or to the super-skill. The leaf's domain is the bounding box.