fix(repos): log the cause when repo create fails#103
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesRepository init error logging
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes
🚥 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 |
![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)
crates/gitlawb-node/src/api/repos.rs (1)
463-473: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNow-redundant handler logging will double-log 5xx failures.
IntoResponsewas updated totracing::error!on every server-error response, but these handlers (git_info_refs,git_receive_pack, and thegit_upload_packfallback at 533-542) still emit their owntracing::error!before returningAppError::git(e). Each git failure will now produce two error lines. These local logs carry richer context (repo,service), so consider keeping them and downgrading/removing the generic log inIntoResponse, or dropping the local logs — but not both.Also applies to: 651-668
🤖 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 `@crates/gitlawb-node/src/api/repos.rs` around lines 463 - 473, The handler-level tracing in git_info_refs, git_receive_pack, and the git_upload_pack fallback is now duplicating the new server-error logging from IntoResponse, so each git 5xx failure is logged twice. Decide on a single logging layer: either keep the repo/service-specific tracing::error! calls in these handlers and remove or downgrade the generic error log in IntoResponse, or remove the local error logs and rely on IntoResponse, but avoid logging the same AppError::git(e) path in both places.
🤖 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 `@crates/gitlawb-node/src/api/changelog.rs`:
- Line 44: Update AppError::git so client-facing error strings no longer use
expanded formatting from format!("{e:#}") and instead return the plain
user-friendly message via e.to_string() or format!("{e}"). Keep the response
body generic in the changelog API path that uses map_err(AppError::git), and if
full error details are needed, emit them only through server-side
tracing::error! while preserving AppError::git as the single place controlling
this formatting.
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 463-473: The handler-level tracing in git_info_refs,
git_receive_pack, and the git_upload_pack fallback is now duplicating the new
server-error logging from IntoResponse, so each git 5xx failure is logged twice.
Decide on a single logging layer: either keep the repo/service-specific
tracing::error! calls in these handlers and remove or downgrade the generic
error log in IntoResponse, or remove the local error logs and rely on
IntoResponse, but avoid logging the same AppError::git(e) path in both places.
🪄 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: 5c1ce05a-5770-489b-ae5e-8cd0d84cf390
📒 Files selected for processing (6)
crates/gitlawb-node/src/api/changelog.rscrates/gitlawb-node/src/api/encrypted.rscrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/error.rs
create_repo was the one git handler that mapped its error without logging
it (info_refs, receive_pack, acquire all log with repo context). So a
failed init_bare returned an opaque 500 "initializing bare repo" with no
server-side trail to the real cause.
Log the full anyhow chain ({:#}, so the leaf git/filesystem error is kept)
with owner+repo context, matching the other handlers. Response unchanged.
mizuki0x
force-pushed
the
fix/surface-git-error-cause
branch
from
4e7da64 to
2f71682
Compare
mizuki0x
changed the title
|
Both of coderabbitai's points are valid and I've addressed them by narrowing this down. Putting the full chain in the response body would leak internal detail, and the central IntoResponse log double-logged the handlers that already log with repo/service context. Dropped the helper, the response change, and the central log entirely. The PR now just adds a single |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Approving. Verified the diff against the sibling handlers and it does what the description says: create_repo was the only git-backed handler that mapped repo_store.init() to AppError::Git without logging the cause, and this brings it to parity. The log is server-side only and the client response is unchanged (still AppError::Git(e.to_string())), so nothing new is exposed to the caller. req.name is charset-restricted upstream and owner_did is the authenticated DID, so there's no log-injection or move/borrow issue in the closure, and init() returns anyhow::Result, so {:#} walks to the leaf cause as intended.
One non-blocking note, no change needed: this logs the full chain via format!("{e:#}") where the other git handlers (repos.rs:504/511/692/706) use plain %e. That's the better choice for an init failure; flagging only so the divergence from the siblings is deliberate.
CI is green on the current head and the earlier review points were addressed by narrowing the change.
@kevincodex1 LGTM
beardthelion
added
crate:node
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
None. This is a clean, low-risk logging improvement that aligns create_repo with the rest of the handler error handling.
@kevincodex1 LGTM
3 participants
What
create_repowas the one git-backed handler that mapped its error without logging it.info_refs,receive_pack, andacquirealltracing::error!withrepo/servicecontext on failure;create_repojust did.map_err(|e| AppError::Git(e.to_string()))?.So a failed
POST /api/v1/reposreturns an opaque500 {"error":"git_error","message":"initializing bare repo"}with no server-side trail to the real cause.to_string()on theanyhow::Errorkeeps only the outer.context(...), and nothing logs the rest, so the underlyinginit_barefailure (thegit initstderr, a filesystem error likeNo space left on device, and so on) is lost.Change
create_reponow logs the full chain on failure ({:#}, so the leaf cause is kept) withowner/repocontext, matching the other git handlers. The response is unchanged, the detail goes to the logs only, not the client.One handler, one log line. No status codes, routes, response bodies, or control flow change.
Note
This makes a failure diagnosable, it doesn't fix whatever the underlying
init_barefailure is (that looked environment-specific when I hit it against a live node, thegit inititself is fine). The point is the next one shows up in the logs with its cause instead of as a bare 500.Summary by CodeRabbit