93 lines
6.0 KiB
Markdown
93 lines
6.0 KiB
Markdown
# 🔍 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]
|