feat(web/redaction): SPA confirm-and-retry for redaction-blocked 403 (#785)#802
Merged
feat(web/redaction): SPA confirm-and-retry for redaction-blocked 403 (#785)#802
Conversation
…785) PR #784 unified the trust-boundary redaction guard server-side: the four mutating web routes return ``HTTP 403 {detail: "redaction_blocked", hits, surface}`` (or, for ``/api/upload``, ``HTTP 200`` with per-file ``error="redaction_blocked (hits=N)"``). The SPA did not yet exercise this: the shared ``api()`` helper stringified the structured detail object to ``[object Object]`` (dropping ``hits``/``surface``), and none of the four mutating call sites sent ``force_unsafe`` after a block. This PR wires the SPA-side flow: - ``api()`` parses the FastAPI-nested ``{detail: {detail, hits, surface}}`` and throws a typed ``RedactionBlockedError``; non-redaction 4xx with a dict ``detail`` no longer surfaces as ``[object Object]`` either. - ``apiWithRedactionRetry()`` (new) wraps ``api()``: on the typed error, shows ``showConfirm`` with the matched-pattern count and a localized surface label, then retries with ``body.force_unsafe = true``. Returns ``null`` on user cancel so the existing call-site catches keep working. - ``uploadFilesWithRedactionRetry()`` (new) covers ``/api/upload``'s divergent shape: scans per-file error strings, sums hits across files, and re-issues the original FormData with ``?force_unsafe=true`` on confirm. Re-uploading already-OK files is idempotent thanks to the server's ``_{mtime_ns}`` collision suffix (system.py:1121). - Three JSON call sites swap to the wrapper: ``d-save-btn`` PATCH, browse-source PATCH, ``/api/add`` POST (after the existing client-side privacy pre-check; the two layers can't both fire because the pre-check sets ``force_unsafe=true`` so the server never returns 403). - Two upload call sites (Index-tab Upload mode and Search-tab drag-drop) share the multipart helper; the previous duplicated 403-stringify logic is gone. - ``settings-harness.js`` scratch promote uses the wrapper too. - 9 new i18n keys in ``en.json`` + ``ko.json`` lockstep: ``confirm.redaction_blocked_*``, ``toast.redaction_bypassed``, and ``surface.web_api_*`` for the four route labels. - Cache-bust: ``app.js?v=102`` (from 101), ``settings-harness.js?v=2`` (from 1). Body changed; without the bump disk-cached scripts would hold the old ``api()`` stringify. Tests: ``tests/web/test_redaction_blocked_retry.py`` adds two browser specs — ``/api/add`` 403 → confirm → retry-with-body-flag, and ``/api/upload`` per-file string → confirm → retry-with-query-param. Both route-stub the backend per the existing ``mm_web_url`` fixture pattern. The metric-counter assertion the issue body asks for (``mem_add_redaction_stats[bypassed][by_tool]``) needs a real-backend fixture and is documented as a follow-up in the test docstring. Out of scope: backend (correct in #784), CLI/MCP/LangGraph (native ``--force-unsafe``/kwarg already), new redaction patterns (``privacy.py`` module docstring). Refs #785 (closes), #784 (Codex review follow-up). Co-Authored-By: Claude <[email protected]>
Follow-up to PR #802 self-review. Three small changes addressing review nits raised during the second pass; no behavior change for the golden path. - ``toast.upload_redaction_cancelled`` (new key, en/ko lockstep) replaces the previous reuse of ``toast.upload_failed`` + ``confirm.redaction_blocked_title`` as the cancel-toast string. The reused phrasing rendered as "Upload failed: Redaction guard blocked write" which mixes a failure verb with a guard label and drops the file count. The dedicated key surfaces the actionable metric (number of files left blocked). Placeholder named ``{count}`` rather than ``{hits}`` to keep the pattern-matches semantics of ``{hits}`` (used in ``toast.redaction_bypassed``) distinct from the file-count semantics here — same placeholder name across surfaces with different units would be a translation trap. - ``uploadFilesWithRedactionRetry()`` return shape gains ``blockedFileCount`` so the upload-tab caller can localize the cancel toast without re-scanning ``data.files``. Search-tab drag- drop caller is unaffected (only reads ``cancelled``/``data``). - Comment correction: the multipart retry is *non-conflicting* (the server's ``_{mtime_ns}`` collision suffix prevents overwrite) but not strictly idempotent — the same content lands twice under two filenames when a mixed batch is retried. Acceptable trade-off but worth stating accurately so the next reader doesn't assume a stronger guarantee. - Both browser specs gain a ``confirm-modal.hidden`` wait after the confirm-ok click. Guards against a regression where ``showConfirm`` resolves but the modal stays visible — would block subsequent interaction without surfacing as a request-level failure. - ``app.js?v=102 → 103`` for the body change. en/ko parity: 701/701 keys, set diff empty. ``ruff check`` + ``ruff format --check`` clean. ``test_i18n`` (20) + ``test_redaction_blocked_retry`` (2) all green. Co-Authored-By: Claude <[email protected]>
This was referenced May 5, 2026
Self-review on PR #802 surfaced two items. * ``d-save-btn`` (app.js:2316) called ``_pushHistory`` before the request, polluting the undo stack with a no-op entry when the user cancels the redaction-blocked confirm dialog. Move the push to after the ``apiWithRedactionRetry`` null-cancel early-return so history only records committed writes. * Add ``test_api_add_403_cancel_does_not_retry_or_emit_bypass_toast`` to ``test_redaction_blocked_retry.py``. The affirmative spec pinned that confirm triggers a retry, but a regression where ``showConfirm`` always resolves true (or ``apiWithRedactionRetry`` drops its ``if (!ok) return null`` branch) would still pass it. The new spec is the symmetric negative: cancel -> exactly one POST, no ``toast.redaction_bypassed``. Mutation-validated by stripping the cancel branch -- the new spec fails as expected (``2 == 1``). Bump ``app.js?v=103`` -> ``v=104`` for the cache-bust. Co-Authored-By: Claude <[email protected]>
6 tasks
memtomem
pushed a commit
that referenced
this pull request
May 5, 2026
…bypassed Codex review on PR #805 flagged that ``uploadFilesWithRedactionRetry`` fired ``toast.redaction_bypassed`` ("entry written") on any HTTP-200 retry, even when ``retryData.files`` had fewer rows than the narrowed ``blockedRows`` or the rows still carried ``error`` fields. Because ``/api/upload`` reports per-file failures inside a 200 response, HTTP status alone is not proof the bypass actually wrote the blocked files — a non-redaction validation error or a redaction class the ``?force_unsafe=true`` query param did not cover would still surface as "entry written" to the audit log. Fix --- Validate the retry response shape before showing the success toast: ``retryFiles.length === blockedRows.length`` AND every retry row must be error-free. On a partial or malformed retry, suppress the success toast and emit a new ``toast.redaction_bypass_partial`` naming the succeeded/total counts; the helper's ``bypassed`` return field tracks the same boolean so callers can branch on accurate state. The merged ``data.files`` already keeps unmatched original rows in place, so the per-file UI continues to surface the truth — only the aggregate toast needed to stop lying. The fix builds on top of the #803 narrowed-retry logic: index-aligned merge already handled the partial-success shape, but the toast/bypass return claimed full success unconditionally. Tests ----- Two new browser specs in ``test_redaction_blocked_retry.py`` pin the two failure shapes Codex called out: * ``test_api_upload_retry_with_persistent_error_row_does_not_claim_bypassed`` — same row count, retry row carries non-redaction ``error``; asserts zero "Bypassed redaction guard" toasts and exactly one "Bypass partial" toast naming 0 of 1. * ``test_api_upload_retry_with_truncated_row_count_does_not_claim_bypassed`` — server returns fewer retry rows than the narrowed FormData carried; asserts zero "Bypassed" toasts and exactly one "Bypass partial" toast naming 1 of 2. Mutation-validated per ``feedback_pin_test_mutation_validation.md``: hard-coding ``fullySucceeded = true`` flips both new specs to fail before being restored. Bumps ``app.js?v=105 → v=106`` for the cache-bust. ``en.json`` + ``ko.json`` ship the new ``toast.redaction_bypass_partial`` key with matching ``{succeeded}``/``{total}`` placeholders so ``test_i18n.TestI18nParity`` stays green. Full CI floor (``uv run pytest -m "not ollama"``) reports 4200 passed, 8 skipped. ``ruff check`` + ``ruff format --check`` clean. Refs PR #802, PR #805, issue #785. Co-Authored-By: Claude <[email protected]>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Summary
PR #784 unified the trust-boundary redaction guard server-side. The four
mutating web routes now signal a hit in two shapes:
/api/add,PATCH /api/chunks/{id}, scratch promote)return
HTTP 403 {detail: {detail: "redaction_blocked", hits, surface}}.POST /api/uploadreturnsHTTP 200with per-fileerror="redaction_blocked (hits=N)"and accepts?force_unsafe=true.The SPA did not yet exercise this:
api()stringified the nested detailobject to
[object Object](droppinghits/surface), none of the fourmutating call sites sent
force_unsafeafter a block, and there was noPlaywright smoke for the UX. This PR closes that gap.
What changed
api()helper parses the FastAPI-nested{detail: {detail, hits, surface}}and throws a typedRedactionBlockedError. Non-redaction4xx with a dict
detailno longer surfaces as[object Object].apiWithRedactionRetry()(new) wrapsapi(): on the typed error,shows
showConfirmwith the matched-pattern count and a localizedsurface label, then retries with
body.force_unsafe = true. Returnsnullon user cancel so existing call-site catches keep working.uploadFilesWithRedactionRetry()(new) covers/api/upload'sdivergent shape: scans per-file errors, sums hits, re-issues the
original FormData with
?force_unsafe=trueon confirm. Mixed batches(≥1 clean + ≥1 blocked) currently re-write clean files under the
server's
_{mtime_ns}collision suffix (system.py:1121); tracked asa follow-up — see web/upload: redaction-blocked retry duplicates clean files in mixed batches #803.
d-save-btnPATCH,browse-source PATCH,
/api/addPOST (after the existing client-sidepre-check — both layers can't both fire because the pre-check sets
force_unsafe=trueso the server never returns 403), Upload tabclick, Search-tab drag-drop.
settings-harness.jsscratch promoteuses the wrapper too.
en.json+ko.jsonlockstep:confirm.redaction_blocked_*,toast.redaction_bypassed, andsurface.web_api_*for the four route labels.app.js?v=104(from 101),settings-harness.js?v=2(from 1).
Tests
tests/web/test_redaction_blocked_retry.pyadds three browser specsthat route-stub the backend per the existing
mm_web_urlfixturepattern:
/api/add403 → confirm dialog naminghits=2+ "Add memory" →click proceed → second POST carries
force_unsafe: truein the body./api/uploadper-fileredaction_blocked (hits=3)→ confirmdialog naming
hits=3+ "File upload" → retry sends?force_unsafe=truequery param./api/add403 → confirm dialog → click Cancel → exactly onePOST recorded, no
toast.redaction_bypassed. Symmetric negative perfeedback_pin_invert_symmetric_assertion.mdso a regression whereshowConfirmalways resolves true (orapiWithRedactionRetrydropsits
if (!ok) return nullbranch) doesn't pass silently.Mutation-validated per
feedback_pin_test_mutation_validation.md:removing
if (!ok) return nullfromapiWithRedactionRetryflips thenew cancel spec to fail (
AssertionError: cancel must not trigger retry, saw 2 POSTs) before being restored.Full CI floor:
uv run pytest -m "not ollama"reports 4197 passed, 8skipped, 0 failed.
ruff check+ruff format --checkclean.Follow-up
Spun out of Codex review on this PR; deferred to keep the scope tight on
the single-file confirm-and-retry path that issue #785 specifies:
on retry and cancel-leaves-UI-stale were spun out for review focus
but folded into PR fix(web/redaction): narrow retry FormData to blocked files only (#803) #805 stacked on this branch.
The issue body asks for the smoke to assert
mem_add_redaction_stats[bypassed][by_tool]increments. That needs areal-backend Playwright fixture which
tests/web/conftest.pydoes notprovide (
lifespan=Noneby design — see fixture docstring). The routecounter is already pinned by
test_web_routes.py::TestAddMemoryRedactionon the backend side; surfacing it in the SPA suite is tracked separately.
Out of scope
--force-unsafe/ kwarg).privacy.pymodule docstring).Test plan
uv run pytest -m "not ollama"— full CI floor greenuv run ruff check packages/memtomem/{src,tests}— cleanuv run ruff format --check packages/memtomem/{src,tests}— cleanuv run pytest packages/memtomem/tests/web/test_redaction_blocked_retry.py— new browser specs greenmm websmoke: pastesk-…into Add → expect dialog → proceed → row lands andmem_add_redaction_statssnapshot shows theweb_api_addbypass incrementRefs #785 (closes), #790 (closes — same four chunk-edit / upload sites the helper covers, filed as a follow-up before this PR landed), #784 (Codex review follow-up). Follow-ups #803 and #804 are addressed in stacked PR #805.
🤖 Generated with Claude Code