golem15/summercms
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(14.3): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-08 12:42:58 +02:00
parent 16a8520bb5
commit 3caaf5f894
2 changed files with 356 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

86
.planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-REVIEW-DISPOSITION.md Normal file
View File

@@ -0,0 +1,86 @@
---
phase: 14.3
review: 14.3-REVIEW.md
titles: json
findings:
- id: CR-01
severity: critical
disposition: open
title: "A framework path containing whitespace writes an unparseable `go.mod`, and `--no-plugin` reports success"
- id: WR-01
severity: warning
disposition: open
title: "Ctrl+C during `summer new` leaves a partial `sm-<name>-app`, and every re-run is then refused"
- id: WR-02
severity: warning
disposition: open
title: "The bootstrap script accepts Go keywords, `main` and `internal`, then clones and installs before `summer new` refuses them"
- id: WR-03
severity: warning
disposition: open
title: "An invalid `SUMMERCMS_REF` on a fresh clone leaves a branch checkout that the next run refuses"
- id: WR-04
severity: warning
disposition: open
title: "The binary-source fallback can resolve into the Go module cache"
- id: WR-05
severity: warning
disposition: open
title: "Every generated application publishes PostgreSQL on the same fixed host port 54320"
- id: WR-06
severity: warning
disposition: open
title: "`go.work` is committed but `go.work.sum` is ignored, so a `--no-plugin` app commits no module checksums at all"
- id: WR-07
severity: warning
disposition: open
title: "`TestTerminalCommands` passes the developer's `SUMMERCMS_DIR` through to the script"
- id: WR-08
severity: warning
disposition: open
title: "The `--generator` gate stage stops `summer serve` but not the app binary it started"
- id: IN-01
severity: info
disposition: open
title: "The security-gate pin check accepts any cell containing the word \"gate\", and only an exact \"open\" status fails"
- id: IN-02
severity: info
disposition: open
title: "The git identity fallback treats name and email independently of the author/committer split and ignores `EMAIL`"
- id: IN-03
severity: info
disposition: open
title: "The framework-containment guard is purely lexical"
- id: IN-04
severity: info
disposition: open
title: "`SUMMERCMS_REF` silently accepts branch names, which never move forward on a re-run"
- id: IN-05
severity: info
disposition: open
title: "The existing-checkout path fetches from `origin` and ignores `SUMMERCMS_REPO` without saying so"
- id: IN-06
severity: info
disposition: open
title: "The site plugin requires framework v0.1.12, which has no `modules/sunrise`"
- id: IN-07
severity: info
disposition: open
title: "`validCommandName` now also admits `new` for application binaries"
- id: IN-08
severity: info
disposition: open
title: "The printed `cd` and next-step lines are not shell-quoted"
- id: IN-09
severity: info
disposition: open
title: "The app name has no length limit, but PostgreSQL truncates database names at 63 bytes"
- id: IN-10
severity: info
disposition: open
title: "`TestNewGitUsesConfiguredIdentity` never exercises a configured `user.name`/`user.email`"
---
# Phase 14.3 review disposition
Every finding starts as `open`. Update each row to `fixed`, `deferred` or `wontfix` as it is triaged.

270
.planning/phases/14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new/14.3-REVIEW.md Normal file
View File

@@ -0,0 +1,270 @@
---
phase: 14.3-one-command-app-scaffolding-summer-new-and-summercms-io-new
reviewed: 2026-10-08T10:41:00Z
depth: standard
files_reviewed: 35
files_reviewed_list:
- cmd/summer/main.go
- cmd/summer/main_test.go
- internal/build/new.go
- internal/build/new_test.go
- internal/build/new_golden_test.go
- internal/build/build_test.go
- internal/build/scaffold.go
- internal/build/stubs/new.tmpl
- internal/build/stubs/plugin.tmpl
- modules/bonfire/command.go
- modules/bonfire/command_test.go
- modules/lagoon/keygen.go
- modules/lagoon/keygen_test.go
- modules/sunrise/new.sh
- modules/sunrise/sunrise.go
- modules/sunrise/sunrise_test.go
- modules/sunrise/script_behavior_test.go
- scripts/check-phase14.3.sh
- examples/hello/config/http.yaml
- README.md
- modules/bonfire/README.md
- modules/lagoon/README.md
- modules/sunrise/README.md
- docs/setup/installation.md
- docs/architecture/go-modules-and-workspaces.md
- docs/console/scaffolding.md
- docs/console/introduction.md
- docs/site.yaml
- ../sm-summercmsio-app/plugins/golem15/summercms/script.go
- ../sm-summercmsio-app/plugins/golem15/summercms/plugin.go
- ../sm-summercmsio-app/plugins/golem15/summercms/routes_test.go
- ../sm-summercmsio-app/terminal_check_test.go
- ../sm-summercmsio-app/vue-summercmsio-app/app/data/terminal.json
- ../sm-summercmsio-app/vue-summercmsio-app/tests/terminal.test.ts
- internal/build/testdata (goldens, read for cross-checking only)
findings:
critical: 1
warning: 8
info: 10
total: 19
status: issues_found
---
# Phase 14.3: Code Review Report
**Reviewed:** 2026-10-08T10:41:00Z
**Depth:** standard
**Files Reviewed:** 35
**Status:** issues_found
## Summary
I reviewed the `summer new` generator (`internal/build/new.go`, the `new/*` stubs and the starter-route `plugin.tmpl` branch), the `new` CLI command, `lagoon.GenerateAppKey`, the `sunrise` bootstrap script and its embedding, the site's `GET /new` handler, the landing-page terminal check and the phase gate script.
The core guards hold up. The target check uses `Lstat`, so it covers a directory, a file, a symlink and a dangling symlink. `os.Mkdir` closes the TOCTOU window. `.env` is created 0600 before the chmod, so it is never briefly readable by others. `.gitignore` keeps `.env` out of the first commit. The fallback git identity is passed with `-c` and never written to the repository config. The bootstrap script is truncation-safe: only `main "$@"` runs at top level, and the test checks every line-boundary prefix of the script plus every byte prefix of its last line. Every child process gets `</dev/null`. Moved upstream tags and non-fast-forward moves are refused. The `GET /new` handler sets `text/plain; charset=utf-8`, `nosniff`, `no-cache` and a strong ETag, and `http.ServeContent` answers 304 correctly.
The defects found:
- **Unquoted framework replace path (blocker).** A framework path containing whitespace produces a `go.mod` that Go cannot parse. With `--no-plugin`, `summer new` still reports success. I reproduced this.
- **Cleanup only on returned errors.** The cleanup promise covers returned errors but not Ctrl+C. The bootstrap script also leaves its own fresh clone in a state that a re-run refuses.
- **Script and generator disagree on names.** The script's name check is weaker than `ValidateAppName`, so a keyword name clones and installs before it fails. The docs say this cannot happen.
- **Binary-source fallback.** This fallback can point an app's `replace` into GOMODCACHE.
- **Shared fixed port.** Every generated app publishes PostgreSQL on the same fixed host port.
- **No checksum file in some apps.** `go.work` is committed but `go.work.sum` is ignored, so a `--no-plugin` app commits no checksums at all.
- **Test and gate reliability.** `TestTerminalCommands` passes `SUMMERCMS_DIR` from the developer's environment through to the script. The `--generator` gate stage stops `summer serve` without stopping the app binary it started.
## Critical Issues
### CR-01: A framework path containing whitespace writes an unparseable `go.mod`, and `--no-plugin` reports success
**File:** `internal/build/stubs/new.tmpl:9`, `internal/build/new.go:324`, also `internal/build/scaffold.go:280-296` (`frameworkReplaceFor`)
**Issue:** `relModulePath` returns the raw relative path, and the template writes it into `go.mod` without quotes: `replace {{.Framework}} => {{.Replace}}`. In `go.mod` syntax, a path containing whitespace must be a quoted string. I reproduced this with `summer new demo --framework "my fw" --no-git --no-plugin`. The command printed `created …` and the next steps, but `go.mod` contained `replace git.golem15.com/golem15/summercms => ../my fw`. `summer build` then failed with `go.mod:9: malformed module path "../my": invalid path element ".."`.
With the starter plugin, the same unquoted path goes into `plugins/<name>/go.mod` through `frameworkReplaceFor`. `go mod tidy` then fails, and the user gets a removed directory and a confusing parse error. The bootstrap script hits the same failure for any `SUMMERCMS_DIR` whose last path segment contains a space, because it passes `--framework "$dir"`. This happens after the clone and `go install` have finished. The generator promises a "ready-to-run application", and this output breaks that promise without any error.
**Fix:** Quote the path whenever it needs quoting, in both templates, or refuse such paths before `os.Mkdir`:
```go
// modFilePath renders a local path as a go.mod/go.work token.
func modFilePath(p string) string {
if strings.ContainsAny(p, " \t\"'`\\") {
return strconv.Quote(p)
}
return p
}
// planNewApp: Replace: modFilePath(replace)
// frameworkReplaceFor: return modFilePath(filepath.ToSlash(rel)), nil
```
Alternatively, write the replace with `go mod edit -replace=` (Go quotes it), as `editAppModule` already does. Add a `TestNew*` case with a framework directory named `"my fw"` that builds the result.
## Warnings
### WR-01: Ctrl+C during `summer new` leaves a partial `sm-<name>-app`, and every re-run is then refused
**File:** `cmd/summer/main.go:15-26`, `internal/build/new.go:161-171`
**Issue:** `NewApp` removes the target only when `populateApp` returns an error. `summer`'s `main` installs no signal handling, and neither does `bonfire.NewRoot`/`Execute`, which pass `cmd.Context()` with no cancellation. SIGINT or SIGTERM therefore kills the process immediately. This most likely happens during the starter plugin's `go mod tidy`, which can download modules for a minute or more. A half-written `sm-<name>-app` (no `main.go`, `plugins.gen.go` or plugin `go.sum`) stays behind. Both `summer new` and the bootstrap script (`new.sh:230`) then refuse the name with "already exists". `docs/console/scaffolding.md` says: "When a step fails after it created the directory, it removes the directory again, so you can rerun the same command." That is not true for an interrupt.
**Fix:** Cancel the context on a signal, so `exec.CommandContext` kills `go`, `populateApp` returns an error, and the existing `RemoveAll` runs:
```go
func main() {
ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()
root, err := bonfire.NewRoot("summer", toolCommands(), os.Stdout)
...
if err := root.ExecuteContext(ctx); err != nil { ... }
}
```
This requires bonfire to expose `ExecuteContext`, or `NewRoot` to accept a context. Alternatively, handle it locally in `newCommand`.
### WR-02: The bootstrap script accepts Go keywords, `main` and `internal`, then clones and installs before `summer new` refuses them
**File:** `modules/sunrise/new.sh:215`
**Issue:** The script only checks `^[a-z][a-z0-9]*$`. `ValidateAppName` also refuses the 25 Go keywords, `main` and `internal`. Running `curl … | bash -s -- go` (or `type`, `map` or `main`) clones the framework, runs `go install`, and only then fails inside `summer new`. `docs/setup/installation.md` says: "The script stops before cloning when the name is invalid". For these names it does not.
**Fix:** Mirror the full rule before any side effect:
```bash
case "$name" in
break|case|chan|const|continue|default|defer|else|fallthrough|for|func|go|goto|if|import|interface|map|package|range|return|select|struct|switch|type|var|main|internal)
printf 'error: application name %q is a Go keyword or reserved (main, internal)\n\n' "$name" >&2
usage >&2; exit 2 ;;
esac
```
Add a `TestScriptBehavior` case asserting no clone for `go` and `main`.
### WR-03: An invalid `SUMMERCMS_REF` on a fresh clone leaves a branch checkout that the next run refuses
**File:** `modules/sunrise/new.sh:163-169`, `:119`
**Issue:** In the fresh-clone branch, `git clone` runs first, and `target_commit` only checks `SUMMERCMS_REF` afterwards. A typo such as `SUMMERCMS_REF=v0.1.99`, or the dash-prefixed value that the test itself uses, ends with "remove $dir and run again". `./summercms` is left on the clone's default branch. On the next run, `check_checkout` refuses with "is on branch main", so the user must delete the directory by hand. The script created that directory in this run, so it can safely remove it. The test "SUMMERCMS_REF starting with a dash is refused" never checks what is left on disk.
**Fix:** Run the cheap format check (`[[ $ref != -* ]]`) in `main` before `checkout_framework`. In the clone branch, remove the directory on failure, because it did not exist before this run:
```bash
target="$(target_commit "$dir" "$ref")" || { rm -rf -- "$dir"; die "no usable target in the new clone; removed $dir"; }
```
### WR-04: The binary-source fallback can resolve into the Go module cache
**File:** `internal/build/new.go:286-297`
**Issue:** `binarySourceFramework` accepts any absolute `runtime.Caller` path whose root `go.mod` declares the framework. A binary installed with `go install git.golem15.com/golem15/summercms/cmd/summer@vX` records its source under `$GOMODCACHE/git.golem15.com/golem15/summercms@vX/…`, because `go install pkg@version` does not imply `-trimpath`. In that case `summer new` (with no sibling and no flag) commits a `replace => ../../…/go/pkg/mod/git.golem15.com/golem15/summercms@vX`. That points into a read-only cache, is machine-specific, and breaks after `go clean -modcache`. It also contradicts the generated CLAUDE.md, which calls it "the local checkout".
**Fix:** Accept only development builds:
```go
if bi, ok := debug.ReadBuildInfo(); ok && bi.Main.Version != "(devel)" && bi.Main.Version != "" {
return "" // installed from a module version, not a checkout
}
```
Or reject roots containing `/pkg/mod/` together with an `@v` path element.
### WR-05: Every generated application publishes PostgreSQL on the same fixed host port 54320
**File:** `internal/build/stubs/new.tmpl:67`, `internal/build/new.go:51`
**Issue:** Each app's compose project gets its own name and volume, but `127.0.0.1:54320` is hard-coded in both `compose.yaml` and `.env`. The generated CLAUDE.md says the framework checkout "is shared with every other application next to it", and the bootstrap script is meant to be re-run for more apps in the same directory. Once two such apps exist, the second `docker compose up -d --wait` fails with "port is already allocated" while the first app's database is running.
**Fix:** Make the port overridable in both files from one variable, for example `"127.0.0.1:${SUMMER_DB_PORT:-54320}:5432"` in compose. Either derive a per-app default (for example a stable hash of the name into 54320-54999) or document the collision and the override in the next steps.
### WR-06: `go.work` is committed but `go.work.sum` is ignored, so a `--no-plugin` app commits no module checksums at all
**File:** `internal/build/stubs/new.tmpl:84`
**Issue:** The app has no root `go.sum` (by design), and its `go.work` is committed. I confirmed that the first `summer build` writes every framework dependency checksum to `go.work.sum`. That file is gitignored. For a `--no-plugin` app, the repository therefore pins no checksums at all. On a fresh clone, integrity for third-party modules depends entirely on GOSUMDB being reachable and enabled, which `GONOSUMDB`, `GOPRIVATE=*`, `GOFLAGS=-insecure` or offline proxies all defeat. Go's guidance is to commit `go.work.sum` whenever `go.work` is committed.
**Fix:** Drop `go.work.sum` from the generated `.gitignore`, and update the scaffolding and go-modules docs and the `dot-gitignore` golden. If it stays ignored by design, record the decision and the accepted risk in the threat register.
### WR-07: `TestTerminalCommands` passes the developer's `SUMMERCMS_DIR` through to the script
**File:** `../sm-summercmsio-app/terminal_check_test.go:68-88` (line 74)
**Issue:** `terminalEnv` drops `SUMMER_*`, `GOWORK` and `GOBIN`, but `SUMMERCMS_DIR` starts with `SUMMERC`, not `SUMMER_`, so it is kept. If a developer has `SUMMERCMS_DIR` exported, the gated test (and the `--terminal` gate stage) runs the script against that directory instead of the temp work dir:
- It fetches and may move that checkout forward.
- It creates `sm-demo-app` next to it, outside `t.TempDir()`.
- The `cd sm-demo-app` step then fails.
- Cleanup's `docker compose down -v` looks in the wrong directory.
`TestBootstrapEndToEnd` (`modules/sunrise/sunrise_test.go:161`) correctly strips every `SUMMERCMS_*` variable and `GOFLAGS`.
**Fix:** Drop `SUMMERCMS_DIR` explicitly (REPO and REF are meant to pass through) and also drop `GOFLAGS`:
```go
case strings.HasPrefix(name, "SUMMER_"), name == "SUMMERCMS_DIR", name == "GOWORK", name == "GOBIN", name == "GOFLAGS":
continue
```
Extend `TestTerminalEnv` to cover these variables.
### WR-08: The `--generator` gate stage stops `summer serve` but not the app binary it started
**File:** `scripts/check-phase14.3.sh:154-156`, `:225-226`
**Issue:** `summer serve` is a delegate. `cmd/summer/runtime.go:117-123` runs `bin/<app> serve` as a child through `exec.CommandContext`, and `summer` installs no signal handler. `kill "$server_pid"` sends SIGTERM to `summer` only, so the app binary is orphaned and can keep holding `:8080`. It dies only when it next writes to its broken stdout or stderr pipe. If it survives, the next `--all` stage that checks ports (`--terminal`, `port_free 8080`) fails, or a stale server answers. The site's `terminalServeBlock` uses `pkill -TERM -P "$serve_pid"` for exactly this reason (`terminal_check_test.go:101`).
**Fix:** Stop the children first, as the site test does:
```bash
pkill -TERM -P "$server_pid" 2>/dev/null || true
kill "$server_pid" 2>/dev/null || true
wait "$server_pid" 2>/dev/null || true
```
A better fix is to forward signals in `runAppBinary` (see WR-01).
## Info
### IN-01: The security-gate pin check accepts any cell containing the word "gate", and only an exact "open" status fails
**File:** `scripts/check-phase14.3.sh:524-531`
**Issue:** `re.search(r"Test[A-Z]\w+|--(…)\b|gate", pinned)` treats any substring "gate" as a pin. That includes "no gate yet" and "delegate". `status == "open"` lets "Open (deferred)" or "open: tracked" pass for non-high rows. Every current row is pinned properly, so this only weakens the fail-closed claim.
**Fix:** Match `\bgate\b` against `--all`, or drop that alternative. Use `status.startswith("open")`.
### IN-02: The git identity fallback treats name and email independently of the author/committer split and ignores `EMAIL`
**File:** `internal/build/new.go:216-223`, `:236-244`
**Issue:** If only `GIT_AUTHOR_EMAIL` is set and there is no config, no fallback is added. The committer email then falls back to auto-detection, which fails in containers with a bare hostname, so the commit becomes a warning. Conversely, `-c user.email=summer@localhost` overrides a user's `EMAIL` environment variable, which git would otherwise use.
**Fix:** Treat each of author and committer separately, or check `git var GIT_COMMITTER_IDENT` / `GIT_AUTHOR_IDENT` and fall back only when one of them fails.
### IN-03: The framework-containment guard is purely lexical
**File:** `internal/build/new.go:153`
**Issue:** `underRoot(framework, target)` compares lexical paths. A parent reached through a symlink into the framework checkout, or a `--framework` symlink pointing at an ancestor, passes the guard.
**Fix:** Also compare `filepath.EvalSymlinks` of both paths, keeping the lexical paths for the `replace`.
### IN-04: `SUMMERCMS_REF` silently accepts branch names, which never move forward on a re-run
**File:** `modules/sunrise/new.sh:118-127`
**Issue:** `rev-parse --verify "${ref}^{commit}"` resolves `main` to the local `refs/heads/main` created by the clone. `fetch` updates only `origin/main`, so a re-run reports "already at the target" on a stale commit. The usage text says "tag or commit", but nothing enforces it.
**Fix:** Try `refs/tags/$ref` first and then a full commit hash. Refuse anything that resolves under `refs/heads/` or `refs/remotes/`.
### IN-05: The existing-checkout path fetches from `origin` and ignores `SUMMERCMS_REPO` without saying so
**File:** `modules/sunrise/new.sh:175`
**Issue:** A checkout whose `origin` is a fork or a stale mirror is moved to that remote's newest tag. `SUMMERCMS_REPO` is not compared and no message is printed. A missing `origin` remote produces the misleading message "a release tag may have moved upstream".
**Fix:** Print `git remote get-url origin` in the "updating" line, and warn when it differs from `$repo`.
### IN-06: The site plugin requires framework v0.1.12, which has no `modules/sunrise`
**File:** `../sm-summercmsio-app/plugins/golem15/summercms/go.mod:5`, `plugin.go:62`
**Issue:** `modules/sunrise` exists only in the untagged working tree. Until v0.1.13 is tagged and the require is bumped (D-28, documented in 14.3-04-SUMMARY), every site build resolves the framework only through the local `replace ../summercms.go`. The bytes served at `/new` are whatever that working tree holds at build time.
**Fix:** Follow the documented release steps. Consider a site test that fails when `go list -m git.golem15.com/golem15/summercms` resolves to a replace while building for deploy.
### IN-07: `validCommandName` now also admits `new` for application binaries
**File:** `modules/bonfire/command.go:112`
**Issue:** The whitelist is shared by the `summer` tool and every app binary, so a plugin can now register a bare `new` command in an application. This is harmless today but broadens the bare-name surface.
**Fix:** Keep the tool-only bare names in the tool's root, or document that `new` is reserved.
### IN-08: The printed `cd` and next-step lines are not shell-quoted
**File:** `modules/sunrise/new.sh:248`, `internal/build/stubs/new.tmpl:134-146`
**Issue:** `printf ' cd %s\n' "$app"` prints a path that, if it contains spaces, breaks when copied and pasted.
**Fix:** Use `printf ' cd %q\n' "$app"`.
### IN-09: The app name has no length limit, but PostgreSQL truncates database names at 63 bytes
**File:** `internal/build/new.go:22`, `internal/build/stubs/new.tmpl:65`
**Issue:** `POSTGRES_DB` is truncated by the server while the `.env` DSN keeps the full name, so `summer migrate` fails with "database does not exist".
**Fix:** Cap names at, for example, 40 characters in `appNameRe`/`ValidateAppName` and in `new.sh`.
### IN-10: `TestNewGitUsesConfiguredIdentity` never exercises a configured `user.name`/`user.email`
**File:** `internal/build/new_test.go:573-591`
**Issue:** The test sets only `GIT_*` environment variables. The `git config <key>` branch of `gitIdentityMissing` that returns "configured" is never covered. Several generator tests (`TestNewCommandViaCLI`, `TestNewEnvFileMode`, `TestNewAgentsSymlink`, `TestNewGitMissingIsWarning`) also run the starter plugin's `go mod tidy` without a `testing.Short()` guard.
**Fix:** Add a case that writes `user.name`/`user.email` to the isolated `GIT_CONFIG_GLOBAL` file. Gate the network-touching tests behind `-short` like `TestNewAppSmoke`, or use `NoPlugin` where the plugin is irrelevant.
---
_Reviewed: 2026-10-08T10:41:00Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_