multi-agent-mux/.agents/reports/reviewer-cline-01/report-75c06a1e.md

🔎 Cross Code Review — Job 75c06a1e

• Reviewer: cline (session: herdr:reviewer-cline-01)
• Job ID: 75c06a1e
• Subject: CLI Option Redesign Proposal for multi-agent-mux-loop (ANALYSIS ONLY — no production code touched)
• Artifact under review: .agents/reports/cli_redesign_opinion.md (new untracked file, 232 lines, 9948 B)
• Cumulative change set: git status → only ?? .agents/reports/cli_redesign_opinion.md; git diff --stat empty. (The Grok work from the prior job 6be7c4c2 is no longer in the working tree — committed/reset between jobs — so it is OUT of this review scope.)
• Mode: ANALYSIS-ONLY per brief — production files must NOT be modified; this review judges the proposal document, not applied code.

---

1. Scope, Methodology & Verification Performed

1.1 What was checked

Axis	Status	Note
Lint	N/A (trivially clean)	Artifact is Markdown prose + illustrative bash blocks; no production shell/Python changed. Markdown is well-formed (7 sections, fenced code, tables). No lint tool applies to an untracked .md proposal.
Operability	No runtime impact	Zero production code modified → system behavior unchanged. Findings below are proposal/blueprint-completeness advisories for a future Creator, NOT breakage in the live repo.
Loss	None	No existing functionality removed or weakened at the repo level (nothing applied).

1.2 Evidence gathered

• Read full cli_redesign_opinion.md (lines 1–232, incl. the truncated middle 51–140 via sed).
• Read actual run_loop.sh: parser (lines 1–120), resolve_planner_session + TARGET validation (295–370).
• Confirmed log_* definitions live at run_loop.sh:141/149/153 — i.e. after the B-13 freeze re-exec (lines 92–112); the NOTE at line 96 explicitly warns log_* are undefined before :114.
• Confirmed load_state_json() (lib.sh:945) reads via env_python "$AGENT_SESSIONS_YAML" → live state, not a direct yaml scan.
• Confirmed --reviewer handler is REVIEWER_LIST="$2"; shift 2 (run_loop.sh:60) — last-value-overwrite, single value.
• Confirmed --reviewer/--all-reviewer interaction is warn-and-precedence, not hard-exit (run_loop.sh:177–181; SKILL.md:169 calls them "mutually exclusive" — spec/doc already diverge from code).
• Confirmed --creator/--planner do NOT currently exist anywhere in run_loop.sh (grep → NONE) → the "introduce" framing is accurate, no name collision.
• Confirmed Phase-2 test target tests/test_tier1_unit.py exists (43789 B).
• Confirmed scope: only the opinion file is untracked; nothing else modified.

---

2. Findings

Severity legend: 🔴 MEDIUM (must address before/at implementation) · 🟡 LOW (advisory/accuracy).

🔴 F1 — Blueprint parser drops existing integer-validation guards (operability regression if followed literally)

Current code validates --plan-talk, --max-loop, --max-rebut as positive integers inside the parser (run_loop.sh:54–73):

--plan-talk)  if [[ ! "$2" =~ ^[0-9]+$ ]]; then echo "ERROR: --plan-talk requires a positive integer."; exit 1; fi; PLAN_TALK_TURNS="$2"; shift 2 ;;
--max-loop)   if [[ ! "$2" =~ ^[0-9]+$ ]] || [ "$2" -le 0 ]; then echo "ERROR: --max-loop requires a positive non-zero integer."; exit 1; fi; MAX_LOOP="$2"; shift 2 ;;
--max-rebut)  if [[ ! "$2" =~ ^[0-9]+$ ]]; then echo "ERROR: --max-rebut requires a non-negative integer."; exit 1; fi; MAX_REBUT="$2"; shift 2 ;;

Proposal blueprint (Section 4) passes them through with no validation:

--plan-talk)  PLAN_TALK_TURNS="$2"; shift 2 ;;
--max-loop)   MAX_LOOP="$2"; shift 2 ;;
--max-rebut)  MAX_REBUT="$2"; shift 2 ;;

A Creator implementing the blueprint by replacement would regress input validation (--max-loop abc would be accepted and later fail with an arithmetic error under set -e). Fix: when editing the existing parser in-place (as Phase 1 intends), preserve the existing guard branches and only insert the new --creator/--planner cases.

🔴 F2 — Spec ↔ blueprint inconsistency: --planner <session> validation is specified (Edge Case 3.4) but NOT coded (Section 4)

Edge Case 3.4 mandates: "Verify session exists and is running. Check if session role contains planner …". Blueprint planner-resolution block does the opposite — unconditional assignment with no check:

if [ -n "$PLANNER_SESSION_OVERRIDE" ]; then
  PLANNER_SESSION="$PLANNER_SESSION_OVERRIDE"   # ← no existence/running/role check
else
  PLANNER_SESSION=$(resolve_planner_session)
fi

This matters because the existing post-freeze guard (run_loop.sh:357–362) only fires on an empty PLANNER_SESSION:

if [ "$PLAN_MODE" = true ]; then
  if [ -z "$PLANNER_SESSION" ]; then
    log_error "Planner mode enabled (--plan) but no running session with a 'planner' role was found."; exit 1
  fi
fi

A bogus non-empty --planner not-a-session (or a reviewer-role session) would pass this guard (it's non-empty) and fail only later at use. There is no equivalent of the TARGET validation block (run_loop.sh:333–352) for the planner override. Fix: add an explicit validate_planner_session() step mirroring the TARGET registration/running check (and the role-soft-warning from 3.4), placed in the post-freeze section next to the existing planner guard.

🔴 F3 — Proposal is unaware of the B-13 freeze re-exec ordering; log_error used before its definition

run_loop.sh re-execs itself from a frozen snapshot between the parser and the post-freeze code (B-13, lines 92–112). The log_* helpers are defined after that freeze (line 141/149/153). The NOTE at line 96 states this explicitly: "log_ are not defined until :114 — use echo here, not log_warn (C2)."* That is why the current mandatory-args check uses raw echo (line 88), not log_error. The proposal's blueprint places log_error "Missing required arguments…"; usage immediately after the parser (Section 4, mandatory-args block) — i.e. before the freeze, where log_error is undefined. Under set -euo pipefail (line 6) this would abort with command not found: log_error. The entire B-13 freeze mechanism is absent from the proposal. Fix: keep pre-freeze error emission as echo (as the live code already does), and confine log_error-based validation to the post-freeze section. The proposal should add a one-line note that all log_* calls in its blueprint are post-freeze.

🟡 F4 — "100% backward-compatible" overclaim w.r.t. --reviewer multi-flag semantics

The proposal (Executive Summary §1, Key Benefit 4; Conclusion §7) claims a "Zero-Breaking-Change Guarantee / 100% backward compatibility." However Edge Case 3.5 + the blueprint change --reviewer from last-value-overwrite to append-on-repeat:

• Current: --reviewer) REVIEWER_LIST="$2"; shift 2 → --reviewer A --reviewer B yields B (last wins).
• Proposed: if [ -n "$REVIEWER_LIST" ]; then REVIEWER_LIST="${REVIEWER_LIST},$2"; … → yields A,B (accumulate). For the common single-occurrence call this is identical, but for the (admittedly rare) repeat-occurrence form the semantics change. Fix: soften the "100%" claim to "backward-compatible for all documented single-use invocations; --reviewer repeat-occurrence semantics change from last-wins to accumulate (intentional enhancement)."

🟡 F5 — --reviewer + --all-reviewer interaction omitted from the edge-case list

Section 3 lists 5 edge cases (creator/target-agent dual spec; planner w/o plan; plan w/o planner; planner validation; reviewer comma vs multi-flag) but not the --reviewer+--all-reviewer conflict. The live code is warn-and-precedence (run_loop.sh:179–181: --all-reviewer takes precedence, the --reviewer list is discarded with a log_warn), while SKILL.md:169 already declares them "mutually exclusive". Since the proposal extends --reviewer to multi-flag append (3.5), the appended list would be silently discarded by the existing precedence logic. A proposal centered on reviewer-tier symmetry should reconcile this (e.g., add Edge Case 6: decide warn-vs-hard-exit for --reviewer + --all-reviewer, and state whether the appended list is preserved or dropped).

