Commit graph

93 commits

Author SHA1 Message Date
demiliani
a300675320 Merge branch 'main' of https://github.com/demiliani/BCQuality into appsource
# Conflicts:
#	microsoft/skills/review/al-appsource-review.md
2026-09-11 16:27:29 +02:00
Jesper Schulz-Wedde
35d0966a8d
Normalize recoverable leaf finding ranges (#180)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
* Normalize recoverable leaf ranges

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Run contract checks with review fixtures

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>
2026-09-11 15:24:20 +02:00
Jesper Schulz-Wedde
51597068b1
Add bounded knowledge retrieval (#179)
* 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
2026-09-11 12:35:31 +02:00
Jesper Schulz-Wedde
c12b2f0a88
Separate analyzer rules from BCQuality knowledge (#178)
Some checks are pending
Validate knowledge index / validate-index (push) Waiting to run
Validate AL review fixtures / validate-review-fixtures (push) Waiting to run
Validate frontmatter and structure / validate (push) Waiting to run
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>
2026-09-11 09:26:14 +02:00
Jesper Schulz-Wedde
ac9e4fd9a2
Complete partner contribution and knowledge consumption guides (#176)
* Improve partner onboarding and documentation navigation

Lead with a complete plugin quick start and add task-oriented usage, troubleshooting, customization, and contribution guides. Preserve the broader plugin framing, correct conflicting contract guidance, support Agents folder reviews, and align repository validation. Convert existing sample references to clickable links without changing knowledge rules.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Complete partner contribution and knowledge consumption guides

Explain direct reading, supplied skills, and custom-agent consumption. Add a first-contribution walkthrough and concrete integration bootstrap, and clarify SetLoadFields guidance with authoritative sources and explicit review heuristics.

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>
2026-09-11 09:09:53 +02:00
Jesper Schulz-Wedde
2b5550c346
Improve partner onboarding and documentation navigation (#174)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
Lead with a complete plugin quick start and add task-oriented usage, troubleshooting, customization, and contribution guides. Preserve the broader plugin framing, correct conflicting contract guidance, support Agents folder reviews, and align repository validation. Convert existing sample references to clickable links without changing knowledge rules.

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2026-09-09 17:31:03 +02:00
Jesper Schulz-Wedde
17bb84a25e
Support standalone runners and complete app-folder reviews (#172)
* 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>
2026-09-09 16:55:35 +02:00
demiliani
e451e92f5d Address Marketplace review feedback 2026-09-07 17:50:59 +02:00
Jesper Schulz-Wedde
8584217c75
Clarify page field caption and tooltip inheritance (#160)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
* Clarify page field caption and tooltip inheritance

Prevent redundant page-level properties by documenting inherited captions and BC24/runtime 13.0 table-field tooltips. Correct companion examples and version-scoped tooltip guidance.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* Address tooltip quality and knowledge scope review feedback

Require useful, behavior-grounded tooltip text rather than caption repetition, improve the samples, and explain why compiler feedback does not prevent redundant page captions.

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>
2026-09-07 15:13:35 +02:00
demiliani
2ec109e534 Merge branch 'main' of https://github.com/demiliani/BCQuality into appsource 2026-09-07 14:28:12 +02:00
Wenjie Fan
1a5afdc0eb
Merge pull request #132 from microsoft/gggdttt-refine-self-improvement-guidance
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
Refine self-improvement review guidance
2026-09-03 15:42:39 +02:00
Jesper Schulz-Wedde
4f0a13a801
Promote knowledge for Microsoft review skills (#153)
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
2026-09-03 15:06:01 +02:00
Kilian Seizinger
82422f94c9
Avoid Public Event publisher (#144)
* 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
2026-09-02 16:07:18 +02:00
Jesper Schulz-Wedde
3e848d1ec2
knowledge(performance): align SetLoadFields placement with AL Guidelines (#130)
* 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
2026-09-02 14:44:09 +02:00
wenjiefan
c213f1495e Exempt optional notification handlers from the HandlerFunctions execution rule
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.
2026-09-02 11:46:13 +02:00
Wenjie Fan
35a7e72f12
knowledge: three false-positive guards from BCApps PR 10277, 10278 and 10346 (#146)
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>
2026-09-02 11:25:19 +02:00
Wael
f027e28f83
knowledge: improve review precision from BCApps PR 10080 feedback (#128)
* knowledge: improve review precision from BCApps PR 10080 feedback

* Update microsoft/knowledge/appsource/object-affixes-prevent-collisions.md

Co-authored-by: Natalie Karolak, MVP <34504100+NKarolak@users.noreply.github.com>

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jesper Schulz-Wedde <JesperSchulz@users.noreply.github.com>
Co-authored-by: Natalie Karolak, MVP <34504100+NKarolak@users.noreply.github.com>
2026-09-02 11:16:30 +02:00
wenjiefan
5016962b40 Align the IsHandled article slug, keywords and good sample with its narrowed scope
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.
2026-09-02 11:08:16 +02:00
Wenjie Fan
00c9307483
knowledge(breaking-changes): adding a parameter to an event publisher is not a signature break (#139)
* 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>
2026-09-02 09:24:54 +02:00
Wenjie Fan
85ceaaa1a4
knowledge(style): event subscribers bind by parameter name, so a shorter list is not a mismatch (#138)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
* 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>
2026-08-28 11:07:05 +02:00
Stefano Demiliani
e9f870a0f8 knowledge(appsource): add AL validation guidance 2026-08-27 17:19:34 +02:00
Djordje Cenic
6e321d3c56 Correct severe misconception about SetCurrentKey in the knowledge base 2026-08-22 12:04:00 +02:00
wenjiefan
ead337f9cb Address review guidance feedback
Preserve independent event seams, cover loop-carried handled state, strengthen checkpoint and UI-handler fixtures, and align DeleteAll fallback guidance.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 10646e50-2d8b-4cca-b02b-dfa78629e6a1
2026-08-18 14:09:11 +02:00
wenjiefan
5f1cff2fb6 Refine self-improvement review guidance
Narrow IsHandled, label-scope, UI-handler, checkpoint, and bulk-operation guidance to evidence-backed false-positive boundaries.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2026-08-18 11:17:14 +02:00
wenjiefan
c1057d38b2 Scope tableextension requirement to Normal fields lacking a classification
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>
2026-08-17 15:25:14 +02:00
wenjiefan
f8acb6cbdd Scope DataClassification inheritance to fields declared in the table
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>
2026-08-17 15:14:53 +02:00
wenjiefan
a668920e96 Correct table-level data classification guidance
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>
2026-08-17 13:16:12 +02:00
wenjiefan
6d1fada5a4 Add FP guards for field relocation to tableextension and event parameter addition
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.
2026-08-04 10:41:03 +02:00
wenjiefan
2b401aa38e Add batch-2 FP guards: sentence-case action captions + primary-key Get is transaction-cached 2026-07-21 11:08:04 +02:00
wenjiefan
c618071ea6 knowledge: add 3 FP-suppression guards from BC apps negative feedback
- 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)
2026-07-21 09:45:09 +02:00
wenjiefan
89bf8fde31 tooltip: PR review flags genuinely-missing tooltips instead of deferring to AA0218
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.
2026-07-20 14:30:29 +02:00
Wenjie Fan
712dee9ec1
style-review: calibrate variable-declaration order (AA0021) to info (#109)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
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>
2026-07-17 14:17:32 +02:00
Wenjie Fan
1bf5a3b276
Add precision guards for systematic agent false-positive patterns (#112)
* 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>
2026-07-17 14:17:04 +02:00
wenjiefan
cdd3d99cf8 review: stop emitting reference-less agent findings in privacy and UI/accessibility leaves
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: [].
2026-07-15 14:57:17 +02:00
Jesper Schulz-Wedde
186d8a1314
Complete AL review knowledge readiness (#108)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate AL review fixtures / validate-review-fixtures (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
* 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>
2026-07-15 10:55:25 +02:00
Jesper Schulz-Wedde
ae04938c03
Emit human-readable domain label on review findings (#54)
* 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>
2026-07-15 10:44:50 +02:00
Wenjie Fan
809af9708e
Privacy DataClassification fixes + keep Label-scope findings at minor (#102)
Some checks failed
Validate knowledge index / validate-index (push) Has been cancelled
Validate frontmatter and structure / validate (push) Has been cancelled
* Fix DataClassification default fact and broaden classification taxonomy

* Keep Label-scope findings at minor; no analyzer enforces label scope

---------

Co-authored-by: wenjiefan <wenjiefan@microsoft.com>
2026-07-14 14:23:03 +02:00
Jesper Schulz-Wedde
3d29c172a9
Promote validated community knowledge (#105)
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>
2026-07-14 14:20:27 +02:00
Wenjie Fan
be1b92b624
style-review: fix example finding id to match references[0].path (#101)
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>
2026-07-14 12:59:25 +02:00
Jesper Schulz-Wedde
0bb1065bc3
Add P0 integration and control add-in runtime guidance (#100)
* Add P0 integration and control add-in guidance

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 02baffe8-0600-430d-81fa-a9993685e7cb

* Correct API part multiplicity guidance

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 02baffe8-0600-430d-81fa-a9993685e7cb

* Refine API part multiplicity guidance

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 1c37924e-9749-4e63-9d58-bd73d659f736

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 12:53:26 +02:00
Jesper Schulz-Wedde
e0ebdd35c7
Add lifecycle error and privacy knowledge (#99)
* Add lifecycle error and privacy knowledge

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec

* Fix lifecycle privacy review findings

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a26cd6d6-ff49-433e-bc53-f645c455ebdd

* Refine lifecycle privacy retrieval

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec

* Make review gates explicit

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 95c06ad8-377d-4faa-8d07-06300b1c81ec

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2026-07-14 12:52:56 +02:00
Jesper Schulz-Wedde
363f08f47e
Add P0 event and interface compatibility knowledge (#98)
* Add P0 extensibility compatibility knowledge

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 645349fd-1892-48f3-8a84-db77d6abd1c3

* Correct event compatibility guidance

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 645349fd-1892-48f3-8a84-db77d6abd1c3

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 12:51:59 +02:00
Jesper Schulz-Wedde
078b869e33
Add per-row AL performance guidance (#97)
* Add per-row performance guidance

Document SetAutoCalcFields for per-row FlowFields and direct writes on iterated records, with focused reviewer retrieval cues.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea

* Bound commit checkpoints by key range

Use a capped ordered query to discover each checkpoint watermark before locking and processing only that key range.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea

* Address performance retrieval review

Retrieve Commit-in-loop guidance precisely, process exact checkpoint key lists, and narrow clone-before-write discovery.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 85be3fc4-5253-47b8-ba1b-6b8fd188fcea

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2026-07-14 12:51:39 +02:00
Jesper Schulz-Wedde
98af9aa1fc
Add missing AL review leaf skills (#96)
* Add missing AL review leaves

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 62d512a7-fd54-43dc-8eb5-485b909c72e5

* Refine test isolation review cue

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 62d512a7-fd54-43dc-8eb5-485b909c72e5

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 12:50:30 +02:00
Wenjie Fan
9214f73819
style-review: calibrate analyzer-redundant rules to info; keep correctness bugs out of style scope (#95)
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>
2026-07-14 11:26:43 +02:00
Jesper Schulz-Wedde
0e06485027
Correct performance knowledge guidance (#94)
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 667c64a8-eb36-4440-bc41-6a97d8fb5542

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 11:26:29 +02:00
Jesper Schulz-Wedde
5706959e4a
Fix lifecycle compatibility guidance (#93)
* Fix lifecycle compatibility guidance

Correct high-confidence Business Central guidance and samples for upgrade tags, collectible errors, trigger semantics, obsoletion, events, interfaces, API contracts, and test transactions.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: e05a43e7-6448-4d67-9c73-798523f5d945

* Address guidance review findings

Gate SecretText guidance to BC23 and clarify that the collectible-error sample intentionally emits a message-only blocking aggregate.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: e05a43e7-6448-4d67-9c73-798523f5d945

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 11:26:16 +02:00
Jesper Schulz-Wedde
aca3986fd0
Correct security and privacy knowledge guidance (#92)
* Correct security and privacy guidance

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 9c2eebc4-dcd5-4b85-8113-90772d818900

* Address security privacy review findings

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 9c2eebc4-dcd5-4b85-8113-90772d818900

---------

Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com>
2026-07-14 11:25:03 +02:00
Jesper Schulz-Wedde
bfda67a95a
Promote security knowledge from community to Microsoft layer (#49)
* 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>
2026-07-14 11:23:57 +02:00
Jesper Schulz-Wedde
766046b85d
Add data-modeling and appsource knowledge articles (MICROSOFT layer) (#65)
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>
2026-07-13 10:35:31 +02:00