diff --git a/run.json b/run.json index b323df9ae..3bbabcb3c 100644 --- a/run.json +++ b/run.json @@ -505,13 +505,14 @@ "status_updated_at": "2026-05-04T18:34:54.323999Z", "pending_control": null, "checkpoint": { - "timestamp": "2026-05-04T18:39:22.661150Z", - "current_node": "preflight_lint", + "timestamp": "2026-05-04T18:54:35.317138Z", + "current_node": "implement", "completed_nodes": [ "start", "toolchain", "preflight_compile", - "preflight_lint" + "preflight_lint", + "implement" ], "node_retries": {}, "context_values": { @@ -521,12 +522,17 @@ "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", "graph.model_stylesheet": "\n * { model: claude-opus-4-7; }\n ", "internal.retry_count.preflight_compile": 0, - "internal.thread_id": "preflight_compile", + "thread.preflight_lint.current_node": "implement", + "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", + "internal.thread_id": "preflight_lint", "thread.toolchain.current_node": "preflight_compile", + "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.retry_count.implement": 0, "graph.rankdir": "LR", "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", "failure_signature": "", - "current_node": "preflight_lint", + "current_node": "implement", "internal.retry_count.preflight_lint": 0, "internal.fidelity": "compact", "internal.retry_count.start": 0, @@ -556,6 +562,43 @@ "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 }, + "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" + ] + }, "start": { "status": "succeeded", "usage": null @@ -570,12 +613,13 @@ "usage": null } }, - "next_node_id": "implement", + "next_node_id": "simplify_opus", "node_visits": { "preflight_lint": 1, "start": 1, "preflight_compile": 1, - "toolchain": 1 + "toolchain": 1, + "implement": 1 } }, "checkpoints": [ @@ -731,6 +775,84 @@ "toolchain": 1 } } + ], + [ + 46, + { + "timestamp": "2026-05-04T18:39:26.876510Z", + "current_node": "preflight_lint", + "completed_nodes": [ + "start", + "toolchain", + "preflight_compile", + "preflight_lint" + ], + "node_retries": {}, + "context_values": { + "graph.model_stylesheet": "\n * { model: claude-opus-4-7; }\n ", + "failure_signature": "", + "internal.retry_count.start": 0, + "failure_class": "", + "internal.retry_count.toolchain": 0, + "internal.run_id": "01KQT1VKB74S0N423QFMFCY3EB", + "thread.preflight_compile.current_node": "preflight_lint", + "thread.toolchain.current_node": "preflight_compile", + "internal.thread_id": "preflight_compile", + "internal.retry_count.preflight_compile": 0, + "internal.fidelity": "compact", + "internal.node_visit_count": 1, + "graph.rankdir": "LR", + "internal.work_dir": "/home/daytona/workspace", + "outcome": "succeeded", + "thread.start.current_node": "toolchain", + "command.output": "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, + "command.stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "current_node": "preflight_lint" + }, + "node_outcomes": { + "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 + }, + "start": { + "status": "succeeded", + "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 + }, + "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": "implement", + "git_commit_sha": "dc09842aac7906c19fb88c56b925f8a46bae98a1", + "node_visits": { + "start": 1, + "toolchain": 1, + "preflight_compile": 1, + "preflight_lint": 1 + } + } ] ], "conclusion": null, @@ -754,7 +876,12 @@ "first_event_seq": 39, "prompt": null, "response": null, - "completion": null, + "completion": { + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-05-04T18:39:22.660548Z" + }, "provider_used": null, "diff": null, "script_invocation": { @@ -762,6 +889,38 @@ "command": "cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", "language": "shell" }, + "script_timing": { + "stdout": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 133002, + "termination": "exited", + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false + }, + "parallel_results": null, + "stdout": null, + "stderr": null, + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false, + "termination": "exited" + }, + "implement@1": { + "first_event_seq": 49, + "prompt": null, + "response": null, + "completion": null, + "provider_used": { + "mode": "agent", + "provider": "anthropic", + "model": "claude-opus-4-7" + }, + "diff": null, + "script_invocation": null, "script_timing": null, "parallel_results": null, "stdout": null, diff --git a/stages/004-preflight_lint@1/script_timing.json b/stages/004-preflight_lint@1/script_timing.json new file mode 100644 index 000000000..c6a0b3f2a --- /dev/null +++ b/stages/004-preflight_lint@1/script_timing.json @@ -0,0 +1,11 @@ +{ + "stdout": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "stderr": "blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126", + "exit_code": 0, + "duration_ms": 133002, + "termination": "exited", + "stdout_bytes": 0, + "stderr_bytes": 0, + "streams_separated": true, + "live_streaming": false +} \ No newline at end of file diff --git a/stages/004-preflight_lint@1/status.json b/stages/004-preflight_lint@1/status.json new file mode 100644 index 000000000..0938b92f3 --- /dev/null +++ b/stages/004-preflight_lint@1/status.json @@ -0,0 +1,6 @@ +{ + "outcome": "succeeded", + "notes": "Script completed: cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1", + "failure_reason": null, + "timestamp": "2026-05-04T18:39:22.660548Z" +} \ No newline at end of file diff --git a/stages/004-preflight_lint@1/stderr.log b/stages/004-preflight_lint@1/stderr.log new file mode 100644 index 000000000..d87ba9545 --- /dev/null +++ b/stages/004-preflight_lint@1/stderr.log @@ -0,0 +1 @@ +blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126 \ No newline at end of file diff --git a/stages/004-preflight_lint@1/stdout.log b/stages/004-preflight_lint@1/stdout.log new file mode 100644 index 000000000..d87ba9545 --- /dev/null +++ b/stages/004-preflight_lint@1/stdout.log @@ -0,0 +1 @@ +blob://sha256/12ae32cb1ec02d01eda3581b127c1fee3b0dc53572ed6baf239721a03d82e126 \ No newline at end of file diff --git a/stages/005-implement@1/prompt.md b/stages/005-implement@1/prompt.md new file mode 100644 index 000000000..a6f802bda --- /dev/null +++ b/stages/005-implement@1/prompt.md @@ -0,0 +1,321 @@ +Goal: # Compound-engineering PR title and body recipe for Fabro + +## Context + +Today 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. + +We 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). + +## Approach + +Replace 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). + +The 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). + +## Critical files + +- `lib/crates/fabro-workflow/src/pipeline/pull_request.rs` — primary; replace prompt + signatures +- `lib/crates/fabro-workflow/tests/it/integration.rs:6893` — one test that calls `build_pr_body` directly; signature update + +## Reuse (do not reimplement) + +- `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. +- `assemble_pr_body`, `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `truncate_pr_body`, `load_pull_request_diff` — unchanged. +- `pr_title_from_goal`, `strip_goal_decoration` — unchanged; demoted from "always-used" to "fallback path." + +## Changes + +### 1. New schema and typed struct + +Add at module top: + +```rust +use std::sync::LazyLock; + +#[derive(Debug, serde::Deserialize)] +struct GeneratedPrContent { + title: String, + body: String, +} + +static PR_CONTENT_SCHEMA: LazyLock = LazyLock::new(|| { + serde_json::json!({ + "type": "object", + "properties": { + "title": { "type": "string", "maxLength": 72 }, + "body": { "type": "string", "minLength": 1 } + }, + "required": ["title", "body"], + "additionalProperties": false + }) +}); +``` + +**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. + +### 2. New system prompt as a `const &str` + +Add as a module-level `const PR_BODY_SYSTEM_PROMPT: &str = "..."`. Verbatim content: + +```text +You are writing a pull request title and description for a code change produced by an AI workflow. + +OUTPUT FORMAT +Return a JSON object with exactly two fields: +- "title": a one-line title, max 72 characters, no trailing period. +- "body": the markdown body as described below. + +DO NOT INCLUDE in the body +- A `#` or `##` title heading at the top — the title goes in the `title` field. +- A "Retro" section, "Fabro Details" section, cost/duration table, or "Generated with" footer — those are appended programmatically after your output. +- The full plan text — the full plan is appended programmatically as a
block. +- Bare `#1`, `#2` list prefixes — GitHub auto-links those as issue references. Use plain `1.`, `2.` instead. +- A test plan unless the testing approach is non-obvious. + +SIZE THE BODY TO THE CHANGE +First classify along two axes from the diff: +- Size: how many files changed, how large the diff is. +- Complexity: trivial (rename / typo / dep bump / config) vs. design decisions / new patterns / cross-cutting concerns. + +Then write at the matching depth: + +| Profile | Body shape | +|---|---| +| Small + simple (typo, config, dep bump) | 1–2 sentences, no headers, total under ~300 characters | +| Small + non-trivial (targeted bugfix, behavioral change) | Short "Problem / Fix" narrative, 3–5 sentences. No headers unless two distinct concerns. | +| Medium feature or refactor | Summary paragraph, then a section explaining what changed and why. Call out design decisions. | +| Large or architecturally significant | Full narrative: problem context, approach chosen (and why), key decisions, migration/rollback notes if relevant. | +| Performance improvement | Include before/after measurements if available. A markdown table works well here. | + +Brevity matters for small changes. A 3-line bugfix with a 20-line description signals miscalibration. When in doubt, shorter is better — reviewers can read the diff. + +WRITING PRINCIPLES +- Lead with value: the first sentence tells the reviewer *why this PR exists*, not *what files changed*. +- Describe the net result, not the journey: skip intermediate failures, debugging steps, and refactors done during development. +- Trust the final diff: if the goal or plan disagree with the diff, the diff is authoritative. +- Explain the non-obvious: spend description space on what the diff doesn't show — why this approach, what was rejected, what to look at first. +- Use structure when it earns its keep: no empty sections, no template headers without content. +- If the body uses any `##` heading, the opening summary must also be under a heading (e.g. `## Summary`); otherwise a bare paragraph is fine. + +PLAN SUMMARY +The full plan is attached separately as a
block, so do not restate it. Include a brief `### Plan Summary` with bullet points only when the change is medium or larger in the sizing matrix above. Skip it for small changes. + +VISUAL AIDS +Include a visual aid only when a reviewer would struggle to reconstruct the mental model from prose alone — based on what changes structurally, not on PR size. Skip for trivial / mechanical changes, or when prose already communicates clearly. + +| PR changes... | Visual aid | +|---|---| +| 3+ interacting components or services | Mermaid component / interaction diagram | +| Multi-step workflow or pipeline with non-obvious sequencing | Mermaid flow diagram | +| 3+ behavioral modes or variants | Markdown comparison table | +| Before/after data or trade-offs | Markdown table | +| Data model changes with 3+ related entities | Mermaid ERD | + +Mermaid: prefer `TB` direction, ≤10 nodes typical. Place inline at the point of relevance, not in a separate "Diagrams" section. +``` + +### 3. Truncation constants and small-model fallback + +Replace inline `50_000` and `20_000` with two tiered constant sets: + +```rust +// Generous tier (≥200k context window) +const MAX_GOAL_CHARS_LARGE: usize = 75_000; +const MAX_PLAN_CHARS_LARGE: usize = 75_000; +const MAX_DIFF_CHARS_LARGE: usize = 250_000; + +// Conservative tier (matches the previous values) +const MAX_GOAL_CHARS_SMALL: usize = 20_000; +const MAX_PLAN_CHARS_SMALL: usize = 20_000; +const MAX_DIFF_CHARS_SMALL: usize = 50_000; +``` + +Resolve the tier by looking up the model in `fabro_model::Catalog::builtin()`: + +```rust +fn truncation_caps(model: &str) -> (usize, usize, usize) { + let large_enough = Catalog::builtin() + .get(model) + .is_some_and(|m| m.limits.context_window >= 200_000); + if large_enough { + (MAX_GOAL_CHARS_LARGE, MAX_PLAN_CHARS_LARGE, MAX_DIFF_CHARS_LARGE) + } else { + (MAX_GOAL_CHARS_SMALL, MAX_PLAN_CHARS_SMALL, MAX_DIFF_CHARS_SMALL) + } +} +``` + +Unknown 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. + +For 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`). + +Goal truncation is silent (no log, no marker). The full goal is *not* attached to the rendered body. + +### 4. Refactor `build_pr_body*` to return both title and body + +Signature change for the three internal builders and the public `build_pr_body`: + +```rust +pub async fn build_pr_body( + diff: &str, goal: &str, model: &str, + run_store: &RunStoreHandle, + llm_source: &dyn CredentialSource, + conclusion: Option<&Conclusion>, +) -> Result<(String, String), String> // was: Result +``` + +Returns `(title, body)`. Callers destructure. + +`build_pr_body_with_client_and_state` becomes: + +1. Truncate `goal`, `diff`, `plan_text` against the tier from `truncation_caps(model)`. +2. Build user prompt with `Goal:` / `Plan:` (when present) / `Diff:` sections. +3. Call `generate_object(params.system(PR_BODY_SYSTEM_PROMPT).prompt(user_prompt), PR_CONTENT_SCHEMA.clone())`. +4. 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. +5. 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. + +**Failure modes — explicit:** + +| Condition | Outcome | +|---|---| +| `generate_object` returns Err (LLM call failure, schema validation failure, JSON parse failure) | Return Err. Caller logs and emits `PullRequestFailed`; no PR is opened. | +| `result.output` is `None` | Return Err. Same as above. | +| Deserialize as `GeneratedPrContent` fails (missing required field, wrong type) | Return Err. Same as above. | +| 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. | +| Title is empty (after trim) | **Allowed through** — return `("", body)`. The caller is responsible for the deterministic title fallback. | +| Title is non-empty but >72 chars | Allowed through — `enforce_title_cap` truncates with `…`. | + +The 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. + +### 5. `maybe_open_pull_request` invokes the fallback when the title is empty + +Today 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: + +```rust +let (llm_title, body) = build_pr_body_with_source_and_state(...).await + .map_err(|err| format!("{err:#}"))?; // any non-title failure: fatal + +let title = if llm_title.trim().is_empty() { + pr_title_from_goal(req.goal) // fallback path: only reached when body succeeded +} else { + llm_title +}; +let title = enforce_title_cap(&title); // unconditional 72-char guarantee, covers fallback path +let body = truncate_pr_body(&body); // unchanged 65,536-char hard cap +``` + +The 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. + +The 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. + +### 6. Title cap helper + +Add a small private helper: + +```rust +fn enforce_title_cap(title: &str) -> String { + const MAX: usize = 72; + if title.chars().count() > MAX { + let truncated: String = title.chars().take(MAX - 1).collect(); + format!("{truncated}\u{2026}") + } else { + title.to_string() + } +} +``` + +Applied 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). + +### 7. Test updates + +In `lib/crates/fabro-workflow/src/pipeline/pull_request.rs`: + +- `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. +- `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."}`. +- `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. +- `empty_diff_returns_none` — unchanged behavior, still returns `Ok(None)` from `maybe_open_pull_request`. +- `pr_title_from_goal` tests (10 of them, lines 1416–1485) — unchanged; the function still exists as a fallback. +- 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: + - **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. + - **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`. + - **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). + - **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.) + - 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()`. + - Pass a non-empty diff so the early-return at `pull_request.rs:460` doesn't fire. + - 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). + - 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). + - 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. + +- 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 `…`. +- 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.) +- New test: `build_pr_body_returns_err_when_body_blank` — two cases via parameterized assertion or two `#[test]`s sharing a helper: + - MockProvider returns `{"title":"Mock","body":""}` — schema rejects via `minLength: 1`, builder returns `Err`. + - MockProvider returns `{"title":"Mock","body":" \n"}` — schema accepts (length ≥ 1), Rust trim-check returns `Err`. + Both cases validate the blank-body-is-fatal contract from §4's failure-mode table. + +In `lib/crates/fabro-workflow/tests/it/integration.rs`: + +- `workflow_run_with_vault_only_openai_codex_builds_pr_body` (line 6893 area) — destructure the tuple, update assertions. + +### 8. What is *not* changing + +- `assemble_pr_body` — body still slots in front of the four programmatic sections in the same order. +- `format_retro_section`, `format_arc_details_section`, `read_plan_text`, `parse_dot_summary`, `format_duration_ms`, `format_cost`, `truncate_pr_body` — unchanged. +- `OpenPullRequestRequest`, `PullRequestRecord`, `AutoMergeOptions` — unchanged shapes. +- 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. +- The 65,536-char body cap and `_(truncated)_` suffix. + +## Verification + +```sh +cargo build --workspace +cargo nextest run -p fabro-workflow +cargo nextest run -p fabro-server +cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings +cargo +nightly-2026-04-14 fmt --check --all +``` + +End-to-end (optional, requires credentials): + +```sh +set -a && source .env && set +a +cargo nextest run -p fabro-workflow --profile e2e --run-ignored only +``` + +Manual smoke: run any workflow that opens a PR and inspect the resulting PR title and body on GitHub. Confirm: + +- Title ≤ 72 chars, no markdown decoration. +- Body opens with a value-first sentence (per "lead with value" principle). +- Body does *not* contain a duplicate Retro / Fabro Details / footer / full-plan section. +- Trailing sections render as before: Plan `
`, `### Retro`, `### Fabro Details`, `⚒️ Generated with [Fabro]`. +- 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. + +## Out of scope (explicit) + +- 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. +- Stage-response inclusion (e.g. the implement node's response as commit-message analog). Worth doing later as a separate change. +- Configurable `pull_request.prompt_preset` or `prompt_override` in `PullRequestSettings`. Not needed for this iteration; the prompt is opinionated by design. +- Separate `pull_request.model` override (use Haiku/mini for body generation to cut cost). Defensible follow-up; out of scope here. +- Attaching the full goal as a `
` block (option B). User chose option A. +- Anthropic probe override to keep connectivity checks on Haiku after the Opus 4.7 default change. Separate concern, surfaced earlier. + + +## Completed stages +- **toolchain**: succeeded + - Script: `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` + - Stdout: + ``` + cargo 1.95.0 (f2d3ce0bd 2026-03-21) + ``` + - Stderr: (empty) +- **preflight_compile**: succeeded + - Script: `cargo check -q --workspace 2>&1` + - Stdout: (empty) + - Stderr: (empty) +- **preflight_lint**: succeeded + - Script: `cargo +nightly-2026-04-14 clippy -q --workspace --all-targets -- -D warnings 2>&1` + - Stdout: (empty) + - Stderr: (empty) + + +Read the plan file referenced in the goal and implement every step. Make all the code changes described in the plan. Use red/green TDD. \ No newline at end of file diff --git a/stages/005-implement@1/provider_used.json b/stages/005-implement@1/provider_used.json new file mode 100644 index 000000000..672cc50e5 --- /dev/null +++ b/stages/005-implement@1/provider_used.json @@ -0,0 +1,5 @@ +{ + "mode": "agent", + "provider": "anthropic", + "model": "claude-opus-4-7" +} \ No newline at end of file diff --git a/stages/005-implement@1/response.md b/stages/005-implement@1/response.md new file mode 100644 index 000000000..5243f580c --- /dev/null +++ b/stages/005-implement@1/response.md @@ -0,0 +1,51 @@ +Everything is green. Let me summarize what was done: + +## Summary + +Implemented the compound-engineering PR title and body recipe per plan: + +### Code changes (`lib/crates/fabro-workflow/src/pipeline/pull_request.rs`) + +1. **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`). + +2. **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. + +3. **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. + +4. **`enforce_title_cap(&str) -> String`** — 72-char Unicode-scalar cap, replacing the trailing char with `…`. + +5. **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. + +6. **`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. + +### Tests added (red→green) + +- `build_pr_body_truncates_long_title` — MockProvider returns 200-char title; assert `chars().count() == 72` and ends with `…`. +- `build_pr_body_returns_err_when_body_empty` — schema rejects `""` (minLength 1). +- `build_pr_body_returns_err_when_body_whitespace` — schema accepts `" \n"`, but the Rust trim-check rejects it; asserts the specific error message. +- `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. +- `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"`. +- `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. + +### Existing tests updated + +- 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. +- `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"`. +- `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. + +### Verification + +- `cargo build --workspace` ✅ +- `cargo nextest run -p fabro-workflow` ✅ (1057/1057 pass, 34 skipped) +- `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`) +- `cargo +nightly-2026-04-14 clippy --workspace --all-targets -- -D warnings` ✅ +- `cargo +nightly-2026-04-14 fmt --check --all` ✅ + +### Out of scope items (per plan) — not changed + +- No `pull_request.prompt_preset`/`prompt_override` config. +- No separate `pull_request.model` override. +- No dynamic budgeting based on tokenizer math (two-tier static lookup as planned). +- No stage-response inclusion. +- Goal truncation is silent — full goal is *not* attached as a `
` block (option A). +- `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. \ No newline at end of file