PR 26: End-to-end hardening and release candidate validation#286
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds HTTPS/TLS gateway readiness ( ChangesRC Validation and Infrastructure
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR introduces a few release-readiness blind spots that should be addressed before using the RC validation as a merge/release gate.
Reviewed changes — I reviewed the PR 26 hardening work across setup manifest refresh, daemon/resource runtime behavior, real-artifact E2E coverage, privileged macOS RC validation, and release/user documentation.
- Refresh setup manifests before system mutation —
pv setupnow downloads or falls back to the Managed Resource artifact manifest before privileged setup steps and default-resource planning. - Install setup-default resources during reconciliation — daemon system reconciliation now installs missing desired default tracks without starting backing runtimes, including PHP/FrankenPHP pairing and Composer handling.
- Harden daemon and gateway runtime readiness — daemon startup now preserves live jobs on duplicate starts, Gateway uses CA-aware HTTPS readiness when possible, Caddy storage is CA-isolated, and RustFS/Postgres runtime contracts were aligned with real artifacts.
- Expand release-candidate validation — the PR adds a manual privileged macOS RC workflow, real-artifact resource matrix coverage, release docs checks, an RC checklist, and user-facing documentation.
GPT ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e43ead29b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for install in installs { | ||
| match install { | ||
| DesiredResourceInstall::PhpPair { track } => { | ||
| commands.install_php_pair(TrackSelector::Track(TrackName::new(track)?), &client)?; |
There was a problem hiding this comment.
When any setup default install fails here, the ? returns immediately from the system reconciliation job, so every later default in installs is skipped. In the fresh pv setup path that means a transient PHP/FrankenPHP download or validation failure prevents Composer/MySQL/Postgres/Redis/etc. from even being attempted, contrary to the setup contract in DESIGN.md that default resource installation should install what it can and then report failures. Accumulate per-resource failures and continue through the remaining defaults before failing the job.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest diagnostics commit. The earlier Gateway fallback binding thread remains open because this delta did not change
crates/daemon/src/gateway_config.rs.
Reviewed changes — I reviewed the commit since the prior Pullfrog review, focused on Gateway readiness failure diagnostics.
- Logged Gateway readiness diagnostics —
crates/daemon/src/gateway.rsnow records a best-effort structured diagnostic event when a newly started runtime fails readiness, including the readiness error, child exit state, and loopback listener port snapshot. - Added runtime diagnostic log fields —
crates/daemon/src/structured_log.rsnow appendsruntime_readiness_diagnosticsevents to the daemon log for post-failure RC evidence. - Promoted
platformfor daemon diagnostics —crates/daemon/Cargo.tomlnow depends onplatformoutside dev-dependencies so daemon runtime code can inspect loopback listeners.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01e527654d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match run_system_output("/sbin/pfctl", &["-s", "nat"]) { | ||
| Ok(rules) => Ok(rules), | ||
| Err(non_sudo_error) => { | ||
| match run_system_output("/usr/bin/sudo", &["-n", "/sbin/pfctl", "-s", "nat"]) { |
There was a problem hiding this comment.
When /sbin/pfctl -s nat needs privileges and the user has no cached sudo timestamp, this fallback runs sudo -n, which I checked with sudo -h is “non-interactive mode, no prompts are used.” In the pv ports:install repair path where the system files are already current but active redirects are unloaded, this returns an error before the later install_pf_redirects call that could have prompted and reloaded pf, so interactive repairs fail unnecessarily; use a prompting sudo path for the mutating repair or defer privileged inspection until after authentication.
Useful? React with 👍 / 👎.
| message: heartbeat_message, | ||
| }, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
If a foreground client keeps the daemon socket open but stops reading (for example its stdout pipe blocks), this .await can park on a full socket buffer. Because it runs inside the selected heartbeat branch, the completion future is no longer polled, so a reconciliation/update job can hang before persisting its final status; make the heartbeat best-effort with a short timeout or drop streaming on backpressure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ℹ️ No new issues found in
91b1b2c. The earlier Pullfrog CA trust repair and empty Gateway fallback binding threads remain open because this delta did not address them.
Reviewed changes — I reviewed the single commit since the prior Pullfrog review, focused on Gateway readiness when PV-managed pf redirects are already active.
- Probed Gateway readiness through public loopback ports —
crates/daemon/src/gateway.rsnow selects80/443for Gateway readiness when active PV redirects match the stored Gateway high-port plan, while falling back to the stored high ports when redirects are absent or unreadable. - Covered public-port readiness construction — the Gateway unit tests now assert both
GatewayHttpsand empty-Gateway TCP readiness use the public ports when the selected port set represents active redirects.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91b1b2cf83
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - name: Build app binary | ||
| env: | ||
| PV_DEFAULT_APP_UPDATE_MANIFEST_URL: ${{ vars.R2_PUBLIC_BASE_URL }}/pv-app-manifest.json | ||
| PV_DEFAULT_ARTIFACT_MANIFEST_URL: ${{ vars.R2_PUBLIC_BASE_URL }}/manifest.json |
There was a problem hiding this comment.
This release build still publishes a darwin-amd64 PV binary from the matrix above, but this new compiled manifest URL can point at the artifact manifest produced by the current publication default, which only requires darwin-arm64 artifacts. On an Intel Mac, pv setup resolves TargetPlatform::DarwinAmd64 and validates every default resource with select_latest(..., target_platform), so a legally-published arm64-only RC manifest makes the shipped amd64 app fail before setup can proceed. Either do not publish/bake this manifest into the amd64 app for the arm64-only RC, or require matching amd64 resource artifacts before releasing the amd64 binary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — I reviewed the single commit since the prior Pullfrog review, focused on avoiding the privileged macOS RC restart race before update validation.
- Moved restart after initial serving validation —
.github/workflows/privileged-macos-rc.ymlnow proves the project serves over HTTP and HTTPS beforepv daemon:restart, then repeats status and serving checks after the restart. - Waited for restart reconciliation to go idle — the RC workflow now polls
pv jobs --jsonuntil no jobs are running beforepv update --check --json, andcrates/pv-release/tests/workflow_defaults.rspins that ordering.
GPT ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — I reviewed the single commit since the prior Pullfrog review, focused on allowing the first app publication run when there is no current stable app manifest or installer yet.
- Allowed first app publication without current stable inputs —
.github/workflows/app-publication.ymlnow callsstage-app-publicationthroughrun_stage_app_publication()and avoids expandingcurrent_stable_argswhen the optional current-stable objects are absent. - Pinned the missing-current-stable workflow contract —
crates/pv-release/tests/workflow_defaults.rsnow asserts the wrapper and zero-argument branch so the publication workflow keeps working underset -u.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
pv/.github/workflows/real-artifact-e2e.yml
Lines 25 to 29 in bceed42
Pin actions in the real-artifact workflow
This new workflow runs several actions through mutable tags here and below (@v4, @stable, @v2) before executing the RC checks. If one of those tags is retargeted or compromised, a manually dispatched real-artifact run would execute unreviewed code in the repository context; this is especially risky for the optional privileged RC path. Pin these action references to full commit SHAs, as the adjacent app release/publication workflows do.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
ℹ️ No new issues found in the new commit. The earlier Gateway fallback listener feedback remains open and unchanged outside this delta.
Reviewed changes — I reviewed the single commit since the prior Pullfrog review, focused on closing the privileged macOS RC workflow race after pv link.
- Waited for link reconciliation before serving checks —
.github/workflows/privileged-macos-rc.ymlnow waits forpv jobs --jsonto show no running jobs afterpv linkbefore checking status and route serving. - Followed HTTP-to-HTTPS redirects in RC probes — the initial and post-restart HTTP checks now pass
--locationand the PV CA so the workflow verifies the HTTPS-backed route reached fromhttp://pv-rc-project.test/. - Covered the workflow contract in release tests —
crates/pv-release/tests/workflow_defaults.rsnow asserts the link-wait ordering and redirect-following HTTP probes.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d96e0bceb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manifest.resolve_track(&resource_name, track_selector)? | ||
| }; | ||
|
|
||
| manifest.select_latest(&resource_name, track, target_platform)?; |
There was a problem hiding this comment.
When the refreshed manifest lacks an artifact for any single setup default on the current platform, this ? aborts pv setup before DNS/ports/CA/daemon registration and before recording the other default resources. For example, a missing Redis/RustFS platform artifact prevents PHP/FrankenPHP and the Gateway from being set up at all, even though setup is meant to install what it can and report default-resource failures afterward. Accumulate this per-resource failure and continue planning the remaining defaults instead of failing the whole setup preflight.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edccaf869e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - Update check and diagnostics executed | ||
| CHECKLIST | ||
|
|
||
| record_status setup required pv setup --yes --no-path |
There was a problem hiding this comment.
When this workflow is run with artifact_manifest_url pointing at a candidate resource manifest (as real-artifact-e2e.yml does via inputs.manifest_url), this pv setup still resolves resources from the installed binary's compiled default; crates/cli/src/environment.rs falls back to resources::default_artifact_manifest_url() and its test asserts runtime env vars are ignored. The script only writes RESOLVED_ARTIFACT_MANIFEST_URL to the evidence summary, so the privileged RC can pass while never downloading or validating the candidate Managed Resource artifacts. Use an installer/binary built with that manifest, or provide a supported override for this setup/reconciliation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
This PR introduces a release-candidate validation gap and an installer download hardening issue in the new privileged macOS RC path.
Reviewed changes — I reviewed the four commits since the prior Pullfrog review, focused on the switch from a local source-built RC binary to the candidate installer and the related workflow/test refactors.
- Installed PV from the candidate installer —
.github/workflows/privileged-macos-rc.ymlnow resolves aninstaller_url, runsscripts/ci/privileged-macos-rc.sh, and executes the downloaded installer before the privileged setup/link/restart/uninstall evidence flow. - Passed installer inputs through real-artifact validation —
.github/workflows/real-artifact-e2e.ymlnow acceptsinstaller_urland forwards it to the reusable privileged macOS RC workflow after the real-artifact jobs. - Added interactive CA trust mode —
platform::PrivilegeModelets normalpv ca:*and setup paths use interactive sudo-backedsecuritycommands whilepv setup --non-interactivekeepssudo -nbehavior. - Moved privileged RC assertions into checked-in script coverage —
crates/pv-release/tests/workflow_defaults.rsnow readsscripts/ci/privileged-macos-rc.shdirectly and checks pinned workflow action references.
GPT ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/ci/privileged-macos-rc.sh (1)
102-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInstaller download can follow HTTP redirects despite HTTPS initial URL.
The
curl --locationcall follows redirects but does not constrain redirect protocols. A malicious redirect could downgrade to HTTP before the installer script executes on a privileged runner.Suggested fix
-record_status download-installer required curl --fail --show-error --silent --location --retry 3 --retry-delay 2 "$RESOLVED_INSTALLER_URL" -o "$PV_RC_INSTALLER" +record_status download-installer required curl --fail --show-error --silent --location --proto '=https' --proto-redir '=https' --retry 3 --retry-delay 2 "$RESOLVED_INSTALLER_URL" -o "$PV_RC_INSTALLER"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/ci/privileged-macos-rc.sh` at line 102, The curl command in the record_status download-installer line uses the --location flag to follow redirects but does not restrict which protocols redirects can use, potentially allowing a downgrade from HTTPS to HTTP. Add the --proto-redir flag set to =https in the curl command to ensure that any HTTP redirects encountered during the download are restricted to HTTPS protocol only, preventing potential man-in-the-middle attacks on the privileged runner.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/ci/privileged-macos-rc.sh`:
- Line 106: The preflight check on the line with record_status and sudo -n true
uses the || operator to call record_blocked when sudo fails, but this does not
actually prevent script continuation since record_blocked succeeds, causing the
entire line to exit successfully. After calling record_blocked to report the
sudo-required failure, the script needs to explicitly exit or return to prevent
subsequent privileged commands from executing and producing noisy output from
unavoidable failures.
---
Duplicate comments:
In `@scripts/ci/privileged-macos-rc.sh`:
- Line 102: The curl command in the record_status download-installer line uses
the --location flag to follow redirects but does not restrict which protocols
redirects can use, potentially allowing a downgrade from HTTPS to HTTP. Add the
--proto-redir flag set to =https in the curl command to ensure that any HTTP
redirects encountered during the download are restricted to HTTPS protocol only,
preventing potential man-in-the-middle attacks on the privileged runner.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8787759f-9c7c-42f0-8ae7-8f313a1cbee9
⛔ Files ignored due to path filters (6)
Cargo.lockis excluded by!**/*.lockcrates/cli/tests/snapshots/ports__ports_install_skips_active_rule_inspection_before_first_install.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/ports__ports_install_writes_prepared_and_system_pf_artifacts.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_config__gateway_config_renderer_outputs_empty_gateway_listener.snapis excluded by!**/*.snapcrates/platform/tests/snapshots/resolver_config__pf_conf_reference_inspection_reports_missing_current_stale_conflict_and_unreadable.snapis excluded by!**/*.snapcrates/platform/tests/snapshots/resolver_config__pf_config_renders_pv_owned_anchor_and_pf_conf_reference.snapis excluded by!**/*.snap
📒 Files selected for processing (31)
.github/workflows/app-publication.yml.github/workflows/privileged-macos-rc.yml.github/workflows/real-artifact-e2e.ymlCargo.tomlDESIGN.mdcrates/cli/src/commands/ca.rscrates/cli/src/commands/ports.rscrates/cli/src/commands/setup.rscrates/cli/src/environment.rscrates/cli/tests/ca.rscrates/cli/tests/ports.rscrates/cli/tests/postgres.rscrates/cli/tests/setup.rscrates/daemon/Cargo.tomlcrates/daemon/src/gateway.rscrates/daemon/src/gateway_config.rscrates/daemon/src/jobs.rscrates/daemon/src/structured_log.rscrates/daemon/src/supervisor.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/real_artifact_resource_matrix.rscrates/daemon/tests/supervisor_foundation.rscrates/platform/Cargo.tomlcrates/platform/src/lib.rscrates/platform/src/pf.rscrates/platform/src/trust.rscrates/platform/tests/resolver_config.rscrates/pv-release/tests/release_docs.rscrates/pv-release/tests/workflow_defaults.rsdocs/user/README.mdscripts/ci/privileged-macos-rc.sh
✅ Files skipped from review due to trivial changes (1)
- docs/user/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- Cargo.toml
- crates/cli/src/commands/ports.rs
- crates/daemon/Cargo.toml
- crates/pv-release/tests/release_docs.rs
- crates/cli/tests/ports.rs
- crates/cli/src/commands/setup.rs
- crates/cli/tests/postgres.rs
- crates/pv-release/tests/workflow_defaults.rs
- .github/workflows/real-artifact-e2e.yml
- DESIGN.md
- crates/daemon/tests/gateway_reconciliation.rs
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — I reviewed the four commits since the prior Pullfrog review, focused on the release workflow platform gate, setup partial-default planning, privileged macOS RC validation, CA/pf privilege handling, Gateway readiness, and foreground job streams.
- Gated app release platforms —
.github/workflows/app-release.ymlnow acceptsapp_platforms, builds and stages only selected app platforms, and the generated installer tolerates an arm64-only preview lane. - Relaxed setup default planning failures —
pv setupnow records and reconciles valid default Managed Resource plans even when unrelated defaults fail manifest planning, then exits failure with the unresolved default errors. - Repaired privileged CA and
pfpaths —pv ca:trust, setup, and ports commands now pass explicit interactive or noninteractive privilege modes, remove denied/stale CA entries before re-trusting, and inspect activepfredirects through the appropriate privilege path. - Hardened Gateway and daemon streaming behavior — the empty Gateway fallback restores explicit loopback binding, foreign-listener probes are bounded, and long-running foreground update/reconcile jobs emit bounded heartbeat log events.
- Closed privileged RC validation gaps —
scripts/ci/privileged-macos-rc.shnow constrains installer redirects to HTTPS, checks the installed binary for the resolved manifest URLs before setup, and fails early when passwordless sudo is unavailable.
GPT ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — I reviewed the single commit since the prior Pullfrog review, focused on the setup default-track cleanup and Gateway timing regression coverage.
- Simplified setup default track cloning —
crates/cli/src/commands/setup.rsnow uses.cloned()when carrying the resolved PHP default track into FrankenPHP planning. - Relaxed the Gateway timing harness —
gateway_reconciliation_bounds_foreign_https_listener_probenow gives the outer test timeout 2 seconds while keeping the production readiness timeout path under test at 100ms.
GPT ｜ 𝕏
1 participant
Summary
Verification
cargo fmt --all --checkgit diff --check HEADactionlintcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo nextest run --workspace --all-features --lockedPV_E2E_REAL_ARTIFACTS=1 PV_E2E_ARTIFACT_MANIFEST_URL=https://artifacts-staging.pv.prvious.dev/manifest.json cargo nextest run -p daemon --locked --run-ignored ignored-only -E 'test(real_artifact)'\n\n## Notes\n-cargo nextest run --workspace --all-targets --all-features --lockedis not used because the existingconfig::bench/configharness=false target is incompatible with nextest--format terse; clippy still covers all targets.\n- The privileged macOS RC workflow is manual so the full system path can be attempted on GitHub-hosted macOS runners without blocking every PR.Summary by CodeRabbit
share/postgres.bki.rdr-anchorfor PV rules.