Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6a73db23-7e6d-4e87-a162-f7f16374bc44
Copilot stopped reviewing on behalf of
Sun Haoran (haoranpb) due to an error
September 24, 2026 16:17
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
Review effort: Lite
Findings: 1
Open (3)
runtime_mismatches()will treat these fields as “don’t care” whenever the runtime evidence is… · Newmin_severityparameter influencesseverity(used forAGENT_MINIMUM_SEVERITY) but does not… · Newstr, while the rest of the PR review pipeline constrains… · New
What changed in this PR
Adds first-class “named definitions” for BC PR Review runs so that only runs matching a fully validated, release-owned configuration are labeled/published as the official baseline, and surfaces that definition metadata throughout results and dashboards.
Changes:
- Introduces a PR Review definition registry and validation (requested definition + runtime evidence resolution).
- Propagates definition metadata and additional PR Review provenance fields into metrics, result summaries, leaderboard aggregates, and dashboards.
- Updates CLI/workflow inputs to select a definition (or mark runs as unclassified to avoid publishing) and expands tests accordingly.
| File | Description |
|---|---|
| tests/test_review_workflows.py | Asserts workflow exposes definition-id and that publishing is suppressed for non-baseline definitions. |
| tests/test_review_runners.py | Updates expected model/defaults and verifies definition_id is passed into the runner. |
| tests/test_pr_review_metrics_reporting.py | Extends provenance propagation tests and validates definition identity behavior in combination keys/aggregation. |
| tests/test_pr_review_metrics.py | Updates manifest fixture structure (adds engine) and new PR review config fields. |
| tests/test_pr_review_definitions.py | Adds test coverage for definition resolution/validation against a canonical run manifest. |
| tests/test_pr_review_agent.py | Verifies env now includes MINIMUM_SEVERITY. |
| src/bcbench/types.py | Extends PRReviewMetrics with definition metadata + additional PR review config fields. |
| src/bcbench/results/leaderboard.py | Adds PR review provenance fields to code-review aggregates and validates definition metadata. |
| src/bcbench/results/codereview.py | Persists/validates definition metadata in results/summaries and includes it in combination identity. |
| src/bcbench/commands/evaluate.py | Adds --definition-id option for evaluate pr-review and updates default model. |
| src/bcbench/agent/pr_review/metrics.py | Resolves a named definition from validated runtime evidence and records it in metrics. |
| src/bcbench/agent/pr_review/definitions.py | Implements the definition registry, resolution, and metadata validation rules. |
| src/bcbench/agent/pr_review/agent.py | Validates requested definitions pre-run and enforces post-run evidence resolves to the requested definition. |
| docs/code-review.md | Displays definition name on the main dashboard (fallback to model) and documents the registry semantics. |
| docs/code-review-details.md | Expands “Advanced Metrics” tables with definition/provenance columns. |
| CATEGORIES.md | Documents the intended boundary: definition metadata stays category-owned (not universal schemas). |
| .github/workflows/pr-review-evaluation.yml | Adds definition-id workflow input, threads it through execution/requeue, and blocks publishing for non-baseline definitions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+63
to
+74
| optional_values = { | ||
| "bcquality_repository": (bcquality_repository, self.bcquality_repository), | ||
| "bcquality_commit": (bcquality_commit, self.bcquality_commit), | ||
| "bcquality_source_snapshot": (bcquality_source_snapshot, self.bcquality_source_snapshot), | ||
| "cli_timeout_minutes": (cli_timeout_minutes, self.cli_timeout_minutes), | ||
| "minimum_severity": (minimum_severity, self.minimum_severity), | ||
| "agent_minimum_severity": (agent_minimum_severity, self.agent_minimum_severity), | ||
| "review_source": (review_source, self.review_source), | ||
| } | ||
| mismatches = [f"{name}={actual_values[name]!r} (expected {expected!r})" for name, expected in expected_values.items() if actual_values[name] != expected] | ||
| mismatches.extend(f"{name}={actual!r} (expected {expected!r})" for name, (actual, expected) in optional_values.items() if actual is not None and actual != expected) | ||
| return mismatches |
| "COPILOT_REVIEW_LEAF_MODEL": leaf_model, | ||
| "COPILOT_REVIEW_LEAF_EXECUTION": leaf_execution, | ||
| "COPILOT_REVIEW_MAX_LEAF_CONCURRENCY": str(max_leaf_concurrency), | ||
| "MINIMUM_SEVERITY": settings["min_severity"], |
Comment on lines
+25
to
+28
| cli_timeout_minutes: int = Field(ge=0) | ||
| minimum_severity: str | ||
| agent_minimum_severity: str | ||
| review_source: str |
dayland
added this pull request to stack #898
September 25, 2026 14:54
Collaborator
|
dayland Oops, adding to stack caused merge conflict |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture
Adds a release-owned BC PR Review definition registry. A definition is derived only when the validated run manifest and resolved BCQuality identity exactly match the registered root/leaf models, serial schedule and declared concurrency, engine pin, Copilot CLI pin, BCQuality commit/source snapshot, timeout, severity policy, and review source.
The canonical
pr-review-production-sol-luna-serial-v1definition is based on the successful evaluation-identity manifest from workflow run 36001653480. Unknown or mismatched evidence remains unclassified; callers cannot provide an arbitrary display label.Schema boundaries
definition_idanddefinition_nameare persisted only through PR Review metrics and Code Review result, summary, and aggregate models. No sharedBaseEvaluationResult,EvaluationResultSummary,LeaderboardAggregate, or generic metrics profile schema is added. Code Review aggregation now includes the immutable definition ID when present, so named, differing, and legacy/unclassified rows cannot combine.Dashboard and workflow
The default Code Review dashboard displays a validated definition name and otherwise falls back to
model, preserving historical rows. The advanced view adds readable definition ID plus root/leaf, scheduling, policy, engine, CLI, and BCQuality provenance. The PR Review workflow defaults to the canonical definition, validates inputs against it before engine execution, and marks unclassified/nonbaseline definitions as non-publishing experiments.Validation
uv run pytest— 1,123 selected tests passedty check, Ruff, workflow YAML parsing, andgit diff --checkpassedsynthetic__security-001; its artifact resolved the canonical definition and did not publish resultsMigration and dependencies
Historical Code Review JSON without definition fields continues to deserialize and display its existing model. This is a stacked follow-up to #851 and assumes #851 plus #881 merge first; it does not modify either prerequisite PR. Future BCal and bug-fix definitions remain category-owned follow-ups.