From 828d90ac4df6007b369f0836f5b1b42e4a28f290 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 28 Sep 2026 01:50:01 +0200 Subject: [PATCH 1/3] fix(cli): keep trace sessions when a commit is amended When you amend an AI-assisted commit with a new message, the new commit did not get the Chainloop-Trace-Sessions trailer, and the post-commit hook recorded it with no sessions. The pre-push hook then dropped the original commit record and skipped the attestation. - The commit-msg hook copies the trailer of HEAD when nothing is staged, because the new commit can then only replace HEAD. - A new post-rewrite hook copies the sessions of each rewritten commit to the record of the commit that replaces it (amend and rebase). Fixes PFM-7437 Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: d7cf52a4-d678-4f0c-907c-077d60025375 --- app/cli/cmd/trace_hook.go | 17 ++ app/cli/internal/trace/hooks/hooks.go | 3 +- app/cli/internal/trace/hooks/hooks_test.go | 10 +- app/cli/pkg/action/trace_hook_handler.go | 134 +++++++++++- app/cli/pkg/action/trace_hook_handler_test.go | 192 ++++++++++++++++++ 5 files changed, 344 insertions(+), 12 deletions(-) diff --git a/app/cli/cmd/trace_hook.go b/app/cli/cmd/trace_hook.go index 45fe24941..af2ed6658 100644 --- a/app/cli/cmd/trace_hook.go +++ b/app/cli/cmd/trace_hook.go @@ -51,6 +51,7 @@ func newTraceHookGitCmd() *cobra.Command { cmd.AddCommand( newTraceHookGitCommitMsgCmd(), newTraceHookGitPostCommitCmd(), + newTraceHookGitPostRewriteCmd(), newTraceHookGitPrePushCmd(), ) @@ -86,6 +87,22 @@ func newTraceHookGitPostCommitCmd() *cobra.Command { } } +func newTraceHookGitPostRewriteCmd() *cobra.Command { + return &cobra.Command{ + Use: "post-rewrite", + Short: "Handle the post-rewrite git hook", + Annotations: map[string]string{ + "skipActionOptsInit": "true", + }, + RunE: func(cmd *cobra.Command, _ []string) error { + cleanup := InitHookLogger() + defer cleanup() + + return action.HandlePostRewriteHook(cmd.Context(), logger) + }, + } +} + func newTraceHookGitPrePushCmd() *cobra.Command { return &cobra.Command{ Use: "pre-push", diff --git a/app/cli/internal/trace/hooks/hooks.go b/app/cli/internal/trace/hooks/hooks.go index e2027e996..2f8eb1542 100644 --- a/app/cli/internal/trace/hooks/hooks.go +++ b/app/cli/internal/trace/hooks/hooks.go @@ -34,6 +34,7 @@ const ( var managedHooks = []hookDef{ {"commit-msg", true}, {"post-commit", false}, + {"post-rewrite", false}, {"pre-push", false}, } @@ -117,7 +118,7 @@ func hookContentWithChain(hookCmd, backupPath string, passArgs bool) string { // Hooks are always written to the *common* hooks dir, since git only reads // hooks/ from the shared .git/ even when invoked from a linked worktree. // -// Pass skipPrePush=true to install only commit-msg and post-commit; used +// Pass skipPrePush=true to install every hook except pre-push; used // by trace run, which drives the attestation push itself. func Install(gitDir string, skipPrePush bool) (string, error) { hooksDir, err := resolveHooksDir(gitDir) diff --git a/app/cli/internal/trace/hooks/hooks_test.go b/app/cli/internal/trace/hooks/hooks_test.go index eaa77828e..b651fdb22 100644 --- a/app/cli/internal/trace/hooks/hooks_test.go +++ b/app/cli/internal/trace/hooks/hooks_test.go @@ -32,7 +32,7 @@ func TestInstall(t *testing.T) { _, err := Install(gitDir, false) require.NoError(t, err) - for _, name := range []string{"commit-msg", "post-commit", "pre-push"} { + for _, name := range []string{"commit-msg", "post-commit", "post-rewrite", "pre-push"} { hookPath := filepath.Join(gitDir, "hooks", name) content, err := os.ReadFile(hookPath) require.NoError(t, err) @@ -49,8 +49,8 @@ func TestInstall(t *testing.T) { require.NoError(t, err) assert.Contains(t, string(commitMsgContent), `"$@"`) - // post-commit and pre-push must NOT forward arguments - for _, name := range []string{"post-commit", "pre-push"} { + // post-commit, post-rewrite and pre-push must NOT forward arguments + for _, name := range []string{"post-commit", "post-rewrite", "pre-push"} { content, err := os.ReadFile(filepath.Join(gitDir, "hooks", name)) require.NoError(t, err) assert.NotContains(t, string(content), `"$@"`) @@ -135,13 +135,13 @@ func TestInstall(t *testing.T) { } func TestInstallSkipPrePush(t *testing.T) { - t.Run("installs commit-msg and post-commit only", func(t *testing.T) { + t.Run("installs every hook except pre-push", func(t *testing.T) { gitDir := t.TempDir() _, err := Install(gitDir, true) require.NoError(t, err) - for _, name := range []string{"commit-msg", "post-commit"} { + for _, name := range []string{"commit-msg", "post-commit", "post-rewrite"} { content, err := os.ReadFile(filepath.Join(gitDir, "hooks", name)) require.NoError(t, err, "hook %s should be installed", name) assert.Contains(t, string(content), HookMarker) diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 375dc7e86..089cefc71 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "sort" "strings" @@ -62,6 +63,33 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { return nil } + client := tracegit.NewGoGitClient() + stagedFiles, err := client.StagedFiles(repoRoot) + if err != nil { + return fmt.Errorf("get staged files: %w", err) + } + + // With nothing staged, the new commit has HEAD's tree, so it can only be + // an amend of HEAD (or an --allow-empty commit). Git gives the hook no + // other sign of an amend done with -m, and that message replaces HEAD's + // trailer. The sessions cannot be matched again because HEAD already + // consumed their pending ranges, so HEAD's trailer is copied instead. + if len(stagedFiles) == 0 { + _, headMsg, err := client.CommitHeadInfo(repoRoot) + if err != nil { + return fmt.Errorf("get HEAD commit: %w", err) + } + + sessionIDs := state.ParseSessionIDsFromTrailer(headMsg) + if len(sessionIDs) == 0 { + return nil + } + + log.Debug().Strs("session_ids", sessionIDs).Msg("nothing staged, keeping the trace sessions trailer of HEAD") + + return appendTrailer(msgFilePath, sessionIDs) + } + attrs, err := state.NewGitStore(gitDir).LoadAllAILineAttributions() if err != nil { return fmt.Errorf("load AI line attributions: %w", err) @@ -70,12 +98,6 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { return nil } - client := tracegit.NewGoGitClient() - stagedFiles, err := client.StagedFiles(repoRoot) - if err != nil { - return fmt.Errorf("get staged files: %w", err) - } - sessionIDs := matchSessionsToFiles(attrs, stagedFiles) if len(sessionIDs) == 0 { return nil @@ -253,6 +275,106 @@ func handlePostCommit(_ context.Context, log zerolog.Logger) error { return nil } +// HandlePostRewriteHook handles the post-rewrite git hook, which git runs after +// `commit --amend` and `rebase` with one " [extra]" line per +// rewritten commit on stdin. +// Errors are logged but never returned (to avoid blocking the rewrite). +func HandlePostRewriteHook(_ context.Context, log zerolog.Logger) error { + if err := handlePostRewrite(os.Stdin, log); err != nil { + log.Debug().Err(err).Msg("post-rewrite hook failed") + } + + return nil +} + +// handlePostRewrite copies the session IDs of each rewritten commit onto the +// record of the commit that replaced it. Without this, an amend that replaces +// the message loses the trailer, and post-commit cannot derive the sessions +// again because the original commit already consumed their pending ranges. The +// old record is then dropped as an orphan at push time, taking the sessions +// with it. +// +// Only records that post-commit already wrote are updated: the rewrite hook +// carries sessions forward, it does not decide which commits are recorded. +func handlePostRewrite(r io.Reader, log zerolog.Logger) error { + gitDir, _, err := tracegit.FindGitDirAndRoot() + if err != nil { + return err + } + + log.Debug().Str("git_dir", gitDir).Msg("post-rewrite hook invoked") + + store := state.NewGitStore(gitDir) + records, err := store.LoadAllCommitRecords() + if err != nil { + return fmt.Errorf("load commit records: %w", err) + } + + bySHA := make(map[string]*state.CommitRecord, len(records)) + for _, rec := range records { + bySHA[rec.SHA] = rec + } + + // A squash maps several old commits onto one new commit, so collect the + // sessions of all of them before writing. + inherited := make(map[string][]string) + scanner := bufio.NewScanner(r) + for scanner.Scan() { + fields := strings.Fields(scanner.Text()) + if len(fields) < 2 { + continue + } + + if old, ok := bySHA[fields[0]]; ok && len(old.SessionIDs) > 0 { + inherited[fields[1]] = append(inherited[fields[1]], old.SessionIDs...) + } + } + if err := scanner.Err(); err != nil { + return fmt.Errorf("read rewritten commits: %w", err) + } + + for newSHA, sessionIDs := range inherited { + rec, ok := bySHA[newSHA] + if !ok { + log.Debug().Str("sha", newSHA).Msg("no record for rewritten commit, skipping") + continue + } + + merged := mergeSessionIDs(rec.SessionIDs, sessionIDs) + if len(merged) == len(rec.SessionIDs) { + continue + } + + log.Debug().Str("sha", newSHA).Strs("session_ids", merged).Msg("carrying sessions over to rewritten commit") + + rec.SessionIDs = merged + if err := store.SaveCommitRecord(rec); err != nil { + return fmt.Errorf("save commit record %s: %w", newSHA, err) + } + } + + return nil +} + +// mergeSessionIDs returns the sorted union of a and b without duplicates. +func mergeSessionIDs(a, b []string) []string { + seen := make(map[string]struct{}, len(a)+len(b)) + out := make([]string, 0, len(a)+len(b)) + for _, list := range [][]string{a, b} { + for _, id := range list { + if _, dup := seen[id]; dup { + continue + } + seen[id] = struct{}{} + out = append(out, id) + } + } + + sort.Strings(out) + + return out +} + // deriveSessionIDsForCommit scans ai-lines data and returns session IDs // that have uncommitted attribution for any file in the given commit. func deriveSessionIDsForCommit(client tracegit.Client, store *state.Store, repoRoot, headSHA string) []string { diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 157b009a3..1f6400f08 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -762,6 +762,198 @@ func TestRebaseLifecycle(t *testing.T) { assert.Equal(t, newSHA, filtered[0].SHA) } +// Amending an AI-assisted commit with a new message replaces the trailer, and +// post-commit finds nothing pending because the first commit already consumed +// the session's ranges. The commit-msg hook copies the trailer of the replaced +// commit, and the post-rewrite hook carries its sessions over to the local +// record, so the pre-push still attests them. +func TestCommitLifecycle_AmendKeepsSessions(t *testing.T) { + dir, gitDir := initGitRepo(t) + store := state.NewGitStore(gitDir) + require.NoError(t, store.InitTraceDir()) + runGit(t, dir, "update-ref", "refs/remotes/origin/main", "HEAD") + + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n"), 0600)) + require.NoError(t, store.RecordLineRanges("sess-1", "a.go", []aicodingsession.LineRange{{Start: 1, End: 1}})) + + commit := func(t *testing.T, subject string, extra ...string) { + t.Helper() + msg := filepath.Join(dir, "msg") + require.NoError(t, os.WriteFile(msg, []byte(subject+"\n"), 0600)) + require.NoError(t, handleCommitMsg(msg, zerolog.Nop())) + runGit(t, dir, append([]string{"commit", "-F", msg}, extra...)...) + require.NoError(t, handlePostCommit(t.Context(), zerolog.Nop())) + } + + runGit(t, dir, "add", "a.go") + commit(t, "fix: first message") + oldSHA := headSHA(t, dir) + require.Equal(t, []string{"sess-1"}, mustLoadRecord(t, store, oldSHA).SessionIDs) + + commit(t, "fix: second message", "--amend") + newSHA := headSHA(t, dir) + require.NotEqual(t, oldSHA, newSHA) + + require.NoError(t, handlePostRewrite(strings.NewReader(oldSHA+" "+newSHA+"\n"), zerolog.Nop())) + + rec := mustLoadRecord(t, store, newSHA) + assert.Equal(t, []string{"sess-1"}, rec.SessionIDs, "the amended commit must keep the replaced commit's session") + assert.Equal(t, "fix: second message\n\nChainloop-Trace-Sessions: sess-1", rec.Message, + "the trailer must survive in the history, not only in the local record") + + all, err := store.LoadAllCommitRecords() + require.NoError(t, err) + filtered := filterCurrentBranchCommits(tracegit.NewGoGitClient(), dir, all, zerolog.Nop()) + require.Len(t, filtered, 1) + assert.Equal(t, newSHA, filtered[0].SHA) +} + +// When nothing is staged, the only commit git can make is an amend of HEAD +// (or an --allow-empty one), so commit-msg copies HEAD's trailer into the new +// message. Git gives the hook no other way to see an amend done with -m. +func TestHandleCommitMsg_NothingStaged(t *testing.T) { + const amendMsg = "fix: second\n" + + cases := []struct { + name string + headMsg string + stage bool + msg string + want string + }{ + { + name: "copies the trailer of the commit being amended", + headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a, session-b\n", + msg: amendMsg, + want: amendMsg + "\nChainloop-Trace-Sessions: session-a, session-b\n", + }, + { + name: "leaves the message alone when HEAD has no trailer", + headMsg: "fix: first\n", + msg: amendMsg, + want: amendMsg, + }, + { + name: "keeps a trailer the user wrote", + headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a\n", + msg: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", + want: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", + }, + { + name: "does not copy HEAD's trailer onto a commit with staged changes", + headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a\n", + stage: true, + msg: "fix: unrelated\n", + want: "fix: unrelated\n", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + dir, gitDir := initGitRepo(t) + require.NoError(t, state.NewGitStore(gitDir).InitTraceDir()) + + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n"), 0600)) + runGit(t, dir, "add", "a.go") + runGit(t, dir, "commit", "-m", tc.headMsg) + + if tc.stage { + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n\nfunc main() {}\n"), 0600)) + runGit(t, dir, "add", "a.go") + } + + msg := filepath.Join(dir, "msg") + require.NoError(t, os.WriteFile(msg, []byte(tc.msg), 0600)) + require.NoError(t, handleCommitMsg(msg, zerolog.Nop())) + + got, err := os.ReadFile(msg) + require.NoError(t, err) + assert.Equal(t, tc.want, string(got)) + }) + } +} + +func TestHandlePostRewrite(t *testing.T) { + const ( + oldSHA = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + otherSHA = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + newSHA = "cccccccccccccccccccccccccccccccccccccccc" + unknownSHA = "dddddddddddddddddddddddddddddddddddddddd" + ) + + cases := []struct { + name string + records []*state.CommitRecord + input string + want []string + noRecord bool + }{ + { + name: "amend carries the old sessions over", + records: []*state.CommitRecord{ + {SHA: oldSHA, SessionIDs: []string{sessionA}}, + {SHA: newSHA}, + }, + input: oldSHA + " " + newSHA + "\n", + want: []string{sessionA}, + }, + { + name: "squash unions the sessions of every replaced commit", + records: []*state.CommitRecord{ + {SHA: oldSHA, SessionIDs: []string{sessionB}}, + {SHA: otherSHA, SessionIDs: []string{sessionA}}, + {SHA: newSHA, SessionIDs: []string{sessionB}}, + }, + input: oldSHA + " " + newSHA + "\n" + otherSHA + " " + newSHA + " extra-info\n", + want: []string{sessionA, sessionB}, + }, + { + name: "unknown old commit leaves the new record alone", + records: []*state.CommitRecord{ + {SHA: newSHA, SessionIDs: []string{sessionA}}, + }, + input: unknownSHA + " " + newSHA + "\n", + want: []string{sessionA}, + }, + { + name: "no record for the new commit is not created", + records: []*state.CommitRecord{ + {SHA: oldSHA, SessionIDs: []string{sessionA}}, + }, + input: oldSHA + " " + newSHA + "\n", + noRecord: true, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, gitDir := initGitRepo(t) + store := state.NewGitStore(gitDir) + require.NoError(t, store.InitTraceDir()) + for _, r := range tc.records { + require.NoError(t, store.SaveCommitRecord(r)) + } + + require.NoError(t, handlePostRewrite(strings.NewReader(tc.input), zerolog.Nop())) + + all, err := store.LoadAllCommitRecords() + require.NoError(t, err) + var got *state.CommitRecord + for _, r := range all { + if r.SHA == newSHA { + got = r + } + } + if tc.noRecord { + assert.Nil(t, got) + return + } + require.NotNil(t, got) + assert.Equal(t, tc.want, got.SessionIDs) + }) + } +} + // headSHA returns the SHA of HEAD in dir. func headSHA(t *testing.T, dir string) string { t.Helper() From 35b350688db8d280e694818de21215211c963344 Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 28 Sep 2026 01:56:35 +0200 Subject: [PATCH 2/3] fix(cli): do not copy the HEAD trailer when git uses a temporary index For `git commit -a` and `git commit `, git builds the commit from a temporary index and names it in GIT_INDEX_FILE. The regular index then shows nothing staged, so the commit-msg hook copied the trailer of HEAD onto a commit that is not an amend. Copy the trailer only when git commits the regular index. Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: d7cf52a4-d678-4f0c-907c-077d60025375 --- app/cli/pkg/action/trace_hook_handler.go | 11 +++- app/cli/pkg/action/trace_hook_handler_test.go | 58 +++++++++++++++---- 2 files changed, 57 insertions(+), 12 deletions(-) diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 089cefc71..103154106 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -74,7 +74,7 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { // other sign of an amend done with -m, and that message replaces HEAD's // trailer. The sessions cannot be matched again because HEAD already // consumed their pending ranges, so HEAD's trailer is copied instead. - if len(stagedFiles) == 0 { + if len(stagedFiles) == 0 && commitsRegularIndex() { _, headMsg, err := client.CommitHeadInfo(repoRoot) if err != nil { return fmt.Errorf("get HEAD commit: %w", err) @@ -108,6 +108,15 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { return appendTrailer(msgFilePath, sessionIDs) } +// commitsRegularIndex reports whether git builds the commit from the regular +// index, which is the one StagedFiles reads. For `git commit -a` and +// `git commit `, git builds the commit from a temporary index +// (index.lock or next-index-*.lock) and names it in GIT_INDEX_FILE. The +// regular index can then show nothing staged for a commit that changes files. +func commitsRegularIndex() bool { + return !strings.HasSuffix(os.Getenv("GIT_INDEX_FILE"), ".lock") +} + // fileOwner is the session currently credited with a staged file's pending // attribution, and when it made the edit. type fileOwner struct { diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 1f6400f08..68ecbc1d2 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -808,18 +808,24 @@ func TestCommitLifecycle_AmendKeepsSessions(t *testing.T) { assert.Equal(t, newSHA, filtered[0].SHA) } -// When nothing is staged, the only commit git can make is an amend of HEAD -// (or an --allow-empty one), so commit-msg copies HEAD's trailer into the new -// message. Git gives the hook no other way to see an amend done with -m. +// When git commits the index as it is and nothing is staged, the only commit +// git can make is an amend of HEAD (or an --allow-empty one), so commit-msg +// copies HEAD's trailer into the new message. Git gives the hook no other way +// to see an amend done with -m. func TestHandleCommitMsg_NothingStaged(t *testing.T) { - const amendMsg = "fix: second\n" + const ( + amendMsg = "fix: second\n" + headWithA = "fix: first\n\nChainloop-Trace-Sessions: session-a\n" + trailerOfA = "\nChainloop-Trace-Sessions: session-a\n" + ) cases := []struct { - name string - headMsg string - stage bool - msg string - want string + name string + headMsg string + stage bool + indexFile string + msg string + want string }{ { name: "copies the trailer of the commit being amended", @@ -827,6 +833,29 @@ func TestHandleCommitMsg_NothingStaged(t *testing.T) { msg: amendMsg, want: amendMsg + "\nChainloop-Trace-Sessions: session-a, session-b\n", }, + { + name: "copies the trailer when git names the regular index", + headMsg: headWithA, + indexFile: ".git/index", + msg: amendMsg, + want: amendMsg + trailerOfA, + }, + { + // git commit -a (and --amend -a) commits from index.lock, which + // the hook cannot compare against HEAD. + name: "does not copy HEAD's trailer onto git commit -a", + headMsg: headWithA, + indexFile: "index.lock", + msg: amendMsg, + want: amendMsg, + }, + { + name: "does not copy HEAD's trailer onto git commit ", + headMsg: headWithA, + indexFile: "next-index-123.lock", + msg: amendMsg, + want: amendMsg, + }, { name: "leaves the message alone when HEAD has no trailer", headMsg: "fix: first\n", @@ -835,13 +864,13 @@ func TestHandleCommitMsg_NothingStaged(t *testing.T) { }, { name: "keeps a trailer the user wrote", - headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a\n", + headMsg: headWithA, msg: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", want: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", }, { name: "does not copy HEAD's trailer onto a commit with staged changes", - headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a\n", + headMsg: headWithA, stage: true, msg: "fix: unrelated\n", want: "fix: unrelated\n", @@ -862,6 +891,13 @@ func TestHandleCommitMsg_NothingStaged(t *testing.T) { runGit(t, dir, "add", "a.go") } + // Set after the git calls above, so they use the regular index. + indexFile := tc.indexFile + if strings.HasSuffix(indexFile, ".lock") { + indexFile = filepath.Join(gitDir, indexFile) + } + t.Setenv("GIT_INDEX_FILE", indexFile) + msg := filepath.Join(dir, "msg") require.NoError(t, os.WriteFile(msg, []byte(tc.msg), 0600)) require.NoError(t, handleCommitMsg(msg, zerolog.Nop())) From 8775880ff89efd499b6abc555bcf460f8e3b2b7e Mon Sep 17 00:00:00 2001 From: "Jose I. Paris" Date: Mon, 28 Sep 2026 02:09:03 +0200 Subject: [PATCH 3/3] fix(cli): detect an amend from the author date in the commit-msg hook The commit-msg hook copied the trailer of HEAD when nothing was staged. This also matched `git commit --allow-empty`, and it missed amends with staged changes. During an amend, git exports the author date of HEAD in GIT_AUTHOR_DATE. Other commits carry the current time. The hook now compares this date with the author date of HEAD, and joins the sessions of HEAD with the sessions of the staged AI edits. The post-rewrite hook still carries the sessions for amends that change the author date. Also extend the hook tests to the post-rewrite hook, and make the amend lifecycle test exercise the post-rewrite hook. Assisted-by: Claude Code Signed-off-by: Jose I. Paris Chainloop-Trace-Sessions: d7cf52a4-d678-4f0c-907c-077d60025375 --- app/cli/internal/trace/git/exec.go | 12 ++ app/cli/internal/trace/git/git.go | 4 + app/cli/internal/trace/git/gogit.go | 20 +++ .../internal/trace/git/merge_branch_test.go | 29 ++++ app/cli/internal/trace/hooks/hooks_test.go | 6 +- app/cli/pkg/action/trace_hook_handler.go | 74 ++++++---- app/cli/pkg/action/trace_hook_handler_test.go | 135 +++++++++++------- 7 files changed, 193 insertions(+), 87 deletions(-) diff --git a/app/cli/internal/trace/git/exec.go b/app/cli/internal/trace/git/exec.go index 4d49eecb0..70be48ef0 100644 --- a/app/cli/internal/trace/git/exec.go +++ b/app/cli/internal/trace/git/exec.go @@ -20,6 +20,7 @@ import ( "os/exec" "strconv" "strings" + "time" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials/aicodingsession" ) @@ -277,6 +278,17 @@ func (c *ExecClient) CommitHeadInfo(repoRoot string) (sha, message string, err e return sha, strings.TrimSpace(message), nil } +// HeadAuthorTime returns the author date of HEAD. +func (c *ExecClient) HeadAuthorTime(repoRoot string) (time.Time, error) { + out := gitOutput(repoRoot, "log", "-1", "--format=%at", "HEAD") + secs, err := strconv.ParseInt(out, 10, 64) + if err != nil { + return time.Time{}, fmt.Errorf("git log HEAD author date %q: %w", out, err) + } + + return time.Unix(secs, 0), nil +} + // StagedFiles returns repo-relative paths of files staged for the current commit. func (c *ExecClient) StagedFiles(repoRoot string) ([]string, error) { out := gitOutput(repoRoot, "diff", "--cached", "--name-only") diff --git a/app/cli/internal/trace/git/git.go b/app/cli/internal/trace/git/git.go index 696b93663..0652ef80f 100644 --- a/app/cli/internal/trace/git/git.go +++ b/app/cli/internal/trace/git/git.go @@ -16,6 +16,8 @@ package git import ( + "time" + "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials/aicodingsession" ) @@ -48,6 +50,8 @@ type Client interface { CodeChangesForCommits(repoRoot string, shas []string) (*aicodingsession.CodeChanges, error) // CommitHeadInfo returns the SHA and message of HEAD. CommitHeadInfo(repoRoot string) (sha, message string, err error) + // HeadAuthorTime returns the author date of HEAD. + HeadAuthorTime(repoRoot string) (time.Time, error) // GeneratedMatcher returns a predicate that reports whether a repo-relative // path is marked linguist-generated=true in the repository's .gitattributes. // Implementations must return a no-op matcher (always false) when attributes diff --git a/app/cli/internal/trace/git/gogit.go b/app/cli/internal/trace/git/gogit.go index 1d446531c..f7f41b0ee 100644 --- a/app/cli/internal/trace/git/gogit.go +++ b/app/cli/internal/trace/git/gogit.go @@ -25,6 +25,7 @@ import ( "path/filepath" "sort" "strings" + "time" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials/aicodingsession" "github.com/go-git/go-billy/v6/osfs" @@ -198,6 +199,25 @@ func (c *GoGitClient) CommitHeadInfo(repoRoot string) (sha, message string, err return head.Hash().String(), strings.TrimSpace(commit.Message), nil } +func (c *GoGitClient) HeadAuthorTime(repoRoot string) (time.Time, error) { + repo, err := openRepo(repoRoot) + if err != nil { + return time.Time{}, fmt.Errorf("open repo: %w", err) + } + + head, err := repo.Head() + if err != nil { + return time.Time{}, fmt.Errorf("get HEAD: %w", err) + } + + commit, err := repo.CommitObject(head.Hash()) + if err != nil { + return time.Time{}, fmt.Errorf("get commit: %w", err) + } + + return commit.Author.When, nil +} + // GeneratedMatcher returns a predicate that reports whether a repo-relative path // is marked linguist-generated=true in the repository's .gitattributes, parsed // in-process via go-git's gitattributes parser. diff --git a/app/cli/internal/trace/git/merge_branch_test.go b/app/cli/internal/trace/git/merge_branch_test.go index a7a14330d..f14e173f4 100644 --- a/app/cli/internal/trace/git/merge_branch_test.go +++ b/app/cli/internal/trace/git/merge_branch_test.go @@ -105,6 +105,35 @@ func setupBranchedRepo(t *testing.T) branchedRepo { } } +func TestHeadAuthorTime(t *testing.T) { + r := setupBranchedRepo(t) + + // Give HEAD a known author date. + cmd := exec.Command("git", "commit", "-q", "--amend", "--no-edit", "--date=@1700000000 +0200") + cmd.Dir = r.dir + out, err := cmd.CombinedOutput() + require.NoError(t, err, string(out)) + + clients := []struct { + name string + client Client + }{ + {"go-git", NewGoGitClient()}, + {"exec", NewExecClient()}, + } + + for _, tc := range clients { + t.Run(tc.name, func(t *testing.T) { + got, err := tc.client.HeadAuthorTime(r.dir) + require.NoError(t, err) + assert.Equal(t, int64(1700000000), got.Unix()) + + _, err = tc.client.HeadAuthorTime(t.TempDir()) + assert.Error(t, err, "a directory without a repository has no HEAD") + }) + } +} + func TestGoGitClient_IsMergeCommit(t *testing.T) { repo := setupBranchedRepo(t) c := NewGoGitClient() diff --git a/app/cli/internal/trace/hooks/hooks_test.go b/app/cli/internal/trace/hooks/hooks_test.go index b651fdb22..b1ffdde7f 100644 --- a/app/cli/internal/trace/hooks/hooks_test.go +++ b/app/cli/internal/trace/hooks/hooks_test.go @@ -122,7 +122,7 @@ func TestInstall(t *testing.T) { assert.Equal(t, filepath.Join(commonDir, "hooks"), hooksDir) // Hooks must land under the common dir, not the worktree-private dir. - for _, name := range []string{"commit-msg", "post-commit", "pre-push"} { + for _, name := range []string{"commit-msg", "post-commit", "post-rewrite", "pre-push"} { content, err := os.ReadFile(filepath.Join(commonDir, "hooks", name)) require.NoError(t, err, "hook %s should exist in common .git/hooks", name) assert.Contains(t, string(content), HookMarker) @@ -196,7 +196,7 @@ func TestHookScriptsExitZero(t *testing.T) { _, err := Install(gitDir, false) require.NoError(t, err) - for _, name := range []string{"commit-msg", "post-commit", "pre-push"} { + for _, name := range []string{"commit-msg", "post-commit", "post-rewrite", "pre-push"} { //nolint:gosec // the hook path is derived from t.TempDir(), not from user input cmd := exec.Command("/bin/sh", filepath.Join(hooksDir, name), "msgfile") cmd.Env = []string{"PATH="} @@ -214,7 +214,7 @@ func TestUninstall(t *testing.T) { _, err = Uninstall(gitDir) require.NoError(t, err) - for _, name := range []string{"commit-msg", "post-commit", "pre-push"} { + for _, name := range []string{"commit-msg", "post-commit", "post-rewrite", "pre-push"} { _, err := os.Stat(filepath.Join(gitDir, "hooks", name)) assert.True(t, os.IsNotExist(err)) } diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 103154106..13008c810 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -23,6 +23,7 @@ import ( "io" "os" "sort" + "strconv" "strings" "time" @@ -37,7 +38,8 @@ import ( ) // HandleCommitMsgHook appends a Chainloop-Trace-Sessions trailer to the commit -// message when AI sessions have modified files staged for commit. +// message when AI sessions have modified files staged for commit, or when the +// commit amends one that has the trailer. // Errors are logged but never returned (to avoid blocking commits). func HandleCommitMsgHook(_ context.Context, msgFilePath string, log zerolog.Logger) error { if err := handleCommitMsg(msgFilePath, log); err != nil { @@ -64,41 +66,35 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { } client := tracegit.NewGoGitClient() - stagedFiles, err := client.StagedFiles(repoRoot) - if err != nil { - return fmt.Errorf("get staged files: %w", err) - } - // With nothing staged, the new commit has HEAD's tree, so it can only be - // an amend of HEAD (or an --allow-empty commit). Git gives the hook no - // other sign of an amend done with -m, and that message replaces HEAD's - // trailer. The sessions cannot be matched again because HEAD already - // consumed their pending ranges, so HEAD's trailer is copied instead. - if len(stagedFiles) == 0 && commitsRegularIndex() { + // An amend with a new message replaces HEAD's trailer, and its sessions + // cannot be matched again because HEAD already consumed their pending + // ranges. The amended commit still holds HEAD's lines, so HEAD's trailer + // is kept. + var sessionIDs []string + if isAmendOfHead(client, repoRoot) { _, headMsg, err := client.CommitHeadInfo(repoRoot) if err != nil { return fmt.Errorf("get HEAD commit: %w", err) } - sessionIDs := state.ParseSessionIDsFromTrailer(headMsg) - if len(sessionIDs) == 0 { - return nil - } - - log.Debug().Strs("session_ids", sessionIDs).Msg("nothing staged, keeping the trace sessions trailer of HEAD") - - return appendTrailer(msgFilePath, sessionIDs) + sessionIDs = state.ParseSessionIDsFromTrailer(headMsg) + log.Debug().Strs("session_ids", sessionIDs).Msg("amending HEAD, keeping its trace sessions") } attrs, err := state.NewGitStore(gitDir).LoadAllAILineAttributions() if err != nil { return fmt.Errorf("load AI line attributions: %w", err) } - if len(attrs) == 0 { - return nil + if len(attrs) > 0 { + stagedFiles, err := client.StagedFiles(repoRoot) + if err != nil { + return fmt.Errorf("get staged files: %w", err) + } + + sessionIDs = mergeSessionIDs(sessionIDs, matchSessionsToFiles(attrs, stagedFiles)) } - sessionIDs := matchSessionsToFiles(attrs, stagedFiles) if len(sessionIDs) == 0 { return nil } @@ -108,13 +104,33 @@ func handleCommitMsg(msgFilePath string, log zerolog.Logger) error { return appendTrailer(msgFilePath, sessionIDs) } -// commitsRegularIndex reports whether git builds the commit from the regular -// index, which is the one StagedFiles reads. For `git commit -a` and -// `git commit `, git builds the commit from a temporary index -// (index.lock or next-index-*.lock) and names it in GIT_INDEX_FILE. The -// regular index can then show nothing staged for a commit that changes files. -func commitsRegularIndex() bool { - return !strings.HasSuffix(os.Getenv("GIT_INDEX_FILE"), ".lock") +// isAmendOfHead reports whether the commit being made replaces HEAD. Git gives +// the commit-msg hook no direct sign of an amend done with -m, but it exports +// the author date of the new commit in GIT_AUTHOR_DATE as "@ ". An +// amend keeps the author date of HEAD, and any other commit, --allow-empty +// included, carries the current time. +// +// An amend with --reset-author, or an author date that the user set in +// another format, is not seen here. The post-rewrite hook still carries the +// sessions over to the local record for those. +func isAmendOfHead(client tracegit.Client, repoRoot string) bool { + raw, ok := strings.CutPrefix(os.Getenv("GIT_AUTHOR_DATE"), "@") + if !ok { + return false + } + + secs, _, _ := strings.Cut(raw, " ") + authored, err := strconv.ParseInt(secs, 10, 64) + if err != nil { + return false + } + + headAuthored, err := client.HeadAuthorTime(repoRoot) + if err != nil { + return false + } + + return headAuthored.Unix() == authored } // fileOwner is the session currently credited with a staged file's pending diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 68ecbc1d2..ae9ae12e0 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -261,6 +261,7 @@ func (f *fakeBranchClient) CodeChangesForCommits(string, []string) (*aicodingses } func (f *fakeBranchClient) CommitHeadInfo(string) (string, string, error) { return "", "", nil } +func (f *fakeBranchClient) HeadAuthorTime(string) (time.Time, error) { return time.Time{}, nil } func (f *fakeBranchClient) GeneratedMatcher(string) func(string) bool { return func(string) bool { return false } } @@ -764,9 +765,10 @@ func TestRebaseLifecycle(t *testing.T) { // Amending an AI-assisted commit with a new message replaces the trailer, and // post-commit finds nothing pending because the first commit already consumed -// the session's ranges. The commit-msg hook copies the trailer of the replaced -// commit, and the post-rewrite hook carries its sessions over to the local -// record, so the pre-push still attests them. +// the session's ranges. When commit-msg cannot see the amend (the handlers run +// outside git here, so git exports no author date), the post-rewrite hook is +// what carries the sessions over to the local record, so the pre-push still +// attests them. func TestCommitLifecycle_AmendKeepsSessions(t *testing.T) { dir, gitDir := initGitRepo(t) store := state.NewGitStore(gitDir) @@ -790,16 +792,21 @@ func TestCommitLifecycle_AmendKeepsSessions(t *testing.T) { oldSHA := headSHA(t, dir) require.Equal(t, []string{"sess-1"}, mustLoadRecord(t, store, oldSHA).SessionIDs) + // The amend also stages a change that no session made. + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n\n// human edit\n"), 0600)) + runGit(t, dir, "add", "a.go") commit(t, "fix: second message", "--amend") newSHA := headSHA(t, dir) require.NotEqual(t, oldSHA, newSHA) + rec := mustLoadRecord(t, store, newSHA) + require.Equal(t, "fix: second message", rec.Message, "commit-msg must not have added a trailer") + require.Empty(t, rec.SessionIDs, "post-commit alone loses the session") + require.NoError(t, handlePostRewrite(strings.NewReader(oldSHA+" "+newSHA+"\n"), zerolog.Nop())) - rec := mustLoadRecord(t, store, newSHA) + rec = mustLoadRecord(t, store, newSHA) assert.Equal(t, []string{"sess-1"}, rec.SessionIDs, "the amended commit must keep the replaced commit's session") - assert.Equal(t, "fix: second message\n\nChainloop-Trace-Sessions: sess-1", rec.Message, - "the trailer must survive in the history, not only in the local record") all, err := store.LoadAllCommitRecords() require.NoError(t, err) @@ -808,80 +815,95 @@ func TestCommitLifecycle_AmendKeepsSessions(t *testing.T) { assert.Equal(t, newSHA, filtered[0].SHA) } -// When git commits the index as it is and nothing is staged, the only commit -// git can make is an amend of HEAD (or an --allow-empty one), so commit-msg -// copies HEAD's trailer into the new message. Git gives the hook no other way -// to see an amend done with -m. -func TestHandleCommitMsg_NothingStaged(t *testing.T) { +// During an amend git exports the author date of HEAD to the hooks, and +// commit-msg uses it to copy HEAD's trailer into the new message: the amended +// commit still holds HEAD's lines. Git gives the hook no other sign of an +// amend done with -m. Any other commit, --allow-empty included, carries the +// current time. +func TestHandleCommitMsg_Amend(t *testing.T) { const ( + headDate = "@1700000000 +0000" amendMsg = "fix: second\n" headWithA = "fix: first\n\nChainloop-Trace-Sessions: session-a\n" trailerOfA = "\nChainloop-Trace-Sessions: session-a\n" ) cases := []struct { - name string - headMsg string - stage bool - indexFile string - msg string - want string + name string + headMsg string + authorDate string // empty unsets GIT_AUTHOR_DATE + stage bool + aiEdit bool // stage an AI edit of session-b + msg string + want string }{ { - name: "copies the trailer of the commit being amended", - headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a, session-b\n", - msg: amendMsg, - want: amendMsg + "\nChainloop-Trace-Sessions: session-a, session-b\n", + name: "amend copies the trailer of HEAD", + headMsg: "fix: first\n\nChainloop-Trace-Sessions: session-a, session-b\n", + authorDate: headDate, + msg: amendMsg, + want: amendMsg + "\nChainloop-Trace-Sessions: session-a, session-b\n", }, { - name: "copies the trailer when git names the regular index", - headMsg: headWithA, - indexFile: ".git/index", - msg: amendMsg, - want: amendMsg + trailerOfA, + name: "amend with staged changes copies the trailer of HEAD", + headMsg: headWithA, + authorDate: headDate, + stage: true, + msg: amendMsg, + want: amendMsg + trailerOfA, }, { - // git commit -a (and --amend -a) commits from index.lock, which - // the hook cannot compare against HEAD. - name: "does not copy HEAD's trailer onto git commit -a", - headMsg: headWithA, - indexFile: "index.lock", - msg: amendMsg, - want: amendMsg, + name: "amend with a staged AI edit joins both sessions", + headMsg: headWithA, + authorDate: headDate, + stage: true, + aiEdit: true, + msg: amendMsg, + want: amendMsg + "\nChainloop-Trace-Sessions: session-a, session-b\n", }, { - name: "does not copy HEAD's trailer onto git commit ", - headMsg: headWithA, - indexFile: "next-index-123.lock", - msg: amendMsg, - want: amendMsg, + name: "allow-empty commit does not get the trailer of HEAD", + headMsg: headWithA, + authorDate: "@1790000000 +0200", + msg: amendMsg, + want: amendMsg, }, { - name: "leaves the message alone when HEAD has no trailer", - headMsg: "fix: first\n", + name: "no author date from git does not copy the trailer", + headMsg: headWithA, msg: amendMsg, want: amendMsg, }, { - name: "keeps a trailer the user wrote", - headMsg: headWithA, - msg: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", - want: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", + name: "author date in another format does not copy the trailer", + headMsg: headWithA, + authorDate: "2023-11-14T22:13:20Z", + msg: amendMsg, + want: amendMsg, }, { - name: "does not copy HEAD's trailer onto a commit with staged changes", - headMsg: headWithA, - stage: true, - msg: "fix: unrelated\n", - want: "fix: unrelated\n", + name: "amend of a commit with no trailer leaves the message alone", + headMsg: "fix: first\n", + authorDate: headDate, + msg: amendMsg, + want: amendMsg, + }, + { + name: "amend keeps a trailer the user wrote", + headMsg: headWithA, + authorDate: headDate, + msg: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", + want: "fix: second\n\nChainloop-Trace-Sessions: session-b\n", }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { dir, gitDir := initGitRepo(t) - require.NoError(t, state.NewGitStore(gitDir).InitTraceDir()) + store := state.NewGitStore(gitDir) + require.NoError(t, store.InitTraceDir()) + t.Setenv("GIT_AUTHOR_DATE", headDate) require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n"), 0600)) runGit(t, dir, "add", "a.go") runGit(t, dir, "commit", "-m", tc.headMsg) @@ -890,13 +912,16 @@ func TestHandleCommitMsg_NothingStaged(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(dir, "a.go"), []byte("package main\n\nfunc main() {}\n"), 0600)) runGit(t, dir, "add", "a.go") } + if tc.aiEdit { + require.NoError(t, store.RecordLineRanges(sessionB, "a.go", []aicodingsession.LineRange{{Start: 3, End: 3}})) + } - // Set after the git calls above, so they use the regular index. - indexFile := tc.indexFile - if strings.HasSuffix(indexFile, ".lock") { - indexFile = filepath.Join(gitDir, indexFile) + // Set after the git calls above, as git sets it for the hook. + if tc.authorDate == "" { + require.NoError(t, os.Unsetenv("GIT_AUTHOR_DATE")) + } else { + t.Setenv("GIT_AUTHOR_DATE", tc.authorDate) } - t.Setenv("GIT_INDEX_FILE", indexFile) msg := filepath.Join(dir, "msg") require.NoError(t, os.WriteFile(msg, []byte(tc.msg), 0600))