fix: refuse task check declarations the completion gate cannot approve

docs/changes/unreleased/1812-check-declarations.md

@@ -0,0 +1,8 @@
+---
+kind: fixed
+title: task check declarations are refused before work when they cannot be rerun
+pr: 1812
+surface: [engine, docs]
+invalidates:
+  - "Task creation and check replacement accepted compound commands or variable-dependent declarations that the completion gate could never approve. Admission now asks that gate's existing predicate before changing the plan, and explains how to declare separate commands from the task-copy root."
+---

internal/manual/chat/worker-harness.md

@@ -937,6 +937,15 @@ that proves it>'` to `plandb add`, and repeat `--check` when the task has more t
 one command to run. The check runs every command exactly as declared before it
 probes any acceptance sentence those commands do not cover.
 
+Each declaration is **one rerunnable command the read-only check can run**.
+`plandb add` and `plandb task set-checks` refuse joins, pipes, redirections,
+variable expansions, command substitutions and commands the check is not allowed to run before anything is
+added or changed. Repeat `--check` for separate commands. Checks run from the
+root of the task's own copy: omit `cd` and name files relative to that root.
+The command is not split or rewritten for you; repair the declaration and try
+the add or set-checks again. Quoted shell characters that are only text remain
+allowed, such as `grep 'alpha && beta' README.md`.
+
 A task with **no declared check** is checked by reading its result and by the
 acceptance alone. Commands mentioned only in the task's prose are not declarations,
 so put every command the check must run on the task with `--check`.

internal/plandb/check_declaration_test.go

