91 lines
5.8 KiB
Markdown
91 lines
5.8 KiB
Markdown
# Cross-Code Review Report — Job 83181aad
|
|
|
|
- **Job ID**: 83181aad
|
|
- **Target**: `fix(lib): resolve workspace CWD mismatch in herdr new-session shim and enforce target workspace slug priority` (commit `20e2e9b`)
|
|
- **Reviewer**: cline
|
|
- **Output Report Path**: `.mam/jobs/83181aad/cline-reports/report-final.md`
|
|
|
|
---
|
|
|
|
## 1. Delta Description
|
|
|
|
Working tree is clean; the fix is committed as `20e2e9b` on top of `da92624` (INSTALL.md `--remove` docs, already reviewed in job 49465a26 — PASS). The changeset touches only `.agents/skills/lib.sh` (47 lines changed, 25 insertions / 22 deletions). Two distinct fixes:
|
|
|
|
### Fix A — Workspace CWD mismatch in herdr new-session shim (`lib.sh:268-284`)
|
|
|
|
**Before**: The `_real_herdr workspace list` JSON output was parsed and the *first* workspace's `workspace_id` was selected unconditionally (`if wss: print(wss[0].get('workspace_id', ''))`).
|
|
|
|
**After**: The target workspace CWD is passed via `TARGET_CWD="${ws:-.}"` and matched against each workspace's `cwd` using `os.path.realpath()` on both sides. If no CWD match is found, `matched_id` stays empty → a new workspace is created (`--cwd "${ws:-.}"`, line 291), identical to the old behavior when `wss` was empty.
|
|
|
|
**Effect**: When a herdr session contains multiple workspaces, the shim now reuses the workspace whose CWD matches the target directory instead of blindly grabbing `wss[0]`. `os.path.realpath` normalizes symlinks and relative paths on both sides.
|
|
|
|
### Fix B — resolve_herdr_session target workspace slug priority (`lib.sh:743-773`)
|
|
|
|
**Before** (fallback order): `HERDR_SESSION_NAME` env → `HERDR_SERVER_NAME` env → (if `ws`) derived `mam-{slug}` → `default`+WARN.
|
|
|
|
**After** (fallback order): (if `ws`) derived `mam-{slug}` → `HERDR_SESSION_NAME`/`HERDR_SERVER_NAME` env → `default`+WARN.
|
|
|
|
**Effect**: When a workspace (`$2`) is provided, the workspace-derived slug now takes **priority** over the env vars. The env-var fallback is preserved for callers that don't pass a workspace. The slug derivation logic itself is unchanged (just reprioritized).
|
|
---
|
|
|
|
## 2. Lint
|
|
|
|
| Check | Result |
|
|
|-------|--------|
|
|
| `bash -n .agents/skills/lib.sh` | ✅ PASS |
|
|
| Embedded Python (CWD-match block, lines 269-283) `compile()` | ✅ PASS |
|
|
| Embedded Python (resolve_herdr_session block, lines 747-771) `compile()` | ✅ PASS |
|
|
| shellcheck | Covered by CI (`deploy/gitea-ci.yml:31`); not installed locally |
|
|
|
|
---
|
|
|
|
## 3. Behavior (동작성)
|
|
|
|
### Fix A — CWD-match correctness
|
|
- Multiple workspaces → selects the one whose `realpath(cwd) == realpath(target_ws)`. ✅
|
|
- `ws` empty → `TARGET_CWD` defaults to `.` → matches current directory. Reasonable. ✅
|
|
- No match → `matched_id=''` → falls through to `workspace create` (line 291). Safe fallback. ✅
|
|
- `os.path.realpath` on both sides handles symlinks, trailing slashes, relative paths. ✅
|
|
|
|
### Fix B — slug priority correctness
|
|
- `ws` truthy → derive `mam-{slug}` directly; env vars never consulted; WARN never fires. ✅
|
|
- `ws` falsy → `HERDR_SESSION_NAME or HERDR_SERVER_NAME`; if both empty/`'default'` → `'default'` + WARN. ✅
|
|
- Slug derivation logic unchanged — only its priority moved up. ✅
|
|
|
|
### Callers (side-effect analysis)
|
|
- `resume_session.sh:50`: `HERDR_SESSION_NAME="$(resolve_herdr_session "$SESSION_NAME" "$WORKSPACE")"` — passes `$WORKSPACE`. With the fix, slug is derived from `$WORKSPACE` (priority) — intended behavior. ✅
|
|
- `update_yaml_resumed.sh:40`: `resolve_herdr_session "$SESSION_NAME" "${WORKSPACE:-}"` — same pattern. ✅
|
|
- Function signature unchanged; no orphaned call sites. ✅
|
|
|
|
### Shim consistency
|
|
- `.mam/shim/herdr` contains the **identical** CWD-match fix (lines 149-165) — verified via `diff` (IDENTICAL). ✅
|
|
- `resolve_herdr_session` correctly **not** in the shim (it is a lib.sh utility function, not a herdr-command function). `grep -c = 0`. ✅
|
|
|
|
---
|
|
|
|
## 4. Loss / Hygiene (유실)
|
|
|
|
- **Single-file code change**: only `.agents/skills/lib.sh` (plus mirrored shim). No test/config/CI files touched. ✅
|
|
- **No orphaning**: signatures preserved; both callers verified. ✅
|
|
- **No stray artifacts**: working tree clean. ✅
|
|
- **No regressions**: see test results below. ✅
|
|
|
|
---
|
|
|
|
## 5. Test Results
|
|
|
|
| Suite | Result | Time |
|
|
|-------|--------|------|
|
|
| `test_workspace_scope.py` (slug derivation, env unset + ws) | **2 passed** | 0.14s |
|
|
| `test_tier1_unit.py -k 'resolve_herdr_session or derive_session_name'` | **5 passed**, 24 deselected | 0.65s |
|
|
| `test_deploy_layout.py` + `test_deploy_freshness.py` + `test_orc_onboard.py` (cross-regression) | **54 passed** | 28.49s |
|
|
|
|
**Coverage note (non-blocking)**: The specific priority reprioritization in Fix B — calling `resolve_herdr_session` with *both* `ws` provided *and* `HERDR_SESSION_NAME` set, asserting the slug wins — is not directly tested. Existing tests either unset env vars (`test_workspace_scope.py`) or omit `ws` (`test_tier1_unit.py`). The behavior is correct and tests pass, but a targeted test would lock in the reprioritization. Fix A's CWD-match flow is also not unit-tested (requires a real herdr binary — pre-existing limitation; the old `wss[0]` logic was likewise untested).
|
|
|
|
---
|
|
|
|
## 6. Verdict
|
|
|
|
The fix is correct, surgical, and well-contained. Fix A (CWD-match) resolves the real workspace-mismatch bug by matching on `realpath(cwd)` instead of blindly taking `wss[0]`, with a safe create-new fallback. Fix B (slug priority) correctly reprioritizes the workspace-derived slug above env vars when `ws` is provided, consistent with both callers that always pass `$WORKSPACE`. Both inline-Python blocks compile; `bash -n` is clean; the shim copy is identical; and the 54-test deploy+orc_onboard regression suite plus 7 direct resolve_herdr/workspace tests all pass with no cross-regression. The only note is a minor non-blocking test-coverage gap for the exact priority-reprioritization scenario. No design-level rework is needed.
|
|
|
|
[VERDICT: PASS] |