fix(dev-lead): configure git identity, surface commit failures, add WebFetch tool access#282
Conversation
…ebFetch tool access
Fixes three issues diagnosed in run #26071797185 where the agent's fixes
were silently lost with a false "Changes committed and pushed" comment:
1. Configure git identity in dev-lead.yml before any writer steps run.
GitHub Actions runners have no default git user.email/name, causing
`git commit` to fail with "fatal: empty ident name not allowed".
2. Fix commit_and_push to surface git commit failures. The function was
called from an `if` statement condition, suspending set -e for the
entire body. A failing `git commit` was silently ignored — execution
continued to `git push` (which exits 0 with "Everything up-to-date"),
the function returned 0, and a false "applied" marker was posted. Now
uses `|| { ...; exit 1; }` so any git commit failure hard-exits CI.
3. Add WebFetch to run_writer allowed tools so the agent can look up
SHA256 hashes, SonarCloud rules, and other external resources without
resorting to curl guesswork.
Adds two regression tests verifying commit failure exits non-zero and
never posts a false "status=applied" marker.
Closes #281
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR fixes three related issues preventing the dev-lead agent from reliably committing work. It configures git identity in the workflow, adds explicit error handling to the commit step to prevent silent failures, tests both failure modes, and expands Claude's write-mode tool access to include WebFetch for external verification. ChangesDev-lead workflow and commit reliability fixes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Dev-Lead — human-pr (no-changes)No changes were needed for this PR. |
There was a problem hiding this comment.
Pull request overview
Fixes a silent failure mode in the dev-lead workflow where missing git identity caused git commit to fail, but the failure was masked by set -e suspension when commit_and_push was invoked as an if condition — resulting in falsely posted "Changes committed and pushed" comments while agent fixes were dropped.
Changes:
- Configure global git identity (
donpetry-bot) indev-lead.ymlbefore writer steps. - Make
git commitfailure incommit_and_pushexplicitlyexit 1, bypassing theset -esuspension caveat. - Add
WebFetchto therun_writerallowed tools list inengine.sh. - Add two regression bats tests asserting non-zero exit and no false
status=appliedmarker on commit failure.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/dev-lead.yml |
Adds a step to set global git user.email/user.name before writer steps run. |
scripts/dev-lead-fix-reviews.sh |
Hard-fails (exit 1) on git commit failure with an error annotation. |
scripts/engine.sh |
Includes WebFetch in the writer's --allowed-tools list. |
tests/dev-lead/unit/test_fix_reviews.bats |
Adds two regression tests covering the commit-failure path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Dev-Lead — fix-bot-comment (no-changes)Engine ran but made no changes. |
1 similar comment
Dev-Lead — fix-bot-comment (no-changes)Engine ran but made no changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/dev-lead-fix-reviews.sh`:
- Around line 129-130: The conditional call to commit_and_push can silently
succeed even if the internal git push fails (because set -e doesn't stop
failures inside an if), so ensure failures are propagated: modify the caller or
the commit_and_push function to check git push's exit status and return non-zero
on error (e.g., capture the git push exit code and return it, or explicitly test
the call at the callsite with if ! commit_and_push; then return 1; fi) so that a
failed push cannot fall through and cause status=applied to be set.
In `@tests/dev-lead/unit/test_fix_reviews.bats`:
- Around line 477-483: After invoking the script with run bash
"$FIX_REVIEWS_SCRIPT", assert the command returned a non-zero exit status to
ensure the test exercised the commit-failure path; check the Bats-provided
status variable (e.g., assert [ "$status" -ne 0 ] or similar) before verifying
"$comment_file" contents so the test fails if the script unexpectedly succeeds.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7f80a95-1fc6-495c-8d81-952acc07d2ea
📒 Files selected for processing (4)
.github/workflows/dev-lead.ymlscripts/dev-lead-fix-reviews.shscripts/engine.shtests/dev-lead/unit/test_fix_reviews.bats
|
No description provided. |
Dev-Lead — fix-reviews (no-changes)No changes were needed for the open review threads. |
…ertion
Per CodeRabbit review:
- `git push` had the same silent-swallow risk as `git commit` when called
from an if-condition context; add `|| { exit 1; }` so push failures are
surfaced the same way as commit failures
- Test 17 now asserts non-zero exit status before checking the comment
file, so it fails correctly if the script unexpectedly succeeds
Dev-Lead — fix-bot-comment (no-changes)Engine ran but made no changes. |
Dev-Lead — human-pr (no-changes)No changes were needed for this PR. |
|
Dev-Lead — fix-bot-comment (no-changes)Engine ran but made no changes. |
![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 explicit error handling for git commit in scripts/dev-lead-fix-reviews.sh to prevent silent failures when set -e is suspended, and adds WebFetch to the allowed tools for Claude in scripts/engine.sh. Additionally, new unit tests are included to verify the robustness of the commit process. Feedback suggests extending this error handling to the git push command and refactoring the new tests to reduce code duplication.
| git commit -m "$commit_msg" || { echo "::error::git commit failed — check git identity configuration on the runner" >&2; exit 1; } | ||
| fi | ||
| git push | ||
| git push || { |
There was a problem hiding this comment.
The commit_and_push function is called from an if condition, which suspends set -e. While you've added error handling for git commit, the git push command on this line lacks similar protection. If git push fails (e.g., due to branch protection rules or network issues), the failure will be silently ignored, and the function will still return a success code. This could lead to the script falsely reporting that changes were pushed.
To ensure robustness, you should add explicit error handling for git push.
| git push || { | |
| git push || { echo "::error::git push failed" >&2; exit 1; } |
References
- Keep error routing logic inline to avoid obscuring intent or introducing false positives.
|
|
||
| # ── commit_and_push failure tests ───────────────────────────────────────────── | ||
|
|
||
| @test "fix-reviews: commit_and_push: git commit failure exits 1 (not silently swallowed)" { |
There was a problem hiding this comment.
There is significant code duplication between the two new tests. Much of the setup code (environment variables, temporary git repository creation, claude stub) is identical in @test "fix-reviews: commit_and_push: git commit failure exits 1 (not silently swallowed)" and @test "fix-reviews: commit_and_push: no false 'applied' marker posted on git commit failure".
To improve maintainability and readability, consider refactoring the common setup logic into a helper function. This would make the tests more concise and easier to manage in the future.
Summary
Fixes three issues diagnosed in run #26071797185 where the dev-lead agent's fixes were silently lost while falsely reporting "Changes committed and pushed."
dev-lead.ymlbefore writer steps — runners have no defaultuser.email/user.name, causing allgit commitcalls to failgit commitfailures incommit_and_push— the function was called from anifcondition, suspendingset -eand silently swallowing the failure; now uses|| { exit 1; }to hard-fail CI visiblyWebFetchtorun_writerallowed tools — agent can now look up SHA256 hashes, SonarCloud rule docs, etc. withoutcurlguessworkRoot cause
commit_and_pushwas called as the condition of anifstatement. In bash withset -euo pipefail,set -eis suspended for the entire function body in that position. So whengit commitfailed with "fatal: empty ident name not allowed", execution continued togit push(exited 0 with "Everything up-to-date"), the function returned 0, andpost_reviews_terminal ... status=appliedfired — posting a false "Changes committed and pushed" comment while all agent work was dropped.Test plan
bats tests/dev-lead/unit/test_fix_reviews.bats— two new regression tests added (tests 16 & 17): verifycommit_and_pushfailure exits non-zero and never posts a falsestatus=appliedmarkerCloses #281
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests