scriptoriumadmin/fabro
1
0
Fork 0
mirror of https://github.com/fabro-sh/fabro.git synced 2026-09-11 22:53:00 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

refactor: simplify structured output error rendering

Browse source 
Follow-up review of the repair-error work. Behavior is the same or better;
the machinery is smaller.

Fixes a false "unchanged from your previous repair" nudge. same_problem_as
fell through to `_ => true`, so any two non-Required issues at the same
instance path, schema path and keyword compared equal. A model that removed
one unexpected property and added another was told it had changed nothing.
SchemaValidationIssue already derives PartialEq, so the 17-line comparison
is now `previous.contains(issue)`.

Drops the hand-written Type and Enum rendering. jsonschema already renders
both, and its messages name the offending value, which the hand-written
ones did not. Also switches masked() back to to_string(): masking replaced
the bad value with a placeholder, working against the goal of an actionable
message, and buys no privacy since the full response is already in the
prompt.

Resolves the schema fragment when the issue is captured rather than
threading Option<&OutputSchemaKind> through rendering. That reverts the
command.rs change and drops the test-only messages() shim. The fragment is
now attached only to Other, where it adds information; for required, type,
enum and additionalProperties it just repeated the prose.

Also: caps the model-controlled unexpected-property list so a wide object
cannot turn the repair prompt into megabytes; drops evaluation_path, which
was dead except under $ref, where it printed a pointer that does not
resolve; drops the keyword field, already named by the schema path; and
records the previous error only after the agent session accepted the
repair, since failover rebuilds the session from the original prompt.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Bryan Helmkamp 2026-08-01 09:04:28 -04:00
parent f75c7a1ba3
commit 0e0dfe4f9d
No known key found for this signature in database
3 changed files with 106 additions and 160 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
lib/components/fabro-workflow/src/handler/command.rs
View file

