fix(cache): only ALLOW may advance safe-prefix cache - #117
Conversation
SafePrefixState is documented as a verified-clean prefix, but should_advance_prefix() returned True for REDACT (when advance_prefix_on_redact=True, the default) and unconditionally for REVIEW. A later append-only scan of the same document_id could then scan only the tail/overlap and return ALLOW, omitting the earlier flagged content from the resulting findings and redacted output. Fix: should_advance_prefix() now returns True only for Action.ALLOW. The advance_prefix_on_redact config field is deprecated (retained for signature compatibility, no effect). The cache policy fingerprint keeps a stable value to avoid invalidating existing cache entries. Regression tests: - TestShouldAdvancePrefix: ALLOW advances; BLOCK/REDACT/REVIEW/ABSTAIN do not - TestNonAllowPrefixNotCached: Guard-level tests using a controlled fake pipeline to verify REDACT and REVIEW do not create safe-prefix entries, and that appending after REDACT re-scans the full document
Greptile SummaryThe PR enforces that only an ALLOW result can create or advance a verified-clean safe prefix, preventing earlier REDACT or REVIEW findings from disappearing during append-only scans.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code defect remaining. The new ALLOW-only advancement rule matches the verified-clean prefix invariant, Guard applies it consistently, and the regression tests cover both cache state and append-only rescanning behavior.
|
| Filename | Overview |
|---|---|
| sdk/src/unplug/core/runtime/cache.py | Restricts safe-prefix advancement to ALLOW and documents that the compatibility parameter is intentionally ignored. |
| sdk/src/unplug/guard.py | Uses the ALLOW-only cache policy and keeps the policy fingerprint stable for the deprecated default setting. |
| sdk/src/unplug/config/cache.py | Marks the retained but behaviorally ignored cache option as deprecated through Pydantic metadata. |
| sdk/tests/unit/core/runtime/test_cache.py | Adds action-level and Guard-level regressions proving non-ALLOW results do not create prefixes and appended documents are fully rescanned. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Scan full document] --> B{Result action}
B -->|ALLOW| C[Store verified-clean safe prefix]
B -->|REDACT, REVIEW, BLOCK, or ABSTAIN| D[Do not store safe prefix]
C --> E[Later append-only scan]
E --> F[Verify prefix and scan overlap plus suffix]
D --> G[Later append-only scan]
G --> H[Rescan full document]
Reviews (1): Last reviewed commit: "fix(cache): only ALLOW may advance safe-..." | Re-trigger Greptile
chiruu12
left a comment
There was a problem hiding this comment.
Verified it reproduces with real scanners (first chunk needs to be over 256 chars so the finding sits outside the overlap window), and your branch fixes it. make check-ci green, 1093 passed.
One change: drop the str(True) line from the fingerprint instead of keeping a constant. ScanCache is in-memory dicts with no persistence, so an upgrade restarts the process and there are no entries to preserve. A value that never varies just misleads the next reader.
Rest looks good.
Summary
Fixes #116
ScanCache.should_advance_prefix()returnedTrueforAction.REDACT(whenadvance_prefix_on_redact=True, the default) and unconditionally forAction.REVIEW. A safe-prefix entry is documented as a verified-clean prefix, but REDACT and REVIEW are non-ALLOW findings. A later append-only scan of the samedocument_idcould then scan only the tail/overlap and returnALLOW, omitting the earlier flagged content.Invariant enforced: A safe-prefix cache entry may be created only after
Action.ALLOW.Changes
sdk/src/unplug/core/runtime/cache.pyshould_advance_prefix()returnsaction == Action.ALLOWonlysdk/src/unplug/config/cache.pyadvance_prefix_on_redactmarkeddeprecated=Truevia Pydantic Fieldsdk/src/unplug/guard.pyTrueconstant; call passesadvance_on_redact=Falsesdk/tests/unit/core/runtime/test_cache.pyadvance_prefix_on_redacthandlingThe config field is deprecated (Pydantic
Field(deprecated=True)) and retained for signature compatibility. It has no effect — the cache now always requires ALLOW to advance. The cache policy fingerprint usesstr(True)to preserve the same value, avoiding invalidation of existing cache entries.Distinction from #82/#87
PR #87 fixed split-injection patterns across the cache boundary (256-char overlap + source/policy-scoped keys). This fix is about non-ALLOW results being treated as verified-clean prefixes, which is orthogonal to the boundary-overlap fix.
Checklist
dev(see BRANCHING.md)cd sdk && uv run ruff check .passes locallycd sdk && uv run ruff format --check .passes locallycd sdk && uv run pytest -qpasses locally (1088 passed, 59 skipped, 0 failed)test_cache.py)Test plan
New regression tests (fail before fix, pass after):
TestShouldAdvancePrefix: ALLOW advances; BLOCK/REDACT/REVIEW/ABSTAIN do notTestNonAllowPrefixNotCached: Guard-level tests using a controlled fake pipeline:Existing tests preserved:
TestSafePrefixBoundaryGuard: split injection still blocks ([security] Safe-prefix cache skips injection spanning the prefix boundary #82/Fix safe-prefix cache boundary bypass #87 regression)TestIncrementalScan: ALLOW prefix + suffix scan still worksFull suite:
uv run pytest -q— 1088 passed, 59 skipped, 0 failedNotes for reviewers
advance_on_redactparameter onshould_advance_prefix()is retained for signature compatibility but is now intentionally ignored. A future major version can remove it.str(True)(matching the previous default) to avoid invalidating existing cache entries for users who hadadvance_prefix_on_redact=True(the default).