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:
BenKurrek
2026-08-04 00:21:43 -04:00
parent 204460d7c2
commit 0e7d14ece3
2 changed files with 53 additions and 10 deletions

View File

@@ -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

View File

@@ -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(