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

docs(11.2): add code review fix report and record dispositions

Browse Source
This commit is contained in:
Jakub Zych
2026-10-01 17:13:08 +02:00
parent bd2f7e7cb5
commit 37f7192832
3 changed files with 138 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
.planning/phases/11.2-ready-to-share-summercms-io-website-and-newsletter-plugin/11.2-REVIEW-DISPOSITION.md
View File

@@ -10,13 +10,13 @@ One row per finding in 11.2-REVIEW.md. Default `open`; set to `fixed`, `accepted
| ID | Severity | Finding | Disposition | Note |
|----|----------|---------|-------------|------|
| CR-01 | critical | The install root is owned by the service account, so root's deploy steps can be redirected through symlinks | open | |
| WR-01 | warning | The HTTPS server block sends no Strict-Transport-Security header | open | |
| WR-02 | warning | The HTTP block redirects ACME challenges, and the app 404s `/.well-known/` | open | |
| WR-03 | warning | The D-40 terminal check puts GOBIN on PATH, so it does not prove the page's commands work in a fresh shell | open | |
| WR-04 | warning | The site test gate and the `pnpm test` script only work on Node 22.18 to 22.x | open | |
| WR-05 | warning | The smoke script can test a different server that already holds the port | open | |
| WR-06 | warning | A release build does not compile what its plugin tests checked, and does not require clean submodules | open | |
| CR-01 | critical | The install root is owned by the service account, so root's deploy steps can be redirected through symlinks | fixed | sm-summercmsio-app 8e5765e |
| WR-01 | warning | The HTTPS server block sends no Strict-Transport-Security header | fixed | sm-summercmsio-app 1dcf774 (no includeSubDomains; check on rome) |
| WR-02 | warning | The HTTP block redirects ACME challenges, and the app 404s `/.well-known/` | fixed | sm-summercmsio-app 1e934ab (webroot path to check on rome) |
| WR-03 | warning | The D-40 terminal check puts GOBIN on PATH, so it does not prove the page's commands work in a fresh shell | accepted | user chose option 1: card unchanged, PATH step added to the installation guide (summercms.go bd2f7e7) |
| WR-04 | warning | The site test gate and the `pnpm test` script only work on Node 22.18 to 22.x | fixed | vue-summercmsio-app 60c3c4a, summercms.go be51bfe, sm-summercmsio-app aeac7bf |
| WR-05 | warning | The smoke script can test a different server that already holds the port | fixed | sm-summercmsio-app 8cfe055 |
| WR-06 | warning | A release build does not compile what its plugin tests checked, and does not require clean submodules | fixed | sm-summercmsio-app fffa38c |
| IN-01 | info | The extension-less docs 301 drops the query string | open | |
| IN-02 | info | `site_label` in site.yaml cannot be paired with `--site-url` alone | open | |
| IN-03 | info | The og:image alt text is not reactive, unlike the other SEO fields | open | |
@@ -26,4 +26,4 @@ One row per finding in 11.2-REVIEW.md. Default `open`; set to `fixed`, `accepted
| IN-07 | info | The external link check follows redirects, so a login redirect would count as a pass | open | |
| IN-08 | info | The scroll-spy section list is defined twice | open | |
open: 15 of 15
fixed: 6, accepted: 1, open: 8 of 15 (IN-01..IN-08, info, out of fix scope)

129
.planning/phases/11.2-ready-to-share-summercms-io-website-and-newsletter-plugin/11.2-REVIEW-FIX.md Normal file
View File

