diff --git a/docs/release-candidate-review.md b/docs/release-candidate-review.md index 123bf3f..849a7b4 100644 --- a/docs/release-candidate-review.md +++ b/docs/release-candidate-review.md @@ -9,17 +9,25 @@ > `docs/infrastructure/database-ownership.md` (who creates which table), > `docs/release-checklist.md` (state of the build). -**Updated 2026-07-19 (second pass).** **B1 is fixed and verified** — `deploy.sh` now loads the shared -environment before it decides anything, requires `DATABASE_PROVIDER` explicitly, validates the rest of -the deployment configuration before touching the stack, and verifies each backup against its own -format. Details under *Blocking*. +**Updated 2026-07-19 (final pass).** Two findings from the first pass are now fixed and verified — +**B1** (the pre-deploy backup silently backed up the wrong thing) and **N2** (`/health` always reported +`version: unknown`). Both are recorded under *Closed since the first pass*, with their original text +kept because the failure modes are worth understanding. -**Verdict: one blocking item remains — CI (B2), which is external.** The deployment path itself is now -sound. +**Verdict: one blocker remains, and it is external — CI (B2).** Nothing in the application or the +deployment path is now known to be blocking. Everything verified here is *local* verification; CI has +proven none of it. + +| Section | Meaning | +|---|---| +| [READY](#ready--completed-technical-checks) | Verified. Nothing further needed before deploying | +| [BLOCKED](#blocked--requires-external-action) | Requires action outside this repository. Deployment is gated on it | +| [MANUAL](#manual--only-the-owner-can-verify) | Only the owner can confirm — sign-in, real data, production smoke tests | +| [Accepted](#accepted-for-the-first-release-non-blocking) | Known limitations, deliberately shipped as-is | --- -## Ready +## READY — completed technical checks ### Database @@ -72,11 +80,20 @@ sound. | Missing `AUTH_JWT_KEY` fails the backend | With `Auth__Require=true`, `Program.cs:241` throws `InvalidOperationException`. It does not silently generate an ephemeral key | | Backup rejects a useless dump | `verify_backup` fails the deploy on an empty file, and on a MariaDB dump lacking `CREATE TABLE` | | Backups never overwrite | UTC-timestamped filenames; retention is explicitly manual and the script says so | -| Test suite | 393 backend tests pass in Release, run for this review | +| **Deploy script loads its own environment** | `deploy.sh` parses the shared `.env` into its own shell before any decision. Values already in the environment (CI's `APP_VERSION`) still win. No value is echoed | +| **`DATABASE_PROVIDER` is required, not defaulted** | Missing or unrecognised aborts the deploy. Verified: exits 1, names the variable, leaves no backup file | +| **Configuration validated before the stack is touched** | Connection string, `AI_SERVICE_TOKEN` and `AUTH_JWT_KEY` are checked before the backup, and therefore before any build, stop or replace | +| **Backups verified per provider** | A dump needs valid gzip, `CREATE TABLE` and the `Dump completed` trailer; an archive needs `jobtracker.db`. Truncated and trailer-stripped dumps both rejected | +| **SQLite volume resolved by real name** | Project-prefixed, and fails if absent — the bare name would have created an empty volume and backed that up | +| **No secret leakage in deploy output** | Zero occurrences of any password, token or key across every failure-path test | +| **`/health` reports the deployed version** | Reads `App:Version`; verified end to end that `App__Version=9.9.9-test` surfaces as `9.9.9-test`, and that an unset value falls back to the assembly version | +| Test suite | 401 backend tests pass in Release, run for this review | --- -## Blocking +## Closed since the first pass + +Both were found by this review, fixed in their own commits, and re-verified. Original findings kept. ### B1. ~~The pre-deploy database backup does not back up the database~~ — **CLOSED 2026-07-19** @@ -162,26 +179,57 @@ is unset rather than skipping in silence), as does `OLLAMA_MODEL` for the warmup | Dump with the trailer stripped | ✅ rejected as truncated | | Secret leakage across every test's output | ✅ zero occurrences of any password, token or key | +### N2. ~~`/health` always reports `version: unknown` under Docker~~ — **CLOSED 2026-07-19** + +*Original finding:* the endpoint read the `APP_VERSION` **environment variable**, but compose passes +`App__Version`, which binds to the `App:Version` **configuration key**. No variable by that name +existed in the container, so the version was always `unknown`. + +**Closed.** `/health` now reads `App:Version` through `IConfiguration` — the approach +`AdminSystemController` already used for the same value. The resolution rule (configured version, else +assembly version) moved to a shared `BuildMetadata` helper rather than being written twice, so the +admin page and `/health` cannot drift apart. + +*Verified against a running backend:* `App__Version=9.9.9-test` reports `9.9.9-test`; unset reports the +assembly version rather than `unknown`. Tests pin the configuration **key**, including that an +`App__Version` environment variable binds to `App:Version` — the original bug failed silently, so a +behavioural test alone would not have caught it. + +--- + +## BLOCKED — requires external action + +One item. Nothing in this repository can clear it. + ### B2. CI is red — deployment is gated on it Unchanged and still external to the repository. Commit `8f73548` changed one markdown file and its test job failed in the same duration band as every other run; a change that cannot affect compilation cannot fail a test job. The `deploy` job declares `needs: test`, so nothing promotes until this clears. -Everything in this review is therefore **local verification**. Blocked on job logs (a read-scoped -Gitea token), `journalctl -u act_runner`, and the runner container configuration. Evidence in -`docs/infrastructure/runner-investigation.md`. +Everything in this review is therefore **local verification**. Blocked on, and needing you: + +- **Job logs** — the Gitea API returns 401 unauthenticated. A read-scoped token would unblock this +- **`journalctl -u act_runner --since '2 hours ago'`** on the runner host +- **The act_runner container/service configuration** + +Evidence in `docs/infrastructure/runner-investigation.md`. + +**The decision this forces.** Either fix the runner, or deploy deliberately from a locally verified +commit with CI knowingly red. The second is a defensible choice for a self-hosted single-node +deployment — but it should be a decision rather than a default, and it means the 401 backend tests and +128 frontend tests have only ever run on one machine. --- -## Non-blocking +## Accepted for the first release (non-blocking) -Known and accepted for the first release. None of these should stop a deploy; all are worth knowing. +Known limitations, deliberately shipped as-is. None should stop a deploy; all are worth knowing. | # | Finding | Why it is not blocking | |---|---|---| | N1 | ~~**`.env.example` omits `DATABASE_PROVIDER` and `JOBTRACKER_CONNECTION_STRING`**~~ — **CLOSED 2026-07-19** | Both added to the template with the connection-string host caveat. `deploy.sh` now hard-fails without `DATABASE_PROVIDER`, so the template had to name it | -| N2 | **`/health` always reports `"version":"unknown"` under Docker** — the endpoint reads the `APP_VERSION` environment variable, but compose passes it as `App__Version` | Liveness is unaffected; only the version string is wrong. `deploy/first-production-deployment.md` shows a populated version in its expected output, which will not match reality | +| N2 | ~~**`/health` always reports `"version":"unknown"` under Docker**~~ — **CLOSED 2026-07-19** | Fixed; see *Closed since the first pass* above | | N3 | **The post-deploy gate in `deploy.sh` checks `.State == running`, not health** — a container can be `running` while `starting` or `unhealthy` | Harmless in practice: `compose up` already blocks on `service_healthy` for the frontend's dependency, so an unhealthy backend aborts the deploy before this check is reached. The check is weaker than it looks, not wrong | | N4 | **Table-count discrepancy across documents** — `database-ownership.md` records 40 tables on MariaDB; `release-checklist.md` and the runbook say ~42 | Both were measured, at different points in Phase 5. Use "the tables listed in `database-ownership.md` all exist" as the check, not a number | | N5 | **`ai-service` opens completely if `AI_SERVICE_TOKEN` is the empty string** — the middleware is `if AI_SERVICE_TOKEN and …`, so a blank token disables the check rather than failing closed | Compose declares the variable with `:?` on both services, so the stack refuses to start without it. No defence in depth behind that, though | @@ -193,24 +241,32 @@ Known and accepted for the first release. None of these should stop a deploy; al --- -## Manual verification required +## MANUAL — only the owner can verify -Only the owner can do these. Nothing in this review, and nothing in the test suite, covers any of -them — every automated check stops at the authentication boundary. +Nothing in this review, and nothing in the test suite, covers any of these — every automated check +stops at the authentication boundary, and none of it has touched production data. + +### Backup and restore readiness + +The automatic backup is fixed and verified against containers (B1). What remains unproven is the one +thing containers cannot prove: that it works **on your database**. + +- [ ] **Take a MariaDB dump by hand from production and restore it into a scratch database.** Not a + formality. Container verification says the mechanism is sound; it says nothing about your data + volume, your disk space, or your MariaDB version's dump quirks. +- [ ] **Confirm `/opt/job-tracker/backups` exists and has room.** `deploy.sh` creates it, but a + full disk fails the backup and therefore the deploy. +- [ ] **Know which backup is the restore point** before you start. After the deploy, confirm the newest + file is `jobtracker--.sql.gz` — a `jobtracker-sqlite-*.tar.gz` would mean the + environment is wrong, though the provider check should now stop that first. ### Before deploying -- [ ] **Take a MariaDB dump by hand and restore it into a scratch database.** B1 is fixed and the - automatic backup is verified against containers, but the first production deploy is still the - wrong moment to discover that a backup does not restore *on your data*. - [ ] **Confirm `/opt/job-tracker/shared/.env` contains `DATABASE_PROVIDER=mariadb` and `JOBTRACKER_CONNECTION_STRING`.** `deploy.sh` now aborts without them, so a missing value costs an aborted deploy rather than a bad backup — but check first and skip the round trip. - [ ] **Confirm the connection-string host resolves *from inside the backend container*** — `127.0.0.1` there means the container, not the Docker host. -- [ ] **After the deploy, check the backup filename.** It must be - `jobtracker--.sql.gz`. A `jobtracker-sqlite-.tar.gz` means the - environment is wrong — though the provider check should now stop that before it happens. - [ ] **Record the current commit** (`git rev-parse HEAD`) and the current row counts for `JobApplications` and `Companies`. The row counts are the check that matters most afterwards. @@ -228,5 +284,15 @@ them — every automated check stops at the authentication boundary. - [ ] **An AI generation completes end to end** and writes nothing until you save it. - [ ] **Email and calendar integrations still connect**, if you use them — the OAuth callbacks were reviewed statically but never exercised against live Google or Microsoft. +- [ ] **`/health` reports the version you deployed**, not `1.0.0.0`. The assembly fallback means + `APP_VERSION` did not reach the container — harmless in itself, but build metadata is then + missing from the admin page too. - [ ] **The full journey once, deliberately** — empty profile through to a recorded outcome. No one has walked it. Friction in that path is currently unmapped. + +### Why this section cannot shrink + +Authenticated smoke testing is not an automation gap that more work would close. Signing in requires a +password, and no automated step in this repository should ever handle one. The 401 backend tests +verify that the authorization boundary *exists* and holds; only you can verify what is behind it, with +your data.