---
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 `:<milestone>-<sha>` 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 `:<milestone>-<sha>` (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_