feat: Add VS Code Extension CI/CD Infrastructure

[madhur310]

Draft PR to test VS Code CI infrastructure. Will update description after testing.

[peternhale]

Reusability Review

This PR provides a comprehensive VS Code extension CI/CD infrastructure with excellent architectural patterns. However, several issues prevent it from being fully reusable across different extension projects.

Critical Issues (Must Fix)

1. Branch-specific action references - All references will break after this branch is merged
2. Hardcoded Apex/Salesforce messaging - Slack notifications contain "Apex Language Support" text
3. Missing action definitions - Several referenced actions are not included in this PR
4. Incomplete documentation - Required secrets and prerequisites not fully documented

Medium Priority

5. Node.js version not parameterized in several workflows
6. Coupled versioning strategy - Odd/even minor version logic may not apply to all extensions
7. Inconsistent action reference style - Mix of local and remote references

See inline comments for specific recommendations.

[peternhale]

Reusability Review

This PR provides a comprehensive VS Code extension CI/CD infrastructure with excellent architectural patterns. However, several issues prevent it from being fully reusable across different extension projects.

Critical Issues (Must Fix)

1. Branch-specific action references - All references to the feature branch will break after merge
2. Hardcoded Apex/Salesforce messaging - Slack notifications contain "Apex Language Support" text
3. Missing action definitions - Several referenced actions are not included in this PR
4. Incomplete documentation - Required secrets and prerequisites not fully documented

Medium Priority

5. Node.js version not parameterized in several workflows
6. Coupled versioning strategy - Odd/even minor version logic may not apply to all extensions
7. Inconsistent action reference style - Mix of local and remote references

See inline comments for specific recommendations.

[peternhale]

Reusability Review

This PR provides a comprehensive VS Code extension CI/CD infrastructure with excellent architectural patterns. However, several issues prevent it from being fully reusable across different extension projects.

---

✅ Strengths (Maintain These Patterns)

• Strong workflow composition - Clear separation of CI, package, publish, promote workflows
• Excellent idempotency - Safe to re-run workflows (checks for existing releases/assets)
• Comprehensive dry-run support - Excellent for testing without side effects
• Parameterized directory structure - extensions-root input enables flexibility

---

❌ Critical Issues (Must Fix Before Merge)

1. Branch-Specific References Will Break After Merge

Files affected:

• .github/workflows/vscode-ci-template.yml (lines 354, 424, 434)
• .github/workflows/vscode-promote-prerelease.yml (lines 1822, 1849, 1880)
• .github/workflows/vscode-publish-extensions.yml (lines 2560, 2640, 2930, 2953, 3142)
• Multiple other workflows

All references to @feat/add-vscode-extension-ci will fail after this branch is merged.

Fix: Update all workflow references to @main or use version tags:

uses: salesforcecli/github-workflows/.github/workflows/vscode-package.yml@main

2. Hardcoded Apex/Salesforce-Specific Content

File: .github/workflows/vscode-publish-extensions.yml (line ~3435)

Slack notification contains hardcoded "🎉 Apex Language Support Extensions Released Successfully!" message.

Fix: Make notification messages configurable:

inputs:
  slack-notification-title:
    description: 'Title for Slack notifications'
    required: false
    default: '🎉 Extensions Released Successfully!'
    type: string

3. Missing Action Definitions

File: .github/workflows/vscode-manual-publish.yml (line 514)

Workflows reference actions not included in this PR:

• ./.github/actions/npm-install-with-retries
• ./.github/actions/npmInstallWithRetries (note casing inconsistency)
• ./.github/actions/check-ci-status
• ./.github/actions/repackage-vsix-stable

Fix: Either include these actions in this PR, or clearly document which actions consuming repos must provide vs. which are in this shared workflows repo.

4. Incomplete Documentation

File: README.md (starting line 3764)

Missing critical information for consuming repositories:

• Prerequisites (which actions must exist)
• Complete secrets documentation (purpose of each secret)
• Repository structure expectations
• Versioning strategy explanation
• Architecture/workflow composition diagram

Fix: Add a comprehensive "Prerequisites and Setup" section.

---

⚠️ Medium Priority Issues

5. Node.js Version Not Parameterized

Files: Most workflows (.github/workflows/vscode-manual-publish.yml line 729, etc.)

Node.js 22.x is hardcoded in multiple workflows. This is already correctly parameterized in vscode-package.yml but missing from others.

Fix: Add to all workflows:

inputs:
  node-version:
    description: 'Node.js version to use'
    required: false
    default: '22.x'
    type: string

6. Coupled Versioning Strategy

File: .github/workflows/vscode-publish-extensions.yml (line ~2669)

The odd/even minor version strategy is deeply embedded in version bumping logic. This convention may not apply to all VS Code extensions.

Recommendation: Extract into a configurable strategy or allow custom scripts.

7. Inconsistent Action Reference Style

Files: Multiple workflows

Mix of local references (./.github/actions/...) and remote references with inconsistent naming (npmInstallWithRetries vs npm-install-with-retries).

Fix: Standardize on one approach and document the pattern.

---

📋 Action Items Summary

Before this PR can be merged:

1. ✅ Update all @feat/add-vscode-extension-ci references to @main
2. ✅ Parameterize Slack notification messages
3. ✅ Include missing actions OR document what consuming repos must provide
4. ✅ Expand README documentation with prerequisites, secrets, and architecture
5. ⚠️ Consider parameterizing Node.js version across all workflows
6. ⚠️ Document or extract the odd/even versioning strategy
7. ⚠️ Standardize action reference style

Overall, this is a solid foundation with excellent architectural patterns. The main blockers are documentation gaps and a few hardcoded assumptions that limit reusability.

[peternhale]

Missing Implementation: extensions: 'changed' Change Detection

I've reviewed the extension publishing workflows in this PR against the reference implementation in apex-language-support-my-work and identified a critical missing feature: the extensions: 'changed' parameter is documented but not implemented.

Current State

vscode-publish-extensions.yml documents the feature:

extensions:
  description: 'Extensions to release (all, changed, or comma-separated extension names)'
  required: false
  default: 'changed'
  type: string

However, the workflow simply passes this value through to the version bumping logic without any change detection:

• Line 59: Parameter accepts 'changed' as a value
• Line 214: SELECTED_EXTENSIONS: ${{ inputs.extensions }} is passed directly to bash script
• Lines 352-382: Version bump logic iterates over $SELECTED_EXTENSIONS but doesn't detect changes

Result: If a caller passes extensions: 'changed', the workflow will literally look for an extension named "changed" and fail to find it.

Reference Implementation

The apex-language-support-my-work repository implements this feature using a dedicated determine-changes job with TypeScript-based change detection:

Workflow structure (nightly-extensions.yml:78-106):

determine-changes:
  runs-on: ubuntu-latest
  outputs:
    selected-extensions: ${{ steps.changes.outputs.selected-extensions }}
    version-bumps: ${{ steps.changes.outputs.version-bumps }}
  steps:
    - name: Determine changes and version bumps
      id: changes
      env:
        IS_NIGHTLY: 'true'
        VERSION_BUMP: 'auto'
        SELECTED_EXTENSIONS: ${{ inputs.extensions }}
      run: |
        npx tsx .github/scripts/index.ts ext-change-detector

bump-versions:
  needs: [determine-changes]  # ← Depends on change detection
  if: needs.determine-changes.outputs.selected-extensions != ''
  env:
    SELECTED_EXTENSIONS: ${{ needs.determine-changes.outputs.selected-extensions }}

Change Detection Logic (.github/scripts/ext-change-detector.ts):

1. Discovery: Scans packages/ for VS Code extensions (packages with publisher field)
2. Git-based detection: For each extension, finds last release tag using pattern {extension-name}-v{version} and runs git diff against that tag
3. Selection handling:
   • 'all' → returns all available extensions
   • 'changed' → returns extensions with git diff changes since last tag
   • 'none' → returns empty list
   • Specific names → validates and returns those extensions
4. Conventional commit analysis: For version-bump: auto, analyzes commit messages to determine bump type (major/minor/patch)

Impact

Without this implementation:

• ❌ Callers cannot use extensions: 'changed' despite documentation
• ❌ No automatic detection of which extensions need publishing
• ❌ Manual specification of extension names required for every release
• ❌ Nightly builds cannot automatically publish only changed extensions

Recommended Fix

Option A: Port TypeScript Implementation (matches apex-language-support pattern)

1. Add .github/scripts/ directory with change detection logic
2. Add determine-changes job before bump-versions:

determine-changes:
  runs-on: ubuntu-latest
  outputs:
    selected-extensions: ${{ steps.changes.outputs.selected-extensions }}
    version-bumps: ${{ steps.changes.outputs.version-bumps }}
  steps:
    - uses: actions/checkout@v6
      with:
        fetch-depth: 0  # Need full history for git diff
    - uses: actions/setup-node@v6
      with:
        node-version: '22.x'
    - uses: salesforcecli/github-workflows/.github/actions/npmInstallWithRetries@main
    - name: Detect changes
      id: changes
      env:
        SELECTED_EXTENSIONS: ${{ inputs.extensions }}
        VERSION_BUMP: ${{ inputs.version-bump }}
        EXTENSIONS_ROOT: ${{ inputs.extensions-root || 'packages' }}
      run: |
        npx tsx .github/scripts/ext-change-detector.ts

3. Update bump-versions to depend on it:

bump-versions:
  needs: [determine-changes]
  if: needs.determine-changes.outputs.selected-extensions != ''
  env:
    SELECTED_EXTENSIONS: ${{ needs.determine-changes.outputs.selected-extensions }}
    VERSION_BUMP: ${{ needs.determine-changes.outputs.version-bumps }}

Option B: Implement in Bash (simpler dependencies)

Add change detection step before version bumping:

- name: Detect changed extensions
  id: detect-changes
  env:
    USER_INPUT: ${{ inputs.extensions }}
    EXTENSIONS_ROOT: ${{ inputs.extensions-root || 'packages' }}
  run: |
    # Handle special values
    if [ "$USER_INPUT" = "all" ]; then
      CHANGED=$(ls -1 "$EXTENSIONS_ROOT" | paste -sd,)
      echo "selected-extensions=$CHANGED" >> $GITHUB_OUTPUT
      exit 0
    fi
    
    if [ "$USER_INPUT" = "none" ]; then
      echo "selected-extensions=" >> $GITHUB_OUTPUT
      exit 0
    fi
    
    # Detect changes
    if [ "$USER_INPUT" = "changed" ]; then
      CHANGED=""
      for ext_dir in "$EXTENSIONS_ROOT"/*/; do
        ext_name=$(basename "$ext_dir")
        
        # Find last tag for this extension
        last_tag=$(git tag -l "${ext_name}-v*" --sort=-v:refname | head -1)
        
        # Check for changes since last tag
        if [ -z "$last_tag" ]; then
          # No previous tag - first release
          CHANGED="${CHANGED}${CHANGED:+,}${ext_name}"
        elif ! git diff --quiet "$last_tag" HEAD -- "$ext_dir"; then
          # Has changes since last tag
          CHANGED="${CHANGED}${CHANGED:+,}${ext_name}"
        fi
      done
      echo "selected-extensions=$CHANGED" >> $GITHUB_OUTPUT
    else
      # User specified explicit names
      echo "selected-extensions=$USER_INPUT" >> $GITHUB_OUTPUT
    fi

Additional Findings

Package-lock.json Updates Missing

The version bump logic updates package.json files but doesn't update package-lock.json, which can cause dependency drift. The apex-language-support implementation includes:

npm install --package-lock-only --ignore-scripts --no-audit
git add package-lock.json

Per-Extension Tag Pattern

The reference implementation uses {extension-name}-v{version} tags to track each extension independently. Your workflow needs to:

1. Find the last tag per extension (not just the most recent global tag)
2. Compare changes in that extension's directory since its last tag
3. Create new tags in the same pattern after version bumps

Questions for Discussion

1. Scope: Should this PR include the change detection implementation, or defer to consuming repositories?
2. Approach: TypeScript scripts (testable, maintainable) vs. bash (simpler dependencies)?
3. Backward compatibility: How should the workflow behave if a caller passes extensions: 'changed' before this is implemented?

Files Needing Updates

If implementing in this PR:

• .github/workflows/vscode-publish-extensions.yml - Add determine-changes job
• .github/scripts/ext-change-detector.ts - Port from apex-language-support
• .github/scripts/utils.ts - Helper functions for git operations
• .github/scripts/types.ts - Type definitions
• .github/scripts/index.ts - CLI entry point
• package.json - Add tsx dependency if using TypeScript
• Documentation - Update with change detection behavior

---

Reference: Comparison analysis performed against:

• This PR: /Users/peter.hale/alm-cli-git/github-workflows
• Reference: /Users/peter.hale/git/apex-language-support-my-work
• VSE (for mono-repo patterns): /Users/peter.hale/git/vse

[peternhale]

Code Review Summary

This PR adds comprehensive VS Code extension CI/CD infrastructure with solid architecture. However, there are several critical issues that need to be addressed before merging, particularly around change detection race conditions and conventional commit parsing.

Critical Issues

• Race condition in parallel extension builds - Could cause incorrect change detection
• Incomplete conventional commit parsing - Missing breaking change patterns
• Bypass confirmation UX issue - Case-sensitive validation will confuse users

Medium Priority

• Tag sorting type safety
• Commit SHA resolution fragility
• Inline version comparison should use existing utilities

See inline comments for detailed recommendations. Overall this is excellent work - just needs these safety improvements before production use.

Recommendation: Request changes for critical issues, then approve after fixes.

[peternhale]

🔴 CRITICAL: Race Condition in Change Detection

Location: .github/scripts/ext-change-detector.ts:441

const diff = await git.diff([lastTag, 'HEAD', '--', extensionPath]);

Problem: When multiple extensions change in a single commit and workflows run in parallel, there's a race condition:

1. Extensions A and B both detect changes against their last tags
2. Both start building and creating tags
3. If B's tag is created while A is still running, A's subsequent builds might see B's tag as HEAD

Impact: Could cause extensions to be incorrectly marked as unchanged, skip commits, or create tags in wrong order.

Recommendation: Add workflow-level concurrency control in vscode-publish-extensions.yml:

concurrency:
  group: vscode-publish-${{ github.ref }}
  cancel-in-progress: false

This ensures sequential processing and prevents tag creation races.

[peternhale]

🟡 MEDIUM: Incomplete Conventional Commit Parsing

Location: .github/scripts/ext-change-detector.ts:589-595

if (/BREAKING CHANGE/i.test(body) || /^[a-z]+(\([^)]*\))?!:/i.test(firstLine)) {
  return 'major';
}
if (/^feat(\([^)]*\))?:/i.test(firstLine) && bump !== 'major') {
  bump = 'minor';
}

Issues:

1. Doesn't handle scoped breaking changes without colon: feat(api)!
2. The regex requires : after !, but conventional commits spec allows ! without colon
3. Only detects BREAKING CHANGE (singular), not BREAKING CHANGES (plural)

Test Cases that Fail:

• fix(auth)! → Should be major, detected as patch ❌
• BREAKING CHANGES: multiple changes → Not detected ❌

Recommendation:

// Check for breaking changes
const hasBreakingFooter = /^BREAKING[- ]CHANGE[S]?:/im.test(body);
const hasBreakingMarker = /^[a-z]+(\([^)]*\))?!(:|$)/i.test(firstLine);
if (hasBreakingFooter || hasBreakingMarker) {
  return 'major';
}

[peternhale]

🔴 CRITICAL: Bypass Confirmation is Case-Sensitive

Location: .github/workflows/vscode-manual-publish.yml:1594-1598

if [ "$BYPASS" != "BYPASS" ]; then
  echo "ERROR: skip-quality-checks is true but confirm-bypass is not 'BYPASS'."
  exit 1
fi

Problem: Case-sensitive string comparison means bypass, Bypass, or BYPASS (with trailing space) all fail validation. The error message doesn't indicate this, so users will struggle to diagnose why their confirmation isn't working.

Recommendation: Normalize the input before comparison:

BYPASS_NORMALIZED=$(echo "$BYPASS" | tr '[:lower:]' '[:upper:]' | xargs)
if [ "$BYPASS_NORMALIZED" != "BYPASS" ]; then
  echo "ERROR: confirm-bypass must be exactly 'BYPASS' (got: '$BYPASS')"
  exit 1
fi

This provides better UX and clearer error messages.

[peternhale]

🟡 MEDIUM: Tag Sorting Type Safety Issue

Location: .github/scripts/ext-change-detector.ts:465-468

.filter((item) => item.version !== null) // Filter out tags we couldn't parse
.sort((a, b) =>
  // Use proper semver comparison (descending order - newest first)
  compareSemver(b.version!, a.version!),
);

Problem: The ! assertion on line 467 assumes filtering removes all null versions, but TypeScript doesn't narrow the type. If a malformed tag slips through (e.g., regex edge case), the assertion could fail at runtime.

Recommendation: Use a type guard for explicit type narrowing:

.filter((item): item is TagWithVersion & { version: SemanticVersion } => 
  item.version !== null
)
.sort((a, b) => compareSemver(b.version, a.version))

This provides compile-time safety without runtime assertions.

[peternhale]

🟡 MEDIUM: Fragile CI Commit Resolution

Location: .github/workflows/vscode-manual-publish.yml:1798-1829

Issues:

1. Hard-coded check name ("CI Complete") — if consuming repos use different names, this silently fails
2. Arbitrary depth limit (20 commits) — no justification or documentation
3. No caching — if multiple jobs need this SHA, they all repeat the expensive walk

Recommendations:

1. Make the check name configurable via workflow input:

required-ci-checks:
  description: 'Comma-separated list of required CI check names'
  required: false
  default: 'CI Complete'
  type: string

2. Document why 20 is chosen, or make it configurable

3. Cache the result as a workflow output:

outputs:
  ci-commit-sha: ${{ steps.ci-commit.outputs.ci-commit-sha }}

This makes the workflow more flexible and reusable across different repos.

[peternhale]

🟢 LOW: Inline Version Comparison Should Use Existing Utilities

Location: .github/workflows/vscode-promote-stable.yml:3307-3316

IS_GT=$(STABLE_VERSION="$STABLE_VERSION" CURRENT_VERSION="$CURRENT_VERSION" node -e '
  const parse = v => String(v).split("-")[0].split(".").map(Number);
  const a = parse(process.env.STABLE_VERSION);
  const b = parse(process.env.CURRENT_VERSION);
  let gt = false;
  for (let i = 0; i < 3; i++) {
    if (a[i] !== b[i]) { gt = a[i] > b[i]; break; }
  }
  console.log(gt ? "yes" : "no");
')

Issue: This inline script duplicates logic that already exists in utils.ts (compareSemver). Inline scripts are harder to test and maintain. Duplicated logic increases the risk of bugs that won't be caught by unit tests.

Recommendation: Use npx semver CLI directly (already in dependencies):

IS_GT=$(npx semver "$STABLE_VERSION" -r ">$CURRENT_VERSION" && echo "yes" || echo "no")

Or extract to a callable script in .github/scripts/ that can be unit tested.

This reduces duplication and improves maintainability.

[peternhale]

🟢 LOW: Missing Input Validation for Extension Lists

Location: .github/scripts/ext-change-detector.ts:482-494

const selected = selectedExtensionsInput
  .split(',')
  .map((s) => s.trim())
  .filter(Boolean);

Issue: The function doesn't validate malformed comma-separated lists. Edge cases that silently get cleaned up:

• Leading/trailing commas: ,ext-a, → produces empty strings (filtered out)
• Multiple commas: ext-a,,ext-b → produces empty strings
• Only commas: ,,,, → produces empty array

While .filter(Boolean) handles this, it silently ignores the malformed input, which could mask user errors.

Recommendation: Add explicit validation with warnings:

if (/^,|,$|,,/.test(selectedExtensionsInput)) {
  log.warning('Malformed extension list detected (extra commas) — cleaning up');
  log.warning(`Raw input: "${selectedExtensionsInput}"`);
}

This helps users identify typos in their input without breaking the workflow.

[peternhale]

Comprehensive Review Complete

I've reviewed the entire PR with special focus on the change detection logic as requested. This is excellent infrastructure work with solid architecture, but there are several issues that need to be addressed before merging.

---

Summary of Findings

🔴 Critical Issues (Must Fix)

1. Race condition in parallel extension builds — Could cause incorrect change detection when multiple extensions change simultaneously
2. Incomplete conventional commit parsing — Missing breaking change patterns like feat(api)! and BREAKING CHANGES (plural)
3. Bypass confirmation UX issue — Case-sensitive validation will confuse users

🟡 Medium Priority (Should Fix)

4. Tag sorting type safety — Missing type guard after null filtering
5. Commit SHA resolution fragility — Hard-coded check names, arbitrary limits
6. Inline version comparison — Duplicates existing utility logic

🟢 Low Priority (Nice to Have)

7. Input validation warnings — Silently cleans malformed comma-separated lists

---

What I Especially Like ✅

• Per-extension tag tracking — Properly handles multi-extension monorepos
• Git diff-based detection — Correct use of tag ranges for change detection
• User selection intersection — Smart handling of special values (all, changed, none)
• Audit logging — Comprehensive tracking of publish events
• Token masking — Proper security practices
• Idempotent workflows — Rerun safety throughout

---

Next Steps

Please address the 3 critical issues (especially the race condition) and consider the medium-priority improvements. I've posted detailed inline comments for each issue with specific recommendations.

Once these are addressed, this will be production-ready! The odd/even versioning convention is clever and the overall workflow composition is excellent.

Happy to re-review after updates. 🚀

[peternhale]

✅ APPROVED - All Critical Issues Resolved

Excellent work addressing all the feedback from the previous review! All critical issues have been properly fixed.

What Was Fixed ✅

1. Branch references → All updated to @main (commit 304eddd)
2. Conventional commit parsing → Now handles BREAKING CHANGES (plural) and feat! syntax
3. Bypass confirmation UX → Case-insensitive validation implemented
4. Type safety → Type guards properly in place

Code Quality Highlights 🌟

• Per-extension tag tracking - Correctly handles multi-extension monorepos
• Idempotent workflows - Safe to re-run with existing release checks
• Comprehensive audit logging - Excellent compliance trail
• Proper token masking - Security best practices followed
• Dry-run support - All destructive operations respect dry-run mode

Optional Enhancements (Not Blockers)

1. Consider adding a Prerequisites section to README documenting which actions consuming repos must provide
2. Current input validation warnings are acceptable but could optionally be errors for stricter validation

Verdict

This infrastructure is production-ready with excellent architectural patterns, proper error handling, and comprehensive audit trails. The remaining suggestions are minor enhancements, not blockers.

Great job systematically addressing each piece of feedback! 🚀