Skip to content

fix(consolidation): apply sanitize_text to the _DedupDecision merge write path - #3144

Merged
nicoloboschi merged 1 commit into
vectorize-io:mainfrom
iRonin:fix/dedup-decision-sanitize-text
Aug 3, 2026
Merged

fix(consolidation): apply sanitize_text to the _DedupDecision merge write path#3144
nicoloboschi merged 1 commit into
vectorize-io:mainfrom
iRonin:fix/dedup-decision-sanitize-text

Conversation

@iRonin

@iRonin iRonin commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Follows up #2544.

@nicoloboschi on that issue:

the _DedupDecision merge path has no output-side scrub at all (merged_text = decision.text → straight to SQL). That's a real asymmetry worth a small dedicated fix (apply the existing character-safety sanitize_text to all three consolidation write paths)... a clean standalone issue I'd support.

This is that fix, scoped to the merge path.

Character-safety only

sanitize_llm_output is an alias of sanitize_text (llm_wrapper.py), which strips ASCII control characters and lone UTF-16 surrogates — the things that break json.loads and the Postgres UTF-8 encode. This is not the declined content sanitizer; no content filtering is added here.

The asymmetry

_CreateAction and _UpdateAction each scrub text through a sanitize_llm_output field validator. _DedupDecision has only _normalize_action, so the merge path handed the LLM's synthesized text to the fold UPDATE with just .strip() applied.

-        best_id=best_id, merged_text=decision.text.strip() or best_text, should_merge=True, best_text=best_text
+    merged_text = (sanitize_llm_output(decision.text) or "").strip() or best_text

The best_text fallback is preserved: a decision whose text sanitizes away to empty still folds into the twin's existing text rather than blanking the row.

Test

Adds test_dedup_llm_merge_sanitizes_text_before_write next to the existing test_dedup_llm_merge_folds_into_twin, which is left untouched. It injects a NUL into the merge decision and asserts the fold UPDATE argument is clean.

Verification

  • The new test is red without the production line and green with it. The failure is on assert "\x00" not in args[1], and the other 41 tests in the file pass either way — so it detects this gap and nothing else.
  • tests/test_consolidation_dedup.py — 42 passed.
  • tests/test_consolidation*.py — 114 passed, vs 113 on unmodified main; the delta is exactly this test. Both runs report the same 83 setup errors, which are environmental: this checkout lacks the optional sentence-transformers extra.
  • tests/test_llm_wrapper.py — 47 passed.
  • ruff check clean, ruff format --check clean, ty check hindsight_api/ reports no diagnostics in consolidator.py.

…rite path

The dedup merge path passed the LLM's synthesized text straight to the fold
UPDATE with only .strip() applied, so control characters and lone surrogates
reached SQL unscrubbed. _CreateAction and _UpdateAction already scrub their
text via a sanitize_llm_output field_validator; the merge path did not.

Character-safety only (control chars + surrogates), matching the existing
create/update behaviour. Adds a regression test alongside the existing fold
test; that test is left untouched.
@iRonin

iRonin commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the red check, since it doesn't come from this PR.

verify-generated-files — the out-of-sync file is skills/hindsight-docs/references/openapi.json (+16/−2). It was last regenerated in 218e6d3 (2026-07-30); 1ce308f (#3109, 2026-07-31) then changed the bank/document response models (adds last_write_at, reworks last_document_at) without a regen, and the diff shape matches those fields. It reproduces on any PR against main and is unrelated to this change, which only touches a function body in consolidator.py plus a test. I've deliberately kept the regeneration out of this PR rather than mix an unrelated generated file into a two-file fix — ./scripts/generate-docs-skill.sh on main should clear it, and I'm happy to leave that to you.

On test evidencetest-api is gated on detect-changes.outputs.has_secrets == 'true', so it can't run on a fork PR; the green checks here don't exercise the new test. The verification section in the description is the evidence I have: the test is red without the production line (failing on assert "\x00" not in args[1]) and green with it, and the consolidation suite goes 113 → 114, so the delta is exactly this test. A maintainer run with secrets would be welcome.

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve. Correct, minimal fix at the single _dedup_adjudicate chokepoint — covers both reconcile-create and reconcile-update fold paths. Fallback (or best_text) and .strip() semantics preserved; test asserts the char is removed, not just that the text was dropped. Character-safety only, consistent with the declined-content-sanitizer decision on #2544/#2547.

@nicoloboschi
nicoloboschi merged commit dbca379 into vectorize-io:main Aug 3, 2026
89 of 90 checks passed
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.

2 participants