diff --git a/README.md b/README.md index acadd9f..6baafcf 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,7 @@ Skills define how agents consume knowledge. They come in three flavors: READ and DO are read on demand — typically when the first dispatched action skill runs. They are not prerequisites for invoking Entry. WRITE is only used when scaffolding new content. -- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes eight leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces). +- **Action skills** — concrete skills that follow the Action Skill template to do real work (review code, audit telemetry, etc.). Action skills live inside the layers that own them (`/microsoft/skills/`, `/community/skills/`, `/custom/skills/`). An action skill is either a **leaf** that evaluates knowledge files directly, or a **super-skill** that composes other action skills (declared via `sub-skills` in frontmatter). The canonical reference is [`microsoft/skills/review/al-code-review.md`](microsoft/skills/review/al-code-review.md) (super-skill), which composes ten leaf skills under [`microsoft/skills/review/`](microsoft/skills/review/) — one per knowledge domain (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces, breaking changes). ### Agent bootstrapping diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al new file mode 100644 index 0000000..b0cc156 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.bad.al @@ -0,0 +1,21 @@ +codeunit 50326 "Order Processor Bad" +{ + // Anti-pattern: every helper is public by default, exposing implementation + // detail as a de-facto API. Each becomes a contract that cannot be changed + // without risking breakage for consumers that bound to it. + procedure ProcessOrder(OrderNo: Code[20]) + begin + ValidateOrder(OrderNo); + PostOrder(OrderNo); + end; + + procedure ValidateOrder(OrderNo: Code[20]) + begin + if OrderNo = '' then + Error('Order number is required.'); + end; + + procedure PostOrder(OrderNo: Code[20]) + begin + end; +} diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al new file mode 100644 index 0000000..1b53a68 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.good.al @@ -0,0 +1,21 @@ +codeunit 50325 "Order Processor Good" +{ + // Supported, stable entry point — intentionally public. + procedure ProcessOrder(OrderNo: Code[20]) + begin + ValidateOrder(OrderNo); + PostOrder(OrderNo); + end; + + // In-app reuse only — internal, so it is not part of the external contract. + internal procedure ValidateOrder(OrderNo: Code[20]) + begin + if OrderNo = '' then + Error('Order number is required.'); + end; + + // Implementation detail confined to this object — local. + local procedure PostOrder(OrderNo: Code[20]) + begin + end; +} diff --git a/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md new file mode 100644 index 0000000..835312d --- /dev/null +++ b/microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [access-modifier, internal, local, public, protected, scope, encapsulation] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Choose access modifiers deliberately + +## Description + +Access is a decision about what you are willing to support forever. The moment a procedure or object is reachable from another extension — no `local`, or a removed `[Scope('OnPrem')]` — it becomes a contract: callers bind to it, and changing or removing it later is a breaking change. The safe default is the narrowest access that works. Use `local` for implementation detail confined to one object, `internal` for code shared within the app but not exposed to consumers, and `protected var` for state intended as an inheritance point for extension objects. Reserve `public` for the deliberate, supported entry points you intend to maintain as a stable API. LLMs tend to make everything public "to be safe," which inverts the rule and turns every helper into an accidental contract. + +## Best Practice + +Start everything `local` or `internal` and promote a member to `public` only when you have decided to support it as a stable contract. Expose a small, intentional surface — the supported entry point — and keep validation, posting, and helper routines `internal` for in-app reuse or `local` when single-object. Do not drop `[Scope('OnPrem')]` without intent, since that too widens the contract. Every public member is a maintenance commitment; spend them deliberately. + +See sample: `choose-access-modifiers-deliberately.good.al`. + +## Anti Pattern + +Declaring every procedure `public` by default, so internal helpers like `ValidateOrder` and `PostOrder` become a de-facto API that consumers bind to and that can no longer be changed freely. Detection: an object where implementation-detail procedures carry no access modifier or are `public` without a reason to support them externally. Default them to `internal`/`local` and make only the intended entry point public. + +See sample: `choose-access-modifiers-deliberately.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al new file mode 100644 index 0000000..a959573 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.bad.al @@ -0,0 +1,10 @@ +codeunit 50306 "Net Amount Api Bad" +{ + // Breaking: the published CalcNet procedure was renamed outright with no + // deprecation window and no [Obsolete] marker. Every extension that called + // CalcNet breaks the instant it consumes this version. + procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(GrossAmount / (1 + TaxRate)); + end; +} diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al new file mode 100644 index 0000000..4f2638a --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.good.al @@ -0,0 +1,15 @@ +codeunit 50305 "Net Amount Api Good" +{ + // Old name kept and marked obsolete: callers still compile but get a warning + // pointing at the replacement, with a tag recording the removal target version. + [Obsolete('Use CalculateNetAmount instead.', '25.0')] + procedure CalcNet(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(CalculateNetAmount(GrossAmount, TaxRate)); + end; + + procedure CalculateNetAmount(GrossAmount: Decimal; TaxRate: Decimal): Decimal + begin + exit(GrossAmount / (1 + TaxRate)); + end; +} diff --git a/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md new file mode 100644 index 0000000..5a699b6 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/deprecate-public-members-with-the-obsolete-lifecycle.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [obsolete, deprecation, obsoletestate, obsoletetag, pending, removed, public-procedure] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Deprecate public members through the Obsolete lifecycle, never delete them outright + +## Description + +Deleting or renaming a published procedure (or object) in a single release is a hard break: dependent extensions that reference it stop compiling the moment they pick up the new version, with no warning window to migrate. AL provides a staged deprecation lifecycle precisely so consumers get advance notice. For a procedure, apply the `[Obsolete('reason', 'tag')]` attribute: the member keeps working but every caller gets a compiler warning naming the replacement and the target version. The member stays through a deprecation window — at least one major release — before it is finally removed. Object- and field-level members use the matching `ObsoleteState = Pending` → `Removed` property progression. LLMs trained to "clean up" code often delete or rename the old member immediately, skipping the window entirely. + +## Best Practice + +When a published procedure is superseded, keep it in place and mark it `[Obsolete('Use CalculateNetAmount instead.', '25.0')]`, where the message names the replacement and the tag records the target version for removal. Have the obsolete member forward to the new one so behavior is preserved during the window. Only after the deprecation window has elapsed — a later release — change its state to removed. This gives every dependent app a compile-time signal and time to migrate before anything actually disappears. + +See sample: `deprecate-public-members-with-the-obsolete-lifecycle.good.al`. + +## Anti Pattern + +Renaming or deleting the published `CalcNet` procedure in place — replacing it with `CalculateNetAmount` and nothing else — so consumers calling `CalcNet` break immediately with no deprecation notice. Detection: a previously shipped non-`local` procedure that vanished or was renamed between versions with no `[Obsolete]` marker left behind on a kept member. Mark it obsolete and keep it for a window instead. + +See sample: `deprecate-public-members-with-the-obsolete-lifecycle.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al new file mode 100644 index 0000000..6c2f7ca --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.bad.al @@ -0,0 +1,10 @@ +codeunit 50301 "Discount Api Bad" +{ + // Breaking: a Rate parameter was added to a procedure that already shipped. + // Every dependent extension that called CalculateDiscount(Amount) now fails + // to compile until it is changed and recompiled. + procedure CalculateDiscount(Amount: Decimal; Rate: Decimal): Decimal + begin + exit(Amount * Rate); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al new file mode 100644 index 0000000..4640a69 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.good.al @@ -0,0 +1,16 @@ +codeunit 50300 "Discount Api Good" +{ + // Published contract — signature kept exactly as it shipped. + procedure CalculateDiscount(Amount: Decimal): Decimal + begin + exit(Amount * 0.05); + end; + + // New capability added as a separate overload, so existing callers of + // CalculateDiscount(Amount) keep compiling. The return value is named, which + // is the one signature change that is always safe to make. + procedure CalculateDiscountWithRate(Amount: Decimal; Rate: Decimal) Discount: Decimal + begin + Discount := Amount * Rate; + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md new file mode 100644 index 0000000..a557d41 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [signature, public-procedure, parameter, return-value, overload, contract] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not change the signature of a published procedure + +## Description + +A procedure that is reachable from outside its object — any procedure not marked `local` (and, for on-prem-scoped code, anything a dependent app can still bind to) — is a contract. Once another extension compiles against it, changing its shape breaks that extension at build time. Signature changes include adding, removing, or reordering parameters, changing a parameter or return type, and toggling a parameter between by-value and `var` (by-reference). The platform treats the procedure's identity as its full signature, so even a "compatible-looking" tweak is a new method to dependents. There is exactly one safe edit: naming a previously unnamed return value, which adds no caller obligation. LLMs routinely "improve" a public procedure in place by adding a parameter, not realizing every consumer must be recompiled. + +## Best Practice + +Treat a published signature as frozen. When new behavior needs more inputs, add a new procedure or overload alongside the original — for example a `CalculateDiscountWithRate(Amount; Rate)` next to the unchanged `CalculateDiscount(Amount)` — and let the old one delegate to the new one. Existing callers keep compiling; new callers opt into the richer entry point. Naming an unnamed return value is the one in-place change that is always safe. + +See sample: `do-not-change-published-procedure-signatures.good.al`. + +## Anti Pattern + +Editing the existing public procedure's parameter list — here, adding a `Rate` parameter to `CalculateDiscount` — so every dependent extension that called the old form fails to compile. Detection: a parameter added, removed, reordered, retyped, or flipped to/from `var`, or a changed return type, on any non-`local` procedure that already shipped. Add a new overload instead. + +See sample: `do-not-change-published-procedure-signatures.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al new file mode 100644 index 0000000..e9d0941 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.bad.al @@ -0,0 +1,13 @@ +codeunit 50321 "Payment Client Bad" +{ + var + AccessToken: Text; + + // Crossing the trust boundary: a public getter hands the raw credential to any + // caller, turning a secret into a de-facto public API that cannot be removed + // later without breaking consumers. + procedure GetAccessToken(): Text + begin + exit(AccessToken); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al new file mode 100644 index 0000000..4073930 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.good.al @@ -0,0 +1,25 @@ +codeunit 50320 "Payment Client Good" +{ + var + AccessToken: Text; + + // Credential flows inward through an internal setter and never leaves the object. + internal procedure SetAccessToken(NewToken: Text) + begin + AccessToken := NewToken; + end; + + // Public API exposes only non-sensitive data — a masked reference, never the token. + procedure GetMaskedReference(): Text + var + Reference: Text; + begin + Reference := LastReference(); + exit('****-' + CopyStr(Reference, StrLen(Reference) - 3)); + end; + + local procedure LastReference(): Text + begin + exit('REF000123456'); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md new file mode 100644 index 0000000..65c7971 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-expose-sensitive-data-through-public-api.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [sensitive-data, secrettext, token, credential, public-api, access-boundary] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not widen access to expose sensitive data through a public API + +## Description + +Every member you make publicly reachable becomes a contract you must keep — and when that member returns a secret, the contract leaks the secret. Widening access to a credential happens in several shapes: a public getter that returns a raw token or password, an event whose parameter carries a secret to every subscriber, or a global variable holding a key that an extension can read. Once such a surface ships, removing it is itself a breaking change, so the exposure is hard to walk back. Sensitive material — tokens, passwords, connection secrets, `SecretText` values, security internals — must stay inside `internal` or `local` members. Public surfaces should expose only non-sensitive business data. LLMs often add a convenient `GetToken()` getter without recognizing it as a permanent security boundary breach. + +## Best Practice + +Keep secrets in `internal` or `local` members, and prefer the `SecretText` type so the value cannot be read back or logged. Where callers genuinely need a credential, pass it inward (a setter) rather than handing it outward (a getter). Public API should return only non-sensitive data — a masked reference, a status, a business identifier — never the raw secret. Treat each public member as a lasting commitment and keep the security-sensitive surface as small as possible. + +See sample: `do-not-expose-sensitive-data-through-public-api.good.al`. + +## Anti Pattern + +A public `GetAccessToken()` that returns the raw token (or an event parameter carrying a credential to all subscribers), turning a secret into a de-facto public API any dependent can consume. Detection: a non-`local` procedure, event parameter, or global variable that surfaces a token, password, key, or other credential. Keep the secret internal and expose only non-sensitive data. + +See sample: `do-not-expose-sensitive-data-through-public-api.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al new file mode 100644 index 0000000..c3727a9 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.bad.al @@ -0,0 +1,22 @@ +codeunit 50316 "Pricing Api Bad" +{ + // Anti-pattern: new surcharge logic is added inside a procedure already marked + // obsolete, and inside a #if not CLEAN25 block. Both are scheduled for removal, + // so this behaviour disappears the moment CLEAN25 is enabled. + [Obsolete('Use GetUnitPrice instead.', '25.0')] + procedure GetPrice(ItemNo: Code[20]): Decimal + var + Price: Decimal; + begin + Price := 100; +#if not CLEAN25 + Price += CalculateSurcharge(ItemNo); +#endif + exit(Price); + end; + + local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal + begin + exit(5); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al new file mode 100644 index 0000000..0049068 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.good.al @@ -0,0 +1,26 @@ +codeunit 50315 "Pricing Api Good" +{ + // Obsolete member left untouched — it only forwards to the replacement and + // gains no new logic. + [Obsolete('Use GetUnitPrice instead.', '25.0')] + procedure GetPrice(ItemNo: Code[20]): Decimal + begin + exit(GetUnitPrice(ItemNo)); + end; + + // New behaviour is built on the supported replacement, not on the obsolete member. + procedure GetUnitPrice(ItemNo: Code[20]): Decimal + begin + exit(CalculateBasePrice(ItemNo) + CalculateSurcharge(ItemNo)); + end; + + local procedure CalculateBasePrice(ItemNo: Code[20]): Decimal + begin + exit(100); + end; + + local procedure CalculateSurcharge(ItemNo: Code[20]): Decimal + begin + exit(5); + end; +} diff --git a/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md new file mode 100644 index 0000000..781810b --- /dev/null +++ b/microsoft/knowledge/breaking-changes/do-not-modify-code-already-marked-obsolete.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [obsolete, clean-flag, conditional-compilation, deprecation, replacement, do-not-extend] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Do not build on code already marked obsolete + +## Description + +A member carrying `[Obsolete]`, or wrapped in a `#if not CLEANxx` conditional-compilation block, is already scheduled for deletion — the `CLEANxx` symbol is flipped on in a future release to strip that code out. Adding logic, raising new events, or taking fresh dependencies on such a member ties live behavior to something the platform is about to remove. When the deprecation completes, everything layered on top breaks. The obsolete marker is a one-way signal: it means "migrate off," never "safe to extend." LLMs frequently edit whatever procedure is nearest to the change, including obsolete ones, and add `#if not CLEANxx` branches without understanding that the block is transient. + +## Best Practice + +Leave obsolete members exactly as they are and implement against the current, supported replacement. New logic — a surcharge calculation, an event publisher, a hook — belongs on the live API (`GetUnitPrice`), never inside the deprecated `GetPrice` or behind a `#if not CLEAN25` guard. If the replacement does not yet exist, create it as a first-class member and build there. The obsolete code should only shrink over time, not accrete new behavior. + +See sample: `do-not-modify-code-already-marked-obsolete.good.al`. + +## Anti Pattern + +Adding a surcharge calculation inside the `[Obsolete]` `GetPrice` procedure, or behind a `#if not CLEAN25` block, so the new behavior is wired to code that will be removed when `CLEAN25` is enabled. Detection: new statements, event declarations, or dependencies introduced inside an `[Obsolete]`-marked member or a `#if not CLEANxx` region. Move the logic onto the supported replacement instead. + +See sample: `do-not-modify-code-already-marked-obsolete.bad.al`. diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al new file mode 100644 index 0000000..0e2f000 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.bad.al @@ -0,0 +1,11 @@ +table 50311 "Customer Profile Bad" +{ + fields + { + field(1; "No."; Code[20]) { } + // Breaking: the published "Email" field was renamed in place. Dependent + // extensions that reference "Email" stop compiling, and the data stored in + // the old column is orphaned on upgrade. + field(2; "Contact Email"; Text[80]) { } + } +} diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al new file mode 100644 index 0000000..ed239d2 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.good.al @@ -0,0 +1,17 @@ +table 50310 "Customer Profile Good" +{ + fields + { + field(1; "No."; Code[20]) { } + // Replacement field shipped alongside the old one. + field(2; "Contact Email"; Text[80]) { } + // Old field kept and marked Pending so dependent code keeps compiling and + // an upgrade codeunit can copy its data before it is finally removed. + field(3; "Email"; Text[80]) + { + ObsoleteState = Pending; + ObsoleteReason = 'Replaced by Contact Email. Will be removed after the deprecation window.'; + ObsoleteTag = '25.0'; + } + } +} diff --git a/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md new file mode 100644 index 0000000..e1e7116 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [table-field, obsoletestate, obsoletereason, obsoletetag, pending, removed, data-loss] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Obsolete published table fields instead of deleting or renaming them + +## Description + +A table field that has shipped carries two contracts at once: extensions reference it by name, and the database holds data in its column. Deleting the field, or renaming it (which the platform treats as drop-plus-add), breaks dependent code at compile time and discards the stored data — a silent data-loss event on upgrade. The fix is the same staged lifecycle used for objects: set `ObsoleteState = Pending` together with `ObsoleteReason` and an `ObsoleteTag` naming the target version, ship the new field alongside, migrate data during the window, and only switch the old field to `ObsoleteState = Removed` in a later release once nothing depends on it. LLMs often "tidy" a schema by renaming a field in place, not realizing this is both a breaking change and a data-loss risk. + +## Best Practice + +Add the replacement field, then mark the old field `ObsoleteState = Pending` with an `ObsoleteReason` that names the replacement and an `ObsoleteTag` carrying the target version (for example `'25.0'`). Keep the obsolete field readable so an upgrade codeunit can copy its data into the new field during the deprecation window. Move it to `ObsoleteState = Removed` only in a later major version, after the window has passed and data has migrated. + +See sample: `obsolete-table-fields-instead-of-deleting-them.good.al`. + +## Anti Pattern + +Renaming the published `Email` field to `Contact Email` directly in the table — or deleting it — so dependent extensions that reference `Email` break and the column's stored values are orphaned on upgrade. Detection: a previously shipped field removed or renamed in a table or table extension with no `ObsoleteState = Pending` step preserving the original. Obsolete the field through the lifecycle instead. + +See sample: `obsolete-table-fields-instead-of-deleting-them.bad.al`. diff --git a/microsoft/skills/review/al-breaking-changes-review.md b/microsoft/skills/review/al-breaking-changes-review.md new file mode 100644 index 0000000..4238bca --- /dev/null +++ b/microsoft/skills/review/al-breaking-changes-review.md @@ -0,0 +1,136 @@ +--- +kind: action-skill +id: al-breaking-changes-review +version: 1 +title: AL breaking changes review +description: Reviews AL source changes against breaking-changes guidance from BCQuality. +inputs: [pr-diff, file-path] +outputs: [findings-report] +bc-version: [all] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# AL breaking changes review + +Reviews AL source changes against the `breaking-changes` 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`. + +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. + +## Source + +Read the BCQuality knowledge index once — the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone — see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint — exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `breaking-changes` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/breaking-changes/**`. + +## Relevance + +Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) 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`. Default to the orchestrator's configured context; if absent, `unknown`. +- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`. + +Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown. + +## Worklist + +Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against: + +- The changed AL object names and types — especially codeunits, tables, and table extensions that expose procedures, fields, or events to other apps, and any member whose access is being widened. +- The changed procedures, fields, and triggers, weighted toward non-`local` procedures, published table fields, event publishers, and any member whose signature, access modifier, or obsolete state is being altered. +- Tokens extracted from the diff that relate to API stability and deprecation (`signature`, `parameter`, `return`, `var`, `Obsolete`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `Pending`, `Removed`, `CLEAN`, `SecretText`, `token`, `internal`, `local`, `public`, `protected`, `Scope`). + +A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. + +Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`. + +When the post-conflict worklist is empty because no applicable breaking-changes knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable breaking-changes 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. 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`. +- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. +- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`. + +Set `confidence` to: + +- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type). +- `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 breaking-changes 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`, `severity` capped at `minor` (agent findings are advisory and non-gating), 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). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material API-stability defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly breaking changes; 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. + +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: restore a published procedure's parameter list and add a new overload for the extra argument; add an `[Obsolete]` attribute to a member being removed; change a needlessly `public` helper to `internal`). For mechanical findings, emit `findings[].suggested-code` with 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. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. + +Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. + +Outcome selection: + +- `completed` — the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty. +- `no-knowledge` — no applicable breaking-changes knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty. +- `not-applicable` — the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task). +- `partial` — a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause. +- `failed` — an unrecoverable error occurred. `outcome-reason` is required. + +## Output + +Output conforms to the DO output contract. A populated example: + +```json +{ + "skill": { "id": "al-breaking-changes-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/breaking-changes/do-not-change-published-procedure-signatures.md", + "severity": "major", + "message": "A parameter was added to the published procedure CalculateDiscount, breaking every dependent extension that called the previous form. Add a new overload alongside the unchanged procedure instead.", + "location": { + "file": "src/Sales/DiscountApi.Codeunit.al", + "line": 12, + "range": { "start-line": 12, "end-line": 15 } + }, + "references": [ + { "path": "microsoft/knowledge/breaking-changes/do-not-change-published-procedure-signatures.md" } + ], + "confidence": "high" + }, + { + "id": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md", + "severity": "minor", + "message": "An implementation-detail helper is declared public with no reason to support it externally, making it a de-facto API. Default it to internal or local.", + "location": { + "file": "src/Sales/OrderProcessor.Codeunit.al", + "line": 20 + }, + "references": [ + { "path": "microsoft/knowledge/breaking-changes/choose-access-modifiers-deliberately.md" } + ], + "confidence": "medium" + } + ], + "suppressed": [] +} +``` + +The empty-corpus case — BCQuality's state until breaking-changes knowledge files land — produces: + +```json +{ + "skill": { "id": "al-breaking-changes-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": [] +} +``` diff --git a/microsoft/skills/review/al-code-review.md b/microsoft/skills/review/al-code-review.md index 7fd1505..0e2ec31 100644 --- a/microsoft/skills/review/al-code-review.md +++ b/microsoft/skills/review/al-code-review.md @@ -3,8 +3,7 @@ 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, error handling, events, interfaces). - +description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces, breaking changes). inputs: [pr-diff, file-path] outputs: [findings-report] bc-version: [all] @@ -21,6 +20,7 @@ sub-skills: - microsoft/skills/review/al-error-handling-review.md - microsoft/skills/review/al-events-review.md - microsoft/skills/review/al-interfaces-review.md + - microsoft/skills/review/al-breaking-changes-review.md --- # AL code review @@ -44,6 +44,7 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills - `microsoft/skills/review/al-error-handling-review.md` - `microsoft/skills/review/al-events-review.md` - `microsoft/skills/review/al-interfaces-review.md` +- `microsoft/skills/review/al-breaking-changes-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.