Uh oh!
There was an error while loading. Please reload this page.
chore(workflows): migrate release/labeling bash logic to Node scripts - #456
Conversation
Warning Review limit reached
More reviews will be available in 52 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:2284245254
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: | ||
| GITHUB_EVENT_NAME: ${{ github.event_name }} | ||
| GITHUB_ACTOR: ${{ github.actor }} | ||
| run: node scripts/workflows/release/trigger-telemetry.cjs |
There was a problem hiding this comment.
In the trigger-telemetry job I inspected, there is no actions/checkout step before this run, so a fresh GitHub Actions runner will not have scripts/workflows/release/trigger-telemetry.cjs in the workspace. The previous inline bash did not need repository files, but this migration makes the first job fail with Cannot find module, which prevents the downstream lint and release jobs from running.
Useful? React with 👍 / 👎.
| return execGit( | ||
| `git log ${rangeStart}..develop --first-parent --pretty=format:"- %h %s"`, | ||
| ); |
There was a problem hiding this comment.
When a dry-run release is dispatched with a notes_from value, that input is interpolated directly into an execSync shell command here. Values such as v1.0.0; <command> are therefore executed by the shell in the release job context, whereas the old bash path quoted the range; use execFileSync/spawnSync with argument arrays or otherwise validate the ref before invoking git.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request transitions custom workflow logic from Bash to Node.js scripts, introducing a workflow validator with guardrails to enforce the new policy, along with several release-related Node scripts, a shared runtime helper, and updated documentation. The review feedback highlights several critical improvements: addressing a potential command injection vulnerability in build-notes-preview.cjs by replacing execSync with execFileSync, ensuring temporary directories in tests are properly cleaned up, broadening the workflow validator's regex to catch all if statements, and hardening the shared runtime helper to support multiline outputs and preserve error stack traces for easier debugging.
| const { execSync } = require("child_process"); | ||
| const { readEnv, log, runMain } = require("../shared/runtime.cjs"); | ||
| function execGit(command, allowError = false) { | ||
| try { | ||
| return execSync(command, { | ||
| encoding: "utf8", | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| }).trim(); | ||
| } catch (error) { | ||
| if (allowError) { | ||
| return ""; | ||
| } | ||
| throw new Error(`Git command failed: ${command}\n${error.message}`); | ||
| } | ||
| } | ||
| function buildGitLogContent(rangeStart) { | ||
| if (rangeStart) { | ||
| return execGit( | ||
| `git log ${rangeStart}..develop --first-parent --pretty=format:"- %h %s"`, | ||
| ); | ||
| } | ||
| return execGit('git log develop --first-parent --pretty=format:"- %h %s"'); | ||
| } | ||
| async function main() { | ||
| let rangeStart = readEnv("INPUT_NOTES_FROM", { defaultValue: "" }).trim(); | ||
| if (!rangeStart) { | ||
| rangeStart = execGit("git describe --tags --abbrev=0", true); | ||
| } |
There was a problem hiding this comment.
Using execSync with a shell-interpolated string containing rangeStart (which is sourced from the user-controlled INPUT_NOTES_FROM environment variable) introduces a Command Injection vulnerability. To eliminate this risk, use execFileSync to execute git directly with an array of arguments, bypassing the shell entirely.
const { execFileSync } = require("child_process");
const { readEnv, log, runMain } = require("../shared/runtime.cjs");
function execGit(args, allowError = false) {
try {
return execFileSync("git", args, {
encoding: "utf8",
stdio: ["ignore", "pipe", "pipe"],
}).trim();
} catch (error) {
if (allowError) {
return "";
}
throw new Error("Git command failed: git " + args.join(" ") + "\n" + error.message);
}
}
function buildGitLogContent(rangeStart) {
if (rangeStart) {
return execGit([
"log",
rangeStart + "..develop",
"--first-parent",
"--pretty=format:- %h %s",
]);
}
return execGit([
"log",
"develop",
"--first-parent",
"--pretty=format:- %h %s",
]);
}
async function main() {
let rangeStart = readEnv("INPUT_NOTES_FROM", { defaultValue: "" }).trim();
if (!rangeStart) {
rangeStart = execGit(["describe", "--tags", "--abbrev=0"], true);
}
| test("trigger-telemetry writes expected GITHUB_OUTPUT and telemetry payload", () => { | ||
| const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "wf-telemetry-")); | ||
| const outputPath = path.join(tempDir, "github_output.txt"); | ||
| execFileSync(process.execPath, [runtimeScript], { | ||
| cwd: tempDir, | ||
| env: { | ||
| ...process.env, | ||
| GITHUB_OUTPUT: outputPath, | ||
| GITHUB_EVENT_NAME: "workflow_dispatch", | ||
| GITHUB_ACTOR: "ash", | ||
| }, | ||
| encoding: "utf8", | ||
| }); | ||
| const outputContent = fs.readFileSync(outputPath, "utf8"); | ||
| const telemetry = JSON.parse( | ||
| fs.readFileSync(path.join(tempDir, "trigger-telemetry.json"), "utf8"), | ||
| ); | ||
| expect(outputContent).toContain("unauthorized_attempts=0"); | ||
| expect(telemetry).toEqual({ | ||
| event: "workflow_dispatch", | ||
| actor: "ash", | ||
| unauthorized_attempts: 0, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test creates a temporary directory using fs.mkdtempSync but never cleans it up. This leaves leftover files and directories on the host machine or CI runner. Wrap the test logic in a try...finally block to ensure the temporary directory is recursively deleted using fs.rmSync after the test completes. (Note: The same cleanup pattern should be applied to the other tests in this file that use fs.mkdtempSync.)
test("trigger-telemetry writes expected GITHUB_OUTPUT and telemetry payload",()=>{consttempDir=fs.mkdtempSync(path.join(os.tmpdir(),"wf-telemetry-"));try{constoutputPath=path.join(tempDir,"github_output.txt");execFileSync(process.execPath,[runtimeScript],{cwd: tempDir,env: {
...process.env,GITHUB_OUTPUT: outputPath,GITHUB_EVENT_NAME: "workflow_dispatch",GITHUB_ACTOR: "ash",},encoding: "utf8",});constoutputContent=fs.readFileSync(outputPath,"utf8");consttelemetry=JSON.parse(fs.readFileSync(path.join(tempDir,"trigger-telemetry.json"),"utf8"),);expect(outputContent).toContain("unauthorized_attempts=0");expect(telemetry).toEqual({event: "workflow_dispatch",actor: "ash",unauthorized_attempts: 0,});}finally{fs.rmSync(tempDir,{recursive: true,force: true});}});| const hasControlFlow = | ||
| /(^|\n)\s*(if\s+\[|if\s+\[\[|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | ||
| runScript, | ||
| ); |
There was a problem hiding this comment.
The regex if\s+\[|if\s+\[\[ only matches if statements that use brackets (e.g., if [ ... or if [[ ...). However, bash if statements can execute any command directly without brackets (e.g., if git diff-index ...; then). Simplify the if match to if\s+ to catch all if control flows.
| consthasControlFlow= | |
| /(^|\n)\s*(if\s+\[|if\s+\[\[|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | |
| runScript, | |
| ); | |
| consthasControlFlow= | |
| /(^|\n)\s*(if\s+|case\s+|for\s+|while\s+|until\s+|select\s+)/m.test( | |
| runScript, | |
| ); |
| function writeGithubOutput(key, value, outputPath = process.env.GITHUB_OUTPUT) { | ||
| if (!outputPath) { | ||
| throw new Error("GITHUB_OUTPUT is not set"); | ||
| } | ||
| fs.appendFileSync(outputPath, `${key}=${value}\n`, "utf8"); | ||
| } |
There was a problem hiding this comment.
writeGithubOutput writes key-value pairs directly as key=value. If value contains newlines, GitHub Actions will fail to parse it correctly or throw an error. Implement the standard GitHub Actions multiline output syntax using a random delimiter when the value contains newlines.
function writeGithubOutput(key, value, outputPath = process.env.GITHUB_OUTPUT) {
if (!outputPath) {
throw new Error("GITHUB_OUTPUT is not set");
}
const strValue = String(value);
if (strValue.includes("\n")) {
const delimiter = "EOF_" + Math.random().toString(36).substring(2);
fs.appendFileSync(outputPath, key + "<<" + delimiter + "\n" + strValue + "\n" + delimiter + "\n", "utf8");
} else {
fs.appendFileSync(outputPath, key + "=" + strValue + "\n", "utf8");
}
}
| async function runMain(mainFn) { | ||
| try { | ||
| await mainFn(); | ||
| } catch (error) { | ||
| console.error(error.message); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
runMain only logs error.message when an exception is caught. This hides the stack trace, making it very difficult to debug unexpected runtime errors (like TypeError or ReferenceError) in CI workflows. Log error.stack or the full error object to provide complete debugging context.
async function runMain(mainFn) {
try {
await mainFn();
} catch (error) {
console.error(error.stack || error);
process.exit(1);
}
}
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
🔍 Reviewer Summary for PR #456CI Status: ✅ Recommendations
|
Uh oh!
There was an error while loading. Please reload this page.
See commit 2284245 for full change-set including workflow migration and included project archival updates.