diff --git a/ai/prompts/comments.md.tmpl b/ai/prompts/comments.md.tmpl index b2d0059..ec98eb1 100644 --- a/ai/prompts/comments.md.tmpl +++ b/ai/prompts/comments.md.tmpl @@ -12,7 +12,7 @@ {{define "park"}}🤖 Parked as `{{.Label}}` — this issue will not be retried automatically. -Remove the `{{.Label}}` label to queue a fresh attempt — any worktree, branch and logs this run produced are preserved and reused, so no work is lost.{{if .Guidance}} +Remove the `{{.Label}}` label to resume this run in the same Claude session — the worktree, branch, logs and session are preserved, so nothing is redone from zero.{{if .Guidance}} {{.Guidance}}{{end}}{{if .Error}} diff --git a/ai/prompts/resume.md.tmpl b/ai/prompts/resume.md.tmpl new file mode 100644 index 0000000..a1076b3 --- /dev/null +++ b/ai/prompts/resume.md.tmpl @@ -0,0 +1,11 @@ +/superpowers:executing-plans — continue + +This is a rework pickup: the same worktree from a previous, interrupted run +(usage limit, crash, or similar) that landed some partial progress. You are +resuming that same session, not starting over. Do NOT conclude the issue is +already implemented just because some code already exists in the worktree — +inspect what is actually there, keep going, and finish: complete the +remaining implementation, make the full test suite pass, and commit. + +HEADLESS: do not ask questions; make reasonable calls and note them in commit +messages. diff --git a/loop.go b/loop.go index 8bd4dfd..83bb74c 100644 --- a/loop.go +++ b/loop.go @@ -222,32 +222,58 @@ func (o *Orchestrator) selectIssues(ctx context.Context, issues []Issue, limit i func (o *Orchestrator) handleIssue(ctx context.Context, issue Issue, kind, base string) error { n := issue.Number branch := branchName(n) + logDir := o.issueLogDir(n) + // A park cause still on disk means this issue was parked as ai-rework and a + // human just removed the label — the ONLY way it re-enters ListEligibleIssues + // from that state. Nothing scans ai-rework or acts on it automatically; this + // only fires on the pickup that follows a human's own action. Resolve the + // resumable session (and its pipeline kind) before ai-wip overwrites nothing + // on disk that readSession needs. + resumeSession, resumeKind := "", "" + if hasParkCause(logDir) { + if si, err := readSession(logDir); err == nil { + resumeSession, resumeKind = si.SessionID, si.Kind + } + } + if resumeSession != "" && resumeKind != "" { + kind = resumeKind + } + if err := o.gh.AddLabel(ctx, n, o.cfg.StateLabels.WIP); err != nil { return err } - recordState(o.issueLogDir(n), o.cfg.StateLabels.WIP) + recordState(logDir, o.cfg.StateLabels.WIP) // Mirror the title next to the state marker: the dashboard otherwise knows // it only for as long as the issue keeps matching its label-scoped query. - recordTitle(o.issueLogDir(n), issue.Title) + recordTitle(logDir, issue.Title) _ = o.gh.Comment(ctx, n, pickupComment(kind, branch)) wtPath, err := o.wt.Create(ctx, o.cfg.WorkDir, n, base) if err != nil { return o.abort(ctx, n, err) } - content, err := o.gh.FetchIssueContent(ctx, n) - if err != nil { - return o.abort(ctx, n, err) - } - content = DownloadIssueImages(ctx, o.runner, content, o.issueLogDir(n)) - c := &Claude{runner: o.runner, logDir: o.issueLogDir(n), configDir: o.cfg.ClaudeConfigDir} + c := &Claude{runner: o.runner, logDir: logDir, configDir: o.cfg.ClaudeConfigDir} uat := &UAT{Target: o.gh, Num: n} var perr error - if kind == "bug" { - perr = RunBugPipeline(ctx, c, o.cfg, wtPath, content, base, uat) + if resumeSession != "" { + // A rework pickup resumes the exact prior session in its preserved + // worktree instead of fetching the issue content and running a fresh + // session — a fresh session only sees the worktree's current contents + // and can mistake the partial progress already there for a finished + // feature, closing the issue with no PR (the bug this branch fixes). + perr = RunResumePipeline(ctx, c, o.cfg, wtPath, kind, resumeSession) } else { - perr = RunFeaturePipeline(ctx, c, o.cfg, wtPath, content, readPersona(o.cfg.PersonaPath), uat) + content, err := o.gh.FetchIssueContent(ctx, n) + if err != nil { + return o.abort(ctx, n, err) + } + content = DownloadIssueImages(ctx, o.runner, content, logDir) + if kind == "bug" { + perr = RunBugPipeline(ctx, c, o.cfg, wtPath, content, base, uat) + } else { + perr = RunFeaturePipeline(ctx, c, o.cfg, wtPath, content, readPersona(o.cfg.PersonaPath), uat) + } } // A Stop landed during the pipeline: skip the normal park/ship/finish outcome // and leave the ticket ai-wip. The launching goroutine's consumeStopping+pause diff --git a/no_auto_retry_test.go b/no_auto_retry_test.go index 30fcc22..0f071e6 100644 --- a/no_auto_retry_test.go +++ b/no_auto_retry_test.go @@ -9,12 +9,14 @@ import ( "time" ) -// ai-rework is a TERMINAL state: the daemon must not even look at the label. A -// parked issue moves only when a human removes it, which puts it back in the -// eligible queue the normal cycle already reads. Retrying failures automatically -// is what re-ran the whole pipeline on one broken issue every cycle, burning -// tokens on work nobody was watching. -func TestDaemonNeverScansOrResumesParkedIssues(t *testing.T) { +// ai-rework is a TERMINAL state: the daemon must not scan the label or act on +// a parked issue on its own. It moves only when a human removes it, which puts +// it back in the eligible queue the normal cycle already reads — that pickup +// resumes the preserved session instead of scanning/polling ai-rework itself, +// so this stays a one-shot reaction to the human's own edit, not the +// every-cycle automatic retry that used to burn tokens on work nobody was +// watching. +func TestDaemonNeverScansParkedIssuesButResumesOnceRequeued(t *testing.T) { env := newFakeEnv(t) prepParked(t, env, "claude debug: terminated: api_error; api status 429; You've hit your usage limit") shipped := make(chan struct{}, 1) @@ -37,7 +39,13 @@ func TestDaemonNeverScansOrResumesParkedIssues(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) done := make(chan struct{}) go func() { - runLoop(ctx, o, o.cfg, true /* sweep */) + // sweep=false: the fake's "issue list" stub answers every query with the + // same issue-7 payload regardless of --label, so a startup sweep for + // ai-wip would "find" issue 7 and clear the park cause this test set up + // before the cycle even runs. Sweeping stale ai-wip is exercised + // elsewhere (TestSweepOrphansRequeuesStaleWIP); this test is only about + // the pickup that follows a human removing ai-rework. + runLoop(ctx, o, o.cfg, false /* sweep */) close(done) }() select { @@ -55,8 +63,33 @@ func TestDaemonNeverScansOrResumesParkedIssues(t *testing.T) { if got := env.callsMatching("gh", "--label ai-rework"); len(got) != 0 { t.Errorf("a cycle must never scan the rework label, got %v", got) } - if got := env.callsMatching("claude", "--resume"); len(got) != 0 { - t.Errorf("a cycle must never resume a saved session, got %v", got) + if got := env.callsMatching("claude", "--resume s1"); len(got) == 0 { + t.Error("the requeued pickup must resume the session prepParked left behind") + } +} + +// Reproduces #49: a rework pickup whose resumed session's final reply reads +// like an already-done claim (plausible — a resumed session mid-implementation +// looking at its own partially-landed code can sound like that) must still +// ship a PR from whatever it committed, not close the issue with none. +func TestReworkPickupShipsEvenWhenReplyLooksAlreadyDone(t *testing.T) { + env := newFakeEnv(t) + prepParked(t, env, "claude execute: terminated: max_turns") + base := env.f.handler + env.f.handler = func(c rcall) (string, string, error) { + if c.name == "claude" && strings.Contains(c.stdin, "rework pickup") { + return claudeJSON("PIPELINE_ALREADY_DONE: this looks finished already", "s2"), "", nil + } + return base(c) + } + if err := runCycle(env.orchestrator()); err != nil { + t.Fatalf("cycle error = %v", err) + } + if len(env.callsMatching("gh", "pr create")) == 0 { + t.Error("a rework pickup must ship a PR even when the resumed reply looks already-done") + } + if len(env.callsMatching("gh", "issue close")) != 0 { + t.Error("a rework pickup must not close the issue as already implemented") } } diff --git a/pipeline_resume.go b/pipeline_resume.go new file mode 100644 index 0000000..29685cd --- /dev/null +++ b/pipeline_resume.go @@ -0,0 +1,32 @@ +package main + +import "context" + +// RunResumePipeline drives a rework pickup: a single Claude turn that resumes +// the exact session a prior run left off in, in its preserved worktree, +// instead of starting a fresh session that only sees the worktree's current +// contents and can mistake partial progress for a finished feature. Unlike +// RunBugPipeline and RunFeaturePipeline, it never checks for an +// already-implemented claim — that check exists to let a FRESH session bail +// out of duplicate work, and is meaningless (and actively harmful: it would +// close the issue with no PR) applied to a session that is itself mid-way +// through implementing the issue. +func RunResumePipeline(ctx context.Context, c *Claude, cfg *Config, wtPath, kind, sessionID string) error { + res, err := c.Call(ctx, ClaudeCall{ + Dir: wtPath, Label: "resume", Prompt: resumePrompt(), Resume: sessionID, + Model: cfg.Models.Architect, + SkipPermissions: true, + DisallowedTools: []string{"AskUserQuestion"}, + }) + // Record before the error check: an errored call (e.g. a 429 session limit) + // still returns a session id, so the next rework pickup resumes THAT one + // instead of the now-stale id this run started from. + if res != nil { + c.RecordSession(res.SessionID, kind) + } + return err +} + +func resumePrompt() string { + return mustRender("resume.md.tmpl", promptData()) +} diff --git a/pipeline_resume_test.go b/pipeline_resume_test.go new file mode 100644 index 0000000..1328927 --- /dev/null +++ b/pipeline_resume_test.go @@ -0,0 +1,106 @@ +package main + +import ( + "context" + "fmt" + "strings" + "testing" +) + +func TestResumePipelineResumesTheSavedSession(t *testing.T) { + f := &fakeRunner{queue: []rresp{{stdout: claudeJSON("Finished the implementation and committed.", "s2")}}} + c := &Claude{runner: f} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus", Effort: "high"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "bug", "s1"); err != nil { + t.Fatal(err) + } + if len(f.calls) != 1 { + t.Fatalf("calls = %d, want 1", len(f.calls)) + } + call := f.calls[0] + if argAfter(call.args, "--resume") != "s1" { + t.Errorf("must resume the saved session id, args = %v", call.args) + } + if call.dir != "/wt" || !hasArg(call.args, "--dangerously-skip-permissions") || + argAfter(call.args, "--model") != "opus" { + t.Errorf("call = %+v", call) + } +} + +// A rework pickup must never close the issue as already-implemented: that +// check exists to let a FRESH session bail out of duplicate work, and applied +// to a session mid-way through implementing the issue it is the exact bug +// this pipeline exists to fix (closing with no PR). So even a reply carrying +// the sentinel is not parsed as one. +func TestResumePipelineNeverReturnsAlreadyDone(t *testing.T) { + f := &fakeRunner{queue: []rresp{{stdout: claudeJSON( + "PIPELINE_ALREADY_DONE: looks finished to me", "s2")}}} + c := &Claude{runner: f} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "feature", "s1"); err != nil { + t.Fatalf("resume must not treat an already-done claim as an error outcome: %v", err) + } +} + +func TestResumePipelinePropagatesError(t *testing.T) { + f := &fakeRunner{queue: []rresp{{err: fmt.Errorf("exit 1")}}} + c := &Claude{runner: f} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "bug", "s1"); err == nil { + t.Error("want error, got nil") + } +} + +func TestResumePipelineRecordsSession(t *testing.T) { + logDir := t.TempDir() + f := &fakeRunner{queue: []rresp{{stdout: claudeJSON("Finished and committed.", "s2")}}} + c := &Claude{runner: f, logDir: logDir} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "feature", "s1"); err != nil { + t.Fatal(err) + } + si, err := readSession(logDir) + if err != nil { + t.Fatalf("session not recorded: %v", err) + } + if si.SessionID != "s2" || si.Kind != "feature" { + t.Errorf("session = %+v, want s2/feature", si) + } +} + +// A resume that errors again (e.g. a fresh 429) must still advance the saved +// session id, exactly like the bug/feature pipelines, so the NEXT rework +// pickup resumes the latest session instead of the now-stale one this run +// started from. +func TestResumePipelineRecordsSessionOnError(t *testing.T) { + logDir := t.TempDir() + f := &fakeRunner{queue: []rresp{{stdout: claudeErrorJSON("hit usage limit again", "s3")}}} + c := &Claude{runner: f, logDir: logDir} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "bug", "s1"); err == nil { + t.Fatal("want the error propagated so the issue is parked again") + } + si, err := readSession(logDir) + if err != nil { + t.Fatalf("session must be recorded even when the call errors: %v", err) + } + if si.SessionID != "s3" { + t.Errorf("session = %+v, want s3", si) + } +} + +func TestResumePipelinePromptContinuesRatherThanRestarts(t *testing.T) { + f := &fakeRunner{queue: []rresp{{stdout: claudeJSON("done", "s2")}}} + c := &Claude{runner: f} + cfg := &Config{Models: Models{Architect: ModelConfig{Model: "opus"}}} + if err := RunResumePipeline(context.Background(), c, cfg, "/wt", "bug", "s1"); err != nil { + t.Fatal(err) + } + prompt := f.calls[0].stdin + if !strings.Contains(strings.ToLower(prompt), "resuming") && !strings.Contains(strings.ToLower(prompt), "continue") { + t.Errorf("resume prompt should tell the session to continue rather than restart: %s", prompt) + } + if strings.Contains(prompt, alreadyDoneSentinel) { + t.Errorf("resume prompt must not invite an already-done claim: %s", prompt) + } +} diff --git a/prompts_golden_test.go b/prompts_golden_test.go index 430492f..21aa221 100644 --- a/prompts_golden_test.go +++ b/prompts_golden_test.go @@ -196,7 +196,7 @@ func TestGoldenNeedsInfoComment(t *testing.T) { } const parkHead = "\U0001f916 Parked as `ai-rework` — this issue will not be retried automatically.\n\n" + - "Remove the `ai-rework` label to queue a fresh attempt — any worktree, branch and logs this run produced are preserved and reused, so no work is lost." + "Remove the `ai-rework` label to resume this run in the same Claude session — the worktree, branch, logs and session are preserved, so nothing is redone from zero." // parkTail is the hidden marker every status comment ends with, so // FetchIssueContent can strip it back out of the next run's issue content. diff --git a/prompts_test.go b/prompts_test.go index 0ea492e..f893a4b 100644 --- a/prompts_test.go +++ b/prompts_test.go @@ -17,6 +17,7 @@ var promptTestData = map[string]map[string]any{ "plan.md.tmpl": {"SpecPath": "docs/spec.md"}, "execute.md.tmpl": {"PlanPath": "docs/plan.md"}, "debug.md.tmpl": {"Issue": "I", "Threshold": 70}, + "resume.md.tmpl": {}, "triage.md.tmpl": {"List": "[]"}, "pickup": {"Kind": "feature", "Branch": "b"}, "already-done": {"Reason": "R"}, diff --git a/tracker.go b/tracker.go index 1ad961a..c78938a 100644 --- a/tracker.go +++ b/tracker.go @@ -238,11 +238,12 @@ func clearState(logDir string) { _ = os.Remove(filepath.Join(logDir, stateFile)) } -// parkCauseFile holds the failure text that parked the issue as ai-rework. It is -// a diagnostic left next to the logs for whoever inspects the workDir: nothing in -// the daemon reads it back, because a parked issue only moves when a human -// removes the label. It is cleared when the issue leaves the parked state, so a -// stale cause can't outlive the failure it describes. +// parkCauseFile holds the failure text that parked the issue as ai-rework. It +// doubles as the on-disk marker handleIssue reads back (via hasParkCause) to +// tell a rework pickup — a human removed the label, so the issue is eligible +// again — apart from a genuinely fresh issue. It is cleared when the issue +// leaves the parked state, so a stale cause can't outlive the failure it +// describes and can't wrongly mark the issue's next fresh pickup as a rework. const parkCauseFile = "park-cause" // recordParkCause writes the park cause to /park-cause. Best-effort, @@ -265,6 +266,17 @@ func clearParkCause(logDir string) { _ = os.Remove(filepath.Join(logDir, parkCauseFile)) } +// hasParkCause reports whether logDir still carries a park cause, i.e. this +// issue was parked as ai-rework and never reached a terminal outcome since — +// the signal handleIssue uses to tell a rework pickup from a fresh one. +func hasParkCause(logDir string) bool { + if logDir == "" { + return false + } + _, err := os.Stat(filepath.Join(logDir, parkCauseFile)) + return err == nil +} + // scanLogs reads cfg.WorkDir/logs and returns one Ticket per issue- dir, // steps ordered by seq and cost summed, sorted by attention-priority status // tier then Number descending (see sortTickets). A missing logs dir yields an