GodopuandClaude Opus 5.5
cb2c84ccd7
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
63 lines
6.3 KiB
Markdown
63 lines
6.3 KiB
Markdown
# Review Report (Round 2): Issue #60 (`fix/issue-60-opencode-v2`)
|
|
|
|
- **Job ID**: `c9d06e85`
|
|
- **Reviewer**: `herdr:reviewer-agy-01` (`agy`)
|
|
- **Commit Range**: `74333f87d0c02888c2b5b281e912d97e74348c01` .. `cb5f439cce41a2741a6b9974a308a765e50f00ad` (4 commits; Round 2 delta: `6b4b96fa1875862ef3bed830fe7b04166b1fad58..cb5f439cce41a2741a6b9974a308a765e50f00ad`, 1 commit)
|
|
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261018T064723Z-79133f/1`
|
|
- **Host / Runtime**: macOS `arm64-apple-darwin25` (Darwin 25.3.0), `/usr/bin/python3` 3.9.6 & `.venv/bin/python` 3.9.6, `/bin/bash` 3.2.57, `opencode` v2.0.32 (`/Users/godopu16/.opencode/bin/opencode`)
|
|
|
|
---
|
|
|
|
## 1. Round 1 Findings Verification (`6b4b96f..cb5f439`)
|
|
|
|
All five Round 1 items addressed in commit `cb5f439` work as designed and their new tests fail against pre-fix Round 1 code (`6b4b96f`):
|
|
|
|
1. **[Resolved — Child session 1.x row purge & residual reporting (`_descendants`, `_LEGACY_TABLES`)]**:
|
|
- `.agents/skills/lib_py/agents/adapters/opencode.py:160-222` resolves all descendant session IDs (`_descendants(db, uuid)`, walking `parent_id` across both `session_v2` and 1.x `session` tables to arbitrary depth with cycle protection in read-only mode) before calling `opencode session delete --standalone -- <uuid>`.
|
|
- `purge_artifacts` then deletes rows for `[uuid] + descendants` across all 1.x legacy tables (`_LEGACY_TABLES`: `message`, `messages`, `part`, `parts`, `todo`, `session_share`, `session`, `sessions`).
|
|
- `residual_artifacts` (`.agents/skills/lib_py/agents/adapters/opencode.py:229-238`) checks `[uuid] + self._descendants(db, uuid)` and tags surviving descendant rows with `[child session <id>]`.
|
|
2. **[Resolved — `conversation_in_use` ignores background `serve --service` daemon]**:
|
|
- `.agents/skills/lib_py/agents/adapters/opencode.py:240-262` checks `conversation_in_use(uuid, [])` first, then inspects `lsof -t <db>` PIDs via `ps -o command= -p <pid>` and ignores processes with `serve --service` in their command line while still treating `--standalone` TUI/server holders as in-use (and failing safe to `True` on `lsof`/`ps` errors).
|
|
3. **[Resolved — `XDG_DATA_HOME` handling in `_data_home(ctx)`]**:
|
|
- `.agents/skills/lib_py/agents/adapters/opencode.py:89-99` uses the caller's `XDG_DATA_HOME` when `ctx.home_dir` resolves to `os.path.expanduser("~")` (or is empty), and `<ctx.home_dir>/.local/share` when `ctx.home_dir` is a separate directory.
|
|
4. **[Resolved — `resident_children = ('opencode',)`]**:
|
|
- `.agents/skills/lib_py/agents/adapters/opencode.py:72-76` declares `resident_children = ('opencode',)`, so `stall_watch.background_children` skips the idle `opencode serve --stdio --port 0` child while still walking any tool subprocesses spawned beneath it.
|
|
5. **[Resolved — Test coverage & pre-fix failure check]**:
|
|
- `test_real_opencode_2x_contract` binds `adapter._bin` to `REAL` (no `PATH` dependency), and `test_comp_opencode_create_and_reconcile_and_stop` (`tests/test_tier2_component.py:1252-1273`) verifies that `reconcile.sh` drift C pins the top-level session rather than a newer child session in the same directory.
|
|
- Running the 5 new tests in `tests/test_issue60_opencode_v2.py` against Round 1 commit `6b4b96f` in an isolated `/tmp` copy fails all 5 (`5 failed, 10 deselected`).
|
|
|
|
---
|
|
|
|
## 2. Issue Blocking Merge
|
|
|
|
### [P1] `tests/test_issue55_agent_homes.py::test_l1_every_path_variable_the_code_reads_is_accounted_for` fails (`XDG_DATA_HOME` missing from `tests/_agent_home_guard.py`)
|
|
|
|
- **Location**: `tests/_agent_home_guard.py:21` (triggered by `.agents/skills/lib_py/agents/adapters/opencode.py:94` reading `os.environ.get("XDG_DATA_HOME")`)
|
|
- **Symptom**:
|
|
```text
|
|
FAILED tests/test_issue55_agent_homes.py::test_l1_every_path_variable_the_code_reads_is_accounted_for
|
|
AssertionError: path variables the adapters/lib.sh read but conftest._isolate_agent_homes does not drop: ['XDG_DATA_HOME']
|
|
```
|
|
- **Root Cause & Linux Isolation Impact**:
|
|
- `test_issue55_agent_homes.py` scans `.agents/skills/lib_py/agents/*.py`, `lib_py/paths.py`, and `lib.sh` for any `*_DIR` / `*_HOME` environment variable read by the adapters and requires every such variable to be accounted for in `tests/_agent_home_guard.py` (`DROPPED_VARS` or `LEFT_ALONE`).
|
|
- Furthermore, `XDG_DATA_HOME` **must** be in `DROPPED_VARS` (not `LEFT_ALONE`): in `opencode.py:95`, `own = os.path.realpath(ctx.home_dir or "") == os.path.realpath(os.path.expanduser("~"))`. When a test isolates `HOME` via `monkeypatch.setenv("HOME", str(tmp_path))` (such as `mam_sandbox`), `os.path.expanduser("~")` equals `ctx.home_dir` (`own` is `True`). On a Linux machine where `XDG_DATA_HOME` is set in the outer shell environment, if `conftest._isolate_agent_homes` does not drop `XDG_DATA_HOME`, `_data_home(ctx)` would use the outer environment's real `XDG_DATA_HOME` instead of `<tmp_path>/.local/share`!
|
|
- **Required Fix**:
|
|
Add `"XDG_DATA_HOME"` to `DROPPED_VARS` in `tests/_agent_home_guard.py:21` (and optionally honour `ambient.get("XDG_DATA_HOME")` for `"opencode"` in `watch_dirs(ambient)` at `tests/_agent_home_guard.py:48`):
|
|
```python
|
|
DROPPED_VARS = ("HOME_DIR", "CLAUDE_PROJECT_DIR", "CLAUDE_CONFIG_DIR", "CODEX_HOME", "XDG_DATA_HOME", "LOCAL_BIN")
|
|
```
|
|
*(Verified in an isolated `/tmp` copy: adding `"XDG_DATA_HOME"` to `DROPPED_VARS` makes both `tests/test_issue55_agent_homes.py` (11/11) and `tests/test_issue60_opencode_v2.py` (15/15, including `test_the_data_dir_follows_the_callers_xdg_data_home_only_for_its_own_home` which sets `XDG_DATA_HOME` inside the test) pass cleanly).*
|
|
|
|
---
|
|
|
|
## 3. Verification Summary
|
|
|
|
- `tests/test_issue60_opencode_v2.py`: **15 passed** (including `test_real_opencode_2x_contract` against `opencode v2.0.32`)
|
|
- `tests/test_a4_adapter_contract.py`: **33 passed**
|
|
- `tests/test_tier2_component.py -k opencode`: **1 passed** (`test_comp_opencode_create_and_reconcile_and_stop`)
|
|
- `tests/test_issue31_purge_residual.py -k opencode`: **1 passed** (`test_opencode_purge_leaves_no_rows`)
|
|
- `tests/test_issue55_agent_homes.py`: **10 passed, 1 failed** (`test_l1_every_path_variable_the_code_reads_is_accounted_for`)
|
|
- Verification worktree `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261018T064723Z-79133f/1` verified clean (`git status -s` empty, no `.mam` directory created).
|
|
|
|
[VERDICT: NOT PASS]
|