# 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]