Walkthrough

This PR makes template selection depend on CLI args or configured setup templates, refactors image-variable replacement, and adds review-app health-check workflow wiring with matching docs and tests.

Changes

Template selection and parsing

Review-app health checks

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

documentation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands.}

Code Review

Overview

This PR fixes a real bug: the duplicate-template validator was scanning every *.yml in the templates directory instead of only the templates that would actually be applied. The fix limits validation to the selected set (setup_app_templates, explicit CLI args, or the full glob as fallback), and adds lazy cp.latest_image evaluation so image API calls are skipped when no template uses image placeholders.

The change is well-scoped and the core logic is correct. A few findings below.

Issues

1. existing_arg_template_filenames silently eats missing-template errors

When config.args names a template that does not exist on disk, the method returns [] and the template validation becomes a no-op — no [FAIL], no error message, the command just moves on. The intent (let apply-template report missing templates itself) is correct, but it is invisible to a reader. A comment explaining the deliberate skip would prevent future confusion; see inline comment.

2. replace_legacy_image_variable check is a substring of the modern placeholder

yaml_file.include?("APP_IMAGE") also matches {{APP_IMAGE}} and {{APP_IMAGE_LINK}} because APP_IMAGE is a substring of both. The current call order — replace_image_variables always runs first and removes those modern forms before replace_legacy_image_variable is reached — makes this safe today. But correctness now depends silently on call order. If the order ever changes, a template with only {{APP_IMAGE_LINK}} would cause cp.latest_image to be called a second time and APP_IMAGE_LINK would be mangled into {latest_image}_LINK. See inline comment for a more defensive guard.

3. Test coverage gap: deprecated APP_IMAGE variable warning

The warns about deprecated variables spec was updated to use app-with-deprecated-variables-no-image.yml, which intentionally omits the legacy APP_IMAGE placeholder, and the assertion was flipped from to include → not_to include for that variable. This means no test currently exercises the code path that detects and reports the APP_IMAGE → {{APP_IMAGE}} deprecation. Adding a dedicated spec (or restoring coverage via a separate template fixture that contains APP_IMAGE but no network-requiring image calls) would prevent a silent regression in that branch.

Minor / Positive Notes

• The lazy-image optimization in replace_image_variables / replace_legacy_image_variable is clean and the new template_parser_spec test pins the behaviour well.
• Replacing Dir.glob-stubbing with dedicated dummy apps makes the doctor specs significantly more realistic and readable.
• ensure_templates_exist! raises a properly-formatted ValidationError with per-template file paths — consistent with the rest of the service.

Greptile Summary

This PR fixes a false-positive duplicate-template validation error triggered when an app's setup_app_templates only selects a subset of all template files in the directory. Doctor and setup-app now validate only the configured templates instead of globbing every .yml file. A secondary optimization lazily fetches cp.latest_image only when a template actually contains an image placeholder.

• DoctorService#template_filenames now resolves the active set from setup_app_templates (with a full-glob fallback for apps without any configured templates) and raises a ValidationError immediately if a configured template file is missing.
• TemplateParser#replace_variables was refactored into replace_image_variables / replace_legacy_image_variable helpers that guard on include? before calling cp.latest_image, avoiding a network/API call for templates that carry no image references.
• Test suite converted from Dir.glob stubs to dedicated app configs (duplicate-templates, alternate-app-template, deprecated-template, missing-template) with matching template fixtures, making each scenario self-contained and runnable against the real code path.

Confidence Score: 4/5

Safe to merge; the core validation logic is correct and the two-phase missing-template flow for apply-template is intentional.

The selected-template filtering, the lazy image fetch, and the ensure_templates_exist! error path all behave correctly. The only note is that existing_arg_template_filenames silently returns [] when files are absent — by design for the apply-template pre-flight — but this implicit contract is undocumented and could surprise future contributors adding a new command with positional-arg templates.

lib/core/doctor_service.rb — the existing_arg_template_filenames contract deserves a comment; all other files are straightforward.

Important Files Changed

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[DoctorService#validate_templates] --> B[template_filenames]
    B --> C{config.args.any?}
    C -- Yes\napply-template path --> D[existing_arg_template_filenames]
    D --> E{All files exist?}
    E -- No --> F[return empty array\nlet apply-template handle error]
    E -- Yes --> G[return filenames]
    C -- No\ndoctor / setup-app path --> H{setup_app_templates\nconfigured?}
    H -- No --> I[Dir.glob all *.yml\nfull fallback]
    H -- Yes --> J[map names to filenames]
    J --> K[ensure_templates_exist!]
    K --> L{Any missing?}
    L -- Yes --> M[raise ValidationError\nMissing templates]
    L -- No --> G
    G --> N[TemplateParser#parse filenames]
    I --> N
    F --> N
    N --> O[replace_variables per file]
    O --> P{includes APP_IMAGE\nor APP_IMAGE_LINK?}
    P -- Yes --> Q[cp.latest_image\nreplace image vars]
    P -- No --> R[skip image fetch]
    Q --> S[check_for_duplicate_templates]
    R --> S
    S --> T{Duplicates?}
    T -- Yes --> U[raise ValidationError\nDuplicate templates]
    T -- No --> V[warn_deprecated_template_variables]

_{Reviews (1): Last reviewed commit: "Validate selected setup templates" | Re-trigger Greptile}

Code Review

Overview

This PR scopes template validation in doctor/setup-app to only the templates selected by setup_app_templates, avoiding false-positive duplicate-kind/name errors when a repo intentionally has alternate template files (e.g. app.yml vs app-review.yml). It also lazily defers cp.latest_image calls until a template actually references an image placeholder. The motivation and approach are sound.

Bug: existing_arg_template_filenames silently passes when files are missing

See inline comment on lib/core/doctor_service.rb:101–106.

When explicit CLI template args are passed to doctor and any of those files don't exist, the method returns []. parse([]) produces zero templates, all checks trivially pass, and the user sees [PASS] templates — without any indication that their template arguments were ignored. The fix is to call ensure_templates_exist! here exactly like the setup_app_templates branch does.

This code path also has no spec coverage (all the new tests drive doctor without positional template args, so config.args is always empty). A test like the apply_template_spec missing-template case would close that gap.

Test coverage gap: APP_IMAGE deprecation warning

See inline comment on spec/command/doctor_spec.rb:106.

The old test verified that APP_IMAGE -> {{APP_IMAGE}} is reported as deprecated. The new test switches to a no-image fixture and asserts it is not reported. The deprecation detection still exists in new_variables, but there is no longer a test that confirms it fires when a bare APP_IMAGE is present.

Everything else looks good

• The lazy image fetch in replace_image_variables / replace_legacy_image_variable is correctly ordered relative to the existing find_deprecated_variables call — no false guards are possible.
• New fixtures (app-review.yml, app-with-deprecated-variables-no-image.yml) and controlplane.yml app entries are well-structured and cover the intended scenarios.
• The ensure_templates_exist! error format (name + path) is clearer than before.
• Docs bump to v5.1.1 is appropriate.

Code Review

Overview

This PR fixes a real-world bug (TanStack review apps failing cpflow 5.1.1 duplicate-template validation) and closes a CI gap (review-app deploys marking success before the workload was actually healthy). The two changes are independent and well-scoped.

What's Good

• Targeted fix for the duplicate-validation bug. Scoping validation to setup_app_templates rather than all templates on disk is the right approach. The fallback to all-templates when setup_app_templates is unset preserves backward compatibility cleanly.
• Lazy image fetching. The new guards in replace_image_variables and replace_legacy_image_variable avoid an unnecessary cp.latest_image API call for templates that don't reference the image. This is a good perf improvement.
• Health check wiring. Re-using the existing cpflow-wait-for-health composite action is the right call. The new REVIEW_APP_HEALTH_CHECK_* env-var knobs allow teams to tune review-app checks independently from production promotion.
• Test fixtures are realistic. Using distinct dummy app entries (duplicate-templates, missing-template, alternate-app-template, etc.) is cleaner than over-using stub_template_filenames and makes the fixture intent self-documenting.

Issues

Three inline comments were posted; summaries below:

1. existing_arg_template_filenames silently returns [] (lib/core/doctor_service.rb:103): When doctor is invoked with explicit template args pointing to missing files, validation silently passes rather than reporting the missing files. This is asymmetric with the setup_app_templates path (which raises ValidationError). If the silent behavior is intentional (to preserve apply-template's own error handling), a one-line comment would prevent future confusion.

2. Double cp.latest_image call possible (lib/core/template_parser.rb:77): A template containing both the new-style {{APP_IMAGE}} token and the legacy bare APP_IMAGE token would trigger two separate cp.latest_image calls — once in replace_image_variables and again in replace_legacy_image_variable. In a race, these could return different images. Low probability in practice, but worth caching the result once per file.

3. "Retrieve app URL" step doesn't guard on health-check outcome (.github/workflows/cpflow-deploy-review-app.yml:462): Safe today because a failing health-check step causes GitHub Actions to skip subsequent steps by default. But if continue-on-error: true were ever added to the health-check step, the deploy would incorrectly be finalized as successful. An explicit && steps.health-check.outputs.healthy == 'true' guard would make the intent clear and future-proof the logic.

Minor Notes

• Default values for the health-check knobs are defined in three places (workflow env:, action.yml defaults, and docs). This is an acceptable tradeoff for self-documenting workflows, just worth being aware of when changing defaults.
• The deprecated-variable test now correctly asserts that APP_IMAGE does not trigger a deprecation warning for the app-with-deprecated-variables-no-image fixture — consistent with the lazy-fetch change meaning APP_IMAGE is no longer eagerly detected in templates that don't use it.

Review

This PR scopes template validation to the setup_app_templates selection (fixing a duplicate-GVC false positive for TanStack review apps that intentionally share a GVC name across templates), lazily fetches the latest Control Plane image only when a template actually needs it, and gates GitHub deployment success on workload health via the existing cpflow-wait-for-health action.

The CI/health-check wiring is solid — the finalize step reads job.status so a failed health check correctly propagates to a GitHub deployment failure rather than silently succeeding. Two issues surfaced from the review:

Bug: explicit-arg template path silently passes validation when a named template is missing

lib/core/doctor_service.rb:104

When config.args is non-empty (e.g. apply-template foo bar), existing_arg_template_filenames returns [] if any file does not exist on disk. validate_templates then calls @template_parser.parse([]), which trivially succeeds — [PASS] templates is printed despite a missing template.

The non-args path (when setup_app_templates is configured) calls ensure_templates_exist! which raises a ValidationError listing missing files. The two paths are asymmetric: one gives an actionable failure, the other manufactures a false pass.

Suggested fix — replace the silent empty-list return with the same guard used on the other path:

def existing_arg_template_filenames
  filenames = config.args.map { |name| @template_parser.template_filename(name) }
  ensure_templates_exist!(config.args, filenames)
  filenames
end

This raises a ValidationError listing each missing template by name and path, matching the behaviour of the setup_app_templates branch and giving the operator an actionable failure before any side-effects occur.

Maintenance: health-check defaults are hardcoded in two places

.github/workflows/cpflow-deploy-review-app.yml:33-36

The four fallback literals ('24', '15', '200 301 302', '10') are also declared as default: values in .github/actions/cpflow-wait-for-health/action.yml. Because the workflow env block always resolves to a non-empty string (the || '...' fires when the repo variable is unset), the action's own defaults are permanently shadowed and become dead code. A maintainer who updates the action's defaults will see no change in behaviour without also updating the workflow literals.

Simpler approach — pass the repo variable directly and let the action's declared defaults serve as the single source of truth:

REVIEW_APP_HEALTH_CHECK_RETRIES: ${{ vars.REVIEW_APP_HEALTH_CHECK_RETRIES }}
REVIEW_APP_HEALTH_CHECK_INTERVAL: ${{ vars.REVIEW_APP_HEALTH_CHECK_INTERVAL }}
REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES: ${{ vars.REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES }}
REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME: ${{ vars.REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME }}

When the repo variable is unset the env var is an empty string; the action receives '' and falls through to its default: value. Defaults live in one place.

Review: Validate selected setup templates and review-app health

This PR fixes a real user-facing bug (duplicate-template validation against all templates instead of the selected set) and closes a deploy-status gap (review-app marking success before the workload is healthy). The Ruby changes are well-structured; the GitHub Actions integration is clean. Four findings below, ranging from a dead guard to a DRY violation.

Finding 1 — Dead guard clause in replace_legacy_image_variable (lib/core/template_parser.rb:74)

The second clause && !yaml_file.include?("{{APP_IMAGE") is always true when this method is reached. replace_image_variables runs first (in replace_variables) and replaces all {{APP_IMAGE}} / {{APP_IMAGE_LINK}} tokens before the call reaches replace_legacy_image_variable. At that point no {{APP_IMAGE prefix survives, so the include? check can never return true.

The clause was evidently added to prevent /\bAPP_IMAGE\b/ from matching inside {{APP_IMAGE}}, but that match is already impossible: { is not a word character, so \b before A never fires inside {{APP_IMAGE}}. The guard provides no protection. See inline comment.

Finding 2 — Deprecation-warning coverage silently narrows when setup_app_templates is configured (lib/core/doctor_service.rb:94-95)

Before this PR, validate_templates globbed all *.yml files in the templates directory, so warn_deprecated_template_variables caught deprecated variables in every template regardless of which ones were selected for setup. After the PR, when setup_app_templates is explicitly configured, only those files are passed to parse(), and @template_parser.deprecated_variables reflects only what was parsed. Templates present on disk but not listed in setup_app_templates are silently excluded from the deprecation scan.

This is arguably intentional ("validate only what you use") and the PR description acknowledges the targeted-validation goal, but the narrowing also affects the deprecation scan — a distinct concern that isn't mentioned. A comment noting this behaviour would prevent future confusion. See inline comment.

Finding 3 — ensure_templates_exist! duplicates ApplyTemplate#ensure_templates! (lib/core/doctor_service.rb:108-117)

Both methods do the same thing: zip names to paths, find missing files, build a coloured "Missing templates:" message, and raise. The differences are cosmetic (error type: ValidationError vs plain RuntimeError; parameter shape: two arrays vs one hash). If the error message wording or format is ever updated, it will need to be changed in both places independently. See inline comment for a pointer to the existing method.

Finding 4 — steps.health-check.outputs.healthy == 'true' guard on "Retrieve app URL" is redundant (.github/workflows/cpflow-deploy-review-app.yml:462)

The health-check action (cpflow-wait-for-health) only exits 0 when it sets healthy=true. When retries are exhausted or the workload is not found, it exits 1, which puts the job into failure state and causes all subsequent non-always() steps to be skipped automatically — including "Retrieve app URL". The extra && steps.health-check.outputs.healthy == 'true' condition therefore only adds value if the action exits 0 without writing the output, which does not happen. The condition creates a false impression that it is an independent safety net. See inline comment.

Code review posted via inline comments — see thread below.

Review summary: two inline comments posted. (1) Medium — latent health-check bypass on the Retrieve app URL step condition in cpflow-deploy-review-app.yml: GHA implicit step-skipping is correct today, but adding continue-on-error: true to health-check in the future would allow a false-success deployment status. Suggest adding steps.health-check.outcome == success to the if: guard. (2) Low/cleanup — existing_arg_template_filenames in doctor_service.rb is called once and is structurally identical to the configured-template branch; a shared resolve_template_filenames(names) helper would remove the duplication. No correctness regressions found in the template-variable substitution refactor — the word-boundary regex and find_deprecated_variables ordering are both correct.

Review: Validate selected setup templates and review-app health

This PR fixes two distinct issues cleanly: the TanStack duplicate-template false-positive (by scoping doctor's template validation to the app's setup_app_templates selection) and a deploy-status gap where a crash-looping workload could still produce a "success" GitHub deployment. The Ruby changes are well-structured and the new test fixtures are a clear improvement over stub_template_filenames.

Two maintenance findings survived verification:

1. Health-check defaults are duplicated between workflow and action

The four REVIEW_APP_HEALTH_CHECK_* env vars defined at the workflow level ('24', '15', '200 301 302', '10') are identical to the default: values already declared in .github/actions/cpflow-wait-for-health/action.yml (lines 21, 25, 32, 36). Because the workflow env vars always resolve to a concrete value before the action sees them, the action's own defaults are permanently shadowed. If the action's defaults are tuned in the future (e.g., bumping retries for slower environments), review-app deployments will silently continue using the old values from the workflow env until both places are updated.

The defaults belong in one place. The simplest fix is to drop the workflow-level env vars and write the repo-var fallback inline at the with: call site, co-located with where they're consumed:

with:
  max_retries: ${{ vars.REVIEW_APP_HEALTH_CHECK_RETRIES || '24' }}
  interval_seconds: ${{ vars.REVIEW_APP_HEALTH_CHECK_INTERVAL || '15' }}
  accepted_statuses: ${{ vars.REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }}
  curl_max_time: ${{ vars.REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME || '10' }}

2. ensure_templates_exist! duplicates formatting logic from ApplyTemplate#ensure_templates!

DoctorService#ensure_templates_exist! (doctor_service.rb:111–120) and ApplyTemplate#ensure_templates! (apply_template.rb:85–95) both iterate name+filename pairs and format missing entries as " - #{name} (#{filename})". If the error format changes in one place (e.g., to include the template directory or a hint URL), the other won't follow. The logic is different enough in error propagation (ValidationError vs RuntimeError + progress.puts) that a direct extraction may not be trivial, but the formatting string itself could live in a shared helper on TemplateParser.

No correctness bugs were found. The {{APP_IMAGE}} substring-safety question (whether .include?("{{APP_IMAGE}}") could fire on a string containing only {{APP_IMAGE_LINK}}) was verified to be safe — Ruby correctly returns false for that check. The \bAPP_IMAGE\b regex correctly protects APP_IMAGE_LINK (underscore is \w, no word boundary after IMAGE), and the replacement ordering ensures {{APP_IMAGE}} tokens are consumed before the legacy regex runs.

Review: Validate selected setup templates and review-app health

This PR does three things: narrows template validation to the setup_app_templates subset (fixing TanStack's false-duplicate error), lazily fetches the latest image only when templates actually reference it, and gates the GitHub deployment success on a real workload health check. The logic and test coverage are solid overall.

Three findings worth addressing:

1. find_deprecated_variables scans the post-substitution YAML — false-positive risk
lib/core/template_parser.rb line 49/55

replace_image_variables is called first (line 49), replacing {{APP_IMAGE}} with the actual image string. The find_deprecated_variables(yaml_file) call at line 55 then does a plain .include?("APP_IMAGE") scan on the already-substituted string. If the image name contains APP_IMAGE as a substring (e.g. ghcr.io/company/APP_IMAGE-utils:v1), the deprecation warning fires for a template that never used the deprecated bare form.

Fix: scan the raw file content before any substitution, or apply the same \bAPP_IMAGE\b word-boundary pattern that replace_legacy_image_variable already uses.

2. apply_template.ensure_templates! is now dead code for the primary error path
lib/command/apply_template.rb lines 85-95

VALIDATIONS = %w[config templates] causes DoctorService#validate_templates to run before call. The new ensure_templates_exist! in DoctorService exits the process before call is ever reached, so the existing ensure_templates! method only fires when DISABLE_VALIDATIONS=true. That edge case is undocumented, and the two guards emit inconsistent error formats ([FAIL] templates vs. a plain progress print + RuntimeError).

Suggestion: add a comment explaining ensure_templates! is retained as a DISABLE_VALIDATIONS fallback, or remove it if that escape hatch is not intended to be supported.

3. TemplateParser#latest_image double-caches an already-memoized call (minor)
lib/core/template_parser.rb line 82

Controlplane#latest_image already memoizes per-GVC, so the @latest_image ||= on the TemplateParser adds a second cache layer that is a no-op in production. The real lazy-fetch guard is the early return in replace_image_variables (line 68). The TemplateParser ||= does matter for instance_double test doubles (it is what makes spec/core/template_parser_spec.rb:110 pass with have_received(:latest_image).once), but this dependency is invisible at the call site. A brief comment would make the intent clear.

Address-review summary

Scan scope: full PR review data for the two current unresolved template parser threads, after pr-security-preflight --repo shakacode/control-plane-flow 363 returned SECURITY_PREFLIGHT_OK at head 881be7427c6300ad4d85c6dc87a9ea57c7592d46.

Mattered

• Addressed lib/core/template_parser.rb deprecated image-token handling in 289de2c: legacy replacements now use a protected token matcher before modern substitutions, and deprecation scanning reads the original template text so substituted image values cannot trigger false warnings or legacy rewrites.
• Addressed the parser-level image memoization thread in 289de2c with a short comment clarifying that one parser instance shares a single image value across modern and legacy image replacements.
• Tightened spec/core/template_parser_spec.rb so the image_link double expects the latest image argument, and added regression coverage for modern {{APP_IMAGE}} values containing legacy-token text.

Validation

• CPLN_ORG=test-org bundle exec rspec spec/core/template_parser_spec.rb spec/command/doctor_spec.rb -> 18 examples, 0 failures.
• bundle exec rubocop -> 200 files inspected, no offenses.
• git diff --check -> pass.
• .agents/bin/lint / .agents/bin/validate are absent from this PR branch checkout; I used the equivalent direct commands from the repo seam (bundle exec rubocop, bundle exec rake).
• Full CPLN_ORG=shakacode-heroku-to-control-plane-ci RSPEC_RETRY_RETRY_COUNT=1 bundle exec rake was attempted, entered long-running external Control Plane integration operations, emitted multiple failure markers without a final report, and was cleanly interrupted per coordinator redirect after exceeding the practical validation window. Treat full-suite validation as a gap for this worker pass.

Workflow Change Audit: existing PR workflow diff, unchanged by 289de2c

• Semantic classification: semantic workflow behavior change in .github/workflows/cpflow-deploy-review-app.yml; it adds a post-deploy review-app health wait before retrieving the app URL.
• on: triggers: no change in the workflow diff.
• permissions:: no change in the workflow diff.
• Secret references: no new secrets.* references in the workflow diff; the new step uses existing review config outputs and vars.REVIEW_APP_HEALTH_CHECK_* knobs.
• Third-party actions: no third-party action added or version-changed; the new step uses the repo-local ./.cpflow/.github/actions/cpflow-wait-for-health action.
• Follow-up exercise tracking: not created by this review-fix lane; coordinator should decide whether existing PR evidence is sufficient before final merge closeout.

Skipped

• None for the two current unresolved threads.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

Review summary

This PR does two things: (1) makes cpflow doctor/apply-template template validation operate only on the setup_app_templates-selected subset (falling back to scanning all templates when unconfigured), fixing a false "duplicate templates" error for apps like TanStack's review app that intentionally have overlapping app.yml/app-review.yml; and (2) adds a blocking health-check step (reusing cpflow-wait-for-health) to the review-app deploy workflow so a crash-looping workload no longer gets marked as a successful GitHub deployment. The core logic is solid — the regex-based legacy-variable detection, lazy latest_image fetching, and the workflow's always()-based status finalization all check out against their edge cases, and the new specs cover the key branches well.

Left inline comments on a few lower-severity items:

• lib/core/doctor_service.rb: duplicate names in setup_app_templates (or apply-template args) aren't deduped, which can produce a confusing "Duplicate templates found" error instead of flagging the real cause (a repeated entry).
• lib/core/doctor_service.rb's new ensure_templates_exist! duplicates Command::ApplyTemplate#ensure_templates!'s zip/reject/format logic, but the two already diverge in error class and message — worth consolidating into one shared helper (e.g. on TemplateParser).
• .github/workflows/cpflow-deploy-review-app.yml: "Retrieve app URL" re-queries cpln workload get for the endpoint that the new health-check step's composite action already fetched on its last successful poll — could reuse an endpoint output instead.
• The new REVIEW_APP_HEALTH_CHECK_* repo variables are a fully independent config surface from production's HEALTH_CHECK_* env in cpflow-promote-staging-to-production.yml (which only exposes ACCEPTED_STATUSES as overridable and has no CURL_MAX_TIME at all) — not a bug, but the two configs will keep drifting apart with different names/knobs.
• spec/command/doctor_spec.rb: the Dir.glob fallback path (no setup_app_templates configured) only has failure-case coverage now; there's no test asserting it still passes cleanly when templates have no issues.

Two other notes not tied to a single diff line:

• CHANGELOG.md wasn't updated. Per AGENTS.md ("Changelog: CHANGELOG.md — Keep-a-Changelog; user-visible changes only"), this PR has at least two user-visible changes worth an entry: the review-app health-check gating behavior and the new REVIEW_APP_HEALTH_CHECK_* variables, plus the doctor/apply-template template-validation behavior change.
• The workflow file repeats the condition steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') verbatim across ~18 steps, and this PR itself had to touch two of them (the new health-check step, and appending && steps.health-check.outcome == 'success' to "Retrieve app URL"). Pre-existing pattern, but a good candidate for a shared step output/env var to make future edits like this one less error-prone.

Nothing found that looks like a release blocker.

Review

This PR (1) narrows the doctor "templates" validation to only parse the templates selected via setup_app_templates/explicit apply-template args instead of every .yml file in the templates directory (fixing a false-positive duplicate-GVC-name error when an app has both app.yml and app-review.yml but only uses one), (2) makes TemplateParser fetch cp.latest_image lazily and only when a template actually uses an image placeholder, using word-boundary regexes to correctly distinguish legacy bare APP_IMAGE/APP_ORG/etc. from the modern {{...}} syntax and from lookalikes like APP_IMAGE_LINK, and (3) adds a "Wait for deployment health" step to the review-app deploy workflow, reusing the existing cpflow-wait-for-health composite action so a deployment is only marked successful once the primary workload is ready and returns an accepted HTTP status.

I traced the new/changed code line by line, checked every caller of the changed methods (TemplateParser#parse, DoctorService#run_validations, the workflow's new step against its if:/output dependencies), audited what behavior the diff removes or narrows, and ran dedicated reuse/simplification/efficiency/altitude/convention passes. Verification:

• The apparent "coverage narrowing" (doctor no longer validates template files that aren't referenced by any app's setup_app_templates) is intentional and explicitly tested (spec/command/doctor_spec.rb's "passes if unselected templates render duplicate kind/names" case), not an oversight — verified against the PR description and inline comments in the code.
• The legacy-variable word-boundary regex, replacement ordering, and lazy/memoized latest_image fetch were all traced against edge cases (e.g. APP_IMAGE_LINK vs bare APP_IMAGE, image values containing APP_IMAGE as a substring) and hold up; the new specs cover exactly these cases.
• The new workflow step's if: conditions, output wiring (steps.review-config.*, steps.health-check.outcome), and the "Finalize deployment status" step's use of job.status are self-consistent — a failing health check correctly fails the job and skips "Retrieve app URL" via default GH Actions short-circuiting, and cpflow-wait-for-health's inputs match its existing usage in cpflow-promote-staging-to-production.yml.

One low-severity item survived as worth a mention (posted inline on lib/core/doctor_service.rb):

• lib/core/doctor_service.rb:112 — ensure_templates_exist! duplicates Command::ApplyTemplate#ensure_templates!'s "missing templates" list-building/raising logic. Not a bug (both paths are reachable — ApplyTemplate's copy still matters when DISABLE_VALIDATIONS=true), but the two could drift apart over time; a shared helper would be a nice-to-have cleanup, not a blocker.

No correctness, security, or performance bugs found in this diff.