From d52112c04531684e9f5b795db3e5d2a9ba2a79e0 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Wed, 7 Oct 2026 14:35:39 +0200 Subject: [PATCH] fix(trace): preselect configured harnesses when re-running trace init Re-running chainloop trace init ticked only Claude Code in the harness prompt, even when the repository was already set up for others. Each provider can now report whether its Chainloop hooks are installed, and init preselects those harnesses, falling back to Claude Code on a first run. Fixes PFM-7633 Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: d9398d05-28da-4a58-a25e-31e0ee714c92 --- app/cli/cmd/trace_init.go | 7 +- app/cli/cmd/trace_init_providers.go | 37 +++++++- app/cli/cmd/trace_init_providers_test.go | 94 ++++++++++++++++++- app/cli/documentation/cli-reference.md | 5 +- app/cli/internal/trace/claude/hooks.go | 20 ++++ app/cli/internal/trace/claude/hooks_test.go | 42 +++++++++ app/cli/internal/trace/cursor/hooks.go | 20 ++++ app/cli/internal/trace/cursor/hooks_test.go | 42 +++++++++ app/cli/internal/trace/opencode/hooks.go | 11 +++ app/cli/internal/trace/provider.go | 5 + .../trace/providers/hooks_installed_test.go | 69 ++++++++++++++ 11 files changed, 339 insertions(+), 13 deletions(-) create mode 100644 app/cli/internal/trace/providers/hooks_installed_test.go diff --git a/app/cli/cmd/trace_init.go b/app/cli/cmd/trace_init.go index 4d80f4b2d..8cecd15a6 100644 --- a/app/cli/cmd/trace_init.go +++ b/app/cli/cmd/trace_init.go @@ -61,8 +61,9 @@ exists, so you need to be logged in. On a terminal it asks which organization and project to use, offering what .chainloop.yml already holds so pressing Enter keeps it. A new project can be named freely; the name is normalized to the lowercase, dash-separated form -Chainloop stores. It then asks which harnesses to trace, with Claude Code -ticked; use space to tick more. Passing --org, --project or a harness flag +Chainloop stores. It then asks which harnesses to trace, with the ones the +repository is already set up for ticked, or Claude Code on a first run; use +space to tick more. Passing --org, --project or a harness flag (--claude, --cursor, --opencode) skips the matching question. Nothing is asked in CI or when the output is redirected: there --project is @@ -96,7 +97,7 @@ The organization, project, workflow and require-trace values are saved to // repository untouched. selected, err := resolveTraceProviders(newNamingPrompter(os.LookupEnv), traceProviderFlags{claude: claudeFlag, cursor: cursorFlag, opencode: opencodeFlag}, - traceInitCanPrompt()) + traceInitCanPrompt(), func() []string { return configuredTraceProviders(repoRoot) }) if err != nil { return stopIfAborted(err) } diff --git a/app/cli/cmd/trace_init_providers.go b/app/cli/cmd/trace_init_providers.go index e4f4cfc18..e88d1f735 100644 --- a/app/cli/cmd/trace_init_providers.go +++ b/app/cli/cmd/trace_init_providers.go @@ -18,6 +18,7 @@ package cmd import ( "errors" + "github.com/chainloop-dev/chainloop/app/cli/internal/trace" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/claude" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/cursor" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/opencode" @@ -78,11 +79,34 @@ func traceProviderNames() []string { return names } +// configuredTraceProviders lists the harnesses whose hooks the repository +// already carries, in the registry's order, so a re-run of init offers what an +// earlier one set up. A harness whose configuration cannot be read counts as +// not configured: init is how it gets repaired, so it must not stop init. +func configuredTraceProviders(repoRoot string) []string { + var configured []trace.Provider + for _, p := range providers.All() { + installed, err := p.HooksInstalled(repoRoot) + if err != nil { + logger.Debug().Err(err).Str("harness", p.Name()).Msg("could not read the harness configuration") + continue + } + + if installed { + configured = append(configured, p) + } + } + + return providerNames(configured) +} + // resolveTraceProviders picks the agents whose hooks init installs. Flags win // and skip the question, the same way --org and --project do. Otherwise a -// terminal session ticks them off a list with the default preselected, and a -// non-interactive one keeps that default so scripted runs are unchanged. -func resolveTraceProviders(p prompter, flags traceProviderFlags, interactive bool) ([]string, error) { +// terminal session ticks them off a list with the harnesses already configured +// preselected, or the default on a first run, and a non-interactive one keeps +// the default so scripted runs are unchanged. configured is only called when +// the question is asked, so the other paths read nothing from the repository. +func resolveTraceProviders(p prompter, flags traceProviderFlags, interactive bool, configured func() []string) ([]string, error) { if flags.any() { return flags.names(), nil } @@ -91,7 +115,12 @@ func resolveTraceProviders(p prompter, flags traceProviderFlags, interactive boo return []string{providers.DefaultProvider}, nil } - chosen, err := p.MultiSelect(providersPromptTitle, traceProviderNames(), []string{providers.DefaultProvider}) + defaults := configured() + if len(defaults) == 0 { + defaults = []string{providers.DefaultProvider} + } + + chosen, err := p.MultiSelect(providersPromptTitle, traceProviderNames(), defaults) if err != nil { return nil, err } diff --git a/app/cli/cmd/trace_init_providers_test.go b/app/cli/cmd/trace_init_providers_test.go index c9972d3ce..83170a267 100644 --- a/app/cli/cmd/trace_init_providers_test.go +++ b/app/cli/cmd/trace_init_providers_test.go @@ -16,6 +16,8 @@ package cmd import ( + "os" + "path/filepath" "testing" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/providers" @@ -34,11 +36,15 @@ func TestResolveTraceProviders(t *testing.T) { name string flags traceProviderFlags interactive bool + // configured are the harnesses whose hooks the repository already has + configured []string // answer is what the user ticks in the multi-select answer []string want []string // wantPrompted is whether the multi-select should have been shown wantPrompted bool + // wantDefaults is what the multi-select comes up with ticked + wantDefaults []string wantErr string }{ { @@ -64,6 +70,32 @@ func TestResolveTraceProviders(t *testing.T) { answer: []string{"cursor", "opencode"}, want: []string{"cursor", "opencode"}, wantPrompted: true, + wantDefaults: []string{providers.DefaultProvider}, + }, + { + name: "a re-run comes up with the configured harnesses ticked", + interactive: true, + configured: []string{providerClaudeCode, "cursor", "opencode"}, + answer: []string{providerClaudeCode, "cursor", "opencode"}, + want: []string{providerClaudeCode, "cursor", "opencode"}, + wantPrompted: true, + wantDefaults: []string{providerClaudeCode, "cursor", "opencode"}, + }, + { + name: "a re-run without the default ticks only what is configured", + interactive: true, + configured: []string{"cursor"}, + answer: []string{"cursor"}, + want: []string{"cursor"}, + wantPrompted: true, + wantDefaults: []string{"cursor"}, + }, + { + name: "flags still win over the configured harnesses", + flags: traceProviderFlags{opencode: true}, + interactive: true, + configured: []string{"cursor"}, + want: []string{"opencode"}, }, { name: "picking nothing is refused", @@ -78,7 +110,13 @@ func TestResolveTraceProviders(t *testing.T) { t.Run(tc.name, func(t *testing.T) { p := &fakePrompter{multiSelectAnswer: tc.answer} - got, err := resolveTraceProviders(p, tc.flags, tc.interactive) + var readConfigured bool + configured := func() []string { + readConfigured = true + return tc.configured + } + + got, err := resolveTraceProviders(p, tc.flags, tc.interactive, configured) if tc.wantErr != "" { require.Error(t, err) @@ -91,22 +129,70 @@ func TestResolveTraceProviders(t *testing.T) { if !tc.wantPrompted { assert.Empty(t, p.multiSelects, "no question should have been asked") + assert.False(t, readConfigured, "the repository is only read to preselect the question") return } require.Len(t, p.multiSelects, 1) - // Every registered provider is offered, with the default ticked. + // Every registered provider is offered. assert.Equal(t, traceProviderNames(), p.multiSelects[0].options) - assert.Equal(t, []string{providers.DefaultProvider}, p.multiSelects[0].defaults) + assert.Equal(t, tc.wantDefaults, p.multiSelects[0].defaults) }) } } func TestResolveTraceProvidersPromptError(t *testing.T) { - _, err := resolveTraceProviders(&fakePrompter{multiSelectErr: errAborted}, traceProviderFlags{}, true) + _, err := resolveTraceProviders(&fakePrompter{multiSelectErr: errAborted}, traceProviderFlags{}, true, func() []string { return nil }) require.ErrorIs(t, err, errAborted) } +// TestConfiguredTraceProviders reads the harnesses back from hooks the real +// providers wrote, which is what a re-run of trace init finds (PFM-7633). +func TestConfiguredTraceProviders(t *testing.T) { + testCases := []struct { + name string + installed []string + want []string + }{ + { + name: "a repository never set up", + want: []string{}, + }, + { + name: "some harnesses set up", + installed: []string{"opencode", "cursor"}, + // In the registry's order, the same order the prompt lists them in. + want: []string{"cursor", "opencode"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + repoRoot := t.TempDir() + for _, p := range providers.ByNames(tc.installed) { + require.NoError(t, p.InstallHooks(repoRoot)) + } + + assert.Equal(t, tc.want, configuredTraceProviders(repoRoot)) + }) + } +} + +// TestConfiguredTraceProvidersSkipsUnreadableSettings keeps a broken harness +// configuration from stopping init, which would otherwise be the way to repair it. +func TestConfiguredTraceProvidersSkipsUnreadableSettings(t *testing.T) { + repoRoot := t.TempDir() + + cursor := providers.ByName("cursor") + require.NoError(t, cursor.InstallHooks(repoRoot)) + + claudeSettings := providers.ByName(providerClaudeCode).SettingsFile(repoRoot) + require.NoError(t, os.MkdirAll(filepath.Dir(claudeSettings), 0o755)) + require.NoError(t, os.WriteFile(claudeSettings, []byte("{not json"), 0o600)) + + assert.Equal(t, []string{"cursor"}, configuredTraceProviders(repoRoot)) +} + func TestTraceProviderFlags(t *testing.T) { t.Run("no flag set", func(t *testing.T) { assert.False(t, traceProviderFlags{}.any()) diff --git a/app/cli/documentation/cli-reference.md b/app/cli/documentation/cli-reference.md index 7ca0e9c10..9ecf4b11a 100644 --- a/app/cli/documentation/cli-reference.md +++ b/app/cli/documentation/cli-reference.md @@ -3310,8 +3310,9 @@ exists, so you need to be logged in. On a terminal it asks which organization and project to use, offering what .chainloop.yml already holds so pressing Enter keeps it. A new project can be named freely; the name is normalized to the lowercase, dash-separated form -Chainloop stores. It then asks which harnesses to trace, with Claude Code -ticked; use space to tick more. Passing --org, --project or a harness flag +Chainloop stores. It then asks which harnesses to trace, with the ones the +repository is already set up for ticked, or Claude Code on a first run; use +space to tick more. Passing --org, --project or a harness flag (--claude, --cursor, --opencode) skips the matching question. Nothing is asked in CI or when the output is redirected: there --project is diff --git a/app/cli/internal/trace/claude/hooks.go b/app/cli/internal/trace/claude/hooks.go index 2b975c01c..0b376d0fb 100644 --- a/app/cli/internal/trace/claude/hooks.go +++ b/app/cli/internal/trace/claude/hooks.go @@ -22,6 +22,7 @@ import ( "io" "os" "path/filepath" + "slices" "strings" "github.com/chainloop-dev/chainloop/app/cli/internal/trace" @@ -163,6 +164,25 @@ func (p *Provider) UninstallHooks(repoRoot string) error { return writeJSONFile(settingsPath, settings) } +// HooksInstalled reports whether .claude/settings.json holds any Chainloop +// hook. User-authored hooks are ignored. +func (p *Provider) HooksInstalled(repoRoot string) (bool, error) { + settings, err := readJSONFile(filepath.Join(repoRoot, settingsFile)) + if err != nil { + return false, err + } + + hooks, _ := settings["hooks"].(map[string]any) + for _, h := range hookEvents { + entries, _ := hooks[h.event].([]any) + if slices.ContainsFunc(entries, entryContainsChainloopHook) { + return true, nil + } + } + + return false, nil +} + // maxHookPayloadBytes caps hook payload reads to defend against runaway or // malformed payloads. const maxHookPayloadBytes = 16 * 1024 * 1024 diff --git a/app/cli/internal/trace/claude/hooks_test.go b/app/cli/internal/trace/claude/hooks_test.go index 5f666f451..0b507b152 100644 --- a/app/cli/internal/trace/claude/hooks_test.go +++ b/app/cli/internal/trace/claude/hooks_test.go @@ -375,6 +375,48 @@ func TestInstallHooksOnNullSettings(t *testing.T) { assert.Contains(t, readSettings(t, repoRoot), "hooks") } +func TestHooksInstalled(t *testing.T) { + testCases := []struct { + name string + settings string + want bool + wantErr bool + }{ + { + name: "only user hooks", + settings: `{"hooks":{"PostToolUse":[{"hooks":[{"type":"command","command":"format.sh"}]}]}}`, + want: false, + }, + { + name: "a chainloop hook next to user hooks", + settings: `{"hooks":{"SessionStart":[{"hooks":[{"type":"command","command":"format.sh"},{"type":"command","command":"chainloop trace hook claude session-start"}]}]}}`, + want: true, + }, + { + name: "malformed settings", + settings: `{not json`, + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + repoRoot := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(repoRoot, ".claude"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(repoRoot, settingsFile), []byte(tc.settings), 0600)) + + got, err := New().HooksInstalled(repoRoot) + if tc.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + func readSettings(t *testing.T, repoRoot string) map[string]any { t.Helper() diff --git a/app/cli/internal/trace/cursor/hooks.go b/app/cli/internal/trace/cursor/hooks.go index 6ab9eb0cd..688712576 100644 --- a/app/cli/internal/trace/cursor/hooks.go +++ b/app/cli/internal/trace/cursor/hooks.go @@ -22,6 +22,7 @@ import ( "io" "os" "path/filepath" + "slices" "strings" "github.com/chainloop-dev/chainloop/app/cli/internal/trace" @@ -164,6 +165,25 @@ func (p *Provider) UninstallHooks(repoRoot string) error { return writeJSONFile(settingsPath, settings) } +// HooksInstalled reports whether .cursor/hooks.json holds any Chainloop hook. +// User-authored hooks are ignored. +func (p *Provider) HooksInstalled(repoRoot string) (bool, error) { + settings, err := readJSONFile(filepath.Join(repoRoot, settingsFile)) + if err != nil { + return false, err + } + + hooks, _ := settings["hooks"].(map[string]any) + for _, h := range cursorHookEvents { + entries, _ := hooks[h.event].([]any) + if slices.ContainsFunc(entries, entryContainsChainloopHook) { + return true, nil + } + } + + return false, nil +} + // cursorHookInput is the wire-level structure of a Cursor hook JSON payload. // Only the fields we consume are modelled; unknown fields are ignored. type cursorHookInput struct { diff --git a/app/cli/internal/trace/cursor/hooks_test.go b/app/cli/internal/trace/cursor/hooks_test.go index 967572343..d42369d06 100644 --- a/app/cli/internal/trace/cursor/hooks_test.go +++ b/app/cli/internal/trace/cursor/hooks_test.go @@ -209,3 +209,45 @@ func TestReadHookInputFallsBackToSessionID(t *testing.T) { require.NoError(t, err) assert.Equal(t, "fallback-session", in.SessionID, "expected fallback to session_id") } + +func TestHooksInstalled(t *testing.T) { + testCases := []struct { + name string + settings string + want bool + wantErr bool + }{ + { + name: "only user hooks", + settings: `{"version":1,"hooks":{"sessionStart":[{"command":"./my-hook.sh","timeout":10}]}}`, + want: false, + }, + { + name: "a chainloop hook next to user hooks", + settings: `{"version":1,"hooks":{"sessionStart":[{"command":"./my-hook.sh"},{"command":"chainloop trace hook cursor session-start"}]}}`, + want: true, + }, + { + name: "malformed settings", + settings: `{not json`, + wantErr: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + repoRoot := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(repoRoot, ".cursor"), 0755)) + require.NoError(t, os.WriteFile(filepath.Join(repoRoot, settingsFile), []byte(tc.settings), 0600)) + + got, err := New().HooksInstalled(repoRoot) + if tc.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} diff --git a/app/cli/internal/trace/opencode/hooks.go b/app/cli/internal/trace/opencode/hooks.go index 2ac5f7247..7e76101e3 100644 --- a/app/cli/internal/trace/opencode/hooks.go +++ b/app/cli/internal/trace/opencode/hooks.go @@ -380,6 +380,17 @@ func (p *Provider) UninstallHooks(repoRoot string) error { return err } +// HooksInstalled reports whether the plugin file is present. The file is +// entirely chainloop-owned, so its presence is the installation. +func (p *Provider) HooksInstalled(repoRoot string) (bool, error) { + _, err := os.Stat(filepath.Join(repoRoot, settingsFile)) + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + + return err == nil, err +} + // maxHookPayloadBytes caps hook payload reads to defend against runaway or // malformed payloads. const maxHookPayloadBytes = 16 * 1024 * 1024 diff --git a/app/cli/internal/trace/provider.go b/app/cli/internal/trace/provider.go index 114cd9b56..9bda6655b 100644 --- a/app/cli/internal/trace/provider.go +++ b/app/cli/internal/trace/provider.go @@ -108,6 +108,11 @@ type Provider interface { // UninstallHooks removes the agent's hooks from the repo. UninstallHooks(repoRoot string) error + // HooksInstalled reports whether the repo already carries the agent's + // Chainloop hooks, so `trace init` can offer the harnesses a repository + // is set up for when it runs again. Hooks the user wrote do not count. + HooksInstalled(repoRoot string) (bool, error) + // ReadHookInput reads hook invocation input from the given reader. ReadHookInput(r io.Reader) (*HookInput, error) diff --git a/app/cli/internal/trace/providers/hooks_installed_test.go b/app/cli/internal/trace/providers/hooks_installed_test.go new file mode 100644 index 000000000..85df212bf --- /dev/null +++ b/app/cli/internal/trace/providers/hooks_installed_test.go @@ -0,0 +1,69 @@ +// +// 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 providers + +import ( + "testing" + + "github.com/chainloop-dev/chainloop/app/cli/internal/trace" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestHooksInstalled checks, for every registered provider, that HooksInstalled +// follows what InstallHooks and UninstallHooks leave in the repository. trace +// init relies on it to offer the harnesses already set up on a re-run. +func TestHooksInstalled(t *testing.T) { + testCases := []struct { + name string + setup func(t *testing.T, p trace.Provider, repoRoot string) + want bool + }{ + { + name: "a repository never set up", + setup: func(*testing.T, trace.Provider, string) {}, + want: false, + }, + { + name: "after init installs the hooks", + setup: func(t *testing.T, p trace.Provider, repoRoot string) { + require.NoError(t, p.InstallHooks(repoRoot)) + }, + want: true, + }, + { + name: "after the hooks are uninstalled again", + setup: func(t *testing.T, p trace.Provider, repoRoot string) { + require.NoError(t, p.InstallHooks(repoRoot)) + require.NoError(t, p.UninstallHooks(repoRoot)) + }, + want: false, + }, + } + + for _, p := range All() { + for _, tc := range testCases { + t.Run(p.Name()+"/"+tc.name, func(t *testing.T) { + repoRoot := t.TempDir() + tc.setup(t, p, repoRoot) + + got, err := p.HooksInstalled(repoRoot) + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } + } +}