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

docs(34): code review fix report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-29 22:37:01 +01:00
parent 75a0dea0a5
commit 7b60c17138
1 changed files with 157 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

157
.planning/phases/34-plugin-marketplace/34-REVIEW-FIX.md Normal file
View File

@@ -0,0 +1,157 @@
---
phase: 34-plugin-marketplace
fixed_at: 2026-10-29T22:45:00Z
review_path: .planning/phases/34-plugin-marketplace/34-REVIEW.md
iteration: 1
findings_in_scope: 13
fixed: 13
skipped: 0
status: all_fixed
---
# Phase 34: Code Review Fix Report
**Fixed at:** 2026-10-29T22:45:00Z
**Source review:** .planning/phases/34-plugin-marketplace/34-REVIEW.md
**Iteration:** 1
**Scope:** Critical + Warning (CR-01, CR-02, WR-01..WR-11). Info findings IN-01..IN-09 were not in scope.
**Summary:**
- Findings in scope: 13
- Fixed: 13 (CR-01, CR-02 and WR-10 change version or compatibility logic: fixed, requires human verification)
- Skipped: 0
Repositories:
- framework: `summercms.go` (branch master)
- site plugin: `sm-summercmsio-app/plugins/golem15/summercms` (repo `sm-summercmsio-plugin`, branch master)
- app: `sm-summercmsio-app` (branch master; only `scripts/changelog-post.py` and its test were staged, the two submodule gitlinks were left uncommitted)
Nothing was pushed, tagged or rebased.
## Fixed Issues
### CR-01: `plugin:update` downgrades module installs pinned at a pseudo-version
**Status:** fixed: requires human verification (version ordering logic)
**Repo:** summercms.go
**Commit:** 6055148
**Files modified:** `modules/kiosk/version.go`, `modules/kiosk/version_test.go`, `modules/kiosk/README.md`, `internal/build/update.go`, `internal/build/install.go`, `internal/build/install_test.go`, `modules/cabana/plugins.go`, `modules/cabana/plugins_test.go`, `modules/cabana/README.md`, `docs/plugins/marketplace.md`
**Applied fix:** New exported `kiosk.Newer(release, installed)` and `kiosk.Comparable(installed)`. A release, a `git describe` string (base tag) and a Go pseudo-version are compared in Go's version order. A pseudo-version counts as newer than the tag it builds on, so `v0.4.1-0.<ts>-<hash>` gets `v0.4.1` and later and never `v0.4.0`. `installedSource` keeps the go.mod version instead of blanking a pseudo-version. `plugin:update` uses `kiosk.Newer` in `pick` and refuses a module whose version `kiosk.Comparable` rejects (`vX.0.0-<ts>-<hash>`, a pre-release); the note names `go get <module>@<tag>`. The requirement checks (`installedVersion`, `checkRequires`) still see `""` for a pseudo-version, as before. The cabana badge (`applyUpdate`) uses the same `kiosk.Newer` rule. Regression tests: `TestUpdatePseudoVersion` (fails without the fix), `TestVersions/Newer`, and new cases in the cabana "update rule" table.
### CR-02: `plugin:install` silently moves already-installed plugins that share a Go module
**Status:** fixed: requires human verification (resolution logic)
**Repo:** summercms.go
**Commit:** 9b72439
**Files modified:** `internal/build/install.go`, `internal/build/source.go`, `internal/build/install_test.go`, `internal/build/install_smoke_test.go`, `docs/plugins/marketplace.md`
**Applied fix:** `PlanInstall` records every module in summer.yaml (`installedModules`, from `installedSource`). In `choose`, a release in such a module is eligible only at the version the application has. The step is marked `existing`, so `ApplyInstall` runs no `go get` and no clone for it and only appends the summer.yaml entry. This holds with `--source` too, whichever way the module is installed. A submodule checkout must contain the package. When no release matches, or the version is not a release (pseudo-version, untagged checkout, local directory), the install fails with a `plugin:update <installed id>` hint. A git URL whose repository is already a submodule (matched against `.gitmodules` before cloning), or whose module is already installed, is refused. Tests: `TestInstallSharedModule` (unit) and `TestInstallSmoke/source_shared_module` (fixture repos: no second submodule, no clone, git URL refused before cloning).
### WR-01: Generated `main.go`/`plugins.gen.go` require framework >= v0.6.0 with no guard
**Repo:** summercms.go
**Commit:** 07673e4
**Files modified:** `internal/build/build.go`, `internal/build/plugindirs.go`, `internal/build/plugindirs_test.go`, `docs/architecture/go-modules-and-workspaces.md`
**Applied fix:** I chose a static check over comparing versions. A local checkout reports the previous tag until it is tagged, so a version compare can be wrong. `resolveBuildMeta` resolves the framework's `party` package with the same `go list` the metadata step uses (it honours go.mod replaces and go.work) and parses it for `func Describe` and `type Info`. If either is missing, `buildMeta.Legacy` is set: `main.go` is written without the `party.Describe` call, `plugins.gen.go` byte-for-byte as before v0.6.0 (imports and `PluginIDs` only), and a warning names v0.6.0. If the package cannot be resolved, the current sources are generated. I verified this by hand: `examples/hello` with go.mod and the plugins' go.mod replacing the framework with a `git archive v0.5.3` export builds with the new `summer build`. Test: `TestLegacyFramework` (old party package → legacy sources and the exact old plugins.gen.go; new party package → Describe; failing go list → current sources).
### WR-02: Install and update are not atomic; a failure leaves the app half-modified
**Repo:** summercms.go
**Commit:** 9977e31
**Files modified:** `internal/build/rollback.go` (new), `internal/build/install.go`, `internal/build/source.go`, `internal/build/update.go`, `internal/build/install_smoke_test.go`, `docs/plugins/marketplace.md`
**Applied fix:** `snapshotApp` records go.mod, go.sum, go.work, go.work.sum, summer.yaml and .gitmodules, plus the index entry of .gitmodules, before `ApplyInstall` and `updater.apply` change anything. On failure, `undo` restores those files. It also removes each submodule added in the run (index entry via `update-index --force-remove`, the `submodule.<path>` config section, the absorbed `.git/modules/<path>` and the checkout), and checks moved submodules out at their previous branch or commit. `applySubmodule` registers the target for cleanup right after the rename. Anything the rollback cannot undo is appended to the error with the command that finishes the job. Test: `TestInstallSmoke/rollback`. It covers install (a submodule added, then a failing `go get`: files, git status and `.git/modules` restored, and the next install succeeds) and update (a submodule moved to v0.1.1, then a failing `go get`: back at v0.1.0, go.mod restored).
### WR-03: Credentials in a user-supplied https git URL end up in `.gitmodules` and output
**Repo:** summercms.go
**Commit:** 2f3eba0
**Files modified:** `internal/build/source.go`, `internal/build/install_smoke_test.go`, `docs/plugins/marketplace.md`
**Applied fix:** `validateGitURL` refuses any user info in an `https://` URL; the error points to a credential helper. ssh and scp-like users are still accepted. A password is still refused in any URL. The refusal happens before the "Cloning" line is printed. Test: new refusals in `TestInstallSmoke/urls` (`https://ghp_token@...`, `https://oauth2@...`, `ssh://git:secret@...`).
### WR-04: changelog-post sends the journal token to the registry endpoint
**Repo:** sm-summercmsio-app
**Commit:** 72bce11
**Files modified:** `scripts/changelog-post.py`, `scripts/test_changelog_post.py`
**Applied fix:** The journal-token fallback is removed. `registry_credential()` reads only `~/.config/summercms/registry-token`. Without that token, `request_ingest()` sends no request and warns that the hourly ingest will pick the tag up. The module docstring now says the journal token never goes to the registry. Tests: `test_without_a_registry_token_nothing_is_sent` (missing and empty file), plus the updated accepted and file-mode tests.
**Follow-up (not done here):** the user asked that only the script and its test be staged, so these app docs were left unchanged and still describe the old fallback: `README.md` (line 66, "else the journal token"), `DEPLOY.md` (lines 129 and 404) and `CLAUDE.md` (line 78, phrasing is still accurate). They need a docs commit in sm-summercmsio-app.
### WR-05: The `registry` token scope covers all of `golem15.summercms.*`, not just ingest
**Repo:** sm-summercmsio-plugin (site plugin submodule)
**Commit:** e82d627
**Files modified:** `token_scopes.go`, `controllers/api/registry.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `registry_api_test.go`, `registry_smoke_test.go`, `README.md`
**Applied fix:** The scope's permissions are now `controllers.PermissionRegistryIngest` only, and the ingest endpoint's `ingestCover` is that permission. Scopes are resolved by name on every request, so existing `registry` tokens narrow without being minted again. An administrator without the ingest permission is no longer offered the scope (the mint answers 422); the tests were adapted to that. Regression: a superuser's registry token gets 403 on `GET`/`POST /backend/api/v1/golem15/summercms/repositories`. A token whose owner later loses the permission still gets 403 on ingest. The scope description strings say the token allows nothing else.
### WR-06: The admin Plugins screen's search cache grows without bound
**Repo:** summercms.go
**Commit:** 7b19303
**Files modified:** `modules/cabana/plugins.go`, `modules/cabana/plugins_test.go`, `modules/cabana/README.md`, `docs/plugins/marketplace.md`
**Applied fix:** `storeList` caps `pluginCatalog.lists` at `marketplaceCacheSize` (256). When a new key arrives and the cache is full, it drops the expired searches first, then the entries that expire soonest. Test: "search cache is bounded" (306 distinct searches leave 256, the oldest is gone, the newest is still a hit, an expired cache shrinks to the new entry).
### WR-07: Disabling a repository does not withdraw its plugins from the public registry
**Repo:** sm-summercmsio-plugin
**Commit:** 8a2f0fa
**Files modified:** `registry/query.go`, `registry_api_test.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `README.md`
**Applied fix:** A shared `public()` scope (published, and `repository_id IN (enabled repositories)`, as a subquery so joined columns stay unambiguous) is applied in `List`, `Versions` and `Detail`. Disabling a repository removes its plugins from the list, the detail and the versions at once. Enabling it again brings them back with their presentation intact. The `enabled_comment` strings (en and pl) and the README describe this kill switch. Test: `TestRegistryAPIShapes/disabled_repository_withdraws_its_plugins` (Docker).
### WR-08: A transient ingest failure leaves a stale `last_error` forever
**Repo:** sm-summercmsio-plugin
**Commit:** a50c47c
**Files modified:** `registry/ingest.go`, `registry_smoke_test.go`, `README.md`
**Applied fix:** `recordError` now also clears `tags_digest`. The next run therefore reads the repository in full and `store` replaces `last_error` with that read's notes, or an empty string. Clearing the error on the unchanged path would also have wiped the skipped-tag notes; this way they survive without a new column. Test: an extension of `TestRegistrySmoke/unchanged`. An unreachable URL sets the error and clears the digest; restoring the URL triggers a full read that clears the error; the run after that is unchanged again.
### WR-09: The hourly `IngestAll` runs inside one 300 s job and starves later repositories
**Repo:** sm-summercmsio-plugin
**Commit:** 0b3fbf4
**Files modified:** `console/registry.go`, `schedule.go`, `registry_smoke_test.go`, `README.md`
**Applied fix:** `summercms:registry-ingest --queue` queues one `registry.IngestArgs` job per enabled repository, or for the one named, and prints `queued` for each. The job is unique per repository while it waits or runs, so repeats do not double up. A disabled repository and `--queue --force` are refused. The hourly schedule now runs the command with `--queue`, so each repository has its own job deadline and retries, and ingest-now jobs wait for at most one repository. Run by hand without the flag, the command still ingests directly as before. Test: `TestRegistrySmoke/scheduled_queue` (schedule args, 2 jobs for 2 enabled repositories, still 2 after repeats, refusals).
### WR-10: Plugins screen and `plugin:install` disagree on compatibility for unknown or dev framework versions
**Status:** fixed: requires human verification (compatibility semantics)
**Repo:** summercms.go
**Commits:** 754ab54, 75a0dea (smoke test expectation)
**Files modified:** `modules/cabana/plugins.go`, `modules/cabana/plugins_test.go`, `modules/cabana/plugins_smoke_test.go`, `modules/cabana/README.md`, `docs/plugins/marketplace.md`
**Applied fix:** The JSON shape is unchanged, so the SPA and the OpenAPI schema stay as they are (`check-admin-openapi.sh --check` passes). When the built framework version is not a release (devel, empty, a hash), `Compatible` is true, so no marker is shown, and the Installed tab gets no `LatestFramework` note. When the newest release is too new, `olderCompatible` looks up the plugin's releases (cached, four at a time). It marks the plugin compatible if `kiosk.NewestCompatible` finds an older release that fits, which is the one `plugin:install` would pick. The 75a0dea commit updates the existing Plugins screen smoke test, whose fixture has exactly that case. Test: "marketplace compatibility".
### WR-11: `plugin:add` now fails on summer.yaml files with an inline `plugins:` list (regression)
**Repo:** summercms.go
**Commit:** e6b3aa2
**Files modified:** `internal/build/manifest.go`, `internal/build/scaffold.go`, `internal/build/install.go`, `internal/build/manifest_append_test.go`, `internal/build/install_smoke_test.go`, `docs/console/scaffolding.md`, `docs/plugins/marketplace.md`
**Applied fix:** `insertManifestPlugins` finds the end of an inline flow list, on one line or several, skipping quotes and comments. It replaces those lines with a block list of the existing items plus the new ones and keeps every other line and comment. A list it cannot edit in place (a tagged `!!seq [...]`, say) falls back to the old full `SaveManifest` rewrite. `appendManifestPlugins` then reports `rewrote`, and `plugin:add` and `plugin:install` print a note that comments were not kept. The edit is still parsed back and verified, and the original bytes are restored on mismatch. Tests: new inline cases in `TestAppendManifestPlugins` (single line with a trailing comment, multi-line with comments, fallback rewrite). The old "inline refused" expectations were replaced.
## Verification
All checks ran in the main checkouts; `workflow.use_worktrees` is false, so no isolated worktree was created.
summercms.go:
- `go vet ./...`: pass
- `go test -short ./...`: pass (the first run failed on `TestPluginsScreenSmoke/marketplace`, whose expectation predated WR-10; 75a0dea fixed it, then the suite passed)
- `go test ./internal/build` without `-short` (fixture repos, module proxy): pass
- `cd examples/hello && go test ./...`: pass
- `go run ./cmd/summer docs:build --check`: no problems found
- `scripts/check-admin-openapi.sh --check`: pass. `admin/` was not changed, so neither `npm --prefix admin run test` nor a dist rebuild was needed; the gate's admin stage still ran vitest (1622 passed), typecheck and `check-admin-dist.sh` (dist matches a fresh build)
- Manual WR-01 check: hello against a `git archive v0.5.3` framework export builds with the new `summer build` (legacy sources plus the warning)
Site plugin (sm-summercmsio-plugin):
- `go vet ./... && go test -short ./...`: pass
- `go test ./...` with Docker (testcontainers Postgres): pass
sm-summercmsio-app:
- `go vet ./... && go test ./...`: pass
- `python3 -m pytest -q scripts/test_changelog_post.py`: 51 passed. `python3 -B -m unittest discover -s scripts -p 'test_*.py'`: 51 OK. A `scripts/__pycache__` left by pytest was removed.
`scripts/check-phase34.sh --all` with Docker available: **Phase 34 gate passed**. Stages self-test, go, plugins (20 repositories), site (including pnpm test, build:changelog, test:changelog and test:marketplace), admin, docs, marker, additive (cabana +43 lines, party +15, kiosk new) and docker (framework `go test ./...`, `TestInstallSmoke`, and the site plugin's `TestRegistrySmoke`, `TestRegistryAPIShapes` and `TestRegistryIngestEndpoint`) all PASS.
Not committed, as instructed: the app's two submodule gitlinks (`plugins/golem15/summercms` now points at the new site-plugin commits; `vue-summercmsio-app`). Other sessions' edits (`.planning/STATE.md`, `state.json`, `G15OFFICE.md`, `.msd/`, other phases' `.gitkeep`) were not touched.
---
_Fixed: 2026-10-29T22:45:00Z_
_Fixer: Claude (msd-code-fixer)_
_Iteration: 1_