Skip to content

fix(tasks): never answer a board read with an empty result you could not obtain #27

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bkearns merged 1 commit into from
Aug 24, 2026
Merged

fix(tasks): never answer a board read with an empty result you could not obtain #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
75 changes: 34 additions & 41 deletions crates/cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1750,7 +1750,8 @@ fn finish_task<T: serde::Serialize>(
args: &serde_json::Value,
value: &T,
) -> Result<String, String> {
let on = forge_tasks::resolve_debug_stop(args.get("debug_stop").and_then(|v| v.as_bool()));
let on = forge_tasks::resolve_debug_stop(args.get("debug_stop").and_then(|v| v.as_bool()))
.map_err(|e| format!("{e:#}"))?;
let v = serde_json::to_value(value).map_err(|e| e.to_string())?;
let out =
forge_tasks::apply_debug_stop(Ok(v), &store.board_health(), on).map_err(|(_, m)| m)?;
Expand Down Expand Up @@ -3045,7 +3046,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
}),
|args| {
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let req = forge_tasks::CreateTaskRequest {
title: args.get("title").and_then(|v| v.as_str()).ok_or("title is required")?.to_string(),
Expand Down Expand Up @@ -3093,7 +3095,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
|args| {
let task_id = args.get("task_id").and_then(|v| v.as_str()).ok_or("task_id is required")?;
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let patch = forge_tasks::UpdateTaskPatch {
status: args.get("status").and_then(|v| v.as_str()).map(str::to_string),
Expand Down Expand Up @@ -3131,7 +3134,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
.and_then(|v| v.as_str())
.ok_or("task_id is required")?;
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store =
forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let task = store.get_task(task_id).map_err(|e| e.to_string())?;
Expand Down Expand Up @@ -3160,7 +3164,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
}),
|args| {
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store =
forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let filter = forge_tasks::TaskFilter {
Expand Down Expand Up @@ -3233,7 +3238,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
.and_then(|v| v.as_str())
.ok_or("child_id is required")?;
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store =
forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
store
Expand Down Expand Up @@ -3272,7 +3278,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
.and_then(|v| v.as_str())
.ok_or("child_id is required")?;
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store =
forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
store
Expand Down Expand Up @@ -3316,7 +3323,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
.and_then(|v| v.as_str())
.unwrap_or("agent");
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store =
forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let comment = store
Expand All @@ -3339,7 +3347,8 @@ fn build_mcp_server() -> anyhow::Result<forge_mcp_server::McpServer> {
}),
|args| {
let cql_hosts =
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()));
forge_tasks::resolve_cql_hosts(args.get("cql_host").and_then(|v| v.as_str()))
.map_err(|e| format!("{e:#}"))?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None).map_err(|e| e.to_string())?;
let board = store.board().map_err(|e| e.to_string())?;
// Slim rows by default. The full board was 619,574 characters for 382
Expand Down Expand Up @@ -6152,10 +6161,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
created_by,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let req = forge_tasks::CreateTaskRequest {
title,
body,
Expand Down Expand Up @@ -6185,10 +6192,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
summary,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let patch = forge_tasks::UpdateTaskPatch {
status,
assignee,
Expand All @@ -6205,10 +6210,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
}

TaskAction::Get { task_id, cql_host } => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let task = store.get_task(&task_id)?;
println!("{}", forge_shared::emit_json(&task, pretty)?);
}
Expand All @@ -6221,10 +6224,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
limit,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let filter = forge_tasks::TaskFilter {
status,
assignee,
Expand All @@ -6251,10 +6252,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
child_id,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
store.link_tasks(&parent_id, &child_id, "child")?;
println!("Linked {} \u{2192} {}", parent_id, child_id);
}
Expand All @@ -6264,10 +6263,8 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
child_id,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
store.unlink_tasks(&parent_id, &child_id)?;
println!("Unlinked {} \u{2194} {}", parent_id, child_id);
}
Expand All @@ -6278,19 +6275,15 @@ fn handle_task(action: TaskAction, pretty: bool) -> anyhow::Result<()> {
author,
cql_host,
} => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let comment = store.add_comment(&task_id, &author, &body)?;
println!("{}", forge_shared::emit_json(&comment, pretty)?);
}

