Dear %1,
%2
', Locked = true;\n+\n+ internal procedure BuildNotificationBody(EmployeeName: Text; Description: Text): Text\n+ begin\n+ exit(StrSubstNo(BodyTemplateTok, EmployeeName, Description));\n+ end;\n+}\n", "expected_comments": [{"file": "src/ExpenseHtmlNotifier.Codeunit.al", "line_start": 10, "line_end": 10, "domain": "security", "severity": "high", "body": "User-supplied EmployeeName and Description are substituted into the HTML body without encoding, enabling stored or reflected XSS. HTML-encode the values before embedding them.", "article": "security/al-has-no-built-in-htmlencode"}], "category": "code-review", "description": "True positive security findings: xss (user-supplied data embedded in HTML without encoding)", "expect_findings": true, "source": "vsoadmin"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__security-014","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-05-15T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"security"},"patch":"diff --git a/src/SecureOperationHelper.Codeunit.al b/src/SecureOperationHelper.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/SecureOperationHelper.Codeunit.al\n@@ -0,0 +1,13 @@\n+codeunit 50105 \"Secure Operation Helper\"\n+{\n+ Access = Internal;\n+\n+ internal procedure DeleteAllRecords(TableNo: Integer)\n+ var\n+ RecRef: RecordRef;\n+ begin\n+ RecRef.Open(TableNo);\n+ RecRef.DeleteAll();\n+ RecRef.Close();\n+ end;\n+}\n","expected_comments":[{"file":"src/SecureOperationHelper.Codeunit.al","line_start":9,"line_end":10,"domain":"security","severity":"high","body":"A caller-provided table number is opened with RecordRef.Open and then DeleteAll is called, letting any caller delete every record in an arbitrary table. Restrict the allowed tables and enforce permission checks.","articles":["security/recordref-open-with-caller-table-must-not-be-public"]}],"category":"code-review","description":"True positive: public procedure uses RecordRef.Open with caller-provided table number, allowing any extension to delete all records from any table through this codeunit's permissions","expect_findings":true,"source":"vsoadmin"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__security-015","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-05-15T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"security"},"patch":"diff --git a/src/ExternalIntegrationMgt.Codeunit.al b/src/ExternalIntegrationMgt.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/ExternalIntegrationMgt.Codeunit.al\n@@ -0,0 +1,24 @@\n+namespace Microsoft.Integration.Partner;\n+\n+codeunit 50205 \"External Integration Mgt.\"\n+{\n+ Access = Internal;\n+\n+ procedure PostToPartner(EndpointUrl: Text): Boolean\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ begin\n+ Content.WriteFrom('{}');\n+ exit(Client.Post(EndpointUrl, Content, Response));\n+ end;\n+\n+ procedure GetFromProvider(EndpointUrl: Text): Boolean\n+ var\n+ Client: HttpClient;\n+ Response: HttpResponseMessage;\n+ begin\n+ exit(Client.Get(EndpointUrl, Response));\n+ end;\n+}\n","expected_comments":[{"file":"src/ExternalIntegrationMgt.Codeunit.al","line_start":14,"line_end":14,"domain":"security","severity":"high","body":"A caller-supplied URL is passed to HttpClient.Post without host or scheme validation, allowing SSRF requests to internal or attacker-chosen endpoints.","articles":["security/validate-user-configurable-urls"]},{"file":"src/ExternalIntegrationMgt.Codeunit.al","line_start":22,"line_end":22,"domain":"security","severity":"high","body":"A caller-supplied URL is passed to HttpClient.Get without host or scheme validation, allowing SSRF requests to internal or attacker-chosen endpoints.","articles":["security/validate-user-configurable-urls"]}],"category":"code-review","description":"True positive security findings: URLs from table fields used in HTTP requests without validation (SSRF risk). Three procedures use user-configurable URLs directly, while two procedures correctly validate using Uri.AreURIsHaveSameHost and Uri.IsValidURIPattern.","expect_findings":true,"source":"vsoadmin"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__security-016","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-05-15T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"security"},"patch":"diff --git a/src/ExpenseHtmlNotifier.Codeunit.al b/src/ExpenseHtmlNotifier.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/ExpenseHtmlNotifier.Codeunit.al\n@@ -0,0 +1,12 @@\n+codeunit 50900 \"Expense Html Notifier\"\n+{\n+ Access = Internal;\n+\n+ var\n+ BodyTemplateTok: Label 'Dear %1,
%2
', Locked = true;\n+\n+ internal procedure BuildNotificationBody(EmployeeName: Text; Description: Text): Text\n+ begin\n+ exit(StrSubstNo(BodyTemplateTok, EmployeeName, Description));\n+ end;\n+}\n","expected_comments":[{"file":"src/ExpenseHtmlNotifier.Codeunit.al","line_start":10,"line_end":10,"domain":"security","severity":"high","body":"User-supplied EmployeeName and Description are substituted into the HTML body without encoding, enabling stored or reflected XSS. HTML-encode the values before embedding them.","articles":["security/al-has-no-built-in-htmlencode"]}],"category":"code-review","description":"True positive security findings: xss (user-supplied data embedded in HTML without encoding)","expect_findings":true,"source":"vsoadmin"} {"repo": "microsoft/BCApps", "instance_id": "synthetic__performance-001", "base_commit": "70fd0246a0a4dbc72cb183ca719106722c03be4d", "created_at": "2026-05-15T00:00:00Z", "environment_setup_version": "27.0", "project_paths": [], "metadata": {"area": "performance"}, "patch": "diff --git a/src/FADepreciationBook.Table.al b/src/FADepreciationBook.Table.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/FADepreciationBook.Table.al\n@@ -0,0 +1,78 @@\n+table 50200 \"FA Depreciation Book FP\"\n+{\n+ DataClassification = CustomerContent;\n+\n+ fields\n+ {\n+ field(1; \"FA No.\"; Code[20])\n+ {\n+ Caption = 'FA No.';\n+ TableRelation = \"Fixed Asset\";\n+ }\n+\n+ field(2; \"Depreciation Book Code\"; Code[10])\n+ {\n+ Caption = 'Depreciation Book Code';\n+ TableRelation = \"Depreciation Book\";\n+ }\n+\n+ field(3; Depreciation; Decimal)\n+ {\n+ FieldClass = FlowField;\n+ CalcFormula = sum(\"FA Ledger Entry\".Amount where(\"FA No.\" = field(\"FA No.\"),\n+ \"Depreciation Book Code\" = field(\"Depreciation Book Code\"),\n+ \"FA Posting Category\" = const(Depreciation)));\n+ Caption = 'Depreciation';\n+ }\n+\n+ field(4; \"Bonus Depr. Applied Amount\"; Decimal)\n+ {\n+ FieldClass = FlowField;\n+ CalcFormula = sum(\"FA Ledger Entry\".Amount where(\"FA No.\" = field(\"FA No.\"),\n+ \"Depreciation Book Code\" = field(\"Depreciation Book Code\"),\n+ \"FA Posting Type\" = const(\"Bonus Depreciation\")));\n+ Caption = 'Bonus Depr. Applied Amount';\n+ }\n+\n+ field(5; \"Use Half-Year Convention\"; Boolean)\n+ {\n+ Caption = 'Use Half-Year Convention';\n+\n+ trigger OnValidate()\n+ var\n+ CannotChangeHalfYearErr: Label 'Cannot change half-year convention after depreciation has been posted.';\n+ CannotChangeBonusErr: Label 'Cannot change half-year convention when bonus depreciation has been applied.';\n+ begin\n+ // CORRECT: CalcFields in OnValidate runs once per user edit, not in a loop\n+ // This is appropriate for validation logic that needs current flowfield values\n+ CalcFields(Depreciation);\n+ if Depreciation <> 0 then\n+ Error(CannotChangeHalfYearErr);\n+\n+ CalcFields(\"Bonus Depr. Applied Amount\");\n+ if \"Bonus Depr. Applied Amount\" <> 0 then\n+ Error(CannotChangeBonusErr);\n+ end;\n+ }\n+\n+ field(6; \"Depreciation Method\"; Option)\n+ {\n+ OptionCaption = 'Straight-Line,Declining-Balance 1,Declining-Balance 2';\n+ OptionMembers = \"Straight-Line\",\"Declining-Balance 1\",\"Declining-Balance 2\";\n+ Caption = 'Depreciation Method';\n+ }\n+\n+ field(7; \"Starting Date\"; Date)\n+ {\n+ Caption = 'Depreciation Starting Date';\n+ }\n+ }\n+\n+ keys\n+ {\n+ key(PK; \"FA No.\", \"Depreciation Book Code\")\n+ {\n+ Clustered = true;\n+ }\n+ }\n+}\ndiff --git a/src/SalesOrderCard.Page.al b/src/SalesOrderCard.Page.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/SalesOrderCard.Page.al\n@@ -0,0 +1,87 @@\n+page 50201 \"Sales Order Card FP\"\n+{\n+ PageType = Card;\n+ SourceTable = \"Sales Header\";\n+ Caption = 'Sales Order Card FP';\n+\n+ layout\n+ {\n+ area(Content)\n+ {\n+ group(General)\n+ {\n+ Caption = 'General';\n+\n+ field(\"No.\"; Rec.\"No.\")\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the number of the sales order.';\n+ }\n+\n+ field(\"Sell-to Customer No.\"; Rec.\"Sell-to Customer No.\")\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the number of the customer who will receive the products on the sales order.';\n+ }\n+\n+ field(\"Document Date\"; Rec.\"Document Date\")\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the date when the sales order was created.';\n+ }\n+\n+ field(\"Total Amount\"; TotalAmount)\n+ {\n+ Caption = 'Total Amount Including VAT';\n+ ApplicationArea = All;\n+ Editable = false;\n+ ToolTip = 'Specifies the total amount including VAT for the sales order.';\n+ }\n+ }\n+ }\n+ }\n+\n+ actions\n+ {\n+ area(Processing)\n+ {\n+ action(RefreshTotals)\n+ {\n+ Caption = 'Refresh Totals';\n+ ApplicationArea = All;\n+ ToolTip = 'Recalculates and refreshes the total amount for the sales order.';\n+ Image = Refresh;\n+\n+ trigger OnAction()\n+ var\n+ TotalRefreshedMsg: Label 'Total refreshed: %1', Comment = '%1 = total amount including VAT';\n+ begin\n+ // CORRECT: Manual refresh action - user-initiated, runs once\n+ Rec.CalcFields(\"Amount Including VAT\");\n+ TotalAmount := Rec.\"Amount Including VAT\";\n+ Message(TotalRefreshedMsg, TotalAmount);\n+ end;\n+ }\n+ }\n+ }\n+\n+ var\n+ TotalAmount: Decimal;\n+\n+ // CORRECT: OnAfterGetCurrRecord fires once per record selection, not per row\n+ // This is the appropriate place to calculate values when user navigates to a record\n+ trigger OnAfterGetCurrRecord()\n+ begin\n+ // Calculate total amount when user selects a sales order\n+ // This runs once when the record is loaded/selected, not in a loop\n+ Rec.CalcFields(\"Amount Including VAT\");\n+ TotalAmount := Rec.\"Amount Including VAT\";\n+ end;\n+\n+ trigger OnNewRecord(BelowxRec: Boolean)\n+ begin\n+ // CORRECT: Initialize values for new record - runs once per new record creation\n+ TotalAmount := 0;\n+ Rec.\"Document Date\" := WorkDate();\n+ end;\n+}\ndiff --git a/src/CustLedgerEntryAggregator.Codeunit.al b/src/CustLedgerEntryAggregator.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/CustLedgerEntryAggregator.Codeunit.al\n@@ -0,0 +1,20 @@\n+codeunit 50202 \"Cust Ledger Entry Aggregator\"\n+{\n+ procedure SumOpenRemainingAmount(CustomerNo: Code[20]): Decimal\n+ var\n+ CustLedgerEntry: Record \"Cust. Ledger Entry\";\n+ Customer: Record Customer;\n+ Total: Decimal;\n+ begin\n+ CustLedgerEntry.SetRange(\"Customer No.\", CustomerNo);\n+ CustLedgerEntry.SetRange(Open, true);\n+ if CustLedgerEntry.FindSet() then\n+ repeat\n+ CustLedgerEntry.CalcFields(\"Remaining Amount\");\n+ Customer.Get(CustLedgerEntry.\"Customer No.\");\n+ if Customer.\"Application Method\" = Customer.\"Application Method\"::Manual then\n+ Total += CustLedgerEntry.\"Remaining Amount\";\n+ until CustLedgerEntry.Next() = 0;\n+ exit(Total);\n+ end;\n+}\n", "expected_comments": [{"file": "src/CustLedgerEntryAggregator.Codeunit.al", "line_start": 13, "line_end": 13, "body": "CalcFields(\"Remaining Amount\") inside a repeat..until loop over \"Cust. Ledger Entry\" (up to 10M rows) issues one SQL query per iteration — classic N+1 against a hot table. — Replace the loop with `CustLedgerEntry.CalcSums(\"Remaining Amount\")` which executes as a single SUM query, or use a SIFT-backed key.", "severity": "high", "domain": "performance"}, {"file": "src/CustLedgerEntryAggregator.Codeunit.al", "line_start": 14, "line_end": 14, "body": "Customer.Get(CustLedgerEntry.\"Customer No.\") inside a repeat..until over Cust. Ledger Entry is an N+1 query: one Customer lookup per ledger row, redundant since every row already has the same Customer No. (the loop is filtered by CustomerNo). — Move the Customer.Get above the loop (single lookup), and add `Customer.SetLoadFields(\"Application Method\")` since only one field is read.", "severity": "high", "domain": "performance"}], "category": "code-review", "description": "False positive performance findings: calcfields_false_positive (30 false positives). Agent flagged these but reviewers rejected them. Enriched with 2 true-positive findings in addition to the false-positive bait.", "expect_findings": true, "source": "vsoadmin"} {"repo": "microsoft/BCApps", "instance_id": "synthetic__performance-002", "base_commit": "70fd0246a0a4dbc72cb183ca719106722c03be4d", "created_at": "2026-05-15T00:00:00Z", "environment_setup_version": "27.0", "project_paths": [], "metadata": {"area": "performance"}, "patch": "diff --git a/src/SetupReader.Codeunit.al b/src/SetupReader.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/SetupReader.Codeunit.al\n@@ -0,0 +1,67 @@\n+codeunit 50211 \"Setup Reader\"\n+{\n+ procedure GetSetupValues()\n+ var\n+ GLSetup: Record \"General Ledger Setup\";\n+ SalesSetup: Record \"Sales & Receivables Setup\";\n+ InventorySetup: Record \"Inventory Setup\";\n+ PurchSetup: Record \"Purchases & Payables Setup\";\n+ begin\n+ // CORRECT: Setup tables typically have only 1 record per company\n+ // Any access pattern (Get, FindSet, FindFirst) is fine for singleton tables\n+ GLSetup.Get();\n+ SalesSetup.Get();\n+ InventorySetup.Get();\n+ PurchSetup.Get();\n+\n+ if GLSetup.\"Additional Reporting Currency\" <> '' then\n+ ProcessACYSettings(GLSetup);\n+\n+ if SalesSetup.\"Credit Warnings\" <> SalesSetup.\"Credit Warnings\"::\"No Warning\" then\n+ EnableCreditWarnings(SalesSetup);\n+ end;\n+\n+ procedure ValidateCompanySettings(): Boolean\n+ var\n+ CompanyInfo: Record \"Company Information\";\n+ begin\n+ // CORRECT: Company Information is a singleton table (1 record per company)\n+ // Get() is the appropriate method for singleton tables\n+ if not CompanyInfo.Get() then\n+ exit(false);\n+\n+ if CompanyInfo.Name = '' then\n+ exit(false);\n+\n+ if CompanyInfo.\"Country/Region Code\" = '' then\n+ exit(false);\n+\n+ exit(true);\n+ end;\n+\n+ procedure GetUserSetupForCurrentUser(var UserSetup: Record \"User Setup\"): Boolean\n+ begin\n+ // CORRECT: Looking up single user's setup record\n+ // Get() with UserId is appropriate for single-record lookup\n+ UserSetup.Reset();\n+ if UserSetup.Get(UserId) then\n+ exit(true);\n+ exit(false);\n+ end;\n+\n+ local procedure ProcessACYSettings(GLSetup: Record \"General Ledger Setup\")\n+ var\n+ ACYEnabledMsg: Label 'ACY is enabled: %1', Comment = '%1 = additional reporting currency';\n+ begin\n+ // Process additional currency settings\n+ Message(ACYEnabledMsg, GLSetup.\"Additional Reporting Currency\");\n+ end;\n+\n+ local procedure EnableCreditWarnings(SalesSetup: Record \"Sales & Receivables Setup\")\n+ var\n+ CreditWarningsEnabledMsg: Label 'Credit warnings are enabled';\n+ begin\n+ // Enable credit warning processing\n+ Message(CreditWarningsEnabledMsg);\n+ end;\n+}\ndiff --git a/src/TempBufferProcessor.Codeunit.al b/src/TempBufferProcessor.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/TempBufferProcessor.Codeunit.al\n@@ -0,0 +1,61 @@\n+codeunit 50210 \"Temp Buffer Processor\"\n+{\n+ procedure ProcessBufferEntries(var TempBuffer: Record \"Integer\" temporary)\n+ var\n+ ProcessedCount: Integer;\n+ TotalAmount: Decimal;\n+ ProcessedEntriesMsg: Label 'Processed %1 entries with total %2', Comment = '%1 = number of entries, %2 = total amount';\n+ begin\n+ // CORRECT: TempBuffer is temporary — all operations are in-memory, no SQL queries\n+ // Any access pattern (FindSet, Get, loops) on temp tables is performant\n+ ProcessedCount := 0;\n+ TotalAmount := 0;\n+\n+ if TempBuffer.FindSet() then\n+ repeat\n+ // This might look suspicious, but it's CORRECT because:\n+ // 1. TempBuffer is temporary (in-memory)\n+ // 2. No database round trips are happening\n+ // 3. All data is already loaded in memory\n+ TotalAmount += TempBuffer.Number;\n+ ProcessedCount += 1;\n+\n+ // Even modifying temp records in a loop is fine\n+ TempBuffer.Number := TempBuffer.Number * 2;\n+ TempBuffer.Modify();\n+\n+ until TempBuffer.Next() = 0;\n+\n+ Message(ProcessedEntriesMsg, ProcessedCount, TotalAmount);\n+ end;\n+\n+ procedure BuildTempData(var TempBuffer: Record \"Integer\" temporary)\n+ var\n+ i: Integer;\n+ begin\n+ // CORRECT: Building temp data - all operations are in-memory\n+ TempBuffer.Reset();\n+ TempBuffer.DeleteAll();\n+\n+ for i := 1 to 100 do begin\n+ TempBuffer.Init();\n+ TempBuffer.Number := Random(1000);\n+ TempBuffer.Insert();\n+ end;\n+ end;\n+\n+ procedure FindMaxValue(var TempBuffer: Record \"Integer\" temporary): Integer\n+ var\n+ MaxValue: Integer;\n+ begin\n+ // CORRECT: Finding max in temp table - no performance concern\n+ MaxValue := 0;\n+ if TempBuffer.FindSet() then\n+ repeat\n+ if TempBuffer.Number > MaxValue then\n+ MaxValue := TempBuffer.Number;\n+ until TempBuffer.Next() = 0;\n+\n+ exit(MaxValue);\n+ end;\n+}\ndiff --git a/src/CustomerLookup.Codeunit.al b/src/CustomerLookup.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/CustomerLookup.Codeunit.al\n@@ -0,0 +1,20 @@\n+codeunit 50212 \"Customer Lookup\"\n+{\n+ procedure GetCustomerName(CustomerNo: Code[20]): Text[100]\n+ var\n+ Customer: Record Customer;\n+ begin\n+ Customer.SetRange(\"No.\", CustomerNo);\n+ if Customer.FindFirst() then\n+ exit(Customer.Name);\n+ exit('');\n+ end;\n+\n+ procedure HasCustomersInCountry(CountryRegionCode: Code[10]): Boolean\n+ var\n+ Customer: Record Customer;\n+ begin\n+ Customer.SetRange(\"Country/Region Code\", CountryRegionCode);\n+ exit(Customer.Count() > 0);\n+ end;\n+}\n", "expected_comments": [{"file": "src/CustomerLookup.Codeunit.al", "line_start": 8, "line_end": 8, "body": "FindFirst() after SetRange on the full primary key (\"No.\") of Customer (up to 800k rows). This still does a SQL SELECT TOP 1 with a range predicate instead of a direct key lookup. — Replace with `if Customer.Get(CustomerNo) then exit(Customer.Name);` which is a direct PK lookup (CodeCop AA0233).", "severity": "medium", "domain": "performance"}, {"file": "src/CustomerLookup.Codeunit.al", "line_start": 18, "line_end": 18, "body": "Count() > 0 on Customer (up to 800k rows) for a pure existence check. Count() materializes a SQL COUNT(*) over the filtered set instead of stopping at the first matching row. — Replace with `exit(not Customer.IsEmpty());` which stops at the first match and is significantly cheaper on large tables.", "severity": "medium", "domain": "performance"}], "category": "code-review", "description": "False positive performance findings: findset_false_positive (69 false positives). Agent flagged these but reviewers rejected them. Enriched with 2 true-positive findings in addition to the false-positive bait.", "expect_findings": true, "source": "vsoadmin"} {"repo": "microsoft/BCApps", "instance_id": "synthetic__performance-003", "base_commit": "70fd0246a0a4dbc72cb183ca719106722c03be4d", "created_at": "2026-05-15T00:00:00Z", "environment_setup_version": "27.0", "project_paths": [], "metadata": {"area": "performance"}, "patch": "diff --git a/src/MigrationSetupHandler.Codeunit.al b/src/MigrationSetupHandler.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/MigrationSetupHandler.Codeunit.al\n@@ -0,0 +1,18 @@\n+codeunit 50221 \"Migration Setup Handler\"\n+{\n+ procedure CountMigratablePermissionSets(): Integer\n+ var\n+ PermissionSet: Record \"Permission Set\";\n+ begin\n+ PermissionSet.SetFilter(\"Role ID\", '%1|%2', 'D365 BASIC', 'D365 READ');\n+ exit(PermissionSet.Count());\n+ end;\n+\n+ procedure CountObsoleteRegisters(): Integer\n+ var\n+ DateComprRegister: Record \"Date Compr. Register\";\n+ begin\n+ DateComprRegister.SetFilter(\"Ending Date\", '<%1', CalcDate('<-2Y>', Today));\n+ exit(DateComprRegister.Count());\n+ end;\n+}\ndiff --git a/src/PermissionSetListOverview.Page.al b/src/PermissionSetListOverview.Page.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/PermissionSetListOverview.Page.al\n@@ -0,0 +1,84 @@\n+page 50220 \"Permission Set List Overview\"\n+{\n+ PageType = List;\n+ ApplicationArea = All;\n+ UsageCategory = Administration;\n+ SourceTable = \"Aggregate Permission Set\";\n+ Caption = 'Permission Set List Overview';\n+ Editable = false;\n+\n+ layout\n+ {\n+ area(Content)\n+ {\n+ repeater(Permissions)\n+ {\n+ field(\"Role ID\"; Rec.\"Role ID\")\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the identifier of the permission set.';\n+ }\n+\n+ field(Name; Rec.Name)\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the display name of the permission set.';\n+ }\n+\n+ field(Scope; Rec.Scope)\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies whether the permission set is defined by the system or by a tenant.';\n+ }\n+\n+ field(\"App Name\"; Rec.\"App Name\")\n+ {\n+ ApplicationArea = All;\n+ ToolTip = 'Specifies the name of the extension that defines the permission set.';\n+ }\n+\n+ field(\"Permission Count\"; PermissionCount)\n+ {\n+ Caption = 'Permission Count';\n+ ApplicationArea = All;\n+ Editable = false;\n+ ToolTip = 'Specifies the number of permissions that belong to the permission set.';\n+ }\n+ }\n+ }\n+ }\n+\n+ actions\n+ {\n+ area(Processing)\n+ {\n+ action(RefreshCounts)\n+ {\n+ Caption = 'Refresh Permission Counts';\n+ ApplicationArea = All;\n+ ToolTip = 'Recalculates the permission count shown for each permission set.';\n+\n+ trigger OnAction()\n+ begin\n+ CurrPage.Update();\n+ end;\n+ }\n+ }\n+ }\n+\n+ var\n+ PermissionCount: Integer;\n+\n+ trigger OnAfterGetRecord()\n+ var\n+ Permission: Record Permission;\n+ begin\n+ Permission.SetRange(\"Role ID\", Rec.\"Role ID\");\n+ PermissionCount := Permission.Count();\n+ end;\n+\n+ trigger OnOpenPage()\n+ begin\n+ Rec.SetFilter(Scope, '%1|%2', Rec.Scope::System, Rec.Scope::Tenant);\n+ end;\n+}\ndiff --git a/src/SalesInvoiceFilter.Codeunit.al b/src/SalesInvoiceFilter.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/SalesInvoiceFilter.Codeunit.al\n@@ -0,0 +1,26 @@\n+codeunit 50222 \"Sales Invoice Filter\"\n+{\n+ procedure ListLinesByDescription(Description: Text[100])\n+ var\n+ SalesInvoiceLine: Record \"Sales Invoice Line\";\n+ begin\n+ SalesInvoiceLine.SetRange(Description, Description);\n+ if SalesInvoiceLine.FindSet() then\n+ repeat\n+ Message('%1 %2', SalesInvoiceLine.\"Document No.\", SalesInvoiceLine.\"Line No.\");\n+ until SalesInvoiceLine.Next() = 0;\n+ end;\n+\n+ procedure SumQuantityByDocument(DocumentNo: Code[20]): Decimal\n+ var\n+ SalesInvoiceLine: Record \"Sales Invoice Line\";\n+ Total: Decimal;\n+ begin\n+ SalesInvoiceLine.SetRange(\"Document No.\", DocumentNo);\n+ if SalesInvoiceLine.FindSet() then\n+ repeat\n+ Total += SalesInvoiceLine.Quantity;\n+ until SalesInvoiceLine.Next() = 0;\n+ exit(Total);\n+ end;\n+}\n", "expected_comments": [{"file": "src/SalesInvoiceFilter.Codeunit.al", "line_start": 7, "line_end": 7, "body": "SetRange on Sales Invoice Line.Description with no SetCurrentKey and no key including Description. Sales Invoice Line is large (up to 3M rows) and the query will table-scan. — Either add `SetCurrentKey` to a key whose leading field matches the filter, or introduce a new key on the source table that covers Description, before filtering.", "severity": "high", "domain": "performance"}, {"file": "src/SalesInvoiceFilter.Codeunit.al", "line_start": 20, "line_end": 20, "body": "FindSet over Sales Invoice Line (3M rows, ~80 fields) loads every field for every row but the loop only reads `Quantity`. — Add `SalesInvoiceLine.SetLoadFields(Quantity);` before SetRange so SQL returns only the Quantity column (plus key fields). Even better, replace the loop with `SalesInvoiceLine.CalcSums(Quantity)` since Quantity is a SumIndexField on this table.", "severity": "medium", "domain": "performance"}, {"file": "src/PermissionSetListOverview.Page.al", "line_start": 77, "line_end": 77, "body": "PermissionCount is computed with Permission.Count() inside OnAfterGetRecord, which fires once per row rendered on this List page (and again while scrolling), so every visible row issues its own SQL COUNT against the Permission table (a per-row N+1 that scales with the number of permission sets shown). — Expose the value as a FlowField on the source table (FieldClass = FlowField, CalcFormula = count(Permission where(\"Role ID\" = field(\"Role ID\")))) so the aggregate is computed by the query engine instead of recomputing it per row.", "severity": "high", "domain": "performance"}], "category": "code-review", "description": "False positive performance findings: index_false_positive (29 false positives). Agent flagged these but reviewers rejected them. Enriched with 2 true-positive findings in addition to the false-positive bait.", "expect_findings": true, "source": "vsoadmin"} diff --git a/docs/code-review.md b/docs/code-review.md index e3c1513f3..59215f557 100644 --- a/docs/code-review.md +++ b/docs/code-review.md @@ -6,7 +6,7 @@ title: Code Review - BC-Bench # Code Review @@ -45,23 +27,21 @@ Unlike the pass/fail categories, code review is scored with **Precision / Recall A gold entry may also declare **`ignored_comments`** — legitimate-but-optional observations (out-of-scope nitpicks, maintainer-judgment calls) that should be neither required nor penalized. Ignored comments are structurally paired against the generated comments and validated by the same LLM judge, in a single judge pass alongside the expected comments. Any generated comment the judge confirms as an ignored match is dropped from scoring entirely: it earns no recall and does not count against precision. Expected always takes precedence, so a comment that could match both is credited as a real find; a comment that does not hold up as an expected match can still be neutralized as ignored rather than counting as a false positive. Most entries leave `ignored_comments` empty, which scores identically to before. -## Configuring Engine Experiments +## Category and runners + +`code-review` is the evaluation contract: it owns the dataset, structured `review.json` output, scorer, result schema, and leaderboard schema. A runner is the system under test. The same entries can be evaluated through the generic GitHub Copilot CLI and Claude Code runners, allowing direct cross-system comparisons under one scorer. -Code Review runs the production BC-ALAgents generate path. A BC-Bench experiment branch can independently select BC-ALAgents and BCQuality sources in `src/bcbench/agent/shared/config.yaml`: +BC PR Review is a separate agent harness fixed to the `code-review` category. It runs the production BC-ALAgents review engine with BCQuality, while generic Copilot and Claude runners continue to use their own prompts and configuration: -```yaml -pr_review: - engine: - repo: microsoft/BC-ALAgents - ref: main - local_path: null - bcquality: - repo: microsoft/BCQuality - ref: main - local_path: null +```text +bcbench evaluate copilotNo results available yet. Check back soon!
{% endif %} +## Performance Leaderboard + +{% if site.data.code-review.aggregate and site.data.code-review.aggregate.size > 0 %} +| Agent | +Model | +Avg Time | +Avg Total Tokens | +Avg API Calls | +Avg AI Credits | +Avg Premium Requests | +Complete Usage | +Avg Knowledge Files | +Avg Knowledge Pruned | +Ver | +
|---|---|---|---|---|---|---|---|---|---|---|
| {{ agg.agent_name }} | +{{ agg.model }} | +{{ agg.average_duration | round: 1 }}s | +{% if agg.average_total_tokens != null %}{{ agg.average_total_tokens | round: 0 }}{% else %}—{% endif %} | +{% if agg.average_api_calls != null %}{{ agg.average_api_calls | round: 1 }}{% else %}—{% endif %} | +{% if agg.average_ai_credits != null %}{{ agg.average_ai_credits | round: 4 }}{% else %}—{% endif %} | +{% if agg.average_premium_requests != null %}{{ agg.average_premium_requests | round: 4 }}{% else %}—{% endif %} | +{% if agg.structured_usage_complete_rate != null %}{{ agg.structured_usage_complete_rate | times: 100.0 | round: 1 }}%{% else %}—{% endif %} | +{% if agg.average_knowledge_files != null %}{{ agg.average_knowledge_files | round: 1 }}{% else %}—{% endif %} | +{% if agg.average_knowledge_pruned != null %}{{ agg.average_knowledge_pruned | round: 1 }}{% else %}—{% endif %} | +{{ agg.benchmark_version }} | +
No performance results available yet. Check back soon!
+{% endif %} + ## Experiment Leaderboard Compares review-knowledge configurations for the same model (see the Baseline Leaderboard above for the plain agent): - **Inline knowledge (pre-#8700)** — the review checklists BCApps shipped inline before adopting BCQuality, injected as custom instructions. -- **PR-review engine** — BC-ALAgents runs against a configured BCQuality revision, with performance and context-filtering metrics captured alongside review quality. {% assign experiment_rows = site.data.code-review.aggregate | where_exp: "agg", "agg.experiment" %} {% if experiment_rows and experiment_rows.size > 0 %} -{% assign experiment_results = experiment_rows | sort: "f1" | reverse %} -| Variant | -Engine / BCQuality | -Agent | -Model | -Micro F1 (95% CI) | -Macro F1 (95% CI) | -Precision | -Recall | -Ver | -
|---|---|---|---|---|---|---|---|---|
| {% if agg.experiment.custom_agent == "bc-review-engine" %}PR-review engine{% elsif agg.experiment.custom_instructions %}Inline knowledge (pre-#8700){% else %}Other{% endif %} | -
- {% if agg.experiment.custom_agent == "bc-review-engine" and agg.experiment.plugins %}
- {% for plugin in agg.experiment.plugins %}
- {% assign plugin_parts = plugin | split: "@" %}
- {% if plugin contains "bc-review-engine@" or plugin contains "BCQuality@" %}{{ plugin_parts[0] }}@{{ plugin_parts[1] | slice: 0, 7 }}{% unless forloop.last %} {% endunless %}{% endif %} - {% endfor %} - {% elsif agg.experiment.custom_agent == "bc-review-engine" or agg.experiment.custom_instructions %}self-contained - {% else %}—{% endif %} - |
- {{ agg.agent_name }} | -{{ agg.model }} | -{{ agg.f1 | times: 100.0 | round: 1 }}%{% if agg.f1_ci_low %} ({{ agg.f1_ci_low | times: 100.0 | round: 1 }}-{{ agg.f1_ci_high | times: 100.0 | round: 1 }}%){% endif %} | -{{ agg.macro_f1 | times: 100.0 | round: 1 }}%{% if agg.macro_f1_ci_low %} ({{ agg.macro_f1_ci_low | times: 100.0 | round: 1 }}-{{ agg.macro_f1_ci_high | times: 100.0 | round: 1 }}%){% endif %} | -{{ agg.precision | times: 100.0 | round: 1 }}% | -{{ agg.recall | times: 100.0 | round: 1 }}% | -{{ agg.benchmark_version }} | -
| Variant | -Engine / BCQuality | -Agent | -Model | -Avg Time | -Avg Tokens | -API Calls | -Est. Credits | -Knowledge Used | -Knowledge Pruned | -Ver | -
|---|---|---|---|---|---|---|---|---|---|---|
| {% if agg.experiment.custom_agent == "bc-review-engine" %}PR-review engine{% elsif agg.experiment.custom_instructions %}Inline knowledge (pre-#8700){% else %}Other{% endif %} | -
- {% if agg.experiment.custom_agent == "bc-review-engine" and agg.experiment.plugins %}
- {% for plugin in agg.experiment.plugins %}
- {% assign plugin_parts = plugin | split: "@" %}
- {% if plugin contains "bc-review-engine@" or plugin contains "BCQuality@" %}{{ plugin_parts[0] }}@{{ plugin_parts[1] | slice: 0, 7 }}{% unless forloop.last %} {% endunless %}{% endif %} - {% endfor %} - {% elsif agg.experiment.custom_agent == "bc-review-engine" or agg.experiment.custom_instructions %}self-contained - {% else %}—{% endif %} - |
- {{ agg.agent_name }} | -{{ agg.model }} | -{{ agg.average_duration | round: 1 }}s | -{% if agg.average_total_tokens %}{{ agg.average_total_tokens | round: 0 }}{% else %}—{% endif %} | -{% if agg.average_api_calls %}{{ agg.average_api_calls | round: 1 }}{% else %}—{% endif %} | -{% if agg.average_estimated_credits %}{{ agg.average_estimated_credits | round: 4 }}{% else %}—{% endif %} | -{% if agg.average_knowledge_used %}{{ agg.average_knowledge_used | round: 1 }}{% else %}—{% endif %} | -{% if agg.average_knowledge_pruned %}{{ agg.average_knowledge_pruned | round: 1 }}{% else %}—{% endif %} | -{{ agg.benchmark_version }} | -
| Variant | +Agent | +Model | +Micro F1 (95% CI) | +Macro F1 (95% CI) | +Precision | +Recall | +Avg Time | +Ver | +
|---|---|---|---|---|---|---|---|---|
| + {%- if agg.experiment.custom_instructions -%}Inline knowledge (pre-#8700) + {%- else -%}Other{%- endif -%} + | +{{ agg.agent_name }} | +{{ agg.model }} | +{{ agg.f1 | times: 100.0 | round: 1 }}%{% if agg.f1_ci_low %} ({{ agg.f1_ci_low | times: 100.0 | round: 1 }}-{{ agg.f1_ci_high | times: 100.0 | round: 1 }}%){% endif %} | +{{ agg.macro_f1 | times: 100.0 | round: 1 }}%{% if agg.macro_f1_ci_low %} ({{ agg.macro_f1_ci_low | times: 100.0 | round: 1 }}-{{ agg.macro_f1_ci_high | times: 100.0 | round: 1 }}%){% endif %} | +{{ agg.precision | times: 100.0 | round: 1 }}% | +{{ agg.recall | times: 100.0 | round: 1 }}% | +{{ agg.average_duration | round: 1 }}s | +{{ agg.benchmark_version }} | +
No experiment results available yet. Check back soon!
{% endif %} @@ -226,8 +179,5 @@ Compares review-knowledge configurations for the same model (see the Baseline Le - **Valid output rate** — fraction of tasks whose output parsed into a structured review. Failures score zero on every other metric. (Reported per run.) - **Micro vs. Macro** — *Micro* sums matched, scorable generated (generated minus ignored), and expected across all tasks (tasks with many comments dominate); *Macro* averages per-task scores (every task counts equally). - **95% CI** — confidence interval bootstrapped over the per-task F1 scores, so the leaderboard reports sampling uncertainty even for a single run. The micro `F1` CI resamples runs; the `Macro F1` CI resamples tasks. -- **Avg Tokens / API Calls / Estimated Credits** — mean PR-review engine usage per evaluated entry. Estimated credits use the engine's configured token prices and are not a currency value. -- **Knowledge Used** — mean number of BCQuality knowledge articles remaining after filtering and available to the reviewer. -- **Knowledge Pruned** — mean number of BCQuality knowledge articles removed by the engine's filtering step before review. [← Back to Home](index.md) diff --git a/notebooks/code-review-coverage.ipynb b/notebooks/code-review-coverage.ipynb index 08ca098d5..ec5cf92fe 100644 --- a/notebooks/code-review-coverage.ipynb +++ b/notebooks/code-review-coverage.ipynb @@ -8,8 +8,8 @@ "# Per-article BCQuality coverage (code-review)\n", "\n", "Ad-hoc analysis of how the code-review gold dataset maps onto BCQuality\n", - "knowledge articles. Every finding is annotated with the article it derives\n", - "from (`ReviewComment.article`), and false-positive-guard entries carry their\n", + "knowledge articles. Every finding is annotated with the articles it derives\n", + "from (`ReviewComment.articles`), and false-positive-guard entries carry their\n", "association at entry level (`metadata.articles`). This notebook aggregates\n", "those annotations via `bcbench.analysis.bcquality_article_coverage`.\n", "\n", @@ -28,7 +28,7 @@ "name": "stdout", "output_type": "stream", "text": [ - "9 articles covered across 17/130 annotated entries; 241 of 250 articles have zero coverage\n" + "9 articles covered across 17/144 annotated entries; 241 of 250 articles have zero coverage\n" ] } ], @@ -408,25 +408,36 @@ " - web-services/version-apis-by-adding-not-mutating-published-versions\n", " - web-services/webhook-eligibility-and-validationtoken-renewal\n", "\n", - "Unannotated entries (113):\n", + "Unannotated entries (127):\n", + " - synthetic__accessibility-setselectionfilter-scope-01\n", " - synthetic__breaking-access-modifier-01\n", + " - synthetic__breaking-event-subscriber-suppress-01\n", " - synthetic__breaking-notification-callback-01\n", + " - synthetic__breaking-protected-var-field-01\n", " - synthetic__breaking-relocation-01\n", + " - synthetic__breaking-scope-creep-unrelated-api-01\n", " - synthetic__caption-clean-01\n", " - synthetic__currrec-clean-01\n", " - synthetic__data-modeling-blocked-validation-skip-01\n", " - synthetic__data-modeling-excluded-from-calculation-01\n", + " - synthetic__data-modeling-tablerelation-restriction-mismatch-01\n", " - synthetic__errh-errortype-internal-01\n", " - synthetic__errh-tryfunction-swallowed-01\n", " - synthetic__error-clean-01\n", + " - synthetic__error-handling-assistedit-cancel-01\n", + " - synthetic__error-handling-case-unreachable-else-01\n", + " - synthetic__error-handling-drilldown-position-01\n", + " - synthetic__error-handling-errorinfo-actionable-boundary-01\n", " - synthetic__error-handling-fieldno-swap-01\n", " - synthetic__error-handling-guiallowed-01\n", " - synthetic__error-handling-silent-skip-01\n", " - synthetic__error-testfield-enabled-01\n", + " - synthetic__events-ishandled-reset-boundary-01\n", " - synthetic__get-clean-01\n", " - synthetic__obsolete-clean-01\n", " - synthetic__perf-batched-commit-checkpoint-01\n", " - synthetic__perf-clean-01\n", + " - synthetic__perf-progress-dialog-deleteall-01\n", " - synthetic__performance-001\n", " - synthetic__performance-002\n", " - synthetic__performance-003\n", @@ -502,9 +513,12 @@ " - synthetic__style-clean-03\n", " - synthetic__style-clean-04\n", " - synthetic__style-duplicate-action-01\n", + " - synthetic__style-field-repurpose-indent-01\n", + " - synthetic__style-showmandatory-flowfield-01\n", " - synthetic__style-this-keyword-01\n", " - synthetic__style-tooltip-mismatch-01\n", " - synthetic__testing-tolerance-clean-01\n", + " - synthetic__testing-ui-handler-assert-after-run-01\n", " - synthetic__upgrade-001\n", " - synthetic__upgrade-002\n", " - synthetic__upgrade-003\n", diff --git a/pyproject.toml b/pyproject.toml index e9eaf5c90..a9f6ecab6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "bcbench" -version = "0.8.1" +version = "0.9.0" description = "Benchmarking tool for Business Central (AL) ecosystem, inspired by SWE-Bench" readme = "README.md" requires-python = ">=3.13,<3.14" @@ -21,7 +21,7 @@ dependencies = [ "typer>=0.9.0", "typing-extensions>=4.0", "pyyaml>=6.0", - "pydantic>=2.0", + "pydantic>=2.12", "textual>=7.0", "numpy>=2.3.5", "scipy>=1.16.3", @@ -40,7 +40,7 @@ default = true where = ["src"] [tool.setuptools.package-data] -bcbench = ["agent/*.yaml"] +bcbench = ["agent/*.yaml", "agent/pr_review/scripts/*.ps1"] [tool.pytest.ini_options] testpaths = ["tests"] diff --git a/src/bcbench/agent/__init__.py b/src/bcbench/agent/__init__.py index 984cb7115..89be26b6b 100644 --- a/src/bcbench/agent/__init__.py +++ b/src/bcbench/agent/__init__.py @@ -3,9 +3,6 @@ from bcbench.agent.bcal import BCalBackendConfig, run_bcal_agent from bcbench.agent.claude import run_claude_code from bcbench.agent.copilot import run_copilot_agent +from bcbench.agent.pr_review import run_pr_review_agent -# The AI harnesses are the top-level backends. The code-review category is NOT a fourth -# harness: it runs the Copilot-powered BC-ALAgents review engine, whose backend lives under -# the copilot package (bcbench.agent.copilot.pr_review.run_pr_review_agent) and is reached -# only through the dedicated `code-review` command, not by picking a harness here. -__all__ = ["BCalBackendConfig", "run_bcal_agent", "run_claude_code", "run_copilot_agent"] +__all__ = ["BCalBackendConfig", "run_bcal_agent", "run_claude_code", "run_copilot_agent", "run_pr_review_agent"] diff --git a/src/bcbench/agent/copilot/pr_review/__init__.py b/src/bcbench/agent/copilot/pr_review/__init__.py deleted file mode 100644 index c5871df5e..000000000 --- a/src/bcbench/agent/copilot/pr_review/__init__.py +++ /dev/null @@ -1,3 +0,0 @@ -from bcbench.agent.copilot.pr_review.agent import run_pr_review_agent - -__all__ = ["run_pr_review_agent"] diff --git a/src/bcbench/agent/copilot/pr_review/agent.py b/src/bcbench/agent/copilot/pr_review/agent.py deleted file mode 100644 index 30fa12851..000000000 --- a/src/bcbench/agent/copilot/pr_review/agent.py +++ /dev/null @@ -1,336 +0,0 @@ -"""Run the BC-ALAgents review engine (generate half) as a BC-Bench agent. - -The code-review category runs the engine's own generate shell -(``Invoke-PRReviewShell.ps1 -GenerateOnly``) in local mode against the entry's changes, -so BC-Bench measures the real PROD engine + BCQuality rather than a divergent -re-implementation. The BC-Bench ``--model`` threads straight through to the single -Copilot the engine spawns (``COPILOT_MODEL``). - -The engine writes ``agent-output.txt`` (the harvested findings report); we map it to -``review.json`` in the repo root so the existing code-review scorer runs unchanged. -""" - -import json -import os -import shutil -import subprocess -import time -from collections.abc import Generator -from contextlib import contextmanager -from pathlib import Path -from typing import Any - -import yaml - -from bcbench.agent.copilot.pr_review.metrics import build_pr_review_metrics -from bcbench.agent.copilot.pr_review.review_output import engine_report_to_review_comments, load_engine_report -from bcbench.config import get_config -from bcbench.dataset import BaseDatasetEntry -from bcbench.dataset.codereview import CodeReviewEntry -from bcbench.exceptions import AgentError, AgentTimeoutError -from bcbench.logger import get_logger -from bcbench.operations.git_operations import clone_repo_at_revision, remove_tree -from bcbench.types import AgentMetrics, EvaluationCategory, ExperimentConfiguration - -logger = get_logger(__name__) -_config = get_config() - -_AGENT_OUTPUT_FILE = "agent-output.txt" -_REVIEW_OUTPUT_FILE = "review.json" -_PREPARE_BCQUALITY_SCRIPT = Path(__file__).parent / "scripts" / "Prepare-BCQualityRoot.ps1" - - -def _load_pr_review_settings() -> dict[str, Any]: - config_file = _config.paths.agent_share_dir / "config.yaml" - data = yaml.safe_load(config_file.read_text()) or {} - return data.get("pr_review") or {} - - -def _validate_engine_root(raw: str | Path) -> Path: - root = Path(raw).expanduser() - shell = root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-PRReviewShell.ps1" - if not shell.exists(): - raise AgentError(f"Engine review shell not found at {shell}. Check the configured BC-ALAgents source.") - return root - - -@contextmanager -def _prepare_engine_root( - settings: dict[str, Any], - destination: Path, - engine_ref: str | None = None, - engine_repo: str | None = None, - engine_local_path: str | None = None, -) -> Generator[Path]: - engine_cfg = settings.get("engine") or {} - environment_root = os.environ.get("BC_PR_REVIEW_ROOT") - if environment_root: - yield _validate_engine_root(environment_root) - return - - if engine_local_path and (engine_repo or engine_ref): - raise AgentError("--engine-local-path cannot be combined with --engine-repo or --engine-ref.") - - cli_remote_source = engine_repo is not None or engine_ref is not None - local_path = engine_local_path if engine_local_path is not None else None if cli_remote_source else engine_cfg.get("local_path") - if local_path: - yield _validate_engine_root(local_path) - return - - repo = engine_repo or engine_cfg.get("repo") - ref = engine_ref or engine_cfg.get("ref") - if not repo or not ref: - raise AgentError("Engine source not configured. Set pr_review.engine.repo/ref, use --engine-repo/--engine-ref, or provide BC_PR_REVIEW_ROOT.") - - try: - clone_repo_at_revision(str(repo), str(ref), destination) - yield _validate_engine_root(destination) - finally: - if destination.exists(): - remove_tree(destination) - - -def _resolve_engine_revision(engine_root: Path) -> str: - """Resolve the engine checkout's git revision (with a dirty marker) for provenance.""" - head = subprocess.run(["git", "-C", str(engine_root), "rev-parse", "HEAD"], capture_output=True, text=True, check=False) - if head.returncode != 0 or not head.stdout.strip(): - return "unknown" - sha = head.stdout.strip() - dirty = subprocess.run(["git", "-C", str(engine_root), "status", "--porcelain"], capture_output=True, text=True, check=False) - if dirty.returncode == 0 and dirty.stdout.strip(): - return f"{sha}-dirty" - return sha - - -def _resolve_pwsh() -> str: - pwsh = shutil.which("pwsh") - if not pwsh: - raise AgentError("PowerShell (pwsh) not found in PATH. The BC-ALAgents engine requires PowerShell 7+.") - return pwsh - - -def _resolve_gh_token() -> str: - token = os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN") - if token: - return token - gh = shutil.which("gh") - if gh: - result = subprocess.run([gh, "auth", "token"], capture_output=True, text=True, check=False) - if result.returncode == 0 and result.stdout.strip(): - return result.stdout.strip() - raise AgentError("No GitHub token available for Copilot CLI auth. Set GH_TOKEN or run `gh auth login`.") - - -def _git(args: list[str], cwd: Path) -> subprocess.CompletedProcess[str]: - return subprocess.run(["git", *args], cwd=str(cwd), capture_output=True, text=True, check=True) - - -def _commit_patch_as_head(repo_path: Path) -> None: - """Commit the applied working-tree patch so the engine can diff base..HEAD. - - The code-review pipeline applies the entry patch as uncommitted changes (and - marks new files intent-to-add). The engine's local mode diffs a committed - ``BASE_REF...HEAD`` range, so materialize the changes as a head commit on top - of the base commit (which is the current HEAD). - """ - _git(["add", "-A"], repo_path) - status = _git(["status", "--porcelain"], repo_path) - if not status.stdout.strip(): - raise AgentError("No changes to review: the entry patch produced an empty working tree diff.") - _git( - ["-c", "user.name=bcbench", "-c", "user.email=bcbench@local", "commit", "-q", "--no-verify", "-m", "bcbench review head"], - repo_path, - ) - - -def _init_trusted_workspace(path: Path) -> Path: - path.mkdir(parents=True, exist_ok=True) - _git(["init", "-q"], path) - _git(["-c", "user.name=bcbench", "-c", "user.email=bcbench@local", "commit", "-q", "--allow-empty", "-m", "trusted"], path) - return path - - -def _prepare_bcquality_root( - engine_root: Path, - pwsh: str, - dest: Path, - bcquality_ref: str | None, - bcquality_repo: str | None = None, - bcquality_local_path: str | None = None, -) -> tuple[Path, str | None]: - env = {**os.environ} - if bcquality_repo: - env["BCQUALITY_REPO"] = bcquality_repo - if bcquality_ref: - env["BCQUALITY_REF"] = bcquality_ref - args = [pwsh, "-NoProfile", "-File", str(_PREPARE_BCQUALITY_SCRIPT), "-EngineRoot", str(engine_root), "-Root", str(dest)] - if bcquality_local_path: - args += ["-LocalPath", bcquality_local_path] - result = subprocess.run( - args, - capture_output=True, - text=True, - env=env, - check=False, - ) - if result.returncode != 0: - logger.error(f"BCQuality preparation failed:\n{result.stdout}\n{result.stderr}") - raise AgentError(f"Failed to prepare BCQuality root (exit {result.returncode}).") - root: Path | None = None - sha: str | None = None - for line in result.stdout.splitlines(): - if line.startswith("root="): - root = Path(line[len("root=") :].strip()) - elif line.startswith("sha="): - sha = line[len("sha=") :].strip() - if root is None or not root.exists(): - raise AgentError("BCQuality preparation did not report a valid root.") - return root, sha - - -def _resolve_bcquality_source( - settings: dict[str, Any], - bcquality_ref: str | None, - bcquality_repo: str | None, - bcquality_local_path: str | None, -) -> tuple[str | None, str | None, str | None]: - bcquality_cfg = settings.get("bcquality") or {} - if bcquality_local_path and (bcquality_repo or bcquality_ref): - raise AgentError("--bcquality-local-path cannot be combined with --bcquality-repo or --bcquality-ref.") - - remote_override = bcquality_repo is not None or bcquality_ref is not None - resolved_repo = bcquality_repo or bcquality_cfg.get("repo") - resolved_ref = bcquality_ref or bcquality_cfg.get("ref") - resolved_local_path = bcquality_local_path - if resolved_local_path is None and not remote_override: - resolved_local_path = bcquality_cfg.get("local_path") - return resolved_ref, resolved_repo, resolved_local_path - - -def _write_review_json(output_dir: Path, repo_path: Path) -> int: - agent_output = output_dir / _AGENT_OUTPUT_FILE - if not agent_output.exists(): - raise AgentError(f"Engine did not produce {_AGENT_OUTPUT_FILE} in {output_dir}.") - report = load_engine_report(agent_output.read_text(encoding="utf-8")) - if report is None: - raise AgentError(f"Engine {_AGENT_OUTPUT_FILE} was empty or not a valid findings report; refusing to score it as a clean review.") - if not isinstance(report.get("findings"), list): - raise AgentError(f"Engine report in {_AGENT_OUTPUT_FILE} has no findings list (got {type(report.get('findings')).__name__}); refusing to score it as a clean review.") - comments = engine_report_to_review_comments(report) - (repo_path / _REVIEW_OUTPUT_FILE).write_text(json.dumps(comments, indent=2), encoding="utf-8") - return len(comments) - - -def run_pr_review_agent( - entry: BaseDatasetEntry, - model: str, - category: EvaluationCategory, - repo_path: Path, - output_dir: Path, - bcquality_ref: str | None = None, - bcquality_repo: str | None = None, - bcquality_local_path: str | None = None, - engine_ref: str | None = None, - engine_repo: str | None = None, - engine_local_path: str | None = None, - min_severity: str | None = None, -) -> tuple[AgentMetrics | None, ExperimentConfiguration]: - """Run the engine's generate half on a code-review entry and write review.json. - - Separate from run_copilot_agent by design: this spawns the PROD BC-ALAgents - PowerShell orchestrator (Copilot is spawned inside the engine, not here), so it - owns none of the copilot-harness prompt/MCP/LSP wiring and takes engine-specific - inputs (BCQuality source, min severity) for the code-review category only. - - Returns: - Tuple of (AgentMetrics, ExperimentConfiguration). - """ - if category is not EvaluationCategory.CODE_REVIEW: - raise AgentError(f"The engine agent only supports the code-review category, got {category.value}.") - if not isinstance(entry, CodeReviewEntry): - raise AgentError(f"The engine agent requires a CodeReviewEntry, got {type(entry).__name__}.") - settings = _load_pr_review_settings() - pwsh = _resolve_pwsh() - gh_token = _resolve_gh_token() - agent_version = str(settings.get("agent_version", "0.0.0")) - severity = min_severity or settings.get("min_severity") or "Low" - bcquality_ref, bcquality_repo, bcquality_local_path = _resolve_bcquality_source( - settings, - bcquality_ref, - bcquality_repo, - bcquality_local_path, - ) - - output_dir.mkdir(parents=True, exist_ok=True) - logger.info(f"Running BC-ALAgents review engine on: {entry.instance_id}") - - with _prepare_engine_root( - settings, - output_dir / "engine", - engine_ref=engine_ref, - engine_repo=engine_repo, - engine_local_path=engine_local_path, - ) as engine_root: - _commit_patch_as_head(repo_path) - trusted_workspace = _init_trusted_workspace(output_dir / "trusted") - bcquality_root, bcquality_sha = _prepare_bcquality_root( - engine_root, - pwsh, - output_dir / "bcquality", - bcquality_ref, - bcquality_repo, - bcquality_local_path, - ) - - shell = engine_root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-PRReviewShell.ps1" - env = { - **os.environ, - "REVIEW_SOURCE": "local", - "BASE_REF": entry.base_commit, - "REVIEW_TARGET_WORKSPACE": str(repo_path), - "REVIEW_WORKSPACE": str(trusted_workspace), - "REVIEW_OUTPUT_DIR": str(output_dir), - "BCQUALITY_ROOT": str(bcquality_root), - "COPILOT_MODEL": model, - "COPILOT_REVIEW_AGENT_VERSION": agent_version, - "COPILOT_REVIEW_LOG_LEVEL": "debug", - "AGENT_MINIMUM_SEVERITY": severity, - "GH_TOKEN": gh_token, - } - - plugins = [f"bc-review-engine@{_resolve_engine_revision(engine_root)}"] - if bcquality_sha: - plugins.append(f"BCQuality@{bcquality_sha}") - config = ExperimentConfiguration( - custom_agent="bc-review-engine", - plugins=plugins, - ) - - start = time.monotonic() - try: - result = subprocess.run( - [pwsh, "-NoProfile", "-File", str(shell), "-GenerateOnly", "-OutputDir", str(output_dir)], - cwd=str(repo_path), - env=env, - capture_output=True, - text=True, - timeout=_config.timeout.agent_execution, - check=True, - ) - logger.debug(f"Engine stdout:\n{result.stdout}") - if result.stderr: - logger.debug(f"Engine stderr:\n{result.stderr}") - count = _write_review_json(output_dir, repo_path) - logger.info(f"Engine review complete for {entry.instance_id}: wrote {count} comment(s) to {_REVIEW_OUTPUT_FILE}") - except subprocess.TimeoutExpired: - logger.exception(f"Engine review timed out after {_config.timeout.agent_execution} seconds") - metrics = AgentMetrics(execution_time=_config.timeout.agent_execution) - raise AgentTimeoutError("Engine review timed out", metrics=metrics, config=config) from None - except subprocess.CalledProcessError as e: - logger.exception(f"Engine review failed (exit {e.returncode}):\n{e.stdout}\n{e.stderr}") - raise AgentError(f"Engine review execution failed: {e}") from None - except Exception: - logger.exception("Unexpected error running engine review") - raise - else: - return build_pr_review_metrics(output_dir, bcquality_root, time.monotonic() - start), config diff --git a/src/bcbench/agent/copilot/pr_review/metrics.py b/src/bcbench/agent/copilot/pr_review/metrics.py deleted file mode 100644 index 9286d0509..000000000 --- a/src/bcbench/agent/copilot/pr_review/metrics.py +++ /dev/null @@ -1,164 +0,0 @@ -import json -import re -from pathlib import Path -from typing import Any - -from bcbench.agent.copilot.metrics import parse_metrics -from bcbench.logger import get_logger -from bcbench.types import AgentMetrics - -logger = get_logger(__name__) - -RUN_METRICS_FILE_NAME = "_run-metrics.json" -FILTER_REPORT_FILE_NAME = "_filter-report.json" -TRANSCRIPT_FILE_NAME = "agent-transcript.log" -METRIC_NUMBER_PATTERN = r"[0-9][0-9,]*(?:\.[0-9]+)?[kKmM]?" -AI_CREDITS_PATTERN = re.compile(rf"(?m)^(?:err:\s*)?AI Credits\s+({METRIC_NUMBER_PATTERN})") -PREMIUM_REQUESTS_PATTERN = re.compile(rf"(?:Requests\s+|Total usage est:\s*)({METRIC_NUMBER_PATTERN})\s+Premium", re.IGNORECASE) -TOKENS_PATTERN = re.compile( - rf"(?m)^(?:err:\s*)?Tokens\s+↑\s*({METRIC_NUMBER_PATTERN})" - rf"(?:\s+\(({METRIC_NUMBER_PATTERN})\s+cached(?:,\s*{METRIC_NUMBER_PATTERN}\s+written)?\))?" - rf"\s+•\s+↓\s*({METRIC_NUMBER_PATTERN})" - rf"(?:\s+\(({METRIC_NUMBER_PATTERN})\s+reasoning\))?" -) - - -def _load_json(path: Path) -> dict[str, Any] | None: - if not path.exists(): - logger.debug(f"Engine perf file not found: {path}") - return None - try: - payload = json.loads(path.read_text(encoding="utf-8")) - except (json.JSONDecodeError, OSError) as exc: - logger.warning(f"Could not read engine perf file {path}: {exc}") - return None - if not isinstance(payload, dict): - logger.warning(f"Engine perf file {path} is not a JSON object; ignoring") - return None - return payload - - -def _as_int(value: object) -> int | None: - if isinstance(value, bool) or not isinstance(value, (int, float, str)): - return None - try: - return int(value) - except (TypeError, ValueError): - return None - - -def _as_float(value: object) -> float | None: - if isinstance(value, bool) or not isinstance(value, (int, float, str)): - return None - try: - return float(value) - except (TypeError, ValueError): - return None - - -def _parse_compact_number(value: str) -> float: - normalized = value.replace(",", "").lower() - multiplier = 1.0 - if normalized.endswith("k"): - normalized = normalized[:-1] - multiplier = 1_000.0 - elif normalized.endswith("m"): - normalized = normalized[:-1] - multiplier = 1_000_000.0 - return float(normalized) * multiplier - - -def parse_run_metrics(path: Path) -> dict[str, Any]: - payload = _load_json(path) - if payload is None: - return {} - - result: dict[str, Any] = {} - for source_key, target_key, coerce in ( - ("prompt_tokens", "prompt_tokens", _as_int), - ("completion_tokens", "completion_tokens", _as_int), - ("total_tokens", "total_tokens", _as_int), - ("api_calls", "api_calls", _as_int), - ("estimated_credits", "estimated_credits", _as_float), - ("wall_time_seconds", "wall_time_seconds", _as_float), - ): - value = coerce(payload.get(source_key)) - if value is not None: - result[target_key] = value - - if "total_tokens" not in result and "prompt_tokens" in result and "completion_tokens" in result: - result["total_tokens"] = int(result["prompt_tokens"]) + int(result["completion_tokens"]) - return result - - -def parse_transcript_metrics(path: Path) -> dict[str, int | float]: - if not path.exists(): - logger.debug(f"Engine transcript not found: {path}") - return {} - try: - lines = path.read_text(encoding="utf-8").splitlines(keepends=True) - except OSError as exc: - logger.warning(f"Could not read engine transcript {path}: {exc}") - return {} - - parsed = parse_metrics(lines, session_log_path=path) - result: dict[str, int | float] = {} - if parsed: - if parsed.prompt_tokens is not None: - result["prompt_tokens"] = parsed.prompt_tokens - if parsed.completion_tokens is not None: - result["completion_tokens"] = parsed.completion_tokens - if parsed.turn_count is not None: - result["api_calls"] = parsed.turn_count - - transcript = "".join(lines) - token_matches = list(TOKENS_PATTERN.finditer(transcript)) - if token_matches: - token_match = token_matches[-1] - result["prompt_tokens"] = int(_parse_compact_number(token_match.group(1))) - result["completion_tokens"] = int(_parse_compact_number(token_match.group(3))) - - credit_matches = list(AI_CREDITS_PATTERN.finditer(transcript)) - if not credit_matches: - credit_matches = list(PREMIUM_REQUESTS_PATTERN.finditer(transcript)) - if credit_matches: - result["estimated_credits"] = _parse_compact_number(credit_matches[-1].group(1)) - if "prompt_tokens" in result and "completion_tokens" in result: - result["total_tokens"] = int(result["prompt_tokens"]) + int(result["completion_tokens"]) - return result - - -def _count_filtered_knowledge(bcquality_root: Path) -> int: - return sum(1 for path in bcquality_root.rglob("*.md") if path.is_file() and "knowledge" in {part.lower() for part in path.relative_to(bcquality_root).parts[:-1]}) - - -def parse_filter_report(path: Path, bcquality_root: Path) -> dict[str, int]: - payload = _load_json(path) - if payload is None: - return {} - removed = payload.get("removed") - if not isinstance(removed, list): - return {} - return { - "knowledge_pruned": sum(1 for item in removed if isinstance(item, dict) and item.get("kind") == "knowledge"), - "knowledge_used": _count_filtered_knowledge(bcquality_root), - } - - -def build_pr_review_metrics(output_dir: Path, bcquality_root: Path, execution_time: float) -> AgentMetrics: - transcript = parse_transcript_metrics(output_dir / TRANSCRIPT_FILE_NAME) - run = {**transcript, **parse_run_metrics(output_dir / RUN_METRICS_FILE_NAME)} - filter_report = output_dir / FILTER_REPORT_FILE_NAME - if not filter_report.exists(): - filter_report = bcquality_root / FILTER_REPORT_FILE_NAME - knowledge = parse_filter_report(filter_report, bcquality_root) - return AgentMetrics( - execution_time=execution_time, - prompt_tokens=_as_int(run.get("prompt_tokens")), - completion_tokens=_as_int(run.get("completion_tokens")), - total_tokens=_as_int(run.get("total_tokens")), - api_calls=_as_int(run.get("api_calls")), - estimated_credits=_as_float(run.get("estimated_credits")), - knowledge_used=knowledge.get("knowledge_used"), - knowledge_pruned=knowledge.get("knowledge_pruned"), - ) diff --git a/src/bcbench/agent/pr_review/__init__.py b/src/bcbench/agent/pr_review/__init__.py new file mode 100644 index 000000000..987026334 --- /dev/null +++ b/src/bcbench/agent/pr_review/__init__.py @@ -0,0 +1,3 @@ +from bcbench.agent.pr_review.agent import run_pr_review_agent + +__all__ = ["run_pr_review_agent"] diff --git a/src/bcbench/agent/pr_review/agent.py b/src/bcbench/agent/pr_review/agent.py new file mode 100644 index 000000000..a4f90f194 --- /dev/null +++ b/src/bcbench/agent/pr_review/agent.py @@ -0,0 +1,233 @@ +"""Run the BC-ALAgents review engine as a BC-Bench agent. + +The code-review category runs the production orchestrator in local mode against the +entry's changes. Local mode executes the complete generation, parsing, filtering, and +artifact pipeline but returns before posting, so BC-Bench measures the real production +engine + BCQuality rather than a divergent re-implementation. The BC-Bench ``--model`` +threads straight through to the Copilot process the engine spawns (``COPILOT_MODEL``). + +The engine writes normalized findings to ``al-code-review-findings.json``; we map them +to ``review.json`` in the repo root so the existing code-review scorer runs unchanged. +""" + +import json +import os +import shutil +import subprocess +import time +from pathlib import Path +from typing import Any + +import yaml + +from bcbench.agent.pr_review.metrics import build_pr_review_metrics +from bcbench.agent.pr_review.review_output import engine_report_to_review_comments, load_engine_report +from bcbench.config import get_config +from bcbench.dataset import BaseDatasetEntry +from bcbench.dataset.codereview import CodeReviewEntry +from bcbench.exceptions import AgentError, AgentTimeoutError +from bcbench.logger import get_logger +from bcbench.operations import commit_changes, has_changes, init_repo +from bcbench.types import AgentMetrics, EvaluationCategory, ExperimentConfiguration + +logger = get_logger(__name__) +_config = get_config() + +_FINDINGS_OUTPUT_FILE = "al-code-review-findings.json" +_REVIEW_OUTPUT_FILE = "review.json" +_PREPARE_BCQUALITY_SCRIPT = Path(__file__).parent / "scripts" / "Prepare-BCQualityRoot.ps1" + + +def _load_pr_review_settings() -> dict[str, Any]: + config_file = _config.paths.agent_share_dir / "config.yaml" + return yaml.safe_load(config_file.read_text(encoding="utf-8"))["pr_review"] + + +def _resolve_pr_review_root(engine_path: Path | None) -> Path: + if engine_path is None: + raise AgentError("Engine root not configured. Pass --engine-path or set BC_PR_REVIEW_ROOT.") + root = engine_path.expanduser().resolve() + engine = root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-CopilotPRReview.ps1" + if not engine.exists(): + raise AgentError(f"Engine orchestrator not found at {engine}. Check --engine-path points at a BC-ALAgents checkout.") + return root + + +def _resolve_pwsh() -> str: + pwsh = shutil.which("pwsh") + if not pwsh: + raise AgentError("PowerShell (pwsh) not found in PATH. The BC-ALAgents engine requires PowerShell 7+.") + return pwsh + + +def _commit_patch_as_head(repo_path: Path) -> None: + """Commit the applied working-tree patch so the engine can diff base..HEAD. + + The code-review pipeline applies the entry patch as uncommitted changes (and + marks new files intent-to-add). The engine's local mode diffs a committed + ``BASE_REF...HEAD`` range, so materialize the changes as a head commit on top + of the base commit (which is the current HEAD). + """ + if not has_changes(repo_path): + raise AgentError("No changes to review: the entry patch produced an empty working tree diff.") + commit_changes(repo_path, "bcbench review head", no_verify=True) + + +def _init_trusted_workspace(path: Path) -> Path: + init_repo(path) + commit_changes(path, "trusted", allow_empty=True) + return path + + +def _prepare_bcquality_root( + engine_root: Path, + pwsh: str, + dest: Path, + bcquality_ref: str | None, + bcquality_repo: str | None = None, + bcquality_local_path: Path | None = None, +) -> Path: + env = {**os.environ} + if bcquality_repo: + env["BCQUALITY_REPO"] = bcquality_repo + if bcquality_ref: + env["BCQUALITY_REF"] = bcquality_ref + args = [pwsh, "-NoProfile", "-File", str(_PREPARE_BCQUALITY_SCRIPT), "-EngineRoot", str(engine_root), "-Root", str(dest)] + if bcquality_local_path: + args += ["-LocalPath", str(bcquality_local_path)] + result = subprocess.run( + args, + capture_output=True, + text=True, + encoding="utf-8", + env=env, + check=False, + ) + if result.returncode != 0: + logger.error(f"BCQuality preparation failed:\n{result.stdout}\n{result.stderr}") + raise AgentError(f"Failed to prepare BCQuality root (exit {result.returncode}).") + root: Path | None = None + for line in result.stdout.splitlines(): + if line.startswith("root="): + root = Path(line[len("root=") :].strip()) + if root is None or not root.exists(): + raise AgentError("BCQuality preparation did not report a valid root.") + return root + + +def _write_review_json(output_dir: Path, repo_path: Path) -> int: + findings_output = output_dir / _FINDINGS_OUTPUT_FILE + if not findings_output.exists(): + raise AgentError(f"Engine did not produce {_FINDINGS_OUTPUT_FILE} in {output_dir}.") + report = load_engine_report(findings_output.read_text(encoding="utf-8")) + if report is None: + raise AgentError(f"Engine {_FINDINGS_OUTPUT_FILE} was empty or invalid; refusing to score it as a clean review.") + outcome = report.get("outcome") + if outcome == "failed": + reason = report.get("outcomeReason") or "unknown reason" + raise AgentError(f"Engine review failed: {reason}") + if outcome not in {"completed", "partial", "not-applicable", "no-knowledge"}: + raise AgentError(f"Engine {_FINDINGS_OUTPUT_FILE} has unsupported outcome {outcome!r}.") + if not isinstance(report.get("findings"), list): + raise AgentError(f"Engine report in {_FINDINGS_OUTPUT_FILE} has no findings list (got {type(report.get('findings')).__name__}); refusing to score it as a clean review.") + comments = engine_report_to_review_comments(report) + (repo_path / _REVIEW_OUTPUT_FILE).write_text(json.dumps(comments, indent=2), encoding="utf-8") + return len(comments) + + +def run_pr_review_agent( + entry: BaseDatasetEntry, + model: str, + category: EvaluationCategory, + repo_path: Path, + output_dir: Path, + engine_path: Path | None = None, + bcquality_ref: str | None = None, + bcquality_repo: str | None = None, + bcquality_local_path: Path | None = None, + min_severity: str | None = None, +) -> tuple[AgentMetrics | None, ExperimentConfiguration]: + """Run the engine's complete local review pipeline and write review.json. + + Separate from run_copilot_agent by design: this spawns the PROD BC-ALAgents + PowerShell orchestrator (Copilot is spawned inside the engine, not here), so it + owns none of the copilot-harness prompt/MCP/LSP wiring and takes engine-specific + inputs (BCQuality source, min severity) for the code-review category only. + + Returns: + Tuple of (AgentMetrics, ExperimentConfiguration). + """ + if category is not EvaluationCategory.CODE_REVIEW: + raise AgentError(f"The engine agent only supports the code-review category, got {category.value}.") + if not isinstance(entry, CodeReviewEntry): + raise AgentError(f"The engine agent requires a CodeReviewEntry, got {type(entry).__name__}.") + + repo_path = repo_path.resolve() + output_dir = output_dir.resolve() + settings = _load_pr_review_settings() + engine_root = _resolve_pr_review_root(engine_path) + pwsh = _resolve_pwsh() + severity = min_severity or settings["min_severity"] + bcquality_cfg = settings["bcquality"] + bcquality_repo = bcquality_repo or bcquality_cfg["repo"] + bcquality_ref = bcquality_ref or bcquality_cfg["ref"] + output_dir.mkdir(parents=True, exist_ok=True) + logger.info(f"Running BC-ALAgents review engine on: {entry.instance_id}") + + _commit_patch_as_head(repo_path) + trusted_workspace = _init_trusted_workspace(output_dir / "trusted") + bcquality_root = _prepare_bcquality_root( + engine_root, + pwsh, + output_dir / "bcquality", + bcquality_ref, + bcquality_repo, + bcquality_local_path, + ) + + engine = engine_root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-CopilotPRReview.ps1" + env = { + **os.environ, + "REVIEW_SOURCE": "local", + "REVIEW_PHASE": "all", + "BASE_REF": entry.base_commit, + "REVIEW_TARGET_WORKSPACE": str(repo_path), + "REVIEW_WORKSPACE": str(trusted_workspace), + "REVIEW_OUTPUT_DIR": str(output_dir), + "BCQUALITY_ROOT": str(bcquality_root), + "GITHUB_REPOSITORY": entry.repo, + "COPILOT_MODEL": model, + "AGENT_MINIMUM_SEVERITY": severity, + } + + config = ExperimentConfiguration() + + start = time.monotonic() + try: + result = subprocess.run( + [pwsh, "-NoProfile", "-File", str(engine)], + cwd=str(repo_path), + env=env, + capture_output=True, + text=True, + encoding="utf-8", + timeout=_config.timeout.agent_execution, + check=True, + ) + logger.debug(f"Engine stdout:\n{result.stdout}") + if result.stderr: + logger.debug(f"Engine stderr:\n{result.stderr}") + count = _write_review_json(output_dir, repo_path) + logger.info(f"Engine review complete for {entry.instance_id}: wrote {count} comment(s) to {_REVIEW_OUTPUT_FILE}") + except subprocess.TimeoutExpired: + logger.exception(f"Engine review timed out after {_config.timeout.agent_execution} seconds") + metrics = AgentMetrics(execution_time=_config.timeout.agent_execution) + raise AgentTimeoutError("Engine review timed out", metrics=metrics, config=config) from None + except subprocess.CalledProcessError as e: + logger.exception(f"Engine review failed (exit {e.returncode}):\n{e.stdout}\n{e.stderr}") + raise AgentError(f"Engine review execution failed: {e}") from None + except Exception: + logger.exception("Unexpected error running engine review") + raise + else: + return build_pr_review_metrics(output_dir, bcquality_root, time.monotonic() - start), config diff --git a/src/bcbench/agent/pr_review/metrics.py b/src/bcbench/agent/pr_review/metrics.py new file mode 100644 index 000000000..052b8834e --- /dev/null +++ b/src/bcbench/agent/pr_review/metrics.py @@ -0,0 +1,133 @@ +import json +from pathlib import Path +from typing import Annotated, Literal + +from pydantic import BaseModel, ConfigDict, Field, ValidationError, model_validator + +from bcbench.exceptions import AgentError +from bcbench.types import AgentMetrics + +FILTER_REPORT_FILE_NAME = "_filter-report.json" +RUN_METRICS_FILE_NAME = "_run-metrics.json" +_KNOWLEDGE_LAYERS = {"microsoft", "community", "custom"} +_NonNegativeInt = Annotated[int, Field(ge=0)] +_NonNegativeFloat = Annotated[float, Field(ge=0)] + + +class _FilterRemoval(BaseModel): + model_config = ConfigDict(extra="ignore", frozen=True) + + kind: Literal["knowledge", "skill"] + + +class _FilterReport(BaseModel): + model_config = ConfigDict(extra="ignore", frozen=True) + + removed: list[_FilterRemoval] + + +class _RunMetrics(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + schema_version: Literal[1] + metrics_source: Literal["copilot-cli-otel", "not-applicable"] + cli_version: str | None + wall_time_seconds: _NonNegativeFloat | None + prompt_tokens: _NonNegativeInt | None + cached_tokens: _NonNegativeInt | None + cache_creation_tokens: _NonNegativeInt | None + completion_tokens: _NonNegativeInt | None + reasoning_tokens: _NonNegativeInt | None + total_tokens: _NonNegativeInt | None + api_calls: _NonNegativeInt | None + failed_api_calls: _NonNegativeInt | None + usage_api_calls: _NonNegativeInt | None + ai_credits: _NonNegativeFloat | None + premium_requests: _NonNegativeFloat | None + models: list[str] + usage_complete: bool + malformed_records: _NonNegativeInt + + @model_validator(mode="after") + def validate_not_applicable_shape(self) -> "_RunMetrics": + if self.metrics_source != "not-applicable": + return self + expected = { + "cli_version": None, + "wall_time_seconds": 0, + "prompt_tokens": 0, + "cached_tokens": 0, + "cache_creation_tokens": 0, + "completion_tokens": 0, + "reasoning_tokens": None, + "total_tokens": 0, + "api_calls": 0, + "failed_api_calls": 0, + "usage_api_calls": 0, + "ai_credits": 0.0, + "premium_requests": None, + "models": [], + "usage_complete": True, + "malformed_records": 0, + } + invalid = [name for name, value in expected.items() if getattr(self, name) != value] + if invalid: + raise ValueError(f"not-applicable metrics have invalid fields: {', '.join(invalid)}") + return self + + +def _load_run_metrics(path: Path) -> _RunMetrics: + if not path.exists(): + raise AgentError(f"Engine run metrics artifact not found at {path}.") + try: + payload = json.loads(path.read_text(encoding="utf-8-sig")) + except (json.JSONDecodeError, OSError) as exc: + raise AgentError(f"Could not read engine run metrics artifact {path}: {exc}") from exc + try: + return _RunMetrics.model_validate(payload) + except ValidationError as exc: + raise AgentError(f"Engine run metrics artifact {path} does not satisfy schema version 1: {exc}") from exc + + +def _load_filter_report(path: Path) -> _FilterReport: + if not path.exists(): + raise AgentError(f"BCQuality filter report not found at {path}.") + try: + payload = json.loads(path.read_text(encoding="utf-8-sig")) + except (json.JSONDecodeError, OSError) as exc: + raise AgentError(f"Could not read BCQuality filter report {path}: {exc}") from exc + try: + return _FilterReport.model_validate(payload) + except ValidationError as exc: + raise AgentError(f"BCQuality filter report {path} has an invalid shape: {exc}") from exc + + +def _count_available_knowledge(bcquality_root: Path) -> int: + def is_knowledge_file(path: Path) -> bool: + parts = path.relative_to(bcquality_root).parts + return len(parts) >= 3 and parts[0].lower() in _KNOWLEDGE_LAYERS and parts[1].lower() == "knowledge" + + return sum(1 for path in bcquality_root.rglob("*.md") if path.is_file() and is_knowledge_file(path)) + + +def build_pr_review_metrics(output_dir: Path, bcquality_root: Path, execution_time: float) -> AgentMetrics: + run = _load_run_metrics(output_dir / RUN_METRICS_FILE_NAME) + report = _load_filter_report(bcquality_root / FILTER_REPORT_FILE_NAME) + return AgentMetrics( + execution_time=execution_time, + prompt_tokens=run.prompt_tokens, + completion_tokens=run.completion_tokens, + cached_tokens=run.cached_tokens, + cache_creation_tokens=run.cache_creation_tokens, + reasoning_tokens=run.reasoning_tokens, + total_tokens=run.total_tokens, + api_calls=run.api_calls, + failed_api_calls=run.failed_api_calls, + usage_api_calls=run.usage_api_calls, + ai_credits=run.ai_credits, + premium_requests=run.premium_requests, + usage_complete=run.usage_complete, + malformed_records=run.malformed_records, + knowledge_files=_count_available_knowledge(bcquality_root), + knowledge_pruned=sum(1 for item in report.removed if item.kind == "knowledge"), + ) diff --git a/src/bcbench/agent/copilot/pr_review/review_output.py b/src/bcbench/agent/pr_review/review_output.py similarity index 72% rename from src/bcbench/agent/copilot/pr_review/review_output.py rename to src/bcbench/agent/pr_review/review_output.py index ab0c86add..5f1def674 100644 --- a/src/bcbench/agent/copilot/pr_review/review_output.py +++ b/src/bcbench/agent/pr_review/review_output.py @@ -1,21 +1,23 @@ -"""Map the engine's findings report onto BC-Bench's review.json schema. +"""Map production-normalized engine findings onto BC-Bench's review.json schema. -The BC-ALAgents engine writes ``agent-output.txt`` (the harvested -``_review-report.json``) in its output directory. Each finding is shaped like:: +The BC-ALAgents engine writes ``al-code-review-findings.json`` after its production +parsing and filtering stages. Each finding is shaped like:: { + "filePath": "