Skip to content

fix(commands): resolve all /compact and /model review issues - #1

Merged
leduckhc merged 5 commits into
mainfrom
cursor/fix-compact-model-commands-3fec
Jul 4, 2026
Merged

fix(commands): resolve all /compact and /model review issues#1
leduckhc merged 5 commits into
mainfrom
cursor/fix-compact-model-commands-3fec

Conversation

@leduckhc

@leduckhc leduckhc commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Fixes all 7 issues identified in the /compact and /model command review, plus 3 findings from a GPT 5.5 second-pass review and the PR CI failures.

Changes

Wire protocol — new session.action_error event

  • server/src/protocol.ts + server/src/protocol/codec.ts: add "session.action_error" to EventKind
  • app/lib/transport/protocol.dart: add sessionActionError with wire string "session.action_error"
  • Both fixture files updated; contract tests updated

pino-mirror.ts — error handling in runAction

  • /compact silent no-op fixed: guard !lastCtx, emit emitActionError("compact", "session not ready")
  • /compact async failure fixed: wrap compact() in Promise.resolve().catch() — if it rejects, emit the error back
  • /model lookup failure fixed: emit error when modelRegistry.find() returns undefined
  • /model promise rejection fixed: void pi.setModel(...).then(...).catch(...) — explicit fire-and-forget with error emission

Client store — session.action_error handling

  • ActionError class in models.dart (SRP: domain model, not store logic)
  • StoreState.actionErrors: Map<String, ActionError> — same pattern as meta
  • Reducer handles sessionActionError: advances cursor, stores per-session, adds no chat item
  • sessionActionErrorProvider family provider
  • foldEvents switch covers sessionActionError (no chat item)

Session screen — error snackbar

  • ref.listen(sessionActionErrorProvider(...)) shows "<action> failed: <reason>" snackbar on new errors (seq-guarded)

client_commands.dart — fixes

  • /compact null-meta guard: "Not available for this session" for non-host sessions
  • /compact snackbar: "Compact requested" (non-confirmatory)
  • /model re-select guard: re-reads sessionMetaProvider after the picker await (fixes stale-capture bug found in review)
  • /model snackbar: "Switching to ${name}…" (non-confirmatory)

CI fixes

  • Root cause fix(commands): resolve all /compact and /model review issues #1: workflow pinned Flutter 3.22.0 / Dart 3.4.0, but app/pubspec.yaml requires Dart >=3.10.0 and Flutter >=3.38.0, so all three Flutter CI jobs failed during dependency resolution
  • Root cause feat: rich tool detail views for read/write/bash/grep #2: after updating the SDK, flutter pub get --offline failed because the fresh GitHub runner cache did not contain all locked packages (flutter_native_splash)
  • Updated Flutter workflow pin to 3.44.4
  • Replaced flutter pub get --offline with flutter pub get --enforce-lockfile, preserving lockfile integrity while allowing cache misses to fetch from pub.dev
  • Removed obsolete flutter pub outdated --mode=null-safety flag unsupported by current Flutter
  • Applied Dart formatter output required by the newer SDK's format gate
  • Removed accidental server/package-lock.json; server uses pnpm-lock.yaml

Tests

  • client_commands_test.dart: /model added to builtin registration test
  • store_reducer_test.dart: new test for session.action_error reducer path

Issue map

# Issue Resolution
1 /compact silent no-op when lastCtx undefined Guard + emitActionError
2 /model promise rejection swallowed void .then().catch() + emitActionError
3 /model model-lookup failure silent Null check + emitActionError
4 Both: optimistic snackbars with no error path Non-confirmatory wording + session.action_error → error snackbar
5 Both: silent no-op on non-host sessions Null-meta guard on /compact
6 /model allows re-selecting current model Re-select guard (reads fresh meta post-await)
7 /model missing from registration test Added
R1 Stale meta in re-select check Re-read provider after await _pickModel
R2 compact() async failures not caught Promise.resolve().catch() wrapping
R3 Floating Promise on setModel chain void prefix
CI1 Flutter checks failing due old SDK Workflow SDK pin updated to Flutter 3.44.4
CI2 Flutter checks failing due offline cache miss Use --enforce-lockfile instead of --offline

Verification

app: flutter pub get --enforce-lockfile
app: flutter analyze --no-pub
app: dart format --output=none --set-exit-if-changed lib test
app: flutter test --coverage
app: flutter pub outdated
server: corepack enable && pnpm install --frozen-lockfile && pnpm test
Open in Web Open in Cursor 

cursoragent and others added 5 commits July 4, 2026 22:37
Add session.action_error to the wire protocol (server + client) so the
pi extension can signal control-action failures back to the phone.

Co-authored-by: Milan Le <leduckhc@users.noreply.github.com>
…ionId field

SRP: ActionError is a domain model, belongs in models.dart alongside
SessionMeta and ModelInfo — not in store.dart.

YAGNI: ActionError.sessionId was set but never read; the store already
keys actionErrors by sessionId so the field was pure dead weight.

Co-authored-by: Milan Le <leduckhc@users.noreply.github.com>
- Re-read sessionMetaProvider after picker await in /model to avoid
  stale current-model check on re-select guard
- Wrap compact() in Promise.resolve().catch() to capture async failures
- Prefix setModel chain with void to be explicit about fire-and-forget

Co-authored-by: Milan Le <leduckhc@users.noreply.github.com>
- Update Flutter CI from 3.22.0 to 3.44.4 so Dart satisfies the app's >=3.10.0 constraint
- Remove the obsolete --mode=null-safety flag from pub outdated on newer Flutter
- Apply formatter output required by the newer Dart SDK used in CI
- Remove accidental npm package-lock; server uses pnpm-lock.yaml

Co-authored-by: Milan Le <leduckhc@users.noreply.github.com>
CI runners do not have every pub package cached for the updated Flutter SDK, so
--offline fails before checks run. Use --enforce-lockfile to fetch missing
packages while still preventing pubspec.lock drift.

Co-authored-by: Milan Le <leduckhc@users.noreply.github.com>
@leduckhc
leduckhc marked this pull request as ready for review July 4, 2026 23:15
@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@leduckhc
leduckhc merged commit 75afd4b into main Jul 4, 2026
3 checks passed
@leduckhc
leduckhc deleted the cursor/fix-compact-model-commands-3fec branch July 5, 2026 08:56
leduckhc added a commit that referenced this pull request Jul 11, 2026
…arding spec

