From 5ce4d03bca6930453dd729ef51c34d553d4bad2d Mon Sep 17 00:00:00 2001 From: Milan Le Date: Tue, 11 Aug 2026 23:02:29 +0200 Subject: [PATCH 01/10] =?UTF-8?q?SPEC-51:=20session=20identity=20=E2=80=94?= =?UTF-8?q?=20spec,=20plan=20and=20design=20board?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copy the underlying agent session id (pi's ACP sessionId / codex threadId) and its on-disk transcript path, mid-turn. Typing pi's /session gets queued behind the running turn; a client command and a menu never do. Spec is rev 2: two independent reviews ran against rev 1 before any code. They caught three things worth having: - D3 was unbuildable. Session has no project path, and pi's transcript slug follows the WORKTREE cwd, not the project root. The nearest precedent (attachPiSession) uses the project path, so copying it would have missed the transcript for every worktree-bound session -- and D9 would have hidden the failure. Resolution moves to SessionManager, keyed on worktreePath ?? project.dto.path, memoized (a readdir per snapshot was unaffordable). - D15: the mockup's 'pi --session 019ff121' was unsafe. pi ids are UUIDv7, whose first 48 bits are a ms timestamp, so 8 chars leave ~65s of ambiguity; real collisions exist on this machine. Review went further and drove it: pi does not error on an ambiguous prefix, it silently picks one and offers to fork it. Full id everywhere. - D4's '~/' abbreviation was unimplementable: the path belongs to the server host and the app cannot know that host's home dir. Cut on review: the identity section inside the context-usage panel (the ring is absent in exactly the four states where the id is wanted), and one of the three panel doors (a tab-menu item duplicating the pane kebab). Added on review: a11y as a locked decision (D18), watch-not-snapshot so a panel opened before the id is assigned fills in live (D19), and the server-path disclosure written down rather than assumed (D21). --- docs/specs/2026-08-11-SPEC-51-PLAN.md | 242 +++++++ .../2026-08-11-SPEC-51-session-identity.md | 186 +++++ mockups/session-identity.html | 684 ++++++++++++++++++ 3 files changed, 1112 insertions(+) create mode 100644 docs/specs/2026-08-11-SPEC-51-PLAN.md create mode 100644 docs/specs/2026-08-11-SPEC-51-session-identity.md create mode 100644 mockups/session-identity.html diff --git a/docs/specs/2026-08-11-SPEC-51-PLAN.md b/docs/specs/2026-08-11-SPEC-51-PLAN.md new file mode 100644 index 00000000..c4857e71 --- /dev/null +++ b/docs/specs/2026-08-11-SPEC-51-PLAN.md @@ -0,0 +1,242 @@ +# SPEC-51 — Plan (rev 2, dual review applied) + +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. | diff --git a/docs/specs/2026-08-11-SPEC-51-session-identity.md b/docs/specs/2026-08-11-SPEC-51-session-identity.md new file mode 100644 index 00000000..1a96b266 --- /dev/null +++ b/docs/specs/2026-08-11-SPEC-51-session-identity.md @@ -0,0 +1,186 @@ +# SPEC-51 — Session identity: copy the id, and its transcript + +**Status:** Draft (rev 2 — dual review applied) · **Priority:** P2 · **Branch:** `feat/get-session-id` +**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 `MenuAnchor` popover on desktop. | Verbatim the `ContextUsageButton` / `ContextUsageDetails` split (`context_usage.dart:199-300`), including the window-clamped width and the `SingleChildScrollView`. | +| **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 + +Filled in on implementation. Required evidence, not claims: + +1. `cd server && node_modules/.bin/tsc -p . --noEmit` clean; `pnpm test` green at **≥ 1313 pass / 0 fail**. +2. `cd app && flutter analyze --fatal-infos --no-pub` → "No issues found"; + `flutter test --no-pub` green against the recorded flake baseline (judge by non-`loading` + failures only). +3. Every new test's bite proven by reverting **only** the production line. +4. **Pixel-perfect 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`. +5. A live probe: a real pi session's `agentSessionId` from the running server, resolved to a + real path, `pi --session ` accepted by the real binary. Deleted afterwards; what it + revealed recorded here. diff --git a/mockups/session-identity.html b/mockups/session-identity.html new file mode 100644 index 00000000..0b28d4ca --- /dev/null +++ b/mockups/session-identity.html @@ -0,0 +1,684 @@ + + + + + +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-51). Three things on this page were + wrong, and all three 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).
    • +
  • 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
+
The id is pi's own. pi-acp reuses pi's session uuid as the ACP + sessionId, so this is the same value /session prints and + pi --session accepts (partial uuid included).
+
+
+ +
+
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…
+
Transcript is on the server's host, not this phone. Copy all gives you the + path; Share sends the file.
+
+
+

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. The panel displays + ~/ because it is narrow; the clipboard carries the real path, because the thing that + receives it is another agent's shell, 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+14Two items in the tab context menu: Copy session id (bare id, zero dialogs — the fastest + door, and the counterweight to “Copy all”) and Session details….
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/.

+
+
+ + + From b132da48397f26d34c0bd82c06b8c07c02665f59 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Tue, 11 Aug 2026 23:32:05 +0200 Subject: [PATCH 02/10] SPEC-51 P1a: the session-identity panel, UI first (no wire yet) Phase A of the plan: build and pixel-verify the whole surface BEFORE freezing the wire contract. The widgets take an app-level SessionIdentity, never a SessionDTO, which is what makes that order possible -- and is the right dependency direction anyway. SessionIdentity per-agent vocabulary as a lookup TABLE, not a switch (ENGINEERING.md's OCP rule), with a safe default that gives an unknown agent a generic label and no resume line rather than an invented CLI. sessionIdentityText() pure; THE copy contract, one place, four callers. SessionIdentityDetails one Copy all (D5), rows omitted when unmeasured (D9), read-only (D8), semantics labels (D18). showSessionIdentity() sheet on mobile, window-clamped popover on desktop. 18 tests, each with a mutation proven to fail it. Two of those mutations initially did NOT bite, and both were my bugs rather than the tests': - the clipboard-failure test asserted on rendered toast text, but the test host has no toast overlay, so findsNothing could never fail. It now asserts against the StatusCenter directly. - two mutation scripts silently no-oped on a Python escaping bug, which looked exactly like a vacuous test until checked. Found by the pixel gate on the real macOS app, not by any test: "1 lines". Reachable in production on a stub/detached session whose only measured value is the makit id. Fixed, and the 1-line case is now asserted in both the visible label and the semantics label. Also fixes a real bug in test/status/status_lifetime_test.dart (SPEC-48 D3's repo-wide guard): it sliced the body out of the ORIGINAL source, so a comment naming the rule it follows supplied the "first await" and turned the correctly-hoisted line below it into an offender -- and a commented-out ref.status counted as a real one. It now scans the blanked copy; offsets are preserved so line numbers still map. Red test added for the false positive, and the guard is proven still to bite by un-hoisting. QA evidence (cua-driver, real macOS window, both themes, 320 and 375pt): both uuid rows render on ONE line -- the measured payoff for dropping the per-row copy column; light-mode Copy all measures 10.95:1 (needs 4.5); line counts 4/3/3/2/1 all correct; draft renders 'Agent not started yet'. Note for anyone repeating that gate: Flutter's macOS AX tree exposes nothing but the menu bar, and synthesized clicks never reached the app (the staleness stamp proved it), so the harness renders every state in one pass and needs no input at all. scroll DID work. Mockup corrected to the shipped type scale (sentence case, as ContextUsageDetails ships) and its in-panel footnotes dropped. --- app/lib/ui/session/session_identity.dart | 456 +++++++++++++++++++++ app/test/session_identity_test.dart | 173 ++++++++ app/test/session_identity_widget_test.dart | 390 ++++++++++++++++++ app/test/status/status_lifetime_test.dart | 45 +- app/tool/session_identity_demo.dart | 213 ++++++++++ mockups/session-identity.html | 20 +- 6 files changed, 1289 insertions(+), 8 deletions(-) create mode 100644 app/lib/ui/session/session_identity.dart create mode 100644 app/test/session_identity_test.dart create mode 100644 app/test/session_identity_widget_test.dart create mode 100644 app/tool/session_identity_demo.dart diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart new file mode 100644 index 00000000..6d1b7223 --- /dev/null +++ b/app/lib/ui/session/session_identity.dart @@ -0,0 +1,456 @@ +// SPEC-51 — 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'; + +// ─── 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 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) { + status.warning( + 'Could not copy session details', + source: StatusSources.session, + detail: '$e', + ); + return; + } + status.info( + 'Session details copied', + source: StatusSources.session, + detail: sessionIdentityLineCountLabel(lines), + ); + } +} + +/// Opens the identity panel: a modal bottom sheet on mobile, an anchored +/// popover on desktop. Same split as `ContextUsageButton` (SPEC-37). +Future showSessionIdentity({ + required BuildContext context, + required SessionIdentity identity, + required bool desktop, +}) { + 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: SessionIdentityDetails(identity: identity), + ), + ), + ); + } + 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. + child: ConstrainedBox( + constraints: BoxConstraints( + maxWidth: math.min( + kIdentityPanelWidth, + window.width - 2 * _kIdentityPanelMargin, + ), + maxHeight: window.height - 2 * _kIdentityPanelMargin, + ), + child: SingleChildScrollView( + child: SessionIdentityDetails(identity: identity), + ), + ), + ), + ); + }, + ); +} diff --git a/app/test/session_identity_test.dart b/app/test/session_identity_test.dart new file mode 100644 index 00000000..d3d76b93 --- /dev/null +++ b/app/test/session_identity_test.dart @@ -0,0 +1,173 @@ +// SPEC-51 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..a75df856 --- /dev/null +++ b/app/test/session_identity_widget_test.dart @@ -0,0 +1,390 @@ +// SPEC-51 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_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')), + ); + expect( + status.events.map((e) => e.title), + contains('Could not copy session details'), + ); + }); + + 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( + '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( + '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..a0994895 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-51'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..1a2e81df --- /dev/null +++ b/app/tool/session_identity_demo.dart @@ -0,0 +1,213 @@ +// Interactive QA harness for the SPEC-51 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/mockups/session-identity.html b/mockups/session-identity.html index 0b28d4ca..6a50d334 100644 --- a/mockups/session-identity.html +++ b/mockups/session-identity.html @@ -108,14 +108,14 @@ .panel{width:300px;background:var(--bg);border:1px solid var(--hairline);border-radius:14px;overflow:hidden} .panel.wide{width:340px} .ph{padding:12px 14px 0} - .ttl{font-size:11px;letter-spacing:.07em;text-transform:uppercase;color:var(--muted);font-weight:700} + .ttl{font-size:11px;letter-spacing:.05em;color:var(--muted);font-weight:600} .sec{border-top:1px solid var(--hairline);padding:10px 14px} .sec.first{border-top:none} /* Stacked row: label above a full-bleed mono value. Removing the 24pt copy column + the 84pt label column returns ~116px to the value, which is what lets a 36-char uuid sit on ONE line at 11.5px mono (≈248px of 312). */ .idrow{padding:5px 0} - .idrow .k{display:block;font-size:10.5px;letter-spacing:.05em;text-transform:uppercase;color:var(--muted);font-weight:600;margin-bottom:2px} + .idrow .k{display:block;font-size:10.5px;color:var(--muted);font-weight:500;margin-bottom:2px} .idrow .v{display:block;font-size:11.5px;word-break:break-all;font-family:ui-monospace,SFMono-Regular,Menlo,monospace;line-height:1.45} .idrow .v.dim{color:var(--muted);font-family:inherit;font-style:italic;word-break:normal} .actrow{display:flex;align-items:center;gap:9px;height:38px;padding:0 14px;font-size:12.5px;color:var(--text);border-top:1px solid var(--hairline)} @@ -233,6 +233,15 @@

Session identity — copy the id, and its transcript

/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).
    • @@ -272,9 +281,7 @@

    One panel, two doors

    recommended
    Copy all4 lines
    Reveal transcript in Finder
    Share transcript…follow-up
    -
    The id is pi's own. pi-acp reuses pi's session uuid as the ACP - sessionId, so this is the same value /session prints and - pi --session accepts (partial uuid included).
    + @@ -394,8 +401,7 @@

    One panel, two doors

    recommended
    Copy all4 lines
    Share transcript…
    -
    Transcript is on the server's host, not this phone. Copy all gives you the - path; Share sends the file.
    +

    Row order is usefulness, not hierarchy: the agent id first From 3c523b3b72607619112e3fb355ffcfff1ffea2c1 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Tue, 11 Aug 2026 23:35:22 +0200 Subject: [PATCH 03/10] =?UTF-8?q?SPEC-51=20P1b:=20freeze=20the=20wire=20co?= =?UTF-8?q?ntract=20=E2=80=94=20agentSessionId=20+=20transcriptPath?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two OPTIONAL fields on SessionDTO, and their app-side decode. Committed on its own, before either implementation, so the parallel server/app work cannot both edit the shared shape. Frozen spelling: `agentSessionId?: string`, `transcriptPath?: string`. Optional is the decision, not an accident: a newer app paired with an older server must render one fewer row rather than a fabricated one, which is the rule createdAt already follows (SPEC-47 D12). App side normalises '' to null at the decode edge. That is not paranoia -- '' is what a sloppy or partially-migrated server sends for "no value", and an empty string here would render a copy affordance that copies nothing, i.e. exactly the placeholder D9 forbids. Non-strings are rejected the same way, so a malformed snapshot degrades one field instead of failing the whole session list. Nothing populates the fields yet -- that is C1b, and its tests are where the bite lives. The vacuous "a DTO built without them is undefined" test from plan rev 1 is deliberately NOT here: optional TS fields are absent by default with zero production code, and JSON.stringify drops undefined. server: tsc clean, 1313 pass / 0 fail (baseline preserved) app: analyze clean, 5 new codec tests green --- app/lib/store/models.dart | 18 ++++++ app/lib/transport/codec.dart | 11 ++++ app/test/session_identity_codec_test.dart | 69 +++++++++++++++++++++++ server/src/protocol.ts | 30 ++++++++++ 4 files changed, 128 insertions(+) create mode 100644 app/test/session_identity_codec_test.dart diff --git a/app/lib/store/models.dart b/app/lib/store/models.dart index ee2903e2..aec52d06 100644 --- a/app/lib/store/models.dart +++ b/app/lib/store/models.dart @@ -1124,6 +1124,8 @@ class Session { this.resumable = false, this.closed = false, this.orphaned = false, + this.agentSessionId, + this.transcriptPath, this.queued = const [], }); @@ -1171,6 +1173,18 @@ class Session { /// repo root (no recreate-worktree path). final bool orphaned; + /// 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-51 (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. @@ -1191,6 +1205,8 @@ class Session { bool? resumable, bool? closed, bool? orphaned, + String? agentSessionId, + String? transcriptPath, List? queued, }) => Session( id: id, @@ -1210,6 +1226,8 @@ class Session { resumable: resumable ?? this.resumable, closed: closed ?? this.closed, orphaned: orphaned ?? this.orphaned, + 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 85853d8e..7268f528 100644 --- a/app/lib/transport/codec.dart +++ b/app/lib/transport/codec.dart @@ -244,6 +244,12 @@ class WireCodec { resumable: j['resumable'] == true, closed: j['closed'] == true, orphaned: j['orphaned'] == true, + // SPEC-51 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']), ), ); @@ -251,6 +257,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/test/session_identity_codec_test.dart b/app/test/session_identity_codec_test.dart new file mode 100644 index 00000000..8b62636f --- /dev/null +++ b/app/test/session_identity_codec_test.dart @@ -0,0 +1,69 @@ +// SPEC-51 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/server/src/protocol.ts b/server/src/protocol.ts index 3196cf2c..3fcf94ab 100644 --- a/server/src/protocol.ts +++ b/server/src/protocol.ts @@ -741,6 +741,36 @@ export interface SessionDTO { * usually still exists, so resume can offer to recreate the worktree. */ orphaned?: boolean; + /** + * The underlying agent's OWN session id — the native ACP `sessionId` or codex + * `threadId` (SPEC-51 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-51 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; } let _seq = 0; From 625319b51779216856f0ed5aa6b49e67a33a9365 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 00:04:48 +0200 Subject: [PATCH 04/10] SPEC-51 P1c: populate the fields (server) and wire the doors (app) Two agents on disjoint trees, then verified here rather than taken on report. server (C1): transcript-path.ts NEW. Agent-agnostic dispatcher: resumeSessionPath verbatim -> pi-only directory scan -> undefined. In its own module because two of its three branches are not pi-specific, so P2's codex resolver is additive rather than a move out of a pi-named file. Never throws. manager.ts projectSessionDTO() populates both fields at the two wire sites (sessions.snapshot, session.listClosed). Session.toDTO() CANNOT do this -- a Session holds projectId + worktreePath, never the project's FS path. cwd = worktreePath ?? project path, because pi's slug follows the cwd pi actually ran in and that is usually the worktree; the nearest precedent (attachPiSession) uses the project path and would have missed every worktree-bound session, with D9 hiding the failure. Memoized per session id including misses: a snapshot is rebroadcast on every metaChanged, so resolving per projection meant a readdir per session per broadcast. app (C2): sessionIdentityProvider store -> SessionIdentity; null FIELDS, never null, never throws, so no call site has to branch. /session a CLIENT command next to /name, so it is intercepted before sendMessage and works MID-TURN. That is the whole bug: pi's own /session is an agent command and gets queued behind the turn. `/session id` copies the bare id; bare opens panel. two doors + tab copy glass menu, pane kebab, and Copy session id on the tab menu (not a third panel door -- cut on review). showSessionIdentity is now exactly-one-of {sessionId, identity}, asserted. The first wiring took a required identity AND an optional sessionId, so all three doors did a ref.read whose result was then discarded: an argument that is ignored depending on another argument is a trap, not an API. Mutations proven to bite (9 across both trees), including the two that matter most: cwd -> project path fails the worktree test, and a prefix-relaxed suffix match fails the D15 collision test. server: tsc clean, 1326 pass / 0 fail (baseline 1313) app: analyze clean, 0 non-loading failures, 55 SPEC-51 tests green --- app/lib/desktop/chat/panes/pane_header.dart | 15 ++ app/lib/desktop/chat/split_view.dart | 39 +++- app/lib/ui/composer/client_commands.dart | 53 +++++ app/lib/ui/session/session_identity.dart | 77 ++++++- app/lib/ui/session/session_screen.dart | 20 ++ app/test/session_command_test.dart | 174 ++++++++++++++ app/test/session_details_doors_test.dart | 225 +++++++++++++++++++ app/test/session_identity_provider_test.dart | 92 ++++++++ app/test/session_identity_widget_test.dart | 32 +++ server/src/manager.test.ts | 140 ++++++++++++ server/src/manager.ts | 57 ++++- server/src/transcript-path.test.ts | 106 +++++++++ server/src/transcript-path.ts | 56 +++++ 13 files changed, 1074 insertions(+), 12 deletions(-) create mode 100644 app/test/session_command_test.dart create mode 100644 app/test/session_details_doors_test.dart create mode 100644 app/test/session_identity_provider_test.dart create mode 100644 server/src/transcript-path.test.ts create mode 100644 server/src/transcript-path.ts 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..6731b2e6 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,9 @@ 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; final selected = await showMenu( context: context, position: RelativeRect.fromRect( @@ -691,8 +698,34 @@ 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 = ref.read(sessionIdentityProvider(sessionId)).agentSessionId; + if (id == null) { + status.warning( + 'No agent session id yet', + source: StatusSources.session, + sessionId: sessionId, + ); + return; + } + await Clipboard.setData(ClipboardData(text: id)); + 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/ui/composer/client_commands.dart b/app/lib/ui/composer/client_commands.dart index 9fe4b03e..e8b71edd 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,57 @@ 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; + } + await Clipboard.setData(ClipboardData(text: id)); + 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/session/session_identity.dart b/app/lib/ui/session/session_identity.dart index 6d1b7223..930da7a4 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -30,6 +30,7 @@ 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) ────────────────────────────────────────────── @@ -79,6 +80,38 @@ const _unknownAgentVocabulary = AgentSessionVocabulary( 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. @@ -401,11 +434,33 @@ class _CopyAllRow extends ConsumerWidget { /// Opens the identity panel: a modal bottom sheet on mobile, an anchored /// popover on desktop. Same split as `ContextUsageButton` (SPEC-37). +/// +/// 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 SessionIdentity identity, 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, @@ -414,9 +469,7 @@ Future showSessionIdentity({ // panel's height depends on how far the transcript path wraps — which on a // 320pt phone is four lines. builder: (_) => SafeArea( - child: SingleChildScrollView( - child: SessionIdentityDetails(identity: identity), - ), + child: SingleChildScrollView(child: body()), ), ); } @@ -445,12 +498,22 @@ Future showSessionIdentity({ ), maxHeight: window.height - 2 * _kIdentityPanelMargin, ), - child: SingleChildScrollView( - child: SessionIdentityDetails(identity: identity), - ), + 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..518e7064 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-51 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/session_command_test.dart b/app/test/session_command_test.dart new file mode 100644 index 00000000..327e6846 --- /dev/null +++ b/app/test/session_command_test.dart @@ -0,0 +1,174 @@ +// SPEC-51 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_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 = []; + + void install(WidgetTester tester) { + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'Clipboard.setData') { + 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('/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..4a8e827b --- /dev/null +++ b/app/test/session_details_doors_test.dart @@ -0,0 +1,225 @@ +// SPEC-51 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/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 = []; + void install(WidgetTester tester) { + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'Clipboard.setData') { + 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) async { + final c = ProviderContainer( + overrides: [ + sessionsProvider.overrideWithValue( + SessionsState([_session('s1', 'First'), _session('s2', 'Second')]), + ), + reposProvider.overrideWithValue(ReposState(const [])), + eventsProvider.overrideWithValue(EventsState(const {}, const {})), + ], + ); + 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); + }); + }); +} diff --git a/app/test/session_identity_provider_test.dart b/app/test/session_identity_provider_test.dart new file mode 100644 index 00000000..45d64383 --- /dev/null +++ b/app/test/session_identity_provider_test.dart @@ -0,0 +1,92 @@ +// SPEC-51 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_widget_test.dart b/app/test/session_identity_widget_test.dart index a75df856..745d2c67 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -357,6 +357,38 @@ void main() { }, ); + 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 { diff --git a/server/src/manager.test.ts b/server/src/manager.test.ts index 148efd95..3df540cf 100644 --- a/server/src/manager.test.ts +++ b/server/src/manager.test.ts @@ -2687,3 +2687,143 @@ test("reopenSession on an already-open session is a no-op", async () => { store.close(); } }); + +// --- SPEC-51 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-51 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-51)", () => { + 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-51)", () => { + 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-51 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 c4eb86fc..9395be3a 100644 --- a/server/src/manager.ts +++ b/server/src/manager.ts @@ -17,6 +17,7 @@ import { CapabilityCache } from "./adapters/capability_cache.js"; import { Session } from "./session.js"; import { DEFAULT_SESSION_TITLE, type ProjectDTO, type RepoDTO, type SessionConfigOption, type SessionDTO } from "./protocol.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"; @@ -103,6 +104,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-51 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). @@ -201,6 +208,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-51 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; @@ -214,6 +230,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"; @@ -332,7 +349,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. @@ -355,7 +372,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); } @@ -386,6 +403,42 @@ export class SessionManager extends EventEmitter { return this.sessions.get(id); } + /** + * Project a session for the wire (SPEC-51 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-51 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/transcript-path.test.ts b/server/src/transcript-path.test.ts new file mode 100644 index 00000000..d79bc0dd --- /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-51 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..e2411eb3 --- /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-51 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-51 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; + } +} From 32111a5835b37ea14b58034ef452d6caf9825f26 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 00:20:00 +0200 Subject: [PATCH 05/10] docs(SPEC-51): record the verification evidence and flip the status Phase D step 5. The Verification section said "filled in on implementation" and the status still said Draft; both are now true statements about the shipped code. The one gate that had no recorded result was the live probe, so it was run: resolveTranscriptPath against the real ~/.pi/agent/sessions tree, for this branch's own worktree, using the pi session id of the session that implemented the feature. It returned the transcript path byte-identical to the one pi itself reports. The same call with the 8-char prefix returned undefined -- D15 holds on real data, not just in the unit test's fixture dir. `pi --help` confirms the resume line's spelling is `--session `. Also noted the tenth mutation: relaxing handleClientCommand's exact name match to startsWith, which `/sessions` is NOT intercepted catches. That mutation was left in the working tree by the previous session and has been reverted -- exact matching is the shipped behaviour. Status is Implemented for P1 only; the codex resolver branch and D12's section inside the usage panel stay deferred, so the header says so rather than implying the whole spec landed. --- .../2026-08-11-SPEC-51-session-identity.md | 47 +++++++++++++------ 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/docs/specs/2026-08-11-SPEC-51-session-identity.md b/docs/specs/2026-08-11-SPEC-51-session-identity.md index 1a96b266..8cf97f6c 100644 --- a/docs/specs/2026-08-11-SPEC-51-session-identity.md +++ b/docs/specs/2026-08-11-SPEC-51-session-identity.md @@ -1,6 +1,9 @@ # SPEC-51 — Session identity: copy the id, and its transcript -**Status:** Draft (rev 2 — dual review applied) · **Priority:** P2 · **Branch:** `feat/get-session-id` +**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). **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 @@ -170,17 +173,33 @@ fail** · `flutter analyze --fatal-infos --no-pub` "No issues found". ## Verification -Filled in on implementation. Required evidence, not claims: - -1. `cd server && node_modules/.bin/tsc -p . --noEmit` clean; `pnpm test` green at **≥ 1313 pass / 0 fail**. -2. `cd app && flutter analyze --fatal-infos --no-pub` → "No issues found"; - `flutter test --no-pub` green against the recorded flake baseline (judge by non-`loading` - failures only). -3. Every new test's bite proven by reverting **only** the production line. -4. **Pixel-perfect 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 +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-51 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`. -5. A live probe: a real pi session's `agentSessionId` from the running server, resolved to a - real path, `pi --session ` accepted by the real binary. Deleted afterwards; what it - revealed recorded here. + 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. ✅ From 2a95bf7ff42e69ca003bc33f8a4fc00a08fafd8e Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 00:21:07 +0200 Subject: [PATCH 06/10] style(app): apply dart format to the SPEC-51 files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Whitespace and trailing commas only — verified by comparing both revisions with all whitespace and commas stripped, so no token changed. `dart format --set-exit-if-changed` is now clean across lib/test/tool, analyze reports no issues, and the 52 SPEC-51 tests in these files stay green. The pre-push hook writes these fixes itself and then fails the push, so they have to land as a commit rather than as a dirty tree. --- app/lib/ui/session/session_identity.dart | 20 +++++------- app/test/session_command_test.dart | 13 ++++---- app/test/session_details_doors_test.dart | 36 ++++++++++++---------- app/test/session_identity_test.dart | 4 +-- app/test/session_identity_widget_test.dart | 22 ++++++------- 5 files changed, 45 insertions(+), 50 deletions(-) diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart index 930da7a4..d0fe65a3 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -183,8 +183,7 @@ class SessionIdentity { /// whose back end has no native session concept. bool get hasAgentSession => agentSessionId != null; - static String? _blankToNull(String? v) => - (v == null || v.isEmpty) ? null : v; + static String? _blankToNull(String? v) => (v == null || v.isEmpty) ? null : v; } // ─── the copy contract (D14) ───────────────────────────────────────────────── @@ -207,12 +206,8 @@ String sessionIdentityText(SessionIdentity identity) { 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'); + 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 @@ -468,9 +463,7 @@ Future showSessionIdentity({ // 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()), - ), + builder: (_) => SafeArea(child: SingleChildScrollView(child: body())), ); } return showDialog( @@ -514,6 +507,7 @@ class _WatchedIdentity extends ConsumerWidget { final String sessionId; @override - Widget build(BuildContext context, WidgetRef ref) => - SessionIdentityDetails(identity: ref.watch(sessionIdentityProvider(sessionId))); + Widget build(BuildContext context, WidgetRef ref) => SessionIdentityDetails( + identity: ref.watch(sessionIdentityProvider(sessionId)), + ); } diff --git a/app/test/session_command_test.dart b/app/test/session_command_test.dart index 327e6846..d3d124e4 100644 --- a/app/test/session_command_test.dart +++ b/app/test/session_command_test.dart @@ -115,7 +115,9 @@ void main() { expect(await r.handled, isTrue); }); - testWidgets('bare /session opens the panel and does NOT copy', (tester) async { + 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()); @@ -139,10 +141,7 @@ void main() { ); 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'), - ); + expect(status.events.map((e) => e.title), contains('Session id copied')); }); testWidgets('/session id with no agent id copies nothing and says why', ( @@ -165,7 +164,9 @@ void main() { ); }); - testWidgets('/sessions (a different word) is NOT intercepted', (tester) async { + 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()); diff --git a/app/test/session_details_doors_test.dart b/app/test/session_details_doors_test.dart index 4a8e827b..4f8cb62d 100644 --- a/app/test/session_details_doors_test.dart +++ b/app/test/session_details_doors_test.dart @@ -37,20 +37,23 @@ class _EmptyStorage implements SecureStore { 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, - ); +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 = []; @@ -189,8 +192,9 @@ void main() { return c; } - Finder chip(String label) => - find.ancestor(of: find.text(label), matching: find.byType(Container)).first; + 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, diff --git a/app/test/session_identity_test.dart b/app/test/session_identity_test.dart index d3d76b93..26b08301 100644 --- a/app/test/session_identity_test.dart +++ b/app/test/session_identity_test.dart @@ -106,9 +106,7 @@ void main() { // 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(); + final valueStarts = lines.map((l) => l.indexOf(': ') + 2).toSet(); expect( valueStarts, hasLength(1), diff --git a/app/test/session_identity_widget_test.dart b/app/test/session_identity_widget_test.dart index 745d2c67..6b76f9a2 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -57,7 +57,10 @@ Widget _host(Widget child, {StatusCenter? status}) => ProviderScope( overrides: [ if (status != null) statusCenterProvider.overrideWithValue(status), ], - child: MaterialApp(theme: makitDarkTheme, home: Scaffold(body: child)), + child: MaterialApp( + theme: makitDarkTheme, + home: Scaffold(body: child), + ), ); void main() { @@ -210,7 +213,8 @@ void main() { expect( destructive.hasMatch(data), isFalse, - reason: 'lifecycle lives in the menu, not one mis-tap from a copy row', + reason: + 'lifecycle lives in the menu, not one mis-tap from a copy row', ); } }); @@ -238,10 +242,7 @@ void main() { await tester.pumpWidget( _host(SessionIdentityDetails(identity: identity())), ); - expect( - find.bySemanticsLabel(RegExp('pi session.*$kId')), - findsOneWidget, - ); + expect(find.bySemanticsLabel(RegExp('pi session.*$kId')), findsOneWidget); handle.dispose(); }); @@ -366,10 +367,8 @@ void main() { await tester.pumpWidget( _host( Builder( - builder: (context) => ElevatedButton( - onPressed: () {}, - child: const Text('x'), - ), + builder: (context) => + ElevatedButton(onPressed: () {}, child: const Text('x')), ), ), ); @@ -403,8 +402,7 @@ void main() { _host( ValueListenableBuilder( valueListenable: notifier, - builder: (_, value, _) => - SessionIdentityDetails(identity: value), + builder: (_, value, _) => SessionIdentityDetails(identity: value), ), ), ); From 8809cfc98c85536719e750d4c1dcb72fe2ed74ff Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 00:52:10 +0200 Subject: [PATCH 07/10] fix(app): report a failed id copy, and survive the tab closing under the menu Three review findings, each with a red test written first. 1. HIGH -- `/session id` awaited Clipboard.setData with no try/catch, so a PlatformException (another process holds the clipboard on Windows; the host denies the write) escaped the handler: the user got neither the id nor a word about why. The panel's `Copy all` already had this guard; the two bare-id paths did not. The review flagged client_commands.dart. The IDENTICAL bug sat in split_view.dart's tab-menu copy, unflagged, so both are fixed -- three copy paths in one feature must not disagree about what a failed write does. All three now report `failure`, not `warning`: an action the user asked for did not happen, which is what status.failure is for. The panel's `warning` was the odd one out and moved too. Severity is now ASSERTED in all three tests, so the consistency is enforced rather than incidental -- flipping any one back to warning fails its test (proven). 2. HIGH -- the tab menu's ref.read(sessionIdentityProvider) ran AFTER the showMenu await. That menu lives in the Navigator's overlay, so it outlives the _TabChip that opened it: close the tab while the menu is open (a server snapshot dropping the session does it for real) and the read hits a dead ref -- `Bad state: Using "ref" when a widget is about to or has been unmounted is unsafe.` A crash instead of a copy. The new test reproduces it exactly that way, by calling unbindSession while the menu is up, and it failed with that message before the fix. Hoisted next to `status`, which is already hoisted for this very reason (SPEC-48 D3). Chose that over the reviewer's alternative of a context.mounted guard: the guard keeps the read fresh but still leaves `ref` use after an await, which is the hazard D3 exists to remove. The cost is that the id is sampled at menu-open rather than at click -- sub-second for right-click -> click, and the surface that must fill in live is the panel, which watches (D19). Measured rather than assumed: broadening status_lifetime_test.dart to catch ref.read/ref.watch after an await repo-wide -- which would have caught this automatically -- reports 24 bare sites across lib/. Out of scope here, so it is named as a follow-up instead of pretending this PR fixed a repo-wide pattern. 3. LOW -- three stale statements in mockups/session-identity.html, each of which misleads someone reading it as the design source: - "Three things on this page were wrong" above a list of six; - prose still claiming the panel DISPLAYS `~/` paths, contradicting both the rendered frames and the amendment 20 lines above it (D4 dropped `~/`); - the delta table still promising `Session details...` in the tab menu -- the fourth door D13 explicitly cut. That is the dangerous one: a delta table reads as a work list. Markup re-validated (balanced tags) after the edits. --- app/lib/desktop/chat/split_view.dart | 31 +++++++++- app/lib/ui/composer/client_commands.dart | 16 ++++- app/lib/ui/session/session_identity.dart | 8 ++- app/test/session_command_test.dart | 51 ++++++++++++++++ app/test/session_details_doors_test.dart | 71 +++++++++++++++++++++- app/test/session_identity_widget_test.dart | 10 ++- mockups/session-identity.html | 18 +++--- 7 files changed, 190 insertions(+), 15 deletions(-) diff --git a/app/lib/desktop/chat/split_view.dart b/app/lib/desktop/chat/split_view.dart index 6731b2e6..ab1599d4 100644 --- a/app/lib/desktop/chat/split_view.dart +++ b/app/lib/desktop/chat/split_view.dart @@ -685,6 +685,20 @@ class _TabChip extends ConsumerWidget { // 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( @@ -708,7 +722,7 @@ class _TabChip extends ConsumerWidget { 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 = ref.read(sessionIdentityProvider(sessionId)).agentSessionId; + final id = identity.agentSessionId; if (id == null) { status.warning( 'No agent session id yet', @@ -717,7 +731,20 @@ class _TabChip extends ConsumerWidget { ); return; } - await Clipboard.setData(ClipboardData(text: id)); + // 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, diff --git a/app/lib/ui/composer/client_commands.dart b/app/lib/ui/composer/client_commands.dart index e8b71edd..e0143ea6 100644 --- a/app/lib/ui/composer/client_commands.dart +++ b/app/lib/ui/composer/client_commands.dart @@ -295,7 +295,21 @@ final List clientCommands = [ ); return; } - await Clipboard.setData(ClipboardData(text: id)); + // 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, diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart index d0fe65a3..bf2b3e4c 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -412,10 +412,14 @@ class _CopyAllRow extends ConsumerWidget { try { await Clipboard.setData(ClipboardData(text: payload)); } catch (e) { - status.warning( + // `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, - detail: '$e', ); return; } diff --git a/app/test/session_command_test.dart b/app/test/session_command_test.dart index d3d124e4..088e7e27 100644 --- a/app/test/session_command_test.dart +++ b/app/test/session_command_test.dart @@ -16,6 +16,7 @@ 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'; @@ -38,11 +39,18 @@ Session _session({String? agentSessionId = kAgentId}) => Session( 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; @@ -164,6 +172,49 @@ void main() { ); }); + 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 { diff --git a/app/test/session_details_doors_test.dart b/app/test/session_details_doors_test.dart index 4f8cb62d..c2f5a2a6 100644 --- a/app/test/session_details_doors_test.dart +++ b/app/test/session_details_doors_test.dart @@ -18,6 +18,9 @@ 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'; @@ -57,11 +60,19 @@ Session _session( 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; @@ -168,7 +179,10 @@ void main() { resetNodeIds(); }); - Future pumpTwoTabs(WidgetTester tester) async { + Future pumpTwoTabs( + WidgetTester tester, { + StatusCenter? status, + }) async { final c = ProviderContainer( overrides: [ sessionsProvider.overrideWithValue( @@ -176,6 +190,7 @@ void main() { ), reposProvider.overrideWithValue(ReposState(const [])), eventsProvider.overrideWithValue(EventsState(const {}, const {})), + if (status != null) statusCenterProvider.overrideWithValue(status), ], ); addTearDown(c.dispose); @@ -225,5 +240,59 @@ void main() { 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_widget_test.dart b/app/test/session_identity_widget_test.dart index 6b76f9a2..58ca3db7 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -10,6 +10,7 @@ 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'; @@ -173,9 +174,14 @@ void main() { 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( - status.events.map((e) => e.title), - contains('Could not copy session details'), + failed.single.severity, + StatusSeverity.failure, + reason: 'all three copy paths report a failed write the same way', ); }); diff --git a/mockups/session-identity.html b/mockups/session-identity.html index 6a50d334..bb3f52d2 100644 --- a/mockups/session-identity.html +++ b/mockups/session-identity.html @@ -218,8 +218,8 @@

    Session identity — copy the id, and its transcript

    - ✎ Amended in planning, before a line was written (SPEC-51). Three things on this page were - wrong, and all three are corrected above — the frames now show what will ship. + ✎ Amended in planning, before a line was written (SPEC-51). 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 @@ -416,9 +416,11 @@

      One panel, two doors

      recommended 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. The panel displays - ~/ because it is narrow; the clipboard carries the real path, because the thing that - receives it is another agent's shell, and ~ only expands if a shell gets there first. +

    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 @@ -665,8 +667,10 @@

    One panel, two doors

    recommended /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+14 - Two items in the tab context menu: Copy session id (bare id, zero dialogs — the fastest - door, and the counterweight to “Copy all”) and Session details…. + Copy 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+6 Session details in the kebab, above the rule. 9app/lib/ui/session/session_screen.dart+6 From 1a0bb8a189d0fa9ef33de0879c0b8aaa251484a2 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 00:59:30 +0200 Subject: [PATCH 08/10] docs: renumber this spec 51 -> 52, because 51 and 50 both shipped first Not a cosmetic rename. While this branch was in flight, `main` took BOTH numbers next to it: SPEC-51 Preview groups (#163) -- docs/UX.md + 14 shipped source files SPEC-50 Profiles (#162) -- a whole server-profiles feature So `SPEC-51` in this branch's code comments pointed at a different, already-shipped feature, and `docs/specs/` held two SPEC-51 files. Neither shipped spec can move, so this one takes 52, the next free number. Renumbered ONLY this feature's own files, derived from this branch's commits (1b928749..2a95bf7f plus the fix commit) rather than by a blanket search-replace: a global `SPEC-51 -> SPEC-52` would have rewritten preview groups' own references in 14 files and docs/UX.md, silently pointing them at this spec instead. Each of the remaining 14 SPEC-51 references was checked to be preview groups'. The branch name (feat/get-session-id) is left alone deliberately -- renaming a pushed branch orphans its PR for no gain. Recorded in the spec header, including the part a rename does not fix: nothing in this repo allocates spec numbers, so two branches drafted in one week collide in silence. `main` already carries that proof -- SPEC-48 names both 2026-08-09-SPEC-48-status-and-activity.md and 2026-08-10-SPEC-48-per-repo-settings.md. Verified after the renumber, on the merge with the NEWER main: server: tsc clean, 2072 pass / 0 fail app: analyze clean, dart format clean, 0 non-loading failures --- app/lib/store/models.dart | 2 +- app/lib/transport/codec.dart | 2 +- app/lib/ui/session/session_identity.dart | 2 +- app/lib/ui/session/session_screen.dart | 2 +- app/test/session_command_test.dart | 2 +- app/test/session_details_doors_test.dart | 2 +- app/test/session_identity_codec_test.dart | 2 +- app/test/session_identity_provider_test.dart | 2 +- app/test/session_identity_test.dart | 2 +- app/test/session_identity_widget_test.dart | 2 +- app/test/status/status_lifetime_test.dart | 2 +- app/tool/session_identity_demo.dart | 2 +- ...EC-51-PLAN.md => 2026-08-11-SPEC-52-PLAN.md} | 5 ++++- ...d => 2026-08-11-SPEC-52-session-identity.md} | 17 +++++++++++++++-- mockups/session-identity.html | 2 +- server/src/manager.test.ts | 10 +++++----- server/src/manager.ts | 8 ++++---- server/src/protocol.ts | 4 ++-- server/src/transcript-path.test.ts | 2 +- server/src/transcript-path.ts | 4 ++-- 20 files changed, 46 insertions(+), 30 deletions(-) rename docs/specs/{2026-08-11-SPEC-51-PLAN.md => 2026-08-11-SPEC-52-PLAN.md} (99%) rename docs/specs/{2026-08-11-SPEC-51-session-identity.md => 2026-08-11-SPEC-52-session-identity.md} (95%) diff --git a/app/lib/store/models.dart b/app/lib/store/models.dart index e835f5d7..995f038b 100644 --- a/app/lib/store/models.dart +++ b/app/lib/store/models.dart @@ -1301,7 +1301,7 @@ class Session { /// 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-51 (D1). + /// older than SPEC-52 (D1). final String? agentSessionId; /// Absolute path to the transcript on the SERVER's host, resolved server-side diff --git a/app/lib/transport/codec.dart b/app/lib/transport/codec.dart index bc46f0e6..37afa002 100644 --- a/app/lib/transport/codec.dart +++ b/app/lib/transport/codec.dart @@ -274,7 +274,7 @@ class WireCodec { ? j['handoffReason'] as String : null, origin: j['origin'] is String ? j['origin'] as String : null, - // SPEC-51 D1/D9: normalise `''` to null at the edge. A blank string is + // 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. diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart index bf2b3e4c..b57e08fa 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -1,4 +1,4 @@ -// SPEC-51 — session identity: the underlying agent session id, its transcript +// 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 diff --git a/app/lib/ui/session/session_screen.dart b/app/lib/ui/session/session_screen.dart index 518e7064..d35a0d0a 100644 --- a/app/lib/ui/session/session_screen.dart +++ b/app/lib/ui/session/session_screen.dart @@ -591,7 +591,7 @@ class _SessionScreenState extends ConsumerState { icon: PhosphorIconsLight.listMagnifyingGlass, label: 'My messages', ), - // SPEC-51 D13: the identity panel's mobile door. Grouped with + // 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. diff --git a/app/test/session_command_test.dart b/app/test/session_command_test.dart index 088e7e27..cb766cdd 100644 --- a/app/test/session_command_test.dart +++ b/app/test/session_command_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 C2b — the `/session` CLIENT command (D7). +// 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 diff --git a/app/test/session_details_doors_test.dart b/app/test/session_details_doors_test.dart index c2f5a2a6..43bf3322 100644 --- a/app/test/session_details_doors_test.dart +++ b/app/test/session_details_doors_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 C2c — the two panel DOORS (D13) and the tab menu's Copy session id +// 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. diff --git a/app/test/session_identity_codec_test.dart b/app/test/session_identity_codec_test.dart index 8b62636f..f42e97e0 100644 --- a/app/test/session_identity_codec_test.dart +++ b/app/test/session_identity_codec_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 B1 — the wire contract for session identity. +// 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 diff --git a/app/test/session_identity_provider_test.dart b/app/test/session_identity_provider_test.dart index 45d64383..cac3f822 100644 --- a/app/test/session_identity_provider_test.dart +++ b/app/test/session_identity_provider_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 C2a — `sessionIdentityProvider(sessionId)`: the seam that maps the +// 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 diff --git a/app/test/session_identity_test.dart b/app/test/session_identity_test.dart index 26b08301..f1582b41 100644 --- a/app/test/session_identity_test.dart +++ b/app/test/session_identity_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 A1 + A2 — the pure half of session identity: the per-agent vocabulary +// 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 diff --git a/app/test/session_identity_widget_test.dart b/app/test/session_identity_widget_test.dart index 58ca3db7..0154052f 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -1,4 +1,4 @@ -// SPEC-51 A3 + A4 — the identity panel: its rows, its ONE copy affordance, its +// 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 diff --git a/app/test/status/status_lifetime_test.dart b/app/test/status/status_lifetime_test.dart index a0994895..1fd5fa9e 100644 --- a/app/test/status/status_lifetime_test.dart +++ b/app/test/status/status_lifetime_test.dart @@ -47,7 +47,7 @@ void main() { }); test('a comment mentioning await does not create a false positive', () { - // Found while adding SPEC-51's identity panel, whose hoist comment + // 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 diff --git a/app/tool/session_identity_demo.dart b/app/tool/session_identity_demo.dart index 1a2e81df..3f96f8f4 100644 --- a/app/tool/session_identity_demo.dart +++ b/app/tool/session_identity_demo.dart @@ -1,4 +1,4 @@ -// Interactive QA harness for the SPEC-51 session-identity panel. Seeded data, +// 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 diff --git a/docs/specs/2026-08-11-SPEC-51-PLAN.md b/docs/specs/2026-08-11-SPEC-52-PLAN.md similarity index 99% rename from docs/specs/2026-08-11-SPEC-51-PLAN.md rename to docs/specs/2026-08-11-SPEC-52-PLAN.md index c4857e71..ef6a6d07 100644 --- a/docs/specs/2026-08-11-SPEC-51-PLAN.md +++ b/docs/specs/2026-08-11-SPEC-52-PLAN.md @@ -1,4 +1,7 @@ -# SPEC-51 — Plan (rev 2, dual review applied) +# 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 diff --git a/docs/specs/2026-08-11-SPEC-51-session-identity.md b/docs/specs/2026-08-11-SPEC-52-session-identity.md similarity index 95% rename from docs/specs/2026-08-11-SPEC-51-session-identity.md rename to docs/specs/2026-08-11-SPEC-52-session-identity.md index 8cf97f6c..40cdd48d 100644 --- a/docs/specs/2026-08-11-SPEC-51-session-identity.md +++ b/docs/specs/2026-08-11-SPEC-52-session-identity.md @@ -1,9 +1,22 @@ -# SPEC-51 — Session identity: copy the id, and its transcript +# 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 @@ -181,7 +194,7 @@ Required evidence, not claims. Recorded as measured: `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-51 tests are green run as a set. ✅ + 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 diff --git a/mockups/session-identity.html b/mockups/session-identity.html index bb3f52d2..3623f590 100644 --- a/mockups/session-identity.html +++ b/mockups/session-identity.html @@ -218,7 +218,7 @@

    Session identity — copy the id, and its transcript

    - ✎ Amended in planning, before a line was written (SPEC-51). Six things on this page were + ✎ 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 diff --git a/server/src/manager.test.ts b/server/src/manager.test.ts index 35cd9181..611f0dc0 100644 --- a/server/src/manager.test.ts +++ b/server/src/manager.test.ts @@ -2878,7 +2878,7 @@ test("reopenSession on an already-open session is a no-op", async () => { } }); -// --- SPEC-51 C1b: agentSessionId + transcriptPath in the projected DTO --------- +// --- 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"; @@ -2909,7 +2909,7 @@ function withSpec51AgentDir(body: (agentDir: string) => void): void { } } -test("listSessions resolves transcriptPath from the WORKTREE slug, not the project slug (SPEC-51 D3)", () => { +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"; @@ -2942,7 +2942,7 @@ test("listSessions resolves transcriptPath from the WORKTREE slug, not the proje }); }); -test("listSessions projects transcriptPath into the DTO (SPEC-51)", () => { +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. @@ -2956,7 +2956,7 @@ test("listSessions projects transcriptPath into the DTO (SPEC-51)", () => { } }); -test("listSessions projects agentSessionId into the DTO (SPEC-51)", () => { +test("listSessions projects agentSessionId into the DTO (SPEC-52)", () => { const store = new SqliteEventStore(); seedColdSession(store, "sess-id", { agentSessionId: "pi-42" }); try { @@ -2968,7 +2968,7 @@ test("listSessions projects agentSessionId into the DTO (SPEC-51)", () => { } }); -test("a draft projects neither agentSessionId nor transcriptPath (SPEC-51 D9)", async () => { +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([]) }); diff --git a/server/src/manager.ts b/server/src/manager.ts index d9800185..9378f15c 100644 --- a/server/src/manager.ts +++ b/server/src/manager.ts @@ -128,7 +128,7 @@ export interface ManagerOpts { /** Override the production pi adapter, used by deterministic e2e tests. */ adapterFactory?: AdapterFactory; /** - * Resolve a session's transcript path (SPEC-51 D3). Injected so tests can + * 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. */ @@ -255,7 +255,7 @@ export class SessionManager extends EventEmitter { private readonly adapterFactory?: AdapterFactory; private readonly transcriptResolver: (q: TranscriptQuery) => string | undefined; /** - * Per-session-id transcript path cache, INCLUDING misses (SPEC-51 D3). A + * 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 @@ -641,7 +641,7 @@ export class SessionManager extends EventEmitter { } /** - * Project a session for the wire (SPEC-51 D1). `Session.toDTO()` cannot do + * 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`. @@ -654,7 +654,7 @@ export class SessionManager extends EventEmitter { }; } - /** Memoized transcript-path lookup (SPEC-51 D3); see {@link transcriptPathMemo}. */ + /** 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 diff --git a/server/src/protocol.ts b/server/src/protocol.ts index ee62892c..4cc5b60b 100644 --- a/server/src/protocol.ts +++ b/server/src/protocol.ts @@ -914,7 +914,7 @@ export interface SessionDTO { origin?: SessionOrigin; /** * The underlying agent's OWN session id — the native ACP `sessionId` or codex - * `threadId` (SPEC-51 D1). For pi this is pi's own session uuid, because + * `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. * @@ -929,7 +929,7 @@ export interface SessionDTO { agentSessionId?: string; /** * Absolute path to this session's transcript on the SERVER's host, or - * undefined when none was resolved (SPEC-51 D3). + * 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. diff --git a/server/src/transcript-path.test.ts b/server/src/transcript-path.test.ts index d79bc0dd..98e3238b 100644 --- a/server/src/transcript-path.test.ts +++ b/server/src/transcript-path.test.ts @@ -7,7 +7,7 @@ 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-51 D15): their first 8 +// 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"; diff --git a/server/src/transcript-path.ts b/server/src/transcript-path.ts index e2411eb3..82859965 100644 --- a/server/src/transcript-path.ts +++ b/server/src/transcript-path.ts @@ -1,7 +1,7 @@ /** * transcript-path — resolve a session's on-disk transcript path. * - * Agent-agnostic dispatcher (SPEC-51 D3). It lives OUTSIDE `pi-sessions.ts` on + * 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. @@ -30,7 +30,7 @@ export interface TranscriptQuery { const PI_AGENT = "pi"; /** - * Resolve the transcript path, or `undefined` (SPEC-51 D3). Order: + * 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 From 6914b5c36f78b9cfd14cb3dc6521e4bde3e5d23b Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 01:06:30 +0200 Subject: [PATCH 09/10] =?UTF-8?q?fix(app):=20the=20desktop=20panel=20is=20?= =?UTF-8?q?centred=20on=20purpose=20=E2=80=94=20say=20so,=20and=20test=20i?= =?UTF-8?q?t?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth review thread. The doc comment promised "an anchored popover on desktop. Same split as ContextUsageButton (SPEC-37)" and D11 specified a MenuAnchor; the code ships showDialog + Alignment.center. So this was not a stale comment, it was an unrecorded spec deviation. Kept the centring, and the reason is a real finding rather than a shrug: the mechanism cannot transfer, because the DOOR TOPOLOGY differs. ContextUsageButton is a persistent control in the composer, so a MenuAnchor has something to stay anchored to for as long as the popover is open. Both of this panel's desktop doors are transient MENU ITEMS -- the pane-header kebab and the mobile glass menu -- so by the time an item is chosen its menu has been dismissed. Anchoring to where a vanished menu item used to be is arbitrary placement dressed up as precision. What SPEC-37 actually contributes is kept: the window-clamped width and the SingleChildScrollView, so a panel opened from a narrow split pane cannot hang off-screen. The real defect underneath: the desktop host had NO test. Every case in session_identity_widget_test.dart passed `desktop: false`, which is exactly how the code and its own doc comment drifted apart and stayed that way through implementation. Two tests added: * desktop opens a centred, window-clamped panel -- not a sheet * a desktop panel in a narrow window is clamped to the window (300pt window, 340pt panel, so the clamp is load-bearing) Both mutation-proven: Alignment.center -> topLeft fails the first, and dropping the math.min window clamp fails the second. D11 amended in the spec and logged as deviation 8, so the next reader does not find a third version of the truth. --- app/lib/ui/session/session_identity.dart | 21 +++++- app/test/session_identity_widget_test.dart | 71 +++++++++++++++++++ docs/specs/2026-08-11-SPEC-52-PLAN.md | 1 + .../2026-08-11-SPEC-52-session-identity.md | 2 +- 4 files changed, 92 insertions(+), 3 deletions(-) diff --git a/app/lib/ui/session/session_identity.dart b/app/lib/ui/session/session_identity.dart index b57e08fa..5631c091 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -431,8 +431,25 @@ class _CopyAllRow extends ConsumerWidget { } } -/// Opens the identity panel: a modal bottom sheet on mobile, an anchored -/// popover on desktop. Same split as `ContextUsageButton` (SPEC-37). +/// 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]: /// diff --git a/app/test/session_identity_widget_test.dart b/app/test/session_identity_widget_test.dart index 0154052f..175ffcf3 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -313,6 +313,77 @@ void main() { 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( 'at 320x360 nothing paints off-screen and Copy all is still reachable', (tester) async { diff --git a/docs/specs/2026-08-11-SPEC-52-PLAN.md b/docs/specs/2026-08-11-SPEC-52-PLAN.md index ef6a6d07..732d9200 100644 --- a/docs/specs/2026-08-11-SPEC-52-PLAN.md +++ b/docs/specs/2026-08-11-SPEC-52-PLAN.md @@ -243,3 +243,4 @@ Per `makit-transcript-row-qa-harness`: | 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 index 40cdd48d..b30e20ee 100644 --- a/docs/specs/2026-08-11-SPEC-52-session-identity.md +++ b/docs/specs/2026-08-11-SPEC-52-session-identity.md @@ -107,7 +107,7 @@ them. **The absence rule is not weakened** (D12) — the panel is reachable with | **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 `MenuAnchor` popover on desktop. | Verbatim the `ContextUsageButton` / `ContextUsageDetails` split (`context_usage.dart:199-300`), including the window-clamped width and the `SingleChildScrollView`. | +| **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. | From 8873c6aaa73c7c44901b833e101a9f9590f57633 Mon Sep 17 00:00:00 2001 From: Milan Le Date: Wed, 12 Aug 2026 01:14:03 +0200 Subject: [PATCH 10/10] fix(app): floor the panel width at zero, in BOTH panels that compute it Fifth review thread, and it was right: window - 2 * margin goes NEGATIVE once the window is narrower than the margins, and a BoxConstraints/SizedBox with a negative max is not normalized -- the layout ASSERTS ("BoxConstraints has both width and height constraints non-normalized") instead of rendering a cramped panel. "The window is absurd" should cost an ugly panel, not a crash. Confirmed by repro before fixing, and the repro is the interesting part. My first attempt -- open the panel in a 20x20 window -- PASSED while proving nothing: at that size the trigger button is unhittable, so the tap lands on nothing and the panel never opens. A test that cannot fail. Both tests now shrink the window WHILE the panel is open, which is also the realistic path: a resize animation or an embedded host hands us one degenerate frame. session_identity.dart maxWidth AND maxHeight floored at 0 (threshold 24pt, margin 12). context_usage.dart the SPEC-37 panel this one copied its sizing from has the SAME bug on the width axis, at a 16pt threshold (margin 8). Its height axis was already safe, floored by _kUsagePanelMinHeight; the width axis had no floor. Not in this PR's diff, but confirmed real with a red test, so it is fixed here per AGENTS.md rather than left for someone to hit. I nearly recorded that second one as "not reproducible": my first attempt used a 20pt window, which is negative for a 12pt margin but still POSITIVE for an 8pt one, so the assert did not fire. It needed 10pt. Worth stating because the near-miss is the whole lesson -- a threshold bug needs a size chosen from the constant, not reused from the previous test. Same coverage hole under both: neither panel had a single `desktop: true` test, so neither desktop host was covered at all. That is also how the identity panel's doc comment came to describe an anchored popover while shipping a centred dialog. --- app/lib/ui/composer/context_usage.dart | 16 +++++++-- app/lib/ui/session/session_identity.dart | 17 +++++++--- app/test/context_usage_test.dart | 27 +++++++++++++++ app/test/session_identity_widget_test.dart | 38 ++++++++++++++++++++++ 4 files changed, 91 insertions(+), 7 deletions(-) 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 index 5631c091..2b1b6ec8 100644 --- a/app/lib/ui/session/session_identity.dart +++ b/app/lib/ui/session/session_identity.dart @@ -504,13 +504,22 @@ Future showSessionIdentity({ 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.min( - kIdentityPanelWidth, - window.width - 2 * _kIdentityPanelMargin, + maxWidth: math.max( + 0, + math.min( + kIdentityPanelWidth, + window.width - 2 * _kIdentityPanelMargin, + ), ), - maxHeight: window.height - 2 * _kIdentityPanelMargin, + maxHeight: math.max(0, window.height - 2 * _kIdentityPanelMargin), ), child: SingleChildScrollView(child: body()), ), 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_identity_widget_test.dart b/app/test/session_identity_widget_test.dart index 175ffcf3..a5c35d78 100644 --- a/app/test/session_identity_widget_test.dart +++ b/app/test/session_identity_widget_test.dart @@ -384,6 +384,44 @@ void main() { 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 {