[codex] Add project-level PHP extension opt-ins#289
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:
📝 WalkthroughWalkthroughThis PR implements end-to-end PHP extension opt-ins. Project PHP config now supports ChangesPHP Extension Opt-Ins
Sequence Diagram(s)sequenceDiagram
participant User as User (pv.yml)
participant Config as config::PhpConfig
participant State as state::Database
participant DaemonEnv as daemon::project_env
participant Resources as resources::php_extensions
participant Gateway as daemon::gateway
participant CLIShim as CLI php/composer shim
User->>Config: php: {version: "8.4", extensions: [redis]}
Config->>Config: parse → PhpConfig
Config->>State: link_project via version_selector()
DaemonEnv->>Resources: resolve_php_extension_request(...)
Resources-->>DaemonEnv: PhpExtensionResolution
DaemonEnv->>State: replace_project_php_runtime(...)
State-->>DaemonEnv: ProjectRecord.php_runtime
Gateway->>DaemonEnv: resolve_project_php_runtime(...)
DaemonEnv-->>Gateway: ResolvedPhpRuntime
Gateway->>Resources: ensure_php_runtime_overlay(...)
Resources-->>Gateway: conf.d overlay path
Gateway->>Resources: php_runtime_environment(...)
Resources-->>Gateway: PHP_INI_SCAN_DIR overlay
CLIShim->>State: read project.php_runtime
CLIShim->>Resources: resolve_persisted_php_extension_modules(...)
CLIShim->>Resources: php_runtime_exec_environment(...)
Resources-->>CLIShim: exec env with overlay
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 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 |
![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: b8c733c3c1
ℹ️ 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".
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR wires the feature end to end, but the current state handling can silently omit requested extensions before reconciliation and can make rollback reads fail after extension-specific workers are persisted.
Reviewed changes — This review covered the initial implementation of project-level PHP extension opt-ins across config, state, daemon runtime planning, CLI shims, release metadata, recipes, docs, and tests.
- Parse and preserve
php.extensions—config::PhpConfigsupports scalar PHP selectors and object-form version-plus-extension requests, with writer coverage preserving extension lists when updating PHP tracks. - Persist project PHP runtime state —
stateadds requested, loaded, and ignored extension columns plus helpers for runtime keys and publicProjectRecord::php_runtimeaccess. - Resolve extensions during reconciliation — daemon project-env reconciliation resolves requested extensions from installed PHP artifact metadata, persists loaded/ignored state, and reports ignored-extension warnings.
- Group and launch extension-specific workers — gateway planning groups FrankenPHP workers by PHP track plus loaded extensions and applies PHP runtime overlays to worker environments.
- Apply overlays to CLI shims —
pv phpand Composer shims reconstruct persisted extension modules and extendPHP_INI_SCAN_DIRfor project-local execution. - Publish extension metadata — resource manifest parsing, release records, PHP recipes, smoke tests, docs, and snapshots now include optional PHP extension metadata.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pv-release/src/record.rs (1)
162-227: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
php_extensionsentries duringReleaseRecord::from_json.
php_extensionsis currently accepted verbatim. Invalidload_kind/path/duplicate names can pass record parsing and then propagate into manifest/runtime metadata.Suggested patch
impl ReleaseRecord { pub fn from_json(path: &Utf8Path, json: &str) -> crate::Result<Self> { let raw: RawReleaseRecord = serde_json::from_str(json).map_err(|error| invalid_release(path, error.to_string()))?; @@ validate_relative_file_list(path, "license_files", &raw.license_files)?; validate_relative_file_list(path, "notice_files", &raw.notice_files)?; validate_relative_path(path, "object_key", &raw.object_key)?; + validate_php_extensions(path, &raw.php_extensions)?; validate_object_key_layout(path, &raw)?; raw.provenance.validate(path)?; @@ } } + +fn validate_php_extensions(path: &Utf8Path, php_extensions: &[PhpExtensionRecord]) -> crate::Result<()> { + let mut seen = BTreeSet::new(); + for extension in php_extensions { + let valid_name = !extension.name.is_empty() + && extension + .name + .bytes() + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_'); + if !valid_name { + return Err(invalid_release( + path, + format!("invalid php_extensions name `{}`", extension.name), + )); + } + if !matches!(extension.load_kind.as_str(), "extension" | "zend_extension") { + return Err(invalid_release( + path, + format!("invalid php_extensions load_kind `{}`", extension.load_kind), + )); + } + validate_relative_path(path, "php_extensions.path", &extension.path)?; + if !seen.insert(extension.name.as_str()) { + return Err(invalid_release( + path, + format!("duplicate php_extensions entry `{}`", extension.name), + )); + } + } + Ok(()) +}🤖 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/pv-release/src/record.rs` around lines 162 - 227, The php_extensions field in the ReleaseRecord::from_json method is not being validated and is assigned directly from the raw data without checking for invalid load_kind values, invalid paths, or duplicate extension names. Add validation logic for php_extensions before the final Ok(Self { ... }) return statement in the from_json method, similar to the existing validate_relative_file_list and validate_relative_path calls, to ensure each php extension entry has a valid load_kind, valid path, and that there are no duplicate extension names. This validation should return an appropriate error using the invalid_release helper function if any validation fails.
🧹 Nitpick comments (4)
crates/config/tests/project_config.rs (1)
127-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new unknown
phpkey branch.The parser now has a dedicated
UnknownPhpKeypath; add one assertion here so typos underphp:stay covered.Suggested test addition
assert!(matches!( ProjectConfig::parse("php:\n extensions:\n - true\n"), Err(ConfigError::InvalidFieldType { field, .. }) if field == "php.extensions" )); + assert!(matches!( + ProjectConfig::parse("php:\n unknown: redis\n"), + Err(ConfigError::UnknownPhpKey { key }) if key == "unknown" + )); Ok(()) }🤖 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/config/tests/project_config.rs` around lines 127 - 143, Add an assertion within the project_config_rejects_invalid_php_extensions_shape test function to cover the UnknownPhpKey error path. Insert a new assert statement that tests a scenario with an unknown or misspelled key under the php configuration section (for example, a typo like "extensios" instead of "extensions"), and verify it matches the ConfigError variant that handles unknown php keys. Follow the same assertion pattern as the existing test cases in the function.crates/daemon/tests/gateway_reconciliation.rs (1)
1697-1706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
php_trackfromruntime_keyparameter instead of hardcoding.The helper hardcodes
php_trackto"8.4"while acceptingruntime_keyas a parameter. This creates a mismatch between the function signature (which suggests flexibility) and implementation (which assumes the track is always8.4). If this helper is called with a different track (e.g.,"8.3") or a runtime key with extensions (e.g.,"8.4+redis"), thephp_trackfield would be incorrect.♻️ Extract php_track from runtime_key
fn php_worker_plan(runtime_key: &str) -> daemon::gateway::PhpWorkerRuntimePlan { + let php_track = if let Some((track, _)) = runtime_key.split_once('+') { + track + } else { + runtime_key + }; daemon::gateway::PhpWorkerRuntimePlan { - php_track: "8.4".to_owned(), + php_track: php_track.to_owned(), runtime_key: runtime_key.to_owned(), loaded_modules: Vec::new(), port: RUNTIME_PORT_FALLBACK_START, projects: Vec::new(), } }🤖 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/daemon/tests/gateway_reconciliation.rs` around lines 1697 - 1706, In the php_worker_plan function, the php_track field is hardcoded to "8.4" instead of being derived from the runtime_key parameter. Extract the PHP version from the runtime_key string (which may contain extensions like "+redis" that need to be stripped) and use that extracted version to set the php_track field instead of the hardcoded value. This ensures the function correctly handles different PHP versions passed via runtime_key and creates a proper relationship between the parameter and the resulting field value.crates/pv-release/tests/recipe_metadata.rs (1)
858-863: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse top-level imports for helper type names.
Line 858 uses fully-qualified type paths in the signature; prefer imported aliases for consistency with Rust style in this module.
♻️ Proposed refactor
-use camino_tempfile::tempdir; +use camino_tempfile::{tempdir, Utf8TempDir}; +use camino::Utf8PathBuf; -fn write_php_recipe(tempdir: &camino_tempfile::Utf8TempDir) -> Result<camino::Utf8PathBuf> { +fn write_php_recipe(tempdir: &Utf8TempDir) -> Result<Utf8PathBuf> {As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
🤖 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/pv-release/tests/recipe_metadata.rs` around lines 858 - 863, The function write_php_recipe uses fully-qualified type paths in its signature (camino_tempfile::Utf8TempDir and camino::Utf8PathBuf) instead of imported aliases. Add top-level imports for Utf8TempDir from camino_tempfile and Utf8PathBuf from camino at the beginning of the file, then update the function signature to use these imported type names directly instead of their fully-qualified paths for consistency with Rust style conventions.Source: Coding guidelines
crates/pv-release/src/recipe.rs (1)
612-614: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winApply the same list invariants to
php.default_extensions(and prevent overlap with optional extensions).
default_extensionscurrently skips duplicate/shape validation, and overlap withoptional_extensionsis not rejected. Both can leak ambiguous values intoPV_BUILD_EXTENSIONS.Suggested patch
impl PhpSettings { fn from_raw(path: &Utf8Path, raw: RawPhpSettings) -> crate::Result<Self> { validate_deployment_target(path, &raw.deployment_target)?; validate_expected_extensions(path, &raw.expected_extensions)?; + validate_extension_list(path, "php.default_extensions", &raw.default_extensions)?; validate_build_extensions(path, &raw.default_extensions, &raw.expected_extensions)?; validate_extension_list(path, "php.optional_extensions", &raw.optional_extensions)?; + let default_extensions = raw + .default_extensions + .iter() + .map(String::as_str) + .collect::<BTreeSet<_>>(); + if let Some(extension) = raw + .optional_extensions + .iter() + .find(|extension| default_extensions.contains(extension.as_str())) + { + return Err(invalid( + path, + format!( + "php.optional_extensions must not contain default extension `{extension}`" + ), + )); + } Ok(Self { deployment_target: raw.deployment_target, default_extensions: raw.default_extensions, optional_extensions: raw.optional_extensions, expected_extensions: raw.expected_extensions, }) } }🤖 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/pv-release/src/recipe.rs` around lines 612 - 614, The `default_extensions` field is missing the same list invariant validations (duplicate/shape checks) that are applied to `optional_extensions` through the `validate_extension_list` call. Add a `validate_extension_list` call for `default_extensions` similar to the one for `optional_extensions`, and add validation logic to prevent overlaps between `default_extensions` and `optional_extensions` to ensure no ambiguous values leak into PV_BUILD_EXTENSIONS. The validation should occur after the existing `validate_build_extensions` call.
🤖 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 `@crates/resources/src/manifest.rs`:
- Around line 592-605: The validate_manifest_php_extension_path function
currently rejects absolute paths and parent directory references but allows
empty strings and current directory paths (.) which can create invalid extension
directives later. Add additional validation checks after the existing
path.is_absolute() and component checks to reject empty paths (check if
path_str.is_empty() or use equivalent) and current directory references (check
if path_str equals "."), returning an appropriate ResourcesError with a
descriptive reason for each rejection case.
In `@crates/resources/src/php_extensions.rs`:
- Around line 146-164: In the php_runtime_environment function, the code
currently only appends the overlay directory to PHP_INI_SCAN_DIR if that key
already exists in the environment dictionary returned from
php_track_environment. If PHP_INI_SCAN_DIR is missing, the overlay is silently
ignored and modules will not load. Instead of using the conditional if let Some
pattern with get_mut on the environment variable, ensure PHP_INI_SCAN_DIR always
receives the overlay path by either inserting it into the environment dictionary
if it doesn't exist, or returning an error to surface the misconfiguration. This
way the overlay created by ensure_php_runtime_overlay is always properly applied
when modules are present.
---
Outside diff comments:
In `@crates/pv-release/src/record.rs`:
- Around line 162-227: The php_extensions field in the ReleaseRecord::from_json
method is not being validated and is assigned directly from the raw data without
checking for invalid load_kind values, invalid paths, or duplicate extension
names. Add validation logic for php_extensions before the final Ok(Self { ... })
return statement in the from_json method, similar to the existing
validate_relative_file_list and validate_relative_path calls, to ensure each php
extension entry has a valid load_kind, valid path, and that there are no
duplicate extension names. This validation should return an appropriate error
using the invalid_release helper function if any validation fails.
---
Nitpick comments:
In `@crates/config/tests/project_config.rs`:
- Around line 127-143: Add an assertion within the
project_config_rejects_invalid_php_extensions_shape test function to cover the
UnknownPhpKey error path. Insert a new assert statement that tests a scenario
with an unknown or misspelled key under the php configuration section (for
example, a typo like "extensios" instead of "extensions"), and verify it matches
the ConfigError variant that handles unknown php keys. Follow the same assertion
pattern as the existing test cases in the function.
In `@crates/daemon/tests/gateway_reconciliation.rs`:
- Around line 1697-1706: In the php_worker_plan function, the php_track field is
hardcoded to "8.4" instead of being derived from the runtime_key parameter.
Extract the PHP version from the runtime_key string (which may contain
extensions like "+redis" that need to be stripped) and use that extracted
version to set the php_track field instead of the hardcoded value. This ensures
the function correctly handles different PHP versions passed via runtime_key and
creates a proper relationship between the parameter and the resulting field
value.
In `@crates/pv-release/src/recipe.rs`:
- Around line 612-614: The `default_extensions` field is missing the same list
invariant validations (duplicate/shape checks) that are applied to
`optional_extensions` through the `validate_extension_list` call. Add a
`validate_extension_list` call for `default_extensions` similar to the one for
`optional_extensions`, and add validation logic to prevent overlaps between
`default_extensions` and `optional_extensions` to ensure no ambiguous values
leak into PV_BUILD_EXTENSIONS. The validation should occur after the existing
`validate_build_extensions` call.
In `@crates/pv-release/tests/recipe_metadata.rs`:
- Around line 858-863: The function write_php_recipe uses fully-qualified type
paths in its signature (camino_tempfile::Utf8TempDir and camino::Utf8PathBuf)
instead of imported aliases. Add top-level imports for Utf8TempDir from
camino_tempfile and Utf8PathBuf from camino at the beginning of the file, then
update the function signature to use these imported type names directly instead
of their fully-qualified paths for consistency with Rust style conventions.
🪄 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: 9f870450-f452-4a88-90de-c94f27e586ae
⛔ Files ignored due to path filters (34)
crates/cli/tests/snapshots/doctor__doctor_fails_when_active_pf_redirects_are_missing.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_fails_when_daemon_socket_is_stale.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_fails_when_system_ca_trust_is_missing.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_fails_when_system_resolver_is_missing.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_fails_with_repair_commands.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_passes_when_required_checks_pass.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/doctor__doctor_warnings_do_not_fail.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_use_latest_preserves_alias_in_config_and_records_resolved_track.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/php__php_use_updates_project_config_state_and_reports_missing_daemon.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/status__status_prefers_ignored_php_extension_over_other_project_env_warnings.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/status__status_reports_warning_project_env_as_success.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_parses_strict_resource_env_shape.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_creates_preferred_file_when_missing.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_preserves_existing_config_file_mode.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_preserves_php_extensions_when_updating_track.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_updates_php_in_alternate_file.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_updates_php_in_discovered_file.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_updates_symlinked_config_target.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__runtime_plan_defaults_document_root_to_project_root_without_public_directory.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__runtime_plan_defaults_document_root_to_public_directory_without_config.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__runtime_plan_groups_linked_projects_by_php_track.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__runtime_plan_resolves_latest_php_track_from_cached_manifest.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/gateway_reconciliation__runtime_plan_uses_project_root_not_original_or_config_path.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/recipe_metadata__print_recipe_env_frankenphp.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/recipe_metadata__print_recipe_env_php.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/record_writer__release_record_writer_serializes_php_extension_metadata.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/release_records__release_records_parse_identity_and_required_fields.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/release_records__release_records_parse_php_extension_metadata.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/artifact_cache_download__manifest_cache_fetches_latest_and_falls_back_to_cached_manifest-2.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/manifest_foundation__manifest_parses_registry_backed_resources_tracks_and_artifacts.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__database_runs_migrations_and_exposes_core_schema.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__linked_projects_preserve_ids_and_refresh_hostnames.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__migrated_project_resource_state_round_trips_through_public_apis.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_ignored_php_extensions.snapis excluded by!**/*.snap
📒 Files selected for processing (51)
.superpowers/sdd/task-2-report.mdDESIGN.mdcrates/cli/src/commands/php.rscrates/cli/src/commands/project.rscrates/cli/src/commands/status.rscrates/cli/tests/composer.rscrates/cli/tests/php.rscrates/cli/tests/project_env.rscrates/cli/tests/status.rscrates/config/src/error.rscrates/config/src/lib.rscrates/config/src/model.rscrates/config/src/parser.rscrates/config/src/writer.rscrates/config/tests/project_config.rscrates/daemon/src/gateway.rscrates/daemon/src/project_env.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/project_env_reconciliation.rscrates/daemon/tests/real_artifact_gateway_e2e.rscrates/pv-release/src/cli.rscrates/pv-release/src/manifest.rscrates/pv-release/src/recipe.rscrates/pv-release/src/record.rscrates/pv-release/src/record_writer.rscrates/pv-release/tests/manifest_generation.rscrates/pv-release/tests/recipe_metadata.rscrates/pv-release/tests/record_writer.rscrates/pv-release/tests/release_records.rscrates/pv-release/tests/smoke.rscrates/resources/src/lib.rscrates/resources/src/manifest.rscrates/resources/src/php_extensions.rscrates/resources/tests/manifest_foundation.rscrates/resources/tests/php_extensions.rscrates/state/src/database.rscrates/state/src/lib.rscrates/state/src/migrations.rscrates/state/src/paths.rscrates/state/src/sql/008_project_php_runtime_extensions.sqlcrates/state/tests/state_foundation.rsdocs/adr/0006-fixed-extension-php-artifacts.mddocs/adr/0014-project-level-php-extension-opt-ins.mddocs/superpowers/plans/2026-06-22-php-extension-opt-ins.mddocs/superpowers/specs/2026-06-22-php-extension-opt-ins-design.mddocs/user/README.mdit/cli.rsrelease/artifacts/README.mdrelease/artifacts/recipes/php/build.shrelease/artifacts/recipes/php/smoke.shrelease/artifacts/recipes/php/tracks.toml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3894539c07
ℹ️ 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.
Important
The new shim fallback fixes fresh-link extension loading, but it also keeps stale extensions active after they are removed from project config.
Reviewed changes — This incremental review covered the new 3894539 fixes on top of the prior Pullfrog review.
- Resolved fresh-link shim fallback —
pv phpand Composer shims now resolve extension requests from project config and the installed PHP artifact when persisted runtime state is empty or stale. - Separated extension worker observed-state identity — extension-specific FrankenPHP workers now use a
PhpRuntimeWorkerobserved-state subject while legacy track-only workers keep the previousPhpWorkersubject. - Tightened PHP extension metadata validation — release records, manifests, recipe metadata, artifact metadata, and tests now reject invalid paths, duplicate names, invalid load kinds, and missing module files.
- Expanded diagnostics and regression coverage — ignored extension messages now preserve multiple warnings, extension-only configs honor the global PHP default, and related snapshots/tests were refreshed.
GPT ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/daemon/tests/daemon_foundation.rs (1)
946-951: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Noneblocker path can make this fallback test nondeterministic.When
bind_preferred_dns_port_pair()returnsNone(Line 946), this test no longer holds the preferred port itself. If the external holder releases the port before startup, the assertions at Lines 950-951 can fail intermittently.Suggested guard to avoid flaky assertions when precondition is not held locally
- let _preferred_dns_port_blockers = bind_preferred_dns_port_pair().await?; + let preferred_dns_port_blockers = bind_preferred_dns_port_pair().await?; let daemon = daemon::RunningDaemon::start(paths.clone()).await?; let port = dns_port(&paths)?; + if preferred_dns_port_blockers.is_none() && port == DNS_PREFERRED_PORT { + daemon.shutdown().await?; + return Ok(()); + } + assert_ne!(port, DNS_PREFERRED_PORT); assert!((RUNTIME_PORT_FALLBACK_START..=RUNTIME_PORT_FALLBACK_END).contains(&port));Also applies to: 1193-1207
🤖 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/daemon/tests/daemon_foundation.rs` around lines 946 - 951, The test relies on bind_preferred_dns_port_pair() to hold the preferred DNS port, but when this function returns None, the port is no longer protected locally, allowing external processes to claim it and making the subsequent assertions nondeterministic. Instead of silently accepting a None return value by assigning to _preferred_dns_port_blockers, add a guard that ensures bind_preferred_dns_port_pair() successfully returns a valid blocker (not None) before proceeding with the daemon startup and port assertions. Apply this same fix to all instances of this pattern, including the related test cases around lines 1193-1207 that have the same vulnerability.crates/state/src/database.rs (1)
2067-2069: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale error wording to
php runtime key.Line 2068 still says “php track,” but this branch now validates/stores
php_runtime_key. Keeping the message aligned avoids misleading diagnostics.Suggested wording fix
- reason: "php worker ports must use owner id `php`, include a php track, and use an empty owner port", + reason: "php worker ports must use owner id `php`, include a php runtime key, and use an empty owner port",🤖 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/state/src/database.rs` around lines 2067 - 2069, The error message in the reason field of the validation check for php worker ports is outdated and references "php track," but the validation logic now uses php_runtime_key instead. Update the reason string to replace the mention of "php track" with "php runtime key" to keep the error message consistent with the actual validation being performed and avoid misleading diagnostics.
🧹 Nitpick comments (3)
crates/daemon/tests/project_env_reconciliation.rs (1)
158-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a top-level import for
write_sensitive_fileinstead of fully qualified calls.Using
state::fs::write_sensitive_file(...)in these new lines goes against the project Rust import preference. Import it once at module scope and call it directly.Suggested refactor
- state::fs::write_sensitive_file(&release.join("lib/php/extensions/redis.so"), "")?; + write_sensitive_file(&release.join("lib/php/extensions/redis.so"), "")?; ... - state::fs::write_sensitive_file(&release.join("lib/php/extensions/redis.so"), "")?; + write_sensitive_file(&release.join("lib/php/extensions/redis.so"), "")?;As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
Also applies to: 203-203
🤖 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/daemon/tests/project_env_reconciliation.rs` at line 158, Add a module-level import statement for write_sensitive_file from state::fs at the top of the project_env_reconciliation.rs file with other imports. Then replace all instances of the fully qualified call state::fs::write_sensitive_file(...) with just write_sensitive_file(...) throughout the file, including the calls at lines 158 and 203 as mentioned in the comment.Source: Coding guidelines
crates/pv-release/tests/recipe_metadata.rs (1)
474-479: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse single-replacement fixture mutation for strict-case isolation.
Using
String::replacehere can mutate multiple occurrences and blur which rule each case is exercising. Preferreplacen(..., 1)per variant to keep each invalid input single-cause.Also applies to: 504-515
🤖 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/pv-release/tests/recipe_metadata.rs` around lines 474 - 479, The test fixtures use String::replace which can mutate multiple occurrences of a pattern, making it unclear which specific rule each test case exercises. Replace all occurrences of the replace method calls (in duplicate_default_extension, invalid_default_extension, overlapping_optional_extension and the similar mutations mentioned at lines 504-515) with replacen(..., 1) to ensure each test case mutates only the first occurrence of the pattern, maintaining strict single-cause isolation for each test variant.crates/resources/tests/php_extensions.rs (1)
114-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the duplicate-name case deterministic.
The
duplicate-namecase can fail early as “missing module,” so it doesn’t reliably validate duplicate-name rejection. Isolate fixtures per case (or seed required.sofiles only for that case) so each case asserts its intended failure mode.Suggested test-shape adjustment
- let artifact = tempdir.path().join("php"); @@ - for (name, metadata) in cases { + for (name, metadata) in cases { + let artifact = tempdir.path().join(format!("php-{name}")); fs::write_sensitive_file(&artifact.join("share/pv/php-extensions.json"), metadata)?; + if name == "duplicate-name" { + fs::write_sensitive_file(&artifact.join("lib/php/extensions/redis.so"), "")?; + fs::write_sensitive_file(&artifact.join("lib/php/extensions/redis-copy.so"), "")?; + } let result = resolve_php_extension_request(&artifact, &["redis".into()]);🤖 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/tests/php_extensions.rs` around lines 114 - 148, The test function php_extension_metadata_rejects_invalid_paths_duplicates_and_missing_modules runs all test cases with the same fixture without creating the actual .so files, causing the duplicate-name case to fail with a missing-module error instead of validating duplicate-name rejection. Restructure the test to create the appropriate fixture setup for each case in the loop, ensuring that the duplicate-name case has the required .so files actually created in the artifact directory so it properly validates duplicate-name rejection, while the missing-module case can continue without those files.
🤖 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.
Outside diff comments:
In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 946-951: The test relies on bind_preferred_dns_port_pair() to hold
the preferred DNS port, but when this function returns None, the port is no
longer protected locally, allowing external processes to claim it and making the
subsequent assertions nondeterministic. Instead of silently accepting a None
return value by assigning to _preferred_dns_port_blockers, add a guard that
ensures bind_preferred_dns_port_pair() successfully returns a valid blocker (not
None) before proceeding with the daemon startup and port assertions. Apply this
same fix to all instances of this pattern, including the related test cases
around lines 1193-1207 that have the same vulnerability.
In `@crates/state/src/database.rs`:
- Around line 2067-2069: The error message in the reason field of the validation
check for php worker ports is outdated and references "php track," but the
validation logic now uses php_runtime_key instead. Update the reason string to
replace the mention of "php track" with "php runtime key" to keep the error
message consistent with the actual validation being performed and avoid
misleading diagnostics.
---
Nitpick comments:
In `@crates/daemon/tests/project_env_reconciliation.rs`:
- Line 158: Add a module-level import statement for write_sensitive_file from
state::fs at the top of the project_env_reconciliation.rs file with other
imports. Then replace all instances of the fully qualified call
state::fs::write_sensitive_file(...) with just write_sensitive_file(...)
throughout the file, including the calls at lines 158 and 203 as mentioned in
the comment.
In `@crates/pv-release/tests/recipe_metadata.rs`:
- Around line 474-479: The test fixtures use String::replace which can mutate
multiple occurrences of a pattern, making it unclear which specific rule each
test case exercises. Replace all occurrences of the replace method calls (in
duplicate_default_extension, invalid_default_extension,
overlapping_optional_extension and the similar mutations mentioned at lines
504-515) with replacen(..., 1) to ensure each test case mutates only the first
occurrence of the pattern, maintaining strict single-cause isolation for each
test variant.
In `@crates/resources/tests/php_extensions.rs`:
- Around line 114-148: The test function
php_extension_metadata_rejects_invalid_paths_duplicates_and_missing_modules runs
all test cases with the same fixture without creating the actual .so files,
causing the duplicate-name case to fail with a missing-module error instead of
validating duplicate-name rejection. Restructure the test to create the
appropriate fixture setup for each case in the loop, ensuring that the
duplicate-name case has the required .so files actually created in the artifact
directory so it properly validates duplicate-name rejection, while the
missing-module case can continue without those files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 60bee46b-2fff-41fc-b74c-c84286d85f3c
⛔ Files ignored due to path filters (7)
crates/cli/tests/snapshots/status__status_prefers_ignored_php_extension_over_other_project_env_warnings.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/recipe_metadata__recipe_metadata_rejects_strict_php_metadata.snapis excluded by!**/*.snapcrates/pv-release/tests/snapshots/release_records__release_records_reject_invalid_php_extension_metadata.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/manifest_foundation__manifest_rejects_invalid_php_extension_metadata.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__php_worker_port_allocator_persists_one_port_per_track.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__runtime_observed_state_round_trips_through_observed_states.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_ignored_php_extensions.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
crates/cli/src/commands/doctor.rscrates/cli/src/commands/logs.rscrates/cli/src/commands/php.rscrates/cli/src/commands/project.rscrates/cli/src/commands/status.rscrates/cli/tests/composer.rscrates/cli/tests/php.rscrates/cli/tests/status.rscrates/daemon/src/gateway.rscrates/daemon/src/project_env.rscrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/project_env_reconciliation.rscrates/pv-release/src/recipe.rscrates/pv-release/src/record.rscrates/pv-release/tests/recipe_metadata.rscrates/pv-release/tests/release_records.rscrates/resources/src/manifest.rscrates/resources/src/php_extensions.rscrates/resources/tests/manifest_foundation.rscrates/resources/tests/php_extensions.rscrates/state/src/database.rscrates/state/tests/state_foundation.rsit/cli.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/cli/tests/status.rs
- it/cli.rs
- crates/pv-release/src/recipe.rs
- crates/cli/tests/php.rs
- crates/daemon/tests/gateway_reconciliation.rs
- crates/cli/tests/composer.rs
- crates/state/tests/state_foundation.rs
- crates/pv-release/src/record.rs
- crates/cli/src/commands/php.rs
- crates/daemon/src/project_env.rs
- crates/resources/src/manifest.rs
- crates/daemon/src/gateway.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cc5698070
ℹ️ 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.
🧹 Nitpick comments (1)
crates/pv-release/tests/archive_validation.rs (1)
509-515: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the release-record fixture builder structural, not string-fragile.
replacenon formatted JSON can silently stop injectingphp_extensionsif whitespace/key formatting changes. Prefer constructing/modifying JSON viaserde_json::Valueso test intent stays stable across formatting refactors.Suggested refactor
fn release_record_json_with_php_extensions(sha256: &str, size: u64) -> String { - release_record_json(sha256, size).replacen( - " \"provenance\": {", - " \"php_extensions\": [\n {\n \"name\": \"redis\",\n \"load_kind\": \"extension\",\n \"path\": \"lib/php/extensions/redis.so\"\n }\n ],\n \"provenance\": {", - 1, - ) + let mut json: serde_json::Value = + serde_json::from_str(&release_record_json(sha256, size)).expect("valid fixture JSON"); + json["php_extensions"] = serde_json::json!([ + { + "name": "redis", + "load_kind": "extension", + "path": "lib/php/extensions/redis.so" + } + ]); + serde_json::to_string_pretty(&json).expect("serializable fixture JSON") }🤖 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/pv-release/tests/archive_validation.rs` around lines 509 - 515, The release_record_json_with_php_extensions function uses string replacen to inject php_extensions into JSON, which is fragile and breaks if whitespace or formatting changes. Instead, parse the result of release_record_json into a serde_json::Value object, structurally add the php_extensions field as an array with the redis extension object, and then serialize it back to a string. This makes the test robust to JSON formatting changes.
🤖 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.
Nitpick comments:
In `@crates/pv-release/tests/archive_validation.rs`:
- Around line 509-515: The release_record_json_with_php_extensions function uses
string replacen to inject php_extensions into JSON, which is fragile and breaks
if whitespace or formatting changes. Instead, parse the result of
release_record_json into a serde_json::Value object, structurally add the
php_extensions field as an array with the redis extension object, and then
serialize it back to a string. This makes the test robust to JSON formatting
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e23405b-193f-4160-8c6a-149eb2fb208a
⛔ Files ignored due to path filters (1)
crates/pv-release/tests/snapshots/archive_validation__archive_validation_rejects_advertised_php_extension_modules_that_are_not_files.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
crates/cli/src/commands/php.rscrates/cli/tests/composer.rscrates/cli/tests/php.rscrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/project_env_reconciliation.rscrates/pv-release/src/archive.rscrates/pv-release/src/record.rscrates/pv-release/tests/archive_validation.rscrates/pv-release/tests/recipe_metadata.rscrates/resources/src/php_extensions.rscrates/resources/tests/php_extensions.rscrates/state/src/database.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/cli/tests/composer.rs
- crates/daemon/tests/daemon_foundation.rs
- crates/pv-release/tests/recipe_metadata.rs
- crates/cli/src/commands/php.rs
- crates/resources/src/php_extensions.rs
- crates/daemon/tests/project_env_reconciliation.rs
- crates/pv-release/src/record.rs
- crates/state/src/database.rs
There was a problem hiding this comment.
Important
The latest shim fix still misses php.version: latest in one stale-runtime path, so the current config can execute the wrong PHP track until reconciliation runs.
Reviewed changes — This incremental review covered the new shim track-resolution fix added after the prior Pullfrog review.
- Resolved concrete config tracks before extension recomputation —
pv phpnow detects when persisted runtime state is on a different concrete track than the currentpv.ymland resolves requested extensions against the config-selected PHP track. - Covered stale persisted runtime switching —
crates/cli/tests/php.rsnow exercises recomputing extensions against a newer configured track when persisted runtime state still points at an older track.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cbb5fe0e2d
ℹ️ 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 latest commit; the prior
php.version: lateststale-track finding still appears unresolved.
Reviewed changes — This incremental review covered the new shim fallback change after the prior Pullfrog review at cbb5fe0.
- Resolved extension-only default-track refresh —
pv phpnow resolves the current global/default PHP track for extension-only project configs before reusing stale persisted extension runtime state. - Covered extension-only stale-runtime switching —
crates/cli/tests/php.rsnow exercises a project withphp.extensionsand no explicit version moving from stale persisted8.3+redisstate to the current global8.4+redisruntime.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d10838910
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44cc1f1e9f
ℹ️ 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.
Reviewed changes — This incremental review covered the new system reconciliation refresh added after the prior Pullfrog review at 7d10838.
- Refreshed PHP extension state after resource installs — System update and reconciliation now run project reconciliation, resource reconciliation, then project reconciliation again so projects that request PHP extensions can record PHP/frankenphp requirements, install missing artifacts, and then refresh loaded/ignored extension state from the installed PHP artifact before gateway reconciliation.
- Covered missing-install extension refresh —
crates/daemon/src/jobs.rsnow includes a regression test that seeds cached PHP/frankenphp artifacts, links a project requestingredis, runs the combined reconciliation helper, and verifies the final persisted PHP runtime loadsredisinstead of leaving it ignored.
GPT ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/daemon/src/jobs.rs (1)
1680-1723: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse top-level imports for the fixture helpers.
Line 1681 and Line 1701 onward use fully qualified
std::...paths in the new helpers. Importstd::{fs, process}at the test-module top and usefs::.../process::Command::new(...)inside the helpers.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
🤖 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/daemon/src/jobs.rs` around lines 1680 - 1723, The fixture helper functions run_fixture_command, copy_file, file_size, and set_executable use fully qualified std paths like std::process::Command and std::fs:: throughout. Add a top-level import statement `use std::{fs, process};` at the test module level, then replace all fully qualified paths: change std::process::Command to process::Command in run_fixture_command, and change all std::fs:: calls to fs:: in the copy_file, file_size, and set_executable functions.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 `@crates/daemon/src/jobs.rs`:
- Around line 1652-1668: The hard-coded absolute paths `/usr/bin/tar` and
`/usr/bin/shasum` in the run_fixture_command calls make the fixture tools
non-portable across different Linux systems. Replace the absolute path
`/usr/bin/tar` with just the command name "tar" to allow PATH resolution. For
the sha256_file method that calls shasum, replace the hard-coded
`/usr/bin/shasum` path with logic that attempts to run "shasum" first, and if
that fails, falls back to running "sha256sum" as an alternative, ensuring the
test works across different Linux runners where either tool might be available.
---
Nitpick comments:
In `@crates/daemon/src/jobs.rs`:
- Around line 1680-1723: The fixture helper functions run_fixture_command,
copy_file, file_size, and set_executable use fully qualified std paths like
std::process::Command and std::fs:: throughout. Add a top-level import statement
`use std::{fs, process};` at the test module level, then replace all fully
qualified paths: change std::process::Command to process::Command in
run_fixture_command, and change all std::fs:: calls to fs:: in the copy_file,
file_size, and set_executable functions.
🪄 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: aee30e7f-7b3a-4394-9af3-bac677cff241
📒 Files selected for processing (4)
crates/cli/src/commands/php.rscrates/cli/tests/php.rscrates/daemon/src/jobs.rscrates/daemon/src/project_env.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/commands/php.rs
- crates/daemon/src/project_env.rs
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — This incremental review covered the two commits after the prior Pullfrog review at 44cc1f1, focused on shim behavior when project config is invalid and daemon test fixture portability.
- Reused persisted shim runtime for invalid project config —
pv phpand Composer shims now fall back to the persisted project PHP runtime whenpv.ymlcannot be parsed, while still validating the installed PHP runtime and persisted extension metadata before execution. - Made archive fixture tooling portable — daemon reconciliation tests now call
tarthroughPATHand fall back fromshasumtosha256sumwhen computing cached artifact checksums.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28178a178
ℹ️ 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.
Reviewed changes — This incremental review covered the new release archive metadata consistency check added after the prior Pullfrog review at a28178a.
- Required artifact PHP extension metadata to match release records —
pv-releasenow rejects PHP artifacts whose advertisedphp_extensionslackshare/pv/php-extensions.json, contain invalid artifact metadata JSON, or differ from the release-record extension catalog. - Covered missing and mismatched metadata failures —
crates/pv-release/tests/archive_validation.rsnow verifies both missing artifact metadata and mismatched catalog entries fail archive validation with clear diagnostics.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef6f9bb7ce
ℹ️ 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".
| if let Some(php) = php { | ||
| let track = match config_track { | ||
| Some(track) => track, | ||
| None => resolve_project_config_php_track_for_shim(paths, database, php)?, |
There was a problem hiding this comment.
When a linked Project has php.version: latest with a persisted concrete runtime, this None arm is reached as soon as the extension list changes, and it resolves latest against the current cached manifest instead of the stored project track. If the manifest default has moved from 8.4 to 8.5, editing only extensions makes pv php/Composer try 8.5+... while gateway reconciliation still applies the explicit latest selector to project.desired_php_track and serves 8.4+..., so CLI and browser runtimes diverge until the config is changed again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Important
The new latest shim test still preserves the stale-track behavior, so pv php and Composer can execute the wrong PHP version until reconciliation updates persisted state.
Reviewed changes — This incremental review covered the new f4bb620 fixes after the prior Pullfrog review at ef6f9bb, focused on shim latest handling, reconciliation refreshes, and PHP extension metadata constraints.
- Exercised
latestshim fallback with stale persisted state —crates/cli/tests/php.rsnow adds aphp.version: latestscenario with persisted8.4state and a manifest default of8.5. - Refreshed project extensions after missing runtime installs —
crates/daemon/src/jobs.rsnow reruns project-env reconciliation after installing missing PHP/frankenphp runtime resources for extension-requesting projects. - Scoped PHP extension metadata to PHP runtimes — release records and resource manifests now reject
php_extensionson non-php/frankenphpartifacts. - Preserved artifact metadata order for loaded extensions —
resources::php_extensionsnow resolves loaded extension modules in artifact metadata order rather than sorted request order.
GPT ｜ 𝕏
| .unwrap_or_default(); | ||
| let config_track = if let Some(php) = php { | ||
| match php.version_selector() { | ||
| Some(selector) if selector != "latest" && selector != track => Some( |
There was a problem hiding this comment.
php.version: latest still takes the _ => None branch here, so current_track falls back to the persisted runtime track instead of the manifest-resolved default. The new regression test sets the manifest default to 8.5 but asserts execution of the persisted 8.4 release, which locks in the stale-track behavior this path was meant to prevent.
Technical details
# `latest` shims still reuse stale persisted tracks
## Affected sites
- `crates/cli/src/commands/php.rs:313` — `latest` never calls `resolve_project_config_php_track_for_shim`, so `config_track` remains `None`.
- `crates/cli/src/commands/php.rs:332` — `current_track` therefore stays at the persisted `project.php_runtime.track`.
- `crates/cli/tests/php.rs:516` — the new `latest` test currently expects `old_release` even though the cached manifest default is `8.5`.
## Required outcome
- A current `php.version: latest` selector must be resolved through the cached manifest before choosing the shim runtime, regardless of stale persisted project runtime state.
- The regression test should fail on stale `8.4` execution and pass only when the shim uses the manifest-selected `8.5` runtime and `8.5+redis` overlay.
## Suggested approach
Resolve `Some(TrackSelector::Latest)` before the empty-extension and requested-extension reuse paths, then compare the resolved track with the persisted track the same way the concrete-track/default-track branches do.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4bb62089a
ℹ️ 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".
| .join("php-runtimes") | ||
| .join(runtime_key) | ||
| .join("conf.d"); | ||
| fs::ensure_user_dir(&conf_dir).map_err(resources_error_from_state)?; |
There was a problem hiding this comment.
When the artifact metadata order changes for the same loaded extension set, the sorted runtime_key stays the same but this directory is only ensured, not cleared. For example, a runtime previously writing 10-redis.ini/20-xdebug.ini and later seeing metadata ordered as xdebug, redis will add 10-xdebug.ini/20-redis.ini while leaving the old files behind, so pv php and FrankenPHP scan duplicate directives and emit double-load warnings (or worse for zend extensions) until the user manually deletes PV config. Recreate or prune conf_dir before writing the current overlay.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest commit; the prior
php.version: lateststale-track finding remains open.
Reviewed changes — This incremental review covered 1edef98, which makes sockets part of the default PHP artifact extension set.
- Built
socketsby default —socketsis now included inREQUIRED_PHP_EXTENSIONS, recipedefault_extensions, expected extension lists, recipe-env snapshots, and the design text so PHP artifacts smoke-test it as a loaded default extension.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1edef9842e
ℹ️ 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".
| } else { | ||
| None | ||
| }; | ||
| let current_track = config_track.as_deref().unwrap_or(&track).to_string(); | ||
| if requested_extensions.is_empty() { |
There was a problem hiding this comment.
For a linked Project with no php key but an old persisted project.php_runtime.track, this branch leaves config_track as None, so the empty-extension return immediately below keeps using the stale persisted track. After pv php:use --global 8.4 only queues reconciliation (or when the daemon is unavailable), pv php/Composer inside that configless Project continue execing the old track even though configless Projects are supposed to follow the current global/default PHP track. Resolve through the global/default flow when php is None instead of falling back to track here.
Useful? React with 👍 / 👎.
| artifact_root, | ||
| &worker.loaded_modules, |
There was a problem hiding this comment.
When the selected PHP and FrankenPHP artifacts for a track do not advertise the same optional modules, worker.loaded_modules has been resolved from the standalone php artifact, but this call writes extension= directives using the FrankenPHP artifact_root. The release tooling validates each record/archive independently, so a mismatched FrankenPHP artifact can still be published and installed; then the CLI loads the extension from the PHP artifact while the browser worker points at a missing .so under the FrankenPHP artifact and runs without it. Validate or re-resolve the loaded catalog against the FrankenPHP artifact before building the worker environment.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
ℹ️ No new issues found in the latest commit; the prior
php.version: lateststale-track finding remains open.
Reviewed changes — This incremental review covered b2db34c, focused on cleanup of generated PHP runtime overlays after extension metadata changes.
- Pruned stale runtime overlay files —
resources::ensure_php_runtime_overlaynow removes the existing runtimeconf.ddirectory before rewriting extension ini files, with resource-level coverage for reordered extension metadata no longer leaving obsolete numbered ini files.
GPT ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2db34cbc4
ℹ️ 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".
| frankenphp_binary="$artifact_root/bin/frankenphp" | ||
| "$frankenphp_binary" php-cli -r 'printf("PHP %s\n", PHP_VERSION);' | grep -F "PHP $expected_version" >/dev/null | ||
| check_extensions "$frankenphp_binary" php-cli -r "foreach (get_loaded_extensions() as \$extension) { echo \$extension, PHP_EOL; }" | ||
| check_extensions "$expected_extensions" "$frankenphp_binary" php-cli -r "foreach (get_loaded_extensions() as \$extension) { echo \$extension, PHP_EOL; }" |
There was a problem hiding this comment.
When StaticPHP ends up loading an optional module by default (for example because the shared-build flag does not keep redis or xdebug disabled), this still passes because check_extensions only verifies that every expected module is present and ignores extra loaded modules. Since expected_extensions now intentionally omits the optional catalog, the release smoke can publish artifacts where Projects without php.extensions still get optional modules loaded by default; make the default smoke fail on any advertised optional module before running the opt-in overlay check.
Useful? React with 👍 / 👎.
| PHP_INI_SCAN_DIR=$scan_dir | ||
| export PHP_INI_SCAN_DIR | ||
| check_extensions "$optional_extensions" "$@" |
There was a problem hiding this comment.
When an optional module only loads because another optional module is also enabled, this single scan directory enables the entire advertised catalog and the one check_extensions call can pass, but PV later writes overlays for exactly the Project's requested subset. A Project requesting only that module can then fail at runtime even though the artifact was published; generate a fresh scan dir and verify each metadata entry independently.
Useful? React with 👍 / 👎.
1 participant
Summary
Adds project-level PHP extension opt-ins without named profiles. Projects can continue using scalar
php: 8.4, or use object form withversionandextensions.This PR wires the feature end to end:
php.extensionsin Project configpv phpand Composerpv listandpv statusValidation
cargo nextest run -p cli -E 'test(status_reports_warning_project_env_as_success) or test(status_prefers_ignored_php_extension_over_other_project_env_warnings)'cargo check --workspace --all-targets --lockedcargo clippy -p cli --all-targets --all-features --locked -- -D warningsNotes
This is a draft PR. The feature branch worktree is preserved at
.worktrees/php-extension-opt-insfor follow-up iteration.Summary by CodeRabbit
php.extensions(supported alongsidephpversion selectors) with runtime-key grouping (e.g.,8.4+redis) and automatic activation via generatedconf.doverlays.pv shim:phpandpv shim:composer about.pv-releasecan record PHP extension modules; manifests include per-artifact extension metadata.phpconfiguration syntax and behavior.