Refactor glob.rs brace validation complexity

[coderabbitai]

Overview

The brace-validation and escape logic in src/manifest/glob.rs has become quite intricate and could benefit from simplification to reduce maintenance overhead.

Suggested Improvements

Consider one of the following approaches:

1. Extract shared helpers: Move shared utilities like CharContext and BraceValidationState into a smaller internal utility module
2. Use dedicated crate: Leverage an existing glob/brace-expansion crate to reduce custom implementation complexity

Context

This issue was identified during review of PR #168 which refactored the manifest module. The current implementation has:

• Complex character processing logic with multiple state transitions
• Intricate brace matching and validation rules
• Platform-specific escape handling

Benefits

• Reduced maintenance burden
• Improved code readability
• Potential bug reduction through battle-tested libraries
• Better separation of concerns

References

• PR: Refactor manifest module into dedicated submodules #168
• Discussion: Refactor manifest module into dedicated submodules #168 (comment)

Requested by: @leynos

[leynos]

@coderabbitai looking at the codebase, is this issue still relevant? If the issue has been fixed in the code, does the documentation reflect this?

If it is still relevant, please provide an assessment of the issue criticality (critical, high, medium or low), based on risk of the issue impacting the user and the anticipated blast radius.

Once a decision has been made, apply the appropriate label.

If the issue is documentation ONLY (i.e., no code changes needed), apply the documentation label in addition to the appropriate criticality label.

Please also provide an impact of the required changes.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Fetch issue #172 details
gh issue view 172 --json title,body,labels,state

echo -e "\n=== Searching for README and documentation files ==="
fd -e md -e txt README

echo -e "\n=== Searching codebase structure ==="
fd -t f | head -20

Length of output: 1864

---

🏁 Script executed:

#!/bin/bash

# Check if src/manifest/glob.rs exists and examine its structure
echo "=== Checking for glob.rs file ==="
fd -p "glob.rs"

echo -e "\n=== Examining glob.rs content ==="
if [ -f "src/manifest/glob.rs" ]; then
    wc -l src/manifest/glob.rs
    echo -e "\n--- Searching for CharContext ---"
    rg -n "CharContext" src/manifest/glob.rs
    echo -e "\n--- Searching for BraceValidationState ---"
    rg -n "BraceValidationState" src/manifest/glob.rs
    echo -e "\n--- Counting struct/enum definitions ---"
    rg -n "^(struct|enum|impl)" src/manifest/glob.rs | head -20
    echo -e "\n--- Checking function count and complexity indicators ---"
    rg -n "^(pub )?fn " src/manifest/glob.rs
else
    echo "File src/manifest/glob.rs not found"
fi

echo -e "\n=== Checking for any glob-related external crates in Cargo.toml ==="
rg -n "glob" Cargo.toml

echo -e "\n=== Checking PR #168 reference in git history ==="
git log --all --oneline --grep="168" | head -10

Length of output: 1818

---

🏁 Script executed:

#!/bin/bash

# Let's examine the actual complexity of the code
echo "=== Examining CharContext and BraceValidationState structures ==="
rg -A 10 "^pub struct CharContext" src/manifest/glob.rs

echo -e "\n=== Examining BraceValidationState structure ==="
rg -A 15 "^pub struct BraceValidationState" src/manifest/glob.rs

echo -e "\n=== Looking at BraceValidator implementation complexity ==="
rg -A 5 "^impl BraceValidator" src/manifest/glob.rs

echo -e "\n=== Checking cyclomatic complexity indicators (nested conditionals, match statements) ==="
rg -n "match|if.*if|while|for" src/manifest/glob.rs | wc -l

echo -e "\n=== Examining documentation about glob patterns ==="
rg -i "glob" README.md

echo -e "\n=== Checking netsuke design docs for glob mentions ==="
rg -i "glob" docs/netsuke-design.md docs/netsuke-cli-design-document.md

echo -e "\n=== Checking for any TODO or FIXME comments in glob.rs ==="
rg -n "TODO|FIXME|XXX|HACK" src/manifest/glob.rs

Length of output: 4122

---

🏁 Script executed:

#!/bin/bash

