document the claude protocol adapter, and fix what the audit found along the way - #180
Merged
Merged
Conversation
The architecture document's container diagram, worker-process subgraph, container technology table, adapter registry description, per-adapter component list, and control-plane section all described the registry as driving exactly four TUI worker adapters, with the earlier headless control plane retired and nothing left beside the terminal path. A fifth adapter now exists: a claude-specific implementation that drives the vendor over its own streaming-JSON protocol on ordinary pipes, rather than a pseudo-terminal, dispatched explicitly per worker and never selected by default. It is experimental, under evaluation, and not yet recommended for use; which control plane should eventually be primary is under evaluation by a pending decision record. Updated the diagram (a fifth node, a pane label loosened to "PTY or protocol"), the technology table (the worker-process row now names both transports), the registry description (five adapters instead of four, with the new adapter's own five components named individually, since three of them -- an approval bridge, a transcript reconciliation pass, and a pre-flight trust check -- have no equivalent on the terminal path), and the control-plane section (retitled, and its pane sentence now says explicitly that the terminal plane's pane carries the vendor's own bytes while the protocol plane's carries crew's own rendering and is not independent evidence). Also corrected the one doc comment this document quotes verbatim (crates/runtime/src/adapter/registry.rs's own "four real worker adapters" comment) so the source and the document that quotes it agree; this is the one non-Markdown line in this change. No behavior change. bun run typecheck/build unaffected (this is a runtime-crate doc comment, not a crates/protocol/ type, so the generated-bindings bundle is untouched).
Both encoded the same wrong assumption: that a clean process exit means the run succeeded, and that a vendor worker never exits on its own. The lifecycle code has never mapped a clean exit straight to success -- it depends on whether the run's turn had already settled, and a clean exit with no settled turn is failed, not succeeded, a judgment the code leaves to the leader rather than guessing. That guard was stated nowhere in the table. The sentence directly below the table asserted the vendor half of the same assumption: that nothing exits on its own, so only the vendor's own end-of-turn signal can end a run. A worker whose whole life is one turn then an exit has the opposite shape, and the table and the sentence disagreed with the code identically -- fixing only one would have left the document still asserting the wrong thing in the other place, so both change together. Replaced the single success/failure/lost row triple with five rows that state the guard explicitly, and revised the sentence to describe both shapes a worker can have without claiming which one the run lifecycle should ultimately model -- that question is under evaluation by a pending decision record, not something this document decides. Pre-existing defect, unrelated to any adapter recently added -- caught while reading the document for an unrelated reason and corrected here because leaving it would have been citing a table already known to be wrong. No behavior change.
Two documents pointed a reader at the maintainer's own local, gitignored file for remaining gaps or open defects: one in the architecture document, one in the operations guide, each missed by an earlier sweep because it cites the register itself by name rather than a specific identifier from it. That file is not present in any clone of this repository and was never meant to be a citable register -- this project's own conventions already say pointing at it preserves nothing checkable, only the appearance of something checkable. Dropped the trailing citation from the architecture document's sentence (it stands fine without it); rewrote both defect-tracking sentences to state plainly that open defects are tracked outside this repository, without naming where. Pre-existing, unrelated to anything else in this series; kept separate since it needs no coordination with the other edits here. No behavior change.
… both halves The troubleshooting section told an operator to "drop the env var (unset, not =0)" to run live conformance against a real vendor CLI. Both halves of that instruction are wrong under cargo. Only the exact string "1" disables the vendor CLI, so "not =0" is false -- setting it to 0 already works. And .cargo/config.toml sets this variable to 1 for every process cargo launches, so it is never actually absent under cargo test; unsetting it in your own shell only lets that block re-supply 1. An operator who followed this instruction got a green conformance run that had silently exercised fixtures instead of the real vendor CLI, with nothing in the output to say so. Replaced with the form this repository's own top-level docs (AGENTS.md, CLAUDE.md) already carry: set the variable to 0 explicitly alongside CREW_LIVE_CWD, rather than unsetting it. No behavior change -- documentation only.
…test suite Two hardcoded figures had drifted far from the current tree: the CLI entry point was described as roughly 330 lines across six subcommands when it is now 1,865 lines, two paragraphs after the same document's own command list already grew to fourteen named commands and wisely declined to give a fixed total; and the protocol crate's root module was said to declare 15 child modules when it declares 18. Replaced the line-count claim with the same growth-resistant phrasing the document already uses two paragraphs earlier -- point the reader at counting it themselves rather than restate a number that will go stale the same way again. Updated the module count and its named list to the current 18. Also added the claude protocol adapter's own integration-test suite to the Tests section's list, which named only the TUI vendor adapters -- same omission as the architecture/code-walkthrough documents, applied here since this file was already being touched. No behavior change.
Both are point-in-time records and stay historical by design, but two claims in each would send a reader who opens them today straight to the wrong action rather than merely being out of date in a harmless way. The live-E2E runbook opens with "the gate is open and no P1 remains," stated in the present tense; since it was written the gate closed on a later attempt, closed again on a different half of the system, and a further attempt was cancelled outright when the control-plane direction changed. A note now brackets that sentence as historical rather than current, without naming where the current state lives. The same runbook's setup block also used a state-directory path literally named for a past attempt, immediately next to a comment warning not to reuse a state directory from an earlier attempt -- replaced with a non-numbered example. The 0.7.0 release checklist states a specific omp version as "the tested host version" with no hedge, and separately says a fourth live E2E attempt is still required -- both true when written, both overtaken since (the installed version has moved, and two further attempts have already happened and one was cancelled). Both now read as point-in-time facts rather than current status. No behavior change. Both files remain historical records; nothing in them beyond these lines was touched.
The opening paragraph said a vendor TUI positions each word with its own cursor-column escape, stated as a universal property. It is true of claude and codex, and the same document says so itself further down for the other two vendors -- a reader who stops at the opening takes a two-vendor property as if it applied to all four. Scoped the opening sentence to the vendors it actually describes. Separately, the composer-pair section described re-keying codex's readiness predicate onto chrome that survives a paste as an active, near-term piece of work the fixture exists to support. That work was dropped along with the terminal control plane's own live-testing path when the control-plane direction changed -- the pin itself is exactly right and still in place (nobody can mistake the bug for fixed while the fixture keeps asserting the un-fixed behavior), but neither this file nor the code it describes said the re-key had been shelved rather than merely pending. Reworded to say the pin stands until the readiness question is re-decided, without naming when or citing anything that does not survive in this repository. No behavior change -- this is a provenance document, not the fixture data or the code it describes.
The source-map table listed the registry as driving four TUI worker adapters with the earlier headless control plane retired and nothing beside the terminal path, named no module from the protocol adapter's own tree, described the run lifecycle's terminal edge without the guard that actually decides it, omitted the protocol adapter's own test suite from the by-subsystem list, and separately said "the four worker adapters are TUI-only now" -- wrong on both the count and the framing, not just the number. Updated the registry and run-lifecycle rows to match what docs/architecture.md now says (five adapters; the terminal-edge guard depends on whether the run's turn had settled, not on exit status alone); added eight rows for the claude protocol adapter's own module tree, following the same one-line-per-file convention as the existing TUI rows; added its test suite to the by-subsystem list, noting that it links this crate as an ordinary dependency rather than compiling under cfg(test), which is what lets it prove something about the shipping binary; and reworded "the four worker adapters are TUI-only now" to state which four, since a fifth, non-TUI adapter exists alongside them. No behavior change.
…guide The test-suite coverage list named the four TUI vendor adapters and nothing else, so a fifth adapter's own suite had no line. Added it, with the same experimental caveat used everywhere else this adapter is introduced. The existing four-adapter line is left as-is -- it is still true, just no longer the complete list. No behavior change.
0025 and 0026 both read Accepted with nothing to qualify them, while a control plane that drives a vendor over its own protocol now runs alongside the terminal one they describe as the whole plane. Neither decision is reversed, and this repository's own rule against rewriting an ADR to look as though it agreed with a later decision means neither body gets edited, and neither gets marked superseded before the decision that would supersede it exists. Leaving the index silent instead told a reader the question was closed. Changed only the two Status cells to note they are under evaluation, and added one sentence below the table saying so plainly, that neither decision has been reversed, and that a reversal (if one comes) will say so in its own text as the rule already requires. No ADR body touched; index only.
This file is for consciously deferred features with a decision trigger. The entry's own trigger -- implement a non-interactive control plane again only if a deployment needs no PTY and the TUI path is confirmed unworkable, designed fresh against the vendors' current protocols rather than by resurrecting the deleted code -- has now partly happened: one vendor is driven over its own streaming-JSON protocol by a new adapter, designed fresh exactly as the entry asked. So the entry was describing neither a deferred feature nor the reason it stopped being deferred: the actual reason was that the terminal control plane's own failures were failures of reading a screen, not the no-PTY-deployment case this entry anticipated, and the entry's own "why deferred" reasoning is the argument that was set aside when the direction changed. Kept the historical "what it is" paragraph describing the pre-crew-v2 state untouched -- it is true independent of anything since. Replaced "why deferred" and "decision trigger" with a shorter status record: what was deferred, that it happened for one vendor and why (with the experimental/not-yet-recommended caveat this document uses everywhere else it introduces the new adapter), that the anticipated trigger was not the actual one, and that what remains genuinely deferred -- the other three vendors, and which control plane is primary -- is under evaluation by a pending decision record rather than by this entry. This is a shrink, not a rewrite: a shipped, evaluated capability does not belong in the deferred-features file even while its status is still experimental. No behavior change.
Today a leader following any of three skill docs' own worked example,
or crew_profile's tool description, would never write mode: "protocol"
for a claude worker -- every one of them shows only mode: "tui", and
the orchestration skill's own closing line ("you never have to
remember to spell out the replacement") actively teaches the reader
that naming a mode is unnecessary. Protocol mode is real, shipped, and
reachable; it was neither documented anywhere a leader would read it
nor refused if requested by name. Fixed the three skill docs and the
CLI reference to say it exists, how it is requested, that omitting
mode still selects tui, and that it is experimental, under evaluation,
and not yet recommended for use.
Separately, no tracked document stated the workspace-trust precondition
that mode -- protocol depends on: the target repository must already
have its one-time trust prompt accepted in claude directly, or the run
is refused before any vendor process spawns. Added it to the
troubleshooting skill (alongside the existing first-run-gate bullets it
sits next to) and to the user guide (a new short section, since nothing
there mentioned adapter modes at all), each stating the user-facing
requirement plainly and the vendor's own keying rule (repository root;
a worktree uses the main checkout's root) sourced to the vendor's
documentation. Neither says anything about how crew itself resolves
that key internally -- that mechanism is a separate, carded question and
not this document's claim to make.
Also fixed the recovery skill's resume-eligibility bullet, which named
TUI mode and headless's refusal but said nothing about the third case:
the protocol adapter has its own distinct, typed refusal for resume,
out of scope for its first version -- not the same refusal as
headless's, and a reader should not have to guess whether an unnamed
third mode resumes or not.
No behavior change.
The known-version table listed three copilot releases; the adapter's own allowlist has grown to five, and this same document already lists all five correctly further down in its own gate table -- the file disagreed with itself, not only with the code. A reader who lands on the earlier table and stops there gets the wrong answer with no signal that a later, correct one exists. Brought the table to the current five entries, and stated plainly that the only copilot release measured on this machine is currently outside it and therefore refused, unlike claude/codex/omp whose installed versions all sit inside their own range gates -- copilot is the only vendor gated by an exact-match list rather than a range, a design choice explained where the list itself lives, not changed here. No gate or allowlist code changed; widening it to include the newer release is a separate question for whoever owns that decision. Also corrected the quoted `copilot --version` example to the version actually installed here, and added a one-line dated note to the older, historical conformance matrix a few sections up making explicit that every vendor's version there has since moved, alongside the header that already marks the whole section historical. No behavior change.
Both top-level guidance files state the bracketed-paste invariant as though it were exhaustive over every way crew delivers a prompt. It is correct and still a live security control for the terminal path, but a worker driven over a vendor's own protocol writes no prompt bytes to a PTY at all -- there is no keystroke-versus-paste distinction to make there in the first place. Left unscoped, a reader has no way to tell whether the invariant was overlooked for that path or genuinely does not apply to it. Added one sentence to each file's own numbered invariant stating the scope explicitly, with the experimental caveat this repository uses everywhere else it names the new path. No behavior change.
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
1 participant
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.
A second, reachable adapter family exists on main and the hand-written documentation never mentioned it: a claude-specific implementation that drives the vendor over its own streaming-JSON protocol instead of a pseudo-terminal, dispatched explicitly per worker, experimental and under evaluation. Multiple documents described the registry as driving exactly four TUI adapters with nothing beside the terminal path, several skill docs and the CLI reference never told a reader the alternative mode existed at all, and a handful of unrelated staleness issues surfaced along the way. This PR brings the documentation in line with what actually ships, states plainly where it does not yet decide anything (which control plane should be primary is left to a pending decision record, never cited by number since it does not exist yet), and fixes what it found along the way.
What changed, by file group
docs/architecture.md(+ one line incrates/runtime/src/adapter/registry.rs): the container diagram, worker-process subgraph, technology table, registry description, per-adapter component list, and control-plane section all now describe five adapters instead of four, with the new one's own five distinguishing components named individually. The one non-Markdown line in this diff isregistry.rs's own doc comment ("four real worker adapters" → "five"), corrected in the same commit as the architecture document that quotes it verbatim, so the two don't disagree with each other.docs/architecture.md(separate commit): the run-state table's success row, and the sentence directly beneath it, both encoded the same wrong assumption — that a clean process exit means success, and that a vendor process never exits on its own. Pre-existing, unrelated to the new adapter; found while reading the same document for another reason and fixed together since they are one error stated twice.docs/architecture.md(separate commit): two sentences citing a local, gitignored file for open defects — a citation this repository's own conventions already rule out elsewhere. Replaced with a plain statement that open defects are tracked outside this repository.docs/code-walkthrough.md: the same adapter-count corrections as the architecture document, plus the new adapter's own module rows and test suite added to their respective lists.docs/development.md: added the new adapter's test suite to the coverage list.docs/adr/README.md: two index Status cells marked "under evaluation by a pending ADR," plus one sentence explaining why, with no ADR body edited (write-once).docs/future-features.md: shrank a deferred-feature entry that had partly happened — one vendor now has exactly the non-interactive control plane the entry described as a future trigger, for a different reason than the entry anticipated. The historical paragraph describing the pre-existing state is untouched.crew-orchestration,crew-approvals,crew-troubleshooting) anddocs/cli-reference.md: all taught or demonstrated only the terminal mode, with one skill's own closing line actively discouraging a reader from ever naming a mode at all. Added that the alternative exists, how it's requested, that omitting it still selects the terminal mode, and the experimental caveat.docs/user-guide.mdandpackages/extension/skills/crew-troubleshooting/SKILL.md: no document stated the workspace-trust precondition the new mode depends on. Added it as three separately-sourced statements: the user-facing requirement, the vendor's own trust-keying rule (repository root; a worktree uses the main checkout's root), and nothing about how this repository's own code resolves that key today, since that mechanism is a separate, open question.packages/extension/skills/crew-recovery/SKILL.md: the resume-eligibility bullet named two of three possible outcomes; added the third.docs/compatibility.md: one vendor's known-version table listed three entries while this same document's own gate table already listed five, and the currently-installed release sits outside both — brought the first table to match the second and stated the release is refused pending a verified handshake. No gate or allowlist code changed. Also dated a much older, already-historical conformance matrix explicitly.docs/rust-tutorial.md: a hardcoded line-count and module-count had drifted far from the current tree (one by a factor of five); replaced the line-count claim with the same growth-resistant phrasing the same document already uses two paragraphs earlier, and updated the module count and name list. Added the new adapter's test suite to the existing test-coverage paragraph.release/live-e2e-runbook-0.7.0.md,release/checklist-0.7.0.md): both are historical by design and stay that way, but each opened with a present-tense claim a reader today would act on and be wrong about (a closed gate, a specific attempt count, a specific vendor version). Bracketed each as a point-in-time record rather than editing the historical content itself.fixtures/adapters/tui-screens/README.md: one claim was stated as universal when it is true of only two of four vendors (the document itself corrects this further down); a second described a piece of follow-up work as imminent when it was dropped, without saying so. Scoped the first and reworded the second to describe the pin as standing until the question is re-decided, without a date or an identifier.AGENTS.mdandCLAUDE.md: the bracketed-paste invariant is stated as if exhaustive; a worker driven over a protocol writes no prompt bytes to a PTY at all, so the invariant has nothing to say there. Scoped both.Gates
bun run check— clean (schema/bindings drift, format, typecheck, extension build,bun test, fullcargo test --workspace)grep -rn 0037 .(excluding.git, build output) — emptymain— 20 files touched, matching exactly the set of findings that needed a change (a larger internal review table also recorded every file checked and found to need no change, so the diff is neither more nor less than what was decided)docs/adr/0*.mdchanged (only the index,docs/adr/README.md— two Status cells and one sentence)crates/protocol/changedNotes