Skip to content

Commit a7630ba

Browse files
committed
fix(s080): the panel told two lies, and the screenshot is what caught them
AC7 is this slice's one manual criterion and it earned its place on the first run. The panel looked right and reported two things wrongly, and no test saw either, because the tests asserted that rows appeared rather than that the rows were true. Every child was labelled active, including the three that had finished, each with a stop button offering to stop work that was over. The panel read SessionRow.status, which is the session lifecycle - active, archived, compacted - and stays active forever after a subagent finishes. It promised which are running and delivered which are not archived. Running state now comes from the process. All four short ids read the same eight characters. These ids are UUIDv7 and time-ordered, so children created in the same second share their leading bytes: measured, four ids generated together had one distinct 8-character prefix between them. A short id whose only job is to tell two rows apart has to come from the random end, so short_id/1 takes the tail. The first fix was then wrong in the other direction, and the tests caught that one: Sessions.state/1 answers with a view map rather than a bare state atom, so comparing it to :idle was true for every child and the panel reported everything as running. Two defects in my own tests, both found by running the suite rather than the file. await_child returns as soon as the child row exists, which create_child/3 inserts before the turn starts, so asserting running? on the next line raced the process it asked about. And ConnCase does not stop the session processes a test starts, so a child left running held the sandbox connection past its owner and failed the next test's setup rather than its own, which is the same shape of leak slice 025 found in persistent_term. scripts/dev_subagents.sh runs the demo the screenshot was taken from. It migrates both repositories before the application starts rather than after, because Oban verifies its migration as it boots and a run that migrates afterwards never gets that far. Both screenshots are kept, before and after, because the difference between them is the argument for the criterion being manual. mix gate exit 0; 639 passed; coverage 80.31%. Signed-off-by: Ayla Croft <aylacroft@proton.me>
1 parent acec0e8 commit a7630ba

4 files changed

Lines changed: 180 additions & 13 deletions

File tree

‎lib/trinity/subagents.ex‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,41 @@ defmodule Trinity.Subagents do
122122
end)
123123
end
124124

125+
@doc """
126+
Whether a child is still working.
127+
128+
Asked of the process, not of the row. `SessionRow.status` is the session's lifecycle - active,
129+
archived, compacted - and a finished subagent is still `"active"` forever, so a panel that reads
130+
it reports every child as running until somebody archives it. The screenshot for this slice's
131+
AC7 is what caught that: four children, three of them long finished, all labelled active with a
132+
stop button each.
133+
"""
134+
@spec running?(String.t()) :: boolean()
135+
def running?(session_id) do
136+
# `Sessions.state/1` answers with a view map, not the bare state atom. Comparing the map to
137+
# `:idle` is never equal, so the first version of this reported every child as running and the
138+
# panel's labels were wrong in the opposite direction from the bug it was written to fix.
139+
case Sessions.state(session_id) do
140+
%{state: state} -> state != :idle
141+
_ -> false
142+
end
143+
rescue
144+
_ -> false
145+
catch
146+
:exit, _ -> false
147+
end
148+
149+
@doc """
150+
A short, distinguishing form of a session id.
151+
152+
The **tail**, not the head. These ids are UUIDv7, which is time-ordered, so children created in
153+
the same second share their leading bytes: measured, four ids generated together had one
154+
distinct 8-character prefix between them. A short id whose whole job is to tell two rows apart
155+
has to come from the random end.
156+
"""
157+
@spec short_id(String.t()) :: String.t()
158+
def short_id(id) when is_binary(id), do: String.slice(id, -8, 8)
159+
125160
@doc "The children of a session, newest first."
126161
@spec children(String.t()) :: [Sessions.SessionRow.t()]
127162
def children(parent_session_id) do

‎lib/trinity_web/live/session_live/show.ex‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ defmodule TrinityWeb.SessionLive.Show do
100100
|> assign(status: status, draft: text, last_seq: last_seq)
101101
|> assign(
102102
approvals: Permissions.pending(id),
103-
subagents: Trinity.Subagents.children(id),
103+
subagents: subagent_panel(id),
104104
pending_count: length(Permissions.pending(:all))
105105
)
106106
|> assign_context(rows)
@@ -136,6 +136,15 @@ defmodule TrinityWeb.SessionLive.Show do
136136
defp content(nil), do: nil
137137
defp content(%Message{content: content}), do: content
138138

139+
# Each child paired with whether it is still working, asked of its process rather than read from
140+
# its row: SessionRow.status is the session lifecycle and stays "active" long after a subagent
141+
# has finished.
142+
defp subagent_panel(session_id) do
143+
session_id
144+
|> Trinity.Subagents.children()
145+
|> Enum.map(&{&1, Trinity.Subagents.running?(&1.id)})
146+
end
147+
139148
# Slice 080 AC7. The subagent tree for this session: what was delegated, how each child ended,
140149
# and a way to stop one. It renders nothing when there are none, because an empty panel on every
141150
# session teaches people to ignore the space it occupies.
@@ -148,20 +157,20 @@ defmodule TrinityWeb.SessionLive.Show do
148157
{gettext("Subagents")} ({length(@panel)})
149158
</h2>
150159
<ul class="flex flex-col gap-1">
151-
<li :for={child <- @panel} class="flex items-center gap-2 text-sm">
160+
<li :for={{child, running?} <- @panel} class="flex items-center gap-2 text-sm">
152161
<span class={[
153162
"rounded px-1.5 py-0.5 font-mono text-meta",
154-
child.status == "active" && "bg-warning/20 text-warning",
155-
child.status != "active" && "bg-base-200 opacity-70"
163+
running? && "bg-warning/20 text-warning",
164+
!running? && "bg-base-200 opacity-70"
156165
]}>
157-
{child.status}
166+
{if running?, do: gettext("running"), else: gettext("done")}
158167
</span>
159168
<.link navigate={~p"/s/#{child.id}"} class="min-w-0 flex-1 truncate hover:underline">
160169
{child.title || gettext("untitled brief")}
161170
</.link>
162-
<span class="font-mono text-meta opacity-50">{String.slice(child.id, 0, 8)}</span>
171+
<span class="font-mono text-meta opacity-50">{Trinity.Subagents.short_id(child.id)}</span>
163172
<button
164-
:if={child.status == "active"}
173+
:if={running?}
165174
type="button"
166175
phx-click="cancel_subagent"
167176
phx-value-id={child.id}
@@ -193,7 +202,7 @@ defmodule TrinityWeb.SessionLive.Show do
193202
# child cancelled while its own children run leaves work nobody is waiting for.
194203
def handle_event("cancel_subagent", %{"id" => id}, socket) do
195204
_ = Trinity.Subagents.cancel_subtree(id)
196-
{:noreply, assign(socket, subagents: Trinity.Subagents.children(socket.assigns.session.id))}
205+
{:noreply, assign(socket, subagents: subagent_panel(socket.assigns.session.id))}
197206
end
198207

199208
def handle_event("cancel", _params, socket) do

