mirror of
https://github.com/nearai/ironclaw.git
synced 2026-09-03 08:06:01 +08:00
fix(extractors): ASCII-only extension normalization + narrow the Debug-payload guidance
Review triage for #7106. **CodeRabbit thread 2 — accepted.** `.claude/rules/types.md:170` and `review-discipline.md:45` require case-insensitive external values to be normalized with `to_ascii_lowercase()`, not Unicode case folding. Both extension registries in this crate used `to_lowercase()`; the sibling registry in `ironclaw_extension_support::coding::file` (`should_extract_document_before_text`) already got it right, so this is the outlier. Note it is a latent-hazard fix, not a live bug: the eight keys (pdf/docx/pptx/xlsx/doc/ppt/xls/rtf) contain none of the letters a Unicode fold can produce from a foreign codepoint, so I could not construct an input where the two differ today. It removes the hazard for the next key added. Test pins both halves: ASCII case-insensitivity still works, and a non-ASCII extension is not folded into an ASCII key. **CodeRabbit thread 1 — guidance tightened, code change refuted.** The reviewer is right that this crate's doc told callers to `tracing::debug!(?error, …)` without naming a ceiling, while `ironclaw_host_runtime/AGENTS.md:28` forbids unredacted user content in that crate's logs. Both docs now say the payload belongs in an operator log and nowhere else, and record what it actually carries. The proposed code change is refused with measurement in the PR thread: it would log strictly less than `main` does today.
This commit is contained in:
@@ -34,8 +34,14 @@ entry name, an offset — any of which can echo the document's own content.
|
||||
- **`Display` renders the classification and nothing else.** Interpolating an
|
||||
`ExtractionError` into model-facing text with `{error}` is safe by
|
||||
construction. That is the entire reason the type exists.
|
||||
- **`Debug` renders the payload.** Log it (`tracing::debug!(?error, …)`);
|
||||
never render it.
|
||||
- **`Debug` renders the payload**, so it belongs in an **operator log** and
|
||||
nowhere else — never a model result, capability output, projected event,
|
||||
snapshot, or user-visible error. What it carries is container/parser
|
||||
*structure* (`lopdf` object ids, byte offsets, dictionary keys; `zip`
|
||||
archive diagnostics; the fixed OOXML entry paths this crate reads), not
|
||||
document text — but a consumer under a stricter redaction charter, notably
|
||||
`ironclaw_host_runtime` (see its `AGENTS.md`), owns that ceiling and should
|
||||
re-check it before widening where the payload goes.
|
||||
|
||||
A new variant must keep that property: its `#[error("…")]` string may not
|
||||
interpolate a field. `every_extraction_failure_display_is_content_free` drives
|
||||
|
||||
@@ -34,17 +34,26 @@ const MAX_DECOMPRESSED_TOTAL: u64 = 100 * 1024 * 1024;
|
||||
///
|
||||
/// **This type carries diagnostic detail that must not reach a model.** The
|
||||
/// payload of [`NotExtractable`](Self::NotExtractable) is whatever the
|
||||
/// underlying parser said — a `pdf-extract` message, a ZIP entry name, an
|
||||
/// offset into the document — and any of it can echo the document's own
|
||||
/// content. So the safety property is built into the type rather than asked
|
||||
/// for in a comment:
|
||||
/// underlying parser said about bytes the *user* supplied — a `pdf-extract`
|
||||
/// message, a ZIP entry name, an offset into the document. It is untrusted
|
||||
/// text of unbounded shape from a third-party parser, which is reason enough
|
||||
/// never to render it. So the safety property is built into the type rather
|
||||
/// than asked for in a comment:
|
||||
///
|
||||
/// - **`Display` renders the classification and nothing else.** It names no
|
||||
/// MIME type, no filename, no parser output. Interpolating an
|
||||
/// `ExtractionError` into model-facing text with `{error}` is safe by
|
||||
/// construction, which is the whole point of the type.
|
||||
/// - **`Debug` renders everything**, so it is the right thing to log
|
||||
/// (`tracing::debug!(?error, …)`) and the wrong thing to render.
|
||||
/// - **`Debug` renders everything**, so it is the wrong thing to render and
|
||||
/// belongs only in an **operator log** (`tracing::debug!(?error, …)`) —
|
||||
/// never in a model result, a capability output, a projected event, a
|
||||
/// snapshot, or a user-visible error. Consumers bound by a stricter
|
||||
/// redaction charter than a debug log (see
|
||||
/// `crates/ironclaw_host_runtime/AGENTS.md`) should re-check that ceiling
|
||||
/// before widening where the payload goes; what it carries today is
|
||||
/// container/parser *structure* — `lopdf`'s object ids, byte offsets and
|
||||
/// dictionary keys, `zip`'s archive diagnostics and the fixed OOXML entry
|
||||
/// paths this crate reads — not document text.
|
||||
///
|
||||
/// Before this was a type the same rule lived as a doc comment on
|
||||
/// `DocumentExtraction::Failed(String)` — and the *other* boundary site,
|
||||
@@ -223,7 +232,7 @@ pub fn extract_document_text_by_filename(
|
||||
) -> Result<Option<String>, ExtractionError> {
|
||||
let ext = filename
|
||||
.and_then(|filename| filename.rsplit('.').next())
|
||||
.map(str::to_lowercase);
|
||||
.map(str::to_ascii_lowercase);
|
||||
let Some(ext) = ext else {
|
||||
return Ok(None);
|
||||
};
|
||||
@@ -704,7 +713,7 @@ fn try_extract_by_extension(data: &[u8], filename: Option<&str>) -> Option<Strin
|
||||
if let Ok(Some(text)) = extract_document_text_by_filename(data, filename) {
|
||||
return Some(text);
|
||||
}
|
||||
let ext = filename?.rsplit('.').next()?.to_lowercase();
|
||||
let ext = filename?.rsplit('.').next()?.to_ascii_lowercase();
|
||||
|
||||
match ext.as_str() {
|
||||
"txt" | "csv" | "tsv" | "json" | "xml" | "yaml" | "yml" | "toml" | "md" | "markdown"
|
||||
@@ -917,6 +926,34 @@ mod tests {
|
||||
assert!(result.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extension_matching_is_ascii_case_insensitive_and_nothing_more() {
|
||||
// Both extension registries normalize with `to_ascii_lowercase`, not
|
||||
// `to_lowercase` (`.claude/rules/types.md`): ASCII case must still
|
||||
// match, and Unicode case folding must not be able to fold a foreign
|
||||
// codepoint into an ASCII key. The keys are all ASCII, so this pins
|
||||
// the normalization rather than a behaviour difference.
|
||||
let pdf = include_bytes!("../../../tests/fixtures/hello.pdf");
|
||||
assert!(
|
||||
extract_document_text_by_filename(pdf, Some("HELLO.PDF"))
|
||||
.expect("uppercase .PDF must still route to the PDF extractor")
|
||||
.is_some()
|
||||
);
|
||||
assert_eq!(
|
||||
extract_document_text_by_filename(pdf, Some("hello.pd\u{212A}")).unwrap(),
|
||||
None,
|
||||
"a non-ASCII extension must not be folded into an ASCII key"
|
||||
);
|
||||
assert_eq!(
|
||||
try_extract_by_extension(b"content", Some("notes.TXT")),
|
||||
Some("content".to_string())
|
||||
);
|
||||
assert_eq!(
|
||||
try_extract_by_extension(b"content", Some("notes.T\u{0130}T")),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn extract_document_text_by_filename_extracts_pdf() {
|
||||
let result = extract_document_text_by_filename(
|
||||
|
||||
Reference in New Issue
Block a user