diff --git a/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md index 34dd2297..2c5fe5bb 100644 --- a/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md +++ b/.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md @@ -5,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. diff --git a/.planning/todos/pending/2026-10-22-console-review-info-findings.md b/.planning/todos/pending/2026-10-22-console-review-info-findings.md new file mode 100644 index 00000000..2cc6f42e --- /dev/null +++ b/.planning/todos/pending/2026-10-22-console-review-info-findings.md @@ -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 ` 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. + +**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, "�")` 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.