tui3: tree rails back in the tasks side column

af-tree-rails-reviews/round0-kimi.md

@@ -0,0 +1,23 @@
+# Deep review — tree rails in the tui3 tasks side column (round 0, kimi)
+
+**Reviewed state:** repository `/Users/santoshkumar/af-tree-rails`, branch `task/restore-tree-rails-in-the-tasks-c2da84`, base `origin/dev` = `e7976208e`, HEAD = `4c1e1d481` ("tui3: tree rails back in the tasks side column"), single commit `e7976208e..HEAD` (`git diff e7976208e..HEAD`, 9 files, +167/−23). Reviewed read-only: no files modified, no commits, no build or test execution, per the review-agent skill's contract.
+
+**Hand-off note:** the sandbox refused the named output path (`/Users/santoshkumar/af-tree-rails-reviews/` is outside this task's copy), so the report lives at `round0-kimi.md` in the assigned tree and should be copied to `/Users/santoshkumar/af-tree-rails-reviews/round0-kimi.md` — verbatim, one `cp` — by whoever integrates this run.
+
+## Findings
+
+No findings.
+
+## Overall assessment
+
+The diff was read in full and each load-bearing seam was traced at HEAD, not inferred from the patch:
+
+- **The lead is one mechanism with two callers.** `railLead(last []bool, elbow bool)` (planrail.go:252) is the only place a lead is spelled, and it is exactly 2 cells per level: `├ ` / `└ ` at the row's own level when `elbow` is set, `│ ` for a non-last ancestor, two spaces for a last ancestor, empty at depth 0. `planRailLines` (~:288), `planPageLines` (~:308) and `railLines` (task.go ~:3113) all build `last` identically (`copy` + append `i == len(kids)-1`) and prepend `lead +` to an unchanged `railEntryRow(e, width)` (task.go:4290, verified byte-for-byte against the brief's "must be unchanged"). No second shape exists.
+- **Depth ≥ 2 last/stem logic is correct.** `railLines`' `open` slice is updated before the recursion (`open = append(open, i < len(nodes)-1)`) and restored after, so a middle child at depth 2+ carries `│ ` at its ancestor's level and its own connector is never confused with its parent's (that confusion is exactly what c263's `TestPlanRailDrawsTheTree` pins, and the shape reads correctly there).
+- **The width invariant holds, and was upgraded honestly.** The old sites subtracted `len(lead)`; the diff switches every cut (`planRailLines`, `planPageLines` row + under-line, `railLines` head + under-stem) to `ansi.StringWidth(lead)`. That is not width laundering: `palette.glyph` (styles.go:1124) returns the bare rune with no styling escapes, and the tree glyphs (`├` U+251C, `└` U+2514, `│` U+2502, glyph.go:288-291) resolve to their Plain spelling in every tier — they carry no NerdFont binding (nerdfont.go:537-546 sets only Name/Meaning, and glyphset.go:297-306 falls back to Plain) and no ASCII binding either, so the ASCII tier also emits the 1-cell box glyph. Every measured lead is exactly 2 cells/level, matching what the brief's width computations assumed. The terminal-dependent caveat (a box glyph reported wide) is real but uniform: both the subtraction and the concat consume the same measurement, so the row still fits; nothing double-counts.
+- **Under-line trunk rule matches between the two renderers.** The node-hung side (`railUnder` caller, task.go:3388-3397) and the plan-page side (`planPageLines` with `hang := a.railLead(at, false)`) both suppress the stem exactly at the row's own deepest level and keep `│ ` where an ancestor's connector is `├`. The change-entry's "hanging `└─`-off-`│`" claim is accurate.
+- **No re-parenting, no second lent-id scheme.** `planRailForest`, `planTwig`, `planRailNode`, `planRailNodeID`/`planRailNodeBit` are untouched by the diff (checked by inspection of both the patch and the live code), and c246's exact-text law ("a dependency never re-parents a row"; Alpha still ordered inside A's family and reading `queued · waits: B`) is still asserted verbatim — the tests were re-texted for the new lead, not re-semanticed.
+- **Tests pin the contract, not just the current output.** c263's `TestPlanRailDrawsTheTree` asserts the depth-2 shape including the continuing stem `│ ├ └ Alpha` after a closed sibling `└ └ B`, so a middle child losing its `│ ` fails it. c266's updated assertions still bind indentation ordering (child's name strictly right of parent's) and the no-clipping law; the new near-clipping condition in `railLines` is `2*(inner+2) <= width`, i.e. the untruncated text plus its own lead would fit — the stem is drawn only when it provably does not crowd a full-length row.
+- **`planroom.go`'s touch is a comment fix that now tells the truth** (`railLead` → `railEntryRow`: the entry row is the two-cell economy, the lead is the caller's). The manual and change-entry edits match the shipped behavior.
+
+Residual risks and material test gaps, in the open: (1) No test drives a family deeper than depth 2, so the `open`/`last` restore-after-recursion invariant at depth 3+ (a stem that must outlive a fully closed grandparent) is pinned by code reading, not by a test. (2) No test asserts behavior on a terminal tier where a box-drawing glyph measures wide; the comment in `railLead` documents that the row stays correct via StringWidth, but nothing exercises it. (3) The plan-page under-block truncation is only covered at width 160 (c263 `TestPlanPageRoom…`); the narrow-width `hang + "    " + under` path at the clipping boundary is untested. None of these rise to a defect in the diff itself; they are gaps worth a follow-up test if the author wants the depth-3+ and narrow-width contracts pinned. The pre-existing `credits_test.go` failures and the wider suite wedge named in the brief are outside this diff and were not re-examined.

