Skip to content

fix: address PR #14 review feedback (exposure layer)#15

Merged
shaypal5 merged 1 commit into
mainfrom
fix/pr14-review-feedback
Apr 29, 2026
Merged

fix: address PR #14 review feedback (exposure layer)#15
shaypal5 merged 1 commit into
mainfrom
fix/pr14-review-feedback

Conversation

@shaypal5

Copy link
Copy Markdown
Contributor

Summary

  • Rename redaction.pymetadata.py — module writes metadata, not redactions; name now matches behavior
  • Clean stale metadata/ on path reuseapply_exposure() removes any pre-existing metadata/ dir when mode is student_public, preventing accidental hidden-truth leakage
  • Widen get_filter() signature — now accepts str | ExposureMode with automatic coercion via ExposureMode(mode)
  • Add tests — path-reuse leakage regression test + string-mode acceptance test (22 exposure tests pass, 547 total)

Addresses all 4 Copilot review comments from #14.

Test plan

  • All 22 exposure tests pass
  • Full suite: 547 passed
  • Ruff lint + format clean

🤖 Generated with Claude Code

…n get_filter, clean stale metadata/

- Rename `exposure/redaction.py` → `exposure/metadata.py` (name now matches behavior)
- `apply_exposure()` removes pre-existing `metadata/` dir in `student_public` mode
  to prevent hidden-truth leakage on path reuse
- `get_filter()` now accepts `str | ExposureMode` with automatic coercion
- Add regression test for path-reuse leakage and string-mode acceptance test

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 29, 2026 05:45
@shaypal5 shaypal5 added type: bugfix Fixes a bug layer: exposure exposure/ truth-mode filtering labels Apr 29, 2026
@github-actions

Copy link
Copy Markdown

pr-agent-context report:

No unresolved review comments, failing checks, or actionable patch coverage gaps were found on PR
#15. Treat this PR as all clear unless new signals appear.

Run metadata:

Tool ref: v4
Tool version: 4.0.20
Trigger: pull request opened
Workflow run: 25092980048 attempt 1
Comment timestamp: 2026-04-29T05:45:51.708753+00:00
PR head commit: 8087a7063a831dc5ef6db8012bcfd1fdb39476f2

@shaypal5 shaypal5 merged commit c0e76d4 into main Apr 29, 2026
7 checks passed
@shaypal5 shaypal5 deleted the fix/pr14-review-feedback branch April 29, 2026 05:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refines the exposure layer to better match naming/behavior, prevent hidden-truth leakage when reusing output directories, and broaden get_filter() to accept string exposure modes with appropriate errors.

Changes:

  • Renames the exposure metadata writer module to metadata.py and updates imports.
  • Ensances apply_exposure() to delete any pre-existing metadata/ directory when writing student_public bundles.
  • Updates get_filter() to accept str | ExposureMode and adds regression/behavior tests for string modes and path reuse.

Reviewed changes

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

File Description
tests/exposure/test_exposure.py Adds tests for string-mode support and for removing stale metadata/ on output path reuse.
leadforge/exposure/modes.py Updates metadata writer import and removes existing metadata/ for student_public mode.
leadforge/exposure/metadata.py Houses metadata writing logic (module renamed from redaction.py); minor comment cleanup.
leadforge/exposure/filters.py Widens get_filter() to accept string modes via ExposureMode(mode) coercion and updates docs/errors.

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

Comment on lines +40 to +41
elif meta_dir.exists():
shutil.rmtree(meta_dir)

Copilot AI Apr 29, 2026

Copy link

Choose a reason for hiding this comment

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

shutil.rmtree(meta_dir) will raise if bundle_root/metadata exists but is not a directory (e.g., a file). Since this cleanup runs in student_public mode to prevent leakage on path reuse, consider handling both cases (remove dir tree when it’s a dir; unlink when it’s a file/symlink) so save() can’t fail due to an unexpected existing path.

Suggested change
elif meta_dir.exists():
shutil.rmtree(meta_dir)
elif meta_dir.exists() or meta_dir.is_symlink():
if meta_dir.is_symlink() or not meta_dir.is_dir():
meta_dir.unlink()
else:
shutil.rmtree(meta_dir)

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

layer: exposure exposure/ truth-mode filtering type: bugfix Fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants