[delete-cloud]

Summary

• add PostgreSQL-backed checkpoint storage and wire HTTP/session persistence defaults through the shared [storage] config
• back HTTP session metadata with the same asyncpg/PGPool stack used by tape and checkpoint persistence instead of introducing a separate psycopg path
• add ADR/task-packet coverage plus regression tests for PG pool JSON codecs, session metadata persistence, and SessionManager/HTTP wiring

Verification

• uv run pytest tests/agentkit/storage/test_pg.py -v
• uv run pytest tests/ui/test_session_persistence.py -v
• uv run pytest tests/ui/test_session_manager_public_api.py -v
• uv run pytest tests/ui/test_http_server.py -v
• uv run pytest tests/coding_agent/plugins/test_storage_factory.py -v

Summary by CodeRabbit

• New Features

  • Optional PostgreSQL backend for session metadata, checkpoints, and JSON codec support; configurable via storage settings and used by the session manager and server.

• Bug Fixes

  • Improved async session lifecycle, reliable shutdown sequencing, SSE/event fan‑out with disconnect cleanup and queue pruning, and serialized persistence I/O.

• Documentation

  • ADR and Phase‑1 task packet documenting PostgreSQL HTTP‑session persistence plan and scope.

• Tests

  • Extensive unit tests covering Postgres stores, session manager async APIs, SSE behavior, persistence lifecycles, and codec registration.

[Copilot]

Pull request overview

This PR completes Phase 1 PostgreSQL persistence for the HTTP/UI session stack by adding a PostgreSQL checkpoint store to AgentKit and wiring SessionManager/HTTP server persistence defaults through the shared [storage] config (agent.toml).

Changes:

• Add PGCheckpointStore to agentkit.storage.pg plus tests (round-trip, list ordering, delete, schema-once, JSON codec init).
• Add synchronous UI PGSessionMetadataStore and extend create_session_store() to support backend="pg" with DSN wiring.
• Make SessionManager and http_server select tape/checkpoint/http-session backends from [storage] config and ensure shutdown closes resources.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
coding-agent/src/agentkit/storage/pg.py	Adds PG checkpoint persistence and JSON/JSONB codec initialization for asyncpg pools.
coding-agent/src/coding_agent/ui/session_store.py	Adds synchronous PG-backed HTTP session metadata store and pg backend factory options.
coding-agent/src/coding_agent/ui/session_manager.py	Wires storage backends via storage_config (pg/fs/jsonl) and adds manager shutdown.
coding-agent/src/coding_agent/ui/http_server.py	Loads [storage] from agent.toml to build the global SessionManager; closes it on shutdown.
coding-agent/src/coding_agent/agent.toml	Adds explicit storage defaults for http sessions and checkpoints.
coding-agent/tests/agentkit/storage/test_pg.py	Expands PG storage tests to cover checkpoint store and codec init.
coding-agent/tests/ui/test_session_persistence.py	Adds tests for PG session metadata store behavior and factory.
coding-agent/tests/ui/test_session_manager_public_api.py	Adds regression test that pg storage_config selects pg backends consistently.
coding-agent/docs/adr/0012-complete-phase1-postgresql-http-session-persistence.md	ADR documenting the phase-1 persistence completion decision and acceptance criteria.
coding-agent/.opencode/prompts/postgres-phase1-completion-task-packet.md	Task packet describing scope, tests, and loop policy for the work.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SessionManager creates a separate PGPool for HTTP session metadata when backend="pg" (via create_session_store) instead of reusing the same pool instance as tape/checkpoint stores. This can double connection pools and complicate shutdown. Consider passing pg_pool=self._get_pg_pool() (or similar) into create_session_store when the resolved backend is pg so all PG-backed storage shares one pool.

Suggested change

	dsn = self._storage_config.get("dsn")
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	dsn = self._storage_config.get("dsn")
	pg_pool = self._get_pg_pool() if backend == "pg" else None
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	pg_pool=pg_pool,

[Copilot]

_create_http_session_store() checks self._storage_config.get("tape_backend") == "pg" without normalizing casing/whitespace, while other code paths lower-case backend values. This can cause inconsistent behavior (e.g., tape_backend="PG" won’t trigger pg defaults for the HTTP session store). Suggest normalizing tape_backend (strip/lower) before comparisons.

[Copilot]

_create_checkpoint_store() decides the default backend based on self._storage_config.get("tape_backend") == "pg" without normalizing the tape_backend string. If tape_backend is configured as "PG"/"Pg" (or has whitespace), the default checkpoint backend will incorrectly fall back to "fs". Consider normalizing tape_backend before this comparison.

Suggested change

	default_backend = (
	"pg" if self._storage_config.get("tape_backend") == "pg" else "fs"
	)
	tape_backend = (
	str(self._storage_config.get("tape_backend", "jsonl")).strip().lower()
	)
	default_backend = "pg" if tape_backend == "pg" else "fs"

[Copilot]

test_pg_session_metadata_store_round_trips_session_metadata() creates a PGSessionMetadataStore (which starts a background event loop thread) but never calls store.close(). Even with a daemon thread, leaving the loop open can leak resources and cause warnings/flakiness across the test suite. Consider closing the store in a finally block (or using a fixture) after assertions.

