[codex] Add PHP track defaults#288
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR implements PHP track-level default configuration seeding under ChangesPHP Track Defaults Implementation
Agent Skills Docs and Ideas
Sequence Diagram(s)sequenceDiagram
participant CLI as PHP/Composer Shim
participant ensure as ensure_php_track_defaults
participant fs as ~/.pv/resources/php/<track>/
participant env_build as php_track_exec_environment
participant Gateway as Daemon Gateway
participant Worker as FrankenPHP Worker
Note over CLI,fs: Install/CLI Flow
CLI->>ensure: (paths, "8.4")
ensure->>fs: mkdir etc/, conf.d/
ensure->>fs: write php.ini if missing
ensure-->>CLI: PhpTrackDefaults
CLI->>env_build: (paths, "8.4")
env_build-->>CLI: [(PHPRC, etc_dir), (PHP_INI_SCAN_DIR, conf_dir)]
CLI->>CLI: exec php with env overlay
Note over Gateway,Worker: Gateway/Worker Flow
Gateway->>ensure: (paths, "8.4")
ensure-->>Gateway: PhpTrackDefaults (already seeded)
Gateway->>Gateway: strip parent PHPRC/PHP_INI_SCAN_DIR
Gateway->>Gateway: apply private [PHPRC, PHP_INI_SCAN_DIR, XDG]
Gateway->>Worker: spawn with private env
Worker->>Worker: read ini from PHPRC path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — This PR adds track-level PHP defaults and routes PHP execution paths through them while keeping fallback artifact config paths isolated from host PHP config.
- Add shared PHP defaults —
resources::php_defaultsseeds mutablephp.iniandconf.dpaths under~/.pv/resources/php/<track>/etcfor supported PHP tracks. - Use track defaults in CLI shims —
shim:phpand Composer-through-PHP execution now setPHPRCandPHP_INI_SCAN_DIRto the track-level defaults instead of artifact release directories. - Use track defaults for FrankenPHP workers — worker process specs and config validation receive the same PHP ini environment while Gateway remains PHP-neutral.
- Strip inherited PHP ini environment — daemon validation and supervised child startup remove parent
PHPRCandPHP_INI_SCAN_DIRbefore applying each process spec's private environment. - Move artifact fallback paths — the PHP recipe passes StaticPHP fallback ini paths under
/var/empty/com.prvious.pv/php, with smoke coverage rejecting/usr/local/etc/phpleaks. - Add coverage and documentation — integration tests, snapshots, release smoke tests, design docs, and release-agent skills were updated for the new PHP track defaults behavior.
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: 2
🧹 Nitpick comments (4)
crates/resources/src/command.rs (1)
435-435: 🧹 Nitpick | 🔵 TrivialUse a top-level import instead of the fully-qualified call.
Line 435 uses
crate::php_defaults::ensure_php_track_defaults(...)with a fully-qualified path. Per coding guidelines, prefer top-level imports over fully-qualified names. Add an import at the module level and use the simple function name instead.♻️ Proposed refactor
+use crate::php_defaults::ensure_php_track_defaults; ... - crate::php_defaults::ensure_php_track_defaults(&self.paths, install.php.track.as_str())?; + ensure_php_track_defaults(&self.paths, install.php.track.as_str())?;🤖 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 `@crates/resources/src/command.rs` at line 435, Replace the fully-qualified function call to crate::php_defaults::ensure_php_track_defaults with a top-level import. Add a use statement at the module level to import ensure_php_track_defaults from crate::php_defaults, then update line 435 to use only the function name ensure_php_track_defaults instead of the fully-qualified path.Source: Coding guidelines
crates/cli/tests/composer.rs (1)
77-77: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winApply the same top-level import style in composer tests.
Import
php_track_defaultsat the module level and reference it directly in changed helpers/assertions.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 500-500
🤖 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 `@crates/cli/tests/composer.rs` at line 77, The `php_track_defaults` function is being called using the fully qualified name `resources::php_track_defaults` instead of being imported at the module level. Add `php_track_defaults` to the top-level imports from the resources module at the beginning of the composer.rs file, then replace the fully qualified references to `resources::php_track_defaults` with just `php_track_defaults` in both locations where it appears (line 77 and line 500).Source: Coding guidelines
crates/cli/tests/php.rs (1)
79-79: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPrefer importing
resourcessymbols at module scope in tests, too.Use top-level imports for
php_track_defaults/PHP_TRACK_DEFAULT_INIrather than fully qualified calls in changed assertions/helpers.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 235-239
🤖 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 `@crates/cli/tests/php.rs` at line 79, The code uses fully qualified calls like resources::php_track_defaults instead of importing these symbols at the module scope. Add top-level imports for php_track_defaults and PHP_TRACK_DEFAULT_INI from the resources module at the beginning of the file in the use statements, then replace all fully qualified calls to resources::php_track_defaults and references to resources::PHP_TRACK_DEFAULT_INI with their direct names throughout the test file (including the instances around line 79 and lines 235-239).Source: Coding guidelines
crates/cli/src/commands/php.rs (1)
264-265: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse top-level imports instead of fully qualified
resources::...calls.Please import these symbols and call them directly for consistency with repository Rust style.
Suggested refactor
use camino::Utf8PathBuf; +use resources::{ensure_php_track_defaults, php_adapter, php_track_exec_environment}; ... - resources::ensure_php_track_defaults(&paths, &track)?; - env.extend(resources::php_track_exec_environment(&paths, &track)?); + ensure_php_track_defaults(&paths, &track)?; + env.extend(php_track_exec_environment(&paths, &track)?); ... - let adapter = resources::php_adapter()?; + let adapter = php_adapter()?;As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 321-323
🤖 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 `@crates/cli/src/commands/php.rs` around lines 264 - 265, Replace the fully qualified `resources::` calls with top-level imports to follow repository Rust style guidelines. Add imports at the top of the file for the functions `ensure_php_track_defaults` and `php_track_exec_environment` from the resources module, then update all occurrences throughout the file (including lines around 321-323 as noted) to call these functions directly without the `resources::` prefix instead of using the fully qualified names.Source: Coding guidelines
🤖 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 @.agents/skills/release-resource-upstream-version/SKILL.md:
- Around line 44-150: The skill currently collects user confirmation for all
inputs upfront (lines 44-54) but then dispatches both the build workflow (lines
145-150) and publication workflow autonomously without re-confirming. Add two
explicit re-confirmation sections: first, insert a "Confirm Build Dispatch"
section immediately after the "Commit And Push" section and before "Build
Artifacts" that lists the finalized inputs and asks the user to confirm before
dispatching the artifact-recipes.yml workflow. Second, insert a "Confirm
Publication Dispatch" section at the end of "Build Artifacts" section that
re-confirms with the user before dispatching the publication workflow,
emphasizing that published artifacts become available to all clients. Both
sections should present the inputs in a clear format and require explicit user
confirmation before proceeding with each autonomous workflow dispatch.
In @.agents/skills/revoke-resource-artifact/SKILL.md:
- Around line 149-173: The emergency manual publication guidance in the
"Publication Options" section lists a "safe order" of 5 sequential steps (lines
165–170) for mutating R2 directly, but these steps are presented as an ordered
batch without explicit re-confirmation gates between each mutation. This creates
ambiguity about whether an agent should execute all steps autonomously after
receiving initial approval on line 155, rather than requiring explicit per-step
human approval. Restructure the numbered list to add explicit re-confirmation
checkpoints before each R2 mutation step: before uploading the revocation JSON,
before generating and uploading the versioned manifest, and before updating the
stable manifest. Ensure each checkpoint requires user confirmation with a clear
prompt before proceeding to the next mutation operation.
---
Nitpick comments:
In `@crates/cli/src/commands/php.rs`:
- Around line 264-265: Replace the fully qualified `resources::` calls with
top-level imports to follow repository Rust style guidelines. Add imports at the
top of the file for the functions `ensure_php_track_defaults` and
`php_track_exec_environment` from the resources module, then update all
occurrences throughout the file (including lines around 321-323 as noted) to
call these functions directly without the `resources::` prefix instead of using
the fully qualified names.
In `@crates/cli/tests/composer.rs`:
- Line 77: The `php_track_defaults` function is being called using the fully
qualified name `resources::php_track_defaults` instead of being imported at the
module level. Add `php_track_defaults` to the top-level imports from the
resources module at the beginning of the composer.rs file, then replace the
fully qualified references to `resources::php_track_defaults` with just
`php_track_defaults` in both locations where it appears (line 77 and line 500).
In `@crates/cli/tests/php.rs`:
- Line 79: The code uses fully qualified calls like
resources::php_track_defaults instead of importing these symbols at the module
scope. Add top-level imports for php_track_defaults and PHP_TRACK_DEFAULT_INI
from the resources module at the beginning of the file in the use statements,
then replace all fully qualified calls to resources::php_track_defaults and
references to resources::PHP_TRACK_DEFAULT_INI with their direct names
throughout the test file (including the instances around line 79 and lines
235-239).
In `@crates/resources/src/command.rs`:
- Line 435: Replace the fully-qualified function call to
crate::php_defaults::ensure_php_track_defaults with a top-level import. Add a
use statement at the module level to import ensure_php_track_defaults from
crate::php_defaults, then update line 435 to use only the function name
ensure_php_track_defaults instead of the fully-qualified path.
🪄 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: 5da23d39-4c2a-41cb-848f-a4b38eef7a1e
⛔ Files ignored due to path filters (11)
crates/cli/tests/snapshots/composer__composer_shim_execs_installed_phar_through_php_shim.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/composer__composer_shim_forwards_help_and_version_flags.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/composer__composer_shim_uses_cached_manifest_default_without_network.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_shim_execs_global_default_track_outside_project.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_shim_execs_resolved_project_track.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_shim_forwards_help_and_version_flags.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_shim_uses_cached_manifest_default_without_network.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__frankenphp_command_and_process_specs_are_stable.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_composer_with_php_pair_seeds_track_defaults.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_install_php_pair_seeds_track_defaults.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/managed_resource_commands__managed_resource_commands_update_php_pairs_preserves_existing_php_ini.snapis excluded by!**/*.snap
📒 Files selected for processing (33)
.agents/skills/release-app-version/SKILL.md.agents/skills/release-app-version/agents/openai.yaml.agents/skills/release-resource-artifacts/SKILL.md.agents/skills/release-resource-artifacts/agents/openai.yaml.agents/skills/release-resource-upstream-version/SKILL.md.agents/skills/release-resource-upstream-version/agents/openai.yaml.agents/skills/revoke-resource-artifact/SKILL.md.agents/skills/revoke-resource-artifact/agents/openai.yaml.agents/skills/update-resource-checksums/SKILL.md.agents/skills/update-resource-checksums/agents/openai.yamlCONTEXT.mdDESIGN.mdIDEAS.mdcrates/cli/src/commands/php.rscrates/cli/tests/composer.rscrates/cli/tests/php.rscrates/daemon/src/gateway.rscrates/daemon/src/supervisor.rscrates/daemon/tests/gateway_config.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/real_artifact_gateway_e2e.rscrates/daemon/tests/supervisor_foundation.rscrates/pv-release/tests/smoke.rscrates/resources/src/command.rscrates/resources/src/lib.rscrates/resources/src/php-defaults.inicrates/resources/src/php_defaults.rscrates/resources/tests/managed_resource_commands.rscrates/resources/tests/php_defaults.rsdocs/2026-06-20-php-track-defaults-design.mddocs/superpowers/plans/2026-06-21-php-track-defaults.mdrelease/artifacts/recipes/php/build.shrelease/artifacts/recipes/php/smoke.sh
![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: 84d0a22506
ℹ️ 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".
| log_errors = On | ||
| ignore_repeated_errors = Off | ||
| ignore_repeated_source = Off | ||
| variables_order = "GPCS" |
There was a problem hiding this comment.
When this file is seeded, the new shim and worker code force every PV PHP process to load it via PHPRC. With variables_order = "GPCS", PHP does not create $_ENV for inherited environment variables because the E component is omitted (see https://www.php.net/manual/en/ini.core.php#ini.variables-order), so a normal shell invocation like FOO=bar pv php -r 'var_dump($_ENV["FOO"] ?? null);' regresses from PHP's built-in default whenever the user has not customized php.ini. Please keep E in this default or omit the directive so external env remains visible.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — This incremental review covered the release metadata bump after the prior Pullfrog review.
- Bumped PHP artifact build revision —
release/artifacts/recipes/php/tracks.tomlnow publishes the PHP and FrankenPHP artifact set aspv3. - Updated release fixture expectations —
crates/pv-releasefixture tests and generated manifest snapshots now expectpv3artifact identities, URLs, checksums, and sizes for all supported PHP tracks and Darwin platforms.
GPT ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — This incremental review covered the new release-safety confirmation gates added after the prior Pullfrog review.
- Added
artifact-recipes.ymldispatch confirmation —release-resource-upstream-versionnow requires a fresh user confirmation of finalized build inputs immediately before dispatching the build workflow. - Added publication dispatch confirmation —
release-resource-upstream-versionnow requires a separate confirmation of publication inputs and client-visible publication impact before dispatchingartifact-publication.yml. - Gated emergency revocation mutations —
revoke-resource-artifactnow separates manual R2 revocation publication into per-mutation approval checkpoints instead of allowing one approval to cover the full sequence.
GPT ｜ 𝕏
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
~/.pv/resources/php/<track>/etc.PHPRCandPHP_INI_SCAN_DIR.php_ini/envini discovery./usr/local/etc/phpand add smoke coverage.Validation
cargo nextest run -p resources -E 'test(php_track_defaults_) | test(managed_resource_commands_install_php_pair_seeds_track_defaults) | test(managed_resource_commands_update_php_pairs_preserves_existing_php_ini)'\n-cargo nextest run -p cli -E 'test(php_shim_sets_only_php_ini_env_overlay) | test(composer_shim_execs_installed_phar_through_php_shim) | test(composer_shim_sets_pv_owned_env_overlay)'\n-cargo nextest run -p daemon -E 'test(frankenphp_command_and_process_specs_are_stable) | test(frankenphp_config_validation_receives_xdg_environment) | test(worker_config_renderer_outputs_track_caddyfile) | test(gateway_config_validation_strips_parent_php_ini_env_when_private_env_omits_it) | test(worker_config_validation_keeps_private_php_ini_env_after_parent_removal) | test(supervisor_start_strips_parent_php_ini_env_when_private_env_omits_it)'\n-cargo nextest run -p pv-release -E 'test(php_build_recipe_smoke)'\n-cargo fmt --all -- --check\n-git diff --check\n-sh -n release/artifacts/recipes/php/build.sh\n-sh -n release/artifacts/recipes/php/smoke.sh\n-shellcheck release/artifacts/recipes/php/build.sh release/artifacts/recipes/php/smoke.sh\n-cargo clippy --workspace --all-targets --all-features --locked -- -D warningsSummary by CodeRabbit
Release Notes
New Features
PHPRC/PHP_INI_SCAN_DIR) for consistentphp.inibehavior.Bug Fixes
/usr/local/etc/phpini fallback by enforcing deterministic safe ini paths during PHP/FrankenPHP operations.Documentation
Tests