Skip to content

refactoring(check_pr): Move script in github_scripts dir #138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cap-ang wants to merge 1 commit into from
Closed

refactoring(check_pr): Move script in github_scripts dir #138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 4 additions & 102 deletions .github/workflows/check_pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ jobs:
contents: read # git diff
pull-requests: write # to add labels and comments
issues: write
if: ${{ github.event_name == 'pull_request' }}
steps:
- uses: actions/checkout@f43a0e5ff2bd294095638e18286ca9a3d1956744 # v3
with:
Expand All @@ -89,109 +90,10 @@ jobs:
- name: Check Size
uses: actions/github-script@d556feaca394842dc55e4734bf3bb9f685482fa0 # v6.3.3
env:
HEAD_REF: ${{ github.head_ref }}
BRANCH_NAME: ${{ github.head_ref }}
IGNORED_FILES: openapi.json, openapi-node.json
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const additions = context.payload.pull_request.additions || 0
const deletions = context.payload.pull_request.deletions || 0
var changes = additions + deletions
console.log('additions: '+additions+' + deletions: '+deletions+ ' = total changes: ' + changes);

const { IGNORED_FILES } = process.env
const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
if (ignored_files.length > 0){
var ignored = 0
const execSync = require('child_process').execSync;
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/"${HEAD_REF}" | grep ' + file + ' | cut -f 1', { encoding: 'utf-8' })
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/"${HEAD_REF}" | grep ' + file + ' | cut -f 2', { encoding: 'utf-8' })

const ignored_additions = ignored_additions_str.split('\n').map(elem=> parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);
const ignored_deletions = ignored_deletions_str.split('\n').map(elem=> parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);

ignored += ignored_additions + ignored_deletions;
}
changes -= ignored
console.log('ignored lines: ' + ignored + ' , consider changes: ' + changes);
}

if (changes < 200){
github.rest.issues.addLabels({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
labels: ['size/small']
})


var labels = await github.rest.issues.listLabelsOnIssue({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
});

if (labels.data.find(label => label.name == 'size/large')){
github.rest.issues.removeLabel({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
name: 'size/large'
})
}
}

if (changes > 400){
github.rest.issues.addLabels({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
labels: ['size/large']
})

var comments = await github.rest.issues.listComments({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
});
for (const comment of comments.data) {
if (comment.body.includes('This PR exceeds the recommended size')){
github.rest.issues.deleteComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: comment.id
})
}
}

github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'This PR exceeds the recommended size of 400 lines. Please make sure you are NOT addressing multiple issues with one PR. _Note this PR might be rejected due to its size._'
})

var labels = await github.rest.issues.listLabelsOnIssue({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
});

if (labels.data.find(label => label.name == 'size/small')){
github.rest.issues.removeLabel({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
name: 'size/small'
})
}

}

const script = require('./.github/workflows/github_scripts/check_size.js')
await script({github, context, core})
99 changes: 99 additions & 0 deletions .github/workflows/github_scripts/check_size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
module.exports = async ({github, context, core}) => {
const additions = context.payload.pull_request.additions || 0
const deletions = context.payload.pull_request.deletions || 0
let changes = additions + deletions;
console.log('additions: ' + additions + ' + deletions: ' + deletions + ' = total changes: ' + changes);

const {IGNORED_FILES, BRANCH_NAME} = process.env
const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
if (ignored_files.length > 0) {
var ignored = 0
const execSync = require('child_process').execSync;
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})

Check warning

Code scanning / CodeQL

Indirect uncontrolled command line Medium

This command depends on an unsanitized
environment variable
Loading
.

Copilot Autofix

AI 3 days ago

In general, the fix is to stop passing untrusted data (like environment variables) into shell commands via string concatenation. Instead, use an API that does not invoke a shell and accepts the command and its arguments as an array (e.g., child_process.execFileSync or spawnSync), or, if a shell is unavoidable, properly escape any untrusted values before interpolation.

