docs(reports): archive v4.1.2 implementation plan and unanimous peer review reports
- Add plan-c107763c (Rev 1 & Rev 2 with creator critique and refinements) - Add report-b11cf3b2 (planner-reviewer-claude-01 PASS) - Add report-c9796d8c (reviewer-creator-grok-01 PASS) - Add report-8e6b346e (reviewer-opencode-01 PASS)
This commit is contained in:
@@ -0,0 +1,150 @@
|
||||
# 📐 Implementation Plan — Job `c107763c`: v4.1.2 Upstream Bug-Fix Triage
|
||||
|
||||
- **Planner**: `planner-reviewer-claude-01`
|
||||
- **Source**: `bug-report/mam_upstream_bug_fix_report.md` (untracked, GAIA Canary Project Team, 2026-08-30)
|
||||
- **Method**: each of the 4 reported issues was independently re-derived against live source (`lib.sh`, `opencode.py`, adapter base class, callers, existing tests) before accepting or rejecting the report's diagnosis — per standing practice, a field report's own diff is a claim, not evidence.
|
||||
|
||||
---
|
||||
|
||||
## Verdict per issue
|
||||
|
||||
| # | Report's claim | My finding | Action |
|
||||
|---|---|---|---|
|
||||
| 1 | `_resolve_herdr_pane_id` generic `agent_kind` fallback misattributes an unrelated same-kind pane | **Confirmed real, unfixed.** | Fix applied |
|
||||
| 2 | `opencode.py::resume_spec` `--session` flag missing due to `materialized` gate | **Not a defect — working as designed.** | No fix (see rationale) |
|
||||
| 3 | `opencode.py::ready_tokens` doesn't match the ASCII banner | **Already fixed in v4.1.1** (job `c38ddfcf`, this exact string is live). | No action, stale report entry |
|
||||
| 4 | `new-session` doesn't reuse an idle pane before splitting | **Confirmed real, unfixed.** | Fix applied |
|
||||
|
||||
---
|
||||
|
||||
## Issue 1: `_resolve_herdr_pane_id` generic `agent_kind` fallback (`lib.sh` ~line 436)
|
||||
|
||||
**Root cause, confirmed by reading the fallback chain in full (`lib.sh:349-450`):** when name/label/agent-exact matching against `herdr pane list` fails, the code looks up the *target's own* kind via `.mam/agent-sessions.yaml` (`agent_kind = s.pane.cmd` for the yaml row whose `name` equals `tn`/`tsa`), then searches **all live panes** for `agent == agent_kind`. If exactly one such pane exists anywhere in the (already workspace-scoped) pane list, it is returned — even though that pane's actual name/label has nothing to do with the target. A second, genuinely same-named agent of the same kind running concurrently (e.g. an orchestrator `agy` pane alongside a dead `creator-agy-01`) gets misattributed as `creator-agy-01`, defeating `has-session`'s liveness check and silently skipping restoration.
|
||||
|
||||
**Verified this is reachable in practice**: `has-session`'s shim arm (`lib.sh:590`) falls through to `_resolve_herdr_pane_id "$sess"` exactly on this path, and `resume_session.sh:73` calls `herdr has-session -t "$SESSION_NAME"` with a full role-prefixed name — precisely the target-name-plus-drifted-kind-match scenario in the report.
|
||||
|
||||
**Fix**: gate the kind-based fallback so it only fires when the caller's *own* search term already equals the bare kind — i.e. this path is for a caller who searched by kind directly (`target == "agy"`), not for silently broadening a specific-name search into a kind-wide one:
|
||||
|
||||
```diff
|
||||
- if agent_kind:
|
||||
+ if agent_kind and (tn == agent_kind or tsa == agent_kind):
|
||||
matching = [p.get('pane_id') for p in panes if p.get('agent') == agent_kind and p.get('pane_id')]
|
||||
```
|
||||
|
||||
**Why this doesn't regress anything**: grepped every real call site of `_resolve_herdr_pane_id` in `lib.sh` (`has-session`, `kill-session`, `capture-pane`, `send-keys`, `set-buffer`) — all pass a full role-prefixed session name (`"$sess"`), never a bare kind string. So in real usage `tn`/`tsa` never equals `agent_kind`, and this fallback branch — which was unsound in every real-world configuration with more than one live agent of the same kind — becomes fully inert rather than merely narrowed. The only existing test touching this path, `test_r1_resolve_pane_id_fail_closed_on_multiple_same_kind_panes` (`tests/test_c1_tui_readiness.py`), asserts the fail-closed (RC=1) outcome on ambiguous multi-match, which still holds unchanged (the branch is skipped, `pid` stays empty, function still returns 1). No test exercises the previous len==1 "success" path, confirming it was never a documented contract.
|
||||
|
||||
## Issue 2: `opencode.py::resume_spec` — **rejected the proposed fix**
|
||||
|
||||
The report proposes removing the `materialized` gate:
|
||||
```diff
|
||||
- if materialized and session_uuid:
|
||||
+ if session_uuid:
|
||||
```
|
||||
|
||||
**Why this is not a genuine bug**: `if materialized and session_uuid:` is the *identical* pattern in all 5 adapters (`claude.py:97`, `grok.py:105`, `agy.py:90`, `hermes.py:88`, `opencode.py:189`) — this is a deliberate, cross-adapter contract, not an opencode-specific oversight. `materialized` is computed by `__main__.py`'s `resume-spec` CLI as `adapter.verify_artifact(uuid, ctx)` — a real, adapter-specific check (for opencode: a live sqlite query against the local session DB, including epoch and cwd/workspace scoping — see `test_opencode_verify_artifact_created_at_epoch_filtering` in `test_a4_adapter_contract.py`). Its purpose is to avoid ever passing `--session <uuid>` for a session that does not actually exist locally, which would risk the CLI erroring or hanging on an invalid id.
|
||||
|
||||
Traced the actual caller, `resume_session.sh:99`: `WORKSPACE` is a required flag (validated at the top of the script), so `DiscoveryContext` is always constructed with a real workspace — `materialized` is never spuriously `False` merely because the workspace argument was missing. I could not reproduce a false-negative in `verify_artifact` itself (DB path resolution, column detection, and `workspace_key()`'s `os.path.realpath()`-based cwd comparison all look sound on inspection, and are covered by existing epoch/cwd tests). Applying the report's fix would (a) break the 5-way adapter consistency this contract relies on, and (b) reintroduce exactly the "resume with an unverified/possibly-nonexistent session id" risk the gate exists to prevent — for no evidence-backed benefit. **No change made.** If the underlying symptom (occasional failure to resume an opencode conversation) recurs, the next diagnostic step is to log `verify_artifact`'s db path / query result for the specific failing session, not to remove the gate.
|
||||
|
||||
## Issue 3: `opencode.py::ready_tokens` — already resolved
|
||||
|
||||
Live value at `opencode.py:24` is already `'OpenCode|Chat|Ask anything|tab agents|ctrl\\+p|Build auto|commands'` — this is Fix 1 from the v4.1.1 release (job `c38ddfcf`, verified by 3 independent reviewers in that round). The bug report's own "already fixed" diff target matches current source byte-for-byte. This entry in the report is stale (written against a pre-v4.1.1 checkout). No action needed.
|
||||
|
||||
## Issue 4: `new-session` idle-pane reuse (`lib.sh` ~line 683)
|
||||
|
||||
**Confirmed real**: read the full `new-session` arm. When `existing_ws` is detected (matching cwd), the code unconditionally proceeds to `pane layout` + `pane split` — there is no check anywhere in the current code for a pre-existing, agent-less pane in that workspace. Every `new-session` into an existing workspace therefore grows the pane count monotonically, even if a prior agent's pane is sitting idle, causing pane proliferation and the reported grid-layout churn.
|
||||
|
||||
**Fix**: before the existing sample-pane/layout/split logic, query `herdr pane list` (workspace-scoped, client-side filtered — consistent with the existing `sample_pane` lookup two lines below) for a pane with no `agent` assigned; if found, reuse its `pane_id` as `target_pane` directly and skip the split entirely:
|
||||
|
||||
```bash
|
||||
idle_pane=$(_real_herdr pane list 2>/dev/null | TARGET_WS="$existing_ws" python3 -c "
|
||||
...
|
||||
for p in panes:
|
||||
if p.get('workspace_id') == target_ws and not p.get('agent') and p.get('pane_id'):
|
||||
print(p.get('pane_id')); break
|
||||
...
|
||||
" 2>/dev/null || echo "")
|
||||
if [ -n "$idle_pane" ]; then
|
||||
target_pane="$idle_pane"
|
||||
else
|
||||
# existing sample_pane / layout / split logic, unchanged
|
||||
fi
|
||||
```
|
||||
|
||||
The existing overflow-threshold-triggers-fresh-workspace branch (`split_dir == "overflow"` → `existing_ws=""`) is preserved unchanged inside the `else`, so idle-pane reuse never bypasses the grid-overflow policy — it only short-circuits when a reusable pane already exists, before that policy is even consulted.
|
||||
|
||||
## Tests added
|
||||
|
||||
New tests exercising the two fixes, following each file's existing conventions:
|
||||
|
||||
- `tests/test_c1_tui_readiness.py`: `test_r2_resolve_pane_id_does_not_misattribute_unrelated_same_kind_pane` — seeds a yaml entry for a target session whose own pane is absent, plus exactly one *unrelated* same-kind live pane; asserts resolution now fails closed (RC=1) instead of returning the unrelated pane's id (the exact regression the report describes).
|
||||
- `tests/test_herdr_shim_contract.py`: `test_h24_new_session_reuses_idle_pane_without_split` — seeds a raw idle pane (no `agent` key) in the target workspace/cwd via the `mock_herdr` state, calls `herdr new-session`, and asserts (a) the new agent's `pane_id` equals the idle pane's id and (b) no `pane split` call was recorded.
|
||||
|
||||
## SemVer
|
||||
|
||||
v4.1.1 → **v4.1.2, PATCH** (SemVer 2.0.0 §6): both applied fixes are internal reliability hardening of already-shipped behavior (pane resolution correctness, pane-reuse efficiency) — no public `--agent` value, flag, or YAML own-key added/removed/changed. Matches the v4.1.1 and v3.0.1 PATCH precedents. 3-way lockstep: `lib.sh:32` `MAM_VERSION`, `VERSIONS.md` (header/date/prose/8-row matrix/new changelog section), 8× `SKILL.md` `version:` frontmatter.
|
||||
|
||||
## Self-caught regression during implementation
|
||||
|
||||
While running the targeted suite after implementing both fixes, 3 previously-passing tests regressed (`test_h16_send_keys_safe_submits_exactly_once`, `test_h18_workspace_scoped_pane_resolution`, `test_h23_persisted_workspace_id_scopes_without_env` — none touching `new-session` or the `agent_kind` fallback directly). Traced it to the Bug-1 fix's own explanatory comment: it used a literal double-quoted phrase (`"any pane of this kind"`) placed *inside* a bash **double**-quoted `python3 -c "..."` block. Bash's own string-termination rules apply inside that block (it's a quoted bash argument, not a heredoc), so the embedded `"` characters prematurely closed the bash string and corrupted the generated Python source for the entire pane-resolution helper — breaking label/name/agent exact-matching too, not just the kind fallback the comment was documenting. Rewrote the comment without embedded double quotes; re-ran the full targeted and full suites clean. Recorded here because it's a instructive shell-scripting trap specific to this codebase's pattern of embedding Python inside double-quoted bash strings (existing code consistently uses single quotes for all embedded Python string literals for exactly this reason — my new comment just hadn't followed that convention).
|
||||
|
||||
## Test results
|
||||
|
||||
- Targeted suite (`test_a4_adapter_contract.py`, `test_c1_tui_readiness.py`, `test_b19_headless_reconcile_fixes.py`, `test_herdr_shim_contract.py`, `test_version_consistency.py`, `test_tier2_component.py`): **121 passed**.
|
||||
- Full suite (`tests/`): **449 passed** (447 pre-existing + 2 new regression tests for Bug-1 and Bug-4), 0 failed, 10m21s.
|
||||
|
||||
## Review
|
||||
|
||||
Per the established MAM workflow, multi-agent peer review (Claude, Grok, OpenCode) is conducted via separately delegated reviewer jobs against this committed diff, not performed by the Planner in this job.
|
||||
|
||||
---
|
||||
|
||||
## Rev. 2 — Refinement in response to `creator-agy-01`'s architectural challenge (job `d14ab345` → `769e0a1c`)
|
||||
|
||||
`creator-agy-01` filed a challenge (`.mam/jobs/d14ab345/agy-reports/report-final.md`) against this plan's Issue-4 fix, plus a secondary observation on Issue-1. Verified both independently against the current live `lib.sh` (still uncommitted working tree, `HEAD=a18f9f8`) rather than accepting the challenge's diagnosis at face value — same standing practice as the original triage. **Both findings are confirmed real; one supporting example in the challenge is corrected below.**
|
||||
|
||||
### Finding A (primary): idle-pane reuse drops caller-supplied `KEY=VAL` env tokens — CONFIRMED, fix required
|
||||
|
||||
Verified directly: `agent start` (`lib.sh:832,834`) passes only `--kind`, `--pane`, and `-- $final_cmd` — confirmed against the H-1 contract test's own whitelist (`{"--kind", "--pane", "--timeout"}`) and forbidden set (`--env` explicitly forbidden on `agent start` under the live 0.8.0 herdr contract). `$env_flags` (parsed from leading `KEY=VAL` tokens in `run_cmd`, `lib.sh:630-644`) is consumed **only** by `pane split` (`lib.sh:756,770`) and `workspace create` (`lib.sh:785`, plus the extra `--env HERDR_WORKSPACE_ID=...` both add). My idle-pane-reuse branch (`lib.sh:729-731`) sets `target_pane="$idle_pane"` and skips both — `$env_flags` genuinely has nowhere to go on that path. This is a real regression my Bug-4 fix introduced: pre-fix, `new-session` into an existing workspace *always* went through `pane split` (with `$env_flags`), so no gap existed.
|
||||
|
||||
**One correction to the challenge's own supporting evidence**: the report's example (`OPENCODE_PERMISSION='{"*":"allow"}'` being lost) does not currently apply — `create_session.sh:217-218` `export`s `OPENCODE_PERMISSION` into its *own* calling shell rather than embedding it as a `KEY=VAL` prefix inside `CMD_FULL` (checked all 3 real `new-session` callers — `create_session.sh`, `resume_session.sh`, `reconcile.sh` — none currently emit a `KEY=VAL`-prefixed command string). So this specific variable was never routed through `$env_flags` even before Bug-4, on any path. That doesn't weaken the finding, though: the `new-session` shim command has a *documented, general-purpose* contract of accepting `KEY=VAL cmd` and threading it through as pane env (the parsing exists precisely for this), and any caller that does use that documented form — current or future — silently loses it exactly as described the moment the target pane happens to be idle-reusable. The defect is in the command's contract, not tied to one specific caller.
|
||||
|
||||
**Refined fix** — replay `$env_flags` via a `env(1)` prefix on `$final_cmd` when the idle-pane path is taken, mirroring what `pane split` would have injected server-side (including the same extra `HERDR_WORKSPACE_ID` the split path adds — needed because agents spawned inside the pane may themselves call MAM tooling that reads this var):
|
||||
|
||||
```bash
|
||||
if [ -n "$idle_pane" ]; then
|
||||
target_pane="$idle_pane"
|
||||
if [ -n "$final_cmd" ]; then
|
||||
idle_env="${env_flags//--env /}"
|
||||
idle_env="${idle_env:+$idle_env }HERDR_WORKSPACE_ID=$existing_ws"
|
||||
final_cmd="env $idle_env $final_cmd"
|
||||
fi
|
||||
else
|
||||
# existing sample_pane / layout / split logic, unchanged
|
||||
fi
|
||||
```
|
||||
|
||||
`${env_flags//--env /}` turns `"--env K1=V1 --env K2=V2"` into `"K1=V1 K2=V2"` via plain bash parameter expansion — no new subprocess, no new quoting model, deliberately matching the existing code's own level of robustness (the split/create paths already word-split `$env_flags` unquoted; a value containing a literal space was already fragile there pre-Bug-4, and fixing that broader pre-existing quoting limitation is out of scope for this PATCH). `env` receives literal `KEY=VAL` argv tokens and execs `$final_cmd` with them set — this is the client-side POSIX equivalent of what herdr's own `--env` flag does server-side, so behavior stays consistent whether a pane was split or reused.
|
||||
|
||||
**New test to add during implementation**: seed a `new-session` call whose `run_cmd` includes a leading `KEY=VAL` token *and* an idle pane in the target workspace; assert the resulting `agent start` call's trailing command (or the mock's recorded `command`/env-visible field) reflects the env var, and that no `pane split` call occurred. Follows the same fail-old/pass-new verification discipline as `test_h24`.
|
||||
|
||||
### Finding B (secondary): the pre-existing exact-match loop's `'agent'` key preempts Bug-1's R-1 guard for bare-kind targets — CONFIRMED, fix required
|
||||
|
||||
Traced precisely as the challenge describes: the exact-match loop (`lib.sh:415-422`, pre-existing "ISSUE-2" logic, not part of my Bug-1 diff) iterates `for key in ('label', 'name', 'agent')` and returns on the **first** pane whose `agent` field equals `tn`/`tsa`. If a caller ever does search by a bare kind (`target == "agy"` — the exact scenario my Bug-1 gate's `tn == agent_kind` condition exists to protect, deliberately, per the original plan's own design), this loop matches on `key='agent'` *before* Bug-1's block is even reached, and returns the first same-kind pane with **no ambiguity check** — silently bypassing the R-1 fail-closed guard I added specifically for that case. Currently harmless in practice (verified again: every real call site still passes a full role-prefixed name, never a bare kind — grepped fresh, unchanged from the original triage), but it makes the R-1 protection in Bug-1's own fallback block dead code for the one scenario it was written to cover, which defeats the purpose of writing it.
|
||||
|
||||
Checked for test fallout: no existing test seeds a pane whose `agent` field is set to a value equal to the *search target string itself* (`test_r1`/`test_r2` both use full names like `"my-unmatched-session"`/`"creator-agy-01"` as targets, never bare `"agy"`/`"claude"`/`"grok"`) — confirmed via `grep` across `test_c1_tui_readiness.py` and `test_herdr_shim_contract.py`. Removing `'agent'` from this tuple is safe against the current suite.
|
||||
|
||||
**Refined fix**:
|
||||
```diff
|
||||
- for key in ('label', 'name', 'agent'):
|
||||
+ for key in ('label', 'name'):
|
||||
```
|
||||
This funnels *all* kind-based resolution — whether derived via the yaml lookup (full-name case) or a literal bare-kind search — exclusively through Bug-1's R-1-guarded block, so the multi-match fail-closed check can never be bypassed regardless of which form the target takes.
|
||||
|
||||
**New test to add during implementation**: bare-kind target (`_resolve_herdr_pane_id "agy"`) against 2 panes both with `agent == "agy"`; assert fail-closed (RC=1), proving R-1 now actually governs this path instead of being preempted.
|
||||
|
||||
### Updated action list for re-implementation
|
||||
|
||||
1. Apply the `env` prefix fix to the idle-pane branch (Finding A).
|
||||
2. Remove `'agent'` from the exact-match tuple (Finding B).
|
||||
3. Add the two new tests above; re-run the full targeted + full suite (expect 451 passed: 449 + 2).
|
||||
4. No change to the Issue-2/Issue-3 verdicts or the SemVer classification (still v4.1.2 PATCH) — both new fixes are the same class of internal reliability hardening, not a public-surface change.
|
||||
Reference in New Issue
Block a user