Commit e69d750
authored
Copy a session's id, and its transcript, mid-turn (SPEC-52) (#164)
* SPEC-51: session identity — spec, plan and design board
Copy the underlying agent session id (pi's ACP sessionId / codex threadId)
and its on-disk transcript path, mid-turn. Typing pi's /session gets queued
behind the running turn; a client command and a menu never do.
Spec is rev 2: two independent reviews ran against rev 1 before any code.
They caught three things worth having:
- D3 was unbuildable. Session has no project path, and pi's transcript slug
follows the WORKTREE cwd, not the project root. The nearest precedent
(attachPiSession) uses the project path, so copying it would have missed
the transcript for every worktree-bound session -- and D9 would have hidden
the failure. Resolution moves to SessionManager, keyed on
worktreePath ?? project.dto.path, memoized (a readdir per snapshot was
unaffordable).
- D15: the mockup's 'pi --session 019ff121' was unsafe. pi ids are UUIDv7,
whose first 48 bits are a ms timestamp, so 8 chars leave ~65s of ambiguity;
real collisions exist on this machine. Review went further and drove it:
pi does not error on an ambiguous prefix, it silently picks one and offers
to fork it. Full id everywhere.
- D4's '~/' abbreviation was unimplementable: the path belongs to the server
host and the app cannot know that host's home dir.
Cut on review: the identity section inside the context-usage panel (the ring
is absent in exactly the four states where the id is wanted), and one of the
three panel doors (a tab-menu item duplicating the pane kebab).
Added on review: a11y as a locked decision (D18), watch-not-snapshot so a
panel opened before the id is assigned fills in live (D19), and the
server-path disclosure written down rather than assumed (D21).
* SPEC-51 P1a: the session-identity panel, UI first (no wire yet)
Phase A of the plan: build and pixel-verify the whole surface BEFORE freezing
the wire contract. The widgets take an app-level SessionIdentity, never a
SessionDTO, which is what makes that order possible -- and is the right
dependency direction anyway.
SessionIdentity per-agent vocabulary as a lookup TABLE, not a switch
(ENGINEERING.md's OCP rule), with a safe default that
gives an unknown agent a generic label and no resume
line rather than an invented CLI.
sessionIdentityText() pure; THE copy contract, one place, four callers.
SessionIdentityDetails one Copy all (D5), rows omitted when unmeasured (D9),
read-only (D8), semantics labels (D18).
showSessionIdentity() sheet on mobile, window-clamped popover on desktop.
18 tests, each with a mutation proven to fail it. Two of those mutations
initially did NOT bite, and both were my bugs rather than the tests':
- the clipboard-failure test asserted on rendered toast text, but the test host
has no toast overlay, so findsNothing could never fail. It now asserts against
the StatusCenter directly.
- two mutation scripts silently no-oped on a Python escaping bug, which looked
exactly like a vacuous test until checked.
Found by the pixel gate on the real macOS app, not by any test: "1 lines".
Reachable in production on a stub/detached session whose only measured value is
the makit id. Fixed, and the 1-line case is now asserted in both the visible
label and the semantics label.
Also fixes a real bug in test/status/status_lifetime_test.dart (SPEC-48 D3's
repo-wide guard): it sliced the body out of the ORIGINAL source, so a comment
naming the rule it follows supplied the "first await" and turned the
correctly-hoisted line below it into an offender -- and a commented-out
ref.status counted as a real one. It now scans the blanked copy; offsets are
preserved so line numbers still map. Red test added for the false positive, and
the guard is proven still to bite by un-hoisting.
QA evidence (cua-driver, real macOS window, both themes, 320 and 375pt):
both uuid rows render on ONE line -- the measured payoff for dropping the
per-row copy column; light-mode Copy all measures 10.95:1 (needs 4.5); line
counts 4/3/3/2/1 all correct; draft renders 'Agent not started yet'.
Note for anyone repeating that gate: Flutter's macOS AX tree exposes nothing but
the menu bar, and synthesized clicks never reached the app (the staleness stamp
proved it), so the harness renders every state in one pass and needs no input at
all. scroll DID work.
Mockup corrected to the shipped type scale (sentence case, as
ContextUsageDetails ships) and its in-panel footnotes dropped.
* SPEC-51 P1b: freeze the wire contract — agentSessionId + transcriptPath
Two OPTIONAL fields on SessionDTO, and their app-side decode. Committed on its
own, before either implementation, so the parallel server/app work cannot both
edit the shared shape.
Frozen spelling: `agentSessionId?: string`, `transcriptPath?: string`.
Optional is the decision, not an accident: a newer app paired with an older
server must render one fewer row rather than a fabricated one, which is the rule
createdAt already follows (SPEC-47 D12).
App side normalises '' to null at the decode edge. That is not paranoia -- '' is
what a sloppy or partially-migrated server sends for "no value", and an empty
string here would render a copy affordance that copies nothing, i.e. exactly the
placeholder D9 forbids. Non-strings are rejected the same way, so a malformed
snapshot degrades one field instead of failing the whole session list.
Nothing populates the fields yet -- that is C1b, and its tests are where the
bite lives. The vacuous "a DTO built without them is undefined" test from plan
rev 1 is deliberately NOT here: optional TS fields are absent by default with
zero production code, and JSON.stringify drops undefined.
server: tsc clean, 1313 pass / 0 fail (baseline preserved)
app: analyze clean, 5 new codec tests green
* SPEC-51 P1c: populate the fields (server) and wire the doors (app)
Two agents on disjoint trees, then verified here rather than taken on report.
server (C1):
transcript-path.ts NEW. Agent-agnostic dispatcher: resumeSessionPath
verbatim -> pi-only directory scan -> undefined. In its
own module because two of its three branches are not
pi-specific, so P2's codex resolver is additive rather
than a move out of a pi-named file. Never throws.
manager.ts projectSessionDTO() populates both fields at the two
wire sites (sessions.snapshot, session.listClosed).
Session.toDTO() CANNOT do this -- a Session holds
projectId + worktreePath, never the project's FS path.
cwd = worktreePath ?? project path, because pi's slug
follows the cwd pi actually ran in and that is usually
the worktree; the nearest precedent (attachPiSession)
uses the project path and would have missed every
worktree-bound session, with D9 hiding the failure.
Memoized per session id including misses: a snapshot is
rebroadcast on every metaChanged, so resolving per
projection meant a readdir per session per broadcast.
app (C2):
sessionIdentityProvider store -> SessionIdentity; null FIELDS, never null,
never throws, so no call site has to branch.
/session a CLIENT command next to /name, so it is
intercepted before sendMessage and works MID-TURN.
That is the whole bug: pi's own /session is an
agent command and gets queued behind the turn.
`/session id` copies the bare id; bare opens panel.
two doors + tab copy glass menu, pane kebab, and Copy session id on the
tab menu (not a third panel door -- cut on review).
showSessionIdentity is now exactly-one-of {sessionId, identity}, asserted. The
first wiring took a required identity AND an optional sessionId, so all three
doors did a ref.read whose result was then discarded: an argument that is
ignored depending on another argument is a trap, not an API.
Mutations proven to bite (9 across both trees), including the two that matter
most: cwd -> project path fails the worktree test, and a prefix-relaxed suffix
match fails the D15 collision test.
server: tsc clean, 1326 pass / 0 fail (baseline 1313)
app: analyze clean, 0 non-loading failures, 55 SPEC-51 tests green
* docs(SPEC-51): record the verification evidence and flip the status
Phase D step 5. The Verification section said "filled in on implementation" and
the status still said Draft; both are now true statements about the shipped code.
The one gate that had no recorded result was the live probe, so it was run:
resolveTranscriptPath against the real ~/.pi/agent/sessions tree, for this
branch's own worktree, using the pi session id of the session that implemented
the feature. It returned the transcript path byte-identical to the one pi itself
reports. The same call with the 8-char prefix returned undefined -- D15 holds on
real data, not just in the unit test's fixture dir. `pi --help` confirms the
resume line's spelling is `--session <path|id>`.
Also noted the tenth mutation: relaxing handleClientCommand's exact name match
to startsWith, which `/sessions` is NOT intercepted catches. That mutation was
left in the working tree by the previous session and has been reverted -- exact
matching is the shipped behaviour.
Status is Implemented for P1 only; the codex resolver branch and D12's section
inside the usage panel stay deferred, so the header says so rather than implying
the whole spec landed.
* style(app): apply dart format to the SPEC-51 files
Whitespace and trailing commas only — verified by comparing both revisions with
all whitespace and commas stripped, so no token changed. `dart format
--set-exit-if-changed` is now clean across lib/test/tool, analyze reports no
issues, and the 52 SPEC-51 tests in these files stay green.
The pre-push hook writes these fixes itself and then fails the push, so they have
to land as a commit rather than as a dirty tree.
* fix(app): report a failed id copy, and survive the tab closing under the menu
Three review findings, each with a red test written first.
1. HIGH -- `/session id` awaited Clipboard.setData with no try/catch, so a
PlatformException (another process holds the clipboard on Windows; the host
denies the write) escaped the handler: the user got neither the id nor a word
about why. The panel's `Copy all` already had this guard; the two bare-id paths
did not.
The review flagged client_commands.dart. The IDENTICAL bug sat in
split_view.dart's tab-menu copy, unflagged, so both are fixed -- three copy
paths in one feature must not disagree about what a failed write does.
All three now report `failure`, not `warning`: an action the user asked for did
not happen, which is what status.failure is for. The panel's `warning` was the
odd one out and moved too. Severity is now ASSERTED in all three tests, so the
consistency is enforced rather than incidental -- flipping any one back to
warning fails its test (proven).
2. HIGH -- the tab menu's ref.read(sessionIdentityProvider) ran AFTER the showMenu
await. That menu lives in the Navigator's overlay, so it outlives the _TabChip
that opened it: close the tab while the menu is open (a server snapshot dropping
the session does it for real) and the read hits a dead ref --
`Bad state: Using "ref" when a widget is about to or has been unmounted is
unsafe.` A crash instead of a copy. The new test reproduces it exactly that way,
by calling unbindSession while the menu is up, and it failed with that message
before the fix.
Hoisted next to `status`, which is already hoisted for this very reason
(SPEC-48 D3). Chose that over the reviewer's alternative of a context.mounted
guard: the guard keeps the read fresh but still leaves `ref` use after an await,
which is the hazard D3 exists to remove. The cost is that the id is sampled at
menu-open rather than at click -- sub-second for right-click -> click, and the
surface that must fill in live is the panel, which watches (D19).
Measured rather than assumed: broadening status_lifetime_test.dart to catch
ref.read/ref.watch after an await repo-wide -- which would have caught this
automatically -- reports 24 bare sites across lib/. Out of scope here, so it is
named as a follow-up instead of pretending this PR fixed a repo-wide pattern.
3. LOW -- three stale statements in mockups/session-identity.html, each of which
misleads someone reading it as the design source:
- "Three things on this page were wrong" above a list of six;
- prose still claiming the panel DISPLAYS `~/` paths, contradicting both the
rendered frames and the amendment 20 lines above it (D4 dropped `~/`);
- the delta table still promising `Session details...` in the tab menu -- the
fourth door D13 explicitly cut. That is the dangerous one: a delta table
reads as a work list.
Markup re-validated (balanced tags) after the edits.
* docs: renumber this spec 51 -> 52, because 51 and 50 both shipped first
Not a cosmetic rename. While this branch was in flight, `main` took BOTH numbers
next to it:
SPEC-51 Preview groups (#163) -- docs/UX.md + 14 shipped source files
SPEC-50 Profiles (#162) -- a whole server-profiles feature
So `SPEC-51` in this branch's code comments pointed at a different, already-shipped
feature, and `docs/specs/` held two SPEC-51 files. Neither shipped spec can move,
so this one takes 52, the next free number.
Renumbered ONLY this feature's own files, derived from this branch's commits
(1b92874..2a95bf7 plus the fix commit) rather than by a blanket search-replace:
a global `SPEC-51 -> SPEC-52` would have rewritten preview groups' own references
in 14 files and docs/UX.md, silently pointing them at this spec instead. Each of
the remaining 14 SPEC-51 references was checked to be preview groups'.
The branch name (feat/get-session-id) is left alone deliberately -- renaming a
pushed branch orphans its PR for no gain.
Recorded in the spec header, including the part a rename does not fix: nothing in
this repo allocates spec numbers, so two branches drafted in one week collide in
silence. `main` already carries that proof -- SPEC-48 names both
2026-08-09-SPEC-48-status-and-activity.md and
2026-08-10-SPEC-48-per-repo-settings.md.
Verified after the renumber, on the merge with the NEWER main:
server: tsc clean, 2072 pass / 0 fail
app: analyze clean, dart format clean, 0 non-loading failures
* fix(app): the desktop panel is centred on purpose — say so, and test it
Fourth review thread. The doc comment promised "an anchored popover on desktop.
Same split as ContextUsageButton (SPEC-37)" and D11 specified a MenuAnchor; the
code ships showDialog + Alignment.center. So this was not a stale comment, it was
an unrecorded spec deviation.
Kept the centring, and the reason is a real finding rather than a shrug: the
mechanism cannot transfer, because the DOOR TOPOLOGY differs. ContextUsageButton
is a persistent control in the composer, so a MenuAnchor has something to stay
anchored to for as long as the popover is open. Both of this panel's desktop doors
are transient MENU ITEMS -- the pane-header kebab and the mobile glass menu -- so
by the time an item is chosen its menu has been dismissed. Anchoring to where a
vanished menu item used to be is arbitrary placement dressed up as precision.
What SPEC-37 actually contributes is kept: the window-clamped width and the
SingleChildScrollView, so a panel opened from a narrow split pane cannot hang
off-screen.
The real defect underneath: the desktop host had NO test. Every case in
session_identity_widget_test.dart passed `desktop: false`, which is exactly how
the code and its own doc comment drifted apart and stayed that way through
implementation. Two tests added:
* desktop opens a centred, window-clamped panel -- not a sheet
* a desktop panel in a narrow window is clamped to the window (300pt window,
340pt panel, so the clamp is load-bearing)
Both mutation-proven: Alignment.center -> topLeft fails the first, and dropping
the math.min window clamp fails the second.
D11 amended in the spec and logged as deviation 8, so the next reader does not
find a third version of the truth.
* fix(app): floor the panel width at zero, in BOTH panels that compute it
Fifth review thread, and it was right: window - 2 * margin goes NEGATIVE once the
window is narrower than the margins, and a BoxConstraints/SizedBox with a negative
max is not normalized -- the layout ASSERTS ("BoxConstraints has both width and
height constraints non-normalized") instead of rendering a cramped panel. "The
window is absurd" should cost an ugly panel, not a crash.
Confirmed by repro before fixing, and the repro is the interesting part. My first
attempt -- open the panel in a 20x20 window -- PASSED while proving nothing: at
that size the trigger button is unhittable, so the tap lands on nothing and the
panel never opens. A test that cannot fail. Both tests now shrink the window WHILE
the panel is open, which is also the realistic path: a resize animation or an
embedded host hands us one degenerate frame.
session_identity.dart maxWidth AND maxHeight floored at 0 (threshold 24pt,
margin 12).
context_usage.dart the SPEC-37 panel this one copied its sizing from has
the SAME bug on the width axis, at a 16pt threshold
(margin 8). Its height axis was already safe, floored by
_kUsagePanelMinHeight; the width axis had no floor. Not
in this PR's diff, but confirmed real with a red test, so
it is fixed here per AGENTS.md rather than left for
someone to hit.
I nearly recorded that second one as "not reproducible": my first attempt used a
20pt window, which is negative for a 12pt margin but still POSITIVE for an 8pt
one, so the assert did not fire. It needed 10pt. Worth stating because the
near-miss is the whole lesson -- a threshold bug needs a size chosen from the
constant, not reused from the previous test.
Same coverage hole under both: neither panel had a single `desktop: true` test, so
neither desktop host was covered at all. That is also how the identity panel's doc
comment came to describe an anchored popover while shipping a centred dialog.1 parent c05e275 commit e69d750
25 files changed
Lines changed: 3970 additions & 9 deletions
File tree
- app
- lib
- desktop/chat
- panes
- store
- transport
- ui
- composer
- session
- test
- status
- tool
- docs/specs
- mockups
- server/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
| |||
145 | 146 | | |
146 | 147 | | |
147 | 148 | | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
148 | 158 | | |
149 | 159 | | |
150 | 160 | | |
| |||
155 | 165 | | |
156 | 166 | | |
157 | 167 | | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
158 | 173 | | |
159 | 174 | | |
160 | 175 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
| 2 | + | |
2 | 3 | | |
3 | 4 | | |
4 | 5 | | |
| |||
7 | 8 | | |
8 | 9 | | |
9 | 10 | | |
| 11 | + | |
10 | 12 | | |
11 | 13 | | |
12 | 14 | | |
| |||
665 | 667 | | |
666 | 668 | | |
667 | 669 | | |
668 | | - | |
669 | | - | |
670 | | - | |
| 670 | + | |
| 671 | + | |
| 672 | + | |
| 673 | + | |
| 674 | + | |
671 | 675 | | |
672 | 676 | | |
673 | 677 | | |
| |||
678 | 682 | | |
679 | 683 | | |
680 | 684 | | |
| 685 | + | |
| 686 | + | |
| 687 | + | |
| 688 | + | |
| 689 | + | |
| 690 | + | |
| 691 | + | |
| 692 | + | |
| 693 | + | |
| 694 | + | |
| 695 | + | |
| 696 | + | |
| 697 | + | |
| 698 | + | |
| 699 | + | |
| 700 | + | |
| 701 | + | |
681 | 702 | | |
682 | 703 | | |
683 | 704 | | |
| |||
691 | 712 | | |
692 | 713 | | |
693 | 714 | | |
| 715 | + | |
| 716 | + | |
| 717 | + | |
| 718 | + | |
| 719 | + | |
694 | 720 | | |
695 | 721 | | |
| 722 | + | |
| 723 | + | |
| 724 | + | |
| 725 | + | |
| 726 | + | |
| 727 | + | |
| 728 | + | |
| 729 | + | |
| 730 | + | |
| 731 | + | |
| 732 | + | |
| 733 | + | |
| 734 | + | |
| 735 | + | |
| 736 | + | |
| 737 | + | |
| 738 | + | |
| 739 | + | |
| 740 | + | |
| 741 | + | |
| 742 | + | |
| 743 | + | |
| 744 | + | |
| 745 | + | |
| 746 | + | |
| 747 | + | |
| 748 | + | |
| 749 | + | |
| 750 | + | |
| 751 | + | |
| 752 | + | |
| 753 | + | |
| 754 | + | |
| 755 | + | |
696 | 756 | | |
697 | 757 | | |
698 | 758 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1237 | 1237 | | |
1238 | 1238 | | |
1239 | 1239 | | |
| 1240 | + | |
| 1241 | + | |
1240 | 1242 | | |
1241 | 1243 | | |
1242 | 1244 | | |
| |||
1296 | 1298 | | |
1297 | 1299 | | |
1298 | 1300 | | |
| 1301 | + | |
| 1302 | + | |
| 1303 | + | |
| 1304 | + | |
| 1305 | + | |
| 1306 | + | |
| 1307 | + | |
| 1308 | + | |
| 1309 | + | |
| 1310 | + | |
| 1311 | + | |
| 1312 | + | |
1299 | 1313 | | |
1300 | 1314 | | |
1301 | 1315 | | |
| |||
1319 | 1333 | | |
1320 | 1334 | | |
1321 | 1335 | | |
| 1336 | + | |
| 1337 | + | |
1322 | 1338 | | |
1323 | 1339 | | |
1324 | 1340 | | |
| |||
1341 | 1357 | | |
1342 | 1358 | | |
1343 | 1359 | | |
| 1360 | + | |
| 1361 | + | |
1344 | 1362 | | |
1345 | 1363 | | |
1346 | 1364 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
274 | 274 | | |
275 | 275 | | |
276 | 276 | | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
277 | 283 | | |
278 | 284 | | |
279 | 285 | | |
280 | 286 | | |
281 | 287 | | |
282 | 288 | | |
283 | 289 | | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
284 | 295 | | |
285 | 296 | | |
286 | 297 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
7 | 7 | | |
8 | 8 | | |
9 | 9 | | |
| 10 | + | |
10 | 11 | | |
11 | 12 | | |
12 | 13 | | |
| |||
20 | 21 | | |
21 | 22 | | |
22 | 23 | | |
| 24 | + | |
23 | 25 | | |
24 | 26 | | |
25 | 27 | | |
| |||
264 | 266 | | |
265 | 267 | | |
266 | 268 | | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
| 300 | + | |
| 301 | + | |
| 302 | + | |
| 303 | + | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
| 315 | + | |
| 316 | + | |
| 317 | + | |
| 318 | + | |
| 319 | + | |
| 320 | + | |
| 321 | + | |
| 322 | + | |
| 323 | + | |
| 324 | + | |
| 325 | + | |
| 326 | + | |
| 327 | + | |
| 328 | + | |
| 329 | + | |
| 330 | + | |
| 331 | + | |
| 332 | + | |
| 333 | + | |
267 | 334 | | |
268 | 335 | | |
269 | 336 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
272 | 272 | | |
273 | 273 | | |
274 | 274 | | |
275 | | - | |
276 | | - | |
277 | | - | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
278 | 288 | | |
279 | 289 | | |
280 | 290 | | |
| |||
0 commit comments