fix(helm): decouple liveness from database availability

[joryirving]

Closes #1171.

Change

• Use TCP port 3000 for process-level liveness instead of the database-aware HTTP endpoint.
• Keep readiness on public /api/health, with its existing PostgreSQL-aware 503 response. No application auth, database, or deployment-administration changes.
• Add CI assertions against the rendered main Deployment so future chart changes cannot silently recouple liveness or break readiness.

Verification

• Helm dependency build (isolated local repository cache), helm lint, and helm template pass.
• scripts/check-helm-probes.py verifies rendered liveness/readiness and exercises a live loopback HTTP503 server: readiness fails while TCP liveness succeeds; stopped server fails TCP liveness. This is a dependency-failure simulation, not a live Kubernetes/PostgreSQL outage.
• Existing health route tests: 3 passed, including rejected database query returning503.
• git diff --check passes.

Open for human review; no approval or merge.

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M3 (anthropic) — primary route · pr-reviewer-action v3.4.1

Partial coverage and requirement traceability gap: required-check coverage is incomplete, and one or more in-scope requirements could not be verified against enforcement or test evidence. Approval is withheld until both gaps are addressed.

Findings (2 blocker, 1 major, 1 minor)

Severity	Location	Finding
🛑 Blocker	scripts/check-helm-probes.py:17	Script opens sys.argv[1] directly without validating that the path resolves inside an allowed directory, rejecting '..', NUL bytes, or symlinks; future callers could be coerced into reading arbitrary files.
🛑 Blocker	scripts/check-helm-probes.py:17	No regression tests for the new script (no co-located .test.py / tests/ entry); edge cases named in must_check ('test with edge-case paths (null bytes, symlinks)') are not covered.
⚠️ Major	.github/workflows/helm-lint.yaml:31	Rendered YAML is written to /tmp/dispatch-rendered.yaml in one step and consumed in the next; if the redirect fails or /tmp is not preserved, the new check step will pass against a missing/empty file with no useful error.
Minor	scripts/check-helm-probes.py:14	fail() raises SystemExit directly; prefer sys.exit(1) for readability, and add a non-zero exit assertion in any future test harness.

Recommendation

Request changes. The helm probe change itself correctly implements PR 1171's acceptance criteria (TCP liveness on port 3000, HTTP readiness on /api/health), and the new scripts/check-helm-probes.py does model and verify the requested dependency-failure behavior. However, two concrete problems block merge:

1. Path traversal in scripts/check-helm-probes.py (blocker). The script opens sys.argv[1] directly with open(..., encoding="utf-8"). The new CI step invokes it as python3 scripts/check-helm-probes.py /tmp/dispatch-rendered.yaml with a hard-coded absolute path, so the corpus does not show a remote attacker reaching this code, but the classifier flagged path_handling_changes and the script's design accepts an arbitrary user-supplied filesystem path. Nothing in the script or the workflow step rejects a path that escapes the intended file (no canonicalization against a known safe directory, no symlink check, no rejection of .. or NUL bytes). A future caller (local developer, CI matrix parameter, or hook) passing a path-traversal payload would have it opened unconditionally. Per AGENTS.md, inputs must be validated; the must-check "review for path traversal vulnerabilities" is unresolved.

2. CI workflow step ordering bug (major). The new "Check rendered probes" step depends on /tmp/dispatch-rendered.yaml, which is produced by the previous "Template chart (default values)" step that previously inlined the redirect (> /tmp/dispatch-rendered.yaml). The diff preserves the redirect on the default-values step, but the script also opens whatever path it is handed. Because the script does not check that the file actually exists or fail with a useful error if rendering silently produced nothing, and because GitHub Actions runners do not always preserve /tmp across steps on every runner image, the assertion can be silently skipped in some environments (the script only fails on SystemExit when YAML load returns no Deployment). Recommend: capture the rendered YAML via a pipeline into a step output / workspace artifact, pass that path explicitly, and have the script assert os.path.isfile(path) before parsing.

3. Pre-flight assert on isfile and realpath (blocker for must-check "test with edge-case paths"). The script does not test (or guard against) symlink-following or NUL-byte paths. With the current code, open(path) will follow symlinks and is undefined on NUL bytes. Add at minimum an os.path.realpath + startswith-style guard, and a unit test for .., symlink, and NUL inputs.

Change-by-change findings

charts/dispatch/values.yaml

• Liveness switched from httpGet: /api/health to tcpSocket: port: 3000. This is exactly what PR 1171 asks for (process-level, database-independent liveness) and does not reintroduce the basic-auth 401 bug from PR 800/PR 810 because no path is consulted. Good.
• Readiness retained on httpGet: /api/health with its existing 503-on-DB-failure contract preserved (see src/app/api/health/route.ts referenced by the issue and src/app/api/health/route.test.ts:53). Good.
• The comment block now documents the rationale. Good.

.github/workflows/helm-lint.yaml

• Adds the script to the paths trigger so it actually runs when changed. Good.
• The default-values template step redirects to /tmp/dispatch-rendered.yaml. The new step consumes that path. The redirect works on Linux GitHub-hosted runners but is fragile; see above.

scripts/check-helm-probes.py

• Static checks (lines 30-42): correctly assert tcpSocket.port == 3000, absence of httpGet, and httpGet.path == "/api/health". Good.
• Loopback simulation (lines 44-77): correctly shows TCP succeeds and HTTP returns 503, then a stopped server fails TCP. This satisfies the issue's "Verify that an unavailable PostgreSQL dependency fails readiness without failing the selected liveness check while the server is listening; a stopped server must still fail liveness."
• Hardening gaps (blockers): no realpath, no symlink check, no NUL/control-character check, no isfile precondition, SystemExit raised via raise SystemExit rather than an explicit sys.exit(1) (minor style; works fine).

Sources

• charts/dispatch/values.yaml diff (liveness/readiness config).
• scripts/check-helm-probes.py (full file in diff).
• .github/workflows/helm-lint.yaml (full file in diff).
• PR 1171 body: exact acceptance criteria quoted above.
• src/app/api/health/route.ts / route.test.ts referenced by the issue for readiness contract.
• deploy/ci/postgres.yaml:60 (in-tree reference for the same TCP-up-DB-down simulation pattern).
• CI check list (Build, Typecheck, Lint, Tests, etc.) all green at HEAD 82a61e3.

Standards Compliance

• AGENTS.md mandates input validation and fail-closed behavior; the new script accepts an arbitrary filesystem path without validation — violated.
• AGENTS.md "Lint/typecheck blocks CI; must pass" — green in CI, but a lint policy in eslint does not cover this Python file. The Python script has no pytest coverage (no test file in tests/ for it). Consider adding scripts/check-helm-probes.test.py (or equivalent) so future regressions are caught.
• Repository conventions for CI gates (.github/workflows/ci.yaml etc.) are not violated; the new workflow step is additive.

Linked Issue Fit

• Acceptance criterion "Use a process-level, database-independent liveness check … TCP on port 3000 is sufficient" — met by the values.yaml change.
• "Retain /api/health for readiness and preserve its existing database-aware response contract" — met (no change to src/app/api/health/route.ts).
• "Render/validate the chart and assert liveness is database-independent and readiness remains /api/health" — met by scripts/check-helm-probes.py static assertions and the new workflow step.
• "Verify that an unavailable PostgreSQL dependency fails readiness without failing the selected liveness check while the server is listening; a stopped server must still fail liveness" — met by the loopback simulation block.
• "No application auth changes, database configuration changes or deployment administration are required" — met.

Tool Harness Findings

The native tool-calling loop issued no tool calls; reviewing the corpus directly (no additional evidence gathered beyond the diff and supporting files).

Claims to Falsify

• Claim 1 ("Add CI assertions against the rendered main Deployment so future chart changes cannot silently recouple liveness or break readiness") — Held for static assertions: scripts/check-helm-probes.py lines 30-42 reject httpGet in liveness and require tcpSocket.port == 3000, and require readinessProbe.httpGet.path == "/api/health". Held for CI wiring: .github/workflows/helm-lint.yaml runs helm template then invokes the script. Unverified items: the script has no regression test (no tests/ or *.test.py companion), so a future change that bypasses the assertion (e.g. moves probes into a sidecar container or a PodTemplate override) would not be caught unless CI happens to be re-run; and the script accepts any path, so a workflow parameterization could trivially point it at a wrong/stale rendered file. Held: 2/3 items; unverified: 1/3 (regression-test coverage).

Unknowns or Needs Verification

• Whether /tmp is reliably preserved across steps on every GitHub-hosted runner variant (ubuntu-latest today; matrix variants could regress).
• Whether the chart's sidecar/init containers (none referenced in the diff) could carry competing probes that would not be visible to container.get("livenessProbe", {}) for the main container — the script only inspects the main container, so a sidecar probe could be silently introduced. Out of scope for the issue but worth a follow-up audit.
• Whether security-audit.yaml or a follow-up Trivy/CodeQL pass will scan the new Python file (no Python audit step visible in the workflow list).

Unaddressed required checks

The classifier marked these checks as required for this PR's risk profile, but the review does not resolve or disposition them:

• review for path traversal vulnerabilities
• test with edge-case paths (null bytes, symlinks)

Requirement trace

1 of 3 requirement(s) not fully traced to enforcement and a test:

• req-ec9624dc5888 — unverifiable (charts/dispatch/values.yaml:49): values.yaml now configures the liveness probe as TCP on port 3000 (decoupled from DB) while readiness remains HTTP GET /api/health on port 3000 (reflects DB). The check-helm-probes.py script asserts that liveness uses tcpSocket:3000 with no httpGet, asserts readiness uses httpGet path=/api/health po