From c62d9b68d92d7456d4f3e57f5bcb97aa8f617aad Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sat, 7 Mar 2026 21:16:27 -0500 Subject: [PATCH] Simplify opaque OpenAI item handling and DockerfileSource serde - Extract ContentPart::is_opaque_openai() to deduplicate matches! patterns - Use std::mem::take to avoid cloning reasoning/message items - Replace hand-rolled DockerfileSource serde with derive + untagged enum - Add --fail-with-body to imagegen curl for better error reporting - Add tmp to .gitignore Co-Authored-By: Claude Opus 4.6 --- .gitignore | 1 + crates/arc-agent/src/history.rs | 8 +-- crates/arc-llm/src/providers/openai.rs | 13 ++-- crates/arc-llm/src/types.rs | 6 ++ crates/arc-workflows/src/cli/run_config.rs | 6 +- crates/arc-workflows/src/daytona_sandbox.rs | 74 ++------------------- tools/imagegen | 2 +- 7 files changed, 24 insertions(+), 86 deletions(-) diff --git a/.gitignore b/.gitignore index edf132f0e..de550c8cb 100644 --- a/.gitignore +++ b/.gitignore @@ -3,3 +3,4 @@ target .entire .claude node_modules +tmp diff --git a/crates/arc-agent/src/history.rs b/crates/arc-agent/src/history.rs index b0cb660b3..0638f6193 100644 --- a/crates/arc-agent/src/history.rs +++ b/crates/arc-agent/src/history.rs @@ -27,7 +27,7 @@ impl History { timestamp: std::time::SystemTime::now(), }); self.turns.extend(preserved); - self.strip_opaque_reasoning(); + self.strip_opaque_provider_items(); } /// Remove provider-specific opaque items that are no longer valid after compaction. @@ -35,12 +35,10 @@ impl History { /// responses; after compaction replaces their surrounding context with a summary, they /// serve no purpose and can violate API constraints (reasoning must be followed by its /// output, identified by the message item's `id`). - fn strip_opaque_reasoning(&mut self) { + fn strip_opaque_provider_items(&mut self) { for turn in &mut self.turns { if let Turn::Assistant { provider_parts, .. } = turn { - provider_parts.retain(|p| { - !matches!(p, ContentPart::Other { kind, .. } if kind == ContentPart::OPENAI_REASONING || kind == ContentPart::OPENAI_MESSAGE) - }); + provider_parts.retain(|p| !p.is_opaque_openai()); } } } diff --git a/crates/arc-llm/src/providers/openai.rs b/crates/arc-llm/src/providers/openai.rs index 4df689e80..87f069511 100644 --- a/crates/arc-llm/src/providers/openai.rs +++ b/crates/arc-llm/src/providers/openai.rs @@ -259,10 +259,7 @@ fn translate_input(messages: &[Message]) -> (Option, Vec - { + ContentPart::Other { data, .. } if part.is_opaque_openai() => { input.push(data.clone()); } _ => {} @@ -825,17 +822,17 @@ fn handle_response_completed( let mut content_parts = Vec::new(); // Reasoning items must precede function calls for Responses API round-trip - for item in &state.reasoning_items { + for item in std::mem::take(&mut state.reasoning_items) { content_parts.push(ContentPart::Other { kind: ContentPart::OPENAI_REASONING.to_string(), - data: item.clone(), + data: item, }); } // Preserve full message output items for Responses API round-tripping - for item in &state.message_items { + for item in std::mem::take(&mut state.message_items) { content_parts.push(ContentPart::Other { kind: ContentPart::OPENAI_MESSAGE.to_string(), - data: item.clone(), + data: item, }); } if !state.accumulated_text.is_empty() { diff --git a/crates/arc-llm/src/types.rs b/crates/arc-llm/src/types.rs index cf9a3ef51..e7438e878 100644 --- a/crates/arc-llm/src/types.rs +++ b/crates/arc-llm/src/types.rs @@ -231,6 +231,12 @@ impl ContentPart { pub fn text(text: impl Into) -> Self { Self::Text(text.into()) } + + /// Returns `true` if this is an opaque OpenAI item (reasoning or message) + /// that should be round-tripped verbatim through the API. + pub fn is_opaque_openai(&self) -> bool { + matches!(self, ContentPart::Other { kind, .. } if kind == Self::OPENAI_REASONING || kind == Self::OPENAI_MESSAGE) + } } // --- 3.1 Message --- diff --git a/crates/arc-workflows/src/cli/run_config.rs b/crates/arc-workflows/src/cli/run_config.rs index 116da5150..7c84a811a 100644 --- a/crates/arc-workflows/src/cli/run_config.rs +++ b/crates/arc-workflows/src/cli/run_config.rs @@ -185,7 +185,7 @@ fn resolve_dockerfile(config: &mut WorkflowRunConfig, config_dir: &Path) -> anyh .and_then(|d| d.snapshot.as_mut()) .and_then(|snap| snap.dockerfile.as_mut()); - if let Some(DockerfileSource::Path(ref rel)) = source { + if let Some(DockerfileSource::Path { path: ref rel }) = source { let path = config_dir.join(rel); let contents = std::fs::read_to_string(&path) .with_context(|| format!("Failed to read dockerfile at {}", path.display()))?; @@ -569,7 +569,9 @@ dockerfile = { path = "./Dockerfile" } let snapshot = config.sandbox.unwrap().daytona.unwrap().snapshot.unwrap(); assert_eq!( snapshot.dockerfile, - Some(DockerfileSource::Path("./Dockerfile".into())) + Some(DockerfileSource::Path { + path: "./Dockerfile".into() + }) ); } diff --git a/crates/arc-workflows/src/daytona_sandbox.rs b/crates/arc-workflows/src/daytona_sandbox.rs index 007c697aa..992e96bee 100644 --- a/crates/arc-workflows/src/daytona_sandbox.rs +++ b/crates/arc-workflows/src/daytona_sandbox.rs @@ -133,77 +133,11 @@ impl<'de> Deserialize<'de> for DaytonaNetwork { /// `Path` variants are resolved to `Inline` during config loading /// (see `run_config::resolve_dockerfile`), so downstream consumers /// should only ever see `Inline`. -#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] +#[serde(untagged)] pub enum DockerfileSource { Inline(String), - Path(String), -} - -impl Serialize for DockerfileSource { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - match self { - DockerfileSource::Inline(s) => serializer.serialize_str(s), - DockerfileSource::Path(p) => { - use serde::ser::SerializeMap; - let mut map = serializer.serialize_map(Some(1))?; - map.serialize_entry("path", p)?; - map.end() - } - } - } -} - -impl<'de> Deserialize<'de> for DockerfileSource { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - struct DockerfileSourceVisitor; - - impl<'de> Visitor<'de> for DockerfileSourceVisitor { - type Value = DockerfileSource; - - fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(formatter, r#"a string or {{ path = "..." }}"#) - } - - fn visit_str(self, value: &str) -> Result { - Ok(DockerfileSource::Inline(value.to_owned())) - } - - fn visit_map>( - self, - mut map: M, - ) -> Result { - let Some(key) = map.next_key::()? else { - return Err(de::Error::custom( - r#"empty table: expected { path = "..." }"#, - )); - }; - - if key != "path" { - return Err(de::Error::custom(format!( - r#"unknown key "{key}": expected "path""# - ))); - } - - let path: String = map.next_value()?; - - if let Some(extra) = map.next_key::()? { - return Err(de::Error::custom(format!( - r#"unexpected key "{extra}": path table must have exactly one key"# - ))); - } - - Ok(DockerfileSource::Path(path)) - } - } - - deserializer.deserialize_any(DockerfileSourceVisitor) - } + Path { path: String }, } /// Snapshot configuration: when present, the sandbox is created from a snapshot @@ -335,7 +269,7 @@ impl DaytonaSandbox { Err(daytona_sdk::DaytonaError::NotFound { .. }) => { let dockerfile = match &snap_cfg.dockerfile { Some(DockerfileSource::Inline(s)) => s.as_str(), - Some(DockerfileSource::Path(_)) => { + Some(DockerfileSource::Path { .. }) => { return Err(format!( "Snapshot '{}': dockerfile path should have been resolved to inline content before sandbox creation", snap_cfg.name diff --git a/tools/imagegen b/tools/imagegen index aa7da7c89..b6c7fabdc 100755 --- a/tools/imagegen +++ b/tools/imagegen @@ -35,7 +35,7 @@ API_URL="https://generativelanguage.googleapis.com/v1beta/models/${MODEL}:genera echo "Generating image for: ${PROMPT}" >&2 echo "Model: ${MODEL}" >&2 -RESPONSE=$(curl -s -X POST "$API_URL" \ +RESPONSE=$(curl -s --fail-with-body -X POST "$API_URL" \ -H "Content-Type: application/json" \ -d "$(jq -n --arg prompt "$PROMPT" '{ contents: [{ parts: [{ text: $prompt }] }],