Suggested change

	session = Session(
	id="pg-session",
	created_at=datetime.now(),
	last_activity=datetime.now(),
	approval_store=ApprovalStore(),
	)
	store.save(session.id, session.to_store_data())
	payload = store.load("pg-session")
	assert payload is not None
	assert payload["id"] == "pg-session"
	assert store.list_sessions() == ["pg-session"]
	assert store.check_health() is True
	try:
	session = Session(
	id="pg-session",
	created_at=datetime.now(),
	last_activity=datetime.now(),
	approval_store=ApprovalStore(),
	)
	store.save(session.id, session.to_store_data())
	payload = store.load("pg-session")
	assert payload is not None
	assert payload["id"] == "pg-session"
	assert store.list_sessions() == ["pg-session"]
	assert store.check_health() is True
	finally:
	store.close()

[Copilot]

PGSessionMetadataStore.load() assumes fetchrow() returns a dict and raises TypeError otherwise. With asyncpg, fetchrow typically returns an asyncpg.Record (mapping-like, not a dict), so this would fail in real PG usage. Consider accepting Mapping/Record types (e.g., use collections.abc.Mapping checks) and extracting the "payload" field without requiring a concrete dict instance.

[Copilot]

PGSessionMetadataStore.list_sessions() enforces that each row returned from fetch() is a dict. asyncpg returns asyncpg.Record objects for rows, so this type check is likely to raise in production. Suggest treating rows as Mapping objects (or coercing via dict(row)) and then reading the "session_id" field.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

PGSessionMetadataStore runs all DB operations on a dedicated background event loop thread, but it calls into the provided pool’s get_pool(). If the pool is an agentkit.storage.pg.PGPool shared with async tape/checkpoint stores (as SessionManager now does), the underlying asyncpg pool and/or asyncio.Lock will bind to whichever event loop touches it first. Subsequent use from the other loop will raise “attached to a different loop”/thread-safety errors. To fix this, either avoid using a shared PGPool across multiple loops (separate pool instance for the metadata store), or rework PGPool to maintain per-event-loop pools, or make the session metadata store async and run it on the same event loop as the rest of the PG backends.

[Copilot]

PGSessionMetadataStore.close() joins the loop thread with a timeout and then unconditionally calls self._loop.close(). If the thread fails to stop within the timeout, closing a running loop can raise at shutdown. Consider either joining without a timeout (or checking is_alive() and handling that case) before closing the loop to make shutdown deterministic.

Suggested change

	self._loop_thread.join(timeout=5)
	self._loop_thread.join(timeout=5)
	if self._loop_thread.is_alive():
	logger.warning(
	"Timed out waiting for postgres session metadata loop thread to stop; "
	"skipping event loop close"
	)
	return

[Copilot]

SessionManager passes the same pg_pool (PGPool) to the HTTP session metadata store and to the async PG tape/checkpoint stores. Because PGSessionMetadataStore runs on its own event loop thread, this leads to the same PGPool being used from multiple event loops, which asyncpg pools/asyncio primitives generally do not support (risking runtime "different event loop" errors). Either ensure all PG-backed stores operate on the same loop, or make PGPool safe to share across loops (e.g., per-loop underlying pools) or avoid sharing the instance across the sync metadata store and async stores.

Suggested change

	pg_pool = self._get_pg_pool() if backend == "pg" else None
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	pg_pool=pg_pool,
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	# Do not share the async PG pool with the HTTP session store:
	# the PG session metadata store may run on a dedicated event-loop
	# thread, while tape/checkpoint stores use the runtime loop.
	# Passing only the DSN lets the session store create its own
	# loop-local PG resources safely.
	pg_pool=None,

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

