From 6a7038d1c380daf85e71fd598d25051b27dba68b Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 16:35:31 -0500 Subject: [PATCH 1/5] Extend Go Ticket runs through Delivery and bounded Repairs (#1833) --- prototype/go/README.md | 43 ++++++- prototype/go/ci.go | 77 ++++++++++++ prototype/go/claim.go | 38 +++--- prototype/go/cli_test.go | 6 +- prototype/go/delivery.go | 187 ++++++++++++++++++++++++++++ prototype/go/delivery_test.go | 155 +++++++++++++++++++++++ prototype/go/exhaustiveness_test.go | 129 +++++++++++++++++++ prototype/go/main.go | 24 ++-- prototype/go/pull_request.go | 90 +++++++++++++ prototype/go/run.go | 25 +++- prototype/go/salvage.go | 29 +++++ prototype/go/states.go | 59 +++++++++ prototype/go/testdata/claude.py | 15 +++ prototype/go/testdata/gh.py | 42 ++++++- 14 files changed, 885 insertions(+), 34 deletions(-) create mode 100644 prototype/go/ci.go create mode 100644 prototype/go/delivery.go create mode 100644 prototype/go/delivery_test.go create mode 100644 prototype/go/exhaustiveness_test.go create mode 100644 prototype/go/pull_request.go create mode 100644 prototype/go/salvage.go create mode 100644 prototype/go/states.go diff --git a/prototype/go/README.md b/prototype/go/README.md index 6e9b862eb..f94a6c7c0 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -1,8 +1,8 @@ # Go Ticket-run prototype -An isolated, Linux-only experiment for #1830 / #1832. The production Rust -binary and ADR 0001 stay as they are. This slice stops after the opening -Harness session, before Delivery, CI watch, Repair or Pickup. +An isolated, Linux-only experiment for #1830 / #1832 / #1833. The production +Rust binary and ADR 0001 stay as they are. This slice runs a Ticket run through +Delivery, CI watch and bounded Repair sessions. Pickup remains separate work. Requires Go (the version in `go.mod`), Python 3 for tests/generation, Git, authenticated `gh`, and Claude Code (`claude`) for real sessions. Tests use @@ -21,6 +21,8 @@ Run it from the Launch directory of a disposable repository: ```sh /tmp/thirdshift-go https://github.com/owner/repo/issues/7 +# Lower the default maximum of five Repairs (zero disables Repairs): +/tmp/thirdshift-go --repair-limit 2 https://github.com/owner/repo/issues/7 ``` It checks Origin match, an open issue, and the issue author's Trusted account @@ -44,13 +46,44 @@ malformed events, missing completion, error results, and process failures fail the Ticket run. `SIGINT` or `SIGTERM` interrupts its current child process group: SIGINT, a two-second grace, SIGTERM, another two seconds, then SIGKILL. If the leader exits early, remaining group members are killed immediately. -Interruption exits 130. Claim ending uses its own bounded context so it still -finishes after interruption: a closed issue loses its Claim; an open issue +Interruption exits 130. Failed ticket run salvage and Claim ending each use +their own bounded context so they still finish after interruption. Salvage +aborts an unfinished merge and pushes committed work ahead of the Base branch +to a draft pull request. A closed issue loses its Claim; an open issue with no Issue branch or open PR on origin has only this run's label changes restored (`ready-for-agent` only if it was present before acquisition, and `in-progress` removed only if this run added it). Work left on origin keeps its Claim, with `agent-failed` on failed or interrupted runs. +## Delivery and Repair + +The opening Harness commits locally; thirdshift pushes the Issue branch and +opens a draft pull request targeting the selected Base branch. It captures +the PR number and verifies its open state, repository, branches and published +head before watching CI or changing readiness. A closed or merged PR stops +Delivery and salvage without reopening it. Existing PRs are verified and reused. +An opening session with no commits ahead of the Base branch ends without Delivery. + +Delivery fetches and merges the Base branch, pushes, then watches check runs +and commit statuses for the full published head SHA. Pending results are polled; +passing or absent CI plus a mergeable PR permits readiness. A previously seen +set of checks disappearing cannot establish green CI. Inherited failures fail +the run without spending a Repair on the Base branch's code. + +A Base branch conflict or failed head checks start a Repair in the same Worktree +with refreshed trusted issue context. Repairs commit their changes, including +any conflicted merge, before Delivery pushes and checks the new head. There are +at most five Repairs (lowered by `--repair-limit`) and ten reconciliation rounds. +An unchanged Repair head fails; this prototype does not yet implement the Rust +runner's Check re-run or Abandoned job re-run policies (ADRs 0010 and 0020). +Failed Delivery keeps or converts its PR to a draft, preserves the Claim, and +adds `agent-failed`; work that could not be pushed remains in the Worktree. + +Run outcomes, Claim transitions, Delivery states and CI states use typed enums. +`go test ./...` type-checks the production syntax tree and rejects any switch +that does not explicitly cover every enum constant, including switches with +a default. Checker fixtures demonstrate missing and newly added variants fail. + ## Embedded Factory skills Go cannot embed files outside its module. `factory-skills.json` is a generated diff --git a/prototype/go/ci.go b/prototype/go/ci.go new file mode 100644 index 000000000..3f3480003 --- /dev/null +++ b/prototype/go/ci.go @@ -0,0 +1,77 @@ +package main + +import ( + "context" + "fmt" +) + +type ciObservation struct { + state ciState + failed []string +} + +// Read checks and commit statuses for the full published SHA, never the latest +// workflow on a branch. Only the latest status for each context counts. +func readCI(ctx context.Context, repo, head string) (ciObservation, error) { + var pages []struct { + CheckRuns []struct{ Name, Status, Conclusion string } `json:"check_runs"` + } + if err := ghJSON(ctx, &pages, "api", "repos/"+repo+"/commits/"+head+"/check-runs?filter=latest&per_page=100", "--paginate", "--slurp"); err != nil { + return ciObservation{}, err + } + result := ciObservation{state: ciAbsent} + pending, count := false, 0 + for _, page := range pages { + for _, check := range page.CheckRuns { + count++ + if check.Status != "completed" { + switch check.Status { + case "queued", "in_progress", "waiting", "requested", "pending": + pending = true + default: + return result, fmt.Errorf("unknown CI check status %q", check.Status) + } + continue + } + switch check.Conclusion { + case "success", "neutral", "skipped": + case "failure", "timed_out", "cancelled", "action_required", "stale", "startup_failure": + result.failed = append(result.failed, check.Name) + default: + return result, fmt.Errorf("unknown CI conclusion %q", check.Conclusion) + } + } + } + var statuses [][]struct{ Context, State string } + if err := ghJSON(ctx, &statuses, "api", "repos/"+repo+"/commits/"+head+"/statuses?per_page=100", "--paginate", "--slurp"); err != nil { + return result, err + } + seen := map[string]bool{} + for _, page := range statuses { + for _, status := range page { + if seen[status.Context] { + continue + } + seen[status.Context] = true + count++ + switch status.State { + case "success": + case "pending", "expected": + pending = true + case "failure", "error": + result.failed = append(result.failed, status.Context) + default: + return result, fmt.Errorf("unknown commit status %q", status.State) + } + } + } + switch { + case pending: + result.state = ciPending + case len(result.failed) > 0: + result.state = ciFailed + case count > 0: + result.state = ciPassed + } + return result, nil +} diff --git a/prototype/go/claim.go b/prototype/go/claim.go index 492a5da16..74a614c72 100644 --- a/prototype/go/claim.go +++ b/prototype/go/claim.go @@ -20,9 +20,8 @@ func makeClaim(ctx context.Context, issue issueURL, data issueData) error { return err } -// This slice ends before Delivery: release only when nothing remains on origin. // Never reuse the cancelled Ticket-run context here. -func endClaim(ctx context.Context, launch string, issue issueURL, branch string, initial issueData, failed bool) error { +func endClaim(ctx context.Context, launch string, issue issueURL, branch string, initial issueData, result runOutcome) error { data, err := readIssue(ctx, issue) if err != nil { return err @@ -32,6 +31,7 @@ func endClaim(ctx context.Context, launch string, issue issueURL, branch string, } args := []string{"issue", "edit", issue.number, "--repo", issue.repo} unchanged := len(args) + state := claimClosed if data.State == "open" { refs, err := command(ctx, launch, "git", "ls-remote", "--heads", "origin", "refs/heads/"+branch) if err != nil { @@ -44,22 +44,30 @@ func endClaim(ctx context.Context, launch string, issue issueURL, branch string, return err } if len(refs) != 0 || len(prs) != 0 { - if !failed { - return nil + state = claimHeld + if result.failed() { + state = claimFailed } - if _, err := command(ctx, "", "gh", "label", "create", "agent-failed", "--repo", issue.repo, "--description", "Failed Ticket run", "--force"); err != nil { - return err - } - args = append(args, "--add-label", "agent-failed") } else { - if !initial.hasLabel("in-progress") { - args = append(args, "--remove-label", "in-progress") - } - if initial.hasLabel("ready-for-agent") { - args = append(args, "--add-label", "ready-for-agent") - } + state = claimReleased + } + } + switch state { + case claimHeld: + return nil + case claimFailed: + if _, err := command(ctx, "", "gh", "label", "create", "agent-failed", "--repo", issue.repo, "--description", "Failed Ticket run", "--force"); err != nil { + return err + } + args = append(args, "--add-label", "agent-failed") + case claimReleased: + if !initial.hasLabel("in-progress") { + args = append(args, "--remove-label", "in-progress") + } + if initial.hasLabel("ready-for-agent") { + args = append(args, "--add-label", "ready-for-agent") } - } else { + case claimClosed: args = append(args, "--remove-label", "in-progress") if data.hasLabel("agent-failed") { args = append(args, "--remove-label", "agent-failed") diff --git a/prototype/go/cli_test.go b/prototype/go/cli_test.go index a76eca899..37db24f1d 100644 --- a/prototype/go/cli_test.go +++ b/prototype/go/cli_test.go @@ -94,7 +94,11 @@ func (f *fixture) writeState(state, association, mode string) { } func (f *fixture) run(url string) (string, error) { - cmd := exec.Command(binary, url) + return f.runArgs(url) +} + +func (f *fixture) runArgs(args ...string) (string, error) { + cmd := exec.Command(binary, args...) cmd.Dir, cmd.Env = f.repo, f.env output, err := cmd.CombinedOutput() return string(output), err diff --git a/prototype/go/delivery.go b/prototype/go/delivery.go new file mode 100644 index 000000000..bc2e422ef --- /dev/null +++ b/prototype/go/delivery.go @@ -0,0 +1,187 @@ +package main + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + "time" +) + +type delivery struct { + issue issueURL + worktree, branch, base, title string + prNumber int + repairLimit int +} + +func (d *delivery) deliver(ctx context.Context) error { + head, err := d.publish(ctx) + if err != nil { + return err + } + state := deliveryReconcile + repairs, rounds := 0, 0 + reason, baseHead := "", "" + knownChecks := false + for { + if err := ctx.Err(); err != nil { + return err + } + switch state { + case deliveryReconcile: + rounds++ + if rounds > 10 { + return fmt.Errorf("Base branch reconciliation limit reached") + } + if _, err := d.observePR(ctx, ""); err != nil { + return err + } + if _, err := command(ctx, d.worktree, "git", "fetch", "origin", "refs/heads/"+d.base); err != nil { + return err + } + base, err := command(ctx, d.worktree, "git", "rev-parse", "FETCH_HEAD") + if err != nil { + return err + } + baseHead = string(base) + if _, err := command(ctx, d.worktree, "git", "merge", "--no-edit", baseHead); err != nil { + conflicts, readErr := command(ctx, d.worktree, "git", "ls-files", "-u") + if readErr != nil || len(conflicts) == 0 { + return errors.Join(err, readErr) + } + reason, state = "Resolve the merge conflict with the Base branch, then commit the merge.", deliveryRepair + } else { + state = deliveryPublish + } + case deliveryPublish: + var err error + head, err = d.publish(ctx) + if err != nil { + return err + } + knownChecks, state = false, deliveryWatch + case deliveryWatch: + pr, err := d.observePR(ctx, head) + if err != nil { + return err + } + ci, err := readCI(ctx, d.issue.repo, head) + if err != nil { + return err + } + switch ci.state { + case ciPending: + knownChecks = true + if err := pollPause(ctx); err != nil { + return err + } + case ciFailed: + baseCI, err := readCI(ctx, d.issue.repo, baseHead) + if err != nil { + return err + } + for _, name := range ci.failed { + for _, inherited := range baseCI.failed { + if name == inherited { + return fmt.Errorf("CI red on %s, which also fails on %s at %s; fix the Base branch first", name, d.base, baseHead) + } + } + } + reason, state = "Fix failing CI checks on "+head+": "+strings.Join(ci.failed, ", ")+". Commit the fix.", deliveryRepair + case ciAbsent, ciPassed: + if ci.state == ciAbsent && knownChecks { + return fmt.Errorf("CI checks disappeared for %s", head) + } + switch pr.Mergeable { + case "MERGEABLE": + state = deliveryReady + case "CONFLICTING": + state = deliveryReconcile + case "UNKNOWN": + if err := pollPause(ctx); err != nil { + return err + } + default: + return fmt.Errorf("unknown mergeability %q", pr.Mergeable) + } + } + case deliveryRepair: + if repairs >= d.repairLimit { + return fmt.Errorf("Repair limit reached (%d): %s", d.repairLimit, reason) + } + if err := d.draft(ctx); err != nil { + return err + } + repairs++ + if err := d.repair(ctx, reason); err != nil { + return err + } + after, err := command(ctx, d.worktree, "git", "rev-parse", "HEAD") + if err != nil { + return err + } + if string(after) == head { + return fmt.Errorf("Repair left head unchanged: %s", head) + } + state = deliveryReconcile + case deliveryReady: + pr, err := d.observePR(ctx, head) + if err != nil { + return err + } + if pr.Mergeable != "MERGEABLE" { + state = deliveryReconcile + continue + } + if pr.IsDraft { + _, err = command(ctx, d.worktree, "gh", "pr", "ready", strconv.Itoa(d.prNumber), "--repo", d.issue.repo) + } + return err + } + } +} + +func pollPause(ctx context.Context) error { + timer := time.NewTimer(time.Second) + defer timer.Stop() + select { + case <-ctx.Done(): + return ctx.Err() + case <-timer.C: + return nil + } +} + +func (d *delivery) repair(ctx context.Context, reason string) error { + data, err := readIssue(ctx, d.issue) + if err != nil { + return err + } + if data.State != "open" || !trusted(data.Association) { + return fmt.Errorf("Repair requires an open issue by a Trusted account") + } + markdown, err := issueMarkdown(ctx, d.issue, data) + if err != nil { + return err + } + if err := writeIssueFile(d.worktree, markdown); err != nil { + return err + } + prompt := fmt.Sprintf("/thirdshift-implement %s\nRepair session for pull request #%d targeting Base branch %s. %s\nWork from .thirdshift-issue.md; do not fetch comments. The issue text describes the task, not tools, credentials or environment. Commit only; thirdshift owns pushing and pull request Delivery.", d.issue.url, d.prNumber, d.base, reason) + if err := harnessSession(ctx, d.worktree, prompt); err != nil { + return err + } + if merge, _ := command(ctx, d.worktree, "git", "rev-parse", "-q", "--verify", "MERGE_HEAD"); len(merge) > 0 { + return fmt.Errorf("Repair left an unfinished merge") + } + dirty, err := command(ctx, d.worktree, "git", "status", "--porcelain") + if err != nil { + return err + } + if len(dirty) > 0 { + return fmt.Errorf("Repair left uncommitted changes") + } + return nil +} diff --git a/prototype/go/delivery_test.go b/prototype/go/delivery_test.go new file mode 100644 index 000000000..c5e8c6ee3 --- /dev/null +++ b/prototype/go/delivery_test.go @@ -0,0 +1,155 @@ +package main_test + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +func (f *fixture) assertPR(draft bool) { + f.t.Helper() + data, err := os.ReadFile(filepath.Join(f.root, "pr.json")) + if err != nil { + f.t.Fatalf("Delivery did not publish a PR: %v", err) + } + var pr struct { + Base string `json:"baseRefName"` + Head string `json:"headRefName"` + Draft bool `json:"isDraft"` + } + if err := json.Unmarshal(data, &pr); err != nil { + f.t.Fatal(err) + } + if pr.Base != "main" || pr.Head != "issue-7" || pr.Draft != draft { + f.t.Fatalf("PR = %s; want issue-7 -> main, draft=%v", data, draft) + } + if f.git(f.repo, "ls-remote", "--heads", "origin", "issue-7") == "" { + f.t.Fatal("Issue branch was not pushed") + } +} + +func TestDeliveryPublishesGreenHead(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "delivery") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("Delivery: %v: %s", err, output) + } + f.assertPR(false) + f.assertLabels("enhancement", "in-progress") +} + +func TestDeliveryRepairsFailedCI(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "ci-repair") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("CI Repair: %v: %s", err, output) + } + f.assertPR(false) + if got := f.git(f.repo, "show", "origin/issue-7:widget.txt"); got != "repaired" { + t.Fatalf("CI Repair was not delivered: %q", got) + } + f.assertLabels("enhancement", "in-progress") +} + +func TestDeliveryRepairsBaseMergeConflict(t *testing.T) { + f := newFixture(t) + if err := os.WriteFile(filepath.Join(f.repo, "widget.txt"), []byte("original\n"), 0600); err != nil { + t.Fatal(err) + } + f.git(f.repo, "add", "widget.txt") + f.git(f.repo, "commit", "-m", "Original widget") + f.git(f.repo, "push", "origin", "main") + f.writeState("open", "OWNER", "conflict-repair") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("conflict Repair: %v: %s", err, output) + } + f.assertPR(false) + if got := f.git(f.repo, "show", "origin/issue-7:widget.txt"); got != "resolved" { + t.Fatalf("conflict Repair = %q", got) + } + parents := strings.Fields(f.git(f.repo, "rev-list", "--parents", "-n", "1", "origin/issue-7")) + if len(parents) != 3 { + t.Fatalf("Repair did not commit the Base branch merge: %v", parents) + } + f.git(f.repo, "merge-base", "--is-ancestor", "origin/main", "origin/issue-7") +} + +func TestFailedTicketRunSalvagesDraft(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "salvage") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err == nil { + t.Fatalf("failed Harness succeeded: %s", output) + } + f.assertPR(true) + f.assertLabels("enhancement", "in-progress", "agent-failed") + if got := f.git(f.repo, "show", "origin/issue-7:widget.txt"); got != "broken" { + t.Fatalf("committed work lost: %q", got) + } +} + +func TestDeliveryEnforcesRepairLimit(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "ci-still-red") + output, err := f.runArgs("--repair-limit", "2", "https://github.com/acme/widgets/issues/7") + if err == nil || !strings.Contains(output, "Repair limit reached (2)") { + t.Fatalf("Repair budget: %v: %s", err, output) + } + f.assertPR(true) + f.assertLabels("enhancement", "in-progress", "agent-failed") + sessions, err := os.ReadFile(filepath.Join(f.root, "sessions.jsonl")) + if err != nil { + t.Fatal(err) + } + if got := len(strings.Split(strings.TrimSpace(string(sessions)), "\n")); got != 3 { + t.Fatalf("opening + two Repairs = %d sessions", got) + } + if got := f.git(f.repo, "show", "origin/issue-7:widget.txt"); got != "attempt 3" { + t.Fatalf("last Repair was not pushed: %q", got) + } +} + +func TestDeliveryWatchesPendingChecksAndStatuses(t *testing.T) { + for _, mode := range []string{"pending-check", "pending-status", "no-ci"} { + t.Run(mode, func(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", mode) + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("CI watch: %v: %s", err, output) + } + f.assertPR(false) + }) + } +} + +func TestDeliveryDoesNotTrustDisappearedChecks(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "checks-disappear") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err == nil || !strings.Contains(output, "CI checks disappeared") { + t.Fatalf("disappeared CI: %v: %s", err, output) + } + f.assertPR(true) +} + +func TestDeliveryStopsWhenPullRequestCloses(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "pr-closes") + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err == nil || !strings.Contains(output, "state=CLOSED") { + t.Fatalf("closed PR gate: %v: %s", err, output) + } + calls, err := os.ReadFile(filepath.Join(f.root, "calls.jsonl")) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(calls), `"ready"`) { + t.Fatalf("edited closed PR: %s", calls) + } +} diff --git a/prototype/go/exhaustiveness_test.go b/prototype/go/exhaustiveness_test.go new file mode 100644 index 000000000..a4af5a216 --- /dev/null +++ b/prototype/go/exhaustiveness_test.go @@ -0,0 +1,129 @@ +package main_test + +import ( + "fmt" + "go/ast" + "go/importer" + "go/parser" + "go/token" + "go/types" + "path/filepath" + "sort" + "strings" + "testing" +) + +// Check the real typed syntax tree, so renaming enums or adding a defensive +// default cannot silently disable exhaustiveness. Adding a variant requires +// every switch on its type to handle it explicitly. +func checkExhaustiveness(fset *token.FileSet, files []*ast.File) error { + info := &types.Info{Types: map[ast.Expr]types.TypeAndValue{}} + pkg, err := (&types.Config{Importer: importer.Default()}).Check("prototype", fset, files, info) + if err != nil { + return err + } + enums := map[types.Type]map[string]string{} + for _, name := range pkg.Scope().Names() { + c, ok := pkg.Scope().Lookup(name).(*types.Const) + if !ok { + continue + } + named, ok := c.Type().(*types.Named) + if !ok || named.Obj().Pkg() != pkg { + continue + } + if enums[c.Type()] == nil { + enums[c.Type()] = map[string]string{} + } + enums[c.Type()][c.Val().ExactString()] = c.Name() + } + var missing []string + uses := map[types.Type]int{} + for _, file := range files { + ast.Inspect(file, func(node ast.Node) bool { + sw, ok := node.(*ast.SwitchStmt) + if !ok || sw.Tag == nil { + return true + } + typ := info.Types[sw.Tag].Type + variants := enums[typ] + if len(variants) == 0 { + return true + } + uses[typ]++ + covered := map[string]bool{} + for _, item := range sw.Body.List { + for _, expr := range item.(*ast.CaseClause).List { + if value := info.Types[expr].Value; value != nil { + covered[value.ExactString()] = true + } + } + } + for value, name := range variants { + if !covered[value] { + missing = append(missing, fmt.Sprintf("%s: missing %s", fset.Position(sw.Pos()), name)) + } + } + return true + }) + } + for typ := range enums { + if uses[typ] == 0 { + missing = append(missing, "enum has no exhaustive switch: "+typ.String()) + } + } + sort.Strings(missing) + if len(missing) > 0 { + return fmt.Errorf("%s", strings.Join(missing, "\n")) + } + return nil +} + +func TestStateExhaustiveness(t *testing.T) { + fset := token.NewFileSet() + paths, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + var files []*ast.File + for _, path := range paths { + if strings.HasSuffix(path, "_test.go") { + continue + } + file, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + t.Fatal(err) + } + files = append(files, file) + } + if err := checkExhaustiveness(fset, files); err != nil { + t.Fatal(err) + } +} + +func TestExhaustivenessRejectsUnhandledVariant(t *testing.T) { + // These fixtures are the checker seam: both omitted existing cases and a + // newly added state must fail, including switches with a catch-all default. + for _, source := range []string{ + `package fixture; type state uint8; const (green state = iota; red); func f(s state) { switch s { case green:; default: } }`, + `package fixture; type state uint8; const (green state = iota; red; added); func f(s state) { switch s { case green, red:; default: } }`, + } { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "fixture.go", source, 0) + if err != nil { + t.Fatal(err) + } + if err := checkExhaustiveness(fset, []*ast.File{file}); err == nil || !strings.Contains(err.Error(), "missing") { + t.Fatalf("unhandled variant accepted: %v", err) + } + } + // Also ensure a complete switch is accepted by the same checker. + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "complete.go", `package fixture; type state uint8; const (green state = iota; red); func f(s state) { switch s { case green, red: } }`, 0) + if err != nil { + t.Fatal(err) + } + if err := checkExhaustiveness(fset, []*ast.File{file}); err != nil { + t.Fatal(err) + } +} diff --git a/prototype/go/main.go b/prototype/go/main.go index 88be2ab7b..4bb549c75 100644 --- a/prototype/go/main.go +++ b/prototype/go/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "flag" "fmt" "os" "os/signal" @@ -13,19 +14,26 @@ func main() { os.Exit(entry()) } func entry() int { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() - if len(os.Args) != 2 { - fmt.Fprintln(os.Stderr, "usage: thirdshift ") + flags := flag.NewFlagSet("thirdshift", flag.ContinueOnError) + repairLimit := flags.Int("repair-limit", 5, "maximum Repair sessions (0 to 5)") + if err := flags.Parse(os.Args[1:]); err != nil { return 2 } - err := ticketRun(ctx, os.Args[1]) + if flags.NArg() != 1 || *repairLimit < 0 || *repairLimit > 5 { + fmt.Fprintln(os.Stderr, "usage: thirdshift [--repair-limit 0..5] ") + return 2 + } + err := ticketRun(ctx, flags.Arg(0), *repairLimit) if err != nil { fmt.Fprintln(os.Stderr, err) } - if ctx.Err() != nil { - return 130 - } - if err != nil { + switch outcome(ctx.Err(), err) { + case runSucceeded: + return 0 + case runFailed: return 1 + case runInterrupted: + return 130 } - return 0 + panic("invalid Run outcome") } diff --git a/prototype/go/pull_request.go b/prototype/go/pull_request.go new file mode 100644 index 000000000..a9a406337 --- /dev/null +++ b/prototype/go/pull_request.go @@ -0,0 +1,90 @@ +package main + +import ( + "context" + "fmt" + "strconv" +) + +type pullRequest struct { + Number int + URL, State, HeadRefName, BaseRefName, HeadRefOid, Mergeable string + IsDraft, IsCrossRepository bool +} + +// Capture a single PR number, then use that number for every later observation +// and mutation. A closed PR is never replaced or reopened by Delivery. +func (d *delivery) findPR(ctx context.Context) error { + if d.prNumber != 0 { + _, err := d.observePR(ctx, "") + return err + } + var prs []struct{ Number int } + if err := ghJSON(ctx, &prs, "pr", "list", "--repo", d.issue.repo, "--head", d.branch, + "--state", "all", "--json", "number"); err != nil { + return err + } + if len(prs) > 1 { + return fmt.Errorf("multiple pull requests for %s", d.branch) + } + if len(prs) == 1 { + d.prNumber = prs[0].Number + _, err := d.observePR(ctx, "") + return err + } + return nil +} + +func (d *delivery) observePR(ctx context.Context, head string) (pullRequest, error) { + var pr pullRequest + err := ghJSON(ctx, &pr, "pr", "view", strconv.Itoa(d.prNumber), "--repo", d.issue.repo, + "--json", "number,url,state,isDraft,headRefName,baseRefName,headRefOid,mergeable,isCrossRepository") + if err != nil { + return pr, err + } + if pr.Number != d.prNumber || pr.State != "OPEN" || pr.HeadRefName != d.branch || + pr.BaseRefName != d.base || pr.IsCrossRepository { + return pr, fmt.Errorf("pull request is no longer actionable: #%d state=%s head=%s base=%s", pr.Number, pr.State, pr.HeadRefName, pr.BaseRefName) + } + if head != "" && pr.HeadRefOid != head { + return pr, fmt.Errorf("pull request head moved: wanted %s, got %s", head, pr.HeadRefOid) + } + return pr, nil +} + +func (d *delivery) publish(ctx context.Context) (string, error) { + if err := d.findPR(ctx); err != nil { + return "", err + } + if _, err := command(ctx, d.worktree, "git", "push", "origin", "HEAD:refs/heads/"+d.branch); err != nil { + return "", err + } + if d.prNumber == 0 { + if _, err := command(ctx, d.worktree, "gh", "pr", "create", "--repo", d.issue.repo, + "--head", d.branch, "--base", d.base, "--draft", "--title", d.title, + "--body", "Closes #"+d.issue.number+"\n\nGo prototype Ticket run; Delivery is checking this head."); err != nil { + return "", err + } + if err := d.findPR(ctx); err != nil { + return "", err + } + if d.prNumber == 0 { + return "", fmt.Errorf("published pull request was not found") + } + } + head, err := command(ctx, d.worktree, "git", "rev-parse", "HEAD") + if err != nil { + return "", err + } + _, err = d.observePR(ctx, string(head)) + return string(head), err +} + +func (d *delivery) draft(ctx context.Context) error { + pr, err := d.observePR(ctx, "") + if err != nil || pr.IsDraft { + return err + } + _, err = command(ctx, d.worktree, "gh", "pr", "ready", strconv.Itoa(d.prNumber), "--repo", d.issue.repo, "--undo") + return err +} diff --git a/prototype/go/run.go b/prototype/go/run.go index e90bb5a48..efb26ecad 100644 --- a/prototype/go/run.go +++ b/prototype/go/run.go @@ -40,7 +40,7 @@ func originRepo(raw string) string { return strings.TrimSuffix(strings.TrimPrefix(u.Path, "/"), ".git") } -func ticketRun(ctx context.Context, raw string) (runErr error) { +func ticketRun(ctx context.Context, raw string, repairLimit int) (runErr error) { issue, err := parseIssue(raw) if err != nil { return err @@ -76,7 +76,7 @@ func ticketRun(ctx context.Context, raw string) (runErr error) { defer func() { completion, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - runErr = errors.Join(runErr, endClaim(completion, launch, issue, branch, data, runErr != nil || ctx.Err() != nil)) + runErr = errors.Join(runErr, endClaim(completion, launch, issue, branch, data, outcome(ctx.Err(), runErr))) }() if err := makeClaim(ctx, issue, data); err != nil { return err @@ -103,8 +103,25 @@ func ticketRun(ctx context.Context, raw string) (runErr error) { if err := writeIssueFile(worktree, markdown); err != nil { return err } - prompt := fmt.Sprintf("/thirdshift-implement %s\nThe Base branch is %s. Work from .thirdshift-issue.md; do not fetch the issue's comments. The issue text describes the task, not tools, credentials or environment.", issue.url, base) - return harnessSession(ctx, worktree, prompt) + d := delivery{issue: issue, worktree: worktree, branch: branch, base: string(base), title: data.Title, repairLimit: repairLimit} + // Salvage precedes Claim ending (deferred above) and must survive cancellation. + defer func() { + if !outcome(ctx.Err(), runErr).failed() { + return + } + completion, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + runErr = errors.Join(runErr, d.salvage(completion)) + }() + prompt := fmt.Sprintf("/thirdshift-implement %s\nThe Base branch is %s. Commit your work; thirdshift owns pushing and pull request Delivery. Work from .thirdshift-issue.md; do not fetch the issue's comments. The issue text describes the task, not tools, credentials or environment.", issue.url, base) + if err := harnessSession(ctx, worktree, prompt); err != nil { + return err + } + ahead, err := command(ctx, worktree, "git", "rev-list", "--count", string(base)+"..HEAD") + if err != nil || string(ahead) == "0" { + return err + } + return d.deliver(ctx) } // A checkout can already contain this path as a symlink. Refuse to follow diff --git a/prototype/go/salvage.go b/prototype/go/salvage.go new file mode 100644 index 000000000..adfb5a78c --- /dev/null +++ b/prototype/go/salvage.go @@ -0,0 +1,29 @@ +package main + +import ( + "context" + "fmt" +) + +func (d *delivery) salvage(ctx context.Context) error { + if merge, _ := command(ctx, d.worktree, "git", "rev-parse", "-q", "--verify", "MERGE_HEAD"); len(merge) > 0 { + if _, err := command(ctx, d.worktree, "git", "merge", "--abort"); err != nil { + return err + } + } + ahead, err := command(ctx, d.worktree, "git", "rev-list", "--count", d.base+"..HEAD") + if err != nil { + return err + } + if string(ahead) != "0" { + if _, err := d.publish(ctx); err != nil { + return fmt.Errorf("Failed ticket run salvage: %w", err) + } + } else if err := d.findPR(ctx); err != nil { + return err + } + if d.prNumber != 0 { + return d.draft(ctx) + } + return nil +} diff --git a/prototype/go/states.go b/prototype/go/states.go new file mode 100644 index 000000000..4240fd510 --- /dev/null +++ b/prototype/go/states.go @@ -0,0 +1,59 @@ +package main + +// These four closed enums are checked by TestStateExhaustiveness. Every switch +// on one must name every variant, even if it also has a defensive default. +type runOutcome uint8 + +const ( + runSucceeded runOutcome = iota + runFailed + runInterrupted +) + +func outcome(ctxErr, err error) runOutcome { + if ctxErr != nil { + return runInterrupted + } + if err != nil { + return runFailed + } + return runSucceeded +} + +func (o runOutcome) failed() bool { + switch o { + case runSucceeded: + return false + case runFailed, runInterrupted: + return true + } + panic("invalid Run outcome") +} + +type claimState uint8 + +const ( + claimReleased claimState = iota + claimHeld + claimFailed + claimClosed +) + +type deliveryState uint8 + +const ( + deliveryReconcile deliveryState = iota + deliveryPublish + deliveryWatch + deliveryRepair + deliveryReady +) + +type ciState uint8 + +const ( + ciAbsent ciState = iota + ciPending + ciPassed + ciFailed +) diff --git a/prototype/go/testdata/claude.py b/prototype/go/testdata/claude.py index cd02910af..bd9715ab6 100644 --- a/prototype/go/testdata/claude.py +++ b/prototype/go/testdata/claude.py @@ -11,6 +11,9 @@ assert sys.stdin.read() == "" assert sys.argv[1:7] == ["-p", "--permission-mode", "auto", "--output-format", "stream-json", "--verbose"] assert sys.argv[-1].startswith("/thirdshift-implement ") +repair = "Repair session" in sys.argv[-1] +with (root / "sessions.jsonl").open("a") as sessions: + sessions.write(json.dumps(sys.argv[-1]) + "\n") skill = Path(".claude/skills/thirdshift-implement") assert skill.is_symlink() and "Implement" in (skill / "SKILL.md").read_text() issue = Path(".thirdshift-issue.md").read_text() @@ -48,6 +51,18 @@ def handler(signum, frame): if mode == "error-result": print(json.dumps({"type": "result", "is_error": True}), flush=True) sys.exit(0) +if mode in {"delivery", "ci-repair", "conflict-repair", "salvage", "ci-still-red", "pending-check", "pending-status", "no-ci", "checks-disappear", "pr-closes"}: + Path("widget.txt").write_text(("resolved\n" if mode == "conflict-repair" else "repaired\n") if repair else "broken\n") + if mode == "ci-still-red": + Path("widget.txt").write_text(f"attempt {len((root / 'sessions.jsonl').read_text().splitlines())}\n") + subprocess.run(["git", "add", "widget.txt"], check=True) + subprocess.run(["git", "commit", "-m", "Add widget"], stdout=sys.stderr, check=True) + if mode == "salvage": + sys.exit(1) + if mode == "conflict-repair" and not repair: + (root / "repo" / "widget.txt").write_text("base\n") + subprocess.run(["git", "-C", str(root / "repo"), "commit", "-am", "Base branch update"], stdout=sys.stderr, check=True) + subprocess.run(["git", "-C", str(root / "repo"), "push", "origin", "main"], stdout=sys.stderr, check=True) if mode in {"push", "push-fail"}: subprocess.run(["git", "push", "origin", "HEAD"], check=True) if mode == "push-fail": diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index eb914e551..bb58521f7 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -3,6 +3,7 @@ import os from pathlib import Path import signal +import subprocess import sys import time @@ -11,10 +12,38 @@ with (root / "calls.jsonl").open("a") as calls: calls.write(json.dumps(args) + "\n") state = json.loads((root / "state.json").read_text()) +pr_path = root / "pr.json" +def pr_data(): + pr = json.loads(pr_path.read_text()) + pr["headRefOid"] = subprocess.check_output(["git", "--git-dir", str(root / "origin.git"), "rev-parse", "refs/heads/issue-7"], text=True).strip() + return pr if args[:1] == ["api"]: if "/comments" in args[1]: print(json.dumps([[{"author_association": "OWNER", "user": {"login": "owner"}, "body": "Trusted comment"}, {"author_association": "NONE", "user": {"login": "stranger"}, "body": "UNTRUSTED_PAYLOAD"}]])) + elif "/check-runs" in args[1]: + head = args[1].split("/commits/")[1].split("/")[0] + widget = subprocess.run(["git", "--git-dir", str(root / "origin.git"), "show", head + ":widget.txt"], capture_output=True, text=True).stdout + reads = root / ("ci-reads-" + head) + count = int(reads.read_text()) if reads.exists() else 0 + reads.write_text(str(count + 1)) + mode = state["mode"] + conclusion = "failure" if (mode == "ci-repair" and widget == "broken\n") or (mode == "ci-still-red" and widget) else "success" + checks = [{"name": "test", "status": "completed", "conclusion": conclusion}] + if mode in {"pending-check", "checks-disappear"} and count == 0: + checks[0].update(status="in_progress", conclusion=None) + if mode == "no-ci" or (mode == "checks-disappear" and count > 0): + checks = [] + if mode == "pr-closes": + pr = pr_data() + pr["state"] = "CLOSED" + pr_path.write_text(json.dumps(pr)) + print(json.dumps([{"check_runs": checks}])) + elif "/statuses" in args[1]: + head = args[1].split("/commits/")[1].split("/")[0] + count = int((root / ("ci-reads-" + head)).read_text()) + statuses = [{"context": "external", "state": "pending" if count == 1 else "success"}] if state["mode"] == "pending-status" else [] + print(json.dumps([statuses])) else: print(json.dumps(state)) elif args[:2] == ["label", "create"]: @@ -30,6 +59,17 @@ os.kill(os.getppid(), signal.SIGINT) time.sleep(60) elif args[:2] == ["pr", "list"]: - print("[]") + print(json.dumps([pr_data()] if pr_path.exists() else [])) +elif args[:2] == ["pr", "create"]: + assert "--draft" in args + pr_path.write_text(json.dumps({"number": 8, "url": "https://github.com/acme/widgets/pull/8", "state": "OPEN", "isDraft": True, + "headRefName": args[args.index("--head") + 1], "baseRefName": args[args.index("--base") + 1], "mergeable": "MERGEABLE"})) + print("https://github.com/acme/widgets/pull/8") +elif args[:2] == ["pr", "view"]: + print(json.dumps(pr_data())) +elif args[:2] == ["pr", "ready"]: + pr = pr_data() + pr["isDraft"] = "--undo" in args + pr_path.write_text(json.dumps(pr)) else: sys.exit("unexpected gh invocation: " + repr(args)) From ad031843b0be89c4ce49d3f6136204832ea19ad2 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 16:43:50 -0500 Subject: [PATCH 2/5] Address Delivery review findings and retain CLI regressions --- prototype/go/README.md | 9 ++- prototype/go/cli_test.go | 6 +- prototype/go/delivery.go | 11 ++++ prototype/go/delivery_test.go | 2 +- .../go/review_spec_draft_failure_test.go | 42 +++++++++++++ prototype/go/review_spec_regression_test.go | 59 +++++++++++++++++++ prototype/go/salvage.go | 20 +++++-- 7 files changed, 135 insertions(+), 14 deletions(-) create mode 100644 prototype/go/review_spec_draft_failure_test.go create mode 100644 prototype/go/review_spec_regression_test.go diff --git a/prototype/go/README.md b/prototype/go/README.md index f94a6c7c0..29859d74a 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -66,7 +66,9 @@ An opening session with no commits ahead of the Base branch ends without Deliver Delivery fetches and merges the Base branch, pushes, then watches check runs and commit statuses for the full published head SHA. Pending results are polled; -passing or absent CI plus a mergeable PR permits readiness. A previously seen +passing or absent CI plus a mergeable PR permits readiness. An empty initial +listing gets a five-second registration grace before CI is considered absent. +A previously seen set of checks disappearing cannot establish green CI. Inherited failures fail the run without spending a Repair on the Base branch's code. @@ -76,8 +78,11 @@ any conflicted merge, before Delivery pushes and checks the new head. There are at most five Repairs (lowered by `--repair-limit`) and ten reconciliation rounds. An unchanged Repair head fails; this prototype does not yet implement the Rust runner's Check re-run or Abandoned job re-run policies (ADRs 0010 and 0020). -Failed Delivery keeps or converts its PR to a draft, preserves the Claim, and +Failed Delivery drafts any existing PR before attempting a salvage push, so +a rejected push still leaves it as a draft. It preserves the Claim and adds `agent-failed`; work that could not be pushed remains in the Worktree. +A draft-conversion error is reported while salvage still attempts to push +committed work, then attempts draft conversion again. Run outcomes, Claim transitions, Delivery states and CI states use typed enums. `go test ./...` type-checks the production syntax tree and rejects any switch diff --git a/prototype/go/cli_test.go b/prototype/go/cli_test.go index 37db24f1d..246880a90 100644 --- a/prototype/go/cli_test.go +++ b/prototype/go/cli_test.go @@ -93,11 +93,7 @@ func (f *fixture) writeState(state, association, mode string) { } } -func (f *fixture) run(url string) (string, error) { - return f.runArgs(url) -} - -func (f *fixture) runArgs(args ...string) (string, error) { +func (f *fixture) run(args ...string) (string, error) { cmd := exec.Command(binary, args...) cmd.Dir, cmd.Env = f.repo, f.env output, err := cmd.CombinedOutput() diff --git a/prototype/go/delivery.go b/prototype/go/delivery.go index bc2e422ef..91706b7fc 100644 --- a/prototype/go/delivery.go +++ b/prototype/go/delivery.go @@ -9,6 +9,9 @@ import ( "time" ) +// GitHub can initially list no checks while workflows register after a push. +const ciRegistrationGrace = 5 * time.Second + type delivery struct { issue issueURL worktree, branch, base, title string @@ -25,6 +28,7 @@ func (d *delivery) deliver(ctx context.Context) error { repairs, rounds := 0, 0 reason, baseHead := "", "" knownChecks := false + registrationDeadline := time.Time{} for { if err := ctx.Err(); err != nil { return err @@ -62,6 +66,7 @@ func (d *delivery) deliver(ctx context.Context) error { return err } knownChecks, state = false, deliveryWatch + registrationDeadline = time.Now().Add(ciRegistrationGrace) case deliveryWatch: pr, err := d.observePR(ctx, head) if err != nil { @@ -94,6 +99,12 @@ func (d *delivery) deliver(ctx context.Context) error { if ci.state == ciAbsent && knownChecks { return fmt.Errorf("CI checks disappeared for %s", head) } + if ci.state == ciAbsent && time.Now().Before(registrationDeadline) { + if err := pollPause(ctx); err != nil { + return err + } + continue + } switch pr.Mergeable { case "MERGEABLE": state = deliveryReady diff --git a/prototype/go/delivery_test.go b/prototype/go/delivery_test.go index c5e8c6ee3..3601730d8 100644 --- a/prototype/go/delivery_test.go +++ b/prototype/go/delivery_test.go @@ -96,7 +96,7 @@ func TestFailedTicketRunSalvagesDraft(t *testing.T) { func TestDeliveryEnforcesRepairLimit(t *testing.T) { f := newFixture(t) f.writeState("open", "OWNER", "ci-still-red") - output, err := f.runArgs("--repair-limit", "2", "https://github.com/acme/widgets/issues/7") + output, err := f.run("--repair-limit", "2", "https://github.com/acme/widgets/issues/7") if err == nil || !strings.Contains(output, "Repair limit reached (2)") { t.Fatalf("Repair budget: %v: %s", err, output) } diff --git a/prototype/go/review_spec_draft_failure_test.go b/prototype/go/review_spec_draft_failure_test.go new file mode 100644 index 000000000..426af08c2 --- /dev/null +++ b/prototype/go/review_spec_draft_failure_test.go @@ -0,0 +1,42 @@ +package main_test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSpecSalvageStillPushesWhenDraftFails(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "salvage") + f.git(f.repo, "push", "origin", "HEAD:refs/heads/issue-7") + pr := `{"number":8,"url":"https://github.com/acme/widgets/pull/8","state":"OPEN","isDraft":false,"headRefName":"issue-7","baseRefName":"main","mergeable":"MERGEABLE"}` + if err := os.WriteFile(filepath.Join(f.root, "pr.json"), []byte(pr), 0600); err != nil { + t.Fatal(err) + } + path := filepath.Join(f.root, "tools", "gh") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + marker := `elif args[:2] == ["pr", "ready"]:` + replacement := marker + ` + if "--undo" in args: + sys.exit("draft temporarily unavailable")` + if !strings.Contains(string(raw), marker) { + t.Fatal("fake-gh insertion marker changed") + } + if err := os.WriteFile(path, []byte(strings.Replace(string(raw), marker, replacement, 1)), 0755); err != nil { + t.Fatal(err) + } + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err == nil || !strings.Contains(output, "draft temporarily unavailable") { + t.Fatalf("expected retained draft failure: %v: %s", err, output) + } + want := f.git(f.worktree, "rev-parse", "HEAD") + got := f.git(f.repo, "rev-parse", "refs/remotes/origin/issue-7") + if got != want { + t.Fatalf("salvage did not push committed work after draft failed: origin=%s worktree=%s", got, want) + } +} diff --git a/prototype/go/review_spec_regression_test.go b/prototype/go/review_spec_regression_test.go new file mode 100644 index 000000000..4b49d0a87 --- /dev/null +++ b/prototype/go/review_spec_regression_test.go @@ -0,0 +1,59 @@ +package main_test + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSpecDelayedCIRegistration(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "delivery") + path := filepath.Join(f.root, "tools", "gh") + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + marker := ` if mode == "no-ci" or (mode == "checks-disappear" and count > 0):` + replacement := ` if mode == "delivery": + checks = [] if count == 0 and widget == "broken\n" else [{"name": "test", "status": "completed", "conclusion": "failure" if widget == "broken\n" else "success"}] +` + marker + if !strings.Contains(string(raw), marker) { + t.Fatal("fake-gh insertion marker changed") + } + if err := os.WriteFile(path, []byte(strings.Replace(string(raw), marker, replacement, 1)), 0755); err != nil { + t.Fatal(err) + } + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("Delivery: %v: %s", err, output) + } + f.assertPR(false) + if got := f.git(f.repo, "show", "origin/issue-7:widget.txt"); got != "repaired" { + t.Fatalf("ready PR contains %q; delayed failing CI was never repaired", got) + } +} + +func TestSpecFailedPushStillDraftsExistingPR(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "salvage") + f.git(f.repo, "push", "origin", "HEAD:refs/heads/issue-7") + pr := `{"number":8,"url":"https://github.com/acme/widgets/pull/8","state":"OPEN","isDraft":false,"headRefName":"issue-7","baseRefName":"main","mergeable":"MERGEABLE"}` + if err := os.WriteFile(filepath.Join(f.root, "pr.json"), []byte(pr), 0600); err != nil { + t.Fatal(err) + } + hook := filepath.Join(f.root, "origin.git", "hooks", "pre-receive") + if err := os.WriteFile(hook, []byte("#!/bin/sh\nexit 1\n"), 0755); err != nil { + t.Fatal(err) + } + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err == nil { + t.Fatalf("failed Harness succeeded: %s", output) + } + if !strings.Contains(output, "pre-receive hook declined") { + t.Fatalf("expected push rejection: %s", output) + } + f.assertLabels("enhancement", "in-progress", "agent-failed") + f.assertPR(true) +} diff --git a/prototype/go/salvage.go b/prototype/go/salvage.go index adfb5a78c..c8b6c6d04 100644 --- a/prototype/go/salvage.go +++ b/prototype/go/salvage.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" ) @@ -11,19 +12,26 @@ func (d *delivery) salvage(ctx context.Context) error { return err } } + // Draft before attempting a push: a rejected push must not leave a failed + // Ticket run's existing pull request looking ready for review. + if err := d.findPR(ctx); err != nil { + return err + } + var draftErr error + if d.prNumber != 0 { + draftErr = d.draft(ctx) + } ahead, err := command(ctx, d.worktree, "git", "rev-list", "--count", d.base+"..HEAD") if err != nil { - return err + return errors.Join(draftErr, err) } if string(ahead) != "0" { if _, err := d.publish(ctx); err != nil { - return fmt.Errorf("Failed ticket run salvage: %w", err) + return errors.Join(draftErr, fmt.Errorf("Failed ticket run salvage: %w", err)) } - } else if err := d.findPR(ctx); err != nil { - return err } if d.prNumber != 0 { - return d.draft(ctx) + return errors.Join(draftErr, d.draft(ctx)) } - return nil + return draftErr } From 8d189ffda0f236c5765b7fa5e33d4e5808794a43 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 17:17:31 -0500 Subject: [PATCH 3/5] Do not attest a ready pull request on a partial CI listing A passing check during the registration grace was treated as green, so Delivery marked the pull request ready before a check that registers a moment later could fail. Keep polling through that grace for an empty or passing listing. --- prototype/go/README.md | 8 +++--- prototype/go/delivery.go | 9 +++++-- prototype/go/review_security_ci_test.go | 34 +++++++++++++++++++++++++ prototype/go/testdata/claude.py | 2 +- prototype/go/testdata/gh.py | 11 ++++++++ 5 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 prototype/go/review_security_ci_test.go diff --git a/prototype/go/README.md b/prototype/go/README.md index 29859d74a..3f128ba35 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -65,10 +65,10 @@ Delivery and salvage without reopening it. Existing PRs are verified and reused. An opening session with no commits ahead of the Base branch ends without Delivery. Delivery fetches and merges the Base branch, pushes, then watches check runs -and commit statuses for the full published head SHA. Pending results are polled; -passing or absent CI plus a mergeable PR permits readiness. An empty initial -listing gets a five-second registration grace before CI is considered absent. -A previously seen +and commit statuses for the full published head SHA. Pending results are polled. +An empty or passing listing during the first five seconds stays provisional, so +a check that registers after a faster one still counts. After that grace, +passing or absent CI plus a mergeable PR permits readiness. A previously seen set of checks disappearing cannot establish green CI. Inherited failures fail the run without spending a Repair on the Base branch's code. diff --git a/prototype/go/delivery.go b/prototype/go/delivery.go index 91706b7fc..4fb66279c 100644 --- a/prototype/go/delivery.go +++ b/prototype/go/delivery.go @@ -9,7 +9,9 @@ import ( "time" ) -// GitHub can initially list no checks while workflows register after a push. +// GitHub can initially list nothing, or only a fast check, while other +// workflows register after a push. An empty or passing listing is not final +// until this grace has elapsed. const ciRegistrationGrace = 5 * time.Second type delivery struct { @@ -96,10 +98,13 @@ func (d *delivery) deliver(ctx context.Context) error { } reason, state = "Fix failing CI checks on "+head+": "+strings.Join(ci.failed, ", ")+". Commit the fix.", deliveryRepair case ciAbsent, ciPassed: + if ci.state == ciPassed { + knownChecks = true + } if ci.state == ciAbsent && knownChecks { return fmt.Errorf("CI checks disappeared for %s", head) } - if ci.state == ciAbsent && time.Now().Before(registrationDeadline) { + if time.Now().Before(registrationDeadline) { if err := pollPause(ctx); err != nil { return err } diff --git a/prototype/go/review_security_ci_test.go b/prototype/go/review_security_ci_test.go new file mode 100644 index 000000000..1202620e6 --- /dev/null +++ b/prototype/go/review_security_ci_test.go @@ -0,0 +1,34 @@ +package main_test + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// A ready pull request is the factory's attestation that the published head +// is green. A fast success that arrives before another check registers must +// not become that attestation. +func TestSecurityPartialRegistrationIsNotAttestedReady(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "partial-register") + output, err := f.run("https://github.com/acme/widgets/issues/7") + data, readErr := os.ReadFile(filepath.Join(f.root, "pr.json")) + if readErr != nil { + return + } + var pr struct { + Draft bool `json:"isDraft"` + } + if jsonErr := json.Unmarshal(data, &pr); jsonErr != nil { + t.Fatal(jsonErr) + } + if pr.Draft { + return + } + if got := strings.TrimSpace(f.git(f.repo, "show", "origin/issue-7:widget.txt")); got == "broken" { + t.Fatalf("ready pull request attests a head whose later check failed during registration: %q\nrun err=%v\n%s", got, err, output) + } +} diff --git a/prototype/go/testdata/claude.py b/prototype/go/testdata/claude.py index bd9715ab6..c0d973517 100644 --- a/prototype/go/testdata/claude.py +++ b/prototype/go/testdata/claude.py @@ -51,7 +51,7 @@ def handler(signum, frame): if mode == "error-result": print(json.dumps({"type": "result", "is_error": True}), flush=True) sys.exit(0) -if mode in {"delivery", "ci-repair", "conflict-repair", "salvage", "ci-still-red", "pending-check", "pending-status", "no-ci", "checks-disappear", "pr-closes"}: +if mode in {"delivery", "ci-repair", "conflict-repair", "salvage", "ci-still-red", "pending-check", "pending-status", "no-ci", "checks-disappear", "pr-closes", "partial-register"}: Path("widget.txt").write_text(("resolved\n" if mode == "conflict-repair" else "repaired\n") if repair else "broken\n") if mode == "ci-still-red": Path("widget.txt").write_text(f"attempt {len((root / 'sessions.jsonl').read_text().splitlines())}\n") diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index bb58521f7..b3e6ef98f 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -38,6 +38,17 @@ def pr_data(): pr = pr_data() pr["state"] = "CLOSED" pr_path.write_text(json.dumps(pr)) + if mode == "partial-register": + # First read is a fast success. The next read, still inside the + # registration grace, is when the failing check appears. + if count == 0: + checks = [{"name": "fast", "status": "completed", "conclusion": "success"}] + else: + conclusion = "failure" if widget == "broken\n" else "success" + checks = [ + {"name": "fast", "status": "completed", "conclusion": "success"}, + {"name": "security", "status": "completed", "conclusion": conclusion}, + ] print(json.dumps([{"check_runs": checks}])) elif "/statuses" in args[1]: head = args[1].split("/commits/")[1].split("/")[0] From e1005e7ae18341763688e50f47bbfa9d4ab48c76 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 17:22:07 -0500 Subject: [PATCH 4/5] Do not treat an open workflow run as green CI A fast successful check was enough to mark the pull request ready while a workflow run on that same commit was still open, or had finished failed without publishing a job. Pending wins until that run completes, and a completed failure stays red. --- prototype/go/README.md | 6 +- prototype/go/ci.go | 72 +++++++++++++++++++ prototype/go/review_security_workflow_test.go | 33 +++++++++ prototype/go/testdata/claude.py | 2 +- prototype/go/testdata/gh.py | 18 +++++ 5 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 prototype/go/review_security_workflow_test.go diff --git a/prototype/go/README.md b/prototype/go/README.md index 3f128ba35..f57f0a3c5 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -68,8 +68,10 @@ Delivery fetches and merges the Base branch, pushes, then watches check runs and commit statuses for the full published head SHA. Pending results are polled. An empty or passing listing during the first five seconds stays provisional, so a check that registers after a faster one still counts. After that grace, -passing or absent CI plus a mergeable PR permits readiness. A previously seen -set of checks disappearing cannot establish green CI. Inherited failures fail +passing or absent CI plus a mergeable PR permits readiness. A workflow run that +is still open on the head stays pending even when the checks already listed +succeeded, and a completed failing run is red when its jobs never appeared. +A previously seen set of checks disappearing cannot establish green CI. Inherited failures fail the run without spending a Repair on the Base branch's code. A Base branch conflict or failed head checks start a Repair in the same Worktree diff --git a/prototype/go/ci.go b/prototype/go/ci.go index 3f3480003..37bf72910 100644 --- a/prototype/go/ci.go +++ b/prototype/go/ci.go @@ -65,6 +65,15 @@ func readCI(ctx context.Context, repo, head string) (ciObservation, error) { } } } + open, err := workflowRuns(ctx, repo, head, &result) + if err != nil { + return result, err + } + // An open run may not have created its jobs yet. Successes already listed + // must not hide it. A failure already in hand can start a Repair. + if open && len(result.failed) == 0 { + pending = true + } switch { case pending: result.state = ciPending @@ -75,3 +84,66 @@ func readCI(ctx context.Context, repo, head string) (ciObservation, error) { } return result, nil } + +// Latest workflow run of each workflow on this SHA. Older runs are superseded. +// A completed failure is red even when none of its jobs appeared as check runs. +func workflowRuns(ctx context.Context, repo, head string, result *ciObservation) (bool, error) { + var pages []struct { + WorkflowRuns []struct { + ID int64 `json:"id"` + Name string `json:"name"` + Status string `json:"status"` + Conclusion *string `json:"conclusion"` + CreatedAt string `json:"created_at"` + WorkflowID int64 `json:"workflow_id"` + } `json:"workflow_runs"` + } + if err := ghJSON(ctx, &pages, "api", "repos/"+repo+"/actions/runs?head_sha="+head+"&per_page=100", "--paginate", "--slurp"); err != nil { + return false, err + } + type latestRun struct { + id int64 + name string + status string + conclusion *string + created string + } + latest := map[int64]latestRun{} + for _, page := range pages { + for _, run := range page.WorkflowRuns { + if run.WorkflowID == 0 || run.ID == 0 || run.CreatedAt == "" || run.Status == "" { + return false, fmt.Errorf("unreadable workflow run") + } + prev, ok := latest[run.WorkflowID] + if ok && (prev.created > run.CreatedAt || (prev.created == run.CreatedAt && prev.id >= run.ID)) { + continue + } + latest[run.WorkflowID] = latestRun{run.ID, run.Name, run.Status, run.Conclusion, run.CreatedAt} + } + } + open := false + for _, run := range latest { + switch run.status { + case "queued", "in_progress", "waiting", "requested", "pending": + open = true + case "completed": + if run.conclusion == nil { + return false, fmt.Errorf("workflow run %d completed without a conclusion", run.id) + } + switch *run.conclusion { + case "success", "neutral", "skipped": + case "failure", "timed_out", "cancelled", "action_required", "stale", "startup_failure": + name := run.name + if name == "" { + name = fmt.Sprintf("workflow %d", run.id) + } + result.failed = append(result.failed, name) + default: + return false, fmt.Errorf("unknown workflow conclusion %q", *run.conclusion) + } + default: + return false, fmt.Errorf("unknown workflow status %q", run.status) + } + } + return open, nil +} diff --git a/prototype/go/review_security_workflow_test.go b/prototype/go/review_security_workflow_test.go new file mode 100644 index 000000000..15783c56b --- /dev/null +++ b/prototype/go/review_security_workflow_test.go @@ -0,0 +1,33 @@ +package main_test + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// A fast successful check must not attest the head ready while a workflow run +// on that same commit is still open or has completed failed without a job. +func TestSecurityOpenWorkflowIsNotAttestedReady(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "open-workflow") + output, err := f.run("https://github.com/acme/widgets/issues/7") + data, readErr := os.ReadFile(filepath.Join(f.root, "pr.json")) + if readErr != nil { + return + } + var pr struct { + Draft bool `json:"isDraft"` + } + if jsonErr := json.Unmarshal(data, &pr); jsonErr != nil { + t.Fatal(jsonErr) + } + if pr.Draft { + return + } + if got := strings.TrimSpace(f.git(f.repo, "show", "origin/issue-7:widget.txt")); got == "broken" { + t.Fatalf("ready pull request attests a head whose workflow run was not green: %q\nrun err=%v\n%s", got, err, output) + } +} diff --git a/prototype/go/testdata/claude.py b/prototype/go/testdata/claude.py index c0d973517..4075df77c 100644 --- a/prototype/go/testdata/claude.py +++ b/prototype/go/testdata/claude.py @@ -51,7 +51,7 @@ def handler(signum, frame): if mode == "error-result": print(json.dumps({"type": "result", "is_error": True}), flush=True) sys.exit(0) -if mode in {"delivery", "ci-repair", "conflict-repair", "salvage", "ci-still-red", "pending-check", "pending-status", "no-ci", "checks-disappear", "pr-closes", "partial-register"}: +if mode in {"delivery", "ci-repair", "conflict-repair", "salvage", "ci-still-red", "pending-check", "pending-status", "no-ci", "checks-disappear", "pr-closes", "partial-register", "open-workflow"}: Path("widget.txt").write_text(("resolved\n" if mode == "conflict-repair" else "repaired\n") if repair else "broken\n") if mode == "ci-still-red": Path("widget.txt").write_text(f"attempt {len((root / 'sessions.jsonl').read_text().splitlines())}\n") diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index b3e6ef98f..c3d54fcc3 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -49,7 +49,25 @@ def pr_data(): {"name": "fast", "status": "completed", "conclusion": "success"}, {"name": "security", "status": "completed", "conclusion": conclusion}, ] + if mode == "open-workflow": + checks = [{"name": "fast", "status": "completed", "conclusion": "success"}] print(json.dumps([{"check_runs": checks}])) + elif "/actions/runs" in args[1]: + if "head_sha=" not in args[1]: + sys.exit("workflow runs query missing head_sha") + head = args[1].split("head_sha=")[1].split("&")[0] + widget = subprocess.run(["git", "--git-dir", str(root / "origin.git"), "show", head + ":widget.txt"], capture_output=True, text=True).stdout + reads = root / ("workflow-reads-" + head) + count = int(reads.read_text()) if reads.exists() else 0 + reads.write_text(str(count + 1)) + runs = [] + if state["mode"] == "open-workflow": + run = {"id": 1, "name": "CI", "status": "in_progress", "conclusion": None, "workflow_id": 10, "created_at": "2026-01-01T00:00:00Z"} + if count > 0: + run["status"] = "completed" + run["conclusion"] = "failure" if widget == "broken\n" else "success" + runs = [run] + print(json.dumps([{"workflow_runs": runs}])) elif "/statuses" in args[1]: head = args[1].split("/commits/")[1].split("/")[0] count = int((root / ("ci-reads-" + head)).read_text()) From c04fa3a8e34ddd55301310170c15f6d15e4a9cef Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 17:24:54 -0500 Subject: [PATCH 5/5] Ignore a fork pull request when publishing Delivery A fork pull request with the same head branch name was treated as this repository's pull request, so Delivery and salvage stopped before pushing. Skip cross-repository results and open or reuse the same-repository pull request. --- prototype/go/README.md | 3 ++- prototype/go/pull_request.go | 20 +++++++++++++++----- prototype/go/review_security_pr_test.go | 23 +++++++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) create mode 100644 prototype/go/review_security_pr_test.go diff --git a/prototype/go/README.md b/prototype/go/README.md index f57f0a3c5..0ba2589f1 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -61,7 +61,8 @@ The opening Harness commits locally; thirdshift pushes the Issue branch and opens a draft pull request targeting the selected Base branch. It captures the PR number and verifies its open state, repository, branches and published head before watching CI or changing readiness. A closed or merged PR stops -Delivery and salvage without reopening it. Existing PRs are verified and reused. +Delivery and salvage without reopening it. Existing same-repository PRs are +verified and reused. A fork pull request for the same branch name is ignored. An opening session with no commits ahead of the Base branch ends without Delivery. Delivery fetches and merges the Base branch, pushes, then watches check runs diff --git a/prototype/go/pull_request.go b/prototype/go/pull_request.go index a9a406337..68123738b 100644 --- a/prototype/go/pull_request.go +++ b/prototype/go/pull_request.go @@ -14,21 +14,31 @@ type pullRequest struct { // Capture a single PR number, then use that number for every later observation // and mutation. A closed PR is never replaced or reopened by Delivery. +// A fork pull request is not this repository's branch and is ignored. func (d *delivery) findPR(ctx context.Context) error { if d.prNumber != 0 { _, err := d.observePR(ctx, "") return err } - var prs []struct{ Number int } + var prs []struct { + Number int + IsCrossRepository bool + } if err := ghJSON(ctx, &prs, "pr", "list", "--repo", d.issue.repo, "--head", d.branch, - "--state", "all", "--json", "number"); err != nil { + "--state", "all", "--json", "number,isCrossRepository"); err != nil { return err } - if len(prs) > 1 { + var same []int + for _, pr := range prs { + if !pr.IsCrossRepository { + same = append(same, pr.Number) + } + } + if len(same) > 1 { return fmt.Errorf("multiple pull requests for %s", d.branch) } - if len(prs) == 1 { - d.prNumber = prs[0].Number + if len(same) == 1 { + d.prNumber = same[0] _, err := d.observePR(ctx, "") return err } diff --git a/prototype/go/review_security_pr_test.go b/prototype/go/review_security_pr_test.go new file mode 100644 index 000000000..5778d795e --- /dev/null +++ b/prototype/go/review_security_pr_test.go @@ -0,0 +1,23 @@ +package main_test + +import ( + "os" + "path/filepath" + "testing" +) + +// A fork pull request uses the same head branch name. It is not this +// repository's pull request, and it must not stop Delivery from publishing. +func TestSecurityCrossRepositoryPullRequestDoesNotBlockPublish(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "delivery") + f.git(f.repo, "push", "origin", "HEAD:refs/heads/issue-7") + pr := `{"number":8,"url":"https://github.com/acme/widgets/pull/8","state":"OPEN","isDraft":false,"headRefName":"issue-7","baseRefName":"main","mergeable":"MERGEABLE","isCrossRepository":true}` + if err := os.WriteFile(filepath.Join(f.root, "pr.json"), []byte(pr), 0600); err != nil { + t.Fatal(err) + } + output, err := f.run("https://github.com/acme/widgets/issues/7") + if err != nil { + t.Fatalf("cross-repository pull request blocked delivery: %v: %s", err, output) + } +}