From 7aec4146580b688f1318eb593b8e82e3001f8187 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 1 Oct 2026 10:23:29 +0200 Subject: [PATCH 1/5] Prepare experimental HTTP knowledge A/B support Integrate necessary deterministic adapter support from PR 881 at 8e2946fd. Preserve default generic harnesses and all 145 existing code-review entries. Add three separately gated experimental controls and original-report integrity validation; no evaluation is authorized. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../install-agent-harnesses/action.yml | 20 +- .github/workflows/pr-review-evaluation.yml | 69 ++++- dataset/codereview.jsonl | 3 + docs/code-review.md | 16 +- pyproject.toml | 2 +- src/bcbench/agent/pr_review/agent.py | 57 +++- src/bcbench/agent/pr_review/metrics.py | 39 ++- src/bcbench/agent/pr_review/run_manifest.py | 259 ++++++++++++++++++ src/bcbench/cli_options.py | 48 ++++ src/bcbench/commands/evaluate.py | 19 +- src/bcbench/commands/run.py | 18 +- src/bcbench/types.py | 7 + tests/test_agent_versions.py | 7 + tests/test_codereview_fixtures.py | 72 ++++- tests/test_pr_review_agent.py | 183 ++++++++++++- tests/test_pr_review_metrics.py | 59 +++- tests/test_pr_review_metrics_reporting.py | 44 ++- tests/test_pr_review_run_manifest.py | 246 +++++++++++++++++ tests/test_review_runners.py | 4 + tests/test_review_workflows.py | 182 +++++++++++- uv.lock | 2 +- 21 files changed, 1291 insertions(+), 65 deletions(-) create mode 100644 src/bcbench/agent/pr_review/run_manifest.py create mode 100644 tests/test_pr_review_run_manifest.py diff --git a/.github/actions/install-agent-harnesses/action.yml b/.github/actions/install-agent-harnesses/action.yml index 915e18c63..877faacfc 100644 --- a/.github/actions/install-agent-harnesses/action.yml +++ b/.github/actions/install-agent-harnesses/action.yml @@ -6,11 +6,18 @@ inputs: description: Full BC-ALAgents commit SHA (blank uses the default pin) required: false default: "" + copilot-version: + description: GitHub Copilot CLI version (defaults to 1.0.82) + required: false + default: "1.0.82" outputs: bc-alagents-path: description: Path to the pinned BC-ALAgents checkout value: ${{ steps.bc-alagents-path.outputs.path }} + copilot-version: + description: Installed GitHub Copilot CLI version + value: ${{ steps.copilot-version.outputs.version }} runs: using: composite @@ -35,8 +42,19 @@ runs: run: npm install -g @anthropic-ai/claude-code@2.1.221 shell: pwsh + - name: Validate GitHub Copilot CLI version + id: copilot-version + shell: pwsh + env: + COPILOT_VERSION: ${{ inputs.copilot-version }} + run: | + if ($env:COPILOT_VERSION -notmatch '\A\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?\z') { + throw "copilot-version must be a pinned semantic version." + } + "version=$env:COPILOT_VERSION" >> $env:GITHUB_OUTPUT + - name: Install GitHub Copilot CLI - run: npm install -g @github/copilot@1.0.82 + run: npm install -g @github/copilot@${{ steps.copilot-version.outputs.version }} shell: pwsh - name: Checkout BC-ALAgents diff --git a/.github/workflows/pr-review-evaluation.yml b/.github/workflows/pr-review-evaluation.yml index 6ac18afdf..e15afc4d2 100644 --- a/.github/workflows/pr-review-evaluation.yml +++ b/.github/workflows/pr-review-evaluation.yml @@ -9,17 +9,37 @@ on: model: description: "Copilot model used internally by BC PR Review" required: false - default: "gpt-5.6-luna" + default: "gpt-5.6-sol" type: choice options: - - "claude-sonnet-5" - - "claude-opus-5" + - "gpt-5.4" - "gpt-5.6-sol" - "gpt-5.6-terra" - "gpt-5.6-luna" - "gpt-5.3-codex" - "mai-code-1.1-flash" - - "gemini-3.7-flash" + leaf-model: + description: "Model requested for domain leaf agents" + required: false + default: "gpt-5.6-luna" + type: choice + options: + - "gpt-5.4" + - "gpt-5.6-luna" + - "mai-code-1.1-flash" + leaf-execution: + description: "Deterministic leaf scheduling mode" + required: false + default: "serial" + type: choice + options: + - "serial" + - "parallel" + max-leaf-concurrency: + description: "Maximum simultaneous leaves in parallel mode" + required: false + default: "4" + type: string engine-sha: description: "Full BC-ALAgents commit SHA (blank uses the default pin)" required: false @@ -46,6 +66,11 @@ on: - "3" - "4" - "5" + entries: + description: "Optional JSON array of entry IDs for a focused experiment" + required: false + default: "" + type: string git-ref: description: "Branch to record for experiment tracking (auto-filled on requeue; leave blank for manual runs)" required: false @@ -95,7 +120,7 @@ jobs: fail-fast: false max-parallel: 64 matrix: - entry: ${{ fromJson(needs.get-entries.outputs.entries) }} + entry: ${{ fromJson(inputs.entries != '' && inputs.entries || needs.get-entries.outputs.entries) }} steps: - name: Checkout repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 @@ -121,6 +146,7 @@ jobs: uses: $/.github/actions/install-agent-harnesses with: engine-sha: ${{ inputs.engine-sha }} + copilot-version: "1.0.88" - name: Run BC PR Review for entry ${{ matrix.entry }} timeout-minutes: 120 @@ -128,6 +154,10 @@ jobs: env: COPILOT_GITHUB_TOKEN: ${{ github.token }} GH_TOKEN: ${{ github.token }} + COPILOT_REVIEW_CLI_VERSION: ${{ steps.install-harnesses.outputs.copilot-version }} + COPILOT_REVIEW_LEAF_MODEL: ${{ inputs.leaf-model }} + COPILOT_REVIEW_LEAF_EXECUTION: ${{ inputs.leaf-execution }} + COPILOT_REVIEW_MAX_LEAF_CONCURRENCY: ${{ inputs.max-leaf-concurrency }} run: | Write-Output "::add-mask::$env:COPILOT_GITHUB_TOKEN" @@ -142,9 +172,32 @@ jobs: if: always() with: name: evaluation-results-${{ github.run_id }}-${{ matrix.entry }} - path: ${{ env.EVALUATION_RESULTS_DIR }}/**/*.jsonl + path: | + ${{ env.EVALUATION_RESULTS_DIR }}/**/*.jsonl + ${{ env.EVALUATION_RESULTS_DIR }}/**/_run-manifest.json retention-days: ${{ (inputs.test-run || inputs.modified-only) && 1 || 30 }} + - name: Upload allowlisted review diagnostics + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + if: always() + with: + name: review-diagnostics-${{ github.run_id }}-${{ matrix.entry }} + path: | + ${{ env.EVALUATION_RESULTS_DIR }}/**/_run-manifest.json + ${{ env.EVALUATION_RESULTS_DIR }}/**/_run-metrics.json + ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-report.json + ${{ env.EVALUATION_RESULTS_DIR }}/**/al-code-review-findings.json + ${{ env.EVALUATION_RESULTS_DIR }}/**/agent-output.txt + ${{ env.EVALUATION_RESULTS_DIR }}/**/agent-transcript.log + ${{ env.EVALUATION_RESULTS_DIR }}/**/leaf-results/*/stdout.txt + ${{ env.EVALUATION_RESULTS_DIR }}/**/leaf-results/*/stderr.txt + ${{ env.EVALUATION_RESULTS_DIR }}/**/_filter-report.json + ${{ env.EVALUATION_RESULTS_DIR }}/**/bcquality/schemas/findings-report.schema.json + ${{ steps.setup-env.outputs.repo_path }}/review.json + ${{ steps.setup-env.outputs.repo_path }}/judge_results.json + if-no-files-found: warn + retention-days: 7 + summarize-results: needs: evaluate-with-pr-review uses: $/.github/workflows/summarize-results.yml @@ -157,7 +210,7 @@ jobs: agent: "BC PR Review" # Scores only the modified entries, so publishing it to Braintrust/Kusto or the # leaderboard would record a partial corpus as a benchmark result. - mock: ${{ inputs.test-run || inputs.modified-only || inputs.engine-sha != '' }} + mock: ${{ inputs.test-run || inputs.modified-only || inputs.engine-sha != '' || inputs.entries != '' }} category: code-review git-ref: ${{ inputs.git-ref || github.ref_name }} secrets: inherit @@ -174,4 +227,4 @@ jobs: repeat: ${{ inputs.repeat }} existing-tag: ${{ needs.pin-commit.outputs.tag-name }} workflow-inputs: | - {"model": "${{ inputs.model }}", "engine-sha": ${{ toJSON(inputs.engine-sha) }}, "test-run": "${{ inputs.test-run }}", "git-ref": "${{ inputs.git-ref || github.ref_name }}"} + {"model": "${{ inputs.model }}", "leaf-model": "${{ inputs.leaf-model }}", "leaf-execution": "${{ inputs.leaf-execution }}", "max-leaf-concurrency": "${{ inputs.max-leaf-concurrency }}", "engine-sha": ${{ toJSON(inputs.engine-sha) }}, "test-run": "${{ inputs.test-run }}", "entries": ${{ toJSON(inputs.entries) }}, "git-ref": "${{ inputs.git-ref || github.ref_name }}"} diff --git a/dataset/codereview.jsonl b/dataset/codereview.jsonl index 35bbe9502..d264b07df 100644 --- a/dataset/codereview.jsonl +++ b/dataset/codereview.jsonl @@ -143,3 +143,6 @@ {"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__http-optional-clean-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpOptionalReturn.Codeunit.al b/src/BCBHttpOptionalReturn.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpOptionalReturn.Codeunit.al\n@@ -0,0 +1,36 @@\n+codeunit 50980 \"BCB HTTP Optional Return\"\n+{\n+ Access = Internal;\n+\n+ internal procedure ReadRate(): Decimal\n+ var\n+ Client: HttpClient;\n+ Response: HttpResponseMessage;\n+ ResponseBody: Text;\n+ Rate: Decimal;\n+ begin\n+ Client.Get(RateUrlTok, Response);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ Response.Content.ReadAs(ResponseBody);\n+ Evaluate(Rate, ResponseBody, 9);\n+ exit(Rate);\n+ end;\n+\n+ internal procedure SendHeartbeat()\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ begin\n+ Content.WriteFrom('{}');\n+ Client.Post(HeartbeatUrlTok, Content, Response);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ end;\n+\n+ var\n+ RateUrlTok: Label 'https://public.example/rate', Locked = true;\n+ HeartbeatUrlTok: Label 'https://public.example/heartbeat', Locked = true;\n+ RequestErr: Label 'The service request failed.';\n+}\n","expected_comments":[],"ignored_comments":[],"category":"code-review","description":"Experimental negative control: bare GET and POST propagate request-execution errors; completed HTTP responses are checked before success processing. Fixed public endpoints and constant payload avoid credential and personal-data transfer concerns. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":false,"source":"http-ab-gate experimental fixture; not accepted for official main"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__http-consumed-false-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpDelivery.Codeunit.al b/src/BCBHttpDelivery.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpDelivery.Codeunit.al\n@@ -0,0 +1,24 @@\n+codeunit 50981 \"BCB HTTP Delivery\"\n+{\n+ Access = Internal;\n+\n+ internal procedure SendHeartbeat(): Boolean\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ RequestSucceeded: Boolean;\n+ begin\n+ Content.WriteFrom('{}');\n+ RequestSucceeded := Client.Post(HeartbeatUrlTok, Content, Response);\n+ if not RequestSucceeded then\n+ exit(true);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ exit(true);\n+ end;\n+\n+ var\n+ HeartbeatUrlTok: Label 'https://public.example/heartbeat', Locked = true;\n+ RequestErr: Label 'The service request failed.';\n+}\n","expected_comments":[{"file":"src/BCBHttpDelivery.Codeunit.al","line_start":14,"line_end":15,"severity":"medium","domain":"error-handling","body":"SendHeartbeat consumes HttpClient.Post's Boolean result but returns true when it is false, reporting successful delivery after a request-execution failure. Propagate the false result or surface the failure instead of returning success. The status check on the successful execution path does not repair the false branch.","articles":[]}],"ignored_comments":[],"category":"code-review","description":"Experimental positive control: an explicitly consumed false HTTP result becomes a true success return. The false path never accesses the response; the completed-exchange path checks status. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":true,"source":"http-ab-gate experimental fixture; not accepted for official main"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__http-status-body-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpRateReader.Codeunit.al b/src/BCBHttpRateReader.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpRateReader.Codeunit.al\n@@ -0,0 +1,22 @@\n+codeunit 50982 \"BCB HTTP Rate Reader\"\n+{\n+ Access = Internal;\n+\n+ internal procedure ReadRate(): Decimal\n+ var\n+ Client: HttpClient;\n+ Response: HttpResponseMessage;\n+ ResponseBody: Text;\n+ Rate: Decimal;\n+ begin\n+ if not Client.Get(RateUrlTok, Response) then\n+ Error(TransportErr);\n+ Response.Content.ReadAs(ResponseBody);\n+ Evaluate(Rate, ResponseBody, 9);\n+ exit(Rate);\n+ end;\n+\n+ var\n+ RateUrlTok: Label 'https://public.example/rate', Locked = true;\n+ TransportErr: Label 'The service request could not be completed.';\n+}\n","expected_comments":[{"file":"src/BCBHttpRateReader.Codeunit.al","line_start":14,"line_end":16,"severity":"medium","domain":"web-services","body":"ReadRate handles a false transport result but parses every completed HTTP response body as the published rate without checking HTTP status. Get can return true for a 4xx or 5xx response, whose error body can cause a misleading numeric parse failure or be returned as business data. Check IsSuccessStatusCode or the expected HTTP status before entering this success parsing path.","articles":[]}],"ignored_comments":[],"category":"code-review","description":"Experimental positive control: false HTTP execution is handled, but a completed non-2xx body enters numeric success parsing. Standard-format numeric conversion avoids a locale-format confound. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":true,"source":"http-ab-gate experimental fixture; not accepted for official main"} diff --git a/docs/code-review.md b/docs/code-review.md index 6e954ca7d..98513adcf 100644 --- a/docs/code-review.md +++ b/docs/code-review.md @@ -51,10 +51,24 @@ The `pr-review` workflow also accepts an `engine-sha` input — a full 40-charac Either way, hold everything else fixed: the benchmark version, the model, the Copilot CLI version the engine uses internally, and the configured minimum severity. Locally, `bcbench evaluate pr-review --engine-path ` requires a clean engine checkout and uses the configured severity; use `bcbench run pr-review` for dirty-checkout smoke tests or `--min-severity` overrides. -BC PR Review records wall-clock duration, prompt/completion/total tokens, and exact AI credits. Usage values come from the engine's strictly validated schema-v1 `_run-metrics.json`, never from console transcripts. API-call details, knowledge-filter counts, token subcategories, completeness diagnostics, and producer metadata remain in that raw artifact rather than being promoted into BC-Bench result and leaderboard schemas. +BC PR Review requires explicit root and leaf models plus deterministic `serial` or bounded `parallel` leaf scheduling. BC-Bench pins only BC-ALAgents; that pinned engine revision owns and resolves its BCQuality dependency. Before scoring, BC-Bench validates the engine's `_run-manifest.json` against the immutable BC-ALAgents commit, pinned Copilot CLI version, resolved scheduling configuration, ordered leaf plan, requested and observed models, process completion, and complete per-process telemetry. The BCQuality revision must be present as engine-produced provenance, but BC-Bench does not select or override it. A mismatched, substituted, reordered, incomplete, or malformed run is rejected rather than included in benchmark results. + +A `partial` engine manifest is a valid production recovery outcome, but not a complete benchmark observation. BC-Bench recognizes it so diagnostics remain clear, then rejects it from scored results alongside failed and incomplete manifests. + +Manifest completion alone is insufficient: the original leaf and root reports must exist, match their declared roles and the pinned BCQuality schema, and preserve every ordered leaf outcome in the root. Failed or partial original reports, missing leaves, nested delegation, and root-only fields on leaf reports are rejected before the gold judge runs. Inactive leaves may report `not-applicable` or `no-knowledge` without findings; these are not evidence of a successful target suppression. + +Generic Copilot evaluations use the action's default CLI pin. PR Review deliberately overrides that pin to the Copilot CLI version whose OpenTelemetry version field the pinned engine validates; update the engine and this paired CLI version together only after an evaluation-identity smoke run succeeds. + +For the experimental CLI `1.0.88` pairing, the engine requires an exact startup version probe but permits absent `cli_version` in OTel. The adapter accepts that documented absence only for `1.0.88`, preserves the telemetry `null`, and still rejects every nonempty conflicting version. Read `_run-manifest.json.configuration.copilot_cli_version` as startup provenance, not as an observed OTel field. + +BC PR Review records wall-clock duration, prompt/completion/total tokens, and exact AI credits. Usage values come from the engine's strictly validated schema-v1 `_run-metrics.json`, never from console transcripts. The validated manifest also records the leaf model, scheduling mode, maximum concurrency, BCQuality revision and source snapshot, and process count on every result. API-call details, knowledge-filter counts, token subcategories, completeness diagnostics, and producer metadata remain in the raw artifacts rather than being promoted into leaderboard schemas. Unavailable AI credits remain `null` in bceval exports; observed zero remains zero. The pinned bc-eval 0.3.14 consumer requires numeric prompt/completion tokens, so its existing zero fallbacks for missing tokens remain unchanged. Use the original per-entry result metrics, not bceval token fields, to distinguish unknown usage from measured zero. +The HTTP optional-return A/B preparation on this experimental branch uses four unchanged historical target entries, the existing `synthetic__errh-tryfunction-swallowed-01` positive control, and three experimental HTTP controls. The new controls are structural fixtures, not AL-compiled tests, and require separate human acceptance. Their HTTP articles are intentionally unannotated because those articles do not exist at the pre-196 baseline. No official gold is reclassified. + +Focused A/B runs require an explicit `entries` array, `test-run=true`, `modified-only=false`, `repeat=1`, and an explicit `engine-sha` equal to that arm's declared action pin. This selects the exact matrix, disables requeue and ephemeral tags, and makes summary `mock=true`: the existing gold judge and reviewer still run, but Braintrust/Kusto storage arguments and leaderboard writes are disabled. Preparation is not permission to dispatch. The diagnostic upload is a filename allowlist; download results and logs promptly (test results expire after one day, diagnostic artifacts after seven). Raw OTel is deleted by the engine after ingestion and is not promised by these artifacts. + ## Baseline Leaderboard {% if site.data.code-review.aggregate and site.data.code-review.aggregate.size > 0 %} diff --git a/pyproject.toml b/pyproject.toml index 4efdafc4c..3462ba2bd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "bcbench" -version = "0.13.0" +version = "0.14.0.dev1" description = "Benchmarking tool for Business Central (AL) ecosystem, inspired by SWE-Bench" readme = "README.md" requires-python = ">=3.13,<3.14" diff --git a/src/bcbench/agent/pr_review/agent.py b/src/bcbench/agent/pr_review/agent.py index 76c04de85..cc5b2ad5c 100644 --- a/src/bcbench/agent/pr_review/agent.py +++ b/src/bcbench/agent/pr_review/agent.py @@ -21,8 +21,10 @@ import yaml +from bcbench.agent.copilot.cli import get_copilot_version 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.agent.pr_review.run_manifest import RUN_MANIFEST_FILE_NAME, load_run_manifest, validate_review_reports, validate_run_manifest from bcbench.config import get_config from bcbench.dataset import BaseDatasetEntry from bcbench.dataset.codereview import CodeReviewEntry @@ -37,6 +39,7 @@ _FINDINGS_OUTPUT_FILE = "al-code-review-findings.json" _REVIEW_OUTPUT_FILE = "review.json" _PREPARE_BCQUALITY_SCRIPT = Path(__file__).parent / "scripts" / "Prepare-BCQualityRoot.ps1" +_COPILOT_CLI_VERSION_ENV = "COPILOT_REVIEW_CLI_VERSION" def _load_pr_review_settings() -> dict[str, Any]: @@ -54,7 +57,7 @@ def _resolve_pr_review_root(engine_path: Path | None) -> Path: return root -def get_pr_review_version(engine_path: Path | None) -> str: +def get_pr_review_version(engine_path: Path | None, *, require_clean: bool = True) -> str: root = _resolve_pr_review_root(engine_path) try: result = subprocess.run( @@ -68,7 +71,7 @@ def get_pr_review_version(engine_path: Path | None) -> str: git_root, commit = result.stdout.strip().splitlines() if Path(git_root).resolve() != root or re.fullmatch(r"[0-9a-f]{40}", commit) is None: raise AgentError(f"PR Review engine path must be the root of a Git checkout: {root}") - if has_changes(root): + if require_clean and has_changes(root): raise AgentError("PR Review evaluations require a clean engine checkout. Commit changes first, or use 'bcbench run pr-review' for a smoke test.") except (OSError, subprocess.SubprocessError, ValueError) as exc: raise AgentError(f"Could not determine PR Review engine version at {root}: {exc}") from exc @@ -82,6 +85,14 @@ def _resolve_pwsh() -> str: return pwsh +def _resolve_pr_review_cli_version(cli_version: str | None = None) -> str: + selected_version = cli_version.strip() if cli_version is not None else get_copilot_version() + cli_version = selected_version.strip() + if re.fullmatch(r"\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?", cli_version) is None: + raise AgentError(f"{_COPILOT_CLI_VERSION_ENV} must be the installed pinned Copilot CLI semantic version.") + return cli_version + + def _environment_without_bcquality_overrides() -> dict[str, str]: return {name: value for name, value in os.environ.items() if not name.startswith("BCQUALITY_")} @@ -159,8 +170,14 @@ def run_pr_review_agent( category: EvaluationCategory, repo_path: Path, output_dir: Path, + agent_version: str, engine_path: Path | None = None, min_severity: str | None = None, + cli_version: str | None = None, + leaf_model: str | None = None, + leaf_execution: str | None = None, + max_leaf_concurrency: int | None = None, + cli_timeout_minutes: int | None = None, ) -> tuple[PRReviewMetrics, ExperimentConfiguration]: """Run the engine's complete local review pipeline and write review.json. @@ -189,6 +206,21 @@ def run_pr_review_agent( _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") + cli_version = _resolve_pr_review_cli_version(cli_version) + leaf_model = (leaf_model or model).strip() + if not leaf_model: + raise AgentError("PR Review leaf model must not be empty.") + leaf_execution = (leaf_execution or "serial").strip().lower() + if leaf_execution not in {"serial", "parallel"}: + raise AgentError("PR Review leaf execution must be 'serial' or 'parallel'.") + if max_leaf_concurrency is None: + max_leaf_concurrency = 4 + if max_leaf_concurrency < 1: + raise AgentError("PR Review max leaf concurrency must be a positive integer.") + if cli_timeout_minutes is None: + cli_timeout_minutes = 30 + if cli_timeout_minutes < 0: + raise AgentError("PR Review CLI timeout minutes must be a non-negative integer.") engine = engine_root / "agents" / "ALReviewAgent" / "scripts" / "Invoke-CopilotPRReview.ps1" env = { @@ -202,6 +234,11 @@ def run_pr_review_agent( "BCQUALITY_ROOT": str(bcquality_root), "GITHUB_REPOSITORY": entry.repo, "COPILOT_MODEL": model, + _COPILOT_CLI_VERSION_ENV: cli_version, + "COPILOT_REVIEW_LEAF_MODEL": leaf_model, + "COPILOT_REVIEW_LEAF_EXECUTION": leaf_execution, + "COPILOT_REVIEW_MAX_LEAF_CONCURRENCY": str(max_leaf_concurrency), + "COPILOT_REVIEW_CLI_TIMEOUT_MINUTES": str(cli_timeout_minutes), "AGENT_MINIMUM_SEVERITY": severity, } @@ -222,6 +259,20 @@ def run_pr_review_agent( logger.debug(f"Engine stdout:\n{result.stdout}") if result.stderr: logger.debug(f"Engine stderr:\n{result.stderr}") + manifest = load_run_manifest(output_dir / RUN_MANIFEST_FILE_NAME) + validate_run_manifest( + manifest, + engine_commit=agent_version, + cli_version=cli_version, + root_model=model, + leaf_model=leaf_model, + leaf_execution=leaf_execution, + max_leaf_concurrency=max_leaf_concurrency, + cli_timeout_minutes=cli_timeout_minutes, + minimum_severity=settings["min_severity"], + agent_minimum_severity=severity, + ) + validate_review_reports(manifest, output_dir) 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: @@ -235,4 +286,4 @@ def run_pr_review_agent( logger.exception("Unexpected error running engine review") raise else: - return build_pr_review_metrics(output_dir, time.monotonic() - start), config + return build_pr_review_metrics(output_dir, time.monotonic() - start, manifest), config diff --git a/src/bcbench/agent/pr_review/metrics.py b/src/bcbench/agent/pr_review/metrics.py index 0d3204697..5629d571c 100644 --- a/src/bcbench/agent/pr_review/metrics.py +++ b/src/bcbench/agent/pr_review/metrics.py @@ -4,6 +4,7 @@ from pydantic import BaseModel, ConfigDict, Field, ValidationError, model_validator +from bcbench.agent.pr_review.run_manifest import RunManifest, matches_cli_telemetry_version from bcbench.exceptions import AgentError from bcbench.types import PRReviewMetrics @@ -76,25 +77,41 @@ def _load_run_metrics(path: Path) -> _RunMetrics: raise AgentError(f"Engine run metrics artifact {path} does not satisfy schema version 1: {exc}") from exc -def build_pr_review_metrics(output_dir: Path, execution_time: float) -> PRReviewMetrics: +def build_pr_review_metrics(output_dir: Path, execution_time: float, manifest: RunManifest | None = None) -> PRReviewMetrics: run = _load_run_metrics(output_dir / RUN_METRICS_FILE_NAME) if run.metrics_source == "not-applicable": raise AgentError("Engine metrics were not applicable. BC-Bench code-review entries must contain AL changes.") + if manifest: + expected_models = {manifest.configuration.root_model, manifest.configuration.leaf_model} + if ( + not matches_cli_telemetry_version(run.cli_version, manifest.configuration.copilot_cli_version) + or not run.usage_complete + or run.malformed_records != 0 + or len(run.models) != len(set(run.models)) + or set(run.models) != expected_models + ): + raise AgentError("Engine aggregate metrics do not match the validated run manifest.") usage_values_available = run.malformed_records == 0 - token_values_available = usage_values_available and run.usage_complete return PRReviewMetrics( execution_time=execution_time, - prompt_tokens=run.prompt_tokens if token_values_available else None, - completion_tokens=run.completion_tokens if token_values_available else None, - total_tokens=run.total_tokens if token_values_available else None, + prompt_tokens=run.prompt_tokens if usage_values_available else None, + cached_tokens=run.cached_tokens if usage_values_available else None, + cache_creation_tokens=run.cache_creation_tokens if usage_values_available else None, + completion_tokens=run.completion_tokens if usage_values_available else None, + reasoning_tokens=run.reasoning_tokens if usage_values_available else None, + total_tokens=run.total_tokens if usage_values_available else None, + api_calls=run.api_calls if usage_values_available else None, + failed_api_calls=run.failed_api_calls if usage_values_available else None, + usage_api_calls=run.usage_api_calls if usage_values_available else None, ai_credits=run.ai_credits if usage_values_available else None, - cached_tokens=run.cached_tokens, - cache_creation_tokens=run.cache_creation_tokens, - reasoning_tokens=run.reasoning_tokens, - api_calls=run.api_calls, - failed_api_calls=run.failed_api_calls, - usage_api_calls=run.usage_api_calls, + models=run.models, usage_complete=run.usage_complete, malformed_records=run.malformed_records, copilot_cli_version=run.cli_version, + leaf_model=manifest.configuration.leaf_model if manifest else None, + leaf_execution=manifest.configuration.leaf_execution if manifest else None, + max_leaf_concurrency=manifest.configuration.max_leaf_concurrency if manifest else None, + bcquality_commit=manifest.bcquality.commit if manifest else None, + bcquality_source_snapshot=manifest.bcquality.source_snapshot if manifest else None, + review_process_count=len(manifest.processes) if manifest else None, ) diff --git a/src/bcbench/agent/pr_review/run_manifest.py b/src/bcbench/agent/pr_review/run_manifest.py new file mode 100644 index 000000000..2cf4486b1 --- /dev/null +++ b/src/bcbench/agent/pr_review/run_manifest.py @@ -0,0 +1,259 @@ +import json +from pathlib import Path +from typing import Any, Literal + +import jsonschema +from pydantic import BaseModel, ConfigDict, Field, ValidationError, model_validator + +from bcbench.exceptions import AgentError + +RUN_MANIFEST_FILE_NAME = "_run-manifest.json" + + +class ProcessMetrics(BaseModel): + model_config = ConfigDict(extra="allow", frozen=True, strict=True) + + cli_version: str | None + models: list[str] + usage_complete: bool + malformed_records: int = Field(ge=0) + + +class ProcessRecord(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + role: Literal["leaf", "root"] + ordinal: int = Field(ge=1) + skill_id: str + requested_model: str + observed_models: list[str] + status: Literal["completed", "failed"] + started_at: str + completed_at: str + duration_seconds: float = Field(ge=0) + exit_code: int | None + report_path: str | None + failure_reason: str | None + metrics: ProcessMetrics | None + + +class RunConfiguration(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + copilot_cli_version: str + root_model: str + leaf_model: str + leaf_execution: Literal["serial", "parallel"] + max_leaf_concurrency: int = Field(ge=1) + cli_timeout_minutes: int = Field(ge=0) + minimum_severity: Literal["Critical", "High", "Medium", "Low"] + agent_minimum_severity: Literal["Critical", "High", "Medium", "Low"] + review_source: Literal["pr", "local"] + + +class ReviewPlan(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + skill_id: Literal["al-code-review"] + leaf_count: int = Field(ge=1) + leaf_ids: list[str] + + @model_validator(mode="after") + def validate_leaf_count(self) -> "ReviewPlan": + if self.leaf_count != len(self.leaf_ids): + raise ValueError("leaf_count does not match leaf_ids") + if len(self.leaf_ids) != len(set(self.leaf_ids)): + raise ValueError("leaf_ids contains duplicates") + return self + + +class Revision(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + commit: str | None = Field(pattern=r"^[0-9a-f]{40}$") + + +class EngineRevision(Revision): + repository: Literal["microsoft/BC-ALAgents"] + agent_version: str + + +class BCQualityRevision(Revision): + source_snapshot: str | None = Field(pattern=r"^[0-9a-f]{64}$") + + +class RunManifest(BaseModel): + model_config = ConfigDict(extra="forbid", frozen=True, strict=True) + + schema_version: Literal[1] + status: Literal["running", "completed", "partial", "failed"] + started_at: str + completed_at: str | None + failure_reason: str | None + engine: EngineRevision + bcquality: BCQualityRevision + configuration: RunConfiguration + plan: ReviewPlan + processes: list[ProcessRecord] + + @model_validator(mode="after") + def validate_lifecycle(self) -> "RunManifest": + if self.status == "running": + if self.completed_at is not None: + raise ValueError("running manifest must not have completed_at") + if self.failure_reason is not None: + raise ValueError("running manifest must not have failure_reason") + elif self.status == "completed": + if self.completed_at is None: + raise ValueError("completed manifest requires completed_at") + if self.failure_reason is not None: + raise ValueError("completed manifest must not have failure_reason") + elif self.status == "partial": + if self.completed_at is None: + raise ValueError("partial manifest requires completed_at") + elif self.status == "failed": + if self.completed_at is None: + raise ValueError("failed manifest requires completed_at") + if not self.failure_reason: + raise ValueError("failed manifest requires failure_reason") + return self + + +def load_run_manifest(path: Path) -> RunManifest: + if not path.exists(): + raise AgentError(f"Engine run manifest 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 manifest artifact {path}: {exc}") from exc + try: + return RunManifest.model_validate(payload) + except ValidationError as exc: + raise AgentError(f"Engine run manifest artifact {path} does not satisfy schema version 1: {exc}") from exc + + +def validate_run_manifest( + manifest: RunManifest, + *, + engine_commit: str, + cli_version: str, + root_model: str, + leaf_model: str, + leaf_execution: str, + max_leaf_concurrency: int, + cli_timeout_minutes: int, + minimum_severity: str, + agent_minimum_severity: str, +) -> None: + expected_configuration = { + "copilot_cli_version": cli_version, + "root_model": root_model, + "leaf_model": leaf_model, + "leaf_execution": leaf_execution, + "max_leaf_concurrency": max_leaf_concurrency, + "cli_timeout_minutes": cli_timeout_minutes, + "minimum_severity": minimum_severity, + "agent_minimum_severity": agent_minimum_severity, + "review_source": "local", + } + actual_configuration = manifest.configuration.model_dump() + mismatches = [f"{name}={actual_configuration[name]!r} (expected {expected!r})" for name, expected in expected_configuration.items() if actual_configuration[name] != expected] + if manifest.status != "completed": + mismatches.append(f"status={manifest.status!r} (expected 'completed')") + if manifest.engine.commit != engine_commit: + mismatches.append(f"engine.commit={manifest.engine.commit!r} (expected {engine_commit!r})") + if manifest.bcquality.commit is None: + mismatches.append("bcquality.commit is missing") + if manifest.bcquality.source_snapshot is None: + mismatches.append("bcquality.source_snapshot is missing") + + expected_process_count = manifest.plan.leaf_count + 1 + if len(manifest.processes) != expected_process_count: + mismatches.append(f"process count={len(manifest.processes)} (expected {expected_process_count})") + else: + expected_ordinals = list(range(1, expected_process_count + 1)) + if [process.ordinal for process in manifest.processes] != expected_ordinals: + mismatches.append("process ordinals do not match the ordered leaf plan") + leaf_processes = manifest.processes[:-1] + root_process = manifest.processes[-1] + if [process.skill_id for process in leaf_processes] != manifest.plan.leaf_ids: + mismatches.append("leaf process IDs do not match the ordered leaf plan") + if any(process.role != "leaf" for process in leaf_processes): + mismatches.append("one or more planned leaf processes have a non-leaf role") + if root_process.role != "root" or root_process.skill_id != "al-code-review": + mismatches.append("final process is not the al-code-review root consolidation") + + for process in manifest.processes: + expected_model = leaf_model if process.role == "leaf" else root_model + if process.status != "completed" or process.exit_code != 0: + mismatches.append(f"{process.skill_id} did not complete successfully") + if process.requested_model != expected_model or process.observed_models != [expected_model]: + mismatches.append(f"{process.skill_id} model telemetry does not match {expected_model!r}") + if process.metrics is None: + mismatches.append(f"{process.skill_id} has no process metrics") + elif ( + not matches_cli_telemetry_version(process.metrics.cli_version, cli_version) + or not process.metrics.usage_complete + or process.metrics.malformed_records != 0 + or process.metrics.models != [expected_model] + ): + mismatches.append(f"{process.skill_id} has incomplete or mismatched process metrics") + + if mismatches: + raise AgentError("Engine run manifest does not match the pinned experiment: " + "; ".join(mismatches)) + + +def matches_cli_telemetry_version(observed: str | None, configured: str) -> bool: + # Engine 2ad4f65's compatibility policy requires a startup probe but permits + # missing OTel cli_version for 1.0.88. Never fill missing telemetry from a pin. + return observed == configured or (observed is None and configured == "1.0.88") + + +def _read_json_object(path: Path) -> dict[str, Any]: + try: + payload = json.loads(path.read_text(encoding="utf-8-sig")) + except (OSError, json.JSONDecodeError) as exc: + raise AgentError(f"Required review artifact is missing or unreadable: {path}") from exc + if not isinstance(payload, dict): + raise AgentError(f"Required review artifact is not a JSON object: {path}") + return payload + + +def validate_review_reports(manifest: RunManifest, output_dir: Path) -> None: + schema = _read_json_object(output_dir / "bcquality" / "schemas" / "findings-report.schema.json") + reports: dict[str, dict[str, Any]] = {} + root = output_dir.resolve() + for process in manifest.processes: + if not process.report_path: + raise AgentError(f"{process.skill_id} has no original report path.") + path = (root / process.report_path).resolve() + if not path.is_relative_to(root): + raise AgentError(f"{process.skill_id} report path escapes the output directory.") + report = _read_json_object(path) + try: + jsonschema.validate(report, schema) + except (jsonschema.ValidationError, jsonschema.SchemaError) as exc: + raise AgentError(f"{process.skill_id} original report fails the pinned BCQuality schema: {exc.message}") from exc + if report.get("skill", {}).get("id") != process.skill_id: + raise AgentError(f"{process.skill_id} original report has the wrong skill identity.") + if process.role == "leaf" and any(key in report for key in ("sub-results", "skipped-sub-skills")): + raise AgentError(f"{process.skill_id} original leaf report contains root-only fields.") + allowed = {"completed"} if process.role == "root" else {"completed", "not-applicable", "no-knowledge"} + if report.get("outcome") not in allowed: + raise AgentError(f"{process.skill_id} original report has unusable outcome {report.get('outcome')!r}.") + if report.get("outcome") != "completed" and report.get("findings"): + raise AgentError(f"{process.skill_id} inactive leaf report contains findings.") + reports[process.skill_id] = report + + root_report = reports["al-code-review"] + nested = root_report.get("sub-results", []) + if [item.get("skill", {}).get("id") for item in nested] != manifest.plan.leaf_ids: + raise AgentError("Original root report does not preserve the complete ordered leaf plan.") + if root_report.get("skipped-sub-skills"): + raise AgentError("Original root report skips planned leaves.") + for item in nested: + skill_id = item["skill"]["id"] + if item.get("outcome") != reports[skill_id]["outcome"]: + raise AgentError(f"{skill_id} root sub-result outcome disagrees with its original leaf report.") + if any(key in item for key in ("sub-results", "skipped-sub-skills")): + raise AgentError(f"{skill_id} root sub-result contains unexpected nested delegation.") diff --git a/src/bcbench/cli_options.py b/src/bcbench/cli_options.py index a1e3c5187..67e4cbdc5 100644 --- a/src/bcbench/cli_options.py +++ b/src/bcbench/cli_options.py @@ -24,6 +24,53 @@ ), ] +PRReviewCliVersion = Annotated[ + str | None, + typer.Option( + "--cli-version", + envvar="COPILOT_REVIEW_CLI_VERSION", + help="GitHub Copilot CLI version expected in the PR Review engine manifest; omitted probes the installed CLI", + ), +] + +PRReviewLeafModel = Annotated[ + str | None, + typer.Option( + "--leaf-model", + envvar="COPILOT_REVIEW_LEAF_MODEL", + help="Copilot model requested for PR Review domain leaf agents; defaults to the root model", + ), +] + +PRReviewLeafExecution = Annotated[ + str | None, + typer.Option( + "--leaf-execution", + envvar="COPILOT_REVIEW_LEAF_EXECUTION", + help="PR Review leaf scheduling mode: serial or parallel", + ), +] + +PRReviewMaxLeafConcurrency = Annotated[ + int, + typer.Option( + "--max-leaf-concurrency", + envvar="COPILOT_REVIEW_MAX_LEAF_CONCURRENCY", + min=1, + help="Maximum simultaneous PR Review leaf agents in parallel mode", + ), +] + +PRReviewCliTimeoutMinutes = Annotated[ + int, + typer.Option( + "--cli-timeout-minutes", + envvar="COPILOT_REVIEW_CLI_TIMEOUT_MINUTES", + min=0, + help="Copilot CLI child-process timeout recorded in the PR Review engine manifest", + ), +] + OutputDir = Annotated[Path, typer.Option(help="Directory to save evaluation results", file_okay=False, dir_okay=True)] RunId = Annotated[str, typer.Option(envvar="GITHUB_RUN_ID", help="Unique identifier for this evaluation run")] @@ -116,6 +163,7 @@ def resolve_evaluation_runtime( CopilotModelName = Literal[ "claude-sonnet-5", "claude-opus-5", + "gpt-5.4", "gpt-5.6-sol", "gpt-5.6-terra", "gpt-5.6-luna", diff --git a/src/bcbench/commands/evaluate.py b/src/bcbench/commands/evaluate.py index 2710dfcfb..105b83548 100644 --- a/src/bcbench/commands/evaluate.py +++ b/src/bcbench/commands/evaluate.py @@ -17,7 +17,12 @@ CopilotModel, EvaluationCategoryOption, OutputDir, + PRReviewCliTimeoutMinutes, + PRReviewCliVersion, PRReviewEnginePath, + PRReviewLeafExecution, + PRReviewLeafModel, + PRReviewMaxLeafConcurrency, RepoPath, RunId, resolve_evaluation_runtime, @@ -185,6 +190,11 @@ def evaluate_pr_review( output_dir: OutputDir = _config.paths.evaluation_results_path, run_id: RunId = "pr_review_test_run", engine_path: PRReviewEnginePath = None, + cli_version: PRReviewCliVersion = None, + leaf_model: PRReviewLeafModel = None, + leaf_execution: PRReviewLeafExecution = None, + max_leaf_concurrency: PRReviewMaxLeafConcurrency = 4, + cli_timeout_minutes: PRReviewCliTimeoutMinutes = 30, ) -> None: """ Evaluate BC PR Review on a single code-review entry. @@ -203,6 +213,7 @@ def evaluate_pr_review( logger.info(f"Running evaluation on entry {entry_id} with the BC-ALAgents review engine") + agent_version = get_pr_review_version(engine_path) context = EvaluationContext( entry=entry, repo_path=repo_path, @@ -210,7 +221,7 @@ def evaluate_pr_review( container=None, model=model, agent_name=AgentHarness.PR_REVIEW, - agent_version=get_pr_review_version(engine_path), + agent_version=agent_version, category=category, ) @@ -222,7 +233,13 @@ def evaluate_pr_review( category=category, model=ctx.model, output_dir=ctx.result_dir, + agent_version=agent_version, engine_path=engine_path, + cli_version=cli_version, + leaf_model=leaf_model, + leaf_execution=leaf_execution, + max_leaf_concurrency=max_leaf_concurrency, + cli_timeout_minutes=cli_timeout_minutes, ), ) diff --git a/src/bcbench/commands/run.py b/src/bcbench/commands/run.py index b0fb2a839..086293548 100644 --- a/src/bcbench/commands/run.py +++ b/src/bcbench/commands/run.py @@ -4,7 +4,7 @@ import typer -from bcbench.agent import BCalBackendConfig, run_bcal_agent, run_claude_code, run_copilot_agent, run_pr_review_agent +from bcbench.agent import BCalBackendConfig, get_pr_review_version, run_bcal_agent, run_claude_code, run_copilot_agent, run_pr_review_agent from bcbench.cli_options import ( ClaudeCodeModel, ContainerCompany, @@ -17,7 +17,12 @@ CopilotModel, EvaluationCategoryOption, OutputDir, + PRReviewCliTimeoutMinutes, + PRReviewCliVersion, PRReviewEnginePath, + PRReviewLeafExecution, + PRReviewLeafModel, + PRReviewMaxLeafConcurrency, RepoPath, resolve_agent_runtime, ) @@ -142,6 +147,11 @@ def run_pr_review( output_dir: OutputDir = _config.paths.evaluation_results_path, engine_path: PRReviewEnginePath = None, min_severity: Annotated[str | None, typer.Option(help="AGENT_MINIMUM_SEVERITY floor (defaults to config)")] = None, + cli_version: PRReviewCliVersion = None, + leaf_model: PRReviewLeafModel = None, + leaf_execution: PRReviewLeafExecution = None, + max_leaf_concurrency: PRReviewMaxLeafConcurrency = 4, + cli_timeout_minutes: PRReviewCliTimeoutMinutes = 30, ) -> None: """ Run BC PR Review on a single code-review entry. @@ -166,8 +176,14 @@ def run_pr_review( repo_path=repo_path, category=category, output_dir=output_dir, + agent_version=get_pr_review_version(engine_path, require_clean=False), engine_path=engine_path, min_severity=min_severity, + cli_version=cli_version, + leaf_model=leaf_model, + leaf_execution=leaf_execution, + max_leaf_concurrency=max_leaf_concurrency, + cli_timeout_minutes=cli_timeout_minutes, ) diff --git a/src/bcbench/types.py b/src/bcbench/types.py index 7e810fd4a..3826e085f 100644 --- a/src/bcbench/types.py +++ b/src/bcbench/types.py @@ -100,9 +100,16 @@ class PRReviewMetrics(AgentMetrics): api_calls: int | None = Field(default=None, ge=0) failed_api_calls: int | None = Field(default=None, ge=0) usage_api_calls: int | None = Field(default=None, ge=0) + models: list[str] = Field(default_factory=list) usage_complete: bool | None = None malformed_records: int | None = Field(default=None, ge=0) copilot_cli_version: str | None = None + leaf_model: str | None = None + leaf_execution: Literal["serial", "parallel"] | None = None + max_leaf_concurrency: int | None = Field(default=None, ge=1) + bcquality_commit: str | None = None + bcquality_source_snapshot: str | None = None + review_process_count: int | None = Field(default=None, ge=1) type AnyAgentMetrics = Annotated[AgentMetrics | PRReviewMetrics, Field(discriminator="kind")] diff --git a/tests/test_agent_versions.py b/tests/test_agent_versions.py index 628a39dca..10cccac8c 100644 --- a/tests/test_agent_versions.py +++ b/tests/test_agent_versions.py @@ -100,6 +100,13 @@ def test_dirty_engine_cannot_be_reported_as_a_commit(engine_root: Path, staged: get_pr_review_version(engine_root) +def test_dirty_engine_commit_can_be_resolved_for_local_smoke_run(engine_root: Path) -> None: + (engine_root / "new-rule.md").write_text("New behavior", encoding="utf-8") + expected = subprocess.run(["git", "-C", str(engine_root), "rev-parse", "HEAD"], capture_output=True, text=True, check=True).stdout.strip() + + assert get_pr_review_version(engine_root, require_clean=False) == expected + + def test_engine_path_must_be_the_checkout_root(engine_root: Path) -> None: nested = engine_root / "nested" _write_engine(nested) diff --git a/tests/test_codereview_fixtures.py b/tests/test_codereview_fixtures.py index 173d0080a..376cd5aa4 100644 --- a/tests/test_codereview_fixtures.py +++ b/tests/test_codereview_fixtures.py @@ -1,3 +1,4 @@ +import hashlib import json import subprocess @@ -11,7 +12,10 @@ _SCAN = "synthetic__performance-014" _API = "synthetic__performance-009" _GROUPS = "synthetic__error-handling-drilldown-position-01" -_ENTRY_IDS = (_SCAN, _API, _GROUPS) +_HTTP_CLEAN = "synthetic__http-optional-clean-01" +_HTTP_FALSE = "synthetic__http-consumed-false-01" +_HTTP_STATUS = "synthetic__http-status-body-01" +_ENTRY_IDS = (_SCAN, _API, _GROUPS, _HTTP_CLEAN, _HTTP_FALSE, _HTTP_STATUS) def _unique_object(pairs): @@ -19,6 +23,13 @@ def _unique_object(pairs): return dict(pairs) +def test_http_experiment_preserves_the_original_corpus(): + lines = EvaluationCategory.CODE_REVIEW.dataset_path.read_bytes().splitlines(keepends=True) + original = b"".join(line.replace(b"\r\n", b"\n") for line in lines if json.loads(line)["instance_id"] not in {_HTTP_CLEAN, _HTTP_FALSE, _HTTP_STATUS}) + assert len(original.splitlines()) == 145 + assert hashlib.sha256(original).hexdigest() == "43bcddc685e3f81cb655fc0efe33f7c47763fe64864ecf62ab69220a231e513c" + + @pytest.fixture(scope="module") def materialized_entries(tmp_path_factory): dataset = EvaluationCategory.CODE_REVIEW.dataset_path @@ -170,3 +181,62 @@ def test_regenerated_header_ids_lose_position_but_business_keys_restore_it(mater restored_id = next(key for key, code in rows.items() if code == selected_code) assert restored_id != old_id assert rows[restored_id] == selected_code + + +@pytest.mark.parametrize("entry_id", [_HTTP_CLEAN, _HTTP_FALSE, _HTTP_STATUS]) +def test_http_controls_have_no_unavailable_article_or_steering_comments(materialized_entries, entry_id): + entry, files = materialized_entries[entry_id] + assert entry.declared_articles() == set() + assert entry.ignored_comments == [] + for source in files.values(): + assert "Access = Internal;" in source + assert "internal procedure " in source + assert "//" not in source.replace("https://", "") + assert "Record " not in source + assert "Authorization" not in source + assert "SecretText" not in source + assert "[TryFunction]" not in source + assert "https://public.example/" in source + + +def test_http_bare_calls_check_status_without_consuming_optional_result(materialized_entries): + entry, files = materialized_entries[_HTTP_CLEAN] + source = files["src/BCBHttpOptionalReturn.Codeunit.al"] + assert entry.expected_comments == [] + assert " Client.Get(RateUrlTok, Response);\n if not Response.IsSuccessStatusCode() then\n Error(RequestErr);" in source + assert " Client.Post(HeartbeatUrlTok, Content, Response);\n if not Response.IsSuccessStatusCode() then\n Error(RequestErr);" in source + assert "Evaluate(Rate, ResponseBody, 9);" in source + assert "Content.WriteFrom('{}');" in source + + +def test_http_consumed_false_reports_success_but_does_not_read_invalid_response(materialized_entries): + entry, files = materialized_entries[_HTTP_FALSE] + source = files["src/BCBHttpDelivery.Codeunit.al"] + assert len(entry.expected_comments) == 1 + assert entry.expected_comments[0].severity == Severity.MEDIUM + assert source.splitlines()[13:17] == [ + " if not RequestSucceeded then", + " exit(true);", + " if not Response.IsSuccessStatusCode() then", + " Error(RequestErr);", + ] + assert "RequestSucceeded := Client.Post(HeartbeatUrlTok, Content, Response);" in source + assert "Response.Content" not in source + assert "returns true when it is false" in entry.expected_comments[0].body + + +def test_http_completed_error_body_reaches_numeric_success_path(materialized_entries): + entry, files = materialized_entries[_HTTP_STATUS] + source = files["src/BCBHttpRateReader.Codeunit.al"] + assert len(entry.expected_comments) == 1 + assert entry.expected_comments[0].severity == Severity.MEDIUM + assert source.splitlines()[11:16] == [ + " if not Client.Get(RateUrlTok, Response) then", + " Error(TransportErr);", + " Response.Content.ReadAs(ResponseBody);", + " Evaluate(Rate, ResponseBody, 9);", + " exit(Rate);", + ] + assert "IsSuccessStatusCode" not in source + assert "HttpStatusCode" not in source + assert "4xx or 5xx" in entry.expected_comments[0].body diff --git a/tests/test_pr_review_agent.py b/tests/test_pr_review_agent.py index 664bdd007..9b07f098f 100644 --- a/tests/test_pr_review_agent.py +++ b/tests/test_pr_review_agent.py @@ -5,7 +5,7 @@ import pytest -from bcbench.agent.pr_review.agent import _prepare_bcquality_root, _resolve_pr_review_root, _write_review_json, run_pr_review_agent +from bcbench.agent.pr_review.agent import _prepare_bcquality_root, _resolve_pr_review_cli_version, _resolve_pr_review_root, _write_review_json, run_pr_review_agent from bcbench.exceptions import AgentError from bcbench.types import EvaluationCategory, PRReviewMetrics from tests.conftest import create_codereview_entry @@ -23,6 +23,67 @@ def _write_output(output_dir: Path, text: str) -> None: (output_dir / "al-code-review-findings.json").write_text(text, encoding="utf-8") +def _write_run_manifest(output_dir: Path, *, root_model: str, leaf_model: str) -> None: + def process(role: str, ordinal: int, skill_id: str, model: str) -> dict: + metrics = { + "cli_version": "1.0.83", + "models": [model], + "usage_complete": True, + "malformed_records": 0, + } + return { + "role": role, + "ordinal": ordinal, + "skill_id": skill_id, + "requested_model": model, + "observed_models": [model], + "status": "completed", + "started_at": "2026-09-14T12:00:00Z", + "completed_at": "2026-09-14T12:00:01Z", + "duration_seconds": 1.0, + "exit_code": 0, + "report_path": f"{role}/{ordinal}/_review-report.json", + "failure_reason": None, + "metrics": metrics, + } + + payload = { + "schema_version": 1, + "status": "completed", + "started_at": "2026-09-14T12:00:00Z", + "completed_at": "2026-09-14T12:00:02Z", + "failure_reason": None, + "engine": {"repository": "microsoft/BC-ALAgents", "commit": "e" * 40, "agent_version": "1.6.6"}, + "bcquality": {"commit": "b" * 40, "source_snapshot": "a" * 64}, + "configuration": { + "copilot_cli_version": "1.0.83", + "root_model": root_model, + "leaf_model": leaf_model, + "leaf_execution": "serial", + "max_leaf_concurrency": 4, + "cli_timeout_minutes": 30, + "minimum_severity": "Medium", + "agent_minimum_severity": "Medium", + "review_source": "local", + }, + "plan": {"skill_id": "al-code-review", "leaf_count": 1, "leaf_ids": ["al-performance-review"]}, + "processes": [ + process("leaf", 1, "al-performance-review", leaf_model), + process("root", 2, "al-code-review", root_model), + ], + } + (output_dir / "_run-manifest.json").write_text(json.dumps(payload), encoding="utf-8") + schema_dir = output_dir / "bcquality" / "schemas" + schema_dir.mkdir(parents=True, exist_ok=True) + (schema_dir / "findings-report.schema.json").write_text('{"type": "object"}', encoding="utf-8") + leaf_report = {"skill": {"id": "al-performance-review"}, "outcome": "completed", "findings": []} + root_report = {"skill": {"id": "al-code-review"}, "outcome": "completed", "findings": [], "sub-results": [leaf_report]} + for process_record, report in zip(payload["processes"], (leaf_report, root_report), strict=True): + path = output_dir / process_record["report_path"] + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(report), encoding="utf-8") + + def test_resolve_pr_review_root_requires_engine_path() -> None: with pytest.raises(AgentError, match="Pass --engine-path"): _resolve_pr_review_root(None) @@ -44,6 +105,22 @@ def test_prepare_bcquality_root_ignores_ambient_overrides(tmp_path: Path, monkey assert not any(name.startswith("BCQUALITY_") for name in child_env) +@pytest.mark.parametrize("value", ["", "latest", "1.0", "1.0.83; injected"]) +def test_pr_review_cli_version_requires_pinned_semver(value: str) -> None: + with pytest.raises(AgentError, match="COPILOT_REVIEW_CLI_VERSION must be"): + _resolve_pr_review_cli_version(value) + + +def test_pr_review_cli_version_uses_workflow_selected_pin() -> None: + assert _resolve_pr_review_cli_version("1.0.83") == "1.0.83" + + +def test_pr_review_cli_version_uses_installed_cli_when_no_workflow_pin(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("COPILOT_REVIEW_CLI_VERSION", raising=False) + with patch("bcbench.agent.pr_review.agent.get_copilot_version", return_value="1.0.83"): + assert _resolve_pr_review_cli_version() == "1.0.83" + + def test_valid_empty_findings_is_a_clean_review(tmp_path: Path) -> None: out, repo = _dirs(tmp_path) _write_output(out, json.dumps({"outcome": "completed", "outcome-reason": "", "findings": []})) @@ -116,6 +193,10 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: monkeypatch.chdir(tmp_path) monkeypatch.setenv("GITHUB_REPOSITORY", "microsoft/BC-Bench") monkeypatch.setenv("BCQUALITY_REF", "ambient-override") + monkeypatch.setenv("COPILOT_REVIEW_CLI_VERSION", "1.0.83") + monkeypatch.setenv("COPILOT_REVIEW_LEAF_MODEL", "gpt-5.4") + monkeypatch.setenv("COPILOT_REVIEW_LEAF_EXECUTION", "serial") + monkeypatch.setenv("COPILOT_REVIEW_MAX_LEAF_CONCURRENCY", "4") settings = {"min_severity": "Medium"} completed = subprocess.CompletedProcess(args=["pwsh"], returncode=0, stdout="✓", stderr="") entry = create_codereview_entry(repo="microsoft/BCApps") @@ -131,7 +212,7 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: { "schema_version": 1, "metrics_source": "copilot-cli-otel", - "cli_version": "1.0.81-0", + "cli_version": "1.0.83", "wall_time_seconds": 2.4, "prompt_tokens": 100, "cached_tokens": 20, @@ -144,13 +225,14 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: "usage_api_calls": 2, "ai_credits": 0.25, "premium_requests": 0.5, - "models": ["gpt-5.6-luna"], + "models": ["gpt-5.4", "gpt-5.6-luna"], "usage_complete": True, "malformed_records": 0, } ), encoding="utf-8", ) + _write_run_manifest(output_dir, root_model="gpt-5.6-luna", leaf_model="gpt-5.4") with ( patch("bcbench.agent.pr_review.agent._load_pr_review_settings", return_value=settings), @@ -169,7 +251,13 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: category=EvaluationCategory.CODE_REVIEW, repo_path=tmp_path / "repo", output_dir=Path("output"), + agent_version="e" * 40, engine_path=tmp_path / "engine", + cli_version="1.0.83", + leaf_model="gpt-5.4", + leaf_execution="serial", + max_leaf_concurrency=4, + cli_timeout_minutes=30, ) assert isinstance(metrics, PRReviewMetrics) @@ -179,7 +267,12 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: assert metrics.total_tokens == 110 assert metrics.ai_credits == 0.25 assert metrics.api_calls == 2 - assert metrics.copilot_cli_version == "1.0.81-0" + assert metrics.copilot_cli_version == "1.0.83" + assert metrics.leaf_model == "gpt-5.4" + assert metrics.leaf_execution == "serial" + assert metrics.max_leaf_concurrency == 4 + assert metrics.bcquality_commit == "b" * 40 + assert metrics.review_process_count == 2 assert config.is_empty() resolve_engine.assert_called_once_with(tmp_path / "engine") prepare_bcquality.assert_called_once_with(tmp_path / "engine", "pwsh", (tmp_path / "output" / "bcquality").resolve()) @@ -190,8 +283,90 @@ def test_engine_environment_uses_target_repository_and_absolute_paths(tmp_path: assert engine_env["REVIEW_OUTPUT_DIR"] == str((tmp_path / "output").resolve()) assert engine_env["REVIEW_WORKSPACE"] == str(tmp_path / "trusted") assert engine_env["BCQUALITY_ROOT"] == str(tmp_path / "bcquality") + assert "BCQUALITY_SHA" not in engine_env assert "BCQUALITY_REF" not in engine_env assert engine_env["GITHUB_REPOSITORY"] == "microsoft/BCApps" assert engine_env["AGENT_MINIMUM_SEVERITY"] == "Medium" + assert engine_env["COPILOT_REVIEW_CLI_VERSION"] == "1.0.83" + assert engine_env["COPILOT_REVIEW_LEAF_MODEL"] == "gpt-5.4" + assert engine_env["COPILOT_REVIEW_LEAF_EXECUTION"] == "serial" + assert engine_env["COPILOT_REVIEW_CLI_TIMEOUT_MINUTES"] == "30" assert run_process.call_args.args[0][-1].endswith("Invoke-CopilotPRReview.ps1") assert "-GenerateOnly" not in run_process.call_args.args[0] + + +def test_engine_configuration_uses_explicit_inputs_not_ambient_environment(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + _, repo = _dirs(tmp_path) + entry = create_codereview_entry() + settings = {"min_severity": "Medium"} + completed = subprocess.CompletedProcess(args=["pwsh"], returncode=0, stdout="", stderr="") + engine_root = tmp_path / "engine" + bcquality_root = tmp_path / "bcquality" + knowledge_root = bcquality_root / "knowledge" + knowledge_root.mkdir(parents=True) + (knowledge_root / "one.md").write_text("# One", encoding="utf-8") + (bcquality_root / "_filter-report.json").write_text('{"removed": []}', encoding="utf-8") + output_dir = tmp_path / "output" + output_dir.mkdir() + (output_dir / "_run-metrics.json").write_text( + json.dumps( + { + "schema_version": 1, + "metrics_source": "copilot-cli-otel", + "cli_version": "1.0.83", + "wall_time_seconds": 1.0, + "prompt_tokens": 1, + "cached_tokens": 0, + "cache_creation_tokens": 0, + "completion_tokens": 1, + "reasoning_tokens": 0, + "total_tokens": 2, + "api_calls": 1, + "failed_api_calls": 0, + "usage_api_calls": 1, + "ai_credits": 0.01, + "premium_requests": 0.0, + "models": ["gpt-5.6-sol", "gpt-5.6-luna"], + "usage_complete": True, + "malformed_records": 0, + } + ), + encoding="utf-8", + ) + _write_run_manifest(output_dir, root_model="gpt-5.6-sol", leaf_model="gpt-5.6-luna") + monkeypatch.setenv("COPILOT_REVIEW_LEAF_MODEL", "ambient-leaf") + monkeypatch.setenv("COPILOT_REVIEW_LEAF_EXECUTION", "parallel") + monkeypatch.setenv("COPILOT_REVIEW_MAX_LEAF_CONCURRENCY", "99") + monkeypatch.setenv("COPILOT_REVIEW_CLI_TIMEOUT_MINUTES", "99") + + with ( + patch("bcbench.agent.pr_review.agent._load_pr_review_settings", return_value=settings), + patch("bcbench.agent.pr_review.agent._resolve_pr_review_root", return_value=engine_root), + patch("bcbench.agent.pr_review.agent._resolve_pwsh", return_value="pwsh"), + patch("bcbench.agent.pr_review.agent._commit_patch_as_head"), + patch("bcbench.agent.pr_review.agent._init_trusted_workspace", return_value=tmp_path / "trusted"), + patch("bcbench.agent.pr_review.agent._prepare_bcquality_root", return_value=bcquality_root), + patch("bcbench.agent.pr_review.agent._write_review_json", return_value=0), + patch("bcbench.agent.pr_review.agent.time.monotonic", side_effect=[1.0, 2.0]), + patch("bcbench.agent.pr_review.agent.subprocess.run", return_value=completed) as run_process, + ): + run_pr_review_agent( + entry=entry, + model="gpt-5.6-sol", + category=EvaluationCategory.CODE_REVIEW, + repo_path=repo, + output_dir=output_dir, + agent_version="e" * 40, + engine_path=engine_root, + cli_version="1.0.83", + leaf_model="gpt-5.6-luna", + leaf_execution="serial", + max_leaf_concurrency=4, + cli_timeout_minutes=30, + ) + + engine_env = run_process.call_args.kwargs["env"] + assert engine_env["COPILOT_REVIEW_LEAF_MODEL"] == "gpt-5.6-luna" + assert engine_env["COPILOT_REVIEW_LEAF_EXECUTION"] == "serial" + assert engine_env["COPILOT_REVIEW_MAX_LEAF_CONCURRENCY"] == "4" + assert engine_env["COPILOT_REVIEW_CLI_TIMEOUT_MINUTES"] == "30" diff --git a/tests/test_pr_review_metrics.py b/tests/test_pr_review_metrics.py index cfa6c75a4..d046d8743 100644 --- a/tests/test_pr_review_metrics.py +++ b/tests/test_pr_review_metrics.py @@ -1,10 +1,13 @@ import json from pathlib import Path +from types import SimpleNamespace +from typing import cast from unittest.mock import patch import pytest from bcbench.agent.pr_review.metrics import RUN_METRICS_FILE_NAME, build_pr_review_metrics +from bcbench.agent.pr_review.run_manifest import RunManifest from bcbench.dataset.codereview import CodeReviewEntry from bcbench.exceptions import AgentError from bcbench.results.bceval_export import write_bceval_results @@ -49,15 +52,16 @@ def test_build_metrics_promotes_public_performance_metrics(tmp_path: Path) -> No assert metrics.kind == "pr-review" assert metrics.execution_time == 12.5 assert metrics.prompt_tokens == 150 - assert metrics.completion_tokens == 28 - assert metrics.total_tokens == 178 - assert metrics.ai_credits == 1.75 assert metrics.cached_tokens == 60 assert metrics.cache_creation_tokens == 10 + assert metrics.completion_tokens == 28 assert metrics.reasoning_tokens == 7 + assert metrics.total_tokens == 178 assert metrics.api_calls == 2 assert metrics.failed_api_calls == 1 assert metrics.usage_api_calls == 2 + assert metrics.ai_credits == 1.75 + assert metrics.models == ["gpt-5.4-mini", "gpt-5.6-sol"] assert metrics.usage_complete is True assert metrics.malformed_records == 0 assert metrics.copilot_cli_version == "1.0.81-0" @@ -82,6 +86,43 @@ def test_legal_null_optional_fields_and_multiple_models_are_accepted(tmp_path: P assert metrics.total_tokens == 178 +def test_cli_1088_null_otel_version_is_preserved_with_startup_provenance(tmp_path: Path) -> None: + from tests.test_pr_review_run_manifest import valid_manifest + + _write_run_metrics(tmp_path, cli_version=None) + payload = valid_manifest() + payload["configuration"].update(copilot_cli_version="1.0.88", root_model="gpt-5.6-sol", leaf_model="gpt-5.4-mini") + manifest = RunManifest.model_validate(payload) + + metrics = build_pr_review_metrics(tmp_path, execution_time=2.0, manifest=manifest) + + assert metrics.copilot_cli_version is None + assert manifest.configuration.copilot_cli_version == "1.0.88" + assert metrics.usage_complete is True + + +@pytest.mark.parametrize( + "overrides", + [ + {"cli_version": "1.0.82"}, + {"usage_complete": False}, + {"malformed_records": 1}, + {"models": ["gpt-5.4-mini", "unexpected-model"]}, + ], +) +def test_validated_manifest_rejects_mismatched_aggregate_metrics(tmp_path: Path, overrides: dict[str, object]) -> None: + _write_run_metrics(tmp_path, **overrides) + configuration = SimpleNamespace( + copilot_cli_version="1.0.81-0", + root_model="gpt-5.6-sol", + leaf_model="gpt-5.4-mini", + ) + manifest = cast(RunManifest, SimpleNamespace(configuration=configuration)) + + with pytest.raises(AgentError, match="aggregate metrics"): + build_pr_review_metrics(tmp_path, execution_time=2.0, manifest=manifest) + + @pytest.mark.parametrize("has_token_usage", [False, True], ids=["no-chat-spans", "chat-without-billing"]) def test_valid_engine_metrics_without_billing_preserve_unknown_credits_through_export(tmp_path: Path, has_token_usage: bool) -> None: # Get-CopilotRunMetrics at engine 159572aad814d6e1022ca8d54e85a6eab7c4d5c6 emits these nullable shapes. @@ -145,9 +186,11 @@ def test_malformed_records_suppress_all_usage_metrics(tmp_path: Path) -> None: assert metrics.completion_tokens is None assert metrics.total_tokens is None assert metrics.ai_credits is None + assert metrics.usage_complete is False + assert metrics.malformed_records == 3 -def test_incomplete_usage_suppresses_tokens_but_preserves_exact_credits(tmp_path: Path) -> None: +def test_incomplete_usage_preserves_observed_values_and_completeness_flag(tmp_path: Path) -> None: _write_run_metrics( tmp_path, prompt_tokens=25, @@ -161,10 +204,12 @@ def test_incomplete_usage_suppresses_tokens_but_preserves_exact_credits(tmp_path metrics = build_pr_review_metrics(tmp_path, execution_time=2.0) - assert metrics.prompt_tokens is None - assert metrics.completion_tokens is None - assert metrics.total_tokens is None + assert metrics.prompt_tokens == 25 + assert metrics.completion_tokens == 5 + assert metrics.total_tokens == 30 assert metrics.ai_credits == 0.1 + assert metrics.usage_complete is False + assert metrics.malformed_records == 0 def test_missing_run_metrics_raises(tmp_path: Path) -> None: diff --git a/tests/test_pr_review_metrics_reporting.py b/tests/test_pr_review_metrics_reporting.py index 738f72e95..ef462ae13 100644 --- a/tests/test_pr_review_metrics_reporting.py +++ b/tests/test_pr_review_metrics_reporting.py @@ -2,7 +2,7 @@ from bcbench.results.codereview import CodeReviewResultSummary from bcbench.results.leaderboard import CodeReviewLeaderboardAggregate -from bcbench.types import AgentMetrics +from bcbench.types import AgentHarness, AgentMetrics, PRReviewMetrics from tests.conftest import create_codereview_result @@ -84,8 +84,25 @@ def test_github_summary_renders_only_public_performance_metrics() -> None: assert diagnostic not in markdown -def test_result_json_excludes_raw_only_diagnostics(tmp_path) -> None: - result = create_codereview_result(metrics=_metrics(duration=4.0, scale=1)) +def test_result_json_preserves_raw_diagnostics(tmp_path) -> None: + metrics = PRReviewMetrics( + execution_time=4.0, + prompt_tokens=900, + cached_tokens=700, + cache_creation_tokens=50, + completion_tokens=100, + reasoning_tokens=40, + total_tokens=1000, + api_calls=5, + failed_api_calls=0, + usage_api_calls=4, + ai_credits=0.5, + models=["claude-sonnet-5", "gpt-5.4"], + copilot_cli_version="1.0.83", + usage_complete=False, + malformed_records=0, + ) + result = create_codereview_result(agent_name=AgentHarness.PR_REVIEW, metrics=metrics) result.save(tmp_path, "results.jsonl") saved_metrics = json.loads((tmp_path / "results.jsonl").read_text(encoding="utf-8"))["metrics"] @@ -94,17 +111,16 @@ def test_result_json_excludes_raw_only_diagnostics(tmp_path) -> None: assert saved_metrics["completion_tokens"] == 100 assert saved_metrics["total_tokens"] == 1000 assert saved_metrics["ai_credits"] == 0.5 - for diagnostic in ( - "cached_tokens", - "cache_creation_tokens", - "reasoning_tokens", - "failed_api_calls", - "usage_api_calls", - "premium_requests", - "usage_complete", - "malformed_records", - ): - assert diagnostic not in saved_metrics + assert saved_metrics["cached_tokens"] == 700 + assert saved_metrics["cache_creation_tokens"] == 50 + assert saved_metrics["reasoning_tokens"] == 40 + assert saved_metrics["api_calls"] == 5 + assert saved_metrics["failed_api_calls"] == 0 + assert saved_metrics["usage_api_calls"] == 4 + assert saved_metrics["models"] == ["claude-sonnet-5", "gpt-5.4"] + assert saved_metrics["copilot_cli_version"] == "1.0.83" + assert saved_metrics["usage_complete"] is False + assert saved_metrics["malformed_records"] == 0 def test_summary_and_leaderboard_schemas_exclude_raw_only_diagnostics() -> None: diff --git a/tests/test_pr_review_run_manifest.py b/tests/test_pr_review_run_manifest.py new file mode 100644 index 000000000..7f33b3ff2 --- /dev/null +++ b/tests/test_pr_review_run_manifest.py @@ -0,0 +1,246 @@ +import json +from copy import deepcopy +from pathlib import Path + +import pytest + +from bcbench.agent.pr_review.run_manifest import load_run_manifest, validate_review_reports, validate_run_manifest +from bcbench.exceptions import AgentError + +ENGINE_COMMIT = "e" * 40 +BCQUALITY_COMMIT = "b" * 40 +CLI_VERSION = "1.0.83" + + +def _metrics(model: str) -> dict: + return { + "cli_version": CLI_VERSION, + "models": [model], + "usage_complete": True, + "malformed_records": 0, + "total_tokens": 10, + } + + +def _process(role: str, ordinal: int, skill_id: str, model: str) -> dict: + return { + "role": role, + "ordinal": ordinal, + "skill_id": skill_id, + "requested_model": model, + "observed_models": [model], + "status": "completed", + "started_at": "2026-09-14T12:00:00.0000000Z", + "completed_at": "2026-09-14T12:00:01.0000000Z", + "duration_seconds": 1.0, + "exit_code": 0, + "report_path": f"{role}-results/{ordinal}/_review-report.json", + "failure_reason": None, + "metrics": _metrics(model), + } + + +def valid_manifest() -> dict: + return { + "schema_version": 1, + "status": "completed", + "started_at": "2026-09-14T12:00:00.0000000Z", + "completed_at": "2026-09-14T12:00:03.0000000Z", + "failure_reason": None, + "engine": { + "repository": "microsoft/BC-ALAgents", + "commit": ENGINE_COMMIT, + "agent_version": "1.6.6", + }, + "bcquality": { + "commit": BCQUALITY_COMMIT, + "source_snapshot": "a" * 64, + }, + "configuration": { + "copilot_cli_version": CLI_VERSION, + "root_model": "claude-sonnet-5", + "leaf_model": "gpt-5.4", + "leaf_execution": "serial", + "max_leaf_concurrency": 4, + "cli_timeout_minutes": 30, + "minimum_severity": "Medium", + "agent_minimum_severity": "Medium", + "review_source": "local", + }, + "plan": { + "skill_id": "al-code-review", + "leaf_count": 2, + "leaf_ids": ["al-performance-review", "al-security-review"], + }, + "processes": [ + _process("leaf", 1, "al-performance-review", "gpt-5.4"), + _process("leaf", 2, "al-security-review", "gpt-5.4"), + _process("root", 3, "al-code-review", "claude-sonnet-5"), + ], + } + + +def _load(tmp_path: Path, payload: dict): + path = tmp_path / "_run-manifest.json" + path.write_text(json.dumps(payload), encoding="utf-8") + return load_run_manifest(path) + + +def _validate(manifest) -> None: + validate_run_manifest( + manifest, + engine_commit=ENGINE_COMMIT, + cli_version=CLI_VERSION, + root_model="claude-sonnet-5", + leaf_model="gpt-5.4", + leaf_execution="serial", + max_leaf_concurrency=4, + cli_timeout_minutes=30, + minimum_severity="Medium", + agent_minimum_severity="Medium", + ) + + +def test_accepts_exact_pinned_runtime(tmp_path: Path) -> None: + _validate(_load(tmp_path, valid_manifest())) + + +def test_accepts_bcquality_revision_resolved_by_pinned_engine(tmp_path: Path) -> None: + payload = valid_manifest() + payload["bcquality"]["commit"] = "f" * 40 + + _validate(_load(tmp_path, payload)) + + +@pytest.mark.parametrize( + ("mutation", "message"), + [ + (lambda data: data.update(status="partial"), "status='partial'"), + (lambda data: data["engine"].update(commit="f" * 40), "engine.commit"), + (lambda data: data["bcquality"].update(commit=None), "bcquality.commit is missing"), + (lambda data: data["configuration"].update(leaf_model="gpt-5.6-luna"), "leaf_model"), + (lambda data: data["processes"][0].update(observed_models=["gemini-3.6-flash"]), "model telemetry"), + (lambda data: data["processes"][0]["metrics"].update(usage_complete=False), "process metrics"), + (lambda data: data["configuration"].update(cli_timeout_minutes=45), "cli_timeout_minutes"), + (lambda data: data["configuration"].update(minimum_severity="High"), "minimum_severity"), + (lambda data: data["configuration"].update(agent_minimum_severity="High"), "agent_minimum_severity"), + (lambda data: data["processes"].reverse(), "process ordinals"), + ], +) +def test_rejects_contaminated_or_incomplete_runtime(tmp_path: Path, mutation, message: str) -> None: + payload = deepcopy(valid_manifest()) + mutation(payload) + + with pytest.raises(AgentError, match=message): + _validate(_load(tmp_path, payload)) + + +@pytest.mark.parametrize( + ("mutation", "message"), + [ + (lambda data: data.update(completed_at=None), "completed manifest requires completed_at"), + (lambda data: data.update(failure_reason="unexpected"), "completed manifest must not have failure_reason"), + (lambda data: data.update(status="failed"), "failed manifest requires failure_reason"), + (lambda data: data.update(status="failed", failure_reason="leaf failed", completed_at=None), "failed manifest requires completed_at"), + (lambda data: data.update(status="running"), "running manifest must not have completed_at"), + ], +) +def test_rejects_inconsistent_manifest_lifecycle(tmp_path: Path, mutation, message: str) -> None: + payload = valid_manifest() + mutation(payload) + + with pytest.raises(AgentError, match=message): + _load(tmp_path, payload) + + +def test_rejects_missing_process_cli_version(tmp_path: Path) -> None: + payload = valid_manifest() + payload["processes"][0]["metrics"]["cli_version"] = None + + with pytest.raises(AgentError, match="process metrics"): + _validate(_load(tmp_path, payload)) + + +@pytest.mark.parametrize(("observed", "valid"), [(None, True), ("1.0.88", True), ("1.0.83", False), ("", False)]) +def test_cli_1088_allows_only_documented_missing_otel_version(tmp_path: Path, observed, valid): + payload = valid_manifest() + payload["configuration"]["copilot_cli_version"] = "1.0.88" + for process in payload["processes"]: + process["metrics"]["cli_version"] = observed + manifest = _load(tmp_path, payload) + kwargs = { + "engine_commit": ENGINE_COMMIT, + "cli_version": "1.0.88", + "root_model": "claude-sonnet-5", + "leaf_model": "gpt-5.4", + "leaf_execution": "serial", + "max_leaf_concurrency": 4, + "cli_timeout_minutes": 30, + "minimum_severity": "Medium", + "agent_minimum_severity": "Medium", + } + if valid: + validate_run_manifest(manifest, **kwargs) + assert manifest.processes[0].metrics.cli_version == observed + else: + with pytest.raises(AgentError, match="process metrics"): + validate_run_manifest(manifest, **kwargs) + + +def _write_reports(tmp_path, payload): + schema_path = tmp_path / "bcquality" / "schemas" / "findings-report.schema.json" + schema_path.parent.mkdir(parents=True) + schema_path.write_text(json.dumps({"type": "object", "required": ["skill", "outcome", "findings"]}), encoding="utf-8") + leaves = [{"skill": {"id": leaf_id}, "outcome": "completed", "findings": []} for leaf_id in payload["plan"]["leaf_ids"]] + reports = [*leaves, {"skill": {"id": "al-code-review"}, "outcome": "completed", "findings": [], "sub-results": deepcopy(leaves)}] + for process, report in zip(payload["processes"], reports, strict=True): + path = tmp_path / process["report_path"] + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(json.dumps(report), encoding="utf-8") + return reports + + +@pytest.mark.parametrize("outcome", ["completed", "not-applicable", "no-knowledge"]) +def test_accepts_consistent_original_reports(tmp_path, outcome): + payload = valid_manifest() + reports = _write_reports(tmp_path, payload) + reports[0]["outcome"] = outcome + reports[-1]["sub-results"][0]["outcome"] = outcome + for process, report in zip(payload["processes"], reports, strict=True): + (tmp_path / process["report_path"]).write_text(json.dumps(report), encoding="utf-8") + validate_review_reports(_load(tmp_path, payload), tmp_path) + + +@pytest.mark.parametrize( + ("report_index", "mutation", "message"), + [ + (0, lambda report: report.update(outcome="failed"), "unusable outcome"), + (0, lambda report: report.update(outcome="partial"), "unusable outcome"), + (0, lambda report: report.update(**{"sub-results": []}), "root-only"), + (0, lambda report: report["skill"].update(id="other"), "wrong skill"), + (0, lambda report: report.pop("findings"), "pinned BCQuality schema"), + (0, lambda report: report.update(outcome="no-knowledge", findings=[{}]), "inactive leaf"), + (2, lambda report: report.update(outcome="partial"), "unusable outcome"), + (2, lambda report: report["sub-results"].pop(), "ordered leaf plan"), + (2, lambda report: report["sub-results"].reverse(), "ordered leaf plan"), + (2, lambda report: report["sub-results"][0].update(outcome="failed"), "disagrees"), + (2, lambda report: report["sub-results"][0].update(**{"sub-results": [{"outcome": "failed"}]}), "nested delegation"), + (2, lambda report: report.update(**{"skipped-sub-skills": ["al-security-review"]}), "skips planned"), + ], +) +def test_completed_manifest_does_not_hide_original_report_failure(tmp_path, report_index, mutation, message): + payload = valid_manifest() + reports = _write_reports(tmp_path, payload) + mutation(reports[report_index]) + (tmp_path / payload["processes"][report_index]["report_path"]).write_text(json.dumps(reports[report_index]), encoding="utf-8") + with pytest.raises(AgentError, match=message): + validate_review_reports(_load(tmp_path, payload), tmp_path) + + +@pytest.mark.parametrize("path", [None, "../outside.json", "missing.json"]) +def test_original_reports_are_required_inside_output_directory(tmp_path, path): + payload = valid_manifest() + _write_reports(tmp_path, payload) + payload["processes"][0]["report_path"] = path + with pytest.raises(AgentError): + validate_review_reports(_load(tmp_path, payload), tmp_path) diff --git a/tests/test_review_runners.py b/tests/test_review_runners.py index 9f2bba7f2..4d4f3eee0 100644 --- a/tests/test_review_runners.py +++ b/tests/test_review_runners.py @@ -93,6 +93,7 @@ def test_pr_review_evaluation_is_fixed_to_runner_and_category(tmp_path: Path) -> assert contexts[0].category is EvaluationCategory.CODE_REVIEW assert contexts[0].model == "gpt-5.6-luna" assert agent_runner.call_args.kwargs["engine_path"] == tmp_path + assert agent_runner.call_args.kwargs["agent_version"] == "a" * 40 get_version.assert_called_once_with(tmp_path) @@ -102,6 +103,7 @@ def test_pr_review_run_is_fixed_to_code_review(tmp_path: Path) -> None: patch.object(CodeReviewEntry, "load", return_value=[entry]), patch.object(CodeReviewPipeline, "setup_workspace"), patch.object(run_commands, "run_pr_review_agent") as agent_runner, + patch.object(run_commands, "get_pr_review_version", return_value="a" * 40) as get_version, ): result = CliRunner().invoke( app, @@ -123,6 +125,8 @@ def test_pr_review_run_is_fixed_to_code_review(tmp_path: Path) -> None: assert agent_runner.call_args.kwargs["category"] is EvaluationCategory.CODE_REVIEW assert agent_runner.call_args.kwargs["model"] == "gpt-5.6-luna" assert agent_runner.call_args.kwargs["engine_path"] == tmp_path + assert agent_runner.call_args.kwargs["agent_version"] == "a" * 40 + get_version.assert_called_once_with(tmp_path, require_clean=False) def test_pr_review_is_public_command() -> None: diff --git a/tests/test_review_workflows.py b/tests/test_review_workflows.py index 3f58a14a0..65f5c5879 100644 --- a/tests/test_review_workflows.py +++ b/tests/test_review_workflows.py @@ -20,6 +20,16 @@ def _workflow(name: str) -> str: return text +def _workflow_dispatch_inputs(workflow: dict) -> dict: + trigger = workflow.get("on", workflow.get(True)) + assert isinstance(trigger, dict) + dispatch = trigger.get("workflow_dispatch") + assert isinstance(dispatch, dict) + inputs = dispatch.get("inputs") + assert isinstance(inputs, dict) + return inputs + + def test_copilot_workflow_routes_code_review_through_copilot() -> None: workflow = _workflow("copilot-evaluation.yml") @@ -40,6 +50,9 @@ def test_claude_workflow_routes_code_review_through_claude() -> None: def test_pr_review_workflow_is_fixed_to_code_review() -> None: workflow = _workflow("pr-review-evaluation.yml") + workflow_data = yaml.safe_load(workflow) + inputs = _workflow_dispatch_inputs(workflow_data) + install = next(step for step in workflow_data["jobs"]["evaluate-with-pr-review"]["steps"] if step.get("id") == "install-harnesses") config = yaml.safe_load(AGENT_CONFIG.read_text(encoding="utf-8")) assert "category: code-review" in workflow @@ -54,15 +67,57 @@ def test_pr_review_workflow_is_fixed_to_code_review() -> None: assert 'agent: "BC PR Review"' in workflow assert '"mai-code-1.1-flash"' in workflow assert "mai-code-1-flash-picker" not in workflow - assert '"gemini-3.7-flash"' in workflow - assert "gemini-3.6-flash" not in workflow - for input_name in ("model:", "engine-sha:", "test-run:", "repeat:", "git-ref:", "modified-only:"): + assert "claude-" not in workflow + assert "gemini-" not in workflow + assert inputs["model"]["default"] == "gpt-5.6-sol" + assert inputs["model"]["options"] == [ + "gpt-5.4", + "gpt-5.6-sol", + "gpt-5.6-terra", + "gpt-5.6-luna", + "gpt-5.3-codex", + "mai-code-1.1-flash", + ] + assert inputs["leaf-model"]["default"] == "gpt-5.6-luna" + assert inputs["leaf-model"]["options"] == ["gpt-5.4", "gpt-5.6-luna", "mai-code-1.1-flash"] + assert inputs["leaf-execution"] == { + "description": "Deterministic leaf scheduling mode", + "required": False, + "default": "serial", + "type": "choice", + "options": ["serial", "parallel"], + } + assert inputs["max-leaf-concurrency"] == { + "description": "Maximum simultaneous leaves in parallel mode", + "required": False, + "default": "4", + "type": "string", + } + assert "COPILOT_REVIEW_CLI_VERSION: ${{ steps.install-harnesses.outputs.copilot-version }}" in workflow + assert install["with"]["copilot-version"] == "1.0.88" + assert "COPILOT_REVIEW_LEAF_MODEL: ${{ inputs.leaf-model }}" in workflow + assert "COPILOT_REVIEW_LEAF_EXECUTION: ${{ inputs.leaf-execution }}" in workflow + assert "COPILOT_REVIEW_MAX_LEAF_CONCURRENCY: ${{ inputs.max-leaf-concurrency }}" in workflow + assert "group: pr-review-evaluation-${{ inputs.modified-only && 'modified' || inputs.test-run && 'test' || 'full' }}" in workflow + assert "repetition-id" not in workflow + for input_name in ( + "model:", + "leaf-model:", + "leaf-execution:", + "max-leaf-concurrency:", + "engine-sha:", + "test-run:", + "modified-only:", + "repeat:", + "entries:", + "git-ref:", + ): assert input_name in workflow def test_pr_review_workflow_passes_optional_engine_sha_to_harness_action() -> None: workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) - engine_input = workflow[True]["workflow_dispatch"]["inputs"]["engine-sha"] + engine_input = _workflow_dispatch_inputs(workflow)["engine-sha"] install = next(step for step in workflow["jobs"]["evaluate-with-pr-review"]["steps"] if step.get("id") == "install-harnesses") assert engine_input["required"] is False @@ -77,7 +132,7 @@ def test_engine_sha_override_is_never_published_as_a_benchmark_result() -> None: workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) summarize = workflow["jobs"]["summarize-results"]["with"] - assert summarize["mock"] == "${{ inputs.test-run || inputs.modified-only || inputs.engine-sha != '' }}" + assert summarize["mock"] == "${{ inputs.test-run || inputs.modified-only || inputs.engine-sha != '' || inputs.entries != '' }}" # Repeats stay available so an override can be measured over several runs. assert "inputs.engine-sha" not in workflow["jobs"]["requeue"]["if"] @@ -86,10 +141,13 @@ def test_engine_sha_override_is_never_published_as_a_benchmark_result() -> None: def test_pr_review_requeue_preserves_engine_sha(engine_sha: str) -> None: workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) payload = workflow["jobs"]["requeue"]["with"]["workflow-inputs"] - expression = "${{ toJSON(inputs.engine-sha) }}" + engine_expression = "${{ toJSON(inputs.engine-sha) }}" + entries_expression = "${{ toJSON(inputs.entries) }}" - assert expression in payload - assert json.loads(payload.replace(expression, json.dumps(engine_sha)))["engine-sha"] == engine_sha + assert engine_expression in payload + assert entries_expression in payload + parsed = json.loads(payload.replace(engine_expression, json.dumps(engine_sha)).replace(entries_expression, json.dumps(""))) + assert parsed["engine-sha"] == engine_sha def test_requeue_workflow_reads_inputs_from_environment() -> None: @@ -126,9 +184,57 @@ def test_pr_review_workflow_treats_modified_only_as_a_partial_run() -> None: def test_agent_harness_action_pins_published_copilot_version() -> None: - action = (ACTIONS / "install-agent-harnesses" / "action.yml").read_text(encoding="utf-8") + action = yaml.safe_load((ACTIONS / "install-agent-harnesses" / "action.yml").read_text(encoding="utf-8")) + copilot_validation = next(step for step in action["runs"]["steps"] if step.get("id") == "copilot-version") + install = next(step for step in action["runs"]["steps"] if step["name"] == "Install GitHub Copilot CLI") + + claude_install = next(step for step in action["runs"]["steps"] if step["name"] == "Install Claude Code") + + assert action["inputs"]["copilot-version"] == { + "description": "GitHub Copilot CLI version (defaults to 1.0.82)", + "required": False, + "default": "1.0.82", + } + assert copilot_validation["env"]["COPILOT_VERSION"] == "${{ inputs.copilot-version }}" + assert "${{" not in copilot_validation["run"] + assert install["run"] == "npm install -g @github/copilot@${{ steps.copilot-version.outputs.version }}" + assert claude_install["run"] == "npm install -g @anthropic-ai/claude-code@2.1.221" + + +@pytest.mark.skipif(PWSH is None, reason="PowerShell is required to test the composite action script") +@pytest.mark.parametrize( + ("copilot_version", "valid"), + [ + ("1.0.83", True), + ("1.0.88", True), + ("1.0.89-1", True), + ("", False), + ("latest", False), + ("1.0", False), + ("1.0.88; Write-Output injected", False), + ], +) +def test_agent_harness_action_validates_copilot_version(copilot_version: str, valid: bool, tmp_path: Path) -> None: + assert PWSH is not None + action = yaml.safe_load((ACTIONS / "install-agent-harnesses" / "action.yml").read_text(encoding="utf-8")) + validation = next(step for step in action["runs"]["steps"] if step.get("id") == "copilot-version") + output = tmp_path / "github-output" - assert "@github/copilot@1.0.82" in action + result = subprocess.run( + [PWSH, "-NoProfile", "-NonInteractive", "-Command", validation["run"]], + env={**os.environ, "COPILOT_VERSION": copilot_version, "GITHUB_OUTPUT": str(output)}, + capture_output=True, + text=True, + check=False, + ) + + if valid: + assert result.returncode == 0, result.stderr + assert output.read_text(encoding="utf-8") == f"version={copilot_version}\n" + else: + assert result.returncode != 0 + assert "copilot-version must be a pinned semantic version." in result.stderr + assert not output.exists() def test_agent_harness_action_pins_and_exports_bc_alagents() -> None: @@ -147,7 +253,8 @@ def test_agent_harness_action_pins_and_exports_bc_alagents() -> None: assert config["runs"]["steps"].index(validation) < config["runs"]["steps"].index(checkout) assert checkout["with"]["ref"] == "${{ steps.engine-sha.outputs.sha }}" assert checkout["with"]["persist-credentials"] is False - assert set(config["outputs"]) == {"bc-alagents-path"} + assert config["outputs"]["copilot-version"]["value"] == "${{ steps.copilot-version.outputs.version }}" + assert set(config["outputs"]) == {"bc-alagents-path", "copilot-version"} @pytest.mark.skipif(PWSH is None, reason="PowerShell is required to test the composite action script") @@ -203,3 +310,56 @@ def test_agent_workflows_select_al_tool_dotnet_version_for_bc_version() -> None: for workflow_name in ("claude-evaluation.yml", "copilot-evaluation.yml"): workflow = _workflow(workflow_name) assert '--framework "net${{ steps.setup-env.outputs.al_tool_dotnet_version }}"' in workflow + + +def test_focused_http_experiment_has_exact_matrix_and_no_publish_route() -> None: + from bcbench.dataset.codereview import CodeReviewEntry + from bcbench.types import EvaluationCategory + + entries = [ + "synthetic__privacy-008", + "synthetic__privacy-010", + "synthetic__privacy-015", + "synthetic__security-clean-02", + "synthetic__errh-tryfunction-swallowed-01", + "synthetic__http-optional-clean-01", + "synthetic__http-consumed-false-01", + "synthetic__http-status-body-01", + ] + dispatch_entries = json.dumps(entries) + dataset = {entry.instance_id: entry for entry in CodeReviewEntry.load(EvaluationCategory.CODE_REVIEW.dataset_path)} + matrix = json.loads(dispatch_entries) + assert matrix == entries + assert len(matrix) == len(set(matrix)) == 8 + assert sum(bool(dataset[entry_id].expected_comments or dataset[entry_id].ignored_comments) for entry_id in matrix) == 6 + assert sum(len(dataset[entry_id].expected_comments) for entry_id in matrix) == 10 + workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) + jobs = workflow["jobs"] + assert jobs["evaluate-with-pr-review"]["strategy"]["matrix"]["entry"] == "${{ fromJson(inputs.entries != '' && inputs.entries || needs.get-entries.outputs.entries) }}" + assert jobs["summarize-results"]["with"]["mock"] == "${{ inputs.test-run || inputs.modified-only || inputs.engine-sha != '' || inputs.entries != '' }}" + assert jobs["pin-commit"]["with"]["test-run"] == "${{ inputs.test-run || inputs.modified-only }}" + assert "!inputs.test-run && !inputs.modified-only" in jobs["requeue"]["if"] + pin = yaml.safe_load(_workflow("pin-evaluation-commit.yml")) + assert "!inputs.test-run && inputs.repeat != '1'" in pin["jobs"]["pin-commit"]["if"] + summary = _workflow("summarize-results.yml") + assert "--use-capi ${{ !inputs.mock && '--storage braintrust --storage kusto' || '' }}" in summary + assert "if: ${{ !inputs.mock && !inputs.skip-leaderboard }}" in summary + generation = next(step for step in jobs["evaluate-with-pr-review"]["steps"] if step.get("name", "").startswith("Run BC PR Review")) + assert "bcbench evaluate pr-review" in generation["run"] + assert "--mock" not in generation["run"] + assert jobs["evaluate-with-pr-review"]["strategy"]["max-parallel"] == 64 + assert "repetition-id" not in _workflow("pr-review-evaluation.yml") + + +def test_pr_review_diagnostics_are_allowlisted_without_raw_otel_or_checkout_upload() -> None: + workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) + steps = workflow["jobs"]["evaluate-with-pr-review"]["steps"] + upload = next(step for step in steps if step["name"] == "Upload allowlisted review diagnostics") + assert upload["if"] == "always()" + paths = upload["with"]["path"].splitlines() + assert len(paths) == 12 + assert all(not path.endswith(("**", "**/*", "**/*.json", "**/*.log")) for path in paths) + for filename in ("_run-manifest.json", "_run-metrics.json", "_review-report.json", "al-code-review-findings.json", "judge_results.json", "review.json"): + assert any(path.endswith("/" + filename) for path in paths) + assert "_copilot-otel" not in upload["with"]["path"] + assert upload["with"]["retention-days"] == 7 diff --git a/uv.lock b/uv.lock index 9fb8f8d47..417da150d 100644 --- a/uv.lock +++ b/uv.lock @@ -245,7 +245,7 @@ wheels = [ [[package]] name = "bcbench" -version = "0.13.0" +version = "0.14.0.dev1" source = { editable = "." } dependencies = [ { name = "jinja2" }, From b67844aea3834ecc4e63698bf76cb1190da24d3b Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 1 Oct 2026 10:25:07 +0200 Subject: [PATCH 2/5] Pin HTTP A/B baseline to the pre-196 knowledge snapshot Declare engine da9236a6f43d47c37e0bd59ebced33b6fe73f277, which owns BCQuality 130d5de6c4bd72d2158240fe431862b6434c735a. Change only the engine pin and its test constant; shared support and version remain identical. No paid execution is authorized. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/actions/install-agent-harnesses/action.yml | 2 +- tests/test_review_workflows.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/actions/install-agent-harnesses/action.yml b/.github/actions/install-agent-harnesses/action.yml index 877faacfc..c31d48fe9 100644 --- a/.github/actions/install-agent-harnesses/action.yml +++ b/.github/actions/install-agent-harnesses/action.yml @@ -31,7 +31,7 @@ runs: id: engine-sha shell: pwsh env: - ENGINE_SHA: ${{ inputs.engine-sha || 'ecf8e31759d6ddd6d78e3a0b7836b40134368009' }} + ENGINE_SHA: ${{ inputs.engine-sha || 'da9236a6f43d47c37e0bd59ebced33b6fe73f277' }} run: | if ($env:ENGINE_SHA -notmatch '\A[0-9a-fA-F]{40}\z') { throw "engine-sha must be a full 40-character hexadecimal commit SHA." diff --git a/tests/test_review_workflows.py b/tests/test_review_workflows.py index 65f5c5879..3c6005b68 100644 --- a/tests/test_review_workflows.py +++ b/tests/test_review_workflows.py @@ -10,7 +10,7 @@ WORKFLOWS = Path(__file__).parents[1] / ".github" / "workflows" ACTIONS = Path(__file__).parents[1] / ".github" / "actions" AGENT_CONFIG = Path(__file__).parents[1] / "src" / "bcbench" / "agent" / "shared" / "config.yaml" -DEFAULT_ENGINE_SHA = "ecf8e31759d6ddd6d78e3a0b7836b40134368009" +DEFAULT_ENGINE_SHA = "da9236a6f43d47c37e0bd59ebced33b6fe73f277" PWSH = shutil.which("pwsh") From 9f6d25cff24d84ea95aa1b4a5d78abf6160afda5 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 1 Oct 2026 10:29:45 +0200 Subject: [PATCH 3/5] Check fixture comments without rewriting URL text Avoid a URL-sanitization-shaped assertion in the experimental fixture test. The check only inspects trusted AL fixture source; fixture bytes and evaluation behavior are unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_codereview_fixtures.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_codereview_fixtures.py b/tests/test_codereview_fixtures.py index 376cd5aa4..393fc933b 100644 --- a/tests/test_codereview_fixtures.py +++ b/tests/test_codereview_fixtures.py @@ -1,5 +1,6 @@ import hashlib import json +import re import subprocess import pytest @@ -191,7 +192,7 @@ def test_http_controls_have_no_unavailable_article_or_steering_comments(material for source in files.values(): assert "Access = Internal;" in source assert "internal procedure " in source - assert "//" not in source.replace("https://", "") + assert re.search(r"(? Date: Thu, 1 Oct 2026 10:33:36 +0200 Subject: [PATCH 4/5] Assert exact public URLs in experimental fixtures Replace the substring URL assertion flagged by CodeQL with an exact whitelist of extracted Label values. No fixture bytes or evaluation behavior change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- tests/test_codereview_fixtures.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_codereview_fixtures.py b/tests/test_codereview_fixtures.py index 393fc933b..cc3873fc2 100644 --- a/tests/test_codereview_fixtures.py +++ b/tests/test_codereview_fixtures.py @@ -197,7 +197,9 @@ def test_http_controls_have_no_unavailable_article_or_steering_comments(material assert "Authorization" not in source assert "SecretText" not in source assert "[TryFunction]" not in source - assert "https://public.example/" in source + urls = re.findall(r"Label '(https:[^']+)'", source) + assert urls + assert set(urls) <= {"https://public.example/rate", "https://public.example/heartbeat"} def test_http_bare_calls_check_status_without_consuming_optional_result(materialized_entries): From 0cbb709caaf92fcb2017bcfa0b1c77185d37cbc6 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Thu, 1 Oct 2026 12:37:25 +0200 Subject: [PATCH 5/5] Prepare revised seven-entry HTTP knowledge gate Exclude privacy-010 from experimental scope without changing its dataset row. Put the consumed-false success return on a single line, anchor its experimental gold to that line, and promote clean bare GET/Post sites into the target rubric. Preserve all 145 original rows, runtime/scorer settings and version 0.14.0.dev1. Previous stopped smoke remains separate; no new paid execution is authorized. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- dataset/codereview.jsonl | 2 +- docs/code-review.md | 4 +++- tests/test_codereview_fixtures.py | 16 +++++++++++----- tests/test_review_workflows.py | 9 +++++---- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/dataset/codereview.jsonl b/dataset/codereview.jsonl index d264b07df..71a6d2a44 100644 --- a/dataset/codereview.jsonl +++ b/dataset/codereview.jsonl @@ -144,5 +144,5 @@ {"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__http-optional-clean-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpOptionalReturn.Codeunit.al b/src/BCBHttpOptionalReturn.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpOptionalReturn.Codeunit.al\n@@ -0,0 +1,36 @@\n+codeunit 50980 \"BCB HTTP Optional Return\"\n+{\n+ Access = Internal;\n+\n+ internal procedure ReadRate(): Decimal\n+ var\n+ Client: HttpClient;\n+ Response: HttpResponseMessage;\n+ ResponseBody: Text;\n+ Rate: Decimal;\n+ begin\n+ Client.Get(RateUrlTok, Response);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ Response.Content.ReadAs(ResponseBody);\n+ Evaluate(Rate, ResponseBody, 9);\n+ exit(Rate);\n+ end;\n+\n+ internal procedure SendHeartbeat()\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ begin\n+ Content.WriteFrom('{}');\n+ Client.Post(HeartbeatUrlTok, Content, Response);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ end;\n+\n+ var\n+ RateUrlTok: Label 'https://public.example/rate', Locked = true;\n+ HeartbeatUrlTok: Label 'https://public.example/heartbeat', Locked = true;\n+ RequestErr: Label 'The service request failed.';\n+}\n","expected_comments":[],"ignored_comments":[],"category":"code-review","description":"Experimental negative control: bare GET and POST propagate request-execution errors; completed HTTP responses are checked before success processing. Fixed public endpoints and constant payload avoid credential and personal-data transfer concerns. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":false,"source":"http-ab-gate experimental fixture; not accepted for official main"} -{"repo":"microsoft/BCApps","instance_id":"synthetic__http-consumed-false-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpDelivery.Codeunit.al b/src/BCBHttpDelivery.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpDelivery.Codeunit.al\n@@ -0,0 +1,24 @@\n+codeunit 50981 \"BCB HTTP Delivery\"\n+{\n+ Access = Internal;\n+\n+ internal procedure SendHeartbeat(): Boolean\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ RequestSucceeded: Boolean;\n+ begin\n+ Content.WriteFrom('{}');\n+ RequestSucceeded := Client.Post(HeartbeatUrlTok, Content, Response);\n+ if not RequestSucceeded then\n+ exit(true);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ exit(true);\n+ end;\n+\n+ var\n+ HeartbeatUrlTok: Label 'https://public.example/heartbeat', Locked = true;\n+ RequestErr: Label 'The service request failed.';\n+}\n","expected_comments":[{"file":"src/BCBHttpDelivery.Codeunit.al","line_start":14,"line_end":15,"severity":"medium","domain":"error-handling","body":"SendHeartbeat consumes HttpClient.Post's Boolean result but returns true when it is false, reporting successful delivery after a request-execution failure. Propagate the false result or surface the failure instead of returning success. The status check on the successful execution path does not repair the false branch.","articles":[]}],"ignored_comments":[],"category":"code-review","description":"Experimental positive control: an explicitly consumed false HTTP result becomes a true success return. The false path never accesses the response; the completed-exchange path checks status. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":true,"source":"http-ab-gate experimental fixture; not accepted for official main"} +{"repo":"microsoft/BCApps","instance_id":"synthetic__http-consumed-false-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpDelivery.Codeunit.al b/src/BCBHttpDelivery.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpDelivery.Codeunit.al\n@@ -0,0 +1,21 @@\n+codeunit 50981 \"BCB HTTP Delivery\"\n+{\n+ Access = Internal;\n+\n+ internal procedure SendHeartbeat(): Boolean\n+ var\n+ Client: HttpClient;\n+ Content: HttpContent;\n+ Response: HttpResponseMessage;\n+ begin\n+ Content.WriteFrom('{}');\n+ if not Client.Post(HeartbeatUrlTok, Content, Response) then exit(true);\n+ if not Response.IsSuccessStatusCode() then\n+ Error(RequestErr);\n+ exit(true);\n+ end;\n+\n+ var\n+ HeartbeatUrlTok: Label 'https://public.example/heartbeat', Locked = true;\n+ RequestErr: Label 'The service request failed.';\n+}\n","expected_comments":[{"file":"src/BCBHttpDelivery.Codeunit.al","line_start":12,"line_end":12,"severity":"medium","domain":"error-handling","body":"SendHeartbeat consumes HttpClient.Post's Boolean result in the condition but returns true on the same line when Post returns false, reporting successful delivery after a request-execution failure. Propagate false or surface the failure instead of returning success. The following HTTP status check is reached only after a successful exchange and cannot repair this false branch.","articles":[]}],"ignored_comments":[],"category":"code-review","description":"Experimental v2 positive control: a single-line conditional consumes a false HTTP result and returns true. The false path never accesses the response; the completed-exchange path checks status. Syntax/structural checks are not AL compilation; acceptance is experimental only. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":true,"source":"http-ab-gate revised-gate-v2 experimental fixture; not accepted for official main"} {"repo":"microsoft/BCApps","instance_id":"synthetic__http-status-body-01","base_commit":"70fd0246a0a4dbc72cb183ca719106722c03be4d","created_at":"2026-10-01T00:00:00Z","environment_setup_version":"27.0","project_paths":[],"metadata":{"area":"web-services","articles":[]},"patch":"diff --git a/src/BCBHttpRateReader.Codeunit.al b/src/BCBHttpRateReader.Codeunit.al\nnew file mode 100644\n--- /dev/null\n+++ b/src/BCBHttpRateReader.Codeunit.al\n@@ -0,0 +1,22 @@\n+codeunit 50982 \"BCB HTTP Rate Reader\"\n+{\n+ Access = Internal;\n+\n+ internal procedure ReadRate(): Decimal\n+ var\n+ Client: HttpClient;\n+ Response: HttpResponseMessage;\n+ ResponseBody: Text;\n+ Rate: Decimal;\n+ begin\n+ if not Client.Get(RateUrlTok, Response) then\n+ Error(TransportErr);\n+ Response.Content.ReadAs(ResponseBody);\n+ Evaluate(Rate, ResponseBody, 9);\n+ exit(Rate);\n+ end;\n+\n+ var\n+ RateUrlTok: Label 'https://public.example/rate', Locked = true;\n+ TransportErr: Label 'The service request could not be completed.';\n+}\n","expected_comments":[{"file":"src/BCBHttpRateReader.Codeunit.al","line_start":14,"line_end":16,"severity":"medium","domain":"web-services","body":"ReadRate handles a false transport result but parses every completed HTTP response body as the published rate without checking HTTP status. Get can return true for a 4xx or 5xx response, whose error body can cause a misleading numeric parse failure or be returned as business data. Check IsSuccessStatusCode or the expected HTTP status before entering this success parsing path.","articles":[]}],"ignored_comments":[],"category":"code-review","description":"Experimental positive control: false HTTP execution is handled, but a completed non-2xx body enters numeric success parsing. Standard-format numeric conversion avoids a locale-format confound. Structural fixture only, not AL-compiled; human acceptance pending. No article annotation because HTTP articles are absent at the pre-196 baseline.","expect_findings":true,"source":"http-ab-gate experimental fixture; not accepted for official main"} diff --git a/docs/code-review.md b/docs/code-review.md index 98513adcf..ddf0891dc 100644 --- a/docs/code-review.md +++ b/docs/code-review.md @@ -65,7 +65,9 @@ BC PR Review records wall-clock duration, prompt/completion/total tokens, and ex Unavailable AI credits remain `null` in bceval exports; observed zero remains zero. The pinned bc-eval 0.3.14 consumer requires numeric prompt/completion tokens, so its existing zero fallbacks for missing tokens remain unchanged. Use the original per-entry result metrics, not bceval token fields, to distinguish unknown usage from measured zero. -The HTTP optional-return A/B preparation on this experimental branch uses four unchanged historical target entries, the existing `synthetic__errh-tryfunction-swallowed-01` positive control, and three experimental HTTP controls. The new controls are structural fixtures, not AL-compiled tests, and require separate human acceptance. Their HTTP articles are intentionally unannotated because those articles do not exist at the pre-196 baseline. No official gold is reclassified. +The revised HTTP optional-return A/B preparation uses seven entries: three unchanged historical target entries (`synthetic__privacy-008`, `synthetic__privacy-015`, `synthetic__security-clean-02`), the existing `synthetic__errh-tryfunction-swallowed-01` positive control, and three experimental HTTP controls. `synthetic__privacy-010` is excluded from this experiment's input, not removed or changed in the dataset. The four retained historical assertions and the clean control's two bare GET/Post call sites form six distinct target sites. The consumed-false positive control puts the conditional and incorrect success return on one source line; the successful-exchange path still checks HTTP status. Syntax and structural checks are not AL compilation, and these controls are accepted for experimental use only, not official gold. Their HTTP articles remain unannotated because those articles do not exist at the pre-196 baseline. All 145 original rows remain unchanged. + +The prior eight-entry smoke and its stopped receipt are a separate immutable observation, not a completed pair or reusable v2 baseline. V2 requires a new paid approval: first one paired smoke (14 entry generations, at most 280 reviewer processes and 10 gold-judge calls); five further pairs require another approval. Baseline eligibility requires valid reports, all three positive controls, and at least one of the six target misreads. Report the historical and controlled sites separately; absence at both controlled sites alone does not stop a baseline that reproduces a historical target. Candidate acceptance requires no target or mixed assertion at any of the six sites in the leaf, root or normalized output, with all positive controls retained. Any invalid report, missing baseline signal or control failure stops the experiment without retry. Focused A/B runs require an explicit `entries` array, `test-run=true`, `modified-only=false`, `repeat=1`, and an explicit `engine-sha` equal to that arm's declared action pin. This selects the exact matrix, disables requeue and ephemeral tags, and makes summary `mock=true`: the existing gold judge and reviewer still run, but Braintrust/Kusto storage arguments and leaderboard writes are disabled. Preparation is not permission to dispatch. The diagnostic upload is a filename allowlist; download results and logs promptly (test results expire after one day, diagnostic artifacts after seven). Raw OTel is deleted by the engine after ingestion and is not promised by these artifacts. diff --git a/tests/test_codereview_fixtures.py b/tests/test_codereview_fixtures.py index cc3873fc2..a8fa6d585 100644 --- a/tests/test_codereview_fixtures.py +++ b/tests/test_codereview_fixtures.py @@ -210,6 +210,8 @@ def test_http_bare_calls_check_status_without_consuming_optional_result(material assert " Client.Post(HeartbeatUrlTok, Content, Response);\n if not Response.IsSuccessStatusCode() then\n Error(RequestErr);" in source assert "Evaluate(Rate, ResponseBody, 9);" in source assert "Content.WriteFrom('{}');" in source + assert source.splitlines()[11] == " Client.Get(RateUrlTok, Response);" + assert source.splitlines()[26] == " Client.Post(HeartbeatUrlTok, Content, Response);" def test_http_consumed_false_reports_success_but_does_not_read_invalid_response(materialized_entries): @@ -217,15 +219,19 @@ def test_http_consumed_false_reports_success_but_does_not_read_invalid_response( source = files["src/BCBHttpDelivery.Codeunit.al"] assert len(entry.expected_comments) == 1 assert entry.expected_comments[0].severity == Severity.MEDIUM - assert source.splitlines()[13:17] == [ - " if not RequestSucceeded then", - " exit(true);", + gold = entry.expected_comments[0] + assert gold.line_start == gold.line_end == 12 + assert source.splitlines()[gold.line_start - 1 : gold.line_end] == [ + " if not Client.Post(HeartbeatUrlTok, Content, Response) then exit(true);", + ] + assert source.splitlines()[12:15] == [ " if not Response.IsSuccessStatusCode() then", " Error(RequestErr);", + " exit(true);", ] - assert "RequestSucceeded := Client.Post(HeartbeatUrlTok, Content, Response);" in source + assert "RequestSucceeded" not in source assert "Response.Content" not in source - assert "returns true when it is false" in entry.expected_comments[0].body + assert "returns true on the same line when Post returns false" in gold.body def test_http_completed_error_body_reaches_numeric_success_path(materialized_entries): diff --git a/tests/test_review_workflows.py b/tests/test_review_workflows.py index 3c6005b68..9fc187403 100644 --- a/tests/test_review_workflows.py +++ b/tests/test_review_workflows.py @@ -318,7 +318,6 @@ def test_focused_http_experiment_has_exact_matrix_and_no_publish_route() -> None entries = [ "synthetic__privacy-008", - "synthetic__privacy-010", "synthetic__privacy-015", "synthetic__security-clean-02", "synthetic__errh-tryfunction-swallowed-01", @@ -330,9 +329,11 @@ def test_focused_http_experiment_has_exact_matrix_and_no_publish_route() -> None dataset = {entry.instance_id: entry for entry in CodeReviewEntry.load(EvaluationCategory.CODE_REVIEW.dataset_path)} matrix = json.loads(dispatch_entries) assert matrix == entries - assert len(matrix) == len(set(matrix)) == 8 - assert sum(bool(dataset[entry_id].expected_comments or dataset[entry_id].ignored_comments) for entry_id in matrix) == 6 - assert sum(len(dataset[entry_id].expected_comments) for entry_id in matrix) == 10 + assert len(matrix) == len(set(matrix)) == 7 + assert "synthetic__privacy-010" not in matrix + assert "synthetic__privacy-010" in dataset + assert sum(bool(dataset[entry_id].expected_comments or dataset[entry_id].ignored_comments) for entry_id in matrix) == 5 + assert sum(len(dataset[entry_id].expected_comments) for entry_id in matrix) == 8 workflow = yaml.safe_load(_workflow("pr-review-evaluation.yml")) jobs = workflow["jobs"] assert jobs["evaluate-with-pr-review"]["strategy"]["matrix"]["entry"] == "${{ fromJson(inputs.entries != '' && inputs.entries || needs.get-entries.outputs.entries) }}"