From dd123821d2efe40986a215d187faca13c13c6654 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 00:35:55 +0200 Subject: [PATCH] docs(19.8): code review of the console overhaul --- .../19.8-REVIEW.md | 336 ++++++++++++++++++ 1 file changed, 336 insertions(+) create mode 100644 .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md diff --git a/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md new file mode 100644 index 00000000..e23b0995 --- /dev/null +++ b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md @@ -0,0 +1,336 @@ +--- +phase: 19.8-console-commands-overhaul +reviewed: 2026-10-21T22:33:36Z +depth: standard +diff_range: v0.3.1..v0.4.1 (19.5 commits and the cooler test commit excluded) +files_reviewed: 93 +files_reviewed_list: + - cmd/summer/docs.go + - cmd/summer/main.go + - cmd/summer/parity.go + - cmd/summer/runtime.go + - docs/examples/blog/console/publish.go + - examples/hello/main.go + - examples/hello/plugins/greeter/plugin.go + - internal/build/artifact.go + - internal/build/build.go + - internal/build/manifest.go + - internal/build/new.go + - internal/build/scaffold.go + - internal/dev/watch.go + - internal/docsite/check_commands.go + - modules/beachcomber/beachcomber.go + - modules/bonfire/command.go + - modules/bonfire/docs.go + - modules/bonfire/errors.go + - modules/bonfire/execute.go + - modules/bonfire/globals.go + - modules/bonfire/helpers.go + - modules/bonfire/help.go + - modules/bonfire/list.go + - modules/bonfire/live.go + - modules/bonfire/output.go + - modules/bonfire/prompts.go + - modules/bonfire/root.go + - modules/bonfire/selector.go + - modules/bonfire/styles.go + - modules/bonfire/version.go + - modules/bonfire/widgets.go + - modules/bouncer/turnstile.go + - modules/cabana/account.go + - modules/cabana/account_preferences.go + - modules/cabana/account_tokens.go + - modules/cabana/actions.go + - modules/cabana/api_tokens.go + - modules/cabana/auth.go + - modules/cabana/commands.go + - modules/cabana/contracts.go + - modules/cabana/crud.go + - modules/cabana/field_dependent.go + - modules/cabana/field_file.go + - modules/cabana/field_type.go + - modules/cabana/http.go + - modules/cabana/log.go + - modules/cabana/partial_render.go + - modules/cabana/query.go + - modules/cabana/relation_field.go + - modules/cabana/reorder.go + - modules/cabana/singleton.go + - modules/cabana/token_commands.go + - modules/cabana/token_guard.go + - modules/conga/client.go + - modules/conga/commands.go + - modules/conga/conga.go + - modules/conga/worker.go + - modules/cooler/cooler.go + - modules/cooler/memory.go + - modules/cooler/ratelimit.go + - modules/cooler/rediscache.go + - modules/cooler/sitecache.go + - modules/diary/channels.go + - modules/diary/component.go + - modules/diary/config.go + - modules/diary/console.go + - modules/diary/daily.go + - modules/diary/diary.go + - modules/flare/commands.go + - modules/flare/flare.go + - modules/lagoon/attach/bucket.go + - modules/lagoon/attach/thumb.go + - modules/lagoon/commands.go + - modules/lagoon/connection.go + - modules/lagoon/encrypted.go + - modules/lagoon/fill.go + - modules/lagoon/keygen.go + - modules/lagoon/migrations.go + - modules/lagoon/purge.go + - modules/lagoon/transaction.go + - modules/lighthouse/centrifugo/client.go + - modules/lighthouse/centrifugo/commands.go + - modules/lighthouse/lighthouse.go + - modules/phrasebook/translator.go + - modules/postcard/drivers.go + - modules/postcard/mailer.go + - modules/sunscreen/sunscreen.go + - modules/surf/accesslog.go + - modules/surf/routelist_command.go + - modules/surf/serve.go + - modules/surf/summary.go + - modules/wristband/oidc_token.go + - scripts/check-phase19.8.sh +findings: + critical: 3 + warning: 8 + info: 9 + total: 20 +status: issues_found +--- + +# Phase 19.8: Code Review Report + +**Reviewed:** 2026-10-21T22:33:36Z +**Depth:** standard +**Files Reviewed:** 93 +**Status:** issues_found + +## Summary + +I reviewed the phase's changed hunks in the 93 non-test source files (`git diff v0.3.1..v0.4.1`). I read the new modules (diary, and bonfire's globals, errors, execute, live, prompts, selector and list) in full. I checked the bubbletea v2.1.0, fang v2.0.1 and gorm v1.31.2 sources wherever bonfire, diary or lagoon depend on how they behave. I confirmed one finding at runtime: a scratch program under a pseudo-terminal (`script -qec`) shows that lines printed during a live view come out in the wrong order and are sometimes lost. Items already listed in `deferred-items.md` are not reported again. + +The request log is clean. It logs the route pattern and never the path, query, headers or remote address. The GORM bridge logs SQL with placeholders. The startup summary shows the database only as host, port and name. The redaction wrapping sits on every diary leaf. + +The main defects are in three places: + +- **Printing above live views.** Lines printed while a spinner, progress bar or process view runs are reordered and sometimes dropped. Any console's output can be redirected to the terminal that owns the view. +- **summer delegates.** Every `summer ` merges the app's stderr into stdout and never gives the app a terminal. The production confirmation is skipped, `--json` output gets log lines mixed in, two error blocks are printed, and exit code 2 becomes 1. +- **Process lifecycle.** Ctrl+C during a spinner leaves the cursor hidden. `summer dev` kills serve 8 seconds before serve's drain deadline. One failed open of a daily log file switches file logging off until the process exits. + +## Critical Issues + +### CR-01: Lines printed above a live view are reordered and can be lost + +**File:** `modules/bonfire/live.go:316-323` (with `printAboveLines` at 377-387 and `runLive` at 365-372) +**Issue:** `printAboveLines` sends one `printAboveMsg` per line, and `liveModel.Update` answers each one with `tea.Println(...)`, which is a `tea.Cmd`. bubbletea v2 runs every Cmd in its own goroutine (`handleCommands`, tea.go:745-757), and that goroutine sends the resulting `printLineMessage` back into the event loop. Two consequences follow: + +1. **Order is not kept.** Successive lines race each other back into the event loop. +2. **Lines can be dropped.** `runLive` sends `finishMsg` as soon as `work` returns, and `finishMsg` ends in `tea.Quit`. A print Cmd still in flight then reaches a loop that has already exited, and its line is never printed. + +This affects every path that prints during a live view: + +- `Output.Println` and `Printf`, which carry command data (tokens, JSON); +- status lines sent through `emit`; +- `liveProgress.Fail` failure lines (widgets.go:130-133), which are sent just before the work function returns; +- every diary console record logged while a view runs (console.go:166). + +**Runtime check:** a program printing `line-00` to `line-19` with `out.Println` inside `out.Spinner` on a pty printed them out of order in 5 of 5 runs (for example `line-01 line-00 line-02 line-04 line-03 …`) and lost a line in 1 of 40 runs. +**Fix:** Deliver the lines through the event loop in order, and drain them before quitting. For example, buffer them in the model and flush them as a single ordered `tea.Sequence`; on finish, quit only after that sequence: +```go +type liveModel struct{ inner tea.Model } + +func (m liveModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + if lines, ok := msg.(printAboveMsg); ok { + // one message per PrintAbove call; tea.Sequence runs the prints in order + var cmds []tea.Cmd + for _, l := range strings.Split(string(lines), "\n") { + cmds = append(cmds, tea.Println(l)) + } + return m, tea.Sequence(cmds...) + } + inner, cmd := m.inner.Update(msg) + m.inner = inner + return m, cmd +} +``` +Ordering across separate PrintAbove calls, and draining before quit, still need one serial path. Either have `runLive` send `finishMsg` only after an acknowledgement message that is queued behind the pending prints, or keep a FIFO in the model and, on `finishMsg`, return `tea.Sequence(..., tea.Quit)` instead of `tea.Quit`. Send each PrintAbove call as one message (not one per line). +Add a pty test that prints N lines during each widget and asserts all N arrive in order. + +### CR-02: summer delegates print a second error block and turn exit code 2 into 1 + +**File:** `cmd/summer/runtime.go:146-150` +**Issue:** When the app binary fails, `runAppBinary` returns `fmt.Errorf("summer: %s: %w", argv, *exec.ExitError)`. `bonfire.Execute` then: + +1. renders a second ERROR block (`summer: migrate --bogus: exit status 2`) under the error block the app binary already printed; +2. maps the error to `ExitFailure`, because `isUsage` does not match an `*exec.ExitError`. + +So `summer migrate --bogus` exits 1, not 2. That breaks the D-18 exit-code contract for every delegate the docs present as the normal entry point ("summer fails when the app binary fails", runtime.go:93). Nothing in `cmd/summer/runtime_test.go` covers a failing child. +**Fix:** Treat the child's failure as already reported and pass its exit code through. For example, add an exit-code carrier that `exitCode` honours: +```go +var ee *exec.ExitError +if errors.As(err, &ee) { + return bonfire.Reported(bonfire.WithExitCode(err, ee.ExitCode())) // new: exitCode() checks interface{ ExitCode() int } +} +``` + +### CR-03: summer delegates hide the terminal from the app and merge its stderr into stdout + +**File:** `cmd/summer/runtime.go:163-168` +**Issue:** `newAppCmd` sets both `cmd.Stdout` and `cmd.Stderr` to `out`. That is the bonfire console, not an `*os.File`, so the child gets a pipe on both streams, and every byte of the child's stderr lands on summer's stdout. Since this phase that has three effects: + +1. **The production confirmation is never asked.** In the child, `Interactive()` needs stdout to be a terminal. `summer migrate`, `summer migrate:rollback`, `summer deferred:purge` and `summer queue:clear` run in production without the D-28 question, even when run in a terminal. +2. **Data output gets log lines mixed in.** Records the child logs to stderr (diary records, error blocks) are mixed into its data on stdout. `summer route:list --json > routes.json` and `summer admin:token:create … --json` capture any INFO plugin boot record (deferred-items lists such records on every application command) or WARN line inside the JSON. `summer key:generate -q` captures any warning together with the key. +3. **Styled output is lost.** `summer serve` and `summer queue:work` never print their styled summaries, and the delegate's widgets never animate. + +`internal/dev/watch.go:81-88` already fixes the same problem for `summer dev` by passing `os.Stdout` and `os.Stderr` on a terminal. The delegates were not given the same fix. +**Fix:** Give the child the process's own streams, and keep stderr separate: +```go +cmd.Stdout, cmd.Stderr = os.Stdout, os.Stderr +if !bonfire.IsTerminal(out) { // tests and Call roots: keep the injected writer + cmd.Stdout, cmd.Stderr = out, os.Stderr +} +``` + +## Warnings + +### WR-01: Ctrl+C during a spinner, progress bar or process view leaves the cursor hidden + +**File:** `modules/bonfire/widgets.go:33,51`, `modules/bonfire/live.go:349-355`, `cmd/summer/main.go:28,76`, `internal/build/build.go:140` +**Issue:** The live views run with `tea.WithoutSignalHandler()` and `tea.WithInput(nil)`, and `Spinner` and `Progress` pass `context.Background()`. Neither `summer` nor the generated main installs a signal handler (`bonfire.Execute(context.Background(), …)`), so Ctrl+C terminates the process with SIGINT's default action. bubbletea hides the cursor when it starts (`ESC[?25l`, confirmed in the pty run) and only shows it again on a clean exit. `summer build`, which always shows a spinner on a terminal, and every plugin command that uses `Progress` therefore leave the user's terminal without a cursor after Ctrl+C. +**Fix:** Run `Execute` under `signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)` and give the widgets the command's context (keep it on the console in `PersistentPreRunE`). Alternatively, have `runLive` register its own SIGINT handler that calls `prog.Kill()` (which restores the terminal) and then re-raises the signal. + +### WR-02: Any console's Println and Printf are sent to whichever live view is running + +**File:** `modules/bonfire/output.go:192-212`, `modules/bonfire/live.go:329-339,345-373` +**Issue:** `activeProg` is a single process-wide value. `console.Println` and `Printf` call `printAboveLines` without checking that this console owns the view (`emit` checks only `w == c.out`). Any other console that prints while a view runs therefore has its data written to the terminal above that view. This includes a `bonfire.Call` root writing into a caller's `bytes.Buffer`, a nested command, or a console whose stdout is a file. For example, a `Process` loop that calls `bonfire.Call(..., &buf)` gets an empty buffer. + +Two more problems sit in the same code: + +- **Check-then-act race.** `liveActive()` (in `console.live`) and the registration in `runLive` are separate steps. Two widgets started concurrently both start programs, and the first one to finish sets `activeProg = nil` while the other is still running. +- **No cleanup on panic.** `runLive` unregisters without `defer`, so a panic in `work` leaves `activeProg` pointing at an orphaned program. + +**Fix:** Store the owning console (or its `out` writer) together with the program, and reroute only when `c == owner`. Register under `liveMu` with a check-and-set that falls back to plain lines when a view is already active. Use `defer` for the unregister, `finishMsg` and wait steps. + +### WR-03: summer dev kills serve 2 seconds after SIGTERM, but serve drains for up to 10 + +**File:** `internal/dev/watch.go:227-238` +**Issue:** Since this phase `summer dev` runs `bin/ serve`, whose shutdown drains requests and the in-process worker for up to 10 seconds and then prints the shutdown report. Before, it ran the binary with no arguments. `child.stop` still sends SIGKILL after 2 seconds. A restart or Ctrl+C that arrives during a slow request or job therefore kills serve before its report is printed (as `summer dev`'s Long help promises). Jobs that are killed remain `running` in River until its stuck-job rescuer picks them up. `cmd/summer/runtime.go:155` (`appShutdownGrace = 15s`) is longer than serve's 10-second timeout for exactly this reason. +**Fix:** Use the same grace in dev, for example `case <-time.After(appShutdownGrace):` with a shared constant longer than serve's 10-second shutdown context. + +### WR-04: One failed open switches a daily log file off until the process exits + +**File:** `modules/diary/daily.go:226-243` +**Issue:** When `open` fails, `disabled = true` is set and never cleared. For a long-running `serve` or `queue:work`, a transient failure at midnight rotation (ENOSPC for a moment, EMFILE during a traffic spike, a directory briefly removed by logrotate or a backup tool) stops all file logging until the next restart. Only one WARN record is written, possibly to journald. The degrade-instead-of-fail rule (T-19.8-06) was meant to keep boot from failing. A server should not lose its WARN and ERROR file trail for days because of one transient failure. +**Fix:** Disable the writer only for the date that failed, retrying on the next date (or after a backoff of about a minute), and log once per failed date: +```go +if d.disabledDate == date { d.mu.Unlock(); return len(p), nil } +if err := d.open(date); err != nil { d.disabledDate = date; … } +``` + +### WR-05: Error blocks, process views and tables write untrusted text to the terminal without stripping escape sequences + +**File:** `modules/bonfire/errors.go:145-169`, `modules/bonfire/live.go:286-289`, `modules/bonfire/widgets.go:142-166` +**Issue:** T-19.8-03 strips control characters only in diary's console handler. `RenderError` passes messages, explanations and the `-v` cause chain through `sunscreen.Scrub` only, and Scrub redacts credential shapes but leaves ESC and other control characters in place. The process view prints `it.err.Error()` and item names unchanged, and `Table` prints cell values unchanged. When the profile allows color, `colorprofile.Writer` passes those sequences through. + +The text can come from outside the operator's control: database or remote-service error text (the centrifugo health error, upstream HTTP bodies), and admin-supplied token names shown by `admin:token:list`. Such text can move the cursor, rewrite earlier lines or set the window title on the operator's terminal. +**Fix:** Export diary's `sanitize` (for example as `bonfire.Sanitize`, with diary calling it) and apply it to every error-block line, process-view item and error, and table cell before styling. + +### WR-06: Setup and ApplyGlobals close daily files that other goroutines may still be writing through + +**File:** `modules/diary/diary.go:156-179,216-227` +**Issue:** `install` swaps the state and then closes the old channels' daily writers. A goroutine that loaded the old state just before the swap still writes through the old `dailyWriter`. Its `Write` finds `file == nil` and reopens the file (`daily.go:233`). Nothing ever closes that writer again, so the descriptor stays open until the GC finalizer closes it. + +`ApplyGlobals` runs in `PersistentPreRunE`, after plugins have booted. By then plugin goroutines (cache, realtime, the worker) may already be logging, so the window is real. + +There is also a check-then-act race: `ApplyGlobals` reads `current` outside `mu`, so two concurrent calls can both rebuild the channels. +**Fix:** Mark the old writers closed for good (`d.closed = true`, with `Write` returning `len(p), nil` once it is set) and do not reopen them. Alternatively, reference-count the old state and close it after in-flight `Handle` calls finish. Take `mu` for the whole of `ApplyGlobals`. + +### WR-07: PendingMigrations keeps its own copy of Migrate's migration sets + +**File:** `modules/lagoon/migrations.go:221-245` vs `modules/lagoon/migrations.go:64-115`; `modules/lagoon/commands.go:139-171` +**Issue:** `migrationSets` re-lists the framework sets and the plugin loop by hand, and `Migrate` does not use it. When a set is added to `Migrate` (as the queue and deferred sets were), several things go wrong without any failing test: + +- the serve startup summary, +- `migrate --pretend`, +- the production question's count, +- the "N applied" lines, +- `migrate:status --json`. + +The "applied" lines are also printed from the list computed before `Migrate` ran, not from what it actually applied. +**Fix:** Make `Migrate` iterate over `migrationSets(gdb, plugins)`, so both functions read one list. Optionally, recompute the pending list after `Migrate` and report the difference. + +### WR-08: The wrong-directory hint repeats every argument, including a --password value, on stderr + +**File:** `modules/bonfire/errors.go:220-224` (via `Fail`, `modules/bonfire/execute.go:346-351`) +**Issue:** When an application binary fails outside its root, `wrongDirectory` builds the Try line from all of `os.Args[1:]`. For example, `cd bin && ./acme admin:create --email a@b.c --password hunter2` fails at `compass.Open`, and the error block prints `cd ..; bin/acme admin:create --email a@b.c --password hunter2`. The deprecated `--password` flag is still accepted, so a password that was only in the shell history now also reaches CI and deploy logs that capture stderr. `sunscreen.Scrub` does not catch it, because it recognises only Bearer, `sk-` and `x-api-key` shapes. +**Fix:** Mask the values of flags whose names look like secrets (`password`, `secret`, `token`, `key`) in the Try line (`--password=***`). Alternatively, print only the command word (`cd ..; bin/acme admin:create …`). + +## Info + +### IN-01: A forced-color console drops HideOnTTY records even when stderr is not a terminal + +**File:** `modules/diary/console.go:117-122` +**Issue:** The styled `consoleHandler` drops every `HideOnTTY` record unconditionally. It is also used when color is forced (`--ansi`, `FORCE_COLOR`) on a non-terminal stderr. With stdout on a terminal and stderr redirected (`FORCE_COLOR=1 bin/acme serve 2>serve.log`), the "startup summary" and "listening on" records never reach the file, although the HideOnTTY docs promise that only a terminal console skips them. +**Fix:** Check `isTerminal(w)` once when the handler is built, and drop the records only when it is true (the same rule `ttyFilter` uses). + +### IN-02: PeekGlobals and cobra disagree on some argument forms + +**File:** `modules/bonfire/globals.go:52-81` +**Issue:** `--quiet=true`, `--no-interaction=true`, `--verbose=2` and `--ansi=false` are ignored by `PeekGlobals` but honoured by pflag. A command flag value that looks like a global (`--name -q`) is the reverse: pflag takes it as the flag's value, `PeekGlobals` reads it as a global. The pre-boot diary setup, the error block's `-v` chain and passthrough globals can therefore differ from what the command runs with. +**Fix:** Parse `--flag=value` forms with `strconv.ParseBool` and `strconv.Atoi`, or re-parse the args with a throwaway pflag FlagSet that holds only the global flags (`ParseErrorsWhitelist.UnknownFlags = true`). + +### IN-03: isUsage recognises cobra's unknown-command error by its text + +**File:** `modules/bonfire/errors.go:41-47,119` +**Issue:** Any error whose message starts with "unknown command" exits 2 and is rewritten as `Command "…" is not defined.`. A plugin error worded that way would be misreported. +**Fix:** Turn cobra's error into a `UsageError` in one place (for example a root `Args` or `ValidArgsFunction` check, or by matching it once in `Execute` before rendering), and test it with `errors.As`. + +### IN-04: The --json flag is read two different ways + +**File:** `modules/cabana/token_commands.go:150,217,299`, `modules/lighthouse/centrifugo/commands.go`, `modules/surf/routelist_command.go` vs `modules/lagoon/commands.go` (`flagOn`) +**Issue:** cabana, centrifugo and route:list compare the value with `== "true"`, while lagoon uses `strconv.ParseBool`. As a result `--json=1` works for `migrate:status` but not for `admin:token:list`. +**Fix:** Add one bonfire helper (for example `bonfire.FlagBool(in, name)`) and use it everywhere. + +### IN-05: summer's scaffolding result lines are now hidden by -q + +**File:** `cmd/summer/main.go:126,334,401` +**Issue:** `created ` and `added ` used to print through `Printf`. They now print through `Success` as `Created …` and `Added …`, so `-q` hides them and their wording changed. Scripts that read the created paths stop working without notice. +**Fix:** Print the created paths with `Println` as data (D-27), or mention the change in `docs/setup/upgrading.md`. + +### IN-06: fang replaces the root help function that NewRootIO installs + +**File:** `modules/bonfire/root.go:139-141`, `modules/bonfire/execute.go:327-337` +**Issue:** `fang.Execute` calls `root.SetHelpFunc(fang's helpFunc)` (fang.go:140), so under `Execute` bonfire's root help function is dead code. Today `Execute` strips `-h` and `--help` when there is no command word, so the replacement is not reachable. A future path that calls the root's help would print fang's layout, not the Artisan list. +**Fix:** Set the root help function again after `fang.Execute` sets its own (not possible from outside), or drop fang and call `root.ExecuteContext` with `RenderError` directly. bonfire already turns off every fang feature except the error handler. + +### IN-07: diary pulls the whole TUI stack into security modules + +**File:** `modules/diary/diary.go:21-22`, `modules/bouncer/turnstile.go`, `modules/wristband/oidc_token.go` +**Issue:** `diary` imports `bonfire`, which imports cobra, fang, bubbletea, bubbles and huh. bouncer's README now says its "only framework dependency is diary", but in practice the security primitive links the console UI stack. That widens the code a security review of bouncer and wristband has to cover. +**Fix:** Move `ColorMode`, `DetectProfile`, `Palette` and `PrintAbove` into a small leaf package (or into diary itself) so that diary does not import bonfire. + +### IN-08: The console sanitizer passes raw C1 bytes through on its fast path + +**File:** `modules/diary/console.go:278-288` +**Issue:** A lone `0x9b` byte (8-bit CSI) is invalid UTF-8. `range` decodes it as `U+FFFD`, which is not a control character, so `sanitize` returns the original string with the raw byte in it. Terminals that interpret 8-bit C1 controls then see an escape sequence. +**Fix:** Run `strings.ToValidUTF8(s, "�")` before the control-character scan. + +### IN-09: The phase gate names consuming applications + +**File:** `scripts/check-phase19.8.sh:71-77,229` +**Issue:** 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. CLAUDE.md says framework code never names a consuming application. Earlier phase gates do the same, so the precedent exists, but this framework repository is published. +**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. + +--- + +_Reviewed: 2026-10-21T22:33:36Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_