fix: close six Console, prompt, and gate defects that assert a state the code lacks - #116
Open
dichovsky wants to merge 5 commits into
Open
fix: close six Console, prompt, and gate defects that assert a state the code lacks#116dichovsky wants to merge 5 commits into
dichovsky wants to merge 5 commits into
Conversation
`ENGINE_META` covered seven of the eight Participant CLI ids, so an Agent joined as `little-coder` fell through to the unknown-engine branch and the Console presented a fully supported platform as unrecognized — the raw id string with the neutral fallback badge. The gap is invisible from the docs-facts guard, which compares generated facts against the platform registry and never reads `web/`. Exporting ENGINE_META lets a test pin its key set to PARTICIPANT_IDS, so the next engine that lands in the shared vocabulary without a badge fails a test instead of shipping. Closes #59
FR-U20 requires that a Team launch from the Console be detached and that attaching stay a terminal-only action. The launch route honored that through the `noAttach` seam; the resume route could not, because `runTeamResume`'s deps slice omitted `noAttach` entirely. The resumed plan carries the same `attach: true` a terminal launch would, so `POST /api/team/resume` drove `tmux attach` inside the headless `crew ui` server process — an HTTP request reaching for a terminal the Operator is not sitting at. Widening the slice is the whole fix: the Console passes `noAttach: true` like its launch route, and the CLI omits it so `crew team resume` keeps its plan-driven attach behavior unchanged. No route, requirement, or user-facing capability changes. The test fake now models two things the real panes already do — registering on the configured client and re-joining an archived row with `--resume` — so the resume happy path is reachable and the zero-attach assertion is real rather than an artifact of an early failure. Closes #50
…eset ADR-0016 decided that crew's Relay delivers the Worker context reset, but its Consequences put "the Relay delivery, the registry field, and their requirements" in a follow-up change that has not landed: `clear_safe` reaches no reset delivery in `src/relay.ts` or `src/launcher/`, and no platform record carries a per-engine reset command. The packaged Manager and Worker prompts asserted the unbuilt half as live, so an Agent launched with a shipped Role waits for a reset that never arrives. The ADR outranks the prompt, so the prompt text moves: both now say crew does not deliver the reset yet and a human types it, while keeping ADR-0016's own finding that a Worker cannot reset itself. Closes #23
`e2e/ui/tsconfig.json` was referenced by no script and no workflow, and it did not compile: alone among the repo's tsconfigs it set `lib: ["ES2023"]` with no DOM lib and omitted `skipLibCheck`, so `tsc -p e2e/ui/tsconfig.json` failed with 139 errors inside playwright-core's own declarations. Playwright transpiles without type-checking, so the specs escaped the repo's otherwise universal gate while the dangling config implied they were covered. Wiring it into `typecheck` rather than the ui-e2e workflow keeps the gate where every other project already is, so a type error surfaces per-PR instead of in a nightly browser smoke. The added guard walks the repo for tsconfigs and fails on any the script does not compile, so the next dangling project cannot repeat this silently. NOTE: `npm run typecheck` now runs FOUR tsconfig projects, not three. Closes #24
Both comments claim an exactness they do not have, and both are the first thing a reader of that module sees. `src/ui/server.ts`'s header says the write surface is EXACTLY the FR-U19 action POSTs and then lists nine, omitting the `/api/agents/:id/archive` and `/api/agents/:id/restore` routes that `isActionPath` admits and that mutate Agent rows. Anyone auditing the write surface from the header would miss two live routes. The neighbouring docstring already says "FR-U19/FR-U36", so the header now matches it. `src/format.ts` attributes `CREDENTIAL_KEY` to a "name-based env-guardrail set" that no code implements — neither `doctor` nor `setup` reads a variable for reporting. The comment now describes the constant as what it is, the credential-word vocabulary of the keyed-pair redaction in free text (FR-J14), and points the environment property at FR-J13, which actually governs it. Comment-only: no route table, dispatch path, or behavior changes, so no test changes are expected. Closes #60 Closes #68
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.
Closes #23
Closes #24
Closes #50
Closes #59
Closes #60
Closes #68
Six defects that share one shape: something in the repo asserts a state of the world that the code does not have. Two of them are only comments, but a comment claiming an exact inventory is the thing a reader audits from — and the other four are live: an HTTP request can drive the Console into
tmux attach, a supported Participant CLI renders as unrecognized, a whole tsconfig project sits outside the typecheck gate, and every shipped Role prompt tells an Agent to wait for a reset that no code delivers.All six were re-verified against
main@09e67cdby opening the cited files, not from the issue bodies. Deltas found are recorded per-issue below.What changed and why
web/view-model.tsENGINE_METAgains alittle-coderentry (labelLittle Coder, matchingdocs/design/setup-integration.mdandsrc/platforms/little-coder.ts) and is exported so a test can pin its key set.src/launcher/resume.ts,src/ui/actions.tsrunTeamResume's deps slice widens from'adapter' | 'delay' | 'relayBin'to include'noAttach';resumeTeampassesnoAttach: trueexactly aslaunchTeamalready does.src/templates.tse2e/ui/tsconfig.json,package.jsonskipLibCheckand a DOM lib so it compiles;typechecknow runs it.src/ui/server.tssrc/format.tsCREDENTIAL_KEYcomment drops the nonexistent env guardrail and points the environment property at FR-J13.#50 — the delicate one
FR-U20 (
docs/design/srs.md:809-812): "A Team launch from the Console shall be detached, and attaching to its session shall remain a terminal-only action." Underdocs/README.md's authority order the SRS is rank 2 and the code is the bug.The mechanism reproduces exactly as filed.
launchTeampassesnoAttach: true;resumeTeamstructurally could not, becauserunTeamResume's deps werePick<LiveLaunchDeps, 'adapter' | 'delay' | 'relayBin'>— the attach seam was not in the slice. The resumed plan is rebuilt from current config with no flags, soattach: flags.noAttach !== true(src/launcher/config.ts:408) yieldsattach: true, andrunLiveLaunchreachedadapter.attach(session)(src/launcher/session.ts:292) inside the foregroundcrew uiserver process.The fix is deliberately the minimum: one type widened, one field passed. No new HTTP route, no new capability, no requirement change, and
crew team resumefrom a terminal is untouched — the CLI omitsnoAttach, so its plan-driven attach behavior is bit-for-bit what it was.web/components/operations.tsxneeded no change; the client was already correct.For #79 (Console team-resume traceability), which is a different PR: the only behavioral change here is that
POST /api/team/resumeno longer attaches.TEAM_POSTSintests/integration/ui-server-team.test.tsis deliberately not touched — adding/api/team/resumeto that shared registry stays #79's, so nothing here collides with it.#23 — why the prompt moves, not the ADR
ADR-0016 is authority rank 1, and its Consequences are explicit: "The Relay delivery, the registry field, and their requirements land in a follow-up change to this one." That follow-up has not landed —
clear_safereaches no delivery insrc/relay.tsorsrc/launcher/, and nosrc/platforms/record carries a reset field. So the shipped prompt is the wrong side.The new wording keeps ADR-0016's own finding (a Worker cannot reset itself — proven impossible on every supported engine) and drops only the claim that crew already delivers the reset.
Deltas against the issue bodies
ENGINE_META's labels "diverge from the registry's". There is no display-name field in the registry at all —grep -n displayName src/platforms/is empty;ParticipantTargetcarriesid/executable, never a human label. So there was nothing to reconcile, and the judgment call the body offers is moot. Only the missing-key gap is closed; the other seven labels are untouched.TeamActionDepsis at:293-300andresumeTeamat:377-387before this change (both shift by the added docblock). The docblock already called it "a DETACHED recovery launch", confirming the divergence was unintended.mainis 139 —npx tsc -p e2e/ui/tsconfig.json 2>&1 | grep -c "error TS"→139, exit2. Same root cause, an order of magnitude more.src/templates.ts:33-34and:73both still hold verbatim. The second half of the issue's acceptance is NOT in this PR — see Cross-PR dependency.:12-18,TASK_ACTION_ROUTE:262,AGENT_ACTION_ROUTE:264-265,POST_PATHS:268-276,isActionPath:277-283, correct docstring:278. One thing the body does not mention: with the agent routes listed, the header's trailing "the destructive three gated by the FR-U25 flag" also became wrong — archive is FR-U25-gated too (src/ui/actions.ts:646-650). Corrected to "the destructive ones (stop, prune, clean, archive)" in the same sentence, so the header is not left half-true.src/doctor.tsandsrc/setup/index.tsread the environment only for PATH lookup and path derivation. FR-J13's title is "No credential env value" (srs.md:692), as the body states.None of the six were already fixed by #85/#91/#94/#97/#101; all six reproduce on
09e67cd.Red → green evidence
Every behavioral fix was driven test-first. Each RED was produced by running the new test against the unfixed source (
git stash push <the source file>, run,git stash pop), not by reasoning about it.web/view-model.test.ts—ENGINE_META covers exactly the shared Participant id vocabulary, plus alittle-coderlabel/glyph caseexpected 'little-coder' to be 'Little Coder'; andTypeError: Cannot convert undefined or null to object(ENGINE_META was not exported)tests/integration/ui-server-team.test.ts—POST /api/team/resume (FR-U20) > resumes a cleanly stopped session DETACHED: zero attach callsAssertionError: expected [ 'isPresent', …(29) ] to not include 'attach'— the resume returned 200 having attached, i.e. the server really did reachtmux attachtests/unit/templates.test.ts—ADR-0016 relay-delivered reset is still a follow-up(2 cases)crew then resets the Worker's session itselfandcrew performs the context reset itselfnpx tsc -p e2e/ui/tsconfig.json, andtests/unit/typecheck-projects.test.ts—npm run typecheck compiles every TypeScript project in the repotscexit 2, 139 errors; the guard failed withtsconfigs outside the typecheck gate: e2e/ui/tsconfig.jsontscexit 0; guard passes#60 and #68 are comment-only and add no tests (
CONTRIBUTING.md:98,.github/ISSUE_TRIAGE.md: docs/comment-only exemption, claimed explicitly here). Every other issue in this PR carries a test — this PR takes no blanket docs-only exemption.One test-fixture change worth flagging
The #50 test needs the resume happy path to actually complete, or "zero attach calls" would pass vacuously on an early failure. Two things in
fakeTmuxprevented that, and both were fixture inaccuracies rather than product behavior:assertArchivedResumable'sagent.platformId !== plan.clientcheck (409 TEAM_DRIFT). Real panes register on the configured client.--resumeto the invocation, which the fake's positional parser read as the agent id, and it never passedresume: true— so the reactivation became a suffixed new row and stage-2 roster readiness failed.Both were corrected in the fake only. All 36 pre-existing tests in that file still pass unchanged.
Verification
Run on the final pushed tree, Node
v24.19.0, uid501(not root).npm run typechecknpm run lintnpm run format:checknpm run builddist/+dist/ui-assets/main.jsnpm run build:docsdist-docs/main.jsnpm run test:coverageCoverage — all four thresholds clear 95%:
src/**,bin/**)Because vitest suppresses the coverage table when a suite fails to collect, those numbers come from an otherwise identical run with the failing file moved aside, which exited 0 — i.e. the thresholds were genuinely evaluated and passed, not inferred.
Pre-existing-failure proof (performed, not assumed). The failure is
tests/integration/package/pack-smoke.test.ts, collapsing inbeforeAllwithTypeError: Cannot read properties of undefined (reading 'files')at:40— this npm parsesnpm pack --jsoninto a shape the test does not expect; its 9 tests skip. Proven bygit stash push -uon every changed file, confirminggit status --porcelainreported only the untrackednode_modulessymlink, re-running that file standalone on the clean tree, thengit stash pop. Identical both ways: 1 failed suite / 9 skipped. This PR's CI run, on the pinned Node24.18.0, is authoritative.The two
doctor.test.tstests that fail under uid 0 did not appear here —id -uis501.Cross-PR dependency — read this before merging
npm run typechecknow runs FOUR tsconfig projects, not three. Exactly three documents state the old count, verbatim and identically. None were edited here — all three are out of scope for this PR — so each needs a lockstep update or it goes stale on merge:CLAUDE.md:59npm run typecheck # 3 tsconfigs, all noEmit: root (incl. tests), web/, docs-site/AGENTS.md:59(byte-identical file)CONTRIBUTING.md:26One correction to the hazard as originally scoped to me:
docs/design/srs.mddoes not state the count. Its automated-gate sentence (:1217) namesnpm run typecheckas a command and never enumerates projects, so it stays true and needs nothing.CONTRIBUTING.md:26carries the claim instead — same total of three sites, different third one.#23's ADR-index half is not done here. The issue's acceptance is "soften the Role wording in
src/templates.tsand add the 'lands in a follow-up change' caveat to the ADR index row".docs/adr/README.md:20still states the mechanism unconditionally, and this PR must not editdocs/**. PR #110 touches bothdocs/adr/README.mdand ADR-0016 but leaves that row's wording unchanged — so nothing currently open closes it. TheCloses #23above is claimed on the code half; if the reviewer wants the ADR index caveat in the same change, that row needs a docs-owning PR.Deliberately not done
crew_versionwas not bumped on the two changed Role prompts (manager/worker stay at4). No code compares versions —parseVersion(src/roles.ts:53) only reports it, andinfoFor's packaged-vs-project classification is a byte comparison, not a version comparison. Bumping would also require editingtests/integration/commands/roles.test.ts:62's pinnedversion: 4. Left minimal and flagged rather than decided silently.e2e/ui/tsconfig.jsonwas not aligned to the root project's other strict flags (exactOptionalPropertyTypes,noUncheckedIndexedAccess,verbatimModuleSyntax). Only the two options e2e/ui tsconfig is neither wired into typecheck nor able to compile #24 names were added; tightening the specs is a separate change.ENGINE_METAlabels ("Codex", "Gemini", "Copilot") are untouched — see the Console: ENGINE_META omits little-coder, so its panes render the raw id #59 delta.Contract impact
No ADR is amended, no
FR-*/NFR-*is added or given new meaning, no CLI grammar rule, record type, error code, command, or flag moves, andCURRENT_SCHEMA_VERSIONis unchanged. #50 moves the code toward FR-U20 rather than changing it.package.json's only edit is thetypecheckscript — no version, dependency, orfileschange.Related open PRs
No file overlap with any open PR (
gh pr list --state open --json number,title,files). #115 touchessrc/doctor.ts, #114src/platforms/shared.ts, #113 the agent guides, #112/#111/#110docs/**. The only relationships are the two dependencies named above: #110 (ADR index, for #23) and #113 (which already editsCLAUDE.md/AGENTS.md— the natural home for the tsconfig-count update).