Chore/improve scaffold rules

[EdwardIrby]

No description provided.

[claude]

Code Review: Chore/improve scaffold-rules

This PR successfully simplifies the scaffold-rules implementation by removing the multi-agent abstraction layer and consolidating to a unified .plaited/rules/ approach. Score: 8/10

✅ Key Strengths

• Removed 156 lines of agent capability matrix logic
• Unified rules path provides consistency
• Added --list flag for rule discovery
• Marker-based updates enable safe, idempotent config modifications
• Good test coverage for happy path

🔍 Code Quality Issues

1. Unnecessary Optional Chaining (src/scaffold-rules.ts:167)

• Use lines[i].trim() instead of lines[i]?.trim()
• Loop condition guarantees i < lines.length

2. Missing Error Stack Trace (src/scaffold-rules.ts:337-341)

• Add console.error(error) before extracting message for debugging

3. Fragile Description Extraction (src/scaffold-rules.ts:161-175)

• Uses heuristics to find first non-heading paragraph
• Could break if template format changes
• Consider extracting from frontmatter instead

4. Regex Nested Quantifiers (src/scaffold-rules.ts:115-116, 122-123)

• Low risk (trusted input), but consider adding max iteration limit

🎯 Testing Gaps

• No edge case tests (empty files, malformed syntax, long descriptions)
• Marker-based update logic not tested (in skill, not CLI)

📋 Documentation Issues

• No migration guide for existing .claude/rules/ users
• CLAUDE.md:38 still references .claude/rules/ (inconsistent)
• MARKERS export (line 55-58) lacks TSDoc

🚀 Recommendations

Required before merge:

1. Fix optional chaining in extractDescription
2. Add error stack logging
3. Document MARKERS export

Recommended (follow-up):

1. Add edge case tests
2. Add iteration limit to conditional loop
3. Write migration guide

Overall: Approve with minor revisions. Great refactoring that simplifies the codebase!

[claude]

Code Review

This PR successfully simplifies the scaffold-rules implementation by removing the multi-agent abstraction and consolidating to a unified .plaited/rules/ approach. Overall, this is a solid refactoring that improves maintainability.

Strengths

1. Significant code reduction: Removed 979 lines vs 377 added, eliminating complex agent capability matrix logic
2. Unified approach: Single .plaited/rules/ path for all agents provides consistency
3. Marker-based updates: Smart design allowing idempotent config file updates without destroying user content
4. Better discoverability: Added --list flag for rule discovery
5. Good test coverage: Comprehensive happy path testing with 146 new test lines
6. Improved documentation: SKILL.md updated appropriately with clearer workflow

Issues Found

1. Missing Error Stack Trace (src/scaffold-rules.ts:349-352)

When catching errors, log the full error object before extracting the message for better debugging. Add console.error(error) before line 350.

Severity: Medium - Makes debugging template processing errors difficult

2. MARKERS Export Documentation (src/scaffold-rules.ts:63-66)

The MARKERS export is well-documented but consider:

• Is this export needed? Only used internally in CLI
• If it needs to be public, add @public tag to TSDoc
• If only for testing, consider making it internal or test-only

Severity: Low - Documentation completeness

3. Potential Infinite Loop Silent Failure (src/scaffold-rules.ts:115-121)

Good addition of maxIterations counter! However, if max iterations is reached, the function silently continues with potentially unprocessed conditionals. Consider logging a warning after the while loop.

Severity: Low - Edge case handling

4. Non-Null Assertion Could Be More Defensive (src/scaffold-rules.ts:179)

Current code with non-null assertion is correct but could be more defensive with an explicit check and continue statement instead.

Severity: Very Low - Code style preference, current code is correct

Test Coverage Gaps

Missing edge case tests:

• No test for when maxIterations is reached in processConditionals
• No test for description extraction with empty/malformed markdown
• No test for error handling path (line 349-352)
• No test for templates with missing/invalid LINK references

The existing tests cover happy paths well, but edge cases could reveal issues.

Recommendations

Required before merge:

1. Add error stack trace logging (line 349) - Important for debugging
2. Clarify MARKERS export purpose - document or make internal

Nice to have (can be follow-up):

1. Add warning when max iterations reached
2. Add edge case tests for error paths
3. Add note in CLAUDE.md explaining path choices

Overall Assessment

Score: 8.5/10 - Excellent refactoring that significantly improves the codebase. The removal of 156+ lines of complex capability matrix logic is a major win for maintainability. The unified approach and marker-based updates are well-designed.

The issues found are minor and mostly about defensive coding and observability. The core logic is sound and well-tested for happy paths.

Recommendation: Approve with minor revisions - Address the two required items above, then this is ready to merge.

[claude]

Code Review: Chore/improve scaffold-rules

This PR successfully simplifies the scaffold-rules implementation by removing the multi-agent abstraction and consolidating to a unified .plaited/rules/ approach. This is a solid refactoring that significantly improves maintainability.

