Add deterministic experiment adjudication to analysis - #55039
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nt-adjudication Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Reviewed PR #55039 for over-engineering. The Go changes replace bare string constants with proper named types (ExperimentDecision, ExperimentDecisionReasonCode, ExperimentReadiness), which is idiomatic Go and reduces error-proneness rather than adding complexity. The workflow markdown changes remove duplicated statistical/guardrail logic from the reporting workflow in favor of consuming the new deterministic core decision, netting fewer lines overall. No unnecessary abstractions, speculative flexibility, or hand-rolled stdlib equivalents found. Lean already. Ship.
|
|
🧠 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
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
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 deterministic decision layer is a good direction, but the CLI path still hides one of the new core states from humans.
Blocking theme
- The new
readinesscontract is serialized in JSON and documented heavily, but the stderr renderer still only surfaces the legacy recommendation string. That makes the operator-facing CLI output lag behind the machine contract and obscures the newREADYvsCOLLECTINGdistinction this PR is trying to establish.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 29.6 AIC · ⌖ 6.91 AIC · ⊞ 7K
Comment /review to run again
Comments that could not be inline-anchored
pkg/cli/experiments_analyze_statistics.go:612
This change adds a new readiness field to the JSON contract, but the CLI stderr rendering still only prints the legacy Recommendation: line. That leaves humans unable to tell READY + INCONCLUSIVE apart from COLLECTING + EXTEND, which is exactly the distinction this PR is introducing and documenting.
<details><summary>💡 Why this matters and how to fix it</summary>
Right now printOneExperimentAnalysis() prints variant progress, the old recommendation, and then the decision. For a rea…
✅ Design Decision Gate — ADR VerifiedADR reviewed: ADR-54978: Deterministic Experiment Decision Engine — implementation aligns with the stated decision. Great work! 🏗️ 📋 Verification SummaryThe ADR describes introducing a pure, stateless The PR implementation matches on all key points:
|
Test Quality Sentinel 🧪Score: 86/100 ✅ Excellent SummaryThis PR adds deterministic experiment adjudication logic with comprehensive test coverage. 4 test files changed, introducing 3 new behavioral tests and enhancing 2 existing tests with new assertions that validate the new readiness and decision contracts. Coverage Breakdown
Test-to-Production RatioAll modified files show healthy ratios (below 2:1 threshold):
Detailed Test ClassificationExpand for per-test review1.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on two issues.
📋 Key Themes & Highlights
Key Themes
- Partial typing:
ExperimentDecisionandExperimentDecisionReasonCodeare now typed constants, butGuardrailStatus.Statusremains an untypedstring. The PR's stated goal is a stable machine-readable contract — guardrail status codes should follow the same pattern to complete that contract. - Degenerate test data:
TestComputeGraderBackedExperimentDecisionuses constant observation values (0.5 / 0.8), producing zero-variance samples. The test exercises an edge-case guard rather than the primary t-test code path.
Positive Highlights
- ✅ Clean promotion of decision and reason-code strings to typed constants — excellent API hygiene.
- ✅
ExperimentReadinessenum (COLLECTING/READY) gives consumers a stable gate without requiring p-value parsing. - ✅
report_actionvscore_decisionseparation in the daily-report workflow correctly isolates the interaction safety hold from the core adjudication — well-designed boundary. - ✅ Good test coverage added for JSON output, CLI rendering, and grader-backed decision paths.
- ✅
INCONCLUSIVEpreserved as a distinct outcome fromREJECT— important for downstream consumers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 59.5 AIC · ⌖ 11.3 AIC · ⊞ 7.6K
Comment /matt to run again
| func pendingGuardrailDecision(guardrails []GuardrailStatus) (string, string, bool) { | ||
| func pendingGuardrailDecision(guardrails []GuardrailStatus) (ExperimentDecisionReasonCode, string, bool) { | ||
| for _, guardrail := range guardrails { | ||
| if guardrail.Status == "insufficient_observations" || guardrail.Status == "missing" { |
There was a problem hiding this comment.
[/codebase-design] GuardrailStatus.Status comparisons still use raw string literals while the surrounding code was upgraded to typed constants — this is the main consistency gap the PR leaves unresolved.
💡 Suggested fix
Introduce a typed GuardrailStatusCode string type (parallel to ExperimentDecisionReasonCode) and replace the raw comparisons:
type GuardrailStatusCode string
const (
GuardrailStatusInsufficientObservations GuardrailStatusCode = "insufficient_observations"
GuardrailStatusMissing GuardrailStatusCode = "missing"
GuardrailStatusUnsupported GuardrailStatusCode = "unsupported"
GuardrailStatusFail GuardrailStatusCode = "fail"
GuardrailStatusPass GuardrailStatusCode = "pass"
)Then GuardrailStatus.Status becomes GuardrailStatusCode, and pendingGuardrailDecision/firstFailedGuardrail compare against the constants. The PR explicitly states its goal is a "stable machine-readable" contract — leaving guardrail status as untyped strings undermines that.
@copilot please address this.
| Direction: "higher_is_better", | ||
| ByVariant: map[string][]GraderMetricObservation{ | ||
| "control": { | ||
| {RunID: "1", Variant: "control", Value: 0.5}, |
There was a problem hiding this comment.
[/tdd] All observations in the new grader-backed decision test use constant values (0.5 / 0.8), producing zero variance per variant. A t-test on zero-variance data yields a degenerate result — the test passes incidentally via an edge-case guard, not by exercising the normal statistical path.
💡 What to do
Add a small amount of variance so the test exercises the real t-test code path:
"control": {
{RunID: "1", Variant: "control", Value: 0.48},
{RunID: "2", Variant: "control", Value: 0.50},
{RunID: "3", Variant: "control", Value: 0.52},
},
"candidate": {
{RunID: "4", Variant: "candidate", Value: 0.78},
{RunID: "5", Variant: "candidate", Value: 0.80},
{RunID: "6", Variant: "candidate", Value: 0.82},
},Also add a separate, intentional zero-variance case that asserts the expected fallback behaviour explicitly.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds deterministic experiment adjudication while preserving legacy recommendations.
Changes:
- Introduces typed readiness, decisions, and reason codes.
- Updates reporting to consume canonical decisions.
- Expands documentation and test coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/experiments_decision.go |
Defines and applies decision contracts. |
pkg/cli/experiments_decision_test.go |
Tests adjudication scenarios. |
pkg/cli/experiments_analyze_statistics.go |
Adds readiness and typed rendering. |
pkg/cli/experiments_analyze_statistics_test.go |
Verifies readiness JSON output. |
pkg/cli/experiments_grader_observations_test.go |
Tests grader-backed promotion. |
pkg/cli/experiments_render_test.go |
Tests decision rendering. |
docs/src/content/docs/experimental/experiments.md |
Documents decision stages and API. |
docs/src/content/docs/experimental/experiments-specification.md |
Extends normative decision requirements. |
.github/aw/experiments.md |
Updates guardrail guidance. |
.github/workflows/daily-experiment-report.md |
Consumes canonical core decisions. |
.github/workflows/daily-experiment-report.lock.yml |
Regenerates the compiled workflow. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
.github/workflows/daily-experiment-report.md:584
- A sparse-interaction hold changes
report_actiontoEXTEND, yet this rule still applies the additiveexperiment:concludedlabel for the underlyingPROMOTE. Because labels are not automatically removed, the issue is marked concluded while the report explicitly requests continued collection. Exclude held promotions from this label.
| `experiment:concluded` | Core `decision` is `PROMOTE` or `REJECT` |
docs/src/content/docs/experimental/experiments-specification.md:753
READYcan also pair withEXTEND: after the primary sample gate passes, missing/unsupported guardrails, an absent comparison, or an uncomputable analysis all produceEXTEND. Omitting that state makes the normative contract contradict both R-STAT-016 andDecideExperiment.
**R-STAT-018**: Structured analysis output MUST expose readiness independently from decision.
Readiness is `COLLECTING` until every variant reaches `min_samples`, then `READY`. `READY` does not
imply `PROMOTE`; a ready experiment may be `PROMOTE`, `REJECT`, or `INCONCLUSIVE`. The legacy
`recommendation` field MAY remain `EXTEND` / `READY_FOR_ANALYSIS` for backward compatibility.
- Files reviewed: 11/11 changed files
- Comments generated: 4
- Review effort level: Balanced
| comparison := decisionComparison(analysis.Comparisons) | ||
| if comparison == nil { | ||
| return result.withDecision(experimentDecisionExtend, "insufficient_observations", | ||
| return result.withDecision(ExperimentDecisionExtend, ExperimentDecisionReasonInsufficientObservations, |
| - `<metric_name>` expected `<threshold>`, got `<actual_value>` for variant `<variant>` | ||
|
|
||
| Recommendation: **ABANDON** — investigate immediately. | ||
| Decision: **REJECT** (`guardrail_failed`) — investigate immediately. |
| - `start_date:` / `end_date:` — ISO-8601 `YYYY-MM-DD`. Outside this window, control variant is returned and counters do not increment. | ||
| - `description:`, `metric:`, `issue:`, `hypothesis:` — governance metadata (no runtime effect). | ||
| - `guardrail_metrics:` — array; if any guardrail fails for any variant, experiment is auto-abandoned. Each entry: | ||
| - `guardrail_metrics:` — array; if any guardrail fails for any variant, the deterministic core decision is `REJECT`. Each entry: |
| **R-STAT-007**: Reporting tools **MUST NOT** issue a PROMOTE decision for any variant | ||
| until all variants in the experiment have accumulated at least `min_samples` runs (or 20 if | ||
| `min_samples` is not declared). When any variant is below threshold, the recommendation | ||
| **MUST** be EXTEND. | ||
| `min_samples` is not declared). When any variant is below threshold, readiness **MUST** be | ||
| `COLLECTING` and the decision **MUST** be `EXTEND`. |
There was a problem hiding this comment.
Review: Add deterministic experiment adjudication to analysis
This is a clean, well-scoped PR that makes two cohesive improvements:
1. Type-safe decision constants (Go)
Replacing untyped string constants (experimentDecisionExtend, etc.) with proper exported types (ExperimentDecision, ExperimentDecisionReasonCode) is a solid improvement. It catches mismatches at compile time and makes API consumers easier to read. The refactor is complete — no leftover raw string literals found.
2. Decoupled Readiness from Decision
Adding ExperimentReadiness as a separate field (COLLECTING/READY) distinct from decision correctly models R-STAT-018: a ready experiment can still be INCONCLUSIVE. The initialization to ExperimentReadinessCollecting in newExperimentAnalysis and the two-branch update in applyExperimentReadiness are correct and consistent.
Workflow prompt changes
The prompt correctly:
- Delegates canonical decisions to
gh aw experiments analyzerather than re-deriving them from p-values - Preserves the interaction safety hold as a presentation layer concern (
report_action: EXTEND) without mutatingcore_decision - Distinguishes
INCONCLUSIVEfromREJECTthroughout
Test coverage
New tests in experiments_grader_observations_test.go and experiments_render_test.go cover the grader-backed decision path and the decision rendering path with typed constants. The JSON output test for readiness: "COLLECTING" is a good regression guard.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 49.7 AIC · ⌖ 9.08 AIC · ⊞ 6.2K
|
Please address the newest blocking review feedback first:
After updating the branch, refresh with the latest base changes if GitHub offers it, rerun any needed checks, and then run the Run: https://github.com/github/gh-aw/actions/runs/32627776751
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the newest blocking reviews in cd85cbb: CLI output now renders readiness, guardrail statuses use typed constants, and grader-backed tests cover normal variance plus the explicit zero-variance fallback. The branch already contains the latest base commit; formatting, standalone lint, full Go tests, targeted tests, and workflow recompilation pass. |
Triage: pr-type:feature · pr-risk:medium · Score: 62/100
Recommended action: Adds deterministic experiment-adjudication logic across analysis/decision code with docs and lock-file regen; 11 files, +324/-198. Review history shows mixed signals: 2× CHANGES_REQUESTED and 3× APPROVED from
|
|
Please address the newest blocking review feedback first:
After updating the branch, refresh with the latest base changes if GitHub offers it, rerun any needed checks, and then run the Run: https://github.com/github/gh-aw/actions/runs/32642933803
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the newest blocking feedback in 8577ca9: guardrails now adjudicate before primary comparison lookup with no-comparison regression coverage, workflow notifications render the canonical decision tuple, and readiness documentation uses usable observations. Targeted tests and workflow compilation pass; the aggregate validation target only reports unrelated pre-existing custom-lint debt. |
…nt-adjudication Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged latest |
gh aw experiments analyzepreviously exposed readiness and statistics but left promotion decisions to reporting agents. This adds a deterministic, machine-readable adjudication contract without mutating experiments or rerunning analysis.Changes
Core contract
EXTEND,PROMOTE,REJECT, andINCONCLUSIVE.COLLECTING/READYreadiness.recommendationfield.Decision policy
INCONCLUSIVEfor unsupported multi-variant decisions.Reporting
Documentation and coverage
{ "readiness": "READY", "decision": "PROMOTE", "reason_code": "candidate_improved", "control": "control", "candidate": "optimized" }