diff --git a/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md b/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md new file mode 100644 index 0000000..8fe57f5 --- /dev/null +++ b/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md @@ -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_`, 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_