Unify safe-output outcome status enums - #54846
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. This PR mechanically consolidates two duplicate outcome-classification enums (OutcomeResult and OutcomeStatus) into one; it removes duplication rather than adding it, and introduces no new abstractions, dependencies, or speculative flexibility. Nothing to cut.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Pull request overview
Consolidates safe-output classification onto OutcomeStatus, resolving #54528.
Changes:
- Removes
OutcomeResultandOutcomeReport.Result. - Migrates evaluators, summaries, rendering, and domain metrics.
- Updates serialization and regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/outcomes_command.go |
Uses unified status in CLI output. |
pkg/cli/outcome_evaluation.go |
Adds error status and normalization. |
pkg/cli/outcome_eval.go |
Removes duplicate result model. |
pkg/cli/outcome_eval_workflow.go |
Migrates workflow evaluators. |
pkg/cli/outcome_eval_workflow_test.go |
Updates workflow assertions. |
pkg/cli/outcome_eval_update.go |
Migrates update evaluators. |
pkg/cli/outcome_eval_update_test.go |
Removes duplicate assertions. |
pkg/cli/outcome_eval_test.go |
Tests unified serialization and summaries. |
pkg/cli/outcome_eval_review.go |
Migrates review evaluators. |
pkg/cli/outcome_eval_pr.go |
Migrates pull-request evaluation. |
pkg/cli/outcome_eval_label.go |
Migrates label evaluation. |
pkg/cli/outcome_eval_jsonl.go |
Removes JSONL result field. |
pkg/cli/outcome_eval_issue.go |
Migrates issue evaluation. |
pkg/cli/outcome_eval_generic.go |
Migrates generic evaluators. |
pkg/cli/outcome_eval_formal_test.go |
Updates formal status invariants. |
pkg/cli/outcome_eval_comment.go |
Migrates comment evaluation. |
pkg/cli/outcome_eval_agent.go |
Migrates agent evaluation. |
pkg/cli/outcome_domain_breakdown.go |
Uses unified status for aggregation. |
pkg/cli/outcome_domain_breakdown_test.go |
Updates domain test fixtures. |
Review details
- Files reviewed: 19/19 changed files
- Comments generated: 2
- Review effort level: Balanced
| "run_id": runID, | ||
| "type": r.Type, | ||
| "result": r.Result, | ||
| "outcome_status": eval.OutcomeStatus, |
|
|
||
| // OutcomeReport is the result of evaluating one safe output item. | ||
| type OutcomeReport struct { | ||
| OutcomeEvaluation |
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
This refactor mostly looks mechanical, but it leaves the public CLI reference out of sync with the code: pkg/cli/README.md still documents the removed OutcomeResult API and old serialized result field.
Blocking theme
- The implementation now exposes only
OutcomeStatus/outcome_status. - The checked-in docs still advertise
OutcomeResult,OutcomeAccepted,OutcomeRejected, and related legacy constants. - That makes the published reference materially wrong for anyone integrating against this output model.
Please update/regenerate the README in the same change so the documented API matches the shipped one.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 17.2 AIC · ⌖ 6.94 AIC · ⊞ 4.6K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — requesting changes for two issues found.
📋 Key Themes & Highlights
Key Themes
- Enum coverage gap:
validResultsinTestFormalOTelGracefulDegradationomitsOutcomeStatusSkipped, diverging from the canonical domain set inTestFormalOutcomeDomainInvariant. A workflow that produces a skipped report would incorrectly fail the P11 invariant assertion. - Stale log key:
result=%sinnormalizeOutcomeEvaluationrefers to the now-removedOutcomeResultfield; the key should bestatus=%sfor accurate debug output.
Positive Highlights
- ✅ Clean removal of
OutcomeResultand the dual-field ambiguity — the unifiedOutcomeStatusenum is much easier to reason about. - ✅ Strong formal test coverage (TLA+ predicates, domain invariant) updated throughout.
- ✅ The
normalizeOutcomeEvaluationchange to returnOutcomeStatusErrorinstead ofOutcomeStatusUnknownfor error paths is a correctness improvement — callers can now distinguish evaluation errors from genuinely unknown outcomes. - ✅ JSON serialization cleanup (
resultfield removed) prevents downstream consumers from relying on the now-ambiguous duplicate field.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 62.4 AIC · ⌖ 10 AIC · ⊞ 7.6K
Comment /matt to run again
| OutcomePending: true, OutcomeLifecycle: true, OutcomeLifecycleClose: true, OutcomeUnknown: true, OutcomeError: true, | ||
| validResults := map[OutcomeStatus]bool{ | ||
| OutcomeStatusAccepted: true, OutcomeStatusRejected: true, OutcomeStatusIgnored: true, | ||
| OutcomeStatusPending: true, OutcomeStatusLifecycle: true, OutcomeStatusLifecycleClose: true, OutcomeStatusUnknown: true, OutcomeStatusError: true, |
There was a problem hiding this comment.
[/codebase-design] validResults is missing OutcomeStatusSkipped, creating a gap between this invariant check and the domain-invariant test at line 58 that includes it. A report with OutcomeStatusSkipped would fail the P11 assertion here.
💡 Suggested fix
validResults := map[OutcomeStatus]bool{
OutcomeStatusAccepted: true, OutcomeStatusRejected: true, OutcomeStatusIgnored: true,
OutcomeStatusPending: true, OutcomeStatusLifecycle: true, OutcomeStatusLifecycleClose: true,
OutcomeStatusUnknown: true, OutcomeStatusError: true, OutcomeStatusSkipped: true,
}The TestFormalOutcomeDomainInvariant test (line 58) now includes OutcomeStatusSkipped in the canonical set; validResults here should match.
@copilot please address this.
| } | ||
|
|
||
| outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, result=%s, detail=%q", report.Type, report.Result, report.Detail) | ||
| outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, result=%s, detail=%q", report.Type, report.OutcomeStatus, report.Detail) |
There was a problem hiding this comment.
[/codebase-design] The log format key result=%s is a stale reference to the removed OutcomeResult type. It should be status=%s to match the renamed field and avoid confusion when reading debug logs.
💡 Suggested fix
outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, status=%s, detail=%q", report.Type, report.OutcomeStatus, report.Detail)@copilot please address this.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (322 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.
Review: Unify safe-output outcome status enums
Good consolidation — removing the parallel OutcomeResult enum and collapsing everything onto OutcomeStatus+OutcomeEvaluation eliminates a long-standing ambiguity about which field represented the authoritative outcome. The migration is consistent across all evaluators, and the tests are properly updated.
Non-blocking issues found
Two log format-string verb labels were not updated to match the new field name. Both files still use result=%s while passing report.OutcomeStatus as the argument:
pkg/cli/outcome_eval_label.go:177—"Label evaluation result: result=%s, label_count=%d"pkg/cli/outcome_evaluation.go:48—"Normalizing outcome from heuristics: type=%s, result=%s, detail=%q"
The value is correct but the key name is misleading and inconsistent. Inline comments added on both lines.
Everything else looks good
OutcomeStatusErrorcorrectly added to the enum and handled innormalizeOutcomeEvaluation.normalizeOutcomeEvaluationearly-exit guard (OutcomeStatus != "" && EvidenceStrength != "") is correct — a pre-filledOutcomeEvaluationshort-circuits heuristics as intended.- Redundant
report.OutcomeStatus = ...assignments before explicitreport.OutcomeEvaluation = OutcomeEvaluation{...}blocks are intentional (keeping the field in sync) and not harmful. - JSONL writer correctly dropped the old
resultfield. - Formal invariant test updated to include
OutcomeStatusSkippedandOutcomeStatusError.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 90 AIC · ⌖ 9.22 AIC · ⊞ 6.2K
| } | ||
|
|
||
| outcomeEvalLabelLog.Printf("Label evaluation result: result=%s, label_count=%d", report.Result, len(labels)) | ||
| outcomeEvalLabelLog.Printf("Label evaluation result: result=%s, label_count=%d", report.OutcomeStatus, len(labels)) |
There was a problem hiding this comment.
The format-string verb label result= is stale after the OutcomeResult → OutcomeStatus rename. The argument is now report.OutcomeStatus, so the key should be outcome_status= to match:
outcomeEvalLabelLog.Printf("Label evaluation result: outcome_status=%s, label_count=%d", report.OutcomeStatus, len(labels))Leaving it as result= means log consumers / grep patterns looking for outcome_status= won't find this entry. @copilot please address this.
| } | ||
|
|
||
| outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, result=%s, detail=%q", report.Type, report.Result, report.Detail) | ||
| outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, result=%s, detail=%q", report.Type, report.OutcomeStatus, report.Detail) |
There was a problem hiding this comment.
The format-string verb label result= is stale after the OutcomeResult → OutcomeStatus rename. The argument is now report.OutcomeStatus, so the key should be outcome_status=:
outcomeEvaluationLog.Printf("Normalizing outcome from heuristics: type=%s, outcome_status=%s, detail=%q", report.Type, report.OutcomeStatus, report.Detail)Leaving it as result= is misleading in logs and inconsistent with the unified naming. @copilot please address this.
…48-8b72-d747995ae1c5
|
🎉 This pull request is included in a new release. Release: |
pkg/clihad two parallel outcome classification enums (OutcomeResultandOutcomeStatus) andOutcomeReportcarried both, making safe-output evaluation ambiguous. This consolidates classification onto one status field.Unified outcome model
OutcomeResultandOutcomeReport.Result.OutcomeStatusErrorto the existingOutcomeStatusenum.OutcomeStatusSkippedin the unified enum.Updated evaluation flow
OutcomeReport.OutcomeStatus.OutcomeEvaluation.Cleaned serialized output
OutcomeReportJSON now exposes a single classification field:outcome_status.resultandoutcome_status.OutcomeReportand outcome JSONL do not emit a separateresultclassification.