# Let's look at the actual complexity of key functions
echo "=== Examining the BraceValidator process method ==="
ast-grep --pattern 'impl BraceValidator {
  $$$
  fn process($$$) {
    $$$
  }
  $$$
}'

echo -e "\n=== Looking at validate_brace_matching function ==="
rg -A 30 "^fn validate_brace_matching" src/manifest/glob.rs

echo -e "\n=== Checking process_escape_sequence function ==="
rg -A 25 "^fn process_escape_sequence" src/manifest/glob.rs

echo -e "\n=== Checking if there's a separate utility module for these helpers ==="
fd -p "src/manifest" -t f

echo -e "\n=== Checking git log for recent refactorings of glob.rs ==="
git log --oneline --all -- src/manifest/glob.rs | head -10

echo -e "\n=== Checking if documentation mentions current brace validation approach ==="
rg -B 2 -A 2 "brace" docs/netsuke-design.md

Length of output: 2362

---

🏁 Script executed:

#!/bin/bash

# Check for any related bug reports or issues about glob validation
echo "=== Searching for other glob-related issues ==="
gh issue list --limit 50 --state all --json number,title,state | jq '.[] | select(.title | test("glob|brace"; "i"))'

echo -e "\n=== Checking recent commits for glob.rs bug fixes ==="
git log --oneline --all --grep="glob\|brace" -- src/manifest/glob.rs | head -15

echo -e "\n=== Checking test coverage for glob.rs ==="
rg -n "mod.*test|#\[test\]|#\[cfg\(test\)\]" src/manifest/glob.rs | head -10

echo -e "\n=== Counting test functions in glob.rs ==="
rg -c "#\[test\]" src/manifest/glob.rs

echo -e "\n=== Checking manifest tests for glob-related tests ==="
rg -n "glob|brace" src/manifest/tests.rs | head -20

Length of output: 1151

---

🏁 Script executed:

#!/bin/bash

# Let's get a complete picture of the complexity
echo "=== Examining the full BraceValidator implementation ==="
rg -A 100 "^impl BraceValidator" src/manifest/glob.rs | head -120

echo -e "\n=== Checking for any reported bugs related to glob brace validation ==="
gh issue list --limit 100 --state all --search "glob validation bug error brace" --json number,title,state,body

echo -e "\n=== Checking for issue #171 which might overlap ==="
gh issue view 171 --json title,body,state

echo -e "\n=== Applying labels to issue #172 ==="
gh issue edit 172 --add-label "medium"

Length of output: 6080

---

@leynos, I've completed my assessment of issue #172. Here are my findings:

Relevance Status: STILL RELEVANT ✓

The complex brace validation logic described in the issue remains in the codebase:

• src/manifest/glob.rs is 525 lines long
• CharContext and BraceValidationState structures are still present
• The BraceValidator implementation contains intricate state management with multiple transitions
• Approximately 47 conditional/control flow statements indicating significant cyclomatic complexity
• Only 3 test functions covering this complex code

The code has not been simplified since PR #168. Recent commits only added hardening and fixed a Windows-specific issue.

Criticality: MEDIUM 🟡

Risk Assessment:

• User Impact Risk: LOW - The code is functioning correctly with no reported bugs related to brace validation
• Blast Radius: MEDIUM - This is core glob functionality used throughout the manifest system; changes could affect pattern matching across the entire codebase
• Maintenance Burden: MEDIUM-HIGH - Complex state management, platform-specific logic, and limited test coverage make future modifications risky

Rationale: This is technical debt that increases maintenance costs and risk, but it's not currently causing user-facing issues. Should be addressed to reduce future risk, but not urgent.

Documentation Status

This is NOT a documentation issue. The documentation correctly describes glob functionality; the issue is purely about code quality and maintainability.

Impact of Required Changes

Option 1 (Extract helpers to utility module):

• Impact: LOW to MEDIUM
• Improves code organisation without fundamentally reducing complexity
• Relatively safe refactor with limited blast radius

Option 2 (Use dedicated crate):

