Skip to content

docs: correct prune counting and crew ui reporting in the CLI contract - #115

Draft
dichovsky wants to merge 1 commit into
mainfrom
docs/cli-contract-prune-and-ui
Draft

docs: correct prune counting and crew ui reporting in the CLI contract#115
dichovsky wants to merge 1 commit into
mainfrom
docs/cli-contract-prune-and-ui

Conversation

@dichovsky

Copy link
Copy Markdown
Owner

Closes #66
Closes #88
Closes #90

docs/design/cli-contract.md is authority level 3 and the surface it describes is a declared stable contract, so a wrong statement in it is load-bearing. Two of the three drifts here are exactly that: a reader who believes a Task-linked Message only ever disappears together with its Task mis-models retention (messages_deleted is the field EXAMPLES.md recommends scripting against), and a reader who scripts around "crew reports whether it opened the browser" is building on output no code emits. The third is a source comment in src/doctor.ts that contradicts the function twenty lines below it, in the one file a contributor reads to learn what doctor reports.

All three reproduce on main @ 09e67cd, verified against the code rather than taken from the issue bodies. Per the authority order in docs/README.md the code is the correct side of each: no FR-*/NFR-* requires the documented behavior — FR-K02/FR-K04 (srs.md:709-718) specify prune eligibility and "report deletion counts" without prescribing how the counts partition, and no FR-U01FR-U11 mentions the browser opener at all. So this is a documentation/comment correction with no behavior change.

What changed and why

Location Issue Change
cli-contract.md:605-612 #66 The messages_deleted sentence said "(Task-linked ones plus standalone read Messages)". pruneState (src/store/maintenance.ts:166-190) has no task_id IS NULL predicate on step 4 — DELETE FROM messages WHERE read_at IS NOT NULL AND created_at < ? (:183-185) removes every remaining old read Message, including ones still linked to a Task that survived the prune (too new, or a sibling Message on it is still unread, per ELIGIBLE_TASKS at :155-157). The sentence now describes both halves of the sum and states the consequence a script author needs: an old read Message can disappear while its Task stays.
cli-contract.md:772-776 #88 (1) The human-rendering bullet claimed crew ui prints "whether it opened the browser" and that --no-open "reports that the browser was not opened". renderUiStarted (src/format.ts:315-333) prints exactly two lines — Console listening at <url> (workspace <path>) and the secret-token warning — and neither mentions the browser; --no-open only skips the opener call (src/ui/index.ts:118-124) and emits nothing of its own. The bullet now states the two lines actually printed and that nothing reports the browser outcome.
cli-contract.md:387-393 #88 (1) The crew ui section's human-output bullet is where a reader looks for opener behavior, so the silent-failure-by-design fact lands here: runUi awaits the opener inside a try/catch {} that swallows the failure so a dead open/xdg-open cannot kill a running Console (src/ui/index.ts:118-124). Nothing was printed about it before and nothing is now — that was undocumented, not just unstated.
cli-contract.md:378-381 #88 (2) The port bullet said an "invalid or unavailable explicit port makes the command fail" without naming a code. The two failures differ: resolvePort (src/ui/index.ts:54-61) throws USAGE for a non-decimal or out-of-range value; an unavailable port surfaces from startUiServer as LAUNCH_FAILED (:100-108), never a fallback bind (FR-U03). Both are now named, matching the meaning table at :484, which PR #85 had already corrected on the LAUNCH_FAILED side. The section's closing "introduces no new error code" claim (:432-433) stays true — both codes are pre-existing.
src/doctor.ts:89-93 #90 The collectParticipantFindings doc comment ended "Version is not a doctor finding — it is surfaced by crew setup." collectVersionFloorFindings (:114-143) pushes a warn finding with code: 'VERSION_FLOOR' and details { target, detected, minimum, verified_on } (:131-141) — confirmed present and emitted at current main, not inferred from the issue. The comment now scopes this function to presence and points at the function that does report version drift. Comment only: no change to collectParticipantFindings' behavior, return value, or any finding it emits.

Deltas against the issue bodies — noted rather than silently absorbed

  1. CLI contract: messages_deleted is described as counting standalone read Messages, but prune deletes every old read Message #66's line numbers moved, and the pre-flight report that it might already be fixed was wrong. The issue cites cli-contract.md:584-585; the text sits at :604-605 on current main (six PRs merged into the file's neighbourhood on 2026-08-06). An audit of a sibling PR reported that :604 already read the corrected form. It does not — :604 read the defective form the issue quotes, verbatim, and the diff here shows it being removed. CLI contract: messages_deleted is described as counting standalone read Messages, but prune deletes every old read Message #66 reproduces in full.
  2. CLI contract: messages_deleted is described as counting standalone read Messages, but prune deletes every old read Message #66's mechanism is broader than the issue's headline. The issue frames the defect as step 4 lacking a task_id IS NULL predicate. The other half also went undocumented: step 2 (DELETE FROM messages WHERE task_id IN (...), maintenance.ts:174) carries no created_at filter, so Messages on a pruned Task are removed whatever their age, entirely independent of the Message cutoff. The replacement states that explicitly rather than only fixing the half the issue names.
  3. CLI contract: the crew ui section describes browser reporting no code emits and omits its failure codes #88's line numbers moved:768-769 on main:772-776 after this change. PR docs: correct CLI contract drift on the --json surface #85 touched this bullet and then reverted its edit (its own body records the revert as the resolution of a HIGH review finding), so the text is byte-identical to what CLI contract: the crew ui section describes browser reporting no code emits and omits its failure codes #88 quotes.
  4. CLI contract: the crew ui section describes browser reporting no code emits and omits its failure codes #88 part 2 asked for a mapping the code confirms but the issue understated. LAUNCH_FAILED is not raised for "a port that is merely unavailable" in general — it is the fallback for any non-CrewError thrown by startUiServer, with EADDRINUSE the motivating case (src/ui/index.ts:100-108). The wording added ("fails as LAUNCH_FAILED when the server cannot bind") describes the bind failure without over-narrowing the code to that one cause.
  5. Doctor: collectParticipantFindings comment says version is not a doctor finding, which VERSION_FLOOR falsifies #90 is accurate as filed, including the :92 line number, the warn severity, and all four details keys. No delta.
  6. No sibling occurrence of any corrected claim survives. grep -n "standalone" docs/design/cli-contract.md → no hits after the change; grep -rn "not a doctor finding\|surfaced by \crew setup`" src/ docs/→ no hits;grep -rn "whether it opened the browser|reports that the browser" docs/ README.md EXAMPLES.md→ no hits.docs/design/data-model.md:527-531` already described step 4 correctly ("the remaining old read Messages") and is untouched — the two level-3 documents now agree.

Wrapping: Prettier is proseWrap: "preserve" and never reflows Markdown, so format:check passes either way and wrapping is hand-maintained. New lines are 90-93 columns, inside the file's existing band. The prune_result paragraph was re-wrapped end-to-end because the inserted clause would otherwise have left a 21-column orphan line; the word-level diff shows the --vacuum tail is re-wrapped only, with no content change. No other paragraph was touched.

Verification

Tests: none added or updated — claiming the docs-only/comment-only exemption explicitly. The diff is one Markdown file plus a comment-only hunk in src/doctor.ts; CONTRIBUTING.md and .github/ISSUE_TRIAGE.md both grant that exemption, and it is stated here as they require. Proven, not assumed, that nothing guards what changed: grep -rn "cli-contract" tests/ returns one hit, tests/unit/format-edge.test.ts:214, a comment about timestamp rendering that reads no doc file. tests/unit/docs-facts.test.ts derives docs-site/generated/facts.json from src/platforms/registry.ts, src/store/schema.ts, src/cli.ts, package.json, vitest.config.ts, and ADR H1 headings — never cli-contract.md and never a src/doctor.ts comment — so no regeneration was needed, and the suite is run below to prove it.

Run on the final pushed tree (rebase onto origin/main @ 09e67cd was a no-op — the branch was already on the tip), Node v24.19.0, uid 501 (not root):

Command Exit Result
npm run typecheck 0 clean, all three projects
npm run lint 0 clean
npm run format:check 0 "All matched files use Prettier code style!"
npm run build 0 dist/ + dist/ui-assets/
npm run build:docs 0 clean
npx vitest run tests/integration/commands/doctor.test.ts tests/integration/commands/doctor-edge.test.ts tests/unit/docs-facts.test.ts 0 3 files passed, 42 passed
npm run test:coverage 1 1389 passed, 11 skipped, 1 failed file — pre-existing and environmental

The doctor suites were run despite the change being a comment, because the diff touches src/doctor.ts. Both files pass here: this machine is uid 501, so the two chmod 000 tests that fail for root elsewhere are not in play.

Pre-existing-failure proof (performed, not assumed). The single failure is tests/integration/package/pack-smoke.test.tsTypeError: Cannot read properties of undefined (reading 'files') at :40, i.e. npm pack --json no longer returns the array shape the test destructures. This box runs npm 12.0.2. Proven by git stash push -u -- docs/design/cli-contract.md src/doctor.ts, confirming git status --porcelain showed only the untracked node_modules symlink and git diff origin/main --stat was empty, then re-running that file on the clean tree: identical failure, same line, same message. git stash pop restored the change. A Markdown-and-comment edit cannot influence an npm pack output shape, and CI pins its own npm.

Coverage thresholds were NOT evaluated locally. test:coverage exited non-zero, so vitest suppressed the coverage table and no claim is made about the 95% numbers. This PR's CI run is authoritative. The only src/** change is a doc comment, which cannot move a coverage number.

Out of scope

Deliberately untouched, owned by other issues or concurrent PRs:

Cross-PR dependency: none. No out-of-scope file needed to change for any of the three fixes.

Related open PRs

None touching the same files. gh pr list --state open --json number,title,files shows #110, #111, #112, #113, #114 open; none includes docs/design/cli-contract.md or src/doctor.ts. The nearest neighbours are #111 (docs/design/data-model.md, the document this change is being reconciled with — it is already correct on the prune step and is not edited here) and #112 (docs/design/srs.md, consulted read-only for the authority check).

The CLI contract is authority level 3 and the surface it describes is a
declared stable contract, so a wrong statement in it is load-bearing:
a reader who believes Task-linked Messages only ever vanish with their
Task mis-models retention, and one who scripts around "crew reports
whether the browser opened" is building on output no code emits.

The doctor comment is a source comment contradicting the function
eighteen lines below it, in the one file a contributor reads to learn
what doctor reports.

No behavior changes: the code is correct on all three points and no
FR-* requires otherwise, so the prose is the side that moves.

Closes #66
Closes #88
Closes #90
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment