docs: correct CLI contract drift on the --json surface - #85
Conversation
01e7565 to
809893b
Compare
dichovsky
left a comment
There was a problem hiding this comment.
Routine review. Roster per .github/ISSUE_TRIAGE.md: the general code reviewer (always), plus — the diff being docs-only — a documentation-accuracy reviewer verifying the new prose against the code it describes and against the authority order, in place of the TypeScript reviewer. No security reviewer: the diff touches no src/store/, src/process.ts, src/which.ts, src/fs-safe.ts, src/setup/, or the Launcher.
CRITICAL: none. HIGH: 1. MEDIUM: 4. LOW: 9. One fix pass is being taken; what it does and does not cover is listed at the end.
HIGH
1. docs/design/cli-contract.md:767-769 — the human-output bullet this PR edited makes two false claims about crew ui. The bullet reads "crew ui prints the authenticated local-only URL and whether it opened the browser … --no-open reports that the browser was not opened", to which this PR appended "; --json opens no browser at all". Only the appended clause is true. renderUiStarted (src/format.ts:326-334) prints exactly two lines — Console listening at <url> (workspace <path>) and the token warning — and neither mentions the browser; runUi then calls the opener silently and swallows its failure with no output (src/ui/index.ts:118-124). crew's human surface never reports whether the browser was opened, and --no-open produces no output of its own.
MEDIUM
2. cli-contract.md:63-73 — the rewritten doctor --system bullet omits VERSION_FLOOR, the one system finding this same PR adds to the closed list. The bullet enumerates the DEPENDENCY_MISSING and SETUP_DRIFT cases but not installed-but-below-minimum, though it is the passage a reader consults to learn what --system reports.
3. cli-contract.md:570-572 — the RESUME_DRIFT description states a stronger guarantee than the code delivers. It says the finding reports "a cleanly stopped session … which team resume can no longer relaunch". Doctor never parses or validates the clean-stop marker (src/doctor.ts:406-417), and listResumableSessions returns [] whenever any crew-owned tmux session is live (src/launcher/resume.ts:143-146), whereas runTeamResume refuses only when a session of that name is live (:195-200). With one Crew running, every other stopped session is flagged RESUME_DRIFT even though crew team resume <session> would relaunch it. Overstating in a level-3 contract document is a real defect.
4. cli-contract.md:376-378 vs :480 — the LAUNCH_FAILED scope widening is only half-applied. The table row now names Console startup failure, but the crew ui section still says an invalid or unavailable explicit port "makes the command fail" without naming a code — the same omission the issue complained about for join/UNSUPPORTED_PLATFORM, which this PR did fix. It also elides that the two cases differ (USAGE for an invalid port value, LAUNCH_FAILED for an unavailable one).
5. architecture.md:155 + PR body — the POST /api/team/resume carve-out is honestly disclosed, but its stated authority rationale is wrong, and I wrote it. The PR body justifies the narrowing with "ADR-0012 (level 1) … still exclude[s] it". But docs/adr/0017-console-now-view-theme-and-agent-archive.md:76-79 — also accepted, also level 1, and later — says "The Console's action surface is no longer limited to Message/Task/Team/maintenance operations (FR-U19 amended); reviewers of future Console changes should treat FR-U19's enumeration, not 'whatever's already wired,' as the actual boundary". ADR-0017 therefore supersedes ADR-0012's closed list and delegates the boundary to FR-U19 — and since #64 merged, FR-U19 (srs.md:806-811) reads "…stopping a Team, resuming a Team, peeking at a pane…". The live conflict is not ADR-vs-route; it is cli-contract.md:400-402's "and nothing else" (level 3) contradicting FR-U19 (level 2), which by the authority order makes the CLI contract the bug. The carve-out itself stands on its own terms — that line enumerates GET routes and lumps every write endpoint as "the action POSTs", so no POST belongs there — but the reasoning offered for it was stale and a reviewer who accepted it as written would cement a wrong authority reading. This also means #25 is closer to resolved than the earlier retraction on that issue concluded.
LOW
cli-contract.md:350-358— theteam resumebullet does not name its--jsonrecord, breaking parity with theteam stopbullet ("--jsonemits onestop_resultrecord").cli-contract.md:480— the widenedLAUNCH_FAILEDcell is the only table cell containing two sentences; "the owned session is torn down. Also the Console server failing to start" can be misparsed as teardown applying to the Console.cli-contract.md:569-570— "theverified_ondate that minimum was checked" is ungrammatical.cli-contract.md:572-573— the reflow leaves a short line mid-paragraph in a file otherwise filled to ~90 columns.cli-contract.md:386— "the machine surface" is a coinage used nowhere else indocs/; the house phrasings are "machine output" and "the--jsonform … is the contract for machines".cli-contract.md:282-285— the implies-launch bullet's second sentence restates the first, and "launch-only flag" is a new term where the file says "launch flags".cli-contract.md:564— "Six of them" hard-codes a count into a closed list, adding a second thing to update whenever a finding code is added.architecture.md:155— the GET enumeration this PR rewrote still omits/api/events(src/ui/server.ts:253-258, handled at:578-580); the list names four of six GET routes even after the fix.architecture.md:157— the adjacentactions.tsrow lists handlers but omitslistResumableTeamSessions(src/ui/actions.ts:455), so the newly documented route has no handler named one line below. Pre-existing non-exhaustiveness (it also omitspeekPane/archiveAgent/restoreAgent).src/doctor.ts:92— source-side, not this PR's prose: the comment oncollectParticipantFindingsstill says "Version is not a doctor finding — it is surfaced bycrew setup", whichcollectVersionFloorFindings(:114-143) has since falsified.
Verified correct (not re-listed as findings)
The documentation reviewer independently re-derived, rather than trusting the PR body: the seventeen-code finding list is exactly what src/doctor.ts can emit — including the pushConfigReadFailure ternary at :340-341 — with nothing missing and nothing listed that doctor cannot emit; the six codes named as shared are exactly the intersection with the ErrorCode union (src/errors.ts:8-26), and all eleven others genuinely have no counterpart. Both new details shapes and severities match. The .crew/generated/<session>/resume.json path is correct against src/doctor.ts:401-409 and src/launcher/artifacts.ts:51-53. The resume_result example matches renderTeamResumeResult (src/format.ts:288-296) field-for-field, panes/relay/attached are literally the same LaunchResult object launch_result renders, and "exactly one … before attaching" holds (src/launcher/session.ts:285-293). --json does suppress the opener independent of --no-open. The Node passage checks out end to end: NODE_FLOOR = '24.15.0', enforced by assertNodeFloor before the app graph is imported, failing with a bare message and exit 1 and no [CODE] prefix. All seven implies-launch flags were confirmed empirically against the vendored commander 15.0.0, including the three negated ones, with --client/--json correctly not implying it. Both error-table rows and the new Agent-lifecycle sentence are reachable as described. Scope discipline is clean: no drive-by polish, no reformatting of untouched paragraphs, and CONTEXT.md vocabulary is used correctly throughout. FR-U49 independently specifies the same resume_result shape this PR documents.
What the fix pass covers
Taking findings 1, 2, 3, 6, 7, 8, 9, 10, 11, 12. Finding 1 is resolved by reverting the appended clause rather than correcting the bullet: the false pre-existing claims are a distinct defect #28 does not name — they concern what human output contains, not when the browser opens — so the scope rule sends them to their own issue rather than into this diff. The authoritative in-scope statement remains at :386-387.
Not folded in, filed separately instead: findings 4, 13, 14, 15. Finding 5 needs no code change — the carve-out stands; the PR body's rationale is corrected in place.
Generated by Claude Code
The `--json` output is a declared stable contract, so scripts written against the documented vocabulary must be able to trust it. Six statements had drifted from the implementation, each of which would mislead such a script: - The doctor finding-code list omitted `VERSION_FLOOR` and `RESUME_DRIFT`, and claimed every finding code reuses the error-code vocabulary — only six of the seventeen have an `ErrorCode` counterpart. - The `resume_result` envelope emitted by `crew team resume --json` was undocumented, and `/api/resumable-sessions` was missing from the Console route list in architecture.md. - `crew ui --json` never opens a browser, but the contract attributed that skip to `--no-open` alone. - `doctor --system` was said to check Node; it does not, and no requirement asks it to — the `>=24.15` floor is enforced pre-command by the executable shim, so the contract now says that instead. Its finding list also omitted the `VERSION_FLOOR` case the same command reports. - Every launch flag implies `--launch` (so `crew team dev --print` is valid), which the contract documented only inside the `--launch` form. - `UNSUPPORTED_PLATFORM` also covers `crew join --platform <unknown>` and `LAUNCH_FAILED` also covers a Console server that fails to start; neither cause appeared in the meaning table. Per the authority order in docs/README.md the docs are the bug in all six cases, so this corrects the documents and leaves the code untouched. Closes #28 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
809893b to
5326a45
Compare
Closes #28
docs/design/cli-contract.mdis authority level 3 and the--jsonsurface it describes is a declared stable contract, so a wrong statement in it is load-bearing: scripts written against the documented vocabulary mishandle real finding codes, an undocumented record type, two error-code causes, and a mispredicted browser side effect. All six drifts in the issue reproduce on currentmain. Per the authority order indocs/README.md, the code is the correct side of each — noFR-*/NFR-*requires adoctorNode finding (FR-K01,srs.md:706-709, enumerates diagnostics generically and names none) and none governs the browser opener — so this is a documentation correction with no behavior change.What changed and why
1. Doctor finding codes —
cli-contract.md:559-577. The closed list gainedVERSION_FLOORandRESUME_DRIFT, with bothdetailsshapes documented in the register the neighbouringSETUP_DRIFTsentence already uses:VERSION_FLOORiswarnwithtarget/detected/minimum/verified_on(src/doctor.ts:131-141);RESUME_DRIFTiswarnwithsession(src/doctor.ts:411-416). Thedoctor --systembullet (:66-71) also gained theVERSION_FLOORcase, since that is the passage a reader consults to learn what--systemreports.The "reuses the error-code vocabulary" sentence was rewritten, and it was materially more wrong than the issue alleges.
doctoremits seventeen codes; onlyDEPENDENCY_MISSING,UNSUPPORTED_SCHEMA,INTEGRITY,TEAM_DRIFT,INVALID_CONFIG, andUNSAFE_PATHexist in theErrorCodeunion (src/errors.ts:8-26). The rest are diagnostic-only, so the old claim was false for eleven codes, not just the two #28 names.RESUME_DRIFT's description states only what doctor observes:collectResumeFindings(src/doctor.ts:399-417) checks for the file's existence and never parses or validates the clean-stop marker, andlistResumableSessions(src/launcher/resume.ts:143-146) returns[]whenever any crew-owned session is live, whilerunTeamResume(:195-200) refuses only for a live session of that name. So the finding cannot promise thatteam resumewould fail, and the prose no longer claims it does.2.
resume_resultenvelope —cli-contract.md:691-698, immediately after thestop_resultexample. Field names and order matchsrc/format.ts:289-296(renderTeamResumeResult,:278) exactly, andFR-U49independently specifies the same shape. Theteam resumebullet (:358) now names the record, for parity with theteam stopbullet. The command itself was already specified at:343-351— only the--jsonenvelope was missing.3. Console route list —
docs/design/architecture.md:155. AddedGET /api/resumable-sessions(src/ui/server.ts:258, handled at:606) to the enumerated GET routes. See Out of scope for why the resume POST is not named.4.
crew ui --jsonopener —cli-contract.md:385-390.src/ui/index.ts:118gates the opener on!options.json && options.open, so--jsonalone never opens a browser; the contract attributed the skip to--no-open.5. Implies-launch flags —
cli-contract.md:283-285. All seven flags (--workers,--task-file,--worktree,--no-worktree,--no-relay,--no-attach,--print) carry.implies({ launch: true })atsrc/cli.ts:288,:293,:298,:306,:308,:310,:313, socrew team dev --printis valid — the spellingREADME.md:104already uses. Confirmed empirically against the vendored commander 15.0.0, including the three negated flags, with--client/--jsoncorrectly not implying it. The grammar block at:266-268is untouched: it shows the flags under the canonical--launchform, which is a subset, not a contradiction.6. Error-code scopes —
cli-contract.md:474(UNSUPPORTED_PLATFORMnow names thecrew join --platformvalue,src/agents.ts:34-39) and:481(LAUNCH_FAILEDnow names a Console server that fails to start,src/ui/index.ts:108). The Agent-lifecycle section did omit the code, as the issue says; it is added at:118-119.Also, the
doctor --system/ Node row —cli-contract.md:63-71.SYSTEM_DEPENDENCIES = ['tmux', 'git'](src/doctor.ts:49) and no Node finding exists, so "Node" is dropped and the passage states that the>=24.15floor is enforced pre-command by the installed shim (assertNodeFloor,bin/crew.ts:25, before the dynamicimport('../src/run.js');NODE_FLOOR = '24.15.0', bare message, exit 1, no[CODE]prefix). This is the Acceptance's second branch. No Node check was added tosrc/doctor.ts.Verification
Tests: none added or updated. Docs-only — two Markdown files, zero code — explicitly exempt under
CONTRIBUTING.md("tests are not applicable for docs-only changes") and.github/ISSUE_TRIAGE.md.Run under Node 24.19.0 with the pinned toolchain, on the final pushed tree (rebase onto
origin/main@97d7f88was a no-op — the branch was already on the tip):npm run typechecknpm run lintnpm run format:checknpm run builddist/+dist/ui-assets/main.jsnpm run test:coveragePre-existing failure proof. The two failures are
tests/integration/commands/doctor.test.ts:319and:373. Bothchmod 000a directory and expect the read to fail, which has no effect for uid 0; this sandbox runs as root. Proven rather than assumed: the change was stashed,git status --porcelainconfirmed empty, and the file re-run on the clean tree produced the identical 2 failed / 31 passed.Because that run failed, vitest suppressed the coverage table — the 95% thresholds were not evaluated locally. This PR's CI run (Node 24.18.0, non-root) is authoritative. The diff touches no
src/**orbin/**file, so coverage cannot have moved.Note on wrapping: the repo sets no
proseWrap, so Prettier defaults to"preserve"and never reflows Markdown —format:checkpasses either way. The finding-codes paragraph was hand-wrapped to the file's ~95-column convention; the word-level diff confirms the adjacentSETUP_DRIFTprose is re-wrapped only, with no content change.Review
Two reviewers ran per
.github/ISSUE_TRIAGE.md: the general code reviewer, and — the diff being docs-only — a documentation-accuracy reviewer verifying the prose against the code it describes, in place of the TypeScript reviewer. Findings are posted in full on this PR: no CRITICAL, 1 HIGH, 4 MEDIUM, 9 LOW. The one allowed fix pass was taken and covered the HIGH plus nine other items; the HIGH was resolved by reverting this PR's edit to the human-output bullet at:768-769(now byte-identical tomain), because the false claims it carries are a distinct defect #28 does not name. Nothing CRITICAL or HIGH survives.Out of scope
Deliberately untouched:
POST /api/team/resumeis not named inarchitecture.md:155— that line enumerates GET routes and lumps every write endpoint as "the action POSTs", so no POST belongs there regardless of how the FR-U19 question resolves. (An earlier draft of this body justified the omission by citing ADR-0012's closed action list. That reasoning was stale and is retracted:docs/adr/0017-…:76-79supersedes ADR-0012 on this point and delegates the boundary to FR-U19, which since docs: authorize the Console team-resume action in FR-U19 #64 merged reads "…stopping a Team, resuming a Team…". The live conflict iscli-contract.md:400-402's "and nothing else" against FR-U19 — a level-3 doc contradicting level 2 — which is Console: team-resume routes exceed FR-U19's closed action list (companion to #3) #25's remaining scope, not this PR's. Correction also posted on CLI contract drift: doctor finding codes, resume_result, ui --json opener, implies-launch flags, error-code scopes #28.)src/launcher/resume.tsis absent fromarchitecture.md:144-153'slauncher/tree — a missing file in §4.1, which is architecture.md §4.1 source tree is substantially stale #35's scope.RESUME_DRIFTomission (docs/design/srs.md:706-709) — Docs: FR-K01's doctor diagnostic list omits RESUME_DRIFT #52.srs.mdis not touched here.messages_deletedsentence atcli-contract.md:584-585— CLI contract: messages_deleted is described as counting standalone read Messages, but prune deletes every old read Message #66.FR-*/NFR-*id, CLI grammar rule, record type, error code, command, or flag is added, removed, or given new behavior; no ADR is amended.CURRENT_SCHEMA_VERSIONstays 7.Distinct problems found during implementation and review, filed rather than folded in:
crew uihuman-output bullet claims browser reporting no code emits (renderUiStartedprints two lines, neither about the browser; the opener's failure is swallowed silently), and the port-failure sentence names neitherUSAGEnorLAUNCH_FAILED.architecture.md:155still omitsGET /api/events, and theactions.tsrow at:157omitslistResumableTeamSessions(pluspeekPane/archiveAgent/restoreAgent).src/doctor.ts:92still says "Version is not a doctor finding", whichcollectVersionFloorFindingsfalsifies.Related open PRs
None. Checked at file level: no other open PR touches
docs/design/cli-contract.mdordocs/design/architecture.md. (#81 —EXAMPLES.md,src/ui/server.ts— merged intomainbefore this branch's final rebase.)