From d056faf6effa8f3379b146022d17eb18e39dffeb Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 16:10:11 +0200 Subject: [PATCH 01/11] feat(cli): print the session link after a trace push After `chainloop trace` attests an AI coding session, log one line per attested session pointing at its dashboard page. The link is only shown when the control plane reports a ui_dashboard_url, reusing the value the crafting state already carries from attestation init rather than making an extra request from inside a git hook. Without a dashboard the line still names the session. The URL is built from the agent's own session ID, the only identifier available at push time, which the dashboard resolves alongside the Chainloop UUID. Closes #3429 Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/action.go | 29 ++++- app/cli/pkg/action/action_test.go | 111 ++++++++++++++++++ app/cli/pkg/action/attestation_push.go | 17 ++- app/cli/pkg/action/trace_attestation.go | 16 +-- app/cli/pkg/action/trace_hook_handler.go | 28 ++++- app/cli/pkg/action/trace_hook_handler_test.go | 64 ++++++++++ 6 files changed, 248 insertions(+), 17 deletions(-) create mode 100644 app/cli/pkg/action/action_test.go diff --git a/app/cli/pkg/action/action.go b/app/cli/pkg/action/action.go index bc97ff74f..45513d2df 100644 --- a/app/cli/pkg/action/action.go +++ b/app/cli/pkg/action/action.go @@ -18,6 +18,7 @@ package action import ( "context" "fmt" + "net/url" "os" "path/filepath" "strings" @@ -194,14 +195,32 @@ func fetchUIDashboardURL(ctx context.Context, cpConnection *grpc.ClientConn) str return resp.UiDashboardUrl } -// buildAttestationViewURL constructs the attestation view URL -// Returns empty string if platformURL is not configured -func buildAttestationViewURL(uiDashboardURL, orgName, digest string) string { - if uiDashboardURL == "" || digest == "" { +// buildDashboardURL constructs a link to a resource page in the web dashboard, +// of the form /u//
/. It returns an empty string when +// the deployment has no dashboard configured or the resource has no ID, which +// is how callers decide whether to show a link at all. The organization and ID +// are path-escaped, since an ID may be an opaque string chosen elsewhere. +func buildDashboardURL(uiDashboardURL, orgName, section, id string) string { + if uiDashboardURL == "" || id == "" { return "" } // Trim trailing slash from platform URL if present uiDashboardURL = strings.TrimRight(uiDashboardURL, "/") - return fmt.Sprintf("%s/u/%s/workflow-runs/%s", uiDashboardURL, orgName, digest) + + return fmt.Sprintf("%s/u/%s/%s/%s", uiDashboardURL, url.PathEscape(orgName), section, url.PathEscape(id)) +} + +// buildAttestationViewURL constructs the attestation view URL +// Returns empty string if platformURL is not configured +func buildAttestationViewURL(uiDashboardURL, orgName, digest string) string { + return buildDashboardURL(uiDashboardURL, orgName, "workflow-runs", digest) +} + +// buildSessionViewURL constructs the URL of an AI coding session's detail page. +// sessionID is the agent's own session ID (Claude Code, Cursor, OpenCode); the +// dashboard resolves a session by that identifier as well as by its Chainloop +// UUID. +func buildSessionViewURL(uiDashboardURL, orgName, sessionID string) string { + return buildDashboardURL(uiDashboardURL, orgName, "sessions", sessionID) } diff --git a/app/cli/pkg/action/action_test.go b/app/cli/pkg/action/action_test.go new file mode 100644 index 000000000..12281981a --- /dev/null +++ b/app/cli/pkg/action/action_test.go @@ -0,0 +1,111 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package action + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// Shared across the action package's tests. +const ( + testDashboardURL = "https://app.chainloop.dev" + testOrgName = "chainloop" + testSessionID = "ses_1" +) + +func TestBuildSessionViewURL(t *testing.T) { + const uuidSessionID = "1d75645b-fd6b-4e7c-9855-f3137add4cf7" + + testCases := []struct { + name string + uiDashboardURL string + orgName string + sessionID string + want string + }{ + { + name: "no dashboard configured", + uiDashboardURL: "", + orgName: testOrgName, + sessionID: uuidSessionID, + want: "", + }, + { + name: "empty session id", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionID: "", + want: "", + }, + { + name: "uuid session id", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionID: uuidSessionID, + want: testDashboardURL + "/u/chainloop/sessions/" + uuidSessionID, + }, + { + name: "trailing slash is trimmed", + uiDashboardURL: testDashboardURL + "/", + orgName: testOrgName, + sessionID: uuidSessionID, + want: testDashboardURL + "/u/chainloop/sessions/" + uuidSessionID, + }, + { + // OpenCode session IDs are not UUIDs; kept as a guard that a + // non-UUID agent ID still passes through unchanged. + name: "opencode style session id", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionID: "ses_8ab12cd34", + want: testDashboardURL + "/u/chainloop/sessions/ses_8ab12cd34", + }, + { + name: "session id is path escaped", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionID: "a b/c", + want: testDashboardURL + "/u/chainloop/sessions/a%20b%2Fc", + }, + { + name: "org name is path escaped", + uiDashboardURL: testDashboardURL, + orgName: "my org", + sessionID: testSessionID, + want: testDashboardURL + "/u/my%20org/sessions/" + testSessionID, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got := buildSessionViewURL(tc.uiDashboardURL, tc.orgName, tc.sessionID) + assert.Equal(t, tc.want, got) + }) + } +} + +// TestBuildAttestationViewURL guards the route the session link now shares a +// builder with, so the refactor cannot silently change it. +func TestBuildAttestationViewURL(t *testing.T) { + const digest = "sha256:abc123" + + want := testDashboardURL + "/u/chainloop/workflow-runs/" + digest + assert.Equal(t, want, buildAttestationViewURL(testDashboardURL, testOrgName, digest)) + assert.Empty(t, buildAttestationViewURL("", testOrgName, digest)) + assert.Empty(t, buildAttestationViewURL(testDashboardURL, testOrgName, "")) +} diff --git a/app/cli/pkg/action/attestation_push.go b/app/cli/pkg/action/attestation_push.go index ea462f618..a706856c3 100644 --- a/app/cli/pkg/action/attestation_push.go +++ b/app/cli/pkg/action/attestation_push.go @@ -63,6 +63,20 @@ type AttestationResult struct { Digest string `json:"digest"` Envelope *dsse.Envelope `json:"envelope"` Status *AttestationStatusResult `json:"status"` + // UIDashboardURL is the base URL of the web dashboard, as reported by the + // control plane at attestation init, or empty when the deployment has no + // UI configured. + UIDashboardURL string `json:"ui_dashboard_url,omitempty"` +} + +// GetOrganization returns the organization the attestation was pushed to as +// reported by the control plane, or an empty string when unknown. Nil-safe. +func (r *AttestationResult) GetOrganization() string { + if r == nil || r.Status == nil || r.Status.WorkflowMeta == nil { + return "" + } + + return r.Status.WorkflowMeta.Organization } type AttestationPush struct { @@ -276,7 +290,8 @@ func (action *AttestationPush) Run(ctx context.Context, attestationID string, ru } // Build attestation view URL - attestationResult.Status.AttestationViewURL = buildAttestationViewURL(crafter.CraftingState.UiDashboardUrl, workflow.GetOrganization(), attestationResult.Digest) + attestationResult.UIDashboardURL = crafter.CraftingState.UiDashboardUrl + attestationResult.Status.AttestationViewURL = buildAttestationViewURL(attestationResult.UIDashboardURL, workflow.GetOrganization(), attestationResult.Digest) action.Logger.Info().Msg("push completed") diff --git a/app/cli/pkg/action/trace_attestation.go b/app/cli/pkg/action/trace_attestation.go index 945c94f17..dddbbcb68 100644 --- a/app/cli/pkg/action/trace_attestation.go +++ b/app/cli/pkg/action/trace_attestation.go @@ -282,11 +282,12 @@ func (e *AttestationExecutor) Reset(ctx context.Context, trigger, reason string) return nil } -// Push finalizes and pushes the attestation. -func (e *AttestationExecutor) Push(ctx context.Context) error { +// Push finalizes and pushes the attestation, returning the push result so +// callers can report where the evidence landed. +func (e *AttestationExecutor) Push(ctx context.Context) (*AttestationResult, error) { cliVersion, cliDigest, err := e.executableInfo() if err != nil { - return fmt.Errorf("resolve executable info: %w", err) + return nil, fmt.Errorf("resolve executable info: %w", err) } a, err := NewAttestationPush(&AttestationPushOpts{ @@ -299,14 +300,15 @@ func (e *AttestationExecutor) Push(ctx context.Context) error { CLIDigest: cliDigest, }) if err != nil { - return fmt.Errorf("create attestation push action: %w", err) + return nil, fmt.Errorf("create attestation push action: %w", err) } - if _, err := a.Run(ctx, "", nil, false); err != nil { - return fmt.Errorf("attestation push: %w", err) + res, err := a.Run(ctx, "", nil, false) + if err != nil { + return nil, fmt.Errorf("attestation push: %w", err) } - return nil + return res, nil } // executableInfo returns the CLI version and SHA-256 digest of the running CLI binary diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index ba8db8540..69d8e1e25 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -553,18 +553,18 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts log.Debug().Str("attestation_id", attestationID).Msg("attestation initialized") // Add evidence for each session - var addedCount int + attestedSessions := make([]string, 0, len(evidenceFiles)) for _, ef := range evidenceFiles { name := evidenceName(ef.sessionID) if err := executor.AddEvidence(ctx, name, ef.tmpPath); err != nil { log.Debug().Err(err).Str("session", ef.sessionID).Msg("could not add evidence") continue } - addedCount++ + attestedSessions = append(attestedSessions, ef.sessionID) log.Debug().Str("session", ef.sessionID).Str("name", name).Msg("evidence added") } - if addedCount == 0 { + if len(attestedSessions) == 0 { log.Debug().Msg("no evidence successfully added, resetting attestation") _ = executor.Reset(ctx, "trace-push", "no CHAINLOOP_AI_CODING_SESSION evidence added") @@ -573,10 +573,16 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts // Push attestation log.Debug().Msg("pushing attestation") - if err := executor.Push(ctx); err != nil { + res, err := executor.Push(ctx) + if err != nil { return fmt.Errorf("attestation push: %w", err) } + // Tell the user where each session landed. The organization comes from the + // control plane rather than opts.Organization, which is empty whenever the + // CLI's current org is used. + logAttestedSessions(log, res.UIDashboardURL, res.GetOrganization(), attestedSessions) + log.Debug().Msg("attestation pushed, wiping single-use trace state") // Mark every AI commit included in this attestation as tracked so that a @@ -605,6 +611,20 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts return nil } +// logAttestedSessions reports one line per attested session, carrying a link +// to the session's page when the deployment has a UI dashboard configured. +// Without one the line still names the session, so the user gets confirmation +// of what was recorded either way. +func logAttestedSessions(log zerolog.Logger, uiDashboardURL, orgName string, sessionIDs []string) { + for _, id := range sessionIDs { + ev := log.Info().Str("session", id) + if url := buildSessionViewURL(uiDashboardURL, orgName, id); url != "" { + ev = ev.Str("url", url) + } + ev.Msg("AI coding session attested") + } +} + // evidenceName returns the material name for a session evidence document. // Format: ai-coding-session-. Underscores are // replaced with hyphens because material names may only contain lowercase diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 76fba30d8..f32e8c1e3 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -16,6 +16,8 @@ package action import ( + "bytes" + "encoding/json" "os" "os/exec" "path/filepath" @@ -690,3 +692,65 @@ func requireFileAttribution(t *testing.T, changes *aicodingsession.CodeChanges, } require.Failf(t, "file not found in CodeChanges", "%s (got %v)", path, changes.Files) } + +// sessionLogEntry is the subset of a logAttestedSessions log line under test. +type sessionLogEntry struct { + Session string `json:"session"` + URL string `json:"url"` +} + +func TestLogAttestedSessions(t *testing.T) { + testCases := []struct { + name string + uiDashboardURL string + orgName string + sessionIDs []string + want []sessionLogEntry + }{ + { + name: "one line per session, each linked", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionIDs: []string{testSessionID, "ses_2"}, + want: []sessionLogEntry{ + {Session: testSessionID, URL: testDashboardURL + "/u/chainloop/sessions/" + testSessionID}, + {Session: "ses_2", URL: testDashboardURL + "/u/chainloop/sessions/ses_2"}, + }, + }, + { + name: "no dashboard still confirms the session, without a url", + uiDashboardURL: "", + orgName: testOrgName, + sessionIDs: []string{testSessionID}, + want: []sessionLogEntry{{Session: testSessionID}}, + }, + { + name: "no sessions logs nothing", + uiDashboardURL: testDashboardURL, + orgName: testOrgName, + sessionIDs: nil, + want: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var buf bytes.Buffer + log := zerolog.New(&buf).Level(zerolog.InfoLevel) + + logAttestedSessions(log, tc.uiDashboardURL, tc.orgName, tc.sessionIDs) + + var got []sessionLogEntry + for line := range strings.SplitSeq(strings.TrimSpace(buf.String()), "\n") { + if line == "" { + continue + } + var entry sessionLogEntry + require.NoError(t, json.Unmarshal([]byte(line), &entry)) + got = append(got, entry) + } + + assert.Equal(t, tc.want, got) + }) + } +} From 872f2de1b4413dd695c1642e5e43785401d72662 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 16:23:14 +0200 Subject: [PATCH 02/11] test(cli): strengthen coverage of the trace session link Address review feedback on the session-link tests. The logging test now uses an organization that differs from the other fixtures and that changes under path escaping, so the asserted URL proves the link is built from the supplied organization rather than from an incidental value. Add a table test for AttestationResult.GetOrganization covering the nil links in the Status chain, which is the accessor the logging call depends on. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/action_test.go | 27 +++++++++++++++++++ app/cli/pkg/action/trace_hook_handler_test.go | 21 ++++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/app/cli/pkg/action/action_test.go b/app/cli/pkg/action/action_test.go index 12281981a..f1991c56a 100644 --- a/app/cli/pkg/action/action_test.go +++ b/app/cli/pkg/action/action_test.go @@ -99,6 +99,33 @@ func TestBuildSessionViewURL(t *testing.T) { } } +// TestAttestationResultGetOrganization covers the accessor RunTracePush feeds +// into the session link, including the nil links in the Status chain. +func TestAttestationResultGetOrganization(t *testing.T) { + testCases := []struct { + name string + res *AttestationResult + want string + }{ + {name: "nil result", res: nil}, + {name: "nil status", res: &AttestationResult{}}, + {name: "nil workflow meta", res: &AttestationResult{Status: &AttestationStatusResult{}}}, + { + name: "organization reported by the control plane", + res: &AttestationResult{Status: &AttestationStatusResult{ + WorkflowMeta: &AttestationStatusWorkflowMeta{Organization: testOrgName}, + }}, + want: testOrgName, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, tc.res.GetOrganization()) + }) + } +} + // TestBuildAttestationViewURL guards the route the session link now shares a // builder with, so the refactor cannot silently change it. func TestBuildAttestationViewURL(t *testing.T) { diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index f32e8c1e3..6674d71d4 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -700,6 +700,15 @@ type sessionLogEntry struct { } func TestLogAttestedSessions(t *testing.T) { + // Deliberately not testOrgName: an organization that differs from the one + // used elsewhere, and that changes under escaping, proves the logged URL + // is built from the argument rather than from any incidental value. + const ( + orgName = "acme corp" + orgInPath = "acme%20corp" + sessionURL = testDashboardURL + "/u/" + orgInPath + "/sessions/" + ) + testCases := []struct { name string uiDashboardURL string @@ -708,26 +717,26 @@ func TestLogAttestedSessions(t *testing.T) { want []sessionLogEntry }{ { - name: "one line per session, each linked", + name: "one line per session, each linked to the given org", uiDashboardURL: testDashboardURL, - orgName: testOrgName, + orgName: orgName, sessionIDs: []string{testSessionID, "ses_2"}, want: []sessionLogEntry{ - {Session: testSessionID, URL: testDashboardURL + "/u/chainloop/sessions/" + testSessionID}, - {Session: "ses_2", URL: testDashboardURL + "/u/chainloop/sessions/ses_2"}, + {Session: testSessionID, URL: sessionURL + testSessionID}, + {Session: "ses_2", URL: sessionURL + "ses_2"}, }, }, { name: "no dashboard still confirms the session, without a url", uiDashboardURL: "", - orgName: testOrgName, + orgName: orgName, sessionIDs: []string{testSessionID}, want: []sessionLogEntry{{Session: testSessionID}}, }, { name: "no sessions logs nothing", uiDashboardURL: testDashboardURL, - orgName: testOrgName, + orgName: orgName, sessionIDs: nil, want: nil, }, From b076941e937e963a8276e4ec0c445834c4c8dc89 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 17:18:20 +0200 Subject: [PATCH 03/11] refactor(cli): reword the trace session line around the link Use "Coding Session Available at " so the line reads as a sentence and the link is the last thing on it, which keeps it clickable in a terminal. The link is inline rather than a structured field, and the session field is dropped on that path because the URL already ends in the session ID. Without a dashboard the line stays a bare confirmation naming the session, since there is no link to point at. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/trace_hook_handler.go | 17 ++++++++++------- app/cli/pkg/action/trace_hook_handler_test.go | 11 ++++++----- 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 69d8e1e25..6de2913d0 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -611,17 +611,20 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts return nil } -// logAttestedSessions reports one line per attested session, carrying a link -// to the session's page when the deployment has a UI dashboard configured. -// Without one the line still names the session, so the user gets confirmation -// of what was recorded either way. +// logAttestedSessions reports one line per attested session. When the +// deployment has a UI dashboard configured the line points at the session's +// page, with the link inline so it reads as a sentence and stays clickable in +// a terminal. Without a dashboard the line still names the session, so the +// user gets confirmation of what was recorded either way. func logAttestedSessions(log zerolog.Logger, uiDashboardURL, orgName string, sessionIDs []string) { for _, id := range sessionIDs { - ev := log.Info().Str("session", id) + // The link already ends in the session ID, so a session field + // alongside it would only repeat itself in the rendered line. if url := buildSessionViewURL(uiDashboardURL, orgName, id); url != "" { - ev = ev.Str("url", url) + log.Info().Msg("Coding Session Available at " + url) + continue } - ev.Msg("AI coding session attested") + log.Info().Str("session", id).Msg("Coding session attested") } } diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 6674d71d4..793be9e20 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -696,7 +696,7 @@ func requireFileAttribution(t *testing.T, changes *aicodingsession.CodeChanges, // sessionLogEntry is the subset of a logAttestedSessions log line under test. type sessionLogEntry struct { Session string `json:"session"` - URL string `json:"url"` + Message string `json:"message"` } func TestLogAttestedSessions(t *testing.T) { @@ -707,6 +707,7 @@ func TestLogAttestedSessions(t *testing.T) { orgName = "acme corp" orgInPath = "acme%20corp" sessionURL = testDashboardURL + "/u/" + orgInPath + "/sessions/" + linkPrefix = "Coding Session Available at " ) testCases := []struct { @@ -722,16 +723,16 @@ func TestLogAttestedSessions(t *testing.T) { orgName: orgName, sessionIDs: []string{testSessionID, "ses_2"}, want: []sessionLogEntry{ - {Session: testSessionID, URL: sessionURL + testSessionID}, - {Session: "ses_2", URL: sessionURL + "ses_2"}, + {Message: linkPrefix + sessionURL + testSessionID}, + {Message: linkPrefix + sessionURL + "ses_2"}, }, }, { - name: "no dashboard still confirms the session, without a url", + name: "no dashboard still confirms the session, without a link", uiDashboardURL: "", orgName: orgName, sessionIDs: []string{testSessionID}, - want: []sessionLogEntry{{Session: testSessionID}}, + want: []sessionLogEntry{{Session: testSessionID, Message: "Coding session attested"}}, }, { name: "no sessions logs nothing", From 0bfa06b121d5cd0704a9c6f3d8373124c2d9b178 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 18:04:51 +0200 Subject: [PATCH 04/11] feat(cli): surface the trace session link through the coding agent When a coding agent runs the git push, the pre-push hook's session link is captured into that agent's shell-tool output rather than shown to the user, who is the audience for it. A successful push now records its session links in the trace directory, which survives the post-push state wipe. The next hook after a shell command takes them, hands them to the agent, and clears them, so a link is announced at most once. Claude Code's provider emits the message on both of its delivery channels: one the client prints directly, one the model can repeat. Cursor has no hook after a shell command; OpenCode's payload shape is not verified yet, so both are no-ops for now. Links expire after ten minutes, because a push the user ran in their own terminal leaves a record no agent hook will ever consume. A push driven by `chainloop trace run` records nothing at all: it completes after the wrapped agent has exited, so no hook of that agent can fire again. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- .../internal/trace/claude/announce_test.go | 96 +++++++++++++ app/cli/internal/trace/claude/provider.go | 33 +++++ app/cli/internal/trace/cursor/provider.go | 7 + app/cli/internal/trace/opencode/provider.go | 8 ++ app/cli/internal/trace/provider.go | 9 ++ app/cli/internal/trace/state/pendinglinks.go | 95 +++++++++++++ .../internal/trace/state/pendinglinks_test.go | 128 ++++++++++++++++++ app/cli/pkg/action/trace_agent_hook.go | 34 +++++ app/cli/pkg/action/trace_hook_handler.go | 34 ++++- app/cli/pkg/action/trace_hook_handler_test.go | 13 +- app/cli/pkg/action/trace_notify_links_test.go | 108 +++++++++++++++ 11 files changed, 561 insertions(+), 4 deletions(-) create mode 100644 app/cli/internal/trace/claude/announce_test.go create mode 100644 app/cli/internal/trace/state/pendinglinks.go create mode 100644 app/cli/internal/trace/state/pendinglinks_test.go create mode 100644 app/cli/pkg/action/trace_notify_links_test.go diff --git a/app/cli/internal/trace/claude/announce_test.go b/app/cli/internal/trace/claude/announce_test.go new file mode 100644 index 000000000..02515d797 --- /dev/null +++ b/app/cli/internal/trace/claude/announce_test.go @@ -0,0 +1,96 @@ +// +// 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 claude + +import ( + "encoding/json" + "io" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAnnounceToUser pins the wire shape Claude Code expects. systemMessage +// must stay top-level: nested inside hookSpecificOutput it is silently ignored. +func TestAnnounceToUser(t *testing.T) { + const msg = "Coding Session Available at https://app.chainloop.dev/u/chainloop/sessions/ses_1" + + testCases := []struct { + name string + msg string + wantEmitted bool + }{ + { + name: "a message goes out on both channels", + msg: msg, + wantEmitted: true, + }, + { + name: "nothing to say emits nothing", + msg: "", + wantEmitted: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + out := captureStdout(t, func() { + require.NoError(t, New().AnnounceToUser(tc.msg)) + }) + + if !tc.wantEmitted { + assert.Empty(t, out, "no message means no stdout, so the hook stays a no-op") + return + } + + var got map[string]any + require.NoError(t, json.Unmarshal([]byte(out), &got)) + + assert.Equal(t, tc.msg, got["systemMessage"], "systemMessage must be top-level") + + hookOut, ok := got["hookSpecificOutput"].(map[string]any) + require.True(t, ok, "hookSpecificOutput must be present") + assert.Equal(t, "PostToolUse", hookOut["hookEventName"]) + + // The model needs the message verbatim to be able to repeat it. + assert.Contains(t, hookOut["additionalContext"], tc.msg) + }) + } +} + +// captureStdout runs fn with os.Stdout redirected to a pipe and returns +// everything written to it. Reads to EOF rather than into a fixed buffer: a +// truncated read would corrupt the payload these tests parse as JSON. +func captureStdout(t *testing.T, fn func()) string { + t.Helper() + + orig := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout = w + t.Cleanup(func() { os.Stdout = orig }) + + fn() + require.NoError(t, w.Close()) + + out, err := io.ReadAll(r) + require.NoError(t, err) + require.NoError(t, r.Close()) + + return string(out) +} diff --git a/app/cli/internal/trace/claude/provider.go b/app/cli/internal/trace/claude/provider.go index 388952737..14ffbc621 100644 --- a/app/cli/internal/trace/claude/provider.go +++ b/app/cli/internal/trace/claude/provider.go @@ -159,6 +159,39 @@ func (p *Provider) SystemMessage(msg string) error { return json.NewEncoder(os.Stdout).Encode(resp) } +// AnnounceToUser emits a PostToolUse hook response on both of Claude Code's +// delivery channels: systemMessage, which the client prints to the user +// without involving the model, and additionalContext, which reaches the model +// so it can repeat the message in its own reply. +// +// Both are used because only the second is confirmed to render in every +// build. Should systemMessage prove universally reliable, dropping +// additionalContext here would spare the model a turn, and this is the one +// place that would have to change. +func (p *Provider) AnnounceToUser(msg string) error { + if msg == "" { + return nil + } + + type hookSpecificOutput struct { + HookEventName string `json:"hookEventName"` + AdditionalContext string `json:"additionalContext,omitempty"` + } + + resp := struct { + SystemMessage string `json:"systemMessage"` + HookSpecificOutput hookSpecificOutput `json:"hookSpecificOutput"` + }{ + SystemMessage: msg, + HookSpecificOutput: hookSpecificOutput{ + HookEventName: eventPostToolUse, + AdditionalContext: "Tell the user the following, including any link verbatim: " + msg, + }, + } + + return json.NewEncoder(os.Stdout).Encode(resp) +} + // ParseSession parses a Claude Code session JSONL and returns structured evidence. func (p *Provider) ParseSession(_ context.Context, opts *trace.ParseOpts) (*aicodingsession.Evidence, error) { jsonlPath, err := findJSONLPath(opts.SessionDir, opts.SessionID) diff --git a/app/cli/internal/trace/cursor/provider.go b/app/cli/internal/trace/cursor/provider.go index dd41f64fe..2efc92177 100644 --- a/app/cli/internal/trace/cursor/provider.go +++ b/app/cli/internal/trace/cursor/provider.go @@ -100,6 +100,13 @@ func (p *Provider) SystemMessage(_ string) error { return nil } +// AnnounceToUser is a no-op for Cursor: it installs only sessionStart, +// sessionEnd and afterFileEdit, so no hook fires after a shell command and +// there is nowhere to deliver the message. +func (p *Provider) AnnounceToUser(_ string) error { + return nil +} + // CaptureFileSnapshot is a no-op for Cursor: the afterFileEdit hook // delivers old/new strings directly, so no pre-edit snapshot is needed. func (p *Provider) CaptureFileSnapshot(_ *state.Store, _ *trace.HookInput) error { diff --git a/app/cli/internal/trace/opencode/provider.go b/app/cli/internal/trace/opencode/provider.go index 3b61a1cfd..f63217943 100644 --- a/app/cli/internal/trace/opencode/provider.go +++ b/app/cli/internal/trace/opencode/provider.go @@ -164,6 +164,14 @@ func (p *Provider) SystemMessage(_ string) error { return nil } +// AnnounceToUser is a no-op for OpenCode until its plugin's response shape +// for surfacing a message is verified against a live session, the way Claude +// Code's was. The hook after a shell command already fires, so wiring this up +// later is a change to this method alone. +func (p *Provider) AnnounceToUser(_ string) error { + return nil +} + // ParseSession reads the copied export JSON for sessionID and returns // structured evidence. func (p *Provider) ParseSession(_ context.Context, opts *trace.ParseOpts) (*aicodingsession.Evidence, error) { diff --git a/app/cli/internal/trace/provider.go b/app/cli/internal/trace/provider.go index d1e12099f..a447ad29b 100644 --- a/app/cli/internal/trace/provider.go +++ b/app/cli/internal/trace/provider.go @@ -97,6 +97,15 @@ type Provider interface { // SystemMessage writes a message to stdout for the agent to display on session start. SystemMessage(msg string) error + + // AnnounceToUser writes a hook response to stdout so the agent puts msg + // in front of the user, after a shell command the agent ran. Which + // channel that uses is the provider's business: agents differ in whether + // they render text directly, relay it through the model, or both. + // + // Providers whose agent has no hook after a shell command implement this + // as a no-op. + AnnounceToUser(msg string) error } // HookInput represents parsed hook invocation data from an AI agent. diff --git a/app/cli/internal/trace/state/pendinglinks.go b/app/cli/internal/trace/state/pendinglinks.go new file mode 100644 index 000000000..7789afdb1 --- /dev/null +++ b/app/cli/internal/trace/state/pendinglinks.go @@ -0,0 +1,95 @@ +// +// 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 state + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "time" +) + +const ( + // pendingLinksFile holds session links produced by a successful trace + // push, waiting for an agent hook to show them to the user. It lives + // directly in the trace directory, which WipeTraceDir preserves. + pendingLinksFile = "pending-links.json" + + // pendingLinksTTL bounds how long a recorded link stays announceable. + // A push run by the user in their own terminal is never consumed by an + // agent hook, and without an expiry the next agent command, possibly in + // a later session, would announce a link the user has long since seen. + pendingLinksTTL = 10 * time.Minute +) + +// pendingLinks is the on-disk record. SavedAt is RFC3339 UTC, matching the +// timestamp format used by the other records in this package. +type pendingLinks struct { + Links []string `json:"links"` + SavedAt string `json:"saved_at"` +} + +// SavePendingLinks records session links for an agent hook to surface to the +// user. An empty list writes nothing: there is nothing to announce, and a +// leftover empty record would only have to be cleaned up later. +func (s *Store) SavePendingLinks(links []string) error { + if len(links) == 0 { + return nil + } + + data, err := json.Marshal(pendingLinks{Links: links, SavedAt: NowTimestamp()}) + if err != nil { + return fmt.Errorf("encode pending links: %w", err) + } + + base := s.traceDirPath() + if err := os.MkdirAll(base, 0o755); err != nil { + return fmt.Errorf("create trace directory: %w", err) + } + + return os.WriteFile(filepath.Join(base, pendingLinksFile), data, 0o600) +} + +// TakePendingLinks returns the recorded session links and clears the record, +// so a link is announced at most once. It returns nil when there is nothing +// recorded, when the record has expired, or when it cannot be read: this +// feeds a cosmetic notification, and no failure here is worth surfacing to +// the caller, let alone failing an agent's tool call over. +// +// The record is cleared even when it could not be parsed, so a corrupt file +// does not wedge the mechanism for every later push. +func (s *Store) TakePendingLinks() []string { + path := filepath.Join(s.traceDirPath(), pendingLinksFile) + + data, err := os.ReadFile(path) + if err != nil { + return nil + } + _ = removeIfExists(path) + + var rec pendingLinks + if err := json.Unmarshal(data, &rec); err != nil { + return nil + } + + savedAt, err := time.Parse(time.RFC3339, rec.SavedAt) + if err != nil || time.Since(savedAt) > pendingLinksTTL { + return nil + } + + return rec.Links +} diff --git a/app/cli/internal/trace/state/pendinglinks_test.go b/app/cli/internal/trace/state/pendinglinks_test.go new file mode 100644 index 000000000..a2d85b863 --- /dev/null +++ b/app/cli/internal/trace/state/pendinglinks_test.go @@ -0,0 +1,128 @@ +// +// 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 state + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTakePendingLinks(t *testing.T) { + links := []string{ + "https://app.chainloop.dev/u/chainloop/sessions/ses_1", + "https://app.chainloop.dev/u/chainloop/sessions/ses_2", + } + + testCases := []struct { + name string + // age of the saved record; zero means "just written" + age time.Duration + links []string + want []string + }{ + { + name: "an empty list is never saved, so nothing comes back", + links: nil, + want: nil, + }, + { + name: "fresh record is returned", + links: links, + want: links, + }, + { + name: "record just inside the TTL is returned", + age: pendingLinksTTL - time.Minute, + links: links, + want: links, + }, + { + name: "record past the TTL is discarded", + age: pendingLinksTTL + time.Minute, + links: links, + want: nil, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + store := NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + + require.NoError(t, store.SavePendingLinks(tc.links)) + if tc.age > 0 { + ageRecord(t, store, tc.age) + } + + got := store.TakePendingLinks() + assert.Equal(t, tc.want, got) + + // Whatever the outcome, nothing is left behind to announce twice. + assert.Empty(t, store.TakePendingLinks(), "links must be consumed exactly once") + }) + } +} + +// ageRecord rewrites the saved record with a timestamp shifted into the past, +// so TTL behaviour is exercised without sleeping. +func ageRecord(t *testing.T, store *Store, age time.Duration) { + t.Helper() + + path := filepath.Join(store.traceDirPath(), pendingLinksFile) + raw, err := os.ReadFile(path) + require.NoError(t, err) + + rec := pendingLinks{} + require.NoError(t, json.Unmarshal(raw, &rec)) + rec.SavedAt = time.Now().UTC().Add(-age).Format(time.RFC3339) + + out, err := json.Marshal(rec) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, out, 0o600)) +} + +// TestPendingLinksSurviveWipe pins the assumption the agent-notification +// handoff rests on: the push saves the links and then wipes single-use trace +// state, so a wipe that also cleared this record would silently swallow every +// notification. +func TestPendingLinksSurviveWipe(t *testing.T) { + store := NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + + links := []string{"https://app.chainloop.dev/u/chainloop/sessions/ses_1"} + require.NoError(t, store.SavePendingLinks(links)) + + require.NoError(t, store.WipeTraceDir()) + + assert.Equal(t, links, store.TakePendingLinks(), "links must outlive the post-push wipe") +} + +func TestTakePendingLinksIgnoresCorruptRecord(t *testing.T) { + store := NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + + path := filepath.Join(store.traceDirPath(), pendingLinksFile) + require.NoError(t, os.WriteFile(path, []byte("{not json"), 0o600)) + + assert.Empty(t, store.TakePendingLinks(), "a corrupt record must not surface links") + assert.NoFileExists(t, path, "a corrupt record must still be cleared") +} diff --git a/app/cli/pkg/action/trace_agent_hook.go b/app/cli/pkg/action/trace_agent_hook.go index 5ef185551..00cf57861 100644 --- a/app/cli/pkg/action/trace_agent_hook.go +++ b/app/cli/pkg/action/trace_agent_hook.go @@ -19,6 +19,7 @@ import ( "errors" "os" "path/filepath" + "strings" "github.com/chainloop-dev/chainloop/app/cli/internal/trace" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/attribution" @@ -173,6 +174,33 @@ func ensureSessionTracked(provider trace.Provider, store *state.Store, repoRoot } } +// notifyPendingSessionLinks hands any session links left by a just-completed +// trace push to the agent, so it can put them in front of the user. Links are +// consumed on read, so a link is announced at most once. +// +// Best effort throughout: this is a notification, and neither a missing link +// nor a provider that cannot deliver one is worth failing an agent's tool +// call over. +func notifyPendingSessionLinks(provider trace.Provider, store *state.Store, log zerolog.Logger) { + links := store.TakePendingLinks() + if len(links) == 0 { + return + } + + // Same wording as the pre-push log line, so the two channels match. + lines := make([]string, 0, len(links)) + for _, link := range links { + lines = append(lines, sessionLinkMessage(link)) + } + + if err := provider.AnnounceToUser(strings.Join(lines, "\n")); err != nil { + log.Debug().Err(err).Msg("could not surface session links through the agent") + return + } + + log.Debug().Int("links", len(links)).Msg("session links handed to the agent") +} + // HandleAgentPostToolUse handles post-edit hooks across providers // (Claude's post-tool-use, Cursor's afterFileEdit) and records AI-attributed // line ranges for the edited file. @@ -213,6 +241,12 @@ func HandleAgentPostToolUse(provider trace.Provider, log zerolog.Logger) error { // every file the command changed to the AI. recordCommandLineRanges(store, repoRoot, sessionID, log) + // The command may have been a `git push`, whose pre-push hook attested + // a session and left its link behind. Show it now: the pre-push output + // went to this tool call's captured stderr, which the user does not + // necessarily read. + notifyPendingSessionLinks(provider, store, log) + return nil } diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 6de2913d0..30b2d1eea 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -581,7 +581,23 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts // Tell the user where each session landed. The organization comes from the // control plane rather than opts.Organization, which is empty whenever the // CLI's current org is used. - logAttestedSessions(log, res.UIDashboardURL, res.GetOrganization(), attestedSessions) + links := logAttestedSessions(log, res.UIDashboardURL, res.GetOrganization(), attestedSessions) + + // Hand the links to the agent hook that runs after this push. When the + // push was driven by a coding agent's shell tool, the log line above is + // captured into that tool's output rather than shown to the user, so the + // hook is what actually puts the link in front of them. A failure here + // costs a notification, never the attestation that already succeeded. + // + // Skipped under `chainloop trace run`, which reaches this point only + // after the agent it wrapped has exited: no hook of that agent can fire + // again, its terminal already showed the line above, and the record + // would just sit there waiting to be announced by an unrelated session. + if store.IsTraceRunActive() { + log.Debug().Msg("trace run owns this push; its terminal already showed the links") + } else if err := store.SavePendingLinks(links); err != nil { + log.Debug().Err(err).Msg("could not record session links for the agent hook") + } log.Debug().Msg("attestation pushed, wiping single-use trace state") @@ -616,16 +632,28 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts // page, with the link inline so it reads as a sentence and stays clickable in // a terminal. Without a dashboard the line still names the session, so the // user gets confirmation of what was recorded either way. -func logAttestedSessions(log zerolog.Logger, uiDashboardURL, orgName string, sessionIDs []string) { +// It returns the links it logged, so the caller can hand them to the agent +// hook that will show them to the user. +func logAttestedSessions(log zerolog.Logger, uiDashboardURL, orgName string, sessionIDs []string) []string { + links := make([]string, 0, len(sessionIDs)) for _, id := range sessionIDs { // The link already ends in the session ID, so a session field // alongside it would only repeat itself in the rendered line. if url := buildSessionViewURL(uiDashboardURL, orgName, id); url != "" { - log.Info().Msg("Coding Session Available at " + url) + log.Info().Msg(sessionLinkMessage(url)) + links = append(links, url) continue } log.Info().Str("session", id).Msg("Coding session attested") } + + return links +} + +// sessionLinkMessage is the one place the user-facing wording lives, so the +// pre-push log line and the agent's notification cannot drift apart. +func sessionLinkMessage(url string) string { + return "Coding Session Available at " + url } // evidenceName returns the material name for a session evidence document. diff --git a/app/cli/pkg/action/trace_hook_handler_test.go b/app/cli/pkg/action/trace_hook_handler_test.go index 793be9e20..4e9482f52 100644 --- a/app/cli/pkg/action/trace_hook_handler_test.go +++ b/app/cli/pkg/action/trace_hook_handler_test.go @@ -748,7 +748,18 @@ func TestLogAttestedSessions(t *testing.T) { var buf bytes.Buffer log := zerolog.New(&buf).Level(zerolog.InfoLevel) - logAttestedSessions(log, tc.uiDashboardURL, tc.orgName, tc.sessionIDs) + gotLinks := logAttestedSessions(log, tc.uiDashboardURL, tc.orgName, tc.sessionIDs) + + // The returned links are what gets handed to the agent hook, so + // they must match the links that were logged, and nothing is + // returned when there is no dashboard to link to. + wantLinks := []string{} + for _, w := range tc.want { + if link, found := strings.CutPrefix(w.Message, linkPrefix); found { + wantLinks = append(wantLinks, link) + } + } + assert.Equal(t, wantLinks, gotLinks) var got []sessionLogEntry for line := range strings.SplitSeq(strings.TrimSpace(buf.String()), "\n") { diff --git a/app/cli/pkg/action/trace_notify_links_test.go b/app/cli/pkg/action/trace_notify_links_test.go new file mode 100644 index 000000000..3848a2892 --- /dev/null +++ b/app/cli/pkg/action/trace_notify_links_test.go @@ -0,0 +1,108 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package action + +import ( + "errors" + "testing" + + "github.com/chainloop-dev/chainloop/app/cli/internal/trace" + "github.com/chainloop-dev/chainloop/app/cli/internal/trace/state" + "github.com/rs/zerolog" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordingProvider captures what notifyPendingSessionLinks hands to the agent. +type recordingProvider struct { + trace.Provider + + calls int + announced string + err error +} + +func (p *recordingProvider) AnnounceToUser(msg string) error { + p.calls++ + p.announced = msg + + return p.err +} + +func TestNotifyPendingSessionLinks(t *testing.T) { + const ( + link1 = "https://app.chainloop.dev/u/chainloop/sessions/ses_1" + link2 = "https://app.chainloop.dev/u/chainloop/sessions/ses_2" + ) + + testCases := []struct { + name string + saved []string + providerErr error + wantCalls int + wantUserMessage string + }{ + { + name: "nothing pending leaves the agent alone", + saved: nil, + wantCalls: 0, + }, + { + name: "one link is announced with the log line's wording", + saved: []string{link1}, + wantCalls: 1, + wantUserMessage: "Coding Session Available at " + link1, + }, + { + name: "several links are announced one per line", + saved: []string{link1, link2}, + wantCalls: 1, + wantUserMessage: "Coding Session Available at " + link1 + "\n" + + "Coding Session Available at " + link2, + }, + { + name: "a provider that cannot deliver is not fatal", + saved: []string{link1}, + providerErr: errors.New("stdout closed"), + wantCalls: 1, + wantUserMessage: "Coding Session Available at " + link1, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + store := state.NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + require.NoError(t, store.SavePendingLinks(tc.saved)) + + p := &recordingProvider{err: tc.providerErr} + + // Must not panic or propagate; it only ever notifies. + notifyPendingSessionLinks(p, store, zerolog.Nop()) + + assert.Equal(t, tc.wantCalls, p.calls) + assert.Equal(t, tc.wantUserMessage, p.announced) + + // Announced once: a second hook firing must stay silent, even + // when the first delivery failed. Re-announcing a link the user + // has already seen on every later shell command is worse than + // dropping one. + before := p.calls + notifyPendingSessionLinks(p, store, zerolog.Nop()) + assert.Equal(t, before, p.calls, "links must be consumed exactly once") + }) + } +} From 6334c22052939f2f3143c69bb227d5d2901e52bc Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 18:18:57 +0200 Subject: [PATCH 05/11] fix(cli): keep session links an agent could not show Address review feedback. Reading the recorded links no longer consumes them: an agent with no way to reach the user was clearing the record it could not act on, so the link was lost without anyone seeing it. Providers that cannot reach the user now say so with ErrAnnounceUnsupported rather than reporting success, and the record is cleared only once an agent has had its go at showing it. A delivery that was attempted and failed still consumes, because retrying on every later shell command would nag far longer than one dropped notification costs. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/internal/trace/cursor/provider.go | 4 +-- app/cli/internal/trace/opencode/provider.go | 12 ++++--- app/cli/internal/trace/provider.go | 12 +++++-- app/cli/internal/trace/state/pendinglinks.go | 26 +++++++++----- .../internal/trace/state/pendinglinks_test.go | 18 ++++++---- app/cli/pkg/action/trace_agent_hook.go | 21 +++++++++-- app/cli/pkg/action/trace_notify_links_test.go | 35 +++++++++++++++---- 7 files changed, 95 insertions(+), 33 deletions(-) diff --git a/app/cli/internal/trace/cursor/provider.go b/app/cli/internal/trace/cursor/provider.go index 2efc92177..8d5ad8e39 100644 --- a/app/cli/internal/trace/cursor/provider.go +++ b/app/cli/internal/trace/cursor/provider.go @@ -100,11 +100,11 @@ func (p *Provider) SystemMessage(_ string) error { return nil } -// AnnounceToUser is a no-op for Cursor: it installs only sessionStart, +// AnnounceToUser is unsupported for Cursor: it installs only sessionStart, // sessionEnd and afterFileEdit, so no hook fires after a shell command and // there is nowhere to deliver the message. func (p *Provider) AnnounceToUser(_ string) error { - return nil + return trace.ErrAnnounceUnsupported } // CaptureFileSnapshot is a no-op for Cursor: the afterFileEdit hook diff --git a/app/cli/internal/trace/opencode/provider.go b/app/cli/internal/trace/opencode/provider.go index f63217943..9a5452406 100644 --- a/app/cli/internal/trace/opencode/provider.go +++ b/app/cli/internal/trace/opencode/provider.go @@ -164,12 +164,14 @@ func (p *Provider) SystemMessage(_ string) error { return nil } -// AnnounceToUser is a no-op for OpenCode until its plugin's response shape -// for surfacing a message is verified against a live session, the way Claude -// Code's was. The hook after a shell command already fires, so wiring this up -// later is a change to this method alone. +// AnnounceToUser is unsupported for OpenCode until its plugin's response +// shape for surfacing a message is verified against a live session, the way +// Claude Code's was. The hook after a shell command already fires, so wiring +// this up later is a change to this method alone. Reporting it as +// unsupported rather than silently succeeding keeps callers from discarding +// content this agent never showed anyone. func (p *Provider) AnnounceToUser(_ string) error { - return nil + return trace.ErrAnnounceUnsupported } // ParseSession reads the copied export JSON for sessionID and returns diff --git a/app/cli/internal/trace/provider.go b/app/cli/internal/trace/provider.go index a447ad29b..b8a192de1 100644 --- a/app/cli/internal/trace/provider.go +++ b/app/cli/internal/trace/provider.go @@ -17,12 +17,19 @@ package trace import ( "context" + "errors" "io" "github.com/chainloop-dev/chainloop/app/cli/internal/trace/state" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials/aicodingsession" ) +// ErrAnnounceUnsupported is returned by AnnounceToUser when the agent has no +// channel for showing the user a message. It means nothing was displayed, as +// opposed to a delivery that was attempted and failed, so a caller holding +// single-use content can keep it rather than throw it away unseen. +var ErrAnnounceUnsupported = errors.New("agent cannot show messages to the user") + // Provider discovers and parses AI coding sessions for a specific agent. // // Providers are stateless singletons from a registry, so the state-touching @@ -103,8 +110,9 @@ type Provider interface { // channel that uses is the provider's business: agents differ in whether // they render text directly, relay it through the model, or both. // - // Providers whose agent has no hook after a shell command implement this - // as a no-op. + // Providers with no way to reach the user return ErrAnnounceUnsupported, + // so callers can tell "shown" apart from "nothing happened" and avoid + // discarding a message nobody saw. AnnounceToUser(msg string) error } diff --git a/app/cli/internal/trace/state/pendinglinks.go b/app/cli/internal/trace/state/pendinglinks.go index 7789afdb1..4c694f579 100644 --- a/app/cli/internal/trace/state/pendinglinks.go +++ b/app/cli/internal/trace/state/pendinglinks.go @@ -64,32 +64,40 @@ func (s *Store) SavePendingLinks(links []string) error { return os.WriteFile(filepath.Join(base, pendingLinksFile), data, 0o600) } -// TakePendingLinks returns the recorded session links and clears the record, -// so a link is announced at most once. It returns nil when there is nothing -// recorded, when the record has expired, or when it cannot be read: this -// feeds a cosmetic notification, and no failure here is worth surfacing to -// the caller, let alone failing an agent's tool call over. +// PendingLinks returns the recorded session links without consuming them, so +// a caller that turns out to be unable to show them leaves them for whoever +// can. Call ClearPendingLinks once they have actually been shown. // -// The record is cleared even when it could not be parsed, so a corrupt file -// does not wedge the mechanism for every later push. -func (s *Store) TakePendingLinks() []string { +// It returns nil when there is nothing recorded, when the record has expired, +// or when it cannot be read: this feeds a cosmetic notification, and no +// failure here is worth surfacing to the caller, let alone failing an agent's +// tool call over. An unreadable or expired record is cleared on the spot, +// since nobody will ever be able to use it and a corrupt file would otherwise +// wedge the mechanism for every later push. +func (s *Store) PendingLinks() []string { path := filepath.Join(s.traceDirPath(), pendingLinksFile) data, err := os.ReadFile(path) if err != nil { return nil } - _ = removeIfExists(path) var rec pendingLinks if err := json.Unmarshal(data, &rec); err != nil { + _ = removeIfExists(path) return nil } savedAt, err := time.Parse(time.RFC3339, rec.SavedAt) if err != nil || time.Since(savedAt) > pendingLinksTTL { + _ = removeIfExists(path) return nil } return rec.Links } + +// ClearPendingLinks drops the record, so its links are shown at most once. +func (s *Store) ClearPendingLinks() error { + return removeIfExists(filepath.Join(s.traceDirPath(), pendingLinksFile)) +} diff --git a/app/cli/internal/trace/state/pendinglinks_test.go b/app/cli/internal/trace/state/pendinglinks_test.go index a2d85b863..2c126d0db 100644 --- a/app/cli/internal/trace/state/pendinglinks_test.go +++ b/app/cli/internal/trace/state/pendinglinks_test.go @@ -26,7 +26,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestTakePendingLinks(t *testing.T) { +func TestPendingLinks(t *testing.T) { links := []string{ "https://app.chainloop.dev/u/chainloop/sessions/ses_1", "https://app.chainloop.dev/u/chainloop/sessions/ses_2", @@ -73,11 +73,15 @@ func TestTakePendingLinks(t *testing.T) { ageRecord(t, store, tc.age) } - got := store.TakePendingLinks() + got := store.PendingLinks() assert.Equal(t, tc.want, got) - // Whatever the outcome, nothing is left behind to announce twice. - assert.Empty(t, store.TakePendingLinks(), "links must be consumed exactly once") + // Reading does not consume: a caller that turns out to be unable + // to show them must leave them for one that can. + assert.Equal(t, tc.want, store.PendingLinks(), "reading must not consume") + + require.NoError(t, store.ClearPendingLinks()) + assert.Empty(t, store.PendingLinks(), "clearing must consume") }) } } @@ -113,16 +117,16 @@ func TestPendingLinksSurviveWipe(t *testing.T) { require.NoError(t, store.WipeTraceDir()) - assert.Equal(t, links, store.TakePendingLinks(), "links must outlive the post-push wipe") + assert.Equal(t, links, store.PendingLinks(), "links must outlive the post-push wipe") } -func TestTakePendingLinksIgnoresCorruptRecord(t *testing.T) { +func TestPendingLinksIgnoresCorruptRecord(t *testing.T) { store := NewGitStore(t.TempDir()) require.NoError(t, store.InitTraceDir()) path := filepath.Join(store.traceDirPath(), pendingLinksFile) require.NoError(t, os.WriteFile(path, []byte("{not json"), 0o600)) - assert.Empty(t, store.TakePendingLinks(), "a corrupt record must not surface links") + assert.Empty(t, store.PendingLinks(), "a corrupt record must not surface links") assert.NoFileExists(t, path, "a corrupt record must still be cleared") } diff --git a/app/cli/pkg/action/trace_agent_hook.go b/app/cli/pkg/action/trace_agent_hook.go index 00cf57861..09cf15752 100644 --- a/app/cli/pkg/action/trace_agent_hook.go +++ b/app/cli/pkg/action/trace_agent_hook.go @@ -182,7 +182,7 @@ func ensureSessionTracked(provider trace.Provider, store *state.Store, repoRoot // nor a provider that cannot deliver one is worth failing an agent's tool // call over. func notifyPendingSessionLinks(provider trace.Provider, store *state.Store, log zerolog.Logger) { - links := store.TakePendingLinks() + links := store.PendingLinks() if len(links) == 0 { return } @@ -193,8 +193,25 @@ func notifyPendingSessionLinks(provider trace.Provider, store *state.Store, log lines = append(lines, sessionLinkMessage(link)) } - if err := provider.AnnounceToUser(strings.Join(lines, "\n")); err != nil { + err := provider.AnnounceToUser(strings.Join(lines, "\n")) + if errors.Is(err, trace.ErrAnnounceUnsupported) { + // This agent has no way to show them, so leave them for one that + // might. Their expiry bounds how long they can linger. + log.Debug().Msg("agent cannot show messages; leaving the session links for later") + + return + } + + // Any other outcome consumed the attempt, success or not. Clearing on a + // failed delivery is deliberate: retrying on every later shell command + // would nag far longer than one dropped notification costs. + if clearErr := store.ClearPendingLinks(); clearErr != nil { + log.Debug().Err(clearErr).Msg("could not clear the recorded session links") + } + + if err != nil { log.Debug().Err(err).Msg("could not surface session links through the agent") + return } diff --git a/app/cli/pkg/action/trace_notify_links_test.go b/app/cli/pkg/action/trace_notify_links_test.go index 3848a2892..a35cd8b99 100644 --- a/app/cli/pkg/action/trace_notify_links_test.go +++ b/app/cli/pkg/action/trace_notify_links_test.go @@ -54,6 +54,9 @@ func TestNotifyPendingSessionLinks(t *testing.T) { providerErr error wantCalls int wantUserMessage string + // wantKept is true when the links must survive for another agent to + // show, rather than being consumed by this attempt. + wantKept bool }{ { name: "nothing pending leaves the agent alone", @@ -74,12 +77,22 @@ func TestNotifyPendingSessionLinks(t *testing.T) { "Coding Session Available at " + link2, }, { - name: "a provider that cannot deliver is not fatal", + name: "a failed delivery is not fatal, and does not nag", saved: []string{link1}, providerErr: errors.New("stdout closed"), wantCalls: 1, wantUserMessage: "Coding Session Available at " + link1, }, + { + // The agent showed nobody anything, so discarding the links here + // would lose them silently. + name: "an agent that cannot show messages keeps the links", + saved: []string{link1}, + providerErr: trace.ErrAnnounceUnsupported, + wantCalls: 1, + wantUserMessage: "Coding Session Available at " + link1, + wantKept: true, + }, } for _, tc := range testCases { @@ -96,13 +109,23 @@ func TestNotifyPendingSessionLinks(t *testing.T) { assert.Equal(t, tc.wantCalls, p.calls) assert.Equal(t, tc.wantUserMessage, p.announced) - // Announced once: a second hook firing must stay silent, even - // when the first delivery failed. Re-announcing a link the user - // has already seen on every later shell command is worse than - // dropping one. + // A second hook firing must stay silent once an agent has had + // its go, even if delivery failed: re-announcing on every later + // shell command is worse than dropping one notification. The + // exception is an agent that cannot show anything at all, which + // must leave the links intact for one that can. before := p.calls notifyPendingSessionLinks(p, store, zerolog.Nop()) - assert.Equal(t, before, p.calls, "links must be consumed exactly once") + + if tc.wantKept { + assert.Equal(t, before+1, p.calls, "unshown links must remain available") + assert.Equal(t, tc.saved, store.PendingLinks(), "unshown links must stay on disk") + + return + } + + assert.Equal(t, before, p.calls, "shown links must be consumed exactly once") + assert.Empty(t, store.PendingLinks()) }) } } From d00167a60f2efad738a665cd8cb1f3d17ba30f15 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 18:21:51 +0200 Subject: [PATCH 06/11] fix(cli): do not infer trace-run from the on-disk sentinel Address review feedback. The push skipped recording session links when the trace-run sentinel was set, but that sentinel is cleared in a deferred call, so a killed run leaves it behind. Every later pre-push attestation in that repository would then silently stop notifying the agent. The caller knows the truth without consulting disk, so `trace run` now says so through a push option instead. Nothing infers it from state that can go stale. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/trace_hook_handler.go | 17 +++++++++++------ app/cli/pkg/action/trace_run.go | 7 +++++-- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 30b2d1eea..14f94c777 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -285,6 +285,11 @@ type RunTracePushOpts struct { // only on CLI flags. Pre-push hook callers leave it false to keep // reading the repo config. IgnoreYAML bool + // SkipAgentNotification suppresses recording session links for an agent + // hook to show later. `trace run` sets it: it reaches the push only + // after the agent it wrapped has exited, so no hook of that agent can + // fire again, and its own terminal already showed the links. + SkipAgentNotification bool // ActionOpts is the root command's initialized options, used to build // the attestation executor. Required: the push cannot run without it. @@ -589,12 +594,12 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts // hook is what actually puts the link in front of them. A failure here // costs a notification, never the attestation that already succeeded. // - // Skipped under `chainloop trace run`, which reaches this point only - // after the agent it wrapped has exited: no hook of that agent can fire - // again, its terminal already showed the line above, and the record - // would just sit there waiting to be announced by an unrelated session. - if store.IsTraceRunActive() { - log.Debug().Msg("trace run owns this push; its terminal already showed the links") + // Skipped for callers that already showed the user, which the caller + // tells us directly rather than us inferring it from on-disk state: the + // trace-run sentinel outlives a killed run, and reading it here would + // silently suppress every later notification in that repository. + if opts.SkipAgentNotification { + log.Debug().Msg("caller already showed the links; not recording them for an agent hook") } else if err := store.SavePendingLinks(links); err != nil { log.Debug().Err(err).Msg("could not record session links for the agent hook") } diff --git a/app/cli/pkg/action/trace_run.go b/app/cli/pkg/action/trace_run.go index ff1019259..308536b6d 100644 --- a/app/cli/pkg/action/trace_run.go +++ b/app/cli/pkg/action/trace_run.go @@ -195,8 +195,11 @@ func TraceRun(ctx context.Context, log zerolog.Logger, opts TraceRunOpts) error WorkflowName: opts.WorkflowName, ProjectVersion: opts.ProjectVersion, IgnoreYAML: true, - ActionOpts: opts.ActionOpts, - CLIVersion: opts.CLIVersion, + // The wrapped agent has already exited and this terminal showed the + // push output, so there is no hook left to notify. + SkipAgentNotification: true, + ActionOpts: opts.ActionOpts, + CLIVersion: opts.CLIVersion, }) } From b5a56363c8ad62ad93317eb5c9955be771845514 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 19:37:39 +0200 Subject: [PATCH 07/11] refactor(cli): tidy the session-link notification path Address review feedback on the two preceding fixes. ClearPendingLinks no longer returns an error its only caller could act on, matching DeleteFileSnapshot and DeleteShellPreSignature, which are the same kind of single-use payload consumed by a best-effort hook. The notifier then stops inspecting one error either side of a mutation and clears in one place. Record why announcing before clearing is at-least-once by choice: the agent never acknowledges what it rendered, so exactly-once is not available, and the other ordering trades a repeated line for a link nobody sees. Split the keep-the-links case into its own test rather than a table flag that gave one subtest two endings, drop a doc claim about corrupt records that a truncating write already prevents, and remove three restatements of rationale that lives with the field it describes. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/internal/trace/opencode/provider.go | 4 +- app/cli/internal/trace/state/pendinglinks.go | 9 ++-- .../internal/trace/state/pendinglinks_test.go | 2 +- app/cli/pkg/action/trace_agent_hook.go | 31 ++++++------- app/cli/pkg/action/trace_hook_handler.go | 8 ++-- app/cli/pkg/action/trace_notify_links_test.go | 46 +++++++++---------- app/cli/pkg/action/trace_run.go | 14 +++--- 7 files changed, 54 insertions(+), 60 deletions(-) diff --git a/app/cli/internal/trace/opencode/provider.go b/app/cli/internal/trace/opencode/provider.go index 9a5452406..5974ff4b1 100644 --- a/app/cli/internal/trace/opencode/provider.go +++ b/app/cli/internal/trace/opencode/provider.go @@ -167,9 +167,7 @@ func (p *Provider) SystemMessage(_ string) error { // AnnounceToUser is unsupported for OpenCode until its plugin's response // shape for surfacing a message is verified against a live session, the way // Claude Code's was. The hook after a shell command already fires, so wiring -// this up later is a change to this method alone. Reporting it as -// unsupported rather than silently succeeding keeps callers from discarding -// content this agent never showed anyone. +// this up later is a change to this method alone. func (p *Provider) AnnounceToUser(_ string) error { return trace.ErrAnnounceUnsupported } diff --git a/app/cli/internal/trace/state/pendinglinks.go b/app/cli/internal/trace/state/pendinglinks.go index 4c694f579..f3af23dc6 100644 --- a/app/cli/internal/trace/state/pendinglinks.go +++ b/app/cli/internal/trace/state/pendinglinks.go @@ -71,9 +71,8 @@ func (s *Store) SavePendingLinks(links []string) error { // It returns nil when there is nothing recorded, when the record has expired, // or when it cannot be read: this feeds a cosmetic notification, and no // failure here is worth surfacing to the caller, let alone failing an agent's -// tool call over. An unreadable or expired record is cleared on the spot, -// since nobody will ever be able to use it and a corrupt file would otherwise -// wedge the mechanism for every later push. +// tool call over. A record that is expired or unparseable is dropped on the +// spot, since nobody can ever use it. func (s *Store) PendingLinks() []string { path := filepath.Join(s.traceDirPath(), pendingLinksFile) @@ -98,6 +97,6 @@ func (s *Store) PendingLinks() []string { } // ClearPendingLinks drops the record, so its links are shown at most once. -func (s *Store) ClearPendingLinks() error { - return removeIfExists(filepath.Join(s.traceDirPath(), pendingLinksFile)) +func (s *Store) ClearPendingLinks() { + _ = removeIfExists(filepath.Join(s.traceDirPath(), pendingLinksFile)) } diff --git a/app/cli/internal/trace/state/pendinglinks_test.go b/app/cli/internal/trace/state/pendinglinks_test.go index 2c126d0db..017cb958c 100644 --- a/app/cli/internal/trace/state/pendinglinks_test.go +++ b/app/cli/internal/trace/state/pendinglinks_test.go @@ -80,7 +80,7 @@ func TestPendingLinks(t *testing.T) { // to show them must leave them for one that can. assert.Equal(t, tc.want, store.PendingLinks(), "reading must not consume") - require.NoError(t, store.ClearPendingLinks()) + store.ClearPendingLinks() assert.Empty(t, store.PendingLinks(), "clearing must consume") }) } diff --git a/app/cli/pkg/action/trace_agent_hook.go b/app/cli/pkg/action/trace_agent_hook.go index 09cf15752..bdafb8518 100644 --- a/app/cli/pkg/action/trace_agent_hook.go +++ b/app/cli/pkg/action/trace_agent_hook.go @@ -175,8 +175,7 @@ func ensureSessionTracked(provider trace.Provider, store *state.Store, repoRoot } // notifyPendingSessionLinks hands any session links left by a just-completed -// trace push to the agent, so it can put them in front of the user. Links are -// consumed on read, so a link is announced at most once. +// trace push to the agent, so it can put them in front of the user. // // Best effort throughout: this is a notification, and neither a missing link // nor a provider that cannot deliver one is worth failing an agent's tool @@ -195,27 +194,27 @@ func notifyPendingSessionLinks(provider trace.Provider, store *state.Store, log err := provider.AnnounceToUser(strings.Join(lines, "\n")) if errors.Is(err, trace.ErrAnnounceUnsupported) { - // This agent has no way to show them, so leave them for one that - // might. Their expiry bounds how long they can linger. + // Nothing was shown, so leave the links for an agent that can show + // them. Their expiry bounds how long they linger. log.Debug().Msg("agent cannot show messages; leaving the session links for later") return } - // Any other outcome consumed the attempt, success or not. Clearing on a - // failed delivery is deliberate: retrying on every later shell command - // would nag far longer than one dropped notification costs. - if clearErr := store.ClearPendingLinks(); clearErr != nil { - log.Debug().Err(clearErr).Msg("could not clear the recorded session links") - } - if err != nil { log.Debug().Err(err).Msg("could not surface session links through the agent") - - return - } - - log.Debug().Int("links", len(links)).Msg("session links handed to the agent") + } else { + log.Debug().Int("links", len(links)).Msg("session links handed to the agent") + } + + // Success or failure, the attempt is spent: retrying on every later shell + // command would nag far longer than one dropped notification costs. + // + // Announcing before clearing makes this at-least-once by choice. The + // agent never acknowledges what it rendered, so exactly-once is not + // available at any price, and the other ordering trades a repeated line + // for a link nobody ever sees. + store.ClearPendingLinks() } // HandleAgentPostToolUse handles post-edit hooks across providers diff --git a/app/cli/pkg/action/trace_hook_handler.go b/app/cli/pkg/action/trace_hook_handler.go index 14f94c777..b1d7a893b 100644 --- a/app/cli/pkg/action/trace_hook_handler.go +++ b/app/cli/pkg/action/trace_hook_handler.go @@ -594,10 +594,10 @@ func RunTracePush(ctx context.Context, log zerolog.Logger, opts RunTracePushOpts // hook is what actually puts the link in front of them. A failure here // costs a notification, never the attestation that already succeeded. // - // Skipped for callers that already showed the user, which the caller - // tells us directly rather than us inferring it from on-disk state: the - // trace-run sentinel outlives a killed run, and reading it here would - // silently suppress every later notification in that repository. + // The caller tells us whether to skip, rather than us inferring it from + // on-disk state: the trace-run sentinel outlives a killed run, and + // reading it here would silently suppress every later notification in + // that repository. if opts.SkipAgentNotification { log.Debug().Msg("caller already showed the links; not recording them for an agent hook") } else if err := store.SavePendingLinks(links); err != nil { diff --git a/app/cli/pkg/action/trace_notify_links_test.go b/app/cli/pkg/action/trace_notify_links_test.go index a35cd8b99..51767f2d5 100644 --- a/app/cli/pkg/action/trace_notify_links_test.go +++ b/app/cli/pkg/action/trace_notify_links_test.go @@ -54,9 +54,6 @@ func TestNotifyPendingSessionLinks(t *testing.T) { providerErr error wantCalls int wantUserMessage string - // wantKept is true when the links must survive for another agent to - // show, rather than being consumed by this attempt. - wantKept bool }{ { name: "nothing pending leaves the agent alone", @@ -83,16 +80,6 @@ func TestNotifyPendingSessionLinks(t *testing.T) { wantCalls: 1, wantUserMessage: "Coding Session Available at " + link1, }, - { - // The agent showed nobody anything, so discarding the links here - // would lose them silently. - name: "an agent that cannot show messages keeps the links", - saved: []string{link1}, - providerErr: trace.ErrAnnounceUnsupported, - wantCalls: 1, - wantUserMessage: "Coding Session Available at " + link1, - wantKept: true, - }, } for _, tc := range testCases { @@ -111,21 +98,34 @@ func TestNotifyPendingSessionLinks(t *testing.T) { // A second hook firing must stay silent once an agent has had // its go, even if delivery failed: re-announcing on every later - // shell command is worse than dropping one notification. The - // exception is an agent that cannot show anything at all, which - // must leave the links intact for one that can. + // shell command is worse than dropping one notification. before := p.calls notifyPendingSessionLinks(p, store, zerolog.Nop()) - if tc.wantKept { - assert.Equal(t, before+1, p.calls, "unshown links must remain available") - assert.Equal(t, tc.saved, store.PendingLinks(), "unshown links must stay on disk") - - return - } - assert.Equal(t, before, p.calls, "shown links must be consumed exactly once") assert.Empty(t, store.PendingLinks()) }) } } + +// TestNotifyPendingSessionLinksKeepsUnshownLinks covers the inverse of the +// consume-once rule: an agent with no way to reach the user showed nobody +// anything, so discarding the links would lose them silently. +func TestNotifyPendingSessionLinksKeepsUnshownLinks(t *testing.T) { + const link = "https://app.chainloop.dev/u/chainloop/sessions/ses_1" + + store := state.NewGitStore(t.TempDir()) + require.NoError(t, store.InitTraceDir()) + require.NoError(t, store.SavePendingLinks([]string{link})) + + p := &recordingProvider{err: trace.ErrAnnounceUnsupported} + + notifyPendingSessionLinks(p, store, zerolog.Nop()) + + assert.Equal(t, 1, p.calls) + assert.Equal(t, []string{link}, store.PendingLinks(), "unshown links must stay on disk") + + // An agent that can show them still gets its turn later. + notifyPendingSessionLinks(p, store, zerolog.Nop()) + assert.Equal(t, 2, p.calls, "unshown links must remain available") +} diff --git a/app/cli/pkg/action/trace_run.go b/app/cli/pkg/action/trace_run.go index 308536b6d..83f924e7c 100644 --- a/app/cli/pkg/action/trace_run.go +++ b/app/cli/pkg/action/trace_run.go @@ -189,14 +189,12 @@ func TraceRun(ctx context.Context, log zerolog.Logger, opts TraceRunOpts) error log.Debug().Msg("wrapped command completed; attesting session") return RunTracePush(ctx, log, RunTracePushOpts{ - AllowEmpty: true, - ProjectName: opts.ProjectName, - Organization: opts.Organization, - WorkflowName: opts.WorkflowName, - ProjectVersion: opts.ProjectVersion, - IgnoreYAML: true, - // The wrapped agent has already exited and this terminal showed the - // push output, so there is no hook left to notify. + AllowEmpty: true, + ProjectName: opts.ProjectName, + Organization: opts.Organization, + WorkflowName: opts.WorkflowName, + ProjectVersion: opts.ProjectVersion, + IgnoreYAML: true, SkipAgentNotification: true, ActionOpts: opts.ActionOpts, CLIVersion: opts.CLIVersion, From ef480667d0a234b9b2a137145743d778d00ae6aa Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 19:49:06 +0200 Subject: [PATCH 08/11] test(cli): exercise the handover the keep-links test claimed Address review feedback. The test said a capable agent still gets its turn later, but reused the unsupported provider for the second call, so it only proved an unsupported agent is retried and never covered the transition it described. The second call now uses a capable provider and asserts it is offered the links, announces them, and consumes them. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/trace_notify_links_test.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/app/cli/pkg/action/trace_notify_links_test.go b/app/cli/pkg/action/trace_notify_links_test.go index 51767f2d5..2280431ec 100644 --- a/app/cli/pkg/action/trace_notify_links_test.go +++ b/app/cli/pkg/action/trace_notify_links_test.go @@ -118,14 +118,20 @@ func TestNotifyPendingSessionLinksKeepsUnshownLinks(t *testing.T) { require.NoError(t, store.InitTraceDir()) require.NoError(t, store.SavePendingLinks([]string{link})) - p := &recordingProvider{err: trace.ErrAnnounceUnsupported} + unsupported := &recordingProvider{err: trace.ErrAnnounceUnsupported} - notifyPendingSessionLinks(p, store, zerolog.Nop()) + notifyPendingSessionLinks(unsupported, store, zerolog.Nop()) - assert.Equal(t, 1, p.calls) + assert.Equal(t, 1, unsupported.calls) assert.Equal(t, []string{link}, store.PendingLinks(), "unshown links must stay on disk") - // An agent that can show them still gets its turn later. - notifyPendingSessionLinks(p, store, zerolog.Nop()) - assert.Equal(t, 2, p.calls, "unshown links must remain available") + // The point of keeping them: an agent that can show them gets its turn + // later, and consumes them as usual. + capable := &recordingProvider{} + + notifyPendingSessionLinks(capable, store, zerolog.Nop()) + + assert.Equal(t, 1, capable.calls, "a capable agent must still be offered the links") + assert.Equal(t, "Coding Session Available at "+link, capable.announced) + assert.Empty(t, store.PendingLinks(), "once shown, the links are consumed") } From c41815e877021d76e983f4c39686164f1a522714 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 22:29:42 +0200 Subject: [PATCH 09/11] feat(cli): say what the trace session banner actually does The session-start banner read "This session will be attested by Chainloop". "Attested" is Chainloop vocabulary, not the developer's: a teammate who cloned the repository and never ran init cannot tell whether that means recorded, uploaded or signed. It also never said where the data goes, which is a poor showing for a tool whose pitch is transparency about what leaves your machine. It now reads: Chainloop Trace is recording this session. Evidence will be sent to organization: project: The destination line appears only when the control plane reports a web dashboard, matching the gate the session link already uses. Finding out costs one Infoz call, which needs no credentials and is abandoned after two seconds, because a developer waiting to type is a worse cost than a missing line. Organization and project come from .chainloop.yml at no cost. Each line is dropped when its fact is unknown, so the banner never names a destination it did not confirm. fetchUIDashboardURL now takes the timeout from its caller, since a person waiting on a command they ran themselves will tolerate far more than an agent hook holding up a first prompt. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7 --- app/cli/pkg/action/action.go | 23 +++++- app/cli/pkg/action/trace_agent_hook.go | 60 +++++++++++++- app/cli/pkg/action/trace_agent_hook_test.go | 2 +- app/cli/pkg/action/trace_banner_test.go | 87 +++++++++++++++++++++ app/cli/pkg/action/workflow_run_describe.go | 2 +- 5 files changed, 167 insertions(+), 7 deletions(-) create mode 100644 app/cli/pkg/action/trace_banner_test.go diff --git a/app/cli/pkg/action/action.go b/app/cli/pkg/action/action.go index 45513d2df..0d9c7bf19 100644 --- a/app/cli/pkg/action/action.go +++ b/app/cli/pkg/action/action.go @@ -42,6 +42,16 @@ const ( // defaultAttestationStateFile is the default file name for local attestation state. defaultAttestationStateFile = "chainloop-attestation.tmp.json" + + // dashboardURLTimeout bounds an Infoz lookup made while a person waits on + // a command they ran themselves. + dashboardURLTimeout = 5 * time.Second + + // hookDashboardURLTimeout is the tighter bound for the same lookup inside + // an agent hook, where the wait sits between the developer and their + // first prompt. Missing the banner's destination line costs far less than + // a visible stall, so this gives up quickly. + hookDashboardURLTimeout = 2 * time.Second ) // AttestationStatePath returns the resolved path for local attestation state. @@ -176,14 +186,19 @@ func getCASBackend(ctx context.Context, client pb.AttestationServiceClient, work return casBackendInfo, artifactCASConn.Close, nil } -// fetchUIDashboardURL retrieves the UI Dashboard URL from the control plane -// Returns empty string if not configured or if fetch fails -func fetchUIDashboardURL(ctx context.Context, cpConnection *grpc.ClientConn) string { +// fetchUIDashboardURL retrieves the UI Dashboard URL from the control plane. +// Returns empty string if not configured or if the fetch fails, so callers +// can treat "no dashboard" and "could not ask" the same way. +// +// The caller chooses the timeout, because the acceptable wait depends on +// where this runs: a person waiting on `workflow run describe` will tolerate +// far more than an agent hook holding up someone's first prompt. +func fetchUIDashboardURL(ctx context.Context, cpConnection *grpc.ClientConn, timeout time.Duration) string { if cpConnection == nil { return "" } - tmoutCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + tmoutCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() client := pb.NewStatusServiceClient(cpConnection) diff --git a/app/cli/pkg/action/trace_agent_hook.go b/app/cli/pkg/action/trace_agent_hook.go index bdafb8518..c1ddfc9df 100644 --- a/app/cli/pkg/action/trace_agent_hook.go +++ b/app/cli/pkg/action/trace_agent_hook.go @@ -16,6 +16,7 @@ package action import ( + "context" "errors" "os" "path/filepath" @@ -56,6 +57,36 @@ func HandleAgentSessionEnd(provider trace.Provider, log zerolog.Logger) error { return nil } +// sessionStartBanner is what the developer sees at the top of a traced +// session. Its audience includes someone who cloned the repository and never +// ran init, so it says what is happening in plain words and, when we know it, +// where the evidence goes. Chainloop's own vocabulary is deliberately absent: +// "attested" does not tell a newcomer whether something is recorded, +// uploaded, or signed, let alone to where. +// +// Each fact is dropped rather than guessed at when it is unknown, so the +// banner never promises a destination that was not confirmed. +func sessionStartBanner(dashboardURL, org, project string) string { + lines := []string{"Chainloop Trace is recording this session."} + + if dashboardURL != "" { + lines = append(lines, "Evidence will be sent to "+strings.TrimRight(dashboardURL, "/")) + } + + var identity []string + if org != "" { + identity = append(identity, "organization: "+org) + } + if project != "" { + identity = append(identity, "project: "+project) + } + if len(identity) > 0 { + lines = append(lines, strings.Join(identity, " ")) + } + + return strings.Join(lines, "\n") +} + // HandleAgentSessionStart handles the agent session-start hook. func HandleAgentSessionStart(provider trace.Provider, log zerolog.Logger) error { input, err := provider.ReadHookInput(os.Stdin) @@ -74,13 +105,40 @@ func HandleAgentSessionStart(provider trace.Provider, log zerolog.Logger) error ensureSessionTracked(provider, store, repoRoot, input, log) - if err := provider.SystemMessage("\n\n*** This session will be attested by Chainloop ***"); err != nil { + banner := sessionStartBanner( + hookDashboardURL(log), + config.LoadOrganizationFromYML(repoRoot), + config.LoadProjectFromYML(repoRoot), + ) + + if err := provider.SystemMessage("\n\n" + banner + "\n"); err != nil { log.Debug().Err(err).Msg("session-start: failed to send system message") } return nil } +// hookDashboardURL asks the control plane where its web dashboard lives, so +// the session banner can name the destination the evidence is bound for. +// Returns an empty string when there is no dashboard, no reachable control +// plane, or no time to find out, in which case the banner simply omits the +// line. +// +// This is the one network call the session-start hook makes, and it is +// deliberately cheap to abandon: Infoz needs no credentials, so no token is +// loaded, and the timeout is short because a developer waiting to type is a +// worse cost than a missing line. +func hookDashboardURL(log zerolog.Logger) string { + conn, err := newControlPlaneConnection("", "") + if err != nil { + log.Debug().Err(err).Msg("session-start: no control plane connection for the banner") + return "" + } + defer func() { _ = conn.Close() }() + + return fetchUIDashboardURL(context.Background(), conn, hookDashboardURLTimeout) +} + // HandleAgentPreToolUse handles the agent pre-tool-use hook. func HandleAgentPreToolUse(provider trace.Provider, log zerolog.Logger) error { input, err := provider.ReadHookInput(os.Stdin) diff --git a/app/cli/pkg/action/trace_agent_hook_test.go b/app/cli/pkg/action/trace_agent_hook_test.go index ff02c72b4..ea970e791 100644 --- a/app/cli/pkg/action/trace_agent_hook_test.go +++ b/app/cli/pkg/action/trace_agent_hook_test.go @@ -122,7 +122,7 @@ func TestHandleAgentSessionStart(t *testing.T) { }) assert.Contains(t, stdout, `"systemMessage"`) - assert.Contains(t, stdout, "This session will be attested by Chainloop") + assert.Contains(t, stdout, "Chainloop Trace is recording this session.") }) t.Run("ignores malformed stdin", func(t *testing.T) { diff --git a/app/cli/pkg/action/trace_banner_test.go b/app/cli/pkg/action/trace_banner_test.go new file mode 100644 index 000000000..33c09d322 --- /dev/null +++ b/app/cli/pkg/action/trace_banner_test.go @@ -0,0 +1,87 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package action + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// TestSessionStartBanner pins what the developer is told at the top of a +// session. The audience includes a teammate who cloned the repository and +// never ran init, so the banner has to say what is happening and where the +// evidence goes without assuming any Chainloop vocabulary. +func TestSessionStartBanner(t *testing.T) { + const ( + recording = "Chainloop Trace is recording this session." + testProject = "chainloop-cli" + ) + + testCases := []struct { + name string + dashboardURL string + org string + project string + want string + }{ + { + name: "nothing known says only what is happening", + want: recording, + }, + { + name: "destination is named when there is a dashboard", + dashboardURL: "https://app.chainloop.dev", + want: recording + "\nEvidence will be sent to https://app.chainloop.dev", + }, + { + name: "trailing slash is trimmed", + dashboardURL: "https://app.chainloop.dev/", + want: recording + "\nEvidence will be sent to https://app.chainloop.dev", + }, + { + name: "organization and project are named when known", + dashboardURL: "https://app.chainloop.dev", + org: testOrgName, + project: testProject, + want: recording + + "\nEvidence will be sent to https://app.chainloop.dev" + + "\norganization: " + testOrgName + " project: " + testProject, + }, + { + name: "identity is named even with no dashboard configured", + org: testOrgName, + project: testProject, + want: recording + "\norganization: " + testOrgName + " project: " + testProject, + }, + { + name: "project alone", + project: testProject, + want: recording + "\nproject: " + testProject, + }, + { + name: "organization alone", + org: testOrgName, + want: recording + "\norganization: " + testOrgName, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, sessionStartBanner(tc.dashboardURL, tc.org, tc.project)) + }) + } +} diff --git a/app/cli/pkg/action/workflow_run_describe.go b/app/cli/pkg/action/workflow_run_describe.go index b2891eef9..0f64ff021 100644 --- a/app/cli/pkg/action/workflow_run_describe.go +++ b/app/cli/pkg/action/workflow_run_describe.go @@ -285,7 +285,7 @@ func (action *WorkflowRunDescribe) Run(ctx context.Context, opts *WorkflowRunDes } var attestationViewURL string - baseUIDashboardURL := fetchUIDashboardURL(ctx, action.cfg.CPConnection) + baseUIDashboardURL := fetchUIDashboardURL(ctx, action.cfg.CPConnection, dashboardURLTimeout) if baseUIDashboardURL != "" { attestationViewURL = buildAttestationViewURL(baseUIDashboardURL, resp.GetResult().GetOrgName(), att.DigestInCasBackend) } From 5bd05c9ede281f9f41e62fad8da56ea83de98770 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 22:45:51 +0200 Subject: [PATCH 10/11] feat(cli): fold the banner destination onto one line and gate its cost The session-start banner spent a line on the destination and another on the organization and project, though between them they answer a single question: where this is going. They now share a line: Chainloop Trace is recording this session. Evidence will be sent to (organization: , project:

) The space before the parenthesis is load-bearing, since it is what lets a terminal linkify the URL without swallowing the punctuation after it. Either half still stands alone when the other is unknown. Building that banner costs a control-plane round trip, and Cursor and opencode discard system messages entirely, so every session start on those agents paid up to two seconds for a string nobody would read. Providers now report whether the channel reaches the user, and the lookup is skipped when it does not. The capability is a predicate rather than a sentinel error because the cost being avoided is in preparing the argument, which a post-hoc error cannot save. A registry test fails when a new provider arrives without a recorded decision, since the zero value would quietly claim no support. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: 2cee6265-17c4-487a-b31d-072037bd3bc7, 82030869-b7f3-4352-bd95-cea2eb75abab --- app/cli/internal/trace/claude/provider.go | 6 ++ app/cli/internal/trace/cursor/provider.go | 6 ++ app/cli/internal/trace/opencode/provider.go | 6 ++ app/cli/internal/trace/provider.go | 6 ++ .../trace/providers/capabilities_test.go | 74 ++++++++++++++ app/cli/pkg/action/trace_agent_hook.go | 34 +++++-- app/cli/pkg/action/trace_banner_test.go | 98 +++++++++++++++++-- 7 files changed, 211 insertions(+), 19 deletions(-) create mode 100644 app/cli/internal/trace/providers/capabilities_test.go diff --git a/app/cli/internal/trace/claude/provider.go b/app/cli/internal/trace/claude/provider.go index 14ffbc621..3b70ef0b6 100644 --- a/app/cli/internal/trace/claude/provider.go +++ b/app/cli/internal/trace/claude/provider.go @@ -146,6 +146,12 @@ func (p *Provider) IsCommandTool(toolName string) bool { return slices.Contains(commandTools, toolName) } +// SupportsSystemMessage is true for Claude Code: it renders the +// systemMessage field of a hook response directly to the user. +func (p *Provider) SupportsSystemMessage() bool { + return true +} + // SystemMessage writes a message to stdout for Claude Code to display on session start. func (p *Provider) SystemMessage(msg string) error { if msg == "" { diff --git a/app/cli/internal/trace/cursor/provider.go b/app/cli/internal/trace/cursor/provider.go index 8d5ad8e39..9010e180e 100644 --- a/app/cli/internal/trace/cursor/provider.go +++ b/app/cli/internal/trace/cursor/provider.go @@ -100,6 +100,12 @@ func (p *Provider) SystemMessage(_ string) error { return nil } +// SupportsSystemMessage is false for Cursor, so callers skip the cost of +// composing a message that SystemMessage would drop. +func (p *Provider) SupportsSystemMessage() bool { + return false +} + // AnnounceToUser is unsupported for Cursor: it installs only sessionStart, // sessionEnd and afterFileEdit, so no hook fires after a shell command and // there is nowhere to deliver the message. diff --git a/app/cli/internal/trace/opencode/provider.go b/app/cli/internal/trace/opencode/provider.go index 5974ff4b1..d72ea885b 100644 --- a/app/cli/internal/trace/opencode/provider.go +++ b/app/cli/internal/trace/opencode/provider.go @@ -164,6 +164,12 @@ func (p *Provider) SystemMessage(_ string) error { return nil } +// SupportsSystemMessage is false for opencode, so callers skip the cost of +// composing a message that SystemMessage would drop. +func (p *Provider) SupportsSystemMessage() bool { + return false +} + // AnnounceToUser is unsupported for OpenCode until its plugin's response // shape for surfacing a message is verified against a live session, the way // Claude Code's was. The hook after a shell command already fires, so wiring diff --git a/app/cli/internal/trace/provider.go b/app/cli/internal/trace/provider.go index b8a192de1..2d9f6a76c 100644 --- a/app/cli/internal/trace/provider.go +++ b/app/cli/internal/trace/provider.go @@ -105,6 +105,12 @@ type Provider interface { // SystemMessage writes a message to stdout for the agent to display on session start. SystemMessage(msg string) error + // SupportsSystemMessage reports whether SystemMessage reaches the user + // rather than being discarded. Callers check it before assembling a + // message that costs something to produce, since for agents without + // such a channel that work buys nothing. + SupportsSystemMessage() bool + // AnnounceToUser writes a hook response to stdout so the agent puts msg // in front of the user, after a shell command the agent ran. Which // channel that uses is the provider's business: agents differ in whether diff --git a/app/cli/internal/trace/providers/capabilities_test.go b/app/cli/internal/trace/providers/capabilities_test.go new file mode 100644 index 000000000..4465abd11 --- /dev/null +++ b/app/cli/internal/trace/providers/capabilities_test.go @@ -0,0 +1,74 @@ +// +// 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/claude" + "github.com/chainloop-dev/chainloop/app/cli/internal/trace/cursor" + "github.com/chainloop-dev/chainloop/app/cli/internal/trace/opencode" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSupportsSystemMessage pins which agents can actually show the +// session-start banner. Callers use this to decide whether building the +// banner is worth its cost, so a provider claiming support it does not have +// buys a control-plane round trip for a message nobody reads. +func TestSupportsSystemMessage(t *testing.T) { + testCases := []struct { + provider string + want bool + why string + }{ + { + provider: claude.Name, + want: true, + why: "Claude Code renders the systemMessage field of a hook response", + }, + { + provider: cursor.Name, + want: false, + why: "Cursor's hook response has no channel for displaying text", + }, + { + provider: opencode.Name, + want: false, + why: "opencode's plugin system has no equivalent channel", + }, + } + + for _, tc := range testCases { + t.Run(tc.provider, func(t *testing.T) { + p := ByName(tc.provider) + require.NotNil(t, p, "provider %q is not registered", tc.provider) + + assert.Equal(t, tc.want, p.SupportsSystemMessage(), tc.why) + }) + } +} + +// TestSupportsSystemMessageCoversEveryProvider fails when a provider is added +// without a decision recorded above, since the default zero value would +// silently claim no support. +func TestSupportsSystemMessageCoversEveryProvider(t *testing.T) { + known := map[string]bool{claude.Name: true, cursor.Name: true, opencode.Name: true} + + for _, p := range All() { + assert.True(t, known[p.Name()], "provider %q has no SupportsSystemMessage expectation", p.Name()) + } +} diff --git a/app/cli/pkg/action/trace_agent_hook.go b/app/cli/pkg/action/trace_agent_hook.go index c1ddfc9df..6ed55a10b 100644 --- a/app/cli/pkg/action/trace_agent_hook.go +++ b/app/cli/pkg/action/trace_agent_hook.go @@ -67,12 +67,6 @@ func HandleAgentSessionEnd(provider trace.Provider, log zerolog.Logger) error { // Each fact is dropped rather than guessed at when it is unknown, so the // banner never promises a destination that was not confirmed. func sessionStartBanner(dashboardURL, org, project string) string { - lines := []string{"Chainloop Trace is recording this session."} - - if dashboardURL != "" { - lines = append(lines, "Evidence will be sent to "+strings.TrimRight(dashboardURL, "/")) - } - var identity []string if org != "" { identity = append(identity, "organization: "+org) @@ -80,11 +74,25 @@ func sessionStartBanner(dashboardURL, org, project string) string { if project != "" { identity = append(identity, "project: "+project) } - if len(identity) > 0 { - lines = append(lines, strings.Join(identity, " ")) + where := strings.Join(identity, ", ") + + // Destination and identity share a line, since they answer one question + // between them: where this is going. The space before the parenthesis + // matters, as it is what lets a terminal linkify the URL without + // swallowing the punctuation that follows it. + switch { + case dashboardURL != "" && where != "": + where = "Evidence will be sent to " + strings.TrimRight(dashboardURL, "/") + " (" + where + ")" + case dashboardURL != "": + where = "Evidence will be sent to " + strings.TrimRight(dashboardURL, "/") + } + + banner := "Chainloop Trace is recording this session." + if where != "" { + banner += "\n" + where } - return strings.Join(lines, "\n") + return banner } // HandleAgentSessionStart handles the agent session-start hook. @@ -105,6 +113,14 @@ func HandleAgentSessionStart(provider trace.Provider, log zerolog.Logger) error ensureSessionTracked(provider, store, repoRoot, input, log) + // Composing the banner costs a control-plane round trip, so it is only + // worth doing for an agent that can put it in front of the user. Cursor + // and opencode would discard it, and the developer would have paid the + // wait for nothing. + if !provider.SupportsSystemMessage() { + return nil + } + banner := sessionStartBanner( hookDashboardURL(log), config.LoadOrganizationFromYML(repoRoot), diff --git a/app/cli/pkg/action/trace_banner_test.go b/app/cli/pkg/action/trace_banner_test.go index 33c09d322..95f1f4fbb 100644 --- a/app/cli/pkg/action/trace_banner_test.go +++ b/app/cli/pkg/action/trace_banner_test.go @@ -16,11 +16,35 @@ package action import ( + "os" + "path/filepath" "testing" + "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/state" + "github.com/rs/zerolog" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +// bannerProvider is a real provider with its banner capability forced, so a +// test can drive both sides of the gate without a second agent. +type bannerProvider struct { + trace.Provider + + supports bool + sysCalls int +} + +func (p *bannerProvider) SupportsSystemMessage() bool { return p.supports } + +func (p *bannerProvider) SystemMessage(string) error { + p.sysCalls++ + + return nil +} + // TestSessionStartBanner pins what the developer is told at the top of a // session. The audience includes a teammate who cloned the repository and // never ran init, so the banner has to say what is happening and where the @@ -44,28 +68,31 @@ func TestSessionStartBanner(t *testing.T) { }, { name: "destination is named when there is a dashboard", - dashboardURL: "https://app.chainloop.dev", - want: recording + "\nEvidence will be sent to https://app.chainloop.dev", + dashboardURL: testDashboardURL, + want: recording + "\nEvidence will be sent to " + testDashboardURL, }, { name: "trailing slash is trimmed", - dashboardURL: "https://app.chainloop.dev/", - want: recording + "\nEvidence will be sent to https://app.chainloop.dev", + dashboardURL: testDashboardURL + "/", + want: recording + "\nEvidence will be sent to " + testDashboardURL, }, { - name: "organization and project are named when known", - dashboardURL: "https://app.chainloop.dev", + // Destination and identity share one line, and the URL keeps a + // space after it so a terminal linkifies it without swallowing + // the parenthesis. + name: "organization and project qualify the destination", + dashboardURL: testDashboardURL, org: testOrgName, project: testProject, want: recording + - "\nEvidence will be sent to https://app.chainloop.dev" + - "\norganization: " + testOrgName + " project: " + testProject, + "\nEvidence will be sent to " + testDashboardURL + + " (organization: " + testOrgName + ", project: " + testProject + ")", }, { - name: "identity is named even with no dashboard configured", + name: "identity stands alone when no dashboard is configured", org: testOrgName, project: testProject, - want: recording + "\norganization: " + testOrgName + " project: " + testProject, + want: recording + "\norganization: " + testOrgName + ", project: " + testProject, }, { name: "project alone", @@ -77,6 +104,14 @@ func TestSessionStartBanner(t *testing.T) { org: testOrgName, want: recording + "\norganization: " + testOrgName, }, + { + name: "dashboard with only a project", + dashboardURL: testDashboardURL, + project: testProject, + want: recording + + "\nEvidence will be sent to " + testDashboardURL + + " (project: " + testProject + ")", + }, } for _, tc := range testCases { @@ -85,3 +120,46 @@ func TestSessionStartBanner(t *testing.T) { }) } } + +// TestSessionStartBannerGate checks that an agent which cannot display the +// banner is never handed one. Building it costs a control-plane round trip, +// which is not worth paying for a string the agent throws away. +func TestSessionStartBannerGate(t *testing.T) { + testCases := []struct { + name string + supports bool + wantSent int + }{ + { + name: "an agent that shows messages gets the banner", + supports: true, + wantSent: 1, + }, + { + name: "an agent that discards them is not asked", + supports: false, + wantSent: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + repoDir := initTempGitRepo(t) + store := state.NewGitStore(filepath.Join(repoDir, ".git")) + require.NoError(t, store.InitTraceDir()) + + origDir, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(repoDir)) + t.Cleanup(func() { _ = os.Chdir(origDir) }) + + withStdin(t, `{"session_id":"abc-123"}`) + + p := &bannerProvider{Provider: claude.New(), supports: tc.supports} + require.NoError(t, HandleAgentSessionStart(p, zerolog.Nop())) + + assert.Equal(t, tc.wantSent, p.sysCalls) + assert.True(t, store.SessionRecordExists("abc-123"), "the session is tracked either way") + }) + } +} From 9f3c867906042a0ec8fa2dd603bc2fccdb980543 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Thu, 10 Sep 2026 23:11:57 +0200 Subject: [PATCH 11/11] feat(trace): widen opencode tracing and add cursor hooks The opencode plugin now traces bash tool invocations in addition to file-writing tools, and resolves every file touched by an apply_patch call instead of only a single filePath/path argument. Hook invocations are fire-and-forget so a missing or failing chainloop binary never blocks tool execution. Also adds .cursor/hooks.json so Cursor sessions emit session-start, session-end, and after-file-edit trace events. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: ses_0c830ab01ffeGQhnPohmihWjP5 --- .cursor/hooks.json | 23 ++++++++ .opencode/plugins/chainloop-trace.ts | 86 +++++++++++++++++++++------- 2 files changed, 87 insertions(+), 22 deletions(-) create mode 100644 .cursor/hooks.json diff --git a/.cursor/hooks.json b/.cursor/hooks.json new file mode 100644 index 000000000..46ef89fd4 --- /dev/null +++ b/.cursor/hooks.json @@ -0,0 +1,23 @@ +{ + "hooks": { + "afterFileEdit": [ + { + "command": "chainloop trace hook cursor after-file-edit", + "timeout": 30 + } + ], + "sessionEnd": [ + { + "command": "chainloop trace hook cursor session-end", + "timeout": 30 + } + ], + "sessionStart": [ + { + "command": "chainloop trace hook cursor session-start", + "timeout": 30 + } + ] + }, + "version": 1 +} diff --git a/.opencode/plugins/chainloop-trace.ts b/.opencode/plugins/chainloop-trace.ts index d6700243d..57befcbcf 100644 --- a/.opencode/plugins/chainloop-trace.ts +++ b/.opencode/plugins/chainloop-trace.ts @@ -1,22 +1,44 @@ import type { Plugin } from "@opencode-ai/plugin" export const ChainloopTrace: Plugin = async ({ $ }) => { - // The commit-msg hook links sessions to commits by cross-referencing - // staged files against AI line attributions recorded by post-tool-use. - // If no file-writing tools (edit, write, apply_patch) are invoked during - // the session, there will be no attributions and the commit will not be - // marked as AI-assisted. const fileWritingTools = ["edit","write","apply_patch"] + const commandTools = ["bash"] - function filePathFromArgs(args: any): string { - if (args?.filePath) return args.filePath - if (args?.path) return args.path - return "" + function filePathsFromArgs(args: any): string[] { + if (args?.filePath) return [args.filePath] + if (args?.path) return [args.path] + if (args?.patchText) return parsePatchPaths(args.patchText) + return [] } + // parsePatchPaths extracts affected file paths from an apply_patch + // patchText payload. Each section starts with *** Add File:, *** Update + // File:, or *** Delete File: followed by the path. Paths are deduplicated + // while preserving first-seen order. + function parsePatchPaths(patchText: string): string[] { + const paths: string[] = [] + const seen = new Set() + const re = /^\*\*\* (?:Add|Update|Delete) File: (.+)$/gm + let m + while ((m = re.exec(patchText)) !== null) { + const p = m[1].trim() + if (p && !seen.has(p)) { + seen.add(p) + paths.push(p) + } + } + return paths + } + + // fire-and-forget: tracing must never block tool execution. If chainloop + // is unavailable or errors, log to stderr and move on. async function fire(event: string, payload: Record) { const json = JSON.stringify(payload) - await $`echo ${json} | chainloop trace hook opencode ${event}` + try { + await $`echo ${json} | chainloop trace hook opencode ${event}` + } catch (err) { + console.error(`chainloop-trace: ${event} hook failed: ${err}`) + } } return { @@ -31,22 +53,42 @@ export const ChainloopTrace: Plugin = async ({ $ }) => { } }, "tool.execute.before": async (input, output) => { + if (commandTools.includes(input.tool)) { + await fire("pre-tool-use", { + session_id: input.sessionID, + hook_event_name: "tool.execute.before", + tool_name: input.tool, + }) + return + } if (!fileWritingTools.includes(input.tool)) return - await fire("pre-tool-use", { - session_id: input.sessionID, - hook_event_name: "tool.execute.before", - tool_name: input.tool, - file_path: filePathFromArgs(output.args), - }) + for (const fp of filePathsFromArgs(output.args)) { + await fire("pre-tool-use", { + session_id: input.sessionID, + hook_event_name: "tool.execute.before", + tool_name: input.tool, + file_path: fp, + }) + } }, "tool.execute.after": async (input) => { + if (commandTools.includes(input.tool)) { + await fire("post-tool-use", { + session_id: input.sessionID, + hook_event_name: "tool.execute.after", + tool_name: input.tool, + }) + return + } if (!fileWritingTools.includes(input.tool)) return - await fire("post-tool-use", { - session_id: input.sessionID, - hook_event_name: "tool.execute.after", - tool_name: input.tool, - file_path: filePathFromArgs(input.args), - }) + for (const fp of filePathsFromArgs(input.args)) { + await fire("post-tool-use", { + session_id: input.sessionID, + hook_event_name: "tool.execute.after", + tool_name: input.tool, + file_path: fp, + }) + } }, } }