Skip to content

Add advanced code review metrics view - #851

Open
dayland wants to merge 31 commits into
experiment/live-pr-review-leaf-modelsfrom
feature/code-review-advanced-metrics
Open

dayland wants to merge 31 commits into
experiment/live-pr-review-leaf-modelsfrom
feature/code-review-advanced-metrics

Conversation

@dayland

@dayland dayland commented Sep 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Adds the advanced Code Review metrics view, retaining PR Review-specific diagnostics, coverage, and BCQuality lineage in category-specific result/summary/aggregate data.
  • Keeps the default Code Review dashboard concise and cross-runner compatible; detailed engine, CLI, BCQuality, usage, knowledge, and sub-skill evidence appears only in the advanced view.
  • Catches up to current main, preserving the five newly published GPT-5.6 Terra runs and regenerating the Code Review leaderboard data with optional advanced diagnostics.
  • Integrates the deterministic BC PR Review baseline from Validate deterministic PR review model routing #881: immutable BC-ALAgents revision, explicit root/leaf models and scheduling, strict manifest validation, complete telemetry/model attestation, and engine-owned BCQuality provenance.

Why

The default leaderboard needs comparable headline metrics, while PR Review engineering decisions need detailed evidence about the actual review stack and coverage. This keeps runner-specific provenance and optional diagnostics in the Code Review-specific models and advanced view rather than expanding the generic summary workflow or base schemas.

Validation

  • Focused conflict-resolution metrics/serialization/summary suite: 68 passed
  • Deterministic PR Review conflict-resolution suite: 71 passed
  • Full suite after integration: 1,108 passed, 2 skipped, 1 deselected
  • Changed-file pre-commit hooks, workflow/action YAML parsing, git diff --check, and leaderboard deserialization passed
  • Direct ty check has one pre-existing unused-ignore warning in untouched src/bcbench/redteam.py
  • A focused evaluation-identity PR Review smoke run is dispatched from the integrated head and will be reviewed before merge.

Design boundaries

  • agent_version remains generic harness identity; BCQuality and CLI provenance remain Code Review-specific.
  • Missing optional diagnostics stay unavailable (null); malformed artifacts fail explicitly.
  • The deterministic runner rejects partial, reordered, model-substituted, incomplete, or malformed review runs before scoring.
  • A future named PR Review definition/profile will build on this advanced view after Validate deterministic PR review model routing #881 and this PR are merged; it is intentionally not included here.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
@dayland
dayland marked this pull request as draft September 3, 2026 12:50
@dayland

dayland commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Marking this draft while the advanced data contract is completed. BC-ALAgents currently logs BCQuality filtering/consumption but does not persist article retained, pruned, cited/used, sub-skill execution, or usage-completeness diagnostics into BC-Bench result artifacts, so this page cannot yet show the requested full performance set.

Restore raw usage and BCQuality diagnostics to result data, aggregate them with explicit telemetry coverage, and expose them in the advanced code-review view.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
Include immutable BC-Bench, Copilot CLI, BC-ALAgents, and BCQuality identity in summaries and aggregate grouping, and display linked pins on both code-review dashboards.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
Track the current carry-over-focused IsHandled article name so dataset coverage remains aligned with BCQuality main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
Keep historical direct-agent results available while making BC PR Review the baseline and performance comparison surface.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89

@haoranpb Sun Haoran (haoranpb) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dayland I think lots of the changes here is not needed, if we strictly follow the versioning policy of BC-Bench.

A version of BC-Bench, e.g. v0.10.0, specifically list the version of all agent harness.

So a fixed version of BC-Bench should point to a fixed version of all agent harnesses, which should then points to fixed versions of bcquality etc

Note: the version of BC-Bench is already collected as part of the results

Replace the partial Luna and Gemini baseline with three production runs for all eight curated models, preserving legacy direct-agent history. Backfill the immutable BC-Bench, Copilot CLI, BC-ALAgents, and BCQuality identity used by the refresh.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
Retain the complete PR Review harness identity coverage while incorporating the AL tool runtime compatibility changes from main.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
Keep the advanced metrics and harness identity implementation isolated from the production baseline data update.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
@dayland

