From 30c9b2c24acf747e8be097afaa35a314ec3e5a2b Mon Sep 17 00:00:00 2001 From: Sahil Ahuja Date: Mon, 22 Jun 2026 14:30:13 +0530 Subject: [PATCH 1/4] =?UTF-8?q?fix:=20DX=20copy=20polish=20=E2=80=94=20wt?= =?UTF-8?q?=20go=20stderr=20nav-confirmation,=20unified=20wt.Warn=20helper?= =?UTF-8?q?,=20list=20--path=20structured=20not-found,=20update=20Short=20?= =?UTF-8?q?capitalization,=20help-text=20+=20glyph=20fixes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/memory/wt-cli/create-output-phases.md | 29 ++- docs/memory/wt-cli/go-command-contract.md | 91 ++++++-- docs/memory/wt-cli/list-status-contract.md | 2 + .../260622-log5-dx-copy-polish/.history.jsonl | 10 + .../260622-log5-dx-copy-polish/.status.yaml | 50 ++++ .../260622-log5-dx-copy-polish/intake.md | 215 ++++++++++++++++++ .../260622-log5-dx-copy-polish/plan.md | 203 +++++++++++++++++ src/cmd/wt/create.go | 19 +- src/cmd/wt/delete.go | 22 +- src/cmd/wt/delete_test.go | 22 ++ src/cmd/wt/go.go | 18 +- src/cmd/wt/go_test.go | 44 ++++ src/cmd/wt/list.go | 6 +- src/cmd/wt/list_test.go | 12 +- src/cmd/wt/update.go | 2 +- src/internal/worktree/errors.go | 8 + src/internal/worktree/errors_test.go | 36 +++ 17 files changed, 743 insertions(+), 46 deletions(-) create mode 100644 fab/changes/260622-log5-dx-copy-polish/.history.jsonl create mode 100644 fab/changes/260622-log5-dx-copy-polish/.status.yaml create mode 100644 fab/changes/260622-log5-dx-copy-polish/intake.md create mode 100644 fab/changes/260622-log5-dx-copy-polish/plan.md diff --git a/docs/memory/wt-cli/create-output-phases.md b/docs/memory/wt-cli/create-output-phases.md index 0aaa5fd..27c95da 100644 --- a/docs/memory/wt-cli/create-output-phases.md +++ b/docs/memory/wt-cli/create-output-phases.md @@ -5,9 +5,10 @@ description: "Phase-separator output contract for `wt create` / `wt init` — Gi # wt-cli: Create Output Phases Contract > Post-implementation behavior capture for the `wt create` / `wt init` phase-separator output. -> Source change: `260531-pnmi-add-phase-separators`. +> Source change: `260531-pnmi-add-phase-separators` +> (single `wt.Warn` warning emitter + `wt delete` stdout→stderr warning realignment added by `260622-log5-dx-copy-polish`). -This file documents the phase-separator output contract that `wt create` and `wt init` honor. Future changes touching `src/cmd/wt/create.go`, `src/cmd/wt/init.go`, or `src/internal/worktree/{crud.go,errors.go}` should preserve these invariants unless an explicit spec amendment supersedes them. +This file documents the phase-separator output contract that `wt create` and `wt init` honor, and — as the canonical stdout/stderr stream-discipline file — the single `wt.Warn` warning emitter that all `Warning:` call sites route through. Future changes touching `src/cmd/wt/create.go`, `src/cmd/wt/init.go`, `src/cmd/wt/delete.go`, or `src/internal/worktree/{crud.go,errors.go}` should preserve these invariants unless an explicit spec amendment supersedes them. ## Requirements @@ -51,6 +52,15 @@ This file documents the phase-separator output contract that `wt create` and `wt - `wt init` was **realigned** so ALL its init diagnostics go to stderr: the `Running worktree init...` / `Worktree init complete.` banner (now `fmt.Fprintln(os.Stderr, ...)`), the Init separator, AND the init script's own stdout (the init child is wired with `cmd.Stdout = os.Stderr` at `init.go:83`, joining `cmd.Stderr = os.Stderr`). `wt init` has **no machine-readable stdout result** — it is a side-effecting command whose outcome is its exit code — so all of its output is diagnostic. - The realignment changed only the stream (stdout → stderr) for `wt init`: exit-code behavior (`ExitInitFailed = 7` on script non-zero exit), graceful-skip behavior, and all output text are unchanged. Existing `wt init` tests assert on the combined `r.Stdout + r.Stderr`, so they stay green. +### `wt.Warn` is the single warning emitter; warnings are stderr-only (`260622-log5`) + +- `src/internal/worktree/errors.go` exposes `func Warn(format string, args ...any)` alongside the other output helpers (`WtError`, `PrintError`, `PhaseSeparator`, `PrintInitFailureBanner`). It is the **single** `Warning:`-diagnostic producer for the CLI — it writes `"%sWarning:%s %s\n"` (`ColorYellow` prefix, `ColorReset`, then the formatted message) to **stderr**. +- It is the warnings analogue of `PhaseSeparator`: color detection reuses the **blanked package-level color vars** (the `NO_COLOR`-blanking `init()` zeroes `ColorYellow`/`ColorReset`), so under `NO_COLOR` it emits a plain `Warning: \n` with **no** ANSI escapes. It MUST NOT call `os.Getenv` afresh. +- **All `Warning:` call sites route through `Warn`** — `create.go` (the uncommitted-changes warning and the open-phase "could not open in …" / reused-worktree-init warnings) and `delete.go` (the two `failed to remove …` warnings and the two pre-menu warnings). Call sites pass the **message text only**, never the literal `Warning: ` prefix — the helper owns the prefix, so re-including it would double it. +- **`wt delete`'s two pre-menu warnings were realigned stdout → stderr.** The uncommitted-changes warning (`handleUncommittedChanges`) and the unpushed-commits warning (`handleUnpushedCommits`) previously printed via `fmt.Printf` (**stdout**); they now route through `wt.Warn` (→ **stderr**), matching the sibling warnings already on stderr in the same file. This is the warnings counterpart of the `wt init` stdout→stderr realignment above — same stream-discipline fix, different command. `wt delete` has no stdout machine contract, but warnings are diagnostics and belong on stderr. +- **Menu-layout spacing is preserved at the call site, not in the helper.** `Warn` appends exactly one trailing `\n`. The two pre-menu warnings need a blank line before and after for menu spacing, so each call site frames the `wt.Warn(...)` with a bare `fmt.Fprintln(os.Stderr)` immediately before and after (reproducing the old `\n…\n\n` framing now that the whole block is on stderr). +- Each warning's **existing wording is preserved** — the consolidation standardizes only the prefix, stream, and color, never the message text. + ## Design Decisions ### Single helper in `errors.go` as the sole separator producer @@ -65,6 +75,13 @@ The runner already prints `Running worktree init...` and already holds the resol The convention captured by this change: **stdout = machine-readable result; stderr = diagnostics** (progress lines, banners, separators, prompts). `wt create` already followed it (stdout = the path line only). `wt init` used stdout for identical init diagnostics, an asymmetry that made separator placement inconsistent. Since `wt init` has no stdout contract (its tests assert on combined streams and no spec pins it to stdout), realigning it to stderr resolves the inconsistency and makes separator placement uniform across both commands. Keeping each command on its own stream was rejected — it enshrines the inconsistency. +### Single `wt.Warn` helper in `errors.go`, not a `cmd/`-local emitter (`260622-log5`) + +**Decision**: a single `Warn(format, args...)` lives in `internal/worktree/errors.go` next to `PhaseSeparator`/`WtError`/`PrintInitFailureBanner`, and every `Warning:` call site in `create.go`/`delete.go` routes through it; the documentation home is this stream-discipline file (a note here), **not** a new dedicated memory file. +**Why**: `errors.go` already centralizes user-facing output formatting and owns the `NO_COLOR`-blanking `init()`, so `Warn` reuses that mechanism and collapses ~10 divergent call sites to one idiom (DRY, and keeps `cmd/` thin per Constitution V) — exactly the rationale that put `PhaseSeparator` here. The warning realignment is the *same* stdout=machine / stderr=human convention this file already canonicalizes (see the `wt init` realignment above), so it belongs as a note here rather than fragmenting the stream-discipline story across two files. A standalone `warning-output-contract.md` would be the smallest file in the bundle for one helper + a two-call-site realignment, against the per-command / per-shared-concern granularity of the sibling files (none of `errors.go`'s other emitters got their own file). +**Rejected**: a `cmd/`-local warning helper (pushes formatting into the thin command layer, can't reuse the package color vars); a dedicated `warning-output-contract.md` memory file (over-granular for a copy-polish change; splits the one stream-discipline contract in two). +*Introduced by*: `260622-log5-dx-copy-polish`. + ### Fixed 40-column width, no terminal-size query Deterministic output for unit tests; no dependency on terminal/ioctl; consistent with the single-binary, no-hidden-state posture (Constitution I). Dynamic width via terminal detection was rejected — non-deterministic in tests and adds a platform-specific code path for a cosmetic gain. (This mirrors `wt list`'s flag-based, no-`isatty` posture; see `list-status-contract.md`.) @@ -72,7 +89,7 @@ Deterministic output for unit tests; no dependency on terminal/ioctl; consistent ## Cross-references - Spec doc: none — phase-separator output is per-command diagnostic structure, not part of `docs/specs/cli-surface.md`'s per-subcommand flag surface. This contract lives in memory only. -- Source: `src/internal/worktree/errors.go` (`PhaseSeparator`, `phaseSeparatorWidth`, the `NO_COLOR`-blanking `init()`), `src/internal/worktree/crud.go` (`RunWorktreeSetupWithObserver` Init separator emission), `src/cmd/wt/create.go` (Git + Open separators, stdout path line), `src/cmd/wt/init.go` (`runInitScript` Init separator + stderr realignment). -- Tests: `src/internal/worktree/errors_test.go` (`PhaseSeparator` unit test: label presence, colored ANSI + `─`, NO_COLOR ASCII `-` with no `\033[`, 40-column visible width, no trailing newline); `src/cmd/wt/create_test.go` (`Created worktree:` stderr + one-line-stdout guard); `src/cmd/wt/init_test.go` (combined-stream `Running worktree init` / `Worktree init complete`). -- Constitution: Principle I (Single-Binary CLI, No Hidden State — motivated the fixed width, no terminal query), Principle VI (Interactive by Default, Scriptable on Demand — stdout=machine-result keeps `wt create`'s path line deterministic for launchers/operators). -- Sibling memory: `wt-cli/init-failure-contract.md` — the init runner / resolver / `*InitNotFound` contract this change builds on (the Init separator sits next to the `Running worktree init` line and respects the not-found and reinstall-window invariants). `wt-cli/list-status-contract.md` and `wt-cli/menu-navigation-contract.md` — same post-change invariant-capture pattern for sibling `wt` subcommands. +- Source: `src/internal/worktree/errors.go` (`PhaseSeparator`, `phaseSeparatorWidth`, `Warn`, the `NO_COLOR`-blanking `init()`), `src/internal/worktree/crud.go` (`RunWorktreeSetupWithObserver` Init separator emission), `src/cmd/wt/create.go` (Git + Open separators, stdout path line, `wt.Warn` call sites), `src/cmd/wt/init.go` (`runInitScript` Init separator + stderr realignment), `src/cmd/wt/delete.go` (`wt.Warn` call sites — incl. the two pre-menu warnings realigned stdout→stderr with `fmt.Fprintln(os.Stderr)` spacing framing). +- Tests: `src/internal/worktree/errors_test.go` (`PhaseSeparator` unit test: label presence, colored ANSI + `─`, NO_COLOR ASCII `-` with no `\033[`, 40-column visible width, no trailing newline; `260622-log5`: `Warn` unit test — color-wrapped `Warning:` to stderr when colored, plain `Warning: ` with no ANSI under blanked color vars); `src/cmd/wt/create_test.go` (`Created worktree:` stderr + one-line-stdout guard); `src/cmd/wt/init_test.go` (combined-stream `Running worktree init` / `Worktree init complete`); `src/cmd/wt/delete_test.go` (`260622-log5`: the uncommitted-changes / unpushed-commits warnings appear on stderr, not stdout). +- Constitution: Principle I (Single-Binary CLI, No Hidden State — motivated the fixed width, no terminal query), Principle V (Internal Package Boundary — `Warn` lives in `internal/worktree`, keeping `cmd/` thin), Principle VI (Interactive by Default, Scriptable on Demand — stdout=machine-result keeps `wt create`'s path line deterministic for launchers/operators). +- Sibling memory: `wt-cli/init-failure-contract.md` — the init runner / resolver / `*InitNotFound` contract this change builds on (the Init separator sits next to the `Running worktree init` line and respects the not-found and reinstall-window invariants). `/wt-cli/go-command-contract.md` — `wt go`'s stderr navigation confirmation (`260622-log5`) honors this same stdout=machine / stderr=human stream discipline. `wt-cli/list-status-contract.md` and `wt-cli/menu-navigation-contract.md` — same post-change invariant-capture pattern for sibling `wt` subcommands. diff --git a/docs/memory/wt-cli/go-command-contract.md b/docs/memory/wt-cli/go-command-contract.md index bbed2b8..c3e9a80 100644 --- a/docs/memory/wt-cli/go-command-contract.md +++ b/docs/memory/wt-cli/go-command-contract.md @@ -6,7 +6,8 @@ description: "`wt go` worktree-selection contract — selection-only navigation > Post-implementation behavior capture for the `wt go` worktree-selection verb > and the `wt open --go` select-then-launch composition. -> Source change: `260620-3pp5-open-worktree-from-worktree`. +> Source change: `260620-3pp5-open-worktree-from-worktree` +> (stderr navigation confirmation added by `260622-log5-dx-copy-polish`). This file documents the contract `wt go` honors and how `wt open --go` composes it. `wt go` is the **selector** half of the selector/launcher split: it picks a @@ -37,8 +38,9 @@ explicit spec amendment supersedes them. case-insensitively via the **shared** `resolveWorktreeByName` (the same resolver `wt open ` uses, in `src/cmd/wt/open.go`) and navigates there. It launches **no** application — navigation is the only effect. -- On success it routes through `navigateTo(path)` (below): writes the resolved - absolute path to `WT_CD_FILE` when set AND prints it to stdout as the last line. +- On success it routes through `navigateTo(ctx, path)` (below): writes the resolved + absolute path to `WT_CD_FILE` when set AND prints it to stdout as the last line, + and emits a stderr navigation confirmation (see the navigation section below). ### `wt go` (no arg) shows the current-repo selection menu from anywhere in the repo @@ -92,14 +94,19 @@ explicit spec amendment supersedes them. ### `wt go` navigates via `WT_CD_FILE` + stdout — `navigateTo`, never `OpenInApp` -- `navigateTo(path string)` in `src/cmd/wt/go.go` is the dedicated navigation - helper. It: - 1. Writes `path` to `WT_CD_FILE` (mode `0600`, truncate-on-write) when the env +- `navigateTo(ctx *wt.RepoContext, path string)` in `src/cmd/wt/go.go` is the + dedicated navigation helper (it takes `ctx` so it can render `ctx.RepoName` in the + stderr confirmation below — `260622-log5` widened the signature from the original + `navigateTo(path string)`; both call sites, the by-name path and the menu path, + pass the resolved `ctx`). It: + 1. Emits a **stderr navigation confirmation** (see the dedicated requirement + below) — the first thing it writes, on the success path only. + 2. Writes `path` to `WT_CD_FILE` (mode `0600`, truncate-on-write) when the env var is set — the identical semantics `launcher-contract.md` §3 fixes for "Open here". A write failure exits `ExitGeneralError`. - 2. **Always** prints the resolved absolute path to stdout as the **last line**, + 3. **Always** prints the resolved absolute path to stdout as the **last line**, so the no-wrapper scripting form `cd "$(command wt go some-name)"` works. - 3. When `WT_CD_FILE` is unset **and** `WT_WRAPPER != "1"`, prints the same + 4. When `WT_CD_FILE` is unset **and** `WT_WRAPPER != "1"`, prints the same two-line "shell wrapper required / `eval "$(wt shell-init)"`" hint to stderr that the launcher's "Open here" emits (the `WT_WRAPPER`-gated hint convention, `launcher-contract.md` §4). @@ -116,6 +123,38 @@ explicit spec amendment supersedes them. verbatim, so the launcher-contract stability guarantees (§6) are unchanged and no constitution amendment is required. +### `wt go` emits a stderr navigation confirmation on success — stdout stays the bare path (`260622-log5`) + +- On the **success path only** (a worktree was resolved by name OR selected from + the menu, i.e. inside `navigateTo`), `wt go` writes a two-line compact-arrow + confirmation to **stderr** so the user can see *where* they are landing: + ``` + → idea / frosted-jaguar (feature-x) + /home/sahil/code/sahil87/idea.worktrees/frosted-jaguar + ``` + - Line 1: `→ {ctx.RepoName} / {filepath.Base(path)} ({branch})` — two spaces + before the parenthesized branch. + - Line 2: the absolute resolved `path`, **two-space-indented**. +- The branch is derived via the existing `getBranchForPath(path)` (in + `src/cmd/wt/open.go`) — the **same** single `git rev-parse --abbrev-ref HEAD` the + open/go menus already run per entry; `wt go` reuses it rather than issuing a + fresh git call (Constitution V). +- **The confirmation is success-path-only.** It is emitted from inside + `navigateTo`, so it never fires on the cancel path (`Cancelled.`) or the + no-worktrees path (`No worktrees found.`) — those return before `navigateTo` is + reached. +- **stdout is unchanged — NON-NEGOTIABLE.** The confirmation is diagnostic copy on + stderr; stdout stays **exactly** the single bare resolved absolute path as the + final (and only) stdout line. This preserves `cd "$(command wt go )"` and + the `WT_CD_FILE` write (mode `0600`, truncate-on-write). The confirmation is the + *first* thing `navigateTo` writes and the bare-path `fmt.Println(path)` stays the + *last* write, so no confirmation text can leak onto stdout. +- **No color.** The confirmation is emitted as plain text (no `ColorYellow`/etc.), + so it is NO_COLOR-safe **by construction** — it never touches the package color + vars and never calls `os.Getenv` afresh. (This is a deliberately simpler posture + than `PhaseSeparator`'s color-detection-via-blanked-vars; `wt go`'s confirmation + has no colored variant.) + ### `wt go` no-arg under `--non-interactive` / non-TTY is deterministic and non-prompting - `wt go` accepts a `--non-interactive` bool flag (Constitution VI). @@ -199,8 +238,8 @@ explicit spec amendment supersedes them. ### `wt go` writes `WT_CD_FILE` + prints stdout via a dedicated `navigateTo`, not `OpenInApp` -**Decision**: a small `navigateTo` helper in `cmd/wt/go.go` does both the -`WT_CD_FILE` write (when set) and the always-print-bare-path stdout emission. +**Decision**: a small `navigateTo(ctx, path)` helper in `cmd/wt/go.go` does both +the `WT_CD_FILE` write (when set) and the always-print-bare-path stdout emission. **Why**: `wt go` is a navigation verb with no app concept, and it must do BOTH sides of the cd contract (write the file when set AND always print the bare path for `cd "$(command wt go)"`). Constitution VII + `launcher-contract.md` §3 fix @@ -210,6 +249,26 @@ wrong shape (it writes `WT_CD_FILE` OR prints a `cd --` line, mutually exclusive and never emits a bare path), and it couples `go` to the launcher app catalog. *Introduced by*: `260620-3pp5-open-worktree-from-worktree`. +### Navigation confirmation on stderr, never stdout (`260622-log5`) + +**Decision**: the compact-arrow `→ {repo} / {worktree} ({branch})` + indented-path +confirmation goes to **stderr**, emitted from inside `navigateTo` (success path +only); stdout stays the bare resolved path as the final line. `navigateTo` was +widened to `navigateTo(ctx, path)` so it has `ctx.RepoName`, and the branch reuses +`getBranchForPath`. +**Why**: the governing stream-discipline rule — stdout = machine result, stderr = +human copy (see `/wt-cli/create-output-phases.md`) — and the `cd "$(command wt go)"` +/ `WT_CD_FILE` scripting contract both require a bare-path stdout. Putting the +confirmation on stderr closes the `wt go` reassurance gap (it previously printed +only a bare path, less informative than `wt create`'s summary) without touching the +machine contract. Plain text (no color) keeps it NO_COLOR-safe by construction and +matches the create-summary's modest styling. +**Rejected**: printing the block to stdout (breaks every scripting/launcher +consumer); a create-style multi-line labeled block or a single dense line (the +compact-arrow form was the user's explicit choice over both); a fresh +`git rev-parse` for the branch (reuse `getBranchForPath` per Constitution V). +*Introduced by*: `260622-log5-dx-copy-polish`. + ### Shared `selectWorktree(ctx, session, prompt)` returning `(path, name, cancelled, noWorktrees, err)` **Decision**: extract the menu logic from `selectAndOpen` into one helper that @@ -278,9 +337,15 @@ otherwise does not want). - Source: `src/cmd/wt/main.go` — `goCmd()` registered in `root.AddCommand(...)`. - Tests: `src/cmd/wt/go_test.go` (unit: name happy path → `WT_CD_FILE`+stdout, unknown name → exit 1, non-git → exit 3, no-arg `--non-interactive` → exit 1 - without prompting), `src/cmd/wt/integration_test.go` (end-to-end `wt go ` - from a sibling worktree, no-arg menu newest-first ordering, - `wt open --go --app open_here`). + without prompting; `260622-log5`: success emits the `→`/repo/worktree/branch + + indented-path confirmation on stderr while stdout stays exactly the bare path, + and the confirmation is absent on the no-worktrees menu path), + `src/cmd/wt/integration_test.go` (end-to-end `wt go ` from a sibling + worktree, no-arg menu newest-first ordering, `wt open --go --app open_here`). +- Sibling memory: `/wt-cli/create-output-phases.md` — the canonical stdout = + machine-result / stderr = human-diagnostic stream-discipline contract this + confirmation honors; it also documents the `wt.Warn` single warning emitter + (`260622-log5`). - Constitution: Principle II (Cobra command surface — `SilenceUsage`/ `SilenceErrors`, `RunE`), III (Typed exit codes — `ExitGitError` / `ExitGeneralError`, no new code), V (selection orchestration lives in `cmd/`; diff --git a/docs/memory/wt-cli/list-status-contract.md b/docs/memory/wt-cli/list-status-contract.md index ff46022..eec4200 100644 --- a/docs/memory/wt-cli/list-status-contract.md +++ b/docs/memory/wt-cli/list-status-contract.md @@ -66,6 +66,7 @@ This file documents the contract that `wt list` honors after the status-opt-in c - `handlePathLookup` uses raw `listWorktreeEntries()`. It runs BEFORE the enrichment dispatch and returns early. No `checkDirty` or `getUnpushedInDir` calls regardless of other flags. - `--path` + `--status` is rejected at flag-validation time, not after enrichment runs. +- **Not-found uses the structured `ExitWithError` form (`260622-log5`).** When `--path ` resolves no worktree, `handlePathLookup` exits via `wt.ExitWithError(wt.ExitGeneralError, "Worktree '' not found", "No worktree with that name in this repository", "Use 'wt list' to see available worktrees")` — the same `Error:`/`Why:`/`Fix:` what/why/fix structure (and `ExitGeneralError = 1`) that `wt go`/`wt open`'s not-found path emits, replacing the prior ad-hoc `fmt.Fprintf(os.Stderr, …) + os.Exit`. This brings the three not-found cases to byte-parity. See `/wt-cli/go-command-contract.md`'s "`wt go ` exits `ExitGeneralError`". ### Mutually exclusive flags @@ -357,6 +358,7 @@ relative buckets). - Sibling memory: `wt-cli/init-failure-contract.md` — same pattern of post-change invariant capture for a different `wt` subcommand. - Sibling memory: `wt-cli/recency-ordering-contract.md` — the shared `RecencyOf`/`RecencyLess`/`SortByRecency` definition that this file's `--sort`/`recent` ordering and `last_active` field consume; also covers the `wt open`/`wt delete` menu ordering. - Sibling memory: `wt-cli/idle-staleness-contract.md` — the `wt.IsIdle` predicate, `DefaultIdleThreshold`, and `wt delete --stale` selector that the `Idle *bool` field and `⚠ idle` marker documented here consume. That file is the authoritative cross-command (list + delete) idle contract; this file documents only the `wt list` display/JSON surface of it. +- Sibling memory: `/wt-cli/go-command-contract.md` — the `wt go`/`wt open` `Worktree '' not found` structured-error format that `--path` not-found now matches byte-for-byte (`260622-log5`). ## Open follow-ups (not in scope for this change) diff --git a/fab/changes/260622-log5-dx-copy-polish/.history.jsonl b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl new file mode 100644 index 0000000..27ec9dc --- /dev/null +++ b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl @@ -0,0 +1,10 @@ +{"action":"enter","driver":"fab-new","event":"stage-transition","stage":"intake","ts":"2026-06-22T06:58:16Z"} +{"args":"DX copy polish for wt CLI + wt go navigation confirmation (Tier 1+2 from audit). wt go stderr confirmation (compact-arrow), unified wt.Warn helper + stream fix, list.go ExitWithError parity, update.go Short capitalization, create/delete help-text clarity, replace ≠ glyph.","cmd":"fab-new","event":"command","ts":"2026-06-22T06:58:16Z"} +{"delta":"+4.7","event":"confidence","score":4.7,"trigger":"calc-score","ts":"2026-06-22T06:59:28Z"} +{"delta":"+0.0","event":"confidence","score":4.7,"trigger":"calc-score","ts":"2026-06-22T06:59:34Z"} +{"cmd":"fab-fff","event":"command","ts":"2026-06-22T08:39:42Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-06-22T08:39:47Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-06-22T08:48:24Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-06-22T08:54:02Z"} +{"event":"review","result":"passed","ts":"2026-06-22T08:54:02Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-06-22T08:59:06Z"} diff --git a/fab/changes/260622-log5-dx-copy-polish/.status.yaml b/fab/changes/260622-log5-dx-copy-polish/.status.yaml new file mode 100644 index 0000000..d48b80f --- /dev/null +++ b/fab/changes/260622-log5-dx-copy-polish/.status.yaml @@ -0,0 +1,50 @@ +id: log5 +name: 260622-log5-dx-copy-polish +created: 2026-06-22T06:58:16Z +created_by: sahil-noon +change_type: fix +issues: [] +progress: + intake: done + apply: done + review: done + hydrate: done + ship: active + review-pr: pending +plan: + generated: true + task_count: 15 + acceptance_count: 15 + acceptance_completed: 0 +confidence: + certain: 7 + confident: 1 + tentative: 0 + unresolved: 0 + score: 4.7 + fuzzy: true + dimensions: + signal: 80.6 + reversibility: 80.0 + competence: 88.1 + disambiguation: 80.6 +stage_metrics: + intake: {started_at: "2026-06-22T06:58:16Z", driver: fab-new, iterations: 1, completed_at: "2026-06-22T08:39:47Z"} + apply: {started_at: "2026-06-22T08:39:47Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:48:24Z"} + review: {started_at: "2026-06-22T08:48:24Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:54:02Z"} + hydrate: {started_at: "2026-06-22T08:54:02Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:59:06Z"} + ship: {started_at: "2026-06-22T08:59:06Z", driver: fab-fff, iterations: 1} +prs: [] +true_impact: + added: 0 + deleted: 0 + net: 0 + excluding: + added: 0 + deleted: 0 + net: 0 + computed_at: "2026-06-22T08:59:06Z" + computed_at_stage: hydrate +summary: 'DX copy polish: wt go stderr nav-confirmation, unified wt.Warn helper (delete warnings stdout→stderr), list --path structured not-found, update Short + help-text/glyph fixes' +# true_impact: lazily created on first apply-finish (no placeholder here). +last_updated: 2026-06-22T08:59:06Z diff --git a/fab/changes/260622-log5-dx-copy-polish/intake.md b/fab/changes/260622-log5-dx-copy-polish/intake.md new file mode 100644 index 0000000..e7d1fa9 --- /dev/null +++ b/fab/changes/260622-log5-dx-copy-polish/intake.md @@ -0,0 +1,215 @@ +# Intake: DX copy polish + `wt go` navigation confirmation + +**Change**: 260622-log5-dx-copy-polish +**Created**: 2026-06-22 + +## Origin + +Initiated conversationally via `/fab-new`, immediately following a **DX/copy audit** of the entire +`wt` CLI user-facing surface (the audit itself was the user's request: "do a run of all user-facing +copy / DX and see whether all we can be more / less verbose", plus a concrete ask: "In the output of +`wt go`, also specify the name of the repo, the current location etc."). + +The audit fanned out three read-only auditors (one per command cluster: create+delete, list+open+go, +init+update+shell-init+main) and the findings were **verified against source** (line numbers +confirmed; one auditor's proposed code that wouldn't compile was corrected). Full report: +`scratchpad/dx-copy-audit.md` (session-local). Findings were triaged into three tiers; the user chose +to implement **Tier 1 + Tier 2** as one coherent change and leave Tier 3 as-is. + +Key decisions made this session (all by the user): +- **`wt go` confirmation format**: compact-arrow (`→ {repo} / {worktree} ({branch})` + indented path), + chosen over a create-style labeled block and over a single line. +- **Scope**: Tier 1 (4 items) + Tier 2 (help text + glyph) via the full `/fab-fff` pipeline. Tier 3 OUT. +- **Governing rule** (pre-existing, reaffirmed): stdout = machine-readable result; stderr = all human + copy. See `docs/memory/wt-cli/create-output-phases.md` and the user-memory note + `wt-stdout-stderr-convention`. + +This is a **copy + consistency** change. The ONLY behavior changes are (a) the new `wt go` stderr +confirmation and (b) moving two `delete` warnings from stdout to stderr. Everything else is string +edits and one refactor (a shared warning helper). + +## Why + +**Problem.** The `wt` CLI copy is well-calibrated overall (not chatty, not cryptic), but the audit +surfaced real **inconsistencies** and one **feature gap**: +1. `wt go` gives no human confirmation of *where* it's navigating — just a bare path. The user can't + see which repo/worktree/branch they're about to land in. (This was the original request.) +2. Warning output is inconsistent: some warnings color-wrap `%sWarning:%s`, most use bare inline + `Warning:`; and two `delete` warnings print to **stdout** (a stream-discipline violation) while + sibling warnings in the same file correctly use stderr. +3. `wt list`'s `--path` not-found error bypasses the `ExitWithError(what,why,fix)` structure that the + byte-identical not-found case in `open`/`go` uses — ad-hoc `fmt.Fprintf` + `os.Exit` instead. +4. `wt update`'s cobra `Short` is lowercase (`"self-update…"`) — every other command capitalizes. +5. A few flag help strings are terse or use jargon ("porcelain"); one message uses a `≠` glyph that + renders poorly in some terminals. + +**Consequence if unfixed.** `wt go` stays less reassuring than `wt create` (which already prints a +`Created worktree: / Path: / Branch:` summary). The warning-stream bug means scripts that capture +`wt delete` stdout could ingest a warning line. The error-structure and capitalization drift make the +CLI read as less cohesive when commands are compared side-by-side. + +**Why this approach.** Surgical, consistency-first. No wholesale rewrites — the audit found nothing +that needs wholesale trimming/expansion. A single shared `wt.Warn` helper collapses ~10 divergent +call sites to one idiom (DRY, matches how `errors.go` already centralizes `WtError`/`RenderWarning`). +The `wt go` confirmation reuses the existing `create`-summary template and the existing +`getBranchForPath` helper, and goes to **stderr** so the stdout machine contract +(`cd "$(command wt go)"`, `WT_CD_FILE`) is untouched. + +## What Changes + +### 1. `wt go` — stderr navigation confirmation (compact-arrow) + +`navigateTo` in `src/cmd/wt/go.go` currently prints only the bare path to stdout (+ the wrapper hint +to stderr when no wrapper). Add a **stderr** confirmation block showing repo + worktree + branch. + +- **stdout stays byte-identical**: the bare resolved path remains the last (and only) stdout line. + This preserves `cd "$(command wt go some-name)"` and the `WT_CD_FILE` write contract + (launcher-contract.md §3). NON-NEGOTIABLE. +- **stderr gains** (compact-arrow form): + ``` + → idea / frosted-jaguar (feature-x) + /home/sahil/code/sahil87/idea.worktrees/frosted-jaguar + ``` + Line 1: `→ {RepoName} / {worktree-basename} ({branch})`. Line 2: two-space-indented absolute path. +- **Wiring**: thread `ctx *wt.RepoContext` into `navigateTo` (it currently takes only `path string`) + so it has `ctx.RepoName`. Update both call sites (the by-name path ~go.go:74 and the menu path + ~go.go:102) to pass `ctx`. Derive the branch via the existing `getBranchForPath(path)` in `open.go` + (one `git rev-parse --abbrev-ref HEAD` in the target worktree — negligible cost; reuse, don't + reimplement). +- **Ordering**: emit the confirmation block to stderr, then the existing wrapper-hint logic (or vice + versa — keep the hint and confirmation both on stderr; the bare path is the final stdout write). + The confirmation should print on the SUCCESS path only (after a worktree is resolved/selected), + not on cancel/no-worktrees. +- **Color**: if using any color (e.g. dimmed path), respect the existing `NO_COLOR`-blanked package + color vars (`wt.ColorReset != ""` test) — do NOT call `os.Getenv` afresh. Plain text is acceptable + too; match the create-summary's level of color. + +### 2. Unified warning helper + stream fix + +Add a single warning emitter and route all warning call sites through it. + +- **New helper** in `src/internal/worktree/errors.go`, alongside `WtError`/`RenderWarning`: + ```go + // Warn writes a color-wrapped "Warning:" diagnostic to stderr. Respects the + // NO_COLOR-blanked package color vars (no fresh os.Getenv). + func Warn(format string, args ...any) { + fmt.Fprintf(os.Stderr, "%sWarning:%s %s\n", ColorYellow, ColorReset, fmt.Sprintf(format, args...)) + } + ``` + (Confirm the exact color var names against the existing file — `ColorYellow`/`ColorReset` per the + `%sWarning:%s Worktree has uncommitted changes` lines already in `delete.go`.) +- **Convert all warning call sites** to `wt.Warn(...)`: + - `create.go`: lines ~127 (already color-wrapped, to stderr — convert for uniformity), ~191, ~389, + ~400, ~404, ~414, ~418 (bare inline `Warning:` → helper). + - `delete.go`: lines ~397, ~477 (bare inline, stderr → helper), and **~668, ~703 — the STREAM FIX**: + these use `fmt.Printf` (**stdout**) for a `%sWarning:%s` warning before an interactive menu. Route + through `wt.Warn` so they land on **stderr**. (`wt delete` has no stdout machine contract, but + warnings are diagnostics and the rest of the file already uses stderr — this removes the + inconsistency.) + - Preserve each warning's wording (the helper only standardizes the `Warning:` prefix + stream + + color); keep the existing message text. Note line ~668/~703 print surrounding blank lines + (`\n…\n\n`) for menu spacing — preserve that spacing (the helper appends one `\n`; add the extra + blank-line framing at the call site if the menu layout needs it). + +### 3. `wt list` `--path` not-found → `ExitWithError` + +`src/cmd/wt/list.go` ~line 311 (the `--path ` lookup, in `handlePathLookup`) does: +```go +fmt.Fprintf(os.Stderr, "Worktree '%s' not found. Use 'wt list' to see available worktrees.\n", name) +os.Exit(wt.ExitGeneralError) +``` +Replace with the structured form, byte-parity with `open.go`/`go.go`'s not-found case: +```go +wt.ExitWithError(wt.ExitGeneralError, + fmt.Sprintf("Worktree '%s' not found", name), + "No worktree with that name in this repository", + "Use 'wt list' to see available worktrees") +``` + +### 4. `wt update` `Short` capitalization + +`src/cmd/wt/update.go:17`: `Short: "self-update the wt binary via Homebrew"` → +`Short: "Self-update the wt binary via Homebrew"`. + +### 5. Help-text clarity (flag descriptions) + glyph + +- `create.go`: + - `--worktree-open` help → list the option space: `"After creation: prompt (menu), default (auto-detect app), skip, or an app name (e.g. code, cursor)"`. + - `--non-interactive` help → drop "porcelain" jargon: `"No prompts; use defaults and skip menus"`. + - (Verify exact current strings/lines before editing — the audit cited ~line 438 area.) +- `delete.go`: + - `--delete-branch` help → tri-state: `"Delete the associated branch: true (always), false (never), auto (default — only if branch name matches worktree name)"`. + - `--delete-remote` help → dependency note: `"Delete the remote-tracking branch when the local branch is deleted (true by default)"`. + - Line ~747: replace the `≠` glyph — `"Skipped branch deletion: %s ≠ worktree name (%s). Use --delete-branch true to force."` → + `"Skipped branch deletion: branch '%s' does not match worktree name '%s'; use --delete-branch=true to force"`. + +> **Verification mandate for apply**: the audit's line numbers were verified at audit time but the +> apply agent MUST re-grep each target string before editing (the file may have shifted). Edit by +> matching the string, not by trusting the line number. + +### Out of scope (Tier 3 — deliberately NOT changed) + +Menu prompts (`"Select worktree to go to:"` / `"Open in:"` / `"Cancelled."` / `"No worktrees found."`), +the `go`/`open` wrapper hints, `main.go:43` root error sink (intentional raw catch-all), `init.go`'s +inline failure line, and `list.go`'s `"Worktrees for:"` header — all judged well-calibrated. Do not +touch them. + +## Affected Memory + +- `wt-cli/go-command-contract`: (modify) — document the new `wt go` stderr navigation-confirmation + block (compact-arrow `→ {repo} / {worktree} ({branch})` + indented path), and reaffirm that stdout + stays the bare path only. The existing contract already covers navigateTo's WT_CD_FILE + stdout + behavior; this adds the stderr confirmation. +- `wt-cli/create-output-phases`: (modify, optional) — this is the canonical stdout/stderr stream + contract. Optionally note the new `wt.Warn` helper as the single warning emitter (stderr, color- + wrapped) and that `wt delete`'s two formerly-stdout warnings were realigned to stderr. If a + dedicated warning-helper contract fits better as its own file, the hydrate stage may create + `wt-cli/warning-output-contract` (new) instead — apply/hydrate's judgment. + +## Impact + +**Code:** +- `src/cmd/wt/go.go` — `navigateTo` signature (`+ctx`), confirmation block, call-site updates. +- `src/internal/worktree/errors.go` — new `Warn` helper. +- `src/cmd/wt/create.go` — warning call sites → `wt.Warn`; `--worktree-open`/`--non-interactive` help. +- `src/cmd/wt/delete.go` — warning call sites → `wt.Warn` (incl. the two stdout→stderr fixes); + `--delete-branch`/`--delete-remote` help; `≠` glyph. +- `src/cmd/wt/list.go` — `--path` not-found → `ExitWithError`. +- `src/cmd/wt/update.go` — `Short` capitalization. + +**Tests** (Constitution IV — test what the user sees): +- `go_test.go` / `integration_test.go` — assert the new stderr confirmation appears on `wt go` + success AND that **stdout is still exactly the bare path** (the critical regression guard). Assert + it does NOT appear on cancel/no-worktrees. +- `delete_test.go` — assert the two realigned warnings now appear on **stderr**, not stdout. +- `errors_test.go` — unit-test `Warn` (color-wrapped to stderr; NO_COLOR → plain, no ANSI). +- `list_test.go` — the `--path` not-found path now emits the what/why/fix structure (assert on the + combined/stderr output and exit code). +- Update any existing test asserting the old `update` `Short`, the old `≠` message, or the old help + strings (Test Integrity: tests conform to the new spec). + +**Exit codes / contracts:** No new exit codes. No env-var changes. The `wt open` launcher-contract +surface `hop` depends on is untouched. `wt go` stdout contract is preserved (the whole point). + +**CI:** gofmt-enforced (module root `src/`) — run `gofmt -l`, `go vet`, `go test ./...` from `src/`. + +## Open Questions + +- None blocking. The one judgment call left to apply: whether the warning realignment + helper + warrants a new `wt-cli/warning-output-contract` memory file vs. a note appended to + `create-output-phases` — deferred to hydrate. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | `wt go` stdout stays the bare path; confirmation goes to stderr only | Governing stream rule + the launcher/WT_CD_FILE contract depend on it; user-confirmed | S:95 R:85 A:100 D:95 | +| 2 | Certain | `wt go` confirmation uses compact-arrow `→ {repo} / {worktree} ({branch})` + indented path | User explicitly chose this format over the two alternatives this session | S:95 R:90 A:95 D:95 | +| 3 | Certain | Scope = Tier 1 + Tier 2; Tier 3 explicitly excluded | User chose this scope this session; Tier 3 items listed as out-of-scope | S:95 R:80 A:95 D:90 | +| 4 | Confident | Single `wt.Warn` helper in errors.go is the right consolidation | Matches how errors.go already centralizes WtError/RenderWarning; collapses ~10 sites; DRY | S:80 R:75 A:90 D:80 | +| 5 | Confident | Realign delete.go:668/703 warnings stdout→stderr (not a breaking change) | wt delete has no stdout machine contract; warnings are diagnostics; sibling warnings already use stderr | S:80 R:70 A:90 D:80 | +| 6 | Confident | Reuse `getBranchForPath` (open.go) for the branch in the confirmation | Already exists and is used by open's menu; reuse over reimplement (Constitution V) | S:75 R:85 A:90 D:80 | +| 7 | Confident | Preserve each warning's existing message text; helper only standardizes prefix/stream/color | Minimizes blast radius; the audit flagged format not wording | S:75 R:80 A:85 D:80 | +| 8 | Tentative | Whether warning realignment gets its own memory file or a note on create-output-phases | Either is defensible; deferred to hydrate's judgment | S:50 R:75 A:60 D:45 | + +8 assumptions (3 certain, 4 confident, 1 tentative, 0 unresolved). diff --git a/fab/changes/260622-log5-dx-copy-polish/plan.md b/fab/changes/260622-log5-dx-copy-polish/plan.md new file mode 100644 index 0000000..9d0c3cb --- /dev/null +++ b/fab/changes/260622-log5-dx-copy-polish/plan.md @@ -0,0 +1,203 @@ +# Plan: DX copy polish + `wt go` navigation confirmation + +**Change**: 260622-log5-dx-copy-polish +**Intake**: `intake.md` + +## Requirements + +### CLI Output: `wt go` navigation confirmation + +#### R1: `wt go` SHALL emit a stderr navigation confirmation on success +On a successful navigation (a worktree was resolved by name or selected from the +menu), `wt go` SHALL write a compact-arrow confirmation block to **stderr**: +line 1 `→ {RepoName} / {worktree-basename} ({branch})`, line 2 a two-space-indented +absolute path. The branch SHALL be derived via the existing `getBranchForPath(path)`. + +- **GIVEN** a repo with a worktree `frosted-jaguar` on branch `feature-x` +- **WHEN** the user runs `wt go frosted-jaguar` +- **THEN** stderr contains `→ {repo} / frosted-jaguar (feature-x)` followed by an indented absolute path line +- **AND** the confirmation is NOT emitted on the cancel or no-worktrees paths + +#### R2: `wt go` stdout SHALL remain exactly the bare resolved path (NON-NEGOTIABLE) +The confirmation is diagnostic copy and MUST NOT alter the stdout machine contract. +stdout SHALL stay the single bare resolved absolute path as the final (and only) +stdout line, preserving `cd "$(command wt go )"` and the `WT_CD_FILE` write. + +- **GIVEN** the user runs `wt go ` with `WT_CD_FILE` set +- **WHEN** navigation succeeds +- **THEN** stdout is exactly the resolved path (no confirmation text on stdout) +- **AND** `WT_CD_FILE` contains the resolved path at mode 0600 + +#### R3: `navigateTo` SHALL receive `*wt.RepoContext` to render the confirmation +`navigateTo(path string)` SHALL become `navigateTo(ctx *wt.RepoContext, path string)` +so it can render `ctx.RepoName`. Both call sites (by-name and menu) SHALL pass `ctx`. + +- **GIVEN** the two `navigateTo` call sites in `go.go` +- **WHEN** the change is applied +- **THEN** both pass the resolved `ctx` and the binary compiles + +### Diagnostics: unified warning helper + stream discipline + +#### R4: A single `wt.Warn` helper SHALL emit color-wrapped warnings to stderr +`internal/worktree/errors.go` SHALL gain `func Warn(format string, args ...any)` +that writes `%sWarning:%s %s\n` (`ColorYellow`/`ColorReset` + formatted message) to +**stderr**, respecting the existing NO_COLOR-blanked package color vars (no fresh +`os.Getenv`). + +- **GIVEN** color is enabled +- **WHEN** `Warn("msg %d", 1)` is called +- **THEN** stderr receives `\033[0;33mWarning:\033[0m msg 1\n` +- **AND** under NO_COLOR (blanked vars) stderr receives `Warning: msg 1\n` with no ANSI + +#### R5: All `create.go`/`delete.go` warning call sites SHALL route through `wt.Warn` +Every `Warning:`-prefixed diagnostic in `create.go` and `delete.go` SHALL be +converted to `wt.Warn(...)`, preserving each message's existing wording (the helper +only standardizes the prefix/stream/color). + +- **GIVEN** the divergent warning call sites +- **WHEN** converted +- **THEN** each emits its original message text via `wt.Warn`, landing on stderr + +#### R6: `delete.go`'s two pre-menu warnings SHALL move from stdout to stderr +The uncommitted-changes and unpushed-commits warnings (currently `fmt.Printf` → +stdout) SHALL be routed through `wt.Warn` (→ stderr). Their surrounding blank-line +spacing for menu layout SHALL be preserved. + +- **GIVEN** an interactive `wt delete` of a worktree with uncommitted changes / unpushed commits +- **WHEN** the warning prints +- **THEN** it appears on **stderr**, not stdout +- **AND** a leading blank line precedes it and a trailing blank line follows it (menu spacing preserved) + +### CLI Consistency: structured errors, capitalization, help text, glyph + +#### R7: `wt list --path` not-found SHALL use the structured `ExitWithError` form +`handlePathLookup`'s raw `fmt.Fprintf(os.Stderr, ...) + os.Exit(...)` SHALL be +replaced with `wt.ExitWithError(wt.ExitGeneralError, "Worktree '%s' not found", +"No worktree with that name in this repository", "Use 'wt list' to see available +worktrees")` — byte-parity with `open.go`/`go.go`'s not-found case. + +- **GIVEN** `wt list --path no-such` +- **WHEN** the name does not resolve +- **THEN** stderr shows the `Error:`/`Why:`/`Fix:` structure and exit code is 1 (ExitGeneralError) + +#### R8: `wt update` `Short` SHALL be capitalized +`update.go`'s `Short: "self-update the wt binary via Homebrew"` SHALL become +`"Self-update the wt binary via Homebrew"`. + +- **GIVEN** `wt --help` / `wt update --help` +- **WHEN** rendered +- **THEN** the update Short begins with a capital "S" + +#### R9: Flag help strings SHALL be clarified and the `≠` glyph replaced +`create.go` `--worktree-open` and `--non-interactive`, and `delete.go` +`--delete-branch` and `--delete-remote`, SHALL adopt the intake's clarified strings. +`delete.go`'s auto-mode skip message SHALL replace the `≠` glyph with plain ASCII, +keeping its two args `(branch, wtName)`. + +- **GIVEN** the relevant `--help` output and the auto-mode skip path +- **WHEN** rendered/triggered +- **THEN** the new strings appear and no `≠` glyph is emitted + +### Non-Goals + +- Tier 3 items are explicitly excluded: menu prompts, go/open wrapper hints, + `main.go` root error sink, `init.go` inline failure line, `list.go` + "Worktrees for:" header. +- No memory or spec hydration in this stage (hydrate stage owns memory; specs + unchanged because the help-text edits do not alter the documented surface — + `cli-surface.md` paraphrases behavior, not the verbatim Go help strings). + +### Design Decisions + +1. **Confirmation on stderr, never stdout**: the launcher/`WT_CD_FILE` contract and + `cd "$(command wt go)"` depend on a bare-path stdout — *Why*: governing + stream-discipline rule (stdout=machine, stderr=human) — *Rejected*: printing the + block to stdout (breaks scripting). +2. **`wt.Warn` in `internal/worktree/errors.go`, not `cmd/`**: collapses ~10 divergent + call sites and keeps `cmd/` thin (Constitution V) — *Why*: matches how `errors.go` + already centralizes `WtError`/`RenderWarning` — *Rejected*: a `cmd/`-local helper. +3. **Reuse `getBranchForPath`**: already used by the open/go menus — *Why*: reuse over + reimplement (Constitution V) — *Rejected*: a fresh `git rev-parse` in `go.go`. + +## Tasks + +### Phase 1: Helper + +- [x] T001 Add `func Warn(format string, args ...any)` to `src/internal/worktree/errors.go` (alongside `WtError`/`PrintError`): writes `%sWarning:%s %s\n` to `os.Stderr` using `ColorYellow`/`ColorReset`. + +### Phase 2: Core string/flow edits + +- [x] T002 In `src/cmd/wt/go.go`: change `navigateTo(path string)` to `navigateTo(ctx *wt.RepoContext, path string)`, add the stderr compact-arrow confirmation block (`→ {ctx.RepoName} / {filepath.Base(path)} ({getBranchForPath(path)})` + 2-space-indented absolute path) emitted before the wrapper-hint/`WT_CD_FILE` logic, keep `fmt.Println(path)` as the last stdout write; add `path/filepath` import. +- [x] T003 In `src/cmd/wt/go.go`: update both `navigateTo` call sites (by-name `~:74`, menu `~:102`) to `navigateTo(ctx, path)`. +- [x] T004 In `src/cmd/wt/create.go`: convert all `Warning:` call sites (uncommitted-changes ~:127; reuse-init ~:191; open-menu/default/named-app warnings ~:389/:400/:404/:414/:418) to `wt.Warn(...)`, stripping the literal `Warning: ` prefix and preserving the remaining message text. +- [x] T005 In `src/cmd/wt/delete.go`: convert the two `fmt.Fprintf(os.Stderr, "Warning: failed to remove %s: %s\n", ...)` warnings (~:397, ~:477) to `wt.Warn("failed to remove %s: %s", ...)`. +- [x] T006 In `src/cmd/wt/delete.go`: route the two pre-menu warnings (uncommitted-changes ~:668; unpushed-commits ~:703) through `wt.Warn` (stdout→stderr), framing each with a leading `fmt.Fprintln(os.Stderr)` and a trailing `fmt.Fprintln(os.Stderr)` to preserve menu spacing. +- [x] T007 In `src/cmd/wt/list.go` `handlePathLookup`: replace the raw `fmt.Fprintf(os.Stderr, "Worktree '%s' not found. ...") + os.Exit(...) + return nil` with `wt.ExitWithError(wt.ExitGeneralError, fmt.Sprintf("Worktree '%s' not found", name), "No worktree with that name in this repository", "Use 'wt list' to see available worktrees")`; drop the now-unreachable `return nil` if Go flags it (ExitWithError does not return). +- [x] T008 In `src/cmd/wt/update.go`: capitalize `Short` to `"Self-update the wt binary via Homebrew"`. +- [x] T009 In `src/cmd/wt/create.go`: set `--worktree-open` help to `"After creation: prompt (menu), default (auto-detect app), skip, or an app name (e.g. code, cursor)"` and `--non-interactive` help to `"No prompts; use defaults and skip menus"`. +- [x] T010 In `src/cmd/wt/delete.go`: set `--delete-branch` help to `"Delete the associated branch: true (always), false (never), auto (default — only if branch name matches worktree name)"`, `--delete-remote` help to `"Delete the remote-tracking branch when the local branch is deleted (true by default)"`, and replace the `≠` skip message with `"Skipped branch deletion: branch '%s' does not match worktree name '%s'; use --delete-branch=true to force"` (keep args `branch, wtName`). + +### Phase 3: Tests + +- [x] T011 In `src/cmd/wt/go_test.go`: add a test that `wt go ` success emits the confirmation block on **stderr** (`→`, repo name, worktree name, branch, indented path) AND that stdout is still exactly the bare path (regression guard for R2). +- [x] T012 In `src/cmd/wt/go_test.go`: add a test that the confirmation does NOT appear on the no-worktrees menu path (no `→` on stderr). +- [x] T013 In `src/internal/worktree/errors_test.go`: unit-test `Warn` — color-wrapped to stderr when color enabled; plain `Warning: ` with no ANSI when color vars are blanked (NO_COLOR). +- [x] T014 In `src/cmd/wt/delete_test.go`: add a test asserting the uncommitted-changes / unpushed-commits warning appears on **stderr** and NOT on stdout. +- [x] T015 In `src/cmd/wt/list_test.go`: add a test that `wt list --path ` emits the structured what/why/fix (`Error:`, `Why:`, `Fix:`) on stderr and exits 1. + +## Execution Order + +- T001 blocks T004, T005, T006 (they call `wt.Warn`). +- T002 blocks T003 (signature before call-site updates). +- Phase 3 tests follow their corresponding implementation tasks. + +## Acceptance + +### Functional Completeness + +- [ ] A-001 R1: `wt go ` (and the menu path) emits `→ {repo} / {worktree} ({branch})` + an indented absolute path on stderr on success +- [ ] A-002 R3: `navigateTo` takes `(ctx *wt.RepoContext, path string)` and both call sites pass `ctx`; binary compiles +- [ ] A-003 R4: `wt.Warn(format, args...)` exists in `errors.go`, writes color-wrapped `Warning:` + message to stderr +- [ ] A-004 R5: every former `Warning:` call site in `create.go`/`delete.go` now calls `wt.Warn` with its original wording preserved +- [ ] A-005 R7: `wt list --path ` uses `wt.ExitWithError(ExitGeneralError, ...)` (structured what/why/fix) +- [ ] A-006 R8: `wt update` Short reads "Self-update the wt binary via Homebrew" +- [ ] A-007 R9: the four flag help strings and the auto-mode skip message match the intake's strings; no `≠` glyph remains + +### Behavioral Correctness + +- [ ] A-008 R2: `wt go` stdout is exactly the bare resolved path (no confirmation leaks to stdout); `WT_CD_FILE` still written at mode 0600 (regression guard test passes) +- [ ] A-009 R6: the two `delete` pre-menu warnings now land on stderr, not stdout, with leading/trailing blank-line spacing preserved + +### Scenario Coverage + +- [ ] A-010 R1: a test asserts the confirmation does NOT appear on the cancel/no-worktrees path +- [ ] A-011 R4: a test asserts `Warn` emits ANSI when colored and plain text under blanked color vars + +### Edge Cases & Error Handling + +- [ ] A-012 R7: `wt list --path ` exits with code 1 (ExitGeneralError) + +### Code Quality + +- [ ] A-013 Pattern consistency: new code follows the cobra/stderr/`wt.`-helper patterns of surrounding code (Constitution II/V); the confirmation respects NO_COLOR-blanked color vars without fresh `os.Getenv` +- [ ] A-014 No unnecessary duplication: branch derivation reuses `getBranchForPath`; warnings reuse the single `wt.Warn` helper +- [ ] A-015 Tooling: `gofmt -l .`, `go vet ./...`, `go test ./...` are all clean/green from `src/` + +## Notes + +- Check items as you review: `- [x]` +- All acceptance items must pass before `/fab-continue` (hydrate) + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | `wt go` stdout stays the bare path; confirmation goes to stderr only | Governing stream rule + launcher/WT_CD_FILE contract; user-confirmed in intake | S:95 R:85 A:100 D:95 | +| 2 | Certain | Compact-arrow confirmation `→ {repo} / {worktree} ({branch})` + indented path | User chose this exact format over alternatives (intake) | S:95 R:90 A:95 D:95 | +| 3 | Confident | `wt.Warn` helper in errors.go is the right consolidation | Matches errors.go's WtError/RenderWarning centralization; DRY (Constitution V) | S:80 R:75 A:90 D:80 | +| 4 | Confident | Strip literal `Warning: ` from each converted format string (helper re-adds it) | Helper standardizes the prefix; preserving it in the format would double it | S:85 R:85 A:90 D:90 | +| 5 | Confident | Frame the two delete warnings with `Fprintln(os.Stderr)` before+after to reproduce the old `\n…\n\n` menu spacing | Old form had leading `\n` + trailing `\n\n`; Warn appends one `\n`, so one blank line each side reproduces it | S:80 R:80 A:85 D:80 | +| 6 | Confident | Reuse `getBranchForPath` (open.go) for the branch in the confirmation | Already exists and used by open/go menus; reuse over reimplement | S:75 R:85 A:90 D:80 | +| 7 | Confident | docs/specs/cli-surface.md left unchanged | It paraphrases behavior (unchanged), not the verbatim Go help strings | S:75 R:80 A:85 D:80 | + +7 assumptions (2 certain, 5 confident, 0 tentative). diff --git a/src/cmd/wt/create.go b/src/cmd/wt/create.go index 6a9b50d..79d342c 100644 --- a/src/cmd/wt/create.go +++ b/src/cmd/wt/create.go @@ -124,8 +124,7 @@ or creates a new branch.`, // Dirty-state check if !nonInteractive && (wt.HasUncommittedChanges() || wt.HasUntrackedFiles()) { - fmt.Fprintf(os.Stderr, "%sWarning: main repo has uncommitted changes%s\n", - wt.ColorYellow, wt.ColorReset) + wt.Warn("main repo has uncommitted changes") choice, err := wt.ShowMenu("How to proceed?", []string{ "Continue anyway", "Stash changes first", @@ -188,7 +187,7 @@ or creates a new branch.`, if worktreeInit == "true" { initScript := wt.InitScriptPath() if err := wt.RunWorktreeSetup(existingWtPath, initScript, ctx.RepoRoot); err != nil { - fmt.Fprintf(os.Stderr, "Warning: worktree init failed for reused worktree %q: %v\n", finalName, err) + wt.Warn("worktree init failed for reused worktree %q: %v", finalName, err) } } fmt.Println(existingWtPath) @@ -386,7 +385,7 @@ or creates a new branch.`, selected := apps[choice-1] wt.SaveLastApp(selected.Cmd) if openErr := wt.OpenInApp(selected.Cmd, wtPath, ctx.RepoName, finalName); openErr != nil { - fmt.Fprintf(os.Stderr, "Warning: could not open in %s: %s\n", selected.Name, openErr) + wt.Warn("could not open in %s: %s", selected.Name, openErr) } if selected.Cmd == "open_here" { suppressPath = true @@ -397,11 +396,11 @@ or creates a new branch.`, apps := wt.BuildAvailableApps() resolved, err := wt.ResolveDefaultApp(apps) if err != nil { - fmt.Fprintf(os.Stderr, "Warning: %s\n", err) + wt.Warn("%s", err) } else { wt.SaveLastApp(resolved.Cmd) if openErr := wt.OpenInApp(resolved.Cmd, wtPath, ctx.RepoName, finalName); openErr != nil { - fmt.Fprintf(os.Stderr, "Warning: could not open in %s: %s\n", resolved.Name, openErr) + wt.Warn("could not open in %s: %s", resolved.Name, openErr) } if resolved.Cmd == "open_here" { suppressPath = true @@ -411,11 +410,11 @@ or creates a new branch.`, apps := wt.BuildAvailableApps() resolved, err := wt.ResolveApp(worktreeOpen, apps) if err != nil { - fmt.Fprintf(os.Stderr, "Warning: %s\n", err) + wt.Warn("%s", err) } else { wt.SaveLastApp(resolved.Cmd) if openErr := wt.OpenInApp(resolved.Cmd, wtPath, ctx.RepoName, finalName); openErr != nil { - fmt.Fprintf(os.Stderr, "Warning: could not open in %s: %s\n", resolved.Name, openErr) + wt.Warn("could not open in %s: %s", resolved.Name, openErr) } if resolved.Cmd == "open_here" { suppressPath = true @@ -436,9 +435,9 @@ or creates a new branch.`, cmd.Flags().StringVar(&worktreeName, "worktree-name", "", "Set worktree name (skips name prompt)") cmd.Flags().StringVar(&worktreeInit, "worktree-init", "", "Run worktree init script: true (default) or false") - cmd.Flags().StringVar(&worktreeOpen, "worktree-open", "", "Open in app after creation, or 'skip'") + cmd.Flags().StringVar(&worktreeOpen, "worktree-open", "", "After creation: prompt (menu), default (auto-detect app), skip, or an app name (e.g. code, cursor)") cmd.Flags().BoolVar(&reuse, "reuse", false, "Reuse existing worktree if name collides (requires --worktree-name)") - cmd.Flags().BoolVar(&nonInteractive, "non-interactive", false, "No prompts, porcelain output") + cmd.Flags().BoolVar(&nonInteractive, "non-interactive", false, "No prompts; use defaults and skip menus") cmd.Flags().StringVar(&base, "base", "", "Git ref (branch, tag, SHA) to use as start-point for new branch") return cmd diff --git a/src/cmd/wt/delete.go b/src/cmd/wt/delete.go index 7ab7d38..f1720f8 100644 --- a/src/cmd/wt/delete.go +++ b/src/cmd/wt/delete.go @@ -136,8 +136,8 @@ Resolution order: --stale, --delete-all, positional args, --worktree-name (depre } cmd.Flags().StringVar(&worktreeName, "worktree-name", "", "Worktree to delete") - cmd.Flags().StringVar(&deleteBranch, "delete-branch", "", "Delete associated branch: true, false, or auto (default: auto — deletes only when branch matches worktree name)") - cmd.Flags().StringVar(&deleteRemote, "delete-remote", "", "Delete remote branch: true (default) or false") + cmd.Flags().StringVar(&deleteBranch, "delete-branch", "", "Delete the associated branch: true (always), false (never), auto (default — only if branch name matches worktree name)") + cmd.Flags().StringVar(&deleteRemote, "delete-remote", "", "Delete the remote-tracking branch when the local branch is deleted (true by default)") cmd.Flags().BoolVar(&deleteAll, "delete-all", false, "Delete all worktrees") cmd.Flags().BoolVarP(&stashFlag, "stash", "s", false, "Stash uncommitted changes before deleting") cmd.Flags().BoolVar(&nonInteractive, "non-interactive", false, "No prompts, use defaults") @@ -394,7 +394,7 @@ func handleDeleteMultiple(session *wt.MenuSession, names []string, nonInteractiv fmt.Println("Removing worktree...") if err := wt.RemoveWorktree(w.path, true); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to remove %s: %s\n", w.name, err) + wt.Warn("failed to remove %s: %s", w.name, err) continue } fmt.Printf("Deleted worktree: %s%s%s\n", wt.ColorGreen, w.name, wt.ColorReset) @@ -474,7 +474,7 @@ func handleDeleteAll(session *wt.MenuSession, nonInteractive bool, deleteBranch, fmt.Println("Removing worktree...") if err := wt.RemoveWorktree(w.path, true); err != nil { - fmt.Fprintf(os.Stderr, "Warning: failed to remove %s: %s\n", w.name, err) + wt.Warn("failed to remove %s: %s", w.name, err) continue } fmt.Printf("Deleted worktree: %s%s%s\n", wt.ColorGreen, w.name, wt.ColorReset) @@ -665,7 +665,11 @@ func handleUncommittedChanges(session *wt.MenuSession, wtName, stashMode string, return nil } - fmt.Printf("\n%sWarning:%s Worktree has uncommitted changes\n\n", wt.ColorYellow, wt.ColorReset) + // Diagnostic → stderr (wt.Warn). The blank lines around it are menu-layout + // framing the helper does not emit (Warn appends a single trailing newline). + fmt.Fprintln(os.Stderr) + wt.Warn("Worktree has uncommitted changes") + fmt.Fprintln(os.Stderr) choice, err := session.Show("What would you like to do?", []string{ "Stash changes and delete (Recommended)", @@ -700,7 +704,11 @@ func handleUnpushedCommits(session *wt.MenuSession, branch string, nonInteractiv } count := wt.GetUnpushedCount(branch) - fmt.Printf("\n%sWarning:%s Branch has %d unpushed commit(s)\n\n", wt.ColorYellow, wt.ColorReset, count) + // Diagnostic → stderr (wt.Warn). The blank lines around it are menu-layout + // framing the helper does not emit (Warn appends a single trailing newline). + fmt.Fprintln(os.Stderr) + wt.Warn("Branch has %d unpushed commit(s)", count) + fmt.Fprintln(os.Stderr) fmt.Println("Commits that will be lost:") lines := wt.GetUnpushedCommitLines(branch, 5) @@ -744,7 +752,7 @@ func handleBranchCleanup(branch, wtName, deleteBranch, deleteRemote string) { if branch == wtName { shouldDelete = true } else { - fmt.Printf("Skipped branch deletion: %s ≠ worktree name (%s). Use --delete-branch true to force.\n", branch, wtName) + fmt.Printf("Skipped branch deletion: branch '%s' does not match worktree name '%s'; use --delete-branch=true to force\n", branch, wtName) } } diff --git a/src/cmd/wt/delete_test.go b/src/cmd/wt/delete_test.go index a13f340..db93ab5 100644 --- a/src/cmd/wt/delete_test.go +++ b/src/cmd/wt/delete_test.go @@ -369,6 +369,28 @@ func TestDelete_MultipleBranchPreservation(t *testing.T) { assertBranchExists(t, repo, "bp-bravo") } +// TestDelete_UncommittedWarning_OnStderr verifies the interactive +// "uncommitted changes" warning now lands on STDERR (not stdout), and that the +// menu prompt itself still renders on stdout. Empty stdin makes the menu return +// on EOF after the warning has printed; we assert only on the warning stream. +func TestDelete_UncommittedWarning_OnStderr(t *testing.T) { + repo := createTestRepo(t) + wtPath := createWorktreeViaWt(t, repo, "dirty-current") + + // Make the worktree dirty so handleUncommittedChanges fires. + os.WriteFile(filepath.Join(wtPath, "dirty.txt"), []byte("uncommitted"), 0644) + gitRun(t, wtPath, "add", "dirty.txt") + + // Interactive (no --non-interactive), run from INSIDE the worktree with no + // name → handleDeleteCurrent → handleUncommittedChanges. Empty stdin makes + // the subsequent menu return on EOF after the warning prints. + r := runWt(t, wtPath, nil, "delete") + + // The warning is a diagnostic and MUST be on stderr, never stdout. + assertContains(t, r.Stderr, "Warning: Worktree has uncommitted changes") + assertNotContains(t, r.Stdout, "Worktree has uncommitted changes") +} + func TestDelete_MultipleWithStash(t *testing.T) { repo := createTestRepo(t) wtPathA := createWorktreeViaWt(t, repo, "stash-alpha") diff --git a/src/cmd/wt/go.go b/src/cmd/wt/go.go index c0dcabb..bb28115 100644 --- a/src/cmd/wt/go.go +++ b/src/cmd/wt/go.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "path/filepath" wt "github.com/sahil87/wt/internal/worktree" "github.com/spf13/cobra" @@ -71,7 +72,7 @@ Requires a git repository — worktree resolution walks the repo's worktree list err.Error(), "Check 'git worktree list' from this repo") } - return navigateTo(path) + return navigateTo(ctx, path) } // No name. A no-arg selection menu has no sensible non-interactive @@ -99,7 +100,7 @@ Requires a git repository — worktree resolution walks the repo's worktree list return nil } - return navigateTo(path) + return navigateTo(ctx, path) }, } @@ -113,11 +114,22 @@ Requires a git repository — worktree resolution walks the repo's worktree list // cd's the parent shell there, and always prints the path to stdout as the last // line so the no-wrapper scripting form (cd "$(command wt go ...)") works. // +// A compact-arrow navigation confirmation (repo / worktree / branch + the +// absolute path) is written to STDERR so the user can see where they are landing +// without polluting the stdout machine contract. stdout stays the bare resolved +// path only — this preserves cd "$(command wt go ...)" and the WT_CD_FILE write. +// // Per Constitution VII, wt never cd's the parent shell directly — it cooperates // via WT_CD_FILE / stdout and the shell wrapper evaluates the result. The // WT_CD_FILE semantics (mode 0600, truncate-on-write, contents = resolved dir // path) are the same ones documented in launcher-contract.md §3 for "Open here". -func navigateTo(path string) error { +func navigateTo(ctx *wt.RepoContext, path string) error { + // Confirmation block (stderr, human copy). getBranchForPath reuses the same + // single git rev-parse the open/go menus use. Emits no color, so it is + // NO_COLOR-safe by construction. + fmt.Fprintf(os.Stderr, "→ %s / %s (%s)\n", ctx.RepoName, filepath.Base(path), getBranchForPath(path)) + fmt.Fprintf(os.Stderr, " %s\n", path) + if cdFile := os.Getenv("WT_CD_FILE"); cdFile != "" { if err := os.WriteFile(cdFile, []byte(path), 0600); err != nil { wt.ExitWithError(wt.ExitGeneralError, diff --git a/src/cmd/wt/go_test.go b/src/cmd/wt/go_test.go index ea6e304..e9251b2 100644 --- a/src/cmd/wt/go_test.go +++ b/src/cmd/wt/go_test.go @@ -52,6 +52,50 @@ func TestGo_NameArg_NavigatesToWorktree(t *testing.T) { } } +// TestGo_NameArg_StderrConfirmation_StdoutStaysBarePath verifies the navigation +// confirmation block lands on STDERR (repo / worktree / branch + indented path) +// while STDOUT stays EXACTLY the bare resolved path — the critical regression +// guard for the stdout machine contract (cd "$(command wt go ...)"). +func TestGo_NameArg_StderrConfirmation_StdoutStaysBarePath(t *testing.T) { + repo := createTestRepo(t) + wtPath := createWorktreeViaWt(t, repo, "frosted-jaguar") + + cdFile := filepath.Join(repo, "wt-cd") + env := []string{"WT_CD_FILE=" + cdFile, "WT_WRAPPER=1"} + + r := runWtSuccess(t, repo, env, "go", "frosted-jaguar") + + // STDOUT must be EXACTLY the bare path (single line, no confirmation text). + if got := strings.TrimRight(r.Stdout, "\n"); got != wtPath { + t.Errorf("stdout must be exactly the bare path %q, got %q", wtPath, got) + } + if strings.Contains(r.Stdout, "→") { + t.Errorf("confirmation arrow must NOT appear on stdout, got: %q", r.Stdout) + } + + // STDERR carries the compact-arrow confirmation block. + assertContains(t, r.Stderr, "→") + assertContains(t, r.Stderr, filepath.Base(repo)) // repo name + assertContains(t, r.Stderr, "frosted-jaguar") // worktree basename + assertContains(t, r.Stderr, "frosted-jaguar)") // branch (in parens; wt create names branch == worktree) + assertContains(t, r.Stderr, wtPath) // indented absolute path line +} + +// TestGo_NoWorktrees_NoConfirmation verifies the confirmation block is NOT +// emitted when there is nothing to navigate to (no non-main worktrees) — the +// arrow must only appear on the success path. +func TestGo_NoWorktrees_NoConfirmation(t *testing.T) { + repo := createTestRepo(t) + + // No extra worktrees created: selectWorktree finds zero options, prints + // "No worktrees found." and returns without navigating. + r := runWtSuccess(t, repo, nil, "go") + + assertContains(t, r.Stdout, "No worktrees found.") + assertNotContains(t, r.Stdout, "→") + assertNotContains(t, r.Stderr, "→") +} + // TestGo_NameArg_CaseInsensitive verifies name resolution is case-insensitive, // matching resolveWorktreeByName's contract shared with `wt open`. func TestGo_NameArg_CaseInsensitive(t *testing.T) { diff --git a/src/cmd/wt/list.go b/src/cmd/wt/list.go index c90860b..d6343fe 100644 --- a/src/cmd/wt/list.go +++ b/src/cmd/wt/list.go @@ -308,8 +308,10 @@ func handlePathLookup(name string, ctx *wt.RepoContext) error { } } - fmt.Fprintf(os.Stderr, "Worktree '%s' not found. Use 'wt list' to see available worktrees.\n", name) - os.Exit(wt.ExitGeneralError) + wt.ExitWithError(wt.ExitGeneralError, + fmt.Sprintf("Worktree '%s' not found", name), + "No worktree with that name in this repository", + "Use 'wt list' to see available worktrees") return nil } diff --git a/src/cmd/wt/list_test.go b/src/cmd/wt/list_test.go index 5624a0a..590b541 100644 --- a/src/cmd/wt/list_test.go +++ b/src/cmd/wt/list_test.go @@ -100,10 +100,14 @@ func TestList_PathNonexistent(t *testing.T) { repo := createTestRepo(t) r := runWt(t, repo, nil, "list", "--path", "nonexistent") - if r.ExitCode == 0 { - t.Error("expected failure for nonexistent worktree --path lookup") - } - assertContains(t, r.Stderr, "not found") + // Structured ExitWithError(ExitGeneralError, ...) — byte-parity with the + // open.go/go.go not-found case (what/why/fix on stderr, exit 1). + if r.ExitCode != 1 { + t.Fatalf("expected exit 1 (ExitGeneralError), got %d\nstderr: %s", r.ExitCode, r.Stderr) + } + assertContains(t, r.Stderr, "Error: Worktree 'nonexistent' not found") + assertContains(t, r.Stderr, "Why: No worktree with that name in this repository") + assertContains(t, r.Stderr, "Fix: Use 'wt list' to see available worktrees") } // --json flag tests diff --git a/src/cmd/wt/update.go b/src/cmd/wt/update.go index 20b4a70..20c3803 100644 --- a/src/cmd/wt/update.go +++ b/src/cmd/wt/update.go @@ -14,7 +14,7 @@ func updateCmd() *cobra.Command { var skipBrewUpdate bool cmd := &cobra.Command{ Use: "update", - Short: "self-update the wt binary via Homebrew", + Short: "Self-update the wt binary via Homebrew", Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { err := update.Run(skipBrewUpdate, version, cmd.OutOrStdout(), cmd.ErrOrStderr()) diff --git a/src/internal/worktree/errors.go b/src/internal/worktree/errors.go index f47f47b..fc2484e 100644 --- a/src/internal/worktree/errors.go +++ b/src/internal/worktree/errors.go @@ -109,6 +109,14 @@ func PrintError(what, why, fix string) { fmt.Fprintln(os.Stderr, WtError(what, why, fix)) } +// Warn writes a color-wrapped "Warning:" diagnostic to stderr. It is the single +// warning emitter for the CLI: all "Warning:" call sites route through it so the +// prefix, stream (stderr), and color are consistent. Respects the +// NO_COLOR-blanked package color vars (no fresh os.Getenv). +func Warn(format string, args ...any) { + fmt.Fprintf(os.Stderr, "%sWarning:%s %s\n", ColorYellow, ColorReset, fmt.Sprintf(format, args...)) +} + // ExitWithError prints a structured error and exits with the given code. func ExitWithError(code int, what, why, fix string) { PrintError(what, why, fix) diff --git a/src/internal/worktree/errors_test.go b/src/internal/worktree/errors_test.go index 68972ba..2fc8397 100644 --- a/src/internal/worktree/errors_test.go +++ b/src/internal/worktree/errors_test.go @@ -163,6 +163,42 @@ func TestWtError_WithColor(t *testing.T) { } } +func TestWarn_WithColor(t *testing.T) { + origYellow, origReset := ColorYellow, ColorReset + defer func() { ColorYellow, ColorReset = origYellow, origReset }() + ColorYellow, ColorReset = "\033[0;33m", "\033[0m" + + out := captureStderr(t, func() { + Warn("uncommitted changes in %s", "repo") + }) + + if !strings.Contains(out, "\033[0;33mWarning:\033[0m uncommitted changes in repo") { + t.Errorf("Warn should color-wrap the Warning: prefix and format args, got: %q", out) + } + if !strings.HasSuffix(out, "\n") { + t.Errorf("Warn must append a single trailing newline, got: %q", out) + } +} + +func TestWarn_NoColor(t *testing.T) { + // NO_COLOR state: package init() blanks the color vars. Replicate via the + // same save/blank/restore pattern the other tests use. + origYellow, origReset := ColorYellow, ColorReset + defer func() { ColorYellow, ColorReset = origYellow, origReset }() + ColorYellow, ColorReset = "", "" + + out := captureStderr(t, func() { + Warn("plain %d", 7) + }) + + if out != "Warning: plain 7\n" { + t.Errorf("NO_COLOR Warn should be plain %q, got: %q", "Warning: plain 7\n", out) + } + if strings.Contains(out, "\033[") { + t.Errorf("NO_COLOR Warn must contain no ANSI escapes, got: %q", out) + } +} + func TestPrintInitFailureBanner_ExitError(t *testing.T) { origRed, origBold, origReset := ColorRed, ColorBold, ColorReset defer func() { ColorRed, ColorBold, ColorReset = origRed, origBold, origReset }() From b8ad6e77640a43ec6b6b97399935382bc88705e7 Mon Sep 17 00:00:00 2001 From: Sahil Ahuja Date: Mon, 22 Jun 2026 14:30:54 +0530 Subject: [PATCH 2/4] Update ship status and record PR URL --- .../260622-log5-dx-copy-polish/.history.jsonl | 1 + fab/changes/260622-log5-dx-copy-polish/.status.yaml | 12 +++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/fab/changes/260622-log5-dx-copy-polish/.history.jsonl b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl index 27ec9dc..a85c4c3 100644 --- a/fab/changes/260622-log5-dx-copy-polish/.history.jsonl +++ b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl @@ -8,3 +8,4 @@ {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-06-22T08:54:02Z"} {"event":"review","result":"passed","ts":"2026-06-22T08:54:02Z"} {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-06-22T08:59:06Z"} +{"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-06-22T09:00:47Z"} diff --git a/fab/changes/260622-log5-dx-copy-polish/.status.yaml b/fab/changes/260622-log5-dx-copy-polish/.status.yaml index d48b80f..d7e247a 100644 --- a/fab/changes/260622-log5-dx-copy-polish/.status.yaml +++ b/fab/changes/260622-log5-dx-copy-polish/.status.yaml @@ -9,8 +9,8 @@ progress: apply: done review: done hydrate: done - ship: active - review-pr: pending + ship: done + review-pr: active plan: generated: true task_count: 15 @@ -33,8 +33,10 @@ stage_metrics: apply: {started_at: "2026-06-22T08:39:47Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:48:24Z"} review: {started_at: "2026-06-22T08:48:24Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:54:02Z"} hydrate: {started_at: "2026-06-22T08:54:02Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:59:06Z"} - ship: {started_at: "2026-06-22T08:59:06Z", driver: fab-fff, iterations: 1} -prs: [] + ship: {started_at: "2026-06-22T08:59:06Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T09:00:47Z"} + review-pr: {started_at: "2026-06-22T09:00:47Z", driver: git-pr, iterations: 1} +prs: + - https://github.com/sahil87/wt/pull/24 true_impact: added: 0 deleted: 0 @@ -47,4 +49,4 @@ true_impact: computed_at_stage: hydrate summary: 'DX copy polish: wt go stderr nav-confirmation, unified wt.Warn helper (delete warnings stdout→stderr), list --path structured not-found, update Short + help-text/glyph fixes' # true_impact: lazily created on first apply-finish (no placeholder here). -last_updated: 2026-06-22T08:59:06Z +last_updated: 2026-06-22T09:00:47Z From fc9ef4c0cc9e693daf30ee5bb395f943633de6ff Mon Sep 17 00:00:00 2001 From: Sahil Ahuja Date: Mon, 22 Jun 2026 14:39:45 +0530 Subject: [PATCH 3/4] fix: address review feedback from @Copilot --- docs/memory/wt-cli/create-output-phases.md | 6 +++--- docs/memory/wt-cli/go-command-contract.md | 5 +++-- src/cmd/wt/delete.go | 2 +- src/cmd/wt/go.go | 17 +++++++++++------ src/internal/worktree/errors.go | 9 ++++++--- 5 files changed, 24 insertions(+), 15 deletions(-) diff --git a/docs/memory/wt-cli/create-output-phases.md b/docs/memory/wt-cli/create-output-phases.md index 27c95da..aa8d8fd 100644 --- a/docs/memory/wt-cli/create-output-phases.md +++ b/docs/memory/wt-cli/create-output-phases.md @@ -52,11 +52,11 @@ This file documents the phase-separator output contract that `wt create` and `wt - `wt init` was **realigned** so ALL its init diagnostics go to stderr: the `Running worktree init...` / `Worktree init complete.` banner (now `fmt.Fprintln(os.Stderr, ...)`), the Init separator, AND the init script's own stdout (the init child is wired with `cmd.Stdout = os.Stderr` at `init.go:83`, joining `cmd.Stderr = os.Stderr`). `wt init` has **no machine-readable stdout result** — it is a side-effecting command whose outcome is its exit code — so all of its output is diagnostic. - The realignment changed only the stream (stdout → stderr) for `wt init`: exit-code behavior (`ExitInitFailed = 7` on script non-zero exit), graceful-skip behavior, and all output text are unchanged. Existing `wt init` tests assert on the combined `r.Stdout + r.Stderr`, so they stay green. -### `wt.Warn` is the single warning emitter; warnings are stderr-only (`260622-log5`) +### `wt.Warn` is the one-line warning helper; warnings are stderr-only (`260622-log5`) -- `src/internal/worktree/errors.go` exposes `func Warn(format string, args ...any)` alongside the other output helpers (`WtError`, `PrintError`, `PhaseSeparator`, `PrintInitFailureBanner`). It is the **single** `Warning:`-diagnostic producer for the CLI — it writes `"%sWarning:%s %s\n"` (`ColorYellow` prefix, `ColorReset`, then the formatted message) to **stderr**. +- `src/internal/worktree/errors.go` exposes `func Warn(format string, args ...any)` alongside the other output helpers (`WtError`, `PrintError`, `PhaseSeparator`, `PrintInitFailureBanner`). It is the **shared one-line** `Warning:`-diagnostic helper used by `create`/`delete` — it writes `"%sWarning:%s %s\n"` (`ColorYellow` prefix, `ColorReset`, then the formatted message) to **stderr**. (Exception: the verbose init not-found warning, `InitNotFound.RenderWarning` in `internal/worktree/init.go`, builds its own multi-line `Warning:` text and does **not** route through `Warn` — so `Warn` is not the CLI's sole `Warning:` producer.) - It is the warnings analogue of `PhaseSeparator`: color detection reuses the **blanked package-level color vars** (the `NO_COLOR`-blanking `init()` zeroes `ColorYellow`/`ColorReset`), so under `NO_COLOR` it emits a plain `Warning: \n` with **no** ANSI escapes. It MUST NOT call `os.Getenv` afresh. -- **All `Warning:` call sites route through `Warn`** — `create.go` (the uncommitted-changes warning and the open-phase "could not open in …" / reused-worktree-init warnings) and `delete.go` (the two `failed to remove …` warnings and the two pre-menu warnings). Call sites pass the **message text only**, never the literal `Warning: ` prefix — the helper owns the prefix, so re-including it would double it. +- **All one-line `Warning:` call sites route through `Warn`** — `create.go` (the uncommitted-changes warning and the open-phase "could not open in …" / reused-worktree-init warnings) and `delete.go` (the two `failed to remove …` warnings and the two pre-menu warnings). (The init not-found renderer noted above is the lone multi-line exception that stays out of band.) Call sites pass the **message text only**, never the literal `Warning: ` prefix — the helper owns the prefix, so re-including it would double it. - **`wt delete`'s two pre-menu warnings were realigned stdout → stderr.** The uncommitted-changes warning (`handleUncommittedChanges`) and the unpushed-commits warning (`handleUnpushedCommits`) previously printed via `fmt.Printf` (**stdout**); they now route through `wt.Warn` (→ **stderr**), matching the sibling warnings already on stderr in the same file. This is the warnings counterpart of the `wt init` stdout→stderr realignment above — same stream-discipline fix, different command. `wt delete` has no stdout machine contract, but warnings are diagnostics and belong on stderr. - **Menu-layout spacing is preserved at the call site, not in the helper.** `Warn` appends exactly one trailing `\n`. The two pre-menu warnings need a blank line before and after for menu spacing, so each call site frames the `wt.Warn(...)` with a bare `fmt.Fprintln(os.Stderr)` immediately before and after (reproducing the old `\n…\n\n` framing now that the whole block is on stderr). - Each warning's **existing wording is preserved** — the consolidation standardizes only the prefix, stream, and color, never the message text. diff --git a/docs/memory/wt-cli/go-command-contract.md b/docs/memory/wt-cli/go-command-contract.md index c3e9a80..fb15338 100644 --- a/docs/memory/wt-cli/go-command-contract.md +++ b/docs/memory/wt-cli/go-command-contract.md @@ -344,8 +344,9 @@ otherwise does not want). worktree, no-arg menu newest-first ordering, `wt open --go --app open_here`). - Sibling memory: `/wt-cli/create-output-phases.md` — the canonical stdout = machine-result / stderr = human-diagnostic stream-discipline contract this - confirmation honors; it also documents the `wt.Warn` single warning emitter - (`260622-log5`). + confirmation honors; it also documents the `wt.Warn` shared one-line warning + helper used by create/delete (the verbose init not-found renderer, + `InitNotFound.RenderWarning`, is the documented exception) (`260622-log5`). - Constitution: Principle II (Cobra command surface — `SilenceUsage`/ `SilenceErrors`, `RunE`), III (Typed exit codes — `ExitGitError` / `ExitGeneralError`, no new code), V (selection orchestration lives in `cmd/`; diff --git a/src/cmd/wt/delete.go b/src/cmd/wt/delete.go index f1720f8..6362eaf 100644 --- a/src/cmd/wt/delete.go +++ b/src/cmd/wt/delete.go @@ -137,7 +137,7 @@ Resolution order: --stale, --delete-all, positional args, --worktree-name (depre cmd.Flags().StringVar(&worktreeName, "worktree-name", "", "Worktree to delete") cmd.Flags().StringVar(&deleteBranch, "delete-branch", "", "Delete the associated branch: true (always), false (never), auto (default — only if branch name matches worktree name)") - cmd.Flags().StringVar(&deleteRemote, "delete-remote", "", "Delete the remote-tracking branch when the local branch is deleted (true by default)") + cmd.Flags().StringVar(&deleteRemote, "delete-remote", "", "Delete the branch on the origin remote (via git push origin --delete) when the local branch is deleted (true by default)") cmd.Flags().BoolVar(&deleteAll, "delete-all", false, "Delete all worktrees") cmd.Flags().BoolVarP(&stashFlag, "stash", "s", false, "Stash uncommitted changes before deleting") cmd.Flags().BoolVar(&nonInteractive, "non-interactive", false, "No prompts, use defaults") diff --git a/src/cmd/wt/go.go b/src/cmd/wt/go.go index bb28115..2e01229 100644 --- a/src/cmd/wt/go.go +++ b/src/cmd/wt/go.go @@ -124,12 +124,10 @@ Requires a git repository — worktree resolution walks the repo's worktree list // WT_CD_FILE semantics (mode 0600, truncate-on-write, contents = resolved dir // path) are the same ones documented in launcher-contract.md §3 for "Open here". func navigateTo(ctx *wt.RepoContext, path string) error { - // Confirmation block (stderr, human copy). getBranchForPath reuses the same - // single git rev-parse the open/go menus use. Emits no color, so it is - // NO_COLOR-safe by construction. - fmt.Fprintf(os.Stderr, "→ %s / %s (%s)\n", ctx.RepoName, filepath.Base(path), getBranchForPath(path)) - fmt.Fprintf(os.Stderr, " %s\n", path) - + // The WT_CD_FILE write is the operation that can still fail, so it runs + // BEFORE the success confirmation. Emitting the "→ repo / …" line first + // would print a misleading success message even when the write then errors + // out and exits non-zero. if cdFile := os.Getenv("WT_CD_FILE"); cdFile != "" { if err := os.WriteFile(cdFile, []byte(path), 0600); err != nil { wt.ExitWithError(wt.ExitGeneralError, @@ -142,6 +140,13 @@ func navigateTo(ctx *wt.RepoContext, path string) error { fmt.Fprintln(os.Stderr, ` Add it to your ~/.zshrc or ~/.bashrc to make it permanent.`) } + // Confirmation block (stderr, human copy). getBranchForPath reuses the same + // single git rev-parse the open/go menus use. Emits no color, so it is + // NO_COLOR-safe by construction. Printed only after the WT_CD_FILE write + // (above) has succeeded — there are no further error/exit conditions below. + fmt.Fprintf(os.Stderr, "→ %s / %s (%s)\n", ctx.RepoName, filepath.Base(path), getBranchForPath(path)) + fmt.Fprintf(os.Stderr, " %s\n", path) + // Always emit the resolved path as the last stdout line for the scripting // path: cd "$(command wt go some-name)". fmt.Println(path) diff --git a/src/internal/worktree/errors.go b/src/internal/worktree/errors.go index fc2484e..4ababa8 100644 --- a/src/internal/worktree/errors.go +++ b/src/internal/worktree/errors.go @@ -110,9 +110,12 @@ func PrintError(what, why, fix string) { } // Warn writes a color-wrapped "Warning:" diagnostic to stderr. It is the single -// warning emitter for the CLI: all "Warning:" call sites route through it so the -// prefix, stream (stderr), and color are consistent. Respects the -// NO_COLOR-blanked package color vars (no fresh os.Getenv). +// helper for one-line "Warning:" diagnostics in the CLI: the create/delete +// warning call sites route through it so the prefix, stream (stderr), and color +// are consistent. (The verbose init not-found warning, InitNotFound.RenderWarning +// in init.go, is the one exception — it builds its own multi-line "Warning:" text +// and does not route through this helper.) Respects the NO_COLOR-blanked package +// color vars (no fresh os.Getenv). func Warn(format string, args ...any) { fmt.Fprintf(os.Stderr, "%sWarning:%s %s\n", ColorYellow, ColorReset, fmt.Sprintf(format, args...)) } From 83d48c201025d28cb4f1c3db157bc1beb050addb Mon Sep 17 00:00:00 2001 From: Sahil Ahuja Date: Mon, 22 Jun 2026 14:40:25 +0530 Subject: [PATCH 4/4] Update review-pr status --- fab/changes/260622-log5-dx-copy-polish/.history.jsonl | 1 + fab/changes/260622-log5-dx-copy-polish/.status.yaml | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/fab/changes/260622-log5-dx-copy-polish/.history.jsonl b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl index a85c4c3..0ba3aa2 100644 --- a/fab/changes/260622-log5-dx-copy-polish/.history.jsonl +++ b/fab/changes/260622-log5-dx-copy-polish/.history.jsonl @@ -9,3 +9,4 @@ {"event":"review","result":"passed","ts":"2026-06-22T08:54:02Z"} {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-06-22T08:59:06Z"} {"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-06-22T09:00:47Z"} +{"event":"review","result":"passed","ts":"2026-06-22T09:10:25Z"} diff --git a/fab/changes/260622-log5-dx-copy-polish/.status.yaml b/fab/changes/260622-log5-dx-copy-polish/.status.yaml index d7e247a..227bc39 100644 --- a/fab/changes/260622-log5-dx-copy-polish/.status.yaml +++ b/fab/changes/260622-log5-dx-copy-polish/.status.yaml @@ -10,7 +10,7 @@ progress: review: done hydrate: done ship: done - review-pr: active + review-pr: done plan: generated: true task_count: 15 @@ -34,7 +34,7 @@ stage_metrics: review: {started_at: "2026-06-22T08:48:24Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:54:02Z"} hydrate: {started_at: "2026-06-22T08:54:02Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T08:59:06Z"} ship: {started_at: "2026-06-22T08:59:06Z", driver: fab-fff, iterations: 1, completed_at: "2026-06-22T09:00:47Z"} - review-pr: {started_at: "2026-06-22T09:00:47Z", driver: git-pr, iterations: 1} + review-pr: {started_at: "2026-06-22T09:00:47Z", driver: git-pr, iterations: 1, completed_at: "2026-06-22T09:10:25Z"} prs: - https://github.com/sahil87/wt/pull/24 true_impact: @@ -49,4 +49,4 @@ true_impact: computed_at_stage: hydrate summary: 'DX copy polish: wt go stderr nav-confirmation, unified wt.Warn helper (delete warnings stdout→stderr), list --path structured not-found, update Short + help-text/glyph fixes' # true_impact: lazily created on first apply-finish (no placeholder here). -last_updated: 2026-06-22T09:00:47Z +last_updated: 2026-06-22T09:10:25Z