Skip to content

Add events knowledge domain and review leaf skill - #43

Merged
Jesper Schulz-Wedde (JesperSchulz) merged 6 commits into
mainfrom
jesperschulz-seed-events-domain
Jun 25, 2026
Merged

Jesper Schulz-Wedde (JesperSchulz) merged 6 commits into
mainfrom
jesperschulz-seed-events-domain

Conversation

@JesperSchulz

Copy link
Copy Markdown
Contributor

Summary

Adds a new events knowledge domain covering AL events & subscribers, fully wired into the AL review pipeline. This is increment #3 of the knowledge-seeding initiative, following error-handling and interfaces. The change is purely additive — no contract change (skills/read.md|write.md|do.md|entry.md untouched).

3 atomic articles (each with .good.al + .bad.al samples)

Under microsoft/knowledge/events/:

  1. Use the IsHandled pattern to make base behaviour overridable — publisher exposes [IntegrationEvent(false, false)] local procedure OnBeforeX(…; var IsHandled: Boolean); the routine raises it and guards if IsHandled then exit;; a subscriber sets IsHandled := true. Bad: no OnBefore/IsHandled hook (not extensible), or a missing if IsHandled then exit; guard.
  2. Publish thin OnBefore/OnAfter integration events to expose extension points — raise thin events at operation boundaries passing var Rec/var IsHandled, with empty publisher bodies. Bad: business logic inside the [IntegrationEvent] publisher body, or no extension points at all.
  3. Choose static vs manual subscribers deliberately; bind manual subscribers with BindSubscription — static [EventSubscriber] for always-on; EventSubscriberInstance = Manual + BindSubscription/UnbindSubscription for scoped behaviour. Bad: static subscriber for always-on side effects, or a manual subscriber bound (and held on a SingleInstance global) and never unbound.

Leaf review skill

  • microsoft/skills/review/al-events-review.md — modeled on al-error-handling-review.md: 5 sections (Source/Relevance/Worklist/Action/Output), sources the events domain, worklist tokens (IntegrationEvent, BusinessEvent, EventSubscriber, IsHandled, BindSubscription, UnbindSubscription, EventSubscriberInstance, OnBefore, OnAfter, Manual), populated + empty-corpus findings-report examples.

Wiring (additive)

  • microsoft/skills/review/al-code-review.md — added the leaf to the frontmatter sub-skills list and the ## Source bullet list; appended , events to the line-6 description parenthetical.
  • README.md — leaf-skill count seven → eight; appended , events to the domain list.

API verification

All AL event syntax was verified against Microsoft Learn (not training recall): IntegrationEvent/BusinessEvent attributes and empty publisher bodies; the exact [EventSubscriber(ObjectType, ObjectId, EventName, ElementName, SkipOnMissingLicense, SkipOnMissingPermission)] order; EventSubscriberInstance = Manual;; BindSubscription/UnbindSubscription; and the var IsHandled: Boolean + if IsHandled then exit; convention. Object IDs use 50220–50235 (within the assigned 50220–50239 range; no collision with error-handling 50180–50191 or interfaces 50200–50219). The .al samples are demonstration-only and not compiled by CI.

Validation

  • python .github/scripts/validate_frontmatter.py → 0 errors, 0 warnings
  • pwsh .github/scripts/Test-KnowledgeIndex.ps1 → PASSED (163 articles, deterministic, full coverage, selection inputs intact)

Notes

  • May need a trivial rebase if PR Add interfaces knowledge domain and review leaf skill #42 (interfaces) merges first — both touch the same al-code-review.md sub-skills/Source/description lines and the README count/domain line. Resolve by keeping both leaves and setting the README count to the correct total.

Part of #34.

Jesper Schulz-Wedde and others added 4 commits June 25, 2026 11:58
Add a new `events` knowledge domain covering AL events & subscribers,
wired into the AL review pipeline.

- 3 atomic articles (+ .good.al/.bad.al samples) under
  microsoft/knowledge/events/: the IsHandled override pattern, thin
  OnBefore/OnAfter integration-event publishers, and static vs manual
  subscribers.
- New leaf skill microsoft/skills/review/al-events-review.md sourcing the
  events domain.
- Wired into microsoft/skills/review/al-code-review.md (sub-skills + Source
  + description) and README.md (leaf-skill count + domain list).

AL event syntax verified against Microsoft Learn. Samples are
demonstration-only (not compiled by CI). Additive change; no contract change.

Part of #34.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add 12 atomic knowledge articles under microsoft/knowledge/events covering
general AL event-design best practices: IsHandled initialization and OnAfter
preservation, appending new event parameters, position-based event naming,
reusing/extending events, avoiding per-iteration publishing, Temp-prefixing
temporary record parameters, unabbreviated parameter names, preferring the
this keyword over IncludeSender, avoiding loosely typed parameters, not
mutating existing event contracts, and not bypassing critical operations
with IsHandled. Each article ships a .good.al and .bad.al demonstration
sample (object IDs 50240-50296; not compiled by CI). Extend the
al-events-review leaf Worklist with one targeted check per new rule.

Additive only; no contract or wiring change (events leaf already wired).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Correct wording in five events articles to reflect that AL event
subscribers bind by parameter name, not position:

- add-new-event-parameters-at-the-end: drop the inaccurate claim that
  appending a parameter forces subscribers to be updated or causes wrong
  values; keep the append-at-end best practice.
- do-not-add-ishandled-to-an-existing-event: reframe from "breaking
  change" to the semantic/purpose shift that leaves existing subscribers
  pointless; rename the breaking-change keyword to semantic-change.
- name-events-by-publisher-position: extend the good sample with
  position-named publishers raised from table and report trigger
  contexts.
- initialize-ishandled-to-false-before-publishing: scope the detection
  and best practice to events that actually carry a var IsHandled, so an
  OnBefore with no IsHandled is not flagged.
- do-not-bypass-critical-operations-with-ishandled: add a litmus-test
  definition of a critical operation (code that cannot stand as an
  independent, self-contained unit).

Knowledge-only; no contract or wiring change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove the last name-vs-position misconception from the Anti Pattern so it
is consistent with the corrected Description: mid-list insertion is framed
as noisy and harder to review rather than as forcing subscriber re-mapping.
Detection sentence unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add @AleksandricMarko and @pchriste-microsoft-com as required reviewers
for the events knowledge domain, matching the existing per-domain expert
ownership convention. Inserted in alphabetical order ahead of the
performance line.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants