fix(chat): give every chat tool failure the shared error shape

Every chat tool failure now carries an HttpError's code and details the
same way. The list_workspaces and get_guide runner previously dropped
both, while its own remediation text told the model to read details.

All three chat runners now build details with
createPublicHttpErrorDetails, the sanitizer MCP and REST already use, so
every surface carries the same value and only the field path differs.
This also stops a server-generated id collision from ever handing the
model another workspace's id.

It also brings the documentation up to date with the review-tools
change: the chat now serves all seven registry tools, the reviewId
decision and the new REVIEW_ID_CARD_MISMATCH code are documented in
docs/conversational-reviews.md, and docs/agent-tool-surfaces.md,
docs/fsrs-scheduling-logic.md and README.md no longer say the chat has
no review tools.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: kirill-markin

README.md

@@ -22,7 +22,7 @@ Card scheduling uses FSRS-based spaced repetition. Detailed scheduling rules liv
 
 The hosted MCP server is available at `https://mcp.flashcards-open-source-app.com/mcp` and is listed in MCP registries as `com.flashcards-open-source-app/flashcards`. It exposes seven tools, most of them workspace-scoped: `list_workspaces`, `sql_query`, `sql_execute`, `get_guide`, `next_review_card`, `reveal_answer`, and `submit_review`.
 
-The dedicated review tools support one-question-at-a-time conversations and idempotent FSRS scheduling. See [conversational reviews](docs/conversational-reviews.md) for the MCP/Agent API contract and voice-session examples. ChatGPT Voice currently does not invoke apps/MCP; these tools do not remove that external limitation.
+The dedicated review tools support one-question-at-a-time conversations and idempotent FSRS scheduling. See [conversational reviews](docs/conversational-reviews.md) for the contract shared by MCP, the in-app chat, and the Agent API, and for voice-session examples. ChatGPT Voice currently does not invoke apps/MCP; these tools do not remove that external limitation.
 
 Interactive clients authenticate with OAuth 2.1 authorization code + PKCE and Dynamic Client Registration. Headless clients can use an `fca_` Bearer token.
 

apps/backend/src/chat/openai/tools/tools.ts

@@ -9,7 +9,7 @@ import {
 } from "../../../database/transient";
 import { GeneratedMediaPromotionStorageTransientError } from "../../../mediaAssets/storage";
 import { resolveAccessibleChatWorkspaceId } from "../../../server/requestContext";
