GodopuandClaude Opus 5.5
44999ff2fb
Issue #42 stages 1-2: the SKILL.md code-block inventory as test data and the removal of blocks the entry scripts already do. Merge-workspace review: round 1 (@8d26106) claude agy grok PASS, codex NOT PASS (R1-R3); fixed in17eba09(docs, inventory data and the guard test only); round 2 4/4 PASS. Full suite at8d261061408 passed / 0 failed. Reports and the round-2 identity sidecars go to .agents/reports/<session>/. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VCgu9Fa9iT7TbJUSzdq4Ff
67 lines
7.1 KiB
Markdown
67 lines
7.1 KiB
Markdown
# Review: PR #46 round 2 @ 17eba09
|
||
|
||
- Role: Reviewer (grok)
|
||
- Identity: base `8d26106195a0a24621f140e76389d206332f84c0`, head `17eba0904ebbf54c1d2243ad61a6f2ba17263889`, tree `a0f1ffede44be7c86267a56b25871e1d081feafb`, 1 commit, clean worktree
|
||
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261003T230658Z-e7866a/1`. Main stayed at `45d1621`. No product file was edited.
|
||
- `git diff --stat 8d26106..HEAD`: 5 files, +93/−19. `git diff --check` is clean.
|
||
|
||
The commit is the three skill docs (create, resume, stop), `tests/data/skill_code_blocks.json`, and `tests/test_issue42_skill_blocks.py`. No script, no Python product module, no version line. Frontmatter stays `4.8.2`.
|
||
|
||
Round 1 on `8d26106` was PASS from this reviewer. Codex R1–R3 are the items this commit answers. They are closed.
|
||
|
||
## R1 — attach names the server
|
||
|
||
`resume_session.sh` is run as a child (`SKILL.md:60`). Its `export HERDR_SESSION_NAME` (`resume_session.sh:137`) ends with that process. Native `herdr --help` documents `--session <name>` as "Use or create a named persistent session" and lists one env var, `HERDR_CONFIG_PATH`. It does not list `HERDR_SESSION_NAME`. `herdr agent attach --help` is `herdr agent attach <TARGET>`. A fake `herdr` on `PATH` recorded `herdr --session probe-srv agent attach probe-name` as those five arguments, and `HERDR_SESSION_NAME=probe-srv herdr agent attach probe-name` as three arguments with no `--session`. The shim that does inject the variable is `lib.sh:_real_herdr` (`255-268`), and only after `lib.sh` is sourced: an explicit `--session` is passed through, otherwise a non-default `HERDR_SESSION_NAME` is prepended.
|
||
|
||
The skill now says that in Case 3 (`SKILL.md:30`), the workflow paragraph (`:63`), and When NOT (`:139`): `herdr --session <server> agent attach <name>`, with `<server>` the row's `herdr_session`. The verification fence reads that field and passes it:
|
||
|
||
```bash
|
||
SERVER=$(python3 -c "... [0].get('herdr_session') or 'default')")
|
||
herdr --session "$SERVER" agent get "$SESSION_NAME"
|
||
herdr --session "$SERVER" agent read "$SESSION_NAME" --source visible --lines 30
|
||
```
|
||
|
||
(`SKILL.md:106-126`). Against temp YAML, a row with `herdr_session: isolated` prints `isolated`. After a successful resume, `update_yaml_resumed.sh:154-156` writes `herdr_session` (and `herdr_server`) from the server the script used, so this post-resume read is the server the resume ran on.
|
||
|
||
## R2 — `--dry-run` is not "write nothing"
|
||
|
||
Both option rows now say: no spawn and no registry (YAML/DB) update; the checks before that exit still run; on macOS they clear the agent binary's quarantine attribute.
|
||
|
||
- create: `xattr -d com.apple.quarantine` is at `create_session.sh:194-196`, after preflight and before the dry-run exit at `280-282`. `spawn` is called at `288`, so dry-run neither spawns nor reaches the YAML write.
|
||
- resume: the same `xattr` is at `resume_session.sh:167-169`. The dry-run that prints the command exits at `225-227`, before `_herdr new-session` (`237`) and before `update_yaml_resumed.sh` (`247`). The Darwin test is `uname` = `Darwin` and the path must be a file.
|
||
|
||
## R3 — a deleted body stays deleted
|
||
|
||
`fingerprint` (`test_issue42_skill_blocks.py:132-134`) is sha256 of the body with all whitespace collapsed, first 12 hex digits. `test_no_deleted_body_came_back` (`150-153`) compares every current fence with `DATA["removed"]` and `DATA["superseded"]` by exact `base_sha` or that fingerprint. It does not read `DATA["blocks"]`, so listing the returned body as class c does not hide it. `test_a_returned_body_is_caught_reindented_and_with_the_inventory_updated` (`156-166`) checks both: the exact sha hits, and a reindented body with a dummy sha still hits.
|
||
|
||
A different example does not match. The fingerprint is the whole fence, not a substring, so an ordinary edit of another example changes the words and stays off the list. The only edit that fails this test is putting a deleted body back, including reindented. The inventory sha lock still requires a JSON update when any fence's text changes; that is the other test, and it is unchanged.
|
||
|
||
`test_superseded_blocks_are_recorded_and_replaced` records the folded create call. Its `base_sha` `ceb96a8c1293` and `base_fingerprint` `b6a8a25279c5` match the fence at `45d1621` create `SKILL.md:263-266` (the `create_session.sh --workspace --agent --role --session` call). The replacement `bash .agents/skills/multi-agent-mux-create/scripts/create_session.sh` is contained in the Workflow fence. Count: 33 current + 5 removed + 1 superseded = 39, and 39 − 2 fences that were not in the original 37 = 37. Current class counts stay 28 c and 5 b.
|
||
|
||
## Inventory and the other doc lines
|
||
|
||
Pairing each block with the `8d26106` inventory by skill, note, class, and kind is equal. One span moved: resume verification `103-124` → `103-128`, still class `b`, kind `verification`. No class or kind flipped.
|
||
|
||
create exit 42 now includes the live process with `MAM_KILL_ON_READY_TIMEOUT=1` (`create/SKILL.md:179`). That is the `else` at `create_session.sh:392-398`: alive and the knob unset preserves the session; otherwise exit 42 and the trap rolls it back.
|
||
|
||
stop exit 0 now says the already-`stopped` no-op does not apply to `--purge-conversation` (`stop/SKILL.md:61`). The script no-ops only when purge is off (`stop_session.sh:184-188`). With purge and without `--yes` it exits 3 (`192-198`).
|
||
|
||
Codex C2 and claude O2/O3 are named in the brief as follow-ups and are not in this diff. The exit tables still match the scanners that landed in round 1; this commit does not change those scanners.
|
||
|
||
## Checks
|
||
|
||
Snapshot, MQTT variables unset, no `.mam.env` exported, interpreter `/Users/godopu16/PuKi/laa/canary_projects/multi-agent-mux-wt/pr-46/.venv/bin/python`, `--basetemp` under `/tmp/c564follow.*` (removed after the run):
|
||
|
||
`132 passed, 1 skipped in 293.02s` — `tests/test_issue42_skill_blocks.py`, `tests/test_tier2_component.py`, `tests/test_orc_onboard.py`, `tests/test_loop_cli.py`, `tests/test_o1_rebuttal.py`, `tests/test_version_consistency.py`.
|
||
|
||
The skip is `test_claude_skills_link_resolves_to_agents_skills`: `.claude/skills` is absent in this worktree snapshot.
|
||
|
||
## Notes that do not change the verdict
|
||
|
||
- No matching row: the SERVER one-liner raises `IndexError` and prints nothing. In a shell without `set -e`, `SERVER` is empty and the next command is `herdr --session ""`. The block is the check after a resume that wrote the row.
|
||
- `herdr_session` missing, empty, or null prints `default`, including when the row has `herdr_server: isolated`. `resolve_herdr_session` (`lib.sh:1601-1604`) would use that alias, then the workspace `mam-<slug>`. A successful resume has already written `herdr_session` (`update_yaml_resumed.sh:154-156`), which is the case this fence is for.
|
||
- Resume's already-running dry-run exits at `resume_session.sh:148-150`, before the quarantine `xattr`. The option row describes the path that resolves the command and exits at `225-227`.
|
||
- `stop/SKILL.md:38` and `:70` still say an already-`stopped` row is always exit 0. The table at `:61` and the script at `184-188` keep the purge exception. The state machine at `:84` labels the re-stop edge "stop default".
|
||
|
||
[VERDICT: PASS]
|