Contributed by CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Each article follows the knowledge file format (frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples).
* Add Finance posting domain-knowledge pilot
Adds three atomic domain-rule knowledge files under community/knowledge/finance/ as a pilot for Type-A (normative) Business Central domain knowledge: post through the posting engine, treat posted ledger entries as immutable, and treat the Dimension Set ID as the source of truth for dimensions. Includes a good/bad AL sample pair for the posting rule.
These encode BC-specific invariants that LLMs reliably get wrong, fitting the existing remedial/atomic knowledge grain with no schema or contract changes. Passes the repo frontmatter validator and is discovered by the knowledge index.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Clarify editable operational fields on posted ledger entries
Addresses review feedback from @JeremyVyska on PR #57: the immutability rule applies to financial content, not the whole entry. Reframes the Description around financial content and gives the operational-field exception (payment/application data, on-hold, applies-to, communication fields edited via CustEntry-Edit/VendEntry-Edit and the ledger entry pages) its own paragraph in Best Practice instead of understating it as a narrow set.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* Expand Finance pilot into a source-verified review domain
Move Finance knowledge to the Microsoft-owned layer, add nine scoped rules with eighteen AL samples, and register bounded Finance review with complete paired evaluation coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Fix Finance review applicability and ownership boundaries
Remove application-area gating and later VAT-field dependencies, align dynamic shared conventions, separate SCM ownership, and keep journal examples focused on the intended invariant.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Remove logo branding (#194)
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add title and description to README
* Add foundational AL developer knowledge (#195)
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Clarify locale-safe DateFormula Evaluate inputs (#193)
* Clarify locale-safe DateFormula Evaluate inputs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Normalize DateFormula article sections
Keep the analyzer-gap explanation in Description and its scoped probe evidence in References, without a novel Validation section. Normative guidance and fixtures are unchanged.
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 App <223556219+Copilot@users.noreply.github.com>
* Add SCM functional knowledge domain (#192)
* Add SCM functional knowledge domain
Introduce nine source-backed rules with original AL sample pairs, bounded SCM review routing, and complete positive/clean evaluation coverage.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Normalize SCM knowledge and review ownership
Align article and AL sample conventions, keep BC facts separate from review mechanics, and clarify reciprocal Finance ownership without bespoke shared test assertions.
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 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>
Use [\slug.good.al\](slug.good.al) form so tools/Knowledge-Retrieval.ps1
Assert-SampleLink validation passes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New remedial article for spotting when AL calls an endpoint that does not
authenticate itself to the client (bare HttpClient.Get, blank SOAP SecretText,
post-DisableHttpsCheck HTTP) and requires the response to be size-, schema-, and
request/response-integrity-validated before it is trusted. Includes the
BC-specific false-positive clarifications (platform buffers the full body, so an
in-AL size check after buffering is correct; no DNS-rebinding/bounded-read demand;
HTTPS not always enforceable) plus good/bad AL samples.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* knowledge(events): ChangeCompany leaves triggers and trigger-event subscribers running in the calling company
ChangeCompany redirects only the data access of a record variable; Learn states that triggers still run in the current company. The database trigger events are raised on every database operation and only pass RunTrigger to the subscriber, so Insert(false) after ChangeCompany still runs every subscriber in the calling company. Generated code either assumes the record 'becomes' a target-company record, or switches RunTrigger off and hand-copies the trigger logic, leaving the subscribers writing to the wrong company; none of the tested runs reached StartSession with the company parameter.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
* knowledge(events): qualify StartSession async semantics and fix concurrency-unsafe sample key
Addresses PR #152 review: StartSession is a fire-and-forget background
session (Ok reports only whether it started, not whether the codeunit
succeeded, and errors inside it do not propagate), so the Best Practice
now scopes the recommendation and calls out the durable status/error
channel a synchronous-success write needs. The good sample's
FindLast()+1 entry-number pattern raced under concurrent background
sessions; switched to AutoIncrement, which the platform guarantees is
unique across concurrent transactions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* knowledge(events): serialize the setup-counter increment; promote article and wire the review skill
PR #152 round 3 (JesperSchulz):
- The good sample's OnAfterInsertEvent subscriber still raced on the
shared "Transfer Setup Good" singleton (Get/increment/Modify);
AutoIncrement only protected the request key. Added
TransferSetup.LockTable() before Get() to serialize concurrent
background sessions.
- Promoted changecompany-runs-triggers-in-the-calling-company from
community/knowledge/events/ to microsoft/knowledge/events/, and wired
ChangeCompany/StartSession/RunTrigger tokens plus a targeted
detection cue into microsoft/skills/review/al-events-review.md so a
diff containing the anti-pattern reliably worklists this article,
preserving the documented RunTrigger=false hand-off exception.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: waldo1001 <12088142+waldo1001@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Generate a deterministic action-skill index from frontmatter, publish structural schemas for orchestration and findings, and validate flat review composition in CI.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: dayland <dayland@microsoft.com>
Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
Use the required READ-convention Markdown links so knowledge retrieval can associate all new samples with their articles.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: dayland <dayland@microsoft.com>
Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
* Add bounded knowledge retrieval
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
* Fix bc-version overflow and pathless-row identity in bounded retrieval
Catalog matching compared an Int32 -BCVersion against a bigint range bound.
PowerShell coerces the right operand to the left operand's type, so a bound
wider than Int32 threw a conversion error and failed the whole domain catalog
rather than the single row. Metadata validation already accepts such bounds,
so compare as bigint on both sides.
The shared pager built its oversized-row message with $row.path, which
throws under Set-StrictMode -Version Latest when a row carries no path,
replacing the explicit bound failure with a property-lookup error. Resolve the
path defensively for dictionary and object rows so the offset-based fallback
is reachable.
Both paths gain regression coverage that fails without these fixes.
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 App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0d8b7764-f15a-49ea-8d50-d9147334f8af
Retire deterministic compiler and analyzer duplicates, remove their review routing, and clarify the admission test for contextual analyzer knowledge.
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Document standalone review runner contract
Keep model selection and scheduling outside BCQuality while allowing orchestrators to run isolated review leaves concurrently with deterministic rollup semantics.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Support complete app folder reviews
Define folder-path as a current-state review scope and accept it across the standalone adapter, broad coordinator, and every AL review leaf.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Add walk-up app review quick start
Put the complete app-folder installation and prompt flow directly in the README so partners can discover the standalone experience without reading integration details first.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Organize conceptual guides under docs
Move architecture and standalone runner documentation out of the repository root, add a documentation index, and update all inbound links.
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 App <223556219+Copilot@users.noreply.github.com>
Move canonical knowledge for Microsoft-owned review domains into the Microsoft layer and document the skill/knowledge co-location policy.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Copilot-Session: 2a6ea875-d38e-4f30-aadb-0d606f9be231
* Avoid Public Event publisher
* knowledge(events): scope public-publisher detection to same-app raisers
The detection rule flagged every public event publisher, including ones
deliberately public so a sibling app can raise them - a contract `internal`
cannot express across app boundaries. Scope the finding to publishers that
are public although only their own app raises them, and record the
cross-app case as a valid Best Practice option.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 0b67b90d-e4b4-4b92-9684-726c72c43b3f
* knowledge(performance): align SetLoadFields placement with AL Guidelines (#120)
The AL Guidelines mark `SetLoadFields` placed before `SetRange`/`SetFilter`
as bad code and recommend filters first, while the BCQuality samples used
the opposite order — contradictory guidance across two Microsoft repos.
Per Learn (`Record.SetLoadFields`), "fields that are filtered upon are
always loaded", so the two orders produce an identical projection. The
upstream rule is a readability convention: keep `SetLoadFields` adjacent
to the read it governs.
- Reorder filters ahead of `SetLoadFields` in the six affected AL samples.
- State the placement convention in the Best Practice section.
- Record in Description that order does not change the projection, and
that only a fieldless `SetLoadFields()` or a later overwriting call does.
- Add an Anti Pattern note so review agents treat the reverse order as a
readability observation, never a performance defect.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
* Clarify partial-record projection changes
Document AddLoadFields, SetBaseLoadFields, and Reset alongside SetLoadFields so the statement-order guidance does not imply those APIs leave the projection unchanged.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 89dba8c8-6529-4b60-956f-875a59be499d
---------
Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 89dba8c8-6529-4b60-956f-875a59be499d
The narrowed UI-handler guidance still stated the execution rule without the
qualifier the linked Microsoft reference uses. The article said every listed
handler must execute at least once, and the testing leaf skill asked for
`[HandlerFunctions(...)]` to match the invoked handlers exactly. The reference
says every *nonoptional* listed handler must execute, and that send-notification
and recall-notification handlers can be optional. As written, an agent could
flag a deliberately unused optional notification handler.
The discriminator is narrower than the handler type. Both
`[SendNotificationHandler([HandlerIsOptional: Boolean])]` and
`[RecallNotificationHandler([HandlerIsOptional: Boolean])]` take an explicit
optionality argument, so `[SendNotificationHandler(true)]` is exempt while the
same attribute written without the argument stays nonoptional like every other
handler type. Keying the exemption on the argument rather than the type keeps it
checkable from the diff and avoids the opposite false positive, where an agent
stops flagging genuinely nonoptional notification handlers.
Changes:
- The article now states the nonoptional qualifier, explains that optionality is
declared rather than inferred, and adds an explicit do-not-flag clause. That
clause also forbids proposing removal, because the listed entry is what keeps
the test passing on the runs where the notification does fire.
- The testing leaf skill carries the same boundary in its `ui-handlers-in-tests`
cue, and its mechanical-fix list no longer allows removing a listed optional
notification handler as a one-click suggestion.
- `SendNotificationHandler` and `RecallNotificationHandler` were missing from the
skill's testing token list, so notification handlers were not reliably
surfaced to the relevance step at all. Both are now listed.
- The good sample gains a test that lists an unreached
`[SendNotificationHandler(true)]`; the bad sample gains the mirror image, an
unreached `[SendNotificationHandler]` with no optionality argument. The pair
differs only by that argument, which is the point.
- `evaluation/review-fixtures.json` pins the testing domain to
`ui-handlers-in-tests` so the boundary is exercised: the good sample is the
clean control at `minimumCleanRate` 1.0 and the bad sample is the expected
finding. Keywords were retagged with `notification` and `optional-handler`.
validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with
32 cases across 16 leaf domains and resolves the testing fixture to this article.
data-modeling: add insert-only-transfer-may-rely-on-caller-cleanup. A filter-and-insert transfer routine was reported for stale rows and duplicate keys even though the field OnValidate trigger calls a sibling cleanup procedure that clears the same range immediately before it. Deciding this requires reading the caller, so the article asks reviewers to trace call sites and keeps uncleared or mismatched-filter paths reportable.
appsource: scope two-level-namespace-replaces-object-affix-not-extension-member-affix to apps that actually configure a mandatory affix. AS0011 only runs when AppSourceCop is enabled with a mandatory affix; a first-party in-box app that ships no such configuration is not subject to it. The member-affix requirement itself is unchanged for apps that do configure one.
testing: allow permission-tests-must-lower-the-execution-context to accept a composed role. The article demanded the exact permission set under test be assigned directly, so a test that lowered permissions through a role including that set and then asserted WritePermission was false was reported as a coverage gap. What matters is the effective context plus a boundary assertion, not which object the test names.
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
The article was rewritten to say a reset is required only when the value can
carry over, and its H1 was updated to match, but three artefacts still carried
the old "always initialize to false" premise:
- The slug still read `initialize-ishandled-to-false-before-publishing`, which
contradicts the body. The slug is not cosmetic: Build-KnowledgeIndex.ps1
ranks candidates on keywords, frontmatter dimensions, domain, path and title,
so a stale path pushes selection back toward the behaviour this change
narrows. Renamed to `reset-ishandled-only-when-the-value-can-carry-over`,
following the existing precedent for conditional slugs such as
`unreleased-symbol-change-is-not-a-breaking-change`.
- Keywords still listed `initialization` and `deterministic` and omitted
`false-positive`, the tag this repository uses for suppression articles.
Replaced with `carry-over` and `loop-iteration` and added `false-positive`.
- The good sample demonstrated only the "prefer separate fresh locals" clause
and contained no reset at all, so the article's headline case had no positive
example. It was also asymmetric with the bad sample, which gained a loop
procedure showing a local that carries `true` into the next iteration.
Added the matching loop procedure to the good sample: a local declared outside
the loop is reset at the top of each iteration. That case cannot be solved by
introducing another local, because AL has no block scope, so it is the only
shape that demonstrates the reset the article still requires. It also gives the
engine the correct `suggested-code` shape for the loop finding; without it the
one-click fix adapted from the good sample would propose splitting the variable
rather than adding one line.
Also renamed the sample codeunits from "IsHandled Init ..." to
"IsHandled Carry Over ...", and updated the two references to the old slug:
the events leaf skill cue and the events pin in evaluation/review-fixtures.json.
validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with
32 cases across 16 leaf domains and resolves the events fixture to the renamed
article.
* knowledge(breaking-changes): exclude appended event parameters from the signature-change rule
The article's detection guidance flags 'a parameter added' on any shipped procedure. Applied to an event publisher that is bound to rather than called, this produced a false positive on BCApps PR 10278, where a trailing var parameter was appended to the existing IntegrationEvent OnAfterOpenForRecRef and the developer twice replied that adding a parameter to an existing integration event is not a breaking change.
It also contradicted events/add-new-event-parameters-at-the-end, which already states that existing subscribers still bind to the leading parameters. Scope the rule to called procedures, carve out appended event parameters as additive, and keep every other event signature edit - removal, reorder, retype, var flip - in scope. Point the IsHandled case at events/do-not-add-ishandled-to-an-existing-event, which owns that semantic concern.
* fix: event parameter additions are additive at any position, not only when appended
The first revision justified the carve-out with leading-prefix binding and limited it to parameters appended at the end. Verified against shipping BCApps code that AL binds subscriber parameters by name, not position, so an added parameter is additive wherever it is placed. Also corrects the cross-reference to events/adding-a-parameter-to-an-event-is-not-a-breaking-change, which already states this rule, and drops reordering from the list of edits that break binding.
* Route event signature edits to the analyzer-backed events article
Address review feedback: name AS0025, AS0063 and AS0077 for the var and rename cases instead of claiming them in the breaking-changes article, and point at events/treat-local-and-internal-events-as-subscriber-contracts which already owns them.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
* knowledge(style): allow event subscribers to omit trailing publisher parameters
The article told reviewers to copy the publisher signature exactly and reproduce every parameter verbatim. That contradicts events/add-new-event-parameters-at-the-end, which already states that existing subscribers bind to the leading parameters, and it produced a false positive on BCApps PR 10277 where a subscriber legitimately declared only the leading two of the publisher's three parameters.
Clarify that the name-match rule applies to every parameter the subscriber declares, state that AL binds on a leading prefix so trailing parameters may be omitted, and keep the real defect - a subscriber list that is not a prefix of the publisher's - as the anti pattern. Add the false-positive keyword for retrieval.
* fix: subscribers bind by parameter name, not by leading prefix
The first revision claimed AL binds a subscriber to a leading prefix of the publisher parameter list and that a parameter may not be skipped in the middle. That is wrong. Verified against shipping BCApps code: Test Runner - Mgt publishes OnBeforeTestMethodRun(var CurrentTestMethodLine; CodeunitID; CodeunitName; FunctionName; FunctionTestPermissions; var Skip), and ALTestRunnerResetEnvironment binds to it declaring (CodeunitID; CodeunitName; FunctionName; FunctionTestPermissions; var CurrentTestMethodLine) - omitting Skip and moving the first parameter to last. Binding is by name, so any subset in any order is valid. Detection now targets a parameter whose name or type matches nothing on the publisher.
---------
Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
The requirement to declare DataClassification explicitly in a tableextension
applies to the Normal fields it adds; FlowFields and FlowFilters are
SystemMetadata automatically and are covered by their own article. Being
added by a table extension is also not itself a finding - the finding is a
Normal field added by a table extension that has no valid explicit
DataClassification.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Table-level DataClassification is the effective default only for Normal
fields declared inside that table object. A tableextension cannot set the
property (AL0246) and its added fields do not inherit the base table value,
so AS0016 still requires each of them to classify itself. State this in both
privacy articles so the guidance cannot suppress genuine findings on the
tableextension pattern, which is how most partner code adds fields.
Also narrow the inheritance claim to verified AppSourceCop behaviour rather
than asserting platform-level resolution, and make the sample's table-level
default semantically representative of its fields while keeping a legitimate
field-level override and demonstrating the tableextension boundary.
Verified with alc.exe 18.0.37.11445 + Microsoft.Dynamics.Nav.AppSourceCop.dll:
the revised sample produces no AS0016, and removing the explicit
classification from the tableextension field makes AS0016 fire.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Document that valid table-level classifications are inherited by fields and update the privacy fixture and related guidance accordingly.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Two knowledge false-positive guards addressing bug 642303 (agent FPs on BCApps PR #9607):
- breaking-changes: relocating a field to a tableextension in the same app under the same field ID/name is a relocation, not a deletion/rename; the field still resolves on the table, so it must not be flagged as a deleted shipped field or require ObsoleteState staging. Scoped to the contract axis; silent on data migration.
- events: adding a parameter to an event publisher does not break existing subscribers (subscribers bind by name and match a subset), so the addition itself must not be reported as a breaking signature change.
- upgrade/obsoletereason-need-not-restate-removal-version: ObsoleteTag carries the version; do not flag ObsoleteReason for omitting it (PR 8290)
- error-handling/unchecked-get-throws-when-record-not-found: a bare Rec.Get() errors on missing record; it is not silently ignored (PR 8584)
- performance/onaftergetcurrrecord-is-not-per-row: OnAfterGetCurrRecord fires on selection change, not per row; CalcFields there is not N+1 (PR 8617)
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>