@ -176,10 +176,9 @@ impl Handler for CommandHandler {
structured_output::validate_response_text(schema, &finalized.output_text),
)
});
let mut outcome = if let Some((schema, Err(error))) = &validation {
let mut outcome = if let Some((_, Err(error))) = &validation {
Outcome::fail_deterministic(schema_validation_failure_reason(
script,
schema,
error,
&finalized.output_text,
))
@ -284,12 +283,11 @@ fn encode_stdin_value(value: serde_json::Value) -> serde_json::Result<Vec<u8>> {
fn schema_validation_failure_reason(
script: &str,
schema: &structured_output::OutputSchemaKind,
error: &StructuredOutputError,
output_text: &str,
) -> String {
let mut reason = format!("Script output failed output_schema validation: {script}");
for message in error.rendered_messages(Some(schema)) {
for message in error.messages() {
reason.push_str("\n- ");
reason.push_str(&message);
}

6
lib/components/fabro-workflow/src/handler/llm/api.rs
View file

@ -1741,7 +1741,6 @@ impl CodergenBackend for AgentApiBackend {
}
let repair_message =
error.repair_message(schema, previous_validation_error.as_ref());
previous_validation_error = Some(error);
let repair_result = live
.session
.process_input_with_runtime(
@ -1752,6 +1751,11 @@ impl CodergenBackend for AgentApiBackend {
live.record_input_timing();
match repair_result {
Ok(()) => {
// Only once the model has actually seen the
// repair can a later identical failure mean it
// ignored the correction. Failover rebuilds the
// session from the original prompt instead.
previous_validation_error = Some(error);
live.record_input_usage().await;
repair_attempts += 1;
response = last_assistant_response(&live.session);

254
lib/components/fabro-workflow/src/handler/structured_output.rs
View file

@ -3,9 +3,8 @@ use std::sync::{Arc, LazyLock};
use fabro_graphviz::graph::Node;
use fabro_llm::types::{ResponseFormat, ResponseFormatType};
use jsonschema::error::{TypeKind, ValidationErrorKind};
use jsonschema::error::ValidationErrorKind;
use jsonschema::paths::Location;
use jsonschema::types::JsonType;
use jsonschema::{ValidationError, Validator};
use serde_json::Value;
@ -51,32 +50,34 @@ pub(crate) enum StructuredOutputErrorKind {
const MAX_SCHEMA_FRAGMENT_CHARS: usize = 320;
/// `additionalProperties` errors carry one entry per unexpected key, and the
/// keys come from model output. Cap them so a wide object can't turn the repair
/// prompt into megabytes.
const MAX_UNEXPECTED_PROPERTIES: usize = 10;
#[derive(Debug, Clone, PartialEq, Eq)]
struct SchemaValidationIssue {
instance_path: Location,
schema_path: Location,
evaluation_path: Location,
keyword: String,
detail: SchemaValidationIssueDetail,
instance_path: Location,
schema_path: Location,
detail: SchemaValidationIssueDetail,
}
/// `Required` and `AdditionalProperties` get bespoke rendering because
/// `jsonschema` names the offending property without ever locating it. Every
/// other keyword already renders a message that names both the value and the
/// constraint, so it goes through `Other` with the schema fragment attached.
#[derive(Debug, Clone, PartialEq, Eq)]
enum SchemaValidationIssueDetail {
Required {
property: String,
},
Type {
expected: Vec<String>,
actual: String,
},
Enum {
options: String,
},
AdditionalProperties {
unexpected: Vec<String>,
total: usize,
},
Other {
message: String,
message: String,
schema_fragment: Option<String>,
},
}
@ -93,72 +94,67 @@ pub(crate) struct StructuredOutputError {
}
impl SchemaValidationIssue {
fn from_error(error: &ValidationError<'_>) -> Self {
fn from_error(error: &ValidationError<'_>, schema: Option<&Value>) -> Self {
let detail = match error.kind() {
ValidationErrorKind::Required { property } => SchemaValidationIssueDetail::Required {
property: property
.as_str()
.map_or_else(|| property.to_string(), str::to_owned),
},
ValidationErrorKind::Type { kind } => SchemaValidationIssueDetail::Type {
expected: expected_json_types(kind),
actual: JsonType::from(error.instance().as_ref()).to_string(),
},
ValidationErrorKind::Enum { options } => SchemaValidationIssueDetail::Enum {
options: bounded_json(options),
},
ValidationErrorKind::AdditionalProperties { unexpected } => {
SchemaValidationIssueDetail::AdditionalProperties {
unexpected: unexpected.clone(),
total: unexpected.len(),
unexpected: unexpected
.iter()
.take(MAX_UNEXPECTED_PROPERTIES)
.cloned()
.collect(),
}
}
_ => SchemaValidationIssueDetail::Other {
message: error.masked().to_string(),
message: error.to_string(),
schema_fragment: schema
.and_then(|schema| schema.pointer(error.schema_path().as_str()))
.map(bounded_json),
},
};
Self {
instance_path: error.instance_path().clone(),
schema_path: error.schema_path().clone(),
evaluation_path: error.evaluation_path().clone(),
keyword: error.kind().keyword().to_string(),
detail,
}
}
fn render(&self, schema: Option<&Value>) -> String {
fn render(&self) -> String {
let mut message = match &self.detail {
SchemaValidationIssueDetail::Required { property } => {
let target_path = self.instance_path.join(property);
format!(
"Missing required property {} at JSON Pointer `{target_path}`. Add it to the object at {}.",
Value::String(property.clone()),
pointer_phrase(&self.instance_path),
)
}
SchemaValidationIssueDetail::Type { expected, actual } => format!(
"At {}, expected JSON type {}, but got {actual}.",
pointer_phrase(&self.instance_path),
format_expected_types(expected),
),
SchemaValidationIssueDetail::Enum { options } => format!(
"At {}, the value is not one of the allowed enum values {options}.",
SchemaValidationIssueDetail::Required { property } => format!(
"Missing required property {} at JSON Pointer `{}`. Add it to the object at {}.",
Value::String(property.clone()),
self.instance_path.join(property),
pointer_phrase(&self.instance_path),
),
SchemaValidationIssueDetail::AdditionalProperties { unexpected } => {
let properties = unexpected
SchemaValidationIssueDetail::AdditionalProperties { unexpected, total } => {
let mut properties = unexpected
.iter()
.map(|property| {
let property_path = self.instance_path.join(property);
format!("{} at `{property_path}`", Value::String(property.clone()))
format!(
"{} at `{}`",
Value::String(property.clone()),
self.instance_path.join(property),
)
})
.collect::<Vec<_>>()
.join(", ");
let remaining = total - unexpected.len();
if remaining > 0 {
let _ = write!(properties, ", and {remaining} more");
}
format!(
"Unexpected properties in the object at {}: {properties}.",
pointer_phrase(&self.instance_path),
)
}
SchemaValidationIssueDetail::Other { message } => format!(
SchemaValidationIssueDetail::Other { message, .. } => format!(
"At {}: {}.",
pointer_phrase(&self.instance_path),
message.trim_end_matches('.'),
@ -167,46 +163,20 @@ impl SchemaValidationIssue {
let _ = write!(
message,
" Schema rule {} (`{}` keyword)",
pointer_code(&self.schema_path),
self.keyword,
" Schema rule: {}",
pointer_phrase(&self.schema_path)
);
if let Some(fragment) = schema
.and_then(|schema| schema.pointer(self.schema_path.as_str()))
.map(bounded_json)
if let SchemaValidationIssueDetail::Other {
schema_fragment: Some(fragment),
..
} = &self.detail
{
message.push_str(": ");
message.push_str(&fragment);
message.push_str(fragment);
}
message.push('.');
if self.evaluation_path != self.schema_path {
let _ = write!(
message,
" Evaluation path: {}.",
pointer_code(&self.evaluation_path),
);
}
message
}
fn same_problem_as(&self, other: &Self) -> bool {
if self.instance_path != other.instance_path
|| self.schema_path != other.schema_path
|| self.keyword != other.keyword
{
return false;
}
match (&self.detail, &other.detail) {
(
SchemaValidationIssueDetail::Required { property: left },
SchemaValidationIssueDetail::Required { property: right },
) => left == right,
(SchemaValidationIssueDetail::Required { .. }, _)
| (_, SchemaValidationIssueDetail::Required { .. }) => false,
_ => true,
}
}
}
impl StructuredOutputError {
@ -230,22 +200,12 @@ impl StructuredOutputError {
self.kind
}
#[cfg(test)]
#[must_use]
pub(crate) fn messages(&self) -> Vec<String> {
self.rendered_messages(None)
}
#[must_use]
pub(crate) fn rendered_messages(&self, schema: Option<&OutputSchemaKind>) -> Vec<String> {
match &self.details {
StructuredOutputErrorDetails::Message(message) => vec![message.clone()],
StructuredOutputErrorDetails::SchemaValidation(issues) => {
let schema = schema.and_then(|schema| match schema {
OutputSchemaKind::Routing => None,
OutputSchemaKind::JsonSchema { schema, .. } => Some(schema),
});
issues.iter().map(|issue| issue.render(schema)).collect()
issues.iter().map(SchemaValidationIssue::render).collect()
}
}
}
@ -275,7 +235,7 @@ impl StructuredOutputError {
}
};
let errors = self
.rendered_messages(Some(schema))
.messages()
.iter()
.map(|message| format!("- {message}"))
.collect::<Vec<_>>()
@ -284,8 +244,7 @@ impl StructuredOutputError {
vec!["Your previous response did not satisfy the node's output_schema.".to_string()];
if previous_error.is_some_and(|previous| self.shares_schema_issue_with(previous)) {
sections.push(
"At least one validation problem below is unchanged from your previous repair. \
Correct the exact JSON Pointer shown."
"At least one validation problem below is unchanged from your previous repair."
.to_string(),
);
}
@ -312,28 +271,7 @@ impl StructuredOutputError {
else {
return false;
};
current
.iter()
.any(|issue| previous.iter().any(|other| issue.same_problem_as(other)))
}
}
fn expected_json_types(kind: &TypeKind) -> Vec<String> {
match kind {
TypeKind::Single(json_type) => vec![json_type.to_string()],
TypeKind::Multiple(json_types) => json_types.iter().map(|kind| kind.to_string()).collect(),
}
}
fn format_expected_types(expected: &[String]) -> String {
let expected = expected
.iter()
.map(|kind| Value::String(kind.clone()).to_string())
.collect::<Vec<_>>();
match expected.as_slice() {
[] => "an allowed type".to_string(),
[expected] => expected.clone(),
_ => format!("one of {}", expected.join(", ")),
current.iter().any(|issue| previous.contains(issue))
}
}
@ -345,27 +283,13 @@ fn pointer_phrase(path: &Location) -> String {
}
}
fn pointer_code(path: &Location) -> String {
if path.as_str().is_empty() {
"`<document root>`".to_string()
} else {
format!("`{path}`")
}
}
fn bounded_json(value: &Value) -> String {
let rendered = value.to_string();
if rendered.chars().count() <= MAX_SCHEMA_FRAGMENT_CHARS {
rendered
} else {
format!(
"{}…",
rendered
.chars()
.take(MAX_SCHEMA_FRAGMENT_CHARS)
.collect::<String>()
)
let mut rendered = value.to_string();
if let Some((offset, _)) = rendered.char_indices().nth(MAX_SCHEMA_FRAGMENT_CHARS) {
rendered.truncate(offset);
rendered.push('…');
}
rendered
}
#[derive(Debug, Clone, PartialEq)]
@ -458,8 +382,8 @@ pub(crate) fn validate_response_text(
) -> Result<ValidatedStructuredOutput, StructuredOutputError> {
match schema {
OutputSchemaKind::Routing => validate_routing_response_text(text),
OutputSchemaKind::JsonSchema { validator, .. } => {
validate_custom_response_text(validator, text)
OutputSchemaKind::JsonSchema { schema, validator } => {
validate_custom_response_text(validator, schema, text)
}
}
}
@ -580,7 +504,7 @@ fn validate_routing_response_text(
if !contains_routing_field(obj) {
continue;
}
validate_value_against_validator(routing_validator(), &parsed)?;
validate_value_against_validator(routing_validator(), &parsed, None)?;
return Ok(ValidatedStructuredOutput { value: parsed });
}
@ -595,6 +519,7 @@ fn validate_routing_response_text(
fn validate_custom_response_text(
validator: &Validator,
schema: &Value,
text: &str,
) -> Result<ValidatedStructuredOutput, StructuredOutputError> {
// Prose after the object can contain braces, so the last candidate is not
@ -605,7 +530,7 @@ fn validate_custom_response_text(
for candidate in candidates.iter().rev() {
match serde_json::from_str::<Value>(candidate) {
Ok(parsed) => {
validate_value_against_validator(validator, &parsed)?;
validate_value_against_validator(validator, &parsed, Some(schema))?;
return Ok(ValidatedStructuredOutput { value: parsed });
}
Err(err) if invalid_json.is_none() => invalid_json = Some(err.to_string()),
@ -628,11 +553,12 @@ fn validate_custom_response_text(
fn validate_value_against_validator(
validator: &Validator,
value: &Value,
schema: Option<&Value>,
) -> Result<(), StructuredOutputError> {
let issues = validator
.iter_errors(value)
.take(5)
.map(|error| SchemaValidationIssue::from_error(&error))
.map(|error| SchemaValidationIssue::from_error(&error, schema))
.collect::<Vec<_>>();
if issues.is_empty() {
Ok(())
@ -959,10 +885,10 @@ mod tests {
let error = validate_response_text(&schema, r#"{"findings":[{}]}"#).unwrap_err();
assert_eq!(error.rendered_messages(Some(&schema)), vec![
assert_eq!(error.messages(), vec![
"Missing required property \"rationale\" at JSON Pointer `/findings/0/rationale`. \
Add it to the object at JSON Pointer `/findings/0`. Schema rule \
`/properties/findings/items/required` (`required` keyword): [\"rationale\"]."
Add it to the object at JSON Pointer `/findings/0`. Schema rule: JSON Pointer \
`/properties/findings/items/required`."
.to_string(),
],);
}
@ -980,13 +906,13 @@ mod tests {
let error =
validate_response_text(&schema, r#"{"line":"85","severity":"CRITICAL"}"#).unwrap_err();
assert_eq!(error.rendered_messages(Some(&schema)), vec![
"At JSON Pointer `/line`, expected JSON type \"integer\", but got string. \
Schema rule `/properties/line/type` (`type` keyword): \"integer\"."
assert_eq!(error.messages(), vec![
"At JSON Pointer `/line`: \"85\" is not of type \"integer\". \
Schema rule: JSON Pointer `/properties/line/type`: \"integer\"."
.to_string(),
"At JSON Pointer `/severity`, the value is not one of the allowed enum values \
[\"HIGH\",\"MEDIUM\",\"LOW\"]. Schema rule `/properties/severity/enum` \
(`enum` keyword): [\"HIGH\",\"MEDIUM\",\"LOW\"]."
"At JSON Pointer `/severity`: \"CRITICAL\" is not one of \"HIGH\", \"MEDIUM\" or \
\"LOW\". Schema rule: JSON Pointer `/properties/severity/enum`: \
[\"HIGH\",\"MEDIUM\",\"LOW\"]."
.to_string(),
],);
}
@ -1004,10 +930,9 @@ mod tests {
let error = validate_response_text(&schema, r#"{"findings":[],"rationale":"wrong level"}"#)
.unwrap_err();
assert_eq!(error.rendered_messages(Some(&schema)), vec![
assert_eq!(error.messages(), vec![
"Unexpected properties in the object at the document root: \"rationale\" at \
`/rationale`. Schema rule `/additionalProperties` (`additionalProperties` \
keyword): false."
`/rationale`. Schema rule: JSON Pointer `/additionalProperties`."
.to_string(),
],);
}
@ -1034,8 +959,7 @@ mod tests {
assert!(
repair.contains(
"At least one validation problem below is unchanged from your previous repair. \
Correct the exact JSON Pointer shown."
"At least one validation problem below is unchanged from your previous repair."
),
"unexpected repair message: {repair}",
);
@ -1045,6 +969,26 @@ mod tests {
);
}
#[test]
fn a_different_problem_at_the_same_location_is_not_called_unchanged() {
let schema = schema(serde_json::json!({
"type": "object",
"additionalProperties": false,
"properties": {
"findings": { "type": "array" }
}
}));
let previous = validate_response_text(&schema, r#"{"stray":1}"#).unwrap_err();
let current = validate_response_text(&schema, r#"{"different":1}"#).unwrap_err();
let repair = current.repair_message(&schema, Some(&previous));
assert!(
!repair.contains("unchanged from your previous repair"),
"unexpected repair message: {repair}",
);
}
#[test]
fn invalid_custom_schema_is_rejected_when_parsing_node_attr() {
let mut node = Node::new("audit");