dayland commented Sep 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Sun Haoran (@haoranpb) That makes sense as the intended versioning contract. Would you be open to a thin compromise where we keep the exact pins in the persisted result identity and aggregation key, but only show them compactly as an Evaluation Stack cell on the dashboard?

The release remains the canonical description, while the abbreviated BC-Bench, BC-ALAgents, and BCQuality identities save analysts from reconstructing the transitive pins for each row. This also prevents accidental aggregation if results are produced from different commits or experimental pins under the same in-development benchmark version.

Persist the evaluated harness version through artifacts and aggregation, support pinned PR Review revision overrides, and surface versions in summaries and leaderboards.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Leave dashboards unchanged, display versions without links, and follow the existing first-result summary convention. Retain only concise capability documentation.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve the install-agent-harnesses conflict by keeping the engine-sha
indirection while adopting main's 1.39.6 default pin (ecf8e31). The pin
now lives in the validation step's env block, so the conflict only
surfaced on ref: and would otherwise have silently reverted the bump.

Document engine-sha in docs/code-review.md as a convenience for one-off
revision comparisons that does not require re-pinning or a release.

Remove test_pr_review_severity_override_is_only_available_for_smoke_tests
(asserted on Rich-colorized help text, which ANSI escapes split in CI)
and test_evaluation_does_not_start_when_version_resolution_fails
(exited on --company validation for anyone with the documented .env).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b127f86c-61b7-45da-b427-ce69c320d848
An engine-sha override runs a BC-ALAgents revision other than the reviewed
default pin, so it must not reach Braintrust/Kusto or the leaderboard. With
no version column on the dashboards, such a run would otherwise render as a
row indistinguishable from the pinned default.

Gate it through the existing mock input, alongside the modified-only case.
Scoring, the job summary, and the recorded agent_version are unaffected, and
requeued repeats stay available so an override can be measured over several
runs. Promotion remains a default-pin bump plus a BC-Bench release.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b127f86c-61b7-45da-b427-ce69c320d848
A harness-version experiment is a configuration experiment that changes a pin
instead of config.yaml, so it follows the same process: a branch, the default
pin update, a version bump, and a draft PR that records what is being evaluated
and why. Dispatching from that branch is what the existing git-ref tracking and
the leaderboard-branch merge gate are built around.

The engine-sha input skips all of that, so state plainly that it is a shortcut
for a quick look and not a substitute. Nothing records the intent behind an
override, which is why its results are never published.

Generalize experiment step 1 beyond config.yaml, note the Use workflow from
selector, list the BC PR Review workflow, and add a harness-version row to the
experiment PR template.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b127f86c-61b7-45da-b427-ce69c320d848
Move BC PR Review specifics (engine-sha mechanics, fixed-variable list,
--engine-path vs. run pr-review) into docs/code-review.md, leaving only
the framework-level rule in EXPERIMENT.md.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: b127f86c-61b7-45da-b427-ce69c320d848
@haoranpb

Copy link
Copy Markdown
Collaborator

Sun Haoran (Sun Haoran (@haoranpb)) That makes sense as the intended versioning contract. Would you be open to a thin compromise where we keep the exact pins in the persisted result identity and aggregation key, but only show them compactly as an Evaluation Stack cell on the dashboard?

The release remains the canonical description, while the abbreviated BC-Bench, BC-ALAgents, and BCQuality identities save analysts from reconstructing the transitive pins for each row. This also prevents accidental aggregation if results are produced from different commits or experimental pins under the same in-development benchmark version.

dayland Good point, you pointed out a very valid gap in the current design:

we can't really experiment with agent harness version.

We never had that need before PR Review Agent, after some thought, what do you think about this #860 ? Could it be an alternative to what we are trying to achieve here? Or at least partly

