Skip to content

feat(cli): implement iterate test + experiments with local persistence#479

Open
forgou37 wants to merge 1 commit into
profullstack:masterfrom
forgou37:feat/iterate-test-experiments-persistence
Open

feat(cli): implement iterate test + experiments with local persistence#479
forgou37 wants to merge 1 commit into
profullstack:masterfrom
forgou37:feat/iterate-test-experiments-persistence

Conversation

@forgou37
Copy link
Copy Markdown
Contributor

Closes #442

Summary

Replaces the stubs in sh1pt iterate test and sh1pt iterate experiments with real file-based persistence, following the same atomic-write pattern used by iterate goals.

Before

$ sh1pt iterate test "show hero CTA as button vs link"
[stub] iterate test "show hero CTA as button vs link" {}

$ sh1pt iterate experiments
[stub] iterate experiments — table of active / concluded tests

After

$ sh1pt iterate test "show hero CTA as button vs link" --variant "use <button> element" --traffic 30
experiment created: a3f9c012
  hypothesis : show hero CTA as button vs link
  variant(s) : use <button> element
  traffic    : 30% to B
  min-sample : 1000
  run `sh1pt iterate experiments` to track progress

$ sh1pt iterate experiments
a3f9c012  active
  show hero CTA as button vs link
  variants: use <button> element

1 active / 1 total

$ sh1pt iterate experiments --end a3f9c012 --winner A --note "no sig. difference after 2k events"
ended: a3f9c012 · winner=A

$ sh1pt iterate experiments --json
{
  "active": [],
  "paused": [],
  "ended": [{ "id": "a3f9c012", "hypothesis": "...", "winner": "A", ... }]
}

Changes

  • Add Experiment interface with id, hypothesis, variants, traffic, minSample, status, winner, note fields
  • EXPERIMENTS_FILE at ~/.config/sh1pt/iterate-experiments.json
  • loadExperiments() / saveExperiments() with same atomic tmp-rename pattern as goals
  • iterate test <hypothesis> creates an entry with a random 8-char hex id
  • iterate experiments lists all experiments grouped by status
  • --end, --pause, --resume flags mutate status and persist
  • --winner A|B|inconclusive and --note recorded on end
  • --json outputs { active, paused, ended } arrays

Replaces stubs with real file-based persistence.
- iterate test <hypothesis> creates Experiment in ~/.config/sh1pt/iterate-experiments.json
- iterate experiments lists active/paused/ended with formatted output or --json
- --end/--pause/--resume mutate status, --winner records outcome

Closes profullstack#442
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 30, 2026

Greptile Summary

This PR replaces CLI stubs for sh1pt iterate test, sh1pt iterate experiments, and sh1pt iterate goals with real JSON file persistence under ~/.config/sh1pt/, using an atomic tmp-then-rename write pattern. The experiments flow supports --end, --pause, --resume, --winner, --note, and --json flags for full lifecycle management.

  • loadExperiments/saveExperiments and loadGoals/saveGoals are added with safe atomic writes and ENOENT-tolerant reads.
  • iterate test <hypothesis> creates a new experiment entry with a random 8-char hex ID; iterate experiments lists and mutates entries by status.
  • Several option values (--winner, --traffic) lack runtime validation, allowing invalid data to be persisted; --winner/--note are silently discarded when passed without --end.

Confidence Score: 3/5

Safe to merge as a stub-replacement, but three new command paths can silently persist invalid or incomplete data to the user's config file.

The --winner field accepts any string and writes it directly to persisted JSON — passing --winner foo corrupts the stored experiment record. The --traffic option accepts out-of-range values like -10 or 200 with no rejection. Passing --winner or --note without --end silently discards the user's intent with no feedback.

packages/cli/src/commands/iterate.ts — specifically the --winner validation path and --traffic bounds check in the experiments action handler.

Important Files Changed

Filename Overview
packages/cli/src/commands/iterate.ts Replaces stubs for iterate test, iterate experiments, and iterate goals with file-based persistence; missing runtime validation for --winner values and --traffic range, and --winner/--note silently dropped when used without --end.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[sh1pt iterate experiments] --> B{mutation flag?}
    B -->|--end id| C[find experiment by id]
    B -->|--pause id| D[find experiment by id]
    B -->|--resume id| E[find experiment by id]
    B -->|none| F{--json?}
    C --> G{found?}
    G -->|no| H[error + exit 1]
    G -->|yes| I[set status=ended / winner / note]
    I --> J[saveExperiments]
    D --> L{found?}
    L -->|no| H
    L -->|yes| M[set status=paused]
    M --> J
    E --> N{found?}
    N -->|no| H
    N -->|yes| O[set status=active]
    O --> J
    F -->|yes| P[print JSON grouped by status]
    F -->|no| Q{any experiments?}
    Q -->|no| R[print empty hint]
    Q -->|yes| S[print table by status]
    T[sh1pt iterate test hypothesis] --> U[loadExperiments]
    U --> V[create Experiment / random hex id]
    V --> W[saveExperiments]
    W --> X[print created summary]
    style H fill:#f66,color:#fff
Loading

Reviews (1): Last reviewed commit: "feat(cli): implement iterate test + expe..." | Re-trigger Greptile

