fix: propagate session ID to LLM request headers in rename task #5624

codefromthecrypt · 2025-11-07T01:53:33Z

Summary

Wrap session name generation task in session_context so that session ID injection works via api_client.rs.

Without this, rename task requests lacked the goose-session-id header despite having the session ID as a parameter.

This means you wouldn't get all requests grouped by the same ID, as the rename task is orphaned. By backfilling tests on this, we also pave the way to test more difficult otel trace propagation.

Type of Change

AI Assistance

This PR was created or reviewed with AI assistance

I know what I want, but still struggle with rust, so I used an agent to investigate the several ways of how to do what I want. I also used it to adapt some similar mermaid diagrams to this, so it is easy to understand the mechanics.

Testing

Unit test matrix covers success, error, and panic cases. Integration test verifies
goose-session-id header appears in rename task's LLM requests without timing dependencies.

Sets foundation for future trace context propagation tests.

Screenshots/Demos (for UX changes)

Before: ApiClient cannot see the session ID on rename

sequenceDiagram
    participant Agent
    participant ReplyInternal
    participant NamingTask
    participant ApiClient
    participant LLM

    Agent->>ReplyInternal: reply(session_id=abc-123)

    rect rgb(255, 200, 200)
    Note over ReplyInternal,LLM: Before: rename task missing session header
    ReplyInternal->>NamingTask: tokio::spawn(without context)
    NamingTask->>ApiClient: generate_session_name()
    Note over ApiClient: current_session_id() → None
    ApiClient->>LLM: POST /chat (no goose-session-id)
    LLM-->>NamingTask: "Generated Name"
    end

    Note over NamingTask: Testing requires sleep-based timing

After: ApiClient can see the session ID on rename

sequenceDiagram
    participant Agent
    participant ReplyInternal
    participant NamingTask
    participant ApiClient
    participant LLM

    Agent->>ReplyInternal: reply(session_id=abc-123)

    rect rgb(191, 223, 255)
    Note over ReplyInternal,LLM: After: session ID injected in headers
    ReplyInternal->>NamingTask: tokio::spawn(with_session_id(abc-123))
    NamingTask->>ApiClient: generate_session_name()
    Note over ApiClient: current_session_id() → abc-123
    ApiClient->>LLM: POST /chat (goose-session-id: abc-123)
    LLM-->>NamingTask: "Generated Name"
    end

    Note over ReplyInternal: Process messages...
    ReplyInternal->>NamingTask: await handle
    NamingTask-->>ReplyInternal: complete
    Note over Agent: Task completion verified (deterministic)

Pull Request Overview

This PR implements conditional session naming to avoid overwriting user-provided session names with auto-generated ones. The key change is to check if the user has explicitly set a session name before spawning the automatic naming task, ensuring user preferences are preserved.

Adds a check for user_set_name flag before spawning the naming task
Waits for the naming task to complete before returning from the reply stream
Includes comprehensive test coverage for both user-provided and auto-generated names

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/goose/src/agents/agent.rs

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

crates/goose/src/agents/agent.rs

codefromthecrypt · 2025-11-07T02:02:05Z

marking draft as I didn't realize we have copilot now, will undraft when done satisfying it.

codefromthecrypt · 2025-11-07T02:13:50Z

crates/goose/src/agents/agent.rs

            }
+
+            // Wait for naming task to complete if it was spawned
+            // We don't propagate panics since session naming is a low-value background task


I added this to satisfy the bot even though it isn't typical in this project to spam the user on things that don't matter. open to advice from a human on it

I disagree with the bot. these warning just go to the log, we won't be looking into it.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

DOsinga

Thanks for doing this. I am not sure I understand the fix though.

you check:
let naming_handle = if !session.user_set_name {

but in the maybe_update_name we have the same check already:

if session.user_set_name {
            return Ok(());
        }

so in which conditions does this not work?

DOsinga · 2025-11-07T14:43:33Z

crates/goose/src/agents/agent.rs

            }
