Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
81 lines
12 KiB
Markdown
81 lines
12 KiB
Markdown
# Review f02af611 — Issue #40
|
||
|
||
PASS. Commit `feccfd5` pins a post-discovered session id when the receiving side has verified `completed`, and agy no longer records another workspace's conversation. The four completion conditions fail on base `ff8b381` and pass on this head. Related tests: 76 passed.
|
||
|
||
Reviewed only the snapshot `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261020T133532Z-a1684a/1` (`loop-20261020T133532Z-a1684a`). Head `feccfd5e0356c8acf0438bc68bce7ea36ce2588e`, tree `0f0f33cc22f5e492a0ddfccb6bac5be888c10d1a`, base `ff8b381520ae04e80c73a9e18bfad73d47e853c9` is an ancestor and is the control-plane HEAD. One commit, 19 files, +1128/−338. `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 `feccfd5` `fix(agents): pin post-discovered session ids on a verified completed; agy never takes another workspace's or session's conversation (Issue #40)`.
|
||
|
||
Host tools: control-plane venv Python 3.9.6, sqlite 3.51.0, bash 3.2.57(1)-release. `bash -n` accepts `lib.sh`, `create_session.sh`, `reconcile.sh`, and `multi-agent-mux-delegate-job`. Linux was not executed here. The real agent databases were not opened.
|
||
|
||
## When a pin happens
|
||
|
||
Class A adapters (`agy`, `opencode`, `codex`, `hermes`) keep `id_lifecycle` `post_discovered`. Class B (`claude`, `grok`) return `pre_assigned`. The default on `BaseAgentAdapter` is `post_discovered`.
|
||
|
||
`capture_active_session_id` is read-only. The default (`base.py:384-400`) keeps a `discover()` candidate only when `verify_session_uuid` accepts it and no other row already records that id. Zero verified candidates is `no-candidate`. Two or more is `ambiguous`. Agy replaces that method (`agy.py:129`): `last_conversations.json` may propose the one id mapped to this cwd, `verify_session_uuid` must accept it, and a holder of that conversation's presence lock or database must be the row's pane process or a descendant (`agy.py:202-231`). A cache entry alone is not enough, because one cwd has one cache slot.
|
||
|
||
The write lives in `lib_py.agents.autopin`, which the agent process does not run. `handle_completed` runs only for a local job record whose status is `completed`. Remote transport, a `remote:` session, and a human job return `skipped`. A dispatch token passed by the caller must match the record. The pin key is `job_id:dispatch_token:attempt_id`, stored in `<registry>/<job>/autopin.json` under a flock on `autopin.lock`. A second call with the same key returns the stored result and does not capture again.
|
||
|
||
Callers:
|
||
|
||
- `delegate_autopin_completed` in `lib.sh` (the shell only launches `python -m lib_py.agents.autopin`).
|
||
- The delegate-job wrapper, after `finalize_unfinished`, when `job_status` is `completed` (`multi-agent-mux-delegate-job` beside the audit-log echo).
|
||
- `create_session.sh`, in the onboarding `completed` branch.
|
||
- `LoopRunner.wait_for_job`, immediately after it observes `completed` (`runner.py:676-679`, `_autopin` at `:690`). This call does not pass `--expect-dispatch-token`; the marker key still uses the token stored on the record.
|
||
|
||
`pin_row` (`autopin.py:73-78`) sets the agent's own key, `session_id_source` `discovered`, `session_id_verified` true, and `last_visible_status` `pinned`, then the caller replaces that status with the resume dry-run result. A captured id is pinned when `verify_tui_viewport` returns 0, and also when it returns something other than 1 (the old degraded pin). Return 1 leaves the id empty and records `viewport-mismatch`. Any other capture status except `pre-assigned`, `already-set`, and `skipped` writes `last_visible_status` `autopin <status> (job <id>): <reason>` and still stores the marker, so that job is not tried again. The function's exit status stays 0 (`|| true` in the shell helper; `main` returns 0).
|
||
|
||
Reconcile drift C for agy, hermes, opencode, and codex calls the same `autopin.capture` (`reconcile.sh:795`) and the same `pin_row` (`:728`). Ambiguous capture sets `ambiguous: N candidates` and does not pin. A single `captured` id still goes through `verify_tui_viewport`. Claude drift C is unchanged and still scans its own jsonl directory.
|
||
|
||
## What changed, and what stayed
|
||
|
||
| File | Why |
|
||
| --- | --- |
|
||
| `lib_py/agents/base.py` | `PRE_ASSIGNED`, `POST_DISCOVERED`, `capture_result`, default `id_lifecycle`, default `capture_active_session_id` |
|
||
| `lib_py/agents/autopin.py` | Receiving-side capture, pin, dedup marker, CLI |
|
||
| `adapters/agy.py` | Discover mode rejects an id the cache maps to another cwd, and rejects a sibling id even when the cache names this cwd. Capture adds the pane-owner check |
|
||
| `adapters/claude.py`, `adapters/grok.py` | `id_lifecycle` is `pre_assigned` |
|
||
| `lib.sh`, delegate-job wrapper, `create_session.sh` | Call the handler after a verified `completed` |
|
||
| `loop/runner.py` | Same handler after `wait_for_job` sees `completed` |
|
||
| `reconcile.sh` | Class A drift C is the shared capture. The agy `*.db` glob and the hermes, opencode, and codex SQL in the shell are gone |
|
||
| create and monitor `SKILL.md`, `docs/NEW_AGENT_INTEGRATION_GUIDE.md` | The new adapter fields and who writes the registry |
|
||
| `tests/test_issue40_auto_pin.py` | The four completion conditions |
|
||
| `test_issue64_ro_connect.py`, `test_issue60_linux_fixes.py`, `test_issue60_opencode_v2.py`, `test_tier1_unit.py`, `test_tier2_component.py` | Assertions that drift C now goes through `autopin.capture`, and the Issue #64 cross-workspace assert is now false |
|
||
|
||
`publish_event.py` and `publisher.py` have no autopin and no session-registry write. `test_call_sites_and_publisher_boundary` checks that and passed. Purge deletes stay read-write. Status still opens hermes, opencode, and codex with `_ro_connect`. Adapter queries still use `_ro_connect`. `reconcile.sh` contains neither `_ro_connect` nor `sqlite3.connect`; the adapters open the databases. `VERSIONS.md` is unchanged.
|
||
|
||
Stop's own capture path is unchanged. A new pin from this handler records `session_id_source: discovered` and `session_id_verified: true`. An id that stop recorded earlier can still disagree with those two fields. The issue comment asked for that stop cleanup only if it stayed small, and it is outside this diff.
|
||
|
||
`_pin_and_verify_resume` now records source and verified for claude too, because it calls `pin_row`. Claude's candidate scan is the same as before.
|
||
|
||
## Completion conditions
|
||
|
||
Pytest used the control-plane venv, `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, and `--basetemp` `/tmp/mamf02-f02af611-pybase` (head) and `/tmp/mamf02-f02af611-pybase-base` (base). Neither path contains `mam-review` or `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`, `MAM_JOBS_DIR`, `AGENT_SESSIONS_YAML`, and the four `XDG_*` homes were dropped. Sandbox tests point `HOME` and `HOME_DIR` at the temporary tree.
|
||
|
||
Head, cwd the snapshot: `tests/test_issue40_auto_pin.py`, `tests/test_issue64_ro_connect.py`, `test_status_and_reconcile_open_the_database_read_only`, `test_status_and_reconcile_read_session_v2`, `test_opencode_shell_and_scripts_integration`, `test_comp_opencode_create_and_reconcile_and_stop`, and `test_comp_codex_create_and_reconcile_and_stop`. 76 passed in 130.63 s. Exit 0. No skip and no error.
|
||
|
||
Base proof is a throwaway `git clone --shared` checked out at `ff8b381`, with `tests/test_issue40_auto_pin.py` and `tests/test_issue64_ro_connect.py` taken from `feccfd5`. The snapshot was not checked out. Result: 15 failed, 13 errors in 19.80 s. Exit 1.
|
||
|
||
1. Another workspace's agy id is not recorded. On base, `test_agy_verify_rejects_id_cached_for_another_workspace` got `True` from `verify_session_uuid`. `test_reconcile_does_not_pin_another_workspaces_agy_conversation` found workspace B's id written on A's row. `test_unpinned_agy_reconcile_leaves_other_workspace_clean_db_without_sidecars` still asserted the old `True`. On head all three pass, and the directory stays the one database file.
|
||
2. The first completed job pins the session's own id with `MAM_NO_SUBSCRIBER_DAEMON=1`. On base, `test_completed_onboarding_pins_agy_without_daemon` errors because `_holder_pids` is absent, the three `test_completed_first_job_pins_opencode_codex_hermes` cases cannot import `autopin`, `test_reconcile_pins_agy_owned_by_the_pane` leaves the id empty, `delegate_autopin_completed` is not a command, and `test_create_onboard_pins_without_daemon` finds no `autopin.json`. On head, agy pins the id the cache maps to A's cwd when A's pane holds it, and opencode, codex, and hermes pin the session in their own workspace. The lib.sh helper, the loop recovery, and `create --onboard` do the same.
|
||
3. One pin per job, and ambiguity pins nothing. On base, `test_wrapper_and_loop_recovery_pin_once` and `test_ambiguous_candidates_leave_reason_without_pin` cannot import `autopin`, and `test_loop_recovery_pins_when_the_wrapper_did_not` leaves the id empty. On head the wrapper call and the loop call share one marker, the yaml mtime does not move the second time, and two opencode sessions in one cwd leave `autopin ambiguous (job amb): 2 candidates` with the own id empty.
|
||
4. Pre-assigned rows and rows that already have an id stay as they were. On base, `test_preassigned_and_existing_ids_are_untouched` errors in the owner fixture, and `test_call_sites_and_publisher_boundary` does not find the wrapper call. On head the claude, grok, already-pinned agy, and already-set opencode rows compare equal before and after, and remote, human, error, and still-running jobs return `skipped` without a marker. A stale `--expect-dispatch-token` also writes no marker.
|
||
|
||
## Remaining risk
|
||
|
||
Agy will not pin from the cache unless `lsof` shows the presence lock or the database held by the pane process or a descendant. No holder, a missing `lsof`, or a pane pid of 0 or 1 is `unverifiable-owner`. That result is stored on the job marker, so the loop recovery does not try the same job again. Reconcile does not read the marker and will try on a later cycle. A worktree creator has no monitor, so a completed turn whose files are already closed stays unpinned until a later completed job. The tests stub `lsof` and `ps`; a live agy process was not used.
|
||
|
||
`claimed_elsewhere` treats every own id on every other row, including a stopped row, as taken. The old drift C list was the live same-cwd siblings. An id that still sits on a stopped row is not given to a new row.
|
||
|
||
Codex drift C now uses `discover()` plus `verify_artifact`, so the epoch check is the rollout file mtime. The removed SQL compared the state-db timestamp. Hermes and opencode still filter cwd and epoch in `discover()`, and opencode still requires `parent_id IS NULL`.
|
||
|
||
The resume dry-run and the viewport check run while `atomic_dump_yaml` holds the registry lock, which is the same arrangement the old `_pin_and_verify_resume` used. `resume_session.sh --dry-run` returns before `update_yaml_resumed.sh` when herdr already has the session. `verify_tui_viewport` reads the pane and does not take that lock.
|
||
|
||
A `mode=ro` open that already has both sidecars can still rewrite `-shm` bytes, and the immutable open can miss a wal created after the exists check. Those are the Issue #64 residuals. This commit removes the reconcile path that opened every agy database.
|
||
|
||
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 `ff8b381520ae04e80c73a9e18bfad73d47e853c9`. 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]
|