fix(backend): remove initialPrompt skip that blocked display name fallback#1562
Open
quay-devel wants to merge 9 commits into
Open
fix(backend): remove initialPrompt skip that blocked display name fallback#1562quay-devel wants to merge 9 commits into
quay-devel wants to merge 9 commits into
Conversation
…lback
Sessions intermittently showed raw session IDs instead of friendly display
names. The AG-UI proxy's triggerDisplayNameGenerationIfNeeded skipped messages
matching initialPrompt (agui_proxy.go:1005-1009), which was intended to
prevent double-generation since CreateSession already fires
GenerateDisplayNameAsync for initialPrompt sessions. However, this also
blocked the fallback path: when CreateSession's async generation failed
(API timeout, Vertex errors), Path B in the AG-UI proxy couldn't retry
because it skipped the very message it needed to generate from.
The skip was redundant — two existing guards already prevent
double-generation:
1. ShouldGenerateDisplayName() returns false if displayName is already set
2. updateSessionDisplayNameInternal() has a first-write-wins race guard
Removing the skip lets the AG-UI proxy naturally retry display name
generation when the runner POSTs the initial prompt to /agui/run, without
risk of overwrites.
Includes 4 regression tests for triggerDisplayNameGenerationIfNeeded
covering: initialPrompt not skipped, skip when name already set, skip
when no user message, and skip when DynamicClient is nil.
## Test plan
- [x] All 26 websocket package tests pass
- [x] gofmt and go vet clean
- [ ] Verify in staging: create sessions with initialPrompt, confirm
display name appears even when Vertex API is slow/unavailable
Fixes ambient-code#1561
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughDisplay-name generation now triggers consistently on first AG-UI proxy interaction without skipping when the user message matches ChangesDisplay-Name Generation Trigger Fix
Possibly Related Issues
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Make GenerateDisplayNameAsync a package-level function variable (matching the existing pattern used for DynamicClient, GetGitHubToken, etc.) so the regression test can swap it and assert it was actually called. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/backend/websocket/agui_proxy_test.go (1)
287-338: ⚡ Quick winStrengthen skip-path tests with explicit non-invocation assertions
The skip cases currently validate outcomes indirectly, so they can still pass if
GenerateDisplayNameAsyncis called but fails early. Stubhandlers.GenerateDisplayNameAsyncand assert it is not called in each skip scenario.Proposed test hardening diff
func TestTriggerDisplayName_SkipsWhenNameAlreadySet(t *testing.T) { @@ + called := false + oldFn := handlers.GenerateDisplayNameAsync + handlers.GenerateDisplayNameAsync = func(projectName, sessionName, userMessage string, sessionCtx handlers.SessionContext) { + called = true + } + defer func() { handlers.GenerateDisplayNameAsync = oldFn }() + msgs := []types.Message{ {ID: "msg-1", Role: "user", Content: "Help me debug auth"}, } triggerDisplayNameGenerationIfNeeded("test-project", "test-session", msgs) + if called { + t.Error("Expected GenerateDisplayNameAsync not to be called when displayName is already set") + } @@ func TestTriggerDisplayName_SkipsWhenNoUserMessage(t *testing.T) { @@ + called := false + oldFn := handlers.GenerateDisplayNameAsync + handlers.GenerateDisplayNameAsync = func(projectName, sessionName, userMessage string, sessionCtx handlers.SessionContext) { + called = true + } + defer func() { handlers.GenerateDisplayNameAsync = oldFn }() + // Only assistant messages — no user content to generate from msgs := []types.Message{ {ID: "msg-1", Role: "assistant", Content: "I'll help you debug"}, } triggerDisplayNameGenerationIfNeeded("test-project", "test-session", msgs) + if called { + t.Error("Expected GenerateDisplayNameAsync not to be called when there is no user message") + } @@ func TestTriggerDisplayName_SkipsWhenDynamicClientNil(t *testing.T) { @@ + called := false + oldFn := handlers.GenerateDisplayNameAsync + handlers.GenerateDisplayNameAsync = func(projectName, sessionName, userMessage string, sessionCtx handlers.SessionContext) { + called = true + } + defer func() { handlers.GenerateDisplayNameAsync = oldFn }() + msgs := []types.Message{ {ID: "msg-1", Role: "user", Content: "Help me debug auth"}, } // Should return early without panic when DynamicClient is nil triggerDisplayNameGenerationIfNeeded("test-project", "test-session", msgs) + if called { + t.Error("Expected GenerateDisplayNameAsync not to be called when DynamicClient is nil") + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/backend/websocket/agui_proxy_test.go` around lines 287 - 338, For each skip-path test (TestTriggerDisplayName_SkipsWhenNameAlreadySet, TestTriggerDisplayName_SkipsWhenNoUserMessage, TestTriggerDisplayName_SkipsWhenDynamicClientNil) stub handlers.GenerateDisplayNameAsync at test start to flip a local "called" flag if invoked, call triggerDisplayNameGenerationIfNeeded with the existing test inputs, then assert the "called" flag is false; restore the original handlers.GenerateDisplayNameAsync and any modified handlers.DynamicClient after the test so other tests are unaffected; this ensures triggerDisplayNameGenerationIfNeeded truly avoids invoking GenerateDisplayNameAsync in those skip scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@components/backend/websocket/agui_proxy_test.go`:
- Around line 287-338: For each skip-path test
(TestTriggerDisplayName_SkipsWhenNameAlreadySet,
TestTriggerDisplayName_SkipsWhenNoUserMessage,
TestTriggerDisplayName_SkipsWhenDynamicClientNil) stub
handlers.GenerateDisplayNameAsync at test start to flip a local "called" flag if
invoked, call triggerDisplayNameGenerationIfNeeded with the existing test
inputs, then assert the "called" flag is false; restore the original
handlers.GenerateDisplayNameAsync and any modified handlers.DynamicClient after
the test so other tests are unaffected; this ensures
triggerDisplayNameGenerationIfNeeded truly avoids invoking
GenerateDisplayNameAsync in those skip scenarios.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d69edfa7-7b46-4b85-94f9-28521466dd60
📒 Files selected for processing (4)
components/backend/handlers/display_name.gocomponents/backend/handlers/sessions.gocomponents/backend/websocket/agui_proxy.gocomponents/backend/websocket/agui_proxy_test.go
💤 Files with no reviewable changes (1)
- components/backend/websocket/agui_proxy.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
initialPromptskip guard inagui_proxy.go:1005-1009that prevented the AG-UI proxy from acting as a fallback when CreateSession's async display name generation failedsessions.goto document the new fallback relationship between the two generation pathstriggerDisplayNameGenerationIfNeededcovering the key code pathsProblem
Sessions intermittently showed raw session IDs (
session-<UUID>) instead of friendly display names. The display name system has two generation paths:GenerateDisplayNameAsyncin a goroutine with 10s timeout/agui/runPOSTPath B intentionally skipped messages matching
initialPromptto prevent double-generation. But this also blocked Path B from acting as a fallback when Path A failed (API timeout, Vertex errors, missing credentials). The only recovery was the user sending a second message.Fix
The
initialPromptskip was redundant — two existing guards already prevent double-generation:ShouldGenerateDisplayName(spec)returnsfalseifdisplayNameis already setupdateSessionDisplayNameInternalhas a first-write-wins race guard atdisplay_name.go:302-307Removing the skip lets Path B naturally retry when Path A fails, with no risk of overwrites. Worst case in a race: one redundant Haiku API call (~$0.001).
Test plan
gofmtandgo vetcleaninitialPrompt, confirm display name appears even when Vertex API is slow/unavailableSummary by CodeRabbit
Bug Fixes
Tests