golem15/summercms
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(19.8): review fixes recorded, info findings captured as a todo

Browse Source 
- CR-01..CR-03 and WR-01..WR-08 fixed with their commits (19.8-05)
- IN-01..IN-09 deferred to one pending todo (minor, framework)
This commit is contained in:
Jakub Zych
2026-10-22 02:43:48 +02:00
parent 1bf64b009f
commit d311d029c3
2 changed files with 139 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

82
.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
View File

@@ -5,85 +5,85 @@ titles: json
findings:
- id: CR-01
severity: critical
disposition: open
disposition: fixed
title: "Lines printed above a live view are reordered and can be lost"
- id: CR-02
severity: critical
disposition: open
disposition: fixed
title: "summer delegates print a second error block and turn exit code 2 into 1"
- id: CR-03
severity: critical
disposition: open
disposition: fixed
title: "summer delegates hide the terminal from the app and merge its stderr into stdout"
- id: WR-01
severity: warning
disposition: open
disposition: fixed
title: "Ctrl+C during a spinner, progress bar or process view leaves the cursor hidden"
- id: WR-02
severity: warning
disposition: open
disposition: fixed
title: "Any console's Println and Printf are sent to whichever live view is running"
- id: WR-03
severity: warning
disposition: open
disposition: fixed
title: "summer dev kills serve 2 seconds after SIGTERM, but serve drains for up to 10"
- id: WR-04
severity: warning
disposition: open
disposition: fixed
title: "One failed open switches a daily log file off until the process exits"
- id: WR-05
severity: warning
disposition: open
disposition: fixed
title: "Error blocks, process views and tables write untrusted text to the terminal without stripping escape sequences"
- id: WR-06
severity: warning
disposition: open
disposition: fixed
title: "Setup and ApplyGlobals close daily files that other goroutines may still be writing through"
- id: WR-07
severity: warning
disposition: open
disposition: fixed
title: "PendingMigrations keeps its own copy of Migrate's migration sets"
- id: WR-08
severity: warning
disposition: open
disposition: fixed
title: "The wrong-directory hint repeats every argument, including a --password value, on stderr"
- id: IN-01
severity: info
disposition: open
disposition: deferred
title: "A forced-color console drops HideOnTTY records even when stderr is not a terminal"
- id: IN-02
severity: info
disposition: open
disposition: deferred
title: "PeekGlobals and cobra disagree on some argument forms"
- id: IN-03
severity: info
disposition: open
disposition: deferred
title: "isUsage recognises cobra's unknown-command error by its text"
- id: IN-04
severity: info
disposition: open
disposition: deferred
title: "The --json flag is read two different ways"
- id: IN-05
severity: info
disposition: open
disposition: deferred
title: "summer's scaffolding result lines are now hidden by -q"
- id: IN-06
severity: info
disposition: open
disposition: deferred
title: "fang replaces the root help function that NewRootIO installs"
- id: IN-07
severity: info
disposition: open
disposition: deferred
title: "diary pulls the whole TUI stack into security modules"
- id: IN-08
severity: info
disposition: open
disposition: deferred
title: "The console sanitizer passes raw C1 bytes through on its fast path"
- id: IN-09
severity: info
disposition: open
disposition: deferred
title: "The phase gate names consuming applications"
open: 20
open: 0
total: 20
recorded: 2026-10-21T22:35:50.040Z
---
@@ -92,26 +92,26 @@ recorded: 2026-10-21T22:35:50.040Z
| Finding | Severity | Disposition | Source |
|---------|----------|-------------|--------|
| CR-01 | critical | open | - |
| CR-02 | critical | open | - |
| CR-03 | critical | open | - |
| WR-01 | warning | open | - |
| WR-02 | warning | open | - |
| WR-03 | warning | open | - |
| WR-04 | warning | open | - |
| WR-05 | warning | open | - |
| WR-06 | warning | open | - |
| WR-07 | warning | open | - |
| WR-08 | warning | open | - |
| IN-01 | info | open | - |
| IN-02 | info | open | - |
| IN-03 | info | open | - |
| IN-04 | info | open | - |
| IN-05 | info | open | - |
| IN-06 | info | open | - |
| IN-07 | info | open | - |
| IN-08 | info | open | - |
| IN-09 | info | open | - |
| CR-01 | critical | fixed | fix b077dc3 (19.8-05) |
| CR-02 | critical | fixed | fix 323413c (19.8-05) |
| CR-03 | critical | fixed | fix 9c89705 (19.8-05) |
| WR-01 | warning | fixed | fix d151a25 (19.8-05) |
| WR-02 | warning | fixed | fix 86e4e27 (19.8-05) |
| WR-03 | warning | fixed | fix fc7706d (19.8-05) |
| WR-04 | warning | fixed | fix c586c6f (19.8-05) |
| WR-05 | warning | fixed | fix a9025c0 (19.8-05) |
| WR-06 | warning | fixed | fix 39677f1 (19.8-05) |
| WR-07 | warning | fixed | fix 020fe83 (19.8-05) |
| WR-08 | warning | fixed | fix 9b62285 (19.8-05) |
| IN-01 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-02 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-03 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-04 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-05 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-06 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-07 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-08 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
| IN-09 | info | deferred | todo .planning/todos/pending/2026-10-22-console-review-info-findings.md |
Dispositions: `open` (recorded, not yet triaged), `fixed`, `skipped`, `deferred`.
Set `deferred` by hand and put the reason in the Source cell; both are preserved. A `|` in the reason is kept as prose and escaped on the next run.

