Skip to content

Commit eadaea3

Browse files
BunsDevCopilot
andauthored
feat(worker): deterministic publication gates and policy modes for review findings (#11) (#53)
Closes #11. Findings now pass an adapter-owned gate chain before any GitHub surface sees them (crates/worker/src/findings.rs): - scope: files the session never consulted (not reviewed, supporting, or in the PR changed set) are withheld as speculative - severity threshold: repo policy min_severity (info..critical) filters below-threshold findings - duplicates: identical (file, line, title) findings collapse The rendered digest is bounded and honest — withheld counts and reasons are always stated, so a quiet report is distinguishable from a filtered one. A new [review] publish policy routes the gated digest: check_run (default), advisory_comment (also on the marker-backed status comment), or request_changes (a blocking PR review verdict submitted with post-gate publication authority — REQUEST_CHANGES when findings exist, COMMENT otherwise). Doctor rejects unknown policy values. Together with what already shipped, this completes #11's gate chain: off-schema envelopes are rejected (contract v2 validation), stale refs are withheld pre-publish (#8), secret redaction covers every published field, and memory writes are approval-gated (#6). A findings confidence axis is specified for headless contract v3; v2 carries severity only. Signed-off-by: Val Alexander <bunsthedev@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent bc80181 commit eadaea3

7 files changed

Lines changed: 782 additions & 3 deletions

File tree

README.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,13 @@ Planned lanes:
113113
| Trigger | Status |
114114
|---|---|
115115
| Push / commit-range review | `push` events are parsed and typed with fixtures today; execution needs a PR-less task kind, which ships with headless contract v3 |
116-
| Advisory / blocking publication gates | Issue #11 |
116+
117+
Review findings pass **deterministic publication gates** before any surface
118+
sees them: out-of-scope files (never consulted by the session), findings below
119+
the repo's `min_severity` policy, and duplicates are withheld — with the
120+
withheld counts stated in the digest. The `[review] publish` policy routes the
121+
gated digest to the Check Run (default), additionally to the status comment
122+
(`advisory_comment`), or as a blocking PR review verdict (`request_changes`).
117123

118124
## Maintainer commands
119125

config/example.toml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,11 @@ trigger_labels = ["coven:fix", "coven:review"] # Labels that trigger this famil
6363
# pull_request = true # Review on opened/synchronize/reopened/ready_for_review
6464
# include_drafts = false # Draft PRs are skipped unless explicitly labeled
6565
# audit_instruction = "Focus on correctness and security."
66+
# min_severity = "medium" # Withhold findings below this severity
67+
# # (info | low | medium | high | critical)
68+
# publish = "check_run" # Where gated findings go: check_run (default),
69+
# # advisory_comment (also on the status comment),
70+
# # request_changes (blocking PR review verdict)
6671
# [review.repos."OpenCoven/coven-code"]
6772
# pull_request = false # Per-repo override
6873

crates/config/src/lib.rs

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,13 @@ pub struct ReviewConfig {
143143
pub include_drafts: bool,
144144
/// Adapter-authored instruction forwarded as the brief's audit_instruction.
145145
pub audit_instruction: Option<String>,
146+
/// Minimum finding severity that publishes (`info`, `low`, `medium`,
147+
/// `high`, `critical`). Absent = every severity publishes (issue #11).
148+
pub min_severity: Option<String>,
149+
/// Where gated findings publish (issue #11): `check_run` (default),
150+
/// `advisory_comment` (also on the status comment), or `request_changes`
151+
/// (submit a PR review that requests changes when findings exist).
152+
pub publish: Option<String>,
146153
/// Per-repo overrides keyed "owner/name".
147154
#[serde(default)]
148155
pub repos: std::collections::HashMap<String, RepoReviewOverride>,
@@ -155,6 +162,8 @@ pub struct RepoReviewOverride {
155162
pub include_drafts: Option<bool>,
156163
pub familiar: Option<String>,
157164
pub audit_instruction: Option<String>,
165+
pub min_severity: Option<String>,
166+
pub publish: Option<String>,
158167
}
159168

160169
impl ReviewConfig {
@@ -185,6 +194,20 @@ impl ReviewConfig {
185194
.and_then(|o| o.audit_instruction.clone())
186195
.or_else(|| self.audit_instruction.clone())
187196
}
197+
198+
/// Minimum publishable severity for `repo` (raw string; issue #11).
199+
pub fn min_severity_for(&self, repo: &str) -> Option<String> {
200+
self.overrides(repo)
201+
.and_then(|o| o.min_severity.clone())
202+
.or_else(|| self.min_severity.clone())
203+
}
204+
205+
/// Findings publication mode for `repo` (raw string; issue #11).
206+
pub fn publish_for(&self, repo: &str) -> Option<String> {
207+
self.overrides(repo)
208+
.and_then(|o| o.publish.clone())
209+
.or_else(|| self.publish.clone())
210+
}
188211
}
189212

190213
#[derive(Debug, Clone, Deserialize, Serialize)]
@@ -476,6 +499,40 @@ impl Config {
476499
);
477500
}
478501
}
502+
// Publication policy values are closed enums (issue #11): a typo would
503+
// silently change what publishes, so doctor rejects unknown values.
504+
let severities = ["info", "low", "medium", "high", "critical"];
505+
let publish_modes = ["check_run", "advisory_comment", "request_changes"];
506+
let mut check_policy = |scope: &str, min_severity: Option<&str>, publish: Option<&str>| {
507+
if let Some(value) = min_severity {
508+
if !severities.contains(&value.to_ascii_lowercase().as_str()) {
509+
out.push(Diagnostic::error(
510+
"review.min_severity",
511+
format!("{scope} has unknown min_severity '{value}' — use one of: {}.", severities.join(", ")),
512+
));
513+
}
514+
}
515+
if let Some(value) = publish {
516+
if !publish_modes.contains(&value.to_ascii_lowercase().as_str()) {
517+
out.push(Diagnostic::error(
518+
"review.publish",
519+
format!("{scope} has unknown publish mode '{value}' — use one of: {}.", publish_modes.join(", ")),
520+
));
521+
}
522+
}
523+
};
524+
check_policy(
525+
"the [review] section",
526+
self.review.min_severity.as_deref(),
527+
self.review.publish.as_deref(),
528+
);
529+
for (repo, o) in &self.review.repos {
530+
check_policy(
531+
&format!("the review override for '{repo}'"),
532+
o.min_severity.as_deref(),
533+
o.publish.as_deref(),
534+
);
535+
}
479536

