From 56cb965d8f29f315512f29a30f5ea01898e2415b Mon Sep 17 00:00:00 2001 From: shiva Date: Sun, 9 Aug 2026 20:33:33 +0530 Subject: [PATCH] fix: PR opened by item done uses the agent's own summary, not a placeholder push_and_open_pr hardcoded "Auto-opened on \`item done\` for {id}." as every PR body, regardless of whether real information about the change was available. agentflare work already parses the headless agent's own final reply (parse_claude_reply) -- its prompt explicitly asks the agent to "summarize what you changed and why" -- but that text only ever reached the item as a comment, never the PR itself. Reviewers opened these PRs cold, with a generic placeholder as the only description, no matter how good the agent's own summary was. - types.rs: ItemRequest gains an optional `summary` field, scoped to `done` (mirrors the existing `push` field's scoping), documented in the item tool's own description. - worktree.rs: push_and_open_pr takes an optional summary and uses it as the PR body when non-blank, falling back to the old placeholder otherwise. Extracted the body-construction logic into a small pure pr_body() so it's directly unit-testable without needing a real repo remote/gh client (push_and_open_pr itself soft-fails without one). - item.rs: item_done threads req.summary through to both push_and_open_pr call sites. - work.rs: agentflare work's own item_done call (the common path -- a headless run that just replies with text and lets the wrapper handle `done`) now passes the already-parsed reply_text as the summary. When the agent instead calls `done` itself mid-session (as item #43's PR #417 did), it can pass its own `summary` directly per the updated tool description. Agentflare-Agent: claude-code Agentflare-Branch: fix-pr-summary-uses-agent-reply --- src/cli/work.rs | 8 +++++++ src/mcp_server.rs | 2 +- src/mcp_server/item.rs | 11 +++++++-- src/mcp_server/types.rs | 5 ++++ src/worktree.rs | 52 ++++++++++++++++++++++++++++++++++++++++- 5 files changed, 74 insertions(+), 4 deletions(-) diff --git a/src/cli/work.rs b/src/cli/work.rs index 724a126c..cef1af43 100644 --- a/src/cli/work.rs +++ b/src/cli/work.rs @@ -506,9 +506,17 @@ pub(crate) fn execute_work(args: WorkArgs, log: &mut dyn std::io::Write) -> i32 (reply, None, None) }; + // The agent may already have called `done` itself with its own + // `summary` (in which case this second call is a no-op — the + // claim is already released) -- but the common case is a + // headless run that just replies with text and lets this + // wrapper handle `done`, so pass the parsed reply through as + // the PR body rather than leaving it as the generic + // placeholder. let done_resp = match mcp.item_done(ItemRequest { action: "done".into(), id: Some(item_id.into()), + summary: Some(reply_text.clone()), ..Default::default() }) { Ok(j) => j, diff --git a/src/mcp_server.rs b/src/mcp_server.rs index 6a24af87..35588c8a 100644 --- a/src/mcp_server.rs +++ b/src/mcp_server.rs @@ -1392,7 +1392,7 @@ impl AgentflareMcp { } #[tool( - description = "Manage work items in the repo's linked project. Single consolidated tool with `action` field (create|get|list|search|update|update_state|delete|claim|heartbeat|release|done|check_merge|cancel|add_label|remove_label|groom|standup|health). `groom` returns a priority+staleness-ranked shortlist with description, stale/unassigned/blocked/duplicate flags, and a pull_next list — all in one call, no per-item `get` round trips needed. `standup` returns done/in_progress(grouped by assignee)/stuck buckets computed server-side. `health` returns a velocity/WIP/stuck scorecard; `bottlenecks` is currently always empty — no handoff log is persisted yet, see `bottleneck_note`. `done` moves an item to \"in_review\" (not \"completed\") when it results in an open PR, and leaves the worktree in place for follow-up commits; call `check_merge` once the PR is confirmed merged to promote it to \"completed\" and clean up the worktree." + description = "Manage work items in the repo's linked project. Single consolidated tool with `action` field (create|get|list|search|update|update_state|delete|claim|heartbeat|release|done|check_merge|cancel|add_label|remove_label|groom|standup|health). `groom` returns a priority+staleness-ranked shortlist with description, stale/unassigned/blocked/duplicate flags, and a pull_next list — all in one call, no per-item `get` round trips needed. `standup` returns done/in_progress(grouped by assignee)/stuck buckets computed server-side. `health` returns a velocity/WIP/stuck scorecard; `bottlenecks` is currently always empty — no handoff log is persisted yet, see `bottleneck_note`. `done` moves an item to \"in_review\" (not \"completed\") when it results in an open PR, and leaves the worktree in place for follow-up commits; call `check_merge` once the PR is confirmed merged to promote it to \"completed\" and clean up the worktree. Pass `summary` on `done` with what you changed and why — it becomes the PR body; omitting it leaves the PR with a generic placeholder description." )] fn item(&self, Parameters(req): Parameters) -> Result { self.item_inner(req) diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index 0eefdc97..89eebc97 100644 --- a/src/mcp_server/item.rs +++ b/src/mcp_server/item.rs @@ -607,13 +607,20 @@ impl AgentflareMcp { Ok::<_, ErrorData>((item_id, owns_claim, item, target_branch)) })??; let should_push = req.push.unwrap_or(true); + let summary = req.summary.as_deref(); let pr_url = match (&item, &target_branch) { (Some(item), Some(target)) if should_push => PROGRESS_SENDER .try_with(|ps| { - crate::worktree::push_and_open_pr(item, &repo_root, target, ps.as_ref()) + crate::worktree::push_and_open_pr( + item, + &repo_root, + target, + ps.as_ref(), + summary, + ) }) .unwrap_or_else(|_| { - crate::worktree::push_and_open_pr(item, &repo_root, target, None) + crate::worktree::push_and_open_pr(item, &repo_root, target, None, summary) }), _ => None, }; diff --git a/src/mcp_server/types.rs b/src/mcp_server/types.rs index f7b34807..2d6e8360 100644 --- a/src/mcp_server/types.rs +++ b/src/mcp_server/types.rs @@ -782,6 +782,11 @@ pub(crate) struct ItemRequest { )] #[serde(default)] pub(crate) push: Option, + #[schemars( + description = "done only: markdown summary of what changed and why — becomes the PR body when a PR opens (falls back to a generic placeholder if omitted). Write this before calling done, not after." + )] + #[serde(default)] + pub(crate) summary: Option, } /// Lean per-item projection for `item(list)` — the raw 19-field `Item` (full diff --git a/src/worktree.rs b/src/worktree.rs index 86e8a109..7a308d6b 100644 --- a/src/worktree.rs +++ b/src/worktree.rs @@ -77,6 +77,18 @@ pub fn is_pr_merged(item: &agentflare_backend::item::Item, repo_root: &Path) -> } } +/// The PR body: `summary` (the agent's own "what changed and why", or an +/// explicit `summary` on the `done` call) when it's real content, else the +/// old generic placeholder. A real summary makes for a far more reviewable +/// PR than the placeholder — reviewers previously had to open the diff +/// cold, with no idea what the change was even trying to do. +fn pr_body(item_id: &str, summary: Option<&str>) -> String { + match summary.map(str::trim).filter(|s| !s.is_empty()) { + Some(s) => s.to_string(), + None => format!("Auto-opened on `item done` for {item_id}."), + } +} + /// Pushes `item`'s isolated worktree branch and opens a PR against /// `target_branch` — the `done`-side counterpart to `create_worktree`. /// Deliberately never merges: unreviewed code should never land on the @@ -90,6 +102,7 @@ pub fn push_and_open_pr( repo_root: &Path, target_branch: &str, progress: Option<&ProgressSender>, + summary: Option<&str>, ) -> Option { let branch = flare_git_core::worktree::push_branch( item, @@ -100,7 +113,7 @@ pub fn push_and_open_pr( if let Some(p) = progress { p.send(0.5, Some(1.0), Some("Creating PR...".into())); } - let body = format!("Auto-opened on `item done` for {}.", item.id); + let body = pr_body(&item.id, summary); let repo = match RepoId::resolve_from_remote(repo_root) { Some(r) => r, None => { @@ -165,3 +178,40 @@ pub fn push_and_open_pr( } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn pr_body_uses_the_summary_when_given_one() { + assert_eq!( + pr_body("item-1", Some("Fixed the race by adding a mutex.")), + "Fixed the race by adding a mutex." + ); + } + + #[test] + fn pr_body_trims_the_summary() { + assert_eq!( + pr_body("item-1", Some(" Fixed the race. \n")), + "Fixed the race." + ); + } + + #[test] + fn pr_body_falls_back_to_the_placeholder_when_summary_is_none() { + assert_eq!( + pr_body("item-1", None), + "Auto-opened on `item done` for item-1." + ); + } + + #[test] + fn pr_body_falls_back_to_the_placeholder_when_summary_is_blank() { + assert_eq!( + pr_body("item-1", Some(" ")), + "Auto-opened on `item done` for item-1." + ); + } +}