diff --git a/app/lib/desktop/chat/panes/pane_header.dart b/app/lib/desktop/chat/panes/pane_header.dart index ec14c5bf..df2efa12 100644 --- a/app/lib/desktop/chat/panes/pane_header.dart +++ b/app/lib/desktop/chat/panes/pane_header.dart @@ -9,6 +9,7 @@ import '../../../store/store.dart'; import '../../../status/status_event.dart'; import '../../../status/status_providers.dart'; import '../../../ui/composer/client_commands.dart'; +import '../../../ui/session/session_identity.dart'; import '../../../ui/widgets/menu_item.dart'; import '../sidebar_layout.dart'; import '../title_bar_strip.dart'; @@ -145,6 +146,15 @@ class SessionActionsMenu extends ConsumerWidget { ref: ref, sessionId: sessionId, ); + case 'details': + // Reads state the client already holds (D13) — not capability + // gated. `desktop: true` for the anchored popover, `sessionId` so + // the open panel watches and fills in live (D19). + showSessionIdentity( + context: context, + desktop: true, + sessionId: sessionId, + ); case 'quit': _confirmClose(context, ref); } @@ -155,6 +165,11 @@ class SessionActionsMenu extends ConsumerWidget { icon: PhosphorIconsLight.pencilSimple, label: 'Rename session', ), + themedMenuItem( + value: 'details', + icon: PhosphorIconsLight.fingerprint, + label: 'Session details', + ), const PopupMenuDivider(), themedMenuItem( value: 'quit', diff --git a/app/lib/desktop/chat/split_view.dart b/app/lib/desktop/chat/split_view.dart index 76ece5c5..ab1599d4 100644 --- a/app/lib/desktop/chat/split_view.dart +++ b/app/lib/desktop/chat/split_view.dart @@ -1,4 +1,5 @@ import 'package:flutter/material.dart' hide Tab, Split; +import 'package:flutter/services.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:phosphoricons_flutter/phosphoricons_flutter.dart'; @@ -7,6 +8,7 @@ import '../../status/status_event.dart'; import '../../status/status_providers.dart'; import '../../store/store.dart'; import '../../ui/composer/client_commands.dart'; +import '../../ui/session/session_identity.dart'; import '../../ui/widgets/menu_item.dart'; import 'desktop_chat_pane.dart'; import 'groups/agent_picker.dart'; @@ -665,9 +667,11 @@ class _TabChip extends ConsumerWidget { ); } - /// Tab context menu (right-click / long-press). One item — "Rename session" - /// — styled to the design system's primary body scale (`bodyMedium` text - /// with a matching 16px glyph). + /// Tab context menu (right-click / long-press): **Rename session** and + /// **Copy session id**. Deliberately NOT a *Session details…* item (D13): a + /// third door onto the same sheet, one pixel from the pane-header kebab on the + /// same platform, was cut on review. **Copy session id** stays because it is a + /// different job — right-click → one click → the bare id, no dialog. Future _showContextMenu( BuildContext context, WidgetRef ref, @@ -678,6 +682,23 @@ class _TabChip extends ConsumerWidget { if (overlayState == null) return; final overlayBox = overlayState.context.findRenderObject(); if (overlayBox is! RenderBox) return; + // Resolved before the `showMenu` await (SPEC-48 D3): `ref` dies with its + // widget, and the copy path reports its outcome after an await. + final status = ref.status; + // The identity is hoisted for the SAME reason, and it is not optional care: + // this menu lives in the Navigator's overlay, so it outlives the tab chip + // that opened it. Close the tab while the menu is open — a server snapshot + // dropping the session does it for real — and a `ref.read` down in the + // `copyId` branch would run on a dead `ref` and throw `Cannot use "ref" + // after the widget was disposed`, i.e. crash instead of copying. + // + // The cost is that the id is sampled at menu-open rather than at click. That + // is sub-second for a right-click → click, and it is the RIGHT trade here: + // the live-filling surface is the panel, which watches (D19). Rejected + // alternative: guarding the late read with `context.mounted`, which keeps the + // read fresh but leaves `ref` use after an await — the hazard SPEC-48 D3 + // exists to remove. + final identity = ref.read(sessionIdentityProvider(sessionId)); final selected = await showMenu( context: context, position: RelativeRect.fromRect( @@ -691,8 +712,47 @@ class _TabChip extends ConsumerWidget { icon: PhosphorIconsLight.pencilSimple, label: 'Rename session', ), + themedMenuItem( + value: 'copyId', + icon: PhosphorIconsLight.copy, + label: 'Copy session id', + ), ], ); + if (selected == 'copyId') { + // The BARE agent session id (D6), not `sessionIdentityText` — that whole + // label:value payload is `Copy all`'s job in the panel. No dialog. + final id = identity.agentSessionId; + if (id == null) { + status.warning( + 'No agent session id yet', + source: StatusSources.session, + sessionId: sessionId, + ); + return; + } + // A clipboard write can throw for real (another process holds it on + // Windows; the host denies it). Unreported, the user gets neither the id + // nor a reason. Same contract as the panel's `Copy all` and `/session id`. + try { + await Clipboard.setData(ClipboardData(text: id)); + } catch (e) { + status.failure( + 'Could not copy session id', + error: e, + source: StatusSources.session, + sessionId: sessionId, + ); + return; + } + status.info( + 'Session id copied', + source: StatusSources.session, + detail: id, + sessionId: sessionId, + ); + return; + } if (selected != 'rename' || !context.mounted) return; await handleClientCommand( '/name', diff --git a/app/lib/store/models.dart b/app/lib/store/models.dart index b2eb7a9b..995f038b 100644 --- a/app/lib/store/models.dart +++ b/app/lib/store/models.dart @@ -1237,6 +1237,8 @@ class Session { this.parentId, this.handoffReason, this.origin, + this.agentSessionId, + this.transcriptPath, this.queued = const [], }); @@ -1296,6 +1298,18 @@ class Session { /// Null on pre-SPEC-46 rows; a plain string so an unknown value never throws. final String? origin; + /// The underlying agent's own session id — pi's ACP `sessionId` (which is pi's + /// OWN session uuid, reused by `pi-acp`) or codex's `threadId`. Null for a + /// draft, for a back end with no native session concept, and for any server + /// older than SPEC-52 (D1). + final String? agentSessionId; + + /// Absolute path to the transcript on the SERVER's host, resolved server-side + /// (D2/D3) — the app never derives it, because the slug algorithm is pi's and + /// the app cannot stat the server's filesystem to check itself. Null for codex + /// in P1 (D16) and whenever no file was found (D9). + final String? transcriptPath; + /// Messages submitted while the agent was busy that could not be steered into /// the running turn (SPEC-35), oldest first. They are delivered one per idle /// transition and can be cancelled until then. @@ -1319,6 +1333,8 @@ class Session { String? parentId, String? handoffReason, String? origin, + String? agentSessionId, + String? transcriptPath, List? queued, }) => Session( id: id, @@ -1341,6 +1357,8 @@ class Session { parentId: parentId ?? this.parentId, handoffReason: handoffReason ?? this.handoffReason, origin: origin ?? this.origin, + agentSessionId: agentSessionId ?? this.agentSessionId, + transcriptPath: transcriptPath ?? this.transcriptPath, queued: queued ?? this.queued, ); } diff --git a/app/lib/transport/codec.dart b/app/lib/transport/codec.dart index 5766a3fa..37afa002 100644 --- a/app/lib/transport/codec.dart +++ b/app/lib/transport/codec.dart @@ -274,6 +274,12 @@ class WireCodec { ? j['handoffReason'] as String : null, origin: j['origin'] is String ? j['origin'] as String : null, + // SPEC-52 D1/D9: normalise `''` to null at the edge. A blank string is + // what a sloppy server sends for "no value", and it would render a copy + // affordance that copies nothing — the placeholder D9 forbids. Doing it + // here means nothing above this line has to think about it. + agentSessionId: _nonEmpty(j['agentSessionId']), + transcriptPath: _nonEmpty(j['transcriptPath']), queued: decodeQueued(j['queued']), ), ); @@ -281,6 +287,11 @@ class WireCodec { return out; } + /// A non-empty string, or null. Rejects non-strings too, so a malformed + /// snapshot degrades one field instead of failing the whole session list. + static String? _nonEmpty(Object? v) => + (v is String && v.isNotEmpty) ? v : null; + /// Decode a session's `queued` array (SPEC-35). Absent/malformed entries yield /// an empty queue rather than failing the whole snapshot: a session list is /// too important to drop over a pending-message chip. diff --git a/app/lib/ui/composer/client_commands.dart b/app/lib/ui/composer/client_commands.dart index 9fe4b03e..e0143ea6 100644 --- a/app/lib/ui/composer/client_commands.dart +++ b/app/lib/ui/composer/client_commands.dart @@ -7,6 +7,7 @@ library; import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:go_router/go_router.dart'; import 'package:phosphoricons_flutter/phosphoricons_flutter.dart'; @@ -20,6 +21,7 @@ import '../../status/status_event.dart'; import '../../status/status_providers.dart'; import '../widgets/sheet_header.dart'; import '../widgets/searchable_list_sheet.dart'; +import '../session/session_identity.dart'; import '../../app/routes.dart'; typedef ClientCmdHandler = @@ -264,6 +266,71 @@ final List clientCommands = [ ); }, ), + ClientCommand( + name: 'session', + description: 'Show this session’s identity, or /session id to copy its id', + handler: (context, ref, {required sessionId, required arg}) async { + // WHY a CLIENT command and not sent to the agent (D7): pi's own `/session` + // is an agent command, so in makit's composer it would fall through to + // `store.sendMessage` and — mid-turn — land in the server's pending queue, + // executing only after the turn it was meant to help you hand off. + // Intercepting it here answers at 100% of a turn. This handler returning + // (via `handleClientCommand` matching) is the fix for that bug. + // + // Resolved before any await (SPEC-48 D3, enforced by + // `test/status/status_lifetime_test.dart`): `ref` dies with its widget. + final status = ref.status; + // `/session id` copies ONLY the bare agent session id (D6). The panel's + // `Copy all` is the "give me everything" job; this is "give me the id", so + // it must not emit the whole label:value payload. + if (arg == 'id') { + final id = ref.read(sessionIdentityProvider(sessionId)).agentSessionId; + if (id == null) { + // Say why rather than copying an empty string: a draft (or a back end + // with no native session concept) has no id to hand off yet. + status.warning( + 'No agent session id yet', + source: StatusSources.session, + sessionId: sessionId, + ); + return; + } + // A clipboard write can throw for real (another process holds it on + // Windows; the host denies it). Unreported, the user gets neither the id + // nor a reason — so the write is waited on, and only a write that landed + // is allowed to claim success. Same contract as the panel's `Copy all`. + try { + await Clipboard.setData(ClipboardData(text: id)); + } catch (e) { + status.failure( + 'Could not copy session id', + error: e, + source: StatusSources.session, + sessionId: sessionId, + ); + return; + } + status.info( + 'Session id copied', + source: StatusSources.session, + detail: id, + sessionId: sessionId, + ); + return; + } + // Bare `/session` opens the panel. Presented as a bottom sheet + // (`desktop: false`) like the other client commands (`/model`, + // `/thinking`): the invocation comes from the composer, where a sheet is + // the established surface. `sessionId` is passed so the open panel watches + // and fills in live (D19). + if (!context.mounted) return; + await showSessionIdentity( + context: context, + desktop: false, + sessionId: sessionId, + ); + }, + ), ClientCommand( name: 'name', description: 'Rename this session (shown in the session list)', diff --git a/app/lib/ui/composer/context_usage.dart b/app/lib/ui/composer/context_usage.dart index bb60933e..a60ba6ad 100644 --- a/app/lib/ui/composer/context_usage.dart +++ b/app/lib/ui/composer/context_usage.dart @@ -272,9 +272,19 @@ class ContextUsageButton extends ConsumerWidget { ), ), child: SizedBox( - width: math.min( - kUsagePanelWidth, - window.width - 2 * _kUsagePanelMargin, + // Floored at zero: `window.width - 2 * margin` goes negative below + // 16pt, and a negative SizedBox width is a non-normalized + // constraint — the layout asserts instead of rendering a cramped + // panel. Reachable for a frame when the window shrinks under an + // open popover. The height axis above was already safe, floored by + // `_kUsagePanelMinHeight`; this axis had no floor. + // (Found via SPEC-52's identity panel, which copied this sizing.) + width: math.max( + 0, + math.min( + kUsagePanelWidth, + window.width - 2 * _kUsagePanelMargin, + ), ), // Scrolls inside the height cap rather than clipping the cost // line off the bottom. `primary: false` because `MenuAnchor` diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart new file mode 100644 index 00000000..2b1b6ec8 --- /dev/null +++ b/app/lib/ui/session/session_identity.dart @@ -0,0 +1,543 @@ +// SPEC-52 — session identity: the underlying agent session id, its transcript +// path, and the one-button copy that gets them somewhere useful. +// +// The problem this exists for: pi's own `/session` is an AGENT command, so in +// makit's composer it falls through to `store.sendMessage` and — mid-turn — +// lands in the server's pending queue, executing only after the turn it was +// meant to help you hand off. Everything here is reachable without touching the +// wire, so it answers at 100% of a turn. +// +// Layering, deliberately: +// * `SessionIdentity` — an app-level value type. The widgets never see +// `SessionDTO`'s field names, which is what let +// this whole file (and its pixel sign-off) land +// before the wire contract was frozen. +// * `sessionIdentityText` — pure. THE copy contract, in one place, shared +// verbatim by the panel, `/session` and the menus. +// Same seam as `context_usage.dart`'s formatters. +// * `SessionIdentityDetails` / `showSessionIdentity` — host-agnostic body plus +// a sheet/popover split, mirroring +// `ContextUsageButton` + `ContextUsageDetails`. +// +// Design reference: `mockups/session-identity.html`. +import 'dart:math' as math; + +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:phosphoricons_flutter/phosphoricons_flutter.dart'; + +import '../../app/theme.dart'; +import '../../status/status_event.dart'; +import '../../status/status_providers.dart'; +import '../../store/store.dart'; + +// ─── per-agent vocabulary (D10) ────────────────────────────────────────────── + +/// What one agent calls its session, and how its CLI resumes one. +/// +/// A record in a lookup table rather than a `switch`, because +/// `docs/ENGINEERING.md`'s open/closed rule is explicit that adding an adapter +/// must not mean editing a growing `switch`. +@immutable +class AgentSessionVocabulary { + const AgentSessionVocabulary({required this.label, required this.resume}); + + /// Row label, in the agent's own noun — codex calls it a thread, so we do. + final String label; + + /// Builds the resume command for [id], or null when this agent has none. + final String Function(String id)? resume; +} + +/// The agents whose CLI we have actually verified. Anything absent falls to +/// [_unknownAgentVocabulary] — the escape hatch that keeps this open/closed. +/// +/// Both forms were checked against the real binaries: `pi --help` documents +/// `--session `, and `codex resume --help` documents +/// `codex resume [SESSION_ID]`. +final Map kAgentSessionVocabulary = { + 'pi': AgentSessionVocabulary( + label: 'pi session', + resume: (id) => 'pi --session $id', + ), + 'codex': AgentSessionVocabulary( + label: 'Thread', + resume: (id) => 'codex resume $id', + ), + // codex's own legacy alias, per `transportFor` on the server. + 'codex-native': AgentSessionVocabulary( + label: 'Thread', + resume: (id) => 'codex resume $id', + ), +}; + +/// No resume line for an agent we have never seen: inventing a CLI invocation +/// is the same failure as inventing a path (D9), just harder to notice, because +/// it looks copy-pasteable. +const _unknownAgentVocabulary = AgentSessionVocabulary( + label: 'Agent session', + resume: null, +); + +// ─── the store seam (C2a) ──────────────────────────────────────────────────── + +/// Maps the store's [Session] to the [SessionIdentity] the panel watches (D19). +/// +/// It returns a `SessionIdentity` with null FIELDS — never null itself — and +/// never throws. Rationale: the panel always has something to show (the makit +/// session id at minimum), so a null provider would force every call site to +/// branch on it. +/// +/// An UNKNOWN session id echoes the requested id back as [makitSessionId] with +/// an unknown agent, rather than throwing or returning null: that is truthful +/// (this client holds no record of it) and, like the empty case, keeps the +/// panel openable without a null check. A draft (no `agent` yet) falls back to +/// its `pendingAgent` for the label, matching `StoreController._cacheCommands`. +final sessionIdentityProvider = Provider.family(( + ref, + sessionId, +) { + final session = ref.watch(sessionsProvider).byId(sessionId); + final agent = session == null + ? '' + : (session.agent.isNotEmpty + ? session.agent + : (session.pendingAgent ?? '')); + return SessionIdentity.from( + agent: agent, + makitSessionId: sessionId, + agentSessionId: session?.agentSessionId, + transcriptPath: session?.transcriptPath, + ); +}); + +// ─── the value type ────────────────────────────────────────────────────────── + +/// Everything the identity panel can say about one session. +/// +/// Fields are nullable and each is independently absent-able: a draft has no +/// agent id, codex has no transcript path in P1, and a stub adapter has neither. +/// Absent means *omitted*, never blank and never a placeholder (D9) — a +/// fabricated path is worse than no path, because it will be pasted into a +/// prompt and the next agent will report it missing. +@immutable +class SessionIdentity { + const SessionIdentity({ + required this.makitSessionId, + required this.agentLabel, + this.agentSessionId, + this.transcriptPath, + this.resumeCommand, + }); + + /// Derives the per-agent vocabulary (D10) and the resume command (D15). + factory SessionIdentity.from({ + required String agent, + required String makitSessionId, + String? agentSessionId, + String? transcriptPath, + }) { + final vocab = kAgentSessionVocabulary[agent] ?? _unknownAgentVocabulary; + final id = _blankToNull(agentSessionId); + return SessionIdentity( + makitSessionId: makitSessionId, + agentLabel: vocab.label, + agentSessionId: id, + transcriptPath: _blankToNull(transcriptPath), + // The FULL id, never a prefix (D15). pi documents `--session` as taking a + // "partial UUID" and it is tempting to shorten for display, but pi ids are + // UUIDv7: the first 48 bits are a millisecond timestamp, so an 8-char + // prefix pins only the top 32 bits and leaves ~65 s of collisions. Two + // real sessions on the author's machine share `019fa9f4`, and pi does not + // error on the ambiguity — it silently picks one and offers to fork it. + resumeCommand: (id == null || vocab.resume == null) + ? null + : vocab.resume!(id), + ); + } + + /// makit's own session uuid. Always present, and last in the panel: it is only + /// ever needed for a bug report. + final String makitSessionId; + + /// Row label for [agentSessionId], in the agent's own noun (D10). + final String agentLabel; + + /// The native ACP `sessionId` / codex `threadId`. For pi this is pi's OWN + /// session uuid — `pi-acp` reuses it as the ACP session id — so it is exactly + /// what pi's `/session` prints and what `pi --session` accepts. + final String? agentSessionId; + + /// Absolute path to the transcript on the SERVER's host (D4/D21). Absolute + /// because the receiver is another agent's shell or prompt, where `~` only + /// expands if a shell gets there first — and because the app cannot know the + /// server host's home directory to abbreviate it honestly. + final String? transcriptPath; + + /// A ready-to-paste resume invocation, or null when this agent has no known + /// CLI or this session has no id. + final String? resumeCommand; + + /// True when the agent has not produced an id yet — a draft, or a session + /// whose back end has no native session concept. + bool get hasAgentSession => agentSessionId != null; + + static String? _blankToNull(String? v) => (v == null || v.isEmpty) ? null : v; +} + +// ─── the copy contract (D14) ───────────────────────────────────────────────── + +/// The clipboard payload: one `label: value` per line, labels padded into a +/// shared column, absent rows omitted. +/// +/// Pure, and the single source of this format, because it has four callers (the +/// panel, `/session`, and two menus) and a format that differs between them is +/// a bug nobody would notice. +/// +/// Plain lines rather than markdown or JSON: this gets pasted into prompts, +/// commit messages, issues and terminal comments, none of which want fences or +/// escaping. +String sessionIdentityText(SessionIdentity identity) { + final rows = <(String, String)>[ + if (identity.agentSessionId case final id?) + (_clipboardLabel(identity.agentLabel), id), + if (identity.transcriptPath case final path?) ('transcript', path), + if (identity.resumeCommand case final cmd?) ('resume', cmd), + ('makit session', identity.makitSessionId), + ]; + final width = rows.map((r) => r.$1.length).fold(0, (a, b) => math.max(a, b)); + return rows.map((r) => '${r.$1.padRight(width)}: ${r.$2}').join('\n'); +} + +/// Lower-cases the display label for the clipboard, so the payload reads as +/// data (`pi session: …`) rather than as UI chrome. +String _clipboardLabel(String displayLabel) => displayLabel.toLowerCase(); + +// ─── the panel ─────────────────────────────────────────────────────────────── + +/// The one copy affordance (D5). Named so the widget test can assert there is +/// exactly one of it — four per-row buttons were designed, measured and +/// superseded, and that assertion is what stops them coming back. +const IconData kSessionIdentityCopyIcon = PhosphorIconsLight.copy; + +/// Shown instead of an id row when the agent has not produced one yet. One line +/// that says so, rather than a row with an empty value (D9). +const String kSessionIdentityNoAgentLine = 'Agent not started yet'; + +/// Panel width on desktop, matching `kUsagePanelWidth`'s role in SPEC-37. +const double kIdentityPanelWidth = 340; + +/// Margin kept clear of the window edge when clamping the desktop popover. +const double _kIdentityPanelMargin = 12; + +/// Host-agnostic body of the identity panel: the rows, then one `Copy all`. +/// +/// Takes a [SessionIdentity] rather than a session id so it can be rendered by +/// the QA harness, by a widget test, and by both hosts without a store. The +/// caller is responsible for watching (D19) — `showSessionIdentity` does. +class SessionIdentityDetails extends ConsumerWidget { + const SessionIdentityDetails({super.key, required this.identity}); + + final SessionIdentity identity; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final theme = Theme.of(context); + final cs = theme.colorScheme; + final rows = [ + if (identity.agentSessionId case final id?) + _IdentityRow(label: identity.agentLabel, value: id) + else + _AbsentAgentLine(), + if (identity.transcriptPath case final path?) + _IdentityRow(label: 'Transcript', value: path), + if (identity.resumeCommand case final cmd?) + _IdentityRow(label: 'Resume with', value: cmd), + _IdentityRow(label: 'makit session', value: identity.makitSessionId), + ]; + + return Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + Padding( + padding: const EdgeInsets.fromLTRB(kSpace12, kSpace12, kSpace12, 0), + child: Text( + 'Session', + style: theme.textTheme.labelSmall?.copyWith( + color: cs.onSurfaceVariant, + letterSpacing: 0.8, + ), + ), + ), + Padding( + padding: const EdgeInsets.symmetric( + horizontal: kSpace12, + vertical: kSpace10, + ), + child: Column( + crossAxisAlignment: CrossAxisAlignment.stretch, + children: rows, + ), + ), + _CopyAllRow(identity: identity), + ], + ); + } +} + +/// One stacked label-over-value row. +/// +/// Stacked rather than side-by-side because dropping the per-row copy column AND +/// the fixed label column returns ~116px to the value, which is what lets a +/// 36-char uuid sit on one line — and a uuid wrapped mid-string is the worst +/// thing this panel could do, since it invites a partial selection. +class _IdentityRow extends StatelessWidget { + const _IdentityRow({required this.label, required this.value}); + + final String label; + final String value; + + @override + Widget build(BuildContext context) { + final theme = Theme.of(context); + final cs = theme.colorScheme; + return Padding( + padding: const EdgeInsets.symmetric(vertical: kSpace4), + // One semantics node for the pair (D18): a screen reader should say + // "pi session, 019f…" rather than announcing a bare uuid with no context. + child: Semantics( + label: '$label, $value', + excludeSemantics: true, + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Text( + label, + style: theme.textTheme.labelSmall?.copyWith( + color: cs.onSurfaceVariant, + ), + ), + const SizedBox(height: kSpace2), + Text(value, style: theme.textTheme.bodySmall?.mono), + ], + ), + ), + ); + } +} + +/// The "no id yet" line for a draft (D9). +class _AbsentAgentLine extends StatelessWidget { + @override + Widget build(BuildContext context) { + final theme = Theme.of(context); + return Padding( + padding: const EdgeInsets.symmetric(vertical: kSpace4), + child: Text( + kSessionIdentityNoAgentLine, + style: theme.textTheme.bodySmall?.copyWith( + color: theme.colorScheme.onSurfaceVariant, + fontStyle: FontStyle.italic, + ), + ), + ); + } +} + +/// `N lines`, or `1 line`. Trivial, and it shipped wrong: the first build read +/// "1 lines" on a stub/detached session whose only measured value is the makit +/// id. Caught by the pixel gate on the real macOS app, not by a test — the tests +/// only exercised the 4- and 3-line cases — so the 1-line case is now asserted. +String sessionIdentityLineCountLabel(int lines) => + lines == 1 ? '1 line' : '$lines lines'; + +/// `Copy all` — the panel's single copy affordance (D5). +class _CopyAllRow extends ConsumerWidget { + const _CopyAllRow({required this.identity}); + + final SessionIdentity identity; + + @override + Widget build(BuildContext context, WidgetRef ref) { + final theme = Theme.of(context); + final cs = theme.colorScheme; + final payload = sessionIdentityText(identity); + final lines = payload.split('\n').length; + return Semantics( + button: true, + // Says WHAT and HOW MUCH before the tap (D18): a screen-reader user cannot + // see the toast that confirms it afterwards. + label: 'Copy session details, ${sessionIdentityLineCountLabel(lines)}', + excludeSemantics: true, + child: InkWell( + onTap: () => _copy(ref, payload, lines), + child: Container( + height: 38, + padding: const EdgeInsets.symmetric(horizontal: kSpace12), + decoration: BoxDecoration( + border: Border(top: BorderSide(color: cs.outlineVariant)), + ), + child: Row( + children: [ + Icon(kSessionIdentityCopyIcon, size: 16, color: cs.primary), + const SizedBox(width: kSpace10), + Text( + 'Copy all', + style: theme.textTheme.bodyMedium?.copyWith(color: cs.primary), + ), + const Spacer(), + Text( + sessionIdentityLineCountLabel(lines), + style: theme.textTheme.labelSmall?.copyWith( + color: cs.onSurfaceVariant, + ), + ), + ], + ), + ), + ), + ); + } + + Future _copy(WidgetRef ref, String payload, int lines) async { + // `ref.status` is resolved BEFORE the await (SPEC-48 D3, enforced by + // `test/status/status_lifetime_test.dart`): a `StatusCenter` never expires, + // but `ref` dies with its widget — and this panel is a sheet that can be + // dismissed mid-flight, which is exactly when there is bad news to deliver. + final status = ref.status; + // The toast is posted only AFTER the write resolves. Reporting "copied" + // before knowing it landed would be a success claim on a waited path. + try { + await Clipboard.setData(ClipboardData(text: payload)); + } catch (e) { + // `failure`, not `warning`: an action the user asked for did not happen. + // All three copy paths in this feature (panel `Copy all`, `/session id`, + // the tab menu) report the same way, so severity is a property of the + // event rather than of which door was used. + status.failure( + 'Could not copy session details', + error: e, + source: StatusSources.session, + ); + return; + } + status.info( + 'Session details copied', + source: StatusSources.session, + detail: sessionIdentityLineCountLabel(lines), + ); + } +} + +/// Opens the identity panel: a modal bottom sheet on mobile, a **centred, +/// window-clamped** panel on desktop. +/// +/// Centred on purpose, and NOT the `MenuAnchor` popover D11 first specified. +/// D11 said "verbatim the `ContextUsageButton` split", but the mechanism cannot +/// transfer, because the door topology is different: `ContextUsageButton` is a +/// persistent control in the composer, so a `MenuAnchor` has something to anchor +/// to for as long as the popover is open. Both of this panel's desktop doors are +/// transient MENU ITEMS (the pane-header kebab, the mobile glass menu) — by the +/// time an item is chosen its menu has been dismissed, so there is nothing left +/// on screen to anchor to, and anchoring to where a vanished item used to be is +/// arbitrary placement dressed up as precision. +/// +/// What was kept from SPEC-37 is the part that matters and is testable: the +/// window-clamped width and the `SingleChildScrollView`, so a panel opened from a +/// narrow split pane cannot hang off-screen. Both properties are pinned by tests +/// in `test/session_identity_widget_test.dart` ("desktop opens a centred, +/// window-clamped panel"), which exist because the desktop host previously had no +/// test at all — which is exactly how the code and this comment drifted apart. +/// +/// Pass EXACTLY ONE of [sessionId] or [identity]: +/// +/// * [sessionId] — the production path. The panel WATCHES +/// [sessionIdentityProvider] and rebuilds live (D19): a draft's panel can be +/// opened before the adapter assigns `agentSessionId`, and that assignment +/// fans out a fresh snapshot, so a watching panel fills in its rows while +/// open rather than lying until reopened. +/// * [identity] — the store-free path, for the QA harness and widget tests. +/// +/// Exactly one, asserted, rather than "both, and one silently wins": the first +/// wiring took a required [identity] *and* an optional [sessionId], so all three +/// doors did a redundant `ref.read` whose result was then discarded. An argument +/// that is ignored depending on another argument is a trap, not an API. +Future showSessionIdentity({ + required BuildContext context, + required bool desktop, + String? sessionId, + SessionIdentity? identity, +}) { + assert( + (sessionId == null) != (identity == null), + 'pass exactly one of sessionId (watches the store) or identity (static)', + ); + Widget body() => sessionId == null + ? SessionIdentityDetails(identity: identity!) + : _WatchedIdentity(sessionId: sessionId); + if (!desktop) { + return showModalBottomSheet( + context: context, + showDragHandle: true, + // Scrollable because the sheet's height is capped by the window while the + // panel's height depends on how far the transcript path wraps — which on a + // 320pt phone is four lines. + builder: (_) => SafeArea(child: SingleChildScrollView(child: body())), + ); + } + return showDialog( + context: context, + barrierColor: Colors.transparent, + builder: (dialogContext) { + final window = MediaQuery.sizeOf(dialogContext); + final cs = Theme.of(dialogContext).colorScheme; + return Align( + alignment: Alignment.center, + child: Material( + color: cs.surface, + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(kRadius12), + side: BorderSide(color: cs.outlineVariant), + ), + clipBehavior: Clip.antiAlias, + // Sized to the WINDOW, not to a constant: SPEC-37 learned that a fixed + // panel opened from a narrow split pane hangs off-screen. + // + // Floored at zero because `window - 2 * margin` goes NEGATIVE below + // 24pt, and a BoxConstraints with a negative max is not normalized — the + // layout asserts rather than rendering a small panel. Degenerate sizes + // are reachable for a frame (a resize animation, an embedded host), and + // "the window is absurd" should cost a cramped panel, not a crash. + child: ConstrainedBox( + constraints: BoxConstraints( + maxWidth: math.max( + 0, + math.min( + kIdentityPanelWidth, + window.width - 2 * _kIdentityPanelMargin, + ), + ), + maxHeight: math.max(0, window.height - 2 * _kIdentityPanelMargin), + ), + child: SingleChildScrollView(child: body()), + ), + ), + ); + }, + ); +} + +/// Renders [SessionIdentityDetails] against a WATCHED identity, so the open +/// panel fills in live when the agent id is assigned underneath it (D19). +class _WatchedIdentity extends ConsumerWidget { + const _WatchedIdentity({required this.sessionId}); + + final String sessionId; + + @override + Widget build(BuildContext context, WidgetRef ref) => SessionIdentityDetails( + identity: ref.watch(sessionIdentityProvider(sessionId)), + ); +} diff --git a/app/lib/ui/session/session_screen.dart b/app/lib/ui/session/session_screen.dart index c7e02803..d35a0d0a 100644 --- a/app/lib/ui/session/session_screen.dart +++ b/app/lib/ui/session/session_screen.dart @@ -27,6 +27,7 @@ import 'navigator/message_navigator_overlay.dart'; import 'navigator/messages_sheet.dart'; import 'navigator/transcript_jumper.dart'; import 'session_pr_chip.dart'; +import 'session_identity.dart'; import 'transcript_list.dart'; import '../../app/routes.dart'; import '../widgets/pr_signals.dart'; @@ -546,6 +547,16 @@ class _SessionScreenState extends ConsumerState { sessionId: widget.sessionId, jumper: _jumper, ); + case 'details': + // Reads state the client already holds — not capability gated, + // so grouped with Rename / My messages (D13). `desktop: false` + // for the mobile bottom sheet; `sessionId` so the open panel + // watches and fills in live (D19). + showSessionIdentity( + context: context, + desktop: false, + sessionId: widget.sessionId, + ); case 'close': _confirmClose(); } @@ -580,6 +591,15 @@ class _SessionScreenState extends ConsumerState { icon: PhosphorIconsLight.listMagnifyingGlass, label: 'My messages', ), + // SPEC-52 D13: the identity panel's mobile door. Grouped with + // Rename / My messages because it too reads state the client + // already holds (the makit id at minimum) and is not capability + // gated — it works on every agent. + themedMenuItem( + value: 'details', + icon: PhosphorIconsLight.fingerprint, + label: 'Session details', + ), if (canModel) themedMenuItem( value: 'model', diff --git a/app/test/context_usage_test.dart b/app/test/context_usage_test.dart index a369b4f2..f133abd7 100644 --- a/app/test/context_usage_test.dart +++ b/app/test/context_usage_test.dart @@ -318,5 +318,32 @@ void main() { expect(find.text(r'$0.18'), findsOneWidget); expect(find.text('Session total'), findsNothing); }); + + testWidgets('the desktop popover survives the window shrinking under it', ( + tester, + ) async { + // Found while fixing the same bug in SPEC-52's identity panel, which copied + // this sizing: `window.width - 2 * margin` goes NEGATIVE below 16pt (2 * _kUsagePanelMargin), and a + // SizedBox with a negative width is a non-normalized constraint -- the + // layout ASSERTS instead of rendering a cramped panel. The height axis was + // already safe (floored by _kUsagePanelMinHeight); the width axis was not. + // + // Shrunk WHILE open, because that is both the realistic path (a resize + // animation or an embedded host hands us one degenerate frame) and the only + // one that works: at 10x10 the ring is unhittable, so the tap lands on + // nothing and the test would assert nothing. + tester.view.physicalSize = const Size(1400, 900); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + final c = _container(_codex); + addTearDown(c.dispose); + await tester.pumpWidget(_wrap(c, desktop: true)); + await tester.tap(find.byType(ContextUsageRing)); + await tester.pumpAndSettle(); + expect(find.byType(ContextUsageDetails), findsOneWidget); + tester.view.physicalSize = const Size(10, 10); + await tester.pump(); + expect(tester.takeException(), isNull); + }); }); } diff --git a/app/test/session_command_test.dart b/app/test/session_command_test.dart new file mode 100644 index 00000000..cb766cdd --- /dev/null +++ b/app/test/session_command_test.dart @@ -0,0 +1,226 @@ +// SPEC-52 C2b — the `/session` CLIENT command (D7). +// +// This is the entire point of the feature. pi's own `/session` is an AGENT +// command, so typed into makit's composer it falls through to +// `store.sendMessage` and — mid-turn — lands in the server's pending queue, +// executing only after the turn it was meant to help you hand off. Registering +// it in `clientCommands` makes `handleClientCommand` intercept it BEFORE the +// wire, so it works mid-turn. +// +// The single load-bearing assertion is `handleClientCommand('/session')` +// returning TRUE: that one boolean encodes the entire bug report — intercepted, +// never sent. +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import 'package:makit/status/status_center.dart'; +import 'package:makit/status/status_event.dart'; +import 'package:makit/status/status_providers.dart'; +import 'package:makit/store/models.dart'; +import 'package:makit/store/store.dart'; +import 'package:makit/ui/composer/client_commands.dart'; + +const kAgentId = '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'; + +Session _session({String? agentSessionId = kAgentId}) => Session( + id: 's1', + projectId: 'p1', + agent: 'pi', + title: 'Session', + status: SessionStatus.idle, + policy: ApprovalPolicy.askOnRisky, + agentSessionId: agentSessionId, +); + +/// Records clipboard writes; the platform channel is the only place a bare id +/// copy is observable. +class _Clipboard { + final List writes = []; + + /// Make the platform channel throw, the way a real clipboard does when another + /// process holds it (Windows) or the host denies the write. + bool fail = false; + + void install(WidgetTester tester) { + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'Clipboard.setData') { + if (fail) { + throw PlatformException(code: 'copy_failed', message: 'denied'); + } + writes.add((call.arguments as Map)['text'] as String); + } + return null; + }, + ); + } + + void remove(WidgetTester tester) => tester.binding.defaultBinaryMessenger + .setMockMethodCallHandler(SystemChannels.platform, null); +} + +/// Runs [raw] through `handleClientCommand` inside a real widget tree. Returns +/// the pending future WRAPPED in a record so it is not flattened/awaited here — +/// bare `/session` opens a modal sheet whose future only completes on dismissal, +/// so callers pump, assert, dismiss, then await `.handled` for the boolean. +Future<({Future handled})> _run( + WidgetTester tester, + String raw, { + required Session session, + StatusCenter? status, +}) async { + late Future pending; + await tester.pumpWidget( + ProviderScope( + overrides: [ + sessionsProvider.overrideWithValue(SessionsState([session])), + if (status != null) statusCenterProvider.overrideWithValue(status), + ], + child: MaterialApp( + home: Consumer( + builder: (context, ref, _) => Scaffold( + body: TextButton( + onPressed: () => pending = handleClientCommand( + raw, + context: context, + ref: ref, + sessionId: session.id, + ), + child: const Text('run'), + ), + ), + ), + ), + ), + ); + await tester.tap(find.text('run')); + await tester.pump(); + return (handled: pending); +} + +/// Dismisses an open modal sheet by tapping its barrier, so a pending +/// `handleClientCommand` future can complete. +Future _dismissSheet(WidgetTester tester) async { + await tester.tapAt(const Offset(10, 10)); + await tester.pumpAndSettle(); +} + +void main() { + late _Clipboard clipboard; + setUp(() => clipboard = _Clipboard()); + + testWidgets('bare /session is intercepted (never sent to the agent)', ( + tester, + ) async { + // THE bug report, as one assertion: a `true` return means the send path + // does not reach `store.sendMessage`, so `/session` no longer queues behind + // the running turn. + final r = await _run(tester, '/session', session: _session()); + await _dismissSheet(tester); + expect(await r.handled, isTrue); + }); + + testWidgets('bare /session opens the panel and does NOT copy', ( + tester, + ) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + await _run(tester, '/session', session: _session()); + expect(find.text(kAgentId), findsOneWidget); // panel is showing the id + expect(clipboard.writes, isEmpty); // copying is /session id's job (D6) + await _dismissSheet(tester); + }); + + testWidgets('/session id copies EXACTLY the bare id and nothing else', ( + tester, + ) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + addTearDown(status.dispose); + final r = await _run( + tester, + '/session id', + session: _session(), + status: status, + ); + expect(await r.handled, isTrue); + expect(clipboard.writes, [kAgentId]); // the bare id, not the whole payload + expect(status.events.map((e) => e.title), contains('Session id copied')); + }); + + testWidgets('/session id with no agent id copies nothing and says why', ( + tester, + ) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + addTearDown(status.dispose); + await _run( + tester, + '/session id', + session: _session(agentSessionId: null), + status: status, + ); + expect(clipboard.writes, isEmpty); + expect( + status.events.map((e) => e.title), + contains('No agent session id yet'), + ); + }); + + testWidgets('/session id reports a clipboard failure instead of throwing', ( + tester, + ) async { + // A `Clipboard.setData` that throws (another process holds the clipboard on + // Windows; the host denies the write) must not escape the handler: the user + // would then get neither the id nor any word about why. Asserted against the + // STATUS BUS, because the test host has no toast overlay — a rendered-text + // assertion here could never fail. + clipboard + ..install(tester) + ..fail = true; + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + addTearDown(status.dispose); + final r = await _run( + tester, + '/session id', + session: _session(), + status: status, + ); + expect(await r.handled, isTrue); + expect( + tester.takeException(), + isNull, + reason: 'the PlatformException must not escape the handler', + ); + expect(clipboard.writes, isEmpty); + expect( + status.events.map((e) => e.title), + isNot(contains('Session id copied')), + reason: 'a failed write must never claim success', + ); + final failed = status.events.where( + (e) => e.title == 'Could not copy session id', + ); + expect(failed, hasLength(1)); + expect( + failed.single.severity, + StatusSeverity.failure, + reason: 'an action the user asked for did not happen', + ); + }); + + testWidgets('/sessions (a different word) is NOT intercepted', ( + tester, + ) async { + // Matching is exact, not prefix-based: `/sessions` must fall through to the + // agent. The "make the matcher prefix-based" mutation flips this to true. + final r = await _run(tester, '/sessions', session: _session()); + expect(await r.handled, isFalse); + }); +} diff --git a/app/test/session_details_doors_test.dart b/app/test/session_details_doors_test.dart new file mode 100644 index 00000000..43bf3322 --- /dev/null +++ b/app/test/session_details_doors_test.dart @@ -0,0 +1,298 @@ +// SPEC-52 C2c — the two panel DOORS (D13) and the tab menu's Copy session id +// (D6). The desktop tab menu deliberately does NOT get a *Session details* item +// — a third door onto the same sheet, one pixel from the pane kebab on the same +// platform, was cut on review; Copy session id is a different job. +// +// The MUTATION guard here is the tab menu's bare-id assertion: pointing Copy +// session id at `sessionIdentityText` (the whole label:value payload) instead +// of the bare id must fail `clipboard.writes == [kAgentId]`. +// +// ignore_for_file: depend_on_referenced_packages +import 'package:flutter/material.dart' hide Tab, Split; +import 'package:flutter/gestures.dart' show kSecondaryButton; +import 'package:flutter/services.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import 'package:makit/desktop/chat/desktop_chat_pane.dart'; +import 'package:makit/desktop/chat/panes/split_node.dart'; +import 'package:makit/desktop/chat/panes/workspace_controller.dart'; +import 'package:makit/desktop/chat/split_tree_view.dart'; +import 'package:makit/status/status_center.dart'; +import 'package:makit/status/status_event.dart'; +import 'package:makit/status/status_providers.dart'; +import 'package:makit/store/connection.dart'; +import 'package:makit/store/models.dart'; +import 'package:makit/store/secure_store.dart'; +import 'package:makit/store/store.dart'; +import 'package:makit/ui/session/session_identity.dart'; +import 'package:makit/ui/session/session_screen.dart'; + +const kAgentId = '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'; + +class _EmptyStorage implements SecureStore { + const _EmptyStorage(); + @override + Future read({required String key}) async => null; + @override + Future write({required String key, required String? value}) async {} + @override + Future delete({required String key}) async {} +} + +Session _session( + String id, + String title, { + String? agentSessionId = kAgentId, +}) => Session( + id: id, + projectId: 'p1', + agent: 'pi', + title: title, + status: SessionStatus.idle, + policy: ApprovalPolicy.askOnRisky, + lastPreview: '', + lastActivityAt: 0, + worktreePath: '/tmp/wt-a', + branch: 'feat/x', + agentSessionId: agentSessionId, +); + +class _Clipboard { + final List writes = []; + + /// Make the platform channel throw, the way a real clipboard does when another + /// process holds it (Windows) or the host denies the write. + bool fail = false; + + void install(WidgetTester tester) { + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'Clipboard.setData') { + if (fail) { + throw PlatformException(code: 'copy_failed', message: 'denied'); + } + writes.add((call.arguments as Map)['text'] as String); + } + return null; + }, + ); + } + + void remove(WidgetTester tester) => tester.binding.defaultBinaryMessenger + .setMockMethodCallHandler(SystemChannels.platform, null); +} + +void main() { + // ── mobile glass menu (session_screen) ──────────────────────────────────── + group('C2c — mobile glass menu door', () { + Future pump(WidgetTester tester) async { + await tester.pumpWidget( + ProviderScope( + overrides: [ + connectionControllerProvider.overrideWith( + (ref) => ConnectionController(const _EmptyStorage()), + ), + projectsProvider.overrideWithValue(ProjectsState(const [])), + reposProvider.overrideWithValue(ReposState(const [])), + sessionsProvider.overrideWithValue( + SessionsState([_session('s1', 'Session')]), + ), + chatItemsProvider('s1').overrideWithValue(const []), + sessionMetaProvider('s1').overrideWithValue(null), + sessionActionErrorProvider('s1').overrideWithValue(null), + commandsProvider('s1').overrideWithValue(const []), + ], + child: const MaterialApp(home: SessionScreen(sessionId: 's1')), + ), + ); + await tester.pump(); + await tester.tap(find.byTooltip('Session actions')); + await tester.pumpAndSettle(); + } + + testWidgets('the menu offers Session details', (tester) async { + await pump(tester); + expect(find.text('Session details'), findsOneWidget); + }); + + testWidgets('selecting Session details opens the panel', (tester) async { + await pump(tester); + await tester.tap(find.text('Session details')); + await tester.pumpAndSettle(); + expect(find.byType(SessionIdentityDetails), findsOneWidget); + expect(find.text(kAgentId), findsOneWidget); + }); + }); + + // ── desktop pane-header kebab (pane_header) ─────────────────────────────── + group('C2c — desktop pane-header door', () { + Future pump(WidgetTester tester) async { + final container = ProviderContainer( + overrides: [ + connectionControllerProvider.overrideWith( + (ref) => ConnectionController(const _EmptyStorage()), + ), + sessionsProvider.overrideWithValue( + SessionsState([_session('s1', 'Session')]), + ), + reposProvider.overrideWithValue(ReposState(const [])), + chatItemsProvider('s1').overrideWithValue(const []), + sessionActionErrorProvider('s1').overrideWithValue(null), + commandsProvider('s1').overrideWithValue(const []), + ], + ); + addTearDown(container.dispose); + await tester.pumpWidget( + UncontrolledProviderScope( + container: container, + child: const MaterialApp( + home: Scaffold(body: DesktopChatPane(sessionId: 's1')), + ), + ), + ); + await tester.pump(); + await tester.tap(find.byTooltip('Session actions')); + await tester.pumpAndSettle(); + } + + testWidgets('the kebab offers Session details', (tester) async { + await pump(tester); + expect(find.text('Session details'), findsOneWidget); + }); + + testWidgets('selecting Session details opens the panel', (tester) async { + await pump(tester); + await tester.tap(find.text('Session details')); + await tester.pumpAndSettle(); + expect(find.byType(SessionIdentityDetails), findsOneWidget); + expect(find.text(kAgentId), findsOneWidget); + }); + }); + + // ── desktop tab menu (split_view) ───────────────────────────────────────── + group('C2c — desktop tab menu', () { + late _Clipboard clipboard; + setUp(() { + clipboard = _Clipboard(); + resetNodeIds(); + }); + + Future pumpTwoTabs( + WidgetTester tester, { + StatusCenter? status, + }) async { + final c = ProviderContainer( + overrides: [ + sessionsProvider.overrideWithValue( + SessionsState([_session('s1', 'First'), _session('s2', 'Second')]), + ), + reposProvider.overrideWithValue(ReposState(const [])), + eventsProvider.overrideWithValue(EventsState(const {}, const {})), + if (status != null) statusCenterProvider.overrideWithValue(status), + ], + ); + addTearDown(c.dispose); + final ws = c.read(workspaceControllerProvider.notifier); + ws.revealSession('s1'); + ws.revealSession('s2'); + await tester.pumpWidget( + UncontrolledProviderScope( + container: c, + child: const MaterialApp(home: Scaffold(body: WorkspaceView())), + ), + ); + await tester.pumpAndSettle(); + return c; + } + + Finder chip(String label) => find + .ancestor(of: find.text(label), matching: find.byType(Container)) + .first; + + testWidgets('offers Copy session id and NOT Session details (D13)', ( + tester, + ) async { + await pumpTwoTabs(tester); + await tester.tap(chip('First'), buttons: kSecondaryButton); + await tester.pumpAndSettle(); + expect(find.text('Copy session id'), findsOneWidget); + expect( + find.text('Session details'), + findsNothing, + reason: 'the tab menu is a different job; the third door was cut (D13)', + ); + }); + + testWidgets('Copy session id copies the BARE id and opens no panel', ( + tester, + ) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + await pumpTwoTabs(tester); + await tester.tap(chip('First'), buttons: kSecondaryButton); + await tester.pumpAndSettle(); + await tester.tap(find.text('Copy session id')); + await tester.pumpAndSettle(); + // The MUTATION bites here: sessionIdentityText would be a multi-line + // label:value payload, not the bare id. + expect(clipboard.writes, [kAgentId]); + expect(find.byType(SessionIdentityDetails), findsNothing); + }); + + testWidgets('Copy session id reports a clipboard failure', (tester) async { + // Same contract as the panel's `Copy all` and `/session id`: a write that + // throws is reported, never swallowed and never claimed as a success. + clipboard + ..install(tester) + ..fail = true; + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + addTearDown(status.dispose); + await pumpTwoTabs(tester, status: status); + await tester.tap(chip('First'), buttons: kSecondaryButton); + await tester.pumpAndSettle(); + await tester.tap(find.text('Copy session id')); + await tester.pumpAndSettle(); + expect(tester.takeException(), isNull); + expect(clipboard.writes, isEmpty); + expect( + status.events.map((e) => e.title), + isNot(contains('Session id copied')), + ); + final failed = status.events.where( + (e) => e.title == 'Could not copy session id', + ); + expect(failed, hasLength(1)); + expect(failed.single.severity, StatusSeverity.failure); + }); + + testWidgets('Copy session id survives the tab closing under the menu', ( + tester, + ) async { + // The 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 this for real) and the chip's `ref` is dead by + // the time the selection comes back — `ref.read` after the await then + // throws `Cannot use "ref" after the widget was disposed`. + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + addTearDown(status.dispose); + final c = await pumpTwoTabs(tester, status: status); + await tester.tap(chip('First'), buttons: kSecondaryButton); + await tester.pumpAndSettle(); + c.read(workspaceControllerProvider.notifier).unbindSession('s1'); + await tester.pump(); + expect(find.text('First'), findsNothing, reason: 'the chip is disposed'); + await tester.tap(find.text('Copy session id')); + await tester.pumpAndSettle(); + expect( + tester.takeException(), + isNull, + reason: 'a disposed ref must not blow up the gesture handler', + ); + }); + }); +} diff --git a/app/test/session_identity_codec_test.dart b/app/test/session_identity_codec_test.dart new file mode 100644 index 00000000..f42e97e0 --- /dev/null +++ b/app/test/session_identity_codec_test.dart @@ -0,0 +1,69 @@ +// SPEC-52 B1 — the wire contract for session identity. +// +// Two OPTIONAL fields on `SessionDTO`: `agentSessionId` and `transcriptPath`. +// Optional so a new app paired with an older server renders fewer rows rather +// than a fabricated one — the same rule `createdAt` follows (SPEC-47 D12). +// +// The empty-string cases are not paranoia. `''` is what a sloppy or partially +// migrated server sends for "I have no value", and an empty string here would +// render a copy affordance that copies nothing — which is exactly the +// placeholder D9 exists to forbid. Normalising at the edge means every consumer +// above this line only has to check for null. +import 'package:flutter_test/flutter_test.dart'; +import 'package:makit/store/models.dart'; +import 'package:makit/transport/codec.dart'; + +Session _decode(Map extra) { + final sessions = WireCodec.decodeSessions([ + { + 'id': 's1', + 'projectId': 'p1', + 'agent': 'pi', + 'title': 'T', + 'status': 'idle', + 'policy': 'ask-on-risky', + ...extra, + }, + ]); + return sessions!.single; +} + +void main() { + test('both identity fields survive the wire', () { + final s = _decode({ + 'agentSessionId': '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f', + 'transcriptPath': '/Users/le/.pi/agent/sessions/--x--/a.jsonl', + }); + expect(s.agentSessionId, '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'); + expect(s.transcriptPath, '/Users/le/.pi/agent/sessions/--x--/a.jsonl'); + }); + + test('an older server that sends neither yields two nulls', () { + final s = _decode({}); + expect(s.agentSessionId, isNull); + expect(s.transcriptPath, isNull); + }); + + test('an empty string is normalised to null, not kept as a blank row', () { + final s = _decode({'agentSessionId': '', 'transcriptPath': ''}); + expect(s.agentSessionId, isNull); + expect(s.transcriptPath, isNull); + }); + + test('a non-string is rejected rather than coerced', () { + // A malformed snapshot must not take down the session list — the same + // degrade-don't-crash rule the rest of `decodeSessions` follows. + final s = _decode({'agentSessionId': 42, 'transcriptPath': false}); + expect(s.agentSessionId, isNull); + expect(s.transcriptPath, isNull); + }); + + test('copyWith carries them', () { + final s = _decode({ + 'agentSessionId': 'a', + 'transcriptPath': '/p', + }).copyWith(title: 'renamed'); + expect(s.agentSessionId, 'a'); + expect(s.transcriptPath, '/p'); + }); +} diff --git a/app/test/session_identity_provider_test.dart b/app/test/session_identity_provider_test.dart new file mode 100644 index 00000000..cac3f822 --- /dev/null +++ b/app/test/session_identity_provider_test.dart @@ -0,0 +1,92 @@ +// SPEC-52 C2a — `sessionIdentityProvider(sessionId)`: the seam that maps the +// store's `Session` to the UI-level `SessionIdentity` the panel watches (D19). +// +// The load-bearing invariant here is that the provider NEVER returns null and +// NEVER throws: the panel always has something to show (the makit session id at +// minimum), so a null provider would force every call site to branch. The +// no-throw tests below are what the "return null instead of a null-fields +// identity" mutation must break. +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:makit/store/models.dart'; +import 'package:makit/store/store.dart'; +import 'package:makit/ui/session/session_identity.dart'; + +const kAgentId = '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'; +const kPath = + '/Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/' + '2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl'; + +Session _session({ + String id = 's1', + String agent = 'pi', + String? agentSessionId, + String? transcriptPath, +}) => Session( + id: id, + projectId: 'p1', + agent: agent, + title: 'Session', + status: SessionStatus.idle, + policy: ApprovalPolicy.askOnRisky, + agentSessionId: agentSessionId, + transcriptPath: transcriptPath, +); + +ProviderContainer _container(List sessions) { + final container = ProviderContainer( + overrides: [sessionsProvider.overrideWithValue(SessionsState(sessions))], + ); + addTearDown(container.dispose); + return container; +} + +void main() { + group('C2a — sessionIdentityProvider', () { + test('both wire fields present → a fully populated identity', () { + final container = _container([ + _session(agentSessionId: kAgentId, transcriptPath: kPath), + ]); + final identity = container.read(sessionIdentityProvider('s1')); + expect(identity.makitSessionId, 's1'); + expect(identity.agentLabel, 'pi session'); + expect(identity.agentSessionId, kAgentId); + expect(identity.transcriptPath, kPath); + expect(identity.resumeCommand, 'pi --session $kAgentId'); + }); + + test('the per-agent label follows the session\'s agent', () { + final container = _container([ + _session(agent: 'codex', agentSessionId: kAgentId), + ]); + final identity = container.read(sessionIdentityProvider('s1')); + expect(identity.agentLabel, 'Thread'); + expect(identity.resumeCommand, 'codex resume $kAgentId'); + }); + + test('neither field → a null-FIELDS identity, not null, and no throw', () { + final container = _container([_session()]); + final identity = container.read(sessionIdentityProvider('s1')); + expect(identity, isNotNull); + expect(identity.agentSessionId, isNull); + expect(identity.transcriptPath, isNull); + expect(identity.resumeCommand, isNull); + // The makit id is always present — that is the whole point of never + // returning null. + expect(identity.makitSessionId, 's1'); + }); + + test('an unknown session id → null-fields identity, no throw', () { + // Choice (stated in the provider): an unknown id echoes back as the makit + // id with an unknown agent, rather than throwing or returning null. That + // is truthful (this client has no record of it) and keeps the panel + // openable without a null check at every call site. + final container = _container([]); + final identity = container.read(sessionIdentityProvider('ghost')); + expect(identity.makitSessionId, 'ghost'); + expect(identity.agentSessionId, isNull); + expect(identity.transcriptPath, isNull); + expect(identity.agentLabel, 'Agent session'); + }); + }); +} diff --git a/app/test/session_identity_test.dart b/app/test/session_identity_test.dart new file mode 100644 index 00000000..f1582b41 --- /dev/null +++ b/app/test/session_identity_test.dart @@ -0,0 +1,171 @@ +// SPEC-52 A1 + A2 — the pure half of session identity: the per-agent vocabulary +// table and the clipboard payload. +// +// These are unit tests on pure functions, deliberately, for the same reason +// `context_usage_test.dart` tests `formatTokens` / `headroomLabel` directly: the +// copy contract is the feature, and a widget test would only observe it through +// three layers of rendering. +import 'package:flutter_test/flutter_test.dart'; +import 'package:makit/ui/session/session_identity.dart'; + +/// Two REAL pi session ids from one directory on the author's machine that share +/// their first 8 characters. They are the standing evidence for D15: pi ids are +/// UUIDv7, whose first 48 bits are a millisecond timestamp, so an 8-char prefix +/// pins only the top 32 bits and leaves ~65 s of ambiguity. +/// +/// Review drove the ambiguity to be sure: `pi --session 019fa9f4` does NOT error +/// — it silently resolves to one of these and offers to fork it. A resume +/// command that can silently target the wrong session is worse than no command, +/// so the full id is asserted below and the prefix form is asserted absent. +const kRealId = '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'; +const kCollidingId = '019fa9f4-d3c8-7e0d-9e34-8c70180ca113'; +const kSharedPrefix = '019fa9f4'; + +const kRealPath = + '/Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/' + '2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl'; + +SessionIdentity _identity({ + String agent = 'pi', + String makitSessionId = '7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c', + String? agentSessionId = kRealId, + String? transcriptPath = kRealPath, +}) => SessionIdentity.from( + agent: agent, + makitSessionId: makitSessionId, + agentSessionId: agentSessionId, + transcriptPath: transcriptPath, +); + +void main() { + group('A1 — per-agent vocabulary (D10, D15)', () { + test('pi gets pi\'s own noun and its --session flag', () { + final id = _identity(agent: 'pi'); + expect(id.agentLabel, 'pi session'); + expect(id.resumeCommand, 'pi --session $kRealId'); + }); + + test('codex gets codex\'s own noun (thread) and its resume verb', () { + final id = _identity(agent: 'codex'); + expect(id.agentLabel, 'Thread'); + expect(id.resumeCommand, 'codex resume $kRealId'); + }); + + test('an unknown agent gets a generic label and NO resume command', () { + // The open/closed escape hatch: a third agent works unedited, it just + // does not get a resume line, because inventing a CLI we have never seen + // is D9's failure mode in command form. + final id = _identity(agent: 'stub'); + expect(id.agentLabel, 'Agent session'); + expect(id.resumeCommand, isNull); + }); + + test('D15 — the resume command carries the FULL id, never a prefix', () { + for (final agent in ['pi', 'codex']) { + final cmd = _identity(agent: agent).resumeCommand!; + expect( + cmd, + contains(kRealId), + reason: '$agent must resume by the whole 36-char id', + ); + // The specific unsafe form, spelled out so the mutation is obvious. + expect( + cmd.endsWith(kSharedPrefix), + isFalse, + reason: + 'a truncated id can silently resolve to $kCollidingId, which ' + 'shares the prefix $kSharedPrefix in the same directory', + ); + } + }); + + test('no agent session id means no resume command, for every agent', () { + for (final agent in ['pi', 'codex', 'stub']) { + expect( + _identity(agent: agent, agentSessionId: null).resumeCommand, + isNull, + reason: '$agent cannot be resumed by an id that does not exist', + ); + } + }); + }); + + group('A2 — sessionIdentityText (D14, D4, D9)', () { + test('four measured values produce four label: value lines', () { + final text = sessionIdentityText(_identity()); + final lines = text.split('\n'); + expect(lines, hasLength(4)); + for (final line in lines) { + expect(line, contains(': '), reason: 'every line is label: value'); + } + expect(text, contains(kRealId)); + expect(text, contains(kRealPath)); + }); + + test('labels are padded into a shared column', () { + // Asserted rather than implied: rev 1 stated the padding but left it + // unprovable, so a stub that never padded would have passed. + final lines = sessionIdentityText(_identity()).split('\n'); + final valueStarts = lines.map((l) => l.indexOf(': ') + 2).toSet(); + expect( + valueStarts, + hasLength(1), + reason: 'all values begin at the same column: $lines', + ); + }); + + test('no trailing newline', () { + expect(sessionIdentityText(_identity()).endsWith('\n'), isFalse); + }); + + test('D4 — the path is absolute and unabbreviated, and no ~ appears', () { + final text = sessionIdentityText(_identity()); + expect(text, contains('/Users/le/.pi/agent/sessions/')); + expect( + text, + isNot(contains('~')), + reason: + 'the path belongs to the SERVER host; ~ only expands if a shell ' + 'gets there first, and the receiver here may be a prompt', + ); + }); + + test('D9 — an unmeasured transcript omits its line entirely', () { + final text = sessionIdentityText(_identity(transcriptPath: null)); + final lines = text.split('\n'); + expect(lines, hasLength(3)); + expect(text, isNot(contains('transcript'))); + expect( + lines.any((l) => l.trim().endsWith(':')), + isFalse, + reason: 'no label may be emitted with an empty value', + ); + expect(text, isNot(contains('\n\n')), reason: 'no blank line'); + }); + + test('D9 — an unknown agent omits the resume line', () { + final text = sessionIdentityText(_identity(agent: 'stub')); + expect(text, isNot(contains('resume'))); + expect(text.split('\n'), hasLength(3)); + }); + + test('the payload is plain text: no markdown, no fences, no JSON', () { + final text = sessionIdentityText(_identity()); + for (final noise in ['```', '**', '- ', '{', '}', '|']) { + expect( + text, + isNot(contains(noise)), + reason: 'must survive being pasted into a prompt or a shell comment', + ); + } + }); + + test('an identity with nothing but the makit id still copies one line', () { + final text = sessionIdentityText( + _identity(agent: 'stub', agentSessionId: null, transcriptPath: null), + ); + expect(text.split('\n'), hasLength(1)); + expect(text, contains('7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c')); + }); + }); +} diff --git a/app/test/session_identity_widget_test.dart b/app/test/session_identity_widget_test.dart new file mode 100644 index 00000000..a5c35d78 --- /dev/null +++ b/app/test/session_identity_widget_test.dart @@ -0,0 +1,535 @@ +// SPEC-52 A3 + A4 — the identity panel: its rows, its ONE copy affordance, its +// read-only-ness, its accessibility, and the two hosts it renders in. +// +// The single-copy-affordance test (D5) is the load-bearing one: four per-row +// copy buttons were designed, measured, and superseded, and this test is what +// stops them coming back. +import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; +import 'package:flutter_test/flutter_test.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:makit/app/theme.dart'; +import 'package:makit/status/status_center.dart'; +import 'package:makit/status/status_event.dart'; +import 'package:makit/status/status_providers.dart'; +import 'package:makit/ui/session/session_identity.dart'; + +const kId = '019fa9f4-443d-7d86-8f4c-d9c4988ddf4f'; +const kPath = + '/Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/' + '2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl'; +const kMakitId = '7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c'; + +SessionIdentity identity({ + String agent = 'pi', + String? agentSessionId = kId, + String? transcriptPath = kPath, +}) => SessionIdentity.from( + agent: agent, + makitSessionId: kMakitId, + agentSessionId: agentSessionId, + transcriptPath: transcriptPath, +); + +/// Captures what the panel actually puts on the clipboard, and can be made to +/// fail so the "never toast success on a failed write" rule is testable. +class _Clipboard { + final List writes = []; + bool fail = false; + + void install(WidgetTester tester) { + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'Clipboard.setData') { + if (fail) throw PlatformException(code: 'denied'); + writes.add((call.arguments as Map)['text'] as String); + } + return null; + }, + ); + } + + void remove(WidgetTester tester) => tester.binding.defaultBinaryMessenger + .setMockMethodCallHandler(SystemChannels.platform, null); +} + +Widget _host(Widget child, {StatusCenter? status}) => ProviderScope( + overrides: [ + if (status != null) statusCenterProvider.overrideWithValue(status), + ], + child: MaterialApp( + theme: makitDarkTheme, + home: Scaffold(body: child), + ), +); + +void main() { + late _Clipboard clipboard; + + setUp(() => clipboard = _Clipboard()); + + group('A3 — the body (D5, D8, D9, D18)', () { + testWidgets('renders one row per measured value', (tester) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + expect(find.text('pi session'), findsOneWidget); + expect(find.text('Transcript'), findsOneWidget); + expect(find.text('Resume with'), findsOneWidget); + expect(find.text('makit session'), findsOneWidget); + expect(find.text(kId), findsOneWidget); + }); + + testWidgets('rows appear in the locked order, top to bottom', ( + tester, + ) async { + // Asserted by painted position, not by mere presence: "in order" is only + // a real assertion if reversing the list can fail it. + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + double dy(String label) => tester.getTopLeft(find.text(label)).dy; + expect(dy('pi session'), lessThan(dy('Transcript'))); + expect(dy('Transcript'), lessThan(dy('Resume with'))); + expect(dy('Resume with'), lessThan(dy('makit session'))); + }); + + testWidgets('D9 — an absent transcript renders NO row, not a blank one', ( + tester, + ) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity(transcriptPath: null))), + ); + expect(find.text('Transcript'), findsNothing); + expect(find.text('pi session'), findsOneWidget); + }); + + testWidgets('D9 — an unknown agent renders no resume row', (tester) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity(agent: 'stub'))), + ); + expect(find.text('Resume with'), findsNothing); + expect(find.text('Agent session'), findsOneWidget); + }); + + testWidgets('a draft says so, and offers no id or resume row', ( + tester, + ) async { + await tester.pumpWidget( + _host( + SessionIdentityDetails( + identity: identity(agentSessionId: null, transcriptPath: null), + ), + ), + ); + expect(find.text(kSessionIdentityNoAgentLine), findsOneWidget); + expect(find.text('Resume with'), findsNothing); + expect(find.text('makit session'), findsOneWidget); + }); + + testWidgets('D5 — there is exactly ONE copy affordance', (tester) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + expect( + find.byIcon(kSessionIdentityCopyIcon), + findsOneWidget, + reason: + 'four per-row buttons were designed and superseded: they cost 116px ' + 'of value width (wrapping a uuid mid-string) and a toast that ' + 'cannot say which one was hit', + ); + }); + + testWidgets('Copy all copies exactly sessionIdentityText', (tester) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + final id = identity(); + await tester.pumpWidget(_host(SessionIdentityDetails(identity: id))); + await tester.tap(find.byIcon(kSessionIdentityCopyIcon)); + await tester.pumpAndSettle(); + expect(clipboard.writes, [sessionIdentityText(id)]); + }); + + testWidgets('a failed clipboard write does not claim success', ( + tester, + ) async { + // Asserted against the STATUS BUS, not against rendered text: the test + // host has no toast overlay, so a `findsNothing` on rendered text could + // never fail and the test would be vacuous. (It was, in the first draft — + // the mutation "toast unconditionally" did not bite until this changed.) + clipboard + ..install(tester) + ..fail = true; + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity()), status: status), + ); + await tester.tap(find.byIcon(kSessionIdentityCopyIcon)); + await tester.pumpAndSettle(); + expect(clipboard.writes, isEmpty); + expect( + status.events.map((e) => e.title), + isNot(contains('Session details copied')), + ); + final failed = status.events.where( + (e) => e.title == 'Could not copy session details', + ); + expect(failed, hasLength(1)); + expect( + failed.single.severity, + StatusSeverity.failure, + reason: 'all three copy paths report a failed write the same way', + ); + }); + + testWidgets('a successful copy posts exactly one confirmation', ( + tester, + ) async { + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + final status = StatusCenter(); + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity()), status: status), + ); + await tester.tap(find.byIcon(kSessionIdentityCopyIcon)); + await tester.pumpAndSettle(); + expect( + status.events.where((e) => e.title == 'Session details copied'), + hasLength(1), + ); + }); + + testWidgets('D8 — read-only: no field, no destructive affordance', ( + tester, + ) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + expect(find.byType(TextField), findsNothing); + expect(find.byType(EditableText), findsNothing); + final destructive = RegExp( + r'rename|delete|close session|kill|quit', + caseSensitive: false, + ); + for (final w in tester.widgetList(find.byType(Text))) { + final data = w.data ?? ''; + expect( + destructive.hasMatch(data), + isFalse, + reason: + 'lifecycle lives in the menu, not one mis-tap from a copy row', + ); + } + }); + + testWidgets('D18 — the copy row names its payload for a screen reader', ( + tester, + ) async { + final handle = tester.ensureSemantics(); + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + // Names WHAT it copies and HOW MUCH: a screen-reader user cannot see the + // toast, so the affordance must say it before the tap. + expect( + find.bySemanticsLabel(RegExp(r'Copy session details.*4 lines')), + findsOneWidget, + ); + handle.dispose(); + }); + + testWidgets('D18 — a value row exposes its label and value together', ( + tester, + ) async { + final handle = tester.ensureSemantics(); + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + expect(find.bySemanticsLabel(RegExp('pi session.*$kId')), findsOneWidget); + handle.dispose(); + }); + + testWidgets('a one-line payload says "1 line", not "1 lines"', ( + tester, + ) async { + // Reachable in production: a stub/detached session has no agent id and no + // transcript, so the makit id is the only measured value. The first build + // read "1 lines" and the pixel gate on the real macOS app caught it. + final lone = identity( + agent: 'stub', + agentSessionId: null, + transcriptPath: null, + ); + expect(sessionIdentityText(lone).split('\n'), hasLength(1)); + final handle = tester.ensureSemantics(); + await tester.pumpWidget(_host(SessionIdentityDetails(identity: lone))); + expect(find.text('1 line'), findsOneWidget); + expect(find.text('1 lines'), findsNothing); + expect( + find.bySemanticsLabel('Copy session details, 1 line'), + findsOneWidget, + ); + handle.dispose(); + }); + + testWidgets('a multi-line payload still pluralises', (tester) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + expect(find.text('4 lines'), findsOneWidget); + }); + + testWidgets('values are monospaced', (tester) async { + await tester.pumpWidget( + _host(SessionIdentityDetails(identity: identity())), + ); + final text = tester.widget(find.text(kId)); + expect(text.style?.fontFamily, kMonoFontFamily); + }); + }); + + group('A4 — the hosts (D11, D19)', () { + testWidgets('mobile opens a bottom sheet', (tester) async { + await tester.pumpWidget( + _host( + Builder( + builder: (context) => ElevatedButton( + onPressed: () => showSessionIdentity( + context: context, + identity: identity(), + desktop: false, + ), + child: const Text('open'), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + expect(find.byType(BottomSheet), findsOneWidget); + expect(find.text(kId), findsOneWidget); + }); + + testWidgets('desktop opens a centred, window-clamped panel — not a sheet', ( + tester, + ) async { + // The desktop host had NO test at all: every case here passed + // `desktop: false`, which is how the panel came to be documented as an + // "anchored MenuAnchor popover" (D11) while shipping a centred dialog. It + // is centred on purpose — both desktop doors are transient MENU ITEMS, so + // by the time one is chosen the menu is gone and there is nothing left on + // screen to anchor to (unlike SPEC-37's ContextUsageButton, a persistent + // control in the composer). This test pins the presentation that actually + // ships, so the doc and the code cannot drift apart again. + tester.view.physicalSize = const Size(1400, 900); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + await tester.pumpWidget( + _host( + Builder( + builder: (context) => ElevatedButton( + onPressed: () => showSessionIdentity( + context: context, + identity: identity(), + desktop: true, + ), + child: const Text('open'), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + expect(find.byType(BottomSheet), findsNothing); + expect(find.byType(SessionIdentityDetails), findsOneWidget); + // Centred: the panel's centre sits on the window's centre, both axes. + final panel = tester.getRect(find.byType(SessionIdentityDetails)); + expect(panel.center.dx, moreOrLessEquals(700, epsilon: 1)); + expect(panel.center.dy, moreOrLessEquals(450, epsilon: 1)); + // Window-clamped, never wider than the fixed panel width (SPEC-37). + expect(panel.width, lessThanOrEqualTo(kIdentityPanelWidth)); + }); + + testWidgets('a desktop panel in a narrow window is clamped to the window', ( + tester, + ) async { + // The SPEC-37 lesson for the desktop host: a fixed-width panel opened from + // a narrow split pane hung off-screen. 300pt is narrower than the panel's + // 340pt, so the clamp is what keeps both margins. + tester.view.physicalSize = const Size(300, 700); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + await tester.pumpWidget( + _host( + Builder( + builder: (context) => ElevatedButton( + onPressed: () => showSessionIdentity( + context: context, + identity: identity(), + desktop: true, + ), + child: const Text('open'), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + final panel = tester.getRect(find.byType(SessionIdentityDetails)); + expect(panel.left, greaterThanOrEqualTo(0)); + expect(panel.right, lessThanOrEqualTo(300)); + expect(panel.width, lessThanOrEqualTo(300 - 2 * 12)); + }); + + testWidgets('a window shrunk below the margins does not throw', ( + tester, + ) async { + // `window.width - 2 * margin` goes NEGATIVE below 24pt, and a BoxConstraints + // whose maxWidth is negative is not normalized: the layout ASSERTS + // ("BoxConstraints has both width and height constraints non-normalized") + // instead of rendering a small panel. + // + // Reproduced by shrinking the window WHILE the panel is open, which is both + // the realistic path (a resize animation or an embedded host hands us one + // degenerate frame) and the only one that works: opening at 20x20 leaves the + // trigger button unhittable, so the tap lands on nothing and the panel never + // opens — a test that passes while asserting nothing. + tester.view.physicalSize = const Size(1400, 900); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.reset); + await tester.pumpWidget( + _host( + Builder( + builder: (context) => ElevatedButton( + onPressed: () => showSessionIdentity( + context: context, + identity: identity(), + desktop: true, + ), + child: const Text('open'), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + expect(find.byType(SessionIdentityDetails), findsOneWidget); + tester.view.physicalSize = const Size(20, 20); + await tester.pump(); + expect(tester.takeException(), isNull); + }); + + testWidgets( + 'at 320x360 nothing paints off-screen and Copy all is still reachable', + (tester) async { + // The SPEC-37 lesson, asserted as the invariant that actually matters. + // Content TALLER than the window is fine — it scrolls; the transcript + // path legitimately wraps to four lines on a 320pt phone. What must never + // happen is the sheet painting outside the window, or the one affordance + // you opened the panel for being unreachable. + clipboard.install(tester); + addTearDown(() => clipboard.remove(tester)); + tester.view.physicalSize = const Size(320, 360); + tester.view.devicePixelRatio = 1.0; + addTearDown(tester.view.resetPhysicalSize); + addTearDown(tester.view.resetDevicePixelRatio); + final id = identity(); + await tester.pumpWidget( + _host( + Builder( + builder: (context) => ElevatedButton( + onPressed: () => showSessionIdentity( + context: context, + identity: id, + desktop: false, + ), + child: const Text('open'), + ), + ), + ), + ); + await tester.tap(find.text('open')); + await tester.pumpAndSettle(); + + final sheet = tester.getRect(find.byType(BottomSheet)); + expect(sheet.left, greaterThanOrEqualTo(0)); + expect(sheet.right, lessThanOrEqualTo(320)); + expect(sheet.bottom, lessThanOrEqualTo(360)); + + await tester.scrollUntilVisible( + find.byIcon(kSessionIdentityCopyIcon), + 80, + scrollable: find.descendant( + of: find.byType(BottomSheet), + matching: find.byType(Scrollable), + ), + ); + await tester.tap(find.byIcon(kSessionIdentityCopyIcon)); + await tester.pumpAndSettle(); + expect(clipboard.writes, [sessionIdentityText(id)]); + }, + ); + + testWidgets('passing both sessionId and identity is a programming error', ( + tester, + ) async { + // The API is exactly-one-of, asserted. The first wiring accepted both and + // silently ignored `identity`, so every door did a redundant `ref.read` + // whose result was discarded. + await tester.pumpWidget( + _host( + Builder( + builder: (context) => + ElevatedButton(onPressed: () {}, child: const Text('x')), + ), + ), + ); + final context = tester.element(find.text('x')); + expect( + () => showSessionIdentity( + context: context, + desktop: false, + sessionId: 's1', + identity: identity(), + ), + throwsAssertionError, + ); + expect( + () => showSessionIdentity(context: context, desktop: false), + throwsAssertionError, + ); + }); + + testWidgets( + 'D19 — the panel fills in live when the id arrives while it is open', + (tester) async { + // A draft's panel can be opened BEFORE the adapter assigns the id. That + // assignment fans out a fresh snapshot, so a watching panel must fill + // in; a snapshotting one would lie until reopened. + final notifier = ValueNotifier( + identity(agentSessionId: null, transcriptPath: null), + ); + addTearDown(notifier.dispose); + await tester.pumpWidget( + _host( + ValueListenableBuilder( + valueListenable: notifier, + builder: (_, value, _) => SessionIdentityDetails(identity: value), + ), + ), + ); + expect(find.text(kSessionIdentityNoAgentLine), findsOneWidget); + expect(find.text(kId), findsNothing); + + notifier.value = identity(); + await tester.pumpAndSettle(); + + expect(find.text(kId), findsOneWidget); + expect(find.text(kSessionIdentityNoAgentLine), findsNothing); + }, + ); + }); +} diff --git a/app/test/status/status_lifetime_test.dart b/app/test/status/status_lifetime_test.dart index 379585e9..1fd5fa9e 100644 --- a/app/test/status/status_lifetime_test.dart +++ b/app/test/status/status_lifetime_test.dart @@ -46,6 +46,39 @@ void main() { ); }); + test('a comment mentioning await does not create a false positive', () { + // Found while adding SPEC-52's identity panel, whose hoist comment + // legitimately names the rule it follows ("resolved before the first + // `await`"). The scanner sliced `body.text` out of the ORIGINAL source, so + // that comment supplied the "first await" and the correctly-hoisted + // `final status = ref.status;` on the NEXT line was reported as an offender. + // + // The guard is deliberately biased toward false positives, but not this kind: + // it made the sanctioned fix un-writable, which pushes authors toward + // wording their comments around the linter instead of hoisting. + const src = ''' +void f() async { + // ref.status is resolved before the first await, deliberately. + final status = ref.status; + await g(); + status.info('fine'); + // ref.status.info('this is commented out and must not count'); +} +'''; + final bodies = _asyncBodies(src).toList(); + expect(bodies, hasLength(1)); + final body = bodies.single; + final firstAwait = body.text.indexOf('await '); + expect(firstAwait, greaterThan(0), reason: 'the real await is still seen'); + expect( + _refStatus.allMatches(body.text).where((m) => m.start > firstAwait), + isEmpty, + reason: + 'the hoisted read is before the real await, and the commented-out ' + 'one is not code', + ); + }); + test('a brace in a string or a comment does not end a body early', () { // The scanner used to count raw braces, so either line below closed the // body and hid the `ref.status` after the await — a silent false negative @@ -78,6 +111,13 @@ final RegExp _asyncOpen = RegExp(r'async\s*\*?\s*\{'); /// /// A nested body is reported inside its parent too — deliberately conservative: /// a false positive costs one hoist, a false negative costs a crash. +/// +/// The returned `text` is the BLANKED copy, not the original: a comment that +/// names the rule ("resolved before the first `await`") used to supply the first +/// `await` and turn the correctly-hoisted line below it into an offender, and a +/// commented-out `ref.status` counted as a real one. Offsets are preserved by the +/// blanking, so `body.start + match.start` still maps onto `src` for line +/// numbers. Iterable<({int start, String text})> _asyncBodies(String src) { final scan = _blankStringsAndComments(src); final out = <({int start, String text})>[]; @@ -89,7 +129,10 @@ Iterable<({int start, String text})> _asyncBodies(String src) { if (scan[i] == '}') { depth--; if (depth == 0) { - out.add((start: open + 1, text: src.substring(open + 1, i))); + // `scan`, not `src`: detection must not see `await` or `ref.status` + // inside a comment or a string. Blanking preserves length, so the + // offsets still map onto the original source for line numbers. + out.add((start: open + 1, text: scan.substring(open + 1, i))); break; } } diff --git a/app/tool/session_identity_demo.dart b/app/tool/session_identity_demo.dart new file mode 100644 index 00000000..3f96f8f4 --- /dev/null +++ b/app/tool/session_identity_demo.dart @@ -0,0 +1,213 @@ +// Interactive QA harness for the SPEC-52 session-identity panel. Seeded data, +// no server, no agent binary. +// +// cd app && flutter run -d macos --debug -t tool/session_identity_demo.dart +// cd app && flutter run -d "iPhone 17" --debug -t tool/session_identity_demo.dart +// +// Every panel is the shipped `SessionIdentityDetails` on the real theme, so row +// pitch, wrap behaviour and mono metrics are the product's — not an +// approximation. The design reference is `mockups/session-identity.html`. +// +// Why a harness at all, when there are 16 widget tests: D5's justification is a +// MEASURED claim — dropping the per-row copy column returned ~116px to each +// value, which is what lets a 36-char uuid sit on one line instead of wrapping +// mid-string. A widget test cannot verify that on the real platform at the real +// text scale; `cua-driver` reading the accessibility tree can. +// +// No controls: every state renders in one pass (see the note in build). The stamp +// reports the live state so a STALE macOS bundle is detectable — the trap that +// once made a whole QA pass grade the wrong theme. +// +// Not part of the app or the test suite: kept out of `lib/` so neither can +// import it. +import 'package:flutter/material.dart'; +import 'package:flutter/semantics.dart'; +import 'package:flutter_riverpod/flutter_riverpod.dart'; +import 'package:makit/app/theme.dart'; +import 'package:makit/ui/session/session_identity.dart'; + +// ── seeded identities ──────────────────────────────────────────────────────── +// +// Real captures, so the wrap behaviour on this page is the wrap behaviour in +// production. `kPiId` and `kCollidingId` share their first 8 characters and both +// exist in one real sessions directory — the evidence behind D15. + +const kPiId = '019ff121-1cc1-7c60-bc40-65890c87e6ff'; +const kPiPath = + '/Users/le/.pi/agent/sessions/' + '--Users-le-.worktrees-makit-feat-get-session-id--/' + '2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl'; +const kCodexId = '019efe19-101b-7183-8345-47f61b78dd61'; +const kMakitId = '7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c'; + +class _Case { + const _Case(this.title, this.identity); + final String title; + final SessionIdentity identity; +} + +final List<_Case> _cases = [ + _Case( + 'pi · live, transcript resolved', + SessionIdentity.from( + agent: 'pi', + makitSessionId: kMakitId, + agentSessionId: kPiId, + transcriptPath: kPiPath, + ), + ), + _Case( + 'pi · transcript unresolved (D9 — row omitted)', + SessionIdentity.from( + agent: 'pi', + makitSessionId: kMakitId, + agentSessionId: kPiId, + ), + ), + _Case( + 'codex · thread, no path in P1 (D16)', + SessionIdentity.from( + agent: 'codex', + makitSessionId: kMakitId, + agentSessionId: kCodexId, + ), + ), + _Case( + 'unknown agent · no resume row (D10 default)', + SessionIdentity.from( + agent: 'stub', + makitSessionId: kMakitId, + agentSessionId: kPiId, + ), + ), + _Case( + 'draft · agent not started (D9)', + SessionIdentity.from(agent: 'pi', makitSessionId: kMakitId), + ), +]; + +void main() { + // Force the semantics tree on, permanently, for two reasons: + // + // 1. Flutter only publishes accessibility nodes when it detects an assistive + // client. `cua-driver` reads the AX tree without announcing itself as one, + // so without this the window exposes nothing but the menu bar and the + // pixel gate has nothing to measure. (Learned the hard way here.) + // 2. D18 is a locked decision, so the semantics labels are part of what this + // harness exists to verify — not an afterthought. + WidgetsFlutterBinding.ensureInitialized(); + SemanticsBinding.instance.ensureSemantics(); + runApp(const ProviderScope(child: _Demo())); +} + +class _Demo extends StatelessWidget { + const _Demo(); + + /// The tightest width is where every risk lives (a 36-char uuid against a + /// 320 pt phone), so it is the width rendered for BOTH themes. The wider + /// columns only ever have more room. + static const double _phone = 320; + + @override + Widget build(BuildContext context) { + // Deliberately input-free: both themes and all five cases render in ONE + // pass. Driving a Flutter macOS window by synthesized CGEvent did not + // register (verified: the staleness stamp never changed), and a QA gate that + // depends on a click landing is a QA gate that silently measures the wrong + // state. Rendering every combination removes the dependency entirely. + return MaterialApp( + debugShowCheckedModeBanner: false, + theme: makitDarkTheme, + home: Scaffold( + backgroundColor: const Color(0xFF0E0E0E), + body: SingleChildScrollView( + child: Column( + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + _stamp(), + _band(makitDarkTheme, 'DARK', _phone), + _band(makitLightTheme, 'LIGHT', _phone), + _band(makitDarkTheme, 'DARK · 375pt', 375), + ], + ), + ), + ), + ); + } + + /// Proof the bundle is not stale: it names every state on the page, so a + /// screenshot that disagrees with the source is obvious at a glance. + Widget _stamp() => Builder( + builder: (context) => Container( + padding: const EdgeInsets.symmetric( + horizontal: kSpace12, + vertical: kSpace8, + ), + color: makitDarkTheme.colorScheme.surfaceContainerLow, + child: Text( + 'session identity · dark+light · ${_phone.round()}pt & 375pt · ' + '${_cases.length} cases · no input required', + style: makitDarkTheme.textTheme.labelMedium, + ), + ), + ); + + Widget _band(ThemeData theme, String title, double width) => Container( + color: theme.colorScheme.surfaceContainerLowest, + padding: const EdgeInsets.fromLTRB(kSpace16, kSpace12, kSpace16, kSpace20), + child: Column( + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + Padding( + padding: const EdgeInsets.only(bottom: kSpace10), + child: Text( + title, + style: theme.textTheme.labelMedium?.copyWith( + color: theme.colorScheme.onSurfaceVariant, + letterSpacing: 1.2, + ), + ), + ), + Theme( + data: theme, + child: Wrap( + spacing: kSpace16, + runSpacing: kSpace16, + crossAxisAlignment: WrapCrossAlignment.start, + children: [for (final c in _cases) _panel(theme, c, width)], + ), + ), + ], + ), + ); + + Widget _panel(ThemeData theme, _Case c, double width) => SizedBox( + width: width, + child: Column( + crossAxisAlignment: CrossAxisAlignment.stretch, + children: [ + Padding( + padding: const EdgeInsets.only(bottom: kSpace6), + child: Text( + c.title.toUpperCase(), + style: theme.textTheme.labelSmall?.copyWith( + color: theme.colorScheme.onSurfaceVariant, + letterSpacing: 0.7, + ), + ), + ), + DecoratedBox( + decoration: BoxDecoration( + color: theme.colorScheme.surface, + border: Border.all(color: theme.colorScheme.outlineVariant), + borderRadius: BorderRadius.circular(kRadius12), + ), + child: ClipRRect( + borderRadius: BorderRadius.circular(kRadius12), + child: SessionIdentityDetails(identity: c.identity), + ), + ), + ], + ), + ); +} diff --git a/docs/specs/2026-08-11-SPEC-52-PLAN.md b/docs/specs/2026-08-11-SPEC-52-PLAN.md new file mode 100644 index 00000000..732d9200 --- /dev/null +++ b/docs/specs/2026-08-11-SPEC-52-PLAN.md @@ -0,0 +1,246 @@ +# SPEC-52 — Plan (rev 2, dual review applied) + +> Renumbered 51 → 52: *Preview groups* took 51 and *Profiles* took 50 on `main` first. See the +> spec header. + +Every task states its **RED TEST** (written first, must fail for the stated reason), its **VERIFY** +command, and its **MUTATION** (the production edit that must make that test fail — a test with no +stated mutation is unproven, and rev 1 had five of them). + +Order is forced by three rules: shared vocabulary and pure functions land before their consumers; +the whole UI is built and pixel-signed-off **before** the wire contract is frozen; and the contract +commit is made by the controller alone so the two Phase-C agents cannot both edit it. + +**Baselines to preserve** (measured in review, not assumed): `tsc -p . --noEmit` clean · +`pnpm test` **1313 pass / 0 fail** · `flutter analyze --fatal-infos --no-pub` "No issues found". + +Flutter on this machine is `/Users/le/Work/Vibe/flutter/bin/flutter` (**not** `~/flutter` — that is +the Linux CI VM path in AGENTS.md). + +--- + +## Phase A — UI first, no wire (app only) + +The UI depends on an app-level value type, **not** on `SessionDTO`. That is what lets A1–A7 finish +before the contract exists, and it is the right dependency direction: the widget must not know the +wire's field names. Review confirmed no Phase-A task secretly needs the contract and nothing in A is +invalidated by B. + +### A0 — correct the mockup first ✅ done + +Rev 1 left this ownerless while A7 pixel-compares against it. Applied: full 36-char ids in every +resume row (D15), absolute paths everywhere (D4), codex's transcript row marked P2 (D16), and an +`✎ Amended in planning` note recording all three. Re-verified in a browser: grids aligned, no broken +glyph refs, no horizontal overflow, both uuid rows one line. + +### A1 — `SessionIdentity` value type + per-agent vocabulary table (D10, D15, D20) + +`app/lib/ui/session/session_identity.dart` (new). Immutable. A **lookup table** keyed by agent id — +not a `switch` (D10) — supplies the label and resume verb; the default branch is the OCP escape +hatch. + +- **RED TEST** `app/test/session_identity_test.dart` + - pi → `agentLabel == 'pi session'`, `resumeCommand == 'pi --session '` + - codex → `agentLabel == 'Thread'`, `resumeCommand == 'codex resume '` + - `'stub'` → `agentLabel == 'Agent session'`, `resumeCommand == null` + - **D15 guard:** asserts the resume string contains the entire 36-char id + (`019fa9f4-443d-7d86-8f4c-d9c4988ddf4f`) **and** that it is not the 8-char-prefix form. This + test is the standing record of a real collision — `pi --session 019fa9f4` silently resolves to + one of two real sessions and offers to fork it. + - `agentSessionId == null` → `resumeCommand == null` for every agent. +- **VERIFY** `flutter test --no-pub test/session_identity_test.dart` +- **MUTATION** truncate the id to 8 chars → the D15 assertion fails. + +### A2 — `sessionIdentityText()` (D14, D4, D9) + +Pure: `SessionIdentity` → clipboard payload. + +- **RED TEST** same file + - four measured values → four `label: value` lines; **two labels share a column** (padding is + asserted, not implied — rev 1 left this unprovable); no trailing newline. + - absolute path verbatim (D4): a `/Users/…` path appears unabbreviated and no `~` appears anywhere + in the output. + - `transcriptPath == null` → three lines, no blank line, no `transcript:` label (D9). + - unknown agent → no `resume:` line. + - no markdown, no fences. +- **VERIFY** same +- **MUTATION** (a) drop the padding → the shared-column assertion fails; (b) append `'\n'` → the + trailing-newline assertion fails; (c) emit `'-'` for a null value → the omission test fails. + +### A3 — `SessionIdentityDetails` body (D5, D8, D9, D18) + +Host-agnostic `Column` shaped like `ContextUsageDetails`: uppercase section cap, stacked label / +mono value rows, one `Copy all` action row, pi-id footnote. + +- **RED TEST** `app/test/session_identity_widget_test.dart` + - one row per measured value, **in the locked order** — asserted by comparing the rows' painted + `dy` positions, not merely by presence (rev 1's "in order" was unprovable). + - null `transcriptPath` → no transcript row (D9). + - **exactly one** copy affordance in the subtree (`findsOneWidget`) — D5, and this is the test + that stops per-row buttons coming back. + - tapping `Copy all` puts exactly `sessionIdentityText(identity)` on the clipboard (intercept + `SystemChannels.platform`) and emits one `status.info`. + - **clipboard failure:** when the platform channel throws, **no** "copied" toast is emitted + (`ENGINEERING.md` — never report success on a waited path). + - **read-only (D8):** no `TextField`, and nothing whose tooltip/label matches + `/rename|delete|close|kill|resume session/i`. + - **a11y (D18):** the `Copy all` row's semantics label names the payload and its line count; each + value row exposes label + value as one semantics node. + - value rows use `kMonoFontFamily`. +- **VERIFY** `flutter test --no-pub test/session_identity_widget_test.dart` +- **MUTATION** (a) add a second copy button → single-affordance fails; (b) reverse the row order → + the `dy` ordering fails; (c) drop the semantics label → the D18 test fails; (d) toast + unconditionally → the clipboard-failure test fails. + +### A4 — `showSessionIdentity()` host split (D11, D19) + +Mobile → `showModalBottomSheet(showDragHandle: true)` + `SafeArea` + `SingleChildScrollView`; +desktop → `MenuAnchor` popover, width `min(kIdentityPanelWidth, window.width - 2*margin)`, height +capped to the window. Copied from `context_usage.dart:228-290`, including the `primary: false` +reason. The body **watches** the identity provider (D19). + +- **RED TEST** same file + - mobile renders inside a `BottomSheet`; desktop inside a `MenuAnchor` subtree. + - at a 320×360 window the painted panel is **≤ window − 2·margin** in both axes (the SPEC-37 + off-screen bug, re-asserted). + - **D19:** with the panel open and the identity initially id-less, pushing an identity **with** an + id rebuilds the panel and the id row appears — without reopening. + - **draft/pending:** an identity with no agent id renders the "not started yet" line and **no** + resume row (rev 1 only eyeballed this in the harness). +- **VERIFY** same +- **MUTATION** (a) hard-code the width to 340 → narrow-window fails; (b) swap `ref.watch` for + `ref.read` → the D19 test fails. + +### A5 — QA harness `app/tool/session_identity_demo.dart` + +Modelled on `tool/tool_row_demo.dart`. Seeded identities (pi with path, pi without, codex, unknown +agent, draft), light/dark toggle, width toggle (320 / 375 / full), and a **top-bar stamp** printing +`session identity · · pt` so a stale macOS bundle is detectable. Not in `lib/`, +not in the suite. Kept despite the YAGNI pass because D5's justification is a *measured* claim +(dropping the copy column took both uuid rows from 2 lines to 1) and a widget test cannot verify row +pitch on the real platform. + +- **VERIFY** `flutter analyze --fatal-infos --no-pub` clean; harness runs on macOS. + +### A6 — Pixel sign-off on the real macOS app (hard gate) + +Per `makit-transcript-row-qa-harness`: +1. `rm -rf .dart_tool/flutter_build build/macos/Build/Products/Debug`, rebuild, and confirm the + top-bar stamp matches the intended state — proof the bundle is not stale. +2. `ps -eo pid,command | grep 'feat-get-session-id/app/build/macos.*MacOS/Makit'` → + `cua-driver call list_windows '{}'` → this pid's window with height > 400. +3. `cua-driver call get_window_state '{"pid":P,"window_id":W}'` — geometry from the AX tree, not + from pixels (AX space = Flutter logical px here; confirm by checking a row's `frame.w` against + the selected pane width). +4. **Measured assertions:** both uuid rows are **one line**; row pitch uniform; `Copy all` row + ≥ 38 px; nothing clipped at 320 pt; light and dark both captured (toggle by element token, + fetched immediately before each click since tokens are re-issued per snapshot). +5. Compare each against `mockups/session-identity.html` at the same widths. +- **GATE** any mismatch is fixed and re-measured before Phase B starts. + +--- + +## Phase B — freeze the contract (controller only, one commit) + +### B1 — `SessionDTO` fields + app model + guard tests (D1) + +- `server/src/protocol.ts`: `agentSessionId?: string`, `transcriptPath?: string`, documented as + optional-for-old-servers with the `createdAt` precedent named. +- `app/lib/store/models.dart`: two **nullable** fields + `fromJson`, normalising `''` → `null` so + D9 holds even against a sloppy server. +- **RED TEST (app)** `Session.fromJson` with the fields → populated; without → both null; with `''` + → both null. +- **MUTATION** remove the `''` normalisation → the empty-string test fails. +- **Dropped from rev 1:** the server-side "a DTO built without them is `undefined`" test. Review + correctly called it vacuous — optional TS fields are absent by default with zero production code, + and `JSON.stringify` drops `undefined`. The real bite lives in C1b. +- **VERIFY** `cd server && node_modules/.bin/tsc -p . --noEmit && pnpm test` · + `cd app && flutter test --no-pub test/models_test.dart` +- Commit alone; the message names the frozen field spelling. + +--- + +## Phase C — implement, in parallel, on disjoint trees + +### C1 — server (allow `server/src/**`, `server/test/**`; deny `app/**`, `docs/**`, `mockups/**`) + +- **C1a** new `server/src/transcript-path.ts` — the **agent-agnostic** dispatcher (steps (a) and (c) + of D3 are not pi-specific; only (b) is), delegating to `pi-sessions.ts` for the pi case so P2's + codex resolver is additive rather than a move. + - **RED TEST** `transcript-path.test.ts` with a temp `MAKIT_PI_AGENT_DIR`: + exact-suffix match wins; **a same-8-char-prefix different-uuid file is not matched** (D15 + asserted server-side too); missing dir / unreadable dir / non-pi agent → `undefined`; + `resumeSessionPath` takes precedence over a derivable path; never throws on a malformed file + (the module boundary rule). + - **MUTATION** relax the match to `includes(id.slice(0,8))` → the collision test fails. +- **C1b** wire it into the DTO projection **in `SessionManager`** (D3), `cwd = session.worktreePath + ?? project.dto.path`, memoized per session id including misses. + - **RED TEST** manager tests: **(i) a worktree-bound session resolves its path from the *worktree* + slug, not the project slug** — the exact bug review found, and untestable from a temp-dir suffix + test alone; (ii) `transcriptPath` appears in the projected DTO (rev 1 asserted only + `agentSessionId` here — the whole path-into-DTO wiring was unproven); (iii) a draft projects + neither field; (iv) a closed/cold session still projects the id (SPEC-29 persistence); (v) two + projections of the same session do **one** directory read (memoization). + - **MUTATION** (a) use `project.dto.path` unconditionally → the worktree test fails; (b) hard-code + `transcriptPath: undefined` → assertion (ii) fails; (c) drop the memo → assertion (v) fails. +- **VERIFY** `node_modules/.bin/tsc -p . --noEmit && pnpm test` (≥ 1313 pass / 0 fail) +- **TRAPS** `pi-sessions.ts` must never throw on a bad file. `git.ts`'s `run()` never rejects — + check `code`. Do not add a fixture to `server/test/fixtures/events.json` (one entry per *session* + kind). + +### C2 — app wiring (allow `app/lib/**`, `app/test/**`; deny `server/**`, `app/tool/**`, `docs/**`) + +- **C2a** `sessionIdentityProvider(sessionId)` — maps the store's session to `SessionIdentity`. + - **RED TEST** (rev 1 had **none** for this task): store with both fields → populated identity; + store with neither → an identity whose id/path are null and which does not throw; unknown + session id → null-fields, no throw. + - **MUTATION** return `null` instead of a null-fields identity → the no-throw tests fail. +- **C2b** `/session` client command (D7), registered next to `/name`. + - **RED TEST** `handleClientCommand('/session')` returns **true** — i.e. is intercepted and never + sent, which is the entire bug report encoded as one assertion; `'/session id'` copies exactly + the bare id; **bare `/session` does *not* copy** (rev 1 left the two branches collapsible); + `'/sessions'` returns false. + - **MUTATION** make the matcher prefix-based → the `/sessions` test fails. +- **C2c** two panel doors + the tab menu's **Copy session id** (D13, D6). + - **RED TEST** each menu contains its item; selecting *Session details* opens the panel; selecting + *Copy session id* copies the bare id and does **not** open a panel. + - **MUTATION** point *Copy session id* at `sessionIdentityText` → the bare-id assertion fails. +- **Dropped from rev 1:** the `SessionIdentitySection` in `ContextUsageDetails` (D12, cut to P2) and + its test. Review showed that test could never fail: appending a section to + `ContextUsageDetails` (the body) cannot affect `ContextUsageButton`'s `SizedBox.shrink` (the + button), so it asserted an unrelated invariant. +- **VERIFY** `flutter analyze --fatal-infos --no-pub && flutter test --no-pub` +- **TRAPS** one `flutter test` at a time; judge the suite by non-`loading` failures; a Dart `Map` + literal is never `==` (assert scalars); Riverpod asserts on changing the *number* of overrides + between `pumpWidget` calls — build the scope once and swap its child. + +--- + +## Phase D — two-pass review, then loop + +1. **Pass 1**, three reviews in parallel on the diff: code correctness (told to *run* the commands + and to name any vacuous test), spec adherence (each locked decision D1–D21, plus a proposed + mutation per test), and line-level review. +2. Triage every finding against the code myself; confirm before dispatching. One fix agent per tree, + each requiring a red test first and a stated mutation. +3. **Pass 2**, scoped to "check the fixes, not the feature": FIXED / PARTIAL / NOT FIXED / REGRESSED + per finding. Remainder fixed by me if small. +4. Live proof the tests cannot give: a throwaway probe against the real running server and the real + `pi` binary — a real session's `agentSessionId` resolved to a real path, and `pi --session ` accepted. Deleted; findings recorded in the spec's Verification section. +5. Fill the Deviations log; flip Status to Implemented with a results table. + +--- + +## Deviations log + +| # | Departure from the spec/mockup | Why | Status | +| --- | --- | --- | --- | +| 1 | Mockup showed `pi --session 019ff121` (8-char prefix); implementation uses the **full** id. | UUIDv7's first 48 bits are a ms timestamp, so 8 chars leave ~65 s of ambiguity. Real collisions on this machine (`019fa9f4-443d…` + `019fa9f4-d3c8…` in one dir). Review went further: `pi --session 019fa9f4` does **not** error — it silently picks one and offers to fork it. | Spec D15; mockup corrected in A0. | +| 2 | Dropped the `~/` display abbreviation. | The path belongs to the *server host*; the app cannot know that host's home dir. Faking it needs a `/Users//` heuristic (wrong on Linux hosts) or `homeDir` on the wire for cosmetics. | Spec D4; mockup corrected in A0. | +| 3 | Transcript-path resolution moved from `Session.toDTO` to `SessionManager`, keyed on `worktreePath ?? project.dto.path`, memoized. | `Session` has no project path (`session.ts:146`), and pi's slug follows the **worktree** cwd — verified on disk. Rev 1's placement could not compile *and* would have missed every worktree-bound session. Memoization added because per-snapshot `readdir` was unaffordable. | Spec D3, rev 2. | +| 4 | `resolveTranscriptPath` lives in a new `transcript-path.ts`, not in `pi-sessions.ts`. | Two of its three branches are agent-agnostic; P2's codex resolver becomes additive instead of forcing a move out of a pi-named module. | Plan C1a, rev 2. | +| 5 | D12 (identity section inside the usage panel) **cut** from P1 to P2. | The ring is absent in four states including the likeliest moment of need, so that door is missing exactly when wanted; it also added an import edge and mixed two row styles in one panel. | Spec D12, rev 2. | +| 6 | Three panel doors → **two**; the desktop tab menu keeps only *Copy session id*. | A tab-menu *Session details…* duplicates the pane-header kebab one pixel away on the same platform. | Spec D13, rev 2. | +| 7 | Added D18 (a11y), D19 (watch not snapshot), D20 (i18n), D21 (path disclosure). | Review found a11y absent where SPEC-47 had locked it; a panel opened before the id is assigned would have shown stale rows. | Spec rev 2. | +| 8 | D11's desktop host is a **centred, window-clamped panel**, not the specified `MenuAnchor` popover. | The mechanism cannot transfer: `ContextUsageButton` is a persistent composer control, so a `MenuAnchor` stays anchored to it, whereas both of this panel's desktop doors are transient menu items — by the time one is chosen the menu is dismissed and there is nothing left to anchor to. Found by review of the shipped code, not by a test, because the desktop host had **no test at all** (every case passed `desktop: false`) — which is how the code and its own doc comment drifted apart. | Spec D11 amended; two desktop tests added, both mutation-proven (alignment → topLeft, and dropping the clamp). | diff --git a/docs/specs/2026-08-11-SPEC-52-session-identity.md b/docs/specs/2026-08-11-SPEC-52-session-identity.md new file mode 100644 index 00000000..b30e20ee --- /dev/null +++ b/docs/specs/2026-08-11-SPEC-52-session-identity.md @@ -0,0 +1,218 @@ +# SPEC-52 — Session identity: copy the id, and its transcript + +**Status:** Implemented — P1 (rev 2 — dual review applied) · **Priority:** P2 · **Branch:** +`feat/get-session-id` +Deferred to P2: the codex/`threadId` resolver branch, and D12's identity section inside the +context-usage panel (cut on review — see the plan's deviations 4 and 5). + +> **Renumbered 51 → 52.** This spec was drafted as SPEC-51, and while it was in flight two other specs +> took the numbers around it on `main`: *Preview groups* took 51 +> (`2026-08-12-SPEC-51-preview-groups.md`, #163, referenced from `docs/UX.md` and fourteen shipped +> source files) and *Profiles* took 50 (`2026-08-10-SPEC-50-profiles.md`, #162). Both shipped first and +> neither can move, so this one took 52, the next free number. The branch name +> (`feat/get-session-id`) is deliberately left alone — renaming a pushed branch would orphan its PR for +> no gain. +> +> The rename does not fix the underlying hazard: nothing in the repo allocates spec numbers, so two +> branches drafted in the same week collide silently. `main` already carries the proof — SPEC-48 names +> *both* `2026-08-09-SPEC-48-status-and-activity.md` and `2026-08-10-SPEC-48-per-repo-settings.md`. + +**Depends on:** SPEC-29 (`agentSessionId` / `resumeSessionPath` persistence, closed-session +resume), SPEC-37 (`ContextUsageDetails` — the panel this appends to, and the ring's absence +rule this must not weaken), SPEC-47 D12 (the precedent for adding one optional field to +`SessionDTO` rather than a new command), SPEC-35 (the mid-turn queue — the thing that makes +the current workaround useless). +**Design board:** [`mockups/session-identity.html`](../../mockups/session-identity.html) — +one panel, three doors, one **Copy all**. Rejected variants recorded there in §"Rejected +alternatives"; the four per-row copy buttons were designed, then superseded, and that row is +kept so review does not re-propose them. + +--- + +## Goal + +Get the underlying agent session id — and the path to its transcript — onto the clipboard +**while the agent is mid-turn**, because that is exactly when you need it: you are handing this +session's work to a second session and you cannot wait for the turn to finish. + +| Question | Answered by | +| --- | --- | +| "what is this session's id, right now, mid-turn?" | `/session id` (client command) or the tab menu's **Copy session id** — bare id, zero dialogs | +| "give me everything I need to hand this off" | **Session details** → one panel → **Copy all** (D5) | +| "where is the transcript on disk?" | the `transcriptPath` row, server-resolved (D2/D3) | +| "how do I resume it in a terminal?" | the `Resume with` row (D10/D15) | + +## The problem, precisely + +`/session` is pi's own slash command. Typed into makit's composer it is **not** intercepted — +`clientCommands` (`app/lib/ui/composer/client_commands.dart:69`) holds only `new, cancel, +unpair, help, ask, compact, thinking, model, name` — so `handleClientCommand` returns false and +the send path falls through to `store.sendMessage` (`desktop_chat_pane.dart:183`). Mid-turn the +server cannot steer it into the running turn, so it lands in `Session.queued` +(`server/src/session.ts`, cap `MAX_QUEUED_MESSAGES = 50`) and executes on the next idle +transition — after the handoff you wanted has already gone stale. + +There is no other route. The app has never been told the id at all. + +## What already exists (and is thrown away) + +This is not new telemetry. Both values are already computed, already persisted, and already +survive a restart: + +- **`Session.agentSessionId`** (`server/src/session.ts:163`) — the native ACP `sessionId` (set + at `acp.ts:285`) or codex `threadId` (`codex.ts:200`). Persisted via `toMeta()` + (`session.ts:305`, the field at `:317`) → `storage/sqlite_event_store.ts:142`, restored on rehydration, and used as the + resume handle by SPEC-29. +- **`Session.resumeSessionPath`** (`session.ts:155`) — the on-disk transcript for sessions + attached from disk (`manager.ts:1176`, from `listPiSessions`). +- **`piSessionsDir(cwd)`** (`server/src/pi-sessions.ts:52`) — pi's slug algorithm, already + implemented and unit-tested (`pi-sessions.test.ts`), already used to list and parse prior + transcripts. + +They are simply **absent from `SessionDTO`** (`protocol.ts:689`). The whole feature is one +projection plus a panel. + +**And the id is the right one.** `pi-acp` reuses pi's *own* session uuid as the ACP +`sessionId` — verified against `~/.pi/pi-acp/session-map.json`, whose entries map +`sessionId → {cwd, sessionFile}` with the same uuid inside the filename. So `agentSessionId` +for a pi session is exactly the value pi's `/session` prints and `pi --session` accepts. + +## Why the context-usage ring cannot be the home for this + +`ContextUsageButton` opens with +`if (usage == null || fraction == null) return const SizedBox.shrink();` +(`app/lib/ui/composer/context_usage.dart:218`). The ring is therefore **absent** in four +states, one of which is the single likeliest moment of need: + +1. before turn 1 — *you are seeding a second session from a fresh one*; +2. on pi without the `makit-pi-usage` extension installed; +3. after a pi compaction, until the next reply (pi nulls the count); +4. whenever an agent reports cost or tokens but no window. + +A control that vanishes in four states cannot be the only route to a value needed in all of +them. **The absence rule is not weakened** (D12) — the panel is reachable without it, and is +*additionally* appended inside it when it happens to be there. + +## Decisions (locked) + +| # | Decision | Why / consequence | +| --- | --- | --- | +| **D1** | Two **optional** fields on `SessionDTO`: `agentSessionId?: string`, `transcriptPath?: string`. No new `cmd` kind. | Rides the existing session snapshot, so the values arrive *with* the session and cost zero round-trips mid-turn. Optional on the wire so a new app against an old server renders fewer rows, never a fabricated one — the `createdAt` precedent (SPEC-47 D12). | +| **D2** | `transcriptPath` is resolved **server-side only**. The app never derives it. | The slug algorithm is pi's, lives in `pi-sessions.ts`, and is already tested there. Duplicating it in Dart would be a second source of truth that drifts on the next pi change — and the app cannot stat the server's filesystem to check itself. | +| **D3** | Resolution happens **in `SessionManager`**, not in `Session.toDTO`, with `cwd = session.worktreePath ?? project.dto.path`. Order: (a) `resumeSessionPath` verbatim; (b) else, for pi only, the entry in `piSessionsDir(cwd)` whose basename ends `_.jsonl`; (c) else **absent**. Memoized per session id — **including misses** — so it costs at most one `readdir` per session per server lifetime and **zero I/O per snapshot**. | Three corrections, all found in review. (i) **`Session` does not know its project's filesystem path** — it holds `projectId` and `worktreePath` only (`session.ts:146,514`), so `toDTO` *cannot* resolve this; the manager can, via `this.projects.get(...)`. (ii) **pi's slug is derived from the cwd pi actually ran in, and that is usually the worktree** — `manager.ts:923` spawns with `worktreePath`, `:1358` with `project.dto.path`, `:1505` with a computed cwd. Verified on disk: this session's transcript is under `--Users-le-.worktrees-makit-feat-get-session-id--`, *not* a project-root slug. Copying the nearest precedent (`attachPiSession`, `manager.ts:1176`, which uses `project.dto.path`) would have silently missed the transcript for **every worktree-bound session** — i.e. every session this feature exists for — and D9 would have hidden the failure. (iii) The reattach path already documents this exact rule (`manager.ts:1478-1485`) but confirms membership with an **async** `listWorktrees`; that is unaffordable per snapshot, and unnecessary here — a wrong directory simply yields no match, and D9 hides the row. | +| **D4** | Paths are **absolute everywhere** — wire, clipboard *and* display. No `~/` abbreviation, no middle-elision. The row wraps. | The receiver of a copied path is another agent's shell or prompt, and `~` only expands if a shell gets there first. The mockup abbreviated to `~/.pi/agent/…`; **that was caught in planning as unimplementable**: the path belongs to the *server host*, and the app cannot know that host's home directory. Faking it needs either a `/Users//` heuristic (wrong on a Linux host, wrong for a non-standard home) or shipping `homeDir` on the wire for pure cosmetics. Absolute-and-wrapped is truthful, and it is what gets copied regardless. | +| **D5** | **One** copy affordance in the panel: a `Copy all` action row. No per-row copy buttons. | Four 24 pt targets stacked 30 pt apart on a phone, a toast that cannot say which one was hit, and 116 px stolen from every value — enough to wrap a 36-char uuid mid-string, which invites a partial selection. Measured in the mockup: dropping the copy column took both uuid rows from 2 lines to 1. | +| **D6** | Single-value copy is not lost, it **moves**: `/session id` and the tab menu's **Copy session id** each copy the bare `agentSessionId` and nothing else. | The panel answers "give me the context"; those two answer "give me the id". One surface is not asked to be both. This is what makes D5 affordable. | +| **D7** | `/session` is a **client** command in `clientCommands`, never sent to the agent. | The only mechanism that works mid-turn: `handleClientCommand` is checked *before* `sendMessage` (`desktop_chat_pane.dart:170`, `session_screen.dart:671`). Bare → opens the panel; `id` → copies the bare id. | +| **D8** | The panel is **read-only**. No rename, no regenerate, no delete, no resume button. | Lifecycle already lives in the same menus (Close / Quit agent) and must not sit one mis-tap from the copy row. | +| **D9** | A row whose value is unmeasured is **omitted**, never rendered blank or as a placeholder. `Copy all` then copies fewer lines. | Same rule SPEC-37 settled on for the ring. A fabricated path is worse than no path: it will be pasted into a prompt and the next agent will report it missing. | +| **D10** | Per-agent vocabulary is a **lookup table** keyed by `SessionDTO.agent` (not a `switch`): pi → label `pi session`, resume `pi --session `; codex → label `Thread`, resume `codex resume `; anything else → label `Agent session`, **no** resume row. | Codex's own word for it is a thread (`thread/start` → `thread.id`). An unknown ACP agent gets no resume line because we do not know its CLI — inventing one is D9's failure mode in command form. A table rather than a `switch` because `docs/ENGINEERING.md`'s OCP rule is explicit: adding an adapter must not mean editing a growing `switch`. The safe default *is* the open/closed escape hatch — a third agent works unedited, just without a resume line. | +| **D11** | One host-agnostic body (`SessionIdentityDetails`) presented as a modal bottom sheet on mobile and a **centred, window-clamped panel** on desktop. | Originally specified as a `MenuAnchor` popover, "verbatim the `ContextUsageButton` / `ContextUsageDetails` split (`context_usage.dart:199-300`)". **Amended on review of the implementation (deviation 8):** the mechanism does not transfer, because the door topology differs. `ContextUsageButton` is a *persistent* composer control, so a `MenuAnchor` has something to stay anchored to; both of this panel's desktop doors are *transient menu items*, and by the time one is chosen its menu is gone. What the split actually contributes — the window-clamped width and the `SingleChildScrollView`, so a panel opened from a narrow split pane cannot hang off-screen — is kept and is now pinned by tests. | +| **D12** | **CUT from P1 → P2.** No `SessionIdentitySection` inside `ContextUsageDetails`. The ring's absence rule (`context_usage.dart:218`) is untouched, as before. | Cut on review, and the argument is this spec's own: the ring is absent in the four states above, *including the likeliest moment of need*, so a door hung off it is missing exactly when it is wanted — near-zero marginal value on top of the two menu doors and `/session`. It is not free either: it adds a `session_identity → context_usage` import edge, and it would put stacked mono rows beside `_Row`'s label/value rows in one panel. Deferring also deletes a whole task whose test was checking the wrong invariant (see §Review findings). | +| **D13** | **Two** panel doors, in menus that already exist and are always present: mobile `_glassMenu` (`session_screen.dart:505`) and the desktop pane-header kebab (`pane_header.dart:154`). The desktop **tab** menu gets **Copy session id** only. | Cut from three on review: a tab-menu *Session details…* is redundant with a pane-header kebab one pixel away on the same platform. The tab menu keeps **Copy session id** because that is a different job (right-click → one click → done), not a second way to open the same sheet. Zero permanent chrome either way; SPEC-40's 375 pt crowding is untouched. | +| **D14** | The clipboard payload is produced by one **pure** function, `sessionIdentityText()`, shared verbatim by the panel, `/session` and both menus. Format: one `label: value` per line, labels padded to a common width, absolute paths, omitted rows absent. | Pure ⇒ unit-tested directly, the same seam `formatTokens` / `headroomLabel` use. One function ⇒ the copy contract cannot diverge between four call sites. Plain lines survive being pasted into a prompt, a commit message, an issue or a terminal comment; markdown or JSON would need escaping. | +| **D15** | The resume command carries the **full** id, never a shortened prefix. | pi documents `--session ` as accepting a "partial UUID", and the mockup used `pi --session 019ff121`. **That is unsafe and was caught before implementation:** pi session ids are UUIDv7, whose first 48 bits are a millisecond timestamp, so an 8-char prefix pins only the top 32 bits and leaves ~65 s of ambiguity. Real collisions exist on this machine — `~/.pi/agent/sessions/--Users-le-.worktrees-makit-when-we-migrated-to-pi-acp-server--/` holds `019fa9f4-443d-…` **and** `019fa9f4-d3c8-…`, and another directory has four files sharing `019f8471`. A copy button that emits an ambiguous resume command is worse than no button. | +| **D16** | `transcriptPath` for **codex** is P2, not P1. P1 ships codex's `Thread` id and resume command with no path row. | The rollout lives at `~/.codex/sessions/YYYY/MM/DD/rollout--.jsonl` — a date-sharded walk with no existing helper, unlike pi's one-directory lookup. D9 lets P1 simply omit the row, so nothing is fabricated and nothing needs redesigning later. | +| **D17** | No new event kind, no broadcast, no watch. | Identity is session *meta*, not a session event. It changes at most once per session (when the adapter starts and `session.ts:388` assigns it), and `metaChanged` already fans out a fresh snapshot. | +| **D18** | **Accessibility is a locked decision, not a follow-up.** The `Copy all` row carries an explicit semantics label naming what it copies and how much (`Copy session details, 4 lines`). Each value row is a `Semantics` node whose label is the human label plus the value; the raw uuid is **not** re-spelled character by character. Covered by a widget test, because the row-QA harness cannot drive hover or VoiceOver. | SPEC-47 D17 already locked a11y for this exact surface family; this spec had nothing, which review correctly called a house regression. A copy-only panel is the worst place to omit it: a screen-reader user cannot see that a tap succeeded, so the label must say what the affordance does *before* the toast confirms it. | +| **D19** | The panel **watches** `sessionIdentityProvider` and rebuilds; it does not capture a snapshot at open time. | A draft's panel can be opened *before* the adapter assigns `agentSessionId`. That assignment fans out a fresh snapshot (D17), so a watching panel fills in its rows live, and a snapshotting one lies until reopened. One `ref.watch` instead of one `ref.read`, plus a test that drives the id in while the panel is open. | +| **D20** | Labels are hard-coded English, singular-only (`pi session`, `Transcript`, `Resume with`, `Copy all`). | Mirrors SPEC-47 D20 verbatim. Called out so review does not re-open it. | +| **D21** | `transcriptPath` discloses the **server host's** absolute filesystem layout — including its username — to any paired device. Accepted, and written down here. | Consistent with what the wire already carries (`worktreePath`, `cwd`, port paths) under the LAN/tailnet pairing trust model: a paired device is already trusted with the repo's contents. `docs/ENGINEERING.md` requires the disclosure be stated rather than assumed, and it is the reason the path is **read-only** on the wire (D8) — the app never sends a path back for the server to open. | + +## What P1 does not do + +- **No codex transcript path** (D16). The row is absent for codex; the id and resume line are not. +- **No Share transcript.** Getting the file from the server host to the phone is the media / + file-serving path, not a clipboard. It is drawn in the mockup marked `follow-up` and is P3. +- **No `⌘I` shortcut.** Proposed in the mockup, deliberately unclaimed until `lib/shortcuts/` + is audited for a conflict. P4. +- **No transcript *viewer*.** Copying a path is the feature; rendering a foreign transcript is + a different one. +- **No right-click menu on the chat body.** There is no `onSecondaryTap` on the transcript + anywhere in `app/lib/` today (the only two are `split_view.dart:641` and + `groups/group_bar.dart:133`). Claiming secondary-click over the transcript would fight text + selection and `chat_message.dart`'s per-message copy for a value that is not per-message. +- **No identity section inside the context-usage panel** (D12, cut to P2). The ring's absence rule is + untouched and no new control enters the composer footer. +- **No third panel door** on the desktop tab menu (D13) — that menu gets **Copy session id** only. + +## What P1 reuses + +| Reused | From | Instead of | +| --- | --- | --- | +| `piSessionsDir()` + slug algorithm | `server/src/pi-sessions.ts:52` | re-deriving pi's layout | +| `agentSessionId` persistence + rehydration | SPEC-29, `storage/sqlite_event_store.ts:142` | a new table or a live-only field | +| optional-field-on-`SessionDTO` pattern | SPEC-47 D12 (`createdAt`) | a new `cmd` kind | +| host-agnostic body + sheet/popover split | `context_usage.dart:199-300` | two divergent panels | +| `themedMenuItem` | `app/lib/ui/widgets/` (used by all three menus) | bespoke menu rows | +| `Clipboard.setData` + `status.info('… copied', detail:)` | `port_detail_sheet.dart:224-229` (the only site passing `detail:`) | a new toast surface | +| pure-formatter-with-unit-tests seam | `context_usage.dart` (`formatTokens`, `headroomLabel`) | logic inside a widget | +| `ClientCommand` record + `handleClientCommand` interception | `client_commands.dart:26-66` | a new send-path branch | + +## Phases + +| Phase | Contents | +| --- | --- | +| **P1** (this spec) | D1–D15, D17: wire fields, pi path resolution, `sessionIdentityText()`, the panel, three menu doors, `/session`, the usage-panel section | +| **P2** | codex rollout-path resolver (D16) | +| **P3** | Share transcript (file transport to the device) | +| **P4** | `⌘I` after a `lib/shortcuts/` audit | + +## Review findings applied (rev 2) + +Two independent reviews ran against rev 1 before any code was written: one for technical +correctness (told to verify every citation and *run* every command against the real binaries), +one for engineering practice (TDD/SOLID/YAGNI/house style, with a mandatory `CUT THIS` list). + +**Measured baselines, to be preserved:** `tsc -p . --noEmit` clean · `pnpm test` **1313 pass / 0 +fail** · `flutter analyze --fatal-infos --no-pub` "No issues found". + +| Finding | Disposition | +| --- | --- | +| **D3 was unbuildable as written** — `Session` has no project path, and pi's slug follows the *worktree* cwd, so the cited precedent would have missed every worktree-bound session. | **Accepted, and fixed beyond the proposal.** Resolution moved to `SessionManager`; cwd = `worktreePath ?? project.dto.path`; memoized including misses, because the reviewer's "resolve at projection time" would have done a `readdir` per session per broadcast. | +| `resolveTranscriptPath` should not live in `pi-sessions.ts` — steps (a) and (c) are agent-agnostic, so P2/codex would force a move. | **Accepted.** New `server/src/transcript-path.ts` owns the dispatcher and delegates to `pi-sessions.ts`; P2 becomes additive. | +| D10's per-agent `switch` violates the house OCP rule. | **Accepted.** Lookup table with a safe default (D10). | +| D12 (identity inside the usage panel) is missing exactly when it is needed, and adds coupling. | **Accepted — cut to P2** (D12). | +| Three panel doors is one too many; the tab-menu *Session details…* duplicates the pane kebab. | **Accepted — two doors** (D13); the tab keeps *Copy session id*. | +| No accessibility decision at all, where SPEC-47 locked one. | **Accepted — D18**, with a widget test. | +| A panel opened before the id is assigned would show stale rows. | **Accepted — D19** (watch, don't snapshot). | +| `transcriptPath` leaks the server's FS layout; state it. | **Accepted — D21.** | +| i18n divergence from SPEC-47 D20. | **Accepted — D20.** | +| Unproven or misdirected tests: `transcriptPath` never asserted in the projection; provider task had no test at all; the usage-panel test could not fail; label-padding and `/session`-branch mutations missing. | **Accepted** — all folded into the plan's rev-2 task list. | +| D15's collision risk is real — and worse than stated: `pi --session 019fa9f4` does **not** error on ambiguity, it silently resolves to one session and offers to fork it. | **Accepted as reinforcement.** D15 stands; the silent-wrong-session behaviour is now the stated reason. | +| Consider cutting the `resume` row entirely — it is the sole reason D10 and D15 exist. | **Rejected.** It is the highest-value line for the stated goal (handing work to a second session), and D15's cost is already paid. Cutting it would leave the user to reconstruct a command from an id, which is the manual step this feature exists to remove. | +| Persist `transcriptPath` so cold sessions report it without re-resolving. *(considered, not raised)* | **Rejected.** Needs a schema migration for a value that the memoized lookup recomputes in one `readdir` on the session's first projection after a restart. | +| D17, D12 self-consistency, D1's wire optionality, and the `pi-acp`-reuses-pi's-uuid claim all verified correct under attack. | No change. | + +## Verification + +Required evidence, not claims. Recorded as measured: + +1. `cd server && node_modules/.bin/tsc -p . --noEmit` clean; `pnpm test` **1326 pass / 0 fail** + (baseline 1313 — +13 net new, nothing regressed). ✅ +2. `cd app && flutter analyze --fatal-infos --no-pub` → "No issues found!"; + `flutter test --no-pub` → **0 non-`loading` failures**. The 15–17 reported failures are all of + the form `loading `, and the set varies run to run; each one passes when run directly. + That is the recorded flake baseline (harness load timeout under full-suite concurrency), not a + regression. The 71 SPEC-52 tests are green run as a set. ✅ +3. Every new test's bite proven by reverting **only** the production line — 9 mutations across the + two trees, listed in the P1c commit body. The two load-bearing ones: `cwd` → project path fails + the worktree test, and relaxing the transcript suffix match to a prefix fails the D15 collision + test. A tenth — `handleClientCommand`'s exact name match → `startsWith` — is caught by + `test/session_command_test.dart`'s "`/sessions` is NOT intercepted". ✅ +4. **Pixel sign-off on the real macOS app**, not on a widget test: the panel rendered through + `app/tool/session_identity_demo.dart`, geometry read from the accessibility tree via + `cua-driver get_window_state` (AX space = Flutter logical px), row pitch and both uuid rows + measured at **one line**, compared against `mockups/session-identity.html`. Light-mode + `Copy all` measured 10.95:1 contrast (needs 4.5); line counts 4/3/3/2/1 all correct. That gate + — not any test — is what found the "1 lines" pluralisation bug, now asserted in both the + visible and the semantics label. ✅ +5. **Live probe** (throwaway, deleted): `resolveTranscriptPath` run against the real + `~/.pi/agent/sessions` tree, for this branch's own worktree, with the pi session id of the + session that implemented the feature. It returned + `…/--Users-le-.worktrees-makit-feat-get-session-id--/2026-08-11T14-01-46-945Z_019ff121-….jsonl` + — byte-identical to the path pi itself reports for that session. The same call with the 8-char + prefix `019ff121` returned `undefined`, so D15 holds on real data. Non-pi, draft, and + unreadable-dir inputs all returned `undefined` without throwing (boundary rule). `pi --help` + confirms the resume line's spelling, `--session `, which accepts both the full id and + the transcript path the panel offers. ✅ diff --git a/mockups/session-identity.html b/mockups/session-identity.html new file mode 100644 index 00000000..3623f590 --- /dev/null +++ b/mockups/session-identity.html @@ -0,0 +1,694 @@ + + + + + +makit — Session identity: copy the id and its transcript + + + + + + +

