feat: add resource-only projects - #311
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:
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 selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds served and resource-only Project modes, immutable Project slugs, mode-aware persistence and daemon reconciliation, configurable environment files, slug-based allocation names, and selector-aware CLI commands. ChangesResource-only Project lifecycle
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)
✨ Finishing Touches 💡 1📝 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 |
![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: c29be2f05a
ℹ️ 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".
| CREATE TRIGGER projects_project_slug_required_insert | ||
| BEFORE INSERT ON projects | ||
| WHEN NEW.project_slug IS NULL OR NEW.project_slug = '' | ||
| BEGIN | ||
| SELECT RAISE(ABORT, 'project slug is required'); |
There was a problem hiding this comment.
After this migration has run, the immediately previous PV binary still inserts Projects without a project_slug column, so this trigger aborts every new pv link with project slug is required. This breaks the documented migration requirement that restoring the previous binary after an update remains safe; keep the column compatible with legacy inserts or populate it without rejecting them.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
| ALTER TABLE projects | ||
| ADD COLUMN serves_http INTEGER NOT NULL DEFAULT 1 CHECK (serves_http IN (0, 1)); | ||
|
|
||
| UPDATE projects SET project_slug = id WHERE project_slug IS NULL; |
There was a problem hiding this comment.
On upgrade, every existing Project receives its random internal ID as its permanent slug rather than the normalized directory basename required by the Project-slug design. Because the migration also makes slugs immutable, relinking cannot repair this: users must select migrated Projects by an opaque ID, and newly added Resource allocations inherit that ID in their generated database, bucket, or prefix names.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
| &project.primary_hostname, | ||
| &config_file.config.hostnames, | ||
| )?; | ||
| let serves_http = project.mode == ProjectMode::Served && config_file.config.serve; |
There was a problem hiding this comment.
When a stored resource-only Project is changed to serve: true while the daemon is unavailable or has not reconciled yet, this conjunction remains false because the persisted mode is still ResourceOnly. Consequently pv project:env omits every ${project_url} and TLS-dependent entry and skips hostname validation, even though those entries are supposed to be restored when serving is enabled; build the preview from the candidate config mode and its slug-derived hostname rather than requiring the old persisted mode to agree.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Two state compatibility issues need to be resolved before merge: existing Projects receive internal-ID slugs, and failed mode transitions can leave persisted state out of sync with the active Gateway.
Reviewed changes — This PR adds resource-only Projects that retain managed resources and generated env while opting out of HTTP serving, and introduces stable Project slugs to support that mode.
- Add resource-only Project configuration —
serve: falsesuppresses Gateway, TLS, and worker demand while preserving dormant serving config and continuing resource reconciliation. - Add configurable env targets —
env_filesafely resolves within the Project root, and rendering omits entries that depend on serving-only placeholders. - Persist Project mode and slug — migration 009 adds immutable slugs and serving mode, with internal compatibility hostnames for new resource-only records.
- Use slugs for allocation namespaces — new SQL databases, Redis prefixes, and RustFS buckets derive names from the stable Project slug.
- Expose mode-aware CLI behavior — list/status output and selectors support slugs, while
pv openrejects resource-only Projects. - Preserve valid runtime state during config edits — reconciliation preflights mode-changing config before applying the candidate Project state.
GPT Sol ｜ 𝕏
| ALTER TABLE projects | ||
| ADD COLUMN serves_http INTEGER NOT NULL DEFAULT 1 CHECK (serves_http IN (0, 1)); | ||
|
|
||
| UPDATE projects SET project_slug = id WHERE project_slug IS NULL; |
There was a problem hiding this comment.
Technical details
# Existing Projects receive internal-ID slugs
## Affected sites
- `crates/state/src/sql/009_project_mode_and_slug.sql:5` — copies the internal `id` into `project_slug`.
- `crates/cli/src/commands/project.rs:530` — treats that migrated value as a user-facing selector.
- `crates/daemon/src/project_env.rs:661` — uses it for newly generated allocation names.
- `DESIGN.md:1260-1262` — specifies basename-derived readable slugs and allocation namespaces.
## Required outcome
- Existing Projects receive the same readable slug shape as newly linked Projects.
- Colliding basename slugs are assigned deterministic available suffixes without changing existing allocation records.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/state/src/database.rs (1)
786-810: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the project's persisted mode instead of forcing
ProjectMode::Served.The guard only rejects projects without a hostname, but a Served→ResourceOnly project retains its primary hostname (line 673-686), so a resource-only project can reach line 810 and silently have
serves_httpflipped back to1. The daemon currently gates this call behindserves_http, but the API is public and the mode flip is invisible. Passproject.modethrough.🛡️ Proposed fix
- update_project_in_transaction(&transaction, project_id, &input, ProjectMode::Served)?; + update_project_in_transaction(&transaction, project_id, &input, project.mode)?;🤖 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 786 - 810, Update the LinkProjectInput construction in the hostname-update flow to preserve the persisted project mode when calling update_project_in_transaction, instead of forcing ProjectMode::Served. Pass project.mode through so ResourceOnly projects remain ResourceOnly while retaining their hostname data.
🧹 Nitpick comments (9)
crates/cli/src/commands/php.rs (1)
668-677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
project_display_namehelper in two command modules. Both files define a byte-identical mode-aware display helper; the shared root cause is a missing single crate-local definition, so future changes to display rules must be made twice.
crates/cli/src/commands/php.rs#L668-L677: delete this copy and import the shared helper instead.crates/cli/src/commands/project.rs#L577-L586: promote this definition topub(crate)(or move it to a sharedcommandshelper module) as the single source of truth.🤖 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 668 - 677, Remove the duplicate project_display_name helper from crates/cli/src/commands/php.rs:668-677 and import the shared implementation. In crates/cli/src/commands/project.rs:577-586, expose project_display_name as pub(crate) so it is the single source of truth for both command modules.it/cli.rs (1)
716-725: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit mode assertion alongside the snapshot.
The test name promises mode visibility, but verification is snapshot-only, so an accidental snapshot re-accept could silently drop the
Modecolumn. A directassert!(list.stdout.contains("resource-only"))/contains("served")pins the contract.🤖 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 `@it/cli.rs` around lines 716 - 725, Add explicit assertions near the existing snapshot in the mode visibility test, checking that list.stdout contains both "resource-only" and "served". Keep the snapshot assertion unchanged so the test directly verifies the Mode column contract.crates/cli/src/commands/project.rs (2)
757-760: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSecond config read per Project.
project_list_statusalready parsedProjectConfigFilefor this project; reading it again here doubles the config I/O for every project inpv list. Consider returning the parsed config (or theenv_file) fromproject_list_statusinstead.🤖 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/project.rs` around lines 757 - 760, Update project_list_status and its caller so the existing ProjectConfigFile parse is reused to obtain env_file, rather than calling ProjectConfigFile::read_from_root again for each project. Return the parsed configuration or env_file from project_list_status while preserving the current status behavior and optional env_file handling.
402-429: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the repeated
serves_httpbranches.Four separate
if serves_http { … } else { String::new() }arms restate the same condition. A single branch that builds the serving fields (or returns early with empty strings) reads better.♻️ Suggested shape
- ProjectEnvContext { - primary_hostname: if serves_http { - context.primary_hostname.unwrap_or_default() - } else { - String::new() - }, - tls_ca_path: if serves_http { - paths.ca_certificate().to_string() - } else { - String::new() - }, - tls_cert_path: if serves_http { - paths.project_tls_certificate(&project_id).to_string() - } else { - String::new() - }, - tls_key_path: if serves_http { - paths.project_tls_private_key(&project_id).to_string() - } else { - String::new() - }, + let (primary_hostname, tls_ca_path, tls_cert_path, tls_key_path) = if serves_http { + ( + context.primary_hostname.unwrap_or_default(), + paths.ca_certificate().to_string(), + paths.project_tls_certificate(&project_id).to_string(), + paths.project_tls_private_key(&project_id).to_string(), + ) + } else { + Default::default() + }; + + ProjectEnvContext { + primary_hostname, + tls_ca_path, + tls_cert_path, + tls_key_path,🤖 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/project.rs` around lines 402 - 429, Refactor project_env_context so the repeated serves_http condition is evaluated once when constructing the serving-related fields. Build primary_hostname, tls_ca_path, tls_cert_path, and tls_key_path together in the HTTP-serving branch, while preserving empty-string values for all four fields when serves_http is false.crates/cli/src/commands/status.rs (1)
307-325: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid the magic
"resource-only"string comparison.
display_namecompares a serialized label, so any change toProjectMode::as_str()silently degrades this to the hostname branch instead of failing to compile. Store theProjectMode(serializing it via serde) or at minimum compare againstProjectMode::ResourceOnly.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/cli/src/commands/status.rs` around lines 307 - 325, Update ProjectStatus::display_name to avoid comparing mode against the hardcoded "resource-only" literal; use the canonical ProjectMode::ResourceOnly.as_str() value, or store ProjectMode directly with serde serialization and compare the enum variant. Preserve the existing slug selection for resource-only projects and hostname fallback for all others.crates/cli/src/error.rs (1)
24-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMessage repeats the selector and only offers one disambiguation path.
selectoris interpolated twice ("selectoracmematches slugacme"), and the hint only tells the user how to reach the hostname-matched Project — there is no stated way to target the slug-matched one. Consider tightening to something like: "{selector}matches a Project slug and the hostname{hostname}on different Projects; pass{hostname}for the served Project, or rename to disambiguate." (crates/cli/tests/project_env.rsLine 269 asserts on this text.)🤖 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/error.rs` around lines 24 - 27, Update the AmbiguousProjectSelector error message to avoid repeating selector and explicitly describe both resolution paths: passing hostname `{hostname}` selects the served Project, while renaming disambiguates the slug match. Preserve the wording expected by the assertion in project_env.rs.crates/cli/src/args.rs (1)
484-503: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming the field to match its new meaning.
All three positionals now accept a slug or hostname, and
crates/cli/src/commands/project.rsalready binds them toselectorlocals (Line 162, Line 499). Keeping the field namedhostnameis misleading for future readers; renaming toproject(orselector) would align the struct with the CLI contract.🤖 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/args.rs` around lines 484 - 503, Rename the positional field hostname to selector (or project) in the relevant argument structs, including the unlink, open, and ProjectEnvArgs definitions, and update all command consumers to use the new field name. Preserve the existing slug-or-hostname CLI behavior and selector bindings in project command handling.crates/config/src/parser.rs (1)
269-280: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
Utf8Componentat the top level instead of fully qualifying it.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".
♻️ Proposed change
- for component in env_file.components() { - match component { - camino::Utf8Component::Normal(_) => depth += 1, - camino::Utf8Component::ParentDir if depth == 0 => { - return Err(ConfigError::EnvFileEscapesProject { env_file }); - } - camino::Utf8Component::ParentDir => depth -= 1, - camino::Utf8Component::CurDir => {} - camino::Utf8Component::RootDir | camino::Utf8Component::Prefix(_) => { - return Err(ConfigError::AbsoluteEnvFile { env_file }); - } + for component in env_file.components() { + match component { + Utf8Component::Normal(_) => depth += 1, + Utf8Component::ParentDir if depth == 0 => { + return Err(ConfigError::EnvFileEscapesProject { env_file }); + } + Utf8Component::ParentDir => depth -= 1, + Utf8Component::CurDir => {} + Utf8Component::RootDir | Utf8Component::Prefix(_) => { + return Err(ConfigError::AbsoluteEnvFile { env_file }); + } } }with
use camino::{Utf8Component, Utf8PathBuf};at the top of the file.🤖 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/src/parser.rs` around lines 269 - 280, Update the imports in parser.rs to include Utf8Component alongside Utf8PathBuf, then replace the fully qualified camino::Utf8Component references in the env_file component match with Utf8Component.Source: Coding guidelines
crates/state/src/database.rs (1)
3431-3502: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSlug generation looks correct; one note on the
.invalidsentinel check.
base[..base_length]is byte-slicing, which is safe here only becauseproject_slug_baseemits ASCII alphanumerics and hyphens — worth a short comment so a future change to the character set doesn't introduce a char-boundary panic.is_internal_project_hostnamematching any.invalidsuffix is safe given hostnames are validated as.test.🤖 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 3431 - 3502, Add a brief comment at the byte-slice in generate_project_slug explaining that base is ASCII-only because project_slug_base restricts generated characters to alphanumerics and hyphens, making byte indexing safe. Do not alter slug generation or the is_internal_project_hostname behavior.
🤖 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/cli/src/commands/project.rs`:
- Around line 42-57: Update the primary_hostname selection in the project
command to detect when args.hostname is explicitly provided while
config_file.config.serve is false. Reject this incompatible combination or emit
a clear warning before continuing, rather than silently discarding the hostname;
preserve the existing hostname selection behavior for valid combinations.
- Around line 530-532: Update the project resolution flow around slug_project,
normalize_primary_hostname, and hostname_project so slug matches are handled
before hostname normalization errors. Treat normalization failure as no hostname
candidate, and only unwrap/use the normalized hostname on ambiguity or matched
paths where it is required; preserve ProjectNotResolved when neither candidate
resolves.
In `@crates/resources/src/allocation.rs`:
- Around line 80-91: Update the allocation-name composition logic for
ResourceAllocationKind::SqlDatabase, RedisPrefix, and RustfsBucket to encode
project_slug and allocation_name with an unambiguous boundary, preventing
distinct component pairs from producing the same physical name. Preserve each
backend’s required naming format while applying the encoding consistently, and
add regression cases covering the specified collision pairs for all three
backends.
In `@crates/state/src/sql/009_project_mode_and_slug.sql`:
- Line 5: The migration’s project_slug backfill must preserve the readable,
basename-derived slug contract instead of copying opaque project IDs. Replace
the UPDATE with a Rust-side migration/backfill that derives slugs from each
project’s path, applies the existing collision-suffixing rules, and writes
unique results before the immutable-slug trigger applies; otherwise explicitly
document and preserve the legacy behavior.
---
Outside diff comments:
In `@crates/state/src/database.rs`:
- Around line 786-810: Update the LinkProjectInput construction in the
hostname-update flow to preserve the persisted project mode when calling
update_project_in_transaction, instead of forcing ProjectMode::Served. Pass
project.mode through so ResourceOnly projects remain ResourceOnly while
retaining their hostname data.
---
Nitpick comments:
In `@crates/cli/src/args.rs`:
- Around line 484-503: Rename the positional field hostname to selector (or
project) in the relevant argument structs, including the unlink, open, and
ProjectEnvArgs definitions, and update all command consumers to use the new
field name. Preserve the existing slug-or-hostname CLI behavior and selector
bindings in project command handling.
In `@crates/cli/src/commands/php.rs`:
- Around line 668-677: Remove the duplicate project_display_name helper from
crates/cli/src/commands/php.rs:668-677 and import the shared implementation. In
crates/cli/src/commands/project.rs:577-586, expose project_display_name as
pub(crate) so it is the single source of truth for both command modules.
In `@crates/cli/src/commands/project.rs`:
- Around line 757-760: Update project_list_status and its caller so the existing
ProjectConfigFile parse is reused to obtain env_file, rather than calling
ProjectConfigFile::read_from_root again for each project. Return the parsed
configuration or env_file from project_list_status while preserving the current
status behavior and optional env_file handling.
- Around line 402-429: Refactor project_env_context so the repeated serves_http
condition is evaluated once when constructing the serving-related fields. Build
primary_hostname, tls_ca_path, tls_cert_path, and tls_key_path together in the
HTTP-serving branch, while preserving empty-string values for all four fields
when serves_http is false.
In `@crates/cli/src/commands/status.rs`:
- Around line 307-325: Update ProjectStatus::display_name to avoid comparing
mode against the hardcoded "resource-only" literal; use the canonical
ProjectMode::ResourceOnly.as_str() value, or store ProjectMode directly with
serde serialization and compare the enum variant. Preserve the existing slug
selection for resource-only projects and hostname fallback for all others.
In `@crates/cli/src/error.rs`:
- Around line 24-27: Update the AmbiguousProjectSelector error message to avoid
repeating selector and explicitly describe both resolution paths: passing
hostname `{hostname}` selects the served Project, while renaming disambiguates
the slug match. Preserve the wording expected by the assertion in
project_env.rs.
In `@crates/config/src/parser.rs`:
- Around line 269-280: Update the imports in parser.rs to include Utf8Component
alongside Utf8PathBuf, then replace the fully qualified camino::Utf8Component
references in the env_file component match with Utf8Component.
In `@crates/state/src/database.rs`:
- Around line 3431-3502: Add a brief comment at the byte-slice in
generate_project_slug explaining that base is ASCII-only because
project_slug_base restricts generated characters to alphanumerics and hyphens,
making byte indexing safe. Do not alter slug generation or the
is_internal_project_hostname behavior.
In `@it/cli.rs`:
- Around line 716-725: Add explicit assertions near the existing snapshot in the
mode visibility test, checking that list.stdout contains both "resource-only"
and "served". Keep the snapshot assertion unchanged so the test directly
verifies the Mode column contract.
🪄 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: 7146c109-184f-496b-8f4f-b3d3ee248a4a
⛔ Files ignored due to path filters (73)
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/list_json__list_json_exposes_resource_only_mode_slug_and_env_file_without_sentinel.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/list_json__list_json_outputs_linked_projects.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/project_env__project_env_rejects_bare_slug_hostname_ambiguity.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/project_env__project_env_resolves_resource_only_slug_and_reads_configured_env_file.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/project_open__open_rejects_resource_only_target_and_excludes_it_from_picker.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/project_open__open_without_current_project_fails_when_non_interactive.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/project_unlink__unlink_resolves_resource_only_slug_and_leaves_managed_env_block.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_discovery_validates_paths_and_conflicts.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_expands_yaml_aliases_and_merge_keys.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_parses_resource_only_controls_and_defaults.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_validates_url_placeholder_scopes.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_resource_only_controls.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/config/tests/snapshots/project_config__project_config_writer_writes_full_config_to_preferred_file.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_env__resource_only_env_omits_serving_placeholders_across_scopes.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__mysql_tests__mysql_reconciliation_creates_database_allocation_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__async_readiness_reassigns_unowned_persisted_port_before_resource_readiness.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__demanded_resource_uses_async_readiness_and_allocation_hooks.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_reconciliation_creates_database_allocation_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__postgres_reconciliation_replaces_stale_admin_username_from_track_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__redis_port_reassignment_refreshes_ready_allocation_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__redis_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__redis_reconciliation_marks_prefix_allocation_ready_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__redis_reconciliation_reuses_ready_prefix_allocation.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__rustfs_allocation_failure_preserves_project_env_and_records_failed_runtime.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__rustfs_port_reassignment_renders_current_endpoint_for_ready_allocation.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__rustfs_project_demand_installs_missing_fixture_track_before_start.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__rustfs_ready_allocation_reconciliation_repairs_missing_bucket_and_preserves_env.snapis excluded by!**/*.snapcrates/daemon/src/managed_resources/snapshots/daemon__managed_resources__tests__rustfs_reconciliation_creates_bucket_and_renders_env.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__config_declared_hostnames_are_persisted_during_reconciliation.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__first_allocation_reconciliation_records_desired_state_before_context_failure.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__generated_allocation_name_too_long_leaves_resource_state_unchanged.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__invalid_resource_only_transition_preserves_served_mode.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__resource_only_project_uses_custom_env_file_and_no_php_worker.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/project_env_reconciliation__resources_and_empty_allocations_without_env_mappings_update_state_without_dotenv.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/resource_allocations__generated_allocation_names_enforce_sixty_three_character_limit.snapis excluded by!**/*.snapcrates/resources/tests/snapshots/resource_allocations__resource_allocations_generate_resource_specific_names.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__generated_env_context_escapes_round_trip_through_state.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!**/*.snapcrates/state/tests/snapshots/state_foundation__project_env_context_uses_ready_allocations_from_required_track_only.snapis excluded by!**/*.snapcrates/state/tests/snapshots/state_foundation__resource_allocations_preserve_generated_names_and_env_context.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_bash_script.snapis excluded by!**/*.snapit/snapshots/cli__completions_generate_zsh_script.snapis excluded by!**/*.snapit/snapshots/cli__core_workflow_command_help_is_documented.snapis excluded by!**/*.snapit/snapshots/cli__project_link_accepts_relative_path_arguments.snapis excluded by!**/*.snapit/snapshots/cli__project_link_list_and_unlink_use_injected_home.snapis excluded by!**/*.snapit/snapshots/cli__project_list_clears_stale_env_status_without_mappings.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_config_hostname_validation_errors.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_env_observed_status.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_env_shape_validation_errors.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_ignored_php_extensions.snapis excluded by!**/*.snapit/snapshots/cli__project_list_reports_invalid_linked_config.snapis excluded by!**/*.snapit/snapshots/cli__project_list_shows_project_and_mode_for_served_and_resource_only_projects.snapis excluded by!**/*.snap
📒 Files selected for processing (37)
DESIGN.mdcrates/cli/src/args.rscrates/cli/src/commands/php.rscrates/cli/src/commands/project.rscrates/cli/src/commands/status.rscrates/cli/src/error.rscrates/cli/tests/list_json.rscrates/cli/tests/php.rscrates/cli/tests/project_env.rscrates/cli/tests/project_open.rscrates/cli/tests/project_unlink.rscrates/config/src/discovery.rscrates/config/src/env.rscrates/config/src/error.rscrates/config/src/lib.rscrates/config/src/model.rscrates/config/src/parser.rscrates/config/tests/project_config.rscrates/config/tests/project_env.rscrates/daemon/src/gateway.rscrates/daemon/src/jobs.rscrates/daemon/src/managed_resources/tests.rscrates/daemon/src/project_env.rscrates/daemon/src/server.rscrates/daemon/src/watcher.rscrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/project_env_reconciliation.rscrates/resources/src/allocation.rscrates/resources/tests/resource_allocations.rscrates/state/src/database.rscrates/state/src/error.rscrates/state/src/lib.rscrates/state/src/migrations.rscrates/state/src/sql/009_project_mode_and_slug.sqlcrates/state/tests/state_foundation.rsit/cli.rs
| let mode = if config_file.config.serve { | ||
| ProjectMode::Served | ||
| } else { | ||
| ProjectMode::ResourceOnly | ||
| }; | ||
| let primary_hostname = match ( | ||
| config_file.config.serve.then_some(args.hostname).flatten(), | ||
| existing.as_ref(), | ||
| ) { | ||
| (Some(hostname), _) => config::normalize_primary_hostname(&hostname)?, | ||
| (None, Some(project)) => project.primary_hostname.clone(), | ||
| (None, Some(project)) => project | ||
| .primary_hostname | ||
| .clone() | ||
| .unwrap_or_else(|| format!("{}.test", project.slug)), | ||
| (None, None) => config::hostname_from_project_path(&project_path)?, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--hostname is silently ignored for resource-only Projects.
config_file.config.serve.then_some(args.hostname).flatten() discards an explicitly provided --hostname when serve: false, and the user gets no feedback. Prefer rejecting the combination (or emitting a warning line) so the ignored flag is visible.
🤖 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/project.rs` around lines 42 - 57, Update the
primary_hostname selection in the project command to detect when args.hostname
is explicitly provided while config_file.config.serve is false. Reject this
incompatible combination or emit a clear warning before continuing, rather than
silently discarding the hostname; preserve the existing hostname selection
behavior for valid combinations.
| let slug_project = database.project_by_slug(selector)?; | ||
| let hostname = config::normalize_primary_hostname(selector)?; | ||
| let hostname_project = database.project_by_hostname(&hostname)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Hostname normalization failure masks a valid slug match / not-found.
config::normalize_primary_hostname(selector)? runs before the slug result is consumed, so an input that isn't a legal DNS label (e.g. my_project) surfaces an invalid-hostname config error instead of the intended ProjectNotResolved. Resolve the slug first and treat normalization failure as "no hostname candidate".
🐛 Suggested fix
- let slug_project = database.project_by_slug(selector)?;
- let hostname = config::normalize_primary_hostname(selector)?;
- let hostname_project = database.project_by_hostname(&hostname)?;
+ let slug_project = database.project_by_slug(selector)?;
+ let hostname = config::normalize_primary_hostname(selector).ok();
+ let hostname_project = match hostname.as_deref() {
+ Some(hostname) => database.project_by_hostname(hostname)?,
+ None => None,
+ };(subsequent uses of hostname then need the Option unwrapped only on the ambiguity/matched paths)
🤖 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/project.rs` around lines 530 - 532, Update the
project resolution flow around slug_project, normalize_primary_hostname, and
hostname_project so slug matches are handled before hostname normalization
errors. Treat normalization failure as no hostname candidate, and only
unwrap/use the normalized hostname on ambiguity or matched paths where it is
required; preserve ProjectNotResolved when neither candidate resolves.
| ResourceAllocationKind::SqlDatabase => { | ||
| format!( | ||
| "{}_{}", | ||
| sql_hostname_slug(primary_hostname), | ||
| project_slug.replace('-', "_"), | ||
| allocation_name.replace('-', "_") | ||
| ) | ||
| } | ||
| ResourceAllocationKind::RedisPrefix => { | ||
| format!( | ||
| "{}-{}-", | ||
| dash_hostname_slug(primary_hostname), | ||
| allocation_name.replace('_', "-") | ||
| ) | ||
| format!("{}-{}-", project_slug, allocation_name.replace('_', "-")) | ||
| } | ||
| ResourceAllocationKind::RustfsBucket => { | ||
| format!( | ||
| "{}-{}", | ||
| dash_hostname_slug(primary_hostname), | ||
| allocation_name.replace('_', "-") | ||
| ) | ||
| format!("{}-{}", project_slug, allocation_name.replace('_', "-")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make allocation-name composition collision-free.
Line 83 can generate identical physical names for distinct projects: SQL maps ("a-b", "c") and ("a", "b_c") to a_b_c. Redis and RustFS similarly collide for ("a-b", "c") and ("a", "b-c"). Use an unambiguous component encoding/boundary (for example, a length-prefixed slug component) and add regression cases for all three backends.
🤖 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/allocation.rs` around lines 80 - 91, Update the
allocation-name composition logic for ResourceAllocationKind::SqlDatabase,
RedisPrefix, and RustfsBucket to encode project_slug and allocation_name with an
unambiguous boundary, preventing distinct component pairs from producing the
same physical name. Preserve each backend’s required naming format while
applying the encoding consistently, and add regression cases covering the
specified collision pairs for all three backends.
| ALTER TABLE projects | ||
| ADD COLUMN serves_http INTEGER NOT NULL DEFAULT 1 CHECK (serves_http IN (0, 1)); | ||
|
|
||
| UPDATE projects SET project_slug = id WHERE project_slug IS NULL; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfilling project_slug from the opaque project ID conflicts with the readable-slug contract.
Existing projects get slugs like k3f9x2a1b7 instead of a basename-derived slug, and because slugs are immutable (trigger at lines 18-25) this can never be corrected. DESIGN lines 1260 and 1076 state the slug is derived from the canonical directory basename and is the readable namespace shown in pv list, used by pv unlink/pv project:env selectors, and used for newly generated allocation names. Consider a Rust-side backfill that derives and collision-suffixes slugs from projects.path during migration, or explicitly document the legacy behavior.
🤖 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/sql/009_project_mode_and_slug.sql` at line 5, The
migration’s project_slug backfill must preserve the readable, basename-derived
slug contract instead of copying opaque project IDs. Replace the UPDATE with a
Rust-side migration/backfill that derives slugs from each project’s path,
applies the existing collision-suffixing rules, and writes unique results before
the immutable-slug trigger applies; otherwise explicitly document and preserve
the legacy behavior.
There was a problem hiding this comment.
Important
The transition fix still persists candidate PHP runtime state before the serving-mode commit, so a later failure can break the fallback worker for the previously active route.
Reviewed changes — The follow-up commit delays serving-mode persistence until reconciliation succeeds and tightens dormant-config and env behavior around resource-only transitions.
- Delayed serving-mode synchronization —
reconcile_loaded_projectnow performs candidate resource, runtime, and env work before committing mode and hostname state. - Preserved pending Gateway state — Gateway planning detects config/persisted-mode mismatches and retains the previously persisted runtime Project until a transition succeeds.
- Validated dormant config shapes — Resource-only config now still normalizes and validates hostname syntax and rejects absolute or escaping document roots while deferring existence checks.
- Cleared dormant managed env values — Empty rendering now removes values from an existing PV-managed block while leaving files untouched when no block exists.
- Stabilized Project list order — Text and JSON list output now sort served hostnames and resource-only slugs by their displayed values.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83d4ba0cab
ℹ️ 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".
| let result = database.link_project_with_mode( | ||
| LinkProjectInput { | ||
| path: project_path.clone(), | ||
| original_path: original_project_path, | ||
| primary_hostname, | ||
| config_path: config_file.path, | ||
| desired_php_track, | ||
| additional_hostnames: config_file.config.hostnames, | ||
| }, | ||
| mode, | ||
| )?; |
There was a problem hiding this comment.
When an already-linked served Project is changed to serve: false with a semantic error that ProjectConfigFile::read_from_root does not catch, such as duplicate rendered env keys, pv link reaches this call and persists ResourceOnly before the daemon runs validate_project_env_shape. Reconciliation then fails after the previous valid mode has already been lost, so a later Gateway reconciliation removes the working route instead of preserving the last valid served state; fully validate the candidate before calling link_project_with_mode, or defer the mode update until reconciliation succeeds.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
| pub fn projects(&self) -> Result<Vec<ProjectRecord>, StateError> { | ||
| let mut statement = self.connection.prepare( | ||
| "SELECT id, path, original_path, primary_hostname, config_path, desired_php_track, created_at, updated_at | ||
| "SELECT id, path, original_path, primary_hostname, config_path, desired_php_track, created_at, updated_at, project_slug, serves_http | ||
| FROM projects | ||
| ORDER BY primary_hostname", | ||
| ORDER BY project_slug", | ||
| )?; |
There was a problem hiding this comment.
For served Projects whose custom primary hostnames sort differently from their directory-derived slugs, Database::projects now returns slug order and resolve_open_project only filters that collection without sorting it again. The numbered pv open picker therefore appears in slug order rather than primary-hostname order, so a selection can refer to a different Project than the documented ordering implies; sort the served picker entries by primary_hostname before displaying them.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/config/src/parser.rs (1)
261-281: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
env_filepaths resolving to the project root.
.anddirectory/..leavedepth == 0, so they are accepted although they target a directory, not an env file. Reject zero-depth final paths and add parser coverage.🤖 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/src/parser.rs` around lines 261 - 281, Update validate_env_file so it rejects paths whose final normalized depth is zero, including "." and paths such as "directory/..", while preserving existing absolute-path and project-escape errors. Add parser tests covering these root-resolving paths and confirming valid env-file paths remain accepted.
🧹 Nitpick comments (2)
crates/config/src/parser.rs (1)
267-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueImport
Utf8Componentat module scope.Replace the fully qualified match patterns with a top-level
use camino::Utf8Component;.As per coding guidelines,
**/*.rs: “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/config/src/parser.rs` around lines 267 - 277, Update the parser module imports to add a top-level use of camino::Utf8Component, then replace the fully qualified camino::Utf8Component variants in the env_file component match with Utf8Component variants. Preserve the existing path validation behavior and error handling.Source: Coding guidelines
crates/daemon/src/project_env.rs (1)
197-217: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer top-level imports over fully-qualified paths.
crate::managed_resources::reconcile_project_resources_with_catalog_and_progress/..._with_progressare called with fully-qualified paths here rather than imported at the top of the file.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/project_env.rs` around lines 197 - 217, Import reconcile_project_resources_with_catalog_and_progress and reconcile_project_resources_with_progress at the module level, then call both functions directly in the resource_result branch instead of using fully qualified crate::managed_resources paths.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/project_env.rs`:
- Around line 195-231: The project mode/hostname update in
synchronize_project_link_state can fail after resource and PHP-runtime state has
been committed, leaving persisted state inconsistent. Make the final
mode/hostname commit and PHP-runtime/environment snapshot writes atomic within
one transaction, or explicitly roll back/reconcile those writes when
synchronize_project_link_state fails; preserve Gateway reconciliation only after
the combined operation succeeds.
---
Outside diff comments:
In `@crates/config/src/parser.rs`:
- Around line 261-281: Update validate_env_file so it rejects paths whose final
normalized depth is zero, including "." and paths such as "directory/..", while
preserving existing absolute-path and project-escape errors. Add parser tests
covering these root-resolving paths and confirming valid env-file paths remain
accepted.
---
Nitpick comments:
In `@crates/config/src/parser.rs`:
- Around line 267-277: Update the parser module imports to add a top-level use
of camino::Utf8Component, then replace the fully qualified camino::Utf8Component
variants in the env_file component match with Utf8Component variants. Preserve
the existing path validation behavior and error handling.
In `@crates/daemon/src/project_env.rs`:
- Around line 197-217: Import
reconcile_project_resources_with_catalog_and_progress and
reconcile_project_resources_with_progress at the module level, then call both
functions directly in the resource_result branch instead of using fully
qualified crate::managed_resources paths.
🪄 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: dfd1e4ad-2239-4873-9095-7afddeed6691
⛔ Files ignored due to path filters (4)
crates/config/tests/snapshots/project_config__project_config_parses_resource_only_controls_and_defaults.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_config__project_config_writer_preserves_resource_only_controls.snapis excluded by!**/*.snapcrates/config/tests/snapshots/project_env__project_env_renderer_returns_empty_output_for_no_mappings.snapis excluded by!**/*.snapit/snapshots/cli__project_list_shows_project_and_mode_for_served_and_resource_only_projects.snapis excluded by!**/*.snap
📒 Files selected for processing (12)
crates/cli/src/commands/project.rscrates/cli/tests/list_json.rscrates/config/src/discovery.rscrates/config/src/env.rscrates/config/src/parser.rscrates/config/tests/project_config.rscrates/config/tests/project_env.rscrates/daemon/src/gateway.rscrates/daemon/src/project_env.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/project_env_reconciliation.rsit/cli.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/daemon/tests/gateway_reconciliation.rs
- it/cli.rs
- crates/config/src/env.rs
- crates/config/src/discovery.rs
- crates/config/tests/project_config.rs
- crates/daemon/src/gateway.rs
- crates/daemon/tests/project_env_reconciliation.rs
- crates/cli/src/commands/project.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 343e20c018
ℹ️ 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(track) = project.php_runtime.track.clone() { | ||
| let uses_project_runtime = config_file.config.serve || config_file.config.php.is_some(); |
There was a problem hiding this comment.
When a served Project with a persisted PHP runtime is edited to serve: false without an explicit php: selection and the candidate config has a semantic error that read_from_root does not detect (for example, duplicate rendered env keys), this condition becomes false and the PHP/Composer shim falls back to the global runtime. Daemon reconciliation subsequently rejects the transition and preserves the last valid served state, so commands run under a different PHP version than that state requires; base shim selection on the reconciled mode or fully validate the candidate before applying its mode semantics.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Important
No new issues were found in 343e20c, and the transition-state feedback is addressed. The previously reported migration backfill issue remains unresolved, so this review does not approve the PR.
Reviewed changes — I reviewed the atomic transition finalization and CLI validation changes added since the prior Pullfrog review.
- Atomically finalized Project state — Combined serving mode, hostname, PHP runtime, and env observation updates in one immediate SQLite transaction.
- Preserved the fallback PHP runtime — Deferred candidate runtime persistence until all preceding reconciliation work succeeds.
- Validated relinks before mutation — Checked rendered env-key conflicts before
pv linkcan replace the persisted serving mode. - Stabilized Project selection — Sorted served Projects by primary hostname before presenting the interactive picker.
- Added rollback coverage — Exercised transaction failure and post-PHP-resolution transition failure paths to verify prior database state remains intact.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/daemon/src/project_env.rs (1)
578-578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
desired_php_track: Noneis silently overwritten by the state layer.
Database::finalize_project_reconciliationsetsinput.link.desired_php_trackfromphp_runtime(crates/state/src/database.rs:868-871), so thisNonenever reaches the database. A brief comment would prevent a reader from concluding the track is cleared here.🤖 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/project_env.rs` at line 578, Add a brief explanatory comment directly above desired_php_track: None in the relevant project reconciliation input, noting that the state layer’s Database::finalize_project_reconciliation overwrites it from php_runtime, so this value does not clear the persisted track.crates/state/src/database.rs (1)
1986-1990: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNaming reads as "outside a transaction" while it is also used inside one.
replace_project_php_runtime_in_connectionis called with&transactionat Line 903 (viaDeref<Target = Connection>), which is correct, but the_in_connectionsuffix conflicts with the_in_transactionconvention used everywhere else in this module and obscures that the write joins the caller's transaction. Considerreplace_project_php_runtime_in(...)or a short doc comment stating it participates in any active transaction.🤖 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 1986 - 1990, Rename replace_project_php_runtime_in_connection to replace_project_php_runtime_in to reflect that it operates on the caller-provided database handle and can participate in an active transaction. Update all call sites, including the invocation through the transaction at Line 903, while preserving the existing behavior.crates/daemon/tests/project_env_reconciliation.rs (1)
213-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePermission restore is skipped if reconciliation returns
Err.
run_project_reconciliation(...)?on Line 213 propagates beforeset_file_mode(&locked_directory, 0o700), leaving a0o500directory behind and potentially failing tempdir cleanup. Capturing the result first and restoring the mode before unwrapping keeps failures readable.♻️ Restore before propagating
- let lines = run_project_reconciliation(&paths, &project).await?; - set_file_mode(&locked_directory, 0o700)?; + let reconciliation = run_project_reconciliation(&paths, &project).await; + set_file_mode(&locked_directory, 0o700)?; + let lines = reconciliation?;🤖 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` around lines 213 - 214, Update the test flow around run_project_reconciliation to capture its result instead of propagating immediately, restore locked_directory permissions with set_file_mode(..., 0o700), then unwrap or propagate the captured reconciliation result. Preserve the existing successful lines handling and ensure permission restoration occurs on both success and error paths.
🤖 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/daemon/src/project_env.rs`:
- Line 578: Add a brief explanatory comment directly above desired_php_track:
None in the relevant project reconciliation input, noting that the state layer’s
Database::finalize_project_reconciliation overwrites it from php_runtime, so
this value does not clear the persisted track.
In `@crates/daemon/tests/project_env_reconciliation.rs`:
- Around line 213-214: Update the test flow around run_project_reconciliation to
capture its result instead of propagating immediately, restore locked_directory
permissions with set_file_mode(..., 0o700), then unwrap or propagate the
captured reconciliation result. Preserve the existing successful lines handling
and ensure permission restoration occurs on both success and error paths.
In `@crates/state/src/database.rs`:
- Around line 1986-1990: Rename replace_project_php_runtime_in_connection to
replace_project_php_runtime_in to reflect that it operates on the
caller-provided database handle and can participate in an active transaction.
Update all call sites, including the invocation through the transaction at Line
903, while preserving the existing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 068cee83-7bca-4178-939c-99f9096758f8
⛔ Files ignored due to path filters (2)
crates/cli/tests/snapshots/project_open__link_rejects_invalid_mode_change_without_replacing_served_state.snapis excluded by!**/*.snapcrates/cli/tests/snapshots/project_open__open_project_picker_sorts_projects_by_primary_hostname.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
crates/cli/src/commands/project.rscrates/cli/tests/project_open.rscrates/daemon/src/project_env.rscrates/daemon/tests/project_env_reconciliation.rscrates/state/src/database.rscrates/state/src/lib.rscrates/state/tests/state_foundation.rs
There was a problem hiding this comment.
Important
The new fallback is still asymmetric for failed resource-only-to-served transitions, so the last valid explicit PHP runtime can change before the transition succeeds.
Reviewed changes since the prior Pullfrog review, I reviewed the PHP shim fallback correction and its regression coverage.
- Preserved served runtime fallback — Changed shim runtime selection to consult persisted served mode when a candidate
serve: falseedit has not reconciled successfully. - Covered failed disable transitions — Added a CLI test proving the persisted project runtime wins over the global default after semantic validation rejects a served-to-resource-only transition.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found in
f4e8d4d. The existing migration slug-backfill finding remains open from the prior review.
Reviewed changes since the prior Pullfrog review, I reviewed the PHP shim runtime-retention correction and its regression coverage.
- Retained persisted Project runtimes — Removed the asymmetric mode/config gate so shims keep the last reconciled explicit runtime through failed or pending transitions in either direction.
- Covered failed enable transitions — Added a focused test proving a resource-only Project retains its explicit runtime when an invalid served transition removes
php. - Preserved successful global fallback — Kept the existing empty-runtime path so successfully reconciled resource-only Projects without explicit PHP continue to use the global runtime.
GPT Sol ｜ 𝕏
1 participant
Summary
serve: falseProjects that continue reconciling Managed Resources, allocations, and env without Gateway routes, TLS demand, or PHP workersenv_filetargets while preserving prior PV-managed blocks on target changes and unlinkpv openfor resource-only Projects, and keep explicit PHP available to CLI shims without creating a workerProduct impact
Projects can now use PV purely for local services and generated environment values. Switching
serveoff preserves dormant serving configuration and hostname reservations, so switching it back on does not require deleting and recreating config. Newly generated Resource allocation names remain stable and readable even when a Project is never assigned a hostname.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace --no-fail-fast -j 4 --status-level fail --final-status-level fail— 1,080 passed, 6 environment-gated tests skippedgit diff --check.snap.newfilesSummary by CodeRabbit
serve: false) for resources and runtimes without gateway routing, worker demand, or hostname reservations.env_filesupport, and stable slug-based resource names.list,status,project:env,open,unlink, andlinkto distinguish served and resource-only Projects.