diff --git a/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md index f0de037b..68a42325 100644 --- a/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md +++ b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md @@ -5,51 +5,51 @@ titles: json findings: - id: WR-09 severity: warning - disposition: open + disposition: fixed title: "Lines printed through a live view after its context is cancelled are silently dropped" - id: WR-10 severity: warning - disposition: open + disposition: fixed title: "A delegate killed by a signal prints its full argv, including a --password value, and exits 1" - id: WR-11 severity: warning - disposition: open + disposition: fixed title: "The wrong-directory hint leaves secret values that start with \"-\" unmasked" - id: WR-12 severity: warning - disposition: open + disposition: fixed title: "migrate:status --json still drops the framework sets that migrate applies" - id: WR-13 severity: warning - disposition: open + disposition: fixed title: "migrate reports failure after a successful Migrate when the follow-up read fails" - id: WR-14 severity: warning - disposition: open + disposition: fixed title: "Process's plain path prints item names without Sanitize" - id: IN-10 severity: info - disposition: open + disposition: deferred title: "console.Error during a live view writes straight to the terminal and tears the view" - id: IN-11 severity: info - disposition: open + disposition: deferred title: "A blank line printed during a live view is dropped" - id: IN-12 severity: info - disposition: open + disposition: deferred title: "Signal relay edge cases: a signal during view start-up, and Windows" - id: IN-13 severity: info - disposition: open + disposition: deferred title: "A delegate's stderr always goes to os.Stderr, bypassing the root's error writer" - id: IN-14 severity: info - disposition: open + disposition: fixed title: "The ErrReported doc comment still says the exit code is 1" - id: IN-15 severity: info - disposition: open + disposition: fixed title: "runLive builds the bubbletea program before it knows it may run" - id: CR-01 severity: critical @@ -131,7 +131,7 @@ findings: severity: info disposition: deferred title: "The phase gate names consuming applications" -open: 12 +open: 0 total: 32 recorded: 2026-10-22T01:19:33.337Z --- @@ -140,18 +140,18 @@ recorded: 2026-10-22T01:19:33.337Z | Finding | Severity | Disposition | Source | |---------|----------|-------------|--------| -| WR-09 | warning | open | - | -| WR-10 | warning | open | - | -| WR-11 | warning | open | - | -| WR-12 | warning | open | - | -| WR-13 | warning | open | - | -| WR-14 | warning | open | - | -| IN-10 | info | open | - | -| IN-11 | info | open | - | -| IN-12 | info | open | - | -| IN-13 | info | open | - | -| IN-14 | info | open | - | -| IN-15 | info | open | - | +| WR-09 | warning | fixed | fix fce232d (quick 261022-4z8) | +| WR-10 | warning | fixed | fix 17c722d (quick 261022-4z8); the exit-130 suggestion is deferred to todo .planning/todos/pending/2026-10-22-console-review-info-findings.md | +| WR-11 | warning | fixed | fix 707372b (quick 261022-4z8) | +| WR-12 | warning | fixed | fix 57a849d (quick 261022-4z8) | +| WR-13 | warning | fixed | fix 0691d26 (quick 261022-4z8) | +| WR-14 | warning | fixed | fix 48e13f9 (quick 261022-4z8) | +| IN-10 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md | +| IN-11 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md | +| IN-12 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md | +| IN-13 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md | +| IN-14 | info | fixed | docs 7c15095 (quick 261022-4z8) | +| IN-15 | info | fixed | fix 1ac75ad (quick 261022-4z8) | | CR-01 | critical | fixed | fix b077dc3 (19.8-05) (not in the current review) | | CR-02 | critical | fixed | fix 323413c (19.8-05) (not in the current review) | | CR-03 | critical | fixed | fix 9c89705 (19.8-05) (not in the current review) | @@ -159,10 +159,10 @@ recorded: 2026-10-22T01:19:33.337Z | WR-02 | warning | fixed | fix 86e4e27 (19.8-05) (not in the current review) | | WR-03 | warning | fixed | fix fc7706d (19.8-05) (not in the current review) | | WR-04 | warning | fixed | fix c586c6f (19.8-05) (not in the current review) | -| WR-05 | warning | fixed | fix a9025c0 (19.8-05) (not in the current review) | +| WR-05 | warning | fixed | fix a9025c0 (19.8-05) (not in the current review); remainder WR-14 (fix 48e13f9) | | WR-06 | warning | fixed | fix 39677f1 (19.8-05) (not in the current review) | -| WR-07 | warning | fixed | fix 020fe83 (19.8-05) (not in the current review) | -| WR-08 | warning | fixed | fix 9b62285 (19.8-05) (not in the current review) | +| WR-07 | warning | fixed | fix 020fe83 (19.8-05) (not in the current review); remainder WR-12 (fix 57a849d) | +| WR-08 | warning | fixed | fix 9b62285 (19.8-05) (not in the current review); remainder WR-11 (fix 707372b) | | IN-01 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md (not in the current review) | | IN-02 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md (not in the current review) | | IN-03 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md (not in the current review) | diff --git a/.planning/todos/pending/2026-10-22-console-review-info-findings.md b/.planning/todos/pending/2026-10-22-console-review-info-findings.md index 2cc6f42e..8f89203e 100644 --- a/.planning/todos/pending/2026-10-22-console-review-info-findings.md +++ b/.planning/todos/pending/2026-10-22-console-review-info-findings.md @@ -1,6 +1,6 @@ --- created: 2026-10-22T00:43:48.000Z -title: Console review info findings (IN-01..IN-09) +title: Console review info findings (IN-01..IN-13) and the delegate signal exit code area: framework severity: minor g15office: pending @@ -19,11 +19,14 @@ files: - modules/bouncer/turnstile.go - modules/wristband/oidc_token.go - scripts/check-phase19.8.sh + - modules/bonfire/live.go + - modules/bonfire/output.go + - cmd/summer/runtime.go --- ## Context -The 19.8 code review (`.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md`, v0.3.1..v0.4.1) found 3 critical issues, 8 warnings and 9 info findings. Plan 19.8-05 fixed the critical issues and the warnings. The nine info findings below are deferred to this todo. Line numbers are the review's; locate the code by symbol. +The 19.8 code review (`.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md`, v0.3.1..v0.4.1) found 3 critical issues, 8 warnings and 9 info findings. Plan 19.8-05 fixed the critical issues and the warnings. The nine info findings below are deferred to this todo. Line numbers are the review's; locate the code by symbol. The re-review of those fixes (same file, gap-closure range dd12382..0ebed68) found six warnings and six info findings; quick task 261022-4z8 fixed the warnings, IN-14 and IN-15, and deferred IN-10..IN-13 and WR-10's exit-code suggestion to this todo (the sections after IN-09). ## IN-01: A forced-color console drops HideOnTTY records even when stderr is not a terminal @@ -96,3 +99,46 @@ The 19.8 code review (`.planning/phases/19.8-console-commands-overhaul/19.8-REVI **Problem:** The script hard-codes `sm-demo-app`, `sm-bm-app`, `sm-grzybyfunkcjonalne-app` and `sm-summercmsio-app`, and splits a consumer's name into two strings to get past the name check. CLAUDE.md says framework code never names a consuming application. Earlier phase gates do the same, but this framework repository is published. **Suggested fix:** Read the sibling paths from environment variables only, with no defaults, or move the cross-repository stages to a gate outside the framework repository. + +## IN-10: console.Error during a live view writes straight to the terminal and tears the view + +**File:** `modules/bonfire/output.go` (`console.Error`, `emit`) + +**Problem:** `Error` writes to `c.err`, and `emit` reroutes only writes to `c.out`. When stderr and stdout are the same terminal (the usual case), an `out.Error(...)` from inside `Spinner`, `Progress` or `Process` lands mid-frame. A pty run showed the spinner glyph left on the error line (`⠹ ✗ stderr-error-line`), with the renderer continuing on the next row. diary's console records on stderr do go above the view through `PrintAbove`, so the two stderr paths behave differently. + +**Suggested fix:** In `emit`, also reroute `w == c.err` when the console owns the running view and `c.err` is a terminal (`printAbove(c, s)`). + +## IN-11: A blank line printed during a live view is dropped + +**File:** `modules/bonfire/live.go` (`printAbove`, the `strings.TrimSuffix` of the block) + +**Problem:** `out.Println()` gives `"\n"`. `TrimSuffix` turns that into `""`, and bubbletea's `insertAbove` returns early for an empty string (cursed_renderer.go in bubbletea v2.1.0). `printAbove` still reports `true`, so the blank separator line is lost. It is printed when no view runs. + +**Suggested fix:** Send `" "` (or `"\u200b"`) for an empty block, or skip the trim when the block is only a newline. + +## IN-12: Signal relay edge cases: a signal during view start-up, and Windows + +**File:** `modules/bonfire/live.go` (`runLive`, `watchSignals`, `raise`) + +**Problem:** This comes from reading bubbletea v2.1.0; the reviewer did not reproduce it. + +1. **Start-up race.** `prog.Run` starts on its own goroutine just before the relay is registered. If a signal arrives before `Run` has created and started its renderer, `Kill` runs `shutdown(true)` while `p.renderer` is still nil. `shutdownOnce` then makes `Run`'s own `shutdown(killed)` a no-op after it has started the renderer and hidden the cursor, so the cursor stays hidden. +2. **Windows.** On Windows, `p.Signal(os.Interrupt)` fails, so `raise` calls `os.Exit(130)`. That skips the command's own handler (for example, `summer dev` stopping its serve child) and its deferred cleanup. + +**Suggested fix:** Before killing, wait until the program reports it has started (for example, have the wrapped model's `Init` close a `ready` channel). On platforms where `raise` cannot deliver the signal, return without exiting when another handler is registered. + +## IN-13: A delegate's stderr always goes to os.Stderr, bypassing the root's error writer + +**File:** `cmd/summer/runtime.go` (`appStreams`) + +**Problem:** Off a terminal, the child's stdout goes through `out`, but its stderr goes to the real `os.Stderr`, not to the error stream of the root that ran the command. For a summer root built with `NewRootIO(…, errW)` (tests, embedding), the app binary's error block cannot be captured. `TestDelegateExitCodePassthrough` therefore cannot check that the binary's own block was printed. + +**Suggested fix:** Pass the root's error writer through (for example, a `bonfire.ErrWriter(out)` accessor), defaulting to `os.Stderr`. + +## WR-10 follow-up: exit code 128+n for a delegate killed by a signal + +**File:** `cmd/summer/runtime.go` (`runAppCmd`) + +**Problem:** Plan 19.8-05 specified exit code 1 for an app binary that dies by a signal, and quick task 261022-4z8 kept it while masking the argv in summer's error block (WR-10). The re-review points out that Ctrl+C is the normal way to stop `summer serve` or `summer queue:work`: since the WR-01 relay re-raises SIGINT, a child that dies by the signal produces an ERROR block and exit 1, where shells and CI expect 130 (128+SIGINT). + +**Suggested fix:** Map signal deaths to 128+n: treat SIGINT and SIGTERM as already reported (`bonfire.Reported(bonfire.WithExitCode(err, 128+n))`, no block), and give other signals exit 128+n with summer's masked block (`argmask.Join(argv)`). `docs/console/introduction.md` documents exit code 1 for a signal death and must change with it, as must `TestDelegateExitCodePassthrough`'s signal subtests.