Skip to content

Conversation

@adslaton
Copy link
Contributor

@adslaton adslaton commented Jul 9, 2025

Summary

Aligns the shared-commands directory with the upstream template structure to ensure consistency and maintainability.

Changes Made

Removed Commands (Not in Template)

  • add-spec-comment.sh - Template doesn't include comment management
  • add-user-story-comment.sh - Template doesn't include user story functionality
  • create-epic.sh - Template doesn't include epic management
  • create-user-story-issue.sh - Template focuses on specs, not user stories

Added Template Files

  • .gitignore - Proper ignore rules for shared-commands directory
  • lib/templates/spec-template.md - Template for technical specifications
  • package.json - Updated to match template exactly with:
    • Scoped package name @stillrivercode/shared-commands
    • Binary configuration for CLI usage
    • Updated dependencies to template versions
    • Files array for npm publishing

Retained Commands (Template Compatible)

  • analyze-issue.sh - Analyzes GitHub issues
  • create-spec-issue.sh - Creates technical specification issues
  • roadmap.sh - Manages project roadmaps

Benefits

  • Consistency: Matches upstream template structure exactly
  • Maintainability: Easier to sync with future template updates
  • Reduced Complexity: Fewer commands to maintain
  • Better Packaging: Proper npm package configuration
  • Template Compatibility: Can receive template updates seamlessly

Test Plan

  • Verify remaining commands work correctly
  • Confirm package.json matches template structure
  • Validate all template files are present
  • Test help outputs for all commands

🤖 Generated with Claude Code

- Remove unused commands not in template:
  - add-spec-comment.sh
  - add-user-story-comment.sh
  - create-epic.sh
  - create-user-story-issue.sh
- Add missing template files:
  - .gitignore
  - lib/templates/spec-template.md
  - package.json (updated to match template exactly)
- Keep only template commands:
  - analyze-issue.sh
  - create-spec-issue.sh
  - roadmap.sh

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@adslaton adslaton self-assigned this Jul 9, 2025
@github-actions
Copy link

github-actions bot commented Jul 9, 2025

🔒 Security Scan Results

✅ npm audit - Dependency Security

No vulnerable dependencies found.

⚠️ audit-ci Security Check

No results file generated.

✅ ESLint Security Analysis

No security issues found.


All security scans passed! No issues found.

@github-actions
Copy link

github-actions bot commented Jul 9, 2025

🤖 AI Review

Pull Request Review: Align shared-commands with upstream template

Summary of Changes

This PR performs a significant restructuring to align the shared-commands directory with an upstream template:

Removed Files:

  • 4 command scripts (add-spec-comment.sh, add-user-story-comment.sh, create-epic.sh, create-user-story-issue.sh)

Added Files:

  • .gitignore with comprehensive ignore rules
  • lib/templates/spec-template.md template file
  • package.json with npm package configuration

Strengths

Clear Alignment Strategy: Well-documented rationale for template alignment
Comprehensive .gitignore: Covers Node.js, OS files, editors, and temporary files
Professional Package Structure: Proper npm scoping and binary configuration
Template Quality: The spec template is comprehensive and well-structured
Documentation: Good explanation of changes and benefits

Issues and Concerns

🔴 Critical Issues

  1. Breaking Changes Without Migration Path

    # These commands are completely removed with no deprecation notice
    - add-spec-comment.sh
    - add-user-story-comment.sh  
    - create-epic.sh
    - create-user-story-issue.sh

    Risk: Existing users/CI pipelines may break immediately

  2. Missing Implementation Files

    // package.json references files that don't exist in this PR
    "main": "index.js",           // ❌ Not present
    "bin": { "shared-commands": "index.js" }, // ❌ Not present  
    "postinstall.js"             // ❌ Referenced but not included
    "setup.sh"                   // ❌ Referenced but not included

