Skip to content

Redact Windows-style scenario paths#11

Merged
RMANOV merged 1 commit into
mainfrom
fix-windows-path-redaction
Apr 25, 2026
Merged

Redact Windows-style scenario paths#11
RMANOV merged 1 commit into
mainfrom
fix-windows-path-redaction

Conversation

@RMANOV
Copy link
Copy Markdown
Owner

@RMANOV RMANOV commented Apr 25, 2026

Follow-up to automated review on PR #10.\n\nIncluded:\n- detect Windows drive and UNC-style paths in public_path()\n- redact literal and repr-escaped path variants from exception messages\n- add regression coverage for FileNotFoundError with a Windows-style filename\n\nValidation:\n- python scripts/verify_public_surface.py -> passed\n- pytest -q python/tests/test_strix_scenario_contract.py -> 10 passed\n- pytest -q -> 141 passed\n- cargo test -p strix-optimizer doctrine_profile_accepts_neutral_aliases --lib -> passed\n- cargo check -p strix-python -> passed

Copilot AI review requested due to automatic review settings April 25, 2026 08:15
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a5826baa97

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +33 to +35
if re.match(r"^[A-Za-z]:[\\/]", path_str) or path_str.startswith("\\\\"):
name = PureWindowsPath(path_str).name or "."
return f"<external>/{name}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Check root-relative paths before Windows redaction

On Windows, every absolute in-repo path (for example C:\repo\sim\scenarios\a.yaml) matches this drive-prefix branch first, so public_path() now returns <external>/a.yaml instead of the previous repo-relative path. That regresses report fidelity for normal runs on Windows and can make outputs ambiguous when consumers expect stable project-relative paths; this branch should run only after the is_relative_to(ROOT) check so in-repo files keep their relative location while external Windows paths are still redacted.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates scenario contract reporting to better redact Windows-style paths from public-facing outputs, preventing leakage of local filesystem layouts in error messages.

Changes:

  • Extend public_path() to recognize Windows drive-letter and UNC-style paths.
  • Redact additional literal/repr-escaped path variants from exception messages.
  • Add a regression test covering FileNotFoundError with a Windows-style filename.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
scripts/strix_scenario_contract.py Enhances path detection/redaction logic in public_path() / public_exception_message().
python/tests/test_strix_scenario_contract.py Adds regression coverage for Windows-style path redaction in exception messages.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +33 to 37
if re.match(r"^[A-Za-z]:[\\/]", path_str) or path_str.startswith("\\\\"):
name = PureWindowsPath(path_str).name or "."
return f"<external>/{name}"
if path.is_relative_to(ROOT):
return str(path.relative_to(ROOT))
Copy link

Copilot AI Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public_path() checks for a Windows drive/UNC prefix before path.is_relative_to(ROOT). On Windows, any absolute repo path (including ones under ROOT) will match ^[A-Za-z]:[\\/] and be treated as <external>/..., so internal scenario/report paths won’t be reported relative to the repo anymore. Consider checking is_relative_to(ROOT) first (and/or only applying the Windows-prefix heuristic when the path is not absolute on the current platform) so in-repo paths still return str(path.relative_to(ROOT)) on Windows.

Suggested change
if re.match(r"^[A-Za-z]:[\\/]", path_str) or path_str.startswith("\\\\"):
name = PureWindowsPath(path_str).name or "."
return f"<external>/{name}"
if path.is_relative_to(ROOT):
return str(path.relative_to(ROOT))
if path.is_relative_to(ROOT):
return str(path.relative_to(ROOT))
if re.match(r"^[A-Za-z]:[\\/]", path_str) or path_str.startswith("\\\\"):
name = PureWindowsPath(path_str).name or "."
return f"<external>/{name}"

Copilot uses AI. Check for mistakes.
@RMANOV RMANOV merged commit 6865c2b into main Apr 25, 2026
14 checks passed
@RMANOV RMANOV deleted the fix-windows-path-redaction branch April 25, 2026 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants