fix(daemon): verify structured process identity - #310
Conversation
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe platform adds process identity inspection, including macOS argument and start-time discovery. The daemon persists this identity in runtime metadata and requires it for ownership verification, adoption, reload, and stop signaling, with expanded forgery and malformed-metadata tests. ChangesProcess Identity Ownership
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Supervisor
participant Platform
participant RuntimeMetadata
participant ManagedProcess
Supervisor->>RuntimeMetadata: Read persisted process_start_identity
Supervisor->>Platform: Inspect live process identity by PID
Platform-->>Supervisor: Return executable, arguments, and start identity
Supervisor->>ManagedProcess: Compare live identity with runtime metadata
ManagedProcess-->>Supervisor: Return ownership match
Supervisor->>ManagedProcess: Signal only when ownership matches
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: 51cad2d97a
ℹ️ 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 Some(process_start_identity) = metadata.process_start_identity else { | ||
| return Ok(None); | ||
| }; |
There was a problem hiding this comment.
After upgrading from the immediately preceding release, every still-running runtime has metadata without process_start_identity, because that field did not previously exist and self-update deliberately leaves child processes running. Returning None here (and likewise in adopt_recorded) makes the new daemon reject all of those runtimes; the gateway path then sees an unverifiable active listener and errors, while Managed Resources cannot be adopted or restarted cleanly. Add a safe one-time compatibility path that verifies legacy metadata and records the observed native identity so normal post-update reconciliation continues.
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
The new identity requirement needs an upgrade transition for runtime metadata written by the previous binary.
Reviewed changes — This PR replaces display-text process ownership checks with structured macOS identity snapshots and native PID-reuse protection before adoption or signaling.
- Add native process identity inspection —
platformreads ordered arguments throughKERN_PROCARGS2and process-start timestamps throughproc_pidinfo, while unsupported targets fail explicitly. - Strengthen daemon ownership checks —
ProcessSupervisorrequires exact executable or script semantics, ordered arguments, metadata, and the recorded native start identity. - Reverify destructive actions — Adopted runtimes are checked again before reload signaling or process-group termination.
- Expand process supervision coverage — macOS integration tests cover direct executables, shebang scripts, spoofed argument zero, reordered or duplicated arguments, malformed metadata, and forged start timestamps.
GPT Sol ｜ 𝕏
| }; | ||
|
|
||
| if metadata.matches(spec, pid) && live_process_matches_spec(pid, spec)? { | ||
| let Some(process_start_identity) = metadata.process_start_identity else { |
There was a problem hiding this comment.
Existing runtime metadata has no process_start_identity, while app updates restart only the daemon and deliberately leave managed children running. This makes the first reconciliation reject every pre-upgrade runtime as unowned, then fail on its occupied listener, so the upgrade can leave the Gateway, workers, and Managed Resources running without an adoption path; please add a safe metadata transition or stop old children before the new verifier takes over.
Technical details
# Pre-upgrade runtimes cannot transition to structured identity
## Affected sites
- `crates/daemon/src/supervisor.rs:221` — `verify_ownership` rejects all metadata written before this field existed.
- `crates/daemon/src/supervisor.rs:263` — `adopt_recorded` has the same unconditional rejection, so stale or changed runtimes cannot be stopped either.
- `crates/daemon/src/lib.rs:237` — daemon shutdown stops the socket and DNS task but does not stop supervised child processes.
- `crates/cli/src/commands/update.rs:523` — app activation kickstarts the new daemon, then reexecutes the Managed Resource update and reconciliation phase.
## Required outcome
- A normal update from the immediately previous release must either safely adopt existing managed children under the new identity model or stop them before the new daemon reconciles.
- After the transition, all later ownership and signaling decisions must retain the new start-identity requirement.
## Suggested approach
Either perform a one-time structured executable/argument verification before backfilling the observed native start identity, or sequence app update so the old daemon terminates its managed children before activating the new verifier.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/daemon/src/supervisor.rs (1)
822-843: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the script-file read when the direct match already succeeds.
script_command_matcheseagerly callsfs::read_to_string(command)on every ownership check — including the common direct-executable case where the result is discarded.verify_ownershipruns on reconciliation paths, so this is avoidable disk I/O proportional to script size.♻️ Short-circuit the script branch
fn process_identity_matches( process_identity: &platform::ProcessIdentity, command: &Utf8Path, arguments: &[String], ) -> bool { let direct_arguments_match = process_identity.arguments == arguments; let direct_command_matches = process_identity.executable == command || (command == Utf8Path::new("/bin/sh") && process_identity.executable == Utf8Path::new("/bin/bash") && process_identity.argument_zero == command.as_str()); + if direct_command_matches && direct_arguments_match { + return true; + } + let script_command_matches = process_identity .arguments .first() .is_some_and(|script| script == command.as_str()) && fs::read_to_string(command).is_ok_and(|source| source.starts_with("#!")); let script_arguments_match = process_identity .arguments .get(1..) .is_some_and(|live_arguments| live_arguments == arguments); - (direct_command_matches && direct_arguments_match) - || (script_command_matches && script_arguments_match) + script_command_matches && script_arguments_match }🤖 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/supervisor.rs` around lines 822 - 843, Update process_identity_matches so the script-specific checks are evaluated only after the direct command-and-arguments match fails, preserving the existing direct match result while avoiding fs::read_to_string(command) for successful direct matches. Keep the current script matching behavior unchanged for non-direct matches.crates/platform/src/process/unsupported.rs (1)
4-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
Err(...?)wrapping.Since
unsupported(...)already returnsResult<T, PlatformError>for anyT, this can be returned directly without theErr(...?)indirection (which makes the outerErr(...)unreachable in practice onceunsupportedreturns its error).♻️ Proposed simplification (verify against sibling unsupported shims for consistency first)
pub(super) fn inspect_process_identity( _pid: u32, ) -> Result<Option<ProcessIdentity>, PlatformError> { - Err(unsupported(PlatformCapability::ProcessInspection)?) + unsupported(PlatformCapability::ProcessInspection) }🤖 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/platform/src/process/unsupported.rs` around lines 4 - 8, Update inspect_process_identity to return unsupported(PlatformCapability::ProcessInspection) directly, removing the redundant Err wrapper and question-mark propagation while preserving the existing Result<Option<ProcessIdentity>, PlatformError> signature and unsupported behavior.crates/platform/src/process.rs (1)
39-41: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a top-level import instead of the fully qualified
crate::PlatformError.
macos.rsandunsupported.rsbothuse crate::PlatformError;at the top and reference it unqualified; this function inlines the fully qualified path instead.As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust."
♻️ Proposed fix
+use crate::PlatformError; + pub fn inspect_process_identity(pid: u32) -> Result<Option<ProcessIdentity>, crate::PlatformError> { +pub fn inspect_process_identity(pid: u32) -> Result<Option<ProcessIdentity>, PlatformError> { implementation::inspect_process_identity(pid) }🤖 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/platform/src/process.rs` around lines 39 - 41, Update inspect_process_identity to import PlatformError at the module level and use the unqualified PlatformError in its return type, matching the style used by macos.rs and unsupported.rs.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.
Nitpick comments:
In `@crates/daemon/src/supervisor.rs`:
- Around line 822-843: Update process_identity_matches so the script-specific
checks are evaluated only after the direct command-and-arguments match fails,
preserving the existing direct match result while avoiding
fs::read_to_string(command) for successful direct matches. Keep the current
script matching behavior unchanged for non-direct matches.
In `@crates/platform/src/process.rs`:
- Around line 39-41: Update inspect_process_identity to import PlatformError at
the module level and use the unqualified PlatformError in its return type,
matching the style used by macos.rs and unsupported.rs.
In `@crates/platform/src/process/unsupported.rs`:
- Around line 4-8: Update inspect_process_identity to return
unsupported(PlatformCapability::ProcessInspection) directly, removing the
redundant Err wrapper and question-mark propagation while preserving the
existing Result<Option<ProcessIdentity>, PlatformError> signature and
unsupported behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e51485b2-0e3b-4c51-8dc0-4e1d0b950cdd
⛔ Files ignored due to path filters (3)
crates/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/tests/snapshots/supervisor_foundation__supervisor_captures_logs_and_runtime_metadata_then_stops_child.snapis excluded by!**/*.snap
📒 Files selected for processing (13)
crates/daemon/src/error.rscrates/daemon/src/managed_resources/mysql_tests.rscrates/daemon/src/supervisor.rscrates/daemon/tests/supervisor_foundation.rscrates/platform/src/ca.rscrates/platform/src/capability.rscrates/platform/src/error.rscrates/platform/src/lib.rscrates/platform/src/process.rscrates/platform/src/process/macos.rscrates/platform/src/process/unsupported.rscrates/platform/tests/process_identity.rscrates/platform/tests/unsupported_process_identity.rs
There was a problem hiding this comment.
Important
No new issues were found in c6481d3. The existing open upgrade-transition thread still requires resolution before merge.
Reviewed changes — The follow-up commit tightened script-backed process ownership and made unavailable macOS argument snapshots fail closed.
- Pinned shebang interpreter identity — Script startup now waits for a stable process identity and records the observed interpreter executable and
argv[0]for later ownership checks. - Handled unavailable argument snapshots —
KERN_PROCARGS2returningEINVALnow reports no observable identity instead of escalating an inspection error. - Expanded regression coverage — Supervisor tests now reject forged script interpreters and spoofed
argv[0], while runtime snapshots include the interpreter identity.
GPT Sol ｜ 𝕏
1 participant
Why
PV previously verified process ownership by running
/bin/psand parsing its human-readable command display. That loses argument boundaries and makes ownership checks vulnerable to ordering, multiplicity, prefix, and PID-reuse mistakes.What changed
platformprocess-identity API containing the executable,argv[0], ordered arguments, and native process-start identityKERN_PROCARGS2andproc_pidinfo, including a stable before/after start-identity check/usr/bin/env python3), and the existing macOS/bin/shcompatibility case/bin/psinvocation and display-text tokenizerPID-reuse protection
Runtime metadata now records the process start timestamp returned by the native macOS process API. A live PID is trusted only when that native identity matches the recorded value, so a recycled PID with otherwise plausible metadata is rejected.
Verification
cargo fmt --all -- --checkcargo nextest run -p platform— 53 passedcargo nextest run -p daemon --test supervisor_foundation— 27 passed, 1 ignoredcargo nextest run -p daemon --no-fail-fast— 263 passed, 6 ignoredcargo clippy --workspace --all-targets --all-features --locked -- -D warnings/bin/psinvocation or obsolete command-line parsercargo shearwas not run because there are no dependency or manifest changes.Limitations
macOS remains the only process-inspection implementation. Linux and Windows return the existing explicit unsupported error. A local Linux cross-check was attempted, but this macOS host lacks
x86_64-linux-gnu-gcc, which is required byringand bundled SQLite before project code can compile; native CI is expected to provide the non-macOS compile evidence.Summary by CodeRabbit