Use the generic agent_version field for BC-ALAgents, retain transitive BCQuality provenance, and pin the merged diagnostics support.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
@dayland
dayland changed the base branch from main to feature/agent-version-identity September 8, 2026 08:46
Base automatically changed from feature/agent-version-identity to main September 8, 2026 08:52
Keep transitive PR Review provenance on the advanced metrics page while using the shared benchmark version presentation on default leaderboards.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 57308020-6803-4e62-b08e-57fa875bff89
@dayland
dayland marked this pull request as ready for review September 8, 2026 09:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Missing engine findings artifacts currently fail evaluation and legacy usage_complete can be mis-aggregated as 0.0 instead of preserved as unavailable.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread src/bcbench/agent/pr_review/metrics.py
Comment thread src/bcbench/results/codereview.py Outdated
Comment thread src/bcbench/agent/pr_review/metrics.py Outdated

@gggdttt Wenjie Fan (gggdttt) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also accept knowledge-sample here? The pinned engine emits this when removing .good.al / .bad.al files from disabled layers. This happens with the default config too, since the community layer is disabled, so the parser will fail even after a successful review.

We should accept these records but keep counting only knowledge entries in knowledge_pruned

Treat missing findings and usage completeness as unavailable where appropriate, and accept the pinned engine's knowledge-sample filter records without counting them as pruned articles.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 08f0eec5-45eb-485a-b89d-d0de33568add
Preserve the advanced PR Review diagnostics on existing leaderboard runs while incorporating the latest main-branch runs and regenerated aggregates.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 08f0eec5-45eb-485a-b89d-d0de33568add

@haoranpb Sun Haoran (haoranpb) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, got caught up on livesite.

Looks good but please trigger a run with this branch before you merge to verify everything is working as expected

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/bcbench/agent/pr_review/metrics.py Outdated
Comment thread src/bcbench/agent/pr_review/metrics.py Outdated
Comment thread src/bcbench/agent/pr_review/metrics.py Outdated
Comment thread tests/test_pr_review_metrics.py Outdated
@haoranpb

Copy link
Copy Markdown
Collaborator

I see some lots of duplicated code with #881 , can you consolidate?

dayland-ms and others added 4 commits September 25, 2026 15:25
…-review-advanced-metrics

# Conflicts:
#	tests/test_pr_review_metrics_reporting.py
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 76eb42c4-2acd-4f9c-a898-f4a44f9d46f7
@dayland

dayland commented Sep 25, 2026

Copy link
Copy Markdown
Collaborator Author

I see some lots of duplicated code with #881 , can you consolidate?

This is on purpose. This PR will not merge until 881 goes in and then will only be the delta.

@dayland
dayland changed the base branch from main to experiment/live-pr-review-leaf-models September 25, 2026 14:53
@dayland
dayland added this pull request to stack #898 September 25, 2026 14:54
@haoranpb
Sun Haoran (haoranpb) requested a balanced review from Copilot September 29, 2026 08:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +318 to +321
copilot_cli_version: str | None = None
bcquality_repository: str | None = None
bcquality_commit: str | None = None
bcquality_version: str | None = None
Comment on lines +186 to +189
"copilot_cli_version": first_run.copilot_cli_version,
"bcquality_repository": first_run.bcquality_repository,
"bcquality_commit": first_run.bcquality_commit,
"bcquality_version": first_run.bcquality_version,
Comment on lines +245 to +249
"average_knowledge_files": mean_metric([run.average_knowledge_files for run in cr_runs]),
"average_knowledge_pruned": mean_metric([run.average_knowledge_pruned for run in cr_runs]),
"average_knowledge_used": mean_metric([run.average_knowledge_used for run in cr_runs]),
"average_knowledge_suppressed": mean_metric([run.average_knowledge_suppressed for run in cr_runs]),
"average_sub_skills_executed": mean_metric([run.average_sub_skills_executed for run in cr_runs]),

@haoranpb Sun Haoran (haoranpb) left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a step back, and thinking from the general archetecture view:

Should all of the calculated metrics be calculated by PR Review Agent instead?

So it remains a black box to BC-Bench, and we just read manifest just like other metrics? It probably doesn't make sense for BC-Bench to know the folder structure of BCQuality, what do you think?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants