Detect Changes based on Lockfile #484
Conversation
… changes for the corresponding package
…ies job doesn't execute
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // Not using the repotools version since this uses @action/exec instead of | ||
| // calling execFile from child_process | ||
| export async function getGitRoot() { | ||
| const { stdout } = await getExecOutput('git rev-parse --show-toplevel'); |
There was a problem hiding this comment.
Bug: getExecOutput is called with the full command string as the executable, instead of separating the command and its arguments.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The getExecOutput function is called with the entire command git rev-parse --show-toplevel as a single string for the command parameter. The @actions/exec API expects the executable and its arguments to be separate parameters. This will cause the shell to attempt to execute a non-existent command named "git rev-parse --show-toplevel", leading to a failure in the GitHub Actions workflow.
💡 Suggested Fix
Pass 'git' as the first argument and ['rev-parse', '--show-toplevel'] as the second argument to getExecOutput.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/actions/src/gitRoot.ts#L6
Potential issue: The `getExecOutput` function is called with the entire command `git
rev-parse --show-toplevel` as a single string for the `command` parameter. The
`@actions/exec` API expects the executable and its arguments to be separate parameters.
This will cause the shell to attempt to execute a non-existent command named `"git
rev-parse --show-toplevel"`, leading to a failure in the GitHub Actions workflow.
Did we get this right? 👍 / 👎 to inform future reviews.
| const { exitCode } = await getExecOutput( | ||
| 'git --no-pager diff --quiet origin/master -- yarn.lock', | ||
| [], | ||
| { | ||
| failOnStdErr: false, | ||
| ignoreReturnCode: true | ||
| } | ||
| ); |
There was a problem hiding this comment.
Bug: getExecOutput is called with the full command string as the executable, instead of separating the command and its arguments.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The getExecOutput function is called with the entire command git --no-pager diff --quiet origin/master -- yarn.lock as a single string for the command parameter. The @actions/exec API expects the executable and its arguments to be separate parameters. This will cause the shell to attempt to execute a non-existent command named "git --no-pager diff --quiet origin/master -- yarn.lock", leading to a failure when checking lockfile changes.
💡 Suggested Fix
Pass 'git' as the first argument and ['--no-pager', 'diff', '--quiet', 'origin/master', '--', 'yarn.lock'] as the second argument to getExecOutput.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/actions/src/lockfiles.ts#L145-L152
Potential issue: The `getExecOutput` function is called with the entire command `git
--no-pager diff --quiet origin/master -- yarn.lock` as a single string for the `command`
parameter. The `@actions/exec` API expects the executable and its arguments to be
separate parameters. This will cause the shell to attempt to execute a non-existent
command named `"git --no-pager diff --quiet origin/master -- yarn.lock"`, leading to a
failure when checking lockfile changes.
Did we get this right? 👍 / 👎 to inform future reviews.
| }); | ||
| // focus all at once | ||
| const workspaces = tabsToBuild.map(each => `@sourceacademy/tab-${each}`); | ||
| const focusExitCode = await exec('yarn workspaces focus', workspaces, { silent: false }); |
There was a problem hiding this comment.
Bug: exec is called with the full command string as the executable, instead of separating the command and its arguments.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The exec function is called with the entire command yarn workspaces focus as a single string for the executable parameter. The @actions/exec API expects the executable and its arguments to be separate parameters. This will cause the shell to attempt to execute a non-existent command named "yarn workspaces focus", leading to a failure in the workspace focus command.
💡 Suggested Fix
Pass 'yarn' as the first argument and ['workspaces', 'focus', ...workspaces] as the second argument to exec.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/actions/src/load-artifacts/index.ts#L43
Potential issue: The `exec` function is called with the entire command `yarn workspaces
focus` as a single string for the `executable` parameter. The `@actions/exec` API
expects the executable and its arguments to be separate parameters. This will cause the
shell to attempt to execute a non-existent command named `"yarn workspaces focus"`,
leading to a failure in the workspace focus command.
Did we get this right? 👍 / 👎 to inform future reviews.
| const buildExitCode = await exec( | ||
| 'yarn workspaces foreach -pA', | ||
| [...workspaceBuildArgs, 'run', 'build'], |
There was a problem hiding this comment.
Bug: exec is called with the full command string as the executable, instead of separating the command and its arguments.
Severity: CRITICAL | Confidence: 1.00
🔍 Detailed Analysis
The exec function is called with the entire command yarn workspaces foreach -pA as a single string for the executable parameter. The @actions/exec API expects the executable and its arguments to be separate parameters. This will cause the shell to attempt to execute a non-existent command named "yarn workspaces foreach -pA", leading to a failure in the build command.
💡 Suggested Fix
Pass 'yarn' as the first argument and ['workspaces', 'foreach', '-pA', ...workspaceBuildArgs, 'run', 'build'] as the second argument to exec.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/actions/src/load-artifacts/index.ts#L50-L52
Potential issue: The `exec` function is called with the entire command `yarn workspaces
foreach -pA` as a single string for the `executable` parameter. The `@actions/exec` API
expects the executable and its arguments to be separate parameters. This will cause the
shell to attempt to execute a non-existent command named `"yarn workspaces foreach
-pA"`, leading to a failure in the build command.
Did we get this right? 👍 / 👎 to inform future reviews.
…es-fix # Conflicts: # .github/actions/src/gitRoot.ts # .github/actions/src/info/__tests__/index.test.ts # .github/actions/src/info/index.ts
|
Also no need to merge just yet, there are probably other things I need to add |
…to workspaces-fix # Conflicts: # .github/actions/src/info/index.ts # .github/actions/src/lockfiles/__tests__/lockfiles.test.ts # docs/src/repotools/7-workflows/1-actions.md # lib/buildtools/src/commands/__tests__/commandUtils.test.ts # lib/buildtools/src/commands/commandUtils.ts
|
There seems to be duplicate computation with the current workflow because when you build tabs, you need to build bundles. Could we just remove them? |
…es-fix # Conflicts: # .github/actions/src/lockfiles/utils.ts
…to workspaces-fix
…to workspaces-fix
… vitest setup files
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| var rx_code = /\n((```|~~~).*\n?([^]*?)\n?\2|(( {4}.*?\n)+))/g; | ||
| var rx_link = /((!?)\[(.*?)\]\((.*?)( ".*")?\)|\\([\\`*_{}\[\]()#+\-.!~]))/g; | ||
| var rx_table = /\n(( *\|.*?\| *\n)+)/g; | ||
| var rx_thead = /^.*\n( *\|( *\:?-+\:?-+\:? *\|)* *\n|)/; |
Check failure
Code scanning / CodeQL
Inefficient regular expression High documentation
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 9 days ago
The core of the problem is the ambiguity in how -+ can match runs of dashes, especially in combination with the alternation and overall repetition. To fix this, we need to rewrite the regular expression so that alternatives are not ambiguous. A well-known strategy is to ensure that each alternative inside a repeated group matches a non-overlapping subset of input. For Markdown table headers (which look like | --- | :---: |, etc.), we can make the regex stricter: match only sections that start with an optional colon, then dashes, then another optional colon (avoiding ambiguity over where the dashes start/finish), and that a table row always starts and ends with a pipe.
How to fix:
- In file
lib/repotools/src/build/docs/drawdown.ts, replace the existing definition ofrx_theadon line 23. - Modify the regex pattern by replacing the ambiguous
-+with an unambiguous construct that matches what is expected in Markdown: e.g.,[ \t]*\|[ \t]*:?-+:?:?[ \t]*\|[ \t]*. - For multiple cells, match groups of these pipes/cells, using
(?: ... )+. - The final regex can be
/^\s*\|(?:\s*\:?-+\:?\s*\|)+\s*\n/which matches a table header row that starts and ends with pipes, has cell separators with dash runs, colons, and can have multiple cells.
Only the regex definition on line 23 needs changing; no imports or method changes are needed.
| @@ -20,7 +20,7 @@ | ||
| var rx_code = /\n((```|~~~).*\n?([^]*?)\n?\2|(( {4}.*?\n)+))/g; | ||
| var rx_link = /((!?)\[(.*?)\]\((.*?)( ".*")?\)|\\([\\`*_{}\[\]()#+\-.!~]))/g; | ||
| var rx_table = /\n(( *\|.*?\| *\n)+)/g; | ||
| var rx_thead = /^.*\n( *\|( *\:?-+\:?-+\:? *\|)* *\n|)/; | ||
| var rx_thead = /^\s*\|(?:\s*\:?-+\:?\s*\|)+\s*\n/; | ||
| var rx_row = /.*\n/g; | ||
| var rx_cell = /\||(.*?[^\\])\|/g; | ||
| var rx_heading = /(?=^|>|\n)([>\s]*?)(#{1,6}) (.*?)( #*)? *(?=\n|$)/g; |
@RichDom2185 provided some code that would help detect which workspaces need to be rebuilt based on diffs across lockfiles. This PR will implement that change, as well as fix some typos in the documentation.