Comment on lines +216 to +237
.action(async (opts: {
json?: boolean;
end?: string;
pause?: string;
resume?: string;
winner?: 'A' | 'B' | 'inconclusive';
note?: string;
}) => {
const experiments = await loadExperiments();
const now = new Date().toISOString();

// Mutating actions
if (opts.end) {
const exp = experiments.find(e => e.id === opts.end);
if (!exp) { console.error(kleur.red(`experiment "${opts.end}" not found`)); process.exit(1); }
exp.status = 'ended';
exp.updatedAt = now;
if (opts.winner) exp.winner = opts.winner;
if (opts.note) exp.note = opts.note;
await saveExperiments(experiments);
console.log(kleur.yellow(`ended: ${opts.end}`) + (opts.winner ? kleur.dim(` · winner=${opts.winner}`) : ''));
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 --winner and --note silently discarded without --end

--winner and --note are only consumed inside the if (opts.end) branch. If a user runs sh1pt iterate experiments --winner A --note "done" (forgetting to supply --end <id>), the handler falls through to the listing path — the options are silently ignored and no winner is recorded. The user gets a normal experiments list with no indication their flags were dropped, leading them to believe the winner was persisted when it was not.

if (!exp) { console.error(kleur.red(`experiment "${opts.end}" not found`)); process.exit(1); }
exp.status = 'ended';
exp.updatedAt = now;
if (opts.winner) exp.winner = opts.winner;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 --winner value not validated at runtime

Commander passes option values as plain strings; the TypeScript union type 'A' | 'B' | 'inconclusive' is compile-time only and offers no protection at runtime. A user who passes --winner foo will have the string "foo" written directly to the persisted JSON file, silently corrupting the winner field. A runtime guard is needed before the value is stored.

Suggested change
if (opts.winner) exp.winner = opts.winner;
const validWinners = ['A', 'B', 'inconclusive'] as const;
if (opts.winner) {
if (!(validWinners as readonly string[]).includes(opts.winner)) {
console.error(kleur.red(`invalid --winner "${opts.winner}" — must be A, B, or inconclusive`));
process.exit(1);
}
exp.winner = opts.winner as 'A' | 'B' | 'inconclusive';
}

Comment on lines +182 to +184
const experiments = await loadExperiments();
const now = new Date().toISOString();
const id = randomBytes(4).toString('hex');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 --traffic value not validated for valid range

Commander's Number coercion accepts any numeric string, including negative values and values above 100. Storing traffic: -10 or traffic: 200 is nonsensical for a traffic-split percentage and will display misleadingly (e.g., "-10% to B"). A simple range guard prevents invalid values from being persisted.

Suggested change
const experiments = await loadExperiments();
const now = new Date().toISOString();
const id = randomBytes(4).toString('hex');
const experiments = await loadExperiments();
const traffic = opts.traffic ?? 50;
if (traffic <= 0 || traffic >= 100) {
console.error(kleur.red(`--traffic must be between 1 and 99 (got ${traffic})`));
process.exit(1);
}
const now = new Date().toISOString();
const id = randomBytes(4).toString('hex');

Comment on lines +207 to 208
const experimentsCmd = iterateCmd
.command('experiments')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 experimentsCmd variable assigned but never used

The return value of .command('experiments') is stored in experimentsCmd, but the variable is never referenced again — the subcommand is already wired into iterateCmd as a side effect of the .command() call itself. The const binding is dead code.

Suggested change
const experimentsCmd = iterateCmd
.command('experiments')
iterateCmd
.command('experiments')

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Comment on lines +228 to +258
if (opts.end) {
const exp = experiments.find(e => e.id === opts.end);
if (!exp) { console.error(kleur.red(`experiment "${opts.end}" not found`)); process.exit(1); }
exp.status = 'ended';
exp.updatedAt = now;
if (opts.winner) exp.winner = opts.winner;
if (opts.note) exp.note = opts.note;
await saveExperiments(experiments);
console.log(kleur.yellow(`ended: ${opts.end}`) + (opts.winner ? kleur.dim(` · winner=${opts.winner}`) : ''));
return;
}

if (opts.pause) {
const exp = experiments.find(e => e.id === opts.pause);
if (!exp) { console.error(kleur.red(`experiment "${opts.pause}" not found`)); process.exit(1); }
exp.status = 'paused';
exp.updatedAt = now;
await saveExperiments(experiments);
console.log(kleur.yellow(`paused: ${opts.pause}`));
return;
}

if (opts.resume) {
const exp = experiments.find(e => e.id === opts.resume);
if (!exp) { console.error(kleur.red(`experiment "${opts.resume}" not found`)); process.exit(1); }
exp.status = 'active';
exp.updatedAt = now;
await saveExperiments(experiments);
console.log(kleur.green(`resumed: ${opts.resume}`));
return;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Conflicting mutation flags silently discard one operation

When a user passes two mutation flags at once — e.g., --end id1 --pause id2 — only the first branch (--end) executes and returns early. The --pause operation is silently dropped. Since these flags target different experiment IDs, the user might reasonably expect both to take effect. At minimum, an error should be emitted when multiple mutation flags are provided simultaneously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cli): implement iterate test + experiments with local persistence

1 participant