Skip to content

Conversation

@rahuldevikar761
Copy link
Collaborator

No description provided.

Copilot AI review requested due to automatic review settings December 9, 2025 20:30
@rahuldevikar761 rahuldevikar761 requested a review from a team as a code owner December 9, 2025 20:30
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a GitHub Actions workflow that automatically versions Node.js sample projects based on git commit history, following a GitVersion-style approach where the patch version equals the commit count for each sample directory.

Key Changes:

  • Adds automated version bumping workflow triggered on pushes to main and user branches
  • Implements commit-count-based versioning (major.minor.commitCount)
  • Creates pull requests with version updates when Node.js sample code changes

});

// Stage the file
execSync(`git add ${packageJsonPath}`);
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same command injection vulnerability as line 90. The packageJsonPath variable should be properly escaped when used in shell commands:

execSync('git add ' + JSON.stringify(packageJsonPath));

Copilot uses AI. Check for mistakes.
Comment on lines +93 to +95
const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
const currentVersion = packageJson.version;
const versionParts = currentVersion.split('.');
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for JSON parsing and file operations. If package.json is malformed or cannot be read, this will throw an unhandled error and the workflow will fail without a clear message.

Add try-catch blocks around file operations:

try {
  const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
  const currentVersion = packageJson.version;
  if (!currentVersion) {
    console.log(`  No version field found, skipping`);
    continue;
  }
  // ... rest of logic
} catch (error) {
  console.error(`  Error processing ${packageJsonPath}: ${error.message}`);
  continue; // Skip this package and continue with others
}
Suggested change
const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
const currentVersion = packageJson.version;
const versionParts = currentVersion.split('.');
let packageJson, currentVersion, versionParts;
try {
packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
currentVersion = packageJson.version;
if (!currentVersion) {
console.log(` No version field found, skipping`);
continue;
}
versionParts = currentVersion.split('.');
} catch (error) {
console.error(` Error processing ${packageJsonPath}: ${error.message}`);
continue;
}

Copilot uses AI. Check for mistakes.
const packageJson = JSON.parse(fs.readFileSync(packageJsonPath, 'utf-8'));
const currentVersion = packageJson.version;
const versionParts = currentVersion.split('.');

Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version string splitting assumes a standard semver format (major.minor.patch), but doesn't validate this assumption. If the version in package.json has a different format (e.g., "1.0" or "1.0.0-beta.1"), accessing versionParts[1] could result in undefined values, leading to malformed versions like "1.undefined.5".

Add validation:

const versionParts = currentVersion.split('.');
if (versionParts.length < 2) {
  console.log(`  Invalid version format: ${currentVersion}, skipping`);
  continue;
}

// Generate new version: major.minor.commitCount
const newVersion = `${versionParts[0]}.${versionParts[1]}.${commitCount}`;
Suggested change
// Validate version format: must have at least major and minor
if (versionParts.length < 2) {
console.log(` Invalid version format: ${currentVersion}, skipping`);
continue;
}

Copilot uses AI. Check for mistakes.
console.log('Sample directories:', sampleDirsArray);

if (sampleDirsArray.length === 0) {
console.log('No samples to version');
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The return statement on line 80 doesn't propagate correctly due to being inside a uses: actions/github-script@v7 block. The script needs to use return to set the output, but it should return a value that can be used by the conditional in line 127.

The logic is correct as-is, but consider making it more explicit by ensuring the return value structure matches what's expected in line 127. The current implementation looks correct, but add a comment to clarify the return contract:

// Return object with hasChanges boolean and bumped array for downstream steps
return { hasChanges: false, bumped: [] };
Suggested change
console.log('No samples to version');
console.log('No samples to version');
// Return object with hasChanges boolean and bumped array for downstream steps

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,173 @@
name: Auto-version Node.js Samples
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Microsoft copyright header. GitHub Actions workflow YAML files require a copyright header at the top:

# Copyright (c) Microsoft Corporation.
# Licensed under the MIT License.

name: Auto-version Node.js Samples

Copilot generated this review using guidance from repository custom instructions.
Comment on lines 13 to 19
permissions:
contents: write
pull-requests: write

jobs:
version-bump:
runs-on: ubuntu-latest
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Permissions should be scoped at the job level rather than the workflow level for better security. Move the permissions block from lines 13-15 to be under the version-bump job (after line 19).

Example:

jobs:
  version-bump:
    runs-on: ubuntu-latest
    permissions:
      contents: write
      pull-requests: write

This follows the principle of least privilege by limiting permissions to only the jobs that need them.

Suggested change
permissions:
contents: write
pull-requests: write
jobs:
version-bump:
runs-on: ubuntu-latest
jobs:
version-bump:
runs-on: ubuntu-latest
permissions:
contents: write
pull-requests: write

Copilot uses AI. Check for mistakes.
console.log(` Git commits: ${commitCount}`);
console.log(` New version: ${newVersion}`);

if (newVersion !== currentVersion) {
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version comparison logic has a bug. When the commit count is small (e.g., "5"), but the current version's patch is a string comparison larger (e.g., "0.1.50"), the new version "0.1.5" will appear different even though it represents fewer commits. This can cause the version to go backwards.

The logic should parse and compare versions numerically or check if newVersion is actually greater than currentVersion:

if (newVersion !== currentVersion && parseInt(commitCount) > parseInt(versionParts[2] || 0)) {
  // Update package.json
  packageJson.version = newVersion;
  // ...
}

This prevents the workflow from creating PRs that would downgrade versions.

Suggested change
if (newVersion !== currentVersion) {
// Only bump version if commitCount (patch) is greater than current patch version
if (parseInt(commitCount, 10) > parseInt(versionParts[2] || "0", 10)) {

Copilot uses AI. Check for mistakes.
console.log(`\nProcessing ${packageJsonPath}`);

// Get git commit count for this directory (auto-incrementing patch version)
const commitCount = execSync(`git rev-list --count HEAD -- ${dir}`, { encoding: 'utf-8' }).trim();
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential command injection vulnerability. The dir variable is used directly in shell commands without proper escaping. If a directory name contains special characters or shell metacharacters, this could lead to command injection or errors.

Use proper escaping or pass the variable safely:

const commitCount = execSync('git rev-list --count HEAD -- ' + JSON.stringify(dir), { encoding: 'utf-8' }).trim();

Or better yet, use the shell: false option with an array of arguments where supported, or sanitize the directory path.

Copilot uses AI. Check for mistakes.
push:
branches:
- main
- 'users/**'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done for test purposes and should be removed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants