fix(members): scrub member payloads span-locally, keep legitimate content byte-exact - #8991
fix(members): scrub member payloads span-locally, keep legitimate content byte-exact#8991javenciu wants to merge 2 commits into
Conversation
github-actions
Bot
added
fork
Design Review (Fable 5, fork) — ✅ PASSDesign-level review of Base code confirms the patch's claims: the sibling Design-Verdict: PASS Root-cause fix (inject original, detect on the view) with a fail-closed floor that converts any Unicode-mapping defect into fidelity loss, never forgery admission. [DESIGN-REVIEWED] 9d59b40 |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of All evidence gathered — the base First-Principles-Verdict: CONCERNS The diff outgrew its description: sequence-wise detection and two extra tests are undeclared, so the pasted fails-before/62-passed evidence describes an earlier diff. What this change shipsIntent: stop the member-payload scrub from rewriting legitimate content — replace only forged marker spans. This is a FIX.
Duplication and sibling counts I ran: Watch
[FIRST-PRINCIPLES-REVIEWED] 9d59b40 |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: action required
…tent byte-exact _scrub_member_payload detected forged member-authority markers on a normalized view (NFKC + Cf drop + Pd fold) but then injected the normalized copy of the WHOLE payload, so legitimate content was rewritten: a permanent rule protecting a fullwidth path reached the member naming its ASCII fold instead, and emoji ZWJ sequences and prose dashes in briefings were destroyed. Detection still runs on the normalized view, now built per character with an origin map back to original offsets (the same mechanism _structural_marker_spans already uses, generalized to NFKC), and only the matched spans are rewritten in the ORIGINAL text via the existing _apply_marker_spans. A span touching any part of a multi-char compatibility fold maps to the whole original character, so spans only over-cover (deny direction). Fail-closed floor: the span-scrubbed result is re-checked against the historical whole-string normalized view; any residual match degrades the scrub to exactly the old behavior (normalize whole payload, substitute every match), so a mapping defect can cost fidelity but never admit a forgery. The overlapping-span merge tail is extracted to _merge_overlapping_spans, shared with _structural_marker_spans unchanged. Owning doc paragraph synced. Part of kirodotdev#8524
…atches the fail-closed floor A combining mark inside a marker word (I + U+0307) composes under the whole-string NFKC floor to a char whose case fold is ASCII, but the per-character span view could not compose it: the span pass went blind, the floor fired, and the whole payload was folded - corrupting the legitimate fullwidth content the span-local rewrite exists to protect. Group each base char with its trailing combining marks, normalize the sequence as one unit, and map every emitted view char to the sequence's full original span. Residual starter-composition divergences (Hangul jamo) cannot reach the ASCII marker alphabet and still land on the unchanged fail-closed floor.
d2b7ff5 to
9d59b40
Compare
github-actions
Bot
added
readiness: checking
1 participant
Problem / Motivation
_scrub_member_payload(src/kiro_crew/context.py) neutralizes forged member-authority markers by matching on a normalized view — NFKC-folded, format/zero-width (Cf) characters dropped, every Unicode dash (Pd) folded to-— and then injects the NORMALIZED copy of the whole payload. Detection needs that view; injection does not. The result is that legitimate content is rewritten on the way to the member:Ａ.txt(fullwidth) reaches the member namingA.txt— a subtly different safety boundary than the user wrote (member payload scrub replaces the whole text with its NFKC normalization #8524's own example);👨👩👧loses its joiners);-.Issue #8524 names two candidate resolutions and calls span-level replacement "the reviewer-preferred fix", pointing at the sibling
_structural_marker_spans, which already matches on a normalized view while carrying an origin map back to original offsets.Why it matters
The rules payload is the user-owned safety boundary of the member prompt (layer 3). A scrub that silently rewrites it means the member can be steered by a boundary the user never wrote — the exact fidelity property the trust/ storage design protects everywhere else. Briefings and descriptions are lower stakes but the same contract: scrubbing is for forgeries, not for legitimate bytes.
What changed (motivation → approach → change)
Observed symptom: any member payload containing fullwidth/compatibility characters, ZWJ sequences, or Unicode dashes is rewritten wholesale, even with no forgery present (reproducer below). Root cause:
_scrub_member_payloadreturns the normalized detection view instead of the original text with matched spans replaced.Approach: generalize the origin-map mechanism
_structural_marker_spansalready uses. That function's view folds_MULTIBYTE_TABLE+Cf+Pdbut deliberately not NFKC; the member scrub requires NFKC (fullwidth bracket/letter confusables). So a sibling is added rather than the structural view widened — widening it would change which spans the per-turn context breakdown attributes, an unrelated behavioral surface.Change:
_member_marker_spans(text)— builds the member view (NFKC +Cfdrop +Pdfold) PER CHARACTER with an origin map, matches_MEMBER_MARKER_RESagainst it, and maps spans back to original coordinates. Per-character NFKC differs from whole-string NFKC only in canonical composition across characters, and every such composition yields a non-ASCII character — it can never produce the ASCII bracket/letter/hyphen alphabet the patterns match on, so per-character detection covers everything whole-string detection does. A match touching any part of a multi-char compatibility fold (㎢→km2) maps to the whole original character: spans only ever over-cover (deny direction)._scrub_member_payloadnow rewrites span-locally in the ORIGINAL text via the existing_apply_marker_spans, then re-checks the result against the historical whole-string view (_member_normalized_view, the old normalization extracted verbatim). Any residual match degrades the scrub to exactly the historical behavior — normalize the whole payload, substitute every match. A mapping defect can therefore cost fidelity, never admit a forgery: every return value either passes the whole-string detector clean or IS its output._merge_overlapping_spansand shared with_structural_marker_spansunchanged (byte-identical logic).Alternatives rejected: (a) documenting normalized-injection semantics (issue option 2) — cheaper, but the safety-boundary corruption case remains, and the issue's own text prefers option 1; (b) reusing
_structural_marker_spansdirectly with the member patterns — its view lacks NFKC, so fullwidth forgeries would stop matching (a security regression); (c) widening the structural view to NFKC — changes span attribution for the per-turn context breakdown, an unrelated surface this fix must not move.Tests
test/test_member_prompt.py, three new tests beside the existing forgery-coverage test (which is unchanged and still passes — all 17 forgery shapes, fullwidth and zero-width included, are still neutralized):test_legitimate_fullwidth_content_survives_scrub_byte_exact— the issue's example rule, ZWJ emoji prose, and combining-mark/compat glyphs return byte-exact.test_scrub_is_span_local_around_a_neutralized_forgery— fullwidth content on both sides of a neutralized fullwidth forgery survives verbatim; a multi-char compatibility fold adjacent to a marker survives (over-cover pins the boundary).test_scrub_fails_closed_when_span_mapping_misses— with the span pass monkeypatched blind, the floor returns exactly the historical whole-payload normalized output with the forgery substituted.Fails-before at the unfixed source (fix stashed, tests kept):
With the fix: 62 passed. Neighbor suites (structural-marker neutralization, context user-span metrics, context core): 29 + 92 passed. mypy/flake8/isort/black clean on both changed files (neither is baselined); repo black gate and docs-lint pass.
Manual verification
Related Issues
Addresses #8524 — implements option 1, the issue's reviewer-preferred fix; the design ratification stays with maintainers, so this deliberately does not auto-close.
Pattern harvest
The defect class is "detect on a transformed view, then inject the view instead of the original." The repo's own
_structural_marker_spansdocstring states the correct discipline (span-local rewrite, origin map); this PR brings the member scrub up to it. One adjacent surface was checked and is already correct:_neutralize_structural_markersrewrites span-locally by construction.Rule candidate: when a security filter matches on a normalized/decoded/folded view of user content, the rewrite must map matches back to original coordinates and touch only those spans — returning the transformed view is a silent content-corruption bug, and the fail-closed shape is to re-check the rewritten result against the original whole-view detector and degrade to it on any residual match.
Checklist
Contribution License Agreement
Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).