docs(19.8-05): complete review fixes plan

2026-10-22 02:45:15 +02:00
parent d311d029c3
commit 89c8325ac4
1 changed files with 255 additions and 0 deletions
--- a/.planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md
+++ b/.planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md
@@ -0,0 +1,255 @@
+---
+phase: 19.8-console-commands-overhaul
+plan: 05
+subsystem: console
+tags: [review-fixes, bonfire, bubbletea, signals, summer, delegates, dev, diary, lagoon, migrations, gate]
+status: complete
+gap_closure: true
+
+requires:
+  - phase: 19.8-01..04
+    provides: the shipped console (v0.4.0, v0.4.1) and its review 19.8-REVIEW.md
+provides:
+  - "CR-01..CR-03 and WR-01..WR-08 fixed, one commit each with a pinning test proven red without the fix"
+  - "Additive API: bonfire.WithExitCode, bonfire.Sanitize"
+  - "scripts/check-phase19.8.sh requires the 19 new pinning tests by name"
+  - "IN-01..IN-09 captured in .planning/todos/pending/2026-10-22-console-review-info-findings.md"
+affects: [19.8-06 release v0.4.2, bonfire live views, summer delegates, summer dev, diary daily files, lagoon migrate]
+
+actuals:
+  tokens: 21548    # chars/4 over the added and removed lines of this plan's diff (86191 chars)
+  tasks: 7
+  commits: 13      # MEASURED: git rev-list --count plan_head_before..plan_head_after
+plan_head_before: e8f5b1c89c639e5d1bcaf3096ddf3f05ea58864a
+plan_head_after: d311d029c3e86736f18fb7e186ca51073fd510b8
+
+tech-stack:
+  added: []
+  patterns:
+    - "Print above a bubbletea program by sending tea.Println(block)() straight to the program queue under a lock, never as a Cmd"
+    - "Live view registry: owner console + program, check-and-set claim, deferred unregister/finish/wait under one lock"
+    - "Signal relay scoped to a live view: Kill, unregister, signal.Stop, re-raise the same signal"
+    - "Re-executed test binary as a helper process (live view signals, delegate exit codes, dev stop grace)"
+    - "Exit-code carrier error (WithExitCode) honoured by Execute before the usage check"
+
+key-files:
+  created:
+    - modules/bonfire/live_signal_unix_test.go
+    - modules/diary/retire_internal_test.go
+    - .planning/todos/pending/2026-10-22-console-review-info-findings.md
+  modified:
+    - modules/bonfire/live.go
+    - modules/bonfire/output.go
+    - modules/bonfire/widgets.go
+    - modules/bonfire/helpers.go
+    - modules/bonfire/errors.go
+    - modules/bonfire/execute.go
+    - modules/bonfire/README.md
+    - modules/diary/daily.go
+    - modules/diary/diary.go
+    - modules/diary/console.go
+    - modules/diary/README.md
+    - modules/lagoon/migrations.go
+    - modules/lagoon/commands.go
+    - modules/lagoon/README.md
+    - cmd/summer/runtime.go
+    - internal/dev/watch.go
+    - docs/console/introduction.md
+    - docs/console/writing-commands.md
+    - docs/console/scaffolding.md
+    - docs/services/logging.md
+    - docs/setup/upgrading.md
+    - scripts/check-phase19.8.sh
+    - .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
+
+key-decisions:
+  - "CR-01: each PrintAbove block is one tea.Println message sent with Program.Send under liveMu (the direct-queue approach held against bubbletea v2.1.0's event loop); liveModel and printAboveMsg removed"
+  - "WR-01: signal handling lives inside runLive (relay SIGINT/SIGTERM only while a view runs, Kill, re-raise); generated main and bonfire.Execute's context unchanged"
+  - "CR-02: explicit additive bonfire.WithExitCode carrier; bonfire does not honour arbitrary ExitCode() errors, so plugin commands returning *exec.ExitError still exit 1"
+  - "WR-03: dev.ShutdownGrace (internal, 15s) shared by summer dev and summer's delegates"
+  - "WR-04: retry a failed daily open after one minute (writer clock) or on a new date; warning text kept, once per date"
+  - "WR-06: retired daily writers open, append and close per late record; diary.Close keeps reopen-on-next-record"
+  - "WR-05: diary's sanitizer moved to bonfire.Sanitize, diary calls it; IN-08 left to the todo"
+  - "WR-07: migrate reports pending-before minus pending-after, also on a partial failure"
+
+patterns-established:
+  - "Each review finding: pinning test red on unchanged code, fix, green, one fix(...) commit with test and docs"
+
+requirements-completed: [D-03, D-04, D-10, D-12, D-18, D-19, D-21, D-25, D-27, D-28, D-29]
+
+duration: 40min
+completed: 2026-10-22
+---
+
+# Phase 19.8 Plan 05: Code Review Fixes Summary
+
+**The 11 critical and warning findings of the 19.8 code review are fixed. Each fix is its own commit, and its pinning test fails when the fix is reverted. Lines printed above live views keep their order and none is lost. A live view takes lines only from the console that started it. Ctrl+C during a view restores the terminal. summer's delegates keep the app binary's exit code and give it the terminal. summer dev waits for serve's drain. Daily log files retry after a failed open, and replaced daily writers are retired. Error blocks, process views and tables strip control characters. Migrate and PendingMigrations read one list of migration sets. The wrong-directory hint masks secret flag values. The nine info findings are in one todo.**
+
+## Performance
+
+- **Duration:** about 40 min
+- **Started:** 2026-10-22T00:04:11Z
+- **Completed:** 2026-10-22T00:44:00Z
+- **Tasks:** 7 of 7
+- **Files modified:** 33 (1695 insertions, 282 deletions)
+
+## Commits per finding
+
+| Finding | Commit | Subject | Pinning tests |
+|---------|--------|---------|---------------|
+| CR-01 | b077dc3 | fix(bonfire): keep the order of lines printed above a live view and drain them before it ends | TestPrintAboveOrderedNoLoss (spinner, progress, process, concurrent_finish) |
+| WR-02 | 86e4e27 | fix(bonfire): reroute only the owning console's lines to a live view | TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters |
+| WR-01 | d151a25 | fix(bonfire): restore the terminal when a live view is interrupted | TestLiveViewRestoresTerminalOnSignal (default, own_handler) |
+| CR-02 | 323413c | fix(summer): pass the app binary's exit code through and print its error once | TestDelegateExitCodePassthrough, TestWithExitCode |
+| CR-03 | 9c89705 | fix(summer): give delegated commands the terminal and keep their stderr apart | TestDelegateStreams, TestAppStreams |
+| WR-03 | fc7706d | fix(dev): give serve the full shutdown grace before killing it | TestDevStopWaitsForGracefulShutdown |
+| WR-08 | 9b62285 | fix(bonfire): mask secret flag values in the wrong-directory hint | TestWrongDirectoryMasksSecrets |
+| WR-04 | c586c6f | fix(diary): retry a daily file that failed to open | TestDailyRetriesAfterFailure |
+| WR-06 | 39677f1 | fix(diary): retire replaced daily writers and serialise ApplyGlobals | TestRetiredDailyWriterKeepsNoFile, TestApplyGlobalsRebuildsOnce |
+| WR-05 | a9025c0 | fix(bonfire): strip control characters from error blocks, process views and tables | TestSanitize, TestRenderErrorStripsControl, TestProcessViewStripsControl, TestTableStripsControl |
+| WR-07 | 020fe83 | fix(lagoon): one migration set list for Migrate and PendingMigrations | TestMigrateAppliedLines (Docker) |
+| gate | 1bf64b0 | test(19.8): gate requires the review fixes' tests | (all of the above by name) |
+| records | d311d02 | docs(19.8): review fixes recorded, info findings captured as a todo | n/a |
+
+No other session's commit sits between them. None of these commits touches `.planning/STATE.md` or `.planning/state.json`.
+
+## RED output on the code before each fix
+
+Each pinning test was first run on the unchanged code (only the test added) during execution.
+
+- **CR-01** TestPrintAboveOrderedNoLoss: `spinner: run 0: line 15 is line-016, want line-015`; `progress: run 0: line 67 is line-068, want line-067`; `process: run 3: line 67 is line-068, want line-067`; `concurrent_finish: run 0: bg-016 printed 0 times` (lost), with `bg-014` printed before `bg-013`.
+- **WR-02** TestPrintlnStaysWithItsConsole: `console B's writer lacks "b-println"` / `console B's "b-println" printed above console A's view`. TestLiveViewSingleOwner: `round 0: 0 consoles printed their plain line to their own writer, want 1` (B's `[...] label-0` landed in A's view). TestLiveViewPanicUnregisters: `the panicked view is still registered`.
+- **WR-01** TestLiveViewRestoresTerminalOnSignal/default: `cursor left hidden: no show-cursor after the last hide-cursor in "\x1b[?25l\x1b[?2004h\r\x1b[J"`. The own_handler subtest passed on the old code too. It pins that the fix keeps the command's own handler working.
+- **CR-02** TestDelegateExitCodePassthrough/exit_code: `exit 1, want the binary's 2` and `summer printed an error block of its own: stderr "\n  ERROR  summer: migrate --bogus: exit status 2 ..."`. The test ran on the code after the pure refactor that factored out `runAppCmd`. TestWithExitCode is new API, so its RED is a compile failure.
+- **CR-03** TestDelegateStreams: `the binary's stderr landed in summer's stdout: "data-line\nlog-line\n"`.
+- **WR-03** TestDevStopWaitsForGracefulShutdown: `child was not allowed to finish its shutdown: signal: killed`, after 2.02s.
+- **WR-08** TestWrongDirectoryMasksSecrets: `Try = "cd ..; {bin} admin:create --email a@b.c --password hunter2", want "... --password ***"`, and the same for `--password=`, `--client-secret`, `--api-token=`, `--key` and `--PASSWD`.
+- **WR-04** TestDailyRetriesAfterFailure: `one_minute_later: read daily file: ... summer-2026-10-21.log: no such file or directory`; `next_date: ... summer-2026-10-22.log: no such file or directory`; `next_date_fails_too: 1 warnings over two failed dates, want 2`.
+- **WR-06** TestRetiredDailyWriterKeepsNoFile: `a late record through the replaced writer left its file open`. TestApplyGlobalsRebuildsOnce: `the channels were rebuilt 2 times, want 1`, then `3 times` (it failed in 2 of 3 runs, because the race is timing-dependent).
+- **WR-05** TestRenderErrorStripsControl: `error block carries "\x1b]0;pwned\a"` (with color on, the OSC title and `\x1b[2J` reached the message, explanation, Try and Caused-by lines). TestProcessViewStripsControl: `process view carries "\x1b]0;pwned\a"`. TestTableStripsControl: `terminal true: table carries "\x1b]0;pwned\a"` (the BEL also broke the box width), and the same with `terminal false`.
+- **WR-07** TestMigrateAppliedLines (Docker): `the set applied before the failure is not reported` and `framework set summercms.attach / summercms.cabana / summercms.deferred / summercms.conga not reported`.
+
+## RED replay from the commits (Task 7)
+
+For each fix commit C: a detached worktree at C under the session scratchpad, `git show C -- . ':!*_test.go' ':!*.md' | git apply -R`, then C's pinning tests by name, then the worktree removed.
+
+| Finding | Commit | Pinning test | RED kind | Result |
+|---------|--------|--------------|----------|--------|
+| CR-01 | b077dc3 | TestPrintAboveOrderedNoLoss | compile (`undefined: printLine`: TestLiveModels in the same test file uses the helper the fix adds) | failed on reverted code |
+| WR-02 | 86e4e27 | TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters | assertion | failed on reverted code |
+| WR-01 | d151a25 | TestLiveViewRestoresTerminalOnSignal | assertion (default: cursor left hidden) | failed on reverted code |
+| CR-02 | 323413c | TestDelegateExitCodePassthrough | compile (`undefined: runAppCmd`, factored out in the same commit; TestWithExitCode tests the new exported identifier) | failed on reverted code |
+| CR-03 | 9c89705 | TestDelegateStreams, TestAppStreams | compile (`undefined: appStreams`, newAppCmd arity) | failed on reverted code |
+| WR-03 | fc7706d | TestDevStopWaitsForGracefulShutdown | compile (`undefined: ShutdownGrace`) | failed on reverted code |
+| WR-08 | 9b62285 | TestWrongDirectoryMasksSecrets | assertion | failed on reverted code |
+| WR-04 | c586c6f | TestDailyRetriesAfterFailure | assertion | failed on reverted code |
+| WR-06 | 39677f1 | TestRetiredDailyWriterKeepsNoFile, TestApplyGlobalsRebuildsOnce | assertion | failed on reverted code |
+| WR-05 | a9025c0 | TestSanitize, TestRenderErrorStripsControl, TestProcessViewStripsControl, TestTableStripsControl | compile (`undefined: Sanitize`, the new exported identifier) | failed on reverted code |
+| WR-07 | 020fe83 | TestMigrateAppliedLines (Docker) | assertion | failed on reverted code |
+
+The plan expected compile REDs only for the two new exported identifiers (CR-02, WR-05). Four replays (CR-01, CR-02, CR-03, WR-03) compile-fail because the fix commit adds an unexported helper or constant that the test file uses. For all four, the section above records the assertion RED from running the test on the unchanged code before the fix.
+
+## Verification
+
+| Check | Result |
+|-------|--------|
+| `go vet ./...` | PASS |
+| `go test -short ./...` after every commit | PASS. The known flake TestOIDCSubjectIsRandomSmoke (deferred-items.md: a random UUID contains `ada`) failed once after the WR-07 commit and passed on one rerun |
+| `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...` (Docker, run directly) | PASS |
+| `go -C examples/hello test -count=1 ./...` | PASS |
+| `bash scripts/check-phase19.8.sh self-test` / `go` / `hello` / `docs` | PASS / PASS (the race stage over diary, sunscreen, bonfire and docsite included) / PASS / PASS |
+| `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh docker` | PASS (TestServeLifecycle; six lagoon tests including TestMigrateAppliedLines; three cabana tests) |
+| `go test -count=3 -race` for the CR-01, WR-02 and WR-06 tests | PASS |
+| Goldens (`modules/bonfire/testdata`) | unchanged |
+| Generated main (`internal/build`) and `cmd/summer/main.go` | untouched by every commit |
+
+Docker-backed tests: **run** (direct Docker socket; `sudo -u golem` was not needed).
+
+## API check
+
+The check took the `func` and `type` lines removed in non-test Go files under `modules/` between plan_head_before and HEAD, kept those with exported names, and removed those added again (`comm -23`). Three lines remain: the `Init`, `Update` and `View` methods of the **unexported** type `liveModel`, which CR-01 deleted. They are not public API. **No exported framework declaration was removed or changed its signature.** Exported identifiers added: `bonfire.WithExitCode(err error, code int) error` and `bonfire.Sanitize(s string) string`. The `Error` and `Unwrap` methods of the unexported type `exitCodeError` are also new. Unexported signatures changed: `runLive` now returns `(started bool, err error)`, and `newAppCmd` takes separate stdout and stderr writers.
+
+## Release impact (for plan 06)
+
+| Finding | Commit | Module | Exported API added | Require bump in plugins | Regenerate in apps |
+|---------|--------|--------|--------------------|-------------------------|--------------------|
+| CR-01 | b077dc3 | bonfire | none | no | no |
+| WR-02 | 86e4e27 | bonfire | none | no | no |
+| WR-01 | d151a25 | bonfire | none | no | no |
+| CR-02 | 323413c | bonfire, cmd/summer | bonfire.WithExitCode | no (no plugin uses it) | no |
+| CR-03 | 9c89705 | cmd/summer | none | no | no |
+| WR-03 | fc7706d | internal/dev, cmd/summer | none (dev.ShutdownGrace is internal) | no | no |
+| WR-08 | 9b62285 | bonfire | none | no | no |
+| WR-04 | c586c6f | diary | none | no | no |
+| WR-06 | 39677f1 | diary | none | no | no |
+| WR-05 | a9025c0 | bonfire, diary | bonfire.Sanitize | no (no plugin uses it) | no |
+| WR-07 | 020fe83 | lagoon | none | no | no |
+
+The generated main is unchanged, so no application needs a regenerate commit. Applications get the fixes by moving to v0.4.2 (`require` bump plus `go mod tidy`) and rebuilding their binary. The fixes reach plugins through the application's framework version, so no shared plugin needs a commit. Developers get the summer CLI fixes (CR-02, CR-03, WR-03) by reinstalling `summer` at v0.4.2. `docs/setup/upgrading.md` describes the delegate changes as "Since 0.4.2".
+
+## Deviations from Plan
+
+### Auto-fixed Issues
+
+**1. [Rule 3 - Blocking] WR-06 tests live in a new internal test file**
+- **Found during:** Task 4
+- **Issue:** TestRetiredDailyWriterKeepsNoFile and TestApplyGlobalsRebuildsOnce need unexported state (`dailyWriter`, `gens`, `install`), but daily_test.go and diary_test.go are the external package `diary_test`.
+- **Fix:** Added `modules/diary/retire_internal_test.go` (package diary). The gate runs it by name like the others.
+- **Commit:** 39677f1
+
+**2. [Rule 2 - Missing critical] Sanitize also applied to the spinner's and progress bar's current item**
+- **Found during:** Task 5
+- **Issue:** `Current(item)` shows the same kind of untrusted item name as the process view, in the spinner and progress frames.
+- **Fix:** `Sanitize(m.current)` in both views, alongside the planned call sites.
+- **Commit:** a9025c0
+
+**3. [Rule 1 - Bug] Delegate long help no longer says "summer fails when the app binary fails"**
+- **Found during:** Task 3 (CR-02)
+- **Fix:** The text now says "When the app binary fails, summer exits with its exit code."
+- **Commit:** 323413c
+
+**4. TestAppStreams takes the terminal decision as a parameter**
+- **Found during:** Task 3 (CR-03)
+- **Issue:** From package main, a terminal `bonfire.Output` can only be built on a pseudo-terminal.
+- **Fix:** `appStreams(out, terminal bool)`. runAppBinary passes `bonfire.IsTerminal(out)`, and the test checks both branches. TestDelegateStreams runs the whole delegate path with a shell-script binary in a temporary application.
+- **Commit:** 9c89705
+
+**5. Migrate's wording when `gdb.DB()` fails**
+- **Found during:** Task 6
+- **Issue:** Migrate now reads `migrationSets`, which calls `gdb.DB()` before any set runs. That error now reads `lagoon: migration sets: ...` instead of `lagoon: migrate queue: ...`. The wording of each set's migrate error (`system_files`, `backend admin`, `deferred bindings`, `queue`, plugin ID) is kept through a `label` field.
+- **Commit:** 020fe83
+
+**6. TestLiveModels "print above" subtest rewritten**
+- **Found during:** Task 1
+- **Issue:** `liveModel` and `printAboveMsg` were removed. The plan allowed this.
+- **Fix:** The subtest now asserts that `printLine` is one message rather than a command. The end-to-end subtest sends `printLine`.
+- **Commit:** b077dc3
+
+---
+
+**Total deviations:** 6 (1 blocking, 1 missing critical, 1 bug, 3 recorded choices). **Impact:** none on scope. No exported API changed beyond the two planned additions.
+
+## Issues Encountered
+
+- `gofmt -l` reports pre-existing formatting drift in files this plan did not create (`cmd/summer/runtime.go`'s rows struct, `cmd/summer/main.go`, several cabana, compass, tide and flare files). It is out of scope and left alone. The code this plan added is gofmt-clean.
+- The wristband flake from deferred-items.md recurred once (see Verification).
+
+## Known Stubs
+
+None.
+
+## Todo
+
+`.planning/todos/pending/2026-10-22-console-review-info-findings.md`: IN-01..IN-09, `severity: minor`, `area: framework`, `g15office: pending` (the orchestrator mirrors it). It notes that IN-08's fix now belongs in `bonfire.Sanitize` and that IN-07 must keep diary's use of `bonfire.Sanitize` in mind.
+
+## Suggested g15office comment (for the orchestrator)
+
+"Plan 19.8-05: 11 review findings fixed with pinning tests; info findings tracked in a todo; release v0.4.2 next."
+
+## Next Phase Readiness
+
+Plan 19.8-06 can tag v0.4.2 once the user gives the go-ahead. Nothing was pushed or tagged.
+
+## Self-Check: PASSED
+
+All created files exist on disk. All 13 commit hashes listed above are in `git log` (b077dc3, 86e4e27, d151a25, 323413c, 9c89705, fc7706d, 9b62285, c586c6f, 39677f1, a9025c0, 020fe83, 1bf64b0, d311d02).