docs(roadmap): add new agent types extension roadmap and peer review reports
This commit is contained in:
@@ -0,0 +1,117 @@
|
||||
# Review Report — Job 4fd933af
|
||||
|
||||
- **Reviewer**: cline (herdr session `reviewer-cline-01`, role: reviewer)
|
||||
- **Job ID**: 4fd933af
|
||||
- **Reviewed branch**: `main` (1 commit ahead of `origin/main`)
|
||||
- **Scope**: Cross code review (lint / operability / drift) of (a) the committed
|
||||
layout-engine enhancement `f3ac68f` and (b) the untracked roadmap document
|
||||
`.agents/reports/new_agent_types_roadmap.md`. Task goals: validate layout
|
||||
correctness, assess new-agent-extension roadmap feasibility, issue verdict.
|
||||
|
||||
## 1. Change Inventory
|
||||
|
||||
| Item | File(s) | Status |
|
||||
|------|---------|--------|
|
||||
| Layout relax | `lib_py/layout.py`, `lib.sh:432`, `.mam.env.example`, `tests/test_layout.py`, `tests/test_tier1_unit.py` | **Committed** (`f3ac68f`) |
|
||||
| New-agent roadmap | `.agents/reports/new_agent_types_roadmap.md` (new, 229 lines) | **Untracked** |
|
||||
|
||||
Commit `f3ac68f` — "feat(layout): relax MAM_MIN_PANE_COLS to 15 and remove
|
||||
vertical height constraints for scrollable terminal split":
|
||||
- `compute_2xk_layout` defaults: `min_cols` 40→**15**, `min_rows` 20→**0**.
|
||||
- CLI `--min-cols`/`--min-rows` argparse defaults: 15 / 0.
|
||||
- `lib.sh:432` fallbacks: `${MAM_MIN_PANE_COLS:-15}` / `${MAM_MIN_PANE_ROWS:-0}`.
|
||||
- Both height-overflow guards now short-circuit on `min_rows > 0 and ...`
|
||||
(single-pane and singleton-column paths), so default `min_rows=0` disables
|
||||
vertical constraints while a positive env value re-engages them.
|
||||
- `.mam.env.example` updated (default 15 / 0, terminology aligned to tmux
|
||||
horizontal=side-by-side / vertical=top-bottom).
|
||||
- Tests rewritten to assert `min_cols=15` boundary (30 split vs 29 overflow)
|
||||
and `min_rows=0` behavior.
|
||||
|
||||
## 2. Lint / Syntax / Drift
|
||||
|
||||
- `python -m py_compile lib_py/layout.py` → **OK**
|
||||
- `bash -n .agents/skills/lib.sh` → **OK**
|
||||
- Stale-default sweep (`:-40`, `:-20`, `min_cols: int = 40`, `min_rows: int = 20`,
|
||||
`default=40`, `default=20`) across `.agents`/`tests` → **clean (no orphans)**.
|
||||
All authoritative default sites are consistently 15 / 0.
|
||||
- Working-tree drift: only the roadmap file is untracked; no stray/dirty
|
||||
submodule or out-of-scope edits this round (cleaner than the prior review).
|
||||
|
||||
## 3. Layout Implementation — Operability Verification
|
||||
|
||||
### Goal: dense tiling in compact viewports without premature overflow
|
||||
- **Column boundary** (`width // 2 < min_cols`, strict `<`): at width 30 →
|
||||
`30//2 = 15`, `15 < 15` False → splits right; at width 29 → `29//2 = 14 < 15`
|
||||
→ `column_width_overflow`. CLI-confirmed: 30-col → `right p1`, 29-col →
|
||||
`overflow p1`. ✓
|
||||
- **`min_rows=0` disables height checks**: a single 80×3 pane → `down p1`
|
||||
(splits down; scrollback rationale holds). With an explicit positive
|
||||
`MAM_MIN_PANE_ROWS`, the `min_rows > 0 and ...` guards re-engage the
|
||||
`single_pane_height_constrained` / `singleton_height_overflow` paths — so
|
||||
the disable is opt-out, not a hard removal. Well-designed. ✓
|
||||
- **Tiling progression** (1→2 down, 2→3 right, 3→4 fill-singleton down,
|
||||
4→5 overflow) unchanged in structure; only thresholds moved. The 54×23 /
|
||||
90–100 col scenarios described in the roadmap now fit 4 panes per workspace
|
||||
where the old 60/20 defaults could not even open a 2nd column. ✓
|
||||
|
||||
### Tests
|
||||
- `tests/test_layout.py` + `tests/test_tier1_unit.py` → **85 passed in 9.11s**.
|
||||
- `tests/test_a4_adapter_contract.py` → **13 passed in 0.47s**.
|
||||
## 4. New-Agent-Types Roadmap — Feasibility Assessment
|
||||
|
||||
The roadmap (`new_agent_types_roadmap.md`) is a planning/architecture document,
|
||||
not executable code. Its value rests on the accuracy of its codebase references
|
||||
and the soundness of its blueprint. Both verified:
|
||||
|
||||
| Roadmap claim | Verification | Result |
|
||||
|---------------|--------------|--------|
|
||||
| Adapter framework `lib_py.agents` with `BaseAgentAdapter` | `lib_py/agents/{base.py,registry.py,sanitize.py,input_region.py,adapters/}` exist | ✓ |
|
||||
| `BaseAgentAdapter` interface (name, own_key, ready_tokens, exit_key, delegate_agent_key, identity_cache_fields, input_prompt, input_rule_pattern, artifact_path, verify_artifact, purge_artifacts, spawn_spec, resume_spec, auth_ok, discover) | All symbols present in `base.py` at the cited lines | ✓ exact |
|
||||
| `DiscoveryContext` used in template code | `class DiscoveryContext` at `base.py:15` | ✓ |
|
||||
| Step 2: add `'newagent': NewAgentAdapter()` to `_ADAPTERS` | `registry.py` `_ADAPTERS` dict registers claude/agy/hermes/cline exactly this way | ✓ proven pattern |
|
||||
| Step 3.1: herdr kind mapping `lib.sh:358-375` | kind dispatch at `lib.sh:357-371` (creator/planner/reviewer → kind) | ✓ (line off by ~4) |
|
||||
| Step 3.3: "automatic via `agent_of_row()`" | `agent_of_row()` + `matches_session_name()` in `registry.py` | ✓ |
|
||||
| Step 5.1: contract tests `test_agent_adapter_registry` / `test_adapter_required_properties` / `test_facts_bridge_eval_contract` | All three present in `test_a4_adapter_contract.py` (lines 28/58/79) | ✓ |
|
||||
| Step 5.2: `tests/test_tier2_component.py` | File exists | ✓ |
|
||||
|
||||
**Feasibility verdict**: The 5-step blueprint (adapter class → registry →
|
||||
`lib.sh` dispatch → herdr `--kind` → contract/lifecycle tests) is the correct
|
||||
layering and is proven by the four existing adapters. The complexity matrix
|
||||
(Tier 1 adapter ≈ 0.5 day; Tier 2 herdr-kind/lifecycle; Tier 3 TUI readiness)
|
||||
is reasonable. The document is actionable as-is.
|
||||
|
||||
## 5. Findings (advisory, non-blocking)
|
||||
|
||||
### F-1 (hygiene): Roadmap file is untracked
|
||||
`git status` lists `.agents/reports/new_agent_types_roadmap.md` as untracked.
|
||||
Since it is a deliverable referenced by this job's output contract, it should
|
||||
be `git add`-ed and committed (e.g. `docs(roadmap): new agent types extension
|
||||
blueprint`) so the planning artifact persists. Content is fine; only tracking
|
||||
state needs action.
|
||||
|
||||
### F-2 (verification gap): Full-suite green not independently confirmed
|
||||
The roadmap §1.1 asserts "all 371 tests across 18 suites pass." This review
|
||||
confirmed 98 unit tests across all touched files plus the adapter-contract
|
||||
suite, but did not re-run the full integration suite within the time budget.
|
||||
**Direction**: confirm the full `pytest tests/` is green in CI before merge;
|
||||
no code change implied.
|
||||
|
||||
### F-3 (doc wording nit): `min_rows=0` is "disabled-by-default", not "removed"
|
||||
Roadmap §1.1 says vertical constraints were "removed." Precisely, the guards
|
||||
are `min_rows > 0 and ...`, so a positive `MAM_MIN_PANE_ROWS` env value
|
||||
re-engages height checks. The doc slightly understates this opt-in
|
||||
re-enablement. **Direction**: one-line wording fix ("disabled by default;
|
||||
re-enabled by setting MAM_MIN_PANE_ROWS>0") for operator accuracy. Non-blocking.
|
||||
|
||||
## 6. Summary
|
||||
|
||||
The committed layout change correctly and consistently relaxes `min_cols` to
|
||||
15 and defaults `min_rows` to 0 (disabled-by-default, re-engaged via env) with
|
||||
matching, passing tests (98 verified green) and clean syntax. The new-agent
|
||||
roadmap is a feasible, accurately-referenced blueprint grounded in the real
|
||||
adapter framework (every cited file, symbol, and test verified to exist). The
|
||||
three findings are commit-hygiene / verification-gap / wording nits that do
|
||||
not affect correctness, operability, or feasibility, and require no redesign.
|
||||
|
||||
[VERDICT: PASS]
|
||||
Reference in New Issue
Block a user