Preserve review-app renderer startup grace

[justin808]

Why

The review-app renderer template used a probe field unsupported by the deployment provider. Removing it alone would also remove the intended cold-start grace and could let liveness restart the renderer during normal pre-seeding.

This addresses #784. The issue intentionally remains open until the merged default workflow is exercised by a fresh disposable verification PR, behavioral smoke passes, and cleanup is confirmed.

What changed

• Remove the unsupported startup probe.
• Preserve its intended 120-second startup allowance with the provider-supported liveness initial delay.
• Add a rendered-template regression that locks the supported delay, probe thresholds, runtime arguments, image, and port.

Workflow change audit

• GitHub workflow files, triggers, permissions, action references, source authorization, and credential-reference shape are byte-identical to the base.
• Readiness behavior and liveness thresholds are unchanged; only the supported liveness delay replaces the unsupported field.
• Security preflight passed; no untrusted issue content was used as instructions.
• Read-only QA and a maker-distinct Sol/xhigh audit found no blocking or discuss findings at the current head.
• The required simplification review completed with no changes.

Verification

• bin/conductor-exec bin/test-cpflow-github-flow
• bin/conductor-exec .agents/bin/validate — 58 files, no offenses
• bin/conductor-exec .agents/bin/test — 51 RSpec examples and 16 Jest tests, zero failures
• YAML parsing, actionlint, protected-path comparison, and git diff --check
• Negative mutations for missing delay, reintroduced unsupported probe, and changed startup command

Churn and follow-through

• Pre-push review: QA, maker-distinct Sol/xhigh adversarial audit, and simplification gate.
• Post-push fix rounds: 0 at PR creation.
• Process-gap disposition: checklist+replay — replay the merged default workflow from a fresh disposable PR before closing Follow-up: Repair review-app setup and exercise deployed smoke #784.

Summary by CodeRabbit

• Bug Fixes

  • Improved node-renderer startup behavior by delaying readiness checks until the service has had more time to initialize.
  • Removed the startup probe to prevent premature health-status failures.

• Tests

  • Added validation to ensure the node-renderer runtime configuration, networking, and health checks remain consistent.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 60edbc3b-51c9-4b35-a6fb-b95a7618afb2

📥 Commits

Reviewing files that changed from the base of the PR and between 5ea9901 and d7663c9.

📒 Files selected for processing (2)

• .controlplane/templates/node-renderer.yml
• bin/test-cpflow-github-flow

---

Walkthrough

The node-renderer template removes its startup probe and delays readiness checks by 120 seconds. The cpflow test script now renders the template and validates its container and probe configuration against expected values.

Changes

Node renderer probes

Layer / File(s)	Summary
Update node-renderer probe behavior .controlplane/templates/node-renderer.yml	The startup probe is removed, TCP readiness and liveness checks remain on port 3800, and readiness checks use a 120-second initial delay.
Validate rendered probe configuration bin/test-cpflow-github-flow	The script substitutes template placeholders, parses the rendered YAML, and verifies command, arguments, image, ports, and probe settings, including rejection of a startup probe.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• shakacode/react-webpack-rails-tutorial#777: Introduces and tunes node-renderer probe configuration that this change validates.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: preserving review-app renderer startup grace while adjusting probe behavior.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/fix-review-app-gate

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

🚀 Quick Review App Commands

Welcome! Here are the commands you can use in this PR:
They require the repository to have cpflow review apps configured, including the CPLN_TOKEN_STAGING secret.

+review-app-deploy

Deploy your PR branch for testing.

+review-app-delete

Remove the review app when done.

+review-app-help

Show detailed instructions, environment setup, and configuration options.

Comment +review-app-help for full setup details.

[claude]

Review: Preserve review-app renderer startup grace

Overview

This PR swaps an unsupported startupProbe (tcpSocket, failureThreshold: 60/periodSeconds: 2 = 120s grace) for the provider-supported initialDelaySeconds: 120 on the existing livenessProbe, and adds a Ruby-based regression test in bin/test-cpflow-github-flow that renders .controlplane/templates/node-renderer.yml and asserts the probe/args/ports/image shape.

