# 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]