fix(node): make creation-route rate limit configurable; fix(gl): surface real errors on pr create failure#148
Conversation
…ace real errors on pr create failure - gitlawb-node: rate limiter was hardcoded to 10 req/hour per DID across all creation routes (repo/issue/PR create, register, fork). Adds GITLAWB_RATE_LIMIT_MAX and GITLAWB_RATE_LIMIT_WINDOW_SECS config fields (clap env-var pattern matching the rest of Config), defaults unchanged. Lets node operators tune this for real agent/bot usage without recompiling. - gl pr create: previously called resp.json() before checking response status, so any non-2xx response with an empty or non-JSON body crashed with a generic "invalid JSON" error instead of surfacing the real cause. Now reads the raw response body first and includes it in the error message on failure.
github-actions
Bot
added
needs-issue
|
Thanks for the contribution. A couple of things will help us review this faster:
See CONTRIBUTING.md. Update the PR and these notes will clear automatically. |
📝 WalkthroughWalkthroughAdds two configurable rate-limiting fields to node Config (max requests, window seconds) wired via CLI/env, uses them to construct the rate limiter in main(), and changes gl's PR create command to read response text first, erroring with the raw body on failure before parsing JSON. ChangesConfigurable Rate Limiting
PR Create Error Handling
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Main
participant Config
participant RateLimiter
Main->>Config: read rate_limit_max_requests, rate_limit_window_secs
Main->>RateLimiter: new(max_requests, window_duration)
Related Issues: Not specified in the provided context. Related PRs: Not specified in the provided context. Suggested labels: enhancement, rate-limiting, cli Suggested reviewers: Not specified in the provided context. 🐰 A window ticks, a limit set, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/config.rs (1)
155-164: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against degenerate configured values.
If
GITLAWB_RATE_LIMIT_WINDOW_SECS=0,rate_limit.rs'scheck()retains timestamps whereduration_since(t) < self.window, so with a zero window this is always false — the timestamp count stays at 0 and the limiter never blocks anything, silently disabling rate limiting. Conversely,GITLAWB_RATE_LIMIT_MAX=0makeslen() >= max_requestsalways true, blocking all creation requests. Since these are now operator-configurable via env vars, a misconfiguration could unexpectedly disable protection or lock out all users.Consider validating these values (e.g., panic/warn on 0) in
Configconstruction ormain().🤖 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/config.rs` around lines 155 - 164, The rate-limit config fields can be set to degenerate values that either disable protection or block all requests, so add validation for rate_limit_max_requests and rate_limit_window_secs before the limiter is used. Guard Config construction or main() against zero values for these settings, and fail fast or emit a clear warning rather than letting rate_limit.rs::check() run with an unusable window or max count.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/config.rs`:
- Around line 155-164: The rate-limit config fields can be set to degenerate
values that either disable protection or block all requests, so add validation
for rate_limit_max_requests and rate_limit_window_secs before the limiter is
used. Guard Config construction or main() against zero values for these
settings, and fail fast or emit a clear warning rather than letting
rate_limit.rs::check() run with an unusable window or max count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9cbeebc7-3682-4383-94ce-66168cdfdedc
📒 Files selected for processing (3)
crates/gitlawb-node/src/config.rscrates/gitlawb-node/src/main.rscrates/gl/src/pr.rs
There was a problem hiding this comment.
Pull request overview
This PR makes two targeted improvements across the CLI and node daemon: (1) the node’s creation-route rate limiting becomes operator-configurable via existing clap/env config patterns, and (2) the gl pr create CLI surfaces the real HTTP failure body instead of masking it behind a JSON parse error.
Changes:
- Make creation-route rate limit parameters configurable (
max_requests,window_secs) instead of hardcoded constants. - Improve
gl pr createerror handling by reading the raw response body first and only parsing JSON on success. - Update node startup to construct the rate limiter from config.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
crates/gl/src/pr.rs |
Improves PR creation failure reporting by using raw response text and parsing JSON only on success. |
crates/gitlawb-node/src/main.rs |
Builds the node’s rate limiter using configuration values instead of hardcoded defaults. |
crates/gitlawb-node/src/config.rs |
Adds new clap/env configuration fields for creation-route rate limiting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let raw_text = resp.text().await.context("failed to read response body")?; | ||
| if !status.is_success() { | ||
| let msg = pr["message"].as_str().unwrap_or("unknown error"); | ||
| anyhow::bail!("create PR failed ({status}): {msg}"); | ||
| anyhow::bail!("create PR failed ({status}): {raw_text}"); | ||
| } |
| let resp = client | ||
| .post(&format!("/api/v1/repos/{owner}/{repo}/pulls"), &payload) | ||
| .await | ||
| .context("failed to connect to node")?; | ||
| let status = resp.status(); | ||
| let pr: Value = resp.json().await.context("invalid JSON")?; | ||
|
|
||
| let raw_text = resp.text().await.context("failed to read response body")?; | ||
| if !status.is_success() { | ||
| let msg = pr["message"].as_str().unwrap_or("unknown error"); | ||
| anyhow::bail!("create PR failed ({status}): {msg}"); | ||
| anyhow::bail!("create PR failed ({status}): {raw_text}"); | ||
| } | ||
| let pr: Value = serde_json::from_str(&raw_text).context("invalid JSON")?; |
| /// Maximum number of creation requests (repo create, issue create, PR | ||
| /// create, register, fork) allowed per DID within the rate-limit window. | ||
| #[arg(long, env = "GITLAWB_RATE_LIMIT_MAX", default_value_t = 10)] | ||
| pub rate_limit_max_requests: u32, | ||
|
|
| /// Rate-limit window size, in seconds, over which | ||
| /// `rate_limit_max_requests` applies. | ||
| #[arg(long, env = "GITLAWB_RATE_LIMIT_WINDOW_SECS", default_value_t = 3600)] | ||
| pub rate_limit_window_secs: u64, |
| let rate_limiter = rate_limit::RateLimiter::new( | ||
| config.rate_limit_max_requests as usize, | ||
| std::time::Duration::from_secs(config.rate_limit_window_secs), | ||
| ); |
beardthelion
added
crate:gl
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I found one issue to clean up before this is ready.
Findings
- [P3] Update the PR description to match the final migration
PR description
The current description still says migration v10 adds both theowner_did TEXTcolumn and theidx_ref_updates_ownerindex, but the final patch intentionally dropped that index and the migration now only adds the nullable column. Since the index was deferred to the follow-up gate work, please update the PR body so reviewers and operators do not come away thinking this branch already ships that index.
Two small, independent fixes found while building an autonomous agent on gitlawb:
1. Configurable creation-route rate limit (gitlawb-node)
The rate limiter was hardcoded to 10 requests/hour per DID, shared across the whole creation-routes group (repo create, issue create, PR create, register, fork):
This is workable for a human clicking around, but a real agent/bot doing modest volume (or retrying after a transient error) hits the wall fast, with no way to raise it short of recompiling. We hit this directly running an ACP-hired worker agent — it also caused a real job to expire mid-SLA while rate-limited.
This PR adds two config fields following the existing clap env-var pattern already used throughout
Config:GITLAWB_RATE_LIMIT_MAX(default 10, unchanged)GITLAWB_RATE_LIMIT_WINDOW_SECS(default 3600, unchanged)main.rsnow builds the limiter from config instead of the hardcoded values. No behavior changes unless a node operator explicitly opts in.2.
gl pr createmasks the real error on failureCalling
.json()before checkingstatusmeans any non-2xx response with an empty or non-JSON body (e.g. a 404 from a malformed URL) crashes with a generic "invalid JSON" error instead of the real one. Fixed by reading the raw response body first and including it in the error message when the request fails.Testing
Both changes verified with
cargo check -p gitlawb-node -p gl— compiles clean, no warnings.Summary by CodeRabbit
New Features
Bug Fixes