Correctness

• The math checks out: 60 × 2s = 120s, matching the new initialDelaySeconds: 120, so the intended cold-start grace is preserved numerically.
• readinessProbe is untouched (failureThreshold: 3, periodSeconds: 5), which is fine since readiness failures don't cause restarts.
• One thing worth confirming before merge (left as an inline comment): the entire premise of this fix is that the provider silently doesn't support startupProbe. There's no prior use of initialDelaySeconds anywhere else in .controlplane/ to establish it's actually honored by this provider — if it's likewise unsupported/ignored, this PR would silently reintroduce the bug it's fixing, and the new test wouldn't catch that since it only validates the rendered YAML shape, not runtime behavior against the actual provider.
• Minor behavioral note: previously, once startupProbe succeeded (as soon as the port opened), liveness monitoring began immediately. With a flat initialDelaySeconds: 120, liveness checks don't start until 120s regardless of how fast the renderer actually comes up, so a crash shortly after a fast startup (e.g. t=20s) won't be caught until ~t=150s instead of much sooner. Likely an acceptable tradeoff for a review-app renderer, just flagging it as a real (small) change in crash-detection latency.

Test coverage

• Good addition: the new test locks in the rendered probe/args/port/image shape and explicitly asserts startupProbe is absent, guarding against regressions in either direction (reintroducing the unsupported field, or losing the grace period).
• I hand-verified the expected values in the test against the current template content and they match exactly (args heredoc, ports, both probes).
• The container.key?("command") check is currently vacuous (the template never sets command, only args), but it's a harmless forward-looking guard, not a bug.

Style / scope

• Change is minimal and focused — no unrelated modifications.
• Comments explaining why the grace period exists are preserved appropriately.

Security

• No security concerns; this is a deployment-probe config change with no secrets, credentials, or workflow-permission changes involved (confirmed via diff — only the two files shown are touched).

