ci: define high-risk pre-merge review - #455
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are internally consistent across tooling/tests/docs/templates and introduce no runtime or workflow behavior changes.
Pull request overview
Adds a durable “high-risk pre-merge review” contract to contributor tooling and documentation, introducing a lightweight classifier plus explicit high-risk rows in the test matrix so regression-sensitive PRs have a consistent pre-mark-ready evidence checklist.
Changes:
- Added a read-only path classifier (
classify_high_risk_changes.dart) that identifies high-risk surfaces from changed file paths. - Extended the contributor test matrix with a new
high-risktier and two new review/evidence rows, with matching unit tests. - Updated contributor docs, PR template, AGENTS policy, and CODEOWNERS to document and protect the new governance/policy entrypoints.
File summaries
| File | Description |
|---|---|
| tool/testing/test_matrix.dart | Introduces high-risk tier plus two high-risk matrix rows; updates tier filtering/help text accordingly. |
| tool/testing/classify_high_risk_changes.dart | New read-only classifier for changed paths that reports whether a PR should follow high-risk pre-merge review. |
| test/unit/tooling/test_matrix_test.dart | Adds coverage asserting the high-risk matrix tier renders expected rows/content. |
| test/unit/tooling/classify_high_risk_changes_test.dart | Adds unit tests validating classifier surface detection and deterministic formatting. |
| doc/testing_matrix.md | Documents the new high-risk tier and the required pre-mark-ready independent QA expectations. |
| AGENTS.md | Records the durable high-risk review policy and evidence expectations for structured-output and other sensitive surfaces. |
| .github/pull_request_template.md | Adds a structured “High-risk regression review” block for consistent PR evidence capture. |
| .github/CODEOWNERS | Protects AGENTS/test-matrix/classifier policy entrypoints via CODEOWNERS. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The new classifier script can mis-handle input decoding and whitespace-only stdin, leading to incorrect or misleading classification output.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
tool/testing/classify_high_risk_changes.dart:170
- Reading stdin with SystemEncoding can mis-decode
git diff --name-onlyoutput on systems where the console code page isn’t UTF-8, which can lead to incorrect path classification for non-ASCII filenames. Usingutf8.decoderis deterministic and matches Git’s typical UTF-8 output.
Future<List<String>> _readPaths() async {
return stdin
.transform(const SystemEncoding().decoder)
.transform(const LineSplitter())
.toList();
tool/testing/classify_high_risk_changes.dart:189
- If stdin contains only blank/whitespace lines,
paths.isEmptyis false butassessHighRiskFiles()will classify an emptychangedFilesset as standard risk, which is misleading (no usable input was provided). Consider failing with usage when no non-empty paths remain after normalization.
final paths = await _readPaths();
if (paths.isEmpty) {
stderr.write('No changed paths were provided.\n${_usage()}');
exitCode = 64;
return;
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
The new policy contract tests/docs have small but meaningful gaps (missing enforced PR-template field and missing CODEOWNERS assertions, plus a potentially misleading origin/main example) that should be tightened to make the intended governance durable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
Changes are limited to policy/tooling/docs with unit-test enforcement, and the only noted issue is a small doc-comment accuracy nit.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tool/testing/classify_high_risk_changes.dart:22
- The
changedFilesdoc comment says the paths are “Normalized”, but the implementation only trims whitespace and filters empty lines; it does not normalize prefixes like./(or other common variants). Either implement normalization or adjust the comment so it matches actual behavior.
/// Normalized, non-empty paths inspected by the classifier.
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It updates repository governance and merge-readiness contracts (docs/template/CODEOWNERS + new policy tooling), which warrants final human maintainer review even though no runtime behavior changes are present.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🟢 Approval recommended
The changes are policy/tooling-only and include concrete, automated contract tests (plus CODEOWNERS protections) that make the new high-risk review requirements durable and hard to regress inadvertently.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Part of #419.
This is the clean policy/bootstrap replacement for the mixed implementation in #420. Keep #419 open: protected GitHub App status publication and repository settings remain a separate security-sensitive phase.
Scope
Removed duplication
Validation
Compatibility
Policy and contributor-tooling only. Runtime behavior and existing required checks are unchanged.
High-risk regression review