From af9d6a122d8bf8d2239a8a3b29d37d7c73ed3a78 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 19 Oct 2026 19:35:06 +0200 Subject: [PATCH 1/2] feat(trace): keep line attribution across renames A file that a commit renames keeps the AI/human split of its lines. Commit lines detect renames, the push blames each session file under its path at HEAD, and the replacement scan follows a renamed file, so a human change to it sends the session again (spec issue-3639 R-007). Amend, rebase, squash, and cherry-pick get lifecycle tests too. Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: 81312179-16f4-4c92-a45b-a104a593a498 --- app/cli/internal/trace/README.md | 2 + app/cli/internal/trace/attribution/enrich.go | 124 ++++++++++-- .../internal/trace/attribution/enrich_test.go | 54 +++++ app/cli/internal/trace/git/git.go | 11 +- app/cli/internal/trace/git/lines.go | 79 +++++++- app/cli/internal/trace/git/lines_test.go | 61 ++++++ app/cli/pkg/action/trace_hook_handler.go | 35 +++- app/cli/pkg/action/trace_hook_handler_test.go | 3 + .../pkg/action/trace_line_attribution_test.go | 1 + .../pkg/action/trace_line_rewrites_test.go | 188 ++++++++++++++++++ app/cli/pkg/action/trace_replacements.go | 73 ++++++- 11 files changed, 589 insertions(+), 42 deletions(-) create mode 100644 app/cli/pkg/action/trace_line_rewrites_test.go diff --git a/app/cli/internal/trace/README.md b/app/cli/internal/trace/README.md index cd9faccfd..eb30e5cdb 100644 --- a/app/cli/internal/trace/README.md +++ b/app/cli/internal/trace/README.md @@ -56,12 +56,14 @@ Which lines of a commit the AI wrote. | Shell changes attribution | Changes that the agent makes with shell commands (for example `sed`) are attributed to the AI too, not only changes made with the file tools. | Partial | Partial | Partial | Partial | | Net contribution | The added lines of a session are the lines that its commits added and that are still in the commit that the push sends. A line that a later commit replaced does not count, and each line counts one time. The removed lines are the sum of the removals of the session's commits (spec issue-3639). | Partial | Partial | Partial | Partial | | Session updates | A later commit that replaces lines of a session sends the evidence of the session again at the next push, with the new numbers. A commit in no session that replaces lines of a session joins it: the commit list of the session holds the commit, and its replacing lines count as human (spec issue-3639). | Partial | Partial | Partial | Partial | +| Renames and rewrites | The AI/human split survives a file rename, an amend, a rebase, a squash, and a cherry-pick. The lines match the trace state by content, and blame follows a renamed file to its lines from before the rename (spec issue-3639). | Partial | Partial | Partial | Partial | Why not Yes: - **Line attribution, partial commits and generated line counts, Cursor:** Cursor has only the `afterFileEdit` hook, which gives the edits but no snapshot before the edit. The provider rebuilds the content before the edit from the edits. - **Net contribution, all agents:** the push finds the last writer of each line with `git blame`. A later commit that only re-indents a line takes the line, because blame sees a change. For Cursor, the limits of line attribution apply too. - **Session updates, all agents:** the update works only in the clone where the work happened, while the CLI keeps the trace state of the session (30 days), and only when the commits of the session are on the branch that the push sends. +- **Renames and rewrites, all agents:** this works only in the clone where the work happened. Git finds a rename only when the file keeps at least half of its content. Two pushed commits can rename a file one after the other. A later change to the file under its middle name does not send the session again. For Cursor, the limits of line attribution apply too. - **Shell changes attribution, all agents:** the hook before a shell command records only a hash of each file, not its content. The file at the `HEAD` commit from before the command stands in for the content before the command. Uncommitted lines that an earlier agent edit recorded are not recorded again, but uncommitted human lines of a file that the command changes count as AI. - **Shell changes attribution, Cursor:** the provider installs the `preToolUse` and `postToolUse` hooks for the `Shell` tool. These hooks are verified for the Cursor IDE only. It is not verified that the Cursor CLI sends them. diff --git a/app/cli/internal/trace/attribution/enrich.go b/app/cli/internal/trace/attribution/enrich.go index 2628d7408..709f778b8 100644 --- a/app/cli/internal/trace/attribution/enrich.go +++ b/app/cli/internal/trace/attribution/enrich.go @@ -16,6 +16,9 @@ package attribution import ( + "maps" + "slices" + "github.com/chainloop-dev/chainloop/app/cli/internal/trace" tracegit "github.com/chainloop-dev/chainloop/app/cli/internal/trace/git" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/state" @@ -57,6 +60,12 @@ type CommitLines struct { // back to the sum of the added lines of each commit. Removed lines are always // the sum over the session's own commits. // +// A file that was renamed after a commit of the session has the lines of its +// new path in tip (R-007). A line there counts for the path that its commit +// changed, so a session that changed the file under both paths counts each +// line once. A commit that renamed a file matches the ledger of the new path, +// then of the old one. +// // A file with an edit recorded before line fingerprints existed keeps the // earlier rule: AI lines are the recorded ranges, capped at the file's diff. func Enrich(sessionID string, ledgers map[string]*state.AILineAttribution, commits []CommitLines, tip map[string][]tracegit.BlameLine, changes *aicodingsession.CodeChanges) { @@ -75,15 +84,25 @@ func Enrich(sessionID string, ledgers map[string]*state.AILineAttribution, commi s.aiAdded, s.humanAdded = net.aiAdded, net.humanAdded } + oldPaths := make(map[string][]string) + for _, c := range commits { + for _, fl := range c.Files { + if fl.OldPath != "" && !slices.Contains(oldPaths[fl.Path], fl.OldPath) { + oldPaths[fl.Path] = append(oldPaths[fl.Path], fl.OldPath) + } + } + } + changes.AILinesAdded, changes.AILinesRemoved = 0, 0 changes.HumanLinesAdded, changes.HumanLinesRemoved = 0, 0 for i := range changes.Files { f := &changes.Files[i] // The marker keeps its earlier meaning, a file that the session - // edited, for platforms that do not read the split (R-009). An empty - // ranges slice is valid: the agent only deleted lines. - if aiRanges, ok := self.Files[f.Path]; ok { + // edited, also under the path that a commit renamed, for platforms + // that do not read the split (R-009). An empty ranges slice is valid: + // the agent only deleted lines. + if aiRanges, ok := editedRanges(self, f.Path, oldPaths[f.Path]); ok { f.Attribution = trace.AttributionAI f.LineRanges = aiRanges f.SessionIDs = []string{sessionID} @@ -117,6 +136,18 @@ type fileSplit struct { aiAdded, aiRemoved, humanAdded, humanRemoved int } +// editedRanges returns the recorded ranges of the session's edits of a file, +// under its path or, when a commit renamed it, under an old path. +func editedRanges(l *state.AILineAttribution, path string, oldPaths []string) ([]aicodingsession.LineRange, bool) { + for _, p := range append([]string{path}, oldPaths...) { + if ranges, ok := l.Files[p]; ok { + return ranges, true + } + } + + return nil, false +} + // splitByPath matches the lines of each commit against the ledgers, and // returns the split of each file that the commits changed. func splitByPath(sessionID string, self *state.AILineAttribution, ledgers map[string]*state.AILineAttribution, commits []CommitLines) map[string]*fileSplit { @@ -142,9 +173,9 @@ func splitByPath(sessionID string, self *state.AILineAttribution, ledgers map[st for _, line := range fl.Added { fp := Fingerprint(line) switch { - case written.take(self, fl.Path, fp): + case written.takeFile(self, fl, fp): s.aiAdded++ - case written.takeAny(others, fl.Path, fp): + case written.takeAny(others, fl, fp): default: s.humanAdded++ } @@ -152,9 +183,9 @@ func splitByPath(sessionID string, self *state.AILineAttribution, ledgers map[st for _, line := range fl.Removed { fp := Fingerprint(line) switch { - case erased.take(self, fl.Path, fp): + case erased.takeFile(self, fl, fp): s.aiRemoved++ - case erased.takeAny(others, fl.Path, fp): + case erased.takeAny(others, fl, fp): default: s.humanRemoved++ } @@ -178,12 +209,14 @@ func netAdded(sessionID string, self *state.AILineAttribution, ledgers map[strin written := linePool{pick: func(a *state.AILineAttribution) map[string]state.LineCounts { return a.Written }} joined := make(map[[2]string]state.LineCounts) + // Two paths of the session can have the same lines in tip, after a + // rename. The order makes the split of such lines the same on each push. out := make(map[string]*fileSplit, len(tip)) - for path, lines := range tip { + for _, path := range slices.Sorted(maps.Keys(tip)) { s := &fileSplit{} out[path] = s - for _, l := range lines { + for _, l := range tip[path] { commit, ok := bySHA[l.SHA] if !ok { continue @@ -191,24 +224,24 @@ func netAdded(sessionID string, self *state.AILineAttribution, ledgers map[strin fp := Fingerprint(l.Text) if commit.Joined != nil { - key := [2]string{commit.SHA, path} - pool, ok := joined[key] - if !ok { - pool = state.LineCounts{} - pool.Add(commit.Joined[path]...) - joined[key] = pool - } - if pool.Take(fp) { + if joinedPool(joined, commit, path, tip).Take(fp) { s.humanAdded++ } continue } + // A line that the commit wrote under another path of the + // session counts for that path. + fl, ok := commit.file(path) + if !ok { + continue + } + switch { - case written.take(self, path, fp): + case written.takeFile(self, fl, fp): s.aiAdded++ - case written.takeAny(otherLedgers(sessionID, ledgers, commit.SessionIDs), path, fp): + case written.takeAny(otherLedgers(sessionID, ledgers, commit.SessionIDs), fl, fp): default: s.humanAdded++ } @@ -218,6 +251,47 @@ func netAdded(sessionID string, self *state.AILineAttribution, ledgers map[strin return out } +// file returns the lines that the commit changed in path. +func (c CommitLines) file(path string) (tracegit.FileLines, bool) { + for _, fl := range c.Files { + if fl.Path == path { + return fl, true + } + } + + return tracegit.FileLines{}, false +} + +// joinedPool returns the human lines that a joined commit added to path, one +// copy per match. A joined commit that changed the file under a path that tip +// does not hold, because a later commit renamed it, shares one pool of those +// lines across the paths of tip. +func joinedPool(pools map[[2]string]state.LineCounts, commit CommitLines, path string, tip map[string][]tracegit.BlameLine) state.LineCounts { + key := [2]string{commit.SHA, path} + if _, direct := commit.Joined[path]; !direct { + key[1] = "" + } + + pool, ok := pools[key] + if ok { + return pool + } + + pool = state.LineCounts{} + if key[1] != "" { + pool.Add(commit.Joined[path]...) + } else { + for p, fps := range commit.Joined { + if _, inTip := tip[p]; !inTip { + pool.Add(fps...) + } + } + } + pools[key] = pool + + return pool +} + // otherLedgers returns the ledgers of the sessions in ids other than // sessionID. func otherLedgers(sessionID string, ledgers map[string]*state.AILineAttribution, ids []string) []*state.AILineAttribution { @@ -256,10 +330,16 @@ func (p *linePool) take(l *state.AILineAttribution, path, fp string) bool { return c.Take(fp) } -// takeAny takes fp from the first ledger that holds it. -func (p *linePool) takeAny(ledgers []*state.AILineAttribution, path, fp string) bool { +// takeFile takes fp from the lines of ledger l for the file, under its path, +// and then under its old path when the commit renamed it. +func (p *linePool) takeFile(l *state.AILineAttribution, fl tracegit.FileLines, fp string) bool { + return p.take(l, fl.Path, fp) || (fl.OldPath != "" && p.take(l, fl.OldPath, fp)) +} + +// takeAny takes fp from the first ledger that holds it for the file. +func (p *linePool) takeAny(ledgers []*state.AILineAttribution, fl tracegit.FileLines, fp string) bool { for _, l := range ledgers { - if p.take(l, path, fp) { + if p.takeFile(l, fl, fp) { return true } } diff --git a/app/cli/internal/trace/attribution/enrich_test.go b/app/cli/internal/trace/attribution/enrich_test.go index e7d38db62..6ef880356 100644 --- a/app/cli/internal/trace/attribution/enrich_test.go +++ b/app/cli/internal/trace/attribution/enrich_test.go @@ -58,6 +58,14 @@ func ledger(sessionID, file string, written, erased []string) *state.AILineAttri } } +// counts holds the fingerprints of lines. +func counts(lines ...string) state.LineCounts { + c := state.LineCounts{} + c.Add(fps(lines...)...) + + return c +} + func TestEnrich_Legacy(t *testing.T) { t.Run("no AI data attributes all changes to human", func(t *testing.T) { changes := &aicodingsession.CodeChanges{ @@ -253,6 +261,16 @@ func TestEnrich_Lines(t *testing.T) { }, want: split{aiAdded: 2, humanAdded: 1, humanRemoved: 1}, }, + { + // R-007: the agent edited the file under its old name, and the + // commit renamed it. + name: "a renamed file matches the ledger of its old path", + ledgers: map[string]*state.AILineAttribution{testSession: ledger(testSession, "old.go", []string{"a"}, []string{"x"})}, + commits: []CommitLines{{SessionIDs: []string{testSession}, Files: []tracegit.FileLines{ + {Path: testFile, OldPath: "old.go", Added: []string{"a", "b"}, Removed: []string{"x", "y"}}, + }}}, + want: split{aiAdded: 1, aiRemoved: 1, humanAdded: 1, humanRemoved: 1}, + }, } for _, tc := range testCases { @@ -353,6 +371,42 @@ func TestEnrich_Net(t *testing.T) { tip: map[string][]tracegit.BlameLine{testFile: nil}, want: split{aiRemoved: 1, humanRemoved: 5}, }, + { + // R-007: the session wrote old.go, a commit renamed it to the + // file, and a later commit of the session changed the file. The + // tip of both paths is the file at HEAD, and each line counts for + // the path that its commit changed. + name: "a line counts for the path that its commit changed", + ledgers: map[string]*state.AILineAttribution{testSession: { + SessionID: testSession, + Written: map[string]state.LineCounts{ + "old.go": counts(five...), + testFile: counts("f"), + }, + }}, + commits: []CommitLines{ + {SHA: c1, SessionIDs: []string{testSession}, Files: []tracegit.FileLines{{Path: "old.go", Added: five}}}, + ownCommit(c2, []string{"f"}, nil), + }, + tip: map[string][]tracegit.BlameLine{ + "old.go": tipOf(c1+":a", c1+":b", c1+":c", c1+":d", c1+":e", c2+":f"), + testFile: tipOf(c1+":a", c1+":b", c1+":c", c1+":d", c1+":e", c2+":f"), + }, + want: split{aiAdded: 1}, + }, + { + // R-007: a commit in no session renamed the file and replaced a + // line of the session in it. + name: "a joined commit counts in a renamed file", + ledgers: map[string]*state.AILineAttribution{testSession: ledger(testSession, testFile, five, nil)}, + commits: []CommitLines{ + ownCommit(c1, five, nil), + {SHA: c2, Files: []tracegit.FileLines{{Path: "new.go", OldPath: testFile, Added: []string{"C"}, Removed: []string{"c"}}}, + Joined: map[string][]string{"new.go": fps("C")}}, + }, + tip: map[string][]tracegit.BlameLine{testFile: tipOf(c1+":a", c1+":b", c2+":C", c1+":d", c1+":e")}, + want: split{aiAdded: 4, humanAdded: 1}, + }, } for _, tc := range testCases { diff --git a/app/cli/internal/trace/git/git.go b/app/cli/internal/trace/git/git.go index 2f73c2cc3..f05e351fe 100644 --- a/app/cli/internal/trace/git/git.go +++ b/app/cli/internal/trace/git/git.go @@ -49,7 +49,10 @@ const ( // FileLines holds the lines that one commit added to and removed from a file, // without their line ends. type FileLines struct { - Path string + Path string + // OldPath is the path of the file in the parent, set when the commit + // renamed the file. + OldPath string Added []string Removed []string // Hunks holds the same lines, split into the hunks of the diff. @@ -91,8 +94,12 @@ type Client interface { CodeChangesForCommits(repoRoot string, shas []string) (*aicodingsession.CodeChanges, error) // CommitLines returns, for each file that the commit changed against its // first parent, the content of the lines it added and removed. Binary - // files are left out. Renames show as a removal and an addition. + // files are left out. A renamed file shows the lines that changed, under + // its new path, with its old path. CommitLines(repoRoot, sha string) ([]FileLines, error) + // Renames returns, for each file that was renamed between two revisions, + // its new path by its old path. + Renames(repoRoot, from, to string) (map[string]string, error) // FileAt returns the content of a repo-relative path at a commit, or nil // when the commit does not hold the path. FileAt(repoRoot, sha, path string) ([]byte, error) diff --git a/app/cli/internal/trace/git/lines.go b/app/cli/internal/trace/git/lines.go index bc0cdb6d3..98e5c6595 100644 --- a/app/cli/internal/trace/git/lines.go +++ b/app/cli/internal/trace/git/lines.go @@ -16,6 +16,8 @@ package git import ( + "bytes" + "context" "errors" "fmt" "io" @@ -59,7 +61,7 @@ func (c *GoGitClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { } } - changes, err := object.DiffTree(baseTree, tree) + changes, err := object.DiffTreeWithOptions(context.Background(), baseTree, tree, object.DefaultDiffTreeOptions) if err != nil { return nil, fmt.Errorf("diff %s: %w", sha, err) } @@ -76,10 +78,13 @@ func (c *GoGitClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { from, to := fp.Files() fl := FileLines{} - if to != nil { - fl.Path = to.Path() - } else if from != nil { + switch { + case to == nil: fl.Path = from.Path() + case from != nil && from.Path() != to.Path(): + fl.Path, fl.OldPath = to.Path(), from.Path() + default: + fl.Path = to.Path() } // A hunk is a run of removed and added chunks between two equal ones. @@ -121,6 +126,46 @@ func (c *GoGitClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { return out, nil } +// Renames returns the new path of each file renamed between two revisions, by +// its old path. +func (c *GoGitClient) Renames(repoRoot, from, to string) (map[string]string, error) { + repo, err := openRepo(repoRoot) + if err != nil { + return nil, fmt.Errorf("open repo: %w", err) + } + + trees := make([]*object.Tree, 0, 2) + for _, rev := range []string{from, to} { + hash, err := repo.ResolveRevision(plumbing.Revision(rev)) + if err != nil { + return nil, fmt.Errorf("resolve %s: %w", rev, err) + } + commit, err := repo.CommitObject(*hash) + if err != nil { + return nil, fmt.Errorf("get commit %s: %w", rev, err) + } + tree, err := commit.Tree() + if err != nil { + return nil, fmt.Errorf("get tree of %s: %w", rev, err) + } + trees = append(trees, tree) + } + + changes, err := object.DiffTreeWithOptions(context.Background(), trees[0], trees[1], object.DefaultDiffTreeOptions) + if err != nil { + return nil, fmt.Errorf("diff %s %s: %w", from, to, err) + } + + out := make(map[string]string) + for _, ch := range changes { + if ch.From.Name != "" && ch.To.Name != "" && ch.From.Name != ch.To.Name { + out[ch.From.Name] = ch.To.Name + } + } + + return out, nil +} + // FileAt returns the content of path at the commit, or nil when the commit // does not hold it. sha can be any revision, such as HEAD. func (c *GoGitClient) FileAt(repoRoot, sha, path string) ([]byte, error) { @@ -158,7 +203,7 @@ func (c *GoGitClient) FileAt(repoRoot, sha, path string) ([]byte, error) { func (c *ExecClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { base := commitParentOrEmptyTree(repoRoot, sha) raw, err := gitRawOutput(repoRoot, "-c", "core.quotePath=false", "diff", "--no-color", "--no-ext-diff", - "--no-renames", "-U0", base, sha) + "-M", "-U0", base, sha) if err != nil { return nil, fmt.Errorf("diff %s: %w", sha, err) } @@ -202,6 +247,11 @@ func (c *ExecClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { cur.Path = strings.TrimSuffix(strings.TrimPrefix(line, "--- a/"), "\t") case strings.HasPrefix(line, "+++ b/"): cur.Path = strings.TrimSuffix(strings.TrimPrefix(line, "+++ b/"), "\t") + // A rename with no changed line has no "---" and "+++" headers. + case strings.HasPrefix(line, "rename from "): + cur.OldPath = strings.TrimPrefix(line, "rename from ") + case strings.HasPrefix(line, "rename to "): + cur.Path = strings.TrimPrefix(line, "rename to ") case strings.HasPrefix(line, "Binary files "): binary = true case strings.HasPrefix(line, "@@"): @@ -229,6 +279,25 @@ func (c *ExecClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { return out, nil } +// Renames reads `git diff --name-status` between two revisions, with rename +// detection on, whatever the configuration of the user says. +func (c *ExecClient) Renames(repoRoot, from, to string) (map[string]string, error) { + raw, err := gitRawOutput(repoRoot, "diff", "--no-ext-diff", "-M", "--name-status", "-z", "--diff-filter=R", from, to) + if err != nil { + return nil, fmt.Errorf("diff %s %s: %w", from, to, err) + } + + // Each rename is three fields: its status, the old path, and the new + // path. + fields := bytes.Split(bytes.TrimSuffix(raw, []byte{0}), []byte{0}) + out := make(map[string]string) + for i := 0; i+2 < len(fields); i += 3 { + out[string(fields[i+1])] = string(fields[i+2]) + } + + return out, nil +} + // hunkOldStart returns the first line of the parent's file that a hunk // header "@@ -a,b +c,d @@" covers. A hunk that removes nothing gives a as the // line before the insertion, so the hunk starts at a+1. diff --git a/app/cli/internal/trace/git/lines_test.go b/app/cli/internal/trace/git/lines_test.go index 43cb36a0f..59e5e92f7 100644 --- a/app/cli/internal/trace/git/lines_test.go +++ b/app/cli/internal/trace/git/lines_test.go @@ -20,6 +20,7 @@ import ( "os/exec" "path/filepath" "sort" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -134,6 +135,66 @@ func TestCommitLinesAndFileAt(t *testing.T) { } } +func TestCommitLinesRenames(t *testing.T) { + dir := t.TempDir() + run := func(args ...string) string { return gitOutput(dir, args...) } + write := func(name, content string) { + require.NoError(t, os.WriteFile(filepath.Join(dir, name), []byte(content), 0600)) + } + + run("init", "-b", "main") + run("config", "user.email", "test@test.com") + run("config", "user.name", "Test") + run("config", "commit.gpgsign", "false") + run("config", "diff.renames", "false") + + body := "one\ntwo\nthree\nfour\nfive\nsix\n" + write("a.txt", body) + write("b.txt", body+"seven\n") + run("add", ".") + run("commit", "-m", "root") + root := run("rev-parse", "HEAD") + + run("mv", "a.txt", "moved.txt") + run("mv", "b.txt", "changed.txt") + write("changed.txt", strings.Replace(body+"seven\n", lineTwo, "TWO", 1)) + run("add", "-A") + run("commit", "-m", "rename") + renamed := run("rev-parse", "HEAD") + + run("mv", "moved.txt", "again.txt") + run("commit", "-m", "rename again") + + clients := map[string]Client{"gogit": NewGoGitClient(), "exec": NewExecClient()} + for name, c := range clients { + t.Run(name, func(t *testing.T) { + t.Run("a renamed file keeps its unchanged lines", func(t *testing.T) { + got, err := c.CommitLines(dir, renamed) + require.NoError(t, err) + sortFileLines(got) + two, upperTwo := []string{lineTwo}, []string{"TWO"} + assert.Equal(t, []FileLines{ + {Path: "changed.txt", OldPath: "b.txt", Added: upperTwo, Removed: two, Hunks: []Hunk{{OldStart: 2, Removed: two, Added: upperTwo}}}, + {Path: "moved.txt", OldPath: "a.txt"}, + }, got) + }) + + t.Run("renames between two commits", func(t *testing.T) { + got, err := c.Renames(dir, root, "HEAD") + require.NoError(t, err) + assert.Equal(t, map[string]string{"a.txt": "again.txt", "b.txt": "changed.txt"}, got) + + got, err = c.Renames(dir, "HEAD", "HEAD") + require.NoError(t, err) + assert.Empty(t, got) + + _, err = c.Renames(dir, "deadbeef00000000000000000000000000000000", "HEAD") + assert.Error(t, err) + }) + }) + } +} + func sortFileLines(fl []FileLines) { sort.Slice(fl, func(i, j int) bool { return fl[i].Path < fl[j].Path }) } diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 3c82cfe17..15ed9c321 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -1076,13 +1076,14 @@ func sessionCodeChanges(gitClient tracegit.Client, repoRoot, sessionID string, c attribution.FilterGenerated(codeChanges, isGenerated) - // A legacy file keeps the range rule, which needs no blame. + // A legacy file keeps the range rule, which needs no blame. A file that a + // later commit renamed is blamed under its path at HEAD (R-007). tip := make(map[string][]tracegit.BlameLine, len(codeChanges.Files)) for _, f := range codeChanges.Files { if l := ledgers[sessionID]; l != nil && l.Legacy[f.Path] { continue } - if lines, ok := cache.blame(f.Path); ok { + if lines, ok := cache.blame(cache.headPath(shas, f.Path)); ok { tip[f.Path] = lines } } @@ -1126,6 +1127,7 @@ type lineCache struct { log zerolog.Logger commits map[string][]tracegit.FileLines blames map[string][]tracegit.BlameLine + renames map[string]map[string]string } func newLineCache(client tracegit.Client, repoRoot string, log zerolog.Logger) *lineCache { @@ -1135,9 +1137,38 @@ func newLineCache(client tracegit.Client, repoRoot string, log zerolog.Logger) * log: log, commits: make(map[string][]tracegit.FileLines), blames: make(map[string][]tracegit.BlameLine), + renames: make(map[string]map[string]string), } } +// headPath returns the path at HEAD of a file that the last of the commits +// that changed it holds at path. A later commit can have renamed the file. It +// returns path when no commit can be read or the renames cannot be read. +func (c *lineCache) headPath(shas []string, path string) string { + for _, sha := range slices.Backward(shas) { + lines, ok := c.commit(sha) + if !ok || !slices.ContainsFunc(lines, func(fl tracegit.FileLines) bool { return fl.Path == path }) { + continue + } + + renames, ok := c.renames[sha] + if !ok { + var err error + if renames, err = c.client.Renames(c.repoRoot, sha, "HEAD"); err != nil { + c.log.Debug().Err(err).Str("sha", sha).Msg("could not read the renames since a commit") + } + c.renames[sha] = renames + } + if to, ok := renames[path]; ok { + return to + } + + return path + } + + return path +} + // commit returns the lines that a commit changed, and false when the commit // cannot be read. func (c *lineCache) commit(sha string) ([]tracegit.FileLines, bool) { diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index be430c55e..752f25c5e 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -299,6 +299,9 @@ func (f *fakeBranchClient) CommitLines(string, string) ([]tracegit.FileLines, er } func (f *fakeBranchClient) FileAt(string, string, string) ([]byte, error) { return nil, nil } +func (f *fakeBranchClient) Renames(string, string, string) (map[string]string, error) { + return nil, nil +} func (f *fakeBranchClient) Blame(string, string, string) ([]tracegit.BlameLine, error) { return nil, nil diff --git a/app/cli/pkg/action/trace_line_attribution_test.go b/app/cli/pkg/action/trace_line_attribution_test.go index 2d35ca355..d35a0858c 100644 --- a/app/cli/pkg/action/trace_line_attribution_test.go +++ b/app/cli/pkg/action/trace_line_attribution_test.go @@ -118,6 +118,7 @@ func (r *lineRepo) push() map[string]pushed { require.NoError(r.t, err) client := tracegit.NewGitClient() + records = filterCurrentBranchCommits(client, r.dir, records, zerolog.Nop()) ledgers := loadLedgers(r.store, zerolog.Nop()) affected, failed := scanReplacements(client, r.store, r.dir, records, ledgers, zerolog.Nop()) require.Empty(r.t, failed) diff --git a/app/cli/pkg/action/trace_line_rewrites_test.go b/app/cli/pkg/action/trace_line_rewrites_test.go new file mode 100644 index 000000000..fbb7ba298 --- /dev/null +++ b/app/cli/pkg/action/trace_line_rewrites_test.go @@ -0,0 +1,188 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package action + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The tests below follow R-007 of spec issue-3639: the AI/human split +// survives a rename, an amend, a rebase, and a squash in the same clone. + +// rewrite runs a git command that rewrites commits, and then the hooks that +// git runs for it: commit-msg and post-commit for the new HEAD commit, and +// post-rewrite with the old commits that it replaced. +func (r *lineRepo) rewrite(subject string, olds []string, args ...string) { + r.t.Helper() + msg := filepath.Join(r.t.TempDir(), "msg") + require.NoError(r.t, os.WriteFile(msg, []byte(subject+"\n"), 0600)) + require.NoError(r.t, handleCommitMsg(msg, zerolog.Nop())) + runGit(r.t, r.dir, append(args, "-F", msg)...) + require.NoError(r.t, handlePostCommit(r.t.Context(), zerolog.Nop())) + + head := headSHA(r.t, r.dir) + var pairs strings.Builder + for _, old := range olds { + pairs.WriteString(old + " " + head + "\n") + } + require.NoError(r.t, handlePostRewrite(strings.NewReader(pairs.String()), zerolog.Nop())) +} + +// An amend that replaces the message drops the trailer. The split stays, also +// for a human line that the amend adds. +func TestLineAttribution_SurvivesAmend(t *testing.T) { + r := newLineRepo(t) + + r.agentWrite(lineSession, "a.go", "one\ntwo\nthree\n") + r.commit("feat: a", "a.go") + old := headSHA(t, r.dir) + + r.humanWrite("a.go", "one\ntwo\nthree\nby hand\n") + runGit(t, r.dir, "add", "a.go") + r.rewrite("feat: a, reworded", []string{old}, "commit", "--amend") + + got := r.push() + require.Contains(t, got, lineSession) + assert.Equal(t, lineSplit{aiAdded: 3, humanAdded: 1}, fileSplitOf(t, got[lineSession].changes, "a.go")) + assert.Equal(t, []string{headSHA(t, r.dir)}, got[lineSession].commits, "the old commit is gone") +} + +// A rebase onto a branch that moved on replays the commits of the session. +func TestLineAttribution_SurvivesRebase(t *testing.T) { + r := newLineRepo(t) + + runGit(t, r.dir, "checkout", "-b", "feature") + r.agentWrite(lineSession, "a.go", "one\ntwo\nthree\n") + r.humanWrite("a.go", "one\ntwo\nthree\nby hand\n") + r.commit("feat: a", "a.go") + old := headSHA(t, r.dir) + + runGit(t, r.dir, "checkout", "main") + r.humanWrite("upstream.txt", "upstream\n") + r.commit("chore: upstream", "upstream.txt") + runGit(t, r.dir, "checkout", "feature") + runGit(t, r.dir, "rebase", "main") + require.NoError(t, handlePostCommit(t.Context(), zerolog.Nop())) + require.NoError(t, handlePostRewrite(strings.NewReader(old+" "+headSHA(t, r.dir)+"\n"), zerolog.Nop())) + + assert.Equal(t, lineSplit{aiAdded: 3, humanAdded: 1}, fileSplitOf(t, r.sessionChanges(lineSession), "a.go")) +} + +// A squash of two commits of the session keeps the split of both. +func TestLineAttribution_SurvivesSquash(t *testing.T) { + r := newLineRepo(t) + + r.agentWrite(lineSession, "a.go", "one\ntwo\nthree\n") + r.commit("feat: a", "a.go") + first := headSHA(t, r.dir) + + r.agentWrite(lineSession, "b.go", "four\nfive\n") + r.humanWrite("a.go", "one\nTWO\nthree\n") + r.commit("feat: b", "a.go", "b.go") + second := headSHA(t, r.dir) + + runGit(t, r.dir, "reset", "--soft", "HEAD~2") + r.rewrite("feat: a and b", []string{first, second}, "commit") + + got := r.push() + require.Contains(t, got, lineSession) + assert.Equal(t, lineSplit{aiAdded: 2, humanAdded: 1}, fileSplitOf(t, got[lineSession].changes, "a.go")) + assert.Equal(t, lineSplit{aiAdded: 2}, fileSplitOf(t, got[lineSession].changes, "b.go")) +} + +// A human rename of a file of the session, before the push, keeps the AI +// lines of the file. +func TestLineAttribution_RenameBeforePush(t *testing.T) { + r := newLineRepo(t) + + r.agentWrite(lineSession, "a.go", content(numbered("ai", 5))) + r.commit("feat: a", "a.go") + runGit(t, r.dir, "mv", "a.go", "b.go") + r.commit("refactor: rename", "b.go") + + assert.Equal(t, lineSplit{aiAdded: 5}, fileSplitOf(t, r.sessionChanges(lineSession), "a.go")) +} + +// A human change to a renamed file of the session, after the push, replaces +// the AI lines and joins the session, as it does without the rename (R-004, +// R-010). +func TestLineAttribution_ChangeAfterRename(t *testing.T) { + testCases := []struct { + name string + // sameCommit renames and changes the file in one commit. + sameCommit bool + // pushRename pushes the rename before the change. + pushRename bool + }{ + {name: "rename and change in two commits"}, + {name: "rename and change in one commit", sameCommit: true}, + {name: "rename pushed before the change", pushRename: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := newLineRepo(t) + + lines := numbered("ai", 5) + r.agentWrite(lineSession, "a.go", content(lines)) + r.commit("feat: a", "a.go") + r.push() + + runGit(t, r.dir, "mv", "a.go", "b.go") + if !tc.sameCommit { + r.commit("refactor: rename", "b.go") + } + if tc.pushRename { + assert.Empty(t, r.push(), "a rename alone changes no line") + } + lines[1] = "by hand" + r.humanWrite("b.go", content(lines)) + r.commit("fix: by hand", "b.go") + human := headSHA(t, r.dir) + + got := r.push() + require.Contains(t, got, lineSession, "the session is sent again") + assert.Equal(t, lineSplit{aiAdded: 4, humanAdded: 1}, fileSplitOf(t, got[lineSession].changes, "a.go")) + assert.Contains(t, got[lineSession].commits, human) + }) + } +} + +// An agent that renames a file of a human and changes one line of it wrote +// that one line only, and removed the line that it replaced. +func TestLineAttribution_AgentRenamesHumanFile(t *testing.T) { + r := newLineRepo(t) + + lines := numbered("h", 5) + r.humanWrite("h.go", content(lines)) + r.commit("feat: h", "h.go") + + r.shellWrite(lineSession, func() { + runGit(t, r.dir, "mv", "h.go", "g.go") + lines[2] = "agent" + r.humanWrite("g.go", content(lines)) + }) + r.commit("refactor: g", "g.go") + + assert.Equal(t, lineSplit{aiAdded: 1, aiRemoved: 1}, fileSplitOf(t, r.sessionChanges(lineSession), "g.go")) +} diff --git a/app/cli/pkg/action/trace_replacements.go b/app/cli/pkg/action/trace_replacements.go index a64c6c994..c1f31f609 100644 --- a/app/cli/pkg/action/trace_replacements.go +++ b/app/cli/pkg/action/trace_replacements.go @@ -38,8 +38,9 @@ import ( // So the commits are read oldest first, and read again until no join changes. // // Only files with ledger entries are blamed, because only those can hold the -// lines of a session. It also returns the commits that it could not read, -// which must stay untracked so that the next push reads them again. +// lines of a session. A file that a commit renamed is blamed under its new +// path too (R-007). It also returns the commits that it could not read, which +// must stay untracked so that the next push reads them again. func scanReplacements(client tracegit.Client, store *state.Store, repoRoot string, records []*state.CommitRecord, ledgers map[string]*state.AILineAttribution, log zerolog.Logger) (affected map[string]struct{}, failed map[string]bool) { bySHA := make(map[string]*state.CommitRecord, len(records)) var untracked []*state.CommitRecord @@ -63,10 +64,12 @@ func scanReplacements(client tracegit.Client, store *state.Store, repoRoot strin scan := &replacementScan{ client: client, repoRoot: repoRoot, + records: records, bySHA: bySHA, ledgerFiles: ledgerFiles, lines: make(map[string][]tracegit.FileLines), blames: make(map[[2]string][]tracegit.BlameLine), + log: log, } affected = make(map[string]struct{}) @@ -115,12 +118,49 @@ func scanReplacements(client tracegit.Client, store *state.Store, repoRoot strin // replacementScan reads the lines of commits and the blame of their parents, // each one time for all the passes of scanReplacements. type replacementScan struct { - client tracegit.Client - repoRoot string - bySHA map[string]*state.CommitRecord + client tracegit.Client + repoRoot string + records []*state.CommitRecord + bySHA map[string]*state.CommitRecord + // ledgerFiles holds the paths that can hold lines of a session: the + // paths of the ledgers, and the paths that a commit renamed them to. ledgerFiles map[string]bool - lines map[string][]tracegit.FileLines - blames map[[2]string][]tracegit.BlameLine + // pushedRenames is set once ledgerFiles holds the paths that pushed + // commits renamed the ledger files to. + pushedRenames bool + lines map[string][]tracegit.FileLines + blames map[[2]string][]tracegit.BlameLine + log zerolog.Logger +} + +// holdsSessionLines reports whether a file of the parent can hold lines of a +// session. A rename in a commit that a push already read is not seen in the +// commits that the scan reads, so on the first file that is not a ledger +// file, the paths at HEAD of the files of the pushed session commits are +// added. +func (s *replacementScan) holdsSessionLines(path string) bool { + if s.ledgerFiles[path] || s.pushedRenames { + return s.ledgerFiles[path] + } + s.pushedRenames = true + + for _, r := range s.records { + if !r.Tracked || len(r.AllSessionIDs()) == 0 { + continue + } + renames, err := s.client.Renames(s.repoRoot, r.SHA, "HEAD") + if err != nil { + s.log.Debug().Err(err).Str("sha", r.SHA).Msg("could not read the renames since a commit") + continue + } + for from, to := range renames { + if s.ledgerFiles[from] { + s.ledgerFiles[to] = true + } + } + } + + return s.ledgerFiles[path] } // replacedSessions adds to affected each session that owns a line that the @@ -139,25 +179,36 @@ func (s *replacementScan) replacedSessions(rec *state.CommitRecord, affected map var joined map[string]map[string][]string for _, fl := range lines { + // The parent holds the file under its old path. + parentPath := fl.Path + if fl.OldPath != "" { + parentPath = fl.OldPath + // The commits are read oldest first, so a later commit that + // changes the file under its new path is blamed too. + if s.ledgerFiles[fl.OldPath] { + s.ledgerFiles[fl.Path] = true + } + } + // A root commit removes nothing, so its parent is never blamed. - if len(fl.Removed) == 0 || !s.ledgerFiles[fl.Path] { + if len(fl.Removed) == 0 || !s.holdsSessionLines(parentPath) { continue } // The removed lines are lines of the parent, so the parent's blame // names the commit that last wrote each one. - key := [2]string{rec.SHA, fl.Path} + key := [2]string{rec.SHA, parentPath} blame, ok := s.blames[key] if !ok { var err error - if blame, err = s.client.Blame(s.repoRoot, rec.SHA+"^", fl.Path); err != nil { + if blame, err = s.client.Blame(s.repoRoot, rec.SHA+"^", parentPath); err != nil { return nil, err } s.blames[key] = blame } for _, h := range fl.Hunks { - for _, sid := range hunkOwners(h, blame, s.bySHA, rec.SHA, fl.Path) { + for _, sid := range hunkOwners(h, blame, s.bySHA, rec.SHA, parentPath) { affected[sid] = struct{}{} if len(rec.SessionIDs) > 0 { continue From a07ef8da3d3f934a01e29d8b80c75aac55786925 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 19 Oct 2026 19:57:05 +0200 Subject: [PATCH 2/2] fix(trace): address review of rename attribution A joined line is found in any file of its join, so a rename after the join does not hide it. A failed read of the renames keeps the commit untracked for the next push. Quoted rename paths are decoded, and the go-git client finds a rename at the same 50% threshold as git. Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: 81312179-16f4-4c92-a45b-a104a593a498 --- .../internal/trace/attribution/enrich_test.go | 13 ++- app/cli/internal/trace/git/lines.go | 25 ++++- app/cli/internal/trace/git/lines_test.go | 99 ++++++++++++++----- .../pkg/action/trace_line_rewrites_test.go | 64 ++++++++++++ app/cli/pkg/action/trace_replacements.go | 57 ++++++++--- 5 files changed, 211 insertions(+), 47 deletions(-) diff --git a/app/cli/internal/trace/attribution/enrich_test.go b/app/cli/internal/trace/attribution/enrich_test.go index 6ef880356..61b7f0f8a 100644 --- a/app/cli/internal/trace/attribution/enrich_test.go +++ b/app/cli/internal/trace/attribution/enrich_test.go @@ -58,6 +58,9 @@ func ledger(sessionID, file string, written, erased []string) *state.AILineAttri } } +// oldFile is the path of testFile before a rename. +const oldFile = "old.go" + // counts holds the fingerprints of lines. func counts(lines ...string) state.LineCounts { c := state.LineCounts{} @@ -265,9 +268,9 @@ func TestEnrich_Lines(t *testing.T) { // R-007: the agent edited the file under its old name, and the // commit renamed it. name: "a renamed file matches the ledger of its old path", - ledgers: map[string]*state.AILineAttribution{testSession: ledger(testSession, "old.go", []string{"a"}, []string{"x"})}, + ledgers: map[string]*state.AILineAttribution{testSession: ledger(testSession, oldFile, []string{"a"}, []string{"x"})}, commits: []CommitLines{{SessionIDs: []string{testSession}, Files: []tracegit.FileLines{ - {Path: testFile, OldPath: "old.go", Added: []string{"a", "b"}, Removed: []string{"x", "y"}}, + {Path: testFile, OldPath: oldFile, Added: []string{"a", "b"}, Removed: []string{"x", "y"}}, }}}, want: split{aiAdded: 1, aiRemoved: 1, humanAdded: 1, humanRemoved: 1}, }, @@ -380,16 +383,16 @@ func TestEnrich_Net(t *testing.T) { ledgers: map[string]*state.AILineAttribution{testSession: { SessionID: testSession, Written: map[string]state.LineCounts{ - "old.go": counts(five...), + oldFile: counts(five...), testFile: counts("f"), }, }}, commits: []CommitLines{ - {SHA: c1, SessionIDs: []string{testSession}, Files: []tracegit.FileLines{{Path: "old.go", Added: five}}}, + {SHA: c1, SessionIDs: []string{testSession}, Files: []tracegit.FileLines{{Path: oldFile, Added: five}}}, ownCommit(c2, []string{"f"}, nil), }, tip: map[string][]tracegit.BlameLine{ - "old.go": tipOf(c1+":a", c1+":b", c1+":c", c1+":d", c1+":e", c2+":f"), + oldFile: tipOf(c1+":a", c1+":b", c1+":c", c1+":d", c1+":e", c2+":f"), testFile: tipOf(c1+":a", c1+":b", c1+":c", c1+":d", c1+":e", c2+":f"), }, want: split{aiAdded: 1}, diff --git a/app/cli/internal/trace/git/lines.go b/app/cli/internal/trace/git/lines.go index 98e5c6595..0354e6599 100644 --- a/app/cli/internal/trace/git/lines.go +++ b/app/cli/internal/trace/git/lines.go @@ -31,6 +31,10 @@ import ( "github.com/go-git/go-git/v6/plumbing/object" ) +// renameOptions find a rename that keeps at least half of the file, as git +// does by default. +var renameOptions = &object.DiffTreeOptions{DetectRenames: true, RenameScore: 50} + // CommitLines returns the lines that the commit added and removed, per file, // against its first parent. func (c *GoGitClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { @@ -61,7 +65,7 @@ func (c *GoGitClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { } } - changes, err := object.DiffTreeWithOptions(context.Background(), baseTree, tree, object.DefaultDiffTreeOptions) + changes, err := object.DiffTreeWithOptions(context.Background(), baseTree, tree, renameOptions) if err != nil { return nil, fmt.Errorf("diff %s: %w", sha, err) } @@ -151,7 +155,7 @@ func (c *GoGitClient) Renames(repoRoot, from, to string) (map[string]string, err trees = append(trees, tree) } - changes, err := object.DiffTreeWithOptions(context.Background(), trees[0], trees[1], object.DefaultDiffTreeOptions) + changes, err := object.DiffTreeWithOptions(context.Background(), trees[0], trees[1], renameOptions) if err != nil { return nil, fmt.Errorf("diff %s %s: %w", from, to, err) } @@ -249,9 +253,9 @@ func (c *ExecClient) CommitLines(repoRoot, sha string) ([]FileLines, error) { cur.Path = strings.TrimSuffix(strings.TrimPrefix(line, "+++ b/"), "\t") // A rename with no changed line has no "---" and "+++" headers. case strings.HasPrefix(line, "rename from "): - cur.OldPath = strings.TrimPrefix(line, "rename from ") + cur.OldPath = unquotePath(strings.TrimPrefix(line, "rename from ")) case strings.HasPrefix(line, "rename to "): - cur.Path = strings.TrimPrefix(line, "rename to ") + cur.Path = unquotePath(strings.TrimPrefix(line, "rename to ")) case strings.HasPrefix(line, "Binary files "): binary = true case strings.HasPrefix(line, "@@"): @@ -298,6 +302,19 @@ func (c *ExecClient) Renames(repoRoot, from, to string) (map[string]string, erro return out, nil } +// unquotePath decodes a path that git wrote in quotes, with C escapes, as it +// does for a path with a control character or a quote in it. +func unquotePath(path string) string { + if !strings.HasPrefix(path, `"`) { + return path + } + if unquoted, err := strconv.Unquote(path); err == nil { + return unquoted + } + + return path +} + // hunkOldStart returns the first line of the parent's file that a hunk // header "@@ -a,b +c,d @@" covers. A hunk that removes nothing gives a as the // line before the insertion, so the hunk starts at a+1. diff --git a/app/cli/internal/trace/git/lines_test.go b/app/cli/internal/trace/git/lines_test.go index 59e5e92f7..49dc3fba1 100644 --- a/app/cli/internal/trace/git/lines_test.go +++ b/app/cli/internal/trace/git/lines_test.go @@ -16,6 +16,7 @@ package git import ( + "fmt" "os" "os/exec" "path/filepath" @@ -31,8 +32,17 @@ const ( lineOne = "one" lineTwo = "two" lineFour = "four" + + fileA = "a.txt" + fileB = "b.txt" + lineUpperTwo = "TWO" ) +// testClients returns both implementations of the client, by name. +func testClients() map[string]Client { + return map[string]Client{"gogit": NewGoGitClient(), "exec": NewExecClient()} +} + func TestCommitLinesAndFileAt(t *testing.T) { dir := t.TempDir() run := func(args ...string) string { return gitOutput(dir, args...) } @@ -45,14 +55,14 @@ func TestCommitLinesAndFileAt(t *testing.T) { run("config", "user.name", "Test") run("config", "commit.gpgsign", "false") - write("a.txt", "one\ntwo\nthree\n") + write(fileA, "one\ntwo\nthree\n") write("gone.txt", "bye\n") write("bin.dat", "\x00\x01\x02") run("add", ".") run("commit", "-m", "root") root := run("rev-parse", "HEAD") - write("a.txt", "one\nTWO\nthree\nfour") + write(fileA, "one\nTWO\nthree\nfour") write("new.txt", "hello\n") write("with space.txt", "spaced\n") write("bin.dat", "\x00\x03") @@ -61,7 +71,7 @@ func TestCommitLinesAndFileAt(t *testing.T) { run("commit", "-m", "second") second := run("rev-parse", "HEAD") - clients := map[string]Client{"gogit": NewGoGitClient(), "exec": NewExecClient()} + clients := testClients() for name, c := range clients { t.Run(name, func(t *testing.T) { t.Run("root commit adds every line", func(t *testing.T) { @@ -70,7 +80,7 @@ func TestCommitLinesAndFileAt(t *testing.T) { sortFileLines(got) rootA, rootGone := []string{lineOne, lineTwo, "three"}, []string{"bye"} assert.Equal(t, []FileLines{ - {Path: "a.txt", Added: rootA, Hunks: []Hunk{{OldStart: 1, Added: rootA}}}, + {Path: fileA, Added: rootA, Hunks: []Hunk{{OldStart: 1, Added: rootA}}}, {Path: "gone.txt", Added: rootGone, Hunks: []Hunk{{OldStart: 1, Added: rootGone}}}, }, got, "binary files are left out") }) @@ -79,9 +89,9 @@ func TestCommitLinesAndFileAt(t *testing.T) { got, err := c.CommitLines(dir, second) require.NoError(t, err) sortFileLines(got) - two, upperTwo, bye := []string{lineTwo}, []string{"TWO"}, []string{"bye"} + two, upperTwo, bye := []string{lineTwo}, []string{lineUpperTwo}, []string{"bye"} assert.Equal(t, []FileLines{ - {Path: "a.txt", Added: []string{upperTwo[0], lineFour}, Removed: two, Hunks: []Hunk{ + {Path: fileA, Added: []string{upperTwo[0], lineFour}, Removed: two, Hunks: []Hunk{ {OldStart: 2, Removed: two, Added: upperTwo}, {OldStart: 4, Added: []string{lineFour}}, }}, @@ -92,7 +102,7 @@ func TestCommitLinesAndFileAt(t *testing.T) { }) t.Run("file content at a commit", func(t *testing.T) { - content, err := c.FileAt(dir, second, "a.txt") + content, err := c.FileAt(dir, second, fileA) require.NoError(t, err) assert.Equal(t, "one\nTWO\nthree\nfour", string(content)) @@ -104,7 +114,7 @@ func TestCommitLinesAndFileAt(t *testing.T) { require.NoError(t, err) assert.Equal(t, "hello\n", string(content), "a revision name resolves") - _, err = c.FileAt(dir, "deadbeef00000000000000000000000000000000", "a.txt") + _, err = c.FileAt(dir, "deadbeef00000000000000000000000000000000", fileA) assert.Error(t, err, "an unknown commit is an error, not an absent file") }) @@ -114,16 +124,16 @@ func TestCommitLinesAndFileAt(t *testing.T) { }) t.Run("blame names the last writer of each line", func(t *testing.T) { - got, err := c.Blame(dir, second, "a.txt") + got, err := c.Blame(dir, second, fileA) require.NoError(t, err) assert.Equal(t, []BlameLine{ {SHA: root, Text: lineOne}, - {SHA: second, Text: "TWO"}, + {SHA: second, Text: lineUpperTwo}, {SHA: root, Text: "three"}, {SHA: second, Text: lineFour}, }, got) - got, err = c.Blame(dir, second+"^", "a.txt") + got, err = c.Blame(dir, second+"^", fileA) require.NoError(t, err) assert.Len(t, got, 3, "a revision name resolves") @@ -149,15 +159,18 @@ func TestCommitLinesRenames(t *testing.T) { run("config", "diff.renames", "false") body := "one\ntwo\nthree\nfour\nfive\nsix\n" - write("a.txt", body) - write("b.txt", body+"seven\n") + write(fileA, body) + write(fileB, body+"seven\n") + // Git quotes a path with a quote in the headers of a diff. + write("q\"ab.txt", "quote one\nquote two\nquote three\n") run("add", ".") run("commit", "-m", "root") root := run("rev-parse", "HEAD") - run("mv", "a.txt", "moved.txt") - run("mv", "b.txt", "changed.txt") - write("changed.txt", strings.Replace(body+"seven\n", lineTwo, "TWO", 1)) + run("mv", fileA, "moved.txt") + run("mv", fileB, "changed.txt") + run("mv", "q\"ab.txt", "q\"moved.txt") + write("changed.txt", strings.Replace(body+"seven\n", lineTwo, lineUpperTwo, 1)) run("add", "-A") run("commit", "-m", "rename") renamed := run("rev-parse", "HEAD") @@ -165,24 +178,25 @@ func TestCommitLinesRenames(t *testing.T) { run("mv", "moved.txt", "again.txt") run("commit", "-m", "rename again") - clients := map[string]Client{"gogit": NewGoGitClient(), "exec": NewExecClient()} + clients := testClients() for name, c := range clients { t.Run(name, func(t *testing.T) { t.Run("a renamed file keeps its unchanged lines", func(t *testing.T) { got, err := c.CommitLines(dir, renamed) require.NoError(t, err) sortFileLines(got) - two, upperTwo := []string{lineTwo}, []string{"TWO"} + two, upperTwo := []string{lineTwo}, []string{lineUpperTwo} assert.Equal(t, []FileLines{ - {Path: "changed.txt", OldPath: "b.txt", Added: upperTwo, Removed: two, Hunks: []Hunk{{OldStart: 2, Removed: two, Added: upperTwo}}}, - {Path: "moved.txt", OldPath: "a.txt"}, + {Path: "changed.txt", OldPath: fileB, Added: upperTwo, Removed: two, Hunks: []Hunk{{OldStart: 2, Removed: two, Added: upperTwo}}}, + {Path: "moved.txt", OldPath: fileA}, + {Path: "q\"moved.txt", OldPath: "q\"ab.txt"}, }, got) }) t.Run("renames between two commits", func(t *testing.T) { got, err := c.Renames(dir, root, "HEAD") require.NoError(t, err) - assert.Equal(t, map[string]string{"a.txt": "again.txt", "b.txt": "changed.txt"}, got) + assert.Equal(t, map[string]string{fileA: "again.txt", fileB: "changed.txt", "q\"ab.txt": "q\"moved.txt"}, got) got, err = c.Renames(dir, "HEAD", "HEAD") require.NoError(t, err) @@ -195,6 +209,45 @@ func TestCommitLinesRenames(t *testing.T) { } } +// Both clients find a rename that keeps at least half of the file, as git +// does by default. +func TestRenamesKeepHalf(t *testing.T) { + dir := t.TempDir() + run := func(args ...string) string { return gitOutput(dir, args...) } + run("init", "-b", "main") + run("config", "user.email", "test@test.com") + run("config", "user.name", "Test") + run("config", "commit.gpgsign", "false") + + var kept, changed strings.Builder + for i := range 20 { + line := fmt.Sprintf("line number %02d\n", i) + kept.WriteString(line) + if i < 9 { + line = fmt.Sprintf("LINE CHANGE %02d\n", i) + } + changed.WriteString(line) + } + require.NoError(t, os.WriteFile(filepath.Join(dir, fileA), []byte(kept.String()), 0600)) + run("add", ".") + run("commit", "-m", "root") + root := run("rev-parse", "HEAD") + + run("mv", fileA, fileB) + require.NoError(t, os.WriteFile(filepath.Join(dir, fileB), []byte(changed.String()), 0600)) + run("add", "-A") + run("commit", "-m", "rename") + + clients := testClients() + for name, c := range clients { + t.Run(name, func(t *testing.T) { + got, err := c.Renames(dir, root, "HEAD") + require.NoError(t, err) + assert.Equal(t, map[string]string{fileA: fileB}, got) + }) + } +} + func sortFileLines(fl []FileLines) { sort.Slice(fl, func(i, j int) bool { return fl[i].Path < fl[j].Path }) } @@ -227,12 +280,12 @@ func TestGoGitBlameFallback(t *testing.T) { run("config", "user.email", "test@test.com") run("config", "user.name", "Test") run("config", "commit.gpgsign", "false") - require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("one\ntwo\n"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, fileA), []byte("one\ntwo\n"), 0600)) run("add", ".") run("commit", "-m", "root") root := run("rev-parse", "HEAD") - got, err := goGitBlame(dir, "HEAD", "a.txt") + got, err := goGitBlame(dir, "HEAD", fileA) require.NoError(t, err) assert.Equal(t, []BlameLine{{SHA: root, Text: lineOne}, {SHA: root, Text: lineTwo}}, got) } diff --git a/app/cli/pkg/action/trace_line_rewrites_test.go b/app/cli/pkg/action/trace_line_rewrites_test.go index fbb7ba298..843092206 100644 --- a/app/cli/pkg/action/trace_line_rewrites_test.go +++ b/app/cli/pkg/action/trace_line_rewrites_test.go @@ -16,11 +16,13 @@ package action import ( + "errors" "os" "path/filepath" "strings" "testing" + tracegit "github.com/chainloop-dev/chainloop/app/cli/internal/trace/git" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -186,3 +188,65 @@ func TestLineAttribution_AgentRenamesHumanFile(t *testing.T) { assert.Equal(t, lineSplit{aiAdded: 1, aiRemoved: 1}, fileSplitOf(t, r.sessionChanges(lineSession), "g.go")) } + +// A human commit that replaces a line that an earlier joined commit added +// joins the session, also when a commit renamed the file in between. +func TestLineAttribution_ChainedJoinAfterRename(t *testing.T) { + r := newLineRepo(t) + + lines := numbered("ai", 5) + r.agentWrite(lineSession, "a.go", content(lines)) + r.commit("feat: a", "a.go") + r.push() + + lines[1] = "first by hand" + r.humanWrite("a.go", content(lines)) + r.commit("fix: first", "a.go") + require.Contains(t, r.push(), lineSession) + + runGit(t, r.dir, "mv", "a.go", "b.go") + r.commit("refactor: rename", "b.go") + r.push() + + lines[1] = "second by hand" + r.humanWrite("b.go", content(lines)) + r.commit("fix: second", "b.go") + second := headSHA(t, r.dir) + + got := r.push() + require.Contains(t, got, lineSession, "the session is sent again") + assert.Equal(t, lineSplit{aiAdded: 4, humanAdded: 1}, fileSplitOf(t, got[lineSession].changes, "a.go")) + assert.Contains(t, got[lineSession].commits, second) +} + +// failingRenamesClient fails to read the renames. +type failingRenamesClient struct { + tracegit.Client +} + +func (failingRenamesClient) Renames(string, string, string) (map[string]string, error) { + return nil, errors.New("read failed") +} + +// A commit that changes a renamed file, when the renames cannot be read, is +// reported, so that the push keeps it untracked and reads it again. +func TestScanReplacements_ReportsFailedRenames(t *testing.T) { + r := newLineRepo(t) + + r.agentWrite(lineSession, "a.go", content(numbered("ai", 3))) + r.commit("feat: a", "a.go") + r.push() + runGit(t, r.dir, "mv", "a.go", "b.go") + r.commit("refactor: rename", "b.go") + r.push() + r.humanWrite("b.go", "changed\n") + r.commit("fix: by hand", "b.go") + human := headSHA(t, r.dir) + + records, err := r.store.LoadAllCommitRecords() + require.NoError(t, err) + client := failingRenamesClient{Client: tracegit.NewGitClient()} + _, failed := scanReplacements(client, r.store, r.dir, records, loadLedgers(r.store, zerolog.Nop()), zerolog.Nop()) + + assert.Equal(t, map[string]bool{human: true}, failed) +} diff --git a/app/cli/pkg/action/trace_replacements.go b/app/cli/pkg/action/trace_replacements.go index c1f31f609..9741f51ff 100644 --- a/app/cli/pkg/action/trace_replacements.go +++ b/app/cli/pkg/action/trace_replacements.go @@ -16,6 +16,7 @@ package action import ( + "fmt" "reflect" "slices" "sort" @@ -126,8 +127,10 @@ type replacementScan struct { // paths of the ledgers, and the paths that a commit renamed them to. ledgerFiles map[string]bool // pushedRenames is set once ledgerFiles holds the paths that pushed - // commits renamed the ledger files to. + // commits renamed the ledger files to. renamesErr is set when the renames + // could not be read. pushedRenames bool + renamesErr error lines map[string][]tracegit.FileLines blames map[[2]string][]tracegit.BlameLine log zerolog.Logger @@ -137,21 +140,30 @@ type replacementScan struct { // session. A rename in a commit that a push already read is not seen in the // commits that the scan reads, so on the first file that is not a ledger // file, the paths at HEAD of the files of the pushed session commits are -// added. -func (s *replacementScan) holdsSessionLines(path string) bool { - if s.ledgerFiles[path] || s.pushedRenames { - return s.ledgerFiles[path] +// added. An error means the renames could not be read, so the commit must be +// read again on the next push. +func (s *replacementScan) holdsSessionLines(path string) (bool, error) { + if s.ledgerFiles[path] { + return true, nil } - s.pushedRenames = true + if !s.pushedRenames { + s.pushedRenames = true + s.renamesErr = s.addPushedRenames() + } + + return s.ledgerFiles[path], s.renamesErr +} +// addPushedRenames adds to ledgerFiles the paths at HEAD of the ledger files +// that the pushed session commits changed. +func (s *replacementScan) addPushedRenames() error { for _, r := range s.records { if !r.Tracked || len(r.AllSessionIDs()) == 0 { continue } renames, err := s.client.Renames(s.repoRoot, r.SHA, "HEAD") if err != nil { - s.log.Debug().Err(err).Str("sha", r.SHA).Msg("could not read the renames since a commit") - continue + return fmt.Errorf("read the renames since %s: %w", r.SHA, err) } for from, to := range renames { if s.ledgerFiles[from] { @@ -160,7 +172,7 @@ func (s *replacementScan) holdsSessionLines(path string) bool { } } - return s.ledgerFiles[path] + return nil } // replacedSessions adds to affected each session that owns a line that the @@ -191,7 +203,14 @@ func (s *replacementScan) replacedSessions(rec *state.CommitRecord, affected map } // A root commit removes nothing, so its parent is never blamed. - if len(fl.Removed) == 0 || !s.holdsSessionLines(parentPath) { + if len(fl.Removed) == 0 { + continue + } + holds, err := s.holdsSessionLines(parentPath) + if err != nil { + return nil, err + } + if !holds { continue } @@ -208,7 +227,7 @@ func (s *replacementScan) replacedSessions(rec *state.CommitRecord, affected map } for _, h := range fl.Hunks { - for _, sid := range hunkOwners(h, blame, s.bySHA, rec.SHA, parentPath) { + for _, sid := range hunkOwners(h, blame, s.bySHA, rec.SHA) { affected[sid] = struct{}{} if len(rec.SessionIDs) > 0 { continue @@ -233,8 +252,10 @@ func (s *replacementScan) replacedSessions(rec *state.CommitRecord, affected map // hunkOwners returns the sessions that own a line that the hunk removed, by // the record of the commit that last wrote the line. A commit owns its line // for each of its sessions, and for each session it joined when its -// replacing hunks added that line. -func hunkOwners(h tracegit.Hunk, blame []tracegit.BlameLine, bySHA map[string]*state.CommitRecord, self, path string) []string { +// replacing hunks added that line. The join holds the line under the path of +// the file at that commit, which a later commit can have renamed, so the +// line is looked up in each file of the join. +func hunkOwners(h tracegit.Hunk, blame []tracegit.BlameLine, bySHA map[string]*state.CommitRecord, self string) []string { var owners []string for i, text := range h.Removed { idx := h.OldStart - 1 + i @@ -253,8 +274,14 @@ func hunkOwners(h tracegit.Hunk, blame []tracegit.BlameLine, bySHA map[string]*s } fp := attribution.Fingerprint(text) for sid, files := range owner.Joined { - if slices.Contains(files[path], fp) && !slices.Contains(owners, sid) { - owners = append(owners, sid) + if slices.Contains(owners, sid) { + continue + } + for _, fps := range files { + if slices.Contains(fps, fp) { + owners = append(owners, sid) + break + } } } }