Skip to content

test(core): unit-test the shared Unicode sanitizer - #3692

Merged
Astro-Han merged 1 commit into
apache:mainfrom
rekcilyssup:test/text-sanitize
Aug 30, 2026
Merged

Astro-Han merged 1 commit into
apache:mainfrom
rekcilyssup:test/text-sanitize

Conversation

@rekcilyssup

Copy link
Copy Markdown
Contributor

Summary

packages/core/src/text-sanitize.ts is the single source of truth (#1404) for the Unicode sanitize pipeline shared by session names (session-name.ts) and foreign-session names (foreign-session.ts) — explicitly documented as security-adjacent (bidi-spoofing defense) shared infrastructure — but it had no direct unit tests; only callers exercised it transitively.

This adds direct coverage of each pipeline stage and edge:

  • NFC normalization — decomposed macOS filenames compose to match.
  • Control characters → replaced with single spaces (multi-line input stays readable).
  • Bidi format characters (ALM/LRM/RLM/LRE…PDI) → replaced with spaces so direction spoofing collapses.
  • Zero-width characters → removed entirely; includes a compound-emoji case showing code points stay intact.
  • Whitespace collapse + trim, code-point capping without splitting surrogate pairs, silent caps via empty suffix, and inputs that sanitize to empty.

Character classes are asserted via \uXXXX escapes in both source and test, per the note in text-sanitize.ts about keeping files text-safe for git.

No production code is touched.

Verification

  • New suite passes (node --test packages/core/dist/__tests__/text-sanitize.test.js).
  • Full @maka/core suite: 660 tests, 660 pass.
  • npx biome check packages/core/src clean; ASF header check passes.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: opencode — identified the coverage gap, wrote the tests against the documented pipeline, hand-checked expectations against the implementation. Commit carries the Generated-By: opencode trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • No

Cover each pipeline stage (NFC, control/bidi replacement, zero-width
removal, whitespace collapse) plus surrogate-safe code-point capping,
silent caps, and inputs that sanitize to empty. The module is the single
source of truth shared by session names and foreign-session names and
had no direct tests.
Generated-By: opencode
@M4n5ter
M4n5ter force-pushed the test/text-sanitize branch from d139db7 to 69c03b6 Compare August 26, 2026 10:02
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Aug 27, 2026
@Astro-Han

Copy link
Copy Markdown
Contributor

Two questions before this lands, both about scope rather than correctness — the assertions themselves are accurate.

What drove this? There's no issue, no reported bug, and no regression this pins. #1404 establishes that text-sanitize.ts is the single source of truth, but that's a statement about ownership, not a request for coverage. Tests are code with a maintenance cost, so it's worth saying in the body what this is protecting against — a bug you hit, a refactor you're planning, or a specific property you found unprotected.

Some of it is already covered, and more so as of an hour ago. #3846 landed session-name.test.ts, which exercises the bidi class through normalizeUserSessionName across the full U+206A–206F range, and foreign-session.test.ts covers the same class through isSafeForeignId. Your bidi test overlaps both.

The parts that genuinely aren't reachable from either caller are narrower and, I think, the real argument for this PR:

  • capping by code points without splitting surrogate pairs — needs a deliberately constructed astral-plane input at the boundary; no session name naturally produces it
  • NFC normalization — decomposed vs. composed forms matching requires constructing both spellings
  • the silent cap via truncatedSuffix: '' — only session-name.ts:91 uses it, and only for its own length

Those three are worth having as a direct contract. The plain-passthrough, control-character, zero-width, and whitespace-collapse cases mostly restate what the two callers' suites already assert from the other side.

Would you narrow this to the properties the callers can't reach, and say in the body what prompted it? If you think the full-pipeline coverage is worth it as a whole, that's a fine answer too — I'd just like the reasoning on the record rather than inferred.

AI-assisted review: I checked both callers' invocations and their existing suites against head 69c03b670 to work out which properties are only reachable directly. No tests run. AI review is not independent human review.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for adding focused coverage at the shared Unicode-sanitization authority. I reviewed exact head 69c03b6708e7c240130e7eefff4d96f72d3f090f.

This has a concrete contract to protect: sanitizeUnicodeText() is the single pipeline shared by session names and foreign-session text at trust-adjacent boundaries. The tests distinguish the behavior of each stage—NFC normalization, control/bidi replacement, zero-width removal, whitespace collapse, code-point capping, and empty output—so a future drift fails at the owner instead of surfacing only through distant callers.

The 98-line scope is proportionate: one test file, no production change, no new helper or parallel authority, and no repeated full matrix across wrapper APIs. I found no P0-P3 issue. The reported exact-head test and windows_recovery checks are green.

中文对照

感谢在共享 Unicode sanitizer 的权威边界补上聚焦测试。我复核了最新提交 69c03b6708e7c240130e7eefff4d96f72d3f090f

这里有明确需要保护的 contract:sanitizeUnicodeText() 是 session name 与 foreign-session text 共用的唯一处理流水线,位于接近信任边界的位置。测试分别锁定 NFC normalization、control/bidi 替换、zero-width 删除、空白收敛、code-point 截断和空结果,因此未来某个阶段发生漂移时,会在 owner 处直接失败,而不是只靠较远的调用方间接暴露。

98 行范围是成比例的:只有一个测试文件,不改生产代码,不新增 helper 或并行权威,也没有把完整矩阵重复套在多个 wrapper API 上。没有发现 P0-P3。GitHub 报告的 exact-head testwindows_recovery 均为绿色。

AI-assisted review: Codex compared the direct suite with existing caller coverage; I independently verified the problem value, scope, exact head, and severity.

@Astro-Han
Astro-Han merged commit 7bc66fd into apache:main Aug 30, 2026
2 checks passed
Astro-Han added a commit that referenced this pull request Aug 30, 2026
* refactor(runtime): remove unvalidated Auto tool guidance

Remove the prompt fragment, composition wiring, export, and feature-specific tests added by #3705. Maka has no demonstrated retry-loop reproduction or A/B evidence for this guidance, and the implemented best-fit wording does not match the issue’s CLI-first premise.

Generated-by: OpenAI Codex

* test(core): remove speculative sanitizer coverage

Remove the direct sanitizer matrix added by #3692. It was not tied to a reported regression, planned behavior change, or failing contract, while existing session-name and foreign-session tests continue to cover the real trust-adjacent behavior.

Generated-by: OpenAI Codex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/S Under 100 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants