docs(layout): add 2xK layout improvement plan and multi-agent review reports
- Add Rev.2 authoritative plan detailing root causes and 2xK decision table - Add Phase 1 & 2 review reports from Claude (planner/reviewer) and Cline (reviewer)
This commit is contained in:
@@ -0,0 +1,103 @@
|
||||
# Code Review — Job `a45784f2`: Layout Engine Implementation (W1–W5)
|
||||
|
||||
- **Reviewer**: cline (session: `herdr:reviewer-cline-01`)
|
||||
- **Subject**: Implement the Layout Engine improvements per approved plan `.agents/reports/layout_engine_improvement_plan.md` (Rev.2) — W1–W5.
|
||||
- **Scope reviewed**: cumulative working-tree diff (`git diff`), focusing on the implementation deliverable (W1–W5). The plan.md document itself was already reviewed and PASSED in the prior job (`a9de9b22`); this review focuses on the **code**.
|
||||
- **Review axes**: lint, operability, loss/omission.
|
||||
|
||||
---
|
||||
|
||||
## 1. Changeset (actual `git diff`, authoritative)
|
||||
|
||||
| File | Change | W# |
|
||||
|---|---|---|
|
||||
| `.agents/skills/lib_py/layout.py` | Refactor `compute_2xk_layout` into shared decision table (`_decide`/`_decide_headless`); add `_full_height_pane`, `_group_columns`, `_area_height`, helper constructors; defaults `max_columns=2`/`max_rows=2` + `is None` guard; CLI `_env_int(default=2)` for `--max-cols`/`--max-rows`. | W1–W4 |
|
||||
| `.agents/skills/lib.sh` (l.~435) | Add `--max-cols "${MAM_MAX_PANE_COLS:-2}" --max-rows "${MAM_MAX_PANE_ROWS:-2}"` to the `python3 -m lib_py.layout` invocation. | W4 |
|
||||
| `.mam.env.example` (l.144–153) | Replace `#default: (unset -> no column cap)` with `#default: 2`; set `# MAM_MAX_PANE_COLS=2`; add `# MAM_MAX_PANE_ROWS=2` block with resize-normalisation warning. | W4 |
|
||||
| `tests/test_layout.py` | Rename/adapt tests to new contract; add `test_cli_max_cols_flag_triggers_overflow`, `test_env_max_cols_applies_without_flag`, `test_lib_sh_passes_max_cols_and_rows`, CLI-default overflow test. | W5 |
|
||||
| `tests/test_tier1_unit.py` | Update `test_layout_default_min_cols_15_in_tier1` (1-pane payloads) and `test_layout_single_workspace_54x23_compact_tiling_tier1` (N=1→right, N=2→down, N=3→down→p2, N=4→overflow/grid_capacity_reached). | W5 |
|
||||
| `tests/test_b19_headless_reconcile_fixes.py` | Adapt `test_bug2` to W2 (N=1 always right; tall-but-narrow→overflow; tall-and-wide→right). | W2/W5 |
|
||||
|
||||
> Note: `resolve_session_id.sh` / `resume_session.sh` (`grok` allowlist) are also in the working tree but belong to the agent-onboarding concern, not W1–W5; reviewed and PASSED in prior job `a9de9b22`, unchanged here.
|
||||
|
||||
---
|
||||
|
||||
## 2. Work-Item Verification
|
||||
|
||||
### W1 — Single decision table (GUI & headless) ✅
|
||||
`_decide` (GUI, geometry-grouped columns) and `_decide_headless` (creation-order occupancy) implement the **same** three-step table:
|
||||
1. open a new column → `right` (`new_column_right`);
|
||||
2. fill the shortest/under-filled column → `down` (`fill_column`);
|
||||
3. capacity reached → `overflow` (`grid_capacity_reached`).
|
||||
|
||||
The two paths differ only in **observation**, never in **decision** — exactly the plan §2 design. Helpers (`_right/_down/_overflow/_pane_id`) are single-responsibility and well-typed; no parity (`n % 2`) logic remains (grep for the old `single_pane_split_down`/`headless_odd_down` reasons returns empty — fully purged).
|
||||
|
||||
### W2 — `single_pane_split_right` ✅
|
||||
N=1 → `right` in both modes:
|
||||
- GUI `_decide` ①: `len(cols) < max_columns` and `_full_height_pane(cols[-1])` is the lone full-height pane → `new_column_right`.
|
||||
- Headless `_decide_headless`: `n(=1) < max_columns(=2)` → `new_column_right`, target `panes[0]`.
|
||||
|
||||
Old `single_pane_split_down` reason string is gone; `test_b19` correctly adapted.
|
||||
|
||||
### W3 — `_full_height_pane` ✅
|
||||
`layout.py:91-97`: returns the pane only when `len(col)==1` and `|height − area_h| ≤ tol(=2)`, else `None`. Used in `_decide` ① to block opening a new column from a half-height pane — preventing the half-height column split the plan calls out (R-2).
|
||||
### W4 — `max_columns=2` / `max_rows=2` triple wiring + env doc ✅ (central risk closed)
|
||||
The plan's pivotal "double omission" (lib.sh never passed `--max-cols`; `_env_int` returned `None`) is **fully resolved** — now four overlapping defaults guarantee a 2×2 cap:
|
||||
1. env `MAM_MAX_PANE_COLS`/`MAM_MAX_PANE_ROWS`;
|
||||
2. shell `${VAR:-2}` fallback at `lib.sh:435`;
|
||||
3. argparse `_env_int(..., default=2)` for `--max-cols`/`--max-rows` (`layout.py:235-236`);
|
||||
4. signature `=2` + `if … is None: … = 2` guard (`layout.py:182-193`).
|
||||
|
||||
`.mam.env.example` updated (the plan's C-5 inconsistency fixed: no more "unset → no column cap"); new `MAM_MAX_PANE_ROWS=2` block with "do not raise until a resize-normalisation pass exists" guidance.
|
||||
|
||||
Verified through **three** integration tests exercising the real CLI entrypoint:
|
||||
- `test_cli_max_cols_flag_triggers_overflow` — `--max-cols 2` reaches `compute_2xk_layout` → `grid_capacity_reached`.
|
||||
- `test_env_max_cols_applies_without_flag` — `MAM_MAX_PANE_COLS=2` honored with **no flag** (the precise "double omission" scenario, now closed).
|
||||
- CLI-default test (no flag, no env) → `grid_capacity_reached`, exercising the argparse `default=2`.
|
||||
- `test_lib_sh_passes_max_cols_and_rows` — content-asserts lib.sh contains the `--max-cols "${MAM_MAX_PANE_COLS:-2}"` / `--max-rows "${MAM_MAX_PANE_ROWS:-2}"` wiring (regression guard). PASSED.
|
||||
|
||||
### W5 — Tests for deterministic 2×2 trajectory ✅
|
||||
Trajectory (both GUI `test_layout_single_workspace_54x23_compact_tiling_tier1` and headless `test_headless_0x0_transitions` / `test_headless_max_columns_growth_guard`):
|
||||
|
||||
| N | direction | target | reason |
|
||||
|---|---|---|---|
|
||||
| 1 | `right` | p1 | `new_column_right` |
|
||||
| 2 | `down` | p1 | `fill_column` |
|
||||
| 3 | `down` | p2 | `fill_column` |
|
||||
| 4 | `overflow` | — | `grid_capacity_reached` |
|
||||
|
||||
`test_headless_max_columns_growth_guard` additionally asserts the default cap holds when the caller **omits** `max_columns` — directly pinning the W4 no-`None` fix.
|
||||
|
||||
---
|
||||
|
||||
## 3. Lint
|
||||
|
||||
- `python -m py_compile .agents/skills/lib_py/layout.py` → **OK**.
|
||||
- `bash -n .agents/skills/lib.sh` → **OK**.
|
||||
- No `ruff`/`flake8`/`pylint` config in the repo, so Python "lint" = `py_compile` + manual review: helpers are single-responsibility, typed, and documented; `_env_int`'s docstring explains the skip-invalid (don't-abort-on-typo) design, consistent with lib.sh's silent-fallback safety model. No dead code; no unreachable branches observed.
|
||||
|
||||
## 4. Operability
|
||||
|
||||
- **Behavior change (N=1 → right)** is the intended Rev.2 contract; all callers/tests updated, no external caller breaks (lib.sh only consumes `direction`/`target`).
|
||||
- **2×2 cap** means overflow at 4 panes; `.mam.env.example` documents the cap and warns against raising `MAM_MAX_PANE_ROWS` pre-resize-normalisation. Operators needing more set the env.
|
||||
- **Defensive `_env_int`** skips unparsable values rather than raising — an operator typo cannot take the layout call down (lib.sh would still fall back to `right`), matching the codebase philosophy.
|
||||
|
||||
## 5. Loss / Omission Check
|
||||
|
||||
None for W1–W5. All four W4 layers present (signature + guard + argparse default + lib.sh + env doc). The only gap is **documentation**, not implementation:
|
||||
|
||||
- **F1 (traceability)**: `tests/test_b19_headless_reconcile_fixes.py` was modified but was **not enumerated in the brief's changeset list**. The change is correct and consistent with W2 (N=1 → right; 80-wide < 2×60 min-cols → overflow; 160-wide → right). Flagging only because the brief under-described the diff.
|
||||
|
||||
## 6. Findings (non-blocking)
|
||||
|
||||
- **F1 — Unlisted modified file**: `tests/test_b19_headless_reconcile_fixes.py` (see §5). Traceability/coverage observation only; the change itself is sound.
|
||||
- **F2 — Scope bundling**: the `grok` resume-allowlist edits (`resolve_session_id.sh`/`resume_session.sh`) remain bundled with the layout deliverable in the working tree. They belong to agent-onboarding; bundling muddies attribution. Already reviewed/PASSED in `a9de9b22`; unchanged here. Observation only.
|
||||
- **F3 — Long lib.sh line (~220 chars at :435)**: a backslash continuation would aid readability, but it matches the pre-existing one-liner style and `bash -n` passes. Cosmetic.
|
||||
- **F4 — Magic tolerances**: `_full_height_pane(tol=2)` and `_group_columns(abs(…)≤2)` share an implicit 2-cell rounding tolerance that isn't a named constant. Reasonable and internally consistent; minor.
|
||||
- **F5 — Resize-normalisation warning**: `.mam.env.example`'s "do not raise … until a resize-normalisation pass exists" is good guidance but references no tracking issue. Minor.
|
||||
|
||||
## 7. Verdict
|
||||
|
||||
The implementation faithfully realizes the approved Rev.2 plan across all five work items. The plan's central risk — the production-inert `max_columns` ("double omission") — is closed via triple-layered defaults and is pinned by integration tests at the real CLI entrypoint (flag path, env path, and default path) plus a lib.sh content guard. The single decision table is clean, the 2×2 trajectory is deterministic in both GUI and headless, and all tests pass (`test_layout.py` 38, `test_tier1_unit.py` 61, `test_b19` 6; `py_compile`/`bash -n` clean). Findings are traceability/scope/cosmetic and non-blocking. No redesign-level rework is required.
|
||||
|
||||
[VERDICT: PASS]
|
||||
@@ -0,0 +1,66 @@
|
||||
# Code Review Report — Job `a9de9b22`
|
||||
|
||||
- **Reviewer**: cline (session: `herdr:reviewer-cline-01`)
|
||||
- **Subject**: Layout engine improvement plan (Rev.2) + cumulative `git diff`
|
||||
- **Changeset**: 2 modified shell scripts + 1 new report (untracked)
|
||||
- `M .agents/skills/multi-agent-mux-resume/scripts/resolve_session_id.sh`
|
||||
- `M .agents/skills/multi-agent-mux-resume/scripts/resume_session.sh`
|
||||
- `?? .agents/reports/layout_engine_improvement_plan.md` (387 lines)
|
||||
- **Date**: 2026-08-26
|
||||
|
||||
---
|
||||
|
||||
## 1. Scope
|
||||
|
||||
The brief's stated work goal is the **layout engine improvement plan** (analyze Herdr skew, produce `.agents/reports/layout_engine_improvement_plan.md`). The cumulative `git diff` is a **mixed changeset**: the plan report (the deliverable) **plus** two `grok` agent-allowlist additions in the resume skill — a separate concern. Both are reviewed below per the brief's "누적 변경분(git diff)" instruction.
|
||||
|
||||
Note: the plan is a **forward-looking spec**; it does **not** modify `layout.py`/`lib.sh` in this changeset (those are future W1–W8 tasks). The only **runtime** behavior change in this diff is the `grok` allowlist.
|
||||
|
||||
## 2. Layout Plan — Verification Against Actual Code
|
||||
|
||||
The plan's high-stakes technical claims were checked against the live source:
|
||||
|
||||
| Claim | Source location | Verified |
|
||||
|---|---|---|
|
||||
| Headless uses `n % 2` parity | `layout.py:115` `if n % 2 == 1:` → `headless_odd_down` (:120); even → `headless_even_right` (:125) | ✅ |
|
||||
| `lib.sh` does **not** pass `--max-cols` | `lib.sh:435` pipes `--min-cols … --min-rows … --sample-pane …` only; no `--max-cols` | ✅ |
|
||||
| `--max-cols` default returns `None` when env unset | `layout.py:203` `default=_env_int("MAM_MAX_COLS","MAM_MAX_PANE_COLS")` — no `default=` arg → `_env_int` returns `None` | ✅ |
|
||||
| "Double omission" makes signature-only fix inert in production | env unset + no flag ⇒ `max_columns=None` ⇒ no cap, regardless of signature default | ✅ Accurate |
|
||||
| `.mam.env.example` self-inconsistent | `:147` `#default: (unset -> no column cap)` vs `:148` `# MAM_MAX_PANE_COLS=3`; no `MAM_MAX_PANE_ROWS` anywhere | ✅ |
|
||||
| Existing tests fix the old contract | `test_layout.py:145` N=1→down; `:400` n=2→right; `:414` n=4 unlimited→right | ✅ |
|
||||
| `fill_singleton_column` checks `len(col)==1` | `layout.py:150` `if len(col) == 1:` → `fill_singleton_column` (:159) | ✅ |
|
||||
|
||||
**Assessment**: The plan's central alarm — that wiring `max_columns` only via the signature default would pass tests but be **silently inert in production** (because `lib.sh` never passes `--max-cols` and `_env_int` yields `None`) — is **technically correct** and is the most valuable finding in the document. The proposed remedy (`lib.sh:435` explicitly pass `--max-cols`/`--max-rows` with `:-2` shell defaults + add `MAM_MAX_PANE_ROWS=2`) is the right fix. The single-decision-table design (§2), the trajectory correction (§3, n=4 stops at 2×2), and the parity-rejection rationale (§1) are internally consistent and actionable (Creator sign-off §13). Three objections sustained + three self-corrections (C-3/C-4/C-5) is a sound revision record.
|
||||
|
||||
As a **report** deliverable, "lint" is N/A; **operability** (actionable/correct) ✅; **loss** — minor, see §5.
|
||||
|
||||
## 3. `grok` Allowlist Changes — Verification
|
||||
|
||||
- `resolve_session_id.sh:39` adds `grok` to the `case`; error message `:40` updated to list grok. ✅
|
||||
- `resume_session.sh:45` adds `grok` to the `case` (error `:46` is generic). ✅
|
||||
- **Coherence**: `resume_session.sh:112` **already** had a `grok)` fallback (`--resume $UUID --permission-mode bypassPermissions`) before this diff — but the top-level validation `:45` rejected `grok`, so that path was **dead/unreachable**. This diff closes the gap: validation now matches the pre-existing downstream support. `grok` is a registered first-class adapter (`lib_py/agents/registry.py:16` `GrokAgentAdapter`), so resolution/resume are grok-aware end-to-end.
|
||||
- **Consistency**: brings the resume skill in line with peer skills (`create_session.sh:93`, `stop_session.sh:97`, `orc_onboard.sh:107` already accept grok). The resume skill was the last holdout.
|
||||
- `bash -n`: both scripts pass.
|
||||
## 4. Test Results
|
||||
|
||||
| Suite | Result |
|
||||
|---|---|
|
||||
| `bash -n` resolve_session_id.sh / resume_session.sh | OK |
|
||||
| `pytest tests/test_layout.py -q` | **26 passed** (unaffected — diff doesn't touch `layout.py`) |
|
||||
| `pytest tests/test_tier1_unit.py -q -k 'resume or grok or agent or find_workspace'` | **13 passed**, 48 deselected |
|
||||
| `pytest tests/test_tier2_component.py -q -k 'resume'` | **8 passed**, 32 deselected |
|
||||
|
||||
No regressions from the `grok` additions; no test asserts the old grok-rejecting behavior.
|
||||
|
||||
## 5. Findings (non-blocking)
|
||||
|
||||
- **F1 — Mixed/unrelated changeset (scope hygiene)**: the `grok` allowlist changes belong to the agent-onboarding concern, not the layout-engine task. Bundling them with the plan report muddies attribution. Observation only (the brief explicitly includes the cumulative diff, so both are reviewed).
|
||||
- **F2 — Stale usage docstrings (cosmetic)**: `resolve_session_id.sh:4,16` and `resume_session.sh:12` still advertise `--agent <claude|agy|hermes|cline>` without `grok`, while the `case` now accepts grok. `--help` understates accepted agents. Pre-existing repo-wide pattern (create/stop share it), but the diff touched these files and could have aligned the docstrings in the same touch. Non-blocking.
|
||||
- **F3 — Minor clarity in plan §1**: the §1 table's "홀짝 반전 결과" column models the *hypothetical literal-reversal* implementation (n=1→right…), whereas the actual current code is the *non-reversed* parity (`odd→down / even→right`). Both are parity-based and both diverge from Rev.2's right-first trajectory, so the thesis holds; §1 lines 45–52 correctly state the *real* current test assertions. A reader skimming only the table could momentarily mis-map it to the current code. Cosmetic.
|
||||
- **F4 — Plan is spec-only this pass**: no `layout.py`/`lib.sh` mutation occurred in this changeset, so the "implementation" reviewed here is a plan + a small agent-allowlist fix — not the layout rework itself. Worth stating to set expectations for the next (W1–W8) implementation pass.
|
||||
|
||||
## 6. Verdict
|
||||
|
||||
The layout plan is technically sound and its pivotal claim (production-inert `max_columns` wiring) is verified against live code; the `grok` allowlist changes are correct, coherent, and tested green. Findings are cosmetic/scope-only and non-blocking. No redesign-level rework is required.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user