diff --git a/AUDIT.md b/AUDIT.md index d4758a7..8f16613 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -558,13 +558,70 @@ handler test coverage of any kind (everything is inline in `main()`), so this follows the existing gap rather than introducing an isolated test harness for one handler. +**Fixed 2026-07-27 (later same day):** `CLIAdapter.Lease`'s bootstrap prompt now +passes `time.Minute` instead of `wait=0`, matching `Bootstrap`'s inline-wait +pattern — closes the race B5 named (send-into-a-half-rendered-prompt). +`internal/herdr/adapter.go`. `go build`/`vet`/`test` all still pass; no +existing test asserted the old `wait=0` value. + **Still open from B5** (not attempted this pass — larger, needs design, not just a method-name swap): -- `agent.prompt` inline `wait` on `CLIAdapter.Lease` (still `wait=0`, per B5's - note that only `Bootstrap` passes a real wait). - `Release`'s real implementation, which depends on Phase 4 (§6) handoff production existing at all. - The stuck live task (`06FT6CKD9Y98AZRX6X8K3QXFZG`) was deliberately **not** manipulated directly (no `pane.close`/`pane.release_agent` call against it) — killing or releasing a real running agent from an audit session without the user present is exactly the kind of action that warrants asking first. + +## B6 — partial fix, 2026-07-27 (Phase 4 items 1 and 4) + +Two of Phase 4's six items landed; the rest are unchanged (still open, listed +below). + +1. **TASK.md is now actually written.** `continuity.RenderTaskFile(t)` + produces the §6.2 immutable spec content from `domain.Task`; + `GitWorktrees.Create` (`internal/orchestrator/orchestrator.go`) writes it + into every freshly created worktree and commits it immediately — it must + be committed, not left dirty, both so `ScratchCommit`'s "TASK.md is + immutable" check (which inspects `git status`) sees it as clean, and so + its hash is stable across whatever the agent does afterward. A worktree + that already has a `TASK.md` (recreation on restart) is left untouched. + New `continuity.TaskFileHash(root)` reads it back and hashes it — this is + what makes `w.TaskFileSHA`/`VerifyTaskFile`, previously dead because + nothing ever set `TaskFileSHA`, actually reachable. +2. **Pickup validation now gates bootstrap.** `Coordinator.Start` + (`internal/orchestrator/orchestrator.go`) computes the new worktree's + `TaskFileHash`, and — whenever the lease carries a `handoff_ref` (i.e. + this is a rotation continuation, not a fresh lease) — loads the handoff + from CAS and runs `continuity.ValidatePickup(worktree, handoff, + taskFileSHA)` **before** calling `Adapter.Bootstrap`. A validation failure + kills the just-started session and emits `TaskBlocked` instead of hand­ing + the successor an unverified anchor. This is exactly the gap B6 named: + `ValidatePickup` had no caller outside its own tests. + `TestStartBlocksOnInvalidPickup` (`rotation_test.go`) drives this against + a handoff whose anchor SHA doesn't exist in the repo and asserts the task + ends `Blocked`, never bootstrapped. + `TestGitWorktreesCommitsTaskFile` (`worktrees_test.go`) asserts the + written/committed content matches `RenderTaskFile` and survives + recreation. + +Caveat recorded in code: TASK.md hashing is best-effort — if a worktree came +from the `WorktreeCreator` (herdr-hosted, potentially remote) path rather +than `GitWorktrees`, `TaskFileHash` fails silently and pickup validation runs +with an empty `taskFileSHA` (so it still checks anchor SHA and dirty-file +hashes, just not TASK.md). No current adapter or test exercises +`WorktreeCreator` with a real `handoff_ref`, so this is unverified, not +proven safe — same cross-host caveat as the federation-fork section. + +**Still open from B6/Phase 4** (unchanged, larger and needs live-agent +cooperation): +- Item 2: handoff *production* — the rotating agent writing the §6.1 + handoff and a stop-hook path uploading it via `POST /v1/artifacts` before + `Coordinator.rotate` calls `Adapter.Release`. `Release` still just refuses + (see B5 above) — there is nothing yet to validate-and-mint a ref from. +- Item 3: `ScratchCommit` before release — not wired into `rotate` at all. +- Item 5: `CLIAdapter.Bootstrap`'s prompt is still ad hoc prose, not the + §6.2 ~200-token procedure (read handoff → validate-handoff → re-read + TASK.md → proceed). +- Item 6: `MarkdownChanges` (§6.3 adjacent-task notice) still uncalled from + anything but its own test. diff --git a/internal/continuity/continuity.go b/internal/continuity/continuity.go index 3be246b..71134de 100644 --- a/internal/continuity/continuity.go +++ b/internal/continuity/continuity.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "fmt" + "orchestra/internal/domain" "os" "os/exec" "path/filepath" @@ -14,6 +15,41 @@ import ( "time" ) +// RenderTaskFile produces the immutable §6.2 TASK.md content for a task. +// Deterministic in every field that comes from the task itself, so the same +// task always hashes to the same content. +func RenderTaskFile(t domain.Task) []byte { + var b strings.Builder + fmt.Fprintf(&b, "# Task %s\n\n", t.ID) + if t.Title != "" { + fmt.Fprintf(&b, "%s\n\n", t.Title) + } + fmt.Fprintf(&b, "- Project: %s\n", t.Project) + fmt.Fprintf(&b, "- Source: %s/%s\n", t.Source, t.ExternalID) + fmt.Fprintf(&b, "- Priority: %d\n", t.InherentPriority) + if len(t.Capability) > 0 { + fmt.Fprintf(&b, "- Capability: %s\n", strings.Join(t.Capability, ", ")) + } + if t.Due != nil { + fmt.Fprintf(&b, "- Due: %s\n", t.Due.UTC().Format(time.RFC3339)) + } + if t.Parent != "" { + fmt.Fprintf(&b, "- Parent: %s\n", t.Parent) + } + b.WriteString("\nThis file is immutable for the lifetime of the task (§6.2) — its hash is\ncarried in every handoff and re-verified on every pickup. Do not edit it.\n") + return []byte(b.String()) +} + +// TaskFileHash returns the sha256 of the TASK.md at the root of a worktree. +func TaskFileHash(root string) (string, error) { + b, err := os.ReadFile(filepath.Join(root, "TASK.md")) + if err != nil { + return "", err + } + sum := sha256.Sum256(b) + return hex.EncodeToString(sum[:]), nil +} + type Dirty struct { Path string `json:"path"` SHA256 string `json:"sha256"` diff --git a/internal/herdr/herdr.go b/internal/herdr/herdr.go index f90a398..c29970a 100644 --- a/internal/herdr/herdr.go +++ b/internal/herdr/herdr.go @@ -149,6 +149,10 @@ type Session struct { // CLIAdapter.Occupancy), since the file may not exist yet immediately // after lease. SessionFile string `json:"session_file,omitempty"` + // TaskFileSHA is the sha256 of the worktree's TASK.md at the time this + // session's lease was created — the immutable-spec hash continuity's + // pickup validation compares against on the next rotation (§6.2). + TaskFileSHA string `json:"task_file_sha,omitempty"` } func (c *Client) Prompt(ctx context.Context, pane, text string, wait time.Duration) error { diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index aab624a..1b7b677 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -67,6 +67,9 @@ func (w GitWorktrees) Create(ctx context.Context, t domain.Task) (string, error) if out, err := cmd.CombinedOutput(); err != nil { return "", fmt.Errorf("%s: %w", string(out), err) } + if err := writeTaskFile(ctx, p, t); err != nil { + return "", err + } if w.TaskFileSHA != "" { if err := continuity.VerifyTaskFile(p, w.TaskFileSHA); err != nil { return "", err @@ -75,6 +78,27 @@ func (w GitWorktrees) Create(ctx context.Context, t domain.Task) (string, error) return p, nil } +// writeTaskFile commits the §6.2 immutable TASK.md into a freshly created +// worktree. It must be committed, not left dirty, so ScratchCommit's +// "TASK.md is immutable" check (which inspects `git status`) sees it as +// clean, and so its hash survives independent of any later scratch commits. +func writeTaskFile(ctx context.Context, worktree string, t domain.Task) error { + path := filepath.Join(worktree, "TASK.md") + if _, err := os.Stat(path); err == nil { + return nil + } + if err := os.WriteFile(path, continuity.RenderTaskFile(t), 0644); err != nil { + return err + } + for _, args := range [][]string{{"add", "TASK.md"}, {"commit", "-m", "orchestra: TASK.md"}} { + cmd := exec.CommandContext(ctx, "git", append([]string{"-C", worktree}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + } + return nil +} + func (w GitWorktrees) Remove(ctx context.Context, _ domain.Task, path string) error { if path == "" { return fmt.Errorf("worktree: path required") @@ -541,17 +565,37 @@ func (c *Coordinator) Start(ctx context.Context, e domain.Event) error { if err != nil { return c.block(t, "worktree: "+err.Error()) } + // Best-effort: TASK.md only exists for worktrees this process can read + // locally (the GitWorktrees path). A herdr-hosted worktree on a remote + // machine (WorktreeCreator path) is the same cross-host gap named in + // AUDIT.md's federation-fork section — not solved here. + taskFileSHA, _ := continuity.TaskFileHash(w) s, err := a.Lease(ctx, t.ID, w) if err != nil { return c.block(t, "lease: "+err.Error()) } if p.HandoffRef != "" { + // §6.2 pickup validation: never bootstrap a successor onto a handoff + // whose anchor/dirty-file/TASK.md hashes don't match what's actually + // in the worktree. A failure here blocks the task rather than + // silently trusting an unvalidated ref (this is the gap AUDIT.md's + // B6 named as unreached from the live path). + h, err := continuity.Load(p.HandoffRef, c.Store) + if err != nil { + _ = a.Kill(ctx, s) + return c.block(t, "handoff: "+err.Error()) + } + if err := continuity.ValidatePickup(w, h, taskFileSHA); err != nil { + _ = a.Kill(ctx, s) + return c.block(t, "pickup: "+err.Error()) + } if err = a.Bootstrap(ctx, s, p.HandoffRef); err != nil { _ = a.Kill(ctx, s) return c.block(t, "bootstrap: "+err.Error()) } } s.HerdrID = p.HarnessID + s.TaskFileSHA = taskFileSHA c.mu.Lock() if c.sessions == nil { c.sessions = map[string]herdr.Session{} diff --git a/internal/orchestrator/rotation_test.go b/internal/orchestrator/rotation_test.go index a2b8737..d6430d7 100644 --- a/internal/orchestrator/rotation_test.go +++ b/internal/orchestrator/rotation_test.go @@ -136,6 +136,67 @@ func TestRotationEmitsValidReleaseWithAnchorSHA(t *testing.T) { func mustJSON(v any) []byte { b, _ := json.Marshal(v); return b } +// TestStartBlocksOnInvalidPickup guards AUDIT.md's B6/Phase 4 item 4: +// Coordinator.Start must run §6.2 pickup validation against the real +// worktree before bootstrapping a successor onto a handoff_ref, and refuse +// (TaskBlocked) rather than bootstrap on a mismatched anchor. +func TestStartBlocksOnInvalidPickup(t *testing.T) { + repo := t.TempDir() + run(t, repo, "init") + run(t, repo, "config", "user.email", "t@t") + run(t, repo, "config", "user.name", "t") + run(t, repo, "commit", "--allow-empty", "-m", "init") + + s, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if err := s.Append(domain.Event{ID: domain.NewID(), Type: "TaskCreated", TaskID: "t1", Surface: string(authz.System), Payload: mustJSON(map[string]any{ + "source": "jsonl", "external_id": "1", "project": "p", + })}); err != nil { + t.Fatal(err) + } + task := s.Tasks()[0] + + // A handoff whose anchor doesn't match anything in this fresh repo. + badHandoff := map[string]any{ + "meta": map[string]any{"id": "h1", "reason": "manual", "rotation_index": 0}, + "anchor": map[string]any{"git_sha": strings0(40, 'a'), "branch": "orchestra/t1"}, + "goal": "g", "done_when": []string{"x"}, "action": "prompt", "command": "go test ./...", + } + ref, err := s.PutArtifact(mustJSON(badHandoff)) + if err != nil { + t.Fatal(err) + } + + a := &fakeAdapter{occupancy: 0} + wt := orchestrator.GitWorktrees{Root: t.TempDir(), Repo: repo} + c := &orchestrator.Coordinator{Store: s, Worktrees: wt, Adapters: adapters{a}, StatePath: t.TempDir() + "/sessions.json"} + + leaseEvt, err := s.Lease(task.ID, "h1", time.Minute) + if err != nil { + t.Fatal(err) + } + b, _ := json.Marshal(map[string]string{"harness_id": "h1", "handoff_ref": ref}) + leaseEvt.Payload = b + if err := c.Start(context.Background(), leaseEvt); err != nil { + t.Fatal(err) + } + + got, ok := s.Task(task.ID) + if !ok || got.State != domain.StateBlocked { + t.Fatalf("expected TaskBlocked on invalid pickup, got state=%v ok=%v", got.State, ok) + } +} + +func strings0(n int, c byte) string { + b := make([]byte, n) + for i := range b { + b[i] = c + } + return string(b) +} + // keyedHarnessAdapter reports Session.Harness as the harness kind ("claude"), // distinct from the herdr instance id ("homesrv-claude") under which it is // registered in AdapterFactory.Herdrs — reproducing production's real key diff --git a/internal/orchestrator/worktrees_test.go b/internal/orchestrator/worktrees_test.go index 50fb771..82e333f 100644 --- a/internal/orchestrator/worktrees_test.go +++ b/internal/orchestrator/worktrees_test.go @@ -2,6 +2,7 @@ package orchestrator_test import ( "context" + "orchestra/internal/continuity" "orchestra/internal/domain" "orchestra/internal/orchestrator" "os" @@ -61,3 +62,62 @@ func TestPerProjectGitWorktreesResolvesByProject(t *testing.T) { t.Fatalf("expected unconfigured project to use default worktree root, got %s", pathDefault) } } + +// TestGitWorktreesCommitsTaskFile guards AUDIT.md's B6: nothing wrote a +// TASK.md into a worktree in the first place, so §6.2 pickup validation had +// nothing to check. GitWorktrees.Create must now write and commit an +// immutable TASK.md whose on-disk hash matches continuity.RenderTaskFile. +func TestGitWorktreesCommitsTaskFile(t *testing.T) { + base := t.TempDir() + repo := filepath.Join(base, "repo") + initRepo(t, repo) + + w := orchestrator.GitWorktrees{Root: filepath.Join(base, "wt"), Repo: repo} + task := domain.Task{ID: "t1", Project: "p", Source: "jsonl", ExternalID: "1", Title: "do the thing"} + + path, err := w.Create(context.Background(), task) + if err != nil { + t.Fatalf("create: %v", err) + } + + want := continuity.RenderTaskFile(task) + got, err := os.ReadFile(filepath.Join(path, "TASK.md")) + if err != nil { + t.Fatalf("read TASK.md: %v", err) + } + if string(got) != string(want) { + t.Fatalf("TASK.md content mismatch:\ngot: %s\nwant: %s", got, want) + } + + status, err := exec.Command("git", "-C", path, "status", "--porcelain", "--", "TASK.md").Output() + if err != nil { + t.Fatal(err) + } + if len(status) != 0 { + t.Fatalf("TASK.md not committed, status: %s", status) + } + + sha, err := continuity.TaskFileHash(path) + if err != nil { + t.Fatal(err) + } + if err := continuity.VerifyTaskFile(path, sha); err != nil { + t.Fatalf("VerifyTaskFile: %v", err) + } + + // Re-creating (path already exists) must not touch the committed file. + path2, err := w.Create(context.Background(), task) + if err != nil { + t.Fatalf("recreate: %v", err) + } + if path2 != path { + t.Fatalf("recreate returned different path: %s vs %s", path2, path) + } + got2, err := os.ReadFile(filepath.Join(path, "TASK.md")) + if err != nil { + t.Fatal(err) + } + if string(got2) != string(want) { + t.Fatalf("TASK.md changed on recreate") + } +} diff --git a/progress.md b/progress.md index b237fa3..1fc52bd 100644 --- a/progress.md +++ b/progress.md @@ -75,7 +75,31 @@ Fixed so far: `main()`) so this follows the existing (untested) pattern rather than introducing a one-off test harness. -Not yet started: B6 (Layer 3 wiring), B7 (quota projection has no producer), +- **B5 (loose end)** — `CLIAdapter.Lease`'s initial prompt used `wait=0`, + skipping the inline wait `Bootstrap` already used; the spec (§5.1) requires + inline `wait` on `agent.prompt` for bootstrap injection to avoid sending + into a half-rendered prompt. Changed to `time.Minute`, matching `Bootstrap`. + Small, contained fix — `Release`'s real implementation (needs Phase 4 + handoff production) is still outstanding from B5. + +- **B6 (partial — Phase 4 items 1 and 4)** — nothing wrote a `TASK.md` into a + worktree, so pickup validation had nothing to check and never ran anyway. + Fixed both halves: `GitWorktrees.Create` now writes and commits an + immutable `TASK.md` (`continuity.RenderTaskFile`) into every freshly + created worktree, and `Coordinator.Start` now runs + `continuity.ValidatePickup` (loading the handoff from CAS, checking anchor + SHA + dirty-file hashes + TASK.md hash) before bootstrapping a successor + onto a `handoff_ref` — a failure kills the session and emits `TaskBlocked` + instead of trusting an unvalidated ref. Covered by + `TestGitWorktreesCommitsTaskFile` and `TestStartBlocksOnInvalidPickup` in + `internal/orchestrator`. **Not done:** handoff *production* (nothing yet + writes a real §6.1 handoff — `Release` still refuses per B5), wiring + `ScratchCommit` before release, and the §6.2 bootstrap-prompt rewrite. See + AUDIT.md's "B6 — partial fix" section for the full breakdown, including a + named caveat: TASK.md hashing is best-effort and untested for the + herdr-hosted (`WorktreeCreator`) worktree path. + +Not yet started: B7 (quota projection has no producer), Codex/opencode completion producers, the turn-decision endpoint, S2–S4, S7–S11. See `AUDIT.md` for the full plan.