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.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The core change is sound and behavior-preserving at the defaults: I confirmed Config's defaults (10/3600) reproduce the old hardcoded limiter (10 allowed, 11th blocked), and that an unsigned creation POST still 401s at require_signature before the limiter runs, so the did == None branch is unreachable on these routes. The key is the verified DID, not a forgeable header, so there is no throttle bypass here. A few things to tighten before merge, highest first.
Findings
-
[P2] Reject 0 for both new rate-limit knobs
crates/gitlawb-node/src/config.rs:155-164
Both accept 0 via env/CLI.GITLAWB_RATE_LIMIT_WINDOW_SECS=0silently disables the limiter (every timestamp is pruned, socheckalways passes) andGITLAWB_RATE_LIMIT_MAX=0blocks all creation (len() >= 0is always true). Both are execution-confirmed. The window case is the worse one: it fails open on a DoS brake with no signal to the operator, and0reads as "unlimited" to most people. Addvalue_parser = clap::value_parser!(u32).range(1..)(and theu64equivalent on the window) so a bad value fails fast at startup instead of at runtime. -
[P3] Sanitize and bound the raw server body before printing it
crates/gl/src/pr.rs:239
The new error path embeds the entire response body verbatim. A hostile or compromised node can return ANSI/terminal control sequences (confirmed: an ESC byte passes through unsanitized) or a very large body, both of which land directly in the caller's terminal. An empty body also yields a danglingcreate PR failed (404):. Strip control characters, cap the length, and substitute a placeholder when the body is empty. -
[P3] Derive retry-after from the configured window
crates/gitlawb-node/src/rate_limit.rs:81
The 429 sends a hardcodedretry-after: 60while the window is now operator-configurable (confirmed still60with a 7200s window). A client that honors Retry-After will retry into another 429 until the real window elapses. Compute the header from the window, or the caller's remaining time. -
[P3] Add a regression test for a non-2xx non-JSON body in
cmd_createcrates/gl/src/pr.rs
The point of the change is to stop masking non-JSON error bodies as "invalid JSON", but the only failure test uses a JSON body and passes under both the old and new code, so it does not lock the behavior in. Add awith_status(502)case with a non-JSON body asserting the raw text surfaces. -
[Process] Rebase onto current main before merge
The base is 11 commits behind and overlapsmain.rs; confirm theRateLimiter::newwiring survives the rebase.
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