217 lines
11 KiB
Markdown
217 lines
11 KiB
Markdown
---
|
||
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_
|