Session identity — copy the id, and its transcript

+

+ A door to the underlying agent session id that works while the agent is mid-turn, because that is + exactly when you need it. Values on this page are real captures: this session's pi id + 019ff121-1cc1-7c60-bc40-65890c87e6ff and transcript under + ~/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/; a real codex thread + 019efe19-101b-7183-8345-47f61b78dd61 with its rollout under ~/.codex/sessions/2026/06/25/. +

+ +
+ ✗ Why /session in the composer cannot solve this. + /session is an agent command: it is not in clientCommands + (app/lib/ui/composer/client_commands.dart — new, cancel, unpair, help, ask, compact, + thinking, model, name), so the send path falls through to store.sendMessage + (desktop_chat_pane.dart:183). Mid-turn the server cannot steer it into the running turn, so it + lands in Session.queued (server/src/session.ts, cap + MAX_QUEUED_MESSAGES = 50) and only runs on the next idle transition — after the work you wanted + to hand off has already finished. +
+ +
+ ✓ The lever already exists. handleClientCommand is checked + before the send/queue path (desktop_chat_pane.dart:170, + session_screen.dart:671) and never touches the wire. Any answer built on a client command or a + menu is available at 100% of a turn — including while the agent is writing. + And the data is already on the server: Session.agentSessionId (session.ts:163, + the native ACP sessionId / codex threadId) and + Session.resumeSessionPath (session.ts:155) are both persisted through + SqliteEventStore. They are simply not on SessionDTO + (protocol.ts:689), so the app has never seen them. +
+ +
+ ⚠ The context-usage ring is the wrong single home — and this is measurable, not aesthetic. + ContextUsageButton opens with + if (usage == null || fraction == null) return const SizedBox.shrink(); + (context_usage.dart:218). So the ring is absent: +
    +
  • before turn 1 — the single most likely moment you want the id, to seed a second session;
    • +
  • on pi without the makit-pi-usage extension installed;
    • +
  • after a pi compaction, until the next reply (pi nulls the count);
    • +
  • whenever an agent reports cost or tokens but no window.
    • +
