Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 34 additions & 0 deletions crates/xgen-cli/src/composition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,21 @@ impl PlanMaterializer for LocalReadMaterializer {
Self::Workspace(materializer) => materializer.materialize(request),
}
}

fn planned_arguments(
&self,
step_id: &str,
capability: &CapabilityRef,
reference: &ReconstructableMaterialReference,
material_digest: &str,
) -> Option<serde_json::Value> {
match self {
Self::ExactFiles(_) => None,
Self::Workspace(materializer) => {
materializer.planned_arguments(step_id, capability, reference, material_digest)
}
}
}
}

struct LocalMaterializer {
Expand Down Expand Up @@ -1526,6 +1541,25 @@ impl PlanMaterializer for LocalMaterializer {
}
self.filesystem.materialize(request)
}

fn planned_arguments(
&self,
step_id: &str,
capability: &CapabilityRef,
reference: &ReconstructableMaterialReference,
material_digest: &str,
) -> Option<serde_json::Value> {
if capability.capability_id == PROCESS_EXECUTE_CAPABILITY_ID {
return self.process.as_ref()?.planned_arguments(
step_id,
capability,
reference,
material_digest,
);
}
self.filesystem
.planned_arguments(step_id, capability, reference, material_digest)
}
}

#[allow(clippy::too_many_arguments, clippy::too_many_lines)]
Expand Down
34 changes: 34 additions & 0 deletions crates/xgen-cli/src/material_catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,23 @@ impl PlanMaterializer for WorkspaceReadMaterializer {
}
self.catalog.persist_request(&request)
}

fn planned_arguments(
&self,
step_id: &str,
capability: &CapabilityRef,
reference: &ReconstructableMaterialReference,
material_digest: &str,
) -> Option<Value> {
self.catalog
.workspace_arguments(
reference,
step_id,
material_digest,
&capability.capability_id,
)
.ok()
}
}

pub(crate) struct WorkspaceReadMaterialProvider {
Expand Down Expand Up @@ -509,6 +526,23 @@ impl PlanMaterializer for ProcessMaterializer {
}
self.catalog.persist_process_request(&request)
}

fn planned_arguments(
&self,
step_id: &str,
capability: &CapabilityRef,
reference: &ReconstructableMaterialReference,
material_digest: &str,
) -> Option<Value> {
if capability.capability_id != PROCESS_EXECUTE_CAPABILITY_ID
|| capability.contract_version != PROCESS_EXECUTE_CONTRACT_VERSION
{
return None;
}
self.catalog
.process_arguments(reference, step_id, material_digest)
.ok()
}
}

