Repository navigation
Conversation
PV 0.2.6 moved every runtime onto monitors and kept the pid-file code only to stop runtimes an older release left running. Once every install runs 0.2.6, that code has nothing left to do, so it goes: - the legacy stop and its pid-file identity checks (argv, script and executable matching) in the supervisor - the cutover that every reconciliation and update job ran first - the pid-file branch of disable and uninstall, which now remove only JSON records that no monitor backs - the pid_path field of ProcessSpec and ManagedProcess, the pid path helpers in PvPaths, and adopt_recorded's unused pid argument - RuntimeMetadata's boot_session_id, process_start_identity and process_executable_identity fields. Old records still parse, since serde ignores the keys. A record without a monitor instance no longer counts as config proof. Releases after 0.2.6 must require 0.2.6, so `pv update` can't skip the cutover; DESIGN.md now says so. CONTRIBUTING.md describes fixtures under monitors and drops the script-identity tests.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📝 Walkthrough
Merge Risk: 🟡 Moderate · up to A release can let older installations skip the runtime migration, and the contributor guide overpromises cleanup after test-process exit. Enforce the release floor and clarify the cleanup guarantee before merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review details
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 69.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 123 functions across 11 files. (4 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: removing pid-file runtime adoption from the daemon. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 69.11% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 123 functions across 11 files. (4 skipped: 4 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
![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
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
CONTRIBUTING.md (1)
61-62: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDo not document SIGKILL cleanup as unconditional.
The monitor attempts bounded process-group cleanup and exits, but cleanup can become
Unprovenif the group does not exit. In that case, the monitor keeps its records and may leave a runtime running.Suggested documentation change
-Whenever the test process exits, even by SIGKILL, its lifeline closes and every monitor it started stops its runtime and -exits. A failed test therefore leaves nothing running. +Whenever the test process exits, even by SIGKILL, its lifeline closes and every monitor it started attempts to stop its +runtime and exits. If cleanup cannot prove that the process group is empty, the monitor keeps its records for recovery.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
0d92e9dc-175e-4d3f-a800-c9d0d5ffde34
⛔ Files ignored due to path filters (2)
crates/daemon/tests/snapshots/gateway_reconciliation__caddy_cli_command_and_process_specs_are_stable.snapis excluded by!**/*.snapcrates/daemon/tests/snapshots/supervisor_foundation__process_spec_debug_omits_empty_private_environment_and_redacts_values.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
CONTRIBUTING.mdDESIGN.mdcrates/cli/tests/daemon.rscrates/cli/tests/support/runtime.rscrates/daemon/src/gateway.rscrates/daemon/src/health.rscrates/daemon/src/jobs.rscrates/daemon/src/managed_resources/fake.rscrates/daemon/src/managed_resources/mailpit.rscrates/daemon/src/managed_resources/mod.rscrates/daemon/src/managed_resources/mysql.rscrates/daemon/src/managed_resources/postgres.rscrates/daemon/src/managed_resources/redis.rscrates/daemon/src/managed_resources/runtime_contracts.rscrates/daemon/src/managed_resources/rustfs.rscrates/daemon/src/managed_resources/tests.rscrates/daemon/src/runtime_stop.rscrates/daemon/src/structured_log.rscrates/daemon/src/supervisor.rscrates/daemon/test-fixtures/supervisor/owned-python-runtime.pycrates/daemon/tests/daemon_foundation.rscrates/daemon/tests/gateway_reconciliation.rscrates/daemon/tests/supervisor_foundation.rscrates/state/src/paths.rscrates/state/tests/state_foundation.rs
💤 Files with no reviewable changes (12)
- crates/daemon/src/managed_resources/mailpit.rs
- crates/daemon/src/managed_resources/redis.rs
- crates/daemon/src/structured_log.rs
- crates/state/src/paths.rs
- crates/daemon/src/managed_resources/mysql.rs
- crates/daemon/src/managed_resources/fake.rs
- crates/daemon/src/managed_resources/mod.rs
- crates/daemon/test-fixtures/supervisor/owned-python-runtime.py
- crates/daemon/src/managed_resources/postgres.rs
- crates/state/tests/state_foundation.rs
- crates/daemon/src/managed_resources/rustfs.rs
- crates/cli/tests/daemon.rs
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The legacy-path deletion introduces a warnings-as-errors build failure on Linux and Windows. Both portability CI jobs fail, and the same failure reproduces locally.
Reviewed changes
Reviewed the complete PR diff and traced monitor ownership, runtime cleanup, config proofs, and the documented release prerequisite.
- Legacy Cutover Removal: Deletes pid-file runtime stopping, pre-job cutover, and legacy process-identity matching.
- Monitor-Only Records: Removes PID-path APIs and legacy metadata fields while retaining monitor-backed adoption and config proofs.
- Shutdown Cleanup: Restricts runtime-record discovery to JSON and preserves monitor shutdown, recovery, and Postgres exit-proof requirements.
- Tests And Documentation: Updates fixtures, path assertions, and snapshots, removes legacy-only tests, and documents the required PV 0.2.6 release transition.
Validation: cargo fmt --all --check and the changed runtime-path integration test passed. RUSTFLAGS="-D warnings" cargo check --locked -p daemon failed with the three diagnostics described inline, matching Linux and Windows CI. macOS lifecycle tests were not run locally because this host is Linux; macOS CI remains in progress.
gpt-6.1-sol ｜ 𝕏
…ilds With the pid-file stop gone, only the macOS monitor stops a process group, so stop_process_group, wait_for_process_group_exit and the Stop signal variant are macOS-only.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed f810dd99 against the previously reviewed baff35ea, with the complete PR diff as context.
- Scoped Process Signals: Gated
ProcessSignal::Stopto macOS, where its remaining constructors and match arms are compiled. - Restored Portable Builds: Gated
stop_process_groupandwait_for_process_group_exitto macOS, matching their remaining monitor callers without changing macOS shutdown behavior.
Validation: RUSTFLAGS="-D warnings" cargo check --locked -p daemon and cargo fmt --all --check passed locally. Linux and Windows portability CI both pass. macOS CI remains in progress; native lifecycle tests were not run on this Linux host because the delta only changes platform compilation guards.
gpt-6.1-sol ｜ 𝕏
Only 0.2.6 moves runtimes from pid files onto monitors, so a later release that allows an older installed PV would let `pv update` skip that step. The release workflow's minimum_pv_version now defaults to 0.2.6 instead of 0.1.0, and the release skill says never to go below it. CONTRIBUTING.md no longer promises that a monitor always proves its runtime's group gone when a test's lifeline closes.
|
@coderabbitai on your CONTRIBUTING.md note about test cleanup: agreed, fixed in e9a1bb6. The fixture-lifecycle text now says that when a test's lifeline closes, each monitor stops its runtime's process group and exits. If a monitor can't prove the group is gone, it keeps its records for recovery, so check for leftover processes after an unexplained failure. It no longer promises that a failed test always leaves nothing running. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed e9a1bb64 against the previously reviewed f810dd99, with the complete PR diff as context.
- Raised the release minimum default to
0.2.6and aligned the release instructions and design documentation with the required monitor cutover. - Clarified fixture cleanup guarantees to describe process-group cleanup and retained recovery records when shutdown cannot be proven.
Validation: cargo test --locked -p pv-release --test workflow_defaults -p self-update --test app_update_manifest passed. Traced the release input into the generated manifest and verified the updater rejects an unmet minimum version. Native macOS lifecycle tests were not rerun for this configuration/documentation-only delta; macOS and Windows CI remain in progress, while Linux CI passes.
gpt-6.1-sol ｜ 𝕏
![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: e9a1bb6408
ℹ️ 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".
| description: "Minimum installed PV version that may apply this app release; at least 0.2.6, which moves pid-file runtimes onto monitors" | ||
| required: true | ||
| default: "0.1.0" | ||
| default: "0.2.6" |
There was a problem hiding this comment.
Because workflow_dispatch inputs are editable, a post-0.2.6 release can still be dispatched with minimum_pv_version=0.2.5: the prepare step only rejects an empty value and publishes every other string unchanged. This lets a pre-0.2.6 installation skip the sole pid-file-to-monitor migration after this commit removes the legacy-runtime stop path, leaving existing runtimes unmanaged or causing reconciliation to collide with their occupied ports. Enforce a parsed minimum of 0.2.6 in the workflow rather than relying on the default.
AGENTS.md reference: AGENTS.md:L4-L4
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
CONTRIBUTING.md (1)
61-62: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the unconditional cleanup guarantee.
Line 61 says every monitor stops its process group and exits when the test process exits. Line 62 allows a monitor to be unable to verify that the group is gone. Describe cleanup as an attempt, not a guaranteed result.
Suggested wording
-Whenever the test process exits, even by SIGKILL, its lifeline closes and every monitor it started stops its runtime's process group and exits. +When the test process exits, its lifeline closes. Each monitor attempts to stop its runtime's process group. If it cannot verify that the group is gone, PV keeps its records for recovery.The PR objective says lifeline closure must not be documented as proof that the runtime group is gone.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: QUIET
- Plan: Advanced
- Run ID:
c7e1e757-553f-4a57-a047-496b8f141510
📒 Files selected for processing (4)
.agents/skills/release-app-version/SKILL.md.github/workflows/app-release.ymlCONTRIBUTING.mdDESIGN.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| description: "Minimum installed PV version that may apply this app release; at least 0.2.6, which moves pid-file runtimes onto monitors" | ||
| required: true | ||
| default: "0.1.0" | ||
| default: "0.2.6" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n -C 10 'minimum_pv_version|0\.2\.6|app_version|app-version|::error::' .github/workflows/app-release.ymlRepository: prvious/pv
Length of output: 7035
🏁 Script executed:
set -e
printf '%s\n' '--- workflow metadata and release-record blocks ---'
nl -ba .github/workflows/app-release.yml | sed -n '42,170p'
nl -ba .github/workflows/app-release.yml | sed -n '250,390p'
printf '%s\n' '--- migration-floor references ---'
rg -n -C 6 -F -- 'minimum_pv_version' .github DESIGN.md CONTRIBUTING.md scripts src 2>/dev/null || true
rg -n -C 5 -F -- '0.2.6' .github DESIGN.md CONTRIBUTING.md scripts src 2>/dev/null || trueRepository: prvious/pv
Length of output: 36573
🏁 Script executed:
set -e
printf '%s\n' '--- write-app-release-record definition and callers ---'
rg -n -C 8 -F -- 'write-app-release-record' . --glob '!target/**' --glob '!node_modules/**'
printf '%s\n' '--- minimum-version consumers in release tooling ---'
rg -n -C 8 -F -- 'minimum-pv-version' crates .github --glob '!target/**'
rg -n -C 8 -F -- 'minimum_pv_version' crates --glob '!target/**'Repository: prvious/pv
Length of output: 5746
🏁 Script executed:
set -e
rg -n -F -- 'write-app-release-record' crates/pv-release
rg -n -F -- 'minimum_pv_version' crates/pv-release
printf '%s\n' '--- pv-release source outline/files ---'
rg --files crates/pv-releaseRepository: prvious/pv
Length of output: 24901
🏁 Script executed:
set -e
printf '%s\n' '--- CLI command declarations and dispatch ---'
nl -ba crates/pv-release/src/cli.rs | sed -n '120,205p'
nl -ba crates/pv-release/src/cli.rs | sed -n '320,415p'
printf '%s\n' '--- record writer ---'
nl -ba crates/pv-release/src/record_writer.rs | sed -n '1,155p'
printf '%s\n' '--- version type definition and comparison support ---'
rg -n -C 8 -F -- 'struct PvVersion' crates/pv-release/src crates/pv-version/src 2>/dev/null || true
rg -n -C 5 -F -- 'enum PvVersion' crates/pv-release/src crates/pv-version/src 2>/dev/null || trueRepository: prvious/pv
Length of output: 12417
🏁 Script executed:
set -e
printf '%s\n' '--- app release request and writer ---'
nl -ba crates/pv-release/src/app.rs | sed -n '300,445p'
nl -ba crates/pv-release/src/app.rs | sed -n '640,705p'
printf '%s\n' '--- release-record parser ---'
nl -ba crates/pv-release/src/record.rs | sed -n '190,280p'
printf '%s\n' '--- app version type and parser references ---'
rg -n -C 8 -F -- 'struct AppUpdateVersion' crates/pv-release/src
rg -n -C 8 -F -- 'enum AppUpdateVersion' crates/pv-release/src
rg -n -C 5 -F -- 'AppUpdateVersion::parse' crates/pv-release/srcRepository: prvious/pv
Length of output: 10531
🏁 Script executed:
set -e
nl -ba crates/pv-release/src/app.rs | sed -n '445,650p'Repository: prvious/pv
Length of output: 7844
Enforce the migration floor for later releases.
minimum_pv_version is caller-controlled. The workflow checks only that it is non-empty, and the app release writer stores it unchanged. AppReleaseRecord::from_json validates only that both versions have valid syntax; it does not compare them. A normal app release after 0.2.6 can therefore be published with a lower minimum and let pv update skip the migration. Add a version-aware check that rejects values below 0.2.6 for later releases while preserving the migration release's required minimum.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd --type f '^CONTRIBUTING\.md$' --exec cat -n {}Length of output: 9547
✏️ Learnings added
You are interacting with an AI system. |
1 participant
PR 5 of the monitor plan: remove the pid-file runtime code. PV 0.2.6 moved every runtime onto monitors. It kept the pid-file code only for the cutover, which stops runtimes an older release left running. Once every install runs 0.2.6, nothing needs it.
Important
Merge any time; release only after the other laptop runs 0.2.6 and passes the three checks: no pid files, monitors in
~/.pv/run/m/, nolegacy_runtime_stop_failed. Release withminimum_pv_version = 0.2.6.pv updaterefuses a manifest that requires a newer PV than the one installed, so a 0.2.5 install can't skip the cutover release.What's removed
stop_legacy_runtimeand the pid-era identity checks in the supervisor: argv, script and executable matching,process_and_group_are_absent, andremove_unchanged_runtime_records.legacy_runtime_stop_failedlog event.ProcessSpec.pid_pathandManagedProcess.pid_pathgateway_pid,worker_pidandresource_pidhelpers inPvPathsadopt_recorded's unused pid argument, nowadopt_recorded(metadata_path)RuntimeMetadata'sboot_session_id,process_start_identityandprocess_executable_identity(your decision, 2026-10-11). The struct has nodeny_unknown_fields, so old records still parse; serde ignores the keys. Monitor records keep their own boot ID.Docs
minimum_pv_versionis still0.1.0, and the release skill makes us confirm it every time.pv_fake::install_monitor, cleanup throughstop_recorded_runtimesandstop_subject, and the test lifeline as the backstop.python3note.Tests
jobs.rsand the startup one indaemon_foundation.rsremove_unchanged_runtime_recordsgateway_reconciliation.rs. Two snapshots lose theirpid_pathlines; I checked that this was the only change in each.daemon_disable_stops_live_runtimes_without_a_plist_or_database, and the monitor prior-boot Postgres disable test stays.Test plan
cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo fmt --all --checkandcargo shearare clean.daemon_foundation,gateway_reconciliation,supervisor_foundationandcli::daemontogether,--stress-count 5. One failed iteration on this branch and one on main (3cd321f7), in different tests, so the flake rate is the same.gateway_reconciliationalone passed 8 out of 8 runs.gateway_reconciliation_replaces_legacy_admin_off_process_before_admin_contact: the replacement Gateway exited immediately. The test frees its reserved ports just before starting, so a parallel test can take one; that's the known port race in test: parallel tests race each other for the same ports #380.Summary by CodeRabbit