mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
21 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Michael Dieringer
|
49ddf8ae46 |
knowledge(style): a new procedure that changes the page's current record should take it as var Record
Adds mutating-procedure-for-a-page-caller-takes-var-record with good/bad samples, a worklist cue in al-style-review, and registration in the style review-fixtures override. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Michael Dieringer
|
f63943dcfd
|
18 more AL/BC patterns: data-modeling, testing, style, security, error-handling, ui, upgrade, web-services, appsource (#157)
* Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files. * Address Jesper Schulz-Wedde's review on PR #157 - release-must-update-app-version.md: reframe around AppSource's actual strict full-version-ordering requirement; scope branching-policy claims as team convention, not platform rule. - pictures-must-use-media-not-blob.md: MediaSet is a collection of independent media objects, not automatic image variants/thumbnails. - log-writes-must-survive-rollback.{md,good.al}: StartSession's only data channel into the new session is its Record parameter to a TableNo-scoped codeunit; a setter called on a local instance before starting the session populates nothing in the new session. - exposed-objects-must-be-in-a-permission-set.md: correct the three exposure mechanisms (Web Services config, PageType/QueryType=API, ServiceEnabled as a method-only attribute). - pages-must-not-contain-business-logic.md: scope to persisted mutations and cross-entry-point rules; presentation-only calculations and table-owned invariants are not violations. - given-blocks-must-cover-full-precondition-chain.good.al: replace invented LibrarySales calls with the real API (CreateCustomer/CreateSalesOrderForCustomerNo/PostSalesDocument). - test-feature-scenario-tags.{md,good.al}: move [SCENARIO] inside the test procedure body to match the current BCApps corpus; keep [FEATURE] at codeunit level per Microsoft's own documented option. - ui-test-codeunit-naming.md: scope the _UT suffix and adjacent-ID pairing as an explicit team convention, not a BCApps-wide standard. - page-design-must-match-bc-page-type-conventions.md / table-design-must-match-bc-table-type-conventions.md: Card's single-key primary-key claim is a contextual heuristic, not a mandatory constraint (Ship-to Address, Customer/Vendor Bank Account are real composite-key Card pages); a Subsidiary table with its own identity commonly gets List+Card, not Worksheet/Tabular. - api-page-least-privilege-write-access.{md,good.al}: only page-placed fields are ever exposed; set InsertAllowed/DeleteAllowed=false in the good sample so a narrow field set can't still create/delete records. - source-organized-by-feature-not-object-type.md, test-one-when-per-test.md: scope as team/testing-design conventions, not Microsoft platform requirements. - upgrade-tag-logic-must-not-nest-deeply.md: add the Microsoft Learn citation that already backs the two-level nesting limit. - Wire the new articles into the testing/data-modeling/error-handling/ security/ui review skills' candidate-selection signals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address second round of Jesper Schulz-Wedde's review on PR #157 - log-writes-must-survive-rollback.good.al: fixed invalid trigger OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set. - exposed-objects-must-be-in-a-permission-set.md: distinguished the three exposure mechanisms (page/query web service or API, codeunit published as a web service, [ServiceEnabled] bound action on a page) and their actual permission targets (page/query "..." = X vs codeunit "..." = X). - code-must-not-change-workdate.md: scoped from an absolute "never" to "not as a side effect of unrelated logic" - verified real WorkDate(x) setter usage in BCApps demo-data generators and test codeunits. - bcpt-scenarios-must-be-app-specific.md: SingleInstance and StartScenario/EndScenario reframed as context-dependent patterns, not mandatory requirements - BCPT Create Customer uses neither. - test-feature-scenario-tags.good.al/.bad.al: replaced the invented LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real, verified price-list-line test using Library - Sales/Library - Inventory/ Library - Price Calculation. - page-design-must-match-bc-page-type-conventions.md: scoped the missing UsageCategory anti-pattern to pages intended as searchable entry points. - defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al: replaced the VAT registration number "low blast radius" example with a genuinely cosmetic field (customer home page URL). - source-organized-by-feature-not-object-type.md: anti-pattern reframed as inconsistency with a repo's own convention, not the object-type scheme itself. - pictures-must-use-media-not-blob.md: removed leftover "image variants" wording contradicting the already-corrected MediaSet description. Proactively fixed while sweeping all fixtures for invented APIs: - given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder called with wrong arity and referenced an undeclared variable. - ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake "Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls and the real Customer Card TestPage. Worklist completeness: added review-skill cues for the 12 of 18 new rules that had none (al-appsource-review.md, al-data-modeling-review.md, al-error-handling-review.md, al-security-review.md, al-style-review.md x3, al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md, al-web-services-review.md), and fixed test-feature-scenario-tags' cue, which only matched the compliant (tagged) shape instead of the anti-pattern (untagged/generic-named test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix ten focused correctness items plus sample links from Jesper's 2026-09-15 re-review Six carried-over threads: - api-page-least-privilege-write-access fixtures: added the mandatory EntityName/EntitySetName properties (AL0485). - pages-must-not-contain-business-logic fixtures: Sales Line has no "Total Amount" field; replaced with the real "Line Amount" (field 103). - test-feature-scenario-tags.good.al and test-one-when-per-test.good.al: CreatePriceHeader leaves a price list in Draft status, which price calculation ignores. Added Validate(Status, Active) + Modify before the sales line that depends on it. Verified Status field/enum against PriceListHeader.Table.al and PriceStatus.Enum.al in the BCApps clone. - exposed-objects-must-be-in-a-permission-set.md: a published codeunit is a SOAP endpoint (SOAP is deprecated), not OData - Page/Query are the OData object types. Corrected and pointed new integrations at API pages/queries instead. - al-error-handling-review.md: the log-writes-must-survive-rollback cue selected on Session.StartSession, which only appears in the compliant fix, never in the anti-pattern - the bad fixture could never be worklisted. Recued on the actual risk shape (log insert around a failed TryFunction/GetLastError* path, then raise/propagate), with StartSession as an explicit compliant discriminator instead. - page-design-must-match-bc-page-type-conventions.md: the enum value is NavigatePage, not Navigate; noted the type list is a selected subset, not an exhaustive PageType catalogue (PromptDialog, ConfigurationDialog, UserControlHost, XmlPort also exist, out of this article's scope). Four new correctness gaps: - release-must-update-app-version.md: "the version is the only identity" was backwards - id is the app's stable identity, version identifies a release/code-state of it. - defensive-vs-offensive-code-must-match-blast-radius.good.al: the "low blast radius" example had no else branch, so a failed Customer.Get() left the field at its prior/default value instead of the explicit chosen fallback the article claims to demonstrate. Added the else. - bcpt-scenarios-must-be-app-specific.good.al: InitTest and both measured StartScenario/EndScenario sections were empty/comment-only, so the "app-specific" fixture measured no actual work. Filled in a real, self-contained header+line creation path. - upgrade-tag-logic-must-not-nest-deeply.good.al: the flattened version dropped both safety conditions the bad fixture had (Discount % = 0, nonblank posting group), silently changing behavior instead of just removing nesting. Extracted the guarded update into a helper with both conditions preserved as early exits. Also converted this PR's remaining plain-backtick "See sample:" sample references (16 articles) to the READ-convention markdown-link form, matching the fix already made on #156/#158. Rebased onto upstream/main (conflicts in al-ui-review.md, al-style-review.md, al-upgrade-review.md against merged upstream PRs - all additive, both sides' worklist cues retained). * Fix four merge-critical issues from Jesper's 2026-09-22 review - pages-must-not-contain-business-logic.good.al/.bad.al: the "good" codeunit still directly assigned real Sales Line."Line Amount" and called Modify(), bypassing the field's normal Validate cascade (discount, VAT, related-amount maintenance) - persisting inconsistent document lines regardless of which object the code lived in. Replaced the real Sales Line example with a self-contained "Sample Order Line" table and switched the codeunit to Validate()/Modify(true), so the fixture demonstrates the page-vs-codeunit separation without teaching unsafe direct field writes to a real BC document table. - bcpt-scenarios-must-be-app-specific.good.al: Customer.FindFirst() assumed a pre-existing customer (fails against an empty environment), and a session-local NextNo counter for the header key collides across concurrent BCPT sessions and repeated runs. Creates its own customer when none exists, and generates keys from CreateGuid() instead of an in-memory counter. - upgrade-tag-logic-must-not-nest-deeply: the rule conflated two different things - nesting one tag's existence check inside another (the real anti-pattern Microsoft's guidance warns against) with having business-data safety conditions inside a single tagged migration's own loop body (which Microsoft's own worked example does, and its own design guidance explicitly requires: "Implement extra safety checks to avoid data corruption, even though you're using upgrade tags"). Rewrote the Description/Best Practice/Anti Pattern to scope the rule to actual tag nesting and migrations blended under one tag, and rewrote both fixtures: good.al now shows two safety conditions correctly nested inside one migration's own loop plus a second, genuinely separate migration as its own flat tagged procedure; bad.al now shows the real anti-pattern, one tag's check nested inside another's guarded body. - table-design-must-match-bc-table-type-conventions: the rule and its worklist cue fired on any new table with a keys block, forcing buffers, queues, logs, mapping tables, and staging tables into the nearest-looking one of nine business-record archetypes. Added an explicit scope note that these nine types aren't an exhaustive table catalogue, and narrowed the al-data-modeling-review.md cue to require positive evidence (a type-specific naming suffix, key shape, or usage) before worklisting, instead of a bare keys/primary-key declaration. * Narrow upgrade-tag nesting cue to match revised article Cue now flags only nested upgrade-tag checks or functionally unrelated migrations under one tag, and explicitly excludes record loops and business-data safety guards belonging to a single migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Michael Dieringer
|
0a8c9a8556
|
18 AL/BC patterns: style, data-modeling, web-services, appsource, breaking-changes, performance, testing (#156)
* Add 18 community AL/BC patterns across style, data-modeling, web-services, appsource, breaking-changes, performance, and testing Contributed by CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Each article follows the knowledge file format (frontmatter, Description/Best Practice/Anti Pattern, sibling .good.al/.bad.al samples). * Address Jesper Schulz-Wedde's review on PR #156 - Rename 3 articles so their .good.al/.bad.al companion stems match (do-not-change-primary-key, testfield-required-setup-field, al-identifiers-english), fixing the R14 orphan-sample errors. - do-not-change-primary-key.good.al: include Flow in the new table's own primary key so it actually models the discriminating dimension. - al-build-output-must-not-pollute-project-root.md: drop the unsubstantiated AL0197 causal claim and the non-existent al.outputPath setting; reframe as build-artifact hygiene sourced from ALTool --outfolder / al_build outputPath. - prefer-email-module.md: Email Message is Codeunit 8904, not a table; distinguish it from the underlying Sent/Outbox/Draft storage. - file-datatype-saas.md: File.Open/Create/Read/Write fails to compile against a Cloud-scoped project, it does not compile and silently fail at runtime. - namespace-must-be-verified-from-source.md: narrow to "resolve from the referenced object's source or symbols," since source-file line one is not the only authoritative source (symbol packages, comments before the namespace line). - test-data-must-be-random-and-complete.md: drop "assume an empty database" and "collision-free" absolutes; reframe around independence from unrelated business records and reserving explicit values for scenario-defining inputs. - binary-choice-must-be-boolean.md: scope to genuine true/false semantics, not mechanical two-member-enum-to-boolean conversion. - document-report-word-layout.md: scope down to a sourced Microsoft Learn recommendation instead of an unconditional performance guarantee; cite the three Learn pages. - Wire the new articles into their review skills' candidate-selection signals (file-datatype-saas, prefer-email-module, namespace-must-be-verified-from-source, var-parameters-require-an- addressable-variable) so they can actually enter a worklist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix dimension-management-wiring.md: ValidateShortcutDimCode and CreateDim do not exist on the current DimensionManagement codeunit Verified against microsoft/BCApps: the real master-table validation procedure is ValidateDimValueCode (or ValidateShortcutDimValues when a DimSetID is also needed), and the real document-side inheritance procedure is GetDefaultDimID, not CreateDim. Caught from Jesper Schulz-Wedde's review thread, which had been partially hidden by GitHub's comment folding. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Address second round of Jesper Schulz-Wedde's review on PR #156 - dimension-management-wiring.md/.good.al: split into the two distinct models the article was conflating - master data (Default Dimension records via ValidateDimValueCode/SaveDefaultDim) vs. transactional/ document data (a single Dimension Set ID assembled via AddDimSource + GetDefaultDimID, verified against BCApps' ExchRateAdjmtProcess.Codeunit.al). Added a compiling document-table example alongside the existing master table one. - Deleted api-page-flowfields-must-be-calcfields (.md/.good.al/.bad.al): Microsoft's own FlowFields documentation states a FlowField used as a control's direct source expression is automatically calculated on any page - no API-page exception is documented, and none could be reproduced. - prefer-email-module.bad.al/.md: Codeunit Mail has no Send/GetErrorDesc members; fixed to the real current 7-argument CreateMessage signature, and corrected the claim that the legacy path "still runs" - its base implementation no longer sends anything, only raises integration events. - check-post-line-batch-pattern.md/.good.al: reframed from a universal invariant to the standard shape, naming the real Gen./Item/CA/Res./Job/ Insurance/Mfg. Item/FA Jnl.-Check Line/-Post Line/-Post Batch codeunits it's based on. Added the missing Check Line companion codeunit so the good fixture is internally complete. - test-data-must-be-random-and-complete.good.al: removed leftover "collision-free" wording contradicting the already-corrected article text. - fixed-choice-set-must-use-enum-not-integer.md: removed the reintroduced state-count heuristic ("the line is the state count"), aligned with binary-choice-must-be-boolean.md's semantics-based distinction. - namespace-must-be-verified-from-source.md: removed the false claim that the compiler and AL Language Server use different namespace-resolution rules. - intrinsic-al-functions-must-use-modern-casing.md: removed the unverified claim that PascalCase is the VS Code formatter's default output. Worklist completeness: added cues for the 8 rules in data-modeling, testing, performance, and web-services that had none (Jesper's explicit ask), plus the same gap in all 7 style rules from this PR (not explicitly named this round, but the identical systemic issue) - 15 cues total across al-data-modeling-review.md, al-testing-review.md, al-performance-review.md, al-web-services-review.md, and al-style-review.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix remaining correctness issues from Jesper's 2026-09-15 re-review - dimension-management-wiring: SaveDefaultDim's third argument is the shortcut dimension number (1-8), not the field's AL field ID; the fixture passed FieldNo(...) = 10. GetDefaultDimID's InheritFromDimSetID must be 0 when recomputing after the linking record changes, not the document's existing Dimension Set ID (which would retain the previous customer's leftover dimensions). Verified against DimensionManagement.Codeunit.al and BankDepositHeader.Table.al in the BCApps reference clone. - check-post-line-batch-pattern: "Post Line writes exactly one line to the ledger" overclaimed - Gen. Jnl.-Post Line alone calls InsertGLEntry from a dozen call sites (balancing entry, VAT, currency rounding, deferrals) and can write several G/L Entries per journal line. Reworded to "posts exactly one journal line" and softened the "distinct, non-overlapping responsibilities" absolute. - namespace-must-be-verified-from-source.bad.al: dropped the "resolves in a local build, fails in VS Code" comment (taught an inherent compiler/language-server disagreement that isn't real); reframed as stale/cached symbols, matching the prose fix already made. - file-datatype-saas.good.al: replaced the deprecated 5-argument UploadIntoStream overload with the current 2-argument one, and actually staged through TempBlob as the article's own Best Practice instructs (the declared TempBlob variable was previously unused). - test-data-must-be-random-and-complete: no longer treats a short-but-valid value as defective merely for being "underfilled" - AL field lengths are maxima, not minimums. Scoped to missing values or a scenario with an explicit length/format requirement (e.g. a truncation test). Updated the al-testing-review.md routing cue to match. - stored-derived-fields-must-not-be-exposed-directly: stopped mandating source-field exposure as part of the core pattern: the good fixture exposed only one of the derived value's two inputs (Hours Used, not Budgeted Hours), making the claimed "so the consumer can verify it" impossible. Reframed as an optional, all-or-nothing addition and fixed the fixture to expose both inputs. Rebased onto upstream/main to resolve conflicts in al-breaking-changes-review.md, al-data-modeling-review.md, al-performance-review.md, and al-style-review.md against merged PRs #148 and #153; all sides' worklist tokens/cues retained. * Fix remaining READ-convention sample links across this PR's 18 articles The same plain-backtick "See sample: \`x.good.al\`." form fixed on al-methods-limited-during-write-transactions (PR #161) turned up repo-wide on 15 more of this PR's articles - Knowledge-Retrieval.ps1 requires the markdown-link form to associate a sample with its article. All 16 fixed; the four local validators (frontmatter, knowledge-index, knowledge-retrieval, review-fixtures, skill-index) pass. * Fix two merge-critical correctness issues from Jesper's 2026-09-22 review - api-page-key-fields-must-be-editable-on-insert.good.al and stored-derived-fields-must-not-be-exposed-directly.good.al: both were writable API pages missing DelayedInsert = true, contradicting this repo's own api-page-delayedinsert-true rule - the canonical "good" samples were teaching code BCQuality itself flags. - dimension-management-wiring.good.al: UpdateDimensionSetID exited early when Customer.Get failed, leaving the previous customer's shortcut dimension and Dimension Set ID in place - the same staleness bug the InheritFromDimSetID = 0 fix (from the prior review round) was meant to prevent, just triggered by a failed lookup instead of a successful one. Now clears the shortcut field and recomputes with an empty source list on a failed lookup too, so GetDefaultDimID correctly returns an empty Dimension Set ID instead of never running. --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Jesper Schulz-Wedde
|
d38377b85e
|
Clarify locale-safe DateFormula Evaluate inputs (#193)
* Clarify locale-safe DateFormula Evaluate inputs Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Normalize DateFormula article sections Keep the analyzer-gap explanation in Description and its scoped probe evidence in References, without a novel Validation section. Normative guidance and fixtures are unchanged. 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> |
||
|
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 |
||
|
Jesper Schulz-Wedde
|
c12b2f0a88
|
Separate analyzer rules from BCQuality knowledge (#178)
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> |
||
|
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> |
||
|
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> |
||
|
Wenjie Fan
|
712dee9ec1
|
style-review: calibrate variable-declaration order (AA0021) to info (#109)
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> |
||
|
Jesper Schulz-Wedde
|
186d8a1314
|
Complete AL review knowledge readiness (#108)
* 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> |
||
|
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> |
||
|
Wenjie Fan
|
809af9708e
|
Privacy DataClassification fixes + keep Label-scope findings at minor (#102)
* 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> |
||
|
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> |
||
|
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> |
||
|
Jesper Schulz-Wedde
|
34c931e1c1
|
Fix knowledge corpus integrity issues (#87)
Repair broken knowledge references and align review examples with canonical articles. Correct explicit version gates, restore a missing title, and recognize the plugin directory in the root guard. Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> |
||
|
Jesper Schulz-Wedde
|
822cae1b27
|
Own the knowledge-index generator + index-aware review skills (#25)
* Make domain-skill knowledge discovery index-aware The 6 AL domain review skills and read.md now enumerate candidate articles from the BCQuality knowledge index (knowledge-index.json) instead of opening every file under the domain folder to read its frontmatter. The worklist selection predicate is unchanged (keywords intersect diff tokens, or topic matches a changed object type) - only the discovery source changes, so the same articles are selected. Full article bodies are read only for worklisted entries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reconcile §Source wording with the lean knowledge index The BCQuality filter now emits a lean index whose per-article description is a one-line hint rather than the full verbatim Description. Update the six domain skills' §Source to say the index carries a one-line description hint (keywords, title, and a one-line description) instead of the full description. The worklist selection predicate is unchanged: keywords drive selection and the agent opens worklisted articles in full for their rule bodies. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Own the knowledge-index generator in BCQuality The knowledge index is an acceleration of the skills' Source step, and its schema is part of that contract — so BCQuality should own the generator rather than each consumer re-implementing it. Add tools/Build-KnowledgeIndex.ps1 (the parser + lean-description shaping + emit, lifted verbatim from the BCAppsBCQuality filter prototype) and document the index in agent-consumption.md. Consumers prune their clone to policy, then call this script; the index stays in lockstep with the Source contract and every orchestrator gets the same faithful index for free. The worklist selection predicate is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Own knowledge-index generation in BCQuality (runtime + CI), not the consumer The index is now produced by BCQuality itself: Entry's preparation step rebuilds knowledge-index.json over the live, already-pruned clone at the start of every run, and a new CI workflow validates the generator's health (determinism, full coverage, selection-input integrity). Consumers no longer invoke or know about the index. Rebuilding over the pruned clone (vs shipping a committed full-corpus index) keeps the index exact for any consumer policy: it can never list a denied article, so policy-excluded rules cannot leak into discovery. READ now states the index is discovery-only -- a finding must cite an article opened in full, and rows whose file is absent are discarded before ranking. - skills/entry.md: new 'Preparation -- knowledge index' precondition - skills/read.md: index ownership + discovery-only invariant - microsoft/skills/review/*.md (6): 'BCQuality builds' (not 'the filter emits') - agent-consumption.md 5a: runtime+CI ownership rationale - .github/workflows/knowledge-index.yml + scripts/Test-KnowledgeIndex.ps1: generator guard - .gitignore: never commit the runtime index Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Make runtime index build non-interactive and self-contained entry.md now gives the exact build command (pwsh ./tools/Build-KnowledgeIndex.ps1) so the agent's preparation step is unambiguous, and the generator's -BCQualityRoot parameter is optional (defaults to the clone root) so it runs in non-interactive -p mode without prompting. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Resolve knowledge-index root to absolute path (cross-platform fix) Get-ChildItem.FullName is always absolute, so deriving the relative article path via Substring(\.Length) requires an absolute root. A relative root such as '.' (used by the CI guard's 'Test-KnowledgeIndex.ps1 -Root .') left the full path almost intact on Linux, producing bogus 'home/runner/.../knowledge' paths and failing the coverage check. Normalise both the generator's -BCQualityRoot and the test's -Root with Resolve-Path before use. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> |
||
|
Jesper Schulz-Wedde
|
f07efa74a2 |
Tighten precision bar for agent findings and cap their severity
Agent findings (model-judgment findings not backed by a curated knowledge file) were producing too many false positives. Raise their emission bar and make them advisory: - skills/do.md: add a canonical 'Precision bar' for agent findings (emit only concrete, material defects an expert would agree on; steelman first; an explicit never-emit list; 'when in doubt, omit') and cap agent-finding severity at minor (advisory, non-gating), with a promotion note for severe-but-uncovered concerns. - al-code-review.md: decouple the mandatory self-review reasoning from output so emitting zero agent findings is a valid outcome; add the severity cap. - Six leaf review skills: add the per-domain precision bar and severity cap (style leaf gets domain-appropriate wording). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jesper Schulz-Wedde
|
d994f2ad0e |
Require suggested-code or omission reason for mechanical findings
BCQ now wins on coverage in the synthetic complex benchmark, but only 2 of 25 rendered comments carried GitHub suggestion blocks while the AIRHack reviewer emitted suggestions for every finding. Tighten the DO contract and leaf-skill instructions so suggested-code is no longer a soft affordance for mechanical fixes. Changes: - Add optional findings[].suggested-code-omission-reason to the DO schema. - Document suggested-code as expected for small, local, mechanical findings, with examples (delete unreachable code, Count() > 0 -> not IsEmpty(), move local Label, add missing ToolTip/OptionCaption, replace string-concatenated Error, change permission token, add an obvious else/guard branch). - Require omission reasons when a mechanical-looking finding omits suggested-code. - Update all AL leaf skills and al-code-review guidance with the same stronger contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jesper Schulz-Wedde
|
3a07ee82d2 |
Allow leaf sub-skills to emit agent findings within their domain
The original DO contract pinned all agent reasoning to the super-skill: 'Agent findings are emitted only by super-skills... Leaf sub-skills MUST NOT emit agent findings'. This funnels all agent reasoning across all 6 domains through a single super-skill pass, which is the root structural cause of the attention dilution we have been chasing in BCAppsBCQuality PRs #28 and #30: - T1 standalone run showed al-security-review finds rimd-on-read-only cleanly when run alone, but emits zero agent findings because the contract forbids it. So obvious things like case-without-else (no matching KB article yet) get dropped on the floor. - The al-code-review self-review pass keeps producing 0-1 agent findings per PR because it is asked to reason across 6 domains in one pass. The fix is to move agent reasoning into the leaves, bounded by each leaf's domain. Each leaf now has both knowledge-backed and agent-finding permissions within its own scope; the super-skill self-review pass becomes a smaller, cross-cutting role. skills/do.md - Replace the 'only by super-skills' / 'MUST NOT' clause with a two-tier model: leaf sub-skills MAY emit agent findings strictly within their declared domain; super-skills MAY emit agent findings for cross-cutting concerns that span domains. - Update the encoding rules: leaf agent findings have references:[] and an agent:-prefixed id, no from-sub-skill (the leaf's own report carries the finding under its own skill.id). Super-skills set from-sub-skill='agent' for their own self-review findings; when rolling up leaf agent findings, they set from-sub-skill=<leaf-id>. - Clarify that 'MUST validate against knowledge' applies to super- skill self-review candidates only - leaves already validated within their domain when they decided to emit. microsoft/skills/review/al-{security,performance,privacy,style,upgrade, ui}-review.md - New paragraph after the confidence rules instructing each leaf to surface domain-specific agent findings when no knowledge file covers a defect the agent recognises from general AL knowledge. - Bound the scope: 'The scope is strictly <domain>; defects outside this domain belong to other leaves and MUST NOT be emitted here.' - Same validation requirement: check the worklist for a matching knowledge file first; if one exists, upgrade to a knowledge-backed finding instead. microsoft/skills/review/al-code-review.md - Rewrite the 'Agent self-review pass' subsection. The pass is now explicitly for cross-cutting concerns that no single leaf could have surfaced because they span multiple domains. Domain-specific reasoning belongs in the leaves, not duplicated here. - Update the rollup behavior to acknowledge leaf-emitted agent findings: they are rolled up like any other sub-skill finding, with from-sub-skill set to the leaf id, and are not re-validated by the super-skill (the leaf already validated within its own domain). - Drop the 'Leaf sub-skills MUST NOT emit agent findings' line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jesper Schulz-Wedde
|
31b9949235 |
Strengthen al-code-review execution, propagate suggested-code to leaves, two new KB articles
Driven by a parity comparison between BCAppsBCQuality PR #27 and BCAppsCampAIRHack PR #162 on byte-identical content: | | BCQuality | AIRHack | |--|--|--| | Total findings | 6 | 10 | | Performance | 0 | 4 | | Security | 0 | 5 | Standalone runs of al-security-review and al-performance-review against the SAME diff produced the expected matches (rimd-on-read-only via inherent-permissions-minimal-grant; redundant-Get via avoid-redundant-get-when-record-already-loaded). The miss in the live run is therefore not a knowledge-coverage gap and not a worklist filtering issue. It is attention dilution inside the al-code-review super-skill, which the model collapses into one rolled-up generation pass on real-size PRs. Changes: microsoft/skills/review/al-code-review.md - New 'Execution discipline (mandatory)' subsection in the Action step that explicitly forbids collapsing leaves into one shared reasoning pass and requires each sub-skill to walk its Source -> Relevance -> Worklist -> Action steps as its own iteration before the next leaf starts. - Self-review pass is now described as the final, mandatory iteration with a concrete candidate-category checklist (architecture-level smells, error-handling gaps, magic constants, privacy/telemetry, resource lifecycle). Returning zero agent findings on a real-size diff is explicitly defined as a defect. microsoft/skills/review/al-{security,performance,privacy,style, upgrade,ui}-review.md - Each leaf skill now states that when an unambiguous .good.al companion exists, findings[].suggested-code should carry the literal replacement for the source lines. Closes the one-click-suggestion gap created when BCQ#19 only updated al-code-review. microsoft/knowledge/security/case-must-handle-unknown-enum-values.{md, bad.al, good.al} - New article: case over a security-sensitive enum (Authentication Type, Authorization Mode, Identity Provider, Permission Scope, Encryption Algorithm) MUST have an else arm. Without it, an unknown enum value silently falls through and the security context never initialises. The bad sample is lifted from the SharePoint Graph helper that triggered the parity finding. microsoft/knowledge/performance/instream-length-unreliable-for-bc- streams.{md,bad,good} - New article: InStream.Length returns 0 / partial for HTTP-response streams and some file-API streams, breaking size-threshold branching in upload code. Bad sample is the simple-vs-chunked Graph upload pattern; good sample materialises into a Temp Blob first. Companion change: microsoft/BCAppsBCQuality#28 extends the orchestrator's bootstrap prompt with the same per-iteration execution discipline and adds a CI warning when a >5-file PR returns zero agent findings (regression signal). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> |
||
|
Jesper Schulz-Wedde
|
287c041844 |
Expand review skills to match the 6-domain knowledge corpus
The knowledge corpus now covers performance, security, privacy, upgrade, style, and UI. Previously only two leaf reviewer skills existed (al-performance-review, al-security-review), so four of the six domains had knowledge with no skill sourcing from them. A community reader landing in privacy/, upgrade/, style/, or ui/ would see articles with no apparent consumer. Three changes: 1. Move existing review skills into `microsoft/skills/review/`. The `review/` subfolder groups all review-kind skills together and leaves room for future non-review action skills at the `microsoft/skills/` level. Updates references in README.md, agent-consumption.md, and skills/entry.md to the new paths. 2. Add four new leaf reviewer skills — al-privacy-review, al-upgrade-review, al-style-review, al-ui-review — each following the same DO template as al-performance-review/al-security-review but sourcing from the corresponding knowledge domain. al-upgrade-review and al-ui-review return `not-applicable` when the diff contains no upgrade surface or no page files, respectively. 3. Update al-code-review to compose all six leaf skills and retarget the dangling references in every populated JSON example (`use-setloadfields.md`, `no-plaintext-secrets-in-telemetry.md`, `avoid-implicit-commit.md` — none of which exist in the corpus) to real knowledge files: `call-setloadfields-before-filters.md`, `use-secrettext-for-credentials.md`, `never-hardcode-secrets-in-al.md`. Validator passes with 0 errors / 0 warnings. |