diff --git a/CHANGELOG.md b/CHANGELOG.md index 782ceb3..dd66442 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **`X-Session-Id` header is now recognized as a fallback** for the existing `X-Client-Session-Id` correlation mechanism (PR #71). opencode and other Vercel AI SDK-based clients send `X-Session-Id` by default; amplifier-agent now picks it up automatically, so session-resume + client-authoritative reconciliation works for opencode with zero config. `X-Client-Session-Id` remains authoritative when both headers are present. + +- **Workspace name is no longer suffixed with the client session id.** Previously, `X-Client-Session-Id: abc` would route requests into `workspaces/-abc/`. Now the workspace stays at `` and per-client distinction is purely at the session_id level (`workspaces//sessions/http-abc/`). This keeps workspace-level hook state (context-intelligence, etc.) shared across all sessions of the same server process, where it belongs. + - **`amplifier-agent serve chat-completions` lifespan now triggers the same module-install path that `amplifier-agent run` uses.** Previously, a fresh `uv tool install amplifier-agent` followed by `serve chat-completions` would fail with `ProviderModuleNotInstalledError` because the lazy-install that `run` gets via `create_session() → session.initialize() → resolver.async_resolve()` never fires for `serve`. The lifespan now calls `prepared.resolver.async_resolve(module_id, source)` for every `PROVIDER_CATALOG` entry before the providers loop — idempotent (no-op on warm cache) and asynchronous (lifespan waits for completion before opening the wire). - **`single_turn.py` now explicitly clears `mount_plan["providers"]` before `inject_provider`.** `bundle.md` now populates the top-level `providers:` section with 4 stubs so `bundle.prepare()` installs them. Without the clear, `inject_provider`'s "no-op if providers already present" guard would fire and skip injecting the runtime provider (with env-var credentials). This mirrors the pattern already used by `_session_runner.run_chat_turn`. diff --git a/src/amplifier_agent_http/routes/chat_completions.py b/src/amplifier_agent_http/routes/chat_completions.py index bb0d408..8ef1d2c 100644 --- a/src/amplifier_agent_http/routes/chat_completions.py +++ b/src/amplifier_agent_http/routes/chat_completions.py @@ -593,31 +593,24 @@ async def chat_completions( # is in the v2 backlog (would need per-session mount-plan isolation). base_workspace = getattr(request.app.state, "resolved_workspace", None) - # Client session correlation (per-request workspace override). - # When the client attaches ``X-Client-Session-Id`` to outbound requests, - # the server suffixes the resolved workspace with the supplied value so - # all turns of one logical client session land under the same on-disk - # bucket: + # Client session correlation. When the client attaches X-Client-Session-Id + # (amplifier-native) or X-Session-Id (opencode / Vercel AI SDK default), + # the deterministic session_id `http-` is used so all turns of + # one logical client session land in the same on-disk session bucket + # under workspaces//sessions/. The workspace itself is NOT + # suffixed -- it stays at server-process scope so hook-level state + # (context-intelligence, workspace-scoped tools, etc.) shares cleanly + # across client sessions. # - # ~/.amplifier-agent/state/workspaces/-/sessions/... + # ~/.amplifier-agent/state/workspaces//sessions/http-/ # - # Without the header, fall back to the base workspace -- behavior is - # identical to non-correlating clients. The header is purely additive - # and opt-in; client-side adapter repos own the policy of when to - # attach it. amplifier-agent has no opinion on the value's shape beyond - # requiring a non-empty trimmed string. - client_session_id = request.headers.get("X-Client-Session-Id") + # X-Client-Session-Id is authoritative when both headers are present; + # X-Session-Id is the fallback for opencode and other Vercel AI SDK clients. + client_session_id = request.headers.get("X-Client-Session-Id") or request.headers.get("X-Session-Id") client_session_id_clean: str = "" - if client_session_id and base_workspace: - # Strip whitespace, defensively constrain to a safe slug shape. - # Clients are expected to send path-safe IDs. + if client_session_id: client_session_id_clean = client_session_id.strip() - if client_session_id_clean: - workspace = f"{base_workspace}-{client_session_id_clean}" - else: - workspace = base_workspace - else: - workspace = base_workspace + workspace = base_workspace # Determine the amplifier session_id and resume flag for this turn. # When the client provides X-Client-Session-Id, we use it deterministically: @@ -627,9 +620,8 @@ async def chat_completions( sid: str | None is_resumed: bool if client_session_id_clean: - # workspace is guaranteed non-None here: client_session_id_clean is - # only set when client_session_id is present AND base_workspace is - # truthy (see the outer if-condition above). + # client_session_id_clean is only set when client_session_id is + # present and non-empty after stripping whitespace. sid = f"http-{client_session_id_clean}" _ws_root = workspaces_root() _state_dir = _ws_root / str(workspace) / "sessions" / sid diff --git a/tests/http/test_reconciler.py b/tests/http/test_reconciler.py index 4420cd4..9330a26 100644 --- a/tests/http/test_reconciler.py +++ b/tests/http/test_reconciler.py @@ -323,7 +323,8 @@ async def _fake_run(**kwargs: Any) -> str: ) # After turn 2, the store should contain the turn-2 client view (T2). - workspace_slug = f"test-ws-{sid_clean}" + # workspace is NOT suffixed -- stays at base_workspace ("test-ws"). + workspace_slug = "test-ws" store = SessionStore(tmp_path / workspace_slug) loaded = store.load(session_id) assert loaded is not None diff --git a/tests/http/test_session_id_fallback.py b/tests/http/test_session_id_fallback.py new file mode 100644 index 0000000..83ef11f --- /dev/null +++ b/tests/http/test_session_id_fallback.py @@ -0,0 +1,232 @@ +"""Tests for X-Session-Id fallback and no-workspace-suffix behavior. + +Covers: +- X-Session-Id alone (no X-Client-Session-Id): server uses it as the client + session id, creates session_dir at workspaces//sessions/http-/. + Workspace is NOT suffixed. +- Both headers present: X-Client-Session-Id takes precedence (amplifier-native + is authoritative). +- Neither header present: fresh random sid per turn, no resume (legacy behavior + unchanged). +- Workspace path is never suffixed with the client_sid regardless of which + header is used. +""" + +from __future__ import annotations + +from contextlib import asynccontextmanager +from pathlib import Path +from typing import Any +from unittest.mock import MagicMock, patch + +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from amplifier_agent_http.routes import chat_completions as cc_module + +# --------------------------------------------------------------------------- +# Shared helpers (mirrors test_reconciler.py) +# --------------------------------------------------------------------------- + +AUTH = {"Authorization": "Bearer test-key"} + +_REGISTRY = {"claude-3-5-sonnet-20241022": "anthropic"} + + +def _make_test_app( + *, + registry: dict[str, str] | None = None, + workspace: str | None = None, +) -> FastAPI: + """Build a minimal FastAPI app for session-id fallback tests.""" + prepared_mock = MagicMock() + prepared_mock.mount_plan = {} + state_registry = registry or {} + + @asynccontextmanager + async def _noop_lifespan(application: FastAPI): + application.state.config = MagicMock() + application.state.config.model_id = "amplifier" + application.state.config.api_key = "test-key" + application.state.prepared = prepared_mock + application.state.agent_configs = {} + application.state.resolved_workspace = workspace + application.state.host_config = {} + application.state.available_models = [] + application.state.served_models_registry = state_registry + yield + + app = FastAPI(lifespan=_noop_lifespan) + app.include_router(cc_module.router) + return app + + +def _chat_payload(model: str = "claude-3-5-sonnet-20241022", **kwargs: Any) -> dict[str, Any]: + base: dict[str, Any] = { + "model": model, + "messages": [{"role": "user", "content": "hello"}], + } + base.update(kwargs) + return base + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +def test_x_session_id_only_uses_it_as_client_sid(tmp_path: Path) -> None: + """Only X-Session-Id set (no X-Client-Session-Id): server uses it as the + client session id, creating a deterministic ``http-`` and storing the + session at workspaces//sessions/http-/. Workspace is NOT + suffixed.""" + app = _make_test_app(registry=_REGISTRY, workspace="test-ws") + captured: dict[str, Any] = {} + + async def _fake_run(**kwargs: Any) -> str: + captured.update(kwargs) + return "ok" + + opencode_sid = "ses_10dba803effekTqUujLAFyL9me" + + with ( + patch( + "amplifier_agent_http.routes.chat_completions.run_chat_turn", + side_effect=_fake_run, + ), + patch( + "amplifier_agent_http.routes.chat_completions.workspaces_root", + return_value=tmp_path, + ), + TestClient(app, raise_server_exceptions=False) as client, + ): + resp = client.post( + "/v1/chat/completions", + json=_chat_payload(), + headers={**AUTH, "X-Session-Id": opencode_sid}, + ) + + assert resp.status_code == 200 + # session_id must be the deterministic http- prefixed form. + assert captured.get("session_id") == f"http-{opencode_sid}" + # workspace passed to run_chat_turn must be the base workspace, not suffixed. + assert captured.get("workspace") == "test-ws" + + +def test_x_client_session_id_wins_over_x_session_id(tmp_path: Path) -> None: + """Both headers present: X-Client-Session-Id takes precedence. The + amplifier-native header is authoritative when both are sent.""" + app = _make_test_app(registry=_REGISTRY, workspace="test-ws") + captured: dict[str, Any] = {} + + async def _fake_run(**kwargs: Any) -> str: + captured.update(kwargs) + return "ok" + + with ( + patch( + "amplifier_agent_http.routes.chat_completions.run_chat_turn", + side_effect=_fake_run, + ), + patch( + "amplifier_agent_http.routes.chat_completions.workspaces_root", + return_value=tmp_path, + ), + TestClient(app, raise_server_exceptions=False) as client, + ): + resp = client.post( + "/v1/chat/completions", + json=_chat_payload(), + headers={ + **AUTH, + "X-Client-Session-Id": "my-native-sid", + "X-Session-Id": "opencode-sdk-sid", + }, + ) + + assert resp.status_code == 200 + # X-Client-Session-Id (amplifier-native) must win. + assert captured.get("session_id") == "http-my-native-sid" + + +def test_neither_header_set_falls_back_to_random_sid(tmp_path: Path) -> None: + """No session headers: behavior is unchanged from pre-PR -- fresh random + sid per turn (session_id=None from route perspective), no resume. + Workspace is base_workspace.""" + app = _make_test_app(registry=_REGISTRY, workspace="test-ws") + captured_calls: list[dict[str, Any]] = [] + + async def _fake_run(**kwargs: Any) -> str: + captured_calls.append(dict(kwargs)) + return "ok" + + with ( + patch( + "amplifier_agent_http.routes.chat_completions.run_chat_turn", + side_effect=_fake_run, + ), + patch( + "amplifier_agent_http.routes.chat_completions.workspaces_root", + return_value=tmp_path, + ), + TestClient(app, raise_server_exceptions=False) as client, + ): + resp1 = client.post( + "/v1/chat/completions", + json=_chat_payload(), + headers=AUTH, # no session headers + ) + resp2 = client.post( + "/v1/chat/completions", + json=_chat_payload(), + headers=AUTH, + ) + + assert resp1.status_code == 200 + assert resp2.status_code == 200 + # No session id -- runner picks a random sid each time. + assert captured_calls[0]["session_id"] is None + assert captured_calls[0]["is_resumed"] is False + assert captured_calls[1]["session_id"] is None + assert captured_calls[1]["is_resumed"] is False + + +def test_x_session_id_workspace_is_not_suffixed(tmp_path: Path) -> None: + """Specifically assert the workspace path is NOT suffixed with the + client_sid. The session_dir lives at + workspaces//sessions/http-/, NOT + workspaces/-/sessions/http-/.""" + app = _make_test_app(registry=_REGISTRY, workspace="test-ws") + + async def _fake_run(**kwargs: Any) -> str: + return "ok" + + opencode_sid = "ses_abc123" + session_id = f"http-{opencode_sid}" + + with ( + patch( + "amplifier_agent_http.routes.chat_completions.run_chat_turn", + side_effect=_fake_run, + ), + patch( + "amplifier_agent_http.routes.chat_completions.workspaces_root", + return_value=tmp_path, + ), + TestClient(app, raise_server_exceptions=False) as client, + ): + client.post( + "/v1/chat/completions", + json=_chat_payload(), + headers={**AUTH, "X-Session-Id": opencode_sid}, + ) + + # The session_dir must be under the un-suffixed base workspace. + correct_session_dir = tmp_path / "test-ws" / "sessions" / session_id + wrong_workspace_dir = tmp_path / f"test-ws-{opencode_sid}" + + assert correct_session_dir.exists(), ( + f"Expected session_dir at {correct_session_dir} but it does not exist. " + f"tmp_path contents: {list(tmp_path.iterdir())}" + ) + assert not wrong_workspace_dir.exists(), f"Workspace was incorrectly suffixed: {wrong_workspace_dir} exists."