Skip to content

fix: stop reporting a failed scrub as a successful one - #10

Merged
abrichr merged 1 commit into
mainfrom
sweep/false-success-privacy
Jul 28, 2026
Merged

fix: stop reporting a failed scrub as a successful one#10
abrichr merged 1 commit into
mainfrom
sweep/false-success-privacy

Conversation

@abrichr

@abrichr abrichr commented Jul 28, 2026

Copy link
Copy Markdown
Member

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 PresidioScrubbingProvider raised ImportError on a complete, working install — the symbol was simply never re-exported from openadapt_privacy/__init__.py.

Two live consumers in openadapt-evals write exactly that import inside try/except ImportError and treat the failure as "the package is not installed":

  • openadapt_evals/workflow/pipeline/scrub.py — logs openadapt-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 — logs PII 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() raises PrivacyModelUnavailable rather than passing text through.

2. ScrubbingProviderFactory.get_for_modality returned [] 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 raises ScrubbingProviderUnavailable.

3. Screenshot.scrub returned an unscrubbed screenshot that looked scrubbed

Screenshot.scrub copied path through unchanged.

  • With no image loaded — which is how DictRecordingLoader._dict_to_recording builds 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.
  • Even with an image loaded, the returned path named unredacted bytes on disk, so loader.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=False is 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's engine/cli.py doctor reports that value. Now derived from installed distribution metadata; an unmeasurable version reports unknown rather than a confident wrong number.

Mutation checks

Every test in tests/test_no_silent_scrub_bypass.py was verified against pre-fix code by reverting each production change individually:

Reverted Failing test Failure
__init__.py test_presidio_provider_importable_from_package_root ImportError: cannot import name 'PresidioScrubbingProvider' from 'openadapt_privacy'
__init__.py test_reported_version_matches_installed_distribution AssertionError: assert '0.1.0' == '1.0.0'
get_for_modality body test_text_modality_is_non_empty_in_a_fresh_interpreter AssertionError: assert 'PRESIDIO' in '[]\n'
Screenshot.scrub body test_unloaded_image_with_path_raises Failed: DID NOT RAISE UnscrubbedScreenshot
Screenshot.scrub body test_scrubbed_screenshot_does_not_name_the_original_file AssertionError: assert '.../screenshot_001.png' is None

Verification

  • uv run pytest tests/ -q --ignore=tests/test_presidio.py → 68 passed, 1 skipped
  • uv sync --extra dev --extra presidio + spacy download en_core_web_sm, uv run pytest tests/ -q95 passed
  • uv run ruff check clean on all touched files

Candidates examined and dismissed

  • PresidioScrubbingProvider.scrub_text returning None for None input — a null passthrough of a null, not a failure disguised as a result.
  • _ensure_spacy_model — already exemplary: raises PrivacyModelUnavailable with "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.py module-level pytest.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 in test.yml fail properly. One genuine observation, not fixed here because it is out of this PR's scope: ruff is declared in [tool.ruff] and in the dev extra but is never run in CI, and the tree currently has 6 pre-existing violations in scripts/ and tests/. That is a missing gate rather than a fake one.

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
@abrichr
abrichr merged commit 3e6c710 into main Jul 28, 2026
4 checks passed
@abrichr
abrichr deleted the sweep/false-success-privacy branch July 28, 2026 03:23
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.

1 participant