@@ -0,0 +1,129 @@
---
phase: 11.2-ready-to-share-summercms-io-website-and-newsletter-plugin
fixed_at: 2026-10-01T15:11:41Z
review_path: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/11.2-ready-to-share-summercms-io-website-and-newsletter-plugin/11.2-REVIEW.md
iteration: 1
findings_in_scope: 7
fixed: 6
skipped: 1
status: partial
---
# Phase 11.2: Code Review Fix Report
**Fixed at:** 2026-10-01T15:11:41Z
**Source review:** /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/11.2-ready-to-share-summercms-io-website-and-newsletter-plugin/11.2-REVIEW.md
**Iteration:** 1
**Summary:**
- Findings in scope: 7 (CR-01, WR-01 to WR-06; IN-* out of scope)
- Fixed: 6
- Skipped: 1 (WR-03, needs a user decision)
`workflow.use_worktrees` is `false`, so every edit and commit happened in the main checkouts. No worktree was created and nothing was pushed or tagged.
## Fixed Issues
### CR-01: The install root is owned by the service account, so root's deploy steps can be redirected through symlinks
**Repo:** sm-summercmsio-app
**Files modified:** `DEPLOY.md`, `deploy/env.example`
**Commit:** 8e5765e
**Applied fix:**
- The service account is now created with `--no-create-home --home /nonexistent`.
- `/srv/summercms-io`, `bin/` and `config/` are created `root:root 0755`. Only `storage/` belongs to the service.
- `.env` is created with `install -o root -g summercms -m 0640 /dev/null`, so the service can read it but not write it. The overview table and the `env.example` header now say the same.
- DEPLOY.md explains why the install root must stay root-owned and adds a `stat` ownership check to run before each deploy.
- It also records that `serve` writes only below `storage/`. I checked the framework: the uploads bucket uses `create_dir`. The only other runtime writer is `compass.Persist`, which writes `config/env/<env>/overrides.yaml`, and only the admin calls it. The site has no admin, and that write would now fail on the root-owned `config/`.
### WR-01: The HTTPS server block sends no Strict-Transport-Security header
**Repo:** sm-summercmsio-app
**Files modified:** `deploy/nginx/summercms.io.conf`, `DEPLOY.md`
**Commit:** 1dcf774
**Applied fix:**
- Added `add_header Strict-Transport-Security "max-age=31536000" always;` to both the apex and the www HTTPS server blocks. Neither block's `location` has its own `add_header`, so the header is inherited.
- Left out `includeSubDomains` on purpose. Which subdomains on rome serve HTTPS cannot be checked from here, so it is marked *check on rome* in a comment.
- Added two curl checks for the header to "Verify after cutover".
### WR-02: The HTTP block redirects ACME challenges, and the app 404s `/.well-known/`
**Repo:** sm-summercmsio-app
**Files modified:** `deploy/nginx/summercms.io.conf`, `DEPLOY.md`
**Commit:** 1e934ab
**Applied fix:**
- The port-80 server now serves `location ^~ /.well-known/acme-challenge/` from `root /var/www/letsencrypt`. Every other path still gets the 301 to the HTTPS apex, now from `location /`.
- Cutover step 2 now checks the lineage's `authenticator` and `webroot_path` and says what to do for webroot and for nginx.
- Step 3 now runs `certbot renew --cert-name summercms.io --dry-run` after the reload.
- The webroot path is a default and is marked *check on rome*.
- The config comment avoids the literal `/etc/letsencrypt` path, because `check-deploy.sh` refuses any `/etc/letsencrypt` string it has not replaced.
### WR-04: The site test gate and the `pnpm test` script only work on Node 22.18 to 22.x
**Repos:** vue-summercmsio-app, summercms.go, sm-summercmsio-app (gitlink)
**Files modified:** `vue-summercmsio-app/package.json`, `vue-summercmsio-app/README.md`, `summercms.go/scripts/check-phase11.2.sh`, `sm-summercmsio-app` gitlink for `vue-summercmsio-app`
**Commits:** 60c3c4a (vue-summercmsio-app), be51bfe (summercms.go), aeac7bf (sm-summercmsio-app gitlink)
**Applied fix:**
- The test script is now `node --test --test-reporter=tap tests/*.test.ts`.
- `engines.node` is now `>=22.18`, and the README requirement line matches.
- In `run_site`, the informational summary grep is now `|| true`, so the explicit `refuse` checks report a missing TAP summary.
- The test command quoted in `11.2-VALIDATION.md` (lines 24 and 49) is now stale. That is a planning doc, so I left it for the orchestrator.
### WR-05: The smoke script can test a different server that already holds the port
**Repo:** sm-summercmsio-app
**Files modified:** `scripts/smoke.sh`
**Commit:** 8cfe055
**Applied fix:**
- Before `serve` starts, the script refuses a busy port. It requires curl exit 7 (connection refused) and fails on any other result, with a message that names `SMOKE_PORT`.
- The readiness loop now checks that `$PID` is alive before each curl.
- After the first response it waits 0.5 s, requires `$PID` to still be alive (a failed bind exits serve), and requires `listening on 127.0.0.1:$PORT` in `serve.log`.
### WR-06: A release build does not compile what its plugin tests checked, and does not require clean submodules
**Repo:** sm-summercmsio-app
**Files modified:** `scripts/build.sh`, `DEPLOY.md`
**Commit:** fffa38c
**Applied fix:**
- New `require_clean` step. It runs `git status --porcelain --ignore-submodules=none` on the app, the site and the plugin, so it also catches app gitlinks that differ from the checked-out submodule commits. A release build runs it before the build and again after `summer build`; the existing check on `main.go` and `plugins.gen.go` is kept.
- I left out the reviewer's `':!public'` exclusion. The plugin's `.gitignore` already ignores `public/site/` and `public/docs/`, and the exclusion would also have hidden edits to the tracked `public/README.md`.
- The temp release `go.work` is now written by `write_release_work` after the tag export (step 2). In release mode, step 3 runs the plugin tests with `GOWORK="$TMP/go.work"`, the same file the final `go build` uses. Dev mode is unchanged.
- The build.sh header and the build paragraph in DEPLOY.md describe the new release rules.
## Skipped Issues
### WR-03: The D-40 terminal check puts GOBIN on PATH, so it does not prove the page's commands work in a fresh shell
**File:** `/media/nvme/dev/golem15/summercms.io/summercms/sm-summercmsio-app/terminal_check_test.go:68-88,107-109` (copy: `vue-summercmsio-app/app/data/terminal.json:9,13`)
**Reason:** Needs a user decision (D-40: page copy changes require approval).
**Resolution (2026-10-01, user decision, option 1):** the page copy and the check stay as they are. `docs/setup/installation.md` (the card's "Installation guide" target) now explains that `go install` writes to `$(go env GOPATH)/bin` and shows `export PATH="$(go env GOPATH)/bin:$PATH"`; summercms.go commit `bd2f7e7`.
**Original issue:** `terminalEnv` prepends the temp GOBIN to PATH, so `TestTerminalCommands` passes even though `summer build` after `go install ./cmd/summer` fails in a common fresh shell where `$(go env GOPATH)/bin` is not on PATH. The options are to change the check so it fails today and documents the gap, or to make the copy PATH-independent with approval.
## Verification
All checks ran in the main checkouts (`workflow.use_worktrees: false`), so they can be reproduced from the trees as committed.
| Fix | Check | Result |
|-----|-------|--------|
| CR-01, WR-01, WR-02 | `scripts/check-deploy.sh` (nginx -t on the edited config, supervisor parse) after each edit | `check-deploy: ok` |
| WR-04 | `pnpm test` in vue-summercmsio-app (Node v22.23.2) | `# tests 30`, `# pass 30`, `# fail 0` |
| WR-04 | `bash scripts/check-phase11.2.sh --site` | `phase11.2 site passed` |
| WR-05 | `scripts/smoke.sh` on the built binary | `smoke: ok` |
| WR-05 | the same, with `python3 -m http.server 18095` holding the port | refused: `port 18095 is already in use (curl exit 0)` |
| WR-06 | `scripts/build.sh dev` | `bin/summercms-io (dev) ready` |
| WR-06 | release rehearsal while the app was dirty, against a scratch framework clone with a local `v0.1.0` tag | refused, listing the dirty files |
| WR-06 | release rehearsal after the commit | `bin/summercms-io (release v0.1.0) ready`; plugin tests passed under the release go.work; `go version -m` shows `git.golem15.com/golem15/summercms v0.1.0` |
| WR-06 | release rehearsal with a tracked edit in the plugin submodule (reverted afterwards) | refused: ` M plugins/golem15/summercms` |
| All | `bash scripts/check-phase11.2.sh --all` in summercms.go | `phase11.2 all passed` (framework, plugin, app, site, built, smoke, deploy, terminal and full stages) |
| All | `go vet ./... && go test ./...` in summercms.go | green |
- The scratch framework clone with the local tag was deleted. No real `v0.1.0` tag was created.
- The last gate run left `bin/summercms-io` as a dev build.
- `SummerCMS landing page.zip` was already untracked in summercms.go before this run, and I did not touch it.
- The new nginx directives were checked with `nginx -t` only. The HSTS header, the ACME webroot and the certbot dry run can only be checked on rome at cutover; DEPLOY.md now includes those steps.
---
_Fixed: 2026-10-01T15:11:41Z_
_Fixer: Claude (gsd-code-fixer)_
_Iteration: 1_

2
.planning/phases/11.2-ready-to-share-summercms-io-website-and-newsletter-plugin/11.2-VALIDATION.md
View File

@@ -21,7 +21,7 @@ validated: "2026-10-01"
| Property | Value |
|----------|-------|
| **Framework** | Go `testing` (stdlib, `httptest`, `testing/fstest`) in summercms.go, sm-summercmsio-plugin and sm-summercmsio-app; `node:test` in vue-summercmsio-app |
| **Config file** | none; `vue-summercmsio-app/package.json` script `"test": "node --test tests/*.test.ts"` |
| **Config file** | none; `vue-summercmsio-app/package.json` script `"test": "node --test --test-reporter=tap tests/*.test.ts"` |
| **Quick run command** | touched repo: `go vet ./... && go test ./...`; site: `pnpm generate && pnpm test` |
| **Full suite command** | framework `go vet ./... && go test ./...`; app `scripts/build.sh dev` then `go vet ./... && go test ./...` (plugin with `SUMMERCMS_REQUIRE_BUILD=1`); site `pnpm generate && pnpm test` |
| **Phase gate** | `scripts/check-phase11.2.sh --all` (stages `--framework`, `--plugin`, `--app`, `--site`, `--built`, `--smoke`, `--deploy`, `--terminal`, `--full`) |