-
Notifications
You must be signed in to change notification settings - Fork 238
tab: enforce deterministic model arg and improve tab-name extraction (opencode) #487
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
f958e5b
c48e74a
b71fedb
f756526
33b2586
96291f0
1a3f3cb
ff9fda8
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,15 @@ import type { MaestroSettings } from './persistence'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const LOG_CONTEXT = '[TabNaming]'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Safe debug wrapper to centralize console.debug error isolation | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const safeDebug = (message: string, data?: any) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| console.debug(message, data); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // swallow | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Helper to create handler options with consistent context | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -84,6 +93,7 @@ export function registerTabNamingHandlers(deps: TabNamingHandlerDependencies): v | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| userMessage: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentType: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sessionCustomModel?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sessionSshRemoteConfig?: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| enabled: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| remoteId: string | null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -122,26 +132,142 @@ export function registerTabNamingHandlers(deps: TabNamingHandlerDependencies): v | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| const baseArgs = (agent.args ?? []).filter( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (arg) => arg !== '--dangerously-skip-permissions' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Fetch stored agent config values (user overrides) early so we can | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // prefer the configured model when building args for the tab naming call. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const allConfigs = agentConfigsStore.get('configs', {}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const agentConfigValues = allConfigs[config.agentType] || {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Resolve model id with stricter rules: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Preference: session override -> agent-config model (only if it looks complete) -> agent.defaultModel | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only accept agent-config model when it contains a provider/model (contains a '/') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let resolvedModelId: string | undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (typeof config.sessionCustomModel === 'string' && config.sessionCustomModel.trim()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolvedModelId = config.sessionCustomModel.trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigValues && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof agentConfigValues.model === 'string' && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigValues.model.trim() && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigValues.model.includes('/') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolvedModelId = agentConfigValues.model.trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (agent as any).defaultModel && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof (agent as any).defaultModel === 'string' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolvedModelId = (agent as any).defaultModel as string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+144
to
+156
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.
The IPC handler's config: {
userMessage: string;
agentType: string;
cwd: string;
sessionSshRemoteConfig?: { ... };
}
Suggested change
Consider adding |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Sanitize resolved model id (remove trailing slashes) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (resolvedModelId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolvedModelId = resolvedModelId.replace(/\/+$/, '').trim(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (resolvedModelId === '') resolvedModelId = undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Debug: log resolved model for tab naming | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| safeDebug('[TabNaming] Resolved model', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sessionId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentType: config.agentType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigModel: agentConfigValues.model, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| resolvedModelId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let finalArgs = buildAgentArgs(agent, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| baseArgs, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| prompt: fullPrompt, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cwd: config.cwd, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| readOnlyMode: true, // Always read-only since we're not modifying anything | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| modelId: resolvedModelId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Apply config overrides from store | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const allConfigs = agentConfigsStore.get('configs', {}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const agentConfigValues = allConfigs[config.agentType] || {}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Apply config overrides from store (other overrides such as customArgs/env) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const configResolution = applyAgentConfigOverrides(agent, finalArgs, { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigValues, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sessionCustomModel: resolvedModelId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| finalArgs = configResolution.args; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Debug: log how model was resolved for tab naming requests so we can | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // verify whether session/agent overrides are applied as expected. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| safeDebug('[TabNaming] Config resolution', { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sessionId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentType: config.agentType, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| modelSource: configResolution.modelSource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| agentConfigModel: agentConfigValues?.model, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| finalArgsPreview: finalArgs.slice(0, 40), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Determine the canonical CLI model to inject. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // resolvedModelId stays stable for logging; cliModelId is what gets injected. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If resolvedModelId has no provider prefix, fall back to agent.defaultModel. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // If neither has a provider prefix, still inject the value so the model is | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // not silently dropped when existing model tokens are stripped. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let cliModelId: string | undefined = resolvedModelId; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cliModelId && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| !cliModelId.includes('/') && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (agent as any).defaultModel && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| typeof (agent as any).defaultModel === 'string' && | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| (agent as any).defaultModel.includes('/') | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cliModelId = (agent as any).defaultModel as string; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | |
| cliModelId && | |
| !cliModelId.includes('/') && | |
| (agent as any).defaultModel && | |
| typeof (agent as any).defaultModel === 'string' && | |
| (agent as any).defaultModel.includes('/') | |
| ) { | |
| cliModelId = (agent as any).defaultModel as string; | |
| } | |
| // If resolvedModelId has no provider prefix and came from agent.defaultModel, | |
| // keep it as-is. Only models from session/config should reach here. | |
| // The agent.defaultModel fallback was already tried during resolution. | |
| let cliModelId: string | undefined = resolvedModelId; |
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
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.
Empty catch violates CLAUDE.md error handling guidelines
CLAUDE.md states: "DO let exceptions bubble up" and "silently swallowing errors hides bugs from Sentry". If model canonicalization fails, finalArgs remains unchanged with no indication of the failure, which could lead to incorrect model selection.
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
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.
Logic contradiction - quoted lines filtered before unquoted is used
Line 492 returns false for lines starting with quotes, so the unquoted variable at line 495 is only computed for lines that don't start with quotes. The comment at line 493 says "Allow quoted single-line outputs to be cleaned later", but those lines are already filtered out at line 492.
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.
Unnecessary type casting -
defaultModelis already part ofAgentConfigdefaultModel?: stringwas added to theAgentConfiginterface in this PR (line 101 of definitions.ts). Theas anycast bypasses type safety.