diff --git a/docs/design/cli-ux-design.md b/docs/design/cli-ux-design.md new file mode 100644 index 0000000..6be7d5a --- /dev/null +++ b/docs/design/cli-ux-design.md @@ -0,0 +1,1006 @@ +# Opal CLI UX Improvements — Technical Design + +**Status:** Draft — awaiting feedback +**Date:** 2026-02-19 +**Requirements:** `docs/design/cli-ux-improvements.md` + +--- + +## Architecture Overview + +The improvements fall into four architectural themes: + +1. **Undo system** — leverages the existing `change_log` for state recovery, + with a lightweight undo stack for tracking CLI operations (IMP-1) +2. **Output / feedback** — richer CLI output from existing commands (IMP-2, 3, 6, 7, 9) +3. **Parser / ID resolution fixes** — correctness in `cmd/` and `engine/` (IMP-4, 5) +4. **New commands & features** — annotations, history, completions, dry-run, version + (IMP-8, 10, 11, 12, 13) + +``` +┌──────────────────────────────────────────────────────┐ +│ cmd/ layer │ +│ │ +│ add done delete modify start stop info edit │ +│ undo uncomplete annotate denotate log version │ +│ completion │ +│ │ +│ ┌────────────┐ ┌──────────────┐ ┌──────────────┐ │ +│ │ confirmutil│ │ feedback │ │ dry-run │ │ +│ │ (IMP-3,6) │ │ (IMP-2,7) │ │ (IMP-10) │ │ +│ └────────────┘ └──────────────┘ └──────────────┘ │ +├──────────────────────────────────────────────────────┤ +│ engine/ layer │ +│ │ +│ ┌──────────┐ ┌──────────┐ ┌──────────┐ ┌─────────┐ │ +│ │ undo.go │ │ annotate │ │ reldate │ │modifier │ │ +│ │ (IMP-1) │ │ (IMP-11) │ │ (IMP-9) │ │(IMP-5) │ │ +│ └──────────┘ └──────────┘ └──────────┘ └─────────┘ │ +│ │ +│ task.go ws.go filter.go display.go database.go │ +├──────────────────────────────────────────────────────┤ +│ SQLite (opal.db) │ +│ │ +│ tasks (+annotations col) tags working_set │ +│ change_log undo_stack (new, lightweight) │ +└──────────────────────────────────────────────────────┘ +``` + +--- + +## Component Designs + +All schema changes are made directly in the existing migration v1 in +`database.go`. No migration system needed — we are pre-production and have no +backwards compatibility constraints. + +### 1. Undo System (IMP-1) + +The undo system builds on top of the existing `change_log`. The change_log +already captures the full task state (via SQLite triggers) on every mutation. +Instead of duplicating that data, undo reads prior states from the change_log +and uses a lightweight stack to track which CLI operations are undoable. + +#### How it works + +The `change_log` stores the **post-mutation** state of a task on every INSERT +and UPDATE (via the `track_task_create` and `track_task_update` triggers). +This means: + +- Entry at `id=N`: the task state **after** the Nth mutation +- Entry at `id=N-1` (same task_uuid): the task state **before** the Nth mutation + +To undo mutation N, we restore the state from entry N-1. The sync client +already has `applyChangeDataToTask()` which parses the key:value `data` field +back into a `*Task` — we reuse this. + +#### Data Model — `undo_stack` table + +```sql +CREATE TABLE undo_stack ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + created_at INTEGER NOT NULL, + op_type TEXT NOT NULL, -- 'add','done','delete','modify','start','stop' + task_uuid TEXT NOT NULL, + change_log_id INTEGER NOT NULL -- the change_log entry created by this mutation +); +``` + +This is deliberately lightweight — no snapshots, no duplicate data. The actual +task state lives in `change_log`. + +The table is capped at 10 rows (oldest evicted on insert). + +#### Engine Interface — `engine/undo.go` + +```go +// RecordUndo records a CLI operation as undoable. +// Called AFTER the mutation (so the change_log entry exists). +// Finds the change_log entry created by this mutation and stores a reference. +func RecordUndo(opType string, taskUUID uuid.UUID) error + +// PopUndo pops the most recent undo entry and reverts the task. +// Returns a description of what was undone for display. +func PopUndo() (description string, err error) +``` + +**`RecordUndo` behavior:** +1. `SELECT MAX(id) FROM change_log WHERE task_uuid = ?` — find the entry just + created by this mutation. +2. `INSERT INTO undo_stack (created_at, op_type, task_uuid, change_log_id) VALUES (...)`. +3. Evict: `DELETE FROM undo_stack WHERE id NOT IN (SELECT id FROM undo_stack ORDER BY id DESC LIMIT 10)`. + +**`PopUndo` behavior (by op type):** + +| Original op | Revert action | +|-------------|---------------| +| `add` | Hard delete the task (`DELETE FROM tasks WHERE uuid = ?`). For recurring tasks, also delete the template. | +| `done` | Find the change_log entry BEFORE `change_log_id` for the same `task_uuid`. Parse its `data` field into a Task. Restore that state (sets status back to pending, clears End). Save. | +| `delete` | Same as `done` — restore from prior change_log entry. | +| `modify` | Restore full state from prior change_log entry. Reconcile tags. | +| `start` | Restore from prior change_log entry (clears Start). | +| `stop` | Restore from prior change_log entry (restores Start). | + +**For `add`:** The change_log entry has `change_type='create'`. There is no +prior entry. `PopUndo` detects `op_type='add'` and performs a hard delete +instead of restoring. + +**Sync behavior:** The undo stack itself is local (not synced), but the +*revert* is a normal `task.Save()` which fires change_log triggers and syncs +like any other mutation. This means: +- Undo on device A → the reverted state syncs to device B +- Device B cannot "undo" device A's operations (the undo stack is per-device) + +#### Uncomplete — dedicated path + +`opal uncomplete` does NOT use the undo stack. It directly: +1. Loads the task by display ID. +2. Asserts `status == StatusCompleted`. +3. Sets `status = StatusPending`, clears `End`. +4. Saves. + +This is simpler and more predictable than routing through undo. Uncomplete is +an explicit, targeted action; undo is a generic "oops" button. + +#### Cmd Layer — `cmd/undo.go`, `cmd/uncomplete.go` + +```go +// cmd/undo.go +var undoCmd = &cobra.Command{ + Use: "undo", + Short: "Undo the last action", + // ... +} + +// cmd/uncomplete.go +var uncompleteCmd = &cobra.Command{ + Use: "uncomplete [filter...]", + Short: "Restore a completed task to pending", + // ... +} +``` + +#### Integration Points + +Every mutating command (`add`, `done`, `delete`, `modify`, `start`, `stop`) +must call `engine.RecordUndo(opType, taskUUID)` AFTER the mutation succeeds. +This is a one-line addition per command. + +--- + +### 2. Action Command Output (IMP-2, 3, 6, 7) + +These four improvements share a common pattern: richer output from existing +action commands. Rather than scattering formatting logic across each `cmd/*.go` +file, we introduce a shared output helper. + +#### Engine Interface — `engine/feedback.go` + +```go +// FormatTaskSummary returns a one-line summary for action feedback. +// Example: `3 "Buy groceries" due:tomorrow +errand` +func FormatTaskSummary(task *Task, ws *WorkingSet) string + +// FormatTaskConfirmList returns the multi-task confirmation block. +// Shows up to 10 tasks, then "...and N more". +func FormatTaskConfirmList(tasks []*Task, ws *WorkingSet) string + +// FormatAddFeedback returns the detailed post-add feedback block. +// Includes display ID, description, all parsed modifiers. +func FormatAddFeedback(task *Task, displayID int) string + +// FormatCompletionFeedback returns completion feedback with recurrence info. +// If the completed task is a recurring instance, includes next instance details. +func FormatCompletionFeedback(task *Task, displayID int, nextInstance *Task) string +``` + +#### IMP-2: Better `add` feedback + +**Changes to `cmd/add.go`:** + +After `CreateTaskWithModifier`, the add command needs a display ID for the new +task. Load the working set and insert the new task into the `working_set` +table at `MAX(display_id) + 1`. This makes the ID immediately usable +(`opal done`) until the next report render. + +```go +// engine/ws.go — new function +func AppendTask(task *Task) (int, error) +``` + +`AppendTask` inserts a new row into `working_set` with +`display_id = MAX(display_id) + 1` and returns the assigned ID. + +**Note on ID stability:** The working set is fully recalculated on every report +render — all display IDs shift based on the report's sort order. The appended +ID is therefore valid only until the next report runs. This is inherent to the +working set design and matches existing behavior (IDs from a previous `list` +can shift after running a different report). The ID is useful for the immediate +"add then act" workflow: `opal add ... && opal 7 done`. + +Output format (non-recurring): +``` +Created task 7 — "Buy groceries" + Due: tomorrow (2026-02-20) + Tags: errand + Priority: D +``` + +Output format (recurring): +``` +Created recurring task 7 — "Weekly review" + Recurrence: 1w + Due: monday (2026-02-24) + Tags: meetings +``` + +#### IMP-3: Show matched tasks in confirmations + +**Shared pattern for `done`, `delete`, `modify`:** + +Replace the current confirmation prompt with `FormatTaskConfirmList`. The +existing confirmation flow in each command becomes: + +```go +if len(tasks) > 1 { + fmt.Print(engine.FormatTaskConfirmList(tasks, ws)) + fmt.Printf("Proceed? (y/N): ") + // ... existing confirm logic +} else if len(tasks) == 1 { + // Single task: show summary, no prompt + fmt.Printf("Completing task %s\n", engine.FormatTaskSummary(tasks[0], ws)) +} +``` + +FR-3.3: Single-task operations show the description but skip the y/N prompt. +This is a behavior change for `done` and `delete` which currently always +prompt. `done` only prompts for len > 1 already. `delete` always prompts — +change it to skip for single-task operations matching by display ID. + +#### IMP-6: Consistent no-match errors + +All action commands adopt the same pattern: + +```go +if len(tasks) == 0 { + fmt.Fprintf(os.Stderr, "No tasks matched filter.\n") + os.Exit(1) +} +``` + +Changes needed: +- `done`: change `fmt.Println` to `fmt.Fprintf(os.Stderr, ...)` + `os.Exit(1)` + (currently prints to stdout, exits 0) +- `delete`: same change (currently prints to stdout, exits 0) +- `modify`: already correct (returns error, which prints to stderr and exits 1) +- `start`, `stop`: verify and align + +#### IMP-7: Recurring task feedback + +**Changes to `engine/task.go` — `Complete()` return value:** + +Currently `Complete()` returns `error`. To surface the spawned instance, change +the signature: + +```go +// Complete marks a task as completed. +// Returns the next recurring instance if one was spawned, or nil. +func (t *Task) Complete() (*Task, error) +``` + +`SpawnNextInstance` already creates and saves the next instance — we just need +to return it. The call chain becomes: + +```go +// engine/recurrence.go +func SpawnNextInstance(completedInstance *Task) (*Task, error) +// ^^^^^ return the new instance + +// engine/task.go +func (t *Task) Complete() (*Task, error) { + // ... save completion ... + if t.ParentUUID != nil { + next, err := SpawnNextInstance(t) + return next, err + } + return nil, nil +} +``` + +**Changes to `cmd/done.go`:** + +```go +next, err := task.Complete() +if err != nil { ... } + +fmt.Print(engine.FormatCompletionFeedback(task, displayID, next)) +// Output: +// Completed task 3 — "Weekly review" +// Next instance created — due: 2026-02-25 (in 7 days) +``` + +--- + +### 3. Parser & ID Resolution Fixes (IMP-4, IMP-5) + +#### IMP-4: Fix `delete` display ID resolution + +The fix is minimal. `cmd/delete.go` currently calls `engine.GetTasks(filter)` +directly. Change it to match the pattern in `done.go` and `modify.go`: + +```go +func deleteTasks(args []string) error { + filter, err := engine.ParseFilter(args) + // ... + + // ADD: Load working set (matches done/modify pattern) + ws, err := engine.LoadWorkingSet() + // ... + + var tasks []*engine.Task + if len(filter.IDs) > 0 { + for _, id := range filter.IDs { + task, err := ws.GetTaskByDisplayID(id) + // ... + tasks = append(tasks, task) + } + } else { + tasks, err = engine.GetTasks(filter) + // ... + } + // ... rest unchanged +} +``` + +Note: `Filter.ToSQL()` already has a `working_set` subquery for IDs, so the +current code does technically resolve IDs. But it reads from the `working_set` +table without loading tasks into memory first, meaning the "task not found" +error messages are less helpful. More importantly, aligning the pattern makes +the confirmation display (IMP-3) consistent across commands. + +#### IMP-5: Modifier key allowlist + +**Canonical key set — `engine/keys.go`:** + +Currently, the set of valid modifier keys is implicitly defined in three +places: `dateKeys` (defined twice in `modifier.go`), the `switch` cases for +non-date attributes (`priority`, `project`, `recur`), and the filter parser's +`key:value` handling. These should be consolidated into a single canonical set. + +```go +// engine/keys.go — single source of truth for attribute keys + +// ValidAttributeKeys are the recognized key:value attribute names. +// Used by parseAddArgs, ParseFilter, and ParseModifier. +var ValidAttributeKeys = map[string]bool{ + "due": true, "priority": true, "project": true, + "recur": true, "status": true, "wait": true, + "scheduled": true, "until": true, +} + +// DateKeys is the subset of attribute keys that hold date values. +// Extracted from the duplicate definitions in modifier.go Apply/ApplyToNew. +var DateKeys = map[string]bool{ + "due": true, "scheduled": true, "wait": true, "until": true, +} + +// FilterOnlyKeys are additional keys valid in filter context but not as +// modifiers (e.g., uuid is a filter, not something you set). +var FilterOnlyKeys = map[string]bool{ + "uuid": true, +} +``` + +**Changes to `cmd/add.go` — `parseAddArgs()`:** + +Replace the current `strings.Contains(arg, ":")` heuristic with an allowlist +check using `engine.ValidAttributeKeys`: + +```go +func parseAddArgs(args []string) (string, []string, error) { + var descParts []string + var modifiers []string + + for _, arg := range args { + if strings.HasPrefix(arg, "+") || strings.HasPrefix(arg, "-") { + modifiers = append(modifiers, arg) + continue + } + + // Check for known modifier pattern: key:value where key is recognized + if idx := strings.Index(arg, ":"); idx > 0 { + key := arg[:idx] + if engine.ValidAttributeKeys[key] { + modifiers = append(modifiers, arg) + continue + } + } + + descParts = append(descParts, arg) + } + + if len(descParts) == 0 { + return "", nil, fmt.Errorf("description is required") + } + + return strings.Join(descParts, " "), modifiers, nil +} +``` + +**Changes to `engine/filter.go` — `ParseFilter()`:** + +Same allowlist plus filter-only keys: + +```go +// In the arg loop, replace strings.Contains(arg, ":") with: +if idx := strings.Index(arg, ":"); idx > 0 { + key := arg[:idx] + if ValidAttributeKeys[key] || FilterOnlyKeys[key] { + // existing attribute/uuid handling + } + // else: silently ignore (not an ID, not a tag — treat as harmless) +} +``` + +**Changes to `engine/modifier.go`:** + +Replace the duplicated `dateKeys` maps with the shared `DateKeys`, and use +`ValidAttributeKeys` for validation: + +```go +func (m *Modifier) Apply(task *Task) error { + resolvedDates := make(map[string]time.Time) + // ... init existing dates ... + + for _, key := range m.AttributeOrder { + valuePtr := m.SetAttributes[key] + + if DateKeys[key] { + // ... existing date handling ... + continue + } + + switch key { + case "priority": + // ... + case "project": + // ... + case "recur": + // ... + default: + return fmt.Errorf("unknown modifier key: %q", key) + } + } + // ... +} +``` + +--- + +### 4. Annotations (IMP-11) + +#### Schema + +Add the `annotations` column directly to the `tasks` table definition in +`database.go` migration v1, and update the change_log triggers to include it: + +```sql +-- In the CREATE TABLE tasks statement, add after parent_uuid: +annotations TEXT DEFAULT NULL + +-- In track_task_create and track_task_update triggers, add: +CASE WHEN NEW.annotations IS NOT NULL + THEN 'annotations: ' || NEW.annotations || CHAR(10) ELSE '' END || +``` + +The column stores a JSON array of annotation objects. NULL = no annotations +(avoids storing `"[]"` on every task). + +#### Data Model + +```go +// engine/task.go — add to Task struct +type Annotation struct { + Timestamp int64 `json:"timestamp"` + Text string `json:"text"` +} + +type Task struct { + // ... existing fields ... + Annotations []Annotation `json:"annotations,omitempty"` +} +``` + +#### Engine Interface — `engine/annotate.go` + +```go +// Annotate appends a timestamped annotation to the task. +func (t *Task) Annotate(text string) error + +// Denotate removes the most recent annotation from the task. +func (t *Task) Denotate() (*Annotation, error) +``` + +**`Annotate` behavior:** +1. Append `{timestamp: now, text: text}` to `t.Annotations`. +2. Serialize annotations to JSON. +3. `UPDATE tasks SET annotations = ?, modified = ? WHERE uuid = ?`. + +**`Denotate` behavior:** +1. Pop last element from `t.Annotations`. +2. If empty, set annotations column to NULL. +3. Save. + +#### SQL Persistence + +The `annotations` column is read/written alongside all other task fields in +`GetTask()`, `GetTasks()`, and `Save()`: + +- `Save()` INSERT/UPDATE: add `annotations` parameter (JSON string or NULL). +- `GetTask()` / `GetTasks()`: add `annotations` to SELECT, unmarshal JSON. + +#### Sync + +Annotations sync automatically — they're a column on the `tasks` table, so the +change_log triggers capture them. The sync client's `applyChangeDataToTask()` +needs a new case to parse the `annotations:` key from change log data. + +#### Display + +- `FormatTaskDetail` (info): Add annotations section after tags, showing each + annotation with timestamp. +- `edit` command: Show annotations as editable lines. Format: + ``` + # Annotations (editable — add/remove/modify lines) + annotation.1: 2026-02-18 09:15 | Traced to token expiry in middleware + annotation.2: 2026-02-18 14:30 | note:debug-auth-issue + ``` + On save, diff the annotations before/after to determine adds, removes, and + edits. Timestamps on new annotations are set to now; existing annotations + preserve their original timestamps unless the text is modified. + +#### Jade-depo Linking + +Deferred to a follow-up. The annotation text field supports any string, so +users can manually write `note:my-note-slug` as a convention. A future +`--note` flag can automate this. + +#### Cmd Layer — `cmd/annotate.go` + +```go +var annotateCmd = &cobra.Command{ + Use: "annotate ", + Short: "Add an annotation to a task", +} + +var denotateCmd = &cobra.Command{ + Use: "denotate", + Short: "Remove the most recent annotation", +} +``` + +Both commands follow the standard ID resolution pattern (load working set, +resolve display ID). + +`annotate` and `denotate` must be registered as known commands in +`root.go`'s `commandNames` slice, and the preprocessor must treat `annotate` +as a command with modifiers (the annotation text is the modifier arg). + +--- + +### 5. Relative Dates (IMP-9) + +#### Engine Interface — `engine/reldate.go` + +```go +// FormatRelativeDate returns a human-readable relative date string. +// Within 14 days: "tomorrow", "in 3d", "2d ago", "today" +// Beyond 14 days: "Feb 28", "Mar 15" +func FormatRelativeDate(t time.Time) string + +// FormatDateWithRelative returns "2026-02-20 (in 2 days)" style. +// Used in info/detail views. +func FormatDateWithRelative(t time.Time) string +``` + +#### Changes to `display.go` + +Replace `formatDue()` to use `FormatRelativeDate`: + +```go +func formatDue(due *time.Time) string { + if due == nil { + return "" + } + return FormatRelativeDate(*due) +} +``` + +The color coding (red for overdue, yellow for today) is preserved — applied on +top of the relative string. + +`FormatTaskDetail` uses `FormatDateWithRelative` for the Due, Scheduled, Wait, +and Until fields. + +--- + +### 6. Dry-Run (IMP-10) + +#### Approach + +A `--dry-run` flag on action commands. This is a Cobra persistent flag on the +root command (available to all subcommands). + +```go +// cmd/root.go +var dryRunFlag bool + +func init() { + rootCmd.PersistentFlags().BoolVar(&dryRunFlag, "dry-run", false, + "Show matched tasks without performing the action") +} +``` + +Each action command checks the flag after resolving tasks and before mutating: + +```go +if dryRunFlag { + fmt.Print(engine.FormatTaskConfirmList(tasks, ws)) + fmt.Println("Dry run — no changes made.") + return nil +} +``` + +This reuses the same `FormatTaskConfirmList` from IMP-3. + +--- + +### 7. Task History (IMP-12) + +Task history IS the `change_log`, viewed from the user's perspective. The +`change_log` table already records every mutation with timestamps, change +types, and full task state — there is no separate history storage. + +`opal log` reads directly from `change_log` and presents it in a +human-readable format. `opal info` shows the last few entries as a +"Recent Changes" section. + +The change_log's 30-day retention (configurable via `sync_config`) applies — +history older than the retention window is automatically cleaned up by the +existing `CleanupChangeLog()` function. + +#### Engine Interface — `engine/history.go` + +```go +type HistoryEntry struct { + ID int + Timestamp time.Time + ChangeType string // "create", "update", "delete" + Data string // raw key:value data from change_log +} + +// GetTaskHistory returns change_log entries for a task UUID. +func GetTaskHistory(taskUUID uuid.UUID) ([]HistoryEntry, error) + +// FormatTaskHistory returns a formatted history display. +// Parses the key:value data to show a diff-style view of what changed +// between consecutive entries. +func FormatTaskHistory(entries []HistoryEntry) string +``` + +**`FormatTaskHistory` diff logic:** + +Rather than dumping the raw key:value data (which is the full state at each +point), compare consecutive entries and show only what changed: + +``` +2026-02-18 09:15 created "Buy groceries" priority:D +errand +2026-02-18 10:30 modified priority: D → H +2026-02-18 14:00 modified due: (none) → 2026-02-20 +2026-02-18 16:00 completed +``` + +This requires parsing the `data` field of adjacent entries and diffing. The +`applyChangeDataToTask()` function in `sync/client.go` already knows how to +parse this format — we extract and reuse that parsing logic. + +#### Cmd Layer — `cmd/log.go` + +```go +var logCmd = &cobra.Command{ + Use: "log [filter]", + Short: "Show change history for a task", +} +``` + +Resolves exactly one task (like `info` and `edit`), then calls +`GetTaskHistory(task.UUID)`. + +#### Info Integration + +`FormatTaskDetail` appends a "Recent Changes" section showing the last 5 +history entries (if available). + +--- + +### 8. Shell Completions & Help Text (IMP-8) + +Cobra has built-in completion generation. The main work is registering dynamic +completions for tags and projects, and improving help text discoverability. + +#### Help Text Improvements + +The current `opal --help` lists all commands and reports in a flat list. We +should use Cobra's command grouping to separate them: + +``` +Commands: + add Add a new task + done Mark tasks as completed + modify Modify tasks + delete Delete tasks + start Start working on a task + stop Stop working on a task + edit Edit a task in $EDITOR + info Show task details + annotate Add an annotation to a task + denotate Remove the most recent annotation + undo Undo the last action + uncomplete Restore a completed task to pending + log Show change history for a task + +Reports: + list Pending tasks (default) + next Next tasks to work on + active Currently active tasks + overdue Overdue tasks + ... + +Other: + completion Generate shell completions + version Print version information + server Manage the API server + sync Sync tasks + setup Interactive setup wizard +``` + +Cobra supports this via `AddGroup()` and setting `GroupID` on commands. + +#### Cmd Layer — `cmd/completion.go` + +```go +var completionCmd = &cobra.Command{ + Use: "completion [bash|zsh|fish]", + Short: "Generate shell completions", +} +``` + +Uses `rootCmd.GenBashCompletion()`, `rootCmd.GenZshCompletion()`, etc. + +#### Dynamic Completions + +Register `ValidArgsFunction` on commands that accept tags/projects: + +```go +addCmd.ValidArgsFunction = func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + // Query DB for tag and project names + tags, _ := engine.GetAllTags() + projects, _ := engine.GetAllProjects() + // ... format as "+tag" and "project:name" +} +``` + +This requires the DB to be initialized during completion. Cobra's completion +mechanism calls `PersistentPreRun`, so `initializeApp()` runs and the DB is +available. + +--- + +### 9. Version Command (IMP-13) + +#### Build-time Variables + +```go +// cmd/version.go +var ( + Version = "dev" + Commit = "unknown" + BuildDate = "unknown" +) + +var versionCmd = &cobra.Command{ + Use: "version", + Short: "Print version information", + Run: func(cmd *cobra.Command, args []string) { + fmt.Printf("opal %s (%s) built %s\n", Version, Commit, BuildDate) + }, +} +``` + +Set via `ldflags` in the build: +```sh +go build -ldflags "-X cmd.Version=$(cat VERSION) -X cmd.Commit=$(git rev-parse --short HEAD) -X cmd.BuildDate=$(date -u +%Y-%m-%d)" +``` + +Also register `rootCmd.Version = Version` so `opal --version` works via Cobra. + +--- + +## Schema Changes + +All changes go directly into the existing migration v1 in `database.go`. No +separate migration needed. + +**New column on `tasks`:** +```sql +annotations TEXT DEFAULT NULL +``` + +**New table:** +```sql +CREATE TABLE undo_stack ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + created_at INTEGER NOT NULL, + op_type TEXT NOT NULL, + task_uuid TEXT NOT NULL, + change_log_id INTEGER NOT NULL +); +``` + +**Updated triggers:** The `track_task_create` and `track_task_update` triggers +gain an additional line to capture annotations: +```sql +CASE WHEN NEW.annotations IS NOT NULL + THEN 'annotations: ' || NEW.annotations || CHAR(10) ELSE '' END || +``` + +--- + +## File & Module Changes + +### New Files + +| File | Purpose | IMP | +|------|---------|-----| +| `engine/undo.go` | Undo stack + change_log-based revert logic | 1 | +| `engine/feedback.go` | Shared action output formatting | 2, 3, 7 | +| `engine/reldate.go` | Relative date formatting | 9 | +| `engine/annotate.go` | Annotation CRUD | 11 | +| `engine/history.go` | Task history (reads change_log) | 12 | +| `engine/keys.go` | Canonical `ValidAttributeKeys`, `DateKeys`, `FilterOnlyKeys` | 5 | +| `cmd/undo.go` | `opal undo` command | 1 | +| `cmd/uncomplete.go` | `opal uncomplete` command | 1 | +| `cmd/annotate.go` | `opal annotate` / `denotate` | 11 | +| `cmd/log.go` | `opal log` command | 12 | +| `cmd/completion.go` | Shell completion generation | 8 | +| `cmd/version.go` | Version command | 13 | + +### Modified Files + +| File | Changes | IMP | +|------|---------|-----| +| `engine/database.go` | Add `annotations` column to tasks, add `undo_stack` table, update triggers | 1, 11 | +| `engine/task.go` | `Annotations` field on Task, `Complete()` returns `*Task`, annotations in Save/Get | 7, 11 | +| `engine/display.go` | Use `FormatRelativeDate` in `formatDue()`, add annotations to `FormatTaskDetail`, add recent history | 9, 11, 12 | +| `engine/modifier.go` | Use shared `DateKeys` / `ValidAttributeKeys`, error on unknown keys | 5 | +| `engine/filter.go` | Use shared `ValidAttributeKeys` / `FilterOnlyKeys` for colon parsing | 5 | +| `engine/recurrence.go` | `SpawnNextInstance` returns `(*Task, error)` | 7 | +| `engine/ws.go` | Add `AppendTask()` function | 2 | +| `cmd/root.go` | Register new commands in groups, add `--dry-run` flag, update `commandNames` | 1, 8, 10, 11, 12, 13 | +| `cmd/add.go` | Use `ValidAttributeKeys` in `parseAddArgs()`, call `RecordUndo`, use `FormatAddFeedback` | 1, 2, 5 | +| `cmd/done.go` | Show matched tasks, capture `Complete()` return, `FormatCompletionFeedback`, `RecordUndo`, stderr on no-match | 1, 3, 6, 7 | +| `cmd/delete.go` | Load working set, show matched tasks, `RecordUndo`, stderr on no-match | 1, 3, 4, 6 | +| `cmd/modify.go` | Show matched tasks, `RecordUndo` | 1, 3 | +| `cmd/start.go` | `RecordUndo`, stderr on no-match | 1, 6 | +| `cmd/stop.go` | `RecordUndo`, stderr on no-match | 1, 6 | +| `cmd/info.go` | Show annotations, show recent history | 11, 12 | +| `cmd/edit.go` | Show annotations as editable lines | 11 | +| `internal/sync/client.go` | Parse `annotations:` key in `applyChangeDataToTask()` | 11 | + +### API Changes (for web parity) + +The REST API should also surface annotations: + +| Endpoint | Change | +|----------|--------| +| `GET /tasks`, `GET /tasks/{uuid}` | Include `annotations` in response | +| `PUT /tasks/{uuid}` | Accept `annotations` in request body | +| `POST /tasks/{uuid}/annotate` | New endpoint — add annotation | +| `POST /tasks/{uuid}/denotate` | New endpoint — remove last annotation | +| `POST /tasks/{uuid}/uncomplete` | New endpoint — restore to pending | + +--- + +## Technical Decisions + +### TD-1: Undo via change_log vs. separate snapshot storage + +**Decision:** Use the existing `change_log` as the source of "before" states. +A lightweight `undo_stack` table stores only references (change_log IDs), not +duplicate snapshots. +**Alternatives:** Separate `undo_log` table with full task JSON snapshots. +**Rationale:** The change_log already captures the full task state on every +mutation via SQLite triggers. Duplicating that data in a separate table wastes +space and introduces a risk of the two copies diverging. The undo revert +creates a normal mutation that syncs via change_log triggers, making undo +visible across devices. +**Consequence:** Undo depth is limited by change_log retention (default 30 +days). If retention cleanup removes the "before" entry, that undo entry becomes +unresolvable. The 10-entry undo cap means this is unlikely in practice — 10 +undos within 30 days is well within retention. + +### TD-2: `Complete()` signature change + +**Decision:** Change `Complete()` to return `(*Task, error)`. +**Alternatives:** Add a separate `CompleteAndGetNext()` method. +**Rationale:** Every caller of `Complete()` benefits from knowing about the +spawned instance. Adding a parallel method creates confusion about which to +call. The return value is simply ignored where not needed. +**Consequence:** All callers of `Complete()` must update to handle the new +return type. There are 3 call sites: `cmd/done.go`, `handlers/tasks.go`, and +`sync/client.go`. + +### TD-3: Allowlist vs. escape syntax for colons + +**Decision:** Allowlist of known modifier keys (`ValidAttributeKeys`). +**Alternatives:** Escape syntax (e.g., `\:` or quoting), blocklist of known +non-modifier patterns. +**Rationale:** Allowlist is the simplest correct approach. Escape syntax adds +cognitive load. Blocklist is fragile (can't anticipate all URLs, time formats, +etc.). The set of modifier keys is small and stable. +**Consequence:** Adding a new task attribute in the future requires adding it +to `ValidAttributeKeys`. This is acceptable — new attributes are rare and +require engine changes anyway. + +### TD-4: Annotations as JSON column vs. separate table + +**Decision:** JSON column on `tasks` table. +**Alternatives:** Separate `annotations` table with FK to tasks. +**Rationale:** Benefits: syncs naturally via existing triggers, no joins +needed, annotations are always co-located with their task. Tradeoff: can't +query individual annotations with SQL (searching annotation text requires JSON +parsing). Acceptable for the expected usage pattern (< 10 annotations per +task). + +### TD-5: No migration system — edit schema directly + +**Decision:** All schema changes go directly into the existing migration v1 in +`database.go`. +**Alternatives:** Add a migration v2 with ALTER TABLE / DROP TRIGGER / etc. +**Rationale:** Not in production. No deployed databases need upgrading. Direct +schema editing is simpler and keeps the codebase clean. A migration system can +be added later when production deployment requires it. + +--- + +## Implementation Order + +Suggested order based on dependencies and risk: + +| Phase | Items | Rationale | +|-------|-------|-----------| +| 1 | IMP-4, IMP-6, IMP-5 | Bug fixes first. Low risk. IMP-5 introduces `engine/keys.go` used everywhere. | +| 2 | IMP-13 | Trivial, establishes version for release tracking. | +| 3 | IMP-9, IMP-3, IMP-2, IMP-7 | Output improvements. IMP-3 provides `FormatTaskConfirmList` reused by IMP-10. IMP-7 requires `Complete()` signature change. | +| 4 | IMP-10 | Dry-run, depends on IMP-3's confirmation display. | +| 5 | IMP-1, IMP-11 | Undo + annotations. Both require schema changes (edit v1 together). | +| 6 | IMP-12 | Task history. No schema changes, reads existing change_log. | +| 7 | IMP-8 | Shell completions + help text improvements. Independent, can be done anytime. | + +--- + +## Resolved Design Questions + +1. **Edit command + annotations:** Annotations are **editable** in `opal edit`. + Parsed as `annotation.N: timestamp | text` lines. Diffed on save to detect + adds, removes, and edits. + +2. **Undo across `add` for recurring tasks:** Yes — hard delete both the + template and the first instance. + +3. **Working set append after add:** `AppendTask` assigns `MAX(display_id) + 1`. + This ID is ephemeral — the working set is fully recalculated on every report + render, and display IDs are determined by the report's sort order, not by + insertion order. The appended ID is valid for the immediate "add then act" + workflow and will shift on the next report. This is consistent with existing + behavior — all display IDs are ephemeral by design. diff --git a/docs/design/cli-ux-improvements.md b/docs/design/cli-ux-improvements.md new file mode 100644 index 0000000..2db0417 --- /dev/null +++ b/docs/design/cli-ux-improvements.md @@ -0,0 +1,470 @@ +# Opal CLI: User Experience Improvements + +**Status:** Draft — awaiting feedback +**Date:** 2026-02-18 + +--- + +## Problem Statement + +Opal's CLI is functional and expressive, but several gaps in feedback, safety, +and discoverability make daily use rougher than it needs to be. These +improvements target the person who uses `opal` 10+ times a day — reducing +friction, preventing mistakes, and surfacing information at the right moment. + +--- + +## Proposed Improvements + +### IMP-1: Undo / Uncomplete + +**Priority:** MUST +**Noted in:** `opal-web/BUGS.md` (missing uncomplete feat) + +#### Problem + +Accidentally completing or deleting the wrong task has no quick recovery path. +The only workaround is `opal edit ` and manually setting status back to +`pending`. This is slow, error-prone, and requires knowing the UUID. + +#### User Stories + +**US-1.1** As a user, I want to uncomplete a task so that I can recover from +accidental completions. + +- **Given** task 3 was just completed +- **When** I run `opal 3 uncomplete` (or `opal undo`) +- **Then** task 3 returns to `pending` status, its `end` timestamp is cleared, + and it reappears in my default report + +**US-1.2** As a user, I want a generic `undo` that reverts my last action so +that I don't need to know the exact reverse command. + +- **Given** I just ran `opal 5 delete` +- **When** I run `opal undo` +- **Then** task 5 is restored to its previous status + +#### Functional Requirements + +1. FR-1.1: `opal undo` MUST revert the last mutating CLI action (done, + delete, modify, add, start, stop). +2. FR-1.2: `opal uncomplete` MUST set a completed task back to pending + and clear the `end` timestamp. +3. FR-1.3: Undo history SHOULD persist across CLI invocations (stored in a + local undo log file or DB table). +4. FR-1.4: Undo SHOULD support at least the last 10 operations. +5. FR-1.5: `opal undo` MUST display what was reverted + (e.g., `Undone: task 3 "Buy milk" restored to pending`). + +#### Design Decisions + +- **Scope:** Local only — undo does NOT propagate across sync boundaries. +- **Undo `add`:** Deletes the created task entirely (hard delete, not soft). +- **Multi-level:** `opal undo` can be called repeatedly to walk back through + the last 10 operations (stack-based, LIFO). + +--- + +### IMP-2: Better Feedback After `add` + +**Priority:** MUST + +#### Problem + +`opal add Buy groceries due:tomorrow +errand` prints: + +``` +Created task 8f3a1b2c-4d5e-6f7a-8b9c-0d1e2f3a4b5c +``` + +The UUID is meaningless for subsequent commands. The user can't confirm their +modifiers were parsed correctly without running a separate `list` or `info`. + +#### User Stories + +**US-2.1** As a user, I want to see the display ID and parsed attributes after +adding a task so that I can confirm it was created correctly and reference it +immediately. + +- **Given** I run `opal add Buy groceries due:tomorrow +errand` +- **When** the task is created +- **Then** I see output like: + ``` + Created task 3 — "Buy groceries" + Due: tomorrow (2026-02-19) + Tags: errand + Priority: default + ``` + +#### Functional Requirements + +1. FR-2.1: `add` MUST display the new task's display ID (not just UUID). +2. FR-2.2: `add` MUST echo back all parsed modifiers so the user can verify. +3. FR-2.3: For recurring tasks, `add` MUST show recurrence interval and the + first instance's due date. +4. FR-2.4: The display ID shown MUST be valid for immediate use + (e.g., `opal 3 done`). + +--- + +### IMP-3: Show Matched Tasks in Confirmations + +**Priority:** MUST + +#### Problem + +Destructive commands (`done`, `delete`, `modify`) prompt with only a count: + +``` +About to complete 3 tasks. Proceed? (y/N): +``` + +The user can't verify *which* 3 tasks will be affected without cancelling and +running a separate `list` command with the same filter. + +#### User Stories + +**US-3.1** As a user, I want to see the list of affected tasks before +confirming a bulk action so that I can verify I'm not making a mistake. + +- **Given** I run `opal +errand done` +- **When** 3 tasks match +- **Then** I see: + ``` + About to complete 3 tasks: + 1 Buy groceries due:tomorrow +errand + 4 Return library books due:fri +errand + 7 Pick up dry cleaning +errand + Proceed? (y/N): + ``` + +#### Functional Requirements + +1. FR-3.1: `done`, `delete`, and `modify` MUST list matched tasks (ID, + description, key attributes) before the confirmation prompt. +2. FR-3.2: If more than 10 tasks match, show the first 10 and note + "...and N more". +3. FR-3.3: Single-task operations (e.g., `opal 3 done`) SHOULD still show the + task description for verification but skip the y/N prompt. + +--- + +### IMP-4: Fix `delete` Not Resolving Display IDs + +**Priority:** MUST (bug fix) + +#### Problem + +`delete` calls `engine.GetTasks(filter)` directly without loading the working +set to resolve display IDs. This means `opal 3 delete` may not resolve ID 3 +correctly, unlike `done` and `modify` which both load the working set. + +#### Functional Requirements + +1. FR-4.1: `delete` MUST load the working set and resolve display IDs the same + way `done` and `modify` do. +2. FR-4.2: All action commands (done, delete, modify, start, stop, info, edit) + MUST use the same ID resolution path. + +--- + +### IMP-5: Handle Colons in Descriptions + +**Priority:** MUST + +#### Problem + +`parseAddArgs` treats any argument containing `:` as a modifier. This silently +drops or misparses descriptions containing colons: + +``` +opal add "Meeting: discuss Q3 goals" # "Meeting:" parsed as modifier +opal add Fix bug in http://example.com # "http:" parsed as modifier +``` + +There's no escaping mechanism or error message — the description is silently +truncated. + +#### User Stories + +**US-5.1** As a user, I want to include colons in task descriptions so that I +can write natural language without worrying about parser conflicts. + +- **Given** I run `opal add "Meeting: discuss Q3 goals"` +- **When** the task is created +- **Then** the description is `Meeting: discuss Q3 goals` with no modifiers + +#### Functional Requirements + +1. FR-5.1: Quoted strings MUST be treated as description text, not parsed for + modifiers. +2. FR-5.2: Only tokens matching a known modifier pattern (`key:value` where + `key` is a recognized attribute like `due`, `priority`, `project`, `recur`, + `status`, `wait`, `scheduled`, `until`) SHOULD be treated as modifiers. +3. FR-5.3: Unknown `key:value` patterns SHOULD be treated as description text, + not silently dropped. +4. FR-5.4: If a token is ambiguous, prefer treating it as description text. + +#### Design Decisions + +- **Allowlist approach:** Only recognized attribute keys (`due`, `priority`, + `project`, `recur`, `status`, `wait`, `scheduled`, `until`) are treated as + modifiers. All other `key:value` tokens are treated as description text. + +--- + +### IMP-6: Consistent Error on No-Match + +**Priority:** SHOULD + +#### Problem + +Action commands behave inconsistently when no tasks match a filter: + +| Command | No-match behavior | Exit code | +|----------|-------------------------------------|-----------| +| `done` | Prints "No tasks matched." | 0 | +| `delete` | Prints "No tasks matched." | 0 | +| `modify` | Returns error "no tasks matched" | 1 | +| `start` | (unknown — needs verification) | ? | +| `stop` | (unknown — needs verification) | ? | + +#### Functional Requirements + +1. FR-6.1: All action commands MUST return exit code 1 when no tasks match an + explicit filter. +2. FR-6.2: All action commands MUST print to stderr (not stdout) when no tasks + match, to support scripting. +3. FR-6.3: The message SHOULD be consistent: + `Error: no tasks matched filter ""`. + +--- + +### IMP-7: Recurring Task Feedback + +**Priority:** SHOULD + +#### Problem + +Completing a recurring task instance gives no indication about recurrence: + +``` +$ opal 3 done +Completed 1 task(s). +``` + +The user doesn't know if a next instance was spawned, when it's due, or whether +the recurrence is still active. + +#### User Stories + +**US-7.1** As a user, I want to see recurrence information when completing a +recurring task so that I know the schedule is continuing. + +- **Given** task 3 is a recurring weekly task +- **When** I run `opal 3 done` +- **Then** I see: + ``` + Completed task 3 — "Weekly review" + Next instance created — due: 2026-02-25 (in 7 days) + ``` + +#### Functional Requirements + +1. FR-7.1: Completing a recurring task instance MUST display whether a new + instance was created and its due date. +2. FR-7.2: If no new instance was created (e.g., recurrence was cleared), the + output MUST say so. +3. FR-7.3: `info` on a recurring instance SHOULD show the recurrence pattern + and parent template UUID. + +--- + +### IMP-8: Shell Completions + +**Priority:** SHOULD + +#### Problem + +No tab completion exists for commands, report names, project names, or tag +names. For a CLI with 14 commands, 13 report names, and user-defined projects +and tags, discoverability is poor. + +#### Functional Requirements + +1. FR-8.1: `opal completion bash|zsh|fish` MUST generate shell completion + scripts (cobra has built-in support for this). +2. FR-8.2: Completions SHOULD cover: commands, report names, `+tag` names, + `project:` values, `priority:` values, and `status:` values. +3. FR-8.3: Dynamic completions for tags and projects SHOULD query the database. +4. FR-8.4: Setup instructions SHOULD be printed after `opal completion `. + +--- + +### IMP-9: Relative Dates in CLI Reports + +**Priority:** SHOULD + +#### Problem + +CLI report tables likely show absolute dates (`2026-02-20`). When scanning a +task list, relative dates ("in 2d", "yesterday", "3w ago") are faster to parse +at a glance. The web UI already uses relative dates. + +#### Functional Requirements + +1. FR-9.1: Due dates in reports MUST be shown as relative when within 14 days + (e.g., "tomorrow", "in 3d", "2d ago"). +2. FR-9.2: Dates beyond 14 days SHOULD fall back to short absolute format + (e.g., "Feb 28", "Mar 15"). +3. FR-9.3: `info` SHOULD show both absolute and relative + (e.g., `Due: 2026-02-20 (in 2 days)`). +4. FR-9.4: Relative display COULD be togglable via config + (`date_display: relative|absolute`). + +--- + +### IMP-10: Dry-Run / Preview for Action Commands + +**Priority:** SHOULD + +#### Problem + +Before running `opal +errand done`, the user often runs `opal +errand list` +first to preview. This is a two-step workflow that could be one step. + +#### Functional Requirements + +1. FR-10.1: `done`, `delete`, `modify`, `start`, and `stop` SHOULD support a + `--dry-run` flag that lists matched tasks without acting. +2. FR-10.2: Dry-run output MUST match the same format as the confirmation + listing (IMP-3), followed by "Dry run — no changes made." +3. FR-10.3: Dry-run MUST exit with code 0 if tasks matched, 1 if none matched. + +--- + +### IMP-11: Task Annotations + +**Priority:** SHOULD + +#### Problem + +There's no way to attach notes to a task after creation. For long-running tasks +like "Debug auth issue" or "Research hosting options", users want to record +progress without cluttering the description. + +#### User Stories + +**US-11.1** As a user, I want to annotate tasks with timestamped notes so that +I can track progress and findings over time. + +- **Given** task 3 exists +- **When** I run `opal 3 annotate "Traced to token expiry in middleware"` +- **Then** the annotation is saved with a timestamp +- **And** `opal 3 info` shows the annotation under the task details + +**US-11.2** As a user, I want to link a task to a jade-depo note so that I can +associate detailed research or write-ups with a task. + +- **Given** task 3 exists and a jade-depo note "debug-auth-issue.md" exists +- **When** I run `opal 3 annotate --note debug-auth-issue` (or similar) +- **Then** the task stores a reference to the jade-depo note +- **And** `opal 3 info` shows the linked note path + +#### Functional Requirements + +1. FR-11.1: `opal annotate ""` SHOULD add a timestamped note. +2. FR-11.2: Annotations MUST be visible in `info` and `edit`. +3. FR-11.3: Annotations MUST sync via the existing change log / sync system. +4. FR-11.4: `opal denotate` COULD remove the most recent annotation. +5. FR-11.5: Annotations SHOULD support linking to jade-depo notes (exact + mechanism TBD — flag, URI scheme, or convention like `note:slug`). + +#### Design Decisions + +- **Storage:** JSON text column (`annotations`) on the `tasks` table. Each + annotation is a JSON object with `timestamp` and `text` fields. Stored as a + JSON array, e.g.: + ```json + [ + {"timestamp": 1708300000, "text": "Traced to token expiry in middleware"}, + {"timestamp": 1708310000, "text": "note:debug-auth-issue"} + ] + ``` + This keeps annotations co-located with the task, avoids schema complexity, + and syncs naturally via the existing change_log triggers. + +#### Open Questions + +- Should annotations be searchable via filters (e.g., `opal annotation:token list`)? +- Jade-depo integration: should `opal 3 annotate --note ` verify the + note exists in jade-depo, or just store the reference loosely? Loose coupling + is simpler but can lead to stale links. + +--- + +### IMP-12: Task History + +**Priority:** COULD + +#### Problem + +The `change_log` table records every mutation for sync, but there's no +user-facing way to view a task's history. Useful for understanding what changed, +when, and debugging unexpected state. + +#### Functional Requirements + +1. FR-12.1: `opal <id> log` COULD display the change history for a task. +2. FR-12.2: Output SHOULD show timestamp, change type, and what changed: + ``` + 2026-02-18 09:15 created "Buy groceries" priority:D + 2026-02-18 10:30 modified priority: D → H + 2026-02-18 14:00 completed + ``` +3. FR-12.3: History MUST respect the existing change_log retention policy. +4. FR-12.4: History SHOULD be surfaced in `info` output (recent changes + section) and available in `edit` as read-only comment lines. + +--- + +### IMP-13: Version Command + +**Priority:** COULD + +#### Functional Requirements + +1. FR-13.1: `opal version` (or `opal --version`) MUST print the build version. +2. FR-13.2: Version SHOULD be set at build time via `ldflags`, reading from a + `VERSION` file in the repo root. +3. FR-13.3: Output SHOULD include version, commit hash, and build date. + +--- + +## Out of Scope + +- **GUI/TUI redesign** — this document covers CLI UX only. +- **New task attributes** (e.g., estimated effort, dependencies between tasks). +- **Multi-user features** — opal is a personal/household tool. +- **Plugin/hook system** — not needed at this stage. +- **Web UI changes** — covered separately in `opal-web/REQUIREMENTS.md`. + +--- + +## Priority Summary + +| ID | Improvement | Priority | Effort | +|--------|--------------------------------------|----------|----------| +| IMP-1 | Undo / uncomplete | MUST | Medium | +| IMP-2 | Better `add` feedback | MUST | Low | +| IMP-3 | Show matched tasks in confirmations | MUST | Low | +| IMP-4 | Fix `delete` display ID resolution | MUST | Low | +| IMP-5 | Handle colons in descriptions | MUST | Medium | +| IMP-6 | Consistent no-match error behavior | SHOULD | Low | +| IMP-7 | Recurring task feedback | SHOULD | Low | +| IMP-8 | Shell completions | SHOULD | Medium | +| IMP-9 | Relative dates in CLI reports | SHOULD | Low | +| IMP-10 | Dry-run flag for actions | SHOULD | Low | +| IMP-11 | Task annotations | SHOULD | Medium | +| IMP-12 | Task history | COULD | Medium | +| IMP-13 | Version command | COULD | Trivial | diff --git a/opal-task/cmd/add.go b/opal-task/cmd/add.go index 607e16a..25d8ee3 100644 --- a/opal-task/cmd/add.go +++ b/opal-task/cmd/add.go @@ -71,23 +71,29 @@ func addTask(args []string) error { return nil } -// parseAddArgs extracts description and modifiers from args -// Description = all non-filter/modifier words joined with spaces -// Filters/Modifiers = args with +, -, or containing : +// parseAddArgs extracts description and modifiers from args. +// Tags (+tag, -tag) are always modifiers. For key:value tokens, only +// recognized attribute keys (engine.ValidAttributeKeys) are treated as +// modifiers — everything else becomes part of the description. func parseAddArgs(args []string) (string, []string, error) { var descParts []string var modifiers []string for _, arg := range args { - isFilterOrModifier := strings.HasPrefix(arg, "+") || - strings.HasPrefix(arg, "-") || - strings.Contains(arg, ":") - - if isFilterOrModifier { + if strings.HasPrefix(arg, "+") || strings.HasPrefix(arg, "-") { modifiers = append(modifiers, arg) - } else { - descParts = append(descParts, arg) + continue } + + if idx := strings.Index(arg, ":"); idx > 0 { + key := arg[:idx] + if engine.ValidAttributeKeys[key] { + modifiers = append(modifiers, arg) + continue + } + } + + descParts = append(descParts, arg) } if len(descParts) == 0 { diff --git a/opal-task/cmd/delete.go b/opal-task/cmd/delete.go index f89c4b6..d08130a 100644 --- a/opal-task/cmd/delete.go +++ b/opal-task/cmd/delete.go @@ -26,14 +26,31 @@ func deleteTasks(args []string) error { return err } - tasks, err := engine.GetTasks(filter) + // Load working set to resolve display IDs (matches done/modify pattern) + ws, err := engine.LoadWorkingSet() if err != nil { - return err + return fmt.Errorf("failed to load working set: %w", err) + } + + var tasks []*engine.Task + + if len(filter.IDs) > 0 { + for _, id := range filter.IDs { + task, err := ws.GetTaskByDisplayID(id) + if err != nil { + return err + } + tasks = append(tasks, task) + } + } else { + tasks, err = engine.GetTasks(filter) + if err != nil { + return err + } } if len(tasks) == 0 { - fmt.Println("No tasks matched.") - return nil + return fmt.Errorf("no tasks matched filter") } fmt.Printf("Delete %d task(s)? (y/N): ", len(tasks)) diff --git a/opal-task/cmd/done.go b/opal-task/cmd/done.go index e1ca8fb..beb251e 100644 --- a/opal-task/cmd/done.go +++ b/opal-task/cmd/done.go @@ -63,8 +63,7 @@ func completeTasks(args []string) error { } if len(tasks) == 0 { - fmt.Println("No tasks matched.") - return nil + return fmt.Errorf("no tasks matched filter") } // Confirm if multiple tasks diff --git a/opal-task/cmd/modify.go b/opal-task/cmd/modify.go index c001f95..6ed1a7c 100644 --- a/opal-task/cmd/modify.go +++ b/opal-task/cmd/modify.go @@ -82,7 +82,7 @@ func modifyTasks(filterArgs, modifierArgs []string) error { } if len(tasks) == 0 { - return fmt.Errorf("no tasks matched") + return fmt.Errorf("no tasks matched filter") } // Confirm if multiple tasks or no filters specified diff --git a/opal-task/cmd/start.go b/opal-task/cmd/start.go index 69bbd1a..9f08a74 100644 --- a/opal-task/cmd/start.go +++ b/opal-task/cmd/start.go @@ -43,6 +43,10 @@ func startTasks(args []string) error { } } + if len(tasks) == 0 { + return fmt.Errorf("no tasks matched filter") + } + for _, task := range tasks { task.StartTask() fmt.Printf("Started task: %s\n", task.Description) diff --git a/opal-task/cmd/stop.go b/opal-task/cmd/stop.go index ed44da2..f7bfb51 100644 --- a/opal-task/cmd/stop.go +++ b/opal-task/cmd/stop.go @@ -43,6 +43,10 @@ func stopTasks(args []string) error { } } + if len(tasks) == 0 { + return fmt.Errorf("no tasks matched filter") + } + for _, task := range tasks { task.StopTask() fmt.Printf("Stopped task: %s\n", task.Description) diff --git a/opal-task/internal/engine/filter.go b/opal-task/internal/engine/filter.go index eaf62ab..2737528 100644 --- a/opal-task/internal/engine/filter.go +++ b/opal-task/internal/engine/filter.go @@ -42,17 +42,18 @@ func ParseFilter(args []string) (*Filter, error) { } else if strings.HasPrefix(arg, "-") && !strings.Contains(arg, ":") { // Exclude tag (but not negative modifiers like priority:-) f.ExcludeTags = append(f.ExcludeTags, strings.TrimPrefix(arg, "-")) - } else if strings.Contains(arg, ":") { - // Attribute filter - parts := strings.SplitN(arg, ":", 2) - key := parts[0] - value := parts[1] + } else if idx := strings.Index(arg, ":"); idx > 0 { + key := arg[:idx] + value := arg[idx+1:] - if key == "uuid" { + if FilterOnlyKeys[key] { + // Filter-only keys (e.g., uuid) f.UUIDs = append(f.UUIDs, value) - } else { + } else if ValidAttributeKeys[key] { + // Known attribute filter f.Attributes[key] = value } + // Unrecognized key:value tokens are silently ignored } else { // Try parsing as numeric ID id, err := strconv.Atoi(arg) diff --git a/opal-task/internal/engine/keys.go b/opal-task/internal/engine/keys.go new file mode 100644 index 0000000..e890eb9 --- /dev/null +++ b/opal-task/internal/engine/keys.go @@ -0,0 +1,30 @@ +package engine + +// ValidAttributeKeys are the recognized key:value attribute names. +// Used by parseAddArgs (cmd/add.go), ParseFilter, and ParseModifier +// to distinguish modifiers from description text. +var ValidAttributeKeys = map[string]bool{ + "due": true, + "priority": true, + "project": true, + "recur": true, + "status": true, + "wait": true, + "scheduled": true, + "until": true, +} + +// DateKeys is the subset of ValidAttributeKeys that hold date values. +// Used by Modifier.Apply and Modifier.ApplyToNew for date parsing. +var DateKeys = map[string]bool{ + "due": true, + "scheduled": true, + "wait": true, + "until": true, +} + +// FilterOnlyKeys are additional keys valid in filter context but not as +// modifiers (e.g., uuid is a filter target, not something you set via modify). +var FilterOnlyKeys = map[string]bool{ + "uuid": true, +} diff --git a/opal-task/internal/engine/modifier.go b/opal-task/internal/engine/modifier.go index c4f1548..c98f805 100644 --- a/opal-task/internal/engine/modifier.go +++ b/opal-task/internal/engine/modifier.go @@ -23,7 +23,9 @@ func NewModifier() *Modifier { } } -// ParseModifier parses command-line args into Modifier +// ParseModifier parses command-line args into Modifier. +// Only recognized attribute keys (ValidAttributeKeys) are accepted; +// unrecognized key:value tokens produce an error. func ParseModifier(args []string) (*Modifier, error) { m := NewModifier() @@ -34,11 +36,13 @@ func ParseModifier(args []string) (*Modifier, error) { } else if strings.HasPrefix(arg, "-") && !strings.Contains(arg, ":") { // Remove tag m.RemoveTags = append(m.RemoveTags, strings.TrimPrefix(arg, "-")) - } else if strings.Contains(arg, ":") { - // Attribute modification - parts := strings.SplitN(arg, ":", 2) - key := parts[0] - value := parts[1] + } else if idx := strings.Index(arg, ":"); idx > 0 { + key := arg[:idx] + value := arg[idx+1:] + + if !ValidAttributeKeys[key] { + return nil, fmt.Errorf("unknown modifier: %q (known: due, priority, project, recur, status, wait, scheduled, until)", key) + } if value == "" { // Clear attribute (priority: with no value) @@ -83,7 +87,7 @@ func (m *Modifier) Apply(task *Task) error { resolvedDates["modified"] = task.Modified // Apply attributes in the order they were specified (important for relative references) - dateKeys := map[string]bool{"due": true, "scheduled": true, "wait": true, "until": true} + dateKeys := DateKeys for _, key := range m.AttributeOrder { valuePtr := m.SetAttributes[key] @@ -151,7 +155,7 @@ func (m *Modifier) ApplyToNew(task *Task) error { } // Apply attributes in the order they were specified (important for relative references) - dateKeys := map[string]bool{"due": true, "scheduled": true, "wait": true, "until": true} + dateKeys := DateKeys for _, key := range m.AttributeOrder { valuePtr := m.SetAttributes[key]