Do not report fallback chat completion when saving fails

[Maya-Kid]

Latest dev synchronization (2026-09-21)

Based on dev@474b904dc3e8014d42404a3c067af9906036a953; current head 73f5442f50cc871415aa6be90df7d85b1ef71150.

Revalidated the combined integration tree 6cd976351b4d7cc7389730fce530526f4d197992 on isolated Linux PostgreSQL fixtures (all 675 tracked source files hash-matched):

• Rust fmt, strict workspace/all-target Clippy, workspace build: pass. Workspace tests: 1,015 passed, 0 failed, 5 ignored. The new upstream delivery tests were additionally run on an exclusive database as CONTRIBUTING requires: both parent delivery/process-kill tests and the busy-deferral unit test passed. The crash-child entry point ran through its parent, not directly; the existing public-network RSS test remains ignored.
• Combined frontend: 129 module tests, 27 browser subtests (29 including parent suites); build/typecheck/guard passed. Browser fixtures ran serially.
• Five browser scenarios against the actual Linux server and PostgreSQL passed: 65-conversation pagination, stale view ownership, citation preview/reattachment/reload, history-to-idle handoff, and actual assistant INSERT failure. Model replies were locally scripted; this is not a new paid-model accuracy evaluation.
• The single PostgreSQL Chat CI command, cargo test -p utopia-server api::chat::, listed and executed 47 tests. All 37 introduced tests were observed passing: Hand off exhausted tool runs to an evidence-only final answer #845 22, Keep older chat handles from unregistering newer generations #848 1, Publish citation sources added by document reads #849 2, Keep fallback retrieval failures distinct from empty results #850 4, Preserve exact chat outcomes across stream closure and reattachment #851 3, Do not report fallback chat completion when saving fails #852 3, and upstream Assert a chat turn ends in exactly one earned terminal #858 2. No test module was deleted during CI consolidation.
• Seven real filtered runs each omitted one group. Cargo returned green, but the independent execution-coverage checker returned exit 1 for every omission; the complete run returned exit 0. All four shared terminal-contract table rows executed without pending skips.

The evidence-only handoff core remains frozen. Earlier validation below belongs to its stated historical heads. Automated browser checks do not replace the maintainer's requested visual review.

---

The legacy RAG fallback ignores an assistant-message save failure and emits done after streaming the answer. It can report success although the answer is absent from history.

Check the append result before done. On failure, retain the streamed content, log the diagnostic server-side and emit a safe error stating that saving could not be confirmed. Do not retry generation or persistence.

Linux/PostgreSQL validation uses real Chat producers, a local provider rejecting tools and a fixture-scoped trigger rejecting assistant inserts:

• Both connected and disconnected-client regressions fail before the fix by reporting done after the rejected INSERT. Both now pass; the disconnected producer unregisters itself.
• The normal commit control is readable when done is observed. All three regressions pass; exactly two tool-negotiation requests plus one plain answer occur, without a save retry.
• The public error excludes the private trigger diagnostic. Formatting and strict workspace clippy pass. CI runs the regressions with the required-database guard.

Only fallback persistence changes here. The agent path's save-before-publish behavior is already handled by #845 and is not duplicated. No migration, outbox, data repair, paid model or business data is involved.

Additional integration validation: isolated tree f7bcd0878bf490d6efb122b71d6f13d379df4709 combines this change with #845 and the other scoped Chat fixes. Linux Rust fmt, strict workspace Clippy, workspace build and 1,013 tests pass (one pre-existing external-HTTPS RSS test ignored); frontend 129 module tests, 27 real-component browser subtests and build pass. Database/PDF test guards were enabled. This combined result is separate from the standalone checks above.

Chrome against the actual Linux backend/PostgreSQL also exercised fallback after tool rejection with an assistant-only INSERT trigger failure. The generated text remains visible with the safe save-confirmation error, no assistant row is stored, internal diagnostics stay out of the page, and #851 does not turn EOF into success.

[WaylandYang]

Coordination note covering this PR and its siblings — #845, #848, #849, #850, #852 — since they land on the same lines.

I trial-merged all six chat PRs onto ea0557ba in order:

OK        #845
CONFLICT  #848  .github/workflows/ci.yml  chat_empty_reply_tests.rs
CONFLICT  #849  .github/workflows/ci.yml  chat.rs  chat_empty_reply_tests.rs
CONFLICT  #850  .github/workflows/ci.yml  chat_empty_reply_tests.rs
OK        #851
CONFLICT  #852  .github/workflows/ci.yml  chat_empty_reply_tests.rs

Whichever of the five merges first, the other four need a rebase. Two shared anchors cause it.

1. chat_empty_reply_tests.rs — each adds its own two lines at the same spot:

#[path = "chat_registry_tests.rs"]    mod registry_tests;      // #848
#[path = "chat_sources_tests.rs"]     mod sources_tests;       // #849
#[path = "chat_fallback_tests.rs"]    mod fallback_tests;      // #850
#[path = "chat_persistence_tests.rs"] mod persistence_tests;   // #852

That one is unavoidable and cheap to resolve — keep all four lines.

2. .github/workflows/ci.yml — this one should not exist at all. Each PR appends a step, and the five filters are all subsets of the same module:

PR	filter
#845	api::chat::chat_empty_reply_tests
#848	chat_empty_reply_tests::registry_tests
#849	chat_empty_reply_tests::sources_tests
#850	chat_empty_reply_tests::fallback_tests
#852	chat_empty_reply_tests::persistence_tests

One step running chat_empty_reply_tests covers all five, and each of these steps otherwise pays its own cargo test -p utopia-server startup against the same database. Five near-identical steps are both the conflict source and four redundant CI invocations that will keep growing with the next sibling.

