feat: implement iterate run + iterate watch with file-based persistence#462
feat: implement iterate run + iterate watch with file-based persistence#462emil07770 wants to merge 1 commit into
Conversation
Greptile SummaryThis PR replaces stub implementations in
Confidence Score: 3/5Three correctness bugs in the core run/watch paths make this risky to ship as-is without fixes. The interactive run path always stores an empty diff because stdio inherit sends the agent output to the terminal rather than capturing it in result.stdout. The --dry-run --json combination silently skips writing to the run history. And the --scope option is never stored in WatchConfig, so every generated crontab fires against the default all scope regardless of what the user configured. packages/cli/src/commands/iterate.ts — all three bugs are in this single file, concentrated in the iterate run action handler and the WatchConfig interface. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[sh1pt iterate run] --> B{dry-run?}
B -- yes and json --> C[print record to stdout not persisted]
B -- yes no json --> D[appendRun then print dry-run notice]
B -- no --> E{auto-apply or json?}
E -- no --> F[readline prompt y/N]
F -- N --> G[appendRun skipped]
F -- y --> H[spawnSync agentBin --print prompt]
E -- yes --> H
H --> I{error or exit nonzero?}
I -- yes --> J[appendRun status=error]
I -- no --> K[record.diff = result.stdout]
K --> L[appendRun status=applied]
M[sh1pt iterate watch] --> N{stop?}
N -- yes --> O[clearWatchConfig]
N -- no --> P{status?}
P -- yes --> Q[print cfg and quiet hours]
P -- no --> R[saveWatchConfig]
R --> S{cloud?}
S -- yes --> T[print cloud deploy instructions]
S -- no --> U[print crontab line]
Reviews (1): Last reviewed commit: "feat: implement iterate run + iterate wa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const result = spawnSync(agentBin, ['--print', prompt], { | ||
| encoding: 'utf8', | ||
| stdio: opts.json ? ['ignore', 'pipe', 'pipe'] : 'inherit', | ||
| }); |
There was a problem hiding this comment.
result.stdout is always null when stdio is 'inherit', so record.diff will always be stored as '' in the normal interactive flow. The agent's output goes to the terminal but is never captured. Only --json mode (which uses 'pipe') would actually populate the diff. To capture output while still streaming to the terminal, use ['inherit', 'pipe', 'inherit'] so stdout is piped while stderr still streams.
| const result = spawnSync(agentBin, ['--print', prompt], { | |
| encoding: 'utf8', | |
| stdio: opts.json ? ['ignore', 'pipe', 'pipe'] : 'inherit', | |
| }); | |
| const result = spawnSync(agentBin, ['--print', prompt], { | |
| encoding: 'utf8', | |
| stdio: opts.json ? ['ignore', 'pipe', 'pipe'] : ['inherit', 'pipe', 'inherit'], | |
| }); |
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!
| if (opts.dryRun) { | ||
| record.status = 'skipped'; | ||
| record.finishedAt = new Date().toISOString(); | ||
| if (opts.json) { | ||
| console.log(JSON.stringify({ run: record, dryRun: true }, null, 2)); | ||
| return; | ||
| } | ||
| console.log(kleur.yellow('\ndry-run — no agent invoked, no changes applied')); | ||
| console.log(kleur.dim(`run id: ${record.id}`)); | ||
| await appendRun(record); | ||
| return; | ||
| } |
There was a problem hiding this comment.
--dry-run and --json are combined, the function returns before appendRun is called, so the run is never written to iterate-runs.json. Every other code path (non-JSON dry-run, user-skipped, agent error, agent success) persists the record. This inconsistency means --dry-run --json runs are invisible in run history.
| if (opts.dryRun) { | |
| record.status = 'skipped'; | |
| record.finishedAt = new Date().toISOString(); | |
| if (opts.json) { | |
| console.log(JSON.stringify({ run: record, dryRun: true }, null, 2)); | |
| return; | |
| } | |
| console.log(kleur.yellow('\ndry-run — no agent invoked, no changes applied')); | |
| console.log(kleur.dim(`run id: ${record.id}`)); | |
| await appendRun(record); | |
| return; | |
| } | |
| if (opts.dryRun) { | |
| record.status = 'skipped'; | |
| record.finishedAt = new Date().toISOString(); | |
| await appendRun(record); | |
| if (opts.json) { | |
| console.log(JSON.stringify({ run: record, dryRun: true }, null, 2)); | |
| return; | |
| } | |
| console.log(kleur.yellow('\ndry-run — no agent invoked, no changes applied')); | |
| console.log(kleur.dim(`run id: ${record.id}`)); | |
| return; | |
| } |
| interface WatchConfig { | ||
| agent: string; | ||
| interval: number; | ||
| quietHours?: string; | ||
| cloud: boolean; | ||
| enabledAt: string; | ||
| lastRunAt?: string; | ||
| } |
There was a problem hiding this comment.
WatchConfig does not include a scope field, so the generated crontab line hardcodes no scope (defaulting to 'all'). A user who runs sh1pt iterate watch --scope perf ... will get a cron entry that silently runs against the all scope instead of perf. The configured scope needs to be stored in WatchConfig and emitted in the crontab line.
| interface WatchConfig { | |
| agent: string; | |
| interval: number; | |
| quietHours?: string; | |
| cloud: boolean; | |
| enabledAt: string; | |
| lastRunAt?: string; | |
| } | |
| interface WatchConfig { | |
| agent: string; | |
| interval: number; | |
| scope: string; | |
| quietHours?: string; | |
| cloud: boolean; | |
| enabledAt: string; | |
| lastRunAt?: string; | |
| } |
| function parseQuietHours(spec: string): { start: number; end: number } | null { | ||
| const m = /^(\d{1,2})-(\d{1,2})$/.exec(spec); | ||
| if (!m) return null; | ||
| return { start: Number(m[1]), end: Number(m[2]) }; | ||
| } |
There was a problem hiding this comment.
parseQuietHours accepts any integer pair without range validation, so values like 25-30 are silently stored and then always evaluate to false in inQuietHours, effectively disabling quiet hours without any warning. A 0–23 bounds check would catch this at configuration time.
| function parseQuietHours(spec: string): { start: number; end: number } | null { | |
| const m = /^(\d{1,2})-(\d{1,2})$/.exec(spec); | |
| if (!m) return null; | |
| return { start: Number(m[1]), end: Number(m[2]) }; | |
| } | |
| function parseQuietHours(spec: string): { start: number; end: number } | null { | |
| const m = /^(\d{1,2})-(\d{1,2})$/.exec(spec); | |
| if (!m) return null; | |
| const start = Number(m[1]); | |
| const end = Number(m[2]); | |
| if (start > 23 || end > 23) return null; | |
| return { start, end }; | |
| } |
| async function readJson<T>(file: string, fallback: T): Promise<T> { | ||
| try { | ||
| const raw = await fs.readFile(GOALS_FILE(), 'utf8'); | ||
| const raw = await fs.readFile(file, 'utf8'); | ||
| const parsed = JSON.parse(raw); | ||
| return parsed && typeof parsed === 'object' ? (parsed as Record<string, string>) : {}; | ||
| return parsed && typeof parsed === 'object' ? (parsed as T) : fallback; | ||
| } catch (err) { | ||
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return {}; | ||
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return fallback; | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
readJson returns the fallback only when the parsed value is falsy or not an object. For loadRuns() the fallback is [], but if the file contains {} the condition typeof parsed === 'object' passes and the plain object is cast to RunRecord[]. The subsequent runs.push(run) call would throw TypeError: runs.push is not a function at runtime. An Array.isArray guard on the fallback type catches this.
| async function readJson<T>(file: string, fallback: T): Promise<T> { | |
| try { | |
| const raw = await fs.readFile(GOALS_FILE(), 'utf8'); | |
| const raw = await fs.readFile(file, 'utf8'); | |
| const parsed = JSON.parse(raw); | |
| return parsed && typeof parsed === 'object' ? (parsed as Record<string, string>) : {}; | |
| return parsed && typeof parsed === 'object' ? (parsed as T) : fallback; | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return {}; | |
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return fallback; | |
| throw err; | |
| } | |
| } | |
| async function readJson<T>(file: string, fallback: T): Promise<T> { | |
| try { | |
| const raw = await fs.readFile(file, 'utf8'); | |
| const parsed = JSON.parse(raw); | |
| if (parsed === null || typeof parsed !== 'object') return fallback; | |
| if (Array.isArray(fallback) && !Array.isArray(parsed)) return fallback; | |
| return parsed as T; | |
| } catch (err) { | |
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') return fallback; | |
| throw err; | |
| } | |
| } |
1 participant
Closes #461
What this PR implements
Full implementation of
sh1pt iterate runandsh1pt iterate watch— replaces the[stub]placeholders with working logic.iterate runiterate goals) and metric signals for the selected scopespawnSync(agentBin, ['--print', prompt])(supportsclaude,codex,qwen)RunRecord(id, startedAt, finishedAt, agent, scope, goals, status, diff) toiterate-runs.json(capped at 100 entries)--dry-run(skip agent, record skipped),--auto-apply(skip y/N prompt),--json(machine-readable output)iterate watchWatchConfigtoiterate-watch.jsonwith agent, interval, quietHours, cloud flag--stop: clears the watch config--status: shows current config and whether quiet hours are active*/N * * * * sh1pt iterate run --agent X --auto-apply)sh1pt scale deploy --cloudinstructionsinQuietHours(spec)helper handles overnight spans (e.g.22-08wraps midnight)Shared infrastructure added
atomicWrite(file, data)—writeFile(.tmp) → rename, mode 0o600readJson<T>(file, fallback)— generic JSON loader with ENOENT handlingSCOPE_SIGNALS— maps scope names to metric signal arraysMetricSnapshot,RunRecord,WatchConfiginterfacesiterate-metrics.jsonTesting