--- 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_