docs(02): add code review report
This commit is contained in:
Jakub Zych
218
.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md
Normal file
218
.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md
Normal file
@@ -0,0 +1,218 @@
|
||||
---
|
||||
status: issues
|
||||
phase: 02-api-parity-harness-bootstrap
|
||||
depth: standard
|
||||
files_reviewed: 20
|
||||
files_reviewed_list:
|
||||
- tide/flow.go
|
||||
- tide/fixture.go
|
||||
- tide/record.go
|
||||
- tide/replay.go
|
||||
- tide/diff.go
|
||||
- tide/proxy.go
|
||||
- tide/rules.go
|
||||
- tide/variables.go
|
||||
- tide/normalize.go
|
||||
- tide/manifest.go
|
||||
- tide/report.go
|
||||
- cmd/summer/parity.go
|
||||
- cmd/summer/main.go
|
||||
- scripts/check-phase2.sh
|
||||
- ../fonoteka.go/parity/check_corpus.go
|
||||
- ../fonoteka.go/parity/php_parity.sh
|
||||
- ../fonoteka.go/parity/capture_clients.mjs
|
||||
- ../fonoteka.go/parity/parity_test.go
|
||||
- ../fonoteka.go/parity/synthetic_test.go
|
||||
- ../fonoteka.go/parity/parity_contract_test.go
|
||||
findings:
|
||||
critical: 1
|
||||
warning: 7
|
||||
info: 4
|
||||
total: 12
|
||||
reviewed: 2026-09-17T12:18:00Z
|
||||
---
|
||||
|
||||
# Phase 2: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-09-17T12:18:00Z
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 20
|
||||
**Status:** issues
|
||||
|
||||
## Summary
|
||||
|
||||
The harness is a real record/replay pipeline: loopback-pinned `ReverseProxy`, placeholder capture, Carbon/id normalizers, and a 154-route corpus with Phase 2 tests that keep PHP routes pending (never a Go pass). `scripts/check-phase2.sh` constructs a unique `fonoteka_parity_*` MariaDB and preflights artisan's active database before migrate.
|
||||
|
||||
The defect that breaks the acceptance test is not SSRF. Short captured numeric IDs are still substituted into JSON bodies and IPv4 literals, so `meta.total` / `current_page` / issuer hosts are bound to `{{id:*}}`. Replay then expands those placeholders from the live capture store. A Go port can return a wrong pagination count or a wrong last IPv4 octet and still match. That is a false-negative on PHP/Go mismatch.
|
||||
|
||||
Loopback bind/upstream and Host pinning look sound. DB isolation in the phase gate is stronger than `php_parity.sh`. Pending-vs-passing is honest in `parity_test.go` / `parity_contract_test.go`; the CLI table using `passing` for a PHP self-check of pending routes is by design.
|
||||
|
||||
## Critical Issues
|
||||
|
||||
### CR-01: Short captured IDs rewrite pagination and IPv4, hiding PHP/Go diffs
|
||||
|
||||
**File:** `tide/variables.go:498-555`
|
||||
**Issue:** `ScrubStep` runs every stored capture value through `replaceAll`. Values shorter than 8 characters use `replaceIsolated`, which treats `.`, `:`, and `,` as boundaries. A captured id `1` therefore rewrites:
|
||||
|
||||
- `"total":1` / `"current_page":1` / `"last_page":1` / `"album_count":1`
|
||||
- `127.0.0.1` → `127.0.0.{{id:…}}`
|
||||
|
||||
`normalize.go` already masks `id` / `*_id` at compare time. Global numeric replace is redundant for those keys and destructive for every other isolated `1`.
|
||||
|
||||
On replay (`tide/replay.go:54-74`) `CaptureStep` writes live IDs into the store, then `expandResponse` fills the expected body from that same store. Expected `meta.total` becomes the live album/user id, not the recorded total. If Go returns `total: 5` and the captured id is also `5`, the count mismatch is reported as a pass.
|
||||
|
||||
This is present in committed fixtures the validators do not inspect as JSON (examples referenced only):
|
||||
|
||||
- `../fonoteka.go/parity/fixtures/nuxt/nuxt-browse.yaml` — `"meta":{"current_page":{{id:album}},"last_page":{{id:album}},"per_page":8,"total":{{id:album}}}`
|
||||
- `../fonoteka.go/parity/fixtures/mcp/mcp-oauth.yaml` — `"issuer":"http:\/\/127.0.0.{{id:album}}:8423"`
|
||||
- `../fonoteka.go/parity/fixtures/routes/GET__api_v1_fonoteka_albums_personal_token.yaml` — `"total":{{id:token}}`
|
||||
|
||||
Phase 02-03 SUMMARY called this "fixed" because seed IDs happen to be `1`. That makes PHP self-replay green and leaves the Go port blind.
|
||||
|
||||
**Fix:** Do not globally substitute numeric capture values into JSON bodies. Keep path/query/header replace for `/albums/1`. Rely on `normalizeJSON` for `id`/`*_id`. Refuse isolated replace when the left-hand byte is `.` (IPv4) or when the JSON key is not an id field:
|
||||
|
||||
```go
|
||||
func replaceAll(s string, pairs [][2]string) string {
|
||||
for _, p := range pairs {
|
||||
if p[0] == "" {
|
||||
continue
|
||||
}
|
||||
numeric := isAllDigits(p[0])
|
||||
if numeric && len(p[0]) < 8 {
|
||||
// Request path/query only; callers should pass a kind.
|
||||
continue
|
||||
}
|
||||
// ...
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
func isIdentByte(c byte) bool {
|
||||
return (c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') ||
|
||||
(c >= 'a' && c <= 'z') || c == '_' || c == '.'
|
||||
}
|
||||
```
|
||||
|
||||
Recapture affected fixtures after the scrubber change. Add a test: store `id:album=1`, scrub `{"id":1,"total":1,"host":"127.0.0.1"}`, assert `total` and the IPv4 literal stay literal `1`.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Fixture save is neither exclusive nor all-or-nothing
|
||||
|
||||
**File:** `tide/proxy.go:179-328`, `tide/fixture.go:50-91`, `tide/manifest.go:376-409`
|
||||
**Issue:** Duplicate-session and `--update` guards are `os.Stat` then later `SaveFlow` → `os.Rename`, which overwrites the destination. Two `parity:proxy` / `parity:record` processes can clobber a committed YAML. `recordStep` writes after every successful step; `failSession` only deletes when `len(buf.steps) == 0`. Flush's comment ("Failed sessions leave no new fixture") is false once step 1 has been committed. A later unclassified-credential or overflow error leaves a partial session file that the next run then rejects as a duplicate.
|
||||
|
||||
**Fix:** Open the final path with `O_EXCL` when not `--update`. Buffer the whole session in memory and `SaveFlow` once in `Flush`. On failure, `os.Remove` the session path regardless of step count (or write only to a `.tmp` name until success).
|
||||
|
||||
### WR-02: Redirect `Location` is recorded but never compared
|
||||
|
||||
**File:** `tide/diff.go:29-84`, `tide/record.go:121-136`
|
||||
**Issue:** `Location` is in `recordedHeaderNames` (and in capture-rules `keep_response_headers`) so OAuth 302s persist it. `globalCompareHeaders` / `extraCompareHeaders` omit `Location`. `compareHeaders` only diffs allow-listed names present on the expected side. A Go port that returns 302 with a different path/host and an empty body will pass. D-14 excluded Date/Server by name; it did not say to skip redirect targets that PKCE depends on.
|
||||
|
||||
**Fix:** Add `Location` to `extraCompareHeaders` (after placeholder expansion). Keep `Set-Cookie` out if cookie timestamps churn; compare the Location path/host even when the `code` query is a placeholder.
|
||||
|
||||
### WR-03: JSON decoder stops after the first value
|
||||
|
||||
**File:** `tide/diff.go:125-133`
|
||||
**Issue:** `decodeJSON` uses `Decoder.Decode` and does not check `More()`. Trailing tokens after a valid JSON value are ignored. PHP `{"data":[]}{"debug":true}` versus Go `{"data":[]}` compares equal. That is a false-negative on envelope shape.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```go
|
||||
func decodeJSON(raw []byte) (any, error) {
|
||||
dec := json.NewDecoder(bytes.NewReader(raw))
|
||||
dec.UseNumber()
|
||||
var v any
|
||||
if err := dec.Decode(&v); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
if dec.More() {
|
||||
return nil, fmt.Errorf("trailing JSON after first value")
|
||||
}
|
||||
return v, nil
|
||||
}
|
||||
```
|
||||
|
||||
### WR-04: Secret scanners miss passwords and live mismatch text can print tokens
|
||||
|
||||
**File:** `tide/variables.go:562-608`, `../fonoteka.go/parity/check_corpus.go:335-378`, `cmd/summer/parity.go:237-239`
|
||||
**Issue:** `rejectUnclassifiedCredentials` / `scanSecrets` cover JWT, `inv_`, `auth_token=`, `client_secret`, and (in tide only) `code_verifier`. They do not treat JSON `"password":"…"` as a credential. Seed and Nuxt/MCP session request bodies still contain `"password":"parity-alice-pass"`. If isolation fails and a real login is recorded, `--check-secrets` stays green. `remainingCredential` also misses opaque `access_token` values that are not JWT-shaped. Manifest replay prints `cov.Diffs` via `out.Error` with expected/actual slices; a compared field that still holds a live JWT leaks it to the terminal (the gate redacts; the CLI does not).
|
||||
|
||||
**Fix:** Reject remaining `"password"\s*:` / `password=` after placeholder strip in both tide and `scanSecrets`. Capture onboarding passwords as `{{password:alice}}` like JWTs, or keep a documented test-password allow-list that still fails any other password-shaped value. Redact JWT/`inv_`/`client_secret` shapes in `MismatchError.Error` and coverage diff lines.
|
||||
|
||||
### WR-05: `php_parity.sh` isolation is a basename substring
|
||||
|
||||
**File:** `../fonoteka.go/parity/php_parity.sh:10-21`
|
||||
**Issue:** T-02-05 requires refusing a database not named for parity. `refuse_db` only checks `basename` contains `parity` and is not exactly `$PHP_ROOT/storage/database.sqlite`. `$PHP_ROOT/storage/parity.sqlite`, `notparity`, or a remote MySQL database whose name contains `parity` all pass. `export_env` always forces `DB_CONNECTION=sqlite`, so it will not hit production MySQL, but it will still write a sqlite file inside the PHP checkout if the path is crafted. The phase gate (`scripts/check-phase2.sh`) is stricter (unique `fonoteka_parity_<run-id>`, artisan preflight, empty schema). Operators using the helper script are not.
|
||||
|
||||
**Fix:** Require the basename to match `*parity*` **and** the file to live under `/tmp/summercms-parity` (or another disposable dir). Refuse any path under `$PHP_ROOT`. Keep the gate's MariaDB name + preflight as the source of truth for `--fresh-php`.
|
||||
|
||||
### WR-06: Parity Postgres container is leaked on `os.Exit`
|
||||
|
||||
**File:** `../fonoteka.go/parity/synthetic_test.go:31-44`
|
||||
**Issue:** `TestMain` `defer stopParityPostgres()` never runs because `os.Exit` skips defers. Repeated `go test ./parity` leaves testcontainers Postgres instances until the reaper timeout. That is a reliability issue for the phase-ending suite.
|
||||
|
||||
**Fix:**
|
||||
|
||||
```go
|
||||
func TestMain(m *testing.M) {
|
||||
code := 1
|
||||
defer stopParityPostgres()
|
||||
if !testShort() {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
|
||||
parityPGErr = startParityPostgres(ctx)
|
||||
cancel()
|
||||
if parityPGErr != nil {
|
||||
fmt.Fprintf(os.Stderr, "parity: testcontainers postgres: %v\n", parityPGErr)
|
||||
return // deferred stop is a no-op if start failed
|
||||
}
|
||||
}
|
||||
code = m.Run()
|
||||
os.Exit(code) // still skips defer — call stopParityPostgres() explicitly before Exit
|
||||
}
|
||||
```
|
||||
|
||||
Call `stopParityPostgres()` immediately before `os.Exit`, not via defer.
|
||||
|
||||
### WR-07: Fixture/vars path checks do not evaluate symlinks
|
||||
|
||||
**File:** `tide/flow.go:200-211`, `tide/variables.go:611-626`, `tide/manifest.go:184-195`
|
||||
**Issue:** `validateSidecar`, `validateFixturePath`, and `varsOutsideFixtures` use `filepath.Abs` + `Clean` / prefix checks. A `body_file` or `--vars` path that is a symlink into `/etc` or into `fixtures/` is not detected. Same class as Phase 1 WR-08.
|
||||
|
||||
**Fix:** `EvalSymlinks` on the absolute paths (and the fixtures root) before the prefix check; reject if either call fails.
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: CLI `passing` on pending routes is PHP self-check language
|
||||
|
||||
**File:** `tide/manifest.go:446-531`, `../fonoteka.go/parity/parity_test.go:138-177`
|
||||
**Issue:** `ReplayManifest` calls `markPassing` whenever a recorded pending fixture matches the target. That is correct for `--self-check` against PHP. `coverageFromManifest` in the app tests never looks at subtest results; it only counts YAML `status: pending`. `runCorpusRoute` does not send pending routes to the synthetic handler, and `TestParityCorpus` / `TestParityContract` assert `Passing == 0` / `Ported == 0`. Phase 2 Go honesty holds. A later change that replays pending routes against `newTarget` would still look "honest" in the coverage subtest because `cov` is computed before the loop.
|
||||
|
||||
**Fix:** Drive the coverage assertion from `runCorpusRoute` outcomes, or document that CLI `passing` means "matched target" not "ported".
|
||||
|
||||
### IN-02: `Store.Save` does not sync; dead helper
|
||||
|
||||
**File:** `tide/variables.go:81-120`, `tide/variables.go:639-645`
|
||||
**Issue:** `SaveFlow` syncs before rename; the 0600 vars file does not. A crash can leave an empty map. `recordedResponseHeaders` is unused.
|
||||
|
||||
**Fix:** `tmp.Sync()` before close in `Save`; delete or use the helper.
|
||||
|
||||
### IN-03: `localhost` is trusted without resolving
|
||||
|
||||
**File:** `tide/proxy.go:353-376`
|
||||
**Issue:** `isLoopbackHost` returns true for the string `localhost` without a lookup. A poisoned `/etc/hosts` would still pass `NewProxy` then dial off-loopback. Client `Host` is correctly ignored (`SetURL` + `Out.Host`). Residual SSRF is configuration, not request-driven.
|
||||
|
||||
**Fix:** Resolve and require every A/AAAA to be loopback, or allow only literal `127.0.0.1` / `::1` in production CLI defaults (tests already use httptest URLs).
|
||||
|
||||
### IN-04: Smoke HTTP child is not trapped
|
||||
|
||||
**File:** `scripts/check-phase2.sh:89-139`
|
||||
**Issue:** `cleanup_smoke` is not an `EXIT` trap. A failed `go build` / record during the synthetic smoke leaves a Python `serve_forever` on a random port. PHP cleanup later is trapped.
|
||||
|
||||
**Fix:** `trap cleanup_smoke EXIT` for that section (or a single combined cleanup).
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-09-17T12:18:00Z_
|
||||
_Reviewer: Claude (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
Reference in New Issue
Block a user