Skip to content

perf(adapters): avoid eager Unicode schema cloning - #4526

Draft
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:agent/unicode-schema-cow-20260913
Draft

luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:agent/unicode-schema-cow-20260913

Conversation

@luvs01

@luvs01 luvs01 commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Avoid eager deep cloning during Unicode tool-schema normalization. Preserve unchanged object identity and copy only changed paths while retaining normalization order and the existing Chat/Responses wire behavior.

Current author verification

Published head: 2014a9b112260861b32dff2b09237f54fbc8ad68. Exact-head ordinary CI run 34994760005 was requested once and is in_progress. The prior Service lifecycle run alone was not treated as a full-suite pass. Known review findings are resolved; no tip-only rebase was performed within the allowed dev-drift window. Current-head CI completion remains pending.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Summary by CodeRabbit

  • Performance

    • Improved Unicode pattern normalization by reducing unnecessary copying while preserving schema and wire behavior.
    • Unchanged schema sections retain their existing structure where possible.
  • Bug Fixes

    • Unsupported Unicode patterns are removed without modifying the original schema.
  • Documentation

    • Added guidance on Unicode pattern normalization across supported integrations and transports.
  • Tests

    • Expanded coverage for nested schemas, shared structures, removed patterns, and input immutability.

@coderabbitai

coderabbitai Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 601affeb-1baa-4dc4-bc09-418a1d2bb982

📥 Commits

Reviewing files that changed from the base of the PR and between aad734a and 2014a9b.

📒 Files selected for processing (6)
  • structure/adapters/registry.md
  • structure/data-planes/inbound-compat.md
  • structure/runtime.md
  • structure/transports/byte-accounting.md
  • structure/transports/inventory.md
  • structure/transports/responses.md

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The Unicode pattern normalizer now uses iterative copy-on-write traversal. It removes unsupported patterns, preserves unchanged references, avoids mutating inputs, and retains existing schema and wire semantics. Tests and documentation cover the new behavior.

Changes

Unicode pattern normalization

Layer / File(s) Summary
Copy-on-write traversal and validation
src/adapters/responses-tool-schema.ts, tests/adapters/openai/openai-chat-hardening.test.ts
The normalizer now uses explicit traversal frames and lazy container cloning. It removes unsupported pattern properties, preserves unchanged subtrees and literal values, returns the original input when unchanged, and keeps the input immutable. Tests cover broad schemas and array paths.
Normalization contract documentation
structure/adapters/registry.md, structure/data-planes/inbound-compat.md, structure/providers/chat-compat.md, structure/providers/cursor.md, structure/runtime.md, structure/transports/byte-accounting.md, structure/transports/inventory.md, structure/transports/responses.md
Documentation describes copy-on-write traversal and states that schema and wire semantics remain unchanged. The byte-accounting document also records identity preservation, immutability, traversal behavior, and test coverage.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 2014a

