fix: refuse plain "mark it done" writes while a task's PR is still open

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"
 )
 
@@ -94,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++
 			}
@@ -212,11 +225,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.Reason())))
+					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++
 			}

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.Reason())))
+					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 <task-id>",
 		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.Reason())))
+				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 <id>' 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

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)

internal/completion/guard.go

@@ -0,0 +1,95 @@
+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 <id> 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 <id>` 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
+}
+
+// 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"
+	}
+	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))
+}

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.Reason() == "" {
+				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.Reason())
+	}
+}

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})
 }