+ A control that vanishes in four states cannot be the only route to a value you need in all of them. + It is still a good second route, because the panel is already becoming the session's fact sheet + (SPEC-37 usage + SPEC-47 SessionEffortSection) — so identity is appended there too, for free. +
+ +
+ ✎ Amended in planning, before a line was written (SPEC-52). Six things on this page were + wrong, and all six are corrected above — the frames now show what will ship. +
    +
  • The short resume id was unsafe. This page showed pi --session 019ff121, leaning on + pi's documented “partial UUID” support. pi session ids are UUIDv7, whose first 48 bits are a + millisecond timestamp — so an 8-char prefix pins only the top 32 bits and leaves ~65 s of + ambiguity. Real collisions exist on this machine: one sessions directory holds + 019fa9f4-443d-… and 019fa9f4-d3c8-…, and another has four files sharing + 019f8471. An ambiguous resume command is worse than no button, so every resume row now + carries the full 36-char id (spec D15).
    • +
  • The ~/ abbreviation was unimplementable. The path belongs to the server host, + not the phone — and the app cannot know that host's home directory. Faking it needs either a + /Users/<x>/ heuristic (wrong on a Linux host, wrong for a non-standard home) or shipping + homeDir on the wire for pure cosmetics. Paths are now absolute everywhere — wire, + clipboard and display — and the row simply wraps (spec D4).
    • +
  • Row labels and the section title are sentence case, not uppercase. Rev 1 set them + text-transform:uppercase; the shipped design system does not — + ContextUsageDetails ships Context usage and rows labelled + Session total / Input / Cost. Matching the panel this one sits + beside beats matching a local choice on this page.
    • +
  • The in-panel footnotes are gone. Explanatory prose belongs in this document, not inside a + panel you read once and then read past forever.
    • +
  • “1 line”, not “1 lines”. Found by the pixel gate on the real macOS app, on the one case the + tests had not exercised: a stub session whose only measured value is the makit id. Now asserted.
    • +
  • codex’s transcript row is P2, not P1: the rollout is date-sharded + (~/.codex/sessions/YYYY/MM/DD/) with no existing helper, whereas pi’s is a single-directory + lookup through already-tested code. D9 lets P1 omit the row rather than fabricate it (spec D16).
    • +
