feat(debugger-tui): B.2 — stack/locals pane (Phase B milestone 2 of #691)

[jsavin]

Summary

Implements Milestone B.2 of planning/phase_b/EXECUTION_PLAN.md — stack/locals pane. Builds on B.1 (#737, merged at b1a085a52).

What this PR proves: the TUI consumes the full debug/suspended notification surface end-to-end. debug/suspended now fans out to debug/getSource (B.1), debug/getStack, and debug/getLocals requests; their responses populate the right pane with the call stack at the top and innermost-frame locals at the bottom. Frame selection via arrow keys updates the script pane's path/line indicators.

What ships

• frontier-cli/debugger_tui.c — tui_store_stack, tui_store_locals, extended tui_write_line to dispatch debug/getStack and debug/getLocals responses, new draw_stack_pane callback rendering stack + locals, frame-selection arrow-key handler, cross-pane source path update on frame select
• frontier-cli/debugger_tui_internal.h — TUI_MAX_FRAMES = 200, TUI_MAX_LOCALS = 500, 5 new state fields (frame_scripts, frame_lines, frame_count, selected_frame, locals_*)
• tests/debugger_tui_tests.c — 8 new behavioral tests covering store-stack, store-locals, cap-truncation, render assertions, frame selection + cross-pane update

Diff: 3 files, +709/-16.

Deviations from the plan (all minor, all documented in commit)

1. debug/getLocals return shape. Plan said "flat JSON object." Actual at debug_handler.c:1900 is result.locals = array of {name, value, type}. Code-verified by the sub-agent and parsed correctly.

2. log_warn removed from tui_store_* truncation paths — including a retroactive fix to B.1's tui_store_source. Reason: -Wl,-undefined,dynamic_lookup in the test build resolves log_write to NULL; the cap-exceeded path would crash the test binary on segfault at address 0x0. This is a real latent bug in B.1 (the TUI_MAX_SOURCE_LINES cap added in PR feat(debugger-tui): B.1 — script pane + debug/getSource wiring (Phase B milestone 1 of #691) #737 round-1's P1-B fix) that never fired because no test sent 10000+ lines. Truncation now silent with an explanatory comment.

3. Frame ordering: outermost-first in debug/getStack wire format. UP arrow decreases selected_frame (toward outermost), DOWN increases toward innermost.

4. Cross-pane source reload on frame select is stub-only in B.2 — direct field update from frame data arrays, no op_dispatch call. Full live debug/getSource reload on frame select wires in B.6 once debug_set_attach_transport is connected. Matches the plan's "implement it by having the stack pane's input callback call the same load_source function" — in B.2 that function is the direct field update.

TDD red/green

RED (compile error, correct fail-on-undefined-state — first TDD attempt before implementing B.2 state struct):

debugger_tui_tests.c:497:20: error: use of undeclared identifier 'TUI_MAX_FRAMES'
debugger_tui_tests.c:498:5: error: no member named 'frame_count' in 'tui_state_t'
debugger_tui_tests.c:499:5: error: no member named 'selected_frame' in 'tui_state_t'

Intermediate failure (uncovered the B.1 log_warn latent bug):

test_stack_frame_count_capped: EXC_BAD_ACCESS at 0x0 (log_write resolved to NULL via dynamic_lookup)

GREEN (after removing log_warn + completing implementation):

[test_report] debugger_tui_tests: 19/19 tests passed

Test plan

• make build — clean
• ./tools/run_headless_tests.sh — 687/687 pass (was 679; +8 new B.2 tests)
• cd tests && make test-integration — 2186 pass / 21 baseline failures (failure names character-for-character identical to baseline)

What's NOT in this PR

• Step/continue keybinds (B.3) — only quit + arrow keys land here
• Breakpoint UI (B.4)
• Cmd-double-click + watchpoints (B.5)
• debug_set_attach_transport wiring (B.6)
• Cross-pane source reload via op_dispatch on frame select (B.6)
• Scratch-eval pane (B.7)
• Shell integration test for real-terminal coverage (still deferred to PR #734 follow-up: P2 polish from B.0 /gate fix-loop (5 items) #736)

Proactive bounds-checking applied (lesson from B.1 round 1)

Following the security review pattern from PR #737:

• All draw callbacks that fill a local line buffer cap to sizeof(buf) - 1 before any memcpy/memset
• tui_store_stack caps at TUI_MAX_FRAMES = 200; tui_store_locals caps at TUI_MAX_LOCALS = 500
• All array-of-pointers allocations use calloc, not malloc; truncate count to populated slots on strdup failure
• All cJSON field reads guarded by cJSON_IsNumber / cJSON_IsString; absent fields preserve previous state (don't reset to sentinel)
• tui_store_* helpers early-return on count <= 0 before mutating other state fields

Sentinel verification

GIL hot path indirectly touched (transport callback in B.6+). Per /auto Phase 5 criterion 8, main-session integration verification run from this worktree at HEAD f82a5b72a. Integration failure-name set matches baseline character-for-character.

🤖 Generated with Claude Code

[jsavin]

/gate Round 2 — Convergence

Verdict: PASS — all reviewers APPROVE, 0 P0/P1 remaining.

Reviewer	Round 1	Round 2
bar-raiser	1 P1 (empty-array stale state) + 2 P2	APPROVE
security	APPROVE + 2 P2	APPROVE
concurrency	APPROVE (no concerns)	APPROVE

Round-1 P1 + P2s addressed in commit d3397fe24

1. P1 (bar-raiser): empty-array state clobber — tui_store_stack and tui_store_locals count <= 0 early-return conflated non-array (invalid) with empty array (legitimately empty). Empty arrays carry definite info ("stack/locals are now empty"). Moved destructive clear before count check.
2. P2 (security): unbounded per-local strdup — Cap name at 256 / value at 4096 via length-bounded malloc + memcpy + explicit NUL. Worst-case heap bounded to TUI_MAX_LOCALS × 4352 ≈ 2.2 MB.
3. P2 (bar-raiser): frame-select source desync — When frame_scripts[new_frame] != script_path, blank the script pane until B.6 wires the live op_dispatch reload.

Deferred P2 items (for follow-up issue)

• log_warn NULL-safe in test build (real test-infra fix, not in this PR scope)
• "" visible indicator in renderer
• Level field recompute from index vs storing the wire value
• content_rows separator on empty locals (cosmetic)
• Frame-select bounds tests at UP-from-0 and DOWN-from-last

Tests at HEAD d3397fe24

• make build: clean
• ./tools/run_headless_tests.sh: 691/691 pass (was 687; +4 regression tests)
• cd tests && make test-integration: 2186 pass / 21 baseline failures (failure names character-for-character identical to baseline)

🤖 Generated with Claude Code /gate

[jsavin]

/auto Summary

What went well: Two-round convergence with proactive lesson-application paying off. The sub-agent applied B.1's hard-won patterns (cap-before-allocation, calloc, cJSON_IsX guards, linebuf bounds) up front, so this round's reviewers found state-machine bugs (empty-array clobber) and per-value memory bounds rather than the buffer-overflow class B.1 had. P1 count dropped milestone-over-milestone (B.0: 4, B.1: 5, B.2: 1) — that's the right direction. B.2's sub-agent also retroactively fixed a latent crash in B.1's tui_store_source (cap path crashed test build because log_write resolves to NULL via dynamic_lookup) — a real cross-cutting find that the B.1 tests didn't exercise.

What could improve: The retroactive log_warn removal is the wrong long-term fix. Silent truncation is a debugging trap. The right fix is making log_write NULL-safe in the test build or stubbing it in test_main.c. Filed as #740 along with 4 other diagnostic-visibility P2s. Also: bar-raiser's P1 (empty-array clobber) was a state-machine subtlety I should have anticipated — B.1's P1-E was "preserve on absent" and the natural symmetry is "commit on empty," but neither I nor the sub-agent caught it pre-review.

Recommendations: For the upcoming B.3 (keybinds), still defer the shell integration test from #736 until B.4 or B.6 — the keybind paths are still mostly synthesizable. But B.6 (lazy-attach wiring) MUST have a real-binary smoke test because that's where the transport's write_line first sees production runtime data. Add the shell test as part of B.6's TDD spec.

🤖 Generated with Claude Code /auto