Overall this is a small, well-tested, low-risk fix. The one thing I'd want confirmed before merge is that initialDelaySeconds is genuinely supported by the CPLN provider for livenessProbe, since that's the crux of whether this actually fixes the underlying issue (#784) or just changes which field silently no-ops.

[justin808]

Address-review summary

Scan scope: full current-head history via check all reviews.

Mattered

• None. No code changes or push were needed.

Skipped

• Provider-support thread: verified and declined for code changes. The authoritative provider reference supports an initial delay of 0–120 seconds, and the configured 120-second delay deliberately preserves slow pre-seeding grace. Evidence reply. Thread resolved.
• The general review summary duplicated the resolved provider-support concern; no separate action.
• Two automation/status comments were non-actionable; no action.

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

[greptile-apps]

Greptile Summary

This PR preserves the renderer's cold-start allowance with provider-supported probe settings. The main changes are:

• Removes the unsupported startup probe.
• Adds a 120-second initial delay to the liveness probe.
• Adds template checks for probe and runtime settings.

Confidence Score: 5/5

This looks safe to merge.

• No blocking issues found in the changed code.
• The new liveness configuration provides the intended startup delay before failed checks can restart the container.
• The template check matches the current placeholder format and normalizes its working directory.

Important Files Changed

Filename	Overview
.controlplane/templates/node-renderer.yml	Replaces the unsupported startup probe with a 120-second liveness initial delay while retaining existing thresholds.
bin/test-cpflow-github-flow	Adds checks for the renderer template's probes, arguments, image, port, and command shape.

_{Reviews (1): Last reviewed commit: "Fix review app startup checks without dr..." | Re-trigger Greptile}

[claude]

Review: Preserve review-app renderer startup grace

Overview

Removes the unsupported startupProbe from the node-renderer Control Plane template and replaces its 120s startup grace with initialDelaySeconds: 120 on the livenessProbe. Adds a Ruby regression check in bin/test-cpflow-github-flow that renders the template and locks down the container's args, image, ports, and probe config.

Code quality / correctness

• The core fix is sound: since startupProbe isn't supported by the provider, moving the grace period onto livenessProbe.initialDelaySeconds is the right mechanism, and readinessProbe is correctly left untouched (readiness failing is non-destructive — it just excludes the pod from traffic, unlike liveness which restarts the container).
• Worth noting for the record (not a bug): this changes liveness semantics slightly. With the old startupProbe, liveness checks began as soon as the TCP port opened (bounded by a 120s cap before the startup probe itself failed and forced a restart). With initialDelaySeconds: 120, liveness checks are now always deferred a full 120s even if the app boots much faster. That's a reasonable trade-off for a "give it room to boot" grace period, just flagging it as an intentional behavior shift rather than a pure no-op.
• I couldn't independently confirm from this repo that the Control Plane provider actually supports initialDelaySeconds on livenessProbe (no other template in .controlplane/templates/ uses probes for comparison). Worth double-checking against cpflow/Control Plane's schema docs if that wasn't already done as part of the verification described in the PR body.

Test coverage

• The added Ruby check in bin/test-cpflow-github-flow is a solid regression guard — it locks the exact args shell script, image placeholder, ports, and both probes, and explicitly asserts startupProbe is absent. I traced through the logic against the current node-renderer.yml and it matches.
• Minor structural note: this check is appended after "${cpflow_cmd[@]}" github-flow-readiness in a script running under set -euo pipefail. If cpflow isn't installed/configured in the environment running this script, the new probe regression check never executes (the script exits before reaching it). This matches the existing pattern of the file (other checks like the GitHub Actions YAML parse are also gated the same way), so it's consistent with prior conventions rather than a new problem — but since this specific check doesn't actually depend on cpflow being present, it could alternatively live earlier in the script (or in a lighter-weight RSpec/standalone check) so it still runs even when cpflow isn't available locally.

Security

No security implications — this is deployment/health-check config only, no secrets or credentials touched.

Risk

Low risk, narrowly scoped change with a good regression test. Main residual risk is the provider-support assumption for initialDelaySeconds called out above, which per the PR description is intended to be verified via a fresh disposable PR before closing #784.

[justin808]

Address-review follow-up on the current-head Claude review:

• The provider’s official container reference documents Initial Delay Seconds for probes with an allowed range of 0–120 seconds, so livenessProbe.initialDelaySeconds: 120 is supported.
• The fixed 120-second liveness delay is the deliberate tradeoff already recorded for preserving cold-start grace.
• I am declining the optional test-placement suggestion. This repository documents bin/test-cpflow-github-flow as a provider-flow validation command, and its structural checks consistently run after the readiness gate. Moving only this assertion would be nit-only churn at the final-candidate stage; the unchanged current-head command has passed.

No code change or push is needed.

[justin808]

Address-review summary

Scan scope: normal cutoff mode, reviewing activity after the previous summary at 2026-07-18T07:00:49Z.

Mattered

• None.

Optional

• Declined with no code change: the suggestion to move the standalone template assertion earlier. The documented command intentionally starts with provider-flow readiness, and the existing structural checks share that ordering. Moving only this assertion would be nit-only final-candidate churn.

Skipped

• The new Greptile summary is an all-clear status with no blocking issue; no action.
• The provider-support and crash-latency notes repeat an already-closed concern or document an intentional tradeoff. Official Control Plane documentation defines Initial Delay Seconds for probes with an allowed range of 0–120 seconds, so the configured value is supported.
• Evidence-backed rationale was recorded; no further action.

Current-head readiness

• Exact head: d7663c9fa5de229f56d6634202167564cf44bc69.
• READY: 10 configured checks are terminal with 0 pending and 0 failing; 0 review threads are unresolved.
• No code change or push was needed.

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

[github-actions]

✅ Review App Deleted

Review app for PR #785 is deleted

🎮 Control Plane Console
📋 View Workflow Logs