From 5f931295244c70f8f31ce9c39a073b069f504dec Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 17 Sep 2026 14:45:35 +0200 Subject: [PATCH] docs(02): add code review fix report Co-authored-by: Cursor --- .../02-REVIEW-FIX.md | 79 +++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 .planning/phases/02-api-parity-harness-bootstrap/02-REVIEW-FIX.md diff --git a/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW-FIX.md b/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW-FIX.md new file mode 100644 index 0000000..800a331 --- /dev/null +++ b/.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW-FIX.md @@ -0,0 +1,79 @@ +--- +phase: 02-api-parity-harness-bootstrap +fixed_at: 2026-09-17T12:55:00Z +review_path: .planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md +iteration: 1 +findings_in_scope: 8 +fixed: 8 +skipped: 0 +status: all_fixed +--- + +# Phase 02: Code Review Fix Report + +**Fixed at:** 2026-09-17T12:55:00Z +**Source review:** `.planning/phases/02-api-parity-harness-bootstrap/02-REVIEW.md` +**Iteration:** 1 + +**Summary:** +- Findings in scope: 8 (CR-01, WR-01 … WR-07) +- Fixed: 8 +- Skipped: 0 +- Info (IN-01 … IN-04): not in scope + +## Fixed Issues + +### CR-01: Short captured IDs rewrite pagination and IPv4, hiding PHP/Go diffs + +**Files modified:** `tide/variables.go`, `tide/capture_test.go`; `../fonoteka.go/parity/fixtures/**` (38 YAML files) +**Commit:** `5994e67` (framework), `d6251e0` (fonoteka fixtures) +**Status:** fixed: requires human verification +**Applied fix:** Short numeric capture values (`len < 8`) are no longer substituted into request/response bodies. Path, query, and headers still isolate `/albums/1`. `.` is an identifier byte so IPv4 last octets stay literal. Added `TestScrubShortIDsLeavePaginationAndIPv4Literal`. Restored committed pagination keys (`current_page`, `last_page`, `total`, `album_count`, counts) and `127.0.0.{{id:*}}` to literal `1` / `127.0.0.1`. JSON `id` / `*_id` placeholders were left in place; `normalizeJSON` still masks those at compare time. + +### WR-01: Fixture save is neither exclusive nor all-or-nothing + +**Files modified:** `tide/fixture.go`, `tide/proxy.go`, `tide/proxy_test.go`, `tide/manifest.go`, `cmd/summer/parity.go` +**Commit:** `7e70afe` +**Applied fix:** Sessions stay in memory until `Flush`. `SaveFlowExclusive` claims the destination with `O_EXCL`. Failed sessions always `os.Remove` the session path. `parity:proxy --update` overwrites. Manifest route recording uses exclusive create unless `--update`. + +### WR-02: Redirect `Location` is recorded but never compared + +**Files modified:** `tide/diff.go`, `tide/headers_test.go` +**Commit:** `5cb2def` +**Applied fix:** Added `Location` to `extraCompareHeaders`. `Set-Cookie` stays off the compare list. + +### WR-03: JSON decoder stops after the first value + +**Files modified:** `tide/diff.go`, `tide/diff_test.go` +**Commit:** `d3d202e` +**Applied fix:** `decodeJSON` errors when `Decoder.More()` is true after the first value. + +### WR-04: Secret scanners miss passwords and live mismatch text can print tokens + +**Files modified:** `tide/variables.go`, `tide/flow.go`, `tide/manifest.go`, `tide/capture_test.go`; `../fonoteka.go/parity/check_corpus.go`, `../fonoteka.go/parity/check_corpus_test.go` +**Commit:** `0ac9dc4` (framework), `1336d53` (fonoteka) +**Applied fix:** After placeholder strip, leftover `"password"` / `password=` fail unless the value is the documented allow-list `parity-alice-pass`. Opaque `"access_token"` leftovers fail. `MismatchError` and coverage diff lines run through `redactSecrets` (JWT, `inv_`, `client_secret`, `auth_token`). + +### WR-05: `php_parity.sh` isolation is a basename substring + +**Files modified:** `../fonoteka.go/parity/php_parity.sh` +**Commit:** `d0f1b61` +**Applied fix:** Basename must still contain `parity`, and the resolved path must live under `$PARITY_ROOT` (`/tmp/summercms-parity` by default). Any path inside `$PHP_ROOT` is refused. + +### WR-06: Parity Postgres container is leaked on `os.Exit` + +**Files modified:** `../fonoteka.go/parity/synthetic_test.go` +**Commit:** `fb64174` +**Applied fix:** `stopParityPostgres()` is called explicitly before every `os.Exit` in `TestMain`. `defer` is no longer used for container cleanup. + +### WR-07: Fixture/vars path checks do not evaluate symlinks + +**Files modified:** `tide/variables.go`, `tide/flow.go`, `tide/manifest.go`, `tide/proxy_test.go` +**Commit:** `f7b81b9` +**Applied fix:** `resolvePath` uses `EvalSymlinks` on the path or its parent before prefix checks. Sidecar reads must stay inside the fixture root after symlink resolution. A vars symlink into `fixtures/` is rejected. + +--- + +_Fixed: 2026-09-17T12:55:00Z_ +_Fixer: the agent (gsd-code-fixer)_ +_Iteration: 1_