Backfill BCQuality article classifications for code-review gold dataset (AB#646153)

[Wenjie Fan (gggdttt)]

Summary

Continues AB#646153 by extending the per-article BCQuality coverage work from #776 (which established the schema, the coverage notebook, and annotated the security domain) to every non-security domain in dataset/codereview.jsonl: performance, style, privacy, upgrade, error-handling, breaking-changes, data-modeling, testing, events, and ui.

Every association was made by reading the actual BCQuality article body (and its .good.al/.bad.al samples) from current microsoft/BCQuality main against the patch/finding — not guessed from the slug name. Where no existing article faithfully covers the exact finding a comment tests, the entry was left unannotated rather than force a mis-crediting association (mirroring the discipline #776 established).

#804 is unrelated to this PR's approach — it already merged into main (CI e2e slug validation) before this branch was created; this PR rebases on top of it and does not duplicate anything from it.

Before → after

Metric	Before	After
Total entries	144	144
Annotated entries	17	114
Unannotated entries	127	30
Unique articles covered	9	80
Comment-level (expected_comments[].articles) annotations	17	164
Entry-level (metadata.articles, FP/by-omission) annotations	4	30

30 entries remain intentionally unannotated:

• 27 non-security entries (this PR's scope) where no current BCQuality article exactly matches the finding — see reasons below.
• 3 security entries (security-007, security-013, sec-internal-not-boundary-01) are out of scope for this PR (security was already handled end-to-end in Add per-article BCQuality coverage for code-review dataset (AB#646153) #776) and were left untouched.

Unannotated, with reason (non-security)

data-modeling (3)

• data-modeling-excluded-from-calculation-01 — migration-buffer field-parity/open-balance calculation defect; no BCQuality article documents this exact scenario.
• data-modeling-blocked-validation-skip-01 — closest article (check-blocked-in-referencing-code-not-in-master) documents the opposite rule (enforcing Blocked); this finding is a migration-specific bypass boundary.
• data-modeling-tablerelation-restriction-mismatch-01 — no article covers keeping a setup field's TableRelation as narrow as its downstream target.

error-handling (7)

• error-testfield-enabled-01, error-handling-fieldno-swap-01, error-handling-guiallowed-01, error-handling-silent-skip-01, error-handling-assistedit-cancel-01, error-handling-drilldown-position-01, error-handling-case-unreachable-else-01 — each is a genuine, narrow AL defect/FP-boundary (wrong FieldNo copy-paste, over-broad GuiAllowed gating, silent success-shaped skip, unconditional SaveRecord after a cancellable AssistEdit, unstable-key drilldown restore, non-extensible-enum exhaustive case) that no existing BCQuality article documents at this level of specificity.

privacy (3)

• privacy-004 — corrected in this PR to a clean/FP-guard entry (see Gold-answer audit below); left unannotated since no article is an exact fit for the corrected scenario.
• privacy-013 — hardcoded personal email addresses in source/config; no article covers this specific case.
• privacy-clean-02 — telemetry message stays dynamic rather than matching an article's static-message good sample closely enough.

style (9)

• style-001, style-003, style-008, style-015 — generic indentation-width/Hungarian-prefix/PascalCase/snake_case naming conventions with no matching BCQuality article.
• style-duplicate-action-01 — obsolete-and-replace page-action FP boundary, not documented.
• style-tooltip-mismatch-01 — tooltip text contradicting an action's real behavior; domain corrected style→ui (see audit) but still no exact article.
• style-this-keyword-01 — flagged for human review, see "BCQuality/gold conflict" below.
• style-showmandatory-flowfield-01, style-field-repurpose-indent-01 — narrow FP/defect boundaries with no matching article.

testing (2), upgrade (3)

• testing-tolerance-clean-01, testing-ui-handler-assert-after-run-01 — no article for timezone-tolerance date-serialization assertions or post-RunModal semantic assertions.
• upgrade-obsolete-caption-01 — caption-only pseudo-deprecation; BCQuality has no page-action-specific obsoletion article (split across obsoletion/breaking-change docs, ambiguous which owns it — flagged suspicious, not forced).
• upgrade-transferfields-field-collision-01, upgrade-broken-hook-copyrows-01 — narrow TransferFields()/CopyRows() mechanics not documented.

BCQuality/gold conflict needing human review

• synthetic__style-this-keyword-01: this entry's own description cites a real BCApps PR (#8959, SpendRequestTests.Codeunit.al) where a developer explicitly rejected the this. requirement for self-calls ("Wrong. We should not use 'this'"). The current BCQuality article style/this-keyword-in-codeunits.md states the rule applies to all codeunits with no stated exception. A sub-agent proposed flipping this entry to expect_findings: true and adding a positive finding, purely from the article's literal text — I rejected that because it would defeat the entry's deliberate purpose (a false-positive guard grounded in real project precedent) and because BCQuality's own text doesn't yet reconcile the conflict. Recommend BCQuality maintainers either add a test-codeunit carve-out to the article or confirm the rule truly applies universally, then re-visit this entry.

Gold-answer audit

While reading each patch against its candidate article, the review also re-verified the existing gold expected_comments/metadata for high-confidence, tightly-coupled inconsistencies (never wording/style preference, never debatable maintainer judgment). 16 corrections applied, all independently re-verified by hand against the raw patch before being merged (not applied blindly from sub-agent proposals):

• performance-011 — 2 comment bodies recommended SetLoadFields for FlowFields, which cannot be preloaded that way; corrected to the real remedy (combine CalcFields calls / SetAutoCalcFields).
• performance-023 — added a missing, independent finding (CalcFields in a loop inside BuildVendorReport) the gold under-specified; also fixed a pre-existing "HTML body" mislabel found in passing (the code builds a plain report body, not HTML).
• style-010, style-018, style-tooltip-mismatch-01 — reclassified domain: style → ui (page-binding/tooltip correctness, not a style convention).
• style-011 — removed a hallucinated finding referencing a procedure (ImportTemplateFromStream) and code shape that don't exist anywhere in the patch.
• style-012 — fixed a misquoted Message() string that didn't match the patch, and added 5 missing xmldoc-for-public-library-procedures findings for sibling public procedures the gold left unflagged.
• style-013 — corrected a finding's line range from the Error() call site to the mislabeled-Err-suffix label's actual declaration line.
• privacy-003 — reclassified domain: privacy → style: BCQuality's own privacy/avoid-strsubstno-prebuild-before-error article states a StrSubstNo-wrapped Error does not log the composed text verbatim to telemetry, contradicting the gold's stated privacy concern; the real, better-matched issue is the style/error-passes-parameters-directly-not-strsubstno (AA0231) label-identity loss.
• privacy-004 — the sole finding rested on GetLastErrorText(true), which BCQuality documents as the scrubbed overload; flipped expect_findings to false and removed the finding.
• privacy-010 — removed a finding asserting an unsupported "classify/tag the PII flow for GDPR" requirement that duplicates the (kept) consent-check finding.
• privacy-011 — removed 3 of 4 findings that flagged fields for omitting DataClassification, when BCQuality's table-level-data-classification-cascades article explicitly says a field inheriting a valid table-level default should not be flagged solely for that omission.
• privacy-012 — removed 3 of 6 findings that flagged Error(Label, values) calls where the Label is passed directly (the documented correct pattern) rather than pre-baked via StrSubstNo.
• upgrade-008 — added a missing, independent finding: the migration logic lives in a Subtype = Install codeunit, which BCQuality documents as never running during an ordinary version upgrade.
• upgrade-009 — corrected a finding's body: the trigger does delegate to a named procedure; the real mismatch (per BCQuality) is that it keeps the upgrade-tag guard inline instead of letting the helper own it.
• errh-errortype-internal-01 — corrected the line range from the if-guard to the actual Error() call site being flagged.

2 sub-agent-proposed corrections were reviewed and rejected (not applied):

• synthetic__events-ishandled-reset-boundary-01 — a sub-agent proposed rewriting the body to drop its explicit "do not flag the sibling fresh-local/second-raise cases" guidance, reasoning the article requires IsHandled := false unconditionally. Rejected: that guidance is the entry's deliberate false-positive-boundary design (per its own description), not an error — removing it would delete legitimate scoring content.
• synthetic__style-this-keyword-01 — see "BCQuality/gold conflict" above.

7 suspicious-but-unresolved cases were logged by domain sub-agents and reviewed but left as-is (debatable, not tightly coupled to hard evidence): performance-010, performance-014, performance-018, performance-024, style-016, style-017, upgrade-obsolete-caption-01.

Validation

• CodeReviewEntry.load(...) — 144/144 entries parse and validate.
• uv run pytest -q (with BCQUALITY_ROOT) — 743 passed, 2 skipped.
• uv run pytest -q -m e2e (article-slug-exists-in-BCQuality check from Validate code-review article slugs against BCQuality #804) — 1 passed.
• uv run ruff format --check . / uv run ruff check . — all clean.
• uv run ty check — 23 pre-existing diagnostics (all in src/bcbench/redteam.py, an unrelated azure.identity import gap); confirmed identical on main before this change, none touch the files this PR modifies.
• notebooks/code-review-coverage.ipynb re-executed against current BCQuality main; every cell has an id, outputs are deterministic (no timestamps/session metadata).
• Diff is scoped to exactly the 99 entries actually changed (verified byte-for-byte preservation of all untouched lines) plus the re-executed notebook.

Scope discipline

• No changes to BCQuality, to scoring behavior, or to the schema from Add per-article BCQuality coverage for code-review dataset (AB#646153) #776/Let a review comment declare several BCQuality articles #805/Validate code-review article slugs against BCQuality #804.
• Security domain untouched (3 pre-existing unannotated security entries left as-is, out of scope).
• No new tests added; relies on Validate code-review article slugs against BCQuality #804's e2e slug-existence check rather than duplicating it.