From fead2527b698d42a191e85185888bb12ac77cd4e Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Wed, 16 Sep 2026 14:31:28 +0200 Subject: [PATCH] docs(01): add code review report --- .../01-REVIEW.md | 266 ++++++++++++++++++ 1 file changed, 266 insertions(+) create mode 100644 .planning/phases/01-framework-kernel-foundation/01-REVIEW.md diff --git a/.planning/phases/01-framework-kernel-foundation/01-REVIEW.md b/.planning/phases/01-framework-kernel-foundation/01-REVIEW.md new file mode 100644 index 0000000..36acc52 --- /dev/null +++ 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/` and then starts `bin/`. + +**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 `:`. 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//` 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/`, 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/`), but will not survive a multi-vendor app without a layout change. + +**Fix:** When a second vendor appears, scaffold `plugins//` (Winter layout) or include vendor in the directory name now. + +--- + +_Reviewed: 2026-09-16T12:29:05Z_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: standard_