From 5c55b0c074de554ce393c384e6e68bdafc3bbd00 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 18:05:22 -0400 Subject: [PATCH 1/2] Fix stage TODO projection ownership --- lib/crates/fabro-store/src/run_state.rs | 84 +++++++++++-------------- 1 file changed, 37 insertions(+), 47 deletions(-) diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index 19c6c6a3d..1b8305f94 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -546,18 +546,27 @@ impl RunProjectionReducer for RunProjection { stage.state = StageState::from(outcome); } EventBody::TodoCreated(props) => { + if !should_project_stage_todo_event(stored, props.list_kind) { + return Ok(()); + } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { return Ok(()); }; - apply_todo_created(stage, props, stored.parent_session_id.as_deref()); + apply_todo_created(stage, props); } EventBody::TodoUpdated(props) => { + if !should_project_stage_todo_event(stored, props.list_kind) { + return Ok(()); + } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { return Ok(()); }; apply_todo_updated(stage, props); } EventBody::TodoDeleted(props) => { + if !should_project_stage_todo_event(stored, props.list_kind) { + return Ok(()); + } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { return Ok(()); }; @@ -681,25 +690,21 @@ impl RunProjectionReducer for RunProjection { } } -fn apply_todo_created( - stage: &mut StageProjection, - props: &TodoCreatedProps, - parent_session_id: Option<&str>, -) { - // OpenAI plan lists are scoped per agent session (`openai_plan:`). - // A child subagent must not displace a different list already in the slot - // (which would clobber the root agent's plan); but it may create the slot - // when empty, or extend its own list. Anthropic task lists are root-scoped - // and pass through unchanged. - if matches!(props.list_kind, TodoListKind::OpenAiPlan) - && parent_session_id.is_some() - && stage - .todos - .as_ref() - .is_some_and(|list| list.list_id != props.list_id) - { - return; - } +/// Decide whether a TODO event should mutate `StageProjection.todos`. +/// +/// OpenAI plan lists are scoped per agent session (`openai_plan:`), +/// so a child/subagent session emits its own list events on the same stage. +/// The stage sidebar represents the root stage agent, so child OpenAI plan +/// events do not belong in this projection. Anthropic task lists are +/// root-scoped (`anthropic_tasks:`) and intentionally shared +/// with subagents, so they always project. +fn should_project_stage_todo_event(stored: &RunEvent, list_kind: TodoListKind) -> bool { + let is_child_openai_plan_event = + matches!(list_kind, TodoListKind::OpenAiPlan) && stored.parent_session_id.is_some(); + !is_child_openai_plan_event +} + +fn apply_todo_created(stage: &mut StageProjection, props: &TodoCreatedProps) { if stage .todos .as_ref() @@ -4712,50 +4717,35 @@ mod tests { } #[test] - fn child_openai_plan_projects_when_no_root_plan_exists() { - // Regression: when a stage's root agent never calls update_plan but - // a subagent does, the subagent's plan must still surface in - // StageProjection.todos. Observed on run 01KSDXK5DJ61CFCK9YSDR8AETQ - // (implement@1), where a gpt-5.5 root delegated to a subagent that - // owned the only plan list — and the sidebar showed no todos. + fn child_openai_plan_does_not_project_when_root_has_no_plan() { let mut state = initialized_projection(); let stage_id = stage_id(); - let child_list = "openai_plan:child_session"; state - .apply_event(&child_stage_event( + .apply_event(&test_stage_event( 1, - created(child_list, TodoListKind::OpenAiPlan, "c-a", 0, "first"), + EventBody::StageStarted(started_props()), stage_id.clone(), )) .unwrap(); state .apply_event(&child_stage_event( 2, - created(child_list, TodoListKind::OpenAiPlan, "c-b", 1, "second"), - stage_id.clone(), - )) - .unwrap(); - state - .apply_event(&child_stage_event( - 3, - updated_status( - child_list, + created( + "openai_plan:child_session", TodoListKind::OpenAiPlan, "c-a", - TodoStatus::Completed, + 0, + "child work", ), stage_id.clone(), )) .unwrap(); - let projection = stage_todos(&state, &stage_id); - assert_eq!(projection.list_id, child_list); - assert_eq!(projection.kind, TodoListKind::OpenAiPlan); - assert_eq!(projection.items.len(), 2); - assert_eq!(projection.items[0].id, "c-a"); - assert_eq!(projection.items[0].status, TodoStatus::Completed); - assert_eq!(projection.items[1].id, "c-b"); - assert_eq!(projection.items[1].status, TodoStatus::Pending); + let stage = state.stage(&stage_id).expect("stage projection present"); + assert!( + stage.todos.is_none(), + "a child session's plan must not become the stage's root plan" + ); } #[test] From d8d1c14116ec115fe63e1820627b0b92ba14fcf7 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Thu, 23 Jul 2026 18:23:37 -0400 Subject: [PATCH 2/2] Clarify stage TODO ownership and plan guidance --- docs/public/api-reference/fabro-api.yaml | 6 +- lib/crates/fabro-agent/src/profiles/openai.rs | 16 +++ lib/crates/fabro-store/src/run_state.rs | 116 +++++++++++++----- lib/crates/fabro-types/src/run_projection.rs | 11 +- 4 files changed, 115 insertions(+), 34 deletions(-) diff --git a/docs/public/api-reference/fabro-api.yaml b/docs/public/api-reference/fabro-api.yaml index ca80fd423..97f58cbb4 100644 --- a/docs/public/api-reference/fabro-api.yaml +++ b/docs/public/api-reference/fabro-api.yaml @@ -10529,7 +10529,11 @@ components: oneOf: - $ref: "#/components/schemas/TodoListProjection" - type: "null" - description: Projected todo / task list for this stage. + description: | + Todo / task list owned by this stage's root agent session. OpenAI + child sessions have separate per-session plans that do not appear + here. Anthropic task lists are root-scoped and shared with child + sessions, so child mutations of that shared list do appear here. subagents: type: array description: Subagents spawned by this stage, in replay/insertion order. diff --git a/lib/crates/fabro-agent/src/profiles/openai.rs b/lib/crates/fabro-agent/src/profiles/openai.rs index ff0215d5c..4a624e3ef 100644 --- a/lib/crates/fabro-agent/src/profiles/openai.rs +++ b/lib/crates/fabro-agent/src/profiles/openai.rs @@ -229,6 +229,11 @@ and focused on the task. {file_edit_failure_guidance} - Do not `git commit` your changes or create new git branches unless explicitly requested. +# Planning + +If you create a checklist or task list, you update item statuses incrementally as each item is \ +completed rather than marking every item done only at the end. + # Validating Your Work If the codebase has tests or the ability to build or run, consider using them to verify your \ @@ -338,6 +343,17 @@ mod tests { assert!(prompt.contains("existing code conventions")); } + #[test] + fn openai_system_prompt_matches_codex_incremental_plan_guidance() { + let profile = OpenAiProfile::new("gpt-5.5"); + let env = MockSandbox::linux(); + let prompt = profile.build_system_prompt(&env, &EnvContext::default(), &[], None, &[]); + assert!(prompt.contains( + "update item statuses incrementally as each item is completed rather than \ + marking every item done only at the end" + )); + } + #[test] fn openai_system_prompt_includes_memory() { let profile = OpenAiProfile::new("o3-mini"); diff --git a/lib/crates/fabro-store/src/run_state.rs b/lib/crates/fabro-store/src/run_state.rs index 1b8305f94..347291ac7 100644 --- a/lib/crates/fabro-store/src/run_state.rs +++ b/lib/crates/fabro-store/src/run_state.rs @@ -546,7 +546,7 @@ impl RunProjectionReducer for RunProjection { stage.state = StageState::from(outcome); } EventBody::TodoCreated(props) => { - if !should_project_stage_todo_event(stored, props.list_kind) { + if !should_project_root_agent_todo_event(stored, props.list_kind) { return Ok(()); } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { @@ -555,7 +555,7 @@ impl RunProjectionReducer for RunProjection { apply_todo_created(stage, props); } EventBody::TodoUpdated(props) => { - if !should_project_stage_todo_event(stored, props.list_kind) { + if !should_project_root_agent_todo_event(stored, props.list_kind) { return Ok(()); } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { @@ -564,7 +564,7 @@ impl RunProjectionReducer for RunProjection { apply_todo_updated(stage, props); } EventBody::TodoDeleted(props) => { - if !should_project_stage_todo_event(stored, props.list_kind) { + if !should_project_root_agent_todo_event(stored, props.list_kind) { return Ok(()); } let Some(stage) = stage_at_stored_or_current_visit(self, stored, event.seq) else { @@ -690,32 +690,37 @@ impl RunProjectionReducer for RunProjection { } } -/// Decide whether a TODO event should mutate `StageProjection.todos`. +/// Decide whether a TODO event should mutate +/// `StageProjection.root_agent_todos`. /// /// OpenAI plan lists are scoped per agent session (`openai_plan:`), /// so a child/subagent session emits its own list events on the same stage. -/// The stage sidebar represents the root stage agent, so child OpenAI plan -/// events do not belong in this projection. Anthropic task lists are -/// root-scoped (`anthropic_tasks:`) and intentionally shared -/// with subagents, so they always project. -fn should_project_stage_todo_event(stored: &RunEvent, list_kind: TodoListKind) -> bool { - let is_child_openai_plan_event = - matches!(list_kind, TodoListKind::OpenAiPlan) && stored.parent_session_id.is_some(); - !is_child_openai_plan_event +/// The root-agent projection excludes those child plans, while the underlying +/// events remain in the run event log. Anthropic task lists are root-scoped +/// (`anthropic_tasks:`) and intentionally shared with +/// subagents, so they always project. +fn should_project_root_agent_todo_event(stored: &RunEvent, list_kind: TodoListKind) -> bool { + match list_kind { + TodoListKind::OpenAiPlan => stored.parent_session_id.is_none(), + TodoListKind::AnthropicTasks => true, + } } fn apply_todo_created(stage: &mut StageProjection, props: &TodoCreatedProps) { if stage - .todos + .root_agent_todos .as_ref() .is_none_or(|list| list.list_id != props.list_id || list.kind != props.list_kind) { - stage.todos = Some(TodoListProjection::new( + stage.root_agent_todos = Some(TodoListProjection::new( props.list_kind, props.list_id.clone(), )); } - let list = stage.todos.as_mut().expect("todo list was just inserted"); + let list = stage + .root_agent_todos + .as_mut() + .expect("todo list was just inserted"); list.upsert(TodoProjection { id: props.todo_id.clone(), status: props.status, @@ -732,7 +737,7 @@ fn apply_todo_created(stage: &mut StageProjection, props: &TodoCreatedProps) { fn apply_todo_updated(stage: &mut StageProjection, props: &TodoUpdatedProps) { if let Some(list) = stage - .todos + .root_agent_todos .as_mut() .filter(|list| list.list_id == props.list_id) { @@ -742,7 +747,7 @@ fn apply_todo_updated(stage: &mut StageProjection, props: &TodoUpdatedProps) { fn apply_todo_deleted(stage: &mut StageProjection, props: &TodoDeletedProps) { let Some(list) = stage - .todos + .root_agent_todos .as_mut() .filter(|list| list.list_id == props.list_id) else { @@ -750,7 +755,7 @@ fn apply_todo_deleted(stage: &mut StageProjection, props: &TodoDeletedProps) { }; list.remove(&props.todo_id); if list.items.is_empty() { - stage.todos = None; + stage.root_agent_todos = None; } } @@ -4450,11 +4455,14 @@ mod tests { StageId::new("code", 1) } - fn stage_todos<'a>(state: &'a RunProjection, stage_id: &StageId) -> &'a TodoListProjection { + fn root_agent_todos<'a>( + state: &'a RunProjection, + stage_id: &StageId, + ) -> &'a TodoListProjection { state .stage(stage_id) - .and_then(|stage| stage.todos.as_ref()) - .expect("stage todos present") + .and_then(|stage| stage.root_agent_todos.as_ref()) + .expect("root agent todos present") } fn child_stage_event(seq: u32, body: EventBody, stage_id: StageId) -> EventEnvelope { @@ -4544,7 +4552,7 @@ mod tests { )) .unwrap(); - let projection = stage_todos(&state, &stage_id); + let projection = root_agent_todos(&state, &stage_id); assert_eq!(projection.list_id, list); assert_eq!(projection.items.len(), 2); assert_eq!(projection.items[0].id, "a"); @@ -4579,7 +4587,7 @@ mod tests { )) .unwrap(); - let projection = stage_todos(&state, &stage_id); + let projection = root_agent_todos(&state, &stage_id); assert_eq!(projection.items.len(), 1); assert_eq!(projection.items[0].id, "b"); } @@ -4618,9 +4626,12 @@ mod tests { )) .unwrap(); - assert_eq!(stage_todos(&state, &plan_one).items[0].subject, "p1"); - assert_eq!(stage_todos(&state, &plan_two).items[0].subject, "p2"); - assert_eq!(stage_todos(&state, &claude).items[0].subject, "claude task"); + assert_eq!(root_agent_todos(&state, &plan_one).items[0].subject, "p1"); + assert_eq!(root_agent_todos(&state, &plan_two).items[0].subject, "p2"); + assert_eq!( + root_agent_todos(&state, &claude).items[0].subject, + "claude task" + ); } #[test] @@ -4706,7 +4717,7 @@ mod tests { )) .unwrap(); - let projection = stage_todos(&state, &stage_id); + let projection = root_agent_todos(&state, &stage_id); assert_eq!(projection.list_id, root_list); assert_eq!(projection.kind, TodoListKind::OpenAiPlan); assert_eq!(projection.items.len(), 2); @@ -4743,11 +4754,56 @@ mod tests { let stage = state.stage(&stage_id).expect("stage projection present"); assert!( - stage.todos.is_none(), + stage.root_agent_todos.is_none(), "a child session's plan must not become the stage's root plan" ); } + #[test] + fn root_openai_plan_projects_after_earlier_child_plan_event() { + let mut state = initialized_projection(); + let stage_id = stage_id(); + let root_list = "openai_plan:root_session"; + state + .apply_event(&test_stage_event( + 1, + EventBody::StageStarted(started_props()), + stage_id.clone(), + )) + .unwrap(); + state + .apply_event(&child_stage_event( + 2, + created( + "openai_plan:child_session", + TodoListKind::OpenAiPlan, + "child-a", + 0, + "child work", + ), + stage_id.clone(), + )) + .unwrap(); + state + .apply_event(&test_stage_event( + 3, + created( + root_list, + TodoListKind::OpenAiPlan, + "root-a", + 0, + "root work", + ), + stage_id.clone(), + )) + .unwrap(); + + let projection = root_agent_todos(&state, &stage_id); + assert_eq!(projection.list_id, root_list); + assert_eq!(projection.items.len(), 1); + assert_eq!(projection.items[0].subject, "root work"); + } + #[test] fn anthropic_child_session_task_events_still_project() { let mut state = initialized_projection(); @@ -4779,7 +4835,7 @@ mod tests { )) .unwrap(); - let projection = stage_todos(&state, &stage_id); + let projection = root_agent_todos(&state, &stage_id); assert_eq!(projection.list_id, list); assert_eq!(projection.kind, TodoListKind::AnthropicTasks); assert_eq!(projection.items.len(), 1); @@ -4845,7 +4901,7 @@ mod tests { )) .unwrap(); - let todo = &stage_todos(&state, &stage_id).items[0]; + let todo = &root_agent_todos(&state, &stage_id).items[0]; assert!(!todo.metadata.contains_key("k1")); assert_eq!(todo.metadata.get("k2"), Some(&serde_json::json!("v2"))); } diff --git a/lib/crates/fabro-types/src/run_projection.rs b/lib/crates/fabro-types/src/run_projection.rs index 6b67282ee..40a30b32c 100644 --- a/lib/crates/fabro-types/src/run_projection.rs +++ b/lib/crates/fabro-types/src/run_projection.rs @@ -351,8 +351,13 @@ pub struct StageProjection { pub usage: BilledTokenCounts, #[serde(default, skip_serializing_if = "Option::is_none")] pub model: Option, - #[serde(default, skip_serializing_if = "Option::is_none")] - pub todos: Option, + /// Todo/task list owned by the stage's root agent session. + /// + /// OpenAI child sessions own separate per-session plans and do not appear + /// here. Anthropic task lists are root-scoped and shared with child + /// sessions, so child mutations of that shared list do appear here. + #[serde(default, rename = "todos", skip_serializing_if = "Option::is_none")] + pub root_agent_todos: Option, #[serde(default, skip_serializing_if = "Vec::is_empty")] pub subagents: Vec, #[serde(default, skip_serializing_if = "SkillsProjection::is_empty")] @@ -439,7 +444,7 @@ impl StageProjection { timing: None, usage: BilledTokenCounts::default(), model: None, - todos: None, + root_agent_todos: None, subagents: Vec::new(), skills: SkillsProjection::default(), permission_level: None,