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.
3 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