feat: route agent sessions by conversation#1089
Conversation
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 3m 12s —— View job Code Review
This PR routes agent sessions per-conversation by threading an ArchitectureThe two-level state file design is the right call:
Issues1.
2. Redundant In agentSessionId: agentSessionId ?? common.conversationId,
3. Per-session state files are never purged
4.
const agentSessionStrategy =
options?.agentSessionStrategy ??
(options?.origin === 'newtab' ? 'main' : 'conversation')When Minor Observations
Test CoverageTest additions are extensive: SummaryThe core session-routing logic is correct and the test suite is thorough. The most actionable issue before merging is #1 (auditing whether |
Greptile SummaryThis PR replaces the hardcoded
Confidence Score: 5/5Safe to merge. The session-routing changes are well-encapsulated, backward-compatible routes are in place, and the test suite covers the new session-scoped paths, history loading, runtime state, and queue behaviour. The implementation is thorough: runtime validation guards all session-ID API inputs, the persisted state uses two pointer files (session-scoped and agent-scoped) for correct The queue drain logic in Important Files Changed
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/server/src/api/services/agents/agent-harness-service.ts:413-430
**Cross-session queue drain head-of-line blocking**
`maybeStartNextFromQueue` always pops the globally oldest message regardless of its session. If that message belongs to session A (which currently has an active turn), it is pushed back to the front and the drain returns — leaving session B's queued message stranded even though session B has no active turn. Session B's message will only be drained after session A's current turn finishes.
In the multi-tab case (tab 1 on agent/session-A, tab 2 on agent/session-B), a user on tab 2 who types a message while agent/session-A has an outstanding queued message in front of it in the FIFO will see no response until session A drains. The message is not lost, but the delay is invisible to the user.
The fix would require either a per-session sub-queue or a scan-for-next-startable-session strategy in `maybeStartNextFromQueue`.
### Issue 2 of 2
packages/browseros-agent/apps/server/src/lib/agents/agent-types.ts:10-11
**`AgentSessionId` is an unbranded string alias**
`type AgentSessionId = string` is structurally identical to `string`, so TypeScript will accept any `string` wherever an `AgentSessionId` is expected and vice versa. Runtime validation via `isAgentSessionId` catches bad values at the API boundary, but internal call sites get no compile-time protection — a raw `string` variable could silently flow into a session-scoped registry key or file-system path without the type system objecting. A nominal/branded type (`type AgentSessionId = string & { readonly __brand: 'AgentSessionId' }`) would surface these mistakes at compile time.
Reviews (3): Last reviewed commit: "refactor: rework agent-session-routing b..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| input: { | ||
| sessionId?: AgentSessionId | ||
| message: string | ||
| attachments?: ReadonlyArray<{ mediaType: string; data: string }> | ||
| cwd?: string | ||
| }, |
There was a problem hiding this comment.
sessionId field in runDetachedTurn input type
input.sessionId is declared in the input type signature but is never read inside the function body — the implementation immediately shadows it with const sessionId = turn.sessionId. Since the turn was already registered with the correct session ID in startTurn, deriving it from the registry is correct, but the sessionId?: AgentSessionId field in the parameter type becomes dead code that adds noise and could mislead future callers into thinking it influences behaviour.
Rule Used: Remove unused/dead code rather than leaving it in ... (source)
Learned From
browseros-ai/BrowserOS-agent#126
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/api/services/agents/agent-harness-service.ts
Line: 671-676
Comment:
**Unused `sessionId` field in `runDetachedTurn` input type**
`input.sessionId` is declared in the input type signature but is never read inside the function body — the implementation immediately shadows it with `const sessionId = turn.sessionId`. Since the turn was already registered with the correct session ID in `startTurn`, deriving it from the registry is correct, but the `sessionId?: AgentSessionId` field in the parameter type becomes dead code that adds noise and could mislead future callers into thinking it influences behaviour.
**Rule Used:** Remove unused/dead code rather than leaving it in ... ([source](https://app.greptile.com/browseros-org-2/-/custom-context?memory=9b045db4-2630-428c-95b7-ccf048d34547))
**Learned From**
[browseros-ai/BrowserOS-agent#126](https://github.com/browseros-ai/BrowserOS-agent/pull/126)
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!
| private async loadLatestAgentSessionRecord( | ||
| agent: AgentPromptInput['agent'], | ||
| ): Promise<AcpSessionRecord | null> { | ||
| const paths = resolveAgentRuntimePaths({ | ||
| browserosDir: this.browserosDir, | ||
| agentId: agent.id, | ||
| }) | ||
| const latestForAgent = await loadLatestRuntimeState(paths.runtimeStatePath) | ||
| if (latestForAgent) { | ||
| const latestRecord = await this.sessionStore.load( | ||
| latestForAgent.runtimeSessionKey, | ||
| ) | ||
| if (latestRecord) return latestRecord | ||
| } | ||
| return this.loadSessionRecord(agent, MAIN_AGENT_SESSION_ID) | ||
| } |
There was a problem hiding this comment.
runtimeStatePath reads in loadLatestAgentSessionRecord
When latestForAgent is found in runtimeStatePath but the session store doesn't have the corresponding runtimeSessionKey record (e.g. the store was cleaned up while the pointer file remained), execution falls through to loadSessionRecord(agent, MAIN_AGENT_SESSION_ID). That function also reads paths.runtimeStatePath for the main session fallback path, causing the same file to be read twice. Consider passing the already-loaded latestForAgent value into loadSessionRecord (or inlining the final fallback) to avoid the duplicate I/O.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/lib/agents/acpx-runtime.ts
Line: 222-237
Comment:
**Redundant `runtimeStatePath` reads in `loadLatestAgentSessionRecord`**
When `latestForAgent` is found in `runtimeStatePath` but the session store doesn't have the corresponding `runtimeSessionKey` record (e.g. the store was cleaned up while the pointer file remained), execution falls through to `loadSessionRecord(agent, MAIN_AGENT_SESSION_ID)`. That function also reads `paths.runtimeStatePath` for the `main` session fallback path, causing the same file to be read twice. Consider passing the already-loaded `latestForAgent` value into `loadSessionRecord` (or inlining the final fallback) to avoid the duplicate I/O.
How can I resolve this? If you propose a fix, please make it concise.
Greptile SummaryThis PR introduces first-class, conversation-scoped agent sessions by threading an
Confidence Score: 4/5Safe to merge; the session-routing logic is well-covered by tests and the backward-compatibility fallback chain for existing main-session history is correctly gated. The implementation is coherent end-to-end and the test suite is thorough. The two comments are minor: The concurrency test in Important Files Changed
Sequence DiagramsequenceDiagram
participant SP as Sidepanel (useChatSession)
participant Req as useChatSessionRequest
participant Route as /agents/:id/sidepanel/chat
participant Svc as AgentHarnessService
participant Reg as TurnRegistry
participant RT as AcpxRuntime
SP->>SP: "agentSessionStrategy = 'conversation' (sidepanel) or 'main' (newtab)"
SP->>SP: "agentSessionId = strategy==='main' ? 'main' : conversationId"
SP->>Req: "buildSidepanelPreparedSendMessagesRequest({ agentSessionId, ... })"
Req->>Route: "POST body { conversationId, agentSessionId, message }"
Route->>Route: parseSidepanelAgentChatBody() validates isAgentSessionId(agentSessionId)
Route->>Svc: "startTurn({ agentId, sessionId: agentSessionId, message })"
Svc->>Reg: register(agentId, sessionId)
Reg-->>Svc: "ActiveTurn { turnId, sessionId }"
Route->>Route: "response header X-Session-Id = agentSessionId"
Svc->>RT: "send({ agent, sessionId, ... })"
RT->>RT: "resolveAgentRuntimePaths({ agentId, sessionId })"
RT->>RT: write runtimeSessionStatePath (session-specific)
RT->>RT: write runtimeStatePath (agent-level pointer)
Note over Svc,Reg: On turn complete
Svc->>Reg: hasActiveForAgent(agentId)?
alt other sessions still running
Svc->>Svc: activity.set(agentId, 'working')
else no active sessions
Svc->>Svc: activity.delete(agentId)
end
Note over Route,RT: History lookup path
Route->>Svc: getHistory(agentId, sessionId)
Svc->>RT: loadSessionRecord(agent, sessionId)
RT->>RT: try runtimeSessionStatePath then sessionStore
RT->>RT: if main: fallback runtimeStatePath then sessionStore
RT->>RT: if main: fallback agent.sessionKey
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/browseros-agent/apps/server/src/lib/agents/agent-types.ts:11-12
The `AgentSessionId` type alias is identical to `string`, so any arbitrary string satisfies it at compile time. The runtime guard `isAgentSessionId` is only applied at the API boundary in `agents.ts`; internal callers (e.g., passing a raw string instead of `MAIN_AGENT_SESSION_ID`) would silently type-check. A discriminated union or branded type would surface misuse earlier.
```suggestion
export const MAIN_AGENT_SESSION_ID = 'main'
/** Either the sentinel `'main'` value or a UUID string. Validated at API boundaries via `isAgentSessionId`. */
export type AgentSessionId = typeof MAIN_AGENT_SESSION_ID | (string & {})
```
### Issue 2 of 2
packages/browseros-agent/apps/server/tests/api/routes/agents.test.ts:615-623
A fixed 5 ms delay is used to ensure the first request is in-flight before the second is issued. This can be flaky in slow CI environments where Node event-loop turns take longer than expected. A deterministic synchronization point (e.g., waiting for the blocking service to acknowledge it received the first turn) would make this reliable.
```suggestion
const first = route.request('/agents/agent-1/sidepanel/chat', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
...validCreatedAgentSidepanelBody(),
conversationId: firstSessionId,
}),
})
// Wait until the blocking service has registered the first turn before
// issuing the second request, so the registry state is deterministic.
await blocking._firstTurnStarted
```
Reviews (2): Last reviewed commit: "feat: send agent session ids from chat c..." | Re-trigger Greptile |
| const first = route.request('/agents/agent-1/sidepanel/chat', { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ | ||
| ...validCreatedAgentSidepanelBody(), | ||
| conversationId: firstSessionId, | ||
| }), | ||
| }) | ||
| await new Promise((r) => setTimeout(r, 5)) |
There was a problem hiding this comment.
| const first = route.request('/agents/agent-1/sidepanel/chat', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| ...validCreatedAgentSidepanelBody(), | |
| conversationId: firstSessionId, | |
| }), | |
| }) | |
| await new Promise((r) => setTimeout(r, 5)) | |
| const first = route.request('/agents/agent-1/sidepanel/chat', { | |
| method: 'POST', | |
| headers: { 'Content-Type': 'application/json' }, | |
| body: JSON.stringify({ | |
| ...validCreatedAgentSidepanelBody(), | |
| conversationId: firstSessionId, | |
| }), | |
| }) | |
| // Wait until the blocking service has registered the first turn before | |
| // issuing the second request, so the registry state is deterministic. | |
| await blocking._firstTurnStarted |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/tests/api/routes/agents.test.ts
Line: 615-623
Comment:
A fixed 5 ms delay is used to ensure the first request is in-flight before the second is issued. This can be flaky in slow CI environments where Node event-loop turns take longer than expected. A deterministic synchronization point (e.g., waiting for the blocking service to acknowledge it received the first turn) would make this reliable.
```suggestion
const first = route.request('/agents/agent-1/sidepanel/chat', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
...validCreatedAgentSidepanelBody(),
conversationId: firstSessionId,
}),
})
// Wait until the blocking service has registered the first turn before
// issuing the second request, so the registry state is deterministic.
await blocking._firstTurnStarted
```
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!
|
Updated: sidepanel UUID sessions no longer affect the New Tab main-session recent-agent row, status, preview, or error badge. Added regression coverage for sidepanel errors and requested-session snapshots. |
|
Greptile (@greptileai) review |
|
Claude (@claude) review |
|
Claude finished Nikhil (@shadowfax92)'s task in 4m 42s —— View job Code Review
This review covers all 6 commits, with particular focus on the two new commits ( What's Working WellThe per-session activity isolation in The Issues Found1. When attachUrl: `/agents/${agent.id}/chat/stream?turnId=${err.turnId}`,But 2. Dead The param type accepts const turn = this.turnRegistry.get(turnId)
const sessionId = turn.sessionId // ← from registry, not inputThe 3. Redundant The method reads 4. A fixed 5 ms delay is used to ensure the first request is in-flight before the second. This is fragile in slow CI. A deterministic approach — waiting until the blocking service has registered the turn — would make it reliable. Greptile's suggestion ( Minor Observations
SummaryFour actionable items before merge: the |
Nikhil (shadowfax92)
force-pushed
the
fix/0601-1647-walrus
branch
from
fd4d59e to
3be8e5e
Compare
✅ Tests passed — 1126/1131
|
1 participant
Summary
mainconversation default./home/agents/:agentId/sessions/:sessionId; old/home/agents/:agentIdredirects to the latest session or a fresh UUID.latestSessionId, while the Home composer starts ACP sends in a fresh UUID session.mainroutes as compatibility fallbacks.sessionIdand drain back into that session.Rework
The previous approach forced New Tab into
main, which solved leakage but created the wrong UX. This rework treats every full-page agent chat as session-addressed: resume the latest real session by default, and use the plus button when the user wants a new session.Test plan
bun run --filter @browseros/server test:apibun run --filter @browseros/server test:libbun run --filter @browseros/agent testbun run --filter @browseros/server typecheckbun run --filter @browseros/agent typecheckgit diff --name-only | sed 's#^packages/browseros-agent/##' | xargs bunx biome checkgit diff --check