fix(gateway): reject unknown session agents instead of provisioning them#111178
Conversation
openclaw-barnacle
Bot
added
gateway
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: fix(gateway): reject unknown session agents instead of provisioning them This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2121e66 to
ba8a473
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: 2121e66197
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // requested page when the same transcript was copied during a store migration. | ||
| limit: configured ? params.limit : 25, | ||
| ...(targetSessionKeys ? { sessionKeys: targetSessionKeys } : {}), | ||
| ...(target ? { storePath: target.storePath } : {}), |
There was a problem hiding this comment.
When an unconfigured agent exists only in a fixed legacy sessions.json, passing its storePath into searchSessionTranscripts resolves the sibling SQLite path and opens it through writable openOpenClawAgentDatabase. This provisions and registers the agent during sessions.search; worse, subsequent discovery sees the new empty SQLite database as authoritative and ignores the legacy file, so sessions.describe can no longer find the previously readable session. Use a read-only/missing-database search path instead of creating the derived store.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 19, 2026, 12:07 AM ET / 04:07 UTC. Summary PR surface: Source +284, Tests +884, Other +1. Total +1169 across 15 files. Reproducibility: yes. from source inspection: use a fixed legacy-only Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the reject-unknown boundary, make legacy-only transcript search use a genuinely non-provisioning reader or skip SQLite transcript search for that target, and defer persisted-target discovery until it is needed for an unconfigured or template-backed agent. Do we have a high-confidence way to reproduce the issue? Yes from source inspection: use a fixed legacy-only Is this the best way to solve the issue? No: rejecting truly unknown agents is the right policy, but the implementation must not reopen a writable derived SQLite store for legacy-only targets or scan configured fixed stores unnecessarily. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against cc57514e680d. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +284, Tests +884, Other +1. Total +1169 across 15 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
ba8a473 to
40a30a3
Compare
openclaw-barnacle
Bot
added
scripts
clawsweeper
Bot
added
rating: 🦪 silver shellfish
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40a30a3c62
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| existing: resolveExistingAgentSessionStoreTargetsSync(cfg, agentId), | ||
| fallback, |
There was a problem hiding this comment.
When session.store is a fixed path and the requested agent is configured, every loadSessionEntry-style lookup now calls resolveExistingAgentSessionStoreTargetsSync, which opens the SQLite database read-only and selects every persisted session key. The result is then discarded because the configured branch always prepends fallback and filters the identical existing path. This adds a second database open and an O(number of sessions) query to hot Gateway reads and mutations; defer existing-store discovery until the agent is unconfigured, or only perform it for template-backed stores where alternate paths can exist.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Merged via squash.
|
1 participant
What Problem This Solves
Calling
sessions.describeorsessions.abortwith a session key for an agent id that doesn't exist (agent:ghost:zzz) silently PROVISIONED that agent — creatingagents/ghost/agent/openclaw-agent.sqlite, anagent_databasesrow, and anagents.listentry that survives restart. A caller with operator scopes and arbitrary session keys could create unbounded agents/databases through a read/abort path. It was also inconsistent:agents.deleteon an unknown id correctly returns not-found. Found by the R7 QA campaign; the fix approach was chosen by the maintainer (reject unknown).Why This Change Was Made
Provisioning is a side effect of opening an agent's store while resolving a session key. The gate should block CREATION of a store that doesn't exist — without losing access to any store that DOES exist.
User Impact
sessions.describe/get/searchandsessions.abortnow resolve read-only: if no store exists for the agent, describe/search return not-found/empty and abort returnsagent "<id>" not found, and nothing is created (no directory, SQLite, registry row, oragents.listentry). Every legitimate way to reach an EXISTING store is preserved — a configured agent, a retired/removed-from-config agent whose store still exists (per-agent, deterministic-template, discovered, or legacysessions.json), an agent that has rows in a shared store, or an in-memory active run.mainandagents.createare unchanged.Evidence
lstat/realpath/root-containment validation (deterministic targets are validated the same way — a symlink escaping the agents root is rejected, not followed).[unknownKey, validKey]no longer mis-reports the valid session as missing.