fix(herdr): scope lab helper calls with a leading --session - #3
Merged
Merged
Conversation
fm-herdr-lab.sh scoped every call by appending a trailing --session. Herdr stops parsing options at a bare --, which its own documented agent-argument passthrough requires, so `run <lab> agent start ... -- <agent args>` handed the isolating flag to the agent instead of the client. The client then fell back to the environment, where HERDR_SOCKET_PATH outranks HERDR_SESSION and, inside any Herdr-managed pane, points at the session that spawned it - the live default fleet session. The helper's whole purpose was defeated by a legitimate command. Scope every call with a leading --session, which Herdr parses before subcommand dispatch so no caller argument can displace it, and clear the ambient HERDR_SOCKET_PATH so even a total flag failure resolves to the lab rather than to the caller's own session. Agent-argument passthrough keeps working. Verified against Herdr 0.8.0: a passthrough command through the fixed helper is answered by the lab socket, the same command through the previous shape is answered by the live default server, and every lifecycle form the helper issues routes correctly with the flag leading. Regression coverage sits on both sides of the vendor boundary, because the verdict is the client's to give: tests/fm-herdr-lab.test.sh now models Herdr's real resolution order rather than asserting argument shape, and pins each measure with its own failing case; tests/fm-herdr-lab-isolation-e2e.test.sh guards the model against the installed client and proves it is not vacuous. That guard contacts no running server - it names a lab session and an ambient socket that do not exist, so the socket in the client's own error is the proof.
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
1 participant
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.
Intent
Close a confirmed fleet-safety hole in bin/fm-herdr-lab.sh, firstmate's own Herdr isolation helper.
THE DEFECT (reproduced, not theoretical). fm-herdr-lab.sh isolated a lab session by APPENDING a trailing flag in fm_herdr_lab_raw:
HERDR_SESSION="$name" herdr "$@" --session "$name". The argument guard rejects a caller-supplied --session and any leading option, but NOT a bare '--'. Herdr documents agent-argument passthrough asherdr agent start <name> --kind K --pane ID -- <agent-args...>, so a legitimate call producedherdr agent start x --kind claude --pane w1:p8 -- --model haiku --session fm-lab-..., where everything after '--' is an AGENT argument. The isolating --session was swallowed. The client then fell back to the ambient environment, and HERDR_SOCKET_PATH BEATS HERDR_SESSION; in a real crewmate environment that socket points at the fleet's live 'default' session. Sofm-herdr-lab.sh run <lab> ... -- <args>dispatched into the captain's live fleet - exactly the failure the helper exists to prevent. The contract's own line 'HERDR_SESSION alone is never accepted as isolation' is why it was not self-healing. Original reproduction: /Users/coder/firstmate/data/otel-herdr-surface/report.md section 1.3.REQUIRED OUTCOME. Make the helper isolate correctly no matter what arguments a caller passes, and make it impossible for this class of bypass to return silently.
The reporting scout proposed three measures, to be evaluated ON THE MERITS rather than applied mechanically, implementing what genuinely closes the hole and stating why anything skipped was rejected:
(1) Reject a bare '--' in the argument guard. Cheap and explicit, but it REMOVES a legitimate Herdr capability, so consider whether that is acceptable or whether passthrough should still work.
(2) Prepend instead of append -
herdr --session "$name" "$@". Herdr accepts a leading --session and no trailing '--' can displace it; preserves passthrough. Required: VERIFY EMPIRICALLY that a prepended --session is honoured for every subcommand form the helper supports, includingagent start ... -- <args>; do not assume the top-level usage line generalises.(3) Belt and braces: export HERDR_SOCKET_PATH to the lab socket inside fm_herdr_lab_raw so even total flag failure fails INTO the lab rather than into 'default'. Since socket path is what actually wins precedence, weigh this as arguably the most robust.
DECISIONS MADE AND WHY (deliberate; not oversights in the diff):
env -u HERDR_SOCKET_PATH) instead of exporting a computed lab socket path. Exporting one would require deriving ~/.config/herdr/sessions//herdr.sock, a layout Herdr owns; a wrong derivation would be pinned ABOVE the correct flag and is a worse failure than none. Clearing removes the only variable that can point at 'default', and it was verified that with the socket cleared, HERDR_SESSION alone resolves to the lab - so a total flag failure still lands in the lab, which is measure 3's actual goal.agent start ... -- <agent-args>, which is precisely what a Herdr lab exists to exercise. With measures 2 and 3 in place the separator is harmless. The pre-existing blanket rejection of a caller-supplied --session anywhere (including after '--') was deliberately left as-is rather than relaxed.REGRESSION COVERAGE (required: at minimum a test that a '--'-bearing command cannot reach the ambient session, colocated per repo convention). Because the verdict - which session a call is answered by - is vendor-emitted, .agents/skills/firstmate-coding-guidelines/SKILL.md requires two tests:
ALSO REQUIRED AND DONE: update the helper's header/usage text since the contract changed, and check whether the generated --herdr-lab brief text in bin/fm-brief.sh states anything now wrong - it did, in two places ('a trailing --session on every call' and 'The helper appends the required trailing --session'), both corrected, with tests/fm-brief.test.sh's assertion updated to match. docs/herdr-backend.md's lab-helper description corrected. A dated maintainer-verification record added to docs/verification/runtime-backends.md per the guidelines, pointing at the live guard as the refresh command.
SAFETY CONTEXT: this was fixed from inside the fleet it protects, where Herdr is the only worker runtime and 'default' hosts the captain's live fleet. All probes used an impossible pane ID and/or an unprovisioned lab name so a mis-routed call could only fail at resolution; no 'default' pane was touched; the default session was verified running and unchanged before and after, by socket path.
KNOWN PRE-EXISTING FAILURES, NOT INTRODUCED HERE (each confirmed to fail identically on the base commit with these changes stashed, and deliberately left alone rather than expanding scope): tests/fm-backend-herdr-presentation-e2e.test.sh workspace-ordering assertion, and tests/fm-kimi-harness.test.sh hook-install assertion. tests/fm-vendor-auth-probe.test.sh has a load-sensitive timing assertion that failed once under load and passes standalone.
What Changed
fm_herdr_lab_rawinbin/fm-herdr-lab.shnow issuesenv -u HERDR_SOCKET_PATH HERDR_SESSION="$name" herdr --session "$name" "$@"instead of appending a trailing--session. A leading flag is parsed before subcommand dispatch, so a caller's bare--agent-argument separator can no longer swallow it, and clearing the ambient socket path - which outranksHERDR_SESSIONand points at the spawning session inside any Herdr-managed pane - means even total flag failure resolves to the lab rather than todefault. The bare--is still accepted, soagent start ... -- <agent-args>keeps working; the guard against caller-supplied--sessionis unchanged, with its rejection message corrected to describe the leading flag.tests/fm-herdr-lab.test.sh's fake client was rebuilt to model Herdr's real resolution order (leading flag, thenHERDR_SOCKET_PATH, thenHERDR_SESSION, then default; option parsing stops at--) and now asserts on the resolved session under a realistic ambient socket, with an independently failing case per measure. Newtests/fm-herdr-lab-isolation-e2e.test.sh(familyreal-herdr-gated, registered inbin/fm-test-run.sh) guards that model against the installed client, including a control proving a--sessionafter--really is dropped; it contacts no running server, naming a lab session and ambient socket that both do not exist.--herdr-labbrief text inbin/fm-brief.sh(two now-wrong statements about a trailing--session, withtests/fm-brief.test.shupdated to match), the helper header,docs/herdr-backend.md, a stale cross-reference comment inbin/backends/zellij.sh, and a dated Herdr 0.8.0 verification record indocs/verification/runtime-backends.mdpointing at the live guard as its refresh command.Verified with
tests/fm-herdr-lab.test.sh(10/10),tests/fm-herdr-lab-isolation-e2e.test.sh(3/3 against herdr 0.8.0),tests/fm-herdr-session-cleanup-e2e.test.sh,tests/fm-brief.test.sh, and a before/after reproduction against real Herdr comparing the socket named in the client's own error. Mutation checks confirm each measure fails independently when reverted.The same trailing-flag shape in
fm_backend_herdr_cli(bin/backends/herdr.sh) was left alone: no caller passes a bare--and its arguments are internally constructed, and clearingHERDR_SOCKET_PATHthere could break a deliberate operator socket override. Rejecting a bare--outright (the scout's first proposed measure) was also rejected, since it would remove the passthrough capability a Herdr lab exists to exercise.Risk Assessment
✅ Low: The production change remains a single well-targeted line in fm_herdr_lab_raw plus a corrected error string, and the one substantive gap from round 1 - the primary leading --session measure having no independently failing regression case - is now closed by a toggle whose separation I verified by tracing both the current and reverted call shapes through the fake client's resolution order.
Testing
Ran the change's own regression tests (fake-client resolution model and the new real-Herdr isolation guard), the updated brief assertion, and the registry/docs meta-tests, all green; then went beyond pass/fail by reproducing the leak end-to-end against the installed Herdr 0.8.0 with the base helper and showing the fixed helper isolate under the identical ambient condition, mutation-checking each measure to prove its test fails independently, and exercising a real lab provision/run/teardown lifecycle plus a real-Herdr smoke test that route through the changed call site. No UI surface is involved - this is a shell helper, so the CLI transcript naming the socket that answered each call is the end-user-visible artifact. The live default session was confirmed running and unchanged by socket path around every real-Herdr run, no lab sessions or tripwires were left behind, and the only working-tree changes are two comment corrections.
Evidence: Before/after CLI transcript: passthrough isolation against real Herdr 0.8.0
$ herdr --version herdr 0.8.0 # Ambient crewmate identity for every probe below (Herdr exports this into # every pane it manages, pointing at the session that spawned the caller): # HERDR_SOCKET_PATH=/tmp/fm-evid-fleet-default.sock # HERDR_SESSION=default # Lab session under test: fm-lab-evid-70666 == BEFORE (base bb95bc6: trailing --session) == $ fm-herdr-lab.sh run fm-lab-evid-70666 agent start probe --kind claude --pane wZZ:p999999 -- --model haiku {"id":"cli:agent:start","error":{"code":"server_not_running","message":"no herdr server is running at /tmp/fm-evid-fleet-default.sock; runherdrto start or attach it"}} >>> LEAK: answered at the AMBIENT socket /tmp/fm-evid-fleet-default.sock - the captain's live fleet, not the lab. $ fm-herdr-lab.sh run fm-lab-evid-70666 agent start probe --kind claude --pane wZZ:p999999 (no passthrough separator) {"id":"cli:agent:start","error":{"code":"server_not_running","message":"no herdr server is running at /Users/coder/.config/herdr/sessions/fm-lab-evid-70666/herdr.sock; runherdr session attach fm-lab-evid-70666to start or attach it"}} >>> Control: without a bare -- the old shape did isolate, which is why the hole stayed hidden. == AFTER (e89da54: leading --session + cleared socket) == $ fm-herdr-lab.sh run fm-lab-evid-70666 agent start probe --kind claude --pane wZZ:p999999 -- --model haiku {"id":"cli:agent:start","error":{"code":"server_not_running","message":"no herdr server is running at /Users/coder/.config/herdr/sessions/fm-lab-evid-70666/herdr.sock; runherdr session attach fm-lab-evid-70666to start or attach it"}} >>> ISOLATED: answered at the LAB socket. Passthrough still works; the ambient socket is never touched. $ fm-herdr-lab.sh run fm-lab-evid-70666 workspace list {"id":"cli:workspace:list","error":{"code":"server_not_running","message":"no herdr server is running at /Users/coder/.config/herdr/sessions/fm-lab-evid-70666/herdr.sock; ..."}} $ fm-herdr-lab.sh run fm-lab-evid-70666 status --json {"client":{"version":"0.8.0","protocol":19,"session":"fm-lab-evid-70666"},"server":{"running":false,"socket":"/Users/coder/.config/herdr/sessions/fm-lab-evid-70666/herdr.sock","session":"fm-lab-evid-70666"}} >>> Every form the helper issues resolves to the lab session socket.Evidence: Mutation check: each implemented measure has its own failing test
== Revert measure 2 (leading --session -> trailing --session) == 61: env -u HERDR_SOCKET_PATH HERDR_SESSION="$name" herdr "$@" --session "$name" ok - fm-herdr-lab: an agent-argument passthrough command cannot reach the ambient session not ok - a passthrough command without the session env fallback reached session 'default' instead of the lab: agent start probe --kind claude --pane w1:p8 -- --model haiku --session fm-lab-leading-flag-75593 == Revert measure 3 (drop env -u HERDR_SOCKET_PATH) == 61: HERDR_SESSION="$name" herdr --session "$name" "$@" not ok - a command whose session flag was ignored reached session 'default' instead of the lab: workspace list == Working tree restored == 61: env -u HERDR_SOCKET_PATH HERDR_SESSION="$name" herdr --session "$name" "$@"Evidence: Live lab lifecycle against real Herdr, default fleet unchanged
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 4 issues found → auto-fixed ✅
tests/fm-herdr-lab.test.sh:214- The intent states "Each measure has its own independently failing case, verified by reverting each in turn", but measure 2 (the prepended --session) has no such case. Restoring the original append shape at bin/fm-herdr-lab.sh:61 (env -u HERDR_SOCKET_PATH HERDR_SESSION="$name" herdr "$@" --session "$name") leaves the entire suite green: in test_passthrough_cannot_reach_the_ambient_session the fake stops flag parsing at the bare--so flag_session is empty, butenv -uhas already removed HERDR_SOCKET_PATH, so the fake falls through to HERDR_SESSION and still resolves to the lab. test_provision_run_and_guarded_teardown also still passes, because the fake strips--sessionfrom args before logging, so the stop/delete grep patterns match either ordering. Measure 3 masks measure 2, so the measure the intent calls primary is the only one with no regression guard - and it is the one that must hold if a future client drops HERDR_SESSION support. Suggested fix: add a fake toggle that ignores HERDR_SESSION (mirroring the existing FM_FAKE_HERDR_IGNORE_SESSION_FLAG at tests/fm-herdr-lab.test.sh:51) and assert a---bearing call still reaches the lab under it.bin/fm-herdr-lab.sh:144- The guard's error message still says "run forbids caller-supplied --session; the helper appends the lab session". The helper no longer appends - line 61 now prepends the flag, and the header comment at lines 16-19 was updated to say so. This is the only text a caller actually sees when the guard fires, and it now describes the exact shape the change exists to remove, so anyone debugging a rejection is pointed at the wrong contract. Change "appends" to "prepends" (or "scopes the call with a leading --session") to match the header, bin/fm-brief.sh, and docs/herdr-backend.md, all of which were corrected in this change.tests/fm-herdr-lab-isolation-e2e.test.sh:73- The non-vacuity control deliberately issues a raw, unroutedherdr agent startfrom inside the fleet it is protecting: the isolating flag sits after the passthrough separator by design, so routing falls to HERDR_SOCKET_PATH=/tmp/fm-lab-amb-$$.sock. That path is never created, so on herdr 0.8.0 the call fails naming it and the assertion holds. But if a client ever resolved a missing socket by falling back to the live default server instead of erroring, this call would be dispatched into the captain's fleet, and the only remaining guard is the impossible pane id wZZ:p999999 causing a resolution-time rejection. The test also does no cleanup and records no tripwire, relying entirely on both the lab session and the ambient socket not existing. Noting the tradeoff, which the test comments already acknowledge, rather than proposing a change - the control is required for the guard not to be vacuous.tests/fm-herdr-lab.test.sh:32- The fake client's comment citesdocs/verification/runtime-backends.md "Herdr lab session routing", but the section added by this change is headed "Lab session routing" (docs/verification/runtime-backends.md:210 also refers to it by that name). The pointer that justifies the fake's resolution model does not match any heading in the file it names.🔧 Fix: Give the leading --session measure its own failing test
✅ Re-checked - no issues remain.
tests/fm-send-secondmate-marker-herdr-e2e.test.sh:15- Applied two comment-only corrections in test files that still described the pre-fix helper contract, so the working tree is not clean: tests/fm-send-secondmate-marker-herdr-e2e.test.sh said the lab helper "appends its own required trailing --session before invoking real Herdr" (now "scopes the call with its own leading --session"), and tests/fm-backend-herdr-eventwait-smoke.test.sh said a routed call "carries the trailing --session" (now "is scoped to the lab session"). No assertions or behavior changed; both files still pass against real Herdr. Flagged because leaving the first statement in place is exactly the guidance that would lead a future maintainer to reintroduce this bug.bash tests/fm-herdr-lab.test.sh- 10/10 pass, includingtest_passthrough_cannot_reach_the_ambient_session,test_leading_flag_alone_survives_the_passthrough_separator, andtest_isolation_survives_losing_the_session_flagbash tests/fm-herdr-lab-isolation-e2e.test.sh- 3/3 pass against the installedherdr 0.8.0, including the control proving the client really drops a--sessionplaced after--bash tests/fm-brief.test.sh- pass, covering the corrected--herdr-labcontract wordingManual before/after reproduction against real Herdr 0.8.0: ran the base-commit helper (git show bb95bc6:bin/fm-herdr-lab.sh) and the target helper with identical argsrun <lab> agent start probe --kind claude --pane wZZ:p999999 -- --model haikuunderHERDR_SOCKET_PATH/HERDR_SESSION=default, comparing the socket named in the client's own errorMutation check: reverted measure 2 (leading--session-> trailing) and re-rantests/fm-herdr-lab.test.sh-not ok - a passthrough command without the session env fallback reached session 'default'Mutation check: reverted measure 3 (droppedenv -u HERDR_SOCKET_PATH) and re-rantests/fm-herdr-lab.test.sh-not ok - a command whose session flag was ignored reached session 'default'bash tests/fm-herdr-session-cleanup-e2e.test.sh- live lab provision/run/teardown through the changedfm_herdr_lab_rawagainst real Herdr, 3/3 pass with the default-fleet tripwire armed and byte-identicalbash tests/fm-backend-herdr-eventwait-smoke.test.sh- 3/3 pass, exercisingpane report-agentthroughfm_herdr_lab_cliagainst real Herdrbash tests/fm-test-run.test.sh,bash tests/fm-test-isolation-proof.test.sh,bash tests/fm-documentation-audiences.test.sh- pass, covering the new test's family registration and the changed docsbin/fm-test-run.sh --list --family real-herdr-gatedplus a scan of every--list-laneslane - confirmsfm-herdr-lab-isolation-e2e.test.shis real-herdr-gated only and never scheduled into a portable parallel laneherdr session list --jsonbefore and after every real-Herdr run - the livedefaultsession stayed running with an unchanged socket path and nofm-lab-session was left behindbin/backends/herdr.sh:367- Two code comments cite a docs/herdr-backend.md section titled "Session targeting: the --session flag, not HERDR_SESSION alone" (bin/backends/herdr.sh:367) and "Session targeting" (bin/fm-backend.sh:845), but no such heading exists; that material now lives under "## Current transport behavior". This is a pre-existing dangling pointer, unrelated to this change's contract, so I left it rather than widening the diff. Worth a one-line follow-up to repoint both comments.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.