From c4839822ed5892ec394bb7d53e1962d8d72c9265 Mon Sep 17 00:00:00 2001 From: Godopu Date: Sat, 25 Jul 2026 00:03:56 +0900 Subject: [PATCH] fix(resume): add --workspace symmetry in resume_session.sh and update test_tier4_e2e for isolation model --- .../report-a86b2edc.md | 49 ++++++++++ .../report-0e947af1.md | 92 +++++++++++++++++++ .../scripts/resume_session.sh | 2 +- tests/test_tier4_e2e.py | 7 +- 4 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 .agents/reports/canary-projects-multi-agent-mux-reviewer-agy/report-a86b2edc.md create mode 100644 .agents/reports/canary-projects-multi-agent-mux-reviewer-claude/report-0e947af1.md diff --git a/.agents/reports/canary-projects-multi-agent-mux-reviewer-agy/report-a86b2edc.md b/.agents/reports/canary-projects-multi-agent-mux-reviewer-agy/report-a86b2edc.md new file mode 100644 index 0000000..d976e83 --- /dev/null +++ b/.agents/reports/canary-projects-multi-agent-mux-reviewer-agy/report-a86b2edc.md @@ -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] diff --git a/.agents/reports/canary-projects-multi-agent-mux-reviewer-claude/report-0e947af1.md b/.agents/reports/canary-projects-multi-agent-mux-reviewer-claude/report-0e947af1.md new file mode 100644 index 0000000..08cffac --- /dev/null +++ b/.agents/reports/canary-projects-multi-agent-mux-reviewer-claude/report-0e947af1.md @@ -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] diff --git a/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh b/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh index a35a542..ba87758 100755 --- a/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh +++ b/.agents/skills/multi-agent-mux-resume/scripts/resume_session.sh @@ -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 diff --git a/tests/test_tier4_e2e.py b/tests/test_tier4_e2e.py index ae34af9..f882e02 100644 --- a/tests/test_tier4_e2e.py +++ b/tests/test_tier4_e2e.py @@ -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', []):