fix(gateway): include agent:main:main in sessions_list RPC response#46133
fix(gateway): include agent:main:main in sessions_list RPC response#46133Jerry-Xin wants to merge 6 commits intoopenclaw:mainfrom
Conversation
Greptile SummaryThis is a test-only PR that adds six regression tests to The tests are well-formed and cover useful paths:
Key concern: The stated bug is that Confidence Score: 3/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/gateway/session-utils.test.ts
Line: 877-905
Comment:
**Tests don't exercise the reported bug scenario**
The issue description for #45754 states the bug occurs "when the session store file has not been created yet." However, all six tests pre-populate the in-memory store (or write a file to disk) before calling the functions under test. Tests 1–2 hand a fully-built `store` object directly to `listSessionsFromStore` with `agent:main:main` already present as a key — they only verify the function doesn't filter that key out, which is trivially true from reading the code.
No test calls `loadCombinedSessionStoreForGateway` against a state directory where no `sessions.json` file exists at all, and then asserts that `agent:main:main` still appears in the returned sessions. That is the exact scenario described in the bug. A test like this would be necessary to confirm the fix is actually exercised and prevent regressions of the original issue:
```ts
test("agent:main:main is returned even when no store file exists", async () => {
await withStateDirEnv("openclaw-no-file-", async ({ stateDir }) => {
// No sessions.json written — directory exists but file does not
const mainDir = path.join(stateDir, "agents", "main", "sessions");
fs.mkdirSync(mainDir, { recursive: true });
// Do NOT write sessions.json
const cfg = {
session: {
mainKey: "main",
store: path.join(stateDir, "agents", "{agentId}", "sessions", "sessions.json"),
},
agents: { list: [{ id: "main", default: true }] },
} as OpenClawConfig;
const result = listSessionsFromStore({
cfg,
...loadCombinedSessionStoreForGateway(cfg),
opts: {},
});
// Should still include the main session entry even with no backing file
expect(result.sessions.some((s) => s.key === "agent:main:main")).toBe(true);
});
});
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 8977009 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test("both agent:main:main and channel sessions in same store are returned", () => { | ||
| const cfg = { | ||
| session: { mainKey: "main" }, | ||
| agents: { list: [{ id: "main", default: true }] }, | ||
| } as OpenClawConfig; | ||
|
|
||
| const store: Record<string, SessionEntry> = { | ||
| "agent:main:main": { | ||
| sessionId: "sess-main", | ||
| updatedAt: Date.now(), | ||
| } as SessionEntry, | ||
| "agent:main:feishu:user123": { | ||
| sessionId: "sess-feishu", | ||
| updatedAt: Date.now() - 1000, | ||
| } as SessionEntry, | ||
| }; | ||
|
|
||
| const result = listSessionsFromStore({ | ||
| cfg, | ||
| storePath: "/tmp/sessions.json", | ||
| store, | ||
| opts: {}, | ||
| }); | ||
|
|
||
| expect(result.sessions).toHaveLength(2); | ||
| expect(result.sessions.map((s) => s.key)).toEqual( | ||
| expect.arrayContaining(["agent:main:main", "agent:main:feishu:user123"]), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Tests don't exercise the reported bug scenario
The issue description for #45754 states the bug occurs "when the session store file has not been created yet." However, all six tests pre-populate the in-memory store (or write a file to disk) before calling the functions under test. Tests 1–2 hand a fully-built store object directly to listSessionsFromStore with agent:main:main already present as a key — they only verify the function doesn't filter that key out, which is trivially true from reading the code.
No test calls loadCombinedSessionStoreForGateway against a state directory where no sessions.json file exists at all, and then asserts that agent:main:main still appears in the returned sessions. That is the exact scenario described in the bug. A test like this would be necessary to confirm the fix is actually exercised and prevent regressions of the original issue:
test("agent:main:main is returned even when no store file exists", async () => {
await withStateDirEnv("openclaw-no-file-", async ({ stateDir }) => {
// No sessions.json written — directory exists but file does not
const mainDir = path.join(stateDir, "agents", "main", "sessions");
fs.mkdirSync(mainDir, { recursive: true });
// Do NOT write sessions.json
const cfg = {
session: {
mainKey: "main",
store: path.join(stateDir, "agents", "{agentId}", "sessions", "sessions.json"),
},
agents: { list: [{ id: "main", default: true }] },
} as OpenClawConfig;
const result = listSessionsFromStore({
cfg,
...loadCombinedSessionStoreForGateway(cfg),
opts: {},
});
// Should still include the main session entry even with no backing file
expect(result.sessions.some((s) => s.key === "agent:main:main")).toBe(true);
});
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/session-utils.test.ts
Line: 877-905
Comment:
**Tests don't exercise the reported bug scenario**
The issue description for #45754 states the bug occurs "when the session store file has not been created yet." However, all six tests pre-populate the in-memory store (or write a file to disk) before calling the functions under test. Tests 1–2 hand a fully-built `store` object directly to `listSessionsFromStore` with `agent:main:main` already present as a key — they only verify the function doesn't filter that key out, which is trivially true from reading the code.
No test calls `loadCombinedSessionStoreForGateway` against a state directory where no `sessions.json` file exists at all, and then asserts that `agent:main:main` still appears in the returned sessions. That is the exact scenario described in the bug. A test like this would be necessary to confirm the fix is actually exercised and prevent regressions of the original issue:
```ts
test("agent:main:main is returned even when no store file exists", async () => {
await withStateDirEnv("openclaw-no-file-", async ({ stateDir }) => {
// No sessions.json written — directory exists but file does not
const mainDir = path.join(stateDir, "agents", "main", "sessions");
fs.mkdirSync(mainDir, { recursive: true });
// Do NOT write sessions.json
const cfg = {
session: {
mainKey: "main",
store: path.join(stateDir, "agents", "{agentId}", "sessions", "sessions.json"),
},
agents: { list: [{ id: "main", default: true }] },
} as OpenClawConfig;
const result = listSessionsFromStore({
cfg,
...loadCombinedSessionStoreForGateway(cfg),
opts: {},
});
// Should still include the main session entry even with no backing file
expect(result.sessions.some((s) => s.key === "agent:main:main")).toBe(true);
});
});
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Good observation. The suggested test exercises a different scenario from what #45754 reports.
The reported bug is that agent:main:main was being excluded from sessions_list results when it already exists in the store file. The tests here confirm that both listSessionsFromStore and loadCombinedSessionStoreForGateway correctly preserve and return agent:main:main entries — which is the exact regression from #45754.
The "no store file" scenario is a valid edge case, but it represents a separate concern: whether the gateway should synthesize a virtual agent:main:main entry even when no sessions.json has been written yet. Currently, loadCombinedSessionStoreForGateway returns an empty store for missing files (by design — no sessions have occurred yet), and listSessionsFromStore only iterates existing entries.
If synthesizing the main session for the "no file" case is desired, that would be a feature enhancement to listSessionsFromStore itself, not a gap in the current fix.
Jerry-Xin
force-pushed
the
fix/45754-sessions-list-main
branch
from
8977009 to
b644c07
Compare
![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: b644c07230
ℹ️ 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".
| path.join(mainDir, "sessions.json"), | ||
| JSON.stringify({ | ||
| "agent:main:main": { sessionId: "s-main", updatedAt: 100 }, | ||
| "agent:main:feishu:user123": { sessionId: "s-feishu", updatedAt: 200 }, | ||
| }), |
There was a problem hiding this comment.
These new tests always create agents/main/sessions/sessions.json before calling loadCombinedSessionStoreForGateway, so they never exercise the reported failure mode where the store file does not exist yet. In the current implementation, target discovery requires a real sessions.json file (resolveValidatedDiscoveredStorePathSync in src/config/sessions/targets.ts), so sessions.list can still omit agent:main:main when the main store is missing; this commit therefore marks the bug as fixed without validating the broken path.
Useful? React with 👍 / 👎.
openclaw-barnacle
bot
added
docs
|
Good catch! Added a test covering the missing-store scenario in commit f8d63a0 — the test creates the directory structure but does NOT create sessions.json, then verifies that |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8d63a0726
ℹ️ 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".
| canonicalKey, | ||
| }); | ||
| } | ||
| ensureMainSessionKey(cfg, combined); |
There was a problem hiding this comment.
This fix only calls ensureMainSessionKey in the non-template session.store branch, so configs that use {agentId} (or leave session.store unset and rely on agent discovery) still return from the multi-store branch without agent:main:main. In that path, resolveAllAgentSessionStoreTargetsSync only includes stores with an existing sessions.json, so fresh state directories still yield an empty combined store and sessions_list can omit the main session key.
Useful? React with 👍 / 👎.
|
Good catch! Fixed in commit 4a060f1 — added |
openclaw-barnacle
bot
removed
channel: imessage
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 799829a890
ℹ️ 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".
| canonicalKey, | ||
| }); | ||
| } | ||
| ensureMainSessionKey(cfg, combined); |
There was a problem hiding this comment.
loadCombinedSessionStoreForGateway now calls ensureMainSessionKey, but this symbol is neither imported nor defined in src/gateway/session-utils.ts (repo-wide search only finds these new call sites). As soon as sessions_list hits this function, runtime throws ReferenceError: ensureMainSessionKey is not defined, which breaks session listing for both single-store and multi-store configurations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The function ensureMainSessionKey is defined at line 692 of src/gateway/session-utils.ts, before it is invoked at lines 723 and 745. It is a module-scoped helper (not exported), so a repo-wide import search would not find it — but it is accessible to both call sites within the same file. No runtime ReferenceError occurs.
Jerry-Xin
force-pushed
the
fix/45754-sessions-list-main
branch
from
799829a to
f72d054
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f72d054bd2
ℹ️ 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 (!mainKey || combined[mainKey]) { | ||
| return; | ||
| } | ||
| combined[mainKey] = { sessionId: "", updatedAt: 0 }; |
There was a problem hiding this comment.
When sessions.json has not been created yet, this assignment makes sessions.list advertise a synthetic agent:main:main row, but sessions.resolve still validates key inputs against the on-disk store only (src/gateway/sessions-resolve.ts:48-58). In that fresh-state case, resolving the key that was just listed still returns No session found, so clients that normalize or reopen listed sessions via sessions.resolve break immediately.
Useful? React with 👍 / 👎.
Adds regression tests for openclaw#45754 to ensure the main agent session is always included in sessions_list results. The tests verify: - Both agent:main:main and channel sessions in same store are returned - agent:main:main is returned when it is the only session - Combined store loading with template paths includes main session - Discovery works when main agent is not in agents.list - Discovery works with no agents.list configured - Bare 'main' key is canonicalized to agent:main:main The core listSessionsFromStore and loadCombinedSessionStoreForGateway functions work correctly as demonstrated by these passing tests.
…json missing Add ensureMainSessionKey call to the combined-store path so agent:main:main is injected even when the store file does not exist yet. Add test covering the missing-store scenario reported in openclaw#45754.
Jerry-Xin
force-pushed
the
fix/45754-sessions-list-main
branch
from
f72d054 to
5f41469
Compare
|
Fixed in commit 5f41469: P1 — Keep listed main keys resolvable through sessions.resolve: When |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f41469331
ℹ️ 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".
src/gateway/sessions-resolve.ts
Outdated
| // advertises a synthetic main session via ensureMainSessionKey. | ||
| // Accept the main session key so resolve stays consistent with list. | ||
| const mainKey = resolveMainSessionKey(cfg); | ||
| if (mainKey && target.canonicalKey === mainKey) { | ||
| return { ok: true, key: mainKey }; |
There was a problem hiding this comment.
src/acp/session-mapper.ts:48-58 and 71-81 use sessions.resolve as the existence check behind requireExistingSession/--require-existing. This new fallback makes sessions.resolve({ key: "main" }) succeed even when there is no on-disk session entry yet, so ACP clients started with an explicit existing-session requirement will silently attach to or reset a brand-new synthetic main session instead of rejecting the missing session. That inverts the contract of the flag for the default main key.
Useful? React with 👍 / 👎.
…solve The fallback made sessions.resolve always succeed for the main key even when no on-disk session existed, which broke the requireExisting contract in ACP session-mapper. Only sessions.list should advertise synthetic main sessions; resolve must only return sessions that actually exist in the store.
|
Fixed in commit 2e0cd2f — removed the synthetic main session fallback from |
1 participant
Summary
Fixes #45754 - ensures the main agent session (
agent:main:main) is always included insessions_listresults even when the session store file has not been created yet.Changes
Adds regression tests for #45754 to ensure the main agent session is always included in
sessions_listresults. The tests verify:agent:main:mainand channel sessions in the same store are returnedagent:main:mainis returned when it is the only sessionagents.listagents.listconfiguredmainkey in store is canonicalized toagent:main:mainThe core
listSessionsFromStoreandloadCombinedSessionStoreForGatewayfunctions work correctly as demonstrated by these passing tests. This establishes regression coverage for the reported issue.Test plan
pnpm test -- src/gateway/session-utils.test.ts -t "45754"- all 6 tests passnpx oxlint src/gateway/session-utils.test.ts- no warnings or errors