feat(cli): rename pf config to pf daemons and add pf settings subcommand#471
Merged
Conversation
…xtract shared path resolution - Rename pf config -> pf daemons (alias daemon), remove old cfg alias - Rename src/cli/config/ -> src/cli/daemons/, struct Config -> Daemons - pf daemons (no subcommand) now lists configured daemons (id + run) - Add pf settings subcommand (list, get, set) with --local/--project/--global - Add enum validation for log_level and log_file_level settings - Add --local/--project flags to daemons add and daemons remove - Extract shared resolve_project_config_path into src/cli/daemons/mod.rs - Add IpcRequest::ReloadConfig for supervisor config reload after set - Change settings() from OnceLock to RwLock<Option<Settings>> for reload - Add SettingsLogsPartial with has_any_set/is_empty for log retention - Fix clippy: unwrap_or_default in logs.rs, Box<Add> in DaemonsCommand - Update tests: pitchfork config -> pitchfork daemons - Regenerate docs via mise run render
gaojunran
changed the title
pf config to pf daemons and add pf settings subcommandpf config to pf daemons and add pf settings subcommand
|
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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR refactors the settings infrastructure to support runtime reload, consolidates daemon and settings management under new CLI commands, and adapts the codebase to work with Arc-backed cached settings passed by reference instead of static references. ChangesSettings reload and CLI command consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Greptile SummaryThis PR renames the
Confidence Score: 5/5Safe to merge — the two issues flagged in earlier review rounds (memory safety in settings reload and the missing logs group handlers) are both addressed in this revision. The settings reload now uses Arc-clone-under-lock so old references are never freed while in use. Both get_setting_value and apply_setting_to_partial handle the logs group. The command rename from config to daemons is mechanical and backed by updated tests. No new unsafe patterns or logic gaps were found. No files require special attention. The new src/cli/settings.rs is the largest addition but is straightforward dispatch code; build/generate_settings.rs carries the memory-safety fix and looks correct. Important Files Changed
Reviews (5): Last reviewed commit: "fix(settings): replace Box::leak with Ar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…-immediate and logs group - Replace unsafe raw pointer in settings() with Box::leak to eliminate use-after-free when reload_settings() replaces the value (PR review P1) - Add --cron-immediate flag to 'daemons add' for cron immediate trigger (PR review P2) - Add 'logs' group handler to get_setting_value and apply_setting_to_partial so 'pf settings get/set logs.*' works correctly
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/settings.rs (1)
43-212: 🏗️ Heavy liftGenerate
is_empty()/has_any_set()instead of hardcoding field listsThese manual checks are tightly coupled to generated partial structs. If a new field is added later and not reflected here, the group can be treated as empty and skipped during serialization even when it has data. Consider generating these methods from
settings.tomlalongside the partial structs.🤖 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 `@src/settings.rs` around lines 43 - 212, The hardcoded is_empty()/has_any_set() implementations (impl SettingsPartialGroup for SettingsPartial, SettingsApiPartial, SettingsGeneralPartial, SettingsIpcPartial, SettingsLogsPartial, SettingsProxyPartial, SettingsSupervisorPartial, SettingsTuiPartial, SettingsWebPartial) risk falling out of sync with generated partial structs when fields are added; replace these manual field-list checks by generating the is_empty() and has_any_set() methods alongside the partial struct generation (from settings.toml) so the generated code inspects each generated Option/field automatically, and update the generator to emit implementations for SettingsPartialGroup (or a derived impl) for each Partial type rather than maintaining the lists by hand.
🤖 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 `@build/generate_settings.rs`:
- Around line 173-201: The current settings() unsafely returns &'static Settings
from SETTINGS: RwLock<Option<Settings>> which can dangle when reload_settings()
replaces/drops the inner value; change the cache to store shared ownership (e.g.
RwLock<Option<Arc<Settings>>>), update settings() to return Arc<Settings> (clone
the Arc for the fast path and when initializing via Settings::load() wrap the
new Settings in Arc), and update reload_settings() to swap in a new
Arc<Settings> instead of replacing a raw Settings so previously handed-out Arcs
remain valid and no unsafe pointer casts are needed.
In `@src/cli/daemons/add.rs`:
- Around line 133-141: Add::run currently performs blocking filesystem I/O
(calls to config_path.exists(), config_path.canonicalize(),
std::fs::create_dir_all(parent), and synchronous PitchforkToml::read /
pt.write), so convert these to async Tokio operations: replace path ops with
tokio::fs equivalents (tokio::fs::create_dir_all, tokio::fs::metadata / exists
pattern, tokio::fs::canonicalize) or wrap the synchronous PitchforkToml::read
and pt.write in tokio::task::spawn_blocking until PitchforkToml is made async;
ensure PitchforkToml::new usage and any code that depends on config_path is
updated to await async calls and, where concurrent IO is needed, coordinate with
tokio::select! to avoid blocking the runtime.
In `@src/cli/daemons/mod.rs`:
- Around line 42-52: The code is performing blocking filesystem I/O inside async
flows; change the sync resolver and checks to async: make
resolve_project_config_path async (e.g., resolve_project_config_path_async) and
convert PitchforkToml::list_paths to an async variant that returns paths without
calling blocking Path::exists; replace all
Path::exists/canonicalize/create_dir_all usages with Tokio async equivalents
(tokio::fs::try_exists or tokio::fs::metadata + .is_ok(),
tokio::fs::canonicalize, tokio::fs::create_dir_all) and update callsites in
add.rs, remove.rs and settings.rs to await these calls and make those functions
async (or use futures::stream / join_all to filter paths concurrently), ensuring
any path filtering logic (the block using exists_filter and
is_project_config_path) uses async checks rather than p.exists().
In `@src/cli/settings.rs`:
- Around line 146-156: SetCmd::run is performing blocking filesystem I/O
(config_path.exists(), std::fs::create_dir_all) and then calls synchronous
PitchforkToml::read / PitchforkToml::write_unlocked which use
std::fs::read_to_string and xx::file::write; change all of these to async to
avoid blocking the tokio runtime: either (A) update PitchforkToml to provide
async equivalents (e.g., async fn read_async / write_async that use
tokio::fs::read_to_string, tokio::fs::create_dir_all and tokio::fs::write), and
replace config_path.exists()/create_dir_all with tokio::fs::metadata/try_exists
or tokio::fs::create_dir_all inside SetCmd::run, or (B) keep PitchforkToml sync
but offload the entire blocking sequence (the calls to PitchforkToml::read /
write_unlocked and any std::fs ops) into tokio::task::spawn_blocking and await
its JoinHandle from SetCmd::run; reference SetCmd::run, PitchforkToml::read,
PitchforkToml::write_unlocked when making the changes.
In `@src/ipc/client.rs`:
- Around line 419-433: The reload_config() implementation currently treats
transport errors from self.request(IpcRequest::ReloadConfig).await as failures,
violating the "best-effort" doc; modify reload_config so that any Err returned
by self.request(...) is handled the same way as IpcResponse::Error (log a
debug/info message like "config reload skipped: {err}" and return Ok(())) and
only treat unexpected successful responses as an Err using
Self::unexpected_response("ConfigReloaded", &rsp).into(); keep references to the
existing IpcResponse::ConfigReloaded and IpcResponse::Error match arms and
update the code path that awaits self.request accordingly.
In `@src/supervisor/ipc_handlers.rs`:
- Around line 116-119: The ReloadConfig branch runs
crate::settings::reload_settings() inline but Settings::load uses blocking
std::fs::read_to_string which can stall Tokio worker threads; update the
IpcRequest::ReloadConfig handling to offload blocking work to a blocking thread
(e.g., use tokio::task::spawn_blocking) and await it before calling
crate::logger::apply_settings(); specifically wrap the call to
crate::settings::reload_settings() (and any heavyweight sync config parsing like
Settings::load) inside spawn_blocking and then call
crate::logger::apply_settings() on the async task’s completion to avoid blocking
the Tokio runtime.
---
Nitpick comments:
In `@src/settings.rs`:
- Around line 43-212: The hardcoded is_empty()/has_any_set() implementations
(impl SettingsPartialGroup for SettingsPartial, SettingsApiPartial,
SettingsGeneralPartial, SettingsIpcPartial, SettingsLogsPartial,
SettingsProxyPartial, SettingsSupervisorPartial, SettingsTuiPartial,
SettingsWebPartial) risk falling out of sync with generated partial structs when
fields are added; replace these manual field-list checks by generating the
is_empty() and has_any_set() methods alongside the partial struct generation
(from settings.toml) so the generated code inspects each generated Option/field
automatically, and update the generator to emit implementations for
SettingsPartialGroup (or a derived impl) for each Partial type rather than
maintaining the lists by hand.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 48e1b132-b3be-484e-b4ce-278c7c2661c2
📒 Files selected for processing (29)
build/generate_settings.rsdocs/cli/commands.jsondocs/cli/config.mddocs/cli/config/remove.mddocs/cli/daemons.mddocs/cli/daemons/add.mddocs/cli/daemons/remove.mddocs/cli/index.mddocs/cli/settings.mddocs/cli/settings/get.mddocs/cli/settings/list.mddocs/cli/settings/set.mddocs/cli/sponsors.mddocs/public/schema.jsonpitchfork.usage.kdlsrc/cli/config/mod.rssrc/cli/config/remove.rssrc/cli/daemons/add.rssrc/cli/daemons/mod.rssrc/cli/daemons/remove.rssrc/cli/mod.rssrc/cli/settings.rssrc/ipc/client.rssrc/ipc/mod.rssrc/settings.rssrc/supervisor/ipc_handlers.rssrc/web/routes/logs.rstest/config.batstest/port.bats
💤 Files with no reviewable changes (5)
- docs/cli/config/remove.md
- docs/cli/sponsors.md
- src/cli/config/mod.rs
- docs/cli/config.md
- src/cli/config/remove.rs
Comment on lines
+42
to
+52
| let paths = PitchforkToml::list_paths(); | ||
| let project_paths: Vec<_> = paths | ||
| .iter() | ||
| .filter(|p| { | ||
| if exists_filter { | ||
| p.exists() && is_project_config_path(p) | ||
| } else { | ||
| is_project_config_path(p) | ||
| } | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all resolver and caller touchpoints before changing signature to async.
# Expected: find resolve_project_config_path definition + all callsites and sync existence/canonicalize usage.
rg -n -C2 'fn resolve_project_config_path|resolve_project_config_path\(' src/cli/daemons/mod.rs src/cli/daemons/add.rs src/cli/daemons/remove.rs src/cli/settings.rs
rg -n -C2 '\.exists\(|\.canonicalize\(|std::fs::create_dir_all' src/cli/daemons/mod.rs src/cli/daemons/add.rs src/cli/daemons/remove.rs src/cli/settings.rsRepository: endevco/pitchfork
Length of output: 3946
Refactor project-config resolution + callers to eliminate synchronous filesystem I/O in async code
resolve_project_config_path in src/cli/daemons/mod.rs is sync, and the resolver pipeline uses p.exists() in PitchforkToml::list_paths() filtering; add/remove and parts of settings also call blocking exists(), canonicalize(), and std::fs::create_dir_all(...) while executing in async command flows.
Current relevant snippet
let paths = PitchforkToml::list_paths();
let project_paths: Vec<_> = paths
.iter()
.filter(|p| {
if exists_filter {
p.exists() && is_project_config_path(p)
} else {
is_project_config_path(p)
}
})
.collect();- Convert the resolver/existence filtering to Tokio async filesystem APIs.
- Update all callsites (
src/cli/daemons/add.rs,src/cli/daemons/remove.rs,src/cli/settings.rs) to replacePath::exists(),Path::canonicalize(), andstd::fs::create_dir_all(...)with Tokio async equivalents so**/*.rsno longer performs blocking I/O in async flows.
🤖 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 `@src/cli/daemons/mod.rs` around lines 42 - 52, The code is performing blocking
filesystem I/O inside async flows; change the sync resolver and checks to async:
make resolve_project_config_path async (e.g., resolve_project_config_path_async)
and convert PitchforkToml::list_paths to an async variant that returns paths
without calling blocking Path::exists; replace all
Path::exists/canonicalize/create_dir_all usages with Tokio async equivalents
(tokio::fs::try_exists or tokio::fs::metadata + .is_ok(),
tokio::fs::canonicalize, tokio::fs::create_dir_all) and update callsites in
add.rs, remove.rs and settings.rs to await these calls and make those functions
async (or use futures::stream / join_all to filter paths concurrently), ensuring
any path filtering logic (the block using exists_filter and
is_project_config_path) uses async checks rather than p.exists().
Source: Coding guidelines
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/cli/daemons/add.rs (1)
132-140:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace blocking FS calls inside
Add::runwith Tokio-compatible I/O.This async path still performs blocking filesystem operations (
exists,create_dir_all,canonicalize, plus syncPitchforkToml::read/pt.write), which can block Tokio worker threads.Suggested direction
- let mut pt = if config_path.exists() { + let mut pt = if tokio::fs::try_exists(&config_path).await? { PitchforkToml::read(&config_path)? } else { if let Some(parent) = config_path.parent() { - std::fs::create_dir_all(parent).map_err(|e| { + tokio::fs::create_dir_all(parent).await.map_err(|e| { miette::miette!( "Failed to create config directory {}: {e}", parent.display() ) })?; } PitchforkToml::new(config_path.clone()) }; @@ - let canonical_path = config_path - .canonicalize() - .unwrap_or_else(|_| config_path.clone()); + let canonical_path = tokio::fs::canonicalize(&config_path) + .await + .unwrap_or_else(|_| config_path.clone());For
PitchforkToml::read/pt.write, move to async internals or isolate viatokio::task::spawn_blockinguntil async APIs exist.As per coding guidelines,
**/*.rs: “All I/O operations must be async using Tokio; usetokio::select!for concurrent operations”. Based on learnings, the same Tokio async-I/O rule applies to**/*.rs.Also applies to: 225-227, 276-276
🤖 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 `@src/cli/daemons/add.rs` around lines 132 - 140, The Add::run async method uses blocking filesystem calls (config_path.exists, std::fs::create_dir_all, std::fs::canonicalize) and synchronous PitchforkToml::read / pt.write which can block Tokio threads; replace these with Tokio-compatible operations by using tokio::fs equivalents (tokio::fs::metadata/is_dir or tokio::fs::try_exists, tokio::fs::create_dir_all, tokio::fs::canonicalize) for path checks/creation and offload PitchforkToml::read and pt.write to tokio::task::spawn_blocking (or make their internals async) so all I/O inside Add::run is non-blocking; apply the same changes to the other occurrences noted around lines 225-227 and 276.Sources: Coding guidelines, Learnings
🤖 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.
Duplicate comments:
In `@src/cli/daemons/add.rs`:
- Around line 132-140: The Add::run async method uses blocking filesystem calls
(config_path.exists, std::fs::create_dir_all, std::fs::canonicalize) and
synchronous PitchforkToml::read / pt.write which can block Tokio threads;
replace these with Tokio-compatible operations by using tokio::fs equivalents
(tokio::fs::metadata/is_dir or tokio::fs::try_exists, tokio::fs::create_dir_all,
tokio::fs::canonicalize) for path checks/creation and offload
PitchforkToml::read and pt.write to tokio::task::spawn_blocking (or make their
internals async) so all I/O inside Add::run is non-blocking; apply the same
changes to the other occurrences noted around lines 225-227 and 276.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3b32890-4928-493c-a94d-4132d8a75faf
📒 Files selected for processing (6)
build/generate_settings.rsdocs/cli/commands.jsondocs/cli/daemons/add.mdpitchfork.usage.kdlsrc/cli/daemons/add.rssrc/cli/settings.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- pitchfork.usage.kdl
- build/generate_settings.rs
- docs/cli/commands.json
- src/cli/settings.rs
…wn_blocking JoinError - Remove SettingsPartialGroup trait and all manual has_any_set/is_empty implementations from src/settings.rs (now generated by codegen) - Add #[allow(dead_code)] to generated has_any_set/is_empty methods - Fix E0277: tokio::task::JoinError doesn't implement miette::Diagnostic - Replace .await?? with .await.into_diagnostic()?.map_err(...) - Add IntoDiagnostic import to add.rs, remove.rs, settings.rs - Clone paths before moving into spawn_blocking closures - Add sponsors command restoration to src/cli/mod.rs - Use spawn_blocking + into_diagnostic in ipc_handlers.rs for ReloadConfig - Make resolve_project_config_path and resolve_config_path async - Convert CLI file I/O to tokio::fs (try_exists, create_dir_all, canonicalize)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
build/generate_settings.rs (1)
169-169: 💤 Low valueDoc comment mentions "RwLock" but actual type differs.
The comment states "RwLock to support runtime reload" but the actual type is
RwLock<Option<&'static Settings>>using aBox::leakapproach. Consider updating the comment to accurately describe the leaky singleton pattern.- /// Global settings instance (RwLock<Arc> to support runtime reload) + /// Global settings instance using leaky singleton pattern for runtime reload🤖 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 `@build/generate_settings.rs` at line 169, Update the doc comment for the global settings instance to reflect the actual implementation: replace the incorrect "RwLock<Arc> to support runtime reload" wording with a description of the leaky singleton pattern used (RwLock<Option<&'static Settings>> created via Box::leak), mentioning that the settings are stored as a leaked &'static reference wrapped in an Option inside an RwLock to allow runtime reload semantics; remove any reference to Arc and explain Box::leak usage and why the Option is present for reload.
🤖 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 `@build/generate_settings.rs`:
- Around line 679-681: The generated has_any_set() function can be empty when
the has_any_set_checks iterator yields no items; update the code generation for
has_any_set to handle the empty case by emitting a default false expression when
#(`#has_any_set_checks`)||* would produce nothing. Concretely, change the
expansion logic that builds pub fn has_any_set(&self) -> bool {
#(`#has_any_set_checks`)||* } so that if has_any_set_checks is empty it emits
"false" (or wraps the joined checks with a fallback like
(#(`#has_any_set_checks`)||* false)), ensuring has_any_set always returns a bool;
adjust the generator that defines has_any_set_checks accordingly.
---
Nitpick comments:
In `@build/generate_settings.rs`:
- Line 169: Update the doc comment for the global settings instance to reflect
the actual implementation: replace the incorrect "RwLock<Arc> to support runtime
reload" wording with a description of the leaky singleton pattern used
(RwLock<Option<&'static Settings>> created via Box::leak), mentioning that the
settings are stored as a leaked &'static reference wrapped in an Option inside
an RwLock to allow runtime reload semantics; remove any reference to Arc and
explain Box::leak usage and why the Option is present for reload.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0925475-d0d2-465c-b43d-6fb123bcd894
📒 Files selected for processing (8)
build/generate_settings.rssrc/cli/daemons/add.rssrc/cli/daemons/mod.rssrc/cli/daemons/remove.rssrc/cli/mod.rssrc/cli/settings.rssrc/ipc/client.rssrc/supervisor/ipc_handlers.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/supervisor/ipc_handlers.rs
- src/ipc/client.rs
- src/cli/daemons/mod.rs
- src/cli/daemons/remove.rs
- src/cli/settings.rs
added 2 commits
June 7, 2026 19:53
…erged in spawn_blocking - Fix potential invalid generated code when has_any_set_checks is empty: use as fallback instead of empty token stream - Wrap PitchforkToml::all_merged() in spawn_blocking in daemons/mod.rs to avoid blocking async runtime with sync filesystem I/O - Use method reference instead of redundant closure (clippy)
…after-free Replace RwLock<Option<&'static Settings>> with RwLock<Option<Arc<Settings>>> and return Arc<Settings> from settings(). This eliminates the use-after-free risk: when reload_settings() replaces the value, old Arc references remain valid until all holders drop them. No more leaked memory on reload. Update all call sites that pass settings() to functions expecting &Settings to use &s (Arc deref) instead of s. Fix E0716 temporary value errors by binding Arc to a let variable before accessing fields.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
daemonscommand for managing daemon configurations (replacesconfig)settingscommand withlist,get, andsetsubcommands for configuration managementdaemons add/removenow support--localand--projectflags to target specific config filessettings setsupports--global,--local, and--projectflags for scope controlDocumentation