98
.planning/todos/pending/2026-10-22-console-review-info-findings.md Normal file
View File

@@ -0,0 +1,98 @@
---
created: 2026-10-22T00:43:48.000Z
title: Console review info findings (IN-01..IN-09)
area: framework
severity: minor
g15office: pending
files:
- modules/diary/console.go
- modules/diary/diary.go
- modules/bonfire/globals.go
- modules/bonfire/errors.go
- modules/bonfire/root.go
- modules/bonfire/execute.go
- modules/cabana/token_commands.go
- modules/lighthouse/centrifugo/commands.go
- modules/surf/routelist_command.go
- modules/lagoon/commands.go
- cmd/summer/main.go
- modules/bouncer/turnstile.go
- modules/wristband/oidc_token.go
- scripts/check-phase19.8.sh
---
## Context
The 19.8 code review (`.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md`, v0.3.1..v0.4.1) found 3 critical issues, 8 warnings and 9 info findings. Plan 19.8-05 fixed the critical issues and the warnings. The nine info findings below are deferred to this todo. Line numbers are the review's; locate the code by symbol.
## IN-01: A forced-color console drops HideOnTTY records even when stderr is not a terminal
**File:** `modules/diary/console.go` (`consoleHandler`, the HideOnTTY check)
**Problem:** The styled `consoleHandler` drops every `HideOnTTY` record. It is also used when color is forced (`--ansi`, `FORCE_COLOR`) on a non-terminal stderr. So 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. The HideOnTTY docs promise that only a terminal console skips them.
**Suggested fix:** Check `isTerminal(w)` once when the handler is built, and drop the records only when it is true (the rule `ttyFilter` already uses).
## IN-02: PeekGlobals and cobra disagree on some argument forms
**File:** `modules/bonfire/globals.go` (`PeekGlobals`)
**Problem:** `PeekGlobals` ignores `--quiet=true`, `--no-interaction=true`, `--verbose=2` and `--ansi=false`, but pflag honours them. It also reads a command flag value that looks like a global (`--name -q`) as a global, where pflag takes it as the flag's value. As a result the pre-boot diary setup, the error block's `-v` chain and the passthrough globals can differ from what the command runs with.
**Suggested fix:** Parse the `--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` (`isUsage`, `RenderError`)
**Problem:** 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.
**Suggested fix:** Turn cobra's error into a `UsageError` in one place (a root `Args` or `ValidArgsFunction` check, or match it once in `Execute` before rendering), and test it with `errors.As`.
## IN-04: The --json flag is read two different ways
**Files:** `modules/cabana/token_commands.go`, `modules/lighthouse/centrifugo/commands.go`, `modules/surf/routelist_command.go` vs `modules/lagoon/commands.go` (`flagOn`)
**Problem:** cabana, centrifugo and route:list compare the value with `== "true"`, while lagoon uses `strconv.ParseBool`. So `--json=1` works for `migrate:status` but not for `admin:token:list`.
**Suggested fix:** Add one bonfire helper (for example `bonfire.FlagBool(in, name)`) and use it everywhere. It would be an additive API, so plugins keep building.
## IN-05: summer's scaffolding result lines are now hidden by -q
**File:** `cmd/summer/main.go` (the scaffolding commands' `Created` and `Added` lines)
**Problem:** `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.
**Suggested 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
**Files:** `modules/bonfire/root.go`, `modules/bonfire/execute.go`
**Problem:** `fang.Execute` calls `root.SetHelpFunc` with fang's own help function, so under `Execute` bonfire's root help function is dead code. Today it is unreachable, because `Execute` strips `-h` and `--help` when no command word is given. A future path that calls the root's help would print fang's layout instead of the Artisan list.
**Suggested fix:** Drop fang and call `root.ExecuteContext` with `RenderError` directly (bonfire already turns off every fang feature except the error handler), or restore the root help function after fang sets its own.
## IN-07: diary pulls the whole TUI stack into security modules
**Files:** `modules/diary/diary.go`, `modules/bouncer/turnstile.go`, `modules/wristband/oidc_token.go`
**Problem:** `diary` imports `bonfire`, which imports cobra, fang, bubbletea, bubbles and huh. So bouncer and wristband, which log through diary, link the console UI stack, and a security review of them has more code to cover.
**Suggested fix:** Move `ColorMode`, `DetectProfile`, `Palette`, `PrintAbove` and now `Sanitize` into a small leaf package (or into diary itself), so diary no longer imports bonfire. Since plan 19.8-05, diary's console channel also calls `bonfire.Sanitize` (WR-05). Keep `bonfire.Sanitize` as the exported entry point when the leaf package is introduced, because it is public API since v0.4.2.
## IN-08: The console sanitizer passes raw C1 bytes through on its fast path
**File:** `modules/bonfire/errors.go` (`bonfire.Sanitize`, formerly diary's `sanitize`)
**Problem:** A lone `0x9b` byte (8-bit CSI) is invalid UTF-8. `range` decodes it as `U+FFFD`, which is not a control character, so the function returns the original string with the raw byte in it. Terminals that interpret 8-bit C1 controls then see an escape sequence.
**Suggested fix:** Since plan 19.8-05 the sanitizer is `bonfire.Sanitize`, used by error blocks, process views, tables and diary's console. Run `strings.ToValidUTF8(s, "<22>")` before the control-character scan there, and add the case to `TestSanitize`.
## IN-09: The phase gate names consuming applications
**File:** `scripts/check-phase19.8.sh` (the `DEMO_APP`, `BM_APP`, `GRZYBY_APP` and `SUMMERCMSIO_APP` defaults)
**Problem:** The script hard-codes `sm-demo-app`, `sm-bm-app`, `sm-grzybyfunkcjonalne-app` and `sm-summercmsio-app`, and splits a consumer's name into two strings to get past the name check. CLAUDE.md says framework code never names a consuming application. Earlier phase gates do the same, but this framework repository is published.
**Suggested fix:** Read the sibling paths from environment variables only, with no defaults, or move the cross-repository stages to a gate outside the framework repository.