release(v4.9.6): opencode 2.x only (Issue #60) - archive the review reports, bump the lockstep version
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
This commit is contained in:
GodopuandClaude Opus 5.5 committed
1 parent
0e05b1b677
commit
cb2c84ccd7
29 files changed
+835
-25
No files matched your search
@@ -0,0 +1,56 @@
|
||||
# Review Report: Issue #60 (`fix/issue-60-opencode-v2`)
|
||||
|
||||
- **Job ID**: `1abe0a90`
|
||||
- **Reviewer**: `herdr:reviewer-agy-01` (`agy`)
|
||||
- **Commit Range**: `74333f87d0c02888c2b5b281e912d97e74348c01` .. `6b4b96fa1875862ef3bed830fe7b04166b1fad58` (3 commits across 13 files)
|
||||
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261018T055915Z-abe16e/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. Review Findings by Focus Area
|
||||
|
||||
### 1.1 OpenCode 2.x Schema, CLI Contract, and Purge Safety
|
||||
- **`_resolve_db` (`opencode debug paths`)**:
|
||||
- `.agents/skills/lib_py/agents/adapters/opencode.py:88-107` invokes `self._run(ctx, ["debug", "paths"], timeout=5)` with `HOME`, `HOME_DIR`, and `XDG_DATA_HOME=<ctx.home_dir>/.local/share`, parses the `db` line, and falls back to `f"{ctx.home_dir}/.local/share/opencode/opencode.db"`.
|
||||
- Verified against real `opencode v2.0.32` in an isolated temporary `HOME`: `debug paths` returns `<home>/.local/share/opencode/opencode.db` without creating the database file.
|
||||
- **`verify_artifact` and `discover` (`session_v2` & `parent_id IS NULL`)**:
|
||||
- `verify_artifact` (`.agents/skills/lib_py/agents/adapters/opencode.py:113-147`) queries `SELECT directory, time_created FROM session_v2 WHERE id=?`, converts millisecond `time_created` (`float(started_at) / 1000.0`) against `ctx.epoch`, and checks `workspace_key(found_cwd) == workspace_key(ctx.cwd)`.
|
||||
- `discover` (`.agents/skills/lib_py/agents/adapters/opencode.py:198-235`) queries `SELECT id FROM session_v2 WHERE parent_id IS NULL`, filtering out subagent child sessions (which share `directory` and have newer `time_created` than the top-level TUI session).
|
||||
- Verified against read-only inspection (`mode=ro`) of the host's migrated `opencode.db` and a fresh `2.0.32` DB in a temporary `HOME`: fresh 2.x databases contain `session_v2` (with `id`, `parent_id`, `directory`, `time_created`) and no `session`/`message`/`part`/`todo`/`session_share` tables.
|
||||
- **`purge_artifacts` and `residual_artifacts`**:
|
||||
- `purge_artifacts` (`.agents/skills/lib_py/agents/adapters/opencode.py:149-181`) checks `self._has_session(db, uuid)` (`SELECT 1 FROM session_v2 WHERE id=?`) before calling `self._run(ctx, ["session", "delete", "--standalone", "--", uuid], timeout=60)`.
|
||||
- Using `--` before `uuid` prevents option-flag injection if an ID ever starts with `-`, and `--standalone` avoids starting or attaching to a per-user background service.
|
||||
- In a fresh 2.0.32 DB, `session_inbox`, `session_message`, `session_pending`, `instruction_entry`, and `instruction_state` all have `ON DELETE CASCADE` foreign keys to `session_v2(id)`, and `opencode session delete` also removes `event` and `event_sequence` rows (`aggregate_id`) for the session and its child sessions while leaving other sessions untouched.
|
||||
- For databases migrated from 1.x, `purge_artifacts` subsequently deletes matching `uuid` rows from `part`/`parts`/`message`/`messages`/`session`/`sessions` (if those tables exist) and `residual_artifacts` reports any surviving `todo.session_id` or `session_v2.id` (e.g., if `session delete` failed or the binary was missing, in which case `purge_artifacts` emits a `WARN:` on `stderr`).
|
||||
|
||||
### 1.2 End-to-End Lifecycle (`create` → `reconcile` drift C → `status` → `stop` → `resume`)
|
||||
- **`create_session.sh`, `resume_session.sh`, `update_yaml_resumed.sh`**:
|
||||
- All three scripts and `OpenCodeAgentAdapter.spawn_spec` / `resume_spec` consistently use `opencode --auto --standalone` (and `opencode --session <uuid> --auto --standalone` on materialized resume), removing the defunct `--agent build` flag rejected by OpenCode 2.x TUI.
|
||||
- **`reconcile.sh` (drift C) and `status.sh`**:
|
||||
- `.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh:955-990` inspects `PRAGMA table_info(session_v2)`, filters by `parent_id IS NULL`, `directory = ?`, and `time_created >= ?` (ms), and validates candidates via `verify_session_uuid(cwd, 'opencode', uuid, s_eval, mode="discover")`.
|
||||
- `.agents/skills/multi-agent-mux-status/scripts/status.sh:127,337` checks `SELECT 1 FROM session_v2 WHERE id=?` in both human and JSON status paths.
|
||||
- **`--standalone` process lifecycle & `stop_session.sh`**:
|
||||
- Verified in an isolated PTY with real `opencode v2.0.32`: `opencode --auto --standalone` spawns `opencode serve --stdio --port 0` as a direct child of the TUI process; closing the PTY master terminates both the TUI and its `opencode serve --stdio --port 0` child cleanly within ~0.1s with zero lingering background processes.
|
||||
- `stop_session.sh`'s graceful `/exit` (`send_keys_safe` with echo wait before `Enter`) -> `herdr kill-session` (closes PTY) -> `SIGKILL` fallback and `in-use` gate (`ps` argv check for `--session <uuid>`) work cleanly with `--standalone`.
|
||||
|
||||
### 1.3 Linux Portability, Shell/Python Compatibility, and Completeness
|
||||
- **Linux & XDG portability**:
|
||||
- `OpenCodeAgentAdapter._run` explicitly sets `HOME`, `HOME_DIR`, and `XDG_DATA_HOME = os.path.join(ctx.home_dir, ".local/share")`, ensuring that even on Linux hosts where `XDG_DATA_HOME` is set in the ambient desktop environment, adapter CLI invocations remain isolated to `ctx.home_dir`.
|
||||
- `test_real_opencode_2x_contract` explicitly unsets `XDG_DATA_HOME`, `XDG_STATE_HOME`, `XDG_CONFIG_HOME`, `XDG_CACHE_HOME`, and `HOME_DIR` via `monkeypatch.delenv`.
|
||||
- **Python 3.9 & Bash 3.2 compatibility**:
|
||||
- Verified on macOS `/usr/bin/python3` / `.venv/bin/python` (3.9.6) and `/bin/bash` (3.2.57 with `set -u`).
|
||||
- **Repository-wide search for stale 1.x assumptions**:
|
||||
- Grepping `.agents/skills/`, `tests/`, and `docs/` for `db path`, `--agent build`, `FROM session\b`, `INTO session\b`, and `table_info(session)` confirms no remaining 1.x code or documentation assumptions exist outside explanatory comments.
|
||||
- **Test coverage & negative regression verification**:
|
||||
- `tests/test_issue60_opencode_v2.py` (10/10 passed, including `test_real_opencode_2x_contract` against real `opencode v2.0.32`), `tests/test_a4_adapter_contract.py`, `tests/test_issue31_purge_residual.py`, `tests/test_issue42_skill_blocks.py`, `tests/test_issue55_agent_homes.py`, and `tests/test_tier1_unit.py` all pass.
|
||||
- Reverting `SELECT id FROM session_v2 WHERE parent_id IS NULL` to `SELECT id FROM session` in an isolated `/tmp` copy immediately fails `test_discover_and_verify_read_session_v2_and_skip_child_sessions` and `test_a_fresh_2x_database_has_no_session_table_and_still_works`.
|
||||
|
||||
---
|
||||
|
||||
## 2. Non-Blocking Observation (Out of Scope for Issue #60)
|
||||
|
||||
- **[Note — `stall_watch.py` `background_children` with `--standalone`]**:
|
||||
- Because `opencode --auto --standalone` keeps `opencode serve --stdio --port 0` alive as a child process of the TUI while idle, and `OpenCodeAgentAdapter.resident_children` is `()`, `lib_py.stall_watch.background_children(agent_pid, adapter.resident_children)` sees the `opencode serve --stdio --port 0` child process and conservatively withholds the optional idle nudge (`NOTIFY_IDLE_AND_NUDGE`). If idle nudging is desired for `--standalone` OpenCode in the future, `stall_watch.background_children` can exempt children whose `argv` matches `opencode serve --stdio` (while still walking any tool subprocesses spawned beneath that server). This does not affect any session lifecycle, discovery, or purge behavior in Issue #60.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,60 @@
|
||||
# Issue #60 Code Review Report — Round 3 (`0e05b1b`)
|
||||
|
||||
- **Job ID**: `5420eae1`
|
||||
- **Reviewer**: `agy` (`herdr:reviewer-agy-01`)
|
||||
- **Branch**: `fix/issue-60-opencode-v2`
|
||||
- **Base .. Head**: `74333f87d0c02888c2b5b281e912d97e74348c01` .. `0e05b1b6777ceb837066454ac688e04a0f5d14a0` (5 commits; Round 3 delta: `0e05b1b`)
|
||||
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261018T073639Z-b5cb7f/1`
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive Summary
|
||||
|
||||
Commit `0e05b1b` cleanly resolves all blocking and non-blocking findings raised in Round 2 (`cb5f439`) without introducing regressions:
|
||||
|
||||
1. **Test isolation & Issue #55 path-variable guard (`tests/_agent_home_guard.py`)**:
|
||||
- `XDG_DATA_HOME` is added to `DROPPED_VARS`, so `conftest._isolate_agent_homes` drops any ambient `XDG_DATA_HOME` for every test (preventing a sandbox that overrides `HOME` from resolving the runner's ambient `XDG_DATA_HOME` via `_data_home(ctx)`).
|
||||
- `watch_dirs(ambient)` now watches `os.path.join(ambient.get("XDG_DATA_HOME") or os.path.join(home, ".local", "share"), "opencode")` so the session leak guard monitors the real opencode data directory even when `XDG_DATA_HOME` is set in the ambient environment.
|
||||
- `tests/test_issue55_agent_homes.py::test_l1_every_path_variable_the_code_reads_is_accounted_for` passes.
|
||||
2. **`status.sh` DB path resolution (`.agents/skills/multi-agent-mux-status/scripts/status.sh:108-111, 308-311`)**:
|
||||
- Both `--json` and text modes now resolve the opencode DB path via `get_adapter('opencode')._resolve_db(DiscoveryContext(workspace=cwd, agent_name='opencode', home_dir=home))` instead of a hardcoded `~/.local/share/opencode` lookup, keeping `status.sh` consistent with `reconcile.sh` and the adapter's `debug paths` / `XDG_DATA_HOME` resolution.
|
||||
3. **Real binary `--version` probe isolation (`tests/test_issue60_opencode_v2.py:294-301`)**:
|
||||
- `_real_opencode()` now runs `opencode --version` inside a `tempfile.TemporaryDirectory()` with `HOME` and `TMPDIR` pointed at that temp dir and `XDG_*` omitted from `env`, preventing import-time log writes to the user's real `~/.local/share/opencode/log`.
|
||||
|
||||
---
|
||||
|
||||
## 2. Verification of Cumulative Changes (`74333f8..0e05b1b`)
|
||||
|
||||
| Area | Verification Result |
|
||||
| :--- | :--- |
|
||||
| **Adapter CLI & Schema (`opencode.py`)** | `spawn_spec` / `resume_spec` use `--auto --standalone` (and `--session <id>` when materialized); `_resolve_db` parses `opencode debug paths` with fallback to `<_data_home>/opencode/opencode.db`; `discover` (`WHERE parent_id IS NULL`) and `verify_artifact` query `session_v2`. |
|
||||
| **Purge & Residual (`opencode.py`)** | Collects `_descendants(db, uuid)` from `session_v2` and legacy `session` before invoking `opencode session delete --standalone -- <uuid>`, deletes `_LEGACY_TABLES` rows for `[uuid] + descendants`, and reports remaining `session_v2` / legacy rows (tagging child sessions with `[child session <id>]`). |
|
||||
| **In-Use & Stall Watch (`opencode.py`)** | `conversation_in_use` ignores `serve --service` holders of `opencode.db` while detecting `--standalone` / `--session` processes; `resident_children = ('opencode',)` prevents idle `opencode serve --stdio --port 0` from tripping `background_children` while still detecting tool subprocesses under it. |
|
||||
| **Shell Scripts (`create_session.sh`, `resume_session.sh`, `update_yaml_resumed.sh`, `reconcile.sh`, `status.sh`)** | All shell launch/resume commands use `--auto --standalone` without `--agent`; `reconcile.sh` drift C queries `session_v2` with `parent_id IS NULL`; `status.sh` uses the adapter's `_resolve_db` and queries `session_v2`. |
|
||||
| **Tests & Guard (`_agent_home_guard.py`, `_fake_opencode.py`, `test_issue60_opencode_v2.py`, `test_a4_adapter_contract.py`, `test_issue31_purge_residual.py`, `test_tier2_component.py`)** | `XDG_DATA_HOME` is in `DROPPED_VARS` and watched by `watch_dirs`; `_real_opencode()` runs `--version` in an isolated temporary `HOME`/`TMPDIR`. |
|
||||
|
||||
---
|
||||
|
||||
## 3. Test Execution Summary
|
||||
|
||||
All tests were executed in the verification worktree (`/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261018T073639Z-b5cb7f/1`) with `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, external `--basetemp`, and isolated environment variables:
|
||||
|
||||
1. **Adapter, Guard, Purge/Residual, and Issue #60 Suites**:
|
||||
- `tests/test_issue60_opencode_v2.py` (15 passed, including `test_real_opencode_2x_contract` against real opencode v2.0.32 in isolated `HOME`/`TMPDIR`)
|
||||
- `tests/test_issue55_agent_homes.py` (11 passed, including `test_l1_every_path_variable_the_code_reads_is_accounted_for`)
|
||||
- `tests/test_a4_adapter_contract.py` (33 passed)
|
||||
- `tests/test_issue31_purge_residual.py` (67 passed)
|
||||
- **Total**: `126 passed in 48.31s`
|
||||
2. **Tier 2 Component Suite (`opencode or status`) with `XDG_DATA_HOME` Canary**:
|
||||
- Ran `tests/test_tier2_component.py -k "opencode or status"` with `XDG_DATA_HOME` pointed at a canary temp directory.
|
||||
- **Result**: `7 passed in 62.22s`, and `XDG_CANARY` remained completely empty.
|
||||
3. **Post-check**:
|
||||
- Verification worktree is clean (`git status -s` empty, no `.mam` directory), and no stray `opencode` test processes remain.
|
||||
|
||||
---
|
||||
|
||||
## 4. Findings
|
||||
|
||||
No blocking or non-blocking issues remaining. The branch `fix/issue-60-opencode-v2` (`74333f8..0e05b1b`) is ready to merge.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,62 @@
|
||||
# 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]
|
||||
Reference in new issue
Block a user