fix: DX copy polish — wt go stderr nav-confirmation, unified wt.Warn, list --path structured not-found, help-text + glyph fixes#24
Conversation
…helper, list --path structured not-found, update Short capitalization, help-text + glyph fixes
There was a problem hiding this comment.
Pull request overview
This PR polishes wt CLI UX/copy consistency by standardizing warning output, adding a human-readable navigation confirmation for wt go (on stderr only), and aligning a few commands/flags with existing structured error and help-text conventions.
Changes:
- Add
wt.Warn(...)helper and routecreate/deletewarnings through it (including moving twodeletewarnings from stdout → stderr). - Add
wt gostderr navigation confirmation while keeping stdout’s bare-path contract unchanged. - Use
ExitWithErrorstructured not-found output forwt list --path, plus help-text/copy tweaks and contract documentation updates.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internal/worktree/errors.go | Adds Warn(...) helper for consistent warning formatting/stream. |
| src/internal/worktree/errors_test.go | Adds unit tests for Warn (colored + NO_COLOR). |
| src/cmd/wt/go.go | Adds stderr navigation confirmation and threads RepoContext into navigateTo. |
| src/cmd/wt/go_test.go | Tests that confirmation is stderr-only and stdout stays bare-path. |
| src/cmd/wt/list.go | Switches --path not-found to structured ExitWithError. |
| src/cmd/wt/list_test.go | Updates test to assert structured what/why/fix error shape + exit code. |
| src/cmd/wt/create.go | Converts warnings to wt.Warn and rewrites some flag help text. |
| src/cmd/wt/delete.go | Converts warnings to wt.Warn, moves two warnings stdout→stderr, rewrites help text + glyph message. |
| src/cmd/wt/delete_test.go | Adds regression test ensuring uncommitted-changes warning is on stderr. |
| src/cmd/wt/update.go | Capitalizes update command Short help text. |
| docs/memory/wt-cli/list-status-contract.md | Documents structured not-found behavior for list --path. |
| docs/memory/wt-cli/go-command-contract.md | Documents new wt go stderr confirmation + signature change. |
| docs/memory/wt-cli/create-output-phases.md | Documents wt.Warn/stream-discipline updates. |
| fab/changes/260622-log5-dx-copy-polish/plan.md | Planning/acceptance details for the change. |
| fab/changes/260622-log5-dx-copy-polish/intake.md | Intake rationale/scope for the change. |
| fab/changes/260622-log5-dx-copy-polish/.status.yaml | Pipeline status metadata. |
| fab/changes/260622-log5-dx-copy-polish/.history.jsonl | Pipeline history log. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 { |
There was a problem hiding this comment.
Fixed — moved the "→ repo / …" stderr confirmation to after the WT_CD_FILE write succeeds, so a write failure no longer prints a misleading success line. (fc9ef4c)
| // 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). |
There was a problem hiding this comment.
Fixed — narrowed the Warn doc comment to "single helper for one-line Warning diagnostics" (create/delete) and called out InitNotFound.RenderWarning as the multi-line exception that does not route through it. (fc9ef4c)
| ### `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: <msg>\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. |
There was a problem hiding this comment.
Fixed — retitled the section to the one-line warning helper, scoped Warn to the create/delete sites, and explicitly noted InitNotFound.RenderWarning (init.go) as the exception so Warn is no longer described as the CLI's sole Warning: producer. (fc9ef4c)
| - 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`). |
There was a problem hiding this comment.
Fixed — reworded the cross-reference to describe wt.Warn as the shared one-line warning helper used by create/delete, with InitNotFound.RenderWarning noted as the documented exception. (fc9ef4c)
| 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)") |
There was a problem hiding this comment.
Fixed — updated the --delete-remote help text to say it deletes the branch on the origin remote (via git push origin --delete), matching DeleteRemoteBranch in internal/worktree/git.go rather than implying a local remote-tracking ref. (fc9ef4c)
Meta
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Impact: +162/−27 code (excluding
fab/,docs/) · +743/−46 totalSummary
The
wtCLI copy was well-calibrated overall but had real inconsistencies surfaced by a full DX/copy audit:wt gogave no human confirmation of where it was navigating, warning output was inconsistent across commands (some color-wrapped to stderr, some bare inline, two incorrectly to stdout), andwt list --path's not-found path bypassed the structuredExitWithErrorform used elsewhere. This change addresses all Tier 1 and Tier 2 findings from that audit in one coherent, surgical fix.Changes
wt gostderr navigation confirmation — compact-arrow form (→ {repo} / {worktree} ({branch})+ indented path) emitted to stderr on success; stdout stays byte-identical (bare path only, preservingcd "$(command wt go)"andWT_CD_FILEcontracts)wt.Warnhelper — newWarn(format, args...)inerrors.go(alongsideWtError/RenderWarning); all ~10 warning call sites acrosscreate.go,delete.goconverted; twodelete.gowarnings moved from stdout to stderr (stream-discipline fix)wt list --pathnot-found →ExitWithError— replaces ad-hocfmt.Fprintf+os.Exitwith the structuredwhat/why/fixform used byopen/gowt updateShortcapitalization —"self-update…"→"Self-update…"(matches all other commands)--worktree-open,--non-interactive,--delete-branch,--delete-remoteflag descriptions rewritten for clarity;≠glyph replaced with plain English in the skipped-branch-deletion message