Skip to content

fix(chat): give a follow-up the documents of the message before it - #329

Closed
dovvnloading wants to merge 2 commits into
mainfrom
fix/follow-up-documents
Closed

dovvnloading wants to merge 2 commits into
mainfrom
fix/follow-up-documents

Conversation

@dovvnloading

Copy link
Copy Markdown
Owner

Problem

  • The follow-up bug (BE-34). Ask "summarise this" with a document attached, then "now list its section headings". The follow-up reached the model with no document text at all, so it made up the answer. Only attachment metadata is stored with a message, and fix(chat): budget context from measured token counts, keep history contiguous, replace streamed answers with their translation #306 only names earlier attachments in the history.
  • Wasted reads (BE-02 remainder). A new turn read every column of every message, and JSON-decoded each reply's reasoning, sources and stats, only to drop them. A fork read and decoded the whole source chat to check one message id.

Root cause

  • Nothing re-resolved the previous turn's documents for a turn that attaches none.
  • The repository had no partial read, only get_chat (everything) and get_chat_overview (no messages).

Change

Two commits.

  1. perf(persistence): read only the message fields a caller uses.
    • DatabaseManager.load_messages(thread_id, columns=...) selects only the named fields from a fixed whitelist; a name outside it is a ValueError and never reaches SQL. It decodes them as load_chat does, in one read transaction, and returns None for an unknown chat.
    • chat_exists is a SELECT 1.
    • Both repositories expose get_messages and chat_exists, covered by the shared contract suite (fix(persistence): harden chat and memory storage #319).
    • A new turn now reads five fields (HISTORY_MESSAGE_FIELDS); the fork check reads ids; rename asks chat_exists.
    • load_chat shares the row decoding, so the two reads cannot drift.
  2. fix(chat): give a follow-up the documents of the message before it.
    • When a turn attaches nothing, the documents on the user's most recent message are resolved again through the same owner and integrity checks, and sent under their own heading, marked as from the previous message.
    • They go through the attachment budget that scales with the context window.
    • Images stay with their own turn. Only the immediately previous message lends documents, and a turn with its own attachments gets only those.
    • A document deleted or expired since is left out; only a count is logged, never a name or text. The turn proceeds.
    • Nothing is persisted on the follow-up, and regenerating its reply brings the same documents back.
    • The new turn's one transcript read moves from prepare() to admission, so the previous turn's attachments are known before resolution. The user turn is still appended with expected_revision set to the admitted revision. Since fix(persistence): harden chat and memory storage #319 any write moves the revision, so a write landing after the read fails the append, as before.

Not included: the third commit of the original branch (a sticky per-thread code contract, BE-27). It touches the code-execution prompt and needs its own review.

Compatibility and rollback

  • No schema or API change.
  • ChatRepository gains two methods. The only implementations are the SQLite adapter and the in-memory double, both updated.
  • Each commit reverts on its own; reverting the second alone restores the old follow-up behaviour.

Checks

Windows, Python 3.14 unless stated.

  • Focused files: python -m pytest -q tests/test_chat_attachments.py tests/test_prompt_shape.py tests/test_event_loop_hygiene.py tests/test_chat_repository_contract.py tests/test_chat_correctness.py tests/test_generation_harness_controls.py gives 284 passed, 19 subtests.
  • Python 3.12 (py -3.12) on the four changed test files: 195 passed.
  • Against the old code:
    • test_chat_attachments.py and test_prompt_shape.py copied onto an export of origin/main (2972c7a) give 10 failed, 78 passed. The 10 are the new follow-up tests, including deleted/expired documents, images not resent, only the previous message, and the budget at two window sizes.
    • test_event_loop_hygiene.py and test_chat_repository_contract.py cannot be collected there, because they use the new repository methods.
  • Pre-push hook (scripts/check.ps1 quick tier): all 11 checks passed in 416.5 s (backend 255.7 s, vitest 91.8 s).

Security, concurrency, data loss

  • Earlier documents are resolved with the requester's owner and the attachment service's integrity checks, exactly like new ones.
  • Partial-read column names come from a fixed whitelist, never from input.
  • Logging: prompt and document text never reach logs.
  • Revision guard: the append is guarded by the admitted revision. A chat deleted between admission and the read is refused as stale_revision.

Limits

  • Not run against a live model; the tests use the fake runtime and real attachment storage.
  • A document comes back only for the turn right after it. A question two turns later does not get it back automatically.

Plan item: BE-34 (remainder), BE-02 (remainder)

🤖 Generated with Claude Code

dovvnloading and others added 2 commits October 17, 2026 09:38
A new turn read the whole transcript to build the model's history, which
copied and JSON-decoded every reply's reasoning, sources and stats only to
drop them; a fork read and decoded the whole source chat just to check
that one message id was in it.

DatabaseManager.load_messages(thread_id, columns=...) selects only the
named fields (a fixed whitelist, never passed to SQL as given), decodes
them the way load_chat does, and returns None for an unknown chat.
chat_exists is a SELECT 1. Both repositories expose them as get_messages
and chat_exists, covered by the shared contract tests. The new-turn
history read now asks for the five fields history is built from, the
fork check reads only ids, and rename asks chat_exists. load_chat shares
the row decoding so the two reads cannot drift.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
"Summarise this" with a document, then "now list its section headings":
only attachment metadata is stored with a message, so the follow-up
reached the model with no document text at all, and it invented the
answer.

When a turn attaches nothing, the documents attached to the user's most
recent message are resolved again, through the same owner and integrity
checks, and sent marked as coming from the previous message, under their
own heading. They go through the attachment budget that scales with the
context window like any document. Images stay with their own turn, only
the immediately previous message lends documents, and a turn that
attaches its own files gets only those. A document deleted or expired
since is left out (a count is logged, never a name or text) and the turn
proceeds. Nothing is persisted on the follow-up; regenerating its reply
brings the same documents back.

To have the previous turn's attachments before resolution, the new
turn's one partial transcript read moves from prepare() to admission.
The user turn is still appended guarded by the admitted revision, so a
write landing after that read fails the append as before. A brand-new
chat no longer makes a read at all.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dovvnloading

Copy link
Copy Markdown
Owner Author

Superseded by #331, which combines this with the other pending fixes into one pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant