docs(quick-261022-4z8): Fix the open 19.8 re-review findings (WR-09..WR-14, IN-14, IN-15)
This commit is contained in:
Jakub Zych
@@ -0,0 +1,367 @@
|
||||
---
|
||||
phase: quick-261022-4z8
|
||||
plan: 01
|
||||
quick_id: 261022-4z8
|
||||
slug: fix-the-open-19-8-re-review-findings-wr-
|
||||
type: execute
|
||||
mode: quick-full
|
||||
wave: 1
|
||||
depends_on: []
|
||||
autonomous: true
|
||||
requirements:
|
||||
- WR-09 # lines lost through a live view after its context is cancelled (verification gap 1)
|
||||
- WR-10 # signal-killed delegate prints raw argv with secrets (verification gap 2)
|
||||
- WR-11 # dash-leading secret values not masked in tryArgs (verification gap 2)
|
||||
- WR-12 # migrate:status --json omits the framework sets (verification gap 3)
|
||||
- WR-13 # migrate exits 1 when only the re-read after a successful Migrate fails
|
||||
- WR-14 # Process's plain path prints item names without Sanitize
|
||||
- IN-14 # ErrReported doc comment still says exit code 1
|
||||
- IN-15 # runLive builds the program before claiming the slot
|
||||
- 4z8-docs # disposition record + todo (IN-10..IN-13, exit-130 suggestion deferred)
|
||||
files_modified:
|
||||
# Task 1: bonfire (IN-14, WR-14, IN-15, WR-09)
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/helpers.go
|
||||
- modules/bonfire/live.go
|
||||
- modules/bonfire/live_test.go
|
||||
- modules/bonfire/README.md
|
||||
- docs/console/writing-commands.md
|
||||
- scripts/check-phase19.8.sh
|
||||
# Task 2: secret masking (WR-11, WR-10)
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/errors_test.go
|
||||
- internal/argmask/argmask.go
|
||||
- cmd/summer/runtime.go
|
||||
- cmd/summer/runtime_test.go
|
||||
- docs/console/introduction.md
|
||||
- docs/console/writing-commands.md
|
||||
- scripts/check-phase19.8.sh
|
||||
# Task 3: lagoon (WR-12, WR-13) + records
|
||||
- modules/lagoon/migrations.go
|
||||
- modules/lagoon/commands.go
|
||||
- modules/lagoon/migrations_test.go
|
||||
- modules/lagoon/example_test.go
|
||||
- modules/lagoon/pending_test.go
|
||||
- modules/lagoon/README.md
|
||||
- docs/console/setup-and-maintenance.md
|
||||
- docs/database/migrations.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
|
||||
|
||||
estimate:
|
||||
tokens: 160000
|
||||
raw_tokens: 160000
|
||||
tasks: 3
|
||||
confidence: low # estimate-calibration: factor 1, sample_count 0
|
||||
|
||||
must_haves:
|
||||
truths:
|
||||
- "WR-09: when the context passed to bonfire.Process is cancelled while its live view runs, the view ends with its last frame and is unregistered without waiting for the work function to return; afterwards Output.Println, Printf, Info and bonfire.PrintAbove print plainly (PrintAbove reports false) and none of those lines is lost. printAbove also reports false for a program whose Run has ended on its own."
|
||||
- "WR-14: bonfire.Process's plain path (no animation: -n on a terminal, forced color off a terminal, nested in another view) prints item names through bonfire.Sanitize; the process view's operation label is sanitized too."
|
||||
- "IN-15: runLive claims the single live slot before it builds the bubbletea program; a widget that loses the slot builds no program, so no child context is registered on the caller's context and no TEA_TRACE file is opened."
|
||||
- "IN-14: the ErrReported doc comment says the exit code is 1 or the code given with WithExitCode."
|
||||
- "WR-11: the wrong-directory Try line masks the argument after a secret-named long flag in the '--flag value' form whatever its first character (a value such as -Xk9!q becomes ***); only a following '--' is left unmasked."
|
||||
- "WR-10: summer's error for an app binary killed by a signal or failing to start names the argv with the same masking as the Try line (one shared implementation in internal/argmask), so a --password, --api-token=... or dash-leading secret value never reaches stderr; the exit code for a signal death stays 1. No exported framework identifier is added, removed or changed."
|
||||
- "WR-12: lagoon.Status (and so migrate:status and migrate:status --json) reads migrationSets, the list Migrate and PendingMigrations use: framework sets first, then plugins with migrations; --json lists every pending migration PendingMigrations returns, a pending framework-set migration included. Status keeps its signature and StatusRow its fields."
|
||||
- "WR-13: when Migrate succeeds but the follow-up PendingMigrations read fails, migrate prints a warning, reports every migration that was pending before the run as applied, prints 'Applied N migrations' and exits 0; when both fail it still returns both errors."
|
||||
- "Every code finding is its own commit with its pinning test (assertion RED recorded on the unchanged code), README/docs updated in the same commit; no Co-Authored-By trailer; nothing pushed or tagged."
|
||||
- "19.8-REVIEW-DISPOSITION.md marks WR-09..WR-14, IN-14 and IN-15 fixed with their commit hashes (frontmatter and table, open: 0), notes the WR-05/WR-07/WR-08 remainders (WR-14, WR-12, WR-11), and marks IN-10..IN-13 deferred; the todo carries IN-10..IN-13 and the exit-130 suggestion."
|
||||
artifacts:
|
||||
- path: modules/bonfire/live.go
|
||||
provides: "runLive claims the slot before tea.NewProgram, ends and unregisters the view on ctx cancellation; liveView.done; printAbove refuses a finished program"
|
||||
contains: "case <-v.done:"
|
||||
- path: modules/bonfire/helpers.go
|
||||
provides: "Process's plain path sanitizes item names"
|
||||
contains: "out.Success(Sanitize(item))"
|
||||
- path: modules/bonfire/errors.go
|
||||
provides: "ErrReported comment with WithExitCode; tryArgs delegating to internal/argmask"
|
||||
contains: "argmask.Join"
|
||||
- path: internal/argmask/argmask.go
|
||||
provides: "Join (shell-quoted argv with secret flag values masked) and Quote, shared by bonfire and cmd/summer"
|
||||
contains: "func Join(args []string) string"
|
||||
- path: cmd/summer/runtime.go
|
||||
provides: "runAppCmd names argv through argmask.Join"
|
||||
contains: "argmask.Join(argv)"
|
||||
- path: modules/lagoon/migrations.go
|
||||
provides: "Status built from migrationSets"
|
||||
contains: "func Status(gdb *gorm.DB, plugins []party.Plugin) ([]StatusRow, error)"
|
||||
- path: modules/lagoon/commands.go
|
||||
provides: "migrate warns instead of failing on a failed re-read after success"
|
||||
contains: "Could not re-read the migration history"
|
||||
- path: modules/bonfire/live_test.go
|
||||
provides: "TestPrintAboveAfterCancel (WR-09), TestNestedProcessBuildsNoProgram (IN-15), plain-path cases in TestProcessViewStripsControl (WR-14)"
|
||||
contains: "func TestPrintAboveAfterCancel"
|
||||
- path: cmd/summer/runtime_test.go
|
||||
provides: "TestRunAppCmdMasksSecrets and the masked signal subtest of TestDelegateExitCodePassthrough (WR-10)"
|
||||
contains: "func TestRunAppCmdMasksSecrets"
|
||||
- path: modules/lagoon/pending_test.go
|
||||
provides: "TestMigrateStatusJSON pending framework migration (WR-12), TestMigrateRereadFailureWarns (WR-13)"
|
||||
contains: "func TestMigrateRereadFailureWarns"
|
||||
- path: .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
|
||||
provides: "dispositions recorded"
|
||||
contains: "open: 0"
|
||||
key_links:
|
||||
- from: cmd/summer/runtime.go
|
||||
to: internal/argmask/argmask.go
|
||||
via: "runAppCmd wraps every child error as summer: <argmask.Join(argv)>: <err>"
|
||||
pattern: "argmask\\.Join\\(argv\\)"
|
||||
- from: modules/bonfire/errors.go
|
||||
to: internal/argmask/argmask.go
|
||||
via: "tryArgs and the cd line use argmask.Join and argmask.Quote (one masking implementation)"
|
||||
pattern: "argmask\\.(Join|Quote)"
|
||||
- from: modules/lagoon/migrations.go
|
||||
to: modules/lagoon/migrations.go
|
||||
via: "Migrate, Status and PendingMigrations all call migrationSets(gdb, plugins)"
|
||||
pattern: "migrationSets\\(gdb, plugins\\)"
|
||||
- from: modules/bonfire/live.go
|
||||
to: modules/bonfire/helpers.go
|
||||
via: "Process passes its ctx to runLive; runLive's cancel watcher finishes and unregisters the view; printAbove checks liveView.done"
|
||||
pattern: "case <-v\\.done"
|
||||
- from: modules/bonfire/helpers.go
|
||||
to: modules/bonfire/errors.go
|
||||
via: "Process plain path calls Sanitize on the item"
|
||||
pattern: "Sanitize\\(item\\)"
|
||||
---
|
||||
|
||||
<objective>
|
||||
Fix the open findings of the 19.8 code re-review (`19.8-REVIEW.md`): WR-09, WR-10, WR-11 and WR-12 (the three verification gaps in `19.8-VERIFICATION.md`), WR-13, WR-14, IN-14 and IN-15. Then record the dispositions and defer IN-10..IN-13 plus WR-10's exit-130 suggestion to the existing todo.
|
||||
|
||||
Purpose: close the three gaps that keep phase 19.8 at `gaps_found`: lines lost after a cancelled live view, secrets leaking through argv, and `migrate:status --json` missing framework sets. The four quality follow-ups go in the same pass.
|
||||
Output: nine local commits on master. There is one `fix(...)`/`docs(bonfire)` commit per finding, each with its pinning test and README/docs, and then one `docs(19.8)` records commit. Nothing is pushed, tagged, released or posted to the changelog.
|
||||
|
||||
Shape: this is a gap-closure-style quick task. The orchestrator fixed the grouping (three tasks), so there is no tracer task. Each finding follows RED → fix → GREEN → commit, the pattern of plan 19.8-05 (`19.8-05-SUMMARY.md`, "Each review finding: pinning test red on unchanged code, fix, green, one fix(...) commit with test and docs").
|
||||
|
||||
**One commit per finding (hard rule).** Each task holds several findings, and each finding is its own commit, in the order the task lists. The commit subject follows the repo's style and ends with the finding ID in parentheses. There is NO `Co-Authored-By` trailer: the user's CLAUDE.md overrides any attribution reminder. Planning docs and code are never in the same commit.
|
||||
|
||||
Decisions (planner's discretion; the user is asleep and the run is unattended, so the executor does not revisit these and records them in the SUMMARY):
|
||||
|
||||
- **P-01 (WR-09 design).** The live program no longer takes the caller's context. Drop bubbletea's context program option from `tea.NewProgram` in `runLive`. Instead, `runLive` starts a watcher goroutine. When `ctx` is done, the watcher takes `liveMu`, unregisters the view if it is still the active one, sends `finishMsg{err: ctx.Err()}`, waits for `finished` and releases `liveMu`. This is the same unregister/finish/wait-under-one-lock step as the deferred cleanup. When `finished` closes first (Run ended on its own, a signal kill, or the normal end), the watcher only unregisters the view under `liveMu` if it is still active. In addition, `liveView` gets a `done <-chan struct{}` field (runLive's `finished` channel, created before the claim). `printAbove` checks it under `liveMu` after the owner check. When it is closed, `printAbove` unregisters the view and returns false. Why not the review's check-then-Send: a cancellation landing between the check and `Program.Send` would still drop the line, because bubbletea's Send returns as soon as its context is done. With P-01 the program stays alive until the watcher has unregistered it under the lock, so every line is either delivered to a running program or printed plainly. P-01 also renders the final frame with the failed rows, which the review notes the killed program never did. Spinner and Progress pass `context.Background()`, so P-01 does not change them. Fallback: if the graceful finish deadlocks or cannot be made to pass under `-race`, implement the review's version and record the deviation. That version keeps the context option, checks `ctx.Done()` and `done` in `printAbove`, and clears `activeView` when `finished` closes.
|
||||
- **P-02 (IN-15).** Claim first, then build. Under `liveMu`, return `false, nil` if `activeView` is set. Otherwise build the program while still holding the lock, register the view and unlock. `NewProgram` takes no bonfire lock and calls nothing that prints, so this cannot deadlock.
|
||||
- **P-03 (WR-14).** Sanitize at the call sites: `out.Success(Sanitize(item))` in `Process`'s plain path, and `Sanitize(op)` for the process view's operation label. Do not sanitize inside `console.status`: callers pass intentionally styled (ANSI) messages to Info/Success/Warning, and those must keep their styling.
|
||||
- **P-04 (WR-11).** In the `--flag value` form, mask the next argument after a secret-named long flag whenever it exists and is not `--`, whatever its first character. `tryArgs` cannot see flag types, so a secret-named boolean flag over-masks the following argument. That is the safe side. The existing case `sync --token --force` therefore changes from `--token --force` to `--token ***`, and the test comment says why. pflag also takes `--force` as the token's value there (v1.0.10 flag.go, the `'--flag arg'` branch).
|
||||
- **P-05 (WR-10).** The user asked for the same masking as `tryArgs` with no new exported API unless needed. It is not needed: the masking moves to a new internal package `internal/argmask`, importable from both `modules/bonfire` and `cmd/summer` (same module root). It provides `Join(args []string) string` (shell-quoted, space-separated, secret values masked, no leading space) and `Quote(s string) string`, which is bonfire's former `shellQuote`. bonfire keeps the unexported `tryArgs` as a thin wrapper (empty for no args, otherwise a space plus `argmask.Join(args)`). It calls `argmask.Quote` for the `cd` path and drops its own `secretWords`/`secretFlag`/`shellQuote`, which nothing else references (verified by grep at planning time). `runAppCmd` builds its error with `argmask.Join(argv)` in every branch: start failure, signal death, and the already-Reported exit branch. The exit code stays 1 on a signal death (plan 05's CR-02 truth). The exit-130 suggestion goes to the todo.
|
||||
- **P-06 (WR-12).** `Status` iterates `migrationSets(gdb, plugins)`: framework sets first (`StatusRow.Plugin` is the set's history ID, for example `summercms.attach`, the same convention as `MigrationRef.Plugin`), then plugins with at least one migration. A plugin whose `HasMigrations` returns none no longer gets an empty row; Migrate and PendingMigrations skip it too, which keeps the three readers consistent. The signature of `Status`, the `StatusRow` type and its fields are unchanged. No consumer outside this repo calls `lagoon.Status` (grep over the meta repo at planning time). The `migrate:status` Description stays as it is, because changing it would churn the list goldens and the list excerpt in `docs/console/introduction.md`. Its Long help and the docs change. The `--json` builder in commands.go keeps its loop: it now covers framework rows, because `row.Plugin` equals `ref.Plugin` for them.
|
||||
- **P-07 (WR-13).** When `migrateErr == nil` and the re-read fails, `applied` is the pending list from before the run. migrate prints `out.Warning("Could not re-read the migration history: " + bonfire.Sanitize(err.Error()))` (wording may be refined; it must contain `Could not re-read the migration history`), then the per-set lines and `Applied N migrations`, and returns nil. When both fail, it keeps `errors.Join(migrateErr, err)`.
|
||||
- **P-08.** Each new top-level pinning test name joins its list in `scripts/check-phase19.8.sh`, in the same commit as the test, so the phase gate keeps requiring it. Tests extended in place are already listed.
|
||||
- **P-09.** `docs/setup/upgrading.md` is not changed, because no release is cut. The SUMMARY gets a "Release-note input" section for whoever tags next: `lagoon.Status` and `migrate:status` now list framework sets; a cancelled Process view ends with its final frame; secret masking now covers dash-leading values and delegate signal errors.
|
||||
</objective>
|
||||
|
||||
<execution_context>
|
||||
@~/.claude/msd-core/workflows/execute-plan.md
|
||||
@~/.claude/msd-core/templates/summary.md
|
||||
</execution_context>
|
||||
|
||||
<context>
|
||||
@CLAUDE.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-VERIFICATION.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-05-SUMMARY.md
|
||||
@.planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
|
||||
@.planning/todos/pending/2026-10-22-console-review-info-findings.md
|
||||
|
||||
Source, read once each, only the regions named:
|
||||
@modules/bonfire/live.go (runLive, liveView, watchSignals, printAbove: about lines 323-487; processModel.View: about lines 287-320)
|
||||
@modules/bonfire/helpers.go (Process: lines 140-180)
|
||||
@modules/bonfire/errors.go (ErrReported: about line 73; wrongDirectory, tryArgs, shellQuote: about lines 250-340)
|
||||
@modules/bonfire/output.go (emit, Println, Printf, status: lines 170-245)
|
||||
@modules/bonfire/live_test.go (lockedBuffer, TestPrintAboveOrderedNoLoss, TestProcessViewStripsControl)
|
||||
@modules/bonfire/errors_test.go (TestWrongDirectoryMasksSecrets, carriesInjected)
|
||||
@modules/bonfire/widgets_test.go (TestTableStripsControl: the console construction with Globals{Color: ColorAlways})
|
||||
@cmd/summer/runtime.go (appStreams, runAppCmd, newAppCmd, runChild: lines 130-238)
|
||||
@cmd/summer/runtime_test.go (TestExitHelperProcess, exitHelperRunner, TestDelegateExitCodePassthrough: lines 290-370)
|
||||
@modules/lagoon/migrations.go (Status, migrationSets, PendingMigrations)
|
||||
@modules/lagoon/commands.go (migrate Run: lines 151-195; migrate:status: lines 255-295)
|
||||
@modules/lagoon/pending_test.go (runLagoon, TestMigrateStatusJSON, TestMigrateAppliedLines)
|
||||
@modules/lagoon/migrations_test.go (TestTwoPluginMigrationSetsIsolated: lines 138-260)
|
||||
@modules/lagoon/example_test.go (migrateBlog, TestDocsMigrate: lines 335-372)
|
||||
@scripts/check-phase19.8.sh (test name lists: lines 110-200)
|
||||
|
||||
Environment rules for every step (from the orchestrator; non-negotiable):
|
||||
- **GOCACHE.** Every go command, including any script that calls go (`scripts/check-phase19.8.sh`), runs with `env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache`. Below, `$GC` stands for that literal path; write it out in full, because the Bash tool runs fish. Wrap multi-statement scripts or `VAR=$(…)` in `bash -c '…'`. The disk is nearly full: do not create other build caches and do not delete the gocache. The orchestrator deletes it at the end.
|
||||
- **Docker.** Docker-backed tests run with `env GOCACHE=$GC TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test …`. If the Docker socket is denied, run the same command through `sudo -u golem` (CLAUDE.md).
|
||||
- **Concurrent sessions.** Record `BASE=$(git rev-parse HEAD)` before the first edit. Before editing any file, run `git status --short -- <file>`. If it shows changes this task did not make, do not edit that file. Record the skipped update in the SUMMARY as a follow-up. Never touch, stage or commit `.planning/STATE.md`, `.planning/state.json`, `.msd/`, any cabana or `docs/backend` file, or other quick or phase directories. Commit with explicit paths only: `git add -- <paths>` then `git commit -m "<subject>" -- <paths>`. The `-- <paths>` form commits only those paths, even if another session has staged something. Never use `git add -A` or `git add .`.
|
||||
- **Foreign test failures.** If `cmd/summer` docs tests (TestDocsTree and others) fail only because of another session's uncommitted files, export clean HEAD with `bash -c 'D=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/clean-head; mkdir -p "$D" && git archive HEAD | tar -x -C "$D"'`, run the failing package's tests there with the same GOCACHE, record both results in the SUMMARY, then remove that export directory.
|
||||
- **Known flake.** `TestOIDCSubjectIsRandomSmoke` (wristband; deferred-items.md) can fail once at random. Rerun it once before treating it as a failure.
|
||||
</context>
|
||||
|
||||
<tasks>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 1: bonfire live views and Process: IN-14, WR-14, IN-15, WR-09 (four commits)</name>
|
||||
<files>modules/bonfire/errors.go, modules/bonfire/helpers.go, modules/bonfire/live.go, modules/bonfire/live_test.go, modules/bonfire/README.md, docs/console/writing-commands.md, scripts/check-phase19.8.sh</files>
|
||||
<behavior>
|
||||
- IN-14: no test (doc comment only).
|
||||
- WR-14 (extend TestProcessViewStripsControl): (a) the existing model case also builds the process model with operation "Push\x1b]0;pwned\x07ing", and the View carries none of the injected sequences while showing "Push" and "ing"; (b) plain path, terminal: newConsole(tty true, interactive true) plus setGlobals(Globals{Color: ColorAlways, NoInteraction: true}), so live() is false and the profile is colored; Process over the item "evil\x1b]0;pwned\x07item" with fn returning nil prints no injected sequence, and "evil" and "item" are visible; (c) plain path off a terminal: tty false plus Globals{Color: ColorAlways}, same assertions. (b) and (c) fail on the unchanged code; TestTableStripsControl shows the colored writer passes OSC and BEL through.
|
||||
- IN-15 (new TestNestedProcessBuildsNoProgram): on a tty console, inside c.Spinner("outer", …), call t.Setenv("TEA_TRACE", <tempdir>/trace.log), then run Process with a test-only counting context over one item "x". The counting context is a type whose Done returns a non-nil channel that is never closed, whose Err and Value return nil, and which implements AfterFunc(f func()) (stop func() bool), counting registrations minus stops under a mutex. context.WithCancel registers a child through the parent's AfterFunc when the parent is not a stdlib cancel context. Assert: fn ran once; the trace file does not exist; outstanding registrations are 0; the "x" success line reached the buffer. On the unchanged code NewProgram opens the trace file and registers one child that is never stopped, so the test fails.
|
||||
- WR-09 (new TestPrintAboveAfterCancel): use a tty console on a lockedBuffer, ctx and cancel from context.WithCancel, and items "first", "second", "third". fn records each item it gets. On "second" it calls cancel(), then polls for up to 2s (every 5ms) until activeView is nil, reading it under liveMu; if the view is still registered it reports t.Errorf("view still registered … after the cancellation"). Then it calls c.Println("after-cancel-println"), c.Info("after-cancel-info") and PrintAbove("after-cancel-printabove"). PrintAbove must report false; when it does, fn writes "after-cancel-printabove\n" to the buffer, as diary does. fn then returns nil. After Process: errors.Is(err, context.Canceled) is true; fn never got "third"; ansi.Strip(output) contains each of the three after-cancel lines exactly once; PrintAbove("after") is false. On the unchanged code the poll times out, PrintAbove reports true and the lines are lost (the review reproduced this 3 of 3 on a pty), so the test fails without a sleep-based race.
|
||||
</behavior>
|
||||
<action>
|
||||
Work in this order. Each step is one commit. Before the first edit, record BASE (see context). For every code finding, write or extend the test first and run it on the unchanged code. Paste the failing output (the RED) into your notes for the SUMMARY. Only then fix, run it GREEN, update the docs and commit.
|
||||
|
||||
1. **IN-14 (doc only, commit 1).** In modules/bonfire/errors.go, change the ErrReported doc comment so it says the error block prints nothing and the exit code is 1, or the code given with WithExitCode (review IN-14 fix text). The `var ErrReported` line itself stays byte-identical. No test. Commit: `docs(bonfire): the ErrReported comment names the WithExitCode code (IN-14)`, paths modules/bonfire/errors.go.
|
||||
|
||||
2. **WR-14 (commit 2).** Extend TestProcessViewStripsControl in modules/bonfire/live_test.go as described in behavior (a)-(c), reusing carriesInjected from errors_test.go. It is the same package, `bonfire`. Run it RED. Fix per P-03: in modules/bonfire/helpers.go, the plain path's success line becomes out.Success(Sanitize(item)). The failure line already goes through failureText. In modules/bonfire/live.go processModel.View, sanitize the operation label (`op`) before rendering. Update modules/bonfire/README.md line 30 ("Table cells, process-view item names and errors, and failure lines are stripped…") to cover Process item names on both paths and the operation label. writing-commands.md:155 already promises this, so leave it. Commit: `fix(bonfire): strip control characters from Process's plain item lines and the view's label (WR-14)`, paths helpers.go, live.go, live_test.go, README.md.
|
||||
|
||||
3. **IN-15 (commit 3).** Add TestNestedProcessBuildsNoProgram to modules/bonfire/live_test.go per behavior and run it RED. Fix per P-02 in runLive: take liveMu; if activeView is set, unlock and return false, nil. Otherwise build the program with the existing options, set activeView and unlock. Keep the rest of runLive unchanged in this commit. Update runLive's doc comment: it claims the slot before building the program, so a widget that loses the slot builds no program. Add TestNestedProcessBuildsNoProgram to the bonfire list in scripts/check-phase19.8.sh (the array that ends with `TestProcessViewStripsControl TestTableStripsControl`), under a comment line `# quick 261022-4z8 (re-review fixes): IN-15, WR-09.` Commit: `fix(bonfire): claim the live view slot before building the program (IN-15)`, paths live.go, live_test.go, scripts/check-phase19.8.sh.
|
||||
|
||||
4. **WR-09 (commit 4).** Add TestPrintAboveAfterCancel per behavior and run it RED. It must fail on the poll and on the lost lines, not hang. Fix per P-01 in modules/bonfire/live.go:
|
||||
- (a) Add `done <-chan struct{}` to liveView, with a field comment. Create `finished` before the claim and set it on the view.
|
||||
- (b) Remove bubbletea's context program option from the NewProgram call, so the program's lifetime is the view's.
|
||||
- (c) After `go prog.Run()`, start the watcher goroutine described in P-01. Select on ctx.Done() and finished. The ctx branch unregisters, sends finishMsg{err: ctx.Err()} and waits for finished, all while holding liveMu. The finished branch only unregisters under liveMu. Both branches clear activeView only when it is still this view. A nil ctx.Done() (Background) just waits for finished.
|
||||
- (d) In printAbove, read activeView into a local named `v`, as in the review's sketch. After the owner check, add a non-blocking select with a `case <-v.done:` branch. When `done` is closed, set activeView to nil and return false.
|
||||
- (e) Update the doc comments of runLive, liveView and printAbove. Say that cancelling ctx ends the view with its last frame and unregisters it without waiting for work, so lines printed after the cancellation print plainly below it. Say also that a program whose Run ended on its own is never sent lines.
|
||||
|
||||
Check that the deferred cleanup still works when the watcher has already finished the program. Its Send returns at once (Run's deferred cancel) and finished is closed. Keep watchSignals as it is.
|
||||
|
||||
Run TestPrintAboveAfterCancel, TestNestedProcessBuildsNoProgram, TestPrintAboveOrderedNoLoss, TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters, TestLiveViewRestoresTerminalOnSignal, TestPrintAbove and TestLiveModels with -race -count=3. All must pass.
|
||||
|
||||
Docs: in modules/bonfire/README.md line 30 and docs/console/writing-commands.md line 77, add one sentence: when the context given to bonfire.Process is cancelled, the view ends with its last frame and later lines print plainly below it. Add TestPrintAboveAfterCancel to the same gate list line as IN-15.
|
||||
|
||||
Commit: `fix(bonfire): end a live view when its context is cancelled and print later lines plainly (WR-09)`, paths live.go, live_test.go, README.md, docs/console/writing-commands.md, scripts/check-phase19.8.sh.
|
||||
|
||||
After each commit, run `env GOCACHE=$GC go vet ./modules/bonfire/` and `env GOCACHE=$GC go test -short ./modules/bonfire/ ./modules/diary/`. diary routes console records through bonfire.PrintAbove.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go test -count=3 -race ./modules/bonfire/ -run 'TestPrintAboveAfterCancel|TestNestedProcessBuildsNoProgram|TestProcessViewStripsControl|TestPrintAboveOrderedNoLoss|TestPrintlnStaysWithItsConsole|TestLiveViewSingleOwner|TestLiveViewPanicUnregisters|TestLiveViewRestoresTerminalOnSignal|TestPrintAbove$|TestLiveModels' && env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go test -short -count=1 ./modules/bonfire/ ./modules/diary/ && env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go vet ./modules/bonfire/ && grep -v '^\s*//' modules/bonfire/live.go | grep -c 'WithContext(' | grep -qx 0 && git log --format=%s -4 > /tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/t1-subjects && grep -c '(IN-14)\|(WR-14)\|(IN-15)\|(WR-09)' /tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/t1-subjects | grep -qx 4</automated>
|
||||
</verify>
|
||||
<done>Four commits exist, in the order IN-14, WR-14, IN-15, WR-09, each touching only its listed paths and none with a Co-Authored-By trailer. The RED output of WR-14, IN-15 and WR-09 on unchanged code is captured for the SUMMARY. The bonfire tests pass with -race -count=3. TestPrintAboveAfterCancel and TestNestedProcessBuildsNoProgram are in the gate's bonfire list. The README and writing-commands.md describe the cancel behaviour and the sanitized plain path.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 2: secret masking: WR-11, WR-10 (two commits)</name>
|
||||
<files>modules/bonfire/errors.go, modules/bonfire/errors_test.go, internal/argmask/argmask.go, cmd/summer/runtime.go, cmd/summer/runtime_test.go, docs/console/introduction.md, docs/console/writing-commands.md, scripts/check-phase19.8.sh</files>
|
||||
<behavior>
|
||||
- WR-11 (extend TestWrongDirectoryMasksSecrets): new cases. `admin:create --email a@b.c --password -Xk9!q` gives `cd ..; {bin} admin:create --email a@b.c --password ***`. `sync --client-secret -s3cr3t --verbose` gives `cd ..; {bin} sync --client-secret *** --verbose`. `sync --token -- x` gives `cd ..; {bin} sync --token -- x`. The existing case `sync --token --force` now expects `cd ..; {bin} sync --token ***` (P-04; the test comment says pflag takes `--force` as the value). Add "-Xk9" and "-s3cr3t" to the list of secrets that must not appear. The new dash cases fail on the unchanged code.
|
||||
- WR-10 (new TestRunAppCmdMasksSecrets; skipped on Windows like its neighbours). argv is `admin:create --email a@b.c --password hunter2 --api-token=t0k3n --client-secret -Xk9q`. Subtest "killed by a signal": newAppCmd(ctx, os.Args[0], ".", []string{"-test.run=^TestExitHelperProcess$"}, io.Discard, io.Discard) with SUMMER_EXIT_HELPER=sigkill appended to cmd.Env, then runAppCmd(cmd, argv). The error is not ErrReported, contains "signal: killed", "--password ***", "--api-token=***" and "--client-secret ***", and contains none of hunter2, t0k3n, -Xk9q. Subtest "start failure": newAppCmd with a binary path inside t.TempDir() that does not exist; same masking assertions. Extend TestDelegateExitCodePassthrough: give its run helper an args parameter, keep the two existing subtests unchanged, and add a subtest "killed by a signal masks secrets". It runs `migrate --password hunter2` through bonfire.Execute with the sigkill helper and asserts exit 1 (bonfire.ExitFailure; P-05 keeps it), exactly one " ERROR " block, "--password ***" in stderr, and no "hunter2" anywhere. All new assertions fail on the unchanged code (raw join of argv).
|
||||
</behavior>
|
||||
<action>
|
||||
Do WR-11 first, then WR-10. Each is its own commit.
|
||||
|
||||
1. **WR-11 (commit 5).** Extend TestWrongDirectoryMasksSecrets per behavior and run it RED. Fix per P-04 in tryArgs (modules/bonfire/errors.go): in the `--flag value` branch, mask args[i+1] when it exists and is not exactly `--`, and drop the leading-dash exclusion. Update tryArgs's doc comment: the next argument is masked whatever its first character, except `--`; a secret-named boolean flag over-masks its follower, which is the safe side. docs/console/writing-commands.md line 205 already promises masking. Add "(also a value that starts with `-`)" after `--password ***` there, so the docs state the rule. Commit: `fix(bonfire): mask secret flag values that start with a dash (WR-11)`, paths errors.go, errors_test.go, docs/console/writing-commands.md.
|
||||
|
||||
2. **WR-10 (commit 6).** Add TestRunAppCmdMasksSecrets and the TestDelegateExitCodePassthrough subtest per behavior, and run them RED. Fix per P-05:
|
||||
- (a) Create internal/argmask/argmask.go with a package doc comment ("Package argmask renders a command line for messages with the values of secret-named long flags masked; bonfire's wrong-directory hint and summer's delegate errors share it"). Move bonfire's secret-word list, the secret-flag check, the masking loop (as `Join`, which joins with single spaces and has no leading space) and the shell quoting (as `Quote`) into it unchanged, apart from the WR-11 rule now in place. It imports only the standard library.
|
||||
- (b) In modules/bonfire/errors.go, tryArgs returns "" for no args and otherwise a space followed by argmask.Join(args). The `cd` path uses argmask.Quote. Delete bonfire's now-unused secretWords, secretFlag and shellQuote, after grepping the package (tests included) to confirm nothing else uses them. TestWrongDirectoryMasksSecrets and TestWrongDirectoryHint must pass unchanged.
|
||||
- (c) In cmd/summer/runtime.go runAppCmd, compute `args := argmask.Join(argv)` once and build the wrapped error from it in place of the raw join. The `strings` import may become unused; remove it if so. Update runAppCmd's doc comment: the error names the command with secret-looking flag values masked; a binary ended by a signal keeps exit code 1.
|
||||
- (d) Add TestRunAppCmdMasksSecrets to the SUMMER list in scripts/check-phase19.8.sh, after the line holding TestDelegateExitCodePassthrough, with a comment `# quick 261022-4z8 (re-review fixes): WR-10.`
|
||||
- (e) Docs: in docs/console/introduction.md line 146, extend the sentence about a binary killed by a signal. The block names the command with the values of secret-looking flags masked as `***`. Keep exit code `1` in the text.
|
||||
|
||||
Run the API check (see verification) to confirm no exported identifier changed. Commit: `fix(summer): mask secret flag values in a delegate's error (WR-10)`, paths internal/argmask/argmask.go, modules/bonfire/errors.go, cmd/summer/runtime.go, cmd/summer/runtime_test.go, docs/console/introduction.md, scripts/check-phase19.8.sh.
|
||||
|
||||
After each commit, run `env GOCACHE=$GC go vet ./internal/argmask/ ./modules/bonfire/ ./cmd/summer/` and the verify command below.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go test -count=1 ./modules/bonfire/ -run 'TestWrongDirectory' && env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go test -count=1 ./cmd/summer/ -run 'TestDelegateExitCodePassthrough|TestRunAppCmdMasksSecrets|TestDelegateStreams|TestAppStreams|TestRunChildForwardsSignals' && env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache go vet ./internal/argmask/ ./modules/bonfire/ ./cmd/summer/ && grep -c 'strings.Join(argv' cmd/summer/runtime.go | grep -qx 0 && grep -q 'argmask.Join(argv)' cmd/summer/runtime.go</automated>
|
||||
</verify>
|
||||
<done>Two commits exist, WR-11 then WR-10, with no trailers. RED output is captured for both. internal/argmask is the only masking implementation, used by bonfire's Try line and by runAppCmd. A signal-killed or unstartable delegate's error never contains a secret value, and a signal death still exits 1. The API check shows no exported identifier removed or changed and none added under modules/. introduction.md and writing-commands.md describe the masking.</done>
|
||||
</task>
|
||||
|
||||
<task type="auto" tdd="true">
|
||||
<name>Task 3: lagoon WR-12 and WR-13 (two commits), then the records commit</name>
|
||||
<files>modules/lagoon/migrations.go, modules/lagoon/commands.go, modules/lagoon/migrations_test.go, modules/lagoon/example_test.go, modules/lagoon/pending_test.go, modules/lagoon/README.md, docs/console/setup-and-maintenance.md, docs/database/migrations.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</files>
|
||||
<precondition>Docker is reachable for testcontainers: `docker info` succeeds, or `sudo -u golem docker info` does. Otherwise stop: WR-12/WR-13 tests are Docker-only.</precondition>
|
||||
<behavior>
|
||||
- WR-12 (extend TestMigrateStatusJSON, Docker). After Migrate(gdb, [alpha]), delete the newest attach history row: the ID of the last entry of attach.Migrations, from the table HistoryTableName("summercms.attach"). That migration is now pending. Compute pending := PendingMigrations(gdb, plugins). For both arg variants, check three things. (1) The entries with applied false, in order, equal pending (plugin and id), each with Table equal to HistoryTableName(ref.Plugin). (2) An entry {summercms.attach, <deleted id>, applied false} is present. (3) The entries whose plugin starts with "demo." equal the old want list exactly. It fails on the unchanged code because the framework entry is missing.
|
||||
- WR-12 existing tests updated to the new Status contract (not RED tests). TestTwoPluginMigrationSetsIsolated (migrations_test.go) asserts the framework rows come first in migrationSets order (summercms.attach, summercms.cabana, DeferredHistoryID, QueueHistoryID). It then checks the demo.alpha and demo.beta rows by Plugin, not by index 0 and 1, with all existing ID assertions kept. TestDocsMigrate (example_test.go, package lagoon_test) asserts that the last line is the existing acme.blog want line and the lines before it start with lagoon's framework set IDs. Leave the docs region code between `docs:start migrate` and `docs:end migrate` byte-identical, so docs/database/migrations.md needs no `summer docs:sync`.
|
||||
- WR-13 (new TestMigrateRereadFailureWarns, Docker, dedicatedDB "lagoon_reread_warns"). Use plugin demo.rereadfirst, whose one migration creates a table, then plugin demo.rereadlast, whose one migration runs `ALTER TABLE <HistoryTableName("demo.rereadfirst")> RENAME COLUMN id TO id_gone`. Plugin IDs allow only lowercase letters, digits and dots. Compute before := PendingMigrations on the fresh DB. Run runLagoon(t, "local", dsn, plugins, "migrate"). Expect exit 0, stdout containing "Could not re-read the migration history", "demo.rereadfirst: 1 applied", "demo.rereadlast: 1 applied" and fmt.Sprintf("Applied %d migrations", len(before)). On the unchanged code the run exits 1 and prints no applied lines.
|
||||
</behavior>
|
||||
<action>
|
||||
1. **WR-12 (commit 7).** Extend TestMigrateStatusJSON per behavior and run it RED with Docker. Fix per P-06 in modules/lagoon/migrations.go: Status calls migrationSets(gdb, plugins) and builds one StatusRow per set (Plugin = set id, Table = HistoryTableName(set id); IDs from the history table ordered by id, or none when the table is missing, exactly as today). The error label becomes `lagoon: status <set id>`. Update the doc comments of StatusRow ("one migration set's recorded IDs: Plugin is the plugin ID or the framework set's history ID") and Status (it reads the history of every set Migrate applies, framework sets first). In modules/lagoon/commands.go, update the migrate:status Long text: it lists the framework's own sets and each plugin with migrations, and --json prints every declared migration of those sets. Keep the Description and Examples. Keep the JSON loop; it is now correct, see P-06.
|
||||
|
||||
Update the existing tests per behavior: TestTwoPluginMigrationSetsIsolated and TestDocsMigrate. Grep `Status(` across modules/lagoon tests for any other caller.
|
||||
|
||||
Docs:
|
||||
- modules/lagoon/README.md line 138 (`lagoon.Status` row): it lists the applied IDs of every set Migrate applies, framework sets first, then plugins.
|
||||
- modules/lagoon/README.md line 261 and docs/console/setup-and-maintenance.md line 17: the table covers the framework sets and every plugin with migrations; --json prints every declared migration of those sets.
|
||||
- docs/database/migrations.md line 84: migrate:status prints the history table and applied IDs of the framework's sets and of each plugin.
|
||||
|
||||
Commit: `fix(lagoon): migrate:status reads the sets migrate applies (WR-12)`, paths migrations.go, commands.go, migrations_test.go, example_test.go, pending_test.go, README.md, docs/console/setup-and-maintenance.md, docs/database/migrations.md.
|
||||
|
||||
2. **WR-13 (commit 8).** Add TestMigrateRereadFailureWarns to modules/lagoon/pending_test.go per behavior and run it RED with Docker. Fix per P-07 in the migrate Run of modules/lagoon/commands.go. After the follow-up PendingMigrations, the applied list starts as the pre-run pending list. On a nil re-read error it becomes appliedSince(pending, after). On a re-read error with migrateErr nil, print the warning, which includes the sanitized error, and continue. When both fail, return both errors joined. The per-set lines, the migrateErr return and the success line keep their current order.
|
||||
|
||||
Add a comment explaining why a failed read after a successful Migrate must not fail the run: the schema change is in place, and a deploy would roll back or alert. In modules/lagoon/README.md and docs/console/setup-and-maintenance.md (the migrate row, line 15), add one clause: if the history cannot be re-read after a successful run, migrate warns and counts every pending migration as applied. Add TestMigrateRereadFailureWarns to DOCKER_LAGOON_TESTS in scripts/check-phase19.8.sh, under `# quick 261022-4z8 (re-review fixes): WR-13.`
|
||||
|
||||
Commit: `fix(lagoon): warn instead of failing when the re-read after a successful migrate fails (WR-13)`, paths commands.go, pending_test.go, README.md, docs/console/setup-and-maintenance.md, scripts/check-phase19.8.sh.
|
||||
|
||||
3. **Full verification** (before the records commit). Run the commands in the plan-level verification section: vet, the full Docker test run, the hello workspace, the gate's self-test, go and docker stages, the API check and gofmt on the touched Go files. Fix any failure in the commit of the finding it belongs to; amend only if that commit is still the newest, otherwise add a follow-up `fix(...)` commit and record it. Apply the foreign-failure rule from the context section if it applies.
|
||||
|
||||
4. **Records commit (commit 9, docs only).** Edit .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md:
|
||||
- In the frontmatter `findings`, set disposition `fixed` for WR-09, WR-10, WR-11, WR-12, WR-13, WR-14, IN-14 and IN-15, and `deferred` for IN-10, IN-11, IN-12 and IN-13. Set `open: 0` and keep `total: 32`.
|
||||
- In the table, set the same dispositions. Each fixed row's Source cell is `fix <short hash> (quick 261022-4z8)`, matching the existing `fix b077dc3 (19.8-05)` style; IN-14's says `docs <hash> (quick 261022-4z8)`. The WR-10 cell adds `; the exit-130 suggestion is deferred to todo .planning/todos/pending/2026-10-22-console-review-info-findings.md`. IN-10..IN-13 cells read `todo .planning/todos/pending/2026-10-22-console-review-info-findings.md`.
|
||||
- Keep the WR-05, WR-07 and WR-08 dispositions `fixed` and append to their Source cells `; remainder WR-14 (fix <hash>)`, `; remainder WR-12 (fix <hash>)` and `; remainder WR-11 (fix <hash>)` respectively. Leave every other row unchanged.
|
||||
|
||||
Edit .planning/todos/pending/2026-10-22-console-review-info-findings.md:
|
||||
- Append sections `## IN-10`, `## IN-11`, `## IN-12` and `## IN-13`, each with File, Problem and Suggested fix taken from 19.8-REVIEW.md (locate code by symbol, as the existing sections do).
|
||||
- Append a section `## WR-10 follow-up: exit code 128+n for a delegate killed by a signal`. It records that plan 05 specified exit 1, which this quick task kept, and gives the review's suggested mapping: SIGINT/SIGTERM as already reported with 128+n, other signals 128+n with the masked block. It also notes that docs/console/introduction.md documents exit 1 and must change with it.
|
||||
- Update the frontmatter: title "Console review info findings (IN-01..IN-13) and the delegate signal exit code"; add modules/bonfire/live.go, modules/bonfire/output.go and cmd/summer/runtime.go to `files`; leave `g15office: pending`.
|
||||
- Extend the Context paragraph with one sentence naming the re-review and this quick task.
|
||||
|
||||
Commit: `docs(19.8): re-review fixes recorded, IN-10..IN-13 and the exit-130 suggestion deferred`, paths the two files only.
|
||||
|
||||
5. Write the SUMMARY (261022-4z8-SUMMARY.md in this quick directory). Model it on 19.8-05-SUMMARY.md and include: plan_head_before (BASE) and plan_head_after; a commits-per-finding table; the RED output per finding; the verification table; the API check; decisions P-01..P-09 as applied, with any deviation; the release-note input (P-09); skipped doc updates, if any; and a suggested g15office comment. Do not commit the SUMMARY or PLAN and do not touch STATE.md; the orchestrator does that. Do not delete the gocache; the orchestrator removes it at the end.
|
||||
</action>
|
||||
<verify>
|
||||
<automated>env GOCACHE=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./modules/lagoon/ -run 'TestMigrateStatusJSON|TestMigrateRereadFailureWarns|TestMigrateAppliedLines|TestPendingMigrations|TestTwoPluginMigrationSetsIsolated|TestDocsMigrate|TestMigratePretend' && grep -v '^\s*//' modules/lagoon/migrations.go | grep -c 'migrationSets(gdb, plugins)' | grep -qx 3 && grep -q '^open: 0$' .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md && grep -q '^## IN-13' .planning/todos/pending/2026-10-22-console-review-info-findings.md</automated>
|
||||
</verify>
|
||||
<done>Commits 7 (WR-12), 8 (WR-13) and 9 (records) exist, with no trailers and only their listed paths. The Docker-backed lagoon tests pass, with RED output captured for WR-12 and WR-13. migrate:status --json lists pending framework migrations. A failed re-read after a successful migrate exits 0 with a warning. The disposition shows open: 0 with hashes in the Source cells. The todo carries IN-10..IN-13 and the exit-130 follow-up. The full verification passes. The SUMMARY is written and not committed.</done>
|
||||
</task>
|
||||
|
||||
</tasks>
|
||||
|
||||
<threat_model>
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description |
|
||||
|----------|-------------|
|
||||
| command line → stderr/CI logs | argv typed by an operator (passwords, tokens) is echoed in hints and errors that CI and log collectors capture |
|
||||
| untrusted text → terminal | item names (remote file names, admin-typed titles, slugs) reach the terminal through Process lines |
|
||||
| database → deploy pipeline | migrate's exit code and applied lines drive deploy gates and rollbacks |
|
||||
|
||||
## STRIDE Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|
||||
|-----------|----------|-----------|----------|-------------|-----------------|
|
||||
| T-4z8-01 | Information disclosure | modules/bonfire/errors.go tryArgs (WR-11) | high | mitigate | Mask the argument after a secret-named long flag whatever its first character, except `--`; pinned by the dash cases in TestWrongDirectoryMasksSecrets |
|
||||
| T-4z8-02 | Information disclosure | cmd/summer/runtime.go runAppCmd (WR-10) | high | mitigate | Error built from internal/argmask.Join(argv) on every branch; TestRunAppCmdMasksSecrets (signal death, start failure) and the masked TestDelegateExitCodePassthrough subtest |
|
||||
| T-4z8-03 | Tampering | modules/bonfire/helpers.go Process plain path, processModel operation label (WR-14) | medium | mitigate | Sanitize(item) and Sanitize(op); plain-path cases in TestProcessViewStripsControl |
|
||||
| T-4z8-04 | Repudiation | lagoon migrate:status --json (WR-12) | medium | mitigate | Status reads migrationSets, so a deploy gate on "applied": false sees pending framework migrations; TestMigrateStatusJSON |
|
||||
| T-4z8-05 | Denial of service (ops) | lagoon migrate re-read (WR-13) | medium | mitigate | A successful Migrate never exits 1 because of a failed read; warning printed; TestMigrateRereadFailureWarns |
|
||||
| T-4z8-06 | Repudiation | bonfire live views (WR-09) | low | mitigate | Lines after a cancellation print plainly instead of being dropped while reported as printed; TestPrintAboveAfterCancel |
|
||||
| T-4z8-07 | Denial of service | runLive on the losing path (IN-15) | low | mitigate | No program, child context or TEA_TRACE fd created when the slot is taken; TestNestedProcessBuildsNoProgram |
|
||||
| T-4z8-08 | Information disclosure | IN-08 C1 bytes through Sanitize (deferred earlier) | low | accept | Still deferred in the todo; out of this task's scope |
|
||||
| T-4z8-SC | Tampering | npm/pip/cargo installs | high | accept | No package installs and no new Go module dependency in this plan (internal/argmask is stdlib-only) |
|
||||
</threat_model>
|
||||
|
||||
<verification>
|
||||
Run after the WR-13 commit, before the records commit. Write `$GC` out in full; it is the gocache path from the context section.
|
||||
|
||||
1. `env GOCACHE=$GC go vet ./...`, which must pass.
|
||||
2. `env GOCACHE=$GC TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...`, which must pass. Rerun the wristband flake once; apply the foreign-failure rule to cmd/summer docs tests.
|
||||
3. `env GOCACHE=$GC go test -count=3 -race ./modules/bonfire/ ./modules/diary/`, which must pass.
|
||||
4. `env GOCACHE=$GC go -C examples/hello test -count=1 ./...` (the separate workspace module), which must pass.
|
||||
5. `env GOCACHE=$GC bash scripts/check-phase19.8.sh self-test`, then `env GOCACHE=$GC bash scripts/check-phase19.8.sh go`, `env GOCACHE=$GC bash scripts/check-phase19.8.sh docs` and `env GOCACHE=$GC TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 bash scripts/check-phase19.8.sh docker`, which must pass. Do not run the plugins, apps or landing stages; they touch sibling repos.
|
||||
6. API check (no exported break). Replace BASE with the recorded hash and run this. S is the session scratchpad, so the repo gets no stray files:
|
||||
`bash -c 'S=/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad; d() { git diff BASE..HEAD -U0 -- "$@" ":(exclude)*_test.go"; }; X="^(func (\([^)]*\) )?[A-Z]|type [A-Z]|var [A-Z]|const [A-Z])"; d modules cmd internal | grep -E "^-(func|type|var|const) " | sed "s/^-//" | sort > $S/R; d modules cmd internal | grep -E "^\+(func|type|var|const) " | sed "s/^+//" | sort > $S/A; d modules | grep -E "^\+(func|type|var|const) " | sed "s/^+//" | sort > $S/AM; echo removed-or-changed:; comm -23 $S/R $S/A | grep -E "$X"; echo added-under-modules:; comm -13 $S/R $S/AM | grep -E "$X"; rm -f $S/R $S/A $S/AM'`
|
||||
Both lists must be empty. internal/argmask's exported Join and Quote live in an internal package, so they are not framework API and do not count.
|
||||
7. `gofmt -l` on every Go file this task touched must print nothing.
|
||||
8. Write `git log --format='%s%n%b' BASE..HEAD` to a scratchpad file, check that git exited 0, then `grep -ci 'co-authored-by'` on that file must print 0. `git log --format=%s BASE..HEAD` must show eight finding subjects ending in (IN-14), (WR-14), (IN-15), (WR-09), (WR-11), (WR-10), (WR-12) and (WR-13), plus the docs(19.8) records commit.
|
||||
9. `git status --short` must show no change of ours left unstaged and no staged foreign file; foreign dirty files (STATE.md, state.json, .msd/, other sessions' edits) are left as they were.
|
||||
</verification>
|
||||
|
||||
<success_criteria>
|
||||
- All three verification gaps of 19.8 are closed by code with pinning tests: gap 1 by WR-09, gap 2 by WR-11 and WR-10, gap 3 by WR-12. WR-13, WR-14, IN-14 and IN-15 are fixed too.
|
||||
- Nine local commits: eight findings, each with its test and docs, and one records commit. No trailers, no push, no tag, no changelog post.
|
||||
- `go vet ./...` and `go test ./...` (with Docker) pass, as do the bonfire and diary race runs, the hello workspace, and the gate's self-test, go, docs and docker stages.
|
||||
- No exported framework identifier is removed, added or changed. `lagoon.Status` keeps its signature and now returns the framework sets first.
|
||||
- The disposition record shows open: 0. IN-10..IN-13 and the exit-130 suggestion are in the todo.
|
||||
</success_criteria>
|
||||
|
||||
<output>
|
||||
Write `.planning/quick/261022-4z8-fix-the-open-19-8-re-review-findings-wr-/261022-4z8-SUMMARY.md` when done (not committed by the executor).
|
||||
|
||||
For the orchestrator after execution:
|
||||
- Commit PLAN and SUMMARY per the quick workflow, in a separate docs commit. Do not touch `.planning/STATE.md` or `state.json`; the user forbade it for this run.
|
||||
- Delete the gocache directory `/tmp/claude-1000/-media-nvme-dev-golem15-summercms-io-summercms-summercms-go/e016575e-9db1-44e1-a366-55d9ae1a9f8b/scratchpad/gocache` after the last go command (executor and any verifier).
|
||||
- Phase 19.8 can now be re-verified to clear its `gaps_found` status. Releasing the fixes (v0.4.x) needs the user's go-ahead.
|
||||
</output>
|
||||
@@ -0,0 +1,258 @@
|
||||
---
|
||||
phase: quick-261022-4z8
|
||||
plan: 01
|
||||
quick_id: 261022-4z8
|
||||
subsystem: console
|
||||
tags: [review-fixes, bonfire, bubbletea, live-views, secrets, summer, delegates, lagoon, migrations, gate]
|
||||
status: complete
|
||||
gap_closure: true
|
||||
|
||||
requires:
|
||||
- phase: 19.8-05
|
||||
provides: the review fixes released as v0.4.2 and their re-review (19.8-REVIEW.md, 19.8-VERIFICATION.md gaps 1-3)
|
||||
provides:
|
||||
- "WR-09..WR-14, IN-14 and IN-15 fixed, one commit each, each code fix with a pinning test proven red on the unchanged code"
|
||||
- "internal/argmask: one secret-masking implementation shared by bonfire's Try line and summer's delegate errors"
|
||||
- "lagoon.Status / migrate:status list the framework sets first (same list as Migrate and PendingMigrations)"
|
||||
- "scripts/check-phase19.8.sh requires the 4 new pinning tests by name"
|
||||
- "19.8-REVIEW-DISPOSITION.md at open: 0; IN-10..IN-13 and the exit-130 suggestion in the console review todo"
|
||||
affects: [phase 19.8 re-verification (gaps_found), next v0.4.x release, bonfire live views, summer delegates, lagoon migrate]
|
||||
|
||||
actuals:
|
||||
tokens: 12206 # chars/4 over the added and removed lines of this task's diff (48825 chars)
|
||||
tasks: 3
|
||||
commits: 9 # MEASURED: git rev-list --count plan_head_before..plan_head_after
|
||||
plan_head_before: a28957f7d0bc6dfe17b6b79fd7b5aada3d222546
|
||||
plan_head_after: a48afbd11e648ba7024c687a18cce3bede24bf51
|
||||
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "Live view lifetime decoupled from the caller's context: a watcher ends and unregisters the view under liveMu on ctx cancellation"
|
||||
- "Claim the single live slot before building the bubbletea program"
|
||||
- "printAbove refuses a view whose Run has ended (liveView.done)"
|
||||
- "Framework-internal helper package (internal/argmask) shared by a module and the summer CLI instead of a new exported API"
|
||||
- "Counting context (AfterFunc) to observe child-context registrations in a test"
|
||||
|
||||
key-files:
|
||||
created:
|
||||
- internal/argmask/argmask.go
|
||||
modified:
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/helpers.go
|
||||
- modules/bonfire/live.go
|
||||
- modules/bonfire/live_test.go
|
||||
- modules/bonfire/errors_test.go
|
||||
- modules/bonfire/README.md
|
||||
- cmd/summer/runtime.go
|
||||
- cmd/summer/runtime_test.go
|
||||
- modules/lagoon/migrations.go
|
||||
- modules/lagoon/commands.go
|
||||
- modules/lagoon/migrations_test.go
|
||||
- modules/lagoon/example_test.go
|
||||
- modules/lagoon/pending_test.go
|
||||
- modules/lagoon/README.md
|
||||
- docs/console/writing-commands.md
|
||||
- docs/console/introduction.md
|
||||
- docs/console/setup-and-maintenance.md
|
||||
- docs/database/migrations.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
|
||||
|
||||
key-decisions:
|
||||
- "P-01 applied as planned: the live program no longer takes the caller's context; watchContext ends the view with its last frame and unregisters it under liveMu; printAbove checks liveView.done. The review's fallback (check-then-Send) was not needed: the design passes -race -count=3."
|
||||
- "P-05 applied: the masking moved to internal/argmask (Join, Quote); no exported framework identifier added, removed or changed."
|
||||
- "P-06 applied: lagoon.Status iterates migrationSets; a plugin whose HasMigrations returns none no longer gets an empty row."
|
||||
- "P-07 applied: a failed re-read after a successful Migrate warns and exits 0, counting the pre-run pending list as applied."
|
||||
|
||||
metrics:
|
||||
duration: "~20 min"
|
||||
completed: 2026-10-22
|
||||
---
|
||||
|
||||
# Quick 261022-4z8: the open 19.8 re-review findings Summary
|
||||
|
||||
**Cancelled live views now end with their last frame and print later lines plainly, delegate errors and Try lines mask every secret flag value (dash-leading included) through one shared `internal/argmask`, and `migrate:status` lists the framework sets that `migrate` applies. Eight review findings were fixed in eight commits, plus one records commit.**
|
||||
|
||||
This closes the three verification gaps of phase 19.8: gap 1 by WR-09, gap 2 by WR-11 and WR-10, gap 3 by WR-12. WR-13, WR-14, IN-14 and IN-15 are fixed as well. IN-10..IN-13 and WR-10's exit-130 suggestion are deferred to the existing todo.
|
||||
|
||||
## Commits per finding
|
||||
|
||||
| # | Finding | Commit | Subject | Pinning test |
|
||||
|---|---------|--------|---------|--------------|
|
||||
| 1 | IN-14 | 7c15095 | docs(bonfire): the ErrReported comment names the WithExitCode code (IN-14) | none (doc comment only) |
|
||||
| 2 | WR-14 | 48e13f9 | fix(bonfire): strip control characters from Process's plain item lines and the view's label (WR-14) | TestProcessViewStripsControl (extended: operation label, two plain-path subtests) |
|
||||
| 3 | IN-15 | 1ac75ad | fix(bonfire): claim the live view slot before building the program (IN-15) | TestNestedProcessBuildsNoProgram (new) |
|
||||
| 4 | WR-09 | fce232d | fix(bonfire): end a live view when its context is cancelled and print later lines plainly (WR-09) | TestPrintAboveAfterCancel (new) |
|
||||
| 5 | WR-11 | 707372b | fix(bonfire): mask secret flag values that start with a dash (WR-11) | TestWrongDirectoryMasksSecrets (extended) |
|
||||
| 6 | WR-10 | 17c722d | fix(summer): mask secret flag values in a delegate's error (WR-10) | TestRunAppCmdMasksSecrets (new), TestDelegateExitCodePassthrough (new subtest) |
|
||||
| 7 | WR-12 | 57a849d | fix(lagoon): migrate:status reads the sets migrate applies (WR-12) | TestMigrateStatusJSON (extended); TestTwoPluginMigrationSetsIsolated and TestDocsMigrate updated to the new contract |
|
||||
| 8 | WR-13 | 0691d26 | fix(lagoon): warn instead of failing when the re-read after a successful migrate fails (WR-13) | TestMigrateRereadFailureWarns (new, Docker) |
|
||||
| 9 | records | a48afbd | docs(19.8): re-review fixes recorded, IN-10..IN-13 and the exit-130 suggestion deferred | n/a |
|
||||
|
||||
No commit carries a Co-Authored-By trailer (`grep -ci co-authored-by` over `git log --format='%s%n%b' BASE..HEAD` prints 0). Nothing was pushed or tagged, and nothing was posted to the changelog.
|
||||
|
||||
## RED output on the unchanged code
|
||||
|
||||
**WR-14** (TestProcessViewStripsControl):
|
||||
```
|
||||
--- FAIL: TestProcessViewStripsControl (0.00s)
|
||||
live_test.go:546: process view carries "\x1b]0;pwned\a": "... Push\x1b]0;pwned\aing 1/2 ..."
|
||||
--- FAIL: TestProcessViewStripsControl/terminal_with_-n (0.00s)
|
||||
live_test.go:577: plain Process line carries "\x1b]0;pwned\a": "\x1b[32m✓\x1b[m evil\x1b]0;pwned\aitem\n"
|
||||
--- FAIL: TestProcessViewStripsControl/no_terminal,_color_forced (0.00s)
|
||||
live_test.go:577: plain Process line carries "\x1b]0;pwned\a": "\x1b[32m✓\x1b[m evil\x1b]0;pwned\aitem\n"
|
||||
```
|
||||
|
||||
**IN-15** (TestNestedProcessBuildsNoProgram):
|
||||
```
|
||||
--- FAIL: TestNestedProcessBuildsNoProgram (0.00s)
|
||||
live_test.go:649: the nested Process built a program: the TEA_TRACE file exists
|
||||
live_test.go:652: 1 child contexts still registered on the caller's context, want 0
|
||||
```
|
||||
|
||||
**WR-09** (TestPrintAboveAfterCancel). It fails on the 2s poll, not by hanging:
|
||||
```
|
||||
--- FAIL: TestPrintAboveAfterCancel (2.00s)
|
||||
live_test.go:686: view still registered 2s after the cancellation
|
||||
live_test.go:691: PrintAbove reported true after the cancellation
|
||||
live_test.go:708: "after-cancel-println" printed 0 times, want 1:
|
||||
live_test.go:708: "after-cancel-info" printed 0 times, want 1:
|
||||
live_test.go:708: "after-cancel-printabove" printed 0 times, want 1:
|
||||
```
|
||||
|
||||
**WR-11** (TestWrongDirectoryMasksSecrets):
|
||||
```
|
||||
["sync" "--token" "--force"]: Try = "cd ..; {bin} sync --token --force", want "cd ..; {bin} sync --token ***"
|
||||
["admin:create" "--email" "a@b.c" "--password" "-Xk9!q"]: Try = "... --password '-Xk9!q'", want "... --password ***"
|
||||
["admin:create" ... "-Xk9!q"]: Try line repeats "-Xk9"
|
||||
["sync" "--client-secret" "-s3cr3t" "--verbose"]: Try = "... --client-secret -s3cr3t --verbose", want "... --client-secret *** --verbose"
|
||||
```
|
||||
|
||||
**WR-10** (TestDelegateExitCodePassthrough, TestRunAppCmdMasksSecrets):
|
||||
```
|
||||
--- FAIL: TestDelegateExitCodePassthrough/killed_by_a_signal_masks_secrets
|
||||
want one error block with the password masked, got 1:
|
||||
stderr "\n ERROR summer: migrate --password hunter2: signal: killed\n\n Run with -v for details.\n\n"
|
||||
the password reached the output
|
||||
--- FAIL: TestRunAppCmdMasksSecrets/killed_by_a_signal
|
||||
error lacks "--password ***": summer: admin:create --email a@b.c --password hunter2 --api-token=t0k3n --client-secret -Xk9q: signal: killed
|
||||
error repeats "hunter2" / "t0k3n" / "-Xk9q"
|
||||
--- FAIL: TestRunAppCmdMasksSecrets/start_failure
|
||||
error lacks "--password ***": summer: admin:create ... --password hunter2 ...: fork/exec .../missing: no such file or directory
|
||||
```
|
||||
|
||||
**WR-12** (TestMigrateStatusJSON, Docker):
|
||||
```
|
||||
--- FAIL: TestMigrateStatusJSON (0.38s)
|
||||
pending_test.go:318: [migrate:status --json]: pending entries = [{demo.beta ... 202610210001_create_pending_beta} {demo.beta ... 202610210002_seed_pending_beta}],
|
||||
want PendingMigrations' [{summercms.attach summer_migrations_summercms_attach 202609180001_create_system_files ...} ...]
|
||||
pending_test.go:321: [migrate:status --json]: the pending framework migration 202609180001_create_system_files is not listed
|
||||
(same two failures for [migrate:status --json -q])
|
||||
```
|
||||
|
||||
**WR-13** (TestMigrateRereadFailureWarns, Docker):
|
||||
```
|
||||
--- FAIL: TestMigrateRereadFailureWarns (0.42s)
|
||||
pending_test.go:428: migrate: exit 1, want 0
|
||||
ERROR lagoon: pending demo.rereadfirst: ERROR: column "id" does not exist (SQLSTATE 42703)
|
||||
```
|
||||
|
||||
## Verification
|
||||
|
||||
| Check | Result |
|
||||
|-------|--------|
|
||||
| `go vet ./...` | pass |
|
||||
| `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...` (Docker suite) | pass, 43 packages ok, exit 0. The wristband flake did not occur. |
|
||||
| `go test -count=3 -race ./modules/bonfire/ ./modules/diary/` | pass (bonfire 15.6s, diary 2.2s) |
|
||||
| Task 1 `-race -count=3` live-view set (TestPrintAboveAfterCancel, TestNestedProcessBuildsNoProgram, TestProcessViewStripsControl, TestPrintAboveOrderedNoLoss, TestPrintlnStaysWithItsConsole, TestLiveViewSingleOwner, TestLiveViewPanicUnregisters, TestLiveViewRestoresTerminalOnSignal, TestPrintAbove, TestLiveModels) | pass |
|
||||
| `go -C examples/hello test -count=1 ./...` | pass |
|
||||
| `scripts/check-phase19.8.sh self-test` | pass |
|
||||
| `scripts/check-phase19.8.sh go` | pass (bonfire list now 44 tests, SUMMER list 7) |
|
||||
| `scripts/check-phase19.8.sh docs` | pass |
|
||||
| `scripts/check-phase19.8.sh docker` | pass (lagoon list now 7 tests, TestMigrateRereadFailureWarns included) |
|
||||
| Task verify greps | `WithContext(` outside comments in live.go: 0; `strings.Join(argv` in runtime.go: 0; `argmask.Join(argv)` present; `migrationSets(gdb, plugins)` outside comments in migrations.go: 3; `open: 0`; `## IN-13` in the todo |
|
||||
| gofmt on touched Go files | Clean in every hunk this task wrote. `gofmt -l` still lists `cmd/summer/runtime.go` and `cmd/summer/runtime_test.go`, but they were already unformatted at BASE (a struct field alignment at runtime.go:27 and a map alignment at runtime_test.go:81, both untouched by this task). Out of scope, not fixed. |
|
||||
| `git status --short` | Only foreign files remain (STATE.md, state.json, .msd/, two .gitkeep files, this quick dir). Nothing of ours is unstaged, and nothing foreign was staged. |
|
||||
|
||||
The plugins, apps and landing stages of the gate were not run, as the plan says (they touch sibling repos).
|
||||
|
||||
## API check (no exported break)
|
||||
|
||||
`git diff BASE..HEAD -U0 -- modules cmd internal ':(exclude)*_test.go'`:
|
||||
|
||||
- removed-or-changed exported: **(empty)**
|
||||
- added under modules/ exported: **(empty)**
|
||||
|
||||
Every changed declaration is unexported: bonfire's `secretWords`, `secretFlag` and `shellQuote` were removed (moved), and `watchContext` was added. `argmask.Join` and `argmask.Quote` are exported but sit in `internal/`, so they are not framework API. `lagoon.Status` keeps its signature, and `lagoon.StatusRow` keeps its fields.
|
||||
|
||||
## Decisions as applied
|
||||
|
||||
- **P-01 (WR-09):** Applied as designed. `tea.WithContext` was removed from `tea.NewProgram`, and `liveView` gained `done <-chan struct{}`, which is runLive's `finished` channel, created before the claim. The new unexported `watchContext(ctx, view, finished)` holds the watcher. On `ctx.Done()` it unregisters the view, sends `finishMsg{err: ctx.Err()}` and waits for `finished`, all under `liveMu`. On `finished` it only unregisters. In both branches it clears `activeView` only if the view is still the active one. The deferred cleanup now also waits for the watcher (`stopWatch()`) after releasing `liveMu`, so no goroutine outlives `runLive`. `printAbove` reads `activeView` into `v` and, after the owner check, does a non-blocking `case <-v.done:` that unregisters and returns false. watchSignals is unchanged. The fallback was not needed.
|
||||
- **P-02 (IN-15):** Applied. `finished` is created before the lock, and the program is built under `liveMu` after the slot check.
|
||||
- **P-03 (WR-14):** Applied at the call sites: `out.Success(Sanitize(item))` and `op := Sanitize(m.operation)`. `console.status` is unchanged.
|
||||
- **P-04 (WR-11):** Applied. The `sync --token --force` case now expects `--token ***`, and the test comment explains why (pflag takes `--force` as the value).
|
||||
- **P-05 (WR-10):** Applied. `internal/argmask` provides `Join` and `Quote` and imports only `strings`. bonfire keeps `tryArgs` as a thin wrapper. `runAppCmd` uses `argmask.Join(argv)` on every branch, a signal death still exits 1, and runtime.go's `strings` import was dropped. modules/bonfire is the first module to import a package under `internal/`; Go's internal rule allows it, because both sit in the same module root.
|
||||
- **P-06 (WR-12):** Applied. The `migrate:status` Description and Examples are unchanged, and only the Long help changed. The goldens are untouched, since no golden carries the Long text. The `--json` loop in commands.go is unchanged.
|
||||
- **P-07 (WR-13):** Applied. A `switch` takes the re-read result: on success it uses `appliedSince`, when Migrate and the re-read both fail it returns `errors.Join`, and when only the re-read fails it warns (`Could not re-read the migration history: <sanitized error>`) and keeps the pre-run pending list.
|
||||
- **P-08:** Applied. TestNestedProcessBuildsNoProgram and TestPrintAboveAfterCancel joined the BONFIRE list, TestRunAppCmdMasksSecrets the SUMMER list, and TestMigrateRereadFailureWarns DOCKER_LAGOON_TESTS. Each joined in the same commit as its test.
|
||||
- **P-09:** `docs/setup/upgrading.md` was not changed.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
**1. [CLAUDE.md doc rule] The WR-10 commit also updates modules/bonfire/README.md**
|
||||
- **Found during:** Task 2, after the WR-10 commit.
|
||||
- **Issue:** CLAUDE.md says a change to a module's dependencies updates its README in the same commit. bonfire now imports `internal/argmask`, and the README's Dependencies section did not say so. The README's error-block paragraph also did not state the dash-leading masking rule.
|
||||
- **Fix:** Added `internal/argmask` to the README's Dependencies line, and "also a value that starts with `-`" to the Try-line masking sentence. Folded into the WR-10 commit with `git commit --amend -- modules/bonfire/README.md` while it was still the newest commit, and before any hash was recorded anywhere. The final WR-10 hash is 17c722d.
|
||||
- **Note:** The plan's WR-10 path list did not include the README. The WR-11 sentence in the README landed in the WR-10 commit rather than in the WR-11 commit; writing-commands.md got the WR-11 sentence in the WR-11 commit as planned.
|
||||
|
||||
**2. [Test detail] TestNestedProcessBuildsNoProgram checks the success line as "✓ x"**
|
||||
- The plan says the "x" success line must reach the buffer. A bare `"x"` substring check would be weak, so the test matches `symCheck+" x"` in the stripped output. The behaviour pinned is unchanged.
|
||||
|
||||
**3. [Test detail] TestMigrateStatusJSON's pending entries use the table from HistoryTableName(ref.Plugin)**
|
||||
- Implemented as planned. `attach.Migrations` holds one migration, `202609180001_create_system_files`, so "the newest attach row" is that one.
|
||||
|
||||
No other deviations. No finding was skipped.
|
||||
|
||||
## Skipped doc updates / follow-ups
|
||||
|
||||
- None skipped. None of the files this task edited had foreign uncommitted changes, which `git status --short -- <file>` checked before each edit.
|
||||
- Pre-existing gofmt drift in `cmd/summer/runtime.go:27`, `cmd/summer/runtime_test.go:81` and `cmd/summer/main.go`, plus several cabana, compass, flare and tide files. It is not caused by this task and was not fixed, per the scope boundary. It may be worth a `/msd-fast` gofmt pass when no other session is editing cabana.
|
||||
|
||||
## Release-note input (P-09, for whoever tags next)
|
||||
|
||||
- `lagoon.Status` and `migrate:status` (table and `--json`) now list the framework's own migration sets first (`summercms.attach`, `summercms.cabana`, deferred bindings, queue), then each plugin with migrations. `--json` now shows a pending framework migration as `"applied": false`. A plugin whose `Migrations()` returns none no longer gets an empty row.
|
||||
- `migrate` no longer exits 1 when the migrations succeeded but reading the history back failed. It prints a warning and counts the pending migrations as applied.
|
||||
- When the context of a `bonfire.Process` view is cancelled, the view ends with its last frame and later lines print plainly below it. Before, they were lost.
|
||||
- `bonfire.Process` strips control characters from item names on its plain path and from the operation label.
|
||||
- Secret masking now also covers values that start with `-` (`--password -Xk9`), and summer's error for a delegate killed by a signal, or failing to start, masks secret flag values. A signal death still exits 1.
|
||||
|
||||
## Suggested g15office comment
|
||||
|
||||
> 19.8 re-review fixes landed locally (quick 261022-4z8). There are nine commits on master, not pushed: WR-09..WR-14, IN-14 and IN-15 are fixed with pinning tests, and IN-10..IN-13 plus the exit-130 idea are deferred to the console review todo. The disposition is at open: 0. Next: re-verify phase 19.8 to clear gaps_found, then a v0.4.x release on the user's go-ahead.
|
||||
|
||||
## Known Stubs
|
||||
|
||||
None.
|
||||
|
||||
## Threat Flags
|
||||
|
||||
None. No new network endpoint, auth path or schema change was added. The threat register's mitigations T-4z8-01..07 are each pinned by the tests listed above.
|
||||
|
||||
## Self-Check: PASSED
|
||||
|
||||
- FOUND: internal/argmask/argmask.go
|
||||
- FOUND: 7c15095, 48e13f9, 1ac75ad, fce232d, 707372b, 17c722d, 57a849d, 0691d26, a48afbd (`git rev-list --count a28957f..a48afbd` = 9)
|
||||
|
||||
## Follow-up: WR-12 rework
|
||||
|
||||
Verification found that 57a849d changed what the exported `lagoon.Status` returns (framework sets first, no row for a plugin with no migrations). That broke `TestRollbackIsolatesFonotekaFullSchema` in `../fonoteka.go/parity`, so the plan's P-06 premise (no outside caller) was wrong. The orchestrator decided to keep `Status` as it was and to keep the WR-12 fix in the command only.
|
||||
|
||||
- **139f5e3** `fix(lagoon): keep Status plugin-only; migrate:status adds the framework sets itself (WR-12)`. `lagoon.Status` is back to its pre-57a849d behaviour: one row per plugin that implements `pact.HasMigrations`, in plugin order, with an empty row for a plugin with no migrations. `migrate:status` (table and `--json`) reads the new unexported `setStatus`, which takes its sets from `migrationSets`, so it still agrees with `migrate --pretend`. Both share the unexported `historyRows`. No exported API was added or changed. `TestTwoPluginMigrationSetsIsolated` and `TestDocsMigrate` are back to their pre-57a849d form. The new `TestStatusPluginRows` pins `Status`, and `TestMigrateStatusJSON` now also checks the table output. The new test is added to `scripts/check-phase19.8.sh`. The lagoon README and `docs/database/migrations.md` describe `Status` as plugin-only again.
|
||||
- **b1fba7e** `docs(19.8): ...`. The WR-12 disposition row names both 57a849d and 139f5e3.
|
||||
- **RED/GREEN.** In a scratch copy where the command calls the restored `Status` (that is, the command as it was before 57a849d), `TestMigrateStatusJSON` fails: the pending framework migration is missing from `--json`, and the table lists only the `demo.*` rows. In a scratch copy of the 57a849d code with the new tests, `TestStatusPluginRows`, `TestTwoPluginMigrationSetsIsolated` and `TestDocsMigrate` fail. All of them pass at 139f5e3.
|
||||
- **Checks.** `go vet ./...` passes, as do `summer docs:build --check` and `scripts/check-phase19.8.sh self-test`. `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./modules/lagoon/...` and the full Docker `go test -count=1 ./...` exit 0.
|
||||
- **fonoteka** (not modified, `replace => ../summercms.go`). `TestRollbackIsolatesFonotekaFullSchema` passes again. `TestMigrateSeedsCanonicalGenres` and `TestRollbackLastIsolatesFonoteka` still fail, and they did before this task: they now get past the row check (`status[0]` is `golem15.user`) but expect 11 user-plugin migrations, and the user plugin has 13.
|
||||
- **Release-note correction.** The first bullet of "Release-note input" above is superseded. `lagoon.Status` does not change. Only `migrate:status` (table and `--json`) now lists the framework's own sets first, and it lists only plugins that have migrations.
|
||||
|
||||
@@ -0,0 +1,168 @@
|
||||
---
|
||||
phase: quick-261022-4z8
|
||||
verified: 2026-10-22T04:30:00Z
|
||||
status: passed
|
||||
score: 12/12 must-haves verified (WR-12 resolved by follow-up 139f5e3)
|
||||
covered_files:
|
||||
- .planning/phases/19.8-console-commands-overhaul/19.8-REVIEW-DISPOSITION.md
|
||||
- .planning/quick/261022-4z8-fix-the-open-19-8-re-review-findings-wr-/261022-4z8-PLAN.md
|
||||
- .planning/quick/261022-4z8-fix-the-open-19-8-re-review-findings-wr-/261022-4z8-SUMMARY.md
|
||||
- .planning/todos/pending/2026-10-22-console-review-info-findings.md
|
||||
- cmd/summer/runtime.go
|
||||
- cmd/summer/runtime_test.go
|
||||
- docs/console/introduction.md
|
||||
- docs/console/setup-and-maintenance.md
|
||||
- docs/console/writing-commands.md
|
||||
- docs/database/migrations.md
|
||||
- internal/argmask/argmask.go
|
||||
- modules/bonfire/README.md
|
||||
- modules/bonfire/errors.go
|
||||
- modules/bonfire/errors_test.go
|
||||
- modules/bonfire/helpers.go
|
||||
- modules/bonfire/live.go
|
||||
- modules/bonfire/live_test.go
|
||||
- modules/lagoon/README.md
|
||||
- modules/lagoon/commands.go
|
||||
- modules/lagoon/example_test.go
|
||||
- modules/lagoon/migrations.go
|
||||
- modules/lagoon/migrations_test.go
|
||||
- modules/lagoon/pending_test.go
|
||||
- scripts/check-phase19.8.sh
|
||||
covered_digest: "v2:sha256:a7e6e0cc40bcaa62cc411ab11ce6be4f7002259775e1e01c5e30862d9a068518"
|
||||
behavior_unverified: 0
|
||||
overrides_applied: 0
|
||||
human_verification:
|
||||
- test: "Decide whether the lagoon.Status contract change (WR-12) is acceptable as non-breaking, given that a sibling consumer depends on the old row set"
|
||||
expected: "Either (a) accept it: update fonoteka.go/parity (withoutSharedPlugins or the index assertions) to the new framework-first rows, and record lagoon.Status's changed result in the next release notes / docs/setup/upgrading.md as a behaviour change; or (b) reject it: keep lagoon.Status plugin-only and add the framework sets only in migrate:status"
|
||||
why_human: "The function signature and StatusRow fields are unchanged, so no compile-level API break exists; but the result set changed and the plan's P-06 premise ('No consumer outside this repo calls lagoon.Status') is false. fonoteka.go/parity calls lagoon.Status in three tests and asserts len(status)==2 and status[0].Plugin=='golem15.user'. Verified: TestRollbackIsolatesFonotekaFullSchema passes against summercms.go at a28957f and fails against a48afbd (status now starts with summercms.attach, summercms.cabana, summercms.deferred, summercms.conga). Whether that counts as an 'exported API break' under the task's constraint is a policy decision."
|
||||
---
|
||||
|
||||
# Quick 261022-4z8: the open 19.8 re-review findings Verification Report
|
||||
|
||||
**Task goal:** Fix the open findings of the 19.8 code re-review (WR-09, WR-10, WR-11, WR-12, WR-13, WR-14, IN-14, IN-15). The constraints are one commit per finding, each with a test that fails without the fix; no exported API breaks; `go vet` and the Docker `go test ./...` pass; the disposition record and the info todo are updated; local commits only.
|
||||
**Verified:** 2026-10-22T04:30:00Z
|
||||
**Status:** human_needed
|
||||
**Re-verification:** No (initial verification)
|
||||
|
||||
Every RED claim in SUMMARY.md was replayed independently. For each finding the parent of the fix commit was exported with `git archive` to the scratchpad, the test file(s) from the fix commit were overlaid, and the named test was run. The same test was then run on the fix commit itself. Every finding was red before its fix and green after it.
|
||||
|
||||
## Goal Achievement
|
||||
|
||||
### Observable Truths
|
||||
|
||||
| # | Truth | Status | Evidence |
|
||||
|---|-------|--------|----------|
|
||||
| 1 | WR-09: cancelling Process's ctx ends the view with its last frame, unregisters it without waiting for work; later Println/Info/PrintAbove print plainly (PrintAbove false), none lost; printAbove refuses a program whose Run ended on its own | ✓ VERIFIED | live.go: `tea.WithContext` removed (no non-comment `WithContext(`); `watchContext` unregisters, sends `finishMsg{ctx.Err()}` and waits for `finished` under liveMu; `liveView.done`; `printAbove` has `case <-v.done:`. TestPrintAboveAfterCancel: RED at fce232d^ (the view was still registered after 2s, PrintAbove returned true, and all three lines were printed 0 times), GREEN at fce232d. `-race -count=10` on the live-view set passes. bubbletea v2.1.0 `Send` selects on `p.ctx.Done()` and `Run` does `defer p.cancel()`, so the deferred cleanup's Send after the watcher finishes cannot block. The "ended on its own" branch was confirmed by a throwaway verifier probe in a scratch copy (activeView with a closed `done`: printAbove returns false and unregisters). No committed test pins that branch (Info). |
|
||||
| 2 | WR-14: Process's plain path prints item names through Sanitize; the view's operation label is sanitized | ✓ VERIFIED | helpers.go:183 `out.Success(Sanitize(item))`; live.go:313 `op := Sanitize(m.operation)`. TestProcessViewStripsControl: RED at 48e13f9^ (label, both plain-path subtests), GREEN at 48e13f9. |
|
||||
| 3 | IN-15: runLive claims the slot before building the program; the loser builds none (no child ctx, no TEA_TRACE file) | ✓ VERIFIED | live.go: the `activeView != nil` check under liveMu comes before `tea.NewProgram`. TestNestedProcessBuildsNoProgram: RED at 1ac75ad^ (the trace file existed and 1 child ctx stayed registered), GREEN at 1ac75ad. |
|
||||
| 4 | IN-14: the ErrReported doc comment names the WithExitCode code | ✓ VERIFIED | errors.go:74-76. The `var ErrReported` line is unchanged. This finding only changes a doc comment, so no test is possible. |
|
||||
| 5 | WR-11: the Try line masks the argument after a secret-named long flag whatever its first character; only `--` is left alone | ✓ VERIFIED | argmask.go `args[i+1] != "--"`. TestWrongDirectoryMasksSecrets: RED at 707372b^ (`-Xk9!q` and `-s3cr3t` in clear text), GREEN at 707372b. |
|
||||
| 6 | WR-10: the error for a signal-killed or unstartable delegate masks argv through the shared internal/argmask; exit 1 is kept; no exported identifier changes | ✓ VERIFIED | runtime.go:174 `argmask.Join(argv)` wraps every branch, and no `strings.Join(argv` remains. bonfire's tryArgs and cd line use `argmask.Join` and `argmask.Quote`; bonfire's own secretWords, secretFlag and shellQuote are gone, so only one implementation exists. TestRunAppCmdMasksSecrets and TestDelegateExitCodePassthrough/killed_by_a_signal_masks_secrets: RED at 17c722d^ (hunter2, t0k3n and -Xk9q in the error), GREEN at 17c722d. |
|
||||
| 7 | WR-12: lagoon.Status reads migrationSets (framework sets first); --json lists every pending migration including framework ones; the signature and StatusRow are unchanged | ✓ VERIFIED | migrations.go:150. `migrationSets(gdb, plugins)` appears 3 times outside comments (Migrate, Status, PendingMigrations). TestMigrateStatusJSON (Docker): RED at 57a849d^ (pending `summercms.attach 202609180001_create_system_files` missing), GREEN at 57a849d. See truth 12 for the consumer impact. |
|
||||
| 8 | WR-13: when Migrate succeeds but the re-read fails, migrate warns, reports the pre-run pending list as applied and exits 0; when both fail, both errors are returned | ✓ VERIFIED | commands.go:177-192 `switch`: `err == nil` gives appliedSince; `migrateErr != nil` gives `errors.Join`; otherwise it warns with `Could not re-read the migration history`. TestMigrateRereadFailureWarns (Docker): RED at 0691d26^ (exit 1), GREEN at 0691d26. The both-fail branch is checked by reading the code only; it is a one-line return kept from before. |
|
||||
| 9 | One commit per finding with its pinning test and docs; no Co-Authored-By trailer; nothing pushed or tagged | ✓ VERIFIED | `git log a28957f..a48afbd` shows 9 commits: 8 finding commits (IN-14, WR-14, IN-15, WR-09, WR-11, WR-10, WR-12, WR-13) and 1 docs(19.8) records commit. `grep -ci co-authored` = 0. No tag points at or contains these commits. The branch is `ahead 16` of origin/master. Minor deviation: modules/bonfire/README.md's WR-11 masking sentence landed in the WR-10 commit (recorded in SUMMARY). |
|
||||
| 10 | The disposition marks WR-09..WR-14, IN-14 and IN-15 fixed with hashes (open: 0) plus the WR-05/07/08 remainders, and IN-10..IN-13 deferred; the todo carries IN-10..IN-13 and the exit-130 suggestion | ✓ VERIFIED | 19.8-REVIEW-DISPOSITION.md has `open: 0`, `total: 32`, and table rows 143-154 and 162-165 with the matching hashes. The todo has `## IN-10`..`## IN-13` and `## WR-10 follow-up: exit code 128+n …`, and its title and files are updated. |
|
||||
| 11 | `go vet ./...` and the Docker `go test ./...` pass | ✓ VERIFIED | `go vet ./...` passes. `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1 go test -count=1 ./...` exits 0 with 43 packages ok (lagoon 40s and lagoon/attach 54s, so the Docker tests ran). |
|
||||
| 12 | No exported API breaks | ? UNCERTAIN | The compile-level API is intact: the diff shows no exported func, type, var or const removed or changed, and none added under modules/ (argmask.Join and argmask.Quote are in internal/). However, **lagoon.Status's result changed** (framework rows first; a plugin with zero migrations no longer gets a row). The plan's P-06 premise that no outside consumer calls it is false: `fonoteka.go/parity/migrate_test.go` (3 calls) and `rollback_isolation_full_test.go` call it, and fonoteka uses `replace => ../summercms.go`. Reproduced: TestRollbackIsolatesFonotekaFullSchema PASSES against a28957f and FAILS against a48afbd (`status = [summercms.attach …]`, `len != 2`). Needs a human decision (see Human Verification). |
|
||||
|
||||
**Score:** 11/12 truths verified (0 present but behavior-unverified, 1 uncertain)
|
||||
|
||||
### Required Artifacts
|
||||
|
||||
| Artifact | Expected | Status | Details |
|
||||
|----------|----------|--------|---------|
|
||||
| `modules/bonfire/live.go` | slot claimed before NewProgram; watcher; liveView.done; printAbove refuses a finished program | ✓ VERIFIED | contains `case <-v.done:`; watchContext wired in runLive |
|
||||
| `modules/bonfire/helpers.go` | Sanitize on plain path | ✓ VERIFIED | `out.Success(Sanitize(item))` |
|
||||
| `modules/bonfire/errors.go` | ErrReported comment; tryArgs via argmask | ✓ VERIFIED | `argmask.Join`, `argmask.Quote` |
|
||||
| `internal/argmask/argmask.go` | Join + Quote, stdlib only | ✓ VERIFIED | imports only `strings`; used by bonfire and cmd/summer |
|
||||
| `cmd/summer/runtime.go` | runAppCmd uses argmask.Join(argv) | ✓ VERIFIED | line 174 |
|
||||
| `modules/lagoon/migrations.go` | Status from migrationSets, signature unchanged | ✓ VERIFIED | line 146 signature identical |
|
||||
| `modules/lagoon/commands.go` | re-read warning | ✓ VERIFIED | `Could not re-read the migration history` |
|
||||
| `modules/bonfire/live_test.go` | TestPrintAboveAfterCancel, TestNestedProcessBuildsNoProgram, plain-path cases | ✓ VERIFIED | present, red-before/green-after replayed |
|
||||
| `cmd/summer/runtime_test.go` | TestRunAppCmdMasksSecrets + masked signal subtest | ✓ VERIFIED | present, replayed |
|
||||
| `modules/lagoon/pending_test.go` | TestMigrateStatusJSON framework pending, TestMigrateRereadFailureWarns | ✓ VERIFIED | present, replayed with Docker |
|
||||
| `19.8-REVIEW-DISPOSITION.md` | open: 0 | ✓ VERIFIED | |
|
||||
|
||||
### Key Link Verification
|
||||
|
||||
| From | To | Via | Status | Details |
|
||||
|------|----|-----|--------|---------|
|
||||
| cmd/summer/runtime.go | internal/argmask | `argmask.Join(argv)` | WIRED | the only argv rendering in runAppCmd |
|
||||
| modules/bonfire/errors.go | internal/argmask | `argmask.Join` / `argmask.Quote` | WIRED | the old local helpers were deleted |
|
||||
| lagoon Migrate/Status/PendingMigrations | migrationSets | `migrationSets(gdb, plugins)` | WIRED | 3 call sites |
|
||||
| modules/bonfire/live.go | helpers.go Process | ctx → runLive → watchContext; printAbove `case <-v.done` | WIRED | |
|
||||
| helpers.go | Sanitize | `Sanitize(item)` | WIRED | |
|
||||
|
||||
### Behavioral Spot-Checks
|
||||
|
||||
| Behavior | Command | Result | Status |
|
||||
|----------|---------|--------|--------|
|
||||
| RED/GREEN replay per finding (WR-14, IN-15, WR-09, WR-11, WR-10) | `git archive <fix>^` + overlay the fix commit's test file(s), `go test -run <test>`; then the same at `<fix>` | all red before the fix, all green after | ✓ PASS |
|
||||
| RED/GREEN replay WR-12, WR-13 (Docker) | same, with `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1` | red before the fix (framework pending missing; exit 1), green after | ✓ PASS |
|
||||
| bonfire + diary under race | `go test -count=3 -race ./modules/bonfire/ ./modules/diary/` | ok | ✓ PASS |
|
||||
| live-view set stress | `go test -count=10 -race -run 'TestPrintAboveAfterCancel\|TestNestedProcessBuildsNoProgram\|TestPrintAboveOrderedNoLoss\|TestLiveViewSingleOwner\|TestLiveViewPanicUnregisters\|TestLiveViewRestoresTerminalOnSignal'` | ok (49s) | ✓ PASS |
|
||||
| printAbove ended-program branch | throwaway probe test in a scratch copy (not committed) | PASS | ✓ PASS |
|
||||
| hello workspace | `go -C examples/hello test -count=1 ./...` | ok | ✓ PASS |
|
||||
| Downstream consumer (fonoteka parity) | copies of fonoteka.go against summercms.go at a28957f and at a48afbd | TestRollbackIsolatesFonotekaFullSchema passes at base and fails at head; the other two Status tests already fail at base (user plugin migration count 13 vs 11, unrelated drift) and fail earlier at head | ✗ regression in a sibling repo (see truth 12) |
|
||||
|
||||
### Probe Execution
|
||||
|
||||
| Probe | Command | Result | Status |
|
||||
|-------|---------|--------|--------|
|
||||
| `scripts/check-phase19.8.sh self-test` | `bash scripts/check-phase19.8.sh self-test` | exit 0 | PASS |
|
||||
| `scripts/check-phase19.8.sh go` | `bash scripts/check-phase19.8.sh go` | exit 0 | PASS |
|
||||
| `scripts/check-phase19.8.sh docs` | `bash scripts/check-phase19.8.sh docs` | exit 0 | PASS |
|
||||
| `scripts/check-phase19.8.sh docker` | with `TESTCONTAINERS_HOST_OVERRIDE=127.0.0.1` | exit 0 | PASS |
|
||||
|
||||
### Requirements Coverage
|
||||
|
||||
| Requirement | Source Plan | Status | Evidence |
|
||||
|-------------|-------------|--------|----------|
|
||||
| WR-09 | 261022-4z8-PLAN | ✓ SATISFIED | truth 1 |
|
||||
| WR-10 | 261022-4z8-PLAN | ✓ SATISFIED | truth 6 |
|
||||
| WR-11 | 261022-4z8-PLAN | ✓ SATISFIED | truth 5 |
|
||||
| WR-12 | 261022-4z8-PLAN | ✓ SATISFIED (consumer impact: truth 12) | truth 7 |
|
||||
| WR-13 | 261022-4z8-PLAN | ✓ SATISFIED | truth 8 |
|
||||
| WR-14 | 261022-4z8-PLAN | ✓ SATISFIED | truth 2 |
|
||||
| IN-14 | 261022-4z8-PLAN | ✓ SATISFIED | truth 4 |
|
||||
| IN-15 | 261022-4z8-PLAN | ✓ SATISFIED | truth 3 |
|
||||
| 4z8-docs | 261022-4z8-PLAN | ✓ SATISFIED | truth 10 |
|
||||
|
||||
### Anti-Patterns Found
|
||||
|
||||
| File | Line | Pattern | Severity | Impact |
|
||||
|------|------|---------|----------|--------|
|
||||
| (added lines in a28957f..a48afbd) | none | TBD/FIXME/XXX/TODO/HACK | none | no debt markers added |
|
||||
| cmd/summer/runtime.go, cmd/summer/runtime_test.go | 27, 81 | gofmt drift | ℹ️ Info | already present at a28957f (checked); not introduced by this task |
|
||||
| modules/bonfire/live.go | printAbove `case <-v.done` | branch not pinned by a committed test | ℹ️ Info | behaviour confirmed by a verifier probe; a small committed test would pin it |
|
||||
| modules/lagoon/migrations.go | Status | consumer-visible result change with no upgrade note | ⚠️ Warning | P-09 deliberately skipped docs/setup/upgrading.md; the SUMMARY's release-note input mentions it, but the false "no outside consumer" premise means fonoteka.go needs a follow-up |
|
||||
|
||||
### Human Verification Required
|
||||
|
||||
### 1. Accept or reject the lagoon.Status contract change
|
||||
|
||||
**Test:** Decide whether WR-12's change to `lagoon.Status` (framework sets first; a plugin with zero migrations no longer gets a row) is acceptable under "no exported API breaks".
|
||||
**Expected:** If you accept it, update `fonoteka.go/parity` (`withoutSharedPlugins` should also drop the `summercms.*` rows, or the index assertions should change) and note the behaviour change in the next release notes and upgrade docs. If you reject it, keep `Status` plugin-only and add the framework sets inside the `migrate:status` command instead.
|
||||
**Why human:** This is a policy call. The Go signature is unchanged. But a sibling consumer (fonoteka.go, `replace => ../summercms.go`) calls `lagoon.Status` and now fails `TestRollbackIsolatesFonotekaFullSchema`, which passes against the pre-task framework. The plan's statement that no outside consumer exists was wrong.
|
||||
|
||||
### Gaps Summary
|
||||
|
||||
No code gap. All eight findings are fixed as the review asked. Each has a pinning test that was independently proven red on the parent commit and green on the fix. The shared masking is in one place (`internal/argmask`). `go vet`, the full Docker suite, the race runs, the hello workspace and the four phase-gate stages all pass. The records are updated, and the commits are local, untagged and without trailers.
|
||||
|
||||
One item needs a human decision. The WR-12 fix changes what `lagoon.Status` returns. That is a behavioural, not compile-level, change to an exported function, and it breaks one previously passing test in the sibling consumer `fonoteka.go/parity`, contrary to the plan's premise. Note also that fonoteka's other two Status-based parity tests were already failing before this task (user plugin migration count drift), so fonoteka's parity suite was not green either way.
|
||||
|
||||
---
|
||||
|
||||
_Verified: 2026-10-22T04:30:00Z_
|
||||
_Verifier: Claude (msd-verifier)_
|
||||
|
||||
## Resolution of the human_needed item (orchestrator, 2026-10-22)
|
||||
|
||||
The open item was the WR-12 behaviour change to the exported `lagoon.Status`, which broke
|
||||
`fonoteka.go/parity` `TestRollbackIsolatesFonotekaFullSchema`. Decision (no consumer breaks):
|
||||
keep `lagoon.Status` plugin-only as before 57a849d; `migrate:status` builds its rows from the
|
||||
shared `migrationSets` through the unexported `setStatus`. Follow-up commits `139f5e3` (fix,
|
||||
`TestStatusPluginRows` pins `Status`, `TestMigrateStatusJSON` still red without the fix) and
|
||||
`b1fba7e` (disposition). Full Docker `go test ./...` green; the fonoteka test passes again. The two
|
||||
other fonoteka `Status` tests fail as before this task (they expect 11 user-plugin migrations, the
|
||||
plugin now has 13) — unrelated, pre-existing.
|
||||
Reference in New Issue
Block a user