pub(crate) struct ProcessMaterialProvider {
Expand Down
5 changes: 5 additions & 0 deletions crates/xgen-cli/tests/interactive_repl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,11 @@ fn one_invalid_response_is_retried_and_the_run_completes_without_resume() {
.map(|_| server.requests.recv_timeout(TEST_TIMEOUT).unwrap())
.collect();
server.handle.join().unwrap();
// ADR-0065: the finished read shows the arguments it ran with.
assert_eq!(
planning_context(&requests[1])["steps"][0]["arguments"]["path"],
"workspace:primary/README.md"
);
// ADR-0064: only the call after the discarded response learns why it was discarded.
assert!(
planning_context(&requests[1])
Expand Down
128 changes: 122 additions & 6 deletions crates/xgen-runtime/src/agent_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ use xgen_policy::ResourceResolver;
use xgen_workgraph::{
AcceptedPlanStep, AgentLoopBudget, CompletionCandidateState, CompletionOutputError,
CompletionOutputRecord, ContinuationAction, DependencyBlockReason, ExpectedPlanningTurn,
FrontierAction, MAX_ACCEPTED_OBJECTIVE_BYTES, MAX_ACCEPTED_PLAN_EDGES, MAX_ACCEPTED_PLAN_STEPS,
ModelCallAbandonmentReason, ModelCallBudget, ModelCallRejectionReason, ModelCallReservation,
ModelCallSettlement, ModelCallStatus, ModelCallUnknownReason, PlannedExecutionProfile,
PlannedInvocationMaterialRecord, PlannedInvocationSpec, PlanningContractError,
ProposalRejectionCode, ReconstructableMaterialReference, RunEvent, RunEventBody, RunState,
StepStatus, ToolOutputRecord, WorkFrontier, dependency_release_block_reason, derive_frontier,
FrontierAction, InvocationMaterialRetention, MAX_ACCEPTED_OBJECTIVE_BYTES,
MAX_ACCEPTED_PLAN_EDGES, MAX_ACCEPTED_PLAN_STEPS, ModelCallAbandonmentReason, ModelCallBudget,
ModelCallRejectionReason, ModelCallReservation, ModelCallSettlement, ModelCallStatus,
ModelCallUnknownReason, PlannedExecutionProfile, PlannedInvocationMaterialRecord,
PlannedInvocationSpec, PlanningContractError, ProposalRejectionCode,
ReconstructableMaterialReference, RunEvent, RunEventBody, RunState, StepStatus,
ToolOutputRecord, WorkFrontier, dependency_release_block_reason, derive_frontier,
receipt_releases_dependency, validate_completion_summary_candidate,
};

Expand All @@ -37,6 +38,8 @@ const MAX_PLANNING_CATALOG_DEFINITIONS: usize = 1_024;
const MAX_PLANNING_SOURCE_STEPS: usize = 4_096;
const MAX_PLANNING_DEFINITION_BYTES: usize = 256 * 1024;
const MAX_PLANNING_STEP_BYTES: usize = 64 * 1024;
/// Larger arguments, such as patch bodies, stay out of the planning context (ADR-0065).
const MAX_PLANNING_STEP_ARGUMENT_BYTES: usize = 2 * 1024;
const MAX_PLANNING_SOURCE_BYTES: usize = 8 * 1024 * 1024;
const MAX_PLANNING_VALUE_DEPTH: usize = 64;
const MAX_PLANNING_VALUE_NODES: usize = 32_768;
Expand Down Expand Up @@ -193,6 +196,10 @@ pub struct PlanningStepSummary {
status: StepStatus,
attempts: u32,
capability: Option<CapabilityRef>,
/// Normalized arguments of a finished Step, when its materializer can return them within
/// `MAX_PLANNING_STEP_ARGUMENT_BYTES` (ADR-0065).
#[serde(skip_serializing_if = "Option::is_none")]
arguments: Option<Value>,
dependency_released: bool,
/// Value-free failure classification of a `failed` Step (ADR-0054); absent otherwise and for
/// failures recorded before classification existed.
Expand Down Expand Up @@ -336,6 +343,7 @@ impl fmt::Debug for PlanningStepSummary {
.field("status", &self.status)
.field("attempts", &self.attempts)
.field("capability", &self.capability)
.field("arguments", &self.arguments.as_ref().map(|_| "<redacted>"))
.field("dependency_released", &self.dependency_released)
.field("failure_code", &self.failure_code)
.field("failure_meaning", &self.failure_meaning)
Expand Down Expand Up @@ -969,6 +977,21 @@ pub trait PlanMaterializer {
&mut self,
request: PlanMaterializationRequest<'_>,
) -> Result<ReconstructableMaterialReference, PlanMaterializerFailure>;

/// Normalized arguments of a finished Step whose material this materializer persisted, shown
/// to later planner turns so they can tell which call produced an output (ADR-0065).
///
/// Implementations must return the arguments only when `reference` reconstructs to exactly
/// this Step, capability, and material digest. `None` leaves the arguments out.
fn planned_arguments(
&self,
_step_id: &str,
_capability: &CapabilityRef,
_reference: &ReconstructableMaterialReference,
_material_digest: &str,
) -> Option<Value> {
None
}
}

/// Why the loop deliberately yielded without selecting or committing more work.
Expand Down Expand Up @@ -1380,11 +1403,13 @@ impl AgentLoop {
if planning_snapshot.state != state {
return Err(AgentLoopError::PlanningSnapshotMismatch);
}
let step_arguments = finished_step_arguments(store, materializer, &state);
let context = match build_context_ordered(
&state,
planning_snapshot.completed_tool_outputs(),
planning_snapshot.planning_step_order(),
planning_snapshot.completed_tool_output_order(),
&step_arguments,
&frontier,
capabilities,
&self.planning_constraints,
Expand Down Expand Up @@ -2310,19 +2335,73 @@ fn build_context(
completed_tool_outputs,
&planning_step_order,
&completed_tool_output_order,
&BTreeMap::new(),
frontier,
registry,
planning_constraints,
budget,
)
}

/// Arguments of finished Steps, read back through the materializer that persisted them (ADR-0065).
///
/// A Step is skipped when its material is missing, ephemeral, bound to another digest, or larger
/// than `MAX_PLANNING_STEP_ARGUMENT_BYTES`; the context then shows it without arguments.
fn finished_step_arguments<S, M>(
store: &S,
materializer: &M,
state: &RunState,
) -> BTreeMap<String, Value>
where
S: RunStore,
M: PlanMaterializer,
{
let mut arguments = BTreeMap::new();
for step in state.steps.values() {
if !matches!(step.status, StepStatus::Completed | StepStatus::Failed) {
continue;
}
let (Some(intent), Some(invocation)) = (&step.intent, &step.planned_invocation) else {
continue;
};
let Ok(Some(material)) = store.load_invocation_material(&intent.effect_id) else {
continue;
};
let InvocationMaterialRetention::ReconstructableReference(reference) = material.retention()
else {
continue;
};
if material.step_id() != step.step_id
|| material.material_digest() != invocation.plan_input_digest()
{
continue;
}
let capability = CapabilityRef {
capability_id: invocation.capability_id().to_owned(),
contract_version: invocation.contract_version().to_owned(),
};
let Some(value) = materializer.planned_arguments(
&step.step_id,
&capability,
reference,
material.material_digest(),
) else {
continue;
};
if canonical_size(&value).is_ok_and(|size| size <= MAX_PLANNING_STEP_ARGUMENT_BYTES) {
arguments.insert(step.step_id.clone(), value);
}
}
arguments
}

#[allow(clippy::too_many_arguments, clippy::too_many_lines)]
fn build_context_ordered(
state: &RunState,
completed_tool_outputs: &BTreeMap<String, ToolOutputRecord>,
planning_step_order: &[String],
completed_tool_output_order: &[String],
step_arguments: &BTreeMap<String, Value>,
frontier: &WorkFrontier,
registry: &CapabilityRegistry,
planning_constraints: &[PlanningConstraint],
Expand Down Expand Up @@ -2479,6 +2558,7 @@ fn build_context_ordered(
capability_id: invocation.capability_id().to_owned(),
contract_version: invocation.contract_version().to_owned(),
}),
arguments: step_arguments.get(step_id).cloned(),
dependency_released: receipt_releases_dependency(step),
failure_code: step.failure_label(),
failure_meaning: step.failure_meaning(),
Expand Down Expand Up @@ -3536,6 +3616,7 @@ mod tests {
&BTreeMap::new(),
&["step-z".to_owned(), "step-a".to_owned()],
&[],
&BTreeMap::new(),
&frontier,
&CapabilityRegistry::new(),
&[],
Expand All @@ -3558,6 +3639,7 @@ mod tests {
&BTreeMap::new(),
&["step-a".to_owned(), "step-a".to_owned()],
&[],
&BTreeMap::new(),
&frontier,
&CapabilityRegistry::new(),
&[],
Expand All @@ -3571,6 +3653,7 @@ mod tests {
&BTreeMap::new(),
&["step-z".to_owned(), "step-a".to_owned()],
&["step-a".to_owned()],
&BTreeMap::new(),
&frontier,
&CapabilityRegistry::new(),
&[],
Expand Down Expand Up @@ -3618,6 +3701,7 @@ mod tests {
&BTreeMap::new(),
&order,
&[],
&BTreeMap::new(),
&frontier,
&CapabilityRegistry::new(),
&[],
Expand All @@ -3636,6 +3720,38 @@ mod tests {
assert!(value["steps"][0].get("repeatMeaning").is_none());
}

#[test]
fn finished_step_arguments_follow_the_capability_of_their_step() {
// ADR-0065: the planner sees which call produced an output.
let state = state(vec![
invoked(step("listed", StepStatus::Completed, Vec::new()), 'a'),
invoked(step("unlisted", StepStatus::Completed, Vec::new()), 'b'),
]);
let frontier = derive_frontier(&state).expect("frontier should derive");
let order = ["listed", "unlisted"].map(str::to_owned);
let arguments = BTreeMap::from([("listed".to_owned(), serde_json::json!({"path": "src"}))]);
let context = build_context_ordered(
&state,
&BTreeMap::new(),
&order,
&[],
&arguments,
&frontier,
&CapabilityRegistry::new(),
&[],
&test_budget(),
)
.expect("context should fit");
let value = serde_json::to_value(&context).expect("context should serialize");
assert_eq!(
value["steps"][0]["arguments"],
serde_json::json!({"path": "src"})
);
assert!(value["steps"][1].get("arguments").is_none());
let raw = serde_json::to_string(&context).expect("context should serialize");
assert!(raw.find("\"capability\"") < raw.find("\"arguments\""));
}

#[test]
fn planning_context_wire_order_is_stable_first_and_volatile_last() {
// ADR-0036: provider prefix caches reuse only a leading run of identical tokens, so the
Expand Down
2 changes: 1 addition & 1 deletion docs/adr/0064-planning-feedback.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,4 +57,4 @@ ADR-0061은 거절된 응답을 2번까지 다시 묻지만 이유는 알려 주

해석: 이 결정으로 생긴 회귀는 보이지 않는다. 읽기 반복 거절은 짧은 루프를 끊지만, 측정에서 가장 긴 루프는
프로세스 실행 반복이었고 이 결정은 그것을 막지 않는다. 같은 명령 반복은 결과를 비교해야 판단할 수 있어
다음 결정으로 남긴다. 거절 이유가 다음 응답을 고치는지는 표본이 작아 판단하지 않는다.
다음 결정으로 남긴다. 원인은 끝난 Step의 인자가 context에 없던 것이었다([ADR-0065](0065-finished-step-arguments.md)). 거절 이유가 다음 응답을 고치는지는 표본이 작아 판단하지 않는다.
Loading