feat: Migrate ci-design-md-check.sh to JavaScript#625
Conversation
|
Warning Review limit reached
More reviews will be available in 57 minutes and 43 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: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
github-actions
Bot
added
area:tests
🔍 Reviewer Summary for PR #625CI Status: ✅ Recommendations
|
github-actions
Bot
added
status:needs-review
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a CI check script (ciDesignMdCheck.js) and its corresponding test suite to validate and lint DESIGN.md files, generating a markdown summary for PR comments. The feedback highlights a critical logic error where linting failures (non-zero exit codes) are caught and handled in a way that falsely passes the CI check with exit code 0. Additionally, there are unused constants and variables in the script, and the main orchestration function ciDesignMdCheck lacks unit test coverage.
| try { | ||
| const lintCmd = `cd "${cliCwd}" && ${cliCmd} lint "${designFile}" --format json`; | ||
| const output = execSync(lintCmd, { encoding: "utf8", stdio: "pipe" }); | ||
| fs.writeFileSync(jsonFile, output); | ||
| return true; | ||
| } catch (error) { | ||
| try { | ||
| const stderr = error.stderr || error.stdout || ""; | ||
| if (stderr) { | ||
| fs.writeFileSync(jsonFile, stderr); | ||
| } | ||
| } catch { | ||
| // Ignore write errors | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
There is a critical logic error in how command execution failures are handled.
When the lint command finds errors, it exits with a non-zero status code. This causes execSync to throw an error. In the catch block, runLintWithAvailableCli writes to the JSON file and returns false.
However, in ciDesignMdCheck, if runLintWithAvailableCli returns false, the script assumes no CLI was found, prints a warning, and exits with 0 (success):
if (!runLintWithAvailableCli(designFile, jsonFile)) {
console.error(
"No runnable DESIGN.md CLI was found after validation. Report written to",
reportFile,
);
process.exit(0);
}This means any lint errors will cause the CI check to falsely pass with exit code 0, completely defeating the purpose of the lint check!
Additionally, using error.stderr || error.stdout will prefer stderr (which often contains non-JSON warnings or logs in CI) over stdout (which contains the actual JSON report), causing JSON parsing to fail.
We should check if error.stdout contains the report, write it, and return true to indicate the CLI successfully ran and produced a report.
| try { | |
| const lintCmd = `cd "${cliCwd}" && ${cliCmd} lint "${designFile}" --format json`; | |
| const output = execSync(lintCmd, { encoding: "utf8", stdio: "pipe" }); | |
| fs.writeFileSync(jsonFile, output); | |
| return true; | |
| } catch (error) { | |
| try { | |
| const stderr = error.stderr || error.stdout || ""; | |
| if (stderr) { | |
| fs.writeFileSync(jsonFile, stderr); | |
| } | |
| } catch { | |
| // Ignore write errors | |
| } | |
| return false; | |
| } | |
| try { | |
| const lintCmd = `cd "${cliCwd}" && ${cliCmd} lint "${designFile}" --format json`; | |
| const output = execSync(lintCmd, { encoding: "utf8", stdio: "pipe" }); | |
| fs.writeFileSync(jsonFile, output); | |
| return true; | |
| } catch (error) { | |
| if (error.stdout) { | |
| try { | |
| fs.writeFileSync(jsonFile, error.stdout); | |
| return true; | |
| } catch { | |
| // Ignore write errors | |
| } | |
| } | |
| return false; | |
| } |
| function runLintWithAvailableCli(designFile, jsonFile) { | ||
| const designDir = path.dirname(path.resolve(designFile)); | ||
| const { cmd: cliCmd, cwd: cliCwd = "." } = findDesignMdCliCmd(null); |
There was a problem hiding this comment.
The variable designDir is declared but never used in runLintWithAvailableCli.
| function runLintWithAvailableCli(designFile, jsonFile) { | |
| const designDir = path.dirname(path.resolve(designFile)); | |
| const { cmd: cliCmd, cwd: cliCwd = "." } = findDesignMdCliCmd(null); | |
| function runLintWithAvailableCli(designFile, jsonFile) { | |
| const { cmd: cliCmd, cwd: cliCwd = "." } = findDesignMdCliCmd(null); |
| const DEFAULT_DESIGN_FILE = | ||
| process.env.DESIGN_MD_FILE || path.join(process.cwd(), "DESIGN.md"); | ||
| const DEFAULT_REPORT = | ||
| process.env.DESIGN_MD_REPORT || | ||
| path.join(process.cwd(), "design-md-validation-report.md"); | ||
| const DEFAULT_JSON_FILE = | ||
| process.env.DESIGN_MD_JSON_REPORT || | ||
| path.join(process.cwd(), "designmd-lint.json"); | ||
| const DEFAULT_COMMENT_FILE = | ||
| process.env.DESIGN_MD_PR_COMMENT || | ||
| path.join(process.cwd(), "design-md-pr-comment.md"); |
There was a problem hiding this comment.
The default constants DEFAULT_DESIGN_FILE, DEFAULT_REPORT, DEFAULT_JSON_FILE, and DEFAULT_COMMENT_FILE are defined at the top of the file but are never used. The ciDesignMdCheck function re-defines these paths locally using repoRoot. These unused constants should be removed to keep the code clean and maintainable.
| const { | ||
| ciDesignMdCheck, | ||
| generatePrComment, | ||
| parseJsonReport, | ||
| } = require("../ciDesignMdCheck"); |
There was a problem hiding this comment.
The main function ciDesignMdCheck is imported but never actually tested in this test suite. This leaves the core CLI orchestration logic completely uncovered by unit tests.
Consider adding tests for ciDesignMdCheck by mocking process.exit, console.log/console.error, and the dependency functions (validateDesignMd, runLintWithAvailableCli) to ensure the main execution flow works as expected under different scenarios.
const {
generatePrComment,
parseJsonReport,
} = require("../ciDesignMdCheck");
🔍 Reviewer Summary for PR #625CI Status: ✅ Recommendations
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
- Create ciDesignMdCheck.js module that depends on validateDesignMd - Implement all functionality from ci-design-md-check.sh: - Run design.md CLI lint command in JSON format - Parse JSON report for errors, warnings, info counts - Generate PR comment markdown with top 10 findings - Exit with error code if lint errors found - Add comprehensive Jest test suite (11 tests) - Tests cover: JSON parsing, comment generation, empty findings, limits, severity levels Depends on: #611 (validateDesignMd migration) Issue: #612 https://claude.ai/code/session_01A1BeR5Rc2vNYaMQtgw6ERd
- Fixed critical logic error where lint failures exited with success (0) instead of error (1) - Changed runLintWithAvailableCli to check error.stdout for JSON report, not error.stderr - Removed unused DEFAULT_* constants and designDir variable - Removed unused variable from runLintWithAvailableCli - Added comprehensive test coverage for runLintWithAvailableCli function - Added full test coverage for ciDesignMdCheck orchestration function - All 19 tests now passing with proper mocking of dependencies This ensures lint failures are properly detected and reported as CI failures. https://claude.ai/code/session_01A1BeR5Rc2vNYaMQtgw6ERd
ashleyshaw
force-pushed
the
feature/migrate-ci-design-md-check
branch
from
4c39ed7 to
0370f45
Compare
2 participants
Summary
Migrated
ci-design-md-check.sh(Bash) to a JavaScript module with comprehensive Jest test coverage. This is the second step in removing all shell scripts.Changes
New Module:
scripts/design-md-agent/ciDesignMdCheck.jsvalidateDesignMdmodule from previous migrationTest Suite:
scripts/design-md-agent/__tests__/ciDesignMdCheck.test.jsFeatures Migrated
Test Results
Related Issues
Closes #612 - Part of #616 (Epic: Migrate All Bash Scripts to JavaScript)
Dependencies
Test Plan
Generated by Claude Code