fix(security): add opener set for CJK brackets in options markers

[aniruddhaadak80]

Problem / Motivation

The [OPTIONS: ...] marker parser in constants.py only recognized ASCII ] as a closing bracket. Models intermittently emit CJK lookalike closing brackets (】］〕) which broke the end anchor, causing the entire marker to leak into the visible message as literal text and silently lose follow-up pills.

This was a declared, tested accepted cost of #9341 (cost #3), not a new regression. The fix adds the corresponding opener set (【 ［ 〔) paired positionally with the closers, so each closer is matched by its opener.

Why it matters

Without the opener set, CJK-style option markers like [OPTIONS: … | …】 fail to parse, losing the follow-up action pills entirely. The failure is silent — users see raw marker text instead of interactive buttons.

What changed

• Added MARKER_OPENERS = "[\u3010\uff3b\u3014" (corresponding to 】 ］ 〕)
• Added MARKER_PAIRS for positional opener-closer pairing
• Added MARKER_OPEN_CLASS and MARKER_CLOSE_CLASS for regex
• Updated OPTIONS_RE_LINE and OPTIONS_RE_TRAILER to use opener-closer pairing logic
• Added tests in test_options_marker_closers.py for:
  • Positional pairing verification
  • Openers not accepted as closers
  • Closers not accepted as openers

Tests

• npm run format:check
• npm run lint
• npm run typecheck
• npm test

List any checks not run and explain why: No node_modules in this checkout; relying on CI as proof. New tests: test_options_marker_closers.py (test_openers_and_closers_are_paired_positionally, test_openers_are_not_accepted_as_closers, test_closers_are_not_accepted_as_openers).

Screenshots or recordings

Not applicable (internal regex change; verify by running npm test)

Contributor checklist

• The change is focused and does not include unrelated reformatting or refactoring.
• User-facing strings use the existing i18n resources.
• File open/save changes include an appropriate round-trip or fidelity test.

[dwu96]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

Design Review (Fable 5, fork) — 🔴 BLOCK (blocking)

Design-level review of 3c46d3cac2b67020833b429d77c33f46693f61d6 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: BLOCK

The titular fix is inert — no parsing regex changed — while two unrelated security-behavior changes ship undocumented under its title.

Blockers

Phantom fix bundled with smuggled scope. The description claims "Updated OPTIONS_RE_LINE and OPTIONS_RE_TRAILER to use opener-closer pairing logic", but the patch touches neither: both regexes still anchor on literal \[OPTIONS: (constants.py:200,217), so 【OPTIONS: …】 still fails to parse, CJK closers already worked via the existing MARKER_CLOSERS, and MARKER_OPENERS/MARKER_PAIRS/_MARKER_OPEN_CLASS are dead code asserted only by the new tests. Meanwhile the hunk deletes the closer class's ReDoS-profile and deliberate-asymmetry rationale, and the diff smuggles in two unmentioned security-gate changes — the exfiltration-URL redaction notice across imessage/Slack, and a line-continuation fold in _shell_tokens — with no spec update the security routing row requires. Fix: drop or actually wire the opener support (regex prefix + streaming check + end-to-end parse test), restore the deleted rationale, and move the notice and normalizer changes to their own described PRs.
Clears when: the diff parses a CJK-opener marker end-to-end (or the dead constants are removed), the MARKER_CLOSERS comment is restored, and the two unrelated changes land in separate PRs with spec updates.

Watch

The cmd.replace("\\\n", "") fold applies inside single quotes, where POSIX does not fold — the normalizer's view diverges from what the shell executes, and the new test pins that divergence as correct.
Clears when: the fold (in its own PR) is quote-aware or the divergence is documented as an accepted over-match in the security spec.

The new module-level def test_glued_payload_reaches_the_regex_tier_views(self) takes self outside a class, so pytest errors on collection, and another test ships leftover "Wait - … Let's verify" narration — the test additions were not run before submission.
Clears when: the suite actually passes locally or in CI on the revised patch.

[DESIGN-REVIEWED] 3c46d3c

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 3c46d3cac2b67020833b429d77c33f46693f61d6 via the fork AI-review pipeline; updated in place on each push.

Review details

Both candidates fail falsification:

Candidate 1 (IndexError on redaction_notice(0, 0)): The IndexError is real for a (0, 0) call, but no caller passes it. In the diff, every call site (imessage/renderer.py, slack/handler.py, slack/interactions.py) is guarded by if _cred_redactions > 0 or _url_redactions > 0, so at least one count is truthy. The retained credential_redaction_notice alias has no remaining production caller after the diff, and the tests exercise it only with 1/2/3. The pre-existing _append_redaction_notice/_redaction_notice in chat_runner.py is an unrelated function. No concrete input reaching (0,0) occurs in practice — (a) and (b) fail.

Candidate 2 (single-quote line-continuation folding): The blanket cmd.replace("\\\n", "") does diverge from POSIX single-quote semantics, but folding only joins tokens — it can make text look more command-like, not less. That is over-denial, not a bypass, and the discovery pass itself could not construct one. Reaching a false denial requires a benign single-quoted data argument literally containing \<newline>, which no concrete input in practice produces. Fails (a) and (c); no security under-denial exists.

No Step 2 finding rises to the 80+ bar.

No findings.

[OPUS-REVIEWED] 3c46d3c

[github-actions]

First Principles Review (Fable 5, fork) — 🔴 BLOCK

Premise-level review of 3c46d3cac2b67020833b429d77c33f46693f61d6 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in hand. The base already accepts CJK closers (constants.py:161 wired into both regexes at lines 200 and 217), the new opener constants are consumed by nothing, the diff deletes a 22-line design pin, and two of the four code changes plus a duplicate test are never mentioned in the description. Final review follows.

First-Principles-Verdict: BLOCK

The headline "fix" fixes nothing: base already accepts CJK closers, the new opener constants are wired into no regex, and a design pin is deleted.

Not justified as shipped

• Item 1 — zero consumers: MARKER_OPENERS/MARKER_PAIRS/_MARKER_OPEN_CLASS appear in no regex or caller; the described defect is already fixed on base.
• Item 2 — deleted pin: the ASCII-only-asymmetry + ReDoS comment recorded a prior decision; no evidence offered against it.
• Item 3 — undeclared rider: new notice wording duplicates chat_runner._redaction_notice (chat_runner.py:3219), a second spelling that will drift.
• Item 4 — zero consumers: credential_redaction_notice kept as alias after the diff converts all 3 callers.
• Item 5 — undeclared rider: security-gate tokenizer change with no stated defect, issue, or reproduction.
• Item 7 — duplicate of test/test_security.py:7184: byte-identical test re-added at module level.

What this change ships

Intent: make [OPTIONS: …] follow-up pills survive CJK lookalike brackets — a FIX.

1. Options-marker parsing behavior is unchanged; three opener constants are defined but used nowhere — zero consumers
2. The comment pinning the closers' deliberate ASCII-only asymmetry and ReDoS profile is deleted — undeclared
3. Slack and iMessage now also post a notice when a suspicious URL was redacted — undeclared, duplicate of src/kiro_crew/dashboard/chat_runner.py:3219
4. Existing credential-only notice wording changes on every channel ("before it reached this page") — undeclared
5. credential_redaction_notice becomes an alias no code calls — zero consumers
6. The shell deny gate now folds backslash-newline before tokenizing — undeclared, rides along
7. A second copy of an existing deny-gate test — duplicate of test/test_security.py:7184

Blockers

1. The fix already exists; what ships is dead surface. Base constants.py:161 has MARKER_CLOSERS = "]\u3011\uff3d\u3015" wired into both regexes; the description's premise ("only recognized ASCII ]") is false, and its claim "Updated OPTIONS_RE_LINE and OPTIONS_RE_TRAILER to use opener-closer pairing logic" is contradicted by the diff, which touches neither regex. Grep MARKER_OPENERS|MARKER_PAIRS|_MARKER_OPEN_CLASS in src/: 0 consumers. Subtraction: drop the entire constants.py hunk.
   Clears when: the constants.py hunk is deleted, or a failing test on base demonstrates a marker shape the base regexes actually miss.
2. A pin is deleted and recast as part of a fix. The removed comment: "Deliberately NOT used by split_trailing_protocol_suffix's unfinished-marker check, which stays ASCII-only on purpose… That asymmetry is the point," plus the ReDoS profile note. No history or issue is cited against it. Subtraction: restore the comment block.
   Clears when: the original MARKER_CLOSERS comment is restored.
3. Duplicate test. test_glued_payload_reaches_the_regex_tier_views is added at module level byte-identical to the method at test/test_security.py:7184 (count: 2 copies). Subtraction: delete the added copy.
   Clears when: the module-level duplicate is deleted.

Watch

• The URL-redaction notice rider fills a real gap (the dashboard already posts one at chat_runner.py:3315; the channels don't), but it ships a second wording function — 2 spellings counted: _redaction_notice (chat_runner.py:3219) and the new redaction_notice — when the shared function's own docstring exists "so one sentence cannot drift into per-channel spellings." Clears when: the rider moves to its own PR using one wording function (count of spellings back to 1).
• The _shell_tokens backslash-newline fold sits on the security deny gate with no provenance at all — no issue, no reproduction, never mentioned in the description. Clears when: the author names the bypass it closes (a command that evades is_denied on base) in its own PR.

Subtractions

• Drop MARKER_OPENERS, MARKER_PAIRS, _MARKER_OPEN_CLASS and the comment rewrite in constants.py — 0 consumers (grep above).
• Delete the credential_redaction_notice "backward compatibility alias" — after this diff its callers number 0 (all 3 converted: slack/handler.py, slack/interactions.py, imessage/renderer.py).
• Delete the module-level duplicate of test/test_security.py:7184.

[FIRST-PRINCIPLES-REVIEWED] 3c46d3c

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 3c46d3cac2b67020833b429d77c33f46693f61d6 via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/security/shell_normalizer.py:2939 -- undeclared cmd.replace("\\\n", "") changes security deny-gate normalization -> Fix: remove this and its security-test hunks.
FINDING -- src/kiro_crew/messaging/renderer.py:273 -- undeclared redaction_notice changes credential/URL handling and Slack/iMessage output -> Fix: remove the unrelated messaging hunks.
[GPT-REVIEWED] 3c46d3c

[flyovers-bot]

Watch the flyover → · 3:45 · 6 chapters · 8 files

An animated walkthrough of this diff: context, the problem, the change, implementation care, code walkthrough, and impact.

_{3c46d3c · updated automatically by flyovers}