---
status: issues
phase: 04-cli-scaffolding-i18n-and-mail
depth: standard
files_reviewed: 36
files_reviewed_list:
  - bonfire/command.go
  - bonfire/root.go
  - cmd/summer/main.go
  - cmd/summer/main_test.go
  - examples/hello/config/mail.yaml
  - examples/hello/hello_test.go
  - examples/hello/plugins/base/lang/en/lang.yaml
  - examples/hello/plugins/base/lang/pl/lang.yaml
  - examples/hello/plugins/base/plugin.go
  - examples/hello/plugins/base/views/mail/hello-en.htm
  - examples/hello/plugins/base/views/mail/hello.htm
  - examples/hello/plugins/base/views/mail/layouts/hello.htm
  - internal/build/artifact.go
  - internal/build/build.go
  - internal/build/build_test.go
  - internal/build/leaf.go
  - internal/build/registry.go
  - internal/build/scaffold.go
  - internal/build/stubs/artifacts.tmpl
  - internal/build/stubs/plugin.tmpl
  - internal/build/stubs/registry.tmpl
  - pact/capabilities.go
  - party/registry.go
  - party/registry_test.go
  - phrasebook/loader.go
  - phrasebook/translator.go
  - phrasebook/translator_test.go
  - postcard/assets/default.htm
  - postcard/drivers.go
  - postcard/mailer.go
  - postcard/mailer_test.go
  - postcard/mailpit_test.go
  - postcard/smtp_test.go
  - postcard/templates.go
  - postcard/templates_test.go
  - scripts/check-phase4.sh
findings:
  critical: 0
  warning: 4
  info: 3
  total: 7
reviewed: 2026-09-18T12:15:00Z
---

# Phase 4: Code Review Report

**Reviewed:** 2026-09-18T12:15:00Z
**Depth:** standard
**Files Reviewed:** 36
**Status:** issues

## Summary

