fix: hardening round 3 — #48 #49 #50 #51 #52 - #61
Conversation
…ize (#50) The reported premise does not reproduce: indexed-array append-assign on unset elements (a[$i]+=x) creates the element without nounset errors on bash 3.2 and 5.x, all element reads in screen.sh already use :- guards, and every plane is initialized at source time. The new tests pin both risk scenarios named in the issue — sparse high row indices and frame growth after empty frames — under set -u, so a refactor that introduces an unguarded read now fails here instead of in production on macOS.
…ted (#49) README and CLAUDE.md described a feature-flag cache (.toolrc.local with SHELLFRAME_TPUT_OK / FD_ALLOC / READ_DECIMAL_T) that no revision of the code ever implemented: capability selection happens inline via BASH_VERSINFO guards and raw ANSI instead of tput. The staleness bug #49 reports therefore cannot occur; the real defect was documentation describing phantom machinery. Both docs now describe the actual inline capability checks, and CLAUDE.md warns against reintroducing references.
Consumers that follow their own fd 3 convention (log tee-ing) collided
with the hardcoded 'exec 3>/dev/tty'. The persistent-fd design is kept;
the NUMBER is now a knob read at source time (SHELLFRAME_TTY_FD, default
3). All 43 library write sites route through $_SF_TTY_FD, open/close go
through eval'd exec so bash 3.2 can handle dynamic fds. bash 4.1+
{var}> auto-allocation was considered and deliberately skipped in favor
of one predictable number per session.
Unit tests: default stays 3; SHELLFRAME_TTY_FD=7 routes renders to fd 7
while a consumer payload on fd 3 is untouched.
The disabled 'read -t 0' prototype destroyed buffered input on bash >=4 (one non-blocking read consumes the buffer, reporting empty) — lost bytes mid-escape-sequence were the historical crash; on 3.2 it silently did nothing. Input probing is unsound in pure bash; coalescing is now TIME-based: draws defer within SHELLFRAME_RENDER_MIN_INTERVAL_MS (default 33), force through after SHELLFRAME_RENDER_MAX_DEFER_MS (default 200), and the idle-timeout branch force-flushes since the queue is provably empty. Requires a sub-second clock; without one, behavior is unchanged (unthrottled). Root cause + rule recorded as hard-won-lessons #10.
…51) busybox date accepts %3N and silently prints seconds-only, so the 3.2 matrix leg got a clock that made every render age look like 0-1 ms — starving all draws. The probe now requires a 13-digit result; platforms without a usable sub-second clock fall back to unthrottled rendering, and the throttle unit tests adapt.
New src/widgets/spinner.sh with three feedback primitives for long-running work in standalone scripts: - shellframe_spinner 'msg' -- cmd... runs the command behind an animator on /dev/tty and propagates its exit status and streams (the $()/exit-code contract). Animator dies with the process group on INT/TERM. - shellframe_progress <cur> <total> [label] idempotent same-line bar; clamps over-range, guards zero totals, rejects non-numerics (rc 2). - shellframe_status 'text' single-line repaint. All three degrade to plain text lines when stderr is not a tty, so $() capture and CI logs stay readable. Unit table covers bar math at 0/50/100%, clamping, rejection, exit-code propagation; PTY tests verify animation frames on the tty, exit propagation, and the trailing clear-line (tail-based, since pty_run normalizes CR even in RAW mode).
…story - #48 completion: the v1 widgets' stdout save-slot still hardcoded fd 3 (exec 3>&1), so the exact filed scenario — a consumer owning fd 3 — lost its 'after' payload even with SHELLFRAME_TTY_FD set. Save slots are now dynamically picked from closed fds (excluding the tty fd) via _shellframe_pick_save_fd; restore/close route through it; each widget self-heals an unopened tty fd before redirecting stdout. - #52: spinner's non-tty branch wrote its label to stdout, polluting $() capture in CI/cron/pipes. Now stderr. - hard-won-lessons #10 corrected after reviewer challenge: held-open fifo/PTY experiments show read -t 0 does NOT consume input on bash 3.2 or 5.x (the earlier claim tested a pipe whose writer had already closed). The consumption story is withdrawn; time-based coalescing stands on its own merits (no stream interaction, version-independent), and the historical crash is recorded as unexplained. Suite 1586/1586; Docker matrix 3/3 PASS.
Review round 3 response — all findings addressed (
|
Review of
|
…ng + docs
- _shellframe_pick_save_fd: 'exec N>&-' is an idempotent no-op that
returns 0 for open AND closed fds, so the old probe never fired and
every widget silently fell back to fd 4. Replaced with the verified
write-or-read probe ({ : >&N || : <&N; }): open passes either,
closed fails both.
- Direct-consumer coverage extended to fd 4 (SF48_FD_NUM=4 fixture
variant) with the knob set — the exact collision the broken probe
hid. Picker selection unit-tested (open candidates skipped in order).
- #44b idle→EOF PTY test gets PTY_TIMEOUT=25: the macOS 3.2 runner was
killing the fixture mid-hold (rc 124) purely on timing margin.
- Lesson #10 heading rewritten to match the withdrawn consumption claim.
- README documents SHELLFRAME_TTY_FD for fd-3 owners.
- The stray autocomplete plan file's deletion is committed for good.
Suite 1589/1589 across 54 files; Docker matrix 3/3 PASS.
Review round 4 response — both blockers fixed with your probe (
|
Round-4 verdict: approved — merge-ready (
|
Summary
Closes the five remaining review issues:
SHELLFRAME_TTY_FDknob (default 3): all 43 library tty writes route through one variable; consumers with their own fd 3 convention set the env before sourcing. Deviation from spec noted: no bash-4.1 auto-allocation, one predictable number per session instead..toolrc.localcache never existed in any revision (archaeology viagit log --all -S). Real defect was doc rot; README/CLAUDE.md corrected to describe the actual inline capability checks._SF_ROW_CURR[$_r]+=lacks the set -u empty-array guard CLAUDE.md mandates #50 — premise not reproducible (append-assign under-uverified safe on 3.2/5.x); sparse-row and frame-growth regression tests added so future unguarded reads fail here first.read -t 0consumes buffered input on bash ≥4 (reproduced; "XYZ" vanished) and is inert on 3.2 — lesson [P3] Scroll container #10 in hard-won-lessons. 33 ms deferral window, 200 ms forced-progress cap, idle-timeout force-flush, graceful fallback without a sub-second clock. Busyboxdate +%s%Nvalidation trap documented and defended.src/widgets/spinner.sh: spinner wrapper (exit-status + stream preserving), idempotent progress bar, status line; non-tty degradation throughout.Test plan
_SF_ROW_CURR[$_r]+=lacks the set -u empty-array guard CLAUDE.md mandates #50), test-input ([Bug] M5: Bracketed-paste drain has no escape hatch and no sanitization (editor) #45/[Bug] M6: SGR mouse parsing is unvalidated and duplicated (input.sh vs shell.sh) #46 round-2 cases incl. NUL semantics + timeout flag), test-shell (throttle decision table, platform-aware)