7.4 KiB
Cross-Code Review — Job e73917f8 (B-3: command -v herdr preflight bypass)
- Job: e73917f8
- Reviewer: cline
- Target: Resolve B-3 —
command -v herdrfalsely matches theherdr()bash function inlib.sh(andtype -P herdrmatches the.mam/shimwrapper), so pre-flight passes on a host with no realherdrbinary. - Date: 2026-08-06
1. Scope & Diff Summary
6 files modified (+68 / −14) plus 1 new test suite (untracked):
| File | Change |
|---|---|
.agents/skills/lib.sh |
+44: new _canonical_file(), _is_shim_path(), rewritten _resolve_real_herdr_path() (PATH scan skipping shim dirs + canonicalised symlink target), new has_real_herdr(). Both _HERDR_SHIM_DIR_PATTERN and _HERDR_SKILLS_BIN_PATTERN defined before use. |
.agents/skills/multi-agent-mux-create/scripts/create_session.sh |
preflight command -v herdr || type -P herdr → has_real_herdr |
.agents/skills/multi-agent-mux-delegate-job/multi-agent-mux-delegate-job |
preflight command -v herdr → has_real_herdr (in run_agent()) |
.agents/skills/multi-agent-mux-create/SKILL.md |
doc preflight → has_real_herdr + explanatory comment |
.agents/skills/multi-agent-mux-status/SKILL.md |
doc preflight → has_real_herdr + blockquote warning |
IMPROVEMENTS.md |
B-3 moved §2(open, 8→7) → §5(completed, 5→6); counts/roadmap updated |
tests/test_b3_herdr_preflight.py |
new, 196 lines, 10 functions / 11 cases (W-1…W-10, W-4 parametrised ×2) |
2. Verification Performed
| Check | Method | Result |
|---|---|---|
| Bash syntax | bash -n on lib.sh, create_session.sh, delegate-job |
PASS (all 3) |
| B-3 unit tests | pytest tests/test_b3_herdr_preflight.py -v |
11/11 PASS (0.67s) |
| Regression | pytest tests/test_workspace_scope.py tests/test_b1_tier3_identity.py -q |
10/10 PASS |
| Lint (shellcheck) | command -v shellcheck |
Not installed in env — pre-existing limitation; bash -n substitutes. Not introduced by B-3. |
| Completeness | grep -rn 'command -v herdr|type -P herdr' .agents/skills --include='*.sh' |
Only an explanatory comment (create_session.sh:84) remains; no executable preflight left. All 4 preflight sites migrated. |
| Source-order | grep lib.sh/has_real_herdr in each script |
lib.sh sourced before has_real_herdr at both call sites (create_session.sh:22→86; delegate-job:46→449). create_session.sh additionally proven e2e by W-5/W-6/W-7. |
3. Logic Audit
3.1 The two bypasses (confirmed by W-1)
command -v herdr→ matchesherdr()shell function (lib.sh:540 →mam_herdr).type -P herdr→ matches$WORKSPACE_ROOT/.mam/shim/herdr, because_init_herdr_isolationruns at source time (lib.sh:1784) and prepends the shim dir to PATH. Both pass on a herdr-less host → preflight was void. W-1 documents this so a future refactor cannot silently regress.
3.2 New resolver (_resolve_real_herdr_path + _is_shim_path + _canonical_file)
- Critical check PASSED:
_HERDR_SHIM_DIR_PATTERN(line 48,/multi-agent-herdr-shim/) and_HERDR_SKILLS_BIN_PATTERN(line 49,/.agents/skills/.bin) are defined before_is_shim_pathreferences them. An unset var would have made the glob**match every path (false-reject all). No such bug. _is_shim_pathmatches"/$1/"(leading+trailing slash normalisation) against:*/.mam/shim/*,*-shim/*,*/multi-agent-herdr-shim/*,*/.agents/skills/.bin/*. Bare relative entry.mam/shim→/.mam/shim/→ caught (W-10).- Double shim check: dir-level (line 81) and canonical-file-level (line 85). A symlink in a normal bin dir pointing into
.mam/shimis resolved by_canonical_file(readlink loop ≤40 hops +cd -P/pwd -P) and rejected on the resolved target (W-8). A symlink to a real binary is accepted (W-9) — canonicalisation is not a blanket symlink rejection. - IFS save/restore around
for dir in $PATH; restored after the loop even onbreak. Empty PATH elements skipped (safe — never picks cwd)._canonical_filefailure falls back to the literal path. Sound.
3.3 Operability — no herdr() regression
herdr()(lib.sh:540) →mam_herdr()(lib.sh:531) → executes$WORKSPACE_ROOT/.mam/shim/herdrdirectly. It does not use lib.sh's_REAL_HERDR_PATH.- The shim wrapper generated by
_init_herdr_isolation(lib.sh:112-180) carries its own embedded_resolve_real_herdr()(lib.sh:118-132) and a localREAL_HERDR— fully self-contained. - Old code set
_REAL_HERDR_PATH="herdr"unconditionally at definition; new code sets it only inside_resolve_real_herdr_pathwhen a real binary is found. Since no consumer (herdr()/shim wrapper) depends on_REAL_HERDR_PATHbeing pre-set, removing the unconditional default is safe. Confirmed by regression suite (10/10) and the e2e W-5/W-6/W-7 tests.
3.4 IMPROVEMENTS.md arithmetic
- Open: 15 = 2 (arch) + 7 (edge, was 8, −B-3) + 2 (orch) + 4 (legacy). ✓
- Completed: 6 = A-1, A-5, B-1, B-3, C-1, O-1. ✓
4. Findings
R-1 (Minor, Non-blocking) — Broad -shim/* pattern is a conservative false-positive
_is_shim_path's *-shim/* glob will reject a legitimate herdr installed in any directory whose canonical path contains the substring -shim/ (e.g. /opt/foo-shim/bin/herdr). This fails closed (preflight says "not installed" when it is) — the safe direction — and is explicitly tested behaviour (W-4 with my-shim/multi-agent-herdr-shim). Acceptable for a preflight gate; a user hitting this can place herdr in a differently-named dir or override _HERDR_SHIM_DIR_PATTERN. No action required; recorded for awareness.
R-2 (Minor, Non-blocking, pre-existing / out-of-scope) — Shim wrapper's own resolver is less strict
The shim wrapper's embedded _resolve_real_herdr() (lib.sh:118-132) uses a simpler dir-name-substring check (no canonical-file symlink resolution) than the new lib.sh has_real_herdr. Thus a symlink-in-normal-dir pointing into .mam/shim is rejected by the preflight but would, if preflight were bypassed, still be selectable by the shim wrapper's resolver (potential recursion). This is a pre-existing characteristic of the shim wrapper, not touched by the B-3 diff, and the new preflight gate now catches it first. Not a B-3 defect; noted as a future consistency follow-up.
R-3 (Info) — shellcheck gate not runnable here
shellcheck is not installed in this environment, so the IMPROVEMENTS §7-D shellcheck gate could not be re-run for the changed files. Pre-existing environment limitation, unchanged by B-3. bash -n syntax validation PASS on all three scripts.
5. Conclusion
B-3 is fully and correctly resolved. The root cause (two independent bypasses — herdr() function match + .mam/shim wrapper match) is correctly diagnosed and fixed with a real-binary PATH scan that skips shim/wrapper dirs at both the directory-name and canonicalised-symlink-target levels. All 4 preflight sites are migrated; no executable command -v herdr/type -P herdr preflight remains. The fix cannot regress the herdr()→shim-wrapper runtime path (independent of _REAL_HERDR_PATH). 11/11 new tests + 10/10 regression tests PASS. IMPROVEMENTS.md bookkeeping is arithmetically consistent. Findings R-1/R-2/R-3 are all non-blocking (R-1/R-2 are conservative-direction or out-of-scope; R-3 is an env limitation).
No design-level rework is warranted — this is a clean, well-tested bug fix.
[VERDICT: PASS]