From 1bf5a3b2764111945051c3881dd0ec338484b3a5 Mon Sep 17 00:00:00 2001 From: Wenjie Fan <31087545+gggdttt@users.noreply.github.com> Date: Fri, 17 Jul 2026 14:17:04 +0200 Subject: [PATCH] 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 --- ...-symbol-change-is-not-a-breaking-change.md | 24 +++++++++++++++++++ .../page-boolean-triggers-default-to-true.md | 24 +++++++++++++++++++ ...oth-getrecord-triggers-is-not-redundant.md | 22 +++++++++++++++++ ...ective-filter-may-live-outside-the-diff.md | 22 +++++++++++++++++ ...age-field-inherits-source-field-tooltip.md | 24 +++++++++++++++++++ ...sed-schema-change-needs-no-upgrade-path.md | 24 +++++++++++++++++++ 6 files changed, 140 insertions(+) create mode 100644 microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md create mode 100644 microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md create mode 100644 microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md create mode 100644 microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md create mode 100644 microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md create mode 100644 microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md diff --git a/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md new file mode 100644 index 0000000..fc39c13 --- /dev/null +++ b/microsoft/knowledge/breaking-changes/unreleased-symbol-change-is-not-a-breaking-change.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: breaking-changes +keywords: [released-baseline, unreleased, rename, renumber, obsolete, api-stability, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Changing an unreleased symbol is not a breaking change + +## Description + +Breaking-change rules protect contracts that have already shipped to customers or are exposed to external extensions. A symbol — an object, field, key, enum value, or procedure — that is new in this app, was introduced and then changed within the same still-unreleased development cycle, or belongs to an app that has no released version yet, can be renamed, renumbered, or removed freely. There is no shipped contract to break, so the change is not a breaking change. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. An app whose `app.json` version has no corresponding released baseline (for example a `1.0.0.0` app that has never shipped) has no protected surface. + +## Best Practice + +Before treating a rename, renumber, or removal as breaking, establish that the affected symbol was present in a released baseline. Do not flag changes to symbols that are new in the current unreleased cycle or that belong to an app with no released version. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than assert a break. + +## Anti Pattern + +Reporting a breaking change for a rename, renumber, or removal without confirming the symbol shipped in a released version — for example flagging a break on an app whose `app.json` version has no released baseline. diff --git a/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md new file mode 100644 index 0000000..f637fae --- /dev/null +++ b/microsoft/knowledge/error-handling/page-boolean-triggers-default-to-true.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [oninsertrecord, onmodifyrecord, ondeleterecord, onquerypage, boolean-trigger, exit, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Page record triggers return true by default; a missing exit(true) does not block the operation + +## Description + +The Boolean page record triggers `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, and `OnQueryClosePage` return `true` by default. When the trigger body omits an explicit return value, the platform treats the result as `true` and the operation proceeds. Only an explicit `exit(false)` — or a reachable code path that returns `false` — cancels the insert, modify, delete, or page close. + +This is a defined exception to the ordinary Boolean method rule, where the default return is `false`. Reviewers unfamiliar with the exception sometimes read a page record trigger that has no `exit(true)` and conclude the operation is blocked; it is not. + +## Best Practice + +Do not claim that a missing `exit(true)` blocks or prevents an insert, modify, or delete, and do not recommend adding `exit(true)` "to let the operation proceed" — that is already the default. Evaluate these triggers only for an explicit or reachable `exit(false)`/false-returning path that would cancel the operation unintentionally. + +## Anti Pattern + +Flagging `OnInsertRecord`, `OnModifyRecord`, `OnDeleteRecord`, or `OnQueryClosePage` as defective because it "does not return `true`", or asserting that inserts/modifies/deletes will silently fail without an explicit `exit(true)`. The default return already permits the operation. diff --git a/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md new file mode 100644 index 0000000..79c3b52 --- /dev/null +++ b/microsoft/knowledge/performance/calcfields-in-both-getrecord-triggers-is-not-redundant.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [calcfields, onaftergetrecord, onaftergetcurrrecord, page-lifecycle, flowfield, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# CalcFields in both OnAfterGetRecord and OnAfterGetCurrRecord is not redundant + +## Description + +`OnAfterGetRecord` fires once per row as the page loads records into the view; `OnAfterGetCurrRecord` fires when a record becomes the active/current record. Calling `CalcFields` in both triggers is not duplicate or redundant work: the two triggers run at different points in the page lifecycle and serve different purposes — populating FlowFields for every displayed row versus refreshing them for the record the user has selected. The same `CalcFields` call appearing in both places is an intentional pattern, not copy-paste waste. + +## Best Practice + +Do not flag `CalcFields` appearing in both `OnAfterGetRecord` and `OnAfterGetCurrRecord` as duplicate, redundant, or removable. Treat each trigger's `CalcFields` on its own lifecycle merits. + +## Anti Pattern + +Recommending that a developer delete one of the two `CalcFields` calls because "the field is already calculated in the other trigger". The genuine per-row FlowField cost is addressed by the separate guidance on FlowField calculation in loops and on hidden FlowFields; it is not addressed by removing a lifecycle-correct `CalcFields`. diff --git a/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md new file mode 100644 index 0000000..50eb014 --- /dev/null +++ b/microsoft/knowledge/performance/page-effective-filter-may-live-outside-the-diff.md @@ -0,0 +1,22 @@ +--- +bc-version: [all] +domain: performance +keywords: [filter, drilldown, lookup, sourcetableview, tablerelation, setrange, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page or lookup's effective filter may be defined outside the changed hunk + +## Description + +The effective filter on a drill-down, lookup, or list result set is frequently defined outside any single changed hunk — on the table via a `SourceTableView` property or a `TableRelation`, or through `SetRange`/`SetFilter` calls in unchanged code that runs before the result is shown. The absence of a filter within the changed lines of a diff is therefore not evidence that the result set is unfiltered or that it will load an entire table. + +## Best Practice + +Do not assert that a drill-down, lookup, or list is "unfiltered" based only on the changed hunk. Confirm the effective filter by checking the page's `SourceTableView`, the field's `TableRelation`, and any `SetRange`/`SetFilter` in the surrounding (possibly unchanged) code before raising a finding about an unbounded result set. + +## Anti Pattern + +Concluding that a lookup or drill-down loads an unfiltered, full-table result set solely because no `SetRange`/`SetFilter` appears in the changed lines, when the filter is defined on the table, in a `TableRelation`, or in unchanged setup code. diff --git a/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md new file mode 100644 index 0000000..a258506 --- /dev/null +++ b/microsoft/knowledge/ui/bound-page-field-inherits-source-field-tooltip.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: ui +keywords: [tooltip, page-field, source-field, inheritance, aa0218, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A page field bound to a table field inherits that field's ToolTip + +## Description + +A page field bound to a table field inherits the source field's `ToolTip` at runtime: the control shows the table field's `ToolTip` even when the page control declares none of its own. A page field without an inline `ToolTip` is therefore not, by itself, a missing-tooltip defect — the text may be supplied by the bound source field. + +The genuinely-missing case — a bound field whose source table field also carries no `ToolTip`, or an unbound control that needs one — is already reported by the compiler analyzer AA0218, which BCQuality calibrates to `info`. That analyzer, not an agent finding, owns the missing-tooltip signal. + +## Best Practice + +Do not raise a missing-`ToolTip` finding for a page field that has a source-table binding; assume the source field supplies the tooltip. Reserve tooltip findings for the cases the dedicated tooltip rules define, and let analyzer AA0218 carry the mechanically-detectable missing-tooltip case at its calibrated severity. + +## Anti Pattern + +Flagging every page field that has no inline `ToolTip` property as an accessibility violation, ignoring that a bound field inherits its source field's tooltip and that AA0218 already covers the truly-missing case. diff --git a/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md new file mode 100644 index 0000000..248943f --- /dev/null +++ b/microsoft/knowledge/upgrade/unreleased-schema-change-needs-no-upgrade-path.md @@ -0,0 +1,24 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [released-baseline, unreleased, schema, migration, obsolete, data-loss, false-positive] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Unreleased schema changes need no upgrade or migration path + +## Description + +Upgrade and migration findings protect data and schema that have already shipped to customers. A schema element — a table, field, key, or enum — that is new in this app, or was added and then changed within the same still-unreleased development cycle, needs no upgrade code or migration path: no customer has data in it yet, so there is nothing to preserve or migrate. Such a change is not an obsoletion, data-loss, or breaking-migration defect. + +Release status is established from the diff, the app's `app.json` version, or a released baseline. A schema element with no released baseline has no persisted customer data to protect. + +## Best Practice + +Before asserting an obsoletion, data-loss, or breaking-migration defect, establish that the affected table, field, key, or enum existed in a released version. Do not require upgrade or migration code for schema that never shipped. When release status cannot be established from the diff, `app.json`, or a released baseline, omit the finding rather than demand a migration path. + +## Anti Pattern + +Demanding an upgrade codeunit, migration path, or data-preservation step, or flagging data loss, for a table, field, key, or enum that is new in the current unreleased cycle and has no released baseline.