Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,8 @@ single-user local setup where the checked-out repo is trusted, set
`POSTIL_ALLOW_CONFIG_API_BASE=1` to honor the config value.

Environment: `POSTIL_API_KEY`, `OPENROUTER_API_KEY`, `MODEL_API_KEY`, or
`LLM_API_KEY`, `POSTIL_API_BASE`,
`LLM_API_KEY`, `POSTIL_API_BASE`, `POSTIL_DETAILS_URL` (optional HTTP(S) target
for GitHub check-run details links),
`REVIEW_MODEL`, `REVIEW_MODEL_CASCADE`, `GITHUB_TOKEN`/`GITHUB_API_URL`,
`GITLAB_TOKEN`/`GITLAB_API_URL`, `BITBUCKET_TOKEN`/`BITBUCKET_USER`/`BITBUCKET_API_URL`,
`AZURE_DEVOPS_TOKEN`/`AZURE_DEVOPS_API_URL`.
Expand Down
128 changes: 98 additions & 30 deletions src/forge/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::envelope::{Envelope, Finding, Severity};
pub struct GitHub {
http: reqwest::Client,
api_base: String,
details_url: Option<String>,
token: String,
owner: String,
repo: String,
Expand All @@ -25,11 +26,13 @@ impl GitHub {
.map_err(|_| anyhow!("GITHUB_TOKEN is required for --forge github"))?;
let api_base = std::env::var("GITHUB_API_URL")
.unwrap_or_else(|_| "https://api.github.com".to_string());
let details_url = valid_details_url(std::env::var("POSTIL_DETAILS_URL").ok());
Ok(GitHub {
http: reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(60))
.build()?,
api_base: api_base.trim_end_matches('/').to_string(),
details_url,
token,
owner: owner.to_string(),
repo: repo.to_string(),
Expand All @@ -49,6 +52,12 @@ impl GitHub {
.header("X-GitHub-Api-Version", "2022-11-28")
}

fn add_details_url(&self, body: &mut serde_json::Value) {
if let Some(details_url) = &self.details_url {
body["details_url"] = json!(details_url);
}
}

async fn check_ok(resp: reqwest::Response, what: &str) -> Result<reqwest::Response> {
let status = resp.status();
if status.is_success() {
Expand All @@ -60,6 +69,53 @@ impl GitHub {
}
}

fn valid_details_url(value: Option<String>) -> Option<String> {
value.filter(|value| {
reqwest::Url::parse(value)
.map(|url| matches!(url.scheme(), "http" | "https") && url.has_host())
.unwrap_or(false)
})
}

fn gate_title(envelope: &Envelope) -> &'static str {
if envelope.gate.failing {
"Merge gate failed"
} else {
"Merge gate passed"
}
}

fn gate_summary(envelope: &Envelope) -> String {
if !envelope.gate.failing {
return format!(
"Merge gate passed: no findings at or above the blocking threshold ({}).\n",
envelope.gate.fail_on
);
}

let failing: Vec<_> = envelope
.findings
.iter()
.filter(|f| {
crate::config::GateLevel::parse(&envelope.gate.fail_on)
.map(|g| g.fails(f.severity))
.unwrap_or(false)
})
.map(|f| format!("- `{}:{}` {}", f.path, f.line, f.title))
.collect();
let noun = if failing.len() == 1 {
"finding"
} else {
"findings"
};
format!(
"Merge gate failed: {} {noun} at or above the blocking threshold ({}).\n\n{}\n",
failing.len(),
envelope.gate.fail_on,
failing.join("\n")
)
}

#[derive(Deserialize)]
struct PrResponse {
title: String,
Expand Down Expand Up @@ -178,13 +234,15 @@ impl Forge for GitHub {
async fn start_checks(&self, head_sha: &str) -> Result<(String, String)> {
let mut ids = Vec::with_capacity(2);
for name in ["postil/review", "postil/gate"] {
let mut body = json!({
"name": name,
"head_sha": head_sha,
"status": "in_progress",
});
self.add_details_url(&mut body);
let resp = self
.request(reqwest::Method::POST, self.url("/check-runs"))
.json(&json!({
"name": name,
"head_sha": head_sha,
"status": "in_progress",
}))
.json(&body)
.send()
.await
.with_context(|| format!("creating check-run {name}"))?;
Expand Down Expand Up @@ -238,47 +296,36 @@ impl Forge for GitHub {
(gate_id, gate, "postil/gate", false),
] {
let gate_note = if name == "postil/gate" {
let failing: Vec<_> = envelope
.findings
.iter()
.filter(|f| {
crate::config::GateLevel::parse(&envelope.gate.fail_on)
.map(|g| g.fails(f.severity))
.unwrap_or(false)
})
.map(|f| format!("- `{}:{}` {}", f.path, f.line, f.title))
.collect();
if envelope.gate.failing {
format!(
"Gate failing at `{}` on:\n{}\n",
envelope.gate.fail_on,
failing.join("\n")
)
} else {
format!("Gate (`failOn: {}`) passing.\n", envelope.gate.fail_on)
}
gate_summary(envelope)
} else {
check_summary(envelope, true)
};
let title = if name == "postil/gate" {
gate_title(envelope).to_string()
} else {
check_title(envelope)
};
let mut output = json!({
// GitHub rejects title >255 and summary >65535 with HTTP 422,
// which would abort posting both checks. Cap both defensively.
"title": super::cap_check_title(&check_title(envelope)),
"title": super::cap_check_title(&title),
"summary": super::cap_check_summary(&gate_note),
});
if with_annotations && !annotations.is_empty() {
output["annotations"] = json!(annotations);
}
let mut body = json!({
"status": "completed",
"conclusion": conclusion(state),
"output": output,
});
self.add_details_url(&mut body);
let resp = self
.request(
reqwest::Method::PATCH,
self.url(&format!("/check-runs/{id}")),
)
.json(&json!({
"status": "completed",
"conclusion": conclusion(state),
"output": output,
}))
.json(&body)
.send()
.await
.with_context(|| format!("completing check-run {name}"))?;
Expand Down Expand Up @@ -316,3 +363,24 @@ impl Forge for GitHub {
Ok(())
}
}

#[cfg(test)]
mod tests {
use super::valid_details_url;

#[test]
fn details_url_accepts_only_http_and_https_urls() {
assert_eq!(
valid_details_url(Some("https://postil.dev/orgs/acme/runs/review-1".into())),
Some("https://postil.dev/orgs/acme/runs/review-1".into())
);
assert_eq!(
valid_details_url(Some("http://localhost:3000/runs/review-1".into())),
Some("http://localhost:3000/runs/review-1".into())
);
assert_eq!(valid_details_url(Some("ftp://postil.dev/run".into())), None);
assert_eq!(valid_details_url(Some("https://".into())), None);
assert_eq!(valid_details_url(Some("not a URL".into())), None);
assert_eq!(valid_details_url(None), None);
}
}
45 changes: 45 additions & 0 deletions tests/e2e.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ fn postil() -> Command {
.env_remove("OPENROUTER_API_KEY")
.env_remove("POSTIL_API_KEY")
.env_remove("POSTIL_API_BASE")
.env_remove("POSTIL_DETAILS_URL")
.env("MODEL_API_KEY", "test-key");
cmd
}
Expand Down Expand Up @@ -830,6 +831,10 @@ async fn github_flow_posts_review_and_completes_both_checks() {
.env("POSTIL_API_BASE", server.uri())
.env("GITHUB_API_URL", server.uri())
.env("GITHUB_TOKEN", "gh-test-token")
.env(
"POSTIL_DETAILS_URL",
"https://postil.dev/orgs/acme/runs/review-7",
)
.args(["review", "--repo", "acme/api", "--pr", "7", "--output-json"])
.assert()
.code(1);
Expand All @@ -847,6 +852,10 @@ async fn github_flow_posts_review_and_completes_both_checks() {
})
.collect();
assert_eq!(creates.len(), 2);
assert!(creates.iter().all(|request| {
request.body_json::<Value>().unwrap()["details_url"]
== "https://postil.dev/orgs/acme/runs/review-7"
}));
let names: Vec<String> = creates
.iter()
.map(|r| {
Expand All @@ -865,12 +874,26 @@ async fn github_flow_posts_review_and_completes_both_checks() {
.map(|r| r.body_json().unwrap())
.collect();
assert_eq!(patches.len(), 2);
assert!(
patches
.iter()
.all(|patch| { patch["details_url"] == "https://postil.dev/orgs/acme/runs/review-7" })
);
let conclusions: Vec<&str> = patches
.iter()
.map(|p| p["conclusion"].as_str().unwrap())
.collect();
assert!(conclusions.contains(&"success"));
assert!(conclusions.contains(&"failure"));
let gate_patch = patches
.iter()
.find(|patch| patch["conclusion"] == "failure")
.unwrap();
assert_eq!(gate_patch["output"]["title"], "Merge gate failed");
assert_eq!(
gate_patch["output"]["summary"],
"Merge gate failed: 1 finding at or above the blocking threshold (error).\n\n- `src/auth.rs:41` Unsanitized input reaches query\n"
);
// Inline review posted with the finding at the cited line.
let review = reqs
.iter()
Expand Down Expand Up @@ -1094,6 +1117,28 @@ async fn github_clean_pr_stays_silent_but_completes_checks() {
})
.collect();
assert_eq!(conclusions, vec!["success", "success"]);
let check_requests: Vec<Value> = reqs
.iter()
.filter(|request| {
request.url.path().starts_with("/repos/acme/api/check-runs")
&& matches!(
request.method,
wiremock::http::Method::POST | wiremock::http::Method::PATCH
)
})
.map(|request| request.body_json().unwrap())
.collect();
assert!(
check_requests
.iter()
.all(|request| request.get("details_url").is_none())
);
let gate_patch = &check_requests[3];
assert_eq!(gate_patch["output"]["title"], "Merge gate passed");
assert_eq!(
gate_patch["output"]["summary"],
"Merge gate passed: no findings at or above the blocking threshold (error).\n"
);
}

#[tokio::test]
Expand Down
Loading