af-tree-rails-reviews/round1b-quality.md

@@ -0,0 +1,34 @@
+# Round 1b — QUALITY + CONTRACT lens: tree rails in the tasks side column
+
+NOTE FOR THE COORDINATOR: this report must be copied to /Users/santoshkumar/af-tree-rails-reviews/round1b-quality.md (this task may only write inside its assigned tree).
+
+Reviewed state: base `origin/dev` **e7976208e**, HEAD **4c1e1d481** ("tui3: tree rails back in the tasks side column"), one commit, 9 files +167/−23. Reviewed `git diff e7976208e..4c1e1d481` in `/Users/santoshkumar/af-tree-rails`, read-only: nothing built, tested, executed, or modified. Lens: architecture, contracts, tests, repo gates. Draft PR Agent-Field/CodeAF#1665 (Fixes #1660) read for the author's description; the diff is what was reviewed. `.senior-dev/` ignored.
+
+## Findings
+
+[P3] `internal/tui3/c266_rail_row_test.go` — the indentation law's stability anchor was weakened in re-texting. The pre-change test matched the row `prefix + "- \u2022 Ship the port"`, which pinned three things at once: the connector glyph (`- `), the gap between connector and mark, and the mark (`•`) staying glued to the lead at its old column. The re-texted version matches `"- Ship the port"`, which any lead variant containing a bare `-` before the title also satisfies (e.g. a regression that renders the lead as `- Ship` with no task mark, or a broken glyph tier spelling the tree glyphs as `-`). The still-passing exact-text assertions at planrail.go's other seams partially cover the shape, but this particular law test no longer pins the connector cell's identity. Fix: restore the marker in the expected string (e.g. match against `lead+mark+title` the way c263_rail_plan_test.go now does, or keep the `- •` spelling with the actual current glyph).
+
+[P3] `internal/manual/chat/worker-harness.md` — the re-texted line overclaims scope. The old text said the side column's rows "carry no tree rails", which was simply true. The new text says "its rows hang by depth with `├ ` and `└ ` leads and `│ ` stems, a task under a task a level in" — true for plan-store rows (planRailLines/planPageLines), but node-hung families rendered by `railLines` (task.go:3113) did NOT gain rails in this diff (verified: `railLines` unchanged, no `railLead` call there). A reader of the manual will expect stems under a live worker's children and not see them. The author fixed exactly this class of overstatement one paragraph down in the same diff ("drawn by app.railEntryRow") but missed this one. Fix: qualify the sentence to the run's plan rows.
+
+## What was checked and holds (no finding)
+
+- **One mechanism, two callers, genuinely shared.** `railLead(last []bool, elbow bool)` (planrail.go:252) is the only lead builder; both `planRailLines` (:284) and `planPageLines` (:312) call it; `railEntryRow` (task.go ~4258) is untouched by the diff; there is no second shape and no copy-pasted lead logic. The extra `elbow` bool (vs a `railUnderLead`) is justified — the false form is taken by two distinct callers (folded tail :296, under-lines :324). Complexity is one loop over levels; nothing needless was added.
+- **Contract boundaries respected.** The lent-node id scheme (`planRailNodeBit` = 1<<63, `planRailNodeID` fnv over a namespaced salt) is unchanged; `teamspagedraw.go` untouched; `railLines` (node-hung families) deliberately untouched — matches the brief's intended design.
+- **Laws hold.** "A DEPENDENCY NEVER RE-PARENTS A ROW" (c246_tree_test.go) untouched and still asserts ordering, not pixels. The `queued · waits: <task>` wording tests are untouched by the diff. The waits-sentence shape test (own-first, `12 steps`) still asserts semantics.
+- **The new rail-shape test (c263_rail_plan_test.go) genuinely pins the contract.** Traced the bug-injection cases against `lead()` + `planPageDraws`: a middle child losing its stem → `aa` renders as air, lead mismatches the `railLead(aa, true)` reference → FAIL. A last-child elbow drawn as branch → `railLead` reference differs → FAIL. A depth-0 lead leak → `leadW` is 0 so `text` becomes `├ Write…` vs expected `Write…` → FAIL. A last-child's depth-2 child growing a stem (`pair` as `│ ` instead of air) → lead mismatch → FAIL. The under-row check (stem inherited on the Handler under-line) is also pinned. The test re-derives expectations from the same `railLead` under test, which would normally be self-referential, but the plain/glyph tier (`tokens.Plain`) pins the glyph strings independently, so a wrong glyph still fails.
+- **Width arithmetic.** The one touched width site (planroom.go:53) replaced `len(lead)` with `ansi.StringWidth(lead)` — strictly more correct (byte-vs-cell); all other room cuts (`entryWidth - leadWidth`, `planLineRoom`) were already cell-based via `ansi.StringWidth`, so the 2-cells-per-level invariant holds. Box-drawing glyphs come from the geometry vocabulary (`tokens.GlyphTreeBranch = "├"` etc.), and the geometry fallback law resolves them to the plain 1-cell rune in every tier including the ASCII floor — no width surprise. `planPageLines` gained `railWidth/2` headroom, removing a pre-existing truncation-at-2-cells edge.
+- **Slice aliasing.** `grow` builds each kid's `last` via `make+copy+append` — fresh backing array per level, no shared-slice corruption.
+- **Cycle/orphan safety.** `planTwigsOf` excludes self-parenting (`up != twig`) and orphans a row whose parent is absent onto the page root; a store cycle longer than self-loop is possible in theory but bounded by the store's own ordering, and is pre-existing behavior, not introduced here.
+- **Repo gates.** Changelog entry present (`docs/changes/unreleased/1665-tree-rails-side-column.md`, `kind: fix`, `pr: 1665`, `surface: chat tasks column`, `invalidates:` filled) — the CI changelog-check is satisfied. `gofmt -l` on the touched directories is clean (listing only, no modification). Namelaw: the one `CodeAF` in `worker-harness.md` is a pre-existing line about the OTHER program (the swe-pro harness trailer), untouched by the diff and allowed.
+
+## Overall assessment
+
+The change is architecturally clean: one shared helper, two callers, all width arithmetic cell-based, no boundary violations, and the changelog gate is satisfied. The two findings are both test/docs-quality P3s — worth fixing, neither blocks.
+
+**Brief-vs-diff discrepancy (observation, not a defect):** the brief describes taskident.go as "the task's number shown on side rows again". The actual taskident.go hunk is a doc-comment-only correction (`[app.railLead]` → `[app.railEntryRow]`); no rendering behavior changes there. The task-number display described was either landed in an earlier wave or is aspirational — nothing in this diff shows numbers on side rows. The sibling reviewers should not hunt for it in this diff.
+
+## Material test gaps / residual risks
+
+- The rail-shape test exercises exactly one family shape (single root, one mid child with kids, one last child). A regression specific to depth ≥ 3 (stem propagation past two last-flag levels) or to a family whose FIRST child is also its last (sole child → `└ `) is not pinned; the `└ ` path at depth 1 is pinned, the depth ≥ 3 path is not. Cheap to add one more kid layer if the author wants it.
+- `railLines` (node-hung families) has no rail-shape coverage because it has no rails — if the manual line (second finding) is instead made TRUE in a follow-up, that path will need the same treatment.
+- Tests were not executed (read-only contract); the verification is by trace against the helpers (`railText`, `readPlanRows`, `railSeam`, `railMark`, `planAppWith`, `update` all exist and match usage).