Skip to content

Commit deb054c

Browse files
GudgeCopilot
andcommitted
Preserve whole-file source location in state-aware phase-config errors
This PR gives state-aware per-backend per-phase configuration parse errors real whole-file line/column locations, matching base-config errors. Details * Retain the decoded request text on ParsedStateAwareRequest (new source_text field) and, at dispatch, deserialize the experimental.<backend>.<phase> sub-slice positionally with config_deserialize::from_str instead of from a serde_json::Value, so typed errors carry source coordinates. * Translate the fragment-local serde location to whole-file coordinates via the sub-slice's byte offset; ConfigDeserializeError gains an optional location override consumed by Display (applied before secret redaction / control-char escaping so all existing guarantees hold), plus byte-offset<->line/column helpers and remap_error_to_source. * Graceful fallback to the prior value-based path when the sub-slice cannot be located, so a would-be typed error is never turned into a navigation panic and there is no regression. Tests * New tests asserting the whole-file line/column for a typed phase-config error at two different global lines (line 5 and line 7), proving the location is computed, not hardcoded; a fallback test that the value-based path still emits the JSON-path prefix without source_text; unit tests for the byte-offset<->line/column helpers (round-trip + hand-computed cases) and the trailing-location rewrite. * cargo fmt --all -- --check: passed. * cargo test -p wxc_common: 500 passed; 0 failed. * cargo clippy -p wxc_common --all-targets -- -D warnings: clean. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 041c1289-dedb-4acf-9949-516feb19fe1e
1 parent 4f36443 commit deb054c

7 files changed

Lines changed: 448 additions & 9 deletions

File tree

docs/state-aware-lifecycle/mxc-state-aware-sandbox-api.md

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1095,8 +1095,10 @@ state-aware-only fields (`phase`, `sandboxId`, `experimental.<backend>.<phase>`)
10951095
are extracted alongside the `ExecutionRequest` and bundled into a
10961096
`ParsedStateAwareRequest` domain model — `{ request: ExecutionRequest, phase:
10971097
Phase, containment: Option<ContainmentBackend>, sandbox_id: Option<String>,
1098-
experimental_raw: Option<serde_json::Value> }` — that the dispatcher consumes
1099-
(§9.3). The bundling does not modify `ExecutionRequest`'s shape. Domain models
1098+
experimental_raw: Option<serde_json::Value>, source_text: Option<Box<str>> }` —
1099+
that the dispatcher consumes (§9.3). `source_text` retains the decoded request
1100+
text so per-backend per-phase config errors can be reported with whole-file
1101+
source location (§9.3). The bundling does not modify `ExecutionRequest`'s shape. Domain models
11001102
are exposed to the dispatch layer; the wire types are an implementation detail of
11011103
the parser and schema generation.
11021104

@@ -1448,7 +1450,13 @@ prefix) or `malformed_id` (no prefix structure) per §8.
14481450
`Result<Option<C>, MxcError>`: it navigates the wire `experimental.<backend_key>.<phase_name>`
14491451
JSON value and deserialises it into `C` when present, returns `Ok(None)` when absent,
14501452
and surfaces malformed JSON as `malformed_request`. The dispatcher passes
1451-
`B::BACKEND_KEY` so each backend reads from its own slot.
1453+
`B::BACKEND_KEY` so each backend reads from its own slot. Typed errors carry the
1454+
complete `experimental.<backend>.<phase>.<field>` JSON path **and** whole-file
1455+
source location (line/column), at parity with base-config errors: the phase-config
1456+
sub-slice is deserialised positionally out of the retained request text and its
1457+
fragment-local serde location is translated back to whole-file coordinates. If the
1458+
sub-slice cannot be located, deserialisation falls back to the value-based path,
1459+
which still reports the JSON path (without a source location).
14521460
`sandbox_id_required()` enforces that non-provision phases carry a `sandboxId`,
14531461
returning `&str` on success or `malformed_request` on absence. `validate_exec_common`
14541462
is a free function in `validator.rs` that checks cross-backend per-phase invariants

src/core/mxc_engine/src/state_aware.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,7 @@ mod tests {
174174
sandbox_id: None,
175175
correlation_vector: None,
176176
experimental_raw: None,
177+
source_text: None,
177178
};
178179

179180
let error = run_state_aware(parsed, false).unwrap_err();

src/core/wxc/src/main.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1710,6 +1710,7 @@ mod tests {
17101710
sandbox_id: Some("iso:wxc-1234".into()),
17111711
correlation_vector: None,
17121712
experimental_raw: None,
1713+
source_text: None,
17131714
};
17141715

17151716
let err = command_override_context_for_state_aware(&parsed, true).unwrap_err();

src/core/wxc_common/src/config_deserialize.rs

Lines changed: 234 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ const SECRET_PATH_MARKERS: &[&str] = &[
3232
pub(crate) struct ConfigDeserializeError {
3333
path: Option<String>,
3434
source: serde_json::Error,
35+
/// Whole-file `(line, column)` that overrides the location baked into
36+
/// `source` when the error was produced from a sub-slice of a larger
37+
/// request (e.g. a state-aware `experimental.<backend>.<phase>` fragment).
38+
/// `None` leaves `source`'s own location untouched.
39+
location_override: Option<(usize, usize)>,
3540
}
3641

3742
impl ConfigDeserializeError {
@@ -41,9 +46,25 @@ impl ConfigDeserializeError {
4146
Self {
4247
path,
4348
source: error.into_inner(),
49+
location_override: None,
4450
}
4551
}
4652

53+
/// Override the source location rendered by `Display` with whole-file
54+
/// coordinates. Used when translating a fragment-local serde location back
55+
/// to its position in the complete request text.
56+
pub(crate) fn with_source_location(mut self, line: usize, column: usize) -> Self {
57+
self.location_override = Some((line, column));
58+
self
59+
}
60+
61+
/// The `(line, column)` serde recorded for this error, or `None` when serde
62+
/// could not attribute a position (it reports line 0 in that case).
63+
pub(crate) fn source_line_column(&self) -> Option<(usize, usize)> {
64+
let line = self.source.line();
65+
(line > 0).then(|| (line, self.source.column()))
66+
}
67+
4768
/// Prefix a path produced while deserializing a JSON subtree with its path
4869
/// in the complete request.
4970
pub(crate) fn with_prefix(mut self, prefix: &str) -> Self {
@@ -77,6 +98,13 @@ impl fmt::Display for ConfigDeserializeError {
7798
} else {
7899
self.source.to_string()
79100
};
101+
// Remap the source's baked-in `line/column` to whole-file coordinates
102+
// before escaping so all downstream guarantees (control-char escaping,
103+
// secret redaction, syntax-vs-data branch) still hold unchanged.
104+
let source = match self.location_override {
105+
Some((line, column)) => rewrite_trailing_location(&source, line, column),
106+
None => source,
107+
};
80108
let source = escape_control_characters(&source);
81109
match self.source.classify() {
82110
Category::Syntax | Category::Eof => {
@@ -106,6 +134,116 @@ fn redact_secret_value(source: &serde_json::Error) -> String {
106134
"invalid secret value".to_string()
107135
}
108136

137+
/// Replace a trailing serde-style ` at line <N> column <M>` suffix in a rendered
138+
/// error message with the supplied whole-file `line`/`column`. serde_json emits
139+
/// this stable suffix on positioned errors; if it is absent (unpositioned
140+
/// message), the location is appended so the caller still gets coordinates.
141+
fn rewrite_trailing_location(rendered: &str, line: usize, column: usize) -> String {
142+
let replacement = format!(" at line {line} column {column}");
143+
if let Some(index) = rendered.rfind(" at line ") {
144+
if is_location_suffix(&rendered[index..]) {
145+
return format!("{}{}", &rendered[..index], replacement);
146+
}
147+
}
148+
format!("{rendered}{replacement}")
149+
}
150+
151+
/// True when `suffix` is exactly ` at line <digits> column <digits>` with no
152+
/// trailing text — serde_json's positioned-error suffix shape.
153+
fn is_location_suffix(suffix: &str) -> bool {
154+
let Some(rest) = suffix.strip_prefix(" at line ") else {
155+
return false;
156+
};
157+
let (line_digits, rest) = split_leading_digits(rest);
158+
if line_digits.is_empty() {
159+
return false;
160+
}
161+
let Some(rest) = rest.strip_prefix(" column ") else {
162+
return false;
163+
};
164+
let (column_digits, rest) = split_leading_digits(rest);
165+
!column_digits.is_empty() && rest.is_empty()
166+
}
167+
168+
fn split_leading_digits(text: &str) -> (&str, &str) {
169+
let end = text
170+
.find(|character: char| !character.is_ascii_digit())
171+
.unwrap_or(text.len());
172+
text.split_at(end)
173+
}
174+
175+
/// 1-based `(line, column)` (serde_json semantics) → byte offset within `text`.
176+
///
177+
/// Column arithmetic assumes ASCII configs (bytes == columns); the parser only
178+
/// ever hands us JSON, which is ASCII outside string literals, and offsets are
179+
/// only used to translate error positions. Returns `None` when the position is
180+
/// out of range so callers can fall back gracefully.
181+
fn byte_offset_of_line_col(text: &str, line: usize, column: usize) -> Option<usize> {
182+
if line == 0 || column == 0 {
183+
return None;
184+
}
185+
let bytes = text.as_bytes();
186+
let mut current_line = 1usize;
187+
let mut line_start = 0usize;
188+
let mut index = 0usize;
189+
while current_line < line && index < bytes.len() {
190+
if bytes[index] == b'\n' {
191+
current_line += 1;
192+
line_start = index + 1;
193+
}
194+
index += 1;
195+
}
196+
if current_line < line {
197+
return None;
198+
}
199+
let offset = line_start + (column - 1);
200+
(offset <= text.len()).then_some(offset)
201+
}
202+
203+
/// Byte offset within `text` → 1-based `(line, column)` (serde_json semantics).
204+
///
205+
/// Line counting is byte-exact; column arithmetic assumes ASCII (see
206+
/// [`byte_offset_of_line_col`]). Operates on bytes to avoid slicing panics on a
207+
/// non-char-boundary offset.
208+
fn line_col_of_byte_offset(text: &str, offset: usize) -> (usize, usize) {
209+
let bytes = text.as_bytes();
210+
let end = offset.min(bytes.len());
211+
let mut line = 1usize;
212+
let mut last_newline: Option<usize> = None;
213+
for (index, byte) in bytes.iter().enumerate().take(end) {
214+
if *byte == b'\n' {
215+
line += 1;
216+
last_newline = Some(index);
217+
}
218+
}
219+
let column = match last_newline {
220+
Some(index) => end - index,
221+
None => end + 1,
222+
};
223+
(line, column)
224+
}
225+
226+
/// Translate a `ConfigDeserializeError` produced by deserializing `fragment`
227+
/// (which begins at byte `fragment_offset` within `source_text`) so its
228+
/// rendered location reports whole-file coordinates instead of fragment-local
229+
/// ones. Any step that cannot be resolved returns `err` unchanged.
230+
pub(crate) fn remap_error_to_source(
231+
err: ConfigDeserializeError,
232+
fragment: &str,
233+
fragment_offset: usize,
234+
source_text: &str,
235+
) -> ConfigDeserializeError {
236+
let Some((line, column)) = err.source_line_column() else {
237+
return err;
238+
};
239+
let Some(local_offset) = byte_offset_of_line_col(fragment, line, column) else {
240+
return err;
241+
};
242+
let global_offset = fragment_offset + local_offset;
243+
let (global_line, global_column) = line_col_of_byte_offset(source_text, global_offset);
244+
err.with_source_location(global_line, global_column)
245+
}
246+
109247
fn escape_control_characters(value: &str) -> String {
110248
let mut escaped = String::with_capacity(value.len());
111249
for character in value.chars() {
@@ -187,7 +325,11 @@ where
187325
let value = deserialize_with_path(&mut deserializer)?;
188326
deserializer
189327
.end()
190-
.map_err(|source| ConfigDeserializeError { path: None, source })?;
328+
.map_err(|source| ConfigDeserializeError {
329+
path: None,
330+
source,
331+
location_override: None,
332+
})?;
191333
Ok(value)
192334
}
193335

@@ -412,6 +554,7 @@ mod tests {
412554
let error = ConfigDeserializeError {
413555
path: Some(path.to_string()),
414556
source: serde_json::from_str::<Secret>(r#"{"wamToken": 123456789}"#).unwrap_err(),
557+
location_override: None,
415558
};
416559

417560
assert!(
@@ -426,6 +569,7 @@ mod tests {
426569
let error = ConfigDeserializeError {
427570
path: Some("monkey".to_string()),
428571
source: serde_json::from_str::<Secret>(r#"{"wamToken": 123456789}"#).unwrap_err(),
572+
location_override: None,
429573
};
430574

431575
assert!(!error.to_string().contains("invalid secret value"));
@@ -449,4 +593,93 @@ mod tests {
449593
let plain = "plain diagnostic text 123";
450594
assert_eq!(escape_diagnostic_text(plain), plain);
451595
}
596+
597+
#[test]
598+
fn byte_offset_and_line_col_round_trip() {
599+
let text = "line one\nline two\nline three\n";
600+
// Walk every byte offset and confirm the offset -> (line,col) -> offset
601+
// round-trip is stable.
602+
for offset in 0..=text.len() {
603+
let (line, column) = line_col_of_byte_offset(text, offset);
604+
assert_eq!(
605+
byte_offset_of_line_col(text, line, column),
606+
Some(offset),
607+
"round trip failed at offset {offset} -> ({line},{column})"
608+
);
609+
}
610+
}
611+
612+
#[test]
613+
fn line_col_of_byte_offset_hand_computed_cases() {
614+
let text = "abc\ndefgh\nij";
615+
// Offset 0 is line 1 column 1.
616+
assert_eq!(line_col_of_byte_offset(text, 0), (1, 1));
617+
// Offset 2 ('c') is line 1 column 3.
618+
assert_eq!(line_col_of_byte_offset(text, 2), (1, 3));
619+
// Offset 4 (start of "defgh") is line 2 column 1.
620+
assert_eq!(line_col_of_byte_offset(text, 4), (2, 1));
621+
// Offset 7 ('g') is line 2 column 4.
622+
assert_eq!(line_col_of_byte_offset(text, 7), (2, 4));
623+
// Offset 10 (start of "ij") is line 3 column 1.
624+
assert_eq!(line_col_of_byte_offset(text, 10), (3, 1));
625+
}
626+
627+
#[test]
628+
fn byte_offset_of_line_col_hand_computed_and_out_of_range() {
629+
let text = "abc\ndefgh\nij";
630+
// Line 2 column 1 is the byte after the first newline.
631+
assert_eq!(byte_offset_of_line_col(text, 2, 1), Some(4));
632+
// Line 3 column 2 -> 'j'.
633+
assert_eq!(byte_offset_of_line_col(text, 3, 2), Some(11));
634+
// A line beyond the text has no offset.
635+
assert_eq!(byte_offset_of_line_col(text, 9, 1), None);
636+
// serde reports 0 for unknown positions; reject those.
637+
assert_eq!(byte_offset_of_line_col(text, 0, 1), None);
638+
assert_eq!(byte_offset_of_line_col(text, 1, 0), None);
639+
}
640+
641+
#[test]
642+
fn rewrite_trailing_location_replaces_existing_suffix() {
643+
let rendered = "missing field `configuration_id` at line 2 column 5";
644+
let rewritten = rewrite_trailing_location(rendered, 7, 11);
645+
assert_eq!(
646+
rewritten,
647+
"missing field `configuration_id` at line 7 column 11"
648+
);
649+
}
650+
651+
#[test]
652+
fn rewrite_trailing_location_appends_when_absent() {
653+
let rendered = "some message without a position";
654+
let rewritten = rewrite_trailing_location(rendered, 3, 4);
655+
assert_eq!(
656+
rewritten,
657+
"some message without a position at line 3 column 4"
658+
);
659+
}
660+
661+
#[test]
662+
fn remap_error_translates_fragment_local_location_to_whole_file() {
663+
// A fragment that starts several lines into the whole file. The typed
664+
// error inside it must be reported at its whole-file line/column.
665+
let source_text = "line1\nline2\nline3\n{\n \"count\": \"many\"\n}\n";
666+
let fragment = "{\n \"count\": \"many\"\n}";
667+
let fragment_offset = source_text.find(fragment).unwrap();
668+
669+
let err = from_str::<Inner>(fragment).unwrap_err();
670+
// Fragment-local location: line 2 of the fragment.
671+
assert_eq!(err.source_line_column().map(|(l, _)| l), Some(2));
672+
673+
let remapped = remap_error_to_source(err, fragment, fragment_offset, source_text);
674+
let message = remapped.to_string();
675+
// The offending field sits on whole-file line 5.
676+
assert!(
677+
message.contains("line 5"),
678+
"expected whole-file line 5, got: {message}"
679+
);
680+
assert!(
681+
!message.contains("line 2"),
682+
"still fragment-local: {message}"
683+
);
684+
}
452685
}

src/core/wxc_common/src/config_parser.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,6 +1261,10 @@ fn convert_wire_state_aware(
12611261
sandbox_id,
12621262
correlation_vector,
12631263
experimental_raw,
1264+
// Retain the decoded request text so the dispatcher can deserialize each
1265+
// `experimental.<backend>.<phase>` sub-slice positionally and report
1266+
// typed errors with whole-file line/column (parity with base config).
1267+
source_text: Some(json.to_owned().into_boxed_str()),
12641268
})
12651269
}
12661270

0 commit comments

Comments
 (0)