Skip to content

Restore the pre-terse review prompt; keep parallel fetches and a runaway output bound #36

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
morgaesis merged 1 commit into from
Jul 10, 2026
Merged

Restore the pre-terse review prompt; keep parallel fetches and a runaway output bound #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions src/llm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ pub struct LlmClient {
/// Retries per model on transient provider errors before the cascade moves on.
const TRANSIENT_RETRIES: u32 = 2;

/// A review returns a compact JSON object, even at the configured maximum of
/// 20 findings. Bound review generation so reasoning-capable models cannot
/// spend minutes producing an oversized completion. Interactive answers use
/// their provider default because they have a different output contract.
const REVIEW_MAX_TOKENS: u32 = 4096;
/// Runaway-generation bound only. It is sized so legitimate reviews (observed
/// up to roughly 12k output tokens) do not truncate. A truncated response goes
/// through JSON repair and can salvage low-quality findings. Interactive answers
/// use their provider default.
const REVIEW_MAX_TOKENS: u32 = 16384;

/// Marker context attached to transport/provider-level failures (endpoint
/// unreachable, HTTP error status, timeout, malformed HTTP envelope) — the
Expand Down
219 changes: 84 additions & 135 deletions src/prompt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! Prompt construction. The system prompt is the noise policy.

use crate::config::{BUILTIN_CONTENT_POLICY, Config};
use crate::config::Config;

pub struct PrContext<'a> {
pub repo: Option<&'a str>,
Expand Down Expand Up @@ -58,61 +58,6 @@ pub fn system_prompt(cfg: &Config) -> String {
Cite ONLY line numbers printed in the left margin of the diff (the new-file line \
numbers). Findings citing other lines are discarded as ungrounded.\n",
);
// Keep the response contract ahead of every repository-specific section.
// Implicit prompt caches match byte-identical prefixes, so global review
// instructions and the built-in policy baseline must precede focus,
// guardrails, policy additions, and tone.
p.push_str(
"\nRespond with ONLY a JSON object, no markdown fences, no prose or reasoning:\n\
{\"summary\": \"empty when clean; otherwise one sentence of at most 40 words\",\n \
\"findings\": [{\"path\": \"file path from the diff\", \"line\": <new-file line>,\n \
\"endLine\": <optional>, \"severity\": \"info|warn|error\",\n \
\"kind\": \"risk|humanEscalation|guardrail|uncertainty|contentPolicy\", \"confidence\": <0..1>,\n \
\"title\": \"imperative title of at most 12 words\", \"body\": \"one concise paragraph of at most 60 words\"}]}\n\
\n\
Each body must state the evidence, impact, and concrete next step without \
restating the diff or these rules. The summary and findings must agree. Every \
risk the summary mentions MUST appear as a structured finding with its diff \
line; if findings is empty, summary MUST be the empty string. A summary that \
narrates problems alongside an empty findings array is invalid output and will \
fail the review.\n",
);

// Config loading keeps the built-in policy at the start and appends optional
// repository rules. Split them after applying the existing aggregate cap so
// the stable baseline remains cacheable across repositories while the exact
// effective policy text stays unchanged.
let content_policy: Option<String> = cfg
.content_policy
.as_deref()
.map(|policy| policy.chars().take(6000).collect());
let (builtin_policy, repo_policy) = match content_policy.as_deref() {
Some(policy) => match policy.strip_prefix(BUILTIN_CONTENT_POLICY) {
Some(additions) => (Some(BUILTIN_CONTENT_POLICY), additions.trim()),
None => (None, policy.trim()),
},
None => (None, ""),
};
if content_policy.is_some() {
p.push_str(
"\nThis repository has content-policy review enabled. Apply the numbered rules \
below ONLY to human-readable prose in the diff (Markdown, code comments, \
docstrings, user-facing/log strings, PR title/description) — never to code \
logic, identifiers, or structured data. Report a violation with kind \
\"contentPolicy\", name the rule number it breaks, and quote or paraphrase the \
specific offending text in the body. A violation in the PR title or description \
MUST cite the path `.postil/pr-description` and one of the numbered lines shown \
for it; a violation in a diff file cites that file and a new-file line as usual. \
Be conservative: this augments the rules above, it does not turn you into a style \
linter; when a line is borderline, do not flag it.\n",
);
if let Some(policy) = builtin_policy {
p.push_str("--- CONTENT POLICY BASELINE ---\n");
p.push_str(policy);
p.push_str("\n--- END CONTENT POLICY BASELINE ---\n");
}
}

if !cfg.focus.is_empty() {
p.push_str(&format!(
"\nThis repository asks for extra attention to: {}.\n",
Expand All @@ -132,12 +77,45 @@ pub fn system_prompt(cfg: &Config) -> String {
p.push_str(&rules);
p.push_str("\n--- END GUARDRAILS ---\n");
}
if !repo_policy.is_empty() {
p.push_str("\n--- REPOSITORY CONTENT POLICY ---\n");
p.push_str(repo_policy);
p.push_str("\n--- END REPOSITORY CONTENT POLICY ---\n");
if let Some(policy) = &cfg.content_policy {
// Content policy reviews human-readable prose in the diff (docs,
// comments, docstrings, PR title/body) for a different class of
// problem than the core rules above: not "is this code correct" but
// "is this text honest, self-consistent, and free of authoring
// residue". Findings are reportable even though they are not a code
// bug, and must quote or paraphrase the offending prose and name the
// numbered rule it breaks.
p.push_str(
"\nThis repository has content-policy review enabled. Apply the numbered rules \
below ONLY to human-readable prose in the diff (Markdown, code comments, \
docstrings, user-facing/log strings, PR title/description) — never to code \
logic, identifiers, or structured data. Report a violation with kind \
\"contentPolicy\", name the rule number it breaks, and quote or paraphrase the \
specific offending text in the body. A violation in the PR title or description \
MUST cite the path `.postil/pr-description` and one of the numbered lines shown \
for it; a violation in a diff file cites that file and a new-file line as usual. \
Be conservative: this augments the rules above, it does not turn you into a style \
linter; when a line is borderline, do not flag it.\n\
--- CONTENT POLICY ---\n",
);
let policy: String = policy.chars().take(6000).collect();
p.push_str(&policy);
p.push_str("\n--- END CONTENT POLICY ---\n");
}
p.push_str(&format!("\nTone for finding bodies: {}.\n", cfg.tone));
p.push_str(
"\nRespond with ONLY a JSON object, no markdown fences, no prose:\n\
{\"summary\": \"1-3 sentences on merge-relevant risk, or empty string if none\",\n \
\"findings\": [{\"path\": \"file path from the diff\", \"line\": <new-file line>,\n \
\"endLine\": <optional>, \"severity\": \"info|warn|error\",\n \
\"kind\": \"risk|humanEscalation|guardrail|uncertainty|contentPolicy\", \"confidence\": <0..1>,\n \
\"title\": \"short imperative title\", \"body\": \"specific, evidence-based markdown\"}]}\n\
\n\
The summary and findings must agree. Every risk the summary mentions MUST appear as \
a structured finding with its diff line; if findings is empty, summary MUST be the \
empty string. A summary that narrates problems alongside an empty findings array is \
invalid output and will fail the review.\n",
);
p
}

Expand Down Expand Up @@ -191,36 +169,8 @@ pub fn render_pr_description(title: Option<&str>, body: Option<&str>) -> (String
(out, line_no)
}

pub fn user_prompt(
ctx: &PrContext,
annotated_diff: &str,
max_findings: usize,
truncated: bool,
) -> String {
let mut p =
format!("Report at most {max_findings} findings; if more exist, keep the most severe.\n");
if ctx.content_policy {
p.push_str(
"The PR title and description below are numbered so you can cite them. A \
content-policy finding about the title/description MUST use the path \
`.postil/pr-description` and one of those line numbers; findings there \
cannot cite any other path.\n",
);
}
if ctx.incremental {
p.push_str(
"This is an INCREMENTAL review: the diff covers only commits pushed since the \
previous review. Earlier findings are tracked separately; review only what is \
shown.\n",
);
}
if truncated {
p.push_str(
"The diff is truncated at the size limit. Review only what is shown and do not \
imply that the omitted remainder was assessed.\n",
);
}
p.push_str("\n--- REVIEW CONTEXT ---\n");
pub fn user_prompt(ctx: &PrContext, annotated_diff: &str, max_findings: usize) -> String {
let mut p = String::new();
if let Some(repo) = ctx.repo {
p.push_str(&format!("Repository: {repo}\n"));
}
Expand All @@ -238,8 +188,14 @@ pub fn user_prompt(
None
};
if let Some(block) = &pr_block {
p.push('\n');
p.push_str(
"\nThe PR title and description below are numbered so you can cite them. A \
content-policy finding about the title/description MUST use the path \
`.postil/pr-description` and one of these line numbers; findings there \
cannot cite any other path.\n\n",
);
p.push_str(block);
p.push('\n');
} else {
if let Some(title) = ctx.title {
p.push_str(&format!("PR title: {title}\n"));
Expand All @@ -252,9 +208,17 @@ pub fn user_prompt(
p.push_str(&format!("PR description:\n{body}\n"));
}
}
p.push_str(
"--- END REVIEW CONTEXT ---\n\nDiff (left margin numbers are new-file line numbers — cite exactly these):\n\n",
);
if ctx.incremental {
p.push_str(
"\nThis is an INCREMENTAL review: the diff below covers only commits pushed \
since the previous review. Earlier findings are tracked separately; review \
only what is shown.\n",
);
}
p.push_str(&format!(
"\nReport at most {max_findings} findings; if more exist, keep the most severe.\n\
\nDiff (left margin numbers are new-file line numbers — cite exactly these):\n\n"
));
p.push_str(annotated_diff);
p
}
Expand Down Expand Up @@ -304,28 +268,33 @@ mod tests {
}

#[test]
fn system_prompt_keeps_global_contract_and_baseline_before_repo_content() {
fn system_prompt_matches_pre_34_section_order_and_contract() {
let mut cfg = Config::default();
cfg.focus = vec!["repository-specific focus".into()];
cfg.guardrails = Some("repository-specific guardrail".into());
cfg.content_policy = Some(format!(
"{BUILTIN_CONTENT_POLICY}\n\n--- REPO-SPECIFIC ADDITIONS ---\nrepository-specific policy"
));
cfg.tone = "repository-specific tone".into();
cfg.focus = vec!["representative focus".into()];
cfg.guardrails = Some("Representative guardrail.".into());
cfg.content_policy = Some("1. Representative content rule.".into());
cfg.tone = "representative tone".into();

let p = system_prompt(&cfg);
let focus = p.find("representative focus").unwrap();
let guardrail = p.find("Representative guardrail.").unwrap();
let policy = p.find("1. Representative content rule.").unwrap();
let tone = p.find("representative tone").unwrap();
let contract = p.find("Respond with ONLY a JSON object").unwrap();
let baseline = p.find("CONTENT POLICY BASELINE").unwrap();
for marker in [
"repository-specific focus",
"repository-specific guardrail",
"repository-specific policy",
"repository-specific tone",
] {
let variable = p.find(marker).unwrap();
assert!(contract < variable, "contract followed {marker}");
assert!(baseline < variable, "baseline followed {marker}");
}
assert!(focus < guardrail && guardrail < policy && policy < tone && tone < contract);
assert!(p.ends_with(
"\nRespond with ONLY a JSON object, no markdown fences, no prose:\n\
{\"summary\": \"1-3 sentences on merge-relevant risk, or empty string if none\",\n \
\"findings\": [{\"path\": \"file path from the diff\", \"line\": <new-file line>,\n \
\"endLine\": <optional>, \"severity\": \"info|warn|error\",\n \
\"kind\": \"risk|humanEscalation|guardrail|uncertainty|contentPolicy\", \"confidence\": <0..1>,\n \
\"title\": \"short imperative title\", \"body\": \"specific, evidence-based markdown\"}]}\n\
\n\
The summary and findings must agree. Every risk the summary mentions MUST appear as \
a structured finding with its diff line; if findings is empty, summary MUST be the \
empty string. A summary that narrates problems alongside an empty findings array is \
invalid output and will fail the review.\n"
));
}

#[test]
Expand All @@ -351,7 +320,7 @@ mod tests {
incremental: false,
content_policy: true,
};
let p = user_prompt(&ctx, "DIFF", 5, false);
let p = user_prompt(&ctx, "DIFF", 5);
assert!(p.contains(".postil/pr-description"));
assert!(p.contains(" 1 Add feature"));
assert!(p.contains("MUST use the path `.postil/pr-description`"));
Expand All @@ -366,7 +335,7 @@ mod tests {
incremental: false,
content_policy: false,
};
let p = user_prompt(&ctx, "DIFF", 5, false);
let p = user_prompt(&ctx, "DIFF", 5);
assert!(!p.contains(".postil/pr-description"));
assert!(p.contains("PR title: Add feature"));
}
Expand All @@ -380,29 +349,9 @@ mod tests {
incremental: true,
content_policy: false,
};
let p = user_prompt(&ctx, "DIFF", 5, false);
let p = user_prompt(&ctx, "DIFF", 5);
assert!(p.contains("INCREMENTAL"));
assert!(p.contains("at most 5 findings"));
assert!(p.ends_with("DIFF"));
}

#[test]
fn user_prompt_places_instructions_before_metadata_and_diff_last() {
let ctx = PrContext {
repo: Some("variable/repository"),
title: Some("Variable title"),
body: Some("Variable body"),
incremental: false,
content_policy: true,
};
let p = user_prompt(&ctx, "VARIABLE DIFF", 7, true);
let instructions = p.find("Report at most 7 findings").unwrap();
let truncation = p.find("diff is truncated").unwrap();
let metadata = p.find("Repository: variable/repository").unwrap();
let diff = p.find("VARIABLE DIFF").unwrap();
assert!(instructions < metadata);
assert!(truncation < metadata);
assert!(metadata < diff);
assert!(p.ends_with("VARIABLE DIFF"));
}
}
8 changes: 7 additions & 1 deletion src/review.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,13 @@ async fn review_diff(cfg: &Config, args: &ReviewArgs, input: ReviewInput<'_>) ->
content_policy: content_policy_active,
};
let system = prompt::system_prompt(cfg);
let user = prompt::user_prompt(&ctx, &annotated, cfg.max_findings, truncated);
let mut user = prompt::user_prompt(&ctx, &annotated, cfg.max_findings);
if truncated {
user.push_str(
"\n\n[NOTE: the diff was truncated at the size limit; review only what \
is shown above.]",
);
}
let client = LlmClient::from_env(cfg)?;
match client.review(cfg, &system, &user).await {
Ok(model_review) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ async fn local_review_reports_grounded_finding_and_gates() {

let requests = server.received_requests().await.unwrap();
let request: Value = requests[0].body_json().unwrap();
assert_eq!(request["max_tokens"], 4096);
assert_eq!(request["max_tokens"], 16384);
assert_eq!(request["messages"].as_array().unwrap().len(), 2);
}

Expand Down
Loading