fix: reflect repository maintenance findings - #834
Conversation
📝 WalkthroughWalkthroughAdds a repository state guard (archived/private detection) to Changesrepo-maintenance enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/claude-workflow-contract.test.jsOops! Something went wrong! :( ESLint: 10.5.0 ReferenceError: describe is not defined test/template-workflows.test.jsOops! Something went wrong! :( ESLint: 10.5.0 ReferenceError: describe is not defined 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7f8fe8388
ℹ️ 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".
| PEER_ISSUES+=("npm lockfile solver が失敗（.context/npm-peer-compat.log を確認）") | ||
| fi | ||
| elif [ -f "pnpm-lock.yaml" ] && command -v pnpm >/dev/null 2>&1; then | ||
| if ! pnpm install --lockfile-only --ignore-scripts >.context/pnpm-peer-compat.log 2>&1; then |
There was a problem hiding this comment.
When this command runs in a pnpm repo with package.json changes or a stale lockfile, this branch mutates pnpm-lock.yaml during a health check: pnpm install --help documents --lockfile-only as "Only pnpm-lock.yaml is updated". That makes check-only maintenance dirty the worktree instead of just writing .context/pnpm-peer-compat.log; use a real dry-run/frozen validation for pnpm.
Useful? React with 👍 / 👎.
| if ! echo "$DEPENDABOT_WF" | grep -Fq "if: github.actor == 'dependabot[bot]'"; then | ||
| DEPENDABOT_AUTOMERGE_ISSUES+=("job-level actor gate が未設定（Dependabot 確認前に write token が発行される）") | ||
| fi | ||
| if ! echo "$DEPENDABOT_WF" | grep -q "contents: read"; then | ||
| DEPENDABOT_AUTOMERGE_ISSUES+=("workflow-level permissions が read-only ではない") |
There was a problem hiding this comment.
When an older workflow has the Dependabot actor condition only on steps, or has contents: read in a comment/job while top-level permissions still grant writes, these file-wide grep checks pass even though the job still gets a write-scoped token before the Dependabot actor gate. Since full mode only rewrites when DEPENDABOT_AUTOMERGE_ISSUES is nonempty, the unsafe workflow is skipped; parse/check YAML locations such as the job if and top-level permissions instead of substring presence.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/template-workflows.test.js (1)
315-317: ⚡ Quick winMatch actual label entries instead of raw substrings.
These assertions can pass if the labels are only present in comments and can fail for valid YAML using double quotes or no quotes. Anchor the check to a
- name:entry while keeping the test dependency-free.♻️ Proposed refactor
- expect(labels).toContain("name: 'dependabot-minor'"); - expect(labels).toContain("name: 'needs-review'"); - expect(labels).toContain("name: 'breaking-change'"); + const requiredLabelNames = ['dependabot-minor', 'needs-review', 'breaking-change']; + for (const labelName of requiredLabelNames) { + expect(labels).toMatch( + new RegExp(`^\\s*-\\s*name:\\s*['"]?${labelName}['"]?\\s*$`, 'm'), + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/template-workflows.test.js` around lines 315 - 317, The test assertions for the labels in the expect statements (checking for 'dependabot-minor', 'needs-review', and 'breaking-change') are matching raw substrings which can pass even if labels appear in comments and fail for valid YAML with different quote styles. Update each of the three expect(labels).toContain() calls to match the actual YAML list entry format by anchoring to the `- name:` prefix and using a pattern that accommodates single quotes, double quotes, or no quotes (for example, check for patterns like `- name:\s*['"]?dependabot-minor['"]?` using a regular expression test rather than a simple substring match). This ensures you are validating proper YAML label entries while remaining dependency-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@script/dependency-health-check.sh`:
- Around line 123-135: The issue is that the code prints a success message "No
peer dependency issues" when PEER_ISSUES is 0, even when the peer dependency
check was actually skipped due to missing node_modules. Restructure the
conditional logic by checking PEER_CHECK_SKIPPED first before printing any
status message. If PEER_CHECK_SKIPPED is true, print the skipped notice
immediately. Only if the check was not skipped should you proceed with the
existing if-else block that checks PEER_ISSUES and prints either issues or the
success message. This ensures the output accurately reflects the actual state of
the check.
---
Nitpick comments:
In `@test/template-workflows.test.js`:
- Around line 315-317: The test assertions for the labels in the expect
statements (checking for 'dependabot-minor', 'needs-review', and
'breaking-change') are matching raw substrings which can pass even if labels
appear in comments and fail for valid YAML with different quote styles. Update
each of the three expect(labels).toContain() calls to match the actual YAML list
entry format by anchoring to the `- name:` prefix and using a pattern that
accommodates single quotes, double quotes, or no quotes (for example, check for
patterns like `- name:\s*['"]?dependabot-minor['"]?` using a regular expression
test rather than a simple substring match). This ensures you are validating
proper YAML label entries while remaining dependency-free.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee423040-d277-4bd7-a4b7-fee957e0fb05
📒 Files selected for processing (4)
.claude/commands/repo-maintenance.mdscript/dependency-health-check.shtest/claude-workflow-contract.test.jstest/template-workflows.test.js
| if [ "$PEER_ISSUES" -gt 0 ]; then | ||
| echo -e " ${YELLOW}⚠ $PEER_ISSUES peer dependency compatibility issues${NC}" | ||
| printf '%s\n' "$PEER_PROBLEMS" | sed '/^$/d' | head -5 | sed 's/^/ - /' | ||
| if [ "$PEER_ISSUES" -gt 5 ]; then | ||
| echo " ... and $((PEER_ISSUES - 5)) more" | ||
| fi | ||
| else | ||
| echo -e " ${GREEN}✓ No peer dependency issues${NC}" | ||
| fi | ||
|
|
||
| if [ "$PEER_CHECK_SKIPPED" = true ]; then | ||
| echo " • Peer dependency check skipped (node_modules not installed)" | ||
| fi |
There was a problem hiding this comment.
Do not report “No peer dependency issues” when the check was skipped.
When node_modules is absent, Line 74 sets PEER_CHECK_SKIPPED=true, but Lines 129-130 still print a green no-issues result before the skipped notice. Put the skipped branch first so the human output matches the actual check state.
🔧 Proposed fix
- if [ "$PEER_ISSUES" -gt 0 ]; then
+ if [ "$PEER_CHECK_SKIPPED" = true ]; then
+ echo " • Peer dependency check skipped (node_modules not installed)"
+ elif [ "$PEER_ISSUES" -gt 0 ]; then
echo -e " ${YELLOW}⚠ $PEER_ISSUES peer dependency compatibility issues${NC}"
printf '%s\n' "$PEER_PROBLEMS" | sed '/^$/d' | head -5 | sed 's/^/ - /'
if [ "$PEER_ISSUES" -gt 5 ]; then
echo " ... and $((PEER_ISSUES - 5)) more"
fi
else
echo -e " ${GREEN}✓ No peer dependency issues${NC}"
fi
-
- if [ "$PEER_CHECK_SKIPPED" = true ]; then
- echo " • Peer dependency check skipped (node_modules not installed)"
- fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ "$PEER_ISSUES" -gt 0 ]; then | |
| echo -e " ${YELLOW}⚠ $PEER_ISSUES peer dependency compatibility issues${NC}" | |
| printf '%s\n' "$PEER_PROBLEMS" | sed '/^$/d' | head -5 | sed 's/^/ - /' | |
| if [ "$PEER_ISSUES" -gt 5 ]; then | |
| echo " ... and $((PEER_ISSUES - 5)) more" | |
| fi | |
| else | |
| echo -e " ${GREEN}✓ No peer dependency issues${NC}" | |
| fi | |
| if [ "$PEER_CHECK_SKIPPED" = true ]; then | |
| echo " • Peer dependency check skipped (node_modules not installed)" | |
| fi | |
| if [ "$PEER_CHECK_SKIPPED" = true ]; then | |
| echo " • Peer dependency check skipped (node_modules not installed)" | |
| elif [ "$PEER_ISSUES" -gt 0 ]; then | |
| echo -e " ${YELLOW}⚠ $PEER_ISSUES peer dependency compatibility issues${NC}" | |
| printf '%s\n' "$PEER_PROBLEMS" | sed '/^$/d' | head -5 | sed 's/^/ - /' | |
| if [ "$PEER_ISSUES" -gt 5 ]; then | |
| echo " ... and $((PEER_ISSUES - 5)) more" | |
| fi | |
| else | |
| echo -e " ${GREEN}✓ No peer dependency issues${NC}" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@script/dependency-health-check.sh` around lines 123 - 135, The issue is that
the code prints a success message "No peer dependency issues" when PEER_ISSUES
is 0, even when the peer dependency check was actually skipped due to missing
node_modules. Restructure the conditional logic by checking PEER_CHECK_SKIPPED
first before printing any status message. If PEER_CHECK_SKIPPED is true, print
the skipped notice immediately. Only if the check was not skipped should you
proceed with the existing if-else block that checks PEER_ISSUES and prints
either issues or the success message. This ensures the output accurately
reflects the actual state of the check.
|
🎉 This PR is included in version 1.116.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
1 participant
Summary
Verification
Summary by CodeRabbit
Release Notes
New Features
Improvements
Tests