Apply cross-run audit clustering to graders and evals - #56146
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds grader and eval signals to cross-run audit clustering.
Changes:
- Derives grader outcome and eval-presence labels from run artifacts.
- Adds
gradersandevalsclustering dimensions. - Adds cluster-count coverage for the new dimensions.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/logs_orchestrator_render.go |
Derives grader and eval cluster values. |
pkg/cli/audit_cross_run.go |
Stores new cluster inputs. |
pkg/cli/audit_cross_run_clusters.go |
Builds grader and eval clusters. |
pkg/cli/audit_cross_run_clusters_test.go |
Tests grouping and counts. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| if graders.ErrorCount > 0 { | ||
| return "error" | ||
| } | ||
| if graders.Failed > 0 { | ||
| return "fail" | ||
| } | ||
| if graders.UnavailableCount > 0 { | ||
| return "unavailable" | ||
| } | ||
| if graders.Passed == graders.Total { | ||
| return "pass" | ||
| } | ||
| return "mixed" |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new grader/eval clustering is headed in the right direction, but the grader bucketing logic currently collapses mixed outcomes into fail, which throws away the distinction this PR claims to add and makes the new cohort analysis misleading.
Blocking theme
deriveGradersClusterValueuses priority checks that makemixedunreachable for common multi-status runs, so partial-pass / partial-fail cohorts are silently misclassified.- The added test coverage only exercises pure
passand purefailinputs, so the broken mixed-state behavior is not protected.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 12.9 AIC · ⌖ 8.1 AIC · ⊞ 4.6K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/logs_orchestrator_render.go:167
The mixed grader cluster is effectively dead code here: any run with both passing and failing grader results is classified as fail (and unavailable is also swallowed by fail), so the new clustering loses the distinction between partial failure and a pure fail cohort.
<details><summary>💡 Why this blocks merge</summary>
countGraderStatuses produces independent counters, but deriveGradersClusterValue short-circuits on Failed > 0 before checking whether the run also had passes or u…
There was a problem hiding this comment.
Review: Apply cross-run audit clustering to graders and evals
The refactoring is clean — extracting the inline cluster-building into buildRunClusters improves readability and makes extending dimensions straightforward. The new graders and evals clustering dimensions follow the established pattern consistently.
One pre-existing inline comment (line 222, logs_orchestrator_render.go) flags a potential logic issue in deriveGradersClusterValue: if countGraderStatuses assigns every result to exactly one counter, the "mixed" branch may be unreachable. Worth verifying or adding a test case that exercises the "mixed" path to confirm the "mixed" return is reachable before merging.
No other blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 18.6 AIC · ⌖ 9.06 AIC · ⊞ 6.2K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /diagnosing-bugs, and /codebase-design — requesting changes on one correctness issue and two supporting improvements.
📋 Key Themes & Highlights
Key Themes
mixedcluster unreachable (correctness):deriveGradersClusterValueuses sequential> 0guards; a run with errors is returned as"error"before the"fail"or"unavailable"checks run, and"mixed"is never reached for any combination of counters withPassed < Total. See existing inline comment on line 222 and the new comment on line 209.- Derivation logic untested: The new test exercises
buildClusterAnalysiswith pre-filledGradersClusterfields, soderiveGradersClusterValueis never exercised by the test suite. Edge cases (mixed,error+fail,nil graders with zero Total) are uncovered. - Magic boolean:
runHasEvals(logsPath, false)buries intent; a named constant or wrapper removes ambiguity.
Positive Highlights
- ✅ Clean extraction of cluster-building into
buildRunClusters— easy to extend. - ✅
absentfallback for empty strings is consistent across both new dimensions. - ✅ Good test structure: parallel, named
RunIDs, explicit cluster lookup loop.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 31.2 AIC · ⌖ 10.2 AIC · ⊞ 7.6K
Comment /matt to run again
| @@ -49,6 +49,48 @@ func TestBuildClusterAnalysis_ClustersConclusion(t *testing.T) { | |||
| assert.Contains(t, failureCluster.RunIDs, int64(3)) | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/tdd] The test sets GradersCluster directly on the struct, bypassing deriveGradersClusterValue entirely — so the derivation logic (including the mixed-reachability bug in logs_orchestrator_render.go) has no unit test coverage.
💡 Add a focused unit test for `deriveGradersClusterValue`
A direct unit test would have caught the > 0 guard that makes mixed unreachable. The cluster-analysis test only exercises the grouping; it never exercises the derivation path.
@copilot please address this.
| graders := extractGradersData(logsPath) | ||
| if graders == nil || graders.Total == 0 { | ||
| return "absent" | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The priority ordering here (error → fail → unavailable → pass) means a run with both failures and unavailable graders returns "fail" and never "unavailable". If the intent is a strict precedence, document it; if not, consider returning "mixed" when multiple non-passing states are present.
💡 Suggested fix for mixed-state detection
nonPassCount := graders.ErrorCount + graders.Failed + graders.UnavailableCount
if nonPassCount > 1 {
return "mixed"
}
if graders.ErrorCount > 0 {
return "error"
}
if graders.Failed > 0 {
return "fail"
}
if graders.UnavailableCount > 0 {
return "unavailable"
}
return "pass"Alternatively, add a comment explaining the intended precedence so future readers don't change the order inadvertently.
@copilot please address this.
| func deriveEvalsClusterValue(logsPath string) string { | ||
| if runHasEvals(logsPath, false) { | ||
| return "present" | ||
| } |
There was a problem hiding this comment.
[/codebase-design] deriveEvalsClusterValue calls runHasEvals(logsPath, false) with a hard-coded false argument, but the boolean's meaning is not obvious at the call site. Consider naming the parameter or using a named constant so the intent is self-documenting.
💡 Example
const includePending = false
if runHasEvals(logsPath, includePending) {Or, if false is the only ever-needed value here, a wrapper runHasCompletedEvals(logsPath) removes the ambiguity entirely.
@copilot please address this.
…sions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (112 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
One small helper is just a wrapper over six cluster appends and has one caller; inlining it would keep the logic local. net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #56146 · codex · mai10 · 6.02 AIC · ⌖ 3.21 AIC · ⊞ 16.7K
Comment /ponytail to run again
| } | ||
| } | ||
|
|
||
| func buildRunClusters(inputs []crossRunInput) []RunCluster { |
There was a problem hiding this comment.
pkg/cli/audit_cross_run_clusters.go:94: yagni: buildRunClusters is just a thin wrapper around six one-line buildDimensionClusters calls and has one caller. Inline those appends in buildClusterAnalysis and keep the logic local.
|
@copilot PR #56146 still needs follow-up.
|
…erogeneous runs Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding review: |
Cross-run
gh aw auditclustering previously covered runtime/task dimensions but omitted grader and eval signals, making multi-run cohort analysis incomplete. This update adds first-class clustering for both so grader outcomes and eval availability are grouped alongside existing dimensions.Cross-run input enrichment
GradersClusterandEvalsClusterfields tocrossRunInput.processedRunsToCrossRunInputs(...)from run artifacts/log paths.New clustering dimensions
buildClusterAnalysis(...)to include:gradersdimension (e.g.,pass,fail,error,unavailable,absent,mixed)evalsdimension (present/absent)Derivation logic
deriveGradersClusterValue(logsPath)using extracted grader summary state.deriveEvalsClusterValue(logsPath)using eval artifact presence (evals.jsonldetection path).Coverage