feat: extend experiments analyze command with statistical computation#30029
Merged
feat: extend experiments analyze command with statistical computation#30029
Conversation
- Add chi-square goodness-of-fit balance test for variant distributions - Add per-variant min_samples progress tracking with EXTEND/READY_FOR_ANALYSIS recommendation - Add Bonferroni correction reporting for K≥3 variant experiments (§11.3) - Add hypothesis, analysis_type, and guardrail display from workflow frontmatter - Load workflow .md frontmatter locally and remotely for experiment config - Add ExperimentAnalysis, VariantAnalysis, GuardrailStatus types to ExperimentDetails - Add 26 unit tests covering all statistical computation paths Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1c09e868-8d00-426b-a438-5b3d8f114ef2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Use math.MaxInt (not MaxInt32) for platform-correctness - Add explicit path validation in loadLocalExperimentConfigs to prevent path traversal - Improve comment rationale for best-effort config loading - Expand chi-square accuracy documentation with concrete limitations - Inline findVariantAnalysis helper into TestPartiallyBelowMinSamples test Agent-Logs-Url: https://github.com/github/gh-aw/sessions/1c09e868-8d00-426b-a438-5b3d8f114ef2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
pelikhan
May 3, 2026 23:52
View session
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 89/100✅ Excellent
Test Classification DetailsView all 17 test classifications
Flagged Tests — Requires ReviewNo tests flagged. All tests are behavioral contract tests. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25294596840
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Extends gh aw experiments analyze with statistical analysis derived from state.json plus optional experiment configuration loaded from workflow markdown frontmatter.
Changes:
- Adds statistical computation + stderr rendering for min-samples gating, chi-square balance test, Bonferroni alpha, and guardrail threshold display.
- Loads experiment configs from local or remote
.github/workflows/*.mdfrontmatter to enrich analysis (hypothesis, weights, min_samples, guardrails). - Exposes the computed analyses in
--jsonoutput via a newanalysesfield onExperimentDetails.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/experiments_command.go | Wires frontmatter config loading into analyze and attaches computed analyses to ExperimentDetails; adds local/remote workflow lookup helpers. |
| pkg/cli/experiments_analyze_statistics.go | Implements analysis types/logic and console rendering for statistical output. |
| pkg/cli/experiments_analyze_statistics_test.go | Adds unit tests covering p-value approximation, weighting, min-samples gating, Bonferroni, and JSON serialization. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (2)
pkg/cli/experiments_command.go:366
- The
contents/endpoint path is being built withurl.PathEscape(apiPath), which will escape/into%2F. For GitHub’srepos/.../contents/{path}API,{path}is expected to contain slashes for directories (e.g..github/workflows/foo.md), so escaping the whole path can cause 404s. Build the endpoint using the raw directory path and escape only the filename segment if needed.
apiPath := ".github/workflows/" + candidate + ".md"
args := []string{"api",
"repos/{owner}/{repo}/contents/" + url.PathEscape(apiPath),
"--jq", ".content",
"--repo", repoOverride,
}
pkg/cli/experiments_command.go:425
- Remote config lookup currently tries only
experimentNameas the workflow markdown basename. Since experiment branches use a sanitized workflow ID (hyphens removed), this will miss common filenames likemy-workflow.md, meaning remote config loading will almost always fall back to defaults. Consider mirroring the local lookup approach remotely: list.github/workflowsvia the contents API, pick the.mdfile whose sanitized basename matchesexperimentName, then fetch that file.
// Scan common workflow file name candidates: the experiment name as-is, and with
// a hyphen reintroduced before common separators. We try the exact name first since
// the sanitized form (hyphens removed, lowercased) is irreversible in general.
candidates := workflowFileCandidates(experimentName)
for _, candidate := range candidates {
apiPath := ".github/workflows/" + candidate + ".md"
args := []string{"api",
"repos/{owner}/{repo}/contents/" + url.PathEscape(apiPath),
"--jq", ".content",
"--repo", repoOverride,
}
cmd := workflow.ExecGH(args...)
out, err := cmd.Output()
if err != nil {
continue
}
b64 := strings.Join(strings.Fields(strings.TrimSpace(string(out))), "")
decoded, err := base64.StdEncoding.DecodeString(b64)
if err != nil {
experimentsLog.Printf("Failed to base64-decode workflow file %s: %v", candidate, err)
continue
}
result, err := parser.ExtractFrontmatterFromContent(string(decoded))
if err != nil {
continue
}
cfg, err := workflow.ParseFrontmatterConfig(result.Frontmatter)
if err != nil {
continue
}
if len(cfg.ExperimentConfigs) > 0 {
experimentsLog.Printf("Loaded remote configs from %s", apiPath)
return cfg.ExperimentConfigs
}
}
experimentsLog.Printf("No remote workflow file found for experiment %s", experimentName)
return nil
}
// findWorkflowFileForExperiment scans .github/workflows/ for a .md file whose sanitized
// basename (lowercase, hyphens removed) matches the given experiment name.
// Returns the file path or "" when no match is found.
func findWorkflowFileForExperiment(experimentName string) string {
mdFiles, err := getMarkdownWorkflowFiles("")
if err != nil {
return ""
}
for _, f := range mdFiles {
base := strings.TrimSuffix(filepath.Base(f), ".md")
if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName {
return f
}
}
return ""
}
// workflowFileCandidates returns a list of candidate workflow file basenames (without .md)
// to try when looking up a remote workflow by its sanitized experiment name.
// The sanitized form is lossy (hyphens removed), so we return the sanitized name itself
// plus the original name as candidates.
func workflowFileCandidates(experimentName string) []string {
// Start with the experiment name as-is (may already be the correct filename).
candidates := []string{experimentName}
return candidates
}
- Files reviewed: 3/3 changed files
- Comments generated: 3
Comment on lines
+146
to
+192
| k := len(variantNames) | ||
|
|
||
| // Chi-square goodness-of-fit balance test. | ||
| if exp.Total > 0 && k >= 2 { | ||
| chi2 := 0.0 | ||
| for i, name := range variantNames { | ||
| expected := float64(exp.Total) * expectedPcts[i] | ||
| if expected > 0 { | ||
| diff := float64(exp.Variants[name]) - expected | ||
| chi2 += (diff * diff) / expected | ||
| } | ||
| } | ||
| df := k - 1 | ||
| pval := chiSquarePValue(chi2, df) | ||
| a.ChiSquare = chi2 | ||
| a.DegreesOfFreedom = df | ||
| a.PValue = pval | ||
| a.IsBalanced = pval >= balanceSignificanceThreshold | ||
| } else { | ||
| a.IsBalanced = true // insufficient data to assess balance | ||
| } | ||
|
|
||
| // Bonferroni correction for K ≥ 3 variants (§11.3). | ||
| if k >= 3 { | ||
| a.BonferroniAlpha = 0.05 / float64(k-1) | ||
| } | ||
|
|
||
| // Recommendation (R-STAT-007). | ||
| belowCount := 0 | ||
| minObserved := math.MaxInt | ||
| for _, v := range a.Variants { | ||
| if v.BelowMinSamples { | ||
| belowCount++ | ||
| } | ||
| if v.Count < minObserved { | ||
| minObserved = v.Count | ||
| } | ||
| } | ||
| if belowCount > 0 { | ||
| a.Recommendation = "EXTEND" | ||
| a.Rationale = fmt.Sprintf("%d of %d variant(s) below min_samples threshold (min observed: %d / %d)", | ||
| belowCount, k, minObserved, a.MinSamples) | ||
| } else { | ||
| a.Recommendation = "READY_FOR_ANALYSIS" | ||
| a.Rationale = fmt.Sprintf("all %d variants have reached min_samples (%d); proceed with outcome metric analysis", | ||
| k, a.MinSamples) | ||
| } |
Comment on lines
+316
to
+342
| // findWorkflowFileForExperiment returns absolute paths from getMarkdownWorkflowFiles, | ||
| // which enumerates only files beneath .github/workflows/ (no user input injected). | ||
| workflowsDir, err := filepath.Abs(getWorkflowsDir()) | ||
| if err != nil { | ||
| experimentsLog.Printf("Failed to resolve workflows dir: %v", err) | ||
| return nil | ||
| } | ||
| if !strings.HasPrefix(filePath, workflowsDir+string(filepath.Separator)) { | ||
| experimentsLog.Printf("Refusing to read workflow file outside .github/workflows/: %s", filePath) | ||
| return nil | ||
| } | ||
|
|
||
| content, err := os.ReadFile(filePath) // #nosec G304 — path confirmed within .github/workflows/ | ||
| if err != nil { | ||
| experimentsLog.Printf("Failed to read workflow file %s: %v", filePath, err) | ||
| return nil | ||
| } | ||
|
|
||
| result, err := parser.ExtractFrontmatterFromContent(string(content)) | ||
| if err != nil { | ||
| experimentsLog.Printf("Failed to parse frontmatter from %s: %v", filePath, err) | ||
| return nil | ||
| } | ||
|
|
||
| cfg, err := workflow.ParseFrontmatterConfig(result.Frontmatter) | ||
| if err != nil { | ||
| experimentsLog.Printf("Failed to parse frontmatter config from %s: %v", filePath, err) |
Comment on lines
+302
to
+313
| // loadLocalExperimentConfigs reads the workflow .md file for the given experiment name | ||
| // and returns the ExperimentConfig map from its frontmatter. | ||
| // experimentName is the sanitized workflow ID (the part after "experiments/" in the branch name). | ||
| // Returns an empty map when the workflow file cannot be found or parsed. | ||
| func loadLocalExperimentConfigs(experimentName string) map[string]*workflow.ExperimentConfig { | ||
| experimentsLog.Printf("Loading local experiment configs for %s", experimentName) | ||
|
|
||
| filePath := findWorkflowFileForExperiment(experimentName) | ||
| if filePath == "" { | ||
| experimentsLog.Printf("No workflow file found for experiment %s", experimentName) | ||
| return nil | ||
| } |
…analysis Generated by the Design Decision Gate workflow from PR #30029 diff. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Commit pushed:
|
Contributor
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (1,117 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: 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. 📋 What the draft ADR coversThe draft captures the following key decisions inferred from the diff:
Please verify these reflect your actual design intent and add any constraints or trade-offs the AI may have missed. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25294596855
|
Collaborator
|
@copilot review all comments |
…l check - computeExperimentAnalysis: add early return with EXTEND when exp.Variants < 2 to avoid nonsensical "all 0 variants have reached min_samples" rationale - loadLocalExperimentConfigs: convert filePath to absolute before boundary check; filepath.Glob with a relative dir returns relative paths, so the old prefix check against an absolute workflowsDir always failed and config loading was never used - Fix comment: "Returns an empty map" → "Returns nil" (matches implementation) - Add TestComputeExperimentAnalysisDegenerateVariants for zero-variant and single-variant cases Agent-Logs-Url: https://github.com/github/gh-aw/sessions/dcad2d14-7ec1-47f9-bb59-1a67151883a0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all three reviewer findings in commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
This was referenced May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the
gh aw experiments analyzecommand with statistical computation of the hypotheses defined in experiments, as described in §11 of the experiments specification.What's new
The
analyzecommand now reads experiment configuration (hypothesis, analysis type, min_samples, guardrail thresholds) from the workflow's.mdfrontmatter and computes:hypothesis:text declared in the experiment confign / min_samples, with a warning indicator (⚠) on variants below thresholdmin_samples(defaults to 20); READY_FOR_ANALYSIS when all variants have sufficient data to proceed with outcome metric analysisweight:config), reporting χ², degrees of freedom, p-value, and abalanced ✓/unbalanced ✗status0.05 / (K−1)) for experiments with K ≥ 3 variants, both in console output and JSONguardrail_metricsthresholds; notes that pass/fail evaluation requires per-run outcome data (R-STAT-009)The analysis is also available in
--jsonoutput via the newanalysesfield onExperimentDetails.Workflow config loading
--repo): scans.github/workflows/for a.mdfile whose sanitized basename matches the experiment name; includes explicit path-boundary validation.--repo): fetches the workflow.mdfrom the default branch via the GitHub API.Specification compliance
state.runs)Changes
pkg/cli/experiments_analyze_statistics.go— all statistical types and logicpkg/cli/experiments_analyze_statistics_test.go— 26 unit testspkg/cli/experiments_command.go— wires config loading and analysis intoRunExperimentsAnalyze; addsAnalyses []ExperimentAnalysistoExperimentDetails