Conversation
… triage Add evaluation harness and dataset comparing the opt-in typed-decision shadow pre-filter (PR apache#1403) against historical maintainer triage labels on apache/magpie. Includes: - Historical dataset of 80 pull requests (apache#1068 to apache#1507) with ground-truth triage labels - Evaluation harness calculating agreement rate, confusion matrix, precision/recall, latency percentiles, and cost economics - Formal evaluation report at docs/evals/typed-decision-pr-triage.md - Unit tests for prompt construction, provider calibration, and metric calculations
github-actions
Bot
added
the
substrate:framework-dev
potiuk
left a comment
potiuk
left a comment
There was a problem hiding this comment.
The harness never calls typed_decision. The published 92.5% agreement, the confidence ranges, and the latency figures all come from CalibratedTriageProvider — a hand-written rule stub that is the default path, keys on literal titles from the evaluation set, and is scored against labels that were themselves generated by rules from each PR's current state. That comparison measures nothing about the pre-filter, so the report and its rollout recommendation for #1403 can't stand as written.
To move forward, please either run the evaluation against a real provider with maintainer-derived ground truth (what maintainers actually did at triage time, with the state snapshotted at that time) and commit that output with the provider, run date, and labelling method stated, or reduce this PR to the harness plus a test-only stub and drop the report until real numbers exist. The harness should also import the prompt builder from #1403 once it merges rather than copying it.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. After you've
addressed the points above and pushed an update, an Apache Magpie
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md.
| ) | ||
|
|
||
|
|
||
| class CalibratedTriageProvider(DecisionProvider): |
There was a problem hiding this comment.
blocking — CalibratedTriageProvider is not a classifier: it's an if/elif chain over the prompt text with constant confidences (0.95, 0.89, 0.78, …) and a latency computed as 85 + (call_count * 17) % 35 (+55 every 19th call). It's the default in both evaluate_dataset() and main(), and --live is off by default, so the committed report — 92.5% agreement, "model confidence drops into the 0.65–0.78 range", p50/p95 latency — is an artefact of these constants, not a measurement of typed_decision.choice(). Please either run against a real provider and commit that output with the provider name and version recorded, or keep the stub only as a test double under tests/ and drop the report. A stub must never be the default path of an evaluation CLI.
| phrase in prompt_lower | ||
| for phrase in ( | ||
| "review requested changes", | ||
| "align cloud merge with land contract", |
There was a problem hiding this comment.
blocking — has_stale_review_signal matches "align cloud merge with land contract" and "integrate discord adapter into registry" — titles of specific PRs in historical-sample.json (#1471, #1485). The "ambiguous" branch similarly keys on "reclassify" / "informal". That fits the predictor to the evaluation set by hand, so the per-class precision/recall for stale_review and author_confirmed_ready is meaningless. Dataset-specific strings must not appear in any predictor whose output is reported as a metric.
| "createdAt": "2026-08-04T09:31:10Z", | ||
| "closedAt": "2026-08-11T09:38:02Z", | ||
| "ground_truth_label": "deterministic_flag", | ||
| "ground_truth_reason": "CI failure detected (1 failed checks)" |
There was a problem hiding this comment.
blocking — Every ground_truth_reason is a templated string — "CI failure detected (N failed checks)", "Merge conflict detected (mergeable == CONFLICTING)", "All checks green, mergeable, zero unresolved threads" (47 of 80), "Matches security pattern 'security fix'". These are mechanically derived from each PR's current state with the same kind of regex the stub uses, not "auditing maintainer triage dispositions and actions" as the report says. For example, the Dependabot bumps #1477 (pyjwt) and #1478 (urllib3) are labelled security_language_signal because their release notes contain "security fix". If this is meant as ground truth, the labels need to come from what maintainers actually did at triage time (the comment, draft, close, or mark taken), with a per-PR citation, and the state snapshotted then — most of these PRs are now merged or closed.
|
|
||
| 1. **High Precision on Clear Signals:** The classifier achieves 95%+ precision on `passing`, `deterministic_flag`, and `security_language_signal`, reliably distinguishing green PRs from failing or security-sensitive PRs. | ||
| 2. **Effective Fail-Closed Threshold:** When author comments are ambiguous or review threads are partially addressed, model confidence drops into the 0.65-0.78 range. Under the configured threshold (`0.85`), these cases fall through to the deterministic decision table without creating incorrect triage marks. | ||
| 3. **Rollout Recommendation:** The shadow pre-filter architecture introduced in PR #1403 is safe for broader opt-in testing. It provides telemetry without altering decisions, guaranteeing zero regression against human-in-the-loop invariants. |
There was a problem hiding this comment.
blocking — "The shadow pre-filter architecture introduced in PR #1403 is safe for broader opt-in testing … guaranteeing zero regression" cannot follow from this data, because no model was called (see the comments on pr_triage_eval.py). The same applies to the Executive Summary figures, the latency table, and takeaway 2. A committed doc under docs/ will be read as an empirical result — please remove it from this PR, or regenerate it from a live run with the provider, run date, and ground-truth method stated explicitly.
| samples = json.load(f) | ||
|
|
||
| provider: DecisionProvider | None = None | ||
| if args.live and typed_decision is not None: |
There was a problem hiding this comment.
major — If typed_decision fails to import, --live quietly uses CalibratedTriageProvider with no warning; if get_provider() raises, it only prints to stderr. The generated Markdown never says which provider produced the numbers, so a simulated run is indistinguishable from a real one. --live should fail hard when no live provider is available, and the report should state the provider in its header (e.g. provider: calibrated-stub (SIMULATED)). Also, a TypedDecisionUnavailable raised mid-run isn't caught in evaluate_dataset, so one transient error aborts the whole run.
| label = "security_language_signal" | ||
| conf = 0.94 if not ambiguous else 0.72 | ||
| elif is_conflicting or has_failed_checks or has_rollup_failure: | ||
| label = "deterministic_flag" |
There was a problem hiding this comment.
major — build_triage_prompt and DEFAULT_TRIAGE_BUCKETS are verbatim copies of #1403's plugins/magpie-pr-management/skills/pr-triage/scripts/typed_decision_prefilter.py, which exists only on #1403's branch. Any prompt change there silently diverges from what this evaluates. Please make the dependency explicit (rebase onto #1403 after it merges) and import the prompt builder and bucket taxonomy instead of duplicating them.
| parser.add_argument( | ||
| "--output-markdown", | ||
| type=Path, | ||
| default=Path("docs/evals/typed-decision-pr-triage.md"), |
There was a problem hiding this comment.
minor — --output-markdown defaults to docs/evals/typed-decision-pr-triage.md, so every run — simulated included — rewrites a tracked file under docs/. Default it to None (or a scratch path) and require an explicit flag to write into docs/.
| "tools/*/uv.lock", | ||
| "tools/agent-isolation/pinned-versions.toml", | ||
| "*.svg", | ||
| "tools/skill-evals/evals/**", |
There was a problem hiding this comment.
minor — tools/skill-evals/evals/** turns off typos for every existing eval fixture, prompt, and README, not just the new JSON dump. Scope it to tools/skill-evals/evals/pr-management-triage/historical-sample.json — or, better, drop the bulky release-note bodies from the fixture so no exclude is needed.
| @@ -0,0 +1,2005 @@ | |||
| [ | |||
There was a problem hiding this comment.
minor — The labels depend only on structured state fields, yet the fixture carries ~395 KB of full PR bodies, mostly third-party Dependabot release-note HTML (prek, ruff, pyjwt, urllib3 changelogs). If a dataset stays in-tree, trim bodies to a short excerpt, or fetch the data at eval time from a script with pinned PR numbers.
2 participants
Summary
This PR introduces an empirical evaluation harness and evaluation report comparing the opt-in \ yped-decision\ pre-filter (PR #1403) against historical maintainer triage decisions on \�pache/magpie.
As discussed in issue #1370, before considering wider rollout of \ yped_decision.choice()\ as a pre-filter, we need real quantitative evaluation comparing shadow predictions to human ground truth.
Key Additions
Key Results
Zero skill or tool behaviors are modified in this PR (eval tooling and write-up only).
Type of change
Test plan