Skip to content

feat(tui): add Stop daemon action to Settings dialog (F1)#34

Merged
artyomsv merged 4 commits into
masterfrom
feature/settings-stop-daemon
Jun 5, 2026
Merged

feat(tui): add Stop daemon action to Settings dialog (F1)#34
artyomsv merged 4 commits into
masterfrom
feature/settings-stop-daemon

Conversation

@artyomsv

@artyomsv artyomsv commented Jun 5, 2026

Copy link
Copy Markdown
Owner

Summary

Adds a "Stop daemon" action row to the Settings dialog (F1 → Settings → ↓ to last row). Pressing y on the confirmation cleanly stops the daemon and quits the TUI in one step — the daemon's stop defers (final snapshot write, PID file removal, log close) all run before the TUI exits, so panes respawn cleanly from the snapshot on next launch.

Closes the operator gap where stopping the daemon from inside Quil meant either (a) opening a fresh terminal to run quil daemon stop, or (b) finding the PID and killing it manually.

UX

F1 → Settings → ↓×7 to "Stop daemon"
> Stop daemon              (closes this TUI window)

Enter → confirmation dialog:

  Stop the daemon?

  This TUI window will close.
  Panes will respawn from the snapshot on next launch.

  y confirm    Esc cancel
  • y accepts → MsgShutdowntea.Quit, all in the same handler return
  • Enter is a deliberate no-op for this confirm. Enter is the universal Settings-toggle commit key; reserving it from the shutdown confirm prevents finger-memory misclicks from killing the daemon and every pane child.
  • Esc returns to Settings with the cursor restored to the Stop daemon row (via label lookup, not positional).

Implementation

  • settingsField.action func(Model) (Model, tea.Cmd) — new optional field. When set, Enter on Settings calls the action instead of opening the inline editor. The existing get/set/isBool wiring is untouched for the other seven config rows.
  • confirmKindShutdown — typed const so the handler and renderer cannot drift on a typo.
  • Synchronous Send + tea.Quit — the IPC MsgShutdown send is inline in handleConfirmKey. By the time the runtime sees tea.Quit, the daemon has the message. This deliberately rejects the original tea.Batch(send, tea.Quit) design because Batch runs commands concurrently with no ordering guarantee — under Batch, main.go's defer client.Close() could race the in-flight send and the daemon could miss the shutdown message. The IPC client is a ~150-byte write to a local Unix socket, so inlining has no UI-thread concern.
  • tuiClient interfaceModel.client was *ipc.Client (concrete). Extracted a one-package interface (Send + Receive) at the consumer so the new Enter-path tests can inject a fakeSender. *ipc.Client satisfies the interface implicitly, so the assignment in NewModel and every existing call site is unchanged.
  • Width bumpdialogWidth 50 → 60. The "(closes this TUI window)" description wrapped in the prior width; the confirm body line "Panes will respawn from the snapshot on next launch." (~52 chars) wrapped too. Matched to the existing disclaimerWidth = 60.

Files

File Change
internal/tui/dialog.go settingsField struct + Stop daemon row + handleSettingsKey action dispatch + handleConfirmKey shutdown branches (Esc + y) + renderConfirmDialog per-kind footer + stopDaemonRowIndex() + confirmKindShutdown const + dialogWidth 50→60
internal/tui/model.go tuiClient interface; Model.client field type lift from *ipc.Client to tuiClient
internal/tui/dialog_test.go 9 tests: field-shape guard, Enter-routing, Esc-returns-to-Settings, render-message-contents (positive + negative for "y confirm"), Enter-is-no-op, y-sends-and-quits (with fakeSender), y-with-send-error-still-quits, y-with-nil-client-still-quits, plus the existing tests with t.Parallel() added
internal/claudehook/claudehook.go One-line backport: cleanup-comment improvement on os.Remove discard (consistency win, no behaviour change)
.claude/CLAUDE.md Settings dialog persistence section updated to document the action-row mechanism, the y-required confirm, and the synchronous-Send rationale
CHANGELOG.md [Unreleased] / Added entry

