feat(widget): conversation thread list — list, new, switch, auto-title

[AmitAvital1]

Stack 2/3 · base: feat/widget-context-usage (#58)

Adds a conversation thread drawer to the widget: list past chats, start a new one, and switch between them. Thread identity is anonymous per-browser (crypto.randomUUID() in localStorage, scoped per endpoint and per user) with an <agent-chat user="..."> seam for host-app identity. Titles are auto-generated on the backend from the first user message (thread_title), keeping the FE a pure pipe.

Backend adds list_sessions / rename_session to the repository port (memory + SQL adapters) and GET /conversations?user_id. A turn's identity is taken from the stored session rather than from the browser: a caller that omits user_id runs as the session's owner, one that sends a different user_id gets a 403 (review feedback, 0c6ed9f).

Verified: make lint + mypy clean, tests + widget typecheck/e2e green.

🤖 Generated with Claude Code

[Asaf-prog]

PR #59

The thread-list feature looks good, but I see a few identity and conversation-ownership issues that should be addressed before merging.

First, the active conversation ID is stored using only the endpoint. It should also be scoped by userId; otherwise, when a different user signs in on the same browser, the widget may reuse the previous user’s conversation.

Second, user_id is sent when the conversation is created, but it is not sent or derived during send and stream. As a result, the agent execution can run with RunContext.user_id = None.

The backend should load the conversation session, validate that it belongs to the authenticated user, and derive the user identity from the session rather than trusting a user_id supplied by the browser.

The new conversation-list endpoint also accepts user_id directly from the query string. This is fine as a temporary anonymous-browser identifier, but it should not be treated as an authorization boundary.

I would request changes here before merging, especially because user identity can affect tool access and authorization.

[AmitAvital1]

All three points were right — fixed in 0c6ed9f.

1. Active conversation id scoped only by endpoint. Correct, and it's a real cross-user leak on a shared browser (or when a host app signs a new user in via <agent-chat user="...">). The key is now agent-chat:<endpoint>:<userId>, and getStoredConversationId / setStoredConversationId / removeStoredConversationId all take the user id, so there's no way to touch the store without naming a user.

2. user_id missing on send/stream → RunContext.user_id = None. This was the one that mattered most, since that's the field hooks and tools authorize on. Fixed the way you suggested — identity comes from the stored session, not the browser:

session = await self._require(conversation_id)
if session.user_id and user_id and user_id != session.user_id:
    raise ConversationAccessDenied(conversation_id)
user_id = session.user_id or user_id

So a caller that sends no user_id runs as the session's owner, and a caller that sends a different one gets 403 instead of being silently rebound. _require now returns the session it already had to fetch, so the check costs no extra query. The widget no longer needs to assert who it is on a turn at all.

3. GET /conversations?user_id= is not an authorization boundary. Agreed — it stays a scoping parameter, and the route docstring now says so explicitly, plus where a deployment puts real auth (in front of the router / by overriding get_service) to derive the id from a verified credential.

New tests: a turn with no client user_id runs as the owner; a mismatched user_id raises and persists nothing.

Rebased on top of the updated #58.

[Asaf-prog]

Thanks for addressing the previous feedback. The localStorage scoping and deriving RunContext.user_id from the stored session are good improvements.

However, I think the main authorization issue is still only partially resolved.

The current check rejects a request only when the client explicitly sends a different user_id:

if session.user_id and user_id and user_id != session.user_id:
    raise ConversationAccessDenied(...)

But the widget does not send user_id during send or stream. This means that anyone who knows a conversation ID can call the endpoint without a user_id, and the backend will automatically execute the request as the session owner:

user_id = session.user_id or user_id

Because RunContext.user_id is used by hooks and tools for authorization, this could make an unauthenticated caller appear as the real session owner.

I think ownership should be validated against a trusted authenticated principal derived by middleware or a dependency, rather than against an optional browser-supplied user_id.

The same validation should apply to:

• listing conversations
• reading messages
• reading token usage
• sending messages
• streaming messages

The browser-generated user ID can still be useful for anonymous session scoping, but it should not be treated as an authorization boundary.

So I would still hold off on merging until the session owner is compared against a trusted caller identity, or until these routes are explicitly separated and documented as anonymous-only APIs.

[AmitAvital1]

You're right, and the hole was wider than the send/stream case you named — thanks for pushing on it. Fixed in 61a5086.

The problem was that the caller's identity had three possible sources (request body, query string, or nothing at all) and the "nothing at all" branch fell back to the owner. So the fix was to give it exactly one source and make every route authorize against it.

Trusted principal via a dependency. get_caller_id is now the only place a caller id enters the API — no route reads one from a body or query string:

CALLER_HEADER = "X-Agent-Chat-User"

def get_caller_id(request: Request) -> str | None:
    return request.headers.get(CALLER_HEADER)

Being the single source is what makes it replaceable. A deployment with real auth overrides this one dependency with a principal derived from a verified credential, and all five routes tighten with it:

app.dependency_overrides[get_caller_id] = my_authenticated_principal

Ownership checked, not inherited. session.user_id or user_id is gone. Every read and write goes through _authorize, which requires an exact match:

async def _authorize(self, conversation_id: str, caller_id: str | None) -> ConversationSession:
    session = await self._require(conversation_id)
    if session.user_id != caller_id:
        raise ConversationAccessDenied(conversation_id)
    return session

Exact means an owned conversation is unreachable both by a different caller and by an anonymous one — the case you identified, where omitting the id was the way in. The turn still runs as the session owner, so RunContext.user_id stays meaningful for hooks and tools, but only the owner reaches it. This now covers all five routes you listed: listing (an unidentified caller gets []), reading messages, reading token usage, sending, and streaming — the last four returning 403.

Anonymous by default, and documented as such. Your last point stands: the browser-generated id is scoping, not authorization. docs/api.mdx now says so in a warning — the header is unverified out of the box, and these routes should be treated as an anonymous API until get_caller_id is overridden. What changed is that the id is no longer an authorization boundary that can be bypassed by omission; it is a boundary that is only as strong as its source, and the source is one function.

I did not add authentication itself. There is none in this service today and picking a scheme is a separate decision — this leaves the seam for it.

Tests: a non-owner and an anonymous caller both get 403 on all four conversation routes, plus service-level coverage that an unowned caller can't read history or usage and that listing is empty. The old "runs as the session owner when the caller sends no user_id" test is inverted into a refusal, since that behavior was the bug.

[Asaf-prog]

Thanks for addressing the authorization issue. The new get_caller_id dependency and exact ownership checks across the conversation routes resolve my previous concern.

However, I found another ownership bypass in the conversation creation flow.

POST /conversations still allows the caller to provide a custom session_id:

class CreateConversationRequest(BaseModel):
    session_id: str | None = None

The route then passes both the caller identity and the supplied session ID into service.create().

In SqlRepository.create_session(), if that session already exists, the existing row is updated:

row.user_id = user_id if user_id is not None else row.user_id

This means that a caller who knows an existing conversation ID could call:

POST /conversations
X-Agent-Chat-User: another-user

{
  "session_id": "existing-conversation-id"
}

and potentially replace the current owner with themselves. After that, the new _authorize() check would succeed because the stored owner was already changed.

There is also inconsistent behavior between the repositories: MemoryRepository returns the existing session without changing ownership, while SqlRepository updates it. Tests using only the memory implementation may therefore miss this issue.

I think one of the following should happen before merging:

1. Remove client-supplied session_id from the public conversation creation API and always generate it server-side, or
2. Return 409 Conflict when the supplied session ID already exists.

In either case, create_session() should never change the owner of an existing conversation.

It would also be useful to add a SQL repository regression test that:

• creates a session owned by alice
• attempts to create the same session ID as bob
• verifies that the request fails
• verifies that ownership remains with alice

I would still hold off on merging until this ownership reassignment path is closed.

[AmitAvital1]

Confirmed and fixed — thanks, this one was the most serious of the three. I reproduced it before fixing: the regression test fails on SQL with assert 'bob' == 'alice' and passes on memory, exactly the divergence you predicted.

Ownership is now set once and never moves (5d45116):

• SqlRepository.create_session() no longer writes user_id on an existing row, and the Repository port states the invariant instead of leaving each backend to infer it — that gap is why the two disagreed.
• ConversationService.create() refuses a session_id owned by anyone else; the route answers 409.

I took your option 2 rather than removing caller-supplied ids, because re-creating an id you already own is a real path — agentctl run --session X calls create on every run — so creation stays idempotent for the owner and only a foreign id is refused. Happy to drop caller-chosen ids from the HTTP API instead if you'd rather have the smaller surface.

The regression test is a contract test, run against both backends. A memory-only suite missing a SQL-only vulnerability was the more interesting half of your comment, so create_session ownership is now pinned for every implementation rather than for one.

Your comment made me audit the rest of the flow, and it found three more (27ef45d):

1. append_message() also wrote session ownership — SQL filled a null user_id from the message, and memory did the same one line over. Not reachable through the API now that _authorize() runs first, but it is the identical shape, so both are gone and the contract test covers "no later write moves the owner". Nothing depended on it.
2. create() originally pre-checked with get_session(), which two concurrent callers could both pass. The repository is the authority now: the caller that did not create it is told the name is taken.
3. An empty X-Agent-Chat-User produced a conversation owned by "", which list_conversations treats as no caller — it existed but no listing could ever reach it. Empty and absent now both mean anonymous, and the header is bounded at 64 characters, the width of the column it becomes.

Also on the widget side: it now treats 403 on a stored conversation like 404 and starts a fresh thread. Once get_caller_id is overridden with real auth, a stored id outlives the identity that created it, and the old code would have retried it forever.

One structural change (f323b99): the status mapping was copy-pasted into eleven except clauses, which is precisely how list_messages and get_usage ended up with no 403 in your previous round. It is one table plus a context manager now, so a route takes the whole mapping or none of it.

Every fix above was verified by reverting it and watching the test fail, not just by a green suite. 555 pytest, 16 Playwright, CI green.

Two things I deliberately left alone: /runs/{run_id} on the engine API has the same "id is a bearer capability" shape but is a different service and out of scope here — worth its own issue if you agree. And upsert_user will create a row for any caller-supplied id in an anonymous deployment, which is rate limiting rather than authorization.

[Asaf-prog]

Thanks for fixing the ownership reassignment issue. The ownership invariant is now much stronger: existing sessions no longer change owners, message writes cannot claim a conversation, and foreign reuse is mapped to 409 Conflict.

However, I do not think the concurrent creation case is fully handled yet.

The current SQL flow still does:

row = await session.get(ConversationSessionRow, sid)

if row is None:
    row = ConversationSessionRow(...)
    session.add(row)

With two truly concurrent requests for the same session_id, both transactions can observe that the row does not exist and both attempt the insert.

One transaction will win, while the other will likely fail with a primary-key IntegrityError during flush or commit.

In that case, create_session() never returns the winning session, so this service-level check is never reached:

if session.user_id != user_id:
    raise ConversationAlreadyExists(session.session_id)

The losing request may therefore return 500 instead of the expected 409 Conflict.

The test named test_create_refuses_an_id_that_lost_the_race does not currently exercise a real race. It creates Alice’s session first and only then calls Bob sequentially.

I think the SQL repository should make this operation atomic, for example by using:

• INSERT ... ON CONFLICT DO NOTHING, followed by loading the stored session, or
• catching the duplicate-key IntegrityError, rolling back the failed transaction, and loading the winning session in a new transaction.

Then the service can reliably compare the returned owner:

• same owner: idempotent success
• different owner: 409 Conflict

Please also add a real SQL concurrency regression test using two separate database sessions and concurrent calls, such as asyncio.gather(), and verify that:

• exactly one caller owns the session
• ownership never changes
• the losing caller receives ConversationAlreadyExists
• the API returns 409, not 500

You can paste the prompt below into your coding agent to implement the fix.

Prompt for the agent:

Fix the remaining concurrent conversation-creation race in PR #59.

Problem

SqlRepository.create_session() currently follows a read-then-insert flow:

row = await session.get(ConversationSessionRow, sid)

if row is None:
    row = ConversationSessionRow(...)
    session.add(row)

Two concurrent callers using the same session_id can both observe that the row does not exist and both attempt to insert it.

One insert succeeds. The other can fail with a primary-key IntegrityError during flush or commit.

That exception currently bypasses the domain-level ownership check in ConversationService.create() and can become an HTTP 500 instead of 409 Conflict.

The existing test_create_refuses_an_id_that_lost_the_race is sequential and does not reproduce this race.

Required behavior

Conversation creation must be atomic and preserve these invariants:

1. Ownership is assigned only when the session is first created.
2. Ownership must never change afterward.
3. Recreating the same session_id by the same owner is idempotent.
4. Recreating the same session_id by another owner raises ConversationAlreadyExists.
5. Concurrent creation must not expose raw database integrity errors.
6. The API must return 409 Conflict, not 500, to the losing foreign caller.

Implementation

Update SqlRepository.create_session() so concurrent creation is handled atomically.

Prefer one of these approaches:

Option A: database upsert

Use the appropriate SQLAlchemy dialect insert:

• PostgreSQL: INSERT ... ON CONFLICT DO NOTHING
• SQLite: supported conflict-ignore equivalent

After the insert attempt, load the session row by session_id and return the stored session.

Do not update user_id when the row already exists.

Option B: handle duplicate-key failure

Attempt the insert and flush it inside the transaction.

If a uniqueness or primary-key IntegrityError occurs:

1. Roll back the failed transaction.
2. Open a clean transaction or session.
3. Load the existing session by session_id.
4. Return the existing session.
5. Do not modify its owner or metadata as part of the failed create attempt.

Do not broadly catch unrelated integrity errors. Only translate the duplicate-session conflict.

Service behavior

Keep the ownership comparison in ConversationService.create():

session = await repository.create_session(...)

if session.user_id != user_id:
    raise ConversationAlreadyExists(session.session_id)

This should remain the authoritative decision:

• same owner: return the existing session ID
• different owner: raise ConversationAlreadyExists

Tests

Add a real SQL concurrency regression test.

The test must:

1. Use the SQL repository, not MemoryRepository.
2. Use separate database sessions/connections for the concurrent operations.
3. Start two creation calls for the same session_id concurrently using asyncio.gather() or an equivalent synchronization barrier.
4. Use different owners, for example alice and bob.
5. Verify that only one owner is persisted.
6. Verify that ownership never changes after the race.
7. Verify that the losing caller gets ConversationAlreadyExists.
8. Add an API-level assertion that the losing request maps to 409 Conflict, not 500.

Avoid a test where one creation completes before the second starts; that does not reproduce the race.

Also keep the existing repository contract tests proving that:

• create_session() never reassigns ownership
• append_message() never claims or changes ownership
• same-owner recreation remains idempotent

Run the full lint, typecheck, pytest, widget checks, and relevant API tests after the change.

[AmitAvital1]

You're right, and I reproduced it before fixing: two repositories over one on-disk SQLite with asyncio.gather raise a bare IntegrityError on ten runs out of ten. Fixed in 506d21d.

Worth stating what was and wasn't at stake, since it shaped the fix: the database always picked exactly one winner, so the data was never at risk. What was wrong is what the loser was told — a 500 for asking after a taken name, plus a logged crash for what is ordinary traffic.

The insert now runs inside a savepoint:

created = ConversationSessionRow(...)
try:
    async with session.begin_nested():
        session.add(created)
    row = created
except IntegrityError:
    row = await session.get(ConversationSessionRow, sid)
    if row is None:
        raise

The savepoint is the part that matters. A plain IntegrityError poisons the surrounding transaction — on PostgreSQL nothing more can run in it — so recovery would have to happen in a second session, which is a trap for whoever edits the method next. Rolling back to a savepoint discards that one statement and leaves the transaction usable, so the read-back happens in place. ConversationService.create() is unchanged and still authoritative: same owner is idempotent, a different owner raises ConversationAlreadyExists.

I went with the savepoint over your Option A (ON CONFLICT DO NOTHING) after writing both. The upsert works, but SQLAlchemy has no portable spelling for it, so it needs a runtime dialect branch, session.connection() to issue Core DML, and a second copy of the column list beside the ORM model. The savepoint keeps the model as the single description of a row and has no backend conditionals, and both supported dialects implement it. I checked it is a real SAVEPOINT / ROLLBACK TO SAVEPOINT / COMMIT round trip rather than a silent no-op, since pysqlite has a history there, and hammered it at 25 runs x 4 concurrent creators: no errors, all callers unanimous on one owner.

The catch is deliberately narrow, per your note about not swallowing unrelated failures: it is absorbed only if the id turns out to be taken. An integrity error that leaves no row to find — the user_id foreign key, a not-null — is re-raised, and there is a test pinning that.

Tests — a new SQL concurrency module, on-disk because an in-memory database is a single shared connection and races nothing:

1. Repository under a real race: both callers get a session, neither an IntegrityError, and both agree on one owner.
2. API under a real race: [200, 409], never a 500. This needs two app instances, the way two replicas would be — my first attempt used one app, passed without the fix because the requests serialised, and only showed itself when I checked it failed on the old code.
3. An unrelated integrity error still propagates.

I did not add a separate service-level assertion: 409 is reachable only through ConversationAlreadyExists, so the API test already proves the loser was refused. The ownership invariants you asked to keep — creation never reassigns, append_message never claims — remain in the contract tests across both backends.

Also deleted test_create_refuses_an_id_that_lost_the_race. You were right that it raced nothing, and it was a weaker copy of the test above it.

On keeping caller-supplied session_id rather than removing it: a stable id lets a host application resume a conversation without storing our UUID, so a 409 there is a real product outcome and not only a race artifact. If it ever grows real users, the better shape is to namespace the id by caller — uuid5(ns, f"{user}:{name}") — so naming another user's conversation becomes impossible by construction rather than refused. Not worth building before there is a client for it.

[Asaf-prog]

Thanks for fixing the actual concurrent insert race. The savepoint approach is sound, and the new SQL concurrency tests now exercise the real failure mode with separate connections and concurrent requests. The 200/409 API assertion also confirms that the losing caller no longer leaks a raw IntegrityError as a 500.

However, I found another issue in SqlRepository.create_session().

The repository contract now says that when a session ID already exists, the existing session should be returned unchanged. MemoryRepository follows that contract:

existing = self._sessions.get(sid)
if existing is not None:
    return existing

But the SQL implementation still updates fields on an existing session:

elif any(
    value is not None
    for value in (system_name, config_path, title, metadata, expires_at)
):
    row.system_name = ...
    row.config_path = ...
    row.title = ...
    row.metadata_json = ...
    row.expires_at = ...

This is both a repository inconsistency and an authorization concern.

ConversationService.create() calls create_session() first and only checks the owner afterward:

session = await self._repository.create_session(...)

if session.user_id != user_id:
    raise ConversationAlreadyExists(session.session_id)

So a caller can submit an existing session_id owned by another user. The SQL repository may modify that session’s system_name, config_path, metadata, or expiration before the service rejects the request with 409.

The caller does not gain ownership, but the foreign session has already been mutated.

This is especially relevant if multiple agent systems or configurations share the same conversation database. A rejected create request could still rebind an existing session to the current application’s system or config.

It also means same-owner recreation is not truly idempotent in SQL, even though it is idempotent in memory.

I think create_session() should treat all creation fields as immutable once the ID exists:

row = await session.get(ConversationSessionRow, sid)

if row is not None:
    return _session(row)

The existing-session update branch should be removed completely.

Fields such as title, metadata, configuration, or expiration should only be changed through explicit update methods that perform their own authorization, such as the existing rename_session() operation.

Please also extend the repository contract tests to run against both memory and SQL:

1. Create a session owned by alice with system-a, config-a, metadata, title, and expiration.
2. Call create_session() again with the same ID but different values.
3. Verify that every original field remains unchanged.
4. Add an API regression test where bob attempts to create Alice’s ID and receives 409.
5. Verify after the rejected request that Alice’s complete session row is unchanged.

The concurrency issue is now fixed, but I would still hold off on merging until existing-session creation is side-effect free and both repository implementations follow the same contract.

[AmitAvital1]

Right on both counts — the divergence between the backends, and the side effect a rejected request could reach. Fixed in 144cb4c.

The part I had missed is that ConversationService.create() passes system_name and config_path on every call, so that branch fired on every rejected create. Naming a live conversation rewrote its system, config and updated_at before the 409 came back. Ownership never moved, but in the shared-database case you describe, a session could be rebound to another application's system by a request that was refused.

create_session() now returns immediately when the id exists:

row = await session.get(ConversationSessionRow, sid)
if row is not None:
    return _session(row)

The update branch is gone. The port states the rule rather than leaving it to each backend to infer — creation fields describe a session being born, so a taken id writes nothing at all, and later changes go through the explicit operations (rename_session and the rest) that answer for their own authorization.

Tests, both of the cases you listed:

1. A contract case creates a fully populated session — owner, system, config, title, metadata, expiry — then re-creates the same id with every field different and asserts the stored row is byte-identical. It runs on both backends and fails on SQL before this change, which is the divergence itself under test.
2. An API case models the shared-database scenario: alice creates shared-id under system-a, bob names it under system-b, and after the 409 alice's complete row is compared against a snapshot taken before the attempt. It fails before the change with the row differing.

I renamed the SQL module from test_sql_concurrency to test_sql_conversation_creation, since it now covers what creation leaves behind as well as the race.

Nothing depended on the old update behaviour — service.create() was its only caller with those fields, and no test asserted it.