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
103 lines
12 KiB
Markdown
103 lines
12 KiB
Markdown
# Review: PR #46 (issue #42 stages 1–2) @ 8d26106
|
||
|
||
- Role: Reviewer (grok)
|
||
- Identity: base `45d1621c93504e6cfa347b31995f70da868f4172`, head `8d26106195a0a24621f140e76389d206332f84c0`, tree `b579380959e08f721492386da904972092bf8d7f`, 3 commits, clean worktree
|
||
- Verified read-only at `/var/folders/q_/68sxgw3j073dgjg8wl9s08vw0000gn/T/mam-review/loop-20261003T222707Z-a0de27/1`. Main stayed at `45d1621`. No product file was edited.
|
||
- `git diff --stat 45d1621..HEAD`: 6 files, +919/−224. `git diff --check` is clean.
|
||
|
||
The range is the four SKILL.md files (create, resume, status, stop), `tests/data/skill_code_blocks.json`, and `tests/test_issue42_skill_blocks.py`. No script, no Python product module, no `VERSION` file, and `tests/test_issue10_13_purge.py` is absent from the diff. Skill frontmatter stays `version: 4.8.2`.
|
||
|
||
## 1. Classification
|
||
|
||
Base `45d1621` has 37 fenced blocks. Head keeps 33 (28 class c, 5 class b) and records 5 removed class (a) blocks. Each removed body was read from `git show 45d1621`. The steps that the scripts actually perform are still done by the entry script; the steps the old fences invented are corrected in the prose that replaced them.
|
||
|
||
- **resume Workflow, base 59–118.** `resolve_session_id.sh` is `resume_session.sh:111-112`. Class A empty UUID sets `FRESH_SPAWN=1` (`agy|hermes|opencode|codex`, lines 123-125); claude/grok exit 1 at line 128. Herdr already up updates the row and exits 0 (`147-158`); there is no `herdr agent attach`. Command resolution is `spawn-spec` or `resume-spec` (`172-197`), then binary checks (`200-210`), `_herdr new-session` (`237`), `handle_startup_dialogs` (`241`), and `update_yaml_resumed.sh` (`247-250`). The deleted fence's `exec herdr agent attach` and trailing attach, and its `$CLAUDE_ID_FLAG` command line, are the mismatches R1–R4. The replacement says the script never attaches and names `herdr agent attach <name>` (`SKILL.md:30`, `:63`, `:135`).
|
||
- **stop pre-flight, base 38–63.** YAML missing is exit 1 (`stop_session.sh:99`). Unregistered session is exit 1 (`140-143`). Already `stopped` and not purging is exit 0 (`184-188`). The fence's `--force-no-yaml` is accepted by no parser arm (`75-90`). The workflow examples that call the entry point stayed (`SKILL.md:42-55`).
|
||
- **create pre-flight, base 35–52.** The script checks the selected agent: workspace directory, `has_real_herdr`, CLI on `PATH`, then that agent's auth (`create_session.sh:111-156`). The fence checked both `claude` and `agy`. The `has_real_herdr` versus `command -v` pitfall remains as prose (`SKILL.md:33`).
|
||
- **create Workflow, base 168–220.** Existing herdr name is exit 3 (`166-168`), the command comes from `spawn-spec` (`207-217`), spawn is `259-276`, TUI wait exits 42 (`361-398`), pane capture is `410-412`. The fence's exit 1, `sleep`, and hard-coded `claude` with no flags are C1–C3; the new summary and exit table use the script's codes.
|
||
- **create YAML append, base 226–252.** `atomic_dump_yaml` writes the row (`491-610`), including `pane.cmd_full`, `attach_command`, and `kill_command` (`532-537`). The lib.sh shim note sits under Output format (`SKILL.md:147`).
|
||
|
||
The five kept class (b) blocks are procedures the entry scripts do not offer:
|
||
|
||
- create `217-239`: post-create `herdr agent get` / YAML / TUI checks, no evidence script.
|
||
- resume `103-124`: the same kind of post-resume checks.
|
||
- stop `127-145`: post-stop checks.
|
||
- status `30-34`: `has_real_herdr` plus `command -v python3`. `status.sh:22` only refuses a missing YAML. With `MAM_NO_AUTOSTART=1` (`status.sh:13`) a host without herdr is reported dead, so the herdr check is agent-side. Kept.
|
||
- monitor `182-210`: the supervising agent's own loop (`sleep 30`, report only new drifts, hard rules). `reconcile.sh:41` polls every `RECONCILE_POLL_INTERVAL`, default 15, which is a different loop. The block is still agent-side guidance, so class (b) is right. The inventory note M1 treats the two sleeps as one disagreement; that note is loose, and the keep decision is right.
|
||
|
||
## 2. Option and exit tables, and the five corrections
|
||
|
||
Parsers match the tables.
|
||
|
||
- create options `SKILL.md:157-170` are the `case` arms at `create_session.sh:74-93`, including `--session|--name`, `--auto-trust` / `--no-auto-trust`, and the `--isolate` no-op.
|
||
- resume options `SKILL.md:65-73` are `resume_session.sh:35-47`.
|
||
- stop has no separate option table. The three workflow calls pass `--session`, `--agent`, `--reason`, and `--purge-conversation --yes`. The parser (`stop_session.sh:75-90`) also takes `--herdr-session` / `--herdr-workspace` (named in the skill intro) and rejects `--mode` / `--capture-id` / `--graceful` with exit 2 (`84-86`), which the exit-2 row states.
|
||
|
||
Exit rows match the paths, under `set -euo pipefail` in all three scripts.
|
||
|
||
| Skill | Script | What the row says |
|
||
|---|---|---|
|
||
| resume 0 | `150`, `158`, `227` | dry-run, or already running so only the row is updated |
|
||
| resume 1 | `128`, `204`, `209`; `update_yaml_resumed.sh:47` | no saved session for claude/grok, binary missing or not executable, and the YAML helper's "file not found" passes through |
|
||
| resume 2 | `46`, `50-56`, `73`, `77`, `104`, `195`, `221`; helper `:42`, `:46`, `:74` | bad args, cwd mismatch or gone, binding refused, whitespace, and the helper's bad-arg exit passes through |
|
||
| resume other | no rollback trap | a later step's code, including a YAML helper code other than 1 or 2 |
|
||
| stop 0 | `188` (and `--help` `88`) | stopped, or already stopped |
|
||
| stop 1 | `99`, `142`, `SystemExit(1)` at `310` | YAML missing or not registered before the stop; after the kill chain, the row disappeared |
|
||
| stop 2 | `86`, `89`, `95`, `98`, `121` | bad args, retired flags, or `--agent` not resolvable |
|
||
| stop 3 | `198` | purge without `--yes` |
|
||
| stop 4 | `281` | purge aborted, herdr still alive, registry left in place |
|
||
| stop other | passthrough declared at `:310` | a registry-update code other than that `SystemExit(1)` |
|
||
| create 0 | `282`, and the success path after `trap - EXIT` at `709` | created, or dry-run |
|
||
| create 1 | `111-156` before spawn; `636` and `659` after | pre-flight, or brief write / injection rc other than 4 (the EXIT trap rolls the session back) |
|
||
| create 2 | `92`, `104-108`, `234`, `245` | bad args, worktree binding refused, whitespace in a forwarded path |
|
||
| create 3 | `168` | herdr name already exists |
|
||
| create 4 | `SystemExit(4)` at `515` | a `running` row of that name; the trap then rolls the herdr session back |
|
||
| create 42 / 43 | `398` / trap `348` | TUI never ready and the process is gone; rollback kill failed and the session is still running |
|
||
|
||
The five corrections match the scripts.
|
||
|
||
- **Case 3.** Already-running resume updates the row and exits 0 (`resume_session.sh:147-158`). `SKILL.md:30` and `:63` say it does not attach and does not check permission flags.
|
||
- **Purge.** `--purge-conversation` without `--yes` exits 3 (`stop_session.sh:192-198`). `SKILL.md:57` says to pass `--yes` only after the user has confirmed it.
|
||
- **status `--json`.** The example keys (`SKILL.md:74-104`) are the reconcile object (`reconcile.sh:1081-1088`: `timestamp`, `yaml_path`, `herdr_sessions_alive`, `herdr_confirmed`, `drifts`, `actions`) plus `sessions_detail` built at `status.sh:213-236`. `resume_state` is `yes` / `MISSING` / `scan` / `no` (`status.sh:71-77` for claude; the other agents return `yes`, `MISSING`, or `no`).
|
||
- **cmd_full.** `SKILL.md:185` says `pane.cmd_full` is the adapter `spawn-spec`. The claude contract is `claude --dangerously-skip-permissions --session-id <uuid>` (`tests/test_a4_adapter_contract.py:335`), and the script fallback is the same shape (`create_session.sh:210`). The old bare `claude` table is gone.
|
||
- **yaml-edit.** No `agent-sessions-yaml-edit` remains under `.agents/skills`. `SKILL.md:185` says the script writes the row.
|
||
|
||
C6 and R7 are the review fixes already on this head. The create header comment `exit 4` (`create_session.sh:19`) is a comment, so the counter skips it; the real code is `SystemExit(4)` at line 515, and the table lists 4. Stop's `sys.exit(7)` (`stop_session.sh:139`) is inside `python3 -c`, and the lookbehind `(?<![\w.])` does not treat it as a shell `exit`. The post-stop `SystemExit(1)` at line 310 is the exit-1 row's second sentence.
|
||
|
||
## 3. An agent can still do the work from the doc
|
||
|
||
Each removed procedure is one entry-point call plus the option and exit tables. create `SKILL.md:155`, resume `:63`, and stop `:91-98` still name the steps the script runs (name derivation, spawn-spec, dialogs, TUI wait, YAML write, capture, purge, delegate events). Attach stays a separate command. Purge still requires a human `--yes` (`stop/SKILL.md:57`). Onboarding that never receives `started` keeps the session and marks the job `error` (`create_session.sh:675-699`); the skill says so at `SKILL.md:87-94`, and the process still exits 0 because the session was created.
|
||
|
||
## 4. Guard test
|
||
|
||
`tests/test_issue42_skill_blocks.py` fails when a fence's start, end, lang, or sha256[:12] drifts, when a class (a) block is present, when the replacement line `bash .agents/skills/<script>` is missing from a remaining fence, or when the exit table's numeric rows disagree with literal `exit N` plus heredoc `SystemExit(N)`. Comments are skipped (`:75-76`). Heredocs are skipped for shell `exit` and scanned for `SystemExit` (`:68-74`). `sys.exit` does not match (`:54`). `test_literal_exit_rule_on_base_scripts` locks create `{0,1,2,3,42,43}`, stop `{0,1,2,3,4}`, resume `{0,1,2}`.
|
||
|
||
The sha lock is the inventory: an edit inside a fence has to update the JSON, and prose outside a fence does not. `test_evidence_points_into_existing_files` (`:174-179`) checks that the path exists and the line range is inside the file. It does not read the cited lines. The citations checked above do point at the steps they name.
|
||
|
||
`test_comp_docs_stop_examples_pass_agent` still requires at least three `stop_session.sh` examples in the stop skill, each containing `--agent` (`tests/test_tier2_component.py:816-832`). The three workflow calls (`stop/SKILL.md:44-54`) all pass `--agent`.
|
||
|
||
## 5. Scope
|
||
|
||
Product scripts and Python are unchanged. No version bump. `tests/test_version_consistency.py` is in the run below.
|
||
|
||
## 6. The "12 purge failures" claim
|
||
|
||
`tests/test_issue10_13_purge.py` is not in `45d1621..HEAD`. This PR cannot be the source of that file's results. On this macOS host that file has been passing; the 12 failures belong to the Linux run called #28. The full suite was left to the orchestrator.
|
||
|
||
## 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/d50follow.*` (removed after the run):
|
||
|
||
`129 passed, 1 skipped in 294.83s` — `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, which the test skips on purpose (`test_issue42_skill_blocks.py:191-192`).
|
||
|
||
## Notes that do not change the verdict
|
||
|
||
- The create Output format sketch (`SKILL.md:140`) shows `cmd_full: claude --dangerously-skip-permissions` without `--session-id`. The following sentence (`:185`) assigns that field to `spawn-spec`. The status `--json` example includes `--session-id` (`status/SKILL.md:100`). For claude the script also writes a `tui` object and `claude_session_id_own` (`create_session.sh:540-552`); the sketch omits them. The agent is told not to append the row by hand.
|
||
- create exit 42 also runs when the process is still alive and `MAM_KILL_ON_READY_TIMEOUT=1` (`create_session.sh:392-398`). The default branch is the one the table describes: the process is gone.
|
||
- `resume_session.sh:146` still comments "print warning or attach". The body at `147-158` only updates the row. The skill text matches the body. The comment is product code, outside this diff.
|
||
- status `SKILL.md:26` still says the monitor polls every 30s. That sentence is unchanged in this diff. The script default is 15s (`reconcile.sh:41`); the 30s figure is the agent runbook's own sleep (`monitor/SKILL.md:191`).
|
||
|
||
[VERDICT: PASS]
|