docs(01): add code review report

2026-09-16 14:31:28 +02:00
parent 2da09c5233
commit fead2527b6
1 changed files with 266 additions and 0 deletions
--- a/.planning/phases/01-framework-kernel-foundation/01-REVIEW.md
+++ b/.planning/phases/01-framework-kernel-foundation/01-REVIEW.md
@@ -0,0 +1,266 @@
+---
+phase: 01-framework-kernel-foundation
+reviewed: 2026-09-16T12:29:05Z
+depth: standard
+files_reviewed: 36
+files_reviewed_list:
+  - backpack/app.go
+  - backpack/services.go
+  - backpack/services_test.go
+  - bonfire/command.go
+  - bonfire/output.go
+  - bonfire/output_test.go
+  - bonfire/prompts.go
+  - bonfire/prompts_test.go
+  - bonfire/root.go
+  - bonfire/widgets.go
+  - cmd/summer/main.go
+  - compass/config.go
+  - compass/config_test.go
+  - compass/env.go
+  - compass/persist.go
+  - examples/hello/hello_test.go
+  - examples/hello/main.go
+  - examples/hello/plugins.gen.go
+  - examples/hello/plugins/greeter/plugin.go
+  - examples/hello/plugins/optional/plugin.go
+  - examples/hello/summer.yaml
+  - festival/bus.go
+  - festival/bus_test.go
+  - internal/build/build.go
+  - internal/build/build_test.go
+  - internal/build/manifest.go
+  - internal/build/scaffold.go
+  - internal/dev/watch.go
+  - internal/dev/watch_test.go
+  - pact/capabilities.go
+  - pact/capabilities_test.go
+  - party/registry.go
+  - party/registry_test.go
+  - scripts/check-phase1.sh
+  - towel/context.go
+  - towel/context_test.go
+findings:
+  critical: 1
+  warning: 10
+  info: 6
+  total: 17
+status: issues_found
+---
+
+# Phase 1: Code Review Report
+
+**Reviewed:** 2026-09-16T12:29:05Z
+**Depth:** standard
+**Files Reviewed:** 36
+**Status:** issues_found
+
+## Summary
+
+Phase 1 kernel is a real boot path: compiled plugins, cobra+koanf, no runtime plugin loading, and `exec.Command` with argv (no YAML shell strings). Import quoting uses `strconv.Quote`, plugin IDs are constrained, and backpack does not import party.
+
+The defects that matter are in the CLI/dev kernel that later phases will reuse: piped multi-prompt input is dropped, `summer dev` child lifecycle does not match the plan, tool stderr is aliased to stdout, and several path/module validations stop short of the threat-model promises (symlink escape, `go mod edit` `=` splitting, YAML `#` comments).
+
+Command injection via `summer build`/`dev` was not found: `go build`, `go mod edit`, and `go work use` are argv-based. Residual risk is unquoted YAML writes and incomplete module-path character sets, not a shell.
+
+## Critical Issues
+
+### CR-01: Sequential prompts drop piped stdin lines
+
+**File:** `bonfire/prompts.go:116-133`
+**Issue:** `readLine` allocates a new `bufio.Reader` on every call unless `c.in` is already a `*bufio.Reader`. `NewOutput` never wraps stdin that way, so each Ask/Choice/Secret does `Read(4096)` into a reader that is then discarded. On a pipe or file, the remainder of stdin is consumed and lost; the next prompt sees EOF and returns the default (or empty secret).
+
+This is the documented non-TTY path (`ask`/`choice`/`secret` read stdin lines). Tests only ever issue one line-reading prompt, so the suite cannot catch it. Confirm currently short-circuits when non-interactive, which hides the bug in `greeter:hello`.
+
+**Fix:** Keep one `*bufio.Reader` on `console` for the Output lifetime:
+
+```go
+type console struct {
+    in      io.Reader
+    reader  *bufio.Reader
+    // ...
+}
+
+func (c *console) readLine() (string, error) {
+    if c.in == nil {
+        return "", io.EOF
+    }
+    if c.reader == nil {
+        c.reader = bufio.NewReader(c.in)
+    }
+    line, err := c.reader.ReadString('\n')
+    line = strings.TrimRight(line, "\r\n")
+    if err != nil && err != io.EOF {
+        return "", err
+    }
+    if err == io.EOF && line == "" {
+        return "", io.EOF
+    }
+    return line, nil // do not TrimSpace here; see WR-01
+}
+```
+
+Add a test that feeds `"alice\nbob\n"` and asserts two `Ask` calls return `alice` then `bob`.
+
+## Warnings
+
+### WR-01: Secret answers are whitespace-trimmed
+
+**File:** `bonfire/prompts.go:125-132`
+**Issue:** `readLine` applies `strings.TrimSpace` to every line, including `Secret`. Leading/trailing spaces in a password or token are silently stripped. `TrimRight(..., "\r\n")` is the only stripping a secret prompt should do.
+
+**Fix:** Return the newline-trimmed line from `readLine`. Apply `strings.TrimSpace` only in `Ask`/`Choice`, never in `Secret`.
+
+### WR-02: `summer dev` starts the new child before reaping the old one
+
+**File:** `internal/dev/watch.go:86-92`
+**Issue:** The plan requires stop-and-reap, then start. The loop does the opposite:
+
+```go
+next, err := startChild(ctx, opts, binPath, out)
+current.stop()
+current = next
+```
+
+Two app processes overlap. Harmless for the Phase 1 hello CLI (it exits immediately — see WR-03), but any later `serve` listener will hit `bind: address already in use` and the new child will die while the old one keeps serving stale code.
+
+**Fix:**
+
+```go
+current.stop()
+current = nil
+next, err := startChild(ctx, opts, binPath, out)
+if err != nil {
+    fmt.Fprintf(out, "start error: %v\n", err)
+    return
+}
+current = next
+```
+
+### WR-03: Watch context is never cancelled by signals; child ignores ctx and gets no args
+
+**File:** `cmd/summer/main.go:13-23`, `internal/dev/watch.go:201-209`
+**Issue:** `root.Execute()` uses cobra's background context. There is no `signal.NotifyContext` and no `ExecuteContext`. `startBin` uses `exec.Command(binPath)` (not `CommandContext`), sets no `cmd.Dir`, and passes no extra args.
+
+Consequences:
+
+- Ctrl+C relies on the default SIGINT handler, which does not run `Watch`'s `defer current.stop()`. Cleanup is accidental process-group delivery.
+- The hello child is invoked as `bin/hello` with no args, prints cobra help, and exits. "Keep the last successful child running on build failure" is vacuously true because nothing stays running.
+- `startBin` discards its `context.Context` argument.
+
+**Fix:** In `cmd/summer/main.go`:
+
+```go
+ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
+defer stop()
+if err := root.ExecuteContext(ctx); err != nil {
+    fmt.Fprintln(os.Stderr, err)
+    os.Exit(1)
+}
+```
+
+Start the child with `exec.CommandContext`, `cmd.Dir = appDir`, `SysProcAttr.Setpgid` (so SIGTERM to the child group does not kill the watcher), and pass through args after `--`. Even a Phase 1 default of no extra args should still be context-bound.
+
+### WR-04: Child binary path is frozen at Watch start
+
+**File:** `internal/dev/watch.go:55-59`
+**Issue:** `binPath` is computed once from the initial `summer.yaml`. `summer.yaml` is a watched file and `build.App` re-reads it, so a `binary:` rename (or a first generate that changes the name) builds `bin/<new>` and then starts `bin/<old>`.
+
+**Fix:** Re-load the manifest (or return the output path from `build.App`) at the start of each successful rebuild before `startChild`.
+
+### WR-05: `build`/`dev` do not find the app root; `make:plugin`/`plugin:add` do
+
+**File:** `cmd/summer/main.go:40-45`, `internal/build/build.go:19-36`, `internal/build/scaffold.go:29-32`
+**Issue:** `MakePlugin`/`AddPlugin` walk up via `findAppDir`. `summer build` and `summer dev` require `summer.yaml` in `Getwd()`. Running `summer build` from `examples/hello/plugins` fails; running `summer make:plugin` from the same directory scaffolds into the hello app. `plugin:add` then joins a relative plugin path with `startDir` (cwd), not `appDir`, so `plugin:add plugins/demo` from a subdirectory resolves to the wrong place.
+
+**Fix:** Resolve `appDir := findAppDir(cwd)` in all four tool commands. Join relative `plugin:add` arguments with `appDir`.
+
+### WR-06: Tool and generated app send cobra stderr to stdout
+
+**File:** `bonfire/root.go:13-15`, `cmd/summer/main.go:14`, `internal/build/build.go:113`
+**Issue:** `NewRoot` is `NewRootIO(..., os.Stdin, out, out)`. The summer tool passes `os.Stdout` only. `Output.Error`, cobra diagnostics, and `go build` stderr all land on stdout. `main` still prints `Execute` errors to `os.Stderr`, so a user redirecting stdout loses some failures and keeps others.
+
+**Fix:** `NewRoot` should default stderr to `os.Stderr`. Tool and generated `main` should call `NewRootIO(name, commands, os.Stdin, os.Stdout, os.Stderr)`. Keep `NewRoot(name, commands, out)` as a test helper that aliases streams, or stop using it for production entrypoints.
+
+### WR-07: Module-path validation is too loose for `go mod edit` and YAML writes
+
+**File:** `internal/build/manifest.go:162-184`, `internal/build/scaffold.go:254-266`, `internal/build/manifest.go:91-102`
+**Issue:** `validateModulePath` rejects whitespace and `$;&|*?<>()[]{}` but allows `=`, `#`, `,`, and `:`. Two concrete failures:
+
+- `go mod edit -require="+modPath+"@v0.0.0"` and `-replace="+modPath+"="+rel` split on `=`. A module path containing `=` is not shell injection (argv is safe) but it is a parse injection into `go mod edit`.
+- `SaveManifest` writes `module: %s` unquoted. A `#` in the path becomes a YAML comment; the next `LoadManifest` sees a truncated path.
+
+`strconv.Quote` on generated Go imports is correct (T-01-01). The gap is the `go mod edit` concatenation and the YAML emitter.
+
+**Fix:** Reject `=`, `#`, `:`, `,`, and `@` anywhere; prefer `module.CheckPath` from `golang.org/x/mod/module`. Quote YAML scalars (or emit via the YAML library). Pass require/replace as separate validated arguments only after that check.
+
+### WR-08: Path containment does not evaluate symlinks
+
+**File:** `internal/build/scaffold.go:100-102`, `internal/build/scaffold.go:417-426`
+**Issue:** T-01-06 requires generated paths to stay under the app root. `underRoot` uses `filepath.Rel` on logical paths. `filepath.Abs` does not call `EvalSymlinks`. `plugin:add plugins/demo` where `plugins/demo` is a symlink to `/etc` or another module still passes `underRoot`, then `go work use` and `go mod edit -replace` follow the symlink.
+
+**Fix:** `EvalSymlinks` on `appDir` and `pluginDir` after `Abs`, then `underRoot` on the resolved paths. Reject if either call fails.
+
+### WR-09: Plugin command names `build` and `dev` bypass the colon rule
+
+**File:** `bonfire/command.go:83-90`
+**Issue:** `validCommandName` is shared by the tool and by plugin commands. `build` and `dev` are always legal, so a plugin can register `Name: "build"` and skip `<namespace>:<verb>`. The plan only exempts those names on the tool binary.
+
+**Fix:** Pass a `kernel bool` (or an allow-list) into `wrap`/`NewRoot`. Kernel roots may accept `build`/`dev`/`make:plugin`/`plugin:add`; app roots must require `ns:verb` with both sides non-empty and no spaces.
+
+### WR-10: Invalid flag shorthand panics during command wrapping
+
+**File:** `bonfire/root.go:65-68`
+**Issue:** `cmd.Flags().StringP` panics if `Shorthand` is not a single character. That panic happens while building the cobra tree (`NewRoot` / generated `main`), so a bad plugin flag crashes process start instead of returning `error`.
+
+**Fix:** Reject shorthand with `utf8.RuneCountInString(flag.Shorthand) != 1` (and empty meaning "no shorthand") and return `fmt.Errorf("bonfire: invalid shorthand %q for flag %q", flag.Shorthand, flag.Name)`.
+
+## Info
+
+### IN-01: `Flag()` treats a non-empty default as "set"
+
+**File:** `bonfire/command.go:65-81`
+**Issue:** If the user did not pass `--mode` and the default is `"slow"`, `Flag("mode")` returns `("slow", true)`. An empty default returns `("", false)`. Commands cannot distinguish "unset" from "defaulted" unless the default is empty.
+
+**Fix:** Document that `ok` means "has a non-empty value, including default", or return `ok` from `Changed` only and add `FlagOrDefault`.
+
+### IN-02: Scaffolded plugin always requires framework `v0.0.0`
+
+**File:** `internal/build/scaffold.go:182-191`
+**Issue:** `pluginGoMod` writes `require git.golem15.com/golem15/summercms v0.0.0`. That only tidies when the app has a `replace` (copied by `frameworkReplaceFor`). An app that depends on a tagged framework module will get a plugin that cannot `go mod tidy` without a replace.
+
+**Fix:** Copy the app's required framework version from its `go.mod`; keep `v0.0.0` only when a replace is present.
+
+### IN-03: `MakePlugin` / `AddPlugin` are not transactional
+
+**File:** `internal/build/scaffold.go:53-81`, `internal/build/scaffold.go:142-151`
+**Issue:** `MakePlugin` can leave `plugins/<name>/` behind on a later `go mod tidy` failure; retry then hits "already exists". `AddPlugin` edits `go.mod` and `go.work` before `SaveManifest`, so a manifest write failure leaves the module graph ahead of `summer.yaml`.
+
+**Fix:** Write to a temp dir and rename; save the manifest first (or roll back `go mod edit` / `go work use` on failure).
+
+### IN-04: `Publish` of a typed-nil concrete pointer is stored
+
+**File:** `backpack/services.go:26-28`
+**Issue:** `any(value) == nil` is false for `var p *T; Publish[*T](p)`. Lookup then returns `(nil, true)` and callers can panic on method call. Interface nil is rejected (tested).
+
+**Fix:** Use `reflect.ValueOf(value).Kind()` / `IsNil` for nillable kinds.
+
+### IN-05: Generated app loads `config` relative to cwd, not the binary
+
+**File:** `internal/build/build.go:98`, `examples/hello/main.go:25`
+**Issue:** `compass.Load("config")` depends on process cwd. `examples/hello/bin/hello greeter:hello` from the repo root fails even though the binary exists. Fine if documented as "run from the app root"; easy to miss once `summer dev` starts a long-lived process.
+
+**Fix:** Resolve config next to the app module (or `filepath.Join(filepath.Dir(os.Args[0]), "..", "config")` only after a documented layout), or set `cmd.Dir = appDir` in `startBin` (WR-03).
+
+### IN-06: Plugin directory is `plugins/<name>`, so vendor collisions share a folder
+
+**File:** `internal/build/scaffold.go:154-157`
+**Issue:** `golem15.blog` and `acme.blog` both scaffold to `plugins/blog`. The second hits "already exists". Matches the Phase 1 plan (`plugins/<plugin>`), but will not survive a multi-vendor app without a layout change.
+
+**Fix:** When a second vendor appears, scaffold `plugins/<vendor>/<plugin>` (Winter layout) or include vendor in the directory name now.
+
+---
+
+_Reviewed: 2026-09-16T12:29:05Z_
+_Reviewer: Claude (gsd-code-reviewer)_
+_Depth: standard_