@@ -0,0 +1,162 @@
+package plandb
+
+import (
+	"encoding/json"
+	"path/filepath"
+	"reflect"
+	"strings"
+	"testing"
+	"time"
+)
+
+// A CHECK THAT CAN NEVER EARN HOLDS IS REFUSED BEFORE THE PLAN CHANGES. The
+// shell CLI is the real admission door used by workers, including replacements.
+func TestCliUnusableCheckDeclarationsChangeNothing(t *testing.T) {
+	for _, command := range []string{
+		"cd /tmp/tree && go test ./...",
+		"go test ./... && go vet ./...",
+		"go test ./...; go vet ./...",
+		"go test ./... | tee result.txt",
+		"go test ./... > result.txt",
+		`grep "$(pwd)" README.md`,
+		`test -f "$FILE"`,
+		"go test ./...\ngo vet ./...",
+		"sudo reboot",
+		"*",
+		"--help",
+	} {
+		for _, verb := range []string{"add", "set-checks"} {
+			t.Run(verb+"/"+command, func(t *testing.T) {
+				h := cliNewHarness(t)
+				h.cliInitFresh()
+				h.cliAdd("Existing", "existing", "--check", "go test ./...")
+				argv := []string{"--db", h.db, "add", "Invalid", "--as", "invalid", "--check", command}
+				if verb == "set-checks" {
+					argv = []string{"--db", h.db, "task", "set-checks", "t-existing", "--check", command}
+				}
+				if code := h.run(argv...); code == 0 {
+					t.Fatalf("unusable declaration was accepted: %q", command)
+				}
+				if !strings.Contains(h.errb.String(), "--check") {
+					t.Fatalf("refusal gives no declaration remedy: %s", h.errb)
+				}
+				store := planReopen(t, h.db)
+				defer store.Close()
+				if store.Task("invalid") != nil || len(store.Tasks()) != 2 {
+					t.Fatal("refused declaration changed the plan")
+				}
+				if got := store.Task("existing").Checks; !reflect.DeepEqual(got, []string{"go test ./..."}) {
+					t.Fatalf("refused replacement changed checks: %q", got)
+				}
+			})
+		}
+	}
+}
+
+// BATCH ADMISSION AND REVISION OWN THE SAME CONTRACT AS THE CLI, and each
+// refusal rolls back the entire write rather than keeping its valid prefix.
+func TestStoreUnusableCheckDeclarationsAreAtomic(t *testing.T) {
+	store := planOpen(t, filepath.Join(t.TempDir(), "plan.db"))
+	defer store.Close()
+	bad := []string{"go test ./... && go vet ./..."}
+	if _, err := store.AddMany([]TaskSpec{
+		{ID: "first", Title: "First", Checks: []string{"go test ./..."}},
+		{ID: "bad", Title: "Bad", Checks: bad},
+	}); err == nil {
+		t.Fatal("batch admitted a check its completion gate cannot approve")
+	}
+	if store.Task("first") != nil || store.Task("bad") != nil {
+		t.Fatal("refused batch kept a partial plan")
+	}
+	planAdd(t, store, TaskSpec{ID: "existing", Title: "Existing", Checks: []string{"go test ./..."}})
+	title := "Changed"
+	if _, err := store.Revise("existing", TaskPatch{Title: &title, Checks: &bad}); err == nil {
+		t.Fatal("revision admitted an unusable check")
+	}
+	if task := store.Task("existing"); task.Title != "Existing" || !reflect.DeepEqual(task.Checks, []string{"go test ./..."}) {
+		t.Fatalf("refused revision changed the contract: %#v", task.TaskSpec)
+	}
+}
+
+func TestUnusableReviewCheckDoesNotReopenFinishedWork(t *testing.T) {
+	store := planOpen(t, filepath.Join(t.TempDir(), "plan.db"))
+	defer store.Close()
+	planAdd(t, store, TaskSpec{ID: "parent", Title: "Finished work"})
+	planFinish(t, store, "parent", "worker", "finished")
+	if _, err := store.AddReviewCheck(TaskSpec{
+		ID: "review", Title: "Check", ParentID: "parent", Role: RoleCheck,
+		Checks: []string{"go test ./... && go vet ./..."},
+	}); err == nil {
+		t.Fatal("unusable review declaration was admitted")
+	}
+	if store.Task("review") != nil || store.Task("parent").Status != StatusDone {
+		t.Fatal("refused review changed finished work")
+	}
+}
+
+// OLD ROWS STILL FACE THE COMPLETION GATE, even when their commands appear
+// with zero exits. Admission must not turn those checks into reading contracts
+// or prevent an unrelated question edit on work that already exists.
+func TestLegacyUnusableChecksCannotEarnHoldsWithZeroExit(t *testing.T) {
+	for _, command := range []string{"go test ./... && go vet ./...", "sudo reboot", "*"} {
+		t.Run(command, func(t *testing.T) {
+			store := planOpen(t, filepath.Join(t.TempDir(), "plan.db"))
+			defer store.Close()
+			planAdd(t, store, TaskSpec{ID: "review", Title: "Check", Role: RoleCheck})
+			// Restore the bytes of a row admitted before this guard existed.
+			if _, err := store.changeTask("review", func(_ *state, task *Task, _ time.Time) error {
+				task.Checks = []string{command}
+				return nil
+			}); err != nil {
+				t.Fatal(err)
+			}
+			question := "Which requirement remains?"
+			if _, err := store.Revise("review", TaskPatch{Question: &question}); err != nil {
+				t.Fatalf("legacy check blocked an unrelated question: %v", err)
+			}
+			if _, err := store.Claim("review", "checker"); err != nil {
+				t.Fatal(err)
+			}
+			line, err := json.Marshal(map[string]any{"kind": "step", "command": command, "exit_code": 0})
+			if err != nil {
+				t.Fatal(err)
+			}
+			writeTrajectoryLines(t, store, "review", string(line))
+			if _, err := store.Done("review", "checker", "holds: a command exited zero", nil, nil); err == nil {
+				t.Fatal("an unusable legacy declaration earned holds")
+			}
+		})
+	}
+}
+
+// SEPARATE COMMANDS AND QUOTED TEXT KEEP THEIR EXACT BYTES and earn holds only
+// after every declaration appears with a zero exit in this checker's record.
+func TestCliSeparateCheckDeclarationsCanEarnRecordedCompletion(t *testing.T) {
+	h := cliNewHarness(t)
+	h.cliInitFresh()
+	checks := []string{"go test ./...", `grep 'alpha && beta' README.md`}
+	h.cliAdd("Check", "review", "--role", RoleCheck, "--check", checks[0], "--check", checks[1])
+	if code := h.run("--db", h.db, "go", "--agent", "checker"); code != 0 {
+		t.Fatalf("claim: %s", h.errb)
+	}
+	if code := h.run("--db", h.db, "done", "t-review", "--agent", "checker", "--result", "holds: both checks passed"); code == 0 {
+		t.Fatal("declaration without recorded runs earned completion")
+	}
+	store := planReopen(t, h.db)
+	if got := store.Task("review").Checks; !reflect.DeepEqual(got, checks) {
+		t.Fatalf("declarations changed: %q", got)
+	}
+	lines := []string{`{"kind":"begin","exits_recorded":true}`}
+	for _, check := range checks {
+		line, err := json.Marshal(map[string]any{"kind": "step", "command": "cd /tmp/tree && " + check, "exit_code": 0})
+		if err != nil {
+			t.Fatal(err)
+		}
+		lines = append(lines, string(line))
+	}
+	writeTrajectoryLines(t, store, "review", lines...)
+	store.Close()
+	if code := h.run("--db", h.db, "done", "t-review", "--agent", "checker", "--result", "holds: both checks passed"); code != 0 {
+		t.Fatalf("recorded declarations did not earn completion: %s", h.errb)
+	}
+}

