fix(scrubber): refuse a scrub that could not run instead of reporting zero redactions - #87
Merged
Merged
Conversation
… zero redactions
`Scrubber.scrub_image` caught `ImportError`, copied the raw screenshot into
the `.scrubbed/` directory, and returned `[]` -- the exact value it returns
when Presidio ran and redacted nothing. `_scrub_text_presidio` did the same,
silently downgrading to the BASIC regex pass while the manifest still recorded
`scrub_level: "enhanced"`.
This is reachable on a stock install, not a hypothetical. `openadapt-privacy`
is a hard dependency but Presidio and spaCy are not in the lock, and
`openadapt_privacy.providers.presidio` imports cleanly without them -- it
raises `ModuleNotFoundError` (an `ImportError`) only when asked to scrub. So
`openadapt-desktop scrub <id> --level enhanced` wrote a `.scrubbed/` tree of
unredacted screenshots, a `scrub_manifest.json` claiming enhanced-level
scrubbing with `total_redactions: 0`, a `review_status.json` of
`pending_review`, and advanced the capture to SCRUBBED -- one operator
approval away from egress.
The fix makes "could not run" representable rather than logged:
- `ScrubbingUnavailableError` carries the level and the underlying cause.
- `_require_provider()` constructs the provider AND scrubs a probe string,
because importing the module is not evidence that it works.
- `scrub_capture` preflights the provider before creating any output, so a
refused scrub leaves no partial directory that reads as a completed one.
- `scrub_image` no longer writes `output_path` when redaction did not run: an
unredacted screenshot must never appear inside `.scrubbed/`.
- `_scrub_text_presidio` refuses instead of returning regex output under an
`enhanced` label.
- The dispatcher returns `{"ok": false, "error": ...}`, audits `scrub_refused`,
and leaves the capture in CAPTURED, which the review state machine already
blocks from every outbound path. The CLI prints the refusal and exits 1.
BASIC level is unchanged: copying screenshots without image redaction is what
that level declares, and the manifest records it.
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.
The defect
Scrubber.scrub_imagereturned[]in two different situations that the caller could not tell apart:.scrubbed/unchanged._scrub_text_presidiohad the same shape, silently downgrading to the BASIC regex pass whilescrub_manifest.jsonstill recordedscrub_level: "enhanced".Why it is reachable on a stock install
openadapt-privacyis a hard dependency, but Presidio and spaCy are not inuv.lock, andopenadapt_privacy.providers.presidioimports cleanly without them. It raisesModuleNotFoundError— a subclass ofImportError— only when asked to scrub. Verified against the repo's own locked environment:So
openadapt-desktop scrub <capture_id> --level enhancedproduced:.scrubbed/screenshots/tree of unredacted screenshots,scrub_manifest.jsonclaimingscrub_level: "enhanced"withtotal_redactions: 0,review_status.jsonofpending_review,CAPTURED -> SCRUBBEDtransition — one operator approval away fromREVIEWED, whichengine/review.pytreats as cleared for egress.The engine is fail-closed everywhere else (
engine/policy.py,engine/runner_loop.py,engine/portal/ingress.py). This was the exception.The fix
"Could not run" is now representable, not merely logged:
ScrubbingUnavailableErrorcarrying the level and the underlying causeengine/scrubber.py_require_provider()constructs the provider and scrubs a probe string — importing the module is not evidence that it worksengine/scrubber.pyscrub_capturepreflights before creating any output, so a refused scrub leaves no partial directory that reads as a completed oneengine/scrubber.pyscrub_imageno longer writesoutput_pathwhen redaction did not runengine/scrubber.py_scrub_text_presidiorefuses instead of returning regex output under anenhancedlabelengine/scrubber.py{"ok": false, "error": ...}, auditsscrub_refused, and leaves the captureCAPTUREDengine/dispatch.pyengine/cli.pyBASIC level is unchanged. Copying screenshots without image redaction is what that level declares, and the manifest records it — a documented, representable degradation, not this defect class.
Tests, and the mutation check
New:
tests/test_engine/test_scrubber.py::TestScrubUnavailableIsNotCleanResult(5 cases) andtests/test_engine/test_dispatch.py::TestScrubCaptureRefusal.Every one was confirmed to FAIL against the pre-fix behaviour. Restoring the old
except ImportError: shutil.copy2(...); return [](leaving the tests untouched):with the defect's own signature in the dispatcher case:
Local verification
uv run ruff check engine/ tests/ scripts/— clean.uv run pytest tests/ -q— 752 passed, 6 skipped.Notes
release.ymlisworkflow_dispatchonly, andnative-release.ymlis tag/release: publishedonly.engine/portal/service.py,engine/private_flow_config.py).scrub_capturewithout alevel, so it uses BASIC and is unaffected. It also discards the result; surfacing the refusal inRecordReview.tsxis a separate, smaller follow-up and is deliberately not in this PR.🤖 Generated with Claude Code
https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM