feat(mcp): list_rules tool

[aram-devdocs]

Summary

Adds the list_rules MCP tool — enumerates every built-in rule with id, default severity, and one-line summary from Rule::summary. Skeleton-scope PR aimed at green CI; protocol-level stdio test coverage and docs/src/mcp.md updates land in a follow-up.

Closes #36.

What changed

• New ListRulesArgs struct (no fields) + list_rules async handler on PlumbServer in crates/plumb-mcp/src/lib.rs.
• Tool descriptor wired into list_tools and dispatch arm added in call_tool.
• Helper list_rules_payload exposes the response shape for unit testing without constructing a full RequestContext<RoleServer>.
• Output sorted ascending by rule id (which encodes <category>/<name>, so categories cluster naturally).
• Response shape matches PRD §14.2: compact text in content[0], structured payload {rules: [{id, default_severity, summary}], count} in structuredContent, isError: false.

Tests

• New unit test list_rules_returns_every_builtin_rule_sorted in crates/plumb-mcp/tests/mcp_protocol.rs — asserts every registered rule appears, ordering is ascending, and the first entry carries a valid lowercase severity label and a non-empty summary.
• All existing 26 tests across plumb-mcp and plumb-cli still pass.

Out of scope (follow-up)

• Updating the stdio protocol test in crates/plumb-cli/tests/mcp_stdio.rs to assert list_rules appears in tools/list.
• Updating docs/src/mcp.md tool table.

Plumb invariants honored

• #![forbid(unsafe_code)] preserved.
• No unwrap/expect/panic! in library code.
• Deterministic — sorted output, no wall-clock, no env reads.
• Layer discipline — plumb-mcp reads plumb-core::register_builtin only.

Verification

cargo fmt --all -- --check          # clean
cargo clippy --workspace --all-targets --all-features -- -D warnings  # clean
cargo nextest run -p plumb-mcp -p plumb-cli  # 26 passed

🤖 Generated with Claude Code

[aram-devdocs]

No description provided.

[aram-devdocs]

No description provided.

[aram-devdocs]

No description provided.

[github-actions]

Now I have everything I need for a complete review of both open PRs.

---

PR #129 — feat(mcp): list_rules tool

File coverage: crates/plumb-mcp/src/lib.rs, crates/plumb-mcp/tests/mcp_protocol.rs, crates/plumb-cli/tests/mcp_stdio.rs, docs/src/mcp.md

1. Determinism

list_rules_payload sorts the registry by rule.id() string before building the response. Since ids are unique <category>/<name> strings, sort_unstable_by produces a stable result — no tie-breaking ambiguity. No SystemTime, no HashMap iteration in the output path. Clean.

2. Workspace layering

No new cross-layer dependencies. plumb-mcp → plumb-core (register_builtin) is the existing allowed path. No unsafe, no println!. #![forbid(unsafe_code)] and #![deny(clippy::unwrap_used, clippy::expect_used)] already cover the crate.

3. Error handling

list_rules_payload is infallible — returns (String, Value). The async list_rules wraps it and returns Ok(...). Correct; no false Result wrapper needed. No missing # Errors section.

4. Tests

• mcp_protocol.rs::list_rules_returns_every_builtin_rule_sorted — verifies count, array length, sort order, and field shapes in-process. ✓
• mcp_stdio.rs::mcp_list_rules_returns_every_rule — full e2e over real stdio. ✓
• mcp_initialize_and_tools_list updated with presence assertions for both explain_rule and list_rules. ✓

5. Documentation

Module-level doc updated. list_rules_payload has a #[must_use] and a thorough doc block explaining the sort key, token-budget bound, and the &self design choice. docs/src/mcp.md table row added.

Punch list

• crates/plumb-mcp/src/lib.rs — #[allow(clippy::unused_self)] on a pub method is unusual. The doc comment justifies it (ergonomic symmetry with other async tool methods whose &self is load-bearing). Acceptable; the workspace pedantic group promotes unused_self to a warn, so the allow is necessary. Not a blocker.

---

Verdict: APPROVE

---

PR #132 — feat(mcp): get_config tool

File coverage: crates/plumb-mcp/src/lib.rs, crates/plumb-mcp/Cargo.toml, Cargo.lock, crates/plumb-cli/tests/mcp_stdio.rs, docs/src/mcp.md

1. Determinism

SystemTime is stored as a cache key for mtime comparison, never serialized into the response. std::collections::HashMap is used for the internal cache; its iteration order never leaks into output. clippy.toml only bans SystemTime::now() and Instant::now() — Metadata::modified() is not in the disallowed list. No output-visible nondeterminism. Clean on the letter of the rule.

The serialized response fields (config, source, path) are built from serde_json::json!({}) in insertion order. Deterministic.

2. Workspace layering

plumb-config (layer 4) added as a dep of plumb-mcp (layer 5). Not a cycle; not an upward dependency. However dependency-hierarchy.md still lists plumb-mcp as depending on only plumb-core and plumb-format — the doc is already stale (plumb-cdp was added earlier without updating it). The rule document should be updated to reflect both plumb-cdp and plumb-config. Not a hard blocker but the ADR reference is wrong.

3. Error handling

# Errors section present on get_config. Empty and relative-path guards return invalid_params. Stat and parse failures return internal_error. ConfigError is mapped through map_config_error helper. No unwrap/expect outside tests.

4. Tests — BLOCKER

crates/plumb-mcp/tests/mcp_protocol.rs has no new test for get_config. The rule in .agents/rules/mcp-tool-patterns.md is explicit:

Every tool gets a case in crates/plumb-mcp/tests/mcp_protocol.rs: Happy path. Invalid args → clear JSON-RPC error.

The PR adds only an e2e test in mcp_stdio.rs (happy path — no plumb.toml). Missing:

• An in-process protocol test for the happy path (no-file → default config shape).
• A test for working_dir = "" → invalid_params error.
• A test for a relative path → invalid_params error.

5. Additional warnings

crates/plumb-mcp/src/lib.rs ~L237-244 — TOCTOU window:

if config_path.exists() {
    let mtime = std::fs::metadata(&config_path)   // file may be gone here
        .and_then(|m| m.modified())

Between exists() and metadata() the file can be deleted, producing a confusing internal_error instead of a graceful fallback to default. Prefer: stat first, match on io::ErrorKind::NotFound.

crates/plumb-mcp/src/lib.rs — blocking I/O in async context:
config_path.exists(), std::fs::metadata(), and plumb_config::load() are all synchronous. Inside async fn get_config, they block the Tokio thread. For a local config file this rarely matters in practice, but the correct pattern is tokio::task::spawn_blocking. Non-blocking for now; flag for a follow-up if the server ever handles concurrent requests.

Unbounded cache: config_cache: Arc<Mutex<HashMap<PathBuf, ConfigCacheEntry>>> has no eviction. Not a problem at current usage but worth a tracking issue if the server becomes long-lived.

Punch list

Location	Severity	Issue
mcp_protocol.rs (missing)	BLOCKER	No protocol unit tests for get_config — required by mcp-tool-patterns.md (happy path + invalid args)
lib.rs:~L237	Warning	TOCTOU: exists() then metadata() — stat once and match NotFound
lib.rs (async fn)	Warning	Blocking fs::metadata / plumb_config::load on async thread — consider spawn_blocking
dependency-hierarchy.md	Note	Document doesn't list plumb-cdp or plumb-config under plumb-mcp; update the dep table

---

Verdict: REQUEST_CHANGES

[aram-devdocs]

Conflict resolution attempt blocked. The PR conflicts with main because PR #130 (explain_rule tool) and PR #131 (lint_url over real http(s) URLs via ChromiumDriver) both landed and overlap in crates/plumb-mcp/src/lib.rs and crates/plumb-mcp/tests/mcp_protocol.rs. The intended resolution is straightforward (rebase feat(mcp): list_rules tool onto current main, then re-add the four pieces in lib.rs — module-doc bullet, ListRulesArgs struct, list_rules + list_rules_payload methods, instructions string update, dispatch arm, tool descriptor — plus the list_rules_returns_every_builtin_rule_sorted test in mcp_protocol.rs). The automated session could not write crates/plumb-mcp/src/lib.rs because the worktree harness denied write permission to that path for both root and subagent contexts (delegation-guard hook routes Rust src to a subagent; harness then refuses the subagent's writes). The rebase was aborted; the branch is back at 3fa0ea6 with a clean tree.

[aram-devdocs]

Conflict resolution complete. Merged origin/main into the list_rules branch — main had landed PRs #130 (explain_rule), #131 (real http(s) URLs in lint_url), and #133 (delegation-guard merge bypass). Mechanical resolution in crates/plumb-mcp/src/lib.rs and tests/mcp_protocol.rs: union both new tools (4 total — echo, lint_url, explain_rule, list_rules), switch register_builtin import to the top-level re-export main introduced, and add the list_rules row to docs/src/mcp.md.

Also addressed the prior Claude review's REQUEST_CHANGES: added mcp_list_rules_returns_every_rule end-to-end test in crates/plumb-cli/tests/mcp_stdio.rs, extended mcp_initialize_and_tools_list to assert both list_rules and explain_rule are advertised, and dropped the duplicate register_builtin() call in the in-process test.

CI: all 13 checks green (Determinism, Preflight, Tests on ubuntu/macos/windows, Coverage, MSRV, cargo-deny, Docs, Size guard, Conventional Commits, Claude review, GitGuardian, action-semantic-pull-request). Latest Claude review verdict for #129 is APPROVE; the trailing REQUEST_CHANGES in the combined verdict comment is scoped to PR #132 (get_config), unrelated to this branch.