bug-210: prevent pipeline from moving story to done when mergemaster reports failure
The pipeline advancement logic was ignoring report_merge_failure and blindly trusting the server-owned completion gates_passed result. Now report_merge_failure sets a flag on the agent entry that the pipeline checks before advancing — stories stay in 4_merge/ when merge fails. Squash merge of feature/story-210 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -217,6 +217,12 @@ struct StoryAgent {
|
||||
project_root: Option<PathBuf>,
|
||||
/// UUID identifying the log file for this session.
|
||||
log_session_id: Option<String>,
|
||||
/// Set to `true` when the agent calls `report_merge_failure`.
|
||||
/// Prevents the pipeline from blindly advancing to `5_done/` after a
|
||||
/// failed merge: the server-owned gate check runs in the feature-branch
|
||||
/// worktree (which compiles fine) and returns `gates_passed=true` even
|
||||
/// though the code was never squash-merged onto master.
|
||||
merge_failure_reported: bool,
|
||||
}
|
||||
|
||||
/// Build an `AgentInfo` snapshot from a `StoryAgent` map entry.
|
||||
@@ -444,6 +450,7 @@ impl AgentPool {
|
||||
completion: None,
|
||||
project_root: Some(project_root.to_path_buf()),
|
||||
log_session_id: Some(log_session_id.clone()),
|
||||
merge_failure_reported: false,
|
||||
},
|
||||
);
|
||||
}
|
||||
@@ -898,6 +905,7 @@ impl AgentPool {
|
||||
completion: CompletionReport,
|
||||
project_root: Option<PathBuf>,
|
||||
worktree_path: Option<PathBuf>,
|
||||
merge_failure_reported: bool,
|
||||
) {
|
||||
let project_root = match project_root {
|
||||
Some(p) => p,
|
||||
@@ -1023,6 +1031,18 @@ impl AgentPool {
|
||||
}
|
||||
}
|
||||
PipelineStage::Mergemaster => {
|
||||
// Block advancement if the mergemaster explicitly reported a failure.
|
||||
// The server-owned gate check runs in the feature-branch worktree (not
|
||||
// master), so `gates_passed=true` is misleading when no code was merged.
|
||||
if merge_failure_reported {
|
||||
slog!(
|
||||
"[pipeline] Pipeline advancement blocked for '{story_id}': \
|
||||
mergemaster explicitly reported a merge failure. \
|
||||
Story stays in 4_merge/ for human review."
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
// Run script/test on master (project_root) as the post-merge verification.
|
||||
slog!(
|
||||
"[pipeline] Mergemaster completed for '{story_id}'. Running post-merge tests on master."
|
||||
@@ -1155,7 +1175,7 @@ impl AgentPool {
|
||||
|
||||
// Extract data for pipeline advance, then remove the entry so
|
||||
// completed agents never appear in list_agents.
|
||||
let (tx, session_id, project_root_for_advance, wt_path_for_advance) = {
|
||||
let (tx, session_id, project_root_for_advance, wt_path_for_advance, merge_failure_reported_for_advance) = {
|
||||
let mut agents = self.agents.lock().map_err(|e| e.to_string())?;
|
||||
let agent = agents.get_mut(&key).ok_or_else(|| {
|
||||
format!("Agent '{agent_name}' for story '{story_id}' disappeared during gate check")
|
||||
@@ -1165,8 +1185,9 @@ impl AgentPool {
|
||||
let sid = agent.session_id.clone();
|
||||
let pr = agent.project_root.clone();
|
||||
let wt = agent.worktree_info.as_ref().map(|w| w.path.clone());
|
||||
let mfr = agent.merge_failure_reported;
|
||||
agents.remove(&key);
|
||||
(tx, sid, pr, wt)
|
||||
(tx, sid, pr, wt, mfr)
|
||||
};
|
||||
|
||||
// Emit Done so wait_for_agent unblocks.
|
||||
@@ -1197,6 +1218,7 @@ impl AgentPool {
|
||||
report_for_advance,
|
||||
project_root_for_advance,
|
||||
wt_path_for_advance,
|
||||
merge_failure_reported_for_advance,
|
||||
)
|
||||
.await;
|
||||
});
|
||||
@@ -1303,6 +1325,48 @@ impl AgentPool {
|
||||
Some((session_id, project_root))
|
||||
}
|
||||
|
||||
/// Record that the mergemaster agent for `story_id` explicitly reported a
|
||||
/// merge failure via the `report_merge_failure` MCP tool.
|
||||
///
|
||||
/// Sets `merge_failure_reported = true` on the active mergemaster agent so
|
||||
/// that `run_pipeline_advance` can block advancement to `5_done/` even when
|
||||
/// the server-owned gate check returns `gates_passed=true` (those gates run
|
||||
/// in the feature-branch worktree, not on master).
|
||||
pub fn set_merge_failure_reported(&self, story_id: &str) {
|
||||
match self.agents.lock() {
|
||||
Ok(mut lock) => {
|
||||
let found = lock.iter_mut().find(|(key, agent)| {
|
||||
let key_story_id = key
|
||||
.rsplit_once(':')
|
||||
.map(|(sid, _)| sid)
|
||||
.unwrap_or(key.as_str());
|
||||
key_story_id == story_id
|
||||
&& pipeline_stage(&agent.agent_name) == PipelineStage::Mergemaster
|
||||
});
|
||||
match found {
|
||||
Some((_, agent)) => {
|
||||
agent.merge_failure_reported = true;
|
||||
slog!(
|
||||
"[pipeline] Merge failure flag set for '{story_id}:{}'",
|
||||
agent.agent_name
|
||||
);
|
||||
}
|
||||
None => {
|
||||
slog_warn!(
|
||||
"[pipeline] set_merge_failure_reported: no running mergemaster found \
|
||||
for story '{story_id}' — flag not set"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
Err(e) => {
|
||||
slog_error!(
|
||||
"[pipeline] set_merge_failure_reported: could not lock agents: {e}"
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Test helper: inject a pre-built agent entry so unit tests can exercise
|
||||
/// wait/subscribe logic without spawning a real process.
|
||||
#[cfg(test)]
|
||||
@@ -1328,6 +1392,7 @@ impl AgentPool {
|
||||
completion: None,
|
||||
project_root: None,
|
||||
log_session_id: None,
|
||||
merge_failure_reported: false,
|
||||
},
|
||||
);
|
||||
tx
|
||||
@@ -1363,6 +1428,7 @@ impl AgentPool {
|
||||
completion: None,
|
||||
project_root: None,
|
||||
log_session_id: None,
|
||||
merge_failure_reported: false,
|
||||
},
|
||||
);
|
||||
tx
|
||||
@@ -1705,6 +1771,7 @@ impl AgentPool {
|
||||
completion: Some(completion),
|
||||
project_root: Some(project_root),
|
||||
log_session_id: None,
|
||||
merge_failure_reported: false,
|
||||
},
|
||||
);
|
||||
tx
|
||||
@@ -1736,6 +1803,7 @@ impl AgentPool {
|
||||
completion: None,
|
||||
project_root: None,
|
||||
log_session_id: None,
|
||||
merge_failure_reported: false,
|
||||
},
|
||||
);
|
||||
tx
|
||||
@@ -2037,7 +2105,7 @@ async fn run_server_owned_completion(
|
||||
|
||||
// Store completion report, extract data for pipeline advance, then
|
||||
// remove the entry so completed agents never appear in list_agents.
|
||||
let (tx, project_root_for_advance, wt_path_for_advance) = {
|
||||
let (tx, project_root_for_advance, wt_path_for_advance, merge_failure_reported_for_advance) = {
|
||||
let mut lock = match agents.lock() {
|
||||
Ok(a) => a,
|
||||
Err(_) => return,
|
||||
@@ -2051,8 +2119,9 @@ async fn run_server_owned_completion(
|
||||
let tx = agent.tx.clone();
|
||||
let pr = agent.project_root.clone();
|
||||
let wt = agent.worktree_info.as_ref().map(|w| w.path.clone());
|
||||
let mfr = agent.merge_failure_reported;
|
||||
lock.remove(&key);
|
||||
(tx, pr, wt)
|
||||
(tx, pr, wt, mfr)
|
||||
};
|
||||
|
||||
// Emit Done so wait_for_agent unblocks.
|
||||
@@ -2075,6 +2144,7 @@ async fn run_server_owned_completion(
|
||||
project_root_for_advance,
|
||||
wt_path_for_advance,
|
||||
watcher_tx,
|
||||
merge_failure_reported_for_advance,
|
||||
);
|
||||
}
|
||||
|
||||
@@ -2092,6 +2162,7 @@ fn spawn_pipeline_advance(
|
||||
project_root: Option<PathBuf>,
|
||||
worktree_path: Option<PathBuf>,
|
||||
watcher_tx: broadcast::Sender<WatcherEvent>,
|
||||
merge_failure_reported: bool,
|
||||
) {
|
||||
let sid = story_id.to_string();
|
||||
let aname = agent_name.to_string();
|
||||
@@ -2102,8 +2173,15 @@ fn spawn_pipeline_advance(
|
||||
child_killers: Arc::new(Mutex::new(HashMap::new())),
|
||||
watcher_tx,
|
||||
};
|
||||
pool.run_pipeline_advance(&sid, &aname, completion, project_root, worktree_path)
|
||||
.await;
|
||||
pool.run_pipeline_advance(
|
||||
&sid,
|
||||
&aname,
|
||||
completion,
|
||||
project_root,
|
||||
worktree_path,
|
||||
merge_failure_reported,
|
||||
)
|
||||
.await;
|
||||
});
|
||||
}
|
||||
|
||||
@@ -3949,6 +4027,7 @@ mod tests {
|
||||
},
|
||||
Some(root.to_path_buf()),
|
||||
None,
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -3986,6 +4065,7 @@ mod tests {
|
||||
},
|
||||
Some(root.to_path_buf()),
|
||||
None,
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -4021,6 +4101,7 @@ mod tests {
|
||||
},
|
||||
Some(root.to_path_buf()),
|
||||
None,
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
|
||||
@@ -6569,6 +6650,7 @@ stage = "qa"
|
||||
},
|
||||
Some(root.to_path_buf()),
|
||||
None,
|
||||
false,
|
||||
)
|
||||
.await;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user