Daemon-side: zero changes. Existing MsgShutdown handler at internal/daemon/daemon.go already writes the final snapshot via stop defers.

Hardening applied during review

A /code-review pass on the initial commit surfaced 6 medium + 14 low findings. Every actionable item is addressed:

  • Criticaltea.Batch race fixed by inlining the Send
  • Critical — Enter-path coverage gap filled by the tuiClient interface + 3 new tests
  • Securityy required instead of Enter (prevents finger-memory misclicks)
  • Code quality — Esc cursor restore is now stopDaemonRowIndex() (label-lookup, not positional)
  • Rulesreq, _ := ipc.NewMessage(...) matches the file's structural convention
  • Rulest.Parallel() added to all tests in dialog_test.go
  • Rules — Bespoke contains test helper dropped in favour of strings.Contains
  • Test honestystop.get(&Model{cfg: config.Default()}) instead of stop.get(nil)
  • Negative assertion — render test now also fails if "Enter confirm" reappears in the shutdown confirm footer

Test plan

  • go vet ./... — clean
  • go test ./... (full suite, 17 packages) — green
  • go test -race on internal/tui/ and internal/ipc/ — clean
  • GOOS=windows go build ./... — clean (confirms the tuiClient interface lift didn't break Windows)
  • Daemon-lifecycle smoke (native arm64 dev build):
    • ./quild-dev --background starts cleanly
    • ./quil-dev daemon stop (same MsgShutdown wire the new F1 action uses) → daemon log: ipc recv: shutdownshutting down (IPC) → final snapshot written → pane children reaped
    • PID file + socket cleaned up
    • Workspace snapshot persisted on shutdown

The interactive TUI smoke (F1 → ↓×7 → Enter → confirm "y confirm" footer → press Enter, observe no-op → press y, daemon stops, TUI exits) was verified manually on macOS arm64.

Caveats / deferred

  • confirmKind asymmetry — promoting the other three string discriminators ("pane", "tab", "instance") to typed consts touches multiple files outside this feature; deferred to a follow-up
  • dialog.go size (2511 lines) — tech debt noted; split-by-dialog-type refactor would land once a techdebt/ directory is established
  • settingsField sum-type split (separate editor vs action variants) — over-engineering for two action rows today; re-evaluate when a third appears

artyomsv and others added 3 commits June 5, 2026 11:23
F1 → Settings now ends with a "Stop daemon" entry that opens a
confirmation explaining the TUI window will close and that panes will
respawn from the snapshot on next launch. Enter on the confirm fires
MsgShutdown over the existing IPC client and tea.Quit's the TUI in the
same tea.Batch, so the daemon's stop defers (final snapshot write, PID
file removal, log close) all run before the TUI exits.

Implemented as a non-config "action row" via a new optional
settingsField.action func(Model) (Model, tea.Cmd) — when set, Enter
calls the action instead of opening the inline editor. The existing
get/set/isBool wiring is untouched for the other seven config rows.

UX details:
  * Description text "(closes this TUI window)" renders in the value
    column so the consequence is visible before pressing Enter
  * Esc on the shutdown confirm returns to Settings with the cursor
    restored to the Stop daemon row (not dialogNone)
  * Send is best-effort — a stale socket logs but does not block the
    TUI's quit, matching the operator intent that "I asked to stop"
    results in the TUI exiting either way

Tests (5 new, all package internal/tui):
  * TestSettingsFields_StopDaemonIsAction — guards the row's shape
  * TestHandleSettingsKey_StopDaemonOpensConfirm — Enter routes to
    confirm, not the inline editor
  * TestHandleConfirmKey_StopDaemonEscReturnsToSettings — Esc lands
    back on the Stop daemon row, not dialogNone
  * TestRenderConfirmDialog_StopDaemonMessage — locks in the warning
    text ("Stop the daemon?", "TUI window will close")
  * TestSettingsFields_LabelsAndInitialValues extended to require the
    new label at the end of the list