✅ Strengths

1. Significant simplification: Removed 1,027 lines vs 451 added, eliminating complex agent capability matrix logic
2. Unified approach: Single .plaited/rules/ path provides consistency across all agents
3. Marker-based updates: Smart design using MARKERS const allows idempotent config file updates without destroying user content
4. Better discoverability: Added --list flag for rule discovery
5. Good test coverage: Comprehensive happy path testing with strong assertions
6. Improved documentation: SKILL.md and CLAUDE.md updated with clearer workflow

🔍 Issues Found

1. Missing Error Stack Trace (src/scaffold-rules.ts:349-352)

Severity: Medium

When catching errors, the full error stack is lost. This makes debugging template processing errors difficult.

} catch (error) {
  const message = error instanceof Error ? error.message : String(error)
  console.error(`Error processing template ${file}: ${message}`)
  process.exit(1)
}

Recommendation: Add console.error(error) before extracting the message:

} catch (error) {
  console.error(`Error processing template ${file}:`, error)
  const message = error instanceof Error ? error.message : String(error)
  console.error(message)
  process.exit(1)
}

2. MARKERS Export Clarity (src/scaffold-rules.ts:63-66)

Severity: Low

The MARKERS constant is exported but only used internally by the CLI. The TSDoc is good, but consider:

• If this export is needed for external use, add @public tag
• If only for testing, consider using @internal tag
• If truly public API, ensure it's documented in package README

Current state is acceptable, but clarification would improve API surface understanding.

3. No Warning for Max Iterations (src/scaffold-rules.ts:115-121)

Severity: Low

Good addition of maxIterations counter to prevent infinite loops! However, if max iterations is reached, the function silently continues with potentially unprocessed conditionals.

Recommendation: Add a warning after the while loop:

while (result !== previousResult && iterations < maxIterations) {
  // ... existing code
}

if (iterations >= maxIterations) {
  console.warn('Warning: Max iterations reached in template processing. Some conditionals may be unprocessed.')
}

4. Regex Nested Quantifiers (src/scaffold-rules.ts:126, 133)

Severity: Very Low

The regex patterns use nested quantifiers which could theoretically cause catastrophic backtracking:

