Uh oh!
There was an error while loading. Please reload this page.
Refactor experiments CLI into focused modules - #53706
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors the experiments CLI into focused modules while preserving existing behavior and API.
Changes:
- Retains command orchestration in
experiments_command.go. - Extracts fetching, state, safety, rendering, and JSON parsing.
- Keeps each module below 500 lines.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/experiments_command.go | Retains command wiring and orchestration. |
pkg/cli/experiments_fetch.go | Handles local and remote data loading. |
pkg/cli/experiments_state.go | Contains state models, parsing, and aggregation. |
pkg/cli/experiments_git_safety.go | Isolates git argument validation. |
pkg/cli/experiments_render.go | Handles human-readable output. |
pkg/cli/experiments_json_utils.go | Parses paginated JSON arrays. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Balanced
| "github.com/github/gh-aw/pkg/tty" | ||
| ) | ||
| func printExperimentDetails(d *ExperimentDetails) { |
| // 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 { |
| // buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts | ||
| // before joining them, preventing flag and path-traversal style injections. |
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #53706 refactors the experiments CLI into focused production modules but does not modify or add any test files (*_test.go). Existing test coverage remains unchanged.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
✅ Ponytail Reviewer completed successfully!
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. All gate actions completed via subagent: draft ADR committed to docs/adr/53706-split-experiments-cli-into-focused-modules.md on PR branch, and blocking ADR Required comment posted on PR #53706.
|
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.
|
There was a problem hiding this comment.
Request changes
This refactor mostly moves code around, but it also ships with a couple of regressions that make the split riskier than it looks.
Blocking themes
- A stale doc comment was moved onto the wrong function, so the new state loader now documents behavior it does not implement.
- The new fetch/state/render modules still have almost no behavioral coverage, which is a bad trade when the stated goal of the issue is maintainability through decomposition.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 33.3 AIC · ⌖ 6.95 AIC · ⊞ 7K
Comment /review to run again
| } | ||
| // buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts | ||
| // before joining them, preventing flag and path-traversal style injections. |
There was a problem hiding this comment.
This split leaves readRemoteExperimentState with the buildSafeGitShowObjectArg doc comment, which now documents the wrong behavior and will mislead the next person debugging remote state loading.
💡 Why this matters
The function below is a remote GitHub API reader, but the attached comment still claims it validates git show ref:path arguments. That is more than a cosmetic nit: exported behavior in this area is already subtle, and stale comments around safety-sensitive code send maintainers to the wrong place when they are tracing fallback/error handling.
A minimal fix is to move the safety-helper comment back onto buildSafeGitShowObjectArg in experiments_git_safety.go and replace this one with the old remote-loader contract, e.g.
// 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).funcreadRemoteExperimentState(repoOverride, branchNamestring) *ExperimentState {
...
}That keeps the new module split readable instead of making the codebase lie about where the safety boundary lives.
| // 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 { |
There was a problem hiding this comment.
This refactor extracted ~900 lines into new modules without adding meaningful behavioral coverage for the moved code, so it increases the chance of silent regressions the next time these paths change.
💡 Why this matters
The new files now own branch discovery, remote workflow lookup, state parsing, git ref safety checks, and human-readable rendering, but the existing review context only shows tests around small helpers like filename matching and result summarization. A decomposition PR is exactly where tests should lock behavior in before future edits scatter across files.
The risky gaps here are the successful and fallback paths, for example:
funcTestReadRemoteExperimentState_FallsBackFromJSONLToJSON(...)
funcTestLoadRemoteExperimentConfigs_PrefersResolvedFilename(...)
funcTestIsSafeExperimentStateRef_RejectsInvalidRefShapes(...)
funcTestPrintExperimentDetails_RecentRunsAndPercentages(...)Without that coverage, the split buys smaller files at the cost of making regressions harder to notice.
There was a problem hiding this comment.
Ponytail review (over-engineering only). This is a mechanical file split with behavior preserved; found a few small shrink/yagni spots:
- Two duplicated 6-line manual string comparators for
slices.SortFuncthat could each bestrings.Compareone-liners. workflowFileCandidatesis a one-caller wrapper around a one-element slice literal.
net: -12 lines possible.
Generated by ✂️ Ponytail Reviewer for #53706 · auto · 33.9 AIC · ⌖ 6.48 AIC · ⊞ 7.3K
Comment /ponytail to run again
| for k, v := range exp.Variants { | ||
| pairs = append(pairs, kv{k, v}) | ||
| } | ||
| slices.SortFunc(pairs, func(a, b kv) int { |
There was a problem hiding this comment.
L31-40: shrink: manual 3-branch cmp for string sort. slices.SortFunc(pairs, func(a, b kv) int { return strings.Compare(a.k, b.k) }), 1 line.
| Total: total, | ||
| }) | ||
| } | ||
| slices.SortFunc(experiments, func(a, b ExperimentVariantStats) int { |
There was a problem hiding this comment.
L175-184: shrink: same 3-branch string comparator repeated from experiments_render.go. slices.SortFunc(experiments, func(a, b ExperimentVariantStats) int { return strings.Compare(a.Name, b.Name) }), 1 line.
| // (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 { |
There was a problem hiding this comment.
L300-307: yagni: workflowFileCandidates wraps a single value in a slice for one caller. Inline candidates := []string{experimentName} at the call site; drop the function.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on three correctness/safety issues and one testability gap.
📋 Key Themes & Highlights
Key Themes
- Misplaced doc comment (
experiments_state.goline 78–80): thebuildSafeGitShowObjectArgdoc comment is now attached toreadRemoteExperimentStateafter the split — a copy-paste artefact that will mislead readers. - Safety boundary fragmentation (
gitRefExistsin state file): the one function that gates rawgit rev-parsecalls lives inexperiments_state.gorather thanexperiments_git_safety.go, splitting the security perimeter across files. - Potential double-count in
appendExperimentRun: if a run record'sBaselineCountsandAssignmentsshare the same experiment key, the variant is incremented twice — worth documenting the exclusivity contract or adding a guard. - Undocumented snapshot-override semantics in
parseExperimentStateJSONL: run records before a snapshot line are silently discarded; this should be explicitly documented. printExperimentDetailsuntestable as-is: it writes directly toos.Stderrwith no writer injection, making it impossible to unit test.isSafeGitTreePathmissing control-character filter: only\x00is blocked; bytes0x01–0x1fand0x7fpass through.
Positive Highlights
- ✅ Excellent separation of concerns — command wiring, data loading, state parsing, rendering, and git safety are now clearly demarcated.
- ✅
experiments_git_safety.gois a strong, well-structured security module with thorough ref validation rules. - ✅
parsePagedJSONArrayis a clean generic utility, easy to test and reuse. - ✅ Existing test coverage for
TestBuildSafeGitShowObjectArg,TestIsSafeExperimentStateRef,TestParseExperimentState, andTestParsePagedJSONArrayall carry over correctly.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 71.9 AIC · ⌖ 10.5 AIC · ⊞ 7.8K
Comment /matt to run again
| } | ||
| var snapshot ExperimentState | ||
| if err := json.Unmarshal([]byte(line), &snapshot); err == nil && snapshot.Counts != nil { |
There was a problem hiding this comment.
[/codebase-design] Silent snapshot-override in parseExperimentStateJSONL — when state = &snapshot replaces the accumulated pointer, any run records processed before that snapshot line are silently discarded. This "last snapshot wins" behaviour is undocumented and a latent correctness trap.
💡 Suggestion
Add a doc comment that explicitly declares the discard semantics:
// A snapshot line resets the base state; run records accumulated before it// are intentionally discarded because the snapshot is a cumulative checkpoint.// Run records that appear after the snapshot are applied on top of it.state=&snapshotIf the intent is ever to accumulate across snapshots, the code is wrong — the comment will make the bug visible immediately.
@copilot please address this.
| if state.Counts[name] == nil { | ||
| state.Counts[name] = map[string]int{} | ||
| } | ||
| state.Counts[name][variant]++ |
There was a problem hiding this comment.
[/codebase-design]appendExperimentRun may double-count when the same experiment name appears in both BaselineCounts and Assignments. The BaselineCounts loop adds cumulative counts, then the Assignments loop increments state.Counts[name][variant]++ for the same run — if these keys overlap, the variant is counted twice.
💡 What to verify / fix
Document the exclusivity contract — either add a guard:
forname, variant:=rangerun.Assignments {
if_, coveredByBaseline:=run.BaselineCounts[name]; coveredByBaseline {
continue// already counted via BaselineCounts
}
...
}Or add a test with a run record that has the same key in both maps and assert the expected final count to confirm current behaviour is intentional.
@copilot please address this.
| } | ||
| // gitRefExists reports whether an experiments/evals state ref exists locally. | ||
| func gitRefExists(ref string) bool { |
There was a problem hiding this comment.
[/codebase-design]gitRefExists belongs in experiments_git_safety.go. It calls isSafeExperimentStateRef and executes git rev-parse --verify — it is a safety/git primitive, not a state helper. Keeping it in experiments_state.go splits the security boundary across two files and makes it easy to add a new ref-reading call without seeing the safety gate nearby.
💡 Suggestion
Move gitRefExists to experiments_git_safety.go alongside buildSafeGitShowObjectArg and isSafeExperimentStateRef. The state file simply calls it; it no longer has to own the safety logic.
@copilot please address this.
| // buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts | ||
| // before joining them, preventing flag and path-traversal style injections. | ||
| func readRemoteExperimentState(repoOverride, branchName string) *ExperimentState { |
There was a problem hiding this comment.
[/codebase-design] The doc comment on line 78 (// buildSafeGitShowObjectArg validates git show's "ref:path" object argument parts...) describes buildSafeGitShowObjectArg, which has moved to experiments_git_safety.go. It is now the doc comment for readRemoteExperimentState, misleading every reader of this function.
💡 Fix
Replace the misplaced comment with one that describes readRemoteExperimentState:
// readRemoteExperimentState fetches experiment state from the remote repository.// Tries state.jsonl first, then falls back to state.json.// Returns an empty state when neither file can be read or parsed.funcreadRemoteExperimentState(repoOverride, branchNamestring) *ExperimentState {@copilot please address this.
| if fileName == "" || strings.HasPrefix(fileName, "-") { | ||
| return false | ||
| } | ||
| if path.IsAbs(fileName) || strings.Contains(fileName, "\\") || strings.Contains(fileName, ":") || strings.ContainsRune(fileName, '\x00') { |
There was a problem hiding this comment.
[/tdd]isSafeGitTreePath accepts "state.jsonl" but would also silently accept "state.jsonl\x7f" or any filename containing ASCII control characters (bytes 0x01–0x1f except \x00). The null-byte guard (ContainsRune('\x00')) is present but other control characters are not filtered. Git itself rejects paths containing control characters on some platforms.
💡 Suggestion
Add a control-character check alongside the null-byte check:
for_, r:=rangefileName {
ifr<0x20||r==0x7f {
returnfalse
}
}And add a test case:
{name: "control char", input: "state\x01.jsonl", want: false},@copilot please address this.
| "github.com/github/gh-aw/pkg/tty" | ||
| ) | ||
| func printExperimentDetails(d *ExperimentDetails) { |
There was a problem hiding this comment.
[/tdd]printExperimentDetails outputs directly to os.Stderr with no test coverage in the new module. The existing tests only exercise formatAssignments from this file. A regression in the rendering logic (wrong column headers, broken table config, nil-pointer on empty Analyses) will only surface at runtime.
💡 Suggestion
Capture stderr via a bytes.Buffer writer and inject it as a parameter:
funcprintExperimentDetails(w io.Writer, d*ExperimentDetails) { ... }Then write a table-output test:
varbuf bytes.BufferprintExperimentDetails(&buf, &ExperimentDetails{WorkflowID: "test", TotalRuns: 3, ...})
assert.Contains(t, buf.String(), "test")
assert.Contains(t, buf.String(), "3")@copilot please address this.
There was a problem hiding this comment.
Review: Refactor experiments CLI into focused modules
The structural split is clean and well-motivated. Security-sensitive paths (isSafeGitTreePath, isSafeExperimentStateRef, buildSafeGitShowObjectArg) look correct: path-traversal is guarded by filepath.Abs + prefix check, and ref-safety functions apply defence-in-depth. No regressions introduced by the mechanical move.
Existing comments to address (already raised by prior reviewer):
- Misplaced
buildSafeGitShowObjectArgdoc comment attached toreadRemoteExperimentState— will mislead readers about the function's contract. - Test coverage gaps on
printExperimentDetailsandexperiments_fetch.godata-loading paths.
One additional observation (non-blocking): In loadRemoteExperimentConfigs, the candidate loop returns only when len(cfg.ExperimentConfigs) > 0. A file that parses successfully but contains zero experiment configs is silently skipped, which also drops a valid evals-only frontmatter. This is likely intentional but deserves an inline comment explaining that evals-only configs are not returned here.
Overall LGTM structurally — please address the doc comment and test coverage concerns before merging.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 58.2 AIC · ⌖ 8.83 AIC · ⊞ 5.7K
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (895 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
|
pelikhan
commented
Aug 18, 2026
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR finisher completed. Addressed actionable findings and pushed |
🎉 This pull request is included in a new release. Release: |
pkg/cli/experiments_command.gocombined command wiring, data retrieval, state parsing, git argument validation, and rendering in a 1,120-line file. This refactor separates those concerns while retaining the existing CLI API and behavior.Command orchestration
experiments_command.go.Data loading
experiments_fetch.gofor local/remote branch discovery, workflow frontmatter loading, and metric-evaluation retrieval.State and safety
experiments_state.gofor experiment state models, JSON/JSONL parsing, aggregation, and ref lookup.experiments_git_safety.gofor validation ofgit show ref:patharguments.Presentation and shared parsing
experiments_render.gofor human-readable detail output.experiments_json_utils.gofor paginated JSON-array decoding.