Skip to content

fix: key the shared workflow's resume step off the token the pane can see - #114

Open
dichovsky wants to merge 2 commits into
mainfrom
fix/setup-workflow-and-registry-revision
Open

fix: key the shared workflow's resume step off the token the pane can see#114
dichovsky wants to merge 2 commits into
mainfrom
fix/setup-workflow-and-registry-revision

Conversation

@dichovsky

Copy link
Copy Markdown
Owner

Closes #73
Closes #75

Both issues sit on the setup/platform-registry seam, and both are cases of text telling a reader to do something the code does not do. #73 misdirects a contributor about when to bump REGISTRY_REVISION; #75 misdirects the model in a resumed pane about where the resume signal comes from. The second one has a user-visible failure mode: FR-U47's whole mechanism is that "each resumed pane's Participant CLI is invited to crew join <id> --resume" (docs/design/srs.md:985-994) — crew never runs the join itself — so a pane that follows its own parse rule literally drops the flag, and the roster gate then fails the relaunch with LAUNCH_FAILED when the planned archived ids never come back active. FR-U47's Verify clause already records that this wiring "has no end-to-end success test yet", so nothing catches it.

Both claims reproduce on main @ 09e67cd, re-checked against the files themselves rather than taken from the issue bodies.

What changed and why

#75 — the parse rule is the side that moves

The direction here was genuinely open, so here is the evidence for the one chosen. A Participant CLI has no view of tmux session history. The only thing that can tell a pane it is recovering a clean stop is the --resume token the Launcher appends: src/launcher/session.ts:137 calls target.invocation(role, agentId, options) with options = { resume } (:129), and each platform appends the literal suffix (src/platforms/claude.ts:34 and its seven siblings). Copilot arrives by the launchArgs branch instead (src/launcher/session.ts:130-134) but reads the identical workflow text. So on a resume {{ROLE_ARGS}} is worker w1 --resume — three tokens against a rule admitting two.

That rules out the other direction. Deleting step 2's resume sentence would make the text self-consistent and break FR-U47: the Launcher would still append the flag, and nothing would forward it to crew join. The Launcher is also the correct side by authority — the SRS (rank 2) documents the invitation mechanism, and setup-integration.md is a design document (rank 4). So step 1 admits the token and step 2 keys off it.

Location Change
src/platforms/shared.ts:121 Step 1: as `<role> [id]` as `<role> [id] [--resume]` . The parse rule now names the token the Launcher actually sends.
src/platforms/shared.ts:125-127 Step 2: "If the pane is recovering a clean stop, add --resume…" → "If the arguments included --resume (this pane is recovering a clean stop), add --resume to the join command." The trigger is now an observable token; the clean-stop meaning is kept as a parenthetical so the why does not disappear.
src/platforms/shared.ts:19 REGISTRY_REVISION 5 → 6. Forced, not chosen — see below.
docs/design/setup-integration.md §4.0 The canonical block is a byte-for-byte mirror (:107-110 promises "word for word"), so both lines move identically. Verified mechanically, not by eye: a script extracts the SHARED_WORKFLOW template literal and the §4.0 fenced block and compares — equal: true, both 1494 bytes.
docs/design/setup-integration.md §3 The prose summary follows in two places, not one: step 1's parse rule (:87) and step 3's resume sentence (:91-93), which now names the trailing token as the pane's only signal.
docs/design/setup-integration.md §4.1-4.8, marker grammar, §7 Eight registry-revision: 5 literals → 6, and §7's "currently at registry-revision 5" → 6.
docs/design/setup-integration.md:534-536 §7's parenthetical attributed the most recent bump to the little-coder target; it now names this workflow change. PR #71 flagged this parenthetical as possibly stale and left it rather than guess — the bump makes it definitively stale, so it moves here.
tests/unit/platforms.test.ts:230-244 All eight content-hash snapshots + expect(REGISTRY_REVISION).toBe(6).
tests/unit/platforms.test.ts (new) A regression test asserting that the token the Launcher appends is the token every rendered artifact's parse rule admits, and that the join step keys off it. This is the guard whose absence is the bug.
tests/integration/commands/setup.test.ts:129 expect(rec.registry_revision).toBe(5)6. Not named by either issue — found by running the suite.
docs-site/generated/facts.json Regenerated (UPDATE_DOCS_FACTS=1). A one-line derived diff, "revision": 56.

The registry-revision hazard, handled explicitly. This change alters the rendered bytes of all eight Participant artifacts, which is exactly the condition setup-integration.md §7 states: "Whenever any generated file's bytes change, REGISTRY_REVISION must be incremented, so that a previously installed, unchanged file is classified as managed-outdated." So the bump is mandatory here, and it is the opposite of the spurious bump #73 is about. The consequence is intended: an already-installed artifact still carries the old text and should be regenerated. managed-outdated regenerates in place with no backup and no --force (§6 Drift states), so no user is blocked. Both hash-bearing test surfaces moved in lockstep; tests/tools/release-smoke.test.ts extracts digests at runtime (:38-40) rather than hard-coding them, so it needed no change.

