Fix ACP session model reporting in sessions list#54647
Conversation
Greptile SummaryThis PR fixes ACP session model reporting in the sessions list by reading the runtime model from Key changes:
Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/session-utils.ts
Line: 1180-1184
Comment:
**Duplicate `isAcpSessionKey` call**
`isAcpSessionKey(key)` is called twice with the same argument on lines 1180 and 1184. Since the result is pure (string ops only — `trim`, `toLowerCase`, `startsWith`), it's cheap, but caching it in a local variable would make the intent clearer and avoid any future drift if the key value were somehow mutable.
```suggestion
const isAcp = isAcpSessionKey(key);
const resolvedModel = isAcp
? resolveExplicitSessionModelIdentityRef(cfg, entry, subagentRun?.model)
: resolveSessionModelIdentityRef(cfg, entry, sessionAgentId, subagentRun?.model);
const modelProvider = resolvedModel?.provider;
const model = resolvedModel?.model ?? (isAcp ? undefined : DEFAULT_MODEL);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Fix ACP session model reporting in sessi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const resolvedModel = isAcpSessionKey(key) | ||
| ? resolveExplicitSessionModelIdentityRef(cfg, entry, subagentRun?.model) | ||
| : resolveSessionModelIdentityRef(cfg, entry, sessionAgentId, subagentRun?.model); | ||
| const modelProvider = resolvedModel?.provider; | ||
| const model = resolvedModel?.model ?? (isAcpSessionKey(key) ? undefined : DEFAULT_MODEL); |
There was a problem hiding this comment.
isAcpSessionKey call
isAcpSessionKey(key) is called twice with the same argument on lines 1180 and 1184. Since the result is pure (string ops only — trim, toLowerCase, startsWith), it's cheap, but caching it in a local variable would make the intent clearer and avoid any future drift if the key value were somehow mutable.
| const resolvedModel = isAcpSessionKey(key) | |
| ? resolveExplicitSessionModelIdentityRef(cfg, entry, subagentRun?.model) | |
| : resolveSessionModelIdentityRef(cfg, entry, sessionAgentId, subagentRun?.model); | |
| const modelProvider = resolvedModel?.provider; | |
| const model = resolvedModel?.model ?? (isAcpSessionKey(key) ? undefined : DEFAULT_MODEL); | |
| const isAcp = isAcpSessionKey(key); | |
| const resolvedModel = isAcp | |
| ? resolveExplicitSessionModelIdentityRef(cfg, entry, subagentRun?.model) | |
| : resolveSessionModelIdentityRef(cfg, entry, sessionAgentId, subagentRun?.model); | |
| const modelProvider = resolvedModel?.provider; | |
| const model = resolvedModel?.model ?? (isAcp ? undefined : DEFAULT_MODEL); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/session-utils.ts
Line: 1180-1184
Comment:
**Duplicate `isAcpSessionKey` call**
`isAcpSessionKey(key)` is called twice with the same argument on lines 1180 and 1184. Since the result is pure (string ops only — `trim`, `toLowerCase`, `startsWith`), it's cheap, but caching it in a local variable would make the intent clearer and avoid any future drift if the key value were somehow mutable.
```suggestion
const isAcp = isAcpSessionKey(key);
const resolvedModel = isAcp
? resolveExplicitSessionModelIdentityRef(cfg, entry, subagentRun?.model)
: resolveSessionModelIdentityRef(cfg, entry, sessionAgentId, subagentRun?.model);
const modelProvider = resolvedModel?.provider;
const model = resolvedModel?.model ?? (isAcp ? undefined : DEFAULT_MODEL);
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70f78c9afb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!trimmedProvider) { | ||
| return resolveSessionModelIdentityLiteral(cfg, trimmedModel); | ||
| } | ||
| const parsed = parseModelRef(trimmedModel, trimmedProvider); |
There was a problem hiding this comment.
When modelProvider is already stored (for example openrouter) and model contains a vendor prefix (anthropic/...), re-parsing model here overwrites the explicit provider with the prefix provider. Since buildGatewaySessionRow now goes through this helper, wrapper-backed sessions can be reported as direct-provider sessions, which corrupts modelProvider in the sessions list and can skew provider-specific cost/context resolution. Treat the explicit provider as authoritative instead of re-parsing prefixed model strings in this branch.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. Source inspection gives a high-confidence current-main path: a gateway Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Rework gateway Do we have a high-confidence way to reproduce the issue? Yes. Source inspection gives a high-confidence current-main path: a gateway Is this the best way to solve the issue? No. The PR targets the right bug, but it re-parses prefixed model strings even when an explicit provider is stored and needs rework against the current ACP sentinel/runtime metadata behavior. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against b180b8ae4833. |
|
Codex review: needs changes before merge. What this changes: The PR updates gateway session model resolution and tests so ACP session rows read Required change before merge: This is a narrow, valid gateway bug with a concrete blocking defect in the source PR and clear replacement/rework boundaries. Review findings:
Review detailsBest possible solution: Update the current gateway session-row resolver so ACP rows use explicit ACP runtime metadata when present, leave Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against e46dccb35374. |
1 participant
Summary
Closes #54640
Testing