internal/plandb/cli_check_placeholder_test.go

@@ -113,7 +113,6 @@ func TestAddKeepsLegitimateChecksExactly(t *testing.T) {
 		{"existing underscore file", "gofmt -l issue_.go"},
 		{"no numbered sibling", "gofmt -l notes_.go"},
 		{"glob", "gofmt -l issue_*.go"},
-		{"shell variable", `test -f "$FILE"`},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			dir := t.TempDir()

internal/plandb/store.go

@@ -229,6 +229,9 @@ func (s *Store) AddMany(specs []TaskSpec) ([]*Task, error) {
 			if err := validateSpec(specs[i]); err != nil {
 				return fmt.Errorf("task %d: %w", i, err)
 			}
+			if err := validateCheckDeclarations(specs[i].Checks); err != nil {
+				return fmt.Errorf("task %d: %w", i, err)
+			}
 			if next.Tasks[specs[i].ID] != nil || next.Deleted[specs[i].ID] || batch[specs[i].ID] {
 				return fmt.Errorf("duplicate task id %q", specs[i].ID)
 			}
@@ -515,6 +518,9 @@ func (s *Store) AddReviewCheck(spec TaskSpec) (*Task, error) {
 		if err := validateSpec(spec); err != nil {
 			return err
 		}
+		if err := validateCheckDeclarations(spec.Checks); err != nil {
+			return err
+		}
 		if spec.Role != RoleCheck {
 			return errors.New("only a review check is seated this way: the child must have role check")
 		}
@@ -804,6 +810,26 @@ func auditableDeclaredCheck(command string) bool {
 // about what a critical command is or the gate and the door drift apart.
 var auditAllowAll = approval.Policy{Default: approval.ActionAllow}
 
+// validateCheckDeclarations asks the completion gate's own question before a
+// declaration is written. A command that gate can never approve must not buy a
+// worker and a check that spend their turns trying to earn an impossible holds.
+// THE COMMAND IS REFUSED, NEVER REWRITTEN: removing a directory change or
+// splitting a composition could check different files or drop a required step.
+func validateCheckDeclarations(checks []string) error {
+	for _, raw := range checks {
+		command := strings.TrimSpace(raw)
+		if command == "" || auditableDeclaredCheck(command) {
+			continue
+		}
+		reason := "is not one command the read-only checker may run"
+		if offending, composed := approval.FirstCompositionOutsideQuotes(command); composed {
+			reason = fmt.Sprintf("joins, redirects or expands commands with %q outside single quotes", string(offending))
+		}
+		return fmt.Errorf("check %q %s. Declare one rerunnable command per --check; checks run from the root of the task's own copy, so omit cd and use paths relative to that root", command, reason)
+	}
+	return nil
+}
+
 // SetVerdictBasis records HOW a task's verdict was earned on the task itself.
 // Done writes it by the same gate that judged the verdict; this store method
 // is the seam a reader with a basis it already holds writes through, and the
@@ -1127,6 +1153,14 @@ func (s *Store) Revise(id string, patch TaskPatch) (*Task, error) {
 		if task.Status != StatusPending && task.Status != StatusReady && !questionOnlyPatch(patch) {
 			return fmt.Errorf("task %q can only be revised before execution", id)
 		}
+		// Only a new declaration is admitted here. An older stored declaration
+		// may be unusable, but that must not prevent unrelated question edits;
+		// its completion still has to pass the unchanged audit gate.
+		if patch.Checks != nil {
+			if err := validateCheckDeclarations(*patch.Checks); err != nil {
+				return err
+			}
+		}
 		applyPatch(&task.TaskSpec, patch)
 		task.TaskSpec = normalizeSpec(task.TaskSpec, next.RootID)
 		if task.ID == next.RootID {

internal/run/trajectory_store_test.go

@@ -57,18 +57,22 @@ func writeCheckRuns(t *testing.T, store *plandb.Store, id string, runs map[strin
 // trees, so this proof cannot depend on recognizing a particular runner word.
 func TestReviewContractComesOnlyFromTheWorkersDeclarationAcrossFixtures(t *testing.T) {
 	fixtures := []struct {
-		name string
-		root string
-		task string
+		name     string
+		root     string
+		task     string
+		declared []string
 	}{
-		{name: "long recorded work", root: "testdata/c249", task: "jcrhzh"},
-		{name: "alternate recorded work", root: "testdata/declared", task: "alternate"},
+		{name: "long recorded work", root: "testdata/c249", task: "jcrhzh", declared: []string{"go test ./...", "go vet ./..."}},
+		{name: "alternate recorded work", root: "testdata/declared", task: "alternate", declared: []string{"python -m unittest discover", "python -m compileall -q src"}},
 	}
 	for _, fixture := range fixtures {
 		t.Run(fixture.name, func(t *testing.T) {
 			recorded := fixtureCommands(t, fixture.root, fixture.task)
-			declared := []string{recorded[len(recorded)-1], recorded[0]}
+			declared := fixture.declared
 			store := runOpenStore(t)
+			// The work record may contain directory wrappers and compositions.
+			// It is evidence about the work, never a source of declarations.
+			writeCheckRuns(t, store, "leaf", map[string]int{recorded[0]: 0, recorded[len(recorded)-1]: 0})
 			seat := newFakeSeat()
 			seat.actions["root"] = splitRoot(t, store, plandb.TaskSpec{
 				ID: "leaf", Title: "the leaf", Checks: declared,
@@ -109,7 +113,14 @@ func TestHoldsRequiresEveryDeclaredRunWithZeroExitAndAuditApproval(t *testing.T)
 				checks = []string{"*"}
 			}
 			store := runOpenStore(t)
-			if _, err := store.AddMany([]plandb.TaskSpec{{ID: "review", Title: "check: leaf", Role: plandb.RoleCheck, Checks: checks}}); err != nil {
+			_, err := store.AddMany([]plandb.TaskSpec{{ID: "review", Title: "check: leaf", Role: plandb.RoleCheck, Checks: checks}})
+			if tc.name == "declaration is not auditable" {
+				if err == nil {
+					t.Fatal("unauditable declaration admitted before completion")
+				}
+				return
+			}
+			if err != nil {
 				t.Fatal(err)
 			}
 			if _, err := store.Claim("review", "review"); err != nil {

internal/session/prompts/bashworker.md

@@ -53,7 +53,12 @@ plandb task cancel t-<id>            # plandb what-if cancel t-<id> previews the
 
 ALWAYS use `--description`. It is the work order: the worker that gets the
 task reads it instead of your whole brief. Repeat `--check` for every command
-that proves the task. Every delegated task has at least one `--check`. `split --into` takes a JSON array
+that proves the task. Each check is one rerunnable command, run from the root
+of the task's own copy: omit `cd`, joins, pipes, redirections, variable expansions
+and command substitutions,
+and name files relative to that root. An unusable check is refused before the
+plan changes; repair the declaration and run the add or set-checks again.
+Every delegated task has at least one `--check`. `split --into` takes a JSON array
 (`deps_on` names sibling titles), comma titles, or an `A > B > C` chain, and
 answers the created ids; use them, not the titles, for everything that
 follows.