Merge pull request #2533 from dgageot/board/builtin-hooks-displaying-unwanted-env-in-0fb6e572

fix(runtime): don't persist session_start hook output as a session message

Author: dgageot

pkg/runtime/hooks.go

@@ -85,32 +85,39 @@ func (r *LocalRuntime) dispatchHook(
 	return result
 }
 
-// executeSessionStartHooks executes session_start hooks and persists any
-// AdditionalContext as a system message on the session. SystemMessage,
-// if any, is emitted as a Warning by [dispatchHook].
-func (r *LocalRuntime) executeSessionStartHooks(ctx context.Context, sess *session.Session, a *agent.Agent, events chan Event) {
-	result := r.dispatchHook(ctx, a, hooks.EventSessionStart, &hooks.Input{
+// executeSessionStartHooks fires session_start once at the top of
+// RunStream and returns its AdditionalContext as transient system
+// messages. The result is NOT persisted to the session: persisting
+// would pollute the visible transcript and (because session_start
+// fires after the user message has been added) shift the message the
+// runtime relays as the [UserMessageEvent]. Callers thread the
+// returned slice through [session.Session.GetMessages] on every
+// iteration so cwd / OS / arch context reaches the model without ever
+// being stored.
+func (r *LocalRuntime) executeSessionStartHooks(ctx context.Context, sess *session.Session, a *agent.Agent, events chan Event) []chat.Message {
+	return contextMessages(r.dispatchHook(ctx, a, hooks.EventSessionStart, &hooks.Input{
 		SessionID: sess.ID,
 		Source:    "startup",
-	}, events)
-	if result == nil || result.AdditionalContext == "" {
-		return
-	}
-	slog.Debug("Session start hook provided additional context", "context", result.AdditionalContext)
-	sess.AddMessage(session.SystemMessage(result.AdditionalContext))
+	}, events))
 }
 
-// executeTurnStartHooks runs turn_start hooks and returns ephemeral
-// system messages to inject into the model call's messages slice.
-//
-// Unlike session_start, the AdditionalContext from turn_start is NOT
-// persisted to the session — it's recomputed every turn. This is the
-// right semantics for fast-changing context like "Today's date" or the
-// contents of a prompt file the user might be editing during the session.
+// executeTurnStartHooks fires turn_start before each model call and
+// returns its AdditionalContext as transient system messages. Like
+// session_start the result is never persisted, but turn_start runs
+// every iteration so its content is recomputed each turn — the right
+// semantics for fast-changing context like the current date or the
+// contents of a prompt file the user might be editing mid-session.
 func (r *LocalRuntime) executeTurnStartHooks(ctx context.Context, sess *session.Session, a *agent.Agent, events chan Event) []chat.Message {
-	result := r.dispatchHook(ctx, a, hooks.EventTurnStart, &hooks.Input{
+	return contextMessages(r.dispatchHook(ctx, a, hooks.EventTurnStart, &hooks.Input{
 		SessionID: sess.ID,
-	}, events)
+	}, events))
+}
+
+// contextMessages converts a context-providing hook's AdditionalContext
+// into a one-element transient system-message slice ready to thread
+// through [session.Session.GetMessages]. Returns nil for empty results
+// so callers can pass it straight to [slices.Concat] without a guard.
+func contextMessages(result *hooks.Result) []chat.Message {
 	if result == nil || result.AdditionalContext == "" {
 		return nil
 	}

pkg/runtime/loop.go

@@ -156,8 +156,12 @@ func (r *LocalRuntime) RunStream(ctx context.Context, sess *session.Session) <-c
 
 		a := r.resolveSessionAgent(sess)
 
-		// Execute session start hooks
-		r.executeSessionStartHooks(ctx, sess, a, events)
+		// session_start fires once per RunStream. Its AdditionalContext
+		// (typically the AddEnvironmentInfo env block) is held as transient
+		// extras and threaded into every model call below — never persisted,
+		// to keep the visible transcript clean and the user message tail
+		// stable.
+		sessionStartMsgs := r.executeSessionStartHooks(ctx, sess, a, events)
 
 		// Emit team information
 		events <- TeamInfo(r.agentDetailsFromTeam(), a.Name())
@@ -366,13 +370,14 @@ func (r *LocalRuntime) RunStream(ctx context.Context, sess *session.Session) <-c
 			}
 
 			// Run turn_start hooks BEFORE building messages so their
-			// AdditionalContext can be spliced after the invariant cache
-			// checkpoint and before the conversation history. The hook
-			// output is not persisted to the session, so per-turn signals
-			// (date, prompt files) refresh every turn without bloating the
-			// stored history.
+			// AdditionalContext, alongside the session_start extras captured
+			// once at the top of RunStream, can be spliced after the invariant
+			// cache checkpoint and before the conversation history. Neither
+			// hook's output is persisted, so per-turn signals (date, prompt
+			// files) refresh every turn while session-level context (cwd, OS,
+			// arch) stays stable — all without bloating the stored history.
 			turnStartMsgs := r.executeTurnStartHooks(ctx, sess, a, events)
-			messages := sess.GetMessages(a, turnStartMsgs...)
+			messages := sess.GetMessages(a, slices.Concat(sessionStartMsgs, turnStartMsgs)...)
 			slog.Debug("Retrieved messages for processing", "agent", a.Name(), "message_count", len(messages))
 
 			// Strip image content from messages if the model doesn't support image input.

pkg/runtime/runtime_test.go

@@ -1994,6 +1994,75 @@ func TestRunStream_EmptyMessages_SendUserMessage(t *testing.T) {
 	require.NotEmpty(t, events)
 }
 
+// TestRunStream_AddEnvironmentInfo_DoesNotPolluteSession pins the
+// regression where session_start hook output (the AddEnvironmentInfo
+// env block) was persisted as a system message on the session AFTER
+// the user's first message had already been added, then surfaced
+// verbatim as the [UserMessageEvent] because the runtime relays
+// messages[len-1] as the "current" user message.
+func TestRunStream_AddEnvironmentInfo_DoesNotPolluteSession(t *testing.T) {
+	t.Parallel()
+
+	stream := newStreamBuilder().
+		AddContent("reply").
+		AddStopWithUsage(5, 5).
+		Build()
+
+	prov := &mockProvider{id: "test/mock-model", stream: stream}
+	root := agent.New(
+		"root", "You are a test agent",
+		agent.WithModel(prov),
+		agent.WithAddEnvironmentInfo(true),
+	)
+	tm := team.New(team.WithAgents(root))
+
+	rt, err := NewLocalRuntime(
+		tm,
+		WithSessionCompaction(false),
+		WithModelStore(mockModelStore{}),
+		WithWorkingDir(t.TempDir()),
+	)
+	require.NoError(t, err)
+
+	sess := session.New(
+		session.WithUserMessage("hello"),
+		session.WithWorkingDir(t.TempDir()),
+	)
+
+	evCh := rt.RunStream(t.Context(), sess)
+	var events []Event
+	for ev := range evCh {
+		events = append(events, ev)
+	}
+
+	// The persisted transcript must contain only the user message and
+	// the assistant reply — no system message smuggled in by the hook.
+	var roles []chat.MessageRole
+	for _, item := range sess.Messages {
+		if item.IsMessage() {
+			roles = append(roles, item.Message.Message.Role)
+		}
+	}
+	assert.Equal(t,
+		[]chat.MessageRole{chat.MessageRoleUser, chat.MessageRoleAssistant},
+		roles,
+		"session_start hook output must not be persisted as a session message",
+	)
+
+	// The UserMessageEvent must mirror the user's input, not the env
+	// info block produced by the hook.
+	var userEvts []*UserMessageEvent
+	for _, ev := range events {
+		if ue, ok := ev.(*UserMessageEvent); ok {
+			userEvts = append(userEvts, ue)
+		}
+	}
+	require.Len(t, userEvts, 1)
+	assert.Equal(t, "hello", userEvts[0].Message)
+	assert.NotContains(t, userEvts[0].Message, "<env>",
+		"user_message event must not leak the AddEnvironmentInfo block")
+}
+
 // recordingProvider wraps a sequence of mock streams and records the tools
 // passed to each CreateChatCompletionStream call.
 type recordingProvider struct {