This change has no identified merge-blocking risk and is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (6 skipped: 6… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main performance change: replacing eager Unicode schema cloning with copy-on-write traversal in the adapter.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (6 skipped: 6 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Sep 13, 2026
@github-actions

github-actions Bot commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

Review readiness checklist

  • ⬜ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ⬜ My PR is ready for review.

2/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 52 / 80

이 PR은 src/adapters/responses-tool-schema.ts 안의 stripUnicodePropertyPatterns를 고칩니다. 지금 dev(HEAD df7dc1be5, 2.54.0)에서는 이 함수가 스키마를 돌면서 방문하는 모든 객체·배열을 미리 복사하고, 형제마다 할당 클로저를 쌓습니다. 그래서 실제로 \p{…} / \P{…} 패턴을 하나도 안 지워도, 필드가 많은 툴 스키마면 출력 트리를 통째로 새로 만듭니다. openai-chat.tsopenai-responses.ts가 요청마다 이 경로를 타므로, 큰 Artifact/툴 스키마에서는 피크 RSS와 할당이 불필요하게 커집니다.

이번 변경은 동작을 바꾸지 않는 복사 시점 최적화입니다. 스택으로 활성 경로만 따라가고, 실제로 패턴을 지운 조상 컨테이너만 복사합니다(copy-on-write). 안 바뀐 형제 객체는 같은 참조를 유지하고, 아무 것도 안 지웠으면 입력 객체 그대로를 반환합니다. name bag(properties 등), const/enum 같은 리터럴, patternProperties/not/oneOf 같은 보존 서브트리, 그리고 별도인 stripResponsesOnlyEncryptedMarker 계약은 그대로입니다. 작성자가 밝힌 측정도 정직합니다. no-op 쪽은 RSS·시간이 줄지만, 패턴을 실제로 지우는 입력은 시간이 조금 늘 수 있다고 적어 두었습니다. 하드 힙 한도가 아니라 할당을 줄이는 최적화라는 점도 structure/transports/byte-accounting.md에 맞게 적혀 있습니다.

구조 문서 여덟 곳이 같은 한 줄을 가리키도록 갱신된 것은 이 저장소의 structure owner 매핑 규칙과 맞습니다. 회귀 테스트 두 개는 “넓은 스키마에서 안 바뀐 형제 정체성 유지”와 “배열 경로·리터럴 보존”을 직접 잠급니다. base는 지금 dev tip과 같고(df7dc1be5), mergeable입니다.

다만 이 PR은 아직 draft입니다. CodeRabbit도 draft라서 리뷰를 건너뛰었고, hygiene/label/enforce-target만 통과한 상태입니다. 작성자도 전체 크로스플랫폼 CI는 아직이라고 적었습니다. 라벨은 enhancement인데 내용은 perf라서, 준비되면 chore/perf 쪽에 가깝게 보이면 됩니다.

라인 - src/adapters/responses-tool-schema.ts stripUnicodePropertyPatterns - 예전 구현은 방문 컨테이너를 전부 eager clone했습니다. 새 구현은 삭제가 난 경로만 clone합니다. 의미는 같아 보이지만, draft 해제 전에 hosted CI(특히 openai-chat-hardening + openai-responses-passthrough)가 초록인지 한 번 더 확인하는 편이 안전합니다.
라인 - 같은 파일의 ownEntries - Object.entries 대신 for…in + hasOwnProperty입니다. 일반 스키마 객체에서는 보통 같지만, 열거 가능한 상속 키가 섞인 이상 입력에서는 예전과 미묘히 달라질 여지는 있습니다. 실무 스키마는 대개 plain object라 위험은 낮습니다.
라인 - 작성자 측정 문단 - changed-path median이 15.1→19.3 ms로 느려질 수 있다고 명시했습니다. no-op 핫패스가 주 목표면 괜찮지만, “항상 더 빠르다”로 홍보하면 안 됩니다.
경로/심볼 - PR draft 상태 - 머지 버튼 전에 Ready for review + 전체 CI가 필요합니다. 지금 tip hygiene만으로는 부족합니다.
경로/심볼 - structure/** 여덟 파일 동일 문장 - structure:check 통과용으로 보이며 내용은 맞습니다. 본문 중복이 거슬리면 나중에 한 owner로만 모아도 되지만, 이번 범위에서 막을 이유는 없습니다.

메인테이너의 판단이 필요한 지점

  • draft를 언제 ready로 올릴지, 전체 CI를 이 PR head에서 반드시 돌릴지
  • changed-path가 약간 느려지는 트레이드오프를 2.54 라인에 그대로 받을지
  • enhancement 라벨을 유지할지, perf/chore로 맞출지

너의 추천
draft를 유지한 채 전체 CI가 초록이 되면 Ready로 올리고 머지하세요. 의미 변경이 없고 dev tip 기준이라 충돌도 없습니다. 지금은 머지하지 마세요. CI와 draft 해제가 먼저입니다.

이 댓글은 grok-bot이 작성했습니다

@luvs01
luvs01 force-pushed the agent/unicode-schema-cow-20260913 branch from 6654819 to a561ddb Compare September 14, 2026 00:41
@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

리뷰 감사합니다. 지적하신 부분을 확인하고 정리했습니다.

ownEntriesfor…in + hasOwnProperty — 여기는 의미 차이가 없습니다. Object.entries는 자기 자신의 열거 가능한 문자열 키만 돌려주고, for…in은 자기 것과 상속된 열거 가능한 문자열 키를 모두 순회하지만 Object.prototype.hasOwnProperty.call(...) 필터가 상속분을 정확히 걸러냅니다. 결과 키 집합은 두 방식이 동일하고, 순서도 정수형 키 오름차순 다음 삽입 순서로 같습니다. 심볼 키는 어느 쪽도 포함하지 않습니다. 따라서 프로토타입이 오염된 이상 입력에서도 동작은 같고, 차이는 중간 배열을 할당하지 않는다는 점뿐입니다. 이 PR의 목적이 할당 감소라서 이렇게 두는 편이 맞다고 봅니다.

성능 서술 — 지적하신 대로 "항상 더 빠르다"로 쓰지 않았고 앞으로도 그렇게 쓰지 않겠습니다. 본문에는 no-op 경로의 RSS·시간 감소와 함께 changed-path median이 15.1 → 19.3 ms로 느려질 수 있다는 점을 그대로 남겨 두었습니다. 목표는 요청마다 타는 no-op 핫패스의 할당 절감이고, 실제로 패턴을 지우는 입력은 스택 순회 비용이 붙습니다.

라벨enhancement보다 perf/chore가 맞다는 지적에 동의합니다. 다만 외부 기여자 권한으로는 라벨을 바꿀 수 없어서(RemoveLabelsFromLabelable 권한 거부) 제가 직접 정리하지 못했습니다. 저장소에 perf 라벨은 없고 chore는 있으니, chore로 옮겨 주시거나 perf 라벨을 새로 두시면 그 기준에 맞추겠습니다.

CI — full cross-platform CI를 이 head에서 돌리고, 특히 openai-chat-hardeningopenai-responses-passthrough가 초록인지 확인한 뒤 결과를 남기겠습니다. 초록이 되기 전에는 draft를 유지합니다. 현재 dev의 windows shard는 tests/clients/desktop-app-restart-posix.test.ts 회귀로 기여자 PR 전반이 막혀 있고, 원인과 수정은 #4564에 올려 두었습니다.

참고로 head가 6654819d4a561ddb64로 바뀌었습니다. 본문과 커밋 메시지에서 저 자신을 가리키던 불필요한 Co-authored-by 트레일러만 제거했고, 트리 내용은 완전히 동일합니다.

@luvs01
luvs01 force-pushed the agent/unicode-schema-cow-20260913 branch from a561ddb to 9c174cc Compare September 14, 2026 01:07
@github-actions
github-actions Bot marked this pull request as ready for review September 14, 2026 01:10
@luvs01
luvs01 force-pushed the agent/unicode-schema-cow-20260913 branch from 9c174cc to c4c9785 Compare September 14, 2026 06:08
@github-actions
github-actions Bot marked this pull request as draft September 14, 2026 06:08
@luvs01
luvs01 marked this pull request as ready for review September 14, 2026 07:30
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-15T01:20:29.592529Z aad734a New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approved on exact head c4c9785d1ef92d09754f91939915bb92b22dc036.

I traced the iterative copy-on-write traversal against the previous eager algorithm. Name bags remain one-level key namespaces, literal/protected subtrees are still skipped, array children reset name-bag state, and every changed descendant is attached through cloned ancestors while untouched siblings keep identity. Deletion occurs only on a cloned output, so the caller's schema is not mutated; a no-op leaves root.output absent and returns the original object.

The wide/deep, array, literal and wire regressions directly cover the material semantics, and the author exact-head ordinary hosted graph is green. The changed-path CPU caveat is documented accurately; this approval is for allocation reduction without claiming a hard heap bound. No merge action was taken.

@github-actions
github-actions Bot marked this pull request as draft September 15, 2026 01:17
@github-actions
github-actions Bot marked this pull request as ready for review September 15, 2026 02:03
…icode-schema-cow-20260913

Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
@lidge-jun
lidge-jun force-pushed the agent/unicode-schema-cow-20260913 branch from aad734a to 2014a9b Compare September 15, 2026 11:14
@github-actions
github-actions Bot marked this pull request as draft September 15, 2026 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants