266 lines
12 KiB
Markdown
266 lines
12 KiB
Markdown
# Cross-Code Review: Job 688f07f2
|
||
|
||
## Scope
|
||
|
||
Re-review of uncommitted working-tree changes (diff baseline: `HEAD` = `2bd59fc`, blob `1009167`) in the `multi-agent-mux` repository. This review verifies whether the critical and minor findings from the prior review (job `8585135b`, verdict NOT PASS) have been addressed.
|
||
|
||
The changeset covers:
|
||
|
||
1. **`.agents/skills/lib.sh`** (161 lines changed) — major refactor of `new-session` codepath + `resolve_herdr_session` fix
|
||
2. **`tests/conftest.py`** (133 lines changed) — mock herdr enhancements + F-1 fix
|
||
3. **`tests/test_herdr_shim_contract.py`** (126 lines, new file) — contract tests H-1 through H-14
|
||
4. **`tests/fixtures/herdr_contract.json`** (35 lines, new file) — herdr 0.7.4 API contract fixture
|
||
|
||
---
|
||
|
||
## 0. Prior Review Findings — Fix Verification
|
||
|
||
The prior review (job `8585135b`) identified four findings. Their status in the current changeset:
|
||
|
||
| ID | Severity | Description | Prior Status | Current Status |
|
||
|----|----------|-------------|--------------|----------------|
|
||
| F-1 | **Critical** | `mam_sandbox` fixture doesn't clear `HERDR_SESSION_NAME` → new tests fail in herdr sessions | NOT PASS | ✅ **FIXED** |
|
||
| F-2 | Minor | `sleep` on last backoff iteration (2s unnecessary delay) | NOT PASS | ✅ **FIXED** |
|
||
| F-3 | Low | H-11~H-13 don't verify split direction (mock always returns wide dims) | Open | ⚠️ Still open (non-blocking) |
|
||
| F-4 | Low | H-9/H-10 are placeholder tests with trivial assertions | Open | ⚠️ Still open (non-blocking) |
|
||
|
||
### F-1 Fix Verification
|
||
|
||
**conftest.py lines 39-40:**
|
||
```python
|
||
monkeypatch.delenv("HERDR_SESSION_NAME", raising=False)
|
||
monkeypatch.delenv("HERDR_SERVER_NAME", raising=False)
|
||
```
|
||
|
||
Added to `mam_sandbox` fixture. This prevents the shim from prepending `--session <name>` to all herdr calls, which previously caused `c[0] == "--session"` instead of `c[0] == "agent"` in the test filter.
|
||
|
||
**Verification:** Ran `test_herdr_shim_contract.py` inside a herdr session (where `HERDR_SESSION_NAME` is set in the environment):
|
||
- All 5 tests PASS in 1.63s (previously failed with `assert 0 > 0` and ~10s delays)
|
||
|
||
### F-2 Fix Verification
|
||
|
||
**lib.sh lines 380-382:**
|
||
```bash
|
||
if [ "$i" -lt 2 ]; then
|
||
sleep "${backoffs[$i]}"
|
||
fi
|
||
```
|
||
|
||
The `sleep` is now guarded by `if [ "$i" -lt 2 ]`, so the 2-second sleep on the last iteration (i=2) is skipped. The loop exits immediately after the final attempt fails.
|
||
|
||
---
|
||
|
||
## 1. lib.sh — Production Code Review
|
||
|
||
### 1A. Stale Temp File Cleanup (line 113)
|
||
|
||
```bash
|
||
rm -f "$wrapper_dir"/herdr.?????? 2>/dev/null || true
|
||
```
|
||
|
||
Cleans up stale `herdr.XXXXXX` temp files from previous runs. The glob `herdr.??????` matches exactly 6-char suffixes produced by `mktemp "$wrapper_dir/herdr.XXXXXX"`. Safe with `2>/dev/null || true`.
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
### 1B. Major Refactor of `new-session` Codepath (lines 265–389)
|
||
|
||
#### Removed Components
|
||
| Component | Analysis |
|
||
|-----------|----------|
|
||
| `kind` detection block (cline/agy/claude/hermes from name/cmd) | ✅ Safe removal — `kind` was only used for `--kind` flag and strip. Both are gone. |
|
||
| Strip duplicate binary path (Go `flag.Parse` fix from b0c2c08) | ✅ Safe removal — the strip was a workaround for `--kind` + binary name duplication. Removing `--kind` eliminates the root cause. |
|
||
| `workspace list` query for CWD matching | ✅ Correct replacement — `WorkspaceInfo` has no `cwd` key (confirmed by `herdr_contract.json`). `PaneInfo` has `cwd`. W1 correctly switches to `pane list`. |
|
||
| `--kind` dual-syntax fallback (try `--kind`, then explicit binary) | ✅ Correct removal — single native syntax `agent start -- <argv>` is the herdr 0.7.4 contract. |
|
||
|
||
#### Added Components
|
||
|
||
**W1 — Pane list CWD matching (lines 268–285):**
|
||
Queries `_real_herdr pane list` and matches pane CWD via `os.path.realpath()` on both sides. Correct — `PaneInfo` has both `cwd` and `workspace_id` properties. Uses `2>/dev/null || echo ""` fallback for error resilience.
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
**W2a — Split direction policy (lines 290–335):**
|
||
Three-step query:
|
||
1. `pane list` → find sample pane in existing workspace
|
||
2. `pane layout --pane <pane_id>` → get focused pane rect
|
||
3. Python logic: `width // 2 >= min_cols` → `right`; `height // 2 >= min_rows` → `down`; else `overflow`
|
||
|
||
Falls back to `--split right` when layout query returns empty (e.g., pane layout API unavailable).
|
||
|
||
**Verdict: ✅ Correct.** Sound layout-aware split policy.
|
||
|
||
**W2b — Overflow threshold (lines 340–342):**
|
||
When `split_dir = "overflow"`, sets `existing_ws=""` to force fresh workspace creation. Prevents unusably tiny panes. Configurable via `MAM_MIN_PANE_COLS` (default 60) and `MAM_MIN_PANE_ROWS` (default 20).
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
**Workspace create with fallback (lines 349–361):**
|
||
- `|| echo ""` fallback prevents `set -e` exit on failure
|
||
- Dual extraction path: `result.workspace.workspace_id` → `result.workspace_id` — more robust
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
**W5/W6 — Backoff retries (lines 363–388):**
|
||
Three retries with 0.5/1/2s backoff. Immediate abort on usage/unknown-flag errors (no point retrying a syntax error). The F-2 fix (sleep guard) is present. Error reporting: `echo "$res" >&2; exit 1` on final failure.
|
||
|
||
**Verdict: ✅ Correct.** F-2 is fixed.
|
||
|
||
### 1C. `resolve_herdr_session` Fix (lines 838–842)
|
||
|
||
```python
|
||
val = s.get('herdr_session') or s.get('herdr_server') or s.get('herdr_workspace')
|
||
if val and val != 'default':
|
||
print(val)
|
||
sys.exit(0)
|
||
```
|
||
|
||
Previously, `val` was printed unconditionally — if `val` was `'default'` (the fallback sentinel), it would be printed and `sys.exit(0)` called, preventing the proper fallback logic below from executing. Now, `'default'` falls through to the workspace-based slug derivation.
|
||
|
||
**Verdict: ✅ Correct.** Prevents `default` from being returned as a real session name.
|
||
|
||
### 1D. Shim File Sync Verification
|
||
|
||
The auto-generated shim file (`.mam/shim/herdr`, 629 lines) contains the same `new-session` codepath as the lib.sh heredoc — including W1, W2a, W2b, W5/W6, and the F-2 sleep guard. The shim is generated from the heredoc at runtime, so it is always in sync.
|
||
|
||
**Verdict: ✅ Shim in sync.**
|
||
|
||
### 1E. Syntax Validation
|
||
|
||
- `bash -n .agents/skills/lib.sh` → **SYNTAX OK**
|
||
|
||
---
|
||
|
||
## 2. conftest.py — Mock Infrastructure Review
|
||
|
||
### 2A. F-1 Fix: Environment Variable Cleanup (lines 39-40)
|
||
|
||
```python
|
||
monkeypatch.delenv("HERDR_SESSION_NAME", raising=False)
|
||
monkeypatch.delenv("HERDR_SERVER_NAME", raising=False)
|
||
```
|
||
|
||
**Verdict: ✅ Critical fix applied.** Both `HERDR_SESSION_NAME` and `HERDR_SERVER_NAME` are cleared, preventing the shim from prepending `--session` to all herdr calls during tests.
|
||
|
||
### 2B. Lock Invariant Documentation (lines 112-117)
|
||
|
||
Documents the critical invariant that the global `fcntl.flock` lock must remain in scope for the entire process lifetime to guarantee consistency.
|
||
|
||
**Verdict: ✅ Correct documentation.**
|
||
|
||
### 2C. Workspace List Response (W10, lines 163-167)
|
||
|
||
Returns `WorkspaceInfo` without `cwd` key, matching the herdr 0.7.4 contract. Previously returned the full workspace dict including `cwd`.
|
||
|
||
**Verdict: ✅ Correct.** Matches `herdr_contract.json` `WorkspaceInfo` properties.
|
||
|
||
### 2D. Workspace Create Response (lines 183-196)
|
||
|
||
Returns full `workspace_created` object with `workspace`, `tab`, and `root_pane`. Also creates a root pane entry in `state["panes"]`.
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
### 2E. Pane Handlers (lines 197-256)
|
||
|
||
- **`pane list`**: Builds panes from agents or `state["panes"]`, supports `--workspace` filter. ✅
|
||
- **`pane split`**: Returns `pane_split` response. ✅
|
||
- **`pane layout`**: Returns `area`, `focused_pane_id`, `panes` with `rect` (width/height). ✅
|
||
|
||
**Verdict: ✅ Correct mock implementation.**
|
||
|
||
### 2F. Flag Whitelist (W8, lines 299-329)
|
||
|
||
```python
|
||
whitelist = {"--cwd", "--workspace", "--tab", "--split", "--env", "--focus", "--no-focus"}
|
||
```
|
||
|
||
Unknown flags trigger a usage error message. **Verdict: ✅ Correct.** Enforces herdr 0.7.4 contract.
|
||
|
||
### 2G. Agent Started Response (lines 448-458)
|
||
|
||
Returns full `agent_started` object with `agent` and `argv`. `TMP_PATH_PLACEHOLDER` is replaced with `str(tmp_path)` at mock generation time.
|
||
|
||
**Verdict: ✅ Correct.**
|
||
|
||
### 2H. Python Syntax Validation
|
||
|
||
- `python3 -m py_compile tests/conftest.py` → **CONFTEST OK**
|
||
|
||
---
|
||
|
||
## 3. Test File Review (test_herdr_shim_contract.py)
|
||
|
||
### 3A. Test Results
|
||
|
||
All 5 tests PASS in 1.63s:
|
||
|
||
| Test | Status |
|
||
|------|--------|
|
||
| `test_h1_to_h8_shim_contract` | ✅ PASS |
|
||
| `test_h9_mock_response_contract_schema` | ✅ PASS |
|
||
| `test_h10_real_herdr_schema_match` | ✅ PASS (skipped — no real herdr binary) |
|
||
| `test_h11_to_h13_layout_policy` | ✅ PASS |
|
||
| `test_h14_mock_concurrency_lock_invariant` | ✅ PASS |
|
||
|
||
### 3B. Test Quality Observations (non-blocking)
|
||
|
||
| Test | Observation | Severity |
|
||
|------|-------------|----------|
|
||
| H-1 to H-8 | ✅ Good coverage of shim contract (flags, path, retries, env, error) | — |
|
||
| H-9 | ⚠️ Trivial — only checks fixture file exists and has expected top-level keys | Low (F-4) |
|
||
| H-10 | ⚠️ Placeholder — assertion uses `or True` (always passes); effectively a skip | Low (F-4) |
|
||
| H-11 to H-13 | ⚠️ Only checks `returncode == 0` — doesn't verify split direction. Mock always returns `width=184` so `184//2=92 >= 60` → always "right". Never exercises `down` or `overflow` paths. | Low (F-3) |
|
||
| H-14 | ✅ Verifies all 10 agents created under concurrent access. Lock prevents data loss. | — |
|
||
|
||
### 3C. Python Syntax Validation
|
||
|
||
- `python3 -m py_compile tests/test_herdr_shim_contract.py` → **TESTFILE OK**
|
||
|
||
---
|
||
|
||
## 4. Fixture Review (herdr_contract.json)
|
||
|
||
- `WorkspaceInfo` has no `cwd` key → confirms W1 switch to `pane list` is correct
|
||
- `PaneInfo` has `cwd` and `workspace_id` → confirms pane-based CWD matching works
|
||
- `AgentStartFlags` matches the whitelist in both lib.sh and conftest.py mock
|
||
|
||
**Verdict: ✅ Correct.** Matches herdr 0.7.4 contract.
|
||
|
||
---
|
||
|
||
## 5. Regression Check
|
||
|
||
Ran key test files to verify no regressions from the changes:
|
||
|
||
| Test File | Result | Time |
|
||
|-----------|--------|------|
|
||
| `test_herdr_shim_contract.py` | 5/5 PASS | 1.63s |
|
||
| `test_tier1_unit.py` | 29/29 PASS | 6.49s |
|
||
| `test_o2_race_free_lock.py` | 22/22 PASS | 11.22s |
|
||
|
||
The full 249-test suite was started but did not complete within the 30s tool timeout (it gets stuck on `test_deploy_layout.py::test_td6_td7_td8_mam_deploy_layout_and_removal`, a known slow integration test unrelated to this changeset). The three test files above — which are the most relevant to the changes — all pass without regressions.
|
||
|
||
---
|
||
|
||
## 6. Summary
|
||
|
||
### Production Code (lib.sh)
|
||
**✅ Correct and well-designed.** The major refactor eliminates the Go `flag.Parse` duplicate path issue at its root (by removing `--kind` entirely), implements layout-aware split direction (W2a/W2b), adds retry with backoff (W5/W6), correctly switches from `workspace list` to `pane list` for CWD matching (W1), and fixes `resolve_herdr_session` to not return `'default'` as a real session name. All variables initialized, `set -u` safe, `bash -n` passes. The `env_flags`/`final_cmd` initialization block (the b0c2c08 regression site) is intact.
|
||
|
||
### Mock Infrastructure (conftest.py)
|
||
**✅ Correct.** Enhanced to support new pane API, workspace create response, flag whitelist, and lock invariant. F-1 critical bug is fixed — `HERDR_SESSION_NAME` and `HERDR_SERVER_NAME` are now cleared in `mam_sandbox`.
|
||
|
||
### New Test File (test_herdr_shim_contract.py)
|
||
**✅ All tests pass.** F-1 fix resolved the critical test failure. The remaining low-severity test quality observations (F-3, F-4) are non-blocking — they don't affect correctness or pass/fail status.
|
||
|
||
### Findings Summary
|
||
|
||
| ID | Severity | Description | Status |
|
||
|----|----------|-------------|--------|
|
||
| F-1 | **Critical** | `mam_sandbox` doesn't clear `HERDR_SESSION_NAME` → tests fail in herdr sessions | ✅ **FIXED** |
|
||
| F-2 | Minor | `sleep` on last backoff iteration (2s unnecessary delay) | ✅ **FIXED** |
|
||
| F-3 | Low | H-11~H-13 don't verify split direction (mock always returns wide dims) | ⚠️ Open (non-blocking) |
|
||
| F-4 | Low | H-9/H-10 are placeholder tests with trivial assertions | ⚠️ Open (non-blocking) |
|
||
|
||
The two actionable findings (F-1 critical, F-2 minor) from the prior review have been fixed. The remaining findings (F-3, F-4) are low-severity test quality observations that do not affect production correctness or test pass/fail outcomes.
|
||
|
||
[VERDICT: PASS] |