/\{\{#if development-skills\}\}((?:(?!\{\{#if )(?!\{\{\^if )(?!\{\{\/if\}\})[\s\S])*?)\{\{\/if\}\}/g

Assessment: Current code is safe because:

• Input is trusted (bundled templates only, not user content)
• maxIterations prevents infinite loops
• Negative lookaheads prevent runaway matching

No action required, but worth noting for future template syntax additions.

📋 Test Coverage Gaps

Missing edge case tests:

• ❌ No test for when maxIterations is reached in processConditionals
• ❌ No test for error handling path (catch block at line 349)
• ❌ No test for templates with malformed/nested conditionals that might trigger max iterations
• ❌ No test for empty template files

Existing tests are strong for happy paths. Edge cases would make coverage more robust but aren't blockers.

🎯 Architecture & Design

Excellent decisions:

• Removing agent-specific logic (AGENT_CAPABILITIES matrix) - major win for maintainability
• Marker-based updates for CLAUDE.md/AGENTS.md - enables safe, idempotent config modifications
• Simplified conditional logic - only development-skills condition (always true in CLI context)
• Clean separation of concerns: processing → generation → output

File organization is sound:

• Template processing functions are pure and testable
• CLI logic cleanly separated from library code
• Type definitions are clear and well-documented

📝 Documentation

CLAUDE.md updates: ✅ Clear, consistent references to .plaited/rules/
SKILL.md updates: ✅ Good workflow documentation with marker-based update instructions
Code comments: ✅ TSDoc is comprehensive and helpful

Minor note: The rules directory convention table is duplicated in both CLAUDE.md and SKILL.md. This is acceptable for discoverability but creates maintenance burden if conventions change.

🔒 Security Considerations

No security issues found:

• Template processing uses trusted bundled content only
• No user input reaches template processing without validation
• File system access is properly scoped to package directory
• Error messages don't leak sensitive information

🚀 Performance

Excellent performance characteristics:

• Removed 156+ lines of capability matrix evaluation
• Template processing is linear with maxIterations cap
• No unnecessary file I/O
• Regex patterns are safe with trusted input

Overall Assessment

Score: 8.5/10 - Excellent refactoring that significantly improves the codebase.

Recommendation: Approve with minor revisions

Required Before Merge:

1. ✅ Add error stack trace logging (severity: medium) - Improves debuggability

Nice to Have (Can be Follow-up):

1. Clarify MARKERS export purpose via TSDoc tag
2. Add warning when max iterations reached
3. Add edge case tests for error paths

The core logic is sound, well-tested for happy paths, and the architectural simplification is a major win. The issues found are minor and mostly about defensive coding and observability.

[claude]

Code Review: Chore/improve scaffold-rules

This PR successfully simplifies the scaffold-rules implementation by removing the multi-agent abstraction layer and consolidating to a unified .plaited/rules/ approach. Overall, this is an excellent refactoring that significantly improves maintainability.

✅ Strengths

1. Major code simplification: Removed 1,034 lines vs 463 added

   • Eliminated complex agent capability matrix logic (AGENT_CAPABILITIES removed)
   • Removed agent-specific conditionals (has-sandbox, supports-slash-commands, agent:*)
   • Consolidated from 2 targets (claude/agents-md) to 1 unified approach

2. Unified rules directory: .plaited/rules/ provides consistency

   • Both CLAUDE.md and AGENTS.md reference the same rule files
   • Clearer mental model for users
   • Better cross-agent portability

3. Marker-based updates: Smart design using MARKERS const

   • Enables idempotent config file updates
   • Preserves user content outside markers
   • Well-documented with TSDoc explaining usage

4. Improved CLI UX:

   • Added --list flag for rule discovery
   • Better validation: warns about invalid rule names with available options
   • Removed confusing --agent parameter

5. Strong test coverage: 148 new test lines

   • Comprehensive happy path testing
   • Good edge case coverage (empty templates, nested conditionals, unknown rules)
   • Tests verify marker-based sections

6. Better error handling: Changed from string concatenation to proper error logging

   • console.error('Error:' , error) in LSP commands (lsp-analyze.ts:97, lsp-find.ts:72, lsp-hover.ts:78, lsp-references.ts:137, lsp-symbols.ts:64)
   • Full stack traces for debugging

🔍 Issues Found

1. Missing Error Stack Trace (src/scaffold-rules.ts:356-357)

Severity: Medium - Actually FIXED

The PR already improved this! Changed from string concatenation to proper error logging with full stack trace. This is a positive change.

2. Max Iterations Warning Implementation (src/scaffold-rules.ts:140-142)

Severity: Low - Well implemented

Good addition of maxIterations counter and warning! This prevents infinite loops from malformed templates. The warning is clear and actionable. This is a positive addition.

3. Regex Pattern Complexity (src/scaffold-rules.ts:128, 135)

Severity: Very Low - Safe as-is

The nested quantifier patterns are safe because:

• Input is trusted (bundled templates only, not user content)
• maxIterations prevents runaway execution
• Negative lookaheads prevent excessive backtracking

No action needed.

📋 Architecture Assessment

Excellent architectural decisions:

1. Removed abstraction layers that did not pay for themselves:

   • Agent capability matrix (AGENT_CAPABILITIES) - 156 lines removed
   • Per-agent conditional logic - simplified to single case
   • Multiple output formats - unified to one

2. Marker-based updates - Clever solution for config file management:

   • Allows programmatic updates without destroying user content
   • Clear boundaries with HTML comments
   • Idempotent by design

3. Template processing simplification:

   • Removed evaluateCondition complexity - always true for development-skills
   • Removed generateCrossReference - path references are uniform
   • Cleaner separation of concerns

📝 Documentation Quality

CLAUDE.md updates: ✅ Excellent

• Updated all references from .claude/rules/ to .plaited/rules/
• Added rules directory convention table explaining the system
• Removed outdated slash command references
• Clear explanation of how .plaited/rules/ relates to agent-specific overrides

SKILL.md updates: ✅ Clear workflow documentation

• Simplified from multi-page command documentation to focused skill guide
• Marker-based update instructions are clear
• Good CLI options reference

Code documentation: ✅ Comprehensive TSDoc

• MARKERS export well-documented with usage instructions
• Template processing functions have clear remarks

🧪 Test Coverage Assessment

Strong happy path coverage:

• ✅ Template processing with conditionals
• ✅ Variable substitution
• ✅ Description extraction
• ✅ Marker-wrapped sections for both CLAUDE.md and AGENTS.md
• ✅ Custom path handling
• ✅ Rule filtering
• ✅ List functionality
• ✅ Unknown rule warnings
• ✅ Edge cases: empty templates, nested conditionals, excessive blank lines

🔒 Security Assessment

No security issues found:

• ✅ Template processing uses trusted bundled content only
• ✅ No user input reaches regex processing without validation
• ✅ File system access properly scoped to package directory
• ✅ maxIterations prevents DoS from infinite loops

🚀 Performance Impact

Significant improvement:

• ✅ Removed 156+ lines of capability matrix evaluation
• ✅ Simplified template processing
• ✅ Linear processing with maxIterations cap

✨ Overall Assessment

Score: 9/10 - Excellent refactoring

Recommendation: ✅ APPROVE - Ready to merge

No blocking issues found. All changes improve codebase quality. The error handling improvements in LSP commands are particularly welcome.

🎯 Follow-up Suggestions (Optional)

Nice-to-haves for future PRs:

1. Consider documenting migration path for existing .claude/rules/ users
2. Add example of marker-based update to SKILL.md

Great work on this refactoring! The simplification from multi-agent abstraction to unified rules directory is exactly the right direction.