feat(elicitate): v0.3.0 — port v0.18.0 installer + v0.19.0 namespace command - #270
feat(elicitate): v0.3.0 — port v0.18.0 installer + v0.19.0 namespace command#270KooshaPari wants to merge 2 commits into
Conversation
…command
Port-forward of the per-namespace installer and runtime namespace
inspector from the wip/2026-07-22-phenotype-tooling-absorbed-go-mod
branch (which was closed as superseded) against the current main.
Main has had rmcp 0.2 -> 1.4, dirs 5 -> 6, schemars 0.8 -> 1.2,
thiserror 1.0 -> 2.0 since the wip branch was created, so the
rmcp-coupled MCP router changes (v0.13-v0.17) are out of scope.
This commit ports only the dep-light additions.
What ships
----------
* elicitate install --register-namespace <id> (repeatable)
- Each valid id gets its own launchd plist / systemd unit /
scheduled task on a deterministic port
* pub fn namespace_port(id) -> u16 — FNV-1a hash, range 7118..=8116
* pub struct NamespaceAutostart { inbox_id, port, target }
- Surfaced in InstallReport::namespace_autostarts
* InstallOptions::extra_inbox_ids: Vec<String>
* install_autostart_for(cli_path, inbox_id, port)
- Shared writer for default + per-namespace units
* pub fn is_valid_inbox_id(id) -> bool — [A-Za-z0-9_-]{1,64}
* pub fn resolve_inbox_root(inbox_id) -> PathBuf
- None / 'default' -> legacy root
- Valid id -> <parent>/inboxes/<id>
- Hostile id -> legacy root (never crashes on untrusted JSON)
* elicitate namespace {list,show,clean}
- Cross-platform autostart discovery (LaunchAgents / systemd-user / schtasks)
- 50ms loopback probe for daemon_live
- Idempotent gc across all registered namespaces
Tests (12 new, all green)
-------------------------
* parse_install_with_register_namespace
* parse_namespace_{list,show,clean_default_age,clean_with_age_and_dry_run}
* namespace_port_{distinct_and_deterministic,falls_in_expected_range}
* truncate_{short,long}
* enumerate_namespaces_default_only_when_no_units
* is_daemon_live_returns_false_for_unused_port
* gc_namespace_dry_run_keeps_files
* is_valid_inbox_id_accepts_alphanumeric_dashes_underscores
* resolve_inbox_root_default_and_namespace
Tests: 120/120 green (70 lib + 26 bin + 14 plugin + 6 lib-int + 4 mcp_stdio).
Build clean with --no-default-features.
Out of scope (requires separate rmcp 1.4 API rewrite)
-----------------------------------------------------
* v0.13.0 elicitate_reply MCP tool
* v0.14.0 multi-inbox (MCP) routing
* v0.16.0 elicitate_enqueue MCP tool
* v0.17.0 elicitate_cancel MCP tool
These all use rmcp 0.2 APIs that no longer exist in rmcp 1.4. The
mcp/router.rs in main currently fails to compile with --features mcp
due to the rmcp upgrade; a separate PR will rewrite the router.
Version bumped 0.2.0 -> 0.3.0.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
📝 WalkthroughSummaryThis PR ports namespace-aware installation and namespace management to current It adds:
The stated result is Must FixNo blocking issues are identified from the supplied summary. Should Fix
Consider
Approve / Request ChangesApprove if the repository checks and the 500-line file-size constraint pass. Otherwise, request changes for the failing checks or constraint violations. WalkthroughThe PR adds namespace validation, deterministic ports, per-namespace daemon installation, namespace status and cleanup commands, and an rmcp 1.4.x router migration. It also upgrades schemars, adds validation tests, and records the 0.3.0 and 0.4.0 releases. ChangesElicitate namespace and MCP updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ElicItateCLI
participant Installer
participant PlatformService
User->>ElicItateCLI: install --register-namespace ID
ElicItateCLI->>Installer: pass namespace IDs
Installer->>Installer: validate ID and compute deterministic port
Installer->>PlatformService: create namespace autostart
PlatformService-->>Installer: return registration result
Installer-->>ElicItateCLI: return install report
ElicItateCLI-->>User: display namespace autostart status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:XL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cb48bcadc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let plist = agents.join(&plist_name); | ||
| let label = format!("com.phenotype.elicitate{label_suffix}"); | ||
| let inbox_args = inbox_id | ||
| .map(|id| format!("<string>--inbox-id</string><string>{id}</string>")) |
There was a problem hiding this comment.
When elicitate install --register-namespace ... is used, the installed launchd/systemd/schtasks command includes daemon --inbox-id <id>, but DaemonArgs does not define any --inbox-id option and a repo-wide search shows no daemon flag with that name. These per-namespace services will exit during clap parsing instead of starting, so the new namespace autostart feature is nonfunctional until the daemon accepts and uses this flag.
Useful? React with 👍 / 👎.
| Err(_) => continue, | ||
| }; | ||
| let state = v.get("state").and_then(|s| s.as_str()).unwrap_or(""); | ||
| let is_terminal = matches!(state, "Answered" | "Cancelled" | "Expired"); |
There was a problem hiding this comment.
Real inbox files serialize RequestState with #[serde(rename_all = "snake_case")], so terminal states are written as answered, cancelled, and expired, not the capitalized strings checked here. As a result, elicitate namespace clean will skip normal terminal entries forever and report zero removals; the new test misses this because it hand-writes capitalized JSON instead of using the actual serializer.
Useful? React with 👍 / 👎.
| let port = namespace_port(id); | ||
| match install_autostart_for(&report.cli_path, Some(id), port) { |
There was a problem hiding this comment.
When --register-namespace is used, this loop creates additional per-namespace launchers such as elicitate.<id>.service / com.phenotype.elicitate.<id>.plist, but uninstall() still only removes the legacy default launcher/task. After uninstalling, those extra launchers are left behind and can keep trying to start the now-removed binary, so the installer should either track and remove namespace units or discover them during uninstall.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 `@crates/elicitate/Cargo.toml`:
- Line 3: Update the package version declared by the elicitate manifest to its
corresponding entry in Cargo.lock by regenerating Cargo.lock after the 0.3.0
change. Ensure the locked elicitate package records version 0.3.0 and commit the
synchronized lockfile.
In `@crates/elicitate/CHANGELOG.md`:
- Around line 43-62: Update the “Tests” heading in the changelog to report 13
new tests, matching the 13 named test entries listed below it.
In `@crates/elicitate/src/bin_elicitate.rs`:
- Around line 1146-1152: Make enumerate_namespaces_default_only_when_no_units
deterministic by isolating discover_namespace_ids from the developer’s machine
state: set HOME to a tempfile::tempdir for the test, or refactor the
unit-directory lookup to accept an injected search root. Ensure the test still
observes only the default namespace and does not read real LaunchAgents or
systemd user directories.
- Around line 184-187: Update the documentation comment near the inbox namespace
management description to remove the inaccurate “v0.18.x” reference, or replace
it with the crate’s current version “0.3.0”; keep the rest of the comment
unchanged and apply the same correction to the matching “v0.19.0” reference.
- Around line 1439-1488: Consolidate autostart naming and home-directory lookup
in shared helpers exposed by installer, including an
autostart_unit_name(inbox_id) function used by both install_autostart_for and
consumers such as autostart_unit_present and discover_namespace_ids. Remove the
unconditional home_dir dependency from autostart_unit_present so Windows checks
schtasks even when USERPROFILE is unset, while retaining home_dir for macOS and
Unix paths. Add a cfg fallback returning false for unsupported targets, and
ensure all platform branches use the shared naming rule.
- Around line 1640-1652: Update the terminal-record cleanup logic around the
state and queued_at_ms extraction to skip records lacking a usable timestamp
instead of defaulting to zero. Identify the persisted
completion/terminal-transition timestamp field by inspecting the record schema,
then use it for the cutoff comparison when present, falling back to queued_at_ms
only when appropriate; preserve deletion only for terminal records older than
the configured age.
- Around line 1625-1651: Update the cleanup loop in cmd_namespace_clean to treat
read_to_string and remove_file failures like malformed JSON: continue processing
remaining entries while collecting each failure. After the loop, report the
collected I/O errors without propagating a per-file error, so the per-namespace
println and total count still execute while preserving successful deletion
counts.
- Around line 1541-1551: Update the inbox ID normalization in cmd_namespace_show
and the corresponding namespace clean lookup so the user-facing value "default"
is converted to the internal "(default)" row ID before searching. Preserve
explicit non-default inbox IDs unchanged and retain the existing not-registered
error behavior.
- Around line 1301-1323: Update the counting block in enumerate_namespaces to
scan pending and answered directories once each, considering only .json files.
While scanning answered records, parse each JSON state and increment answered
only for non-Cancelled/non-Expired records, while incrementing expired for
Expired records; preserve pending as the count of JSON files.
In `@crates/elicitate/src/inbox/mod.rs`:
- Around line 242-255: Update resolve_inbox_root so valid namespace IDs are
joined directly beneath default_inbox_root(), removing the parent() derivation
and fallback. Preserve default_inbox_root() for None and "default";
additionally, avoid silently routing invalid IDs to the default inbox by
changing the API to return an appropriate Result and propagate validation errors
to callers.
- Around line 225-231: Update is_valid_inbox_id to reject non-lowercase
characters, ensuring accepted inbox IDs are already lowercase and therefore
consistent with resolve_inbox_root and namespace_port. Preserve the existing
length, non-empty, and allowed-character checks.
In `@crates/elicitate/src/installer.rs`:
- Around line 57-71: Update the install flow to detect duplicate ports assigned
by namespace_port before creating NamespaceAutostart units. Track ports already
assigned to other inbox IDs, emit a warning when a collision is found, and
ensure the duplicate is surfaced rather than silently treating both namespaces
as live; keep namespace_port’s deterministic mapping unchanged unless
implementing forward probing with the selected port recorded in
NamespaceAutostart.
- Around line 92-97: Update the doc comment for extra_inbox_ids to state that
invalid IDs are skipped and a warning is recorded in the install report,
matching the behavior of the registration loop.
- Around line 475-502: Escape all interpolated plist path values before
constructing the XML in the installer’s plist-generation flow. Apply the
five-entity XML escaping to cli_path and home (and any other user-derived
interpolated values such as label if applicable), then interpolate the escaped
strings while preserving the existing plist structure and fs::write behavior.
- Around line 556-568: Update the ExecStart construction in the installer flow
to quote the executable path derived from cli_path so spaces remain part of the
path, and escape every percent character by doubling it for systemd specifier
handling. Keep the existing daemon arguments and optional inbox-id formatting
unchanged.
- Around line 254-261: Move dry-run namespace reporting into the early-return
branch of install, using each requested namespace ID to populate
report.namespace_autostarts with namespace_port(id) and the existing dry-run
target values before returning. Remove the unreachable opts.dry_run block inside
the installation loop, and extend the dry-run test coverage to assert the
reported namespace ports.
- Around line 506-519: Update the Windows task naming logic around task_name to
use unsuffixed ElicitateDaemon for the default namespace, matching
autostart_unit_present. For non-default namespaces, preserve inbox_id verbatim
or apply a reversible encoding consistently with discover_namespace_ids; remove
the lossy hyphen-to-underscore mapping and update discovery accordingly so
namespace IDs, ports, and inbox roots round-trip correctly.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f851219a-88e0-455a-b572-f393c16113b7
📒 Files selected for processing (5)
crates/elicitate/CHANGELOG.mdcrates/elicitate/Cargo.tomlcrates/elicitate/src/bin_elicitate.rscrates/elicitate/src/inbox/mod.rscrates/elicitate/src/installer.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Check: Summary: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
GitHub Check: Mergify Merge Queue: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
🔇 Additional comments (9)
crates/elicitate/CHANGELOG.md (4)
16-18: 🩺 Stability & AvailabilityVerify that hashed namespace ports cannot conflict.
The documented range contains only 999 ports. A deterministic hash cannot assign every valid namespace ID a distinct port. The current test only compares
proj-aandproj-batcrates/elicitate/src/bin_elicitate.rsLines 1114-1127. If each namespace daemon requires a unique listener, add collision detection or resolution during installation. Otherwise, remove the claim that different IDs always produce distinct ports.Also applies to: 49-50
22-30: 🗄️ Data Integrity & IntegrationPreserve namespace identity for explicit commands.
is_valid_inbox_idacceptsdefault, whileresolve_inbox_rootmapsdefaultand hostile IDs to the legacy root. Verify that--register-namespace defaultis deduplicated instead of creating another autostart for the same inbox. Verify thatnamespace clean --inbox-id <invalid>rejects the value instead of deleting terminal entries from the legacy root.Also applies to: 36-38
78-81: 🎯 Functional CorrectnessVerify the public
mcpfeature before release.The notes state that
--features mcpfails to compile incrates/elicitate/src/mcp/router.rs. A clean--no-default-featuresbuild does not validate an exposed optional feature. Ifmcpremains part of the 0.3.0 feature surface, fix, disable, or explicitly mark it unsupported and cover that policy in CI.
8-15: LGTM!Also applies to: 19-21, 31-35, 39-42, 64-77, 83-86
crates/elicitate/src/bin_elicitate.rs (4)
1490-1504: The port-probe false-positive concern is already covered by thenamespace_portcollision comment oncrates/elicitate/src/installer.rs. No separate action here.
18-18: LGTM!Also applies to: 101-102, 367-367, 635-635
248-253: LGTM!
1360-1377: LGTM!crates/elicitate/src/installer.rs (1)
108-108: LGTM!Also applies to: 208-208
| [package] | ||
| name = "elicitate" | ||
| version = "0.2.0" | ||
| version = "0.3.0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Synchronize Cargo.lock with the manifest version.
crates/elicitate/Cargo.toml declares 0.3.0, but Cargo.lock still records elicitate as 0.2.0 at Lines 864-866. Regenerate and commit Cargo.lock before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/Cargo.toml` at line 3, Update the package version declared
by the elicitate manifest to its corresponding entry in Cargo.lock by
regenerating Cargo.lock after the 0.3.0 change. Ensure the locked elicitate
package records version 0.3.0 and commit the synchronized lockfile.
| ### Tests (12 new, all green) | ||
|
|
||
| - `parse_install_with_register_namespace` — `--register-namespace` accepts | ||
| repeated values | ||
| - `parse_namespace_list` / `parse_namespace_show` / `parse_namespace_clean_default_age` / | ||
| `parse_namespace_clean_with_age_and_dry_run` — CLI subcommand surface | ||
| - `namespace_port_distinct_and_deterministic` — same id → same port; | ||
| different ids → distinct ports; never collides with `DEFAULT_PORT` | ||
| - `namespace_port_falls_in_expected_range` — every namespace port falls in | ||
| `DEFAULT_PORT+1..=DEFAULT_PORT+999` | ||
| - `truncate_short_and_long` — table formatter correctness | ||
| - `enumerate_namespaces_default_only_when_no_units` — default row always | ||
| present | ||
| - `is_daemon_live_returns_false_for_unused_port` — smoke check | ||
| - `gc_namespace_dry_run_keeps_files` — `--dry-run` reports without | ||
| touching disk | ||
| - `is_valid_inbox_id_accepts_alphanumeric_dashes_underscores` — id shape | ||
| validator | ||
| - `resolve_inbox_root_default_and_namespace` — None / "default" / valid / | ||
| hostile all behave correctly |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the release-note test count.
The list contains 13 named tests, but the heading says 12.
Proposed correction
-### Tests (12 new, all green)
+### Tests (13 new, all green)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### Tests (12 new, all green) | |
| - `parse_install_with_register_namespace` — `--register-namespace` accepts | |
| repeated values | |
| - `parse_namespace_list` / `parse_namespace_show` / `parse_namespace_clean_default_age` / | |
| `parse_namespace_clean_with_age_and_dry_run` — CLI subcommand surface | |
| - `namespace_port_distinct_and_deterministic` — same id → same port; | |
| different ids → distinct ports; never collides with `DEFAULT_PORT` | |
| - `namespace_port_falls_in_expected_range` — every namespace port falls in | |
| `DEFAULT_PORT+1..=DEFAULT_PORT+999` | |
| - `truncate_short_and_long` — table formatter correctness | |
| - `enumerate_namespaces_default_only_when_no_units` — default row always | |
| present | |
| - `is_daemon_live_returns_false_for_unused_port` — smoke check | |
| - `gc_namespace_dry_run_keeps_files` — `--dry-run` reports without | |
| touching disk | |
| - `is_valid_inbox_id_accepts_alphanumeric_dashes_underscores` — id shape | |
| validator | |
| - `resolve_inbox_root_default_and_namespace` — None / "default" / valid / | |
| hostile all behave correctly | |
| ### Tests (13 new, all green) | |
| - `parse_install_with_register_namespace` — `--register-namespace` accepts | |
| repeated values | |
| - `parse_namespace_list` / `parse_namespace_show` / `parse_namespace_clean_default_age` / | |
| `parse_namespace_clean_with_age_and_dry_run` — CLI subcommand surface | |
| - `namespace_port_distinct_and_deterministic` — same id → same port; | |
| different ids → distinct ports; never collides with `DEFAULT_PORT` | |
| - `namespace_port_falls_in_expected_range` — every namespace port falls in | |
| `DEFAULT_PORT+1..=DEFAULT_PORT+999` | |
| - `truncate_short_and_long` — table formatter correctness | |
| - `enumerate_namespaces_default_only_when_no_units` — default row always | |
| present | |
| - `is_daemon_live_returns_false_for_unused_port` — smoke check | |
| - `gc_namespace_dry_run_keeps_files` — `--dry-run` reports without | |
| touching disk | |
| - `is_valid_inbox_id_accepts_alphanumeric_dashes_underscores` — id shape | |
| validator | |
| - `resolve_inbox_root_default_and_namespace` — None / "default" / valid / | |
| hostile all behave correctly |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/CHANGELOG.md` around lines 43 - 62, Update the “Tests”
heading in the changelog to report 13 new tests, matching the 13 named test
entries listed below it.
| /// Inspect and manage inbox namespaces. v0.18.x ships a per-namespace | ||
| /// installer (--register-namespace) and CLI flag (--inbox-id), but the | ||
| /// runtime needs a way to see what's running, where each inbox lives, and | ||
| /// how to clean up expired entries across many namespaces at once. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale version reference in the doc comment.
The comment cites "v0.18.x". This PR bumps the crate from 0.2.0 to 0.3.0. A matching comment at line 1280 cites "v0.19.0". Neither version exists in this crate's history. Remove the version references or replace them with 0.3.0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/bin_elicitate.rs` around lines 184 - 187, Update the
documentation comment near the inbox namespace management description to remove
the inaccurate “v0.18.x” reference, or replace it with the crate’s current
version “0.3.0”; keep the rest of the comment unchanged and apply the same
correction to the matching “v0.19.0” reference.
| #[test] | ||
| fn enumerate_namespaces_default_only_when_no_units() { | ||
| let rows = enumerate_namespaces(Path::new("/tmp/no-such-inbox")); | ||
| assert_eq!(rows.len(), 1, "default row must always be present"); | ||
| assert_eq!(rows[0].inbox_id, "(default)"); | ||
| assert_eq!(rows[0].port, elicitate::inbox::daemon::DEFAULT_PORT); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
This test depends on the developer's machine state.
enumerate_namespaces calls discover_namespace_ids, which reads the real ~/Library/LaunchAgents on macOS and the real ~/.config/systemd/user on Linux. The default_inbox_root argument does not isolate that lookup. On any machine where elicitate install --register-namespace ran, rows.len() exceeds 1 and this test fails.
Override HOME for the test with a tempfile::tempdir, or extract the unit-directory lookup into a function that accepts the search root.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/bin_elicitate.rs` around lines 1146 - 1152, Make
enumerate_namespaces_default_only_when_no_units deterministic by isolating
discover_namespace_ids from the developer’s machine state: set HOME to a
tempfile::tempdir for the test, or refactor the unit-directory lookup to accept
an injected search root. Ensure the test still observes only the default
namespace and does not read real LaunchAgents or systemd user directories.
| let pending = std::fs::read_dir(elicitate::inbox::inbox_pending_dir(&inbox_root)) | ||
| .map(|d| d.filter_map(|e| e.ok()).count()) | ||
| .unwrap_or(0); | ||
| let answered = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) | ||
| .map(|d| d.filter_map(|e| e.ok()).count()) | ||
| .unwrap_or(0); | ||
|
|
||
| let mut expired = 0usize; | ||
| if let Ok(entries) = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) { | ||
| for e in entries.flatten() { | ||
| let path = e.path(); | ||
| if path.extension().and_then(|s| s.to_str()) != Some("json") { | ||
| continue; | ||
| } | ||
| if let Ok(text) = std::fs::read_to_string(&path) { | ||
| if let Ok(v) = serde_json::from_str::<serde_json::Value>(&text) { | ||
| if v.get("state").and_then(|s| s.as_str()) == Some("Expired") { | ||
| expired += 1; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
answered over-counts, and the directory is scanned three times.
Two problems in this block.
The answered count includes every entry in the answered directory. That directory also holds Cancelled and Expired records, and it can hold non-JSON files. The expired count is then derived from the same directory. A reader of elicitate namespace list sees ANSW and EXPD columns whose sum exceeds the record count. The pending count has the same non-JSON problem.
The answered directory is also read twice, and every JSON file is read and parsed to compute expired. enumerate_namespaces repeats this for each namespace on every namespace list, namespace show, and namespace clean invocation.
Compute all three counts in one pass and filter on the .json extension.
♻️ Proposed single-pass counting
- let pending = std::fs::read_dir(elicitate::inbox::inbox_pending_dir(&inbox_root))
- .map(|d| d.filter_map(|e| e.ok()).count())
- .unwrap_or(0);
- let answered = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root))
- .map(|d| d.filter_map(|e| e.ok()).count())
- .unwrap_or(0);
-
- let mut expired = 0usize;
- if let Ok(entries) = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) {
- for e in entries.flatten() {
- let path = e.path();
- if path.extension().and_then(|s| s.to_str()) != Some("json") {
- continue;
- }
- if let Ok(text) = std::fs::read_to_string(&path) {
- if let Ok(v) = serde_json::from_str::<serde_json::Value>(&text) {
- if v.get("state").and_then(|s| s.as_str()) == Some("Expired") {
- expired += 1;
- }
- }
- }
- }
- }
+ let is_json = |p: &Path| p.extension().and_then(|s| s.to_str()) == Some("json");
+
+ let pending = std::fs::read_dir(elicitate::inbox::inbox_pending_dir(&inbox_root))
+ .map(|d| d.flatten().filter(|e| is_json(&e.path())).count())
+ .unwrap_or(0);
+
+ let mut answered = 0usize;
+ let mut expired = 0usize;
+ if let Ok(entries) = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) {
+ for e in entries.flatten() {
+ let path = e.path();
+ if !is_json(&path) {
+ continue;
+ }
+ let state = std::fs::read_to_string(&path)
+ .ok()
+ .and_then(|t| serde_json::from_str::<serde_json::Value>(&t).ok())
+ .and_then(|v| v.get("state").and_then(|s| s.as_str()).map(str::to_owned));
+ match state.as_deref() {
+ Some("Expired") => expired += 1,
+ Some("Answered") => answered += 1,
+ _ => {}
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let pending = std::fs::read_dir(elicitate::inbox::inbox_pending_dir(&inbox_root)) | |
| .map(|d| d.filter_map(|e| e.ok()).count()) | |
| .unwrap_or(0); | |
| let answered = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) | |
| .map(|d| d.filter_map(|e| e.ok()).count()) | |
| .unwrap_or(0); | |
| let mut expired = 0usize; | |
| if let Ok(entries) = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) { | |
| for e in entries.flatten() { | |
| let path = e.path(); | |
| if path.extension().and_then(|s| s.to_str()) != Some("json") { | |
| continue; | |
| } | |
| if let Ok(text) = std::fs::read_to_string(&path) { | |
| if let Ok(v) = serde_json::from_str::<serde_json::Value>(&text) { | |
| if v.get("state").and_then(|s| s.as_str()) == Some("Expired") { | |
| expired += 1; | |
| } | |
| } | |
| } | |
| } | |
| } | |
| let is_json = |p: &Path| p.extension().and_then(|s| s.to_str()) == Some("json"); | |
| let pending = std::fs::read_dir(elicitate::inbox::inbox_pending_dir(&inbox_root)) | |
| .map(|d| d.flatten().filter(|e| is_json(&e.path())).count()) | |
| .unwrap_or(0); | |
| let mut answered = 0usize; | |
| let mut expired = 0usize; | |
| if let Ok(entries) = std::fs::read_dir(elicitate::inbox::answered_dir(&inbox_root)) { | |
| for e in entries.flatten() { | |
| let path = e.path(); | |
| if !is_json(&path) { | |
| continue; | |
| } | |
| let state = std::fs::read_to_string(&path) | |
| .ok() | |
| .and_then(|t| serde_json::from_str::<serde_json::Value>(&t).ok()) | |
| .and_then(|v| v.get("state").and_then(|s| s.as_str()).map(str::to_owned)); | |
| match state.as_deref() { | |
| Some("Expired") => expired += 1, | |
| Some("Answered") => answered += 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 `@crates/elicitate/src/bin_elicitate.rs` around lines 1301 - 1323, Update the
counting block in enumerate_namespaces to scan pending and answered directories
once each, considering only .json files. While scanning answered records, parse
each JSON state and increment answered only for non-Cancelled/non-Expired
records, while incrementing expired for Expired records; preserve pending as the
count of JSON files.
| /// Extra inbox namespace ids whose daemons should be registered alongside | ||
| /// the default one. Each namespace gets its own LaunchAgent / systemd unit | ||
| /// / scheduled task with a deterministic port (`7117 + hash(id) % 999 + 1`). | ||
| /// Invalid ids (per [`crate::inbox::is_valid_inbox_id`]) are silently | ||
| /// skipped at install time. | ||
| pub extra_inbox_ids: Vec<String>, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment contradicts the implementation.
The comment states that invalid IDs are "silently skipped". The loop at lines 247-253 pushes a warning into report.warnings. Update the comment.
📝 Proposed doc fix
- /// Invalid ids (per [`crate::inbox::is_valid_inbox_id`]) are silently
- /// skipped at install time.
+ /// Invalid ids (per [`crate::inbox::is_valid_inbox_id`]) are skipped at
+ /// install time and reported in `InstallReport::warnings`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Extra inbox namespace ids whose daemons should be registered alongside | |
| /// the default one. Each namespace gets its own LaunchAgent / systemd unit | |
| /// / scheduled task with a deterministic port (`7117 + hash(id) % 999 + 1`). | |
| /// Invalid ids (per [`crate::inbox::is_valid_inbox_id`]) are silently | |
| /// skipped at install time. | |
| pub extra_inbox_ids: Vec<String>, | |
| /// Extra inbox namespace ids whose daemons should be registered alongside | |
| /// the default one. Each namespace gets its own LaunchAgent / systemd unit | |
| /// / scheduled task with a deterministic port (`7117 + hash(id) % 999 + 1`). | |
| /// Invalid ids (per [`crate::inbox::is_valid_inbox_id`]) are skipped at | |
| /// install time and reported in `InstallReport::warnings`. | |
| pub extra_inbox_ids: Vec<String>, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/installer.rs` around lines 92 - 97, Update the doc
comment for extra_inbox_ids to state that invalid IDs are skipped and a warning
is recorded in the install report, matching the behavior of the registration
loop.
| if opts.dry_run { | ||
| report.namespace_autostarts.push(NamespaceAutostart { | ||
| inbox_id: id.clone(), | ||
| port: namespace_port(id), | ||
| target: PathBuf::new(), | ||
| }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unreachable dry-run branch; --dry-run never reports namespace ports.
install returns early at line 217 when opts.dry_run is true. Execution never reaches line 235, so this if opts.dry_run block inside the loop is dead code.
The user-visible effect is that elicitate install --dry-run --register-namespace proj-a prints an empty namespace_autostarts array. The deterministic port preview is lost, which is the main reason to run the dry run.
Populate namespace_autostarts in the early dry-run branch instead, and remove the dead block here. The existing dry-run test at lines 606-616 does not cover namespaces; add a case that asserts the ports.
🐛 Proposed fix
Populate the report before the early return:
if opts.dry_run {
// In dry-run, populate the fields without touching the filesystem.
report.cli_path = bin_dir.join(if cfg!(windows) { "elicitate.exe" } else { "elicitate" });
report.mcp_path = bin_dir.join(if cfg!(windows) { "elicitate-mcp.exe" } else { "elicitate-mcp" });
+ for id in &opts.extra_inbox_ids {
+ if !crate::inbox::is_valid_inbox_id(id) {
+ report
+ .warnings
+ .push(format!("skip namespace {id:?}: invalid id"));
+ continue;
+ }
+ report.namespace_autostarts.push(NamespaceAutostart {
+ inbox_id: id.clone(),
+ port: namespace_port(id),
+ target: PathBuf::new(),
+ });
+ }
return Ok(report);
}Then delete the unreachable block:
- if opts.dry_run {
- report.namespace_autostarts.push(NamespaceAutostart {
- inbox_id: id.clone(),
- port: namespace_port(id),
- target: PathBuf::new(),
- });
- continue;
- }
let port = namespace_port(id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if opts.dry_run { | |
| report.namespace_autostarts.push(NamespaceAutostart { | |
| inbox_id: id.clone(), | |
| port: namespace_port(id), | |
| target: PathBuf::new(), | |
| }); | |
| continue; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/installer.rs` around lines 254 - 261, Move dry-run
namespace reporting into the early-return branch of install, using each
requested namespace ID to populate report.namespace_autostarts with
namespace_port(id) and the existing dry-run target values before returning.
Remove the unreachable opts.dry_run block inside the installation loop, and
extend the dry-run test coverage to assert the reported namespace ports.
| let xml = format!( | ||
| r#"<?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> | ||
| <plist version="1.0"> | ||
| <dict> | ||
| <key>Label</key><string>com.phenotype.elicitate</string> | ||
| <key>Label</key><string>{label}</string> | ||
| <key>ProgramArguments</key> | ||
| <array> | ||
| <string>{}</string> | ||
| <string>{cli}</string> | ||
| <string>daemon</string> | ||
| <string>--port</string><string>{port}</string> | ||
| {inbox_args} | ||
| </array> | ||
| <key>RunAtLoad</key><true/> | ||
| <key>KeepAlive</key><true/> | ||
| <key>StandardOutPath</key><string>{}/Library/Logs/elicitate.out</string> | ||
| <key>StandardErrorPath</key><string>{}/Library/Logs/elicitate.err</string> | ||
| <key>StandardOutPath</key><string>{home}/Library/Logs/elicitate{label_suffix}.out</string> | ||
| <key>StandardErrorPath</key><string>{home}/Library/Logs/elicitate{label_suffix}.err</string> | ||
| </dict> | ||
| </plist> | ||
| "#, | ||
| cli_path.display(), | ||
| home.display(), | ||
| home.display(), | ||
| label = label, | ||
| cli = cli_path.display(), | ||
| port = port, | ||
| inbox_args = inbox_args, | ||
| home = home.display(), | ||
| label_suffix = label_suffix, | ||
| ); | ||
| fs::write(&plist, xml).map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Escape interpolated paths in the plist XML.
cli_path and home are interpolated into the plist without XML escaping. cli_path derives from the user-supplied --prefix. A path that contains &, <, or > produces a malformed plist. launchd then rejects the file and the daemon never starts. The install reports success, because fs::write succeeds.
Escape the five XML entities before interpolation, or build the plist with a plist serialization crate.
🛡️ Proposed minimal escaping helper
fn xml_escape(s: &str) -> String {
s.replace('&', "&")
.replace('<', "<")
.replace('>', ">")
.replace('"', """)
.replace('\'', "'")
}- cli = cli_path.display(),
+ cli = xml_escape(&cli_path.display().to_string()),
port = port,
inbox_args = inbox_args,
- home = home.display(),
+ home = xml_escape(&home.display().to_string()),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/installer.rs` around lines 475 - 502, Escape all
interpolated plist path values before constructing the XML in the installer’s
plist-generation flow. Apply the five-entity XML escaping to cli_path and home
(and any other user-derived interpolated values such as label if applicable),
then interpolate the escaped strings while preserving the existing plist
structure and fs::write behavior.
| #[cfg(target_os = "windows")] | ||
| { | ||
| let task_name = format!( | ||
| "ElicitateDaemon.{}", | ||
| inbox_id.unwrap_or("default").replace('-', "_") | ||
| ); | ||
| let tr_args = format!( | ||
| "\"{}\" daemon --port {}{}", | ||
| cli_path.display(), | ||
| port, | ||
| inbox_id | ||
| .map(|id| format!(" --inbox-id {id}")) | ||
| .unwrap_or_default() | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Windows task naming breaks the discovery contract in two ways.
First, the default task name does not match the reader. This code names the default task ElicitateDaemon.default, because inbox_id.unwrap_or("default") applies. autostart_unit_present in crates/elicitate/src/bin_elicitate.rs (lines 1466-1469) queries ElicitateDaemon for the default namespace. The default row therefore always reports autostart_present: false on Windows. The macOS and systemd branches use the unsuffixed name for the default, so the Windows branch is the outlier.
Second, the - to _ mapping is lossy and not reversible. discover_namespace_ids in crates/elicitate/src/bin_elicitate.rs (line 1410) reverses it with replace('_', "-"). An ID such as team_alpha is written as ElicitateDaemon.team_alpha and read back as team-alpha. That is a different namespace ID, so namespace_port and resolve_inbox_root return a different port and a different inbox root than the installed daemon uses. IDs that contain - and _ also collide with each other.
Use the unsuffixed ElicitateDaemon for the default namespace. Keep the ID verbatim in the task name, or apply a reversible encoding on both sides.
🐛 Proposed naming fix
- let task_name = format!(
- "ElicitateDaemon.{}",
- inbox_id.unwrap_or("default").replace('-', "_")
- );
+ let task_name = match inbox_id {
+ Some(id) => format!("ElicitateDaemon.{id}"),
+ None => "ElicitateDaemon".to_string(),
+ };Apply the matching change in crates/elicitate/src/bin_elicitate.rs:
let task = match inbox_id {
- Some(id) => format!("ElicitateDaemon.{}", id.replace('-', "_")),
+ Some(id) => format!("ElicitateDaemon.{id}"),
None => "ElicitateDaemon".to_string(),
}; if let Some(rest) = line.strip_prefix("ElicitateDaemon.") {
if !rest.is_empty() {
- ids.push(rest.replace('_', "-"));
+ ids.push(rest.to_string());
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(target_os = "windows")] | |
| { | |
| let task_name = format!( | |
| "ElicitateDaemon.{}", | |
| inbox_id.unwrap_or("default").replace('-', "_") | |
| ); | |
| let tr_args = format!( | |
| "\"{}\" daemon --port {}{}", | |
| cli_path.display(), | |
| port, | |
| inbox_id | |
| .map(|id| format!(" --inbox-id {id}")) | |
| .unwrap_or_default() | |
| ); | |
| #[cfg(target_os = "windows")] | |
| { | |
| let task_name = match inbox_id { | |
| Some(id) => format!("ElicitateDaemon.{id}"), | |
| None => "ElicitateDaemon".to_string(), | |
| }; | |
| let tr_args = format!( | |
| "\"{}\" daemon --port {}{}", | |
| cli_path.display(), | |
| port, | |
| inbox_id | |
| .map(|id| format!(" --inbox-id {id}")) | |
| .unwrap_or_default() | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/installer.rs` around lines 506 - 519, Update the Windows
task naming logic around task_name to use unsuffixed ElicitateDaemon for the
default namespace, matching autostart_unit_present. For non-default namespaces,
preserve inbox_id verbatim or apply a reversible encoding consistently with
discover_namespace_ids; remove the lossy hyphen-to-underscore mapping and update
discovery accordingly so namespace IDs, ports, and inbox roots round-trip
correctly.
| let exec_args = format!( | ||
| "{} daemon --port {}{}", | ||
| cli_path.display(), | ||
| port, | ||
| inbox_id | ||
| .map(|id| format!(" --inbox-id {id}")) | ||
| .unwrap_or_default() | ||
| ); | ||
| let body = format!( | ||
| "[Unit]\nDescription=Elicitate inbox daemon\nAfter=network.target\n\n\ | ||
| [Service]\nExecStart={} daemon\nRestart=on-failure\n\n\ | ||
| "[Unit]\nDescription={description}\nAfter=network.target\n\n\ | ||
| [Service]\nExecStart={exec_args}\nRestart=on-failure\n\n\ | ||
| [Install]\nWantedBy=default.target\n", | ||
| cli_path.display() | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote ExecStart and escape % for systemd.
systemd splits ExecStart on whitespace. A cli_path that contains a space produces a broken command line. systemd also expands % as a specifier prefix, so a % in the path corrupts the value. cli_path derives from the user-supplied --prefix, so both characters are reachable.
Quote the executable path and double each %.
🐛 Proposed fix
let exec_args = format!(
- "{} daemon --port {}{}",
- cli_path.display(),
+ "\"{}\" daemon --port {}{}",
+ cli_path.display().to_string().replace('%', "%%"),
port,
inbox_id
.map(|id| format!(" --inbox-id {id}"))
.unwrap_or_default()
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let exec_args = format!( | |
| "{} daemon --port {}{}", | |
| cli_path.display(), | |
| port, | |
| inbox_id | |
| .map(|id| format!(" --inbox-id {id}")) | |
| .unwrap_or_default() | |
| ); | |
| let body = format!( | |
| "[Unit]\nDescription=Elicitate inbox daemon\nAfter=network.target\n\n\ | |
| [Service]\nExecStart={} daemon\nRestart=on-failure\n\n\ | |
| "[Unit]\nDescription={description}\nAfter=network.target\n\n\ | |
| [Service]\nExecStart={exec_args}\nRestart=on-failure\n\n\ | |
| [Install]\nWantedBy=default.target\n", | |
| cli_path.display() | |
| ); | |
| let exec_args = format!( | |
| "\"{}\" daemon --port {}{}", | |
| cli_path.display().to_string().replace('%', "%%"), | |
| port, | |
| inbox_id | |
| .map(|id| format!(" --inbox-id {id}")) | |
| .unwrap_or_default() | |
| ); | |
| let body = format!( | |
| "[Unit]\nDescription={description}\nAfter=network.target\n\n\ | |
| [Service]\nExecStart={exec_args}\nRestart=on-failure\n\n\ | |
| [Install]\nWantedBy=default.target\n", | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/installer.rs` around lines 556 - 568, Update the
ExecStart construction in the installer flow to quote the executable path
derived from cli_path so spaces remain part of the path, and escape every
percent character by doubling it for systemd specifier handling. Keep the
existing daemon arguments and optional inbox-id formatting unchanged.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if v.get("state").and_then(|s| s.as_str()) == Some("Expired") { | ||
| expired += 1; | ||
| } |
There was a problem hiding this comment.
Suggestion: Persisted RequestState values use snake_case serialization, so terminal records contain answered, cancelled, or expired, not the Rust variant names. This comparison never counts real expired records; compare against the serialized lowercase values or deserialize into RequestState. [data type]
Severity Level: Major ⚠️
- ❌ Namespace list underreports expired entries.
- ⚠️ Operators receive incorrect inbox activity statistics.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/bin_elicitate.rs
**Line:** 1317:1319
**Comment:**
*Data Type: Persisted `RequestState` values use `snake_case` serialization, so terminal records contain `answered`, `cancelled`, or `expired`, not the Rust variant names. This comparison never counts real expired records; compare against the serialized lowercase values or deserialize into `RequestState`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let port = elicitate::installer::namespace_port(&id); | ||
| let root = elicitate::inbox::resolve_inbox_root(Some(&id)); | ||
| rows.push(build_namespace_row(Some(id), root, port)); |
There was a problem hiding this comment.
Suggestion: Namespace enumeration accepts the caller's default_inbox_root for the default row but recomputes each namespace root through resolve_inbox_root, which reads the process environment instead of the supplied path. When the CLI uses a custom --inbox-dir, list/show/clean therefore inspect the default environment-derived namespace directories rather than the user's selected data root. Resolve namespaces relative to default_inbox_root or pass the selected root into the resolver. [api mismatch]
Severity Level: Major ⚠️
- ❌ Namespace list/show inspect the wrong data root.
- ❌ Namespace clean can leave custom-root records untouched.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/bin_elicitate.rs
**Line:** 1372:1374
**Comment:**
*Api Mismatch: Namespace enumeration accepts the caller's `default_inbox_root` for the default row but recomputes each namespace root through `resolve_inbox_root`, which reads the process environment instead of the supplied path. When the CLI uses a custom `--inbox-dir`, list/show/clean therefore inspect the default environment-derived namespace directories rather than the user's selected data root. Resolve namespaces relative to `default_inbox_root` or pass the selected root into the resolver.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if let Some(rest) = line.strip_prefix("ElicitateDaemon.") { | ||
| if !rest.is_empty() { | ||
| ids.push(rest.replace('_', "-")); | ||
| } |
There was a problem hiding this comment.
Suggestion: On Windows, installation preserves underscores in the scheduled-task name, but namespace discovery converts every underscore to a hyphen. A valid ID such as team_alpha is registered as ElicitateDaemon.team_alpha and later discovered as team-alpha, so namespace list/show/clean cannot find the installed namespace. Use an unambiguous reversible encoding for task names or stop rewriting underscores during discovery. [inconsistent naming]
Severity Level: Major ⚠️
- ❌ Windows namespace list reports incorrect identifiers.
- ❌ Show and clean cannot target the registered ID reliably.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/bin_elicitate.rs
**Line:** 1408:1411
**Comment:**
*Inconsistent Naming: On Windows, installation preserves underscores in the scheduled-task name, but namespace discovery converts every underscore to a hyphen. A valid ID such as `team_alpha` is registered as `ElicitateDaemon.team_alpha` and later discovered as `team-alpha`, so namespace list/show/clean cannot find the installed namespace. Use an unambiguous reversible encoding for task names or stop rewriting underscores during discovery.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if let Some(target) = &inbox_id { | ||
| if &row.inbox_id != target { | ||
| continue; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The default row is represented internally as "(default)", but the CLI's natural --inbox-id default value is not normalized before filtering. namespace clean --inbox-id default therefore matches no row and silently cleans nothing, despite resolve_inbox_root(Some("default")) treating default as the legacy inbox. Normalize default to the internal default-row identifier or compare against both values. [api mismatch]
Severity Level: Major ⚠️
- ❌ Default namespace cleanup silently does nothing.
- ⚠️ Users must use an undocumented internal identifier.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/bin_elicitate.rs
**Line:** 1593:1597
**Comment:**
*Api Mismatch: The default row is represented internally as `"(default)"`, but the CLI's natural `--inbox-id default` value is not normalized before filtering. `namespace clean --inbox-id default` therefore matches no row and silently cleans nothing, despite `resolve_inbox_root(Some("default"))` treating `default` as the legacy inbox. Normalize `default` to the internal default-row identifier or compare against both values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let is_terminal = matches!(state, "Answered" | "Cancelled" | "Expired"); | ||
| if !is_terminal { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Suggestion: The garbage collector makes the same casing mistake for every terminal state. Since persisted values are lowercase snake-case, namespace clean skips all answered, cancelled, and expired records and leaves them on disk. Use the serialized state values or deserialize the field into RequestState. [data type]
Severity Level: Major ⚠️
- ❌ Namespace clean removes no normal terminal records.
- ⚠️ Answered-directory audit data grows indefinitely.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/bin_elicitate.rs
**Line:** 1641:1644
**Comment:**
*Data Type: The garbage collector makes the same casing mistake for every terminal state. Since persisted values are lowercase snake-case, `namespace clean` skips all answered, cancelled, and expired records and leaves them on disk. Use the serialized state values or deserialize the field into `RequestState`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let offset = (h % 999) + 1; // 1..=999 | ||
| crate::inbox::daemon::DEFAULT_PORT.saturating_add(offset as u16) |
There was a problem hiding this comment.
Suggestion: Only 999 hash buckets are available for an unbounded set of valid namespace IDs, so distinct namespaces can receive the same port. Both autostart registrations are then created, but one daemon fails to bind and the install report gives no collision warning. Maintain a persistent collision-free allocation or detect duplicate ports before registering the daemons. [possible bug]
Severity Level: Major ⚠️
- ❌ One namespace daemon fails to bind after collision.
- ⚠️ Installation provides no collision warning or remediation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/installer.rs
**Line:** 69:70
**Comment:**
*Possible Bug: Only 999 hash buckets are available for an unbounded set of valid namespace IDs, so distinct namespaces can receive the same port. Both autostart registrations are then created, but one daemon fails to bind and the install report gives no collision warning. Maintain a persistent collision-free allocation or detect duplicate ports before registering the daemons.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| for id in &opts.extra_inbox_ids { | ||
| if !crate::inbox::is_valid_inbox_id(id) { | ||
| report | ||
| .warnings |
There was a problem hiding this comment.
Suggestion: Installation creates one persistent autostart registration for every valid extra_inbox_id, but uninstall still removes only the default registration. After installing any namespace, the documented uninstall leaves its daemon unit/task/LaunchAgent behind and it can continue running after the binaries are removed. Track and remove the namespace registrations during uninstall, or make uninstall discover them using the same platform-specific naming scheme. [missing cleanup]
Severity Level: Major ⚠️
- ❌ Uninstall leaves namespace autostarts installed.
- ⚠️ Removed binaries can generate repeated supervisor failures.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/installer.rs
**Line:** 247:250
**Comment:**
*Missing Cleanup: Installation creates one persistent autostart registration for every valid `extra_inbox_id`, but `uninstall` still removes only the default registration. After installing any namespace, the documented uninstall leaves its daemon unit/task/LaunchAgent behind and it can continue running after the binaries are removed. Track and remove the namespace registrations during uninstall, or make uninstall discover them using the same platform-specific naming scheme.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| inbox_id | ||
| .map(|id| format!(" --inbox-id {id}")) | ||
| .unwrap_or_default() | ||
| ); |
There was a problem hiding this comment.
Suggestion: The generated autostart commands pass --inbox-id, but DaemonArgs does not define that option and cmd_daemon always uses the single inbox_dir argument. Every namespace daemon therefore exits immediately with an unknown-argument error instead of serving its namespace. Add a daemon namespace option and resolve the corresponding inbox root before starting the daemon, or remove this argument and configure the root another way. [api mismatch]
Severity Level: Critical 🚨
- ❌ Registered namespace daemons exit at startup.
- ❌ Per-namespace inboxes never receive daemon service.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/installer.rs
**Line:** 516:519
**Comment:**
*Api Mismatch: The generated autostart commands pass `--inbox-id`, but `DaemonArgs` does not define that option and `cmd_daemon` always uses the single `inbox_dir` argument. Every namespace daemon therefore exits immediately with an unknown-argument error instead of serving its namespace. Add a daemon namespace option and resolve the corresponding inbox root before starting the daemon, or remove this argument and configure the root another way.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let exec_args = format!( | ||
| "{} daemon --port {}{}", | ||
| cli_path.display(), | ||
| port, | ||
| inbox_id |
There was a problem hiding this comment.
Suggestion: The systemd unit embeds cli_path.display() directly in ExecStart. A valid installation prefix containing spaces is parsed as multiple command-line tokens, so systemd cannot execute the installed binary and the service fails to start. Escape or quote the executable path using systemd's command-line quoting rules. [api mismatch]
Severity Level: Major ⚠️
- ❌ Systemd namespace daemon fails with spaced prefixes.
- ⚠️ Installed autostart appears present but is unusable.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/installer.rs
**Line:** 556:560
**Comment:**
*Api Mismatch: The systemd unit embeds `cli_path.display()` directly in `ExecStart`. A valid installation prefix containing spaces is parsed as multiple command-line tokens, so systemd cannot execute the installed binary and the service fails to start. Escape or quote the executable path using systemd's command-line quoting rules.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let _ = Command::new("systemctl") | ||
| .args(["--user", "enable", "--now", "elicitate.service"]) | ||
| .args(["--user", "enable", "--now", &unit_name]) | ||
| .status(); | ||
| Ok(unit) |
There was a problem hiding this comment.
Suggestion: The result of systemctl --user enable --now is discarded and the function returns Ok(unit) regardless of whether systemd accepted the unit or started the daemon. Installation consequently records a namespace autostart as successful while reporting no warning when the service fails. Check the command result and return an error on a non-success status. [error handling]
Severity Level: Major ⚠️
- ❌ Install reports failed namespace startup as successful.
- ⚠️ Operators receive misleading autostart status.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/elicitate/src/installer.rs
**Line:** 570:573
**Comment:**
*Error Handling: The result of `systemctl --user enable --now` is discarded and the function returns `Ok(unit)` regardless of whether systemd accepted the unit or started the daemon. Installation consequently records a namespace autostart as successful while reporting no warning when the service fails. Check the command result and return an error on a non-success status.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixRewrites crates/elicitate/src/mcp/router.rs to work against rmcp 1.4. Main has been on rmcp 1.4 since #260, but the router code was still written against the rmcp 0.2 API, which broke the --features mcp build. This commit makes --features mcp compile cleanly and the elicitate-mcp binary successfully complete an MCP initialize handshake. What changed ------------ * rmcp::Error → rmcp::ErrorData (alias deprecated since 0.13) * Parameters import: handler::server::tool::Parameters → handler::server::wrapper::Parameters * ServerInfo / Implementation non-exhaustive struct fields: - Use ServerInfo::new(ServerCapabilities::default()) .with_server_info(Implementation::new("elicitate", version)) - Cannot construct ServerInfo { server_info: Implementation { … } } directly * Content::json returns Result<Self, ErrorData> — propagate the error * CallToolResult: use success(vec) / error(vec) constructors instead of struct literal with is_error field * #[tool(...)] macro: provide explicit input_schema and output_schema attributes since the macro's auto-inference tries to call schema_for_input which doesn't exist in rmcp 1.4 Dep bump -------- * schemars 0.8 → 1.0 in elicitate/Cargo.toml Required: rmcp 1.4 pulls schemars 1.0 internally. Without the bump, the JsonSchema derive macro expanded against the 0.8 source while rmcp::schemars resolved to 1.0, producing confusing 'argument #5 missing' errors. Tests ----- * Added: params_roundtrip_via_spec — ElicitateParams → PromptSpec * Existing: 121/121 still green with --features mcp (70 lib + 26 bin + 14 plugin + 6 lib-int + 4 mcp_stdio + 1 router) * New total: 122/122 Verification ------------ $ echo '{"jsonrpc":"2.0","id":1,"method":"initialize",...}' \ | ./target/debug/elicitate-mcp {"jsonrpc":"2.0","id":1,"result":{"protocolVersion":"2024-11-05", "capabilities":{},"serverInfo":{"name":"elicitate","version":"0.4.0"}}} End-to-end MCP handshake works. Unblocked follow-up ------------------- This unblocks porting the rmcp-coupled features from wip/2026-07-22-phenotype-tooling-absorbed-go-mod: * v0.13.0 elicitate_reply MCP tool * v0.14.0 multi-inbox (MCP) routing * v0.16.0 elicitate_enqueue MCP tool * v0.17.0 elicitate_cancel MCP tool Each will need a similar rmcp 1.4 adapter as it's ported. Version bumped 0.3.0 → 0.4.0.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/elicitate/CHANGELOG.md`:
- Line 34: Update the fenced command-and-output block near the reported
changelog entry to specify the console language identifier, changing the opening
fence to use console while preserving its contents.
In `@crates/elicitate/src/mcp/router.rs`:
- Around line 178-198: The params_roundtrip_via_spec test only verifies some
converted fields. Add assertions covering question, field (including its Boolean
label and default), and urgency, while retaining the existing checks; rename the
test to reflect one-way conversion if it does not actually round-trip back to
ElicitateParams.
- Around line 94-101: The elicit method declares ElicitResponse as its output
schema but currently returns unstructured CallToolResult values. Update its
success and failure paths to use CallToolResult::structured(...) and
CallToolResult::structured_error(...) with typed ElicitResponse-compatible
payloads, preserving the existing response behavior and error semantics.
- Around line 147-152: Update ServerHandler::get_info to construct capabilities
with ServerCapabilities::builder().enable_tools().build() instead of
ServerCapabilities::default(), while preserving the existing server identity
metadata.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0ed2847f-b26b-4ea2-938c-9a799c9d086f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/elicitate/CHANGELOG.mdcrates/elicitate/Cargo.tomlcrates/elicitate/src/mcp/router.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Summary
⚠️ CI failures not shown inline (2)
GitHub Check: Summary: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
GitHub Check: Mergify Merge Queue: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
🧰 Additional context used
🪛 markdownlint-cli2 (0.23.2)
crates/elicitate/CHANGELOG.md
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
crates/elicitate/CHANGELOG.md (1)
54-132: 📐 Maintainability & Code QualityRecheck the 0.3.0 test count.
A previous review reported a mismatch between the heading and the named tests. Confirm that the final 0.3.0 entry reports the number of tests that it lists.
#!/usr/bin/env bash set -euo pipefail sed -n '54,132p' crates/elicitate/CHANGELOG.mdcrates/elicitate/Cargo.toml (1)
3-3: 🗄️ Data Integrity & IntegrationVerify Cargo.lock after the 0.4.0 version bump.
crates/elicitate/Cargo.tomlnow declares0.4.0, but the current review context does not includeCargo.lock. Confirm that itselicitatepackage entry also declares0.4.0before publishing.#!/usr/bin/env bash set -euo pipefail lockfile="$(fd -a -t f '^Cargo\.lock$' . | head -n1)" test -n "$lockfile" manifest_version="$(sed -n 's/^version = "\(.*\)"/\1/p' crates/elicitate/Cargo.toml | head -n1)" lock_version="$( awk ' $0 == "name = \"elicitate\"" { found=1; next } found && $0 ~ /^version = "/ { gsub(/"/, "", $0) sub(/^version = /, "", $0) print exit } ' "$lockfile" )" test "$manifest_version" = "$lock_version"crates/elicitate/src/mcp/router.rs (1)
3-29: LGTM!Also applies to: 116-116
|
|
||
| ### Verification | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced block.
markdownlint reports MD040 at Line 34. Use console for this command-and-output block.
Proposed fix
-```
+```console📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 34-34: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/CHANGELOG.md` at line 34, Update the fenced
command-and-output block near the reported changelog entry to specify the
console language identifier, changing the opening fence to use console while
preserving its contents.
Source: Linters/SAST tools
| description = "Render a native OS popup and block until the human operator responds (or the prompt times out). Use this whenever an autonomous agent needs a single, structured decision from a human: a confirmation, a multi-choice selection, a secret, a disambiguation. Returns a typed JSON ElicitResponse.", | ||
| input_schema = rmcp::handler::server::tool::schema_for_type::<ElicitateParams>(), | ||
| output_schema = rmcp::handler::server::tool::schema_for_type::<crate::spec::ElicitResponse>() | ||
| )] | ||
| async fn elicit( | ||
| &self, | ||
| Parameters(params): Parameters<ElicitateParams>, | ||
| ) -> Result<CallToolResult, rmcp::Error> { | ||
| ) -> Result<CallToolResult, rmcp::ErrorData> { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file='crates/elicitate/src/mcp/router.rs'
rg -n -C 2 \
'output_schema|Content::json|CallToolResult::(success|error|structured|structured_error)' \
"$file"
if rg -q 'output_schema' "$file" &&
rg -q 'Content::json' "$file" &&
! rg -q 'CallToolResult::(structured|structured_error)\(|wrapper::Json' "$file"; then
echo "Declared output schema has no structured result path"
exit 1
fiRepository: KooshaPari/phenotype-tooling
Length of output: 1644
🌐 Web query:
docs.rs rmcp 1.4 CallToolResult success error structured structured_error
💡 Result:
In the rmcp 1.4 crate, CallToolResult is the structure used to represent the result of a tool call [1][2]. It provides specialized constructor methods to handle success and error scenarios, including those involving structured data [1][2]. The constructor methods are as follows: success(content: Vec) -> Self Creates a successful tool result with standard unstructured content [1][2]. error(content: Vec) -> Self Creates a tool-level error result. This is used when the tool executes successfully but fails (e.g., no results found or an external API error), allowing the error content to be rendered by the caller's MCP client [1][3][4]. structured(value: Value) -> Self Creates a successful tool result that includes structured content (represented as a serde_json::Value) [1][5][2]. structured_error(value: Value) -> Self Creates an error tool result that includes structured content [1][5][2]. This is typically used when a tool defines an output schema, requiring the error response to also provide structured data that conforms to that schema [6]. In the rmcp SDK, there is a clear distinction between these tool-level errors (returned as Ok(CallToolResult::error(...))) and protocol errors (returned as Err(McpError)), the latter of which are reserved for infrastructure or routing failures that clients handle opaquely [7][3][4]. The structured methods were introduced to support tools that utilize an output_schema, ensuring that both successful outputs and errors can provide typed, machine-readable data alongside human-readable content [6].
Citations:
- 1: https://docs.rs/rmcp/latest/rmcp/model/struct.CallToolResult.html
- 2: https://docs.rs/brk_rmcp/latest/brk_rmcp/model/struct.CallToolResult.html
- 3: https://crates.io/crates/rmcp
- 4: https://github.com/modelcontextprotocol/rust-sdk
- 5: https://github.com/modelcontextprotocol/rust-sdk/blob/e9029ccc/crates/rmcp/tests/test_structured_output.rs
- 6: Add support for
Tool.outputSchemaandCallToolResult.structuredContentmodelcontextprotocol/rust-sdk#316 - 7: docs(server): document Err vs Ok(CallToolResult::error) visibility contract on ServerHandler::call_tool modelcontextprotocol/rust-sdk#854
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for file in crates/cargo.toml Cargo.toml; do
if [ -f "$file" ]; then
rg -n 'rmcp|modelcontextprotocol' "$file" -C 2 || true
fi
done
rg -n 'struct CallToolResult|impl CallToolResult|fn success|fn error|fn structured|fn structured_error|struct Content|struct ContentBlock' crates -S --glob '*.rs' | head -80Repository: KooshaPari/phenotype-tooling
Length of output: 528
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Cargo files containing rmcp:"
fd -a 'Cargo.toml$' . | while read -r f; do
if rg -q 'rmcp' "$f"; then
echo "== $f =="
rg -n 'rmcp' "$f" -C 2
fi
done
echo
echo "router relevant section:"
sed -n '80,145p' crates/elicitate/src/mcp/router.rs
echo
echo "search output schema declarations:"
rg -n 'output_schema|tool\(' crates -g '*.rs' | head -80Repository: KooshaPari/phenotype-tooling
Length of output: 3421
Return structured content for the declared output schema.
elicit declares output_schema = ...ElicitResponse, but it returns Content via CallToolResult::success / CallToolResult::error, which produce unstructured tool results. Use CallToolResult::structured(...) for successful responses and CallToolResult::structured_error(...) for typed tool-level failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/mcp/router.rs` around lines 94 - 101, The elicit method
declares ElicitResponse as its output schema but currently returns unstructured
CallToolResult values. Update its success and failure paths to use
CallToolResult::structured(...) and CallToolResult::structured_error(...) with
typed ElicitResponse-compatible payloads, preserving the existing response
behavior and error semantics.
| fn get_info(&self) -> ServerInfo { | ||
| ServerInfo { | ||
| protocol_version: ProtocolVersion::default(), | ||
| capabilities: ServerCapabilities::default(), | ||
| server_info: Implementation { | ||
| name: "elicitate".to_string(), | ||
| version: env!("CARGO_PKG_VERSION").to_string(), | ||
| }, | ||
| instructions: Some( | ||
| "elicitate_mcp renders a native OS popup and blocks until the human responds. \ | ||
| Use it whenever you need a single, structured decision from the operator: \ | ||
| confirmations, secrets, multi-choice selection, disambiguation. Returns typed \ | ||
| JSON: {status: answered|cancelled|timed_out|failed, value?, notes?}." | ||
| .to_string(), | ||
| ), | ||
| } | ||
| ServerInfo::new(ServerCapabilities::default()) | ||
| .with_server_info(Implementation::new( | ||
| "elicitate", | ||
| env!("CARGO_PKG_VERSION"), | ||
| )) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file='crates/elicitate/src/mcp/router.rs'
rg -n -C 3 \
'get_info|ServerCapabilities|enable_tools|tool_handler' \
"$file"
if rg -q 'ServerCapabilities::default\(\)' "$file" &&
! rg -q 'enable_tools' "$file"; then
echo "MCP server does not explicitly advertise the tools capability"
exit 1
fiRepository: KooshaPari/phenotype-tooling
Length of output: 950
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Relevant router.rs section:"
sed -n '1,220p' crates/elicitate/src/mcp/router.rs
echo
echo "Search for ServerCapabilities builder / enable_tools in the whole repository:"
rg -n 'ServerCapabilities::builder|enable_tools|tools\[' crates Cargo.toml crates/*/Cargo.toml || true
echo
echo "Workspace dependency constraints for rmcp:"
rg -n 'rmcp' Cargo.toml crates packages.toml Cargo.lock crates elicitate -g 'Cargo.toml' -g 'Cargo.lock' || trueRepository: KooshaPari/phenotype-tooling
Length of output: 7705
🌐 Web query:
docs.rs rmcp 1.4 ServerCapabilities default builder enable_tools tools optional
💡 Result:
In rmcp (Rust), ServerCapabilities::builder is used to populate the server’s advertised MCP capabilities, including “tools”. The builder exposes an enable_tools method and a separate enable_tool_list_changed method (which only affects the tools capability). 1) Enable “tools” capability Use: - ServerCapabilities::builder.enable_tools....build This sets ServerCapabilities.tools to Some(ToolsCapability {... }) [1][2]. 2) Enable “tools list changed” sub-capability If you also want to advertise that your server’s tool list can change, chain: -.enable_tool_list_changed The implementation sets ToolsCapability.list_changed = Some(true) when tools are enabled [3]. 3) Where this shows up in the ServerCapabilities struct ServerCapabilities has a tools: Option field, alongside other optional capability sections like prompts/resources/logging/etc. [1]. Example (from docs.rs) let cap = ServerCapabilities::builder.enable_logging.enable_experimental.enable_prompts.enable_resources.enable_tools.enable_tool_list_changed.build; [1] Cargo feature note (for availability in the crate) The builder method documentation notes that enable_tool_list_changed is only available when the crate features needed for server/tooling are enabled (“crate features macros or server only.”) [2].
Citations:
- 1: https://docs.rs/rmcp/latest/rmcp/model/struct.ServerCapabilities.html
- 2: https://docs.rs/rmcp/latest/rmcp/model/struct.ServerCapabilitiesBuilder.html
- 3: https://github.com/modelcontextprotocol/rust-sdk/blob/e9029ccc/crates/rmcp/src/model/capabilities.rs
Advertise the tools capability during initialization.
ServerCapabilities::default() does not set ServerCapabilities::tools, so MCP clients will not treat this server as offering tools. Build capabilities with ServerCapabilities::builder().enable_tools().build() in ServerHandler::get_info().
Proposed fix
- ServerInfo::new(ServerCapabilities::default())
+ ServerInfo::new(
+ ServerCapabilities::builder()
+ .enable_tools()
+ .build(),
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn get_info(&self) -> ServerInfo { | |
| ServerInfo { | |
| protocol_version: ProtocolVersion::default(), | |
| capabilities: ServerCapabilities::default(), | |
| server_info: Implementation { | |
| name: "elicitate".to_string(), | |
| version: env!("CARGO_PKG_VERSION").to_string(), | |
| }, | |
| instructions: Some( | |
| "elicitate_mcp renders a native OS popup and blocks until the human responds. \ | |
| Use it whenever you need a single, structured decision from the operator: \ | |
| confirmations, secrets, multi-choice selection, disambiguation. Returns typed \ | |
| JSON: {status: answered|cancelled|timed_out|failed, value?, notes?}." | |
| .to_string(), | |
| ), | |
| } | |
| ServerInfo::new(ServerCapabilities::default()) | |
| .with_server_info(Implementation::new( | |
| "elicitate", | |
| env!("CARGO_PKG_VERSION"), | |
| )) | |
| fn get_info(&self) -> ServerInfo { | |
| ServerInfo::new( | |
| ServerCapabilities::builder() | |
| .enable_tools() | |
| .build(), | |
| ) | |
| .with_server_info(Implementation::new( | |
| "elicitate", | |
| env!("CARGO_PKG_VERSION"), | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/mcp/router.rs` around lines 147 - 152, Update
ServerHandler::get_info to construct capabilities with
ServerCapabilities::builder().enable_tools().build() instead of
ServerCapabilities::default(), while preserving the existing server identity
metadata.
|
|
||
| #[test] | ||
| fn params_roundtrip_via_spec() { | ||
| let p = ElicitateParams { | ||
| title: "Ship?".into(), | ||
| question: "Should we ship v1?".into(), | ||
| field: crate::spec::FieldSpec::Boolean { | ||
| label: "Ship?".into(), | ||
| default: Some(true), | ||
| }, | ||
| notes: None, | ||
| buttons: None, | ||
| urgency: crate::spec::Urgency::Warning, | ||
| timeout_secs: 120, | ||
| request_id: Some("test-1".into()), | ||
| }; | ||
| let s: PromptSpec = p.into(); | ||
| assert_eq!(s.title, "Ship?"); | ||
| assert_eq!(s.request_id.as_deref(), Some("test-1")); | ||
| assert_eq!(s.timeout_secs, 120); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert every populated field in the conversion test.
The fixture sets question, field, and urgency, but the test asserts only title, request_id, and timeout_secs. A regression that drops another mapped value will pass.
Add assertions for all populated fields. Rename the test if it remains a one-way conversion.
Proposed test additions
assert_eq!(s.title, "Ship?");
+ assert_eq!(s.question, "Should we ship v1?");
+ assert_eq!(s.urgency, crate::spec::Urgency::Warning);
+ assert!(matches!(
+ s.field,
+ crate::spec::FieldSpec::Boolean {
+ label,
+ default: Some(true),
+ } if label == "Ship?"
+ ));
assert_eq!(s.request_id.as_deref(), Some("test-1"));
assert_eq!(s.timeout_secs, 120);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn params_roundtrip_via_spec() { | |
| let p = ElicitateParams { | |
| title: "Ship?".into(), | |
| question: "Should we ship v1?".into(), | |
| field: crate::spec::FieldSpec::Boolean { | |
| label: "Ship?".into(), | |
| default: Some(true), | |
| }, | |
| notes: None, | |
| buttons: None, | |
| urgency: crate::spec::Urgency::Warning, | |
| timeout_secs: 120, | |
| request_id: Some("test-1".into()), | |
| }; | |
| let s: PromptSpec = p.into(); | |
| assert_eq!(s.title, "Ship?"); | |
| assert_eq!(s.request_id.as_deref(), Some("test-1")); | |
| assert_eq!(s.timeout_secs, 120); | |
| } | |
| #[test] | |
| fn params_roundtrip_via_spec() { | |
| let p = ElicitateParams { | |
| title: "Ship?".into(), | |
| question: "Should we ship v1?".into(), | |
| field: crate::spec::FieldSpec::Boolean { | |
| label: "Ship?".into(), | |
| default: Some(true), | |
| }, | |
| notes: None, | |
| buttons: None, | |
| urgency: crate::spec::Urgency::Warning, | |
| timeout_secs: 120, | |
| request_id: Some("test-1".into()), | |
| }; | |
| let s: PromptSpec = p.into(); | |
| assert_eq!(s.title, "Ship?"); | |
| assert_eq!(s.question, "Should we ship v1?"); | |
| assert_eq!(s.urgency, crate::spec::Urgency::Warning); | |
| assert!(matches!( | |
| s.field, | |
| crate::spec::FieldSpec::Boolean { | |
| label, | |
| default: Some(true), | |
| } if label == "Ship?" | |
| )); | |
| assert_eq!(s.request_id.as_deref(), Some("test-1")); | |
| assert_eq!(s.timeout_secs, 120); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/elicitate/src/mcp/router.rs` around lines 178 - 198, The
params_roundtrip_via_spec test only verifies some converted fields. Add
assertions covering question, field (including its Boolean label and default),
and urgency, while retaining the existing checks; rename the test to reflect
one-way conversion if it does not actually round-trip back to ElicitateParams.
|
Closing due to merge conflicts. Cannot auto-rebase. |
User description
Ports the dep-light v0.18.0 + v0.19.0 work from the wip/2026-07-22-phenotype-tooling-absorbed-go-mod branch (closed as superseded in #267) against current main.
Main has had rmcp 0.2 -> 1.4, dirs 5 -> 6, schemars 0.8 -> 1.2, thiserror 1.0 -> 2.0 since the wip branch was created. The rmcp-coupled MCP router changes from v0.13.0-v0.17.0 are NOT included here — they require a separate rmcp 1.4 API rewrite.
This PR is scoped to the dep-light additions:
Tests: 120/120 green. Build clean with --no-default-features. v0.2.0 -> v0.3.0.
CodeAnt-AI Description
Add per-namespace daemons and namespace management to Elicitate
What Changed
namespace list,namespace show, andnamespace cleancommands to inspect daemon status, inbox activity, and remove old terminal entriesImpact
✅ Isolated inboxes for named projects and teams✅ Reliable per-namespace daemon startup✅ Easier namespace monitoring and cleanup💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.