‎scripts/dev_subagents.sh‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
#!/usr/bin/env bash
2+
# SPDX-FileCopyrightText: Sudo Apt Holdings LLC
3+
# SPDX-License-Identifier: Apache-2.0
4+
# A session with subagents, for slice 080's AC7 screenshot. Same shape as
5+
# scripts/dev_chat_on_test_registry.sh: the test environment's Repo without its sandbox pool, its
6+
# own database (never the suite's), the fake provider, the application started by hand, and the
7+
# port printed as PORT=<n>.
8+
#
9+
# It delegates three briefs and leaves one child running, because a panel screenshot of three
10+
# finished children shows the list but not the thing the panel exists for: a stop control next to
11+
# work that is still going.
12+
cd "$(dirname "$0")/.."
13+
export MIX_ENV=test
14+
exec systemd-run --user --scope -p MemoryMax=32G --quiet -- mix run --no-start --no-halt -e '
15+
repo = Application.get_env(:trinity, Trinity.Repo) |> Keyword.delete(:pool) |> Keyword.put(:database, "trinity_screenshots.db")
16+
Application.put_env(:trinity, Trinity.Repo, repo)
17+
rrepo = Application.get_env(:trinity, Trinity.Repo.Receipts) |> Keyword.delete(:pool) |> Keyword.put(:database, "trinity_screenshots_receipts.db")
18+
Application.put_env(:trinity, Trinity.Repo.Receipts, rrepo)
19+
endpoint = Application.get_env(:trinity, TrinityWeb.Endpoint) |> Keyword.put(:check_origin, false)
20+
Application.put_env(:trinity, TrinityWeb.Endpoint, endpoint)
21+
Application.put_env(:trinity, :mcp_boot, false)
22+
# Both repos are migrated before the application starts, not after: Oban verifies its migration
23+
# as it boots (slice 050), so a run that migrates afterwards never gets that far.
24+
{:ok, _} = Application.ensure_all_started(:ecto_sql)
25+
for r <- [Trinity.Repo, Trinity.Repo.Receipts] do
26+
{:ok, pid} = r.start_link()
27+
Ecto.Migrator.run(r, :up, all: true)
28+
GenServer.stop(pid)
29+
end
30+
{:ok, _} = Application.ensure_all_started(:trinity)
31+
Logger.configure(level: :warning)
32+
33+
alias Trinity.{Sessions, Subagents}
34+
alias Trinity.LLM.Providers.Fake
35+
36+
answer = fn text ->
37+
Enum.flat_map(String.split(text), &[{:text_delta, &1 <> " "}, {:sleep, 10}]) ++
38+
[{:usage, %{input_tokens: 40, output_tokens: 30}}, {:done, :stop}]
39+
end
40+
41+
{:ok, parent} = Sessions.create_session(%{persona_id: Sessions.default_persona().id, title: "Refactoring the receipt chain"})
42+
43+
Fake.scripts([
44+
answer.("The chain writer checkpoints every 200 receipts and on terminate. Three call sites."),
45+
answer.("Two tests cover the checkpoint path; neither covers terminate during sandbox teardown."),
46+
answer.("The verifier reads the registry, not the key file, so it needs no custody adapter.")
47+
])
48+
49+
for brief <- [
50+
"Read lib/trinity/receipts/chain_writer.ex and say when it checkpoints",
51+
"Find the tests that cover checkpointing and say what they miss",
52+
"Say what the verifier needs from key custody, if anything"
53+
] do
54+
{:ok, _} = Subagents.delegate(parent.id, brief)
55+
end
56+
57+
# One left running, so the panel shows an active child with its stop control.
58+
Fake.scripts([[{:text_delta, "Reading the migration history "}, {:sleep, 600_000}, {:done, :stop}]])
59+
spawn(fn -> Subagents.delegate(parent.id, "Trace every migration that touched the receipts table") end)
60+
Process.sleep(1_500)
61+
62+
{:ok, {_, port}} = TrinityWeb.Endpoint.server_info(:http)
63+
IO.puts("PORT=#{port}")
64+
IO.puts("SESSION=#{parent.id}")
65+
'

‎test/trinity_web/live/subagent_panel_test.exs‎

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,42 @@ defmodule TrinityWeb.SubagentPanelTest do
2121
setup do
2222
Fake.clear()
2323
on_exit(fn -> Fake.clear() end)
24+
# ConnCase does not stop the session processes a test starts, and a child left running holds
25+
# the sandbox connection past its owner, which fails the *next* test's setup rather than this
26+
# one's. Cleaned up here for the same reason SessionCase does it.
27+
on_exit(fn -> Trinity.SessionCase.stop_all_sessions() end)
2428

2529
{:ok, parent} =
2630
Sessions.create_session(%{persona_id: Sessions.default_persona().id, title: "parent"})
2731

2832
{:ok, parent: parent}
2933
end
3034

35+
test "a short id distinguishes children rather than sharing a prefix with them" do
36+
ids = for _ <- 1..4, do: Trinity.UUID.generate()
37+
38+
# Measured, not assumed: four ids generated together had one distinct 8-character prefix
39+
# between them, which is what sent the first version of this panel out with four identical
40+
# labels.
41+
assert ids |> Enum.map(&String.slice(&1, 0, 8)) |> Enum.uniq() |> length() == 1
42+
assert ids |> Enum.map(&Subagents.short_id/1) |> Enum.uniq() |> length() == 4
43+
end
44+
45+
test "a finished child is shown as done, with no stop control", %{conn: conn, parent: parent} do
46+
Fake.scripts([script_deltas(1, "finished ")])
47+
{:ok, result} = Subagents.delegate(parent.id, "a brief that completes")
48+
49+
{:ok, _live, html} = live(conn, ~p"/s/#{parent.id}")
50+
51+
# The row's own status is still "active" here, because that field is the session lifecycle.
52+
assert Sessions.get_session(result.session_id).status == "active"
53+
54+
# The panel must not repeat that: the child has finished, and offering to stop it is a button
55+
# that does nothing.
56+
assert html =~ "done"
57+
refute html =~ ~s(phx-value-id="#{result.session_id}")
58+
end
59+
3160
test "with no subagents the panel is absent rather than empty", %{conn: conn, parent: parent} do
3261
{:ok, _live, html} = live(conn, ~p"/s/#{parent.id}")
3362
refute html =~ ~s(id="subagents")
@@ -44,26 +73,55 @@ defmodule TrinityWeb.SubagentPanelTest do
4473

4574
assert html =~ ~s(id="subagents")
4675
assert html =~ "read the long file and answer"
47-
assert html =~ String.slice(result.session_id, 0, 8)
76+
# The tail, not the head: these ids are UUIDv7 and children made in the same second share
77+
# their leading bytes, so a head-based short id tells two rows apart only by accident.
78+
assert html =~ Subagents.short_id(result.session_id)
79+
assert html =~ "done"
4880
assert html =~ ~s(href="/s/#{result.session_id}")
4981
end
5082

51-
test "stopping a subagent from the panel cancels it and the panel survives", %{
83+
test "stopping a running subagent cancels it, and the panel survives", %{
5284
conn: conn,
5385
parent: parent
5486
} do
55-
Fake.scripts([script_deltas(1, "done ")])
56-
{:ok, result} = Subagents.delegate(parent.id, "a brief to stop")
87+
# A child left running, so there is a stop control to press. {:sleep, ms} holds the turn open;
88+
# a script that merely omits its terminator does not, which is a lesson from this slice's
89+
# supervision test.
90+
Fake.scripts([[{:text_delta, "working "}, {:sleep, 30_000}, {:done, :stop}]])
91+
task = Task.async(fn -> Subagents.delegate(parent.id, "a brief to stop") end)
92+
93+
# The row exists before the process does: create_child/3 inserts the session, and the turn
94+
# starts after. Waiting for the row and asserting running? in the same breath is a race the
95+
# first version of this test lost.
96+
child_id = await_child(parent.id)
97+
assert await_running(child_id), "the child never reached a running state"
5798

5899
{:ok, live, _html} = live(conn, ~p"/s/#{parent.id}")
59100

60101
html =
61102
live
62-
|> element(~s(#subagents button[phx-value-id="#{result.session_id}"]))
103+
|> element(~s(#subagents button[phx-value-id="#{child_id}"]))
63104
|> render_click()
64105

65106
# The page still renders and still knows about the child; cancelling is not deleting.
66107
assert html =~ "a brief to stop"
108+
Task.await(task, 30_000)
109+
end
110+
111+
defp await_running(session_id, attempts \\ 200) do
112+
cond do
113+
Subagents.running?(session_id) -> true
114+
attempts > 0 -> Process.sleep(10) && await_running(session_id, attempts - 1)
115+
true -> false
116+
end
117+
end
118+
119+
defp await_child(parent_id, attempts \\ 200) do
120+
case Subagents.children(parent_id) do
121+
[child | _] -> child.id
122+
[] when attempts > 0 -> Process.sleep(10) && await_child(parent_id, attempts - 1)
123+
[] -> flunk("no child session appeared")
124+
end
67125
end
68126

69127
defp script_deltas(n, text) do

0 commit comments

Comments
 (0)