fix(permissions): stop tlive from changing what CC asks, and close #64 - #66
Merged
Conversation
"Why didn't tlive hold this one?" was unanswerable from the outside. Five paths return without a card — policy allow, sub-agent pass-through, no answer surface, timeout, caller gone — and the log recorded only the IPC kind, so all five looked identical. Diagnosing one report cost several rounds of guesswork and three wrong conclusions. Each outcome now emits a stable reason tag, and a held request emits the surface that settled it (remote / local-terminal / timeout / caller-gone / daemon-shutdown). The no-answer-surface case records every input, not just the verdict, because which surface was missing is the whole question. The local-prompt path logs whether it suppressed the notification because a held card already owned the session, or forwarded it because nothing did — the two are indistinguishable from outside and confusing them is how a pass-through got read as a held approval. Identity fields only. Tool inputs, prompts and card bodies stay out: the log is shared by every session on the machine and any of those can carry secrets. A test asserts that.
…ing why Every change here restores the same contract: installing tlive must not alter which prompts the user sees, nor add messages the no-hook baseline never sends. - PermissionRequest fires ONLY when CC is about to show a dialog, so anything auto-allowed there deletes a prompt the user would have seen. READ_ONLY_TOOLS (Read/Glob/Grep) was allowed unconditionally on the theory that the `*` matcher added friction to suppress; that theory was wrong — the hook never sees an ungated Read. It is now opt-in behind `approvals.autoApprove`, which defaults to off, and `/safe off` no longer silently switches read-only allow ON for a user who never asked for any auto-approval. - The bare "answer in the terminal" IM text is no longer pushed in `full` mode. Reaching that path in full mode means tlive saw the request and handed it back (sub-agent pass-through, policy allow, no answer surface), and "handed back" is defined as "behave as if tlive were not installed". It also cannot be attributed: CC's Notification input carries neither agent_id nor tool_name, and its session_id is the main session's even for a sub-agent's dialog. `notify` mode still sends it — there it is the only signal a dialog is waiting. - The "Always allow <tool>" button carried the tool name in its callback payload. Telegram caps callback_data at 64 bytes and rejects the WHOLE card when a button is over, which every name longer than 17 characters was — i.e. every `mcp__<server>__<tool>`. Measured 79 bytes for `mcp__codegraph__codegraph_status`, so MCP approvals silently never arrived and the request hung until the caller died. The payload is now requestId-only and the daemon resolves the name from the pending request. - A card that failed to send was swallowed with `.catch(() => undefined)`, which is why the above left no trace anywhere. Failures are now logged as `permission.card.undelivered` with the channel and error. - Observational hooks waited up to 4s on the daemon. They run synchronously and post-tool-use fires on every tool call, so that was a direct tax on the terminal whenever the daemon was slow; capped at 1.5s. - `/safe` had two hand-written copies of its wording (CLI + IM) and the "off" text promised a read-only exception the default no longer grants. Both now share SAFE_TOGGLE_MESSAGE. Comment corrections, all from measurements rather than reading: CC builds every hook's base input with the MAIN session id and puts sub-agent identity only in agent_id, so `sessionId` discriminates nothing between a session and its sub-agents — only matchAgent does. hasPendingFor and LocalPrompts' single slot are therefore proxies that cannot be made exact, and are documented as such. The claim that a held hook leaves CC with no local dialog is false for the main session: measured 5/5 holds produced a permission_prompt 6.0s in, and four were answered at the keyboard mid-hold.
…no bytes `shadow.write(data)` is asynchronous — xterm queues the chunk and parses it on a later macrotask, yielding periodically while output keeps flowing — but the broadcast to sockets is synchronous. So at attach time the serializer could be arbitrarily far behind the wire, and building a snapshot straight away produced one of two failures: an empty snapshot that the `length > 0` guard dropped entirely, leaving the tab blank for as long as the session stayed quiet; or a snapshot ending at offset P spliced onto live bytes starting at Q > P, silently losing everything between with no error and no gap marker. Two things were needed and neither works alone: 1. A parse barrier. An empty write takes its place in xterm's queue behind every chunk already queued and its callback fires once the parser has consumed them, so the serialized screen accounts for exactly the bytes broadcast before the call. 2. Holding that client's live frames until the snapshot is on the wire. Without it the barrier only moves the gap: bytes broadcast while waiting would arrive ahead of a snapshot that already contains their predecessors. The buffer is released on every path including a serialize failure, where dropping it would turn a cosmetic error into lost output. Past the barrier the `length > 0` check becomes sound and is kept: empty now means the screen really is empty, not "not parsed yet". The regression test drives sustained numbered output and attaches mid-burst, then asserts the client's whole stream contains an unbroken run of markers. Without the barrier it fails immediately — measured a 223-line hole, 7038 to 7262. Concatenating before extracting is deliberate: a marker split across the snapshot/live boundary is completed by the live tail, so write boundaries cannot cause a false failure. The existing late-joiner test no longer reaches into host.serializer to know when attaching is safe, since the flush makes it deterministic from outside. It is NOT a guard for this bug and now says so: measured with the flush removed, the parser still wins that particular race and the test passes anyway. Closes #64
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Six baseline violations in the permission path, plus #64. Every fix here came out
of measuring the real thing against Claude Code's own behaviour rather than
reading our own comments, and several of those comments turned out to be wrong.
The contract
Installing tlive must not change which prompts the user sees, nor add
messages the no-hook baseline never sends. Each item below restores that.
What was broken
Read/Glob/Grep were auto-allowed unconditionally. The justification in the
code was that CC doesn't prompt for them anyway and our
*matcher would addfriction. That reasoning is wrong:
PermissionRequestfires only when CC isabout to show a dialog, so the hook never sees an ungated Read — there is no
friction to suppress, and every allow there deleted a prompt the user would
have seen (a Read outside the working directory, for instance). Now opt-in
behind
approvals.autoApprove, default off./safe offalso stopped silentlyswitching read-only allow on for someone who never opted into any
auto-approval.
MCP approvals never arrived. The "Always allow <tool>" button carried the
tool name in its callback payload. Telegram caps
callback_dataat 64 bytes andrejects the whole card when any button is over — which every tool name longer
than 17 characters was, i.e. every
mcp__<server>__<tool>. Measured 79 bytes formcp__codegraph__codegraph_status. The request then hung with no answer surfaceuntil the caller died, and nothing was logged, because the send was
fire-and-forget with
.catch(() => undefined). Payload is now requestId-only(46 bytes, always safe) with the name resolved from the pending request, and
failed sends are logged as
permission.card.undelivered.A bare, un-answerable "answer in the terminal" text was pushed in full mode.
Reaching that path in full mode means tlive saw the request and handed it back
(sub-agent pass-through, policy allow, no answer surface) — and "handed back"
means "behave as if tlive were not installed". It also cannot be attributed: CC's
Notificationinput carries neitheragent_idnortool_name, and itssession_idis the main session's even when the dialog belongs to a sub-agent.notifymode still sends it, since there it is the only signal a dialog iswaiting.
Observational hooks could add 4s to any tool call. They run synchronously and
post-tool-usefires on every call, so a slow daemon taxed the terminaldirectly. Capped at 1.5s; a healthy local socket round trip is sub-millisecond.
/safehad two hand-written copies of its wording and the "off" textpromised a read-only exception the new default no longer grants.
#64 — attaching during output lost bytes.
shadow.write(data)isasynchronous while the broadcast to sockets is synchronous, so a snapshot built
at attach time could end at offset P while the client's live bytes start at
Q > P, silently dropping everything between; or be empty and dropped by the
length > 0guard, leaving the tab blank. Fixed with a parse barrier plusholding that client's frames until its snapshot is on the wire — neither works
alone, since the barrier by itself just moves the gap. The regression test
attaches mid-burst and asserts an unbroken run of markers; without the barrier it
fails immediately (measured a 223-line hole, 7038 → 7262).
Comment corrections
These were load-bearing and wrong:
sub-agent identity only in
agent_id. SosessionIddiscriminates nothingbetween a session and its sub-agents — only
matchAgentdoes.hasPendingForand
LocalPrompts' single slot are proxies that cannot be made exact, andnow say so.
measured 5/5 holds produced a
permission_prompt6.0s in (spread 16ms), andfour were answered at the keyboard mid-hold. The 6s delay is the
notification's own, gated on the user having been idle — which is why its
absence had looked like the dialog's absence.
Verification
tscclean, 670 tests, 70 files. Each fix was driven from a failing test first;the MCP button and the #64 gap both reproduce deterministically without the fix.
Sub-agent behaviour is deliberately left alone here — it depends on an
asymmetry filed upstream as anthropics/claude-code#82150 (the hook is awaited
before the dialog is built for background sub-agents, while the main session runs
both in parallel), and a design that assumes either answer would need reworking
if that changes.
Closes #64