#73 — the comment is the wrong side

src/platforms/shared.ts:7-9 read "…a change there is a registry-revision bump here", which read literally requires a bump for a doc edit that changes no rendered byte. A bump reclassifies every installed artifact as managed-outdated and sends every user through a needless regenerate. §7's bytes rule is the one the code implements — markerLine() (:160-163) stamps REGISTRY_REVISION into each artifact and classifyArtifact() (:198-203) compares the stored value against it — so the comment is the incorrect side, per the issue's own reasoning and confirmed here. It now states the bytes condition, and keeps the point it exists to make (the doc is the source of the platform facts this module mirrors).

This is shipped as its own commit (3580e7d), separate from the #75 fix, because it is a comment-only change with no behavioral effect.

Deltas against the issue bodies

Noted rather than silently absorbed. Every cited line was opened at main @ 09e67cd.

  1. Setup: shared.ts header says any setup-integration.md change bumps the registry revision, contradicting §7 #73's doc line numbers shifted by five. The bytes rule it cites at setup-integration.md:534-536 is at :539-541 on main; the text is verbatim what the issue quotes. PR docs: sync setup-integration with shipped workflow text and registry revision #71 added five lines earlier in the file after the issue was filed.
  2. Setup: shared.ts header says any setup-integration.md change bumps the registry revision, contradicting §7 #73's :487 citation for the managed-outdated reclassification is now :499. :486-487 is the sentence defining registry-revision; the Drift-states table row that actually specifies the reclassification is at :499. The claim holds; only the pointer moved.
  3. Setup: shared.ts header says any setup-integration.md change bumps the registry revision, contradicting §7 #73's source citations are exact. src/platforms/shared.ts:7-9 and markerLine() at :160-163 land precisely.
  4. Setup: the shared workflow asks a pane to detect a clean-stop resume its own parse rule hides #75's source citations are all exactSHARED_WORKFLOW :121-142, step 1 :121, step 2 :125-127, REGISTRY_REVISION :19, src/platforms/claude.ts:34, src/launcher/session.ts:137 and :129-134, tests/unit/platforms.test.ts:127-139 and :230-244. Nothing had been half-fixed by the six PRs that merged in the meantime.
  5. Setup: the shared workflow asks a pane to detect a clean-stop resume its own parse rule hides #75's setup-integration.md:101-104 citation for the byte-for-byte mirror rule is now :107-110 — the same five-line shift from docs: sync setup-integration with shipped workflow text and registry revision #71.
  6. Setup: the shared workflow asks a pane to detect a clean-stop resume its own parse rule hides #75 under-counts §3. It says "§3's prose summary follows"; §3 states the workflow in six numbered steps and the drift is in two of them (step 1's parse rule and step 3's resume sentence). Both moved.
  7. Setup: the shared workflow asks a pane to detect a clean-stop resume its own parse rule hides #75 misses one test that had to move. It names the content-hash snapshots at tests/unit/platforms.test.ts:230-244, which is correct, but not tests/integration/commands/setup.test.ts:129, which hard-codes registry_revision === 5. It failed on the first full run and is fixed here.

Verification

Tests: added and updated — no exemption claimed. The diff touches .ts, so .github/ISSUE_TRIAGE.md's docs-only exemption does not apply. One new test (the parse-rule/appended-token agreement guard) plus three lockstep updates.

Run on the pushed tree, Node 24.18.0:

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
npm run test:coverage 1 1390 passed, 11 skipped, 1 failed suite — pre-existing and environmental
npx vitest run tests/unit/platforms.test.ts tests/unit/docs-facts.test.ts 0 50 passed

Pre-existing-failure proof (performed, not assumed). The single failure is a suite-level error in tests/integration/package/pack-smoke.test.ts:40TypeError: Cannot read properties of undefined (reading 'files'), from npm pack --json returning a different shape under this machine's npm than the test's { filename, files }[] assumption. Proven by git stash push -u, confirming git status --porcelain showed nothing tracked, re-running the file on the clean tree — identical failure, same line, same message — then git stash pop. No test in it executed on either tree (9 skipped both times), and this PR touches nothing about packaging. CI runs a different npm and is authoritative.

Coverage thresholds were NOT evaluated locally. Vitest suppresses the coverage table on a failing run, so the 95% numbers were never displayed and no claim is made about them. This PR's CI run is authoritative. The only src/** change is a constant and a string literal inside an already-covered module, so coverage cannot have moved.

