From e5328d22a12df9f019531d01e00f01c46356edcf Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 8 Oct 2026 00:11:11 +0200 Subject: [PATCH 1/2] fix(trace): make requireTrace block the push again The managed pre-push hook script ended with exit 0, so git ignored the exit status of chainloop trace hook git pre-push and requireTrace had no effect. The pre-push script now propagates chainloop's status (a missing binary still never fails a hook), and IsInstalled compares the full script so agent hooks reinstall outdated scripts automatically. Any error of the pre-push command, setup included, now blocks the push only when requireTrace is enabled; otherwise the hook warns with the cause and the push continues. The hook log file is now closed at process exit instead of when the hook command returns, so the final error line reaches log.txt and the "file already closed" message no longer shows on every hook run. Fixes PFM-7644 Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: a5941a7b-7e2e-46b4-b95d-cd9fadb00b3b --- app/cli/cmd/root.go | 1 + app/cli/cmd/trace_hook.go | 75 +++++----- app/cli/cmd/trace_hook_test.go | 65 +++++++++ app/cli/cmd/trace_hooklog.go | 34 +++-- app/cli/cmd/trace_hooklog_test.go | 29 ++++ app/cli/internal/trace/hooks/hooks.go | 92 ++++++++----- app/cli/internal/trace/hooks/hooks_test.go | 130 +++++++++++++++--- app/cli/main.go | 13 +- app/cli/pkg/action/trace_hook_handler.go | 29 ++-- app/cli/pkg/action/trace_hook_handler_test.go | 35 +++++ 10 files changed, 390 insertions(+), 113 deletions(-) create mode 100644 app/cli/cmd/trace_hook_test.go diff --git a/app/cli/cmd/root.go b/app/cli/cmd/root.go index 7c5d10922..2689382db 100644 --- a/app/cli/cmd/root.go +++ b/app/cli/cmd/root.go @@ -102,6 +102,7 @@ func Execute(rootCmd *cobra.Command) error { // every command, including the ones that fail and so skip cobra's post-run hooks. executed, err := rootCmd.ExecuteC() reportCommand(executed, time.Since(processStart), err) + err = applyPrePushPolicy(executed, err) if err != nil { // The local file is pointing to the wrong organization, we remove it diff --git a/app/cli/cmd/trace_hook.go b/app/cli/cmd/trace_hook.go index 182656d92..ed1ec5f1a 100644 --- a/app/cli/cmd/trace_hook.go +++ b/app/cli/cmd/trace_hook.go @@ -67,8 +67,7 @@ func newTraceHookGitCommitMsgCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(cmd *cobra.Command, args []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleCommitMsgHook(cmd.Context(), args[0], logger) }, @@ -80,8 +79,7 @@ func newTraceHookGitPostCommitCmd() *cobra.Command { Use: "post-commit", Short: "Handle the post-commit git hook", RunE: func(cmd *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandlePostCommitHook(cmd.Context(), logger) }, } @@ -95,25 +93,41 @@ func newTraceHookGitPostRewriteCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(cmd *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandlePostRewriteHook(cmd.Context(), logger) }, } } +// prePushHookAnnotation marks the pre-push hook command for applyPrePushPolicy. +const prePushHookAnnotation = "prePushHook" + +// applyPrePushPolicy applies requireTrace to any error of the pre-push hook +// command: setup, run and cleanup alike. The managed pre-push script +// propagates the command's exit status to git, so only this decides whether +// the push is blocked. +func applyPrePushPolicy(executed *cobra.Command, err error) error { + if err == nil || executed == nil || executed.Annotations[prePushHookAnnotation] != trueString { + return err + } + + return action.PrePushFailure(err, config.LoadRequireTraceFromYML("."), logger) +} + func newTraceHookGitPrePushCmd() *cobra.Command { return &cobra.Command{ Use: "pre-push", Short: "Handle the pre-push git hook", + // Any error of this command, setup included, goes through + // action.PrePushFailure in Execute. + Annotations: map[string]string{ + prePushHookAnnotation: trueString, + }, RunE: func(cmd *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() - - requireTrace := config.LoadRequireTraceFromYML(".") + InitHookLogger() - return action.HandlePrePushHook(cmd.Context(), requireTrace, logger, action.RunTracePushOpts{ + return action.HandlePrePushHook(cmd.Context(), logger, action.RunTracePushOpts{ ActionOpts: ActionOpts, CLIVersion: Version, Mode: aicodingsession.ModeCoding, @@ -147,8 +161,7 @@ func newTraceHookClaudeSessionStartCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionStart(claude.New(), logger) }, } @@ -162,8 +175,7 @@ func newTraceHookClaudeUserPromptSubmitCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPromptSubmit(claude.New(), logger) }, } @@ -177,8 +189,7 @@ func newTraceHookClaudeSessionEndCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionEnd(claude.New(), logger) }, } @@ -192,8 +203,7 @@ func newTraceHookClaudePreToolUseCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPreToolUse(claude.New(), logger) }, } @@ -207,8 +217,7 @@ func newTraceHookClaudePostToolUseCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPostToolUse(claude.New(), logger) }, } @@ -237,8 +246,7 @@ func newTraceHookCursorSessionStartCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionStart(cursor.New(), logger) }, } @@ -252,8 +260,7 @@ func newTraceHookCursorSessionEndCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionEnd(cursor.New(), logger) }, } @@ -267,8 +274,7 @@ func newTraceHookCursorAfterFileEditCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPostToolUse(cursor.New(), logger) }, } @@ -299,8 +305,7 @@ func newTraceHookOpenCodeSessionStartCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionStart(opencode.New(), logger) }, } @@ -314,8 +319,7 @@ func newTraceHookOpenCodeUserPromptSubmitCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPromptSubmit(opencode.New(), logger) }, } @@ -329,8 +333,7 @@ func newTraceHookOpenCodeSessionEndCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentSessionEnd(opencode.New(), logger) }, } @@ -344,8 +347,7 @@ func newTraceHookOpenCodePreToolUseCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPreToolUse(opencode.New(), logger) }, } @@ -359,8 +361,7 @@ func newTraceHookOpenCodePostToolUseCmd() *cobra.Command { "skipActionOptsInit": "true", }, RunE: func(_ *cobra.Command, _ []string) error { - cleanup := InitHookLogger() - defer cleanup() + InitHookLogger() return action.HandleAgentPostToolUse(opencode.New(), logger) }, } diff --git a/app/cli/cmd/trace_hook_test.go b/app/cli/cmd/trace_hook_test.go new file mode 100644 index 000000000..81c0daa0c --- /dev/null +++ b/app/cli/cmd/trace_hook_test.go @@ -0,0 +1,65 @@ +// +// 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 cmd + +import ( + "errors" + "os" + "path/filepath" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestApplyPrePushPolicy(t *testing.T) { + failure := errors.New("setup failed") + prePush := newTraceHookGitPrePushCmd() + other := &cobra.Command{Use: "other"} + + testCases := []struct { + name string + executed *cobra.Command + err error + requireTrace bool + wantErr bool + }{ + {name: "success stays success", executed: prePush}, + {name: "pre-push failure fails open by default", executed: prePush, err: failure}, + {name: "pre-push failure blocks with requireTrace", executed: prePush, err: failure, requireTrace: true, wantErr: true}, + {name: "other commands keep their error", executed: other, err: failure, wantErr: true}, + {name: "unknown command keeps its error", err: failure, wantErr: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + if tc.requireTrace { + require.NoError(t, os.WriteFile(filepath.Join(dir, ".chainloop.yml"), []byte("requireTrace: true\n"), 0600)) + } + t.Chdir(dir) + + err := applyPrePushPolicy(tc.executed, tc.err) + if tc.wantErr { + require.ErrorIs(t, err, failure) + return + } + + assert.NoError(t, err) + }) + } +} diff --git a/app/cli/cmd/trace_hooklog.go b/app/cli/cmd/trace_hooklog.go index 22fed008a..399311440 100644 --- a/app/cli/cmd/trace_hooklog.go +++ b/app/cli/cmd/trace_hooklog.go @@ -23,8 +23,9 @@ import ( "github.com/rs/zerolog" ) -// hookLogFile holds the trace log file while hook commands run, so it can be -// closed by the cleanup closure returned from InitHookLogger. +// hookLogFile holds the trace log file opened by InitHookLogger. It stays open +// until CloseHookLog runs at process exit, so the lines logged after the +// command returns (main's final error line, telemetry) still reach it. var hookLogFile *os.File // stderrMinLevel is the minimum zerolog.Level that hook commands write to @@ -67,6 +68,12 @@ func hookStderrWriter() *levelFilterWriter { } } +// hookStderrLogger builds the stderr-only hook logger, used when no trace log +// file is open. +func hookStderrLogger() zerolog.Logger { + return zerolog.New(hookStderrWriter()).Level(stderrMinLevel) +} + // InitHookLogger reconfigures the root logger for git/agent hook commands: // colorless output, all levels written to the trace state's log.txt, and // Warn+ (or Debug, with --debug) written to stderr. Colorless matters because @@ -75,9 +82,9 @@ func hookStderrWriter() *levelFilterWriter { // // If trace state cannot be located or the file cannot be opened, the logger // falls back to colorless stderr-only output so the rest of the hook still -// runs with consistent formatting. Returns a cleanup function that closes the -// log file — callers should defer it at the top of a hook's RunE. -func InitHookLogger() func() { +// runs with consistent formatting. The log file is closed by CloseHookLog, not +// by the hook command: main logs a failed command's error after RunE returns. +func InitHookLogger() { closeHookLogFile() // Mirror initLogger: Info normally, Debug with --debug. Info matters — @@ -89,19 +96,28 @@ func InitHookLogger() func() { stderrMinLevel = zerolog.DebugLevel } - logger = zerolog.New(hookStderrWriter()).Level(stderrMinLevel) + logger = hookStderrLogger() store, _, err := state.Locate() if err != nil { - return func() {} + return } if err := initHookLogFile(store); err != nil { logger.Debug().Err(err).Msg("could not open hook log file") - return func() {} + } +} + +// CloseHookLog closes the trace log file opened by InitHookLogger and points +// the root logger back at stderr, so a later log line never writes to a +// closed file. main calls it right before the process exits. +func CloseHookLog() { + if hookLogFile == nil { + return } - return closeHookLogFile + closeHookLogFile() + logger = hookStderrLogger() } // initHookLogFile opens the trace log file and reassigns the root logger to a diff --git a/app/cli/cmd/trace_hooklog_test.go b/app/cli/cmd/trace_hooklog_test.go index 1b8fce80d..f692576e6 100644 --- a/app/cli/cmd/trace_hooklog_test.go +++ b/app/cli/cmd/trace_hooklog_test.go @@ -97,6 +97,35 @@ func TestInitHookLogFileFailsOnBadPath(t *testing.T) { assert.Error(t, err) } +// TestCloseHookLog covers the exit path: main logs a failed hook's error after +// RunE returns, then closes the file. Nothing logged afterwards (telemetry) +// may write to the closed file. +func TestCloseHookLog(t *testing.T) { + store := state.NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + resetHookLoggerState(t) + + var writeErrs []error + prevHandler := zerolog.ErrorHandler + zerolog.ErrorHandler = func(err error) { writeErrs = append(writeErrs, err) } + t.Cleanup(func() { zerolog.ErrorHandler = prevHandler }) + + require.NoError(t, initHookLogFile(store)) + l := Logger() + l.Error().Msg("final error line") + + CloseHookLog() + l = Logger() + l.Debug().Msg("logged after close") + CloseHookLog() + + assert.Empty(t, writeErrs) + content, err := os.ReadFile(store.LogFilePath()) + require.NoError(t, err) + assert.Contains(t, string(content), "final error line") + assert.NotContains(t, string(content), "logged after close") +} + func TestCloseHookLogFileIdempotent(_ *testing.T) { // Calling close without init should not panic closeHookLogFile() diff --git a/app/cli/internal/trace/hooks/hooks.go b/app/cli/internal/trace/hooks/hooks.go index 2f8eb1542..e7bc50f35 100644 --- a/app/cli/internal/trace/hooks/hooks.go +++ b/app/cli/internal/trace/hooks/hooks.go @@ -32,16 +32,20 @@ const ( // managedHooks defines every git hook that Install creates. // Order matters: hooks are installed in this order. var managedHooks = []hookDef{ - {"commit-msg", true}, - {"post-commit", false}, - {"post-rewrite", false}, - {"pre-push", false}, + {name: "commit-msg", passArgs: true}, + {name: "post-commit"}, + {name: "post-rewrite"}, + {name: "pre-push", blocking: true}, } type hookDef struct { // name is both the git hook filename and the "chainloop trace hook git " subcommand. name string passArgs bool + // blocking makes the hook exit with chainloop's status, so a failure aborts + // the git operation. Only pre-push sets it: the pre-push command fails only + // when requireTrace is enabled. + blocking bool } // selectedHooks returns managedHooks with pre-push filtered out when @@ -61,9 +65,10 @@ func selectedHooks(skipPrePush bool) []hookDef { return out } -// IsInstalled reports whether the managed hooks are present and carry -// the chainloop marker. Pass skipPrePush=true to exclude pre-push from -// the check, matching what trace run installs. +// IsInstalled reports whether the managed hooks are present and match the +// scripts this CLI writes. A managed script written by an older CLI reports +// false, so the caller reinstalls it. Pass skipPrePush=true to exclude +// pre-push from the check, matching what trace run installs. // // gitDir may point at a per-worktree gitdir; the common hooks dir is // resolved internally (see resolveHooksDir). @@ -74,8 +79,13 @@ func IsInstalled(gitDir string, skipPrePush bool) bool { } for _, h := range selectedHooks(skipPrePush) { - content, err := os.ReadFile(filepath.Join(hooksDir, h.name)) - if err != nil || !strings.Contains(string(content), HookMarker) { + hookPath := filepath.Join(hooksDir, h.name) + content, err := os.ReadFile(hookPath) + if err != nil { + return false + } + + if string(content) != hookContent(h, chainTarget(hookPath+hookBackupSuffix)) { return false } } @@ -83,32 +93,38 @@ func IsInstalled(gitDir string, skipPrePush bool) bool { return true } -// hookContent generates a hook script for the given command. -// When passArgs is true, "$@" is appended so the hook receives its positional arguments -// (e.g. commit-msg receives the message file path as $1). -func hookContent(hookCmd string, passArgs bool) string { +// hookContent generates the hook script for h. When passArgs is true, "$@" is +// appended so the hook receives its positional arguments (e.g. commit-msg +// receives the message file path as $1). When backupPath is not empty, the +// script chains to that backed-up foreign hook. +// +// A missing chainloop binary never fails the hook: without the command -v +// guard sh exits 127 and git aborts. Non-blocking hooks also ignore +// chainloop's exit status, so tracing never blocks a commit. +func hookContent(h hookDef, backupPath string) string { args := "" - if passArgs { + if h.passArgs { args = ` "$@"` } - // The trailing exit 0 keeps tracing from ever blocking a commit or push: - // without it a missing chainloop binary makes sh exit 127 and git aborts. - return fmt.Sprintf("#!/bin/sh\n%s\nchainloop trace hook git %s%s\nexit 0\n", HookMarker, hookCmd, args) -} + cmd := fmt.Sprintf("chainloop trace hook git %s%s", h.name, args) + if h.blocking { + // Propagate chainloop's status so requireTrace can block the push. + cmd += " || exit $?" + } -// hookContentWithChain generates a hook script that chains to a backup. -func hookContentWithChain(hookCmd, backupPath string, passArgs bool) string { - args := "" - if passArgs { - args = ` "$@"` + var b strings.Builder + fmt.Fprintf(&b, "#!/bin/sh\n%s\n", HookMarker) + fmt.Fprintf(&b, "if command -v chainloop >/dev/null 2>&1; then\n\t%s\nfi\n", cmd) + if backupPath != "" { + // exec makes the chained hook's status the hook's status. The trailing + // exit 0 covers a backup that lost its executable bit, where the && list + // would otherwise fail the hook. + fmt.Fprintf(&b, "[ -x \"%s\" ] && exec \"%s\" \"$@\"\n", backupPath, backupPath) } + b.WriteString("exit 0\n") - // exec makes the chained hook's status the hook's status. The trailing - // exit 0 covers a backup that lost its executable bit, where the && list - // would otherwise fail the hook (see hookContent). - return fmt.Sprintf("#!/bin/sh\n%s\nchainloop trace hook git %s%s\n[ -x \"%s\" ] && exec \"%s\" \"$@\"\nexit 0\n", - HookMarker, hookCmd, args, backupPath, backupPath) + return b.String() } // Install installs git hooks for trace automation. gitDir may be either @@ -133,7 +149,7 @@ func Install(gitDir string, skipPrePush bool) (string, error) { hookPath := filepath.Join(hooksDir, h.name) backupPath := hookPath + hookBackupSuffix - if err := installSingleHook(hookPath, backupPath, h.name, h.passArgs); err != nil { + if err := installSingleHook(hookPath, backupPath, h); err != nil { return "", fmt.Errorf("install %s hook: %w", h.name, err) } } @@ -141,7 +157,7 @@ func Install(gitDir string, skipPrePush bool) (string, error) { return hooksDir, nil } -func installSingleHook(hookPath, backupPath, hookCmd string, passArgs bool) error { +func installSingleHook(hookPath, backupPath string, h hookDef) error { existing, err := os.ReadFile(hookPath) if err != nil && !errors.Is(err, os.ErrNotExist) { return fmt.Errorf("read existing hook: %w", err) @@ -169,13 +185,19 @@ func installSingleHook(hookPath, backupPath, hookCmd string, passArgs bool) erro } // Preserve chaining if a backup exists (idempotent reinstall) - if backupExists { - //nolint:gosec // git only runs hooks that are executable, so 0600 would silently disable them - return os.WriteFile(hookPath, []byte(hookContentWithChain(hookCmd, backupPath, passArgs)), 0755) + //nolint:gosec // git only runs hooks that are executable, so 0600 would silently disable them + return os.WriteFile(hookPath, []byte(hookContent(h, chainTarget(backupPath))), 0755) +} + +// chainTarget returns backupPath when a backed-up foreign hook exists there, +// or "" when there is nothing to chain to. Lstat: a dangling symlink still +// counts, matching what installSingleHook refuses to overwrite. +func chainTarget(backupPath string) string { + if _, err := os.Lstat(backupPath); err != nil { + return "" } - //nolint:gosec // git only runs hooks that are executable, so 0600 would silently disable them - return os.WriteFile(hookPath, []byte(hookContent(hookCmd, passArgs)), 0755) + return backupPath } // Uninstall removes git hooks installed by trace. gitDir semantics match diff --git a/app/cli/internal/trace/hooks/hooks_test.go b/app/cli/internal/trace/hooks/hooks_test.go index b1ffdde7f..c8ec5ceb7 100644 --- a/app/cli/internal/trace/hooks/hooks_test.go +++ b/app/cli/internal/trace/hooks/hooks_test.go @@ -16,6 +16,7 @@ package hooks import ( + "fmt" "os" "os/exec" "path/filepath" @@ -160,6 +161,43 @@ func TestInstallSkipPrePush(t *testing.T) { assert.True(t, IsInstalled(gitDir, true), "should be installed when only checking the trace-run subset") assert.False(t, IsInstalled(gitDir, false), "default check requires pre-push and should fail") }) + + t.Run("IsInstalled reports outdated managed scripts so they get reinstalled", func(t *testing.T) { + testCases := []struct { + name string + foreignHook bool + }{ + {name: "plain"}, + {name: "chained", foreignHook: true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gitDir := t.TempDir() + hooksDir := filepath.Join(gitDir, "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0755)) + if tc.foreignHook { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-push"), []byte("#!/bin/sh\nexit 0\n"), 0600)) + } + + _, err := Install(gitDir, false) + require.NoError(t, err) + require.True(t, IsInstalled(gitDir, false)) + + // A pre-push script written by an older CLI that always exits 0. + stale := "#!/bin/sh\n" + HookMarker + "\nchainloop trace hook git pre-push\nexit 0\n" + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "pre-push"), []byte(stale), 0600)) + assert.False(t, IsInstalled(gitDir, false)) + + _, err = Install(gitDir, false) + require.NoError(t, err) + assert.True(t, IsInstalled(gitDir, false)) + + _, err = os.Stat(filepath.Join(hooksDir, "pre-push"+hookBackupSuffix)) + assert.Equal(t, tc.foreignHook, err == nil, "backup must be kept as-is") + }) + } + }) } func TestInstallRefusesToClobberBackup(t *testing.T) { @@ -181,27 +219,81 @@ func TestInstallRefusesToClobberBackup(t *testing.T) { assert.Equal(t, original, string(content)) } -// TestHookScriptsExitZero runs each generated hook with an empty PATH (no -// chainloop binary, as in GUI git clients) and asserts it still succeeds: -// a non-zero hook aborts the user's commit. -func TestHookScriptsExitZero(t *testing.T) { - gitDir := t.TempDir() - hooksDir := filepath.Join(gitDir, "hooks") - require.NoError(t, os.MkdirAll(hooksDir, 0755)) - - // post-commit gets a foreign hook so the chained variant is covered too, - // with its executable bit cleared to exercise the failing `&&` list. - require.NoError(t, os.WriteFile(filepath.Join(hooksDir, "post-commit"), []byte("#!/bin/sh\nexit 0\n"), 0600)) +// TestHookScriptsExitStatus runs each generated hook against a fake chainloop +// binary, or none at all (as in GUI git clients). Only pre-push propagates a +// chainloop failure, so requireTrace can block the push; every other hook must +// never abort the user's commit, and a missing binary never fails any hook. +func TestHookScriptsExitStatus(t *testing.T) { + testCases := []struct { + name string + hook string + chainloopExit int + noBinary bool + foreignHook bool + // foreignNoExec clears the foreign hook's executable bit, which + // exercises the failing `&&` list in the chained script. + foreignNoExec bool + wantErr bool + wantChained bool + }{ + {name: "pre-push blocks on chainloop failure", hook: "pre-push", chainloopExit: 1, wantErr: true}, + {name: "pre-push passes on chainloop success", hook: "pre-push", chainloopExit: 0}, + {name: "chained pre-push blocks without running the foreign hook", hook: "pre-push", chainloopExit: 1, foreignHook: true, wantErr: true}, + {name: "chained pre-push runs the foreign hook on success", hook: "pre-push", chainloopExit: 0, foreignHook: true, wantChained: true}, + {name: "commit-msg ignores chainloop failure", hook: "commit-msg", chainloopExit: 1}, + {name: "post-commit ignores chainloop failure", hook: "post-commit", chainloopExit: 1}, + {name: "post-rewrite ignores chainloop failure", hook: "post-rewrite", chainloopExit: 1}, + {name: "chained post-commit ignores chainloop failure", hook: "post-commit", chainloopExit: 1, foreignHook: true, wantChained: true}, + {name: "pre-push passes without chainloop", hook: "pre-push", noBinary: true}, + {name: "commit-msg passes without chainloop", hook: "commit-msg", noBinary: true}, + {name: "post-commit passes without chainloop", hook: "post-commit", noBinary: true}, + {name: "post-rewrite passes without chainloop", hook: "post-rewrite", noBinary: true}, + {name: "chained pre-push runs the foreign hook without chainloop", hook: "pre-push", noBinary: true, foreignHook: true, wantChained: true}, + {name: "chained post-commit passes with a non-executable foreign hook", hook: "post-commit", noBinary: true, foreignHook: true, foreignNoExec: true}, + } - _, err := Install(gitDir, false) - require.NoError(t, err) + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + gitDir := t.TempDir() + hooksDir := filepath.Join(gitDir, "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0755)) + + marker := filepath.Join(t.TempDir(), "chained") + if tc.foreignHook { + mode := os.FileMode(0755) + if tc.foreignNoExec { + mode = 0600 + } + // A redirection, not touch: PATH holds only the fake binary. + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, tc.hook), + []byte("#!/bin/sh\n: > \""+marker+"\"\n"), mode)) + } + + _, err := Install(gitDir, false) + require.NoError(t, err) - 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="} - out, err := cmd.CombinedOutput() - assert.NoError(t, err, "hook %s must exit 0 without chainloop on PATH: %s", name, out) + binDir := t.TempDir() + if !tc.noBinary { + //nolint:gosec // the fake binary must be executable + require.NoError(t, os.WriteFile(filepath.Join(binDir, "chainloop"), + []byte(fmt.Sprintf("#!/bin/sh\nexit %d\n", tc.chainloopExit)), 0755)) + } + + //nolint:gosec // the hook path is derived from t.TempDir(), not from user input + cmd := exec.Command("/bin/sh", filepath.Join(hooksDir, tc.hook), "origin") + cmd.Env = []string{"PATH=" + binDir} + out, err := cmd.CombinedOutput() + if tc.wantErr { + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr, "hook output: %s", out) + assert.Equal(t, tc.chainloopExit, exitErr.ExitCode()) + } else { + require.NoError(t, err, "hook output: %s", out) + } + + _, statErr := os.Stat(marker) + assert.Equal(t, tc.wantChained, statErr == nil, "foreign hook ran") + }) } } diff --git a/app/cli/main.go b/app/cli/main.go index 231ee96b9..0edde2838 100644 --- a/app/cli/main.go +++ b/app/cli/main.go @@ -43,13 +43,20 @@ func main() { FormatTimestamp: func(interface{}) string { return "" }, }) rootCmd := cmd.NewRootCmd(logger) + exitCode := 0 if err := cmd.Execute(rootCmd); err != nil { // cmd.Logger() rather than the local logger: a hook command may have - // swapped the root logger for a colorless one, and this final line - // should match. + // swapped the root logger for a colorless one that also writes to the + // trace log file, and this final line belongs in both. hookLogger := cmd.Logger() - msg, exitCode := errorInfo(err, hookLogger) + var msg string + msg, exitCode = errorInfo(err, hookLogger) hookLogger.Error().Msg(msg) + } + + // After the final error line, and before os.Exit, which skips defers. + cmd.CloseHookLog() + if exitCode != 0 { os.Exit(exitCode) } } diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 1a435449a..f1cc2c4c1 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -526,19 +526,28 @@ type RunTracePushOpts struct { CLIVersion string } -// HandlePrePushHook handles the pre-push git hook. -// When requireTrace is true, errors from the attestation push are -// propagated so that the git push is blocked. When false, errors are -// logged but never returned. -func HandlePrePushHook(ctx context.Context, requireTrace bool, log zerolog.Logger, opts RunTracePushOpts) error { +// HandlePrePushHook handles the pre-push git hook. The caller passes a +// returned error through PrePushFailure. +func HandlePrePushHook(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts) error { drainPushStdin() - if err := RunTracePush(ctx, log, opts); err != nil { - if requireTrace { - return fmt.Errorf("attestation failed (--require-trace is enabled): %w", err) - } + return RunTracePush(ctx, log, opts) +} - log.Debug().Err(err).Msg("pre-push hook failed") +// PrePushFailure decides what a failed pre-push hook does. The managed +// pre-push script propagates the hook's exit status to git. When +// requireTrace is true, it returns the error so that the git push is +// blocked. When false, it logs a warning, so the user knows why the session +// is missing, and returns nil so the push continues. +func PrePushFailure(err error, requireTrace bool, log zerolog.Logger) error { + if requireTrace { + return fmt.Errorf("attestation failed (--require-trace is enabled): %w", err) + } + + if msg, ok := AuthErrorMessage(err); ok { + log.Warn().Msgf("AI coding session not uploaded: %s", msg) + } else { + log.Warn().Err(err).Msg("AI coding session not uploaded") } return nil diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index ae9ae12e0..1fee2e421 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -18,6 +18,7 @@ package action import ( "bytes" "encoding/json" + "errors" "os" "os/exec" "path/filepath" @@ -30,11 +31,45 @@ import ( tracegit "github.com/chainloop-dev/chainloop/app/cli/internal/trace/git" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/state" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials/aicodingsession" + jwtMiddleware "github.com/go-kratos/kratos/v2/middleware/auth/jwt" "github.com/rs/zerolog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestPrePushFailure(t *testing.T) { + expired := jwtMiddleware.ErrTokenExpired.GRPCStatus().Err() + + testCases := []struct { + name string + err error + requireTrace bool + wantErr bool + wantLog string + }{ + {name: "requireTrace blocks the push", err: expired, requireTrace: true, wantErr: true}, + {name: "auth failure warns with the fix", err: expired, wantLog: `please run \"chainloop auth login\" again`}, + {name: "other failure warns with the cause", err: errors.New("boom"), wantLog: "boom"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + err := PrePushFailure(tc.err, tc.requireTrace, zerolog.New(&buf)) + + if tc.wantErr { + require.ErrorIs(t, err, tc.err) + assert.Empty(t, buf.String(), "the error is logged once, by main") + return + } + + require.NoError(t, err) + assert.Contains(t, buf.String(), `"level":"warn"`) + assert.Contains(t, buf.String(), tc.wantLog) + }) + } +} + // Fixture names reused across the session-matching tests. const ( sessionA = "session-a" From b7cc4845b20b6679d1b3b00bcdb6db572287cb94 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 8 Oct 2026 00:16:23 +0200 Subject: [PATCH 2/2] test(trace): add golden files for the generated git hook scripts Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: a5941a7b-7e2e-46b4-b95d-cd9fadb00b3b --- app/cli/internal/trace/hooks/hooks_test.go | 53 +++++++++++++++++++ .../trace/hooks/testdata/commit-msg.sh | 6 +++ .../hooks/testdata/post-commit-chained.sh | 7 +++ .../trace/hooks/testdata/post-commit.sh | 6 +++ .../trace/hooks/testdata/post-rewrite.sh | 6 +++ .../trace/hooks/testdata/pre-push-chained.sh | 7 +++ .../internal/trace/hooks/testdata/pre-push.sh | 6 +++ 7 files changed, 91 insertions(+) create mode 100644 app/cli/internal/trace/hooks/testdata/commit-msg.sh create mode 100644 app/cli/internal/trace/hooks/testdata/post-commit-chained.sh create mode 100644 app/cli/internal/trace/hooks/testdata/post-commit.sh create mode 100644 app/cli/internal/trace/hooks/testdata/post-rewrite.sh create mode 100644 app/cli/internal/trace/hooks/testdata/pre-push-chained.sh create mode 100644 app/cli/internal/trace/hooks/testdata/pre-push.sh diff --git a/app/cli/internal/trace/hooks/hooks_test.go b/app/cli/internal/trace/hooks/hooks_test.go index c8ec5ceb7..70a3ef035 100644 --- a/app/cli/internal/trace/hooks/hooks_test.go +++ b/app/cli/internal/trace/hooks/hooks_test.go @@ -20,6 +20,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -219,6 +220,58 @@ func TestInstallRefusesToClobberBackup(t *testing.T) { assert.Equal(t, original, string(content)) } +// TestHookGoldenFiles compares the hook scripts Install writes against the +// committed golden files in testdata/. The temporary hooks directory is +// replaced by a placeholder so the files stay stable. Run with +// UPDATE_GOLDEN=1 to regenerate them after an intentional change: +// +// UPDATE_GOLDEN=1 go test ./app/cli/internal/trace/hooks/ -run TestHookGoldenFiles +func TestHookGoldenFiles(t *testing.T) { + const hooksDirPlaceholder = "" + + testCases := []struct { + hook string + foreignHook bool + golden string + }{ + {hook: "commit-msg", golden: "testdata/commit-msg.sh"}, + {hook: "post-commit", golden: "testdata/post-commit.sh"}, + {hook: "post-rewrite", golden: "testdata/post-rewrite.sh"}, + {hook: "pre-push", golden: "testdata/pre-push.sh"}, + {hook: "pre-push", foreignHook: true, golden: "testdata/pre-push-chained.sh"}, + {hook: "post-commit", foreignHook: true, golden: "testdata/post-commit-chained.sh"}, + } + + for _, tc := range testCases { + t.Run(filepath.Base(tc.golden), func(t *testing.T) { + gitDir := t.TempDir() + hooksDir := filepath.Join(gitDir, "hooks") + require.NoError(t, os.MkdirAll(hooksDir, 0755)) + if tc.foreignHook { + require.NoError(t, os.WriteFile(filepath.Join(hooksDir, tc.hook), []byte("#!/bin/sh\n"), 0600)) + } + + _, err := Install(gitDir, false) + require.NoError(t, err) + + content, err := os.ReadFile(filepath.Join(hooksDir, tc.hook)) + require.NoError(t, err) + generated := strings.ReplaceAll(string(content), hooksDir, hooksDirPlaceholder) + + if os.Getenv("UPDATE_GOLDEN") == "1" { + //nolint:gosec // golden paths are fixed testdata paths from the table above + require.NoError(t, os.WriteFile(tc.golden, []byte(generated), 0600)) + return + } + + expected, err := os.ReadFile(tc.golden) + require.NoError(t, err, "golden file missing; run UPDATE_GOLDEN=1 to generate") + assert.Equal(t, string(expected), generated, + "generated hook does not match golden file %s; run UPDATE_GOLDEN=1 to update", tc.golden) + }) + } +} + // TestHookScriptsExitStatus runs each generated hook against a fake chainloop // binary, or none at all (as in GUI git clients). Only pre-push propagates a // chainloop failure, so requireTrace can block the push; every other hook must diff --git a/app/cli/internal/trace/hooks/testdata/commit-msg.sh b/app/cli/internal/trace/hooks/testdata/commit-msg.sh new file mode 100644 index 000000000..40baaded1 --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/commit-msg.sh @@ -0,0 +1,6 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git commit-msg "$@" +fi +exit 0 diff --git a/app/cli/internal/trace/hooks/testdata/post-commit-chained.sh b/app/cli/internal/trace/hooks/testdata/post-commit-chained.sh new file mode 100644 index 000000000..3bbda8d73 --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/post-commit-chained.sh @@ -0,0 +1,7 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git post-commit +fi +[ -x "/post-commit.chainloop-backup" ] && exec "/post-commit.chainloop-backup" "$@" +exit 0 diff --git a/app/cli/internal/trace/hooks/testdata/post-commit.sh b/app/cli/internal/trace/hooks/testdata/post-commit.sh new file mode 100644 index 000000000..790dde91d --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/post-commit.sh @@ -0,0 +1,6 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git post-commit +fi +exit 0 diff --git a/app/cli/internal/trace/hooks/testdata/post-rewrite.sh b/app/cli/internal/trace/hooks/testdata/post-rewrite.sh new file mode 100644 index 000000000..fca10c39b --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/post-rewrite.sh @@ -0,0 +1,6 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git post-rewrite +fi +exit 0 diff --git a/app/cli/internal/trace/hooks/testdata/pre-push-chained.sh b/app/cli/internal/trace/hooks/testdata/pre-push-chained.sh new file mode 100644 index 000000000..9a8bfae06 --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/pre-push-chained.sh @@ -0,0 +1,7 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git pre-push || exit $? +fi +[ -x "/pre-push.chainloop-backup" ] && exec "/pre-push.chainloop-backup" "$@" +exit 0 diff --git a/app/cli/internal/trace/hooks/testdata/pre-push.sh b/app/cli/internal/trace/hooks/testdata/pre-push.sh new file mode 100644 index 000000000..23389263f --- /dev/null +++ b/app/cli/internal/trace/hooks/testdata/pre-push.sh @@ -0,0 +1,6 @@ +#!/bin/sh +# chainloop-trace-managed +if command -v chainloop >/dev/null 2>&1; then + chainloop trace hook git pre-push || exit $? +fi +exit 0