109 lines
7.0 KiB
Markdown
109 lines
7.0 KiB
Markdown
# Cross Code Review — Job a99563f9
|
||
|
||
**Review target**: B-1 edge-case fix — `find_workspace_uuid` tier-3 identity cache lookup in `lib.sh`
|
||
**Changeset**: Working-tree diff (2 files: `.agents/skills/lib.sh` +45/−25, `IMPROVEMENTS.md` +21/−5) + new untracked test `tests/test_b1_tier3_identity.py` (8 tests, V-1..V-8)
|
||
**Reviewer**: cline | **Date**: 2026-08-05
|
||
|
||
---
|
||
|
||
## 1. Bug Analysis — Original Code (HEAD) vs Fix
|
||
|
||
### 1.1 Original bugs in `find_workspace_uuid` tier-3 (lib.sh:1265-1305 at HEAD)
|
||
|
||
The original tier-3 identity cache lookup had **5 distinct defects**:
|
||
|
||
| # | Bug | Impact |
|
||
|---|-----|--------|
|
||
| 1 | `db_path = f"{mam_dir}/agent-sessions.db" if 'mam_dir' in locals()` — `mam_dir` is **never defined** in this Python scope | Always falls to `os.path.join(ws, ".mam", ...)` — guesses path instead of using the authoritative `YAML_PATH` env var. Breaks when workspace dir ≠ state dir. |
|
||
| 2 | `import yaml` at top of try block | If PyYAML is missing, the **entire** try block fails — including the SQLite DB branch that doesn't need yaml. Tier-3 is permanently dead. |
|
||
| 3 | `d = yaml.safe_load(f)` — **shadows** the merged state dict `d` (loaded at line 1132) | Corrupts the state dictionary for any code after tier-3 that reads `d`. |
|
||
| 4 | `ai.get('conversation_id')` for hermes/cline (lines 1297, 1299) | Reads from the top-level `ai` dict instead of the agent-specific `ai_agent` sub-dict. Wrong lookup — `conversation_id` is per-agent, not top-level. |
|
||
| 5 | `ai = {}` initialized, then `ai = json.loads(row[0]).get('agent_identities', {})` — no type guard | If `agent_identities` is a non-dict (e.g., corrupted string), `ai.get(agent)` at line 1289 raises `AttributeError`, causing `rc=1` (violates the "always exits 0" contract). |
|
||
|
||
### 1.2 Fix applied (working tree)
|
||
|
||
The fix addresses all 5 bugs:
|
||
|
||
1. **Path guessing eliminated**: Uses `d.get('agent_identities')` from the already-loaded merged state (primary source), falling back to `os.environ['YAML_PATH']` (authoritative path set by `env_python`).
|
||
2. **`import yaml` moved inside `elif` branch**: SQLite DB branch now works without PyYAML.
|
||
3. **`_ydoc` replaces `d`**: No shadowing of the merged state dict.
|
||
4. **`ai_agent.get('conversation_id')`**: Correct sub-dict lookup for hermes/cline.
|
||
5. **Type guards**: `isinstance(ai, dict)` checks before use; `if not isinstance(ai, dict): ai = {}` final guard ensures graceful degradation.
|
||
|
||
### 1.3 Design principle: DB is authority, YAML is mirror
|
||
|
||
The fix establishes a clear priority order (V-8 test):
|
||
1. Check `d` (merged state, loaded from DB first, YAML fallback) — primary source
|
||
2. If `d` doesn't have `agent_identities`, read from `$YAML_PATH` — but DB branch takes priority over YAML branch
|
||
---
|
||
|
||
## 2. Test & Syntax Validation
|
||
|
||
| Check | Result | Detail |
|
||
|-------|--------|--------|
|
||
| `bash -n` syntax (lib.sh) | ✅ PASS | No syntax errors |
|
||
| `py_compile` (test file) | ✅ PASS | `tests/test_b1_tier3_identity.py` compiles |
|
||
| `test_b1_tier3_identity.py` (V-1..V-8) | ✅ **8/8 PASS** (1.11s) | All regression tests pass |
|
||
| `test_workspace_scope.py` | ✅ 2/2 PASS | No regression |
|
||
| `test_tier1_unit.py` (find_workspace_uuid tests) | ✅ 3/3 PASS | `test_resume_find_workspace_uuid_empty`, `_target_non_existent`, `_invalid_agent` — no regression |
|
||
|
||
### 2.1 Test coverage detail (V-1..V-8)
|
||
|
||
| Test | Scenario | PASS |
|
||
|------|----------|------|
|
||
| V-1 | tier-3 honours `AGENT_SESSIONS_YAML` path when workspace ≠ state dir | ✅ |
|
||
| V-2 | tier-3 DB branch works even when PyYAML module is absent (PYTHONPATH stub) | ✅ |
|
||
| V-3 | Non-dict `agent_identities` (corrupted string) handled gracefully, exits 0 | ✅ |
|
||
| V-4 | hermes tier-3 fallback reads `conversation_id` from `ai_agent` (not `ai`) | ✅ |
|
||
| V-5 | tier-3 identity ignored if `project_cwd` doesn't match workspace | ✅ |
|
||
| V-6 | tier-3 returns empty string when `agent_identities` is absent (silent) | ✅ |
|
||
| V-7 | `load_state_json` preserves `agent_identities` in state blob | ✅ |
|
||
| V-8 | tier-3 does NOT read YAML mirror when DB exists without identity (DB authority) | ✅ |
|
||
---
|
||
|
||
## 3. IMPROVEMENTS.md Documentation Review
|
||
|
||
The IMPROVEMENTS.md changes correctly:
|
||
- Mark B-1 as ✅ resolved with detailed fix description (F1/F2)
|
||
- Add two new related findings: B-10 (no write path for `agent_identities`) and B-11 (`load_state_json` PyYAML hard dependency)
|
||
- Update the total count from 18 → 19 (B-1 resolved: −1, B-10 + B-11 added: +2, net +1) — arithmetic verified: 8 unresolved edge-case bugs → 9 ✓
|
||
- Update the NOTE block to include B-1 completion alongside A-1 and A-5
|
||
|
||
The new B-10 and B-11 findings are properly scoped as future work, not part of this fix.
|
||
|
||
---
|
||
|
||
## 4. Findings
|
||
|
||
### 4.1 Blocking defects — NONE
|
||
|
||
No syntax errors, no test failures, no regressions. The fix correctly addresses all 5 original bugs with proper type guards and test coverage.
|
||
|
||
### 4.2 Non-blocking observations
|
||
|
||
**R-1 (Info — `agent_identities` write path absence is tracked as B-10)**
|
||
The fix correctly reads `agent_identities` but, as noted in the new B-10 finding in IMPROVEMENTS.md, no code in the repository actually *writes* `agent_identities`. This means tier-3 is structurally always empty for newly created workspaces — it only serves as a backward-compat read path for legacy state files that may have `agent_identities` populated. This is a known limitation, not a defect in this fix. The decision to add a write path or document it as legacy-only is tracked as B-10 for future work.
|
||
|
||
**R-2 (Info — `os.environ['YAML_PATH']` KeyError risk)**
|
||
The fallback path at lib.sh:1271 uses `os.environ['YAML_PATH']` (not `.get()`). If `YAML_PATH` is somehow unset, this would raise `KeyError`. However, `env_python` (line 664) always sets `YAML_PATH` as the first env var, so this is safe in practice. Using `os.environ.get('YAML_PATH', '')` would be more defensive, but the current code is correct given the `env_python` contract.
|
||
|
||
**R-3 (Info — Test file is untracked)**
|
||
`tests/test_b1_tier3_identity.py` is untracked (`git status` shows `??`). It should be committed alongside the lib.sh fix. Not a code issue, just a staging note.
|
||
|
||
---
|
||
|
||
## 5. Verdict
|
||
|
||
The B-1 fix is a well-executed, surgical correction of 5 distinct bugs in the `find_workspace_uuid` tier-3 identity cache lookup:
|
||
|
||
1. **Path guessing** → uses authoritative `$YAML_PATH` env var
|
||
2. **PyYAML hard dependency** → `import yaml` deferred to YAML-only branch
|
||
3. **State dict shadowing** → uses `_ydoc` instead of `d`
|
||
4. **Wrong sub-dict lookup** → `ai_agent.get()` instead of `ai.get()` for hermes/cline
|
||
5. **Missing type guards** → `isinstance` checks prevent `AttributeError` on corrupt data
|
||
|
||
The fix is backed by 8 comprehensive regression tests (V-1..V-8) covering all 5 bugs plus the DB-authority-over-YAML-mirror design principle. All tests pass. No regressions in pre-existing tests. The IMPROVEMENTS.md documentation is accurate and properly tracks the two new related findings (B-10, B-11) for future work.
|
||
|
||
No blocking issues. The code is correct, tested, and well-documented.
|
||
|
||
[VERDICT: PASS] |