diff --git a/cmd/task/bulk.go b/cmd/task/bulk.go index 7f8ffb4c..79c33092 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" ) @@ -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++ } diff --git a/cmd/task/main.go b/cmd/task/main.go index adde9799..294ad457 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.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 ", 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 ' 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..3553e90c --- /dev/null +++ b/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 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 +} + +// 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)) +} diff --git a/internal/completion/guard_test.go b/internal/completion/guard_test.go new file mode 100644 index 00000000..c6d66ccc --- /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.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()) + } +} 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 ```