+
+ + +
+
+

One panel, two doors

recommended +

Both doors open the same SessionIdentityDetails — sheet on mobile, popover on desktop, exactly the ContextUsageDetails pattern.

+
+
+
+
+
The panel · pi session, mid-turn
+
+
Session
+
+
+ pi session + 019ff121-1cc1-7c60-bc40-65890c87e6ff +
+
+ Transcript + /Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl +
+
+ Resume with + pi --session 019ff121-1cc1-7c60-bc40-65890c87e6ff +
+
+ makit session + 7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c +
+
+
Copy all4 lines
+
Reveal transcript in Finder
+
Share transcript…follow-up
+ +
+
+ +
+
Door 1 — /session, a client command
+

Added to clientCommands next to /name. Intercepted before + sendMessage, so it runs mid-turn and never enters the queue. Bare + /session opens the panel; /session id puts the agent id — and + only the id — on the clipboard and toasts. That is the two-keystroke path for the actual + complaint, and it is why the panel itself does not need per-value buttons.

+
+ + Session id copied019ff121-1cc1-7c60-bc40-65890c87e6ff +
+

Toast is status.info('… copied', detail: …) — the same + call port_detail_sheet.dart:225 and settings_screen.dart:92 already make. + No new notification surface.

+ +
Door 2 — the ⋯ / right-click menus
+

