GodopuandClaude Opus 5.5
cffe279eb0
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
94 lines
8.5 KiB
Markdown
94 lines
8.5 KiB
Markdown
# PR #62 Code Review Report — Issue #60 Linux Follow-up (`1d7b7da..6a4e385`)
|
||
|
||
- **Job ID**: `ba63f0b5`
|
||
- **Reviewer**: `agy` (`herdr:reviewer-agy-01`)
|
||
- **Branch**: `fix/issue-60-linux`
|
||
- **Base .. Head**: `1d7b7dab58a10fe86fd747b7290d47674ca93230` .. `6a4e385a78c8a561d5cf3644336de6a352ea9271` (8 commits)
|
||
- **Verification Worktree**: `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261019T000805Z-975c4c/1`
|
||
|
||
---
|
||
|
||
## 1. Executive Summary
|
||
|
||
All production code changes in PR #62 (`_ro_connect`, `_DB_PATHS` cache, `input_placeholder_pattern`, `auth_ok` / `auth-ok`, `stop_session.sh` log text, and `test_u1e`) are well-designed and behave as intended on macOS (Python 3.9.6, Apple SQLite 3.51.0, opencode 2.0.32).
|
||
|
||
However, **5 test cases in `tests/test_issue60_linux_fixes.py` fail on macOS** (`test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-*]`) due to a macOS vs. Linux SQLite difference in `wal_home`'s `make("clean")` fixture helper:
|
||
- On Linux (upstream SQLite default `SQLITE_FCNTL_PERSIST_WAL = 0`), closing the last connection to a WAL database (`_wal_writer(db, ws).close()`) checkpoints the WAL into `opencode.db` and unlinks `opencode.db-wal` and `opencode.db-shm`.
|
||
- On macOS (`/usr/bin/python3` linked against Apple's `/usr/lib/libsqlite3.dylib`, which sets `SQLITE_FCNTL_PERSIST_WAL = 1` by default), `_wal_writer(db, ws).close()` checkpoints the WAL into `opencode.db` and truncates `opencode.db-wal` to 0 bytes, **but leaves `opencode.db-wal` and `opencode.db-shm` on disk**.
|
||
- Consequently, `assert make(state) == listing` at `tests/test_issue60_linux_fixes.py:181` fails with `AssertionError: assert ['opencode.db', 'opencode.db-shm', 'opencode.db-wal'] == ['opencode.db']` before `_ro_connect` is even called.
|
||
|
||
Unlinking `opencode.db-wal` and `opencode.db-shm` after `_wal_writer(db, ws).close()` in `make("clean")` makes all 20 `(query × state)` cases pass on macOS as well.
|
||
|
||
---
|
||
|
||
## 2. Findings
|
||
|
||
### `[P1]` Blocking — `wal_home` fixture `make("clean")` fails on macOS due to Apple SQLite `SQLITE_FCNTL_PERSIST_WAL=1`
|
||
|
||
- **Location**: `tests/test_issue60_linux_fixes.py:157-159, 181`
|
||
- **Symptom**:
|
||
Running `pytest tests/test_issue60_linux_fixes.py` on macOS (`/usr/bin/python3` 3.9.6, `sqlite3.sqlite_version == '3.51.0'`) fails 5 tests:
|
||
- `FAILED tests/test_issue60_linux_fixes.py::test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-_descendants]`
|
||
- `FAILED tests/test_issue60_linux_fixes.py::test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-_has_session]`
|
||
- `FAILED tests/test_issue60_linux_fixes.py::test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-discover]`
|
||
- `FAILED tests/test_issue60_linux_fixes.py::test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-residual_artifacts]`
|
||
- `FAILED tests/test_issue60_linux_fixes.py::test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-verify_artifact]`
|
||
- **Root Cause**:
|
||
Apple's system `libsqlite3.dylib` enables `SQLITE_FCNTL_PERSIST_WAL` by default. When `_wal_writer(db, ws).close()` runs on macOS, SQLite checkpoints the committed WAL frames into `opencode.db` (`8192` bytes) and truncates `opencode.db-wal` to `0` bytes, but keeps `opencode.db-wal` and `opencode.db-shm` on disk instead of unlinking them.
|
||
- **Verification & Fix**:
|
||
In `make(state)` (`tests/test_issue60_linux_fixes.py:157-159`), removing any remaining sidecars after `_wal_writer(db, ws).close()` checkpoints the DB:
|
||
```python
|
||
def make(state):
|
||
if state == "clean":
|
||
_wal_writer(db, ws).close()
|
||
for suffix in ("-wal", "-shm"):
|
||
if os.path.exists(str(db) + suffix):
|
||
os.unlink(str(db) + suffix)
|
||
```
|
||
leaves `opencode.db` with its WAL header and checkpointed rows and no sidecar files (`["opencode.db"]`). Verified on macOS that all 20 `(query × state)` combinations pass with this change, and that `_ro_connect(db)`'s `mode=ro&immutable=1` branch is crucial on macOS (where a plain `mode=ro` open on a sidecar-free WAL-mode DB raises `sqlite3.OperationalError: unable to open database file`, and a plain read-write `sqlite3.connect(db)` creates and leaves `-wal` and `-shm`).
|
||
|
||
---
|
||
|
||
### `[P2]` Non-blocking — `status.sh` and `reconcile.sh` still open `opencode.db` with read-write `sqlite3.connect(odb)`
|
||
|
||
- **Locations**:
|
||
- `.agents/skills/multi-agent-mux-status/scripts/status.sh:116, 316`
|
||
- `.agents/skills/multi-agent-mux-monitor/scripts/reconcile.sh:953`
|
||
- **Observation**:
|
||
While `OpenCodeAgentAdapter` methods (`verify_artifact`, `discover`, `_has_session`, `_descendants`, `residual_artifacts`) now use `_ro_connect(db)`, `status.sh` (both `--json` and text mode) and `reconcile.sh` (drift C, which `status.sh` also runs via `--dry-run`) still call `sqlite3.connect(odb)` directly:
|
||
- On macOS, calling `sqlite3.connect(odb)` on a sidecar-free WAL-mode `opencode.db` creates `opencode.db-wal` and `opencode.db-shm` and leaves them on disk after `.close()`.
|
||
- On Linux, calling `sqlite3.connect(odb)` on a `leftover` WAL database checkpoints the WAL and deletes `opencode.db-wal` and `opencode.db-shm` on `.close()`.
|
||
- **Suggestion**:
|
||
In `status.sh:108-120, 308-320`, replace the raw `sqlite3.connect(odb)` block with `get_adapter('opencode')._has_session(odb, u)` (which already uses `_ro_connect`), and in `reconcile.sh:953`, use `_ro_connect(odb)` from `lib_py.agents.adapters.opencode`.
|
||
|
||
---
|
||
|
||
## 3. Detailed Review by Checklist Item
|
||
|
||
1. **`_ro_connect` on macOS (`.agents/skills/lib_py/agents/adapters/opencode.py:13-36`)**:
|
||
- **Both `-wal` and `-shm` present (`leftover` / `writer`)**: `sqlite3.connect(f"file:{quote(path)}?mode=ro", uri=True)` reads uncommitted-to-main-file (WAL-committed) rows through the existing WAL index without creating or removing sidecars.
|
||
- **No `-wal` (`clean`)**: `sqlite3.connect(f"file:{quote(path)}?mode=ro&immutable=1", uri=True)` reads `opencode.db` directly without creating `-wal`/`-shm` (and avoids `sqlite3.OperationalError: unable to open database file` that plain `mode=ro` raises on macOS when opening a WAL-header DB without sidecars). Because every query opens, fetches, and immediately closes `conn`, any race with a writer starting right after `os.path.exists(path + "-wal")` only sees the pre-WAL snapshot for that single short read and never holds an immutable handle across writes.
|
||
- **`-wal` without `-shm` (`walonly`)**: Copies `opencode.db` and `opencode.db-wal` into a `tempfile.TemporaryDirectory(prefix="mam-opencode-")`, opens the private copy so SQLite can rebuild `-shm` and recover the committed WAL frames, backs it up to `:memory:`, and cleans up the temp dir. Verified on macOS that committed rows in `-wal` are returned and the source directory remains `['opencode.db', 'opencode.db-wal']`.
|
||
2. **`_DB_PATHS` process-local cache (`opencode.py:40, 144-160`)**:
|
||
- Keyed by `(bin_path, ctx.home_dir, self._data_home(ctx))` (or `(bin_path, HOME, XDG_DATA_HOME)` when `ctx.home_dir` is unset).
|
||
- Different `HOME`, `XDG_DATA_HOME`, or binary paths produce distinct cache keys; failed `debug paths` runs (`returncode != 0` or no `db` line) are not cached.
|
||
3. **`auth_ok` & `create_session.sh` (`opencode.py:317-321`, `__main__.py:120-127`, `create_session.sh:146-151`)**:
|
||
- Checks binary existence on `PATH` or `<home>/.opencode/bin/opencode` without spawning `opencode --version` (preventing log writes to the real opencode data dir). Other agents' branches in `create_session.sh` are untouched.
|
||
4. **`input_placeholder_pattern` (`opencode.py:75-79`, `base.py:212-217`, `input_region.py:80-81`)**:
|
||
- `re.fullmatch(r'Ask anything(?:…|\.\.\.)(?: "[^"]*"?)?', head.strip())` strips both full and narrow-truncated 2.x home screen placeholders while preserving user input starting with `Ask anything…` (such as `'Ask anything… "x" and fix it'`). Other adapters return `None`.
|
||
5. **Python 3.9 & macOS compatibility**:
|
||
- All syntax, type annotations, and shell scripts are compatible with Python 3.9.6 and macOS bash 3.2 (`set -euo pipefail`). `git diff --check` is clean.
|
||
|
||
---
|
||
|
||
## 4. Test Execution Summary
|
||
|
||
Executed in `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261019T000805Z-975c4c/1` with isolated `HOME`, external `--basetemp`, `PYTHONDONTWRITEBYTECODE=1`, and `-p no:cacheprovider`:
|
||
- `tests/test_issue60_opencode_v2.py`: 15 passed
|
||
- `tests/test_a4_adapter_contract.py`: 33 passed
|
||
- `tests/test_issue50_dispatch.py`: 49 passed
|
||
- `tests/test_issue55_agent_homes.py`: 11 passed
|
||
- `tests/test_issue60_linux_fixes.py`: 40 passed, **5 failed** (`test_a_query_reads_right_and_leaves_the_sidecars_alone[clean-listing0-*]`)
|
||
|
||
[VERDICT: NOT PASS]
|