Suggestion: let #845 keep a single step named for the module rather than for the fix, and drop the ci.yml hunk from the other four. Then only the one-line mod additions collide, which is a trivial resolution and does not grow.

None of this is about the fixes themselves, which I am reviewing separately. #850 and #852 in particular are the same family as #845 — the chat path reporting success when something underneath it failed — and that family is worth naming.

[Maya-Kid]

Thanks for trial-merging these and identifying the shared conflict points. Agreed — I'll follow this approach: keep one module-wide chat_empty_reply_tests CI step in #845, name it for the module, and remove the redundant ci.yml additions from #848, #849, #850, and #852.

I'll also preserve all four test modules when resolving the conflicts and update the remaining branches as the PRs land. The local integration run already used that single module-wide step and retained all the tests; the individual PR branches still need that cleanup.

[Maya-Kid]

Completed the coordination cleanup and pushed the updated branches. All nine related PR branches are rebased onto dev@79caca04; #856 retains its explicit #854 dependency. The implementation and test files are unchanged by this maintenance pass.

• Hand off exhausted tool runs to an evidence-only final answer #845 now supplies one PostgreSQL-backed Chat module step: cargo test -p utopia-server api::chat::.
• Removed only the duplicate workflow additions from Keep older chat handles from unregistering newer generations #848, Publish citation sources added by document reads #849, Keep fallback retrieval failures distinct from empty results #850 and Do not report fallback chat completion when saving fails #852. All four test modules are retained. Hand off exhausted tool runs to an evidence-only final answer #845 should land first to provide the shared database-backed CI step.
• On the integrated tree, that exact command both listed 45 tests and executed 45 passing tests. An independent manifest taken from the original PR heads confirms all 35 newly introduced Chat tests actually ran: Hand off exhausted tool runs to an evidence-only final answer #845 22, Keep older chat handles from unregistering newer generations #848 1, Publish citation sources added by document reads #849 2, Keep fallback retrieval failures distinct from empty results #850 4, Preserve exact chat outcomes across stream closure and reattachment #851 3, Do not report fallback chat completion when saving fails #852 3. Non-Chat unit suites remain covered by the existing workspace job.
• I then ran six deliberately incomplete variants, omitting each PR's group in turn. Cargo returned success for all six, but the independent executed-test coverage check returned exit 1 and identified the missing group every time. The complete run passed the same check. This is an execution check, not just a green cargo exit or --list check.

Revalidation on isolated integration tree cc3aa07b8ef357034e8dbee97ab3dc8401b5d1d7: formatting, strict workspace Clippy and build pass; 1,013 Rust tests pass (one pre-existing external-network test ignored), 129 frontend tests, 27 browser subtests, and five Chrome/actual Linux backend/PostgreSQL scenarios pass. The DB/PDF guards were enabled. These new integration runs used a local scripted model, not a new paid-model evaluation.

The duplicate 0042 validation document was also removed from #845: durable findings stay in the original ADR, and historical run tables/SHAs now live in its PR description. Its evidence-only handoff design is frozen. Per-branch current heads and the validation scope are recorded in the updated PR descriptions.

[Maya-Kid]

Completed and pushed the current-dev synchronization for #845, #848–#852 and #854–#856, based on 474b904dc3e8014d42404a3c067af9906036a953. #851 retains both its stream tests and upstream #858’s terminal tests; the UI branches now follow #854 → #856 → #855.

The single Postgres-backed Chat command executed 47 tests, including all 37 introduced tests across seven groups. Seven deliberately filtered green Cargo runs were each rejected by the independent executed-test coverage checker. No tests were dropped to consolidate CI.

Combined tree 6cd976351b4d7cc7389730fce530526f4d197992: fmt/strict Clippy/build pass; workspace 1,015 pass, 5 ignored. The upstream delivery parent/process-kill and busy-deferral tests additionally passed on an exclusive database, as CONTRIBUTING requires. Frontend 129 module tests and 27 browser subtests pass, as do five actual Linux/PostgreSQL browser scenarios using a scripted model. All PR descriptions now identify their current heads and distinguish historical evaluation from this refresh.

[WaylandYang]

Correction to my note above: "keep all four lines" was half the advice. rustfmt also wants them sorted, so resolving in merge order fails cargo fmt --check:

#[path = "chat_fallback_tests.rs"]
mod fallback_tests;
#[path = "chat_persistence_tests.rs"]
mod persistence_tests;
#[path = "chat_registry_tests.rs"]
mod registry_tests;
#[path = "chat_sources_tests.rs"]
mod sources_tests;

I hit this resolving the four locally — it is the resolver's trap, not a fault in any of these PRs, but whoever merges will step in the same place.

Also worth reporting: I built a tree with #848, #849, #850, #851 and #852 all merged onto 474b904d and ran it end to end against an isolated PostgreSQL database.

cargo test -p utopia-server   355 passed; 0 failed; 1 ignored (the existing live-HTTPS RSS test)
  of which 26 chat tests, including every regression these five ship
pnpm test                     129 passed (19 files)
typecheck / guard / clippy -D warnings / fmt   clean

The combination is healthy, and the ci.yml collapse worked — the only remaining conflict is the four mod lines.

One thing I looked for specifically: chat_terminal_tests::a_turn_ends_in_exactly_one_earned_terminal (#858, now on dev) stays green with all five in, so none of them breaks the invariant. #851's lagged_subscribers_receive_an_error_not_done and first_terminal_freezes_the_snapshot_and_broadcast are that same rule enforced server-side, which is a better place for it than my client-observable assertion — those two together are the load-bearing part of #857.

Merging in the order #848, #851, #849, #850, #852.