One new item, Session details, in the two menus that already exist and are + always present: mobile's glass overflow (session_screen.dart:_glassMenu) and the desktop + pane header kebab (panes/pane_header.dart:154). The desktop tab context menu + (split_view.dart:_showContextMenu) gets Copy session id only — a third way to + open the same sheet, one pixel from the kebab, was cut on review.

+ +
Not a third door — inside the usage panel cut to P2
+

Drawn in rev 1 as SessionIdentitySection appended below + SessionEffortSection. Cut on review, using this page's own argument: the ring is + absent in the four states above — including before turn 1, the likeliest moment of need — so a door + hung off it is missing exactly when it is wanted. It also added a + session_identity → context_usage import edge and would have put stacked mono rows beside + _Row's label/value rows in one panel. Deferred to P2.

+

Not a fourth door: right-click on the transcript. The chat body has no + onSecondaryTap anywhere in lib/ today — the only two are on the tab strip + and the group bar. Claiming secondary-click over the transcript would fight text selection and + chat_message.dart's own copy affordances for a value that is not per-message. The tab + menu is one pixel away and already a menu.

+
+
+
+
+ + +
+

iPhone — mid-turn, the pain case

recommended +

Agent is writing. The ⋯ menu and /session both answer now; a typed /session to the agent would not.