480537
// ── Memory governance (issue #6) ────────────────────────────────
481538
// Memory is off by default; when an operator enables it anywhere,
@@ -585,6 +642,12 @@ fn next_step_for(field: &str, _message: &str) -> &'static str {
585642
"api.tenants[].token" => {
586643
"Generate long random tokens (e.g. openssl rand -hex 32) and keep each scope's token unique."
587644
}
645+
"review.min_severity" => {
646+
"Set review.min_severity to one of: info, low, medium, high, critical."
647+
}
648+
"review.publish" => {
649+
"Set review.publish to one of: check_run, advisory_comment, request_changes."
650+
}
588651
_ => "Update this config field, then rerun coven-github doctor.",
589652
}
590653
}
@@ -729,17 +792,36 @@ mod tests {
729792
pull_request: true,
730793
include_drafts: false,
731794
audit_instruction: Some("global".to_string()),
795+
min_severity: Some("medium".to_string()),
796+
publish: None,
732797
repos: std::collections::HashMap::from([(
733798
"OpenCoven/quiet".to_string(),
734799
RepoReviewOverride {
735800
pull_request: Some(false),
736801
include_drafts: Some(true),
737802
familiar: Some("nova".to_string()),
738803
audit_instruction: None,
804+
min_severity: None,
805+
publish: Some("advisory_comment".to_string()),
739806
},
740807
)]),
741808
};
742809

810+
// Publication policy inherits globally and overrides per repo (#11).
811+
assert_eq!(
812+
policy.min_severity_for("OpenCoven/loud").as_deref(),
813+
Some("medium")
814+
);
815+
assert_eq!(
816+
policy.min_severity_for("OpenCoven/quiet").as_deref(),
817+
Some("medium")
818+
);
819+
assert_eq!(policy.publish_for("OpenCoven/loud"), None);
820+
assert_eq!(
821+
policy.publish_for("OpenCoven/quiet").as_deref(),
822+
Some("advisory_comment")
823+
);
824+
743825
assert!(policy.pull_request_enabled("OpenCoven/loud"));
744826
assert!(!policy.pull_request_enabled("OpenCoven/quiet"));
745827
assert!(!policy.drafts_included("OpenCoven/loud"));

crates/github/src/pr.rs

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,60 @@ fn update_comment_request(
265265
}
266266
}
267267

268+
/// Verdict of an adapter-submitted PR review (issue #11).
269+
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
270+
pub enum ReviewVerdict {
271+
/// Findings exist: block with requested changes.
272+
RequestChanges,
273+
/// Nothing actionable: a non-blocking comment review.
274+
Comment,
275+
}
276+
277+
impl ReviewVerdict {
278+
fn as_str(self) -> &'static str {
279+
match self {
280+
ReviewVerdict::RequestChanges => "REQUEST_CHANGES",
281+
ReviewVerdict::Comment => "COMMENT",
282+
}
283+
}
284+
}
285+
286+
/// Submits a pull request review with the given verdict and body
287+
/// (`request_changes` publication mode, issue #11).
288+
pub async fn submit_review_with_base_url(
289+
api_base_url: &str,
290+
installation_token: &str,
291+
repo_owner: &str,
292+
repo_name: &str,
293+
pr_number: u64,
294+
verdict: ReviewVerdict,
295+
body: &str,
296+
) -> Result<()> {
297+
let client = client()?;
298+
send_json(
299+
&client,
300+
api_base_url,
301+
installation_token,
302+
submit_review_request(repo_owner, repo_name, pr_number, verdict, body),
303+
)
304+
.await?;
305+
Ok(())
306+
}
307+
308+
fn submit_review_request(
309+
repo_owner: &str,
310+
repo_name: &str,
311+
pr_number: u64,
312+
verdict: ReviewVerdict,
313+
body: &str,
314+
) -> GitHubRequest {
315+
GitHubRequest {
316+
method: "POST",
317+
path: format!("/repos/{repo_owner}/{repo_name}/pulls/{pr_number}/reviews"),
318+
body: serde_json::json!({ "event": verdict.as_str(), "body": body }),
319+
}
320+
}
321+
268322
#[cfg(test)]
269323
mod comment_tests {
270324
use super::*;
@@ -285,6 +339,16 @@ mod comment_tests {
285339
assert_eq!(request.body["body"], json!("new body"));
286340
}
287341

342+
#[test]
343+
fn submit_review_request_posts_the_verdict() {
344+
let request =
345+
submit_review_request("octo", "repo", 88, ReviewVerdict::RequestChanges, "digest");
346+
assert_eq!(request.method, "POST");
347+
assert_eq!(request.path, "/repos/octo/repo/pulls/88/reviews");
348+
assert_eq!(request.body["event"], json!("REQUEST_CHANGES"));
349+
assert_eq!(request.body["body"], json!("digest"));
350+
}
351+
288352
#[test]
289353
fn issue_comment_deserializes_id_body_and_author() {
290354
let comments: Vec<IssueComment> = serde_json::from_value(json!([

crates/webhook/src/routes.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,6 +1003,8 @@ mod review_lane_tests {
10031003
pull_request: true,
10041004
include_drafts: false,
10051005
audit_instruction: None,
1006+
min_severity: None,
1007+
publish: None,
10061008
repos: std::collections::HashMap::new(),
10071009
}
10081010
}
@@ -1100,6 +1102,8 @@ mod review_lane_tests {
11001102
include_drafts: None,
11011103
familiar: None,
11021104
audit_instruction: None,
1105+
min_severity: None,
1106+
publish: None,
11031107
},
11041108
);
11051109
let state = app_state_with_review(review);

0 commit comments

Comments
 (0)