diff --git a/README.md b/README.md index f5bdb54..be7d65a 100644 --- a/README.md +++ b/README.md @@ -171,9 +171,9 @@ git-review feedback git-review feedback --json ``` -This read-only command returns notes persisted by git-review, including reviews completed outside an agent session. +This read-only command returns commit-review notes persisted by git-review and file-review notes from tuicr, including existing saved sessions. It does not launch tuicr, recover an attempt, or change either review store. -Interrupted sessions whose notes were not captured by git-review still require explicit interactive recovery. +Interrupted commit-review sessions whose notes were not captured by git-review still require explicit interactive recovery. Feedback results use the versioned result envelope, with `"command":"feedback"`. The optional `feedback` array contains records with these fields: @@ -189,9 +189,16 @@ Superseded attempts, changed or removed stacks, and addressed attempts remain hi The command does not infer that historical feedback needs another fix or that a changed stack resolved it. Malformed stored review data is an error rather than silently omitted feedback. -The result is `pending` when current saved feedback exists and `idle` otherwise. +The optional `fileFeedback` array contains file-review results with `"command":"files"`, the saved session path, comments, and session notes. +It includes local sessions in the feature checkout and its annotation directories, not commit-review sessions in the detached checkout. +These records are repository-scoped and remain visible after branch changes while their tuicr sessions contain notes. +They do not imply current stack feedback or authorize fixup commits. +Address them as new commits, logically organized and sized. +Resolve or remove obsolete notes in tuicr when they no longer apply. + +The result is `pending` when current commit feedback or any saved file feedback exists and `idle` otherwise. An `idle` result can still include historical records. -The `feedback` field is omitted when no saved notes exist, and `entries` is empty. +Each feedback array is omitted when it has no records, and `entries` is empty. Active attempts remain visible in `active`, but collecting feedback does not resume them. ### Review a stack @@ -305,7 +312,8 @@ Use `--session` to select an exact saved session when several file reviews match File reviews run in the feature checkout, not the detached review worktree. `--all`, explicit paths, and `--untracked` do not require a configured upstream. They hold the same repository-wide worktree lock as `commits` reviews, so `status --is-reviewing` reports true during either review. -They do not create git-review attempts and do not affect `status` counts or `feedback` records. +They do not create git-review attempts or affect `status` counts. +Retrieve their comments and session notes with `git-review feedback`, in the separate `fileFeedback` JSON array or the terminal “File feedback” section. `approve` applies only to commit stacks, not file reviews. Successful `files` output uses `"command":"files"` to distinguish it from `"command":"commits"`. @@ -368,7 +376,8 @@ Successful results other than `status` contain: | `entries` | Unresolved stacks, or the stacks selected by approval/review | | `active` | Persisted active attempts | | `decision` | Required explicit choice, when present | -| `feedback` | Current and historical saved feedback records for the feedback command, when notes exist | +| `feedback` | Current and historical commit-review feedback records, when notes exist | +| `fileFeedback` | Saved file-review results from tuicr, when notes exist | Needs-review outcomes are `pending` or `idle`. Review outcomes include `approved`, `feedback`, `incomplete`, `decision_required`, `cancelled`, `idle`, `range_complete`, and `no_waiting`. diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 0c60bcb..e55aee9 100644 --- a/internal/cli/cli_test.go +++ b/internal/cli/cli_test.go @@ -28,7 +28,8 @@ func git(t *testing.T, dir string, args ...string) string { func featureRepo(t *testing.T) string { t.Helper() - dir := t.TempDir() + dir, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) git(t, dir, "init", "-b", "main") git(t, dir, "config", "user.name", "Test") git(t, dir, "config", "user.email", "test@example.com") diff --git a/internal/cli/files_functional_test.go b/internal/cli/files_functional_test.go index 988997b..316fa7c 100644 --- a/internal/cli/files_functional_test.go +++ b/internal/cli/files_functional_test.go @@ -15,12 +15,14 @@ func TestRealTuicrFileScopes(t *testing.T) { env := realTuicrEnvironment(t) binary := buildGitReview(t) for _, tc := range []struct { - name string - args []string - cwd string - previous bool + name string + args []string + cwd string + previous bool + noUpstream bool }{ {name: "nested file", args: []string{"src/nested.txt"}}, + {name: "annotation without upstream", args: []string{"src/nested.txt"}, noUpstream: true}, {name: "nested file after root review", args: []string{"src/nested.txt"}, previous: true}, {name: "nested directory", args: []string{"src"}, previous: true}, {name: "untracked from subdirectory", args: []string{"--untracked"}, cwd: "src", previous: true}, @@ -37,11 +39,15 @@ func TestRealTuicrFileScopes(t *testing.T) { if tc.previous { reviewRealFile(t, binary, dir, env, "feature.txt", "Prior root feedback", "feature.txt") } + if tc.noUpstream { + git(t, dir, "branch", "--unset-upstream") + } result := reviewRealFile(t, binary, filepath.Join(dir, tc.cwd), env, "nested.txt", "Nested feedback", tc.args...) require.Equal(t, dir, result.Repo) require.Len(t, result.Review.Comments, 1) require.Equal(t, "Nested feedback", result.Review.Comments[0]["content"]) require.Equal(t, "src/nested.txt", result.Review.Comments[0]["path"]) + checkRealFileFeedback(t, binary, filepath.Join(dir, tc.cwd), env, result) }) } } @@ -88,10 +94,42 @@ func TestRealTuicrBranchFiles(t *testing.T) { require.Len(t, result.Review.Comments, 1) require.Equal(t, "Branch feedback", result.Review.Comments[0]["content"]) require.Equal(t, "feature.txt", result.Review.Comments[0]["path"]) + git(t, dir, "commit", "--allow-empty", "-m", "Advance branch after file review") + checkRealFileFeedback(t, binary, filepath.Join(dir, tc.cwd), env, result) }) } } +func checkRealFileFeedback(t *testing.T, binary, dir string, env []string, reviewed review.FilesResult) { + t.Helper() + before, err := os.ReadFile(reviewed.Session) + require.NoError(t, err) + out := runRealCommand(t, binary, dir, env, "feedback", "--json") + var result struct { + Status string `json:"status"` + Feedback []review.FeedbackRecord `json:"feedback"` + FileFeedback []review.FilesResult `json:"fileFeedback"` + } + require.NoError(t, json.Unmarshal([]byte(out), &result)) + require.Equal(t, "pending", result.Status) + require.Empty(t, result.Feedback, "file feedback must not become a stack attempt") + var found bool + for _, file := range result.FileFeedback { + if file.Session == reviewed.Session { + found = true + require.Equal(t, "files", file.Command) + require.Equal(t, reviewed.Review, file.Review) + } + } + require.True(t, found, "feedback must include the saved file review") + terminal := runRealCommand(t, binary, dir, env, "feedback") + require.Contains(t, terminal, "File feedback:") + require.Contains(t, terminal, reviewed.Review.Comments[0]["content"]) + after, err := os.ReadFile(reviewed.Session) + require.NoError(t, err) + require.Equal(t, before, after, "feedback must not change the tuicr session") +} + func reviewRealFile(t *testing.T, binary, dir string, env []string, file, feedback string, args ...string) review.FilesResult { t.Helper() command := append([]string{"files", "--json"}, args...) diff --git a/internal/cli/tuicr_functional_test.go b/internal/cli/tuicr_functional_test.go index 83dba4f..531be27 100644 --- a/internal/cli/tuicr_functional_test.go +++ b/internal/cli/tuicr_functional_test.go @@ -21,7 +21,8 @@ func realTuicrEnvironment(t *testing.T) []string { } _, err := exec.LookPath("tuicr") require.NoError(t, err, "functional tests require tuicr on PATH") - home := t.TempDir() + home, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) overrides := map[string]string{ "HOME": home, "XDG_DATA_HOME": filepath.Join(home, "data"), "XDG_CACHE_HOME": filepath.Join(home, "cache"), "XDG_CONFIG_HOME": filepath.Join(home, "config"), "TERM": "xterm-256color", "COLORTERM": "truecolor", "GIT_CONFIG_GLOBAL": os.DevNull, "GIT_CONFIG_NOSYSTEM": "1", diff --git a/internal/display/render.go b/internal/display/render.go index 948b6e6..c0c4b1c 100644 --- a/internal/display/render.go +++ b/internal/display/render.go @@ -181,6 +181,14 @@ func renderFeedbackResult(result review.Result) (string, error) { if err := renderFeedback(&out, result.Feedback); err != nil { return "", err } + for _, file := range result.FileFeedback { + fmt.Fprintln(&out, "File feedback:") + text, err := renderFilesResult(file) + if err != nil { + return "", err + } + fmt.Fprintln(&out, text) + } if result.Status == "idle" { fmt.Fprintln(&out, "No current saved feedback.") } else { diff --git a/internal/review/file_feedback.go b/internal/review/file_feedback.go new file mode 100644 index 0000000..b5e9724 --- /dev/null +++ b/internal/review/file_feedback.go @@ -0,0 +1,122 @@ +package review + +import ( + "fmt" + "path/filepath" + "strings" +) + +func readFileFeedbackEntry(store, repo string, entry object) (*FilesResult, error) { + canonical, relativeScope, err := localFileFeedbackScope(repo, entry) + if err != nil { + return nil, err + } + if canonical == "" { + return nil, nil + } + session, path, err := readFileFeedbackSession(store, entry) + if err != nil { + return nil, err + } + source, err := fileFeedbackSource(session, relativeScope) + if err != nil { + return nil, err + } + if source == "" { + return nil, nil + } + if err := validateSessionRepo(session, filepath.Clean(canonical)); err != nil { + return nil, err + } + saved, err := decodeFileFeedbackSession(session, canonical, source) + if err != nil { + return nil, err + } + if saved.Status() != "feedback" { + return nil, nil + } + result := FilesResult{Version: 1, Command: "files", Repo: repo, Session: path, AllFiles: source == "pristine"} + if relativeScope != "." { + result.Path = filepath.ToSlash(relativeScope) + } + result, err = fileReviewResult(result, saved) + return &result, err +} + +func localFileFeedbackScope(repo string, entry object) (string, string, error) { + kind, err := decodeRequired[object](entry, "kind") + if err != nil { + return "", "", err + } + entryType, err := decodeRequired[string](kind, "type") + if err != nil { + return "", "", err + } + if entryType != "local" { + return "", "", nil + } + canonical, err := decodeRequired[string](entry, "canonical_repo_path") + if err != nil { + return "", "", err + } + relativeScope, err := filepath.Rel(repo, canonical) + if err != nil { + return "", "", err + } + if relativeScope == ".." || strings.HasPrefix(relativeScope, ".."+string(filepath.Separator)) { + return "", "", nil + } + return canonical, relativeScope, nil +} + +func readFileFeedbackSession(store string, entry object) (object, string, error) { + relative, err := decodeRequired[string](entry, "path") + if err != nil { + return nil, "", err + } + if !sessionPathPattern.MatchString(relative) { + return nil, "", fmt.Errorf("unsafe session path %q", relative) + } + path, err := safeSessionPath(store, relative) + if err != nil { + return nil, "", err + } + session, err := readObject(path) + return session, path, err +} + +func fileFeedbackSource(session object, relativeScope string) (string, error) { + if raw, ok := session["pr_session_key"]; ok { + if string(raw) != "null" { + return "", nil + } + } + source, err := decodeRequired[string](session, "diff_source") + if err != nil { + return "", err + } + switch source { + case "pristine", "working_tree": + return source, nil + case "commit_range": + // Branch diffs launched by files always use the feature repository root. + if relativeScope == "." { + return source, nil + } + } + return "", nil +} + +func decodeFileFeedbackSession(session object, canonical, source string) (*SavedReview, error) { + if source != "commit_range" { + return decodeFileSession(session, canonical) + } + commits, err := decodeRequired[[]string](session, "commit_range") + if err != nil { + return nil, err + } + if err := validateCommits(commits); err != nil { + return nil, err + } + return decodeSession(session, Stack{Repo: canonical, Commits: commits}) +} diff --git a/internal/review/file_feedback_test.go b/internal/review/file_feedback_test.go new file mode 100644 index 0000000..4229b47 --- /dev/null +++ b/internal/review/file_feedback_test.go @@ -0,0 +1,55 @@ +package review + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestFileFeedbackDiscoversNotesWithoutChangingSessions(t *testing.T) { + r := testRepo(t) + store := t.TempDir() + path := filepath.Join(store, "sessions/0123456789abcdef.json") + session := fileFixture(t, r.Dir, "working_tree", "file", store, false, "sessions/0123456789abcdef.json") + session["session_notes"] = "Check the public API" + writeJSON(t, path, session) + before, err := os.ReadFile(path) + require.NoError(t, err) + reader := SavedReader{Stores: []string{store, store}} + feedback, err := reader.FileFeedback(r.Dir) + require.NoError(t, err) + require.Len(t, feedback, 1, "duplicate index discovery must not repeat feedback") + require.Equal(t, "feedback", feedback[0].Status) + require.Empty(t, feedback[0].Review.Comments) + require.Equal(t, "Check the public API", *feedback[0].Review.SessionNotes) + after, err := os.ReadFile(path) + require.NoError(t, err) + require.Equal(t, before, after) + + // A different repository must not inherit this annotation, even if its + // directory name starts with the same prefix. + other := r.Dir + "-other" + require.NoError(t, os.Mkdir(other, 0o700)) + t.Cleanup(func() { _ = os.Remove(other) }) + otherStore := t.TempDir() + otherSession := fileFixture(t, other, "working_tree", "file", otherStore, false, "sessions/0123456789abcdef.json") + otherSession["session_notes"] = "Unrelated feedback" + writeJSON(t, filepath.Join(otherStore, "sessions/0123456789abcdef.json"), otherSession) + feedback, err = (SavedReader{Stores: []string{otherStore}}).FileFeedback(r.Dir) + require.NoError(t, err) + require.Empty(t, feedback) + + delete(session, "session_notes") + writeJSON(t, path, session) + feedback, err = reader.FileFeedback(r.Dir) + require.NoError(t, err) + require.Empty(t, feedback, "incomplete reviews without notes are not feedback") + + // Corrupt notes must be reported, not mistaken for absent feedback. + session["session_notes"] = 42 + writeJSON(t, path, session) + _, err = reader.FileFeedback(r.Dir) + require.ErrorContains(t, err, "session_notes") +} diff --git a/internal/review/files.go b/internal/review/files.go index 5227a50..7377674 100644 --- a/internal/review/files.go +++ b/internal/review/files.go @@ -75,11 +75,8 @@ type FileReviewer struct { // Review launches tuicr for working-tree files and reads its saved session. func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesResult, error) { var result FilesResult - if options.All && (options.Untracked || options.Path != "") { - return result, fmt.Errorf("--all cannot be combined with --untracked or an explicit path") - } - if conflictingFileScope(options) { - return result, fmt.Errorf("--untracked cannot be combined with an explicit path") + if err := validateFileScope(options); err != nil { + return result, err } repo, err := r.Service.Repository.git(ctx, "rev-parse", "--show-toplevel") if err != nil { @@ -95,7 +92,7 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe _ = lock.Close() }() - if !options.All && !options.Untracked && options.Path == "" { + if reviewBranchFiles(options) { return r.reviewBranchDiff(ctx, repo, options.Session, lock) } allFiles := options.All @@ -133,50 +130,6 @@ func (r FileReviewer) Review(ctx context.Context, options FilesOptions) (FilesRe return fileReviewResult(result, saved) } -// reviewBranchDiff uses Git's upstream merge-base without imposing the linear -// history and stack-placement rules required by commit reviews. -func (r FileReviewer) reviewBranchDiff(ctx context.Context, repo, selector string, lock *os.File) (FilesResult, error) { - result := FilesResult{Version: 1, Command: "files", Repo: repo} - repository := r.Service.Repository - view, _, err := repository.branchView(ctx) - if err != nil { - return result, err - } - head, err := repository.Resolve(ctx, "HEAD") - if err != nil { - return result, err - } - base, err := repository.git(ctx, "merge-base", "--all", view.Target, head) - if err != nil { - return result, err - } - if strings.Contains(base, "\n") { - return result, fmt.Errorf("review requires one merge-base with upstream") - } - history, err := repository.git(ctx, "rev-list", "--reverse", base+".."+head) - if err != nil { - return result, err - } - if history == "" { - return fileReviewResult(result, nil) - } - // Run at the root so invocation from a subdirectory keeps the same scope - // and saved-session identity. - r.Service.Repository.Dir = repo - if err := r.launch(ctx, []string{"tui", "--no-update-check", "--stdout", "--revisions", base + ".." + head}, lock); err != nil { - return result, err - } - scope := Stack{Repo: repo, Base: base, Tip: head, Commits: strings.Split(history, "\n")} - saved, err := r.Reader.Read(scope, selector) - if err != nil { - return result, err - } - if saved != nil { - result.Session = saved.Path - } - return fileReviewResult(result, saved) -} - func (r FileReviewer) launch(ctx context.Context, args []string, lock *os.File) error { cmd := exec.CommandContext(ctx, "tuicr", args...) cmd.Dir = r.Service.Repository.Dir @@ -225,6 +178,50 @@ func (r FileReviewer) readReview(path, diffSource, selector string) (*SavedRevie return r.Reader.ReadFile(root, diffSource, selector) } +// reviewBranchDiff uses Git's upstream merge-base without imposing the linear +// history and stack-placement rules required by commit reviews. +func (r FileReviewer) reviewBranchDiff(ctx context.Context, repo, selector string, lock *os.File) (FilesResult, error) { + result := FilesResult{Version: 1, Command: "files", Repo: repo} + repository := r.Service.Repository + view, _, err := repository.branchView(ctx) + if err != nil { + return result, err + } + head, err := repository.Resolve(ctx, "HEAD") + if err != nil { + return result, err + } + base, err := repository.git(ctx, "merge-base", "--all", view.Target, head) + if err != nil { + return result, err + } + if strings.Contains(base, "\n") { + return result, fmt.Errorf("review requires one merge-base with upstream") + } + history, err := repository.git(ctx, "rev-list", "--reverse", base+".."+head) + if err != nil { + return result, err + } + if history == "" { + return fileReviewResult(result, nil) + } + // Run at the root so invocation from a subdirectory keeps the same scope + // and saved-session identity. + r.Service.Repository.Dir = repo + if err := r.launch(ctx, []string{"tui", "--no-update-check", "--stdout", "--revisions", base + ".." + head}, lock); err != nil { + return result, err + } + scope := Stack{Repo: repo, Base: base, Tip: head, Commits: strings.Split(history, "\n")} + saved, err := r.Reader.Read(scope, selector) + if err != nil { + return result, err + } + if saved != nil { + result.Session = saved.Path + } + return fileReviewResult(result, saved) +} + // Tuicr keys --file sessions by the canonical parent of a file or by the // selected directory. --all-files uses the canonical current directory. func fileSessionRoot(dir, path string) (string, error) { @@ -289,6 +286,36 @@ func makeFileCommentsRepoRelative(saved *SavedReview, repo string) error { return nil } +func validateFileScope(options FilesOptions) error { + if conflictingAllFileScope(options) { + return fmt.Errorf("--all cannot be combined with --untracked or an explicit path") + } + if conflictingFileScope(options) { + return fmt.Errorf("--untracked cannot be combined with an explicit path") + } + return nil +} + +func conflictingAllFileScope(options FilesOptions) bool { + if !options.All { + return false + } + if options.Untracked { + return true + } + return options.Path != "" +} + +func reviewBranchFiles(options FilesOptions) bool { + if options.All { + return false + } + if options.Untracked { + return false + } + return options.Path == "" +} + func conflictingFileScope(options FilesOptions) bool { if !options.Untracked { return false diff --git a/internal/review/git.go b/internal/review/git.go index 5f01218..fc95428 100644 --- a/internal/review/git.go +++ b/internal/review/git.go @@ -150,26 +150,15 @@ func (r Repository) Snapshot(ctx context.Context) (Snapshot, error) { } func (r Repository) branchView(ctx context.Context) (Snapshot, string, error) { - var view Snapshot - repo, err := r.git(ctx, "rev-parse", "--show-toplevel") - if err != nil { - return view, "", err - } - branch, err := r.git(ctx, "symbolic-ref", "--quiet", "--short", "HEAD") - if err != nil { - return view, "", fmt.Errorf("review requires a branch, not detached HEAD: %w", err) - } - common, err := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-common-dir") + view, repo, err := r.checkoutView(ctx) if err != nil { return view, "", err } target, err := r.git(ctx, "rev-parse", "--symbolic-full-name", "@{upstream}") if err != nil { - return view, "", fmt.Errorf("branch %s requires a configured Git upstream: %w", branch, err) + return view, "", fmt.Errorf("branch %s requires a configured Git upstream: %w", view.Branch, err) } - view.Branch = branch view.Target = target - view.Path = filepath.Join(common, "tuicr-reviews", fmt.Sprintf("%x.json", sha256.Sum256([]byte(branch)))) return view, repo, nil } @@ -191,6 +180,25 @@ func (r Repository) changeID(ctx context.Context, sha string, ids map[string]boo return id, nil } +func (r Repository) checkoutView(ctx context.Context) (Snapshot, string, error) { + var view Snapshot + repo, err := r.git(ctx, "rev-parse", "--show-toplevel") + if err != nil { + return view, "", err + } + branch, err := r.git(ctx, "symbolic-ref", "--quiet", "--short", "HEAD") + if err != nil { + return view, "", fmt.Errorf("review requires a branch, not detached HEAD: %w", err) + } + common, err := r.git(ctx, "rev-parse", "--path-format=absolute", "--git-common-dir") + if err != nil { + return view, "", err + } + view.Branch = branch + view.Path = filepath.Join(common, "tuicr-reviews", fmt.Sprintf("%x.json", sha256.Sum256([]byte(branch)))) + return view, repo, nil +} + func (r Repository) git(ctx context.Context, args ...string) (string, error) { out, err := r.gitOutput(ctx, args...) return strings.TrimSpace(out), err diff --git a/internal/review/git_test.go b/internal/review/git_test.go index c9db9b4..5019279 100644 --- a/internal/review/git_test.go +++ b/internal/review/git_test.go @@ -3,6 +3,7 @@ package review import ( "fmt" "os/exec" + "path/filepath" "strings" "testing" @@ -20,7 +21,8 @@ func gitTest(t *testing.T, dir string, args ...string) string { func testRepo(t *testing.T) Repository { t.Helper() - dir := t.TempDir() + dir, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) gitTest(t, dir, "init", "-b", "main") gitTest(t, dir, "config", "user.name", "Test") gitTest(t, dir, "config", "user.email", "test@example.com") diff --git a/internal/review/saved.go b/internal/review/saved.go index 5b2b77d..31dc0c4 100644 --- a/internal/review/saved.go +++ b/internal/review/saved.go @@ -1,6 +1,7 @@ package review import ( + "cmp" "encoding/json/jsontext" "encoding/json/v2" "errors" @@ -50,6 +51,42 @@ func (s *SavedReview) Status() string { // SavedReader reads indexed sessions without modifying the tuicr store. type SavedReader struct{ Stores []string } +// FileFeedback reads local sessions in the feature checkout and its annotation +// directories. Commit reviews use a separate detached checkout, not this scope. +// Sessions remain available after branch changes and are not stack approvals. +func (r SavedReader) FileFeedback(repo string) ([]FilesResult, error) { + repo, err := filepath.EvalSymlinks(repo) + if err != nil { + return nil, err + } + var results []FilesResult + seen := map[string]bool{} + for _, store := range r.Stores { + entries, err := readIndex(store) + if err != nil { + return nil, err + } + for _, rows := range entries { + for _, entry := range rows { + result, err := readFileFeedbackEntry(store, repo, entry) + if err != nil { + return nil, err + } + if result == nil { + continue + } + if seen[result.Session] { + continue + } + seen[result.Session] = true + results = append(results, *result) + } + } + } + slices.SortFunc(results, func(a, b FilesResult) int { return cmp.Compare(a.Session, b.Session) }) + return results, nil +} + // Read returns nil only when no matching session exists. Corruption is an error. func (r SavedReader) Read(scope Stack, selector string) (*SavedReview, error) { if err := validateCommits(scope.Commits); err != nil { diff --git a/internal/review/service.go b/internal/review/service.go index b532b0c..bc5c4f1 100644 --- a/internal/review/service.go +++ b/internal/review/service.go @@ -16,15 +16,16 @@ type Entry struct { // Result is the shared input to JSON encoding and terminal rendering. type Result struct { - Version int `json:"version"` - Command string `json:"command"` - Status string `json:"status"` - View Snapshot `json:"view"` - Entries []Entry `json:"entries"` - Active []Attempt `json:"active"` - Decision *Decision `json:"decision,omitzero"` - Feedback []FeedbackRecord `json:"feedback,omitempty"` - Summary []StateCount `json:"summary,omitempty"` + Version int `json:"version"` + Command string `json:"command"` + Status string `json:"status"` + View Snapshot `json:"view"` + Entries []Entry `json:"entries"` + Active []Attempt `json:"active"` + Decision *Decision `json:"decision,omitzero"` + Feedback []FeedbackRecord `json:"feedback,omitempty"` + FileFeedback []FilesResult `json:"fileFeedback,omitempty"` + Summary []StateCount `json:"summary,omitempty"` } // FeedbackRecord retains captured scope and marks obsolete or addressed notes. @@ -69,6 +70,27 @@ func countStates(view Snapshot, state State) []StateCount { return counts } +func appendCommitFeedback(result *Result, state State) error { + for _, attempt := range state.Attempts { + if len(attempt.Review) == 0 { + continue + } + var saved SavedReview + if err := json.Unmarshal(attempt.Review, &saved); err != nil { + return fmt.Errorf("invalid stored review %s: %w", attempt.ID, err) + } + if saved.Status() != "feedback" { + continue + } + historical := !currentAttempt(result.View, state, attempt) + result.Feedback = append(result.Feedback, FeedbackRecord{Attempt: attempt, Historical: historical}) + if !historical { + result.Status = "pending" + } + } + return nil +} + // Service coordinates the current branch with its durable review history. type Service struct{ Repository Repository } @@ -113,9 +135,9 @@ func (s Service) Approve(ctx context.Context, first, last string) (Result, error return result, nil } -// Feedback reads every persisted attempt with notes without changing either store. +// Feedback reads saved commit and file feedback without changing either store. func (s Service) Feedback(ctx context.Context) (Result, error) { - view, err := s.Repository.Snapshot(ctx) + view, repo, err := s.Repository.checkoutView(ctx) if err != nil { return Result{}, err } @@ -123,23 +145,24 @@ func (s Service) Feedback(ctx context.Context) (Result, error) { if err != nil { return Result{}, err } - result := Result{Version: 1, Command: "feedback", Status: "idle", View: view, Entries: []Entry{}, Active: activeAttempts(state)} - for _, attempt := range state.Attempts { - if len(attempt.Review) == 0 { - continue - } - var saved SavedReview - if err := json.Unmarshal(attempt.Review, &saved); err != nil { - return Result{}, fmt.Errorf("invalid stored review %s: %w", attempt.ID, err) - } - if saved.Status() != "feedback" { - continue - } - historical := !currentAttempt(view, state, attempt) - result.Feedback = append(result.Feedback, FeedbackRecord{Attempt: attempt, Historical: historical}) - if !historical { - result.Status = "pending" - } + view, err = s.feedbackView(ctx, view, state) + if err != nil { + return Result{}, err + } + reader, err := DefaultSavedReader() + if err != nil { + return Result{}, err + } + files, err := reader.FileFeedback(repo) + if err != nil { + return Result{}, err + } + result := Result{Version: 1, Command: "feedback", Status: "idle", View: view, Entries: []Entry{}, Active: activeAttempts(state), FileFeedback: files} + if len(files) > 0 { + result.Status = "pending" + } + if err := appendCommitFeedback(&result, state); err != nil { + return Result{}, err } return result, nil } @@ -176,6 +199,25 @@ func (s Service) Status(ctx context.Context, needsReview bool) (Result, error) { return result, nil } +// File annotation does not require an upstream or linear commit history. +// Preserve stack validation when there are commit-review attempts to classify. +func (s Service) feedbackView(ctx context.Context, view Snapshot, state State) (Snapshot, error) { + if len(state.Attempts) > 0 { + return s.Repository.Snapshot(ctx) + } + head, err := s.Repository.Resolve(ctx, "HEAD") + if err != nil { + return Snapshot{}, err + } + target, err := s.Repository.git(ctx, "for-each-ref", "--format=%(upstream)", "refs/heads/"+view.Branch) + if err != nil { + return Snapshot{}, err + } + view.Head = head + view.Target = target + return view, nil +} + func (s Service) selectRange(ctx context.Context, view Snapshot, first, last string) ([]Stack, error) { startSHA, err := s.Repository.Resolve(ctx, first) if err != nil { diff --git a/skills/git-review/SKILL.md b/skills/git-review/SKILL.md index cdc9ede..dad2ea7 100644 --- a/skills/git-review/SKILL.md +++ b/skills/git-review/SKILL.md @@ -75,11 +75,19 @@ Inspect the JSON result, not only the exit code. Stop on command errors or an unsupported output version. The supported output version is `1`. -Read the `feedback` array rather than `entries`. -The array is absent when no saved notes exist. +Read both `feedback` and `fileFeedback`, not `entries`. +Each array is absent when it has no records. +The `feedback` array contains commit-review feedback. Each record contains an `attempt` and a `historical` boolean. -For each selected attempt, read: +The `fileFeedback` array contains saved file reviews with `"command":"files"`. +Read each result’s `review.comments` and `review.sessionNotes`. +These repository-scoped sessions can predate the current branch state. +Check whether their notes still apply before changing code. +Address file feedback as new commits, not `fixup!`, `amend!`, or `reword!` commits. +Do not treat file feedback as a commit-review attempt or stack approval. + +For each selected commit-review attempt, read: - `id`: identifies the feedback attempt. - `stack`: contains the captured commits, tip, subject, and optional Change-Id.