-import { HttpError } from "../../../shared/errors";
+import { createPublicHttpErrorDetails, HttpError } from "../../../shared/errors";
 import {
   ensureAIChatSyncReplica,
   ensureAIChatSyncReplicaWithDeadline,
@@ -452,7 +452,7 @@ function toComparableKeyList(keys: ReadonlyArray<string>): string {
  * optional `workspaceId` - would otherwise reach the chat model as something else: an added
  * optional argument would be invisible to it, and an added required one would fail every call
  * inside the spec's own parse against a schema the model was never shown. Property types stay
- * unguarded; the one enum that could drift is spread from `GUIDE_TOPICS`.
+ * unguarded; the two enums that could drift are spread from `GUIDE_TOPICS` and `REVIEW_RATINGS`.
  */
 function requireChatFunctionTool(spec: AgentToolSpec): OpenAI.Responses.FunctionTool {
   const functionTool = CHAT_FUNCTION_TOOLS[spec.name];
@@ -927,7 +927,7 @@ async function executeSqlChatToolCall(
         error: serializeToolError(error),
         instructions,
         code: error.code ?? undefined,
-        details: error.details ?? undefined,
+        details: createPublicHttpErrorDetails(error.details) ?? undefined,
       }
       : {
         sql,
@@ -959,8 +959,9 @@ async function executeSqlChatToolCall(
 
 /**
  * A failure, including arguments the schema rejects, comes back as the same `{ ok: false }`
- * envelope a failed SQL call returns rather than as a throw, because a thrown tool call ends the
- * run: the model repairs its call and continues on its own remediation instructions instead.
+ * envelope a failed SQL call returns, carrying an `HttpError`'s `code` and `details`, rather than
+ * as a throw, because a thrown tool call ends the run: the model repairs its call and continues on
+ * its own remediation instructions instead.
  */
 async function executeReadOnlyChatToolCall<Data>(
   spec: AgentToolSpec<Data>,
@@ -988,15 +989,25 @@ async function executeReadOnlyChatToolCall<Data>(
       toolErrorClass: null,
     };
   } catch (error) {
-    return {
-      output: createToolErrorResult(spec.name, {
+    const instructions = createAgentRemediationInstructions(
+      error instanceof HttpError ? error.code : null,
+      getChatToolFailureStatusCode(error),
+      { surface: "chat", toolName: spec.name },
+    );
+    const payload: ToolErrorPayload = error instanceof HttpError
+      ? {
         error: serializeToolError(error),
-        instructions: createAgentRemediationInstructions(
-          error instanceof HttpError ? error.code : null,
-          getChatToolFailureStatusCode(error),
-          { surface: "chat", toolName: spec.name },
-        ),
-      }),
+        instructions,
+        code: error.code ?? undefined,
+        details: createPublicHttpErrorDetails(error.details) ?? undefined,
+      }
+      : {
+        error: serializeToolError(error),
+        instructions,
+      };
+
+    return {
+      output: createToolErrorResult(spec.name, payload),
       isMutating: false,
       succeeded: false,
       shouldInvalidateMainContent: false,
@@ -1070,7 +1081,7 @@ async function executeReviewChatToolCall(
         error: serializeToolError(error),
         instructions,
         code: error.code ?? undefined,
-        details: error.details ?? undefined,
+        details: createPublicHttpErrorDetails(error.details) ?? undefined,
       }
       : {
         error: serializeToolError(error),

docs/agent-tool-surfaces.md

@@ -20,9 +20,9 @@ registers the specs that list its own surface
 | `sql_execute` | MCP, chat |
 | `list_workspaces` | MCP, chat |
 | `get_guide` | MCP, chat |
-| `next_review_card` | MCP |
-| `reveal_answer` | MCP |
-| `submit_review` | MCP |
+| `next_review_card` | MCP, chat |
+| `reveal_answer` | MCP, chat |
+| `submit_review` | MCP, chat |
 | `add_generated_image_to_card` | chat only, not a registry spec |
 
 Every workspace-scoped registry spec takes the same optional `workspaceId`
@@ -46,13 +46,16 @@ the MCP gateway's 29-second integration timeout
 (`infra/aws/lib/gateways/mcp-gateway.ts`). That is one dated sample rather than a
 standing contract; re-measure before deciding.
 
-The chat deliberately has no review tools. A review needs one durable `reviewId`
-per learner review, scoped to the authenticated connection, and the chat's
-connection id is the constant `"chat-v2"`
-(`apps/backend/src/chat/openai/tools/tools.ts`), which cannot scope one the way
-an API key connection does. Until that identity is decided, the chat calls the
-SQL, workspace, and guide tools only. The review contract itself is
-[conversational reviews](conversational-reviews.md).
+The chat serves all seven registry tools, review included. The model supplies
+the `reviewId` there exactly as it does on MCP, and a `reviewId` reused on a
+different card is refused on every surface with its own code instead of passing
+as a retry (`submitAgentReview` in `apps/backend/src/agent/reviews.ts`). What
+the surfaces do not share is the sync replica that forms half of that dedup key:
+the chat authenticates as no agent connection, so a review is stored against the
+workspace's shared AI-chat replica (`buildChatAgentToolContext` in
+`apps/backend/src/chat/openai/tools/tools.ts`), which makes one `reviewId`
+namespace cover every member and every session of that workspace. The review
+contract itself is [conversational reviews](conversational-reviews.md).
 
 ## REST is not a registry surface
 
@@ -82,7 +85,16 @@ result envelope, and output budget, and each of those stays in its own adapter:
   failing it (`apps/backend/src/chat/openai/tools/toolResults.ts`). Its envelope is
   `{ ok, tool, data, instructions }` with no `docs` block, and a failure comes
   back as `{ ok: false }` rather than as a throw, because a thrown tool call
-  ends the run.
+  ends the run. That failure carries an `HttpError`'s `code` and `details` at
+  the envelope's top level, beside an `error` of just `name` and `message`
+  (`createToolErrorResult`, same module), where MCP and REST nest both under the
+  agent envelope's `error` (`createAgentErrorEnvelope` in
+  `apps/backend/src/agent/envelope.ts`). Only `code` and `details` move with
+  that, and `details` moves unchanged, because all three surfaces build it with
+  `createPublicHttpErrorDetails` (`apps/backend/src/shared/errors.ts`); `ok`,
+  `instructions`, and `error.message` are read at the same path on either shape,
+  which is why a remediation meaning can be reworded for the chat over the path
+  alone.
 - REST: the same agent envelope as MCP, built per route against the request URL
   (`apps/backend/src/routes/agent.ts`).
 
@@ -94,7 +106,8 @@ Two invariants are load-bearing and invisible unless you look for them:
   workspace.
 - Every action a surface's tools do not reach is bound to
   `unboundAgentToolAction` (`apps/backend/src/aiTools/toolRegistry/actions.ts`).
-  Each adapter names all of them, so omitting one is a type error and a tool
+  Both adapters bind every action for real today, so nothing reaches it, but each
+  adapter still names all of them, so omitting one is a type error and a tool
   added to a surface later cannot quietly reach production code around that
   surface's dependencies and its tests' fakes.
 
@@ -134,11 +147,11 @@ The per-code map and the full remediation text live in the remediation module,
 which is keyed by HTTP error code. Four facts are surface-specific enough to
 record here:
 
-- `WORKSPACE_NOT_FOUND` (404) is reachable from the chat's SQL tools when the
-  model passes a `workspaceId` the account cannot access: the chat's workspace
-  resolution reaches `assertUserHasWorkspaceAccess`
-  (`apps/backend/src/workspaces/selection.ts`) before any SQL runs. It is the
-  only meaning the chat alone words.
+- `WORKSPACE_NOT_FOUND` (404) is reachable from every workspace-scoped chat tool
+  when the model passes a `workspaceId` the account cannot access: the chat's
+  workspace resolution reaches `assertUserHasWorkspaceAccess`
+  (`apps/backend/src/workspaces/selection.ts`) before the tool's own work runs.
+  It is the only meaning the chat alone words.
 - `WORKSPACE_SELECTION_REQUIRED` (409) is reachable on REST and MCP but never on
   the chat, whose tool context always passes the session workspace as the
   selected default, so its resolver never sees none (`buildChatAgentToolContext`

docs/conversational-reviews.md

@@ -1,10 +1,11 @@
-# Conversational reviews over MCP and the Agent API
+# Conversational reviews over MCP, the in-app chat, and the Agent API
 
-An MCP-capable voice client can review one question at a time, assess the learner's
-answer, explain any gaps, choose a rating, and persist the next FSRS schedule
-without asking the learner to rate every card. The calling agent performs the
-assessment; the backend accepts its rating and schedules the review. No SQL writes
-to review history or hidden scheduler columns are allowed.
+An MCP-capable voice client, the in-app chat, or an Agent API caller can review
+one question at a time, assess the learner's answer, explain any gaps, choose a
+rating, and persist the next FSRS schedule without asking the learner to rate
+every card. The calling agent performs the assessment; the backend accepts its
+rating and schedules the review. No SQL writes to review history or hidden
+scheduler columns are allowed.
 
 **External limitation:** ChatGPT Voice currently does not invoke apps/MCP. These
 server tools do not remove that OpenAI product limitation. They support clients
@@ -14,12 +15,11 @@ microphone, speech recognition, speech synthesis, or a ChatGPT Voice integration
 
 ## Tools and HTTP actions
 
-The three review tools are the only ones MCP serves alone; `list_workspaces`,
-`sql_query`, `sql_execute`, and `get_guide` are shared with the in-app chat,
-which has no review tools ([agent tool surfaces](agent-tool-surfaces.md)).
-`get_guide` topic `review_flow` returns this review contract.
+MCP and the in-app chat serve the same registry tools, the three review tools
+included ([agent tool surfaces](agent-tool-surfaces.md)). `get_guide` topic
+`review_flow` returns this review contract.
 
-| MCP tool | Agent API action | Result in `data` | Effect |
+| Tool | Agent API action | Result in `data` | Effect |
 | --- | --- | --- | --- |
 | `next_review_card` | `POST /v1/agent/reviews/next` | `workspaceId`, `card: {cardId, frontText}` or `card: null` | Read only |
 | `reveal_answer` | `POST /v1/agent/reviews/reveal` | `workspaceId`, `cardId`, `backText` | Read only |
@@ -29,10 +29,13 @@ The optional `workspaceId` argument and the rejection of unknown arguments are
 shared agent-tool rules, described once in
 [agent tool surfaces](agent-tool-surfaces.md). An HTTP request with no body at
 all is a valid call. `reveal_answer` requires `cardId`. HTTP actions use
-`Authorization: ApiKey <fca_...>` and the same JSON arguments as the MCP tools.
-MCP continues to accept OAuth authorization or an API key as a Bearer token.
-Authentication and current workspace membership are checked on each request. IDs
-are UUIDs.
+`Authorization: ApiKey <fca_...>` and the same JSON arguments the tools take.
+MCP continues to accept OAuth authorization or an API key as a Bearer token. The
+chat has no API key of its own: it runs under the chat session's transport,
+including a native guest session (`assertSupportedTransport` in
+`apps/backend/src/chat/http/dependencies.ts`), and nothing on the review path
+gates on being signed in. Authentication and current workspace membership are
+checked on each request. IDs are UUIDs.
 
 ## Choosing the next card
 
@@ -122,8 +125,10 @@ values remain 0–3. What each rating means, how to judge an equivalent or
 ambiguous answer, when to honor a learner's explicit rating, and that “perfectly
 remembered” is only a spoken alias for `Easy`, are all one constant:
 `REVIEW_FLOW_INSTRUCTIONS` in `apps/backend/src/agent/reviewContract.ts`, which
-every review tool returns in full with each result and `get_guide` topic
-`review_flow` serves on demand.
+MCP and the HTTP actions return in full with each result and `get_guide` topic
+`review_flow` serves on demand. A chat review result points the model at that
+topic instead of carrying the constant (`CHAT_REVIEW_RESULT_INSTRUCTIONS` in
+`apps/backend/src/chat/openai/tools/tools.ts`).
 
 ## Result, retries, and offline behavior
 
@@ -136,18 +141,27 @@ day interval and can be zero for a minutes-long learning step. `state` is
 writable through this contract.
 
 - Generate and durably retain one `reviewId` UUID per learner review, scoped to
-  the authenticated connection and workspace. Keep the same connection when
-  recovering an uncertain request. Reconnecting with a different connection or
-  changing the ID is a new review, not a retry.
+  the workspace and to the calling surface's own sync replica
+  ([agent tool surfaces](agent-tool-surfaces.md)): on MCP and the HTTP actions
+  that replica is the agent connection, so keep the same connection when
+  recovering an uncertain request and treat a different one as a new review; on
+  the chat it is shared, so a `reviewId` must be unique across the whole
+  workspace. Changing the ID is a new review, not a retry.
 - A repeated submission is deduplicated by the
   `UNIQUE (workspace_id, replica_id, client_event_id)` constraint on
   `content.review_events`, so it can never record a second review or advance the
-  schedule again. A retry whose review already landed answers
-  `409 REVIEW_EVENT_CONFLICT` with the card's current schedule in
-  `error.details.reviewSchedule` (`cardId`, `dueAt`, `intervalSeconds`,
-  `scheduledDays`, `state`, `reps`, `lapses`). The same code answers an
-  unrelated pre-existing event identity, which likewise cannot advance
-  scheduling. What the calling agent should do about it is in
+  schedule again. Retrying the same card answers `409 REVIEW_EVENT_CONFLICT`
+  with that card's current schedule in the failure's `reviewSchedule` details
+  (`cardId`, `dueAt`, `intervalSeconds`, `scheduledDays`, `state`, `reps`,
+  `lapses`), wherever that surface's envelope carries a failure's `details`
+  ([agent tool surfaces](agent-tool-surfaces.md)); on the chat, whose replica is
+  shared, two members or two sessions of one workspace that chose the same
+  `reviewId` for that card reach the same code without either retrying. Retrying
+  that card is still the only supported reuse of a `reviewId`.
+- `409 REVIEW_ID_CARD_MISMATCH` means the `reviewId` already identifies a
+  recorded review of a different card, so nothing was stored for the card just
+  submitted and no schedule advanced. Generate a new `reviewId` for it and
+  submit again. What the calling agent should do about either code is in
   `REVIEW_FLOW_INSTRUCTIONS`.
 - `409 REVIEW_STALE` means the card's stored `fsrs_last_reviewed_at` is at or after
   the current server time, so the scheduler cannot move forward from it. Only server
@@ -162,9 +176,10 @@ writable through this contract.
 
 ## Implementation and verification
 
-`apps/backend/src/agent/reviewContract.ts` shares strict schemas across MCP and
-HTTP. `apps/backend/src/agent/reviews.ts` uses the existing agent replica identity
-and delegates to `cards/review/reviews.ts::submitReviewInExecutor`. The scheduler
+`apps/backend/src/agent/reviewContract.ts` shares strict schemas across MCP, the
+chat, and HTTP. `apps/backend/src/agent/reviews.ts` takes the sync replica from
+its caller rather than choosing one, and delegates to
+`cards/review/reviews.ts::submitReviewInExecutor`. The scheduler
 algorithm is unchanged. One workspace-locked transaction inserts the review,
 updates FSRS state, and records progress/activity facts and hot sync metadata;
 review history continues through its append-only sequence, and post-commit
@@ -173,8 +188,11 @@ permission was added, and this work adds no table of its own.
 
 Run backend `npm test`, `npm run lint`, `npm run test:mcp`, and
 `npm run test:postgres-integration` with an isolated PostgreSQL 18 administrative
-URL in `POSTGRES_INTEGRATION_ADMIN_URL`. The deployment smoke script also checks
-the tool inventory.
+URL in `POSTGRES_INTEGRATION_ADMIN_URL`. The deployment smoke script
+`scripts/checks/check-mcp-smoke.sh` checks the tool inventory and then runs one
+real review over MCP with no model in the loop: next, reveal, submit, and a
+repeat of that submission, which must answer `REVIEW_EVENT_CONFLICT` with the
+stored schedule.
 
 For a manual voice smoke check after deployment, follow the session above with
 one disposable card for each rating. Verify that the agent explains gaps,

docs/fsrs-scheduling-logic.md

@@ -405,9 +405,9 @@ Any scheduler change must update:
 ## Dedicated agent review adapter
 
 The [conversational review contract](conversational-reviews.md) exposes the existing
-scheduler through MCP `submit_review` and HTTP `POST /v1/agent/reviews/submit`, and
-selects its next card with the queue order above. It maps exact
-`Again`/`Hard`/`Good`/`Easy` strings to 0–3, stamps the review instant on the server
-because the surface is online only, and returns the due time, interval, state, reps,
-and lapses. It does not change the scheduler algorithm or released first-party sync
-contracts.
+scheduler through the MCP and in-app chat `submit_review` tools and HTTP
+`POST /v1/agent/reviews/submit`, and selects its next card with the queue order above.
+It maps exact `Again`/`Hard`/`Good`/`Easy` strings to 0–3, stamps the review instant
+on the server because the surface is online only, and returns the due time, interval,
+state, reps, and lapses. It does not change the scheduler algorithm or released
+first-party sync contracts.