diff --git a/lib/crates/fabro-workflow/src/pipeline/mod.rs b/lib/crates/fabro-workflow/src/pipeline/mod.rs index 284c32a1f..424dd69bd 100644 --- a/lib/crates/fabro-workflow/src/pipeline/mod.rs +++ b/lib/crates/fabro-workflow/src/pipeline/mod.rs @@ -19,7 +19,8 @@ pub use initialize::initialize; pub use parse::parse; pub(crate) use persist::persist; pub use pull_request::{ - AutoMergeOptions, OpenPullRequestRequest, build_pr_body, maybe_open_pull_request, pull_request, + AutoMergeOptions, OpenPullRequestRequest, PrContent, build_pr_content, maybe_open_pull_request, + pull_request, }; pub use retro::{retro, run_retro}; pub use transform::transform; diff --git a/lib/crates/fabro-workflow/src/pipeline/prompts/pr_body.md b/lib/crates/fabro-workflow/src/pipeline/prompts/pr_body.md new file mode 100644 index 000000000..9175e62db --- /dev/null +++ b/lib/crates/fabro-workflow/src/pipeline/prompts/pr_body.md @@ -0,0 +1,54 @@ +You are writing a pull request title and description for a code change produced by an AI workflow. + +OUTPUT FORMAT +Return a JSON object with exactly two fields: +- "title": a one-line title, max 72 characters, no trailing period. +- "body": the markdown body as described below. + +DO NOT INCLUDE in the body +- A `#` or `##` title heading at the top -- the title goes in the `title` field. +- A "Retro" section, "Fabro Details" section, cost/duration table, or "Generated with" footer -- those are appended programmatically after your output. +- The full plan text -- the full plan is appended programmatically as a
block. +- Bare `#1`, `#2` list prefixes -- GitHub auto-links those as issue references. Use plain `1.`, `2.` instead. +- A test plan unless the testing approach is non-obvious. + +SIZE THE BODY TO THE CHANGE +First classify along two axes from the diff: +- Size: how many files changed, how large the diff is. +- Complexity: trivial (rename / typo / dep bump / config) vs. design decisions / new patterns / cross-cutting concerns. + +Then write at the matching depth: + +| Profile | Body shape | +|---|---| +| Small + simple (typo, config, dep bump) | 1-2 sentences, no headers, total under ~300 characters | +| Small + non-trivial (targeted bugfix, behavioral change) | Short "Problem / Fix" narrative, 3-5 sentences. No headers unless two distinct concerns. | +| Medium feature or refactor | Summary paragraph, then a section explaining what changed and why. Call out design decisions. | +| Large or architecturally significant | Full narrative: problem context, approach chosen (and why), key decisions, migration/rollback notes if relevant. | +| Performance improvement | Include before/after measurements if available. A markdown table works well here. | + +Brevity matters for small changes. A 3-line bugfix with a 20-line description signals miscalibration. When in doubt, shorter is better -- reviewers can read the diff. + +WRITING PRINCIPLES +- Lead with value: the first sentence tells the reviewer *why this PR exists*, not *what files changed*. +- Describe the net result, not the journey: skip intermediate failures, debugging steps, and refactors done during development. +- Trust the final diff: if the goal or plan disagree with the diff, the diff is authoritative. +- Explain the non-obvious: spend description space on what the diff doesn't show -- why this approach, what was rejected, what to look at first. +- Use structure when it earns its keep: no empty sections, no template headers without content. +- If the body uses any `##` heading, the opening summary must also be under a heading (e.g. `## Summary`); otherwise a bare paragraph is fine. + +PLAN SUMMARY +The full plan is attached separately as a
block, so do not restate it. Include a brief `### Plan Summary` with bullet points only when the change is medium or larger in the sizing matrix above. Skip it for small changes. + +VISUAL AIDS +Include a visual aid only when a reviewer would struggle to reconstruct the mental model from prose alone -- based on what changes structurally, not on PR size. Skip for trivial / mechanical changes, or when prose already communicates clearly. + +| PR changes... | Visual aid | +|---|---| +| 3+ interacting components or services | Mermaid component / interaction diagram | +| Multi-step workflow or pipeline with non-obvious sequencing | Mermaid flow diagram | +| 3+ behavioral modes or variants | Markdown comparison table | +| Before/after data or trade-offs | Markdown table | +| Data model changes with 3+ related entities | Mermaid ERD | + +Mermaid: prefer `TB` direction, <=10 nodes typical. Place inline at the point of relevance, not in a separate "Diagrams" section. diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs index 8825f8eef..35f27aa92 100644 --- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs @@ -11,7 +11,7 @@ use fabro_store::RunProjection; use fabro_types::PullRequestRecord; use fabro_types::settings::run::MergeStrategy; use fabro_util::text::strip_goal_decoration; -use tracing::{debug, info}; +use tracing::{debug, info, warn}; use super::types::{Concluded, Finalized, PullRequestOptions}; use crate::event::{Event, RunNoticeLevel}; @@ -19,130 +19,75 @@ use crate::outcome::{StageOutcome, format_cost as outcome_format_cost}; use crate::records::{Conclusion, RunSpec}; use crate::runtime_store::RunStoreHandle; -/// Maximum length of a PR title (Unicode scalar values). Single source of -/// truth — referenced by the structured-output schema, the system prompt, -/// and [`enforce_title_cap`]. +/// Maximum length of a PR title (Unicode scalar values). const PR_TITLE_MAX_CHARS: usize = 72; /// Structured output schema for the LLM-generated PR title and body. -/// -/// `title` is required but allows empty strings (the only signal that -/// triggers the deterministic title fallback in -/// [`maybe_open_pull_request`]). `body` requires `minLength: 1` because -/// there is no body fallback — an empty body is fatal. static PR_CONTENT_SCHEMA: LazyLock = LazyLock::new(|| { serde_json::json!({ "type": "object", "properties": { - "title": { "type": "string", "maxLength": PR_TITLE_MAX_CHARS }, - "body": { "type": "string", "minLength": 1 } + "title": { "type": "string" }, + "body": { "type": "string" } }, "required": ["title", "body"], "additionalProperties": false }) }); +/// Complete pull request content generated for a workflow run. #[derive(Debug, serde::Deserialize)] -struct GeneratedPrContent { - title: String, - body: String, +pub struct PrContent { + pub title: String, + pub body: String, } /// System prompt that instructs the LLM how to write a Fabro PR title and /// body. The trailing programmatic sections (Plan `
`, Retro, /// Fabro Details, footer) are appended after the LLM body — the prompt /// explicitly forbids the LLM from duplicating them. -// -// The "max 72 characters" instruction must stay in sync with -// `PR_TITLE_MAX_CHARS` and the schema above; the prompt is advisory and -// `enforce_title_cap` is the actual enforcement. -const PR_BODY_SYSTEM_PROMPT: &str = "You are writing a pull request title and description for a code change produced by an AI workflow. +const PR_BODY_SYSTEM_PROMPT: &str = include_str!("prompts/pr_body.md"); -OUTPUT FORMAT -Return a JSON object with exactly two fields: -- \"title\": a one-line title, max 72 characters, no trailing period. -- \"body\": the markdown body as described below. +const DEFAULT_PR_TITLE: &str = "Update workflow output"; +const EMPTY_BODY_NOTICE: &str = "> _The LLM did not produce a description for this change. The diff and the appended details are the source of truth for review._"; -DO NOT INCLUDE in the body -- A `#` or `##` title heading at the top — the title goes in the `title` field. -- A \"Retro\" section, \"Fabro Details\" section, cost/duration table, or \"Generated with\" footer — those are appended programmatically after your output. -- The full plan text — the full plan is appended programmatically as a
block. -- Bare `#1`, `#2` list prefixes — GitHub auto-links those as issue references. Use plain `1.`, `2.` instead. -- A test plan unless the testing approach is non-obvious. - -SIZE THE BODY TO THE CHANGE -First classify along two axes from the diff: -- Size: how many files changed, how large the diff is. -- Complexity: trivial (rename / typo / dep bump / config) vs. design decisions / new patterns / cross-cutting concerns. - -Then write at the matching depth: - -| Profile | Body shape | -|---|---| -| Small + simple (typo, config, dep bump) | 1–2 sentences, no headers, total under ~300 characters | -| Small + non-trivial (targeted bugfix, behavioral change) | Short \"Problem / Fix\" narrative, 3–5 sentences. No headers unless two distinct concerns. | -| Medium feature or refactor | Summary paragraph, then a section explaining what changed and why. Call out design decisions. | -| Large or architecturally significant | Full narrative: problem context, approach chosen (and why), key decisions, migration/rollback notes if relevant. | -| Performance improvement | Include before/after measurements if available. A markdown table works well here. | - -Brevity matters for small changes. A 3-line bugfix with a 20-line description signals miscalibration. When in doubt, shorter is better — reviewers can read the diff. - -WRITING PRINCIPLES -- Lead with value: the first sentence tells the reviewer *why this PR exists*, not *what files changed*. -- Describe the net result, not the journey: skip intermediate failures, debugging steps, and refactors done during development. -- Trust the final diff: if the goal or plan disagree with the diff, the diff is authoritative. -- Explain the non-obvious: spend description space on what the diff doesn't show — why this approach, what was rejected, what to look at first. -- Use structure when it earns its keep: no empty sections, no template headers without content. -- If the body uses any `##` heading, the opening summary must also be under a heading (e.g. `## Summary`); otherwise a bare paragraph is fine. - -PLAN SUMMARY -The full plan is attached separately as a
block, so do not restate it. Include a brief `### Plan Summary` with bullet points only when the change is medium or larger in the sizing matrix above. Skip it for small changes. - -VISUAL AIDS -Include a visual aid only when a reviewer would struggle to reconstruct the mental model from prose alone — based on what changes structurally, not on PR size. Skip for trivial / mechanical changes, or when prose already communicates clearly. - -| PR changes... | Visual aid | -|---|---| -| 3+ interacting components or services | Mermaid component / interaction diagram | -| Multi-step workflow or pipeline with non-obvious sequencing | Mermaid flow diagram | -| 3+ behavioral modes or variants | Markdown comparison table | -| Before/after data or trade-offs | Markdown table | -| Data model changes with 3+ related entities | Mermaid ERD | - -Mermaid: prefer `TB` direction, ≤10 nodes typical. Place inline at the point of relevance, not in a separate \"Diagrams\" section."; - -/// Truncation budget for the LLM prompt's goal / plan / diff sections. +/// Truncation budget for the LLM prompt's plan / diff sections. +#[derive(Debug, PartialEq, Eq)] struct TruncationCaps { - goal: usize, plan: usize, diff: usize, } -/// Generous tier for models with ≥200k context windows. -const TRUNCATION_LARGE: TruncationCaps = TruncationCaps { - goal: 75_000, - plan: 75_000, - diff: 250_000, -}; - -/// Conservative tier (matches the pre-refactor values). Used for smaller -/// or unknown models. -const TRUNCATION_SMALL: TruncationCaps = TruncationCaps { - goal: 20_000, - plan: 20_000, - diff: 50_000, -}; +const DIFF_HARD_CAP: usize = 500_000; +const PLAN_HARD_CAP: usize = 100_000; +const DIFF_FRACTION_NUM: usize = 4; +const PLAN_FRACTION_NUM: usize = 1; +const FRACTION_DEN: usize = 10; +const UNKNOWN_MODEL_CTX: usize = 200_000; /// Resolve truncation caps based on the model's context window. Unknown -/// models fall through to the conservative tier. -fn truncation_caps(model: &str) -> &'static TruncationCaps { - let large_enough = Catalog::builtin() +/// models use the baseline 200k context-window assumption. +fn truncation_caps(model: &str) -> TruncationCaps { + let ctx = Catalog::builtin() .get(model) - .is_some_and(|m| m.context_window() >= 200_000); - if large_enough { - &TRUNCATION_LARGE - } else { - &TRUNCATION_SMALL + .and_then(|m| usize::try_from(m.context_window()).ok()) + .unwrap_or(UNKNOWN_MODEL_CTX); + + truncation_caps_for_context_window(ctx) +} + +fn truncation_caps_for_context_window(ctx: usize) -> TruncationCaps { + TruncationCaps { + diff: ctx + .saturating_mul(DIFF_FRACTION_NUM) + .checked_div(FRACTION_DEN) + .unwrap_or(DIFF_HARD_CAP) + .min(DIFF_HARD_CAP), + plan: ctx + .saturating_mul(PLAN_FRACTION_NUM) + .checked_div(FRACTION_DEN) + .unwrap_or(PLAN_HARD_CAP) + .min(PLAN_HARD_CAP), } } @@ -172,12 +117,18 @@ fn enforce_title_cap(title: &str) -> String { /// Derive a PR title from the workflow goal. /// -/// Uses the first line, truncated to 120 characters for readability. The -/// caller is expected to apply [`enforce_title_cap`] afterwards if a -/// stricter cap is required (the wider cap here is the legacy behaviour -/// for the deterministic fallback path). +/// Uses the first line, truncated to the same cap as LLM-generated titles. fn pr_title_from_goal(goal: &str) -> String { - truncate_with_ellipsis(strip_goal_decoration(goal), 120) + truncate_with_ellipsis(strip_goal_decoration(goal), PR_TITLE_MAX_CHARS) +} + +fn fallback_pr_title(goal: &str) -> String { + let title = pr_title_from_goal(goal); + if title.trim().is_empty() { + DEFAULT_PR_TITLE.to_string() + } else { + title + } } /// Truncate a PR body to fit GitHub's 65,536 character limit. @@ -435,75 +386,43 @@ async fn load_pull_request_diff(run_store: &RunStoreHandle) -> String { .unwrap_or_default() } -/// Build a complete PR title and body by combining LLM-generated narrative -/// with programmatic sections (plan, retro, fabro details). -/// -/// Returns `(title, body)`. The title may be the empty string when the LLM -/// returned a usable body but no usable title — callers fall back to -/// [`pr_title_from_goal`] in that case. Every other generation failure is -/// surfaced as `Err`. -pub async fn build_pr_body( +/// Build complete PR content by combining LLM-generated narrative with +/// deterministic fallbacks and programmatic sections. +pub async fn build_pr_content( diff: &str, goal: &str, model: &str, run_store: &RunStoreHandle, llm_source: &dyn CredentialSource, conclusion: Option<&Conclusion>, -) -> Result<(String, String), String> { + run_state: Option<&RunProjection>, +) -> Result { let client = Client::from_source(llm_source) .await .map_err(|e| format!("Failed to create LLM client: {e}"))?; - build_pr_body_with_client(diff, goal, model, run_store, conclusion, Arc::new(client)).await -} - -async fn build_pr_body_with_client( - diff: &str, - goal: &str, - model: &str, - run_store: &RunStoreHandle, - conclusion: Option<&Conclusion>, - client: Arc, -) -> Result<(String, String), String> { - build_pr_body_with_client_and_state(diff, goal, model, run_store, conclusion, client, None) - .await -} - -async fn build_pr_body_with_source_and_state( - diff: &str, - goal: &str, - model: &str, - run_store: &RunStoreHandle, - llm_source: &dyn CredentialSource, - conclusion: Option<&Conclusion>, - run_state: Option<&fabro_store::RunProjection>, -) -> Result<(String, String), String> { - let client = Client::from_source(llm_source) - .await - .map_err(|e| format!("Failed to create LLM client: {e}"))?; - - build_pr_body_with_client_and_state( + build_pr_content_with_client( diff, goal, model, run_store, conclusion, - Arc::new(client), run_state, + Arc::new(client), ) .await } -async fn build_pr_body_with_client_and_state( +async fn build_pr_content_with_client( diff: &str, goal: &str, model: &str, run_store: &RunStoreHandle, conclusion: Option<&Conclusion>, + run_state: Option<&RunProjection>, client: Arc, - run_state: Option<&fabro_store::RunProjection>, -) -> Result<(String, String), String> { - info!("Building PR body"); +) -> Result { + info!("Building PR content"); let loaded_run_state = if run_state.is_none() { run_store @@ -524,16 +443,15 @@ async fn build_pr_body_with_client_and_state( let dot_source = run_state.and_then(|state| state.graph_source.clone()); let caps = truncation_caps(model); - let truncated_goal = truncate_chars(goal, caps.goal); let truncated_diff = truncate_chars(diff, caps.diff); let prompt = if let Some(ref plan) = plan_text { let truncated_plan = truncate_chars(plan, caps.plan); format!( - "Goal: {truncated_goal}\n\nPlan:\n```\n{truncated_plan}\n```\n\nDiff:\n```\n{truncated_diff}\n```" + "Goal: {goal}\n\nPlan:\n```\n{truncated_plan}\n```\n\nDiff:\n```\n{truncated_diff}\n```" ) } else { - format!("Goal: {truncated_goal}\n\nDiff:\n```\n{truncated_diff}\n```") + format!("Goal: {goal}\n\nDiff:\n```\n{truncated_diff}\n```") }; let params = GenerateParams::new(model, client) @@ -547,15 +465,22 @@ async fn build_pr_body_with_client_and_state( let output = result .output .ok_or_else(|| "LLM generation returned no structured output".to_string())?; - let generated: GeneratedPrContent = serde_json::from_value(output) + let generated: PrContent = serde_json::from_value(output) .map_err(|e| format!("Failed to deserialize PR content: {e}"))?; - if generated.body.trim().is_empty() { - return Err("LLM generated an empty PR body".to_string()); - } + let title = if generated.title.trim().is_empty() { + fallback_pr_title(goal) + } else { + generated.title.trim().to_string() + }; + let title = enforce_title_cap(&title); - let title = enforce_title_cap(generated.title.trim()); - let llm_body = generated.body; + let llm_body = if generated.body.trim().is_empty() { + warn!(model = %model, "LLM generated empty PR body; using skeleton PR body"); + EMPTY_BODY_NOTICE.to_string() + } else { + generated.body + }; let retro_section = retro.as_ref().map(format_retro_section).unwrap_or_default(); let arc_details_section = conclusion @@ -570,9 +495,9 @@ async fn build_pr_body_with_client_and_state( &arc_details_section, ); - info!("PR body generated"); + info!("PR content generated"); - Ok((title, body)) + Ok(PrContent { title, body }) } /// Auto-merge configuration for a pull request. @@ -594,7 +519,7 @@ pub struct OpenPullRequestRequest<'a> { pub run_store: &'a RunStoreHandle, pub llm_source: &'a dyn CredentialSource, pub conclusion: Option<&'a Conclusion>, - pub run_state: Option<&'a fabro_store::RunProjection>, + pub run_state: Option<&'a RunProjection>, } /// Optionally open a pull request after a successful workflow run. @@ -613,7 +538,7 @@ pub async fn maybe_open_pull_request( let (owner, repo) = github_app::parse_github_owner_repo(&https_url).map_err(|err| format!("{err:#}"))?; - let (llm_title, body) = build_pr_body_with_source_and_state( + let content = build_pr_content( req.diff, req.goal, req.model, @@ -624,14 +549,8 @@ pub async fn maybe_open_pull_request( ) .await .map_err(|err| format!("{err:#}"))?; - let body = truncate_pr_body(&body); - - let title = if llm_title.is_empty() { - pr_title_from_goal(req.goal) - } else { - llm_title - }; - let title = enforce_title_cap(&title); + let body = truncate_pr_body(&content.body); + let title = content.title; let created = github_app::create_pull_request( &req.github, @@ -1286,15 +1205,16 @@ mod tests { } #[tokio::test] - async fn build_pr_body_uses_in_memory_conclusion() { + async fn build_pr_content_uses_in_memory_conclusion() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let (title, body) = build_pr_body_with_client( + let PrContent { title, body } = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client( "mock", &pr_content_json("Mock title", "Narrative from mock."), @@ -1311,7 +1231,7 @@ mod tests { } #[tokio::test] - async fn build_pr_body_uses_store_records_without_legacy_files() { + async fn build_pr_content_uses_store_records_without_legacy_files() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); @@ -1363,19 +1283,21 @@ mod tests { .await .unwrap(); - let (_, body) = build_pr_body_with_client( + let body = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client( "mock", &pr_content_json("Mock title", "Narrative from mock."), ), ) .await - .unwrap(); + .unwrap() + .body; assert!(body.contains("Narrative from mock.")); assert!(body.contains("### Retro")); @@ -1384,7 +1306,7 @@ mod tests { } #[tokio::test] - async fn build_pr_body_uses_plan_text_from_store_without_response_md() { + async fn build_pr_content_uses_plan_text_from_store_without_response_md() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); @@ -1453,48 +1375,52 @@ mod tests { .await .unwrap(); - let (_, body) = build_pr_body_with_client( + let body = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client( "mock", &pr_content_json("Mock title", "Narrative from mock."), ), ) .await - .unwrap(); + .unwrap() + .body; assert!(body.contains("Full plan")); assert!(body.contains("Plan from store")); } #[tokio::test] - async fn build_pr_body_uses_explicit_llm_client() { + async fn build_pr_content_uses_explicit_llm_client() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let (_, body) = build_pr_body_with_client( + let body = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "gpt-5.4", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client( "openai", &pr_content_json("Explicit title", "Narrative from explicit client."), ), ) .await - .unwrap(); + .unwrap() + .body; assert!(body.contains("Narrative from explicit client.")); assert!(!body.contains("Narrative from mock.")); } #[tokio::test] - async fn build_pr_body_uses_vault_only_openai_codex_source() { + async fn build_pr_content_uses_vault_only_openai_codex_source() { let server = MockServer::start_async().await; let response_mock = server .mock_async(|when, then| { @@ -1534,13 +1460,14 @@ mod tests { let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); let run_store_handle: RunStoreHandle = run_store.into(); - let (title, body) = build_pr_body( + let PrContent { title, body } = build_pr_content( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "gpt-5.4", &run_store_handle, llm_source.as_ref(), Some(&make_test_conclusion()), + None, ) .await .unwrap(); @@ -1642,7 +1569,7 @@ mod tests { fn pr_title_truncates_long_line() { let long = "x".repeat(300); let title = pr_title_from_goal(&long); - assert_eq!(title.chars().count(), 120); + assert_eq!(title.chars().count(), 72); assert!(title.ends_with('…')); } @@ -1665,6 +1592,42 @@ mod tests { assert_eq!(pr_title_from_goal("Fix bug"), "Fix bug"); } + #[test] + fn truncation_caps_scale_with_context_window_and_clamp() { + assert_eq!( + truncation_caps_for_context_window(100_000), + TruncationCaps { + diff: 40_000, + plan: 10_000, + } + ); + assert_eq!( + truncation_caps_for_context_window(200_000), + TruncationCaps { + diff: 80_000, + plan: 20_000, + } + ); + assert_eq!( + truncation_caps_for_context_window(1_000_000), + TruncationCaps { + diff: 400_000, + plan: 100_000, + } + ); + assert_eq!( + truncation_caps_for_context_window(10_000_000), + TruncationCaps { + diff: 500_000, + plan: 100_000, + } + ); + assert_eq!(truncation_caps("unknown-model"), TruncationCaps { + diff: 80_000, + plan: 20_000, + }); + } + #[tokio::test] async fn empty_diff_returns_none() { let store = test_store(); @@ -1760,68 +1723,144 @@ mod tests { /// MockProvider returns an over-long title; builder must cap it at 72 /// chars and end with `…`. Exercises [`enforce_title_cap`] inside - /// [`build_pr_body_with_client_and_state`]. + /// [`build_pr_content_with_client`]. #[tokio::test] - async fn build_pr_body_truncates_long_title() { + async fn build_pr_content_truncates_long_title() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); let long_title = "x".repeat(200); let payload = pr_content_json(&long_title, "Body content."); - let (title, _) = build_pr_body_with_client( + let title = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client("mock", &payload), ) .await - .unwrap(); + .unwrap() + .title; assert_eq!(title.chars().count(), 72); assert!(title.ends_with('\u{2026}')); } - /// Empty bodies are fatal. Real providers may reject this via the - /// schema's `minLength`; the Rust-side trim check also catches it for - /// local/mock providers. #[tokio::test] - async fn build_pr_body_returns_err_when_body_empty() { + async fn build_pr_content_uses_default_title_when_generated_and_goal_titles_empty() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); - let payload = pr_content_json("Mock", ""); - let result = build_pr_body_with_client( + let payload = pr_content_json("", "Body content."); + let title = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n", - "Implement feature", + "## Plan:", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client("mock", &payload), ) - .await; + .await + .unwrap() + .title; - assert!(result.is_err(), "expected Err, got {result:?}"); + assert_eq!(title, DEFAULT_PR_TITLE); } - /// Whitespace-only bodies pass schema validation but fail the - /// `body.trim().is_empty()` check inside the builder. + /// Empty or whitespace-only bodies use the skeleton fallback instead of + /// aborting PR creation. #[tokio::test] - async fn build_pr_body_returns_err_when_body_whitespace() { + async fn build_pr_content_uses_skeleton_when_body_empty() { let store = test_store(); let run_store = store.create_run(&fixtures::RUN_1).await.unwrap(); + + let run_spec = RunSpec { + run_id: fixtures::RUN_1, + settings: fabro_types::WorkflowSettings::default(), + graph: Graph::new("test"), + workflow_slug: Some("test".to_string()), + source_directory: Some("/tmp/project".to_string()), + git: None, + labels: HashMap::new(), + provenance: None, + manifest_blob: None, + definition_blob: None, + fork_source_ref: None, + in_place: false, + }; + append_event(&run_store, &fixtures::RUN_1, &Event::RunCreated { + run_id: fixtures::RUN_1, + settings: serde_json::to_value(&run_spec.settings).unwrap(), + graph: serde_json::to_value(&run_spec.graph).unwrap(), + workflow_source: Some("digraph test { plan -> code }".to_string()), + workflow_config: None, + labels: run_spec.labels.clone().into_iter().collect(), + run_dir: "/tmp/project".to_string(), + source_directory: run_spec.source_directory.clone(), + workflow_slug: run_spec.workflow_slug.clone(), + db_prefix: None, + provenance: None, + manifest_blob: None, + git: None, + fork_source_ref: None, + in_place: false, + web_url: None, + }) + .await + .unwrap(); + append_event(&run_store, &fixtures::RUN_1, &Event::StageCompleted { + node_id: "plan".to_string(), + name: "plan".to_string(), + index: 0, + duration_ms: 1, + status: "succeeded".to_string(), + preferred_label: None, + suggested_next_ids: vec![], + billing: None, + failure: None, + notes: None, + files_touched: vec![], + context_updates: None, + jump_to_node: None, + context_values: None, + node_visits: None, + loop_failure_signatures: None, + restart_failure_signatures: None, + response: Some("Plan from store".to_string()), + attempt: 1, + max_attempts: 1, + }) + .await + .unwrap(); + append_event(&run_store, &fixtures::RUN_1, &Event::RetroCompleted { + duration_ms: 1, + response: Some(String::new()), + retro: Some(serde_json::to_value(make_test_retro()).unwrap()), + }) + .await + .unwrap(); + let payload = pr_content_json("Mock", " \n"); - let result = build_pr_body_with_client( + let body = build_pr_content_with_client( "diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n", "Implement feature", "mock-model", &run_store.clone().into(), Some(&make_test_conclusion()), + None, explicit_client("mock", &payload), ) - .await; + .await + .unwrap() + .body; - let err = result.expect_err("expected Err for whitespace-only body"); - assert!(err.contains("empty PR body"), "unexpected error: {err}"); + assert!(body.contains("The LLM did not produce a description")); + assert!(body.contains("Full plan")); + assert!(body.contains("Plan from store")); + assert!(body.contains("### Retro")); + assert!(body.contains("### Fabro Details")); + assert!(body.contains("Generated with [Fabro](https://fabro.sh)")); } // ── maybe_open_pull_request fallback tests ────────────────────────── @@ -1978,9 +2017,9 @@ mod tests { } } - /// LLM returns a usable body but an empty title; `maybe_open_pull_request` - /// must fall back to `pr_title_from_goal` (first line, decoration - /// stripped) and the PR creation must succeed with that title. + /// LLM returns a usable body but an empty title; the content builder + /// falls back to `pr_title_from_goal` (first line, decoration stripped) + /// and PR creation succeeds with that title. #[tokio::test] async fn maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title() { let payload = pr_content_json("", "Narrative."); @@ -2012,10 +2051,8 @@ mod tests { harness.assert_mocks_called_once().await; } - /// LLM returns an empty title; the fallback path produces a long title - /// (close to `pr_title_from_goal`'s 120-char cap), and the unconditional - /// `enforce_title_cap` in `maybe_open_pull_request` must still bring it - /// down to 72 chars ending with `…`. + /// LLM returns an empty title; the content builder fallback still caps + /// the deterministic goal title at 72 chars ending with `…`. #[tokio::test] async fn maybe_open_pull_request_caps_fallback_title_at_72_chars() { let payload = pr_content_json("", "Narrative."); diff --git a/lib/crates/fabro-workflow/src/pull_request.rs b/lib/crates/fabro-workflow/src/pull_request.rs index 3989bf63c..3b65dfcbc 100644 --- a/lib/crates/fabro-workflow/src/pull_request.rs +++ b/lib/crates/fabro-workflow/src/pull_request.rs @@ -1,4 +1,4 @@ pub use crate::pipeline::{ - AutoMergeOptions, OpenPullRequestRequest, PullRequestRecord, build_pr_body, + AutoMergeOptions, OpenPullRequestRequest, PrContent, PullRequestRecord, build_pr_content, maybe_open_pull_request, }; diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs index 742218c14..f6ece331b 100644 --- a/lib/crates/fabro-workflow/tests/it/integration.rs +++ b/lib/crates/fabro-workflow/tests/it/integration.rs @@ -6896,7 +6896,7 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { let run_store = store.open_run_reader(&run_options.run_id).await.unwrap(); let run_store_handle: fabro_workflow::runtime_store::RunStoreHandle = run_store.into(); - let (title, body) = fabro_workflow::pull_request::build_pr_body( + let content = fabro_workflow::pull_request::build_pr_content( "diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n", "Implement feature", "gpt-5.4", @@ -6912,12 +6912,13 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() { billing: None, total_retries: 0, }), + None, ) .await .expect("PR body should build from vault-only credentials"); - assert_eq!(title, "Vault title"); - assert!(body.contains("Narrative from vault source.")); + assert_eq!(content.title, "Vault title"); + assert!(content.body.contains("Narrative from vault source.")); response_mock.assert_async().await; }