From 9ecb76f038294c1b51af98d345d98c1f8ece1082 Mon Sep 17 00:00:00 2001 From: kl3inIT Date: Thu, 6 Aug 2026 13:49:27 +0700 Subject: [PATCH 1/3] docs: open the assistant conversation memory SSOT increment The debate record decided the persistence boundary: one store, not two. The deciding argument was the defending side's own concession that no meaningful value remains in Spring AI's stock JDBC repository or dialect, which left a permanently project-owned persistence stack holding a projection derivable from the transcript. Measurement retired the drift claim that opened the question. In 546 conversations the model window equals LEAST(transcript, 20) in 514, every remaining difference but one is a failed or model-free turn, and no row exists in memory that is absent from the transcript. The second store is not diverging; it is a pure function of the first, which is why it earns nothing. The winning position won conditionally, so the design records six binding constraints rather than the shape originally proposed: no no-op add(), explicit turn identity, in-flight USER excluded by construction, clear() not delegated to the domain delete, snap-forward counted as a cost rather than a gain, and the drift claim withdrawn. Composer attachment, Knowledge Base format coverage and the unwired multimodal pipeline go to the backlog. Attachment is a permission boundary question, not a UI change, and the multimodal pipeline cannot run while image upload is refused. Co-Authored-By: Claude Opus 5 --- .../challenge-brief.md | 133 ++++++++ .../challenge-verdict.md | 293 ++++++++++++++++++ .../design.md | 174 +++++++++++ .../plan.md | 43 +++ docs/roadmap.md | 19 ++ 5 files changed, 662 insertions(+) create mode 100644 docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-brief.md create mode 100644 docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-verdict.md create mode 100644 docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/design.md create mode 100644 docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md diff --git a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-brief.md b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-brief.md new file mode 100644 index 00000000..0c27e7c0 --- /dev/null +++ b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-brief.md @@ -0,0 +1,133 @@ +# Debate Brief — Chat Transcript SSOT + +You are one of two architects debating a material persistence-boundary decision +in the OrgMemory repository. Read this file in full, inspect the repository +evidence yourself, then write your response. + +## Hard constraints + +- **Read-only.** Do not edit, create, or delete any repository file. Do not run + migrations, mutate any database, or change git state. +- Inspect the repo freely to check the claims below. Do not trust this brief; + verify it. If a claim here is wrong, say so with the file and line. +- Your response goes into the debate record file you are told to append to. + Plain Markdown. No tools-only output, no truncation. + +## The question + +OrgMemory stores the same assistant conversation in two Postgres tables. Should +it collapse to one store, or keep two and fix the weaker one? + +## Current state (verify these) + +**Table A — `assistant_conversation_messages`** (`core/src/main/resources/db/migration/V6__assistant_conversation_history.sql`) +- Columns include `organization_id`, `actor_user_id`, `role`, `content`, + `sequence_id bigint GENERATED ALWAYS AS IDENTITY`, `version`. +- `CHECK (role IN ('USER','ASSISTANT'))`, `CHECK (length(content) BETWEEN 1 AND 200000)`. +- Foreign keys to `app_users` and to `assistant_conversations`, `ON DELETE CASCADE`. +- Written by `AssistantConversationService` (`core/src/main/java/com/orgmemory/core/assistant/AssistantConversationService.java`): + the USER row when a turn starts, the ASSISTANT row in `completeTurn(...)`, + which returns early when the answer is blank. +- Read by `AssistantConversationService.history(...)`, which serves + `GET /api/assistant/conversations/{conversationId}/messages`. +- Also carries answer citations. + +**Table B — `spring_ai_chat_memory`** (same migration file, top) +- Columns: `conversation_id varchar(36)`, `content text`, `type varchar(10)`, + `timestamp`, `sequence_id bigint`. **No `organization_id`, no `actor_user_id`, + no foreign key.** +- `CHECK (type IN ('USER','ASSISTANT','SYSTEM','TOOL'))`. +- Written by Spring AI's `MessageChatMemoryAdvisor` through the `ChatMemory` + bean built in `apps/api/src/main/java/com/orgmemory/api/assistant/AssistantConfiguration.java` + as `MessageWindowChatMemory.builder().maxMessages(20).build()`, wrapped in + `ObservedChatMemory`. +- `MessageWindowChatMemory` trims to its window on write and `saveAll` replaces + the conversation's row set, so rows beyond the window are physically deleted. +- Consumed only via the `ChatMemory` interface at + `integrations/ai-model-gateways/src/main/java/com/orgmemory/integrations/ai/gateway/SpringAiChatModelAdapter.java` + (`MessageChatMemoryAdvisor.builder(chatMemory).build()`, ~line 277 and again + in `assistantMemoryClient`). + +**Consistency today.** `apps/api/src/main/java/com/orgmemory/api/assistant/AssistantController.java` +(~line 379) deletes a conversation by calling `conversations.delete(actor, conversationId)` +and then `memory.clear(conversationId.toString())` — two stores, two calls, not +one transaction, orchestrated in the delivery layer. + +## Measured evidence from the production deployment (2026-08-06) + +Production data is test-only; there are no real customers and no backfill +obligation. + +- `assistant_conversation_messages`: 1148 rows / 539 conversations. Longest + conversation 56 messages. +- `spring_ai_chat_memory`: 1085 rows / 519 conversations. Longest 20 — exactly + the window cap, confirming trimming is live. +- **20 conversations have zero rows in `spring_ai_chat_memory`** while their + transcript exists in table A. +- Message type distribution in `spring_ai_chat_memory`: `USER` 591, + `ASSISTANT` 494. **No `SYSTEM` and no `TOOL` rows exist**, despite the schema + allowing them. + +## Position A — collapse to one store + +Implement `ChatMemory` over `assistant_conversation_messages`. Drop +`MessageWindowChatMemory`, `ChatMemoryRepository`, and the +`spring_ai_chat_memory` table. Keep the `ChatMemory` *interface*, because +`SpringAiChatModelAdapter` depends on it. + +Proposed shape: +- `add()` → no-op, documented: `AssistantConversationService` is the sole writer + and is the only caller holding `organizationId`, `actorUserId` and citations, + which `ChatMemory.add(String, List)` does not receive. +- `get()` → windowed read ordered by `sequence_id`, snapped forward to the + nearest `USER` message so the window never begins on an assistant reply whose + prompt was cut off. +- `clear()` → delegate to the conversation service delete. + +Claimed benefits: drift becomes structurally impossible; model memory enters the +tenancy model for the first time; deletion becomes one domain transaction; the +destructive `saveAll` disappears. + +## Position B — keep two stores, fix the weaker one + +Keep the separation the migration comment declares intentional +(`-- Spring AI ChatMemory is the bounded context sent back to the model. The +complete product transcript is stored separately below.`). Bring +`spring_ai_chat_memory` into the tenancy model instead: add `organization_id`, +foreign keys and cascade, and move deletion out of the controller into a single +domain transaction. + +Claimed benefits: the model window and the product transcript are genuinely +different concerns with different lifecycles and retention; keeping Spring AI's +own repository preserves upstream compatibility and avoids an interface +implementation whose `add()` violates its contract by doing nothing. + +## Points each side must engage with + +1. Is a no-op `add()` an acceptable implementation of `ChatMemory`, or a + contract violation that will mislead the next maintainer? What happens if a + future Spring AI upgrade, or another advisor, calls `add()` and expects + persistence? +2. Does the absence of `SYSTEM`/`TOOL` rows today prove table A can hold + everything the model window needs, or is it an artifact of the current + feature set that tool-calling will invalidate? Note the Skill tool loop in + `apps/api/.../AssistantConfiguration.java` and `AssistantSkillToolCallbacks`. +3. `completeTurn(...)` returns early on a blank answer, so a failed turn leaves + a USER row with no ASSISTANT row. Under each position, what does the model + window see on the next turn, and is the snap-forward read sufficient? +4. What explains the 20 conversations with zero rows in + `spring_ai_chat_memory`, and does either position prevent that class of + divergence or merely relabel it? +5. Retention and privacy: `spring_ai_chat_memory` holds message content with no + tenant column. Under Position B, is adding `organization_id` sufficient, or + does the second writer remain the actual risk? +6. Cost and reversibility: which position is cheaper to undo if wrong? + +## Required output shape + +1. **Position** — which architecture you defend, in one sentence. +2. **Evidence** — concrete file paths and line references supporting it. +3. **Attacks** — specific, evidence-backed attacks on the opposing position. + Attack the position, not a strawman of it. +4. **Concessions** — what the other side is genuinely right about. +5. **Falsifier** — what fact, if true, would change your mind. diff --git a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-verdict.md b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-verdict.md new file mode 100644 index 00000000..7061821c --- /dev/null +++ b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/challenge-verdict.md @@ -0,0 +1,293 @@ +# Verdict — Chat Transcript SSOT + +Judge: independent, non-participant. Decided on the debate record alone +(brief, both round-1 files, the round-2 instruction file with the moderator's +measured evidence, and both round-2 files). No repository inspection was +performed and no claim was independently verified. + +--- + +## 1. Decision + +**Collapse to one store: `assistant_conversation_messages` becomes the sole +persisted store for both the product transcript and the model context, and +`spring_ai_chat_memory` is dropped — Position A wins, subject to the binding +constraints in §4.** + +--- + +## 2. Rationale + +### 2.1 Position B's own round-2 reframing dissolves Position B + +B's round 2 does not defend the architecture B argued in round 1. It states: + +> "**No meaningful value remains in Spring AI's stock JDBC repository or +> dialect.** A custom repository should replace them; implementing both a custom +> repository and a custom dialect would be redundant because the upstream binder +> cannot supply the new tenant values. **That part of A's attack succeeds.**" + +and concludes: + +> "Keep two stores, but **stop calling Table B an upstream-owned store. It is a +> project-governed, tenant-linked, disposable projection** of completed +> model-memory lifecycle." + +Round 1 rested Position B on two pillars: a genuine semantic boundary, and +upstream compatibility. The second pillar is explicitly surrendered. What +remains is a self-described *disposable projection* — B's word — of table A, +requiring a project-owned repository, a project-owned dialect-equivalent, a +project-owned transaction wrapper, a project-owned lock, a key-type change, two +new columns, a composite FK, a backfill of untenanted rows, and two new +integration test classes. That is a large, permanently owned persistence surface +whose entire purpose is to hold a derivable projection. A projection with no +independent source of truth belongs in a query, not in a table. + +The residual upstream value B enumerates is honest and correctly scoped — +"the `ChatMemory` SPI", "`MessageWindowChatMemory`'s bounded-window algorithm", +"`MessageChatMemoryAdvisor`'s prompt integration", "Spring AI `Message` types" — +but B itself labels it: "That is **algorithm and integration reuse, not +persistence reuse**." Algorithm reuse does not require a second table. It is +precisely what A proposed to keep as a read policy. + +### 2.2 The moderator's measurement is the load-bearing fact, and it cuts for A + +The measurement refuted part of the brief (drift is ~0.5%, not endemic) and +refuted B's round-1 causal hypothesis outright. B conceded this: "I withdraw my +round-1 explanation of the measured zero-memory conversations." + +The decisive shape of that data is not the divergence count but its *absence*: + +> `memory_rows = LEAST(transcript_rows, 20)` — 514/546 conversations; +> `memory_rows > transcript_rows` — **0**. + +A store whose population is reproducible by a bounded expression over another +store, with no row the other store lacks, is a cache. B was entitled to argue +that the split was a boundary; the data says it is a projection. A framed this +correctly: "I would have accepted 'the two stores drift' as evidence for A; what +I actually got is stronger: the second store is a pure function of the first." + +Critically, B accepted the acceptance condition *in round 1*, before seeing the +data: + +> "If the desired product rule is ultimately 'model context is always a pure +> deterministic window over committed product transcript rows,' a second physical +> store would be unjustified." + +The measurement then showed it already is that, in 514 of 546 conversations, with +the remainder explained by `memory = transcript − failed_turns`. B's round-2 +attempt to keep the boundary alive rests on reinterpreting the residue — +"model-invocation admission and failed-turn omission are the stronger +persistence-boundary argument." But admission and omission are *selection +predicates*. A's completed-turn window rule (round 2, Q4, rule 2: "Drop every +USER row that is not immediately followed — in `sequence_id` order — by an +ASSISTANT row") implements exactly that selection as a read policy. B named a +filter and called it a boundary. + +### 2.3 B's round-2 change inventory concedes the architecture while disputing the cost + +The most telling artifact in the record is B's own "Viable A" inventory. It +proposes: + +> "Add `.../TranscriptContextAdvisor.java`: read completed transcript turns +> before the prompt but never call `ChatMemory.add`; this avoids A's impossible +> verified-duplicate timing." + +That is a complete, workable single-store design — authored by the losing side. +B's remaining disagreement is therefore not about *whether* one store can serve +both roles; B has designed the mechanism by which it does. B's disagreement is +about A's *cost estimate* ("A can be smaller only by accepting incorrect pairing +and the broken guarded ASSISTANT add"). On that narrower point B is right, and +§4 makes it binding. But a cost correction is not a defeat of the architecture. + +### 2.4 Tenancy and deletion decide the tie-break that remains + +Both sides agree table B holds message content with no tenant, no actor, and no +foreign key, deleted by a second non-atomic call from the delivery layer. B +agrees the fix requires forking vendor persistence. Under A, the content lives in +exactly one table already inside the tenancy model, and deletion becomes the +owned parent delete plus `ON DELETE CASCADE` — A's round 2: "`AssistantController` +line 380 removed; FK cascade only." B reaches the same deletion end-state, but +only after building the FK, the key conversion, the backfill and the lock first. +Where two designs converge on the same invariant, the one that reaches it by +deleting a table beats the one that reaches it by building a parallel persistence +stack. + +--- + +## 3. The rejected alternative, in its strongest form + +Position B, at its best, is this: + +*The boundary is not transcript-vs-copy; it is **what the product recorded** +versus **what the model was actually given**. Those are different facts about the +world. A turn that errored, was cancelled, or never invoked a model is a real +event in the transcript and a non-event in the model's context. The measurement +proves the distinction is live and stable: the residue is not noise, it is exactly +`transcript − failed_turns`, i.e. the model-invocation lifecycle faithfully +recorded. Materializing that lifecycle gives you an auditable record of what the +model saw, independent of inference; it lets model context have its own retention +and reset clock; and it leaves room for future model-only state (tool results, +durable system context) that table A's `CHECK (role IN ('USER','ASSISTANT'))` +forbids. Collapsing replaces a recorded fact with a derived one, and derivations +silently change whenever anyone edits the query.* + +**What would have made it win.** Any one of: + +1. A written retention or erasure requirement with two genuinely different clocks + — A conceded twice that this is "the only argument I could not answer with a + read policy over table A." +2. A concrete, near-term requirement for durable SYSTEM or TOOL content in the + model window. Both sides agreed the current absence is structural, so this + needed to be a stated product requirement, not a possibility. +3. A demonstration that "what the model saw" must be independently auditable — + e.g. a compliance or evaluation requirement — rather than reconstructible. +4. A second `ChatMemory` consumer outside the assistant module, which would make + the coupling of the model window to `AssistantConversationService` a real cost. + +B produced none of these. B's round-2 case reduces to concurrency safety and +turn-identification — both of which are implementation defects in A, correctable +without a second table, and both of which are now binding constraints below. + +--- + +## 4. Binding constraints — what Position A conceded, which the implementation MUST honor + +These are not footnotes. A won on the boundary and lost on much of the execution. +The design that ships is A's boundary with B's corrections. + +**C1 — The no-op `add()` is dead, and so is the "guarded duplicate" `add()`.** +A withdrew the no-op in round 1 ("A no-op `add()` is genuinely bad, and B is +right to name it"). B then killed the replacement in round 2, unrefuted, because +A's round 2 was written in parallel and never answered it: + +> "Spring AI invokes the advisor's after-write from the aggregation callback … +> while OrgMemory writes the transcript answer only later in the controller's +> downstream `doOnComplete`. A's guarded ASSISTANT add therefore **either rejects +> every healthy completion or writes before the alleged canonical writer.**" + +This attack stands. The implementation must therefore **not** retain +`MessageChatMemoryAdvisor`'s write path. Adopt B's own proposal: a project-owned, +**read-only** context advisor that assembles the bounded window before the prompt +and never calls `ChatMemory.add`. If a `ChatMemory` bean is retained at all, its +`add` must be an explicit, telemetered rejection — never a silent no-op, and +never a "verify it was already written" check whose precondition cannot hold. + +**C2 — Turn identity must be explicit; role + `sequence_id` is not sufficient.** +B's unrefuted attack: + +> "Table A has role and a global sequence but no `turn_id` or completion status … +> `beginTurn` and `completeTurn` are separate transactions … Concurrent calls can +> therefore produce `U1,U2,A2,A1`. Neither 'drop a trailing USER' nor 'pair each +> USER with the next ASSISTANT' identifies the true turns." + +A half-conceded this by making it falsifier #4 and by admitting the inference "is +sound only because `beginTurn` and `completeTurn` are the sole writers … A +reviewer should treat that as an invariant to guard." Sole-writer-ness does not +imply serialized ordering, so the concession does not cover the concurrency case. +Ship a `turn_id` (per B's `V24` sketch: nullable, backfilled only for unambiguous +adjacent legacy pairs, with partial uniqueness for one USER and one ASSISTANT per +turn), or prove a one-turn-in-flight invariant with a test that reproduces +`U1,U2,A2,A1`. A's "~10 lines" estimate is withdrawn by this constraint. + +**C3 — `clear()` must not delete the transcript and must not require ambient +identity.** A withdrew the brief's shape entirely: "`clear()` as written in the +brief is wrong, and I withdraw it entirely." The replacement is the +`memory_reset_sequence_id` watermark on `assistant_conversations`, with `get()` +reading only `sequence_id > memory_reset_sequence_id`. That watermark column is +part of the migration, not optional. Any implementation requiring a `CurrentActor` +inside `clear(String)` is out of contract and must fail test T-C2. + +**C4 — The current in-flight USER must be excluded structurally, not by timing +accident.** A conceded the duplicate-USER defect in full ("B's mechanism is +exactly right"). The window contract must exclude any USER row lacking an +ASSISTANT successor, so exclusion follows from the query definition rather than +from write ordering. Test T-G2 (assert the captured prompt is exactly +`[System, U1, A1, U2]`, U2 appearing once) is the gate. + +**C5 — Snap-forward is not a benefit of collapsing.** A conceded in round 1 that +Spring AI 2.0.0 already snaps the window head to a USER message. Retain the +behavior for parity; claim no credit for it. + +**C6 — Drift was never the case for A, and must not be cited as one.** A +retracted the "~97 orphaned USER rows in live model windows" attack: "those 97 +orphaned USER rows are present in table A too — the imbalance is a property of the +turn lifecycle, not of the second store." Orphan replay is a shared read-policy +defect; C4's rule fixes it under either architecture. The consolidation write-up +must not claim collapsing cured a drift problem that measurement showed was +~0.5%. + +**C7 — Cost honesty.** A conceded "'smaller patch' is not the same as 'cheaper to +undo'", and B's inventory is the better cost estimate. Plan for a migration +(turn_id + watermark + drop, with the drop split into a separate migration for +rolling deployment as B proposes), service and controller changes to carry the +turn id, a new context advisor, adapter rewiring, dependency and +`application.yml` cleanup, and the full test set — not "one migration, one +~70-line class." + +**C8 — Deletion via cascade, one command.** Both sides converged: the second +`memory.clear` call in the controller disappears; owned parent delete plus +`ON DELETE CASCADE` on the conversation FK is the whole deletion path. Note A's +own round-2 correction that only the conversation FK cascades — the `app_users` FK +does not — so every cascade claim must route through the conversation FK. + +**C9 — Record the decision.** A's unrebutted point that `docs/decisions/` holds +nothing on this boundary, and that the split rests on a two-line SQL comment, +applies equally to the new state. This debate is the required architecture +challenge; it must be written up as a decision, with B's position recorded as the +rejected alternative in the form given in §3. + +--- + +## 5. Open questions the record did NOT settle + +1. **The ~3 unexplained conversations.** Two incompatible mechanisms were + proposed and neither was tested. A argues the moderator's refutation is a + measurement artifact, because the no-evidence sentences were rewritten on + 2026-08-05 by `145a27b6`, one day before measurement, so "essentially the + entire production dataset predates the current wording," and predicts all three + match the pre-`145a27b6` string. B proposes a lost whole-window update from two + concurrent `add` calls racing `saveAll`'s delete-then-insert, and explicitly + declines to explain the two `2/1/0` cases: "Plausible classes are a later stale + replacement/clear, an older deployed code/configuration, or external/manual + mutation. The counts alone cannot distinguish them." **The record is + insufficient here.** Run A's proposed query (re-match the three conversations, + including the historical no-evidence wording, and note that the `6/3/4` case was + never in the tested population at all) before implementation. If the match + fails, an unidentified writer of ASSISTANT transcript rows exists and must be + found first — under a single store that writer becomes a correctness problem, + not a reconciliation problem. +2. **Concurrency semantics of the winning design.** B's lost-update mechanism was + argued against table B, but the analogous question for table A — what happens + when two turns of one conversation overlap — is unanswered by either side. C2 + makes turn identity mandatory; it does not define the intended behavior. Decide + explicitly: reject concurrent turns, serialize them, or define a deterministic + window under interleaving. +3. **What replaces `MessageChatMemoryAdvisor`'s streaming aggregation.** B listed + "streaming aggregation" among the upstream value worth preserving. C1 removes + the advisor's write path. Whether any aggregation behavior is lost, and whether + a read-only advisor can sit at the same order relative to `ToolCallingAdvisor`, + is unexamined in the record. +4. **SYSTEM and TOOL persistence.** Both sides agree the current absence is + structural (vendor filtering plus advisor nesting at order 200 vs 300), and + both agree neither architecture supports durable tool state today. Neither + established whether it will be needed. A conceded the need "is real and A does + not get it for free" — costing a role CHECK migration. Left open. +5. **Retention.** No written requirement for a separate model-context retention + clock was produced by either side, and A named it as the one argument it could + not answer. If such a requirement exists anywhere in product or compliance + scope, it should be surfaced before the drop migration, not after. +6. **Invisible memory reset.** A's own falsifier #5: the watermark makes a reset + inferable from the transcript. If the product requires a model-context reset + that leaves no trace visible to a user or admin, C3's design does not provide + it and needs revisiting. +7. **Backfill and legacy pairing.** B's inventory requires deciding what happens + to legacy rows whose turns cannot be unambiguously paired ("leave ambiguous + legacy rows visible in transcript but ineligible for model context"). The "no + backfill obligation" premise came from the brief and was flagged by B as not + repository-verifiable; confirm it against the migration conventions before + relying on it. +8. **Guarding the writer invariant.** A proposes an ArchUnit or module-visibility + test to prevent a third writer of `assistant_conversation_messages`. With + `turn_id` (C2) the inference is less fragile, but whether the guard is still + required was not settled. diff --git a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/design.md b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/design.md new file mode 100644 index 00000000..0040076d --- /dev/null +++ b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/design.md @@ -0,0 +1,174 @@ +# Assistant Conversation Memory SSOT + +## Intent + +Make one table the single persisted home of an Assistant conversation, and make +a failed turn say why it failed to the person who hit it. Today the same +conversation lives in two tables written by two different writers, and a turn +that fails tells the user only "The assistant stream failed." + +## Observed problem + +### One conversation, two stores + +`assistant_conversation_messages` (`V6__assistant_conversation_history.sql:43`) +holds the product transcript with `organization_id`, `actor_user_id`, foreign +keys to `app_users` and `assistant_conversations`, and a generated +`sequence_id`. It serves `GET /api/assistant/conversations/{id}/messages`. + +`spring_ai_chat_memory` (same migration, line 3) holds the model window. It has +`conversation_id varchar(36)`, `content`, `type`, `timestamp`, `sequence_id` — +**no tenant column, no actor column, no foreign key** — while holding raw +message content. It is written by Spring AI's `MessageChatMemoryAdvisor` through +`MessageWindowChatMemory.builder().maxMessages(20)` +(`AssistantConfiguration.java:55`). + +Consistency between them is maintained by `AssistantController.java:379-380` +calling `conversations.delete(...)` and then `memory.clear(...)` — two stores, +two calls, outside one transaction, orchestrated in the delivery layer. Any +future deletion path that forgets the second call leaves message content in a +table that cannot be filtered by organization to find it again. + +### A failed turn is mute + +`UiMessageStream.Encoder.error()` emits the fixed string +`"The assistant stream failed."` for every failure. A rate-limited gateway, an +expired credential, a model that no longer exists on the gateway, and a broken +deployment are indistinguishable to the person who has to decide what to do +next. + +Northstar solved exactly this in `AssistantStreamFailures.java`, which reads only +the HTTP status off the failure and returns a fixed sentence per status. Its +javadoc names the same before-state OrgMemory is still in: *"a model that cannot +serve this chat was indistinguishable from a broken deployment."* + +## Evidence that shaped the decision + +### The two stores are not drifting + +Measured against the deployment on 2026-08-06, 546 conversations: + +| Shape | Conversations | +| --- | --- | +| `memory = LEAST(transcript, 20)` — exactly the window rule | 514 | +| Explained by a failed turn or a model-free no-evidence turn | 31 | +| Unexplained | 1 | +| `memory > transcript` | 0 | + +The original framing of "20 orphaned conversations" as drift was wrong. Those +are turns where the model was never invoked: the transcript correctly holds the +question (and, for a no-evidence turn, the static answer), and the model window +correctly holds nothing. Both stores agree. + +This measurement did not weaken the case for collapsing — it changed the reason. +The second store is not diverging; it is a **pure function** of the first plus +"was the model invoked". That is precisely the condition under which a second +physical store earns nothing. + +### Nothing model-only exists to protect + +The `CHECK (type IN ('USER','ASSISTANT','SYSTEM','TOOL'))` on +`spring_ai_chat_memory` is partly dead schema: + +- `JdbcChatMemoryRepository.saveAll` (spring-ai 2.0.0, lines 112-116) filters out + `ToolResponseMessage` and tool-calling `AssistantMessage`, and logs a warning + when it does. +- `MessageChatMemoryAdvisor` sits at `HIGHEST_PRECEDENCE + 200` + (`Advisor.DEFAULT_CHAT_MEMORY_PRECEDENCE_ORDER`) while `ToolCallingAdvisor` is + at `HIGHEST_PRECEDENCE + 300`, so the tool loop runs *inside* the memory + advisor and tool messages never reach `ChatMemory` at all. + +Production confirms it: `USER` 591, `ASSISTANT` 494, `SYSTEM` 0, `TOOL` 0. +Permission-scoped grounding is rebuilt into the request-local system message on +every turn by design, so it must not be persisted either. + +## Decision + +Collapse to one store. `assistant_conversation_messages` becomes the sole +persisted home of both the product transcript and the model context; +`spring_ai_chat_memory`, `MessageWindowChatMemory` and `ChatMemoryRepository` +are dropped. + +This was decided by an independent two-architect debate rather than by the +proposer. The full record is in `challenge-brief.md` and `challenge-verdict.md`. +The deciding argument was the defending side's own round-2 concession: *"No +meaningful value remains in Spring AI's stock JDBC repository or dialect… that +part of A's attack succeeds"*, leaving a permanently project-owned persistence +stack holding a derivable projection. + +### Rejected alternative + +Keep two stores and bring `spring_ai_chat_memory` into the tenancy model: change +`conversation_id` to UUID, add `organization_id` and `actor_user_id`, add a +composite foreign key with cascade, and serialize the read-trim-replace cycle +under a parent lock. + +Its strongest form is that the model-invocation boundary is a real semantic +distinction, observable in the data, that lets model memory carry its own +retention and reset lifecycle independent of the product transcript. It would +have won on evidence of **model-only state that cannot be derived from the +transcript**. Both sides agreed no such state exists today: the vendor filters +tool messages out, and system grounding is deliberately request-local. + +It would also have required replacing the stock repository and dialect anyway — +`PostgresChatMemoryRepositoryDialect` binds only the five upstream columns and +`ChatMemory.add(String, List)` receives no tenant — so the upstream +compatibility that justified the split does not survive its own fix. + +## Binding constraints from the debate + +The winning position won **conditionally**. These are design requirements, not +notes: + +1. **No no-op `add()`.** The originally proposed shape is withdrawn. Spring AI + writes the assistant message from the aggregation callback + (`MessageChatMemoryAdvisor:136-162`) while OrgMemory writes the transcript + answer later in the controller's `doOnComplete` + (`AssistantController.java:419-436`). A guarded `add()` would therefore either + reject every healthy completion or write ahead of the canonical writer. + Replace `MessageChatMemoryAdvisor` with a project-owned **read-only** context + advisor that never calls `ChatMemory.add`. +2. **Explicit turn identity is required.** `assistant_conversation_messages` has + `role` and a global `sequence_id` but no `turn_id`, and `beginTurn` / + `completeTurn` are separate transactions. Concurrent turns can persist as + `U1,U2,A2,A1`, which no ordering heuristic pairs correctly. Add `turn_id`. +3. **The current in-flight USER must be excluded by construction.** `beginTurn` + commits the USER row before the model call and the adapter also passes the + question as `.user(...)` (`SpringAiChatModelAdapter.java:202-213`), so a naive + read would send it twice. Select completed turns only. +4. **`clear()` is not delegated to the domain delete.** `ChatMemory.clear(String)` + carries no actor while the domain delete requires `CurrentActor`. Deletion + stays at the owned controller boundary and relies on the existing cascade; + the second `memory.clear` call is removed. +5. **Snap-forward is not a gain of this change.** `MessageWindowChatMemory:113-119` + already snaps the retained window forward to a USER message. Collapsing the + stores means OrgMemory must now implement that behavior itself rather than + inherit it — it is a cost, not a benefit. +6. **The drift claim is withdrawn** and must not appear in the spec. + +## Scope + +In scope: + +- One persisted conversation store with explicit turn identity. +- A project-owned read-only transcript context advisor replacing + `MessageChatMemoryAdvisor`. +- Status-mapped failure sentences for a failed turn, ported from Northstar's + `AssistantStreamFailures` shape. + +Out of scope, deliberately: + +- Assistant composer file attachment, Knowledge Base format coverage, and wiring + the built-but-unwired LightRAG multimodal pipeline. Recorded in the roadmap + Engineering Backlog; attachment in particular is a permission-boundary question + needing its own challenge, not a UI change. + +## Open questions the record did not settle + +- The one genuinely unexplained conversation (`transcript=6, assistant=3, + memory=4`) has a proposed mechanism — two concurrent `add` calls overwriting + one another through the read-then-replace cycle — but no proven diagnosis. + Collapsing removes the class, so this is not a blocker; it is not an + explanation either. +- Whether legacy rows without `turn_id` stay eligible for model context. The + safe default is transcript-visible but context-ineligible. diff --git a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md new file mode 100644 index 00000000..46199a77 --- /dev/null +++ b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md @@ -0,0 +1,43 @@ +# Assistant Conversation Memory SSOT Plan + +- [x] Measure the real relationship between the two stores against the + deployment; retire the incorrect drift claim. +- [x] Complete the independent architecture challenge as a two-architect debate + with a no-tools judge; record brief and verdict. +- [ ] Port status-mapped failure sentences so a failed turn names its cause. + Independent of the store change and shippable on its own. +- [ ] Add `turn_id` to `assistant_conversation_messages` with partial uniqueness + for one USER and one ASSISTANT per turn; leave legacy rows nullable, + transcript-visible and context-ineligible. +- [ ] Carry the turn id through `beginTurn` and `completeTurn` so the pair is + written against one identity rather than inferred from sequence order. +- [ ] Add a project-owned read-only transcript context advisor that selects the + last completed turns, excludes the in-flight USER by construction, and snaps + the window forward to a USER boundary. +- [ ] Replace `MessageChatMemoryAdvisor` with that advisor in both memory client + paths; remove the `ChatMemory` bean, `ObservedChatMemory`, the JDBC memory + starter and its schema-initialization setting. +- [ ] Remove the second `memory.clear` call from conversation deletion and let + the existing cascade do the work. +- [ ] Drop `spring_ai_chat_memory` in a migration separate from the one that + adds `turn_id`, so a rolling deployment never runs the new reader against a + dropped table or the old writer against a missing one. +- [ ] Cover: turn pairing under `U1,U2,A2,A1` concurrency, exclusion of the + in-flight USER, failed and cancelled turns, model-free no-evidence turns, + legacy null-`turn_id` rows, window bound of 20, deletion cascade, and one + status-mapped sentence per failure class. +- [ ] Reconcile the Assistant spec and test matrix, record the persistence + decision with its rejected alternative, and refresh `Source:`/`Reconciled:`. +- [ ] Run `:core:test`, `:apps:api:test`, lint, typecheck, production build, and + browser verification of a failed turn's message. + +## Sequencing note + +The failure-sentence port is deliberately first and separate: it is the only +item that changes what a user sees today, it touches no schema, and it completes +the diagnosability work already shipped in `f38a5357` (which made failures +attributable to operators but left them mute to users). + +The store collapse is gated behind turn identity. Without `turn_id` the reader +cannot pair turns correctly under concurrency, and shipping the reader first +would replace a working window with a subtly wrong one. diff --git a/docs/roadmap.md b/docs/roadmap.md index a19cefb1..d5f6802a 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -54,6 +54,7 @@ The table is a delivery index, not a second description of current behavior. | Increment | Status | Remaining gate | | --- | --- | --- | +| [Assistant conversation memory SSOT](increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md) | active | one persisted conversation store with explicit turn identity, a read-only transcript context advisor, and a failed turn that names its cause | | [Assistant Skill activity receipt](increments/completed/2026-08-06-assistant-skill-activity-receipt/verification.md) | shipped | keeps waiting through the first rendered token and exposes only bounded successful Skill activity in a current-turn receipt | | [Agentic Skill beta](increments/completed/2026-08-05-agentic-skill-beta/verification.md) | shipped | delivered actor-scoped progressive Skill disclosure, a bounded read-only Assistant tool loop, and truthful Skill activity without server-side package execution | | [Knowledge workspace and document reader](increments/completed/2026-08-05-knowledge-workspace-document-reader/verification.md) | shipped | completed the governed right-side reader, safe Markdown presentation, truthful access copy, and cross-format browser coverage | @@ -111,6 +112,24 @@ implementation-active until their predecessor exit gates pass. ## Engineering Backlog +- Decide whether a file attached at the Assistant composer may become evidence. + The composer has no attachment path at all today: `prompt-input.tsx` submits + `files: []` and `AssistantPage` forwards only `message.text`. Copying + Northstar's per-turn attachment flow would bypass the permission model that + makes an OrgMemory answer citable, so the question is a permission boundary — + who owns the file, who may see it, and whether it is turn-local or durable + evidence — and needs its own architecture challenge, not a UI change. +- Widen Knowledge Base ingestion coverage. `KnowledgeContentType` allows upload + of PDF, DOCX, PPTX, MD and TXT only; `SpringAiDocumentParser.ALLOWED_MEDIA_TYPES` + matches. Spreadsheets (XLSX/XLS/CSV), HTML, JSON/XML and legacy Office are + unsupported, and every image type is explicitly `uploadAllowed=false`. Adding a + format is not just an allowlist entry: each needs a parser, a chunking shape + and a browser-safe delivery type. +- Wire or retire the LightRAG multimodal pipeline. `graph-rag-core/multimodal` + ships 22 classes covering IMAGE, TABLE and EQUATION with a + `SpringAiMultimodalAnalyzer` adapter, but `MultimodalProcessor` has no caller + outside its own package and the analyzer is never constructed. It cannot run + while image upload is disallowed, so the two decisions are coupled. - Fence Source Ingestion with a never-reused claim epoch and an exact durable Asset-publication permit, then prove manifest-pinned recovery before exposing a manual FAILED retry. The rejected proposal and required test matrix are in From 36fc409c1f39c2dc4bb370144fef2cd14f363e34 Mon Sep 17 00:00:00 2001 From: kl3inIT Date: Thu, 6 Aug 2026 13:58:59 +0700 Subject: [PATCH 2/3] feat(assistant): let a failed turn name what the caller can do Every failure ended on the fixed frame "The assistant stream failed.", so an expired gateway credential, a rate limit, a retired model and a broken deployment were indistinguishable to the only person able to act on them. f38a5357 made a failure attributable to whoever operates the deployment; this makes it actionable to whoever is sitting in front of it. Ported from Northstar's AssistantStreamFailures, whose javadoc names the same before-state. Two departures. Saturation is read from the bounded failureCode on AssistantUnavailableException rather than a status, because the retrieval scheduler rejects the turn before any gateway is contacted and so it never had one. Administrator-facing advice does not name a Settings screen the ordinary actor may not reach. Every returned sentence stays a fixed string with nothing interpolated from the failure, which is why this reads a status rather than a message: a chatty gateway must not be able to echo a key or a prompt fragment into a browser. An unrecognized failure still ends opaque. Co-Authored-By: Claude Opus 5 --- .../assistant/AssistantStreamFailures.java | 84 +++++++++++++++++++ .../api/assistant/UiMessageStream.java | 8 +- .../AssistantStreamFailuresTests.java | 83 ++++++++++++++++++ .../api/assistant/UiMessageStreamTests.java | 21 +++++ .../plan.md | 2 +- docs/specs/domains/assistant-and-mcp.md | 10 +++ docs/tests/domains/assistant-and-mcp.md | 4 + 7 files changed, 208 insertions(+), 4 deletions(-) create mode 100644 apps/api/src/main/java/com/orgmemory/api/assistant/AssistantStreamFailures.java create mode 100644 apps/api/src/test/java/com/orgmemory/api/assistant/AssistantStreamFailuresTests.java diff --git a/apps/api/src/main/java/com/orgmemory/api/assistant/AssistantStreamFailures.java b/apps/api/src/main/java/com/orgmemory/api/assistant/AssistantStreamFailures.java new file mode 100644 index 00000000..2e0b2697 --- /dev/null +++ b/apps/api/src/main/java/com/orgmemory/api/assistant/AssistantStreamFailures.java @@ -0,0 +1,84 @@ +package com.orgmemory.api.assistant; + +import com.orgmemory.core.assistant.AssistantUnavailableException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +/** + * Turns a failed turn into one short sentence the person who hit it can act on. + * + *

A failed turn used to end as the fixed frame {@link #GENERIC}, so an expired gateway + * credential, a rate limit, a retired model and a broken deployment were indistinguishable to the + * only person in a position to do something about them. {@code failure_code} and the {@code WARN} + * line make a failure attributable to whoever operates the deployment; this makes it actionable to + * whoever is sitting in front of it. + * + *

Every returned sentence is a fixed string. Nothing is interpolated from the + * failure, so a chatty or misconfigured gateway can never echo a key, a prompt fragment or provider + * internals into the browser. That constraint is the reason this reads a status rather than a + * message. + * + *

Saturation is read from the bounded {@code failureCode} carried on + * {@link AssistantUnavailableException} rather than from a status, because it never had one: the + * retrieval scheduler rejects the turn before any gateway is contacted. Everything else is keyed on + * the leading HTTP status that clients put on the message ({@code "400: ..."} from the OpenAI SDK, + * {@code "404 Not Found: ..."} from Spring) rather than on provider exception types, because the + * provider SDK belongs to the AI integration module and the delivery layer only needs the status. + */ +final class AssistantStreamFailures { + + static final String GENERIC = "The assistant stream failed."; + + static final String BUSY = + "The assistant is busy right now. Send the message again in a moment."; + + /** A leading three-digit status, as HTTP client exceptions render it. */ + private static final Pattern STATUS_PREFIX = Pattern.compile("^\\s*([45]\\d{2})(?::|\\s)"); + + private static final int MAX_CAUSE_DEPTH = 10; + + private AssistantStreamFailures() { + } + + static String describe(Throwable error) { + Throwable cause = error; + for (int depth = 0; cause != null && depth < MAX_CAUSE_DEPTH; depth++) { + if (cause instanceof AssistantUnavailableException unavailable + && AssistantRetrievalScheduler.REJECTED.equals(unavailable.failureCode())) { + return BUSY; + } + int status = status(cause.getMessage()); + if (status > 0) { + return forStatus(status); + } + cause = cause.getCause() == cause ? null : cause.getCause(); + } + return GENERIC; + } + + private static int status(String message) { + if (message == null) { + return 0; + } + Matcher matcher = STATUS_PREFIX.matcher(message); + return matcher.find() ? Integer.parseInt(matcher.group(1)) : 0; + } + + static String forStatus(int status) { + return switch (status) { + case 400, 422 -> "The selected model rejected this request. " + + "Pick a different chat model and send it again."; + case 401, 403 -> "The AI gateway rejected its credentials. " + + "Ask an administrator to update its key."; + case 404 -> "The selected model is no longer available on this gateway. " + + "Pick another model."; + case 408, 504 -> "The AI gateway did not answer in time. Send the message again."; + case 429 -> "The AI gateway is rate limiting requests. " + + "Send the message again in a moment."; + default -> status >= 500 + ? "The AI gateway failed while answering. Send the message again." + : "The AI gateway rejected this request. " + + "Ask an administrator to check the model and gateway."; + }; + } +} diff --git a/apps/api/src/main/java/com/orgmemory/api/assistant/UiMessageStream.java b/apps/api/src/main/java/com/orgmemory/api/assistant/UiMessageStream.java index 5975c3e9..4b22cccb 100644 --- a/apps/api/src/main/java/com/orgmemory/api/assistant/UiMessageStream.java +++ b/apps/api/src/main/java/com/orgmemory/api/assistant/UiMessageStream.java @@ -32,7 +32,9 @@ static Flux> encode( Flux.just(encoder.finish(), encoder.done())) .onErrorResume(AssistantStreamAbortedException.class, error -> Flux.just(encoder.abort(error.getMessage()), encoder.done())) - .onErrorResume(ignored -> Flux.just(encoder.error(), encoder.done())); + .onErrorResume(error -> Flux.just( + encoder.error(AssistantStreamFailures.describe(error)), + encoder.done())); }); } @@ -81,9 +83,9 @@ ServerSentEvent finish() { return event(json.writeValueAsString(fields("type", "finish", "finishReason", "stop"))); } - ServerSentEvent error() { + ServerSentEvent error(String errorText) { return event(json.writeValueAsString( - fields("type", "error", "errorText", "The assistant stream failed."))); + fields("type", "error", "errorText", errorText))); } ServerSentEvent abort(String reason) { diff --git a/apps/api/src/test/java/com/orgmemory/api/assistant/AssistantStreamFailuresTests.java b/apps/api/src/test/java/com/orgmemory/api/assistant/AssistantStreamFailuresTests.java new file mode 100644 index 00000000..ca0f5617 --- /dev/null +++ b/apps/api/src/test/java/com/orgmemory/api/assistant/AssistantStreamFailuresTests.java @@ -0,0 +1,83 @@ +package com.orgmemory.api.assistant; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.orgmemory.core.assistant.AssistantUnavailableException; +import java.util.concurrent.RejectedExecutionException; +import org.junit.jupiter.api.Test; + +/** + * The sentence a failed turn ends on is the only thing the person who hit it can act on, and it is + * also the last place a gateway's own words could reach a browser. These hold both ends: the + * failure has to be named, and naming it must not quote anything the failure said. + */ +class AssistantStreamFailuresTests { + + @Test + void namesSaturationFromTheFailureCodeRatherThanAStatus() { + // The retrieval scheduler rejects before any gateway is contacted, so this failure never + // had a status to read. Without the code it would fall through to the generic sentence and + // tell a user to do nothing while the correct advice is to wait a moment. + AssistantUnavailableException rejected = new AssistantUnavailableException( + "The assistant is temporarily busy", + new RejectedExecutionException("queue full"), + AssistantRetrievalScheduler.REJECTED); + + assertThat(AssistantStreamFailures.describe(rejected)) + .isEqualTo(AssistantStreamFailures.BUSY); + } + + @Test + void distinguishesCredentialFailureFromRateLimitFromRetiredModel() { + assertThat(AssistantStreamFailures.describe(new IllegalStateException("401: no key"))) + .contains("credentials"); + assertThat(AssistantStreamFailures.describe(new IllegalStateException("429 Too Many"))) + .contains("rate limiting"); + assertThat(AssistantStreamFailures.describe(new IllegalStateException("404 Not Found: x"))) + .contains("no longer available"); + } + + @Test + void readsTheStatusThroughAWrappedCause() { + Throwable wrapped = new IllegalStateException( + "assistant failed", + new IllegalStateException("503: upstream down")); + + assertThat(AssistantStreamFailures.describe(wrapped)) + .isEqualTo(AssistantStreamFailures.forStatus(503)); + } + + @Test + void fallsBackToTheGenericSentenceWhenNothingIsRecognizable() { + assertThat(AssistantStreamFailures.describe(new IllegalStateException("provider secret"))) + .isEqualTo(AssistantStreamFailures.GENERIC); + } + + /** + * A self-referential cause is not hypothetical: exception plumbing that re-wraps its own cause + * produces one, and an unguarded walk would spin forever inside a streaming response. + */ + @Test + void terminatesOnASelfReferentialCauseChain() { + Throwable looping = new IllegalStateException("no status here") { + @Override + public synchronized Throwable getCause() { + return this; + } + }; + + assertThat(AssistantStreamFailures.describe(looping)) + .isEqualTo(AssistantStreamFailures.GENERIC); + } + + @Test + void neverQuotesTheFailureItDescribes() { + String untrustedFailureDetail = "provider diagnostic containing a prompt fragment"; + + assertThat(AssistantStreamFailures.describe( + new IllegalStateException( + "500: " + untrustedFailureDetail, new RuntimeException(untrustedFailureDetail)))) + .doesNotContain(untrustedFailureDetail) + .doesNotContain("prompt fragment"); + } +} diff --git a/apps/api/src/test/java/com/orgmemory/api/assistant/UiMessageStreamTests.java b/apps/api/src/test/java/com/orgmemory/api/assistant/UiMessageStreamTests.java index 182e707c..32b155a9 100644 --- a/apps/api/src/test/java/com/orgmemory/api/assistant/UiMessageStreamTests.java +++ b/apps/api/src/test/java/com/orgmemory/api/assistant/UiMessageStreamTests.java @@ -132,4 +132,25 @@ void providerFailureEmitsOpaqueErrorAndDone() { assertThat(data.getFirst()).contains("\"type\":\"start\""); assertThat(data).allMatch(frame -> !frame.contains("provider secret")); } + + /** + * The opaque frame above is the floor, not the contract. A failure that carries a status has to + * reach the browser as the sentence for that status, or the encoder is silently discarding the + * only actionable thing the failure knew. + */ + @Test + void aRecognizedFailureReachesTheBrowserAsItsOwnSentence() { + List data = UiMessageStream.encode( + Flux.error(new IllegalStateException("429: slow down", null)), + MESSAGE_ID, + json, + Duration.ofHours(1), + Duration.ofMinutes(1)) + .map(ServerSentEvent::data) + .collectList() + .block(); + + assertThat(data).anyMatch(frame -> frame.contains("rate limiting")); + assertThat(data).allMatch(frame -> !frame.contains("slow down")); + } } diff --git a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md index 46199a77..356392c3 100644 --- a/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md +++ b/docs/increments/active/2026-08-06-assistant-conversation-memory-ssot/plan.md @@ -4,7 +4,7 @@ deployment; retire the incorrect drift claim. - [x] Complete the independent architecture challenge as a two-architect debate with a no-tools judge; record brief and verdict. -- [ ] Port status-mapped failure sentences so a failed turn names its cause. +- [x] Port status-mapped failure sentences so a failed turn names its cause. Independent of the store change and shippable on its own. - [ ] Add `turn_id` to `assistant_conversation_messages` with partial uniqueness for one USER and one ASSISTANT per turn; leave legacy rows nullable, diff --git a/docs/specs/domains/assistant-and-mcp.md b/docs/specs/domains/assistant-and-mcp.md index 000a1402..325665b6 100644 --- a/docs/specs/domains/assistant-and-mcp.md +++ b/docs/specs/domains/assistant-and-mcp.md @@ -73,6 +73,16 @@ safe as a meter tag. An unavailable turn is also logged at `WARN` with its failure code and cause: it is a `BusinessException`, which the API layer answers without logging, so nothing else records it. +A failed turn ends on a sentence naming what the caller can do about it, not a +single opaque frame. Retrieval-scheduler saturation is read from the bounded +failure code carried on the exception, because that failure is raised before any +gateway is contacted and never had a status; every other case is keyed on the +leading HTTP status the client puts on the message, so the delivery layer never +depends on provider exception types. Every returned sentence is a fixed string +and nothing is interpolated from the failure, so a chatty or misconfigured +gateway cannot echo a key, a prompt fragment, or provider internals into the +browser. An unrecognized failure still ends on the opaque generic sentence. + Blocking permission-scoped retrieval runs on an Assistant-owned fixed scheduler with configured concurrency, a finite queue, sanitized overload rejection, and bounded shutdown. The server begins the UI message stream before scheduling diff --git a/docs/tests/domains/assistant-and-mcp.md b/docs/tests/domains/assistant-and-mcp.md index c0074400..77454299 100644 --- a/docs/tests/domains/assistant-and-mcp.md +++ b/docs/tests/domains/assistant-and-mcp.md @@ -74,6 +74,10 @@ Reconciled: `2026-08-06-assistant-skill-activity-receipt (64221f86)`. | An unavailable turn publishes why, and an answered turn publishes `none`, as a low-cardinality tag | `DefaultAssistantTurnObservationConventionTests` | covered | | The failure code cannot smuggle free text onto a meter tag | `DefaultAssistantTurnObservationConventionTests#carriesNoFreeTextOnTheLowCardinalitySurface`, `AssistantTurnEventTests` | covered | | An unavailable turn is logged at `WARN` with its failure code and cause | none | gap — asserted only by reading `AssistantService`; no test pins the log | +| A failed turn ends on a sentence naming the cause: saturation from the failure code, everything else from the HTTP status | `AssistantStreamFailuresTests#namesSaturationFromTheFailureCodeRatherThanAStatus`, `#distinguishesCredentialFailureFromRateLimitFromRetiredModel`, `#readsTheStatusThroughAWrappedCause` | covered | +| A recognized failure reaches the browser as its own sentence rather than the opaque frame | `UiMessageStreamTests#aRecognizedFailureReachesTheBrowserAsItsOwnSentence` | covered | +| No failure sentence quotes the failure it describes, and an unrecognized failure stays opaque | `AssistantStreamFailuresTests#neverQuotesTheFailureItDescribes`, `#fallsBackToTheGenericSentenceWhenNothingIsRecognizable`, `UiMessageStreamTests#providerFailureEmitsOpaqueErrorAndDone` | covered | +| A self-referential cause chain terminates instead of spinning inside a streaming response | `AssistantStreamFailuresTests#terminatesOnASelfReferentialCauseChain` | covered | | Time to first token stops at the first token, not the last | `AssistantTurnObservationTests#stopsAtTheFirstTokenRatherThanTheLast` | covered | | A turn that emitted nothing records no latency sample | `AssistantTurnObservationTests#recordsNothingWhenNoTokenEverReachedTheCaller`, `#recordsNothingWhenThereWasNoAccessibleEvidenceToAnswerFrom` | covered | | Assistant meters carry no tenant, request or conversation identifier | `AssistantTurnObservationTests#carriesNoIdentifierThatWouldGrowASeriesPerTenantOrRequest` | covered | From f010d9aa2824341826b36c2b08c9660ebcc0519e Mon Sep 17 00:00:00 2001 From: kl3inIT Date: Thu, 6 Aug 2026 14:32:23 +0700 Subject: [PATCH 3/3] chore(release): add failure sentence changelog entry The change alters what a user reads when a turn fails, so it is product-impacting and the release gate requires an entry. Patch rather than minor: no new capability, an existing message becomes specific. Co-Authored-By: Claude Opus 5 --- .../2026-08-06-assistant-failure-sentences.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 .tegami/2026-08-06-assistant-failure-sentences.md diff --git a/.tegami/2026-08-06-assistant-failure-sentences.md b/.tegami/2026-08-06-assistant-failure-sentences.md new file mode 100644 index 00000000..9318318e --- /dev/null +++ b/.tegami/2026-08-06-assistant-failure-sentences.md @@ -0,0 +1,18 @@ +--- +packages: + orgmemory: patch +subject: Tell people what to do when an Assistant turn fails +--- + +## Fixes + +A failed Assistant turn now ends on a sentence naming what the person who hit it +can do next, instead of one generic message for every cause. An expired gateway +key, a rate limit, a model that is no longer offered, a gateway that did not +answer in time, and a busy assistant are now distinguishable and separately +actionable. + +Every message remains a fixed sentence chosen from the failure's category, so a +misconfigured or unusually talkative AI gateway cannot surface its own text, +credentials, or prompt content in the browser. A failure that matches no known +category still ends on the previous generic message.