+
+
+ +
+
1 · ⋯ overflow, agent busy
+
+
9:41
􀙇 􀛨
+
+ + Get session idfeat-get-session-id · pi + +
+
+
mockup the session-id UI first
+
Reading mockups/context-usage.html …
+
Grepping agentSessionId in server/src …
+
Working · 1m 12s
+
+
+
Message…
+
+
+ piSonnet 4.6 + Approval +
+ + + +
+
+
+
+
+
Rename session
+
My messages
+
Session details
+
+
Close session
+
+
+

One item, above the rule, with Rename and My messages — all three + read state the client already holds and are not capability-gated. Model/Thinking (which are gated) + stay below.

+
+ +
+
2 · the sheet
+
+
9:41
􀙇 􀛨
+
+
+
+
Session
+
+
+ pi session + 019ff121-1cc1-7c60-bc40-65890c87e6ff +
+
+ Transcript + /Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl +
+
+ Resume with + pi --session 019ff121-1cc1-7c60-bc40-65890c87e6ff +
+
+ makit session + 7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c +
+
+
Copy all4 lines
+
Share transcript…
+ +
+
+

Row order is usefulness, not hierarchy: the agent id first + because it is what gets pasted; makit's own uuid last because it is only ever needed for a bug report. + The path wraps rather than ellipsizing — a truncated path is worthless, and this sheet exists to be + read from as well as copied.

