Godopu
25de01eaad
- Wrap inject_instructions with safe || rc=0 under set -e to prevent premature exit - Ensure terminal error events are published on instruction injection failure - Update 2-reviewers final review reports with Round 3 PASS verdicts
86 lines
6.1 KiB
Markdown
86 lines
6.1 KiB
Markdown
# Prompt-Lock Fix — Final Review (Creator Claude)
|
|
|
|
- **Reviewer**: Creator Claude (`canary-projects-multi-agent-mux-creator-claude`)
|
|
- **Brief**: `.mam/reports/brief-rereview-prompt-lock.md`
|
|
- **Round 1** (2026-07-11, commit `e613f4a`): ❌ FAIL — full findings preserved in git history (this file as committed in `da895fc`).
|
|
- **Round 2** (2026-07-11, commit `da895fc`): ❌ **FAIL — one single-line blocker remains** (F5, new in the fix commit). Everything else is verified fixed.
|
|
- **Round 3** (2026-07-11, working tree on top of `da895fc`): ✅ **PASS** — see below.
|
|
|
|
---
|
|
|
|
## Round 3 Verdict: ✅ PASS (working-tree state; commit required)
|
|
|
|
The F5 fix is applied in the working tree of `create_session.sh:387-388` **byte-identical to the prescribed replacement**:
|
|
|
|
```bash
|
|
rc=0
|
|
inject_instructions "$SESSION_NAME" "$instructions" "$DELEGATE_JOB_ID" || rc=$?
|
|
```
|
|
|
|
- Idiom correctness under `set -euo pipefail` was already proven empirically in round 2 (P2: event published with true `rc=4`, EXIT trap fires, script exits 1). The `||` form suppresses `set -e` for the command, so injection failures now reach `delegate_publish_event error` — MS-7's no-zombie-jobs contract holds on the failure path.
|
|
- `bash -n` passes; shellcheck `-S warning`: **0 findings**.
|
|
- Diff scope verified: the only code change versus `da895fc` is this 4-line block; all other working-tree changes are review reports.
|
|
- lib.sh is unchanged since round 2, where the full functional suite passed 5/5 against the committed helpers (T-A3…T-E3: dialog refusal rc=2 / flood rc=1 / happy-path rc=0 / instant banner / one-Enter trust acceptance).
|
|
|
|
**Conditions attached to this PASS:**
|
|
1. The fix is **uncommitted** — it must be committed for the verdict to bind to a ref (suggested: `fix(create): make injection-failure error event survive set -e (|| rc=$?)`). Include the pending review reports (this file, Reviewer Cline's staged modification and new v2 report) per the durable-reports convention.
|
|
2. **DoD-5 follow-up** (non-blocking, reaffirmed): capture-validate the dialog signature tokens for agy/hermes/cline in the field; claude tokens match known real CLI text and unmatched tokens now fail loud, not silent.
|
|
3. Update FW-W2's resolution commit reference once the fix commit exists.
|
|
|
|
---
|
|
|
|
## Round 2 Verdict: ❌ FAIL (NOT PASS) — F5 only
|
|
|
|
### ✅ F1 (blank-padded viewport windows) — VERIFIED FIXED
|
|
`da895fc` applies the prescribed `_pane_tail()` helper verbatim (lib.sh:1117) and rewires all three windowing sites (`_pane_dialog_open`, `send_keys_safe` submit-verify, `handle_startup_dialogs`). Re-ran the full functional suite against the **committed** code on an isolated scratch server (`tmux -L sks-review`):
|
|
|
|
| Test | Scenario | Expected | Result |
|
|
|---|---|---|---|
|
|
| T-A3 | dialog mock, `SKS_DIALOG_TIMEOUT=6` | rc=2, zero paste leakage | ✅ rc=2, 0 occurrences in pane |
|
|
| T-B3 | perpetually flooding pane | rc=1 (quiescence gate) | ✅ rc=1 |
|
|
| T-C3 | happy-path mock prompt TUI | rc=0, line received | ✅ rc=0, `RECEIVED-OK len=41` |
|
|
| T-D3 | ready banner on screen | fast return 0 | ✅ rc=0 in 0 s |
|
|
| T-E3 | trust dialog then banner | exactly one Enter, ready detected | ✅ rc=0 in 2 s, banner reached |
|
|
|
|
`bash -n` passes; shellcheck `-S warning` on lib.sh: 0 findings.
|
|
|
|
### ❌ F5 — NEW BLOCKER: the F3 fix regressed error-event publication under `set -e`
|
|
`create_session.sh` runs under `set -euo pipefail` (line 20). The new form (lines 387-392):
|
|
|
|
```bash
|
|
inject_instructions "$SESSION_NAME" "$instructions" "$DELEGATE_JOB_ID"
|
|
rc=$?
|
|
if [ "$rc" -ne 0 ]; then
|
|
delegate_publish_event "$DELEGATE_JOB_ID" error "instruction injection failed (prompt-lock, rc=$rc)"
|
|
```
|
|
|
|
Under `set -e`, a **bare failing command aborts the script immediately** — `rc=$?` and the `delegate_publish_event error` line are never reached. Proven empirically:
|
|
|
|
```
|
|
set -euo pipefail; f(){ return 4; }; trap "echo TRAP-FIRED" EXIT
|
|
f; rc=$?; echo "EVENT-PUBLISHED rc=$rc" → output: TRAP-FIRED only, exit 4
|
|
rc=0; f || rc=$?; if [ "$rc" -ne 0 ]; ... → output: EVENT-PUBLISHED rc=4, TRAP-FIRED, exit 1
|
|
```
|
|
|
|
Consequence on injection failure: the EXIT trap still rolls back the session and isolation home, but **no terminal `error` event is ever published** — the delegator waits for watchdog timeout. That is precisely the zombie-job outcome MS-7 exists to prevent, so the fix traded round 1's cosmetic `rc=0` misreport (F3) for a functional regression on the same path. Ironically the round-1 code *did* publish the event.
|
|
|
|
**Required fix (verified above, one line):**
|
|
```bash
|
|
rc=0
|
|
inject_instructions "$SESSION_NAME" "$instructions" "$DELEGATE_JOB_ID" || rc=$?
|
|
if [ "$rc" -ne 0 ]; then
|
|
```
|
|
|
|
Scope confirmed limited to this one site: the delegate-job wrapper uses the `if ! send_keys_safe …` guard form and `stop_session.sh` uses `send_keys_safe … || echo …` — both are `set -e`-safe and report correct rc.
|
|
|
|
### Remaining non-blocking items
|
|
1. **DoD-5 (real-TUI token validation)** — still no recorded capture evidence. Partially mitigated: the claude tokens (`Do you trust the files`, `Yes, proceed`/`No, exit`) match the real Claude Code CLI dialog text, and with fail-loud semantics an unmatched token now degrades to a loud rc≠0 + rollback rather than a silent lock. Recommendation to Planner: accept with a follow-up task to capture-validate agy/hermes/cline dialog text in the field, rather than blocking on it again.
|
|
2. **Working-tree hygiene**: `.agents/reports/canary-projects-multi-agent-mux-reviewer-cline/report-prompt-lock-analysis.md` has an uncommitted modification — a committed audit record edited in place (not by me). Commit or revert it deliberately alongside the F5 fix.
|
|
3. FW-W2 stays legitimately marked resolved once F5 lands; update its commit reference then.
|
|
|
|
---
|
|
|
|
## Summary for the Planner
|
|
|
|
The hard problem is solved and proven: dialog gating, quiescence, banner detection, and trust-dialog acceptance all behave correctly on the committed helpers (5/5 functional tests). What remains is a one-line `set -e` idiom fix in `create_session.sh` (`|| rc=$?`) so injection failures publish their terminal error event — the empirical proof and exact replacement are above. Fix that line, decide the stray report edit, and round 3 is a rubber stamp.
|