create_session_store() raises if resolved_dsn is missing even when pg_pool is explicitly provided. Since pg_pool is an injection/override hook, consider validating that either a DSN (arg/env) or pg_pool is present, and only error when both are absent (or drop pg_pool if it isn't intended to bypass DSN resolution).

Suggested change

	if not resolved_dsn:
	raise ValueError("PG session store requires dsn")
	if pg_pool is None and not resolved_dsn:
	raise ValueError("PG session store requires dsn or pg_pool")

[delete-cloud]

@copilot apply changes based on the comments in this thread

[Copilot]

@copilot apply changes based on the comments in this thread

Applied the requested review-thread follow-up in c109c4a (with a small test-only cleanup in 48f6699). create_session_store() now accepts an injected pg_pool without requiring a duplicate DSN, and there’s regression coverage for that path. No UI changes in this update.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

dsn_obj is validated with .strip() but then the unstripped value is passed into PGPool(dsn=dsn_obj). If the config contains leading/trailing whitespace, this will pass validation but can still fail to connect. Consider normalizing once (e.g., dsn = dsn_obj.strip()) and using the normalized value for pool construction.

Suggested change

	self._pg_pool = PGPool(dsn=dsn_obj)
	dsn = dsn_obj.strip()
	self._pg_pool = PGPool(dsn=dsn)

[Copilot]

resolved_dsn is checked for truthiness but isn’t normalized before being used to construct PGPool. A DSN coming from env/config with trailing whitespace would be accepted but could still fail at connection time. Consider stripping the DSN before passing it to PGPool (and possibly treating an all-whitespace DSN as missing).

[Copilot]

This test instantiates PGSessionMetadataStore, which starts a real background event-loop thread in __init__. Patching _loop_thread/_loop afterwards means store.close() won’t stop/close the real thread/loop that was started, so the test can leak a running daemon thread across the suite. Consider patching threading.Thread / asyncio.new_event_loop before constructing the store (or constructing the store via __new__ and setting fields) so the test only exercises the fake thread/loop it asserts against.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

PGPool now registers json/jsonb codecs via init=_init_connection_codecs. With these codecs enabled, code that passes pre-serialized JSON strings into $n::jsonb parameters will be double-encoded (json.dumps(str) -> quoted JSON string), and fetches will decode back to Python str instead of dict/list. Either stop json.dumps(...) for JSONB parameters and pass Python objects directly, or adjust/remove the custom codecs so they accept already-encoded JSON text.

Suggested change

init=_init_connection_codecs,

[Copilot]

PGCheckpointStore.save() serializes JSONB values with json.dumps() even though PGPool connections now register json/jsonb codecs. This combination will store JSON strings inside JSONB columns (double-encoding), and load()/list_by_tape() will fail type checks because meta/entries/plugin_states/extra decode as str. Pass Python objects directly for these parameters (and optionally drop the explicit ::jsonb casts if you rely on column typing).

Suggested change

	json.dumps(meta_payload),
	json.dumps(list(snapshot.tape_entries)),
	json.dumps(snapshot.plugin_states),
	json.dumps(snapshot.extra),
	meta_payload,
	list(snapshot.tape_entries),
	snapshot.plugin_states,
	snapshot.extra,

[Copilot]

PGSessionMetadataStore.save() passes json.dumps(data) into a $2::jsonb parameter. When the default pool is agentkit.storage.pg.PGPool (which now registers json/jsonb codecs), this will double-encode the payload and load() will decode a str instead of a dict. Pass the SessionPayload object directly for the JSONB parameter (or adjust codecs/casts so the driver receives raw JSON text).

[Copilot]

PGSessionMetadataStore starts the loop thread in init and immediately returns. Calls into _run_sync() can race with the thread starting / the loop beginning run_forever(), causing asyncio.run_coroutine_threadsafe to fail if invoked too early. Add a startup barrier (e.g., threading.Event set from _run_loop after set_event_loop) so init waits until the loop is ready.

Suggested change

	self._loop = asyncio.new_event_loop()
	self._loop_thread = threading.Thread(
	target=self._run_loop,
	name="pg-session-metadata-store",
	daemon=True,
	)
	self._loop_thread.start()
	def _run_loop(self) -> None:
	asyncio.set_event_loop(self._loop)
	self._loop = asyncio.new_event_loop()
	self._loop_ready = threading.Event()
	self._loop_thread = threading.Thread(
	target=self._run_loop,
	name="pg-session-metadata-store",
	daemon=True,
	)
	self._loop_thread.start()
	self._loop_ready.wait()
	def _run_loop(self) -> None:
	asyncio.set_event_loop(self._loop)
	self._loop_ready.set()

[coderabbitai]

♻️ Duplicate comments (1)

coding-agent/src/coding_agent/ui/http_server.py (1)

797-815: ⚠️ Potential issue | 🟡 Minor

TOCTOU race remains in close_session endpoint.

The endpoint still calls has_session_async before get_session_async, creating a race window where another coroutine could remove the session between the two calls. This matches the past review comment that suggested collapsing into a single try/except.

However, the current code does handle KeyError from close_session (lines 804-805), which provides partial mitigation. The remaining issue is that get_session_async on line 800 could raise KeyError without being caught.

🛡️ Proposed fix to handle race properly

 async def close_session(
     request: Request,
     session_id: str,
     api_key: str | None = Depends(verify_api_key),
 ) -> CloseSessionResponse:
     """Close session and release resources."""
-    if not await session_manager.has_session_async(session_id):
-        raise HTTPException(status_code=404, detail="Session not found")
-
-    session = await session_manager.get_session_async(session_id)
-
     try:
+        session = await session_manager.get_session_async(session_id)
+    except KeyError as exc:
+        raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc
+
+    try:
         await session_manager.close_session(session_id)
     except KeyError as exc:
         raise HTTPException(status_code=404, detail=_key_error_detail(exc)) from exc
     except Exception as exc:
         raise HTTPException(status_code=500, detail=str(exc)) from exc

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 797 - 815, The
endpoint has a TOCTOU race: calling has_session_async before get_session_async
allows the session to be deleted between calls and get_session_async can raise
KeyError unhandled; fix by removing the separate has_session_async check and
wrap the retrieval/close/broadcast sequence in a single try/except that catches
KeyError (use _key_error_detail for 404) and Exception for 500, i.e., call
session = await session_manager.get_session_async(session_id) and await
session_manager.close_session(session_id) inside the same try block and handle
KeyError and other exceptions as already done for close_session so no uncaught
KeyError escapes.

🧹 Nitpick comments (6)

coding-agent/tests/ui/test_session_manager_public_api.py (3)

281-318: Same mutable class attribute pattern - consider consolidating test fixtures.

This test duplicates the FakePGPool pattern with the same test isolation concern. Consider extracting common fake classes to module-level fixtures to reduce duplication and centralize the cleanup logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/tests/ui/test_session_manager_public_api.py` around lines 281 -
318, Extract the duplicated FakePGPool (and related
FakePGTapeStore/FakePGCheckpointStore if duplicated elsewhere) out of
test_session_manager_reuses_pg_pool_for_http_session_store into a shared
module-level test fixture or helper (e.g., module-level FakePGPool with a
reset/clear method or a pytest fixture) and update this test to use that shared
fixture; ensure the shared FakePGPool exposes a way to clear its instances list
between tests so test isolation is preserved, and update references in the test
(SessionManager invocation and assertions checking FakePGPool.instances) to use
the shared symbol.

---

232-278: Mutable class attribute instances may cause test pollution across runs.

FakePGPool.instances is a class-level mutable list that accumulates across test runs within the same process. If tests are run in a non-isolated manner (e.g., pytest with multiple tests), leftover entries from previous tests could cause assertion failures or incorrect counts.

Consider clearing the list at the start of each test or using an instance-scoped fixture.

♻️ Proposed fix

 def test_session_manager_uses_pg_backends_when_storage_config_requests_pg() -> None:
     class FakePGPool:
-        instances: list[FakePGPool] = []
+        instances: list["FakePGPool"] = []

         def __init__(self, *, dsn: str) -> None:
             self.dsn = dsn
             self.__class__.instances.append(self)
+
+    FakePGPool.instances.clear()  # Reset for test isolation

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/tests/ui/test_session_manager_public_api.py` around lines 232 -
278, The test
test_session_manager_uses_pg_backends_when_storage_config_requests_pg uses a
class-level mutable list FakePGPool.instances which can leak state across tests;
modify the test to reset FakePGPool.instances to an empty list at the start (or
teardown) so each run is isolated—i.e., before creating any FakePGPool
instances, set FakePGPool.instances = [] (or use an instance-scoped fixture to
initialize/clear it) so the final assertion len(FakePGPool.instances) == 2 is
reliable.

---

467-469: Redundant assertion on line 468.

The assertion assert close_thread_ids == [close_thread_ids[0]] is always true when the list has exactly one element (which is what line 467 implicitly checks). Consider replacing with a clearer assertion.

♻️ Proposed fix for clarity

     assert close_called.is_set() is True
-    assert close_thread_ids == [close_thread_ids[0]]
+    assert len(close_thread_ids) == 1
     assert close_thread_ids[0] != loop_thread_id

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/tests/ui/test_session_manager_public_api.py` around lines 467 -
469, The test currently uses a redundant assertion `assert close_thread_ids ==
[close_thread_ids[0]]`; replace that with an explicit length check for clarity:
assert that close_called.is_set() is True, assert len(close_thread_ids) == 1 to
show exactly one close thread was recorded (use the variable close_thread_ids),
and keep the existing uniqueness check assert close_thread_ids[0] !=
loop_thread_id to ensure the close ran on a different thread.

coding-agent/src/coding_agent/ui/session_manager.py (2)

521-527: Clarify intent: isawaitable check after executor call.

The _run_store_io runs the close method in an executor (synchronously). If close() is a regular sync method, close_result will be its return value (typically None). The isawaitable check would only matter if a sync close method unexpectedly returns a coroutine object, which is unusual.

This appears defensive but consider adding a brief comment explaining the intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 521 - 527,
In _close_resource_async, clarify why we call close via _run_store_io then still
check isawaitable on close_result: add a short inline comment above the
isawaitable check explaining that _run_store_io runs the close method in an
executor (covers sync close implementations) but we defensively handle the rare
case where a sync close returns a coroutine object (or a close implementation is
async and _run_store_io returned that coroutine), so we await it to support both
patterns; reference the symbols _close_resource_async, _run_store_io, close, and
isawaitable in the comment.

---

431-439: Fire-and-forget task may silently swallow exceptions.

The cleanup task created on line 439 runs without error handling. If _close_runtime_adapter raises an exception, it will be logged to the event loop's exception handler but otherwise silently ignored. Consider adding error handling or at minimum logging.

♻️ Proposed fix to log cleanup failures

+    async def _close_runtime_adapter_safe(self, adapter: object | None) -> None:
+        try:
+            await self._close_runtime_adapter(adapter)
+        except Exception:
+            logger.debug("Runtime adapter cleanup failed", exc_info=True)
+
     def _close_runtime_sync_safe(self, session: Session) -> None:
         adapter = session.runtime_adapter
         self._invalidate_runtime(session)
         try:
             loop = asyncio.get_running_loop()
         except RuntimeError:
             asyncio.run(self._close_runtime_adapter(adapter))
             return
-        _ = loop.create_task(self._close_runtime_adapter(adapter))
+        _ = loop.create_task(self._close_runtime_adapter_safe(adapter))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/session_manager.py` around lines 431 - 439,
The fire-and-forget close task in _close_runtime_sync_safe can silently swallow
exceptions from _close_runtime_adapter; modify _close_runtime_sync_safe so that
after creating the task (for the adapter returned by session.runtime_adapter and
after calling _invalidate_runtime(session)) you attach a done-callback that
checks task.exception() and logs any exception (via the module/class logger) or
explicitly handles it, so cleanup failures are surfaced; reference
_close_runtime_sync_safe, _close_runtime_adapter, session.runtime_adapter and
_invalidate_runtime to locate where to add the callback and logging.

coding-agent/tests/ui/test_http_server.py (1)

809-875: Consider tightening the assertion on line 872.

The assertion assert len(get_session_calls) >= 2 is intentionally loose, but it might make debugging harder if the behavior changes unexpectedly. Consider documenting why the exact count isn't asserted.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/tests/ui/test_http_server.py` around lines 809 - 875, The loose
assertion `assert len(get_session_calls) >= 2` in
test_event_generator_uses_public_session_apis_for_keepalive_exit should be
tightened: replace it with an explicit expectation (for example `assert
len(get_session_calls) == 2`) to make the test deterministic, or if the number
can legitimately vary, add a short comment immediately above the assertion
explaining why the exact count is non-deterministic and what minimum is
expected; locate this in the test function referencing get_session_calls and
session_manager.get_session_async (and adjust expected count to match actual
observed behavior).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 797-815: The endpoint has a TOCTOU race: calling has_session_async
before get_session_async allows the session to be deleted between calls and
get_session_async can raise KeyError unhandled; fix by removing the separate
has_session_async check and wrap the retrieval/close/broadcast sequence in a
single try/except that catches KeyError (use _key_error_detail for 404) and
Exception for 500, i.e., call session = await
session_manager.get_session_async(session_id) and await
session_manager.close_session(session_id) inside the same try block and handle
KeyError and other exceptions as already done for close_session so no uncaught
KeyError escapes.

---

Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_manager.py`:
- Around line 521-527: In _close_resource_async, clarify why we call close via
_run_store_io then still check isawaitable on close_result: add a short inline
comment above the isawaitable check explaining that _run_store_io runs the close
method in an executor (covers sync close implementations) but we defensively
handle the rare case where a sync close returns a coroutine object (or a close
implementation is async and _run_store_io returned that coroutine), so we await
it to support both patterns; reference the symbols _close_resource_async,
_run_store_io, close, and isawaitable in the comment.
- Around line 431-439: The fire-and-forget close task in
_close_runtime_sync_safe can silently swallow exceptions from
_close_runtime_adapter; modify _close_runtime_sync_safe so that after creating
the task (for the adapter returned by session.runtime_adapter and after calling
_invalidate_runtime(session)) you attach a done-callback that checks
task.exception() and logs any exception (via the module/class logger) or
explicitly handles it, so cleanup failures are surfaced; reference
_close_runtime_sync_safe, _close_runtime_adapter, session.runtime_adapter and
_invalidate_runtime to locate where to add the callback and logging.

In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 809-875: The loose assertion `assert len(get_session_calls) >= 2`
in test_event_generator_uses_public_session_apis_for_keepalive_exit should be
tightened: replace it with an explicit expectation (for example `assert
len(get_session_calls) == 2`) to make the test deterministic, or if the number
can legitimately vary, add a short comment immediately above the assertion
explaining why the exact count is non-deterministic and what minimum is
expected; locate this in the test function referencing get_session_calls and
session_manager.get_session_async (and adjust expected count to match actual
observed behavior).

In `@coding-agent/tests/ui/test_session_manager_public_api.py`:
- Around line 281-318: Extract the duplicated FakePGPool (and related
FakePGTapeStore/FakePGCheckpointStore if duplicated elsewhere) out of
test_session_manager_reuses_pg_pool_for_http_session_store into a shared
module-level test fixture or helper (e.g., module-level FakePGPool with a
reset/clear method or a pytest fixture) and update this test to use that shared
fixture; ensure the shared FakePGPool exposes a way to clear its instances list
between tests so test isolation is preserved, and update references in the test
(SessionManager invocation and assertions checking FakePGPool.instances) to use
the shared symbol.
- Around line 232-278: The test
test_session_manager_uses_pg_backends_when_storage_config_requests_pg uses a
class-level mutable list FakePGPool.instances which can leak state across tests;
modify the test to reset FakePGPool.instances to an empty list at the start (or
teardown) so each run is isolated—i.e., before creating any FakePGPool
instances, set FakePGPool.instances = [] (or use an instance-scoped fixture to
initialize/clear it) so the final assertion len(FakePGPool.instances) == 2 is
reliable.
- Around line 467-469: The test currently uses a redundant assertion `assert
close_thread_ids == [close_thread_ids[0]]`; replace that with an explicit length
check for clarity: assert that close_called.is_set() is True, assert
len(close_thread_ids) == 1 to show exactly one close thread was recorded (use
the variable close_thread_ids), and keep the existing uniqueness check assert
close_thread_ids[0] != loop_thread_id to ensure the close ran on a different
thread.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c3d96b0a-626e-4e89-b06e-b64c6521c13f

📥 Commits

Reviewing files that changed from the base of the PR and between 75a5c82 and ef6bf9a.

📒 Files selected for processing (4)

• coding-agent/src/coding_agent/ui/http_server.py
• coding-agent/src/coding_agent/ui/session_manager.py
• coding-agent/tests/ui/test_http_server.py
• coding-agent/tests/ui/test_session_manager_public_api.py

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

coding-agent/src/coding_agent/ui/http_server.py (2)

824-832: ⚠️ Potential issue | 🟠 Major

Return the fallback denial from the same lookup path.

This still does a stale existence check before get_session_async(). If the session is removed between those awaits, KeyError escapes and the agent loop fails instead of returning the "Session not found" rejection this function is meant to produce.

Suggested fix

-    if not await session_manager.has_session_async(session_id):
-        return ApprovalResponse(
-            session_id=session_id,
-            request_id=approval_req.request_id,
-            approved=False,
-            feedback="Session not found",
-        )
-
-    session = await session_manager.get_session_async(session_id)
+    try:
+        session = await session_manager.get_session_async(session_id)
+    except KeyError:
+        return ApprovalResponse(
+            session_id=session_id,
+            request_id=approval_req.request_id,
+            approved=False,
+            feedback="Session not found",
+        )

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 824 - 832, The
current code does a separate exists check via
session_manager.has_session_async(session_id) then calls
session_manager.get_session_async(session_id), which can race and raise
KeyError; change the flow to call get_session_async(session_id) inside a
try/except KeyError (or the session-manager's not-found exception) and on
exception return the ApprovalResponse(session_id=session_id,
request_id=approval_req.request_id, approved=False, feedback="Session not
found") so the fallback denial is returned from the same lookup path; you may
remove or keep the prior has_session_async call but ensure get_session_async is
still wrapped to handle missing sessions.

---

525-528: ⚠️ Potential issue | 🟠 Major

Collapse the remaining session existence checks into the fetch/mutate call.

These handlers still have the same check-then-act race that was fixed in close_session. If a session is deleted after has_session_async() returns, get_session_async() or add_event_queue_async() can escape as a 500 instead of a 404. Catch KeyError from the actual lookup/mutation path and map that directly.

Also applies to: 611-614, 646-651

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 525 - 528, The
code currently performs a separate existence check with
session_manager.has_session_async(session_id) before calling
session_manager.get_session_async or session_manager.add_event_queue_async,
which creates a race where the session can be deleted between calls and produce
a 500; remove the preliminary has_session_async calls and instead call the
fetch/mutate method (e.g., get_session_async, add_event_queue_async) directly
inside a try/except that catches KeyError and raises
HTTPException(status_code=404). Update the handlers that use
has_session_async/get_session_async (including the call sites around
get_session_async at the shown block and the other ranges referenced) to map
KeyError from the actual lookup/mutation to a 404 rather than relying on a prior
existence check.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 65-75: The code currently swallows ConfigError and OSError when
calling load_agent_toml(config_path) and silently returns {} (downgrading
persistence); change the error handling so only a missing config file falls back
to defaults and all other errors fail fast: call
load_agent_toml(config_path).extra.get("storage", {}) as before but replace the
broad except (ConfigError, OSError) with a narrow except FileNotFoundError (and
possibly IsADirectoryError) that logs and returns {}, and for any ConfigError or
other OSError re-raise (or let propagate) so malformed or unreadable configs do
not silently downgrade; keep the same logger usage and references to
load_agent_toml and config_path to locate the change.

---

Duplicate comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 824-832: The current code does a separate exists check via
session_manager.has_session_async(session_id) then calls
session_manager.get_session_async(session_id), which can race and raise
KeyError; change the flow to call get_session_async(session_id) inside a
try/except KeyError (or the session-manager's not-found exception) and on
exception return the ApprovalResponse(session_id=session_id,
request_id=approval_req.request_id, approved=False, feedback="Session not
found") so the fallback denial is returned from the same lookup path; you may
remove or keep the prior has_session_async call but ensure get_session_async is
still wrapped to handle missing sessions.
- Around line 525-528: The code currently performs a separate existence check
with session_manager.has_session_async(session_id) before calling
session_manager.get_session_async or session_manager.add_event_queue_async,
which creates a race where the session can be deleted between calls and produce
a 500; remove the preliminary has_session_async calls and instead call the
fetch/mutate method (e.g., get_session_async, add_event_queue_async) directly
inside a try/except that catches KeyError and raises
HTTPException(status_code=404). Update the handlers that use
has_session_async/get_session_async (including the call sites around
get_session_async at the shown block and the other ranges referenced) to map
KeyError from the actual lookup/mutation to a 404 rather than relying on a prior
existence check.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 951d61c1-a459-4906-834a-5c7a1e7c530e

📥 Commits

Reviewing files that changed from the base of the PR and between ef6bf9a and 6b32664.

📒 Files selected for processing (2)

• coding-agent/src/coding_agent/ui/http_server.py
• coding-agent/tests/ui/test_http_server.py

✅ Files skipped from review due to trivial changes (1)

• coding-agent/tests/ui/test_http_server.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't silently downgrade persistence on config errors.

Falling back to {} on ConfigError or a generic OSError means a bad or unreadable agent.toml boots the server with the default storage backend instead of PostgreSQL. That turns a config bug into silent non-persistent sessions/checkpoints. Limit the fallback to the intentional “no local config present” case, and fail fast for malformed/unreadable configs.

Possible tightening

 def _load_storage_config() -> dict[str, Any]:
     config_path = Path(__file__).resolve().parent.parent / "agent.toml"
     try:
         return cast(
             dict[str, Any], load_agent_toml(config_path).extra.get("storage", {})
         )
-    except (ConfigError, OSError):
+    except FileNotFoundError:
         logger.warning(
             "Unable to load storage config from %s; using defaults",
             config_path,
             exc_info=True,
         )
         return {}
+    except (ConfigError, OSError) as exc:
+        raise RuntimeError(
+            f"Failed to load storage config from {config_path}"
+        ) from exc

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 65 - 75, The
code currently swallows ConfigError and OSError when calling
load_agent_toml(config_path) and silently returns {} (downgrading persistence);
change the error handling so only a missing config file falls back to defaults
and all other errors fail fast: call
load_agent_toml(config_path).extra.get("storage", {}) as before but replace the
broad except (ConfigError, OSError) with a narrow except FileNotFoundError (and
possibly IsADirectoryError) that logs and returns {}, and for any ConfigError or
other OSError re-raise (or let propagate) so malformed or unreadable configs do
not silently downgrade; keep the same logger usage and references to
load_agent_toml and config_path to locate the change.

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3bf95f1d-0eb6-4603-a9ab-4e9d29a7a674

📥 Commits

Reviewing files that changed from the base of the PR and between 6b32664 and 54e4ee6.

📒 Files selected for processing (2)

• coding-agent/src/coding_agent/ui/session_store.py
• coding-agent/tests/ui/test_session_manager_public_api.py

[coderabbitai]

🧹 Nitpick comments (3)

coding-agent/tests/ui/test_session_persistence.py (1)

808-813: Minor: Use direct attribute access instead of getattr with constant.

Static analysis correctly identifies that getattr(operation, "close") can be simplified.

♻️ Simplify attribute access

     def fake_run_sync(operation: object) -> None:
-        cast(object, operation)
-        close = getattr(operation, "close")
-        cast(object, close)
-        close()
+        operation.close()  # type: ignore[union-attr]
         return None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/tests/ui/test_session_persistence.py` around lines 808 - 813,
The helper fake_run_sync uses getattr(operation, "close") unnecessarily; replace
getattr and the redundant cast of close by directly calling operation.close(),
e.g., call operation.close() (you can also remove the cast(object, close) since
direct attribute access is explicit) and keep the function returning None to
preserve behavior in fake_run_sync.

coding-agent/src/coding_agent/ui/session_store.py (2)

484-485: Redundant type check after dict() constructor.

The isinstance(row_dict_obj, dict) check at line 484 is unreachable since dict() always returns a dict. While defensive, this is dead code.

♻️ Remove redundant check

     try:
         row_items = cast(Iterable[tuple[object, object]], row)
         row_dict_obj = dict(row_items)
     except Exception as exc:
         raise TypeError(f"{context} must be convertible to a dict") from exc
-    if not isinstance(row_dict_obj, dict):
-        raise TypeError(f"{context} must decode to a dict")
     return cast(dict[str, object], row_dict_obj)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/session_store.py` around lines 484 - 485,
The isinstance check guarding row_dict_obj is redundant because row_dict_obj is
created via dict() earlier and will always be a dict; remove the unreachable
check and its TypeError raise (the lines referencing isinstance(row_dict_obj,
dict) and raise TypeError(f"{context} must decode to a dict")). Keep the
surrounding logic intact so decoding proceeds normally; locate the check by
searching for the row_dict_obj variable and the context message in
session_store.py to remove that conditional block.

---

228-231: Bare except Exception: pass during cancellation cleanup is acceptable here, but consider adding debug logging.

The pattern is valid since you're in cleanup mode after a timeout and only need to ensure the future completes. Since this catches Exception (not BaseException), KeyboardInterrupt and SystemExit still propagate correctly.

For debugging production issues, you might consider logging at DEBUG level, but this is optional.

♻️ Optional: Add debug logging for visibility

             try:
                 _ = future.result(timeout=0.1)
-            except (FutureTimeoutError, asyncio.CancelledError, Exception):
-                pass
+            except (FutureTimeoutError, asyncio.CancelledError) as exc:
+                logger.debug("Future cancellation completed: %s", type(exc).__name__)
+            except Exception as exc:
+                logger.debug("Unexpected exception during cancellation cleanup: %s", exc)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@coding-agent/src/coding_agent/ui/session_store.py` around lines 228 - 231,
The cleanup block currently swallows all exceptions with "except
(FutureTimeoutError, asyncio.CancelledError, Exception): pass"; update this
handler to emit a debug-level log so failures during future cleanup are visible
in diagnostics: catch the same exceptions around future.result(...) and call the
module/class logger (or logging.getLogger(__name__)) to log the exception and
the future identity (e.g., logger.debug("future cleanup failed", exc_info=True,
extra={"future": repr(future)})) so you retain the same behavior but surface
debug details; keep using FutureTimeoutError and asyncio.CancelledError in the
tuple to preserve semantics.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@coding-agent/src/coding_agent/ui/session_store.py`:
- Around line 484-485: The isinstance check guarding row_dict_obj is redundant
because row_dict_obj is created via dict() earlier and will always be a dict;
remove the unreachable check and its TypeError raise (the lines referencing
isinstance(row_dict_obj, dict) and raise TypeError(f"{context} must decode to a
dict")). Keep the surrounding logic intact so decoding proceeds normally; locate
the check by searching for the row_dict_obj variable and the context message in
session_store.py to remove that conditional block.
- Around line 228-231: The cleanup block currently swallows all exceptions with
"except (FutureTimeoutError, asyncio.CancelledError, Exception): pass"; update
this handler to emit a debug-level log so failures during future cleanup are
visible in diagnostics: catch the same exceptions around future.result(...) and
call the module/class logger (or logging.getLogger(__name__)) to log the
exception and the future identity (e.g., logger.debug("future cleanup failed",
exc_info=True, extra={"future": repr(future)})) so you retain the same behavior
but surface debug details; keep using FutureTimeoutError and
asyncio.CancelledError in the tuple to preserve semantics.

In `@coding-agent/tests/ui/test_session_persistence.py`:
- Around line 808-813: The helper fake_run_sync uses getattr(operation, "close")
unnecessarily; replace getattr and the redundant cast of close by directly
calling operation.close(), e.g., call operation.close() (you can also remove the
cast(object, close) since direct attribute access is explicit) and keep the
function returning None to preserve behavior in fake_run_sync.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 8f7ce818-4b9c-4d18-8508-2d92bcd6483b

📥 Commits

Reviewing files that changed from the base of the PR and between 54e4ee6 and 26ab735.

📒 Files selected for processing (2)

• coding-agent/src/coding_agent/ui/session_store.py
• coding-agent/tests/ui/test_session_persistence.py

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

SessionManager passes the same PGPool instance (via _get_pg_pool()) into create_session_store(pg_pool=...), but PGSessionMetadataStore runs DB operations on its own dedicated event loop thread. PGPool contains an asyncio.Lock and creates an asyncpg pool bound to the event loop that first calls get_pool(); reusing a single PGPool across the main loop (tape/checkpoints) and the metadata store’s background loop will raise “bound to a different event loop”/cross-loop asyncpg errors at runtime. Consider giving the HTTP session metadata store its own PGPool instance (e.g., pass only dsn so create_session_store builds a separate pool, or explicitly create a second PGPool for metadata), or refactor PGSessionMetadataStore to operate on the same loop as the async PG backends instead of a private loop thread.

Suggested change

	pg_pool = self._get_pg_pool() if backend == "pg" else None
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	pg_pool=pg_pool,
	return create_session_store(
	backend=backend,
	dsn=dsn if isinstance(dsn, str) else None,
	pg_pool=None,

[Copilot]

create_session_store() only falls back to in-memory on OSError, but RedisSessionStore.init calls client.ping(), and redis-py typically raises redis.exceptions.ConnectionError/RedisError (not OSError) when Redis is unreachable. That would now bubble up and prevent the intended fallback behavior when Redis is down. Suggest catching broader Redis connectivity exceptions (or Exception) while still allowing ConfigError to propagate (e.g., except Exception as exc: if isinstance(exc, ConfigError): raise ...; log+fallback otherwise).

[Copilot]

In get_events() keepalive handling, the TimeoutError branch calls has_session_async() and then has_event_queue_async(). If the session is removed between these two awaits (or was cached when has_session_async ran), has_event_queue_async() can raise KeyError, which will currently escape the generator and likely surface as an SSE error. Consider wrapping the has_event_queue_async() call in a try/except KeyError and breaking cleanly (similar to the disconnect cleanup), or combining the checks into a single call that can’t race into an unhandled KeyError.

Suggested change

	if not await session_manager.has_event_queue_async(
	session_id, queue
	):
	try:
	if not await session_manager.has_event_queue_async(
	session_id, queue
	):
	break
	except KeyError:

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The close-session endpoint returns detail=str(exc) for unexpected errors. This can leak internal exception messages (and potentially sensitive data like DSNs/credentials) to API clients. Prefer logging the exception server-side and returning a generic 500 message, or redact/sanitize the error text before including it in the response.

Suggested change

	raise HTTPException(status_code=500, detail=str(exc)) from exc
	logger.exception("Unexpected error while closing session %s", session_id)
	raise HTTPException(
	status_code=500, detail="Internal server error"
	) from exc

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In close_session(), the except (asyncio.CancelledError, asyncio.TimeoutError): pass block will also swallow cancellation of the current task (e.g., request disconnect or server shutdown) if it happens while awaiting wait_for(session.task, ...). That can prevent cooperative cancellation and keep shutdown hanging. Consider handling CancelledError separately: only ignore it when it comes from awaiting the cancelled session.task (e.g., if session.task.done(): ...), otherwise re-raise to propagate caller cancellation.

[Copilot]

shutdown_session_runtime() has the same cancellation-swallowing pattern as close_session(): catching asyncio.CancelledError from wait_for(session.task, ...) and continuing can suppress cancellation of the shutdown task itself. Please re-raise caller cancellation (while still tolerating CancelledError from the session task completing) so shutdown remains responsive/correct under cancellation.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.