For this specific file, the problematic parts are lines 14–15, where execSync is called with a shell command string containing BRANCH_NAME and a pipeline to grep/cut. We can replace the use of execSync with execFileSync and pass the arguments as an array so that BRANCH_NAME is not interpreted by a shell. To preserve existing behavior (which uses grep and cut to process the git diff --numstat output), we can instead: (1) call git directly via execFileSync with arguments ['--no-pager','diff','--numstat',\origin/main..origin/${BRANCH_NAME}`], (2) parse the resulting text in JavaScript to extract additions and deletions for each ignored file, and (3) compute the totals as before. This avoids any shell, gets rid of grep/cut, and keeps functionality equivalent. Concretely, inside .github/workflows/github_scripts/check_size.js, we will: add a require('child_process').execFileSyncalongside (or replacing)execSync, call it once to obtain the diff output, then for each fileinignored_files`, parse that shared diff output and sum the additions/deletions for matching lines. Lines 11–22 will be updated accordingly, with no changes to surrounding logic.

Suggested changeset 1
.github/workflows/github_scripts/check_size.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/github_scripts/check_size.js b/.github/workflows/github_scripts/check_size.js
--- a/.github/workflows/github_scripts/check_size.js
+++ b/.github/workflows/github_scripts/check_size.js
@@ -8,19 +8,19 @@
     const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
     if (ignored_files.length > 0) {
         var ignored = 0
-        const execSync = require('child_process').execSync;
-        for (const file of IGNORED_FILES.trim().split(',')) {
+        const execFileSync = require('child_process').execFileSync;
+        const diffOutput = execFileSync('git', ['--no-pager', 'diff', '--numstat', 'origin/main..origin/' + BRANCH_NAME], {encoding: 'utf-8'});
+        for (const file of ignored_files) {
+            const lines = diffOutput.split('\n').filter(l => l.includes(file));
+            const ignored_additions = lines
+                .map(l => l.trim().split('\t')[0])
+                .map(val => parseInt(val || 0))
+                .reduce((accumulator, currentValue) => accumulator + currentValue, 0);
+            const ignored_deletions = lines
+                .map(l => l.trim().split('\t')[1])
+                .map(val => parseInt(val || 0))
+                .reduce((accumulator, currentValue) => accumulator + currentValue, 0);
 
-            const ignored_additions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
-            const ignored_deletions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})
-
-            const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
-                (accumulator, currentValue) => accumulator + currentValue,
-                0);
-            const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
-                (accumulator, currentValue) => accumulator + currentValue,
-                0);
-
             ignored += ignored_additions + ignored_deletions;
         }
         changes -= ignored
EOF
@@ -8,19 +8,19 @@
const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
if (ignored_files.length > 0) {
var ignored = 0
const execSync = require('child_process').execSync;
for (const file of IGNORED_FILES.trim().split(',')) {
const execFileSync = require('child_process').execFileSync;
const diffOutput = execFileSync('git', ['--no-pager', 'diff', '--numstat', 'origin/main..origin/' + BRANCH_NAME], {encoding: 'utf-8'});
for (const file of ignored_files) {
const lines = diffOutput.split('\n').filter(l => l.includes(file));
const ignored_additions = lines
.map(l => l.trim().split('\t')[0])
.map(val => parseInt(val || 0))
.reduce((accumulator, currentValue) => accumulator + currentValue, 0);
const ignored_deletions = lines
.map(l => l.trim().split('\t')[1])
.map(val => parseInt(val || 0))
.reduce((accumulator, currentValue) => accumulator + currentValue, 0);

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})

const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);
const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);

ignored += ignored_additions + ignored_deletions;
}
changes -= ignored
Copilot is powered by AI and may make mistakes. Always verify output.
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})

Check warning

Code scanning / CodeQL

Indirect uncontrolled command line Medium

This command depends on an unsanitized
environment variable
Loading
.

Copilot Autofix

AI 3 days ago

In general, the problem should be fixed by avoiding shell command construction with string concatenation for untrusted data, and instead using APIs that accept an argument array and do not invoke a shell (child_process.execFileSync or spawnSync with shell: false). This prevents shell metacharacters in BRANCH_NAME (or in file) from being interpreted by a shell.

The best fix here is to (1) switch from execSync to execFileSync and (2) replace the grep/cut pipeline with logic implemented in JavaScript: run git diff --numstat as git with arguments, capture its stdout, then filter lines and extract columns in Node. This removes all shell parsing and still preserves behavior.

Concretely, in .github/workflows/github_scripts/check_size.js:

  • At line 11, change the import from execSync to execFileSync.
  • Replace the two execSync('git --no-pager diff ... | grep ... | cut -f N', ...) calls at lines 14–15 with a single execFileSync('git', [...]) call that gets the full --numstat output once per loop iteration (or once total; but to avoid broader refactoring, keep it per loop), then:
    • Split the output into lines.
    • For each line that includes file, split on whitespace to get additions, deletions, and filename.
    • Accumulate additions and deletions into ignored_additions_str and ignored_deletions_str-equivalents (or directly into numbers).
  • Maintain the rest of the logic (parsing, reducing) so functionality stays the same.

No extra external libraries are needed; we only use the standard child_process module and string processing.

