Skip to content

docs: correct drifted command and Io-seam facts in agent guides - #101

Open
dichovsky wants to merge 2 commits into
mainfrom
docs/agent-guide-command-annotations
Open

docs: correct drifted command and Io-seam facts in agent guides#101
dichovsky wants to merge 2 commits into
mainfrom
docs/agent-guide-command-annotations

Conversation

@dichovsky

@dichovsky dichovsky commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Closes #41

CLAUDE.md and AGENTS.md are the files an agent reads before doing anything else, and five of their claims are falsifiable and false. That is a different failure mode from ordinary prose drift: an agent that trusts them skips build:docs failures, typechecks one project instead of three, or "improves" coverage of a file the gate deliberately excludes. Every edit here restates what package.json, .github/workflows/ci.yml, vitest.config.ts, and src/io.ts already do — the code is the correct side of all five.

All five reproduce on main @ 97d7f88, verified directly rather than from the issue body.

Updated after review. The fix pass (aeac101) added a third file, CONTRIBUTING.md, and reworked two of the original edits. See Review below; the sections here describe the final state.

What changed and why

CLAUDE.md and AGENTS.md (identical hunks in both)

Location Change
:57-58 npm run build — the script is tsc -p tsconfig.build.json && npm run build:web; the annotation named only the tsc half, so the esbuild bundle into dist/ui-assets/ was invisible. npm run typecheck — the script runs three tsconfigs (tsconfig.json, web/tsconfig.json, docs-site/tsconfig.json); the annotation named one. All three are noEmit, so the annotation now says so rather than attaching it to the root project alone.
:58 (new line) A npm run build:docs entry was added to the block. The prose below now names it as a gate to reproduce locally, and no listed command ran it — test:coverage does not.
:71-76 The CI paragraph listed five gate steps and omitted npm run build:docs (ci.yml:34), which sits between build and test:coverage. Now six, in ci.yml's actual order, scoped to the build-test job — ci.yml also defines publish-dry-run, which runs on every PR and is now named.
:87-95 The Io bullet named six of ten members and then asserted "Everything crew touches in the environment is one of these fields". Completing the enumeration was not enough to make that sentence true — the filesystem is not in Io; 21 src/ modules import node:fs directly. The bullet now leads with the scoped invariant (every process-environment input), states the filesystem exclusion and that a stubbed Io therefore does not isolate a test from disk, names src/fs-safe.ts / src/workspace.ts as the path policy, and then enumerates all ten.
:181-184 The Tests section's coverage-gate sentence gave the include list (src/**, bin/**) without the exclusion at vitest.config.ts:13 (src/io.ts).

CONTRIBUTING.md

Location Change
:36 The prose CI step list said five steps ending in npm test. Now six in ci.yml's order, ending in npm run test:coverage — the distinction is load-bearing, since test:coverage is what enforces the thresholds.
:99 The "Before you open a PR" checklist carried the same five-command list. Same correction.

Three deltas against the issue body, noted rather than silently absorbed:

  1. The coverage exclusion is vitest.config.ts:13, not :14 (:14 is reporter).
  2. The Io interface opens at src/io.ts:33, not :46; the range the body cites (:46-65) is the four omitted members.
  3. The Io drift is in both files, not just AGENTS.md. diff <(tail -n +2 CLAUDE.md) <(tail -n +2 AGENTS.md) returns exactly one hunk — line 2, "guidance to Claude Code (claude.ai/code)" vs "guidance to coding agents". Everything else is byte-identical. So every edit landed in both files at the same lines, and that invariant was re-checked after each pass and after each rebase: still exactly the one line-2 hunk.

Wrapping: Prettier is configured proseWrap: "preserve" and never reflows Markdown, so format:check passes either way and the wrapping is hand-maintained. Max line length is unchanged at 102 (CLAUDE.md) / 100 (AGENTS.md), on pre-existing lines, inside the files' existing band.

Verification

Tests: none added — claiming the docs-only exemption. The diff is three Markdown files and zero code, which CONTRIBUTING.md:95 ("tests are not applicable for docs-only changes") and .github/ISSUE_TRIAGE.md ("Tests are not required for docs-only, config-only, or pure-UI-only changes — say so explicitly in the PR body when claiming that exemption") both grant.

