Preserve deployed env values on redeploy and accept quoted config

[christophervoelpel]

Summary

Preserve the deployed app's DICTATION_ENABLED value when it is omitted from config.txt; use 1 when no deployed value exists. Explicit config takes precedence. The live lookup now runs after deployment confirmation and API enablement, so a first deployment can enable Cloud Run before querying it. Invalid explicit values still fail before provisioning, and failed or malformed live queries still stop deployment.

Config validation accepts double-quoted values while rejecting empty or whitespace-containing values. The development guide now describes redeploy inheritance accurately. Addresses SM-8 and SM-9.

Validation

• test/test_deploy_safety.py: 59 passed, including a first-deploy ordering regression and quoted-value checks.
• bash -n deploy.sh, shellcheck --severity=error deploy.sh, and git diff --check: passed.
• First-deploy sequencing is tested with mocked gcloud calls. No cloud deployment was performed for this follow-up.

[gps-readability-bot]

Readability review is not required.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [PavitraGahaneGoogle]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [abeyuka625]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [Jelena0921]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [HalleyInteractive]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [tmcfarlin412]

[christophervoelpel]

PR #197 — P2 — first-deploy lookup precedes API enablement

Reviewed head: 8f7399edd693f15a96910e0d3dbcba45628d28b7.

Please defer the omitted-DICTATION_ENABLED live lookup at deploy.sh:306–317 until after required API enablement, before deploying the app.

The new gcloud run services list call runs before this script enables run.googleapis.com. On a fresh project with that API disabled and the optional setting omitted, a non-interactive lookup fails and exits before reaching the advertised first-deploy default of 1. The copied template explicitly sets 1, so that ordinary path is unaffected; the bug concerns the documented omission path.

This conclusion comes from script ordering plus the existing list-failure test, not a live fresh-project deployment. The exact gcloud extraction formats were separately checked offline for 0, 1, and missing values.

Acceptance: first deploy with the optional flag omitted reaches default 1; redeploy preserves live 0; explicit local 0/1 wins; permission/network failures after provisioning still fail closed. Keep this scoped to ordering and its regression test.

[christophervoelpel]

Review: needs rework — the probe runs too early in the script

Multi-agent review (reviewer → independent critique agent re-verifying each claim against the code). Findings below survived verification; rejected recommendations are listed explicitly.

What's good first: the tests are not source-text greps — they extract the real block from deploy.sh and run it under bash with a stubbed gcloud, so precedence and every failure mode are genuinely exercised (22 pass against the branch). Quoting and set -euo pipefail interaction are correct throughout: if ! VAR=$(...) suspends errexit properly, all expansions are quoted, and the 0/1 validation runs after the live value is adopted, so a comma- or newline-bearing live value can't be injected into the --set-env-vars list at line 863.

Caution

Blocker: breaks the first deploy on a fresh project

Real line numbers on the merge result:

What	Line
New DICTATION_ENABLED probe (gcloud run services list)	~306
Deploy-target confirmation — read -r -p "Proceed and deploy to '$PROJECT'?"	390
gcloud services enable (incl. run.googleapis.com)	448
First actual use of DICTATION_ENABLED	863

So the probe queries Cloud Run ~140 lines before the API is enabled, and ~85 lines before the operator has confirmed which project they're deploying to. deploy.sh explicitly supports the brand-new-project case ("on a brand-new project it is otherwise off", ~line 413).

The new code is fail-closed — the test suite pins this exact path ({"list_fail": True} → rc 1) — so on a fresh project the deploy now aborts. I could not verify offline whether gcloud run services list hard-errors on a disabled API or interactively offers to enable it, but both outcomes are wrong here: either a first deploy that used to work now fails, or an API gets enabled on a project the user hasn't confirmed yet.

Recommended fix

1. Move the whole DICTATION_ENABLED block to just after gcloud services enable (~line 455) and before the first use at 863. This is the one change that matters.

