GodopuandClaude Opus 5.5
50c8da8269
Review loop (--review-only, --all-reviewer): round 1 (@325b056) 4/4 PASS,
round 2 (@db7f4ca, HERDR_SESSION) 4/4 PASS. Full suite at 325b056 on
macOS / Python 3.9 / bash 3.2 / herdr 0.9.3 from an agent pane:
1845 passed / 0 failed / 56 skipped (18 test_deploy_freshness skips: no
nats-docker submodule in the fresh worktree); the panes of the real
servers (default, multi-agent-mux) and ~/.claude/projects were unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
52 lines
6.7 KiB
Markdown
52 lines
6.7 KiB
Markdown
# Review 191f0ec3 — a test run from a pane no longer opens workspaces on that pane's server
|
||
|
||
Head `325b056057fcdd717d2c81017fc5ef57183cb4d0` (tree `7905896f005159d596e3872d0ee2c3c12061f09a`) is two commits on base `8b0b4a07dc76a4915895bdde6170dc8eed9b5f74`: `525df2b` (the leak fix) and `325b056` (the generated-shim pin moves to `525df2b`). Four files, +106/−5. `git diff --check` is clean. Snapshot porcelain empty. Control-plane main is still `8b0b4a07dc76a4915895bdde6170dc8eed9b5f74`, porcelain empty.
|
||
|
||
Both measured causes are right, and the fix matches them.
|
||
|
||
The suite that left seven workspaces on `multi-agent-mux` ran with `--basetemp .../pytest-pr56-shim`. `mock_herdr` puts its `bin/herdr` under that basetemp. Both resolvers used to skip any PATH directory whose path contained `-shim` anywhere, so they walked past the mock and executed `/opt/homebrew/bin/herdr`. The real binary follows `HERDR_SOCKET_PATH` ahead of `HOME`, and `mam_sandbox` left the pane's socket in the environment.
|
||
|
||
`_is_shim_path` (`lib.sh:113-125`) still matches `/.mam/shim/`, `/multi-agent-herdr-shim/`, and `/.agents/skills/.bin`. The substring `*-shim/*` is gone. A second `case` matches only when the path with one trailing slash stripped is itself `*-shim`. `_resolve_real_herdr_path` (`:127-143`) skips a PATH directory on that test, then skips the candidate when `_is_shim_path` matches `dirname` of the canonical file (`:136`). It still prints the PATH entry, `$dir/herdr`. The generated shim's `_resolve_real_herdr` (`:172-186`) now requires `${dir%/}` to fail the pattern `*-shim`, and it still rejects a directory containing `.mam/shim`.
|
||
|
||
`mam_sandbox` deletes `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, and `HERDR_TAB_ID` (`tests/conftest.py:296-298`).
|
||
|
||
## Wrapper directories
|
||
|
||
A decoy-only probe sourced this snapshot's `lib.sh` with `WORKSPACE_ROOT=/tmp/m191ws`. PATH was the case directory, a stand-in, `/usr/bin`, and `/bin`. The real binary was not on it.
|
||
|
||
| PATH directory | `_resolve_real_herdr_path` | generated shim |
|
||
|---|---|---|
|
||
| `.../pytest-x-shim/case0/bin` | that mock | that mock |
|
||
| a directory whose own name is `herdr-shim` | stand-in | stand-in |
|
||
| `.../multi-agent-herdr-shim/srv` | stand-in | that directory |
|
||
| `.../.agents/skills/.bin` | stand-in | that directory |
|
||
| `.../.mam/shim/extra` | stand-in | stand-in |
|
||
|
||
An ancestor named `pytest-x-shim` above `bin/` is selected by both resolvers. A directory that is itself `*-shim` is skipped by both. `.mam/shim` and a child of it are skipped by both. L1 and L1b assert the same two outcomes inside the suite, with a decoy behind the mock, and both passed.
|
||
|
||
`lib.sh` still excludes `/multi-agent-herdr-shim/` and `/.agents/skills/.bin`. The generated shim excludes a PATH entry whose own name ends in `-shim`, so the directory `multi-agent-herdr-shim` itself is skipped by both. A child of that directory, whose own name does not end in `-shim`, is excluded only by `_resolve_real_herdr_path`. `README.md:102` and `README.ko.md:84` still describe `/tmp/multi-agent-herdr-shim/<HERDR_SERVER_NAME>/herdr`. No script in this tree creates that path; `_init_herdr_isolation` writes `$WORKSPACE_ROOT/.mam/shim/herdr`. The `.agents/skills/.bin` gap on the generated shim is older than this commit: the previous generated check was the substring `*"-shim"*`, and `.bin` never matched it. Nothing in `lib.sh` prepends `.agents/skills/.bin` onto PATH.
|
||
|
||
## What still reaches the real binary
|
||
|
||
A direct call of `mam_sandbox`, with probe values set first, left `HERDR_SESSION` and `HERDR_ENV` set and removed `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, `HERDR_TAB_ID`, `HERDR_SESSION_NAME`, `HERDR_WORKSPACE_ID`, and `HERDR_CONFIG_PATH`. `HOME` was the sandbox. L2's `PANE_VARS` (`tests/test_herdr_real_server_leak.py:74-75`) are the socket, the pane and tab ids, the workspace id, and `HERDR_SESSION_NAME`. The test passed. It does not include `HERDR_SESSION`.
|
||
|
||
This reviewer process inherited `HERDR_SESSION` from its pane. `lib.sh:1411-1415` records that herdr 0.8.2 left `HERDR_SESSION` unset on a default-server pane and exported the socket to every pane. `docs/contracts/issue-6/implementation_plan.md` records the measurement that the real binary follows `HERDR_SESSION`, `HERDR_SOCKET_PATH`, and `--session`, and that `HERDR_SESSION_NAME` alone reaches the default socket. The calls that created the seven workspaces go through these resolvers, which now return the mock, so they do not reach the binary that reads `HERDR_SESSION`. A later call that misses the mock still inherits `HERDR_SESSION`.
|
||
|
||
The suite's intentional real-server tests already drop it. `tests/test_issue45_restore.py` lists `HERDR_SESSION` in `SCOPE_ENV` and builds the server env from what remains. `tests/test_issue50_dispatch_herdr.py` `Box.env` drops every `HERDR_*` name, and `Box.herdr` passes only `HOME`, `PATH`, and `--session`. `tests/test_herdr_shim_contract.py::test_h10_real_herdr_schema_match` calls `herdr api schema` with the inherited environment and does not use `mam_sandbox`. That test was already in the tree. I did not run it, and I did not run the real binary. `api schema` is a read. The workspace-creating path was the resolver.
|
||
|
||
`HERDR_ENV` also survived the fixture call. The measured contract names `HERDR_SESSION`, `HERDR_SOCKET_PATH`, and `--session`. I did not treat `HERDR_ENV` as a second route.
|
||
|
||
## Shared control plane
|
||
|
||
`test_s11_this_branch_does_not_change_the_generated_shim` (`tests/test_issue23_24_startup_path.py:294-315`) compares the shim heredoc with `git show 525df2b:.agents/skills/lib.sh`, unless `MAM_BASE_SHA` overrides it. It passed, so the heredoc at `325b056` is the heredoc at `525df2b`. The second commit changes the pin and the comment. Sourcing `lib.sh` rewrites `$WORKSPACE_ROOT/.mam/shim/herdr`. This review sourced it with `WORKSPACE_ROOT` pointed at `/tmp/m191ws` and at pytest sandboxes. The narrowing becomes the control plane's shim when this branch is merged and something sources `lib.sh` with that workspace root.
|
||
|
||
## What was run
|
||
|
||
Control-plane venv, Python 3.9.6, pytest 8.4.2, `PYTHONDONTWRITEBYTECODE=1`, `-p no:cacheprovider`, `--basetemp=/tmp/m191bt-leak2` (the path contains neither `shim` nor `mam-review`). Inherited `HERDR_SOCKET_PATH`, `HERDR_PANE_ID`, `HERDR_TAB_ID`, `HERDR_WORKSPACE_ID`, `HERDR_SESSION_NAME`, `HERDR_SESSION`, `HERDR_SERVER_NAME`, `HERDR_CONFIG_PATH`, `HERDR_ENV`, `WORKSPACE_ROOT`, `MAM_ENV_FILE`, `HOME_DIR`, `CLAUDE_PROJECT_DIR`, and `CODEX_HOME` were unset.
|
||
|
||
`tests/test_herdr_real_server_leak.py` and `tests/test_issue23_24_startup_path.py::test_s11_this_branch_does_not_change_the_generated_shim`: 4 passed in 1.50s. The session guards that travel with that run completed. The #55 agent-home guard did not fire.
|
||
|
||
User `~/.config/herdr/config.toml` mtime stayed `1784456394`. Snapshot porcelain stayed empty after the run.
|
||
|
||
[VERDICT: PASS]
|