Repository navigation
feat(bin): add harness adapter interface - #1265
sbracewell64 wants to merge 3 commits into
Conversation
f2089a8 to
6a2952d
Compare
Extract the per-harness busy signature that bin/fm-tmux-lib.sh owned into a harness-axis adapter layer - bin/fm-harness-adapter.sh plus bin/harnesses/<name>.sh - with ZERO behavior change. The busy table is the most-consumed harness fact in the system: 8 call sites across 5 files. It lived in a BACKEND-named library, so bin/fm-crew-state.sh, bin/fm-supervise-daemon.sh, and bin/fm-pending-reply-lib.sh each had to source a tmux library purely to read harness knowledge, and bin/fm-crew-state.sh routes non-tmux backends through it explicitly. bin/fm-watch.sh reached it transitively through a sibling. After this change bin/fm-pending-reply-lib.sh and bin/fm-watch.sh need no tmux library at all, and bin/fm-tmux-lib.sh keeps only the tmux pane probe that consumes the signature. The dispatcher mirrors bin/fm-backend.sh: a known-name registry, per-adapter files that are each their own lint root, and thin per-op dispatch. It keeps TWO deliberately different name sets - FM_HARNESS_KNOWN for crewmate and secondmate launches, and the narrower FM_HARNESS_PRIMARY for the primary session. kimi is in the first and not the second, which is why there is no docs/supervision-protocols/kimi.md and no kimi arm in bin/fm-supervision-instructions.sh; collapsing the sets would silently promote kimi to a primary harness. Resolving the pi-signed -> pi alias once in the dispatcher retires 15 scattered `pi|pi-signed` case arms. Two deliberate departures from the backend template, both because harness adapters are pure data rather than command sequences. Adapters are loaded eagerly at source time, not lazily per call: the matcher reads stdin, so it always runs as a pipeline subshell and a lazy source could never populate a cross-call guard - it would re-read the adapter on every busy check. And each signature is a constant rather than a function, so the hot path resolves it with a variable read instead of a fork. Measured over 200 checks, the result is at parity with the pre-refactor code (1526us vs 1576us per check); the intermediate lazy-plus-command-substitution shape was 2x slower. Byte-identical behavior is proven, not asserted. tests/fm-harness-adapter.test.sh runs the PRE-refactor fm_busy_lines_match, extracted from the merge-base with main, and the refactored fm_harness_busy_match against the same capture corpus in separate processes, then diffs the verdict logs: 108 harness/capture pairs, byte-identical. It also diffs the resolved regex string for every harness name, the empty name, and an unregistered name. tests/lib.sh gains fm_test_base_ref as the single owner of old-vs-new baseline resolution, shared with tests/fm-backend.test.sh. It now picks the NEWEST merge-base among candidate refs rather than the first that resolves: a disposable task worktree is routinely cut from a pool whose local main is many commits stale, and pinning the baseline there made unrelated landed changes read as behavior differences. Contract docs updated in the same change: AGENTS.md section 4, CONTRIBUTING.md (whose harness-adapter bullet named the old busy-signature location), .agents/skills/harness-adapters/SKILL.md, bin/fm-lint.sh's roots and its canonical-file-set test, and bin/fm-test-run.sh's family map.
6a2952d to
f3c2308
Compare
|
Closing: superseded by #1327 ( This PR moved the per-harness busy signature out of Continuing would also create a second competing per-harness busy owner alongside #1327's own The remaining stages of this work (turn-end install/cleanup, process-name identity, and launch command/flags) are unaffected by #1327 and will ship as separate PRs against current |
Intent
Ship phase 3 of the firstmate harness-interface project: extract a harness adapter interface (bin/fm-harness-adapter.sh + bin/harnesses/.sh) around the per-harness busy signature that bin/fm-tmux-lib.sh used to own, with ZERO behavior change. This is PR A of an approved five-PR, method-sliced staging (A: busy signature; B: turn-end install/cleanup; C: process identity; D: launch command and flags; E: fold per-harness supervision prose into docs/supervision-protocols). Method-slicing was chosen over harness-slicing specifically to avoid a temporary dual code path where per-harness knowledge would live in two shapes across two PRs.
Deliberate framing: this imitates the maintainer's own adapter-layer template in upstream PR #183 (feat: a new interface, a byte-identical-behavior guarantee stated in the PR body, old-versus-new conformance tests as the evidence, and every contract doc updated in the same PR). Byte-identical behavior is a HARD GATE here, not an aspiration: no behavior change is intended anywhere in this diff.
Motivation (measured, from a prior architecture review): the busy table is the most-consumed harness fact in the system - 8 call sites across 5 files - and it lived in a BACKEND-named library. bin/fm-crew-state.sh, bin/fm-supervise-daemon.sh, and bin/fm-pending-reply-lib.sh each had to source a tmux library purely to read harness knowledge, and bin/fm-crew-state.sh routes NON-tmux backends through it explicitly. bin/fm-watch.sh reached it transitively through a sibling library. After this change bin/fm-pending-reply-lib.sh and bin/fm-watch.sh need no tmux library at all.
Decisions a reviewer reading only the diff would not know:
The registry deliberately keeps TWO different harness name sets. FM_HARNESS_KNOWN (7 names) is launchable as a crewmate/secondmate per docs/configuration.md; FM_HARNESS_PRIMARY (6 names) is supported for the primary session per README.md Requirements. kimi is deliberately in the first and NOT the second. That is exactly why there is no docs/supervision-protocols/kimi.md and no kimi arm in bin/fm-supervision-instructions.sh - those absences are correct, not drift. Do NOT suggest merging the two lists or adding a kimi arm: it would silently promote kimi to a primary harness and contradict what README advertises.
Two deliberate departures from the bin/fm-backend.sh template, both because harness adapters are pure data rather than command sequences. (a) Adapters load EAGERLY at source time, not lazily per call: fm_harness_busy_match reads stdin, so it always runs as a pipeline subshell, and a lazy source could never populate a cross-call guard - it would re-read the adapter on every busy check in the watcher poll loop. (b) Each signature is a CONSTANT, not a function, so the hot path resolves it with a variable read instead of a fork. Measured over 200 checks: this is at parity with pre-refactor code (1526us vs 1576us per check), while an intermediate lazy-plus-command-substitution shape measured 2x slower. The eager-load rationale is documented at the foot of the dispatcher.
The SC2034 'appears unused' disables in bin/harnesses/*.sh are deliberate: each adapter is linted as its own canonical root, so its consumer (the dispatcher) is out of shellcheck's scope. bin/fm-lint.sh's ROOTS and tests/fm-lint.test.sh's canonical file set were both extended for the new directory.
tests/lib.sh gains fm_test_base_ref as the single owner of old-vs-new baseline resolution, shared with the pre-existing tests/fm-backend.test.sh (whose private copy was removed). It now picks the NEWEST merge-base among candidate refs rather than the first that resolves. This is a real bug fix, not cosmetics: a disposable task worktree is routinely cut from a pool whose local main is many commits stale, and pinning the baseline there made unrelated already-landed changes read as behavior differences. Verified that tests/fm-backend.test.sh still passes with the corrected, newer baseline.
tests/fm-kimi-harness.test.sh's two location assertions were retargeted from grepping bin/fm-tmux-lib.sh's source text to asserting on the RESOLVED regex, so they follow the fact instead of its address. Its behavioral assertions were not changed.
Deliberately OUT of scope, do not flag as missing:
Evidence: tests/fm-harness-adapter.test.sh runs the PRE-refactor fm_busy_lines_match (extracted from the merge-base with main) and the refactored fm_harness_busy_match against the same capture corpus in separate processes, then diffs the verdict logs byte-for-byte - 108 harness/capture pairs identical - plus a byte-identical diff of the resolved regex string for every harness name, the empty name, and an unregistered name, and safety assertions that an unregistered harness never borrows another's signature and FM_BUSY_REGEX still overrides everything. A real-tmux end-to-end run on an isolated socket confirmed the capture-to-adapter-to-verdict path.
Note on the local test suite: seven test files fail on this machine (fm-backend-tmux-smoke, fm-calm-pi-extension, fm-watcher-lock, fm-pi-watch-extension, fm-turnend-guard, fm-secondmate-harness, fm-session-start). Each was verified to fail IDENTICALLY on a clean checkout of the base commit c0c0881 with none of these changes present - they are environmental to this machine (WSL2 tmux, node ESM, watcher beacon timing, and an ancestry test that detects the real claude process the session runs inside), not regressions from this work.
What Changed
bin/fm-harness-adapter.shplus per-harness data adapters underbin/harnesses/(claude, codex, grok, kimi, opencode, pi) — that now owns the per-harness busy signature previously hard-coded inbin/fm-tmux-lib.sh. All 8 call sites acrossbin/fm-crew-state.sh,bin/fm-supervise-daemon.sh,bin/fm-pending-reply-lib.sh,bin/fm-watch.sh, andbin/fm-tmux-lib.shresolve busy state through the adapter, sofm-pending-reply-lib.shandfm-watch.shno longer source any tmux library. Adapters load eagerly as constants (parity with pre-refactor timing), and the registry keeps the crewmate-launchable (FM_HARNESS_KNOWN) and primary-session (FM_HARNESS_PRIMARY) name sets deliberately separate.tests/fm-harness-adapter.test.shruns the pre-refactor matcher (extracted from the merge-base) and the newfm_harness_busy_matchagainst the same capture corpus and diffs verdicts byte-for-byte across 108 harness/capture pairs, plus resolved-regex diffs per harness and safety checks that unregistered harnesses borrow nothing andFM_BUSY_REGEXstill overrides.tests/lib.shgains a sharedfm_test_base_refbaseline resolver that picks the newest merge-base (fixing false diffs in worktrees cut from a stale local main), and the kimi test's location assertions now target the resolved regex instead of source-text greps.bin/fm-lint.shROOTS,tests/fm-lint.test.sh), the gotmp test fixtures, and docs (CONTRIBUTING.md,docs/configuration.md,docs/scripts.md,docs/tmux-backend.md, agent skills) were extended for the newbin/harnesses/directory; the review pipeline caught and auto-fixed a missed gotmp fixture symlink and a stale shellcheck-inventory doc line.Risk Assessment
✅ Low: The fix commit resolves both round-1 findings exactly as instructed (verified by re-running the fixture sourcing repro, which now loads fully under set -eu), and the underlying branch remains the deliberately mechanical, conformance-tested byte-identical extraction already verified in round 1.
Testing
Ran the four touched test suites (harness-adapter conformance, backend, kimi-harness, gotmp) — all pass, including the byte-identical gate diffing 108 pre-refactor vs refactored busy verdicts and every resolved regex string against the merge-base — and performed a manual real-tmux end-to-end check on an isolated socket proving the capture-to-adapter-to-verdict path yields correct busy/idle classifications; tests/fm-lint.test.sh was left to the lint step per the no-linters rule, and the working tree was left clean.
Evidence: Harness-adapter conformance test transcript (byte-identical gate)
ok - harness registry: the crewmate set and the narrower primary set stay distinct ok - harness registry: pi-signed resolves to pi once, so call sites drop their own alias arms ok - harness registry: every verified harness has a sourceable adapter with a non-empty signature ok - busy signatures: every harness's regex string is byte-identical to the pre-refactor constant ok - busy verdicts: 108 harness/capture pairs classify identically old vs new ok - safety: an unregistered harness is never classified busy by borrowing a verified signature ok - safety: claude's and kimi's scoped signatures stay out of the shared fallback ok - safety: FM_BUSY_REGEX still overrides every per-harness signature all fm-harness-adapter tests passedEvidence: Real-tmux end-to-end: capture-pane → adapter → verdict
=== real-tmux end-to-end: capture-pane -> fm-harness-adapter -> busy/idle verdict === tmux tmux 3.6, isolated socket: fm-adapter-e2e-36202 --- pane capture (claude busy spinner on screen) --- 1:✻ Thinking… (12s · esc to interrupt) verdict harness='claude' -> busy verdict harness='EMPTY' -> busy verdict harness='codex' -> busy --- pane capture (idle composer prompt on screen) --- 1:│ > │ verdict harness='claude' -> idle verdict harness='EMPTY' -> idle === done ===Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
tests/fm-gotmp.test.sh:60- tests/fm-gotmp.test.sh's fake bin/ trees (built at lines 53-61 and repeated at 158-162) symlink the real bin/fm-tmux-lib.sh and bin/fm-composer-lib.sh but not the new bin/fm-harness-adapter.sh or bin/harnesses/. fm-tmux-lib.sh now sources fm-harness-adapter.sh via dirname of BASH_SOURCE, which resolves to the fake tree, so the source fails (reproduced: 'No such file or directory', exit 1 under set -eu). The teardown tests currently pass only because every tmux-backend dispatch on their path is '|| true'-guarded, which suspends errexit and lets the library half-load with fm_harness_busy_match undefined and FM_HARNESS_BUSY_REGEX_DEFAULT unset — the fail-open state the adapter's fail-closed load guard exists to prevent. tests/fm-backend.test.sh's equivalent fixture was updated for the new sibling (copies fm-harness-adapter.sh and bin/harnesses/); this fixture was missed. Fix: symlink fm-harness-adapter.sh and the harnesses/ directory into both fake trees..agents/skills/firstmate-coding-guidelines/SKILL.md:97- The shellcheck inventory prose in .agents/skills/firstmate-coding-guidelines/SKILL.md line 97 still reads "bin/*.shandbin/backends/*.shmust passshellcheck" — this PR extended the canonical lint set with bin/harnesses/.sh (bin/fm-lint.sh ROOTS, tests/fm-lint.test.sh CANON, and CONTRIBUTING.md were all updated), so this doc line is now stale. Add bin/harnesses/.sh to it.🔧 Fix: link harness adapter into gotmp fixtures; update lint docs
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-harness-adapter.test.sh— registry name sets, pi-signed alias, byte-identical regex strings and 108 old-vs-new verdict pairs, unregistered-harness and FM_BUSY_REGEX safety propertiesbash tests/fm-backend.test.sh— backend conformance suite now consuming the shared fm_test_base_ref baseline resolverbash tests/fm-kimi-harness.test.sh— kimi suite with location assertions retargeted to the resolved regexbash tests/fm-gotmp.test.sh— gotmp fixtures with the adapter linked inManual real-tmux end-to-end on isolated socket: rendered claude busy spinner and idle composer prompt in a live pane, piped capture-pane output through fm_harness_busy_match, verified busy/idle verdictsSanity-checked that bin/fm-pending-reply-lib.sh and bin/fm-watch.sh no longer source a tmux library for busy matching✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.