feat: improve recursive environment glob diagnostics (Fixes #427)

[Karthik Nadig (karthiknadig)]

Summary

• warn immediately when environmentDirectories contains recursive ** path segments
• report exact pattern names and elapsed time for slow configure glob expansions
• recommend non-recursive container-directory patterns in the JSON-RPC documentation
• support recursive globstar detection through brace expansion without flagging foo**bar
• add direct coverage for classification and directory-only expansion

Validation

• cargo test -p pet-fs
• cargo test -p pet configure
• .\scripts\rust-precommit.ps1

Fixes #427

[github-actions]

Performance Report (Linux) ➖

Metric	PR (P50)	PR (P95)	Baseline (P50)	Delta	Change
Server Startup	1ms	1ms	1ms	0ms	0%
Full Refresh	55ms	499ms	55ms	0ms	0%

Results based on 10 iterations. P50 = median, P95 = 95th percentile.

---

Legend

• 🚀 Significant speedup (>100ms faster)
• ✅ Faster than baseline
• ➖ No significant change
• 🔺 Slower than baseline (>100ms)
• ⚠️ Significant slowdown (>500ms)

[github-actions]

Test Coverage Report (Linux)

Metric	Value
Current Coverage	80.7%
Base Branch Coverage	80.6%
Delta	.1% ✅

---

Coverage increased! Great work!

[github-actions]

Performance Report (macOS)

Metric	PR (P50)	PR (P95)	Baseline (P50)	Delta
Server Startup	98ms	648ms	69ms	29ms
Full Refresh	144ms	30209ms	118ms	26ms

Results based on 10 iterations. P50 = median, P95 = 95th percentile.

---

Legend

• 🚀 Significant speedup (>100ms faster)
• ✅ Faster than baseline
• ➖ No significant change
• 🔺 Slower than baseline (>100ms)
• ⚠️ Significant slowdown (>500ms)

[github-actions]

Test Coverage Report (Windows)

Metric	Value
Current Coverage	77.6%
Base Branch Coverage	77.52%
Delta	0.08% ✅

---

Coverage increased! Great work!

[github-actions]

Performance Report (Windows) ✅

Metric	PR (P50)	PR (P95)	Baseline (P50)	Delta	Change
Server Startup	9ms	12ms	9ms	0ms	0%
Full Refresh	136ms	1401ms	164ms	-28ms	-17.1%

Results based on 10 iterations. P50 = median, P95 = 95th percentile.

---

Legend

• 🚀 Significant speedup (>100ms faster)
• ✅ Faster than baseline
• ➖ No significant change
• 🔺 Slower than baseline (>100ms)
• ⚠️ Significant slowdown (>500ms)

[Copilot]

Pull request overview

This PR improves configure-time diagnostics around glob expansion for workspaceDirectories / environmentDirectories, aiming to make risky recursive patterns (**) and slow glob expansions easier to identify before the client times out.

Changes:

• Adds per-pattern timing and slow-expansion warnings during configure glob expansion.
• Warns when environmentDirectories contains recursive glob patterns and updates JSON-RPC docs to recommend bounded patterns.
• Adds initial unit coverage for recursive glob pattern detection.

Show a summary per file

File	Description
docs/JSONRPC.md	Documents bounded environmentDirectories patterns and discourages recursive ** patterns due to timeout risk.
crates/pet/src/jsonrpc.rs	Adds per-pattern glob expansion timing/warnings and early recursive pattern warnings during configure.
crates/pet-fs/src/glob.rs	Introduces is_recursive_glob_pattern helper and adds basic unit tests for it.

Review details

Suppressed comments (1)

crates/pet-fs/src/glob.rs:215

• The new tests for is_recursive_glob_pattern don't cover the false-positive case where "**" appears within a single segment (e.g. "foo**bar"). Adding a negative assertion will lock in the intended semantics (recursive only when "**" is its own path segment).

        assert!(is_recursive_glob_pattern("**/.venv"));
        assert!(is_recursive_glob_pattern("/home/user/**/venv"));
        assert!(!is_recursive_glob_pattern(".venv"));
        assert!(!is_recursive_glob_pattern("*/.venv"));
    }

• Files reviewed: 3/3 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (4)

crates/pet-fs/src/glob.rs:25

• is_recursive_glob_pattern() currently returns true for any string containing "", but the doc comment says it should detect globstar recursion across path components. This can produce false positives (e.g., "foobar") and cause misleading warnings.

/// Returns true when a glob can traverse an unbounded number of path components.
pub fn is_recursive_glob_pattern(path: &str) -> bool {
    path.contains("**")
}

crates/pet-fs/src/glob.rs:214

• The new test_is_recursive_glob_pattern doesn’t cover the common false-positive case where "**" appears within a single path component (which should not be treated as recursive globstar traversal). Adding a regression assertion here will protect the intended semantics of is_recursive_glob_pattern().

    fn test_is_recursive_glob_pattern() {
        assert!(is_recursive_glob_pattern("**/.venv"));
        assert!(is_recursive_glob_pattern("/home/user/**/venv"));
        assert!(!is_recursive_glob_pattern(".venv"));
        assert!(!is_recursive_glob_pattern("*/.venv"));

crates/pet/src/jsonrpc.rs:607

• The warning message recommends '.venv' / '*/.venv' as bounded environmentDirectories patterns, but environment_directories is treated as a set of directories that contain multiple environments (PET enumerates their children). Suggest updating the guidance in this warning to avoid steering users toward patterns that would typically point at a single environment directory.

        warn!(
            "Recursive environmentDirectories pattern '{}' can make configure slow; prefer bounded patterns such as '.venv' or '*/.venv'",
            pattern.display()
        );

docs/JSONRPC.md:88

• environmentDirectories is described as a list of (generally global) directories that contain environments; examples like ".venv" and "*/.venv" are typically workspace-relative environment folders and don’t match how PET consumes environment_directories (it enumerates child directories under each entry). Consider updating the examples/warning to reflect directories that contain environments, and format the recursive example as inline code for readability.

   * Bounded glob patterns are supported (e.g., ".venv", "*/.venv", "/home/user/envs/*").
   * Avoid recursive workspace-wide patterns such as "**/.venv": they can traverse large directory trees,
   * delay configure responses, and trigger client timeouts.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

docs/JSONRPC.md:88

• This new guidance warns against recursive patterns, but the preceding workspaceDirectories docs still give a recursive example ("**/.venv") and contain a typo ("enviornents"). That’s inconsistent and may encourage exactly the pattern we’re discouraging.

   * Bounded glob patterns are supported (e.g., ".venv", "*/.venv", "/home/user/envs/*").
   * Avoid recursive workspace-wide patterns such as "**/.venv": they can traverse large directory trees,
   * delay configure responses, and trigger client timeouts.

• Files reviewed: 3/3 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (2)

docs/JSONRPC.md:88

• “Bounded patterns” is ambiguous without a definition, and PET will still expand recursive ** patterns (this PR adds warnings for them). Consider rephrasing to “Glob patterns are supported” while keeping the explicit guidance to avoid recursive ** patterns for performance.

   * Values identify directories that contain environments. Bounded patterns are supported (e.g., `/home/user/envs`, `/home/user/*/envs`).
   * Avoid recursive patterns such as `/home/user/**/envs`: they can traverse large directory trees,
   * delay configure responses, and trigger client timeouts.

docs/JSONRPC.md:77

• The term “bounded glob patterns” isn’t defined here, which makes it unclear what patterns are considered acceptable. Since PET supports ** but this is discouraged for performance reasons, it would be clearer to say “Glob patterns are supported” and explicitly call out avoiding recursive ** patterns.

This issue also appears on line 86 of the same file.

   * Bounded glob patterns are supported (e.g., `/home/user/projects/*`).

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (3)

crates/pet-fs/src/glob.rs:25

• is_recursive_glob_pattern only detects ** when it is the entire path segment. Since PET glob expansion supports brace groups (e.g. {bin,Scripts}), a recursive option inside a brace group like "{foo,**}/.venv" would currently be missed, even though it can still traverse unbounded components.

/// Returns true when a glob can traverse an unbounded number of path components.
pub fn is_recursive_glob_pattern(path: &str) -> bool {
    path.split(['/', '\\']).any(|segment| segment == "**")
}

crates/pet-fs/src/glob.rs:214

• Consider adding a test case for recursive ** within a brace-expanded segment (e.g. {foo,**}/.venv) to cover the intended behavior of is_recursive_glob_pattern when brace expansion is used elsewhere in the glob implementation.

    fn test_is_recursive_glob_pattern() {
        assert!(is_recursive_glob_pattern("**/.venv"));
        assert!(is_recursive_glob_pattern("/home/user/**/venv"));
        assert!(!is_recursive_glob_pattern(".venv"));

docs/JSONRPC.md:88

• The PR description says the JSON-RPC docs should recommend bounded patterns like .venv and */.venv, but the updated environmentDirectories docs only mention /home/user/.../envs examples. Either update the docs to include the .venv/*/.venv recommendations (and an example of a recursive pattern like **/.venv), or adjust the PR description to match the documentation change.

   * Values identify directories that contain environments. Glob patterns are supported (e.g., `/home/user/envs`, `/home/user/*/envs`).
   * Avoid recursive patterns such as `/home/user/**/envs`: they can traverse large directory trees,
   * delay configure responses, and trigger client timeouts.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (1)

crates/pet/src/jsonrpc.rs:607

• The warning message includes POSIX-specific example paths (/home/user/...), which can be confusing on Windows where this diagnostic is also emitted. Consider using a platform-neutral placeholder (e.g. <root>/envs) so the guidance is clear everywhere.

        warn!(
            "Recursive environmentDirectories pattern '{}' can make configure slow; prefer bounded container-directory patterns such as '/home/user/envs' or '/home/user/*/envs'",
            pattern.display()
        );

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (2)

docs/JSONRPC.md:88

• The examples for environmentDirectories (e.g., envs, */envs) look like relative paths, but the rest of the docs/sample usage describe these as fully-qualified paths. Relative patterns are resolved against PET's current working directory, which is client-dependent and can make this guidance misleading. Consider switching the examples to absolute/fully-qualified patterns (or explicitly stating how relative paths are resolved).

   * Values identify directories that contain environments. Glob patterns are supported (e.g., `envs`, `*/envs`).
   * Avoid recursive patterns such as `**/envs`: they can traverse large directory trees,
   * delay configure responses, and trigger client timeouts.

crates/pet/src/jsonrpc.rs:607

• The warning message recommends patterns like envs / */envs, which read as relative paths. Since environmentDirectories is generally passed as fully-qualified paths (and relative patterns depend on the server CWD), consider making the suggestion path-anchored to avoid confusion.

    for pattern in recursive_environment_patterns(patterns) {
        warn!(
            "Recursive environmentDirectories pattern '{}' can make configure slow; prefer non-recursive container-directory patterns such as 'envs' or '*/envs'",
            pattern.display()

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite