Skip to content

dataset(code-review): add synthetic reaction gold answers from BCApps PR 10990-neg-8222ad38e67a1bf4ee3b67d2389a957f0d00c77a #913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Wenjie Fan (gggdttt) wants to merge 1 commit into from
Closed

dataset(code-review): add synthetic reaction gold answers from BCApps PR 10990-neg-8222ad38e67a1bf4ee3b67d2389a957f0d00c77a #913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .github/self-improvement-selection.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
{
"version": 1,
"mode": "new",
"datasetBase": "e89b01069c079e3e4741587ffd5b4c45eae85143",
"datasetHash": "46A978950BE2F3B98ED647D3AF503448E74879D29E95988C1394EB90DD443EB6",
"sourceRepo": "microsoft/BCApps",
"requestId": "neg-8222ad38e67a1bf4ee3b67d2389a957f0d00c77a",
"sourceCommentUrls": [
"https://github.com/microsoft/BCApps/pull/10990#discussion_r3926331607"
],
"entryIds": [
"synthetic__upgrade-demo-data-not-upgraded-01"
],
"newEntryIds": [
"synthetic__upgrade-demo-data-not-upgraded-01"
],
"reusedEntryIds": [],
"selections": [
{
"instanceId": "synthetic__upgrade-demo-data-not-upgraded-01",
"origin": "new",
"coverage": [
{
"commentUrl": "https://github.com/microsoft/BCApps/pull/10990#discussion_r3926331607",
"purpose": "calibration_context",
"file": "src/DemoAgentContosoModule.Codeunit.al",
"lineStart": 9,
"lineEnd": 9,
"rationale": "Reproduces the reviewed boundary: a Contoso demo-data module's CreateMasterData() wires a new VAT-rate seeding codeunit that inserts persisted setup records, with no Subtype = Upgrade codeunit to propagate those records to already-provisioned tenants, matching the bot's upgrade-codeunit-subtype and install-code-does-not-run-on-version-upgrade reasoning. Feedback was negative (THUMBS_DOWN) and the author replied 'Not needed here, this is Preview demo data', rejecting applicability to Contoso/demo-data tooling specifically -- a product/process judgment call, not a confirmed absence of the underlying defect pattern. Per the no-invented-clean-negative rule, the expected finding is retained rather than converted into a false_positive_guard; this is deliberately scoped as calibration_context so the disagreement is captured honestly. Limitation: does not prove the bot's generic upgrade-safety guidance extends to demo-data/Preview-tooling modules, and does not resolve whether BC's Contoso demo-data lifecycle exempts such modules from Subtype = Upgrade requirements -- that remains an open, human-reviewed question."
}
]
}
],
"payloadHashes": [
{
"sha256": "B293CB4B0388B7C50273298DC004B4DDA3AE77E927446CF574641F6C6A4CCE8F",
"instanceId": "synthetic__upgrade-demo-data-not-upgraded-01"
}
]
}
1 change: 1 addition & 0 deletions dataset/codereview.jsonl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,4 @@
{"repo": "microsoft/BCApps", "instance_id": "synthetic__breaking-scope-creep-unrelated-api-01", "base_commit": "397d01199c321e774edaf23a7290fee40f75c6a6", "created_at": "2026-08-18T00:00:00Z", "environment_setup_version": "27.0", "project_paths": [], "metadata": {"area": "breaking-changes"}, "patch": "diff --git a/src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al b/src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al\n--- a/src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al\n+++ b/src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al\n@@ -906,7 +906,7 @@ table 8059 \"Subscription Line\"\n until BillingLine.Next() = 0;\n end;\n \n- internal procedure UpdateNextBillingDate(LastBillingToDate: Date)\n+ procedure UpdateNextBillingDate(LastBillingToDate: Date)\n var\n NewNextBillingDate: Date;\n OriginalInvoicedToDate: Date;\ndiff --git a/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al b/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n--- a/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n+++ b/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n@@ -463,6 +463,7 @@ codeunit 8062 \"Billing Proposal\"\n BillingLine.\"Subscription Contract Line No.\" := ServiceCommitment.\"Subscription Contract Line No.\";\n BillingLine.\"Discount %\" := ServiceCommitment.\"Discount %\";\n BillingLine.Discount := ServiceCommitment.Discount;\n+ ServiceObject.SetLoadFields(Quantity);\n ServiceObject.Get(ServiceCommitment.\"Subscription Header No.\");\n BillingLine.\"Service Object Quantity\" := BillingLine.GetSign() * ServiceObject.Quantity;\n OnAfterUpdateBillingLineFromSubscriptionLine(BillingLine, ServiceCommitment);\n", "expected_comments": [{"file": "src/Apps/W1/Subscription Billing/App/Service Commitments/Tables/SubscriptionLine.Table.al", "line_start": 909, "line_end": 909, "severity": "low", "domain": "breaking-changes", "body": "This performance change also widens the existing UpdateNextBillingDate procedure from internal to public, creating a new external compatibility commitment unrelated to the SetLoadFields optimization. Split the API change into a dedicated review or provide the intended external-consumer and compatibility rationale; otherwise keep it internal. This is an API-scope concern, not a security boundary, so no authorization check is implied.", "articles": ["breaking-changes/choose-access-modifiers-deliberately"]}], "category": "code-review", "description": "Low-severity API-scope concern: a real base-to-head internal-to-public change is bundled with an unrelated SetLoadFields optimization. This differs from synthetic__breaking-access-modifier-01, which tests a newly introduced internal helper that should be local.", "expect_findings": true, "source": "vsoadmin"}
{"repo": "microsoft/BCApps", "instance_id": "synthetic__perf-progress-dialog-deleteall-01", "base_commit": "397d01199c321e774edaf23a7290fee40f75c6a6", "created_at": "2026-08-18T00:00:00Z", "environment_setup_version": "27.0", "project_paths": [], "metadata": {"area": "performance", "articles": ["performance/use-deleteall-for-filtered-bulk-deletion"]}, "patch": "diff --git a/src/Apps/W1/Subscription Billing/App/Billing/Tables/BillingLine.Table.al b/src/Apps/W1/Subscription Billing/App/Billing/Tables/BillingLine.Table.al\n--- a/src/Apps/W1/Subscription Billing/App/Billing/Tables/BillingLine.Table.al\n+++ b/src/Apps/W1/Subscription Billing/App/Billing/Tables/BillingLine.Table.al\n@@ -249,10 +249,11 @@ table 8061 \"Billing Line\"\n var\n BillingLine2: Record \"Billing Line\";\n begin\n if \"Document No.\" <> '' then\n Error(CannotDeleteBillingLinesWithDocumentNoErr);\n+ BillingLine2.SetLoadFields(\"Entry No.\");\n FindFirstBillingLineForServiceCommitment(BillingLine2);\n if (BillingLine2.\"Entry No.\" = \"Entry No.\") then\n ResetServiceCommitmentNextBillingDate()\n else\n Error(OnlyLastServiceLineCanBeDeletedErr, \"Subscription Header No.\");\n RecalculateCustomerContractHarmonizedBillingFields();\ndiff --git a/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al b/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n--- a/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n+++ b/src/Apps/W1/Subscription Billing/App/Billing/Codeunits/BillingProposal.Codeunit.al\n@@ -557,7 +557,10 @@ codeunit 8062 \"Billing Proposal\"\n ClearBillingProposalOptionsMySuggestionsOnlyTxt: Label 'All billing proposals (user %1 only), Only current billing template proposal', Comment = '%1: User ID';\n ClearBillingProposalQst: Label 'Which billing proposal(s) should be deleted?';\n StrMenuResponse: Integer;\n+ Counter: Integer;\n+ ProgressWindow: Dialog;\n+ DeletingBillingLinesMsg: Label 'Deleted billing lines: #1######';\n begin\n DisplayErrorIfNotAuthorizedToClearProposalOrDeleteDocuments();\n BillingTemplate.Get(BillingTemplateCode);\n if BillingTemplate.\"My Suggestions Only\" then\n@@ -575,12 +578,26 @@ codeunit 8062 \"Billing Proposal\"\n BillingLine.SetRange(Partner, BillingTemplate.Partner);\n if BillingTemplate.\"My Suggestions Only\" then\n BillingLine.SetRange(\"User ID\", UserId());\n- BillingLine.DeleteAll(true);\n+ ProgressWindow.Open(DeletingBillingLinesMsg);\n+ if BillingLine.FindSet() then\n+ repeat\n+ Counter += 1;\n+ ProgressWindow.Update(1, Counter);\n+ BillingLine.Delete(true);\n+ until BillingLine.Next() = 0;\n+ ProgressWindow.Close();\n end;\n 2:\n begin\n BillingLine.SetRange(\"Billing Template Code\", BillingTemplate.Code);\n- BillingLine.DeleteAll(true);\n+ ProgressWindow.Open(DeletingBillingLinesMsg);\n+ if BillingLine.FindSet() then\n+ repeat\n+ Counter += 1;\n+ ProgressWindow.Update(1, Counter);\n+ BillingLine.Delete(true);\n+ until BillingLine.Next() = 0;\n+ ProgressWindow.Close();\n end;\n end;\n end;\n", "expected_comments": [], "category": "code-review", "description": "False-positive boundary: the replaced operation is DeleteAll(true), and the target Billing Line table visibly has substantive OnDelete work that forces the bulk call to execute row by row. The explicit FindSet/Delete(true) loop preserves that trigger behavior while adding per-row progress; this narrow bulk-regression case is not a generic progress-dialog exemption.", "expect_findings": false, "source": "vsoadmin"}
{"repo":"microsoft/BCApps","instance_id":"synthetic__events-ishandled-added-to-shipped-event-01","base_commit":"9c19e09197bc23a6fdbc74bed36ba592f6c30e08","created_at":"2026-08-31T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"events","articles":[]},"patch":"diff --git a/src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al b/src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al\nindex 8070179cca..3e56413041 100644\n--- a/src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al\n+++ b/src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al\n@@ -1048,8 +1048,13 @@ table 5767 \"Warehouse Activity Line\"\n OutstandingQtyCannotbeLessThanZeroErr: Label 'Outstanding Qty. base cannot be less than 0.';\n \n procedure CalcQty(QtyBase: Decimal): Decimal\n+ var\n+ NewQtyBase: Decimal;\n+ IsHandled: Boolean;\n begin\n- OnBeforeCalcQty(Rec, QtyBase);\n+ OnBeforeCalcQty(Rec, QtyBase, NewQtyBase, IsHandled);\n+ if IsHandled then\n+ exit(Round(NewQtyBase, UOMMgt.QtyRndPrecision()));\n TestField(\"Qty. per Unit of Measure\");\n exit(Round(QtyBase / \"Qty. per Unit of Measure\", UOMMgt.QtyRndPrecision()));\n end;\n@@ -3464,7 +3469,7 @@ table 5767 \"Warehouse Activity Line\"\n end;\n \n [IntegrationEvent(false, false)]\n- local procedure OnBeforeCalcQty(var WarehouseActivityLine: Record \"Warehouse Activity Line\"; QtyBase: Decimal)\n+ local procedure OnBeforeCalcQty(var WarehouseActivityLine: Record \"Warehouse Activity Line\"; QtyBase: Decimal; var NewQtyBase: Decimal; var IsHandled: Boolean)\n begin\n end;\n \n","expected_comments":[{"file":"src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al","line_start":3472,"line_end":3472,"severity":"medium","domain":"events","body":"OnBeforeCalcQty already shipped as a plain notification event with the signature (var WarehouseActivityLine: Record \"Warehouse Activity Line\"; QtyBase: Decimal). Appending var NewQtyBase: Decimal and var IsHandled: Boolean converts it into an overridable seam. Existing subscribers keep binding, because AL binds event parameters by name and a subscriber may omit parameters it does not declare, so nothing fails to compile - but the contract they were written against changed underneath them: any subscriber can now set IsHandled and suppress the base rounding for every existing caller of CalcQty. Leave the shipped event untouched and publish a separate OnBefore event that carries NewQtyBase and IsHandled at the point you want to make overridable, so the notification contract and the override seam stay distinct.","articles":["events/do-not-add-ishandled-to-an-existing-event"]}],"ignored_comments":[{"file":"src/Layers/W1/BaseApp/Warehouse/Activity/WarehouseActivityLine.Table.al","line_start":1056,"line_end":1057,"severity":"low","domain":"events","body":"Acceptable but not required: the new IsHandled guard also skips TestField(\"Qty. per Unit of Measure\"). CalcQty is a pure, side-effect-free calculation, and events/do-not-bypass-critical-operations-with-ishandled explicitly permits an IsHandled guard around such a calculation, so this observation must not be scored as a false positive nor demanded for recall.","articles":["events/do-not-bypass-critical-operations-with-ishandled"]}],"category":"code-review","description":"Real BCApps change that appends var NewQtyBase and var IsHandled to the already-shipped local IntegrationEvent OnBeforeCalcQty on table 5767, converting a notification event into an overridable seam. Binding is unaffected (AL binds event parameters by name), so the finding is the contract change, not a compile or binding break. The IsHandled guard skipping TestField is carried as an ignored comment because the guarded body is a pure calculation, which do-not-bypass-critical-operations-with-ishandled permits. The fresh local IsHandled needs no explicit reset, consistent with synthetic__events-ishandled-reset-boundary-01, so an initialize-IsHandled finding here is a false positive.","expect_findings":true,"source":"handcrafted"}
{"repo":"microsoft/BCApps","instance_id":"synthetic__upgrade-demo-data-not-upgraded-01","base_commit":"b149af232025ef8709bf1d3b6de175a48eabc8e6","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"upgrade"},"patch":"diff --git a/src/DemoAgentContosoModule.Codeunit.al b/src/DemoAgentContosoModule.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/DemoAgentContosoModule.Codeunit.al\n@@ -0,0 +1,19 @@\n+codeunit 50130 \"Demo Agent Contoso Module\" implements \"Contoso Demo Data Module\"\n+{\n+ procedure CreateSetupData()\n+ begin\n+ end;\n+\n+ procedure CreateMasterData()\n+ begin\n+ Codeunit.Run(Codeunit::\"Create Demo VAT Rates\");\n+ end;\n+\n+ procedure CreateTransactionalData()\n+ begin\n+ end;\n+\n+ procedure CreateHistoricalData()\n+ begin\n+ end;\n+}\ndiff --git a/src/CreateDemoVATRates.Codeunit.al b/src/CreateDemoVATRates.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/CreateDemoVATRates.Codeunit.al\n@@ -0,0 +1,15 @@\n+codeunit 50131 \"Create Demo VAT Rates\"\n+{\n+ procedure InsertDefaultRates()\n+ var\n+ VATPostingSetup: Record \"VAT Posting Setup\";\n+ begin\n+ if not VATPostingSetup.Get('DOMESTIC', 'VAT25') then begin\n+ VATPostingSetup.Init();\n+ VATPostingSetup.Validate(\"VAT Bus. Posting Group\", 'DOMESTIC');\n+ VATPostingSetup.Validate(\"VAT Prod. Posting Group\", 'VAT25');\n+ VATPostingSetup.\"VAT %\" := 25;\n+ VATPostingSetup.Insert();\n+ end;\n+ end;\n+}\n","expected_comments":[{"file":"src/DemoAgentContosoModule.Codeunit.al","line_start":9,"line_end":9,"severity":"medium","domain":"upgrade","body":"The new VAT-rate seeding step (Codeunit.Run(Codeunit::\"Create Demo VAT Rates\")) is wired only into \"Demo Agent Contoso Module\".CreateMasterData(), which the demo-data tool invokes during setup/demo-data creation, not during extension upgrade. \"Create Demo VAT Rates\" inserts persisted \"VAT Posting Setup\" records, so a tenant that provisioned this demo data before this change will not receive the new rows on upgrade: CreateMasterData is never re-run by OnUpgradePerCompany/OnUpgradePerDatabase, and an install-subtype-style entry point does not run on a version upgrade either. If these records must reach already-provisioned tenants, add a Subtype = Upgrade codeunit that calls the same insert logic from OnUpgradePerCompany.","articles":["upgrade/upgrade-codeunit-subtype","upgrade/install-code-does-not-run-on-version-upgrade"]}],"category":"code-review","description":"Calibration case (BCApps PR 10990, discussion r3926331607, THUMBS_DOWN): a Contoso demo-data module wires a new persisted VAT-rate seeding codeunit only into CreateMasterData() -- the demo-data tool's setup entry point -- with no Subtype = Upgrade codeunit to propagate the new VAT Posting Setup records to tenants that already created this demo data. The maintainer rejected the review bot's equivalent finding with 'Not needed here, this is Preview demo data', which is a product/process judgment call about Contoso/demo-data tooling specifically, not a confirmation that persisted data behind a non-upgrade entry point is harmless in general: the underlying technical gap (new setup/master data inserted only from a non-upgrade trigger) is unchanged and is kept as the expected finding here. This entry does not assert that the source PR's rejection was wrong, and does not encode a clean-negative guard for demo-data modules; it preserves the finding for calibration against that disagreement rather than inventing either a confirmed defect-free pattern or a new precondition.","expect_findings":true,"source":"vsoadmin"}
Loading