From 145e0b19b0cbf48d26c7697aa0e8d09f29f5b2bd Mon Sep 17 00:00:00 2001 From: Fabro Date: Mon, 4 May 2026 15:17:27 -0400 Subject: [PATCH] =?UTF-8?q?checkpoint=20=E2=9A=92=EF=B8=8F=20Generated=20w?= =?UTF-8?q?ith=20[Fabro](https://fabro.sh)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- run.json | 367 +++++++++++++++++---- stages/007-simplify_gpt@1/diff.patch | 356 ++++++++++++++++++++ stages/007-simplify_gpt@1/status.json | 6 + stages/008-verify@1/script_invocation.json | 5 + 4 files changed, 667 insertions(+), 67 deletions(-) create mode 100644 stages/007-simplify_gpt@1/diff.patch create mode 100644 stages/007-simplify_gpt@1/status.json create mode 100644 stages/008-verify@1/script_invocation.json diff --git a/run.json b/run.json index c72d77da7..f1266d9ae 100644 --- a/run.json +++ b/run.json @@ -505,8 +505,8 @@ "status_updated_at": "2026-05-04T18:34:54.323999Z", "pending_control": null, "checkpoint": { - "timestamp": "2026-05-04T19:15:07.170434Z", - "current_node": "simplify_gpt", + "timestamp": "2026-05-04T19:17:27.604819Z", + "current_node": "verify", "completed_nodes": [ "start", "toolchain", @@ -514,10 +514,12 @@ "preflight_lint", "implement", "simplify_opus", - "simplify_gpt" + "simplify_gpt", + "verify" ], "node_retries": {}, "context_values": { + "internal.retry_count.verify": 0, "outcome": "succeeded", "internal.retry_count.preflight_compile": 0, "response.simplify_opus": "Both clean. \n\n## Summary of cleanups\n\nAggregated findings across the three review agents and fixed the actionable ones:\n\n**Code reuse**\n- **Unified `enforce_title_cap` and `pr_title_from_goal`'s truncation logic** into a shared `truncate_with_ellipsis(s, max) -> String` helper. Eliminates the duplicated `chars().count() > N → take(N-1).collect() + '…'` algorithm. `pr_title_from_goal` is now a one-liner.\n- **Switched from field access to the `m.context_window()` accessor** in `truncation_caps`, matching the convention used by the other 4 catalog-consumer call sites in the workspace (`fabro-agent`, `fabro-llm`, etc.).\n\n**Code quality**\n- **Hoisted the magic `72`** into a module-level `PR_TITLE_MAX_CHARS: usize` constant, referenced from both the JSON schema (`maxLength: PR_TITLE_MAX_CHARS`) and `enforce_title_cap`. The system prompt's \"max 72 characters\" wording got an explicit sync-comment.\n- **Replaced `(String, String)` return type with a named `PrContent { title, body }` struct** across the four `build_pr_body*` signatures and all five call sites. Prevents accidental field swap and self-documents at the call site.\n- **Collapsed the six tier constants** (`MAX_GOAL_CHARS_LARGE/SMALL`, etc.) into a `TruncationCaps` struct with two `const`s (`TRUNCATION_LARGE`, `TRUNCATION_SMALL`). Call site went from `let (g, p, d) = truncation_caps(...)` to `let caps = truncation_caps(...)`.\n- **Consolidated split `use` statements** at the top of the file. The file previously had ~150 lines of new constants/functions wedged between two `use` blocks; all imports are now at the top.\n- **Eliminated the redundant double `enforce_title_cap`** in `maybe_open_pull_request` by moving it into the fallback branch only — the LLM-title branch is already capped inside the builder. Added a comment explaining why the fallback branch needs re-capping (`pr_title_from_goal` has a wider 120-char cap).\n- **Fixed misleading `truncate_chars` doc comment** — it was documented as \"chars\" but actually byte-caps and aligns to a UTF-8 boundary. Doc now correctly says \"bytes\" and notes the char/byte equivalence for ASCII.\n\n**Skipped (false positives or low value)**\n- `Catalog::builtin()` cost — verified to be a static `LazyLock`, no rebuild per call.\n- `PR_CONTENT_SCHEMA.clone()` — `generate_object` takes `Value` by value; clone is dwarfed by the LLM HTTP call.\n- `pr_content_json` cross-crate sharing — only two external duplications, low value to expose via `test-support`.\n- `setup_fallback_test_harness` partial overlap with the existing vault test — different shapes (one needs GitHub mock + final_patch event, the other doesn't); merging would tangle.\n- Whitespace-leak in title — verified non-issue: builder trims via `enforce_title_cap(generated.title.trim())` before storing on the struct.\n- `_vault_dir`/`_openai_server` keepalive pattern in test harness — already commented; standard Rust pattern.\n\n**Verification**: `cargo check --workspace --all-targets` clean, `cargo nextest run -p fabro-workflow` 1057/1057 passed, `cargo nextest run -p fabro-server` 443/443 passed, `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` clean, `cargo +nightly-2026-04-14 fmt --check --all` clean.", @@ -532,8 +534,9 @@ "internal.retry_count.preflight_lint": 0, "internal.node_visit_count": 1, "thread.preflight_compile.current_node": "preflight_lint", - "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.output": "blob://sha256/510d88f09eb4441cc3dd3be90e9296f0a52f7906e342de45aeeae4cbd378e535", "failure_class": "", + "thread.simplify_gpt.current_node": "verify", "thread.start.current_node": "toolchain", "internal.retry_count.simplify_opus": 0, "internal.retry_count.simplify_gpt": 0, @@ -542,9 +545,9 @@ "last_response": "Implemented cleanup fixes after review.\n\nNotes:\n- I launched the three review agents twice in parallel, but all six subagents failed with an internal LLM item-persistence error before returning findin", "internal.retry_count.implement": 0, "response.implement": "Everything is green. Let me summarize what was done:\n\n## Summary\n\nImplemented the compound-engineering PR title and body recipe per plan:\n\n### Code changes (`lib/crates/fabro-workflow/src/pipeline/pull_request.rs`)\n\n1. **Schema + typed struct** — Added `PR_CONTENT_SCHEMA` (`LazyLock`) with required `title` (max 72) and `body` (min 1), and `GeneratedPrContent { title, body }` deserialized from structured output. Strict (no `additionalProperties`).\n\n2. **System prompt** — Added `PR_BODY_SYSTEM_PROMPT` constant containing the verbatim recipe from the plan: output format, \"do not include\" guardrails, sizing matrix, writing principles, plan-summary rule, visual-aid table.\n\n3. **Truncation tiers** — Added six constants and `truncation_caps(model)` that selects the large tier (75k/75k/250k) when `Catalog::builtin().get(model).limits.context_window >= 200_000`, else the conservative tier (20k/20k/50k matching the previous values). Added `truncate_chars` helper for char-boundary-safe slicing.\n\n4. **`enforce_title_cap(&str) -> String`** — 72-char Unicode-scalar cap, replacing the trailing char with `…`.\n\n5. **Refactored builders** — `build_pr_body`, `build_pr_body_with_client`, `build_pr_body_with_source_and_state`, `build_pr_body_with_client_and_state` now return `Result<(String, String), String>`. The inner builder switches `generate` → `generate_object`, deserializes to `GeneratedPrContent`, applies the failure-mode table from the plan: missing/invalid output → `Err`, blank/whitespace body → `Err`, empty title → passes through unchanged (caller handles fallback), oversized title → capped via `enforce_title_cap`. The body is unchanged from the LLM and assembled normally with the trailing programmatic sections.\n\n6. **`maybe_open_pull_request`** — Destructures `(llm_title, body)`. If `llm_title.trim().is_empty()`, falls back to `pr_title_from_goal(req.goal)`; otherwise uses the LLM title. Then `enforce_title_cap` runs unconditionally (covers both branches), so the fallback path can never breach the 72-char contract that the LLM path satisfies via the schema.\n\n### Tests added (red→green)\n\n- `build_pr_body_truncates_long_title` — MockProvider returns 200-char title; assert `chars().count() == 72` and ends with `…`.\n- `build_pr_body_returns_err_when_body_empty` — schema rejects `\"\"` (minLength 1).\n- `build_pr_body_returns_err_when_body_whitespace` — schema accepts `\" \\n\"`, but the Rust trim-check rejects it; asserts the specific error message.\n- `setup_fallback_test_harness` — shared helper standing up an `httpmock::MockServer` for OpenAI (`POST /v1/responses`, bearer `vault-openai-key`), a separate mock for GitHub (`POST /repos/owner/repo/pulls`, bearer `test-token`), a `Vault`-backed `VaultCredentialSource` with `OPENAI_BASE_URL` env override, `GitHubCredentials::Token(\"test-token\")`, and a `RunStoreHandle` seeded with `WorkflowRunCompleted { final_patch: Some(...) }` so `load_pull_request_diff` returns content. The `_openai_server` field is held to keep the mock listener alive.\n- `maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title` — OpenAI mock returns `{\"title\":\"\",\"body\":\"Narrative.\"}`, goal `\"Fix telemetry leak\\n\\ndetails...\"`; asserts `record.title == \"Fix telemetry leak\"`.\n- `maybe_open_pull_request_caps_fallback_title_at_72_chars` — same harness, 200-char goal forces `pr_title_from_goal` to return ~120 chars; asserts the final title is exactly 72 chars and ends with `…`, proving the load-bearing unconditional `enforce_title_cap` after fallback selection.\n\n### Existing tests updated\n\n- All 5 existing `build_pr_body_*` tests in `pull_request.rs` now destructure `(title, body)` and route their MockProvider/openai responses through the new `pr_content_json(title, body)` helper. `build_pr_body_uses_in_memory_conclusion` and `build_pr_body_uses_vault_only_openai_codex_source` additionally assert on title equality.\n- `lib/crates/fabro-workflow/tests/it/integration.rs:6893` (`workflow_run_with_vault_only_openai_codex_builds_pr_body`) — updated to destructure the tuple, return structured-output JSON from the OpenAI mock, and assert `title == \"Vault title\"`.\n- `lib/crates/fabro-server/src/server/tests.rs:3668` (`create_run_pull_request_creates_and_persists_record`) — updated the OpenAI responses mock to return structured-output JSON instead of plain text.\n\n### Verification\n\n- `cargo build --workspace` ✅\n- `cargo nextest run -p fabro-workflow` ✅ (1057/1057 pass, 34 skipped)\n- `cargo nextest run -p fabro-server` ✅ (441/443 pass; the two `*_returns_svg` failures are pre-existing and unrelated — confirmed by reproducing them on the parent commit `dc09842a`)\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` ✅\n- `cargo +nightly-2026-04-14 fmt --check --all` ✅\n\n### Out of scope items (per plan) — not changed\n\n- No `pull_request.prompt_preset`/`prompt_override` config.\n- No separate `pull_request.model` override.\n- No dynamic budgeting based on tokenizer math (two-tier static lookup as planned).\n- No stage-response inclusion.\n- Goal truncation is silent — full goal is *not* attached as a `
` block (option A).\n- `assemble_pr_body`, `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `truncate_pr_body`, `load_pull_request_diff`, `pr_title_from_goal` (with its 120-char cap), `OpenPullRequestRequest`, `PullRequestRecord`, `AutoMergeOptions`, the 65,536-char body cap, and the `_(truncated)_` suffix are unchanged.", - "internal.thread_id": "simplify_opus", + "internal.thread_id": "simplify_gpt", "failure_signature": "", - "current_node": "simplify_gpt", + "current_node": "verify", "internal.fidelity": "compact", "internal.retry_count.start": 0, "internal.retry_count.toolchain": 0, @@ -597,6 +600,15 @@ "/home/daytona/workspace/lib/crates/fabro-workflow/tests/it/integration.rs" ] }, + "verify": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/510d88f09eb4441cc3dd3be90e9296f0a52f7906e342de45aeeae4cbd378e535", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: 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", + "usage": null + }, "simplify_gpt": { "status": "succeeded", "context_updates": { @@ -687,15 +699,16 @@ "usage": null } }, - "next_node_id": "verify", + "next_node_id": "fmt", "node_visits": { + "simplify_gpt": 1, + "implement": 1, "preflight_lint": 1, "start": 1, "simplify_opus": 1, - "simplify_gpt": 1, + "verify": 1, "preflight_compile": 1, - "toolchain": 1, - "implement": 1 + "toolchain": 1 } }, "checkpoints": [ @@ -1214,6 +1227,204 @@ "start": 1 } } + ], + [ + 956, + { + "timestamp": "2026-05-04T19:15:10.124035Z", + "current_node": "simplify_gpt", + "completed_nodes": [ + "start", + "toolchain", + "preflight_compile", + "preflight_lint", + "implement", + "simplify_opus", + "simplify_gpt" + ], + "node_retries": {}, + "context_values": { + "thread.implement.current_node": "simplify_opus", + "internal.thread_id": "simplify_opus", + "graph.model_stylesheet": "\n * { model: claude-opus-4-7; }\n ", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "response.simplify_gpt": "Implemented cleanup fixes after review.\n\nNotes:\n- I launched the three review agents twice in parallel, but all six subagents failed with an internal LLM item-persistence error before returning findings. I proceeded with the review manually.\n\nFixed:\n- Removed the extra public `PrContent` struct and restored the requested `Result<(String, String), String>` return shape for `build_pr_body*`.\n- Updated callers/tests to destructure `(title, body)` instead of using a new public wrapper type.\n- Made `maybe_open_pull_request` apply `enforce_title_cap` unconditionally after selecting LLM title vs. fallback title.\n- Changed prompt input truncation helper to actually cap by Unicode scalar count instead of byte count while preserving UTF-8 safety.\n- Strengthened fallback PR tests to assert both OpenAI and GitHub mocks were called exactly once.\n- Clarified the empty-body test comment so it doesn’t falsely claim local mock providers perform schema `minLength` validation.\n\nValidated:\n- `cargo check -q -p fabro-workflow`\n- `cargo check -q -p fabro-server`\n- `cargo nextest run -p fabro-workflow pull_request`\n- `cargo nextest run -p fabro-workflow workflow_run_with_vault_only_openai_codex_builds_pr_body`\n- `cargo nextest run -p fabro-server create_run_pull_request_creates_and_persists_record`\n- `cargo +nightly-2026-04-14 clippy -q -p fabro-workflow --all-targets -- -D warnings`\n- `cargo +nightly-2026-04-14 fmt --check --all`", + "internal.retry_count.simplify_opus": 0, + "internal.retry_count.preflight_compile": 0, + "last_response": "Implemented cleanup fixes after review.\n\nNotes:\n- I launched the three review agents twice in parallel, but all six subagents failed with an internal LLM item-persistence error before returning findin", + "current_node": "simplify_gpt", + "thread.simplify_opus.current_node": "simplify_gpt", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "graph.goal": "# Compound-engineering PR title and body recipe for Fabro\n\n## Context\n\nToday Fabro's PR body is generated by a hardcoded two-line system prompt at `lib/crates/fabro-workflow/src/pipeline/pull_request.rs:373-377`. The title is derived deterministically from the workflow goal's first line (`pr_title_from_goal`). Compound-engineering's `git-commit-push-pr` skill produces noticeably better PR descriptions because it embeds a sizing matrix, writing principles, a visual-aid table, and the `#1`-as-issue-ref footgun, and it generates the title from change context.\n\nWe want to port the *valuable* parts of that recipe into Fabro's existing pipeline while keeping Fabro's signature trailing sections (Plan `
`, `### Retro`, `### Fabro Details`, `⚒️ Generated with [Fabro]` footer). Per user direction, the LLM self-classifies the change against the sizing matrix (no Rust-side bucketing), and goal truncation is silent (option A: accept the loss; do not attach the full goal to the rendered body).\n\n## Approach\n\nReplace the plain-text LLM call with a single `generate_object` call returning a structured `{title, body}` JSON object. Embed the compound-engineering recipe in the system prompt with explicit \"do not duplicate the trailing sections\" guardrails. Raise input truncation caps to fit modern context windows and add a goal cap (previously uncapped).\n\nThe trailing programmatic sections are unchanged — `assemble_pr_body` still prepends the LLM body to Plan / Retro / Fabro Details / footer. `pr_title_from_goal` survives as the fallback for the narrow case where structured output succeeded with a usable body but the LLM returned an empty title; every other generation failure remains fatal (run still completes, no PR is opened).\n\n## Critical files\n\n- `lib/crates/fabro-workflow/src/pipeline/pull_request.rs` — primary; replace prompt + signatures\n- `lib/crates/fabro-workflow/tests/it/integration.rs:6893` — one test that calls `build_pr_body` directly; signature update\n\n## Reuse (do not reimplement)\n\n- `fabro_llm::generate::generate_object(params, schema)` — `lib/crates/fabro-llm/src/generate.rs:920-945`. Existing pattern in `fabro-hooks/src/executor.rs:27-37, 315-338` (LazyLock schema + typed deserialize + warn-on-failure). Mirror that pattern.\n- `assemble_pr_body`, `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `truncate_pr_body`, `load_pull_request_diff` — unchanged.\n- `pr_title_from_goal`, `strip_goal_decoration` — unchanged; demoted from \"always-used\" to \"fallback path.\"\n\n## Changes\n\n### 1. New schema and typed struct\n\nAdd at module top:\n\n```rust\nuse std::sync::LazyLock;\n\n#[derive(Debug, serde::Deserialize)]\nstruct GeneratedPrContent {\n title: String,\n body: String,\n}\n\nstatic PR_CONTENT_SCHEMA: LazyLock = LazyLock::new(|| {\n serde_json::json!({\n \"type\": \"object\",\n \"properties\": {\n \"title\": { \"type\": \"string\", \"maxLength\": 72 },\n \"body\": { \"type\": \"string\", \"minLength\": 1 }\n },\n \"required\": [\"title\", \"body\"],\n \"additionalProperties\": false\n })\n});\n```\n\n**Schema vs fallback semantics.** Both fields are `required` so a missing field fails structured-output validation (fatal — propagates as Err). The schema does *not* set `minLength` on `title`, so an empty-string title deserializes successfully and is the *only* signal that triggers the deterministic title fallback in `maybe_open_pull_request`. `body` has `minLength: 1` because we have no body fallback — an empty body is fatal too. This narrower fallback promise (empty title only, not missing title) keeps the schema strict without making the fallback dead code.\n\n### 2. New system prompt as a `const &str`\n\nAdd as a module-level `const PR_BODY_SYSTEM_PROMPT: &str = \"...\"`. Verbatim content:\n\n```text\nYou are writing a pull request title and description for a code change produced by an AI workflow.\n\nOUTPUT FORMAT\nReturn a JSON object with exactly two fields:\n- \"title\": a one-line title, max 72 characters, no trailing period.\n- \"body\": the markdown body as described below.\n\nDO NOT INCLUDE in the body\n- A `#` or `##` title heading at the top — the title goes in the `title` field.\n- A \"Retro\" section, \"Fabro Details\" section, cost/duration table, or \"Generated with\" footer — those are appended programmatically after your output.\n- The full plan text — the full plan is appended programmatically as a
block.\n- Bare `#1`, `#2` list prefixes — GitHub auto-links those as issue references. Use plain `1.`, `2.` instead.\n- A test plan unless the testing approach is non-obvious.\n\nSIZE THE BODY TO THE CHANGE\nFirst classify along two axes from the diff:\n- Size: how many files changed, how large the diff is.\n- Complexity: trivial (rename / typo / dep bump / config) vs. design decisions / new patterns / cross-cutting concerns.\n\nThen write at the matching depth:\n\n| Profile | Body shape |\n|---|---|\n| Small + simple (typo, config, dep bump) | 1–2 sentences, no headers, total under ~300 characters |\n| Small + non-trivial (targeted bugfix, behavioral change) | Short \"Problem / Fix\" narrative, 3–5 sentences. No headers unless two distinct concerns. |\n| Medium feature or refactor | Summary paragraph, then a section explaining what changed and why. Call out design decisions. |\n| Large or architecturally significant | Full narrative: problem context, approach chosen (and why), key decisions, migration/rollback notes if relevant. |\n| Performance improvement | Include before/after measurements if available. A markdown table works well here. |\n\nBrevity 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.\n\nWRITING PRINCIPLES\n- Lead with value: the first sentence tells the reviewer *why this PR exists*, not *what files changed*.\n- Describe the net result, not the journey: skip intermediate failures, debugging steps, and refactors done during development.\n- Trust the final diff: if the goal or plan disagree with the diff, the diff is authoritative.\n- 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.\n- Use structure when it earns its keep: no empty sections, no template headers without content.\n- If the body uses any `##` heading, the opening summary must also be under a heading (e.g. `## Summary`); otherwise a bare paragraph is fine.\n\nPLAN SUMMARY\nThe 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.\n\nVISUAL AIDS\nInclude 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.\n\n| PR changes... | Visual aid |\n|---|---|\n| 3+ interacting components or services | Mermaid component / interaction diagram |\n| Multi-step workflow or pipeline with non-obvious sequencing | Mermaid flow diagram |\n| 3+ behavioral modes or variants | Markdown comparison table |\n| Before/after data or trade-offs | Markdown table |\n| Data model changes with 3+ related entities | Mermaid ERD |\n\nMermaid: prefer `TB` direction, ≤10 nodes typical. Place inline at the point of relevance, not in a separate \"Diagrams\" section.\n```\n\n### 3. Truncation constants and small-model fallback\n\nReplace inline `50_000` and `20_000` with two tiered constant sets:\n\n```rust\n// Generous tier (≥200k context window)\nconst MAX_GOAL_CHARS_LARGE: usize = 75_000;\nconst MAX_PLAN_CHARS_LARGE: usize = 75_000;\nconst MAX_DIFF_CHARS_LARGE: usize = 250_000;\n\n// Conservative tier (matches the previous values)\nconst MAX_GOAL_CHARS_SMALL: usize = 20_000;\nconst MAX_PLAN_CHARS_SMALL: usize = 20_000;\nconst MAX_DIFF_CHARS_SMALL: usize = 50_000;\n```\n\nResolve the tier by looking up the model in `fabro_model::Catalog::builtin()`:\n\n```rust\nfn truncation_caps(model: &str) -> (usize, usize, usize) {\n let large_enough = Catalog::builtin()\n .get(model)\n .is_some_and(|m| m.limits.context_window >= 200_000);\n if large_enough {\n (MAX_GOAL_CHARS_LARGE, MAX_PLAN_CHARS_LARGE, MAX_DIFF_CHARS_LARGE)\n } else {\n (MAX_GOAL_CHARS_SMALL, MAX_PLAN_CHARS_SMALL, MAX_DIFF_CHARS_SMALL)\n }\n}\n```\n\nUnknown models (`get` returns `None`) fall through to the conservative tier — safer than assuming large context for an unrecognized id. `fabro_model::Catalog` is already a transitive dep of `fabro-workflow` via `start.rs`'s use of `fabro_model::Provider`; no new Cargo entry needed.\n\nFor the large tier, worst-case input is ~400k chars ≈ 100k tokens. Fits in 200k-context Sonnet/Haiku with ~50k-token headroom for system prompt + output. Comfortable on 1M-context models. The conservative tier preserves today's behavior for the two ≤131k-context models in the catalog (`gpt-5.3-codex-spark`, `mercury-2`).\n\nGoal truncation is silent (no log, no marker). The full goal is *not* attached to the rendered body.\n\n### 4. Refactor `build_pr_body*` to return both title and body\n\nSignature change for the three internal builders and the public `build_pr_body`:\n\n```rust\npub async fn build_pr_body(\n diff: &str, goal: &str, model: &str,\n run_store: &RunStoreHandle,\n llm_source: &dyn CredentialSource,\n conclusion: Option<&Conclusion>,\n) -> Result<(String, String), String> // was: Result\n```\n\nReturns `(title, body)`. Callers destructure.\n\n`build_pr_body_with_client_and_state` becomes:\n\n1. Truncate `goal`, `diff`, `plan_text` against the tier from `truncation_caps(model)`.\n2. Build user prompt with `Goal:` / `Plan:` (when present) / `Diff:` sections.\n3. Call `generate_object(params.system(PR_BODY_SYSTEM_PROMPT).prompt(user_prompt), PR_CONTENT_SCHEMA.clone())`.\n4. On success: deserialize `result.output` as `GeneratedPrContent`, trim title, enforce 72-char cap via `enforce_title_cap` (truncate at `floor_char_boundary` + `…` if exceeded — same pattern as `pr_title_from_goal`), keep body as-is.\n5. Pass body through `assemble_pr_body(llm_body, plan_text, retro_section, arc_details_section)` — unchanged. Return `(title, assembled_body)` where `title` may be the empty string after trimming.\n\n**Failure modes — explicit:**\n\n| Condition | Outcome |\n|---|---|\n| `generate_object` returns Err (LLM call failure, schema validation failure, JSON parse failure) | Return Err. Caller logs and emits `PullRequestFailed`; no PR is opened. |\n| `result.output` is `None` | Return Err. Same as above. |\n| Deserialize as `GeneratedPrContent` fails (missing required field, wrong type) | Return Err. Same as above. |\n| Body is blank (`generated.body.trim().is_empty()`) | Return Err. Schema's `minLength: 1` rejects truly empty strings, but a body of `\" \\n\"` would pass the schema; the Rust trim-check catches whitespace-only bodies as well. The body content is preserved as-is once it passes the check — no leading/trailing whitespace stripping, since markdown can rely on it. |\n| Title is empty (after trim) | **Allowed through** — return `(\"\", body)`. The caller is responsible for the deterministic title fallback. |\n| Title is non-empty but >72 chars | Allowed through — `enforce_title_cap` truncates with `…`. |\n\nThe narrower fallback promise: only an empty/whitespace title triggers the deterministic title fallback. Every other error path is fatal. This makes the fallback path actually reachable from the caller and prevents the bug where structured-output failure swallows both fields and there's no body to use anyway.\n\n### 5. `maybe_open_pull_request` invokes the fallback when the title is empty\n\nToday it calls `pr_title_from_goal(req.goal)` unconditionally. New flow — the deterministic fallback only fires when the LLM returned a body but no usable title:\n\n```rust\nlet (llm_title, body) = build_pr_body_with_source_and_state(...).await\n .map_err(|err| format!(\"{err:#}\"))?; // any non-title failure: fatal\n\nlet title = if llm_title.trim().is_empty() {\n pr_title_from_goal(req.goal) // fallback path: only reached when body succeeded\n} else {\n llm_title\n};\nlet title = enforce_title_cap(&title); // unconditional 72-char guarantee, covers fallback path\nlet body = truncate_pr_body(&body); // unchanged 65,536-char hard cap\n```\n\nThe unconditional `enforce_title_cap` after fallback selection is load-bearing: `pr_title_from_goal`'s built-in cap is 120 chars (a holdover from the previous deterministic-only flow), so without re-capping here a long goal would breach the new 72-char contract. Don't lower `pr_title_from_goal`'s internal cap — keep the cap enforcement in one place at the caller, where it covers both branches.\n\nThe pipeline stage at `pull_request.rs:538-623` already converts the propagated Err into a `PullRequestFailed` event without aborting the run, so there is no behavior change for callers when generation fails fully.\n\n### 6. Title cap helper\n\nAdd a small private helper:\n\n```rust\nfn enforce_title_cap(title: &str) -> String {\n const MAX: usize = 72;\n if title.chars().count() > MAX {\n let truncated: String = title.chars().take(MAX - 1).collect();\n format!(\"{truncated}\\u{2026}\")\n } else {\n title.to_string()\n }\n}\n```\n\nApplied inside `build_pr_body_with_client_and_state` to the LLM-returned title before returning. `pr_title_from_goal`'s existing 120-char cap stays as-is (it's the fallback and the goal-derived title is shorter in practice; introducing a 72-char cap there is a separate, low-value change).\n\n### 7. Test updates\n\nIn `lib/crates/fabro-workflow/src/pipeline/pull_request.rs`:\n\n- `MockProvider::complete` and `::stream` currently return plain text. Update to return JSON matching the schema: `{\"title\":\"Mock title\",\"body\":\"Narrative from mock.\"}`. Both call sites (`response_text` field) get this JSON string.\n- `openai_responses_payload` helper (line ~761) returns a JSON-shaped fake API response; update its `text` to be the JSON string `{\"title\":\"…\",\"body\":\"Narrative from vault source.\"}`.\n- `build_pr_body_uses_in_memory_conclusion`, `build_pr_body_uses_store_records_without_legacy_files`, `build_pr_body_uses_plan_text_from_store_without_response_md`, `build_pr_body_uses_explicit_llm_client`, `build_pr_body_uses_vault_only_openai_codex_source` — destructure the new tuple, assert on title and body separately. Existing body assertions (`contains(\"Narrative from mock.\")` etc.) become body-side; add a `title == \"Mock title\"` assertion to one of them.\n- `empty_diff_returns_none` — unchanged behavior, still returns `Ok(None)` from `maybe_open_pull_request`.\n- `pr_title_from_goal` tests (10 of them, lines 1416–1485) — unchanged; the function still exists as a fallback.\n- New test: `maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title` — exercises the actual fallback branch in `maybe_open_pull_request`, not just the builder. **`MockProvider` cannot drive this path**: `maybe_open_pull_request` → `build_pr_body_with_source_and_state` → `Client::from_source(llm_source)` builds a real provider client from the credential source, bypassing any in-process provider injection. Use a real provider HTTP mock instead. Setup:\n - **OpenAI mock** (mirrors `build_pr_body_uses_vault_only_openai_codex_source` at `pull_request.rs:1322`): start an `httpmock::MockServer`, register `POST /v1/responses` with `Authorization: Bearer vault-openai-key`, return `openai_responses_payload(r#\"{\"title\":\"\",\"body\":\"Narrative.\"}\"#)`. The existing helper wraps any string into the OpenAI Responses-API envelope; here that string is the structured-output JSON.\n - **Credential source**: load a `Vault` with an `openai_codex` API-key credential of `vault-openai-key`, wrap it in `VaultCredentialSource::with_env_lookup` returning the OpenAI mock's `/v1` URL for `OPENAI_BASE_URL`. Use this `Arc` as the `llm_source`.\n - **GitHub mock**: a separate `httpmock::MockServer` exposing only `POST /repos/{owner}/{repo}/pulls`, returning a valid PR JSON (e.g. `{ \"number\": 1, \"html_url\": \"...\", \"node_id\": \"...\" }` with a 201 status).\n - **GitHub credentials**: `fabro_github::GitHubCredentials::Token(\"test-token\".to_string())` rather than the App variant. App credentials would force this test to also mock JWT signing and the installation-token exchange; Token credentials let `create_pull_request` go straight to the PR endpoint with a static `Authorization: Bearer test-token` header. (Implementer: confirm the variant name in `lib/crates/fabro-github/src/lib.rs`; if it's `Pat` or similar instead of `Token`, use that.)\n - Pass `&github_server.url(\"\")` as the second arg to `GitHubContext::new(&creds, ...)` so the mock's base URL replaces the production `github_api_base_url()`.\n - Pass a non-empty diff so the early-return at `pull_request.rs:460` doesn't fire.\n - Pass a goal like `\"Fix telemetry leak\\n\\ndetails...\"`. Use a `model` of `\"gpt-5.4\"` (catalog hit, large-tier truncation, matches the OpenAI mock).\n - Pass a `RunStoreHandle` whose `state().final_patch` returns a non-empty diff (mirror the `load_pull_request_diff_uses_store_without_disk_patch` test at line 1521 for the event-append pattern).\n - Assert: `PullRequestRecord.title == \"Fix telemetry leak\"`, the OpenAI mock fired exactly once, the GitHub mock fired exactly once with `Authorization: Bearer test-token`. Optionally inspect the captured GitHub request body to confirm the title sent on the wire matches.\n\n- New test: `maybe_open_pull_request_caps_fallback_title_at_72_chars` — guards the unconditional `enforce_title_cap` in §5. Same harness as the test above (extract a private `setup_fallback_test_harness()` helper to avoid duplicating the OpenAI + GitHub + Vault wiring). Differences: pass a goal that's a single ~200-char line with no newlines and no `Plan:` prefix, so `pr_title_from_goal` returns close to its 120-char cap. The OpenAI mock returns `{\"title\":\"\",\"body\":\"Narrative.\"}` so the fallback fires. Assert `PullRequestRecord.title.chars().count() == 72` and the title ends with `…`.\n- New test: `build_pr_body_truncates_long_title` — MockProvider returns `{\"title\":\"x\".repeat(200),\"body\":\"…\"}`; assert the title returned from `build_pr_body` is exactly 72 chars and ends with `…`. (This one stays at the builder level — it's testing `enforce_title_cap`, not the fallback.)\n- New test: `build_pr_body_returns_err_when_body_blank` — two cases via parameterized assertion or two `#[test]`s sharing a helper:\n - MockProvider returns `{\"title\":\"Mock\",\"body\":\"\"}` — schema rejects via `minLength: 1`, builder returns `Err`.\n - MockProvider returns `{\"title\":\"Mock\",\"body\":\" \\n\"}` — schema accepts (length ≥ 1), Rust trim-check returns `Err`.\n Both cases validate the blank-body-is-fatal contract from §4's failure-mode table.\n\nIn `lib/crates/fabro-workflow/tests/it/integration.rs`:\n\n- `workflow_run_with_vault_only_openai_codex_builds_pr_body` (line 6893 area) — destructure the tuple, update assertions.\n\n### 8. What is *not* changing\n\n- `assemble_pr_body` — body still slots in front of the four programmatic sections in the same order.\n- `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `parse_dot_summary`, `format_duration_ms`, `format_cost`, `truncate_pr_body` — unchanged.\n- `OpenPullRequestRequest`, `PullRequestRecord`, `AutoMergeOptions` — unchanged shapes.\n- The two production callers (`fabro-server/src/server/handler/pull_requests.rs:262-277` and the `pull_request` pipeline stage at `pull_request.rs:573`) — unchanged. Both go through `maybe_open_pull_request`, which absorbs the new title flow internally.\n- The 65,536-char body cap and `_(truncated)_` suffix.\n\n## Verification\n\n```sh\ncargo build --workspace\ncargo nextest run -p fabro-workflow\ncargo nextest run -p fabro-server\ncargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings\ncargo +nightly-2026-04-14 fmt --check --all\n```\n\nEnd-to-end (optional, requires credentials):\n\n```sh\nset -a && source .env && set +a\ncargo nextest run -p fabro-workflow --profile e2e --run-ignored only\n```\n\nManual smoke: run any workflow that opens a PR and inspect the resulting PR title and body on GitHub. Confirm:\n\n- Title ≤ 72 chars, no markdown decoration.\n- Body opens with a value-first sentence (per \"lead with value\" principle).\n- Body does *not* contain a duplicate Retro / Fabro Details / footer / full-plan section.\n- Trailing sections render as before: Plan `
`, `### Retro`, `### Fabro Details`, `⚒️ Generated with [Fabro]`.\n- For a small mechanical change, the body is short (sizing matrix small+simple); for a multi-stage feature run, the body is multi-paragraph with sections.\n\n## Out of scope (explicit)\n\n- Fully dynamic truncation budgeting (`min(MAX_DIFF_CHARS, ctx_window / 4)`). The plan uses a two-tier static lookup keyed on a 200k threshold instead — simpler, addresses the small-context-model risk, and avoids tokenizer math.\n- Stage-response inclusion (e.g. the implement node's response as commit-message analog). Worth doing later as a separate change.\n- Configurable `pull_request.prompt_preset` or `prompt_override` in `PullRequestSettings`. Not needed for this iteration; the prompt is opinionated by design.\n- Separate `pull_request.model` override (use Haiku/mini for body generation to cut cost). Defensible follow-up; out of scope here.\n- Attaching the full goal as a `
` block (option B). User chose option A.\n- Anthropic probe override to keep connectivity checks on Haiku after the Opus 4.7 default change. Separate concern, surfaced earlier.\n", + "internal.retry_count.preflight_lint": 0, + "thread.preflight_lint.current_node": "implement", + "outcome": "succeeded", + "thread.start.current_node": "toolchain", + "internal.retry_count.implement": 0, + "internal.retry_count.simplify_gpt": 0, + "failure_signature": "", + "internal.fidelity": "compact", + "response.simplify_opus": "Both clean. \n\n## Summary of cleanups\n\nAggregated findings across the three review agents and fixed the actionable ones:\n\n**Code reuse**\n- **Unified `enforce_title_cap` and `pr_title_from_goal`'s truncation logic** into a shared `truncate_with_ellipsis(s, max) -> String` helper. Eliminates the duplicated `chars().count() > N → take(N-1).collect() + '…'` algorithm. `pr_title_from_goal` is now a one-liner.\n- **Switched from field access to the `m.context_window()` accessor** in `truncation_caps`, matching the convention used by the other 4 catalog-consumer call sites in the workspace (`fabro-agent`, `fabro-llm`, etc.).\n\n**Code quality**\n- **Hoisted the magic `72`** into a module-level `PR_TITLE_MAX_CHARS: usize` constant, referenced from both the JSON schema (`maxLength: PR_TITLE_MAX_CHARS`) and `enforce_title_cap`. The system prompt's \"max 72 characters\" wording got an explicit sync-comment.\n- **Replaced `(String, String)` return type with a named `PrContent { title, body }` struct** across the four `build_pr_body*` signatures and all five call sites. Prevents accidental field swap and self-documents at the call site.\n- **Collapsed the six tier constants** (`MAX_GOAL_CHARS_LARGE/SMALL`, etc.) into a `TruncationCaps` struct with two `const`s (`TRUNCATION_LARGE`, `TRUNCATION_SMALL`). Call site went from `let (g, p, d) = truncation_caps(...)` to `let caps = truncation_caps(...)`.\n- **Consolidated split `use` statements** at the top of the file. The file previously had ~150 lines of new constants/functions wedged between two `use` blocks; all imports are now at the top.\n- **Eliminated the redundant double `enforce_title_cap`** in `maybe_open_pull_request` by moving it into the fallback branch only — the LLM-title branch is already capped inside the builder. Added a comment explaining why the fallback branch needs re-capping (`pr_title_from_goal` has a wider 120-char cap).\n- **Fixed misleading `truncate_chars` doc comment** — it was documented as \"chars\" but actually byte-caps and aligns to a UTF-8 boundary. Doc now correctly says \"bytes\" and notes the char/byte equivalence for ASCII.\n\n**Skipped (false positives or low value)**\n- `Catalog::builtin()` cost — verified to be a static `LazyLock`, no rebuild per call.\n- `PR_CONTENT_SCHEMA.clone()` — `generate_object` takes `Value` by value; clone is dwarfed by the LLM HTTP call.\n- `pr_content_json` cross-crate sharing — only two external duplications, low value to expose via `test-support`.\n- `setup_fallback_test_harness` partial overlap with the existing vault test — different shapes (one needs GitHub mock + final_patch event, the other doesn't); merging would tangle.\n- Whitespace-leak in title — verified non-issue: builder trims via `enforce_title_cap(generated.title.trim())` before storing on the struct.\n- `_vault_dir`/`_openai_server` keepalive pattern in test harness — already commented; standard Rust pattern.\n\n**Verification**: `cargo check --workspace --all-targets` clean, `cargo nextest run -p fabro-workflow` 1057/1057 passed, `cargo nextest run -p fabro-server` 443/443 passed, `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` clean, `cargo +nightly-2026-04-14 fmt --check --all` clean.", + "graph.rankdir": "LR", + "failure_class": "", + "last_stage": "simplify_gpt", + "internal.run_id": "01KQT1VKB74S0N423QFMFCY3EB", + "thread.preflight_compile.current_node": "preflight_lint", + "internal.node_visit_count": 1, + "thread.toolchain.current_node": "preflight_compile", + "internal.retry_count.toolchain": 0, + "internal.work_dir": "/home/daytona/workspace", + "internal.retry_count.start": 0, + "response.implement": "Everything is green. Let me summarize what was done:\n\n## Summary\n\nImplemented the compound-engineering PR title and body recipe per plan:\n\n### Code changes (`lib/crates/fabro-workflow/src/pipeline/pull_request.rs`)\n\n1. **Schema + typed struct** — Added `PR_CONTENT_SCHEMA` (`LazyLock`) with required `title` (max 72) and `body` (min 1), and `GeneratedPrContent { title, body }` deserialized from structured output. Strict (no `additionalProperties`).\n\n2. **System prompt** — Added `PR_BODY_SYSTEM_PROMPT` constant containing the verbatim recipe from the plan: output format, \"do not include\" guardrails, sizing matrix, writing principles, plan-summary rule, visual-aid table.\n\n3. **Truncation tiers** — Added six constants and `truncation_caps(model)` that selects the large tier (75k/75k/250k) when `Catalog::builtin().get(model).limits.context_window >= 200_000`, else the conservative tier (20k/20k/50k matching the previous values). Added `truncate_chars` helper for char-boundary-safe slicing.\n\n4. **`enforce_title_cap(&str) -> String`** — 72-char Unicode-scalar cap, replacing the trailing char with `…`.\n\n5. **Refactored builders** — `build_pr_body`, `build_pr_body_with_client`, `build_pr_body_with_source_and_state`, `build_pr_body_with_client_and_state` now return `Result<(String, String), String>`. The inner builder switches `generate` → `generate_object`, deserializes to `GeneratedPrContent`, applies the failure-mode table from the plan: missing/invalid output → `Err`, blank/whitespace body → `Err`, empty title → passes through unchanged (caller handles fallback), oversized title → capped via `enforce_title_cap`. The body is unchanged from the LLM and assembled normally with the trailing programmatic sections.\n\n6. **`maybe_open_pull_request`** — Destructures `(llm_title, body)`. If `llm_title.trim().is_empty()`, falls back to `pr_title_from_goal(req.goal)`; otherwise uses the LLM title. Then `enforce_title_cap` runs unconditionally (covers both branches), so the fallback path can never breach the 72-char contract that the LLM path satisfies via the schema.\n\n### Tests added (red→green)\n\n- `build_pr_body_truncates_long_title` — MockProvider returns 200-char title; assert `chars().count() == 72` and ends with `…`.\n- `build_pr_body_returns_err_when_body_empty` — schema rejects `\"\"` (minLength 1).\n- `build_pr_body_returns_err_when_body_whitespace` — schema accepts `\" \\n\"`, but the Rust trim-check rejects it; asserts the specific error message.\n- `setup_fallback_test_harness` — shared helper standing up an `httpmock::MockServer` for OpenAI (`POST /v1/responses`, bearer `vault-openai-key`), a separate mock for GitHub (`POST /repos/owner/repo/pulls`, bearer `test-token`), a `Vault`-backed `VaultCredentialSource` with `OPENAI_BASE_URL` env override, `GitHubCredentials::Token(\"test-token\")`, and a `RunStoreHandle` seeded with `WorkflowRunCompleted { final_patch: Some(...) }` so `load_pull_request_diff` returns content. The `_openai_server` field is held to keep the mock listener alive.\n- `maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title` — OpenAI mock returns `{\"title\":\"\",\"body\":\"Narrative.\"}`, goal `\"Fix telemetry leak\\n\\ndetails...\"`; asserts `record.title == \"Fix telemetry leak\"`.\n- `maybe_open_pull_request_caps_fallback_title_at_72_chars` — same harness, 200-char goal forces `pr_title_from_goal` to return ~120 chars; asserts the final title is exactly 72 chars and ends with `…`, proving the load-bearing unconditional `enforce_title_cap` after fallback selection.\n\n### Existing tests updated\n\n- All 5 existing `build_pr_body_*` tests in `pull_request.rs` now destructure `(title, body)` and route their MockProvider/openai responses through the new `pr_content_json(title, body)` helper. `build_pr_body_uses_in_memory_conclusion` and `build_pr_body_uses_vault_only_openai_codex_source` additionally assert on title equality.\n- `lib/crates/fabro-workflow/tests/it/integration.rs:6893` (`workflow_run_with_vault_only_openai_codex_builds_pr_body`) — updated to destructure the tuple, return structured-output JSON from the OpenAI mock, and assert `title == \"Vault title\"`.\n- `lib/crates/fabro-server/src/server/tests.rs:3668` (`create_run_pull_request_creates_and_persists_record`) — updated the OpenAI responses mock to return structured-output JSON instead of plain text.\n\n### Verification\n\n- `cargo build --workspace` ✅\n- `cargo nextest run -p fabro-workflow` ✅ (1057/1057 pass, 34 skipped)\n- `cargo nextest run -p fabro-server` ✅ (441/443 pass; the two `*_returns_svg` failures are pre-existing and unrelated — confirmed by reproducing them on the parent commit `dc09842a`)\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` ✅\n- `cargo +nightly-2026-04-14 fmt --check --all` ✅\n\n### Out of scope items (per plan) — not changed\n\n- No `pull_request.prompt_preset`/`prompt_override` config.\n- No separate `pull_request.model` override.\n- No dynamic budgeting based on tokenizer math (two-tier static lookup as planned).\n- No stage-response inclusion.\n- Goal truncation is silent — full goal is *not* attached as a `
` block (option A).\n- `assemble_pr_body`, `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `truncate_pr_body`, `load_pull_request_diff`, `pr_title_from_goal` (with its 120-char cap), `OpenPullRequestRequest`, `PullRequestRecord`, `AutoMergeOptions`, the 65,536-char body cap, and the `_(truncated)_` suffix are unchanged." + }, + "node_outcomes": { + "simplify_opus": { + "status": "succeeded", + "context_updates": { + "last_response": "Both clean. \n\n## Summary of cleanups\n\nAggregated findings across the three review agents and fixed the actionable ones:\n\n**Code reuse**\n- **Unified `enforce_title_cap` and `pr_title_from_goal`'s trunc", + "last_stage": "simplify_opus", + "response.simplify_opus": "Both clean. \n\n## Summary of cleanups\n\nAggregated findings across the three review agents and fixed the actionable ones:\n\n**Code reuse**\n- **Unified `enforce_title_cap` and `pr_title_from_goal`'s truncation logic** into a shared `truncate_with_ellipsis(s, max) -> String` helper. Eliminates the duplicated `chars().count() > N → take(N-1).collect() + '…'` algorithm. `pr_title_from_goal` is now a one-liner.\n- **Switched from field access to the `m.context_window()` accessor** in `truncation_caps`, matching the convention used by the other 4 catalog-consumer call sites in the workspace (`fabro-agent`, `fabro-llm`, etc.).\n\n**Code quality**\n- **Hoisted the magic `72`** into a module-level `PR_TITLE_MAX_CHARS: usize` constant, referenced from both the JSON schema (`maxLength: PR_TITLE_MAX_CHARS`) and `enforce_title_cap`. The system prompt's \"max 72 characters\" wording got an explicit sync-comment.\n- **Replaced `(String, String)` return type with a named `PrContent { title, body }` struct** across the four `build_pr_body*` signatures and all five call sites. Prevents accidental field swap and self-documents at the call site.\n- **Collapsed the six tier constants** (`MAX_GOAL_CHARS_LARGE/SMALL`, etc.) into a `TruncationCaps` struct with two `const`s (`TRUNCATION_LARGE`, `TRUNCATION_SMALL`). Call site went from `let (g, p, d) = truncation_caps(...)` to `let caps = truncation_caps(...)`.\n- **Consolidated split `use` statements** at the top of the file. The file previously had ~150 lines of new constants/functions wedged between two `use` blocks; all imports are now at the top.\n- **Eliminated the redundant double `enforce_title_cap`** in `maybe_open_pull_request` by moving it into the fallback branch only — the LLM-title branch is already capped inside the builder. Added a comment explaining why the fallback branch needs re-capping (`pr_title_from_goal` has a wider 120-char cap).\n- **Fixed misleading `truncate_chars` doc comment** — it was documented as \"chars\" but actually byte-caps and aligns to a UTF-8 boundary. Doc now correctly says \"bytes\" and notes the char/byte equivalence for ASCII.\n\n**Skipped (false positives or low value)**\n- `Catalog::builtin()` cost — verified to be a static `LazyLock`, no rebuild per call.\n- `PR_CONTENT_SCHEMA.clone()` — `generate_object` takes `Value` by value; clone is dwarfed by the LLM HTTP call.\n- `pr_content_json` cross-crate sharing — only two external duplications, low value to expose via `test-support`.\n- `setup_fallback_test_harness` partial overlap with the existing vault test — different shapes (one needs GitHub mock + final_patch event, the other doesn't); merging would tangle.\n- Whitespace-leak in title — verified non-issue: builder trims via `enforce_title_cap(generated.title.trim())` before storing on the struct.\n- `_vault_dir`/`_openai_server` keepalive pattern in test harness — already commented; standard Rust pattern.\n\n**Verification**: `cargo check --workspace --all-targets` clean, `cargo nextest run -p fabro-workflow` 1057/1057 passed, `cargo nextest run -p fabro-server` 443/443 passed, `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` clean, `cargo +nightly-2026-04-14 fmt --check --all` clean." + }, + "notes": "Stage completed: simplify_opus", + "usage": { + "input": { + "usage": { + "model": { + "provider": "anthropic", + "model_id": "claude-opus-4-7" + }, + "tokens": { + "input_tokens": 81685, + "output_tokens": 24785, + "reasoning_tokens": 0, + "cache_read_tokens": 3745415, + "cache_write_tokens": 100714 + } + }, + "facts": { + "provider": "anthropic", + "cache_write_5m_tokens": 100714, + "cache_write_1h_tokens": 0 + } + }, + "total_usd_micros": 3530219 + }, + "files_touched": [ + "/home/daytona/workspace/lib/crates/fabro-workflow/src/pipeline/pull_request.rs", + "/home/daytona/workspace/lib/crates/fabro-workflow/tests/it/integration.rs" + ] + }, + "preflight_lint": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "usage": null + }, + "preflight_compile": { + "status": "succeeded", + "context_updates": { + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "command.output": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: cargo check -q --workspace 2>&1", + "usage": null + }, + "implement": { + "status": "succeeded", + "context_updates": { + "last_stage": "implement", + "last_response": "Everything is green. Let me summarize what was done:\n\n## Summary\n\nImplemented the compound-engineering PR title and body recipe per plan:\n\n### Code changes (`lib/crates/fabro-workflow/src/pipeline/pul", + "response.implement": "Everything is green. Let me summarize what was done:\n\n## Summary\n\nImplemented the compound-engineering PR title and body recipe per plan:\n\n### Code changes (`lib/crates/fabro-workflow/src/pipeline/pull_request.rs`)\n\n1. **Schema + typed struct** — Added `PR_CONTENT_SCHEMA` (`LazyLock`) with required `title` (max 72) and `body` (min 1), and `GeneratedPrContent { title, body }` deserialized from structured output. Strict (no `additionalProperties`).\n\n2. **System prompt** — Added `PR_BODY_SYSTEM_PROMPT` constant containing the verbatim recipe from the plan: output format, \"do not include\" guardrails, sizing matrix, writing principles, plan-summary rule, visual-aid table.\n\n3. **Truncation tiers** — Added six constants and `truncation_caps(model)` that selects the large tier (75k/75k/250k) when `Catalog::builtin().get(model).limits.context_window >= 200_000`, else the conservative tier (20k/20k/50k matching the previous values). Added `truncate_chars` helper for char-boundary-safe slicing.\n\n4. **`enforce_title_cap(&str) -> String`** — 72-char Unicode-scalar cap, replacing the trailing char with `…`.\n\n5. **Refactored builders** — `build_pr_body`, `build_pr_body_with_client`, `build_pr_body_with_source_and_state`, `build_pr_body_with_client_and_state` now return `Result<(String, String), String>`. The inner builder switches `generate` → `generate_object`, deserializes to `GeneratedPrContent`, applies the failure-mode table from the plan: missing/invalid output → `Err`, blank/whitespace body → `Err`, empty title → passes through unchanged (caller handles fallback), oversized title → capped via `enforce_title_cap`. The body is unchanged from the LLM and assembled normally with the trailing programmatic sections.\n\n6. **`maybe_open_pull_request`** — Destructures `(llm_title, body)`. If `llm_title.trim().is_empty()`, falls back to `pr_title_from_goal(req.goal)`; otherwise uses the LLM title. Then `enforce_title_cap` runs unconditionally (covers both branches), so the fallback path can never breach the 72-char contract that the LLM path satisfies via the schema.\n\n### Tests added (red→green)\n\n- `build_pr_body_truncates_long_title` — MockProvider returns 200-char title; assert `chars().count() == 72` and ends with `…`.\n- `build_pr_body_returns_err_when_body_empty` — schema rejects `\"\"` (minLength 1).\n- `build_pr_body_returns_err_when_body_whitespace` — schema accepts `\" \\n\"`, but the Rust trim-check rejects it; asserts the specific error message.\n- `setup_fallback_test_harness` — shared helper standing up an `httpmock::MockServer` for OpenAI (`POST /v1/responses`, bearer `vault-openai-key`), a separate mock for GitHub (`POST /repos/owner/repo/pulls`, bearer `test-token`), a `Vault`-backed `VaultCredentialSource` with `OPENAI_BASE_URL` env override, `GitHubCredentials::Token(\"test-token\")`, and a `RunStoreHandle` seeded with `WorkflowRunCompleted { final_patch: Some(...) }` so `load_pull_request_diff` returns content. The `_openai_server` field is held to keep the mock listener alive.\n- `maybe_open_pull_request_falls_back_to_goal_title_when_llm_returns_empty_title` — OpenAI mock returns `{\"title\":\"\",\"body\":\"Narrative.\"}`, goal `\"Fix telemetry leak\\n\\ndetails...\"`; asserts `record.title == \"Fix telemetry leak\"`.\n- `maybe_open_pull_request_caps_fallback_title_at_72_chars` — same harness, 200-char goal forces `pr_title_from_goal` to return ~120 chars; asserts the final title is exactly 72 chars and ends with `…`, proving the load-bearing unconditional `enforce_title_cap` after fallback selection.\n\n### Existing tests updated\n\n- All 5 existing `build_pr_body_*` tests in `pull_request.rs` now destructure `(title, body)` and route their MockProvider/openai responses through the new `pr_content_json(title, body)` helper. `build_pr_body_uses_in_memory_conclusion` and `build_pr_body_uses_vault_only_openai_codex_source` additionally assert on title equality.\n- `lib/crates/fabro-workflow/tests/it/integration.rs:6893` (`workflow_run_with_vault_only_openai_codex_builds_pr_body`) — updated to destructure the tuple, return structured-output JSON from the OpenAI mock, and assert `title == \"Vault title\"`.\n- `lib/crates/fabro-server/src/server/tests.rs:3668` (`create_run_pull_request_creates_and_persists_record`) — updated the OpenAI responses mock to return structured-output JSON instead of plain text.\n\n### Verification\n\n- `cargo build --workspace` ✅\n- `cargo nextest run -p fabro-workflow` ✅ (1057/1057 pass, 34 skipped)\n- `cargo nextest run -p fabro-server` ✅ (441/443 pass; the two `*_returns_svg` failures are pre-existing and unrelated — confirmed by reproducing them on the parent commit `dc09842a`)\n- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` ✅\n- `cargo +nightly-2026-04-14 fmt --check --all` ✅\n\n### Out of scope items (per plan) — not changed\n\n- No `pull_request.prompt_preset`/`prompt_override` config.\n- No separate `pull_request.model` override.\n- No dynamic budgeting based on tokenizer math (two-tier static lookup as planned).\n- No stage-response inclusion.\n- Goal truncation is silent — full goal is *not* attached as a `
` block (option A).\n- `assemble_pr_body`, `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `truncate_pr_body`, `load_pull_request_diff`, `pr_title_from_goal` (with its 120-char cap), `OpenPullRequestRequest`, `PullRequestRecord`, `AutoMergeOptions`, the 65,536-char body cap, and the `_(truncated)_` suffix are unchanged." + }, + "notes": "Stage completed: implement", + "usage": { + "input": { + "usage": { + "model": { + "provider": "anthropic", + "model_id": "claude-opus-4-7" + }, + "tokens": { + "input_tokens": 122862, + "output_tokens": 32133, + "reasoning_tokens": 0, + "cache_read_tokens": 8222989, + "cache_write_tokens": 134698 + } + }, + "facts": { + "provider": "anthropic", + "cache_write_5m_tokens": 134698, + "cache_write_1h_tokens": 0 + } + }, + "total_usd_micros": 6370991 + }, + "files_touched": [ + "/home/daytona/workspace/lib/crates/fabro-server/src/server/tests.rs", + "/home/daytona/workspace/lib/crates/fabro-workflow/src/pipeline/pull_request.rs", + "/home/daytona/workspace/lib/crates/fabro-workflow/tests/it/integration.rs" + ] + }, + "simplify_gpt": { + "status": "succeeded", + "context_updates": { + "last_stage": "simplify_gpt", + "last_response": "Implemented cleanup fixes after review.\n\nNotes:\n- I launched the three review agents twice in parallel, but all six subagents failed with an internal LLM item-persistence error before returning findin", + "response.simplify_gpt": "Implemented cleanup fixes after review.\n\nNotes:\n- I launched the three review agents twice in parallel, but all six subagents failed with an internal LLM item-persistence error before returning findings. I proceeded with the review manually.\n\nFixed:\n- Removed the extra public `PrContent` struct and restored the requested `Result<(String, String), String>` return shape for `build_pr_body*`.\n- Updated callers/tests to destructure `(title, body)` instead of using a new public wrapper type.\n- Made `maybe_open_pull_request` apply `enforce_title_cap` unconditionally after selecting LLM title vs. fallback title.\n- Changed prompt input truncation helper to actually cap by Unicode scalar count instead of byte count while preserving UTF-8 safety.\n- Strengthened fallback PR tests to assert both OpenAI and GitHub mocks were called exactly once.\n- Clarified the empty-body test comment so it doesn’t falsely claim local mock providers perform schema `minLength` validation.\n\nValidated:\n- `cargo check -q -p fabro-workflow`\n- `cargo check -q -p fabro-server`\n- `cargo nextest run -p fabro-workflow pull_request`\n- `cargo nextest run -p fabro-workflow workflow_run_with_vault_only_openai_codex_builds_pr_body`\n- `cargo nextest run -p fabro-server create_run_pull_request_creates_and_persists_record`\n- `cargo +nightly-2026-04-14 clippy -q -p fabro-workflow --all-targets -- -D warnings`\n- `cargo +nightly-2026-04-14 fmt --check --all`" + }, + "notes": "Stage completed: simplify_gpt", + "usage": { + "input": { + "usage": { + "model": { + "provider": "openai", + "model_id": "gpt-5.5" + }, + "tokens": { + "input_tokens": 6208977, + "output_tokens": 13260, + "reasoning_tokens": 7138, + "cache_read_tokens": 6068224, + "cache_write_tokens": 0 + } + }, + "facts": { + "provider": "open_ai" + } + }, + "total_usd_micros": 34690937 + } + }, + "start": { + "status": "succeeded", + "usage": null + }, + "toolchain": { + "status": "succeeded", + "context_updates": { + "command.output": "blob://sha256/fc14b2ba2d770e5cd3169df7a29525c962adfc4cfa3097b9098c63ebd61a748c", + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126" + }, + "notes": "Script completed: command -v cargo >/dev/null || { curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y && sudo ln -sf $HOME/.cargo/bin/* /usr/local/bin/; }; cargo --version 2>&1", + "usage": null + } + }, + "next_node_id": "verify", + "git_commit_sha": "fdb007c032a4bbf9b71c5af71cee5397c2864fbb", + "node_visits": { + "simplify_opus": 1, + "start": 1, + "implement": 1, + "toolchain": 1, + "simplify_gpt": 1, + "preflight_compile": 1, + "preflight_lint": 1 + } + } ] ], "conclusion": null, @@ -1270,45 +1481,6 @@ "live_streaming": false, "termination": "exited" }, - "simplify_opus@1": { - "first_event_seq": 320, - "prompt": null, - "response": null, - "completion": { - "outcome": "succeeded", - "notes": "Stage completed: simplify_opus", - "failure_reason": null, - "timestamp": "2026-05-04T19:05:56.363262Z" - }, - "provider_used": { - "mode": "agent", - "provider": "anthropic", - "model": "claude-opus-4-7" - }, - "diff": null, - "script_invocation": null, - "script_timing": null, - "parallel_results": null, - "stdout": null, - "stderr": null - }, - "simplify_gpt@1": { - "first_event_seq": 647, - "prompt": null, - "response": null, - "completion": null, - "provider_used": { - "mode": "agent", - "provider": "openai", - "model": "gpt-5.5" - }, - "diff": null, - "script_invocation": null, - "script_timing": null, - "parallel_results": null, - "stdout": null, - "stderr": null - }, "implement@1": { "first_event_seq": 49, "prompt": null, @@ -1368,6 +1540,85 @@ "live_streaming": true, "termination": "exited" }, + "simplify_opus@1": { + "first_event_seq": 320, + "prompt": null, + "response": null, + "completion": { + "outcome": "succeeded", + "notes": "Stage completed: simplify_opus", + "failure_reason": null, + "timestamp": "2026-05-04T19:05:56.363262Z" + }, + "provider_used": { + "mode": "agent", + "provider": "anthropic", + "model": "claude-opus-4-7" + }, + "diff": null, + "script_invocation": null, + "script_timing": null, + "parallel_results": null, + "stdout": null, + "stderr": null + }, + "start@1": { + "first_event_seq": 15, + "prompt": null, + "response": null, + "completion": { + "outcome": "succeeded", + "notes": null, + "failure_reason": null, + "timestamp": "2026-05-04T18:34:56.389351Z" + }, + "provider_used": null, + "diff": null, + "script_invocation": null, + "script_timing": null, + "parallel_results": null, + "stdout": null, + "stderr": null + }, + "simplify_gpt@1": { + "first_event_seq": 647, + "prompt": null, + "response": null, + "completion": { + "outcome": "succeeded", + "notes": "Stage completed: simplify_gpt", + "failure_reason": null, + "timestamp": "2026-05-04T19:15:07.168752Z" + }, + "provider_used": { + "mode": "agent", + "provider": "openai", + "model": "gpt-5.5" + }, + "diff": null, + "script_invocation": null, + "script_timing": null, + "parallel_results": null, + "stdout": null, + "stderr": null + }, + "verify@1": { + "first_event_seq": 959, + "prompt": null, + "response": null, + "completion": null, + "provider_used": null, + "diff": null, + "script_invocation": { + "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" + }, + "script_timing": null, + "parallel_results": null, + "stdout": null, + "stderr": null + }, "preflight_compile@1": { "first_event_seq": 29, "prompt": null, @@ -1404,24 +1655,6 @@ "streams_separated": true, "live_streaming": false, "termination": "exited" - }, - "start@1": { - "first_event_seq": 15, - "prompt": null, - "response": null, - "completion": { - "outcome": "succeeded", - "notes": null, - "failure_reason": null, - "timestamp": "2026-05-04T18:34:56.389351Z" - }, - "provider_used": null, - "diff": null, - "script_invocation": null, - "script_timing": null, - "parallel_results": null, - "stdout": null, - "stderr": null } } } \ No newline at end of file diff --git a/stages/007-simplify_gpt@1/diff.patch b/stages/007-simplify_gpt@1/diff.patch new file mode 100644 index 000000000..a343c11e6 --- /dev/null +++ b/stages/007-simplify_gpt@1/diff.patch @@ -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 `
`, 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 { ++) -> 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, +-) -> Result { ++) -> 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 { ++) -> 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, + run_state: Option<&fabro_store::RunProjection>, +-) -> Result { ++) -> 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("Full plan")); + 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, + 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; + } + diff --git a/stages/007-simplify_gpt@1/status.json b/stages/007-simplify_gpt@1/status.json new file mode 100644 index 000000000..04727fced --- /dev/null +++ b/stages/007-simplify_gpt@1/status.json @@ -0,0 +1,6 @@ +{ + "outcome": "succeeded", + "notes": "Stage completed: simplify_gpt", + "failure_reason": null, + "timestamp": "2026-05-04T19:15:07.168752Z" +} \ No newline at end of file diff --git a/stages/008-verify@1/script_invocation.json b/stages/008-verify@1/script_invocation.json new file mode 100644 index 000000000..b849f4af1 --- /dev/null +++ b/stages/008-verify@1/script_invocation.json @@ -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" +} \ No newline at end of file