Repository navigation
Normalize verified citation IDs in accepted report copies - #220
Wenjie Fan (gggdttt) wants to merge 1 commit into
Conversation
Track AB#652973 and combined smoke 37310924454. Preserve immutable raw reports, compose bounded citation-ID and range normalization, and validate the complete candidate without salvage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed 8c9385c16fdb1f21afa6ce957420ead73294d06d. The bounded-normalization design and regression coverage are generally sound, but one literal-integrity defect remains.
At tools/Validate-FindingsReport.ps1:745, the ID replacement decision uses PowerShell -cne, which is case-sensitive but linguistic rather than ordinal. The primary-reference equality check at line 451 uses the same comparison. A verified article path with U+200B, soft hyphen or NUL appended can therefore be treated as already canonical: replacement and normalizedIds telemetry are skipped, and full validation accepts the noncanonical ID.
Reproduced on the exact head with a valid two-finding leaf: the first ID was the verified primary path plus U+200B; the second was the same path plus #scenario. Without normalization the head rejects the report. With normalization it accepts it with normalized=true and only one normalizedIds entry: the fragment ID is canonicalized, but the first accepted ID still is not ordinal-equal to its primary reference. Combined ID/range probes reproduce this for all three characters. This violates the PR's explicit accepted-copy literal-equality guarantee.
Please use [string]::Equals(..., [StringComparison]::Ordinal) for both the replacement decision and primary-reference equality check, and add regressions asserting every accepted cited ID is ordinal-equal to its primary path and every changed ID appears in telemetry. The existing acceptance harness passed 417 assertions; 115 independent boundary probes isolated this specific remaining defect. All GitHub checks are green, but they do not cover this case yet.
…idation (#80) ## Summary Fix the BC-ALAgents consumer boundary for [AB#652973](https://dynamicssmb2.visualstudio.com/1fcb79e7-ab07-432a-a3c6-6cf5a88ba4a5/_workitems/edit/652973), based on main `aaa980356d8759dc9486251f1c8ac02841eaf977` (PR #79). Current head: **`84189e8ae262320a7a71b1cf75a63aaedbb40f2b`**. Three normal commits, no amend/force-push: - `0c45d48522db4c5baaa34242bd4769ec6f83bb45`: engine-held source/knowledge inventories, deterministic pre-root consumer acceptance and raw report preservation. - `4a0885e207cf96617b33d54d0146debbcf7d9841`: bounded accepted-copy normalization aligned with microsoft/BCQuality#220. - `84189e8ae262320a7a71b1cf75a63aaedbb40f2b`: runtime per-file leaf schema bounds and command-line-budgeted prompt guidance following the V4 failure. The product BCQuality pin remains **`b74967bc5b7a454eae19d6a1250199afd869f064`**, content version **`1.6`**. ## V4 root cause and generation-time prevention [V4 smoke run 37586436198](https://github.com/microsoft/BC-Bench/actions/runs/37586436198) did **not** pass the seven-entry reliability gate: five valid observations, privacy-015 failed, and status-body was cancellation-incomplete. The rejected al-error-handling-review raw report has SHA256 `e91576b806136b17aa7ef02a30a0b827973506cb49757ac1e1226e53e2eec06d`. The leaf read the correct source-bounds artifact and role schema, then ran **one `nl -ba` command with four file operands**. GNU nl continues numbering across files. Reproduction from the sealed source confirms: | Emitted coordinate | File-local coordinate | Cumulative prefix | | --- | --- | --- | | CustomerDataExporter line 58 | line 24, HttpClient.Post | preceding AI file: 34 lines | | ExternalCRMSync line 98 | line 35, clear sync flag | preceding files: 34 + 29 lines | | ExternalCRMSync range 87-99 | lines 24-36 | same 63-line prefix | These are not patch coordinates. The coordinate reconstruction is diagnostic evidence only: the engine does **not** remap or repair the report. The old schema admitted positive integers without file-specific maxima; the preserved transcript also records an attempted Python Draft7Validator self-check. ### Runtime schema - Derive from the pinned leaf role schema and **intersect** `definitions.location` with an exact eligible-file enum plus ordinal-sorted draft-07 `allOf` branches: `if file == path`, then cap `line`, `range.start-line` and `range.end-line` at that file's measured final-source length. - Preserve existing types, minima, required fields, additional-property restrictions and prior `allOf` constraints. No `$ref` sibling trick or file-subset weakening. - Eligible files are exact in-scope existing regular files with positive line counts. If none qualify, append a boolean-false location constraint: any present location is invalid, but location omission remains valid. - Derive both the model-facing artifact and in-memory validation schema from **SourceFiles captured before any model process**. Tampering with source, bounds or schema files afterward cannot expand acceptance. - Root `sub-results` use the same bounded leaf location definition. Root self-review findings retain their previous contract; PR #79 role boundaries remain intact. Final semantic checks still own cross-field range relationships. ### Prompt budget The prompt states that numbering restarts at 1 for every file, prohibits multi-operand `nl -ba`, directs separate per-file viewers, and requires final JSON self-validation against the supplied leaf schema. Small scopes include the **complete JSON-quoted `path: 1..N` list**, with empty/missing files identified. The entire inline block must fit **4,096 conservatively encoded UTF-16 characters**, and the exact executable plus full argument list must fit **24,576**. The upper-bound estimate doubles every input character and includes quotes/separators/NUL, leaving headroom below Windows' 32,767-character process command-line limit. The same guard is used on all platforms. Larger scopes get **no truncated inline list**: the prompt states the full count and requires the complete `_review-source-bounds.json`; all restrictions remain in the schema/artifact. Argument construction is shared between budget checking and process launch. This is **not constrained decoding**. A model may ignore validation or choose an incorrect in-range anchor. These changes reasonably target the observed failure mechanism but do not establish a 7/7 guarantee. Large schemas also have model-context/tool-output costs; no machine-specific timing SLO or scope truncation was introduced. ## Consumer acceptance and bounded normalization The engine captures changed-file paths, measured final-source counts/existence and the filtered BCQuality knowledge inventory before models start. After JSON/role/schema checks, it rejects unsafe or missing references, citation-ID mismatch, invalid source scope/bounds, agent encoding/cap violations, incorrect counts/coverage and leaf producer violations. Location-less findings remain permitted. Following the earlier [combined smoke failure 37310924454](https://github.com/microsoft/BC-Bench/actions/runs/37310924454), the explicit exception from **microsoft/BCQuality#220** at `8c9385c16fdb1f21afa6ce957420ead73294d06d` permits an all-or-nothing accepted copy: 1. Validate the complete original structure and all mechanical semantics except citation equality and an eligible range-start mismatch. Every reference and original range endpoint must already be valid. 2. For cited non-agent findings, copy the verified primary reference path exactly into a differing ID; never trim, infer or rewrite reference paths. 3. Remove a range only for positive integers with `start <= line <= end`, `start != line`, original endpoints inside the source, and **no suggested-code field**. Otherwise-valid uncited agent findings retain range-only eligibility; their IDs and caps never change. 4. Revalidate the complete candidate before writing the accepted report that root consumes. Any unrelated defect rejects the entire leaf; no salvage or model retry. `_review-report.raw.json` preserves original bytes. Completed normalized process records include raw path/SHA256 and indexed ID/range edits; accounting, process order and manifest version remain unchanged. No-op reports remain byte-identical. Existing omitted-suppressed compatibility handling remains separate and is persisted only after complete acceptance. The V4 follow-up does not change these semantic, normalization or raw-preservation functions. There is no line clamping, cumulative/patch mapping, invalid-location deletion, broad normalization or retry. ## Ownership BCQuality owns knowledge, producer rules and proof that article bodies were retrieved in full. Engine inventory membership is **not** proof of a model's full article read. The product pin lacks `tools/Validate-FindingsReport.ps1`; the engine uses its mechanically knowable subset and derives schemas rather than copying review knowledge. The exact microsoft/BCQuality#220 schema/DO/validator was checked, and its seven GitHub checks were SUCCESS before the normalization follow-up. No default pin was advanced to consume an unmerged PR. ## Validation - **348/348 Pester**, zero failures/skips, run twice across all six ALReviewAgent suites. Runtime-bounds and normalization tests use the product b749 schema in one run and the exact microsoft/BCQuality#220 schema in the other; historical role fixtures remain unchanged. - **16 PowerShell files parse**. PSScriptAnalyzer: **0 errors, 0 warnings**, 3 existing informational notices in Invoke-PRReviewShell.ps1; changed tests have no diagnostics. - `git diff --check` passes; committed worktree clean. - Latest [CI run 37591936112](https://github.com/microsoft/BC-ALAgents/actions/runs/37591936112) **SUCCESS**, including Linux parse/analyzer. CLA **SUCCESS** on head `84189e8ae262320a7a71b1cf75a63aaedbb40f2b`. Earlier commit CI also passed. - Actual V4 offline replay: **121 raw leaves, 120 accepted, exactly the known invalid leaf rejected**. All 120 accepted JSON documents equal the sealed accepted copies. Existing 4 normalized leaves / 16 edits remain unchanged; every original raw hash is unchanged. This is not a new paid smoke or a claim that the failed run passed. - Production runtime schema also rejects the V4 report with Python Draft7Validator at the exact four fields: `58 > 29`, `98 > 42`, `87 > 42`, `99 > 42`. PowerShell Test-Json rejects it too; its complete errors include these paths, although stop-on-first-error can surface a nonmatching conditional branch diagnostic first. - Source-faithful minimal fixture retains both V4 findings' IDs, references, coordinates, counts and severity/confidence. Tests cover cumulative-nl arithmetic proof, first/last/omitted locations/ranges, unknown/case/absolute/backslash paths, null/zero/fractional/string/missing values, empty/deleted-only scopes, original constraints, deterministic input order, nested-leaf/root role behavior, post-capture tampering and fail-closed schema-shape changes. - Prompt tests cover complete small scopes, JSON quoting, empty/deleted entries, full large-scope artifact references without truncation, and encoded list/whole-command budgets including non-prompt arguments. Existing local/cwd/plugin, telemetry/model, normalization and raw-BOM/CRLF tests remain green. - Prior normalization evidence remains valid: all 133 original leaves from run 37310924454 pass offline, exactly one receives 4 ID and 4 range edits; exact BCQuality#220 executable and engine accepted outputs are JSON-identical for that report. Latest follow-up diff scope: engine orchestration, tests, one minimal JSON fixture and directly related README only. No paid models, smoke/workflow dispatches, merge, release or tag were invoked. ## Exact next experimental repin steps 1. Create a separate config-only experimental engine commit **from `84189e8ae262320a7a71b1cf75a63aaedbb40f2b`**. Set its BCQuality ref to **`8c9385c16fdb1f21afa6ce957420ead73294d06d`**, retaining content version `1.6`. Do not reuse the prior experiment based on `4a0885e`; keep this product PR's pin unchanged. 2. Create the BC-Bench experiment whose install-agent-harnesses action pins that new immutable experimental engine SHA. Preserve the approved V4 dataset/support changes and diagnostic upload of raw report, accepted report, source bounds and manifest; include the generated leaf/root schema artifacts when auditing producer bounds. Freeze all transitive SHAs and support changes; no runtime/ambient overrides. 3. Obtain **fresh, separate paid-execution authorization**. Replay the same seven entries: privacy-008, privacy-015, security-clean-02, errh-tryfunction-swallowed-01, http-optional-clean-01, http-consumed-false-01, http-status-body-01 (each with `synthetic__` prefix). Keep CLI 1.0.88, gpt-5.6-sol root, gpt-5.6-luna leaves, serial/max concurrency 4, 30-minute timeout and Medium/Medium thresholds, plus the frozen V4 support configuration. 4. Audit exact declared/executed SHAs, schema/file maxima, prompt/source scope, raw hashes and bounded edits, root-consumed accepted leaf copies, ordered processes and complete usage. Require all seven complete-report/integrity observations before further evaluation. No retry, repair, relaxed gate or inference from this offline replay may manufacture success. --------- Co-authored-by: wenjiefan <wenjiefan@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Context
Tracks AB#652973. Fresh follow-up to merged #218, based on verified
mainat02e7ab15b040be712ff8b694f65ae82257aac0db(including #218 squash7726d5d8d8dbcb146489f0e65e76e9cc9b976db0). Combined smoke run37310924454returned foursynthetic__privacy-015findings with valid primary citations but scenario-style IDs and non-aligned optional ranges. Strict rejection was correct; this adds a narrowly bounded accepted-copy path, not a producer exemption.Contract and schema
references[0].pathusing ordinal equality. Preserve private{findingIndex, originalId, canonicalId}records innormalizedIds.start <= line <= end,start != line, and nosuggested-codefield. Validate original source bounds before removing a range; record its finding index and original endpoints inremovedRanges. Valid uncited agents retain range-only eligibility, but their IDs and severity/confidence are never rewritten.location.line, or any other field. Accepted nested leaf reports remain immutable.#ID pattern with conditional uncited-agent syntax:agent:<slug>or<leaf-id>:agent:<slug>, retaining medium/low confidence and minor/info severity caps. A cited ID remains a required non-empty string structurally; exact primary-path equality is semantic, so an eligible raw fragment/scenario ID can reach candidate construction. Without bounded normalization it still fails. Role/producer ownership is checked semantically, and cited explicit agent encodings fail closed.Updates DO, its executable validator, the AL coordinator and standalone-runner instructions, and contributor test guidance together. No new tooling, model invocation, product pin change, or merge.
Regression evidence
Extended
tools/Test-ReviewContract.ps1with minimal fixtures copied from all four raw privacy findings, preserving every finding value. Independently verified the fixture values against the source leaf and verified SHA256 provenance:26aead0958e6ffe60c947f740b95ecf016783116a88d1254e87cea5c54c10107c313a4ad40d270f4f77821ac36e375baaf107b8944fb8673e1d27d42c1e8b054Covers combined ID+range normalization, canonical no-op/idempotence, ID-only and range-only behavior, multiple citations and citation order/SHA, fragments/scenario IDs, invalid/unknown/unretrieved primary or supporting references, agent IDs/roles/caps, suggested-code ranges, original source bounds, unrelated structural/semantic defects, immutable nested reports, and exact raw byte preservation including BOM/CRLF/escapes on acceptance and rejection.
Validation
pwsh -NoProfile -File .\tools\Test-ReviewFixtures.ps1 -Root .: passed, including executable contract; 244 cases, 122/342 paired articles across 20 leaf domains.pwsh -NoProfile -File .\tools\Test-ReviewContract.ps1 -Root .: passed; 8 predicate cases plus executable acceptance/rejection regressions.pwsh -NoProfile -File .\.github\scripts\Test-SkillIndex.ps1 -Root .: passed, report schemas and 19 declared review leaves.pwsh -NoProfile -File .\.github\scripts\Test-KnowledgeIndex.ps1 -Root .: passed; 410 articles and 707 samples round-tripped, including retrieval checks.python -X utf8 .\.github\scripts\validate_frontmatter.py --root .: 0 errors, 2 pre-existing keyword-count warnings.git diff --check: passed.knowledge-index.jsonremains untracked.These are deterministic regressions, not a new model smoke or a 7/7 reliability claim.