From d7dfc96b78ba938aa20dd922ca9ad9d8a8d6d1f4 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 16:38:28 -0500 Subject: [PATCH 1/3] Implement Go Pickup reservation and bounded BFS verification (#1834) --- prototype/go/PICKUP-MODEL.md | 97 +++++++++ prototype/go/README.md | 42 +++- prototype/go/cli_test.go | 7 +- prototype/go/main.go | 9 +- prototype/go/pickup.go | 71 +++++++ prototype/go/pickup_model_test.go | 319 ++++++++++++++++++++++++++++++ prototype/go/pickup_test.go | 255 ++++++++++++++++++++++++ prototype/go/ready.go | 277 ++++++++++++++++++++++++++ prototype/go/reservation.go | 60 ++++++ prototype/go/run.go | 6 + prototype/go/testdata/claude.py | 11 ++ prototype/go/testdata/gh.py | 47 ++++- 12 files changed, 1192 insertions(+), 9 deletions(-) create mode 100644 prototype/go/PICKUP-MODEL.md create mode 100644 prototype/go/pickup.go create mode 100644 prototype/go/pickup_model_test.go create mode 100644 prototype/go/pickup_test.go create mode 100644 prototype/go/ready.go create mode 100644 prototype/go/reservation.go diff --git a/prototype/go/PICKUP-MODEL.md b/prototype/go/PICKUP-MODEL.md new file mode 100644 index 000000000..e8f246847 --- /dev/null +++ b/prototype/go/PICKUP-MODEL.md @@ -0,0 +1,97 @@ +# Bounded Pickup model + +`pickup_model_test.go` is an independent, deterministic BFS model of the Go +prototype's ADR-0017 protocol. It stores comparable states in a visited set and +FIFO queue, retaining predecessor/action links. The first counterexample is +minimal in action count; mutation tests pin its length and replay the witness. +This is bounded exploration, not a proof for arbitrary host counts or GitHub +consistency. No external model checker or Go dependency is required. + +## Bounds and assumptions + +- Three hosts, one initially open Ready issue, one attempt per host, at most + three Claim applications. Each selection records the Claim count as its Ready + generation. Later hosts may select after a failed winner or Day shift hand-back. +- Git ref creates and owner-leased deletes are atomic. Tokens are unique per + attempt; an existing reservation is never adopted or expired. The Day shift + observes the token and confirms the owner crashed before deleting it. +- Issue labels and complete, ordered history are linearizable snapshots. History + pagination/snapshot consistency, network failures, permissions and trust are + outside the model. CLI tests cover pagination, trust and remote errors. All + other Ready gates are fixed favorable, for a Ticket (Spec runs are unsupported). +- A successful label edit atomically adds Claim/removes Ready. Applying Claim + when already present adds no event. Distinct reads, writes, local dispatch, + completion and crashes remain separate actions. Constant reads such as label + creation, branch/PR absence, and setup details are projected out. +- Each host may crash at every live phase and never resumes that attempt. Its + reservation remains until confirmed-dead recovery. A failure before dispatch + still executes Claim ending; a lost response after a write has the state of + a crash or failed request after that write. A dispatched session may leave + origin work and keep its Claim, leave no work and restore entry labels, or + close its issue. Issue closure and ending's label read/write are separate + actions. Day shift hand-back can reopen a closed issue. +- Day shift can withdraw Ready. Hand-back removes Claim/adds Ready only after + dispatched work ended. Day shift does not delete a live reservation. + +## Requests and control flow + +| Model action | Go request or control | +| --- | --- | +| `ListReady` | `pickupRun`: paginated open Ready listing; nonessential data projected out | +| `SelectHistory` | `readReady`: complete events history, reject active Claim, capture generation | +| `CreateReservation` | `reserve`: push unique parentless commit with empty create lease | +| `RefreshIssue` | `pickupReserved` → `readReady`: current issue/labels, retained as Claim entry | +| `RefreshHistory` | `readReady`: history check; `pickupReserved`: selected-generation comparison | +| `PutClaimLabels` | `runTicket` → `makeClaim`: `gh issue edit`, Ready off / Claim on | +| `PrepareWorktree` | `runTicket`: local preparation after successful Claim | +| `Dispatch` | `runTicket` → `harnessSession`: start the opening Harness session | +| `FailBeforeDispatch` | Claim request/setup/session-start failure; no dispatched session | +| `RunEndsWithoutOriginWork` | session ends, origin branch and PR absent | +| `ReadEndingLabels` | `endClaim` → `readIssue`; branch/PR reads fixed absent | +| `RestoreEntryLabels` | `endClaim`: undo this attempt's Claim/Ready changes, or remove Claim if the ending read found the issue closed | +| `KeepClaimWithOriginWork` | `endClaim`: origin work exists, keep Claim | +| `IssueClosed` | session closes issue; subsequent ending reads state and removes Claim without restoring Ready | +| `ReleaseReservation` | `reservation.release`: token-leased delete on a completion context | +| `Crash` | process death; no deferred completion | +| `DayReadsReservation` | operator observes the owner token | +| `DayDeletesCrashedReservation` | owner-token lease after confirmed death | +| `DayWithdrawsReady`, `DayHandsBack` | Day shift label changes | + +The Go slice applies Claim **before Harness dispatch**, as requested by #1834. +The Rust model's documented pre-Claim dispatch counterexample concerns its +earlier dispatch marker. It is not silently removed from the Rust model and a +green Go suite does not establish the Rust literal property. Here, both +`one dispatch per Ready generation` and `one claimed dispatch per Ready generation` +hold at the documented Go dispatch marker. Both predicates remain explicit. +Reachability assertions cover three competing selections, failed-winner Ready +restoration, pre-dispatch failures, issue closure, crash recovery and hand-back. + +## Counterexamples and sensitivity + +`WithoutClaimCount` removes only the generation comparison after reservation. +Host B selects generation 0; host A claims, dispatches and ends. After Ready is +restored, B reserves and dispatches its stale generation-0 selection. The +shortest witness has 19 actions. + +`UnconditionalCreate` replaces the empty create lease with an unconditional +overwrite. Both hosts reserve and refresh before either Claim. Their idempotent +Claim edits then permit both sessions to dispatch generation 0. The shortest +witness has 16 actions. Owner-token release remains guarded in both mutations. + +Run safety and positive counterexample regressions: + +```sh +go test -run '^TestPickupModel' -count=1 -v +``` + +These optional negative assertions intentionally exit nonzero with the trace: + +```sh +PICKUP_MODEL_VARIANT=WithoutClaimCount go test -run '^TestPickupModelMutations/WithoutClaimCount$' -count=1 -v +PICKUP_MODEL_VARIANT=UnconditionalCreate go test -run '^TestPickupModelMutations/UnconditionalCreate$' -count=1 -v +``` + +When changing `pickup.go`, `ready.go`, `reservation.go`, `claim.go` or the +dispatch ordering in `run.go`, compare the requests and interleavings here +with the code before relying on the checker. Preserve counterexamples rather +than weakening either property to obtain a passing suite. diff --git a/prototype/go/README.md b/prototype/go/README.md index 6e9b862eb..9ff314c7f 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -1,8 +1,9 @@ # Go Ticket-run prototype -An isolated, Linux-only experiment for #1830 / #1832. The production Rust +An isolated, Linux-only experiment for #1830 / #1832 / #1834. 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. +Harness session, before Delivery, CI watch or Repair. Pickup can select and +reserve a Ready issue and start that same Ticket run. 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 +22,7 @@ Run it from the Launch directory of a disposable repository: ```sh /tmp/thirdshift-go https://github.com/owner/repo/issues/7 +/tmp/thirdshift-go pickup ``` It checks Origin match, an open issue, and the issue author's Trusted account @@ -51,6 +53,42 @@ 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. +## Pickup + +`pickup` lists every page of open `ready-for-agent` issues and tries the lowest +number first. It checks the author, the latest Ready label's actor and subsequent +title/body edits against the Trusted account rule. Complete label history rejects +an active Claim and requires a Ready hand-back after a previous Claim ended, +including either ordering of a same-second hand-back. It passes over triage +labels, Base/Security fixes, sub-issues, open blockers, and issues shaped within +30 minutes. Existing origin Issue branches or PRs in any state require a hand-back. + +Only Ticket runs are supported: issues with sub-issues (Specs) are passed over. +This experiment has no production Pass limits, Sweep, CI checks or notifications, +and inherits the first slice's refusal to resume an existing local branch or +Worktree. It uses the checked-out Base branch and takes no Pickup options. + +For each candidate, ADR-0017 creates `refs/heads/thirdshift/pickup/` on +origin with an empty create lease and a unique, parentless, empty-tree commit. +Its message records the token, host, Launch directory, PID and start time. +Pickup then rechecks the issue, trust, complete history and Claim-application +count. A busy reservation or stale selection moves on to the next candidate; +no available candidate exits successfully without starting a session. + +A successful Claim precedes Harness dispatch. The reservation lasts through +Claim ending, even on interruption, and release requires the owner's exact +commit as its lease. Remote errors fail closed; uncertain create or failed +release names the ref and token for manual recovery. Crashes retain reservations +until the Day shift confirms the owner stopped; reservations never expire. + +The independent in-test BFS model checks both dispatch properties with three +hosts and requires shortest counterexamples for `WithoutClaimCount` and +`UnconditionalCreate`. See [the request mapping and limits](PICKUP-MODEL.md). + +```sh +go test -run '^TestPickupModel' -count=1 -v +``` + ## Embedded Factory skills Go cannot embed files outside its module. `factory-skills.json` is a generated diff --git a/prototype/go/cli_test.go b/prototype/go/cli_test.go index a76eca899..8a7cca1e7 100644 --- a/prototype/go/cli_test.go +++ b/prototype/go/cli_test.go @@ -216,10 +216,13 @@ func waitFor(t *testing.T, description string, check func() bool) { t.Fatalf("timed out waiting for %s", description) } -func (f *fixture) start(mode string) (*exec.Cmd, <-chan error) { +func (f *fixture) start(mode string, args ...string) (*exec.Cmd, <-chan error) { f.t.Helper() f.writeState("open", "OWNER", mode) - cmd := exec.Command(binary, "https://github.com/acme/widgets/issues/7") + if len(args) == 0 { + args = []string{"https://github.com/acme/widgets/issues/7"} + } + cmd := exec.Command(binary, args...) cmd.Dir, cmd.Env = f.repo, f.env output, err := os.Create(filepath.Join(f.root, "output")) if err != nil { diff --git a/prototype/go/main.go b/prototype/go/main.go index 88be2ab7b..52fbc5163 100644 --- a/prototype/go/main.go +++ b/prototype/go/main.go @@ -14,10 +14,15 @@ 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 ") + fmt.Fprintln(os.Stderr, "usage: thirdshift | pickup") return 2 } - err := ticketRun(ctx, os.Args[1]) + var err error + if os.Args[1] == "pickup" { + err = pickupRun(ctx) + } else { + err = ticketRun(ctx, os.Args[1]) + } if err != nil { fmt.Fprintln(os.Stderr, err) } diff --git a/prototype/go/pickup.go b/prototype/go/pickup.go new file mode 100644 index 000000000..474631dde --- /dev/null +++ b/prototype/go/pickup.go @@ -0,0 +1,71 @@ +package main + +import ( + "context" + "errors" + "fmt" + "sort" + "strconv" +) + +func pickupRun(ctx context.Context) error { + root, err := command(ctx, "", "git", "rev-parse", "--show-toplevel") + if err != nil { + return err + } + launch := string(root) + origin, err := command(ctx, launch, "git", "config", "--get", "remote.origin.url") + if err != nil { + return err + } + repo := originRepo(string(origin)) + if repo == "" { + return fmt.Errorf("Pickup requires a GitHub origin") + } + var pages [][]struct{ Number int } + if err := ghJSON(ctx, &pages, "api", "repos/"+repo+"/issues?state=open&labels=ready-for-agent&per_page=100", "--paginate", "--slurp"); err != nil { + return err + } + var numbers []int + for _, page := range pages { + for _, item := range page { + if item.Number <= 0 { + return fmt.Errorf("invalid candidate issue number %d", item.Number) + } + numbers = append(numbers, item.Number) + } + } + sort.Ints(numbers) + for _, number := range numbers { + issue := issueURL{"https://github.com/" + repo + "/issues/" + strconv.Itoa(number), repo, strconv.Itoa(number)} + _, generation, ready, err := readReady(ctx, launch, issue) + if err != nil { + return err + } + if !ready { + continue + } + r, err := reserve(ctx, launch, issue) + if err != nil { + return err + } + if r == nil { + continue + } + dispatched, err := pickupReserved(ctx, launch, issue, generation, r) + if err != nil || dispatched { + return err + } + } + fmt.Println("Pickup skipped: no Ready issue available") + return nil +} + +func pickupReserved(ctx context.Context, launch string, issue issueURL, generation int, r *reservation) (dispatched bool, runErr error) { + defer func() { runErr = errors.Join(runErr, r.release()) }() + data, current, ready, err := readReady(ctx, launch, issue) + if err != nil || !ready || current != generation { + return false, err + } + return true, runTicket(ctx, launch, issue, data) +} diff --git a/prototype/go/pickup_model_test.go b/prototype/go/pickup_model_test.go new file mode 100644 index 000000000..6ec4af568 --- /dev/null +++ b/prototype/go/pickup_model_test.go @@ -0,0 +1,319 @@ +package main_test + +import ( + "fmt" + "os" + "slices" + "strings" + "testing" +) + +// Independent bounded model; request mapping and limits: PICKUP-MODEL.md. +// Every edge is one remote request or an explicit local control/death marker. +type pickupPhase uint8 + +const ( + modelList pickupPhase = iota + modelSelectHistory + modelReserve + modelRefreshIssue + modelRefreshHistory + modelClaimWrite + modelSetup + modelDispatch + modelRunning + modelEndRead + modelEndWrite + modelRelease + modelDone + modelCrashed +) + +type pickupProtocol uint8 + +const ( + guarded pickupProtocol = iota + WithoutClaimCount + UnconditionalCreate +) + +func (p pickupProtocol) String() string { + return [...]string{"ADR-0017", "WithoutClaimCount", "UnconditionalCreate"}[p] +} + +type modelLabels struct{ ready, claimed bool } +type pickupHost struct { + phase pickupPhase + generation uint8 + entry, ending modelLabels + dispatched, claimCompleted bool + endingClosed bool +} +type pickupState struct { + hosts [3]pickupHost + labels modelLabels + reservation, dayObserved int8 // -1 means absent; otherwise the unique attempt/host. + claimCount uint8 + closed bool +} +type pickupEdge struct { + state pickupState + action string +} + +func pickupSuccessors(s pickupState, protocol pickupProtocol) []pickupEdge { + var edges []pickupEdge + add := func(next pickupState, action string, h int) { + edges = append(edges, pickupEdge{next, fmt.Sprintf("%s(%d)", action, h)}) + } + for h, host := range s.hosts { + n := s + next := &n.hosts[h] + action := "" + switch host.phase { + case modelList: + action = "ListReady" + next.phase = modelDone + if !s.closed && s.labels.ready && !s.labels.claimed { + next.phase = modelSelectHistory + } + case modelSelectHistory: + action = "SelectHistory" + next.generation, next.phase = s.claimCount, modelReserve + if s.labels.claimed { + next.phase = modelDone + } + case modelReserve: + action, next.phase = "CreateReservation", modelDone + if s.reservation == -1 || protocol == UnconditionalCreate { + n.reservation, next.phase = int8(h), modelRefreshIssue + } + case modelRefreshIssue: + action = "RefreshIssue" + next.entry, next.phase = s.labels, modelRelease + if !s.closed && s.labels.ready && !s.labels.claimed { + next.phase = modelRefreshHistory + } + case modelRefreshHistory: + action, next.phase = "RefreshHistory", modelRelease + if !s.labels.claimed && (protocol == WithoutClaimCount || s.claimCount == host.generation) { + next.phase = modelClaimWrite + } + case modelClaimWrite: + action = "PutClaimLabels" + if !s.labels.claimed { + n.claimCount++ + } + n.labels = modelLabels{claimed: true} + next.claimCompleted, next.phase = true, modelSetup + case modelSetup: + action, next.phase = "PrepareWorktree", modelDispatch + case modelDispatch: + action = "Dispatch" + next.dispatched, next.phase = true, modelRunning + case modelRunning: + action, next.phase = "RunEndsWithoutOriginWork", modelEndRead + case modelEndRead: + action = "ReadEndingLabels" + next.ending, next.phase = s.labels, modelRelease + next.endingClosed = s.closed + if s.labels.claimed { + next.phase = modelEndWrite + } + case modelEndWrite: + action = "RestoreEntryLabels" + n.labels = modelLabels{ready: host.ending.ready || host.entry.ready, claimed: host.entry.claimed} + if host.endingClosed { + n.labels = modelLabels{ready: host.ending.ready} + } + next.phase = modelRelease + case modelRelease: + action = "ReleaseReservation" + if s.reservation == int8(h) { + n.reservation = -1 + } + next.phase = modelDone + case modelDone, modelCrashed: + continue + default: + panic("unhandled Pickup phase") + } + add(n, action, h) + n = s + n.hosts[h].phase = modelCrashed + add(n, "Crash", h) + if host.phase == modelClaimWrite || host.phase == modelSetup || host.phase == modelDispatch { + n = s + n.hosts[h].phase = modelEndRead + add(n, "FailBeforeDispatch", h) + } + if host.phase == modelRunning { + n = s + n.hosts[h].phase = modelRelease + add(n, "KeepClaimWithOriginWork", h) + n = s + n.closed, n.hosts[h].phase = true, modelEndRead + add(n, "IssueClosed", h) + } + } + if s.reservation != -1 && s.dayObserved != s.reservation { + n := s + n.dayObserved = s.reservation + edges = append(edges, pickupEdge{n, "DayReadsReservation"}) + } + if s.dayObserved != -1 && s.reservation == s.dayObserved && s.hosts[s.dayObserved].phase == modelCrashed { + n := s + n.reservation = -1 + edges = append(edges, pickupEdge{n, "DayDeletesCrashedReservation"}) + } + if s.labels.ready { + n := s + n.labels.ready = false + edges = append(edges, pickupEdge{n, "DayWithdrawsReady"}) + } else if allDispatchedWorkEnded(s) { + n := s + n.labels = modelLabels{ready: true} + n.closed = false + edges = append(edges, pickupEdge{n, "DayHandsBack"}) + } + return edges +} + +func allDispatchedWorkEnded(s pickupState) bool { + for _, host := range s.hosts { + if host.dispatched && host.phase != modelRelease && host.phase != modelDone && host.phase != modelCrashed { + return false + } + } + return true +} + +func uniquePickupGeneration(s pickupState, claimedOnly bool) bool { + for h, host := range s.hosts { + if !host.dispatched || (claimedOnly && !host.claimCompleted) { + continue + } + for _, other := range s.hosts[:h] { + if other.dispatched && (!claimedOnly || other.claimCompleted) && host.generation == other.generation { + return false + } + } + } + return true +} + +type pickupNode struct { + state pickupState + parent int + action string +} +type pickupExploration struct { + states int + dispatchTrace, claimedTrace []string + reached map[string]bool +} + +// FIFO discovery + visited-state deduplication makes the first violating +// trace shortest by action count. Parent links retain a replayable witness. +func explorePickup(protocol pickupProtocol) pickupExploration { + initial := pickupState{labels: modelLabels{ready: true}, reservation: -1, dayObserved: -1} + queue := []pickupNode{{state: initial, parent: -1}} + seen := map[pickupState]bool{initial: true} + result := pickupExploration{reached: map[string]bool{}} + trace := func(index int) []string { + var actions []string + for queue[index].parent != -1 { + actions = append(actions, queue[index].action) + index = queue[index].parent + } + slices.Reverse(actions) + return actions + } + for head := 0; head < len(queue); head++ { + s := queue[head].state + if !uniquePickupGeneration(s, false) && result.dispatchTrace == nil { + result.dispatchTrace = trace(head) + } + if !uniquePickupGeneration(s, true) && result.claimedTrace == nil { + result.claimedTrace = trace(head) + } + if result.dispatchTrace != nil && result.claimedTrace != nil { + break // Both minimal witnesses found for a broken variant. + } + if s.hosts[0].phase == modelReserve && s.hosts[1].phase == modelReserve && s.hosts[2].phase == modelReserve { + result.reached["three hosts select"] = true + } + for _, edge := range pickupSuccessors(s, protocol) { + for _, host := range edge.state.hosts { + if host.dispatched && host.claimCompleted && host.phase == modelRelease && edge.state.labels.ready && host.ending.claimed { + result.reached["failed winner restores Ready"] = true + } + } + if edge.action == "DayDeletesCrashedReservation" || edge.action == "DayHandsBack" || strings.HasPrefix(edge.action, "RestoreEntryLabels") || strings.HasPrefix(edge.action, "FailBeforeDispatch") || strings.HasPrefix(edge.action, "IssueClosed") { + result.reached[strings.SplitN(edge.action, "(", 2)[0]] = true + } + if !seen[edge.state] { + seen[edge.state] = true + queue = append(queue, pickupNode{edge.state, head, edge.action}) + } + } + } + result.states = len(seen) + return result +} + +func TestPickupModelSafety(t *testing.T) { + result := explorePickup(guarded) + t.Logf("ADR-0017: %d unique states", result.states) + if result.dispatchTrace != nil { + t.Fatalf("one dispatch per Ready generation: %v", result.dispatchTrace) + } + if result.claimedTrace != nil { + t.Fatalf("one claimed dispatch per Ready generation: %v", result.claimedTrace) + } + for _, name := range []string{"three hosts select", "DayDeletesCrashedReservation", "DayHandsBack", "failed winner restores Ready", "RestoreEntryLabels", "FailBeforeDispatch", "IssueClosed"} { + if !result.reached[name] { + t.Errorf("vacuous safety: did not reach %s", name) + } + } +} + +func TestPickupModelMutations(t *testing.T) { + for _, protocol := range []pickupProtocol{WithoutClaimCount, UnconditionalCreate} { + t.Run(protocol.String(), func(t *testing.T) { + result := explorePickup(protocol) + t.Logf("%s: %d states; shortest dispatch trace (%d): %v; shortest claimed trace (%d): %v", protocol, result.states, len(result.dispatchTrace), result.dispatchTrace, len(result.claimedTrace), result.claimedTrace) + if result.dispatchTrace == nil || result.claimedTrace == nil { + t.Fatal("mutation did not violate both safety properties") + } + wantLength := 19 + if protocol == UnconditionalCreate { + wantLength = 16 + } + if len(result.dispatchTrace) != wantLength || len(result.claimedTrace) != wantLength { + t.Fatalf("minimal trace length changed: want %d; replay and review the request mapping", wantLength) + } + s := pickupState{labels: modelLabels{ready: true}, reservation: -1, dayObserved: -1} + for index, action := range result.claimedTrace { + found := false + for _, edge := range pickupSuccessors(s, protocol) { + if edge.action == action { + s, found = edge.state, true + break + } + } + if !found || (index < wantLength-1 && !uniquePickupGeneration(s, true)) { + t.Fatalf("invalid minimal witness at %s", action) + } + } + if uniquePickupGeneration(s, true) { + t.Fatal("replayed witness did not violate claimed-dispatch safety") + } + // Optional executable negative assertions; ordinary CI requires the + // counterexamples and remains green while guarding sensitivity. + if os.Getenv("PICKUP_MODEL_VARIANT") == protocol.String() { + t.Fatalf("one claimed dispatch per Ready generation: %v", result.claimedTrace) + } + }) + } +} diff --git a/prototype/go/pickup_test.go b/prototype/go/pickup_test.go new file mode 100644 index 000000000..f45c1f0f5 --- /dev/null +++ b/prototype/go/pickup_test.go @@ -0,0 +1,255 @@ +package main_test + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" +) + +func (f *fixture) pickupConfig(config map[string]any) { + f.t.Helper() + data, err := json.Marshal(config) + if err != nil { + f.t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(f.root, "pickup.json"), data, 0600); err != nil { + f.t.Fatal(err) + } +} + +func labelEvent(event, label, actor, at string) map[string]any { + return map[string]any{"event": event, "label": map[string]string{"name": label}, + "actor": map[string]string{"login": actor}, "created_at": at} +} + +func readyEvents() [][]map[string]any { + return [][]map[string]any{{labelEvent("labeled", "ready-for-agent", "acme", "2020-01-01T00:00:00Z")}} +} + +func (f *fixture) assertNoSession() { + f.t.Helper() + if data, err := os.ReadFile(filepath.Join(f.root, "sessions.jsonl")); !os.IsNotExist(err) { + f.t.Fatalf("unexpected session: %s (%v)", data, err) + } +} + +func TestPickupContentionMovesToNextIssue(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "reservation") + f.pickupConfig(map[string]any{"pages": [][]map[string]int{{{"number": 8}, {"number": 7}}}}) + token := f.git(f.repo, "rev-parse", "HEAD") + f.git(f.repo, "push", "origin", token+":refs/heads/thirdshift/pickup/7") + output, err := f.run("pickup") + if err != nil || !strings.Contains(output, `"result":"done"`) { + t.Fatalf("Pickup run: %v: %s", err, output) + } + if got := f.git(filepath.Join(f.root, "repo-issue-8"), "branch", "--show-current"); got != "issue-8" { + t.Fatalf("selected %s, want issue-8", got) + } + if got := f.git(f.repo, "ls-remote", "origin", "refs/heads/thirdshift/pickup/7"); !strings.HasPrefix(got, token+"\t") { + t.Fatalf("changed competing reservation: %s", got) + } +} + +func TestPickupRechecksIssueAndCompleteHistory(t *testing.T) { + for _, change := range []string{"closed", "unready", "active-claim", "intervening-claim", "untrusted-label", "edited"} { + t.Run(change, func(t *testing.T) { + f := newFixture(t) + refresh := map[string]any{} + switch change { + case "closed": + refresh["data"] = map[string]any{"state": "closed"} + case "unready": + refresh["data"] = map[string]any{"labels": []map[string]string{{"name": "needs-info"}}} + case "active-claim", "intervening-claim": + events := readyEvents() + events = append(events, []map[string]any{labelEvent("labeled", "in-progress", "acme", "2020-01-02T00:00:00Z")}) + if change == "intervening-claim" { + events = append(events, []map[string]any{ + labelEvent("unlabeled", "in-progress", "acme", "2020-01-02T00:00:00Z"), + labelEvent("labeled", "ready-for-agent", "acme", "2020-01-02T00:00:00Z"), + }) + } + refresh["events"] = events + case "untrusted-label": + refresh["events"] = [][]map[string]any{{labelEvent("labeled", "ready-for-agent", "outsider", "2020-01-01T00:00:00Z")}} + case "edited": + refresh["edit_pages"] = [][]map[string]any{{}, {{"editedAt": "2020-01-02T00:00:00Z", "editor": map[string]string{"login": "outsider"}}}} + } + f.pickupConfig(map[string]any{"pages": [][]map[string]int{{{"number": 7}, {"number": 8}}}, + "issues": map[string]any{"7": map[string]any{"refresh": refresh}}}) + output, err := f.run("pickup") + if err != nil || !strings.Contains(output, `"result":"done"`) { + t.Fatalf("Pickup run: %v: %s", err, output) + } + if _, err := os.Stat(f.worktree); !os.IsNotExist(err) { + t.Fatalf("stale issue dispatched: %v", err) + } + if got := f.git(filepath.Join(f.root, "repo-issue-8"), "branch", "--show-current"); got != "issue-8" { + t.Fatalf("selected %s", got) + } + if got := f.git(f.repo, "ls-remote", "origin", "refs/heads/thirdshift/pickup/*"); got != "" { + t.Fatalf("stale reservation retained: %s", got) + } + }) + } +} + +func TestPickupRejectsUnreadyOrUntrustedCandidates(t *testing.T) { + for _, gate := range []string{"author", "latest-label", "no-label-history", "title-edit", "body-edit", "active-history", "released-without-handback", "unsettled", "parent", "blocker", "spec", "triage", "started", "pr"} { + t.Run(gate, func(t *testing.T) { + f := newFixture(t) + config := map[string]any{} + pickup := map[string]any{"issues": map[string]any{"7": config}} + switch gate { + case "author": + config["data"] = map[string]any{"author_association": "NONE"} + case "latest-label": + config["events"] = append(readyEvents(), []map[string]any{labelEvent("labeled", "ready-for-agent", "outsider", "2020-01-02T00:00:00Z")}) + case "no-label-history": + config["events"] = [][]map[string]any{} + case "title-edit": + config["events"] = append(readyEvents(), []map[string]any{{"event": "renamed", "created_at": "2020-01-02T00:00:00Z", "actor": map[string]string{"login": "outsider"}}}) + case "body-edit": + config["edit_pages"] = [][]map[string]any{{}, {{"editedAt": "2020-01-02T00:00:00Z", "editor": nil}}} + case "active-history", "released-without-handback": + events := append(readyEvents(), []map[string]any{labelEvent("labeled", "in-progress", "acme", "2020-01-02T00:00:00Z")}) + if gate == "released-without-handback" { + events = append(events, []map[string]any{labelEvent("unlabeled", "in-progress", "acme", "2020-01-03T00:00:00Z")}) + } + config["events"] = events + case "unsettled": + config["events"] = [][]map[string]any{{labelEvent("labeled", "ready-for-agent", "acme", time.Now().UTC().Format(time.RFC3339))}} + case "parent": + config["facts"] = map[string]any{"parent": map[string]string{"url": "https://github.com/acme/widgets/issues/1"}} + case "blocker": + config["facts"] = map[string]any{"blockedBy": map[string]any{"nodes": []map[string]string{{"state": "OPEN"}}}} + case "spec": + config["facts"] = map[string]any{"subIssues": map[string]int{"totalCount": 1}} + case "triage": + config["data"] = map[string]any{"labels": []map[string]string{{"name": "ready-for-agent"}, {"name": "needs-triage"}}} + case "started": + f.git(f.repo, "push", "origin", "HEAD:refs/heads/issue-7") + case "pr": + pickup["prs"] = map[string]any{"issue-7": []map[string]any{{"headRefName": "issue-7-branch-2", "isCrossRepository": false}}} + } + f.pickupConfig(pickup) + output, err := f.run("pickup") + if err != nil || !strings.Contains(output, "Pickup skipped") { + t.Fatalf("wanted skip: %v: %s", err, output) + } + f.assertNoSession() + f.assertLabels("enhancement", "ready-for-agent") + }) + } +} + +func TestPickupSameIdentityOtherCloneCannotDispatchReservedIssue(t *testing.T) { + f := newFixture(t) + _, done := f.start("stream", "pickup") + waitFor(t, "reserved Harness session", func() bool { return strings.Contains(f.output(), `"session_id"`) }) + other := filepath.Join(f.root, "other") + f.git(f.root, "clone", "-b", "main", filepath.Join(f.root, "origin.git"), other) + f.git(other, "remote", "set-url", "origin", "https://github.com/acme/widgets.git") + f.git(other, "config", "url."+filepath.Join(f.root, "origin.git")+".insteadOf", "https://github.com/acme/widgets.git") + // Simulate a stale GitHub read while the first host is still running. + f.writeState("open", "OWNER", "stream") + cmd := exec.Command(binary, "pickup") + cmd.Dir, cmd.Env = other, f.env + output, err := cmd.CombinedOutput() + if err != nil || !strings.Contains(string(output), "Pickup skipped") { + t.Fatalf("competing clone: %v: %s", err, output) + } + if err := os.WriteFile(filepath.Join(f.root, "finish"), nil, 0600); err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatalf("owner: %v: %s", err, f.output()) + } + sessions, err := os.ReadFile(filepath.Join(f.root, "sessions.jsonl")) + if err != nil || len(strings.Split(strings.TrimSpace(string(sessions)), "\n")) != 1 { + t.Fatalf("duplicate dispatch: %s, %v", sessions, err) + } +} + +func TestPickupReservationEndsAfterInterruption(t *testing.T) { + f := newFixture(t) + cmd, done := f.start("stream", "pickup") + waitFor(t, "reserved Harness session", func() bool { return strings.Contains(f.output(), `"session_id"`) }) + if err := cmd.Process.Signal(syscall.SIGINT); err != nil { + t.Fatal(err) + } + if err := <-done; err == nil { + t.Fatalf("interrupted command succeeded: %s", f.output()) + } + if got := f.git(f.repo, "ls-remote", "origin", "refs/heads/thirdshift/pickup/*"); got != "" { + t.Fatalf("interruption retained reservation: %s", got) + } + f.assertLabels("enhancement", "ready-for-agent") +} + +func TestPickupReleaseNeverDeletesChangedOwner(t *testing.T) { + f := newFixture(t) + _, done := f.start("stream", "pickup") + waitFor(t, "reserved Harness session", func() bool { return strings.Contains(f.output(), `"session_id"`) }) + replacement := f.git(f.repo, "rev-parse", "HEAD") + f.git(f.repo, "push", "--force", "origin", replacement+":refs/heads/thirdshift/pickup/7") + if err := os.WriteFile(filepath.Join(f.root, "finish"), nil, 0600); err != nil { + t.Fatal(err) + } + if err := <-done; err == nil || !strings.Contains(f.output(), "our token") { + t.Fatalf("wanted release recovery diagnostic: %v: %s", err, f.output()) + } + if got := f.git(f.repo, "ls-remote", "origin", "refs/heads/thirdshift/pickup/7"); !strings.HasPrefix(got, replacement+"\t") { + t.Fatalf("changed reservation deleted: %s", got) + } +} + +func TestPickupReservationPermissionFailureFailsClosed(t *testing.T) { + f := newFixture(t) + if err := os.WriteFile(filepath.Join(f.root, "origin.git/hooks/pre-receive"), []byte("#!/bin/sh\necho permission-denied >&2\nexit 1\n"), 0755); err != nil { + t.Fatal(err) + } + output, err := f.run("pickup") + if err == nil || !strings.Contains(output, "our token") || !strings.Contains(output, "refs/heads/thirdshift/pickup/7") { + t.Fatalf("wanted fail-closed recovery diagnostic: %v: %s", err, output) + } + f.assertNoSession() + f.assertLabels("enhancement", "ready-for-agent") +} + +func TestPickupSameSecondHandbackAndTrustedMember(t *testing.T) { + f := newFixture(t) + events := append(readyEvents(), []map[string]any{ + labelEvent("labeled", "in-progress", "acme", "2020-01-02T00:00:00Z"), + labelEvent("labeled", "ready-for-agent", "member", "2020-01-03T00:00:00Z"), + labelEvent("unlabeled", "in-progress", "acme", "2020-01-03T00:00:00Z"), + }) + f.pickupConfig(map[string]any{"issues": map[string]any{"7": map[string]any{"events": events}}, "members": map[string]int{"member": 204}}) + output, err := f.run("pickup") + if err != nil || !strings.Contains(output, `"result":"done"`) { + t.Fatalf("same-second hand-back: %v: %s", err, output) + } +} + +func TestPickupSelectsLowestReadyAcrossPages(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "reservation") + f.pickupConfig(map[string]any{"pages": [][]map[string]int{{{"number": 12}}, {{"number": 7}}}}) + output, err := f.run("pickup") + if err != nil || !strings.Contains(output, `"result":"done"`) { + t.Fatalf("Pickup run: %v: %s", err, output) + } + if got := f.git(f.worktree, "branch", "--show-current"); got != "issue-7" { + t.Fatalf("selected %s, want issue-7", got) + } + if got := f.git(f.repo, "ls-remote", "--heads", "origin", "refs/heads/thirdshift/pickup/*"); got != "" { + t.Fatalf("reservation not released: %s", got) + } + f.assertLabels("enhancement", "ready-for-agent") +} diff --git a/prototype/go/ready.go b/prototype/go/ready.go new file mode 100644 index 000000000..87c8e3041 --- /dev/null +++ b/prototype/go/ready.go @@ -0,0 +1,277 @@ +package main + +import ( + "bytes" + "context" + "fmt" + "os/exec" + "sort" + "strconv" + "strings" + "time" +) + +type historyEvent struct { + Event string `json:"event"` + At time.Time `json:"created_at"` + Actor *struct{ Login string } `json:"actor"` + Label struct{ Name string } `json:"label"` +} + +// The complete event history determines the selection's Ready generation. +// Fold Claim events before Ready at a shared timestamp, keeping Claim order. +func readReady(ctx context.Context, launch string, issue issueURL) (issueData, int, bool, error) { + data, err := readIssue(ctx, issue) + if err != nil { + return data, 0, false, err + } + if data.State != "open" || len(data.PullRequest) != 0 || !trusted(data.Association) || !data.hasLabel("ready-for-agent") { + return data, 0, false, nil + } + for _, label := range []string{"in-progress", "needs-triage", "needs-info", "ready-for-human", "wontfix", "base-fix", "security-fix"} { + if data.hasLabel(label) { + return data, 0, false, nil + } + } + var pages [][]historyEvent + if err := ghJSON(ctx, &pages, "api", "repos/"+issue.repo+"/issues/"+issue.number+"/events?per_page=100", "--paginate", "--slurp"); err != nil { + return data, 0, false, err + } + var events []historyEvent + for _, page := range pages { + events = append(events, page...) + } + for _, event := range events { + if event.At.IsZero() { + return data, 0, false, fmt.Errorf("issue history has no created_at") + } + } + sort.SliceStable(events, func(i, j int) bool { + if events[i].At.Equal(events[j].At) { + return events[i].Label.Name != "ready-for-agent" && events[j].Label.Name == "ready-for-agent" + } + return events[i].At.Before(events[j].At) + }) + var readyAt time.Time + var readyActor string + count, claimed, released, handedBack := 0, false, false, false + for _, event := range events { + applied := event.Event == "labeled" + if event.Event != "labeled" && event.Event != "unlabeled" { + continue + } + switch event.Label.Name { + case "in-progress": + if applied { + count++ + claimed, released, handedBack = true, false, false + } else if claimed { + claimed, released = false, true + } + case "ready-for-agent": + if applied { + readyAt, readyActor = event.At, "" + if event.Actor != nil { + readyActor = event.Actor.Login + } + handedBack = released && !claimed + } else { + handedBack = false + } + } + } + if claimed || readyAt.IsZero() || time.Since(readyAt) < 30*time.Minute { + return data, count, false, nil + } + ok, err := trustedActor(ctx, issue.repo, readyActor) + if err != nil || !ok { + return data, count, false, err + } + for _, event := range events { + if event.Event != "renamed" || !event.At.After(readyAt) { + continue + } + actor := "" + if event.Actor != nil { + actor = event.Actor.Login + } + ok, err := trustedActor(ctx, issue.repo, actor) + if err != nil || !ok { + return data, count, false, err + } + } + ok, err = readyFacts(ctx, issue, readyAt) + if err != nil || !ok { + return data, count, false, err + } + refs, err := command(ctx, launch, "git", "ls-remote", "--heads", "origin", "refs/heads/issue-"+issue.number, "refs/heads/issue-"+issue.number+"-branch-*") + if err != nil { + return data, count, false, err + } + firstBranch := "issue-" + issue.number + var prs []struct { + HeadRefName string + IsCrossRepository bool + } + if err := ghJSON(ctx, &prs, "pr", "list", "--repo", issue.repo, "--search", "head:"+firstBranch, "--state", "all", "--limit", "1000", "--json", "headRefName,isCrossRepository"); err != nil { + return data, count, false, err + } + started := len(refs) != 0 || len(prs) == 1000 + for _, pr := range prs { + suffix := strings.TrimPrefix(pr.HeadRefName, firstBranch+"-branch-") + number, _ := strconv.Atoi(suffix) + if !pr.IsCrossRepository && (pr.HeadRefName == firstBranch || (suffix != pr.HeadRefName && number >= 2)) { + started = true + } + } + return data, count, handedBack || (count == 0 && !started), nil +} + +func trustedActor(ctx context.Context, repo, login string) (bool, error) { + if login == "" { + return false, nil + } + owner := strings.SplitN(repo, "/", 2)[0] + if strings.EqualFold(owner, login) { + return true, nil + } + ok, err := membership(ctx, "repos/"+repo+"/collaborators/"+login) + if err != nil || ok { + return ok, err + } + var repository struct{ Owner struct{ Type string } } + if err := ghJSON(ctx, &repository, "api", "repos/"+repo); err != nil { + return false, err + } + if repository.Owner.Type == "Organization" { + return membership(ctx, "orgs/"+owner+"/members/"+login) + } + return false, nil +} + +func membership(ctx context.Context, path string) (bool, error) { + cmd := exec.Command("gh", "api", "--include", path) + var stdout, stderr bytes.Buffer + cmd.Stdout, cmd.Stderr = &stdout, &stderr + err := supervise(ctx, cmd) + fields := strings.Fields(strings.SplitN(stdout.String(), "\n", 2)[0]) + if len(fields) >= 2 { + if fields[1] == "204" && err == nil { + return true, nil + } + if fields[1] == "404" && ctx.Err() == nil { + return false, nil + } + } + return false, fmt.Errorf("could not check Trusted account membership for %s: %v: %s", path, err, stderr.String()) +} + +const readyFactsQuery = `query($owner: String!, $repo: String!, $number: Int!, $after: String) { + repository(owner: $owner, name: $repo) { + issue(number: $number) { + parent { url } + subIssues(first: 1) { totalCount } + blockedBy(first: 100) { nodes { state } pageInfo { hasNextPage } } + shaped: timelineItems(last: 1, itemTypes: [SUB_ISSUE_ADDED_EVENT, SUB_ISSUE_REMOVED_EVENT, BLOCKED_BY_ADDED_EVENT, BLOCKED_BY_REMOVED_EVENT]) { + nodes { + ... on SubIssueAddedEvent { createdAt } + ... on SubIssueRemovedEvent { createdAt } + ... on BlockedByAddedEvent { createdAt } + ... on BlockedByRemovedEvent { createdAt } + } + } + edits: userContentEdits(first: 100, after: $after) { + nodes { editedAt editor { login } } + pageInfo { hasNextPage endCursor } + } + } + } +}` + +// This vertical slice dispatches Ticket runs only. Specs are passed over, +// rather than executing a multi-Ticket Spec as if it were one Ticket. +func readyFacts(ctx context.Context, issue issueURL, readyAt time.Time) (bool, error) { + parts := strings.SplitN(issue.repo, "/", 2) + after := "" + for { + var response struct { + Errors []struct{ Message string } + Data struct { + Repository struct { + Issue *struct { + Parent *struct{ URL string } + SubIssues struct{ TotalCount int } + BlockedBy struct { + Nodes []struct{ State string } + PageInfo struct{ HasNextPage bool } + } + Shaped struct { + Nodes []struct{ CreatedAt time.Time } + } + Edits struct { + Nodes []struct { + EditedAt time.Time + Editor *struct{ Login string } + } + PageInfo struct { + HasNextPage bool + EndCursor string + } + } + } + } + } + } + args := []string{"api", "graphql", "-f", "query=" + readyFactsQuery, "-f", "owner=" + parts[0], "-f", "repo=" + parts[1], "-F", "number=" + issue.number} + if after != "" { + args = append(args, "-f", "after="+after) + } + if err := ghJSON(ctx, &response, args...); err != nil { + return false, err + } + facts := response.Data.Repository.Issue + if len(response.Errors) != 0 || facts == nil { + return false, fmt.Errorf("invalid Ready facts response: %v", response.Errors) + } + if facts.Parent != nil || facts.SubIssues.TotalCount != 0 || facts.BlockedBy.PageInfo.HasNextPage { + return false, nil + } + for _, blocker := range facts.BlockedBy.Nodes { + if blocker.State != "CLOSED" { + return false, nil + } + } + for _, shaped := range facts.Shaped.Nodes { + if shaped.CreatedAt.IsZero() { + return false, fmt.Errorf("shaping event has no createdAt") + } + if time.Since(shaped.CreatedAt) < 30*time.Minute { + return false, nil + } + } + for _, edit := range facts.Edits.Nodes { + if edit.EditedAt.IsZero() { + return false, fmt.Errorf("body edit has no editedAt") + } + if !edit.EditedAt.After(readyAt) { + continue + } + actor := "" + if edit.Editor != nil { + actor = edit.Editor.Login + } + ok, err := trustedActor(ctx, issue.repo, actor) + if err != nil || !ok { + return false, err + } + } + page := facts.Edits.PageInfo + if !page.HasNextPage { + return true, nil + } + if page.EndCursor == "" || page.EndCursor == after { + return false, fmt.Errorf("body edit history pagination did not advance") + } + after = page.EndCursor + } +} diff --git a/prototype/go/reservation.go b/prototype/go/reservation.go new file mode 100644 index 000000000..59b06c1d2 --- /dev/null +++ b/prototype/go/reservation.go @@ -0,0 +1,60 @@ +package main + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "os" + "strings" + "time" +) + +type reservation struct{ launch, ref, token string } + +func reserve(ctx context.Context, launch string, issue issueURL) (*reservation, error) { + ref := "refs/heads/thirdshift/pickup/" + issue.number + nonce := make([]byte, 32) + if _, err := rand.Read(nonce); err != nil { + return nil, err + } + tree, err := command(ctx, launch, "git", "mktree") + if err != nil { + return nil, err + } + host, err := os.Hostname() + if err != nil { + host = "unknown" + } + message := fmt.Sprintf("thirdshift Pickup reservation for %s\n\nToken: %s\nHost: %s\nLaunch: %s\nProcess: %d\nStarted: %s", + issue.url, hex.EncodeToString(nonce), host, launch, os.Getpid(), time.Now().UTC().Format(time.RFC3339Nano)) + token, err := command(ctx, launch, "git", "-c", "commit.gpgSign=false", "commit-tree", string(tree), "-m", message) + if err != nil { + return nil, err + } + r := &reservation{launch, ref, string(token)} + _, err = command(ctx, launch, "git", "push", "--no-verify", "--force-with-lease="+ref+":", "origin", r.token+":"+ref) + if err == nil { + return r, nil + } + // A lost response can mean our create succeeded. Only evidence of a + // different owner permits moving on; uncertainty retains the ref. + completion, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + remote, readErr := command(completion, launch, "git", "ls-remote", "--refs", "origin", ref) + fields := strings.Fields(string(remote)) + if readErr == nil && len(fields) == 2 && fields[1] == ref && fields[0] != r.token { + return nil, nil + } + return nil, fmt.Errorf("could not reserve %s; check origin by hand (our token %s; remote read: %v): %w", ref, r.token, readErr, err) +} + +func (r *reservation) release() error { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + _, err := command(ctx, r.launch, "git", "push", "--no-verify", "--force-with-lease="+r.ref+":"+r.token, "origin", ":"+r.ref) + if err != nil { + return fmt.Errorf("could not release %s; check origin by hand (our token %s): %w", r.ref, r.token, err) + } + return nil +} diff --git a/prototype/go/run.go b/prototype/go/run.go index e90bb5a48..84245b84f 100644 --- a/prototype/go/run.go +++ b/prototype/go/run.go @@ -67,6 +67,12 @@ func ticketRun(ctx context.Context, raw string) (runErr error) { if !trusted(data.Association) { return fmt.Errorf("issue author is not a Trusted account") } + return runTicket(ctx, launch, issue, data) +} + +// Pickup supplies the issue it re-verified while holding its reservation. +// A successful Claim precedes any dispatch to the Harness. +func runTicket(ctx context.Context, launch string, issue issueURL, data issueData) (runErr error) { base, err := command(ctx, launch, "git", "symbolic-ref", "--short", "HEAD") if err != nil { return err diff --git a/prototype/go/testdata/claude.py b/prototype/go/testdata/claude.py index cd02910af..2f59780ff 100644 --- a/prototype/go/testdata/claude.py +++ b/prototype/go/testdata/claude.py @@ -17,8 +17,19 @@ assert "Trusted comment" in issue and "UNTRUSTED_PAYLOAD" not in issue state = json.loads((root / "state.json").read_text()) assert {item["name"] for item in state["labels"]} == {"enhancement", "in-progress"} +with (root / "sessions.jsonl").open("a") as sessions: + sessions.write(json.dumps({"cwd": str(Path.cwd()), "prompt": sys.argv[-1]}) + "\n") print(json.dumps({"type": "system", "subtype": "init", "session_id": "fixture"}), flush=True) mode = state["mode"] +if mode == "reservation": + number = sys.argv[-1].split("/issues/")[1].split()[0] + ref = "refs/heads/thirdshift/pickup/" + number + token = subprocess.check_output(["git", "ls-remote", "origin", ref], text=True).split()[0] + subprocess.run(["git", "fetch", "origin", ref], check=True, capture_output=True) + assert subprocess.check_output(["git", "show", "-s", "--format=%P", token], text=True).strip() == "" + assert subprocess.check_output(["git", "ls-tree", "-r", token], text=True).strip() == "" + message = subprocess.check_output(["git", "show", "-s", "--format=%B", token], text=True) + assert all(field in message for field in ["Token:", "Host:", "Launch:", "Process:", "Started:"]) if mode in {"stubborn", "leader-exits"}: def handler(signum, frame): with (root / "signals").open("a") as events: diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index eb914e551..8fc7432ed 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -2,6 +2,7 @@ import json import os from pathlib import Path +import re import signal import sys import time @@ -11,12 +12,51 @@ with (root / "calls.jsonl").open("a") as calls: calls.write(json.dumps(args) + "\n") state = json.loads((root / "state.json").read_text()) +pickup = json.loads((root / "pickup.json").read_text()) if (root / "pickup.json").exists() else {} +match = re.search(r"/issues/(\d+)", args[1]) if args[:1] == ["api"] else None +number = match[1] if match else next((arg[7:] for arg in args if arg.startswith("number=")), "7") +reads_path = root / "reads.json" +reads = json.loads(reads_path.read_text()) if reads_path.exists() else {} +config = pickup.get("issues", {}).get(number, {}) +if match and "/events?" not in args[1] and "/comments" not in args[1]: + reads[number] = reads.get(number, 0) + 1 + reads_path.write_text(json.dumps(reads)) +if reads.get(number, 0) >= 2: + config = config | config.get("refresh", {}) if args[:1] == ["api"]: - if "/comments" in args[1]: + if args[1] == "--include": + code = pickup.get("members", {}).get(args[2].split("/")[-1], 404) + print(f"HTTP/2.0 {code} Status\n") + sys.exit(0 if code == 204 else 1) + elif args[1] == "repos/acme/widgets": + print(json.dumps({"owner": {"type": "Organization"}})) + elif args[1] == "graphql": + facts = { + "parent": None, "subIssues": {"totalCount": 0}, + "blockedBy": {"nodes": [], "pageInfo": {"hasNextPage": False}}, + "shaped": {"nodes": []}, + "edits": {"nodes": [], "pageInfo": {"hasNextPage": False}} + } | config.get("facts", {}) + if "edit_pages" in config: + cursor = next((arg[6:] for arg in args if arg.startswith("after=")), "0") + page = int(cursor) + more = page + 1 < len(config["edit_pages"]) + facts["edits"] = {"nodes": config["edit_pages"][page], + "pageInfo": {"hasNextPage": more, "endCursor": str(page + 1)}} + if config.get("bad_graphql"): + print(json.dumps({"errors": [{"message": "unavailable"}]})) + else: + print(json.dumps({"data": {"repository": {"issue": facts}}})) + elif "/issues?" in args[1]: + print(json.dumps(pickup.get("pages", [[{"number": 7}]]))) + elif "/events?" in args[1]: + print(json.dumps(config.get("events", [[{"event": "labeled", "created_at": "2020-01-01T00:00:00Z", + "actor": {"login": "acme"}, "label": {"name": "ready-for-agent"}}]]))) + elif "/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"}]])) else: - print(json.dumps(state)) + print(json.dumps(state | config.get("data", {}))) elif args[:2] == ["label", "create"]: pass elif args[:2] == ["issue", "edit"]: @@ -30,6 +70,7 @@ os.kill(os.getppid(), signal.SIGINT) time.sleep(60) elif args[:2] == ["pr", "list"]: - print("[]") + branch = args[args.index("--head") + 1] if "--head" in args else args[args.index("--search") + 1][5:] + print(json.dumps(pickup.get("prs", {}).get(branch, []))) else: sys.exit("unexpected gh invocation: " + repr(args)) From fef1253cb66042fc34663fe3b2747eab55b57165 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 16:46:02 -0500 Subject: [PATCH 2/3] Align Claim label requests with the Pickup model and share Launch discovery --- prototype/go/PICKUP-MODEL.md | 5 +- prototype/go/README.md | 4 +- prototype/go/claim.go | 47 ++++++++++----- prototype/go/cli_test.go | 4 -- prototype/go/pickup.go | 9 +-- prototype/go/process.go | 5 ++ prototype/go/run.go | 18 +++--- .../go/spec_atomic_label_regression_test.go | 59 +++++++++++++++++++ prototype/go/testdata/gh.py | 22 +++---- 9 files changed, 127 insertions(+), 46 deletions(-) create mode 100644 prototype/go/spec_atomic_label_regression_test.go diff --git a/prototype/go/PICKUP-MODEL.md b/prototype/go/PICKUP-MODEL.md index e8f246847..320591d22 100644 --- a/prototype/go/PICKUP-MODEL.md +++ b/prototype/go/PICKUP-MODEL.md @@ -42,7 +42,7 @@ consistency. No external model checker or Go dependency is required. | `CreateReservation` | `reserve`: push unique parentless commit with empty create lease | | `RefreshIssue` | `pickupReserved` → `readReady`: current issue/labels, retained as Claim entry | | `RefreshHistory` | `readReady`: history check; `pickupReserved`: selected-generation comparison | -| `PutClaimLabels` | `runTicket` → `makeClaim`: `gh issue edit`, Ready off / Claim on | +| `PutClaimLabels` | `runTicket` → `makeClaim` → `changeLabels`: single REST whole-label PUT, Ready off / Claim on | | `PrepareWorktree` | `runTicket`: local preparation after successful Claim | | `Dispatch` | `runTicket` → `harnessSession`: start the opening Harness session | | `FailBeforeDispatch` | Claim request/setup/session-start failure; no dispatched session | @@ -58,6 +58,9 @@ consistency. No external model checker or Go dependency is required. | `DayWithdrawsReady`, `DayHandsBack` | Day shift label changes | The Go slice applies Claim **before Harness dispatch**, as requested by #1834. +`TestPickupClaimRequestsMatchModelAtomicTransitions` keeps acquisition and ending +on single whole-label PUTs: `gh issue edit` splits add/remove into concurrent +mutations and does not implement the modeled swap. The Rust model's documented pre-Claim dispatch counterexample concerns its earlier dispatch marker. It is not silently removed from the Rust model and a green Go suite does not establish the Rust literal property. Here, both diff --git a/prototype/go/README.md b/prototype/go/README.md index 9ff314c7f..b7c6f11fe 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -32,7 +32,9 @@ Launch directory must have a checked-out Base branch. This first slice creates or directory fails safely rather than resuming or overwriting it. The Claim replaces `ready-for-agent` and clears `agent-failed`, preserving -other labels. The session gets `.thirdshift-issue.md` with the title, body, +other labels from its issue read. Acquisition and ending each use one whole-label +REST PUT to swap Ready/Claim atomically, matching the model's transition. +The session gets `.thirdshift-issue.md` with the title, body, and paginated comments, replacing each untrusted comment with a notice. Factory skills are linked under `.claude/skills/thirdshift-*`. Both those links and the issue file are excluded through the shared Git `info/exclude`. diff --git a/prototype/go/claim.go b/prototype/go/claim.go index 492a5da16..ea38c09c1 100644 --- a/prototype/go/claim.go +++ b/prototype/go/claim.go @@ -2,7 +2,9 @@ package main import ( "context" + "encoding/json" "fmt" + "slices" ) func makeClaim(ctx context.Context, issue issueURL, data issueData) error { @@ -10,13 +12,30 @@ func makeClaim(ctx context.Context, issue issueURL, data issueData) error { "--description", "A Claim: thirdshift has taken this issue", "--force"); err != nil { return err } - args := []string{"issue", "edit", issue.number, "--repo", issue.repo, "--add-label", "in-progress"} - for _, label := range []string{"ready-for-agent", "agent-failed"} { - if data.hasLabel(label) { - args = append(args, "--remove-label", label) + return changeLabels(ctx, issue, data, []string{"in-progress"}, []string{"ready-for-agent", "agent-failed"}) +} + +// One whole-label PUT matches the model's atomic Ready/Claim transition. +// gh issue edit starts independent add/remove mutations and cannot do that. +func changeLabels(ctx context.Context, issue issueURL, current issueData, add, remove []string) error { + labels := []string{} + for _, label := range current.Labels { + if !slices.Contains(remove, label.Name) { + labels = append(labels, label.Name) + } + } + for _, label := range add { + if !slices.Contains(labels, label) { + labels = append(labels, label) } } - _, err := command(ctx, "", "gh", args...) + payload, err := json.Marshal(struct { + Labels []string `json:"labels"` + }{labels}) + if err != nil { + return err + } + _, err = commandInput(ctx, "", payload, "gh", "api", "repos/"+issue.repo+"/issues/"+issue.number+"/labels", "--method", "PUT", "--input", "-") return err } @@ -30,8 +49,7 @@ func endClaim(ctx context.Context, launch string, issue issueURL, branch string, if !data.hasLabel("in-progress") { return nil } - args := []string{"issue", "edit", issue.number, "--repo", issue.repo} - unchanged := len(args) + var add, remove []string if data.State == "open" { refs, err := command(ctx, launch, "git", "ls-remote", "--heads", "origin", "refs/heads/"+branch) if err != nil { @@ -50,24 +68,23 @@ func endClaim(ctx context.Context, launch string, issue issueURL, branch string, 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") + add = append(add, "agent-failed") } else { if !initial.hasLabel("in-progress") { - args = append(args, "--remove-label", "in-progress") + remove = append(remove, "in-progress") } if initial.hasLabel("ready-for-agent") { - args = append(args, "--add-label", "ready-for-agent") + add = append(add, "ready-for-agent") } } } else { - args = append(args, "--remove-label", "in-progress") + remove = append(remove, "in-progress") if data.hasLabel("agent-failed") { - args = append(args, "--remove-label", "agent-failed") + remove = append(remove, "agent-failed") } } - if len(args) == unchanged { + if len(add) == 0 && len(remove) == 0 { return nil } - _, err = command(ctx, "", "gh", args...) - return err + return changeLabels(ctx, issue, data, add, remove) } diff --git a/prototype/go/cli_test.go b/prototype/go/cli_test.go index 8a7cca1e7..837c002d0 100644 --- a/prototype/go/cli_test.go +++ b/prototype/go/cli_test.go @@ -152,10 +152,6 @@ func TestTicketRunLinksEmbeddedSkillsAndStreamsSession(t *testing.T) { if err != nil || !strings.Contains(string(data), "Good and Bad Tests") { t.Fatalf("embedded supporting skill file: %v", err) } - calls, _ := os.ReadFile(filepath.Join(f.root, "calls.jsonl")) - if !strings.Contains(string(calls), `"--add-label", "in-progress"`) || !strings.Contains(string(calls), `"--remove-label", "ready-for-agent"`) { - t.Fatalf("missing Claim transition: %s", calls) - } f.assertLabels("enhancement", "ready-for-agent") } diff --git a/prototype/go/pickup.go b/prototype/go/pickup.go index 474631dde..cede9ab8f 100644 --- a/prototype/go/pickup.go +++ b/prototype/go/pickup.go @@ -9,16 +9,11 @@ import ( ) func pickupRun(ctx context.Context) error { - root, err := command(ctx, "", "git", "rev-parse", "--show-toplevel") + launch, origin, err := launchOrigin(ctx) if err != nil { return err } - launch := string(root) - origin, err := command(ctx, launch, "git", "config", "--get", "remote.origin.url") - if err != nil { - return err - } - repo := originRepo(string(origin)) + repo := originRepo(origin) if repo == "" { return fmt.Errorf("Pickup requires a GitHub origin") } diff --git a/prototype/go/process.go b/prototype/go/process.go index 0ada1d701..17d722a83 100644 --- a/prototype/go/process.go +++ b/prototype/go/process.go @@ -47,8 +47,13 @@ func supervise(ctx context.Context, cmd *exec.Cmd) error { } func command(ctx context.Context, dir, name string, args ...string) ([]byte, error) { + return commandInput(ctx, dir, nil, name, args...) +} + +func commandInput(ctx context.Context, dir string, input []byte, name string, args ...string) ([]byte, error) { cmd := exec.Command(name, args...) cmd.Dir = dir + cmd.Stdin = bytes.NewReader(input) var stdout, stderr bytes.Buffer cmd.Stdout, cmd.Stderr = &stdout, &stderr if err := supervise(ctx, cmd); err != nil { diff --git a/prototype/go/run.go b/prototype/go/run.go index 84245b84f..2b2043d85 100644 --- a/prototype/go/run.go +++ b/prototype/go/run.go @@ -40,21 +40,25 @@ func originRepo(raw string) string { return strings.TrimSuffix(strings.TrimPrefix(u.Path, "/"), ".git") } -func ticketRun(ctx context.Context, raw string) (runErr error) { - issue, err := parseIssue(raw) +func launchOrigin(ctx context.Context) (string, string, error) { + root, err := command(ctx, "", "git", "rev-parse", "--show-toplevel") if err != nil { - return err + return "", "", err } - root, err := command(ctx, "", "git", "rev-parse", "--show-toplevel") + origin, err := command(ctx, string(root), "git", "config", "--get", "remote.origin.url") + return string(root), string(origin), err +} + +func ticketRun(ctx context.Context, raw string) (runErr error) { + issue, err := parseIssue(raw) if err != nil { return err } - launch := string(root) - origin, err := command(ctx, launch, "git", "config", "--get", "remote.origin.url") + launch, origin, err := launchOrigin(ctx) if err != nil { return err } - if !strings.EqualFold(originRepo(string(origin)), issue.repo) { + if !strings.EqualFold(originRepo(origin), issue.repo) { return fmt.Errorf("Origin match failed: %s does not match %s", origin, issue.repo) } data, err := readIssue(ctx, issue) diff --git a/prototype/go/spec_atomic_label_regression_test.go b/prototype/go/spec_atomic_label_regression_test.go new file mode 100644 index 000000000..14ea5c328 --- /dev/null +++ b/prototype/go/spec_atomic_label_regression_test.go @@ -0,0 +1,59 @@ +package main_test + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// Each model Claim transition must correspond to one atomic remote replacement. +// gh issue edit --add-label/--remove-label starts independent mutations. +func TestPickupClaimRequestsMatchModelAtomicTransitions(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "reservation") + output, err := f.run("pickup") + if err != nil { + t.Fatalf("Pickup run: %v: %s", err, output) + } + f.assertLabels("enhancement", "ready-for-agent") + raw, err := os.ReadFile(filepath.Join(f.root, "calls.jsonl")) + if err != nil { + t.Fatal(err) + } + puts := 0 + for _, line := range strings.Split(strings.TrimSpace(string(raw)), "\n") { + var args []string + if err := json.Unmarshal([]byte(line), &args); err != nil { + t.Fatal(err) + } + if len(args) >= 2 && args[0] == "issue" && args[1] == "edit" { + t.Errorf("Claim transition uses gh issue edit, whose label additions/removals are separate concurrent GraphQL mutations: %v", args) + } + if len(args) < 2 || args[0] != "api" { + continue + } + endpoint, method := false, "" + for i, arg := range args { + if arg == "repos/acme/widgets/issues/7/labels" { + endpoint = true + } + if (arg == "--method" || arg == "-X") && i+1 < len(args) { + method = args[i+1] + } + if strings.HasPrefix(arg, "--method=") { + method = strings.TrimPrefix(arg, "--method=") + } + } + if endpoint { + if method != "PUT" { + t.Errorf("labels endpoint needs atomic full replacement via PUT, got %v", args) + } + puts++ + } + } + if puts != 2 { + t.Errorf("want one label PUT for Claim acquisition and one for ending, got %d", puts) + } +} diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index 8fc7432ed..7cbaf4e25 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -18,7 +18,7 @@ reads_path = root / "reads.json" reads = json.loads(reads_path.read_text()) if reads_path.exists() else {} config = pickup.get("issues", {}).get(number, {}) -if match and "/events?" not in args[1] and "/comments" not in args[1]: +if match and "/events?" not in args[1] and "/comments" not in args[1] and not args[1].endswith("/labels"): reads[number] = reads.get(number, 0) + 1 reads_path.write_text(json.dumps(reads)) if reads.get(number, 0) >= 2: @@ -47,6 +47,16 @@ print(json.dumps({"errors": [{"message": "unavailable"}]})) else: print(json.dumps({"data": {"repository": {"issue": facts}}})) + elif args[1].endswith("/labels"): + assert args[args.index("--method") + 1] == "PUT" + payload = json.load(sys.stdin) + assert isinstance(payload["labels"], list) + state["labels"] = [{"name": name} for name in sorted(set(payload["labels"]))] + (root / "state.json").write_text(json.dumps(state)) + if "in-progress" in payload["labels"] and state["mode"] == "interrupt-claim": + os.kill(os.getppid(), signal.SIGINT) + time.sleep(60) + print(json.dumps(state["labels"])) elif "/issues?" in args[1]: print(json.dumps(pickup.get("pages", [[{"number": 7}]]))) elif "/events?" in args[1]: @@ -59,16 +69,6 @@ print(json.dumps(state | config.get("data", {}))) elif args[:2] == ["label", "create"]: pass -elif args[:2] == ["issue", "edit"]: - labels = {item["name"] for item in state["labels"]} - for index, arg in enumerate(args): - if arg == "--add-label": labels.update(args[index + 1].split(",")) - if arg == "--remove-label": labels.difference_update(args[index + 1].split(",")) - state["labels"] = [{"name": name} for name in sorted(labels)] - (root / "state.json").write_text(json.dumps(state)) - if "--add-label" in args and args[args.index("--add-label") + 1] == "in-progress" and state["mode"] == "interrupt-claim": - os.kill(os.getppid(), signal.SIGINT) - time.sleep(60) elif args[:2] == ["pr", "list"]: branch = args[args.index("--head") + 1] if "--head" in args else args[args.index("--search") + 1][5:] print(json.dumps(pickup.get("prs", {}).get(branch, []))) From 1476c691341c1670d0b356894068ba4dbe4315f1 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 6 Nov 2026 17:23:28 -0500 Subject: [PATCH 3/3] Refuse a Claim that would erase a label applied after the readiness read Pickup built its whole-label PUT from the issue snapshot taken before history and trust checks. A gate label applied in that window was left out of the replacement, so the PUT deleted it and the Harness still started. Re-read at the PUT, skip the claim when that read shows the gate closed, and keep every other label from the new read. --- prototype/go/PICKUP-MODEL.md | 2 +- prototype/go/README.md | 10 +++++-- prototype/go/claim.go | 52 +++++++++++++++++++++++++++++++-- prototype/go/pickup.go | 6 +++- prototype/go/pickup_test.go | 26 +++++++++++++++++ prototype/go/testdata/claude.py | 3 +- prototype/go/testdata/gh.py | 8 +++++ 7 files changed, 99 insertions(+), 8 deletions(-) diff --git a/prototype/go/PICKUP-MODEL.md b/prototype/go/PICKUP-MODEL.md index 320591d22..1c33fcd19 100644 --- a/prototype/go/PICKUP-MODEL.md +++ b/prototype/go/PICKUP-MODEL.md @@ -42,7 +42,7 @@ consistency. No external model checker or Go dependency is required. | `CreateReservation` | `reserve`: push unique parentless commit with empty create lease | | `RefreshIssue` | `pickupReserved` → `readReady`: current issue/labels, retained as Claim entry | | `RefreshHistory` | `readReady`: history check; `pickupReserved`: selected-generation comparison | -| `PutClaimLabels` | `runTicket` → `makeClaim` → `changeLabels`: single REST whole-label PUT, Ready off / Claim on | +| `PutClaimLabels` | `runTicket` → `makeClaim` → `changeLabels`: re-read labels, then one REST whole-label PUT, Ready off / Claim on. A gate label applied after the refresh snapshot skips the PUT and the dispatch | | `PrepareWorktree` | `runTicket`: local preparation after successful Claim | | `Dispatch` | `runTicket` → `harnessSession`: start the opening Harness session | | `FailBeforeDispatch` | Claim request/setup/session-start failure; no dispatched session | diff --git a/prototype/go/README.md b/prototype/go/README.md index b7c6f11fe..7d90d6125 100644 --- a/prototype/go/README.md +++ b/prototype/go/README.md @@ -31,9 +31,13 @@ Launch directory must have a checked-out Base branch. This first slice creates `issue-7` and a sibling Worktree `-issue-7`; an existing branch or directory fails safely rather than resuming or overwriting it. -The Claim replaces `ready-for-agent` and clears `agent-failed`, preserving -other labels from its issue read. Acquisition and ending each use one whole-label -REST PUT to swap Ready/Claim atomically, matching the model's transition. +The Claim re-reads labels immediately before one whole-label REST PUT. +If that read shows the issue is no longer open, Ready was removed, or an +unready label was applied after the readiness snapshot, Pickup skips the +issue and a direct Ticket run stops, without writing labels. Otherwise the +PUT replaces `ready-for-agent`, clears `agent-failed`, and preserves every +other label from the re-read. Ending uses the same single PUT and re-reads +before it, so a label applied while branch and PR state is checked is kept. The session gets `.thirdshift-issue.md` with the title, body, and paginated comments, replacing each untrusted comment with a notice. Factory skills are linked under `.claude/skills/thirdshift-*`. Both those diff --git a/prototype/go/claim.go b/prototype/go/claim.go index ea38c09c1..1d3ba30b2 100644 --- a/prototype/go/claim.go +++ b/prototype/go/claim.go @@ -3,16 +3,58 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "slices" ) +// errStaleClaim means the issue read that authorized this Claim is no longer +// the issue's labels. Pickup passes over it. A direct Ticket run stops. +// The whole-label PUT must not erase that newer decision. +var errStaleClaim = errors.New("ready state changed after the readiness read") + +// Labels that stop a Claim when they appear after the readiness snapshot. +// agent-failed and in-progress are included so a later failure or Claim is +// not cleared by this attempt's replacement. +var claimGateLabels = []string{ + "in-progress", "agent-failed", "needs-triage", "needs-info", + "ready-for-human", "wontfix", "base-fix", "security-fix", +} + func makeClaim(ctx context.Context, issue issueURL, data issueData) error { if _, err := command(ctx, "", "gh", "label", "create", "in-progress", "--repo", issue.repo, "--description", "A Claim: thirdshift has taken this issue", "--force"); err != nil { return err } - return changeLabels(ctx, issue, data, []string{"in-progress"}, []string{"ready-for-agent", "agent-failed"}) + // The readiness snapshot is from before history, trust and reservation + // checks. Replace labels from a new read, and do not claim if that read + // shows the Day shift closed the gate. + fresh, err := readIssue(ctx, issue) + if err != nil { + return err + } + if err := rejectStaleClaim(data, fresh); err != nil { + return err + } + return changeLabels(ctx, issue, fresh, []string{"in-progress"}, []string{"ready-for-agent", "agent-failed"}) +} + +func rejectStaleClaim(before, fresh issueData) error { + if before.State == "open" && fresh.State != "open" { + return fmt.Errorf("%w: issue is no longer open", errStaleClaim) + } + if len(before.PullRequest) == 0 && len(fresh.PullRequest) != 0 { + return fmt.Errorf("%w: issue is now a pull request", errStaleClaim) + } + if before.hasLabel("ready-for-agent") && !fresh.hasLabel("ready-for-agent") { + return fmt.Errorf("%w: ready-for-agent was removed", errStaleClaim) + } + for _, label := range claimGateLabels { + if fresh.hasLabel(label) && !before.hasLabel(label) { + return fmt.Errorf("%w: %s was applied", errStaleClaim, label) + } + } + return nil } // One whole-label PUT matches the model's atomic Ready/Claim transition. @@ -86,5 +128,11 @@ func endClaim(ctx context.Context, launch string, issue issueURL, branch string, if len(add) == 0 && len(remove) == 0 { return nil } - return changeLabels(ctx, issue, data, add, remove) + // Branch and PR reads sit between the ending snapshot and this PUT. + // Replace from a current read so a label applied in that window is kept. + fresh, err := readIssue(ctx, issue) + if err != nil { + return err + } + return changeLabels(ctx, issue, fresh, add, remove) } diff --git a/prototype/go/pickup.go b/prototype/go/pickup.go index cede9ab8f..c8bea6f8d 100644 --- a/prototype/go/pickup.go +++ b/prototype/go/pickup.go @@ -62,5 +62,9 @@ func pickupReserved(ctx context.Context, launch string, issue issueURL, generati if err != nil || !ready || current != generation { return false, err } - return true, runTicket(ctx, launch, issue, data) + err = runTicket(ctx, launch, issue, data) + if errors.Is(err, errStaleClaim) { + return false, nil + } + return true, err } diff --git a/prototype/go/pickup_test.go b/prototype/go/pickup_test.go index f45c1f0f5..cc94bffe2 100644 --- a/prototype/go/pickup_test.go +++ b/prototype/go/pickup_test.go @@ -237,6 +237,32 @@ func TestPickupSameSecondHandbackAndTrustedMember(t *testing.T) { } } +func TestPickupRefusesClaimWhenGateLabelAppearsAfterRead(t *testing.T) { + for _, label := range []string{"wontfix", "priority"} { + t.Run(label, func(t *testing.T) { + f := newFixture(t) + f.writeState("open", "OWNER", "reservation") + f.pickupConfig(map[string]any{"poison_label": label}) + output, err := f.run("pickup") + if label == "priority" { + if err != nil || !strings.Contains(output, `"result":"done"`) { + t.Fatalf("benign label should still dispatch: %v: %s", err, output) + } + f.assertLabels("enhancement", "priority", "ready-for-agent") + return + } + if err != nil || !strings.Contains(output, "Pickup skipped") { + t.Fatalf("wanted skip, got %v: %s", err, output) + } + f.assertNoSession() + f.assertLabels("enhancement", "ready-for-agent", "wontfix") + if got := f.git(f.repo, "ls-remote", "origin", "refs/heads/thirdshift/pickup/*"); got != "" { + t.Fatalf("stale reservation retained: %s", got) + } + }) + } +} + func TestPickupSelectsLowestReadyAcrossPages(t *testing.T) { f := newFixture(t) f.writeState("open", "OWNER", "reservation") diff --git a/prototype/go/testdata/claude.py b/prototype/go/testdata/claude.py index 2f59780ff..717dbe3c7 100644 --- a/prototype/go/testdata/claude.py +++ b/prototype/go/testdata/claude.py @@ -16,7 +16,8 @@ issue = Path(".thirdshift-issue.md").read_text() assert "Trusted comment" in issue and "UNTRUSTED_PAYLOAD" not in issue state = json.loads((root / "state.json").read_text()) -assert {item["name"] for item in state["labels"]} == {"enhancement", "in-progress"} +names = {item["name"] for item in state["labels"]} +assert "in-progress" in names and "ready-for-agent" not in names, names with (root / "sessions.jsonl").open("a") as sessions: sessions.write(json.dumps({"cwd": str(Path.cwd()), "prompt": sys.argv[-1]}) + "\n") print(json.dumps({"type": "system", "subtype": "init", "session_id": "fixture"}), flush=True) diff --git a/prototype/go/testdata/gh.py b/prototype/go/testdata/gh.py index 7cbaf4e25..3b84eebcc 100644 --- a/prototype/go/testdata/gh.py +++ b/prototype/go/testdata/gh.py @@ -60,6 +60,14 @@ elif "/issues?" in args[1]: print(json.dumps(pickup.get("pages", [[{"number": 7}]]))) elif "/events?" in args[1]: + # A label applied after the issue snapshot and before Claim. Production + # gh is not involved; this only sequences the fixture's own state. + label = pickup.get("poison_label") + if reads.get(number, 0) >= 2 and isinstance(label, str) and label: + names = [item.get("name") for item in state.get("labels", [])] + if label not in names: + state.setdefault("labels", []).append({"name": label}) + (root / "state.json").write_text(json.dumps(state)) print(json.dumps(config.get("events", [[{"event": "labeled", "created_at": "2020-01-01T00:00:00Z", "actor": {"login": "acme"}, "label": {"name": "ready-for-agent"}}]]))) elif "/comments" in args[1]: