fix: stop reporting a failed scrub as a successful one - #10
Merged
Conversation
Three paths returned a success-shaped value when scrubbing could not run. 1. `from openadapt_privacy import PresidioScrubbingProvider` raised ImportError on a complete, working install, because the package root never re-exported it. Live consumers wrap that import in `except ImportError` and read it as "openadapt-privacy is not installed", then continue with scrubbing disabled - so text and screenshots reached VLM APIs unredacted while the log said the package was absent. The provider module has no import-time Presidio dependency, so a PEP 562 lazy re-export makes the documented import succeed whenever the package is installed; a genuinely missing provider dependency now surfaces as `PrivacyModelUnavailable` from `_ensure_spacy_model`, which raises rather than passing text through. 2. `ScrubbingProviderFactory.get_for_modality` read `ScrubbingProvider.__subclasses__()` without importing any provider module, so it returned `[]` for TEXT in any process that had only imported the package root. A caller iterating that list scrubs nothing and observes a normal empty result. Built-in provider modules are now imported before the registry is read, so `[]` means "inspected, none supports this modality"; a provider module that fails to import raises `ScrubbingProviderUnavailable` instead of degrading to an empty list. 3. `Screenshot.scrub` returned a new Screenshot carrying `path` unchanged. With no `image` loaded - which is how `DictRecordingLoader` builds every Screenshot - nothing was redacted at all, yet the result was indistinguishable from a scrubbed one and still named the original file. Even with an image loaded, the returned `path` named unredacted bytes on disk, so saving a "scrubbed" recording emitted references to the raw screenshots. Scrubbing an unloaded-but-present image now raises `UnscrubbedScreenshot`, and a scrubbed Screenshot no longer names the unscrubbed file. Also derive `__version__` from installed distribution metadata. The hard-coded `0.1.0` literal was four minor releases stale and is what `openadapt-desktop`'s doctor reports. Every test in `tests/test_no_silent_scrub_bypass.py` was mutation-checked: each production change was reverted individually and the corresponding test confirmed to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Direct answer to the question that prompted this sweep
Yes — a scrub failure could be reported as a successful scrub, and it was happening in live code. Three distinct paths; the first is a real, currently-active PHI bypass in a consuming repo.
1. Package root never re-exported
PresidioScrubbingProvider(highest blast radius)from openadapt_privacy import PresidioScrubbingProviderraisedImportErroron a complete, working install — the symbol was simply never re-exported fromopenadapt_privacy/__init__.py.Two live consumers in
openadapt-evalswrite exactly that import insidetry/except ImportErrorand treat the failure as "the package is not installed":openadapt_evals/workflow/pipeline/scrub.py— logsopenadapt-privacy not installed. PII scrubbing DISABLED. Screenshots will be sent to VLM APIs unscrubbed.and returns the original text/image.openadapt_evals/adapters/scrub_middleware.py— logsPII scrubbing is DISABLED — screenshots will pass through unmodified.and returns the original bytes.So the observable behaviour before this PR: install
openadapt-privacy[presidio]correctly, and scrubbing still silently never runs. The warning names the wrong cause, so the operator's fix ("install the package") cannot resolve it.The provider module has no import-time Presidio dependency (all Presidio imports are inside functions), so a PEP 562 lazy
__getattr__makes the documented import succeed whenever the package is installed. A genuinely missing model still fails loud:_ensure_spacy_model()raisesPrivacyModelUnavailablerather than passing text through.2.
ScrubbingProviderFactory.get_for_modalityreturned[]for "never imported"It read
ScrubbingProvider.__subclasses__()without importing any provider module.__subclasses__()only sees imported classes, so in any process that had imported only the package root,get_for_modality(Modality.TEXT)returned[]— identical to "no provider supports this modality". A caller iterating that list scrubs nothing and sees a normal empty result.Built-in provider modules are now imported before the registry is read, so
[]genuinely means "inspected, none supports this modality". A provider module that fails to import raisesScrubbingProviderUnavailable.3.
Screenshot.scrubreturned an unscrubbed screenshot that looked scrubbedScreenshot.scrubcopiedpaththrough unchanged.imageloaded — which is howDictRecordingLoader._dict_to_recordingbuilds every Screenshot — nothing was redacted at all, yet the result was byte-identical in shape to a scrubbed one and still named the original file.loader.load_and_scrub(src, scrubber, scrub_images=True)therefore returned a "scrubbed" recording whose screenshots were the raw originals.pathnamed unredacted bytes on disk, soloader.save(scrubbed, dest)wrote JSON referencing the raw screenshots.Now: scrubbing an unloaded-but-present image raises
UnscrubbedScreenshot, and a scrubbed Screenshot no longer names the unscrubbed file. A Screenshot with neither image nor path is still a legitimate no-op.scrub_images=Falseis unchanged — that is an explicit, documented opt-out, not a silent one.4.
__version__was a stale literal__version__ = "0.1.0"against a package on 1.0.0.openadapt-desktop'sengine/cli.pydoctor reports that value. Now derived from installed distribution metadata; an unmeasurable version reportsunknownrather than a confident wrong number.Mutation checks
Every test in
tests/test_no_silent_scrub_bypass.pywas verified against pre-fix code by reverting each production change individually:__init__.pytest_presidio_provider_importable_from_package_rootImportError: cannot import name 'PresidioScrubbingProvider' from 'openadapt_privacy'__init__.pytest_reported_version_matches_installed_distributionAssertionError: assert '0.1.0' == '1.0.0'get_for_modalitybodytest_text_modality_is_non_empty_in_a_fresh_interpreterAssertionError: assert 'PRESIDIO' in '[]\n'Screenshot.scrubbodytest_unloaded_image_with_path_raisesFailed: DID NOT RAISE UnscrubbedScreenshotScreenshot.scrubbodytest_scrubbed_screenshot_does_not_name_the_original_fileAssertionError: assert '.../screenshot_001.png' is NoneVerification
uv run pytest tests/ -q --ignore=tests/test_presidio.py→ 68 passed, 1 skippeduv sync --extra dev --extra presidio+spacy download en_core_web_sm,uv run pytest tests/ -q→ 95 passeduv run ruff checkclean on all touched filesCandidates examined and dismissed
PresidioScrubbingProvider.scrub_textreturningNoneforNoneinput — a null passthrough of a null, not a failure disguised as a result._ensure_spacy_model— already exemplary: raisesPrivacyModelUnavailablewith "No scrub was attempted", and is revalidated on every analyzer access so a cached engine cannot mask a config change._filter_automation_false_positives— a deliberate, tested precision filter, not a swallowed failure.tests/test_phi_recall.pymodule-levelpytest.skip— a visible, reported skip, not a silently passing gate.Recording.scrub(scrub_images=False)— an explicit caller-named opt-out.Fake-gate inventory
.github/workflows/contains no|| true,continue-on-error,exit 0,set +e, or swallowed step. Both jobs intest.ymlfail properly. One genuine observation, not fixed here because it is out of this PR's scope: ruff is declared in[tool.ruff]and in thedevextra but is never run in CI, and the tree currently has 6 pre-existing violations inscripts/andtests/. That is a missing gate rather than a fake one.