75 lines
8.0 KiB
Markdown
75 lines
8.0 KiB
Markdown
# 🔍 Cross-Review: Agent-Creation Reliability Fixes & v4.1.1 PATCH Release (Job 89fd2305)
|
||
|
||
- **Reviewer**: `reviewer-opencode-01` (role: reviewer)
|
||
- **Target**: The 4 fixes documented in `mam-agent-creation-fix-report.md` and the SemVer v4.1.1 release — committed as `eb8057b` (bump) + `a93c32c` (SemVer section-reference fixup), now on `main`. Brief correctly states "(no changes since base commit)" — the work is landed, not in-flight; this is a review of commits `6de1535..a93c32c` (2 commits) plus the fix report doc itself.
|
||
- **Method**: Every fix verified in live source (adapter, shim template, generated `.mam/shim/herdr`, real `herdr` CLI output), not from the field report or plan at face value. Own runs: lockstep test, targeted contract tests, full 447-test suite, syntax checks. Also verified all 3 sibling peer-review jobs in the registry.
|
||
|
||
---
|
||
|
||
## 0. My Verification Runs
|
||
|
||
| Check | Result |
|
||
|---|---|
|
||
| `bash -n lib.sh`, `py_compile opencode.py` | OK |
|
||
| `pytest tests/test_version_consistency.py` | **2 passed** — 3-way lockstep holds at `4.1.1` (`lib.sh:32` `MAM_VERSION="4.1.1"`, 8× `SKILL.md` `version: 4.1.1`, `VERSIONS.md` header `v4.1.1` + date 2026-08-30 + prose + 8 matrix rows) |
|
||
| `pytest tests/test_a4_adapter_contract.py` | **25 passed** — includes the updated `ready_tokens` contract string |
|
||
| `pytest tests/test_c1_tui_readiness.py::test_adapter_strong_weak_partition` | **1 passed** — S/W set algebra holds |
|
||
| Full suite `pytest tests/ -q` | **447 passed / 0 failed** (679s, my own run) |
|
||
| Working tree | Clean — zero diff vs `a93c32c`; no incidental drift |
|
||
| Generated shim `.mam/shim/herdr` | Contains all new code (PYTHONPATH injection at lines 10–12, timeout fallback, label lookup) — template regeneration propagated correctly |
|
||
|
||
## 1. Fix-by-Fix Verification
|
||
|
||
### Fix 1 — OpenCode `ready_tokens` (opencode.py:23) — VERIFIED
|
||
- Live value: `'OpenCode|Chat|Ask anything|tab agents|ctrl\+p|Build auto|commands'`. The `\\+` in the Python literal produces `ctrl\+p` at runtime, which `grep -E` in `wait_for_tui_ready` (lib.sh:1953) reads as a literal `+` — correct escaping; an unescaped `+` would be an ERE quantifier and never match the on-screen hint.
|
||
- I am the direct beneficiary and living proof of this fix: my session (`reviewer-opencode-01`) creation previously stalled on the ASCII-banner mismatch; the new tokens (`Ask anything`, `Build auto`, `tab agents`, `commands`) are the actual TUI surface I render on every startup. Facts bridge emits all 7 tokens as strong (`MAM_STRONG_READY_TOKENS`), `weak=''` — the `S ∨ (W ∧ C)` decision correctly degrades to plain S, same as agy/hermes/grok.
|
||
- Lockstep test update in `test_a4_adapter_contract.py:77` matches the new string exactly.
|
||
|
||
### Fix 2 — Shim `PYTHONPATH` injection (lib.sh:162–164, inside the `<<'EOF'` heredoc) — VERIFIED
|
||
- Injected immediately after `set -euo pipefail`; single-quoted heredoc means `${BASH_SOURCE[0]}` is copied verbatim and self-resolves at shim runtime. Path algebra: `$WORKSPACE_ROOT/.mam/shim/../../.agents/skills` = `$WORKSPACE_ROOT/.agents/skills` — correct.
|
||
- Confirmed present in the *generated* `.mam/shim/herdr` (lines 10–12), not just the template — existing workspaces pick it up on regeneration.
|
||
- Prepend-not-replace semantics (`"$_SKILL_DIR:${PYTHONPATH:-}"`) and silent no-op on missing path — safe under `set -euo pipefail`.
|
||
|
||
### Fix 3 — `agent start` timeout liveness re-query + send-text fallback (lib.sh ~824–846) — VERIFIED
|
||
- The branch fires only on `timed out waiting for agent startup`; it does **not** promote that string to success (the pre-existing safety comment and `test_agent_start_success_tokens_exclude_startup_timeout` remain intact — verified passing).
|
||
- Liveness ground truth is `_herdr_agent_get_scoped` (herdr's own registry), not the ambiguous text; the fallback (`pane send-text` + Enter, sleep 1, re-query) mirrors the existing paste-buffer pattern. Other CLI errors still `exit 1`. `$target_pane` is allocated and non-empty-checked before the retry loop.
|
||
|
||
### Fix 4 — `_resolve_herdr_pane_id` fast-path + label resolution (lib.sh ~351–384) — VERIFIED
|
||
- Fast-path regex `^w[A-Za-z0-9]+:p[A-Za-z0-9]+$` returns immediately on real pane ids (alphanumeric-workspace safe, consistent with H-20's regex family).
|
||
- Label translation: real `herdr workspace list` on this machine emits `{"workspace_id":"w2G","label":"mam-agent",...}` — I confirmed the live schema uses `label`, and the lookup matches `w.get('label')`. The `|| echo "$target_ws"` fallback pattern matches `_herdr_agent_get_scoped`'s established `set -e`-safe idiom.
|
||
- **Docs nit (agrees with Grok's finding)**: the field report (§2.2.③) and `VERSIONS.md` B-4 say "`label` **및** `workspace_label`" both are matched, but the code matches only `label`. Since live herdr and the mock both expose `label`, this is docs over-tightness, not a functional gap — the lookup works against real schema.
|
||
|
||
## 2. SemVer v4.1.1 (PATCH) — Correct
|
||
|
||
- No public surface changed: no `--agent` value, flag, YAML own-key, or documented contract added/removed. All 4 fixes are internal reliability hardening of code shipped in v4.1.0. SemVer 2.0.0 PATCH (§6) is the correct class; matches the v3.0.1 (shim routing) precedent.
|
||
- `a93c32c` correctly fixed the changelog's initial "§8 PATCH" mislabel to "§6 PATCH" — caught and fixed within 6 minutes of the bump commit.
|
||
- Lockstep complete and verified (see §0); v4.1.0's changelog section is preserved (no historical 유실).
|
||
|
||
## 3. Consensus Verification (job registry)
|
||
|
||
All sibling peer reviews for this release are real and registry-verifiable:
|
||
|
||
| Reviewer | Job | Verdict | Notes |
|
||
|---|---|---|---|
|
||
| `creator-agy-01` (lead) | `50cb5ac0` | PASS | 110 targeted tests, 4/4 fixes verified |
|
||
| `reviewer-creator-grok-01` | `31a6733b` | PASS | 78 targeted tests, independent source-level re-derivation |
|
||
| `planner-reviewer-claude-01` | `b74ced56` | running (started after mine) | pending at time of my report |
|
||
|
||
Planner's implementation plan (`plan-c38ddfcf.md`, committed in `eb8057b`) documents the design rationale for each fix, including why Fix 3 doesn't reintroduce the bug the old comment warned about, and the pre-landing correction of a double-emit fallback draft issue — verifiable against the committed code.
|
||
|
||
## 4. Residual Nits (non-blocking; concur with Grok's independent list)
|
||
|
||
- **Generic strong tokens**: `commands`/`Chat` in the strong set can false-trip on incidental text. The distinctive tokens dominate in practice; if false-ready ever appears, demote those two to `weak_ready_tokens`. (As the agent whose TUI is being detected, I note `Build auto` + `Ask anything` are reliably present on every real startup.)
|
||
- **`workspace_label` docs overclaim**: field report + VERSIONS.md B-4 vs code (only `label`) — recommend a one-word docs correction in a future commit; functionally correct against live schema.
|
||
- **No new dedicated tests** for the PYTHONPATH-in-shim, timeout-fallback branch, or label→id translation. Adjacent contracts (D-1, D-4, D-6, H-19, H-20) all still pass, and the full 447 suite is green, but a one-line shim `PYTHONPATH` assertion would have been cheap insurance.
|
||
- **Fast-path accepts foreign pane ids as-is**: correct for the name-resolution fail-closed design (callers resolving from session names is the guarded path); noted for the record.
|
||
|
||
None of these are blocking: they are docs tightness and test-coverage depth on a green, verified PATCH.
|
||
|
||
---
|
||
|
||
## 5. Verdict
|
||
|
||
All 4 fixes are present, correct, and independently re-verified in live source by me — including against the real herdr CLI's JSON schema and my own session's actual TUI surface. The v4.1.1 PATCH classification is correct with complete 3-way lockstep, the changelog accurately describes the changes (post-`a93c32c`), the full test suite passes 447/447 in my own run, and the worker + Grok peer reviews already issued PASS from independent verification chains. Residual items are minor docs/test-coverage nits that do not affect correctness. No lint, functionality, or data-loss issues; no design-level rework; no planner escalation.
|
||
|
||
[VERDICT: PASS] |