Nothing guards any of the three files. grep -rn "CLAUDE.md\|AGENTS.md" tests/ returns only tests/integration/commands/init.test.ts, which writes and reads a CLAUDE.md inside a temporary fixture cwd to exercise crew init's guide-append path — it never reads the repository's own copy. tests/unit/docs-facts.test.ts derives from src/platforms/registry.ts, src/store/schema.ts, src/cli.ts, package.json, and ADR H1 headings, never these files, so docs-site/generated/facts.json needed no regeneration. That absence of any guard is itself now filed as #103.

Run on the final pushed tree, post-rebase, under Node 24.18.0 (CI's pinned version):

Command Exit Result
npm run typecheck 0 clean, all three projects
npm run lint 0 clean
npm run format:check 0 clean
npm run build 0 dist/ + dist/ui-assets/
npm run build:docs 0 clean — run because this PR now documents it as a gate
npm run test:coverage 1 1395 passed, 3 skipped, 2 failed — both pre-existing and environmental

Pre-existing-failure proof (performed, not assumed). The two failures are tests/integration/commands/doctor.test.ts → "degrades a raw filesystem read failure in project roles instead of aborting doctor" and "degrades unreadable roles and teams directories to whole-listing warnings". Both chmod 000 a directory and expect the read to fail; this container runs as uid 0 (id -u0), which ignores mode bits, so the reads succeed and the expected INVALID_CONFIG warning never fires — the assertion failure is expected undefined to match object { severity: 'warn' }. Proven by git stash push -u, confirming git status --porcelain empty and git diff origin/main empty, re-running on the clean tree, then git stash pop. Both runs identical: 2 failed / 31 passed for that file standalone, and 1 failed file / 110 passed / 2 skipped — 2 failed / 1395 passed / 3 skipped for the full suite. Re-confirmed unchanged in count and identity after the fix pass. A Markdown-only edit cannot influence them, and CI is neither root nor affected.

Coverage thresholds were NOT evaluated locally. Because test:coverage exited non-zero, vitest suppressed the coverage table, so the 95% statements/branches/functions/lines numbers were never displayed and no claim is made about them. This PR's CI run is authoritative. The diff touches no src/** or bin/** file, so coverage cannot have moved.

One environment note for anyone reproducing: the container's default node is v22.22.2, below the repo's >=24.15 floor, and typecheck fails with TS2688: Cannot find type definition file for 'node' until Node 24 is installed and npm ci re-run under it. That is a sandbox property, not a repo defect.

Branch was cut from and rebased onto origin/main @ 97d7f88; both pre-push rebases were no-ops — main had not moved. The fix pass is a separate commit (9d44815aeac101), not an amend. No force-push.

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 and against the authority order, in place of the TypeScript reviewer. No security reviewer: the diff touches nothing under src/store/, src/process.ts, src/which.ts, src/fs-safe.ts, src/setup/, or the Launcher. Findings are posted in full on this PR: 0 CRITICAL, 3 HIGH, 4 MEDIUM, 8 LOW.

The one allowed fix pass was taken and covered all three HIGHs plus two MEDIUMs and three LOWs. The HIGHs were:

  • The Io invariant sentence was still false even with the enumeration completed, because the filesystem sits outside Io entirely — the single most consequential finding, since a reader concludes a stubbed Io isolates a test from the environment.
  • The random gloss said "seeded in tests", which is false of captureIo (tests/helpers/io.ts:80 injects the constant 0.5); only the stress suite seeds a real stream. It also overstated the scope and dropped the production path.
  • The CONTRIBUTING.md deferral. The two reviewers disagreed; resolved against the issue text, whose Acceptance names "(and CONTRIBUTING.md's step count)" explicitly. The deferral also created a live divergence — two files in the repo disagreeing about the same workflow. Folded in, limited to the step count at :36 and :99.

Contract impact

None. CLAUDE.md, AGENTS.md, and CONTRIBUTING.md sit outside the four-tier authority order in docs/README.md:9-12 entirely — they are onboarding and contributor guides that summarize docs/README.md and defer to it. No ADR is amended, no FR-*/NFR-* is added, renumbered, or given new meaning, and no CLI grammar rule, record type, error code, command, or flag moves. package.json is not touched. CURRENT_SCHEMA_VERSION stays 7 — there is no schema change.

Out of scope

Deliberately untouched, and each filed rather than folded in:

Relationship to #78. This PR corrects CONTRIBUTING.md:36 and :99 because #41's Acceptance names that file's step count. #78 covers the same two lines with a tighter specification, so it is substantially addressed here — but this PR does not claim to close it, and whether to narrow or close #78 is left to a human.

Related open PRs

#94 (docs/release-status-0-1-1) also touches CLAUDE.md and AGENTS.md — at :29, the "0.1.0 is published to npm" sentence in the "What this is" section. This PR edits :57-58, :71-76, :87-95, and :181-184. No line overlap, so a conflict is unlikely, but the two must not both be merged without a rebase check — and whichever lands second must re-confirm the CLAUDE.md/AGENTS.md byte-identity invariant, since both PRs edit the pair in parallel.

No open PR touches CONTRIBUTING.md — re-checked at file level after the fix pass brought it into this diff.

The other three open PRs touch none of these files: #97 and #98 (docs/design/product-spec.md, architecture.md, decisions.md), #91 (docs/adr/*, decisions.md), #85 (docs/design/cli-contract.md, architecture.md).

CLAUDE.md and AGENTS.md make falsifiable claims that automation acts on, and
five of them no longer match the repo. An agent reading them would skip
build:docs failures, typecheck only the root project, or "improve" coverage of
a file the gate deliberately excludes.

Corrected against the current sources:

- `npm run build` also runs `build:web` (package.json).
- `npm run typecheck` runs three projects, not one (root, web/, docs-site/);
  the (noEmit, includes tests) fact stays attached to the root project.
- CI runs six gate steps, not five — `build:docs` sits between `build` and
  `test:coverage` (.github/workflows/ci.yml).
- The `Io` bullet enumerated six of ten members, so its "everything crew
  touches in the environment is one of these fields" claim was false; it now
  lists `random`, `runProcess`, `runInteractive`, and `onTransactionStep`
  (src/io.ts:33-66).
- The 95% coverage gate excludes `src/io.ts` (vitest.config.ts:13).

Both files stay byte-identical apart from their title line.

CONTRIBUTING.md's step count is deliberately left alone; issue #78 owns that
file and specifies the change more tightly.

Closes #41

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Da8HvwhguZ4RDpbRphpUy7

@dichovsky dichovsky left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — 2 reviewers, per .github/ISSUE_TRIAGE.md

Roster: the general code reviewer (always) and, 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 nothing under src/store/, src/process.ts, src/which.ts, src/fs-safe.ts, src/setup/, or the Launcher.

Both reviewers verified every new claim against source rather than against this PR's body. Combined: 0 CRITICAL, 3 HIGH, 4 MEDIUM, 8 LOW.

Verified clean by both, explicitly: the mirror invariant holds at identical line numbers in both files (identical hunk headers @@ -54,8 @@, @@ -67,8 @@, @@ -82,10 @@, @@ -173,7 @@; diff <(tail -n +2 CLAUDE.md) <(tail -n +2 AGENTS.md) yields exactly the one 2c2 hunk). The CI step list and order match ci.yml exactly. The coverage sentence matches vitest.config.ts character-for-character in meaning. The build annotation is accurate. All ten Io members are enumerated with none missing and none invented. The runProcess / onTransactionStep glosses are accurate and not overstated. Wrapping stays in band (new prose max 90; fence lines 100/98 against a pre-existing 99 and printWidth: 100). Scope is tight — every edit site traces to one of #41's five bullets.


HIGH

H1 — :91: the sentence the Io fix exists to make true is still false. (general reviewer)

The enumeration is now complete, but the retained next sentence — "Everything crew touches in the environment is one of these fields" — remains untrue for a reason completing the list cannot fix: the filesystem is not in Io. 21 files under src/ import node:fs directly (src/workspace.ts, src/fs-safe.ts, src/config.ts, src/store/index.ts, src/setup/fs.ts, src/which.ts, src/ui/server.ts, src/launcher/*, …). #41's bullet 5 named this sentence as why the omission mattered, so completing the list alone does not discharge it.

Not pedantic: an agent concludes a fully-stubbed Io isolates a test from the environment. It does not, and a test written on that assumption touches the real filesystem. The charitable reading — "environment" meaning narrowly the process environment — was considered and rejected, because the same bullet now covers subprocess spawning, which is outside any narrow reading. The term is never defined in the file. (The sentence is true for env/cwd: no direct process.env/process.cwd() survives in src/.)

H2 — :88: "random … seeded in tests" is false of the helper nearly every test uses. (both reviewers — HIGH for one, MEDIUM for the other)

tests/helpers/io.ts:79-80 is random: () => 0.5 — a constant — under the comment "Deterministic by default; stress tests override with a seeded stream." Only tests/helpers/stress.ts:48 injects a real seeded LCG, from CREW_STRESS_SEED. An agent reading "seeded in tests" goes looking for a seed in captureIo and finds none; one writing a new test may assume varied values where the default is fixed.

Two further problems in the same parenthetical: "the single source of nondeterminism" unqualified overstates src/io.ts:40-46, which scopes it to the Store's contention-retry jitter (consumer: randomizedBackoff, src/store/connection.ts:23); and the gloss silently drops the one fact with production consequences — production wires Math.random, never seeded.

This is precisely the defect class this PR exists to remove.

H3 — deferring CONTRIBUTING.md leaves an item #41's Acceptance names by name, and creates a live divergence. (documentation reviewer; the general reviewer rated this LOW)

The two reviewers disagreed here, so I resolved it against the issue text. #41's Acceptance reads: "the Commands and Io-seam sections in both files (and CONTRIBUTING.md's step count) match the current scripts, workflows, coverage config, and Io interface." The general reviewer's basis for accepting the deferral — that #41 "names only CLAUDE.md/AGENTS.md" — is not accurate; the step count is named explicitly. The documentation reviewer is right on the facts.

And the deferral has a cost this PR created: CONTRIBUTING.md:36 and :99 still say CI runs five steps while CLAUDE.md/AGENTS.md now say six, about the same workflow file, in the same repository. Before this PR all three agreed and were all wrong; after it they disagree. My collision-avoidance reasoning was also speculative — no open PR currently touches CONTRIBUTING.md (checked at file level: #97/#98, #94, #91, #85 touch none of it).

Resolution: fold in the step count at :36 and :99 — exactly what #41's Acceptance names, no more. CONTRIBUTING.md:24-25, which carries the same build/typecheck annotation drift, is not named by #41 and will be filed separately rather than folded in. This overlaps #78, which specifies the same two lines more tightly; #78 is left for a human to narrow or close, and this PR does not claim to close it.


MEDIUM

M1 — :58: the (noEmit, includes tests) parenthetical implies the other two projects emit. (both reviewers) All three set "noEmit": true (tsconfig.json:16, web/tsconfig.json:16, docs-site/tsconfig.json:17); the only emitting project is tsconfig.build.json. Attaching the parenthetical to root alone invites exactly the wrong mental model. Only "includes tests" is genuinely distinguishing. (Minor: "projects" also collides with vitest's projects key.)

M2 — build:docs is named as a gate to "match locally" but is not in the Commands block. (documentation reviewer) The prose now enumerates six steps and says "so match them locally", but the block lists no command that runs build:docs, and test:coverage does not. An agent following the instruction literally cannot reproduce the fifth gate. (build:web needs no entry — build invokes it and the annotation says so.)

M3 — the new "excluding src/io.ts" now disagrees with the SRS, which outranks these files. (documentation reviewer) docs/design/srs.md:1179-1181 (NFR-MNT-01) states the gate over src/** and bin/** with no exclusion, and docs/design/testing-strategy.md:28-29 repeats it. Not folded in, deliberately. By the authority order the SRS (rank 2) outranks vitest.config.ts, which means the honest reading is not "the SRS prose is stale" but "either the NFR must be amended or the exclusion must go" — a contract decision, and .github/ISSUE_TRIAGE.md routes those away from the routine. The disagreement pre-exists this diff; the diff only makes it visible. Filed as its own issue.

M4 — docs/design/srs.md:1217 still specifies the automated gate as five commands (… && npm test), with no build:docs and npm test rather than test:coverage. (documentation reviewer) Same authority-order reasoning as M3, same handling: filed, not folded in.


LOW

L1 — :71 "All six must pass" omits the second job in the same workflow. (both) ci.yml defines build-test (the six steps) and publish-dry-run, which also runs on every PR. The old text had the same shape, so not a regression — but the newly explicit enumeration reads as exhaustive. Scoping it to the build-test job is cheap. Taking this.

L2 — the runInteractive gloss drops the contrast that justifies the seam. (documentation reviewer) Beside "runProcess (capture-only, timeout-bounded)", "one foreground stdio-inheriting child" invites the inference that it is bounded too; src/io.ts:12-18 and :59-64 are emphatic that it is unbounded, and ADR-0008 makes that the reason it exists separately. Taking this.

L3 — CLAUDE.md:139 now carries a dangling reference. (documentation reviewer) "src/process.ts is the real Io.runProcess … for version probes" — src/process.ts:66 also exports nodeRunInteractive, and bin/crew.ts:56-57 wires both. docs/design/architecture.md:142 gets it right. Pre-existing omission, but this diff is what introduces runInteractive to the reader. Not taking — outside #41's five bullets, and the "no adjacent improvements" rule governs. Noted for a follow-up.

L4 — .github/ISSUE_TRIAGE.md's own verification gate lists five commands and omits build:docs, so contributors following CLAUDE.md and agents following the routine doc now run different gates. (general reviewer) Out of scope for this PR; recorded here rather than filed, since the routine's local gate and CI's gate being different may well be intentional.

L5 — no automated guard exists for this drift class, and this PR widens the hand-copied surface. (both reviewers) The Io bullet is a hand-maintained parallel copy of an interface declaration — structurally the failure mode ADR-0006 forbids for the platform registry and tests/unit/docs-facts.test.ts was built to prevent for docs-site/. The mirror invariant is likewise enforced by nothing but author discipline. Correctly out of #41's scope; filed as a follow-up.

L6 — the lint annotation at :59 (eslint . vs eslint . --ignore-pattern dist-docs) — deferral to #20 confirmed sound. (both) The general reviewer verified the premise: eslint.config.js:7 ignores dist/**, coverage/**, node_modules/**, experiments/**, eslint.config.js and notably not dist-docs/**, which is exactly why the CLI flag exists. If #20 moves dist-docs/** into that array the annotation becomes true with no docs edit. Staying deferred.

L7 — :72's build:docs rationale clause is a mild scope stretch — accurate and short; bullet 4 asks only for the count and list. Keeping, tightened.

L8 — proportionality of the Io bullet (7 lines vs a 4-line neighbour). Both reviewers judged it acceptable rather than excessive: clock already carried a gloss, so glossing the four new members is internally consistent, and bare names would leave onTransactionStep opaque. The documentation reviewer's one suggestion — lead with the invariant sentence, then enumerate, so the actionable rule is not last — is adopted as part of the H1 fix.


Disposition

Taking the single fix pass .github/ISSUE_TRIAGE.md allows, covering H1, H2, H3, M1, M2, L1, L2, L7. M3, M4, L3, L5 and CONTRIBUTING.md:24-25 are filed as separate issues rather than folded in — each is either a contract decision above this routine's authority or outside #41's bullets. The reviewer then re-checks the changed areas only; there is no second round.


Generated by Claude Code

Review follow-up on the #41 fix. Three claims were still false or misleading.

The `Io` bullet's closing invariant ("everything crew touches in the
environment is one of these fields") stayed false even after the enumeration
was completed, because the filesystem is not in `Io` at all: 21 modules under
`src/` import `node:fs` directly, and path policy lives in `src/fs-safe.ts` and
`src/workspace.ts`. An agent could read the old sentence and conclude a stubbed
`Io` isolates a test from disk. The claim is now scoped to the process
environment, states the filesystem exclusion, and leads the bullet so the
actionable rule comes first rather than last.

The `random` gloss claimed "seeded in tests", but `tests/helpers/io.ts:80` is a
constant `() => 0.5`; only the stress suite injects a seeded LCG
(`tests/helpers/stress.ts`). It also dropped the production path and overstated
its scope. It now covers all three paths and carries the Store retry-jitter
scope from `src/io.ts:40-46`.

CONTRIBUTING.md:36 and :99 still said CI runs five steps ending in `npm test`,
which contradicted the six-step list this PR had just written into the agent
guides about the same workflow. #41's acceptance names this file explicitly.

Smaller corrections:

- `noEmit` applies to all three typecheck projects, not just root
  (web/tsconfig.json:16, docs-site/tsconfig.json:17); only `tsconfig.build.json`
  emits. Reworded so it no longer implies otherwise.
- Added `npm run build:docs` to the Commands block — the prose says to match CI
  locally, but no listed command ran it.
- Scoped "all six must pass" to the `build-test` job; `ci.yml:51` defines a
  second `publish-dry-run` job that also runs on every PR.
- `runInteractive` now reads as unbounded, matching src/io.ts:13-14.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Da8HvwhguZ4RDpbRphpUy7

@dichovsky dichovsky left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-check of the fix pass (9d44815aeac101)

Scope per .github/ISSUE_TRIAGE.md: the changed areas only. There is no second round.

Verdict: no CRITICAL or HIGH finding survives. Marking this ready for review. Two MEDIUMs remain and are recorded below rather than fixed — one of them is a claim this PR's own fix pass introduced, so it is flagged plainly rather than buried.

Resolved and verified

Finding Verdict Evidence re-derived
H2random gloss RESOLVED All four clauses check out: bin/crew.ts:55 random: Math.random; tests/helpers/io.ts:81 random: () => 0.5 inside captureIo; tests/helpers/stress.ts:41-49 SEEDED_RANDOM_SNIPPET, an LCG seeded from CREW_STRESS_SEED. "Store retry jitter only" is true, not a new overclaim — in src/store/index.ts the field is assigned once (:195) and read once (:242, sleep(backoffMs(this.#random))), and backoffMs (src/store/connection.ts:22-24) is its only consumer. No id generation, no token minting, no other draw.
H3CONTRIBUTING.md RESOLVED :36-38 matches ci.yml:27-40 in both membership and order. :99-100's split code span is valid CommonMark and renders as one correct command — verified by byte inspection (cat -A): 2-space continuation indent, no trailing whitespace, no stray backtick, and the spec converts the line ending to a space. :24-25 confirmed untouched, correctly left for #104.
M1noEmit RESOLVED All three typecheck projects set "noEmit": true. tsconfig.build.json ("noEmit": false) is not misrepresented — it is named on the immediately preceding line.
M2build:docs entry RESOLVED Annotation matches package.json; "CI gate" matches ci.yml:34; "not in the package" matches files: ["dist","README.md","LICENSE"]. Comment column aligns with all seven neighbours.
L2runInteractive unboundedness RESOLVED Matches src/io.ts:13-17 ("TTY-owning, UNbounded") and :59-63 ("has NO timeout"), and ADR-0008 :27-31.
L1 — second CI job RESOLVED (with a new MEDIUM, below) Both job names correct: build-test (ci.yml:13), publish-dry-run (ci.yml:51).

Mechanical checks — all pass. Mirror invariant holds with identical fix-pass hunk headers in both files (@@ -55,7 +55,8 @@, @@ -68,10 +69,11 @@, @@ -83,12 +85,14 @@). npx prettier --check clean on all three files. Longest touched line 93 (CONTRIBUTING.md:36), inside the existing 92-94 band. Scope clean — every line in the fix-pass diff traces to a finding.

Compression audit. Dropping "for an operation" from the clock gloss is safe (operationTime, src/store/connection.ts:45-51, still implements the semantics; the only Date.now() in src/ is a fallback at src/store/index.ts:194 that bin/crew.ts:54 always preempts).

Residual — MEDIUM, not fixed

R1 — H1 is PARTIALLY RESOLVED. The Io invariant is narrower but still not literally true.

The big win is real and verified: exactly 21 src/ modules import node:fs (independently re-derived, no double-count from src/ui/server.ts's additional node:fs/promises), the filesystem is correctly excluded, and "a stubbed Io does not isolate a test from disk" is true and is the actionable payload.

But the universal quantifier still over-reaches. Process-environment reads in src/ that are not in Io: process.on('SIGINT'/'SIGTERM'/'SIGHUP') (src/ui/index.ts:70-76, src/relay.ts:269-293) — signals are unambiguously inputs, the cleanest counterexample; process.platform (src/ui/index.ts:120); process.argv[1] + process.execPath (src/cli.ts:64-65, src/launcher/index.ts:44-45, src/ui/server.ts:292-293); process.versions.node (src/node-floor.ts:42). And "input" is the wrong noun for half the ten — src/io.ts:4-6 says so itself: "stdout/stderr are sinks, stdin/cwd/env are inputs", while runProcess/runInteractive are effects and onTransactionStep is an outbound callback.

Graded MEDIUM rather than HIGH because the largest and most consequential hole is now named and correctly described. The reviewer's suggested edit is a single deletion — drop the opening "these ten fields are every process-environment input crew reads" clause and lead with the enumeration — which would remove the falsehood, eliminate a forward reference across the filesystem digression, drop one of two redundant counters, and bring the bullet from 9 lines back under the 8-line budget it overran. Left for a human, since the one fix pass is spent.

R2 — NEW: publish-dry-run does not rehearse the dry-run on every PR.

The fix pass introduced: "A second job, publish-dry-run, rehearses npm publish --dry-run on every PR." The job runs on every PR, but the dry-run step (ci.yml:65-73) is guarded and skips when the manifest version is already on npm. package.json is 0.1.1 and npm view @dichovsky/crew version returns 0.1.1, so against current main that step prints ::notice:: … skipping dry-run on every PR and no rehearsal happens. The claim is true only on a version-bumping PR.

This is a newly-authored claim in precisely the drift class this PR exists to eliminate, which is why it is called out rather than left in the tail. Suggested wording: "…rehearses npm publish --dry-run, skipping when the version is already on npm."

Residual — LOW

  • "All six steps of that build-test job" misstates the job's composition: it has eleven steps (checkout, setup-node, npm ci, apt-get tmux, tmux -V, then the six gates). The prior "All six must pass" was safer, since "six" referred back to the six named gates. Suggested: "All six of those gates in the build-test job must pass."
  • runInteractive lost its exclusivity. "reserved for tmux attach" → "for tmux attach", dropping the constraint src/io.ts:15-17 states in capitals ("uses it ONLY for tmux attach — Every other tmux/git call … goes through runProcess") and ADR-0008 treats as load-bearing. Restoring the word "only" costs nothing and stays within the line.
  • The path-policy pointer omits src/setup/fs.ts. CLAUDE.md:150-152 names a third owner — "setup's guarded writes to global/project paths outside .crew/ (a deliberately separate, narrower policy than the workspace-scoped fs-safe.ts)". The parenthetical is a pointer, not an enumeration, so this is minor.
  • build:docs rationale softened from "fails the PR on" to "catches" — "must pass" earlier in the sentence still carries the consequence.

Disposition

Per .github/ISSUE_TRIAGE.md: nothing CRITICAL or HIGH survives, so the PR is marked ready for review. R1 and R2 are left for the human reviewer — R2 in particular is a one-clause correction that a maintainer may want before merging, and both are cheap enough to fold into whatever lands next on these files.


Generated by Claude Code

@dichovsky
dichovsky marked this pull request as ready for review August 6, 2026 18:28

Copy link
Copy Markdown
Owner Author

CI status: no run has completed for this PR, and the one that started was cancelled before it ever acquired a runner.

Recording this because the PR body defers the coverage-threshold claim to "this PR's CI run", and there is not yet a run to defer to.

  • Run 31125511593 (ci.yml, pull_request, head 9d44815) was created 18:15:01 and cancelled at 18:32:14. Both jobs — ubuntu / node 24.18.0 and publish dry-run — ended cancelled, not failure, with runner_id: 0 and an empty runner_name: they queued for ~17 minutes and never got a runner.
  • The head commit aeac101 (the fix pass) has no check suite at allGET /pulls/101/status reports state: pending, total_count: 0.

So the red X on this PR is queue starvation, not a test result. Nothing in the diff can cause it: the change is three Markdown files and zero code, and the same pattern is visible on other open PRs right now — #94 and #91 both show failure conclusions from the same window, and #98's run has been sitting queued.

No fix pushed, because there is nothing here to fix. I am not pushing an empty commit to force a re-trigger — that would add noise to the history for a runner-availability problem that resolves on its own. The next push to this branch, or a manual re-run of the workflow, will produce the real signal.

What is still owed when a run does complete: the 95% coverage thresholds were never evaluated locally (vitest suppresses the table on a non-zero exit, and this container fails two doctor tests as uid 0 — proven pre-existing in the PR body). The diff touches no src/** or bin/** file, so coverage cannot have moved, but that remains an argument rather than a measurement until CI says so.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants