The v1.4 policy deferred every missing-tooltip case to compiler analyzer AA0218. But AA0218 severity is per-app ruleset config and is routinely downgraded to info/None or disabled, so a genuine gap can ship unflagged. PR review is the last line of defence and should raise it independently.
Keeps the false-positive guard intact: a bound field whose source table field supplies a ToolTip still inherits it and is NOT flagged. Adds the genuinely-missing case (bound field whose source is also tooltip-less, or an unbound control) as a medium-severity finding. Updates both the ui inheritance article and the style AA0218 article.
Online eval shows style/variable-declaration-order-by-type firing as false positives (tp/fp 2/1) at minor. The article title itself is 'Order variable declarations by type (CodeCop AA0021)', so it is analyzer-redundant exactly like the this-keyword AA0248, label-suffix AA0074, and ToolTip rules already calibrated to info. Add AA0021 to the analyzer-redundant list so it emits at info and stops competing with substantive style review.
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
* Add precision guards for systematic agent false-positive patterns
Encodes reviewer-confirmed FP guards from the online eval: tooltip-inherited, page-trigger default return, drill-down filter not visible in diff, dual-trigger CalcFields (do.md); and a released-baseline precondition for breaking-change/upgrade findings on never-shipped symbols.
* Scope suggested-code and location to exactly the changed lines
Addresses reviewer-reported misplaced suggestions from the eval: insert-only-property emitting the whole field, single-statement rewrites anchored on the procedure name, and reductive multi-line collapses. The skill now emits a location range that matches precisely the rewritten lines.
* Correct suggested-code scoping guidance to match one-click anchor mechanics
A lone inserted line matches no existing file line and cannot be anchored; bracket the new line with one adjacent unchanged line instead. Reductive collapses omit suggested-code and fall back to a manual snippet.
* Move BC-specific FP guards out of do.md into leaf skills
do.md is the stable action-skill template and must stay domain-agnostic (JesperSchulz review). Relocate the four known false-positive patterns to their domain leaves: ToolTip-inheritance to al-ui-review; drill-down/lookup filtering and CalcFields lifecycle to al-performance-review; page-trigger exit(true) semantics to al-error-handling-review.
* Move suggested-code line-scoping guidance out of do.md into al-code-review
do.md must not carry instructions for how the review skill behaves (JesperSchulz review). Relocate the location/suggested-code precise-span rules to al-code-review's existing Suggested-code guidance section. do.md is now unchanged vs main.
* Move false-positive guards from skills into knowledge files
Keep review skills slim (finders/appliers). The FP guards and released-baseline preconditions previously embedded in leaf skills become negative-clarification knowledge articles in their domains, and the agent-findings policy edits to al-ui/al-privacy are reverted to main. Adds 6 knowledge files: error-handling (page-boolean-triggers-default-to-true), ui (bound-page-field-inherits-source-field-tooltip), performance (calcfields-in-both-getrecord-triggers-is-not-redundant, page-effective-filter-may-live-outside-the-diff), breaking-changes (unreleased-symbol-change-is-not-a-breaking-change), upgrade (unreleased-schema-change-needs-no-upgrade-path).
* Revert branch's suggested-code scoping addition in al-code-review
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.
* Restore al-ui/al-privacy review skills to base (zero diff in PR)
These two leaf skills carried an accidental net change against the PR merge-base because an earlier revert used the current origin/main (post-#110) instead of the branch base (pre-#110). Restoring them to the merge-base version removes them from the PR diff entirely. Three-way merge still preserves main's #110 suppression.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
Online eval (182 PRs) shows the reference-less agent-finding channel is where 86% of false positives come from, and it is net-negative in the lowest-yield domains: privacy agent findings score 0 TP / 8 FP and UI/accessibility 1 TP / 11 FP. Restrict these two leaves to knowledge-backed findings only; an uncovered concern is omitted (and, if material and recurring, fixed durably by adding a BCQuality article per the self-improvement loop) instead of emitted with references: [].
* Complete AL review knowledge readiness
Fill telemetry and Query coverage, strengthen thin review domains, correct audited content defects, and add deterministic cheap-model evaluation and reference-integrity safeguards.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Generalize review fixture discovery
Derive smoke cases from the leaf, domain, and paired-sample conventions so new leaves require no scoring-contract changes. Keep only exceptional selection/context overrides and fail when retrieval metadata cannot rank the selected article.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Preserve published field IDs in sample
Keep the existing Email and Contact Email field IDs unchanged, clarify that the sample represents an independent baseline, and use a local breaking-change rule for the generic smoke evaluation.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Clarify published field identity rules
State explicitly that a published field keeps its ID, name, and type while a replacement is added as a separate field under an unused ID.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
* Align field obsoletion sample baselines
Use Email field ID 3 as the shared baseline so the bad example demonstrates a same-ID rename while the good example retains the original field and adds a separate replacement.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 9825b012-e653-496a-9310-c1f4b6f8ac27
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
* Emit human-readable domain label on review findings
Add an optional findings[].domain field to the DO review output contract so
each finding carries its own human-readable review-domain display label. Leaf
review skills set it on every finding they emit; the al-code-review super-skill
copies it verbatim during rollup and sets it to "Agent" for its own
cross-cutting agent findings. This decouples consumers from BCQuality's domain
taxonomy: they render finding.domain verbatim instead of maintaining a
sub-skill-id -> label map.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Define domain display-label constraints
Clarify that review domains may contain internal whitespace, punctuation, case-sensitive text, and non-ASCII characters. Require consumers to preserve and safely encode the complete label instead of relying on lossy slugs, matching the replacement BC-ALAgents consumer.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 77d0a40e-8bf5-40ac-a450-40eb0255db03
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Move eight net-new rules into the Microsoft layer, remove six overlapping articles, and update review skill discovery and references.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b130227-d418-4bc0-9e7d-ec6a37adf039
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
The al-style-review worked example set findings[0].id to a non-existent
knowledge file (apply-approved-label-suffixes.md) while references[0].path
points to the real file (label-suffix-approved-list.md). The DO output
contract requires id to equal references[0].path for citation-based
findings. Align id to the existing file.
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
Online-eval data shows the style leaf is the largest source of dismissed
findings. Two causes:
- Mechanical, analyzer-enforced conventions (this-keyword AA0248, label
suffixes AA0074, missing ToolTip, label scope) fire at gating severity
and duplicate what CodeCop/AppSourceCop already report. Calibrate them
to info so a severity-gating consumer drops the redundant noise.
- Correctness/logic/data-integrity defects get reframed as style
conventions and emitted here. Sharpen the scope boundary: such defects
belong to the relevant domain leaf, or to al-code-review's cross-cutting
agent channel when no knowledge file covers them, never to this leaf.
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
* Promote security knowledge from community to Microsoft layer
Pure git-mv relocation of the SECURITY domain from the community layer to the Microsoft layer. No content changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review feedback on security knowledge promotion
- do-not-grant-rights-beyond-a-users-entitlement.md: drop the See sample
reference to a .good.al file that does not exist
- Remove the 'Contributions welcome' boilerplate line from
compose-permission-sets, prefer-oauth2, and protect-sensitive-data
- protect-sensitive-data-in-temporary-tables: remove the pointless
DeleteAll on the locally scoped temp buffer in the good sample and
reword Best Practice to note local buffers are cleaned up automatically
- Drop guard-bulk-operations-with-istemporary from the promotion; it
stays in the community layer pending a decision on whether it is security
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Remove Contributions welcome boilerplate from do-not-grant article for consistency
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author 7 remedial BCQuality knowledge articles plus good/bad AL samples
(21 files) covering AL master-table and data-model design:
- data-modeling: master No. from number series in OnInsert; use codeunit
"No. Series" not obsolete NoSeriesManagement; setup table is a singleton;
set Last Date Modified in OnModify and OnRename; enforce Blocked in
referencing code not in the master.
- style: ApplicationArea required on page controls (AS0062).
- appsource: object affixes prevent collisions (AS0011).
Clean-room authored from own BC knowledge; specifics verified against public
sources only (learn.microsoft.com, microsoft/BCApps). Introduces two new
domains (data-modeling, appsource).
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Repair broken knowledge references and align review examples with canonical articles. Correct explicit version gates, restore a missing title, and recognize the plugin directory in the root guard.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add testing knowledge: UI handlers, table relations, asserterror, fixtures (P1+P2)
Six BC-specific testing-domain knowledge articles in community/knowledge/testing/, each with .good.al/.bad.al samples:
- ui-calls-require-test-handlers
- tablerelation-requires-prerequisite-records
- handlers-enqueue-never-assert
- handlerfunctions-attribute-must-match-ui-path
- asserterror-needs-expectederror-and-code
- use-library-codeunits-for-test-fixtures
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Move testing knowledge from community to microsoft layer
Relocates the six P1+P2 testing-domain articles (18 files: .md + .good.al + .bad.al each) from community/knowledge/testing/ to microsoft/knowledge/testing/ per maintainer request. Pure git-mv rename; no content or frontmatter changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Address review: merge handler articles, adopt enqueue-driven pattern
Respond to @nikolakukrika's review on #62:
- Merge ui-calls-require-test-handlers, handlerfunctions-attribute-must-match-ui-path
and handlers-enqueue-never-assert into a single ui-handlers-in-tests article.
- Adopt the enqueue-from-test / dequeue-and-assert-in-handler pattern using
Assert.ExpectedConfirm/ExpectedMessage (substring match), with Initialize()
clearing LibraryVariableStorage and AssertEmpty() proving exact call counts.
- asserterror sample now uses Assert.ExpectedTestFieldError + FieldCaption instead
of hardcoded message/code; article text points to the library helpers.
- Drop the tablerelation article and fold its test-relevant ordering point
(relations checked on Validate/Insert(true); build parents first) into
use-library-codeunits-for-test-fixtures.
Article count 198 -> 195.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Pure git-mv relocation of all 7 performance articles from community/knowledge/performance/ to microsoft/knowledge/performance/. No content changes.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
De-enumerate the AL review leaf list so it lives in exactly one place:
the al-code-review super-skill's frontmatter sub-skills list.
- al-code-review.md: drop the enumerated parenthetical from description
and remove the redundant bullet re-list in the Source section.
- README.md: remove the hardcoded count word and the enumerated domain
list from the leaf-skill sentence.
- validate_frontmatter.py: add cross-file rule R26 asserting a
super-skill's declared sub-skills exactly match the sibling
al-*-review.md leaf files on disk (missing, stale, and unregistered
leaves all fail CI).
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Seed web-services (API v2) knowledge domain
Add eight web-services API page knowledge articles (each with .good.al/.bad.al samples), a new al-web-services-review leaf skill, and wire it into al-code-review and the README.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Trim web-services domain to 6 non-duplicative articles
Drop API entity-naming/camelCase and DelayedInsert articles (owned by the style domain). Reframe the committed-data and API-versioning articles to stay strictly within the endpoint design/behavior lane, and update the leaf skill's worklist tokens and Output example accordingly.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Seed a new breaking-changes (AL API stability) knowledge domain with six
articles plus good/bad AL samples, a new al-breaking-changes-review leaf
skill, and minimal wiring into al-code-review and the README.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Add events knowledge domain and review leaf skill
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 general AL event-design articles to events domain
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>
* Refine events articles after review feedback
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>
* Soften Anti Pattern wording in add-new-event-parameters article
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 events domain reviewers to CODEOWNERS
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>
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the interfaces knowledge domain covering AL interfaces and enum-with-implementation: three atomic articles with good/bad AL samples, a new al-interfaces-review leaf skill, and additive wiring into al-code-review and the README. Purely additive; no contract change.
Part of #34.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A closed range like [23..28] wrongly implies guidance stops applying after
BC28, so a reviewer targeting BC29+ would not match the file. Introduce an
open-ended shorthand [N..] meaning ''version N and every later version''.
- validate_frontmatter.py: RANGE_SHORTHAND allows an optional upper bound;
expand_bc_version returns the normalized string ''N..'' for open-ended.
- read.md: document the fourth bc-version form and its matching rule
(matches target >= N; not enumerable).
- write.md: prefer [N..] over a closed range for a feature introduced in N
and not expected to be removed.
- Apply [23..] to the actionable-errors article (actionable errors shipped
in BC23 and are not version-bounded above).
- README: mention [N..] in the frontmatter example.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Make domain-skill knowledge discovery index-aware
The 6 AL domain review skills and read.md now enumerate candidate articles
from the BCQuality knowledge index (knowledge-index.json) instead of opening
every file under the domain folder to read its frontmatter. The worklist
selection predicate is unchanged (keywords intersect diff tokens, or topic
matches a changed object type) - only the discovery source changes, so the
same articles are selected. Full article bodies are read only for worklisted
entries.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Reconcile §Source wording with the lean knowledge index
The BCQuality filter now emits a lean index whose per-article description is a
one-line hint rather than the full verbatim Description. Update the six domain
skills' §Source to say the index carries a one-line description hint (keywords,
title, and a one-line description) instead of the full description. The
worklist selection predicate is unchanged: keywords drive selection and the
agent opens worklisted articles in full for their rule bodies.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Own the knowledge-index generator in BCQuality
The knowledge index is an acceleration of the skills' Source step, and its
schema is part of that contract — so BCQuality should own the generator rather
than each consumer re-implementing it. Add tools/Build-KnowledgeIndex.ps1 (the
parser + lean-description shaping + emit, lifted verbatim from the
BCAppsBCQuality filter prototype) and document the index in agent-consumption.md.
Consumers prune their clone to policy, then call this script; the index stays
in lockstep with the Source contract and every orchestrator gets the same
faithful index for free. The worklist selection predicate is unchanged.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Own knowledge-index generation in BCQuality (runtime + CI), not the consumer
The index is now produced by BCQuality itself: Entry's preparation step
rebuilds knowledge-index.json over the live, already-pruned clone at the start
of every run, and a new CI workflow validates the generator's health
(determinism, full coverage, selection-input integrity). Consumers no longer
invoke or know about the index.
Rebuilding over the pruned clone (vs shipping a committed full-corpus index)
keeps the index exact for any consumer policy: it can never list a denied
article, so policy-excluded rules cannot leak into discovery. READ now states
the index is discovery-only -- a finding must cite an article opened in full,
and rows whose file is absent are discarded before ranking.
- skills/entry.md: new 'Preparation -- knowledge index' precondition
- skills/read.md: index ownership + discovery-only invariant
- microsoft/skills/review/*.md (6): 'BCQuality builds' (not 'the filter emits')
- agent-consumption.md 5a: runtime+CI ownership rationale
- .github/workflows/knowledge-index.yml + scripts/Test-KnowledgeIndex.ps1: generator guard
- .gitignore: never commit the runtime index
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Make runtime index build non-interactive and self-contained
entry.md now gives the exact build command (pwsh ./tools/Build-KnowledgeIndex.ps1)
so the agent's preparation step is unambiguous, and the generator's -BCQualityRoot
parameter is optional (defaults to the clone root) so it runs in non-interactive
-p mode without prompting.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Resolve knowledge-index root to absolute path (cross-platform fix)
Get-ChildItem.FullName is always absolute, so deriving the relative article
path via Substring(\.Length) requires an absolute root. A relative root
such as '.' (used by the CI guard's 'Test-KnowledgeIndex.ps1 -Root .') left the
full path almost intact on Linux, producing bogus 'home/runner/.../knowledge'
paths and failing the coverage check. Normalise both the generator's
-BCQualityRoot and the test's -Root with Resolve-Path before use.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
---------
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Agent findings (model-judgment findings not backed by a curated knowledge
file) were producing too many false positives. Raise their emission bar and
make them advisory:
- skills/do.md: add a canonical 'Precision bar' for agent findings (emit only
concrete, material defects an expert would agree on; steelman first; an
explicit never-emit list; 'when in doubt, omit') and cap agent-finding
severity at minor (advisory, non-gating), with a promotion note for
severe-but-uncovered concerns.
- al-code-review.md: decouple the mandatory self-review reasoning from output
so emitting zero agent findings is a valid outcome; add the severity cap.
- Six leaf review skills: add the per-domain precision bar and severity cap
(style leaf gets domain-appropriate wording).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BCQ now wins on coverage in the synthetic complex benchmark, but only
2 of 25 rendered comments carried GitHub suggestion blocks while the
AIRHack reviewer emitted suggestions for every finding. Tighten the
DO contract and leaf-skill instructions so suggested-code is no longer
a soft affordance for mechanical fixes.
Changes:
- Add optional findings[].suggested-code-omission-reason to the DO
schema.
- Document suggested-code as expected for small, local, mechanical
findings, with examples (delete unreachable code, Count() > 0 ->
not IsEmpty(), move local Label, add missing ToolTip/OptionCaption,
replace string-concatenated Error, change permission token, add an
obvious else/guard branch).
- Require omission reasons when a mechanical-looking finding omits
suggested-code.
- Update all AL leaf skills and al-code-review guidance with the same
stronger contract.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
Drop the specific pattern checklist (architecture-level smells, error-
handling gaps, magic constants, privacy/telemetry surface, resource
lifecycle - with concrete code patterns under each). It baked case-
study findings into the skill contract and aged badly. Replace with a
domain-level framing: walk by the domains the sub-skills already cover
(performance, security, privacy, style, upgrade, UI) plus the cross-
cutting concerns (architecture, error handling, resource lifecycle).
The domains are anchors for completeness, not a script.
The structural rule - do an explicit self-review pass after the leaves
- is what carries the value. The content of the pass belongs in the
agent's general AL judgement and in the knowledge layer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Execution-discipline change + suggested-code propagation in the
skills is the structural fix. The two knowledge articles
(case-must-handle-unknown-enum-values, instream-length-unreliable-for-bc-streams)
were T4 follow-ups derived from a single parity case study; they need
broader review before landing as canonical BCQuality knowledge and are
out of scope for this PR.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Driven by a parity comparison between BCAppsBCQuality PR #27 and
BCAppsCampAIRHack PR #162 on byte-identical content:
| | BCQuality | AIRHack |
|--|--|--|
| Total findings | 6 | 10 |
| Performance | 0 | 4 |
| Security | 0 | 5 |
Standalone runs of al-security-review and al-performance-review against
the SAME diff produced the expected matches (rimd-on-read-only via
inherent-permissions-minimal-grant; redundant-Get via
avoid-redundant-get-when-record-already-loaded). The miss in the live
run is therefore not a knowledge-coverage gap and not a worklist
filtering issue. It is attention dilution inside the al-code-review
super-skill, which the model collapses into one rolled-up generation
pass on real-size PRs.
Changes:
microsoft/skills/review/al-code-review.md
- New 'Execution discipline (mandatory)' subsection in the Action step
that explicitly forbids collapsing leaves into one shared reasoning
pass and requires each sub-skill to walk its Source -> Relevance ->
Worklist -> Action steps as its own iteration before the next leaf
starts.
- Self-review pass is now described as the final, mandatory iteration
with a concrete candidate-category checklist (architecture-level
smells, error-handling gaps, magic constants, privacy/telemetry,
resource lifecycle). Returning zero agent findings on a real-size
diff is explicitly defined as a defect.
microsoft/skills/review/al-{security,performance,privacy,style,
upgrade,ui}-review.md
- Each leaf skill now states that when an unambiguous .good.al
companion exists, findings[].suggested-code should carry the
literal replacement for the source lines. Closes the
one-click-suggestion gap created when BCQ#19 only updated
al-code-review.
microsoft/knowledge/security/case-must-handle-unknown-enum-values.{md,
bad.al,
good.al}
- New article: case over a security-sensitive enum (Authentication
Type, Authorization Mode, Identity Provider, Permission Scope,
Encryption Algorithm) MUST have an else arm. Without it, an unknown
enum value silently falls through and the security context never
initialises. The bad sample is lifted from the SharePoint Graph
helper that triggered the parity finding.
microsoft/knowledge/performance/instream-length-unreliable-for-bc-
streams.{md,bad,good}
- New article: InStream.Length returns 0 / partial for HTTP-response
streams and some file-API streams, breaking size-threshold branching
in upload code. Bad sample is the simple-vs-chunked Graph upload
pattern; good sample materialises into a Temp Blob first.
Companion change: microsoft/BCAppsBCQuality#28 extends the
orchestrator's bootstrap prompt with the same per-iteration execution
discipline and adds a CI warning when a >5-file PR returns zero agent
findings (regression signal).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the contract field, skill instructions, and two knowledge articles
that BCAppsBCQuality's PR-review agent needs to match (and exceed) the
coverage of the embedded review agent in BCAppsCampAIRHack:
skills/do.md
- New optional findings[].suggested-code field. Documents what it MUST
contain (a literal line-replacement payload) and when to emit it.
microsoft/skills/review/al-code-review.md
- Instructs both the agent self-review pass and rolled-up sub-skill
findings to populate suggested-code when the fix is mechanical.
- Lists examples (dead code removal, Count > 0 -> IsEmpty, object-scope
Label) that map to issues observed in the parity comparison.
microsoft/knowledge/style/telemetry-event-id-stable-unique.{md,bad.al,good.al}
- New knowledge article: telemetry event IDs must be stable, unique,
and non-placeholder. Closes a gap surfaced by the parity comparison.
microsoft/knowledge/style/labels-declared-at-object-scope.{md,bad.al,good.al}
- New knowledge article: Labels must live in the object-level var
block, not in procedure-local var blocks. Closes the second gap.
microsoft/knowledge/privacy/no-pii-in-telemetry-message-string.md
- Adds an explicit note that changing DataClassification alone does not
make embedding PII into the message string acceptable, plus links to
the two adjacent privacy articles. Resolves the privacy advice the
parity comparison flagged as ambiguous.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous LLM-generated knowledge files contained factual
hallucinations. The most visible was the claim that `FindFirst` /
`FindLast` "forces a full-table scan" on an unfiltered record - it does
not; those APIs return a single row via the current key.
Other inaccuracies the audit found and fixed:
* `FindSet(true)` was described as "taking a LockTable". The correct
upstream phrasing is that `FindSet(true)` sets
`ReadIsolation::UpdLock` on the read. UpdLock and LockTable are
related but distinct mechanisms.
* The list of production-scale tables had been invented beyond the
upstream source (e.g. "Detailed Cust. Ledg. Entry") without a
citation. The regenerated list matches the ten tables upstream lists
with their P95 row counts.
* `SetLoadFields` guidance had been augmented with an extra mechanism
claim ("the database resolves the filter using the index without
hydrating the value") not present in upstream.
Approach: full regeneration of `microsoft/knowledge/` from the six
upstream BCApps Code Review instruction files, with Microsoft Learn /
the AL language reference as a secondary source. Every claim in every
regenerated file is anchored to a verbatim upstream quote (or a Learn
URL); the audit trail lives in artifacts/trace-<domain>.json on the
session workspace.
The PR #11 transaction/error-handling cluster is preserved verbatim:
* performance/understand-implicit-transaction-boundary.md
* performance/codeunit-run-as-atomic-sub-operation.{md,good.al,bad.al}
* performance/codeunit-run-requires-prior-commit-inside-transaction.{md,good.al,bad.al}
* performance/use-tryfunction-for-error-catching-not-rollback.{md,good.al,bad.al}
* performance/avoid-commit-inside-loops.{md,good.al,bad.al}
* security/commitbehavior-attribute-scopes-explicit-commits.{md,good.al,bad.al}
* testing/transactionmodel-attribute-governs-test-transactions.{md,good.al,bad.al}
These articles already cite Microsoft Learn and were carefully
cross-referenced; the regeneration skips their topics rather than
duplicating them.
File counts after regeneration:
performance 35 .md (5 preserved + 30 new)
privacy 17 .md
security 18 .md (1 preserved + 17 new)
style 33 .md
testing 1 .md (preserved)
ui 19 .md
upgrade 18 .md
Total 141 atomic knowledge files, each strictly one rule. All pass
.github/scripts/validate_frontmatter.py with 0 errors and 0 warnings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Let super-skills surface findings the agent identifies on its own,
clearly tagged so consumers can render them differently from
knowledge-backed ones.
- skills/do.md: permit references:[] when from-sub-skill='agent';
define the agent-finding encoding (id 'agent:<slug>', confidence
capped at medium, self-contained message); restrict agent findings
to super-skills only.
- microsoft/skills/review/al-code-review.md: add a self-review pass
to Action that validates agent-identified candidates against
BCQuality (cite if matched, suppress if contradicted, surface as
agent finding otherwise). Add example finding.
- agent-consumption.md, README.md: describe the additive model and
the from-sub-skill: 'agent' marker so consumer orchestrators know
to render unbacked findings.
Strictly additive: existing knowledge-backed flow is unchanged and
backward compatible.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cover non-obvious platform behaviors a capable LLM reliably gets wrong:
hidden FlowFields still calculate, LockTable scopes to the whole table,
query objects bypass the primary-key cache, table-event subscribers
disable bulk ModifyAll/DeleteAll, Blob fields are uncached, OnCompanyOpen
subscribers block every session creation, and the test framework
disables bulk insert mode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- performance/use-setloadfields-for-partial-records: Clarify that
filter-only fields (SetRange/SetFilter) do not need to be listed in
SetLoadFields — the DB resolves them via the index without hydrating
the value into AL memory.
- performance/avoid-calcfields-in-loops: Add explicit exception for
OnAfterGetRecord and OnValidate triggers, which are platform-managed
and not developer-authored loops.
- performance/split-read-only-and-write-paths-to-avoid-locktable: Add
ReadIsolation as the primary recommendation for read-only paths;
LockTable reserved for confirmed write paths only.
- performance/prefer-direct-record-over-recordref: Scope the finding to
hot unbounded loops (10k+ rows) over ledger-entry-scale tables;
RecordRef in bounded/admin/setup contexts is not a concern.
- upgrade/enum-changes-must-be-additive-at-the-end: Replace direct
ObsoleteState = Removed guidance with the two-stage workflow (Pending
first, Removed later); reference use-obsolete-pending-before-removed.
- upgrade/use-datatransfer-for-large-dataset-initialization: Add the
>300,000 records threshold as the concrete trigger for requiring
DataTransfer.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The 35 articles still in their seed form previously carried a banner
reading "Seed article. ... Domain stewards should expand, restructure,
and refine as needed." For a community preview, that phrasing reads as
"TODO left in production" to first-time visitors.
Replace all three banner variants (performance-seeded, security-seeded,
community-ported) with a single positive invitation:
> Contributions welcome — open a PR to refine or extend this article.
Content and structure of the articles are unchanged; only the leading
quote block differs. Articles that had their banner fully stripped in
the earlier triage pass (the showcase-grade ten) are unaffected.
