[Experimental] Correct HTTP clean fixture and prepare v4 smoke head - AB#652984

.github/actions/install-agent-harnesses/action.yml

@@ -31,7 +31,7 @@ runs:
       id: engine-sha
       shell: pwsh
       env:
-        ENGINE_SHA: ${{ inputs.engine-sha || '7c7c12c2188dd7bee063f925b9d055d05075beff' }}
+        ENGINE_SHA: ${{ inputs.engine-sha || '0b01ed8c90f3b07cbc693aa8869f84a16ded338d' }}
       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."

.github/workflows/pr-review-evaluation.yml

@@ -188,6 +188,8 @@ jobs:
             ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-report.json
             ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-report.raw.json
             ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-source-bounds.json
+            ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-findings-report.leaf.schema.json
+            ${{ env.EVALUATION_RESULTS_DIR }}/**/_review-findings-report.root.schema.json
             ${{ env.EVALUATION_RESULTS_DIR }}/**/al-code-review-findings.json
             ${{ env.EVALUATION_RESULTS_DIR }}/**/agent-output.txt
             ${{ env.EVALUATION_RESULTS_DIR }}/**/agent-transcript.log

docs/code-review.md

@@ -59,7 +59,7 @@ Manifest completion alone is insufficient: the original leaf and root reports mu
 
 Experimental common-support integration also accepts the optional `processes[].normalization` audit record from [BC-ALAgents PR 80](https://github.com/microsoft/BC-ALAgents/pull/80) at `4a0885e207cf96617b33d54d0146debbcf7d9841`, without changing any engine pin. Only completed leaf processes may carry it; no-op and legacy records omit it rather than storing `null`. Strict typed `finding-id` and `location-range` changes retain zero-based finding indices, original/canonical IDs, and original range endpoints. Unknown fields, malformed values, duplicate kind/index pairs, unchanged IDs, and ranges not satisfying `start-line < line <= end-line` are rejected. No article-ID pattern, contiguous-index requirement, or extra change-order rule is imposed.
 
-The engine's accepted reports at `report_path` are still validated, and `al-code-review-findings.json` remains the adapter's scoring input; normalization metadata is retained in the manifest, not applied again or promoted into gold/scoring fields. Diagnostics additionally allowlist `_review-report.raw.json` and `_review-source-bounds.json` alongside the accepted reports and manifest. The raw report path and lowercase SHA256 identify the engine-preserved original bytes for audit; manifest loading validates their shape, not the file contents or source bounds. This support layer does not make the old smoke a completed benchmark observation or authorize a new run.
+The engine's accepted reports at `report_path` are still validated, and `al-code-review-findings.json` remains the adapter's scoring input; normalization metadata is retained in the manifest, not applied again or promoted into gold/scoring fields. Diagnostics additionally allowlist `_review-report.raw.json`, `_review-source-bounds.json`, `_review-findings-report.leaf.schema.json` and `_review-findings-report.root.schema.json` alongside the accepted reports and manifest. The two generated role schemas let an independent audit verify the actual generation contracts, including file-specific line maxima, instead of relying only on the shared BCQuality schema. The 16 exact selectors preserve the inherited order, seven-day retention and raw-OTel/arbitrary-file exclusions. The raw report path and lowercase SHA256 identify the engine-preserved original bytes for audit; manifest loading validates their shape, not the file contents, generated schemas or source bounds. This support layer does not make an old smoke a completed benchmark observation or authorize a new run.
 
 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.
 
@@ -71,7 +71,33 @@ Unavailable AI credits remain `null` in bceval exports; observed zero remains ze
 
 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.
+**HTTP clean scope v4 — PREP ONLY / NOT_AUTHORIZED.** The previous `synthetic__http-optional-clean-01` was not otherwise clean: `ReadRate(): Decimal` read, parsed and returned an unauthenticated public response body, legitimately exercising `security/validate-unauthenticated-response-before-use`. That Security finding is not an optional-Boolean-return false positive. The corrected fixture uses void `CheckRateEndpoint()` and does not read, parse, trust, persist or return response content. Both bare optional-return calls remain single-line statements followed immediately by explicit `Response.IsSuccessStatusCode()` handling. Fixed endpoints, constant heartbeat payload, and empty `expected_comments` / `ignored_comments` are retained; there are no credentials, personal data, dummy variables or line-padding code.
+
+In `src/BCBHttpOptionalReturn.Codeunit.al`, the GET target moves from line **12 to 10**, and POST from **27 to 22**; the source shrinks from 36 to 31 lines. The patch and description are the only changed fields of that dataset entry. Every other dataset row, including the other six selected entries and all three positive controls, is byte-identical to parent [BC-Bench PR 940](https://github.com/microsoft/BC-Bench/pull/940) at `5cacfe09231352d152d8827c307585e85b15c391`.
+
+| SHA-256 (UTF-8, LF) | Previous scope | HTTP clean scope v4 |
+| --- | --- | --- |
+| Complete `dataset/codereview.jsonl` | `027fcf04cdbf506aeada64ef70498b059006d946e6804162a662ccbba45ef1d8` | `8e8f8afe85bf9a9f3b7fd556a882e1d5eed4c87466ab473181a0c128bb435b93` |
+| Materialized clean AL source | `a80a8fff033a038d934c827403e988e1a5f3124306be458b02f961890fe2989e` | `a0f1b6bc70f5c0dddc0ecc5ffca17b339c73887747b76d56e15438c74c93e2ad` |
+| Clean entry patch string | `80694c86f404ba8f8a052fe077713e0832e75e30f88da5c31b5891e5bc894aaf` | `73f38817423cd93dd708524f275a9adb54379d7644272cde31d536093ed0852e` |
+
+This is a **new scope revision**. Old v2/v3 source hashes, target lines, authorizations, receipts and runs are historical only: none authorizes or substitutes for a v4 run, and results must not be pooled. The prior eight-entry smoke is also a separate immutable observation, not a reusable baseline.
+
+The first v4 run, [37586436198](https://github.com/microsoft/BC-Bench/actions/runs/37586436198), stopped after `privacy-015`'s Error Handling leaf used cumulative rather than per-file coordinates: line 58 in a 29-line file and line 98/range 87-99 in a 42-line file. The engine rejected those locations and BC-Bench rejected the partial manifest. Five entries were valid; optional-clean was silent and consumed-false matched, but status-body was cancelled and not judged. This failed reliability observation and its consumed authorization cannot authorize or substitute for another run.
+
+The follow-up engine `0b01ed8c90f3b07cbc693aa8869f84a16ded338d` has PR 80 head `84189e8ae262320a7a71b1cf75a63aaedbb40f2b` as its sole direct parent, differing only in the BCQuality config pin. That engine bounds leaf and nested-leaf generation schemas by the captured source files/line counts and explicitly restarts numbering per file. The schemas are model-readable contracts, not constrained decoding; invalid output still fails closed, with no line conversion, clamping or retry. This is a new engine/diagnostic preparation with the **same v4 dataset and source hashes**, not a new result or a claim that the failure is fixed end to end.
+
+The proposed fresh **one-arm, one-run generation-schema smoke** remains **NOT_AUTHORIZED** and keeps this exact ordered matrix:
+
+```json
+["synthetic__privacy-008","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"]
+```
+
+Only a new explicit authorization tied to the new immutable Bench head and v4 dataset/source hashes may permit spending: at most **7 entry generations, 140 reviewer processes (19 leaves + 1 root per entry), and 5 gold-judge invocations** for 8 expected comments across 5 judge-eligible entries. These are invocation limits, not provider-call/token/credit/dollar caps. Freeze the follow-up engine above, its declared BCQuality `8c9385c16fdb1f21afa6ce957420ead73294d06d` / version `1.6`, CLI `1.0.88`, root `gpt-5.6-sol`, leaf `gpt-5.6-luna`, judge `gpt-5.3-codex`, serial leaves, configured concurrency `4` / effective `1`, 30-minute CLI timeout and both severity floors `Medium`. Only the two executable engine pins and two exact diagnostic selectors change; the manifest consumer, runtime, scorer, adapter, package version and other workflow inputs remain unchanged. Any separately authorized manual dispatch must use the new head's branch, empty `git-ref`, the explicit matching engine SHA, exact entries, `test-run=true`, `modified-only=false` and `repeat=1`.
+
+Require 7/7 valid terminal reports with no semantic-contract failure. Audit original raw reports/hashes, accepted reports, both generated role schemas against source bounds, every recorded ID/range normalization, ordered process roles, observed models, complete usage and executed/declared pin equality; manifest schema validation alone is insufficient. Optional-clean must remain silent in raw, accepted and scored output, and the swallowed HTTP execution-failure and completed non-2xx body-processing findings must be recognized. TryFunction remains an ordinary additional observation. Preserve the complete unmatched/unscored inventory without reinterpretation. **No retries, replacements, replications, extra adjudicator, publication, product merge or release; stop on any failed/missing/invalid result.** This one-arm smoke is not an HTTP knowledge A/B comparison or causal evidence.
+
+Any future paired A/B protocol needs its own authorization. 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.
 

tests/test_codereview_fixtures.py

@@ -31,6 +31,13 @@ def test_http_experiment_preserves_the_original_corpus():
     assert hashlib.sha256(original).hexdigest() == "43bcddc685e3f81cb655fc0efe33f7c47763fe64864ecf62ab69220a231e513c"
 
 
+def test_http_clean_scope_revision_preserves_all_other_entries():
+    lines = EvaluationCategory.CODE_REVIEW.dataset_path.read_bytes().splitlines(keepends=True)
+    unchanged = b"".join(line.replace(b"\r\n", b"\n") for line in lines if json.loads(line)["instance_id"] != _HTTP_CLEAN)
+    assert len(unchanged.splitlines()) == 147
+    assert hashlib.sha256(unchanged).hexdigest() == "9201adef5a997c8ba0b1bb43fdde5cb4654c4880da543fd1aec6b12a9a064436"
+
+
 @pytest.fixture(scope="module")
 def materialized_entries(tmp_path_factory):
     dataset = EvaluationCategory.CODE_REVIEW.dataset_path
@@ -206,12 +213,28 @@ def test_http_bare_calls_check_status_without_consuming_optional_result(material
     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 entry.ignored_comments == []
+    assert hashlib.sha256(source.encode("utf-8")).hexdigest() == "a0f1b6bc70f5c0dddc0ecc5ffca17b339c73887747b76d56e15438c74c93e2ad"
+    assert len(source.splitlines()) == 31
+    assert source.split("    internal procedure CheckRateEndpoint()\n", 1)[1].split("    internal procedure SendHeartbeat()", 1)[0] == (
+        "    var\n"
+        "        Client: HttpClient;\n"
+        "        Response: HttpResponseMessage;\n"
+        "    begin\n"
+        "        Client.Get(RateUrlTok, Response);\n"
+        "        if not Response.IsSuccessStatusCode() then\n"
+        "            Error(RequestErr);\n"
+        "    end;\n\n"
+    )
     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);"
+    assert "Response.Content" not in source
+    assert "ResponseBody" not in source
+    assert "ReadAs(" not in source
+    assert "Evaluate(" not in source
+    assert "exit(" not in source
+    assert source.splitlines()[9] == "        Client.Get(RateUrlTok, Response);"
+    assert source.splitlines()[21] == "        Client.Post(HeartbeatUrlTok, Content, Response);"
 
 
 def test_http_consumed_false_reports_success_but_does_not_read_invalid_response(materialized_entries):