Repository navigation
Add deterministic experiment decisions - #54978
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality 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
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ 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 decision layer is close, but the new guardrail plumbing still has blocking correctness holes: unresolved guardrails can permanently force EXTEND, and the machine-readable guardrail summary reports passed: true even when no guardrails exist.
Blocking themes
- Guardrail status defaults leak into the decision engine and can make some experiments impossible to conclude.
- The JSON contract now emits contradictory guardrail state for experiments with no configured guardrails.
I ignored the background sub-agent output because it produced no findings.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 29.4 AIC · ⌖ 7.01 AIC · ⊞ 7K
Comment /review to run again
| } | ||
| applyExperimentBalance(&a, exp.Name, exp.Total, cfg, variantCounts, variantNames, expectedPcts) | ||
| applyExperimentReadiness(&a, graderObservations != nil) | ||
| applyExperimentGuardrails(&a, guardrailObservations) |
There was a problem hiding this comment.
This marks every configured guardrail as unresolved when the experiment only has assignment counts, because applyExperimentGuardrails leaves the default unsupported status in place and DecideExperiment treats any unsupported guardrail as an automatic EXTEND. That means native/non-observation-backed metrics can never reach INCONCLUSIVE or any final decision once a guardrail is configured, even after min_samples is met.
💡 Why this blocks merge
The spec says guardrails should extend only when observations are missing or insufficient, not when the analyzer simply doesn't have a per-run metric source for that guardrail. Right now the flow is:
applyExperimentGuardrails(&a, guardrailObservations) // no-op when observations are nil
// guardrail.Status stays "unsupported"
...
pendingGuardrailDecision(...) => "guardrail_unsupported" => EXTENDSo a config like a native primary metric plus any guardrail will never terminate. Either leave unresolved guardrails out of the blocking path unless a metric source was actually attempted, or introduce a distinct "not_evaluated" state that does not force EXTEND.
| } | ||
| return ExperimentDecisionResult{ | ||
| Direction: analysis.MetricDirection, | ||
| Guardrails: ExperimentDecisionGuardrails{Configured: len(analysis.Guardrails) > 0, Passed: guardrailPassSummary(analysis.Guardrails)}, |
There was a problem hiding this comment.
This leaks a fake “all guardrails passed” result into the JSON output when no guardrails are configured, because guardrailPassSummary returns true for the empty slice while Configured is false. Consumers now have to special-case a contradictory payload (configured: false, passed: true) instead of treating the absence of guardrails as “not applicable”.
💡 Why this matters
The current constructor builds:
Guardrails: ExperimentDecisionGuardrails{
Configured: len(analysis.Guardrails) > 0,
Passed: guardrailPassSummary(analysis.Guardrails),
}and guardrailPassSummary returns &true for len == 0. That encodes a semantic lie into the machine-readable API and makes downstream automation harder to reason about. Return nil when no guardrails exist, and only emit passed once the experiment actually has mandatory guardrails to evaluate.
Test Quality Sentinel Analysis 🧪PR: #54978 — "Add deterministic experiment decisions" SummaryThis PR adds deterministic experiment decision logic with comprehensive test coverage. The new features introduce decision-making rules for experiment promotions, rejections, and extensions based on statistical evidence and guardrail validation. Test Coverage Overview
Test MetricsNew Behavioral Tests: 16 scenarios across 3 new test functions
Test-to-Code Ratio
All ratios are healthy — no test inflation detected. Detailed Test AnalysisNew Test Functions (Click to expand)1.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 96/100 — Excellent
All 16 new test scenarios verify design contracts. Edge-case coverage at 87.5%. Zero implementation-detail tests. Test-to-code ratio: 0.92:1 (balanced). No guideline violations.
Implementation-Test Ratio: 0% (threshold: 30%) ✅
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Ponytail review — two thin pass-through wrappers found; no other over-engineering spotted in the diff.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #54978 · auto · 57.3 AIC · ⌖ 5.67 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
pkg/cli/experiments_analyze_statistics.go:184
L184-194: yagni: computeExperimentAnalysisWithObservations only forwards to computeExperimentAnalysisWithObservationBundle with guardrailObservations=nil, and its only non-test caller was replaced by the Bundle variant. Delete the wrapper; call the Bundle version directly (nil guardrails) from tests.
pkg/cli/experiments_command.go:357
L357-367: yagni: computeExperimentAnalysesWithObservations only forwards to computeExperimentAnalysesWithObservationBundle wrapping graderObservationSets in a struct, with a single caller at line 354. Inline the Bundle call there and delete this middle layer.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,142 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.
The deterministic decision engine is well-structured and the precedence logic aligns with the specification. The test coverage in experiments_decision_test.go is solid. Three issues worth addressing before merge:
- Asymmetric regression/improvement threshold comparison (blocking) — regression uses
>while improvement uses>=, creating a subtle boundary inconsistency at the exact threshold value. - Dead-code
"missing"guardrail status (non-blocking) — thependingGuardrailDecisionchecks for a status value that is never set, making the branch unreachable. - Multi-variant vs below-min-samples precedence ordering (non-blocking) — a multi-variant experiment with low samples hits the
EXTEND/insufficient_samplespath before theINCONCLUSIVE/unsupported_multi_variantpath, which may obscure the real reason.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 54.3 AIC · ⌖ 9.32 AIC · ⊞ 6.2K
| } | ||
| if len(analysis.Variants) == 2 { | ||
| return analysis.Variants[0].Name, analysis.Variants[1].Name | ||
| } |
There was a problem hiding this comment.
Asymmetric threshold comparison for regression vs improvement
The regression check uses strict > while the improvement check uses >=:
regressionMaterial := normalized < 0 && -normalized > result.Policy.RegressionTolerance
improvementMaterial := normalized > 0 && normalized >= result.Policy.MinimumEffectThis asymmetry means a candidate that regresses by exactly regression_tolerance is not rejected, while a candidate that improves by exactly minimum_effect is promoted. Since regression_tolerance defaults to minimum_effect, this inconsistency may cause surprising behavior at the boundary. If the conservative-rejection design is intentional, please document it. Otherwise, consider making it >= for symmetry:
regressionMaterial := normalized < 0 && -normalized >= result.Policy.RegressionTolerance@copilot please address this.
| comparison := decisionComparison(analysis.Comparisons) | ||
| if comparison == nil { | ||
| return result.withDecision(experimentDecisionExtend, "insufficient_observations", | ||
| "primary metric observations are not available for statistical comparison") |
There was a problem hiding this comment.
Dead-code "missing" status in pendingGuardrailDecision
pendingGuardrailDecision checks for guardrail.Status == "missing", but this value is never set anywhere in the codebase — guardrails start at "unsupported" (set in newExperimentAnalysis) and transition to "insufficient_observations", "unsupported", "fail", or "pass". The "missing" branch is dead code.
Either remove it or add a path that sets "missing" (e.g., when a guardrail name references a metric with zero observations before the min-samples check).
@copilot please address this.
| return result.withDecision(experimentDecisionExtend, "insufficient_observations", | ||
| "at least two variants are required for a decision") | ||
| } | ||
| if hasVariantBelowMinimum(analysis.Variants) { |
There was a problem hiding this comment.
Multi-variant path reached before below-min-samples check
The current precedence is:
len(variants) < 2→ EXTENDhasVariantBelowMinimum→ EXTENDlen(variants) != 2→ INCONCLUSIVE (multi-variant)
But a multi-variant experiment that also has variants below min_samples reaches step 2 first and returns EXTEND, while spec R-STAT-016 says INCONCLUSIVE should be returned for >2 variants. The current ordering causes the "insufficient_samples" EXTEND to shadow the unsupported multi-variant signal, which may hide the real issue for multi-variant experiments.
Consider checking len(variants) != 2 immediately after the < 2 check to make the multi-variant path explicit before applying single-variant readiness logic:
if len(analysis.Variants) < 2 { ... }
if len(analysis.Variants) != 2 { return inconclusive / unsupported_multi_variant }
if hasVariantBelowMinimum(...) { return extend }@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds deterministic experiment decisions to the analysis pipeline, including policy configuration, guardrail evaluation, and machine-readable output.
Changes:
- Adds
EXTEND,PROMOTE,REJECT, andINCONCLUSIVEdecision logic. - Resolves grader/eval guardrails and exposes decision details through CLI/JSON.
- Updates schemas, tests, and experiment documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/frontmatter_types.go |
Defines decision policy configuration. |
pkg/workflow/compiler_experiments.go |
Extracts policies and validates guardrail references. |
pkg/workflow/compiler_experiments_test.go |
Tests guardrail reference validation. |
pkg/workflow/compiler_experiment_decision_test.go |
Tests decision policy extraction. |
pkg/parser/schemas/main_workflow_schema.json |
Adds the decision configuration schema. |
pkg/cli/experiments_grader_observations.go |
Resolves grader-backed guardrails. |
pkg/cli/experiments_grader_observations_test.go |
Updates grader artifact-loading tests. |
pkg/cli/experiments_eval_observations.go |
Resolves eval-backed guardrails. |
pkg/cli/experiments_decision.go |
Implements deterministic decisions. |
pkg/cli/experiments_decision_test.go |
Tests decisions and guardrail evaluation. |
pkg/cli/experiments_command.go |
Wires guardrail observations into analysis. |
pkg/cli/experiments_analyze_statistics.go |
Integrates decisions, output, and guardrail status. |
pkg/cli/experiments_analyze_statistics_test.go |
Verifies decision JSON fields. |
docs/src/content/docs/reference/frontmatter-full.md |
Regenerates frontmatter reference content. |
docs/src/content/docs/experimental/experiments.md |
Documents configuration and decision output. |
docs/src/content/docs/experimental/experiments-specification.md |
Specifies deterministic decision semantics. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 4
- Review effort level: Balanced
| if hasVariantBelowMinimum(analysis.Variants) { | ||
| return result.withDecision(experimentDecisionExtend, "insufficient_samples", | ||
| "one or more variants have fewer usable observations than min_samples") | ||
| } | ||
| if len(analysis.Variants) != 2 { | ||
| return result.withDecision(experimentDecisionInconclusive, "unsupported_multi_variant", | ||
| "automatic decisions currently require exactly two variants") | ||
| } |
| "enum": ["t_test", "mann_whitney", "proportion_test", "bayesian_ab"], | ||
| "description": "Statistical test to use for automated analysis by the reporting workflow. Valid values: t_test (Welch's two-sample t-test), mann_whitney (non-parametric rank test), proportion_test (two-proportion z-test), bayesian_ab (Bayesian A/B test)." | ||
| }, | ||
| "decision": { |
| Each entry in `analyses` includes `decision`, `reason_code`, `control`, `candidate`, `samples`, | ||
| `effect`, `evidence`, `decision_guardrails`, and `decision_policy`. Future promotion automation | ||
| can consume these fields without rerunning statistics or interpreting grader artifacts. |
| type GuardrailStatus struct { | ||
| Name string `json:"name"` | ||
| Threshold string `json:"threshold"` | ||
| Name string `json:"name"` |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — requesting changes on test coverage gaps and a correctness issue.
📋 Key Themes & Highlights
Key Issues
-
guardrailPassSummaryreturns&truefor no guardrails —Guardrails.Passedis a non-niltruepointer whenConfigured == false, which is contradictory and could mislead downstream consumers (line 243 ofexperiments_decision.go). -
Asymmetric
RegressionTolerancebranch untested — every test passesnilforregressionTolerance, so the path where tolerance differs from minimum effect (line 262 ofexperiments_analyze_statistics.go) has no coverage. -
Unknown
MetricDirectionpath untested —decisionEffectreturnsnilwhen direction is"", causing anEXTEND/analysis_unavailableresult, but no test exercises this. -
Alpha expression is semantically fragile —
*comparison.PValue <= 1-policy.Confidenceis arithmetically correct but reads as if confidence and alpha are the same concept. A namedalphavariable would make this self-documenting. -
ExperimentDecisionResultembedded inExperimentAnalysis— promotes all decision fields to the top-level struct and JSON schema silently. Consider a named field or add a comment explaining the intentional flattening.
Positive Highlights
- ✅ Pure, side-effect-free
DecideExperimentfunction is an excellent design choice — easy to test and reason about. - ✅ Deterministic precedence ordering (samples → guardrails → evidence → effect) is clear and well-documented in the PR description.
- ✅ Bayesian and frequentist paths handled in a single function with consistent output types.
- ✅ Good table-driven test structure with descriptive names that read as specifications.
- ✅
withDecisionvalue-receiver helper avoids mutation bugs on the result.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 82.9 AIC · ⌖ 11.7 AIC · ⊞ 7.6K
Comment /matt to run again
| } | ||
| if cfg.Decision != nil { | ||
| a.DecisionPolicy.MinimumEffect = cfg.Decision.MinimumEffect | ||
| a.DecisionPolicy.RegressionTolerance = cfg.Decision.MinimumEffect |
There was a problem hiding this comment.
[/tdd] RegressionTolerance silently defaults to MinimumEffect when regression_tolerance is omitted — the asymmetric-threshold branch has zero test coverage.
💡 Suggested tests
Add table rows to TestDecideExperiment exercising distinct minimumEffect vs regressionTolerance:
{
name: "asymmetric tolerance: small regression below tolerance rejects",
analysis: readyDecisionTestAnalysis("max", -0.015, &pValueStrong, nil, 0.05, floatPointer(0.01)),
want: experimentDecisionReject, reasonCode: "candidate_regressed",
},
{
name: "asymmetric tolerance: regression between tolerances is inconclusive",
analysis: readyDecisionTestAnalysis("max", -0.03, &pValueStrong, nil, 0.05, floatPointer(0.05)),
want: experimentDecisionInconclusive, reasonCode: "effect_below_threshold",
},Without this, the branch at experiments_analyze_statistics.go:262–265 where RegressionTolerance diverges from MinimumEffect is never exercised by tests.
@copilot please address this.
| if comparison.PValue == nil { | ||
| return nil | ||
| } | ||
| evidence.Significant = *comparison.PValue <= 1-policy.Confidence |
There was a problem hiding this comment.
[/diagnosing-bugs] Frequentist significance check uses 1-policy.Confidence which is correct numerically at runtime, but the semantics are fragile: alpha (the rejection threshold) equals 1 - confidence, so the expression is accidentally right. A reader — or future edit that changes confidence semantics — can easily break this silently.
💡 Suggested fix
Introduce a named local variable to make the intent explicit:
// alpha is the maximum acceptable false-positive rate (e.g. 0.05 at 95% confidence)
alpha := 1.0 - policy.Confidence
evidence.Significant = *comparison.PValue <= alphaThis is also consistent with how the Bayesian branch uses policy.Confidence directly (the conceptual mirror). The current form compiles and tests correctly today, but is a maintenance trap.
@copilot please address this.
| } | ||
|
|
||
| func guardrailPassSummary(guardrails []GuardrailStatus) *bool { | ||
| if len(guardrails) == 0 { |
There was a problem hiding this comment.
[/codebase-design] guardrailPassSummary returns &true when guardrails is empty, meaning Guardrails.Passed is a non-nil true pointer even when no guardrails are configured at all. A downstream consumer checking result.Guardrails.Passed != nil && *result.Guardrails.Passed would incorrectly conclude all guardrails passed in experiments that have none.
💡 Suggested fix
Return nil for no-guardrail case, consistent with the Guardrails.Configured == false signal already present:
func guardrailPassSummary(guardrails []GuardrailStatus) *bool {
if len(guardrails) == 0 {
return nil // no guardrails configured; leave Passed unset
}
// ...
}The Configured field already communicates "no guardrails"; Passed == &true when Configured == false is contradictory.
@copilot please address this.
| // ExperimentAnalysis holds statistical analysis results for one named A/B experiment. | ||
| // The analysis is computed from state.jsonl/state.json counts and optional experiment configuration. | ||
| type ExperimentAnalysis struct { | ||
| ExperimentDecisionResult |
There was a problem hiding this comment.
[/codebase-design] ExperimentDecisionResult is embedded (promoted) into ExperimentAnalysis. This means all 10+ decision fields (decision, reason, reason_code, control, candidate, samples, effect, evidence, decision_guardrails, decision_policy) are promoted to the top level of ExperimentAnalysis in Go, and they serialize as top-level JSON keys. The struct is already large; embedding adds invisible coupling — a future field added to ExperimentDecisionResult silently appears in ExperimentAnalysis JSON output.
💡 Consider using a named field instead
type ExperimentAnalysis struct {
// ...existing fields...
Decision ExperimentDecisionResult `json:"decision_result,omitempty"`
}A named field makes the boundary explicit, keeps the JSON schema stable, and avoids unexpected promotions. Consumers access analysis.Decision.Decision instead of analysis.Decision, which is admittedly slightly verbose — but that's a good signal this struct is doing double duty.
If the promotion is intentional for JSON flattening, add a comment to ExperimentAnalysis explaining why.
@copilot please address this.
| direction := analysis.MetricDirection | ||
| if direction != "min" && direction != "max" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
[/tdd] No test covers the direction == "" early-exit path in decisionEffect — when MetricDirection is neither "min" nor "max", the function returns nil, causing DecideExperiment to emit EXTEND with reason_code: "analysis_unavailable". This path is reachable for any experiment using a native metric without a resolved direction.
💡 Suggested test
{
name: "unknown direction returns extend with analysis_unavailable",
analysis: func() ExperimentAnalysis {
a := readyDecisionTestAnalysis("max", 0.2, &pValueStrong, nil, 0.05, nil)
a.MetricDirection = "" // simulate unresolved direction
return a
}(),
want: experimentDecisionExtend, reasonCode: "analysis_unavailable",
},@copilot please address this.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed a pr-finisher pass and pushed the fixes in |
|
@copilot this PR still has outstanding review follow-up before maintainer investigation will be efficient.
Recent blocking reviews:
Run: https://github.com/github/gh-aw/actions/runs/32620039467
|
|
🎉 This pull request is included in a new release. Release: |
Experiment analysis previously stopped at readiness and statistical comparisons. This adds a deterministic, machine-readable policy layer that recommends
EXTEND,PROMOTE,REJECT, orINCONCLUSIVE.Changes
Decision engine
ExperimentAnalysis → ExperimentDecisionResulttransformation.Decision policy
minimum_effect,regression_tolerance, andconfidencesettings.Guardrails
CLI and JSON
decision,reason_code,control,candidate,samples,effect,evidence,decision_guardrails, anddecision_policyto JSON output.Scope
INCONCLUSIVE.EXTEND.Run: https://github.com/github/gh-aw/actions/runs/32618879655> Generated by 👨🍳 PR Sous Chef · gpt54 · 6.44 AIC · ⌖ 8.16 AIC · ⊞ 9.5K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32620039467> Generated by 👨🍳 PR Sous Chef · gpt54 · 32.1 AIC · ⌖ 10 AIC · ⊞ 7.1K · ◷