No daemon changes — existing MsgShutdown handler already writes the
final snapshot via the daemon's stop defers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ests

Address all actionable findings from /code-review on the prior commit:

Must-fix (security #1 / code-review #1):
  * Replace tea.Batch(send, tea.Quit) with a SYNCHRONOUS Send + tea.Quit.
    tea.Batch runs concurrently with no ordering guarantee — the daemon
    could miss the MsgShutdown when tea.Quit and main.go's
    `defer client.Close()` raced the in-flight Send. The IPC client is a
    one-shot ~150-byte write to a local Unix socket, so inlining the Send
    has no UI-thread concern and closes the race entirely.

Must-fix (qa coverage gap):
  * Extract a minimal `tuiClient` interface (Send + Receive) at the
    consumer in model.go; change Model.client from *ipc.Client to that
    interface. *ipc.Client satisfies it implicitly so call sites and
    NewModel are unchanged. Added a `fakeSender` test stub that records
    every Send.
  * Three new tests now cover the previously-untested Enter path:
      - TestHandleConfirmKey_StopDaemonYSendsAndQuits asserts that
        MsgShutdown lands on the wire synchronously AND tea.Quit is
        returned.
      - TestHandleConfirmKey_StopDaemonYWithSendErrorStillQuits locks in
        the "fail open" contract: send error → still quit.
      - TestHandleConfirmKey_StopDaemonYWithNilClientStillQuits guards
        the defensive nil-client branch.

Should-fix (security #2 — UX hardening):
  * Require explicit `y` to accept the shutdown confirm. Enter is the
    universal Settings-row commit key, so reserving it from the shutdown
    confirm prevents finger-memory misclicks from killing the daemon and
    every pane child. New test TestHandleConfirmKey_StopDaemonEnterIsNoOp
    locks this in. The confirm dialog's footer now reads
    "y confirm    Esc cancel" so the help line matches the handler.

Should-fix (code-review #2):
  * Esc cursor restore is now `stopDaemonRowIndex()` (label-lookup) so a
    future action row inserted after Stop daemon does not silently
    misplace the cursor onto the new row.

Should-fix (rules #1):
  * Drop the misleading `if err == nil` guard around ipc.NewMessage; use
    `req, _ := ipc.NewMessage(...)` to match every other call site in
    the same file (the structural exception to "never assign to _").

Should-fix (rules #2):
  * Add t.Parallel() to all 9 tests in dialog_test.go (matches the
    package-wide convention in notes_test.go).

Should-fix (rules #3):
  * Replace the bespoke `contains` test helper with strings.Contains.

Other small cleanups:
  * `stop.get(nil)` → `stop.get(&Model{cfg: config.Default()})` so the
    test is honest about the getter's contract.
  * Negative assertion added to the render test: shutdown confirm must
    NOT show "Enter confirm" in its footer.

Verification:
  * go vet — clean
  * go test ./... — 17 packages, all green
  * go test -race on internal/tui/ and internal/ipc/ — clean
  * GOOS=windows go build ./... — clean

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The Settings dialog rendered the new "Stop daemon" row with its
description "(closes this TUI window)" wrapping onto a second line
because the row exactly fills the dialog width:
  cursor (2) + label (24) + value (24) = 50

The shutdown confirm body has the same constraint — the line "Panes will
respawn from the snapshot on next launch." is ~52 chars and was wrapping
inside the 50-wide border too.

Match dialogWidth to disclaimerWidth (60) so:
  * the Stop daemon row fits in one line
  * the shutdown confirm body renders cleanly
  * the visual style is consistent across all fixed-width modal dialogs
  * other dialogs using dialogWidth (About, Shortcuts, non-plugin
    Confirm) gain a few cols of breathing room — their content was
    already short so nothing else changes

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
artyomsv added a commit that referenced this pull request Jun 5, 2026
## Summary

Restructures project documentation so that the GitHub repo browser is
easy to navigate and the headline MCP feature finally has a user-facing
guide.

**Before** — 8 markdown files at the repo root totalling 2735 lines; the
README alone was 307 lines covering Features + Architecture + Quick
start + Configuration + Project Structure + Roadmap. No user-facing MCP
guide existed (only a PRD in `docs/roadmap/mcp-server.md`).

**After** — 4 files at the root (the GitHub-conventional set: README,
CHANGELOG, CONTRIBUTING, LICENSE), everything else under `docs/` as a
navigable tree. README is a 95-line landing page. New `docs/mcp.md`
documents all 17 MCP tools with input schemas pulled directly from
`cmd/quil/mcp_tools.go`, plus client wiring for Claude Desktop, Claude
Code, Cursor, and VS Code Copilot.

## Structure

```
quil/
├── README.md             95 lines — landing page only (was 307)
├── CHANGELOG.md          (unchanged — convention)
├── CONTRIBUTING.md       rewritten as a thin pointer page (was 119 lines of duplicated build info)
├── LICENSE
└── docs/
    ├── README.md         NEW — documentation index, github.com renders it on `docs/` click
    ├── installation.md   NEW — every install path + build-from-source
    ├── quick-start.md    NEW — first-launch walkthrough
    ├── features.md       NEW — capability tour grouped by area
    ├── keybindings.md    NEW — full keymap + customization
    ├── configuration.md  NEW — ~/.quil/config.toml reference
    ├── mcp.md            NEW — 317-line MCP user guide (the headline)
    ├── plugin-reference.md  (existing)
    ├── troubleshooting.md  NEW — daemon won't start, MCP not detected, log locations, reset
    ├── architecture.md   moved from root via git mv (history preserved)
    ├── roadmap.md        moved
    ├── vision.md         moved
    ├── prd.md            moved
    ├── versioning.md     moved
    ├── plans/            (existing)
    ├── roadmap/          (existing)
    └── superpowers/      (existing)
```

## What changed by file

| File | Lines (before → after) | Purpose |
|---|---|---|
| `README.md` | 307 → 95 | Landing page: hero + install (3 paths) +
5-key quick start + MCP wiring snippet + Documentation table |
| `CONTRIBUTING.md` | 119 → 111 | Thin pointer page: branch/commit
conventions + doc-maintenance checklist. Build instructions moved into
`docs/installation.md`. |
| `docs/README.md` | new (48 lines) | Documentation index — what
github.com auto-renders when a visitor clicks the `docs/` folder |
| `docs/installation.md` | new (142 lines) | Linux/macOS one-liner,
Windows zip, Go install, Docker + native build paths, dev/debug
variants, uninstall |
| `docs/quick-start.md` | new (111 lines) | What to do after install — 5
keys, opening a typed pane, automatic persistence, AI assistant
connection |
| `docs/features.md` | new (224 lines) | Feature catalog grouped:
persistence, layout, clipboard, plugins, observability, pane notes,
operations |
| `docs/keybindings.md` | new (143 lines) | Full keymap + customization
syntax, why Tab/Shift+Tab are not bound |
| `docs/configuration.md` | new (153 lines) | Every section + every key
of `~/.quil/config.toml` with defaults and effects |
| `docs/mcp.md` | new (317 lines) | **Headline doc.** Architecture
diagram, wiring snippets for Claude Desktop / Claude Code / Cursor / VS
Code Copilot, all 17 tools with input schemas, redaction model, per-pane
logging, troubleshooting |
| `docs/troubleshooting.md` | new (197 lines) | Daemon won't start,
blank TUI, MCP not detected, version mismatch, log file locations,
force-stop, full reset |
| `ARCHITECTURE.md` → `docs/architecture.md` | moved | `git mv`
preserves history |
| `PRD.md` → `docs/prd.md` | moved | Cross-references inside fixed up |
| `ROADMAP.md` → `docs/roadmap.md` | moved | Internal `docs/roadmap/...`
links rewritten as sibling links |
| `VISION.md` → `docs/vision.md` | moved | |
| `VERSIONING.md` → `docs/versioning.md` | moved | |
| `.claude/CLAUDE.md` | edited | Documents section rewritten to reflect
new tree |
| `site/src/pages/docs.astro` | edited | Reorganised into 5 sections
(Getting started / Using Quil / Authoring / Project context / Future
roadmap) with current paths + 4 new entries |
| `site/src/components/Footer.astro` | edited | Doc links updated; MCP
guide added |
| `docs/plans/2026-03-09-m1-foundation.md` | 1 line | Historical plan's
`Reference:` pointers updated to `../prd.md` and `../vision.md` |

## Why MCP got its own user guide

`quil mcp` is the project's headline AI feature — Quil becomes the
bridge between your AI assistant and your terminal. But until this PR,
the only MCP documentation was `docs/roadmap/mcp-server.md`, which is a
PRD describing the architecture, not a user guide. A user could not
figure out how to wire Quil into Claude Desktop or Cursor without
reading the source code.

`docs/mcp.md` solves that. It contains:

1. A diagram of how `quil mcp` bridges MCP JSON-RPC over stdio to the
daemon IPC over the Unix socket
2. **Copy-pasteable JSON config snippets** for 4 popular MCP clients
(Claude Desktop, Claude Code CLI, Cursor, VS Code Copilot Chat)
3. **All 17 tools documented in a single table per group** (Discovery,
Reading, Interacting, Lifecycle, TUI cooperation, Event observation,
Memory reporting) with input schemas pulled directly from
`cmd/quil/mcp_tools.go` so the docs cannot drift
4. **Example AI prompts** showing real workflows: build monitoring,
cross-pane context, workspace setup, triaging a stuck pane
5. **Security model** — the `<<REDACT>>...<</REDACT>>` marker contract +
the regex-fallback layer (OpenAI keys, GitHub PATs, JWTs, BIP-32
extended keys) that catches secrets the AI forgot to wrap
6. **Per-pane MCP logging** — where the log files live, what gets
logged, the orange border highlight
7. **Troubleshooting** — daemon connect failure, AI client not seeing
the server, send_keys racing the target TUI, where to look in the logs

## Cross-reference verification

A custom relative-link checker was written for the new `docs/` tree:

```bash
for f in docs/*.md; do
  while IFS= read -r link; do
    target=$(echo "$link" | sed -nE 's/.*\]\(([^)#]+)(#[^)]*)?\).*/\1/p')
    case "$target" in http*|/*) continue;; *.md)
      full="$(dirname "$f")/$target"
      [ ! -f "$full" ] && echo "BROKEN in $f: $target"
    esac
  done < <(grep -oE '\]\([^)]+\.md(#[^)]*)?\)' "$f")
done
```

All internal MD links resolve. The recursive grep for stale root-MD path
mentions across the whole repo (excluding historical content under
`docs/plans/`, `docs/roadmap/`, `docs/superpowers/`) returns only:

- Historical CHANGELOG entries that describe past releases (won't touch)
- Intentional "moved from root `X.md`" labels in CLAUDE.md
- One historical PRD note in `docs/roadmap/pane-notes.md` (preserved for
archaeology)

## Verification

- [x] `astro check` + `astro build` against `site/` — **0 errors, 0
warnings**, 11 pages built
- [x] Relative-link checker walks every MD link in `docs/` — all resolve
- [x] `git mv` preserved file history for all 5 moved files (verify with
`git log --follow docs/architecture.md`)
- [x] README went 307 → 95 lines; root `.md` count went 8 → 4

## Independence from other open PRs

This PR is independent of #34 (`feat(tui): add Stop daemon action`).
They touch disjoint files:

- This PR: docs/, README.md, CONTRIBUTING.md, .claude/CLAUDE.md, site/
- #34: internal/tui/, internal/claudehook/

Either can land first; merging order does not require re-basing.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@artyomsv artyomsv merged commit 218ac31 into master Jun 5, 2026
5 checks passed
@artyomsv artyomsv deleted the feature/settings-stop-daemon branch June 5, 2026 12:13
artyomsv added a commit that referenced this pull request Jun 5, 2026
…36)

## Summary

Fixes the false-positive minor bump on PR #35 (docs restructure) that
shipped **v1.13.0 with no user-visible changes**.

The release pipeline was scanning both subject AND body of every commit
since the last tag, hoping to catch type lines preserved in squash
bodies when the subject was branch-name-derived. That heuristic produced
this false positive: the #35 PR description contained the literal
back-tick-wrapped reference

> This PR is independent of #34 (`` `feat(tui): add Stop daemon action`
``).

…and the regex `\bfeat[(:/]` matched the substring `feat(` inside the
backticks just as readily as it would have matched a real `feat(tui):`
commit. The release job classified the PR as `minor` and shipped v1.13.0
with nothing user-facing in it.

## Fix

| What | Before | After |
|---|---|---|
| Type classification (feat/fix/perf/bang) | scans `%s%n%b` (subject +
body) | scans `%s` (subject only) — the merger sets the squash subject
explicitly and it IS the authoritative classification |
| `BREAKING CHANGE` detection | matched anywhere in subject+body via
`BREAKING CHANGE` | matches `^BREAKING[ -]CHANGE:` in body **anchored to
line start with trailing colon** — the Conventional Commits footer form.
Prose mentions like "this is not a BREAKING CHANGE" no longer match. |
| Branch-name fallback (`feat/foo-bar`) | covered via body scan | still
covered — the existing `[(:/]` regex on subjects accepts `feat/foo-bar`
form natively |

Diff is ~30 lines, all in `.github/workflows/release.yml`.

## Why this never bit before

The body-scan was a defense-in-depth measure for branch-name-derived
squash subjects. In practice, every merge in this repo's history has had
a properly conventional subject set by the merger, so the body scan
never had to do its intended job — it just sat there waiting to
false-positive. PR #35 was the first PR whose description text contained
a conventional-commit-style reference to another PR, and that's when the
bug surfaced.

## Self-test

Smoke-tested the new regex on the prose vs. footer distinction:

```
$ echo "BREAKING CHANGE: yes" | grep -qE '^BREAKING[ -]CHANGE:'
→ match (correct — real footer)

$ echo "no BREAKING CHANGE here" | grep -qE '^BREAKING[ -]CHANGE:'
→ no match (correct — prose mention)

$ echo "feat(api): add foo" | grep -qiE '\bfeat[(:/]'
→ match (correct — real subject)

$ echo "ref to feat(api)" | grep -qiE '\bfeat[(:/]'
→ matches the regex but the body is no longer scanned for feat —
  irrelevant to the bump path now
```

## Why this PR doesn't bump

The PR is committed as `chore(ci):`, which falls through to the "skip
release" branch in the workflow itself. No `v1.14.1` release will be cut
by merging this; the next user-visible feature PR will bump as normal.

## Test plan

- [x] Inspected the PR #35 squash body to confirm the matching string
(`` `feat(tui)` `` inside backticks)
- [x] Verified the new `^BREAKING[ -]CHANGE:` anchor matches the
Conventional Commits footer form but not casual prose
- [x] Confirmed `chore(ci):` does not match the bump regexes — this PR's
own merge will skip the release job (no v1.14.1 will ship)
- [x] Diff is small and reviewable (~30 lines, single file)
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.

1 participant