From a41bf435555a065c441af01dee7531c7ba67db23 Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 19:21:03 +0300 Subject: [PATCH 1/8] feat(annotation): parser for FormatOutput markdown round-trip Add Parse for the markdown shape Store.FormatOutput emits so annotations can be reconstructed from previously-saved review files. Make escapeHeaderLines/parser body-line stripping symmetric on "left-trimmed prefix is '## '", so pre-indented "## " lines round-trip losslessly at any indent depth. --- app/annotation/parse.go | 132 ++++++++++++++++++++++++ app/annotation/parse_test.go | 190 +++++++++++++++++++++++++++++++++++ app/annotation/store.go | 13 +-- 3 files changed, 329 insertions(+), 6 deletions(-) create mode 100644 app/annotation/parse.go create mode 100644 app/annotation/parse_test.go diff --git a/app/annotation/parse.go b/app/annotation/parse.go new file mode 100644 index 00000000..a5ba8f8f --- /dev/null +++ b/app/annotation/parse.go @@ -0,0 +1,132 @@ +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 a hard +// error rather than a silent skip — the format is bidirectional and a stray +// header indicates a malformed input. +func Parse(r io.Reader) ([]Annotation, error) { + scanner := bufio.NewScanner(r) + scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) + + var ( + out []Annotation + current *Annotation + body []string + seenHeader bool + nonBlankSeen bool + ) + + flush := func() { + if current == nil { + return + } + // The format always emits a trailing newline after the body. Strip + // exactly one trailing empty line that came from the format separator. + if n := len(body); n > 0 && body[n-1] == "" { + body = body[:n-1] + } + current.Comment = strings.Join(body, "\n") + out = append(out, *current) + current = nil + body = nil + } + + for scanner.Scan() { + line := scanner.Text() + if strings.HasPrefix(line, "## ") { + ann, err := parseHeader(line) + if err != nil { + return nil, err + } + flush() + seenHeader = true + current = &ann + continue + } + + if !seenHeader { + if strings.TrimSpace(line) == "" { + continue + } + nonBlankSeen = true + break + } + + // Inverse of escapeHeaderLines: strip exactly one leading space from + // body lines whose first non-space content begins with "## ". + if strings.HasPrefix(line, " ") && strings.HasPrefix(strings.TrimLeft(line, " "), "## ") { + line = line[1:] + } + body = append(body, line) + } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("scan annotations: %w", err) + } + + if !seenHeader && nonBlankSeen { + return nil, errors.New("annotation input has content before any header") + } + + flush() + return out, nil +} + +// parseHeader parses a single "## ..." header line into an Annotation. +// returns an error if the line does not match the expected grammar. +func 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..d90eea8f --- /dev/null +++ b/app/annotation/parse_test.go @@ -0,0 +1,190 @@ +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_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..2231347d 100644 --- a/app/annotation/store.go +++ b/app/annotation/store.go @@ -152,18 +152,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 } } From 5dc605d021d128c85e5a3099e124e8112fab11cc Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 19:21:13 +0300 Subject: [PATCH 2/8] feat(annotations): --annotations flag preloads annotations on startup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an opt-in --annotations=PATH flag (no-ini, per-invocation only) that parses a markdown file in the same shape Store.FormatOutput emits and preloads the annotation store before the TUI starts. Records whose file/line are not present in the resolved diff are dropped with a stderr warning. Untracked files surface in the UI via a separate loadUntracked callback rather than ChangedFiles, so the preload folds them in via untrackedFn and reads their line set from disk via diff.ReadFileAsAdded — mirroring Model.resolveEmptyDiff so any file written by -o (including untracked) round-trips back through --annotations. --- app/annotations_load.go | 178 ++++++++++++++++++++++++++ app/annotations_load_test.go | 239 +++++++++++++++++++++++++++++++++++ app/config.go | 1 + app/main.go | 6 + 4 files changed, 424 insertions(+) create mode 100644 app/annotations_load.go create mode 100644 app/annotations_load_test.go diff --git a/app/annotations_load.go b/app/annotations_load.go new file mode 100644 index 00000000..937a0236 --- /dev/null +++ b/app/annotations_load.go @@ -0,0 +1,178 @@ +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" +) + +// 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 { + f, err := os.Open(path) //nolint:gosec // user-supplied path is intentional + if err != nil { + return fmt.Errorf("open annotations file: %w", err) + } + defer f.Close() + + records, err := annotation.Parse(f) + if err != nil { + return fmt.Errorf("parse annotations: %w", err) + } + + known, err := resolveKnownFiles(renderer, ref, staged, untrackedFn, warnOut) + if err != nil { + return err + } + + // cache parsed FileDiff per file so we don't re-fetch on each annotation + lineCache := make(map[string]map[lineKey]struct{}) + + for _, a := range records { + status, ok := known[a.File] + if !ok { + _, _ = fmt.Fprintf(warnOut, "warning: --annotations: file %q not in diff, dropping annotation\n", a.File) + continue + } + if a.Line == 0 { + store.Add(a) + continue + } + lines := lookupLineSet(renderer, ref, staged, workDir, a.File, status, lineCache, warnOut) + if _, ok := lines[lineKey{line: a.Line, kind: a.Type}]; !ok { + _, _ = fmt.Fprintf(warnOut, "warning: --annotations: %s:%d (%s) not in diff, dropping\n", a.File, a.Line, a.Type) + continue + } + store.Add(a) + } + return nil +} + +// resolveKnownFiles returns the set of paths the preload should accept, +// mirroring ui.loadFiles: when running with no ref and no --staged, staged-only +// FileAdded entries are folded in if the unstaged set is empty, and untracked +// files (when untrackedFn is provided) are always folded in so the +// show-untracked UI toggle round-trips. +func resolveKnownFiles(renderer ui.Renderer, ref string, staged bool, untrackedFn func() ([]string, error), + warnOut io.Writer, +) (map[string]diff.FileStatus, error) { + files, err := renderer.ChangedFiles(ref, 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 + } + // fold in untracked files so annotations against them round-trip; matches + // ui.loadFiles which appends FileUntracked entries from loadUntracked. + if untrackedFn != nil { + if ut, utErr := untrackedFn(); utErr != nil { + _, _ = fmt.Fprintf(warnOut, "warning: --annotations: list untracked files: %v\n", utErr) + } else { + for _, p := range ut { + if _, ok := known[p]; !ok { + known[p] = diff.FileUntracked + } + } + } + } + if ref != "" || staged || len(files) > 0 { + return known, nil + } + stagedFiles, sErr := renderer.ChangedFiles("", true) + if sErr != nil { + _, _ = fmt.Fprintf(warnOut, "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 lookupLineSet(renderer ui.Renderer, ref string, staged bool, workDir, file string, status diff.FileStatus, + cache map[string]map[lineKey]struct{}, warnOut io.Writer, +) map[lineKey]struct{} { + if lines, ok := cache[file]; ok { + return lines + } + var ( + dl []diff.DiffLine + err error + used string + ) + switch { + case status == diff.FileUntracked && workDir != "": + used = "read" + dl, err = diff.ReadFileAsAdded(filepath.Join(workDir, file)) + default: + used = "diff" + fileStaged := staged + if !staged && ref == "" && status == diff.FileAdded { + fileStaged = true + } + dl, err = renderer.FileDiff(ref, file, fileStaged, 0) + } + var lines map[lineKey]struct{} + if err != nil { + _, _ = fmt.Fprintf(warnOut, "warning: --annotations: %s diff for %q: %v\n", used, file, err) + lines = map[lineKey]struct{}{} + } else { + lines = buildLineSet(dl) + } + cache[file] = lines + return lines +} + +type lineKey struct { + line int + kind string +} + +// 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..622965bd --- /dev/null +++ b/app/annotations_load_test.go @@ -0,0 +1,239 @@ +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "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 (+)\northerwise 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_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/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 From 6d77fa7ac9a7f8fbc6fee86582ed54f8e9e0066a Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 19:21:22 +0300 Subject: [PATCH 3/8] docs(annotations): document --annotations flag and round-trip workflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a Preloading Annotations section to README, site docs, and the Claude/codex plugin usage references; note the bidirectional -o ⇄ --annotations workflow in CHANGELOG and the in-session review preload path in plugin SKILL files. --- .claude-plugin/skills/revdiff/SKILL.md | 7 ++++++- .claude-plugin/skills/revdiff/references/usage.md | 4 ++++ README.md | 8 ++++++++ plugins/codex/skills/revdiff/SKILL.md | 7 ++++++- plugins/codex/skills/revdiff/references/usage.md | 4 ++++ site/docs.html | 9 ++++++++- 6 files changed, 36 insertions(+), 3 deletions(-) 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..03f23a53 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. 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..83a62dd1 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.

@@ -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 From 795813491aa42958436db0229ca57fff1a232545 Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 19:21:26 +0300 Subject: [PATCH 4/8] chore(plans): archive --annotations implementation plan --- .../2026-04-27-preload-annotations.md | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 docs/plans/completed/2026-04-27-preload-annotations.md 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/` From 7f0edcef24bd7318ac8b75d9748cbbbbf8cd7f19 Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 20:26:54 +0300 Subject: [PATCH 5/8] feat(diff): export SanitizeCommitText for external callers Public wrapper around sanitizeCommitText so callers outside the diff package (e.g. preloaded annotation comments) can apply the same ANSI / C1 / control-byte stripping before content reaches a terminal renderer. --- app/diff/diff.go | 6 ++++++ 1 file changed, 6 insertions(+) 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 From 6e393bd127552dc7f9bbb5a4cd8d4fc4645db52e Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 20:27:04 +0300 Subject: [PATCH 6/8] refactor(annotation): parser type, Store.Load, tolerate unescaped ## - Move parseHeader onto a parser struct so state (out/current/body/ seenHeader) lives on the value rather than as closure-captured locals in Parse, matching the project rule of methods over standalone helpers. - Tolerate "## " body lines that don't match the header grammar by folding them into the current record. Hand-authored or LLM-generated bodies often mention "## something" without the leading-space escape; before the first header the grammar error still propagates. - Add Store.Load(io.Reader) as the symmetric inverse of Store.FormatOutput on the API surface. Callers that need to filter records (preloader) keep using Parse + Add directly. --- app/annotation/parse.go | 111 ++++++++++++++++++++--------------- app/annotation/parse_test.go | 21 +++++++ app/annotation/store.go | 17 ++++++ 3 files changed, 103 insertions(+), 46 deletions(-) diff --git a/app/annotation/parse.go b/app/annotation/parse.go index a5ba8f8f..723bfb49 100644 --- a/app/annotation/parse.go +++ b/app/annotation/parse.go @@ -24,79 +24,98 @@ var headerRe = regexp.MustCompile(`^## (.+?)(?::(\d+)(?:-(\d+))?)? \((file-level // 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 a hard -// error rather than a silent skip — the format is bidirectional and a stray -// header indicates a malformed input. +// 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) { - scanner := bufio.NewScanner(r) - scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) + p := parser{scanner: bufio.NewScanner(r)} + p.scanner.Buffer(make([]byte, 64*1024), 4*1024*1024) + return p.parse() +} - var ( - out []Annotation - current *Annotation - body []string - seenHeader bool - nonBlankSeen bool - ) +type parser struct { + scanner *bufio.Scanner - flush := func() { - if current == nil { - return - } - // The format always emits a trailing newline after the body. Strip - // exactly one trailing empty line that came from the format separator. - if n := len(body); n > 0 && body[n-1] == "" { - body = body[:n-1] - } - current.Comment = strings.Join(body, "\n") - out = append(out, *current) - current = nil - body = nil - } + out []Annotation + current *Annotation + body []string + seenHeader bool + nonBlankSeen bool +} - for scanner.Scan() { - line := scanner.Text() +func (p *parser) parse() ([]Annotation, error) { + for p.scanner.Scan() { + line := p.scanner.Text() if strings.HasPrefix(line, "## ") { - ann, err := parseHeader(line) + ann, err := p.parseHeader(line) if err != nil { - return nil, err + // 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 } - flush() - seenHeader = true - current = &ann + p.flush() + p.seenHeader = true + p.current = &ann continue } - if !seenHeader { + if !p.seenHeader { if strings.TrimSpace(line) == "" { continue } - nonBlankSeen = true + p.nonBlankSeen = true break } - // Inverse of escapeHeaderLines: strip exactly one leading space from - // body lines whose first non-space content begins with "## ". - if strings.HasPrefix(line, " ") && strings.HasPrefix(strings.TrimLeft(line, " "), "## ") { - line = line[1:] - } - body = append(body, line) + p.appendBody(line) } - if err := scanner.Err(); err != nil { + if err := p.scanner.Err(); err != nil { return nil, fmt.Errorf("scan annotations: %w", err) } - if !seenHeader && nonBlankSeen { + if !p.seenHeader && p.nonBlankSeen { return nil, errors.New("annotation input has content before any header") } - flush() - return out, nil + 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 parseHeader(line string) (Annotation, error) { +func (p *parser) parseHeader(line string) (Annotation, error) { m := headerRe.FindStringSubmatch(line) if m == nil { return Annotation{}, fmt.Errorf("malformed annotation header: %q", line) diff --git a/app/annotation/parse_test.go b/app/annotation/parse_test.go index d90eea8f..b3fe9dc6 100644 --- a/app/annotation/parse_test.go +++ b/app/annotation/parse_test.go @@ -173,6 +173,27 @@ func TestParse_RoundTrip(t *testing.T) { 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. diff --git a/app/annotation/store.go b/app/annotation/store.go index 2231347d..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. From b94f88a99f697537a19c7e351513d6976cd0ad07 Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 20:27:18 +0300 Subject: [PATCH 7/8] refactor(annotations): preloader struct, size cap, sanitize comments - Collapse preloadAnnotations / resolveKnownFiles / lookupLineSet into methods on a preloader struct so call sites stop threading 5-8 parameters through and the warn-printf calls fold into a warnf helper. Per project rule: functions called only from each other belong on a shared receiver. - Harden file read: Stat first, reject non-regular files (FIFO would block os.Open before bubbletea initializes), reject oversize inputs up front against info.Size() with a 1 MiB cap, layer io.LimitReader as belt-and-braces during parse. - Sanitize parsed Comment text via diff.SanitizeCommitText before Store.Add. Preload sources are user- or LLM-supplied and may carry stray ANSI / CR-overwrite / C1 bytes that the TUI's Renderer.AnnotationInline would wrap into the screen verbatim. - Doc-comment in resolveKnownFiles now calls out the deliberate divergence from ui.loadFiles (always-fold-untracked vs UI's toggle-gated fold) and the renamed-file orphan-drop limitation. - Tests for FIFO rejection, oversize file, and ANSI/CR sanitization. --- app/annotations_load.go | 164 +++++++++++++++++++++++++---------- app/annotations_load_test.go | 64 +++++++++++++- 2 files changed, 180 insertions(+), 48 deletions(-) diff --git a/app/annotations_load.go b/app/annotations_load.go index 937a0236..77d21b28 100644 --- a/app/annotations_load.go +++ b/app/annotations_load.go @@ -11,6 +11,34 @@ import ( "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 @@ -23,54 +51,100 @@ import ( 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 fmt.Errorf("open annotations file: %w", err) + return nil, fmt.Errorf("open annotations file: %w", err) } defer f.Close() - records, err := annotation.Parse(f) + records, err := annotation.Parse(io.LimitReader(f, maxAnnotationsFileSize+1)) if err != nil { - return fmt.Errorf("parse annotations: %w", err) + return nil, fmt.Errorf("parse annotations: %w", err) } + return records, nil +} - known, err := resolveKnownFiles(renderer, ref, staged, untrackedFn, warnOut) +func (p *preloader) load(records []annotation.Annotation) error { + known, err := p.resolveKnownFiles() if err != nil { return err } - // cache parsed FileDiff per file so we don't re-fetch on each annotation - lineCache := make(map[string]map[lineKey]struct{}) - 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 { - _, _ = fmt.Fprintf(warnOut, "warning: --annotations: file %q not in diff, dropping annotation\n", a.File) + p.warnf("warning: --annotations: file %q not in diff, dropping annotation\n", a.File) continue } if a.Line == 0 { - store.Add(a) + p.store.Add(a) continue } - lines := lookupLineSet(renderer, ref, staged, workDir, a.File, status, lineCache, warnOut) + lines := p.lookupLineSet(a.File, status) if _, ok := lines[lineKey{line: a.Line, kind: a.Type}]; !ok { - _, _ = fmt.Fprintf(warnOut, "warning: --annotations: %s:%d (%s) not in diff, dropping\n", a.File, a.Line, a.Type) + p.warnf("warning: --annotations: %s:%d (%s) not in diff, dropping\n", a.File, a.Line, a.Type) continue } - store.Add(a) + p.store.Add(a) } return nil } -// resolveKnownFiles returns the set of paths the preload should accept, -// mirroring ui.loadFiles: when running with no ref and no --staged, staged-only -// FileAdded entries are folded in if the unstaged set is empty, and untracked -// files (when untrackedFn is provided) are always folded in so the -// show-untracked UI toggle round-trips. -func resolveKnownFiles(renderer ui.Renderer, ref string, staged bool, untrackedFn func() ([]string, error), - warnOut io.Writer, -) (map[string]diff.FileStatus, error) { - files, err := renderer.ChangedFiles(ref, staged) +// 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) } @@ -78,25 +152,23 @@ func resolveKnownFiles(renderer ui.Renderer, ref string, staged bool, untrackedF for _, fe := range files { known[fe.Path] = fe.Status } - // fold in untracked files so annotations against them round-trip; matches - // ui.loadFiles which appends FileUntracked entries from loadUntracked. - if untrackedFn != nil { - if ut, utErr := untrackedFn(); utErr != nil { - _, _ = fmt.Fprintf(warnOut, "warning: --annotations: list untracked files: %v\n", utErr) + if p.untrackedFn != nil { + if ut, utErr := p.untrackedFn(); utErr != nil { + p.warnf("warning: --annotations: list untracked files: %v\n", utErr) } else { - for _, p := range ut { - if _, ok := known[p]; !ok { - known[p] = diff.FileUntracked + for _, path := range ut { + if _, ok := known[path]; !ok { + known[path] = diff.FileUntracked } } } } - if ref != "" || staged || len(files) > 0 { + if p.ref != "" || p.staged || len(files) > 0 { return known, nil } - stagedFiles, sErr := renderer.ChangedFiles("", true) + stagedFiles, sErr := p.renderer.ChangedFiles("", true) if sErr != nil { - _, _ = fmt.Fprintf(warnOut, "warning: --annotations: resolve staged files: %v\n", sErr) + p.warnf("warning: --annotations: resolve staged files: %v\n", sErr) return known, nil } for _, fe := range stagedFiles { @@ -114,10 +186,8 @@ func resolveKnownFiles(renderer ui.Renderer, ref string, staged bool, untrackedF // 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 lookupLineSet(renderer ui.Renderer, ref string, staged bool, workDir, file string, status diff.FileStatus, - cache map[string]map[lineKey]struct{}, warnOut io.Writer, -) map[lineKey]struct{} { - if lines, ok := cache[file]; ok { +func (p *preloader) lookupLineSet(file string, status diff.FileStatus) map[lineKey]struct{} { + if lines, ok := p.lineCache[file]; ok { return lines } var ( @@ -126,35 +196,34 @@ func lookupLineSet(renderer ui.Renderer, ref string, staged bool, workDir, file used string ) switch { - case status == diff.FileUntracked && workDir != "": + case status == diff.FileUntracked && p.workDir != "": used = "read" - dl, err = diff.ReadFileAsAdded(filepath.Join(workDir, file)) + dl, err = diff.ReadFileAsAdded(filepath.Join(p.workDir, file)) default: used = "diff" - fileStaged := staged - if !staged && ref == "" && status == diff.FileAdded { + fileStaged := p.staged + if !p.staged && p.ref == "" && status == diff.FileAdded { fileStaged = true } - dl, err = renderer.FileDiff(ref, file, fileStaged, 0) + dl, err = p.renderer.FileDiff(p.ref, file, fileStaged, 0) } var lines map[lineKey]struct{} if err != nil { - _, _ = fmt.Fprintf(warnOut, "warning: --annotations: %s diff for %q: %v\n", used, file, err) + p.warnf("warning: --annotations: %s diff for %q: %v\n", used, file, err) lines = map[lineKey]struct{}{} } else { lines = buildLineSet(dl) } - cache[file] = lines + p.lineCache[file] = lines return lines } -type lineKey struct { - line int - kind string +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 +// 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)) @@ -176,3 +245,4 @@ func buildLineSet(lines []diff.DiffLine) map[lineKey]struct{} { } return out } + diff --git a/app/annotations_load_test.go b/app/annotations_load_test.go index 622965bd..9768045e 100644 --- a/app/annotations_load_test.go +++ b/app/annotations_load_test.go @@ -5,6 +5,9 @@ import ( "errors" "os" "path/filepath" + "runtime" + "strings" + "syscall" "testing" "github.com/stretchr/testify/assert" @@ -68,7 +71,7 @@ func TestPreloadAnnotations_ChangedFilesError(t *testing.T) { 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 (+)\northerwise valid file but bad line\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) @@ -228,6 +231,65 @@ func TestPreloadAnnotations_UntrackedListError(t *testing.T) { 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() From 3d849f745e7a3c68c99e2abeb4a1c1231bfd0afa Mon Sep 17 00:00:00 2001 From: tushkanin Date: Mon, 27 Apr 2026 20:27:26 +0300 Subject: [PATCH 8/8] feat(stdin): reject --stdin --annotations combination Line-scoped annotations preloaded against a synthetic stdin buffer have no real diff to validate against and would orphan-drop on every record. File-level records keyed on the synthetic stdin name happen to work, but documenting half-support is worse than excluding the combination outright. Joins the existing mutually-exclusive list with refs / --staged / --only / --all-files / --include / --exclude. --- README.md | 2 +- app/stdin.go | 3 +++ site/docs.html | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 03f23a53..5d2f7875 100644 --- a/README.md +++ b/README.md @@ -533,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/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/site/docs.html b/site/docs.html index 83a62dd1..53bd418c 100644 --- a/site/docs.html +++ b/site/docs.html @@ -228,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