GodopuandClaude Opus 5.5
38ea4f3b8e
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
86 lines
11 KiB
Markdown
86 lines
11 KiB
Markdown
# Review 366a4d58 — Issue #64
|
||
|
||
PASS. Commit `b0eccc5` moves `_ro_connect` into `base.py` and uses it for every query-only open of an agy, codex, or hermes database found in the skills tree. The other-workspace agy sidecars come from reconcile drift C: it walks every conversation `*.db` and `verify_artifact` used to open each one read-write. On this host that open leaves `-wal` and `-shm` after close, which is what the #55 name guard counts. Head related tests: 95 passed. The new file on base: 23 failed, 20 passed.
|
||
|
||
Reviewed only the snapshot `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261019T134017Z-d90c23/1` (`loop-20261019T134017Z-d90c23`). Head `b0eccc530149bc6e9211727e19921665c7b80492`, tree `f5a412a3ee96465db7c4b3223d2d76e95716ddfa`, base `3ce5d5fdc108b0435e933a251fe1c1256b5d7973` is an ancestor and is the control-plane HEAD. One commit, 8 files, +461/−56. `git diff --check` is clean. Snapshot porcelain is empty and the snapshot has no `.mam`. Control-plane porcelain is empty. No product file was edited. The source worktree was not run.
|
||
|
||
The commit is `b0eccc5` `fix(agents): open agy, codex and hermes databases read-only (Issue #64)`. Files: `agy.py`, `codex.py`, `hermes.py`, `opencode.py`, `base.py`, `reconcile.sh`, `status.sh`, and new `tests/test_issue64_ro_connect.py`.
|
||
|
||
Host tools: control-plane venv Python 3.9.6, sqlite 3.51.0, bash 3.2.57(1)-release. `bash -n` accepts `status.sh` and `reconcile.sh`. The changed modules imported and ran under pytest. The opencode binary was not launched. Linux was not executed here. The real database was not opened.
|
||
|
||
## Cause
|
||
|
||
Reconcile drift C for an unpinned running agy row (`reconcile.sh:786-817`) lists every `<HOME_DIR>/.gemini/antigravity-cli/conversations/*.db` (`:800`) and calls `verify_session_uuid(cwd, 'agy', uuid, s_eval, mode="discover")` (`:816`) for each basename. `home` in that script is `os.environ['HOME_DIR']` (`:437`). `verify_session_uuid` (`verify_session.py:83-108`) builds a `DiscoveryContext` and returns `adapter.verify_artifact`. `DiscoveryContext` fills a missing `home_dir` through `resolve_home` (`paths.py:11`): the argument, then `HOME_DIR`, then `HOME`, then `~`. The reconcile call omits `home_dir`, so the open uses the same home as the glob.
|
||
|
||
On base, `AgyAgentAdapter.verify_artifact` did `sqlite3.connect(path)` and `SELECT count(*) FROM steps` once the file existed and was new enough for the epoch. A discover-mode cache miss returns false only when the uuid is in `_sibling_claimed_uuids` (siblings on the same pane cwd). `last_conversations.json` mapping that uuid to a different workspace does not skip the open. A read-write open of a WAL database on macOS sqlite 3.51.0 creates `-wal` and `-shm` when they are absent and leaves those names after close. The #55 guard snapshots entry names at session start and fails when a new name appears. At check time no process held the database because the connection had already closed. That matches the v4.9.7 timing: the review loop and the suite were running reconcile against the real home.
|
||
|
||
`status.sh` agy (`:78-81` and `:278-281`) returns from `os.path.exists` of the `.db`. It does not open sqlite.
|
||
|
||
Head `agy.py:74` uses `_ro_connect(path)` for the same `SELECT count(*) FROM steps`. With no `-wal`, that open is `mode=ro&immutable=1` and does not create sidecars.
|
||
|
||
`test_unpinned_agy_reconcile_leaves_other_workspace_clean_db_without_sidecars` reproduces this on a temporary home. `last_conversations.json` maps the conversation to workspace B, the row is an unpinned agy session whose pane cwd is workspace A, and the test calls `verify_session_uuid` (the function drift C calls). The function returns true and the directory stays one file. On base the same test failed because the listing gained `-wal` and `-shm`. The runtime test also spawns `reconcile.sh --once --emit-diff --dry-run` for a same-workspace unpinned agy session, with `HOME` and `HOME_DIR` pointed at the sandbox, and that listing stays unchanged.
|
||
|
||
A cache miss still accepts the other workspace's database when it has at least one step and is not sibling-claimed. The new test asserts that true return. Drift C can still treat that file as a candidate. The open no longer creates names.
|
||
|
||
## Changed opens
|
||
|
||
`_ro_connect` is `base.py:111-134`. Both sidecars use `file:<abspath>?mode=ro`. No `-wal` uses `mode=ro&immutable=1`. A `-wal` without `-shm` is copied into `tempfile.TemporaryDirectory(prefix="mam-ro-")`, opened read-write on that copy only, backed into a memory connection, and the temp directory is removed. The copy name is `os.path.basename(path)` or `state.db`, so `<uuid>.db`, `state.db`, and `state_5.sqlite` keep their `-wal` sibling. The temp prefix was `mam-opencode-` while the helper lived in `opencode.py`.
|
||
|
||
`db_rows_naming` (`base.py:137-145`) now defaults to `_ro_connect`. A caller can still pass `connect`.
|
||
|
||
Query sites on the helper:
|
||
|
||
- `agy.py:74` `verify_artifact` (`SELECT count(*) FROM steps`). `discover` (`agy.py:165-175`) reads `last_conversations.json` and calls `verify_artifact`.
|
||
- `codex.py:80` `_find_rollout_via_db`. `codex.py:195` `discover` (`PRAGMA table_info(threads)` and `SELECT id, cwd FROM threads`). `codex.py:167-169` `residual_artifacts` goes through `db_rows_naming` for `state_*.sqlite` and `thread_history_*.sqlite`.
|
||
- `hermes.py:51` `verify_artifact`. `hermes.py:100` `discover`.
|
||
- `opencode.py` imports `_ro_connect` from `base` (`:3`). Query sites stay `:149`, `:212`, `:234`, `:249`, `:316`, and `connect=_ro_connect` at `:249`. The identity test asserts `opencode_mod._ro_connect is base_mod._ro_connect`.
|
||
- `status.sh`: `_ro_connect(hdb)` at `:91` and `:291`, `_ro_connect(odb)` at `:116` and `:316`, `_ro_connect(db)` at `:132` and `:332`. The static test requires each of those counts to be 2.
|
||
- `reconcile.sh`: import at `:423`. Hermes drift `_ro_connect(hdb)` at `:865`, opencode `_ro_connect(odb)` at `:954`, codex drift `_ro_connect(state_db)` at `:1048`. The static test requires each count to be 1.
|
||
|
||
`status.sh` and `reconcile.sh` contain no `sqlite3.connect` literal.
|
||
|
||
## Left as read-write
|
||
|
||
These opens write, or they are MAM's own databases. They stay `sqlite3.connect`:
|
||
|
||
- `opencode.py:193` deletes 1.x rows after `session delete`.
|
||
- `hermes.py:74` purge `DELETE`.
|
||
- `codex.py:152` purge `DELETE FROM threads`.
|
||
- `lib.sh:1599` and `lib.sh:2068` (timeout 60).
|
||
- `lib_py/state.py:17`.
|
||
- `lib_py/verify_session.py:41` opens the state database beside `AGENT_SESSIONS_YAML` or `YAML_PATH` (`<yaml stem>.db`, `SELECT data FROM state`, timeout 5). That is MAM's `agent-sessions` state, not an agent conversation database.
|
||
- `lib_py/loop/runner.py:416`.
|
||
- `lib_py/atomic_yaml.py:61`.
|
||
|
||
Agy purge removes files and does not open sqlite. Claude and grok adapters have no `sqlite3.connect`. The three `sqlite3.connect` calls inside `_ro_connect` are the helper: the `mode=ro` URI, the immutable URI, and the read-write open of the private copy.
|
||
|
||
`VERSIONS.md` and the skill guides are unchanged. Version bumps stay with the orchestrator.
|
||
|
||
## Tests
|
||
|
||
Pytest used the control-plane venv, `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, and a `--basetemp` outside the snapshot. The basetemp path contains neither `mam-review` nor `shim`. Inherited `HERDR_*`, `MAM_*`, `MQTT_*`, `SKS_*`, `DELEGATE_JOB_LOGS_DIR`, `HOME_DIR`, `CLAUDE_PROJECT_DIR`, `CLAUDE_CONFIG_DIR`, `CLAUDE_CODE_TMPDIR`, `CODEX_HOME`, `PYTHONPATH`, `WORKSPACE_ROOT`, `MAM_ENV_FILE`, `AGENT_SESSIONS_YAML`, `XDG_STATE_HOME`, `XDG_CONFIG_HOME`, `XDG_CACHE_HOME`, and `XDG_DATA_HOME` were dropped. `WORKSPACE_ROOT` was not pointed at the snapshot.
|
||
|
||
Head, cwd the snapshot, basetemp `/tmp/mam366-366a4d58-pybase`, files `tests/test_issue64_ro_connect.py` and `tests/test_issue60_linux_fixes.py`: 95 passed in 72.05 s. Exit 0. No skip and no error. The new file is 43 of those (helper identity, agy, codex including `db_rows_naming`, hermes, one static script check, four runtime script states, and the cross-workspace case). The issue #60 file is the other 52, so the move still passes the existing opencode read-only tests.
|
||
|
||
Base proof used a throwaway `git clone --shared` of the snapshot, checked out at `3ce5d5f`, with only `tests/test_issue64_ro_connect.py` taken from `b0eccc5`. Cwd was the clone. Basetemp was `/tmp/mam366-366a4d58-pybase-base`. Result: 23 failed, 20 passed in 10.84 s. Exit 1. The failures are the helper-location test, every clean and wal-only adapter query, the static script count (`_ro_connect(hdb)` was 0), runtime clean and wal-only, and the cross-workspace clean listing. Clean `db_rows_naming` returned `[]` and clean `residual_artifacts` returned only the rollout file; on base that helper's default open is a `mode=ro` URI (`base.py` at `3ce5d5f`). The other clean and wal-only failures are listings that gained `-wal` and `-shm` after a query that still returned the row. The 20 passes are the leftover and live-writer states, where those names were already present, so a read-write open did not change the listing.
|
||
|
||
The four states in the new file are clean (close, then unlink `-wal` and `-shm` when they exist), leftover sidecars, `-wal` without `-shm`, and a connection left open. Each query asserts the right row and an unchanged directory listing. Runtime covers `reconcile.sh --once --emit-diff --dry-run` and `status.sh --json` plus text mode. Agy, hermes, and codex `resume_state` are `yes`.
|
||
|
||
## Remaining risk
|
||
|
||
The check for `-wal` and the immutable connect are two steps. A wal created in that gap is invisible to the immutable open. A writer that already has both sidecars is read with `mode=ro`.
|
||
|
||
`mode=ro` with both sidecars rewrites the existing `-shm` bytes (the reader index). The `-wal` bytes and the file names stay. That rewrite was measured on the Issue #60 review and left in place.
|
||
|
||
Purge `DELETE` opens stay read-write, so a purge can still create or remove sidecars. That is the write the brief left out.
|
||
|
||
Drift C can still accept another workspace's agy database when the file has steps and the uuid is not sibling-claimed. The candidate decision is unchanged. The open no longer adds names.
|
||
|
||
`verify_session.py:41` still opens MAM's own state database read-write.
|
||
|
||
The user's background service stayed up. Its pid is 23630 and the command is `/Users/godopu16/.opencode/bin/opencode serve --service`. The real database mtime is 1792420196 and its size is 36712448. `~/.config/opencode/service.json` mtime is 1792246838. `~/.local/state/opencode/service.json` mtime is 1792365227. The real `opencode.log` mtime is 1792420391 and its size is 2699345. The herdr config mtime is 1784456394.
|
||
|
||
After the report, snapshot porcelain is still empty, the snapshot still has no `.mam`, and control-plane porcelain is still empty at `3ce5d5fdc108b0435e933a251fe1c1256b5d7973`. The herdr config mtime is still 1784456394. The opencode database mtime is still 1792420196 and its size is still 36712448. The opencode log mtime is still 1792420391 and its size is still 2699345. Service pid 23630 is still `opencode serve --service`.
|
||
|
||
[VERDICT: PASS]
|