+
+            // Wait for naming task to complete if it was spawned
+            // We don't propagate panics since session naming is a low-value background task


I disagree with the bot. these warning just go to the log, we won't be looking into it.

codefromthecrypt · 2025-11-08T04:36:17Z

@DOsinga good point about the check being internal there. I think probably in the process of trying to make a black box test I may have goofed about this improving the opt-out of the llm call, where it just opts out of spawning a task which could outlive the calling function.

I had several things in progress since several weeks ago on a local branch, and got things mixed up.

So TL;DR: this doesn't obviate the expensive LLM call (already done) all it does is ensure the task doesn't escape the closure of the calling function, which you could argue is a helpful or not. If you want it to escape, it makes things a bit harder to do with replay tests. The fact that there are none yet, that I'm aware of, might reduce the impact on this point.

Over to you.

codefromthecrypt · 2025-11-08T23:11:34Z

@DOsinga so, I chopped this off a pending otel propagation PR, and lost some signal in doing so. I redid this PR for the essentials which are not about whether rename happens or not.

make rename testable without sleeps (bind the task to the outer closure)
fix the missing session ID propagation when renaming the session (Propagate session ID in LLM and MCP requests #5165 missed this)
backfill missing tests so that when we go back for otel, there's an example of how to reach this part.

codefromthecrypt · 2025-11-08T23:13:18Z

crates/goose/src/agents/agent.rs

-            }
-        });
+        let naming_handle = tokio::spawn(
+            crate::session_context::with_session_id(Some(session_id.clone()), async move {


without this, even though the session id is propagated manually, it is not available to api_client which looks for it in context, this is the missing piece and also a place for folks to look at when they get around to otel. Any async seams need a hard look or things like session or in the future trace propagation will break.

codefromthecrypt · 2025-11-08T23:14:43Z

crates/goose/src/agents/agent.rs

+    // Verifies session ID is visible in generate_session_name when user hasn't provided a name.
+    // When user provides name, maybe_update_name early-returns and session ID isn't captured.
+    #[test_case(NamingBehavior::Success, None, "Generated Name", true)]
+    #[test_case(NamingBehavior::Error, None, "initial", true)]


error and panic cases here are just to make sure our ignoring the same doesn't accidentally crash the critical code. we didn't have a problem so far, but it is easy to create one, so these cases help IMHO

Wrap session name generation task in session_context to inject session ID header into LLM requests via api_client.rs. Without this, rename task requests lacked the goose-session-id header despite having the session ID as a parameter. Follows up block#5165 by extending session ID propagation to the rename task. ```mermaid sequenceDiagram participant Agent participant ReplyInternal participant NamingTask participant ApiClient participant LLM Agent->>ReplyInternal: reply(session_id=abc-123) rect rgb(255, 200, 200) Note over ReplyInternal,LLM: Before: rename task missing session header ReplyInternal->>NamingTask: tokio::spawn(without context) NamingTask->>ApiClient: generate_session_name() Note over ApiClient: current_session_id() → None ApiClient->>LLM: POST /chat (no goose-session-id) LLM-->>NamingTask: "Generated Name" end Note over NamingTask: Testing requires sleep-based timing ``` ```mermaid sequenceDiagram participant Agent participant ReplyInternal participant NamingTask participant ApiClient participant LLM Agent->>ReplyInternal: reply(session_id=abc-123) rect rgb(191, 223, 255) Note over ReplyInternal,LLM: After: session ID injected in headers ReplyInternal->>NamingTask: tokio::spawn(with_session_id(abc-123)) NamingTask->>ApiClient: generate_session_name() Note over ApiClient: current_session_id() → abc-123 ApiClient->>LLM: POST /chat (goose-session-id: abc-123) LLM-->>NamingTask: "Generated Name" end Note over ReplyInternal: Process messages... ReplyInternal->>NamingTask: await handle NamingTask-->>ReplyInternal: complete Note over Agent: Task completion verified (deterministic) ``` Unit test matrix covers success, error, and panic cases. Integration test verifies goose-session-id header appears in rename task's LLM requests without timing dependencies. Sets foundation for future trace context propagation tests. Signed-off-by: Adrian Cole <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codefromthecrypt · 2025-11-10T12:32:16Z

opened #5649 as especially as even trivial workflows need approval, it is annoying to find one missed then repeat the rebase drift loop again. I think we can make it easier to not fail

codefromthecrypt · 2025-11-11T10:33:55Z

crates/goose/src/agents/agent.rs

-                warn!("Failed to generate session description: {}", e);
-            }
-        });
+        let naming_handle = tokio::spawn(crate::session_context::with_session_id(


@alexhancock so this code and the test below makes sure the session rename task is also grouped by the session.id, which ack is not something people always care about, but if you think systemically there's probably no value in intentionally orphaning it. I noticed your ACP pr, if it top-levels the session ID stuff, will end up needing a similar line in spawn. #5657

codefromthecrypt · 2025-11-11T11:42:13Z

gonna save our time on this. Anyone who wants to harvest tests or approach here at the point it becomes relevant, do so. Meanwhile I think review and rebase duty is better spent elsewhere.

Copilot AI review requested due to automatic review settings November 7, 2025 01:53

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 7, 2025 01:58

Copilot AI reviewed Nov 7, 2025

View reviewed changes

crates/goose/src/agents/agent.rs Outdated Show resolved Hide resolved

crates/goose/src/agents/agent.rs Show resolved Hide resolved

codefromthecrypt marked this pull request as draft November 7, 2025 02:02

codefromthecrypt force-pushed the perf/skip-session-naming-when-user-provided branch from 3e15d21 to 0e33d76 Compare November 7, 2025 02:07

codefromthecrypt commented Nov 7, 2025

View reviewed changes

codefromthecrypt marked this pull request as ready for review November 7, 2025 02:14

Copilot AI review requested due to automatic review settings November 7, 2025 02:14

Copilot AI reviewed Nov 7, 2025

View reviewed changes

DOsinga reviewed Nov 7, 2025

View reviewed changes

codefromthecrypt force-pushed the perf/skip-session-naming-when-user-provided branch from 0e33d76 to 0241d55 Compare November 8, 2025 23:01

codefromthecrypt changed the title ~~fix: honor user-provided session names~~ fix: propagate session ID to LLM request headers in rename task Nov 8, 2025

codefromthecrypt commented Nov 8, 2025

View reviewed changes

codefromthecrypt force-pushed the perf/skip-session-naming-when-user-provided branch from 0241d55 to cf757c7 Compare November 10, 2025 12:26

Copilot AI review requested due to automatic review settings November 10, 2025 12:26

Copilot AI reviewed Nov 10, 2025

View reviewed changes

codefromthecrypt requested a review from DOsinga November 10, 2025 12:38

codefromthecrypt commented Nov 11, 2025

View reviewed changes

codefromthecrypt closed this Nov 11, 2025

fix: propagate session ID to LLM request headers in rename task #5624

fix: propagate session ID to LLM request headers in rename task #5624

Uh oh!

Conversation

codefromthecrypt commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of Change

AI Assistance

Testing

Screenshots/Demos (for UX changes)

Before: ApiClient cannot see the session ID on rename

After: ApiClient can see the session ID on rename

Related

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

codefromthecrypt commented Nov 7, 2025

Uh oh!

codefromthecrypt Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

DOsinga Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

DOsinga left a comment

Choose a reason for hiding this comment

Uh oh!

DOsinga Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Nov 8, 2025

Uh oh!

codefromthecrypt commented Nov 8, 2025

Uh oh!

codefromthecrypt Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

codefromthecrypt commented Nov 10, 2025

Uh oh!

codefromthecrypt Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Nov 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codefromthecrypt commented Nov 7, 2025 •

edited

Loading