Scaffolding, phrasebook, and postcard are a real boot path: `make:*` writes Winter-shaped leaves and a temp+rename `registry.gen.go`, `summer build` runs a models-leaf import check before `go build`, `party.Activate` publishes a translator and mailer then fails Boot on missing mail files, and Send renders Markdown through `html/template` + goldmark (unsafe HTML off) with CR/LF header rejection and mandatory TLS unless `none` is set. String Vars cannot drop raw `<script>` into HTML; mail dotted names reject `..` / `/` / `\`; lang paths must be exactly `lang/<locale>/<group>.yaml`; SMTP dial-fail tests do not echo `secretpass`.

The defects that matter are enforcement holes, not missing features. D-11 is prefix-matching and non-recursive, and it silently skips plugins whose replace path is outside the app root. Concurrent `make:*` can drop registry entries. Locale catalog keys keep the directory spelling (`pt_BR` vs `pt-BR`). Mail still executes caller `Vars` as `html/template` data, so a `template.HTML` value bypasses the escaping the tests cover, and the regex post-check does not catch `onclick=` or `<img onerror>`.

No path-traversal read of host files was found on the Send or Get paths (lookups are in-memory after boot). `scripts/check-phase4.sh` does not leak a background process.

## Warnings

### WR-01: Models leaf check is bypassable

**File:** `internal/build/leaf.go:22-80`
**Issue:** D-11 requires `summer build` to fail when `models/` imports a sibling package. `inspectModelsImports` only reads immediate `models/*.go` (subdirectories are skipped), only compares the import string to `module/{classes,controllers,console,jobs,middleware,updates}`, and `checkModelsLeaf` `continue`s when `localModuleDir` is outside the app root. Three bypasses:

1. `models/internal/hook.go` (or any nested package) importing `module/classes` is never parsed.
2. `import "alias.example/classes"` plus `replace alias.example/classes => ./classes` does not match the module-prefix test.
3. A plugin whose `go.mod` replace is an absolute path outside `appDir` is not checked at all.

The happy-path test writes `models/bad.go` under a plugin inside the copied hello app, so none of these are covered.

**Fix:** Walk `models/` recursively for `.go` files; resolve each import against the plugin directory (or reject any import whose cleaned replace target is a sibling leaf); return an error instead of `continue` when a manifest plugin cannot be located inside the app root.

```go
if !underRoot(appDir, pluginDir) {
    return fmt.Errorf("build: plugin %s: models leaf check skipped; %s is outside the app root", p.ID, pluginDir)
}
```

### WR-02: Concurrent `make:*` can lose registry entries

**File:** `internal/build/registry.go:80-111`, `internal/build/artifact.go:467-477,510-538`, `internal/build/scaffold.go:249-256`
**Issue:** `writeRegistry` is atomic for one writer (temp file + `Rename`). The surrounding protocol is not. `rejectDuplicateFile` / `nextMigrationFile` are stat-then-write; `refreshRegistry` scans then writes with no file lock. Two overlapping `make:model` / `make:migration` calls can both pass the existence check, both write artifacts, then the earlier scan can rename a registry that omits the later file. `go mod tidy` on the same `go.mod` races the same way. Agent-driven parallel `make:*` is in scope for this repo's loop.

**Fix:** Hold an exclusive flock on `registry.gen.go` (or a `make.lock` in the plugin dir) from duplicate checks through `refreshRegistry` + tidy. Open artifact files with `O_CREATE|O_EXCL`. After the lock, rescan immediately before writing the registry.

### WR-03: Locale catalog keys are not canonicalized

**File:** `phrasebook/loader.go:243-263`, `phrasebook/translator.go:189-214`
**Issue:** Load stores messages under the directory name (`pt_BR`, `en-us`). `fallbackChain` adds the requested string, then `language.Parse(requested).String()` (typically `pt-BR` / `en-US`) and parents, then the configured fallback. D-04's `pl-PL` → `pl` case works because files live under `pl`. A Winter/Laravel `lang/pt_BR/` tree is not found for a `pt-BR` request (and the reverse misses if files use hyphens and the request uses underscores). Map lookup is also case-sensitive (`en-us` vs `en-US`). Missing keys then return the raw key instead of the translation that was loaded.

The hyphen parent walk itself is correct; the catalog key is what never matches.

**Fix:** On Load, key the catalog with a canonical form (`strings.ReplaceAll(locale, "_", "-")` then `language.Parse` / `tag.String()`, plus the original spelling). Use the same function for `GetIn` / `ChoiceIn` so request, parent, and file locale collapse to one chain.

### WR-04: Caller `Vars` can bypass HTML escaping; `validateHTML` is not a real sanitizer

**File:** `postcard/templates.go:336-346,367-377,399-409`, `postcard/mailer.go:16-23`
**Issue:** Plan T-04-08 / D-07: goldmark stays safe-mode and trusted `template.HTML` is only for layout-injected, already-rendered content. `render` passes `msg.Vars` (`map[string]any`) straight into `execHTML` for subject and Markdown body. A plugin that puts `template.HTML`, `template.URL`, or `template.JS` in `Vars` skips escaping. Layout then injects the Markdown HTML as `template.HTML`.

The regex net does not close that hole: `rawUnsafeTag` ignores `<img>` / `<svg>`; `eventHandler` requires whitespace around `=` so `onclick=alert(1)` and `<svg/onload=…>` do not match. String Vars are covered by tests; typed HTML is not. Collection invitations and other user-authored fields will flow through this API.

**Fix:** Coerce `Vars` to strings (or reject `html/template` typed values) before `execHTML`. Keep `template.HTML` / `template.CSS` only on the layout data built inside `render`. Replace the regex with a real HTML policy (or drop the claim that `validateHTML` is the safety net).

## Info

### IN-01: SMTP driver errors are wrapped without redaction

**File:** `postcard/drivers.go:163-165`, `postcard/mailer.go:79-81`
**Issue:** `DialAndSendWithContext` is returned as `fmt.Errorf("smtp: %w", err)` and then `postcard: %w`. Dial-to-port-1 tests assert `secretpass` is absent. AUTH / server-response failures are not tested; go-mail can include SMTP reply text. Log driver is clean (no password field, no HTML body).

**Fix:** Map driver errors to a stable `postcard: smtp send failed` (optionally `errors.Is` for tests) and log the verbose error at debug without username/password.

### IN-02: Any extra file in `LangFS` fails plugin boot

**File:** `phrasebook/loader.go:80-107,243-263`
**Issue:** `WalkDir` plus fail-closed `parseLangPath` treats `lang/en/lang.yaml.bak`, `README.md`, or a nested `lang/en/nested/x.yaml` as a hard Load error naming the plugin. That is safe against traversal (`../` cleans to a non-`lang/a/b.yaml` path). It also means `//go:embed .` or an editor backup in `lang/` cannot boot. Mail registration only reads declared names, which is the better shape.

**Fix:** Skip non-matching paths (or only `fs.Glob` `lang/*/*.yaml`) instead of failing the catalog.

### IN-03: `data:` is banned in all rendered HTML

**File:** `postcard/templates.go:25,406-408`
**Issue:** `(javascript|vbscript|data):` rejects every data URI, including `data:image/png;base64,…` used as inline mail images, and also fails a send whose body merely mentions `javascript:`. Conservative for v1; it is not XSS by itself.

**Fix:** Restrict the scheme check to URL attributes (`href` / `src` / `xlink:href`) and allow `data:image/*`.

---

_Reviewed: 2026-09-18T12:15:00Z_
_Reviewer: gsd-code-reviewer_
_Depth: standard_