docs(improvements): record O-4 Python Abstract Agent Adapter architecture proposal (100% PASS)
This commit is contained in:
@@ -0,0 +1,90 @@
|
||||
# Cross-Code Review Report — Job 3b42cc9b
|
||||
|
||||
- **Job ID**: 3b42cc9b
|
||||
- **Reviewer**: cline (herdr:canary-projects-multi-agent-mux-creator-cline)
|
||||
- **Base commit**: `245abe6` (working tree clean — diff reviewed: `9df0fc3..245abe6`)
|
||||
- **Scope**: Audit `create_session.sh`, `reconcile.sh`, `resolve_session_id.sh`, `lib.sh` for 5 objectives.
|
||||
- **Output**: `.mam/jobs/3b42cc9b/cline-reports/report-final.md`
|
||||
|
||||
## 1. Audit Scope & Method
|
||||
|
||||
The task is an **audit** of the current committed state of the four target scripts against five stated objectives:
|
||||
|
||||
1. Sequential prompt injection
|
||||
2. Post-spawn auto-pinning
|
||||
3. Occupied-ID preemption guard
|
||||
4. Stage 3 viewport verification
|
||||
5. No UUID cross-talk or shadowing
|
||||
|
||||
Method: read each target file end-to-end, trace each objective from creation → reconcile → resume, run `bash -n` (×4) + embedded-Python `compile()` (×8), then execute the three relevant test suites against the live tree.
|
||||
|
||||
## 2. Lint & Test Results
|
||||
|
||||
| Check | Result |
|
||||
|---|---|
|
||||
| `bash -n` `lib.sh` | PASS |
|
||||
## 3. Objective-by-Objective Audit
|
||||
|
||||
### 3.1 Sequential Prompt Injection — PASS
|
||||
|
||||
`create_session.sh` enforces a strict spawn→ready→inject sequence:
|
||||
- `spawn` (L165) → `wait_for_tui_ready` (L205, polls up to 30×1s for agent-specific ready tokens) → `handle_startup_dialogs` (claude only, L211) → pane meta capture → YAML append → **single** `inject_instructions` call (L376).
|
||||
- No prompt is injected before the TUI is ready; only one prompt is injected per creation (no concurrent multi-prompt race).
|
||||
- `inject_instructions` (lib.sh L1847) delegates to `send_keys_safe` (lib.sh L1932), which waits for `_pane_quiescent`, clears blocking dialogs (timeout-bounded), then atomically `set-buffer`/`paste-buffer`/`delete-buffer` + `C-m`. Submission is verified against rendered tokens (`●`, `✽`, `…ing`, `esc to interrupt`) over up to 3 retries.
|
||||
- The `--submit-job` path publishes `started` **only after** injection returns rc 0 (L382); on failure it publishes `error` and exits 1 (L378-380). Sequential and ordered.
|
||||
|
||||
### 3.2 Post-Spawn Auto-Pinning — PASS
|
||||
|
||||
- `create_session.sh` (claude, L153/L157): `SESSION_UUID="$(mam_gen_uuid)"` → `CMD_FULL="... --session-id ${SESSION_UUID}"` → YAML stores `claude_session_id_own=assigned`, `session_id_source='assigned'`, `session_id_verified=False` (L320-323).
|
||||
- `reconcile.sh` drift C0 confirms the assigned ID once the transcript materializes: `verify_session_uuid(mode="revalidate")` (lib.sh L1207-1209 shortcut returns True when workspace matches + source==assigned + verified==False, then the on-disk `.jsonl` check at L1212-1244 confirms it), after which `_pin_and_verify_resume` (reconcile.sh L432) sets `session_id_verified=True` and `last_visible_status='pinned'`.
|
||||
- An immediate priority reconcile cycle is kicked off asynchronously right after creation (create_session.sh L384: `reconcile.sh --once &`), so pinning is attempted promptly without waiting for the next scheduled cycle.
|
||||
|
||||
### 3.3 Occupied-ID Preemption Guard — PASS
|
||||
|
||||
Four independent layers enforce that a fresh/resume session never gets an ID already occupied:
|
||||
1. **Assign-time**: `mam_gen_uuid` generates a fresh random UUID (no reuse of existing).
|
||||
2. **Resolve-time** (`find_workspace_uuid`, lib.sh L1406-1420): builds `running_ids` from ALL running sessions' own-IDs; `emit(u)` silently skips any UUID in `running_ids`. A resume will never be handed a live session's ID.
|
||||
3. **Discover-time** (agy path, `verify_session_uuid` lib.sh L1263-1265): rejects a candidate present in `row['_sibling_claimed_uuids']` — collected in reconcile.sh L664-673 from sibling rows sharing the same cwd that are not stopped/terminated.
|
||||
4. **Write-time** (validation layer, lib.sh L1083-1094): ID Uniqueness Check raises `SystemExit` if two running sessions share the same own-ID — defense-in-depth at persistence time.
|
||||
| `bash -n` `create_session.sh` | PASS |
|
||||
| `bash -n` `reconcile.sh` | PASS |
|
||||
| `bash -n` `resolve_session_id.sh` | PASS |
|
||||
| Embedded Python `compile()` (8 blocks across 5 files) | PASS |
|
||||
| `tests/test_uuid_target.py` | **13/13 PASS** (53.43s) |
|
||||
### 3.4 Stage 3 Viewport Verification — PASS
|
||||
|
||||
`verify_tui_viewport` (lib.sh L1338-1363) implements the 3-stage viewport check:
|
||||
- rc 2: session gone or pane capture empty/unavailable (degraded).
|
||||
- rc 0: workspace `basename` (whitespace-stripped) appears in pane content (match).
|
||||
- rc 1: a `/path/` pattern appears but the workspace basename does not (mismatch).
|
||||
|
||||
`reconcile.sh` (all 4 agents, e.g. agy L686-695) gates pinning on this: with exactly one valid candidate, rc 0 → `_pin_and_verify_resume(degraded=False)`; rc 1 → `C-warn`, **not pinned** (will retry); rc 2 → `_pin_and_verify_resume(degraded=True)` (pin via stages 1-3 only, documented degraded path). Tests T-6 (degraded) and T-7 (mismatch) cover the non-happy paths.
|
||||
|
||||
### 3.5 No UUID Cross-Talk or Shadowing — PASS
|
||||
|
||||
- **Workspace scoping**: `verify_session_uuid` ORDERING INVARIANT (lib.sh L1199-1205) — the `workspace_key(cwd) != workspace_key(ws)` check runs BEFORE the assigned-id shortcut, so a row from a **different** workspace is rejected first even when assigned+unverified (tested T-12). `find_workspace_uuid` only considers sessions whose `pane.cwd == ws` (L1426).
|
||||
- **C-ambiguous guard** (reconcile.sh, all 4 agents): when `len(valid_candidates) > 1`, reports `C-ambiguous` and does **not** pin (tested T-4) — no silent attribution of a possibly-wrong UUID.
|
||||
- **Path canonicalization**: `mam_abs_workspace` uses `cd -P && pwd -P` (physical path) and `workspace_key` uses `os.path.realpath`. Shell (create/resolve) and Python (verify/find) therefore agree on the workspace key, preventing cross-talk from symlink/logical-path divergence (tested T-10 symlink + 6/6 path forms).
|
||||
- `resolve_session_id.sh` (L44) is a thin wrapper over `find_workspace_uuid`, preserving the same workspace-isolated resolution path (P0-C: never returns a global id whose `project_cwd` differs from this workspace).
|
||||
| `tests/test_o3_scoped_guard.py` + `test_sanity.py` + `test_b4_session_created.py` | **47/47 PASS** (18.03s) |
|
||||
| `tests/test_tier3_integration.py::test_integration_stop_purge_combination` | **1/1 PASS** (33.26s) |
|
||||
## 4. Findings (Non-Blocking)
|
||||
|
||||
All findings are non-blocking; none require design rework.
|
||||
|
||||
| # | Finding | Severity | Location |
|
||||
|---|---|---|---|
|
||||
| A-1 | **Wrapper-mode clears `SESSION_UUID` after `CMD_FULL` is composed.** In `spawn`'s claude wrapper branch (L170), `SESSION_UUID=""` is set *after* `CMD_FULL` already baked `--session-id ${SESSION_UUID}` (L157). The YAML `cmd_full` display field (L304) therefore records `--session-id <uuid>` even though the wrapper launch cleared it. The authoritative fields (`claude_session_id_own`, `session_id_verified`) are unaffected, so pinning/resume are correct; only the cosmetic `cmd_full` string is misleading. | Low / cosmetic | create_session.sh L157, L170, L304 |
|
||||
| A-2 | **`verify_session_uuid` breaks on first cwd-bearing line.** In the claude branch (L1234-1236), the loop breaks as soon as a line carrying `cwd` is found, even if `sessionId` has not yet been confirmed on that line. If a transcript interleaves an unrelated `cwd` before the matching `sessionId` line, the function could `break` before `valid_session` is set. Safe-direction (fails closed — returns False rather than mis-attribute), so not a correctness bug, but worth a comment. | Low | lib.sh L1232-1236 |
|
||||
| A-3 | **`mam_session_iso_root`/`mam_workspace_key` spawn Python per resume.** Each resume call re-launches a Python interpreter for these helpers, adding minor latency. No correctness impact. | Low / perf | lib.sh (helpers) |
|
||||
| A-4 | **Drift-B `endswith` vs `row_agent()`.** The drift-C loops in `reconcile.sh` use the `row_agent()` helper, but the drift-B A-1 gate (the `endswith('-creator-<agent>')` inference in `find_workspace_uuid` L1473-1488) uses inline `endswith`. Different context (B is offline resolution; C is live reconcile), so not a bug, but a single-source consolidation would reduce drift. | Low / consistency | lib.sh L1473-1488 |
|
||||
|
||||
No `[ESCALATE: PLANNER]` is warranted: every objective is satisfied and all findings are local fixes or cosmetic.
|
||||
|
||||
## 5. Completeness & Loss Check
|
||||
|
||||
- **Completeness**: all 5 objectives are implemented and exercised by `test_uuid_target.py` (13 cases) plus the cross-regression suite (47 cases). Every objective maps to at least one passing test (T-4 ambiguous, T-6/T-7 viewport, T-10 symlink, T-12 ordering invariant, T-5 custom name pinning).
|
||||
- **Loss check**: the committed diff (`9df0fc3..245abe6`) adds `mam_gen_uuid`, `mam_abs_workspace`, `mam_workspace_key`, `mam_session_iso_root`, the C0/C-ambiguous/revalidate logic, and path canonicalization without removing prior resolution behavior for non-claude agents (agy/hermes/cline branches preserved and extended). No functional regression (47/47 + 1/1).
|
||||
- **Working tree**: clean — no uncommitted changes outstanding for this scope.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user