diff --git a/crates/xgen-cli/src/driver.rs b/crates/xgen-cli/src/driver.rs index d21ee01..3d6d641 100644 --- a/crates/xgen-cli/src/driver.rs +++ b/crates/xgen-cli/src/driver.rs @@ -533,7 +533,7 @@ fn model_call_may_be_next(store: &S) -> Result= agent.budget.max_model_turns || model_calls.reserved_calls >= model_calls.budget.max_model_calls() diff --git a/crates/xgen-cli/tests/workspace_discovery.rs b/crates/xgen-cli/tests/workspace_discovery.rs index 6214969..7d3bf91 100644 --- a/crates/xgen-cli/tests/workspace_discovery.rs +++ b/crates/xgen-cli/tests/workspace_discovery.rs @@ -838,6 +838,141 @@ fn exact_patch_requires_write_approval_and_survives_process_resume() { assert!(output.get("edits").is_none()); } +#[test] +fn failed_read_is_shown_to_the_planner_and_the_task_can_complete() { + let fixture = tempdir().expect("test directory should exist"); + let state_root = fixture.path().join("state"); + let workspace = fixture.path().join("workspace"); + fs::create_dir_all(&workspace).expect("workspace should create"); + // One byte over the read-text ceiling (ADR-0022) fails as a definite read-only failure. + fs::write(workspace.join("large.txt"), "x".repeat(64 * 1024 + 1)) + .expect("large file should write"); + fs::write(workspace.join("README.md"), "fixture workspace").expect("README should write"); + let server = SequentialServer::spawn_responses(vec![ + plan_response( + "read_large", + "Read the large file", + "xgeny.fs/read-text", + &json!({"path": "large.txt"}), + ), + plan_response( + "read_readme", + "Read the README instead", + "xgeny.fs/read-text", + &json!({"path": "README.md"}), + ), + completion_response(), + ]); + + let output = bounded_output(xgen(&state_root).args([ + "run", + "--workspace", + path_text(&workspace), + "--base-url", + &server.base_url, + "--model", + MODEL, + "--tokenizer", + TOKENIZER, + "--allow-dir", + ".", + "--allow-read", + "--allow-remote-model-egress", + "Summarize the workspace files.", + ])) + .expect("xgen run should finish"); + assert_eq!(output.status.code(), Some(0), "{}", stderr(&output)); + assert_eq!(String::from_utf8_lossy(&output.stdout), COMPLETION); + + let requests = (0..3) + .map(|_| { + server + .requests + .recv_timeout(TEST_TIMEOUT) + .expect("every model request should arrive") + }) + .collect::>(); + server.handle.join().expect("provider server should finish"); + + let after_failure = planning_context(&requests[1]); + assert_eq!(after_failure["steps"][0]["status"], "failed"); + assert_eq!(after_failure["toolOutputs"], json!([])); + let serialized = after_failure.to_string(); + assert!(!serialized.contains(path_text(&workspace))); + let before_completion = planning_context(&requests[2]); + assert_eq!(before_completion["steps"][1]["status"], "completed"); + assert_eq!( + chronological_output_capabilities(&before_completion), + ["xgeny.fs/read-text"] + ); + + let run_id = extract_run_id(&stderr(&output)); + let store = SqliteRunStore::open_existing_read_only( + state_root.join("runs").join(&run_id).join("run.sqlite3"), + ) + .expect("run store should open"); + let snapshot = store.load().unwrap().expect("run should exist"); + let mut statuses = snapshot + .state + .steps + .values() + .map(|step| step.status) + .collect::>(); + statuses.sort_by_key(|status| format!("{status:?}")); + assert_eq!(statuses, [StepStatus::Completed, StepStatus::Failed]); +} + +#[test] +fn failed_write_still_stops_the_run() { + let fixture = tempdir().expect("test directory should exist"); + let state_root = fixture.path().join("state"); + let workspace = fixture.path().join("workspace"); + fs::create_dir_all(workspace.join("src")).expect("workspace should create"); + let source = "pub fn answer() -> u32 {\n 41\n}\n"; + fs::write(workspace.join("src/lib.rs"), source).expect("source should write"); + let server = SequentialServer::spawn_responses(vec![plan_response( + "patch_source", + "Patch with a stale digest", + "xgeny.fs/apply-patch", + &json!({ + "path": "src/lib.rs", + "expectedDigest": test_sha256_digest(b"stale contents"), + "edits": [{ + "oldText": "41", + "newText": "42" + }] + }), + )]); + + let output = bounded_output(xgen(&state_root).args([ + "run", + "--workspace", + path_text(&workspace), + "--base-url", + &server.base_url, + "--model", + MODEL, + "--tokenizer", + TOKENIZER, + "--allow-dir", + "src", + "--allow-write", + "--allow-remote-model-egress", + "Update the answer.", + ])) + .expect("xgen run should finish"); + server.handle.join().expect("provider server should finish"); + assert!( + stderr(&output).contains("reason=failed_work"), + "{}", + stderr(&output) + ); + assert_eq!( + fs::read_to_string(workspace.join("src/lib.rs")).unwrap(), + source + ); +} + fn assert_resume_scope_and_material_failures( state_root: &Path, workspace: &Path, diff --git a/crates/xgen-runtime/src/agent_loop.rs b/crates/xgen-runtime/src/agent_loop.rs index c1f9a5f..0cb78e9 100644 --- a/crates/xgen-runtime/src/agent_loop.rs +++ b/crates/xgen-runtime/src/agent_loop.rs @@ -1189,7 +1189,7 @@ impl AgentLoop { } if state.steps.len() >= usize::try_from(loop_state.budget.max_planned_steps).unwrap_or(usize::MAX) - && !frontier.all_steps_receipt_completed() + && !frontier.all_steps_settled() { return Ok(AgentLoopTick::Quiescent { reason: AgentLoopQuiescence::PlannedStepBudgetExhausted, @@ -1590,7 +1590,7 @@ impl AgentLoop { F: EventFactory, { if (response_kind == xgen_workgraph::ResponseKind::TaskCompletion - && !frontier.all_steps_receipt_completed()) + && !frontier.all_steps_settled()) || (response_kind == xgen_workgraph::ResponseKind::Conversation && frontier.total_steps != 0) { @@ -1976,7 +1976,7 @@ fn action_would_issue_new_tool_call(state: &RunState, action: &FrontierAction) - fn terminal_quiescence(frontier: &WorkFrontier) -> Option { if !frontier.manual_required_step_ids.is_empty() { Some(AgentLoopQuiescence::ManualIntervention) - } else if !frontier.failed_step_ids.is_empty() { + } else if frontier.has_blocking_failures() { Some(AgentLoopQuiescence::FailedSteps) } else if !frontier.unverified_completed_step_ids.is_empty() { Some(AgentLoopQuiescence::UnverifiedCompletion) @@ -1984,7 +1984,7 @@ fn terminal_quiescence(frontier: &WorkFrontier) -> Option { Some(AgentLoopQuiescence::BlockedDependencies) } else if !frontier.waiting.is_empty() { Some(AgentLoopQuiescence::WaitingDependencies) - } else if frontier.total_steps > 0 && !frontier.all_steps_receipt_completed() { + } else if frontier.total_steps > 0 && !frontier.all_steps_settled() { Some(AgentLoopQuiescence::NoActionableWork) } else { None diff --git a/crates/xgen-workgraph/src/lib.rs b/crates/xgen-workgraph/src/lib.rs index 46b3425..028b5e9 100644 --- a/crates/xgen-workgraph/src/lib.rs +++ b/crates/xgen-workgraph/src/lib.rs @@ -3030,6 +3030,8 @@ pub struct WorkFrontier { pub verified_completed_step_ids: Vec, pub unverified_completed_step_ids: Vec, pub failed_step_ids: Vec, + /// Subset of `failed_step_ids` whose durable intent is read-only (ADR-0052). + pub read_only_failed_step_ids: Vec, pub manual_required_step_ids: Vec, } @@ -3048,6 +3050,22 @@ impl WorkFrontier { pub fn all_steps_receipt_completed(&self) -> bool { self.total_steps > 0 && self.verified_completed_step_ids.len() == self.total_steps } + + /// Report whether a failed Step has side effects that must stop the Run. + #[must_use] + pub fn has_blocking_failures(&self) -> bool { + self.failed_step_ids.len() > self.read_only_failed_step_ids.len() + } + + /// Report whether every planned Step is receipt-completed or a definite read-only failure. + /// + /// A failed read has no effect to undo, so a task may complete past it (ADR-0052). + #[must_use] + pub fn all_steps_settled(&self) -> bool { + self.total_steps > 0 + && self.verified_completed_step_ids.len() + self.read_only_failed_step_ids.len() + == self.total_steps + } } #[derive(Debug, Error, Clone, PartialEq, Eq)] @@ -3148,6 +3166,7 @@ struct FrontierAssembly<'a> { verified_completed_step_ids: Vec, unverified_completed_step_ids: Vec, failed_step_ids: Vec, + read_only_failed_step_ids: Vec, manual_required_step_ids: Vec, processed: usize, } @@ -3281,6 +3300,13 @@ impl<'a> FrontierAssembly<'a> { } StepStatus::Failed => { self.failed_step_ids.push(step_id.to_owned()); + if step + .intent + .as_ref() + .is_some_and(|intent| intent.effect_class == EffectClass::ReadOnly) + { + self.read_only_failed_step_ids.push(step_id.to_owned()); + } DependencyOutcome::Blocked } StepStatus::ManualRequired => { @@ -3308,6 +3334,7 @@ impl<'a> FrontierAssembly<'a> { self.verified_completed_step_ids.sort(); self.unverified_completed_step_ids.sort(); self.failed_step_ids.sort(); + self.read_only_failed_step_ids.sort(); self.manual_required_step_ids.sort(); WorkFrontier { @@ -3322,6 +3349,7 @@ impl<'a> FrontierAssembly<'a> { verified_completed_step_ids: self.verified_completed_step_ids, unverified_completed_step_ids: self.unverified_completed_step_ids, failed_step_ids: self.failed_step_ids, + read_only_failed_step_ids: self.read_only_failed_step_ids, manual_required_step_ids: self.manual_required_step_ids, } } @@ -4143,7 +4171,7 @@ fn record_completion_candidate( } let frontier = derive_frontier(state).map_err(map_plan_frontier_error)?; match response_kind { - ResponseKind::TaskCompletion if !frontier.all_steps_receipt_completed() => { + ResponseKind::TaskCompletion if !frontier.all_steps_settled() => { return Err(TransitionError::CompletionCandidateRequiresReceiptCompletedPlan); } ResponseKind::Conversation if frontier.total_steps != 0 => { @@ -5378,6 +5406,41 @@ mod tests { *intent } + #[test] + fn frontier_settles_definite_read_only_failures_but_not_other_failures() { + let mut records = Vec::new(); + let state = planned_state(&mut records); + let RunEventBody::EffectIntentCommitted { intent, .. } = + intent(&state, "step-material", "effect-material", 1) + else { + panic!("helper must create an intent") + }; + let failed_with = |effect_class| { + let mut failed = state.clone(); + let step = failed.steps.get_mut("step-material").unwrap(); + step.status = StepStatus::Failed; + let mut step_intent = (*intent).clone(); + step_intent.effect_class = effect_class; + step.intent = Some(step_intent); + derive_frontier(&failed).expect("frontier should derive") + }; + + let read = failed_with(EffectClass::ReadOnly); + assert_eq!(read.failed_step_ids, ["step-material"]); + assert_eq!(read.read_only_failed_step_ids, ["step-material"]); + assert!(!read.has_blocking_failures()); + assert!(read.all_steps_settled()); + assert!(!read.all_steps_receipt_completed()); + + for effect_class in [EffectClass::Idempotent, EffectClass::NonIdempotent] { + let other = failed_with(effect_class); + assert_eq!(other.failed_step_ids, ["step-material"]); + assert!(other.read_only_failed_step_ids.is_empty()); + assert!(other.has_blocking_failures()); + assert!(!other.all_steps_settled()); + } + } + #[test] fn invocation_material_record_detects_cross_effect_and_content_tampering() { let mut records = Vec::new(); diff --git a/docs/adr/0022-capability-confined-filesystem-read-adapter.md b/docs/adr/0022-capability-confined-filesystem-read-adapter.md index a4cbeaa..898bdf8 100644 --- a/docs/adr/0022-capability-confined-filesystem-read-adapter.md +++ b/docs/adr/0022-capability-confined-filesystem-read-adapter.md @@ -223,6 +223,7 @@ Linux, macOS와 Windows의 `cargo test --workspace --locked`가 동일 gate를 - filesystem timeout, cancellation, streaming, binary, range/chunk read - case/Unicode alias의 stable physical file identity - ReadOnly crash 뒤 자동 retry와 tool failure를 다음 model turn에 전달하는 typed error output + (확정된 read-only 실패 뒤 계획을 계속하는 것은 [ADR-0052](0052-recoverable-read-only-step-failures.md)가 다룬다. 실패 원인 분류 전달은 여전히 범위 밖이다) - remote model egress 승인 ## 결과 diff --git a/docs/adr/0052-recoverable-read-only-step-failures.md b/docs/adr/0052-recoverable-read-only-step-failures.md new file mode 100644 index 0000000..2dbb066 --- /dev/null +++ b/docs/adr/0052-recoverable-read-only-step-failures.md @@ -0,0 +1,73 @@ +# ADR-0052: 확정된 read-only Step 실패 뒤 계획 계속 + +- 상태: Accepted +- 기준일: 2026-10-25 +- 변경 분류: 범용 runtime 계약. 특정 도구·파일·업무 규칙 없음. +- 기존 경계: ADR-0015, ADR-0022, ADR-0038 +- protocol·journal·SQLite schema 변경: 없음. reducer의 완료 수락 조건만 넓어진다. +- planner prompt 변경: 없음. prompt 바이트는 저장된 request-profile digest에 묶여 있다. + +## 문제 + +Step 하나가 `Failed`가 되면 frontier가 `failed_step_ids`를 보고하고, agent loop는 다음 planning 전에 +`FailedSteps`로 멈춘다. public CLI는 이를 `rejected: failed_work`로 끝낸다. 부작용이 없는 읽기 +실패도 같은 길을 탄다. + +실사용 시나리오에서 75 KB 파일을 `xgeny.fs/read-text`로 읽으려다 64 KiB 상한(ADR-0022)에 걸렸고, +요청 전체가 8초 만에 거절됐다. 없는 경로, UTF-8이 아닌 파일, 일반 파일이 아닌 대상도 같다. 모델이 +목록·검색 결과로 다른 경로를 고르거나 실패를 설명할 기회가 없다. 같은 세션에서 테스트 실패(exit 1)는 +정상 observation이라 모델이 이어서 고칠 수 있었다. 실패의 종류가 아니라 실패가 어느 계층에서 +보고되는지가 결과를 갈랐다. + +## 결정 + +### 1. 확정된 read-only 실패는 settled로 본다 + +`StepStatus::Failed`이고 durable `EffectIntent.effect_class`가 `read_only`인 Step을 +*recoverable read failure*로 분류한다. frontier는 이 Step ID를 `failed_step_ids`에 그대로 남기고, +추가로 `read_only_failed_step_ids`에도 넣는다. + +- `Failed`는 adapter의 확정 실패나 검증 실패로만 도달한다. `EffectUnknown`, `Reconciling`, + `ManualRequired`는 이 분류에 들어가지 않는다. crash 뒤 결과를 모르는 읽기는 기존처럼 manual이다. +- read-only effect는 외부 상태를 바꾸지 않는다. 실패한 읽기를 지나쳐도 되돌릴 부작용이 없다. +- `idempotent`, `non_idempotent`, `compensatable`, `unknown` 실패는 기존처럼 Run을 멈춘다. + 쓰기·실행 실패 뒤 계속하려면 부분 효과의 의미를 따로 정해야 하고, 이 ADR의 범위가 아니다. + +### 2. agent loop는 read-only 실패만 남았을 때 계속 계획한다 + +`terminal_quiescence`는 read-only가 아닌 실패가 있을 때만 `FailedSteps`를 반환한다. 실패한 Step은 +dependency 관점에서는 계속 `Blocked`다. 따라서 그 Step에 의존하는 기존 Step은 진행되지 않고, 새 +proposal이 실패한 Step에 의존하면 기존처럼 `BlockedExistingDependency`로 거절된다. + +모델은 planning context의 `steps`에서 그 Step의 `status: failed`, objective, capability를 본다. +실패한 Step에는 toolOutput이 없다. 경로·OS 오류·adapter 내부 분류는 context에 넣지 않는다 +(ADR-0022, ADR-0038의 redaction 유지). + +### 3. 완료는 모든 Step이 receipt-completed 또는 recoverable read failure일 때 수락한다 + +`WorkFrontier::all_steps_settled()`는 Step이 하나 이상 있고, 모든 Step이 receipt-completed이거나 +recoverable read failure일 때 참이다. `TaskCompletion` 수락(reducer와 agent loop 양쪽)과 planned-step +budget 판정은 이 조건을 쓴다. `Conversation` 응답은 기존처럼 Step이 없을 때만 허용한다. + +reducer 조건이 넓어지기만 하므로 기존 journal은 새 binary에서 그대로 replay된다. 새 조건으로 수락한 +완료가 있는 journal은 이 변경 이전 binary에서 replay되지 않는다. + +## 결과 + +- 읽기 실패 하나로 요청 전체가 끝나지 않는다. 모델은 다른 읽기를 계획하거나 실패를 완료 요약에서 + 설명할 수 있다. +- 실패한 읽기도 tool-call과 planned-step budget을 소비한다. 반복 실패는 예산 소진으로 끝난다 + (ADR-0015). +- 실패 원인 분류(too-large, not-found 등)는 아직 모델에 전달하지 않는다. 지금 adapter는 고정 분류의 + digest만 남긴다. typed failure output은 별도 결정이 필요하다. +- 64 KiB를 넘는 파일을 읽는 방법은 여전히 없다. ADR-0022가 예정한 range/chunk capability는 후속 + 작업이다. + +## 검증 + +- frontier 분류: read-only 실패는 `read_only_failed_step_ids`에 들어가고 쓰기 실패는 들어가지 않는다. +- reducer: read-only 실패와 receipt-completed Step만 있으면 `TaskCompletion`을 수락하고, 쓰기 실패가 + 있으면 거절한다. +- agent loop: read-only 실패 뒤 `FailedSteps` 대신 다음 planning으로 진행하고, 쓰기 실패 뒤에는 + 기존처럼 `FailedSteps`로 멈춘다. +- 실사용: 64 KiB 초과 파일이 있는 저장소에서 같은 요청이 `failed_work` 없이 최종 응답까지 간다.