⚠️ Major Concerns

  1. No Dependency Analysis

    • No evidence that removed commands aren't used elsewhere
    • Missing impact analysis on existing workflows
  2. Template Placeholder Issues

    # In spec-template.md - these will break if not replaced
    {{TITLE}}           # Handlebars syntax without processor
    {{ISSUE_NUMBER}}    # May cause confusion
    {{#USER_STORY_ISSUE}} # Conditional blocks need handling
  3. Test Coverage Claims Unverified

    • PR description claims tests passed but no test files visible
    • No evidence of actual testing of remaining commands

⚠️ Security & Performance

  1. Package.json Security

    {
      "author": "",              // Empty author field
      "scripts": {
        "postinstall": "node postinstall.js"  // Runs arbitrary code on install
      }
    }
  2. Missing Validation

    • No validation that remaining commands work with new structure
    • No verification of lib directory compatibility

Suggestions for Improvement

Immediate Actions Required

  1. Add Missing Files

    # Create these referenced files
    touch index.js postinstall.js setup.sh
  2. Provide Migration Guide

    ## Migration Guide
    
    ### Removed Commands
    - `add-spec-comment.sh` → Use `create-spec-issue.sh --issue` instead
    - `create-epic.sh` → Use GitHub Projects or regular issues with 'epic' label
  3. Fix Template Syntax

    # Replace Handlebars with actual placeholders or document processing requirements
    # Technical Specification: [TITLE]
    **Issue:** #[ISSUE_NUMBER]
  4. Complete Package Structure

    // index.js
    #!/usr/bin/env node
    console.log('Shared commands CLI');

Recommended Additions

  1. Add Deprecation Period

    # Instead of removing, deprecate first
    echo "WARNING: This command is deprecated. Use X instead." >&2
    exit 1
  2. Validation Scripts

    # Add validation that remaining commands work
    ./commands/analyze-issue.sh --help
    ./commands/create-spec-issue.sh --help  
    ./commands/roadmap.sh --help
  3. Security Improvements

    {
      "author": "Your Name <email@example.com>",
      "repository": {
        "type": "git",
        "url": "your-repo-url"
      }
    }

Testing Requirements

Before Merge:

  • Verify all referenced files in package.json exist
  • Test remaining commands work correctly
  • Validate template syntax doesn't break tools
  • Confirm no existing workflows depend on removed commands
  • Test npm package installation process

Overall Recommendation

REQUEST CHANGES 🔄

While the template alignment strategy is sound, this PR has several critical issues that must be addressed:

  1. Broken package.json - References non-existent files
  2. No migration path - Breaking changes without guidance
  3. Untested changes - Claims of testing not substantiated
  4. Template syntax issues - May break downstream tools

Suggested Approach

  1. Phase 1: Add missing files, fix package.json
  2. Phase 2: Provide migration guide and deprecation notices
  3. Phase 3: Validate all functionality works
  4. Phase 4: Merge with proper release notes

The concept is excellent, but the execution needs refinement to avoid breaking existing users and workflows.


This review was automatically generated by anthropic/claude-sonnet-4 via OpenRouter. Please consider it as supplementary feedback alongside human review.

@github-actions github-actions bot added ai-reviewed Reviewed by AI PR review system breaking-change Breaking API changes labels Jul 9, 2025
@adslaton
Copy link
Contributor Author

adslaton commented Jul 9, 2025

Response to AI Review Feedback

Thanks for the detailed review! Addressed the main concerns:

✅ Fixed Issues

  1. Missing Files: All files referenced in package.json now exist and are properly configured

      • Updated to reference only template commands
      • Already existed, checks for required dependencies
      • Updated usage examples to match current commands
  2. Package.json Complete: Added proper author information

  3. Template Syntax: The {{TITLE}} and {{#USER_STORY_ISSUE}} syntax is correct - it's Handlebars templating used by the shared commands system

📝 Migration Plan Note

No migration plan needed - This project has very low adoption so far, so breaking changes won't impact existing users. The removed commands weren't being used in production workflows.

🧪 Verification

All remaining commands tested and working:

  • ./commands/analyze-issue.sh --help
  • ./commands/create-spec-issue.sh --help
  • ./commands/roadmap.sh --help

Ready for merge! 🚀

- Update index.js to reference only template commands
- Update setup.sh usage examples for current commands
- Add proper author to package.json
- All referenced files now exist and work correctly

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@adslaton adslaton merged commit c6876d6 into main Jul 9, 2025
3 checks passed
@adslaton adslaton deleted the feature/align-shared-commands-with-template branch July 9, 2025 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-reviewed Reviewed by AI PR review system breaking-change Breaking API changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants