From 44607688d5488acdba4cc2530cf2d180adf078ed Mon Sep 17 00:00:00 2001 From: Umputun Date: Sun, 13 Sep 2026 23:08:19 -0500 Subject: [PATCH] build: move main package to app/revdiff so go install yields revdiff `go install` produced a binary named `app`, because Go names it after the last path element. All other packages stay under `app/`. go installs skip the Makefile ldflags, so `--version` printed `unknown` on that path. It now uses the module version from build info when no revision was injected. Fix #359 --- .../skills/revdiff/references/install.md | 8 +++++++ .claude/rules/gotchas.md | 8 +++---- .goreleaser.yml | 2 +- .zed/tasks.json | 4 ++-- CLAUDE.md | 7 +++--- Makefile | 2 +- README.md | 8 +++++++ app/{ => revdiff}/annotations_load.go | 0 app/{ => revdiff}/annotations_load_test.go | 0 app/{ => revdiff}/compare.go | 0 app/{ => revdiff}/compare_test.go | 0 app/{ => revdiff}/config.go | 0 app/{ => revdiff}/config_test.go | 0 app/{ => revdiff}/history_save.go | 0 app/{ => revdiff}/main.go | 14 ++++++++++- app/{ => revdiff}/main_test.go | 24 +++++++++++++++++++ app/{ => revdiff}/plugin_exit_code_test.go | 3 ++- app/{ => revdiff}/quitter_moq_test.go | 0 app/{ => revdiff}/renderer_setup.go | 0 app/{ => revdiff}/renderer_setup_test.go | 0 app/{ => revdiff}/reviewinfo.go | 0 app/{ => revdiff}/reviewinfo_test.go | 0 app/{ => revdiff}/signal.go | 0 app/{ => revdiff}/signal_test.go | 0 app/{ => revdiff}/stdin.go | 0 app/{ => revdiff}/stdin_test.go | 0 app/{ => revdiff}/termbg.go | 0 app/{ => revdiff}/termbg_test.go | 0 .../plugin-exit-code/fake-overlay-backend.sh | 0 .../plugin-exit-code/fake-revdiff-output.sh | 0 .../plugin-exit-code/fake-stdout-launcher.sh | 0 ...-2026-07-16T10-54-26-session-current.jsonl | 0 .../testdata/themes/corrupted.ini | 0 .../testdata/themes/duplicate.ini | 0 app/{ => revdiff}/testdata/themes/good.ini | 0 .../testdata/themes/no_theme.ini | 0 app/{ => revdiff}/themes.go | 0 app/{ => revdiff}/themes_test.go | 2 +- app/ui/doc.go | 8 +++---- docs/ARCHITECTURE.md | 20 ++++++++-------- flake.nix | 8 +------ .../skills/revdiff/references/install.md | 8 +++++++ site/docs.html | 3 +++ 43 files changed, 94 insertions(+), 35 deletions(-) rename app/{ => revdiff}/annotations_load.go (100%) rename app/{ => revdiff}/annotations_load_test.go (100%) rename app/{ => revdiff}/compare.go (100%) rename app/{ => revdiff}/compare_test.go (100%) rename app/{ => revdiff}/config.go (100%) rename app/{ => revdiff}/config_test.go (100%) rename app/{ => revdiff}/history_save.go (100%) rename app/{ => revdiff}/main.go (97%) rename app/{ => revdiff}/main_test.go (93%) rename app/{ => revdiff}/plugin_exit_code_test.go (99%) rename app/{ => revdiff}/quitter_moq_test.go (100%) rename app/{ => revdiff}/renderer_setup.go (100%) rename app/{ => revdiff}/renderer_setup_test.go (100%) rename app/{ => revdiff}/reviewinfo.go (100%) rename app/{ => revdiff}/reviewinfo_test.go (100%) rename app/{ => revdiff}/signal.go (100%) rename app/{ => revdiff}/signal_test.go (100%) rename app/{ => revdiff}/stdin.go (100%) rename app/{ => revdiff}/stdin_test.go (100%) rename app/{ => revdiff}/termbg.go (100%) rename app/{ => revdiff}/termbg_test.go (100%) rename app/{ => revdiff}/testdata/plugin-exit-code/fake-overlay-backend.sh (100%) rename app/{ => revdiff}/testdata/plugin-exit-code/fake-revdiff-output.sh (100%) rename app/{ => revdiff}/testdata/plugin-exit-code/fake-stdout-launcher.sh (100%) rename app/{ => revdiff}/testdata/plugin-exit-code/rollout-2026-07-16T10-54-26-session-current.jsonl (100%) rename app/{ => revdiff}/testdata/themes/corrupted.ini (100%) rename app/{ => revdiff}/testdata/themes/duplicate.ini (100%) rename app/{ => revdiff}/testdata/themes/good.ini (100%) rename app/{ => revdiff}/testdata/themes/no_theme.ini (100%) rename app/{ => revdiff}/themes.go (100%) rename app/{ => revdiff}/themes_test.go (99%) diff --git a/.claude-plugin/skills/revdiff/references/install.md b/.claude-plugin/skills/revdiff/references/install.md index 8780781c..c859d090 100644 --- a/.claude-plugin/skills/revdiff/references/install.md +++ b/.claude-plugin/skills/revdiff/references/install.md @@ -5,6 +5,14 @@ brew install umputun/apps/revdiff ``` +**Go:** + +```bash +go install github.com/umputun/revdiff/app/revdiff@latest +``` + +Installs the `revdiff` binary into `GOBIN` (defaults to `$(go env GOPATH)/bin`). Add that directory to your `PATH`. + **Binary releases:** download from [GitHub Releases](https://github.com/umputun/revdiff/releases) (deb, rpm, archives for linux/darwin amd64/arm64). ## Claude Code Plugin diff --git a/.claude/rules/gotchas.md b/.claude/rules/gotchas.md index 5ad3754e..dcd6c032 100644 --- a/.claude/rules/gotchas.md +++ b/.claude/rules/gotchas.md @@ -8,7 +8,7 @@ - `setupVCSRenderer()` (in `renderer_setup.go`) detects VCS via `diff.DetectVCS()` (walks up looking for `.jj`, `.git`, `.hg` — in that precedence order; `.jj` wins over `.git` in colocated repos); if no VCS is found and `--only` is set, uses `FileReader` for standalone file review. `--stdin` skips VCS lookup entirely, validates non-TTY stdin, reads payload before starting Bubble Tea, and reopens `/dev/tty` for interactive key input. `--all-files` is supported for git and jj; not supported in hg. `--compare-old/--compare-new` also skips VCS lookup — `run()` branches before `setupVCSRenderer`, sets `renderer = diff.NewCompareReader(old, new)` and `workDir = filepath.Dir(abs(newPath))` directly; `gitRoot`, `blamer`, `untrackedFn`, and `commitLogger` remain nil. - `--all-files` mode uses `DirectoryReader` (git ls-files) to list all tracked files; `--include` wraps any renderer with `IncludeFilter` for prefix-based inclusion, `--exclude` wraps with `ExcludeFilter` for prefix-based exclusion (include narrows first, then exclude removes). Those wrappers only filter `ChangedFiles`; untracked files come straight from the VCS `UntrackedFiles` call and bypass the renderer chain, so `filterUntracked` (in `renderer_setup.go`) re-applies the same prefixes via `diff.FilterPaths` at the composition root — a future change to include/exclude filtering must touch both paths or untracked scoping silently re-breaks. `--include` is mutually exclusive with `--only`. `--all-files` is mutually exclusive with refs, `--staged`, and `--only`. `--stdin` is mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--include`, and `--exclude`. `--compare-old/--compare-new` is mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--stdin`, `--include`, `--exclude`, and `--annotations`. - Rename-aware diffs (git-only): `FileDiff` takes a single `diff.FileDiffRequest` value (`Ref`, `Path`, `OldPath`, `Staged`, `ContextLines`) — not positional args — across all three interfaces (`diff.Renderer`, `ui.Renderer`, `review.FileDiffer`) and every implementation/wrapper. `FileEntry.OldPath` is the rename origin (empty for non-renames), populated only by `(*Git).ChangedFiles` from git's `R old new` / `C` pairs. `(*Git).pathArgs(req)` emits `-M -- ` when `OldPath != "" && OldPath != Path` so git pairs the rename into a minimal diff (otherwise `-- `); the old-side probes `totalOldLines`/`binarySizeDesc` read `OldPath` when set. hg and jj never set `OldPath` (jj decomposes renames into delete+add; hg `status` reports `R` as *removed*), so non-git renderers ignore it — no behavior change there. UI: `sidepane.FileTree` exposes `OldPath(path)` (parallel to `FileStatus(path)`, populated in `NewFileTree` + `Rebuild`); `fetchEffectiveFileDiff` threads it into the request, and `view.go` renders `old → new` in the diff-pane header via `loadedFileState.oldName` / `fileLoadedMsg.oldName` (still passed through `truncateHeaderTitle`). Review stats (`ComputeStats`) and the annotations preloader (`lookupLineSet`) also set `OldPath` so their per-file diffs match the displayed rename-aware diff. -- Untracked renames (git-only): a plain `mv old new` (no `git mv`/staging) leaves `old` as an unstaged deletion and `new` untracked, so `git diff -M` never pairs them — `(*Git).ChangedFiles` reports only `D old`. `(*Git).UntrackedRenames(untracked)` recovers the pairing off a **throwaway index**: `(*Git).tempIndexWithIntentToAdd` copies `.git/index` to a temp file (resolved via `git rev-parse --git-path index`), runs `git add -N -- ` against the copy with `GIT_INDEX_FILE` set (via `runGitEnv`/`runVCSEnv`), then `git diff --name-status -M` reports the pairs as renames; the real index and working tree are never touched. It returns `FileEntry{Status: FileRenamed, Path: new, OldPath: old}` only for renames whose new side is in the passed (already include/exclude-filtered) untracked set. Wiring: `vcsSetup.untrackedRenamesFn` (git case only → `g.UntrackedRenames`, nil for hg/jj) → `ModelConfig.LoadUntrackedRenames` → `Model.loadUntrackedRenames`. `loadFiles` merges via `detectUntrackedRenames` (gated to unstaged working-tree mode: `m.cfg.ref == "" && !m.cfg.staged`) + `mergeUntrackedEntries`, which drops the standalone `D old` and skips the new path from the plain-untracked append. `FileDiff` renders these via `untrackedRenameDiff` (same throwaway-index trick), selected by `isUntrackedRename(req)` = `Ref == "" && !Staged && OldPath != "" && OldPath != Path` — the only producer of `OldPath` in unstaged mode is `UntrackedRenames`, so that condition uniquely identifies the case. `(*Git).parseNameStatusEntries` is the shared NUL-field name-status parser for both `ChangedFiles` and `UntrackedRenames`. All git calls in this path use `(*Git).renameIndexEnv` which sets `GIT_INDEX_FILE` **and** `GIT_LITERAL_PATHSPECS=1` so a working-tree filename that looks like pathspec magic (e.g. `:(top)x`) is treated literally rather than as a pathspec. A fresh repo with no commits has no `.git/index`; `UntrackedRenames` treats that `fs.ErrNotExist` as "no renames possible" and returns `nil, nil`. The `--annotations` preloader (`app/annotations_load.go`) mirrors this: `preloadAnnotations` takes the same `untrackedRenamesFn`, and `(*preloader).foldUntrackedRenames` upgrades untracked entries to `FileRenamed` + records `OldPath` + drops the origin deletion under the same gate, so `lookupLineSet` resolves the rename-aware diff and `(-)`/context annotations on untracked renames round-trip (without it the preloader read `new` as all-added and dropped them). +- Untracked renames (git-only): a plain `mv old new` (no `git mv`/staging) leaves `old` as an unstaged deletion and `new` untracked, so `git diff -M` never pairs them — `(*Git).ChangedFiles` reports only `D old`. `(*Git).UntrackedRenames(untracked)` recovers the pairing off a **throwaway index**: `(*Git).tempIndexWithIntentToAdd` copies `.git/index` to a temp file (resolved via `git rev-parse --git-path index`), runs `git add -N -- ` against the copy with `GIT_INDEX_FILE` set (via `runGitEnv`/`runVCSEnv`), then `git diff --name-status -M` reports the pairs as renames; the real index and working tree are never touched. It returns `FileEntry{Status: FileRenamed, Path: new, OldPath: old}` only for renames whose new side is in the passed (already include/exclude-filtered) untracked set. Wiring: `vcsSetup.untrackedRenamesFn` (git case only → `g.UntrackedRenames`, nil for hg/jj) → `ModelConfig.LoadUntrackedRenames` → `Model.loadUntrackedRenames`. `loadFiles` merges via `detectUntrackedRenames` (gated to unstaged working-tree mode: `m.cfg.ref == "" && !m.cfg.staged`) + `mergeUntrackedEntries`, which drops the standalone `D old` and skips the new path from the plain-untracked append. `FileDiff` renders these via `untrackedRenameDiff` (same throwaway-index trick), selected by `isUntrackedRename(req)` = `Ref == "" && !Staged && OldPath != "" && OldPath != Path` — the only producer of `OldPath` in unstaged mode is `UntrackedRenames`, so that condition uniquely identifies the case. `(*Git).parseNameStatusEntries` is the shared NUL-field name-status parser for both `ChangedFiles` and `UntrackedRenames`. All git calls in this path use `(*Git).renameIndexEnv` which sets `GIT_INDEX_FILE` **and** `GIT_LITERAL_PATHSPECS=1` so a working-tree filename that looks like pathspec magic (e.g. `:(top)x`) is treated literally rather than as a pathspec. A fresh repo with no commits has no `.git/index`; `UntrackedRenames` treats that `fs.ErrNotExist` as "no renames possible" and returns `nil, nil`. The `--annotations` preloader (`app/revdiff/annotations_load.go`) mirrors this: `preloadAnnotations` takes the same `untrackedRenamesFn`, and `(*preloader).foldUntrackedRenames` upgrades untracked entries to `FileRenamed` + records `OldPath` + drops the origin deletion under the same gate, so `lookupLineSet` resolves the rename-aware diff and `(-)`/context annotations on untracked renames round-trip (without it the preloader read `new` as all-added and dropped them). - jj paths are fileset expressions, not literal paths (issue #341): `jj diff` and `jj file show` parse post-`--` arguments as jj's fileset query language, so `$ ( ) : #` fail to parse and `* ? & | ~` silently resolve to a *different* set of files — the latter concatenates other files' diffs into one file's view, which `parseUnifiedDiff` absorbs as bogus context/add/remove rows under a correct-looking filename. Every path handed to those two commands goes through `(*Jj).pathArg`, which wraps it as `cwd-file:""` (backslash and double-quote escaped). This is the jj analogue of git's `GIT_LITERAL_PATHSPECS=1`. Unconditional, because **revdiff's jj floor is 0.27** (README). Measured against official binaries: blame is broken throughout 0.20-0.26 (`jj file annotate` absent before 0.23, present but without `-T` through 0.26), and the commit-info popup additionally fails before 0.23 because the `\x00` escapes in `jjCommitLogTemplate` do not parse. So at least one advertised feature is broken on every jj below 0.27, though the core diff pane itself works further back. 0.27 is also where the `ui.allow-filesets` opt-out was removed, so every *supported* jj parses filesets and the quoting needs no capability probe or version branch. Do not add one unless the floor is deliberately lowered below 0.27. **`cwd-file:` not `root-file:`** — jj runs with `cmd.Dir = workDir` and emits paths relative to it, so `cwd-file:` resolves whatever `ChangedFiles` reported. `root-file:` rejects an absolute path outright, which regressed `--only=/abs/path` (the documented Zed task passes `$ZED_FILE`, always absolute) from a working context-only fallback to `error loading diff`; it also rejects a `../` sibling and silently matches the wrong file for a path below a `workDir` that is not the repo root. **Do not apply it to `jj file annotate`** (`jjblame.go`) — that takes a genuine path and rejects a fileset with `No such path`; likewise `jj file list` (`directory.go`) takes no path args. hg passes literal paths (it needs an explicit `glob:`/`re:` prefix) and needs no equivalent. - `diff.readReaderAsContext()` is the shared parser for file-backed and stdin-backed context-only views. Preserve its behavior if you change binary detection, line-length handling, or line numbering. - Overlay popups managed by `overlay.Manager`. `Compose()` uses ANSI-aware compositing via `charmbracelet/x/ansi.Cut`. `HandleKey()` returns `Outcome` — Model switches on `OutcomeKind` for side effects (file jumps, theme apply/persist). Overlay kinds: help, annot-list, theme-select, info, file-picker. One overlay at a time — opening any overlay auto-closes whichever was previously open @@ -24,10 +24,10 @@ - `renderDiff()` is O(current file's diff lines) — it rebuilds the full styled string for every line even though only the viewport's rows are visible. Measured on an M1 Ultra at 160x44 with the default palette: ~6.7us and ~10 allocs per line, so 3.5ms at 500 lines, 14ms at 2k, 72ms at 10k, 337ms at 50k. `View()` by contrast is flat (~1.2ms at every size), so any per-event cost that scales with diff size is a `SetContent(m.renderDiff())` call, not the pane/viewport layer. Benchmarks: `BenchmarkModel_RenderDiff` / `BenchmarkModel_View` (`diffview_test.go`), `BenchmarkModel_AnnotationKeystroke` / `BenchmarkModel_AnnotationBlinkTick` (`annotate_test.go`); `benchModel` there must keep both the `filesLoadedMsg` and the seq-matched `fileLoadedMsg`, or `View` short-circuits to `"loading files..."` and `renderDiff` to `" no changes"` and the numbers silently become meaningless. **Per-line render cache**: The key holds `contentWidth`, the **resolved** `diffContentWidth()`, never the raw `layout.width`/`treeWidth` it comes from: that function branches on `treePaneHidden()` (`treeHidden || (singleFile && mdTOC == nil)`), so keying the raw inputs let a `t` press that leaves `treeWidth` at 0 change every line's cut and pad width without moving the key. `renderDiff` memoizes each line's rendered block (the diff line plus any annotation rows below it) on `Model.renderCache`, a `*diffRenderCache`. Post-cache the same measurements are 0.23ms at 500 lines, 0.92ms at 2k, 4.5ms at 10k, 25ms at 50k, with **4 allocs per render** regardless of size and regardless of how many annotations the file carries — `buildAnnotationMap` is keyed on the comparable `annotLineKey` precisely because `lineRenderFlags` runs per line on every render including pure cache hits, and the string form (`annotationKey`'s `Sprintf`) allocated per line for any file with at least one annotation. `BenchmarkModel_AnnotatedKeystroke` is what covers that case; the other benchmarks add no annotations. Any benchmark or test that assigns `file.highlighted` after a load must call `invalidateRenderCaches()` or it silently measures cached unhighlighted blocks — the same production obligation, not a harness quirk. The cache is held behind a **pointer on purpose**: `renderDiff` has a value receiver, so a plain field would memoize into a copy that is thrown away — the pointer is what lets every Model copy share one instance (same reason `annot.rowCache` works, a map being a reference type). Sharing across copies is safe because entries are keyed by the state they were rendered under, so a block written by a copy that was later discarded is still correct for any copy whose key matches. Invalidation is in two halves and BOTH matter: comparable state lives in `globalRenderKey` and self-invalidates (add a field there for any new render input, or stale rows paint after that state moves), while **four** inputs that are not comparable — the style resolver, `file.blameData`, `file.highlighted` and `file.intraRanges` — invalidate only through `invalidateRenderCaches()` (renamed from `invalidateAnnotationRows`, moved to `diffview.go` beside the cache; it clears both memos, and `handleBlameLoaded` and `refreshDiff` call it). The last two are covered *today* only by where they happen to be mutated (`loadSeq`/`fileName` for `handleFileLoaded`, the `wordDiff` key field for `toggleWordDiff`) — that is not a guarantee, so **any new path that re-highlights or recomputes intra-line ranges owes an explicit `invalidateRenderCaches()`**. `refreshDiff` is the shape to copy: it reassigns `file.highlighted` and invalidates itself rather than trusting its caller. The blame gutter renders a relative age, so the key carries a one-minute bucket when blame is on; blame-off sessions never pay it. `lineRenderFlags` returns early when the annotation map is empty — without that guard `annotationKey`'s `fmt.Sprintf` ran per line and cost ~3 allocs/line on what was otherwise a pure cache hit. The live annotation input row is never cached (textinput state is not reducible to a comparable key). Two tests guard this and they are **not** redundant: `TestModel_RenderDiffGolden` pins exact bytes but always renders on a fresh model, so it cannot catch a stale cache; `TestModel_RenderDiffCacheMatchesColdRender` reaches each state on a model whose cache was dirtied by a *different* state, which is the only shape that catches a key gap. **That second test is not a completeness proof and must not be described as one** — it reaches exactly as far as the `goldenStates` matrix, and a field is covered only when two states differ in it *and* still produce a cache hit. `search.term` was missing from the key and the test passed anyway, because the single search state made every pair flip `searchMatch` and miss; a review caught it, the matrix did not. Adding a field to `globalRenderKey` means adding a state that isolates it, or the blind spot just moves. **Do not read the matrix as broad coverage**: of the key's fields it isolates only `contentWidth`, `scrollX`, `wrap`, `lineNumbers`, `wordDiff`, `noColors` and `searchTerm`. The rest are unisolated — never varied at all (`lineNumWidth`, `singleColLineNum`, `tabSpaces`, `annotPrefix`, `annotFilePrefix`, `fileName`, `loadSeq`), varied only in tandem with another key field so either one alone still misses (`narrow-pane` moves width AND treeWidth; `blame` moves showBlame AND blameAuthorLen), or varied without discriminating (`blameMinute`, since cold and warm renders share a minute bucket). `lineRenderFlags.comment` is unisolated too: no two states put different comment text on the same line. The golden's blame times are deliberately **relative** (`time.Now().Add(-3h30m)`, always rendering `3h`) because `RelativeAge` buckets by whole hours under 24h — an absolute instant bakes the hour of generation into the fixture and goes red at the next boundary. Remaining per-keystroke cost at 10k lines is ~17ms, of which ~7.4ms is `viewport.SetContent` → `findLongestLineWidth` doing an `ansi.StringWidth` grapheme scan of the whole content; revdiff cannot use that result (it does its own horizontal scroll via `layout.scrollX` + `applyHorizontalScroll`, truncating each line to pane width, and never sets viewport's `xOffset`) but cannot avoid it while handing bubbles a full content string. **Key-driven navigation still rebuilds the whole content string** — only the wheel path is debounced (see issue #179 above). The annotation input is painted inside `renderDiff`, so it cannot repaint without a re-render: the input's textinput uses `cursor.CursorStatic` (set in `newAnnotationInput` **before** `ti.Focus()`, or Focus schedules a blink cmd anyway) so no `BlinkMsg` timer exists to force one twice a second, and the annotating branch in `Update` re-renders only when `m.annot.input.Value()` actually changed — the guard that keeps ctrl+v paste repainting. Consequence for tests: starting an annotation returns a **nil** command; roughly a dozen tests assert exactly that. - Reload (`R` key): `reloadState` on `Model` holds `pending bool` (waiting for y/cancel), `hint string` (transient status-bar message), and `applicable bool` (false in `--stdin` mode — stream consumed). `ReloadApplicable` is wired at the composition root in `main.go`, following the same pattern as `CommitsApplicable`. The reload method is named `triggerReload()` — not `reload()` — because Go forbids a method and a field with the same name on the same type (`Model.reload` is the state field). Reload resets the diff cursor to the top of the file; tree selection (which file) is restored by `SelectByPath` in `handleFilesLoaded`. Reviewed files store a versioned semantic SHA-256 fingerprint over status, rename metadata, and ordered changed content; line numbers, context rows, and compact dividers are excluded so rebases that only shift a hunk retain the mark. `loadFiles` re-fetches only previously reviewed paths with at most four workers and reconciles the captured snapshot atomically. Pending async marks survive a file-list refresh only when their path remains present. Binary and placeholder rows are not safe to fingerprint from rendered text and are conservatively unmarked on reload. - Output flush (`O` key, `flush_output` action): `Model.handleFlushOutput` (in `app/ui/output.go`) exports current annotations without exiting through `m.cfg.outputPath`, `m.postFlushHook`, or both. An empty store is checked first (`"No annotations to flush"`); a non-empty store with neither target gets `"Output flush requires -o/--output or --post-flush-command"`. The file writer remains store-owned rather than injected: `annotation.Store.WriteFile(path)` does `FormatOutput()` then delegates to `fsutil.AtomicWriteFile` (atomic temp-file+rename, 0o600). Hook-only mode calls `Store.FormatOutput()` directly; combined mode reuses the exact string returned by `WriteFile`, so the file and hook cannot drift. The exit-time `writeAnnotationOutput` file branch calls `fsutil.AtomicWriteFile` directly with the already-computed output string, so file flush and exit share the same atomic writer. Flush is a pure export: the store is never mutated, so annotations persist in-session and re-flush uses the full current set. Clearing stays exclusively `R` reload's job (`store.Clear()`). `--post-flush-command` / `REVDIFF_POST_FLUSH_COMMAND` / `post-flush-command` wires `handoff.Runner` through `ui.PostFlushHook`; the hook reads the snapshot on stdin through `tea.ExecProcess`, with stdout suppressed and stderr plus `/dev/tty` available. Hook failure never rolls back a file written by the same flush. Completion restores mouse tracking when the session enabled it. File-backed hints retain their existing text; hook-only hints name the post-flush command without claiming a file write. -- Signal-safe save (`app/signal.go` + `finalize()` in `main.go`): `run()` appends `tea.WithoutSignalHandler()` so bubbletea does not install its own SIGINT/SIGTERM handler, then `shutdownGuard.watch(p)` installs one covering SIGHUP/SIGTERM that calls `p.Quit()` exactly once (`sync.Once`) and flips an `atomic.Bool`. SIGINT is registered on the same channel via `Notify` but drained by `handle()` without quitting (a `continue` on `syscall.SIGINT`) so a Ctrl-C the user meant for an external `$EDITOR` (delivered in cooked mode while `tea.ExecProcess` owns the terminal) does not quit revdiff — registering it via `Notify` (not `signal.Ignore`) is what suppresses its default terminate disposition, which would otherwise hard-kill with no save. Trade-off: `kill -INT` no longer quits revdiff (`kill -TERM` still does). The flag (`wasSignaled()`) is read only after `p.Run()` joins, so the unsynchronized annotation store is never touched off the main goroutine (no lock needed). The `watch()` stop func is idempotent (`sync.Once`) and restores default disposition for all three via `signal.Stop`+`close` (`signal.Reset` does NOT undo a `signal.Ignore`, which is why SIGINT goes through `Notify`+drain rather than `signal.Ignore`); `run()` calls it explicitly **before** `finalize` (and via `defer` as a panic net) so a slow or hung finalize (`saveHistory` shells out to git) stays killable by a second signal instead of being caught-and-swallowed. The `p.Run()` tail routes through `finalize(finalizeReq{...})`: discarded or empty output → write nothing; otherwise `saveHistory` always runs (safety net) and a signal-driven exit stops there — history only, never the `-o` output. **SIGTERM semantic change**: a signal-delivered SIGTERM used to reach bubbletea's own handler → QuitMsg → wrote BOTH history and the `-o` output; now (like SIGHUP) it is a safety-net save — history only, no `-o` handoff, because a signal is not a deliberate handoff. Typed interactive `^C` is unaffected (bubbletea raw mode has ISIG off, so `^C` is a keystroke, not a signal; revdiff binds no `ctrl+c` action). Recovery is the existing "load newest from `~/.config/revdiff/history/`" flow. Separately, `REVDIFF_TMUX_WINDOW=1` (a launcher env var read in `agentdeck-window.sh`, both plugin copies) promotes the existing agent-deck tmux window backend to a first-class interactive mode: a server-owned `tmux new-window` survives a client disconnect (SSH drop / VPN expiry) where a `display-popup` cannot. The explicit opt-in is captured in `_rd_focus` **before** `_rd_winmode` folds user-opt and agent-deck auto-detection together — only the opt-in opens the window focused and restores the prior active window on exit; agent-deck auto-detection stays background (`-d`, no focus steal). +- Signal-safe save (`app/revdiff/signal.go` + `finalize()` in `main.go`): `run()` appends `tea.WithoutSignalHandler()` so bubbletea does not install its own SIGINT/SIGTERM handler, then `shutdownGuard.watch(p)` installs one covering SIGHUP/SIGTERM that calls `p.Quit()` exactly once (`sync.Once`) and flips an `atomic.Bool`. SIGINT is registered on the same channel via `Notify` but drained by `handle()` without quitting (a `continue` on `syscall.SIGINT`) so a Ctrl-C the user meant for an external `$EDITOR` (delivered in cooked mode while `tea.ExecProcess` owns the terminal) does not quit revdiff — registering it via `Notify` (not `signal.Ignore`) is what suppresses its default terminate disposition, which would otherwise hard-kill with no save. Trade-off: `kill -INT` no longer quits revdiff (`kill -TERM` still does). The flag (`wasSignaled()`) is read only after `p.Run()` joins, so the unsynchronized annotation store is never touched off the main goroutine (no lock needed). The `watch()` stop func is idempotent (`sync.Once`) and restores default disposition for all three via `signal.Stop`+`close` (`signal.Reset` does NOT undo a `signal.Ignore`, which is why SIGINT goes through `Notify`+drain rather than `signal.Ignore`); `run()` calls it explicitly **before** `finalize` (and via `defer` as a panic net) so a slow or hung finalize (`saveHistory` shells out to git) stays killable by a second signal instead of being caught-and-swallowed. The `p.Run()` tail routes through `finalize(finalizeReq{...})`: discarded or empty output → write nothing; otherwise `saveHistory` always runs (safety net) and a signal-driven exit stops there — history only, never the `-o` output. **SIGTERM semantic change**: a signal-delivered SIGTERM used to reach bubbletea's own handler → QuitMsg → wrote BOTH history and the `-o` output; now (like SIGHUP) it is a safety-net save — history only, no `-o` handoff, because a signal is not a deliberate handoff. Typed interactive `^C` is unaffected (bubbletea raw mode has ISIG off, so `^C` is a keystroke, not a signal; revdiff binds no `ctrl+c` action). Recovery is the existing "load newest from `~/.config/revdiff/history/`" flow. Separately, `REVDIFF_TMUX_WINDOW=1` (a launcher env var read in `agentdeck-window.sh`, both plugin copies) promotes the existing agent-deck tmux window backend to a first-class interactive mode: a server-owned `tmux new-window` survives a client disconnect (SSH drop / VPN expiry) where a `display-popup` cannot. The explicit opt-in is captured in `_rd_focus` **before** `_rd_winmode` folds user-opt and agent-deck auto-detection together — only the opt-in opens the window focused and restores the prior active window on exit; agent-deck auto-detection stays background (`-d`, no focus steal). - Compact mode (`C` key, `--compact` / `--compact-context=N`): shrinks the VCS diff at generation time (not at render time) by passing a `contextLines int` parameter through every `Renderer.FileDiff()` callsite. VCS renderers (Git, Hg, Jj) translate it via per-renderer helpers — `gitContextArg` / `hgContextArg` produce `-U`, `jjContextArg` produces `--context=`. Sentinel for full-file: `contextLines <= 0` or `>= 1000000` → use the full-file arg (`-U1000000` / `--context=1000000`). Context-only sources (FileReader, DirectoryReader, StdinReader) accept the parameter but ignore it — there are no changes to contextualize. `CompareReader` honors `contextLines` — passes it to `unifiedContextArg` and calls `countFileLines(oldPath)` for the trailing-divider probe — so compact mode is applicable (`compactApplicable` returns `true` for `CompareReader`). `CompactApplicable` is computed at the composition root (`compactApplicable()` in `main.go`) via type assertion on the renderer chain (same approach as `CommitLogger`, no new interface); `false` for `--stdin`, `--all-files`, and file-only modes without VCS. Toggle re-fetches only the current file via `reloadCurrentFile()` — a deliberately lightweight sibling of `triggerReload()` that bumps `file.loadSeq` and returns `loadFileDiff(m.file.name)` without re-fetching the files list or commit log. Cursor position is preserved across the toggle (issue #271): `toggleCompactMode()` captures a `compactAnchor` (via `captureCompactAnchor()`) before the re-fetch and `handleFileLoaded` restores it via `applyCompactAnchor()`. The anchor resolves by source line number (`diffLineNum` + `changeType` through `findDiffLineIndex`); when that line is absent from the re-fetched diff (a context line dropped in the full→compact direction) it falls back to the nearest hunk captured at toggle time (`nearestHunkIndex()`), then to `skipInitialDividers()`. In collapsed mode both direct-set branches call `adjustCursorIfHidden()` so the restored cursor never lands on a collapsed-hidden removed line (e.g. a modify hunk's start). The anchor is `seq`-tagged to the exact reload it belongs to (`anchor.seq = m.file.loadSeq` after `reloadCurrentFile()`), so a superseded load — an intervening file switch or `R` reload — discards the stale anchor via the `a.seq == msg.seq` guard rather than repositioning the wrong file. Anchor state lives on `compactState.pendingAnchor`. `m.currentContextLines()` is the helper every `FileDiff` callsite must use — returns `m.modes.compactContext` if `m.modes.compact && m.compact.applicable`, else `0`. Runtime state (applicability + transient hint) lives on `compactState` alongside `reloadState` / `commitsState`; user-toggled state (on/off, context size) stays on `modeState` with the other view toggles. Composes cleanly with `--collapsed` (different layers: compact shrinks diff before parsing, collapsed hides removed lines during rendering). Divider lines now carry line-count labels: `parseUnifiedDiff` emits `⋯ N line[s] ⋯` dividers at three positions — leading (first hunk does not start at line 1), between non-adjacent hunks (computed from `prevOldEnd` tracked via hunk-header metadata, handles insertion-only `@@ -K,0 ...` hunks where `oldNum` does not advance on `+` lines), and trailing (last hunk does not reach EOF). Trailing requires `totalOldLines` passed by the VCS impl; Git/Hg/Jj `FileDiff` fetch it via `git show :` / `hg cat` / `jj file show` and only when `contextLines > 0 && contextLines < fullContextSentinel` — full-file mode always reaches EOF so the probe is skipped. - `diff.DiffLine.Content` for `ChangeDivider` rows is a human-readable `⋯ N line[s] ⋯` label produced by `parseUnifiedDiff`. Never pattern-match the string — dispatch on `ChangeType == diff.ChangeDivider`. Test fixtures may construct `ChangeDivider` rows with arbitrary `Content` (e.g. `"..."`, `"@@..."`); that is a test-only shortcut and NOT the parser's contract. -- Info overlay (`i` key, unified review/description/commits popup) uses the `diff.CommitLogger` capability interface (additive to `diff.Renderer`). Model resolves a `commitLogSource` at construction: explicit `ModelConfig.CommitLog` wins, else type-asserts the renderer for `CommitLogger`, else the commits section is hidden via `CommitsApplicable=false` and the overlay still opens (the description and session sections carry the popup's signal). `CommitsApplicable` is computed at the composition root by `commitsApplicable()` in `main.go` (using the `commitLogger` field populated by `setupVCSRenderer` in `renderer_setup.go`) — Model copies it, does not re-derive. Data is fetched eagerly at startup and on `R` reload: `Init()` and `triggerReload()` both return `tea.Batch(m.loadFiles(), m.loadCommits())`, running files and commits loads in parallel as independent goroutines. `loadCommits()` captures `m.commits.loadSeq` at invocation time and tags the resulting `commitsLoadedMsg` with it; `handleCommitsLoaded` drops any message whose seq no longer matches (stale-result guard, mirrors the files-load pattern). Eager parallel fetch shrinks the window where overlay and diff can disagree from "time until first `i` press" to "skew between two parallel goroutine starts" (milliseconds). Not a strict snapshot guarantee — the two subprocesses each resolve `HEAD` independently — but the practical race window is tens of ms instead of minutes. `handleInfo` always opens the popup; if commits are still loading, the commits section renders an inline `loading commits…` placeholder that flips to the rendered list when the fetch lands (via `refreshInfoOverlay` pushing a fresh spec into the open overlay). Aggregate `+/-` line stats are loaded lazily on first overlay open via `triggerReviewStats()` and gated on `ModelConfig.ReviewInfo` (a `*ReviewInfoConfig` pointer) — **nil disables the entire review-info subsystem**; every derived render path (footer, rows, stats trigger) checks `m.review.cfg != nil` consistently or `loading…` can stick on the footer forever. Production wires a non-nil `*ReviewInfoConfig` via `reviewInfoFromOptions` in `app/reviewinfo.go`; focused tests pass nil to bypass the subsystem (the legacy commit-only popup behavior). The description prose is sanitized + chroma-highlighted once in `NewModel` via `precomputeDescriptionHighlight` and cached on `reviewInfoState.descriptionHighlighted`. `--description-file` reads bound the size cap with `io.LimitReader` and reject non-regular files (FIFOs, devices, dirs); `safeWorkDirPath` for untracked-file stats resolves both sides via `filepath.EvalSymlinks` so a symlink inside workDir does not redirect reads off-tree under non-racing filesystem state — best-effort, not TOCTOU-proof: the call returns the pre-resolution path and the later `os.Stat` / `diff.ReadFileAsAdded` reopens it, so a local attacker who swaps a path component between validation and read can still redirect that subsequent open. Threat model is "untrusted VCS listing under a trusted working tree", not "hostile filesystem racing the reviewer process". `readUntracked` rejects non-regular files (FIFOs, sockets, devices) before reading and flags binary/placeholder rows from `diff.ReadFileAsAdded` as `Partial=true` so the footer never silently treats unreadable files as zero-line. Untrusted text routes through `diff.SanitizeCommitText` (the same strip path commit author/subject/body already use); description and info-row sanitizers wrap it — description preserves LF/TAB for paragraphs, info rows map each surviving LF/TAB to a single space for single-line display (no whitespace-run collapsing). The deprecated `commit_info` keymap alias rewrites to `info` and surfaces a single `[WARN]` per process via `warnOnceDeprecatedAlias` (a `sync.Map` of already-logged aliases) so a keybindings file with multiple `commit_info` lines does not spam the log. Hg cannot use literal NUL in argv templates — use ASCII US/RS (`\x1f`/`\x1e`) as field/record separators for hg only +- Info overlay (`i` key, unified review/description/commits popup) uses the `diff.CommitLogger` capability interface (additive to `diff.Renderer`). Model resolves a `commitLogSource` at construction: explicit `ModelConfig.CommitLog` wins, else type-asserts the renderer for `CommitLogger`, else the commits section is hidden via `CommitsApplicable=false` and the overlay still opens (the description and session sections carry the popup's signal). `CommitsApplicable` is computed at the composition root by `commitsApplicable()` in `main.go` (using the `commitLogger` field populated by `setupVCSRenderer` in `renderer_setup.go`) — Model copies it, does not re-derive. Data is fetched eagerly at startup and on `R` reload: `Init()` and `triggerReload()` both return `tea.Batch(m.loadFiles(), m.loadCommits())`, running files and commits loads in parallel as independent goroutines. `loadCommits()` captures `m.commits.loadSeq` at invocation time and tags the resulting `commitsLoadedMsg` with it; `handleCommitsLoaded` drops any message whose seq no longer matches (stale-result guard, mirrors the files-load pattern). Eager parallel fetch shrinks the window where overlay and diff can disagree from "time until first `i` press" to "skew between two parallel goroutine starts" (milliseconds). Not a strict snapshot guarantee — the two subprocesses each resolve `HEAD` independently — but the practical race window is tens of ms instead of minutes. `handleInfo` always opens the popup; if commits are still loading, the commits section renders an inline `loading commits…` placeholder that flips to the rendered list when the fetch lands (via `refreshInfoOverlay` pushing a fresh spec into the open overlay). Aggregate `+/-` line stats are loaded lazily on first overlay open via `triggerReviewStats()` and gated on `ModelConfig.ReviewInfo` (a `*ReviewInfoConfig` pointer) — **nil disables the entire review-info subsystem**; every derived render path (footer, rows, stats trigger) checks `m.review.cfg != nil` consistently or `loading…` can stick on the footer forever. Production wires a non-nil `*ReviewInfoConfig` via `reviewInfoFromOptions` in `app/revdiff/reviewinfo.go`; focused tests pass nil to bypass the subsystem (the legacy commit-only popup behavior). The description prose is sanitized + chroma-highlighted once in `NewModel` via `precomputeDescriptionHighlight` and cached on `reviewInfoState.descriptionHighlighted`. `--description-file` reads bound the size cap with `io.LimitReader` and reject non-regular files (FIFOs, devices, dirs); `safeWorkDirPath` for untracked-file stats resolves both sides via `filepath.EvalSymlinks` so a symlink inside workDir does not redirect reads off-tree under non-racing filesystem state — best-effort, not TOCTOU-proof: the call returns the pre-resolution path and the later `os.Stat` / `diff.ReadFileAsAdded` reopens it, so a local attacker who swaps a path component between validation and read can still redirect that subsequent open. Threat model is "untrusted VCS listing under a trusted working tree", not "hostile filesystem racing the reviewer process". `readUntracked` rejects non-regular files (FIFOs, sockets, devices) before reading and flags binary/placeholder rows from `diff.ReadFileAsAdded` as `Partial=true` so the footer never silently treats unreadable files as zero-line. Untrusted text routes through `diff.SanitizeCommitText` (the same strip path commit author/subject/body already use); description and info-row sanitizers wrap it — description preserves LF/TAB for paragraphs, info rows map each surviving LF/TAB to a single space for single-line display (no whitespace-run collapsing). The deprecated `commit_info` keymap alias rewrites to `info` and surfaces a single `[WARN]` per process via `warnOnceDeprecatedAlias` (a `sync.Map` of already-logged aliases) so a keybindings file with multiple `commit_info` lines does not spam the log. Hg cannot use literal NUL in argv templates — use ASCII US/RS (`\x1f`/`\x1e`) as field/record separators for hg only - **ANSI nesting with lipgloss**: `lipgloss.Render()` emits `\033[0m` (full reset) which breaks outer style backgrounds. For styled substrings inside a lipgloss container (status bar separators, search highlights, diff cursor, annotation lines), use raw ANSI sequences via the style sub-package (`style.AnsiFg`, `resolver.Color`), or dedicated `Renderer` methods. Never use `lipgloss.NewStyle().Render()` for inline elements within a lipgloss-rendered parent. - **Background fill for themed panes**: lipgloss pane `Render()` and viewport internal padding emit plain spaces after reset, causing terminal default bg. Workarounds: (1) `extendLineBg()` pads lines to full width, (2) `padContentBg()` re-pads pane content, (3) `BorderBackground()` on border styles. **Ordering**: `extendLineBg()` must be called AFTER `applyHorizontalScroll()`. - Horizontal scroll indicators (`«`/`»`): see `applyHorizontalScroll()` in `diffview.go`. `«` replaces first visible column when scrolled past hidden content. `»` extends 1 col into right padding. Bg split: `«` and separator space use line bg via `indicatorBg()`, `»` glyph uses `DiffBg`. Only in unwrapped mode. @@ -35,7 +35,7 @@ - Status bar mode icons: `▼⊂◉↩≋⊟#b±✓∅` rendered via `statusModeIcons()`. Graceful degradation drops segments on narrow terminals. The glyphs live in one place, `statusIconForAction` (same file, keyed by `keymap.Action`): the status bar reads them from there and the help overlay shows each beside the key that drives it. A new icon needs an entry there plus an indicator row in `statusModeIcons`. `TestBuildHelpSpec_StatusIconsOnToggleRows` pins the help mapping. - Search and hunk navigation use `centerViewportOnCursor()` (cursor ends up in the middle of the page). `syncViewportToCursor()` is the general "keep cursor visible" path and is called from cursor moves (j/k, g/G, page up/down), content mutations (annotation save/delete, blame load), and layout changes (tree/wrap/blame/line-number toggles, resize, file load). It uses `cursorVisualRange()` so the cursor's full logical line — wrap-continuation rows plus any injected annotation rows — stays visible, not just the cursor's top row. - **Viewport scroll after content mutation**: `syncViewportToCursor()` calls `SetContent(renderDiff())` itself before setting `YOffset` — callers must not pre-`SetContent` around it, and any code that injects rows below a diff line (wrap continuations, multi-line annotations, future overlays) must route scroll through this function so visual-height math stays consistent with `cursorVisualRange()` / `hunkLineHeight()`. -- **Page/half-page cursor walks and `cursorOnAnnotation`**: `moveDiffCursorDownBy` / `moveDiffCursorUpBy` (`app/ui/diffnav.go`) detect "no movement possible" by comparing `diffCursor` **and** `cursorOnAnnotation` against the previous **single** step. So `moveDiffCursorDownWithHunks` may clear `cursorOnAnnotation` **only** together with an advancing `diffCursor` — clearing it on the last navigable line's annotation, where no next line exists, reads as progress forever and freezes the TUI (the visual delta oscillates by the diff line's own wrapped height — `cursorViewportYFromOffsets` adds `wrappedLineCount(diffCursor)` when the flag is set, not the annotation's row count — and both ends of the oscillation are fixed relative to the `startY` captured at loop entry, so the walk cycles forever whenever both ends stay below `startY + rows`). This freeze is unrecoverable from inside the TUI: it wedges the bubbletea event loop inside `Update`, so `shutdownGuard`'s `p.Quit()` blocks forever sending on the unbuffered `msgs` channel that only the wedged `eventLoop` drains (nothing cancels `p.ctx` — no `Kill()` caller, and `defer p.cancel()` runs only when `Run` returns). `signal.Notify` (`app/signal.go`) has already taken over SIGHUP/SIGTERM/SIGINT, and its `signal.Stop` restore only runs after `p.Run()` returns, which it never does — so `kill -TERM` does NOT reach the signal-safe-save path, it just parks the handler goroutine on the same blocked send. Only a signal outside that set ends it: `kill -QUIT` (Go runtime dumps goroutine stacks and dies, leaving the terminal in raw mode — run `reset` after) or `kill -9`. Neither runs `finalize`: no history auto-save and no `-o` write — but an earlier `O` flush's snapshot survives on disk. The up direction is deliberately asymmetric: the annotation sub-row renders *below* its diff line (`rowOnAnnotationSubLine` in `app/ui/annotate.go`), so clearing the flag without moving `diffCursor` IS the upward move, and that cursor strictly decreases so it cannot cycle. Absolute placements (H/M/L in `diffnav.go`, mouse click in `mouse.go`) are one-shot assignments outside these loops. Both walks also **undo** the step that carries the delta past `rows`, restoring `diffCursor` and `cursorOnAnnotation` (the only two fields `moveDiffCursor*WithHunks` mutates) and breaking immediately: one cursor step can span many rows via `hunkLineHeight`, and scrolling by that whole height moved the viewport further than a page, past rows it never rendered. The rollback cannot cycle — it is always followed by `break`. It is gated on `worthRollingBack(walked, rows)` (`walked*2 >= rows`) and that gate is **not** cosmetic: a block taller than the page has no selectable position inside it, so rolling back to the row or two walked before it scrolls the pane by almost nothing and the next press takes the same oversized step anyway. Gating on "has moved at all" instead produced exactly that one-row collapse. `walked` is 0 on the first step, so the walk can never refuse to move. A single step taller than the whole page still scrolls by its full height and still skips — unavoidable while the viewport follows the cursor, and the reason `pgdown` then `pgup` is only reversible for uniform-height content. The two directions accumulate differently and the comments say so: walking **down** the delta grows by the height of the line being *left* (`offsets[i]` is that line's top row), walking **up** by the height of the line *arrived at*, including the annotation block rendered below it. +- **Page/half-page cursor walks and `cursorOnAnnotation`**: `moveDiffCursorDownBy` / `moveDiffCursorUpBy` (`app/ui/diffnav.go`) detect "no movement possible" by comparing `diffCursor` **and** `cursorOnAnnotation` against the previous **single** step. So `moveDiffCursorDownWithHunks` may clear `cursorOnAnnotation` **only** together with an advancing `diffCursor` — clearing it on the last navigable line's annotation, where no next line exists, reads as progress forever and freezes the TUI (the visual delta oscillates by the diff line's own wrapped height — `cursorViewportYFromOffsets` adds `wrappedLineCount(diffCursor)` when the flag is set, not the annotation's row count — and both ends of the oscillation are fixed relative to the `startY` captured at loop entry, so the walk cycles forever whenever both ends stay below `startY + rows`). This freeze is unrecoverable from inside the TUI: it wedges the bubbletea event loop inside `Update`, so `shutdownGuard`'s `p.Quit()` blocks forever sending on the unbuffered `msgs` channel that only the wedged `eventLoop` drains (nothing cancels `p.ctx` — no `Kill()` caller, and `defer p.cancel()` runs only when `Run` returns). `signal.Notify` (`app/revdiff/signal.go`) has already taken over SIGHUP/SIGTERM/SIGINT, and its `signal.Stop` restore only runs after `p.Run()` returns, which it never does — so `kill -TERM` does NOT reach the signal-safe-save path, it just parks the handler goroutine on the same blocked send. Only a signal outside that set ends it: `kill -QUIT` (Go runtime dumps goroutine stacks and dies, leaving the terminal in raw mode — run `reset` after) or `kill -9`. Neither runs `finalize`: no history auto-save and no `-o` write — but an earlier `O` flush's snapshot survives on disk. The up direction is deliberately asymmetric: the annotation sub-row renders *below* its diff line (`rowOnAnnotationSubLine` in `app/ui/annotate.go`), so clearing the flag without moving `diffCursor` IS the upward move, and that cursor strictly decreases so it cannot cycle. Absolute placements (H/M/L in `diffnav.go`, mouse click in `mouse.go`) are one-shot assignments outside these loops. Both walks also **undo** the step that carries the delta past `rows`, restoring `diffCursor` and `cursorOnAnnotation` (the only two fields `moveDiffCursor*WithHunks` mutates) and breaking immediately: one cursor step can span many rows via `hunkLineHeight`, and scrolling by that whole height moved the viewport further than a page, past rows it never rendered. The rollback cannot cycle — it is always followed by `break`. It is gated on `worthRollingBack(walked, rows)` (`walked*2 >= rows`) and that gate is **not** cosmetic: a block taller than the page has no selectable position inside it, so rolling back to the row or two walked before it scrolls the pane by almost nothing and the next press takes the same oversized step anyway. Gating on "has moved at all" instead produced exactly that one-row collapse. `walked` is 0 on the first step, so the walk can never refuse to move. A single step taller than the whole page still scrolls by its full height and still skips — unavoidable while the viewport follows the cursor, and the reason `pgdown` then `pgup` is only reversible for uniform-height content. The two directions accumulate differently and the comments say so: walking **down** the delta grows by the height of the line being *left* (`offsets[i]` is that line's top row), walking **up** by the height of the line *arrived at*, including the annotation block rendered below it. - Single-file mode (`m.file.singleFile`): one file → tree hidden, diff full width. Exception: markdown full-context gets TOC pane. - Tree pane toggle (`t` key): `m.layout.treeHidden` orthogonal to `singleFile`. - Markdown TOC: activated when `singleFile && isMarkdownFile && isFullContext`. Uses `paneTree` slot. diff --git a/.goreleaser.yml b/.goreleaser.yml index bf659d28..6940d270 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -2,7 +2,7 @@ version: 2 builds: - id: revdiff - main: ./app + main: ./app/revdiff binary: revdiff env: - CGO_ENABLED=0 diff --git a/.zed/tasks.json b/.zed/tasks.json index 04d131c4..dc70e452 100644 --- a/.zed/tasks.json +++ b/.zed/tasks.json @@ -1,14 +1,14 @@ [ { "label": "revdiff: run", - "command": "go run ./app --dbg", + "command": "go run ./app/revdiff --dbg", "cwd": "$ZED_WORKTREE_ROOT", "use_new_terminal": true, "allow_concurrent_runs": false }, { "label": "revdiff: run staged", - "command": "go run ./app --staged --dbg", + "command": "go run ./app/revdiff --staged --dbg", "cwd": "$ZED_WORKTREE_ROOT", "use_new_terminal": true, "allow_concurrent_runs": false diff --git a/CLAUDE.md b/CLAUDE.md index 2daf4d2f..4ae80722 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -13,7 +13,8 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit - Vendor after adding deps: `go mod vendor` ## Project Structure -- `app/` - composition root (`package main`), split by concern: `main.go` (entrypoint + `run()`), `config.go` (options/parsing), `stdin.go` (stdin mode), `renderer_setup.go` (VCS wiring), `themes.go` (theme CLI + adapter), `history_save.go` (session save) +- `app/` - all Go source +- `app/revdiff/` - composition root (`package main`), split by concern: `main.go` (entrypoint + `run()`), `config.go` (options/parsing), `stdin.go` (stdin mode), `renderer_setup.go` (VCS wiring), `themes.go` (theme CLI + adapter), `history_save.go` (session save) - `app/diff/` - VCS interaction (git + hg + jj), unified diff parsing, VCS detection, Mercurial + Jujutsu support. `compare.go` — `CompareReader` renderer for `--compare-old/--compare-new` (runs `git diff --no-index`, no repo required) - `app/ui/` - bubbletea TUI package. Single `Model` struct with state grouped into sub-structs (`cfg`, `layout`, `file`, `modes`, `nav`, `search`, `annot`), methods split across files by concern (~500 lines each). Each source file has a matching `_test.go`. See `app/ui/doc.go` for package docs, `docs/ARCHITECTURE.md` for file-by-file breakdown. Does not import `app/theme` or `app/fsutil` — theme operations go through the `ThemeCatalog` interface - `app/ui/style/` - color/style resolution: hex-to-ANSI, lipgloss styles, SGR tracking, HSL math. Types: `Resolver`, `Renderer`, `SGR`. Also `display.go` - package-level `SanitizeFilenameForDisplay` and `TruncateLeftToWidth`, the shared helpers every filename-rendering surface must route through @@ -44,7 +45,7 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit - `--theme NAME` loads theme; `--dump-theme` exports resolved colors; `--list-themes` lists available; `--init-themes` re-creates bundled - Theme precedence: `--theme` overwrites all 23 color fields + chroma-style, ignoring `--color-*` flags or env vars - Theme values applied via `applyTheme()` in `themes.go` which overwrites `opts.Colors.*` after `parseArgs()`. `colorFieldPtrs(opts)` is the single source of truth for color key → struct field mapping — adding a new color requires changes in `theme.go` colorKeys + options struct + `colorFieldPtrs()` in `themes.go` -- Theme ownership split: `app/theme.Catalog` owns discovery/loading, `app/ui.ThemeCatalog` interface consumed by UI, `app/themes.go` wires adapter composing catalog + config persistence +- Theme ownership split: `app/theme.Catalog` owns discovery/loading, `app/ui.ThemeCatalog` interface consumed by UI, `app/revdiff/themes.go` wires adapter composing catalog + config persistence - `ini-name` tags ensure config keys match CLI long flag names - Keybindings file: `~/.config/revdiff/keybindings` (`map ` / `unmap ` format) - `--keys` overrides keybindings path, `--dump-keys` prints effective bindings @@ -72,7 +73,7 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit - **Launchers must parse under bash 3.2** (`/bin/bash` on stock macOS, where `#!/usr/bin/env bash` resolves unless a newer bash is on PATH). Its parser scans a `$( )` command substitution for quotes **before** it processes a heredoc inside it, so an odd number of apostrophes in a heredoc opened **inside** one opens a quote that never closes and the whole script fails to parse — reported a hundred lines later, on an unrelated valid line. Two heredocs per launcher are nested that way, the `GHOSTTY_TERM_ID=$(osascript ...)` and `ITERM_NEW_SESSION=$(osascript ...)` captures; the close-pane heredocs beside them are plain commands and are unaffected. Keep the nested bodies apostrophe-free; balancing them is not a fix, since a later edit to one word re-breaks it. No parse check can guard this in CI — ubuntu and every Homebrew bash accept the broken form — so `TestLauncherNestedHeredocsHaveNoApostrophes` guards it textually instead, and `/bin/bash -n` is the direct check on macOS. Introduced by #309, reported in #314. - **shellcheck is pinned in CI** (`koalaman/shellcheck:v0.11.0`, digest-pinned in `.github/workflows/ci.yml` because a docker tag is mutable and a retag would change the checker silently) because the runner image's version drifts and a `# shellcheck disable=` is version-sensitive: `SC2317` ("command appears unreachable") was split into `SC2329` ("function never invoked") in 0.11, so a suppression naming only the code your local shellcheck emits passes locally and fails CI — which is exactly how #344 broke. Name **both** codes. The one place this bites is `herdr_cleanup_unlaunched`: it is the only trap in the launcher that calls a *function* rather than inlining its commands, and shellcheck cannot see invocations inside the trap's quoted string. The warning is a false positive — mutating the function's body fails three `TestHerdrSignalPaneOwnership` cases — so suppress it, don't inline the function to appease the linter. Reproduce CI exactly with `find . -name '*.sh' -not -path './.git/*' -not -path './vendor/*' -print0 | xargs -0 docker run --rm -v "$PWD":/mnt:ro koalaman/shellcheck:v0.11.0@sha256:61862eba1fcf09a484ebcc6feea46f1782532571a34ed51fedf90dd25f925a8d`. - **Launcher override chain**: both Claude plugins resolve their launcher script via `resolve-launcher.sh` through `user → bundled` layers (first executable wins). The planning plugin's user layer is `${CLAUDE_PLUGIN_DATA}/scripts/` under Claude and `${PLUGIN_DATA}/scripts/` under Codex. There is **no project-level (`.claude/...` or `.codex/...`) executable layer by design** — the planning hook fires automatically in any repo, and a repo-controlled launcher would run on routine agent actions. The Pi extension and manual Codex diff-review skill do not use this plugin-data override. -- **Overlay stderr relay**: `launch-revdiff.sh` appends `2>$ERR_FILE` to `REVDIFF_CMD` once, right after the argument loop, so every backend captures revdiff's stderr without per-backend edits (the redirect stays the trailing token through `write_rc_cmd` / `write_fifo_rc_cmd`, the heredoc launch scripts, and the later `/usr/bin/env` prepend). `print_output_and_exit` replays the file on any exit code other than 0 or 10 — those two are successes, and revdiff writes ordinary warnings to stderr, so relaying them would put noise on every successful review. All ten `EXIT` traps in `launch-revdiff.sh` list `$ERR_FILE` — an override that omits it leaks the file into `$TMPDIR`, which `TestShellLaunchersPreserveAnnotationExitCode` catches by asserting no `revdiff-err-*` survives the run. The stderr expectations live in that test because a second launcher-by-backend pass puts the `app` package over the timeout in `make race` (now `-timeout=180s`, matching CI); a new launcher-wide behavior belongs in the same matrix, gated per launcher via `relaysStderr` (`launch-plan-review.sh` has no relay). The sourced agent-deck window backend (`agentdeck-window.sh`) is the eleventh execution path and carries no relay code of its own: it builds its command through `write_rc_cmd`, exits through `print_output_and_exit`, and installs no `EXIT` trap by design, so it rides the base one. It is the only backend absent from `launcherBackends()`, so the relay tests do not cover it. `plugins/revdiff-planning/scripts/launch-plan-review.sh` has no relay: its per-backend exit tails are duplicated inline with no shared helpers. +- **Overlay stderr relay**: `launch-revdiff.sh` appends `2>$ERR_FILE` to `REVDIFF_CMD` once, right after the argument loop, so every backend captures revdiff's stderr without per-backend edits (the redirect stays the trailing token through `write_rc_cmd` / `write_fifo_rc_cmd`, the heredoc launch scripts, and the later `/usr/bin/env` prepend). `print_output_and_exit` replays the file on any exit code other than 0 or 10 — those two are successes, and revdiff writes ordinary warnings to stderr, so relaying them would put noise on every successful review. All ten `EXIT` traps in `launch-revdiff.sh` list `$ERR_FILE` — an override that omits it leaks the file into `$TMPDIR`, which `TestShellLaunchersPreserveAnnotationExitCode` catches by asserting no `revdiff-err-*` survives the run. The stderr expectations live in that test because a second launcher-by-backend pass puts the main package over the timeout in `make race` (now `-timeout=180s`, matching CI); a new launcher-wide behavior belongs in the same matrix, gated per launcher via `relaysStderr` (`launch-plan-review.sh` has no relay). The sourced agent-deck window backend (`agentdeck-window.sh`) is the eleventh execution path and carries no relay code of its own: it builds its command through `write_rc_cmd`, exits through `print_output_and_exit`, and installs no `EXIT` trap by design, so it rides the base one. It is the only backend absent from `launcherBackends()`, so the relay tests do not cover it. `plugins/revdiff-planning/scripts/launch-plan-review.sh` has no relay: its per-backend exit tails are duplicated inline with no shared helpers. - **herdr pane-scoped overlay (`REVDIFF_HERDR_PANE=1`)**: opt-in, off by default; unset, the `herdr tab create` + `pane run` block is reached byte-identical, because pane mode is a block ahead of it that exits on its own rather than a flag threaded through the shared path. The tab block itself is byte-identical, but the EXIT trap and the generated launch script ARE shared — the ownership machinery there is inert in tab mode only because `HERDR_TARGET` stays empty, so treat both as pane/tab common code when editing. **`$HERDR_PANE_ID` is a name collision** — herdr injects it into every managed pane and the tab path uses it as its own local, so the caller's id is copied to `HERDR_CALLER_PANE` first; losing it types the launch command into the agent's own shell. Ownership is `HERDR_TARGET` (non-empty = a pane owes a close); `herdr_close_pane` clears it before shelling out so a re-entrant call cannot close twice. **Cleanup is decided by evidence from the pane, not by `pane run` returning** — herdr may start the review before that call returns, or not after it. The dispatched script's first line touches `$SENTINEL.started`, and `HERDR_DISPATCHED` is set to 1 immediately before `pane run` is called; `herdr_cleanup_unlaunched` preserves the pane while **either** signal says a review may exist and the sentinel says it has not finished, closing and cleaning up otherwise (never started — holds only a shell; already finished — done). `HERDR_DISPATCHED` is claimed **before** `pane run` and never cleared, because a pending signal is serviced the instant that call returns and before any later assignment — ownership taken afterwards would miss a dispatch herdr had already accepted, and clearing it on the refusal path would let a signal landing inside that path's evidence grace close a pane whose review may already be running. Nothing needs the clear: `herdr_close_pane` discharges ownership by emptying `HERDR_TARGET`, which is what `herdr_cleanup_unlaunched` gates on. A launcher killed while herdr is wedged inside the call therefore preserves the pane: the state is unknown, and unknown is never destroyed. The marker still matters on the failure path, where it is the only evidence that a refused-looking dispatch actually started. For the same reason a nonzero `pane run` does not close a pane that has already announced itself. That is what `SKILL.md` promises the driving agent: a launcher killed on timeout leaves a *live* review open with nothing lost. Ownership does not depend on the order of `pane run` and the cosmetic `pane zoom` — the marker settles it — so the zoom stays first and revdiff starts in an already-zoomed pane rather than being resized during startup. The marker is written with `touch … || true`, never `: >`: a redirection failure on a special builtin kills a POSIX shell outright and revdiff would never run. After dispatch the script belongs to the pane (it ends with `rm -f "$0"`); the completion path removes it for a pane that died first. `TestHerdrSignalPaneOwnership` signals a real launcher to pin both halves; three of its cases reach the trap's close — wedged in `pane split`, interrupted in `pane split`, and wedged in `pane zoom` — all of them pre-dispatch, where the pane provably holds only a shell; every later case is the preserve half — without it the close is mutation-invisible. The block also adds `trap 'exit 130' INT` / `trap 'exit 143' TERM` for agterm parity, and the **tab** path inherits them. They do not change *whether* cleanup runs — bash runs the EXIT trap on an untrapped SIGTERM too, same 143 — they change *when*: a trapped signal is deferred until the in-flight foreground command returns, so cleanup observes the state after the pending `herdr` call finishes rather than racing it. Pane-mode ownership depends on that ordering; for the tab path it is a no-op beyond a deterministic exit code. A trapped signal is deferred until the in-flight foreground command returns and then runs **before the next statement**, so `trap 'exit 143' TERM` across `pane split` would exit after the pane exists but before `HERDR_TARGET` names it, leaving the EXIT trap nothing to close. The split-and-parse window therefore installs *recording* traps (`trap 'HERDR_SIGNALLED=130' INT`, `trap 'HERDR_SIGNALLED=143' TERM`) and pays the signal once ownership is held; both exits from that window — success and the tab fallthrough — must restore `exit 130`/`exit 143` first, or a signal on a path with no pane to protect is swallowed and the launcher hangs (`killed on the tab fallthrough still exits` is the guard, via a bounded wait). The refusal path's evidence grace (a wall-clock interval of one to two seconds: `SECONDS=0`, loop while `-lt 2`) records signals for the same reason: exiting mid-grace hands the trap a state it must read as "may be live" and preserve, stranding a pane the completed check would have closed. **A recording trap alone is not enough there**, because a *process-group* signal (interactive Ctrl-C, `kill -- -pgid`) also kills the foreground `sleep`: it returns nonzero and `set -e` aborts before the evidence check, resurrecting the strand. The grace is therefore measured on the wall clock, `while [ ! -f marker ] && [ "$SECONDS" -lt 2 ]` around `sleep 0.3 || true`: a killed sleep costs an early wakeup and nothing else, `|| true` keeps it from tripping errexit, and the interval is served no matter how many signals arrive. **An absent marker is evidence only once that interval has elapsed**, and then the pane IS closed (`a relentlessly signaled grace is still served` pins this with a minimum-duration assertion, since the close alone would also pass against a launcher that never waited). Each trap records its **own** status (`130` for INT, `143` for TERM) rather than a boolean, or a recorded Ctrl-C is paid as a fixed `exit 143` and misreports the signal. Reaching the grace deterministically needs no timing: a PATH-injected `sleep` raises the signal *before* sleeping, so it is already pending when the grace sleep becomes the foreground command (`FAKE_SLEEP_KILL=parent|group`, the group variant requiring `setpgid` or the kill lands on `go test` itself). A trap with a *command* is reset to default in children, which is why this is safe where SIG_IGN is not. Do **not** bracket the create window with `trap '' INT TERM`: `trap ''` is SIG_IGN, inherited across `exec`, so the herdr child ignores INT/TERM and a hung server makes the launcher unkillable except by SIGKILL. The probe covers `pane split`/`get`/`close` (all three are used at runtime; a CLI lacking `get` abandons a live review); `pane zoom` is unprobed as cosmetic. The wait loop treats `{"error":{"code":"pane_not_found"}}` as authoritative death (nothing closed, nothing warned) and any other error as transient — it keeps polling, warning once per outage streak at 10 misses and backing the poll off from 0.3s to 2s so a downed control plane is not hammered (both reset on a good poll; the sentinel is still checked every iteration), because a generic error is not evidence the pane died and any deadline turns unknown liveness into a closed live review. **Never guess which pane to close**: a split that returns no parseable id warns and exits 1 rather than diffing `pane list`, since a recovered id may belong to another herdr client; an id equal to `$HERDR_CALLER_PANE` is rejected for the same reason. - **agterm pane-scoped overlay (`REVDIFF_AGTERM_PANE=1`)**: opt-in, off by default — unset leaves the agterm branch's call exactly as it was. It adds `--pane $AGTERM_PANE` to `session overlay open` so the review covers the agent's pane alone instead of the whole session, and it needs all four of: the env var set to `1`, `$AGTERM_PANE` being `left`/`right` (`scratch` is full-coverage with no sibling), a `--pane`-capable agtermctl (`agterm_supports_pane_overlay`, which short-circuits before the split read), and a split confirmed by `agterm_session_split` — a **window-scoped** `tree --json` read (`tree` defaults to the FRONTMOST window, so an unscoped read finds no session and reports every split as absent) parsed with jq, which reports "not split" when jq is missing. That probe exists because `--pane` reached agtermctl only after agterm v0.9.0; it reads the PATH agtermctl, which is not always the CLI of the running app, and the post-call fallback is what covers the skew: on a nonzero exit whose captured agtermctl stderr matches `pane overlay already open|pane not visible`, the launcher retries session-wide (agterm refused before running revdiff, so nothing is re-executed — the grep is gated on agterm's own message precisely so a revdiff failure never triggers a second review). agtermctl's stderr is captured separately from revdiff's (`$ERR_FILE`) and replayed either way; its stdout is dropped because `print_output_and_exit` owns the launcher's stdout. Both launcher copies carry it; `TestAgtermPaneOverlayOptIn` covers the gate, the fallback, and the default path. Known limitation, documented beside the gate: `$AGTERM_PANE` is baked into the shell's environ at spawn, so a pane agterm promoted into the main slot keeps `right` — promote-then-re-split scopes the overlay to the NEW sibling instead of this pane, and the fallback cannot catch it because that pane genuinely exists and agterm raises no error. `session status` takes a stable `--pane-id` token for exactly this, `overlay open` does not yet, and failing closed to the session-wide overlay is deliberately not the answer. - **Launcher env vars don't reach the tmux/zellij popup**: `launch-revdiff.sh` spawns the revdiff process in a fresh shell inside the multiplexer popup that does NOT inherit the parent shell's environment, so env-var config set before the launch is dropped (e.g. `REVDIFF_THEME=gruvbox launch-revdiff.sh HEAD~10` does not apply the theme). Pass it as a CLI flag instead: `launch-revdiff.sh --theme gruvbox HEAD~10`. Applies to any env-var-configurable option launched through the overlay. diff --git a/Makefile b/Makefile index db809f4e..9afddb05 100644 --- a/Makefile +++ b/Makefile @@ -9,7 +9,7 @@ REV=$(if $(filter --,$(GIT_REV)),latest,$(GIT_REV)) all: test build build: - go build -ldflags "-X main.revision=$(REV) -s -w" -o .bin/revdiff.$(BRANCH) ./app + go build -ldflags "-X main.revision=$(REV) -s -w" -o .bin/revdiff.$(BRANCH) ./app/revdiff cp .bin/revdiff.$(BRANCH) .bin/revdiff test: diff --git a/README.md b/README.md index 443420ce..d53f2d67 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,14 @@ sudo dpkg -i revdiff_*.deb sudo rpm -i revdiff_*.rpm ``` +**Go:** + +```bash +go install github.com/umputun/revdiff/app/revdiff@latest +``` + +Installs the `revdiff` binary into `GOBIN` (defaults to `$(go env GOPATH)/bin`). Add that directory to your `PATH`. + **Binary releases:** download from [GitHub Releases](https://github.com/umputun/revdiff/releases) (deb, rpm, archives for linux/darwin amd64/arm64). ## Claude Code Plugin diff --git a/app/annotations_load.go b/app/revdiff/annotations_load.go similarity index 100% rename from app/annotations_load.go rename to app/revdiff/annotations_load.go diff --git a/app/annotations_load_test.go b/app/revdiff/annotations_load_test.go similarity index 100% rename from app/annotations_load_test.go rename to app/revdiff/annotations_load_test.go diff --git a/app/compare.go b/app/revdiff/compare.go similarity index 100% rename from app/compare.go rename to app/revdiff/compare.go diff --git a/app/compare_test.go b/app/revdiff/compare_test.go similarity index 100% rename from app/compare_test.go rename to app/revdiff/compare_test.go diff --git a/app/config.go b/app/revdiff/config.go similarity index 100% rename from app/config.go rename to app/revdiff/config.go diff --git a/app/config_test.go b/app/revdiff/config_test.go similarity index 100% rename from app/config_test.go rename to app/revdiff/config_test.go diff --git a/app/history_save.go b/app/revdiff/history_save.go similarity index 100% rename from app/history_save.go rename to app/revdiff/history_save.go diff --git a/app/main.go b/app/revdiff/main.go similarity index 97% rename from app/main.go rename to app/revdiff/main.go index 8cc2ff65..3f621140 100644 --- a/app/main.go +++ b/app/revdiff/main.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "runtime/debug" tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" @@ -46,7 +47,8 @@ func main() { // early-exit commands that don't need theme resolution if opts.Version { - fmt.Printf("version: %s\n", revision) + info, _ := debug.ReadBuildInfo() + fmt.Printf("version: %s\n", buildVersion(revision, info)) os.Exit(0) } @@ -95,6 +97,16 @@ func main() { } } +func buildVersion(rev string, info *debug.BuildInfo) string { + if rev != "" && rev != "unknown" { + return rev + } + if info != nil && info.Main.Version != "" && info.Main.Version != "(devel)" { + return info.Main.Version + } + return "unknown" +} + func run(opts options) (int, error) { // force lipgloss to truecolor when colors are enabled. revdiff's raw-ANSI // helpers (style.ansiColor) always emit truecolor, but lipgloss respects diff --git a/app/main_test.go b/app/revdiff/main_test.go similarity index 93% rename from app/main_test.go rename to app/revdiff/main_test.go index a2f8a2a0..09cd8c4a 100644 --- a/app/main_test.go +++ b/app/revdiff/main_test.go @@ -9,6 +9,7 @@ import ( "os/exec" "path/filepath" "runtime" + "runtime/debug" "testing" "time" @@ -16,6 +17,29 @@ import ( "github.com/stretchr/testify/require" ) +func TestBuildVersion(t *testing.T) { + tests := []struct { + name string + revision string + info *debug.BuildInfo + want string + }{ + {name: "ldflags revision wins", revision: "v1.14.0-custom", info: &debug.BuildInfo{Main: debug.Module{Version: "v1.14.0"}}, want: "v1.14.0-custom"}, + {name: "ldflags without build info", revision: "v1.14.0-custom", want: "v1.14.0-custom"}, + {name: "installed module version", revision: "unknown", info: &debug.BuildInfo{Main: debug.Module{Version: "v1.14.0"}}, want: "v1.14.0"}, + {name: "empty revision", info: &debug.BuildInfo{Main: debug.Module{Version: "v1.14.0"}}, want: "v1.14.0"}, + {name: "development build", revision: "unknown", info: &debug.BuildInfo{Main: debug.Module{Version: "(devel)"}}, want: "unknown"}, + {name: "empty module version", revision: "unknown", info: &debug.BuildInfo{}, want: "unknown"}, + {name: "missing build info", revision: "unknown", want: "unknown"}, + {name: "missing revision and build info", want: "unknown"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, buildVersion(tt.revision, tt.info)) + }) + } +} + type errWriter struct{} func (errWriter) Write([]byte) (int, error) { return 0, errors.New("write failed") } diff --git a/app/plugin_exit_code_test.go b/app/revdiff/plugin_exit_code_test.go similarity index 99% rename from app/plugin_exit_code_test.go rename to app/revdiff/plugin_exit_code_test.go index 12d9dee4..7b262cdc 100644 --- a/app/plugin_exit_code_test.go +++ b/app/revdiff/plugin_exit_code_test.go @@ -992,6 +992,7 @@ func TestCodexPlanReviewHook(t *testing.T) { liveTranscript := filepath.Join( root, "app", + "revdiff", "testdata", "plugin-exit-code", "rollout-2026-07-16T10-54-26-session-current.jsonl", @@ -1709,7 +1710,7 @@ func testRepoRoot(t *testing.T) string { t.Helper() wd, err := os.Getwd() require.NoError(t, err) - return filepath.Dir(wd) + return filepath.Dir(filepath.Dir(wd)) } func runTestCmd(t *testing.T, r cmdReq) cmdResult { diff --git a/app/quitter_moq_test.go b/app/revdiff/quitter_moq_test.go similarity index 100% rename from app/quitter_moq_test.go rename to app/revdiff/quitter_moq_test.go diff --git a/app/renderer_setup.go b/app/revdiff/renderer_setup.go similarity index 100% rename from app/renderer_setup.go rename to app/revdiff/renderer_setup.go diff --git a/app/renderer_setup_test.go b/app/revdiff/renderer_setup_test.go similarity index 100% rename from app/renderer_setup_test.go rename to app/revdiff/renderer_setup_test.go diff --git a/app/reviewinfo.go b/app/revdiff/reviewinfo.go similarity index 100% rename from app/reviewinfo.go rename to app/revdiff/reviewinfo.go diff --git a/app/reviewinfo_test.go b/app/revdiff/reviewinfo_test.go similarity index 100% rename from app/reviewinfo_test.go rename to app/revdiff/reviewinfo_test.go diff --git a/app/signal.go b/app/revdiff/signal.go similarity index 100% rename from app/signal.go rename to app/revdiff/signal.go diff --git a/app/signal_test.go b/app/revdiff/signal_test.go similarity index 100% rename from app/signal_test.go rename to app/revdiff/signal_test.go diff --git a/app/stdin.go b/app/revdiff/stdin.go similarity index 100% rename from app/stdin.go rename to app/revdiff/stdin.go diff --git a/app/stdin_test.go b/app/revdiff/stdin_test.go similarity index 100% rename from app/stdin_test.go rename to app/revdiff/stdin_test.go diff --git a/app/termbg.go b/app/revdiff/termbg.go similarity index 100% rename from app/termbg.go rename to app/revdiff/termbg.go diff --git a/app/termbg_test.go b/app/revdiff/termbg_test.go similarity index 100% rename from app/termbg_test.go rename to app/revdiff/termbg_test.go diff --git a/app/testdata/plugin-exit-code/fake-overlay-backend.sh b/app/revdiff/testdata/plugin-exit-code/fake-overlay-backend.sh similarity index 100% rename from app/testdata/plugin-exit-code/fake-overlay-backend.sh rename to app/revdiff/testdata/plugin-exit-code/fake-overlay-backend.sh diff --git a/app/testdata/plugin-exit-code/fake-revdiff-output.sh b/app/revdiff/testdata/plugin-exit-code/fake-revdiff-output.sh similarity index 100% rename from app/testdata/plugin-exit-code/fake-revdiff-output.sh rename to app/revdiff/testdata/plugin-exit-code/fake-revdiff-output.sh diff --git a/app/testdata/plugin-exit-code/fake-stdout-launcher.sh b/app/revdiff/testdata/plugin-exit-code/fake-stdout-launcher.sh similarity index 100% rename from app/testdata/plugin-exit-code/fake-stdout-launcher.sh rename to app/revdiff/testdata/plugin-exit-code/fake-stdout-launcher.sh diff --git a/app/testdata/plugin-exit-code/rollout-2026-07-16T10-54-26-session-current.jsonl b/app/revdiff/testdata/plugin-exit-code/rollout-2026-07-16T10-54-26-session-current.jsonl similarity index 100% rename from app/testdata/plugin-exit-code/rollout-2026-07-16T10-54-26-session-current.jsonl rename to app/revdiff/testdata/plugin-exit-code/rollout-2026-07-16T10-54-26-session-current.jsonl diff --git a/app/testdata/themes/corrupted.ini b/app/revdiff/testdata/themes/corrupted.ini similarity index 100% rename from app/testdata/themes/corrupted.ini rename to app/revdiff/testdata/themes/corrupted.ini diff --git a/app/testdata/themes/duplicate.ini b/app/revdiff/testdata/themes/duplicate.ini similarity index 100% rename from app/testdata/themes/duplicate.ini rename to app/revdiff/testdata/themes/duplicate.ini diff --git a/app/testdata/themes/good.ini b/app/revdiff/testdata/themes/good.ini similarity index 100% rename from app/testdata/themes/good.ini rename to app/revdiff/testdata/themes/good.ini diff --git a/app/testdata/themes/no_theme.ini b/app/revdiff/testdata/themes/no_theme.ini similarity index 100% rename from app/testdata/themes/no_theme.ini rename to app/revdiff/testdata/themes/no_theme.ini diff --git a/app/themes.go b/app/revdiff/themes.go similarity index 100% rename from app/themes.go rename to app/revdiff/themes.go diff --git a/app/themes_test.go b/app/revdiff/themes_test.go similarity index 99% rename from app/themes_test.go rename to app/revdiff/themes_test.go index da1f7207..c9945eba 100644 --- a/app/themes_test.go +++ b/app/revdiff/themes_test.go @@ -887,7 +887,7 @@ func parsePatchedConfig(t *testing.T, path string) (options, error) { func TestPatchConfigTheme_testdataRoundTrip(t *testing.T) { tests := []struct { name string - fixture string // path under app/testdata/themes/ + fixture string }{ {name: "good config (theme already in [Application Options])", fixture: "good.ini"}, {name: "no theme line, trailing [color options]", fixture: "no_theme.ini"}, diff --git a/app/ui/doc.go b/app/ui/doc.go index 4e66f160..d5da1fd7 100644 --- a/app/ui/doc.go +++ b/app/ui/doc.go @@ -44,14 +44,14 @@ // // Theme discovery and persistence are accessed through the [ThemeCatalog] interface defined // in model.go. This package does not import app/theme or app/fsutil — the concrete adapter -// is wired in app/themes.go, composing theme.Catalog with config file persistence. +// is wired in app/revdiff/themes.go, composing theme.Catalog with config file persistence. // // Intra-line word-diff algorithms and the shared highlight marker insertion engine live // in the [worddiff] sub-package (app/ui/worddiff/). It owns the tokenizer, LCS algorithm, // line pairing, similarity gate, and ANSI-aware highlight marker insertion used by both // word-diff and search highlighting. Model holds the worddiff type through a consumer-side // interface (wordDiffer) defined in model.go; concrete *worddiff.Differ is injected via -// ModelConfig.WordDiffer wired in app/main.go. +// ModelConfig.WordDiffer wired in app/revdiff/main.go. // // Color and style management lives in the [style] sub-package (app/ui/style/). // It owns all hex-to-ANSI conversion, lipgloss style construction, SGR state tracking, @@ -64,7 +64,7 @@ // including cursor/offset management, entry parsing, and rendering logic. // Model holds sidepane types through consumer-side interfaces (FileTreeComponent, // TOCComponent) defined in model.go; concrete construction is injected via -// ModelConfig.NewFileTree and ModelConfig.ParseTOC factory closures wired in app/main.go. +// ModelConfig.NewFileTree and ModelConfig.ParseTOC factory closures wired in app/revdiff/main.go. // // Layered popup UI lives in the [overlay] sub-package (app/ui/overlay/). // It owns help, annotation list, theme selector, and file picker overlays — all popup state @@ -74,7 +74,7 @@ // coordinator enforces mutual exclusivity (one overlay at a time) and routes // key events and compose calls to the active overlay. Model holds the Manager // through a consumer-side interface (overlayManager) defined in model.go; -// concrete *overlay.Manager is injected via ModelConfig.Overlay wired in app/main.go. +// concrete *overlay.Manager is injected via ModelConfig.Overlay wired in app/revdiff/main.go. // // The key interfaces consumed by Model are [Renderer] (provides changed files and diffs), // [SyntaxHighlighter] (provides ANSI-highlighted lines), [Blamer] (provides blame data), diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index b438005a..9aee0b99 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -6,7 +6,7 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit ``` ┌─────────────────────────────────────────────────────┐ -│ app/ — composition root (package main) │ +│ app/revdiff/ — composition root (package main) │ │ main.go — main(), early-exit flow │ │ config.go — options, parseArgs, config IO │ │ stdin.go — stdin validation, /dev/tty │ @@ -35,7 +35,7 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit ## Package Responsibilities -### app/ (composition root) +### app/revdiff/ (composition root) `package main` is the composition root, split across files by concern: @@ -169,7 +169,7 @@ across files by concern to keep files under ~500 lines: `flushWheelPending()` is called from `handleWheelDebounce`, `handleKey`, `handleResize`, and `handleBlameLoaded` (any path that runs `syncViewportToCursor` or reads `m.nav.diffCursor` must flush first). Mouse tracking is enabled program-wide via `tea.WithMouseCellMotion()` in - `app/main.go` unless `--no-mouse` / `REVDIFF_NO_MOUSE` is set + `app/revdiff/main.go` unless `--no-mouse` / `REVDIFF_NO_MOUSE` is set Each source file has a matching `_test.go`. @@ -196,7 +196,7 @@ mini-models. **Theme boundary** — `app/ui` does not import `app/theme` or `app/fsutil`. Theme discovery and persistence are accessed through the `ThemeCatalog` interface (defined in `model.go`), with a -concrete adapter wired in `app/themes.go`. +concrete adapter wired in `app/revdiff/themes.go`. ### app/ui/style/ — color, style resolution, and display helpers @@ -342,7 +342,7 @@ File layout: Bundled themes: revdiff, catppuccin-mocha, catppuccin-latte, dracula, gruvbox, nord, solarized-dark. Community themes live in `themes/gallery/`. -23 color keys mapped via `colorFieldPtrs()` in `app/themes.go` — single source of truth for color +23 color keys mapped via `colorFieldPtrs()` in `app/revdiff/themes.go` — single source of truth for color key to struct field mapping. ### app/annotation/ — annotation store @@ -398,7 +398,7 @@ On a signal-delivered exit (a SIGHUP from a dropped SSH/tmux client, or a SIGTER `main.go` invokes this save as a crash-recovery net and stops there — history only, never the `-o` output. This is a deliberate semantic change: a signal-delivered SIGTERM no longer writes `-o`, because a signal is not the deliberate handoff that `q`/`O` perform. The wiring lives at the -composition root — `shutdownGuard` in `app/signal.go` plus `tea.WithoutSignalHandler()` so revdiff +composition root — `shutdownGuard` in `app/revdiff/signal.go` plus `tea.WithoutSignalHandler()` so revdiff owns SIGHUP/SIGTERM instead of bubbletea. SIGINT is caught and drained so a Ctrl-C during an external `$EDITOR` does not quit revdiff. The guard is stopped (default signal disposition restored for all three) before `finalize()` runs, so a slow or hung finalize (`saveHistory` shells out to @@ -434,7 +434,7 @@ belong to the consumer. `OpenFilePicker()`, `OpenInfo()`, `UpdateInfo()`, `Close()`, `HandleKey()`, `HandleMouse()`, `Compose()`; implemented by `overlay.Manager` - **`ThemeCatalog`** — `Entries()`, `Resolve()`, `Persist()`; implemented by `themeCatalog` adapter - in `app/themes.go` (composes `theme.Catalog` + config persistence) + in `app/revdiff/themes.go` (composes `theme.Catalog` + config persistence) - **`ExternalEditor`** — `Command(content)` for annotation temp-file editing, `SourceCommand(path string, line int)` for opening source files; implemented by `editor.Editor` (default wiring via `ModelConfig.Editor`; stubbed in tests) @@ -601,14 +601,14 @@ User presses '?' / '@' / 'T' / 'P' / 'i' - **History**: `~/.config/revdiff/history/` (auto-save dir) Theme precedence: `--theme` overwrites all 23 color fields + chroma-style, ignoring `--color-*` -flags or env vars. Applied via `applyTheme()` in `app/themes.go` which directly overwrites +flags or env vars. Applied via `applyTheme()` in `app/revdiff/themes.go` which directly overwrites `opts.Colors.*` fields after `parseArgs()`. Adding a new color requires changes in three places: `theme.go` colorKeys + options struct + -`colorFieldPtrs()` in `app/themes.go`. +`colorFieldPtrs()` in `app/revdiff/themes.go`. Theme ownership is split by concern: `app/theme` owns discovery/loading/installation via `Catalog`, -`app/ui` consumes a `ThemeCatalog` interface for selector/preview/apply, and `app/themes.go` wires a +`app/ui` consumes a `ThemeCatalog` interface for selector/preview/apply, and `app/revdiff/themes.go` wires a thin adapter composing `theme.Catalog` + config file persistence. ## Input Modes diff --git a/flake.nix b/flake.nix index 691a3ded..161595ea 100644 --- a/flake.nix +++ b/flake.nix @@ -28,7 +28,7 @@ # so build straight from the vendored tree with no network fetch. vendorHash = null; - subPackages = [ "app" ]; + subPackages = [ "app/revdiff" ]; # Tests need the git working tree, which is absent in the Nix sandbox. doCheck = false; @@ -44,12 +44,6 @@ "-X main.revision=${version}" ]; - # The main package lives in ./app, so the produced binary is named - # `app`; rename it to `revdiff`. - postInstall = '' - mv $out/bin/app $out/bin/revdiff - ''; - meta = { description = "TUI for reviewing diffs, files, and documents with inline annotations"; homepage = "https://github.com/umputun/revdiff"; diff --git a/plugins/codex/skills/revdiff/references/install.md b/plugins/codex/skills/revdiff/references/install.md index 935ca56f..0253159a 100644 --- a/plugins/codex/skills/revdiff/references/install.md +++ b/plugins/codex/skills/revdiff/references/install.md @@ -5,6 +5,14 @@ brew install umputun/apps/revdiff ``` +**Go:** + +```bash +go install github.com/umputun/revdiff/app/revdiff@latest +``` + +Installs the `revdiff` binary into `GOBIN` (defaults to `$(go env GOPATH)/bin`). Add that directory to your `PATH`. + **Binary releases:** download from [GitHub Releases](https://github.com/umputun/revdiff/releases) (deb, rpm, archives for linux/darwin amd64/arm64). ## Codex Plugin diff --git a/site/docs.html b/site/docs.html index fd0ebbc6..9e98398e 100644 --- a/site/docs.html +++ b/site/docs.html @@ -125,6 +125,9 @@

Debian/Ubuntu

RPM-based (Fedora, RHEL)

sudo rpm -i revdiff_*.rpm

Download the latest .rpm for your architecture from GitHub Releases.

+

Go

+
go install github.com/umputun/revdiff/app/revdiff@latest
+

Installs the revdiff binary into GOBIN (defaults to $(go env GOPATH)/bin). Add that directory to your PATH.

Binary releases

Download from GitHub Releases — deb, rpm, archives for linux/darwin amd64/arm64.