Skip to content

Copy a session's id, and its transcript, mid-turn (SPEC-52) - #164

Merged
leduckhc merged 11 commits into
mainfrom
feat/get-session-id
Aug 11, 2026
Merged

Copy a session's id, and its transcript, mid-turn (SPEC-52)#164
leduckhc merged 11 commits into
mainfrom
feat/get-session-id

Conversation

@leduckhc

@leduckhc leduckhc commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Typing pi's own /session into makit's composer does not answer you — it is an agent command, so
it falls through to store.sendMessage and, mid-turn, lands in the server's pending queue. It
executes only after the turn it was meant to help you hand off. This branch makes session identity
something makit itself can answer, at 100% of a turn.

What changed

A panel (app/lib/ui/session/session_identity.dart) showing the agent session id, its on-disk
transcript path, a ready-to-paste resume command, and the makit session id — one Copy all, rows
omitted when unmeasured rather than shown as placeholders, read-only, with semantics labels. Per-agent
vocabulary is a lookup table, not a switch, so an unknown agent gets a generic label and no
invented CLI.

Three doors, deliberately not four: the mobile glass menu, the desktop pane-header kebab, and
Copy session id on the desktop tab menu. A tab-menu Session details… was cut on review — it would
have duplicated the pane kebab one pixel away on the same platform.

/session as a CLIENT command, registered next to /name, so handleClientCommand intercepts it
before the wire. Bare /session opens the panel; /session id copies only the bare id. That
interception is the bug fix.

Two optional wire fieldsagentSessionId? and transcriptPath? on SessionDTO — frozen in
their own commit before either implementation, so the parallel server/app work could not both edit the
shared shape. Optional is the decision, not an accident: a newer app against an older server renders
one fewer row instead of a fabricated one. The app normalises '' to null at the decode edge,
because an empty string would render a copy affordance that copies nothing.

Server-side resolution in a new server/src/transcript-path.ts: resumeSessionPath verbatim →
pi-only directory scan → undefined, never throwing. It is keyed on worktreePath ?? project path,
because pi's transcript slug follows the cwd pi actually ran in, which is usually the worktree — the
nearest precedent (attachPiSession) uses the project path and would have missed the transcript for
every worktree-bound session. Memoized per session id including misses, since a snapshot is
rebroadcast on every metaChanged and resolving per projection meant a readdir per session per
broadcast.

Why the full id, and no ~/

The mockup showed pi --session 019ff121. pi session ids are UUIDv7, whose first 48 bits are a
millisecond timestamp, so 8 chars leave ~65s of ambiguity — and real same-prefix collisions exist on
this machine. Driving the real binary settled it: pi --session <ambiguous prefix> does not error,
it silently resolves to one session and offers to fork it. Full id everywhere, and the transcript
suffix match is an exact _<id>.jsonl, never a prefix. The ~/ display abbreviation was dropped
because the path belongs to the server host, whose home dir the app cannot know.

How it was tested

  • server: tsc --noEmit clean; pnpm test 1326 pass / 0 fail (baseline 1313).
  • app: flutter analyze --fatal-infos --no-pub → "No issues found!"; dart format --set-exit-if-changed clean; 0 non-loading failures in flutter test. The 15–17 reported
    failures are all loading <file>, vary run to run, and each passes when run directly — the recorded
    flake baseline, not a regression. 71 SPEC-52 tests green.
  • Mutation-tested, 10 of them, each proven to turn a test red by reverting only the production
    line: cwd → project path fails the worktree test; a prefix-relaxed suffix match fails the
    collision test; handleClientCommand's exact match → startsWith fails "/sessions is NOT
    intercepted". Two mutations that initially did not bite were fixed as real bugs in the tests.
  • Pixel gate on the real macOS app, not a widget test: rendered through
    app/tool/session_identity_demo.dart, geometry read from the accessibility tree with cua-driver,
    both uuid rows measured at one line, Copy all at 10.95:1 contrast. That gate — not any test — is
    what caught the "1 lines" pluralisation bug.
  • Live probe: resolveTranscriptPath against the real ~/.pi/agent/sessions tree, using the pi
    session id of the session that implemented this feature, returned the transcript path
    byte-identical to what pi itself reports. The 8-char prefix returned undefined. Non-pi, draft and
    unreadable-dir inputs returned undefined without throwing.

Scope

P1. Deferred to P2: the codex/threadId resolver branch (additive to transcript-path.ts by design)
and D12's identity section inside the context-usage panel — cut because the ring is absent in exactly
the four states where the id is most wanted.

Spec: docs/specs/2026-08-11-SPEC-52-session-identity.md · plan:
docs/specs/2026-08-11-SPEC-52-PLAN.md · design board: mockups/session-identity.html


Note

Medium Risk
Touches the session wire contract and SessionManager DTO projection, and discloses absolute server filesystem paths to paired clients. Well-covered by tests, but the surface area is large and path resolution sits on the hot snapshot path.

Overview
Makes session identity answerable mid-turn by intercepting /session as a client command (instead of queuing behind the agent) and exposing a read-only identity panel with one Copy all.

Panel + doors. New session_identity.dart shows agent id, transcript path, resume command, and makit id — omitting unmeasured rows. Opened from the mobile glass menu and desktop pane kebab; the tab menu gets Copy session id only (bare id, no dialog). Bare /session opens the panel; /session id copies just the id.

Wire + server. Adds optional agentSessionId / transcriptPath on SessionDTO. The server resolves pi transcripts in transcript-path.ts (exact _<id>.jsonl suffix, keyed on worktree cwd, memoized including misses) and projects them through SessionManager. Empty strings normalize to null at the app codec edge.

Also floors usage-panel width at zero so a shrink-under-open popover cannot assert, and hardens clipboard-failure / disposed-ref handling on all three copy paths.

Reviewed by Cursor Bugbot for commit 8873c6a. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Add session identity panel with clipboard copy for session ID and transcript path

  • Adds a SessionIdentityDetails panel (bottom sheet on mobile, anchored popover on desktop) surfacing session ID, transcript path, agent-specific vocabulary, and resume commands via showSessionIdentity.
  • Exposes session identity through three entry points: the session screen overflow menu, the desktop pane header kebab menu, and a new /session client command (/session id copies the bare agent session ID to clipboard).
  • Desktop tab context menu gains a 'Copy session id' action that writes the agent session ID to the clipboard with success/failure reporting via StatusCenter.
  • Extends SessionDTO and the Session model with agentSessionId and transcriptPath fields; the server resolves transcript paths for pi sessions using exact UUID suffix matching in resolveTranscriptPath.
  • Fixes a layout crash in ContextUsageButton where the popover width could go negative on very small windows.

Macroscope summarized 8873c6a.


Updates since the PR was opened

Merged main (873a121a). Four conflicts, all "both sides added optional fields in the same
place", resolved as a union: SessionDTO now carries SPEC-46's parentId/handoffReason/origin
alongside this branch's two fields, and manager.ts keeps both sides' methods. The question worth
checking was whether SPEC-46's new wire path (server.ts visibleSessions) bypasses
projectSessionDTO and would therefore ship without the two new fields — it does not:
session.toDTO() still has exactly one caller, and visibleSessions filters
manager.listSessions() rather than building DTOs. The two features do interact, and it is benign:
an agent-token principal now gets a per-principal-filtered snapshot, so it sees transcriptPath for
its own session only — inside D21's disclosure and narrower than the paired-device case D21 argued.