TaskAction::Board { cql_host } => {
let store = forge_tasks::TaskStore::connect(
&forge_tasks::resolve_cql_hosts(cql_host.as_deref()),
None,
)?;
let cql_hosts = forge_tasks::resolve_cql_hosts(cql_host.as_deref())?;
let store = forge_tasks::TaskStore::connect(&cql_hosts, None)?;
let board = store.board()?;
println!("{}", forge_shared::emit_json(&board, pretty)?);
}
Expand Down
92 changes: 92 additions & 0 deletions crates/cli/tests/task_reads_fail_loud.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
//! End-to-end guard for the exact shape of the reported bug:
//!
//! $ forge task list --limit 1 # nothing listening on the CQL port
//! [] # exit 0, nothing on stderr
//!
//! A read against an unreachable board must exit non-zero, must not print an
//! empty JSON result on stdout, and must name the host and port it tried.
//! `/whats-next` and `/roadmap` read this board; an empty array they cannot
//! distinguish from a dead one turns "I could not look" into "there is no work".

use std::net::TcpListener;
use std::process::Command;

/// A `127.0.0.1:<port>` that nothing is listening on.
fn dead_contact_point() -> String {
let listener = TcpListener::bind("127.0.0.1:0").expect("bind ephemeral port");
let addr = listener.local_addr().expect("local_addr").to_string();
drop(listener);
addr
}

/// Run `frg task <args...> --cql-host <dead>` with the ambient config layers
/// neutralised, so the test asserts on the flag it passes and nothing else.
fn run_task_read(args: &[&str], host: &str) -> (bool, String, String) {
let out = Command::new(env!("CARGO_BIN_EXE_frg"))
.arg("task")
.args(args)
.arg("--cql-host")
.arg(host)
.env_remove("FORGE_CQL_HOST")
.env_remove("FORGE_DEBUG_STOP")
.output()
.expect("run frg");
(
out.status.success(),
String::from_utf8_lossy(&out.stdout).into_owned(),
String::from_utf8_lossy(&out.stderr).into_owned(),
)
}

#[test]
fn task_list_against_a_dead_board_exits_non_zero_and_prints_no_empty_array() {
let host = dead_contact_point();
let (ok, stdout, stderr) = run_task_read(&["list", "--limit", "1"], &host);

assert!(
!ok,
"an unreachable board must not exit 0; stdout was {stdout:?}"
);
assert!(
stdout.trim().is_empty(),
"an unreachable board must print nothing on stdout, not a result; got {stdout:?}"
);
assert!(
stderr.contains(&host),
"stderr must name the host and port tried, got: {stderr}"
);
}

#[test]
fn task_board_against_a_dead_board_exits_non_zero_and_names_the_host() {
let host = dead_contact_point();
let (ok, stdout, stderr) = run_task_read(&["board"], &host);

assert!(
!ok,
"an unreachable board must not exit 0; stdout was {stdout:?}"
);
assert!(
stdout.trim().is_empty(),
"an unreachable board must print nothing on stdout; got {stdout:?}"
);
assert!(
stderr.contains(&host),
"stderr must name the host and port tried, got: {stderr}"
);
}

#[test]
fn task_get_against_a_dead_board_exits_non_zero_and_names_the_host() {
let host = dead_contact_point();
let (ok, stdout, stderr) = run_task_read(&["get", "t_deadbeef"], &host);

assert!(
!ok,
"an unreachable board must not exit 0; stdout was {stdout:?}"
);
assert!(
stderr.contains(&host),
"stderr must name the host and port tried, got: {stderr}"
);
}
31 changes: 26 additions & 5 deletions crates/sheet-sync/src/board.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,15 @@ use forge_tasks::TaskStatus;

/// Board read/write boundary for pull.
pub trait BoardSink {
/// The board's *current* status for `task_id`, or `None` if no such task
/// is known to the board. Feeds [`crate::board_plan::plan_pull`]'s
/// The board's *current* status for `task_id`, or `Ok(None)` if no such
/// task is known to the board. Feeds [`crate::board_plan::plan_pull`]'s
/// never-move-backward rule — see that module's doc.
fn existing_status(&self, task_id: &str) -> Option<TaskStatus>;
///
/// A read that FAILS is an `Err`, never `Ok(None)`. Collapsing the two
/// disarmed the never-move-backward rule exactly when the board was
/// unreachable: an unreadable `complete` task looked like a task with no
/// status, and the pull happily reset it to the sheet's value.
fn existing_status(&self, task_id: &str) -> anyhow::Result<Option<TaskStatus>>;

/// Applies one planned op to the board. Returns the task id it created
/// or updated; `None` for [`BoardOp::Skip`], which never touches the
Expand All @@ -35,6 +40,9 @@ pub(crate) struct FakeBoard {
/// was (and wasn't) persisted before the failure.
fail_on_call: Option<usize>,
call_count: usize,
/// When true, `existing_status` returns `Err` — simulates the board being
/// unreadable while a pull is deciding whether a task's status is protected.
fail_status_read: bool,
}

#[cfg(test)]
Expand All @@ -46,6 +54,7 @@ impl FakeBoard {
next_id: 0,
fail_on_call: None,
call_count: 0,
fail_status_read: false,
}
}

Expand All @@ -58,6 +67,15 @@ impl FakeBoard {
}
}

/// A [`FakeBoard`] whose `existing_status` fails — the board is
/// reachable enough to be asked and cannot answer.
pub(crate) fn new_failing_status_read() -> Self {
Self {
fail_status_read: true,
..Self::new()
}
}

fn mint_task_id(&mut self) -> String {
self.next_id += 1;
format!("t_{}", self.next_id)
Expand All @@ -66,8 +84,11 @@ impl FakeBoard {

#[cfg(test)]
impl BoardSink for FakeBoard {
fn existing_status(&self, task_id: &str) -> Option<TaskStatus> {
self.statuses.get(task_id).cloned()
fn existing_status(&self, task_id: &str) -> anyhow::Result<Option<TaskStatus>> {
if self.fail_status_read {
anyhow::bail!("FakeBoard: simulated status-read failure for {task_id}");
}
Ok(self.statuses.get(task_id).cloned())
}

fn apply(&mut self, op: &BoardOp) -> anyhow::Result<Option<String>> {
Expand Down
12 changes: 9 additions & 3 deletions crates/sheet-sync/src/board_exec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl BoardExec {
/// that function's doc) and wraps the resulting store. No tenant
/// scoping: sheet-sync operates on the default tenant.
pub fn connect(cql_host: Option<&str>) -> anyhow::Result<Self> {
let hosts = resolve_cql_hosts(cql_host);
let hosts = resolve_cql_hosts(cql_host)?;
let store = TaskStore::connect(&hosts, None)?;
Ok(Self { store })
}
Expand All @@ -39,8 +39,14 @@ impl BoardExec {
}

impl BoardSink for BoardExec {
fn existing_status(&self, task_id: &str) -> Option<TaskStatus> {
self.store.get_task(task_id).ok().map(|t| t.task.status)
fn existing_status(&self, task_id: &str) -> anyhow::Result<Option<TaskStatus>> {
// `get_task(..).ok()` turned "the board is unreachable" into "no such
// task", which is the answer that lets a pull overwrite a protected
// status. `find_task` keeps absence and failure apart.
Ok(self
.store
.find_task(task_id)?
.map(|found| found.task.status))
}

fn apply(&mut self, op: &BoardOp) -> anyhow::Result<Option<String>> {
Expand Down
Loading