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

docs(11): update code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-09-30 22:24:17 +02:00
parent 93c735b8a2
commit c6f6bdfcb8
1 changed files with 29 additions and 317 deletions
Download Patch File Download Diff File Expand all files Collapse all files

346
.planning/phases/11-jobs-realtime-and-search-infrastructure/11-REVIEW.md
View File

@@ -1,343 +1,55 @@
---
phase: 11-jobs-realtime-and-search-infrastructure
reviewed: 2026-09-30T13:16:12Z
reviewed: 2026-09-30T19:41:53Z
depth: standard
files_reviewed: 84
files_reviewed: 13
files_reviewed_list:
- cmd/summer/main.go
- cmd/summer/parity.go
- cmd/summer/runtime.go
- examples/hello/main.go
- ../fonoteka.go/config/push.yaml
- ../fonoteka.go/config/queue.yaml
- ../fonoteka.go/config/realtime.yaml
- ../fonoteka.go/config/search.yaml
- ../fonoteka.go/main.go
- ../fonoteka.go/parity/capture-rules.yaml
- ../fonoteka.go/parity/check_corpus.go
- ../fonoteka.go/parity/php_parity.sh
- ../fonoteka.go/parity/README.md
- ../fonoteka.go/plugins/golem15/fonoteka/classes/ws/collection_authorizer.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/ws/wishlist_authorizer.go
- ../fonoteka.go/plugins/golem15/fonoteka/models/album_search.go
- ../fonoteka.go/plugins/golem15/fonoteka/plugin.go
- ../fonoteka.go/plugins/golem15/fonoteka/realtime.go
- ../fonoteka.go/plugins/golem15/fonoteka/routes.go
- ../fonoteka.go/plugins/golem15/fonoteka/schedule.go
- ../fonoteka.go/plugins/golem15/fonoteka/search.go
- ../fonoteka.go/README.md
- internal/build/artifact.go
- internal/build/build.go
- internal/build/stubs/artifacts.tmpl
- modules/beachcomber/beachcomber.go
- modules/beachcomber/engines.go
- modules/beachcomber/README.md
- modules/beachcomber/searchable.go
- modules/beachcomber/sync.go
- modules/beachcomber/typesense/config.go
- modules/beachcomber/typesense/engine.go
- modules/bonfire/call.go
- modules/bonfire/README.md
- modules/bouncer/jwt.go
- modules/bouncer/README.md
- modules/compass/persist.go
- modules/compass/README.md
- modules/conga/client.go
- modules/conga/commands.go
- modules/conga/conga.go
- modules/conga/job.go
- modules/conga/README.md
- modules/conga/record.go
- modules/conga/schedule.go
- modules/conga/scheduler.go
- modules/conga/worker.go
- modules/flare/commands.go
- modules/flare/encrypt.go
- modules/flare/flare.go
- modules/flare/README.md
- modules/flare/vapid.go
- modules/lagoon/connection.go
- modules/lagoon/migrations.go
- modules/lagoon/ondatabase.go
- modules/lagoon/queue_migrations.go
- modules/beachcomber/sync_test.go
- modules/cabana/crud.go
- modules/cabana/relation.go
- modules/lagoon/README.md
- modules/lagoon/transaction.go
- modules/lighthouse/broadcast.go
- modules/lighthouse/centrifugo/client.go
- modules/lighthouse/centrifugo/commands.go
- modules/lighthouse/centrifugo/config.go
- modules/lighthouse/centrifugo/driver.go
- modules/lighthouse/centrifugo/handlers.go
- modules/lighthouse/centrifugo/token.go
- modules/lighthouse/channel.go
- modules/lighthouse/drivers.go
- modules/lighthouse/job.go
- modules/lighthouse/lighthouse.go
- modules/lighthouse/README.md
- modules/lighthouse/registry.go
- modules/lighthouse/route.go
- modules/lighthouse/suppress.go
- modules/lighthouse/users.go
- modules/pact/capabilities.go
- modules/pact/README.md
- modules/surf/README.md
- modules/surf/serve.go
- modules/tide/centrifugo.go
- modules/tide/centrifugo_golden.go
- modules/tide/README.md
- README.md
- scripts/check-phase10.1.sh
- modules/lagoon/transaction_test.go
- scripts/check-phase10.sh
- scripts/check-phase11.sh
- ../fonoteka.go/plugins/golem15/fonoteka/admin_albums_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/search_smoke_test.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go
- ../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service_db_test.go
findings:
critical: 1
warning: 7
info: 10
total: 18
status: issues_found
critical: 0
warning: 0
info: 0
total: 0
status: clean
---
# Phase 11: Code Review Report
**Reviewed:** 2026-09-30T13:16:12Z
**Reviewed:** 2026-09-30T19:41:53Z
**Depth:** standard
**Files Reviewed:** 84
**Status:** issues_found
**Files Reviewed:** 13
**Status:** clean
## Summary
I reviewed the Phase 11 code: the River job layer (conga, the lagoon queue migrations, OnDatabase and AfterCommit), the scheduler, realtime (lighthouse, the Centrifugo driver, the subscribe proxy and the fonoteka authorizers), search (beachcomber and Typesense), Web Push (flare), the parity tooling (tide broadcasts) and the fonoteka.go bindings. For files that already existed, I reviewed only the Phase 11 diff (`718a35c..HEAD`, and `3d4895d^..HEAD` in fonoteka.go). I compared behaviour against the PHP originals in `/media/nvme/dev/golem15/fonoteka` where parity was in question.
The three previously reported defects were re-reviewed against the current working tree. Lagoon now binds nested transaction reuse to the exact parent connection and tests an unrelated transaction handle; the Phase 10 gate now proves expected skips are observed and self-tests both missing and unexpectedly passing cases; and the Phase 11 gate now includes the exact transaction-edge test plus RC-15 removal coverage.
Several parts hold up:
- The subscribe proxy compares the secret in constant time and fails closed.
- Channel parsing and the collection and wishlist authorizers match the PHP rules exactly.
- Broadcast jobs are enqueued on the write's `*sql.Tx` inside a savepoint.
- The River payload travels as a JSON string, so its key order survives JSONB.
- VAPID and Typesense keys are kept out of logs and error messages.
- The push endpoint allowlist rejects non-https URLs, URLs with user info and redirects.
All reviewed fixes meet quality standards. No open issues found.
The main defect is the order of commit and sync:
- `lagoon.AfterCommit` runs its callback immediately inside a plain `gorm` transaction.
- The framework's own admin CRUD (cabana) and the fonoteka `SaveAlbum` both use plain transactions.
- As a result, Typesense is updated before the commit and before the belongs-to-many pivots are written. The index keeps stale `artist_ids` after a successful edit, and keeps changes the database rolled back.
## Narrative Findings (AI reviewer)
Other issues:
- Framework API traps: a poisoned query handle passed to broadcast callbacks, identifier tokens that the proxy's PHP (int) cast turns into user ids, and the misleading nested `lagoon.Transaction` contract.
- Scheduled commands that open their own database fail inside a running worker.
- The VAPID `--update` flow writes the private key into the app's repository tree, which is not gitignored.
- The subscribe proxy shares its rate limit bucket with public traffic.
No open findings.
## Critical Issues
## Resolved Findings
### CR-01: The search index syncs mid-transaction, before pivots are written, and diverges from committed data
**File:** `modules/lagoon/transaction.go:100-117`, `modules/beachcomber/sync.go:86-90` (callers: `modules/cabana/crud.go:311-373`, `../fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go:55-67`)
**Issue:** When `lagoon.AfterCommit` finds neither a `lagoon.Transaction` buffer nor a GORM-started transaction, it runs the callback immediately. That is always the case inside a plain `gdb.Transaction(...)`. beachcomber relies on `AfterCommit` for every Searchable write, so in a plain transaction `syncOne` reloads the row, builds the document and sends the Typesense upsert or delete in the middle of the transaction.
The framework's own admin write path is exactly this shape: `CRUDService` in cabana calls `s.DB.WithContext(ctx).Transaction(...)`. It then runs `tx.Save(target)` (sync fires here) and only afterwards `syncBelongsToMany(...)` and `formAfterUpdate(...)`.
Consequences for the fonoteka album admin form, which has an `artists` relation field:
1. After a successful edit that changes artists, `ToSearchableArray` has already read the old `golem15_fonoteka_album_artists` rows. The document keeps stale `artist_ids`, and nothing repairs it until the album is saved again.
2. When `syncBelongsToMany`, the after hook, or a BulkDelete row fails after an earlier row was synced, the database rolls back, but Typesense already holds the upsert or has lost the document.
3. The Typesense HTTP call, bounded at 2s, runs while the transaction holds the row locks.
`SaveAlbum` has the same order: `tx.Save(album)` runs before `syncArtists`. Unlike PHP `AlbumWriteService`, it also never re-saves after the pivot sync; PHP calls `$album->save()` / `touch()` after syncing. The `updated` broadcast payload built in the same callback (`realtime.go:112`) also carries the pre-edit artist list.
This behaviour is documented and tested ("a plain gorm transaction syncs immediately"), but the documented design contradicts the beachcomber contract ("built from the committed row") on the framework's main write path.
**Fix:** Route framework and app writes through `lagoon.Transaction` so after-commit work waits for the commit:
```go
// modules/cabana/crud.go (and BulkDelete/Delete/relation.go writes)
err := lagoon.Transaction(ctx, s.DB, func(ctx context.Context, tx *gorm.DB) error {
// ... unchanged body, using tx
})
// fonoteka classes/album_write_service.go
return lagoon.Transaction(ctx, gdb, func(ctx context.Context, tx *gorm.DB) error {
// validate, ResolveArtists, tx.Save(album), syncArtists(...)
})
```
Separately, make `AfterCommit` refuse to run inside a foreign `*sql.Tx`: log at Warn and skip, or return an error, instead of running immediately. This way a plain transaction can never push uncommitted state to an external system. Add a cabana admin-edit test that changes the artists and asserts the indexed `artist_ids`.
## Warnings
### WR-01: Broadcast channel and payload callbacks get a handle that continues the write's statement after `WithContext`
**File:** `modules/lighthouse/broadcast.go:443-449`
**Issue:** `inSavepoint` builds the handle it passes to `Broadcastable.BroadcastChannels`, `BroadcastPayload` and the `Binding` functions with `db.Session(&gorm.Session{NewDB: true, Context: ctx})`. `lagoon/transaction.go:155-166` documents exactly why this is unsafe on a callback's handle:
- the `Context` option clones the write's statement (table, WHERE and SET clauses);
- the usual idiom `tx.WithContext(ctx)` calls `Session{Context}` again with `NewDB` false (`clone = 2`), so the next chained call continues from that clone.
A channel function written as `tx.WithContext(ctx).Model(&Collection{}).Where("id = ?", m.CollectionID).Take(&c)` therefore queries the written model's table, with the write's primary key condition added. The broadcast is then silently skipped, or it computes the wrong channels. The fonoteka bindings avoid `WithContext` by chance; the framework API does not protect against it. beachcomber already uses the clean-handle fix.
**Fix:** Reuse the clean-handle construction:
```go
tx := db.Session(&gorm.Session{NewDB: true, Context: ctx}).Clauses().Session(&gorm.Session{NewDB: true})
```
(or export `lagoon.CleanHandle` and use it in lighthouse and beachcomber). Add a test whose channel function starts with `tx.WithContext(ctx)`.
### WR-02: Scheduled commands that open their own database fail inside a running worker
**File:** `modules/conga/commands.go:215-225`, `modules/conga/scheduler.go:147-161`
**Issue:** The scheduler runs catalog commands in process (`cat.Call`) inside `serve`, `queue:work` or `schedule:run`. All three have already published `*sql.DB` and `*gorm.DB`. Commands built on the `withDB` pattern then run `OpenFromApp` (a second pool) followed by `lagoon.Publish`, and `backpack.Registry.Publish` returns `duplicate provider for *sql.DB`. This affects conga's own `queue:clear`, lagoon's migrate commands (`lagoon/commands.go:78-88`) and any plugin command copied from them, so scheduling one of them always fails. `scheduleRunOnce` works around this for itself with `withAppDB`, but the commands it calls do not.
**Fix:** Make `withDB` in conga (and lagoon) reuse a published handle, the way `withAppDB` and `cabana.withAdminDB` already do:
```go
func withDB(ctx context.Context, app *backpack.App, fn func() error) error {
if gdb, ok := app.Lookup[*gorm.DB](); ok && gdb != nil {
return fn()
}
// ... open, publish, defer close
}
```
Add a scheduler test that runs a DB-using command while the database is published.
### WR-03: A nested `lagoon.Transaction` over the root handle is an independent transaction, but its callbacks wait on the parent
**File:** `modules/lagoon/transaction.go:54-65`
**Issue:** The nested branch is chosen from the context buffer alone. If the inner call receives the app's root `*gorm.DB` rather than the outer `tx`, `gdb.WithContext(childCtx).Transaction` opens a new, independent transaction that commits on its own. This is common when a service takes `(ctx, app DB)`. The child's AfterCommit callbacks are nevertheless moved into the parent buffer:
- if the parent rolls back, the callbacks are dropped although the child's writes are committed, so search sync or blob deletes are lost;
- the child's writes are not rolled back with the parent, contrary to the documented "a nested Transaction becomes a savepoint".
**Fix:** Take the nested path only when `gdb` is actually inside a transaction:
```go
if parent, ok := ...; ok && parent != nil {
if _, inTx := gdb.Statement.ConnPool.(gorm.TxCommitter); inTx {
// savepoint path (current code)
}
// otherwise: treat as a top-level transaction with its own buffer
// (run its callbacks after its own commit), or return an error.
}
```
### WR-04: `websockets:generate-vapid-keys --update` writes the VAPID private key into the application's repository tree
**File:** `modules/flare/commands.go:182-193`; `../fonoteka.go/main.go` (`compass.Load("config")`); `../fonoteka.go/.gitignore`
**Issue:** `saveKeys` persists `push.private_key` through `compass.Persist` to `<config dir>/env/<env>/overrides.yaml`. fonoteka.go loads `config` relative to the working directory, so running the command from the repository root writes the private key to `fonoteka.go/config/env/<env>/overrides.yaml`. Neither fonoteka.go nor the framework `.gitignore` ignores that path, so the next `git add -A` commits the key. `config/push.yaml` explicitly warns against committing it.
**Fix:** Add `config/env/*/overrides.yaml` to the application `.gitignore` (and to the scaffolded app template). The command could also print a warning, or refuse, when the destination is inside a git work tree and not ignored (`git check-ignore`).
### WR-05: Identifier connection tokens are authorized as user ids by the subscribe proxy
**File:** `modules/lighthouse/centrifugo/token.go:86-107`, `modules/lighthouse/centrifugo/handlers.go:147-152`
**Issue:** `ForIdentifier` and `SubscriptionForIdentifier` sign tokens whose `sub` is an arbitrary non-user identifier. Centrifugo forwards that `sub` as `user` to the subscribe proxy, which converts it with `lighthouse.PHPInt`, the PHP `(int)` cast. An identifier such as `"42-guest"`, `"7abc"` or a UUID that starts with digits (`"42a1c9…"`) becomes user id 42 or 7. Every authorizer then evaluates the subscribe as that user, which lets a guest or device identifier subscribe to a real user's collection channels. The cast is PHP parity, but pairing it with a public identifier-token API is a privilege-confusion trap. There are no callers today.
**Fix:** Make the proxy accept only canonical decimal user ids: `strconv.ParseUint(user, 10, 64)` after checking the value round-trips. Deny or route any other `sub` to a separate identifier authorizer. Alternatively, require identifiers to carry a non-numeric prefix, validated in `ForIdentifier`. Document the rule in the README.
### WR-06: The default broadcast payload serializes the in-memory model: zero values on partial updates, and every exported field
**File:** `modules/lighthouse/broadcast.go:424-432`
**Issue:** Without a payload override, `prepare` marshals `t.model`, the value GORM wrote.
- For `db.Model(&T{ID: 5}).Updates(map[string]any{"name": "x"})` (a documented, non-zero-key path that *is* broadcast), the model holds only the id and the assigned columns. Subscribers receive `"owner_id":0`, `"name":""` and similar zero values as if they were the new state.
- The struct is marshalled with `encoding/json`, so every exported field without `json:"-"` is published, for example a password hash or an `Encrypted` column's JSON form. The WinterCMS `toArray()` honours `$hidden`.
The delete path already reloads the full row; the create and update paths do not.
**Fix:** For `ActionUpdated` (and `ActionCreated`), reload the row by primary key inside the savepoint before building the default payload, as `snapshot` does. Document that the default payload publishes every JSON-visible field, or require models that use the default payload to implement a serializer.
### WR-07: The Centrifugo subscribe proxy shares an IP-keyed rate limit bucket with public traffic
**File:** `../fonoteka.go/plugins/golem15/fonoteka/routes.go:86`, `../fonoteka.go/plugins/golem15/fonoteka/plugin.go:321-330`, `modules/lighthouse/route.go:571`
**Issue:** `Surfaces.Middleware` (`throttle:ws-api`) is appended to the ServerToServer subscribe route too. The request has no principal, so it is keyed `wsapi:ip:<ClientIP>` with 120 per minute. The throttle runs before the proxy's secret check, so requests with a wrong secret count against the bucket. With the shipped `trusted_proxies: []`, every request that reaches the app through a local reverse proxy has the same `RemoteAddr`. Any anonymous client can then send 120 bogus `POST /api/realtime/subscribe` per minute and exhaust the bucket Centrifugo's legitimate calls share. Centrifugo treats a 429 as an internal error, so every user's subscribes fail. PHP has the same throttle, but the Go port is the place to stop copying a denial-of-service vector.
**Fix:** Do not apply the user-facing bucket to ServerToServer routes. Either give `lighthouse.Surfaces` a separate `ServerToServerMiddleware`, or let `Mount` apply `Middleware` only to UserAuth and Public routes. If the proxy needs a limit, key it after the secret check, and document that `http.trusted_proxies` must be set behind a proxy.
## Info
### IN-01: Parity tooling cannot detect key-order drift, and several payload maps are unordered
**File:** `modules/tide/centrifugo_golden.go:197-235`; `../fonoteka.go/plugins/golem15/fonoteka/realtime.go:96`; `modules/lighthouse/centrifugo/handlers.go:211`
**Issue:** `DiffPublications` compares bodies structurally with key order ignored. As a result, the byte-for-byte order that `BroadcastArgs` carefully preserves is never asserted. `albumBroadcast.Album` and `Result.Info` are `map[string]any`, so `encoding/json` emits their keys alphabetically, whereas PHP emits insertion order (see `fixtures/broadcasts/created.yaml`).
**Fix:** Add an ordered comparison mode to `DiffPublications` for asserted goldens. When Phase 12 fills the album subtree, build it as an ordered struct.
### IN-02: The fallback to the in-memory album in `albumPayload` is unreachable
**File:** `../fonoteka.go/plugins/golem15/fonoteka/realtime.go:112-116`
**Issue:** The broadcast callback always runs inside a transaction, because GORM opens one for every write. A failed `Preload…Take` therefore aborts it, so the following `enqueue` fails and the savepoint is rolled back. `SerializeAlbum(m)` is never published.
**Fix:** Return the read error, or wrap the fresh read in its own savepoint if the fallback is intended.
### IN-03: The River scheduler and `schedule:run --once` disagree on DST days and sub-minute intervals
**File:** `modules/conga/schedule.go:272-286`, `modules/conga/commands.go:159-172`
**Issue:** `Every.Next` adds absolute durations to local midnight, while `dueAt` uses wall-clock minutes:
- On a 23-hour or 25-hour day, `Every(6h)` fires at 07:00, 13:00 and 19:00 local (or gains an extra 23:00 run that the 24h `ByPeriod` uniqueness may then drop).
- `--once` fires at 06:00, 12:00 and 18:00.
- `Every(90s)` fires every 90 seconds under River but every 3 minutes under `--once`.
**Fix:** Compute `Every` on wall-clock time (`time.Date(..., h, m, ...)`), and document or reject intervals that are not whole minutes.
### IN-04: `summer_jobs` rows can stay IN_PROGRESS forever
**File:** `modules/conga/commands.go:185-211`, `modules/conga/worker.go:243-262`
**Issue:** In these cases the row is never finalized:
- `queue:clear` deletes River jobs but leaves the linked rows at StatusInProgress;
- a job that River's rescuer discards, or one that returns `river.JobCancel` or `JobSnooze` on a non-final attempt, is never recorded either.
**Fix:** Mark linked rows StatusStopped in `clearQueue` (join on `river_job_id`), and handle `JobCancel` errors in `runAttempt`.
### IN-05: The centrifugo and typesense `Config` structs do not redact their secrets
**File:** `modules/lighthouse/centrifugo/config.go:477-498`, `modules/beachcomber/typesense/config.go:24-38`
**Issue:** `Driver.Config()` and `Engine.Config()` return structs holding `APIKey`, `TokenSecret` and `ProxySecret`, which `%v` or `slog.Any` print in full. `flare.Config` implements `String`, `GoString` and `LogValue` for exactly this reason. HS256 `token_secret` also has no minimum length.
**Fix:** Add the same redacting `String`, `GoString` and `LogValue` methods, and warn when `token_secret` is shorter than 32 bytes.
### IN-06: `parity:broadcasts --api-key` puts a secret on the command line
**File:** `cmd/summer/parity.go:84,112`
**Issue:** Flag values show up in `ps` output and shell history. The environment variable alternative already exists.
**Fix:** Drop the flag, or document it as test-only and prefer `PARITY_CENTRIFUGO_API_KEY`.
### IN-07: The removal harness leaves mutated security code behind on SIGTERM or SIGKILL
**File:** `scripts/check-phase11.sh:344-390`
**Issue:** The harness removes a protection from tracked source and restores it only in a Python `finally`. That block survives SIGINT and timeouts, but not SIGTERM (for example a CI job cancel) or SIGKILL. The file is then left with the protection removed.
**Fix:** Install a SIGTERM handler that restores the file (`signal.signal(signal.SIGTERM, ...)`), or mutate a scratch copy of the tree instead of the working tree.
### IN-08: `Service.Emit` drops the caller's context when `db` is nil
**File:** `modules/lighthouse/suppress.go:428-434`, `modules/lighthouse/job.go:83-93`
**Issue:** `enqueue` takes its context only from `db.Statement.Context`. With a nil `db`, the insert runs on `context.Background()`, ignoring the caller's cancellation or deadline.
**Fix:** Pass `ctx` through to `enqueue` explicitly.
### IN-09: The three modules detect a transaction in different ways
**File:** `modules/lighthouse/broadcast.go:449`, `modules/beachcomber/sync.go:266`, `modules/conga/conga.go:494-500`
**Issue:** lighthouse and conga test for `*sql.Tx`; beachcomber tests for `gorm.TxCommitter`. If PrepareStmt mode is ever enabled, the connection pool is `*gorm.PreparedStmtTX`, and lighthouse then inserts broadcast jobs outside the write's transaction, so rolled-back writes are published.
**Fix:** Share one helper that recognizes `gorm.TxCommitter` and unwraps `PreparedStmtTX` to its `*sql.Tx`.
### IN-10: `RecordBroadcasts` returns before the recorder has released its port
**File:** `modules/tide/centrifugo.go:229-233`
**Issue:** The deferred `cancel()` starts `Shutdown` asynchronously. A second recording on the same `--listen` address in the same process can then fail with "address already in use".
**Fix:** After `cancel()`, wait on `errCh` (with a short timeout) before returning.
- **Resolved CR-01 — nested Lagoon transaction ownership:** `afterCommitBuffer` records the parent `gorm.ConnPool`, `owns` requires exact connection-pool identity, and nested calls reject foreign transaction handles. `TestTransactionAfterCommit/nested_rejects_unrelated_transaction_handle` covers the regression. The focused Lagoon tests pass.
- **Resolved WR-01 — Phase 10 expected-skip enforcement:** the gate records observed skips, rejects both missing skips and unexpectedly passing pending tests, and includes self-tests for both failure modes. The gate self-test passes.
- **Resolved WR-02 — Phase 11 exact-name and removal coverage:** `TestTransactionEdges` is included in the exact-name list, and RC-15 mutates the parent-ownership guard and requires `TestTransactionAfterCommit` to catch its removal. The gate self-test and RC-15 removal check pass.
---
_Reviewed: 2026-09-30T13:16:12Z_
_Reviewer: Claude (gsd-code-reviewer)_
_Reviewed: 2026-09-30T19:41:53Z_
_Reviewer: the agent (gsd-code-reviewer)_
_Depth: standard_