Three review threads fixed (8809cfc9), each with a red test first:

  • /session id swallowed a throwing Clipboard.setData. Fixed — and the identical bug in
    split_view.dart's tab-menu copy, which review did not flag
    , because three copy paths in one
    feature must not disagree about what a failed write does. All three now report failure (the
    panel's warning was the odd one out); severity is asserted in all three tests.
  • The tab menu read sessionIdentityProvider after the showMenu await, so closing the tab while
    the menu was open crashed with Bad state: Using "ref" … unmounted is unsafe. Hoisted next to
    status, which is hoisted for that same reason (SPEC-48 D3), rather than guarded with
    context.mounted — the guard would leave ref use after an await, which is the hazard D3 removes.
  • Three stale statements in the mockup, including a delta-table row still promising the fourth door
    D13 cut. A delta table reads as a work list, so that one could have put the door back.

Renumbered SPEC-51 → SPEC-52 (1a0bb8a1). While this branch was in flight, main took both
adjacent numbers: 51 by Preview groups (#163, referenced from docs/UX.md + 14 shipped files) and
50 by Profiles (#162). Neither shipped spec can move, so this one took 52. Renumbered only this
feature's own files, derived from this branch's commit range rather than by blanket search-replace —
a global replace would have silently repointed preview groups' own 14 references at this spec.

Verification re-run on the merged tree (the earlier numbers were pre-merge):

  • server: tsc clean, 2072 pass / 0 fail.
  • app: analyze --fatal-infos clean, dart format --set-exit-if-changed clean, 0 non-loading
    failures
    .
  • Honest caveat: one server test flaked once during this work (strictEqual, expected 4) and did
    not recur in seven subsequent full-suite runs; it is not in this branch's files, and
    transcript-path.test.ts + manager.test.ts were run 12 consecutive times (110 pass) to be sure.
    Flagging rather than burying it.
  • Known follow-up, measured not guessed: broadening status_lifetime_test.dart from ref.status to
    any ref.read/ref.watch after an await would have caught finding feat: rich tool detail views for read/write/bash/grep #2 automatically, but reports
    24 pre-existing bare sites across lib/ — too wide for this PR.

Round 2 (conflicts + five review threads)

Two more review threads arrived on the fixes and both were right, so they are fixed too
(6914b5c3, 8873c6aa):

  • The desktop panel was documented as an "anchored MenuAnchor popover" (D11) but ships a centred
    dialog.
    Kept the centring, with a reason: the mechanism cannot transfer, because the door topology
    differs. ContextUsageButton is a persistent composer control, so a MenuAnchor stays anchored
    to it; both of this panel's desktop doors are transient menu items, so by the time one is chosen
    its menu is gone and there is nothing left to anchor to. D11 amended and logged as deviation 8.
  • window - 2 * margin goes negative below the margin width, and a negative BoxConstraints/
    SizedBox is non-normalized — the layout asserts instead of rendering a cramped panel. Floored at
    zero. The SPEC-37 panel this one copied its sizing from has the same bug on the width axis (16pt
    threshold vs 24pt here; its height axis was already floored), so that is fixed here too, with its
    own red test.

Both threads share one root cause worth naming: neither panel had a single desktop: true test, so
neither desktop host was covered at all. That is how the doc comment and the code drifted apart and
stayed that way through implementation. Four desktop tests added (centred, window-clamped, narrow-window
clamp, degenerate-window no-throw), all mutation-proven.

Two self-corrections from this round, recorded because the near-misses are the lesson:

  • My first degenerate-window test passed while proving nothing — at 20×20 the trigger button is
    unhittable, so the tap landed on nothing and the panel never opened. Both tests now shrink the window
    while the panel is open, which is also the realistic path.
  • I nearly filed the context_usage.dart bug as "not reproducible" by reusing that 20pt window: it is
    negative for a 12pt margin but still positive for an 8pt one. It needed 10pt. A threshold bug needs a
    size derived from the constant, not inherited from the previous test.

All five review threads are resolved with replies. All 13 CI checks pass.

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).
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.
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
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
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.
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.
@cursor

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_64ebd4ae-bb05-4e54-9b39-0b08066b0ec7)

@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@leduckhc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 509fbe1b-0398-407e-a8a0-7df09a0dfdbc

📥 Commits

Reviewing files that changed from the base of the PR and between c05e275 and 8873c6a.

📒 Files selected for processing (25)
  • app/lib/desktop/chat/panes/pane_header.dart
  • app/lib/desktop/chat/split_view.dart
  • app/lib/store/models.dart
  • app/lib/transport/codec.dart
  • app/lib/ui/composer/client_commands.dart
  • app/lib/ui/composer/context_usage.dart
  • app/lib/ui/session/session_identity.dart
  • app/lib/ui/session/session_screen.dart
  • app/test/context_usage_test.dart
  • app/test/session_command_test.dart
  • app/test/session_details_doors_test.dart
  • app/test/session_identity_codec_test.dart
  • app/test/session_identity_provider_test.dart
  • app/test/session_identity_test.dart
  • app/test/session_identity_widget_test.dart
  • app/test/status/status_lifetime_test.dart
  • app/tool/session_identity_demo.dart
  • docs/specs/2026-08-11-SPEC-52-PLAN.md
  • docs/specs/2026-08-11-SPEC-52-session-identity.md
  • mockups/session-identity.html
  • server/src/manager.test.ts
  • server/src/manager.ts
  • server/src/protocol.ts
  • server/src/transcript-path.test.ts
  • server/src/transcript-path.ts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread app/lib/ui/composer/client_commands.dart Outdated
Comment thread mockups/session-identity.html Outdated
Comment thread app/lib/desktop/chat/split_view.dart Outdated
Four conflicts, all of one shape: main and this branch each added optional
fields/methods in the same place. Resolved as a union, main's first.

  protocol.ts   SessionDTO gains SPEC-46's parentId/handoffReason/origin next to
                this branch's agentSessionId/transcriptPath.
  models.dart   the same union in four places -- ctor, fields, copyWith params,
                copyWith body.
  codec.dart    both decode blocks.
  manager.ts    SPEC-46's lineage/readTranscript methods and this branch's
                projectSessionDTO/transcriptPathFor.

Checked rather than assumed: SPEC-46 added a wire path of its own (server.ts
visibleSessions, which filters the snapshot per principal), so the question was
whether any NEW SessionDTO site bypasses projectSessionDTO and would therefore
ship without the two new fields. It does not -- session.toDTO() is still called
from exactly one place, projectSessionDTO, and visibleSessions filters
manager.listSessions() rather than building DTOs itself.

Worth recording because the two features interact: an agent-token principal now
receives a filtered snapshot, so it sees transcriptPath for its own session only.
That is inside D21's disclosure (an agent already knows its own cwd) and narrower
than the paired-device case D21 actually argued.

server: tsc clean, 2065 pass / 0 fail
app:    analyze clean, 0 non-loading failures
…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.
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
@cursor

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_f1fa2a7b-61d7-42ef-8763-df11b81268c8)

@leduckhc leduckhc changed the title Copy a session's id, and its transcript, mid-turn (SPEC-51) Copy a session's id, and its transcript, mid-turn (SPEC-52) Aug 11, 2026
Comment thread app/lib/ui/session/session_identity.dart Outdated
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.
@cursor

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_a0f2b12c-ad49-400f-a864-ca3b66cb30b7)

Comment thread app/lib/ui/session/session_identity.dart
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.
@cursor

cursor Bot commented Aug 11, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_bc9ecd03-7e95-40de-b8e8-45561322f7da)

@leduckhc
leduckhc merged commit e69d750 into main Aug 11, 2026
16 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 11, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant