Sanitize scenario exception reports#10
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens the “public-safe” scenario contract reporting by redacting local filesystem paths from scenario validation failures, and adds regression tests to ensure exception reports don’t leak temp/external paths.
Changes:
- Introduces
public_exception_message()to redact filesystem paths from exception strings (includingOSError.filename/filename2). - Updates
validate_scenario()to emitpublic_path()and sanitized error messages on YAML parse/read failures. - Adds tests ensuring parse/read failures don’t include
tmp_pathin either the reported path or error message.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/strix_scenario_contract.py | Adds exception-message sanitization and uses it for YAML load failures in scenario validation output. |
| python/tests/test_strix_scenario_contract.py | Adds regression tests to prevent leaking local temp paths in scenario validation failure reports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
There was a problem hiding this comment.
The new public-safe tests cover POSIX absolute paths, but they won’t catch the common OSError.__str__ behavior of rendering filenames via repr(filename) (escaping backslashes). Adding a regression that constructs a FileNotFoundError(..., filename="C:\\tmp\\missing.yaml") and asserts the redaction removes both the raw and repr-escaped forms would help ensure Windows-style paths are also public-safe.
| windows_path = "C:\\tmp\\missing.yaml" | |
| windows_error = FileNotFoundError(2, "No such file or directory", windows_path) | |
| sanitized_error = module._public_message(str(windows_error)) | |
| assert windows_path not in sanitized_error | |
| assert repr(windows_path)[1:-1] not in sanitized_error |
| def public_exception_message(exc: Exception, path: Path) -> str: | ||
| """Return an exception message with local filesystem paths redacted.""" | ||
|
|
||
| message = str(exc) or type(exc).__name__ | ||
| replacements = {str(path): public_path(path)} | ||
| filename = getattr(exc, "filename", None) | ||
| filename2 = getattr(exc, "filename2", None) | ||
| for candidate in (filename, filename2): | ||
| if candidate: | ||
| candidate_path = Path(candidate) | ||
| replacements[str(candidate_path)] = public_path(candidate_path) | ||
|
|
||
| for raw, safe in replacements.items(): | ||
| if raw: | ||
| message = message.replace(raw, safe) | ||
| return message |
There was a problem hiding this comment.
public_exception_message() replaces only the literal str(path) / str(Path(exc.filename)) substrings. For OSError/FileNotFoundError, Python’s default __str__ often embeds the filename using repr(filename), which can escape backslashes (e.g. C:\\tmp\\x.yaml) and won’t match str(Path(...)), leaving absolute paths unredacted on Windows-style inputs. Consider also redacting the repr(...) form (and/or normalizing both slash styles) when applying replacements so the sanitization works across platforms.
Follow-up to automated review on PR #9.\n\nIncluded:\n- sanitize validate_scenario() parse/read error reports with public_path()\n- redact OSError filename fields from public evidence error messages\n- add regression tests for broken YAML and missing scenario files outside the repo\n\nValidation:\n- python scripts/verify_public_surface.py -> passed\n- pytest -q python/tests/test_strix_scenario_contract.py -> 9 passed\n- pytest -q -> 140 passed\n- python scripts/strix_scenario_contract.py --scenario-dir /tmp/does-not-exist --output /tmp/strix-missing-scenario-dir-exception-safe.json -> exits nonzero with public-safe failure report\n- cargo test -p strix-optimizer doctrine_profile_accepts_neutral_aliases --lib -> passed\n- cargo check -p strix-python -> passed