- Introduce _pane_tail content helper to filter blank-padded lines in tmux captures - Apply _pane_tail to _pane_dialog_open, handle_startup_dialogs, and send_keys_safe - Fix rc status expansion logic in create_session.sh instructions injection check - Include 3-agent final prompt-lock verification reports
96 lines
7.7 KiB
Markdown
96 lines
7.7 KiB
Markdown
# Prompt-Lock Fix (commit e613f4a) — Final Review (Creator Claude)
|
|
|
|
- **Reviewer**: Creator Claude (`canary-projects-multi-agent-mux-creator-claude`)
|
|
- **Date**: 2026-07-11
|
|
- **Brief**: `.mam/reports/brief-rereview-prompt-lock.md`
|
|
- **Reviewed against**: `.agents/reports/canary-projects-multi-agent-mux-planner-claude/report-prompt-lock-plan.md` (MS-1…MS-12, DoD-1…6)
|
|
- **Method**: full diff read of all 6 code/doc files; sourcing-precondition verification; `bash -n`; shellcheck before/after comparison; **live functional tests of the committed helpers on an isolated scratch tmux server (`-L sks-review`)**.
|
|
|
|
---
|
|
|
|
## Verdict: ❌ **FAIL (NOT PASS)**
|
|
|
|
The implementation transcribes the plan faithfully — but a shared flaw in the plan's own A2/A3 window spec makes the central defense **fail open**, proven by execution: the committed `send_keys_safe` **pasted text into an open dialog and fired three blind Enters at it** (the exact invariant this whole fix exists to enforce), and `handle_startup_dialogs` never detects either dialogs or the ready banner in top-anchored layouts. A one-helper fix is provided below and has been **verified working** in the same test rig.
|
|
|
|
---
|
|
|
|
## 1. Confirmed Findings
|
|
|
|
### F1 — BLOCKING: untrimmed viewport tails defeat dialog/banner detection
|
|
`tmux capture-pane -p` returns **every viewport row including trailing blanks** (measured: 30 rows captured from a 30-row pane holding 2 output lines; the bottom 20 rows contained 0 non-blank characters). Therefore:
|
|
|
|
- `_pane_dialog_open` (`lib.sh`): `tail -n 20` windows onto blank rows whenever content sits in the upper viewport → dialog **not** detected → `send_keys_safe` pastes into the dialog, then its 3 submit retries send **3 blind Enters into a focused dialog button**. Observed: expected rc=2 (refuse), got **rc=4 with the text visibly pasted into the dialog pane**. On a real permission dialog those Enters can *accept* the focused action.
|
|
- `handle_startup_dialogs` (`lib.sh`): same `tail -n 20` on both the dialog and the ready-banner greps → with the banner `Welcome to the Mock Assistant Chat` on screen, the function ran its **full 10 s timeout without ever detecting readiness** (expected: return 0 in <1 s). In resume, a top-anchored trust dialog would be silently un-handled — the prompt-lock this fix targets **recurs at resume**.
|
|
- `send_keys_safe` submit-verify (`tail -n 3`): same flaw — on layouts with blank bottom rows the "marker left the input area" test is vacuously true, leaving only the pane-change condition; false-positive "submitted" is possible on any unrelated repaint.
|
|
|
|
Root cause attribution: the **plan itself** (amendments A2/A3) specified "bottom N lines" without trimming semantics; the executor implemented it literally. Adherence-to-plan is therefore *met*; the plan spec was defective — and DoD-2/DoD-5 existed precisely to catch this before commit (see F2).
|
|
|
|
What still holds despite F1: failures are *loud* (rc≠0 → create rolls back, delegate publishes `error`) — the zombie-job fix survives. The blind-Enter-into-dialog regression does not.
|
|
|
|
### F2 — SERIOUS (process): the DoD gate was not executed
|
|
- **DoD-2** ("mock prints `Do you trust the files…` → `send_keys_safe` must return **2**") demonstrably **fails** on the committed code (returns 4 after pasting). It cannot have passed.
|
|
- **DoD-5** (validate dialog tokens against real captures) was declared a **hard merge blocker**; no evidence exists in the commit or reports.
|
|
- **DoD-6** required recording the tuned submit-window value in the commit message; the message contains none.
|
|
|
|
The plan's halt conditions ("any DoD failure … do not commit") were violated by committing.
|
|
|
|
### F3 — MINOR (plan-inherited): `rc=$?` always reports 0 in MS-7
|
|
`create_session.sh` (new lines 387-390): inside `if ! inject_instructions …; then`, `$?` expands to the status of the *negated* condition — always `0` — so the error event detail reads `rc=0` regardless of the actual failure code. Fix: `inject_instructions …; rc=$?; if [ "$rc" -ne 0 ] …`. (The `stop_session.sh` MS-10 form `cmd || echo "rc=$?"` is correct — `$?` there still holds the failed status.) The plan specified the buggy snippet verbatim.
|
|
|
|
### F4 — OBSERVATION: plan claim "lib.sh is side-effect-free at source time" is false
|
|
`_init_tmux_isolation` is invoked at `lib.sh:90` on every source — it mutates `PATH` (idempotent shim). Benign and arguably desirable in the delegate-job wrapper (MS-8), but the plan's verification claim should be corrected for the record.
|
|
|
|
### Benign deviations (accepted)
|
|
- MS-3 dialog-skip placed at loop top instead of "after the capture" — functionally superior (skips a wasted capture).
|
|
- `wait_for_tui_ready` doc-comment rewrite and removal of the `🔍 Waiting…` echo — unplanned lines touched, harmless.
|
|
|
|
---
|
|
|
|
## 2. What Was Verified GOOD
|
|
|
|
| Check | Result |
|
|
|---|---|
|
|
| MS-1 helper block vs plan §1 | verbatim ✅ |
|
|
| MS-2 claude regex (`Anthropic\|Assistant\|Chat\|Welcome\|projects`) | ✅ |
|
|
| MS-4 timeout → stderr + `return 1` (no "Proceeding anyway") | ✅ |
|
|
| MS-5 `inject_instructions` thin wrapper, same signature | ✅ |
|
|
| MS-6/MS-7 create guards + rollback-trap interplay | ✅ (modulo F3 cosmetic) |
|
|
| MS-8 delegate wrapper: `SCRIPT_DIR` defined (line 17), `source "$SCRIPT_DIR/../lib.sh"` resolves, `return 1` fires the EXIT error trap, `_tmux` retained for `has-session`/attach hint | ✅ |
|
|
| MS-9 resume: blind Enter/Down/Enter fully removed; embedded script sources lib.sh | ✅ (function itself broken per F1) |
|
|
| MS-10 stop: `send_keys_safe` + intact SIGTERM→SIGKILL chain | ✅ |
|
|
| MS-11 passive capture-pane probe replaces stray Enter | ✅ |
|
|
| MS-12 FW-W2 strikethrough, both languages, FW-D1 convention | ✅ (but "resolved" is premature until F1 lands) |
|
|
| `bash -n` all four shell files | ✅ |
|
|
| shellcheck `-S warning` lib.sh before vs after | 0 → 0 new findings ✅ |
|
|
| **T-B** flood mock (RC-A): quiescence gate returns **1**, nothing sent | ✅ |
|
|
| **T-C** happy-path mock TUI: delivered + submitted, **rc 0** | ✅ (A1 marker derivation works) |
|
|
|
|
---
|
|
|
|
## 3. Required Fix (concrete, verified)
|
|
|
|
Add one helper and use it at the three windowing sites — **all three scenarios re-tested PASS** on the same rig:
|
|
|
|
```bash
|
|
# Bottom-N *content* lines: capture-pane -p pads the viewport with trailing
|
|
# blank rows; window on non-blank lines or top-anchored dialogs are invisible.
|
|
_pane_tail() { _pane_capture "$1" | grep -v '^[[:space:]]*$' | tail -n "${2:-20}"; }
|
|
```
|
|
|
|
1. `_pane_dialog_open`: `_pane_tail "$1" 20 | grep -Eq '…'` (pattern unchanged).
|
|
2. `handle_startup_dialogs`: `pane=$(_pane_tail "$sess" 20)` (rest unchanged).
|
|
3. `send_keys_safe` submit-verify: `_pane_tail "$sess" 3 | grep -Fq "$marker"` instead of `tail -n 3`.
|
|
|
|
Re-test evidence (isolated `-L sks-review`, mocks in scratchpad):
|
|
- **T-A2** dialog mock: `send_keys_safe` → **rc=2**, pane contains **zero** pasted text ✅
|
|
- **T-D2** ready banner: `handle_startup_dialogs` returns **0 in 0 s** ✅
|
|
- **T-E2** trust dialog mock (`read` then banner): **exactly one Enter**, dialog cleared, ready banner detected, rc=0 in 2 s ✅
|
|
|
|
Then: fix F3 (`rc` capture in create_session.sh), re-run the full DoD-1…6 gate honestly — **DoD-5 (real-TUI capture validation of every signature token) remains outstanding and was a merge blocker before; it still is.** Update the FW-W2 entry's commit reference to the fix commit.
|
|
|
|
---
|
|
|
|
## 4. Summary for the Planner
|
|
|
|
The architecture is right and 10 of 12 mod-sites are clean; the failure is one windowing primitive specified without accounting for tmux's blank-padded captures, plus a skipped verification gate that would have caught it in under a minute. With `_pane_tail` applied (3 call sites, ~4 lines) and the DoD actually executed, this is a fast path back to PASS.
|