-
Notifications
You must be signed in to change notification settings - Fork 0
fix(harness): improve auto-rename prompt quality #422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9af5a2b
afed30b
cca4606
55ff47c
474210d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,13 +13,6 @@ import { calculateCost } from './types.js'; | |
|
|
||
| const log = createLogger('provider:anthropic'); | ||
|
|
||
| /** Vertex AI model name mapping (Vertex uses different naming). */ | ||
| const VERTEX_MODEL_MAP: Record<string, string> = { | ||
| 'claude-opus-4-6': 'claude-opus-4-6@20250514', | ||
| 'claude-sonnet-4-6': 'claude-sonnet-4-6@20250514', | ||
| 'claude-haiku-4-5': 'claude-3-5-haiku@20241022', | ||
| }; | ||
|
|
||
| export interface AnthropicVertexProviderOptions { | ||
| /** GCP project ID. Falls back to ANTHROPIC_VERTEX_PROJECT_ID env var. */ | ||
| projectId?: string; | ||
|
|
@@ -58,7 +51,7 @@ export class AnthropicVertexModelProvider implements ModelProvider { | |
| const systemMessages = messages.filter((m) => m.role === 'system'); | ||
| const conversationMessages = messages.filter((m) => m.role !== 'system'); | ||
|
|
||
| const apiModel = this.isVertex ? (VERTEX_MODEL_MAP[this.model] ?? this.model) : this.model; | ||
| const apiModel = this.model; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 regressions: The VERTEX_MODEL_MAP removal means callers must now pass Vertex-format model names (e.g. 'claude-haiku-4-5@20251001') directly. If any caller passes standard API names like 'claude-opus-4-6' to a Vertex client, the API call will fail — previously the map translated these. Verify that all call sites that construct AnthropicVertexModelProvider with Vertex enabled pass Vertex-compatible model names. |
||
|
|
||
| const response = await this.client.messages.create({ | ||
| model: apiModel, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -156,21 +156,12 @@ describe('generateSessionName', () => { | |
|
|
||
| expect(result).toBe('Auth Bug Fix Session'); | ||
| expect(mockCreate).toHaveBeenCalledOnce(); | ||
| expect(mockCreate).toHaveBeenCalledWith( | ||
| { | ||
| model: AUTO_RENAME_MODEL, | ||
| max_tokens: 20, | ||
| system: | ||
| 'What is the user trying to accomplish? Generate a 3-6 word title capturing their intent or goal. Be action-oriented and specific. Return only the title, nothing else.', | ||
| messages: [ | ||
| { | ||
| role: 'user', | ||
| content: 'Fix the auth bug\nUpdate login page', | ||
| }, | ||
| ], | ||
| }, | ||
| { timeout: 5000 }, | ||
| ); | ||
| const call = mockCreate.mock.calls[0]; | ||
| expect(call[0].model).toBe(AUTO_RENAME_MODEL); | ||
| expect(call[0].max_tokens).toBe(20); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 style: The server test relaxed its assertion from exact argument matching (toHaveBeenCalledWith) to individual field checks (call[0].model, call[0].messages[0].content). This is fine for resilience against prompt changes, but the system prompt (NAMING_SYSTEM_PROMPT) is now completely unverified in the server test suite. Consider adding a minimal assertion like |
||
| expect(call[0].messages[0].content).toContain('Fix the auth bug'); | ||
| expect(call[0].messages[0].content).toContain('Update login page'); | ||
| expect(call[1]).toEqual({ timeout: 5000 }); | ||
| }); | ||
|
|
||
| it('falls back to keyword extraction when API call fails', async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 unsafe_assumptions: The quote-stripping regex uses a character class that includes both whitespace (\s) and quote chars with
+quantifier:^[\s"'\u201C\u201D\u2018\u2019]+. This means a title like" Spaced Title "correctly producesSpaced Title, but a title that legitimately starts with an apostrophe (e.g.'Twas) would lose it. Low-risk for session names but worth noting.