+
+ +
+
One button — what it puts on the clipboard
+
pi session: 019ff121-1cc1-7c60-bc40-65890c87e6ff +transcript: /Users/le/.pi/agent/sessions/--Users-le-.worktrees-makit-feat-get-session-id--/2026-08-11T14-01-46-945Z_019ff121-1cc1-7c60-bc40-65890c87e6ff.jsonl +resume: pi --session 019ff121-1cc1-7c60-bc40-65890c87e6ff +makit session: 7c9e6d5a-1f42-4b8e-9a01-2d3f4e5a6b7c
+

Labelled plain lines, and the path is absolute — on screen and on the clipboard. + The panel shows the same absolute path it copies: ~/ was dropped everywhere (see the + amendment above, spec D4), because the path belongs to the server host and the app cannot + know that host's home directory. It is also the right payload regardless: what receives it is + another agent's shell or prompt, and ~ only expands if a shell gets there first. + One key: value per line survives being pasted into a prompt, a commit message, an issue + or a terminal comment — no markdown, no JSON, no fences to escape.

+

What one button costs, honestly: when you want only the id, you now get + four lines and have to trim. That is the trade — and it is paid off elsewhere rather than in this + panel: /session id and the tab menu's Copy session id both copy the bare id and + nothing else. So the panel answers “give me the context” and those two answer “give me the id”, + instead of one surface trying to be both with four ambiguous 24pt targets.

+

It also fixed the row layout. Dropping the copy column returned ~116px to each + value, which is what lets a 36-char uuid sit on one line (≈248px of 312) instead of wrapping mid-uuid + — and a wrapped uuid is the single worst thing this panel could do, because it invites a partial + selection.

+

The transcript row disappears rather than lies. Present only when the server + resolved a real file — resumeSessionPath verbatim, or a pi path derived from + piSessionsDir(cwd). “Copy all” then copies three lines, not four with a blank. A path + that does not exist on the server host is worse than no row: it will be pasted into a prompt, and the + next agent will report it missing.

+

Read-only by construction. No rename, no regenerate, no delete. Session + lifecycle already lives in this same menu (Close / Quit agent) and must not sit one mis-tap from the + copy row.

+
+ + Session details copied4 lines · pi session, transcript, resume, makit session +
+

And it all works while the agent is busy. Nothing here talks to the agent — every + value is already in the session snapshot the client holds — so there is no request to queue and no + “wait for idle” state to design.

+
+ +
+
+
+ + +
+

macOS — pane kebab and tab right-click

recommended +

Popover anchored like the usage panel (MenuAnchor, 300–340pt, window-clamped).

+
+
+
+
Pane header ⋯ → Session details
+
+
+ + makit — makit +
+
+
+
feat-get-session-id
+
feat-serving-html
+
main
+
+
+
+
Get session id
+
Review PR 157
+
+
+ Get session id + + +
+
+
mockup the session-id UI first
+
Working · 1m 12s
+
+
+
+
+
+
+
Rename session
+
Session details⌘I
+
+
Quit agent
+
+
+
+ +
+
Tab right-click · today vs proposed
+
+
+
+
Rename session
+
+

as built one item.

+
+
+
+
Rename session
+
Copy session id
+
+

proposed Copy session id only — the + zero-dialog path: right-click the tab, one click, it is on the clipboard. Fastest route of all, + ~12 lines. A Session details… item was here in rev 1 and was + cut: it duplicated the pane-header kebab on the same platform.

+
+
+

⌘I is proposed, not assumed — it must be checked against + lib/shortcuts/ before it ships. If taken, the menus alone are enough; the shortcut is + the cheap part, not the point.

+

Why not a new sidebar row or a status-bar chip: the id is needed a few times a + day, by one person, for one purpose. Permanent chrome for an occasional copy is how a footer ends up + at 500pt of natural width on a 375pt phone (see context-usage.html). Menus are free.

+
+
+
+
+ + +
+

Per-agent and per-state truth

+

What each row can honestly say. Absent ≠ empty: a row with no value is omitted, never shown blank.

+
+
+
+
pi (ACP) · live
+
pi session019ff121-…-c87e6ff
+
Transcript…/2026-08-11T14-01-46-945Z_019ff121-….jsonl
+
Resumepi --session 019ff121-1cc1-7c60-bc40-65890c87e6ff
+

Path resolved server-side by piSessionsDir(cwd) + (pi-sessions.ts:52) + the one file whose name ends _<id>.jsonl. + Existing, tested code.

+
+
+
codex · live
+
Thread019efe19-101b-7183-8345-47f61b78dd61
+
RolloutP2 — row omitted in P1
+
Resumecodex resume 019efe19-101b-7183-8345-47f61b78dd61
+

Label is Thread, codex's own word (thread/start → + threadId). The rollout sits at + ~/.codex/sessions/YYYY/MM/DD/rollout-<ts>-<threadId>.jsonl — a date-sharded walk + with no existing helper, unlike pi's one-directory lookup, so P2. D9 lets + P1 omit the row rather than fake it.

+
+
+
Draft (pending)
+
Agentnot started yet
+
makit session7c9e6d5a-…
+

pending: true, no worktree, no agent → no native id exists. The panel says so + in one line instead of showing an empty row.

+
+
+
Closed / cold (SPEC-29)
+
pi session019ff0a3-…
+
Stateclosed · resumable
+

Works because agentSessionId is persisted, not live state + (sqlite_event_store.ts:142). This is a real win: the id of a session you closed + yesterday is still copyable.

+
+
+
Attached from disk
+
Transcript/tmp/prior.jsonl
+

Sessions resumed via attachPiSession already carry + resumeSessionPath verbatim — show it as-is, do not re-derive.

+
+
+
Stub / detached
+
Agentno native session
+

DetachedAdapter.agentSessionId = undefined + (detached.ts:16). Menu item stays enabled — the makit id is still worth a bug report — + but the agent rows are gone.

+
+
+

One rule for the whole panel: every row is a measurement or it is absent. No + placeholder dashes, no "unknown", no fabricated path. That is the same rule + context-usage.html settled on for the ring, and it is why this panel can be trusted enough + to paste from.

+
+
+ + +
+

Rejected alternatives

+

All were cheaper to describe than the chosen shape; each fails on a state that actually occurs.

+
+ + + + + + + + + + + + + + + + + + + + + + +
VariantCostWhy it lost
Teach pi's /session to run mid-turnserver + pi✗ Not makit's to fix. Mid-turn steering is the agent's capability; the queue is the + honest fallback. And it would still be pi-only.
Ring / usage panel only~30 lines✗ Absent in 4 states including before turn 1 — the likeliest moment of need + (context_usage.dart:218).
Session id in the pane header subtitle0 new surfaces✗ A 36-char uuid in a 12pt title strip ellipsizes to nothing, and it is permanent + chrome for an occasional need.
Right-click the transcript body~40 lines✗ No onSecondaryTap exists on the chat body today; claiming it fights + text selection and per-message copy.
Long-press the agent pill in the footer~15 lines✗ Invisible affordance, and the pill already owns tap (model picker). Two gestures on + one 60pt target.
Settings → Diagnostics screen~20 lines✗ Four navigations away from the session, and not per-session at all.
Four per-row copy buttons~10 lines✗ Superseded. Four 24pt targets stacked 30pt apart on a phone, a toast that + cannot say which one you hit, and 116px stolen from every value — enough to wrap a 36-char uuid + mid-string, which invites a partial selection. Single-value copies moved to /session id + and the tab menu instead, where they cost nothing.
Identity section inside the usage panel (rev 1, cut)~4 lines✗ Missing in exactly the four states where the ring is absent — including before turn 1. + Adds an import edge and mixes two row styles in one panel. Deferred to P2.
A third panel door on the tab menu (rev 1, cut)~6 lines✗ Duplicates the pane-header kebab one pixel away on the same platform. The tab keeps + Copy session id, which is a different job.
Client command + menu items → one panel, one Copy allsee deltas✓ Works mid-turn, works before turn 1, works on a closed session, works on both agents, + adds no permanent chrome, and reuses three existing patterns.
+
+
+ + +
+

Deltas — ordered by value per line changed

+

Sizes are estimates for the shipping slice, tests excluded.

+
+ + + + + + + + + + + + + + + + + + + + + +
#FileSizeChange & why
1server/src/protocol.ts+8Add agentSessionId? and transcriptPath? to SessionDTO. + Optional on the wire so a new app against an old server renders fewer rows, never a fabricated one — + the same rule createdAt follows.
2server/src/manager.ts+10Populate both in the DTO projection: agentSessionId verbatim; + transcriptPath = resumeSessionPath when set, else resolved for pi via + piSessionsDir(cwd) + suffix match on _<id>.jsonl.
3app/lib/store/models.dart+6Two nullable fields on the session model + fromJson. Nullable, not defaulted — + an empty string here would render a copy button that copies nothing.
4app/lib/ui/session/session_identity.dart~150 newSessionIdentityDetails (host-agnostic body), a stacked label/value row (no per-row + button), one Copy all action row, showSessionIdentity() (sheet on mobile, popover + on desktop), and the per-agent labels. Modelled line-for-line on ContextUsageDetails / + port_detail_sheet.dart.
5↳ sessionIdentityText() in the same file~25Pure function: session → the labelled multi-line clipboard payload, absolute paths, rows omitted + when unmeasured. Pure so it is unit-tested directly — the same seam formatTokens / + headroomLabel use in context_usage.dart, and the only place the copy + contract lives. Shared verbatim by the panel, /session and both menus.
6app/lib/ui/composer/client_commands.dart+22/session — the fix for the actual complaint. Bare opens the panel; + /session id copies the bare id and toasts. Sits with /name: not + capability-gated, reads only what the client holds.
7app/lib/desktop/chat/split_view.dart+14Copy session id in the tab context menu (bare id, zero dialogs — the fastest door, and the + counterweight to “Copy all”). Only that: a Session details… item here was cut on + review (D13) as a third door onto the same sheet, one pixel from the pane-header kebab on the same + platform — see the tab menu above.
8app/lib/desktop/chat/panes/pane_header.dart+6Session details in the kebab, above the rule.
9app/lib/ui/session/session_screen.dart+6Same item in _glassMenu, grouped with Rename / My messages.
+

Deliberately NOT changed: the ring's absence rule + (context_usage.dart:218) stays exactly as SPEC-37 decided — this design works around + it rather than weakening it; the footer gains no control, so the 375pt crowding problem is untouched; + no new wire command (the ids ride the existing session snapshot, so they arrive with the session and + cost zero round-trips mid-turn); attachPiSession and the resume path are read-only here.

