Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Greptile SummaryThis PR adds HTTP URL rewrite support to
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant lib as lib.rs (eval_to_json_with_options)
participant Evaluator
participant rewrite as rewrite_url()
participant http_cache
participant HTTP as HTTP Server
Caller->>lib: eval_to_json_with_options(path, EvalOptions { http_rewrites })
lib->>Evaluator: new() + set_http_rewrites(rules)
lib->>Evaluator: eval_source(source, path)
Evaluator->>Evaluator: fetch_source(original_url)
Evaluator->>rewrite: rewrite_url(original_url)
rewrite-->>Evaluator: fetch_url (longest-prefix match applied)
Evaluator->>http_cache: get(fetch_url)
alt cache hit
http_cache-->>Evaluator: cached body
else cache miss
Evaluator->>HTTP: GET fetch_url
HTTP-->>Evaluator: response body
Evaluator->>http_cache: insert(fetch_url, body)
end
Evaluator-->>lib: Value
lib-->>Caller: serde_json::Value
Reviews (1): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile |
| pub fn set_http_rewrites(&mut self, rules: &[String]) { | ||
| self.http_rewrites = rules | ||
| .iter() | ||
| .filter_map(|rule| { | ||
| let (src, tgt) = rule.split_once('=')?; | ||
| Some((src.to_string(), tgt.to_string())) | ||
| }) | ||
| .collect(); | ||
| } |
There was a problem hiding this comment.
split_once('=') on a rule like "=https://mirror.local/" succeeds and produces ("", "https://mirror.local/"). Since url.starts_with("") is always true in Rust, this empty-source rule will match every URL and redirect all HTTP traffic to the target. There is no guard against an empty source prefix, and no error or warning is emitted.
If a user accidentally adds a leading = (e.g., from a misconfigured env var like HK_PKL_HTTP_REWRITE=https://mirror.local/), every fetch will silently be rerouted.
Consider validating the parsed rule before inserting it:
pub fn set_http_rewrites(&mut self, rules: &[String]) {
self.http_rewrites = rules
.iter()
.filter_map(|rule| {
let (src, tgt) = rule.split_once('=')?;
if src.is_empty() {
eprintln!("pklr: ignoring rewrite rule with empty source prefix: {rule}");
return None;
}
Some((src.to_string(), tgt.to_string()))
})
.collect();
}| pub fn set_http_rewrites(&mut self, rules: &[String]) { | ||
| self.http_rewrites = rules | ||
| .iter() | ||
| .filter_map(|rule| { | ||
| let (src, tgt) = rule.split_once('=')?; | ||
| Some((src.to_string(), tgt.to_string())) | ||
| }) | ||
| .collect(); | ||
| } |
There was a problem hiding this comment.
Rules that do not contain = are silently discarded by filter_map. If a caller passes a mis-formatted rule string, they receive no indication that the rule was ignored. This could make debugging very difficult, especially when the feature is configured through an environment variable like HK_PKL_HTTP_REWRITE.
Since set_http_rewrites currently returns (), it cannot propagate errors. Consider either:
- Returning
Result<()>and propagating the first malformed rule as an error, or - At minimum, adding a
debug!/eprintln!for any rule that fails to parse.
| let fetch_url = rewritten.as_ref(); | ||
| if let Some(cached) = self.http_cache.get(fetch_url) { | ||
| return Ok(cached.clone()); | ||
| } | ||
| let body = self | ||
| .http_client | ||
| .get(url) | ||
| .get(fetch_url) | ||
| .send() | ||
| .await | ||
| .map_err(|e| Error::Eval(format!("HTTP fetch failed for {url}: {e}")))? | ||
| .map_err(|e| Error::Eval(format!("HTTP fetch failed for {fetch_url}: {e}")))? | ||
| .error_for_status() | ||
| .map_err(|e| Error::Eval(format!("HTTP error for {url}: {e}")))? | ||
| .map_err(|e| Error::Eval(format!("HTTP error for {fetch_url}: {e}")))? | ||
| .text() | ||
| .await | ||
| .map_err(|e| Error::Eval(format!("HTTP read failed for {url}: {e}")))?; | ||
| self.http_cache.insert(url.to_string(), body.clone()); | ||
| .map_err(|e| Error::Eval(format!("HTTP read failed for {fetch_url}: {e}")))?; | ||
| self.http_cache.insert(fetch_url.to_string(), body.clone()); | ||
| Ok(body) | ||
| } | ||
|
|
There was a problem hiding this comment.
The http_cache is now keyed by fetch_url (the post-rewrite URL) rather than the original url. While this is logically correct (two original URLs that rewrite to the same mirror will share a cache entry), the error messages now report only the rewritten URL, making it harder to correlate a failure back to the original URL the pkl file imported.
Consider including the original URL in error messages when a rewrite is active:
let orig = url;
let rewritten = self.rewrite_url(url);
let fetch_url = rewritten.as_ref();
// ...
.map_err(|e| {
if fetch_url != orig {
Error::Eval(format!("HTTP fetch failed for {orig} (rewritten to {fetch_url}): {e}"))
} else {
Error::Eval(format!("HTTP fetch failed for {fetch_url}: {e}"))
}
})?Add `set_http_rewrites()` to the Evaluator and `eval_to_json_with_options()` to the public API. Rewrite rules use the same `source_prefix=target_prefix` format as pkl CLI's `--http-rewrite` flag, with longest-prefix-match semantics. Rewrites are applied in `fetch_source()` and `extract_package_zip()`, covering all HTTP fetches (imports, amends, extends, packages, read()). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## 🤖 New release * `pklr`: 0.2.1 -> 0.2.2 (✓ API compatible changes) <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.2.2](v0.2.1...v0.2.2) - 2026-03-24 ### Added - add HTTP URL rewrite support ([#46](#46)) ### Fixed - strip inherited class definitions from extends and remote amends ([#48](#48)) - strip inherited class definitions from amends output ([#45](#45)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
Summary
set_http_rewrites()to theEvaluatorfor configuring URL rewrite ruleseval_to_json_with_options()withEvalOptionsstruct to the public APIsource_prefix=target_prefixformat matching pkl CLI's--http-rewritefetch_source()andextract_package_zip()HK_PKL_HTTP_REWRITEwith the pklr backendTest plan
rewrite_url_longest_prefix_winstestrewrite_url_no_rules_is_identitytestThis PR was generated by Claude Code.