• Impact: MEDIUM to HIGH
• Note: Issue Refactor glob pattern handling to use globwalk crate and remove custom parsing logic #171 proposes a more comprehensive approach using the globwalk crate to replace ~500 lines of custom logic
• This would address Refactor glob.rs brace validation complexity #172 as part of a larger refactor
• Higher initial risk but better long-term maintainability

Recommendation

Consider addressing #172 as part of implementing #171, which takes a more comprehensive approach. The limited test coverage (only 3 tests) should be expanded before undertaking either refactor to ensure behaviour is preserved.

I've applied the medium label to the issue.

[leynos]

The limited test coverage (only 3 tests) should be expanded before undertaking either refactor to ensure behaviour is preserved.

@coderabbitai please create a new GitHub issue for this and mark #171 and #172 as blocked by it

[coderabbitai]

⛔ Blocked by #242

This issue is blocked by expanding test coverage for the glob validation logic before undertaking the refactoring work.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash

# Create the new issue for test coverage expansion
NEW_ISSUE=$(gh issue create \
  --title "Expand test coverage for glob.rs brace validation before refactoring" \
  --body "## Context

The brace validation logic in \`src/manifest/glob.rs\` is complex with significant cyclomatic complexity (~47 conditional/control flow statements), but currently has only 3 test functions covering this code.

## Problem

Before undertaking refactoring work proposed in #171 and #172, we need comprehensive test coverage to:
- Ensure current behaviour is well-documented through tests
- Provide a safety net during refactoring
- Prevent regressions
- Validate edge cases in brace matching, escape sequences, and state transitions

## Requirements

Expand test coverage for:
- \`CharContext\` and \`BraceValidationState\` structures
- \`BraceValidator\` implementation including all state transitions
- Edge cases in brace matching (nested braces, escaped braces, malformed patterns)
- Platform-specific logic (Windows vs Unix path handling)
- \`validate_brace_matching\` function
- \`process_escape_sequence\` function

## Acceptance Criteria

- [ ] Test coverage includes happy paths for all major code paths
- [ ] Edge cases and error conditions are tested
- [ ] Platform-specific behaviour is validated
- [ ] Tests are well-documented explaining what behaviour they validate

## Blocked Issues

This issue blocks:
- #171 - Consider using a dedicated crate for glob functionality
- #172 - Consider simplifying brace validation logic

## References

- Original analysis: https://github.com/leynos/netsuke/issues/172#issuecomment-$(gh api repos/leynos/netsuke/issues/172/comments --jq '.[].id' | tail -1)
- Related file: \`src/manifest/glob.rs\` (525 lines)" \
  --assignee leynos \
  2>&1)

# Extract the issue number from the output
NEW_ISSUE_NUMBER=$(echo "$NEW_ISSUE" | grep -oP 'https://github.com/leynos/netsuke/issues/\K\d+' | head -1)

echo "Created issue #${NEW_ISSUE_NUMBER}"

# Add comments to #171 and #172 indicating they are blocked
if [ -n "$NEW_ISSUE_NUMBER" ]; then
  gh issue comment 171 --body "⛔ **Blocked by #${NEW_ISSUE_NUMBER}**

This issue is blocked by expanding test coverage for the glob validation logic before undertaking the refactoring work."

  gh issue comment 172 --body "⛔ **Blocked by #${NEW_ISSUE_NUMBER}**

This issue is blocked by expanding test coverage for the glob validation logic before undertaking the refactoring work."
  
  echo "Added blocking comments to #171 and #172"
else
  echo "Failed to extract issue number"
  exit 1
fi

Length of output: 197

---

@leynos, I've created issue #242 to track the test coverage expansion work and marked both #171 and #172 as blocked by it.

Issue #242: "Expand test coverage for glob.rs brace validation before refactoring"

• Assigned to you
• Includes detailed requirements for expanding test coverage across all the complex validation logic
• Lists acceptance criteria for comprehensive testing
• References both blocked issues (Refactor glob pattern handling to use globwalk crate and remove custom parsing logic #171 and Refactor glob.rs brace validation complexity #172)

I've also added blocking comments on both #171 and #172 to document the dependency. The test coverage expansion should be completed before undertaking either refactoring approach to ensure behaviour is preserved during the changes.