From 0d7eb0298406896d44ef2a9fd9948e00a79e1aeb Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Mon, 20 Jul 2026 11:01:13 -0500 Subject: [PATCH 1/2] fix: refuse plain "mark it done" writes while a task's PR is still open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks with live, unreviewed PRs were being buried in Done. Complete() already routes a PR-bearing task to 'blocked' so a human decides when it ships, but `ty close`, `ty status done` and `ty bulk close` are plain status writes that skip that decision entirely — and agents reach for them constantly, since `ty close` is on PATH, needs no MCP session, and the gate-step log line advertises it ("Approve it with `ty close ` to release the next phase"). Five tasks were found done this morning whose PRs are still open today (influencekit #760, #1084, #1187, #1195, #1283). - Add completion.CheckDoneWrite: refuses the write when a PR exists and is still open or draft. Merged/closed PRs pass (the human already decided) and no-PR tasks pass, which keeps workflow gate-step releases working since non-terminal steps never open a PR. - Guard `ty close` (with --force), `ty status done`, and `ty bulk close`. - Add completion.RecordStatusWrite so every plain write to done leaves an audit line naming its surface. These writes previously left no trace at all, which is what made diagnosing this an archaeology dig. - Web/desktop board keeps working unguarded — it is a human surface — but now records the same audit line. - internal/ai: stop defaulting an unparsed status to done. The most destructive of the six statuses was the guess for the most ambiguous input. - skills/taskyou: stop documenting `ty close` as the way to mark work complete. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/task/bulk.go | 9 +++ cmd/task/main.go | 37 +++++++++++- internal/ai/command.go | 9 ++- internal/completion/guard.go | 93 +++++++++++++++++++++++++++++++ internal/completion/guard_test.go | 88 +++++++++++++++++++++++++++++ internal/web/handlers.go | 8 +++ skills/taskyou/SKILL.md | 12 +++- 7 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 internal/completion/guard.go create mode 100644 internal/completion/guard_test.go diff --git a/cmd/task/bulk.go b/cmd/task/bulk.go index 7f8ffb4c..2252974d 100644 --- a/cmd/task/bulk.go +++ b/cmd/task/bulk.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/cobra" + "github.com/bborn/workflow/internal/completion" "github.com/bborn/workflow/internal/db" ) @@ -212,11 +213,19 @@ Examples: fmt.Println(dimStyle.Render(fmt.Sprintf("Task #%d is already done, skipping", id))) continue } + // Bulk is where an unguarded write does the most damage: one command + // can bury a dozen tasks that were each waiting on a human. + if guard := completion.CheckDoneWrite(task); guard != nil { + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Skipping task #%d: %s", id, guard.Error()))) + failed++ + continue + } if err := database.UpdateTaskStatus(id, db.StatusDone); err != nil { fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Error closing task #%d: %v", id, err))) failed++ continue } + completion.RecordStatusWrite(database, id, db.StatusDone, "`ty bulk close`") fmt.Println(successStyle.Render(fmt.Sprintf("Closed task #%d: %s", id, task.Title))) succeeded++ } diff --git a/cmd/task/main.go b/cmd/task/main.go index adde9799..8714f4cb 100644 --- a/cmd/task/main.go +++ b/cmd/task/main.go @@ -25,6 +25,7 @@ import ( "github.com/spf13/cobra" "github.com/bborn/workflow/internal/autocomplete" + "github.com/bborn/workflow/internal/completion" "github.com/bborn/workflow/internal/config" "github.com/bborn/workflow/internal/db" "github.com/bborn/workflow/internal/events" @@ -2184,10 +2185,23 @@ Valid statuses: backlog, queued, processing, blocked, done, archived.`, os.Exit(1) } + // Same protection as `ty close`: this is the other plain write that can + // reach 'done', so it must not become the way around the guard. + if status == db.StatusDone { + if guard := completion.CheckDoneWrite(task); guard != nil { + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to mark task #%d done: %s", taskID, guard.Error()))) + fmt.Fprintln(os.Stderr, dimStyle.Render(" Merge or close the PR to complete it automatically, or use `ty close --force`.")) + os.Exit(1) + } + } + if err := database.UpdateTaskStatus(taskID, status); err != nil { fmt.Fprintln(os.Stderr, errorStyle.Render("Error: "+err.Error())) os.Exit(1) } + if status == db.StatusDone { + completion.RecordStatusWrite(database, taskID, status, "`ty status`") + } fmt.Println(successStyle.Render(fmt.Sprintf("Task #%d moved to %s", taskID, status))) }, @@ -2253,6 +2267,7 @@ Valid statuses: backlog, queued, processing, blocked, done, archived.`, rootCmd.AddCommand(pinCmd) // Close subcommand - mark a task as done + var closeForce bool closeCmd := &cobra.Command{ Use: "close ", ValidArgsFunction: completeTaskIDs, @@ -2269,7 +2284,11 @@ Valid statuses: backlog, queued, processing, blocked, done, archived.`, Examples: task close 42 task done 42 - task complete 42`, + task complete 42 + +A task whose pull request is still open is refused: it is awaiting human review, +and the daemon completes it automatically once the PR is merged or closed. Use +--force to override.`, Args: cobra.ExactArgs(1), Run: func(cmd *cobra.Command, args []string) { var taskID int64 @@ -2302,6 +2321,16 @@ Examples: return } + // A task whose PR is still open is waiting on a human, not finished. + // Refusing here is the whole point of the command's existence being + // widely known to agents: `ty close` is the one completion-shaped verb + // that skips every rule, so it must not be able to bury live work. + if guard := completion.CheckDoneWrite(task); guard != nil && !closeForce { + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to close task #%d: %s", taskID, guard.Error()))) + fmt.Fprintln(os.Stderr, dimStyle.Render(" Merge or close the PR to complete it automatically, or re-run with --force.")) + os.Exit(1) + } + // Note: We intentionally do NOT kill the agent session when closing a task. // The tmux window is kept around so users can review the agent's work. // Use 'task sessions cleanup' or 'task delete ' to clean up windows. @@ -2310,10 +2339,16 @@ Examples: fmt.Fprintln(os.Stderr, errorStyle.Render("Error: "+err.Error())) os.Exit(1) } + source := "`ty close`" + if closeForce { + source = "`ty close --force`" + } + completion.RecordStatusWrite(database, taskID, db.StatusDone, source) fmt.Println(successStyle.Render(fmt.Sprintf("Closed task #%d: %s", taskID, task.Title))) }, } + closeCmd.Flags().BoolVar(&closeForce, "force", false, "Close even if the task's PR is still open") rootCmd.AddCommand(closeCmd) // Retry subcommand - retry a blocked/failed task diff --git a/internal/ai/command.go b/internal/ai/command.go index f66fb687..2d5c312b 100644 --- a/internal/ai/command.go +++ b/internal/ai/command.go @@ -274,8 +274,15 @@ func (s *CommandService) parseResponse(response, originalInput string) (*Command // Try to extract task ID from original input cmd.TaskID = extractTaskID(originalInput) } + // No default. This used to fall back to StatusDone, which meant any + // utterance the model classified as "update status" but couldn't pin a + // status to silently became "mark it done" — the most destructive of the + // six statuses chosen as the guess for the most ambiguous input. if cmd.Status == "" { - cmd.Status = db.StatusDone + return &Command{ + Type: CommandUnknown, + Message: "I couldn't tell which status you meant — say e.g. \"move #42 to blocked\".", + }, nil } if cmd.Message == "" { cmd.Message = fmt.Sprintf("Updating task #%d to %s", cmd.TaskID, cmd.Status) diff --git a/internal/completion/guard.go b/internal/completion/guard.go new file mode 100644 index 00000000..758b7222 --- /dev/null +++ b/internal/completion/guard.go @@ -0,0 +1,93 @@ +package completion + +import ( + "fmt" + + "github.com/bborn/workflow/internal/db" + "github.com/bborn/workflow/internal/github" +) + +// OpenPRGuard is the still-open PR that makes a plain "mark it done" write wrong. +// +// Why this exists: Complete() routes a PR-bearing task to 'blocked' so a human +// decides when it ships. But `ty close`, `ty status done` and `ty bulk close` +// are plain status writes that skip that decision entirely, and agents reach for +// them constantly — `ty close` is on PATH, needs no MCP session, and the gate-step +// log line advertises it ("Approve it with `ty close ` to release the next +// phase"). The result is work with an open PR buried in Done where the human who +// was supposed to review it never sees it again. +// +// The guard is deliberately narrow: it fires only when a PR exists AND is still +// open or draft. Merged and closed PRs pass — the human already decided. Tasks +// with no PR pass, which is what keeps legitimate workflow-gate releases working, +// since non-terminal steps never open a PR (only the sink step does). +type OpenPRGuard struct { + Number int + URL string + Draft bool +} + +// Error renders the refusal shown to whoever tried the write. +func (g *OpenPRGuard) Error() string { + kind := "open" + if g.Draft { + kind = "draft" + } + msg := fmt.Sprintf("PR #%d is still %s — this task is waiting on a human review, not finished", g.Number, kind) + if g.URL != "" { + msg += "\n " + g.URL + } + return msg +} + +// CheckDoneWrite reports the open PR blocking a plain write to 'done', or nil if +// the write is allowed. +// +// It reads the PR state already persisted on the task rather than shelling out to +// `gh`. That is a deliberate trade: the daemon's reconciler and Complete() both +// keep this field fresh, and a guard that made a network call on every `ty close` +// would add latency to the common path and fail open whenever GitHub was slow — +// exactly when a wrong answer is most costly. A task whose PR info was never +// recorded falls back to PRNumber, which is set whenever a PR is known at all. +func CheckDoneWrite(task *db.Task) *OpenPRGuard { + if task == nil { + return nil + } + + if info := github.UnmarshalPRInfo(task.PRInfoJSON); info != nil { + switch info.State { + case github.PRStateMerged, github.PRStateClosed: + // The human already merged or closed it — nothing left to protect. + return nil + case github.PRStateOpen, github.PRStateDraft: + return &OpenPRGuard{ + Number: info.Number, + URL: info.URL, + Draft: info.State == github.PRStateDraft, + } + } + } + + // No usable PR state recorded. A known PR number still means a PR was opened + // for this task and nothing has told us it reached a terminal state, so treat + // it as open: failing closed here costs one `--force`, while failing open + // costs a silently buried task. + if task.PRNumber > 0 { + return &OpenPRGuard{Number: task.PRNumber, URL: task.PRURL} + } + return nil +} + +// RecordStatusWrite leaves an audit line naming the surface that changed a task's +// status. Plain status writes previously landed with no trace at all — no task +// log, no PR-info update — which made "who marked this done?" unanswerable after +// the fact and turned a simple diagnosis into an archaeology dig through daemon +// logs and event timestamps. Every non-Complete path that reaches 'done' should +// call this. +func RecordStatusWrite(database *db.DB, taskID int64, status, source string) { + if database == nil { + return + } + database.AppendTaskLog(taskID, "system", + fmt.Sprintf("Status set to %q via %s (plain status write — completion rules not evaluated).", status, source)) +} diff --git a/internal/completion/guard_test.go b/internal/completion/guard_test.go new file mode 100644 index 00000000..70be4b81 --- /dev/null +++ b/internal/completion/guard_test.go @@ -0,0 +1,88 @@ +package completion + +import ( + "testing" + + "github.com/bborn/workflow/internal/db" + "github.com/bborn/workflow/internal/github" +) + +func prJSON(t *testing.T, state github.PRState, number int) string { + t.Helper() + return github.MarshalPRInfo(&github.PRInfo{ + Number: number, + URL: "https://github.com/o/r/pull/1", + State: state, + }) +} + +func TestCheckDoneWrite(t *testing.T) { + tests := []struct { + name string + task *db.Task + blocked bool + }{ + { + name: "nil task is allowed", + task: nil, + blocked: false, + }, + { + name: "no PR at all is allowed", + task: &db.Task{ID: 1}, + blocked: false, + }, + { + // The case that motivated the guard: five tasks were buried in Done + // while their PRs sat open and unreviewed. + name: "open PR is refused", + task: &db.Task{ID: 2, PRNumber: 10, PRInfoJSON: prJSON(t, github.PRStateOpen, 10)}, + blocked: true, + }, + { + name: "draft PR is refused", + task: &db.Task{ID: 3, PRNumber: 11, PRInfoJSON: prJSON(t, github.PRStateDraft, 11)}, + blocked: true, + }, + { + // The human already decided; the daemon's reconciler promotes these. + name: "merged PR is allowed", + task: &db.Task{ID: 4, PRNumber: 12, PRInfoJSON: prJSON(t, github.PRStateMerged, 12)}, + blocked: false, + }, + { + name: "closed PR is allowed", + task: &db.Task{ID: 5, PRNumber: 13, PRInfoJSON: prJSON(t, github.PRStateClosed, 13)}, + blocked: false, + }, + { + // Fail closed: a PR number with no recorded state means a PR exists and + // nothing has told us it reached a terminal state. + name: "PR number with no recorded state is refused", + task: &db.Task{ID: 6, PRNumber: 14}, + blocked: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + guard := CheckDoneWrite(tt.task) + if got := guard != nil; got != tt.blocked { + t.Fatalf("CheckDoneWrite() blocked = %v, want %v", got, tt.blocked) + } + if guard != nil && guard.Error() == "" { + t.Error("guard returned an empty explanation") + } + }) + } +} + +// A workflow gate step is released with `ty close`, which is the legitimate use of +// the command. Non-terminal steps never open a PR (only the sink step does), so +// the guard must not stand in the way of advancing a pipeline. +func TestCheckDoneWriteAllowsGateStepRelease(t *testing.T) { + gateStep := &db.Task{ID: 7, Tags: "pipeline,gate"} + if guard := CheckDoneWrite(gateStep); guard != nil { + t.Fatalf("gate step release was refused: %s", guard.Error()) + } +} diff --git a/internal/web/handlers.go b/internal/web/handlers.go index 716d3233..686dabd7 100644 --- a/internal/web/handlers.go +++ b/internal/web/handlers.go @@ -6,6 +6,7 @@ import ( "strconv" "time" + "github.com/bborn/workflow/internal/completion" "github.com/bborn/workflow/internal/db" "github.com/bborn/workflow/internal/github" ) @@ -360,6 +361,12 @@ func (s *Server) handleSetStatus(w http.ResponseWriter, r *http.Request) { jsonErr(w, "failed to update status", http.StatusInternalServerError) return } + // The board is a human surface, so this write is allowed even with an open PR + // — dragging a card to Done is a deliberate human decision. It is still + // recorded, so "who marked this done?" stays answerable. + if req.Status == db.StatusDone { + completion.RecordStatusWrite(s.db, id, req.Status, "the web board (PATCH status)") + } jsonOK(w, map[string]bool{"ok": true}) } @@ -398,6 +405,7 @@ func (s *Server) handleCloseTask(w http.ResponseWriter, r *http.Request) { jsonErr(w, "failed to close task", http.StatusInternalServerError) return } + completion.RecordStatusWrite(s.db, task.ID, db.StatusDone, "the web board (close)") jsonOK(w, map[string]bool{"ok": true}) } diff --git a/skills/taskyou/SKILL.md b/skills/taskyou/SKILL.md index 12926c2c..a6b8e428 100644 --- a/skills/taskyou/SKILL.md +++ b/skills/taskyou/SKILL.md @@ -68,7 +68,8 @@ Use `ty` (short) or `taskyou` (full) — both work identically. | Retry with feedback | `ty retry --feedback "..."` | | Change status | `ty status ` | | Pin/prioritize | `ty pin ` | -| Close/complete | `ty close ` | +| Finish your own task | `ty complete --summary "..."` (routes a PR-bearing task to review) | +| Close someone else's task (human action) | `ty close ` — refused while its PR is open | | Delete | `ty delete ` | | See executor output | `ty output ` | | Send input to executor | `ty input "message"` | @@ -167,13 +168,18 @@ Move tasks between columns: ```bash ty status backlog # Move to backlog ty status queued # Queue for execution -ty status done # Mark complete +ty status blocked # Park for human review ``` +`ty status done` and `ty close ` are refused while the task's PR is still +open — that task is waiting on a human, and the daemon completes it automatically +once the PR is merged or closed. To finish your *own* task, call `taskyou_complete` +(or `ty complete`), which routes a PR-bearing task to review instead of Done. + ### 8. Close and Cleanup ```bash -ty close # Mark as done +ty close # Mark as done (human action; refused while the PR is open) ty delete # Permanently remove ``` From 77ee5b8f3d91bb969b91eddd1f0c60a4d9796e1c Mon Sep 17 00:00:00 2001 From: Bruno Bornsztein Date: Mon, 20 Jul 2026 15:51:45 -0500 Subject: [PATCH 2/2] fix: satisfy errname lint + guard `ty bulk status done` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename OpenPRGuard.Error() → Reason(). The type is a verdict, not an error value, and is never returned through the error interface; the Error() method name tripped golangci-lint's errname (types with Error() must be named XxxError). Renaming the method is truer than renaming the type. - Close a gap in this PR's own coverage: `ty bulk status done` was a plain write to done that skipped the guard. It now runs CheckDoneWrite + RecordStatusWrite like the other done-writing paths. Co-Authored-By: Claude Opus 4.8 (1M context) --- cmd/task/bulk.go | 14 +++++++++++++- cmd/task/main.go | 4 ++-- internal/completion/guard.go | 6 ++++-- internal/completion/guard_test.go | 4 ++-- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/cmd/task/bulk.go b/cmd/task/bulk.go index 2252974d..79c33092 100644 --- a/cmd/task/bulk.go +++ b/cmd/task/bulk.go @@ -95,11 +95,23 @@ Examples: failed++ continue } + // `ty bulk status done` reaches the same buried-with-an-open-PR outcome + // as `ty bulk close`, so it gets the same guard. + if status == db.StatusDone { + if guard := completion.CheckDoneWrite(task); guard != nil { + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Skipping task #%d: %s", id, guard.Reason()))) + failed++ + continue + } + } if err := database.UpdateTaskStatus(id, status); err != nil { fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Error updating task #%d: %v", id, err))) failed++ continue } + if status == db.StatusDone { + completion.RecordStatusWrite(database, id, db.StatusDone, "`ty bulk status done`") + } fmt.Println(successStyle.Render(fmt.Sprintf("Task #%d moved to %s", id, status))) succeeded++ } @@ -216,7 +228,7 @@ Examples: // Bulk is where an unguarded write does the most damage: one command // can bury a dozen tasks that were each waiting on a human. if guard := completion.CheckDoneWrite(task); guard != nil { - fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Skipping task #%d: %s", id, guard.Error()))) + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Skipping task #%d: %s", id, guard.Reason()))) failed++ continue } diff --git a/cmd/task/main.go b/cmd/task/main.go index 8714f4cb..294ad457 100644 --- a/cmd/task/main.go +++ b/cmd/task/main.go @@ -2189,7 +2189,7 @@ Valid statuses: backlog, queued, processing, blocked, done, archived.`, // reach 'done', so it must not become the way around the guard. if status == db.StatusDone { if guard := completion.CheckDoneWrite(task); guard != nil { - fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to mark task #%d done: %s", taskID, guard.Error()))) + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to mark task #%d done: %s", taskID, guard.Reason()))) fmt.Fprintln(os.Stderr, dimStyle.Render(" Merge or close the PR to complete it automatically, or use `ty close --force`.")) os.Exit(1) } @@ -2326,7 +2326,7 @@ and the daemon completes it automatically once the PR is merged or closed. Use // widely known to agents: `ty close` is the one completion-shaped verb // that skips every rule, so it must not be able to bury live work. if guard := completion.CheckDoneWrite(task); guard != nil && !closeForce { - fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to close task #%d: %s", taskID, guard.Error()))) + fmt.Fprintln(os.Stderr, errorStyle.Render(fmt.Sprintf("Refusing to close task #%d: %s", taskID, guard.Reason()))) fmt.Fprintln(os.Stderr, dimStyle.Render(" Merge or close the PR to complete it automatically, or re-run with --force.")) os.Exit(1) } diff --git a/internal/completion/guard.go b/internal/completion/guard.go index 758b7222..3553e90c 100644 --- a/internal/completion/guard.go +++ b/internal/completion/guard.go @@ -27,8 +27,10 @@ type OpenPRGuard struct { Draft bool } -// Error renders the refusal shown to whoever tried the write. -func (g *OpenPRGuard) Error() string { +// Reason renders the refusal shown to whoever tried the write. This is +// deliberately not an Error() method: OpenPRGuard is a verdict, not an error +// value, and is never returned through the error interface. +func (g *OpenPRGuard) Reason() string { kind := "open" if g.Draft { kind = "draft" diff --git a/internal/completion/guard_test.go b/internal/completion/guard_test.go index 70be4b81..c6d66ccc 100644 --- a/internal/completion/guard_test.go +++ b/internal/completion/guard_test.go @@ -70,7 +70,7 @@ func TestCheckDoneWrite(t *testing.T) { if got := guard != nil; got != tt.blocked { t.Fatalf("CheckDoneWrite() blocked = %v, want %v", got, tt.blocked) } - if guard != nil && guard.Error() == "" { + if guard != nil && guard.Reason() == "" { t.Error("guard returned an empty explanation") } }) @@ -83,6 +83,6 @@ func TestCheckDoneWrite(t *testing.T) { func TestCheckDoneWriteAllowsGateStepRelease(t *testing.T) { gateStep := &db.Task{ID: 7, Tags: "pipeline,gate"} if guard := CheckDoneWrite(gateStep); guard != nil { - t.Fatalf("gate step release was refused: %s", guard.Error()) + t.Fatalf("gate step release was refused: %s", guard.Reason()) } }