+

Follow-ups, explicitly out of the first slice: + (a) the codex rollout-path resolver (date-sharded walk under ~/.codex/sessions/); + (b) Share transcript…, which needs the file to travel from the server host to the phone — that is + the media/file-serving path, not a clipboard; + (c) a ⌘I binding, pending a check of lib/shortcuts/.

+
+
+ + + diff --git a/server/src/manager.test.ts b/server/src/manager.test.ts index ec637923..611f0dc0 100644 --- a/server/src/manager.test.ts +++ b/server/src/manager.test.ts @@ -2877,3 +2877,143 @@ test("reopenSession on an already-open session is a no-op", async () => { store.close(); } }); + +// --- SPEC-52 C1b: agentSessionId + transcriptPath in the projected DTO --------- + +// A real UUIDv7 pi session id (see transcript-path.test.ts for the collision pair). +const SPEC51_ID = "019fa9f4-443d-7d86-8f4c-d9c4988ddf4f"; + +/** Seed a pi transcript for `cwd` named `_.jsonl`; returns its path. */ +function seedPiTranscript(agentDir: string, cwd: string, id: string): string { + const dir = piSessionsDir(cwd, agentDir); + mkdirSync(dir, { recursive: true }); + const path = join(dir, `2026-01-01T00-00-00-000Z_${id}.jsonl`); + writeFileSync( + path, + JSON.stringify({ type: "session", version: 3, id, timestamp: "2026-01-01T00:00:00.000Z", cwd }) + "\n", + ); + return path; +} + +/** Run `body` with a throwaway MAKIT_PI_AGENT_DIR, restoring + cleaning after. */ +function withSpec51AgentDir(body: (agentDir: string) => void): void { + const agentDir = mkdtempSync(join(tmpdir(), "makit-c1b-")); + const prev = process.env.MAKIT_PI_AGENT_DIR; + process.env.MAKIT_PI_AGENT_DIR = agentDir; + try { + body(agentDir); + } finally { + if (prev === undefined) delete process.env.MAKIT_PI_AGENT_DIR; + else process.env.MAKIT_PI_AGENT_DIR = prev; + rmSync(agentDir, { recursive: true, force: true }); + } +} + +test("listSessions resolves transcriptPath from the WORKTREE slug, not the project slug (SPEC-52 D3)", () => { + withSpec51AgentDir((agentDir) => { + const store = new SqliteEventStore(); + const projectPath = "/repo/root"; + const worktreePath = "/repo/root/.wt/feat-x"; + // The SAME id is seeded under BOTH slugs: a project-slug resolver would find + // the decoy, so the test can only pass if the worktree slug is used. + seedPiTranscript(agentDir, projectPath, SPEC51_ID); // decoy + const wtFile = seedPiTranscript(agentDir, worktreePath, SPEC51_ID); + store.saveSession({ + id: "sess-wt", + projectId: "proj-x", + agent: "pi", + title: "wt work", + status: "idle", + policy: "ask-on-risky", + createdAt: 1, + lastActivityAt: 2, + lastPreview: "", + agentSessionId: SPEC51_ID, + branch: "feat-x", + worktreePath, + }); + try { + const mgr = new SessionManager({ projects: [{ id: "proj-x", path: projectPath }], store }); + const dto = mgr.listSessions().find((d) => d.id === "sess-wt")!; + assert.equal(dto.transcriptPath, wtFile); + } finally { + store.close(); + } + }); +}); + +test("listSessions projects transcriptPath into the DTO (SPEC-52)", () => { + const store = new SqliteEventStore(); + // A resumeSessionPath is authoritative and dir-independent, so this proves the + // whole path-into-DTO wiring without depending on a slug lookup. + seedColdSession(store, "sess-path", { agentSessionId: "pi-x", resumeSessionPath: "/disk/transcript.jsonl" }); + try { + const mgr = new SessionManager({ projects: [], store }); + const dto = mgr.listSessions().find((d) => d.id === "sess-path")!; + assert.equal(dto.transcriptPath, "/disk/transcript.jsonl"); + } finally { + store.close(); + } +}); + +test("listSessions projects agentSessionId into the DTO (SPEC-52)", () => { + const store = new SqliteEventStore(); + seedColdSession(store, "sess-id", { agentSessionId: "pi-42" }); + try { + const mgr = new SessionManager({ projects: [], store }); + const dto = mgr.listSessions().find((d) => d.id === "sess-id")!; + assert.equal(dto.agentSessionId, "pi-42"); + } finally { + store.close(); + } +}); + +test("a draft projects neither agentSessionId nor transcriptPath (SPEC-52 D9)", async () => { + const cwd = mkdtempSync(join(tmpdir(), "makit-c1b-draft-")); + try { + const mgr = new SessionManager({ projects: [cwd], adapterFactory: () => stubAdapter([]) }); + const projectId = mgr.listProjects()[0].id; + const draft = await mgr.spawnPendingSession(projectId); + const dto = mgr.listSessions().find((d) => d.id === draft.id)!; + assert.equal(dto.agentSessionId, undefined); + assert.equal(dto.transcriptPath, undefined); + } finally { + rmSync(cwd, { recursive: true, force: true }); + } +}); + +test("a closed/cold session still projects its agentSessionId (SPEC-29 persistence)", async () => { + const store = new SqliteEventStore(); + seedColdSession(store, "sess-closed", { agentSessionId: "pi-cold", closed: true }); + try { + const mgr = new SessionManager({ projects: [{ id: "proj-x", path: "/repo/root" }], store }); + const dto = (await mgr.listClosedSessions()).find((d) => d.id === "sess-closed")!; + assert.equal(dto.agentSessionId, "pi-cold"); + } finally { + store.close(); + } +}); + +test("two projections of the same session perform only ONE transcript resolution (memoization)", () => { + const store = new SqliteEventStore(); + seedColdSession(store, "sess-memo", { agentSessionId: "pi-memo" }); + let reads = 0; + try { + const mgr = new SessionManager({ + projects: [], + store, + // Inject the resolver so the readdir is observable (D3 memoization). + transcriptResolver: () => { + reads++; + return "/resolved/once.jsonl"; + }, + }); + const first = mgr.listSessions().find((d) => d.id === "sess-memo")!; + const second = mgr.listSessions().find((d) => d.id === "sess-memo")!; + assert.equal(first.transcriptPath, "/resolved/once.jsonl"); + assert.equal(second.transcriptPath, "/resolved/once.jsonl"); + assert.equal(reads, 1, "resolved once, then served from the memo"); + } finally { + store.close(); + } +}); diff --git a/server/src/manager.ts b/server/src/manager.ts index 0fb82e47..9378f15c 100644 --- a/server/src/manager.ts +++ b/server/src/manager.ts @@ -29,6 +29,7 @@ import { } from "./protocol.js"; import { spawnBoundError, spawnDepth, type LineageNode } from "./lineage.js"; import { listPiSessions, parseTranscript, type PiSessionMeta } from "./pi-sessions.js"; +import { resolveTranscriptPath, type TranscriptQuery } from "./transcript-path.js"; import { DetachedAdapter } from "./adapters/detached.js"; import { withDeadline } from "./adapters/deadline.js"; import { buildAdapter, piAcpSpec } from "./agent_factory.js"; @@ -126,6 +127,12 @@ export interface ManagerOpts { projects: Array; /** Override the production pi adapter, used by deterministic e2e tests. */ adapterFactory?: AdapterFactory; + /** + * Resolve a session's transcript path (SPEC-52 D3). Injected so tests can + * observe the (memoized) directory read; production uses the real resolver + * over the pi agent dir. + */ + transcriptResolver?: (q: TranscriptQuery) => string | undefined; /** * Called with the current `{ id, path }` list after every add/remove so the * caller can persist them (ids included, so they survive a restart). @@ -246,6 +253,15 @@ export class SessionManager extends EventEmitter { /** Tail of the per-repo worktree-creation chain (see withWorktreeCreateLock). */ private readonly worktreeCreateLock = new Map>(); private readonly adapterFactory?: AdapterFactory; + private readonly transcriptResolver: (q: TranscriptQuery) => string | undefined; + /** + * Per-session-id transcript path cache, INCLUDING misses (SPEC-52 D3). A + * `sessions.snapshot` is rebroadcast on every `metaChanged` (150ms coalesce), + * so resolving per projection would `readdir` per session per broadcast — a + * real regression. Memoized here it costs at most one read per session per + * server lifetime and zero I/O per snapshot. + */ + private readonly transcriptPathMemo = new Map(); private readonly onProjectsChanged?: (projects: PersistedProject[]) => void; private readonly defaultModel?: string; private readonly defaultAgentId: string; @@ -268,6 +284,7 @@ export class SessionManager extends EventEmitter { constructor(opts: ManagerOpts) { super(); this.adapterFactory = opts.adapterFactory; + this.transcriptResolver = opts.transcriptResolver ?? ((q) => resolveTranscriptPath(q)); this.onProjectsChanged = opts.onProjectsChanged; this.defaultModel = opts.defaultModel; this.defaultAgentId = "pi"; @@ -529,7 +546,7 @@ export class SessionManager extends EventEmitter { // Closed sessions (SPEC-29) are hidden from the ACTIVE list, but kept in // the registry (resumable + restorable). `allSessions()` still returns them // for fan-out/lookup; only this DTO list excludes them. - return [...this.sessions.values()].filter((s) => !s.closed).map((s) => s.toDTO()); + return [...this.sessions.values()].filter((s) => !s.closed).map((s) => this.projectSessionDTO(s)); } /** The closed sessions (SPEC-29), for the "Show closed" list. Newest first. @@ -552,7 +569,7 @@ export class SessionManager extends EventEmitter { liveByProject.set(session.projectId, live); } const orphaned = this.isOrphaned(session.worktreePath, project.dto.path, live); - out.push({ ...session.toDTO(), orphaned }); + out.push({ ...this.projectSessionDTO(session), orphaned }); } return out.sort((a, b) => b.lastActivityAt - a.lastActivityAt); } @@ -623,6 +640,42 @@ export class SessionManager extends EventEmitter { return (this.sessions.get(sessionId)?.events ?? []).slice(-limit); } + /** + * Project a session for the wire (SPEC-52 D1). `Session.toDTO()` cannot do + * this: `agentSessionId` is passed through verbatim, but `transcriptPath` + * needs the project's filesystem path, which the session does not hold + * (`projectId` + `worktreePath` only) — the manager does, via `this.projects`. + */ + private projectSessionDTO(session: Session): SessionDTO { + return { + ...session.toDTO(), + agentSessionId: session.agentSessionId, + transcriptPath: this.transcriptPathFor(session), + }; + } + + /** Memoized transcript-path lookup (SPEC-52 D3); see {@link transcriptPathMemo}. */ + private transcriptPathFor(session: Session): string | undefined { + if (this.transcriptPathMemo.has(session.id)) return this.transcriptPathMemo.get(session.id); + // A draft has no id and no resume handle yet: nothing to resolve, and no I/O + // to spend. Deliberately NOT memoized — the id is assigned later (session.ts + // captureAgentSessionId) and must resolve on the next projection. + if (!session.agentSessionId && !session.resumeSessionPath) return undefined; + const project = this.projects.get(session.projectId); + // cwd is the WORKTREE pi actually ran in (D3), NOT the project root: pi's + // slug follows the spawn cwd, and worktree-bound sessions spawn in the + // worktree. Using project.dto.path here would miss every such session. + const cwd = session.worktreePath ?? project?.dto.path; + const path = this.transcriptResolver({ + agent: session.agent, + agentSessionId: session.agentSessionId, + resumeSessionPath: session.resumeSessionPath, + cwd, + }); + this.transcriptPathMemo.set(session.id, path); + return path; + } + /** Set the loopback bridge + askUser wiring so subsequently-spawned * sessions can transport interactive prompts (`ctx.ui.*` / ACP * permission/elicitation) to the app. */ diff --git a/server/src/protocol.ts b/server/src/protocol.ts index e5c13fce..4cc5b60b 100644 --- a/server/src/protocol.ts +++ b/server/src/protocol.ts @@ -912,6 +912,36 @@ export interface SessionDTO { handoffReason?: string; /** SPEC-46 (D10): which client created this session. Absent means "app" (pre-SPEC-46 rows). */ origin?: SessionOrigin; + /** + * The underlying agent's OWN session id — the native ACP `sessionId` or codex + * `threadId` (SPEC-52 D1). For pi this is pi's own session uuid, because + * `pi-acp` reuses it as the ACP session id, so it is exactly the value pi's + * `/session` prints and `pi --session` accepts. + * + * Optional on the wire, deliberately: a newer app paired with an older server + * must render one fewer row rather than a fabricated one. Same rule as + * `createdAt` (SPEC-47 D12). Undefined for a draft and for a back end with no + * native session concept (`DetachedAdapter`, the stub). + * + * Already persisted through `SessionMeta` (SPEC-29), so a CLOSED session still + * reports it — which is the point: yesterday's session id is still copyable. + */ + agentSessionId?: string; + /** + * Absolute path to this session's transcript on the SERVER's host, or + * undefined when none was resolved (SPEC-52 D3). + * + * Resolved server-side only (D2): the slug algorithm is pi's and lives in + * `pi-sessions.ts`, and the app cannot stat this filesystem to check itself. + * Absolute rather than `~`-relative (D4) because the receiver is another + * agent's shell or prompt, and because the app cannot know this host's home + * directory. Undefined for codex in P1 (D16). + * + * NOTE (D21): this discloses the host's filesystem layout, including its + * username, to any paired device — accepted under the same pairing trust model + * that already carries `worktreePath`, and read-only in that direction. + */ + transcriptPath?: string; } /** diff --git a/server/src/transcript-path.test.ts b/server/src/transcript-path.test.ts new file mode 100644 index 00000000..98e3238b --- /dev/null +++ b/server/src/transcript-path.test.ts @@ -0,0 +1,106 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync, chmodSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { resolveTranscriptPath } from "./transcript-path.js"; +import { piSessionsDir } from "./pi-sessions.js"; + +// Two REAL colliding UUIDv7 ids from this machine (SPEC-52 D15): their first 8 +// chars are identical (`019fa9f4`), so a prefix match would resolve the wrong +// one. Asserted server-side too, not just in the app. +const ID_A = "019fa9f4-443d-7d86-8f4c-d9c4988ddf4f"; +const ID_B = "019fa9f4-d3c8-7e0d-9e34-8c70180ca113"; + +/** Seed a pi transcript for `cwd` named `_.jsonl`. Returns its path. */ +function seed(agentDir: string, cwd: string, id: string): string { + const dir = piSessionsDir(cwd, agentDir); + mkdirSync(dir, { recursive: true }); + const path = join(dir, `2026-01-01T00-00-00-000Z_${id}.jsonl`); + writeFileSync( + path, + JSON.stringify({ type: "session", version: 3, id, timestamp: "2026-01-01T00:00:00.000Z", cwd }) + "\n", + ); + return path; +} + +function withAgentDir(run: (agentDir: string) => void): void { + const agentDir = mkdtempSync(join(tmpdir(), "makit-tp-")); + try { + run(agentDir); + } finally { + rmSync(agentDir, { recursive: true, force: true }); + } +} + +test("resolveTranscriptPath returns the exact-suffix match for a pi session", () => { + withAgentDir((agentDir) => { + const cwd = "/work/proj"; + const path = seed(agentDir, cwd, ID_A); + assert.equal(resolveTranscriptPath({ agent: "pi", agentSessionId: ID_A, cwd }, agentDir), path); + }); +}); + +test("resolveTranscriptPath does not match a different uuid sharing the first 8 chars (D15)", () => { + withAgentDir((agentDir) => { + const cwd = "/work/collide"; + const wanted = seed(agentDir, cwd, ID_A); + seed(agentDir, cwd, ID_B); // same 8-char prefix, different uuid + // Ask for A → must get A's file, never B's. + assert.equal(resolveTranscriptPath({ agent: "pi", agentSessionId: ID_A, cwd }, agentDir), wanted); + // And asking for B returns B, proving both are present and the suffix is exact. + const wantedB = piSessionsDir(cwd, agentDir); + assert.equal( + resolveTranscriptPath({ agent: "pi", agentSessionId: ID_B, cwd }, agentDir), + join(wantedB, `2026-01-01T00-00-00-000Z_${ID_B}.jsonl`), + ); + }); +}); + +test("resolveTranscriptPath returns undefined when the slug dir is absent", () => { + withAgentDir((agentDir) => { + assert.equal(resolveTranscriptPath({ agent: "pi", agentSessionId: ID_A, cwd: "/nope" }, agentDir), undefined); + }); +}); + +test("resolveTranscriptPath returns undefined for an unreadable dir (never throws)", () => { + withAgentDir((agentDir) => { + const cwd = "/work/locked"; + seed(agentDir, cwd, ID_A); + const dir = piSessionsDir(cwd, agentDir); + chmodSync(dir, 0o000); + try { + assert.equal(resolveTranscriptPath({ agent: "pi", agentSessionId: ID_A, cwd }, agentDir), undefined); + } finally { + chmodSync(dir, 0o755); // restore so rmSync can clean up + } + }); +}); + +test("resolveTranscriptPath returns undefined for a non-pi agent even when a file matches (D16)", () => { + withAgentDir((agentDir) => { + const cwd = "/work/codex"; + seed(agentDir, cwd, ID_A); + assert.equal(resolveTranscriptPath({ agent: "codex", agentSessionId: ID_A, cwd }, agentDir), undefined); + }); +}); + +test("resolveTranscriptPath prefers resumeSessionPath over a derivable path", () => { + withAgentDir((agentDir) => { + const cwd = "/work/attached"; + seed(agentDir, cwd, ID_A); // a derivable path exists… + const authoritative = "/some/attached/from/disk.jsonl"; + // …but the on-disk resume handle is authoritative and returned verbatim. + assert.equal( + resolveTranscriptPath({ agent: "pi", agentSessionId: ID_A, resumeSessionPath: authoritative, cwd }, agentDir), + authoritative, + ); + }); +}); + +test("resolveTranscriptPath returns undefined with no agentSessionId", () => { + withAgentDir((agentDir) => { + assert.equal(resolveTranscriptPath({ agent: "pi", cwd: "/work/proj" }, agentDir), undefined); + }); +}); diff --git a/server/src/transcript-path.ts b/server/src/transcript-path.ts new file mode 100644 index 00000000..82859965 --- /dev/null +++ b/server/src/transcript-path.ts @@ -0,0 +1,56 @@ +/** + * transcript-path — resolve a session's on-disk transcript path. + * + * Agent-agnostic dispatcher (SPEC-52 D3). It lives OUTSIDE `pi-sessions.ts` on + * purpose: two of its three branches are not pi-specific, so P2's codex resolver + * (`~/.codex/sessions/YYYY/MM/DD/rollout--.jsonl`) becomes an + * additive branch here rather than forcing a move out of a pi-named module. + * + * Boundary rule (docs/ENGINEERING.md, mirrored from pi-sessions.ts): the pi + * branch reads an untrusted directory on disk. It MUST NEVER throw — a missing + * dir, an unreadable dir, or a stray entry all yield `undefined`. + */ + +import { readdirSync } from "node:fs"; +import { join } from "node:path"; +import { piSessionsDir, realAgentDir } from "./pi-sessions.js"; + +/** Everything the resolver needs from a session, decoupled from `Session`. */ +export interface TranscriptQuery { + /** The session's agent id (`pi`, `codex`, …). Only `pi` derives a path in P1. */ + agent: string; + /** Native agent session/thread id; absent for a draft. */ + agentSessionId?: string; + /** On-disk transcript for a session attached from disk (authoritative). */ + resumeSessionPath?: string; + /** The cwd pi actually ran in — the WORKTREE, usually (see manager wiring). */ + cwd?: string; +} + +const PI_AGENT = "pi"; + +/** + * Resolve the transcript path, or `undefined` (SPEC-52 D3). Order: + * (a) `resumeSessionPath` verbatim — authoritative for disk-attached sessions; + * (b) else, for pi ONLY, the entry in `piSessionsDir(cwd)` whose basename ends + * `_.jsonl` — an EXACT suffix, never a prefix, because pi + * session ids are UUIDv7 and two can share their first 8 chars (D15); + * (c) else `undefined`. + */ +export function resolveTranscriptPath(q: TranscriptQuery, agentDir: string = realAgentDir()): string | undefined { + if (q.resumeSessionPath) return q.resumeSessionPath; + + if (q.agent !== PI_AGENT || !q.agentSessionId || !q.cwd) return undefined; + + const dir = piSessionsDir(q.cwd, agentDir); + // Full `_.jsonl` suffix: the leading `_` and full 36-char id together rule + // out a same-prefix sibling (D15); listing the entry also proves it exists. + const suffix = `_${q.agentSessionId}.jsonl`; + try { + const match = readdirSync(dir).find((entry) => entry.endsWith(suffix)); + return match ? join(dir, match) : undefined; + } catch { + // Missing / unreadable dir → no path. Never throw (boundary rule). + return undefined; + } +}