docs(19.8): re-review of the code review fixes
This commit is contained in:
Jakub Zych
@@ -1,336 +1,253 @@
|
||||
---
|
||||
phase: 19.8-console-commands-overhaul
|
||||
reviewed: 2026-10-21T22:33:36Z
|
||||
reviewed: 2026-10-22T01:17:37Z
|
||||
depth: standard
|
||||
diff_range: v0.3.1..v0.4.1 (19.5 commits and the cooler test commit excluded)
|
||||
files_reviewed: 93
|
||||
diff_range: dd12382..0ebed68 (gap-closure plan 19.8-05, released as v0.4.2)
|
||||
files_reviewed: 31
|
||||
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
|
||||
- cmd/summer/runtime_test.go
|
||||
- docs/console/introduction.md
|
||||
- docs/console/scaffolding.md
|
||||
- docs/console/writing-commands.md
|
||||
- docs/services/logging.md
|
||||
- docs/setup/upgrading.md
|
||||
- internal/dev/watch.go
|
||||
- internal/docsite/check_commands.go
|
||||
- modules/beachcomber/beachcomber.go
|
||||
- modules/bonfire/command.go
|
||||
- modules/bonfire/docs.go
|
||||
- internal/dev/watch_test.go
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/errors_test.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/live_signal_unix_test.go
|
||||
- modules/bonfire/live_test.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/README.md
|
||||
- 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/bonfire/widgets_test.go
|
||||
- modules/diary/console.go
|
||||
- modules/diary/daily.go
|
||||
- modules/diary/daily_test.go
|
||||
- modules/diary/diary.go
|
||||
- modules/flare/commands.go
|
||||
- modules/flare/flare.go
|
||||
- modules/lagoon/attach/bucket.go
|
||||
- modules/lagoon/attach/thumb.go
|
||||
- modules/diary/README.md
|
||||
- modules/diary/retire_internal_test.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
|
||||
- modules/lagoon/pending_test.go
|
||||
- modules/lagoon/README.md
|
||||
- scripts/check-phase19.8.sh
|
||||
findings:
|
||||
critical: 3
|
||||
warning: 8
|
||||
info: 9
|
||||
total: 20
|
||||
critical: 0
|
||||
warning: 6
|
||||
info: 6
|
||||
total: 12
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 19.8: Code Review Report
|
||||
# Phase 19.8: Code Review Report (re-review after 19.8-05)
|
||||
|
||||
**Reviewed:** 2026-10-21T22:33:36Z
|
||||
**Reviewed:** 2026-10-22T01:17:37Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 93
|
||||
**Files Reviewed:** 31
|
||||
**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.
|
||||
This is an incremental re-review of the gap-closure commits `dd12382..HEAD` (b077dc3 to 020fe83, plus the gate and docs commits). I read every listed file in full where it changed. I checked the fixes against the bubbletea v2.1.0 sources (`Program.Send`, `Kill`, `shutdown`, `eventLoop`, `cursedRenderer.insertAbove`) and against pflag v1.0.10. I also ran two scratch programs under a pseudo-terminal (`script -qec`).
|
||||
|
||||
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.
|
||||
Verification:
|
||||
|
||||
The main defects are in three places:
|
||||
- `go vet` is clean on the touched packages.
|
||||
- `go test -short -race` passes for bonfire and diary.
|
||||
- The new summer and dev tests (`TestDelegate*`, `TestAppStreams`, `TestDevStopWaitsForGracefulShutdown`) pass.
|
||||
|
||||
- **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 <app command>` 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.
|
||||
All three critical findings from the first review are fixed. So are WR-01 to WR-06. WR-05, WR-07 and WR-08 are fixed as described, but each leaves a gap of the same kind elsewhere, so they are only partly closed (WR-12, WR-14, WR-10 and WR-11 below).
|
||||
|
||||
## Critical Issues
|
||||
The fixes introduced no new critical defect. The main new problems are:
|
||||
|
||||
### CR-01: Lines printed above a live view are reordered and can be lost
|
||||
- **Lost lines after cancellation.** A live view whose context is cancelled stays registered, so lines printed through it are dropped while `printAbove` reports success. I confirmed this on a pty.
|
||||
- **Signal-killed delegates.** Since the CR-02 fix, only a signal-killed delegate gets summer's error block. That block repeats the full argv, including a `--password` value, and the exit code is 1.
|
||||
- **Migration accounting.** `migrate:status --json` still uses its own plugin-only list. `migrate` reports a failure when only the read after a successful `Migrate` fails.
|
||||
|
||||
**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:
|
||||
New finding IDs continue after the first review's IDs (WR-09 and up, IN-10 and up). `19.8-REVIEW-DISPOSITION.md` drops a recorded decision when its ID is reused for a different finding, so the old IDs are not reused here.
|
||||
|
||||
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.
|
||||
## Prior Findings: Verified State
|
||||
|
||||
This affects every path that prints during a live view:
|
||||
| ID | Verified state | Evidence |
|
||||
|----|----------------|----------|
|
||||
| CR-01 | **Fixed** | `printAbove` sends one `printLineMessage` per call straight into the program's unbuffered queue while holding `liveMu` (live.go:477-487). The event loop handles it inline (`insertAbove`, bubbletea tea.go:888), so lines keep their order. `finishMsg` is sent under the same lock only after every earlier send has been received (live.go:387-393), so no line is lost when the view quits. On a pty the lines came out in order. `TestPrintAboveOrderedNoLoss` covers spinner, progress, process and a concurrent finish. A remaining gap after context cancellation is WR-09. |
|
||||
| CR-02 | **Fixed** | `runAppCmd` wraps an exited child as `Reported(WithExitCode(err, code))` (runtime.go:170-176), and `exitCode` honours the code (execute.go:76-85). `TestDelegateExitCodePassthrough` checks that the code is 2 and that summer prints no block of its own. The signal-killed branch has new problems, see WR-10. |
|
||||
| CR-03 | **Fixed** | `appStreams` gives the child `os.Stdout`/`os.Stderr` on a terminal and `out`/`os.Stderr` otherwise (runtime.go:155-160). The child's stderr no longer lands in summer's stdout, and on a terminal the child sees it and asks the production question. A minor gap is IN-13. |
|
||||
| WR-01 | **Fixed** | `watchSignals` relays SIGINT/SIGTERM, calls `prog.Kill()` (which restores the cursor) and re-raises the signal (live.go:410-466). `TestLiveViewRestoresTerminalOnSignal` covers both the default action and the command's own handler. Edge cases are in IN-12. |
|
||||
| WR-02 | **Fixed** | `activeView` records the owner, and `printAbove(c, …)` reroutes only the owner's output (live.go:477-481). The slot is claimed with check-and-set under `liveMu` (live.go:362-368), and cleanup is deferred, so it also runs on a panic (live.go:378-395). |
|
||||
| WR-03 | **Fixed** | `child.stop` waits `dev.ShutdownGrace` (15s), and summer's delegates use the same constant (watch.go:22-27, 243; runtime.go:185). |
|
||||
| WR-04 | **Fixed** | A failed open is retried after `dailyRetry` (1 minute) or on the next date, with one warning per date (daily.go:70-99). The test covers both paths. |
|
||||
| WR-05 | **Partly fixed** | Error blocks (every line, the Try lines and the causes), process-view names and errors, spinner and progress current items, failure lines and table cells now go through `Sanitize`. `Process`'s plain path still prints item names raw (WR-14). The deferred IN-08 (a raw C1 byte passes `Sanitize`'s fast path) now applies to every one of these sinks, not only diary. |
|
||||
| WR-06 | **Fixed** | Replaced writers are retired: a late record opens the file, appends and closes it again (daily.go:100-104, 166-174). `ApplyGlobals` holds `mu` for its whole run (diary.go:228-240). There is no reentrancy on `mu` from the handler path. |
|
||||
| WR-07 | **Partly fixed** | `Migrate` and `PendingMigrations` now share `migrationSets`, and the applied lines are computed from the pending list before and after the run. `Status` (used by `migrate:status`, and by its `--json` output) still walks only `plugins` (WR-12). A new error path is in WR-13. |
|
||||
| WR-08 | **Partly fixed** | `tryArgs` masks `--flag=value` and `--flag value` (errors.go:310-331). A value that starts with `-` is not masked, although pflag accepts it (WR-11). The same argv now leaks through the delegate's signal error (WR-10). |
|
||||
| IN-01..IN-09 | Deferred (todo `2026-10-22-console-review-info-findings.md`) | Not re-reported. IN-08 is now wider, as noted under WR-05. |
|
||||
|
||||
- `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(<pending prints>..., 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
|
||||
}
|
||||
```
|
||||
## Narrative Findings (AI reviewer)
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Ctrl+C during a spinner, progress bar or process view leaves the cursor hidden
|
||||
### WR-09: Lines printed through a live view after its context is cancelled are silently dropped
|
||||
|
||||
**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.
|
||||
**File:** `modules/bonfire/live.go:354-360, 477-487` (with `modules/bonfire/helpers.go:150-170`)
|
||||
**Issue:** `runLive` passes the command's context to bubbletea (`tea.WithContext(ctx)`). `Process` passes the caller's `ctx`. When that context is cancelled, the program shuts down: `Program.Send` now returns at once because `p.ctx` is done (bubbletea tea.go:1211-1216). However, `activeView` still points at the view until `work` returns. Only the deferred cleanup or the signal relay clears it, and the relay runs only for SIGINT/SIGTERM. Meanwhile:
|
||||
|
||||
### WR-02: Any console's Println and Printf are sent to whichever live view is running
|
||||
- `printAbove` hands the line to a dead program and still returns `true`, so the caller never falls back to a plain write.
|
||||
- This applies to `out.Println`/`Printf` data, to Success/Info/Warning lines on stdout, and to every diary console record routed through `bonfire.PrintAbove`.
|
||||
- These lines are lost for as long as the current `fn(ctx, item)` keeps running after the cancellation.
|
||||
- Because the program was killed, the final frame and the failed rows are never rendered either.
|
||||
|
||||
**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.
|
||||
**Runtime check:** a `Process` whose first item calls `cancel()`, waits 300 ms and then calls `out.Println("after-cancel-line")` and `bonfire.PrintAbove(...)` printed neither line in 3 of 3 pty runs. `PrintAbove` returned `true`. `out.Error`, which goes to stderr and so bypasses `printAbove`, did print.
|
||||
|
||||
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/<app> 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:
|
||||
This is the CR-01 failure mode (a line reported as printed but never shown) on a new path. It hits any `Process` caller that uses a deadline, a parent cancel or a non-signal shutdown.
|
||||
**Fix:** Have `printAbove` refuse a program that has stopped, and unregister the view when the program ends on its own:
|
||||
```go
|
||||
if d.disabledDate == date { d.mu.Unlock(); return len(p), nil }
|
||||
if err := d.open(date); err != nil { d.disabledDate = date; … }
|
||||
type liveView struct {
|
||||
owner *console
|
||||
prog *tea.Program
|
||||
done <-chan struct{} // the `finished` channel of runLive
|
||||
}
|
||||
|
||||
func printAbove(owner *console, s string) bool {
|
||||
liveMu.Lock()
|
||||
defer liveMu.Unlock()
|
||||
v := activeView
|
||||
if v == nil || (owner != nil && v.owner != owner) {
|
||||
return false
|
||||
}
|
||||
select {
|
||||
case <-v.done: // program gone (ctx cancelled, Run failed): print plainly
|
||||
activeView = nil
|
||||
return false
|
||||
default:
|
||||
}
|
||||
v.prog.Send(printLine(strings.TrimSuffix(s, "\n")))
|
||||
return true
|
||||
}
|
||||
```
|
||||
`Send` can still race with a cancellation that lands between the check and the send. To close that window, also watch `ctx.Done()` in a goroutine that clears `activeView` under `liveMu`. Add a pty or buffer test that cancels the context mid-item and asserts that later lines appear.
|
||||
|
||||
### WR-10: A delegate killed by a signal prints its full argv, including a --password value, and exits 1
|
||||
|
||||
**File:** `cmd/summer/runtime.go:170-178`
|
||||
**Issue:** After the CR-02 fix, every child failure that is not a normal exit returns `fmt.Errorf("summer: %s: %w", strings.Join(argv, " "), err)` unmasked. This covers death by a signal (`exit.Exited()` false) and every `cmd.Start` error. `RenderError` passes the message only through `sunscreen.Scrub`, which recognises Bearer, `sk-` and `x-api-key` shapes, so `summer admin:create --email a@b.c --password hunter2` prints `ERROR summer: admin:create --email a@b.c --password hunter2: signal: interrupt` on stderr. It happens when the user presses Ctrl+C, when a CI timeout sends SIGKILL, or on an OOM kill.
|
||||
|
||||
This is the leak WR-08 fixed for the wrong-directory hint, on a path that the CR-02 fix now makes the only one that prints summer's own block.
|
||||
|
||||
In addition, 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 (any delegate without its own handler, or one interrupted during a live view) now produces a spurious ERROR block and exit 1. Shells and CI expect 130 (128+SIGINT). The docs ("A binary killed by a signal gets an error block from summer and exit code 1") describe this behaviour but do not justify it.
|
||||
**Fix:** Mask the argv with the same helper the hint uses (export it, for example `bonfire.MaskArgs`), or name only the command word. Map signal deaths to 128+n, and treat SIGINT/SIGTERM as already reported:
|
||||
```go
|
||||
var exit *exec.ExitError
|
||||
if errors.As(err, &exit) {
|
||||
if ws, ok := exit.Sys().(syscall.WaitStatus); ok && ws.Signaled() {
|
||||
code := 128 + int(ws.Signal())
|
||||
if ws.Signal() == syscall.SIGINT || ws.Signal() == syscall.SIGTERM {
|
||||
return bonfire.Reported(bonfire.WithExitCode(err, code))
|
||||
}
|
||||
return bonfire.WithExitCode(fmt.Errorf("summer: %s: %w", argv[0], err), code)
|
||||
}
|
||||
…
|
||||
}
|
||||
```
|
||||
|
||||
### WR-05: Error blocks, process views and tables write untrusted text to the terminal without stripping escape sequences
|
||||
### WR-11: The wrong-directory hint leaves secret values that start with "-" unmasked
|
||||
|
||||
**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.
|
||||
**File:** `modules/bonfire/errors.go:325-329`
|
||||
**Issue:** `tryArgs` masks the next argument after `--password` only when it does not start with `-`. pflag takes the next argument as the value of a non-boolean flag whatever its first character is (pflag v1.0.10 flag.go:1020-1023, the `'--flag arg'` branch). So `bin/acme admin:create --email a@b.c --password -Xk9!q` runs with the password `-Xk9!q`, and the Try line then prints it in clear text. A dash is a common first character for generated passwords. The docs (writing-commands.md) promise that such values are masked.
|
||||
**Fix:** Mask the next argument whenever the flag takes a value. A secret-named long flag in `--name value` form always consumes the next argument, unless that argument is `--` or the flag is known to be boolean:
|
||||
```go
|
||||
if i+1 < len(args) && args[i+1] != "--" {
|
||||
b.WriteString(" ***")
|
||||
i++
|
||||
}
|
||||
```
|
||||
Add a test case with a value that starts with a dash.
|
||||
|
||||
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-12: migrate:status --json still drops the framework sets that migrate applies
|
||||
|
||||
### WR-06: Setup and ApplyGlobals close daily files that other goroutines may still be writing through
|
||||
**File:** `modules/lagoon/migrations.go:141-160`, `modules/lagoon/commands.go:270-289`
|
||||
**Issue:** WR-07's point was that the commands which report on migrations must read the list `Migrate` applies. `Status` still walks only `plugins`. `migrate:status --json` then emits pending refs only when `ref.Plugin == row.Plugin`. The pending migrations of `summercms.attach`, `summercms.cabana`, the deferred-bindings set and the queue set (all returned by `PendingMigrations`) are therefore silently omitted, both from the JSON (whose example says "pending migrations included") and from the table.
|
||||
|
||||
**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.
|
||||
After a framework upgrade that adds a River queue migration, `migrate --pretend` lists it but `migrate:status --json` reports nothing pending. A deploy gate that checks for `"applied": false` passes when it should not.
|
||||
**Fix:** Build `Status` from `migrationSets(gdb, plugins)` (skipping or including empty plugin sets consistently), so all three commands read one list. Extend `TestMigrateStatusJSON` to assert that a framework set's pending migration appears.
|
||||
|
||||
`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.
|
||||
### WR-13: migrate reports failure after a successful Migrate when the follow-up read fails
|
||||
|
||||
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`.
|
||||
**File:** `modules/lagoon/commands.go:173-180`
|
||||
**Issue:** After `Migrate` returns, the command calls `PendingMigrations` again. If that read fails (a dropped connection, a statement timeout, a permission problem on a history table), it returns `errors.Join(migrateErr, err)`, which is non-nil even when `migrateErr` is nil. A migration that was fully applied then:
|
||||
|
||||
### WR-07: PendingMigrations keeps its own copy of Migrate's migration sets
|
||||
- prints no applied lines and no `Applied N migrations`;
|
||||
- exits 1.
|
||||
|
||||
**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:
|
||||
A deploy pipeline will treat it as failed and may roll back or alert, even though the schema change is in place.
|
||||
**Fix:** When `migrateErr == nil`, a failed follow-up read must not turn the run into a failure. Fall back to reporting `pending` as applied, with a warning:
|
||||
```go
|
||||
after, err := PendingMigrations(gdb, plugins)
|
||||
applied := pending
|
||||
if err == nil {
|
||||
applied = appliedSince(pending, after)
|
||||
} else if migrateErr == nil {
|
||||
out.Warning(fmt.Sprintf("Could not re-read the migration history: %v", err))
|
||||
} else {
|
||||
return errors.Join(migrateErr, err)
|
||||
}
|
||||
```
|
||||
|
||||
- the serve startup summary,
|
||||
- `migrate --pretend`,
|
||||
- the production question's count,
|
||||
- the "N applied" lines,
|
||||
- `migrate:status --json`.
|
||||
### WR-14: Process's plain path prints item names without Sanitize
|
||||
|
||||
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 …`).
|
||||
**File:** `modules/bonfire/helpers.go:183` (also `modules/bonfire/live.go:313-317`, the `operation` label)
|
||||
**Issue:** The WR-05 fix sanitises item names in the live process view (live.go:292) and in failure lines (`failureText`). The plain path, however, prints a successful item with `out.Success(item)`, and `status` writes it unchanged. The plain path runs whenever stdout is a terminal but the view does not animate: with `-n`, inside another live view (now the normal fallback when `runLive` reports `started == false`), or with a forced color profile. In those cases an item name that holds an escape sequence (a remote file name, an admin-typed title, a post slug) reaches the terminal unchanged. The same item is cleaned on one path and raw on the other.
|
||||
**Fix:** `out.Success(Sanitize(item))` in `Process`, or sanitise inside `console.status` and `console.Error` so every status line is covered regardless of caller. Add a plain-path case to `TestProcessViewStripsControl`.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: A forced-color console drops HideOnTTY records even when stderr is not a terminal
|
||||
### IN-10: console.Error during a live view writes straight to the terminal and tears the view
|
||||
|
||||
**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).
|
||||
**File:** `modules/bonfire/output.go:232-238`
|
||||
**Issue:** `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.
|
||||
**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-02: PeekGlobals and cobra disagree on some argument forms
|
||||
### IN-11: A blank line printed during a live view is dropped
|
||||
|
||||
**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`).
|
||||
**File:** `modules/bonfire/live.go:485`
|
||||
**Issue:** `out.Println()` gives `"\n"`. `TrimSuffix` turns that into `""`, and bubbletea's `insertAbove` returns early for an empty string (cursed_renderer.go:776-778). `printAbove` still reports `true`, so the blank separator line is lost. It is printed when no view runs.
|
||||
**Fix:** Send `" "` (or `"\u200b"`) for an empty block, or skip the trim when the block is only a newline.
|
||||
|
||||
### IN-03: isUsage recognises cobra's unknown-command error by its text
|
||||
### IN-12: Signal relay edge cases: a signal during view start-up, and Windows
|
||||
|
||||
**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`.
|
||||
**File:** `modules/bonfire/live.go:370-375, 427-436, 454-466`
|
||||
**Issue:** This comes from reading bubbletea v2.1.0; I did not reproduce it.
|
||||
|
||||
### IN-04: The --json flag is read two different ways
|
||||
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.
|
||||
|
||||
**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.
|
||||
**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-05: summer's scaffolding result lines are now hidden by -q
|
||||
### IN-13: A delegate's stderr always goes to os.Stderr, bypassing the root's error writer
|
||||
|
||||
**File:** `cmd/summer/main.go:126,334,401`
|
||||
**Issue:** `created <path>` and `added <dir>` 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`.
|
||||
**File:** `cmd/summer/runtime.go:155-160`
|
||||
**Issue:** 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.
|
||||
**Fix:** Pass the root's error writer through (for example, a `bonfire.ErrWriter(out)` accessor), defaulting to `os.Stderr`.
|
||||
|
||||
### IN-06: fang replaces the root help function that NewRootIO installs
|
||||
### IN-14: The ErrReported doc comment still says the exit code is 1
|
||||
|
||||
**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.
|
||||
**File:** `modules/bonfire/errors.go:73-75`
|
||||
**Issue:** The comment says "the error block prints nothing and the exit code is 1". With `WithExitCode`, a Reported error can now exit with any code from 1 to 255 (the README was updated, the comment was not).
|
||||
**Fix:** "… and the exit code is 1, or the code of WithExitCode."
|
||||
|
||||
### IN-07: diary pulls the whole TUI stack into security modules
|
||||
### IN-15: runLive builds the bubbletea program before it knows it may run
|
||||
|
||||
**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, "<22>")` 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.
|
||||
**File:** `modules/bonfire/live.go:354-366`
|
||||
**Issue:** `tea.NewProgram` runs before the slot check. It calls `context.WithCancel(ctx)`, and with `TEA_TRACE` set it opens a trace file. When the slot is taken (`started == false`, now the normal fallback for nested widgets), that cancel is never called. With `Process`'s cancellable `ctx`, the child context stays registered on the parent until the parent ends, and the trace file descriptor leaks. A `Process` nested in a loop accumulates both.
|
||||
**Fix:** Claim the slot first and construct the program only after `activeView` is set (or call `prog.Kill()` on the losing path).
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-21T22:33:36Z_
|
||||
_Reviewed: 2026-10-22T01:17:37Z_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
|
||||
Reference in New Issue
Block a user