diff --git a/CHANGELOG.md b/CHANGELOG.md index dac5f3b..e34da9b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,27 @@ evidence for each increment is retained by the maintainers and summarised here. ## 2026-09-24 +### `slice/014` — The prompt window is the recent conversation +A session that held more than five hundred messages was building every subsequent request from its +**first** five hundred. The query that fetched the window ordered oldest-first and then took a +limit, which is right for the interface and the export, which page forwards through a conversation, +and wrong for building a prompt. Past that point the assistant was answering from the opening of the +session with the current exchange absent from the request entirely. Compaction does not delete rows, +so any long-running session reached that point by design rather than by accident. + +It explains a second symptom too. Compaction decides what still needs summarising from the most +recent compaction *in the window it is given*, so a window pinned to the oldest rows let it +re-summarise ground it had already covered, spending a model call to learn nothing. + +The fix is a second accessor rather than a change to the first: the prompt and the compactor ask for +the most recent rows in reading order, and the forward pager keeps its behaviour, with a test saying +so. Two callers with opposite needs sharing one function is how this happened. + +Found by accident. Slice 110 registered one tool, which pushed a deliberately small test window past +a threshold, which made compaction frequent enough to take a session past five hundred rows for the +first time in any test. Proven by reverting the fix and watching the new test report the defect in +its own failure message, rather than by the fix passing. + ### `slice/004` — Property-based testing, and the assertions it enables The canonicalisers, the argument validator, the tighten-only state modifier and the receipt chain are now exercised by generated input rather than only by chosen input. That matters most where the code diff --git a/coverage.tsv b/coverage.tsv index c95dcd1..75ae532 100644 --- a/coverage.tsv +++ b/coverage.tsv @@ -31,3 +31,4 @@ slice_id percent sha date 029 80.31 16e42ad 2026-09-24 120 80.31 78ee950 2026-09-24 004 80.31 91de943 2026-09-24 +014 80.31 1a5d480 2026-09-24 diff --git a/lib/trinity/sessions.ex b/lib/trinity/sessions.ex index 7b4b57d..3a5321e 100644 --- a/lib/trinity/sessions.ex +++ b/lib/trinity/sessions.ex @@ -63,6 +63,14 @@ defmodule Trinity.Sessions do @spec history(session_id(), keyword()) :: [Message.t()] def history(session_id, opts \\ []), do: Store.history(session_id, opts) + @doc """ + The most recent `limit` rows, ascending: the window a prompt is built from (slice 014). + + Not `history/2`, which pages forwards from the beginning and returns the oldest rows when limited. + """ + @spec recent_history(session_id(), keyword()) :: [Message.t()] + def recent_history(session_id, opts \\ []), do: Store.recent_history(session_id, opts) + @doc "Marks a session archived." @spec archive(SessionRow.t()) :: {:ok, SessionRow.t()} | {:error, Ecto.Changeset.t()} def archive(%SessionRow{} = session), do: Store.update_session(session, %{status: "archived"}) diff --git a/lib/trinity/sessions/session.ex b/lib/trinity/sessions/session.ex index 8aeebfb..c47e385 100644 --- a/lib/trinity/sessions/session.ex +++ b/lib/trinity/sessions/session.ex @@ -355,7 +355,10 @@ defmodule Trinity.Sessions.Session do persona = session.persona_id && Store.get_persona(session.persona_id) # Slice 020: the declared surface of this turn, into the request and onto the row. tools = Trinity.Tools.to_llm_tools() - history = Trinity.Sessions.history(id, limit: 500) + # Slice 014: the **recent** 500 rows, not the first 500. `history/2` orders ascending and then + # limits, so past 500 rows it would hand this the opening of the session for ever and the + # current exchange would not be in the request at all. + history = Trinity.Sessions.recent_history(id, limit: 500) # Slice 033: the project's AGENTS.md, read now, so a change is in this turn (live reload). context = [ diff --git a/lib/trinity/sessions/store.ex b/lib/trinity/sessions/store.ex index b40da94..7ca85a2 100644 --- a/lib/trinity/sessions/store.ex +++ b/lib/trinity/sessions/store.ex @@ -113,6 +113,30 @@ defmodule Trinity.Sessions.Store do |> Repo.all() end + @doc """ + The most recent `limit` rows, in ascending order (slice 014). + + `history/2` above orders ascending and then limits, so it returns the **oldest** rows: it is the + forward pager the interface and the export walk a session with, and it is right for that. It is + wrong for building a prompt. A session that holds more rows than the window would otherwise be + answered from its opening, with the current exchange missing from the request entirely, and + compaction handed the same stale window would re-summarise ground it had already covered. + + Descending with a limit takes the newest; the reverse puts them back in the order a conversation + has to be read in. + """ + @spec recent_history(String.t(), keyword()) :: [Message.t()] + def recent_history(session_id, opts) do + limit = Keyword.get(opts, :limit, 200) + + Message + |> where([m], m.session_id == ^session_id) + |> order_by([m], desc: m.seq) + |> limit(^limit) + |> Repo.all() + |> Enum.reverse() + end + @spec get_persona(String.t()) :: Persona.t() | nil def get_persona(id), do: Repo.get(Persona, id) diff --git a/test/trinity/sessions/prompt_window_test.exs b/test/trinity/sessions/prompt_window_test.exs new file mode 100644 index 0000000..dc110ce --- /dev/null +++ b/test/trinity/sessions/prompt_window_test.exs @@ -0,0 +1,78 @@ +# SPDX-FileCopyrightText: Sudo Apt Holdings LLC +# SPDX-License-Identifier: Apache-2.0 +defmodule Trinity.Sessions.PromptWindowTest do + @moduledoc """ + Slice 014 AC1 and AC2: the window a prompt is built from is the **recent** conversation. + + No existing test reaches a session longer than the window, which is why this went unseen. Every + assertion here is at a row count above it, because below it the defect is invisible by definition. + """ + use Trinity.DataCase, async: false + + alias Trinity.Sessions + + @window 500 + @rows 520 + + defp session_with_rows!(n) do + session = Trinity.Factory.session!() + + rows = + for seq <- 1..n do + now = DateTime.utc_now() + + %{ + id: Trinity.UUID.generate(), + session_id: session.id, + seq: seq, + role: if(rem(seq, 2) == 1, do: "user", else: "assistant"), + content: "message #{seq}", + parts: %{"text" => "message #{seq}"}, + inserted_at: now, + updated_at: now + } + end + + Trinity.Repo.insert_all(Trinity.Sessions.Message, rows) + session + end + + test "AC1: the window ends at the newest row, not the five-hundredth" do + session = session_with_rows!(@rows) + + seqs = Sessions.recent_history(session.id, limit: @window) |> Enum.map(& &1.seq) + + assert List.last(seqs) == @rows, + "the prompt window ends at seq #{List.last(seqs)} of #{@rows}. Past the window the " <> + "assistant answers from the opening of the session with the current exchange missing" + + assert length(seqs) == @window + assert List.first(seqs) == @rows - @window + 1 + end + + test "AC2: the window reads oldest to newest, because a conversation is an order" do + session = session_with_rows!(@rows) + seqs = Sessions.recent_history(session.id, limit: @window) |> Enum.map(& &1.seq) + + assert seqs == Enum.sort(seqs), + "the window came back newest-first, which would present the conversation backwards" + end + + test "a session shorter than the window is unchanged by any of this" do + session = session_with_rows!(10) + seqs = Sessions.recent_history(session.id, limit: @window) |> Enum.map(& &1.seq) + assert seqs == Enum.to_list(1..10) + end + + test "AC3: history/2 still pages forwards from the beginning, for the callers that walk a session" do + session = session_with_rows!(@rows) + + first_page = Sessions.history(session.id, limit: 10) |> Enum.map(& &1.seq) + second_page = Sessions.history(session.id, limit: 10, offset: 10) |> Enum.map(& &1.seq) + + assert first_page == Enum.to_list(1..10), + "history/2 is the forward pager the UI and the export use; this slice must not change it" + + assert second_page == Enum.to_list(11..20) + end +end diff --git a/test/trinity/telemetry/telemetry_test.exs b/test/trinity/telemetry/telemetry_test.exs index aafa8b3..15e906b 100644 --- a/test/trinity/telemetry/telemetry_test.exs +++ b/test/trinity/telemetry/telemetry_test.exs @@ -170,12 +170,38 @@ defmodule Trinity.TelemetryTest do {:ok, _} = Sessions.send_user_message(session.id, "first") assert_receive {^ref, [:trinity, :llm, :call, :stop], _, first}, 5_000 + # The stop event fires when the model call ends, and the session is still `:thinking` while it + # writes the result. Sending the next message on that event is a race: it passed on the + # ordinary gate for weeks and failed on the FIPS leg, which is slower, with + # `{:error, {:busy, :thinking}}`. Waiting for idle is what the test meant all along. + await_idle(session.id) + {:ok, _} = Sessions.send_user_message(session.id, "second") assert_receive {^ref, [:trinity, :llm, :call, :stop], _, second}, 5_000 refute first.trace_id == second.trace_id end + defp await_idle(id, timeout \\ 5_000) do + deadline = System.monotonic_time(:millisecond) + timeout + await_idle(id, deadline, :first) + end + + defp await_idle(id, deadline, _) do + case Sessions.state(id) do + %{state: :idle} -> + :ok + + other -> + if System.monotonic_time(:millisecond) > deadline do + flunk("session #{id} was #{inspect(other)} and never became idle") + else + Process.sleep(10) + await_idle(id, deadline, :again) + end + end + end + test "outside a trace an event carries no trace id, rather than an invented one" do ref = make_ref() me = self()