Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/cli/work.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
Comment on lines +509 to +519

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Reconcile agent-initiated done calls before relying on this second call.

If the agent calls done and opens a PR, src/mcp_server/item.rs keeps the claim held while the item is in_review (Lines 627-636). The wrapper therefore calls item_done again instead of treating it as a no-op. src/worktree.rs then returns the existing PR (Lines 146-151) without applying reply_text to its body.

If the first done omitted summary, the generic placeholder remains. Update the existing PR body or change the completion flow so the parsed reply is applied before the PR is opened. Correct the no-op comment.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/cli/work.rs` around lines 509 - 519, Update the completion flow around
the wrapper’s mcp.item_done call to reconcile an agent-initiated done before
relying on the second call: ensure reply_text is applied to an existing PR body
when the initial summary was omitted, or apply it before opening the PR. Correct
the nearby comment so it no longer claims the second call is a no-op while the
item remains in_review.

..Default::default()
}) {
Ok(j) => j,
Expand Down
2 changes: 1 addition & 1 deletion src/mcp_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<ItemRequest>) -> Result<String, ErrorData> {
self.item_inner(req)
Expand Down
11 changes: 9 additions & 2 deletions src/mcp_server/item.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
5 changes: 5 additions & 0 deletions src/mcp_server/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,11 @@ pub(crate) struct ItemRequest {
)]
#[serde(default)]
pub(crate) push: Option<bool>,
#[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<String>,
}

/// Lean per-item projection for `item(list)` — the raw 19-field `Item` (full
Expand Down
52 changes: 51 additions & 1 deletion src/worktree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -90,6 +102,7 @@ pub fn push_and_open_pr(
repo_root: &Path,
target_branch: &str,
progress: Option<&ProgressSender>,
summary: Option<&str>,
) -> Option<String> {
let branch = flare_git_core::worktree::push_branch(
item,
Expand All @@ -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 => {
Expand Down Expand Up @@ -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."
);
}
}
Loading