Commit ef10fd1
authored
SPEC-46: the CLI is a client — sessions from the terminal, handoff as a command (#159)
* docs(spec): SPEC-46 — the CLI is a client, and handoff is a command
The CLI can inspect makit and attach to one session, but not start work:
every session is born in the app, so the handoff gesture (summarise, spawn,
pick harness, paste) has no automation path even though every step already
has a WSS handler.
Specs the verb set (ls/new/send/tail/wait/resume/rm/handoff), one transport
(WSS; the frozen control socket stays lifecycle-only), a CLI-owned
capability-scoped device instead of borrowing devices.json[0], and
MAKIT_SESSION_ID/MAKIT_CLI_TOKEN in startOpts so an agent can drive makit
from inside its own session — bounded server-side by spawn depth/fan-out.
* docs(spec): SPEC-46 D13/D14 — route a stranded prompt up the lineage, and caption it
Resolves the spec's one blocking open question. askDevice's
subscribed.has(sessionId) filter assumes every session was born on a screen,
which is the assumption agent-side spawning breaks: the child's first
permission request finds no subscriber, so it is either a context-free push
or rejected outright ('no subscribed clients to ask') — a handoff dead on
arrival.
Audience resolution becomes: own subscribers → nearest ancestor's subscribers
via parentId → all authed clients → wake push. Same rule for a tool
permission and for an elicitation, since no policy can answer the latter.
Auto-approving agent-spawned sessions was considered and rejected: it grants
unsupervised shell access in a worktree that may hold credentials, and fixes
only half the request kinds. --yolo stays opt-in per handoff.
D14 makes the fallback tolerable: a prompt for a session the client never
opened must name it.
* docs(spec): SPEC-46 D15 — makit new owns a worktree, handoff inherits one
Resolves the worktree open question. Session-owns-a-worktree is the invariant
the rest of makit assumes: tab groups are keyed by worktree, ports are
attributed per branch, and Wrap up means remove-tree/delete-branch/ff-base.
Dropping a CLI agent into the user's checkout mints sessions that cannot be
wrapped up and whose diff is tangled with the user's own edits.
So makit new always creates one (--here opts out), and with -m the message
seeds the branch name via worktree.create's existing slugify/unique path —
something the app cannot do, since its gesture order is worktree-first.
handoff inverts it: the child inherits the parent's tree, because the
manifest's file:line refs and the uncommitted work being handed over live
there. That forces the parent-lifecycle question (two agents, one tree),
which is now the spec's first open question.
* docs(spec): SPEC-46 D16 — parallel agents may share a worktree, unguarded
A handoff leaves the parent live: the parent is often still finishing
something useful, and 'also work on this' is a normal handoff, so an
archive-the-parent default would destroy work to prevent a problem the user
wants. No refusal, no warning; ls showing branch/worktree is the whole
mitigation.
The one hazard that would have justified a guard is already handled —
removeWorktree reconciles EVERY session bound to the tree (live -> archive,
never kill), so Wrap up cannot delete a tree from under a co-tenant. That
existing behaviour is now load-bearing, so it gets a regression lock in the
acceptance criteria.
Accepted unmitigated: index contention, two agents one file, and SPEC-41/42
port attribution being per-worktree (it cannot name which co-tenant opened a
port). Also escapes raw pipes that were splitting three table rows.
* docs(spec): SPEC-46 rev 2 — apply dual codex review, full scope kept
Two review passes (FLAWED / RESCOPE) falsified six claims; each was re-checked
against the code before acceptance:
- startOpts already injects MAKIT_SESSION_ID (manager.ts:1282), so D3 only adds
project/worktree/depth + the scoped token.
- Nothing emits status:"error" — adapters emit session.error then settle idle,
so D8's exit 20 keys off the event.
- sub {fromSeq} cannot bound a tail (no limit, no published latest seq, and
session.events hydrates the whole log), so --carry gets a bounded
session.transcript command instead.
- PortDTO.sessionId does attribute a co-tenant's port; D16's risk list was
wrong about it.
- Promotion fires on any string, "" included — 'substantive' was a code
comment's word, not behaviour.
- The stub emits no awaiting-* and no error status, so proving 10/11/20 means
extending it.
Three real defects fixed: parentId is now derived from the credential (a wire
parentId let a spawn bearer forge shallow ancestry, foreign parents and
cycles); D13 now stores audience eligibility on the pending request and
authorizes srv.response (replayPendingTo ignored eligibility and the first
answer won unchecked, so an agent's own token could approve another session's
tool call); wait is edge-triggered.
caps enforcement was promoted from open question to D17 — four decisions rested
on a connection principal that does not exist. Also records the SQLite/Flutter
DTO migration work, the pi-acp env spike, and the P1/P2 app-work incoherence.
Scope kept at the requester's direction: the rescope-to-handoff-only,
cut-the-manifest and cut---carry recommendations are recorded as rejected with
reasons in the new Review findings applied section.
* docs(spec): SPEC-46 rev 3 + implementation plan
The D3 spike ran: MAKIT_CLI_TOKEN reaches pi, not just pi-acp. pi-acp's
PiRpcProcess.spawn passes env: process.env (dist/index.js:137), a shim
substituted via PI_ACP_PI_COMMAND received the token intact, and a live pi
agent under pi-acp has MAKIT_SESSION_ID today. Rev 2's premise was wrong: env
travels by inheritance, extensions by argv, so the fixed argv that drops
SpawnOpts.extensions is not the same seam. Constraint found: env is a
spawn-time snapshot, so a token cannot be rotated without an adapter restart.
D17 gained a fanout gate. fanout() sends every session's events to every
authed client, ignoring subscription by design (subscription_hub.ts:76), so a
router-only capability check would let an agent-scoped token read every
transcript on the machine -- the same class of leak as the srv.response hole
the rev-2 review found, and for the same reason: neither path is a command.
The plan pins four contracts the spec left open: the capability map (default
deny + a completeness test over router.kinds()), the agent token store
(in-memory, never devices.json, which persists and would outlive its session),
session.transcript's shape, and exit 3-vs-4 via the existing requireDaemon()
control-socket probe.
* feat(protocol): SPEC-46 shared contract — lineage, caps, principal, cli.grant
The vocabulary every SPEC-46 workstream depends on, landed in one commit so
parallel work cannot drift on it:
- SessionDTO/SessionMeta gain parentId/handoffReason/origin (D10). The codec
needed no change -- DTO fields pass through -- and the snapshots fixture now
carries them, so the existing round-trip test covers them.
- DeviceCap + PairedDevice.caps (D2). Absent means FULL access: every phone
paired before SPEC-46 must keep working, so the absence is load-bearing.
- Principal (D17) on WsClient, with isFullAccess/hasCap/isAgentScoped. An
explicitly EMPTY caps array is a revocation, NOT full access -- the tempting
`!caps?.length` reading would turn a stripped credential into an
unrestricted one, so the contract test pins both edges.
- cli.grant + CliGrantData on the control socket (D2). Additive, which the
frozen contract permits; the test asserts the nine v1 verbs are untouched and
that exactly one was added, so a session verb cannot quietly land here
instead of on WSS (D1).
- CmdKind documents session.transcript (D5/C3).
No behaviour yet: nothing reads the principal and nothing mints a cap. Those
are the enforcement tasks, and they are what the parallel agents implement.
* test(stub): SPEC-46 T15 — the stub can be blocked, and can fail
`makit wait` promises distinct exit codes for finished (0), blocked on you
(10/11) and failed (20), and none of the last three could be proven: the stub
only ever went running -> idle, had no awaiting-* path at all, and its two
session.error emissions were internal bridge faults a test could not ask for.
Adds three triggers -- AWAIT_APPROVAL, AWAIT_INPUT, FAIL_TURN -- through the
shared TurnStatusTracker rather than hand-rolled emits. That is the point of
that class: the coarse status channel is typed "idle" | "running", so a gate is
a session.status EVENT, and having two spellings of it is what drifted acp and
codex into stuck-spinner bugs.
Two details are load-bearing, and each has a test that bites (proven by
mutation):
- the gate never clears itself. `wait --for approval` exiting 10 only means
something if the gate persists; a self-clearing gate passes for the wrong
reason.
- FAIL_TURN settles IDLE, not "error". Nothing in makit emits status "error",
which is exactly why wait keys exit 20 off the event -- so the stub is now
pinned against quietly acquiring an "error" status and invalidating that.
cancel() releases a parked gate by dropping the turn first, then the gate, then
settling: leaveApproval() alone would emit a spurious "running", and leaving
the gate counted would wedge the session forever (settleIdle could never fire).
* feat(storage): SPEC-46 T9 — persist + project session lineage
D10 makes lineage protocol data, not CLI bookkeeping: without it the phone
shows mystery sessions appearing with no explanation, and D9's depth/fan-out
guard has nothing to count. This lands the storage + server-DTO half; nothing
sets the fields yet (that is T10), so the tests set them directly.
SqliteEventStore gains parent_id/handoff_reason/origin via the same in-place
ALTER TABLE pattern the five prior nullable columns use — no table rebuild, no
data loss. The migration is load-bearing: a row written before SPEC-46 must
rehydrate with all three undefined, never null and never a throw, so the test
seeds a real pre-SPEC-46 schema (a sessions table without the new columns),
inserts a row, then lets migrate() run and reads it back.
Session carries the three fields as immutable (set once at construction,
restored on rehydration), persists them through toMeta, and emits them on
toDTO the same way branch/worktreePath flow meta → session → DTO. Absent
lineage stays undefined end-to-end.
* feat(app): SPEC-46 T9 — parse session lineage on the Flutter DTO
The Flutter Session DTO is hand-maintained, so it drifts from the wire the
moment a field lands server-side. Add parentId/handoffReason/origin as String?
(matching the nullable-metadata style of branch/worktreePath/pendingAgent, not
the always-present status/policy enums), so the app can later caption
"handed off from …".
Every field defaults to null when absent: the server will happily send
sessions with no lineage — every session created before this feature — and a
phone that threw on a missing key would crash on the user's device. The codec
parses each only when it is a String, so an unknown or malformed value degrades
to null rather than failing the whole snapshot.
Wire parsing lives in transport/codec.dart's decodeSessions (the real
fromJson seam), with the field + copyWith declarations on models.dart's Session.
* pi-agent: Lineage persistence and Flutter DTO
* feat(ws): SPEC-46 T1 — cli.grant mints a revocable CLI device
The CLI becomes a WSS client (D1), so it needs its own credential rather than
borrowing devices.json[0]. `grantCli()` mints a `cli@<hostname>` device with
caps ["client"] — a subject the user can revoke without unpairing their phone,
which is the whole point of D2. It is idempotent by label: a CLI that lost its
~/.makit/cli.json cache must not mint a second row, or `makit devices` (which
D2 exists to make truthful) fills with duplicates.
The verb rides the control socket, not a QR pair token: a process that can
write ~/.makit/control.sock is already the local user, so demanding a phone
camera to authorise a local terminal would be theatre (D2). The dispatch case
is additive — the frozen v1 verb list is untouched.
Regression pinned: a device with no caps still authenticates, so every phone
paired before SPEC-46 keeps working — absence of caps is full access, never a
denial.
* chore: drop a leaked app/.dart_tool symlink
A subagent created it as a build-environment shim in its isolated worktree and
it rode along in the merge. It is a machine-local symlink into another
checkout's .dart_tool -- committing it would break every other clone.
* feat(ws): SPEC-46 T2 — resolve a principal at hello
Before SPEC-46 an authed socket carried only {deviceId, label} and every socket
could do everything, because the only clients were the user's own devices. D17
needs a subject on the connection so the router and the fanout gate have
something to check. AuthGate now widens AuthRegistry.authenticate to carry the
device's caps and builds a Principal onto the client — caps passed through
verbatim, so an existing phone (no caps) resolves to undefined, which
isFullAccess reads as full access. No enforcement yet; this task only makes the
subject exist.
The handshake seam also gains an optional agent-token consult after the
registry (the C2 store, wired in T3): a bearer the registry does not know is
tried against the per-session store, yielding a session-scoped principal. An
unknown bearer still closes 4401 exactly as before — widening the success path
must not widen the failure path.
* feat(ws): SPEC-46 T3 — per-session agent token, in-memory by construction
D3 puts a credential in each agent's environment so it can drive its own
children. That credential must die with its session — an agent token that
outlived its session would authorise "spawn as a session that no longer
exists". The obvious place, devices.json, is exactly wrong: the registry
persists on every mutation, so a token written there survives a crash as a live
credential and pollutes `makit devices`, which D2 exists to make truthful.
SessionTokenStore is therefore in-memory. That makes D3's "rejected once its
session ends" true by construction: a restart has no sessions, so it has no
tokens. AuthGate consults the registry first, then this store, so an agent
token is a strictly additional subject and a phone bearer never pays for the
lookup.
startOpts mints the token (as MAKIT_CLI_TOKEN, alongside MAKIT_PROJECT_ID /
MAKIT_WORKTREE / MAKIT_SPAWN_DEPTH), and the re-attach path drops-then-mints in
the same step — the only correct rotation seam, because env is a spawn-time
snapshot and re-attach is the one moment the adapter (and thus its env)
restarts. MAKIT_SPAWN_DEPTH is display-only (D9 recomputes depth server-side)
and stays 0 until the lineage counter lands in T11.
* feat(ws): SPEC-46 T4 — enforce the capability map at the router
D2 gives the CLI caps and D3 gives an agent a scoped token, but caps are
decoration until something refuses on them. The router is the one correct
place: the check runs BEFORE handler lookup, so a refused command never enters
a handler — a per-handler check would spread the rule across every command file
and still miss the non-command paths (fanout, srv.response). Full access and a
`client` principal pass everything; the three agent caps carve out the narrow
surface a handoff child needs (send.message/session.action, session.spawn/
worktree.create), and everything else — session.kill, ports.*, pr.*,
devices.* — is refused for an agent token.
Completeness is a test, not a discipline. `CommandRouter.kinds()` exists so the
suite can assert every registered kind appears in the map against the REAL
router built by the now module-level `buildCommandRouter` — not a toy. It
already earned its keep: it caught pr.markReady/updateBranch/squashMerge, which
are registered from a table and are exactly the kind of omission that would
otherwise let a command silently become agent-reachable.
* feat(ws): SPEC-46 T5 — gate event fanout on the principal
The router check (T4) is necessary but not sufficient, because fanout is not a
command. `fanout` auto-mirrors every session's events to every authed client,
ignoring subscription by design — right for a phone, which must stay in sync
without sub'ing first. But an agent-scoped token that merely connects would
then receive the transcript of every session on the machine, including its
parent's and unrelated users' work: the same class of hole as srv.response, and
for the same reason (neither is a command).
So a session-scoped principal (sessionId set) now receives fanout only for its
own session; a `client`/no-caps principal keeps today's auto-mirror exactly —
two regression locks pin that. The previously-unused `_sessionId` parameter is
now the gate's key. (Descendant sessions, which D17 also grants, wait on the
lineage data that T9/T10 persist.)
* feat(cli): SPEC-46 T6 — the shared WSS client and its own credential
* feat(cli): SPEC-46 T7 — makit ls over WSS, with the output and exit-code contracts
* feat(cli): SPEC-46 T8 — attach re-homed on the CLI's credential; devices.json reading deleted
* refactor(cli): SPEC-46 — one connect path for every session verb
* feat(cli): SPEC-46 T18 — pure handoff-manifest parse + deterministic markdown render
* feat(cli): SPEC-46 T12 — makit new, with a worktree of its own
* feat(ws): SPEC-46 T10 — derive session.spawn parentId/origin from the credential
An agent-scoped token's parent IS its own session; a body parentId naming a
different session is refused BadRequest (D9). A human client spawns a root,
origin mapped honestly from the principal: agent-scoped→agent, client cap→cli,
full-access→app (D10). Lineage is threaded into spawnPendingSession and set on
the Session at construction.
* feat(ws): SPEC-46 T11 — bound spawn depth and live fan-out server-side
Refuse a spawn past depth 3 or beyond 4 live children, with a message naming
the limit (D9). Depth/fan-out are recomputed by walking persisted parentId
links in the manager, never from the forgeable MAKIT_SPAWN_DEPTH. The pure walk
in lineage.ts terminates on a forged cycle and on a missing/archived ancestor.
* feat(ws): SPEC-46 T13 — session.transcript, a bounded transcript slice (C3)
cmd {session.transcript, sessionId, limit} → ack {events}: the last limit
events, oldest-first, served from EventStore.read with limit clamped 1..200 and
returned verbatim (same wire shape as fanout, no projection — D7). Added to the
capability map under cap read so the completeness test stays green.
* feat(cli): SPEC-46 T19 — makit handoff: a manifest, the parent's tree, a fresh agent
* feat(cli): SPEC-46 T14 — send, a message to a session over send.message
Adds the shared CLI test harness (test/support/cli_home.ts). --attach is
parsed but deferred with a usage error (media upload needs the bearer + an
HTTPS client for the self-signed loopback cert); text turns ship.
* feat(cli): SPEC-46 T14 — tail, replay + stream a session over sub
--json is one SessionEvent per line, byte-identical to the wire (D7); human
output goes through renderEvent. The sub ack marks end-of-replay for the
non-follow case. Extends stub_wss to replay events on sub, push live events,
and error a sub.
* feat(cli): SPEC-46 T14 — resume, re-attach a cold session via session.attach
* feat(cli): SPEC-46 T14 — rm, archive by default and kill only with --kill
* feat(ws): SPEC-46 T20 — audience ladder + stored eligibility + response authorization (D13a/b/c)
* feat(ws): SPEC-46 T20 — gate --yolo to human credentials at session.spawn (D13)
* feat(cli): SPEC-46 T16/T17 — wait (edge-triggered) and run, plus a safe stdout seam for tests
* test(e2e): SPEC-46 — a control socket in the keyless loop, so the CLI verbs can be driven
* feat(ws): SPEC-46 T21 — carry a self-describing session caption on srv.request (D14)
* feat(app): SPEC-46 T21 — caption a stranded prompt from the srv.request envelope (D14)
* feat(app): SPEC-46 T21 — caption an elicitation too, not just a permission prompt
* fix(manager): SPEC-46 D3/D9 — the agent's env carries its real project, worktree and depth
* feat(cli): SPEC-46 — send --attach uploads to POST /media instead of refusing
* docs(spec): SPEC-46 — tick P0/P1 acceptance, record what is proven narrower
* docs(plan): SPEC-46 P2 — drop makit log, and why approve/answer needs no D13 change
* feat(cli): SPEC-46 U1/U2 — makit tree (lineage projection) and makit ask (delegate and print the answer)
* feat(app): SPEC-46 U5 — caption handed-off sessions in the list
* fix(cli/ws): SPEC-46 — replay is not a turn; a human may state its handoff parent; refusals are sentences
* feat(cli): SPEC-46 U3 — makit approve <id> [--deny]
Answer a session's pending confirmAction from the terminal so a session an
agent started (nobody on a screen) can be unblocked from the shell that
started it. Client-only: sub replays the pending srv.request, the CLI sends
srv.response — D13's authorization stays server-side, the client only mirrors
D13(b) by answering solely the session it named. No pending prompt exits
non-zero (bounded wait) rather than hanging.
* feat(cli): SPEC-46 U3 — makit answer <id> TEXT
Fill a session's pending input elicitation with TEXT from the terminal, the
sibling of makit approve over the same shared srv.response sender.
* docs(plan): SPEC-46 U4 — thread/fork's real shape, its rollout precondition, and the four decisions
* fix(app): SPEC-46 — lineage with no reason is 'Continued from', since a fork is not a handoff
* feat(ws): SPEC-46 U4 — forkSession() adapter seam, codex thread/fork
The optional AgentAdapter.forkSession() SPEC-29 named but never cut: an
adapter-native head fork returning the new thread's id. Codex implements it
via thread/fork; a fork of a thread with no persisted rollout raises the new
ForkPreconditionError so the command can refuse in plain words rather than
relay codex's -32600 string.
* feat(ws): SPEC-46 U4 — session.fork command, gated on capabilities.fork
The verb SPEC-29 marked PENDING. Forks a live session at its head via the
adapter's forkSession(), then adopts the forked thread through the existing
resume path: spawnPendingSession carries resumeAgentSessionId onto the draft,
and promotion resumes it (thread/resume) instead of thread/start — the trap
that would silently hand back a fresh conversation. parentId is the source,
handoffReason stays unset (a fork is not a handoff, D6/D10), and the
depth/fan-out guard runs before the fork so a refusal leaves no orphan thread.
Unsupported harnesses (pi) are refused with D6's precise sentence naming
handoff; a draft or a rollout-less thread is refused in plain words.
* feat(ws): SPEC-46 U4 — makit fork <id> [--agent A] [--worktree]
The CLI verb: a thin client of session.fork. Inherits the source's worktree
and branch by default (D15 inverse), --worktree takes a fresh tree. A server
refusal (unsupported harness, no rollout, depth bound) prints as a sentence
via failCommand, not a stack trace. session.fork is mapped client/full-access
only in the capability map — a fork names its source on the argv, so unlike
session.spawn it is not agent-reachable (least privilege).
* test(ws): SPEC-46 U4 — lock session.fork out of agent tokens
* fix(ws): SPEC-46 U4 — do not invent an '<agent>-acp' component in the fork refusal
* style(app): SPEC-46 — dart format the files this branch touched
* fix(ws): SPEC-46 D17 — gate every session read path on the principal, not just fanout
sub, the auth snapshot, the live snapshot broadcast and session.transcript all reached
session data without consulting the principal, so an agent token could read every
session on the machine — the property D17 exists to deny. One rule in ws/read_access.ts
now serves all five paths, with a test that enumerates the call sites.
Also: agents are excluded from every prompt rung (they could never answer, but were
shown the question); wait/ask treat an exited agent as terminal for any --for instead
of hanging; approve/answer tolerate replay order; handoff refuses an empty manifest.
* fix(test): SPEC-46 — a second e2e harness must refuse, not clobber the first's control socket
* fix(ws): SPEC-46 U4 — fork always uses source harness, never defaults to pi
* fix(ws/codex): SPEC-46 U4 — a fork inherits its source's harness, re-attaches a cold source, releases the thread
Driven against real pi and codex. pi refuses per D6 (pi-acp advertises no fork).
codex forks with real fidelity (the child recalled the parent's planted word), but
three bugs blocked it: the child defaulted to pi and died on session/load, a cold
source was reported as incapable, and the forking process still holds the new thread.
The first two are fixed with tests; the third is documented with the evidence and the
candidate next steps.
* fix(cli): SPEC-46 — every verb reaches an exit code, and its credential is scoped to loopback
An automated review of the branch, with each finding verified against the code
before it was believed. The unifying defect: a CLI verb could end without ever
producing an exit code, which for a contract built on them (D8) is the one
outcome a caller cannot handle.
- `awaitSnapshot`/`awaitProjects` were the two awaits not keyed by a frame id,
so `failAll` skipped them: a socket that closed mid-await left the promise
unsettled, holding the event loop open forever. Stranded `ls`, `new`, `run`,
`fork` and `wait` alike. Their test file ran 240s before the fix and 0.3s after.
A second caller also silently orphaned the first's resolver.
- `run` and `ask` had no try/finally, so a refusal the server is *expected* to
give (the D9 depth bound, a full queue) escaped as an unhandled rejection with
a stack trace and a leaked socket. `new` already had the pattern.
- `wait` kept delivering events after it settled — `run`/`ask` print from
`onEvent`, so a finished command emitted the *next* turn's output as its own —
and its timeout bypassed `settle` entirely. A misspelled `--for aproval` was
silently widened to `any`, so a script written to block until a human is asked
something sailed past a completed turn instead. Now exit 2.
- The media upload had no timeout: a server that took the connection and stalled
hung the verb with no exit code at all (the test hung 200s before the fix).
- `--port` with no value became NaN, was dialled, and failed as *exit 4*
("your credential is the problem") for what is plainly a typo. Validated once
in `connectCli` rather than at fifty-odd `argv[++i]` sites.
- TLS: `rejectUnauthorized: false` was justified as "the CLI talks to loopback"
but `host` comes from argv, so the exemption covered every remote host too.
Now loopback only; D11 owns remote, with the pinning it requires. Proved by
driving a non-loopback name that resolves to the local server — it refuses the
self-signed cert instead of trusting it silently.
- The protocol envelope was overwritable by command fields (`...fields` spread
last), and `respond` took `m.id` unchecked — a prompt without one produced a
response the server could correlate to nothing, so the answer vanished while
the verb reported success and the human believed they had approved a tool call.
- `new` left the worktree behind when the spawn was refused, one per attempt on
exactly the agent-retry path D9's bound exists to make safe; `fork` forked a
source missing from the snapshot with no worktree at all.
- `ls --archived` and `handoff --file` surfaced server refusals and a mis-pathed
file as raw node errors, and both `handoff` and `connect` called `process.exit`
past the `finally` that was meant to close their socket.
Not fixed, deliberately: `wait`'s snapshot-then-subscribe window. A transition
landing inside the discarded replay leaves the wait running to its timeout, and
closing it needs an end-of-replay marker the wire does not have (`sub` takes no
limit, no DTO publishes a latest seq). Documented in place as a spec question,
with why `--timeout` is load-bearing until then.
One reported finding was rejected: a code fence in carried transcript text
cannot escape the excerpt, because every line is prefixed `[seq] kind` and
payload whitespace is collapsed, so nothing can start a line. Pinned as a test
of that invariant rather than changed.
* fix(ws,manager): SPEC-46 — an agent's token dies with its session, and D9's bound is a reservation
The server half of the same review. Three of these are authorization defects
that the branch's own decisions already forbid on paper.
- **D3's "revoked when the session ends" was not true.** `sessionTokens.drop()`
ran only before a re-attach re-mint, so `killSession` and `archiveSession`
ended a session while leaving its `MAKIT_CLI_TOKEN` valid for the rest of the
daemon's life — an agent process that outlived its kill() still held
spawn/send/read. Dropped on both paths, before the kill, since a token that
survives a *failed* kill is exactly the case that matters. The store's comment
claimed the guarantee was "true by construction" from being in-memory; it was
not, and now says which callers own it.
- **D9's fan-out bound was advisory.** The command layer checked it, then
`spawnPendingSession` awaited `listWorktrees` before registering the row — so
two spawns fired together from one parent both saw a free slot and both landed.
The caller the bound was written for is an agent in a retry loop, which is
precisely what fires concurrent spawns. Re-checked in the same synchronous step
as `sessions.set`, which makes it a reservation rather than a second stale read.
- **The auth gate trusted an under-specified principal.** `isFullAccess` reads a
missing `caps` as FULL access and `isAgentScoped` reads a missing `sessionId`
as "not an agent", so either omission would have promoted an agent past the
capability map and every read gate. The real store sets both; the interface it
satisfies does not require it, so the gate now fails closed at the boundary.
- `grantCli` returned an existing `cli@<host>` row as-is, so a row that reached
devices.json without caps came back as full access — the opposite of the
least-privilege subject D2 exists to create. Repaired and persisted.
- `session.fork` validated a mismatched `--agent` *after* `adapter.forkSession()`
had already succeeded, orphaning a codex thread no makit session references and
no verb can reach. Moved above the fork, beside the depth guard that is ordered
that way for this exact reason.
- `session.transcript` was bounded on the wire but not in memory: `readTranscript`
read the entire log and sliced it, which is the cost D5 exists to remove
("would load and ship the whole transcript to print five lines"). Added
`EventStore.readTail`, bounded by LIMIT in SQL, with a test that counts rows at
the SQL seam — a `.slice()` passes every other assertion while keeping the cost.
- The stub's FAIL_TURN timer was untracked, unlike SLOW's, so it fired
`session.error` after `kill()` and left a cancelled turn wedged running. This is
the harness the keyless loop and every queue test run on, so a stray terminal
event there is a flake with a plausible-looking cause.
- The e2e harness returned the *phone's* bearer from `cli.grant`, so the CLI
authenticated as a full-access device and no test could ever catch a path that
confuses `caps: ["client"]` with caps-absent — the invariant most of D2 rests
on. It now seeds two devices with two bearers, and stubs the unimplemented
control verbs by name instead of failing as "not a function".
One reported finding was rejected: `session.fork` needs no `canReadSession`
gate, because `COMMAND_CAPABILITIES` maps it to `[]` and so no agent token can
dispatch it at all. That is deliberate and documented at the mapping.
* fix(app): SPEC-46 D14 — a captioned prompt stays readable on a phone
D14 made both prompt dialogs open-ended: the caption carries a session title and
a whole handoff reason, and the message is whatever the agent wrote. Neither
`content` was scrollable, so a long prompt on a compact device is text the user
can neither finish reading nor scroll past to reach Deny/Approve — which trains
them to answer without reading, the exact failure captioning was added to prevent.
Measured: a realistic 1307-character permission message in a 320x320 view.
Uses AlertDialog's own `scrollable` flag rather than a hand-rolled scroll view.
The test asserts the *caption* has a SingleChildScrollView ancestor, because the
preview's SelectableText has an internal Scrollable that satisfies a looser
assertion while the caption and message stay stuck.
Also strengthens two assertions that passed against the regressions they were
meant to catch: the elicitation caption test never asserted the handoff reason it
seeded (so a caption lost from `_showInput` alone would not have failed it), and
the no-lineage tile test checked only for 'Handed off', passing against a
`_handoffCaption` that captioned every session 'Continued from another session'.
Note: the reported RenderFlex overflow could not be reproduced — AlertDialog
bounds content in a Flexible — so this fixes the readability defect that is
real and leaves the layout as it is.
* fix(test): keep the shared snapshots fixture byte-identical across app and server
`snapshots.json` is duplicated in app/ and server/ and CI (`fixture-sync`)
asserts the two copies are byte-identical. The merge resolved only the server
copy — the conflicted one — so the app copy silently kept main's version with no
SPEC-46 lineage, and the two diverged in both content and formatting.
Takes main's compact one-line layout for both, with D10's `parentId` /
`handoffReason` / `origin` added to `s1`, and writes the same bytes to each. The
lineage fields have to be in the shared fixture rather than a server-only one:
the point of the contract pair is that the Dart codec and the TS protocol are
tested against the same wire, which is what stops the app and server projections
drifting when a field is added.
Verified: server protocol contract 16/16, app `codec_contract_test.dart` 12/12,
`diff -q` between the two copies is clean.
* fix(test): force the stub server's sockets shut so a stalled request cannot wedge the suite
CI's server `test` job was cancelled at its 15-minute limit having produced only
22 seconds of output. That shape is not slowness — `node --test` buffers each
file's TAP until the file finishes and then waits for every file, so one file
that never completes looks exactly like this: the others report, and then silence
until the job is killed.
The file that never completed is the one that asks the stub to accept an upload
and never answer it (`mediaStall`, added with the media-upload timeout test).
`server.close()` stops accepting connections but *waits* for the ones still open,
and that request is deliberately still open — so `stub.close()` never resolved.
It passed locally because macOS tears the socket down on the client's
`req.destroy()`; on Linux the server-side socket lingers, which is why only CI
hung.
`closeAllConnections()` before `close()` makes teardown unconditional, so a test
may leave a request unanswered — which is the point of `mediaStall` — without the
failure mode being an unreadable timeout fifteen minutes later.
Verified on Node 22 (CI's version, since the local default is 26): the five
stub-heavy CLI files 56/56, and the full suite 1622/1623 — the one failure is a
pre-existing git-worktree test that passes twice in isolation and had been
starved to a 233-second runtime by local load, not by this change.
* test(server): bound every test at 120s so a hang names itself instead of killing the job
The server `test` job has now been cancelled at its 15-minute limit three times,
always after the same 22 seconds of output, and the diagnostics are useless: with
no per-test timeout a hung test produces silence, then `The operation was
canceled`, with nothing naming the test or even the file.
Reproducing it needs the runner itself — the suite passes on macOS/Node 26,
on Node 22 at concurrency 2, and in `node:22` under Docker with `--cpus=2`
(1623 tests, exit 0), so inference has run out of road. A per-test timeout turns
the next run into evidence: node reports `testTimeoutFailure` with the test's name
and location.
120s is ~100x the slowest legitimate test here, so it cannot start failing honest
work; it only converts an unbounded hang into a named failure. Worth keeping
afterwards for the same reason — a suite with no upper bound can always take CI
down with no explanation.
* fix(cli): bound the snapshot waits, so no verb can hang without an exit code
The server `test` job kept dying at its 15-minute cap, and with the per-test
timeout in place it finally named itself: `src/cli/wait.test.ts` times out right
after its three argv-only tests — i.e. on the first test that actually connects.
It reproduces on the runner every time and on nothing else: the suite passes on
macOS/Node 26, on Node 22 at concurrency 2, and in `node:22` under Docker at both
`--cpus=2` and `--cpus=4` (1623 tests, exit 0). Two hypotheses died on the way
here: forcing the stub's sockets shut did not fix it, and RSA keygen is not the
cost (a 2048-bit `selfsigned.generate` measures 1ms, four concurrently still 0s).
What the investigation did expose is a real defect of the same family as the two
already fixed on this branch: `awaitSnapshot()` / `awaitProjects()` reject when
the socket *closes*, but a server that stays up and simply never pushes was
waited on **forever**. That keeps the socket open and the event loop alive, so
the verb produces no output and no exit code — the one outcome D8's contract
cannot express, and precisely how a CI job ends up cancelled minutes later with
nothing naming the cause. `ls`, `new`, `run`, `fork`, `ask` and `wait` all await
a snapshot before doing anything, so all six inherited it.
Both waits are now bounded at 15s and reject with a sentence. The server pushes
both snapshots immediately after `hello.ack`, so this can only fire when
something is genuinely wrong; the timer is unref'd so it can never itself be the
reason a process stays alive.
Verified: server 1624/1624 on Node 26 and the stub-heavy files 66/66 on Node 22
(CI's version). Whether this is the runner's exact stall or not, the class of
failure it removes is the one that was killing the job blind.
* fix(test): script live events on subscription, not on a timer — the CI hang's root cause
`src/cli/wait.test.ts` timed out on the GitHub runner every single run while
passing everywhere else (macOS/Node 26, Node 22 at concurrency 2, `node:22` in
Docker at --cpus=2 and --cpus=4). The per-test timeout added earlier is what
finally named the file; this is the cause.
`waitWith` pushed its scripted `running` → `idle` events on fixed 15ms/30ms
timers, started before `runWait`. But the verb has to open a unix socket,
complete a TLS handshake, `hello`, take a snapshot and only then `sub` — and on a
loaded runner that takes longer than 30ms. The events therefore landed *before*
the subscription existed, `awaitOutcome` never saw the transition it is waiting
for, and because that first test deliberately passes no `--timeout`, `wait`
blocked forever. A pure race, invisible on any machine fast enough to connect in
under 30ms.
The stub now exposes `onSub`, fired the moment it has acked a `sub` — i.e. when
the client is provably subscribed and past end-of-replay — and the three
harnesses that scripted live events on timers (`wait`, `run`, `ask`) push from
there instead. No delay is needed because the socket preserves order, and no
delay is wanted: the delay *was* the bug.
Verified under deliberate starvation, which is what the runner amounts to: with
`--cpus=0.35` in `node:22`, where the connect takes many times the old 30ms
budget, wait/run/ask are 43/43. Full suite 1624/1624.
* refactor(cli): centralize the async ceremony in connectCli helpers
Each of the 10 verbs that call `cmd` had duplicated the try/finally/close dance,
plus separate implementations of the same argv validators (bounds checks at 59+ sites).
Extract:
- validateArgs(host, port) in connectCli — one bounds check for both, informs --help
- withClient(args, fn) — owns try/finally/close, rejects WireError to failCommand()
- failCommand(err) in connect.ts — maps error to exit code, prints message
This shrinks each verb by 1-2 lines of ceremony and centralizes the shared
pattern. Reduces copy-paste by ~40 lines across the CLI. The 4 event-loop verbs
(attach, respond, run, tail) use their own patterns and are exempt.
* refactor(cli): one withClient() owns the connect/refuse/close contract, not eleven verbs
"Connect, run, map the error, close in a finally" was written out in eleven verbs
and got it right in seven. `send`, `rm` and `resume` caught everything and printed
a bare message, so a genuine bug lost its stack and reported exit 1 as if the
server had declined; `wait` had no mapping at all, which mattered the moment the
snapshot wait became bounded, because its rejection had nowhere to land.
Fixing one bug in seven files and leaving it in four is the signature of a missing
abstraction, so the contract now lives in one place:
withClient(args, async (client) => { ... })
AuthError -> exit 4 WireError -> a sentence, exit 1
always closes
Every converted verb is now its own body and nothing else, and D8's exit-code
contract is structural rather than eleven instances of per-file discipline. This
also deletes `run.ts`'s `runTurn` helper, which only existed to give a `finally`
something to wrap.
Two things this refactor forced into the open, both kept:
- `uploadMedia` rejected with a plain `Error`, which only `send`'s
catch-everything block made presentable. Rather than restore that block, the
refusal is now the `WireError` it always was: the class is "the server declined,
or the operation failed for a reason the user must read" — not "a WebSocket
frame said no" — and `POST /media` fails that way over a different transport.
`WireError` now documents that role instead of leaving it implied. Caught by an
existing test, which is exactly what it was for.
- `attach`, `tail` and `respond` are NOT converted. They never call `client.cmd`,
they own their own event loop and exit code, and wrapping them would be an
indirection that buys nothing. My review counted fourteen candidates; the honest
number is eleven.
Net -60 lines. Server 1624/1624, typecheck clean.
* refactor(cli): declarative parseFlags() parser — replaces 59 hand-rolled argv loops with one canonical spec
* refactor(cli): one declarative argv parser, replacing 59 hand-rolled scan sites
Every verb hand-rolled the same `for` loop over argv with `argv[++i]` — 59 such
sites across 16 files — and all of them shared one defect: a value flag in last
position reads past the end. `makit ls --port` became port `NaN`; `makit send s1
-m` sent an agent the literal text `"undefined"`.
I "fixed" that earlier by validating host and port inside `connectCli` and wrote
that one check there "beats fifty-odd bounds checks at the parse sites". That was
wrong. It fixed the two flags that happen to pass through that function and left
`-m`, `--agent`, `--for`, `--carry`, `--branch`, `--project`, `--base`, `--since`
and `--to` broken, because a missing value is a property of *parsing*, not of
connecting. `parseFlags` now rejects it once, for every flag, with the flag named.
The parser knows five value shapes and no verbs: `--timeout` seconds→ms and
`--carry last:N` stay in the verbs that own those meanings, and each verb keeps
its explicit `Args` interface as the typed contract. Two things fall out for free:
- `--for` becomes `{ type: "enum", values: [...] }`, so the hand-written
validation I had added to `wait` is now declarative and applies to `run` too.
- `run` stopped parsing one argv twice against two specs; it parses once against
`{ ...NEW_FLAGS, ...WAIT_FLAGS }`.
Unknown flags are still ignored. That is deliberate: `run` relied on it, and
tightening it is a behaviour change rather than a refactor.
Also folded in: the eight copies of `failUsage` collapse to one, and the dead half
of my old band-aid is gone — the literal-`"undefined"` host it guarded against was
never something a user typed, it was the parsing bug's output. Its test now
asserts what the code still owns (the range, and an empty host); removing it
surfaced that an unresolvable `--host` has no connect timeout, which is real but a
separate concern and left alone.
59 scan sites → 0 outside the parser. Server 1630/1630 (6 new parser tests),
typecheck clean, and not one verb test needed changing.
* docs(ws): clarify capability map is forward-looking, not exhaustive
* Merge origin/main into feat/cli-client1 parent e536ddc commit ef10fd1
107 files changed
Lines changed: 11513 additions & 447 deletions
File tree
- .pnpm-store/v11
- projects
- app
- lib
- store
- transport
- ui
- home
- widgets
- test
- ui/home
- docs/specs
- server
- src
- adapters
- cli
- daemon
- pairing
- storage
- ws
- commands
- test
- protocol
- support
- ws
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
Binary file not shown.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1124 | 1124 | | |
1125 | 1125 | | |
1126 | 1126 | | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
1127 | 1130 | | |
1128 | 1131 | | |
1129 | 1132 | | |
| |||
1171 | 1174 | | |
1172 | 1175 | | |
1173 | 1176 | | |
| 1177 | + | |
| 1178 | + | |
| 1179 | + | |
| 1180 | + | |
| 1181 | + | |
| 1182 | + | |
| 1183 | + | |
| 1184 | + | |
| 1185 | + | |
| 1186 | + | |
| 1187 | + | |
| 1188 | + | |
1174 | 1189 | | |
1175 | 1190 | | |
1176 | 1191 | | |
| |||
1191 | 1206 | | |
1192 | 1207 | | |
1193 | 1208 | | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
1194 | 1212 | | |
1195 | 1213 | | |
1196 | 1214 | | |
| |||
1210 | 1228 | | |
1211 | 1229 | | |
1212 | 1230 | | |
| 1231 | + | |
| 1232 | + | |
| 1233 | + | |
1213 | 1234 | | |
1214 | 1235 | | |
1215 | 1236 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
269 | 269 | | |
270 | 270 | | |
271 | 271 | | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
272 | 277 | | |
273 | 278 | | |
274 | 279 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
131 | 131 | | |
132 | 132 | | |
133 | 133 | | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
134 | 156 | | |
135 | 157 | | |
136 | 158 | | |
| |||
168 | 190 | | |
169 | 191 | | |
170 | 192 | | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
171 | 211 | | |
172 | 212 | | |
173 | 213 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
334 | 334 | | |
335 | 335 | | |
336 | 336 | | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
337 | 343 | | |
338 | 344 | | |
339 | 345 | | |
340 | 346 | | |
341 | 347 | | |
| 348 | + | |
342 | 349 | | |
343 | 350 | | |
344 | 351 | | |
| |||
387 | 394 | | |
388 | 395 | | |
389 | 396 | | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
390 | 400 | | |
391 | | - | |
392 | | - | |
393 | | - | |
394 | | - | |
395 | | - | |
396 | | - | |
397 | | - | |
398 | | - | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
399 | 419 | | |
400 | 420 | | |
401 | 421 | | |
| |||
493 | 513 | | |
494 | 514 | | |
495 | 515 | | |
| 516 | + | |
| 517 | + | |
| 518 | + | |
| 519 | + | |
| 520 | + | |
| 521 | + | |
| 522 | + | |
| 523 | + | |
| 524 | + | |
| 525 | + | |
| 526 | + | |
| 527 | + | |
| 528 | + | |
| 529 | + | |
| 530 | + | |
| 531 | + | |
| 532 | + | |
| 533 | + | |
| 534 | + | |
| 535 | + | |
| 536 | + | |
| 537 | + | |
| 538 | + | |
| 539 | + | |
| 540 | + | |
| 541 | + | |
| 542 | + | |
| 543 | + | |
| 544 | + | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
| 551 | + | |
| 552 | + | |
| 553 | + | |
| 554 | + | |
| 555 | + | |
| 556 | + | |
| 557 | + | |
| 558 | + | |
| 559 | + | |
| 560 | + | |
| 561 | + | |
| 562 | + | |
| 563 | + | |
| 564 | + | |
| 565 | + | |
| 566 | + | |
| 567 | + | |
| 568 | + | |
| 569 | + | |
| 570 | + | |
| 571 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
49 | 49 | | |
50 | 50 | | |
51 | 51 | | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
52 | 97 | | |
0 commit comments