feat(create,resume,stop): verify and standardize --herdr-session option with full peer review
- Standardize --herdr-session as primary flag with --herdr-server alias across create_session.sh, resume_session.sh, update_yaml_resumed.sh, and stop_session.sh - Guard HERDR_SESSION_NAME in create_session.sh from being overwritten by workspace slug defaults when explicitly provided - Forward explicit --herdr-session from resume_session.sh to update_yaml_resumed.sh and force-update row metadata - Add 5 new Tier 2 component tests covering CLI dry-run parsing, usage matching, default preservation, YAML serialization, and resume propagation - Update multi-agent-mux-create/SKILL.md documentation - Verified by autonomous multi-agent loop with unanimous PASS verdicts from Claude and Cline
This commit is contained in:
@@ -0,0 +1,264 @@
|
||||
# Cross-Code Review — Job 40944efc
|
||||
|
||||
- **Reviewer**: cline
|
||||
- **Target**: `--herdr-session` (alias `--herdr-server`) standardization across 6 files (`+245 / −56`)
|
||||
- **Base commit**: working tree (unstaged diff)
|
||||
- **Date**: 2026-08-24
|
||||
|
||||
---
|
||||
|
||||
## §0. Verdict Summary
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| `bash -n` (4 changed shell scripts) | 4/4 OK |
|
||||
| Changeset-specific tests (6) | 6/6 PASS |
|
||||
| Full pytest suite (parallel run) | 346 passed, 0 failed (462.39s) |
|
||||
| `--herdr-session` parsing consistency (4 scripts) | Consistent |
|
||||
| `HERDR_SESSION_NAME` not clobbered when explicit | Verified (3 guard sites) |
|
||||
| Companion script forwarding (resume → update_yaml) | Both call sites forward |
|
||||
| Backward compat (`--herdr-server`, `HERDR_SERVER_NAME`) | Retained as alias/fallback |
|
||||
| SKILL.md documentation | Updated, duplicate block removed |
|
||||
|
||||
**Previous N-1 (resume post-spawn not forwarding `--herdr-session`): FIXED.**
|
||||
|
||||
---
|
||||
|
||||
## §1. create_session.sh — Guard Hardening
|
||||
|
||||
### 1.1 Three guard sites verified
|
||||
|
||||
All three sites now wrap the clobbering logic in `if [ -z "$HERDR_SERVER_OPT" ]; then … fi`, so an explicitly provided `--herdr-session` value is never overwritten:
|
||||
|
||||
| Site | Location | Behavior when `--herdr-session` explicit |
|
||||
|---|---|---|
|
||||
| ① ws_slug default | ~line 135 | Skipped — `HERDR_SESSION_NAME` preserved |
|
||||
| ② spawn() internal | ~line 174 | Skipped — `HERDR_SESSION_NAME` preserved |
|
||||
| ③ post-spawn resolve | ~line 211 | Skipped — `resolve_herdr_workspace` not called |
|
||||
|
||||
**Line 78-80** (pre-guard): `export HERDR_SESSION_NAME="$HERDR_SERVER_OPT"` is set immediately after arg parsing, before any guard can interfere. ✅
|
||||
|
||||
### 1.2 Dry-run output
|
||||
|
||||
```bash
|
||||
echo "[dry-run] would spawn: herdr session '$SESSION_NAME' in $WORKSPACE (agent=$AGENT, herdr_session=${HERDR_SESSION_NAME:-default})"
|
||||
```
|
||||
Correctly surfaces the resolved `herdr_session` value. ✅
|
||||
|
||||
### 1.3 YAML serialization
|
||||
|
||||
`atomic_dump_yaml` receives `HERDR_SESSION_NAME="${HERDR_SESSION_NAME:-default}"` as an env var (line ~289). Inside the Python heredoc:
|
||||
|
||||
```python
|
||||
server_name = os.environ.get('HERDR_SESSION_NAME', 'default') # line ~297
|
||||
…
|
||||
'herdr_session': server_name, # line ~309
|
||||
'herdr_server': server_name, # line ~310
|
||||
'start_command': f'HERDR_SESSION_NAME={server_name} herdr agent attach {name}',
|
||||
'attach_command': f'HERDR_SESSION_NAME={server_name} herdr agent attach {name}',
|
||||
'kill_command': f'HERDR_SESSION_NAME={server_name} herdr kill-session -t {name}',
|
||||
```
|
||||
|
||||
All 5 fields (`herdr_session`, `herdr_server`, `start_command`, `attach_command`, `kill_command`) are derived from `server_name`. ✅
|
||||
|
||||
### 1.4 `--herdr-session default` edge case
|
||||
|
||||
Test `test_comp_create_herdr_session_default_preserved` passes `--herdr-session default` and asserts `herdr_session == "default"` in YAML. The guard `if [ -z "$HERDR_SERVER_OPT" ]` is false (since `HERDR_SERVER_OPT="default"` is non-empty), so the ws_slug override is skipped and the literal `"default"` is preserved. ✅
|
||||
---
|
||||
|
||||
## §2. resume_session.sh — Both Call Sites Forward `--herdr-session`
|
||||
|
||||
### 2.1 Argument parsing
|
||||
|
||||
```bash
|
||||
--herdr-session|--herdr-server) HERDR_SERVER_OPT="$2"; shift 2 ;;
|
||||
```
|
||||
Consistent with create/stop. `HERDR_SERVER_OPT=""` initialized → `set -u` safe. ✅
|
||||
|
||||
### 2.2 Export logic (lines 57-62)
|
||||
|
||||
```bash
|
||||
if [ -n "$HERDR_SERVER_OPT" ]; then
|
||||
export HERDR_SESSION_NAME="$HERDR_SERVER_OPT"
|
||||
else
|
||||
HERDR_SESSION_NAME="$(resolve_herdr_session "$SESSION_NAME" "$WORKSPACE")"
|
||||
export HERDR_SESSION_NAME
|
||||
fi
|
||||
```
|
||||
Explicit value takes priority; otherwise resolves from registry. ✅
|
||||
|
||||
### 2.3 Forwarding to update_yaml_resumed.sh — BOTH call sites
|
||||
|
||||
| Call site | Lines | Forwards `--herdr-session`? |
|
||||
|---|---|---|
|
||||
| Already-running path | 72-74 | ✅ `--herdr-session "$HERDR_SESSION_NAME"` |
|
||||
| Post-spawn path | 136-138 | ✅ `--herdr-session "$HERDR_SESSION_NAME"` |
|
||||
|
||||
**This fixes the N-1 from the prior review (job f03021cf)** where the post-spawn call at line 136 did not forward the flag. Both paths now propagate the resolved session name to the YAML updater. ✅
|
||||
|
||||
---
|
||||
|
||||
## §3. update_yaml_resumed.sh — `HERDR_SERVER_OPT_EXPLICIT` Mechanism
|
||||
|
||||
### 3.1 Explicit-tracking env var (lines 42-49)
|
||||
|
||||
```bash
|
||||
if [ -n "$HERDR_SERVER_OPT" ]; then
|
||||
export HERDR_SESSION_NAME="$HERDR_SERVER_OPT"
|
||||
export HERDR_SERVER_OPT_EXPLICIT="1" # explicit flag passed
|
||||
else
|
||||
HERDR_SESSION_NAME="$(resolve_herdr_session "$SESSION_NAME" "${WORKSPACE:-}")"
|
||||
export HERDR_SESSION_NAME
|
||||
export HERDR_SERVER_OPT_EXPLICIT="0" # resolved, not explicit
|
||||
fi
|
||||
```
|
||||
|
||||
This is a new, clean mechanism that distinguishes "user explicitly passed `--herdr-session`" from "value was resolved from registry/env". ✅
|
||||
|
||||
### 3.2 Propagation to Python heredoc (line 95)
|
||||
|
||||
```bash
|
||||
atomic_dump_yaml "$AGENT_SESSIONS_YAML" \
|
||||
… PANE_PID="$PANE_PID" CHILD_PID="$CHILD_PID" \
|
||||
HERDR_SERVER_OPT_EXPLICIT="${HERDR_SERVER_OPT_EXPLICIT:-0}" <<'PYEOF'
|
||||
```
|
||||
The env var is forwarded to the Python subprocess. ✅
|
||||
|
||||
### 3.3 Else-branch conditional overwrite (lines 130-139)
|
||||
|
||||
For an **existing** target row:
|
||||
|
||||
```python
|
||||
sn = os.environ.get('HERDR_SESSION_NAME')
|
||||
is_explicit = os.environ.get('HERDR_SERVER_OPT_EXPLICIT') == '1'
|
||||
if sn:
|
||||
if is_explicit or not target.get('herdr_session'):
|
||||
target['herdr_session'] = sn
|
||||
target['herdr_server'] = sn
|
||||
target['start_command'] = f'HERDR_SESSION_NAME={sn} herdr agent attach {name}'
|
||||
target['attach_command'] = f'HERDR_SESSION_NAME={sn} herdr agent attach {name}'
|
||||
target['kill_command'] = f'HERDR_SESSION_NAME={sn} herdr kill-session -t {name}'
|
||||
```
|
||||
|
||||
| Scenario | `is_explicit` | `target['herdr_session']` exists | Action |
|
||||
|---|---|---|---|
|
||||
| Explicit `--herdr-session NEW` | 1 | yes (OLD) | **Overwrites** to NEW ✅ |
|
||||
| Explicit `--herdr-session NEW` | 1 | no | Overwrites to NEW ✅ |
|
||||
| Resolved (no flag) | 0 | yes | **Preserves** existing ✅ |
|
||||
| Resolved (no flag) | 0 | no | **Backfills** from resolved sn ✅ |
|
||||
| Resolved, sn absent | 0 | — | Skips (no-op) ✅ |
|
||||
|
||||
This is a significant improvement over the previous `setdefault`-only approach. When explicit, it always overwrites (fixing the orphan-registry edge case). When not explicit, it preserves the existing value and only backfills if missing. ✅
|
||||
|
||||
### 3.4 New-target path (lines 111-129)
|
||||
|
||||
When the target row doesn't exist, a new entry is created with `server_name = os.environ.get('HERDR_SESSION_NAME', default_server)` and all 5 fields populated. ✅
|
||||
|
||||
---
|
||||
|
||||
## §4. stop_session.sh — Consistent Parsing
|
||||
|
||||
### 4.1 Argument parsing (line 77)
|
||||
|
||||
```bash
|
||||
--herdr-session|--herdr-server) HERDR_SERVER_OPT="$2"; shift 2 ;;
|
||||
```
|
||||
Identical pattern to the other 3 scripts. ✅
|
||||
|
||||
### 4.2 Export logic (lines 104-109)
|
||||
|
||||
```bash
|
||||
if [ -n "$HERDR_SERVER_OPT" ]; then
|
||||
export HERDR_SESSION_NAME="$HERDR_SERVER_OPT"
|
||||
else
|
||||
HERDR_SESSION_NAME="$(resolve_herdr_workspace "$SESSION_NAME" "${WORKSPACE:-$WORKSPACE_ROOT}")"
|
||||
export HERDR_SESSION_NAME
|
||||
fi
|
||||
```
|
||||
Consistent with resume's pattern. ✅
|
||||
|
||||
### 4.3 Usage/docs updated
|
||||
|
||||
Both the header comment (line 4, 19) and `usage()` (lines 44-45, 52) document `--herdr-session` with the `--herdr-server` alias. ✅
|
||||
|
||||
---
|
||||
|
||||
## §5. SKILL.md — Documentation
|
||||
|
||||
- **Title**: Renamed "Herdr Server Isolation (격리 서버)" → "Herdr Session Isolation (격리 세션)" ✅
|
||||
- **Primary names**: `HERDR_SESSION_NAME` env var and `--herdr-session` flag documented as standard ✅
|
||||
- **Alias note**: "(opt-in; alias: `--herdr-server`; legacy env alias: `HERDR_SERVER_NAME`)" ✅
|
||||
- **Duplicate "Recommended Alias" block removed**: The previous version had a misplaced/repeated paragraph. This is now cleaned up. ✅
|
||||
- **Wording fix**: "this now maps to" → "this maps to" (removed erroneous "now") ✅
|
||||
- **Migration examples**: Updated to use `HERDR_SESSION_NAME` and `--herdr-session` ✅
|
||||
|
||||
---
|
||||
|
||||
## §6. Backward Compatibility
|
||||
|
||||
| Legacy mechanism | Status | Evidence |
|
||||
|---|---|---|
|
||||
| `--herdr-server` flag | Retained as alias in all 4 scripts | `--herdr-session\|--herdr-server)` parser case |
|
||||
| `HERDR_SERVER_NAME` env var | Retained as fallback in `lib.sh` | `resolve_herdr_workspace`: `os.environ.get('HERDR_SESSION_NAME', '') or os.environ.get('HERDR_SERVER_NAME', '')` (line 1043) |
|
||||
| `reconcile.sh` env fallback | Retained | `elif 'HERDR_SERVER_NAME' in os.environ:` (line 386-387) |
|
||||
| Existing tests using `HERDR_SERVER_NAME` | Still pass | `test_tier1_unit.py`, `test_tier3_integration.py`, `test_workspace_scope.py` — all in the 346 passed |
|
||||
|
||||
Zero functionality loss. A user who has `HERDR_SERVER_NAME` exported or uses `--herdr-server` will see identical behavior. ✅
|
||||
---
|
||||
|
||||
## §7. Test Coverage
|
||||
|
||||
### 7.1 Changeset-specific tests (6 total: 5 new + 1 modified)
|
||||
|
||||
| Test | Feature | Status | Runtime |
|
||||
|---|---|---|---|
|
||||
| `test_comp_create_usage_matches_parser` | Create: usage docs + parser | PASS | 2.09s |
|
||||
| `test_comp_create_herdr_session_cli_parsing_dry_run` | Create: `--herdr-session` + `--herdr-server` dry-run | PASS | 2.34s |
|
||||
| `test_comp_create_herdr_session_default_preserved` | Create: `--herdr-session default` preserved | PASS | (batch 24.20s) |
|
||||
| `test_comp_create_herdr_session_yaml_propagation` | Create: YAML field propagation (5 fields) | PASS | (batch 24.20s) |
|
||||
| `test_comp_resume_herdr_session_propagation` | Resume: NEW overwrites OLD (N-1 fix) | PASS | 5.35s |
|
||||
| `test_comp_stop_usage_matches_parser` (modified) | Stop: `--herdr-session` parser acceptance | PASS | 1.16s |
|
||||
|
||||
### 7.2 Coverage assessment
|
||||
|
||||
- **CLI parsing**: Both `--herdr-session` and `--herdr-server` tested in dry-run mode ✅
|
||||
- **Usage/parser matching**: Create + stop both verify usage() advertises flags that the parser accepts ✅
|
||||
- **YAML propagation**: `herdr_session`, `herdr_server`, `start_command`, `attach_command`, `kill_command` all asserted ✅
|
||||
- **Default preservation**: `--herdr-session default` edge case covered ✅
|
||||
- **Resume overwrite**: Explicit `--herdr-session NEW` overwriting `OLD` in existing row — directly tests the N-1 fix ✅
|
||||
|
||||
### 7.3 Full suite
|
||||
|
||||
A parallel full-suite run (by the claude reviewer) completed: **346 passed, 0 failed** (462.39s). This includes all changeset-specific tests plus tier1/tier3/tier4/integration/e2e suites. ✅
|
||||
|
||||
---
|
||||
|
||||
## §8. Non-blocking Observations
|
||||
|
||||
### N-1 (FIXED — no longer an issue)
|
||||
|
||||
The previous review (job f03021cf) noted that `resume_session.sh` line 136-137 (post-spawn `update_yaml_resumed.sh` call) did not forward `--herdr-session`. **This is now fixed**: both call sites (already-running at line 72 and post-spawn at line 136) forward `--herdr-session "$HERDR_SESSION_NAME"`. Additionally, `update_yaml_resumed.sh` now uses the `HERDR_SERVER_OPT_EXPLICIT` mechanism to force-overwrite the existing row's `herdr_session`/`herdr_server`/commands when the flag is explicit. The new test `test_comp_resume_herdr_session_propagation` directly verifies this. ✅
|
||||
|
||||
### N-2 (pre-existing, out of scope)
|
||||
|
||||
`deploy/install_mam.sh` (line 330), `deploy/install.sh` (line 524), and potentially `README.ko.md` still use `HERDR_SERVER_NAME` as the primary env var name in user-facing instructions. These are pre-existing references not introduced by this changeset and are out of scope. The legacy alias still works via `lib.sh`'s fallback, so there is no functional impact — only documentation consistency.
|
||||
|
||||
### N-3 (environmental, not a changeset defect)
|
||||
|
||||
The environment has dozens of orphaned `reconcile.sh --subscribe --idle-timeout 0` daemon processes from prior test runs, plus a live herdr server. This slowed independent test execution but did not affect results — the parallel full suite (346 passed) and all individually-run changeset tests confirm correctness.
|
||||
|
||||
---
|
||||
|
||||
## §9. Conclusion
|
||||
|
||||
This changeset is a clean, well-tested standardization of `--herdr-session` across the multi-agent-mux skill scripts. Key strengths:
|
||||
|
||||
1. **Correctness**: All 3 guard sites in `create_session.sh` properly protect explicit values from being clobbered.
|
||||
2. **Consistency**: All 4 scripts use the same `--herdr-session|--herdr-server) HERDR_SERVER_OPT="$2"` parsing pattern and the same `if [ -n "$HERDR_SERVER_OPT" ]` export logic.
|
||||
3. **N-1 fix**: The previous review's blocking observation (resume post-spawn not forwarding `--herdr-session`) is fully addressed — both call sites now forward, and `update_yaml_resumed.sh` uses `HERDR_SERVER_OPT_EXPLICIT` to force-overwrite when explicit.
|
||||
4. **Backward compatibility**: `--herdr-server` flag and `HERDR_SERVER_NAME` env var are retained as aliases/fallbacks with zero functionality loss.
|
||||
5. **Test coverage**: 6 changeset-specific tests (5 new + 1 modified) cover CLI parsing, usage/parser matching, default preservation, YAML field propagation, and resume overwrite. Full suite: 346 passed, 0 failed.
|
||||
|
||||
No blocking issues found. No design-level rework needed.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user