Compare commits
21 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
ead8e42211 | ||
|
|
cf061857a9 | ||
|
|
8020b177ff | ||
|
|
89c8325ac4 | ||
|
|
d311d029c3 | ||
|
|
1bf64b009f | ||
|
|
020fe838ea | ||
|
|
a9025c0d12 | ||
|
|
39677f1819 | ||
|
|
c586c6f8a9 | ||
|
|
9b62285654 | ||
|
|
fc7706dfff | ||
|
|
9c89705747 | ||
|
|
323413c741 | ||
|
|
d151a252c5 | ||
|
|
86e4e270d3 | ||
|
|
b077dc3598 | ||
|
|
e8f5b1c89c | ||
|
|
dd123821d2 | ||
|
|
0b1d7e5a79 | ||
|
|
6c2164ad38 |
@@ -1470,7 +1470,7 @@ Plans:
|
||||
4. A sweep of all existing commands (framework modules and shared plugins) checks them against an Artisan feature checklist and fixes the gaps found, without changing command logic or breaking existing names and flags. The checklist covers confirmation with `--force` on destructive actions, `--pretend`/dry-run where it applies, progress bars and tables for bulk work, and `--json` output for scripting. Long-running commands such as `serve` and `queue:work` print a startup summary of what booted (environment, plugins, database and migrations state, queues, scheduler, mail, cache and realtime drivers, URLs) and report what they did on shutdown. One-shot commands end with a result line and a meaningful exit code.
|
||||
5. The `diary` and `bonfire` READMEs, the docs console and logging pages, the summercms.io landing site and its roadmap presentation reflect the new console. The new code has unit tests, delivered in the phase's last plan.
|
||||
|
||||
**Plans:** 3/4 plans executed
|
||||
**Plans:** 5/6 plans executed
|
||||
|
||||
Plans:
|
||||
**Wave 1**
|
||||
@@ -1483,7 +1483,9 @@ Plans:
|
||||
- [x] 19.8-03-PLAN.md — sweep of every command (checklist, hints, serve/queue:work/dev summaries and reports, http request log), docs, landing and roadmap deck; release checkpoint, then plugin commits and app pointer bumps
|
||||
|
||||
**Wave 4** *(blocked on Wave 3 completion)*
|
||||
- [ ] 19.8-04-PLAN.md — unit and golden tests for all new code, `scripts/check-phase19.8.sh` gate across repos, post-release fixes go-ahead
|
||||
- [x] 19.8-04-PLAN.md — unit and golden tests for all new code, `scripts/check-phase19.8.sh` gate across repos, post-release fixes go-ahead
|
||||
- [x] 19.8-05-PLAN.md — gap closure: fixes for the code review's critical and warning findings (live-view output order, summer delegates' exit codes and terminal, Ctrl+C cursor, dev shutdown grace, diary file retry, escape stripping, migration sets), each with a pinning test
|
||||
- [ ] 19.8-06-PLAN.md — gap closure: v0.4.2 release go-ahead, tag and changelog, demo roadmap sync, full gate
|
||||
|
||||
### Phase 20: Demo instance with every shared plugin
|
||||
|
||||
@@ -1588,7 +1590,7 @@ Phases execute in numeric order: 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 →
|
||||
| 19.6. Two-factor auth and device flow in sm-user-plugin | 0/TBD | Not started | - |
|
||||
| 19.6.1. Backend Administrators screen (INSERTED) | 0/TBD | Not started | - |
|
||||
| 19.7. Generic MCP module | 0/TBD | Not started | - |
|
||||
| 19.8. Console commands overhaul (INSERTED) | 3/4 | In Progress| |
|
||||
| 19.8. Console commands overhaul (INSERTED) | 5/6 | In Progress| |
|
||||
| 20. Demo instance with every shared plugin | 0/TBD | Not started | - |
|
||||
| 30. Płytarium cutover | 0/TBD | Not started | - |
|
||||
| 31. Chat polish against WavePath clients | 0/TBD | Not started | - |
|
||||
|
||||
@@ -14,12 +14,12 @@ requires:
|
||||
provides:
|
||||
- "scripts/check-phase19.8.sh: fail-closed gate (self-test, go, hello, docs, plugins, apps, landing, docker) with --self-test"
|
||||
- "Unit and golden tests for diary, sunscreen, bonfire, docsite and every swept framework command"
|
||||
- "fix(bonfire): PeekGlobals reads clustered shorthands (post-release fix, in v0.4.0)"
|
||||
- "fix(bonfire): PeekGlobals reads clustered shorthands, released as SummerCMS v0.4.1 with its changelog post"
|
||||
affects: [19.8 verification, the next framework tag]
|
||||
|
||||
actuals:
|
||||
tokens: 53163 # chars/4 over the added lines of this plan's diff (goldens excluded)
|
||||
tasks: 4 # Tasks 1-4 done; Task 5 is the checkpoint, Task 6 waits on its reply
|
||||
tasks: 6 # Tasks 1-4, the Task 5 checkpoint and Task 6 (release v0.4.1)
|
||||
commits: 11 # MEASURED: git rev-list --count plan_head_before..plan_head_after
|
||||
plan_head_before: b906a31c368ad6a59ea8f6ac82121714ffad20da
|
||||
plan_head_after: 72b8383c1022a993b66c6cd8b4f739bdee062246
|
||||
@@ -69,7 +69,7 @@ key-files:
|
||||
- cmd/summer/runtime_test.go
|
||||
|
||||
key-decisions:
|
||||
- "PeekGlobals reads a single-dash cluster letter by letter like pflag (-qn, -vq, -nvv) and stops at the first non-global letter; released as a post-release fix pending the Task 5 reply"
|
||||
- "PeekGlobals reads a single-dash cluster letter by letter like pflag (-qn, -vq, -nvv) and stops at the first non-global letter; released as SummerCMS v0.4.1"
|
||||
- "BuildVersion reads build info through an unexported package variable (refactor, no behaviour change) so tests pin the tagged, replaced and go.work cases"
|
||||
- "Interactive confirmation is tested on a real pseudo-terminal (syscall ioctls on /dev/ptmx) instead of adding a pty dependency or a bonfire test seam"
|
||||
- "The landing stage runs nuxt generate before the TAP suite and checks the roadmap snapshot against origin/master; TestTerminalCommands (deployed one-liner) is opt-in"
|
||||
@@ -135,7 +135,7 @@ coverage:
|
||||
|
||||
duration: 69min
|
||||
completed: 2026-10-21
|
||||
status: checkpoint
|
||||
status: complete
|
||||
---
|
||||
|
||||
# Phase 19.8 Plan 04: unit tests and the phase gate Summary
|
||||
@@ -264,9 +264,37 @@ None. No new endpoint or input path; the goldens hold fixed fake values (secret
|
||||
|
||||
No g15office tool is available in this executor session: the Phase 19.8 milestone task was not commented. Suggested comment once the Task 5 reply is in: "Plan 19.8-04 executed: phase gate green (--all with Docker), one post-release fix (PeekGlobals clustered shorthands); waiting on verification."
|
||||
|
||||
## Task 6: published
|
||||
|
||||
- **Range check** (after `git fetch`): `origin/master..v0.4.1` held 30 commits, all `.planning/` documents (this plan's and the 19.6, 19.6.1 and 19.7 sessions', `c1f5ded` included, approved with the reply) or this plan's code commits; no other session's code. `go vet ./...` and `go test -short ./...` green before tagging.
|
||||
- **Tag:** annotated **v0.4.1** (tag object `520d0c7`) on `8bacc22` (master tip when tagged: another session's `docs(19.6.1): map phase to its g15office task` landed after the reply commit `2ca68f8`; it is planning-only and inside the approved range). Contains `efb043e` and every test commit of this plan.
|
||||
- **Pushed:** framework `master` `99b6df4..8bacc22` (pushed as the exact tagged commit, `8bacc22:refs/heads/master`), then the tag. Nothing force-pushed. `git ls-remote --exit-code --tags origin v0.4.1` exits 0.
|
||||
- **Changelog:** https://summercms.io/changelog/summercms-0-4-1 (post id 36, published_at the tag date 2026-10-21T22:15:15Z), notes under `## Fixed` and `## Changed`, no planning IDs; the token was read from its file and never printed.
|
||||
- **Plugins and applications:** nothing to publish (no require bumps, no regenerate commits; the table needs none).
|
||||
- This SUMMARY's finalising commit is made after the push and stays local until the next push.
|
||||
|
||||
### Gate after the release
|
||||
|
||||
`PHASE198_REQUIRE_DOCKER=1 TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh --all` at `8bacc22`:
|
||||
|
||||
```
|
||||
PASS self-test
|
||||
PASS go
|
||||
PASS hello
|
||||
PASS docs
|
||||
PASS plugins
|
||||
PASS apps
|
||||
NOT RUN TestTerminalCommands (the deployed one-liner end to end: needs network, Docker and ports 54320 and 8080; set SUMMERCMS_TERMINAL_CHECK=1)
|
||||
FAIL landing
|
||||
```
|
||||
|
||||
`landing` fails on one check only: `sync-roadmap: plugins/demo/content/ROADMAP.md differs from origin/master`. The push published the other sessions' ROADMAP.md changes (19.6.1 inserted, 19.6 and 19.7 updates: 42 insertions, 6 deletions), so sm-demo-app's roadmap snapshot is now one sync behind origin, as deferred-items.md (plan 03) predicted ("re-sync after those commits are pushed"). Every other landing check passed in the same run (nuxt generate, 38/38 TAP, the terminal and roadmap Go tests), and with `ROADMAP_REF=99b6df4` (the previous origin/master) the landing stage passes. `--all` stops at the first red stage, so the docker stage was run on its own afterwards: **PASS** (TestServeLifecycle, the five lagoon checklist tests, the three cabana token JSON tests).
|
||||
|
||||
**Open for the user:** in sm-demo-app run `scripts/sync-roadmap.sh` and commit the refreshed `plugins/demo/content/ROADMAP.md` (a sibling-repository commit, not covered by the release reply, so not made here). After that `--all` is fully green.
|
||||
|
||||
## Next
|
||||
|
||||
Task 5 reply ("release vX.Y.Z", "push-only" or "hold"), then Task 6 under it, then `/msd-verify-work 19.8`.
|
||||
Re-sync the demo roadmap snapshot (above), then `/msd-verify-work 19.8`.
|
||||
|
||||
## Self-Check: PASSED
|
||||
|
||||
|
||||
456
.planning/phases/19.8-console-commands-overhaul/19.8-05-PLAN.md
Normal file
456
.planning/phases/19.8-console-commands-overhaul/19.8-05-PLAN.md
Normal file
@@ -0,0 +1,456 @@
|
||||
---
|
||||
phase: 19.8-console-commands-overhaul
|
||||
plan: 05
|
||||
type: execute
|
||||
wave: 1
|
||||
depends_on: []
|
||||
gap_closure: true
|
||||
files_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/bonfire/live_test.go
|
||||
- modules/bonfire/live_signal_unix_test.go
|
||||
- modules/bonfire/widgets_test.go
|
||||
- modules/bonfire/errors_test.go
|
||||
- modules/diary/daily.go
|
||||
- modules/diary/diary.go
|
||||
- modules/diary/console.go
|
||||
- modules/diary/README.md
|
||||
- modules/diary/daily_test.go
|
||||
- modules/diary/diary_test.go
|
||||
- modules/lagoon/migrations.go
|
||||
- modules/lagoon/commands.go
|
||||
- modules/lagoon/README.md
|
||||
- modules/lagoon/pending_test.go
|
||||
- cmd/summer/runtime.go
|
||||
- cmd/summer/runtime_test.go
|
||||
- internal/dev/watch.go
|
||||
- internal/dev/watch_test.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
|
||||
- .planning/todos/pending/2026-10-22-console-review-info-findings.md
|
||||
- .planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md
|
||||
autonomous: true
|
||||
requirements: [D-03, D-04, D-10, D-12, D-18, D-19, D-21, D-25, D-27, D-28, D-29]
|
||||
user_setup: []
|
||||
|
||||
estimate:
|
||||
tokens: 200000
|
||||
raw_tokens: 200000
|
||||
tasks: 7
|
||||
confidence: low
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "CR-01 (D-03, D-27): lines printed while a Spinner, Progress or Process view runs (Output.Println and Printf, status lines, Progress Fail lines, bonfire.PrintAbove from diary) appear in the order they were printed and none is lost when the view ends; a line printed from another goroutine while the view finishes appears once, after the final frame (TestPrintAboveOrderedNoLoss)."
|
||||
- "WR-02 (D-03): only the console that started a live view sends its Println, Printf and status lines above it; every other console (a bonfire.Call buffer, a nested command, a console on a file) writes to its own writer. At most one view registers at a time (check and set under one lock; a widget that loses falls back to plain lines), and a panic in the work function still unregisters, finishes and waits for the view (TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters)."
|
||||
- "WR-01 (D-03): SIGINT or SIGTERM during a live view kills the view (cursor and terminal restored) and then still ends the process with the signal's default action, or reaches the command's own signal handler when it has one. The generated main and bonfire.Execute's context are unchanged (TestLiveViewRestoresTerminalOnSignal)."
|
||||
- "CR-02 (D-18): `summer <delegate>` exits with the app binary's exit code (an exit 2 from the binary stays 2) and prints no second error block; a binary killed by a signal still gets summer's error block and exit 1. The carrier is the additive bonfire.WithExitCode, honoured by bonfire.Execute (TestDelegateExitCodePassthrough, TestWithExitCode)."
|
||||
- "CR-03 (D-27, D-28, D-04): on a terminal the delegated binary gets summer's own stdout and stderr (so its production question, styled summaries and widgets work); off a terminal its stdout goes through summer's Output and its stderr to summer's stderr, never into stdout (TestDelegateStreams, TestAppStreams)."
|
||||
- "WR-03 (D-25, D-29): summer dev sends serve SIGTERM and waits the same grace as the delegates (longer than serve's 10-second drain) before it kills it (TestDevStopWaitsForGracefulShutdown)."
|
||||
- "WR-04 (D-10): a daily file that fails to open is skipped and retried one minute later by the writer's clock, or at the first record of a new date, whichever comes first; the warning is logged once per date (TestDailyRetriesAfterFailure; TestDailyUnwritable and TestDailyDegradesSmoke stay green)."
|
||||
- "WR-06 (D-10): daily writers replaced by Setup, Bootstrap, ApplyGlobals or Reset are retired: a late record through them is appended and the file closed again, so no descriptor stays open; diary.Close keeps its documented reopen-on-next-record behaviour for the installed writers. ApplyGlobals holds the diary lock for its whole run, so concurrent calls rebuild the channels once (TestRetiredDailyWriterKeepsNoFile, TestApplyGlobalsRebuildsOnce)."
|
||||
- "WR-05 (D-12, D-18): error blocks (message, explanation, Try lines, -v chain), process-view item names and errors, and table cells strip control characters other than tab before styling, through the additive bonfire.Sanitize, which diary's console handler also uses (TestRenderErrorStripsControl, TestProcessViewStripsControl, TestTableStripsControl, TestSanitize; TestConsoleStripsControlChars stays green)."
|
||||
- "WR-07 (D-21, D-04): Migrate applies exactly the sets migrationSets lists (one list for Migrate, PendingMigrations, --pretend, the production question, the startup summary and migrate:status --json), keeping each set's error wording; migrate's 'N applied' lines and its result line count what Migrate actually applied, also when it fails partway (TestMigrateAppliedLines, Docker)."
|
||||
- "WR-08 (D-19): the wrong-directory Try line keeps the command and its other arguments but replaces the value of every flag whose name contains password, passwd, secret, token or key with ***, in both the --flag value and --flag=value forms (TestWrongDirectoryMasksSecrets)."
|
||||
- "Each CR and WR finding is fixed in its own commit that contains its pinning test, and replaying the commit without its non-test, non-doc changes makes the test fail (RED replay in Task 7). 19.8-REVIEW-DISPOSITION.md marks CR-01..CR-03 and WR-01..WR-08 fixed with their commit; IN-01..IN-09 are deferred to one pending todo (severity minor, area framework)."
|
||||
- "No exported identifier of the framework is removed or changes its signature; the only API additions are bonfire.WithExitCode and bonfire.Sanitize. The generated main is unchanged, so no shared plugin needs a require bump and no application a regenerate commit unless the SUMMARY's Release impact table says otherwise."
|
||||
artifacts:
|
||||
- path: "modules/bonfire/live.go"
|
||||
provides: "ordered, drained printing above live views; owner-checked registration; signal-safe teardown"
|
||||
contains: "func runLive("
|
||||
- path: "modules/bonfire/live_test.go"
|
||||
provides: "CR-01 and WR-02 pinning tests"
|
||||
contains: "TestPrintAboveOrderedNoLoss"
|
||||
- path: "modules/bonfire/live_signal_unix_test.go"
|
||||
provides: "WR-01 pinning test through a re-executed helper process"
|
||||
contains: "TestLiveViewRestoresTerminalOnSignal"
|
||||
- path: "modules/bonfire/errors.go"
|
||||
provides: "bonfire.WithExitCode, bonfire.Sanitize in the error block, masked wrong-directory hint"
|
||||
contains: "func WithExitCode("
|
||||
- path: "cmd/summer/runtime.go"
|
||||
provides: "delegates with pass-through exit codes and separate streams"
|
||||
contains: "WithExitCode"
|
||||
- path: "modules/diary/daily.go"
|
||||
provides: "retry after a failed open; retired writers"
|
||||
contains: "retire"
|
||||
- path: "modules/lagoon/migrations.go"
|
||||
provides: "Migrate over migrationSets"
|
||||
contains: "migrationSets(gdb, plugins)"
|
||||
- path: ".planning/todos/pending/2026-10-22-console-review-info-findings.md"
|
||||
provides: "IN-01..IN-09 captured"
|
||||
contains: "IN-09"
|
||||
key_links:
|
||||
- from: "modules/bonfire/output.go"
|
||||
to: "modules/bonfire/live.go"
|
||||
via: "Println, Printf and emit reroute above a view only when this console owns it"
|
||||
pattern: "owner"
|
||||
- from: "modules/diary/console.go"
|
||||
to: "modules/bonfire/errors.go"
|
||||
via: "diary's console sanitizer delegates to bonfire.Sanitize"
|
||||
pattern: "bonfire.Sanitize"
|
||||
- from: "cmd/summer/runtime.go"
|
||||
to: "modules/bonfire/execute.go"
|
||||
via: "runAppBinary wraps a child exit in Reported and WithExitCode; exitCode honours the carrier"
|
||||
pattern: "WithExitCode"
|
||||
- from: "modules/lagoon/migrations.go"
|
||||
to: "modules/lagoon/commands.go"
|
||||
via: "migrate reports pending-before minus pending-after as its applied lines"
|
||||
pattern: "PendingMigrations"
|
||||
- from: "scripts/check-phase19.8.sh"
|
||||
to: "modules/bonfire/live_test.go"
|
||||
via: "the go stage requires the pinning tests by name"
|
||||
pattern: "TestPrintAboveOrderedNoLoss"
|
||||
prohibitions:
|
||||
- statement: "No exported framework identifier is removed, renamed or changes its signature (core shared plugins and applications build against them); additions only."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
- statement: "WR-01 is fixed inside bonfire's live views: the generated main (internal/build), bonfire.Execute's context and Ctrl+C semantics of commands without a live view do not change."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
- statement: "IN-01..IN-09 are not fixed in this plan; they go to the pending todo."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
- statement: "No new Go module dependency; nothing is pushed or tagged by this plan (plan 06 owns the release)."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
---
|
||||
|
||||
## Phase Goal
|
||||
|
||||
ROADMAP Goal (not in user-story format; plans 01-04 recorded the decision not to invent one): console commands in `summer` and in application binaries reach the Artisan bar; a user sees what a command did, can ask for more or less of it, and learns from `--help` how to use it.
|
||||
|
||||
This plan (gap closure 1 of 2, wave 1) closes the code review of the shipped console (19.8-REVIEW.md, v0.3.1..v0.4.1): the three critical findings CR-01..CR-03 and the eight warnings WR-01..WR-08. The nine info findings IN-01..IN-09 become one pending todo. Plan 06 releases the fixes as v0.4.2 under a go-ahead.
|
||||
|
||||
## Spec-less probe fallback
|
||||
|
||||
Spec-less probe fallback skipped: the phase has no REQUIREMENTS.md IDs (`Requirements: TBD`) and no SPEC.md, as in plans 01-04. `requirements` lists the CONTEXT decisions each finding breaks; `must_haves` derive from the review's findings.
|
||||
|
||||
## Decisions taken in this plan (Claude's discretion inside the review's suggested fixes)
|
||||
|
||||
- **Order of work.** The tracer (Task 1) is CR-01 alone: one finding taken through the whole loop that every later task repeats (pinning test red, fix, green, one commit). The review's "bonfire live view" group (CR-01, WR-01, WR-02) therefore spans Tasks 1 and 2.
|
||||
- **CR-01 mechanism.** Each PrintAbove call becomes one print message placed straight on the program's message queue by the calling goroutine (the bubbletea event loop prints such a message itself, in queue order), instead of a model reply that bubbletea runs as a command on its own goroutine. Unregistering, sending the finish message and waiting for the program happen as one step under the registry lock, so no line reaches a stopped program and no line races the final frame. The review's alternative (a FIFO in the model drained with `tea.Sequence` before quitting) is the fallback if bubbletea v2.1.0's source does not support the first approach.
|
||||
- **WR-01 mechanism.** The review offers a process-wide `signal.NotifyContext` around Execute or a handler inside `runLive`. This plan takes the handler inside `runLive`: a process-wide context would change Ctrl+C for every plugin command that ignores its context (the process would no longer stop) and would change the generated main, forcing regenerate commits in every application.
|
||||
- **CR-02 carrier.** An explicit, additive `bonfire.WithExitCode(err, code)`; bonfire does not honour every error that happens to have an `ExitCode()` method, so plugin commands that return an `*exec.ExitError` keep exiting 1.
|
||||
- **WR-05 home.** `bonfire.Sanitize`, as the review suggests; diary's console sanitizer delegates to it so one implementation exists. IN-08 (raw C1 bytes) stays in the todo and now points at `bonfire.Sanitize`.
|
||||
- **WR-04 retry.** Retry after one minute (writer's clock) or on a new date, whichever comes first; one warning per date. The existing warning message `daily log file disabled` is kept so the pinned tests and any log alerts keep matching.
|
||||
- **WR-06 retired writers.** A retired writer appends a late record and closes the file again (open, write, close), so neither a record nor a descriptor is lost. `diary.Close` keeps its documented behaviour (installed writers reopen on the next record).
|
||||
- **WR-07 applied lines.** migrate computes pending migrations before and after Migrate and prints the difference, also when Migrate fails partway (then it prints what was applied and returns the error).
|
||||
- **Review line numbers.** The review's line numbers for `modules/diary/daily.go` and `modules/bonfire/execute.go` do not match the source (the code is unchanged since v0.4.1); every task names the symbols to locate.
|
||||
|
||||
## Artifacts this plan produces
|
||||
|
||||
- Fix commits (one per finding, each with its pinning test) in bonfire, cmd/summer, internal/dev, diary and lagoon, with README and docs updates in the same commit where documented behaviour or exported API changes.
|
||||
- `scripts/check-phase19.8.sh` requiring the new tests by name.
|
||||
- `19.8-REVIEW-DISPOSITION.md` updated; `.planning/todos/pending/2026-10-22-console-review-info-findings.md`.
|
||||
- `19.8-05-SUMMARY.md` with the RED evidence per finding, the Release impact table and `plan_head_before`/`plan_head_after`.
|
||||
|
||||
<objective>
|
||||
Fix the eleven critical and warning findings of the 19.8 code review, each in its own commit with a test that fails without the fix, without breaking any exported API, and capture the nine info findings as a todo.
|
||||
|
||||
Purpose: the console overhaul shipped in v0.4.0/v0.4.1 loses and reorders output above live views, hides the terminal from summer's delegates, misreports their exit codes, leaves the cursor hidden after Ctrl+C, drops the daily log trail after one failed open and prints untrusted escape sequences; D-03, D-10, D-12, D-18, D-19, D-21, D-25, D-27, D-28 and D-29 promise otherwise.
|
||||
Output: eleven fix commits, one gate commit, one planning-docs commit, the SUMMARY with a Release impact table for plan 06.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@~/.claude/msd-core/workflows/execute-plan.md
|
||||
@~/.claude/msd-core/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-CONTEXT.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-04-SUMMARY.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/deferred-items.md
|
||||
|
||||
Repos and concurrency: summercms.go only. Several sessions work in this repository: before each task run `git status --porcelain` on its paths, stage only those paths by name (never `git add -A`), never touch `.planning/STATE.md` or `.planning/state.json` by hand, no co-author tags, planning docs and code in separate commits. Record `git rev-parse HEAD` as `plan_head_before` in the SUMMARY before the first commit. `go vet ./...` and `go test -short ./...` green at every commit; `go -C examples/hello test ./...` green before the last commit.
|
||||
|
||||
Fix loop for every finding (the tracer proves it once): write the pinning test, run it and keep the failing output (RED) for the SUMMARY, implement the fix, run the test and its package green (with `-race` for bonfire and diary), update README and docs when exported API or documented behaviour changes, commit the test, the fix and the docs together as one `fix(<module>): ...` commit. The RED replay in Task 7 re-proves each RED from the commits.
|
||||
|
||||
Docker: Docker-backed tests need `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1`; if the socket is denied although the user is in the docker group, run the tests through `sudo -u golem`. A Docker test that could not run is NOT RUN in the SUMMARY, never PASS.
|
||||
|
||||
<interfaces>
|
||||
- bonfire console seam: `newConsole(in, out, errW, lookup, tty, interactive *bool)` and `setGlobals(Globals)` (modules/bonfire/output.go); `boolPtr`, `noEnv` (widgets_test.go); a terminal console on a bytes.Buffer runs the live path and the buffer receives the PrintAbove lines (TestPrintAbove).
|
||||
- bonfire live path: `runLive(ctx, out, c, model, work)`, `printAboveLines(s)`, `liveActive()`, `liveMu`, `activeProg`, `liveModel`, `printAboveMsg`, `finishMsg` (live.go); `console.live()`, `emit`, `Println`, `Printf` (output.go); `Spinner`, `Progress`, `liveProgress.Fail`, `Table` (widgets.go); `Process`, `PrintAbove` (helpers.go).
|
||||
- bubbletea v2.1.0 (path: `go list -m -f '{{.Dir}}' charm.land/bubbletea/v2`): tea.go `handleCommands` (each command on its own goroutine), the event loop's `case printLineMessage` (prints in the loop), `Program.Send` (returns once the program has stopped), `Program.Kill` (restores the terminal), `execSequenceMsg`; renderer.go `Println`.
|
||||
- bonfire errors: `RenderError`, `wrongDirectory(name, err, args, dir)`, `shellQuote`, `Reported`, `ErrReported`, `isUsage` (errors.go); `Execute`, `exitCode`, `Fail` (execute.go).
|
||||
- summer delegates: `appRunner`, `delegateCommand`, `runAppBinary`, `newAppCmd`, `runChild`, `appShutdownGrace` (cmd/summer/runtime.go); test helpers `captureAppRunner`, `runSummer`, `TestSignalHelperProcess`, `helperArgv`, `helperEnv`, `syncBuffer`, `waitForOutput` (cmd/summer/runtime_test.go).
|
||||
- summer dev: `Options{Build, Start, Debounce}`, `watch`, `child.stop`, `startBin` (internal/dev/watch.go); serve's drain timeout is the 10-second `context.WithTimeout` in modules/surf/serve.go.
|
||||
- diary: `dailyWriter{mu, disabled, file, date, now, warn}`, `Write`, `open`, `Close`, `newDailyWriter` (daily.go); `install`, `SetVerbosity`, `ApplyGlobals`, `Close`, `closeChannels`, `Reset`, `mu`, `current`, `gens` (diary.go); `channelBuilder.warn` with message `daily log file disabled` (channels.go); `sanitize` (console.go); `Options.Now`.
|
||||
- lagoon: `Migrate`, `migrator`, `migrationSet`, `migrationSets`, `PendingMigrations` (migrations.go); the `migrate` command's Run (commands.go); Docker helpers `pendingPlugins`, `migPlugin`, `runLagoon` (pending_test.go).
|
||||
</interfaces>
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="tracer" tdd="true">
|
||||
<name>Task 1: Tracer: CR-01 lines printed above a live view keep their order and none is lost when the view ends (one fix through the whole loop: red test, fix, green, one commit)</name>
|
||||
<files>modules/bonfire/live.go, modules/bonfire/widgets.go, modules/bonfire/output.go, modules/bonfire/helpers.go, modules/bonfire/live_test.go</files>
|
||||
<behavior>
|
||||
- TestPrintAboveOrderedNoLoss, subtests spinner, progress and process: on a terminal console over a bytes.Buffer (newConsole with tty and interactive true), 20 runs per widget; in each run the work function prints 100 numbered lines (`line-000` .. `line-099`, mixing Output.Println, Output.Printf with a trailing newline, Output.Info and bonfire.PrintAbove), the progress subtest also records two failures through bonfire.Failed between them, and the last line is printed immediately before the work function returns. After the widget returns, the ANSI-stripped buffer holds every numbered line exactly once and in order, and both failure lines.
|
||||
- Subtest concurrent_finish: a goroutine started inside the work function keeps calling bonfire.PrintAbove after work returns; each of its lines appears exactly once in the buffer (above the view or after its final frame), none is lost.
|
||||
- TestPrintAbove, TestWidgetMatrix and TestLiveModels stay green.
|
||||
</behavior>
|
||||
<read_first>modules/bonfire/live.go (whole file), modules/bonfire/widgets.go (Spinner, Progress, liveProgress), modules/bonfire/output.go (emit, Println, Printf, live), modules/bonfire/helpers.go (Process, PrintAbove), modules/bonfire/widgets_test.go (TestPrintAbove and the console helpers), modules/bonfire/live_test.go, the bubbletea v2.1.0 sources named in the interfaces block, 19.8-REVIEW.md (CR-01)</read_first>
|
||||
<action>Per CR-01 (D-03, D-27). Verify the review's claim against bubbletea v2.1.0's source before changing anything: `liveModel.Update` answers each `printAboveMsg` with `tea.Println(...)`, a command that `handleCommands` runs on its own goroutine, so successive lines race back into the event loop and a line still in flight when `finishMsg` quits the program is dropped.
|
||||
|
||||
Write TestPrintAboveOrderedNoLoss in modules/bonfire/live_test.go and run it on the unchanged code; keep the failing output for the SUMMARY (out-of-order or missing lines). If it does not fail in three runs, raise the runs or the line count until it does, and record the numbers.
|
||||
|
||||
Fix in live.go (and where the callers send): every PrintAbove call (printAboveLines, and liveProgress.Fail's failure line) becomes ONE print message for the whole block, placed directly on the program's message queue by the calling goroutine in call order, so the event loop prints it itself without a command goroutine (for example send the message value that `tea.Println(block)` returns, through `Program.Send`, which also returns instead of blocking once the program has stopped; never call `Program.Println`, which blocks forever on a stopped program). Then make the end of runLive one step under `liveMu`: clear the registration, send `finishMsg` and wait for the program to finish while holding the lock, so a printer that arrives meanwhile waits and then writes directly after the final frame. Remove `printAboveMsg` handling from `liveModel` if nothing sends it any more. If the source shows the direct-queue approach does not hold, implement the review's alternative instead (a FIFO of pending lines in the model, drained with `tea.Sequence(<prints>..., tea.Quit)` on `finishMsg`) and record why.
|
||||
|
||||
No exported signature changes (bonfire.PrintAbove keeps its signature and meaning). The README's PrintAbove row already describes the intended behaviour; no docs change unless a sentence there contradicts the fix. Commit test and fix together: `fix(bonfire): keep the order of lines printed above a live view and drain them before it ends`.</action>
|
||||
<verify>
|
||||
<automated>go test -count=1 -race -short -run '^(TestPrintAboveOrderedNoLoss|TestPrintAbove|TestWidgetMatrix|TestLiveModels)$' -v ./modules/bonfire 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 4 && go vet ./... && go test -count=1 -short ./...</automated>
|
||||
<fails_when>CR-01: fewer than 4 top-level "--- PASS: Test" lines (a numbered line missing, duplicated or out of order in any of the 60 widget runs, a failure line missing, a concurrent_finish line lost or printed twice, or a race report), or vet or the short suite fail.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- CR-01: `go test -count=3 -race -short -run '^TestPrintAboveOrderedNoLoss$' ./modules/bonfire` exits 0 (three clean passes in a row).
|
||||
- CR-01: the SUMMARY records the RED output of TestPrintAboveOrderedNoLoss on the code before the fix (the first out-of-order or missing line).
|
||||
- The CR-01 fix commit (found by its subject in `git log --format='%h %s' <plan_head_before>..HEAD`) lists modules/bonfire/live_test.go together with the changed non-test files in `git show --stat`.
|
||||
</acceptance_criteria>
|
||||
<done>Output printed during any live view arrives complete and in order, proven by a deterministic buffer test that fails on the shipped code, in one commit.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 2: WR-02 then WR-01: a live view takes lines only from the console that owns it, registers by check and set and unregisters on panic; Ctrl+C or SIGTERM during a view restores the terminal and keeps the signal's effect</name>
|
||||
<files>modules/bonfire/live.go, modules/bonfire/output.go, modules/bonfire/widgets.go, modules/bonfire/helpers.go, modules/bonfire/live_test.go, modules/bonfire/live_signal_unix_test.go, modules/bonfire/README.md</files>
|
||||
<behavior>
|
||||
- TestPrintlnStaysWithItsConsole: while a Spinner runs on terminal console A (buffer A), console B (a non-terminal console over buffer B) calls Println, Printf and Info; buffer B holds all three and buffer A none of them; console A's own Println still prints above its view.
|
||||
- TestLiveViewSingleOwner: two terminal consoles start a Spinner at the same moment (a barrier, 200 rounds); in every round exactly one runs as a live view and the other prints its plain `[...] <label>` line; after each round bonfire.PrintAbove reports false.
|
||||
- TestLiveViewPanicUnregisters: a Spinner whose work panics; the test recovers the panic; afterwards bonfire.PrintAbove reports false and a new Spinner runs as a live view.
|
||||
- TestLiveViewRestoresTerminalOnSignal (unix build tag, the test binary re-executed as a helper with an environment switch, like cmd/summer's TestSignalHelperProcess): the helper runs a Spinner on a terminal console over its stdout pipe, prints `ready` on stderr and blocks; the test sends SIGINT. Subtest default: the helper dies by SIGINT (default action kept) and its stdout holds the show-cursor sequence after the last hide-cursor sequence. Subtest own_handler: the helper also holds its own signal.NotifyContext, exits 0 by itself after its context ends, and its stdout holds the show-cursor sequence after the last hide-cursor sequence.
|
||||
</behavior>
|
||||
<read_first>modules/bonfire/live.go and output.go after Task 1, modules/bonfire/widgets.go, modules/bonfire/helpers.go (Process passes its ctx), cmd/summer/runtime_test.go (TestSignalHelperProcess, helperArgv, helperEnv, waitForOutput: the re-exec helper pattern), modules/lagoon/pty_linux_test.go (the syscall-only pseudo-terminal helper, only if a pipe does not show the cursor sequences), internal/build/build.go (the generated main: must stay unchanged), 19.8-REVIEW.md (WR-01, WR-02)</read_first>
|
||||
<action>Two findings, two commits, WR-02 first because WR-01 builds on the registration it reworks.
|
||||
|
||||
WR-02 (D-03): verify against the source that `console.Println`, `console.Printf` and `emit` reroute through the single process-wide `activeProg` without checking which console started it, that `console.live()`'s `liveActive()` check and runLive's registration are separate steps, and that runLive unregisters without `defer`. Write the three WR-02 tests and record their RED output (the single-owner test may need more rounds to fail; if it does not fail in three runs, add an unexported test hook called between the check and the registration, nil in production, to force the interleaving, and record that). Fix: keep the owning console with the registered program; Println, Printf and emit reroute only when the calling console is the owner (bonfire.PrintAbove, used by diary, keeps rerouting to whichever view runs, as documented). Replace the separate check with a check-and-set claim under `liveMu` taken before the program starts; a widget that cannot claim prints its plain lines as nested widgets do today. Unregister, finish and wait in a deferred step (keeping Task 1's single locked step), so a panic in work leaves no orphaned registration and still re-panics. Commit: `fix(bonfire): reroute only the owning console's lines to a live view`.
|
||||
|
||||
WR-01 (D-03): verify that the live views run with bubbletea's own signal handling off, that Spinner and Progress pass a background context and that neither summer nor the generated main installs a signal handler, so SIGINT's default action ends the process with the cursor hidden. Write TestLiveViewRestoresTerminalOnSignal and record its RED output (no show-cursor after the last hide-cursor). First confirm on the unchanged code that the helper's stdout pipe carries the hide-cursor sequence; if a pipe never carries the cursor sequences, open a pseudo-terminal for the helper's stdout with a copy of the syscall-only helper from modules/lagoon/pty_linux_test.go (linux build tag, no new dependency). Fix inside runLive only: for the life of the view, relay SIGINT and SIGTERM to a channel with signal.Notify (skip a signal that signal.Ignored reports); on a signal, kill the program (Program.Kill restores the cursor and the terminal), wait for it to stop, clear the registration if it is still this view, stop relaying (signal.Stop), then re-raise the same signal on the own process (os.FindProcess(os.Getpid()) then Signal; if that fails, exit with 128 plus the signal number) so the default action, or the command's own registered handler, still applies. When the view ends normally, stop relaying before runLive returns. Do not change internal/build's generated main, bonfire.Execute's context or the summer main. README: the Spinner, Progress and Process rows or the widgets paragraph say that Ctrl+C during a view restores the terminal and then stops the command as before. Commit: `fix(bonfire): restore the terminal when a live view is interrupted`.</action>
|
||||
<verify>
|
||||
<automated>go test -count=1 -race -short -run '^(TestPrintlnStaysWithItsConsole|TestLiveViewSingleOwner|TestLiveViewPanicUnregisters|TestLiveViewRestoresTerminalOnSignal|TestPrintAboveOrderedNoLoss|TestPrintAbove|TestWidgetMatrix)$' -v ./modules/bonfire 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 7</automated>
|
||||
<fails_when>WR-02: TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner or TestLiveViewPanicUnregisters does not pass (a line lands in another console's view, two views run at once, or a panic leaves a registration behind). WR-01: TestLiveViewRestoresTerminalOnSignal does not pass (the cursor is left hidden, the default signal action is lost, or the command's own handler no longer receives the signal). Any race report, or Task 1's tests regress.</fails_when>
|
||||
<automated>go vet ./... && go test -count=1 -short ./... && F=$(git log --format= --name-only --grep='^fix(bonfire): restore the terminal when a live view is interrupted$' --grep='^fix(bonfire): reroute only the owning console' -- internal/build cmd/summer/main.go) && test -z "$F"</automated>
|
||||
<fails_when>vet or the short suite fail, or either fix commit touches the generated main (internal/build) or summer's main.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- WR-02: `go test -count=3 -race -short -run '^(TestPrintlnStaysWithItsConsole|TestLiveViewSingleOwner|TestLiveViewPanicUnregisters)$' ./modules/bonfire` exits 0, and the SUMMARY records each test's RED output.
|
||||
- WR-01: `go test -count=1 -short -run '^TestLiveViewRestoresTerminalOnSignal$' -v ./modules/bonfire 2>&1 | grep -cE -- '--- PASS: TestLiveViewRestoresTerminalOnSignal/(default|own_handler)'` prints 2, and the SUMMARY records the RED output.
|
||||
- `git log --format=%s <plan_head_before>..HEAD` lists the WR-02 and WR-01 fix subjects as two separate commits (other sessions' commits may sit between them), and `git show --stat` of each lists its test file.
|
||||
</acceptance_criteria>
|
||||
<done>A live view belongs to one console, cannot be registered twice or left registered, and an interrupted view gives the terminal back without changing how the signal ends the command.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 3: summer delegates and dev: CR-02 exit codes pass through without a second error block, CR-03 the binary gets the terminal and a separate stderr, WR-03 dev waits serve's full drain, WR-08 the wrong-directory hint masks secret flag values</name>
|
||||
<files>cmd/summer/runtime.go, cmd/summer/runtime_test.go, modules/bonfire/execute.go, modules/bonfire/errors.go, modules/bonfire/errors_test.go, modules/bonfire/README.md, internal/dev/watch.go, internal/dev/watch_test.go, docs/console/introduction.md, docs/console/writing-commands.md, docs/console/scaffolding.md, docs/setup/upgrading.md</files>
|
||||
<behavior>
|
||||
- TestWithExitCode (bonfire): bonfire.Execute returns 2 for a command returning Reported(WithExitCode(err, 2)) and prints no error block; WithExitCode(err, 3) without Reported prints the block and returns 3; codes below 1 or above 255 return ExitFailure; WithExitCode(nil, 2) is nil; errors.Is and errors.As still see the wrapped error.
|
||||
- TestDelegateExitCodePassthrough (cmd/summer): a summer root runs a delegate whose child (the test binary re-executed as a helper) writes a line on stderr and exits 2; Execute returns 2 and summer's own output and error streams hold no ERROR block and no `summer: migrate` text. A child killed by SIGKILL gives exit 1 and one summer error block naming the signal.
|
||||
- TestAppStreams (cmd/summer): the stream choice for a terminal Output returns os.Stdout and os.Stderr; for a non-terminal Output it returns that Output and os.Stderr.
|
||||
- TestDelegateStreams (cmd/summer): with a non-terminal buffer console, a helper child writing `data-line` to stdout and `log-line` to stderr leaves `data-line` in the buffer and no `log-line` in it.
|
||||
- TestDevStopWaitsForGracefulShutdown (internal/dev): a helper child that traps SIGTERM, sleeps 2.5 seconds and exits 0 is stopped through child.stop and reports exit 0 (not killed); dev's stop grace equals the delegates' grace and exceeds 10 seconds.
|
||||
- TestWrongDirectoryMasksSecrets (bonfire): from a temp tree (application root with summer.yaml, cwd without config/), args `admin:create --email a@b.c --password hunter2`, `--password=hunter2`, `--client-secret s3cr3t`, `--api-token=t0k3n` and `--key k3y` give a Try line without hunter2, s3cr3t, t0k3n or k3y, with `--password ***` or `--password=***` in their place and `--email a@b.c` unchanged; TestWrongDirectoryHint stays green.
|
||||
</behavior>
|
||||
<read_first>cmd/summer/runtime.go (delegateCommand, runAppBinary, newAppCmd, runChild, appShutdownGrace), cmd/summer/runtime_test.go (captureAppRunner, runSummer, the signal helper), cmd/summer/main.go (how the root is built), modules/bonfire/execute.go (Execute, exitCode, Fail), modules/bonfire/errors.go (Reported, RenderError, wrongDirectory, shellQuote), modules/bonfire/errors_test.go and wrongdir_smoke_test.go (TestWrongDirectoryHint), internal/dev/watch.go (watch's stream choice, child.stop, startBin), internal/dev/watch_test.go (TestStartBinServe, TestDevGracefulNil), modules/surf/serve.go (the 10-second drain), docs/console/introduction.md ("Exit codes", "Commands that summer delegates"), docs/console/scaffolding.md (summer dev), docs/console/writing-commands.md (wrong-directory example), docs/setup/upgrading.md ("Upgrading to 0.4"), modules/bonfire/README.md (error block paragraph, API table), 19.8-REVIEW.md (CR-02, CR-03, WR-03, WR-08)</read_first>
|
||||
<action>Four findings, four commits, in this order. Verify each claim against the source first and record the pinning test's RED output.
|
||||
|
||||
CR-02 (D-18): runAppBinary returns `summer: <argv>: exit status N` wrapping the child's *exec.ExitError, so Execute renders a second error block under the one the binary printed and maps it to exit 1. Add to bonfire an exported carrier `WithExitCode(err error, code int) error` (unexported type with Error, Unwrap; nil stays nil) and make `exitCode` honour it before the usage check (a code outside 1..255 maps to ExitFailure). In runAppBinary, when the child exited with a code (`ExitError.Exited()`), return `bonfire.Reported(bonfire.WithExitCode(<the wrapped error>, code))`; a child ended by a signal keeps today's error block and exit 1. Factor the part of runAppBinary after locating the binary into a function the tests can call with the test binary as the child. README: an API row for bonfire.WithExitCode and the Reported row amended ("exit code 1, or the code of bonfire.WithExitCode"); docs/console/introduction.md: the delegates section says summer exits with the binary's exit code; docs/setup/upgrading.md: one bullet under "Upgrading to 0.4" behaviour changes. Commit: `fix(summer): pass the app binary's exit code through and print its error once`.
|
||||
|
||||
CR-03 (D-27, D-28, D-04): newAppCmd sets the child's stdout and stderr to summer's Output, so the child never sees a terminal (no production question, no styled summary, no widgets) and its stderr lands in summer's stdout. Add a stream choice used by the delegates: on a terminal Output (bonfire.IsTerminal) the child gets os.Stdout and os.Stderr; otherwise stdout goes to the Output (keeps tests and Call roots capturing) and stderr to os.Stderr. newAppCmd takes the two writers (unexported; adjust its existing tests). The child keeps os.Stdin. Docs: the delegates section of docs/console/introduction.md says the binary writes to the terminal directly and its stderr stays separate, so `summer route:list --json > routes.json` captures only the JSON; extend the upgrading bullet. Commit: `fix(summer): give delegated commands the terminal and keep their stderr apart`.
|
||||
|
||||
WR-03 (D-25, D-29): child.stop in internal/dev/watch.go kills serve two seconds after SIGTERM while serve drains for up to ten. Move the grace into one constant shared by the delegates and dev: `ShutdownGrace` in internal/dev (15 seconds, with a comment naming serve's 10-second drain; internal, so not public API), used by child.stop, with cmd/summer's appShutdownGrace defined as `dev.ShutdownGrace`. The helper child for the test may be the test binary re-executed with an environment switch. docs/console/scaffolding.md: the summer dev paragraph says Ctrl+C waits for serve's shutdown report (up to its drain time) before stopping it. Commit: `fix(dev): give serve the full shutdown grace before killing it`.
|
||||
|
||||
WR-08 (D-19): wrongDirectory builds the Try line from every argument, so a `--password` value reaches stderr and CI logs. When building the Try line, replace the value of every long flag whose name (case-insensitive) contains password, passwd, secret, token or key with `***`, in the `--flag value` form (the next argument when it does not start with `-`) and the `--flag=value` form; write the mask without shell quoting; leave every other argument as today. README (error block paragraph) and docs/console/writing-commands.md (the wrong-directory paragraph) say secret-looking flag values are masked. Commit: `fix(bonfire): mask secret flag values in the wrong-directory hint`.
|
||||
|
||||
No other exported signature changes; bonfire.Reported, bonfire.Fail and bonfire.Execute keep their signatures.</action>
|
||||
<verify>
|
||||
<automated>go test -count=1 -race -short -run '^(TestWithExitCode|TestWrongDirectoryMasksSecrets|TestWrongDirectoryHint|TestExitCodes|TestReportedSilent|TestErrorGolden)$' -v ./modules/bonfire 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 6</automated>
|
||||
<fails_when>CR-02 (bonfire side): TestWithExitCode fails (the carrier code is ignored, a block is printed for a Reported carrier, or an out-of-range code escapes). WR-08: TestWrongDirectoryMasksSecrets fails (a secret value appears in the Try line or another argument changed). Or TestWrongDirectoryHint, TestExitCodes, TestReportedSilent or the error goldens regress.</fails_when>
|
||||
<automated>go test -count=1 -short -run '^(TestDelegateExitCodePassthrough|TestAppStreams|TestDelegateStreams|TestRunChildForwardsSignals|TestNewAppCmdCancelSendsSIGTERM|TestDelegateExamples)$' -v ./cmd/summer 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 6</automated>
|
||||
<fails_when>CR-02: TestDelegateExitCodePassthrough fails (summer exits 1 for a child's 2, prints a second error block, or a signalled child is silent). CR-03: TestAppStreams or TestDelegateStreams fails (the child's stderr reaches summer's stdout, or a terminal Output does not hand the child the process's own streams). Or the existing delegate signal tests regress.</fails_when>
|
||||
<automated>go test -count=1 -short -run '^(TestDevStopWaitsForGracefulShutdown|TestStartBinServe|TestDevReport|TestDevGracefulNil)$' -v ./internal/dev 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 4 && go vet ./... && go test -count=1 -short ./... && go run ./cmd/summer docs:build --check</automated>
|
||||
<fails_when>WR-03: TestDevStopWaitsForGracefulShutdown fails (serve is killed before its drain ends, or dev's grace differs from the delegates'). Or the dev tests, vet, the short suite or the docs checker (a README or docs identifier that does not exist) fail.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- CR-02: `grep -c '^func WithExitCode(' modules/bonfire/errors.go modules/bonfire/execute.go | awk -F: '{s+=$2} END {print s}'` prints 1 and `grep -c 'bonfire.WithExitCode' modules/bonfire/README.md` prints at least 1.
|
||||
- CR-03: `grep -c 'os.Stderr' cmd/summer/runtime.go` prints at least 1 and TestDelegateStreams passes.
|
||||
- WR-03: `grep -c 'dev.ShutdownGrace' cmd/summer/runtime.go` prints at least 1 and `grep -c 'ShutdownGrace' internal/dev/watch.go` prints at least 2 (declaration and use in child.stop); TestDevStopWaitsForGracefulShutdown passes.
|
||||
- WR-08: TestWrongDirectoryMasksSecrets passes and `grep -ci 'masked\|\*\*\*' docs/console/writing-commands.md` prints at least 1.
|
||||
- `git log --format=%s <plan_head_before>..HEAD` lists the four fix subjects as four separate commits; the SUMMARY records each pinning test's RED output.
|
||||
</acceptance_criteria>
|
||||
<done>summer's delegates behave like running the binary directly (terminal, separate streams, same exit code, one error block), summer dev lets serve finish its shutdown, and the wrong-directory hint never repeats a secret.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 4: diary: WR-04 a failed daily open is retried after a minute or on the next date, WR-06 replaced daily writers keep no descriptor and ApplyGlobals rebuilds once</name>
|
||||
<files>modules/diary/daily.go, modules/diary/diary.go, modules/diary/daily_test.go, modules/diary/diary_test.go, modules/diary/README.md, docs/services/logging.md</files>
|
||||
<behavior>
|
||||
- TestDailyRetriesAfterFailure: with an injected clock at 2026-10-21 10:00:00 and a regular file where the log directory should be, the first record warns once (`daily log file disabled`); the test removes the blocking file; a record at 10:00:30 writes nothing and does not warn again; a record at 10:01:01 creates summer-2026-10-21.log holding it. A second scenario: the failure on 2026-10-21 23:59:59, blocker removed, a record at 2026-10-22 00:00:01 creates summer-2026-10-22.log; if the new date fails as well, exactly one more warning appears for it.
|
||||
- TestRetiredDailyWriterKeepsNoFile: a daily writer that has written once is retired (as install and Reset do with the previous channels); a later record still lands in the day's file and afterwards the writer holds no open file; diary.Close on the installed writers still lets the next record reopen the file.
|
||||
- TestApplyGlobalsRebuildsOnce: after Setup with color auto, 50 goroutines released by a barrier call ApplyGlobals with color never; the channels are rebuilt exactly once (the generation counter advances by one) and every call's verbosity is applied; run under -race.
|
||||
- TestDailyUnwritable, TestDailyDegradesSmoke, TestDailyRotation, TestConcurrentWrites and TestResetRestoresDefault stay green.
|
||||
</behavior>
|
||||
<read_first>modules/diary/daily.go (dailyWriter, Write, open, Close), modules/diary/diary.go (install, SetVerbosity, ApplyGlobals, Close, closeChannels, Reset), modules/diary/channels.go (buildChannels, channelBuilder.warn), modules/diary/daily_test.go (TestDailyUnwritable, the setup helper, lockedBuffer, dailyOnly), modules/diary/diary_test.go, modules/diary/README.md (Daily files), docs/services/logging.md (the paragraph about a file that cannot be opened), 19.8-REVIEW.md (WR-04, WR-06), 19.8-01-PLAN.md threat T-19.8-06</read_first>
|
||||
<action>Two findings, two commits. Locate the code by symbol (the review's daily.go line numbers do not match the file). Verify each claim and record the pinning tests' RED output.
|
||||
|
||||
WR-04 (D-10): dailyWriter.Write sets `disabled` on the first failed open and never clears it, so one transient failure (ENOSPC, EMFILE, a directory briefly removed at rotation) stops file logging until restart. Replace the permanent flag with a retry point: after a failed open, skip writes (still returning len(p), nil) until one minute has passed by the writer's clock or the local date changes, whichever comes first, then try to open again; warn through the existing warn function once per failed date (keep the message `daily log file disabled` so the pinned tests and existing alerts match). T-19.8-06's rule stands: boot never fails on a log file. Update the dailyWriter doc comment, modules/diary/README.md (Daily files bullet) and docs/services/logging.md (the cannot-be-opened paragraph: the channel skips records and tries again a minute later or on the next day, warning once per day). Commit: `fix(diary): retry a daily file that failed to open`.
|
||||
|
||||
WR-06 (D-10): install and Reset swap the state and then close the old channels' daily writers, but a goroutine holding the old state reopens the file through the closed writer and nothing closes it again; ApplyGlobals reads the state outside `mu`, so two concurrent calls can both rebuild. Add a retire step for replaced channels, separate from Close: a retired daily writer closes its file, and every later write opens the day's file, appends the record and closes it again (no record lost, no descriptor kept). install and Reset retire the old channels; the public diary.Close keeps closing the installed writers with reopen on the next record, as documented. Make ApplyGlobals hold `mu` for its whole run, using lock-free internal variants of install and SetVerbosity so nothing locks twice. Commit: `fix(diary): retire replaced daily writers and serialise ApplyGlobals`.
|
||||
|
||||
No exported signature changes in diary.</action>
|
||||
<verify>
|
||||
<automated>go test -count=1 -race -short -run '^(TestDailyRetriesAfterFailure|TestRetiredDailyWriterKeepsNoFile|TestApplyGlobalsRebuildsOnce|TestDailyUnwritable|TestDailyDegradesSmoke|TestDailyRotation|TestConcurrentWrites|TestResetRestoresDefault)$' -v ./modules/diary 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 8</automated>
|
||||
<fails_when>WR-04: TestDailyRetriesAfterFailure fails (no retry after a minute or on a new date, a retry before the minute, or more than one warning per date). WR-06: TestRetiredDailyWriterKeepsNoFile or TestApplyGlobalsRebuildsOnce fails (a retired writer keeps a file open or loses the record, diary.Close no longer reopens, or concurrent ApplyGlobals calls rebuild more than once). Or a race report, or the existing daily, rotation, concurrency or reset tests regress.</fails_when>
|
||||
<automated>go vet ./... && go test -count=1 -short ./... && go run ./cmd/summer docs:build --check</automated>
|
||||
<fails_when>vet, the short suite or the docs checker fail.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- WR-04: `grep -ci 'next day\|a minute\|one minute' docs/services/logging.md` prints at least 1 and TestDailyRetriesAfterFailure passes.
|
||||
- WR-06: `go test -count=3 -race -short -run '^(TestRetiredDailyWriterKeepsNoFile|TestApplyGlobalsRebuildsOnce)$' ./modules/diary` exits 0.
|
||||
- `git log --format=%s <plan_head_before>..HEAD` lists the WR-04 and WR-06 fix subjects as two separate commits; the SUMMARY records each pinning test's RED output.
|
||||
</acceptance_criteria>
|
||||
<done>A long-running server keeps its file log trail through transient failures and setup changes, without leaking descriptors.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 5: WR-05 error blocks, process views and tables strip control characters through the additive bonfire.Sanitize, which diary's console handler also uses</name>
|
||||
<files>modules/bonfire/errors.go, modules/bonfire/live.go, modules/bonfire/widgets.go, modules/bonfire/helpers.go, modules/bonfire/errors_test.go, modules/bonfire/live_test.go, modules/bonfire/widgets_test.go, modules/bonfire/README.md, modules/diary/console.go, docs/console/writing-commands.md</files>
|
||||
<behavior>
|
||||
- TestSanitize: tab kept, CR and LF become spaces, ESC, BEL, other C0 controls and DEL removed, clean strings returned unchanged; the same table as diary's TestConsoleStripsControlChars.
|
||||
- TestRenderErrorStripsControl: with color forced on (Globals Color always), an error whose message, HintError explanation and Try text and wrapped cause (at -v) carry `\x1b]0;pwned\x07` and `\x1b[2J` renders a block that contains neither injected sequence (the palette's own styling sequences may remain) and still shows the visible text.
|
||||
- TestProcessViewStripsControl: the process model's View, with an item name and an item error carrying the same sequences, contains neither.
|
||||
- TestTableStripsControl: Table on a terminal console with color forced, and on a non-terminal console, with a cell and a header carrying the sequences, prints neither.
|
||||
- TestConsoleStripsControlChars (diary) and the bonfire goldens stay green.
|
||||
</behavior>
|
||||
<read_first>modules/diary/console.go (sanitize and its callers), modules/bonfire/errors.go (RenderError, writeStyled, causes), modules/bonfire/live.go (processModel.View), modules/bonfire/widgets.go (Table, failureLine), modules/bonfire/helpers.go (Process, failure), modules/bonfire/errors_test.go, modules/bonfire/live_test.go, modules/bonfire/widgets_test.go, modules/diary/console_test.go (TestConsoleStripsControlChars), modules/bonfire/README.md, docs/console/writing-commands.md, 19.8-REVIEW.md (WR-05, IN-08), 19.8-01-PLAN.md threat T-19.8-03</read_first>
|
||||
<action>Per WR-05 (D-12, D-18; extends threat T-19.8-03 from diary's console to bonfire's output). Verify against the source that RenderError passes text through sunscreen.Scrub only (which keeps ESC and other control characters), that processModel.View prints item names and `it.err.Error()` unchanged and that Table prints cells unchanged, and that the colorprofile writer passes such sequences through when color is on. Write the four tests with color forced on (the NoTTY profile strips all escape sequences and would hide the defect) and record their RED output.
|
||||
|
||||
Move diary's sanitizer into bonfire as the exported `Sanitize(s string) string` with the same rules (tab kept, CR and LF to a space, every other control character dropped) and make diary's console handler call it, deleting diary's copy. Apply it before styling to: every error-block line (message, explanation, Try lines and the -v chain, after splitting into lines and alongside sunscreen.Scrub), process-view item names and item errors, the failure lines of Progress and Process, and table headers and cells in both the terminal and the plain layout. Do not add the IN-08 change (invalid UTF-8 and raw C1 bytes); the todo records it against bonfire.Sanitize.
|
||||
|
||||
README: an API row for bonfire.Sanitize and one sentence in the error block and widgets text that printed text is stripped of control characters; docs/console/writing-commands.md: one sentence where error output and tables are described. Commit: `fix(bonfire): strip control characters from error blocks, process views and tables`.</action>
|
||||
<verify>
|
||||
<automated>go test -count=1 -race -short -run '^(TestSanitize|TestRenderErrorStripsControl|TestProcessViewStripsControl|TestTableStripsControl|TestErrorGolden|TestListGolden|TestWidgetMatrix|TestLiveModels)$' -v ./modules/bonfire 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 8 && go test -count=1 -race -short -run '^(TestConsoleStripsControlChars|TestSlogtestConsoleHandler)$' ./modules/diary && go vet ./... && go test -count=1 -short ./... && go run ./cmd/summer docs:build --check</automated>
|
||||
<fails_when>WR-05: an injected OSC or CSI sequence survives in an error block, process view or table (TestRenderErrorStripsControl, TestProcessViewStripsControl, TestTableStripsControl), TestSanitize disagrees with diary's rules, diary's console tests regress, a golden changes, or vet, the short suite or the docs checker fail.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- WR-05: `grep -c '^func Sanitize(' modules/bonfire/*.go | awk -F: '{s+=$2} END {print s}'` prints 1, `grep -c 'bonfire.Sanitize' modules/diary/console.go` prints at least 1 and `grep -c 'bonfire.Sanitize' modules/bonfire/README.md` prints at least 1.
|
||||
- `git log --format=%h <plan_head_before>..HEAD -- modules/bonfire/testdata` prints nothing (no golden changed by this plan).
|
||||
- The SUMMARY records the RED output of the three WR-05 output tests.
|
||||
</acceptance_criteria>
|
||||
<done>Text from databases, remote services or admins can no longer move the cursor or retitle the operator's terminal through error blocks, process views or tables.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 6: WR-07 Migrate and PendingMigrations read one migration set list, and migrate's applied lines count what Migrate actually applied</name>
|
||||
<files>modules/lagoon/migrations.go, modules/lagoon/commands.go, modules/lagoon/pending_test.go, modules/lagoon/README.md</files>
|
||||
<precondition>A Docker daemon answers (`docker info`, directly or through `sudo -u golem`); TestMigrateAppliedLines is a testcontainers test. Without one, stop the task and report it.</precondition>
|
||||
<behavior>
|
||||
- TestMigrateAppliedLines (Docker, skipped under -short through the package's TestMain): two fixture plugins, alpha with one migration that succeeds and beta with one whose SQL fails; `migrate` on a fresh database prints `alpha: 1 applied` and the framework sets' applied lines, returns beta's error and prints no success line; after replacing beta's migration with a valid one, a second `migrate` prints `beta: 1 applied` and `Applied 1 migrations`, and a third prints `Nothing to migrate`.
|
||||
- TestPendingMigrations, TestMigratePretend, TestMigrateStatusJSON, TestMigrateRunsSystemFilesFirst and TestTwoPluginMigrationSetsIsolated stay green.
|
||||
</behavior>
|
||||
<read_first>modules/lagoon/migrations.go (Migrate, migrator, migrationSet, migrationSets, PendingMigrations), modules/lagoon/commands.go (the migrate command's Run), modules/lagoon/pending_test.go (pendingPlugins, migPlugin, runLagoon, TestMain), modules/lagoon/migrations_test.go (TestMigrateRunsSystemFilesFirst), modules/lagoon/README.md, 19.8-REVIEW.md (WR-07)</read_first>
|
||||
<action>Per WR-07 (D-21, D-04). Verify that Migrate lists the framework sets and the plugin loop by hand while PendingMigrations reads migrationSets, and that migrate prints its applied lines from the list computed before Migrate ran (so a failure partway prints none). Write TestMigrateAppliedLines and record its RED output (no applied lines when beta fails).
|
||||
|
||||
Make Migrate iterate `migrationSets(gdb, plugins)`, so the startup summary, --pretend, the production question's count, the applied lines and migrate:status --json read the same list. Give each set a label so Migrate's error wording stays as today (`lagoon: migrate system_files: ...`, `backend admin`, `deferred bindings`, `queue`, `<plugin id>`). In the migrate command, compute PendingMigrations before and after Migrate and print the per-set applied lines and the result line from the difference; when Migrate fails, print the lines for what was applied and then return its error. Migrate's signature and its order of sets do not change. README: the migrate description says the applied lines count what was applied, also when a migration fails. Commit: `fix(lagoon): one migration set list for Migrate and PendingMigrations`.</action>
|
||||
<verify>
|
||||
<automated>TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 -timeout 10m -run '^(TestMigrateAppliedLines|TestPendingMigrations|TestMigratePretend|TestMigrateStatusJSON|TestMigrateRunsSystemFilesFirst|TestTwoPluginMigrationSetsIsolated)$' -v ./modules/lagoon 2>&1 | grep -c -- '^--- PASS: Test' | grep -qx 6 && go vet ./... && go test -count=1 -short ./...</automated>
|
||||
<fails_when>WR-07: TestMigrateAppliedLines fails (no applied lines on a partial failure, wrong counts, or a success line after a failure), any of the five existing migration tests regress, a test was skipped (fewer than 6 passes means Docker did not run: report NOT RUN, do not count it as passed), or vet or the short suite fail.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- WR-07: `grep -v '^\s*//' modules/lagoon/migrations.go | grep -c 'attach.Migrations'` prints 1 and `grep -v '^\s*//' modules/lagoon/migrations.go | grep -c 'QueueMigrations(sqlDB)'` prints 1 (the framework sets are listed once, in migrationSets).
|
||||
- WR-07: `grep -c 'migrationSets(gdb, plugins)' modules/lagoon/migrations.go` prints at least 2 (Migrate and PendingMigrations).
|
||||
- `git log --format=%s <plan_head_before>..HEAD` lists the WR-07 fix subject; the SUMMARY records the RED output.
|
||||
</acceptance_criteria>
|
||||
<done>A migration set added once is applied, counted, pretended and reported everywhere, and migrate reports what it really applied.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 7: The gate requires every pinning test, each fix is replayed red without its code, the full suites are green with Docker, the disposition record is updated and IN-01..IN-09 become one pending todo</name>
|
||||
<files>scripts/check-phase19.8.sh, .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md, .planning/todos/pending/2026-10-22-console-review-info-findings.md, .planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md</files>
|
||||
<read_first>scripts/check-phase19.8.sh (the required-name arrays and run_go, run_docker), 19.8-REVIEW-DISPOSITION.md (frontmatter and table format, the note on setting dispositions by hand), 19.8-REVIEW.md (IN-01..IN-09), .planning/todos/pending/2026-10-21-quiet-plugin-boot-logs-on-console-commands.md (todo format), this plan's commits (git log plan_head_before..HEAD)</read_first>
|
||||
<action>(1) Gate. Add every new test name to the required arrays in scripts/check-phase19.8.sh with a `# Plan 05 (review fixes)` comment: BONFIRE_TESTS gets TestPrintAboveOrderedNoLoss, TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters, TestLiveViewRestoresTerminalOnSignal, TestWithExitCode, TestWrongDirectoryMasksSecrets, TestSanitize, TestRenderErrorStripsControl, TestProcessViewStripsControl, TestTableStripsControl; DIARY_TESTS gets TestDailyRetriesAfterFailure, TestRetiredDailyWriterKeepsNoFile, TestApplyGlobalsRebuildsOnce; SUMMER_TESTS gets TestDelegateExitCodePassthrough, TestAppStreams, TestDelegateStreams; DEV_TESTS gets TestDevStopWaitsForGracefulShutdown; DOCKER_LAGOON_TESTS gets TestMigrateAppliedLines. Use the names the tasks actually committed (record any rename). Leave IN-09's naming of consuming applications alone (it is in the todo). Commit: `test(19.8): gate requires the review fixes' tests`.
|
||||
|
||||
(2) RED replay. For each of the eleven fix commits C, in a temporary detached worktree under the session scratchpad (`git worktree add --detach <dir> C`), reverse-apply C's changes outside test files and Markdown (`git -C <dir> show C -- . ':!*_test.go' ':!*.md' > <dir>.patch && git -C <dir> apply -R <dir>.patch`), run C's pinning test by name and expect it to FAIL (a compile failure counts for the tests of the two new exported identifiers; say so in the table), then `git worktree remove --force <dir>`. The lagoon replay needs Docker. Record per finding: commit, pinning test, RED kind (assertion or compile), result. A pinning test that passes on the reverted code means the test does not pin the fix: stop and fix the test in its own `test(...)` commit before continuing.
|
||||
|
||||
(3) Full verification: `go vet ./...`; `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...` (through `sudo -u golem` if the socket is denied; the known wristband flake TestOIDCSubjectIsRandomSmoke from deferred-items.md may be rerun once and recorded); `go -C examples/hello test -count=1 ./...`; `bash scripts/check-phase19.8.sh go`; `bash scripts/check-phase19.8.sh docs`; `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh docker`. API check: no exported func, method or type declaration line removed between plan_head_before and HEAD in non-test Go files under modules/ unless the identical line is re-added (`comm -23` of the removed and added `func`/`type` lines, filtered to exported names, prints nothing).
|
||||
|
||||
(4) Records. Edit 19.8-REVIEW-DISPOSITION.md by hand: CR-01..CR-03 and WR-01..WR-08 `disposition: fixed` in the frontmatter and `fixed` in the table, with Source `fix <short hash> (19.8-05)`; IN-01..IN-09 `deferred` with Source `todo .planning/todos/pending/2026-10-22-console-review-info-findings.md`; `open:` set to the number of findings still open (0). Create the todo in the existing format: frontmatter `created` (ISO time), `title: Console review info findings (IN-01..IN-09)`, `area: framework`, `severity: minor`, `g15office: pending` (the orchestrator mirrors it; no g15office tool here), `files:` the files the nine findings name; body: one section per finding with its file, the problem and the review's suggested fix, noting that IN-08's fix now belongs in bonfire.Sanitize (Task 5) and that IN-07 should keep diary's new use of bonfire.Sanitize in mind. Commit both planning files together: `docs(19.8): review fixes recorded, info findings captured as a todo`.
|
||||
|
||||
(5) SUMMARY: plan_head_before and plan_head_after, the commits per finding, the RED replay table, the verification results (Docker run or NOT RUN), the API check result, and a Release impact table (finding, commit, module, exported API added, needs require in plugins yes/no, regenerate in apps yes/no). Expected: API added bonfire.WithExitCode and bonfire.Sanitize, no plugin uses them, the generated main is unchanged, so no require bump and no regenerate; any row that differs says why. Add a 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").</action>
|
||||
<verify>
|
||||
<automated>go vet ./... && go -C examples/hello test -count=1 ./... && bash scripts/check-phase19.8.sh go && bash scripts/check-phase19.8.sh docs && TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh docker</automated>
|
||||
<fails_when>vet, the example application's tests, or the gate's go, docs or docker stage fail (a pinning test missing, skipped or red by name, a stale docs identifier, or a Docker test red; with no Docker daemon the docker stage refuses, which is a failure here).</fails_when>
|
||||
<automated>TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./... && grep -c 'disposition: fixed' .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md | grep -qx 11 && grep -c 'disposition: deferred' .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md | grep -qx 9 && test -f .planning/todos/pending/2026-10-22-console-review-info-findings.md</automated>
|
||||
<fails_when>The full suite with Docker fails, the disposition record does not show 11 fixed and 9 deferred findings, or the todo file is missing.</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- `grep -cE 'TestPrintAboveOrderedNoLoss|TestDelegateExitCodePassthrough|TestDailyRetriesAfterFailure|TestDevStopWaitsForGracefulShutdown|TestMigrateAppliedLines|TestRenderErrorStripsControl|TestWrongDirectoryMasksSecrets' scripts/check-phase19.8.sh` prints at least 7.
|
||||
- The SUMMARY's RED replay table has eleven rows, each with result "failed on reverted code".
|
||||
- `grep -cE '^(severity: minor|area: framework|g15office: pending)$' .planning/todos/pending/2026-10-22-console-review-info-findings.md` prints 3 and `grep -cE '^## IN-0[1-9]' .planning/todos/pending/2026-10-22-console-review-info-findings.md` prints 9.
|
||||
- The SUMMARY holds the Release impact table and the API check result (no exported declaration removed).
|
||||
- None of this plan's commits (the hashes the SUMMARY lists) includes `.planning/STATE.md` or `.planning/state.json`: for each listed hash, `git show --name-only --format= <hash>` names neither file. Other sessions' edits and commits to them are left alone.
|
||||
</acceptance_criteria>
|
||||
<done>Every review fix is pinned by a gated test that is proven to fail without it, the whole framework suite is green with Docker, the review record says what happened to each finding, and plan 06 has the table it needs to release.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| database, remote services and admin input to the operator's terminal | error text, item names and table cells printed by commands |
|
||||
| command arguments to stderr and CI logs | the wrong-directory hint repeats arguments |
|
||||
| summer to the delegated app binary | streams, terminal, exit code and signals |
|
||||
| long-running processes to the filesystem | daily log files over days of uptime |
|
||||
| framework to shared core plugins and applications | exported APIs and the generated main |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|----------|-------------|-----------------|
|
||||
| T-19.8-26 | Tampering | bonfire error block, process view, table (WR-05) | medium | mitigate | bonfire.Sanitize applied to every printed line, item, error and cell before styling; tests with color forced on prove OSC and CSI sequences are gone |
|
||||
| T-19.8-27 | Information disclosure | wrong-directory hint (WR-08) | medium | mitigate | values of flags named like password, passwd, secret, token or key replaced by *** in the Try line; TestWrongDirectoryMasksSecrets |
|
||||
| T-19.8-28 | Elevation of privilege | summer delegates skip the production question (CR-03) | high | mitigate | the child gets summer's own streams on a terminal, so bonfire.Interactive and ConfirmProceed see a terminal; TestAppStreams pins the choice |
|
||||
| T-19.8-29 | Tampering | --json and -q data captured through summer (CR-03) | medium | mitigate | the child's stderr goes to stderr, never into stdout; TestDelegateStreams |
|
||||
| T-19.8-30 | Repudiation | daily log trail lost after one failed open (WR-04) | medium | mitigate | retry after a minute or on the next date, one warning per date; TestDailyRetriesAfterFailure |
|
||||
| T-19.8-31 | Denial of service | descriptors leaked by replaced daily writers (WR-06) | low | mitigate | retired writers open, append and close per late record; TestRetiredDailyWriterKeepsNoFile |
|
||||
| T-19.8-32 | Denial of service | signal handling in live views (WR-01) | medium | mitigate | the view relays SIGINT and SIGTERM only while it runs, restores the terminal, then re-raises so the default action or the command's own handler applies; both paths tested in a helper process |
|
||||
| T-19.8-33 | Tampering | shared plugin and application contracts | medium | mitigate | additions only (WithExitCode, Sanitize); Task 7's API check finds no removed exported declaration; the generated main is untouched (Task 2 verify) |
|
||||
| T-19.8-SC | Tampering | dependencies | low | accept | this plan adds no module; tests use the standard library, the existing charm modules and testcontainers |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- Task 1 (tracer): CR-01 red then green, -race, three clean runs.
|
||||
- Task 2: WR-02 and WR-01 tests, generated main untouched.
|
||||
- Task 3: bonfire, cmd/summer and internal/dev tests for CR-02, CR-03, WR-03, WR-08; docs checker.
|
||||
- Task 4: diary tests for WR-04 and WR-06 with -race; docs checker.
|
||||
- Task 5: WR-05 tests with color forced on; goldens unchanged; docs checker.
|
||||
- Task 6: WR-07 Docker test plus the existing migration tests.
|
||||
- Task 7: gate names, RED replay of all eleven commits, `go vet ./...`, `go test ./...` with Docker, examples/hello, gate stages go, docs and docker, API check, disposition and todo.
|
||||
- Feedback sampling: after every commit `go vet ./...` and `go test -short ./...`.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- CR-01..CR-03 and WR-01..WR-08 fixed, one commit each, each with a gated pinning test proven red without the fix.
|
||||
- No exported API removed or changed; two additive identifiers documented in the bonfire README and docs.
|
||||
- `go vet ./...`, `go test ./...` with Docker, examples/hello and the gate's go, docs and docker stages green.
|
||||
- 19.8-REVIEW-DISPOSITION.md: 11 fixed, 9 deferred; the IN todo exists with `g15office: pending` for the orchestrator.
|
||||
- The SUMMARY's Release impact table tells plan 06 whether any plugin or application needs a commit.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md` when done, with plan_head_before and plan_head_after, the commit per finding with its pinning test and RED output, the RED replay table, the verification results (Docker run or NOT RUN), the API check, the Release impact table, the todo path, and the suggested g15office comment for the orchestrator.
|
||||
</output>
|
||||
@@ -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).
|
||||
211
.planning/phases/19.8-console-commands-overhaul/19.8-06-PLAN.md
Normal file
211
.planning/phases/19.8-console-commands-overhaul/19.8-06-PLAN.md
Normal file
@@ -0,0 +1,211 @@
|
||||
---
|
||||
phase: 19.8-console-commands-overhaul
|
||||
plan: 06
|
||||
type: execute
|
||||
wave: 2
|
||||
depends_on: ["19.8-05"]
|
||||
gap_closure: true
|
||||
files_modified:
|
||||
- .planning/phases/19.8-console-commands-overhaul/19.8-06-SUMMARY.md
|
||||
- ../sm-demo-app/plugins/demo/content/ROADMAP.md
|
||||
- ../sm-demo-app/plugins/demo/roadmap/names.go
|
||||
autonomous: false
|
||||
requirements: [D-26]
|
||||
user_setup: []
|
||||
|
||||
estimate:
|
||||
tokens: 60000
|
||||
raw_tokens: 60000
|
||||
tasks: 3
|
||||
confidence: low
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "D-26: nothing is tagged or pushed before the Task 1 reply, which the SUMMARY records verbatim; before the reply no tag points at HEAD."
|
||||
- "On 'release vX.Y.Z' (proposed v0.4.2): after git fetch, origin/master..master holds only plan 05's code commits and planning documents (this phase's and other sessions'); vet, the short suite and the docs checker are green at the commit that gets tagged; an annotated vX.Y.Z tag listing the fixes in user words points at that commit; master is pushed as exactly that commit, then the tag; nothing is force-pushed; the same-day changelog post with user-facing notes under ## Fixed exists and its URL is in the SUMMARY; the token is never printed."
|
||||
- "On 'push-only': the same range check and checks, then master pushed as the tested commit; no tag, no changelog post. On 'hold': nothing leaves this machine."
|
||||
- "Shared plugins get a require bump and applications a regenerate commit only for rows of plan 05's Release impact table that say so (expected: none, because the two API additions are used by no plugin and the generated main did not change)."
|
||||
- "sm-demo-app's roadmap snapshot equals origin/master's .planning/ROADMAP.md, every phase in it has a Polish title and every open phase a Polish summary, in separate sm-demo-app commits (Polish names first, then the snapshot) that are pushed only under 'release' or 'push-only'; when the snapshot already matches, sm-demo-app is not touched."
|
||||
- "`PHASE198_REQUIRE_DOCKER=1 TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh --all` ends with 'Phase 19.8 gate passed' (TestTerminalCommands may print NOT RUN)."
|
||||
- "Nothing is deployed in any option."
|
||||
artifacts:
|
||||
- path: ".planning/phases/19.8-console-commands-overhaul/19.8-06-SUMMARY.md"
|
||||
provides: "the release proposal, the reply, the tag, pushed ranges, the changelog URL and the --all stage summary"
|
||||
contains: "## Task 1 reply"
|
||||
key_links:
|
||||
- from: "framework tag vX.Y.Z"
|
||||
to: "https://summercms.io/changelog"
|
||||
via: "scripts/changelog-post.py source and publish in ../sm-summercmsio-app"
|
||||
pattern: "changelog-post.py"
|
||||
- from: "../sm-demo-app/plugins/demo/content/ROADMAP.md"
|
||||
to: "origin/master:.planning/ROADMAP.md"
|
||||
via: "scripts/sync-roadmap.sh with ROADMAP_REF=origin/master; the gate's landing stage checks it"
|
||||
pattern: "sync-roadmap"
|
||||
prohibitions:
|
||||
- statement: "No push or tag before the Task 1 reply; never a force-push; never a deploy."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
- statement: "Another session's code commit in origin/master..master stops the release: nothing is pushed until the user decides."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
- statement: "The changelog token is never printed, copied or minted; a missing or refused token stops and asks the user."
|
||||
status: unresolved
|
||||
verification: judgment
|
||||
---
|
||||
|
||||
## Phase Goal
|
||||
|
||||
ROADMAP Goal (not in user-story format; plans 01-04 recorded the decision not to invent one): console commands reach the Artisan bar. This plan (gap closure 2 of 2, wave 2) publishes plan 05's review fixes to the applications' build path: a patch tag with its changelog post, under a go-ahead, then proves the whole phase again with the gate.
|
||||
|
||||
## Spec-less probe fallback
|
||||
|
||||
Spec-less probe fallback skipped: no REQUIREMENTS.md IDs and no SPEC.md (as in plans 01-05). The release follows D-26 (nothing pushed or tagged without the go-ahead) and the procedure of plan 03 Task 7 step (1) and plan 04 Tasks 5-6.
|
||||
|
||||
## Decisions taken in this plan
|
||||
|
||||
- **Same shape as v0.4.1.** The checkpoint and the release copy plan 04 Tasks 5-6: range check after a fetch, annotated tag, push of the exact tagged commit to master, then the tag, then the changelog post. The proposed tag is v0.4.2 unless another tag appeared after v0.4.1, in which case the proposal is the next patch after the newest tag.
|
||||
- **Plugins and applications.** Only rows of plan 05's Release impact table that say "needs require" or "regenerate" lead to commits there; plan 05 expects none. A consumer picks the fixes up with its next framework require bump.
|
||||
- **Demo roadmap snapshot.** The gate's landing stage compares sm-demo-app's `plugins/demo/content/ROADMAP.md` with origin/master's `.planning/ROADMAP.md`. The release push publishes this phase's and other sessions' roadmap changes, which turned the landing stage red after v0.4.1. Task 3 re-syncs it when the check fails, adding Polish names first for any new phase (TestRoadmapPolishTitles), so every sm-demo-app commit is green.
|
||||
|
||||
<objective>
|
||||
Release plan 05's fixes as v0.4.2 under the user's go-ahead, with the same-day changelog post, keep the demo roadmap snapshot in step with what was published, and finish with a green phase gate.
|
||||
|
||||
Purpose: D-26 (nothing published without the go-ahead) and the changelog rule in CLAUDE.md; the fixes only help applications once they are tagged.
|
||||
Output: on release, tag vX.Y.Z on origin and a changelog post; on push-only, master on origin; in every option, a green `--all` run in the SUMMARY.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@~/.claude/msd-core/workflows/execute-plan.md
|
||||
@~/.claude/msd-core/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-04-SUMMARY.md
|
||||
|
||||
Release procedure to repeat: 19.8-03-PLAN.md Task 7 step (1) and 19.8-04-PLAN.md Tasks 5 and 6 (and the "Task 6: published" section of 19.8-04-SUMMARY.md). Changelog: ../sm-summercmsio-app/DEPLOY.md "Posting to the changelog" and the usage header of ../sm-summercmsio-app/scripts/changelog-post.py.
|
||||
|
||||
Concurrency: other sessions commit planning documents (and sometimes code) to summercms.go and may push. Stage by explicit path, never `git add -A`, never touch `.planning/STATE.md` or `.planning/state.json` by hand, no co-author tags. In sm-demo-app commit only the two roadmap paths and check `git status --porcelain` and `git log origin/master..master` there first. Docker tests need `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1`; if the socket is denied, run through `sudo -u golem`.
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="checkpoint:decision" gate="blocking">
|
||||
<name>Task 1: Go-ahead to publish the review fixes: release v0.4.2, push-only, or hold</name>
|
||||
<read_first>19.8-05-SUMMARY.md (fix commits, RED replay, verification, Release impact table), 19.8-04-SUMMARY.md ("Task 6: published": how v0.4.1 went out), 19.8-04-PLAN.md Task 5 (the checkpoint this one copies), CLAUDE.md (Commits and concurrency, the changelog rule)</read_first>
|
||||
<acceptance_criteria>
|
||||
- Before the checkpoint returns, a draft 19.8-06-SUMMARY.md holds a `## Release proposal` section: the output of `git fetch origin` and of `git log --format='%h %s' origin/master..master`, each commit classified as plan 05 code, planning documents (this phase or another session) or other code (with its paths from `git show --name-only --format= <hash>`); the newest tag (`git tag --sort=-v:refname`, first line) and the proposed tag; plan 05's Release impact table; the result of `ROADMAP_REF=origin/master ../sm-demo-app/scripts/sync-roadmap.sh --check`; and plan 05's verification status (Docker run or NOT RUN).
|
||||
- Before the reply, `T=$(git tag --points-at HEAD) && test -z "$T"` exits 0.
|
||||
- After the reply, the SUMMARY records it verbatim under `## Task 1 reply`, with the chosen tag on a release.
|
||||
</acceptance_criteria>
|
||||
<decision>Publish plan 05's fixes now, and with which patch tag?</decision>
|
||||
<context>Plan 05 fixed the eleven critical and warning findings of the code review, each in its own commit with a gated pinning test, and recorded a Release impact table. Releasing means an annotated patch tag (proposed v0.4.2) on the tested master commit, pushing master as exactly that commit and then the tag, and a same-day changelog post with the fixes under ## Fixed. Pushing master also publishes every unpushed planning-document commit of other sessions listed in the proposal; another session's code commit in the range blocks the release until you decide. Plugins and applications get commits only where the Release impact table says so (expected: none). After the push, sm-demo-app's roadmap snapshot is re-synced if it no longer matches origin/master (Task 3). Nothing is deployed in any option. If the proposal shows another session's code in the range, a red check or a Docker NOT RUN in plan 05, the recommended answer is hold.</context>
|
||||
<options>
|
||||
<option id="release">
|
||||
<name>release <tag> (for example "release v0.4.2")</name>
|
||||
<pros>Applications and plugins can pin the fixes; the changelog shows them the same day; the demo roadmap snapshot follows the published roadmap.</pros>
|
||||
<cons>Another tag and changelog post one day after v0.4.1; publishes other sessions' planning commits.</cons>
|
||||
</option>
|
||||
<option id="push-only">
|
||||
<name>push-only</name>
|
||||
<pros>The fixes are on origin/master for the next release; no tag or post today.</pros>
|
||||
<cons>Tagged consumers keep v0.4.1 and its defects until the next tag.</cons>
|
||||
</option>
|
||||
<option id="hold">
|
||||
<name>hold</name>
|
||||
<pros>Nothing leaves this machine.</pros>
|
||||
<cons>The fixes stay local; verification sees them unpublished.</cons>
|
||||
</option>
|
||||
</options>
|
||||
<resume-signal>Reply "release vX.Y.Z", "push-only" or "hold". "release" and "push-only" also authorise pushing sm-demo-app's Polish-name and roadmap-snapshot commits from Task 3 when they are its only unpushed commits. A release also authorises the plugin require bumps and application regenerate commits that plan 05's Release impact table names (expected none). Nothing is ever force-pushed or deployed.</resume-signal>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 2: Under the Task 1 reply, publish: range check, checks, annotated tag on the tested commit, push master then the tag, same-day changelog post; plugin and application commits only where plan 05's table asks</name>
|
||||
<precondition>19.8-06-SUMMARY.md (draft) records the Task 1 reply under `## Task 1 reply`. On "hold" this task records "release held, nothing published" and stops.</precondition>
|
||||
<files>.planning/phases/19.8-console-commands-overhaul/19.8-06-SUMMARY.md</files>
|
||||
<read_first>19.8-06-SUMMARY.md draft (Release proposal, Task 1 reply), 19.8-05-SUMMARY.md (fix commits and Release impact table), 19.8-03-PLAN.md Task 7 step (1), 19.8-04-SUMMARY.md ("Task 6: published"), ../sm-summercmsio-app/DEPLOY.md ("Posting to the changelog"), ../sm-summercmsio-app/scripts/changelog-post.py (usage header only)</read_first>
|
||||
<action>Per D-26, under the recorded reply. Never force-push, never deploy, never print the changelog token.
|
||||
|
||||
On "release vX.Y.Z":
|
||||
(1) `git fetch origin`; list `git log --format='%h %s' origin/master..master` and classify each commit with `git show --name-only --format= <hash>`: a commit touching only `.planning/` is a planning document and may go out; a commit touching anything else must be one of plan 05's commits listed in its SUMMARY. Any other code commit stops the task: record it in the SUMMARY and ask the user (nothing pushed). Confirm the newest tag is still the one the proposal named; if another tag appeared since, stop and ask (the reply's tag may now be wrong).
|
||||
(2) At master HEAD with a clean tree for this phase's paths (`git status --porcelain`; other sessions' STATE.md and state.json edits are left alone): `go vet ./...`, `go test -count=1 -short ./...` and `go run ./cmd/summer docs:build --check` green. Record the tested commit's hash.
|
||||
(3) `git tag -a vX.Y.Z <tested hash>` with a message: first line `SummerCMS vX.Y.Z`, a blank line, one sentence ("Fixes from the review of the 0.4 console."), then one bullet per fix in user words (no CR/WR IDs, no planning or quick codes): lines printed during spinners and progress bars keep their order and none is lost; Ctrl+C during a spinner or progress bar restores the cursor; a command's output never lands in another command's live view; summer's delegated commands get the terminal (production question, styled output), keep their stderr apart from data and exit with the binary's exit code; summer dev lets serve finish its shutdown; a daily log file that failed to open is retried; replaced log files are closed; error blocks, process views and tables strip terminal control characters; the wrong-directory hint masks password, secret, token and key values; migrate reports what it actually applied (adjust to what plan 05 actually shipped).
|
||||
(4) `git push origin <tested hash>:refs/heads/master` (a fast-forward of exactly the tagged commit, so commits landing meanwhile are not published), then `git push origin vX.Y.Z`; `git ls-remote --exit-code --tags origin vX.Y.Z` exits 0.
|
||||
(5) Changelog, the same day, from ../sm-summercmsio-app: `scripts/changelog-post.py source vX.Y.Z`; write user-facing Markdown notes to a file in the session scratchpad: no H1, a `## Fixed` section with the bullets above (more readable sentences are fine; no planning IDs, no quick codes); then `scripts/changelog-post.py publish vX.Y.Z <notes file> --excerpt "<one line: console fixes for 0.4>"`. If the token file is missing or the token is refused, stop and ask the user. Record the post URL.
|
||||
(6) For each plan 05 Release impact row marked "needs require": in that plugin repository `go get git.golem15.com/golem15/summercms@vX.Y.Z`, `go mod tidy`, vet and short tests, one commit `chore: require SummerCMS vX.Y.Z`, push. For each row marked "regenerate": regenerate main.go, build, `bin/<app> list`, one commit `build: regenerate main for SummerCMS vX.Y.Z`, push. With no such rows record "plugins and applications: nothing to publish".
|
||||
|
||||
On "push-only": steps (1) and (2), then `git push origin <tested hash>:refs/heads/master`; no tag, no changelog post.
|
||||
|
||||
Record in the SUMMARY: the range classification after the fetch, the tested hash, the tag object, the pushed range, the changelog URL, and every plugin or application commit (or "none").</action>
|
||||
<verify>
|
||||
<automated>T="${RELEASE_TAG:?set RELEASE_TAG to the tag named in the Task 1 reply}" && git ls-remote --exit-code --tags origin "$T" && git fetch origin && git merge-base --is-ancestor "$T^{commit}" origin/master && test "$(git cat-file -t "$T")" = tag</automated>
|
||||
<fails_when>On release: the newest reachable tag is not on origin, the tagged commit is not on origin/master, or the tag is lightweight instead of annotated. On "push-only" or "hold" this command is not run; the SUMMARY records the pushed range or "nothing published".</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- On release: `git ls-remote --exit-code --tags origin vX.Y.Z` exits 0, `git cat-file -t vX.Y.Z` prints `tag`, and the SUMMARY holds the changelog post URL (https://summercms.io/changelog/...).
|
||||
- The SUMMARY lists every published repository with its pushed range, or "nothing published" on hold.
|
||||
- The SUMMARY's range classification lists no "other code" commit, or records that the task stopped because of one.
|
||||
</acceptance_criteria>
|
||||
<done>The fixes are published exactly as the user chose, with a tag and changelog post on release, and nothing else of another session's code went out.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto">
|
||||
<name>Task 3: The demo roadmap snapshot matches the published roadmap (Polish names for new phases first, then the snapshot, in sm-demo-app, only when the check fails), and the whole phase gate is green with Docker required</name>
|
||||
<files>../sm-demo-app/plugins/demo/roadmap/names.go, ../sm-demo-app/plugins/demo/content/ROADMAP.md, .planning/phases/19.8-console-commands-overhaul/19.8-06-SUMMARY.md</files>
|
||||
<read_first>../sm-demo-app/scripts/sync-roadmap.sh (header: ROADMAP_REF, --check), ../sm-demo-app/plugins/demo/roadmap/names.go (phaseText entries, the client-name denylist), ../sm-demo-app/plugins/demo/roadmap/*_test.go (TestRoadmapPolishTitles, TestRoadmapNoClientNames), `git -C ../sm-demo-app log --oneline -5` (the earlier "Polish name for roadmap phase" and "sync the ROADMAP.md snapshot" commits as the pattern), scripts/check-phase19.8.sh (run_landing), 19.8-04-SUMMARY.md ("Gate after the release": the red landing stage after v0.4.1)</read_first>
|
||||
<action>(1) Conditional roadmap sync. `git fetch origin` in summercms.go, then `ROADMAP_REF=origin/master ../sm-demo-app/scripts/sync-roadmap.sh --check`. If it prints ok, record "snapshot in step, sm-demo-app untouched" and go to (2). Otherwise, in sm-demo-app: check `git status --porcelain` and `git log --oneline origin/master..master` (another session's uncommitted roadmap edits or unpushed commits there: stop and report); run `ROADMAP_REF=origin/master scripts/sync-roadmap.sh` to write the snapshot, then `go test -count=1 -short -run '^(TestRoadmapParse|TestRoadmapDeckOrder|TestRoadmapNoClientNames|TestRoadmapPolishTitles)$' ./plugins/demo/roadmap`. If TestRoadmapPolishTitles names a phase without a Polish title (or an open phase without a Polish summary), add its `phaseText` entry to plugins/demo/roadmap/names.go in the style of the existing entries: a short Polish title and, for open phases, a one-line Polish summary of the roadmap goal, naming no client application (TestRoadmapNoClientNames enforces the denylist). Commit so that every commit is green: first names.go alone (`feat(demo): Polish name for roadmap phase <N>`, one commit naming all new phases if several), then the snapshot alone (`chore: sync the ROADMAP.md snapshot (summercms <short hash of origin/master>)`), staged by path. Re-run the four roadmap tests. Push sm-demo-app only when the Task 1 reply was "release" or "push-only" and `git log origin/master..master` there lists only these commits; on "hold" leave them local and say so. Never deploy.
|
||||
|
||||
(2) Gate. Run `PHASE198_REQUIRE_DOCKER=1 TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh --all` (through `sudo -u golem` if the Docker socket is denied) and paste its stage summary into the SUMMARY. A red stage is diagnosed and fixed through the owning repository's normal flow (a framework defect goes back to a plan 05 style fix commit and blocks completion; a sibling repository's unrelated pre-existing failure is recorded with evidence that it also fails without this plan's commits) and the gate re-run until green.
|
||||
|
||||
(3) Record in the SUMMARY a suggested g15office comment for the orchestrator (no g15office tool here): on release "Phase 19.8 review fixes released as SummerCMS vX.Y.Z (changelog: <URL>); phase gate green; ready for /msd-verify-work", otherwise the matching hold or push-only wording.</action>
|
||||
<verify>
|
||||
<automated>ROADMAP_REF=origin/master ../sm-demo-app/scripts/sync-roadmap.sh --check && LOG="${TMPDIR:?set TMPDIR to the session scratchpad}/check-phase19.8-06.log" && PHASE198_REQUIRE_DOCKER=1 TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh --all >"$LOG" 2>&1 && tail -20 "$LOG" && grep -q '^Phase 19.8 gate passed$' "$LOG" && ! grep -qE '^(FAIL|NOT RUN docker)' "$LOG"</automated>
|
||||
<fails_when>The demo snapshot differs from origin/master's ROADMAP.md, the gate exits non-zero or lacks "Phase 19.8 gate passed", a stage line starts with FAIL, or docker did not run (PHASE198_REQUIRE_DOCKER=1 turns a missing daemon into a failure).</fails_when>
|
||||
</verify>
|
||||
<acceptance_criteria>
|
||||
- The SUMMARY holds the --all stage summary with every stage PASS (TestTerminalCommands may be NOT RUN).
|
||||
- When sm-demo-app was changed: `git -C ../sm-demo-app log --format=%s -2` shows the Polish-name commit (when one was needed) before the snapshot commit, `git -C ../sm-demo-app show --name-only --format= HEAD` names only plugins/demo/content/ROADMAP.md, and on release or push-only `git -C ../sm-demo-app log --oneline origin/master..master` prints nothing.
|
||||
- When sm-demo-app was not changed, the SUMMARY says the snapshot was already in step.
|
||||
</acceptance_criteria>
|
||||
<done>The published roadmap and the demo deck agree, and one green gate run proves the whole phase after the release.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| this machine to git.golem15.com | pushes and tags publish framework code, other sessions' planning commits and sm-demo-app commits |
|
||||
| this machine to summercms.io | the changelog post, authenticated by a token file |
|
||||
| roadmap to the public demo deck | phase titles and summaries shown to prospective clients |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|----------|-------------|-----------------|
|
||||
| T-19.8-34 | Repudiation | release publication | high | mitigate | blocking checkpoint before any push; after a fetch every commit in origin/master..master is classified by its paths, and any code commit not listed by plan 05 stops the release |
|
||||
| T-19.8-35 | Tampering | pushed master | medium | mitigate | push exactly the tested, tagged commit as a fast-forward (`<hash>:refs/heads/master`); never force-push; vet, short tests and the docs checker green at that commit |
|
||||
| T-19.8-36 | Information disclosure | changelog token | medium | mitigate | changelog-post.py reads the token file itself; the token is never printed, copied or minted; a missing or refused token stops and asks the user |
|
||||
| T-19.8-37 | Information disclosure | demo roadmap deck | medium | mitigate | new Polish names follow the existing entries and TestRoadmapNoClientNames rejects client application names before the snapshot commit |
|
||||
| T-19.8-SC | Tampering | dependencies | low | accept | no module is added; plugin requires move only to this framework's own tag, and only for rows of plan 05's table |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
- Task 1: proposal written before the checkpoint; no tag on HEAD before the reply; reply recorded verbatim.
|
||||
- Task 2: on release, tag annotated and on origin, its commit on origin/master, changelog URL recorded; on push-only, pushed range recorded; on hold, nothing published.
|
||||
- Task 3: demo snapshot equals origin/master's roadmap; --all green with Docker required.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- The user's choice is executed exactly: release (tag, push, changelog), push-only or hold; nothing deployed or force-pushed.
|
||||
- No other session's code went out with the release.
|
||||
- The demo roadmap snapshot and Polish names are current, in separate sm-demo-app commits, only when needed.
|
||||
- `bash scripts/check-phase19.8.sh --all` is green with Docker required.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Create `.planning/phases/19.8-console-commands-overhaul/19.8-06-SUMMARY.md` when done, with the release proposal, the Task 1 reply verbatim, the range classification, the tested hash, the tag and tag object, the pushed ranges, the changelog post URL, plugin and application commits (or "none"), the sm-demo-app commits (or "snapshot already in step"), the --all stage summary, and the suggested g15office comment for the orchestrator.
|
||||
</output>
|
||||
@@ -0,0 +1,92 @@
|
||||
---
|
||||
phase: 19.8-console-commands-overhaul
|
||||
plan: 06
|
||||
subsystem: release
|
||||
tags: [release, v0.4.2, changelog, demo-roadmap, gate]
|
||||
status: draft
|
||||
gap_closure: true
|
||||
---
|
||||
|
||||
# Phase 19.8 Plan 06: Release of the Review Fixes Summary (draft)
|
||||
|
||||
**Draft. Task 1 (the release go-ahead) is waiting for the user's reply. Nothing has been pushed or tagged.**
|
||||
|
||||
## Release proposal
|
||||
|
||||
Prepared on 2026-10-22 at master `8020b17` (before this draft's own commit). All commands below are read-only towards origin, apart from `git fetch`, which only updates local refs.
|
||||
|
||||
### `git fetch origin`
|
||||
|
||||
Exit 0, no output: origin/master is still `6c2164a` (`docs(19.8-04): v0.4.1 released; roadmap progress 4/4 plans`). No other session has pushed since v0.4.1.
|
||||
|
||||
### `git log --format='%h %s' origin/master..master` (18 commits), classified
|
||||
|
||||
| Commit | Subject | Class | Paths |
|
||||
|--------|---------|-------|-------|
|
||||
| 8020b17 | docs(19.8-05): roadmap progress 5/6 plans | planning, this phase | .planning/ROADMAP.md |
|
||||
| 89c8325 | docs(19.8-05): complete review fixes plan | planning, this phase | .planning/phases/19.8-.../19.8-05-SUMMARY.md |
|
||||
| d311d02 | docs(19.8): review fixes recorded, info findings captured as a todo | planning, this phase | 19.8-REVIEW-DISPOSITION.md, .planning/todos/pending/2026-10-22-console-review-info-findings.md |
|
||||
| 1bf64b0 | test(19.8): gate requires the review fixes' tests | plan 05 code | scripts/check-phase19.8.sh |
|
||||
| 020fe83 | fix(lagoon): one migration set list for Migrate and PendingMigrations | plan 05 code (WR-07) | modules/lagoon/{README.md,commands.go,migrations.go,pending_test.go} |
|
||||
| a9025c0 | fix(bonfire): strip control characters from error blocks, process views and tables | plan 05 code (WR-05) | docs/console/writing-commands.md, modules/bonfire/{README.md,errors.go,errors_test.go,helpers.go,live.go,live_test.go,widgets.go,widgets_test.go}, modules/diary/console.go |
|
||||
| 39677f1 | fix(diary): retire replaced daily writers and serialise ApplyGlobals | plan 05 code (WR-06) | modules/diary/{daily.go,diary.go,retire_internal_test.go} |
|
||||
| c586c6f | fix(diary): retry a daily file that failed to open | plan 05 code (WR-04) | docs/services/logging.md, modules/diary/{README.md,daily.go,daily_test.go} |
|
||||
| 9b62285 | fix(bonfire): mask secret flag values in the wrong-directory hint | plan 05 code (WR-08) | docs/console/writing-commands.md, modules/bonfire/{README.md,errors.go,errors_test.go} |
|
||||
| fc7706d | fix(dev): give serve the full shutdown grace before killing it | plan 05 code (WR-03) | cmd/summer/runtime.go, docs/console/scaffolding.md, internal/dev/{watch.go,watch_test.go} |
|
||||
| 9c89705 | fix(summer): give delegated commands the terminal and keep their stderr apart | plan 05 code (CR-03) | cmd/summer/{runtime.go,runtime_test.go}, docs/console/introduction.md, docs/setup/upgrading.md |
|
||||
| 323413c | fix(summer): pass the app binary's exit code through and print its error once | plan 05 code (CR-02) | cmd/summer/{runtime.go,runtime_test.go}, docs/console/introduction.md, docs/setup/upgrading.md, modules/bonfire/{README.md,errors.go,errors_test.go,execute.go} |
|
||||
| d151a25 | fix(bonfire): restore the terminal when a live view is interrupted | plan 05 code (WR-01) | modules/bonfire/{README.md,live.go,live_signal_unix_test.go} |
|
||||
| 86e4e27 | fix(bonfire): reroute only the owning console's lines to a live view | plan 05 code (WR-02) | modules/bonfire/{README.md,helpers.go,live.go,live_test.go,output.go,widgets.go} |
|
||||
| b077dc3 | fix(bonfire): keep the order of lines printed above a live view and drain them before it ends | plan 05 code (CR-01) | modules/bonfire/{live.go,live_test.go,widgets.go} |
|
||||
| e8f5b1c | docs(19.8): gap-closure plans for the code review findings and v0.4.2 | planning, this phase | .planning/ROADMAP.md, 19.8-05-PLAN.md, 19.8-06-PLAN.md |
|
||||
| dd12382 | docs(19.8): code review of the console overhaul | planning, this phase | 19.8-REVIEW.md |
|
||||
| 0b1d7e5 | docs(19.8): record code review disposition | planning, this phase | 19.8-REVIEW-DISPOSITION.md |
|
||||
|
||||
**Result:** 12 plan 05 code commits (all twelve listed in 19.8-05-SUMMARY.md), 6 planning documents of this phase, **0 planning documents of other sessions, 0 other code**. This draft's own commit (`docs(19.8-06): ...`, planning only) will be the 19th. The range check is repeated after a fresh fetch in Task 2 step (1).
|
||||
|
||||
### Tags
|
||||
|
||||
- Newest tag (`git tag --sort=-v:refname | head -1`): **v0.4.1** (on `8bacc22`, an ancestor of origin/master).
|
||||
- **Proposed tag: v0.4.2.**
|
||||
- `T=$(git tag --points-at HEAD) && test -z "$T"`: exits 0 (no tag on HEAD).
|
||||
|
||||
### Release impact table (from 19.8-05-SUMMARY.md)
|
||||
|
||||
| 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 |
|
||||
|
||||
No row asks for a require bump or a regenerate: **plugins and applications get no commits** under any reply.
|
||||
|
||||
### Demo roadmap snapshot
|
||||
|
||||
- `ROADMAP_REF=origin/master ../sm-demo-app/scripts/sync-roadmap.sh --check`: **`sync-roadmap: ok`** (exit 0). sm-demo-app is clean (`git status --porcelain` empty) and has no unpushed commits (`origin/master..master` empty).
|
||||
- Against local master the check fails, as expected: the unpushed `.planning/ROADMAP.md` changes are 19.8's own plan list and progress (`4/4` to `5/6`, plans 05 and 06 listed). They add no new phase, so no Polish name is needed. After a push (release or push-only), Task 3 makes one sm-demo-app commit, `chore: sync the ROADMAP.md snapshot (summercms <hash>)`, and pushes it only under release or push-only.
|
||||
|
||||
### Plan 05 verification status and checks at HEAD now
|
||||
|
||||
- Plan 05: Docker-backed tests **run** (direct socket): `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...` PASS and the gate's docker stage PASS (TestMigrateAppliedLines included). Gate stages self-test, go, hello, docs PASS.
|
||||
- Re-run now at `8020b17`: `go vet ./...` **PASS**; `go run ./cmd/summer docs:build --check` **PASS** ("no problems found"); `go test -count=1 -short ./...` **PASS** with `GOCACHE` in the session scratchpad (tmpfs).
|
||||
- **Environment warning:** the first short run failed in `cmd/summer` (TestNewCommandViaCLI: `go mod tidy ... no space left on device`). `/home` is full (477G, 1.7G free; `~/.cache/go-build` is 33G) and `/media/nvme` is at 99% (19G free). The code is not at fault: the same suite is green with the build cache on tmpfs. Task 2 step (2) and the Task 3 `--all` gate (nuxt generate, Docker images, app builds) will run with `GOCACHE` on tmpfs; freeing space on `/home` (for example `go clean -cache`) is the user's call and was not done here.
|
||||
|
||||
### Recommendation
|
||||
|
||||
**release v0.4.2.** The range holds only plan 05's code and this phase's planning documents, every check is green (Docker included in plan 05), and the table needs no plugin or application commits.
|
||||
|
||||
## Task 1 reply
|
||||
|
||||
The user's reply, verbatim:
|
||||
|
||||
> release
|
||||
|
||||
Read as the recommended option with the proposed tag: **release v0.4.2** (RELEASE_TAG=v0.4.2). The reply authorises an annotated tag v0.4.2 on the tested commit, a fast-forward push of exactly that commit to origin master followed by the tag, the same-day changelog post, the sm-demo-app roadmap snapshot commit and its push (when it is the only unpushed commit there), then the `--all` gate. No force push, no deploy.
|
||||
@@ -0,0 +1,118 @@
|
||||
---
|
||||
phase: 19.8
|
||||
review: 19.8-REVIEW.md
|
||||
titles: json
|
||||
findings:
|
||||
- id: CR-01
|
||||
severity: critical
|
||||
disposition: fixed
|
||||
title: "Lines printed above a live view are reordered and can be lost"
|
||||
- id: CR-02
|
||||
severity: critical
|
||||
disposition: fixed
|
||||
title: "summer delegates print a second error block and turn exit code 2 into 1"
|
||||
- id: CR-03
|
||||
severity: critical
|
||||
disposition: fixed
|
||||
title: "summer delegates hide the terminal from the app and merge its stderr into stdout"
|
||||
- id: WR-01
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "Ctrl+C during a spinner, progress bar or process view leaves the cursor hidden"
|
||||
- id: WR-02
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "Any console's Println and Printf are sent to whichever live view is running"
|
||||
- id: WR-03
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "summer dev kills serve 2 seconds after SIGTERM, but serve drains for up to 10"
|
||||
- id: WR-04
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "One failed open switches a daily log file off until the process exits"
|
||||
- id: WR-05
|
||||
severity: warning
|
||||
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: fixed
|
||||
title: "Setup and ApplyGlobals close daily files that other goroutines may still be writing through"
|
||||
- id: WR-07
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "PendingMigrations keeps its own copy of Migrate's migration sets"
|
||||
- id: WR-08
|
||||
severity: warning
|
||||
disposition: fixed
|
||||
title: "The wrong-directory hint repeats every argument, including a --password value, on stderr"
|
||||
- id: IN-01
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "A forced-color console drops HideOnTTY records even when stderr is not a terminal"
|
||||
- id: IN-02
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "PeekGlobals and cobra disagree on some argument forms"
|
||||
- id: IN-03
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "isUsage recognises cobra's unknown-command error by its text"
|
||||
- id: IN-04
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "The --json flag is read two different ways"
|
||||
- id: IN-05
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "summer's scaffolding result lines are now hidden by -q"
|
||||
- id: IN-06
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "fang replaces the root help function that NewRootIO installs"
|
||||
- id: IN-07
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "diary pulls the whole TUI stack into security modules"
|
||||
- id: IN-08
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "The console sanitizer passes raw C1 bytes through on its fast path"
|
||||
- id: IN-09
|
||||
severity: info
|
||||
disposition: deferred
|
||||
title: "The phase gate names consuming applications"
|
||||
open: 0
|
||||
total: 20
|
||||
recorded: 2026-10-21T22:35:50.040Z
|
||||
---
|
||||
|
||||
# Phase 19.8: Code Review Disposition
|
||||
|
||||
| Finding | Severity | Disposition | Source |
|
||||
|---------|----------|-------------|--------|
|
||||
| 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.
|
||||
Re-running the gate keeps every row it can. A row the current review no longer reports is kept and its Source cell flagged, so a finding does not leave this record silently. ONE exception: when a finding id is REUSED by a different finding, the earlier decision cannot keep a row — the id is taken — and it is dropped. A RECORDED decision (anything but `open`) is named on the console when that happens; a row still at `open` is replaced silently, because `open` records no decision to lose.
|
||||
336
.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md
Normal file
336
.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md
Normal file
@@ -0,0 +1,336 @@
|
||||
---
|
||||
phase: 19.8-console-commands-overhaul
|
||||
reviewed: 2026-10-21T22:33:36Z
|
||||
depth: standard
|
||||
diff_range: v0.3.1..v0.4.1 (19.5 commits and the cooler test commit excluded)
|
||||
files_reviewed: 93
|
||||
files_reviewed_list:
|
||||
- cmd/summer/docs.go
|
||||
- cmd/summer/main.go
|
||||
- cmd/summer/parity.go
|
||||
- cmd/summer/runtime.go
|
||||
- docs/examples/blog/console/publish.go
|
||||
- examples/hello/main.go
|
||||
- examples/hello/plugins/greeter/plugin.go
|
||||
- internal/build/artifact.go
|
||||
- internal/build/build.go
|
||||
- internal/build/manifest.go
|
||||
- internal/build/new.go
|
||||
- internal/build/scaffold.go
|
||||
- internal/dev/watch.go
|
||||
- internal/docsite/check_commands.go
|
||||
- modules/beachcomber/beachcomber.go
|
||||
- modules/bonfire/command.go
|
||||
- modules/bonfire/docs.go
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/execute.go
|
||||
- modules/bonfire/globals.go
|
||||
- modules/bonfire/helpers.go
|
||||
- modules/bonfire/help.go
|
||||
- modules/bonfire/list.go
|
||||
- modules/bonfire/live.go
|
||||
- modules/bonfire/output.go
|
||||
- modules/bonfire/prompts.go
|
||||
- modules/bonfire/root.go
|
||||
- modules/bonfire/selector.go
|
||||
- modules/bonfire/styles.go
|
||||
- modules/bonfire/version.go
|
||||
- modules/bonfire/widgets.go
|
||||
- modules/bouncer/turnstile.go
|
||||
- modules/cabana/account.go
|
||||
- modules/cabana/account_preferences.go
|
||||
- modules/cabana/account_tokens.go
|
||||
- modules/cabana/actions.go
|
||||
- modules/cabana/api_tokens.go
|
||||
- modules/cabana/auth.go
|
||||
- modules/cabana/commands.go
|
||||
- modules/cabana/contracts.go
|
||||
- modules/cabana/crud.go
|
||||
- modules/cabana/field_dependent.go
|
||||
- modules/cabana/field_file.go
|
||||
- modules/cabana/field_type.go
|
||||
- modules/cabana/http.go
|
||||
- modules/cabana/log.go
|
||||
- modules/cabana/partial_render.go
|
||||
- modules/cabana/query.go
|
||||
- modules/cabana/relation_field.go
|
||||
- modules/cabana/reorder.go
|
||||
- modules/cabana/singleton.go
|
||||
- modules/cabana/token_commands.go
|
||||
- modules/cabana/token_guard.go
|
||||
- modules/conga/client.go
|
||||
- modules/conga/commands.go
|
||||
- modules/conga/conga.go
|
||||
- modules/conga/worker.go
|
||||
- modules/cooler/cooler.go
|
||||
- modules/cooler/memory.go
|
||||
- modules/cooler/ratelimit.go
|
||||
- modules/cooler/rediscache.go
|
||||
- modules/cooler/sitecache.go
|
||||
- modules/diary/channels.go
|
||||
- modules/diary/component.go
|
||||
- modules/diary/config.go
|
||||
- modules/diary/console.go
|
||||
- modules/diary/daily.go
|
||||
- modules/diary/diary.go
|
||||
- modules/flare/commands.go
|
||||
- modules/flare/flare.go
|
||||
- modules/lagoon/attach/bucket.go
|
||||
- modules/lagoon/attach/thumb.go
|
||||
- modules/lagoon/commands.go
|
||||
- modules/lagoon/connection.go
|
||||
- modules/lagoon/encrypted.go
|
||||
- modules/lagoon/fill.go
|
||||
- modules/lagoon/keygen.go
|
||||
- modules/lagoon/migrations.go
|
||||
- modules/lagoon/purge.go
|
||||
- modules/lagoon/transaction.go
|
||||
- modules/lighthouse/centrifugo/client.go
|
||||
- modules/lighthouse/centrifugo/commands.go
|
||||
- modules/lighthouse/lighthouse.go
|
||||
- modules/phrasebook/translator.go
|
||||
- modules/postcard/drivers.go
|
||||
- modules/postcard/mailer.go
|
||||
- modules/sunscreen/sunscreen.go
|
||||
- modules/surf/accesslog.go
|
||||
- modules/surf/routelist_command.go
|
||||
- modules/surf/serve.go
|
||||
- modules/surf/summary.go
|
||||
- modules/wristband/oidc_token.go
|
||||
- scripts/check-phase19.8.sh
|
||||
findings:
|
||||
critical: 3
|
||||
warning: 8
|
||||
info: 9
|
||||
total: 20
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 19.8: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-10-21T22:33:36Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 93
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
I reviewed the phase's changed hunks in the 93 non-test source files (`git diff v0.3.1..v0.4.1`). I read the new modules (diary, and bonfire's globals, errors, execute, live, prompts, selector and list) in full. I checked the bubbletea v2.1.0, fang v2.0.1 and gorm v1.31.2 sources wherever bonfire, diary or lagoon depend on how they behave. I confirmed one finding at runtime: a scratch program under a pseudo-terminal (`script -qec`) shows that lines printed during a live view come out in the wrong order and are sometimes lost. Items already listed in `deferred-items.md` are not reported again.
|
||||
|
||||
The request log is clean. It logs the route pattern and never the path, query, headers or remote address. The GORM bridge logs SQL with placeholders. The startup summary shows the database only as host, port and name. The redaction wrapping sits on every diary leaf.
|
||||
|
||||
The main defects are in three places:
|
||||
|
||||
- **Printing above live views.** Lines printed while a spinner, progress bar or process view runs are reordered and sometimes dropped. Any console's output can be redirected to the terminal that owns the view.
|
||||
- **summer delegates.** Every `summer <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.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Lines printed above a live view are reordered and can be lost
|
||||
|
||||
**File:** `modules/bonfire/live.go:316-323` (with `printAboveLines` at 377-387 and `runLive` at 365-372)
|
||||
**Issue:** `printAboveLines` sends one `printAboveMsg` per line, and `liveModel.Update` answers each one with `tea.Println(...)`, which is a `tea.Cmd`. bubbletea v2 runs every Cmd in its own goroutine (`handleCommands`, tea.go:745-757), and that goroutine sends the resulting `printLineMessage` back into the event loop. Two consequences follow:
|
||||
|
||||
1. **Order is not kept.** Successive lines race each other back into the event loop.
|
||||
2. **Lines can be dropped.** `runLive` sends `finishMsg` as soon as `work` returns, and `finishMsg` ends in `tea.Quit`. A print Cmd still in flight then reaches a loop that has already exited, and its line is never printed.
|
||||
|
||||
This affects every path that prints during a live view:
|
||||
|
||||
- `Output.Println` and `Printf`, which carry command data (tokens, JSON);
|
||||
- status lines sent through `emit`;
|
||||
- `liveProgress.Fail` failure lines (widgets.go:130-133), which are sent just before the work function returns;
|
||||
- every diary console record logged while a view runs (console.go:166).
|
||||
|
||||
**Runtime check:** a program printing `line-00` to `line-19` with `out.Println` inside `out.Spinner` on a pty printed them out of order in 5 of 5 runs (for example `line-01 line-00 line-02 line-04 line-03 …`) and lost a line in 1 of 40 runs.
|
||||
**Fix:** Deliver the lines through the event loop in order, and drain them before quitting. For example, buffer them in the model and flush them as a single ordered `tea.Sequence`; on finish, quit only after that sequence:
|
||||
```go
|
||||
type liveModel struct{ inner tea.Model }
|
||||
|
||||
func (m liveModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
|
||||
if lines, ok := msg.(printAboveMsg); ok {
|
||||
// one message per PrintAbove call; tea.Sequence runs the prints in order
|
||||
var cmds []tea.Cmd
|
||||
for _, l := range strings.Split(string(lines), "\n") {
|
||||
cmds = append(cmds, tea.Println(l))
|
||||
}
|
||||
return m, tea.Sequence(cmds...)
|
||||
}
|
||||
inner, cmd := m.inner.Update(msg)
|
||||
m.inner = inner
|
||||
return m, cmd
|
||||
}
|
||||
```
|
||||
Ordering across separate PrintAbove calls, and draining before quit, still need one serial path. Either have `runLive` send `finishMsg` only after an acknowledgement message that is queued behind the pending prints, or keep a FIFO in the model and, on `finishMsg`, return `tea.Sequence(<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
|
||||
}
|
||||
```
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Ctrl+C during a spinner, progress bar or process view leaves the cursor hidden
|
||||
|
||||
**File:** `modules/bonfire/widgets.go:33,51`, `modules/bonfire/live.go:349-355`, `cmd/summer/main.go:28,76`, `internal/build/build.go:140`
|
||||
**Issue:** The live views run with `tea.WithoutSignalHandler()` and `tea.WithInput(nil)`, and `Spinner` and `Progress` pass `context.Background()`. Neither `summer` nor the generated main installs a signal handler (`bonfire.Execute(context.Background(), …)`), so Ctrl+C terminates the process with SIGINT's default action. bubbletea hides the cursor when it starts (`ESC[?25l`, confirmed in the pty run) and only shows it again on a clean exit. `summer build`, which always shows a spinner on a terminal, and every plugin command that uses `Progress` therefore leave the user's terminal without a cursor after Ctrl+C.
|
||||
**Fix:** Run `Execute` under `signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)` and give the widgets the command's context (keep it on the console in `PersistentPreRunE`). Alternatively, have `runLive` register its own SIGINT handler that calls `prog.Kill()` (which restores the terminal) and then re-raises the signal.
|
||||
|
||||
### WR-02: Any console's Println and Printf are sent to whichever live view is running
|
||||
|
||||
**File:** `modules/bonfire/output.go:192-212`, `modules/bonfire/live.go:329-339,345-373`
|
||||
**Issue:** `activeProg` is a single process-wide value. `console.Println` and `Printf` call `printAboveLines` without checking that this console owns the view (`emit` checks only `w == c.out`). Any other console that prints while a view runs therefore has its data written to the terminal above that view. This includes a `bonfire.Call` root writing into a caller's `bytes.Buffer`, a nested command, or a console whose stdout is a file. For example, a `Process` loop that calls `bonfire.Call(..., &buf)` gets an empty buffer.
|
||||
|
||||
Two more problems sit in the same code:
|
||||
|
||||
- **Check-then-act race.** `liveActive()` (in `console.live`) and the registration in `runLive` are separate steps. Two widgets started concurrently both start programs, and the first one to finish sets `activeProg = nil` while the other is still running.
|
||||
- **No cleanup on panic.** `runLive` unregisters without `defer`, so a panic in `work` leaves `activeProg` pointing at an orphaned program.
|
||||
|
||||
**Fix:** Store the owning console (or its `out` writer) together with the program, and reroute only when `c == owner`. Register under `liveMu` with a check-and-set that falls back to plain lines when a view is already active. Use `defer` for the unregister, `finishMsg` and wait steps.
|
||||
|
||||
### WR-03: summer dev kills serve 2 seconds after SIGTERM, but serve drains for up to 10
|
||||
|
||||
**File:** `internal/dev/watch.go:227-238`
|
||||
**Issue:** Since this phase `summer dev` runs `bin/<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:
|
||||
```go
|
||||
if d.disabledDate == date { d.mu.Unlock(); return len(p), nil }
|
||||
if err := d.open(date); err != nil { d.disabledDate = date; … }
|
||||
```
|
||||
|
||||
### WR-05: Error blocks, process views and tables write untrusted text to the terminal without stripping escape sequences
|
||||
|
||||
**File:** `modules/bonfire/errors.go:145-169`, `modules/bonfire/live.go:286-289`, `modules/bonfire/widgets.go:142-166`
|
||||
**Issue:** T-19.8-03 strips control characters only in diary's console handler. `RenderError` passes messages, explanations and the `-v` cause chain through `sunscreen.Scrub` only, and Scrub redacts credential shapes but leaves ESC and other control characters in place. The process view prints `it.err.Error()` and item names unchanged, and `Table` prints cell values unchanged. When the profile allows color, `colorprofile.Writer` passes those sequences through.
|
||||
|
||||
The text can come from outside the operator's control: database or remote-service error text (the centrifugo health error, upstream HTTP bodies), and admin-supplied token names shown by `admin:token:list`. Such text can move the cursor, rewrite earlier lines or set the window title on the operator's terminal.
|
||||
**Fix:** Export diary's `sanitize` (for example as `bonfire.Sanitize`, with diary calling it) and apply it to every error-block line, process-view item and error, and table cell before styling.
|
||||
|
||||
### WR-06: Setup and ApplyGlobals close daily files that other goroutines may still be writing through
|
||||
|
||||
**File:** `modules/diary/diary.go:156-179,216-227`
|
||||
**Issue:** `install` swaps the state and then closes the old channels' daily writers. A goroutine that loaded the old state just before the swap still writes through the old `dailyWriter`. Its `Write` finds `file == nil` and reopens the file (`daily.go:233`). Nothing ever closes that writer again, so the descriptor stays open until the GC finalizer closes it.
|
||||
|
||||
`ApplyGlobals` runs in `PersistentPreRunE`, after plugins have booted. By then plugin goroutines (cache, realtime, the worker) may already be logging, so the window is real.
|
||||
|
||||
There is also a check-then-act race: `ApplyGlobals` reads `current` outside `mu`, so two concurrent calls can both rebuild the channels.
|
||||
**Fix:** Mark the old writers closed for good (`d.closed = true`, with `Write` returning `len(p), nil` once it is set) and do not reopen them. Alternatively, reference-count the old state and close it after in-flight `Handle` calls finish. Take `mu` for the whole of `ApplyGlobals`.
|
||||
|
||||
### WR-07: PendingMigrations keeps its own copy of Migrate's migration sets
|
||||
|
||||
**File:** `modules/lagoon/migrations.go:221-245` vs `modules/lagoon/migrations.go:64-115`; `modules/lagoon/commands.go:139-171`
|
||||
**Issue:** `migrationSets` re-lists the framework sets and the plugin loop by hand, and `Migrate` does not use it. When a set is added to `Migrate` (as the queue and deferred sets were), several things go wrong without any failing test:
|
||||
|
||||
- the serve startup summary,
|
||||
- `migrate --pretend`,
|
||||
- the production question's count,
|
||||
- the "N applied" lines,
|
||||
- `migrate:status --json`.
|
||||
|
||||
The "applied" lines are also printed from the list computed before `Migrate` ran, not from what it actually applied.
|
||||
**Fix:** Make `Migrate` iterate over `migrationSets(gdb, plugins)`, so both functions read one list. Optionally, recompute the pending list after `Migrate` and report the difference.
|
||||
|
||||
### WR-08: The wrong-directory hint repeats every argument, including a --password value, on stderr
|
||||
|
||||
**File:** `modules/bonfire/errors.go:220-224` (via `Fail`, `modules/bonfire/execute.go:346-351`)
|
||||
**Issue:** When an application binary fails outside its root, `wrongDirectory` builds the Try line from all of `os.Args[1:]`. For example, `cd bin && ./acme admin:create --email a@b.c --password hunter2` fails at `compass.Open`, and the error block prints `cd ..; bin/acme admin:create --email a@b.c --password hunter2`. The deprecated `--password` flag is still accepted, so a password that was only in the shell history now also reaches CI and deploy logs that capture stderr. `sunscreen.Scrub` does not catch it, because it recognises only Bearer, `sk-` and `x-api-key` shapes.
|
||||
**Fix:** Mask the values of flags whose names look like secrets (`password`, `secret`, `token`, `key`) in the Try line (`--password=***`). Alternatively, print only the command word (`cd ..; bin/acme admin:create …`).
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: A forced-color console drops HideOnTTY records even when stderr is not a terminal
|
||||
|
||||
**File:** `modules/diary/console.go:117-122`
|
||||
**Issue:** The styled `consoleHandler` drops every `HideOnTTY` record unconditionally. It is also used when color is forced (`--ansi`, `FORCE_COLOR`) on a non-terminal stderr. With stdout on a terminal and stderr redirected (`FORCE_COLOR=1 bin/acme serve 2>serve.log`), the "startup summary" and "listening on" records never reach the file, although the HideOnTTY docs promise that only a terminal console skips them.
|
||||
**Fix:** Check `isTerminal(w)` once when the handler is built, and drop the records only when it is true (the same rule `ttyFilter` uses).
|
||||
|
||||
### IN-02: PeekGlobals and cobra disagree on some argument forms
|
||||
|
||||
**File:** `modules/bonfire/globals.go:52-81`
|
||||
**Issue:** `--quiet=true`, `--no-interaction=true`, `--verbose=2` and `--ansi=false` are ignored by `PeekGlobals` but honoured by pflag. A command flag value that looks like a global (`--name -q`) is the reverse: pflag takes it as the flag's value, `PeekGlobals` reads it as a global. The pre-boot diary setup, the error block's `-v` chain and passthrough globals can therefore differ from what the command runs with.
|
||||
**Fix:** Parse `--flag=value` forms with `strconv.ParseBool` and `strconv.Atoi`, or re-parse the args with a throwaway pflag FlagSet that holds only the global flags (`ParseErrorsWhitelist.UnknownFlags = true`).
|
||||
|
||||
### IN-03: isUsage recognises cobra's unknown-command error by its text
|
||||
|
||||
**File:** `modules/bonfire/errors.go:41-47,119`
|
||||
**Issue:** Any error whose message starts with "unknown command" exits 2 and is rewritten as `Command "…" is not defined.`. A plugin error worded that way would be misreported.
|
||||
**Fix:** Turn cobra's error into a `UsageError` in one place (for example a root `Args` or `ValidArgsFunction` check, or by matching it once in `Execute` before rendering), and test it with `errors.As`.
|
||||
|
||||
### IN-04: The --json flag is read two different ways
|
||||
|
||||
**File:** `modules/cabana/token_commands.go:150,217,299`, `modules/lighthouse/centrifugo/commands.go`, `modules/surf/routelist_command.go` vs `modules/lagoon/commands.go` (`flagOn`)
|
||||
**Issue:** cabana, centrifugo and route:list compare the value with `== "true"`, while lagoon uses `strconv.ParseBool`. As a result `--json=1` works for `migrate:status` but not for `admin:token:list`.
|
||||
**Fix:** Add one bonfire helper (for example `bonfire.FlagBool(in, name)`) and use it everywhere.
|
||||
|
||||
### IN-05: summer's scaffolding result lines are now hidden by -q
|
||||
|
||||
**File:** `cmd/summer/main.go:126,334,401`
|
||||
**Issue:** `created <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`.
|
||||
|
||||
### IN-06: fang replaces the root help function that NewRootIO installs
|
||||
|
||||
**File:** `modules/bonfire/root.go:139-141`, `modules/bonfire/execute.go:327-337`
|
||||
**Issue:** `fang.Execute` calls `root.SetHelpFunc(fang's helpFunc)` (fang.go:140), so under `Execute` bonfire's root help function is dead code. Today `Execute` strips `-h` and `--help` when there is no command word, so the replacement is not reachable. A future path that calls the root's help would print fang's layout, not the Artisan list.
|
||||
**Fix:** Set the root help function again after `fang.Execute` sets its own (not possible from outside), or drop fang and call `root.ExecuteContext` with `RenderError` directly. bonfire already turns off every fang feature except the error handler.
|
||||
|
||||
### IN-07: diary pulls the whole TUI stack into security modules
|
||||
|
||||
**File:** `modules/diary/diary.go:21-22`, `modules/bouncer/turnstile.go`, `modules/wristband/oidc_token.go`
|
||||
**Issue:** `diary` imports `bonfire`, which imports cobra, fang, bubbletea, bubbles and huh. bouncer's README now says its "only framework dependency is diary", but in practice the security primitive links the console UI stack. That widens the code a security review of bouncer and wristband has to cover.
|
||||
**Fix:** Move `ColorMode`, `DetectProfile`, `Palette` and `PrintAbove` into a small leaf package (or into diary itself) so that diary does not import bonfire.
|
||||
|
||||
### IN-08: The console sanitizer passes raw C1 bytes through on its fast path
|
||||
|
||||
**File:** `modules/diary/console.go:278-288`
|
||||
**Issue:** A lone `0x9b` byte (8-bit CSI) is invalid UTF-8. `range` decodes it as `U+FFFD`, which is not a control character, so `sanitize` returns the original string with the raw byte in it. Terminals that interpret 8-bit C1 controls then see an escape sequence.
|
||||
**Fix:** Run `strings.ToValidUTF8(s, "<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.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-21T22:33:36Z_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
@@ -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.
|
||||
@@ -11,9 +11,9 @@ import (
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"syscall"
|
||||
"time"
|
||||
|
||||
"git.golem15.com/golem15/summercms/internal/build"
|
||||
"git.golem15.com/golem15/summercms/internal/dev"
|
||||
"git.golem15.com/golem15/summercms/modules/bonfire"
|
||||
)
|
||||
|
||||
@@ -90,7 +90,7 @@ func delegateCommand(name, description, hint string) bonfire.Command {
|
||||
long := description + ".\n\nsummer runs bin/<binary> " + name +
|
||||
" in the application directory, building the binary first when it is missing. Every flag and argument after " +
|
||||
name + ", including --help, is passed to the app binary unchanged, and so are the global options (-v, -q, --ansi, --no-ansi, -n, --env). " +
|
||||
"summer fails when the app binary fails."
|
||||
"When the app binary fails, summer exits with its exit code."
|
||||
if hint != "" {
|
||||
long += "\n\nCommon flags: " + hint + "."
|
||||
}
|
||||
@@ -141,30 +141,60 @@ func runAppBinary(ctx context.Context, argv []string, out bonfire.Output) error
|
||||
return fmt.Errorf("summer: refusing to recurse into %s", bin)
|
||||
}
|
||||
}
|
||||
cmd := newAppCmd(ctx, bin, appDir, argv, out)
|
||||
stdout, stderr := appStreams(out, bonfire.IsTerminal(out))
|
||||
return runAppCmd(newAppCmd(ctx, bin, appDir, argv, stdout, stderr), argv)
|
||||
}
|
||||
|
||||
// appStreams picks the app binary's stdout and stderr. On a terminal
|
||||
// (terminal is bonfire.IsTerminal(out)) the binary gets summer's own
|
||||
// streams, so it sees the terminal: it asks the production question, prints
|
||||
// styled summaries and animates its widgets. Otherwise its stdout goes
|
||||
// through out (tests and bonfire.Call roots capture it) and its stderr to
|
||||
// summer's stderr, so log lines and error blocks never mix into data such
|
||||
// as --json output.
|
||||
func appStreams(out bonfire.Output, terminal bool) (stdout, stderr io.Writer) {
|
||||
if terminal {
|
||||
return os.Stdout, os.Stderr
|
||||
}
|
||||
return out, os.Stderr
|
||||
}
|
||||
|
||||
// runAppCmd runs cmd, the app binary started with argv, forwarding SIGINT
|
||||
// and SIGTERM to it. When the binary exits with a code, the error carries
|
||||
// that code (bonfire.WithExitCode) and is marked as already reported.
|
||||
func runAppCmd(cmd *exec.Cmd, argv []string) error {
|
||||
sigs := make(chan os.Signal, 2)
|
||||
signal.Notify(sigs, os.Interrupt, syscall.SIGTERM)
|
||||
defer signal.Stop(sigs)
|
||||
if err := runChild(cmd, sigs); err != nil {
|
||||
return fmt.Errorf("summer: %s: %w", strings.Join(argv, " "), err)
|
||||
err = fmt.Errorf("summer: %s: %w", strings.Join(argv, " "), err)
|
||||
var exit *exec.ExitError
|
||||
if errors.As(err, &exit) && exit.Exited() {
|
||||
// The binary printed its own error; summer exits with its
|
||||
// code and prints nothing more. A binary ended by a signal
|
||||
// printed nothing, so it gets summer's error block.
|
||||
return bonfire.Reported(bonfire.WithExitCode(err, exit.ExitCode()))
|
||||
}
|
||||
return err
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// appShutdownGrace is how long a cancelled app binary may take to exit after
|
||||
// SIGTERM before it is killed. It is longer than the 10-second shutdown
|
||||
// timeout of serve and the workers, so they finish a graceful shutdown.
|
||||
const appShutdownGrace = 15 * time.Second
|
||||
// SIGTERM before it is killed: the grace summer dev gives serve as well,
|
||||
// longer than the 10-second shutdown timeout of serve and the workers.
|
||||
const appShutdownGrace = dev.ShutdownGrace
|
||||
|
||||
// newAppCmd builds the command that runs the app binary bin with argv in dir.
|
||||
// The child inherits summer's stdin, so the hidden password prompts of
|
||||
// admin:create and admin:reset-password read from the terminal. Cancelling
|
||||
// ctx sends SIGTERM, and the child is killed only after appShutdownGrace.
|
||||
func newAppCmd(ctx context.Context, bin, dir string, argv []string, out io.Writer) *exec.Cmd {
|
||||
// newAppCmd builds the command that runs the app binary bin with argv in
|
||||
// dir, writing to stdout and stderr (see appStreams). The child inherits
|
||||
// summer's stdin, so the hidden password prompts of admin:create and
|
||||
// admin:reset-password read from the terminal. Cancelling ctx sends
|
||||
// SIGTERM, and the child is killed only after appShutdownGrace.
|
||||
func newAppCmd(ctx context.Context, bin, dir string, argv []string, stdout, stderr io.Writer) *exec.Cmd {
|
||||
cmd := exec.CommandContext(ctx, bin, argv...)
|
||||
cmd.Dir = dir
|
||||
cmd.Stdout = out
|
||||
cmd.Stderr = out
|
||||
cmd.Stdout = stdout
|
||||
cmd.Stderr = stderr
|
||||
cmd.Stdin = os.Stdin
|
||||
cmd.Env = os.Environ()
|
||||
cmd.Cancel = func() error {
|
||||
|
||||
@@ -5,9 +5,11 @@ import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"os/exec"
|
||||
"os/signal"
|
||||
"path/filepath"
|
||||
"runtime"
|
||||
"slices"
|
||||
"strings"
|
||||
@@ -189,7 +191,7 @@ func TestRunChildForwardsSignals(t *testing.T) {
|
||||
for _, sig := range []os.Signal{os.Interrupt, syscall.SIGTERM} {
|
||||
t.Run(sig.String(), func(t *testing.T) {
|
||||
var out syncBuffer
|
||||
cmd := newAppCmd(context.Background(), os.Args[0], ".", helperArgv(), &out)
|
||||
cmd := newAppCmd(context.Background(), os.Args[0], ".", helperArgv(), &out, &out)
|
||||
helperEnv(cmd)
|
||||
sigs := make(chan os.Signal, 1)
|
||||
done := make(chan error, 1)
|
||||
@@ -218,7 +220,7 @@ func TestNewAppCmdCancelSendsSIGTERM(t *testing.T) {
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
defer cancel()
|
||||
var out syncBuffer
|
||||
cmd := newAppCmd(ctx, os.Args[0], ".", helperArgv(), &out)
|
||||
cmd := newAppCmd(ctx, os.Args[0], ".", helperArgv(), &out, &out)
|
||||
helperEnv(cmd)
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- runChild(cmd, nil) }()
|
||||
@@ -288,3 +290,126 @@ func TestDelegateExamples(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestExitHelperProcess is not a real test: TestDelegateExitCodePassthrough
|
||||
// re-executes the test binary with SUMMER_EXIT_HELPER set to get a child
|
||||
// that prints an error on stderr and exits 2, or that is killed by SIGKILL.
|
||||
func TestExitHelperProcess(t *testing.T) {
|
||||
switch os.Getenv("SUMMER_EXIT_HELPER") {
|
||||
case "exit2":
|
||||
fmt.Fprintln(os.Stderr, "ERROR the app binary failed")
|
||||
os.Exit(2)
|
||||
case "sigkill":
|
||||
p, _ := os.FindProcess(os.Getpid())
|
||||
_ = p.Signal(syscall.SIGKILL)
|
||||
time.Sleep(10 * time.Second)
|
||||
os.Exit(3)
|
||||
}
|
||||
}
|
||||
|
||||
// exitHelperRunner replaces appRunner with one that runs the exit helper
|
||||
// as the app binary.
|
||||
func exitHelperRunner(t *testing.T, mode string) {
|
||||
t.Helper()
|
||||
prev := appRunner
|
||||
appRunner = func(ctx context.Context, argv []string, out bonfire.Output) error {
|
||||
stdout, stderr := appStreams(out, bonfire.IsTerminal(out))
|
||||
cmd := newAppCmd(ctx, os.Args[0], ".", []string{"-test.run=^TestExitHelperProcess$"}, stdout, stderr)
|
||||
cmd.Env = append(os.Environ(), "SUMMER_EXIT_HELPER="+mode)
|
||||
return runAppCmd(cmd, argv)
|
||||
}
|
||||
t.Cleanup(func() { appRunner = prev })
|
||||
}
|
||||
|
||||
// TestDelegateExitCodePassthrough (CR-02): summer exits with the app
|
||||
// binary's exit code and prints no error block of its own; a binary killed
|
||||
// by a signal gets summer's error block and exit 1.
|
||||
func TestDelegateExitCodePassthrough(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("SIGKILL is POSIX-only")
|
||||
}
|
||||
run := func(t *testing.T, mode string) (string, string, int) {
|
||||
t.Helper()
|
||||
exitHelperRunner(t, mode)
|
||||
var out, errW syncBuffer
|
||||
root, err := bonfire.NewRootIO("summer", toolCommands(), strings.NewReader(""), &out, &errW)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
code := bonfire.Execute(context.Background(), root, []string{"migrate", "--bogus"})
|
||||
return out.String(), errW.String(), code
|
||||
}
|
||||
|
||||
t.Run("exit code", func(t *testing.T) {
|
||||
out, errOut, code := run(t, "exit2")
|
||||
if code != 2 {
|
||||
t.Errorf("exit %d, want the binary's 2", code)
|
||||
}
|
||||
for _, s := range []string{out, errOut} {
|
||||
if strings.Contains(s, " ERROR ") || strings.Contains(s, "summer: migrate") {
|
||||
t.Errorf("summer printed an error block of its own:\nstdout %q\nstderr %q", out, errOut)
|
||||
break
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("killed by a signal", func(t *testing.T) {
|
||||
out, errOut, code := run(t, "sigkill")
|
||||
if code != bonfire.ExitFailure {
|
||||
t.Errorf("exit %d, want 1", code)
|
||||
}
|
||||
if n := strings.Count(errOut, " ERROR "); n != 1 || !strings.Contains(errOut, "summer: migrate --bogus: signal: killed") {
|
||||
t.Errorf("want one error block naming the signal, got %d:\nstdout %q\nstderr %q", n, out, errOut)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// TestDelegateStreams (CR-03): off a terminal the app binary's stdout goes
|
||||
// through summer's Output and its stderr to summer's stderr, never into
|
||||
// summer's stdout.
|
||||
func TestDelegateStreams(t *testing.T) {
|
||||
if runtime.GOOS == "windows" {
|
||||
t.Skip("the fake app binary is a shell script")
|
||||
}
|
||||
app := t.TempDir()
|
||||
if err := os.WriteFile(filepath.Join(app, "summer.yaml"), []byte("module: example.com/acme\nbinary: acme\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := os.MkdirAll(filepath.Join(app, "bin"), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
script := "#!/bin/sh\necho data-line\necho log-line >&2\n"
|
||||
if err := os.WriteFile(filepath.Join(app, "bin", "acme"), []byte(script), 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
t.Chdir(app)
|
||||
out, err := runSummer(t, "route:list", "--json")
|
||||
if err != nil {
|
||||
t.Fatalf("route:list: %v\n%s", err, out)
|
||||
}
|
||||
if !strings.Contains(out, "data-line") {
|
||||
t.Errorf("the binary's stdout did not reach summer's Output: %q", out)
|
||||
}
|
||||
if strings.Contains(out, "log-line") {
|
||||
t.Errorf("the binary's stderr landed in summer's stdout: %q", out)
|
||||
}
|
||||
}
|
||||
|
||||
// TestAppStreams (CR-03): on a terminal the app binary gets summer's own
|
||||
// stdout and stderr; off a terminal its stdout goes through the Output and
|
||||
// its stderr to summer's stderr.
|
||||
func TestAppStreams(t *testing.T) {
|
||||
var buf bytes.Buffer
|
||||
out := bonfire.NewOutput(strings.NewReader(""), &buf, &buf)
|
||||
if bonfire.IsTerminal(out) {
|
||||
t.Fatal("a buffer console reports a terminal")
|
||||
}
|
||||
stdout, stderr := appStreams(out, true)
|
||||
if stdout != os.Stdout || stderr != os.Stderr {
|
||||
t.Errorf("terminal: streams %v, %v; want os.Stdout, os.Stderr", stdout, stderr)
|
||||
}
|
||||
stdout, stderr = appStreams(out, false)
|
||||
if stdout != io.Writer(out) || stderr != os.Stderr {
|
||||
t.Errorf("not a terminal: streams %v, %v; want the Output, os.Stderr", stdout, stderr)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -141,6 +141,10 @@ summer -v migrate --pretend
|
||||
summer admin:token:create jane --scope journal.posts --json
|
||||
```
|
||||
|
||||
The binary writes to your terminal directly, so it asks the production question, prints its styled startup summary and animates its spinners as if you had run it yourself. Its error stream stays separate from its output: `summer route:list --json > routes.json` captures only the JSON, while warnings and log lines still reach the terminal on stderr.
|
||||
|
||||
When the binary fails, `summer` exits with the binary's exit code (a usage error stays `2`) and prints nothing more: the binary has already printed its error. A binary killed by a signal gets an error block from `summer` and exit code `1`.
|
||||
|
||||
When `summer` receives SIGINT or SIGTERM while the binary runs, it passes the signal on and waits for the binary to shut down, so stopping `summer serve` with Ctrl+C or `kill` frees its port.
|
||||
|
||||
`summer` does not rebuild an existing binary before it delegates. Run `summer build` after you change code, or keep `summer dev` running. Every other runtime command, such as `deferred:purge` or the commands your plugins add, is run on the binary directly.
|
||||
|
||||
@@ -20,7 +20,7 @@ summer build
|
||||
summer dev
|
||||
```
|
||||
|
||||
`summer dev` passes its global options on to `serve`: `summer dev -v` shows the per-plugin lines of the startup summary and every component's info logs, `summer dev -q` only warnings and errors. Ctrl+C stops `serve`, which prints its shutdown report, then `summer dev` prints its own report (rebuilds, failed builds, restarts, uptime) and exits 0.
|
||||
`summer dev` passes its global options on to `serve`: `summer dev -v` shows the per-plugin lines of the startup summary and every component's info logs, `summer dev -q` only warnings and errors. Ctrl+C stops `serve` and waits for its shutdown report: `serve` drains the requests in flight for up to 10 seconds, and `summer dev` kills it only if it is still running 15 seconds after the signal (a restart after a rebuild waits the same way). Then `summer dev` prints its own report (rebuilds, failed builds, restarts, uptime) and exits 0.
|
||||
|
||||
Do not edit `main.go` or `plugins.gen.go`: `summer build` rewrites them from the manifest.
|
||||
|
||||
|
||||
@@ -152,7 +152,7 @@ The framework's own commands follow these rules, and so should a plugin's:
|
||||
|
||||
## Errors and hints
|
||||
|
||||
When `Run` returns an error, the binary prints a block on stderr: the ERROR badge and the message, then "Run with -v for details.". With `-v` the block lists the wrapped errors under "Caused by:" instead. Messages pass through `sunscreen.Scrub`, so a token inside an error text is not printed.
|
||||
When `Run` returns an error, the binary prints a block on stderr: the ERROR badge and the message, then "Run with -v for details.". With `-v` the block lists the wrapped errors under "Caused by:" instead. Messages pass through `sunscreen.Scrub`, so a token inside an error text is not printed, and through `bonfire.Sanitize`, which drops control characters: an error text from a database or a remote service cannot move the cursor, clear the screen or retitle the terminal. Table cells and the item names and errors of `bonfire.Process` are cleaned the same way.
|
||||
|
||||
Wrap an error with `bonfire.WithHint` to explain it and to say what to run next. `{bin}` in the Try text becomes `bin/<name>` or `summer`, so the hint names the right program:
|
||||
|
||||
@@ -202,7 +202,7 @@ An application binary also explains the most common mistake: running it from the
|
||||
cd ../..; bin/acme migrate
|
||||
```
|
||||
|
||||
Only the message changes: the binary still reads its configuration from `config/` in the working directory. `summer` is not affected, because its commands find the application root themselves.
|
||||
The suggested command repeats your arguments, except that the value of a flag whose name contains `password`, `passwd`, `secret`, `token` or `key` is masked as `***` (`--password ***`, `--api-token=***`), so a secret typed on the command line does not reach logs that capture stderr. Only the message changes: the binary still reads its configuration from `config/` in the working directory. `summer` is not affected, because its commands find the application root themselves.
|
||||
|
||||
## Documentation coverage
|
||||
|
||||
|
||||
@@ -97,7 +97,7 @@ Every record carries the attribute `component=blog`, and passes the component's
|
||||
|
||||
A daily channel with `path: storage/logs/summer.log` writes `storage/logs/summer-2026-10-21.log` today and `storage/logs/summer-2026-10-22.log` after local midnight. A relative path resolves against the application root, the directory that holds `config/`, and may not leave it; an absolute path is used as given. Files are created with mode 0640 in directories created with mode 0750, because logs carry user identifiers, and a file is only created once a record reaches it. With `days: 14` the files of today and the 13 days before are kept and older files of the same name pattern are deleted; other files in the directory are never touched. `days: 0` keeps every file.
|
||||
|
||||
If the directory cannot be created or the file cannot be opened (a read-only container filesystem, missing permissions), the channel logs one warning from component `diary` on the console naming the path and the error, and is disabled. The start-up never fails because of a log file.
|
||||
If the directory cannot be created or the file cannot be opened (a read-only container filesystem, missing permissions), the channel logs a warning from component `diary` on the console naming the path and the error, once per day. It then skips records and tries to open the file again one minute later, or at the first record of the next day, whichever comes first, so a passing failure (a full disk, too many open files, a directory removed for a moment during rotation) does not end file logging until the next restart. The start-up never fails because of a log file.
|
||||
|
||||
## Redaction
|
||||
|
||||
|
||||
@@ -43,6 +43,7 @@ Applications pin exact versions, so a new release never reaches an application u
|
||||
- **Exit codes.** A failed command exits 1 and a usage error (an unknown command or flag, a wrong number of arguments) exits 2. Errors print as a block on stderr with a hint where the framework knows the fix, and `-v` shows the wrapped errors.
|
||||
- **`-q` keeps data.** `-q` hides information and success lines, spinners and progress bars, but still prints what a command exists to produce: the token of `admin:token:create`, the key of `key:generate` and every `--json` output.
|
||||
- **Production confirmation.** `migrate`, `migrate:rollback`, `deferred:purge` and `queue:clear` ask before they run when the environment is `production` and they run on an interactive terminal; `--force` skips the question. Off a terminal, as in deploy scripts over SSH, nothing changes.
|
||||
- **`summer` delegates keep the binary's exit code.** Since 0.4.2 `summer migrate`, `summer serve` and the other delegated commands exit with the app binary's exit code and print only the binary's error block; 0.4.0 and 0.4.1 printed a second block and exited 1. The binary also gets the terminal again, so the production question, styled summaries and spinners work through `summer`, and its stderr no longer lands in `summer`'s stdout: a script that captured `summer route:list --json` together with log lines now gets only the JSON.
|
||||
- **`summer dev` runs `serve`.** It used to start the binary without a command, which printed the help and stopped; it now runs `bin/<app> serve` with the global options you give `summer dev`.
|
||||
- **Result lines.** `summer build` ends with `Built bin/<app> in <time>`, the `make:` commands print `Created <path>` per file, `migrate` prints `Applied N migrations` or `Nothing to migrate`, and `queue:clear` prints `Cleared N jobs from <queue>`. Adjust scripts that matched the old lower-case lines.
|
||||
|
||||
|
||||
@@ -19,6 +19,13 @@ import (
|
||||
|
||||
const defaultDebounce = 200 * time.Millisecond
|
||||
|
||||
// ShutdownGrace is how long a stopped app binary may take to exit after
|
||||
// SIGTERM before it is killed, for summer dev and summer's delegates alike.
|
||||
// It is longer than the 10-second shutdown timeout of serve (its request
|
||||
// drain) and the workers, so they finish a graceful shutdown and print
|
||||
// their report.
|
||||
const ShutdownGrace = 15 * time.Second
|
||||
|
||||
// Options customizes the watch loop. Tests inject Build and Start.
|
||||
type Options struct {
|
||||
Build func(ctx context.Context, appDir string, out io.Writer) error
|
||||
@@ -224,6 +231,8 @@ func startChild(ctx context.Context, opts Options, binPath string, args []string
|
||||
return &child{cmd: cmd, wait: ch}, nil
|
||||
}
|
||||
|
||||
// stop sends the child SIGTERM and waits for it to exit, killing it only
|
||||
// after ShutdownGrace.
|
||||
func (c *child) stop() {
|
||||
if c == nil || c.cmd == nil || c.cmd.Process == nil {
|
||||
return
|
||||
@@ -231,7 +240,7 @@ func (c *child) stop() {
|
||||
_ = c.cmd.Process.Signal(syscall.SIGTERM)
|
||||
select {
|
||||
case <-c.wait:
|
||||
case <-time.After(2 * time.Second):
|
||||
case <-time.After(ShutdownGrace):
|
||||
_ = c.cmd.Process.Kill()
|
||||
<-c.wait
|
||||
}
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"io"
|
||||
"os"
|
||||
"os/exec"
|
||||
"os/signal"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"sync"
|
||||
@@ -620,3 +621,43 @@ func TestDevGracefulNil(t *testing.T) {
|
||||
t.Fatalf("no dev report:\n%s", out.String())
|
||||
}
|
||||
}
|
||||
|
||||
// TestStopHelperProcess is not a real test: TestDevStopWaitsForGracefulShutdown
|
||||
// re-executes the test binary with DEV_STOP_HELPER=1 to get a child that,
|
||||
// like serve draining requests, takes 2.5 seconds to shut down after
|
||||
// SIGTERM and then exits 0.
|
||||
func TestStopHelperProcess(t *testing.T) {
|
||||
if os.Getenv("DEV_STOP_HELPER") != "1" {
|
||||
return
|
||||
}
|
||||
sigs := make(chan os.Signal, 1)
|
||||
signal.Notify(sigs, syscall.SIGTERM)
|
||||
fmt.Println("ready")
|
||||
select {
|
||||
case <-sigs:
|
||||
time.Sleep(2500 * time.Millisecond)
|
||||
os.Exit(0)
|
||||
case <-time.After(30 * time.Second):
|
||||
os.Exit(3)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDevStopWaitsForGracefulShutdown (WR-03): child.stop sends SIGTERM
|
||||
// and waits for a child that takes longer than two seconds to drain
|
||||
// instead of killing it; the grace exceeds serve's 10-second drain.
|
||||
func TestDevStopWaitsForGracefulShutdown(t *testing.T) {
|
||||
if ShutdownGrace <= 10*time.Second {
|
||||
t.Fatalf("ShutdownGrace %s does not exceed serve's 10-second drain", ShutdownGrace)
|
||||
}
|
||||
t.Setenv("DEV_STOP_HELPER", "1")
|
||||
var out safeBuffer
|
||||
c, err := startChild(t.Context(), Options{Start: startBin}, os.Args[0], []string{"-test.run=^TestStopHelperProcess$"}, &out, &out)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
waitUntil(t, "helper ready", func() bool { return strings.Contains(out.String(), "ready") })
|
||||
c.stop()
|
||||
if st := c.cmd.ProcessState; st == nil || !st.Exited() || st.ExitCode() != 0 {
|
||||
t.Fatalf("child was not allowed to finish its shutdown: %v", st)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -27,7 +27,7 @@ Use cases:
|
||||
- Usage strings and argument-count checks derived from the declared arguments (`<name>` for required, `[name]` for optional).
|
||||
- Scalar flags, bare flags (`bonfire.Flag.Bare`, so `--force` alone stores `true`) and ordered repeatable flags (`bonfire.Flag.Repeatable`, read back through `bonfire.Input.Flags`).
|
||||
- Styled status lines: `bonfire.Output.Info`, `bonfire.Output.Success`, `bonfire.Output.Warning` and `bonfire.Output.Error` (the last one writes to the error stream).
|
||||
- Widgets on the charm stack, ported from SSU's terminal UI: box-drawn tables (`bonfire.Output.Table`, lipgloss), a spinner around a function (`bonfire.Output.Spinner`), a gradient progress bar (`bonfire.Output.Progress`, whose `bonfire.Progress` also implements `bonfire.ItemProgress`) and a multi-row process view for bulk loops (`bonfire.Process`), each a bubbletea program that keeps stdin and signals to the command. Log lines and output printed while a view runs appear above it (`bonfire.PrintAbove`).
|
||||
- Widgets on the charm stack, ported from SSU's terminal UI: box-drawn tables (`bonfire.Output.Table`, lipgloss), a spinner around a function (`bonfire.Output.Spinner`), a gradient progress bar (`bonfire.Output.Progress`, whose `bonfire.Progress` also implements `bonfire.ItemProgress`) and a multi-row process view for bulk loops (`bonfire.Process`), each a bubbletea program that keeps stdin and signals to the command: Ctrl+C or SIGTERM during a view restores the cursor and the terminal, then stops the command as before (or reaches its own signal handler). Log lines, and output the command prints through the console that started the view, appear above a running view (`bonfire.PrintAbove`); other consoles (a `bonfire.Call` buffer, a console on a file) keep writing to their own writers. Table cells, process-view item names and errors, and failure lines are stripped of control characters (`bonfire.Sanitize`).
|
||||
- Prompts: `bonfire.Output.Ask`, `bonfire.Output.Confirm` and `bonfire.Output.Secret` use huh fields on an interactive terminal, and `bonfire.Output.Choice` an arrow-key selector with a confirmation screen. Off a terminal they read lines from stdin as before (a piped password still works), return their defaults when input ends, and `bonfire.Output.Confirm` returns its default without asking. A cancelled prompt returns `bonfire.ErrAborted`. `bonfire.ConfirmProceed` asks before a destructive step only on an interactive terminal.
|
||||
- Degradation: every widget follows the terminal and the global options, and the `bonfire.Output` interface is unchanged for plugins:
|
||||
|
||||
@@ -46,7 +46,7 @@ Use cases:
|
||||
- Hidden commands (`bonfire.Command.Hidden`) run but stay out of the list unless `list --all`.
|
||||
- JSON catalog: `list --format=json` prints every command, hidden ones flagged, with arguments, options, defaults and examples as a `bonfire.Listing` (`bonfire.Describe` builds it from a command slice).
|
||||
- Documentation coverage: `bonfire.CheckDocs` reports commands lacking a description, long help, examples, or argument and option descriptions.
|
||||
- Error block and exit codes: `bonfire.Execute` runs the root through fang and returns `bonfire.ExitOK`, `bonfire.ExitFailure`, or `bonfire.ExitUsage` for an unknown command or flag and a wrong number of arguments (`bonfire.UsageError`). Errors print on the error stream as a block (`bonfire.RenderError`): the ERROR badge and the message, the explanation and Try lines of `bonfire.WithHint` (`{bin}` becomes `bin/<name>` or `summer`), cobra's "did you mean" suggestions as Try lines, and "Run with -v for details." or, at `-v`, the wrapped error chain; every message passes through `sunscreen.Scrub`. Errors wrapped by `bonfire.Reported` print nothing. `bonfire.Fail` prints errors that happen before the root exists. For an application binary (any root other than `summer`) that fails, other than with a usage error, while the working directory has no `config/` directory and a parent directory holds `summer.yaml`, both add the hint "This command runs from the application root." with `cd <relative path>; bin/<name> <args>` (only the message changes).
|
||||
- Error block and exit codes: `bonfire.Execute` runs the root through fang and returns `bonfire.ExitOK`, `bonfire.ExitFailure`, or `bonfire.ExitUsage` for an unknown command or flag and a wrong number of arguments (`bonfire.UsageError`). Errors print on the error stream as a block (`bonfire.RenderError`): the ERROR badge and the message, the explanation and Try lines of `bonfire.WithHint` (`{bin}` becomes `bin/<name>` or `summer`), cobra's "did you mean" suggestions as Try lines, and "Run with -v for details." or, at `-v`, the wrapped error chain; every message passes through `sunscreen.Scrub` and every line through `bonfire.Sanitize`, which strips control characters. Errors wrapped by `bonfire.Reported` print nothing. An error carrying a code through `bonfire.WithExitCode` exits with that code; summer uses it to exit with the code of an app binary it ran. `bonfire.Fail` prints errors that happen before the root exists. For an application binary (any root other than `summer`) that fails, other than with a usage error, while the working directory has no `config/` directory and a parent directory holds `summer.yaml`, both add the hint "This command runs from the application root." with `cd <relative path>; bin/<name> <args>` (only the message changes); the values of flags named like a password, secret, token or key are masked as `***` in that line.
|
||||
- Name checks at boot: two commands with the same name, or a command named `list` or `help`, make `bonfire.NewRoot` fail with `bonfire.ErrDuplicateCommand`; a command flag that reuses a global option's name or shorthand fails with `bonfire.ErrReservedFlag` instead of panicking.
|
||||
- Injectable streams (`bonfire.NewRootIO`, `bonfire.NewOutput`) so commands can be tested against buffers.
|
||||
- In-process calls: `bonfire.Call` runs a named command with arguments against any writer (Laravel `Artisan::call`), and `bonfire.Catalog` holds an application binary's final command list so code outside the Cobra root, such as the conga scheduler, can call any registered command.
|
||||
@@ -139,7 +139,9 @@ if catalog.Has("blog:import") {
|
||||
| `bonfire.HintError` | An error with an `Explanation` and a `Try` text that the error block prints. |
|
||||
| `bonfire.WithHint` | Wraps an error in a `bonfire.HintError`; `{bin}` in the Try text becomes `bin/<name>` or `summer`. |
|
||||
| `bonfire.ErrReported` | Marks an error whose details the command already printed. |
|
||||
| `bonfire.Reported` | Wraps an error so it matches `bonfire.ErrReported`: exit code 1, no error block. |
|
||||
| `bonfire.Reported` | Wraps an error so it matches `bonfire.ErrReported`: exit code 1, or the code of `bonfire.WithExitCode`, and no error block. |
|
||||
| `bonfire.Sanitize` | Removes control characters other than tab (line breaks become spaces), so untrusted text cannot move the cursor or retitle the terminal; error blocks, process views, failure lines, tables and diary's console use it. |
|
||||
| `bonfire.WithExitCode` | Makes an error carry the exit code `bonfire.Execute` returns for it (1 to 255; other codes give 1). |
|
||||
| `bonfire.Listing` | The JSON catalog of `list --format=json`: `Application` (`bonfire.ApplicationInfo`), `GlobalOptions` and `Commands`. |
|
||||
| `bonfire.ApplicationInfo` | Name, version and framework version of a binary in a `bonfire.Listing`. |
|
||||
| `bonfire.CommandInfo` | One command of a `bonfire.Listing`: name, description, long help, usage, hidden and passthrough switches, `Arguments`, `Options` and `Examples`. |
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"unicode"
|
||||
|
||||
"git.golem15.com/golem15/summercms/modules/sunscreen"
|
||||
"github.com/charmbracelet/colorprofile"
|
||||
@@ -93,13 +94,77 @@ func Reported(err error) error {
|
||||
return &reportedError{err: err}
|
||||
}
|
||||
|
||||
// Sanitize removes control characters other than tab from s; carriage
|
||||
// returns and line feeds become spaces, so a multi-line value stays on one
|
||||
// line. Text from outside the operator's control (database or remote error
|
||||
// text, names an administrator typed) then cannot move the cursor, clear
|
||||
// the screen or set the window title. Error blocks, process views, failure
|
||||
// lines and tables pass their text through it, and so does diary's console
|
||||
// channel. A clean string is returned unchanged.
|
||||
func Sanitize(s string) string {
|
||||
clean := true
|
||||
for _, r := range s {
|
||||
if r != '\t' && unicode.IsControl(r) {
|
||||
clean = false
|
||||
break
|
||||
}
|
||||
}
|
||||
if clean {
|
||||
return s
|
||||
}
|
||||
var b strings.Builder
|
||||
for _, r := range s {
|
||||
switch {
|
||||
case r == '\n' || r == '\r':
|
||||
b.WriteByte(' ')
|
||||
case r != '\t' && unicode.IsControl(r):
|
||||
default:
|
||||
b.WriteRune(r)
|
||||
}
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
|
||||
// sanitizeLines applies Sanitize to every line of s, keeping the line
|
||||
// breaks.
|
||||
func sanitizeLines(s string) string {
|
||||
lines := strings.Split(s, "\n")
|
||||
for i, line := range lines {
|
||||
lines[i] = Sanitize(line)
|
||||
}
|
||||
return strings.Join(lines, "\n")
|
||||
}
|
||||
|
||||
// exitCodeError carries the exit code Execute returns for an error.
|
||||
type exitCodeError struct {
|
||||
err error
|
||||
code int
|
||||
}
|
||||
|
||||
func (e *exitCodeError) Error() string { return e.err.Error() }
|
||||
|
||||
func (e *exitCodeError) Unwrap() error { return e.err }
|
||||
|
||||
// WithExitCode returns err carrying code as the process exit code: Execute
|
||||
// returns code instead of ExitFailure or ExitUsage. A code outside 1 to 255
|
||||
// gives ExitFailure. A nil err stays nil. Wrap it with Reported when the
|
||||
// error was already printed, as summer does with the exit code of an app
|
||||
// binary it ran.
|
||||
func WithExitCode(err error, code int) error {
|
||||
if err == nil {
|
||||
return nil
|
||||
}
|
||||
return &exitCodeError{err: err, code: code}
|
||||
}
|
||||
|
||||
// RenderError writes err to w as the console error block: the ERROR badge
|
||||
// and the message, the explanation and the Try lines of a HintError ({bin}
|
||||
// replaced by summer or bin/<name>, for the root named name), then "Run with
|
||||
// -v for details." at verbosity 0 or the wrapped error chain under "Caused
|
||||
// by:" at -v and above. cobra's unknown command error becomes `Command "x"
|
||||
// is not defined.` with its suggestions as Try lines, and other usage errors
|
||||
// suggest the command's help. Every message passes through sunscreen.Scrub.
|
||||
// suggest the command's help. Every message passes through sunscreen.Scrub,
|
||||
// and every line through Sanitize.
|
||||
// Errors wrapped by Reported print nothing.
|
||||
func RenderError(w io.Writer, name string, err error, g Globals) {
|
||||
if err == nil || w == nil || errors.Is(err, ErrReported) {
|
||||
@@ -142,7 +207,7 @@ func RenderError(w io.Writer, name string, err error, g Globals) {
|
||||
|
||||
var b strings.Builder
|
||||
b.WriteString("\n")
|
||||
for i, line := range strings.Split(sunscreen.Scrub(strings.TrimRight(message, "\n")), "\n") {
|
||||
for i, line := range strings.Split(sanitizeLines(sunscreen.Scrub(strings.TrimRight(message, "\n"))), "\n") {
|
||||
if i == 0 {
|
||||
b.WriteString(" " + p.ErrorBold.Reverse(true).Render(" ERROR ") + " " + line + "\n")
|
||||
continue
|
||||
@@ -151,21 +216,21 @@ func RenderError(w io.Writer, name string, err error, g Globals) {
|
||||
}
|
||||
if explanation != "" {
|
||||
b.WriteString("\n")
|
||||
for _, line := range strings.Split(sunscreen.Scrub(explanation), "\n") {
|
||||
for _, line := range strings.Split(sanitizeLines(sunscreen.Scrub(explanation)), "\n") {
|
||||
b.WriteString(" " + line + "\n")
|
||||
}
|
||||
}
|
||||
if try != "" {
|
||||
b.WriteString("\n " + p.Header.Render("Try:") + "\n")
|
||||
for _, line := range strings.Split(strings.ReplaceAll(try, "{bin}", bin), "\n") {
|
||||
b.WriteString(" " + p.Success.Render(sunscreen.Scrub(line)) + "\n")
|
||||
b.WriteString(" " + p.Success.Render(Sanitize(sunscreen.Scrub(line))) + "\n")
|
||||
}
|
||||
}
|
||||
if g.Verbosity > 0 {
|
||||
if chain := causes(err, err.Error()); len(chain) > 0 {
|
||||
b.WriteString("\n " + p.Header.Render("Caused by:") + "\n")
|
||||
for _, c := range chain {
|
||||
b.WriteString(" " + p.Muted.Render("- "+sunscreen.Scrub(c)) + "\n")
|
||||
b.WriteString(" " + p.Muted.Render("- "+sanitizeLines(sunscreen.Scrub(c))) + "\n")
|
||||
}
|
||||
}
|
||||
} else {
|
||||
@@ -183,7 +248,8 @@ const wrongDirectoryExplanation = "This command runs from the application root."
|
||||
// name other than summer) failed while the working directory dir ("" means
|
||||
// os.Getwd) has no config directory and a parent directory holds
|
||||
// summer.yaml: compass then found no configuration, which is why the command
|
||||
// failed (D-19). The Try line is "cd <relative path>; bin/<name> <args>".
|
||||
// failed (D-19). The Try line is "cd <relative path>; bin/<name> <args>",
|
||||
// with the values of secret-looking flags masked (tryArgs).
|
||||
// Usage errors, Reported errors and summer are left alone; nothing else
|
||||
// about the failure changes.
|
||||
func wrongDirectory(name string, err error, args []string, dir string) error {
|
||||
@@ -217,13 +283,54 @@ func wrongDirectory(name string, err error, args []string, dir string) error {
|
||||
if rerr != nil {
|
||||
return err
|
||||
}
|
||||
try := "cd " + shellQuote(rel) + "; {bin}"
|
||||
for _, a := range args {
|
||||
try += " " + shellQuote(a)
|
||||
}
|
||||
try := "cd " + shellQuote(rel) + "; {bin}" + tryArgs(args)
|
||||
return &HintError{Err: err, Explanation: wrongDirectoryExplanation, Try: try}
|
||||
}
|
||||
|
||||
// secretWords mark a long flag whose value the Try line masks.
|
||||
var secretWords = []string{"password", "passwd", "secret", "token", "key"}
|
||||
|
||||
// secretFlag reports whether the long flag name (without "--") looks like it
|
||||
// carries a secret.
|
||||
func secretFlag(name string) bool {
|
||||
name = strings.ToLower(name)
|
||||
for _, w := range secretWords {
|
||||
if strings.Contains(name, w) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// tryArgs renders args for the wrong-directory Try line, each preceded by a
|
||||
// space and quoted for a shell. The value of a long flag named like a
|
||||
// secret becomes ***, in the "--flag=value" form and in the "--flag value"
|
||||
// form (the next argument, when it does not start with "-"), so a password
|
||||
// typed on the command line does not reach stderr and CI logs.
|
||||
func tryArgs(args []string) string {
|
||||
var b strings.Builder
|
||||
for i := 0; i < len(args); i++ {
|
||||
a := args[i]
|
||||
b.WriteByte(' ')
|
||||
name, long := strings.CutPrefix(a, "--")
|
||||
flag, _, inline := strings.Cut(name, "=")
|
||||
if !long || flag == "" || !secretFlag(flag) {
|
||||
b.WriteString(shellQuote(a))
|
||||
continue
|
||||
}
|
||||
if inline {
|
||||
b.WriteString(shellQuote("--"+flag) + "=***")
|
||||
continue
|
||||
}
|
||||
b.WriteString(shellQuote(a))
|
||||
if i+1 < len(args) && !strings.HasPrefix(args[i+1], "-") {
|
||||
b.WriteString(" ***")
|
||||
i++
|
||||
}
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
|
||||
// shellQuote single-quotes s when a shell would split or expand it.
|
||||
func shellQuote(s string) string {
|
||||
if s != "" && !strings.ContainsAny(s, " \t\n'\"\\$`*?[]{}()<>|&;#~!") {
|
||||
|
||||
@@ -182,3 +182,144 @@ func TestWrongDirectoryHint(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestWithExitCode (CR-02): Execute returns the code an error carries
|
||||
// through WithExitCode, prints no block when it is also Reported, maps a
|
||||
// code outside 1..255 to ExitFailure, and errors.Is and errors.As still see
|
||||
// the wrapped error.
|
||||
func TestWithExitCode(t *testing.T) {
|
||||
plainEnv(t)
|
||||
base := errors.New("child failed")
|
||||
command := func(err error) []Command {
|
||||
return []Command{{Name: "acme:run", Description: "Run", Run: func(context.Context, Input, Output) error { return err }}}
|
||||
}
|
||||
_, errOut, code := runGolden(t, command(Reported(WithExitCode(base, 2))), "acme:run")
|
||||
if code != 2 || errOut != "" {
|
||||
t.Errorf("Reported(WithExitCode(err, 2)): exit %d, stderr %q; want 2 and no block", code, errOut)
|
||||
}
|
||||
_, errOut, code = runGolden(t, command(WithExitCode(base, 3)), "acme:run")
|
||||
if code != 3 || !strings.Contains(errOut, "ERROR") || !strings.Contains(errOut, "child failed") {
|
||||
t.Errorf("WithExitCode(err, 3): exit %d, stderr %q; want 3 and the block", code, errOut)
|
||||
}
|
||||
for _, bad := range []int{0, -1, 256} {
|
||||
if _, _, code := runGolden(t, command(Reported(WithExitCode(base, bad))), "acme:run"); code != ExitFailure {
|
||||
t.Errorf("WithExitCode(err, %d): exit %d, want %d", bad, code, ExitFailure)
|
||||
}
|
||||
}
|
||||
if WithExitCode(nil, 2) != nil {
|
||||
t.Error("WithExitCode(nil, 2) is not nil")
|
||||
}
|
||||
wrapped := fmt.Errorf("outer: %w", WithExitCode(&HintError{Err: base, Try: "x"}, 4))
|
||||
var hint *HintError
|
||||
if !errors.Is(wrapped, base) || !errors.As(wrapped, &hint) || wrapped.Error() != "outer: child failed" {
|
||||
t.Errorf("WithExitCode hides the wrapped error: %v", wrapped)
|
||||
}
|
||||
if code := exitCode(wrapped); code != 4 {
|
||||
t.Errorf("exitCode through a wrapper = %d, want 4", code)
|
||||
}
|
||||
}
|
||||
|
||||
// TestWrongDirectoryMasksSecrets (WR-08): the wrong-directory Try line
|
||||
// replaces the value of every flag named like a password, secret, token or
|
||||
// key with ***, in both the "--flag value" and "--flag=value" forms, and
|
||||
// keeps every other argument.
|
||||
func TestWrongDirectoryMasksSecrets(t *testing.T) {
|
||||
plainEnv(t)
|
||||
root := t.TempDir()
|
||||
if err := os.WriteFile(filepath.Join(root, "summer.yaml"), []byte("binary: acme\n"), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
sub := filepath.Join(root, "bin")
|
||||
if err := os.MkdirAll(sub, 0o755); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
fail := errors.New("lagoon: database.dsn is empty")
|
||||
cases := []struct {
|
||||
args []string
|
||||
want string
|
||||
}{
|
||||
{[]string{"admin:create", "--email", "a@b.c", "--password", "hunter2"}, "cd ..; {bin} admin:create --email a@b.c --password ***"},
|
||||
{[]string{"admin:create", "--email", "a@b.c", "--password=hunter2"}, "cd ..; {bin} admin:create --email a@b.c --password=***"},
|
||||
{[]string{"sync", "--client-secret", "s3cr3t", "--verbose"}, "cd ..; {bin} sync --client-secret *** --verbose"},
|
||||
{[]string{"sync", "--api-token=t0k3n"}, "cd ..; {bin} sync --api-token=***"},
|
||||
{[]string{"sync", "--key", "k3y", "acme blog"}, "cd ..; {bin} sync --key *** 'acme blog'"},
|
||||
{[]string{"sync", "--PASSWD", "hunter2"}, "cd ..; {bin} sync --PASSWD ***"},
|
||||
{[]string{"sync", "--token", "--force"}, "cd ..; {bin} sync --token --force"},
|
||||
}
|
||||
for _, tc := range cases {
|
||||
got := wrongDirectory("acme", fail, tc.args, sub)
|
||||
var hint *HintError
|
||||
if !errors.As(got, &hint) {
|
||||
t.Fatalf("%q: no hint: %v", tc.args, got)
|
||||
}
|
||||
if hint.Try != tc.want {
|
||||
t.Errorf("%q: Try = %q, want %q", tc.args, hint.Try, tc.want)
|
||||
}
|
||||
for _, secret := range []string{"hunter2", "s3cr3t", "t0k3n", "k3y"} {
|
||||
if strings.Contains(hint.Try, secret) {
|
||||
t.Errorf("%q: Try line repeats %q: %q", tc.args, secret, hint.Try)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// injected are terminal control sequences carried by untrusted text: an
|
||||
// OSC window title and a CSI screen clear.
|
||||
var injected = []string{"\x1b]0;pwned\x07", "\x1b[2J", "\x07"}
|
||||
|
||||
// carriesInjected reports which injected sequence s still holds.
|
||||
func carriesInjected(s string) string {
|
||||
for _, bad := range injected {
|
||||
if strings.Contains(s, bad) {
|
||||
return bad
|
||||
}
|
||||
}
|
||||
return ""
|
||||
}
|
||||
|
||||
// TestRenderErrorStripsControl (WR-05): with color on, control sequences
|
||||
// in the message, explanation, Try lines and the -v chain never reach the
|
||||
// terminal; the visible text stays.
|
||||
func TestRenderErrorStripsControl(t *testing.T) {
|
||||
plainEnv(t)
|
||||
cause := errors.New("cause \x1b]0;pwned\x07 from the database\x1b[2J")
|
||||
err := &HintError{
|
||||
Err: fmt.Errorf("sync failed \x1b]0;pwned\x07 here: %w", cause),
|
||||
Explanation: "the remote said \x1b[2J no",
|
||||
Try: "{bin} sync --retry \x1b]0;pwned\x07",
|
||||
}
|
||||
var buf bytes.Buffer
|
||||
RenderError(&buf, "acme", err, Globals{Color: ColorAlways, Verbosity: 1})
|
||||
out := buf.String()
|
||||
if bad := carriesInjected(out); bad != "" {
|
||||
t.Errorf("error block carries %q:\n%q", bad, out)
|
||||
}
|
||||
if !strings.Contains(out, "\x1b[") {
|
||||
t.Errorf("color is off, the test does not exercise styled output:\n%q", out)
|
||||
}
|
||||
for _, want := range []string{"sync failed", "the remote said", "sync --retry", "from the database"} {
|
||||
if !strings.Contains(out, want) {
|
||||
t.Errorf("error block lacks %q:\n%q", want, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestSanitize (WR-05): tab stays, CR and LF become spaces, every other
|
||||
// control character (ESC, BEL, C0, DEL) is removed; clean text is returned
|
||||
// unchanged. The rules are those of diary's console channel.
|
||||
func TestSanitize(t *testing.T) {
|
||||
for in, want := range map[string]string{
|
||||
"": "",
|
||||
"plain text ✓": "plain text ✓",
|
||||
"a\tb": "a\tb",
|
||||
"line one\nline two\rthree": "line one line two three",
|
||||
"evil \x1b[31mred\x1b[0m\a bell": "evil [31mred[0m bell",
|
||||
"a\x1b]0;title\x07b": "a]0;titleb",
|
||||
"nul\x00 and del\x7f and \x01": "nul and del and ",
|
||||
"boom\x1b[2Jclear": "boom[2Jclear",
|
||||
} {
|
||||
if got := Sanitize(in); got != want {
|
||||
t.Errorf("Sanitize(%q) = %q, want %q", in, got, want)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,6 +2,7 @@ package bonfire
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"os"
|
||||
"slices"
|
||||
@@ -23,11 +24,11 @@ const (
|
||||
)
|
||||
|
||||
// Execute runs root with args and returns the process exit code: ExitOK,
|
||||
// ExitFailure, or ExitUsage for usage errors. With no command word in args
|
||||
// (only global options, -h or --help) it prints the command list. Errors are
|
||||
// rendered on the root's error stream by RenderError through fang's error
|
||||
// handler; fang is used without its version flag, man page and completion
|
||||
// commands.
|
||||
// ExitFailure, ExitUsage for usage errors, or the code an error carries
|
||||
// through WithExitCode. With no command word in args (only global options,
|
||||
// -h or --help) it prints the command list. Errors are rendered on the
|
||||
// root's error stream by RenderError through fang's error handler; fang is
|
||||
// used without its version flag, man page and completion commands.
|
||||
func Execute(ctx context.Context, root *cobra.Command, args []string) int {
|
||||
if ctx == nil {
|
||||
ctx = context.Background()
|
||||
@@ -73,9 +74,15 @@ func Fail(w io.Writer, name string, err error, g Globals) int {
|
||||
}
|
||||
|
||||
func exitCode(err error) int {
|
||||
var carrier *exitCodeError
|
||||
switch {
|
||||
case err == nil:
|
||||
return ExitOK
|
||||
case errors.As(err, &carrier):
|
||||
if carrier.code < 1 || carrier.code > 255 {
|
||||
return ExitFailure
|
||||
}
|
||||
return carrier.code
|
||||
case isUsage(err):
|
||||
return ExitUsage
|
||||
default:
|
||||
|
||||
@@ -149,7 +149,7 @@ func Process(ctx context.Context, out Output, operation string, items []string,
|
||||
}
|
||||
if c, ok := out.(*console); ok && c.live() && len(items) > 0 {
|
||||
var errs []error
|
||||
_ = runLive(ctx, c.out, c, newProcessModel(c.palette, operation, items), func(send func(tea.Msg)) error {
|
||||
started, _ := runLive(ctx, c.out, c, newProcessModel(c.palette, operation, items), func(send func(tea.Msg)) error {
|
||||
for i, item := range items {
|
||||
if err := ctx.Err(); err != nil {
|
||||
errs = append(errs, err)
|
||||
@@ -165,7 +165,9 @@ func Process(ctx context.Context, out Output, operation string, items []string,
|
||||
}
|
||||
return nil
|
||||
})
|
||||
return errors.Join(errs...)
|
||||
if started {
|
||||
return errors.Join(errs...)
|
||||
}
|
||||
}
|
||||
var errs []error
|
||||
for _, item := range items {
|
||||
@@ -175,7 +177,7 @@ func Process(ctx context.Context, out Output, operation string, items []string,
|
||||
}
|
||||
if err := fn(ctx, item); err != nil {
|
||||
errs = append(errs, failure(item, err))
|
||||
out.Error(failureLine(item, err))
|
||||
out.Error(failureText(item, err))
|
||||
continue
|
||||
}
|
||||
out.Success(item)
|
||||
|
||||
@@ -4,14 +4,20 @@ package bonfire
|
||||
// process.go and progress.go) to bubbletea v2, without SSU's git and
|
||||
// engine types. Each runs as a bubbletea program with input off (stdin
|
||||
// stays in cooked mode, so Ctrl+C reaches the command's own signal
|
||||
// handling), no signal handler of its own and the command's context.
|
||||
// handling), without bubbletea's signal handler and with the command's
|
||||
// context. While a view runs, runLive relays SIGINT and SIGTERM only to
|
||||
// restore the terminal before the signal takes its usual effect.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"os/signal"
|
||||
"strings"
|
||||
"sync"
|
||||
"syscall"
|
||||
|
||||
"charm.land/bubbles/v2/progress"
|
||||
"charm.land/bubbles/v2/spinner"
|
||||
@@ -46,10 +52,18 @@ type (
|
||||
}
|
||||
// finishMsg ends a live view; err is the result of the work.
|
||||
finishMsg struct{ err error }
|
||||
// printAboveMsg is a line to print above the view.
|
||||
printAboveMsg string
|
||||
)
|
||||
|
||||
// printLine is the message that prints s, one or more lines, above the
|
||||
// running view. It is bubbletea's own Println message, sent straight to
|
||||
// the program's queue: the event loop prints it itself, in the order the
|
||||
// messages arrive. Returning tea.Println as a command instead would run
|
||||
// it on a goroutine of its own, so lines would race each other and a line
|
||||
// still in flight when the view quits would be lost.
|
||||
func printLine(s string) tea.Msg {
|
||||
return tea.Println(s)()
|
||||
}
|
||||
|
||||
// itemState is the display state of one item in the process view.
|
||||
type itemState int
|
||||
|
||||
@@ -126,7 +140,7 @@ func (m spinnerModel) View() tea.View {
|
||||
line += fmt.Sprintf(" %d/%d", m.done, m.total)
|
||||
}
|
||||
if m.current != "" {
|
||||
line += " " + p.Muted.Render(m.current+"…")
|
||||
line += " " + p.Muted.Render(Sanitize(m.current)+"…")
|
||||
}
|
||||
if m.failed > 0 {
|
||||
line += " " + p.ErrorBold.Render(fmt.Sprintf("! %d failed", m.failed))
|
||||
@@ -179,7 +193,7 @@ func (m progressModel) View() tea.View {
|
||||
}
|
||||
line := " " + m.bar.ViewAs(pct) + m.palette.Muted.Render(fmt.Sprintf(" %d/%d", m.done, m.total))
|
||||
if m.current != "" && !m.finished {
|
||||
line += " " + m.palette.Muted.Render(m.current)
|
||||
line += " " + m.palette.Muted.Render(Sanitize(m.current))
|
||||
}
|
||||
if m.failed > 0 {
|
||||
line += " " + m.palette.ErrorBold.Render(fmt.Sprintf("! %d failed", m.failed))
|
||||
@@ -275,17 +289,18 @@ func (m processModel) View() tea.View {
|
||||
var b strings.Builder
|
||||
start, end := m.window()
|
||||
for _, it := range m.items[start:end] {
|
||||
name := Sanitize(it.name)
|
||||
switch it.state {
|
||||
case itemWaiting:
|
||||
b.WriteString(p.Muted.Render(" "+symWaiting) + " " + p.Muted.Render(it.name))
|
||||
b.WriteString(p.Muted.Render(" "+symWaiting) + " " + p.Muted.Render(name))
|
||||
case itemRunning:
|
||||
b.WriteString(" " + m.spin.View() + " " + it.name)
|
||||
b.WriteString(" " + m.spin.View() + " " + name)
|
||||
case itemDone:
|
||||
b.WriteString(p.Success.Render(" "+symCheck) + " " + it.name)
|
||||
b.WriteString(p.Success.Render(" "+symCheck) + " " + name)
|
||||
case itemFailed:
|
||||
b.WriteString(p.Error.Render(" "+symCross) + " " + it.name)
|
||||
b.WriteString(p.Error.Render(" "+symCross) + " " + name)
|
||||
if it.err != nil {
|
||||
b.WriteString(" " + p.Error.Render(it.err.Error()))
|
||||
b.WriteString(" " + p.Error.Render(Sanitize(it.err.Error())))
|
||||
}
|
||||
}
|
||||
b.WriteString("\n")
|
||||
@@ -306,82 +321,167 @@ func (m processModel) View() tea.View {
|
||||
return tea.NewView(b.String())
|
||||
}
|
||||
|
||||
// liveModel wraps a view so lines from PrintAbove print above it.
|
||||
type liveModel struct {
|
||||
inner tea.Model
|
||||
// liveView is the running live program and the console that started it.
|
||||
type liveView struct {
|
||||
owner *console
|
||||
prog *tea.Program
|
||||
}
|
||||
|
||||
func (m liveModel) Init() tea.Cmd { return m.inner.Init() }
|
||||
|
||||
func (m liveModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
|
||||
if line, ok := msg.(printAboveMsg); ok {
|
||||
return m, tea.Println(string(line))
|
||||
}
|
||||
inner, cmd := m.inner.Update(msg)
|
||||
m.inner = inner
|
||||
return m, cmd
|
||||
}
|
||||
|
||||
func (m liveModel) View() tea.View { return m.inner.View() }
|
||||
|
||||
// The running live program, for PrintAbove. Only one live view runs at a
|
||||
// time; widgets started while one runs print plain lines.
|
||||
// The running live view, for PrintAbove and the owning console's output.
|
||||
// Only one live view runs at a time; a widget that cannot claim the slot
|
||||
// (a nested widget, or a second console at the same moment) prints plain
|
||||
// lines.
|
||||
var (
|
||||
liveMu sync.Mutex
|
||||
activeProg *tea.Program
|
||||
activeView *liveView
|
||||
)
|
||||
|
||||
// liveActive reports whether a live view is running.
|
||||
func liveActive() bool {
|
||||
liveMu.Lock()
|
||||
defer liveMu.Unlock()
|
||||
return activeProg != nil
|
||||
}
|
||||
// errLivePanic is the result the final frame shows when work panicked.
|
||||
var errLivePanic = errors.New("bonfire: live view work panicked")
|
||||
|
||||
// runLive runs model as a bubbletea program on out while work runs on the
|
||||
// calling goroutine. work gets a send function for progress messages. When
|
||||
// work returns, the program is unregistered (so PrintAbove stops sending to
|
||||
// it), told to finish and waited for; it never receives lines after that.
|
||||
func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, work func(send func(tea.Msg)) error) error {
|
||||
// calling goroutine, with c as the owner of the view. It first claims the
|
||||
// single live slot under liveMu; when another view holds it, runLive
|
||||
// returns started false without running work, and the caller prints plain
|
||||
// lines instead. work gets a send function for progress messages. When
|
||||
// work returns or panics, the view is unregistered (so PrintAbove stops
|
||||
// sending to it), told to finish and waited for, all under liveMu; it
|
||||
// never receives lines after that, and a panic goes on after the cleanup.
|
||||
func runLive(ctx context.Context, out io.Writer, c *console, model tea.Model, work func(send func(tea.Msg)) error) (started bool, err error) {
|
||||
if ctx == nil {
|
||||
ctx = context.Background()
|
||||
}
|
||||
prog := tea.NewProgram(liveModel{inner: model},
|
||||
prog := tea.NewProgram(model,
|
||||
tea.WithInput(nil),
|
||||
tea.WithOutput(out),
|
||||
tea.WithoutSignalHandler(),
|
||||
tea.WithContext(ctx),
|
||||
tea.WithColorProfile(c.profile()),
|
||||
)
|
||||
view := &liveView{owner: c, prog: prog}
|
||||
liveMu.Lock()
|
||||
if activeView != nil {
|
||||
liveMu.Unlock()
|
||||
return false, nil
|
||||
}
|
||||
activeView = view
|
||||
liveMu.Unlock()
|
||||
|
||||
finished := make(chan struct{})
|
||||
go func() {
|
||||
defer close(finished)
|
||||
_, _ = prog.Run()
|
||||
}()
|
||||
liveMu.Lock()
|
||||
activeProg = prog
|
||||
liveMu.Unlock()
|
||||
stopSignals := watchSignals(view, finished)
|
||||
|
||||
err := work(prog.Send)
|
||||
|
||||
liveMu.Lock()
|
||||
activeProg = nil
|
||||
liveMu.Unlock()
|
||||
prog.Send(finishMsg{err: err})
|
||||
<-finished
|
||||
return err
|
||||
completed := false
|
||||
defer func() {
|
||||
result := err
|
||||
if !completed {
|
||||
result = errLivePanic
|
||||
}
|
||||
// Unregister, finish and wait as one step: a printer that arrives
|
||||
// meanwhile waits for the lock and then writes after the final
|
||||
// frame, so no line reaches a stopped program or races the last
|
||||
// frame.
|
||||
liveMu.Lock()
|
||||
if activeView == view {
|
||||
activeView = nil
|
||||
}
|
||||
prog.Send(finishMsg{err: result})
|
||||
<-finished
|
||||
liveMu.Unlock()
|
||||
stopSignals()
|
||||
}()
|
||||
err = work(prog.Send)
|
||||
completed = true
|
||||
return true, err
|
||||
}
|
||||
|
||||
// printAboveLines prints s, one or more lines, above the running live view.
|
||||
// It reports false when no view runs.
|
||||
// watchSignals relays SIGINT and SIGTERM (those not ignored) while view
|
||||
// runs. The views run without bubbletea's own signal handling, so without
|
||||
// this a Ctrl+C would end the process with the cursor still hidden. On a
|
||||
// signal the program is killed (bubbletea then restores the cursor and the
|
||||
// terminal), the view is unregistered and the signal is raised again on
|
||||
// the process with the relay stopped, so its default action, or the
|
||||
// command's own handler, still applies. The returned function stops the
|
||||
// relay when the view ends normally; a signal that arrived meanwhile is
|
||||
// raised again too.
|
||||
func watchSignals(view *liveView, finished <-chan struct{}) (stop func()) {
|
||||
var relay []os.Signal
|
||||
for _, sig := range []os.Signal{os.Interrupt, syscall.SIGTERM} {
|
||||
if !signal.Ignored(sig) {
|
||||
relay = append(relay, sig)
|
||||
}
|
||||
}
|
||||
if len(relay) == 0 {
|
||||
return func() {}
|
||||
}
|
||||
sigs := make(chan os.Signal, 1)
|
||||
signal.Notify(sigs, relay...)
|
||||
quit := make(chan struct{})
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
defer close(done)
|
||||
select {
|
||||
case sig := <-sigs:
|
||||
view.prog.Kill()
|
||||
<-finished
|
||||
liveMu.Lock()
|
||||
if activeView == view {
|
||||
activeView = nil
|
||||
}
|
||||
liveMu.Unlock()
|
||||
signal.Stop(sigs)
|
||||
raise(sig)
|
||||
case <-quit:
|
||||
}
|
||||
}()
|
||||
return func() {
|
||||
close(quit)
|
||||
<-done
|
||||
signal.Stop(sigs)
|
||||
select {
|
||||
case sig := <-sigs:
|
||||
raise(sig)
|
||||
default:
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// raise sends sig to the own process; when that fails, it exits with 128
|
||||
// plus the signal number, as a shell reports a death by that signal.
|
||||
func raise(sig os.Signal) {
|
||||
p, err := os.FindProcess(os.Getpid())
|
||||
if err == nil {
|
||||
err = p.Signal(sig)
|
||||
}
|
||||
if err != nil {
|
||||
code := 1
|
||||
if s, ok := sig.(syscall.Signal); ok {
|
||||
code = 128 + int(s)
|
||||
}
|
||||
os.Exit(code)
|
||||
}
|
||||
}
|
||||
|
||||
// printAboveLines prints s, one or more lines, above the running live view,
|
||||
// whichever console started it (bonfire.PrintAbove). It reports false when
|
||||
// no view runs.
|
||||
func printAboveLines(s string) bool {
|
||||
return printAbove(nil, s)
|
||||
}
|
||||
|
||||
// printAbove prints s above the running live view when owner is nil or is
|
||||
// the console that started the view, and reports whether it did.
|
||||
func printAbove(owner *console, s string) bool {
|
||||
liveMu.Lock()
|
||||
defer liveMu.Unlock()
|
||||
if activeProg == nil {
|
||||
if activeView == nil || (owner != nil && activeView.owner != owner) {
|
||||
return false
|
||||
}
|
||||
for _, line := range strings.Split(strings.TrimSuffix(s, "\n"), "\n") {
|
||||
activeProg.Send(printAboveMsg(line))
|
||||
}
|
||||
// One message for the whole block, sent while holding liveMu: blocks
|
||||
// from different goroutines keep the order of their calls.
|
||||
activeView.prog.Send(printLine(strings.TrimSuffix(s, "\n")))
|
||||
return true
|
||||
}
|
||||
|
||||
108
modules/bonfire/live_signal_unix_test.go
Normal file
108
modules/bonfire/live_signal_unix_test.go
Normal file
@@ -0,0 +1,108 @@
|
||||
//go:build unix
|
||||
|
||||
package bonfire
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"os/exec"
|
||||
"os/signal"
|
||||
"strings"
|
||||
"syscall"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
|
||||
const (
|
||||
hideCursor = "\x1b[?25l"
|
||||
showCursor = "\x1b[?25h"
|
||||
)
|
||||
|
||||
// TestLiveSignalHelper is the helper process of
|
||||
// TestLiveViewRestoresTerminalOnSignal: with BONFIRE_LIVE_SIGNAL_HELPER set
|
||||
// it runs a spinner on a terminal console over its stdout, prints ready on
|
||||
// stderr and waits. In own_handler mode it also holds its own signal
|
||||
// context and exits 0 once that context ends.
|
||||
func TestLiveSignalHelper(t *testing.T) {
|
||||
mode := os.Getenv("BONFIRE_LIVE_SIGNAL_HELPER")
|
||||
if mode == "" {
|
||||
return
|
||||
}
|
||||
ctx := context.Background()
|
||||
if mode == "own_handler" {
|
||||
var stop context.CancelFunc
|
||||
ctx, stop = signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM)
|
||||
defer stop()
|
||||
}
|
||||
c := newConsole(strings.NewReader(""), os.Stdout, os.Stderr, noEnv, boolPtr(true), boolPtr(true))
|
||||
_ = c.Spinner("waiting for a signal", func() error {
|
||||
fmt.Fprintln(os.Stderr, "ready")
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
case <-time.After(20 * time.Second):
|
||||
}
|
||||
return nil
|
||||
})
|
||||
if mode == "own_handler" && ctx.Err() != nil {
|
||||
os.Exit(0)
|
||||
}
|
||||
os.Exit(3)
|
||||
}
|
||||
|
||||
// TestLiveViewRestoresTerminalOnSignal (WR-01): SIGINT during a live view
|
||||
// shows the cursor again and then still ends the process by the signal,
|
||||
// or reaches the command's own signal handler when it has one.
|
||||
func TestLiveViewRestoresTerminalOnSignal(t *testing.T) {
|
||||
for _, mode := range []string{"default", "own_handler"} {
|
||||
t.Run(mode, func(t *testing.T) {
|
||||
var stdout, stderr lockedBuffer
|
||||
cmd := exec.Command(os.Args[0], "-test.run=^TestLiveSignalHelper$")
|
||||
cmd.Env = append(os.Environ(), "BONFIRE_LIVE_SIGNAL_HELPER="+mode)
|
||||
cmd.Stdout, cmd.Stderr = &stdout, &stderr
|
||||
if err := cmd.Start(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
deadline := time.Now().Add(10 * time.Second)
|
||||
for !strings.Contains(stderr.String(), "ready") || !strings.Contains(stdout.String(), hideCursor) {
|
||||
if time.Now().After(deadline) {
|
||||
_ = cmd.Process.Kill()
|
||||
t.Fatalf("helper not ready; stdout %q, stderr %q", stdout.String(), stderr.String())
|
||||
}
|
||||
time.Sleep(10 * time.Millisecond)
|
||||
}
|
||||
if err := cmd.Process.Signal(os.Interrupt); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
done := make(chan error, 1)
|
||||
go func() { done <- cmd.Wait() }()
|
||||
var err error
|
||||
select {
|
||||
case err = <-done:
|
||||
case <-time.After(10 * time.Second):
|
||||
_ = cmd.Process.Kill()
|
||||
t.Fatalf("helper did not end after SIGINT; stdout %q", stdout.String())
|
||||
}
|
||||
out := stdout.String()
|
||||
if i := strings.LastIndex(out, hideCursor); !strings.Contains(out[i:], showCursor) {
|
||||
t.Errorf("cursor left hidden: no show-cursor after the last hide-cursor in %q", out)
|
||||
}
|
||||
switch mode {
|
||||
case "default":
|
||||
var ee *exec.ExitError
|
||||
if !errors.As(err, &ee) {
|
||||
t.Fatalf("helper exited cleanly (%v), want death by SIGINT", err)
|
||||
}
|
||||
ws, ok := ee.Sys().(syscall.WaitStatus)
|
||||
if !ok || !ws.Signaled() || ws.Signal() != syscall.SIGINT {
|
||||
t.Errorf("helper ended with %v, want death by SIGINT", err)
|
||||
}
|
||||
case "own_handler":
|
||||
if err != nil {
|
||||
t.Errorf("helper with its own handler ended with %v, want exit 0; stderr %q", err, stderr.String())
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -2,9 +2,13 @@ package bonfire
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"regexp"
|
||||
"strings"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
@@ -206,21 +210,20 @@ func TestLiveModels(t *testing.T) {
|
||||
})
|
||||
|
||||
t.Run("print above", func(t *testing.T) {
|
||||
var m tea.Model = liveModel{inner: newSpinnerModel(p, "x")}
|
||||
before := frame(m)
|
||||
m, cmd := m.Update(printAboveMsg("a log line"))
|
||||
if cmd == nil || frame(m) != before {
|
||||
t.Error("printAboveMsg did not become a Println command or reached the inner model")
|
||||
// A printed block is one message for the program's queue, not a
|
||||
// command bubbletea would run on a goroutine of its own.
|
||||
msg := printLine("first\nsecond")
|
||||
if msg == nil {
|
||||
t.Fatal("printLine returned no message")
|
||||
}
|
||||
m, _ = m.Update(itemMsg{done: 1, total: 2})
|
||||
if !strings.Contains(frame(m), "1/2") {
|
||||
t.Errorf("other messages do not reach the inner model: %q", frame(m))
|
||||
if _, isCmd := msg.(tea.Cmd); isCmd {
|
||||
t.Error("printLine returned a command")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("program end to end", func(t *testing.T) {
|
||||
var buf bytes.Buffer
|
||||
prog := tea.NewProgram(liveModel{inner: newSpinnerModel(p, "Importing posts")},
|
||||
prog := tea.NewProgram(newSpinnerModel(p, "Importing posts"),
|
||||
tea.WithInput(nil),
|
||||
tea.WithOutput(&buf),
|
||||
tea.WithoutSignalHandler(),
|
||||
@@ -233,7 +236,7 @@ func TestLiveModels(t *testing.T) {
|
||||
done <- err
|
||||
}()
|
||||
prog.Send(itemMsg{done: 1, total: 2, item: "first"})
|
||||
prog.Send(printAboveMsg("imported first"))
|
||||
prog.Send(printLine("imported first"))
|
||||
prog.Send(itemMsg{done: 2, total: 2, item: "second"})
|
||||
prog.Send(finishMsg{})
|
||||
select {
|
||||
@@ -253,3 +256,296 @@ func TestLiveModels(t *testing.T) {
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// lockedBuffer is a bytes.Buffer safe for a live program and other
|
||||
// goroutines writing at the same time.
|
||||
type lockedBuffer struct {
|
||||
mu sync.Mutex
|
||||
buf bytes.Buffer
|
||||
}
|
||||
|
||||
func (b *lockedBuffer) Write(p []byte) (int, error) {
|
||||
b.mu.Lock()
|
||||
defer b.mu.Unlock()
|
||||
return b.buf.Write(p)
|
||||
}
|
||||
|
||||
func (b *lockedBuffer) String() string {
|
||||
b.mu.Lock()
|
||||
defer b.mu.Unlock()
|
||||
return b.buf.String()
|
||||
}
|
||||
|
||||
var numberedLine = regexp.MustCompile(`(line|bg)-\d{3}`)
|
||||
|
||||
// numbered returns the numbered lines of the ANSI-stripped output in the
|
||||
// order they appear.
|
||||
func numbered(out string) []string {
|
||||
return numberedLine.FindAllString(ansi.Strip(out), -1)
|
||||
}
|
||||
|
||||
// printNumbered prints line-<from> .. line-<to-1> through the console's
|
||||
// print paths in turn: Println, Printf, Info and bonfire.PrintAbove.
|
||||
func printNumbered(c *console, from, to int) {
|
||||
for i := from; i < to; i++ {
|
||||
line := fmt.Sprintf("line-%03d", i)
|
||||
switch i % 4 {
|
||||
case 0:
|
||||
c.Println(line)
|
||||
case 1:
|
||||
c.Printf("%s\n", line)
|
||||
case 2:
|
||||
c.Info(line)
|
||||
case 3:
|
||||
if !PrintAbove(line) {
|
||||
c.Println(line)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// checkNumbered fails unless out holds line-000 .. line-<n-1> exactly once
|
||||
// and in order.
|
||||
func checkNumbered(t *testing.T, run int, out string, n int) {
|
||||
t.Helper()
|
||||
got := numbered(out)
|
||||
for i := 0; i < n; i++ {
|
||||
want := fmt.Sprintf("line-%03d", i)
|
||||
if i >= len(got) {
|
||||
t.Fatalf("run %d: %d of %d lines printed, %s missing:\n%s", run, len(got), n, want, ansi.Strip(out))
|
||||
}
|
||||
if got[i] != want {
|
||||
t.Fatalf("run %d: line %d is %s, want %s (lines: %v)", run, i, got[i], want, got)
|
||||
}
|
||||
}
|
||||
if len(got) != n {
|
||||
t.Fatalf("run %d: %d lines printed, want %d: %v", run, len(got), n, got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestPrintAboveOrderedNoLoss (CR-01): every line printed while a spinner,
|
||||
// progress or process view runs appears once and in order; a line printed
|
||||
// from another goroutine while the view finishes is not lost.
|
||||
func TestPrintAboveOrderedNoLoss(t *testing.T) {
|
||||
const runs, lines = 20, 100
|
||||
newTTY := func(out io.Writer) *console {
|
||||
return newConsole(strings.NewReader(""), out, io.Discard, noEnv, boolPtr(true), boolPtr(true))
|
||||
}
|
||||
|
||||
t.Run("spinner", func(t *testing.T) {
|
||||
for run := range runs {
|
||||
out := &lockedBuffer{}
|
||||
c := newTTY(out)
|
||||
if err := c.Spinner("working", func() error {
|
||||
printNumbered(c, 0, lines)
|
||||
return nil
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
checkNumbered(t, run, out.String(), lines)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("progress", func(t *testing.T) {
|
||||
for run := range runs {
|
||||
out := &lockedBuffer{}
|
||||
c := newTTY(out)
|
||||
if err := c.Progress(lines, func(p Progress) error {
|
||||
printNumbered(c, 0, 40)
|
||||
Failed(p, "item-a", errors.New("first failure"))
|
||||
printNumbered(c, 40, 80)
|
||||
Failed(p, "item-b", errors.New("second failure"))
|
||||
printNumbered(c, 80, lines)
|
||||
return nil
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
got := out.String()
|
||||
checkNumbered(t, run, got, lines)
|
||||
for _, want := range []string{"item-a: first failure", "item-b: second failure"} {
|
||||
if !strings.Contains(ansi.Strip(got), want) {
|
||||
t.Fatalf("run %d: failure line %q missing:\n%s", run, want, ansi.Strip(got))
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("process", func(t *testing.T) {
|
||||
items := []string{"a", "b", "c", "d"}
|
||||
for run := range runs {
|
||||
out := &lockedBuffer{}
|
||||
c := newTTY(out)
|
||||
next := 0
|
||||
err := Process(context.Background(), c, "Pushing", items, func(context.Context, string) error {
|
||||
printNumbered(c, next, next+lines/len(items))
|
||||
next += lines / len(items)
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
checkNumbered(t, run, out.String(), lines)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("concurrent_finish", func(t *testing.T) {
|
||||
const bg = 200
|
||||
for run := range runs {
|
||||
out := &lockedBuffer{}
|
||||
c := newTTY(out)
|
||||
started := make(chan struct{})
|
||||
done := make(chan struct{})
|
||||
if err := c.Spinner("working", func() error {
|
||||
go func() {
|
||||
defer close(done)
|
||||
for i := range bg {
|
||||
line := fmt.Sprintf("bg-%03d", i)
|
||||
if !PrintAbove(line) {
|
||||
_, _ = io.WriteString(out, line+"\n")
|
||||
}
|
||||
if i == 5 {
|
||||
close(started)
|
||||
}
|
||||
}
|
||||
}()
|
||||
<-started
|
||||
return nil
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
<-done
|
||||
count := map[string]int{}
|
||||
for _, l := range numbered(out.String()) {
|
||||
count[l]++
|
||||
}
|
||||
for i := range bg {
|
||||
l := fmt.Sprintf("bg-%03d", i)
|
||||
if count[l] != 1 {
|
||||
t.Fatalf("run %d: %s printed %d times:\n%s", run, l, count[l], ansi.Strip(out.String()))
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// TestPrintlnStaysWithItsConsole (WR-02): while a view runs on console A,
|
||||
// another console's Println, Printf and Info go to its own writer; A's own
|
||||
// Println still prints above its view.
|
||||
func TestPrintlnStaysWithItsConsole(t *testing.T) {
|
||||
bufA, bufB := &lockedBuffer{}, &lockedBuffer{}
|
||||
a := newConsole(strings.NewReader(""), bufA, io.Discard, noEnv, boolPtr(true), boolPtr(true))
|
||||
b := newConsole(strings.NewReader(""), bufB, bufB, noEnv, boolPtr(false), boolPtr(false))
|
||||
err := a.Spinner("working", func() error {
|
||||
b.Println("b-println")
|
||||
b.Printf("b-printf %d\n", 1)
|
||||
b.Info("b-info")
|
||||
a.Println("a-println")
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
gotA, gotB := ansi.Strip(bufA.String()), ansi.Strip(bufB.String())
|
||||
for _, want := range []string{"b-println", "b-printf 1", "b-info"} {
|
||||
if !strings.Contains(gotB, want) {
|
||||
t.Errorf("console B's writer lacks %q:\n%s", want, gotB)
|
||||
}
|
||||
if strings.Contains(gotA, want) {
|
||||
t.Errorf("console B's %q printed above console A's view:\n%s", want, gotA)
|
||||
}
|
||||
}
|
||||
if !strings.Contains(gotA, "a-println") {
|
||||
t.Errorf("console A's own line missing above its view:\n%s", gotA)
|
||||
}
|
||||
}
|
||||
|
||||
// TestLiveViewSingleOwner (WR-02): two consoles starting a spinner at the
|
||||
// same moment never both run a live view; the one that loses prints its
|
||||
// plain line to its own writer.
|
||||
func TestLiveViewSingleOwner(t *testing.T) {
|
||||
const rounds = 200
|
||||
for round := range rounds {
|
||||
bufs := [2]*lockedBuffer{{}, {}}
|
||||
var start, entered sync.WaitGroup
|
||||
start.Add(1)
|
||||
entered.Add(2)
|
||||
var wg sync.WaitGroup
|
||||
for i := range 2 {
|
||||
wg.Add(1)
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
c := newConsole(strings.NewReader(""), bufs[i], io.Discard, noEnv, boolPtr(true), boolPtr(true))
|
||||
start.Wait()
|
||||
_ = c.Spinner(fmt.Sprintf("label-%d", i), func() error {
|
||||
entered.Done()
|
||||
entered.Wait() // both work functions run at once
|
||||
return nil
|
||||
})
|
||||
}()
|
||||
}
|
||||
start.Done()
|
||||
wg.Wait()
|
||||
plain := 0
|
||||
for i, b := range bufs {
|
||||
if strings.Contains(ansi.Strip(b.String()), fmt.Sprintf("[...] label-%d", i)) {
|
||||
plain++
|
||||
}
|
||||
}
|
||||
if plain != 1 {
|
||||
t.Fatalf("round %d: %d consoles printed their plain line to their own writer, want 1:\nA: %q\nB: %q",
|
||||
round, plain, ansi.Strip(bufs[0].String()), ansi.Strip(bufs[1].String()))
|
||||
}
|
||||
if PrintAbove("after the round") {
|
||||
t.Fatalf("round %d: a view is still registered", round)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestLiveViewPanicUnregisters (WR-02): a panic in the work function still
|
||||
// ends and unregisters the view; the next widget runs as a live view.
|
||||
func TestLiveViewPanicUnregisters(t *testing.T) {
|
||||
out := &lockedBuffer{}
|
||||
c := newConsole(strings.NewReader(""), out, io.Discard, noEnv, boolPtr(true), boolPtr(true))
|
||||
func() {
|
||||
defer func() {
|
||||
if r := recover(); r != "boom" {
|
||||
t.Fatalf("recovered %v, want the work function's panic", r)
|
||||
}
|
||||
}()
|
||||
_ = c.Spinner("panicking", func() error { panic("boom") })
|
||||
}()
|
||||
if PrintAbove("after the panic") {
|
||||
t.Fatal("the panicked view is still registered")
|
||||
}
|
||||
next := &lockedBuffer{}
|
||||
c2 := newConsole(strings.NewReader(""), next, io.Discard, noEnv, boolPtr(true), boolPtr(true))
|
||||
var during bool
|
||||
if err := c2.Spinner("next", func() error {
|
||||
during = PrintAbove("above the next view")
|
||||
return nil
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !during || strings.Contains(ansi.Strip(next.String()), "[...] next") {
|
||||
t.Errorf("the next spinner did not run as a live view:\n%s", ansi.Strip(next.String()))
|
||||
}
|
||||
}
|
||||
|
||||
// TestProcessViewStripsControl (WR-05): an item name and an item error
|
||||
// carrying control sequences render without them.
|
||||
func TestProcessViewStripsControl(t *testing.T) {
|
||||
name := "evil\x1b]0;pwned\x07item"
|
||||
var m tea.Model = newProcessModel(DefaultPalette(), "Pushing", []string{name, "next"})
|
||||
m, _ = m.Update(tea.WindowSizeMsg{Width: 120, Height: 24})
|
||||
m, _ = m.Update(stepMsg{item: name, state: itemFailed, err: errors.New("rejected \x1b[2J by the remote"), done: 1})
|
||||
m, _ = m.Update(stepMsg{item: "next", state: itemRunning, done: 1})
|
||||
out := m.View().Content
|
||||
if bad := carriesInjected(out); bad != "" {
|
||||
t.Errorf("process view carries %q:\n%q", bad, out)
|
||||
}
|
||||
for _, want := range []string{"evil", "item", "rejected", "by the remote"} {
|
||||
if !strings.Contains(ansi.Strip(out), want) {
|
||||
t.Errorf("process view lacks %q:\n%q", want, out)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -128,10 +128,11 @@ func (c *console) setGlobals(g Globals) {
|
||||
|
||||
// live reports whether widgets animate: stdout is a terminal and none of
|
||||
// -q, --no-ansi and -n is set (D-03). A live view that is already running
|
||||
// makes nested widgets print plain lines.
|
||||
// makes nested widgets print plain lines: runLive then reports that it did
|
||||
// not start.
|
||||
func (c *console) live() bool {
|
||||
g := c.globals
|
||||
return c.tty && !g.Quiet && g.Color != ColorNever && !g.NoInteraction && !liveActive()
|
||||
return c.tty && !g.Quiet && g.Color != ColorNever && !g.NoInteraction
|
||||
}
|
||||
|
||||
// canPrompt reports whether prompts may wait for input from a person:
|
||||
@@ -170,13 +171,14 @@ func (c *console) styled(w io.Writer) io.Writer {
|
||||
return &colorprofile.Writer{Forward: w, Profile: c.profile()}
|
||||
}
|
||||
|
||||
// emit writes s, a styled line or block, to w. While a live view runs on
|
||||
// stdout, lines for stdout print above it instead of tearing it.
|
||||
// emit writes s, a styled line or block, to w. While a live view this
|
||||
// console started runs on stdout, lines for stdout print above it instead
|
||||
// of tearing it.
|
||||
func (c *console) emit(w io.Writer, s string) {
|
||||
if w == nil {
|
||||
return
|
||||
}
|
||||
if w == c.out && printAboveLines(s) {
|
||||
if w == c.out && printAbove(c, s) {
|
||||
return
|
||||
}
|
||||
_, _ = io.WriteString(c.styled(w), s)
|
||||
@@ -194,7 +196,7 @@ func (c *console) Println(a ...any) {
|
||||
return
|
||||
}
|
||||
s := fmt.Sprintln(a...)
|
||||
if printAboveLines(s) {
|
||||
if printAbove(c, s) {
|
||||
return
|
||||
}
|
||||
_, _ = io.WriteString(c.out, s)
|
||||
@@ -205,7 +207,7 @@ func (c *console) Printf(format string, a ...any) {
|
||||
return
|
||||
}
|
||||
s := fmt.Sprintf(format, a...)
|
||||
if strings.HasSuffix(s, "\n") && printAboveLines(s) {
|
||||
if strings.HasSuffix(s, "\n") && printAbove(c, s) {
|
||||
return
|
||||
}
|
||||
_, _ = io.WriteString(c.out, s)
|
||||
|
||||
@@ -26,13 +26,16 @@ func (c *console) Spinner(message string, fn func() error) error {
|
||||
if c.globals.Quiet {
|
||||
return fn()
|
||||
}
|
||||
if !c.live() {
|
||||
c.Printf("[...] %s\n", message)
|
||||
return fn()
|
||||
if c.live() {
|
||||
started, err := runLive(context.Background(), c.out, c, newSpinnerModel(c.palette, message), func(func(tea.Msg)) error {
|
||||
return fn()
|
||||
})
|
||||
if started {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return runLive(context.Background(), c.out, c, newSpinnerModel(c.palette, message), func(func(tea.Msg)) error {
|
||||
return fn()
|
||||
})
|
||||
c.Printf("[...] %s\n", message)
|
||||
return fn()
|
||||
}
|
||||
|
||||
func (c *console) Progress(total int, fn func(Progress) error) error {
|
||||
@@ -45,12 +48,15 @@ func (c *console) Progress(total int, fn func(Progress) error) error {
|
||||
if c.globals.Quiet {
|
||||
return fn(&quietProgress{c: c})
|
||||
}
|
||||
if !c.live() {
|
||||
return fn(&progressBar{c: c, total: total, lastPct: -1})
|
||||
if c.live() {
|
||||
started, err := runLive(context.Background(), c.out, c, newProgressModel(c.palette, total), func(send func(tea.Msg)) error {
|
||||
return fn(&liveProgress{send: send, total: total})
|
||||
})
|
||||
if started {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return runLive(context.Background(), c.out, c, newProgressModel(c.palette, total), func(send func(tea.Msg)) error {
|
||||
return fn(&liveProgress{send: send, total: total})
|
||||
})
|
||||
return fn(&progressBar{c: c, total: total, lastPct: -1})
|
||||
}
|
||||
|
||||
// progressBar prints a line at every tenth of the work when widgets do not
|
||||
@@ -90,7 +96,7 @@ func (p *progressBar) Advance(step int) {
|
||||
func (p *progressBar) Current(string) {}
|
||||
|
||||
func (p *progressBar) Fail(item string, err error) {
|
||||
p.c.Error(failureLine(item, err))
|
||||
p.c.Error(failureText(item, err))
|
||||
}
|
||||
|
||||
// quietProgress prints nothing but failures (-q).
|
||||
@@ -103,7 +109,7 @@ func (p *quietProgress) Advance(int) {}
|
||||
func (p *quietProgress) Current(string) {}
|
||||
|
||||
func (p *quietProgress) Fail(item string, err error) {
|
||||
p.c.Error(failureLine(item, err))
|
||||
p.c.Error(failureText(item, err))
|
||||
}
|
||||
|
||||
// liveProgress drives the live progress model.
|
||||
@@ -129,7 +135,7 @@ func (p *liveProgress) Current(item string) {
|
||||
|
||||
func (p *liveProgress) Fail(item string, err error) {
|
||||
p.send(itemMsg{done: p.current, total: p.total, item: item, err: err})
|
||||
p.send(printAboveMsg(failureLine(item, err)))
|
||||
p.send(printLine(failureText(item, err)))
|
||||
}
|
||||
|
||||
func failureLine(item string, err error) string {
|
||||
@@ -139,7 +145,14 @@ func failureLine(item string, err error) string {
|
||||
return item + ": " + err.Error()
|
||||
}
|
||||
|
||||
// failureText is the failure line as printed: one line without control
|
||||
// characters.
|
||||
func failureText(item string, err error) string {
|
||||
return Sanitize(failureLine(item, err))
|
||||
}
|
||||
|
||||
func (c *console) Table(headers []string, rows [][]string) {
|
||||
headers = padRow(headers, len(headers))
|
||||
if !c.tty {
|
||||
c.Println(strings.Join(headers, "\t"))
|
||||
for _, row := range rows {
|
||||
@@ -165,11 +178,13 @@ func (c *console) Table(headers []string, rows [][]string) {
|
||||
c.emit(c.out, t.String()+"\n")
|
||||
}
|
||||
|
||||
// padRow returns n cells of row (missing cells empty), each passed through
|
||||
// Sanitize so a cell cannot move the cursor or retitle the terminal.
|
||||
func padRow(row []string, n int) []string {
|
||||
out := make([]string, n)
|
||||
for i := 0; i < n; i++ {
|
||||
if i < len(row) {
|
||||
out[i] = row[i]
|
||||
out[i] = Sanitize(row[i])
|
||||
}
|
||||
}
|
||||
return out
|
||||
|
||||
@@ -295,3 +295,26 @@ func TestPrintAbove(t *testing.T) {
|
||||
t.Errorf("the spinner did not run as a live view:\n%s", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestTableStripsControl (WR-05): table headers and cells carrying control
|
||||
// sequences print without them, boxed with color on a terminal and as
|
||||
// tab-separated lines off one.
|
||||
func TestTableStripsControl(t *testing.T) {
|
||||
headers := []string{"Name\x1b]0;pwned\x07", "Scope"}
|
||||
rows := [][]string{{"token \x1b[2J one", "journal\x07.posts"}}
|
||||
for _, tty := range []bool{true, false} {
|
||||
var out bytes.Buffer
|
||||
c := newConsole(strings.NewReader(""), &out, &out, noEnv, boolPtr(tty), boolPtr(tty))
|
||||
c.setGlobals(Globals{Color: ColorAlways})
|
||||
c.Table(headers, rows)
|
||||
got := out.String()
|
||||
if bad := carriesInjected(got); bad != "" {
|
||||
t.Errorf("terminal %v: table carries %q:\n%q", tty, bad, got)
|
||||
}
|
||||
for _, want := range []string{"Name", "token", "one", "journal", "posts"} {
|
||||
if !strings.Contains(got, want) {
|
||||
t.Errorf("terminal %v: table lacks %q:\n%q", tty, want, got)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -30,7 +30,7 @@ Use cases:
|
||||
- Environment overrides: `SUMMER_LOG__` variables overlay the defaults key by key, and string values parse where YAML gives numbers or lists (`SUMMER_LOG__CHANNELS__DAILY__DAYS=30`, `SUMMER_LOG__CHANNELS__STACK__CHANNELS=console,daily`).
|
||||
- Validation: an unknown driver, format or level, a stack naming a missing channel or containing itself, and a relative daily path that leaves the application root fail `diary.Setup` with an error naming the key.
|
||||
- Console lines in bonfire's palette: time, level, the component as a fixed-width prefix, the message and `key=value` attributes. Control characters other than tab are removed from messages and values, so logged data cannot inject terminal escape sequences. While a bonfire live view (spinner, progress bar, process view) runs, styled lines print above it through `bonfire.PrintAbove`.
|
||||
- Daily files: the path `storage/logs/summer.log` gives files `storage/logs/summer-2026-10-21.log`, created mode 0640 in directories created 0750, opened on the first record, rotated at local midnight, and deleted after the retention in days (only files of that name pattern). A relative path resolves against the application root. A directory that cannot be written logs one warning from component `diary` on the console and disables the channel; it never fails the start-up.
|
||||
- Daily files: the path `storage/logs/summer.log` gives files `storage/logs/summer-2026-10-21.log`, created mode 0640 in directories created 0750, opened on the first record, rotated at local midnight, and deleted after the retention in days (only files of that name pattern). A relative path resolves against the application root. A file that cannot be opened (a directory that cannot be written, a full disk) logs a warning from component `diary` on the console, once per day; the channel skips records and tries again a minute later or at the first record of the next day, and it never fails the start-up.
|
||||
- Redaction: every channel leaf is wrapped by `sunscreen.Wrap`.
|
||||
- Runtime verbosity: `diary.SetVerbosity` lets info (1) or debug (2) through for every component and adds source locations on styled lines (3); quiet raises only the console channels to warn.
|
||||
- Console global options: `diary.FromGlobals` turns `-v`/`-vv`/`-vvv`, `-q` and `--ansi`/`--no-ansi` into `diary.Options`, and `diary.ApplyGlobals` applies them to the running setup (the generated main registers it with `bonfire.OnGlobals`).
|
||||
|
||||
@@ -12,7 +12,6 @@ import (
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
"time"
|
||||
"unicode"
|
||||
|
||||
"charm.land/lipgloss/v2"
|
||||
"git.golem15.com/golem15/summercms/modules/bonfire"
|
||||
@@ -141,12 +140,12 @@ func (h *consoleHandler) Handle(ctx context.Context, r slog.Record) error {
|
||||
label, style := levelLabel(r.Level, p)
|
||||
b.WriteString(style.Render(label))
|
||||
b.WriteString(strings.Repeat(" ", max(0, 5-len(label))+1))
|
||||
name := sanitize(component)
|
||||
name := bonfire.Sanitize(component)
|
||||
if name != "" {
|
||||
b.WriteString(p.Header.Render(name))
|
||||
}
|
||||
b.WriteString(strings.Repeat(" ", max(0, componentWidth-len(name))+1))
|
||||
b.WriteString(sanitize(r.Message))
|
||||
b.WriteString(bonfire.Sanitize(r.Message))
|
||||
for _, list := range [][]consoleAttr{h.attrs, recAttrs} {
|
||||
for _, a := range list {
|
||||
b.WriteByte(' ')
|
||||
@@ -247,7 +246,7 @@ func appendAttr(out []consoleAttr, prefix string, a slog.Attr) []consoleAttr {
|
||||
}
|
||||
return out
|
||||
}
|
||||
return append(out, consoleAttr{key: sanitize(prefix + a.Key), value: formatValue(a.Value)})
|
||||
return append(out, consoleAttr{key: bonfire.Sanitize(prefix + a.Key), value: formatValue(a.Value)})
|
||||
}
|
||||
|
||||
func formatValue(v slog.Value) string {
|
||||
@@ -266,35 +265,9 @@ func formatValue(v slog.Value) string {
|
||||
default:
|
||||
s = v.String()
|
||||
}
|
||||
s = sanitize(s)
|
||||
s = bonfire.Sanitize(s)
|
||||
if s == "" || strings.ContainsAny(s, " \t\"=") {
|
||||
return strconv.Quote(s)
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
// sanitize removes control characters other than tab; line breaks become
|
||||
// spaces so a multi-line value stays on one line.
|
||||
func sanitize(s string) string {
|
||||
clean := true
|
||||
for _, r := range s {
|
||||
if r != '\t' && unicode.IsControl(r) {
|
||||
clean = false
|
||||
break
|
||||
}
|
||||
}
|
||||
if clean {
|
||||
return s
|
||||
}
|
||||
var b strings.Builder
|
||||
for _, r := range s {
|
||||
switch {
|
||||
case r == '\n' || r == '\r':
|
||||
b.WriteByte(' ')
|
||||
case r != '\t' && unicode.IsControl(r):
|
||||
default:
|
||||
b.WriteRune(r)
|
||||
}
|
||||
}
|
||||
return b.String()
|
||||
}
|
||||
|
||||
@@ -12,23 +12,38 @@ import (
|
||||
|
||||
const dateLayout = "2006-01-02"
|
||||
|
||||
// dailyRetry is how long a daily writer skips records after a file failed
|
||||
// to open before it tries again on the same date.
|
||||
const dailyRetry = time.Minute
|
||||
|
||||
// dailyWriter writes to one file per local date: path storage/logs/summer.log
|
||||
// gives storage/logs/summer-2026-10-21.log. Files open lazily on the first
|
||||
// write (no empty files), are created 0o640 in directories created 0o750,
|
||||
// and on every open the files of the same base older than the retention are
|
||||
// deleted. If a file cannot be opened the writer reports once through warn
|
||||
// and is disabled: later writes succeed without writing anything.
|
||||
// deleted. If a file cannot be opened the writer skips records (writes
|
||||
// still succeed) and tries again one minute later by its clock, or at the
|
||||
// first record of a new date, whichever comes first; it reports through
|
||||
// warn once per date that fails.
|
||||
type dailyWriter struct {
|
||||
mu sync.Mutex
|
||||
dir string
|
||||
base string
|
||||
ext string
|
||||
days int
|
||||
now func() time.Time
|
||||
warn func(path string, err error)
|
||||
file *os.File
|
||||
date string
|
||||
disabled bool
|
||||
mu sync.Mutex
|
||||
dir string
|
||||
base string
|
||||
ext string
|
||||
days int
|
||||
now func() time.Time
|
||||
warn func(path string, err error)
|
||||
file *os.File
|
||||
date string
|
||||
// failedDate is the date whose file failed to open last; no open is
|
||||
// tried for it before retryAt.
|
||||
failedDate string
|
||||
retryAt time.Time
|
||||
// warnedDate is the last date a failure was reported for.
|
||||
warnedDate string
|
||||
// retired is set when a new setup replaced the writer's channels: a
|
||||
// late record is still appended, but the file is closed again after
|
||||
// every write, so the writer keeps no descriptor.
|
||||
retired bool
|
||||
}
|
||||
|
||||
func newDailyWriter(path string, days int, now func() time.Time, warn func(string, error)) *dailyWriter {
|
||||
@@ -54,15 +69,19 @@ func (d *dailyWriter) fileName(date string) string {
|
||||
// Write appends p to the current day's file in one write call.
|
||||
func (d *dailyWriter) Write(p []byte) (int, error) {
|
||||
d.mu.Lock()
|
||||
if d.disabled {
|
||||
d.mu.Unlock()
|
||||
return len(p), nil
|
||||
}
|
||||
date := d.now().Local().Format(dateLayout)
|
||||
now := d.now()
|
||||
date := now.Local().Format(dateLayout)
|
||||
if d.file == nil || date != d.date {
|
||||
if date == d.failedDate && now.Before(d.retryAt) {
|
||||
d.mu.Unlock()
|
||||
return len(p), nil
|
||||
}
|
||||
if err := d.open(date); err != nil {
|
||||
d.disabled = true
|
||||
warn := d.warn
|
||||
d.failedDate, d.retryAt = date, now.Add(dailyRetry)
|
||||
var warn func(string, error)
|
||||
if d.warnedDate != date {
|
||||
d.warnedDate, warn = date, d.warn
|
||||
}
|
||||
name := d.fileName(date)
|
||||
d.mu.Unlock()
|
||||
if warn != nil {
|
||||
@@ -70,8 +89,13 @@ func (d *dailyWriter) Write(p []byte) (int, error) {
|
||||
}
|
||||
return len(p), nil
|
||||
}
|
||||
d.failedDate = ""
|
||||
}
|
||||
n, err := d.file.Write(p)
|
||||
if d.retired {
|
||||
_ = d.file.Close()
|
||||
d.file, d.date = nil, ""
|
||||
}
|
||||
d.mu.Unlock()
|
||||
return n, err
|
||||
}
|
||||
@@ -139,6 +163,16 @@ func (d *dailyWriter) prune(today string) {
|
||||
}
|
||||
}
|
||||
|
||||
// retire closes the open file for good: later writes, from goroutines that
|
||||
// still hold the replaced channels, open the day's file, append and close
|
||||
// it again.
|
||||
func (d *dailyWriter) retire() error {
|
||||
d.mu.Lock()
|
||||
d.retired = true
|
||||
d.mu.Unlock()
|
||||
return d.Close()
|
||||
}
|
||||
|
||||
// Close closes the open file. A later write opens it again.
|
||||
func (d *dailyWriter) Close() error {
|
||||
d.mu.Lock()
|
||||
|
||||
@@ -9,6 +9,7 @@ import (
|
||||
"strings"
|
||||
"syscall"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"git.golem15.com/golem15/summercms/modules/diary"
|
||||
)
|
||||
@@ -268,3 +269,79 @@ func TestPathConfinement(t *testing.T) {
|
||||
t.Errorf("relative path did not resolve against the root: %d lines", len(lines))
|
||||
}
|
||||
}
|
||||
|
||||
// TestDailyRetriesAfterFailure (WR-04): a daily file that failed to open is
|
||||
// retried one minute later by the writer's clock, or on the first record of
|
||||
// a new date, whichever comes first; the warning appears once per date.
|
||||
func TestDailyRetriesAfterFailure(t *testing.T) {
|
||||
start := func(t *testing.T, at time.Time) (root string, clk *clock, buf *lockedBuffer) {
|
||||
t.Helper()
|
||||
root = t.TempDir()
|
||||
blocker := filepath.Join(root, "blocker")
|
||||
if err := os.WriteFile(blocker, []byte("x"), 0o600); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
clk = newClock(at)
|
||||
buf = &lockedBuffer{}
|
||||
setup(t, diary.Options{Root: root, Stderr: buf, Now: clk.Now},
|
||||
append(slices.Clone(dailyOnly), "SUMMER_LOG__CHANNELS__DAILY__PATH=blocker/logs/summer.log")...)
|
||||
return root, clk, buf
|
||||
}
|
||||
warnings := func(buf *lockedBuffer) int { return strings.Count(buf.String(), "daily log file disabled") }
|
||||
blog := diary.For("blog")
|
||||
|
||||
t.Run("one minute later", func(t *testing.T) {
|
||||
root, clk, buf := start(t, day(21, 10, 0, 0))
|
||||
blog.Info("first")
|
||||
if n := warnings(buf); n != 1 {
|
||||
t.Fatalf("%d warnings after the failed open, want 1:\n%s", n, buf.String())
|
||||
}
|
||||
if err := os.Remove(filepath.Join(root, "blocker")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
file := filepath.Join(root, "blocker", "logs", "summer-2026-10-21.log")
|
||||
clk.Set(day(21, 10, 0, 30))
|
||||
blog.Info("within the minute")
|
||||
if _, err := os.Stat(file); !os.IsNotExist(err) {
|
||||
t.Fatalf("retried before the minute passed (stat err %v)", err)
|
||||
}
|
||||
if n := warnings(buf); n != 1 {
|
||||
t.Fatalf("%d warnings within the minute, want 1:\n%s", n, buf.String())
|
||||
}
|
||||
clk.Set(day(21, 10, 1, 1))
|
||||
blog.Info("after the minute")
|
||||
lines := readDaily(t, file)
|
||||
if len(lines) != 1 || !strings.Contains(lines[0], "after the minute") {
|
||||
t.Fatalf("daily file after the retry = %q, want the record after the minute", lines)
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("next date", func(t *testing.T) {
|
||||
root, clk, buf := start(t, day(21, 23, 59, 59))
|
||||
blog.Info("before midnight")
|
||||
if err := os.Remove(filepath.Join(root, "blocker")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
clk.Set(day(22, 0, 0, 1))
|
||||
blog.Info("after midnight")
|
||||
lines := readDaily(t, filepath.Join(root, "blocker", "logs", "summer-2026-10-22.log"))
|
||||
if len(lines) != 1 || !strings.Contains(lines[0], "after midnight") {
|
||||
t.Fatalf("daily file of the new date = %q", lines)
|
||||
}
|
||||
if n := warnings(buf); n != 1 {
|
||||
t.Errorf("%d warnings, want 1:\n%s", n, buf.String())
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("next date fails too", func(t *testing.T) {
|
||||
_, clk, buf := start(t, day(21, 23, 59, 59))
|
||||
blog.Info("before midnight")
|
||||
clk.Set(day(22, 0, 0, 1))
|
||||
blog.Info("after midnight")
|
||||
clk.Set(day(22, 0, 2, 0))
|
||||
blog.Info("later")
|
||||
if n := warnings(buf); n != 2 {
|
||||
t.Errorf("%d warnings over two failed dates, want 2:\n%s", n, buf.String())
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
@@ -156,6 +156,12 @@ func Setup(cfg *compass.Config, opts Options) error {
|
||||
func install(conf Config, ch *channels, opts Options) {
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
installLocked(conf, ch, opts)
|
||||
}
|
||||
|
||||
// installLocked makes ch the installed setup and retires the channels it
|
||||
// replaces; the caller holds mu.
|
||||
func installLocked(conf Config, ch *channels, opts Options) {
|
||||
if saved == nil {
|
||||
saved = &savedDefault{logger: slog.Default(), writer: log.Writer(), flags: log.Flags()}
|
||||
}
|
||||
@@ -174,7 +180,7 @@ func install(conf Config, ch *channels, opts Options) {
|
||||
old := current.Swap(st)
|
||||
slog.SetDefault(slog.New(newComponentHandler("")))
|
||||
if old != nil {
|
||||
closeChannels(old.channels)
|
||||
retireChannels(old.channels)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -186,6 +192,11 @@ func install(conf Config, ch *channels, opts Options) {
|
||||
func SetVerbosity(v int, quiet bool) {
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
setVerbosityLocked(v, quiet)
|
||||
}
|
||||
|
||||
// setVerbosityLocked is SetVerbosity for a caller that holds mu.
|
||||
func setVerbosityLocked(v int, quiet bool) {
|
||||
verbosity.Store(int64(v))
|
||||
st := current.Load()
|
||||
if st == nil {
|
||||
@@ -212,18 +223,21 @@ func FromGlobals(g bonfire.Globals) Options {
|
||||
// the verbosity and quiet switch (SetVerbosity) and, when it differs from
|
||||
// the one the channels were built with, the console color mode, which
|
||||
// rebuilds the channels from the same configuration. The generated main
|
||||
// registers it with bonfire.OnGlobals.
|
||||
// registers it with bonfire.OnGlobals. It holds the setup lock for its
|
||||
// whole run, so concurrent calls rebuild the channels once.
|
||||
func ApplyGlobals(g bonfire.Globals) {
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
st := current.Load()
|
||||
if st != nil && st.opts.Color != g.Color {
|
||||
opts := st.opts
|
||||
opts.Color, opts.Verbosity, opts.Quiet = g.Color, g.Verbosity, g.Quiet
|
||||
if ch, err := buildChannels(st.conf, opts); err == nil {
|
||||
install(st.conf, ch, opts)
|
||||
installLocked(st.conf, ch, opts)
|
||||
return
|
||||
}
|
||||
}
|
||||
SetVerbosity(g.Verbosity, g.Quiet)
|
||||
setVerbosityLocked(g.Verbosity, g.Quiet)
|
||||
}
|
||||
|
||||
type hideOnTTYKey struct{}
|
||||
@@ -264,6 +278,22 @@ func Close() error {
|
||||
return closeChannels(st.channels)
|
||||
}
|
||||
|
||||
// retireChannels closes the files of channels a new setup replaced, for
|
||||
// good: a goroutine that still holds the old state appends its record and
|
||||
// the file is closed again, so no descriptor stays open.
|
||||
func retireChannels(ch *channels) {
|
||||
if ch == nil {
|
||||
return
|
||||
}
|
||||
for _, c := range ch.closers {
|
||||
if r, ok := c.(interface{ retire() error }); ok {
|
||||
_ = r.retire()
|
||||
continue
|
||||
}
|
||||
_ = c.Close()
|
||||
}
|
||||
}
|
||||
|
||||
func closeChannels(ch *channels) error {
|
||||
if ch == nil {
|
||||
return nil
|
||||
@@ -275,9 +305,9 @@ func closeChannels(ch *channels) error {
|
||||
return errors.Join(errs...)
|
||||
}
|
||||
|
||||
// Reset drops the installed setup, closes its files and restores the process
|
||||
// default logger (and the standard log package's output) captured at the
|
||||
// first Bootstrap or Setup. Tests call it in t.Cleanup.
|
||||
// Reset drops the installed setup, closes its files for good and restores
|
||||
// the process default logger (and the standard log package's output)
|
||||
// captured at the first Bootstrap or Setup. Tests call it in t.Cleanup.
|
||||
func Reset() {
|
||||
mu.Lock()
|
||||
defer mu.Unlock()
|
||||
@@ -290,6 +320,6 @@ func Reset() {
|
||||
saved = nil
|
||||
}
|
||||
if old != nil {
|
||||
closeChannels(old.channels)
|
||||
retireChannels(old.channels)
|
||||
}
|
||||
}
|
||||
|
||||
123
modules/diary/retire_internal_test.go
Normal file
123
modules/diary/retire_internal_test.go
Normal file
@@ -0,0 +1,123 @@
|
||||
package diary
|
||||
|
||||
import (
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sync"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"git.golem15.com/golem15/summercms/modules/bonfire"
|
||||
)
|
||||
|
||||
// installDefault builds the default configuration (console and daily) on
|
||||
// root and installs it, as Setup does.
|
||||
func installDefault(t *testing.T, root string, color bonfire.ColorMode) *channels {
|
||||
t.Helper()
|
||||
opts := Options{
|
||||
Root: root,
|
||||
Stderr: io.Discard,
|
||||
Environ: []string{},
|
||||
Color: color,
|
||||
Now: func() time.Time { return time.Date(2026, 10, 21, 12, 0, 0, 0, time.Local) },
|
||||
}.normalize()
|
||||
conf := DefaultConfig()
|
||||
ch, err := buildChannels(conf, opts)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
install(conf, ch, opts)
|
||||
return ch
|
||||
}
|
||||
|
||||
// dailyOf returns the daily writer of built channels.
|
||||
func dailyOf(t *testing.T, ch *channels) *dailyWriter {
|
||||
t.Helper()
|
||||
for _, c := range ch.closers {
|
||||
if d, ok := c.(*dailyWriter); ok {
|
||||
return d
|
||||
}
|
||||
}
|
||||
t.Fatal("no daily writer in the channels")
|
||||
return nil
|
||||
}
|
||||
|
||||
func (d *dailyWriter) hasFile() bool {
|
||||
d.mu.Lock()
|
||||
defer d.mu.Unlock()
|
||||
return d.file != nil
|
||||
}
|
||||
|
||||
// TestRetiredDailyWriterKeepsNoFile (WR-06): a daily writer replaced by a
|
||||
// new setup is retired: a late record through it (from a goroutine that
|
||||
// still holds the old state) is appended to the day's file and the file is
|
||||
// closed again. diary.Close keeps letting the installed writers reopen.
|
||||
func TestRetiredDailyWriterKeepsNoFile(t *testing.T) {
|
||||
t.Cleanup(Reset)
|
||||
root := t.TempDir()
|
||||
file := filepath.Join(root, "storage", "logs", "summer-2026-10-21.log")
|
||||
|
||||
old := dailyOf(t, installDefault(t, root, bonfire.ColorNever))
|
||||
if _, err := old.Write([]byte("first\n")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
installed := dailyOf(t, installDefault(t, root, bonfire.ColorNever))
|
||||
if old.hasFile() {
|
||||
t.Fatal("the replaced writer still holds its file")
|
||||
}
|
||||
if _, err := old.Write([]byte("late\n")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if old.hasFile() {
|
||||
t.Error("a late record through the replaced writer left its file open")
|
||||
}
|
||||
|
||||
if _, err := installed.Write([]byte("current\n")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := Close(); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if _, err := installed.Write([]byte("reopened\n")); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !installed.hasFile() {
|
||||
t.Error("the installed writer did not reopen its file after diary.Close")
|
||||
}
|
||||
raw, err := os.ReadFile(file)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if got := string(raw); got != "first\nlate\ncurrent\nreopened\n" {
|
||||
t.Errorf("daily file = %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplyGlobalsRebuildsOnce (WR-06): concurrent ApplyGlobals calls with
|
||||
// a new color mode rebuild the channels once, and every call's verbosity
|
||||
// is applied.
|
||||
func TestApplyGlobalsRebuildsOnce(t *testing.T) {
|
||||
t.Cleanup(Reset)
|
||||
installDefault(t, t.TempDir(), bonfire.ColorAuto)
|
||||
before := gens.Load()
|
||||
var start, done sync.WaitGroup
|
||||
start.Add(1)
|
||||
for range 50 {
|
||||
done.Add(1)
|
||||
go func() {
|
||||
defer done.Done()
|
||||
start.Wait()
|
||||
ApplyGlobals(bonfire.Globals{Color: bonfire.ColorNever, Verbosity: 2})
|
||||
}()
|
||||
}
|
||||
start.Done()
|
||||
done.Wait()
|
||||
if n := gens.Load() - before; n != 1 {
|
||||
t.Errorf("the channels were rebuilt %d times, want 1", n)
|
||||
}
|
||||
st := current.Load()
|
||||
if st.opts.Color != bonfire.ColorNever || st.verbosity != 2 || Verbosity() != 2 {
|
||||
t.Errorf("state after ApplyGlobals: color %v, verbosity %d (Verbosity %d)", st.opts.Color, st.verbosity, Verbosity())
|
||||
}
|
||||
}
|
||||
@@ -256,7 +256,7 @@ storage:
|
||||
|
||||
| Command | Flags | Description |
|
||||
|---------|-------|-------------|
|
||||
| `migrate` | `--pretend`, `--force` | Runs the framework migrations (`system_files`, the backend admin tables, `deferred_bindings` and the job queue), then each plugin's migrations in dependency order; prints `<set>: N applied` per set and `Applied N migrations`, or `Nothing to migrate`. `--pretend` prints each pending migration as `<plugin>: <id>` and `N migrations would run`, and applies nothing. In production on an interactive terminal it asks first (`--force` skips the question); off a terminal it never asks. |
|
||||
| `migrate` | `--pretend`, `--force` | Runs the framework migrations (`system_files`, the backend admin tables, `deferred_bindings` and the job queue), then each plugin's migrations in dependency order; prints `<set>: N applied` per set and `Applied N migrations`, or `Nothing to migrate`. The applied lines count what was actually applied: when a migration fails, `migrate` prints the lines for the sets applied before it, then the error, and no `Applied` line. `--pretend` prints each pending migration as `<plugin>: <id>` and `N migrations would run`, and applies nothing. In production on an interactive terminal it asks first (`--force` skips the question); off a terminal it never asks. |
|
||||
| `migrate:rollback` | `--plugin <id>`, `--pretend`, `--force` | Rolls back the last applied migration of the given plugin; without the flag, of the last activated plugin that has migrations. Prints `Rolled back <id> of <plugin>`; `--pretend` names the migration and changes nothing. Production confirmation as for `migrate`. |
|
||||
| `migrate:status` | `--json` | Prints a table of plugin, history table and applied migration IDs. `--json` prints a JSON array of every declared migration of those plugins: `plugin`, `table`, `id`, `applied`. |
|
||||
| `deferred:purge` | `--days <n>`, `--pretend`, `--force` | Deletes deferred bindings older than `n` days (default `database.deferred_bindings.purge_days`, else 5), with the unattached uploads they hold (blobs after the commit) and the records created under deferral, and prints the counts. Records that were only linked are kept. Model types come from every plugin's `pact.HasModels`; the uploads bucket from the published one or `storage.uploads.bucket_url`. `--pretend` counts the bindings past the cut-off and deletes nothing; production confirmation as for `migrate`. |
|
||||
|
||||
@@ -103,6 +103,22 @@ func lastApplied(gdb *gorm.DB, plugins []party.Plugin, pluginID string) (string,
|
||||
return "", nil
|
||||
}
|
||||
|
||||
// appliedSince returns the migrations of before that after no longer
|
||||
// lists as pending, in the order of before.
|
||||
func appliedSince(before, after []MigrationRef) []MigrationRef {
|
||||
still := make(map[MigrationRef]bool, len(after))
|
||||
for _, ref := range after {
|
||||
still[ref] = true
|
||||
}
|
||||
var applied []MigrationRef
|
||||
for _, ref := range before {
|
||||
if !still[ref] {
|
||||
applied = append(applied, ref)
|
||||
}
|
||||
}
|
||||
return applied
|
||||
}
|
||||
|
||||
// statusJSON is one migration of migrate:status --json.
|
||||
type statusJSON struct {
|
||||
Plugin string `json:"plugin"`
|
||||
@@ -154,12 +170,18 @@ asks.`,
|
||||
if err := confirmProduction(app, in, out, fmt.Sprintf("Apply %d migrations to the production database?", len(pending)), "Migration cancelled."); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := Migrate(gdb, plugins); err != nil {
|
||||
return err
|
||||
migrateErr := Migrate(gdb, plugins)
|
||||
// Report what Migrate actually applied: the pending
|
||||
// migrations before it ran that are no longer pending,
|
||||
// also when it failed partway.
|
||||
after, err := PendingMigrations(gdb, plugins)
|
||||
if err != nil {
|
||||
return errors.Join(migrateErr, err)
|
||||
}
|
||||
applied := appliedSince(pending, after)
|
||||
counts := map[string]int{}
|
||||
var order []string
|
||||
for _, ref := range pending {
|
||||
for _, ref := range applied {
|
||||
if counts[ref.Plugin] == 0 {
|
||||
order = append(order, ref.Plugin)
|
||||
}
|
||||
@@ -168,7 +190,10 @@ asks.`,
|
||||
for _, plugin := range order {
|
||||
out.Info(fmt.Sprintf("%s: %d applied", plugin, counts[plugin]))
|
||||
}
|
||||
out.Success(fmt.Sprintf("Applied %d migrations", len(pending)))
|
||||
if migrateErr != nil {
|
||||
return migrateErr
|
||||
}
|
||||
out.Success(fmt.Sprintf("Applied %d migrations", len(applied)))
|
||||
return nil
|
||||
})
|
||||
},
|
||||
|
||||
@@ -60,58 +60,23 @@ func migrator(gdb *gorm.DB, pluginID string, migrations []*gormigrate.Migration)
|
||||
// Migrate runs the framework-owned system_files, backend-admin,
|
||||
// deferred-binding (DeferredBindingMigrations, history DeferredHistoryID) and
|
||||
// job-queue (QueueMigrations) sets first, then each plugin's HasMigrations set
|
||||
// in party.Activate order.
|
||||
// in party.Activate order. It applies the sets migrationSets lists, the list
|
||||
// PendingMigrations reads, and stops at the first set that fails.
|
||||
func Migrate(gdb *gorm.DB, plugins []party.Plugin) error {
|
||||
if gdb == nil {
|
||||
return fmt.Errorf("lagoon: gorm db is nil")
|
||||
}
|
||||
m, err := migrator(gdb, "summercms.attach", attach.Migrations)
|
||||
sets, err := migrationSets(gdb, plugins)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if err := m.Migrate(); err != nil {
|
||||
return fmt.Errorf("lagoon: migrate system_files: %w", err)
|
||||
}
|
||||
admin, err := migrator(gdb, "summercms.cabana", BackendAdminMigrations)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if err := admin.Migrate(); err != nil {
|
||||
return fmt.Errorf("lagoon: migrate backend admin: %w", err)
|
||||
}
|
||||
deferred, err := migrator(gdb, DeferredHistoryID, DeferredBindingMigrations)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if err := deferred.Migrate(); err != nil {
|
||||
return fmt.Errorf("lagoon: migrate deferred bindings: %w", err)
|
||||
}
|
||||
sqlDB, err := gdb.DB()
|
||||
if err != nil {
|
||||
return fmt.Errorf("lagoon: migrate queue: %w", err)
|
||||
}
|
||||
queue, err := migrator(gdb, QueueHistoryID, QueueMigrations(sqlDB))
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if err := queue.Migrate(); err != nil {
|
||||
return fmt.Errorf("lagoon: migrate queue: %w", err)
|
||||
}
|
||||
for _, p := range plugins {
|
||||
hm, ok := p.(pact.HasMigrations)
|
||||
if !ok {
|
||||
continue
|
||||
}
|
||||
migrations := hm.Migrations()
|
||||
if len(migrations) == 0 {
|
||||
continue
|
||||
}
|
||||
m, err := migrator(gdb, p.ID(), migrations)
|
||||
for _, s := range sets {
|
||||
m, err := migrator(gdb, s.id, s.migrations)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if err := m.Migrate(); err != nil {
|
||||
return fmt.Errorf("lagoon: migrate %s: %w", p.ID(), err)
|
||||
return fmt.Errorf("lagoon: migrate %s: %w", s.label, err)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
@@ -211,7 +176,10 @@ type MigrationRef struct {
|
||||
|
||||
// migrationSet is one history table's declared migrations.
|
||||
type migrationSet struct {
|
||||
id string
|
||||
id string
|
||||
// label names the set in Migrate's errors: "system_files", "backend
|
||||
// admin", "deferred bindings", "queue" or the plugin ID.
|
||||
label string
|
||||
migrations []*gormigrate.Migration
|
||||
}
|
||||
|
||||
@@ -224,10 +192,10 @@ func migrationSets(gdb *gorm.DB, plugins []party.Plugin) ([]migrationSet, error)
|
||||
return nil, fmt.Errorf("lagoon: migration sets: %w", err)
|
||||
}
|
||||
sets := []migrationSet{
|
||||
{"summercms.attach", attach.Migrations},
|
||||
{"summercms.cabana", BackendAdminMigrations},
|
||||
{DeferredHistoryID, DeferredBindingMigrations},
|
||||
{QueueHistoryID, QueueMigrations(sqlDB)},
|
||||
{"summercms.attach", "system_files", attach.Migrations},
|
||||
{"summercms.cabana", "backend admin", BackendAdminMigrations},
|
||||
{DeferredHistoryID, "deferred bindings", DeferredBindingMigrations},
|
||||
{QueueHistoryID, "queue", QueueMigrations(sqlDB)},
|
||||
}
|
||||
for _, p := range plugins {
|
||||
hm, ok := p.(pact.HasMigrations)
|
||||
@@ -235,7 +203,7 @@ func migrationSets(gdb *gorm.DB, plugins []party.Plugin) ([]migrationSet, error)
|
||||
continue
|
||||
}
|
||||
if ms := hm.Migrations(); len(ms) > 0 {
|
||||
sets = append(sets, migrationSet{p.ID(), ms})
|
||||
sets = append(sets, migrationSet{p.ID(), p.ID(), ms})
|
||||
}
|
||||
}
|
||||
return sets, nil
|
||||
|
||||
@@ -294,3 +294,59 @@ func TestMigrateStatusJSON(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestMigrateAppliedLines (WR-07): migrate's "N applied" lines count what
|
||||
// Migrate applied, also when a later set fails: the sets before the failure
|
||||
// are reported, the error is returned and no success line is printed. The
|
||||
// next run reports only what it applied.
|
||||
func TestMigrateAppliedLines(t *testing.T) {
|
||||
db, dsn := dedicatedDB(t, "lagoon_applied_lines")
|
||||
gdb, err := Use(t.Context(), db)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
create := func(name, sql string) *gormigrate.Migration {
|
||||
return &gormigrate.Migration{
|
||||
ID: "202610220001_create_" + name,
|
||||
Migrate: func(tx *gorm.DB) error { return tx.Exec(sql).Error },
|
||||
Rollback: func(tx *gorm.DB) error { return tx.Exec(`DROP TABLE IF EXISTS ` + name).Error },
|
||||
}
|
||||
}
|
||||
alpha := migPlugin{id: "demo.okay", migrations: []*gormigrate.Migration{create("applied_okay", `CREATE TABLE applied_okay (id BIGSERIAL PRIMARY KEY)`)}}
|
||||
broken := migPlugin{id: "demo.broken", migrations: []*gormigrate.Migration{create("applied_broken", `CREATE TABLE applied_broken (id BOGUS_TYPE)`)}}
|
||||
framework, err := PendingMigrations(gdb, nil)
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
out, errOut, code, _ := runLagoon(t, "local", dsn, []party.Plugin{alpha, broken}, "migrate")
|
||||
if code != bonfire.ExitFailure || !strings.Contains(errOut, "demo.broken") {
|
||||
t.Fatalf("migrate with a failing set: exit %d\n%s%s", code, out, errOut)
|
||||
}
|
||||
if !strings.Contains(out, "demo.okay: 1 applied\n") {
|
||||
t.Errorf("the set applied before the failure is not reported:\n%s", out)
|
||||
}
|
||||
seen := map[string]bool{}
|
||||
for _, ref := range framework {
|
||||
if !seen[ref.Plugin] && !strings.Contains(out, ref.Plugin+": ") {
|
||||
t.Errorf("framework set %s not reported:\n%s", ref.Plugin, out)
|
||||
}
|
||||
seen[ref.Plugin] = true
|
||||
}
|
||||
if strings.Contains(out, "demo.broken: ") || strings.Contains(out, "Applied ") {
|
||||
t.Errorf("a failed set or a success line reported:\n%s", out)
|
||||
}
|
||||
|
||||
fixed := migPlugin{id: "demo.broken", migrations: []*gormigrate.Migration{create("applied_broken", `CREATE TABLE applied_broken (id BIGSERIAL PRIMARY KEY)`)}}
|
||||
out, errOut, code, _ = runLagoon(t, "local", dsn, []party.Plugin{alpha, fixed}, "migrate")
|
||||
if code != bonfire.ExitOK {
|
||||
t.Fatalf("second migrate: exit %d\n%s%s", code, out, errOut)
|
||||
}
|
||||
if !strings.Contains(out, "demo.broken: 1 applied\n") || !strings.Contains(out, "Applied 1 migrations") || strings.Contains(out, "demo.okay: ") {
|
||||
t.Errorf("second migrate result lines:\n%s", out)
|
||||
}
|
||||
out, _, _, _ = runLagoon(t, "local", dsn, []party.Plugin{alpha, fixed}, "migrate")
|
||||
if !strings.Contains(out, "Nothing to migrate") {
|
||||
t.Errorf("third migrate:\n%s", out)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -99,6 +99,9 @@ DIARY_TESTS=(
|
||||
TestNoRecursionWithoutState TestHideOnTTY TestConcurrentWrites
|
||||
# Plan 04: testing/slogtest conformance.
|
||||
TestSlogtestConsoleHandler TestSlogtestPipeline
|
||||
# Plan 05 (review fixes): daily retry, retired writers (WR-04, WR-06).
|
||||
TestDailyRetriesAfterFailure TestRetiredDailyWriterKeepsNoFile
|
||||
TestApplyGlobalsRebuildsOnce
|
||||
)
|
||||
SUNSCREEN_TESTS=(
|
||||
# Plan 04: the compatibility entry point.
|
||||
@@ -119,6 +122,13 @@ BONFIRE_TESTS=(
|
||||
# Plan 04: widgets, live views and prompts (D-03, D-27, D-28).
|
||||
TestWidgetMatrix TestLiveModels TestPromptsNoInteraction
|
||||
TestSecretWithNoInteraction TestConfirmProceed TestPrintAbove
|
||||
# Plan 05 (review fixes): live views (CR-01, WR-01, WR-02), exit codes
|
||||
# (CR-02), the masked hint (WR-08), control characters (WR-05).
|
||||
TestPrintAboveOrderedNoLoss TestPrintlnStaysWithItsConsole
|
||||
TestLiveViewSingleOwner TestLiveViewPanicUnregisters
|
||||
TestLiveViewRestoresTerminalOnSignal TestWithExitCode
|
||||
TestWrongDirectoryMasksSecrets TestSanitize TestRenderErrorStripsControl
|
||||
TestProcessViewStripsControl TestTableStripsControl
|
||||
)
|
||||
DOCSITE_TESTS=(
|
||||
# Plan 04: the docs checker reads past the global options.
|
||||
@@ -160,12 +170,16 @@ FLARE_TESTS=(
|
||||
DEV_TESTS=(
|
||||
# Plan 04: summer dev runs serve (D-29).
|
||||
TestStartBinServe TestDevReport TestDevGracefulNil
|
||||
# Plan 05 (review fixes): serve's full shutdown grace (WR-03).
|
||||
TestDevStopWaitsForGracefulShutdown
|
||||
)
|
||||
SUMMER_TESTS=(
|
||||
# Plan 03.
|
||||
TestCommandsDocumented TestDocsTree
|
||||
# Plan 04: the delegates' help (D-17).
|
||||
TestDelegateExamples
|
||||
# Plan 05 (review fixes): delegate exit codes and streams (CR-02, CR-03).
|
||||
TestDelegateExitCodePassthrough TestAppStreams TestDelegateStreams
|
||||
)
|
||||
|
||||
# hello stage.
|
||||
@@ -179,6 +193,8 @@ DOCKER_SURF_TESTS=(TestServeLifecycle)
|
||||
DOCKER_LAGOON_TESTS=(
|
||||
TestPendingMigrations TestMigratePretend TestRollbackPretend
|
||||
TestDeferredPurgePretend TestMigrateStatusJSON
|
||||
# Plan 05 (review fixes): applied lines from one set list (WR-07).
|
||||
TestMigrateAppliedLines
|
||||
)
|
||||
DOCKER_CABANA_TESTS=(TestTokenCreateJSON TestTokenListJSON TestTokenCreateQuietKeepsToken)
|
||||
|
||||
|
||||
Reference in New Issue
Block a user