tab: enforce deterministic model arg and improve tab-name extraction (opencode)#487
Open
chr1syy wants to merge 4 commits intoRunMaestro:mainfrom
Open
tab: enforce deterministic model arg and improve tab-name extraction (opencode)#487chr1syy wants to merge 4 commits intoRunMaestro:mainfrom
chr1syy wants to merge 4 commits intoRunMaestro:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds session-level model override and stricter model-resolution, canonicalization, sanitization, SSH/stdin handling, and diagnostics to the tab-naming IPC flow; extends agent and IPC typings to include Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Renderer
participant IPC as Main IPC
participant Handler as TabNamingHandler
participant Agent as Agent/CLI
participant Extractor as extractTabName
Renderer->>IPC: generateTabName(userMessage, agentType, cwd, sessionCustomModel)
IPC->>Handler: forward request
Handler->>Handler: load agent config (+ defaultModel), apply overrides
Handler->>Handler: resolve model (sessionCustomModel > agent-config model with '/' > agent.defaultModel)
Handler->>Handler: sanitize args, strip existing --model/-m, inject canonical --model
alt remote SSH
Handler->>Agent: wrap command for SSH (maybe add --input-format), send prompt via stdin if supported
else local
Handler->>Agent: spawn local process with finalArgs
end
Agent-->>Handler: stdout/stderr (raw output)
Handler->>Extractor: extractTabName(raw output)
Extractor-->>Handler: tab name candidate or warning
Handler-->>IPC: return tab name + diagnostics
IPC-->>Renderer: deliver tab name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/ipc/handlers/tabNaming.ts (1)
261-262:⚠️ Potential issue | 🟡 MinorNon-adjacent includes check may produce false positives.
The condition
finalArgs.includes('--input-format') && finalArgs.includes('stream-json')could incorrectly return true if'stream-json'appears elsewhere in the args (e.g., as a value for a different flag). Consider checking adjacency:-const hasStreamJsonInput = - finalArgs.includes('--input-format') && finalArgs.includes('stream-json'); +const inputFormatIdx = finalArgs.indexOf('--input-format'); +const hasStreamJsonInput = + inputFormatIdx !== -1 && finalArgs[inputFormatIdx + 1] === 'stream-json';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 261 - 262, The current hasStreamJsonInput check can false-positive because it only checks finalArgs.includes('--input-format') && finalArgs.includes('stream-json'); instead, find the index of '--input-format' in finalArgs (e.g., const idx = finalArgs.indexOf('--input-format')) and set hasStreamJsonInput = idx !== -1 && finalArgs[idx + 1] === 'stream-json' so you verify adjacency; update the code around the hasStreamJsonInput variable in tabNaming.ts accordingly.
🧹 Nitpick comments (4)
src/main/ipc/handlers/tabNaming.ts (4)
135-152: Type casting suggests missing type definition forsessionCustomModel.The config parameter is cast to
anyto accesssessionCustomModel, but this property isn't defined in the config type (lines 83-91). If this property is expected, add it to the type definition for type safety.async (config: { userMessage: string; agentType: string; cwd: string; + sessionCustomModel?: string; sessionSshRemoteConfig?: { enabled: boolean; remoteId: string | null; workingDirOverride?: string; }; }): Promise<string | null> => {Then replace the
(config as any).sessionCustomModelcasts with direct property access.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 135 - 152, The code uses (config as any).sessionCustomModel to read sessionCustomModel and bypasses typing; add sessionCustomModel?: string to the config type/interface used by the function that declares the config parameter so the property is properly typed, then remove the casts and access config.sessionCustomModel directly when setting resolvedModelId (the same pattern should be preserved for resolvedModelId, agentConfigValues.model and (agent as any).defaultModel — replace the (agent as any).defaultModel cast by adding defaultModel?: string to the Agent type/interface or using the existing Agent type so you can use agent.defaultModel directly).
391-402: MutatingresolvedModelIdaffects diagnostic logging.The mutation at line 401 changes
resolvedModelIdafter it was originally resolved. This variable is later used in logging (line 468), which will show the modified value rather than the original resolution. Consider using a separate variable for the final injected model.+ let finalModelForInjection = resolvedModelId; if ( - !resolvedModelId.includes('/') && + !finalModelForInjection.includes('/') && (agent as any).defaultModel && typeof (agent as any).defaultModel === 'string' && (agent as any).defaultModel.includes('/') ) { - resolvedModelId = (agent as any).defaultModel as string; + finalModelForInjection = (agent as any).defaultModel as string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 391 - 402, The code currently mutates resolvedModelId (in the block that prefers agent.defaultModel when resolvedModelId lacks a provider prefix), which alters the value later used for diagnostic logging; instead, introduce a new variable (e.g., injectedModelId or cliModelId) to hold the model string that will be injected as a CLI flag and leave resolvedModelId unchanged for logs. Update the injection logic to assign to injectedModelId when you would have reassigned resolvedModelId and use injectedModelId for any CLI/config insertion while keeping resolvedModelId for later logging and diagnostics; reference resolvedModelId, (agent as any).defaultModel, and the injection site in tabNaming.ts when making the change.
161-171: Overly defensive try/catch around debug logging.The try/catch blocks wrapping
console.debugcalls appear throughout the file (lines 161-171, 190-201, 293-317, etc.).console.debugrarely throws, so this adds noise without meaningful protection.Consider a helper function if error isolation is truly needed:
const safeDebug = (message: string, data?: object) => { try { console.debug(message, data); } catch { /* swallow */ } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 161 - 171, Remove the repetitive try/catch wrappers around console.debug in this module and replace them with a single safeDebug helper; create a top-level const safeDebug = (msg: string, data?: any) => { try { console.debug(msg, data); } catch { /* swallow */ } } and then change each protected console.debug call (the ones in tabNaming.ts that currently wrap sessionId/agentType/agentConfigModel/resolvedModelId and similar debug blocks) to call safeDebug(message, data) instead; this keeps error isolation but eliminates noisy duplicated try/catch blocks and centralizes behavior.
319-374: Redundant model flag processing.The deduplication logic (lines 323-357) extracts the last
--modelvalue and consolidates it, then lines 358-374 convert--model <value>to--model=<value>, and finally lines 376-424 strip all model tokens and re-inject the canonical one.Since lines 376-424 remove all model tokens anyway, the earlier deduplication and conversion steps are effectively discarded. Consider simplifying to just:
- Remove all model tokens
- Inject the canonical model
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 319 - 374, The earlier two-step dedupe/convert logic around finalArgs (the inner try using lastModelVal and the rebuilt conversion into `--model=<value>`) is redundant because later code already strips model tokens and reinjects a canonical model; remove the whole deduplication block (the try { ... } that computes lastModelVal and the subsequent loop that builds rebuilt) and replace it with a single pass before the prompt separator: compute sepIndex from finalArgs, filter out any `--model` and `--model=...` tokens in finalArgs.slice(0, sepIndex), then if you have a canonical model id (use resolvedModelId or the previously determined canonical variable), push `--model=<canonical>` into the filtered args, and set finalArgs = [...filteredArgs, ...finalArgs.slice(sepIndex)]; keep the console.debug that logs sessionId and the canonical value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 261-262: The current hasStreamJsonInput check can false-positive
because it only checks finalArgs.includes('--input-format') &&
finalArgs.includes('stream-json'); instead, find the index of '--input-format'
in finalArgs (e.g., const idx = finalArgs.indexOf('--input-format')) and set
hasStreamJsonInput = idx !== -1 && finalArgs[idx + 1] === 'stream-json' so you
verify adjacency; update the code around the hasStreamJsonInput variable in
tabNaming.ts accordingly.
---
Nitpick comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 135-152: The code uses (config as any).sessionCustomModel to read
sessionCustomModel and bypasses typing; add sessionCustomModel?: string to the
config type/interface used by the function that declares the config parameter so
the property is properly typed, then remove the casts and access
config.sessionCustomModel directly when setting resolvedModelId (the same
pattern should be preserved for resolvedModelId, agentConfigValues.model and
(agent as any).defaultModel — replace the (agent as any).defaultModel cast by
adding defaultModel?: string to the Agent type/interface or using the existing
Agent type so you can use agent.defaultModel directly).
- Around line 391-402: The code currently mutates resolvedModelId (in the block
that prefers agent.defaultModel when resolvedModelId lacks a provider prefix),
which alters the value later used for diagnostic logging; instead, introduce a
new variable (e.g., injectedModelId or cliModelId) to hold the model string that
will be injected as a CLI flag and leave resolvedModelId unchanged for logs.
Update the injection logic to assign to injectedModelId when you would have
reassigned resolvedModelId and use injectedModelId for any CLI/config insertion
while keeping resolvedModelId for later logging and diagnostics; reference
resolvedModelId, (agent as any).defaultModel, and the injection site in
tabNaming.ts when making the change.
- Around line 161-171: Remove the repetitive try/catch wrappers around
console.debug in this module and replace them with a single safeDebug helper;
create a top-level const safeDebug = (msg: string, data?: any) => { try {
console.debug(msg, data); } catch { /* swallow */ } } and then change each
protected console.debug call (the ones in tabNaming.ts that currently wrap
sessionId/agentType/agentConfigModel/resolvedModelId and similar debug blocks)
to call safeDebug(message, data) instead; this keeps error isolation but
eliminates noisy duplicated try/catch blocks and centralizes behavior.
- Around line 319-374: The earlier two-step dedupe/convert logic around
finalArgs (the inner try using lastModelVal and the rebuilt conversion into
`--model=<value>`) is redundant because later code already strips model tokens
and reinjects a canonical model; remove the whole deduplication block (the try {
... } that computes lastModelVal and the subsequent loop that builds rebuilt)
and replace it with a single pass before the prompt separator: compute sepIndex
from finalArgs, filter out any `--model` and `--model=...` tokens in
finalArgs.slice(0, sepIndex), then if you have a canonical model id (use
resolvedModelId or the previously determined canonical variable), push
`--model=<canonical>` into the filtered args, and set finalArgs =
[...filteredArgs, ...finalArgs.slice(sepIndex)]; keep the console.debug that
logs sessionId and the canonical value.
Greptile SummaryImproved model resolution, argument sanitization, and debugging for tab naming, but introduced a model precedence bug and error handling issues. Key Changes:
Critical Issues:
Confidence Score: 2/5
Important Files Changed
Last reviewed commit: c48e74a |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
292
to
424
| // Final safety sanitization: ensure args are all plain strings | ||
| try { | ||
| const nonStringItems = finalArgs.filter((a) => typeof a !== 'string'); | ||
| if (nonStringItems.length > 0) { | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[TabNaming] Removing non-string args before spawn', { | ||
| sessionId, | ||
| removed: nonStringItems.map((i) => ({ typeof: typeof i, preview: String(i) })), | ||
| }); | ||
| finalArgs = finalArgs.filter((a) => typeof a === 'string'); | ||
| } | ||
|
|
||
| // Extract model arg value for debugging (if present) | ||
| const modelIndex = finalArgs.indexOf('--model'); | ||
| if (modelIndex !== -1 && finalArgs.length > modelIndex + 1) { | ||
| const modelVal = finalArgs[modelIndex + 1]; | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[TabNaming] Final --model value', { | ||
| sessionId, | ||
| value: modelVal, | ||
| type: typeof modelVal, | ||
| }); | ||
| } | ||
| } catch (err) { | ||
| // swallow safety log errors | ||
| } | ||
|
|
||
| // Quote model values that contain slashes so they survive shell-based | ||
| // spawns (PowerShell can interpret unquoted tokens containing slashes). | ||
| try { | ||
| // Deduplicate --model flags and ensure exactly one is present before the prompt separator | ||
| try { | ||
| const sepIndex = | ||
| finalArgs.indexOf('--') >= 0 ? finalArgs.indexOf('--') : finalArgs.length; | ||
| let lastModelVal: string | undefined; | ||
| for (let i = 0; i < sepIndex; i++) { | ||
| if (finalArgs[i] === '--model' && finalArgs.length > i + 1) { | ||
| const cand = finalArgs[i + 1]; | ||
| if (typeof cand === 'string' && cand.trim()) { | ||
| lastModelVal = cand; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (lastModelVal !== undefined) { | ||
| const newArgs: string[] = []; | ||
| for (let i = 0; i < sepIndex; i++) { | ||
| if (finalArgs[i] === '--model') { | ||
| i++; // skip value | ||
| continue; | ||
| } | ||
| newArgs.push(finalArgs[i]); | ||
| } | ||
| // Insert the single canonical model flag | ||
| newArgs.push('--model', lastModelVal); | ||
| // Append remaining args (including '--' and prompt) | ||
| finalArgs = [...newArgs, ...finalArgs.slice(sepIndex)]; | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[TabNaming] Deduplicated --model flags', { | ||
| sessionId, | ||
| canonical: lastModelVal, | ||
| }); | ||
| } | ||
| } catch (err) { | ||
| // ignore dedupe failures | ||
| } | ||
| // Convert separate --model <value> pairs into a single --model=<value> | ||
| // token so shells don't split values. Then enforce a single canonical | ||
| // CLI model token derived from our resolvedModelId (if available). | ||
| const rebuilt: string[] = []; | ||
| for (let i = 0; i < finalArgs.length; i++) { | ||
| const a = finalArgs[i]; | ||
| if (a === '--model' && i + 1 < finalArgs.length) { | ||
| const raw = finalArgs[i + 1]; | ||
| const val = | ||
| typeof raw === 'string' ? raw.replace(/^['\"]|['\"]$/g, '') : String(raw); | ||
| rebuilt.push(`--model=${val}`); | ||
| i++; // skip the value | ||
| } else { | ||
| rebuilt.push(a); | ||
| } | ||
| } | ||
| finalArgs = rebuilt; | ||
|
|
||
| // Remove any existing model tokens (either --model=... or -m/value) | ||
| const withoutModel: string[] = []; | ||
| for (let i = 0; i < finalArgs.length; i++) { | ||
| const a = finalArgs[i]; | ||
| if (typeof a === 'string' && a.startsWith('--model')) { | ||
| // skip | ||
| continue; | ||
| } | ||
| if (a === '-m' && i + 1 < finalArgs.length) { | ||
| i++; // skip short form value | ||
| continue; | ||
| } | ||
| withoutModel.push(a); | ||
| } | ||
|
|
||
| // If we have a resolvedModelId (from session/agent/default), prefer inserting | ||
| // it explicitly as a CLI flag to avoid relying on OpenCode config/env. | ||
| if (resolvedModelId && typeof resolvedModelId === 'string') { | ||
| // If resolvedModelId doesn't look like provider/model, prefer agent.defaultModel | ||
| if ( | ||
| !resolvedModelId.includes('/') && | ||
| (agent as any).defaultModel && | ||
| typeof (agent as any).defaultModel === 'string' && | ||
| (agent as any).defaultModel.includes('/') | ||
| ) { | ||
| resolvedModelId = (agent as any).defaultModel as string; | ||
| } | ||
|
|
||
| if (resolvedModelId && resolvedModelId.includes('/')) { | ||
| const modelToken = `--model=${resolvedModelId}`; | ||
| // Insert before the argument separator `--` if present | ||
| const sep = withoutModel.indexOf('--'); | ||
| if (sep === -1) { | ||
| withoutModel.push(modelToken); | ||
| } else { | ||
| withoutModel.splice(sep, 0, modelToken); | ||
| } | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[TabNaming] Injected canonical --model for spawn', { | ||
| sessionId, | ||
| model: resolvedModelId, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| finalArgs = withoutModel; | ||
| } catch (err) { | ||
| // swallow | ||
| } |
There was a problem hiding this comment.
Model injection runs after SSH command wrapping
The entire new sanitization and model-injection block (lines 292–424) executes after the SSH command is constructed (lines 244–290). When SSH is enabled, finalArgs at this point already contains the SSH-specific argument list (i.e., the remote agent command has been embedded as a single shell string or as SSH flags). The model-injection logic then scans those SSH args for -- separators and injects --model=<value> into the wrong position, corrupting the SSH command structure.
To fix this, the model injection must happen before buildSshCommand is called, so the model flag ends up inside the remote agent invocation, not in the SSH wrapper:
// ✅ Inject model flag here, before SSH wrapping
// ... (sanitize finalArgs)
if (config.sessionSshRemoteConfig?.enabled && ...) {
const sshCommand = await buildSshCommand(...)
command = sshCommand.command;
finalArgs = sshCommand.args;
}
// ❌ Do NOT modify finalArgs here for SSH sessions
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
376
to
421
| // Remove any existing model tokens (either --model=... or -m/value) | ||
| const withoutModel: string[] = []; | ||
| for (let i = 0; i < finalArgs.length; i++) { | ||
| const a = finalArgs[i]; | ||
| if (typeof a === 'string' && a.startsWith('--model')) { | ||
| // skip | ||
| continue; | ||
| } | ||
| if (a === '-m' && i + 1 < finalArgs.length) { | ||
| i++; // skip short form value | ||
| continue; | ||
| } | ||
| withoutModel.push(a); | ||
| } | ||
|
|
||
| // If we have a resolvedModelId (from session/agent/default), prefer inserting | ||
| // it explicitly as a CLI flag to avoid relying on OpenCode config/env. | ||
| if (resolvedModelId && typeof resolvedModelId === 'string') { | ||
| // If resolvedModelId doesn't look like provider/model, prefer agent.defaultModel | ||
| if ( | ||
| !resolvedModelId.includes('/') && | ||
| (agent as any).defaultModel && | ||
| typeof (agent as any).defaultModel === 'string' && | ||
| (agent as any).defaultModel.includes('/') | ||
| ) { | ||
| resolvedModelId = (agent as any).defaultModel as string; | ||
| } | ||
|
|
||
| if (resolvedModelId && resolvedModelId.includes('/')) { | ||
| const modelToken = `--model=${resolvedModelId}`; | ||
| // Insert before the argument separator `--` if present | ||
| const sep = withoutModel.indexOf('--'); | ||
| if (sep === -1) { | ||
| withoutModel.push(modelToken); | ||
| } else { | ||
| withoutModel.splice(sep, 0, modelToken); | ||
| } | ||
| // eslint-disable-next-line no-console | ||
| console.debug('[TabNaming] Injected canonical --model for spawn', { | ||
| sessionId, | ||
| model: resolvedModelId, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| finalArgs = withoutModel; |
There was a problem hiding this comment.
All model flags stripped but not re-injected when model lacks a provider prefix
The withoutModel loop unconditionally removes every --model=… and -m token from finalArgs (lines 376–389). The model is only re-injected at line 405 when resolvedModelId.includes('/'). If resolvedModelId was resolved from agent.defaultModel and that string has no / component (e.g. just a model short-name), neither the fallback at line 395 nor the injection at line 404 fires, and the spawn happens with no model argument at all — silently, because the outer try/catch swallows everything.
Trace for a model without a provider prefix:
resolvedModelId = "my-model"after initial resolution- Line 395:
!resolvedModelId.includes('/')istrue, but ifagent.defaultModelalso lacks/, the innerifis not entered andresolvedModelIdis unchanged - Line 404:
resolvedModelId.includes('/')isfalse→ no token injected finalArgs = withoutModel— all previous model flags stripped, nothing added back
The agent CLI then runs with whatever its built-in default model is, silently diverging from the configured value. The fix is to inject the model token regardless of whether it contains a /, or at minimum, avoid stripping model tokens that cannot be re-injected.
Comment on lines
134
to
152
| let resolvedModelId: string | undefined; | ||
| if ( | ||
| typeof (config as any).sessionCustomModel === 'string' && | ||
| (config as any).sessionCustomModel.trim() | ||
| ) { | ||
| resolvedModelId = (config as any).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; | ||
| } |
There was a problem hiding this comment.
sessionCustomModel branch is dead code — field is not in the handler config type
The IPC handler's config parameter (lines 83–92) is typed as:
config: {
userMessage: string;
agentType: string;
cwd: string;
sessionSshRemoteConfig?: { ... };
}sessionCustomModel is not part of this type. Accessing it via (config as any).sessionCustomModel will always evaluate to undefined, so the first branch of the model-resolution chain (lines 135–139) is never entered. The PR description lists "session override" as the highest-priority model source, but it will never be applied until sessionCustomModel is added to both the config type and the IPC caller.
Suggested change
| let resolvedModelId: string | undefined; | |
| if ( | |
| typeof (config as any).sessionCustomModel === 'string' && | |
| (config as any).sessionCustomModel.trim() | |
| ) { | |
| resolvedModelId = (config as any).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; | |
| } | |
| let resolvedModelId: string | undefined; | |
| if ( | |
| typeof (config as any).sessionCustomModel === 'string' && | |
| (config as any).sessionCustomModel.trim() | |
| ) { | |
| resolvedModelId = (config as any).sessionCustomModel.trim(); | |
| } else if ( |
Consider adding sessionCustomModel?: string to the config interface and threading the value through from the caller, or removing this branch if it is not yet implemented.
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
393
to
402
| if (resolvedModelId && typeof resolvedModelId === 'string') { | ||
| // If resolvedModelId doesn't look like provider/model, prefer agent.defaultModel | ||
| if ( | ||
| !resolvedModelId.includes('/') && | ||
| (agent as any).defaultModel && | ||
| typeof (agent as any).defaultModel === 'string' && | ||
| (agent as any).defaultModel.includes('/') | ||
| ) { | ||
| resolvedModelId = (agent as any).defaultModel as string; | ||
| } |
There was a problem hiding this comment.
resolvedModelId mutation inside a swallowed try/catch causes misleading debug logs
resolvedModelId is mutated at line 401 inside the outer try/catch whose errors are silently swallowed (line 422–424). This variable is captured by reference in the onExit closure (line 464) and will reflect the post-mutation value when logged after process exit — even if the mutation happened in a code path that was not supposed to be the "winning" resolution.
More importantly, resolvedModelId was already used as sessionCustomModel in applyAgentConfigOverrides (line 184) with the original value. The late mutation only affects the final injection (line 405) and the debug logs, creating an inconsistency between what was passed to applyAgentConfigOverrides and what is ultimately injected/logged.
Consider using a separate variable (e.g. finalModelId) for this late fallback so the original resolvedModelId remains stable for logging purposes.
…, and type safety - Move model token stripping/re-injection before SSH wrapping (fixes corrupted SSH args) - Inject canonical model unconditionally via cliModelId (fixes silent drop for bare model names) - Add sessionCustomModel to config type and thread it from renderer through preload to handler (fixes dead-code branch) - Use separate cliModelId variable for late fallback, leaving resolvedModelId stable for logging - Fix stream-json adjacency check (indexOf + [idx+1] instead of two includes) - Add safeDebug helper to replace repetitive try/catch blocks around console.debug - Add defaultModel?: string to AgentConfig interface Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 490-496: The early check that returns false for lines starting
with a quote (the condition using trimmed.startsWith('"') ||
trimmed.startsWith("'")) is dropping valid single-line quoted tab names; remove
or disable that early-return so quoted single-line outputs like "\"Fix CI flaky
tests\"" can be handled by the later unquoting logic (the unquoted =
trimmed.replace(/^['"]+|['"]+$/g, '') and subsequent return). After removing the
early-return, add a sanity guard to still reject lines that become empty or only
quotes after unquoting (e.g., check unquoted.length > 0) so you don't
re-introduce false positives.
- Around line 245-251: The canonical model reinjection currently hardcodes
'--model=<value>' which breaks agents that expect other flag styles (e.g.,
'-m'). Update the injection logic around cliModelId/sanitized/filteredPrefix
(the Re-inject block and where filteredPrefix is used) to preserve the original
agent's model flag style: inspect the existing args/filteredPrefix for a model
flag pattern (e.g., '-m', '--model', '--model=') and choose the same flag
tokenization (short vs long and joined with '=' vs separate token) when pushing
the sanitized value; fall back to '--model=' only if no model flag form is
detectable. Keep safeDebug and trimming as-is.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/agents/definitions.tssrc/main/ipc/handlers/tabNaming.tssrc/main/preload/tabNaming.tssrc/renderer/global.d.tssrc/renderer/hooks/input/useInputProcessing.ts
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
206
to
214
| 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; | ||
| } |
There was a problem hiding this comment.
Fallback logic violates stated model precedence order by overriding session models.
If resolvedModelId = "gpt-4" (from session without /), and agent.defaultModel = "openai/gpt-4" (with /), this code replaces the session model with the agent default, violating the documented precedence: session > agent config > agent default.
Suggested change
| 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; |
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
155
to
156
| (agent as any).defaultModel && | ||
| typeof (agent as any).defaultModel === 'string' |
There was a problem hiding this comment.
Unnecessary type casting - defaultModel is already part of AgentConfig
defaultModel?: string was added to the AgentConfig interface in this PR (line 101 of definitions.ts). The as any cast bypasses type safety.
Suggested change
| (agent as any).defaultModel && | |
| typeof (agent as any).defaultModel === 'string' | |
| } else if ( | |
| agent.defaultModel && | |
| typeof agent.defaultModel === 'string' | |
| ) { | |
| resolvedModelId = agent.defaultModel; |
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
259
to
261
| } catch (err) { | ||
| // swallow | ||
| } |
There was a problem hiding this comment.
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.
src/main/ipc/handlers/tabNaming.ts
Outdated
Comment on lines
492
to
495
| if (trimmed.startsWith('"') || trimmed.startsWith("'")) return false; | ||
| // Allow quoted single-line outputs to be cleaned later, but lines that begin | ||
| // with quotes are typically examples and should be ignored. | ||
| const unquoted = trimmed.replace(/^['"]+|['"]+$/g, ''); |
There was a problem hiding this comment.
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.
…cedence, and quote filter - Use agent.modelArgs() for model re-injection instead of hardcoded --model= (Codex uses -m, other agents may have different styles) - Remove cliModelId fallback that incorrectly overrode session model with agent.defaultModel when resolvedModelId lacked a provider prefix — resolvedModelId already encodes the correct session > agent-config > agent-default precedence - Remove (agent as any).defaultModel casts — defaultModel is typed on AgentConfig - Remove try/catch swallowing model canonicalization errors (bubbles per CLAUDE.md) - Fix extractTabName quote filter: fully-wrapped quoted strings like "Fix CI tests" are now kept for the unquoting step instead of being discarded early Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/ipc/handlers/tabNaming.ts (2)
358-386: Consider usinglogger.warninstead ofconsole.warnfor consistency.The rest of the file uses the
loggerutility for warnings (e.g., line 119, 332), but line 376 usesconsole.warndirectly. This may bypass any configured log formatting or transport.♻️ Proposed fix for consistency
if (genericRegex.test(String(output))) { - console.warn( + logger.warn( '[TabNaming] Agent returned a generic tab name candidate; consider adjusting prompt or model', + LOG_CONTEXT, { sessionId, detected: String(output).trim().slice(0, 80), } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 358 - 386, Replace the direct console.warn call inside the TabNaming logging try-block with the module's logger.warn to keep logging consistent; specifically, in the block that checks genericRegex.test(String(output)) (inside the function handling tab naming), call logger.warn with the same descriptive message and the same metadata object ({ sessionId, detected: String(output).trim().slice(0, 80) }) so formatting/transports are preserved—ensure logger is the same logger used elsewhere in this file.
404-416: The outer try-catch is redundant sincesafeDebugalready handles errors.The
safeDebughelper (lines 32-37) already wrapsconsole.debugin a try-catch, so the additional try-catch here provides no extra safety.♻️ Proposed simplification
// Spawn the process // When using SSH with stdin, pass the flag so ChildProcessSpawner // sends the prompt via stdin instead of command line args - try { - // Debug: log full finalArgs array and types just before spawn - // (kept in console.debug for diagnosis only) - safeDebug('[TabNaming] About to spawn with final args', { - sessionId, - command, - cwd, - sendPromptViaStdin: shouldSendPromptViaStdin, - finalArgsDetail: finalArgs.map((a) => ({ value: a, type: typeof a })), - }); - } catch (err) { - // ignore logging failures - } + // Debug: log full finalArgs array and types just before spawn + // (kept in console.debug for diagnosis only) + safeDebug('[TabNaming] About to spawn with final args', { + sessionId, + command, + cwd, + sendPromptViaStdin: shouldSendPromptViaStdin, + finalArgsDetail: finalArgs.map((a) => ({ value: a, type: typeof a })), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 404 - 416, The outer try-catch surrounding the diagnostic call is redundant because safeDebug already catches errors; remove the wrapping try { ... } catch (err) { } and directly call safeDebug(...) where the block currently is (the call that logs sessionId, command, cwd, shouldSendPromptViaStdin and finalArgsDetail), leaving safeDebug, finalArgs, sessionId, command and cwd references intact so the logging remains but without the extra try-catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 309-321: The current try-catch wraps both the
computation/filtering of nonStringItems and the call to safeDebug, which can
swallow errors from the filtering and leave finalArgs unfiltered; compute
nonStringItems and perform finalArgs = finalArgs.filter(...) outside any
try-catch so any unexpected errors propagate, then wrap only the safeDebug
logging call in try-catch to swallow logging errors; reference finalArgs and
safeDebug in your change and ensure the removal logic (nonStringItems.map and
finalArgs.filter) runs before any catch block.
---
Nitpick comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 358-386: Replace the direct console.warn call inside the TabNaming
logging try-block with the module's logger.warn to keep logging consistent;
specifically, in the block that checks genericRegex.test(String(output)) (inside
the function handling tab naming), call logger.warn with the same descriptive
message and the same metadata object ({ sessionId, detected:
String(output).trim().slice(0, 80) }) so formatting/transports are
preserved—ensure logger is the same logger used elsewhere in this file.
- Around line 404-416: The outer try-catch surrounding the diagnostic call is
redundant because safeDebug already catches errors; remove the wrapping try {
... } catch (err) { } and directly call safeDebug(...) where the block currently
is (the call that logs sessionId, command, cwd, shouldSendPromptViaStdin and
finalArgsDetail), leaving safeDebug, finalArgs, sessionId, command and cwd
references intact so the logging remains but without the extra try-catch.
Comment on lines
309
to
321
| // Final safety sanitization: ensure args are all plain strings | ||
| try { | ||
| const nonStringItems = finalArgs.filter((a) => typeof a !== 'string'); | ||
| if (nonStringItems.length > 0) { | ||
| safeDebug('[TabNaming] Removing non-string args before spawn', { | ||
| sessionId, | ||
| removed: nonStringItems.map((i) => ({ typeof: typeof i, preview: String(i) })), | ||
| }); | ||
| finalArgs = finalArgs.filter((a) => typeof a === 'string'); | ||
| } | ||
| } catch (err) { | ||
| // swallow safety log errors | ||
| } |
There was a problem hiding this comment.
The try-catch should only wrap logging, not the actual filtering operation.
If the filter at line 317 throws (unlikely but possible), the error is swallowed and finalArgs may retain non-string items, causing a less clear spawn failure later.
🛡️ Proposed fix to separate concerns
// Final safety sanitization: ensure args are all plain strings
- try {
- const nonStringItems = finalArgs.filter((a) => typeof a !== 'string');
- if (nonStringItems.length > 0) {
+ const nonStringItems = finalArgs.filter((a) => typeof a !== 'string');
+ if (nonStringItems.length > 0) {
+ try {
safeDebug('[TabNaming] Removing non-string args before spawn', {
sessionId,
removed: nonStringItems.map((i) => ({ typeof: typeof i, preview: String(i) })),
});
- finalArgs = finalArgs.filter((a) => typeof a === 'string');
+ } catch {
+ // swallow safety log errors
}
- } catch (err) {
- // swallow safety log errors
+ finalArgs = finalArgs.filter((a) => typeof a === 'string');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/ipc/handlers/tabNaming.ts` around lines 309 - 321, The current
try-catch wraps both the computation/filtering of nonStringItems and the call to
safeDebug, which can swallow errors from the filtering and leave finalArgs
unfiltered; compute nonStringItems and perform finalArgs = finalArgs.filter(...)
outside any try-catch so any unexpected errors propagate, then wrap only the
safeDebug logging call in try-catch to swallow logging errors; reference
finalArgs and safeDebug in your change and ensure the removal logic
(nonStringItems.map and finalArgs.filter) runs before any catch block.
…dundant try-catch - Compute nonStringItems and filter finalArgs outside any try-catch so unexpected filter errors propagate; only wrap the safeDebug log call - Replace console.warn with logger.warn (with LOG_CONTEXT) for consistent logging transports when a generic tab name candidate is detected - Remove redundant try-catch around safeDebug pre-spawn diagnostic call since safeDebug already handles its own errors internally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/ipc/handlers/tabNaming.ts (1)
172-185: Model arguments may be added twice before canonicalization.
buildAgentArgsat line 177 adds model args viamodelId: resolvedModelId, thenapplyAgentConfigOverridesat line 183 also receivessessionCustomModel: resolvedModelIdwhich adds model args again throughconfigOptions. While the canonicalization at lines 207-244 deduplicates these, this creates unnecessary work and could cause subtle bugs if canonicalization logic changes.Consider passing
modelId: undefinedtobuildAgentArgssinceapplyAgentConfigOverrideshandles model injection, or vice versa.♻️ Option A: Skip model in buildAgentArgs
let finalArgs = buildAgentArgs(agent, { baseArgs, prompt: fullPrompt, cwd: config.cwd, readOnlyMode: true, // Always read-only since we're not modifying anything - modelId: resolvedModelId, + modelId: undefined, // Model handled by applyAgentConfigOverrides });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/ipc/handlers/tabNaming.ts` around lines 172 - 185, The code is adding model args twice: buildAgentArgs(...) is called with modelId: resolvedModelId and then applyAgentConfigOverrides(..., { sessionCustomModel: resolvedModelId }) injects model args again; change the buildAgentArgs invocation in this block to omit the model (e.g., pass modelId: undefined or remove the modelId property) so that applyAgentConfigOverrides is the single source of model injection and then keep finalArgs = configResolution.args as-is (referencing buildAgentArgs, applyAgentConfigOverrides, resolvedModelId, finalArgs, and configResolution.args).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/ipc/handlers/tabNaming.ts`:
- Around line 172-185: The code is adding model args twice: buildAgentArgs(...)
is called with modelId: resolvedModelId and then applyAgentConfigOverrides(...,
{ sessionCustomModel: resolvedModelId }) injects model args again; change the
buildAgentArgs invocation in this block to omit the model (e.g., pass modelId:
undefined or remove the modelId property) so that applyAgentConfigOverrides is
the single source of model injection and then keep finalArgs =
configResolution.args as-is (referencing buildAgentArgs,
applyAgentConfigOverrides, resolvedModelId, finalArgs, and
configResolution.args).
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.
This pull request makes significant improvements to the tab naming logic for agent-based sessions, focusing on more robust and predictable model resolution, enhanced argument sanitization, and improved debugging and logging. The changes ensure that the correct model is used for tab naming, prevent invalid or duplicate CLI arguments, and provide better diagnostics for troubleshooting tab naming quality.
Model Resolution and Argument Handling:
--modelflags, converting them to a single--model=<value>form, and ensuring only valid model values are passed to the agent CLI. Also, the canonical model flag is injected just before process spawn.Debugging and Logging Enhancements:
Tab Name Extraction Improvements:
Summary by CodeRabbit
New Features
Bug Fixes
Improvements