Suggested changeset 1
.github/workflows/github_scripts/check_size.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/.github/workflows/github_scripts/check_size.js b/.github/workflows/github_scripts/check_size.js
--- a/.github/workflows/github_scripts/check_size.js
+++ b/.github/workflows/github_scripts/check_size.js
@@ -8,19 +8,38 @@
     const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
     if (ignored_files.length > 0) {
         var ignored = 0
-        const execSync = require('child_process').execSync;
+        const execFileSync = require('child_process').execFileSync;
         for (const file of IGNORED_FILES.trim().split(',')) {
 
-            const ignored_additions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
-            const ignored_deletions_str = execSync('git --no-pager  diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})
+            const numstatOutput = execFileSync(
+                'git',
+                ['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`],
+                {encoding: 'utf-8'}
+            );
 
-            const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
-                (accumulator, currentValue) => accumulator + currentValue,
-                0);
-            const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
-                (accumulator, currentValue) => accumulator + currentValue,
-                0);
+            const matchingLines = numstatOutput
+                .split('\n')
+                .filter(line => line && line.includes(file));
 
+            const additionsList = [];
+            const deletionsList = [];
+
+            for (const line of matchingLines) {
+                const parts = line.trim().split(/\s+/);
+                if (parts.length >= 3) {
+                    additionsList.push(parts[0]);
+                    deletionsList.push(parts[1]);
+                }
+            }
+
+            const ignored_additions = additionsList
+                .map(elem => parseInt(elem || 0))
+                .reduce((accumulator, currentValue) => accumulator + currentValue, 0);
+
+            const ignored_deletions = deletionsList
+                .map(elem => parseInt(elem || 0))
+                .reduce((accumulator, currentValue) => accumulator + currentValue, 0);
+
             ignored += ignored_additions + ignored_deletions;
         }
         changes -= ignored
EOF
@@ -8,19 +8,38 @@
const ignored_files = IGNORED_FILES.trim().split(',').filter(word => word.length > 0);
if (ignored_files.length > 0) {
var ignored = 0
const execSync = require('child_process').execSync;
const execFileSync = require('child_process').execFileSync;
for (const file of IGNORED_FILES.trim().split(',')) {

const ignored_additions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 1', {encoding: 'utf-8'})
const ignored_deletions_str = execSync('git --no-pager diff --numstat origin/main..origin/' + BRANCH_NAME + ' | grep ' + file + ' | cut -f 2', {encoding: 'utf-8'})
const numstatOutput = execFileSync(
'git',
['--no-pager', 'diff', '--numstat', `origin/main..origin/${BRANCH_NAME}`],
{encoding: 'utf-8'}
);

const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);
const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);
const matchingLines = numstatOutput
.split('\n')
.filter(line => line && line.includes(file));

const additionsList = [];
const deletionsList = [];

for (const line of matchingLines) {
const parts = line.trim().split(/\s+/);
if (parts.length >= 3) {
additionsList.push(parts[0]);
deletionsList.push(parts[1]);
}
}

const ignored_additions = additionsList
.map(elem => parseInt(elem || 0))
.reduce((accumulator, currentValue) => accumulator + currentValue, 0);

const ignored_deletions = deletionsList
.map(elem => parseInt(elem || 0))
.reduce((accumulator, currentValue) => accumulator + currentValue, 0);

ignored += ignored_additions + ignored_deletions;
}
changes -= ignored
Copilot is powered by AI and may make mistakes. Always verify output.

const ignored_additions = ignored_additions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);
const ignored_deletions = ignored_deletions_str.split('\n').map(elem => parseInt(elem || 0)).reduce(
(accumulator, currentValue) => accumulator + currentValue,
0);

ignored += ignored_additions + ignored_deletions;
}
changes -= ignored
console.log('ignored lines: ' + ignored + ' , consider changes: ' + changes);
}

var labels = await github.rest.issues.listLabelsOnIssue({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
});

if (changes <= 400) {
if (labels.data.find(label => label.name === 'size/large')) {
github.rest.issues.removeLabel({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
name: 'size/large'
})
}
}

if (changes >= 200) {
if (labels.data.find(label => label.name === 'size/small')) {
github.rest.issues.removeLabel({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
name: 'size/small'
})
}
}

var comments = await github.rest.issues.listComments({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo
});
for (const comment of comments.data) {
if (comment.body.includes('This PR exceeds the recommended size')) {
github.rest.issues.deleteComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: comment.id
})
}
}

if (changes < 200) {
github.rest.issues.addLabels({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
labels: ['size/small']
})
}

if (changes > 400) {
github.rest.issues.addLabels({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
labels: ['size/large']
})

github.rest.issues.createComment({
issue_number: context.issue.number,
owner: context.repo.owner,
repo: context.repo.repo,
body: 'This PR exceeds the recommended size of 400 lines. Please make sure you are NOT addressing multiple issues with one PR. _Note this PR might be rejected due to its size._'
})

}
}
Loading