diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 1d8a0e0..c477bf7 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -303,5 +303,11 @@ jobs: uses: https://github.com/ChristopherHX/gitea-upload-artifact@v4 with: name: playwright-traces-${{ github.run_id }} - path: apps/pwa/test-results/ + # Upload BOTH the raw traces/screenshots/videos (test-results/) AND the + # navigable HTML report (playwright-report/, built by --reporter=list,html). + # Without the report dir the most useful triage artifact for a remote CI + # failure is built on every run and then discarded at runner teardown (WR-02). + path: | + apps/pwa/test-results/ + apps/pwa/playwright-report/ retention-days: 14 diff --git a/.gitea/workflows/publish.yml b/.gitea/workflows/publish.yml index d6a2449..2e12f90 100644 --- a/.gitea/workflows/publish.yml +++ b/.gitea/workflows/publish.yml @@ -41,6 +41,12 @@ jobs: - name: Compute image tags id: tags run: | + set -euo pipefail + # Fail closed if GITHUB_SHA is empty/unset (Gitea runner env parity is not + # guaranteed across versions). Without this guard SHORT_SHA degrades to "" + # and the immutable tag silently becomes :v1.1- — a valid-but-wrong tag that + # overwrites the milestone pointer and destroys rollback traceability (WR-03). + : "${GITHUB_SHA:?GITHUB_SHA is empty — refusing to build a malformed image tag}" SHORT_SHA=${GITHUB_SHA:0:7} MILESTONE="${{ env.MILESTONE }}" echo "latest=git.bergerhouse.net/luckberg/familysync-api:latest" >> $GITHUB_OUTPUT @@ -53,8 +59,15 @@ jobs: # Secret is named REGISTRY_PAT (not GITEA_REGISTRY_PAT): Gitea reserves the GITEA_ prefix # for secret names, so the GITEA_-prefixed name cannot be created. - name: Docker login + # Bind the secret through env: so it is never substituted into the rendered + # script body. Read it as $REGISTRY_PAT and pipe with printf '%s' (echo is not + # safe for arbitrary strings — a trailing newline or shell-significant char + # would mangle the password into a confusing `unauthorized`) (WR-05). + env: + REGISTRY_PAT: ${{ secrets.REGISTRY_PAT }} run: | - echo "${{ secrets.REGISTRY_PAT }}" | \ + set -euo pipefail + printf '%s' "$REGISTRY_PAT" | \ docker login git.bergerhouse.net \ --username luckberg \ --password-stdin @@ -63,13 +76,18 @@ jobs: # lockfile from the root context; building from apps/api/ would fail to find them. - name: Build and push run: | + set -euo pipefail docker build --target production \ -f apps/api/Dockerfile \ -t ${{ steps.tags.outputs.latest }} \ -t ${{ steps.tags.outputs.sha_tag }} \ . - docker push ${{ steps.tags.outputs.latest }} - docker push ${{ steps.tags.outputs.sha_tag }} + # Push the IMMUTABLE :- tag FIRST. set -euo pipefail stops on + # the first failed push, so :latest is only moved after the immutable, + # rollback-traceable tag has landed — a failed second push can never leave + # :latest advanced without a corresponding rollback tag (WR-04). + docker push ${{ steps.tags.outputs.sha_tag }} # immutable first + docker push ${{ steps.tags.outputs.latest }} # move pointer only after immutable lands # Always drop the stored credential from the runner after push (defence in depth). - name: Docker logout diff --git a/.planning/phases/08-gitea-ci/08-REVIEW.md b/.planning/phases/08-gitea-ci/08-REVIEW.md new file mode 100644 index 0000000..2e0c0f6 --- /dev/null +++ b/.planning/phases/08-gitea-ci/08-REVIEW.md @@ -0,0 +1,216 @@ +--- +phase: 08-gitea-ci +reviewed: 2026-06-11T00:00:00Z +depth: deep +files_reviewed: 3 +files_reviewed_list: + - .gitea/workflows/ci.yml + - .gitea/workflows/publish.yml + - apps/api/src/db/migrations/0000_baseline.sql +findings: + critical: 0 + warning: 5 + info: 2 + total: 7 +status: fixes_applied +fix_summary: + fixed: [WR-02, WR-03, WR-04, WR-05, IN-01] + deferred: [IN-02] # ESLint gate is Phase 13's deliverable + fixed_at: 2026-06-11 +--- + +# Phase 8: Code Review Report (Re-Review, Post-Split) + +**Reviewed:** 2026-06-11 +**Depth:** deep +**Files Reviewed:** 3 +**Status:** issues_found + +## Summary + +Re-review of Phase 8 (Gitea CI) against the post-split state: `publish` is now its own +`publish.yml` triggered on `push: branches: [main]`, and `ci.yml` holds the three PR-gated +jobs (`fast-checks`, `api`, `harness`). + +**Resolved since prior review:** +- **WR-01 (publish ran without test gating / orphaned pending status)** — RESOLVED. Publish + is split into `publish.yml`, triggered only on `push` to `main`. PR jobs gate the merge via + required status checks + branch protection (direct/force push blocked). No `needs:` is needed + because publish never shares a workflow invocation with the test jobs. Architecture confirmed + intentional; not re-raised. + +**Still open (re-located to the split files):** +- WR-02 (harness HTML report built then discarded) — still open, now in `ci.yml`. +- WR-03 (unguarded `${GITHUB_SHA:0:7}` → malformed tag) — still open, now in `publish.yml`. +- WR-04 (non-atomic two-push, `:latest` before immutable tag) — still open, now in `publish.yml`. +- IN-01 (>3072-byte UNIQUE indexes, MariaDB-only) — still open in `0000_baseline.sql`. + +**New findings from the deep pass:** +- WR-05 (secret interpolated into `run:` script body via `echo` instead of `env:`) — new. +- IN-02 (`lint` step is a documented no-op that masks lint failures) — new. + +No Critical findings. The publish flow has no test `needs:` by design (gated by branch +protection), so it is not flagged. The dominant theme is robustness/observability gaps in the +publish + harness steps and MariaDB-specific schema portability. + +## Narrative Findings (AI reviewer) + +## Warnings + +### WR-02: Harness HTML report is generated but never uploaded — FIXED (commit 44a9c30) + +> Resolution: extended the existing `ChristopherHX/gitea-upload-artifact@v4` failure step's +> `path:` to a multi-line list uploading both `apps/pwa/test-results/` and +> `apps/pwa/playwright-report/`. Reporter unchanged; step still `if: failure()`. + + +**File:** `.gitea/workflows/ci.yml:293` (report generation) and `:301-307` (upload step) +**Issue:** The harness runs Playwright with `--reporter=list,html`. The `html` reporter writes +its output to `apps/pwa/playwright-report/` (Playwright's default `outputFolder`, not overridden +in `playwright.config.ts`). The failure-artifact upload step (`if: failure()`) only uploads +`path: apps/pwa/test-results/`. The HTML report — the most useful artifact for triaging a remote +CI failure — is built on every run and then discarded when the runner is torn down. `test-results/` +contains traces/screenshots/videos but not the navigable HTML report. +**Fix:** Either drop `html` from the reporter (saves build time if it is genuinely unwanted), or +upload it. Preferred — add the report to the existing upload, or a second upload step: +```yaml + - name: Upload Playwright HTML report + if: failure() + uses: https://github.com/ChristopherHX/gitea-upload-artifact@v4 + with: + name: playwright-report-${{ github.run_id }} + path: apps/pwa/playwright-report/ + retention-days: 14 +``` +(Or set `path: |` with both `apps/pwa/test-results/` and `apps/pwa/playwright-report/` on the +existing step.) + +### WR-03: Unguarded `${GITHUB_SHA:0:7}` can emit a malformed image tag — FIXED (commit 6bcf867) + +> Resolution: added `set -euo pipefail` + a `: "${GITHUB_SHA:?...}"` fail-closed guard +> before `SHORT_SHA=${GITHUB_SHA:0:7}`, so an empty SHA aborts the step instead of +> producing `:v1.1-`. + + +**File:** `.gitea/workflows/publish.yml:44` +**Issue:** `SHORT_SHA=${GITHUB_SHA:0:7}` has no guard for an empty/unset `GITHUB_SHA`. If the +runner does not populate `GITHUB_SHA` (Gitea Actions env parity is not guaranteed across runner +versions; the comment only asserts it via probe P-13, not a runtime check), `SHORT_SHA` becomes +empty and the immutable tag silently degrades to `git.bergerhouse.net/luckberg/familysync-api:v1.1-` +— a valid-but-wrong tag that overwrites the milestone pointer and destroys rollback traceability. +Because the default Actions shell runs with `pipefail`/`-e` but NOT `-u`, the empty expansion does +not error; it proceeds. +**Fix:** Fail closed when the SHA is missing: +```bash + set -euo pipefail + if [ -z "${GITHUB_SHA:-}" ]; then + echo "GITHUB_SHA is empty — cannot compute immutable tag" >&2 + exit 1 + fi + SHORT_SHA=${GITHUB_SHA:0:7} +``` + +### WR-04: Build-and-push is non-atomic and pushes `:latest` before the immutable tag — FIXED (commit 4001cd5) + +> Resolution: reordered the pushes so the immutable `:-` tag goes first, +> `:latest` second; added `set -euo pipefail` so the step stops on the first failed push. + + +**File:** `.gitea/workflows/publish.yml:64-72` +**Issue:** The step runs `docker build` then two sequential `docker push` calls. `:latest` is +pushed first (line 71), then `:-` (line 72). If the second push fails (registry +hiccup, auth expiry, network), `:latest` already moved to the new image while the immutable, +rollback-traceable tag was never published — the exact tag operators would reach for to roll back +does not exist, but `:latest` already advanced. Although the Actions default shell injects `-e` +(so a failed first command aborts the step), ordering still means a partial-failure window leaves +`:latest` ahead of the immutable record. Push order should be immutable-first. +**Fix:** Push the immutable tag first, then `:latest`, and make the shell strict explicitly: +```bash + set -euo pipefail + docker build --target production \ + -f apps/api/Dockerfile \ + -t ${{ steps.tags.outputs.latest }} \ + -t ${{ steps.tags.outputs.sha_tag }} \ + . + docker push ${{ steps.tags.outputs.sha_tag }} # immutable first + docker push ${{ steps.tags.outputs.latest }} # move pointer only after immutable lands +``` + +### WR-05: Registry PAT is interpolated into the `run:` script body instead of passed via `env:` — FIXED (commit 58861d9) + +> Resolution: bound `REGISTRY_PAT` through step-level `env:` and replaced the +> `echo "${{ secrets.REGISTRY_PAT }}" | ...` pipe with `printf '%s' "$REGISTRY_PAT" | ...`. +> Existing PAT-via-stdin and REGISTRY_PAT-naming comments preserved. + + +**File:** `.gitea/workflows/publish.yml:55-60` +**Issue:** `echo "${{ secrets.REGISTRY_PAT }}" | docker login ... --password-stdin` interpolates +the secret into the shell script text at template-expansion time. Two problems: +(1) Robustness — if the PAT ever contains a shell-significant character or a trailing newline, +`echo` may mangle or split it (`echo` is not safe for arbitrary strings; `printf %s` is). A +mangled-but-nonempty password produces a confusing `unauthorized` rather than a clear failure. +(2) Surface — template-substituting a secret into the script body is the documented anti-pattern +versus binding it through `env:` (the script then references `$REGISTRY_PAT`), which keeps the +secret out of the rendered command line / step definition and is the recommended pattern for +Actions-compatible runners. Gitea's log scrubber masks it either way, so this is a robustness/ +hardening WARNING, not a leak. +**Fix:** +```yaml + - name: Docker login + env: + REGISTRY_PAT: ${{ secrets.REGISTRY_PAT }} + run: | + set -euo pipefail + printf '%s' "$REGISTRY_PAT" | docker login git.bergerhouse.net \ + --username luckberg --password-stdin +``` + +## Info + +### IN-01: Two UNIQUE constraints exceed the 3072-byte index limit (MariaDB-only) — FIXED, docs-only (commit bf09110) + +> Resolution: added explanatory SQL comments above `uniq_calendar_user_url` and +> `uniq_push_endpoint` in the already-applied `0000_baseline.sql`, documenting the +> over-length-index dependency on MariaDB 11's long-unique HASH behavior. NO schema, +> column, or index altered (migration is live on main/production). + + +**File:** `apps/api/src/db/migrations/0000_baseline.sql:100` (`uniq_push_endpoint`) and `:48` +(`uniq_calendar_user_url`) +**Issue:** With the default `utf8mb4` charset (4 bytes/char): +- `uniq_push_endpoint UNIQUE(endpoint)` where `endpoint varchar(2048)` → 2048 × 4 = 8192 bytes. +- `uniq_calendar_user_url UNIQUE(user_id, url)` where `url varchar(1024)` → 4 + 1024 × 4 = 4100 bytes. +Both exceed InnoDB's 3072-byte index-key limit. They succeed on MariaDB 11.x (the CI service image +`mariadb:11`) because MariaDB silently builds over-length UNIQUE constraints as long-unique HASH +indexes. The same DDL fails hard on MySQL 8 and on MariaDB configured with +`innodb_large_prefix` semantics disabled or a stricter SQL mode. This is engine-pinned, not a bug +on the current target — the project hard-constrains to MariaDB (no PostgreSQL/MySQL) — so it is INFO. +**Fix:** No change required while MariaDB is the only target. If portability is ever wanted, either +(a) shorten the columns (e.g. `url varchar(768)`, `endpoint` hashed to a `char(64)` digest column +with the UNIQUE on the digest), or (b) add an explicit comment in `schema.ts` documenting the +MariaDB long-unique-HASH dependency so a future MySQL migration is not silently broken. Schema.ts +already carries a CR-02 note on the `endpoint(2048)` width; extend it to record the index-limit +caveat. + +### IN-02: `Lint` step is a no-op that will mask real lint failures once ESLint is wired — DEFERRED to Phase 13 + +> Not fixed this phase. Wiring a real ESLint gate (`pnpm -r --if-present lint`) is Phase 13's +> deliverable. `ci.yml`'s lint step left untouched intentionally. + + +**File:** `.gitea/workflows/ci.yml:31-32` (`pnpm lint`) +**Issue:** Per the in-file comment, no package defines a `lint` script, so `pnpm lint` (root) prints +`ERR_PNPM_RECURSIVE_RUN_NO_SCRIPT` but exits 0 — the step is green regardless. This is acceptable +for the current phase (lint wiring is explicitly out of scope), but it is a latent trap: when a +`lint` script is later added to one package, `pnpm lint` at the root still will not run it unless +the invocation is `pnpm -r lint`, and even then `--if-present` semantics differ. The step gives a +false sense that linting is enforced. +**Fix:** When lint is wired, switch to `pnpm -r --if-present lint` (runs lint only in packages that +define it, fails the job on real lint errors) and remove the no-op comment. No action this phase; +tracked so the green-but-empty step is not mistaken for working lint enforcement. + +--- + +_Reviewed: 2026-06-11_ +_Reviewer: Claude (gsd-code-reviewer)_ +_Depth: deep_ diff --git a/apps/api/src/db/migrations/0000_baseline.sql b/apps/api/src/db/migrations/0000_baseline.sql index d239477..c79b19b 100644 --- a/apps/api/src/db/migrations/0000_baseline.sql +++ b/apps/api/src/db/migrations/0000_baseline.sql @@ -45,6 +45,11 @@ CREATE TABLE `calendars` ( `last_synced_at` timestamp, `is_shared` boolean NOT NULL DEFAULT false, CONSTRAINT `calendars_id` PRIMARY KEY(`id`), + -- NOTE (IN-01): UNIQUE(user_id, url) with url varchar(1024)/utf8mb4 is ~4100 bytes, + -- over InnoDB's 3072-byte index-key limit. Succeeds only because MariaDB 11 silently + -- builds over-length UNIQUE constraints as long-unique HASH indexes; this same DDL + -- fails on MySQL 8 or with stricter SQL modes. Engine-pinned to MariaDB (project hard + -- constraint). Already applied on main/production — do not alter. CONSTRAINT `uniq_calendar_user_url` UNIQUE(`user_id`,`url`) ); --> statement-breakpoint @@ -97,6 +102,11 @@ CREATE TABLE `push_subscriptions` ( `created_at` timestamp NOT NULL DEFAULT (now()), `updated_at` timestamp DEFAULT (now()) ON UPDATE CURRENT_TIMESTAMP, CONSTRAINT `push_subscriptions_id` PRIMARY KEY(`id`), + -- NOTE (IN-01): UNIQUE(endpoint) with endpoint varchar(2048)/utf8mb4 is ~8192 bytes, + -- over InnoDB's 3072-byte index-key limit. Succeeds only because MariaDB 11 silently + -- builds over-length UNIQUE constraints as long-unique HASH indexes; this same DDL + -- fails on MySQL 8 or with stricter SQL modes. Engine-pinned to MariaDB (project hard + -- constraint). Already applied on main/production — do not alter. CONSTRAINT `uniq_push_endpoint` UNIQUE(`endpoint`) ); --> statement-breakpoint