docs-site/generated/facts.json was regenerated, not hand-edited: UPDATE_DOCS_FACTS=1 npx vitest run tests/unit/docs-facts.test.ts, per that file's own header instructions. It is derived from src/platforms/registry.ts and re-derives to the committed bytes (docs-facts.test.ts then passes without the env var). The one-number diff is the whole change.

Markdown wrapping is hand-maintained (proseWrap: "preserve"), so format:check passes either way. New lines stay inside the file's existing band; no untouched paragraph was reflowed.

Contract impact

No requirement, CLI grammar rule, error code, command, or flag changes. crew join --resume already exists (src/cli.ts:129, FR-C05/FR-C06) — this change only makes the shared workflow text point at it correctly. CURRENT_SCHEMA_VERSION is untouched. The one contract-shaped move is REGISTRY_REVISION 5 → 6, which is setup-integration.md §7's own required response to a rendered-byte change.

Related open PRs

No file overlap. Checked at file level against all four open PRs: #110 (docs/adr/*, product-spec.md, .github/workflows/stress.yml), #111 (README.md, EXAMPLES.md, architecture.md, data-model.md, decisions.md, tests/tools/release-smoke.test.ts), #112 (configuration.md, security.md, srs.md, testing-strategy.md), #113 (AGENTS.md, CLAUDE.md). None touches docs/design/setup-integration.md, src/platforms/**, tests/unit/platforms.test.ts, tests/integration/commands/setup.test.ts, or docs-site/generated/facts.json. #111 does touch tests/tools/release-smoke.test.ts, but that file derives content hashes at runtime rather than pinning them, so there is no semantic conflict with the revision bump either.

src/launcher/** was not touched. The fix needed no Launcher change — the Launcher's resume plumbing is already correct, and it is the workflow text that was out of step with it. Named here because a concurrent PR was expected to own resume.ts/session.ts.

Out of scope

  • The §2 canonical-matrix Invocation cells still show /crew <role> [id] without [--resume]. Deliberate, and unchanged from PR docs: sync setup-integration with shipped workflow text and registry revision #71's reasoning: those cells state the interactive invocation a human types, which is what setup prints (FR-G07), and the note below the table (:41-46) already covers the Launcher's resume suffix for both the invocation and launch_args paths.
  • FR-U47's missing end-to-end resume test. Its Verify clause names the gap itself. The new guard in tests/unit/platforms.test.ts closes the specific text/token disagreement this PR is about, not the resume round trip; that remains untested and out of scope here.
  • Nothing guards §4.0 against re-drifting from SHARED_WORKFLOW. The mirror was verified by a throwaway script, not by a committed test. That is already filed as Tests: nothing guards setup-integration.md §4.0 or its registry-revision examples against drift #74.

The module header told a contributor that any change to
docs/design/setup-integration.md is a registry-revision bump. Read
literally that requires a bump for a doc edit that changes no rendered
byte — correcting stale prose, syncing a description — and a bump
reclassifies every already-installed artifact as `managed-outdated`,
sending every user through a regenerate for a file whose content did not
change.

The comment is the wrong side: §7 of that document states the rule as
"whenever any generated file's bytes change", and it is the bytes rule
the code implements — `markerLine` stamps `REGISTRY_REVISION` into each
artifact and `classifyArtifact` compares the stored value against it. The
header keeps the point it exists to make (the doc is the source of the
platform facts this module mirrors) and now states the bump condition the
way the code and §7 both do.

Refs #73
… see

A Participant CLI has no view of tmux session history, so "if the pane is
recovering a clean stop" named a state it cannot observe. The only
evidence a resumed pane gets is the ` --resume` token the Launcher
appends to its invocation — and step 1's parse rule admitted `<role>
[id]` only, telling the model to ignore the very token step 2 depended
on. A model following step 1 literally drops it and joins without
`--resume`; the roster gate then fails the relaunch with `LAUNCH_FAILED`
because the planned archived ids never come back active.

The parse rule is the side that moves. Removing step 2's resume sentence
would break the mechanism FR-U47 rests on, since the Launcher does append
the flag and the pane is the only thing that can forward it to
`crew join`. Step 1 now admits an optional trailing `--resume`, and step
2 keys off that token instead of an unobservable session state.

This alters the rendered bytes of all eight Participant artifacts, so
REGISTRY_REVISION goes 5 -> 6 per setup-integration.md §7 and the
per-platform content-hash snapshots move with it. §4.0 of that document
is a byte-for-byte mirror of the source text (verified mechanically) and
§3's prose summary follows; the revision literals in its examples, marker
grammar, and §7 track the bump, as does the derived docs-site facts file.

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

Labels

None yet

Projects

None yet

1 participant