mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-06 08:18:58 +00:00
parent
4aa8f51b64
commit
145e0b19b0
4 changed files with 667 additions and 67 deletions
367
run.json
367
run.json
File diff suppressed because one or more lines are too long
356
stages/007-simplify_gpt@1/diff.patch
Normal file
356
stages/007-simplify_gpt@1/diff.patch
Normal file
|
|
@ -0,0 +1,356 @@
|
|||
diff --git a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs
|
||||
index cd84af8b..8825f8ee 100644
|
||||
--- a/lib/crates/fabro-workflow/src/pipeline/pull_request.rs
|
||||
+++ b/lib/crates/fabro-workflow/src/pipeline/pull_request.rs
|
||||
@@ -48,18 +48,6 @@ struct GeneratedPrContent {
|
||||
body: String,
|
||||
}
|
||||
|
||||
-/// LLM-derived PR title and the fully assembled body (LLM narrative plus
|
||||
-/// programmatic Plan / Retro / Fabro Details / footer sections).
|
||||
-///
|
||||
-/// `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`.
|
||||
-#[derive(Debug, Clone)]
|
||||
-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 `<details>`, Retro,
|
||||
/// Fabro Details, footer) are appended after the LLM body — the prompt
|
||||
@@ -158,16 +146,12 @@ fn truncation_caps(model: &str) -> &'static TruncationCaps {
|
||||
}
|
||||
}
|
||||
|
||||
-/// Truncate `s` to at most `max` bytes, aligned to a UTF-8 char boundary
|
||||
-/// (so the returned slice never splits a multibyte sequence). The cap is
|
||||
-/// in bytes for cheap context-window safety; for ASCII-heavy diffs and
|
||||
-/// goals this matches a char count.
|
||||
+/// Truncate `s` to at most `max` Unicode scalar values without splitting a
|
||||
+/// UTF-8 sequence.
|
||||
fn truncate_chars(s: &str, max: usize) -> &str {
|
||||
- if s.len() > max {
|
||||
- &s[..s.floor_char_boundary(max)]
|
||||
- } else {
|
||||
- s
|
||||
- }
|
||||
+ s.char_indices()
|
||||
+ .nth(max)
|
||||
+ .map_or(s, |(boundary, _)| &s[..boundary])
|
||||
}
|
||||
|
||||
/// Truncate `s` to at most `max` Unicode scalar values, replacing the
|
||||
@@ -454,7 +438,10 @@ async fn load_pull_request_diff(run_store: &RunStoreHandle) -> String {
|
||||
/// Build a complete PR title and body by combining LLM-generated narrative
|
||||
/// with programmatic sections (plan, retro, fabro details).
|
||||
///
|
||||
-/// See [`PrContent`] for the empty-title-as-fallback-signal contract.
|
||||
+/// 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(
|
||||
diff: &str,
|
||||
goal: &str,
|
||||
@@ -462,7 +449,7 @@ pub async fn build_pr_body(
|
||||
run_store: &RunStoreHandle,
|
||||
llm_source: &dyn CredentialSource,
|
||||
conclusion: Option<&Conclusion>,
|
||||
-) -> Result<PrContent, String> {
|
||||
+) -> Result<(String, String), String> {
|
||||
let client = Client::from_source(llm_source)
|
||||
.await
|
||||
.map_err(|e| format!("Failed to create LLM client: {e}"))?;
|
||||
@@ -477,7 +464,7 @@ async fn build_pr_body_with_client(
|
||||
run_store: &RunStoreHandle,
|
||||
conclusion: Option<&Conclusion>,
|
||||
client: Arc<Client>,
|
||||
-) -> Result<PrContent, String> {
|
||||
+) -> Result<(String, String), String> {
|
||||
build_pr_body_with_client_and_state(diff, goal, model, run_store, conclusion, client, None)
|
||||
.await
|
||||
}
|
||||
@@ -490,7 +477,7 @@ async fn build_pr_body_with_source_and_state(
|
||||
llm_source: &dyn CredentialSource,
|
||||
conclusion: Option<&Conclusion>,
|
||||
run_state: Option<&fabro_store::RunProjection>,
|
||||
-) -> Result<PrContent, String> {
|
||||
+) -> Result<(String, String), String> {
|
||||
let client = Client::from_source(llm_source)
|
||||
.await
|
||||
.map_err(|e| format!("Failed to create LLM client: {e}"))?;
|
||||
@@ -515,7 +502,7 @@ async fn build_pr_body_with_client_and_state(
|
||||
conclusion: Option<&Conclusion>,
|
||||
client: Arc<Client>,
|
||||
run_state: Option<&fabro_store::RunProjection>,
|
||||
-) -> Result<PrContent, String> {
|
||||
+) -> Result<(String, String), String> {
|
||||
info!("Building PR body");
|
||||
|
||||
let loaded_run_state = if run_state.is_none() {
|
||||
@@ -585,7 +572,7 @@ async fn build_pr_body_with_client_and_state(
|
||||
|
||||
info!("PR body generated");
|
||||
|
||||
- Ok(PrContent { title, body })
|
||||
+ Ok((title, body))
|
||||
}
|
||||
|
||||
/// Auto-merge configuration for a pull request.
|
||||
@@ -626,10 +613,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 PrContent {
|
||||
- title: llm_title,
|
||||
- body,
|
||||
- } = build_pr_body_with_source_and_state(
|
||||
+ let (llm_title, body) = build_pr_body_with_source_and_state(
|
||||
req.diff,
|
||||
req.goal,
|
||||
req.model,
|
||||
@@ -642,14 +626,12 @@ pub async fn maybe_open_pull_request(
|
||||
.map_err(|err| format!("{err:#}"))?;
|
||||
let body = truncate_pr_body(&body);
|
||||
|
||||
- // The LLM-title path is already capped inside the builder via
|
||||
- // `enforce_title_cap`; the fallback path uses `pr_title_from_goal`,
|
||||
- // which has a wider 120-char cap and so needs re-capping here.
|
||||
let title = if llm_title.is_empty() {
|
||||
- enforce_title_cap(&pr_title_from_goal(req.goal))
|
||||
+ pr_title_from_goal(req.goal)
|
||||
} else {
|
||||
llm_title
|
||||
};
|
||||
+ let title = enforce_title_cap(&title);
|
||||
|
||||
let created = github_app::create_pull_request(
|
||||
&req.github,
|
||||
@@ -1307,7 +1289,7 @@ mod tests {
|
||||
async fn build_pr_body_uses_in_memory_conclusion() {
|
||||
let store = test_store();
|
||||
let run_store = store.create_run(&fixtures::RUN_1).await.unwrap();
|
||||
- let pr = build_pr_body_with_client(
|
||||
+ let (title, body) = build_pr_body_with_client(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"mock-model",
|
||||
@@ -1321,8 +1303,7 @@ mod tests {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
- assert_eq!(pr.title, "Mock title");
|
||||
- let body = pr.body;
|
||||
+ assert_eq!(title, "Mock title");
|
||||
assert!(body.contains("Narrative from mock."));
|
||||
assert!(body.contains("### Fabro Details"));
|
||||
assert!(body.contains("Ran 3 stages in 2m 30s for $0.42"));
|
||||
@@ -1382,7 +1363,7 @@ mod tests {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
- let body = build_pr_body_with_client(
|
||||
+ let (_, body) = build_pr_body_with_client(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"mock-model",
|
||||
@@ -1394,8 +1375,7 @@ mod tests {
|
||||
),
|
||||
)
|
||||
.await
|
||||
- .unwrap()
|
||||
- .body;
|
||||
+ .unwrap();
|
||||
|
||||
assert!(body.contains("Narrative from mock."));
|
||||
assert!(body.contains("### Retro"));
|
||||
@@ -1473,7 +1453,7 @@ mod tests {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
- let body = build_pr_body_with_client(
|
||||
+ let (_, body) = build_pr_body_with_client(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"mock-model",
|
||||
@@ -1485,8 +1465,7 @@ mod tests {
|
||||
),
|
||||
)
|
||||
.await
|
||||
- .unwrap()
|
||||
- .body;
|
||||
+ .unwrap();
|
||||
|
||||
assert!(body.contains("<summary>Full plan</summary>"));
|
||||
assert!(body.contains("Plan from store"));
|
||||
@@ -1496,7 +1475,7 @@ mod tests {
|
||||
async fn build_pr_body_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_body_with_client(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"gpt-5.4",
|
||||
@@ -1508,8 +1487,7 @@ mod tests {
|
||||
),
|
||||
)
|
||||
.await
|
||||
- .unwrap()
|
||||
- .body;
|
||||
+ .unwrap();
|
||||
|
||||
assert!(body.contains("Narrative from explicit client."));
|
||||
assert!(!body.contains("Narrative from mock."));
|
||||
@@ -1556,7 +1534,7 @@ mod tests {
|
||||
let run_store = store.create_run(&fixtures::RUN_1).await.unwrap();
|
||||
let run_store_handle: RunStoreHandle = run_store.into();
|
||||
|
||||
- let pr = build_pr_body(
|
||||
+ let (title, body) = build_pr_body(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"gpt-5.4",
|
||||
@@ -1567,8 +1545,8 @@ mod tests {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
- assert_eq!(pr.title, "Vault title");
|
||||
- assert!(pr.body.contains("Narrative from vault source."));
|
||||
+ assert_eq!(title, "Vault title");
|
||||
+ assert!(body.contains("Narrative from vault source."));
|
||||
response_mock.assert_async().await;
|
||||
}
|
||||
|
||||
@@ -1789,7 +1767,7 @@ mod tests {
|
||||
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_body_with_client(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn x() {}\n",
|
||||
"Implement feature",
|
||||
"mock-model",
|
||||
@@ -1798,14 +1776,15 @@ mod tests {
|
||||
explicit_client("mock", &payload),
|
||||
)
|
||||
.await
|
||||
- .unwrap()
|
||||
- .title;
|
||||
+ .unwrap();
|
||||
|
||||
assert_eq!(title.chars().count(), 72);
|
||||
assert!(title.ends_with('\u{2026}'));
|
||||
}
|
||||
|
||||
- /// Schema rejects truly empty bodies via `minLength: 1`.
|
||||
+ /// 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() {
|
||||
let store = test_store();
|
||||
@@ -1856,20 +1835,33 @@ mod tests {
|
||||
// Held to keep the mock listener alive for the duration of the test;
|
||||
// the test interacts with it via `Client::from_source` (which goes
|
||||
// out via HTTP to the mock URL stored in `llm_source`).
|
||||
- _openai_server: MockServer,
|
||||
+ openai_server: MockServer,
|
||||
github_server: MockServer,
|
||||
+ openai_mock_id: usize,
|
||||
+ github_mock_id: usize,
|
||||
llm_source: Arc<dyn CredentialSource>,
|
||||
creds: fabro_github::GitHubCredentials,
|
||||
run_store: RunStoreHandle,
|
||||
}
|
||||
|
||||
+ impl FallbackHarness {
|
||||
+ async fn assert_mocks_called_once(&self) {
|
||||
+ httpmock::Mock::new(self.openai_mock_id, &self.openai_server)
|
||||
+ .assert_async()
|
||||
+ .await;
|
||||
+ httpmock::Mock::new(self.github_mock_id, &self.github_server)
|
||||
+ .assert_async()
|
||||
+ .await;
|
||||
+ }
|
||||
+ }
|
||||
+
|
||||
/// Stand up an OpenAI mock that returns the given structured-output
|
||||
/// payload, a GitHub mock that accepts a PR creation, a vault-backed
|
||||
/// credential source, and a run store seeded with a non-empty
|
||||
/// `final_patch`.
|
||||
async fn setup_fallback_test_harness(openai_payload_text: &str) -> FallbackHarness {
|
||||
let openai_server = MockServer::start_async().await;
|
||||
- openai_server
|
||||
+ let openai_mock = openai_server
|
||||
.mock_async(|when, then| {
|
||||
when.method(POST)
|
||||
.path("/v1/responses")
|
||||
@@ -1881,7 +1873,7 @@ mod tests {
|
||||
.await;
|
||||
|
||||
let github_server = MockServer::start_async().await;
|
||||
- github_server
|
||||
+ let github_mock = github_server
|
||||
.mock_async(|when, then| {
|
||||
when.method(POST)
|
||||
.path("/repos/owner/repo/pulls")
|
||||
@@ -1971,10 +1963,15 @@ mod tests {
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
+ let openai_mock_id = openai_mock.id;
|
||||
+ let github_mock_id = github_mock.id;
|
||||
+
|
||||
FallbackHarness {
|
||||
_vault_dir: vault_dir,
|
||||
- _openai_server: openai_server,
|
||||
+ openai_server,
|
||||
github_server,
|
||||
+ openai_mock_id,
|
||||
+ github_mock_id,
|
||||
llm_source,
|
||||
creds,
|
||||
run_store: run_store.into(),
|
||||
@@ -2012,6 +2009,7 @@ mod tests {
|
||||
|
||||
let record = result.expect("PR record should be Some");
|
||||
assert_eq!(record.title, "Fix telemetry leak");
|
||||
+ harness.assert_mocks_called_once().await;
|
||||
}
|
||||
|
||||
/// LLM returns an empty title; the fallback path produces a long title
|
||||
@@ -2050,5 +2048,6 @@ mod tests {
|
||||
let record = result.expect("PR record should be Some");
|
||||
assert_eq!(record.title.chars().count(), 72);
|
||||
assert!(record.title.ends_with('\u{2026}'));
|
||||
+ harness.assert_mocks_called_once().await;
|
||||
}
|
||||
}
|
||||
diff --git a/lib/crates/fabro-workflow/tests/it/integration.rs b/lib/crates/fabro-workflow/tests/it/integration.rs
|
||||
index 2574ee17..742218c1 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 pr = fabro_workflow::pull_request::build_pr_body(
|
||||
+ let (title, body) = fabro_workflow::pull_request::build_pr_body(
|
||||
"diff --git a/src/lib.rs b/src/lib.rs\n+fn new_feature() {}\n",
|
||||
"Implement feature",
|
||||
"gpt-5.4",
|
||||
@@ -6916,8 +6916,8 @@ async fn workflow_run_with_vault_only_openai_codex_builds_pr_body() {
|
||||
.await
|
||||
.expect("PR body should build from vault-only credentials");
|
||||
|
||||
- assert_eq!(pr.title, "Vault title");
|
||||
- assert!(pr.body.contains("Narrative from vault source."));
|
||||
+ assert_eq!(title, "Vault title");
|
||||
+ assert!(body.contains("Narrative from vault source."));
|
||||
response_mock.assert_async().await;
|
||||
}
|
||||
|
||||
6
stages/007-simplify_gpt@1/status.json
Normal file
6
stages/007-simplify_gpt@1/status.json
Normal file
|
|
@ -0,0 +1,6 @@
|
|||
{
|
||||
"outcome": "succeeded",
|
||||
"notes": "Stage completed: simplify_gpt",
|
||||
"failure_reason": null,
|
||||
"timestamp": "2026-05-04T19:15:07.168752Z"
|
||||
}
|
||||
5
stages/008-verify@1/script_invocation.json
Normal file
5
stages/008-verify@1/script_invocation.json
Normal file
|
|
@ -0,0 +1,5 @@
|
|||
{
|
||||
"script": "cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1 && cargo dev docs refresh 2>&1 && cargo dev docs check 2>&1",
|
||||
"command": "cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1 && cargo nextest run --cargo-quiet --workspace --status-level fail 2>&1 && cargo dev docs refresh 2>&1 && cargo dev docs check 2>&1",
|
||||
"language": "shell"
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue