fix(resume): add --workspace symmetry in resume_session.sh and update test_tier4_e2e for isolation model
This commit is contained in:
@@ -0,0 +1,49 @@
|
||||
# 🔍 Code Review Report: Commit d2a8247 & 57bc1b2
|
||||
|
||||
- **Job ID**: `a86b2edc`
|
||||
- **Reviewer**: agy (`herdr:canary-projects-multi-agent-mux-reviewer-agy`)
|
||||
- **Target Commits**:
|
||||
- `d2a82478e936c2bf1de08f923a4c4563cb4d90ce`: `fix(resume): pass workspace, role, and epoch to update_yaml_resumed.sh to fix fallback silent bugs`
|
||||
- `57bc1b297b83d987d6050b10be4c3faef7d1f56b`: `fix(resume): declare default AGENT variable in update_yaml_resumed.sh to avoid unbound variable error`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
This cross-code review evaluated the latest fix commit (`d2a8247`) and follow-up fix (`57bc1b2`) in the `multi-agent-mux-resume` skill.
|
||||
- **Commit `d2a8247`**: Resolved silent fallback bugs in `update_yaml_resumed.sh` by properly passing `--workspace`, `--role`, and epoch timestamp when auto-creating missing session entries during resume.
|
||||
- **Commit `57bc1b2`**: Fixed an `unbound variable` bash error under `set -u` by declaring `AGENT=""` at script initialization.
|
||||
|
||||
All changes adhere to project safety guidelines, pass schema validation, and maintain complete protocol alignment.
|
||||
|
||||
---
|
||||
|
||||
## 2. Review Findings & Technical Analysis
|
||||
|
||||
### 2.1 Commit `d2a8247`: Workspace, Role, and Epoch Passing
|
||||
- **Files Modified**:
|
||||
- `.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh`
|
||||
- `.agents/skills/multi-agent-mux-resume/scripts/update_yaml_resumed.sh`
|
||||
- **Analysis**:
|
||||
- **CLI Argument Expansion**: Added `--workspace` and `--role` parsing to `update_yaml_resumed.sh`, and updated `resume_session.sh` to forward `--workspace "$WORKSPACE"`.
|
||||
- **Pattern-Based Role & Agent Inference**: Accurately infers `ROLE` (`planner`, `reviewer`, `creator`) and `AGENT` (`claude`, `agy`, `hermes`, `cline`) from session name patterns (e.g., `*-planner-*`, `*-reviewer-*`) when omitted.
|
||||
- **Timestamp Accuracy**: Calculates `NOW_EPOCH=$(date +%s)` so `herdr_session_epoch` reflects the actual resume epoch timestamp instead of defaulting to `0`.
|
||||
- **Atomic Injection**: Safely forwards `NOW_EPOCH`, `TARGET_WORKSPACE`, and `ROLE` through environment variables into `atomic_dump_yaml`, preventing string-interpolation shell vulnerabilities.
|
||||
|
||||
### 2.2 Commit `57bc1b2`: Unbound Variable Initialization
|
||||
- **Files Modified**:
|
||||
- `.agents/skills/multi-agent-mux-resume/scripts/update_yaml_resumed.sh`
|
||||
- **Analysis**:
|
||||
- Added `AGENT=""` declaration alongside `SESSION_NAME` and `UUID` initialization.
|
||||
- Fixes `bash: AGENT: unbound variable` crash under `set -euo pipefail` when `update_yaml_resumed.sh` is called without `--agent`.
|
||||
|
||||
---
|
||||
|
||||
## 3. Protocol Alignment & Verification
|
||||
|
||||
- **Schema Validation**: Auto-created target entries strictly adhere to `atomic_dump_yaml`'s `_validate()` constraints (`name`, `status='running'`, `role`, `pane`, `herdr_server`, `start_command`, `attach_command`, `kill_command`).
|
||||
- **Git Repository Status**: All commits are pushed and up to date with `origin/main`.
|
||||
|
||||
---
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,92 @@
|
||||
# 🔍 Code Review Report — Job 0e947af1
|
||||
|
||||
- **Job ID**: `0e947af1`
|
||||
- **Reviewer**: claude (`herdr:canary-projects-multi-agent-mux-reviewer-claude`)
|
||||
- **Target Commits**:
|
||||
- `d2a8247`: `fix(resume): pass workspace, role, and epoch to update_yaml_resumed.sh to fix fallback silent bugs`
|
||||
- `57bc1b2`: `fix(resume): declare default AGENT variable in update_yaml_resumed.sh to avoid unbound variable error` (latest)
|
||||
- **Files**: `.agents/skills/multi-agent-mux-resume/scripts/update_yaml_resumed.sh`, `.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
`d2a8247` extended `update_yaml_resumed.sh` to accept `--workspace`/`--role` and compute `epoch`, so that auto-created session rows (added in `cb88771`) get correct `cwd`/`role` instead of silent placeholder fallbacks. While rewriting the variable block, it accidentally **dropped the pre-declaration of `AGENT=""`**, which — under `set -euo pipefail` — breaks the script whenever it's invoked without an explicit `--agent` flag (the documented fallback-inference path). `57bc1b2` is a one-line fix that restores the declaration.
|
||||
|
||||
I reproduced the regression directly (see §2) and confirmed `57bc1b2` resolves it without side effects. I also found one **unresolved, narrower gap** left over from `d2a8247` itself (§3), and one **pre-existing, unrelated** test failure (§4). Neither blocks this fix.
|
||||
|
||||
**Verdict: the change under review (57bc1b2) is correct, minimal, and verified safe.**
|
||||
|
||||
---
|
||||
|
||||
## 2. Verified Regression + Fix
|
||||
|
||||
**Reproduction — `d2a8247` (before `57bc1b2`)**, invoking the script the way `SKILL.md`'s documented manual-recovery example and `tests/test_tier2_component.py::test_comp_resume_update_yaml` do — i.e. *without* `--agent`, relying on suffix inference:
|
||||
|
||||
```
|
||||
$ bash update_yaml_resumed.sh --session test-resumed-session-creator-claude --uuid new-uuid-999
|
||||
update_yaml_resumed.sh: line 43: AGENT: unbound variable
|
||||
exit code: 1
|
||||
```
|
||||
|
||||
This is because `d2a8247`'s diff replaced the `AGENT=""` line with `WORKSPACE=""`/`ROLE=""` instead of adding to it:
|
||||
|
||||
```diff
|
||||
SESSION_NAME=""
|
||||
UUID=""
|
||||
-AGENT=""
|
||||
+WORKSPACE=""
|
||||
+ROLE=""
|
||||
```
|
||||
|
||||
`AGENT` is only ever *assigned* inside the arg-parsing loop if `--agent` is passed; the fallback block (`if [ -z "$AGENT" ]; then ...`) reads it unconditionally, so any invocation without `--agent` reads an undeclared variable and — because of `set -u` — crashes before the fallback logic even runs.
|
||||
|
||||
**At current HEAD (with `57bc1b2` applied)**, the same invocation:
|
||||
|
||||
```
|
||||
$ bash update_yaml_resumed.sh --session test-resumed-session-creator-claude --uuid new-uuid-999
|
||||
updated: test-resumed-session-creator-claude status=running (resume id -> per-row own id)
|
||||
exit code: 0
|
||||
```
|
||||
|
||||
`57bc1b2`'s fix is exactly the declaration restore, nothing else — a correct, minimal, low-risk patch.
|
||||
|
||||
**Test suite**: `tests/ -k "resume or resumed"` → 14 passed, 1 failed (unrelated, see §4). `test_comp_resume_update_yaml` — the test that exercises this exact no-`--agent` path — passes at HEAD.
|
||||
|
||||
`bash -n` syntax check: both `update_yaml_resumed.sh` and `resume_session.sh` OK.
|
||||
|
||||
---
|
||||
|
||||
## 3. Residual Gap in `d2a8247` (not addressed by `57bc1b2`, non-blocking)
|
||||
|
||||
`d2a8247`'s commit message claims it "passes workspace, role, and epoch ... to fix fallback silent bugs," but the wiring is incomplete in `resume_session.sh`:
|
||||
|
||||
- The **"herdr already running"** branch (`resume_session.sh:58-59`) still calls `update_yaml_resumed.sh` with only `--session/--uuid/--agent` — **no `--workspace`, no `--role`**.
|
||||
- Only the **"newly spawned"** branch (`resume_session.sh:118-119`) passes `--workspace`.
|
||||
- `--role` is never passed by *any* caller in the repo (confirmed via grep across `.agents/`, `deploy/`, `tests/`) — it always relies on `update_yaml_resumed.sh`'s suffix-based inference (`*-planner-*`/`*-reviewer-*`/else `creator`).
|
||||
|
||||
**Practical impact**: low but real. The role-inference fallback is safe in practice because session names consistently follow the `-creator-/-planner-/-reviewer-` convention. The missing `--workspace` at line 58 only matters in the narrow case where `update_yaml_resumed.sh` has to *auto-create* a session row (the `cb88771` feature) for an already-running herdr session that has no existing YAML entry (e.g. an orphaned/manually-attached session) — in that case `cwd` falls back to `WORKSPACE_ROOT` (the mux repo root) rather than the actual workspace the caller passed to `resume_session.sh --workspace`. That's precisely the class of "fallback silent bug" `d2a8247` set out to fix, just not closed on this call site.
|
||||
|
||||
**Recommendation** (fast-follow, not a blocker): add `--workspace "$WORKSPACE"` to the call at `resume_session.sh:58-59` for symmetry with line 118-119. Not a design/redesign issue — a one-line change, so no escalation warranted.
|
||||
|
||||
---
|
||||
|
||||
## 4. Pre-existing, Unrelated Test Failure (noted for awareness)
|
||||
|
||||
`tests/test_tier4_e2e.py::test_e2e_scenario2_disconnect_resume` fails with `KeyError: 'isolation'` (`orig_session["isolation"]["root"]`). This predates the commits under review: `f0a2103` (`refactor(isolation): simplify agent session isolation...`, 2 commits before `d2a8247`) removed the `isolation` field from session rows, and the subsequent test-alignment pass (`9ba45e5`) updated `test_tier1_unit.py`/`test_tier2_component.py`/`conftest.py` but missed this e2e test. Out of scope for this review; flagging so it isn't mistaken for a regression from `d2a8247`/`57bc1b2`.
|
||||
|
||||
---
|
||||
|
||||
## 5. Conclusion
|
||||
|
||||
| Item | Status |
|
||||
|---|---|
|
||||
| `57bc1b2` fixes the `AGENT` unbound-variable regression | ✅ Verified via direct reproduction |
|
||||
| No new regressions introduced by `57bc1b2` | ✅ Confirmed (diff is a single added line) |
|
||||
| `bash -n` syntax | ✅ Pass on both scripts |
|
||||
| Resume-related test suite | ✅ 14/15 pass (1 pre-existing unrelated failure, §4) |
|
||||
| `d2a8247`'s stated goal (workspace/role always correctly propagated) | ⚠️ Partially incomplete (§3) — narrow edge case, simple 1-line fix, not blocking |
|
||||
|
||||
No design-level rework is required; the outstanding item (§3) is a straightforward follow-up patch.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -56,7 +56,7 @@ if herdr has-session -t "$SESSION_NAME" 2>/dev/null; then
|
||||
echo "herdr '$SESSION_NAME' already running."
|
||||
# Just update YAML to make sure it's set to running
|
||||
bash "$(dirname "${BASH_SOURCE[0]}")/update_yaml_resumed.sh" \
|
||||
--session "$SESSION_NAME" --uuid "$UUID" --agent "$AGENT"
|
||||
--session "$SESSION_NAME" --uuid "$UUID" --agent "$AGENT" --workspace "$WORKSPACE"
|
||||
exit 0
|
||||
fi
|
||||
|
||||
|
||||
@@ -103,11 +103,10 @@ def test_e2e_scenario2_disconnect_resume(mam_sandbox, mock_herdr, mock_agents):
|
||||
# Simulate first message creating own ID (materialized own ID)
|
||||
own_uuid = "e2e-own-uuid-111"
|
||||
|
||||
iso_root = Path(orig_session["isolation"]["root"])
|
||||
key = str(tmp_path).replace('/', '-').replace('_', '-')
|
||||
proj_dir = iso_root / "projects" / key
|
||||
proj_dir.mkdir(parents=True, exist_ok=True)
|
||||
(proj_dir / f"{own_uuid}.jsonl").write_text(json.dumps({"sessionId": own_uuid}) + "\n")
|
||||
claude_dir = Path(os.environ.get("CLAUDE_PROJECT_DIR", Path.home() / ".claude" / "projects")) / key
|
||||
claude_dir.mkdir(parents=True, exist_ok=True)
|
||||
(claude_dir / f"{own_uuid}.jsonl").write_text(json.dumps({"sessionId": own_uuid}) + "\n")
|
||||
|
||||
mutation = f"""
|
||||
for s in d.get('herdr_sessions', []):
|
||||
|
||||
Reference in New Issue
Block a user