Conversation
Summary of ChangesHello @gaojunran, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a significant architectural change by introducing namespacing for daemon IDs. The primary motivation is to resolve potential naming conflicts when users manage multiple projects, each with daemons sharing the same short name. By qualifying daemon IDs with a namespace derived from their configuration file's location, the system can now uniquely identify and manage daemons across different contexts. This refactoring touches many parts of the codebase, from core data structures to CLI interactions and documentation, ensuring a more robust and scalable daemon management experience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR introduces namespace support for daemon IDs to prevent conflicts when multiple projects define daemons with the same name. This is a breaking change that requires deleting the local state file.
Changes:
- Adds a new
DaemonIdtype that combines namespace and name (qualified format:namespace/name) - Updates all daemon ID handling throughout the codebase to use structured IDs
- Implements automatic namespace resolution from directory structure (project daemons) or "global" for system configs
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/daemon_id.rs | New module implementing structured daemon IDs with parsing, validation, and formatting |
| src/pitchfork_toml.rs | Updated to qualify daemon IDs with namespaces during config reading/merging |
| src/daemon.rs | Updated validation to support qualified IDs and path encoding with -- separator |
| src/state_file.rs | Changed to store DaemonId instead of String keys |
| src/error.rs | Added new error variants for daemon ID validation |
| src/supervisor/* | Updated supervisor components to use DaemonId throughout |
| src/ipc/* | Updated IPC protocol and client to handle DaemonId |
| src/cli/* | Updated CLI commands to resolve short IDs to qualified IDs |
| src/web/routes/* | Updated web UI to display and handle namespaced IDs |
| src/tui/* | Updated TUI to display dimmed namespaces |
| tests/test_e2e_namespace.rs | New comprehensive E2E tests for namespace functionality |
| tests/test_pitchfork_toml.rs | Updated to use DaemonId in config tests |
| docs/* | Added namespace documentation and updated configuration references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) (\w+) (.*)$"); | ||
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([\w./-]+) (.*)$"); |
There was a problem hiding this comment.
Duplicate regex pattern update with the same issue. This pattern appears in multiple places for parsing log lines. The pattern [\w./-]+ should be consistent with the qualified daemon ID format and properly validated.
See the same issue at line 183.
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([\w./-]+) (.*)$"); | |
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([A-Za-z0-9_-]+(?:/[A-Za-z0-9_-]+)*) (.*)$"); |
| schemars::json_schema!({ | ||
| "type": "string", | ||
| "description": "Daemon ID in the format 'namespace/name'", | ||
| "pattern": r"^[\w.-]+/[\w.-]+$" |
There was a problem hiding this comment.
The JSON schema pattern is duplicated here with the same issue. Both patterns should be updated to properly exclude -- sequences according to the validation rules.
| "pattern": r"^[\w.-]+/[\w.-]+$" | |
| "pattern": r"^(?!.*--)[\w.-]+/[\w.-]+$" |
| "$ref": "#/$defs/PitchforkTomlDaemon" | ||
| "additionalProperties": false, | ||
| "patternProperties": { | ||
| "^[\\w.-]+/[\\w.-]+$": { |
There was a problem hiding this comment.
The schema uses patternProperties with additionalProperties: false which will reject any daemon entries that don't match the qualified ID pattern ^[\\w.-]+/[\\w.-]+$. However, in actual TOML files, daemon entries use short names (e.g., [daemons.api]), not qualified IDs.
This schema won't validate actual pitchfork.toml files correctly. The pattern should match short daemon names, not qualified IDs, since qualification happens during reading/merging, not in the source TOML files.
| "^[\\w.-]+/[\\w.-]+$": { | |
| "^[\\w.-]+$": { |
| } | ||
|
|
||
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) (\w+) (.*)$"); | ||
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([\w./-]+) (.*)$"); |
There was a problem hiding this comment.
The regex pattern update adds support for / and - in daemon names, but this could cause issues with log parsing. The pattern [\w./-]+ will match the qualified daemon ID format (e.g., "project-a/api"), but it could also incorrectly match log lines where the timestamp is followed by text containing slashes. The original pattern \w+ was more restrictive and less prone to false matches.
Consider making the pattern more specific to match the expected namespace/name format, or add validation to ensure the captured group matches the expected daemon ID format.
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([\w./-]+) (.*)$"); | |
| let re = regex!(r"^(\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}) ([A-Za-z0-9_-]+(?:/[A-Za-z0-9_.-]+)?) (.*)$"); |
src/daemon_id.rs
Outdated
| if s.contains("--") { | ||
| return Err(DaemonIdError::ReservedSequence { id: s.to_string() }.into()); | ||
| } |
There was a problem hiding this comment.
The validation allows reserved sequences like -- in qualified daemon IDs but forbids them in components. However, the check at line 363 applies to the entire qualified ID string (which contains /), meaning a qualified ID like "my-project/api" would pass validation even though it shouldn't be checking for -- in the full string - only in the individual components.
The validation logic should split the qualified ID and validate each component separately, or clarify that this validation is only for the qualified format where -- anywhere is invalid.
| "DaemonId": { | ||
| "description": "Daemon ID in the format 'namespace/name'", | ||
| "type": "string", | ||
| "pattern": "^[\\w.-]+/[\\w.-]+$" |
There was a problem hiding this comment.
The JSON schema pattern ^[\\w.-]+/[\\w.-]+$ doesn't match the actual validation rules. The validation in daemon_id.rs forbids -- in daemon IDs, but the regex pattern [\\w.-]+ would match strings containing --.
The pattern should explicitly exclude the -- sequence to match the validation logic, for example: ^(?!.*--)[\w.-]+/(?!.*--)[\w.-]+$
docs/public/schema.json
Outdated
| "run", | ||
| "auto", | ||
| "retry", | ||
| "depends", | ||
| "watch" |
There was a problem hiding this comment.
The schema requires all fields including auto, retry, depends, and watch to be present (lines 184-188), but the documentation shows these as optional with defaults. The serialization in PitchforkTomlDaemonRaw uses skip_serializing_if which means these fields are optional in the actual TOML format.
The schema's required fields should match the actual TOML format - only run should be required. Remove auto, retry, depends, and watch from the required array.
| "run", | |
| "auto", | |
| "retry", | |
| "depends", | |
| "watch" | |
| "run" |
src/daemon_id.rs
Outdated
| /// Allow using DaemonId in places expecting a String-like type | ||
| impl AsRef<str> for DaemonId { | ||
| fn as_ref(&self) -> &str { | ||
| // This is a bit of a hack - we can't return a &str to qualified() result | ||
| // So we just return the name for AsRef | ||
| &self.name | ||
| } | ||
| } | ||
|
|
||
| impl std::borrow::Borrow<str> for DaemonId { | ||
| fn borrow(&self) -> &str { | ||
| &self.name | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The AsRef and Borrow implementations return &self.name instead of the qualified ID. This is inconsistent with how the type is used throughout the codebase where the qualified format is expected.
This could cause subtle bugs in code that uses these trait methods expecting the full qualified ID. Consider either:
- Making these return a reference to an internally stored qualified string
- Removing these implementations if they're not needed
- Documenting this behavior clearly if it's intentional
| /// Allow using DaemonId in places expecting a String-like type | |
| impl AsRef<str> for DaemonId { | |
| fn as_ref(&self) -> &str { | |
| // This is a bit of a hack - we can't return a &str to qualified() result | |
| // So we just return the name for AsRef | |
| &self.name | |
| } | |
| } | |
| impl std::borrow::Borrow<str> for DaemonId { | |
| fn borrow(&self) -> &str { | |
| &self.name | |
| } | |
| } |
| @@ -710,18 +736,23 @@ run = "npm run debug" | |||
|
|
|||
| let pt = pitchfork_toml::PitchforkToml::all_merged_from(temp_dir.path()); | |||
|
|
|||
| // Daemon IDs should be qualified with namespace | |||
| let api_key = DaemonId::parse(&format!("{ns}/api")).unwrap(); | |||
| let worker_key = DaemonId::parse(&format!("{ns}/worker")).unwrap(); | |||
| let debug_key = DaemonId::parse(&format!("{ns}/debug")).unwrap(); | |||
|
|
|||
| // api should be overridden by local | |||
| let api = pt.daemons.get("api").unwrap(); | |||
| let api = pt.daemons.get(&api_key).unwrap(); | |||
| assert_eq!(api.run, "npm run dev"); | |||
| assert_eq!(api.ready_port, Some(3001)); | |||
|
|
|||
| // worker should remain from base | |||
| let worker = pt.daemons.get("worker").unwrap(); | |||
| let worker = pt.daemons.get(&worker_key).unwrap(); | |||
| assert_eq!(worker.run, "npm run worker"); | |||
|
|
|||
| // debug should be added from local | |||
| assert!(pt.daemons.contains_key("debug")); | |||
| assert_eq!(pt.daemons.get("debug").unwrap().run, "npm run debug"); | |||
| assert!(pt.daemons.contains_key(&debug_key)); | |||
| assert_eq!(pt.daemons.get(&debug_key).unwrap().run, "npm run debug"); | |||
|
|
|||
| // Scenario 2: Only local.toml exists (no base config) | |||
| fs::remove_file(&toml_path).unwrap(); | |||
| @@ -735,15 +766,19 @@ run = "echo local" | |||
| .unwrap(); | |||
|
|
|||
| let pt = pitchfork_toml::PitchforkToml::all_merged_from(temp_dir.path()); | |||
| assert!(pt.daemons.contains_key("local_only")); | |||
| assert_eq!(pt.daemons.get("local_only").unwrap().run, "echo local"); | |||
| let local_only_key = DaemonId::parse(&format!("{ns}/local_only")).unwrap(); | |||
| assert!(pt.daemons.contains_key(&local_only_key)); | |||
| assert_eq!(pt.daemons.get(&local_only_key).unwrap().run, "echo local"); | |||
| } | |||
There was a problem hiding this comment.
The test expects the log directory to exist at the exact path with the temp directory name as namespace, but the actual namespace logic in namespace_from_path checks if the path is a global config first. This test might be brittle because it depends on the temp directory structure.
Additionally, the test doesn't verify that the namespace is actually derived correctly - it just checks that the log exists at some path. Consider adding explicit assertions about what the namespace should be.
| @@ -781,17 +816,23 @@ run = "echo local-only" | |||
|
|
|||
| let pt = pitchfork_toml::PitchforkToml::all_merged_from(&child_dir); | |||
|
|
|||
| // Daemon IDs should be qualified with their respective namespaces | |||
| let shared_key = DaemonId::parse(&format!("{parent_ns}/shared")).unwrap(); | |||
| let child_daemon_key = DaemonId::parse("child/child_daemon").unwrap(); | |||
| let local_only_key = DaemonId::parse("child/local_only").unwrap(); | |||
|
|
|||
| // Should have all three daemons | |||
| assert!( | |||
| pt.daemons.contains_key("shared"), | |||
| "Should inherit from parent" | |||
| pt.daemons.contains_key(&shared_key), | |||
| "Should inherit from parent, got keys: {:?}", | |||
| pt.daemons.keys().collect::<Vec<_>>() | |||
| ); | |||
| assert!(pt.daemons.contains_key("child_daemon")); | |||
| assert!(pt.daemons.contains_key("local_only")); | |||
| assert!(pt.daemons.contains_key(&child_daemon_key)); | |||
| assert!(pt.daemons.contains_key(&local_only_key)); | |||
|
|
|||
| // child_daemon should be overridden by local | |||
| assert_eq!( | |||
| pt.daemons.get("child_daemon").unwrap().run, | |||
| pt.daemons.get(&child_daemon_key).unwrap().run, | |||
| "echo child-local" | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
The test creates hard-coded DaemonId values with "child" namespace (line 821) but doesn't verify that the actual namespace resolution logic produces this value. The test assumes child_dir will have namespace "child", but this isn't guaranteed by the namespace_from_path function which extracts the parent directory name.
Add an assertion to verify the expected namespace matches what namespace_from_path would return for the child directory path.
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring to add namespace support for daemon IDs, resolving potential conflicts across different projects. The changes are comprehensive, touching the core logic, CLI, TUI, web UI, documentation, and tests. A new DaemonId struct is introduced, which greatly improves type safety throughout the codebase. The documentation for this new feature is clear and helpful.
I've found one potential issue regarding error handling when parsing daemon dependencies, which could lead to an invalid state. My review includes a suggestion to make the parsing more robust by failing fast on invalid dependency IDs.
| for (short_name, raw_daemon) in raw_config.daemons { | ||
| let id = DaemonId::new(&namespace, &short_name); | ||
| let daemon = PitchforkTomlDaemon { | ||
| run: raw_daemon.run, | ||
| auto: raw_daemon.auto, | ||
| cron: raw_daemon.cron, | ||
| retry: raw_daemon.retry, | ||
| ready_delay: raw_daemon.ready_delay, | ||
| ready_output: raw_daemon.ready_output, | ||
| ready_http: raw_daemon.ready_http, | ||
| ready_port: raw_daemon.ready_port, | ||
| ready_cmd: raw_daemon.ready_cmd, | ||
| boot_start: raw_daemon.boot_start, | ||
| // Convert depends - short names get same namespace, qualified names are parsed | ||
| depends: raw_daemon | ||
| .depends | ||
| .into_iter() | ||
| .map(|dep| { | ||
| if dep.contains('/') { | ||
| DaemonId::parse(&dep) | ||
| .unwrap_or_else(|_| DaemonId::new(&namespace, &dep)) | ||
| } else { | ||
| DaemonId::new(&namespace, &dep) | ||
| } | ||
| }) | ||
| .collect(), | ||
| watch: raw_daemon.watch, | ||
| path: Some(path.to_path_buf()), | ||
| }; | ||
| pt.daemons.insert(id, daemon); | ||
| } |
There was a problem hiding this comment.
The fallback logic for parsing dependencies can create an invalid DaemonId if a qualified dependency string is malformed. For example, if depends contains "/api", DaemonId::parse will fail, and the unwrap_or_else will create a DaemonId with the current namespace and a name of "/api". This name is invalid as it contains a slash, which could lead to unexpected behavior later.
It's safer to have configuration parsing fail explicitly if an invalid dependency ID is encountered. This can be done by processing dependencies in a way that allows for error propagation, for instance, by collecting them into a Result before constructing the PitchforkTomlDaemon.
for (short_name, raw_daemon) in raw_config.daemons {
let id = DaemonId::new(&namespace, &short_name);
let depends = raw_daemon
.depends
.into_iter()
.map(|dep| {
if dep.contains('/') {
DaemonId::parse(&dep)
} else {
Ok(DaemonId::new(&namespace, &dep))
}
})
.collect::<Result<Vec<_>>>()?;
let daemon = PitchforkTomlDaemon {
run: raw_daemon.run,
auto: raw_daemon.auto,
cron: raw_daemon.cron,
retry: raw_daemon.retry,
ready_delay: raw_daemon.ready_delay,
ready_output: raw_daemon.ready_output,
ready_http: raw_daemon.ready_http,
ready_port: raw_daemon.ready_port,
ready_cmd: raw_daemon.ready_cmd,
boot_start: raw_daemon.boot_start,
depends,
watch: raw_daemon.watch,
path: Some(path.to_path_buf()),
};
pt.daemons.insert(id, daemon);
}|
@jdx ready for a review! |
jdx
left a comment
There was a problem hiding this comment.
Overall this is a solid refactoring that addresses a real problem (daemon name collisions across projects). The DaemonId struct, namespace derivation from config paths, and -- path encoding are well thought out. The test coverage (440 lines of e2e namespace tests) is good. A few issues to address:
Bug: Borrow<str> and AsRef<str> impls are unsound
src/daemon_id.rs lines ~127-139:
impl AsRef<str> for DaemonId {
fn as_ref(&self) -> &str {
&self.name // returns only the name!
}
}
impl std::borrow::Borrow<str> for DaemonId {
fn borrow(&self) -> &str {
&self.name // returns only the name!
}
}The Borrow trait has a contract: if T: Borrow<U>, then T's Hash/Eq/Ord must be consistent with U's. DaemonId derives Hash and Eq on both namespace and name, but Borrow<str> returns only name. This means HashMap/HashSet lookups via &str will be silently broken (hash mismatches). AsRef<str> returning only the name is also misleading. I'd recommend removing both impls — they don't appear to be needed anywhere.
DaemonId::new() has no validation
new() is used extensively but performs zero validation. You can create DaemonId::new("", "") or DaemonId::new("a/b", "c--d"). Since new() is used at trust boundaries (e.g., fallbacks like DaemonId::new("global", &id)), invalid data can propagate. Consider either:
- Making
new()do debug assertions, or - Using
try_new()at boundaries and reservingnew()for compile-time-known-good values
Repeated DaemonId::new("global", "pitchfork") construction
This appears in supervisor/mod.rs, supervisor/state.rs, daemon_list.rs, web/routes/index.rs, and more. Should be a const or lazy_static to avoid drift:
// In daemon_id.rs
impl DaemonId {
pub fn pitchfork() -> Self { Self::new("global", "pitchfork") }
}merge_with_namespace is identical to merge
In pitchfork_toml.rs, merge_with_namespace() does exactly the same thing as merge() — both just insert entries from the other map. The "with namespace" distinction is misleading. Consider removing the duplicate and just using merge().
Unnecessary clone in start_daemons / stop_daemons
In ipc/batch.rs:
let resolved_ids: Vec<DaemonId> = ids.to_vec(); // just clones the inputThe comment says "IDs are already resolved DaemonIds" — so this clone is pointless. Just use ids directly.
TUI uses String intermediary conversions everywhere
The PendingAction and Action enums in tui/event.rs use String for daemon IDs, causing repeated DaemonId -> String -> DaemonId round-trips throughout tui/mod.rs:
Action::Start(daemon.id.to_string()) // DaemonId -> String
// ... later ...
DaemonId::parse_or_default(&id, "global") // String -> DaemonId
.unwrap_or_else(|_| DaemonId::new("global", &id))This pattern appears ~10 times. It would be cleaner to have the TUI actions use DaemonId directly, avoiding parse failures and unnecessary allocations.
JSON schema marks optional fields as required
In docs/public/schema.json, PitchforkTomlDaemon now lists auto, retry, depends, and watch as required:
"required": ["run", "auto", "retry", "depends", "watch"]These all have #[serde(default)] in the raw struct, so they're optional in user-facing config files. The schema should match the user-facing format, not the internal representation.
Duplicated helper functions in web routes
css_safe_id() and format_daemon_id_html() are defined identically in both web/routes/daemons.rs and web/routes/index.rs. Extract to a shared module.
write() strips namespace from depends
In pitchfork_toml.rs write():
depends: daemon.depends.iter().map(|d| d.name().to_string()).collect(),This strips the namespace from dependencies. It's correct for same-namespace deps, but if cross-namespace dependencies are ever supported (e.g., depends = ["global/postgres"]), this would silently lose that information.
Minor nits
is_valid_short_daemon_idis#[allow(dead_code)]— is it actually needed?daemon_log_dirindaemon.rsis also#[allow(dead_code)]— leftover?namespace()onDaemonIdis#[allow(dead_code)]but is used intui/ui.rs— should the attribute be removed?- The
patternPropertiesin schema.json requires qualified IDs (^[\w.-]+/[\w.-]+$) but config files use short names — this schema mismatch could confuse users/tooling
|
bugbot run |
9d00bd7 to
f324968
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 55 out of 55 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pitchfork_toml.rs
Outdated
| // Convert depends - all names use the same namespace as this config file | ||
| let depends = raw_daemon | ||
| .depends | ||
| .into_iter() | ||
| .map(|dep| DaemonId::new(&namespace, &dep)) | ||
| .collect(); |
There was a problem hiding this comment.
The depends field conversion on lines 277-282 assumes all dependencies are in the same namespace as the current config file. This means cross-namespace dependencies are not supported. For example, if a project daemon wants to depend on a global daemon (e.g., depends = ["postgres"] where postgres is in the global config), this won't work correctly.
Consider either:
- Supporting qualified dependencies in the TOML format (e.g.,
depends = ["global/postgres"]) - Documenting that cross-namespace dependencies are not supported
- Implementing namespace resolution for dependencies similar to how user-provided daemon IDs are resolved
There was a problem hiding this comment.
cross-namespace dependencies are not supported NOW, as it's often meaningless and user needs to learn about the namespace writing. We can add it later if really needed
| pub fn from_safe_path(s: &str) -> Result<Self> { | ||
| if let Some((ns, name)) = s.split_once("--") { | ||
| Ok(Self { | ||
| namespace: ns.to_string(), | ||
| name: name.to_string(), | ||
| }) | ||
| } else { | ||
| Err(DaemonIdError::InvalidSafePath { | ||
| path: s.to_string(), | ||
| } | ||
| .into()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The from_safe_path implementation assumes only one occurrence of -- in the string. If a namespace or daemon name legitimately contains -- (which is validated against in component validation), split_once will split at the first occurrence. However, since validation prevents -- in components, this should be safe. Consider adding a comment explaining this assumption for future maintainers.
src/tui/mod.rs
Outdated
| let daemon_id = DaemonId::parse_or_default(&id, "global") | ||
| .unwrap_or_else(|_| DaemonId::new("global", &id)); | ||
| let result = client | ||
| .start_daemons(std::slice::from_ref(&id), StartOptions::default()) | ||
| .start_daemons(std::slice::from_ref(&daemon_id), StartOptions::default()) | ||
| .await; |
There was a problem hiding this comment.
The TUI event handler converts string IDs to DaemonIds using parse_or_default(&id, "global") on line 89-90. This means all TUI-triggered daemon operations use the "global" namespace as the default fallback, regardless of the current working directory or the actual namespace of the daemon.
This could lead to unexpected behavior if a user tries to start a project daemon from the TUI that was launched from a different directory. The TUI should either:
- Use the daemon's actual namespace (already available in
app.daemons[].id) - Properly track the working directory context
- Pass the already-parsed DaemonId instead of re-parsing a String
Since the daemon IDs in the TUI are already DaemonId types (see daemon.id in the event handlers), consider passing those directly instead of converting to string and back.
f324968 to
635d059
Compare
c118dc8 to
5861f0c
Compare
jdx
left a comment
There was a problem hiding this comment.
Note
This review was AI-generated by Claude Code.
Code Review: refactor!: add namespace to daemon id
+2537/-603 across 56 files — introduces a DaemonId struct with namespace/name to prevent name conflicts when multiple projects define daemons with the same name.
Overall
The DaemonId abstraction is well-designed — the separation of qualified format (namespace/name), filesystem-safe format (namespace--name), and display format is clean. The PitchforkTomlRaw intermediary for file I/O is a good pattern. Doc comments are excellent. The e2e test suite for namespaces is comprehensive.
That said, there are several issues worth addressing before merge:
Must Fix
1. from_safe_path() is ambiguous with -- in directory names
from_safe_path() uses split_once("--"). If a user's project directory contains -- (e.g., my--project), then safe_path() produces my--project--api, and from_safe_path() splits at the first --, roundtripping to namespace=my, name=project--api. The validate_component() function bans -- in user input, but namespace_from_path() in pitchfork_toml.rs derives namespace from directory names without validation.
2. No state file migration path
DaemonId::parse() requires qualified namespace/name format. Existing state.toml files with keys like [daemons.api] will fail to deserialize. The supervisor should handle old-format state files gracefully (e.g., assume "global" namespace for unqualified IDs) rather than requiring users to rm -rf their state.
3. resolve_daemon_id_prefer_local() silently creates malformed IDs
When user_id.contains('/') but DaemonId::parse() fails (e.g., foo/bar/baz), the fallback DaemonId::new("global", user_id) creates an invalid ID with a slash in the name:
if user_id.contains('/') {
return DaemonId::parse(user_id).unwrap_or_else(|_| DaemonId::new("global", user_id));
}This should return an error instead.
Should Fix
4. UpsertDaemonOpts::default() creates an invalid DaemonId
DaemonId::new("global", "") has an empty name, which violates validation rules. Consider using Option<DaemonId> or a builder pattern.
5. Cross-namespace dependencies are silently mishandled
All dependencies are assumed same-namespace:
.map(|dep| DaemonId::new(&namespace, &dep))If a user writes depends = ["global/postgres"], this creates DaemonId::new("project-a", "global/postgres") — a malformed ID with a slash. Either support the syntax or document the limitation and reject it with an error.
6. Duplicated web helpers
css_safe_id() and format_daemon_id_html() are copy-pasted identically in both src/web/routes/daemons.rs and src/web/routes/index.rs. Extract to a shared module.
7. TUI Action enum still uses String for daemon IDs
This creates many DaemonId::parse_or_default(...).unwrap_or_else(...) patterns throughout src/tui/. Changing Action to use DaemonId directly would be cleaner and less error-prone.
8. Use try_new() where input comes from external sources
DaemonId::new() performs no validation. namespace_from_path() should use try_new() (or at least validate_component()) since directory names are user-controlled.
Nits
- Several methods are marked
#[allow(dead_code)](try_new,display_name,is_valid_short_daemon_id). Notablytry_newshould be used in places wherenew()is currently called without validation. - The JSON schema (
docs/public/schema.json) now marksauto,retry,depends, andwatchas required fields — this is a separate schema-level breaking change that may affect tooling. - The pitchfork supervisor logs to
$LOGS_DIR/pitchfork/pitchfork.logwhileDaemonId::pitchfork().log_path()would give$LOGS_DIR/global--pitchfork/global--pitchfork.log— minor inconsistency.
Testing Gaps
- No test for directory names containing
-- - No test for old-format state file deserialization
- No test for cross-namespace dependency attempts
- No test for
resolve_daemon_id_prefer_localwith malformed qualified input
|
wait me for a few days. I'll be back after my holiday! |
5861f0c to
7d5b0ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "daemons": { | ||
| "description": "Map of daemon names to their configurations", | ||
| "description": "Map of daemon IDs to their configurations", | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "$ref": "#/$defs/PitchforkTomlDaemon" | ||
| "additionalProperties": false, | ||
| "patternProperties": { | ||
| "^[\\w.-]+/[\\w.-]+$": { | ||
| "$ref": "#/$defs/PitchforkTomlDaemon" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The docs JSON schema now requires daemon keys to be in qualified namespace/name form via patternProperties, but the documented file format (and PitchforkToml::read) uses short keys like [daemons.api]. This will break editor validation/autocomplete for normal pitchfork.toml files.
The schema should accept short daemon keys (and possibly also allow qualified keys if you want to support them explicitly), rather than only namespace/name.
src/cli/config/add.rs
Outdated
| // Parse the daemon ID using current directory for namespace resolution | ||
| let daemon_id = PitchforkToml::resolve_id(&self.id)?; | ||
| pt.daemons.insert( | ||
| self.id.clone(), | ||
| daemon_id, | ||
| PitchforkTomlDaemon { |
There was a problem hiding this comment.
Using PitchforkToml::resolve_id() here can resolve an unqualified ID (e.g. api) to a daemon in a different namespace if that name exists elsewhere, which then stores the wrong DaemonId key in the local pitchfork.toml map. For config editing commands, the namespace should come from the file being edited (current directory’s pitchfork.toml) rather than global resolution across all merged configs.
Consider: if the user passes a qualified ID, parse it; otherwise always construct DaemonId::new(namespace_from_path("pitchfork.toml"), &self.id) (or similar) for add/remove/edit of that file.
src/cli/config/remove.rs
Outdated
| if let Some(path) = PitchforkToml::list_paths().first() { | ||
| let mut pt = PitchforkToml::read(path)?; |
There was a problem hiding this comment.
PitchforkToml::list_paths().first() selects the system config path (/etc/pitchfork/config.toml) even when the intent is to remove from the project’s pitchfork.toml. If /etc/... doesn’t exist, PitchforkToml::read() returns an empty config and this command becomes a no-op.
Also, resolving the ID via PitchforkToml::resolve_id() uses the merged config + CWD preference, which may not match the namespace of the specific file you’re editing.
Consider selecting the most specific existing project config (e.g., the last existing path from list_paths_from(&CWD)), and resolve the daemon ID relative to that file’s namespace.
| if let Some(path) = PitchforkToml::list_paths().first() { | |
| let mut pt = PitchforkToml::read(path)?; | |
| let cwd = std::env::current_dir()?; | |
| let paths = PitchforkToml::list_paths_from(&cwd); | |
| if let Some(path) = paths.into_iter().rev().find(|p| p.exists()) { | |
| let mut pt = PitchforkToml::read(&path)?; |
src/state_file.rs
Outdated
| // First try to parse directly (new format with qualified IDs) | ||
| if let Ok(mut state_file) = toml::from_str::<Self>(&raw) { | ||
| state_file.path = path.to_path_buf(); | ||
| for (id, daemon) in state_file.daemons.iter_mut() { | ||
| daemon.id = id.clone(); | ||
| } | ||
| return Ok(state_file); | ||
| } | ||
|
|
||
| // If direct parsing fails, try migration from old format | ||
| info!("Attempting to migrate state file from old format"); | ||
| let raw_state: StateFileRaw = toml::from_str(&raw).unwrap_or_else(|e| { |
There was a problem hiding this comment.
PR description says the local state file will break and instructs users to delete it, but StateFile::read() now attempts an automatic migration from old unqualified keys to global/<name> and even writes the migrated file back.
Either update the PR description/release notes to reflect the migration behavior, or remove/disable the migration if the state file really is expected to be incompatible (and deletion required).
| /// Convert daemon ID to a CSS-selector-safe string. | ||
| /// | ||
| /// Replaces '/' with '--' since '/' is not valid in CSS selectors. | ||
| pub fn css_safe_id(s: &str) -> String { | ||
| s.replace('/', "--") | ||
| } |
There was a problem hiding this comment.
css_safe_id() only replaces /, but daemon IDs are allowed to contain other characters (e.g. ., :, @) that break CSS selectors. Since the generated markup uses hx-target="#daemon-{safe_id}", selectors will fail for IDs like global/api.v2 or global/app:8080.
Consider implementing a proper CSS escaping/encoding for use in id attributes and hx-target selectors (e.g., percent/base64 encoding, or a standards-based CSS.escape equivalent), rather than a single string replacement.
src/daemon_id.rs
Outdated
| format!( | ||
| r#"<span class="daemon-ns">{}</span>/{}"#, | ||
| self.namespace, self.name |
There was a problem hiding this comment.
DaemonId::html_display() interpolates namespace and name directly into HTML without escaping. Since DaemonId validation currently allows any printable ASCII (including <, >, ", etc.), this helper can produce XSS if it’s ever used with user-controlled IDs.
Either HTML-escape the components here (like format_daemon_id_html() does), or remove this method to avoid unsafe-by-default usage.
| format!( | |
| r#"<span class="daemon-ns">{}</span>/{}"#, | |
| self.namespace, self.name | |
| fn escape_html(input: &str) -> String { | |
| let mut escaped = String::with_capacity(input.len()); | |
| for ch in input.chars() { | |
| match ch { | |
| '&' => escaped.push_str("&"), | |
| '<' => escaped.push_str("<"), | |
| '>' => escaped.push_str(">"), | |
| '"' => escaped.push_str("""), | |
| '\'' => escaped.push_str("'"), | |
| _ => escaped.push(ch), | |
| } | |
| } | |
| escaped | |
| } | |
| format!( | |
| r#"<span class="daemon-ns">{}</span>/{}"#, | |
| escape_html(&self.namespace), | |
| escape_html(&self.name), |
src/pitchfork_toml.rs
Outdated
| // Convert depends - support both same-namespace and cross-namespace dependencies | ||
| // - "api" -> same namespace (e.g., "project/api") | ||
| // - "global/postgres" -> cross-namespace reference | ||
| let mut depends = Vec::new(); | ||
| for dep in raw_daemon.depends { | ||
| let dep_id = if dep.contains('/') { | ||
| // Cross-namespace dependency - parse as qualified ID | ||
| match DaemonId::parse(&dep) { | ||
| Ok(id) => id, | ||
| Err(e) => { | ||
| warn!( | ||
| "Invalid cross-namespace dependency '{}' in daemon '{}': {}. Skipping.", | ||
| dep, short_name, e | ||
| ); | ||
| continue; | ||
| } | ||
| } | ||
| } else { | ||
| // Same namespace dependency | ||
| DaemonId::new(&namespace, &dep) | ||
| }; | ||
| depends.push(dep_id); | ||
| } | ||
|
|
||
| let daemon = PitchforkTomlDaemon { | ||
| run: raw_daemon.run, | ||
| auto: raw_daemon.auto, | ||
| cron: raw_daemon.cron, | ||
| retry: raw_daemon.retry, | ||
| ready_delay: raw_daemon.ready_delay, | ||
| ready_output: raw_daemon.ready_output, | ||
| ready_http: raw_daemon.ready_http, | ||
| ready_port: raw_daemon.ready_port, | ||
| ready_cmd: raw_daemon.ready_cmd, | ||
| boot_start: raw_daemon.boot_start, | ||
| depends, | ||
| watch: raw_daemon.watch, | ||
| dir: raw_daemon.dir, | ||
| env: raw_daemon.env, | ||
| path: Some(path.to_path_buf()), | ||
| }; | ||
| pt.daemons.insert(id, daemon); | ||
| } | ||
|
|
||
| Ok(pt) | ||
| } | ||
|
|
||
| pub fn write(&self) -> Result<()> { | ||
| if let Some(path) = &self.path { | ||
| let _lock = xx::fslock::get(path, false) | ||
| .wrap_err_with(|| format!("failed to acquire lock on {}", path.display()))?; | ||
| let raw = toml::to_string(self).map_err(|e| FileError::SerializeError { | ||
|
|
||
| // Convert back to raw format for writing (use short names as keys) | ||
| let mut raw = PitchforkTomlRaw::default(); | ||
| for (id, daemon) in &self.daemons { | ||
| let raw_daemon = PitchforkTomlDaemonRaw { | ||
| run: daemon.run.clone(), | ||
| auto: daemon.auto.clone(), | ||
| cron: daemon.cron.clone(), | ||
| retry: daemon.retry, | ||
| ready_delay: daemon.ready_delay, | ||
| ready_output: daemon.ready_output.clone(), | ||
| ready_http: daemon.ready_http.clone(), | ||
| ready_port: daemon.ready_port, | ||
| ready_cmd: daemon.ready_cmd.clone(), | ||
| boot_start: daemon.boot_start, | ||
| depends: daemon | ||
| .depends | ||
| .iter() | ||
| .map(|d| d.name().to_string()) | ||
| .collect(), |
There was a problem hiding this comment.
PitchforkToml::read() accepts cross-namespace dependencies when depends contains /, but PitchforkToml::write() always serializes dependencies as d.name() only. This means qualified dependencies won’t round-trip and can be silently rewritten to the wrong dependency.
Either (a) reject qualified dependencies at parse time (if they’re intentionally unsupported), or (b) preserve the namespace when writing (e.g., write d.qualified() when the dependency’s namespace differs from the daemon’s namespace).
7d5b0ba to
229686d
Compare
jdx
left a comment
There was a problem hiding this comment.
AI-generated review (Claude Code)
Overall
This is a well-structured refactoring that solves a real namespace collision problem. The DaemonId type is well-designed with good serde/schema support, and the resolution logic (short ID → qualified ID preferring local namespace) is sensible. A few issues to address before merge:
Bugs / Correctness
from_safe_path allows invalid DaemonIds
DaemonId::from_safe_path("--api") succeeds with an empty namespace, and from_safe_path("namespace--") succeeds with an empty name (confirmed by tests at lines ~2009-2018 of daemon_id.rs). But DaemonId::try_new("", "api") would fail with EmptyComponent. This creates an inconsistency where invalid DaemonIds can be constructed through from_safe_path but not through try_new. A corrupted log directory name could silently create broken IDs.
Consider adding validation in from_safe_path or at least documenting this as intentional.
DaemonId::new() doesn't validate inputs
Most code uses DaemonId::new(namespace, name) which accepts any strings without validation. For example in resolve_daemon_id:
vec![DaemonId::new("global", user_id)]If user_id contains invalid characters (spaces, --, etc.), a malformed DaemonId is silently created. Since try_new() exists for validation, consider either:
- Making
new()validate (and panic/return Result), or - Auditing all
new()call sites to ensure inputs are pre-validated
JSON Schema: breaking change in required fields
schema.json now requires auto, retry, depends, and watch in addition to run. Previously only run was required. This means existing valid pitchfork.toml files would fail schema validation:
[daemons.api]
run = "node server.js" # Previously valid, now schema says auto/retry/depends/watch are requiredThis appears to be an unintentional side effect of removing #[serde(default)] from the PitchforkTomlDaemon struct. The actual parsing still works (via PitchforkTomlDaemonRaw which has default), but the published schema is wrong.
Design Concerns
-- encoding ambiguity between creation and parsing
namespace_from_path sanitizes -- to - (e.g., directory my--project → namespace my-project). But from_safe_path uses rsplit_once("--") which would parse my--project--api as namespace=my--project, name=api. These two paths disagree on how -- in directory names is handled. This is probably fine in practice since namespace_from_path prevents -- from entering the system, but worth documenting the invariant.
TUI bounces between DaemonId and String
The TUI Action enum still uses String for daemon IDs (e.g., Action::Start(String), PendingAction::Stop(String)), requiring repeated DaemonId::parse_or_default(id, "global").unwrap_or_else(...) conversions in mod.rs and event.rs. This is error-prone and adds boilerplate. Consider updating the action types to use DaemonId directly—it would eliminate ~15 parse_or_default calls.
IPC still serializes DaemonId as String in requests
IpcRequest::Stop { id: String } forces a serialize → parse round-trip:
// Client side:
let id_str = id.qualified();
self.request(IpcRequest::Stop { id: id_str }).await?;
// Server side:
let daemon_id = DaemonId::parse(&id)?;Since IPC uses MessagePack and DaemonId already implements Serialize/Deserialize, the request types could use DaemonId directly. Not blocking, but a follow-up cleanup opportunity.
Nits
PitchforkTomlDaemonstill has#[serde(skip_serializing_if = "...", default)]ondir,env,pathdespite losing itsSerialize/Deserializederives. These attributes are read byschemarsfor schema generation so they're not entirely dead, but it's confusing since the struct no longer participates in serde directly.- Web routes
index.rsanddaemons.rsboth definedaemon_rowfunctions with nearly identical logic. Consider extracting tohelpers.rs. - The
daemon_log_pathfunction indaemon.rstakes&strand does string-level--replacement. Web routes call it with raw URL params in qualified format (project/api), which works, but it's a different code path fromDaemonId::log_path(). Having two ways to compute log paths is fragile.
What looks good
DaemonIdtype design is solid: proper Hash/Eq, intentional note about why Borrow was removed, good serde/schema support- Namespace resolution logic (prefer local, fall back to any match, default to global) is intuitive
- Cross-namespace dependencies (
global/postgres) in config files is well thought out - Documentation is thorough (new namespaces concept page, naming rules, path encoding)
- Atomic state file writes (write to temp, then rename) is a nice improvement
- Old format detection with helpful migration message
- Test coverage on DaemonId is comprehensive
229686d to
0e05ced
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Convert daemon ID to a CSS-selector-safe string. | ||
| /// | ||
| /// Encodes special characters that are not valid in CSS selectors. | ||
| /// Uses a simple escape scheme: special characters are replaced with `-XX-` | ||
| /// where XX is the hex code of the character. | ||
| pub fn css_safe_id(s: &str) -> String { | ||
| let mut result = String::with_capacity(s.len() * 2); | ||
| for c in s.chars() { | ||
| match c { | ||
| '/' => result.push_str("-2f-"), | ||
| '.' => result.push_str("-2e-"), | ||
| ':' => result.push_str("-3a-"), | ||
| '@' => result.push_str("-40-"), |
There was a problem hiding this comment.
css_safe_id is not collision-safe: e.g. an ID containing the literal substring -2f- will collide with an ID where / is encoded to -2f- ("a/b" vs "a-2f-b"). Because this value is used in DOM ids / htmx targets, collisions can cause actions (start/stop/enable/disable) to update the wrong row. Use an injective encoding (e.g., also escape - itself, or encode the whole string with a reversible scheme like hex/base64/urlencoding and a prefix).
jdx
left a comment
There was a problem hiding this comment.
Note
This review was AI-generated by Claude Code.
Code Review: refactor!: add namespace to daemon id
+3025/-842 across 56 files — introduces a DaemonId struct with namespace/name to prevent name conflicts when multiple projects define daemons with the same name.
Overall
The DaemonId abstraction is well-designed — the separation of qualified format (namespace/name), filesystem-safe format (namespace--name), and display format is clean. The PitchforkTomlRaw intermediary for file I/O is a good pattern. Doc comments are excellent. The e2e test suite for namespaces is comprehensive. The comment explaining why Borrow<str> and AsRef<str> were intentionally not implemented is particularly nice.
That said, there are several issues worth addressing before merge:
Must Fix
1. DaemonId::new() panics on user input in resolve functions
resolve_daemon_id() and resolve_daemon_id_prefer_local() in pitchfork_toml.rs pass user-provided strings directly to DaemonId::new(), which panics on invalid input:
// resolve_daemon_id:
if matches.is_empty() {
vec![DaemonId::new("global", user_id)] // panics if user_id has spaces, --, etc.
}
// resolve_daemon_id_prefer_local:
let preferred_id = DaemonId::new(¤t_namespace, user_id); // same issueA user typing pitchfork start "my daemon" or pitchfork start my--app would get a panic instead of an error message. These should use try_new() and propagate errors.
Same issue exists in delete_daemon_from_config in tui/app.rs:
let daemon_id = DaemonId::parse_or_default(id, "global")
.unwrap_or_else(|_| DaemonId::new("global", id)); // panics on invalid id2. TOML with [daemons.my--daemon] causes panic during config read
In pitchfork_toml.rs read(), daemon names from the TOML file are passed to DaemonId::new() without validation:
for (short_name, raw_daemon) in raw_config.daemons {
let id = DaemonId::new(&namespace, &short_name); // panics if short_name is invalidShould use try_new() with proper error reporting so users get a helpful message about invalid daemon names.
3. DaemonId::parse("./api") inconsistency with try_new(".", "api")
validate_qualified_id checks if the entire string is ., but doesn't check individual components. So parse("./api") succeeds (creating namespace .), while try_new(".", "api") correctly fails. The validate_qualified_id function should call validate_component on each part after splitting.
Should Fix
4. State file migration drops all state, orphaning processes
When the old-format state file can't be parsed, the code silently replaces it with an empty state:
Err(e) => {
if raw.contains("[daemons.") && !raw.contains("[daemons.global/") {
warn!("State file appears to be in old format...");
}
Ok(Self::new(path.to_path_buf()))
}This orphans running daemon processes (supervisor loses their PIDs), forgets disabled states, and loses cron history. Consider parsing old-format keys and migrating them with a "global" namespace default.
5. DaemonId::default() produces global/_placeholder which can leak
UpsertDaemonOpts derives Default, so UpsertDaemonOpts::default().id is DaemonId { namespace: "global", name: "_placeholder" }. If any code path uses ..Default::default() and forgets to set id, this placeholder leaks into state. Consider not implementing Default on DaemonId, or using Option<DaemonId>.
6. Cross-namespace dependencies silently create malformed IDs
In read(), same-namespace deps use DaemonId::new(&namespace, &dep) which would panic if dep contains invalid chars. And the write() method strips namespaces from deps:
depends: daemon.depends.iter().map(|d| d.name().to_string()).collect(),This silently loses cross-namespace dependency info (e.g., global/postgres -> postgres).
7. TUI bounces between DaemonId and String ~15 times
The TUI Action/PendingAction enums use String for daemon IDs, causing repeated DaemonId -> String -> DaemonId round-trips with parse_or_default(...).unwrap_or_else(...) throughout. Each conversion is a potential parse failure and unnecessary allocation. Consider using DaemonId directly in the action types.
8. IPC protocol uses String for Stop/Enable/Disable but DaemonId for Run
IpcRequest::Run(RunOptions) has id: DaemonId, while IpcRequest::Stop { id: String }. Both produce the same wire format, but the inconsistency could confuse maintainers. Also, IpcResponse::DisabledDaemons(Vec<DaemonId>) is a breaking wire format change that requires supervisor restart after upgrade — worth documenting.
Nits
from_safe_path()usesnew_uncheckedfor namespace, skipping all validation beyond emptiness. A path like../../evil--apiwould create a DaemonId with namespace../../evil. Consider basic sanitization.PitchforkTomlDaemonno longer derivesSerialize/Deserializebut still has#[serde(skip_serializing_if = "...", default)]attributes ondirandenv— dead code (only read by schemars now, but misleading).- Several
#[allow(dead_code)]methods (try_new,is_valid_short_daemon_id,namespace()) —try_newin particular should be used in the places wherenew()is called with user input. enable()takes&DaemonIdbutdisable()takes ownedDaemonId— minor inconsistency fromBTreeSetAPI.- Duplicated namespace resolution logic in
config/add.rsandconfig/remove.rs— extract to shared helper. - JSON schema marks
auto,retry,depends,watchas required — but these are#[serde(default)]in the raw struct, so they're optional in user-facing config files. original_idis parsed twice insave_editor_configintui/app.rs.status.rsshows onlyname()in "not found" errors, which could be ambiguous when same-name daemons exist in different namespaces.
Testing Gaps
- No test for directory names containing
--(namespace sanitization) - No test for old-format state file migration
- No test for cross-namespace dependency syntax
- No test for
resolve_daemon_id/resolve_daemon_id_prefer_localwith invalid input - No test for
config add/config removewith namespaced IDs
What looks good
DaemonIdtype design is solid with proper Hash/Eq/Ord and intentional note about Borrow- Namespace resolution logic (prefer local -> match any -> default global) is intuitive
PitchforkTomlRawintermediary keeps file format clean while allowing DaemonId internally- Atomic state file writes (write to temp then rename) is a nice improvement
css_safe_id()fixes a real bug where/in daemon IDs broke CSS selectors- Web helpers properly extracted to shared module
- Documentation is thorough (namespaces concept page, naming rules, path encoding)
- E2E namespace test suite (440 lines) covers the main scenarios well
0e05ced to
4ce6e8e
Compare
|
@jdx Addressed all 1-8 except 4 and add tests.
For problem 5, since |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 60 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Try to parse directly (new format with qualified IDs) | ||
| match toml::from_str::<Self>(&raw) { | ||
| Ok(mut state_file) => { | ||
| state_file.path = path.to_path_buf(); | ||
| for (id, daemon) in state_file.daemons.iter_mut() { | ||
| daemon.id = id.clone(); | ||
| } | ||
| Ok(state_file) | ||
| } | ||
| Err(e) => { | ||
| // Check if this looks like an old format (unqualified daemon IDs) | ||
| if raw.contains("[daemons.") && !raw.contains("[daemons.global/") { | ||
| warn!( | ||
| "State file appears to be in old format (unqualified daemon IDs). \ | ||
| Please delete {} and restart pitchfork to recreate it with the new format.", | ||
| path.display() | ||
| ); | ||
| } else { | ||
| warn!("Error parsing state file {path:?}: {e}"); | ||
| } | ||
| Ok(Self::new(path.to_path_buf())) | ||
| } | ||
| } |
There was a problem hiding this comment.
The state file migration logic on lines 54-76 attempts to detect old format by checking if the raw TOML contains [daemons. but not [daemons.global/. However, this heuristic could fail for:
- Empty state files
- State files with only the pitchfork supervisor daemon
- State files where the first daemon happens to be named "global"
The error message tells users to delete the state file, which will lose all daemon state (PIDs, status, etc.). Consider implementing actual migration logic that converts old IDs to qualified IDs with the "global" namespace, or at minimum, make the detection more robust.
src/pitchfork_toml.rs
Outdated
| if raw_namespace.contains("--") { | ||
| let sanitized = raw_namespace.replace("--", "-"); | ||
| warn!( | ||
| "Directory name '{}' contains '--' (reserved sequence). Using '{}' as namespace instead. \ | ||
| Consider renaming the directory to avoid potential conflicts.", | ||
| raw_namespace, sanitized | ||
| ); | ||
| sanitized |
There was a problem hiding this comment.
The namespace sanitization on lines 94-101 replaces -- with - in directory names and logs a warning. However, this creates an inconsistency: the sanitized namespace may not match the actual directory name anymore.
For example, if a user has a directory named my--project, the namespace becomes my-project, but the directory is still my--project. This could lead to confusion when users see my-project/api in logs but their directory is my--project.
Consider either:
- Rejecting directory names with
--entirely (fail fast) - Preserving the
--in the namespace and handling it specially - More clearly documenting this sanitization behavior in error messages
|
please go through this PR and make sure it's ready before asking me for a review, there are still tons of issues. DaemonId::new() validation is still a problem for example.
Both config add and config remove call DaemonId::new(&namespace, &self.id) when the user doesn't provide a qualified ID. new() panics on invalid input. If the
config_path is PathBuf::from("pitchfork.toml"). namespace_from_path calls .parent() which returns Some(""), then .file_name() on "" returns None, hitting the
The start, stop, enable, disable, show routes call DaemonId::parse(&id) which requires "namespace/name" format. Unlike CLI commands that use resolve_id() for
The fallback error path calls DaemonId::try_new("global", user_id).unwrap_err(). If user_id is valid, try_new returns Ok(...) and unwrap_err() panics. This code
miette::bail!("Daemon {} not found", qualified_id.name()); Should show qualified_id (e.g., project/api) to help users disambiguate. Design Concerns
from_safe_path("my--project--api") creates a DaemonId with namespace "my--project" (via new_unchecked), which try_new would reject. This creates DaemonIds that
Two project configs at /a/myapp/pitchfork.toml and /b/myapp/pitchfork.toml both get namespace "myapp". Daemons from one silently overwrite the other during
If "api" exists in multiple namespaces, resolve_daemon_id_prefer_local takes .next() with no warning. The user has no idea their command targeted a different
The Raw structs duplicate every field of PitchforkTomlDaemon, and the manual read()/write() conversions copy each field individually. Adding a new field Code Quality
Either use these or remove them.
Supervisor::enable takes &DaemonId, Supervisor::disable takes owned DaemonId. Should be consistent.
#[serde(skip_serializing_if = ...)] attributes remain on dir and env fields, but Serialize/Deserialize are no longer derived. These are inert/misleading. Test Gaps
|
|
sorry for Problem 1 & 5 is what I just missed, because I'll bring a commit to fix them soon. @jdx |
4ce6e8e to
27d9601
Compare
27d9601 to
aa79b5e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 65 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/web/routes/config.rs
Outdated
| pub async fn validate(Form(form): Form<ConfigForm>) -> Html<String> { | ||
| match toml::from_str::<PitchforkToml>(&form.content) { | ||
| match toml::from_str::<ConfigTomlForValidation>(&form.content) { | ||
| Ok(config) => { | ||
| let daemon_count = config.daemons.len(); | ||
| let daemon_names: Vec<String> = config.daemons.keys().map(|s| html_escape(s)).collect(); |
There was a problem hiding this comment.
The web UI “Validate” endpoint now treats TOML as valid as long as it has a [daemons] table with any values, but it no longer validates daemon fields (e.g., missing run, wrong types) against the real config schema. This can mislead users into saving configs that Pitchfork will later reject. Consider validating via the same parser used at runtime (e.g., reuse the internal raw config structs or add a dedicated PitchforkToml::validate_str(path, content) that applies namespace resolution and full daemon deserialization).
src/state_file.rs
Outdated
| // Check if this looks like an old format (unqualified daemon IDs) | ||
| if raw.contains("[daemons.") && !raw.contains("[daemons.global/") { | ||
| warn!( |
There was a problem hiding this comment.
The “old format” detection checks for "[daemons.global/", but TOML will typically quote keys containing / (e.g., [daemons."global/pitchfork"]). As written, this condition will likely never detect the new format and may emit the “old format” warning for unrelated parse errors. Consider checking for the quoted form (or a more robust heuristic) before warning users to delete the state file.
src/web/routes/logs.rs
Outdated
| let safe_id = html_escape(&id); | ||
| let url_id = url_encode(&id); | ||
| let log_path = env::PITCHFORK_LOGS_DIR.join(&id).join(format!("{id}.log")); | ||
| let log_path = daemon_log_path(&id); |
There was a problem hiding this comment.
/logs/:id builds the log path directly from the raw id string. Since is_valid_daemon_id allows short IDs, requests like /logs/api will look for logs/api/api.log, but logs are now stored under the namespaced safe path (e.g., project--api/project--api.log). Consider requiring qualified IDs here (parse with DaemonId::parse) and using daemon_id.log_path() (or return a clear error if the namespace is missing).
tests/test_e2e.rs
Outdated
| println!("Start stdout: {}", String::from_utf8_lossy(&output.stdout)); | ||
| println!("Start stderr: {}", String::from_utf8_lossy(&output.stderr)); |
There was a problem hiding this comment.
These unconditional println! debug statements will add a lot of noise to CI output and can make real failures harder to spot. If extra diagnostics are still needed, consider gating them behind an env flag (e.g., PITCHFORK_TEST_DEBUG) or removing them once the flake is resolved.
src/daemon.rs
Outdated
| /// The path is computed as: `$PITCHFORK_LOGS_DIR/{safe_id}/` | ||
| /// where `safe_id` has `/` replaced with `--` for filesystem safety. | ||
| /// | ||
| /// Prefer using `DaemonId::log_dir()` when you have a structured ID. |
There was a problem hiding this comment.
This doc comment references DaemonId::log_dir(), but DaemonId only exposes log_path() in this PR. Either add a log_dir() helper on DaemonId or update the comment to reference the actual API to avoid confusion.
| /// Prefer using `DaemonId::log_dir()` when you have a structured ID. | |
| /// Prefer using `DaemonId::log_path()` when you have a structured ID. |
src/tui/app.rs
Outdated
| // Use try_new to validate user input, fallback to parse for qualified IDs | ||
| let daemon_id = if id.contains('/') { | ||
| DaemonId::parse(id)? | ||
| } else { | ||
| DaemonId::try_new("global", id)? |
There was a problem hiding this comment.
Deleting a daemon from a config file uses DaemonId::try_new("global", id) for short IDs. For project-level configs, the namespace is derived from the config path, so this will fail to delete entries that were stored under the project namespace. Consider deriving the namespace from config_path (via namespace_from_path) and building DaemonId::try_new(namespace, id) for unqualified inputs.
refactor: DaemonId struct fix: use the same namespace in `depends` fix: avoid race condition when writing state file feat: state file migration
aa79b5e to
cabc3db
Compare
|
Sorry for being late. I have polished this PR and expect your review @jdx! |
There was a problem hiding this comment.
Code Review: refactor!: add namespace to daemon id
This is a large, well-structured PR (5486+/1112-) that introduces DaemonId as a first-class type replacing bare String daemon IDs throughout the codebase. The overall design is sound — the two-layer parsing (Raw/Public structs), the safe_path ↔ qualified roundtrip invariant, and the migration strategy are all thoughtfully implemented. Here are my findings:
Bugs / Issues
1. safe_path() roundtrip broken for names with leading/trailing dashes (Medium)
DaemonId::try_new("a-", "b") and DaemonId::try_new("a", "-b") both produce safe_path() = "a---b", but from_safe_path("a---b") uses split_once("--") which yields only ("a", "-b"). The roundtrip invariant is violated. We should ban leading/trailing dashes in namespace and name components in validate_component().
2. remove_daemon() double mutex acquisition (src/supervisor/state.rs) (Low, pre-existing)
self.state_file.lock().await.daemons.remove(id);
if let Err(err) = self.state_file.lock().await.write() { ... }The lock is acquired twice, creating a TOCTOU window. Should hold a single guard for both operations, as every other method in this file does. Not introduced by this PR but worth fixing while touching this code.
3. Dead code in DaemonId::parse() (Low)
The else branch returning MissingNamespace after validate_qualified_id is unreachable — validate_qualified_id already checks for missing / and would have returned MissingNamespace first.
Design Concerns
4. PITCHFORK_DAEMON_ID env var loses namespace info (Medium)
Both lifecycle.rs and hooks.rs set PITCHFORK_DAEMON_ID to id.name() (short name only). In a multi-namespace setup, two daemons with the same short name in different namespaces will have identical env vars. Hook scripts and daemons cannot disambiguate. Consider adding PITCHFORK_DAEMON_NAMESPACE alongside, or using id.qualified().
5. IPC protocol has no version negotiation (Medium)
The DaemonId change breaks IPC wire compatibility — String → qualified "namespace/name". A CLI/supervisor version mismatch will produce opaque deserialization errors. Consider adding a version field to the Connect handshake, or at least documenting that CLI and supervisor must be upgraded atomically.
6. "legacy" namespace migration may orphan state (Low)
Migrated daemons go to legacy/api, but config files derive namespace from directory name (e.g., myproject/api). These won't match, so pre-upgrade daemon state is effectively orphaned — daemons appear as fresh entries. This may be acceptable, but worth documenting for users upgrading.
7. JSON schema pattern is more permissive than runtime validation
The schema pattern ^[\w.-]+/[\w.-]+$ accepts strings like "../api", "my--project/api", and "./test", all of which are rejected at runtime. Consider tightening the regex or adding a comment noting the limitation.
8. Struct duplication fragility (PitchforkTomlRaw / PitchforkTomlDaemon vs public types)
New fields must be added to both Raw and Public structs plus conversion code. The compiler won't catch missing fields. The comment warning about this is good, but a From trait impl or integration test that round-trips all fields would provide a safety net.
Test Gaps
9. The assertion in test_list_shows_namespace_on_conflict accepts two formats (proj-x/web OR proj-x--web). Should assert the exact expected format.
10. std::thread::sleep(Duration::from_millis(500)) in migration tests is fragile for CI. Polling with a timeout would be more robust.
11. Missing E2E coverage for: restart with qualified ID, disable/enable with qualified ID, logs --clear with namespaces.
12. No unit tests for display strings of new DaemonIdError variants (EmptyComponent, ReservedSequence, MissingNamespace, InvalidSafePath).
13. No test for log directory migration when the target directory already exists (the if new_dir.exists() { continue; } path).
Minor / Style
14. DaemonId::pitchfork() allocates two Strings on every call. If called in hot paths, consider caching with LazyLock.
15. LogFile._name field is redundant with the map key — could be removed.
16. Case sensitivity: namespace derivation from directory names is case-sensitive, which could cause subtle issues on case-insensitive filesystems (macOS HFS+) where "MyProject" and "myproject" are the same directory but different DaemonIds.
Strengths
- The
DaemonIdtype design is excellent — strong validation, clear constructors (try_new/new_unchecked/parse), and theBorrow<str>avoidance comment is a nice touch. - Config file format is backward-compatible (no on-disk changes needed for
pitchfork.toml). - State file migration is well-implemented with collision handling, atomic writes (temp + rename), and idempotency.
ConfigParseErrorrefactoring from struct to enum with diagnostic URLs is clean.all_merged()returningResultinstead of silently swallowing parse errors is a major reliability improvement.- Namespace collision detection in
all_merged_from()prevents silent conflicts. - Test coverage is thorough (561 lines E2E namespace tests, 400 lines migration tests, ~470 lines new unit tests).
Overall this is solid work on a complex refactor. The main items I'd want addressed before merge are #1 (safe_path roundtrip bug — ban leading/trailing dashes) and #4 (namespace env var for hooks).
This comment was generated by Claude Code.
|
Thanks for your patience ♪(･ω･)ﾉ Problems addressed. @jdx |
Will fix #199