146 lines
15 KiB
Markdown
146 lines
15 KiB
Markdown
# 🛡️ Cross-Code Review: Job cfe439f6 — Auto UUID Capture & Pinning for Fresh Agent Sessions
|
|
|
|
- **Job ID**: cfe439f6
|
|
- **Reviewer**: cline
|
|
- **Target**: Review and implement robust automatic UUID capture and pinning mechanisms for fresh agent sessions (preventing null `session_id_own` after create/onboard). Update `create_session.sh`, `reconcile.sh`, `lib.sh`, and relevant skill/rule docs.
|
|
- **Change scope**: 13 modified files + 1 new test file (uncommitted working-tree diff; 268 insertions, 62 deletions).
|
|
|
|
## 0. Verdict Summary
|
|
|
|
The implementation is **sound and complete**. The core goal — preventing a null `session_id_own` after `create`/`onboard` by assigning a UUID at creation time and confirming it on disk via the reconciler — is achieved through a coherent three-stage protocol: (1) `create_session.sh` generates a UUID via `mam_gen_uuid` and passes `claude --session-id <uuid>`, recording `session_id_source: assigned` / `session_id_verified: false`; (2) `reconcile.sh` drift C0 verifies the transcript materialized and promotes `session_id_verified: true` / `last_visible_status: pinned`; (3) `resume_session.sh` detects whether the transcript exists on disk and chooses `--session-id` (fresh) vs `-r` (resume). Path normalization (`os.path.realpath` / `cd -P && pwd -P`) is applied consistently across shell and Python, closing the symlink/trailing-slash key-mismatch class. Lint passes, the new 13-case suite is green, and there is no cross-regression. Five non-blocking findings are documented below; none require a design-level rework.
|
|
|
|
[VERDICT: PASS]
|
|
|
|
---
|
|
|
|
## 1. Change Inventory (verified against working tree)
|
|
|
|
| File | Change | Status |
|
|
|---|---|---|
|
|
| `.agents/MULTI_AGENT_RULES.md` / `.ko.md` | New "Session ID Lifecycle & Auto-Assignment Protocol" section (auto-assign, first-message materialization, C0 confirmation, C-ambiguous guard, path equivalence) | ✅ verified |
|
|
| `.agents/skills/lib.sh` | New `mam_gen_uuid`, `mam_abs_workspace`, `mam_workspace_key`, `mam_session_iso_root`; `workspace_key` → `os.path.realpath`; `verify_session_uuid` multi-line scan + revalidate shortcut + iso_root paths + ordering invariant; `verify_tui_viewport` simplified; `find_workspace_uuid`/`verify_tui_viewport` → `mam_abs_workspace`; `_pane_capture` JSON unwrap | ✅ verified |
|
|
| `create_session.sh` | `mam_gen_uuid` for claude; `--session-id` flag in CMD_FULL; `session_id_source`/`session_id_verified` fields in YAML | ✅ verified |
|
|
| `reconcile.sh` | `row_agent()` helper; drift C0 (assigned-ID confirmation); C-ambiguous guard (all 4 agents); `os.path.realpath` in drift-B A-1 gate; drift-C loops use `row_agent()` | ✅ verified |
|
|
| `resume_session.sh` | `CLAUDE_ID_FLAG` logic: `--session-id` if transcript unmaterialized, `-r` if exists; `mam_workspace_key` + `mam_session_iso_root` for path resolution | ✅ verified |
|
|
| `multi-agent-mux-create/SKILL.md` | Pitfalls updated: removed "don't trust --session-id" / "first message generates id"; added auto-assignment + materialization docs | ✅ verified |
|
|
| `multi-agent-mux-monitor/SKILL.md` | Drift C rewritten: C0 (assigned confirmation), C (unassigned materialize), C-ambiguous (multiple candidates) | ✅ verified |
|
|
| `multi-agent-mux-resume/SKILL.md` | CMD_FULL comment: `--session-id` vs `-r` based on transcript existence | ✅ verified |
|
|
| `IMPROVEMENTS.md` | Rev.2 (b4a1d094) completed-task entry | ✅ verified |
|
|
| `LOG.md` | Agent status `stopped`→`running`, "캡처 완료"→"복원 완료" | ✅ verified |
|
|
| `tests/conftest.py` | Mock extracts `--session-id <uuid>` from cmd; `ws_abs = os.path.realpath(cwd)` | ✅ verified |
|
|
| `tests/test_tier3_integration.py` | `key = os.path.realpath(str(tmp_path))...` | ✅ verified |
|
|
| `tests/test_uuid_target.py` | NEW — 13 tests (T-1..T-13) | ✅ verified |
|
|
|
|
`git status --porcelain`: ` M` on 13 tracked files + `??` on `tests/test_uuid_target.py` (uncommitted — see R-4).
|
|
|
|
---
|
|
|
|
## 2. Lint / Static Checks
|
|
|
|
| Check | Command | Result |
|
|
|---|---|---|
|
|
| Shell syntax (lib.sh) | `bash -n .agents/skills/lib.sh` | ✅ PASS |
|
|
| Shell syntax (reconcile.sh) | `bash -n .../reconcile.sh` | ✅ PASS |
|
|
| Shell syntax (create_session.sh) | `bash -n .../create_session.sh` | ✅ PASS |
|
|
| Shell syntax (resume_session.sh) | `bash -n .../resume_session.sh` | ✅ PASS |
|
|
| Embedded Python (`_pane_capture`) | `sed -n '1872,1882p' lib.sh \| python3 -c 'compile(...)'` | ✅ PASS |
|
|
| `workspace_key` availability in reconcile | `exec(os.environ['MAM_VERIFY_PY'])` at reconcile.sh:327 loads `workspace_key` + `verify_session_uuid` from shared `VERIFY_SESSION_PYTHON` | ✅ no NameError |
|
|
| Path-key parity | `mam_workspace_key` (shell `tr '/_' '--'`) vs Python `.replace("/","-").replace("_","-")` — both over `realpath`/`cd -P && pwd -P` | ✅ verified by T-13 |
|
|
|
|
`shellcheck` is not installed locally (matches prior job convention); CI gate remains the authority for that check.
|
|
|
|
---
|
|
|
|
## 3. Operability / Logic Review
|
|
|
|
### 3.1 Create: auto-assignment (`create_session.sh:148-157, 312-324`)
|
|
- `SESSION_UUID="$(mam_gen_uuid)"` for claude only (agy/hermes/cline don't accept `--session-id`).
|
|
- `CMD_FULL="${RESOLVED_BIN} --dangerously-skip-permissions --session-id ${SESSION_UUID}"`.
|
|
- YAML entry records `claude_session_id_own = assigned`, `session_id_source = 'assigned'`, `session_id_verified = False`, `last_visible_status = "assigned (awaiting first message)"`. Correct — the row is created atomically with the assigned UUID, so there is no window where `session_id_own` is null after create. This is the core fix for the stated goal.
|
|
|
|
### 3.2 Reconciler C0: assigned-ID confirmation (`reconcile.sh:587-610`)
|
|
- Iterates `session_id_source == 'assigned' and not session_id_verified` running sessions.
|
|
- Calls `verify_session_uuid(cwd, agent, uuid, s, mode="discover")` — note **discover** mode, so the revalidate shortcut does NOT fire; the agent-specific transcript check runs (file exists, sessionId matches, cwd matches, epoch floor applies).
|
|
- On success: promotes `session_id_verified = True`, `last_visible_status = 'pinned'`, reports drift class C "confirmed on disk".
|
|
- **Separation from drift C**: drift C only processes sessions where `claude_session_id_own` is falsy (`if s.get('claude_session_id_own'): continue`). An assigned session has the UUID set, so drift C skips it. No double-processing. Correct.
|
|
|
|
### 3.3 Reconciler C-ambiguous guard (`reconcile.sh:628-633, 678-683, 721-726, 765-770`)
|
|
- For unassigned/legacy sessions, after scanning candidates, `if len(valid_candidates) > 1`: reports `C-ambiguous`, sets `last_visible_status = "ambiguous: N candidates"`, does NOT pin. The `== 1` pin block is skipped. Correct — prevents random pinning when multiple transcripts match.
|
|
|
|
### 3.4 `verify_session_uuid` hardening (`lib.sh:1173-1244`)
|
|
- **Ordering invariant** (lib.sh:1199-1209): workspace-key check runs BEFORE the revalidate shortcut. Comment cites T-12. This prevents handing back an id that belongs to a different workspace purely because the row is assigned+unverified. Verified by T-12.
|
|
- **Revalidate shortcut** (mode=="revalidate" + assigned + unverified → return True): lets a freshly-resumed session validate before its transcript is rewritten. The workspace check still guards it. Correct.
|
|
- **Multi-line scan** (lib.sh:1219-1242): reads up to 50 lines looking for `sessionId == uuid` and `cwd`. The old code read only the first line. This handles transcripts where the first line is a `queue-operation` event (no cwd) and the cwd appears on a later `user` line. Verified by T-2.
|
|
- **iso_root paths**: claude uses `(iso_root + "/projects") if iso_root else c_dir`; agy/hermes/cline use `iso_root or home`. Consistent with the isolation-root model. Verified by T-11.
|
|
|
|
### 3.5 Path normalization (`lib.sh:813-823, 1174-1180`; `reconcile.sh:509-510`; `resume_session.sh:85-95`)
|
|
- `mam_abs_workspace`: `( cd -P "$p" && pwd -P )` — resolves symlinks to real path.
|
|
- `mam_workspace_key`: pipes `mam_abs_workspace` through `tr '/_' '--'`.
|
|
- Python `workspace_key`: `os.path.realpath(path)` then `.replace("/","-").replace("_","-")`.
|
|
- These produce identical keys for absolute, trailing-slash, `..`-dotdot, and symlink inputs. Verified by T-13 (4 variations). This closes the symlink-key-mismatch class that could cause a session to be invisible to the reconciler.
|
|
|
|
### 3.6 Resume flag selection (`resume_session.sh:83-95`)
|
|
- `CLAUDE_ID_FLAG="-r"` default; if the transcript `${_proj_dir}/${_ws_key}/${UUID}.jsonl` does NOT exist → `--session-id` (fresh spawn). Otherwise `-r` (resume).
|
|
- `_proj_dir` resolves via `mam_session_iso_root` (isolation root) or `CLAUDE_PROJECT_DIR`/`$HOME/.claude/projects`. Matches `verify_session_uuid`'s claude base path. Correct — this is the key mechanism preventing resume from failing on a freshly created (unmaterialized) session.
|
|
|
|
### 3.7 `_pane_capture` JSON unwrap (`lib.sh:1864-1884`)
|
|
- The herdr shim returns `{"result": {"read": {"text": "..."}}}`. The new `_pane_capture` extracts `result.read.text` and falls back to raw on any parse failure. Defensive and correct — keeps `verify_tui_viewport` working against the JSON-wrapping shim.
|
|
|
|
### 3.8 `verify_tui_viewport` simplification (`lib.sh:1338-1362`)
|
|
- Removed 4 identical per-agent case branches (all did `grep -q "$base"`). Now: flatten whitespace, fixed-string grep on `base_flat`; regex fallback for absolute-path detection. Cleaner, equivalent behavior.
|
|
|
|
---
|
|
|
|
## 4. Test Results
|
|
|
|
| Suite | Command | Result |
|
|
|---|---|---|
|
|
| UUID target (new) | `pytest tests/test_uuid_target.py -q` | **13 passed** (57.27s) |
|
|
| Pure-lib.sh subset (fast) | `pytest test_t3 test_t12 test_t13 -v` | **3 passed** (0.40s) |
|
|
| Cross-regression (O-3+sanity+B-4) | `pytest tests/test_o3_scoped_guard.py tests/test_sanity.py tests/test_b4_session_created.py -q` | **47 passed** (23.79s) |
|
|
| Tier3 (modified `realpath` assertion) | `pytest tests/test_tier3_integration.py::test_integration_stop_purge_combination -q` | **1 passed** (36.61s) |
|
|
|
|
The 13-case `test_uuid_target.py` covers: T-1 create-assigned, T-2 assigned-ID promotion after materialize, T-3 different-cwd transcript not pinned, T-4 C-ambiguous (2 candidates), T-5 custom session name pinned, T-6..T-9 (agy/hermes/cline/legacy paths), T-10 path-variation resume (`/`, `..`, symlink), T-11 legacy isolation row, T-12 other-workspace revalidate fails (ordering invariant), T-13 shell/Python `workspace_key` equivalence across 4 path forms.
|
|
|
|
---
|
|
|
|
## 5. Findings (non-blocking)
|
|
|
|
### R-1: Wrapper-mode `SESSION_UUID` clear leaves misleading `cmd_full` in YAML (`create_session.sh:164-170, 156`)
|
|
In the claude `spawn()` branch, when a wrapper binary is used (`[ -x "$WRAPPER" ] && basename != claude`, or `--wrapper`), `SESSION_UUID=""` is cleared **after** `CMD_FULL` was already built as `... --session-id <uuid>`. The YAML therefore records `pane.cmd_full` containing `--session-id <uuid>` but `claude_session_id_own = None` / `session_id_source = 'pending-discovery'` (because `assigned = os.environ.get('SESSION_UUID', '') or None` reads the cleared value). The wrapper is expected to manage its own session-id, so functionally the session is discovered later via drift C — no runtime break. But the recorded `cmd_full` is cosmetically misleading. **Severity**: low. **Suggested fix**: rebuild `CMD_FULL` without `--session-id` in the wrapper branch (or clear it before `CMD_FULL` is composed and re-add only in the non-wrapper path).
|
|
|
|
### R-2: drift-B still inlines `name.endswith('-creator-<agent>')` (`reconcile.sh:494-500`)
|
|
The drift-B auto-register section infers the agent from the session-name suffix inline, while the new `row_agent()` helper (reconcile.sh:567-578) does the same with a pane-metadata-first fallback. This is **not** a bug — drift-B processes herdr sessions not yet in YAML (no `pane` dict), so the name convention is the only signal and `row_agent()` would degrade to the same fallback. It is a missed consolidation opportunity only. **Severity**: cosmetic. **Suggested fix**: optionally call `row_agent(t)` (it falls back to name) for single-source consistency.
|
|
|
|
### R-3: `verify_session_uuid` claude scan breaks on first cwd-bearing line (`lib.sh:1234-1236`)
|
|
The multi-line scan sets `found_cwd` and `break`s on the first line carrying a `cwd` field, even if `valid_session` (sessionId match) hasn't been confirmed on that line. In real claude transcripts every line carries the same `sessionId`, so `valid_session` and `found_cwd` converge. The only risk is a pathological transcript whose first cwd line has a *different* sessionId, which would return False (safe direction — fails closed). **Severity**: low / safe-direction. No fix required; documented for completeness.
|
|
|
|
### R-4: Changes uncommitted in working tree
|
|
`git status` shows 13 modified + 1 untracked file, none staged or committed. This matches the prior job's R-3 observation and is a process/policy note, not a code defect. The diff under review is the working-tree state. **Suggested action**: `git add -A && git commit -m "feat(uuid): auto-assign & pin session UUID at create; reconcile C0 confirm + C-ambiguous + path canonicalization"`.
|
|
|
|
### R-5: `mam_session_iso_root` spawns a Python process per resume (`resume_session.sh:86`, `lib.sh:828-842`)
|
|
`mam_session_iso_root` and `mam_workspace_key` each shell out to `env_python`. On the resume path this adds two Python startup costs. Correct and well-isolated; only a minor latency note for cold-resume. **Severity**: perf, non-blocking.
|
|
|
|
---
|
|
|
|
## 6. Completeness / Loss Check
|
|
|
|
| Aspect | Coverage |
|
|
|---|---|
|
|
| Goal: no null `session_id_own` after create | ✅ T-1 asserts `claude_session_id_own` non-null + `source=assigned` + `verified=False` immediately after create |
|
|
| Goal: confirmation after first message | ✅ T-2 asserts `verified=True` + `pinned` after transcript materialize + one reconcile cycle |
|
|
| Goal: resume uses correct flag | ✅ T-10 exercises `--session-id` vs `-r` via `--dry-run` across path variations; resume_session.sh:83-95 logic matches verify path |
|
|
| Ambiguity defense | ✅ T-4 asserts C-ambiguous reported + no pinning for 2 candidates |
|
|
| Cross-workspace safety | ✅ T-12 asserts revalidate fails for assigned row of a different workspace (ordering invariant) |
|
|
| Path canonicalization | ✅ T-13 asserts shell/Python key equivalence across 4 path forms; conftest + tier3 use `os.path.realpath` |
|
|
| Orphaned code | None found — `_pane_capture`/`verify_tui_viewport` simplifications remove now-redundant branches; `endswith` at reconcile.sh:494-500 is a distinct context (drift-B), not an orphan |
|
|
| Doc/test sync | ✅ SKILL.md (create/monitor/resume) + MULTI_AGENT_RULES + IMPROVEMENTS all reflect the new protocol; conftest mock updated to honor `--session-id` |
|
|
| Cross-regression | ✅ 47/47 (O-3 + sanity + B-4) green — no regression to prior fixes |
|
|
|
|
---
|
|
|
|
## 7. Conclusion
|
|
|
|
The change set implements the auto-assignment-and-pinning protocol end-to-end with clean separation of concerns: create assigns, reconcile confirms (C0) and guards ambiguity (C-ambiguous), resume adapts to transcript existence. Path canonicalization is applied uniformly in shell and Python and is proven equivalent by T-13. The ordering invariant (workspace check before revalidate shortcut) closes the cross-workspace leakage class (T-12). Lint passes on all touched shell scripts and embedded Python; the new 13-case suite is fully green; cross-regression (47) and the modified tier3 test are green. Five non-blocking findings are recorded (R-1..R-5), none requiring design-level rework. The stated goal — preventing a null `session_id_own` after create/onboard — is met.
|
|
|
|
[VERDICT: PASS] |