diff --git a/.claude-plugin/skills/revdiff/SKILL.md b/.claude-plugin/skills/revdiff/SKILL.md index 69affbd8..ecdbc96c 100644 --- a/.claude-plugin/skills/revdiff/SKILL.md +++ b/.claude-plugin/skills/revdiff/SKILL.md @@ -1,6 +1,6 @@ --- name: revdiff -description: Review diffs, files, and documents with inline annotations in a TUI overlay, or answer questions about revdiff usage, configuration, themes, and keybindings. Opens revdiff in tmux/zellij/kitty/wezterm/cmux/ghostty/iterm2/emacs-vterm, captures annotations, and addresses them. Works in git, hg, and jj repos (auto-detected). Activates on "revdiff", "review diff", "review changes", "annotate diff", "git review with revdiff", "hg review with revdiff", "review jj change", "interactive diff review", "revdiff all files", "review all files", "browse all files", "revdiff ", "revdiff README.md", "revdiff /tmp/notes.txt", "review this file", "annotate this file", "review file with revdiff", "revdiff config", "revdiff themes", "revdiff keybindings", "how to configure revdiff", "what themes does revdiff have". +description: Review diffs, files, and documents with inline annotations in a TUI overlay, or answer questions about revdiff usage, configuration, themes, and keybindings. Opens revdiff in tmux/zellij/kitty/wezterm/cmux/ghostty/iterm2/emacs-vterm, captures annotations, and addresses them. Works in git, hg, and jj repos (auto-detected). Activates on "revdiff", "review diff", "review changes", "annotate diff", "git review with revdiff", "hg review with revdiff", "review jj change", "interactive diff review", "revdiff all files", "review all files", "browse all files", "revdiff ", "revdiff README.md", "revdiff /tmp/notes.txt", "review this file", "annotate this file", "review file with revdiff", "open this review in revdiff", "show review in revdiff", "review in revdiff", "revdiff config", "revdiff themes", "revdiff keybindings", "how to configure revdiff", "what themes does revdiff have". argument-hint: 'optional: ref(s), "all files", or file path' allowed-tools: [Bash, Read, Edit, Write, Grep, Glob] --- @@ -18,6 +18,7 @@ Review diffs with inline annotations using revdiff TUI in a terminal overlay. Wo - "revdiff all files exclude vendor" - "revdiff README.md", "revdiff docs/plan.md", "revdiff /tmp/notes.txt" — single-file review (`--only` mode) - "review this file", "annotate this file", "review file with revdiff" +- "open this review in revdiff", "show review in revdiff", "review in revdiff" — open an in-session review (preload mode) ## Answering Questions @@ -37,6 +38,10 @@ ${CLAUDE_SKILL_DIR}/scripts/read-latest-history.sh The script resolves the history dir from `$REVDIFF_HISTORY_DIR` (default `~/.config/revdiff/history`), finds the repo subdir via VCS root basename (jj/git/hg), and prints the newest `.md` file found. Each history file contains a header (path, refs, and — when available — a git commit hash), the annotations in `## file:line (type)` format, and the raw git diff for annotated files. The `commit:` line and diff block are captured from git only; in hg/jj repos the diff block will be empty and no commit hash is recorded. See `references/usage.md` "Review History" section for directory layout, stdin/only handling, and override options. +## Opening an In-Session Review + +When the user asks to open an in-session review in revdiff (the conversation already contains review comments produced earlier in the session), write those comments to a temp file (e.g. `/tmp/revdiff-review-XXXXXX.md`) using the format documented in `references/usage.md` ("Output Format" section), then run the normal launcher flow (Step 1 ref detection, Step 2 invocation) with `--annotations=` appended. Step 3 onward handles the curated annotations as usual. + ## How It Works 1. Launch revdiff in a terminal overlay (tmux popup, Zellij floating pane, kitty overlay, wezterm/Kaku split-pane, cmux split, ghostty split+zoom, iTerm2 split pane, or Emacs vterm frame) diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index 8ad58a96..5249ac7e 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -246,6 +246,10 @@ Comment body lines starting with `## ` (the record-header form) are prefixed wit Use `--output` / `-o` flag to write annotations to a file instead of stdout. +## Preloading Annotations + +Use `--annotations=PATH` to preload the annotation store from a markdown file in the same `-o` format. The format is bidirectional: any file written by `-o` can be read back via `--annotations` for round-trip workflows — review, quit, edit the file externally, relaunch, and continue from the preloaded state. + ## Review History When you quit with annotations (`q`), revdiff automatically saves a copy of the review session to `~/.config/revdiff/history//.md`. This is a safety net — if annotations are lost (process crash, agent fails to capture stdout), the history file preserves them. diff --git a/README.md b/README.md index f647a18e..5d2f7875 100644 --- a/README.md +++ b/README.md @@ -318,6 +318,7 @@ Positional arguments support several forms: | `-X`, `--exclude` | Exclude files matching prefix, may be repeated, env: `REVDIFF_EXCLUDE` (comma-separated) | | | `-F`, `--only` | Show only matching files by exact path or suffix, may be repeated (e.g. `--only=model.go`) | | | `-o`, `--output` | Write annotations to file instead of stdout, env: `REVDIFF_OUTPUT` | | +| `--annotations` | Preload annotations from a markdown file in `-o` format | | | `--history-dir` | Directory for review history auto-saves, env: `REVDIFF_HISTORY_DIR` | `~/.config/revdiff/history/` | | `--config` | Path to config file, env: `REVDIFF_CONFIG` | `~/.config/revdiff/config` | | `--keys` | Path to keybindings file, env: `REVDIFF_KEYS` | `~/.config/revdiff/keybindings` | @@ -484,8 +485,15 @@ printf '# Plan\n\nShip it\n' | revdiff --stdin --stdin-name plan.md # capture annotations from generated output some-command | revdiff --stdin --output /tmp/annotations.txt + +# round-trip: capture, edit externally, reload +revdiff -o review.md HEAD~1 +$EDITOR review.md +revdiff --annotations=review.md HEAD~1 ``` +`--annotations` reads the same markdown format that `-o` writes (see [Output Format](#output-format) below), so any file revdiff produces can be loaded back. You can also hand-author or generate that file from any other source — each record is `## path/to/file.go:LINE (+)` followed by the comment body — then step through the comments inline against the actual diff, edit or delete them in the TUI, and quit with `q` to write the final set to stdout (or to `-o`). + ### All-Files Mode Use `--all-files` (or `-A`) to browse all tracked files in a project, not just files with changes. This turns revdiff into a general-purpose code annotation tool. All files are shown in context-only mode (no `+`/`-` markers) with full annotation and syntax highlighting support. @@ -525,7 +533,7 @@ Two scenarios trigger this mode: Use `--stdin` to review arbitrary piped or redirected text as a single synthetic file. All lines are shown as context, so the normal single-file review flow still works: annotations, file-level notes, search, wrap, collapsed mode, and structured output. -`--stdin` is explicit and mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--include`, and `--exclude`. stdin mode requires piped or redirected input; plain terminal stdin is rejected to avoid accidentally launching an empty scratch buffer. +`--stdin` is explicit and mutually exclusive with refs, `--staged`, `--only`, `--all-files`, `--include`, `--exclude`, and `--annotations`. stdin mode requires piped or redirected input; plain terminal stdin is rejected to avoid accidentally launching an empty scratch buffer. Use `--stdin-name` to control the synthetic filename. This gives annotation output a stable key and enables filename-based syntax highlighting or markdown TOC activation: diff --git a/app/annotation/parse.go b/app/annotation/parse.go new file mode 100644 index 00000000..723bfb49 --- /dev/null +++ b/app/annotation/parse.go @@ -0,0 +1,151 @@ +package annotation + +import ( + "bufio" + "errors" + "fmt" + "io" + "regexp" + "strconv" + "strings" +) + +// headerRe matches the four record-header shapes emitted by Store.FormatOutput: +// +// ## path (file-level) +// ## path:N (T) +// ## path:N-M (T) +// +// where T is one of "+", "-", or " " (literal space). +var headerRe = regexp.MustCompile(`^## (.+?)(?::(\d+)(?:-(\d+))?)? \((file-level|\+|-| )\)$`) + +// Parse reads the markdown produced by Store.FormatOutput and returns the +// recovered annotations in source order. Duplicates (same file/line/type) are +// returned as separate records; callers feed them through Store.Add to apply +// last-write-wins semantics. +// +// A line beginning with "## " that does NOT match the header grammar is folded +// into the body of the current record so hand-authored or LLM-generated bodies +// can mention "## something" without escaping. If such a line appears before +// any record header, it is reported as an error. +func Parse(r io.Reader) ([]Annotation, error) { + p := parser{scanner: bufio.NewScanner(r)} + p.scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) + return p.parse() +} + +type parser struct { + scanner *bufio.Scanner + + out []Annotation + current *Annotation + body []string + seenHeader bool + nonBlankSeen bool +} + +func (p *parser) parse() ([]Annotation, error) { + for p.scanner.Scan() { + line := p.scanner.Text() + if strings.HasPrefix(line, "## ") { + ann, err := p.parseHeader(line) + if err != nil { + // non-grammar "## " line inside a record: treat as body content + // (post-strip of any leading-space escape) so authored bodies + // can mention "## foo" without escaping. Before the first + // header, propagate the error. + if !p.seenHeader { + return nil, err + } + p.appendBody(line) + continue + } + p.flush() + p.seenHeader = true + p.current = &ann + continue + } + + if !p.seenHeader { + if strings.TrimSpace(line) == "" { + continue + } + p.nonBlankSeen = true + break + } + + p.appendBody(line) + } + if err := p.scanner.Err(); err != nil { + return nil, fmt.Errorf("scan annotations: %w", err) + } + + if !p.seenHeader && p.nonBlankSeen { + return nil, errors.New("annotation input has content before any header") + } + + p.flush() + return p.out, nil +} + +// appendBody adds a body line, stripping the inverse of escapeHeaderLines: +// exactly one leading space when the line's first non-space content begins +// with "## ". +func (p *parser) appendBody(line string) { + if strings.HasPrefix(line, " ") && strings.HasPrefix(strings.TrimLeft(line, " "), "## ") { + line = line[1:] + } + p.body = append(p.body, line) +} + +func (p *parser) flush() { + if p.current == nil { + return + } + // FormatOutput always emits a trailing newline after the body. Strip + // exactly one trailing empty line that came from the format separator. + if n := len(p.body); n > 0 && p.body[n-1] == "" { + p.body = p.body[:n-1] + } + p.current.Comment = strings.Join(p.body, "\n") + p.out = append(p.out, *p.current) + p.current = nil + p.body = nil +} + +// parseHeader parses a single "## ..." header line into an Annotation. +// returns an error if the line does not match the expected grammar. +func (p *parser) parseHeader(line string) (Annotation, error) { + m := headerRe.FindStringSubmatch(line) + if m == nil { + return Annotation{}, fmt.Errorf("malformed annotation header: %q", line) + } + ann := Annotation{File: m[1]} + if m[4] == "file-level" { + // file-level headers are emitted as "## path (file-level)" with no + // numeric suffix on the path. if the regex consumed a `:N`/`:N-M` + // tail into the optional line group, the path itself ended in + // `:N`/`:N-M` — restore it so paths that look numeric round-trip. + if m[2] != "" { + ann.File += ":" + m[2] + if m[3] != "" { + ann.File += "-" + m[3] + } + } + return ann, nil + } + ann.Type = m[4] + n, err := strconv.Atoi(m[2]) + if err != nil { + return Annotation{}, fmt.Errorf("malformed annotation header line number: %q", line) + } + ann.Line = n + if m[3] != "" { + end, err := strconv.Atoi(m[3]) + if err != nil { + return Annotation{}, fmt.Errorf("malformed annotation header end line: %q", line) + } + ann.EndLine = end + } + return ann, nil +} diff --git a/app/annotation/parse_test.go b/app/annotation/parse_test.go new file mode 100644 index 00000000..b3fe9dc6 --- /dev/null +++ b/app/annotation/parse_test.go @@ -0,0 +1,211 @@ +package annotation + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParse_Empty(t *testing.T) { + got, err := Parse(strings.NewReader("")) + require.NoError(t, err) + assert.Nil(t, got) +} + +func TestParse_OnlyWhitespace(t *testing.T) { + got, err := Parse(strings.NewReader("\n\n \n")) + require.NoError(t, err) + assert.Nil(t, got) +} + +func TestParse_SingleAdd(t *testing.T) { + in := "## handler.go:43 (+)\nuse errors.Is()\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, Annotation{File: "handler.go", Line: 43, Type: "+", Comment: "use errors.Is()"}, got[0]) +} + +func TestParse_SingleDel(t *testing.T) { + in := "## store.go:18 (-)\nremoved\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, Annotation{File: "store.go", Line: 18, Type: "-", Comment: "removed"}, got[0]) +} + +func TestParse_SingleContext(t *testing.T) { + in := "## plan.md:3 ( )\ncontext note\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, Annotation{File: "plan.md", Line: 3, Type: " ", Comment: "context note"}, got[0]) +} + +func TestParse_FileLevel(t *testing.T) { + in := "## handler.go (file-level)\nneeds refactor\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, Annotation{File: "handler.go", Line: 0, Type: "", Comment: "needs refactor"}, got[0]) +} + +func TestParse_Range(t *testing.T) { + in := "## handler.go:43-67 (+)\nrefactor hunk\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, Annotation{File: "handler.go", Line: 43, EndLine: 67, Type: "+", Comment: "refactor hunk"}, got[0]) +} + +func TestParse_MultilineBody(t *testing.T) { + in := "## handler.go:43 (+)\nfirst point\nsecond point\nthird point\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "first point\nsecond point\nthird point", got[0].Comment) +} + +func TestParse_EscapedHeaderInBody(t *testing.T) { + // FormatOutput prefixes body lines starting with "## " with a single space. + // Parse must strip exactly one leading space from such lines. + in := "## handler.go:43 (+)\nbody mentions\n ## not a header\nstill body\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "body mentions\n## not a header\nstill body", got[0].Comment) +} + +func TestParse_MixedRecords(t *testing.T) { + in := "## a.go (file-level)\nfile note\n\n## a.go:10 (+)\nline note\n\n## b.go:5 (-)\nb note\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 3) + assert.Equal(t, Annotation{File: "a.go", Line: 0, Type: "", Comment: "file note"}, got[0]) + assert.Equal(t, Annotation{File: "a.go", Line: 10, Type: "+", Comment: "line note"}, got[1]) + assert.Equal(t, Annotation{File: "b.go", Line: 5, Type: "-", Comment: "b note"}, got[2]) +} + +func TestParse_DuplicateLastWriteWins(t *testing.T) { + // Parse returns both entries; Store.Add applies last-write-wins on insertion. + in := "## h.go:43 (+)\nold\n\n## h.go:43 (+)\nnew\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 2) + + s := NewStore() + for _, a := range got { + s.Add(a) + } + anns := s.Get("h.go") + require.Len(t, anns, 1) + assert.Equal(t, "new", anns[0].Comment) +} + +func TestParse_FileLevelPathWithColonNumberSuffix(t *testing.T) { + // A path that genuinely ends in `:N` (or `:N-M`) must round-trip through + // the file-level header form. The lazy regex would otherwise consume the + // numeric tail into the line-number group and silently rewrite the path. + cases := []struct { + in string + want string + }{ + {"## foo:12 (file-level)\nnote\n", "foo:12"}, + {"## weird:12-34 (file-level)\nnote\n", "weird:12-34"}, + } + for _, tc := range cases { + got, err := Parse(strings.NewReader(tc.in)) + require.NoError(t, err, "parse %q", tc.in) + require.Len(t, got, 1) + assert.Equal(t, tc.want, got[0].File) + assert.Equal(t, 0, got[0].Line) + } +} + +func TestParse_FileLevelPathWithColonRoundTrip(t *testing.T) { + s := NewStore() + s.Add(Annotation{File: "weird:12", Line: 0, Type: "", Comment: "file note"}) + parsed, err := Parse(strings.NewReader(s.FormatOutput())) + require.NoError(t, err) + require.Len(t, parsed, 1) + assert.Equal(t, "weird:12", parsed[0].File) + assert.Equal(t, 0, parsed[0].Line) +} + +func TestParse_MalformedHeader(t *testing.T) { + cases := []string{ + "## not a real header\nbody\n", + "## file.go:abc (+)\nbody\n", + "## file.go:10 (?)\nbody\n", + "## file.go:10\nbody\n", + } + for _, in := range cases { + _, err := Parse(strings.NewReader(in)) + assert.Error(t, err, "expected error for %q", in) + } +} + +func TestParse_BodyBeforeAnyHeader(t *testing.T) { + // Garbage before the first header is an error. + _, err := Parse(strings.NewReader("garbage\n## file.go:1 (+)\nbody\n")) + assert.Error(t, err) +} + +func TestParse_RoundTrip(t *testing.T) { + s := NewStore() + s.Add(Annotation{File: "a.go", Line: 0, Type: "", Comment: "file-level\nwith ## inside"}) + s.Add(Annotation{File: "a.go", Line: 10, Type: "+", Comment: "add note"}) + s.Add(Annotation{File: "a.go", Line: 20, EndLine: 25, Type: "-", Comment: "del range"}) + s.Add(Annotation{File: "b.go", Line: 5, Type: " ", Comment: "context\nmulti\n## escaped\nbottom"}) + + formatted := s.FormatOutput() + + parsed, err := Parse(strings.NewReader(formatted)) + require.NoError(t, err) + + got := NewStore() + for _, a := range parsed { + got.Add(a) + } + + assert.Equal(t, s.FormatOutput(), got.FormatOutput()) +} + +func TestParse_UnescapedHashInBodyToleratedAfterHeader(t *testing.T) { + // LLM- or hand-authored bodies can include lines starting with "## " + // without the leading-space escape. After a record header is in scope, + // such lines are folded into the body rather than rejected. + in := "## a.go:1 (+)\nbody before\n## not actually a header\nbody after\n" + got, err := Parse(strings.NewReader(in)) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "body before\n## not actually a header\nbody after", got[0].Comment) +} + +func TestStore_Load_RoundTrip(t *testing.T) { + src := NewStore() + src.Add(Annotation{File: "a.go", Line: 0, Type: "", Comment: "file note"}) + src.Add(Annotation{File: "a.go", Line: 5, Type: "+", Comment: "line note"}) + + dst := NewStore() + require.NoError(t, dst.Load(strings.NewReader(src.FormatOutput()))) + assert.Equal(t, src.FormatOutput(), dst.FormatOutput()) +} + +func TestParse_RoundTripPreservesIndentedHashHeader(t *testing.T) { + // Body lines whose non-space prefix is "## " must round-trip without + // losing leading whitespace, regardless of indent depth. + s := NewStore() + s.Add(Annotation{File: "a.go", Line: 1, Type: "+", Comment: "## zero indent"}) + s.Add(Annotation{File: "a.go", Line: 2, Type: "+", Comment: " ## one space"}) + s.Add(Annotation{File: "a.go", Line: 3, Type: "+", Comment: " ## two spaces"}) + + parsed, err := Parse(strings.NewReader(s.FormatOutput())) + require.NoError(t, err) + require.Len(t, parsed, 3) + assert.Equal(t, "## zero indent", parsed[0].Comment) + assert.Equal(t, " ## one space", parsed[1].Comment) + assert.Equal(t, " ## two spaces", parsed[2].Comment) +} diff --git a/app/annotation/store.go b/app/annotation/store.go index 6612276e..390cef14 100644 --- a/app/annotation/store.go +++ b/app/annotation/store.go @@ -2,6 +2,7 @@ package annotation import ( "fmt" + "io" "sort" "strings" ) @@ -112,6 +113,22 @@ func (s *Store) Files() []string { return files } +// Load parses markdown produced by FormatOutput from r and adds each recovered +// annotation via Add (so duplicate file/line/type pairs apply last-write-wins). +// It is the symmetric inverse of FormatOutput on the API surface; callers that +// need to filter records (e.g. drop orphans against a diff) should use Parse +// directly and Add the survivors themselves. +func (s *Store) Load(r io.Reader) error { + records, err := Parse(r) + if err != nil { + return err + } + for _, a := range records { + s.Add(a) + } + return nil +} + // FormatOutput produces the structured output format for stdout. // Files are sorted alphabetically, annotations within each file by line number. // Returns empty string if no annotations exist. @@ -152,18 +169,19 @@ func (s *Store) FormatOutput() string { return buf.String() } -// escapeHeaderLines prefixes any body line that starts with "## " (the -// record-header form) with a single space so parsers splitting on "## " record -// headers cannot confuse a body line for a new record header. Other heading -// forms like "### subheader" are not escaped since they cannot collide with -// the record-header split marker. +// escapeHeaderLines prefixes any body line whose first non-space content is +// "## " with a single extra space. The parser inverts this by stripping one +// leading space from any body line that, after left-trimming, begins with +// "## ". Escaping pre-indented variants (e.g. " ## ") keeps the round-trip +// symmetric for arbitrary user content. Other heading forms like "### " are +// not escaped since they cannot collide with the record-header split marker. func (s *Store) escapeHeaderLines(body string) string { if !strings.Contains(body, "## ") { return body } lines := strings.Split(body, "\n") for i, line := range lines { - if strings.HasPrefix(line, "## ") { + if strings.HasPrefix(strings.TrimLeft(line, " "), "## ") { lines[i] = " " + line } } diff --git a/app/annotations_load.go b/app/annotations_load.go new file mode 100644 index 00000000..77d21b28 --- /dev/null +++ b/app/annotations_load.go @@ -0,0 +1,248 @@ +package main + +import ( + "fmt" + "io" + "os" + "path/filepath" + + "github.com/umputun/revdiff/app/annotation" + "github.com/umputun/revdiff/app/diff" + "github.com/umputun/revdiff/app/ui" +) + +// maxAnnotationsFileSize caps the bytes read from --annotations. Annotation +// files aggregate many records (LLM reviews, history exports) so 1 MiB is a +// generous practical ceiling; anything larger almost certainly indicates the +// flag was pointed at the wrong file. +const maxAnnotationsFileSize = 1 << 20 + +// preloader bundles the inputs and warning sink for --annotations preload. +// All helpers are methods so call-sites stay free of the wide parameter +// lists earlier shapes accumulated. +type preloader struct { + store *annotation.Store + renderer ui.Renderer + ref string + staged bool + untrackedFn func() ([]string, error) + workDir string + warnOut io.Writer + + // lineCache memoises the (line, change-type) set per file so + // repeated annotations on the same file do not re-fetch its diff. + lineCache map[string]map[lineKey]struct{} +} + +type lineKey struct { + line int + kind string +} + +// preloadAnnotations parses the markdown file at path (same format as +// Store.FormatOutput) and feeds each record through store.Add after dropping +// orphans against the resolved diff. file-level records (Line == 0) require +// only that the file be present in ChangedFiles. Line-scoped records require +// both the file and a matching DiffLine (same line number for the record's +// change type) to be present. Untracked files (surfaced in the UI via the +// show-untracked toggle) are folded in via untrackedFn so annotations saved +// against them round-trip; their line-set is read from disk as all-added +// lines, mirroring ui.resolveEmptyDiff. Dropped records are warned to warnOut. +func preloadAnnotations(path string, store *annotation.Store, renderer ui.Renderer, ref string, staged bool, + untrackedFn func() ([]string, error), workDir string, warnOut io.Writer, +) error { + records, err := readAnnotationsFile(path) + if err != nil { + return err + } + + p := &preloader{ + store: store, + renderer: renderer, + ref: ref, + staged: staged, + untrackedFn: untrackedFn, + workDir: workDir, + warnOut: warnOut, + lineCache: make(map[string]map[lineKey]struct{}), + } + return p.load(records) +} + +// readAnnotationsFile rejects non-regular files (FIFO, device, anything that +// would block os.Open) and oversize inputs before parsing. The size guard +// runs first against info.Size() so we never start scanning a 100 MiB file +// just to reject it; io.LimitReader is layered on top as belt-and-braces in +// case the file grows between Stat and Open. +func readAnnotationsFile(path string) ([]annotation.Annotation, error) { + info, err := os.Stat(path) + if err != nil { + return nil, fmt.Errorf("open annotations file: %w", err) + } + if !info.Mode().IsRegular() { + return nil, fmt.Errorf("open annotations file: %s: not a regular file", path) + } + if info.Size() > maxAnnotationsFileSize { + return nil, fmt.Errorf("annotations file %s exceeds %d bytes (got %d)", path, maxAnnotationsFileSize, info.Size()) + } + f, err := os.Open(path) //nolint:gosec // user-supplied path is intentional + if err != nil { + return nil, fmt.Errorf("open annotations file: %w", err) + } + defer f.Close() + + records, err := annotation.Parse(io.LimitReader(f, maxAnnotationsFileSize+1)) + if err != nil { + return nil, fmt.Errorf("parse annotations: %w", err) + } + return records, nil +} + +func (p *preloader) load(records []annotation.Annotation) error { + known, err := p.resolveKnownFiles() + if err != nil { + return err + } + + for _, a := range records { + // Sanitize the comment text before it reaches Store.Add: the + // preload source is user- or LLM-supplied and may carry stray + // ANSI / CR-overwrite / C1 bytes that Renderer.AnnotationInline + // wraps into the TUI verbatim. Mirrors diff.sanitizeCommitText + // usage on commit Author/Subject/Body. + a.Comment = diff.SanitizeCommitText(a.Comment) + + status, ok := known[a.File] + if !ok { + p.warnf("warning: --annotations: file %q not in diff, dropping annotation\n", a.File) + continue + } + if a.Line == 0 { + p.store.Add(a) + continue + } + lines := p.lookupLineSet(a.File, status) + if _, ok := lines[lineKey{line: a.Line, kind: a.Type}]; !ok { + p.warnf("warning: --annotations: %s:%d (%s) not in diff, dropping\n", a.File, a.Line, a.Type) + continue + } + p.store.Add(a) + } + return nil +} + +// resolveKnownFiles returns the set of paths the preload should accept. +// Mirrors ui.loadFiles' assembly of the visible file set with one deliberate +// divergence: untracked files are always folded in here, whereas the UI only +// surfaces them when its show-untracked toggle is on. The preload is +// upstream of the toggle, so it has to accept either viewing mode for the +// round-trip to be lossless. +// +// When running with no ref and no --staged, staged-only FileAdded entries +// are folded in if the unstaged set is empty (matches the UI's empty-diff +// fallback). Files renamed since the annotations file was generated are +// keyed under their old path and will orphan-drop here — a known limitation +// of the path-based format. +func (p *preloader) resolveKnownFiles() (map[string]diff.FileStatus, error) { + files, err := p.renderer.ChangedFiles(p.ref, p.staged) + if err != nil { + return nil, fmt.Errorf("resolve diff for annotation preload: %w", err) + } + known := make(map[string]diff.FileStatus, len(files)) + for _, fe := range files { + known[fe.Path] = fe.Status + } + if p.untrackedFn != nil { + if ut, utErr := p.untrackedFn(); utErr != nil { + p.warnf("warning: --annotations: list untracked files: %v\n", utErr) + } else { + for _, path := range ut { + if _, ok := known[path]; !ok { + known[path] = diff.FileUntracked + } + } + } + } + if p.ref != "" || p.staged || len(files) > 0 { + return known, nil + } + stagedFiles, sErr := p.renderer.ChangedFiles("", true) + if sErr != nil { + p.warnf("warning: --annotations: resolve staged files: %v\n", sErr) + return known, nil + } + for _, fe := range stagedFiles { + if _, ok := known[fe.Path]; ok { + continue + } + if fe.Status == diff.FileAdded { + known[fe.Path] = fe.Status + } + } + return known, nil +} + +// lookupLineSet returns the cached line-set for file, fetching FileDiff on +// miss. Mirrors ui.resolveEmptyDiff: staged-only FileAdded entries retry with +// --cached when the request was unstaged, and FileUntracked entries are read +// from disk as all-added lines. +func (p *preloader) lookupLineSet(file string, status diff.FileStatus) map[lineKey]struct{} { + if lines, ok := p.lineCache[file]; ok { + return lines + } + var ( + dl []diff.DiffLine + err error + used string + ) + switch { + case status == diff.FileUntracked && p.workDir != "": + used = "read" + dl, err = diff.ReadFileAsAdded(filepath.Join(p.workDir, file)) + default: + used = "diff" + fileStaged := p.staged + if !p.staged && p.ref == "" && status == diff.FileAdded { + fileStaged = true + } + dl, err = p.renderer.FileDiff(p.ref, file, fileStaged, 0) + } + var lines map[lineKey]struct{} + if err != nil { + p.warnf("warning: --annotations: %s diff for %q: %v\n", used, file, err) + lines = map[lineKey]struct{}{} + } else { + lines = buildLineSet(dl) + } + p.lineCache[file] = lines + return lines +} + +func (p *preloader) warnf(format string, args ...any) { + _, _ = fmt.Fprintf(p.warnOut, format, args...) +} + +// buildLineSet maps each renderable diff line to its (line-number, change-type) +// key. Mirrors Model.diffLineNum: removals key on OldNum, all other change +// types key on NewNum. +func buildLineSet(lines []diff.DiffLine) map[lineKey]struct{} { + out := make(map[lineKey]struct{}, len(lines)) + for _, dl := range lines { + if dl.ChangeType == diff.ChangeDivider { + continue + } + var n int + switch dl.ChangeType { + case diff.ChangeRemove: + n = dl.OldNum + default: + n = dl.NewNum + } + if n == 0 { + continue + } + out[lineKey{line: n, kind: string(dl.ChangeType)}] = struct{}{} + } + return out +} + diff --git a/app/annotations_load_test.go b/app/annotations_load_test.go new file mode 100644 index 00000000..9768045e --- /dev/null +++ b/app/annotations_load_test.go @@ -0,0 +1,301 @@ +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/umputun/revdiff/app/annotation" + "github.com/umputun/revdiff/app/diff" + "github.com/umputun/revdiff/app/ui/mocks" +) + +func writeTempAnnotations(t *testing.T, body string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "notes.md") + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + return path +} + +func TestParseArgs_AnnotationsFlag(t *testing.T) { + opts, err := parseArgs(append(noConfigArgs(t), "--annotations", "/tmp/notes.md")) + require.NoError(t, err) + assert.Equal(t, "/tmp/notes.md", opts.Annotations) +} + +func TestParseArgs_AnnotationsDefaultEmpty(t *testing.T) { + opts, err := parseArgs(noConfigArgs(t)) + require.NoError(t, err) + assert.Empty(t, opts.Annotations) +} + +func TestPreloadAnnotations_FileNotFound(t *testing.T) { + store := annotation.NewStore() + r := &mocks.RendererMock{} + err := preloadAnnotations(filepath.Join(t.TempDir(), "missing.md"), store, r, "", false, nil, "", &bytes.Buffer{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "open annotations file") + assert.Equal(t, 0, store.Count()) +} + +func TestPreloadAnnotations_ParseError(t *testing.T) { + path := writeTempAnnotations(t, "## bad header without parens\nbody\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { return nil, nil }, + } + err := preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "parse annotations") +} + +func TestPreloadAnnotations_ChangedFilesError(t *testing.T) { + path := writeTempAnnotations(t, "## a.go (file-level)\nhi\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { return nil, errors.New("boom") }, + } + err := preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolve diff") +} + +func TestPreloadAnnotations_DropsOrphans(t *testing.T) { + body := "## a.go (file-level)\nfile-level note\n\n" + + "## a.go:5 (+)\nline-add note\n\n" + + "## a.go:99 (+)\notherwise valid file but bad line\n\n" + + "## ghost.go (file-level)\nghost\n\n" + + "## ghost.go:1 (+)\nghost line\n" + path := writeTempAnnotations(t, body) + + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + FileDiffFunc: func(_, file string, _ bool, _ int) ([]diff.DiffLine, error) { + assert.Equal(t, "a.go", file) + return []diff.DiffLine{ + {OldNum: 0, NewNum: 5, Content: "added", ChangeType: diff.ChangeAdd}, + {OldNum: 4, NewNum: 0, Content: "removed", ChangeType: diff.ChangeRemove}, + {OldNum: 6, NewNum: 6, Content: "ctx", ChangeType: diff.ChangeContext}, + }, nil + }, + } + warn := &bytes.Buffer{} + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", warn)) + + assert.Equal(t, 2, store.Count()) + all := store.All() + require.Len(t, all["a.go"], 2) + assert.Equal(t, 0, all["a.go"][0].Line) // file-level sorted first + assert.Equal(t, 5, all["a.go"][1].Line) + + out := warn.String() + assert.Contains(t, out, "a.go:99") + assert.Contains(t, out, "ghost.go") +} + +func TestPreloadAnnotations_RemovedLineMatchesOldNum(t *testing.T) { + path := writeTempAnnotations(t, "## a.go:4 (-)\nremoved-line note\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + FileDiffFunc: func(string, string, bool, int) ([]diff.DiffLine, error) { + return []diff.DiffLine{ + {OldNum: 4, NewNum: 0, Content: "gone", ChangeType: diff.ChangeRemove}, + }, nil + }, + } + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{})) + assert.Equal(t, 1, store.Count()) + assert.True(t, store.Has("a.go", 4, "-")) +} + +func TestPreloadAnnotations_ContextMatchesNewNum(t *testing.T) { + path := writeTempAnnotations(t, "## a.go:7 ( )\ncontext note\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + FileDiffFunc: func(string, string, bool, int) ([]diff.DiffLine, error) { + return []diff.DiffLine{ + {OldNum: 7, NewNum: 7, Content: "ctx", ChangeType: diff.ChangeContext}, + }, nil + }, + } + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{})) + assert.True(t, store.Has("a.go", 7, " ")) +} + +func TestPreloadAnnotations_DuplicateLastWriteWins(t *testing.T) { + path := writeTempAnnotations(t, "## a.go:5 (+)\nfirst\n\n## a.go:5 (+)\nsecond\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + FileDiffFunc: func(string, string, bool, int) ([]diff.DiffLine, error) { + return []diff.DiffLine{{NewNum: 5, ChangeType: diff.ChangeAdd}}, nil + }, + } + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{})) + assert.Equal(t, 1, store.Count()) + got := store.Get("a.go") + require.Len(t, got, 1) + assert.Equal(t, "second", got[0].Comment) +} + +func TestPreloadAnnotations_StagedOnlyAddedFile(t *testing.T) { + // Mirrors ui.loadFiles + ui.resolveEmptyDiff: when ref=="" and !staged + // and the unstaged set is empty, staged-only FileAdded entries surface in + // the UI. Annotations saved from that startup path must round-trip. + body := "## new.go (file-level)\nnote\n\n## new.go:2 (+)\nadd note\n" + path := writeTempAnnotations(t, body) + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(_ string, staged bool) ([]diff.FileEntry, error) { + if staged { + return []diff.FileEntry{{Path: "new.go", Status: diff.FileAdded}}, nil + } + return nil, nil + }, + FileDiffFunc: func(_, file string, staged bool, _ int) ([]diff.DiffLine, error) { + assert.Equal(t, "new.go", file) + assert.True(t, staged, "FileAdded with empty unstaged set must retry with --cached") + return []diff.DiffLine{ + {NewNum: 1, Content: "first", ChangeType: diff.ChangeAdd}, + {NewNum: 2, Content: "second", ChangeType: diff.ChangeAdd}, + }, nil + }, + } + warn := &bytes.Buffer{} + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", warn)) + assert.Equal(t, 2, store.Count(), "warnings: %s", warn.String()) + assert.True(t, store.Has("new.go", 2, "+")) +} + +func TestPreloadAnnotations_UntrackedFile(t *testing.T) { + // Untracked files surface in the UI via the show-untracked toggle, not + // renderer.ChangedFiles. Annotations saved against them must round-trip: + // the file is folded into the known set via untrackedFn and its line set + // is read from disk as all-added lines. + dir := t.TempDir() + filePath := filepath.Join(dir, "new.go") + require.NoError(t, os.WriteFile(filePath, []byte("first\nsecond\nthird\n"), 0o600)) + + body := "## new.go (file-level)\nfile note\n\n" + + "## new.go:2 (+)\nline note\n\n" + + "## new.go:99 (+)\nout-of-range\n" + path := writeTempAnnotations(t, body) + + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { return nil, nil }, + } + untracked := func() ([]string, error) { return []string{"new.go"}, nil } + + warn := &bytes.Buffer{} + require.NoError(t, preloadAnnotations(path, store, r, "", false, untracked, dir, warn)) + + assert.Equal(t, 2, store.Count(), "warnings: %s", warn.String()) + assert.True(t, store.Has("new.go", 0, ""), "file-level annotation must round-trip") + assert.True(t, store.Has("new.go", 2, "+"), "line-2 annotation must match disk content") + assert.Contains(t, warn.String(), "new.go:99") +} + +func TestPreloadAnnotations_UntrackedListError(t *testing.T) { + path := writeTempAnnotations(t, "## a.go (file-level)\nnote\n") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + } + untracked := func() ([]string, error) { return nil, errors.New("git boom") } + + warn := &bytes.Buffer{} + require.NoError(t, preloadAnnotations(path, store, r, "", false, untracked, "", warn)) + assert.Equal(t, 1, store.Count(), "tracked annotations survive when untracked listing fails") + assert.Contains(t, warn.String(), "list untracked files") +} + +func TestPreloadAnnotations_RejectsNonRegularFile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("FIFO not portable") + } + dir := t.TempDir() + fifo := filepath.Join(dir, "fifo") + require.NoError(t, syscall.Mkfifo(fifo, 0o600)) + + store := annotation.NewStore() + r := &mocks.RendererMock{} + err := preloadAnnotations(fifo, store, r, "", false, nil, "", &bytes.Buffer{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "not a regular file") +} + +func TestPreloadAnnotations_RejectsOversizeFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "big.md") + // header + body that scrolls past the 1 MiB cap + body := "## a.go:1 (+)\n" + strings.Repeat("x", maxAnnotationsFileSize+10) + "\n" + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + + store := annotation.NewStore() + r := &mocks.RendererMock{} + err := preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds") +} + +func TestPreloadAnnotations_SanitizesCommentText(t *testing.T) { + // Stray ANSI / CR-overwrite bytes in the source file must be stripped + // before reaching Store.Add — the TUI renderer wraps comment text in + // ANSI italic without a second pass. + body := "## a.go:5 (+)\nrogue \x1b[31mred\x1b[0m and \rcr-overwrite\n" + path := writeTempAnnotations(t, body) + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { + return []diff.FileEntry{{Path: "a.go", Status: diff.FileModified}}, nil + }, + FileDiffFunc: func(string, string, bool, int) ([]diff.DiffLine, error) { + return []diff.DiffLine{{NewNum: 5, ChangeType: diff.ChangeAdd}}, nil + }, + } + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{})) + got := store.Get("a.go") + require.Len(t, got, 1) + assert.NotContains(t, got[0].Comment, "\x1b") + assert.NotContains(t, got[0].Comment, "\r") + assert.Contains(t, got[0].Comment, "rogue red") + assert.Contains(t, got[0].Comment, "cr-overwrite") +} + +func TestValidateStdinFlags_RejectsAnnotations(t *testing.T) { + opts := options{Stdin: true, Annotations: "/tmp/n.md"} + err := validateStdinFlags(opts) + require.Error(t, err) + assert.Contains(t, err.Error(), "--annotations") +} + +func TestPreloadAnnotations_EmptyFile(t *testing.T) { + path := writeTempAnnotations(t, "") + store := annotation.NewStore() + r := &mocks.RendererMock{ + ChangedFilesFunc: func(string, bool) ([]diff.FileEntry, error) { return nil, nil }, + } + require.NoError(t, preloadAnnotations(path, store, r, "", false, nil, "", &bytes.Buffer{})) + assert.Equal(t, 0, store.Count()) +} diff --git a/app/config.go b/app/config.go index da087d65..62bda337 100644 --- a/app/config.go +++ b/app/config.go @@ -37,6 +37,7 @@ type options struct { AllFiles bool `long:"all-files" short:"A" no-ini:"true" description:"browse all tracked files, not just diffs (git and jj only)"` Stdin bool `long:"stdin" no-ini:"true" description:"review stdin as a scratch buffer"` StdinName string `long:"stdin-name" no-ini:"true" description:"synthetic file name for stdin content"` + Annotations string `long:"annotations" no-ini:"true" description:"preload annotations from a markdown file written by -o (round-trip)"` Exclude []string `long:"exclude" short:"X" ini-name:"exclude" env:"REVDIFF_EXCLUDE" env-delim:"," description:"exclude files matching prefix (may be repeated)"` Include []string `long:"include" short:"I" ini-name:"include" env:"REVDIFF_INCLUDE" env-delim:"," description:"include only files matching prefix (may be repeated)"` Only []string `long:"only" short:"F" no-ini:"true" description:"show only these files (may be repeated)"` diff --git a/app/diff/diff.go b/app/diff/diff.go index 3822e659..12f99bb3 100644 --- a/app/diff/diff.go +++ b/app/diff/diff.go @@ -120,6 +120,12 @@ const commitLogFormat = "%H%x1f%an <%ae>%x1f%cI%x1f%s%n%b" //nolint:gocritic // explicit ASCII 0x20..0x2F range for CSI intermediate bytes var ansiCSIRe = regexp.MustCompile("\x1b\\[[0-9;?]*[\x20-\x2f]*[a-zA-Z~]") +// SanitizeCommitText is the exported alias for sanitizeCommitText. Used by +// callers outside the diff package (e.g. preloaded annotation comments) that +// need the same control-byte / ANSI / C1 stripping applied before content +// reaches a terminal renderer. +func SanitizeCommitText(s string) string { return sanitizeCommitText(s) } + // sanitizeCommitText neutralizes bytes that could trigger terminal side effects // when a crafted commit Author/Subject/Body is rendered verbatim inside the // overlay. Used by VCS CommitLog parsers so the overlay renderer can treat the diff --git a/app/main.go b/app/main.go index dd6ae6c7..8dfb418c 100644 --- a/app/main.go +++ b/app/main.go @@ -127,6 +127,12 @@ func run(opts options) error { commitLogger = setup.commitLogger } + if opts.Annotations != "" { + if perr := preloadAnnotations(opts.Annotations, store, renderer, opts.ref(), opts.Staged, untrackedFn, workDir, os.Stderr); perr != nil { + return perr + } + } + // construct the three style types per D15: Resolver first, Renderer from Resolver, SGR is zero-value styleColors := optsToStyleColors(opts) var res style.Resolver diff --git a/app/stdin.go b/app/stdin.go index df221a18..59383d75 100644 --- a/app/stdin.go +++ b/app/stdin.go @@ -40,6 +40,9 @@ func validateStdinFlags(opts options) error { if len(opts.Include) > 0 { return errors.New("--stdin cannot be used with --include") } + if opts.Annotations != "" { + return errors.New("--stdin cannot be used with --annotations") + } return nil } diff --git a/docs/plans/completed/2026-04-27-preload-annotations.md b/docs/plans/completed/2026-04-27-preload-annotations.md new file mode 100644 index 00000000..9e013021 --- /dev/null +++ b/docs/plans/completed/2026-04-27-preload-annotations.md @@ -0,0 +1,76 @@ +# Preload annotations from FormatOutput markdown (--annotations flag) + +## Overview +Add a `--annotations=PATH` flag that preloads the annotation store at startup by parsing the same markdown format `Store.FormatOutput()` emits. Enables a round-trip workflow: `revdiff -o notes.md ...` → edit → `revdiff --annotations=notes.md ...`. Primary use case: seeding LLM-generated code review comments into the annotation gutter. Scope is locked by issue #153 discussion (11 + 2 points, all agreed by maintainer). Caller-side LLM tooling is out of scope — the contract is the bidirectional FormatOutput format. + +## Context +- Files involved: + - `app/annotation/store.go` — existing `Annotation` struct + `FormatOutput()` (write-only today; parser is the inverse) + - `app/annotation/store_test.go` — existing test patterns (direct string assertions, no fixtures) + - `app/config.go` — `options` struct, add `Annotations` field next to `Stdin` + - `app/main.go` — `run()` wires parser between config parsing and Bubble Tea start + - `app/diff/` — diff result is what we resolve files/lines against for the drop-orphans check + - `README.md` — one example under run-configuration + - `.claude-plugin/skills/revdiff/SKILL.md` and `references/usage.md` — short reference for AI agents +- Related patterns: + - `Stdin` flag in `options` (per-invocation, `no-ini:"true"`) + - `Store.Add` last-write-wins semantics (store.go:30-38) — preload uses the same path + - `escapeHeaderLines` (store.go:160-171) — parser inverts this +- Dependencies: none new + +## Development Approach +- Testing approach: TDD — parser is mechanical inverse of an already-tested format, table-driven tests fit naturally +- Complete each task fully before moving to the next +- CRITICAL: every task MUST include new/updated tests +- CRITICAL: all tests must pass before starting next task + +## Implementation Steps + +### Task 1: Parser in app/annotation/parse.go + +**Files:** +- Create: `app/annotation/parse.go` +- Create: `app/annotation/parse_test.go` + +- [x] write tests first: each header shape (`(+)`, `(-)`, `( )`, `(file-level)`), single line, range `n-m`, multiline body, escaped ` ## ` body line (one leading space stripped), mixed records, duplicate (last-write-wins via `Store.Add`), empty input = no-op + nil error, malformed header = error +- [x] write a `Parse(FormatOutput(s))` round-trip property test against a synthesized store covering all four shapes and a body containing `## ` +- [x] implement `func Parse(r io.Reader) ([]Annotation, error)` — single regex over header lines matching the four shapes; body = lines between header and next `## ` or EOF, trimmed of one trailing newline; invert `escapeHeaderLines` (strip exactly one leading space from body lines whose prefix is ` ## `); non-matching `## ` header = error (no silent skip) +- [x] regex must accept literal space inside parens: `(+)`, `(-)`, `( )`, `(file-level)` — per maintainer point 3 +- [x] run `make test` — must pass before task 2 + +### Task 2: CLI flag and run() wiring + +**Files:** +- Modify: `app/config.go` +- Modify: `app/main.go` + +- [x] add `Annotations string` to `options` next to `Stdin`, with `no-ini:"true"` and a long flag `annotations` taking a path +- [x] in `run()` after config parse, if `Annotations != ""`: open file (file-not-found = hard error to stderr, non-zero exit, before Bubble Tea starts), call `annotation.Parse`, on parse error fail hard; on success feed each record through `Store.Add` +- [x] resolve diff first, then drop orphans: file-level record (`Line=0`) requires file-in-diff; line-scoped requires both file and line present; warn on stderr per dropped record, drop silently from store +- [x] write tests for config parsing of the new flag and for the load+drop path (table-driven against a fake diff result if feasible, otherwise integration-style in `app/`) +- [x] run `make test` and `make lint` — must pass before task 3 + +### Task 3: Documentation + +**Files:** +- Modify: `README.md` +- Modify: `.claude-plugin/skills/revdiff/SKILL.md` +- Modify: `.claude-plugin/skills/revdiff/references/usage.md` +- Modify: `site/docs.html` (per CLAUDE.md sync rule with README) +- Modify: `plugins/codex/skills/revdiff/SKILL.md` if it mirrors the same flag list + +- [x] README: one example under run-configuration: `revdiff --annotations=review.md HEAD~1`, plus a one-line note that the format is the same `-o` output (round-trip) +- [x] SKILL.md + usage.md: short reference noting the flag exists and the format is the FormatOutput shape both directions +- [x] site/docs.html: add the flag to the options reference and a usage example, mirroring README +- [x] no test changes for docs — but verify: grep all occurrences of `--stdin` in docs and confirm `--annotations` is added in matching spots + +### Task 4: Verify acceptance criteria + +- [x] `make test` (race + coverage) passes — all packages green; toolchain version mismatch warning in env is pre-existing +- [x] `make lint` passes — 0 issues +- [x] coverage on `app/annotation/parse.go` ≥ 80% — 97.2% (Parse 97.2%, parseHeader 94.1%) + +### Task 5: Update changelog and move plan + +- [x] update CHANGELOG with `--annotations` entry under unreleased +- [x] move `docs/plans/2026-04-27-preload-annotations.md` to `docs/plans/completed/` diff --git a/plugins/codex/skills/revdiff/SKILL.md b/plugins/codex/skills/revdiff/SKILL.md index 9719f413..2701ab7b 100644 --- a/plugins/codex/skills/revdiff/SKILL.md +++ b/plugins/codex/skills/revdiff/SKILL.md @@ -1,6 +1,6 @@ --- name: revdiff -description: Review diffs, files, and documents with inline annotations in a TUI overlay, or answer questions about revdiff usage, configuration, themes, and keybindings. Opens revdiff in tmux/zellij/kitty/wezterm/cmux/ghostty/iterm2/emacs-vterm, captures annotations, and addresses them. Works in git, hg, and jj repos (auto-detected). Activates on "revdiff", "review diff", "review changes", "annotate diff", "git review with revdiff", "hg review with revdiff", "review jj change", "interactive diff review", "revdiff all files", "review all files", "browse all files", "revdiff ", "revdiff README.md", "revdiff /tmp/notes.txt", "review this file", "annotate this file", "review file with revdiff", "revdiff config", "revdiff themes", "revdiff keybindings", "how to configure revdiff", "what themes does revdiff have". +description: Review diffs, files, and documents with inline annotations in a TUI overlay, or answer questions about revdiff usage, configuration, themes, and keybindings. Opens revdiff in tmux/zellij/kitty/wezterm/cmux/ghostty/iterm2/emacs-vterm, captures annotations, and addresses them. Works in git, hg, and jj repos (auto-detected). Activates on "revdiff", "review diff", "review changes", "annotate diff", "git review with revdiff", "hg review with revdiff", "review jj change", "interactive diff review", "revdiff all files", "review all files", "browse all files", "revdiff ", "revdiff README.md", "revdiff /tmp/notes.txt", "review this file", "annotate this file", "review file with revdiff", "open this review in revdiff", "show review in revdiff", "review in revdiff", "revdiff config", "revdiff themes", "revdiff keybindings", "how to configure revdiff", "what themes does revdiff have". argument-hint: 'optional: ref(s), "all files", or file path' allowed-tools: [Bash, Read, Edit, Write, Grep, Glob] --- @@ -33,6 +33,7 @@ Use `$SCRIPT_DIR` in place of script paths throughout this skill. - "revdiff all files exclude vendor" - "revdiff README.md", "revdiff docs/plan.md", "revdiff /tmp/notes.txt" — single-file review (`--only` mode) - "review this file", "annotate this file", "review file with revdiff" +- "open this review in revdiff", "show review in revdiff", "review in revdiff" — open an in-session review (preload mode) ## Answering Questions @@ -52,6 +53,10 @@ $SCRIPT_DIR/read-latest-history.sh The script resolves the history dir from `$REVDIFF_HISTORY_DIR` (default `~/.config/revdiff/history`), finds the repo subdir via VCS root basename (jj/git/hg), and prints the newest `.md` file found. Each history file contains a header (path, refs, and — when available — a git commit hash), the annotations in `## file:line (type)` format, and the raw git diff for annotated files. The `commit:` line and diff block are captured from git only; in hg/jj repos the diff block will be empty and no commit hash is recorded. See `references/usage.md` "Review History" section for directory layout, stdin/only handling, and override options. +## Opening an In-Session Review + +When the user asks to open an in-session review in revdiff (the conversation already contains review comments produced earlier in the session), write those comments to a temp file (e.g. `/tmp/revdiff-review-XXXXXX.md`) using the format documented in `references/usage.md` ("Output Format" section), then run the normal launcher flow (Step 1 ref detection, Step 2 invocation) with `--annotations=` appended. Step 3 onward handles the curated annotations as usual. + ## How It Works 1. Launch revdiff in a terminal overlay (tmux popup, Zellij floating pane, kitty overlay, wezterm/Kaku split-pane, cmux split, ghostty split+zoom, iTerm2 split pane, or Emacs vterm frame) diff --git a/plugins/codex/skills/revdiff/references/usage.md b/plugins/codex/skills/revdiff/references/usage.md index 8ad58a96..5249ac7e 100644 --- a/plugins/codex/skills/revdiff/references/usage.md +++ b/plugins/codex/skills/revdiff/references/usage.md @@ -246,6 +246,10 @@ Comment body lines starting with `## ` (the record-header form) are prefixed wit Use `--output` / `-o` flag to write annotations to a file instead of stdout. +## Preloading Annotations + +Use `--annotations=PATH` to preload the annotation store from a markdown file in the same `-o` format. The format is bidirectional: any file written by `-o` can be read back via `--annotations` for round-trip workflows — review, quit, edit the file externally, relaunch, and continue from the preloaded state. + ## Review History When you quit with annotations (`q`), revdiff automatically saves a copy of the review session to `~/.config/revdiff/history//.md`. This is a safety net — if annotations are lost (process crash, agent fails to capture stdout), the history file preserves them. diff --git a/site/docs.html b/site/docs.html index 95e54080..53bd418c 100644 --- a/site/docs.html +++ b/site/docs.html @@ -202,7 +202,13 @@

