diff --git a/docs/release-candidate-review.md b/docs/release-candidate-review.md index 849a7b4..d6ba23e 100644 --- a/docs/release-candidate-review.md +++ b/docs/release-candidate-review.md @@ -9,10 +9,17 @@ > `docs/infrastructure/database-ownership.md` (who creates which table), > `docs/release-checklist.md` (state of the build). -**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. +**Updated 2026-07-19 (final pass).** Three findings are now fixed and verified: + +- **B1** — the pre-deploy backup silently backed up the wrong thing (`66b02bc`) +- **N2** — `/health` always reported `version: unknown` (`8f6f2ba`) +- **CV languages** — human languages were dropped depending on the host's ICU data (`9681618`) + +The first two are recorded under *Closed since the first pass* with their original text, because the +failure modes are worth understanding. Also since the previous pass: the deployment sequence was +re-verified by line number rather than by prose, and a full backup → restore → start-the-app rehearsal +was completed and recorded in +[`docs/operations/production-backup-verification.md`](operations/production-backup-verification.md). **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 @@ -22,7 +29,7 @@ proven none of it. |---|---| | [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 | +| [MANUAL VERIFICATION](#manual-verification--only-the-owner-can-do-these) | 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 | --- @@ -67,6 +74,47 @@ proven none of it. | **JobEvent is the timeline source of truth** | `ApplicationTimelineService` interprets `JobEvent` rows and stores nothing. Emission is centralised in `JobLifecycleEvents` | | **AI is suggestion-only** | Generation appends to `AiInteraction`; nothing is written to a profile, variant, cover letter or prep item without an explicit user save | +### Deployment sequence — verified against the code, not the prose + +Checked by line number in `deploy/deploy.sh` and `StartupInitializationExtensions.cs` on 2026-07-19. + +**Before anything is replaced** — the order is what matters, and it holds: + +| Order | Step | Where | +|---|---|---| +| 1 | Load `/opt/job-tracker/shared/.env` into the deploy shell | `deploy.sh` (before any decision) | +| 2 | **Validate configuration** — provider, connection string, `AI_SERVICE_TOKEN`, `AUTH_JWT_KEY` | `deploy.sh:343` | +| 3 | **Take and verify the database backup**; abort the deploy if it fails | `deploy.sh:347` | +| 4 | Build images | `deploy.sh:375` | +| 5 | Replace containers (`up -d --force-recreate`) | `deploy.sh:382` | + +Nothing is built, stopped or replaced before validation and backup. Confirmed: 343 and 347 both +precede 375 and 382. + +**During deployment** — backend startup, `InitializeJobTrackerAsync`: + +| Order | Step | Where | +|---|---|---| +| 1 | `ReconcileSchema()` — pass 1, repair and reconciler-owned tables | `StartupInitializationExtensions.cs:1965` | +| 2 | `Database.Migrate()` — migration-owned tables | `:1973` | +| 3 | `ReconcileSchema()` — pass 2, everything pass 1 had to skip | `:1984` | + +**Migrations expected to run: none that create anything.** All seven Phase 4/5 migrations were +confirmed to have a **literally empty `Up` body** (0 statements each): `AddCareerProfileRelationalChildren`, +`AddCvVariants`, `AddAiInteractions`, `AddApplicationChecklistItems`, `SyncCareerChildKeyLengths`, +`AddCoverLetterVersions`, `AddInterviewPrepItems`. The reconciler owns their DDL with correct +per-provider types. This is what makes a code rollback safe — migration history never runs ahead of +the schema. + +**Health checks and failure behaviour:** + +| Item | Verified | +|---|---| +| Backend health check | `curl` against `/health`, 90s start period for first-boot reconciliation | +| Frontend health check | `wget` against nginx | +| Dependency gate | `frontend` declares `depends_on: backend: condition: service_healthy` — a backend that never reports healthy makes `compose up` fail and `set -e` aborts the deploy | +| Rollback procedure documented | `deploy/first-production-deployment.md` and `deploy/README.md`, both distinguishing code rollback from database restore | + ### Deployment | Item | Verified | @@ -87,7 +135,7 @@ proven none of it. | **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 | +| Test suite | 420 backend tests pass in Release, and also pass under `DOTNET_SYSTEM_GLOBALIZATION_INVARIANT=1` and on Linux — see the ICU finding below | --- @@ -197,6 +245,37 @@ behavioural test alone would not have caught it. --- +### CV languages depended on the host's ICU data — **CLOSED 2026-07-19** + +Found while investigating a reported CI failure. `HumanLanguageCatalog` built its lookup table solely +from `CultureInfo.GetCultures`, so which languages counted as human languages depended on the +machine's culture data rather than on the CV. Measured: **806 cultures** on a normal Windows or Linux +box, **exactly 1** under globalization-invariant mode. + +Consequences, all silent — no error, no log line: + +| Environment | Behaviour | +|---|---| +| Full ICU | Correct | +| Trimmed ICU data | Names present in the reduced set survive, the rest are dropped — a CV keeps English and loses Norwegian | +| Invariant mode | **Every** language dropped; a CV import loses its Languages section entirely | + +**The tests were right and were not modified.** The catalog is now seeded explicitly with the languages +a CV realistically lists, before the culture enumeration, which still runs and still adds breadth. +Nothing in the seed collides with a technical skill — `Go`, `Java`, `Swift`, `Rust` and `Basic` are +deliberately absent, and `Basic` is also a proficiency level. + +*Verified:* 420 tests pass in four environments — Windows and Linux, each with full ICU and with +`DOTNET_SYSTEM_GLOBALIZATION_INVARIANT=1`. Before the fix the invariant runs failed 5 tests. A new +`HumanLanguageCatalogTests` pins the seeded catalog, confirmed non-vacuous by removing the seed and +watching 15 tests fail. + +**Why this matters beyond the bug:** the defect was invisible to a normal local test run. It is the +clearest evidence in this review that *passing locally* and *correct in the deployed container* are +different claims — which is exactly what B2 below leaves unresolved. + +--- + ## BLOCKED — requires external action One item. Nothing in this repository can clear it. @@ -217,7 +296,7 @@ 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 +deployment — but it should be a decision rather than a default, and it means the 420 backend tests and 128 frontend tests have only ever run on one machine. --- @@ -241,15 +320,37 @@ Known limitations, deliberately shipped as-is. None should stop a deploy; all ar --- -## MANUAL — only the owner can verify +## MANUAL VERIFICATION — only the owner can do these 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. +### Post-deployment checklist — walk this in order + +Each item names what "wrong" looks like, because "it loaded" is not a check. + +| # | Area | Check | Wrong looks like | +|---|---|---|---| +| 1 | **Login** | Sign in with a real existing account | Password rejected, or the session drops on refresh — the latter means `AUTH_JWT_KEY` changed | +| 2 | **Existing applications** | The list loads and the **count matches the pre-deploy number** | Any drop. This is the single most important check on the page | +| 3 | **Application workspace** | Open one application: Overview, Checklist, Timeline, Analysis, Match | A section erroring. Empty is **correct** for applications that predate the feature | +| 4 | **Career profile** | Opens with your real experience, education, skills — and **languages** | Languages missing or reduced to English only. That is the bug fixed in `9681618`; if it reappears the container's ICU data differs from what was tested | +| 5 | **CV builder** | Lists existing variants; open one; it renders with its theme | A variant that opens blank, or loses its theme | +| 6 | **Public CV** | Open `/cv/` for an already-public variant, then **refresh it** | A 404 on refresh — that is SPA deep-link routing, not the CV | +| 7 | **AI features** | Run one generation (interview prep or cover letter) | A 5xx, or a hang. If `ai-service` is down the deploy still succeeds — it is not a deploy gate | +| 8 | **Attachments** | Download an existing attachment from an old application | A 404. Attachments live in the `jobtracker_data` volume; a missing file means the volume did not survive | + +Then confirm `/health` reports the version you deployed rather than `1.0.0.0`. + ### 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**. +A full backup → verify → restore → start-the-app rehearsal was completed on 2026-07-19 and is recorded +in [`docs/operations/production-backup-verification.md`](operations/production-backup-verification.md): +42 tables dumped and restored into a clean MariaDB 11 container, every table's row count identical, +content and foreign keys intact, and the application started healthy against the restored database. + +**That rehearsal used seeded data, not production data.** No production host was contacted. What +remains unproven is the one thing a rehearsal 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 @@ -259,6 +360,9 @@ thing containers cannot prove: that it works **on your database**. - [ ] **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. +- [ ] **Check non-ASCII text survived the round trip.** Open a restored CV or career profile containing + `æ`, `ø` or `å` and confirm it is not mangled. The rehearsal data was ASCII-heavy, so this is the + most likely silent failure and the least likely to be noticed. ### Before deploying @@ -270,29 +374,16 @@ thing containers cannot prove: that it works **on your database**. - [ ] **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. -### After deploying +### After deploying — beyond the eight-point checklist above -- [ ] **Login with a real existing account.** Requires a password; no automated step here handles one. -- [ ] **Existing user data intact** — application and company counts match the pre-deploy numbers - exactly, and applications open with their real content. -- [ ] **CV builder loads an existing variant**, including via an application deep link - (`/career/builder/:id`). -- [ ] **Application workspace renders** — Overview, Checklist, Timeline, Analysis, Match. New sections - being empty on existing applications is correct, not a fault. -- [ ] **Public CV resolves** at `/cv/` for a variant already marked public, including on refresh - and from a shared link. -- [ ] **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 +password, and no automated step in this repository should ever handle one. The 420 backend tests verify that the authorization boundary *exists* and holds; only you can verify what is behind it, with your data.