summercms/.planning/phases/01-framework-kernel-foundation/01-REVIEW.md

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
01-framework-kernel-foundation	2026-09-16T12:29:05Z	standard	36	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	critical warning info total 1 10 6 17	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:

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:

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:

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:

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