Public-launch polish:
- README: hero (logo, tagline, badges), "Why Makit" value prop, demo
  placeholder + docs/media/ recording guide (#1)
- SECURITY.md: makit-specific vulnerability disclosure policy (private
  reporting via GitHub advisories / license@getmakit.dev) (#5)
- .github/ISSUE_TEMPLATE (bug, feature, config) + PULL_REQUEST_TEMPLATE (#5)
- docs/specs/SPEC-09: onboarding readiness-wizard + tray-polish plan (#4)

#4 is specced, not implemented: the onboarding wizard is a stateful
readiness machine and the tray polish is design-led -- both warrant a spec
plus mockups before code, per the repo spec-before-implementation rule.
leduckhc added a commit that referenced this pull request Jul 16, 2026
Fixes from reviewer/QA feedback:
- (#1) Header tap activation: move GestureDetector.onTap from no-op to controller.setActive
- (#4) Divider drag staleness: use live ref.read for ratio instead of build-time snapshot
- (#7) Sibling-focus on close: prefer adjacent pane over first-leaf when closing
- (#5) Global fallback only for active pane: inactive panes no longer mirror global selection
- (#6) Accessibility: add 'Move pane' semantics to grip + bump close button to 24px (min target)
- (#8) Divider offset handling: document that moveLeaf correctly uses global offset

All changes driven by TDD: failing test → fix → green.
Test additions: +18 (header-tap, fallback, a11y, divider ratio)
Total: 429 tests pass, analyze clean.
leduckhc added a commit that referenced this pull request Jul 16, 2026
- Gate the global worktree fallback with trackGlobalSelection so only the
  active pane mirrors the global session/worktree draft (#1); delegate both
  fallbacks to DesktopChatPane via trackGlobalSelection=active.
- Quit: capture notifiers before await and run pane-unbind + selection clear
  regardless of widget unmount; guard only the snackbar with messenger.mounted.
- Split: bail with a mounted check after the new-session dialog before touching
  providers through a possibly-disposed ref.
- Test: assert the close target via widgetWithIcon(IconButton, Icons.close).
- Add tests: non-tracking pane ignores the global worktree draft.

All TDD (failing test -> fix). analyze clean; 430 tests pass.
leduckhc added a commit that referenced this pull request Jul 16, 2026
* feat(desktop): add split-pane layout for chat sessions

Introduce a pane tree (pane_node, pane_tree_controller, pane_tree_view)
so the desktop chat surface can split into multiple session panes.
DesktopChatPane now accepts an explicit sessionId and optional header,
and the session title/actions menu are extracted for reuse in the pane
tab strip. Wire split shortcuts (Cmd-D / Cmd-Shift-D) and bind sidebar
selection to the active pane.

* fix(desktop): resolve pane-splitting review findings

- #1: Header tap now activates pane (removed no-op onTap)
- #4: Divider drag no longer stale (read state live via controller)
- #5: Only active pane tracks global selection (inactive panes stay pinned or empty)
- #7: Close button focuses sibling pane (not first leaf)
- #6: a11y improvements (Move semantics label, 24px close target)
- #3: Cancel ⌘D dismisses new empty pane (TDD guard tests added)

Added 27 new tests; all 429 pass. analyze: clean.

* fix(desktop): resolve pane-split review findings (TDD-driven)

Fixes from reviewer/QA feedback:
- (#1) Header tap activation: move GestureDetector.onTap from no-op to controller.setActive
- (#4) Divider drag staleness: use live ref.read for ratio instead of build-time snapshot
- (#7) Sibling-focus on close: prefer adjacent pane over first-leaf when closing
- (#5) Global fallback only for active pane: inactive panes no longer mirror global selection
- (#6) Accessibility: add 'Move pane' semantics to grip + bump close button to 24px (min target)
- (#8) Divider offset handling: document that moveLeaf correctly uses global offset

All changes driven by TDD: failing test → fix → green.
Test additions: +18 (header-tap, fallback, a11y, divider ratio)
Total: 429 tests pass, analyze clean.

* fix(desktop): address CodeRabbit review on split panes

- Gate the global worktree fallback with trackGlobalSelection so only the
  active pane mirrors the global session/worktree draft (#1); delegate both
  fallbacks to DesktopChatPane via trackGlobalSelection=active.
- Quit: capture notifiers before await and run pane-unbind + selection clear
  regardless of widget unmount; guard only the snackbar with messenger.mounted.
- Split: bail with a mounted check after the new-session dialog before touching
  providers through a possibly-disposed ref.
- Test: assert the close target via widgetWithIcon(IconButton, Icons.close).
- Add tests: non-tracking pane ignores the global worktree draft.

All TDD (failing test -> fix). analyze clean; 430 tests pass.
leduckhc added a commit that referenced this pull request Jul 20, 2026
* feat(app): migrate icons to Phosphor (Light) + always-visible send button

- Swap Material Icons / material_symbols_icons for Phosphor icons via
  phosphoricons_flutter (Dart 3.x compatible; the original phosphor_flutter
  extends the now-final IconData and won't compile on Flutter 3.44).
- Default weight PhosphorIconsLight.*; PhosphorIconsFill.* for filled
  states (status dots, favorite stars, selected check-circles).
- PRs now use gitPullRequest (sidebar worktree + repo PR pill).
- Composer send button is always visible: disabled/grayish when empty,
  enabled once there's text.
- Update find.byIcon test expectations to the new icons.

* feat(app): stock M3 theme + sidebar polish

Theme (lib/app/theme.dart):
- Adopt stock Material 3 via ColorScheme.fromSeed (green seed), keeping a
  neutral C=0 surface ramp so backgrounds stay grey (no green tint) while
  gaining the M3 surfaceContainer* elevation hierarchy. Brand-accent uses
  colorScheme.primary; dropped kComposerBox*/kRepoAccent/_kBrandBlue.
- MASTER.md updated: color model marked migrated to stock M3.

Sidebar (lib/desktop/chat/desktop_sidebar.dart):
- Background: surfaceContainer (distinct panel).
- #1 Indent grid: repo icon 16 / worktree icon 32 / branch name, sub-label
  and nested sessions all at 54 (sub-label under the branch name).
- #2 Inset rounded selection pill across worktree/draft/session rows.
- #4 Status dots moved to the leading icon column, palette-tuned colors
  (primary/error/outline + softened ambers); idle reserves an empty slot.
- #5 Footer gear normalized to 18px; removed dead weight:200 param.

Shell (desktop_chat_shell.dart): resize-handle divider flush to the sidebar
edge (no gap).

Mockups: add mockups/sidebar-cleanup.html (before/after reference).

* feat(sidebar): section-header repo row + consistent hover pills

Repo header redesigned as a proper section header:
- Plain folder icon (drop folderStar), uppercase muted labelMedium name.
- Collapse caret: fades in on hover/focus, always shown when collapsed.
- Whole row is now one inset-8 / radius-10 InkWell pill (matches the
  worktree/session rows), so the hover background spans folder + name +
  caret + the actions instead of only the name.
- Repo actions (…) and New worktree (+) are hover-only nested buttons,
  size-matched to 22px.

Tests updated for the uppercased label and hover-only +.

Mockups: add repo-header.html and repo-hover.html (before/after refs).

* fix(sidebar): remove the repo group collapse caret entirely

* feat(chat): consistent transcript layout tokens, tuned tool colors, shared busy shimmer

Give the chat transcript one shared layout contract so rows align by
construction instead of by matching per-widget insets:

- Add chat_metrics.dart: gutter/row-gap + radius scale tokens, tuned
  tool-risk colors, and the transcriptRow() wrapper.
- Make item widgets gutter-agnostic (zero horizontal padding); both the
  mobile SessionScreen and desktop DesktopChatPane apply the single
  gutter + row gap via transcriptRow, fixing the 16/28/32px left-edge
  jitter and uneven vertical rhythm.
- Replace raw Colors.orange/red on tool cards with the tuned palette that
  matches the sidebar status dots; ok/copied checks use colorScheme.primary.
- Unify the busy indicator to the shimmer word on both surfaces and drop
  the now-dead compact flag across chatItemWidget/ThinkingLine/ErrorBanner/
  WorkingIndicator.
- Document the tokens in the design system; add mockups/chat-pane.html.

* feat(sidebar): add 'Add repo' button in footer + update empty-state copy

* feat(panes): Close pane vs Quit session, consistent across single + split

Every pane (single-leaf tree or split) is headed by _PaneHeaderStrip, which
already carries both Session actions + Close pane; this makes the two behaviors
distinct and consistent:

- Close pane (X) closes only the pane; the session keeps running. Closing the
  last pane now clears the worktree's tree to the empty 'Select or start a
  session' placeholder instead of being a no-op, and resets the sidebar
  highlight. Extracted as closePane().
- Quit session (in Session actions) kills the session AND closes its pane: the
  menu takes the hosting leaf id and closes that pane after the kill, then
  unbinds the session from any other panes. The newer-selection guard is kept.

Updated the controller's last-pane close test and added pane-tree widget tests
for Close-keeps-session and Quit-closes-pane.

* feat(panes): close the pane optimistically on Quit

Quit now closes the pane immediately instead of awaiting the server. The kill
request fires in the background; the sidebar reconciles from server snapshots,
so on failure the session simply stays/reappears there and we surface a
snackbar. Notifiers are captured before the async gap since the optimistic
close disposes the menu's widget. Test asserts the pane is gone while the kill
is still in flight.

* feat(panes): title-bar hierarchy + separator (implements title-bars mockup)

Give the two stacked desktop title rows a clear hierarchy and separation:

- Window title (_WorktreeTitle): worktree/branch becomes a quiet uppercase,
  letter-spaced, muted context label (matches the sidebar repo header), keeping
  its fork icon.
- Pane title (_PaneHeaderStrip): session name is the primary line (w500,
  onSurface) with a leading status dot for icon parity; leading edge aligned to
  the 12px pane gutter under the worktree fork icon.
- Thin 1px hairline (outlineVariant) under the window title strip separating
  'which worktree' from the session + transcript.
- Extract the sidebar's status dot into a shared SessionStatusDot widget used by
  both the sidebar tiles and the pane header (DRY).

Updated the title-strip tests for the uppercase label and documented the title
hierarchy in the design system.

* fix(panes): don't uppercase the window title (keep branch case)

Keep the muted, letter-spaced context-label treatment but preserve the
worktree/branch's original case (branch names + path fallbacks read correctly).

* feat(panes): give the window title strip the sidebar background

Wrap the pane-area window title strip in surfaceContainer (the sidebar's
background) so the two form one continuous top band across the sidebar and
pane area.

* fix(shell): fill the sidebar/title-bar gap in the top band

The sidebar resize handle's transparent grab strip showed the chat bg as a
notch between the sidebar and the (now surfaceContainer) window title strip.
Fill just the title-bar row of the handle with surfaceContainer so the top
band is continuous; the hairline divider + transparent grab strip stay below.

* fix(titlebar): center the leading toggle so it never moves on show/hide

The sidebar toggle was top-aligned with a per-call-site inset (7 in the sidebar
header, 3 in the pane strip), so toggling the sidebar jumped the icon ~4px \u2014
made worse by a taller strip. Center the leading control vertically and drop
the ad-hoc leadingTop param so the icon sits at the same height regardless of
sidebar state or strip height.

* fix(titlebar): continuous hairline under the whole top band

The titlebar bottom border only existed in the pane area, so it stopped 8px
short of the sidebar (the resize handle) and was absent over the sidebar header.
Add the 1px divider under the sidebar header and across the resize handle so
the hairline runs unbroken across sidebar + handle + pane title (all at y=32).

* fix(shell): full-height sidebar divider; titlebar hairline stops at it

Swap the top-band separator scheme: the sidebar is now delimited by a
full-height vertical hairline (flush to its edge, through the title band and
down the chat body), and the horizontal titlebar hairline meets but no longer
crosses the sidebar. The resize handle carries the vertical line at its left,
the surfaceContainer band fill, and the horizontal hairline to the right of the
divider (connecting to the pane title strip's divider). Removed the sidebar
header's horizontal divider.

* feat(chat): center the desktop transcript to the readable width

The transcript rows were capped by a ConstrainedBox but not centered \u2014 a no-op
in a ListView, so content stretched edge-to-edge. Wrap each row in Center +
ConstrainedBox (kReadableContentMaxWidth) like the composer, so the transcript
column lines up with the input. The ListView stays full width so the mouse
wheel still scrolls anywhere in the pane.

* perf(chat): batch initial history replay; load desktop sidebar first

Buffer a session's sub-replay events between the sub and its ack, then
apply them in one state update. The reversed transcript now lands
directly at the newest message instead of visibly racing through
history top-to-bottom, and the whole initial load is a single rebuild.

Defer the desktop pane's per-session subscribe to a post-frame callback
so the sidebar paints before the conversation replay begins.

* fix(sidebar): update title bar strip height to match macOS standards

* feat(desktop): add ⌘T new-pane and ⌘W close-pane shortcuts

⌘T (newPane) resets the active pane to a fresh empty harness-picker pane
in the same worktree without splitting; the bound session keeps running.
⌘W (closePane) closes the active pane's view only — the session is not
deleted. Both are rebindable in Settings → Shortcuts → Window.

- PaneTreeController.resetActiveToEmpty replaces the active leaf with a
  new null-session leaf.
- newPaneInActiveWorktree / closeActivePane helpers wire the actions and
  keep the sidebar selection in sync.

* fix(desktop): sync selection after pane close

* chore: apply required formatting

* fix(store): avoid optimistic replay sequence collisions
leduckhc added a commit that referenced this pull request Jul 22, 2026
…25) (#90)

* feat(app): inline expandable tool rows (SPEC-24)

Render tool calls in the transcript as collapsed one-liners that expand
in place (mirroring ThinkingLine) instead of big cards that navigate to a
full-screen detail page. Applies to both iOS SessionScreen and macOS
DesktopChatPane via the shared chatItemWidget.

- ToolRenderer: detail(Scaffold) -> body(inline sections) + summaryLine
- ToolCodeBlock: syntax-highlighted, copyable code blocks (flutter_highlight)
  for bash command/output, read/write content, grep/generic output
- add memory + skill renderers; bounded (~20-line) inner-scroll for long bodies
- remove ToolCallDetailScreen, the /tool route, and desktop _openToolDetail

* fix(app): address SPEC-24 review (gpt-5.6-sol)

- BLOCKING: key transcript rows by item identity (KeyedSubtree +
  chatItemKey) at the ListView-child level so inline tool expand/collapse
  state stays with the right call as the reversed list reorders; the leaf
  key alone was insufficient. Add a regression test.
- memory renderer: title args 'Input' (not 'Saved') so searches/other
  actions read correctly
- drop now-dead toolDisplayName helper (orphaned by the inline refactor)
- tests: cover memory/skill bodies + icon-to-collapse; update spec

* fix(app): give expanded tool body its own ScrollController

The inline Scrollbar had no controller, so on macOS/desktop hover it grabbed
the (empty) PrimaryScrollController while the transcript ListView uses its own
controller, asserting 'Scrollbar's ScrollController has no ScrollPosition
attached'. Attach a dedicated controller to the Scrollbar + SingleChildScrollView.

Regression test hovers the scrollbar thumb under TargetPlatform.macOS with
overflowing content (fails without the fix).

* fix(app): make whole tool-row header toggle expand/collapse

Previously only the ~16px leading icon collapsed an expanded tool row, so
collapsing was nearly undiscoverable. Make the entire header a single
InkWell toggle in both states, add a rotating disclosure caret affordance,
comfortable hit padding, and button/expanded Semantics. The body stays a
separate scroll/copy region below the header.

* feat(app): collapse tool header to just the verb when expanded

When a tool row is expanded, the full command/path already shows in the
body, so the header repeating it is redundant and long. Add ToolRenderer.label
(the verb: Ran/Read/Edited/Wrote/Grep/Memory/Skill) and show it instead of
the full summaryLine while expanded; collapsing restores the full one-liner.

* fix(app): move tool fold/unfold caret to the trailing edge

Place the disclosure caret (>/v) at the right end of the header row (after
the status glyph) instead of leading, per UX request.

* feat(app): show tool disclosure caret only on hover

The fold/unfold caret fades in on pointer hover (InkWell.onHover) and its
space is reserved so the row never reflows. On touch (iOS) there is no hover,
so the caret stays hidden; the whole header remains tappable to toggle.

* docs(mockup): ask-user question rendered inline in chat (SPEC-25 draft)

HTML mockup exploring moving askUserQuestion from the modal AskWizard to an
inline transcript card: before/after, single/multi-select, free-text, a
multi-question stepper, resolved-history state, and awaiting-composer options.

* docs(spec): SPEC-25 ask-user question inline in chat

* feat(app): render askUserQuestion inline in the chat (SPEC-25)

Move the live askUserQuestion elicitation from the modal AskWizard to an
inline transcript card, per SPEC-25 decisions:
- new elicitation store (store/elicitation.dart): passive per-session
  PendingAsk state, answered via the connection's respondTo; clears on the
  'responded' stream (answers from notifications sync the card away)
- srv_request_handler: route foreground/desktop askUserQuestion into the
  store instead of a modal; confirmAction/input/generic stay modal; mobile
  background notification + desktop reminder paths unchanged
- ask_card.dart: inline card — single/multi-select, always-Submit, a
  multi-question n/N stepper, and 'Type a different answer' handoff
- transcript wiring: render the pending ask as the trailing row on iOS
  (SessionScreen) and macOS (DesktopChatPane), keyed by requestId
- composer: paused with a hint while awaiting; free-text mode re-enables it
  and routes the next submit to the elicitation response (single-question)
- resolved state = the persisted askUserQuestion tool row folds to a
  one-liner (reuses SPEC-24 fold), so no new answered widget

Tests: elicitation store, ask-card widget (all question types + free-text),
and updated the desktop handler test to assert inline (not modal).

* fix(app): address SPEC-25 review (gpt-5.6-sol)

- BLOCKING: an awaiting ask now takes priority over the working indicator
  (trailerFor). Pi stays running while it emits askUserQuestion, so
  running && pendingAsk co-occur; the old code hid the question AND paused
  the composer -> deadlock. Unit-tested.
- BLOCKING: free-text answers use a dedicated empty answer controller (keyed
  by requestId) on both surfaces, so a pre-ask normal draft can't leak in as
  the answer; the normal draft is preserved separately.
- add a Skip action (wires up the previously-unused cancel()) so multi-
  question asks have an escape hatch and the agent is never left hung.
- elicitation.add() drops a displaced request's stale mapping (one ask per
  session); a late 'responded' for it no longer clears the new card.
- ask option tiles gain checkbox/radio Semantics; actions use a wrap-safe
  layout to avoid overflow on narrow widths / large text.
- tests: trailerFor priority, store displacement, Skip/cancel, composer
  disabled state.

* feat(app): answered askUserQuestion renders as a quiet resolved card

Change SPEC-25 decision #1: instead of folding the answered question into a
one-liner tool row, render the persisted (ended) askUserQuestion as a
neutral-bordered AnsweredAskCard — chosen option highlighted, the rest dimmed
(matching the old _AskUserQuestionRenderer answered form) — shown inline as
history while the agent's turn continues below.

- new AnsweredAskCard (ask_card.dart); chatItemWidget routes ended
  askUserQuestion tool calls to it, bypassing the SPEC-24 fold for that name
- tests: AnsweredAskCard highlight/dim + chatItemWidget routing

* fix(app): route pi's ask_user (not just askUserQuestion) to the resolved card

The persisted ask tool is named ask_user (pi's toolName), but _isAnsweredAsk
only matched 'askUserQuestion', so answered asks still fell through to the
folding tool row. Match both, normalizing underscores/case (ask_user,
askUserQuestion, AskUser).

* fix(app): resolved ask card reads pi's real ask_user shape; fix action layout

#2: pi's ask_user result carries the answer in details.response / the
'User answered: <text>' output and options as {title} (not the uicall
{answers}/{label} shape), so the resolved card highlighted nothing. Read both
shapes: options via label|title, chosen via details.answers -> details.response.text
-> stripped output. Free-text answers now show as the chosen row.

#1: the live ask card's actions no longer Wrap the 'Type a different answer'
label away from Submit — free-text is its own row above [Skip … Submit].

tests: pi details shape (title options + freeform response), output-derived
choice, free-text answer rendering.

* docs(mockup): propose inline free-text field for ask-user (SPEC-25 #3)

Replace the composer-handoff / modal-input free-text approaches with an
inline text field revealed inside the ask card itself; composer stays paused.

* feat(app): render context and comment in answered ask cards

* feat(app): answered ask card shows Skipped state for cancelled asks

Follow-up to context/comment rendering: the header now reflects a cancelled
(skipped) ask with an x-circle + 'Skipped' label instead of always 'Answered';
cancelled asks already highlight nothing. Tests cover context + option
descriptions + comment, and the skipped state.

* fix(app): render multi-select ask_user inline (was a modal/pane)

pi-ask-user's headless multi-select fallback calls ctx.ui.input with the
options embedded in the prompt text ('Options (select one or more):\n1. ...')
and expects a comma-separated reply — so makit showed it as an input dialog,
not the inline checkbox card.

Parse that shape back into a structured multi-select PendingAsk
(PendingAsk.fromMultiSelectInput: question + context + options{label,description},
multi:true) and render it inline via the existing AskCard; answer on the
'input' channel as comma-joined titles (cancel likewise). The live AskCard now
also renders context. Single free-text input stays modal.

tests: parse (question/context/options+desc), submit/cancel via input channel,
and inline multi-select widget submitting comma-separated titles.

* fix(app): address PR #90 review (CodeRabbit)

- ask card free-text mode keeps a Skip escape hatch (was dropped when the
  free-text note replaced the actions row, leaving the ask un-cancellable)
- clear the dedicated free-text answer controller when the ask ends or leaves
  free-text mode, so a later answer composer never reopens with a stale draft
  (both session_screen + desktop_chat_pane)
- mockup: single-select copy now says select + Submit (not answer-on-tap),
  matching the shipped explicit-Submit behavior

tests: free-text Skip cancels the ask.

* refactor(mockup): safer icon injection via data-icon attributes + fallback

* docs(mockup): replace innerHTML rewrite with in-place icon injection

Swap the document.body.innerHTML placeholder rewrite (which recreates every
node, resetting <input> state and dropping listeners) for a TreeWalker that
replaces only placeholder text nodes in place, preserving DOM state. Drops the
dead data-icon loop (no element used it). Icons still render (52 SVGs, 0
placeholders left).

* docs(mockup): skip script/style nodes in icon TreeWalker

The walker started at document.body and also visited this script's own text
node (which contains the __TOKEN__ keys/regex), injecting SVGs into the script
element. Reject script/style subtrees via acceptNode so only visible
placeholders are replaced. Verified: 0 svg inside script, script source
preserved, all visible icons still render.
leduckhc added a commit that referenced this pull request Aug 11, 2026
* SPEC-46 P1a: the doc index's security boundary and title extraction

Docs preview (mockups, specs, plans) from the phone and desktop, replacing the
Finder-then-browser dance and the "ask the agent for a port" workflow. This is
the first slice: the spec, the design board, and the two pure modules the rest
of the feature is built on.

`resolveDocPath` is the single way a path reaches the serving layer, so both
`docs.read` and the static route cannot disagree about what is servable. It
refuses traversal, absolute paths, escaping symlinks, dotfiles, excluded build
dirs, non-allowlisted extensions and oversize files, and it never throws — a
rejection is a value. Containment is checked by path *segment*, not string
prefix, so `/repo-evil` cannot pass as inside `/repo`.

`readDocMeta` gives a document a human name, because
2026-08-07-SPEC-44-ports-forward.md is unreadable on a 375pt row while the real
title is already in the file. Reads a bounded 64 KB prefix rather than the whole
document.

Two things a live probe over all 131 real docs changed:
- an H2 fallback when a document has no H1 (three files here start at `##`;
  their heading beats their filename). 130/131 now yield a real title.
- no front-matter `name:` extraction: all 22 SKILL.md files already have an H1,
  so it would have been speculative.

Mutation-tested: the prefix-confusion test was vacuous as first written (the
`..` segment rule rejected the input before the containment check ran), so it
now reaches that check through a symlink instead.

Refs: docs/specs/2026-08-09-SPEC-46-doc-preview.md, mockups/doc-preview.html

* SPEC-46 P1b: freeze the docs wire contract

DocDTO / DocsSnapshotDTO / DocGrantDTO, the five `docs.*` commands, and
`docs.snapshot` as a host-only broadcast kind. Committed on its own so the
server and app halves can be built in parallel without both editing protocol.ts.

Two corrections this slice forced:

D11 was wrong. It claimed no new event kind was needed, citing SPEC-41 — but
SPEC-41 is precisely what introduced `ports.snapshot`; SPEC-44 is the spec that
adds none. `docs.snapshot` now follows ports.snapshot exactly: excluded from
`SessionEventKind` and present in `HOST_ONLY_KIND_FLAGS`, so a machine-wide
index can never be persisted into a session's append-only log.

`EVENT_KINDS` was an unguarded list. Mutation-testing the new contract test
showed one of its assertions was vacuous: `decodeFrame` validates only v/t/id
and never inspects `kind`, and `decodeSessionEvent` rejects a host-only kind for
being host-only before EVENT_KINDS is consulted — so a missing entry there is
undetectable at runtime, and my test's stated reason was false. Rather than keep
an assertion that cannot fail, EVENT_KINDS is now derived from an
`EVENT_KIND_FLAGS: Record<EventKind, true>`, the same compiler-enforced pattern
HOST_ONLY_KIND_FLAGS already uses (finding 26). Dropping a kind is now a build
error instead of silence, and the test says plainly what it can and cannot prove.

Contract fixture lives in snapshots.json (not events.json, which asserts one
entry per *session* kind), and covers an HTML board, a spec with a parsed
docStatus, and a doc with no optional fields set — so "absent" cannot silently
become `false`.

1150/1150 server tests green, tsc clean.

* SPEC-46: correct the mockup's published URL to a capability URL (D9)

The Option D frame drew /docs/<branch>/<path>. That cannot work: a URL that
must open in Safari cannot carry a bearer header, so the capability has to be
in the path — /docs/<grantId>/<relPath>, 32 bytes of CSPRNG. Annotated in the
frame rather than quietly redrawn, so the reasoning survives.

* SPEC-46 P1c: the docs index, grants, and the publish route (server)

roots/scan/changed/grants/route/publish/read/service plus the `docs.*` command
handlers and server wiring. Built by a subagent under the frozen contract; the
notes below are what my own review of it changed.

Two defects found by live probes, not by the tests:

The dedicated doc listener hung on every unmatched path. `attachDocRoute`
ignores non-/docs paths by design, so it can share a listener — but the doc
listener is dedicated, so nothing answered `/`, `/docs`, `/etc/passwd` or
`/favicon.ico` and the socket sat until Node's 60s headers timeout. Safari
requests /favicon.ico on every visit and the listener is bound to a routable
address, so that was a free socket-exhaustion vector. `attachDocNotFound` now
terminates anything the route declined, attached only on the dedicated listener.

Published URLs were not percent-encoded. A document with a space produced a
malformed URL, and one containing `#` or `?` truncated the path — the route then
compared a different relPath against the grant and returned 404, i.e. the
publish button silently produced a dead link for an ordinarily-named file. Now
encoded per segment, so slashes survive; proven end-to-end against a real
listener with `mockups/my notes #2 & draft.md`.

Also corrected publish.ts's doc comment, which described the tailscale-serve
design that was not what got implemented (see the D10 deviation below).

Verified myself rather than taking the report's word: tsc clean, 1219/1219
(baseline 1150), and a live probe serving a real file over a real socket —
200 for a valid grant, 404 for a wrong grantId, 404 for traversal.

Known deviation, still open: D10 specifies a loopback-only listener fronted by
`tailscale serve`; the implementation binds the doc listener directly to makit's
routable host over plain HTTP. That resolves a real D10/D15 conflict the
subagent correctly identified (a loopback-only listener cannot have a LAN
fallback), but it drops the stable https ts.net hostname that was Option D's
stated benefit and leaves a routable listener open even when nothing is
published. Not yet resolved — tracked for the next slice.

* SPEC-46 D10/D15 rev 2: bind the doc port lazily, and only on the tailnet

Resolves the conflict a subagent correctly found in rev 1: D10 said the doc
listener was loopback-only fronted by `tailscale serve`, while D15 promised a LAN
fallback. Both cannot hold — a loopback-only listener is unreachable over the
LAN — so the fallback was impossible as written.

Resolved toward the tighter option rather than the more convenient one:

Tailnet only. The capability lives in the URL path (D9), so on a LAN that URL
crosses the wire in cleartext and anyone sniffing the Wi-Fi could replay it for
the grant's lifetime. On the tailnet WireGuard already encrypts it. No tailnet
address now means publish refuses with a stated reason. `reach` keeps its
`"tailnet" | "lan"` union so the wire contract and the app's pills need no change
if LAN is ever reinstated behind an explicit opt-in.

Bound lazily, released when idle. rev 1 bound a routable port at startup and held
it for the life of the server whether or not anything was ever published. It now
binds on the first publish and closes when the last grant is revoked or reaped —
and because an expiry has no event of its own, `grants()` signals too, so a TTL
expiry frees the port without a timer.

No `tailscale serve`. It would buy a stable https ts.net hostname, but the URL is
never typed by a human — it is tapped, copied, or scanned from a QR — so it does
not justify a setup/teardown lifecycle on the user's machine. Recorded in D10
rev 2 with that reasoning rather than left as an undocumented deviation.

Proven by live probe, not just unit tests: zero listening sockets before any
publish, one while published (fetch → 200), zero after unpublish with the URL
then refusing the connection.

1227/1227 server tests green, tsc clean.

* SPEC-46 P1d: the Docs screen, markdown preview, and publish sheet (app)

Store, Docs screen (grouped repo → worktree, filters, search), the doc row, the
markdown preview widget, the publish sheet with QR, the desktop sub-row glyph +
popover, and the mobile worktree-row glyph. Built by a subagent that hit its
turn limit mid-edit, so everything below is my own verification.

One dead-end bug it shipped, caught by reading the wiring rather than the tests:
the Docs screen was UNREACHABLE on mobile. The phone's only entry point is the
worktree-row glyph, that glyph hides when the worktree owns no docs, and nothing
on the home screen held `docs.watch` — so no snapshot ever arrived, the list
stayed empty, and the glyph never rendered. Every widget test missed it because
they inject `docsProvider` directly instead of letting it arrive over the wire.
The row now holds the ref-counted watch exactly as it already holds
`ports.watch` for the plug, with a test that fails when the single `watch()`
line is removed.

The agent also reverted its own mobile app-bar Docs button after finding it
overflows the 320pt app bar as a 6th control — the right call, and the reason
the row glyph is the mobile entry. The 320pt overflow test still passes with the
new glyph mounted.

Verified rather than reported: `flutter analyze --fatal-infos` clean; three full
suite runs with ZERO non-loading failures; the failing sets were random (92
distinct files across 3 runs, none failing in all three, no docs test failing
consistently) and every flagged file passes in isolation — the known
pre-existing `loading <file> [E]` flake, which persists at --concurrency 1.

Traps checked explicitly: `docs.watch` uses fire-and-forget `send`, not
`request` (no leaked 10s ack timers); desktop opens docs via a popover, not
`context.go`, so it does not depend on a GoRouter the desktop shell has not got;
the mobile `context.go(kRouteDocs)` is on a mobile-only widget and the route is
registered.

Pre-existing, NOT touched: `desktop_sidebar.dart` still calls
`context.go('$kRoutePorts?repo=…')` for the SPEC-42 ports menu item, which has
no GoRouter on desktop and will throw at runtime. Unrelated to this change.

* SPEC-46 P1e: re-index docs when the worktree list arrives

The Docs screen was empty against the real server — 0 docs where a direct
`scanWorktree` found 131 — and it never recovered.

The app sends `docs.watch {on:true}` from the home screen's initState, so it
lands immediately after `hello.ack`, BEFORE the server's first `repos.snapshot`.
The doc index is keyed off that worktree list, so the 0→1 edge (which scans
synchronously, no debounce) walked zero worktrees and broadcast an empty index.
Nothing else triggered a re-walk: the only re-index trigger was the filesystem
watcher, and no file had changed. On mobile that compounded into the dead end
fixed in the previous commit — an empty index means the row glyph never renders,
so the screen is unreachable.

`lastGitOnlyRepos = gitOnly` now also calls `docsService.onWorktreeChange()`,
debounced inside the service. The worktree list is an input to the index, so a
change to it must re-walk, exactly as a file change does.

Found by driving the real WSS server with the app's own timing, not by tests —
every unit test supplied a worktree list up front, so none could see it. Guarded
now by a service test that fails if a later worktree list does not produce a
fresh walk (proven by memoizing the list and watching it fail).

Verified end to end against the real stub server with the app's timing:
snapshot #1 empty (repos not yet loaded) → snapshot #2 with 1008 docs across 8
worktrees, scanOk=true, 225 html / 783 md, 393 carrying a parsed Status, 11
changed against their merge base, real extracted titles, and no dotfile,
`.git/` or `node_modules/` path present. Publish correctly refuses on the
loopback dev server (D15: no tailnet address ⇒ share nothing).

1228/1228 server tests green, tsc clean.

* SPEC-46: let the P1e regression test type-check

`pnpm typecheck` failed at HEAD with two TS7006 errors in the test P1e added.
The deps object was passed as `} as never)`, which suppresses contextual typing
for the whole argument, so `onSnapshot: (s) =>` and `scan: (worktreePath) =>`
had no inferred parameter types and tripped `noImplicitAny`.

The casts were load-bearing for nothing: this file's own `makeService` helper
(line 38) builds the same deps with no casts at all and infers cleanly. Dropping
them fixes the build AND restores real checking of the deps this test asserts
against — `as never` had been hiding the shape, which is how a `stdout`-only
`exec` (no `stderr`) got past review.

12/12 docs service tests still pass, 1228/1228 server tests green, tsc clean.

Note, not touched: the test above it (~line 205) uses the same `as never` idiom.
It does not error today only because its callbacks take no parameters, so it is
a latent version of this same failure.

* fix(qa): stop offering --lan as a remedy publishing cannot use

Found by driving the real app against a loopback-bound server: the publish
refusal read "makit is loopback-only. Start Tailscale (or pass --lan) and try
again." Passing --lan does not help. It binds a 192.168/10.x host, and the doc
listener only ever binds a tailnet address:

  tailnetAddressFromBindHost 127.0.0.1   -> null  (refuses)
                             192.168.1.9 -> null  (refuses)
                             10.0.0.5    -> null  (refuses)
                             100.119.58.97 -> ok

So the message sent the user down a road that ends in the same refusal. D15
rev 2 dropped the LAN fallback on purpose — the capability lives in the URL
path (D9), which on a LAN would cross the wire in cleartext — and D15's whole
point is that failure is "stated, never silent". A remedy that cannot work is
the one thing it forbids: the refusal was lying.

The existing D15 test only asserted `reason.length > 0`, which is how this got
through review. The new test asserts the content: no "--lan", and Tailscale —
the one remedy that does work — is named.

1229/1229 server tests green, tsc clean. Re-verified on the simulator: the
sheet now reads "Start Tailscale and try again."

* fix(qa): let the title lead the doc preview, not the front-matter chips

The preview hoisted the Status/Priority/Branch chips above the document's H1,
so on every spec in the repo the metadata outranked the title. The file itself
is written the other way round — H1 on line 1, `**Status:** …` on line 3 — and
mockup Card 6 draws title, then the strip, then the divider. The toolbar
already carries the title, so putting metadata first bought nothing.

`parseDocFrontMatter` knew where the line was and threw that away: it removed
the line and returned one body, which left the caller no way to render the
strip in place. It now returns the markdown either side — `lead` (in this repo,
the H1) and `body` — and the preview renders lead, chips, body.

Two details worth naming:
- The old "collapse a doubled blank line" fix-up is gone; splitting at the line
  and trimming blank edges makes it unnecessary.
- Blank edges are trimmed with `^\n+` / `\n+$` rather than `String.trim()`, so a
  body that opens with an indented code block keeps its indentation.

There are now two MarkdownBody widgets when a doc has front matter, which is
why `renders the markdown body` asserts findsNWidgets(2).

Regression test asserts geometry, not structure: the chip strip's dy must be
greater than the H1's, so re-hoisting the chips fails the test. A second test
pins the no-front-matter path, which must stay a single unchanged body.

46/46 app docs tests green, flutter analyze clean. Verified on the iOS
simulator against the real server: H1, then chips, then Design board, then the
rule — the order mockup Card 6 draws.

* feat(docs): index every doc git does not ignore, and make the list navigable

Reported from the popover on `teachme`: only three root `.md` files showed. That
was D1 working as written — the index walked `mockups/`, `docs/` and root `*.md`
— and D1 was written against this repo's layout. Measured:

  teachme   3 docs indexed,  69 present   (docs live in flutter/…, ssf/…)
  makit   132 docs indexed, 143 present

makit's sidebar holds many repos, so one repo's convention is the wrong default.

D1 rev 2 asks git instead: every `.md`/`.html` from `git ls-files --cached
--others --exclude-standard`, so tracked and freshly-written untracked docs both
appear, wherever they live. Deferring to `.gitignore` also *tightens* the
security boundary — a gitignored `secrets.md` can no longer be indexed or
served, where rev 1 indexed it happily, since the dotfile rule never covered it.
Dot-directories still drop out via D2, so `.agents/skills/**/SKILL.md` stays out
with no opt-in needed (24 machine-facing files that would have drowned 30 boards).

Extension filtering happens in the lister, not at the boundary, so a large repo
does not pay a realpath+stat per non-document file it tracks.

`.makit/docs.json` `roots` inverts meaning: the index is broad by default, so
naming roots is now how a project narrows it. That must beat git's list or there
would be no way to opt out of the breadth — pinned by a test. A worktree git
cannot answer for (not a repository) still falls back to rev 1's walk, so a plain
directory does not silently show zero docs; the pre-existing scan tests now
exercise that path, since their fixtures are not repos.

Rows show the full path, truncated from the LEFT — `…/learning-records/0006.md`
rather than `/Users/le/Work/teachme/flutter/learning-…`. For a root-level file
the old relPath subtitle just repeated the title (`Notes` / `NOTES.md`) and said
nothing about location. The path is wrapped in an LTR isolate (U+2066/U+2069)
because the RTL paragraph that moves the ellipsis to the visual start would
otherwise reorder the leading `/` to the wrong end.

The popover also stopped scaling: it built one row per doc in a Column and grew
to the window's height. It is now a lazy ListView capped near a dozen rows, with
a search field over titles and paths and a real empty state. A fixed itemExtent
was tried and reverted — it overflowed by 5px the moment a row carried an extra
badge, so rows size naturally and only the panel is capped.

1233/1233 server tests, 39 app docs tests, tsc + analyze clean. Verified against
the real repos: teachme 3 → 69, this worktree 132 → 143, no dotfile, .git,
node_modules or build path in either.

* docs(mockups): the docs popover searches, scrolls and caps (rev 2)

The board drew the popover as "3 recent" with an "All 132 docs…" escape hatch to
the full screen. That design assumed a small index. D1 rev 2 broke the
assumption: this worktree now indexes 143 documents and teachme 69, and the three
most recent are almost never the one you are reaching for — so the popover became
a dead end for its own primary job.

Redrawn: a search field over titles and paths, a list capped near a dozen rows
that scrolls, a `12 of 143` count while filtering (so an empty result reads as a
filter, not an empty repo), and worktree-relative paths truncated from the left —
the header already names the worktree, and the filename is the part being read.
The rationale paragraph records why, including that the global Docs screen keeps
absolute paths because its worktree headers are not sticky.

Numbers are this worktree's throughout the popover; a first pass mixed teachme's
69 into a frame labelled feat/serving-html.

Not touched: the iPhone frame in Card 2 still reads "132 files · 3 worktrees" and
"All 132 / Mockups 27 / Specs 70". Those predate rev 2 and are now understated.

* fix(qa): unwrap the grant object from docs.publish ack

* test(docs): pin the docs.publish ack shape that broke HTML preview

c10fbb8 fixed the unwrap but left the hole that produced it: the shape was
asserted nowhere. This is the second wire-shape mismatch on this feature to reach
a real device with both suites green, and the mechanism is the same each time —
the two sides are tested against different fakes:

  server test  reads ack.grant.grantId          (nested — correct)
  app test     built a DocGrant directly        (never parsed an ack at all)

So nothing failed until a real publish returned "an unusable grant".

`DocGrant.fromAck` now owns the nesting, tested against the actual frame: the
nested shape parses, a flat ack is refused rather than half-parsed, and a missing
or malformed grant is refused. store.dart delegates instead of unwrapping inline,
so there is one place to be wrong.

fromAck also widens the type check to `is! Map` + `Map<String,dynamic>.from(...)`
rather than `is! Map<String, dynamic>`, matching the tolerant-parsing convention
in ports.dart — a codec that hands back Map<dynamic,dynamic> would otherwise be
rejected for the wrong reason.

21/21 store docs tests, analyze clean.

* docs(spec): D8 rev 2 — where the viewer is decides how HTML opens

rev 1 said "in P1, HTML is reachable only by tailnet publish". That forced the
same-machine case through a network round trip it does not need: with Tailscale
off, a board sitting on the very machine you were looking at could not be opened,
and the failure read `Could not publish`. The document lives on the server's
filesystem, so the only real question is whether the viewer is on that host:

  local client (loopback)  ->  docs.open, the host's OS opener. No HTTP, no
                               grant, no TTL, no Tailscale, no listener.
  remote client (tailnet)  ->  publish and serve, exactly as D9/D10/D15 say.

Per **client**, not per server — one server holds a loopback desktop app and a
tailnet phone at the same moment, and `isLocal` is already computed per client
from the remote address in server.ts. Publish stays available to a local client as
a secondary action, since that is how a board gets to the phone from the desk.

Opening on the host is a new server capability, so it is gated to local clients
and still routed through D2's single path boundary: a remote client asking for
`docs.open` is refused, never served.

The board had drawn `Open · Browser · Reveal` on the popover row from the start;
rev 1 shipped one of the three. Card 6 now leads with an "open on the host" row,
states the resolution as a where-the-viewer-is question, and draws the same sheet
with its two primary actions side by side.

Also corrected while in here:
- Card 6 still credited `tailscale serve`, which D10 rev 2 dropped.
- The wire table recorded `docs.publish` as returning `DocGrantDTO`; it returns
  `{grant: DocGrantDTO}`, nested. That undocumented nesting is what broke HTML
  preview on a real device (a84fde7).
- Verification step 3 still expected "27 boards and 70 specs" from a
  `find mockups docs` — dead after D1 rev 2. It now uses the git listing the
  index actually uses, and states 143 here / 69 in teachme. Ran it: 143.
- Verification gained the two cases that would have caught this: open with
  Tailscale off must still work, and `docs.open` from the phone must be refused.

No code in this commit. `docs.open` is not implemented yet.

* feat(docs): open HTML on the host when the viewer is already there (D8 rev 2)

Implements the decision recorded in e169f53. A board sitting on the machine you
are looking at is now one tap away with Tailscale off: no HTTP, no grant, no TTL,
no listener. Publishing stays for the case it cannot cover — a different device.

Server
  docs/open.ts       resolve through D2, then the platform opener (open /
                     xdg-open / cmd start ""). The path is an argv element, never
                     concatenated into a command string, so `a b; touch pwned.md`
                     is inert — asserted.
  docs.open          gated on ctx.client.isLocal; a remote client is refused with
                     a reason, never served.
  hello.ack          now carries isLocal, per client.

Reconciled two in-flight designs rather than duplicating them. The command-layer
gate was already right and is kept as-is. Two things changed underneath it:

- DocsService.open delegated to an optional `deps.open` that server.ts never
  passed, so it returned false for every request — the feature could not work.
  It now calls openDocOnHost, which is wired and tested.
- The port was sync `boolean`, so every failure surfaced as "could not open
  document". It is now async and reason-carrying, matching what publish already
  does (D15's degrade-loudly rule): a refused dotfile says "dotfile".

App
  MakitConnState.serverIsLocal, set from hello.ack before fan-out. NOT inferred
  from the stored host: mDNS rediscovery rewrites that behind us, and a
  loopback-looking host is not proof. Absent (older server) reads as remote,
  because publishing works everywhere and the fallback must be the one that
  cannot be wrong.
  The HTML notice leads with "Open in browser" for a local client and demotes
  publish to "Share to a device…" — still reachable, since that is how a board
  gets to the phone from the desk. A remote client is unchanged.

Tests: opener platform matrix, the six paths D2 refuses (and that none of them
spawn), argv-not-shell, a failing opener's reason; the local/remote command gate
and the stated reason; hello.ack carrying isLocal both ways; serverIsLocal
defaulting false, set by isLocal:true, and treating an absent field as remote;
both notice variants including that opening locally publishes nothing.

1242/1242 server, 88 app tests here, tsc + analyze clean.

Not done: Reveal in Finder (the mockup's third action), and no device check yet —
the app binary needs a rebuild before this is visible.

* fix(qa): three high-severity bugs from ocr review

- serverIsLocal not reset on connection start, persisting stale state across server switches
- FutureBuilder error case renders infinite spinner instead of showing reason
- ListView.builder rows share keys on filter, breaking widget state on search

* fix(security): use powershell Start-Process instead of cmd /c start on Windows

Windows cmd /c uses argument concatenation that can be escaped if the path
contains unescaped quotes. Switch to powershell -Command Start-Process,
which parses arguments as script block expressions and is resistant to
quote-injection. Also add -FilePath as a named arg to be explicit about
what we are opening.

* refactor(docs): fix 13 ocr audit findings, restructure scan and listener

* fix(docs): the bugs the ocr review found, with tests that bite

Six defects, five of them mine from the last two commits.

app — the popover crashed on a short surface
  `(rows * rowHeight).clamp(0.0, maxHeight - 96)` throws when maxHeight < 96,
  because Dart's clamp asserts lower <= upper. A 180pt-tall window gives the
  panel ~90pt, so it threw ArgumentError instead of rendering. Fixed by deleting
  the arithmetic: the list is `Flexible` inside a `ConstrainedBox`, so the cap is
  a maximum and the constraint system squeezes it. `kDocsPopoverChromeHeight` is
  gone with it.
  The first attempt (max/min instead of clamp) only moved the failure to a
  RenderFlex overflow — hand-computing the height was the wrong shape, not the
  wrong formula. Test pinned at 180pt, where the old expression provably throws
  (verified by evaluating it standalone) and the new code renders.

server — isLocal reached two of three hello.ack sends
  `handlePair` was missed, so a device that pairs never learned it was local and
  silently published instead of opening: the feature quietly off for exactly the
  clients most likely to be on the same machine. Test asserts all three paths.

server — Windows was a command-injection hole, twice
  `cmd /c start ""` re-parses `& | > < ^`. The follow-up, `powershell -Command
  Start-Process`, is worse: `-Command` *joins* its remaining arguments into a
  script, so a path containing `;` or `$(...)` is evaluated. Both contradicted
  the "argv, never a shell" claim the module is built on, and the test only ever
  exercised darwin.
  Windows is not a target for makit's server, so it is now refused with a reason.
  That deletes the injection surface, makes the previously-dead `undefined` branch
  reachable, and drops the `platform === "win32"` special case at the call site.
  A test asserts win32 is refused and spawns nothing, so re-adding a shell path
  fails.

server — two publish-path races
  `ensureOrigin` was unguarded, so two concurrent publishes each bound a port and
  the first listener leaked unreachable. It now coalesces on one in-flight bind.
  `close()` cleared its fields before the socket finished closing, so a publish
  arriving mid-close bound a second listener; it now awaits the in-flight bind.
  A throwing `reach()` escaped as an unhandled rejection instead of D15's stated
  reason. Tests for the shared bind and the stated reason.

server — a stat per doc, twice
  `resolveDocPath` already stat'd the file to validate it, then the caller stat'd
  again for the mtime. `DocPathResult` now carries `modifiedAt`, halving the
  syscalls per scanned document (286 -> 143 here).

1246/1246 server, tsc clean. App: 2061 pass, analyze clean; the 19 "loading"
failures are the known flutter_tester flake — all pass in isolation.

* fix(docs): defensive error handling in scan loop

* docs: SPEC-46 P1 branch summary for review

* fix(docs): add the ocr findings that didn't make the previous commit

* fix(docs): five medium-severity audit findings from ocr review

- docs.grants now evicts least-recently-used grants when the live set hits the
  cap (DocsGrantStore), preventing unbounded memory growth on long-lived servers
  (medium/bug: grant set memory leak).
- publishDoc now updates the grant's nowMs before every tick, not once at sheet
  open, so the 'expires in 5m' countdown stays accurate if the user leaves the
  sheet open (medium/maintainability: stale timestamp in UI).
- docs.grants filters responses to tolerate server bugs: a mismatch between
  server and client (DTO shape, types) no longer crashes the app (medium/bug:
  unguarded cast).
- docs screen's filter count now recomputes on every keystroke in the search
  field, not once at filter change, so narrowing a query updates the counts
  (medium/maintainability: UI lag on search).
- the service now catches scan errors and publishes scanOk:false (instead of
  rejecting the promise), so a slow worktree filesystem does not crash the
  server's snapshot loop (medium/bug: error handling).
- service.ts run(git) now uses a timeout so a stalled git process cannot starve
  the index. Tracked fall through to walk when git times out (medium/bug:
  potential hang).

* fix(docs): the medium-severity ocr findings worth fixing

Verified each before touching it — one was a false positive.

server
  service.ts   `void runScan()` had no catch, so a throwing scan escaped as an
               unhandled rejection: nothing published, nothing logged, the Docs
               screen simply frozen on its last snapshot. It now publishes
               scanOk:false with the reason, which is what that flag is for.
  changed.ts   the merge-base diff had no timeout, the same hang risk already
               fixed in tracked.ts. Bounded at 10s; past it `changed` is
               undetermined, which D14 already models as absent-not-guessed.
  grants.ts    reaping was lazy (inside resolve/list), so a publish loop could
               grow the map without bound between reads — each entry pinning a
               path and holding the doc listener open. mint() now reaps first and
               caps at 64 live grants, evicting the least recently seen. The reap
               loop list() already ran is extracted and shared.

app
  publish_sheet  `nowMs` was read once at the build that first showed the grant,
                 so the expiry pill read "30 min" forever. A 30s ticker while a
                 grant is live, cancelled in dispose.
  docs_screen    docsFilterCounts walks every doc and does not depend on the
                 query, but ran on every rebuild — each keystroke was O(docs).
                 Memoised against snapshot identity.
  store.dart     `ack['grants'] as List?` threw a cast error into the caller's
                 Future on a malformed payload; now tolerant, per ports.dart.

NOT fixed — false positive: the review claimed both MarkdownBody widgets lack an
`extensionSet` and so would not render GFM tables. flutter_markdown_plus renders
them by default; verified with a throwaway widget test (Table present, no literal
pipes). Left alone.

Also: Card 2's iPhone frame still carried pre-rev-2 numbers (132 files / 3
worktrees / All 132 / Mockups 27 / Specs 70). Refreshed to the real counts.

Two tests I wrote and then deleted rather than ship: a "grants payload tolerance"
test that re-implemented the expression inline (it could not fail if the
production code regressed) and, earlier, a duplicate auth_gate test misnamed as a
live daemon probe.

1248/1248 server, tsc clean. App analyze clean; 2079 pass, the 11 "loading"
failures are the flutter_tester flake with zero non-loading failures.

* fix(test): re-sync the shared snapshots fixture after the main merge

CI's fixture-sync job requires app/test/fixtures/snapshots.json and
server/test/fixtures/snapshots.json to be byte-identical. The main merge resolved
the two copies differently: I resolved the server one semantically (main's
rewritten ports.snapshot plus our docs.snapshot frame), while the app copy took
main's side and lost the docs frame.

Frames 1-3 were already identical, so the app copy did carry main's updated
ports.snapshot — only our docs.snapshot was missing. Copied server -> app.

Caught by CI, not by either local suite: the server tests read the server copy
and the app's codec_contract_test read the app copy, so each was internally
consistent and neither noticed the divergence. That asymmetry is exactly what the
fixture-sync job exists to catch.

app/test/codec_contract_test.dart 12/12, server 1414/1414.

* fix(docs): migrate from snackbar to StatusCenter (SPEC-48)

* fix(qa): route doc link copy through StatusCenter, not showSnackBar (SPEC-48)

* fix(docs): address CodeRabbit review on SPEC-46 P1

53 review threads, verified individually rather than applied wholesale.

Availability / correctness:
- listener: persistent 'error' handler after bind (an EMFILE later would have
  crashed the process); bind is serialised against an in-flight close.
- route: destroy the socket when the catch fires after headers were sent,
  instead of leaving the response to hang until the headers timeout.
- roots: bound the user-supplied .makit/docs.json read at 64 KB.
- scan: yield to the event loop every 32 candidates so a large repo's
  per-file sync work no longer stalls the watchers and the WSS.
- changed: -c core.quotePath=false so non-ASCII paths match the ls-files list.
- service: a worktree change arriving mid-walk is queued and re-run once
  instead of being dropped; a walk that fails after the last watcher leaves
  no longer overwrites a good cache; per-worktree scan + merge-base diff fan
  out concurrently.

Scoping (SPEC-44 owner model) — docs commands constrained against
server-authoritative state, not merely type-checked:
- read/publish/open refuse a worktreePath the index never reported;
- grants are minted with the caller's deviceId, docs.grants returns only the
  caller's shares, and docs.unpublish refuses a foreign id indistinguishably
  from an unknown one, so it cannot probe another device's grant ids.

App:
- publish_sheet: a grant that lands after the sheet is dismissed is revoked,
  so a doc cannot stay shared the user believes they cancelled.
- desktop_sidebar: clear the host's docs/ports popover latch when the glyph
  unmounts, so the overflow menu no longer sticks in place of the diff pill.
- doc_preview: an external link that fails to open now reports through
  StatusCenter instead of failing silently.
- docs_screen: lazy ListView.builder + memoised subtitle.

Tests: extracted server/test/ws/docs_deps_stub.ts so a new DocsCommandPort
member breaks one place instead of four (adding isIndexedWorktree had broken
all four inline copies). Plus coverage for the paths above, and the restored
SPEC-42 P2c docker/reach contract test the branch had dropped.

BRANCH_SUMMARY.md is removed — branch-scoped status belongs in the PR body,
not the tree.

Verified: server pnpm typecheck clean, 1465 tests pass; app flutter analyze
clean, 2667 tests pass. Key fixes mutation-tested (latch, scoping, stub).

* fix(docs): the published URL is plain HTTP on the tailnet IP, not https

CodeRabbit caught the mockup's capability URL showing an `https://` MagicDNS
origin while the same card's prose (and D10 rev 2) says the listener is plain
HTTP. `DocListener.bindOnce` builds `http://${bindHost}:${port}`, so the example
was wrong on both the scheme and the host form.

Fixed the mockup, and the same drift in the shipped `DocReach.origin` doc
comment, which still offered `https://host.ts.net` as an example P1 can never
produce. The `https://host.ts.net` test fixtures are normalised to the real
`http://<tailnet-ip>:<port>` shape for the same reason: fixtures get read as the
canonical shape of a value.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants