feat(list): always display fully-qualified daemon names#467
Conversation
📝 WalkthroughWalkthroughThe PR adds a persisted-state fallback to daemon ID resolution and simplifies list display-name generation to use per-entry qualified styling; tests are updated to require fully-qualified daemon names in list output. ChangesDaemon ID Resolution and Display Name
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR changes
Confidence Score: 5/5Safe to merge — changes are well-scoped, the state-file fallback generalisation is backwards-compatible, and the list simplification has no failure modes. All three changed files have clean, correct logic. The No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "feat(list): always display fully-qualifi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_e2e_namespace.rs (1)
292-295:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert start commands succeed before validating list output.
This test can pass even when daemon startup fails, because
listalso shows available daemons. Please assert success for both start calls to prevent false positives.Suggested fix
- let _ = env.run_command_in_dir(&["start", "unique-api"], &project); + let start_api = env.run_command_in_dir(&["start", "unique-api"], &project); + assert!(start_api.status.success(), "Failed to start unique-api"); env.sleep(Duration::from_secs(1)); - let _ = env.run_command_in_dir(&["start", "unique-worker"], &project); + let start_worker = env.run_command_in_dir(&["start", "unique-worker"], &project); + assert!(start_worker.status.success(), "Failed to start unique-worker"); env.sleep(Duration::from_secs(1));🤖 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 `@tests/test_e2e_namespace.rs` around lines 292 - 295, The test currently ignores the results of env.run_command_in_dir when starting daemons; capture each call's result (the calls invoking "start" for "unique-api" and "unique-worker") and assert they succeeded before proceeding to list validation — e.g. store the return value from env.run_command_in_dir for each start and assert success (include the command output in the assertion message for debugging) so the test fails if a daemon failed to start rather than producing a false positive from the list output.src/cli/list.rs (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate
listhelp output examples to qualified IDs.The documented output still shows short names, but runtime now always renders
namespace/name. This will mislead users reading--help.Suggested doc update
-Output: - Name PID Status Error - api 12345 running - worker available - db errored exit code 127" +Output: + Name PID Status Error + my-namespace/api 12345 running + my-namespace/worker available + my-namespace/db errored exit code 127"🤖 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/list.rs` around lines 30 - 34, Update the help example in src/cli/list.rs so it shows qualified IDs (namespace/name) instead of short names; locate the multiline help/example string used for the "list" command (the help text or constant that contains the "Output:" block) and replace lines like "api", "worker", "db" with their qualified forms (e.g., "default/api", "default/worker", "default/db" or the appropriate namespace used by runtime) so the printed --help output matches the runtime rendering of namespace/name.
🤖 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.
Outside diff comments:
In `@src/cli/list.rs`:
- Around line 30-34: Update the help example in src/cli/list.rs so it shows
qualified IDs (namespace/name) instead of short names; locate the multiline
help/example string used for the "list" command (the help text or constant that
contains the "Output:" block) and replace lines like "api", "worker", "db" with
their qualified forms (e.g., "default/api", "default/worker", "default/db" or
the appropriate namespace used by runtime) so the printed --help output matches
the runtime rendering of namespace/name.
In `@tests/test_e2e_namespace.rs`:
- Around line 292-295: The test currently ignores the results of
env.run_command_in_dir when starting daemons; capture each call's result (the
calls invoking "start" for "unique-api" and "unique-worker") and assert they
succeeded before proceeding to list validation — e.g. store the return value
from env.run_command_in_dir for each start and assert success (include the
command output in the assertion message for debugging) so the test fails if a
daemon failed to start rather than producing a false positive from the list
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ece0bac3-c0c1-44f9-984f-790ef150ba0b
📒 Files selected for processing (3)
src/cli/list.rssrc/pitchfork_toml.rstests/test_e2e_namespace.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pitchfork_toml.rs
Modify pitchfork ls to always show namespace/name format instead of omitting the namespace when there's no local conflict. Also add state_file fallback in resolve_daemon_id() for resolving short daemon IDs globally when no local config matches. When ambiguous, error now lists all matching fully-qualified IDs.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/pitchfork_toml.rs`:
- Around line 425-435: The find_in_state_file function logs a warning for any
StateFile::read error, including the expected "file not found" case; change it
to only warn when the state file exists but is unreadable by inspecting the read
error (use e.kind() == io::ErrorKind::NotFound or check std::fs::metadata on
env::PITCHFORK_STATE_FILE) and suppress the warning when the file is absent,
otherwise log the error and return Vec::new(); update references in
find_in_state_file and keep StateFile::read usage intact.
🪄 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: 7aad1baf-6c6a-4d11-b9d5-8a23a1b03b74
📒 Files selected for processing (3)
src/cli/list.rssrc/pitchfork_toml.rstests/test_e2e_namespace.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_e2e_namespace.rs
- src/cli/list.rs
2 participants
fixes #465.
Modify pitchfork ls to always show namespace/name format instead of omitting the namespace when there's no local conflict.
Also add state_file fallback in resolve_daemon_id() for resolving short daemon IDs globally when no local config matches. When ambiguous, error now lists all matching fully-qualified IDs.
Summary by CodeRabbit
New Features
Bug Fixes
Tests