diff --git a/docs/adr/53706-split-experiments-cli-into-focused-modules.md b/docs/adr/53706-split-experiments-cli-into-focused-modules.md new file mode 100644 index 00000000000..f0e8d7f9467 --- /dev/null +++ b/docs/adr/53706-split-experiments-cli-into-focused-modules.md @@ -0,0 +1,58 @@ +# ADR-53706: Split Experiments CLI into Focused Modules + +**Date**: 2026-08-18 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`pkg/cli/experiments_command.go` grew to approximately 1,120 lines by combining command wiring, local and remote data fetching, experiment state parsing (JSON and JSONL formats), git argument validation, human-readable rendering, and paginated JSON decoding in a single file. This made it difficult to navigate to a specific concern, reason about each responsibility in isolation, and maintain the file without risk of touching unrelated logic. The file also could not be unit-tested at the concern level because all functions were interleaved. + +### Decision + +We will decompose `experiments_command.go` into six files within the same `cli` package, each owning a single concern: + +- `experiments_command.go` — Cobra command construction and list/analyze orchestration +- `experiments_fetch.go` — local and remote branch discovery, workflow frontmatter loading, and metric-evaluation retrieval +- `experiments_state.go` — experiment state models, JSON/JSONL parsing, aggregation, and branch ref lookup helpers +- `experiments_git_safety.go` — validation of `git show ref:path` arguments (ref safety, tree path safety) +- `experiments_render.go` — human-readable detail output to stderr +- `experiments_json_utils.go` — paginated JSON-array decoding shared by fetch logic + +The existing CLI API and all runtime behavior are preserved exactly; this is a structural refactor only. + +### Alternatives Considered + +#### Alternative 1: Keep the monolith, add section comments + +Add named comment blocks (`// --- Fetching ---`, `// --- Rendering ---`) to partition the single file visually without moving any code. This avoids a larger diff and leaves the package topology unchanged. + +This was not chosen because comment-based partitioning does not enforce separation — functions in any section can freely call functions in any other, and editors/grep still return results from one enormous file. Navigation and isolated review remain hard. + +#### Alternative 2: Extract to a dedicated sub-package (`pkg/cli/experiments/`) + +Move all experiment logic into a `experiments` sub-package, exporting only the types and functions needed by the `cli` package, and importing them in `experiments_command.go`. + +This was not chosen for this PR because it would require renaming types and adding explicit exports across a wider surface, increasing the diff size and review burden. It also risks breaking other callers in the `cli` package. A file-level split within the same package achieves meaningful separation with minimal risk and can be followed by a package extraction in a future PR if warranted. + +### Consequences + +#### Positive +- Each file has a single, named purpose, making it easy to locate code by concern. +- Future additions (new fetching strategies, alternative rendering modes) have a natural home without growing `experiments_command.go`. +- Reviewers can audit security-sensitive logic (git argument validation) in isolation in `experiments_git_safety.go`. +- Smaller files are easier to hold in working memory during review. + +#### Negative +- Functions in the same package still share package scope; the file split does not create an enforced interface boundary. Cross-concern coupling can silently re-emerge over time. +- Readers must discover which file owns which concern; there is no directory-level signal (unlike a sub-package) to guide them. + +#### Neutral +- The CLI public API and all data formats (state.jsonl, state.json) are unchanged, so no downstream callers or tests need updates. +- The total line count across all six files equals the original single-file line count; no logic was added or removed. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/experiments_command.go b/pkg/cli/experiments_command.go index 500626cf3b6..68eacebf433 100644 --- a/pkg/cli/experiments_command.go +++ b/pkg/cli/experiments_command.go @@ -1,83 +1,18 @@ package cli import ( - "bufio" - "encoding/base64" - "encoding/json" - "errors" "fmt" - "io" - "net/url" "os" - "os/exec" - "path" - "path/filepath" - "slices" - "strconv" - "strings" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" - "github.com/github/gh-aw/pkg/errorutil" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/parser" - "github.com/github/gh-aw/pkg/setutil" - "github.com/github/gh-aw/pkg/sliceutil" - "github.com/github/gh-aw/pkg/tty" "github.com/github/gh-aw/pkg/workflow" "github.com/spf13/cobra" ) var experimentsLog = logger.New("cli:experiments_command") -// experimentsBranchPrefix is the git branch prefix used to identify experiment state branches. -const experimentsBranchPrefix = "experiments/" -const evalsBranchPrefix = constants.EvalsBranchPrefix + "/" - -// ExperimentState represents experiment state stored in experiments/* branches. -// This matches the legacy JSON snapshot format and the JSONL run-ledger format written by pick_experiment.cjs. -type ExperimentState struct { - Counts map[string]map[string]int `json:"counts"` // experiment name → variant → count - Runs []ExperimentRunRecord `json:"runs,omitempty"` -} - -// ExperimentRunRecord represents a single workflow run in the JSONL ledger. -type ExperimentRunRecord struct { - RunID string `json:"run_id"` - Timestamp string `json:"timestamp"` - Assignments map[string]string `json:"assignments"` - BaselineCounts map[string]map[string]int `json:"baseline_counts,omitempty"` -} - -// ExperimentVariantStats holds counts for all variants of one named A/B experiment. -type ExperimentVariantStats struct { - Name string `json:"name"` - Variants map[string]int `json:"variants"` // variant → count - Total int `json:"total"` -} - -// ExperimentInfo represents a single experiment workflow for list output. -type ExperimentInfo struct { - WorkflowID string `json:"workflow_id" console:"header:Workflow"` - Branch string `json:"branch" console:"header:Branch"` - Experiments int `json:"experiments" console:"header:Experiments"` - TotalRuns int `json:"total_runs" console:"header:Total Runs"` - LastRun string `json:"last_run" console:"header:Last Run"` -} - -// ExperimentDetails represents detailed information about a specific experiment workflow. -type ExperimentDetails struct { - WorkflowID string `json:"workflow_id"` - Branch string `json:"branch"` - TotalRuns int `json:"total_runs"` - Experiments []ExperimentVariantStats `json:"experiments"` - RecentRuns []ExperimentRunRecord `json:"recent_runs,omitempty"` - // Analyses holds the statistical analysis for each named experiment. - // Populated by RunExperimentsAnalyze; absent in list output. - Analyses []ExperimentAnalysis `json:"analyses,omitempty"` -} - -// ExperimentsListConfig holds configuration for the experiments list subcommand. type ExperimentsListConfig struct { RepoOverride string JSONOutput bool @@ -320,801 +255,3 @@ func computeExperimentAnalyses( } return analyses } - -type evalResultRecord struct { - ID string `json:"id"` - Answer string `json:"answer"` - RunID string `json:"runid"` - Timestamp string `json:"timestamp"` -} - -func loadLocalMetricEvalResults(workflowID string) map[string]MetricEvalResults { - branchName := workflow.WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID) - ref := "origin/" + branchName - if !gitRefExists(ref) { - if !gitRefExists(branchName) { - return nil - } - ref = branchName - } - if !isSafeExperimentStateRef(ref) { - experimentsLog.Printf("Rejecting unsafe git ref: %q", ref) - return nil - } - objectArg, err := buildSafeGitShowObjectArg(ref, constants.EvalsResultFilename) - if err != nil { - experimentsLog.Printf("Rejecting unsafe git show argument (ref=%q file=%q): %v", ref, constants.EvalsResultFilename, err) - return nil - } - cmd := exec.Command("git", "show", objectArg) - out, err := cmd.Output() - if err != nil { - return nil - } - return summarizeMetricEvalResults(out) -} - -func loadRemoteMetricEvalResults(repoOverride, workflowID string) map[string]MetricEvalResults { - branchName := workflow.WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID) - decoded, err := readRemoteRepoBranchFile(repoOverride, branchName, constants.EvalsResultFilename, "") - if err != nil { - return nil - } - return summarizeMetricEvalResults(decoded) -} - -func summarizeMetricEvalResults(data []byte) map[string]MetricEvalResults { - scanner := bufio.NewScanner(strings.NewReader(string(data))) - results := map[string]MetricEvalResults{} - for scanner.Scan() { - line := strings.TrimSpace(scanner.Text()) - if line == "" { - continue - } - var record evalResultRecord - if err := json.Unmarshal([]byte(line), &record); err != nil { - continue - } - if record.ID == "" { - continue - } - summary := results[record.ID] - summary.Total++ - switch strings.ToUpper(strings.TrimSpace(record.Answer)) { - case "YES": - summary.Yes++ - case "NO": - summary.No++ - default: - summary.Unknown++ - } - summary.LatestAnswer = strings.ToUpper(strings.TrimSpace(record.Answer)) - summary.LatestRunID = record.RunID - results[record.ID] = summary - } - if len(results) == 0 { - return nil - } - return results -} - -// experimentFrontmatterResult holds both the experiment configs and evals config parsed -// from a workflow's frontmatter. -type experimentFrontmatterResult struct { - ExperimentConfigs map[string]*workflow.ExperimentConfig - Evals *workflow.EvalsConfig -} - -// loadLocalExperimentConfigs reads the workflow .md file for the given experiment name -// and returns the ExperimentConfig map and EvalsConfig from its frontmatter. -// experimentName is the sanitized workflow ID (the part after "experiments/" in the branch name). -// Returns a zero-value result when the workflow file cannot be found or parsed. -func loadLocalExperimentConfigs(experimentName string) experimentFrontmatterResult { - 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 experimentFrontmatterResult{} - } - - // Verify that the resolved path is within .github/workflows/ to prevent path traversal. - // findWorkflowFileForExperiment returns paths from filepath.Glob with a relative base dir, - // so convert both sides to absolute paths before the prefix check. - absFilePath, err := filepath.Abs(filePath) - if err != nil { - experimentsLog.Printf("Failed to resolve absolute path for %s: %v", filePath, err) - return experimentFrontmatterResult{} - } - workflowsDir, err := filepath.Abs(getWorkflowsDir()) - if err != nil { - experimentsLog.Printf("Failed to resolve workflows dir: %v", err) - return experimentFrontmatterResult{} - } - if !strings.HasPrefix(absFilePath, workflowsDir+string(filepath.Separator)) { - experimentsLog.Printf("Refusing to read workflow file outside .github/workflows/: %s", absFilePath) - return experimentFrontmatterResult{} - } - - content, err := os.ReadFile(absFilePath) // #nosec G304 -- path confirmed within .github/workflows/ - if err != nil { - experimentsLog.Printf("Failed to read workflow file %s: %v", absFilePath, err) - return experimentFrontmatterResult{} - } - - result, err := parser.ExtractFrontmatterFromContent(string(content)) - if err != nil { - experimentsLog.Printf("Failed to parse frontmatter from %s: %v", filePath, err) - return experimentFrontmatterResult{} - } - - cfg, err := workflow.ParseFrontmatterConfig(result.Frontmatter) - if err != nil { - experimentsLog.Printf("Failed to parse frontmatter config from %s: %v", filePath, err) - return experimentFrontmatterResult{} - } - - evals, err := workflow.ParseEvalsFromFrontmatter(result.Frontmatter) - if err != nil { - experimentsLog.Printf("Failed to parse evals config from %s: %v", filePath, err) - // Non-fatal: continue without evals resolution. - } - - return experimentFrontmatterResult{ - ExperimentConfigs: cfg.ExperimentConfigs, - Evals: evals, - } -} - -// loadRemoteExperimentConfigs fetches the workflow .md file from the repository default branch -// via the GitHub API and returns the ExperimentConfig map and EvalsConfig from its frontmatter. -// Returns a zero-value result when the file cannot be fetched or parsed. -func loadRemoteExperimentConfigs(repoOverride, experimentName string) experimentFrontmatterResult { - experimentsLog.Printf("Loading remote experiment configs for %s from %s", experimentName, repoOverride) - - // Build the candidate list. First, use the directory listing to find the exact filename - // whose sanitized basename matches experimentName (e.g. "ci-coach" for "cicoach"). - // Fall back to the bare experiment name if the listing is unavailable. - candidates := workflowFileCandidates(experimentName) - if resolved := findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName); resolved != "" && resolved != experimentName { - // Prepend the resolved name so it is tried before the bare sanitized form. - // Skip when resolved == experimentName to avoid a redundant fetch. - candidates = append([]string{resolved}, candidates...) - } - - for _, candidate := range candidates { - apiPath := constants.WorkflowsDirSlash + 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 - } - - evals, err := workflow.ParseEvalsFromFrontmatter(result.Frontmatter) - if err != nil { - experimentsLog.Printf("Failed to parse evals config from %s: %v", apiPath, err) - // Non-fatal: continue without evals resolution. - } - - if len(cfg.ExperimentConfigs) > 0 { - experimentsLog.Printf("Loaded remote configs from %s", apiPath) - return experimentFrontmatterResult{ - ExperimentConfigs: cfg.ExperimentConfigs, - Evals: evals, - } - } - } - - experimentsLog.Printf("No remote workflow file found for experiment %s", experimentName) - return experimentFrontmatterResult{} -} - -// findRemoteWorkflowFilenameForExperiment lists .md files in .github/workflows/ via the -// GitHub API and returns the basename (without .md) of the first file whose sanitized name -// matches experimentName. This mirrors findWorkflowFileForExperiment for remote repos. -// Returns "" when the directory cannot be listed or no match is found. -func findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName string) string { - args := []string{"api", - "repos/{owner}/{repo}/contents/.github/workflows", - "--jq", `[.[] | select(.name | endswith(".md")) | .name]`, - "--repo", repoOverride, - } - cmd := workflow.ExecGH(args...) - out, err := cmd.Output() - if err != nil { - experimentsLog.Printf("Failed to list remote workflow files from %s: %v", repoOverride, err) - return "" - } - - var filenames []string - if err := json.Unmarshal(out, &filenames); err != nil { - experimentsLog.Printf("Failed to parse remote workflow file listing: %v", err) - return "" - } - - return matchWorkflowFilenameByExperiment(filenames, experimentName) -} - -// matchWorkflowFilenameByExperiment returns the basename (without .md) of the first file in -// filenames whose sanitized name matches experimentName. Returns "" when no match is found. -// Logs a warning when more than one file maps to the same sanitized name. -// -// Note: normalizeWorkflowID calls filepath.Base internally, so any path prefix in filenames -// is stripped before matching. Callers that supply bare filenames (e.g. "my-flow.md") are -// unaffected; callers supplying full paths (e.g. ".github/workflows/my-flow.md") will have -// the directory component removed — only the basename is returned and compared. -func matchWorkflowFilenameByExperiment(filenames []string, experimentName string) string { - var matches []string - for _, filename := range filenames { - base := normalizeWorkflowID(filename) - if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { - matches = append(matches, base) - } - } - if len(matches) == 0 { - return "" - } - if len(matches) > 1 { - experimentsLog.Printf("Ambiguous experiment name %q: multiple workflow files match (%s); using first", experimentName, strings.Join(matches, ", ")) - } - return matches[0] -} - -// 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 := normalizeWorkflowID(f) - if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { - return f - } - } - return "" -} - -// workflowFileCandidates returns a fallback list of candidate workflow file basenames (without .md) -// for remote lookups when the directory listing is unavailable. The sanitized form -// (hyphens removed, lowercased) is irreversible, so only the experiment name itself is -// returned here. The caller should prefer findRemoteWorkflowFilenameForExperiment which -// resolves the real filename by scanning the remote directory. -func workflowFileCandidates(experimentName string) []string { - // Return the experiment name as-is as a last-resort fallback. - return []string{experimentName} -} - -// fetchLocalExperiments lists experiment branches and reads their state from the local git repo. -func fetchLocalExperiments() ([]ExperimentInfo, error) { - experimentsLog.Print("Fetching local experiment branches via git for-each-ref") - - cmd := exec.Command("git", "for-each-ref", - "--sort=-committerdate", - "--format=%(refname:short)", - "refs/remotes/origin/"+experimentsBranchPrefix+"*", - "refs/heads/"+experimentsBranchPrefix+"*", - ) - output, err := cmd.Output() - if err != nil { - var exitErr *exec.ExitError - if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { - return []ExperimentInfo{}, nil - } - return nil, fmt.Errorf("failed to list experiment branches: %w", err) - } - - seen := make(map[string]struct { - }) - var experiments []ExperimentInfo - - for line := range strings.SplitSeq(strings.TrimSpace(string(output)), "\n") { - if line == "" { - continue - } - workflowID := extractExperimentName(line) - if workflowID == "" || setutil.Contains(seen, workflowID) { - continue - } - seen[workflowID] = struct { - }{} - - branchName := experimentsBranchPrefix + workflowID - // Prefer remote ref; fall back to local. - ref := "origin/" + branchName - if !gitRefExists(ref) { - ref = branchName - } - state := readLocalExperimentState(ref) - experiments = append(experiments, experimentInfoFromState(workflowID, branchName, state)) - } - - return experiments, nil -} - -// fetchRemoteExperiments lists experiment branches and reads their state via the GitHub API. -func fetchRemoteExperiments(repoOverride string) ([]ExperimentInfo, error) { - experimentsLog.Printf("Fetching remote experiment branches: repo=%s", repoOverride) - - args := []string{"api", "repos/{owner}/{repo}/branches", - "--paginate", - "--jq", `[.[] | select(.name | startswith("` + experimentsBranchPrefix + `")) | .name]`, - "--repo", repoOverride, - } - cmd := workflow.ExecGH(args...) - output, err := cmd.Output() - if err != nil { - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - return nil, fmt.Errorf("failed to fetch branches (exit %d): %s", exitErr.ExitCode(), strings.TrimSpace(string(exitErr.Stderr))) - } - return nil, fmt.Errorf("failed to fetch branches: %w", err) - } - - branchNames, err := parsePagedJSONArray[string](string(output)) - if err != nil { - return nil, fmt.Errorf("failed to parse branch list: %w", err) - } - - var experiments []ExperimentInfo - for _, branchName := range branchNames { - workflowID := strings.TrimPrefix(branchName, experimentsBranchPrefix) - state := readRemoteExperimentState(repoOverride, branchName) - experiments = append(experiments, experimentInfoFromState(workflowID, branchName, state)) - } - - return experiments, nil -} - -// fetchLocalExperimentDetails reads experiment state from a local experiment branch. -func fetchLocalExperimentDetails(branchName, workflowID string) (*ExperimentDetails, error) { - experimentsLog.Printf("Fetching local experiment details: branch=%s", branchName) - - ref := "origin/" + branchName - if !gitRefExists(ref) { - if !gitRefExists(branchName) { - return nil, fmt.Errorf("experiment branch %q not found locally (tried origin/%s and %s)", - branchName, branchName, branchName) - } - ref = branchName - } - - state := readLocalExperimentState(ref) - return experimentDetailsFromState(workflowID, branchName, state), nil -} - -// fetchRemoteExperimentDetails reads experiment state from a remote experiment branch. -func fetchRemoteExperimentDetails(repoOverride, branchName, workflowID string) (*ExperimentDetails, error) { - experimentsLog.Printf("Fetching remote experiment details: repo=%s, branch=%s", repoOverride, branchName) - - // Verify the branch exists. - encodedBranch := url.PathEscape(branchName) - checkArgs := []string{"api", - "repos/{owner}/{repo}/branches/" + encodedBranch, - "--jq", ".name", - "--repo", repoOverride, - } - checkCmd := workflow.ExecGH(checkArgs...) - if _, err := checkCmd.Output(); err != nil { - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - stderr := strings.TrimSpace(string(exitErr.Stderr)) - if errorutil.IsNotFoundOutput(stderr) { - return nil, fmt.Errorf("experiment %q not found in %s", workflowID, repoOverride) - } - return nil, fmt.Errorf("failed to fetch experiment branch (exit %d): %s", exitErr.ExitCode(), stderr) - } - return nil, fmt.Errorf("failed to fetch experiment branch: %w", err) - } - - state := readRemoteExperimentState(repoOverride, branchName) - return experimentDetailsFromState(workflowID, branchName, state), nil -} - -func experimentStateFilenames() []string { - return []string{"state.jsonl", "state.json"} -} - -// readLocalExperimentState reads experiment state from a local git ref (e.g. "origin/experiments/foo"). -// Returns an empty state when the file is absent or cannot be parsed. -func readLocalExperimentState(ref string) *ExperimentState { - for _, fileName := range experimentStateFilenames() { - objectArg, err := buildSafeGitShowObjectArg(ref, fileName) - if err != nil { - experimentsLog.Printf("Skipping unsafe git show argument (ref=%q file=%q): %v", ref, fileName, err) - continue - } - cmd := exec.Command("git", "show", objectArg) - out, err := cmd.Output() - if err == nil { - return parseExperimentState(out) - } - } - return emptyExperimentState() -} - -// buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts -// before joining them, preventing flag and path-traversal style injections. -func buildSafeGitShowObjectArg(ref, fileName string) (string, error) { - if !isSafeExperimentStateRef(ref) { - return "", errors.New("unsafe git ref") - } - if !isSafeGitTreePath(fileName) { - return "", errors.New("unsafe git tree path") - } - return ref + ":" + fileName, nil -} - -func isSafeExperimentStateRef(ref string) bool { - if !isSafeGitRevisionArg(ref) { - return false - } - - // Allow direct object IDs (including abbreviated prefixes) for future callers - // while rejecting revision operators. - if isHexObjectIDPrefix(ref) { - return true - } - - trimmed := strings.TrimPrefix(ref, "origin/") - if !strings.HasPrefix(trimmed, experimentsBranchPrefix) && !strings.HasPrefix(trimmed, evalsBranchPrefix) { - return false - } - - return isSafeGitRefName(trimmed) -} - -func isHexObjectIDPrefix(ref string) bool { - // Require >=7 chars to avoid accepting short hex-like experiment names as SHAs. - // 64 keeps compatibility with SHA-256 object IDs. - if len(ref) < 7 || len(ref) > 64 { - return false - } - for _, r := range ref { - if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') { - return false - } - } - return true -} - -// isSafeGitRefName validates a refname with check-ref-format-equivalent rules. -func isSafeGitRefName(ref string) bool { - hasInvalidShape := ref == "" || - strings.HasPrefix(ref, "/") || - strings.HasSuffix(ref, "/") || - strings.HasSuffix(ref, ".") - hasInvalidSequences := strings.Contains(ref, "//") || - strings.Contains(ref, "..") || - strings.Contains(ref, "@{") || - strings.Contains(ref, "\\") - if hasInvalidShape || hasInvalidSequences { - return false - } - - for part := range strings.SplitSeq(ref, "/") { - if part == "" || strings.HasPrefix(part, ".") || strings.HasSuffix(part, ".lock") { - return false - } - for _, r := range part { - if r <= ' ' || r == '~' || r == '^' || r == ':' || r == '?' || r == '*' || r == '[' || r == '\x7f' { - return false - } - } - } - return true -} - -// isSafeGitTreePath validates a git tree entry path used in "ref:path" syntax. -// Git tree paths always use forward slashes across platforms, so this intentionally -// uses the slash-based path package (not filepath) for normalization checks. -func isSafeGitTreePath(fileName string) bool { - if fileName == "" || strings.HasPrefix(fileName, "-") { - return false - } - if path.IsAbs(fileName) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') { - return false - } - clean := path.Clean(fileName) - if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") { - return false - } - return clean == fileName -} - -// readRemoteExperimentState fetches experiment state from an experiments/* branch via the GitHub API. -// Returns an empty state on any error (branch missing, file absent, parse failure). -func readRemoteExperimentState(repoOverride, branchName string) *ExperimentState { - for _, fileName := range experimentStateFilenames() { - decoded, err := readRemoteRepoBranchFile(repoOverride, branchName, fileName, "") - if err == nil { - return parseExperimentState(decoded) - } - } - return emptyExperimentState() -} - -func appendExperimentRun(state *ExperimentState, run ExperimentRunRecord) { - if state.Counts == nil { - state.Counts = map[string]map[string]int{} - } - for name, variants := range run.BaselineCounts { - if state.Counts[name] == nil { - state.Counts[name] = map[string]int{} - } - for variant, count := range variants { - state.Counts[name][variant] += count - } - } - for name, variant := range run.Assignments { - if state.Counts[name] == nil { - state.Counts[name] = map[string]int{} - } - state.Counts[name][variant]++ - } - state.Runs = append(state.Runs, run) -} - -func parseExperimentStateJSONL(data []byte) *ExperimentState { - state := emptyExperimentState() - for line := range strings.SplitSeq(string(data), "\n") { - line = strings.TrimSpace(line) - if line == "" { - continue - } - - var snapshot ExperimentState - if err := json.Unmarshal([]byte(line), &snapshot); err == nil && snapshot.Counts != nil { - state = &snapshot - continue - } - - var run ExperimentRunRecord - if err := json.Unmarshal([]byte(line), &run); err != nil || run.RunID == "" || run.Timestamp == "" || len(run.Assignments) == 0 { - experimentsLog.Printf("parseExperimentStateJSONL: skipping unrecognized line") - continue - } - appendExperimentRun(state, run) - } - return state -} - -// parseExperimentState unmarshals raw JSON or JSONL into an ExperimentState. -// Returns an empty state when parsing fails or the data is invalid. -func parseExperimentState(data []byte) *ExperimentState { - var state ExperimentState - if err := json.Unmarshal(data, &state); err == nil && state.Counts != nil { - return &state - } - return parseExperimentStateJSONL(data) -} - -// emptyExperimentState returns a zero-value ExperimentState with an initialised Counts map. -func emptyExperimentState() *ExperimentState { - return &ExperimentState{Counts: map[string]map[string]int{}} -} - -// experimentInfoFromState builds an ExperimentInfo summary from experiment state. -func experimentInfoFromState(workflowID, branchName string, state *ExperimentState) ExperimentInfo { - return ExperimentInfo{ - WorkflowID: workflowID, - Branch: branchName, - Experiments: len(state.Counts), - TotalRuns: experimentTotalRuns(state), - LastRun: experimentLastRun(state), - } -} - -// experimentDetailsFromState builds ExperimentDetails from experiment state. -func experimentDetailsFromState(workflowID, branchName string, state *ExperimentState) *ExperimentDetails { - experiments := make([]ExperimentVariantStats, 0, len(state.Counts)) - for name, variants := range state.Counts { - total := 0 - for _, c := range variants { - total += c - } - experiments = append(experiments, ExperimentVariantStats{ - Name: name, - Variants: variants, - Total: total, - }) - } - slices.SortFunc(experiments, func(a, b ExperimentVariantStats) int { - switch { - case a.Name < b.Name: - return -1 - case a.Name > b.Name: - return 1 - default: - return 0 - } - }) - - recentRuns := state.Runs - const maxRecentRuns = 10 - if len(recentRuns) > maxRecentRuns { - recentRuns = recentRuns[len(recentRuns)-maxRecentRuns:] - } - - return &ExperimentDetails{ - WorkflowID: workflowID, - Branch: branchName, - TotalRuns: experimentTotalRuns(state), - Experiments: experiments, - RecentRuns: recentRuns, - } -} - -// experimentTotalRuns returns the total number of runs recorded in the state. -// Prefers the runs array length when non-empty; falls back to summing all variant counts. -func experimentTotalRuns(state *ExperimentState) int { - if len(state.Runs) > 0 { - return len(state.Runs) - } - total := 0 - for _, variants := range state.Counts { - for _, c := range variants { - total += c - } - } - return total -} - -// experimentLastRun returns the date (YYYY-MM-DD) of the most recent run, or "" if unknown. -func experimentLastRun(state *ExperimentState) string { - if len(state.Runs) == 0 { - return "" - } - ts := state.Runs[len(state.Runs)-1].Timestamp - if len(ts) >= 10 { - return ts[:10] - } - return ts -} - -// extractExperimentName extracts the workflow ID from a branch ref. -// -// "origin/experiments/my-workflow" → "my-workflow" -// "experiments/my-workflow" → "my-workflow" -// "experiments/" → "" (bare prefix, rejected by callers) -func extractExperimentName(ref string) string { - ref = strings.TrimPrefix(ref, "origin/") - if !strings.HasPrefix(ref, experimentsBranchPrefix) { - return "" - } - // An empty result here (bare "experiments/" ref) is acceptable: callers - // guard against empty workflow IDs with `if workflowID == ""` checks. - return strings.TrimPrefix(ref, experimentsBranchPrefix) -} - -// gitRefExists reports whether an experiments/evals state ref exists locally. -func gitRefExists(ref string) bool { - if !isSafeExperimentStateRef(ref) { - return false - } - cmd := exec.Command("git", "rev-parse", "--verify", ref) - return cmd.Run() == nil -} - -// printExperimentDetails renders experiment details to stderr in human-readable form. -func printExperimentDetails(d *ExperimentDetails) { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Experiment workflow: "+d.WorkflowID)) - fmt.Fprintf(os.Stderr, " Branch: %s\n", d.Branch) - fmt.Fprintf(os.Stderr, " Total runs: %d\n", d.TotalRuns) - - if len(d.Experiments) > 0 { - for _, exp := range d.Experiments { - // Sort variants for deterministic display. - type kv struct { - k string - v int - } - pairs := make([]kv, 0, len(exp.Variants)) - for k, v := range exp.Variants { - pairs = append(pairs, kv{k, v}) - } - slices.SortFunc(pairs, func(a, b kv) int { - switch { - case a.k < b.k: - return -1 - case a.k > b.k: - return 1 - default: - return 0 - } - }) - rows := make([][]string, 0, len(pairs)) - for _, p := range pairs { - pct := 0 - if exp.Total > 0 { - pct = p.v * 100 / exp.Total - } - rows = append(rows, []string{p.k, strconv.Itoa(p.v), strconv.Itoa(pct) + "%"}) - } - if len(rows) > 0 { - fmt.Fprintf(os.Stderr, "\n%s", console.RenderTable(console.TableConfig{ - Title: fmt.Sprintf("%s (total: %d)", exp.Name, exp.Total), - Headers: []string{"Variant", "Count", "Percent"}, - Rows: rows, - TTYFunc: tty.IsStderrTerminal, - })) - } - } - } else { - fmt.Fprintln(os.Stderr, "\nNo experiment data found (state.jsonl/state.json not present or empty).") - } - - printExperimentAnalyses(d.Analyses) - - if len(d.RecentRuns) > 0 { - rows := make([][]string, 0, len(d.RecentRuns)) - for _, run := range d.RecentRuns { - date := run.Timestamp - if len(date) >= 10 { - date = date[:10] - } - rows = append(rows, []string{date, run.RunID, formatAssignments(run.Assignments)}) - } - fmt.Fprintf(os.Stderr, "\n%s", console.RenderTable(console.TableConfig{ - Title: "Recent runs", - Headers: []string{"Date", "Run ID", "Assignments"}, - Rows: rows, - TTYFunc: tty.IsStderrTerminal, - })) - } -} - -// formatAssignments formats a map of experiment→variant as "k=v, k=v" sorted by key. -func formatAssignments(assignments map[string]string) string { - if len(assignments) == 0 { - return "-" - } - keys := sliceutil.SortedKeys(assignments) - parts := make([]string, 0, len(keys)) - for _, k := range keys { - parts = append(parts, k+"="+assignments[k]) - } - return strings.Join(parts, ", ") -} - -// parsePagedJSONArray parses multiple JSON arrays (one per page from --paginate) -// concatenated in the output and returns a merged slice. -func parsePagedJSONArray[T any](output string) ([]T, error) { - var result []T - decoder := json.NewDecoder(strings.NewReader(output)) - for { - var page []T - if err := decoder.Decode(&page); err != nil { - if errors.Is(err, io.EOF) { - break - } - return nil, err - } - result = append(result, page...) - } - return result, nil -} diff --git a/pkg/cli/experiments_command_test.go b/pkg/cli/experiments_command_test.go index 033042c1484..be38b759d27 100644 --- a/pkg/cli/experiments_command_test.go +++ b/pkg/cli/experiments_command_test.go @@ -79,6 +79,12 @@ func TestBuildSafeGitShowObjectArg(t *testing.T) { fileName: "-n", shouldErr: true, }, + { + name: "rejects control character in file name", + ref: "origin/experiments/my-feature", + fileName: "state\x01.jsonl", + shouldErr: true, + }, } for _, tt := range tests { @@ -291,6 +297,32 @@ func TestParseExperimentStateJSONLBaselineCounts(t *testing.T) { assert.Len(t, state.Runs, 2) } +func TestAppendExperimentRunAddsAssignmentToBaseline(t *testing.T) { + state := emptyExperimentState() + appendExperimentRun(state, ExperimentRunRecord{ + RunID: "1", + Timestamp: "2026-08-18T12:00:00Z", + Assignments: map[string]string{"feature": "A"}, + BaselineCounts: map[string]map[string]int{ + "feature": {"A": 2, "B": 1}, + }, + }) + + assert.Equal(t, map[string]map[string]int{ + "feature": {"A": 3, "B": 1}, + }, state.Counts) +} + +func TestParseExperimentStateJSONLSnapshotDiscardsEarlierRuns(t *testing.T) { + state := parseExperimentState([]byte(`{"run_id":"before","timestamp":"2026-08-18T10:00:00Z","assignments":{"feature":"A"}} +{"counts":{"feature":{"B":2}}} +{"run_id":"after","timestamp":"2026-08-18T12:00:00Z","assignments":{"feature":"B"}}`)) + + assert.Equal(t, map[string]map[string]int{"feature": {"B": 3}}, state.Counts) + require.Len(t, state.Runs, 1) + assert.Equal(t, "after", state.Runs[0].RunID) +} + func TestExperimentTotalRunsFallback(t *testing.T) { // When no runs array present, sum variant counts. state := &ExperimentState{ diff --git a/pkg/cli/experiments_fetch.go b/pkg/cli/experiments_fetch.go new file mode 100644 index 00000000000..45c0200d39f --- /dev/null +++ b/pkg/cli/experiments_fetch.go @@ -0,0 +1,433 @@ +package cli + +import ( + "bufio" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "net/url" + "os" + "os/exec" + "path/filepath" + "strings" + + "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/errorutil" + "github.com/github/gh-aw/pkg/parser" + "github.com/github/gh-aw/pkg/setutil" + "github.com/github/gh-aw/pkg/workflow" +) + +type evalResultRecord struct { + ID string `json:"id"` + Answer string `json:"answer"` + RunID string `json:"runid"` + Timestamp string `json:"timestamp"` +} + +func loadLocalMetricEvalResults(workflowID string) map[string]MetricEvalResults { + branchName := workflow.WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID) + ref := "origin/" + branchName + if !gitRefExists(ref) { + if !gitRefExists(branchName) { + return nil + } + ref = branchName + } + if !isSafeExperimentStateRef(ref) { + experimentsLog.Printf("Rejecting unsafe git ref: %q", ref) + return nil + } + objectArg, err := buildSafeGitShowObjectArg(ref, constants.EvalsResultFilename) + if err != nil { + experimentsLog.Printf("Rejecting unsafe git show argument (ref=%q file=%q): %v", ref, constants.EvalsResultFilename, err) + return nil + } + cmd := exec.Command("git", "show", objectArg) + out, err := cmd.Output() + if err != nil { + return nil + } + return summarizeMetricEvalResults(out) +} + +func loadRemoteMetricEvalResults(repoOverride, workflowID string) map[string]MetricEvalResults { + branchName := workflow.WorkflowStateBranchName(constants.EvalsBranchPrefix, workflowID) + decoded, err := readRemoteRepoBranchFile(repoOverride, branchName, constants.EvalsResultFilename, "") + if err != nil { + return nil + } + return summarizeMetricEvalResults(decoded) +} + +func summarizeMetricEvalResults(data []byte) map[string]MetricEvalResults { + scanner := bufio.NewScanner(strings.NewReader(string(data))) + results := map[string]MetricEvalResults{} + for scanner.Scan() { + line := strings.TrimSpace(scanner.Text()) + if line == "" { + continue + } + var record evalResultRecord + if err := json.Unmarshal([]byte(line), &record); err != nil { + continue + } + if record.ID == "" { + continue + } + summary := results[record.ID] + summary.Total++ + switch strings.ToUpper(strings.TrimSpace(record.Answer)) { + case "YES": + summary.Yes++ + case "NO": + summary.No++ + default: + summary.Unknown++ + } + summary.LatestAnswer = strings.ToUpper(strings.TrimSpace(record.Answer)) + summary.LatestRunID = record.RunID + results[record.ID] = summary + } + if len(results) == 0 { + return nil + } + return results +} + +// experimentFrontmatterResult holds both the experiment configs and evals config parsed +// from a workflow's frontmatter. +type experimentFrontmatterResult struct { + ExperimentConfigs map[string]*workflow.ExperimentConfig + Evals *workflow.EvalsConfig +} + +// loadLocalExperimentConfigs reads the workflow .md file for the given experiment name +// and returns the ExperimentConfig map and EvalsConfig from its frontmatter. +// experimentName is the sanitized workflow ID (the part after "experiments/" in the branch name). +// Returns a zero-value result when the workflow file cannot be found or parsed. +func loadLocalExperimentConfigs(experimentName string) experimentFrontmatterResult { + 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 experimentFrontmatterResult{} + } + + // Verify that the resolved path is within .github/workflows/ to prevent path traversal. + // findWorkflowFileForExperiment returns paths from filepath.Glob with a relative base dir, + // so convert both sides to absolute paths before the prefix check. + absFilePath, err := filepath.Abs(filePath) + if err != nil { + experimentsLog.Printf("Failed to resolve absolute path for %s: %v", filePath, err) + return experimentFrontmatterResult{} + } + workflowsDir, err := filepath.Abs(getWorkflowsDir()) + if err != nil { + experimentsLog.Printf("Failed to resolve workflows dir: %v", err) + return experimentFrontmatterResult{} + } + if !strings.HasPrefix(absFilePath, workflowsDir+string(filepath.Separator)) { + experimentsLog.Printf("Refusing to read workflow file outside .github/workflows/: %s", absFilePath) + return experimentFrontmatterResult{} + } + + content, err := os.ReadFile(absFilePath) // #nosec G304 -- path confirmed within .github/workflows/ + if err != nil { + experimentsLog.Printf("Failed to read workflow file %s: %v", absFilePath, err) + return experimentFrontmatterResult{} + } + + result, err := parser.ExtractFrontmatterFromContent(string(content)) + if err != nil { + experimentsLog.Printf("Failed to parse frontmatter from %s: %v", filePath, err) + return experimentFrontmatterResult{} + } + + cfg, err := workflow.ParseFrontmatterConfig(result.Frontmatter) + if err != nil { + experimentsLog.Printf("Failed to parse frontmatter config from %s: %v", filePath, err) + return experimentFrontmatterResult{} + } + + evals, err := workflow.ParseEvalsFromFrontmatter(result.Frontmatter) + if err != nil { + experimentsLog.Printf("Failed to parse evals config from %s: %v", filePath, err) + // Non-fatal: continue without evals resolution. + } + + return experimentFrontmatterResult{ + ExperimentConfigs: cfg.ExperimentConfigs, + Evals: evals, + } +} + +// loadRemoteExperimentConfigs fetches the workflow .md file from the repository default branch +// via the GitHub API and returns the ExperimentConfig map and EvalsConfig from its frontmatter. +// Returns a zero-value result when the file cannot be fetched or parsed. +func loadRemoteExperimentConfigs(repoOverride, experimentName string) experimentFrontmatterResult { + experimentsLog.Printf("Loading remote experiment configs for %s from %s", experimentName, repoOverride) + + // Build the candidate list. First, use the directory listing to find the exact filename + // whose sanitized basename matches experimentName (e.g. "ci-coach" for "cicoach"). + // Fall back to the bare experiment name if the listing is unavailable. + candidates := workflowFileCandidates(experimentName) + if resolved := findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName); resolved != "" && resolved != experimentName { + // Prepend the resolved name so it is tried before the bare sanitized form. + // Skip when resolved == experimentName to avoid a redundant fetch. + candidates = append([]string{resolved}, candidates...) + } + + for _, candidate := range candidates { + apiPath := constants.WorkflowsDirSlash + 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 + } + + evals, err := workflow.ParseEvalsFromFrontmatter(result.Frontmatter) + if err != nil { + experimentsLog.Printf("Failed to parse evals config from %s: %v", apiPath, err) + // Non-fatal: continue without evals resolution. + } + + if len(cfg.ExperimentConfigs) > 0 { + experimentsLog.Printf("Loaded remote configs from %s", apiPath) + return experimentFrontmatterResult{ + ExperimentConfigs: cfg.ExperimentConfigs, + Evals: evals, + } + } + } + + experimentsLog.Printf("No remote workflow file found for experiment %s", experimentName) + return experimentFrontmatterResult{} +} + +// findRemoteWorkflowFilenameForExperiment lists .md files in .github/workflows/ via the +// GitHub API and returns the basename (without .md) of the first file whose sanitized name +// matches experimentName. This mirrors findWorkflowFileForExperiment for remote repos. +// Returns "" when the directory cannot be listed or no match is found. +func findRemoteWorkflowFilenameForExperiment(repoOverride, experimentName string) string { + args := []string{"api", + "repos/{owner}/{repo}/contents/.github/workflows", + "--jq", `[.[] | select(.name | endswith(".md")) | .name]`, + "--repo", repoOverride, + } + cmd := workflow.ExecGH(args...) + out, err := cmd.Output() + if err != nil { + experimentsLog.Printf("Failed to list remote workflow files from %s: %v", repoOverride, err) + return "" + } + + var filenames []string + if err := json.Unmarshal(out, &filenames); err != nil { + experimentsLog.Printf("Failed to parse remote workflow file listing: %v", err) + return "" + } + + return matchWorkflowFilenameByExperiment(filenames, experimentName) +} + +// matchWorkflowFilenameByExperiment returns the basename (without .md) of the first file in +// filenames whose sanitized name matches experimentName. Returns "" when no match is found. +// Logs a warning when more than one file maps to the same sanitized name. +// +// Note: normalizeWorkflowID calls filepath.Base internally, so any path prefix in filenames +// is stripped before matching. Callers that supply bare filenames (e.g. "my-flow.md") are +// unaffected; callers supplying full paths (e.g. ".github/workflows/my-flow.md") will have +// the directory component removed — only the basename is returned and compared. +func matchWorkflowFilenameByExperiment(filenames []string, experimentName string) string { + var matches []string + for _, filename := range filenames { + base := normalizeWorkflowID(filename) + if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { + matches = append(matches, base) + } + } + if len(matches) == 0 { + return "" + } + if len(matches) > 1 { + experimentsLog.Printf("Ambiguous experiment name %q: multiple workflow files match (%s); using first", experimentName, strings.Join(matches, ", ")) + } + return matches[0] +} + +// 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 := normalizeWorkflowID(f) + if workflow.SanitizeWorkflowIDForCacheKey(base) == experimentName { + return f + } + } + return "" +} + +// workflowFileCandidates returns a fallback list of candidate workflow file basenames (without .md) +// for remote lookups when the directory listing is unavailable. The sanitized form +// (hyphens removed, lowercased) is irreversible, so only the experiment name itself is +// returned here. The caller should prefer findRemoteWorkflowFilenameForExperiment which +// resolves the real filename by scanning the remote directory. +func workflowFileCandidates(experimentName string) []string { + return []string{experimentName} +} + +// fetchLocalExperiments lists experiment branches and reads their state from the local git repo. +func fetchLocalExperiments() ([]ExperimentInfo, error) { + experimentsLog.Print("Fetching local experiment branches via git for-each-ref") + + cmd := exec.Command("git", "for-each-ref", + "--sort=-committerdate", + "--format=%(refname:short)", + "refs/remotes/origin/"+experimentsBranchPrefix+"*", + "refs/heads/"+experimentsBranchPrefix+"*", + ) + output, err := cmd.Output() + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { + return []ExperimentInfo{}, nil + } + return nil, fmt.Errorf("failed to list experiment branches: %w", err) + } + + seen := make(map[string]struct { + }) + var experiments []ExperimentInfo + + for line := range strings.SplitSeq(strings.TrimSpace(string(output)), "\n") { + if line == "" { + continue + } + workflowID := extractExperimentName(line) + if workflowID == "" || setutil.Contains(seen, workflowID) { + continue + } + seen[workflowID] = struct { + }{} + + branchName := experimentsBranchPrefix + workflowID + // Prefer remote ref; fall back to local. + ref := "origin/" + branchName + if !gitRefExists(ref) { + ref = branchName + } + state := readLocalExperimentState(ref) + experiments = append(experiments, experimentInfoFromState(workflowID, branchName, state)) + } + + return experiments, nil +} + +// fetchRemoteExperiments lists experiment branches and reads their state via the GitHub API. +func fetchRemoteExperiments(repoOverride string) ([]ExperimentInfo, error) { + experimentsLog.Printf("Fetching remote experiment branches: repo=%s", repoOverride) + + args := []string{"api", "repos/{owner}/{repo}/branches", + "--paginate", + "--jq", `[.[] | select(.name | startswith("` + experimentsBranchPrefix + `")) | .name]`, + "--repo", repoOverride, + } + cmd := workflow.ExecGH(args...) + output, err := cmd.Output() + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + return nil, fmt.Errorf("failed to fetch branches (exit %d): %s", exitErr.ExitCode(), strings.TrimSpace(string(exitErr.Stderr))) + } + return nil, fmt.Errorf("failed to fetch branches: %w", err) + } + + branchNames, err := parsePagedJSONArray[string](string(output)) + if err != nil { + return nil, fmt.Errorf("failed to parse branch list: %w", err) + } + + var experiments []ExperimentInfo + for _, branchName := range branchNames { + workflowID := strings.TrimPrefix(branchName, experimentsBranchPrefix) + state := readRemoteExperimentState(repoOverride, branchName) + experiments = append(experiments, experimentInfoFromState(workflowID, branchName, state)) + } + + return experiments, nil +} + +// fetchLocalExperimentDetails reads experiment state from a local experiment branch. +func fetchLocalExperimentDetails(branchName, workflowID string) (*ExperimentDetails, error) { + experimentsLog.Printf("Fetching local experiment details: branch=%s", branchName) + + ref := "origin/" + branchName + if !gitRefExists(ref) { + if !gitRefExists(branchName) { + return nil, fmt.Errorf("experiment branch %q not found locally (tried origin/%s and %s)", + branchName, branchName, branchName) + } + ref = branchName + } + + state := readLocalExperimentState(ref) + return experimentDetailsFromState(workflowID, branchName, state), nil +} + +// fetchRemoteExperimentDetails reads experiment state from a remote experiment branch. +func fetchRemoteExperimentDetails(repoOverride, branchName, workflowID string) (*ExperimentDetails, error) { + experimentsLog.Printf("Fetching remote experiment details: repo=%s, branch=%s", repoOverride, branchName) + + // Verify the branch exists. + encodedBranch := url.PathEscape(branchName) + checkArgs := []string{"api", + "repos/{owner}/{repo}/branches/" + encodedBranch, + "--jq", ".name", + "--repo", repoOverride, + } + checkCmd := workflow.ExecGH(checkArgs...) + if _, err := checkCmd.Output(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + stderr := strings.TrimSpace(string(exitErr.Stderr)) + if errorutil.IsNotFoundOutput(stderr) { + return nil, fmt.Errorf("experiment %q not found in %s", workflowID, repoOverride) + } + return nil, fmt.Errorf("failed to fetch experiment branch (exit %d): %s", exitErr.ExitCode(), stderr) + } + return nil, fmt.Errorf("failed to fetch experiment branch: %w", err) + } + + state := readRemoteExperimentState(repoOverride, branchName) + return experimentDetailsFromState(workflowID, branchName, state), nil +} diff --git a/pkg/cli/experiments_fetch_test.go b/pkg/cli/experiments_fetch_test.go new file mode 100644 index 00000000000..478344e5148 --- /dev/null +++ b/pkg/cli/experiments_fetch_test.go @@ -0,0 +1,60 @@ +//go:build !integration + +package cli + +import ( + "encoding/base64" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func installExperimentFetchFakeGH(t *testing.T, workflowContent, state string) { + t.Helper() + binDir := t.TempDir() + script := fmt.Sprintf(`#!/bin/sh +case "$*" in + *"/branches/experiments%%2Fci-coach"*) echo "experiments/ci-coach" ;; + *"contents/.github/workflows"*) echo '["ci-coach.md"]' ;; + *"ci-coach.md"*) echo %q ;; + *"state.jsonl"*) echo %q ;; + *) exit 1 ;; +esac +`, base64.StdEncoding.EncodeToString([]byte(workflowContent)), base64.StdEncoding.EncodeToString([]byte(state))) + require.NoError(t, os.WriteFile(filepath.Join(binDir, "gh"), []byte(script), 0o755)) + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("GH_TOKEN", "") + t.Setenv("GITHUB_TOKEN", "") +} + +func TestLoadRemoteExperimentConfigsResolvesWorkflowFilename(t *testing.T) { + installExperimentFetchFakeGH(t, `--- +experiments: + style: + variants: [concise, detailed] +--- +# Workflow +`, "") + + result := loadRemoteExperimentConfigs("octo/repo", "cicoach") + + require.Contains(t, result.ExperimentConfigs, "style") + assert.Equal(t, []string{"concise", "detailed"}, result.ExperimentConfigs["style"].Variants) +} + +func TestFetchRemoteExperimentDetailsLoadsState(t *testing.T) { + installExperimentFetchFakeGH(t, "", `{"run_id":"1","timestamp":"2026-08-18T12:00:00Z","assignments":{"style":"concise"}}`) + + details, err := fetchRemoteExperimentDetails("octo/repo", "experiments/ci-coach", "ci-coach") + + require.NoError(t, err) + assert.Equal(t, "ci-coach", details.WorkflowID) + assert.Equal(t, 1, details.TotalRuns) + require.Len(t, details.Experiments, 1) + assert.Equal(t, "style", details.Experiments[0].Name) + assert.Equal(t, map[string]int{"concise": 1}, details.Experiments[0].Variants) +} diff --git a/pkg/cli/experiments_git_safety.go b/pkg/cli/experiments_git_safety.go new file mode 100644 index 00000000000..48c624d7025 --- /dev/null +++ b/pkg/cli/experiments_git_safety.go @@ -0,0 +1,110 @@ +package cli + +import ( + "errors" + "os/exec" + "path" + "strings" +) + +// buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts +// before joining them, preventing flag and path-traversal style injections. +func buildSafeGitShowObjectArg(ref, fileName string) (string, error) { + if !isSafeExperimentStateRef(ref) { + return "", errors.New("unsafe git ref") + } + if !isSafeGitTreePath(fileName) { + return "", errors.New("unsafe git tree path") + } + return ref + ":" + fileName, nil +} + +// gitRefExists reports whether an experiments/evals state ref exists locally. +func gitRefExists(ref string) bool { + if !isSafeExperimentStateRef(ref) { + return false + } + return exec.Command("git", "rev-parse", "--verify", ref).Run() == nil +} + +func isSafeExperimentStateRef(ref string) bool { + if !isSafeGitRevisionArg(ref) { + return false + } + + // Allow direct object IDs (including abbreviated prefixes) for future callers + // while rejecting revision operators. + if isHexObjectIDPrefix(ref) { + return true + } + + trimmed := strings.TrimPrefix(ref, "origin/") + if !strings.HasPrefix(trimmed, experimentsBranchPrefix) && !strings.HasPrefix(trimmed, evalsBranchPrefix) { + return false + } + + return isSafeGitRefName(trimmed) +} + +func isHexObjectIDPrefix(ref string) bool { + // Require >=7 chars to avoid accepting short hex-like experiment names as SHAs. + // 64 keeps compatibility with SHA-256 object IDs. + if len(ref) < 7 || len(ref) > 64 { + return false + } + for _, r := range ref { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') { + return false + } + } + return true +} + +// isSafeGitRefName validates a refname with check-ref-format-equivalent rules. +func isSafeGitRefName(ref string) bool { + hasInvalidShape := ref == "" || + strings.HasPrefix(ref, "/") || + strings.HasSuffix(ref, "/") || + strings.HasSuffix(ref, ".") + hasInvalidSequences := strings.Contains(ref, "//") || + strings.Contains(ref, "..") || + strings.Contains(ref, "@{") || + strings.Contains(ref, "\\") + if hasInvalidShape || hasInvalidSequences { + return false + } + + for part := range strings.SplitSeq(ref, "/") { + if part == "" || strings.HasPrefix(part, ".") || strings.HasSuffix(part, ".lock") { + return false + } + for _, r := range part { + if r <= ' ' || r == '~' || r == '^' || r == ':' || r == '?' || r == '*' || r == '[' || r == '\x7f' { + return false + } + } + } + return true +} + +// isSafeGitTreePath validates a git tree entry path used in "ref:path" syntax. +// Git tree paths always use forward slashes across platforms, so this intentionally +// uses the slash-based path package (not filepath) for normalization checks. +func isSafeGitTreePath(fileName string) bool { + if fileName == "" || strings.HasPrefix(fileName, "-") { + return false + } + if path.IsAbs(fileName) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") { + return false + } + for _, r := range fileName { + if r < 0x20 || r == 0x7f { + return false + } + } + clean := path.Clean(fileName) + if clean == "." || clean == ".." || strings.HasPrefix(clean, "../") { + return false + } + return clean == fileName +} diff --git a/pkg/cli/experiments_json_utils.go b/pkg/cli/experiments_json_utils.go new file mode 100644 index 00000000000..0e6a8f4041c --- /dev/null +++ b/pkg/cli/experiments_json_utils.go @@ -0,0 +1,24 @@ +package cli + +import ( + "encoding/json" + "errors" + "io" + "strings" +) + +func parsePagedJSONArray[T any](output string) ([]T, error) { + var result []T + decoder := json.NewDecoder(strings.NewReader(output)) + for { + var page []T + if err := decoder.Decode(&page); err != nil { + if errors.Is(err, io.EOF) { + break + } + return nil, err + } + result = append(result, page...) + } + return result, nil +} diff --git a/pkg/cli/experiments_render.go b/pkg/cli/experiments_render.go new file mode 100644 index 00000000000..6104c0cc8d4 --- /dev/null +++ b/pkg/cli/experiments_render.go @@ -0,0 +1,86 @@ +package cli + +import ( + "fmt" + "os" + "slices" + "strconv" + "strings" + + "github.com/github/gh-aw/pkg/console" + "github.com/github/gh-aw/pkg/sliceutil" + "github.com/github/gh-aw/pkg/tty" +) + +func printExperimentDetails(d *ExperimentDetails) { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Experiment workflow: "+d.WorkflowID)) + fmt.Fprintf(os.Stderr, " Branch: %s\n", d.Branch) + fmt.Fprintf(os.Stderr, " Total runs: %d\n", d.TotalRuns) + + if len(d.Experiments) > 0 { + for _, exp := range d.Experiments { + // Sort variants for deterministic display. + type kv struct { + k string + v int + } + pairs := make([]kv, 0, len(exp.Variants)) + for k, v := range exp.Variants { + pairs = append(pairs, kv{k, v}) + } + slices.SortFunc(pairs, func(a, b kv) int { + return strings.Compare(a.k, b.k) + }) + rows := make([][]string, 0, len(pairs)) + for _, p := range pairs { + pct := 0 + if exp.Total > 0 { + pct = p.v * 100 / exp.Total + } + rows = append(rows, []string{p.k, strconv.Itoa(p.v), strconv.Itoa(pct) + "%"}) + } + if len(rows) > 0 { + fmt.Fprintf(os.Stderr, "\n%s", console.RenderTable(console.TableConfig{ + Title: fmt.Sprintf("%s (total: %d)", exp.Name, exp.Total), + Headers: []string{"Variant", "Count", "Percent"}, + Rows: rows, + TTYFunc: tty.IsStderrTerminal, + })) + } + } + } else { + fmt.Fprintln(os.Stderr, "\nNo experiment data found (state.jsonl/state.json not present or empty).") + } + + printExperimentAnalyses(d.Analyses) + + if len(d.RecentRuns) > 0 { + rows := make([][]string, 0, len(d.RecentRuns)) + for _, run := range d.RecentRuns { + date := run.Timestamp + if len(date) >= 10 { + date = date[:10] + } + rows = append(rows, []string{date, run.RunID, formatAssignments(run.Assignments)}) + } + fmt.Fprintf(os.Stderr, "\n%s", console.RenderTable(console.TableConfig{ + Title: "Recent runs", + Headers: []string{"Date", "Run ID", "Assignments"}, + Rows: rows, + TTYFunc: tty.IsStderrTerminal, + })) + } +} + +// formatAssignments formats a map of experiment→variant as "k=v, k=v" sorted by key. +func formatAssignments(assignments map[string]string) string { + if len(assignments) == 0 { + return "-" + } + keys := sliceutil.SortedKeys(assignments) + parts := make([]string, 0, len(keys)) + for _, k := range keys { + parts = append(parts, k+"="+assignments[k]) + } + return strings.Join(parts, ", ") +} diff --git a/pkg/cli/experiments_render_test.go b/pkg/cli/experiments_render_test.go new file mode 100644 index 00000000000..906cf57fb63 --- /dev/null +++ b/pkg/cli/experiments_render_test.go @@ -0,0 +1,71 @@ +//go:build !integration + +package cli + +import ( + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func captureExperimentDetailsStderr(t *testing.T, details *ExperimentDetails) string { + t.Helper() + original := os.Stderr + reader, writer, err := os.Pipe() + require.NoError(t, err) + os.Stderr = writer + defer func() { + os.Stderr = original + _ = writer.Close() + _ = reader.Close() + }() + + printExperimentDetails(details) + require.NoError(t, writer.Close()) + os.Stderr = original + + output, err := io.ReadAll(reader) + require.NoError(t, err) + return string(output) +} + +func TestPrintExperimentDetailsEmpty(t *testing.T) { + output := captureExperimentDetailsStderr(t, &ExperimentDetails{ + WorkflowID: "empty-workflow", + Branch: "experiments/empty-workflow", + }) + + assert.Contains(t, output, "Experiment workflow: empty-workflow") + assert.Contains(t, output, "Branch: experiments/empty-workflow") + assert.Contains(t, output, "Total runs: 0") + assert.Contains(t, output, "No experiment data found") +} + +func TestPrintExperimentDetailsPopulated(t *testing.T) { + output := captureExperimentDetailsStderr(t, &ExperimentDetails{ + WorkflowID: "test-workflow", + Branch: "experiments/test-workflow", + TotalRuns: 2, + Experiments: []ExperimentVariantStats{{ + Name: "style", + Variants: map[string]int{"verbose": 1, "concise": 1}, + Total: 2, + }}, + RecentRuns: []ExperimentRunRecord{{ + RunID: "123", + Timestamp: "2026-08-18T12:00:00Z", + Assignments: map[string]string{"style": "concise"}, + }}, + }) + + assert.Contains(t, output, "style (total: 2)") + assert.Contains(t, output, "concise") + assert.Contains(t, output, "verbose") + assert.Contains(t, output, "50%") + assert.Contains(t, output, "Recent runs") + assert.Contains(t, output, "2026-08-18") + assert.Contains(t, output, "style=concise") +} diff --git a/pkg/cli/experiments_state.go b/pkg/cli/experiments_state.go new file mode 100644 index 00000000000..618157104c1 --- /dev/null +++ b/pkg/cli/experiments_state.go @@ -0,0 +1,237 @@ +package cli + +import ( + "encoding/json" + "os/exec" + "slices" + "strings" + + "github.com/github/gh-aw/pkg/constants" +) + +const experimentsBranchPrefix = "experiments/" +const evalsBranchPrefix = constants.EvalsBranchPrefix + "/" + +type ExperimentState struct { + Counts map[string]map[string]int `json:"counts"` // experiment name → variant → count + Runs []ExperimentRunRecord `json:"runs,omitempty"` +} + +// ExperimentRunRecord represents a single workflow run in the JSONL ledger. +type ExperimentRunRecord struct { + RunID string `json:"run_id"` + Timestamp string `json:"timestamp"` + Assignments map[string]string `json:"assignments"` + BaselineCounts map[string]map[string]int `json:"baseline_counts,omitempty"` +} + +// ExperimentVariantStats holds counts for all variants of one named A/B experiment. +type ExperimentVariantStats struct { + Name string `json:"name"` + Variants map[string]int `json:"variants"` // variant → count + Total int `json:"total"` +} + +// ExperimentInfo represents a single experiment workflow for list output. +type ExperimentInfo struct { + WorkflowID string `json:"workflow_id" console:"header:Workflow"` + Branch string `json:"branch" console:"header:Branch"` + Experiments int `json:"experiments" console:"header:Experiments"` + TotalRuns int `json:"total_runs" console:"header:Total Runs"` + LastRun string `json:"last_run" console:"header:Last Run"` +} + +// ExperimentDetails represents detailed information about a specific experiment workflow. +type ExperimentDetails struct { + WorkflowID string `json:"workflow_id"` + Branch string `json:"branch"` + TotalRuns int `json:"total_runs"` + Experiments []ExperimentVariantStats `json:"experiments"` + RecentRuns []ExperimentRunRecord `json:"recent_runs,omitempty"` + // Analyses holds the statistical analysis for each named experiment. + // Populated by RunExperimentsAnalyze; absent in list output. + Analyses []ExperimentAnalysis `json:"analyses,omitempty"` +} + +func experimentStateFilenames() []string { + return []string{"state.jsonl", "state.json"} +} + +// readLocalExperimentState reads experiment state from a local git ref (e.g. "origin/experiments/foo"). +// Returns an empty state when the file is absent or cannot be parsed. +func readLocalExperimentState(ref string) *ExperimentState { + for _, fileName := range experimentStateFilenames() { + objectArg, err := buildSafeGitShowObjectArg(ref, fileName) + if err != nil { + experimentsLog.Printf("Skipping unsafe git show argument (ref=%q file=%q): %v", ref, fileName, err) + continue + } + cmd := exec.Command("git", "show", objectArg) + out, err := cmd.Output() + if err == nil { + return parseExperimentState(out) + } + } + return emptyExperimentState() +} + +// readRemoteExperimentState reads state.jsonl or state.json from a remote experiment branch. +// Returns an empty state when neither file can be read or parsed. +func readRemoteExperimentState(repoOverride, branchName string) *ExperimentState { + for _, fileName := range experimentStateFilenames() { + decoded, err := readRemoteRepoBranchFile(repoOverride, branchName, fileName, "") + if err == nil { + return parseExperimentState(decoded) + } + } + return emptyExperimentState() +} + +func appendExperimentRun(state *ExperimentState, run ExperimentRunRecord) { + if state.Counts == nil { + state.Counts = map[string]map[string]int{} + } + for name, variants := range run.BaselineCounts { + if state.Counts[name] == nil { + state.Counts[name] = map[string]int{} + } + for variant, count := range variants { + state.Counts[name][variant] += count + } + } + for name, variant := range run.Assignments { + if state.Counts[name] == nil { + state.Counts[name] = map[string]int{} + } + // BaselineCounts captures totals before this run, so the assigned + // variant is intentionally added as the current run. + state.Counts[name][variant]++ + } + state.Runs = append(state.Runs, run) +} + +func parseExperimentStateJSONL(data []byte) *ExperimentState { + state := emptyExperimentState() + for line := range strings.SplitSeq(string(data), "\n") { + line = strings.TrimSpace(line) + if line == "" { + continue + } + + var snapshot ExperimentState + if err := json.Unmarshal([]byte(line), &snapshot); err == nil && snapshot.Counts != nil { + // A snapshot is a cumulative checkpoint, so it discards preceding run records. + state = &snapshot + continue + } + + var run ExperimentRunRecord + if err := json.Unmarshal([]byte(line), &run); err != nil || run.RunID == "" || run.Timestamp == "" || len(run.Assignments) == 0 { + experimentsLog.Printf("parseExperimentStateJSONL: skipping unrecognized line") + continue + } + appendExperimentRun(state, run) + } + return state +} + +// parseExperimentState unmarshals raw JSON or JSONL into an ExperimentState. +// Returns an empty state when parsing fails or the data is invalid. +func parseExperimentState(data []byte) *ExperimentState { + var state ExperimentState + if err := json.Unmarshal(data, &state); err == nil && state.Counts != nil { + return &state + } + return parseExperimentStateJSONL(data) +} + +// emptyExperimentState returns a zero-value ExperimentState with an initialised Counts map. +func emptyExperimentState() *ExperimentState { + return &ExperimentState{Counts: map[string]map[string]int{}} +} + +// experimentInfoFromState builds an ExperimentInfo summary from experiment state. +func experimentInfoFromState(workflowID, branchName string, state *ExperimentState) ExperimentInfo { + return ExperimentInfo{ + WorkflowID: workflowID, + Branch: branchName, + Experiments: len(state.Counts), + TotalRuns: experimentTotalRuns(state), + LastRun: experimentLastRun(state), + } +} + +// experimentDetailsFromState builds ExperimentDetails from experiment state. +func experimentDetailsFromState(workflowID, branchName string, state *ExperimentState) *ExperimentDetails { + experiments := make([]ExperimentVariantStats, 0, len(state.Counts)) + for name, variants := range state.Counts { + total := 0 + for _, c := range variants { + total += c + } + experiments = append(experiments, ExperimentVariantStats{ + Name: name, + Variants: variants, + Total: total, + }) + } + slices.SortFunc(experiments, func(a, b ExperimentVariantStats) int { + return strings.Compare(a.Name, b.Name) + }) + + recentRuns := state.Runs + const maxRecentRuns = 10 + if len(recentRuns) > maxRecentRuns { + recentRuns = recentRuns[len(recentRuns)-maxRecentRuns:] + } + + return &ExperimentDetails{ + WorkflowID: workflowID, + Branch: branchName, + TotalRuns: experimentTotalRuns(state), + Experiments: experiments, + RecentRuns: recentRuns, + } +} + +// experimentTotalRuns returns the total number of runs recorded in the state. +// Prefers the runs array length when non-empty; falls back to summing all variant counts. +func experimentTotalRuns(state *ExperimentState) int { + if len(state.Runs) > 0 { + return len(state.Runs) + } + total := 0 + for _, variants := range state.Counts { + for _, c := range variants { + total += c + } + } + return total +} + +// experimentLastRun returns the date (YYYY-MM-DD) of the most recent run, or "" if unknown. +func experimentLastRun(state *ExperimentState) string { + if len(state.Runs) == 0 { + return "" + } + ts := state.Runs[len(state.Runs)-1].Timestamp + if len(ts) >= 10 { + return ts[:10] + } + return ts +} + +// extractExperimentName extracts the workflow ID from a branch ref. +// +// "origin/experiments/my-workflow" → "my-workflow" +// "experiments/my-workflow" → "my-workflow" +// "experiments/" → "" (bare prefix, rejected by callers) +func extractExperimentName(ref string) string { + ref = strings.TrimPrefix(ref, "origin/") + if !strings.HasPrefix(ref, experimentsBranchPrefix) { + return "" + } + // An empty result here (bare "experiments/" ref) is acceptable: callers + // guard against empty workflow IDs with `if workflowID == ""` checks. + return strings.TrimPrefix(ref, experimentsBranchPrefix) +}