feat: auto-resume on provider usage limit reset + /code-review command

[gr3enarr0w]

Summary

Closes #27
Closes #32

---

feat(workflow-manager): auto-resume after usage-limit pause (#27)

When a workflow hits a provider quota/usage limit, it's now checkpointed as paused (already in place via #26). This PR adds the automatic resume layer on top.

How it works

• ExecOptions.autoResume (default true) — opt out per-execution by passing autoResume: false
• When executeRun catches a PROVIDER_USAGE_LIMIT error, it calls scheduleAutoResume(runId, resetHint)
• parseResetHintMs() extracts hours/minutes from the verbatim resetHint string (e.g. "~3h", "45 min"), falls back to 60 minutes
• Exponential backoff: each re-pause doubles the delay; MAX_AUTO_RESUME_ATTEMPTS = 5 then gives up
• recoverStaleRuns() now re-arms timers on cold start for any persisted paused + pauseReason === "usage_limit" runs
• stop() and deleteRun() cancel pending timers so stopped runs don't ghost-resume
• Emits "autoResuming" event before each attempt for observability

---

feat(code-review): /code-review built-in command (#32)

New src/code-review.ts implementing generateCodeReviewWorkflow(), registered as /code-review.

Phases

Phase 1 — Find (7 parallel agents)

Agent	Tier	Focus
A — line-by-line scan	medium	Inverted conditions, off-by-one, nil deref, wrong variable, swallowed error
B — removed-behavior audit	medium	Every deleted line: invariant named, where re-established
C — cross-file tracer	medium	Changed functions → grep callers → call site breakage
D — reuse	small	New code duplicating existing helpers
E — simplification	small	Redundant state, copy-paste variation, dead code
F — efficiency	small	Redundant I/O, sequential work that could parallel
G — altitude	big	Fix at wrong abstraction level

Phase 2 — Verify — parallel verifier per candidate, drops REFUTED

Phase 3 — Report — big-tier synthesis, ranked A/B/C > D/E/F > G, capped at 10

Input modes

/code-review                  → git diff HEAD
/code-review HEAD~3..HEAD     → git range
/code-review src/foo.ts       → specific file
/code-review 42               → gh pr diff 42

[QuintinShaw]

Thanks — the auto-resume work is the valuable part here, but this PR is doing three separate things and I can't take it as one:

• agent-registry.ts / workflow.ts / agent-registry.test.ts are byte-identical to feat: wire agent frontmatter isolation to worktree execution #43 (same isolation wiring) — I'm landing feat: wire agent frontmatter isolation to worktree execution #43 for that.
• The /code-review command overlaps feat: add built-in /code-review workflow command #42; see my note there. We're consolidating on this generator, but it needs tests — it currently breaks commands.length === 4.
• Auto-resume (Auto-resume a workflow once a provider usage limit resets #27) is the only unique change, and it ships with no tests.

Could you rebase this down to just auto-resume + /code-review, drop the isolation files (they'll come from #43), and add coverage for parseResetHintMs, the backoff/cap in scheduleAutoResume, and the recoverStaleRuns re-arm? Happy to merge once it's scoped to its own change.

[QuintinShaw]

Thanks for the quick turnaround — the split is clean (no more #43 overlap, rebases onto main with zero conflicts) and the new tests are real. Two things before merge, both in the new code:

1. stop() can't cancel an auto-resume timer that recoverStaleRuns() arms on cold start. The if (!managed) return false guard returns before cancelAutoResume() runs, and recovered runs aren't in this.runs — so a run the user explicitly stops can still silently auto-resume and re-spend quota. deleteRun() already cancels unconditionally; stop() should mirror it (move the two cancel lines above the guard).

2. The /code-review diff-source runs through exec() (/bin/sh -c) with the arg interpolated raw in the .. and filename branches: git diff ${input} / git diff HEAD -- ${input}. That's a shell-injection footgun. worktree.ts already uses promisify(execFile) + array args — please follow that here.

A regression test asserting both stop() and deleteRun() call cancelAutoResume would pin #1 (that gap is exactly why it slipped through). Optional: clamp the backoff with Math.max(delay, 60_000) so a hint that parses to ~0 doesn't fire five instant retries.

[gr3enarr0w]

PR Reviewer Guide 🔍

(Review updated until commit 2347f1a)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 27 - Partially compliant Compliant requirements: ExecOptions.autoResume defaults to true with per-execution opt-out executeRun calls scheduleAutoResume on PROVIDER_USAGE_LIMIT parseResetHintMs extracts hours/minutes and falls back to 60 minutes Exponential backoff doubles the delay each re-pause MAX_AUTO_RESUME_ATTEMPTS = 5 recoverStaleRuns re-arms timers on cold start stop() and deleteRun() cancel pending timers "autoResuming" event emitted before each attempt Non-compliant requirements: The backoff attempt counter is held only in memory, so the cap resets on process restart and a run can exceed the intended maximum attempts across restarts Requires further human verification: None 32 - Partially compliant Compliant requirements: src/code-review.ts created with generateCodeReviewWorkflow() /code-review registered alongside existing commands Input modes for no args, git range, file path, and PR number are implemented Model tier routing matches the spec 7 parallel finder agents with structured candidate output Candidates are deduplicated before verify Ranking correctness (A/B/C) before cleanup (D/E/F) before altitude (G) with cap at 10 Synthesis produces JSON findings and a markdown report Non-compliant requirements: README.md documentation for /code-review is not updated Verify pass does not use the verify() stdlib with reviewers: 1, threshold: 0.5; it uses manual agent() calls Verifier agents are not given the relevant file(s) context beyond the diff Requires further human verification: Confirm whether the verify() stdlib exists in this codebase; if it does, the manual agent implementation should be replaced

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Non-persistent attempt cap autoResumeAttempts is stored only in a private Map, so the exponential-backoff attempt counter resets to 0 on process restart. A run that has already been auto-resumed several times and re-paused can receive another full set of attempts after a cold start, breaking the MAX_AUTO_RESUME_ATTEMPTS cap and causing repeated resume attempts against an exhausted budget. private autoResumeAttempts = new Map<string, number>();

[gr3enarr0w]

Persistent review updated to latest commit 2347f1a

[QuintinShaw]

This bundles two unrelated features with very different risk profiles, and they're blocking each other: /code-review is close to mergeable, while auto-resume needs another iteration. Please split it — /code-review can land quickly.

Auto-resume (#27), the things that need rework:

1. When the timer fires, resume() returning false (lease held, script missing) is silently swallowed: the attempt is consumed, autoResuming was already emitted, no timer is rescheduled — the run stays paused forever. Reschedule on failure or don't consume the attempt.
2. Cold-start re-arm computes the full delay from "now" instead of the remaining time from persisted updatedAt. Timers are unref()'d, so anyone who restarts pi more often than the hint interval never auto-resumes — Auto-resume a workflow once a provider usage limit resets #27 explicitly asked for a timer that fires when the reset window elapses.
3. autoResume: false isn't persisted: recoverStaleRuns() re-arms unconditionally and a re-pause after manual resume() gets the default true. Same for stop() — the persisted pauseReason survives, so the next restart re-arms the ghost timer.
4. Please add a floor on the delay (a hint parsing to ~0 can fire before persistRun writes the paused state — the callback then sees a non-paused run and drops tracking), and a fake-timer test that actually fires the callback end-to-end; the current backoff test patches setTimeout so the fire path is never executed.

/code-review (#32), minor: raise execFileAsync's maxBuffer (default 1 MB — gh pr diff on a big PR throws) and cap/truncate the diff fed into the 7 finder prompts; the issue's file list includes a README update; and either use the existing verify() stdlib for the verify phase per the issue spec or note why the hand-rolled verifier is preferable.

[QuintinShaw]

Hey @gr3enarr0w — no rush at all, just checking in since it's been a couple weeks. Are you planning to pick up the review notes here? The /code-review half is close to mergeable; the auto-resume half is where the rework is (the reschedule/persistence points above). If you're swamped, I'm happy to take it forward from here and keep you credited — just say which you'd prefer.

[QuintinShaw]

Heads up — I've split the /code-review half of this PR out and landed it separately in #62 (merged), crediting you as co-author. It's the same command with a few maintainer follow-ups on top: diff-capture maxBuffer raised to 64MB with a clear over-limit message, a 200k-char truncation guard, and README docs. Closed #32.

This PR stays open for the auto-resume half, which still needs the rework from my earlier review (the resume()→false swallow, cold-start re-arm using the full delay vs remaining-from-updatedAt, the autoResume:false+stop() ghost re-arm, a delay floor, and the untested timer-fire path). No rush — happy to take that forward too if you'd rather hand it off, otherwise it's all yours. Thanks again for both features.

[QuintinShaw]

Hi @gr3enarr0w — an update on this one. The /code-review half shipped separately a while back as #62. For the auto-resume half: since it had been open a few weeks and auto-resume-on-usage-limit is something I needed for my own long runs, I implemented it in #78 (now merged, closing #27).

I want to be straight that your PR directly informed that work — reviewing it is what surfaced the exact edge cases the rewrite had to get right (the swallowed resume()→false, cold-start using remaining vs full delay, the un-persisted opt-out, the missing delay floor, the untested timer path). So thank you — it wasn't wasted even though the final code took a different route (a standalone scheduler, to stay decoupled from manager internals).

I think this can be closed now that both halves are covered (#62 + #78), but I'll leave that to you in case you'd rather repurpose it. Really appreciate the contributions — hope to see more.