docs(ops): add release candidate review
Final validation pass before the first production deployment. Findings only; no application code changed. Verified against the implementation rather than the other documents: migration and reconciler ownership, startup order, authorization coverage, tenant isolation, AI service protection, file access, the five architecture rules, container dependency ordering and failure behaviour. Two blocking items, one new: deploy.sh symlinks the shared .env for docker compose but never sources it, so DATABASE_PROVIDER is unset in the script's own shell and backup_database takes the SQLite branch on a MariaDB host. It tars the data volume, verify_backup only checks the file is non-empty on that path, and the deploy reports a verified backup that contains no database dump. Same root cause silently disables the APP_PUBLIC_BASE_URL smoke check and the Ollama warmup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
cesnimda
@@ -0,0 +1,181 @@
|
||||
# Release candidate review
|
||||
|
||||
> 2026-07-19. Final validation pass before the first production deployment. **No application code was
|
||||
> changed by this review** — findings only. Every claim below was checked against the implementation,
|
||||
> not against the other documents. Where a document and the code disagreed, the code is reported.
|
||||
>
|
||||
> Companions: `deploy/first-production-deployment.md` (how to deploy),
|
||||
> `docs/production-readiness-review.md` (what was audited),
|
||||
> `docs/infrastructure/database-ownership.md` (who creates which table),
|
||||
> `docs/release-checklist.md` (state of the build).
|
||||
|
||||
**Verdict: do not deploy yet.** Two blocking items, one of them new and material — the pre-deploy
|
||||
database backup does not do what every other document assumes it does.
|
||||
|
||||
---
|
||||
|
||||
## Ready
|
||||
|
||||
### Database
|
||||
|
||||
| Item | Verified against |
|
||||
|---|---|
|
||||
| Startup order is connect → reconcile → `Migrate()` → reconcile → seed | `StartupInitializationExtensions.InitializeJobTrackerAsync`; matches `database-ownership.md` exactly |
|
||||
| Migration ownership | `Companies`, `JobApplications`, `Jobs`, `Correspondences`, `Attachments`, `JobEvents`, `RuleSettings`, Identity — created only by migrations; reconciler repairs but never `CREATE TABLE`s them |
|
||||
| Reconciler ownership | Every Phase 4/5 table is reconciler-owned with a paired no-op migration holding the snapshot. The seven no-op migrations named in `database-ownership.md` all exist on disk |
|
||||
| Dependency guards | Reconciler tables with an FK are guarded on the parent existing, so pass 1 skips and pass 2 creates. `EnsureMySqlIndex` guards on table existence, not just index existence |
|
||||
| Reconciler is non-destructive | `DropMalformedMySqlTable` checks row count first and skips any table holding rows |
|
||||
| Provider selection accepts both spellings | `Program.cs:68` — `provider is "mysql" or "mariadb"`. `deploy/README.md` documents `DATABASE_PROVIDER=mariadb`; that value works |
|
||||
| Rollback reasoning is sound | Phase 4/5 migrations are no-ops, so reverting code never leaves migration history ahead of the schema. Older code ignores the extra tables |
|
||||
|
||||
### Security
|
||||
|
||||
| Item | Verified against |
|
||||
|---|---|
|
||||
| Explicit authorization | Every user-owned controller carries a class-level `[Authorize(AuthenticationSchemes = "local")]`. Confirmed directly on `JobApplicationsController`, `CompaniesController`, `CorrespondenceController`, `RulesController`, `AttachmentsController`, `BackupController`, `ExportController` |
|
||||
| Independent of `Auth:Require` | The `FallbackPolicy` at `Program.cs:380` is *additional*, not the only defence. `/api/jobapplications` returns 401 with `Auth:Require` unset |
|
||||
| Anonymous surface is a deliberate allow-list | `PublicCvController` (class-level `[AllowAnonymous]`), `ClientErrorsController`, `/health`, and per-method anonymity on `AuthController` / `TwoFactorController` / the two OAuth callbacks |
|
||||
| OAuth callbacks are not an open door | `GmailController.Callback` and `MicrosoftGraphController.Callback` are anonymous by necessity but gated on `ConsumeState(state)`; an unknown or replayed state is rejected before any token exchange |
|
||||
| Tenant isolation | 25 global query filters, all deny-on-null (`CurrentUserId != null && OwnerUserId == CurrentUserId`), covering every owner-scoped root entity |
|
||||
| Attachments are tenant-scoped and path-safe | Resolved through the parent `JobApplication` query (so the job-level filter applies); stored names go through `Path.GetFileName` + `BuildStoredFileName`, so a crafted upload name cannot escape the attachments root |
|
||||
| AI service is not reachable from outside | `ai-service` is on `ai_internal` only — not on `default`, not on the external `shared_services`, no published port. Backend is the only member that can route to it |
|
||||
| AI service authenticates its caller | `X-Ai-Service-Token` middleware with `hmac.compare_digest`; only `/health` is open. Compose declares the token with `:?` so a deploy that forgets it fails loudly |
|
||||
| Public CV is opt-in | Off by default, per-variant, unguessable slug, `noindex` |
|
||||
| Secrets not committed | `.env` is gitignored (`.gitignore:8`); nothing sensitive tracked |
|
||||
| Backup handles the password safely | `MYSQL_PWD`, never on the command line, so it cannot reach the process list or the deploy log. Dump stderr is scrubbed before it is echoed |
|
||||
|
||||
### Application integrity — architecture rules
|
||||
|
||||
| Rule | Verified |
|
||||
|---|---|
|
||||
| **CareerProfile is the only editable career source** | The two Phase 5 services that touch `CareerProfiles` — `ApplicationChecklistService:343` and `ApplicationIntelligenceService:199` — both read `AsNoTracking()`. Nothing downstream writes to it |
|
||||
| **CvVariant is a derived lens** | `ApplicationAssetsService.AttachVariantAsync` writes only `JobApplicationId` and `UpdatedAtUtc`. Attaching a CV to an application never touches variant content |
|
||||
| **Application Workspace aggregates only** | `ApplicationIntelligenceService` and `ApplicationTimelineService` contain **zero** `SaveChanges` calls. They are pure projections |
|
||||
| **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
|
||||
|
||||
| Item | Verified |
|
||||
|---|---|
|
||||
| CI gates deployment | `deploy` job declares `needs: test` and `if: push && ref == refs/heads/main` |
|
||||
| Deploy is pinned to the tested commit | The remote script `git reset --hard ${{ github.sha }}` and aborts if that commit is not fetchable |
|
||||
| Container dependency ordering | `frontend` declares `depends_on: backend: condition: service_healthy`. A backend that never reports healthy makes `compose up` fail, and `set -e` aborts the deploy — a broken backend cannot present as a running stack |
|
||||
| Health checks exist on both services | `curl` against `/health` for backend (90s start period, covering first-boot reconciliation), `wget` against nginx for frontend |
|
||||
| `/health` does not touch the database | Deliberate: a DB-querying probe would restart a healthy backend on any database blip, and would hand out an unauthenticated way to probe database availability |
|
||||
| Missing `AI_SERVICE_TOKEN` fails the stack | `${AI_SERVICE_TOKEN:?…}` in compose, on both `backend` and `ai-service` |
|
||||
| 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 |
|
||||
|
||||
---
|
||||
|
||||
## Blocking
|
||||
|
||||
### B1. The pre-deploy database backup does not back up the database — it silently backs up the wrong thing
|
||||
|
||||
**Severity: critical. This invalidates the restore point that the entire deployment plan depends on.**
|
||||
|
||||
`deploy/deploy.sh` symlinks `/opt/job-tracker/shared/.env` into the checkout so that **docker compose**
|
||||
can read it. It never *sources* it. There is no `set -a`, no `. .env`, no `export` of the database
|
||||
variables anywhere in the script.
|
||||
|
||||
So the script's own shell evaluates:
|
||||
|
||||
```sh
|
||||
local provider="${DATABASE_PROVIDER:-sqlite}"
|
||||
```
|
||||
|
||||
`DATABASE_PROVIDER` is not set in that shell. The CI deploy step exports only `APP_VERSION`,
|
||||
`APP_COMMIT_SHA` and `APP_BUILD_STAMP`, and a non-interactive `ssh` session does not read a profile.
|
||||
**`provider` resolves to `sqlite` on a MariaDB production host.**
|
||||
|
||||
The consequence is not a loud failure, which is what makes this serious:
|
||||
|
||||
1. The SQLite branch runs and tars the `jobtracker_data` volume.
|
||||
2. That volume exists in production (the backend mounts it for `/data`, exports and attachments), so
|
||||
the tar succeeds and produces a non-empty file.
|
||||
3. `verify_backup` is called **without** the `CREATE TABLE` expectation on this path — it only checks
|
||||
the file is non-empty.
|
||||
4. The script prints `Backup verified: …` and the deploy proceeds.
|
||||
|
||||
The operator sees a green backup line and a new file in `/opt/job-tracker/backups`. There is no
|
||||
MariaDB dump. If the deploy then damages the schema, there is nothing to restore.
|
||||
|
||||
**Same root cause, two further silent effects:**
|
||||
|
||||
- `APP_PUBLIC_BASE_URL` is likewise unset, so the public smoke check at the end of `deploy.sh` never
|
||||
runs. `deploy/first-production-deployment.md` states it does.
|
||||
- `OLLAMA_MODEL` is unset, so the post-deploy Ollama warmup never runs.
|
||||
|
||||
**How to confirm before trusting any fix:** run the deploy and check that the newest file in
|
||||
`/opt/job-tracker/backups` is named `jobtracker-<dbname>-<stamp>.sql.gz`, not
|
||||
`jobtracker-sqlite-<stamp>.tar.gz`. The filename alone distinguishes the two paths.
|
||||
|
||||
**Interim manual workaround if you deploy before this is fixed:** take the MariaDB dump by hand,
|
||||
verify it restores into a scratch database, and treat `deploy.sh`'s backup line as meaningless.
|
||||
|
||||
### 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`.
|
||||
|
||||
---
|
||||
|
||||
## Non-blocking
|
||||
|
||||
Known and accepted for the first release. None of these should stop a deploy; all are worth knowing.
|
||||
|
||||
| # | Finding | Why it is not blocking |
|
||||
|---|---|---|
|
||||
| N1 | **`.env.example` omits `DATABASE_PROVIDER` and `JOBTRACKER_CONNECTION_STRING`** — both are consumed by `docker-compose.yml`, and the second is the only way to reach a database at all | The production `.env` already has them, and `deploy/README.md:48-49` documents both. Only bites someone building a new environment from the template |
|
||||
| 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 |
|
||||
| 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 |
|
||||
| N6 | **Logging is console-only** | Captured by `docker logs`; adequate for a single-node self-hosted deployment. No retention, structure or aggregation |
|
||||
| N7 | **No global exception handler** | Unhandled errors return a bare 500 with no correlation id. No stack traces leak outside Development, so this is a supportability cost, not a security one |
|
||||
| N8 | **`ClientErrorsController` is anonymous** | By design — browser error reports must work on pages reached before sign-in. Size-limited to 32 KB and field-truncated; stores nothing |
|
||||
| N9 | **A code rollback does not recover rows written to the new tables** | Correct and documented. A *code* rollback loses nothing; only a *database restore* discards checklist items, cover letter versions, interview prep and AI history created since the dump |
|
||||
| N10 | **Prompt quality is unmeasured** | Interview generation now receives analysis and match context. Whether the output is *better* is a judgement no test makes |
|
||||
|
||||
---
|
||||
|
||||
## Manual verification required
|
||||
|
||||
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.
|
||||
|
||||
### Before deploying
|
||||
|
||||
- [ ] **Take a MariaDB dump by hand and restore it into a scratch database.** Given B1, this is not
|
||||
optional and not a formality. `deploy.sh`'s backup line cannot currently be trusted on a MariaDB
|
||||
host.
|
||||
- [ ] **Confirm `/opt/job-tracker/shared/.env` contains `DATABASE_PROVIDER` and
|
||||
`JOBTRACKER_CONNECTION_STRING`**, and that the `Server=` host resolves *from inside the backend
|
||||
container* — `127.0.0.1` there means the container, not the host.
|
||||
- [ ] **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
|
||||
|
||||
- [ ] **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/<slug>` 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.
|
||||
- [ ] **The full journey once, deliberately** — empty profile through to a recorded outcome. No one has
|
||||
walked it. Friction in that path is currently unmapped.
|
||||
Reference in New Issue
Block a user