Fix #63992: resolveSessionKey hardcodes DEFAULT_AGENT_ID instead of using configured default agent#64108
Fix #63992: resolveSessionKey hardcodes DEFAULT_AGENT_ID instead of using configured default agent#64108mushuiyu886 wants to merge 4 commits into
Conversation
…ULT_AGENT_ID
resolveSessionKey() was hardcoding DEFAULT_AGENT_ID ("main") when building
session keys for non-group chats, ignoring any configured default agent.
Now passes the OpenClawConfig to resolveDefaultAgentId() so that user-defined
default agents are respected when resolving session keys.
Fixed call sites:
- src/config/sessions/session-key.ts: added cfg parameter and use resolveDefaultAgentId
- src/agents/command/session.ts: pass opts.cfg to resolveSessionKey
- extensions/slack/src/monitor/context.ts: pass params.cfg to resolveSessionKey
Fixes openclaw#63992
openclaw-barnacle
Bot
added
channel: slack
![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: 9e3963e8a9
ℹ️ 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".
| const ctx: MsgContext | undefined = opts.to?.trim() ? { From: opts.to } : undefined; | ||
| let sessionKey: string | undefined = | ||
| explicitSessionKey ?? (ctx ? resolveSessionKey(scope, ctx, mainKey) : undefined); | ||
| explicitSessionKey ?? (ctx ? resolveSessionKey(scope, ctx, mainKey, opts.cfg) : undefined); |
There was a problem hiding this comment.
When no explicit session key is provided, this new call can now produce agent:<default-agent>:..., but storePath/sessionStore were already resolved earlier from explicitSessionKey and still default to the main store. In configs where the default agent is not main, that writes a non-main agent key into the main store, while other code paths that resolve store location from the key’s agent id (for example status/session resolution flows) will read from the non-main agent store and miss the session, splitting history across stores.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a regression where Confidence Score: 5/5Safe to merge; the fix is correct and backward-compatible, with only P2 findings. The core change is logically sound: adding the optional cfg parameter and delegating to resolveDefaultAgentId correctly fixes the regression for both the direct-chat and group key paths. Backward compatibility is preserved for all callers that omit cfg. The two identified findings are P2 — a cross-layer import concern and a missing migration path for existing group session keys — neither of which blocks merge or causes immediate data corruption. src/config/sessions/session-key.ts — cross-layer import and missing group session migration path Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/config/sessions/session-key.ts
Line: 10
Comment:
**Cross-layer import adds weight to a lightweight utility**
`session-key.ts` is a low-level `config/sessions/` utility; importing `resolveDefaultAgentId` from `agents/agent-scope.ts` pulls in `createSubsystemLogger`, `resolveAgentModelFallbackValues`, `resolveStateDir`, and the rest of the `agents/` module graph into any code path (and test suite) that imports `session-key.ts`.
`main-session.ts` already solves the same problem without the extra dependency:
```ts
const agents = cfg?.agents?.list ?? [];
const defaultAgentId =
agents.find((agent) => agent?.default)?.id ?? agents[0]?.id ?? DEFAULT_AGENT_ID;
```
Consider inlining this short computation (and replicating the multi-default warning if desired) to avoid widening the import surface of an otherwise lightweight module.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/config/sessions/session-key.ts
Line: 49
Comment:
**No migration path for existing group session keys**
The fix correctly replaces `DEFAULT_AGENT_ID` in both the direct-chat branch (line 47) and the group fallback branch (line 49). For users who previously configured a non-`"main"` default agent, group sessions were stored under `agent:main:<channel>:group:…`; after this change, new messages to the same groups look for `agent:<custom>:<channel>:group:…`, orphaning the older entries.
The direct-chat path has `canonicalizeMainSessionAlias` to cover this legacy shape (see `main-session.ts` lines 62-67). Group session keys have no equivalent migration seam, so any existing group conversation context stored under the old `agent:main:` prefix will be silently dropped for affected users.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(sessions): use configured default ag..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| import { normalizeE164 } from "../../utils.js"; | ||
| import { normalizeExplicitSessionKey } from "./explicit-session-key-normalization.js"; | ||
| import { resolveGroupSessionKey } from "./group.js"; | ||
| import { resolveDefaultAgentId } from "../../agents/agent-scope.js"; |
There was a problem hiding this comment.
session-key.ts is a low-level config/sessions/ utility; importing resolveDefaultAgentId from agents/agent-scope.ts pulls in createSubsystemLogger, resolveAgentModelFallbackValues, resolveStateDir, and the rest of the agents/ module graph into any code path (and test suite) that imports session-key.ts.
main-session.ts already solves the same problem without the extra dependency:
const agents = cfg?.agents?.list ?? [];
const defaultAgentId =
agents.find((agent) => agent?.default)?.id ?? agents[0]?.id ?? DEFAULT_AGENT_ID;Consider inlining this short computation (and replicating the multi-default warning if desired) to avoid widening the import surface of an otherwise lightweight module.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/session-key.ts
Line: 10
Comment:
**Cross-layer import adds weight to a lightweight utility**
`session-key.ts` is a low-level `config/sessions/` utility; importing `resolveDefaultAgentId` from `agents/agent-scope.ts` pulls in `createSubsystemLogger`, `resolveAgentModelFallbackValues`, `resolveStateDir`, and the rest of the `agents/` module graph into any code path (and test suite) that imports `session-key.ts`.
`main-session.ts` already solves the same problem without the extra dependency:
```ts
const agents = cfg?.agents?.list ?? [];
const defaultAgentId =
agents.find((agent) => agent?.default)?.id ?? agents[0]?.id ?? DEFAULT_AGENT_ID;
```
Consider inlining this short computation (and replicating the multi-default warning if desired) to avoid widening the import surface of an otherwise lightweight module.
How can I resolve this? If you propose a fix, please make it concise.| return canonical; | ||
| } | ||
| return `agent:${DEFAULT_AGENT_ID}:${raw}`; | ||
| return `agent:${defaultAgentId}:${raw}`; |
There was a problem hiding this comment.
The fix correctly replaces DEFAULT_AGENT_ID in both the direct-chat branch (line 47) and the group fallback branch (line 49). For users who previously configured a non-"main" default agent, group sessions were stored under agent:main:<channel>:group:…; after this change, new messages to the same groups look for agent:<custom>:<channel>:group:…, orphaning the older entries.
The direct-chat path has canonicalizeMainSessionAlias to cover this legacy shape (see main-session.ts lines 62-67). Group session keys have no equivalent migration seam, so any existing group conversation context stored under the old agent:main: prefix will be silently dropped for affected users.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/config/sessions/session-key.ts
Line: 49
Comment:
**No migration path for existing group session keys**
The fix correctly replaces `DEFAULT_AGENT_ID` in both the direct-chat branch (line 47) and the group fallback branch (line 49). For users who previously configured a non-`"main"` default agent, group sessions were stored under `agent:main:<channel>:group:…`; after this change, new messages to the same groups look for `agent:<custom>:<channel>:group:…`, orphaning the older entries.
The direct-chat path has `canonicalizeMainSessionAlias` to cover this legacy shape (see `main-session.ts` lines 62-67). Group session keys have no equivalent migration seam, so any existing group conversation context stored under the old `agent:main:` prefix will be silently dropped for affected users.
How can I resolve this? If you propose a fix, please make it concise.
This comment was marked as spam.
This comment was marked as spam.
P1: Reorder store loading after session key derivation in
resolveSessionKeyForRequest() so non-main default agents load
the correct store.
P2: Inline resolveDefaultAgentId computation to remove cross-layer
import from agent-scope.ts, following the pattern in main-session.ts.
P2: Add canonicalizeGroupSessionKey() for migration from old group
key format (provider:group:id) to new agent-scoped format
(agent:{id}:provider:group:id).
Review feedback addressedUpdated this PR to address all three review comments: P1: Store loading order (session.ts)
P2: Cross-layer import (session-key.ts)
P2: Group session key migration (session-key.ts)
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 145ea44214
ℹ️ 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".
| explicitSessionKey ?? (ctx ? resolveSessionKey(scope, ctx, mainKey, opts.cfg) : undefined); | ||
|
|
||
| // Now load the store from the derived key's agent ID. | ||
| const storeAgentId = resolveAgentIdFromSessionKey(sessionKey ?? explicitSessionKey); |
There was a problem hiding this comment.
Deriving storeAgentId from the newly resolved key means --to requests now start from the configured default agent store, and collectSessionIdMatchesForRequest only scans listAgentIds(cfg) beyond that primary store. In configs that set a non-main default and only list that agent (for example agents.list=[{id:"ops",default:true}]), the legacy main store is no longer searched, so resuming with sessionId can miss an existing session and silently fork history into a new agent store entry.
Useful? React with 👍 / 👎.
When the configured default agent is not "main", the store scan in collectSessionIdMatchesForRequest would skip the legacy "main" store, causing sessionId resume to miss existing sessions and silently fork history. Now always scans the "main" agent store as a fallback.
Fix for P2: legacy main-store lookupAddressed the latest review feedback on Problem: After the P1 fix changed Fix: Commit: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14693c8e8b
ℹ️ 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".
| let sessionKey: string | undefined = | ||
| explicitSessionKey ?? (ctx ? resolveSessionKey(scope, ctx, mainKey) : undefined); | ||
| explicitSessionKey ?? (ctx ? resolveSessionKey(scope, ctx, mainKey, opts.cfg) : undefined); | ||
|
|
||
| // Now load the store from the derived key's agent ID. | ||
| const storeAgentId = resolveAgentIdFromSessionKey(sessionKey ?? explicitSessionKey); | ||
| let storePath = resolveStorePath(sessionCfg?.store, { | ||
| agentId: storeAgentId, |
There was a problem hiding this comment.
resolveSessionKeyForRequest now derives the key with resolveSessionKey(..., opts.cfg), so a non-main default agent produces agent:<default>:main and immediately scopes the store load to that agent. On plain --to resumes (no sessionId), there is no fallback to the legacy agent:main:main entry/store, so existing conversations created before this change are no longer resumed and a new history is silently forked. Please keep a legacy main-key lookup/alias path for this branch, not only the sessionId search path.
Useful? React with 👍 / 👎.
3 participants
Summary
Fixes #63992
Problem
resolveSessionKey()was hardcodingDEFAULT_AGENT_ID("main") when building session keys for non-group chats, ignoring any configured default agent. This was a regression from PR #30654.Changes
Root fix (
src/config/sessions/session-key.ts)OpenClawConfigimportresolveDefaultAgentIdimport fromagent-scope.tscfg?: OpenClawConfigparameter toresolveSessionKeyDEFAULT_AGENT_IDwithresolveDefaultAgentId(cfg ?? {})Call site fixes
src/agents/command/session.ts: passopts.cfgtoresolveSessionKeyextensions/slack/src/monitor/context.ts: passparams.cfgtoresolveSessionKeyNotes
Some call sites (whatsapp, auto-reply/reply/session.ts) already use
canonicalizeMainSessionAliasas a workaround and are unaffected by this change. The fix is backward compatible - call sites that don't passcfgwill continue to useDEFAULT_AGENT_ID="main".Testing
Existing tests pass. The fix allows custom default agents (e.g.,
default: trueon a non-main agent) to be properly respected when resolving session keys.