From 453bebe6b0771d05caf98f87070e6a92c82e43b3 Mon Sep 17 00:00:00 2001 From: shiva Date: Thu, 13 Aug 2026 15:05:42 +0530 Subject: [PATCH] fix: block item done from completing when push/PR creation fails on real commits Item #109: a real commit landed on the claimed branch and was pushed to origin, but gh pr create never resulted in a PR (soft-failed silently), and the item still went straight to completed. push_and_open_pr's soft-fail-and-return-None contract predates item #420's split of done, back when the item was already marked completed before this code ran. Now that the completed transition happens right here, a diverged branch with should_push set but no resulting PR must hard-error instead of falling through to mark_completed -- same treatment item #92 already gives an auto-commit failure on a dirty tree. Added a regression test using a local bare-repo origin (push succeeds, gh pr create is unreachable) to reproduce push-without-PR and confirm done returns a hard error, posts a comment, and leaves completed_at unset. item.rs crossed the 1500-line LOC gate as a result; allowlisted at the frozen 2000-line ceiling like the codebase's other over-limit files, since splitting its per-action handlers into submodules is separate, larger work than a completion-correctness fix should carry. Agentflare-Agent: claude-code Agentflare-Branch: task/109-agentflare-work-marks-an-item-done-even Agentflare-Item: 109 --- scripts/loc-gate.sh | 6 + src/mcp_server/item.rs | 30 +++-- src/mcp_server/tests/item_pr_failure_tests.rs | 115 ++++++++++++++++++ src/mcp_server/tests/mod.rs | 1 + 4 files changed, 144 insertions(+), 8 deletions(-) create mode 100644 src/mcp_server/tests/item_pr_failure_tests.rs diff --git a/scripts/loc-gate.sh b/scripts/loc-gate.sh index c07f19e71..009d77183 100644 --- a/scripts/loc-gate.sh +++ b/scripts/loc-gate.sh @@ -21,6 +21,12 @@ ALLOWLIST=( # small, unrelated fix shouldn't be blocked on splitting. Frozen at # <= FROZEN_LIMIT; a real split is separate work. src/cli/work.rs + # At 1493 lines on master, item #109's push/PR-failure completion gate + # (mirroring the existing auto-commit-failure gate right above it) pushed + # it over LIMIT. Splitting item.rs's per-action handlers into submodules + # is worth doing but is a separate, larger refactor than a completion- + # correctness fix should carry. Frozen at <= FROZEN_LIMIT. + src/mcp_server/item.rs ) cd "$(dirname "$0")/.." diff --git a/src/mcp_server/item.rs b/src/mcp_server/item.rs index b02f16ce8..539ae44f6 100644 --- a/src/mcp_server/item.rs +++ b/src/mcp_server/item.rs @@ -828,6 +828,10 @@ impl AgentflareMcp { // no-ops (logging) rather than trusting push/PR success alone as // proof nothing would be lost. let in_review = owns_claim && pr_url.is_some(); + let diverged = item + .as_ref() + .zip(target_branch.as_ref()) + .is_some_and(|(i, t)| crate::worktree::branch_diverged(i, &repo_root, t)); // No PR resulted — either nothing was ever committed on the claimed // branch, or a real commit's push/PR failed for some other reason. // Only the former should block completion: marking an item @@ -836,14 +840,24 @@ impl AgentflareMcp { // merely replies with text, with no tool use, previously exited 0 // and sailed straight through to `mark_completed` below with zero // code changed. - let nothing_was_ever_committed = !in_review - && owns_claim - && item - .as_ref() - .zip(target_branch.as_ref()) - .is_none_or(|(item, target)| { - !crate::worktree::branch_diverged(item, &repo_root, target) - }); + let nothing_was_ever_committed = !in_review && owns_claim && !diverged; + // A real commit exists but push/PR creation soft-failed and never + // produced a PR (item #109) -- hard-error instead of completing. + let push_or_pr_failed = !in_review && owns_claim && should_push && diverged; + if push_or_pr_failed { + let _ = self.comment_impl(CommentRequest { + action: "create".into(), + item_id: Some(item_id.clone()), + body: Some(format!( + "## agentflare work — PR creation failed\n\nThe branch has real commits but no pull request resulted; check server logs for item {item_id}. Left in place rather than completed." + )), + ..Default::default() + }); + return Err(ErrorData::internal_error( + format!("item {item_id}: real commits but no PR resulted -- not marking completed"), + None, + )); + } // Shared by both "nothing was ever committed" (worktree is clean by // definition, safe to remove) and a real completion: release the // lease so the item is genuinely available again — either for a diff --git a/src/mcp_server/tests/item_pr_failure_tests.rs b/src/mcp_server/tests/item_pr_failure_tests.rs new file mode 100644 index 000000000..784bc5c04 --- /dev/null +++ b/src/mcp_server/tests/item_pr_failure_tests.rs @@ -0,0 +1,115 @@ +#[allow(unused_imports)] +use super::*; + +// Item #109: a real commit landed on the claimed branch and `git push` +// itself succeeded, but no PR resulted (here: `origin` is a local bare +// repo, so `RepoId::resolve_from_remote` can't recognize it as a GitHub +// remote and `push_and_open_pr` skips PR creation -- the same soft-fail +// path a missing `gh` credential or a `gh pr create` error would take). +// `done` must not fall through to `mark_completed`: a pushed branch with +// no PR anywhere is not "in_review", and it's certainly not "completed". +#[test] +fn item_done_reports_a_hard_error_when_push_succeeds_but_no_pr_results() { + let tmp = tempfile::tempdir().unwrap(); + let repo_dir = tempfile::tempdir().unwrap(); + let repo_root = repo_dir.path().to_path_buf(); + let origin_dir = tempfile::tempdir().unwrap(); + let run_git = |dir: &std::path::Path, args: &[&str]| { + let out = std::process::Command::new("git") + .args(args) + .current_dir(dir) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + out + }; + run_git(origin_dir.path(), &["init", "--bare", "-b", "master"]); + run_git(&repo_root, &["init", "-b", "master"]); + run_git(&repo_root, &["config", "user.email", "test@test.com"]); + run_git(&repo_root, &["config", "user.name", "Test"]); + run_git(&repo_root, &["commit", "--allow-empty", "-m", "initial"]); + run_git( + &repo_root, + &[ + "remote", + "add", + "origin", + origin_dir.path().to_str().unwrap(), + ], + ); + run_git(&repo_root, &["push", "origin", "master"]); + + let s = AgentflareMcp { + backend_db_override: Some(tmp.path().join("backend.db")), + backend_project_link_override: Some(tmp.path().join("project.json")), + worktree_repo_root_override: Some(repo_root), + ..Default::default() + }; + + let created: serde_json::Value = + serde_json::from_str(&s.item(Parameters(empty_item_create("Test"))).unwrap()).unwrap(); + let item_id = created["id"].as_str().unwrap().to_string(); + + let claimed: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "claim".into(), + id: Some(item_id.clone()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let worktree_path = std::path::PathBuf::from(claimed["worktree_path"].as_str().unwrap()); + std::fs::write(worktree_path.join("real_work.txt"), "real work").unwrap(); + + let err = s + .item(Parameters(ItemRequest { + action: "done".into(), + id: Some(item_id.clone()), + summary: Some("Did the real work.".into()), + ..Default::default() + })) + .unwrap_err(); + + assert!( + err.message.contains("no PR resulted") || err.message.contains("not marking completed"), + "error must call out the missing PR: {}", + err.message + ); + + let comments: serde_json::Value = serde_json::from_str( + &s.comment(Parameters(CommentRequest { + action: "list".into(), + item_id: Some(item_id.clone()), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + let arr = comments.as_array().unwrap(); + assert!( + arr.iter().any(|c| c["body"] + .as_str() + .unwrap_or_default() + .contains("PR creation failed")), + "a PR-failure comment must be posted on the item: {arr:?}" + ); + + let item_state: serde_json::Value = serde_json::from_str( + &s.item(Parameters(ItemRequest { + action: "get".into(), + id: Some(item_id), + ..Default::default() + })) + .unwrap(), + ) + .unwrap(); + assert!( + item_state["completed_at"].is_null(), + "a push with no resulting PR must not be treated as completion: {item_state:?}" + ); +} diff --git a/src/mcp_server/tests/mod.rs b/src/mcp_server/tests/mod.rs index 7f360b169..070f33e12 100644 --- a/src/mcp_server/tests/mod.rs +++ b/src/mcp_server/tests/mod.rs @@ -351,6 +351,7 @@ mod action_tests; mod artifact_tests; mod asset_tests; mod item_commit_failure_tests; +mod item_pr_failure_tests; mod item_tests; mod item_tests_reporting; mod project_resolution_tests;