Examples

printf '# Plan\n\nBody\n' | revdiff --stdin --stdin-name plan.md # capture annotations from generated output -some-command | revdiff --stdin --output /tmp/annotations.txt +some-command | revdiff --stdin --output /tmp/annotations.txt + +# round-trip: capture, edit externally, reload +revdiff -o review.md HEAD~1 +$EDITOR review.md +revdiff --annotations=review.md HEAD~1 +

--annotations reads the same markdown format that -o writes (see Output format), so any file revdiff produces can be loaded back. You can also hand-author or generate that file from any other source — each record is ## path/to/file.go:LINE (+) followed by the comment body — then step through the comments inline against the actual diff, edit or delete them in the TUI, and quit with q to write the final set to stdout (or to -o).

All-files mode

Use --all-files (or -A) to browse all tracked files, not just files with changes. This turns revdiff into a general-purpose code annotation tool. All files are shown in context-only mode with full annotation and syntax highlighting support.

@@ -222,7 +228,7 @@

Context-only file review

Scratch-buffer review

Use --stdin to review arbitrary piped or redirected text as a single synthetic file. All lines are treated as context, so the normal single-file review flow still works: annotations, file-level notes, search, wrap, collapsed mode, and structured output.

-

--stdin is explicit, requires piped or redirected input, and is mutually exclusive with refs, --staged, --only, --all-files, --include, and --exclude.

+

--stdin is explicit, requires piped or redirected input, and is mutually exclusive with refs, --staged, --only, --all-files, --include, --exclude, and --annotations.

Use --stdin-name to control the synthetic filename for annotation output and extension-based highlighting or markdown TOC activation.

echo "plain text" | revdiff --stdin printf '# Plan\n\nBody\n' | revdiff --stdin --stdin-name plan.md @@ -361,6 +367,7 @@

Options

-X, --excludeExclude files by prefix (repeatable) -F, --onlyShow only matching files -o, --outputWrite annotations to file + --annotationsPreload annotations from a markdown file in -o format --history-dirDirectory for review history auto-saves~/.config/revdiff/history/ --keysPath to keybindings file~/.config/revdiff/keybindings --dump-keysPrint effective keybindings and exit