diff --git a/.planning/phases/01-foundation/01-REVIEWS.md b/.planning/phases/01-foundation/01-REVIEWS.md new file mode 100644 index 0000000..b153e1d --- /dev/null +++ b/.planning/phases/01-foundation/01-REVIEWS.md @@ -0,0 +1,81 @@ +--- +phase: 1 +reviewers: [codex] +reviewed_at: 2026-05-14T15:23:07Z +plans_reviewed: [01-01-PLAN.md, 01-02-PLAN.md, 01-03-PLAN.md, 01-04-PLAN.md] +skipped: [claude (self), gemini (not installed), coderabbit (not installed), opencode (not installed), qwen (not installed), cursor (not installed), ollama (not running), lm_studio (not running), llama_cpp (not running)] +--- + +# Cross-AI Plan Review — Phase 1: Foundation + +## Codex Review + +## Summary + +The plans are unusually thorough and trace requirements well, but they are too brittle in a few places. The biggest risks are not architectural; they are execution-contract issues: commands that may not work as written, tests that are intentionally red but may block normal `go test ./...`, and a few contradictions between “vendored/static/no CDN/no generated files” policies and the proposed bootstrap flow. Overall, the phase goal is achievable, but the plans need tightening before execution. + +## Strengths + +- Clear phase boundary: Foundation stays focused on scaffold, health checks, templ/HTMX, local Postgres, migrations, logging, and README. +- Good traceability from FOUND-01..05 to concrete files, commands, tests, and checkpoints. +- Strong dependency sequencing: scaffold → UI/tests → green implementation → README. +- Good operational basics: request IDs, structured logs, graceful shutdown, timeouts, DB health behavior. +- Strong UI discipline for a small phase: tokens, components, CSS ownership, and no raw button styling in page templates. +- Good local-dev validation, including browser HTMX checks and README onboarding walkthrough. + +## Concerns + +- **HIGH: `go mod tidy` in Plan 01 may remove all pinned runtime deps.** Since no Go code imports chi/templ/pgx/goose/uuid yet, `go mod tidy` can drop those `require` lines. The acceptance checks may fail immediately. +- **HIGH: Tailwind standalone download URL is likely fragile.** `tailwindcss-$(uname -s | tr A-Z a-z)-$(uname -m)` often does not match GitHub asset naming, especially macOS `darwin` vs `macos` and `x86_64` vs `x64`. +- **HIGH: Intentional RED tests can break later scaffold verification if not isolated.** Plan 02 says `go test ./internal/web` must fail, but also introduces `pool_test.go` referencing `db.NewPool` before it exists. Any accidental `go test ./...` after Plan 02 will fail beyond the intended web RED gate. +- **MEDIUM: “Vendored” HTMX is gitignored.** The plan says HTMX is vendored, but `.gitignore` excludes `static/htmx.min.js`. That makes it bootstrap-generated, not repo-vendored. This is workable locally, but the wording and later deployment assumptions should be consistent. +- **MEDIUM: CDN policy is inconsistent.** Success criteria say “No CDN references in any committed file,” while the justfile intentionally contains an `unpkg.com` bootstrap URL. Later verification allows this exception. Make the exception explicit everywhere. +- **MEDIUM: Plan 01 `just generate` will fail until Plan 02 creates UI CSS and Plan 03 creates templates/cmd.** This is acceptable if documented, but the justfile exists before its recipes are fully runnable. +- **MEDIUM: UI component CSS may not pass Tailwind v4 processing as written.** Plain nested CSS like `&:hover` is only safe if the Tailwind pipeline supports nesting in that context. Prefer non-nested `.ui-button-solid-default-md:hover`. +- **MEDIUM: `templ Card(attrs templ.Attributes)` with children needs exact templ syntax.** The plan’s interface is conceptually right, but templ child syntax should be verified against current templ rules before locking tests around it. +- **LOW: Goose no-op migration with `SELECT 1` is fine but aesthetically odd.** It exercises goose, but creates no domain object. That is acceptable for Phase 1. +- **LOW: README asks for `just clean`, but no `clean` recipe exists.** It is listed only in fallback Method B, but it violates the “no aspirational commands” rule unless added or removed. + +## Suggestions + +- Fix Plan 01 dependency pinning: either skip `go mod tidy` until code imports the deps, add actual imports in early files, or accept that deps land in Plan 03 rather than Plan 01. +- Replace Tailwind URL logic with explicit OS/arch mapping in shell, covering `darwin → macos`, `x86_64 → x64`, `aarch64/arm64 → arm64`. +- Change the CDN rule to: “No runtime CDN references; bootstrap-time download URLs are allowed only in `justfile`.” +- Decide whether HTMX is committed or generated. If generated, call it “bootstrapped local asset,” not “vendored,” and ensure deploy/build docs run bootstrap. +- Make Plan 02 verification only run package-specific checks that tolerate the intentional RED state. Avoid any command that implies `go test ./...` should pass before Plan 03. +- Use non-nested CSS selectors in component CSS for maximum compatibility. +- Remove `just clean` from README Method B or add a real `clean` recipe. +- Add one explicit “Plan 01 incomplete recipe caveat”: `generate`, `test`, `build`, and `dev` are listed early but only become runnable after Plans 02/03. + +## Risk Assessment + +**Overall risk: MEDIUM.** + +The architecture is low-risk and appropriate for the phase, but execution risk is medium because several command contracts are likely to fail exactly where the plan expects mechanical reliability: Go module tidying, Tailwind binary download, generated asset policy, and intentional red-test handling. Tightening those will make the plans strong enough to execute. + +--- + +## Consensus Summary + +Only one external reviewer (Codex) was available — the host AI is Claude Code (skipped for independence), and no other CLIs or local model servers were installed/running. A true consensus across multiple AI systems is not possible from this run; treat the Codex review below as a single independent perspective rather than convergent feedback. + +### Agreed Strengths (Codex only) + +- Tight phase boundary — Foundation stays scoped to scaffold, health checks, templ/HTMX, Postgres + migrations, logging, README. +- Clear FOUND-01..05 traceability into concrete files, commands, tests, and checkpoints. +- Strong sequencing: scaffold → UI/tests (RED) → GREEN implementation → README. +- Good operational basics: request IDs, structured logs, graceful shutdown, DB-health semantics on `/healthz`. + +### Agreed Concerns (Codex only — HIGH severity) + +1. **`go mod tidy` in Plan 01-01 may strip pinned runtime deps** (chi, templ, pgx, goose, uuid) because no Go file imports them yet. +2. **Tailwind standalone download URL is fragile** — `uname -s` / `uname -m` outputs don't match GitHub release asset naming (`darwin` vs `macos`, `x86_64` vs `x64`). +3. **Intentional RED tests in Plan 01-02 can leak** — a stray `go test ./...` will fail beyond the intended web RED gate (e.g., `pool_test.go` referencing `db.NewPool` before it exists). + +### Divergent Views + +Not applicable — only one reviewer. + +### Recommended Next Step + +Re-run `/gsd:review --phase 1` after installing at least one additional independent CLI (e.g., `gemini`, `codex` already present, plus `coderabbit` for diff-based review or `opencode`/`qwen`/`cursor`) so concerns can be cross-validated. In the meantime, the three Codex HIGH-severity concerns above are concrete and actionable — feed them into `/gsd-plan-phase 1 --reviews`.