2. Delete the env sanity probe (commit 8f7399e, ~16 lines at 319-334) along with it, plus its two test cases and the full_env / full_env_fail branches of the mock builder (~35 test lines):

   # Sanity check: the deployed app service always has env vars (e.g. ROLE, AUTH_MODE).
   # If the env list is empty, format extraction is broken; fail closed.

   It's a second full describe of the same resource, solely to detect that a gcloud projection might be wrong — defensive code for a state you can verify once by hand, at the cost of doubling preflight latency. Once the ordering in (1) is fixed, its failure mode largely can't occur. Instead, verify env.filter(name=DICTATION_ENABLED).extract(value).flatten() once against a live service and note the result in the PR. That manual check is the thing genuinely worth doing before merge.

3. Reword the error text at 313-314 / 338-339. "Set DICTATION_ENABLED explicitly in config.txt or check gcloud credentials" names the wrong cause for the commonest failures — API disabled, or a mistyped project. Say those instead.

Additional finding: the widened config regex now accepts whitespace

The new pattern ("[^"]+"|[A-Za-z0-9._$-]+) accepts anything inside double quotes, so:

REGION="us central1"

now passes config validation and fails later inside a gcloud flag, where the error is much harder to trace. This was caught before. [^"[:space:]]+ inside the quoted alternative keeps the fix (quoted values) without the hole.

Related, and deliberate as far as I can tell: PROJECT='single-quoted' is valid bash in a sourced config.txt but is still rejected. The new error text tells users to use double quotes, so that's documented rather than broken — noting it only so nobody "fixes" it twice.

Stale doc

DEVELOPING.md:284 still reads "defaults to 1 when omitted". After this PR, omitted means "inherit the live value"; it only defaults to 1 when the service is absent. The adjacent "Existing explicit 0 values are preserved" (README.md:393, DEVELOPING.md:286) is what this PR makes true, so the scope is right — just that one sentence is now stale.

Explicitly not recommended

• Don't parametrise the grep-pattern test at test/test_deploy_safety.py:637-646. Scraping the pattern out of deploy.sh and un-escaping it is ugly, but it tests the pattern that actually ships; a parametrised copy duplicates deploy.sh's quoting rules and the two will drift.
• Don't add more failure-mode test cases. The 22 bash-level tests already cover more than this feature warrants.

[christophervoelpel]

Consolidated Review

Verdict: Fix one blocker, then merge. This reconciles the 19:29 and 19:50 comments on this PR against head 8f7399e; every item below was re-verified by tracing deploy.sh callers and running the tests and probes under Evidence.

Do before merge

1. Move the DICTATION_ENABLED preservation block (deploy.sh:306-350) to run after gcloud services enable (deploy.sh:490) and before its first use in --set-env-vars (deploy.sh:905). The probe fires at line 306, well before the confirmation prompt (432) and before run.googleapis.com is guaranteed enabled (490); nothing between there and line 905 reads its output, so relocating it changes nothing before confirmation. Acceptance: test/test_deploy_safety.py -k dictation_env_preservation (10 tests) still passes moved, and a first deploy with the flag omitted on a project without Cloud Run enabled no longer aborts before confirmation.

Optional, does not block

• Tighten the quoted-value regex at deploy.sh:267 from [^"]+ to [^"[:space:]]+, so REGION="us central1" is caught at config-validation time instead of failing later inside a gcloud flag. Fails safely either way; config.txt is trusted, sourced input, not a security boundary.
• Update the stale sentence at DEVELOPING.md:284 ("defaults to 1 when omitted"): omission now inherits the live value, defaulting to 1 only when the service doesn't exist yet. DEVELOPING.md:286 and README.md:393 are already correct.
• Reword the probe failure text at deploy.sh:314, 325, 339 to name API enablement or an unconfirmed project, more probable than bad credentials once item 1 lands.

Rejected or superseded, do not re-litigate

• The 19:50 comment's call to delete the LIVE_ENV_VARS sanity check (deploy.sh:319-334): rejected. ROLE and AUTH_MODE are always present on a real deployed service, so an empty result there means gcloud's field extraction broke, distinct from LIVE_DICTATION coming back empty, which is expected on a service deployed before this feature existed. Deleting the check would collapse a fail-closed signal into the same default-1 fallback as a benign case. Keep it; merging the two describe calls is a later optimization.
• The 19:50 comment's PROJECT='single-quoted' note: documented, intentional, no fix needed.
• Line numbers differ between the 19:50 comment and this note because the 19:50 comment counted on the pre-PR file; at head the probe is at 306, the confirmation prompt at 432, services enable at 490 and the first use at 905. Same defect either way.
• The 19:50 comment's "don't parametrize the grep-pattern test" and "don't add more failure-mode cases" notes: agreed.

Evidence

• The three new gcloud projections were run unchanged against a live Cloud Run app service deployed from this repository (existing project, API enabled): the service list returned app, the env-name extraction returned ROLE,AUTH_MODE,WORKER_URL,IAP_AUDIENCE,FIRESTORE_DB_UI,DICTATION_ENABLED,DICTATION_MODE, and the DICTATION_ENABLED value extraction returned 1, all with exit code 0. This is the manual format check the 19:50 comment asked for; the extraction works.
• bash -n deploy.sh and shellcheck --severity=error deploy.sh: both clean.
• TZ=UTC python -m pytest -q test/test_deploy_safety.py: 56 passed.
• TZ=UTC python -m pytest -q test/test_deploy_safety.py -k dictation_env_preservation -v: 10 passed, list_fail already fails closed before confirmation or API enablement.
• Regex probe: REGION="us central1" matches the current pattern, rejected by [^"[:space:]]+; PROJECT='single-quoted' matches neither.
• git diff --check origin/main..HEAD: clean.
• Not run: live deployment against a fresh project with Cloud Run disabled, browser or UI checks, paid model calls.

Way forward

• Suggested order for the batch: Use an explicit MIME allow-list and support AVIF #196, Clear video source on resolve failure and keep cached blobs #205, Validate video duration limits and audio parameter types #204, Exclude archived candidates from the rendered video #199 and Clamp clip duration to the footage remaining after skip_time #200 first (independent, ready or one small fix each), then Preserve deployed env values on redeploy and accept quoted config #197 and Remove the duplicate GCS signing cache from orch.py #198 once their fixes land, then the stacked children Validate model-returned product and image ids #201 (on Use an explicit MIME allow-list and support AVIF #196), Treat an absent storyboard key as no change #203 (on Remove the duplicate GCS signing cache from orch.py #198) and Guard setup inputs on the real project load error and hydration state #202 (on Exclude archived candidates from the rendered video #199), each rebased onto main after its parent squash-merges and retargeted to main so CI runs.
• Land the reorder plus the optional regex and doc tweaks in one follow-up commit, then re-run the affected test selections.
• Confirm manually on a fresh-project deploy (Cloud Run disabled, flag omitted) that the relocated probe runs after API enablement and lands on the default of 1.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [HalleyInteractive,
  tmcfarlin412]

[christophervoelpel]

Follow-up on Consolidated Review

I addressed the required ordering fix and the accepted optional cleanup from the Consolidated Review on head d53ee53dd747.

• Moved the DICTATION_ENABLED lookup until after confirmation and gcloud services enable, before the first environment-variable use.
• Kept early validation for explicit values and the fail-closed live-environment checks.
• Tightened quoted config values to reject whitespace-containing values and corrected the development-guide wording for inheritance.

Verification: TZ=UTC python -m pytest -q test/test_deploy_safety.py passed with 59 tests, including first-deploy ordering and preservation behavior. bash -n deploy.sh, ShellCheck at error severity, and git diff --check passed. The first-deploy sequencing is covered with mocked gcloud calls; no fresh-project deployment was performed.

Current-head GitHub CI is complete: Python 3.11/3.12/3.13, UI build/lint/tests, deploy checks and security scans passed. Conditional zizmor jobs were skipped.

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [uenke]

[gps-readability-bot]

Still need readability approvals from:

• gps-readability-python-approvers -
  Can grant: [uenke]