docs: trace crew team resume with SRS requirements - #49
Conversation
`crew team resume` was documented in the CLI contract's normative grammar and covered by tests, but had no SRS requirement behind it, while its sibling recovery verb `crew team stop` is traced by FR-U26-FR-U29. Behavior changes to resume's eligibility proof, strict plan match, archived-exact reactivation, or its `resume_result` record therefore had no requirement to verify against. Adds FR-U39-FR-U42 mirroring the FR-U26-FR-U29 treatment of stop, each with a Verify pointer to the tests that already cover it, and widens the group-U heading, the 1.3.2 summary, and the two FR-U01-FR-U38 id ranges accordingly. Docs-only; no runtime effect. Closes #3 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WgMaAYnExRQpdbfVSoMPKK
There was a problem hiding this comment.
Pull request overview
Adds missing SRS traceability for the existing crew team resume <session> behavior by introducing new Group U functional requirements (FR-U39–FR-U42) and updating surrounding SRS indexing text to reflect that Group U now covers both owned-Team stop and resume.
Changes:
- Add four new SRS requirements for
crew team resume(eligibility proof, strict plan match, archived-exact reactivation, andresume_resultrecord shape). - Widen Group U headings/summary text from stop-only to stop/resume.
- Update Appendix references/ranges to include
FR-U01–FR-U42.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dichovsky
left a comment
There was a problem hiding this comment.
Routine review of 9431b14. Roster for a docs-only diff: general code reviewer, plus prose-versus-code verification in place of the TypeScript reviewer. No security reviewer — the diff touches no module on that list.
No CRITICAL findings. Four HIGH were raised; one is in scope and is being fixed, three are pre-existing problems this PR did not create.
Verified correct
Worth recording what held up, since most of the diff is factual claims about code:
- FR-U39's four preconditions, their error codes, and their order match
src/launcher/resume.ts:181-200exactly. All four codes exist insrc/errors.tswith the meanings implied. - "worded so it leaks no filesystem path" is true of the actual
NOT_FOUNDmessage (resume.ts:68,79), asserted verbatim attests/unit/launcher/resume.test.ts:290-300. - FR-U40's "identical — apart from its creation timestamp" is exact:
comparablePlanomits exactlycreated_atandplansMatchcomparesJSON.stringifyof the remainder. If anything the real check is stronger than the prose — string comparison makes JSON key order significant. - FR-U41's archived-exact rule matches
assertArchivedResumablefield for field. - FR-U42's record matches
src/format.ts:277-302byte for byte, and the test usestoEqual, so no extra keys. - Every
*Verify:*path exists, and each parenthetical case list maps to real tests — no pointer overstates its case list. - The "no test covers a successful resume" grounds claim is correct; every
runTeamResumecase in the repo is a rejection. - No accepted ADR mentions
team resume, so nothing higher in the authority order is contradicted. Ids are contiguous, defined once each, and nothing links to the old group-U heading anchor.
HIGH — in scope, being fixed
1. The Appendix E all-nine-P grade is not honest for the new requirements. Extending the group-U default row to FR-U42 asserts every group-U requirement passes all nine §5.2.5 characteristics, but FR-U39 packs five rules into one id and FR-U41 bundles three — against this document's own rule at srs.md:39-40 ("Each requirement states exactly one rule"). The sibling stop feature splits the same shape into four singular ids. Being fixed by splitting rather than by adding Appendix E override rows, since #3's acceptance is to mirror the FR-U26–FR-U29 treatment and that treatment is singular requirements.
HIGH — out of scope, tracked separately
2. FR-U19's closed enumeration contradicts the shipped Console. Both reviewers flagged that POST /api/team/resume and GET /api/resumable-sessions exist while FR-U19 says the action surface "shall be limited to" a list omitting resume, and both wanted FR-U19 extended here on the FR-U36 precedent. Declining: that is exactly #25, which was filed for it. Widening a closed normative enumeration is a contract change that deserves its own reviewed diff, not a rider on a traceability fix.
3. The Console resume path attaches tmux inside the headless crew ui process, violating FR-U20. resumeTeam cannot pass noAttach — the field is absent from TeamActionDeps and from runTeamResume's deps slice — so the Console-built plan carries attach: true and runLiveLaunch reaches adapter.attach(session) in the server process. A real code defect, found because this PR forced a close reading of the resume path. Filed as #50.
MEDIUM — in scope, being fixed
shallis missing from FR-U41's second half, againstsrs.md:39.- Marker retirement is not an invariant.
retireResumeMarkerswallows every error; it also runs afterrunLiveLaunchreturns, which blocks ontmux attachon the CLI default path, and is skipped entirely if attach exits non-zero. Being reworded to best-effort. (Independently raised by Copilot — agreed.) - The
--resumeinspection pointer names the wrong file and understates the evidence:resume.tsonly setsresume: true; the flag is appended bypaneLaunchand the platform registry, andtests/unit/platforms.test.ts:127-139already pins it for all eight platforms. Being recited as automated test. (Also raised by Copilot — agreed.) - §1.1
srs.md:53carries the same stop-only enumeration that §1.3.2 got fixed for, and was missed. - The tmux dependency statements at
:123-124,:1190-1192,:1198still scope tmux to launch/stop/peek and now under-report. - Nothing requires
team stopto write the marker resume depends on.writeResumeMarkeris untraced, so the new contract depends on a producer no requirement obliges to exist. Adding that requirement. - Present-but-invalid artifacts yield
INVALID_CONFIG, and the no-path-leak guarantee holds only for the ENOENT case. (Copilot's line-947 comment — agreed, being scoped correctly.)
MEDIUM / LOW — out of scope, tracked separately
- Resume success-path and Console-endpoint test coverage is absent — #51.
- FR-K01's diagnostic list omits
RESUME_DRIFT— #52. describe('renderTeamResumeResult (FR-U32)')is a wrong cross-reference; FR-U42 is now the right id — #48.- FR-U34's Operations bullet omits the resumable-session surface, and there is no FR-U35 counterpart for the resumable listing — part of #25.
product-spec.md:139omits resume from the Console capability list; no CHANGELOG entry. Judged not worth widening a traceability fix for.
The *Verify: automated test — …; inspection — …* form has no precedent in this document (LOW) — being replaced with FR-U11's actual inspection and automated test idiom.
One fix pass is in flight covering everything marked in scope; the reviewers then re-check only the changed areas.
Generated by Claude Code
Review of PR #49 found the first pass overstated both its own quality grade and the code it traces. Three requirements packed nine rules and five error codes into FR-U39-FR-U41 while Appendix E's group-U default asserted all nine 29148 characteristics for them, and two of the claims were not true of the implementation. - Split the resume contract into FR-U40-FR-U49, one rule and at most one error code each, mirroring how FR-U26-FR-U29 treat `team stop`, so the all-nine-P section default is honest with no override rows. - Add FR-U39: an owned-Team stop writes the clean-stop marker the resume contract consumes, which no requirement previously obliged to exist. - Word marker retirement as best-effort (FR-U48): `retireResumeMarker` swallows every error, runs only after `runLiveLaunch` returns (which blocks on the default attach), and never runs when the attach fails. - State the id-reactivation rule as an outcome (FR-U47): crew appends `--resume` to the pane invocation and the Participant CLI runs its own join; the roster gate is the enforcement. Cite the platform test that covers it and keep inspection only for the untested success path. - Separate absent artifacts (NOT_FOUND, path-free message) from present but malformed ones (INVALID_CONFIG), which do name the artifact. - Use the document's existing Verify idioms, and add resume to the tmux dependency statements in 1.1, 1.2, 1.3.4, Assumptions, Dependencies, plus the Appendix D and Appendix E id ranges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WgMaAYnExRQpdbfVSoMPKK
dichovsky
left a comment
There was a problem hiding this comment.
Re-check of the fix pass (44a3a15), changed areas only. Both reviewers: nothing CRITICAL or HIGH survives. This was the single allowed fix pass, so no further round follows.
CI is green on 44a3a15 — ubuntu / node 24.18.0 and publish dry-run both pass, e2e jobs skipped as expected on an unlabeled PR. That also settles the earlier note about local test failures: they were environment artifacts (root user defeating the chmod 000 doctor tests, no tmux, sandboxed npm pack), not the change.
The split landed correctly
FR-U39–FR-U42 became FR-U39–FR-U49. Verified independently by both reviewers: ids parse to exactly 1–49 with no gaps or duplicates; each new requirement contains exactly one shall and carries at most one error code; both group-U range references updated (Appendix D counts, Appendix E default row), with FR-U26–U29 correctly left alone; the diff still touches only docs/design/srs.md, and FR-U19, FR-U20, FR-U34, FR-K01 are outside every hunk.
The re-wrapped paragraphs were checked at word-stream level and are pure insertions — no unrelated churn.
Accepted findings — all confirmed fixed, not softened
- Marker retirement (FR-U48) — all three clauses independently verified against
resume.ts:123-129,220andsession.ts:277,292-299. A genuine tightening. - "Rejoins exact ids" (FR-U47) — restated as an outcome with the mechanism in a non-normative
*Note:*. Thetests/unit/platforms.test.tsandteam-launch-live.test.ts:472citations were both opened and say what is claimed. The roster-gate citation is a shared gate, and the reviewer judged citing it legitimate because the requirement text says so explicitly and labels the resume-specific wiring as untested. - Wrong inspection pointer — gone; the
--resumeinvocation is now cited as the automated test it actually has. - Invented Verify idiom — gone;
grep "; inspection —"is empty. - §1.1/§1.2, the tmux dependency statements, and the untraced marker producer — all fixed.
- Present-but-invalid artifacts — split into FR-U41 (absent →
NOT_FOUND, path-free message) and FR-U42 (malformed →INVALID_CONFIG, may name the relative artifact), which is the right shape given one-code-per-id.
The three new inspection-only pointers (FR-U39, FR-U42, FR-U48) were each checked for whether a citable test actually exists. All three grounds hold — inspection is not being used to dodge a citation. Notably the one place where a test did already exist for a claimed-inspection behavior got converted to a real test citation.
Residual — not blocking, flagged for a human
MEDIUM — FR-U48 is the one id the split missed. "removal is attempted only after the relaunched session releases the foreground, is skipped when that attachment ends in failure" states two separate observable rules in the indicative, neither entailed by the leading "shall not depend". Structurally the same as FR-D23, whose Appendix E override row reads S = NE. So the all-nine-P default row still overclaims for exactly one requirement. Fix is either a further split (timing vs. tolerance) or an S = NE override row plus a flagged-fix line. Left for a human because the fix-pass budget is spent.
LOW:
:942— the "record-producing" qualifier on FR-U39 is vacuous:stop.ts:217-222writes the marker unconditionally and beforerenderTeamStopResult. Two words to drop.:993— "after the relaunched session releases the foreground" presumes an attach; withrelay.attachfalse there is none. "afterrunLiveLaunchreturns successfully" covers both. Related: this clause is currently true partly because of #50, so fixing #50 would make it stale for the Console path.:985— FR-U47's Note assertsLAUNCH_FAILEDon non-registration without citing the FR that owns the roster gate.:984— "invited tocrew join <id> --resume" elides one hop; the pane actually receives the registry invocation and the CLI composes the join persrc/platforms/shared.ts:118-128.- FR-U39 is a
team stoprule filed at the end of group U with no forward pointer from FR-U26–U29. Append-only ids force the position and a pointer would mean editing the out-of-scope stop block, so this is noted as a deliberate choice rather than a defect.
Status
The PR was taken out of draft before the review roster finished — not by the routine. Since nothing CRITICAL or HIGH survives, that state is correct and I am leaving it ready for review. It has not been approved: the routine does not approve its own work.
Also filed from this review, none folded in: #50 (Console resume attaches tmux inside the headless crew ui process, violating FR-U20 — a real code defect), #51 (crew team resume has no success-path or Console-endpoint test coverage), #52 (FR-K01 omits RESUME_DRIFT).
Generated by Claude Code
The renderTeamResumeResult suite was labelled FR-U32 (deleted-Workspace recovery). FR-U49 now owns the resume_result contract and cites this test in its Verify clause, so the label has to name it.
Closes #3
What changed and why
crew team stopis traced by FR-U26–FR-U29, but its sibling recovery verbcrew team resumehad no SRS coverage at all — it was specified only one authority level down, in the CLI contract (cli-contract.md:343-351), and implemented insrc/launcher/resume.tswith tests. Per the authority order indocs/README.md, that is the SRS being incomplete, not the code being wrong, so the fix is additive documentation.Four new requirements in
docs/design/srs.md, mirroring the FR-U26–FR-U29 treatment ofteam stop:DEPENDENCY_MISSING/NOT_FOUND/INVALID_CONFIG/ALREADY_EXISTScodes the implementation actually throws.TEAM_DRIFTand relaunches nothing.resume_resultrecord and its five fields.Three supporting edits, all of which follow from group U no longer being stop-only:
#### U.section heading widened from "owned-Team stop" to "owned-Team stop/resume".FR-U01–FR-U38toFR-U01–FR-U42.No behavior change.
docs/design/srs.mdis the only file touched.Note on FR-U41's Verify pointer
The resume success path has no automated test in the repo —
tests/unit/launcher/resume.test.tsexercises seven failure branches, and nothing asserts that a valid resume relaunches, thatresume: truereaches the pane invocations, or that the marker is unlinked. Rather than cite a test that does not cover the rule, FR-U41 uses the SRS's existingautomated test — …; inspection — …idiom (precedent: FR-U11, FR-U19) and points the success half atsrc/launcher/resume.ts. The coverage hole itself is real and is being filed separately.Verification
Docs-only change: no tests added, claiming the docs-only exemption in
.github/ISSUE_TRIAGE.md. The gate was still run in full.npm run typechecktsconfig.json,web/tsconfig.json,docs-site/tsconfig.jsonnpm run lintnpm run format:checkAll matched files use Prettier code style!npm run builddist/ui-assets/main.js 88.4kbnpm run test:coverageThe 9 failures reproduce identically on an unmodified tree (verified by stashing the change and re-running): 4 in
tests/integration/package/pack-smoke.test.ts(needs a realnpm pack+ install), 2 intests/integration/commands/doctor.test.ts(the sandbox runs as root, sochmod 000does not make a directory unreadable and the expectedINVALID_CONFIGwarnings never fire), 2 intests/unit/bin-entry-guard.test.ts(subprocess entry-point guard), 1 intests/spawn/tmux-e2e.test.ts(no tmux installed). Vitest suppresses the coverage table on a failing run, so the 95% thresholds were not evaluated locally. A Markdown-only edit cannot influence any of these — this PR's CI run on GitHub-hosted runners is the authority for the gate.Rebased onto
origin/main@d311767immediately before pushing; the base had not moved, and the gate was re-run afterward with identical results.Related open PRs
None — this is the only open pull request on the repository.
Out of scope
Deliberately untouched, each tracked separately:
/api/team/resume+/api/resumable-sessionsroutes — Console: team-resume routes exceed FR-U19's closed action list (companion to #3) #25.resume_resultenvelope in the CLI contract and the resume routes inarchitecture.md— CLI contract drift: doctor finding codes, resume_result, ui --json opener, implies-launch flags, error-code scopes #28.describe('renderTeamResumeResult (FR-U32)')intests/unit/format.test.ts— Tests: renderTeamResumeResult suite cites FR-U32, which is deleted-Workspace recovery #48. FR-U42 is now the correct id for that suite.Generated by Claude Code