🟡 F6 — Minor mechanism imprecision in Edge Case 3.3

3.3 says resolve_planner_session does a "dynamic scan of .mam/agent-sessions.yaml". The actual function (run_loop.sh:306) calls load_state_json() (lib.sh:945, which reads via env_python "$AGENT_SESSIONS_YAML") and filters herdr_sessions for role containing planner and status == running. The spirit is correct; the mechanism (live state JSON, not a direct yaml file scan) is imprecise. Low impact — worth a one-word correction for accuracy so a future implementer greps the right symbol (load_state_json, not agent-sessions.yaml).

Minor / non-issues (noted for completeness)

• REBUT_TOTAL_BUDGET=$((MAX_REBUT * MAX_LOOP)) (run_loop.sh:85), the run-wide rebuttal cap, is absent from the blueprint. It is an internal computed var, not a CLI flag, so not required — but a thorough blueprint should preserve it when refactoring the parser region. Negligible.
• usage() help-text update is correctly deferred to Phase 1 (acknowledged). Fine.
• No --planner/--creator name collision with existing flags or internal vars (PLANNER_SESSION exists at run_loop.sh:354; the blueprint uses PLANNER_SESSION_OVERRIDE, distinct). ✓

---

3. What the Proposal Gets Right

• Flag inventory is accurate. Every current flag (--plan, --plan-talk, --reviewer, --all-reviewer, --max-loop, --max-rebut, --verbose, --cleanup, --target-agent, --task, -h|--help) is correctly enumerated and matched. No phantom flags invented.
• Planner-not-found error message is verbatim-accurate. Edge Case 3.3's ERROR: Planner mode enabled (--plan) but no running session with a 'planner' role was found. matches run_loop.sh:359 exactly.
• Conflict resolution for --creator vs --target-agent (same→accept idempotently; different→exit 1) is sound and avoids silent-precedence bugs.
• --planner implicit PLAN_MODE=true is a clean ergonomic win with no naming collision.
• Symmetry matrix (§2) is conceptually correct and aligns with the 3 MAM pillars (Planner/Creator/Reviewer).
• Migration plan (§6) is reasonable; it correctly defers tests to Phase 2, and the named test file tests/test_tier1_unit.py exists.
• The document is explicitly labeled "Analysis & Proposal (Non-Mutating)" and the working tree confirms no production files were touched — the ANALYSIS-ONLY constraint was honored.

---

4. Cross-Axis Summary

Axis	Result
Lint	N/A — doc-only; Markdown well-formed; no production code linted.
Operability	No runtime impact — zero production code changed. The 🔴 findings are blueprint-completeness advisories for a future Creator, not live breakage.
Loss	None — no existing functionality removed or weakened at the repo level.
Spec/Doc integrity	Proposal is internally mostly consistent; 🔴 F2 (spec↔blueprint) and 🟡 F4/F5 (overclaim + missing edge case) are the notable gaps.

---

5. Recommendation

The document is a sound, non-mutating analysis/proposal and is acceptable as a foundation for a future implementation cycle. None of the findings require design-level rework or replanning — every 🔴 item is an implementation-detail refinement that the proposal's own Phase 1 (parser update) / Phase 2 (tests) plan can absorb:

• F1 → preserve validation guards when editing the parser in-place.
• F2 → add a validate_planner_session() step mirroring the existing TARGET validation.
• F3 → keep pre-freeze errors as echo; note that all log_* calls are post-freeze.
• F4/F5/F6 → soften the 100% claim, add a --reviewer+--all-reviewer edge case, correct the load_state_json reference.

Because this is analysis-only and the repository is not affected, there is nothing to break and no replanning is warranted.

[VERDICT: PASS]