From 50c04150aa93045508be03aacc66412cbd9d20be Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 09:47:35 -0400 Subject: [PATCH 1/4] fix: refuse unusable check declarations before task admission --- internal/manual/chat/worker-harness.md | 9 ++ internal/plandb/check_declaration_test.go | 161 ++++++++++++++++++++++ internal/plandb/store.go | 34 +++++ internal/run/trajectory_store_test.go | 9 +- internal/session/prompts/bashworker.md | 6 +- 5 files changed, 217 insertions(+), 2 deletions(-) create mode 100644 internal/plandb/check_declaration_test.go diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index d719bf6b25..91b0642bf5 100644 --- a/internal/manual/chat/worker-harness.md +++ b/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, +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`. diff --git a/internal/plandb/check_declaration_test.go b/internal/plandb/check_declaration_test.go new file mode 100644 index 0000000000..54c760d875 --- /dev/null +++ b/internal/plandb/check_declaration_test.go @@ -0,0 +1,161 @@ +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`, + "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) + } +} diff --git a/internal/plandb/store.go b/internal/plandb/store.go index 4e37e50e2d..d2c0acb733 100644 --- a/internal/plandb/store.go +++ b/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 { diff --git a/internal/run/trajectory_store_test.go b/internal/run/trajectory_store_test.go index 7fd017a576..83438bb166 100644 --- a/internal/run/trajectory_store_test.go +++ b/internal/run/trajectory_store_test.go @@ -109,7 +109,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 { diff --git a/internal/session/prompts/bashworker.md b/internal/session/prompts/bashworker.md index 6c042c0176..6b85628b53 100644 --- a/internal/session/prompts/bashworker.md +++ b/internal/session/prompts/bashworker.md @@ -53,7 +53,11 @@ plandb task cancel t- # plandb what-if cancel t- 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 and 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. From fbdb431cc7e2f2d1a1ecc92b5cf1d2438506c643 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 09:49:31 -0400 Subject: [PATCH 2/4] test: align dynamic check declarations with completion law --- internal/manual/chat/worker-harness.md | 2 +- internal/plandb/check_declaration_test.go | 1 + internal/plandb/cli_check_placeholder_test.go | 1 - internal/session/prompts/bashworker.md | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 91b0642bf5..7a001d4951 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -939,7 +939,7 @@ 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, -substitutions and commands the check is not allowed to run before anything is +shell expansions 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 diff --git a/internal/plandb/check_declaration_test.go b/internal/plandb/check_declaration_test.go index 54c760d875..2a37dd687f 100644 --- a/internal/plandb/check_declaration_test.go +++ b/internal/plandb/check_declaration_test.go @@ -19,6 +19,7 @@ func TestCliUnusableCheckDeclarationsChangeNothing(t *testing.T) { "go test ./... | tee result.txt", "go test ./... > result.txt", `grep "$(pwd)" README.md`, + `test -f "$FILE"`, "go test ./...\ngo vet ./...", "sudo reboot", "*", diff --git a/internal/plandb/cli_check_placeholder_test.go b/internal/plandb/cli_check_placeholder_test.go index 145b2e8740..3a6de5985a 100644 --- a/internal/plandb/cli_check_placeholder_test.go +++ b/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() diff --git a/internal/session/prompts/bashworker.md b/internal/session/prompts/bashworker.md index 6b85628b53..cb070b010a 100644 --- a/internal/session/prompts/bashworker.md +++ b/internal/session/prompts/bashworker.md @@ -54,7 +54,7 @@ plandb task cancel t- # plandb what-if cancel t- 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. Each check is one rerunnable command, run from the root -of the task's own copy: omit `cd`, joins, pipes, redirections and substitutions, +of the task's own copy: omit `cd`, joins, pipes, redirections and shell expansions, 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 From 47a34831111caf681e7231ed845bc6e93cd3f6b7 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 09:52:55 -0400 Subject: [PATCH 3/4] test: separate recorded work from admitted proof commands --- internal/manual/chat/worker-harness.md | 2 +- internal/run/trajectory_store_test.go | 16 ++++++++++------ internal/session/prompts/bashworker.md | 3 ++- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/manual/chat/worker-harness.md b/internal/manual/chat/worker-harness.md index 7a001d4951..968ef5e285 100644 --- a/internal/manual/chat/worker-harness.md +++ b/internal/manual/chat/worker-harness.md @@ -939,7 +939,7 @@ 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, -shell expansions and commands the check is not allowed to run before anything is +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 diff --git a/internal/run/trajectory_store_test.go b/internal/run/trajectory_store_test.go index 83438bb166..72de33ecd7 100644 --- a/internal/run/trajectory_store_test.go +++ b/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, diff --git a/internal/session/prompts/bashworker.md b/internal/session/prompts/bashworker.md index cb070b010a..dc056fd0af 100644 --- a/internal/session/prompts/bashworker.md +++ b/internal/session/prompts/bashworker.md @@ -54,7 +54,8 @@ plandb task cancel t- # plandb what-if cancel t- 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. Each check is one rerunnable command, run from the root -of the task's own copy: omit `cd`, joins, pipes, redirections and shell expansions, +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 From b48dc27579af098431c84a3007b03e1a025371d6 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 13 Oct 2026 10:43:39 -0400 Subject: [PATCH 4/4] docs: record check declaration repair for PR 1812 --- docs/changes/unreleased/1812-check-declarations.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 docs/changes/unreleased/1812-check-declarations.md diff --git a/docs/changes/unreleased/1812-check-declarations.md b/docs/changes/unreleased/1812-check-declarations.md new file mode 100644 index 0000000000..0f3db090ba --- /dev/null +++ b/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." +---