Refine self-improvement review guidance - #132
Conversation
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>
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
|
Gentle ping — this has been open since 18 August with all checks green and no review yet. Context on why it is shaped the way it is, in case the size is what is holding it up. The substance is five knowledge articles, corrected after adjudicating false positives from the PR-review self-improvement runs on BCApps 9067, 7938 and 8553:
The eight If the |
…arrowed 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.
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
One blocking false-positive boundary remains in the UI-handler guidance.
microsoft/knowledge/testing/ui-handlers-in-tests.md now says every listed handler must execute, and microsoft/skills/review/al-testing-review.md requires [HandlerFunctions] to exactly match invoked handlers. The linked Microsoft documentation is narrower: every nonoptional handler must execute, and it explicitly permits send-notification and recall-notification handlers to be optional. As written, agents can flag or remove a deliberately unused optional notification handler.
Please qualify the article's two-sided-contract wording and the review cue to exempt optional SendNotificationHandler/RecallNotificationHandler entries (and ideally encode that boundary in the fixture). The rest of the event-state, Label-scope, checkpoint, bulk-operation, and semantic UI-verification changes look sound.
…tion 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.
|
Thanks, you caught a real bug. Fixed in c213f14. You were right that the article was too strict, but the rule turned out to be a bit narrower than "notification handlers are exempt", so I want to explain what I did. Optional is something you declare, not something the handler type gives you. The attribute takes a boolean: The docs never say what the default is when you leave the argument out, so I counted the real usage in
Ten people wrote The article had a second way to produce this false positive. I built a small synthetic PR with a
Both baseline runs flagged the handler, but not for the reason you expected. Neither one complained that it never executed. They complained it had no enqueue/dequeue and no Worth noting: in both candidate runs the testing leaf still loaded and evaluated the article (worklist=2, evaluated=2) and returned nothing. So it is suppressed on purpose, not just missed during retrieval. The part that worried me more was in the skill, not the article. Fixture. One thing I should flag: |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
The update resolves the optional-handler false positive precisely.
- Optionality is keyed to the explicit
HandlerIsOptionalargument, so[SendNotificationHandler(true)]and[RecallNotificationHandler(true)]may remain listed without executing, while the plain nonoptional forms still must execute. - The reviewer no longer proposes deleting optional notification handlers.
- Relevance tokens and the paired good/bad fixture now exercise the exact boundary.
The rest of the previously reviewed guidance remains sound.
|
|
||
| A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event exactly once per procedure invocation is already deterministic. Initialization does not repeat for each loop iteration: a local declared outside a loop can carry `true` from one iteration to the next even when the source contains only one textual event raise. Outside a loop, reaching a later raise after `if IsHandled then exit;` also proves the value is `false`, provided that early exit is semantically correct and does not skip required downstream events. | ||
|
|
||
| ## Best Practice |
There was a problem hiding this comment.
IsHandled is bad in general. You can check with Marko Aleksandric to update these rules. I would state DO NOT INTRODUCE IsHandled :)
| ## Description | ||
|
|
||
| Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows. | ||
| Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). |
There was a problem hiding this comment.
Actually this can be a pattern. We commit ever x rows, or every x minutes. We need to ensure that the loop is possible to be entered again.
| // This still opened a FindSet over the complete remaining tail; | ||
| // periodic commits do not turn retrieval into bounded TOP X. | ||
| if ProcessedCount mod 500 = 0 then | ||
| Commit(); |
There was a problem hiding this comment.
Shouldn't we memorize where we have stopped somehow? LastCustomerNo is not tracked anywhere. If this one fails, we will go through the list all over again.
There should be a sample with time too.
| Commit(); | ||
|
|
||
| // LastCustomerNo exists only in memory, so a retry cannot exclude | ||
| // work that was already committed. |
There was a problem hiding this comment.
This one is OK, we only waste time. It is safe to Rerun, there is no partial commits etc...
| end; | ||
| LastCustomerNo := NormalizeState."Last Customer No."; | ||
|
|
||
| while NormalizeNextChunk(LastCustomerNo) do begin |
There was a problem hiding this comment.
It would be good to have the bad sample and good sample at the same time that is using the same code.
| // reply the test enqueued for it. | ||
| Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question); | ||
| Reply := LibraryVariableStorage.DequeueBoolean(); | ||
| CapturedCustomerNo := CustomerCard."No.".Value(); |
There was a problem hiding this comment.
We should use library variable storage here. First to ensure that the ConfirmHandler is expected.
Then to dequeue the reply and store the value. This way we ensure that we do not dismiss the dialogs blindly, that we are getting exact number of dialogs as expected.
| Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); | ||
| end; | ||
|
|
||
| [ConfirmHandler] |
There was a problem hiding this comment.
Ideally we should get the confirm handler back to have good and bad scenarios aligned
| ## Best Practice | ||
|
|
||
| Make the test own the expectations and the handlers consume them. Before acting, the test `Enqueue`s — in interaction order — the expected text (a stable substring) and any reply each handler must return. The handler `Dequeue`s the expected text, verifies it with the purpose-built asserts (`Assert.ExpectedMessage`, `Assert.ExpectedConfirm`, `Assert.ExpectedStrMenu` — which match on a fragment, not the full localized caption), then `Dequeue`s and returns its reply. Finish the test body with `LibraryVariableStorage.AssertEmpty` to prove every enqueued interaction fired exactly once, and start each test with an `Initialize` that calls `LibraryVariableStorage.Clear` so a value leaked by an earlier test cannot cascade. List in `[HandlerFunctions]` precisely the handlers the scenario triggers — no superset "just in case", no subset that happens to work today. | ||
| List the handlers the scenario triggers, keep an optional notification handler listed for a notification the scenario may conditionally raise, and make each executed handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. |
There was a problem hiding this comment.
The text about using library variable storrage should be listed.
There should be a strong recommendation to have one handler of a given type if possible
| The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. | ||
|
|
||
| - `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them — `initialize-ishandled-to-false-before-publishing`. | ||
| - An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `reset-ishandled-only-when-the-value-can-carry-over`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. |
There was a problem hiding this comment.
Is handled is bad, sync with Marko
4 participants
Summary
Refine the second self-improvement batch into evidence-backed BCQuality guidance rather than copying reaction-generated changes verbatim.
IsHandledreset findings to values that can actually carrytrue, including loop-carried stateModifyAll/DeleteAllfallback guidance with documented platform conditionsFalse-positive boundaries
falsefor one non-looping raise. A local declared outside a loop initializes once and can carrytrueacross iterations. A later raise reached only after a semantically validif IsHandled then exit;remains proven false.[HandlerFunctions]proves listed handlers execute. Missing semantic verification isminor; queue storage is not mandatory for every handler. Fixtures cover omitted handlers, listed-but-unreached handlers, and meaningful capture/assert verification.FindSetremains unbounded, and the checkpoint sample initializes its persisted watermark on cold start.Validation
python .github/scripts/validate_frontmatter.py --root .tools/Test-ReviewFixtures.ps1 -Root . -PrepareDirectory <temp>.github/scripts/Test-KnowledgeIndex.ps1 -Root .git diff --checkDeliberately unchanged
internal-access-is-not-a-security-boundary