Skip to content

fix(claude): bound bundled-skill marker path inspection - #5606

Open
luvs01 wants to merge 2 commits into
devfrom
stack/claude-skill-marker-bounds
Open

luvs01 wants to merge 2 commits into
devfrom
stack/claude-skill-marker-bounds

Conversation

@luvs01

@luvs01 luvs01 commented Sep 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Bound Claude bundled-skill marker inspection to a 4,096-code-unit directory line, with one extra character to detect the terminating newline. Oversized or newline-free directory lines remain unchanged; normal POSIX, Windows, mixed and UNC basename matching is preserved. The default claude-api blocklist, explicit empty-list opt-out, 10,000-character payload threshold and native Anthropic passthrough are unchanged.

This carries only the Claude source/test portion of #5310, retaining its source author/date and commit attribution. Kiro is consolidated in #5553, while the Fernet portion remains separate; this PR alone does not fully supersede #5310.

Verification

  • Head 305d53910edd276e080219ace13e7653fc620513, based on dev a077087b741a7f7cb1d07b801ec831965d180839.
  • Bun 1.4.0 through the repository wrapper: tests/claude-integration/claude-inbound.test.ts passed 52 tests / 0 failures, including default/disabled/custom policy, Windows paths and exact 4,096/4,097 boundaries. The newline-free fixture was then strengthened to end in a matching basename after 10,000 path segments; that changed case passed separately (1 test / 0 failures).
  • TypeScript, structure, privacy, file-size ratchet and whitespace checks passed. The documentation site built all 497 pages. A commit-message correction after validation left the tested tree unchanged.
  • Focused coverage was used while other worktrees were active; full-suite, exact-head hosted CI and other-platform validation are not claimed. This stays draft.

The oversized-line pass-through is intentional: this optimization does not truncate arbitrary caller text or guarantee that every oversized blocked-skill document is removed. The user guide and owning structure document describe that boundary.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of bundled-skill text with very long directory markers.
    • Long first lines exceeding the supported inspection window are now preserved unchanged.
    • Directory markers at the supported boundary are handled consistently, including paths using different slash styles and letter casing.
  • Documentation

    • Added guidance describing bundled-skill text routing and long-directory handling.
  • Tests

    • Expanded coverage for oversized paths, boundary-length markers, and text without a terminating newline.

Carry only the Claude skill marker paths from the source change. Read at most
4097 path characters before deciding whether the marker can be elided, and
keep oversized marker paths unchanged. Preserve Windows/POSIX basename and
the default claude-api blocklist and explicit blockedSkills override behavior.

Partial source paths: src/claude/inbound.ts; tests/claude-integration/claude-inbound.test.ts.
The source commit also contains separate Kiro and Fernet changes, not included here.

(cherry picked in part from commit fe13531)
@coderabbitai

coderabbitai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b4d13fbc-0222-428f-b57a-d93ef706a31b

📥 Commits

Reviewing files that changed from the base of the PR and between 6d5d501 and 305d539.

📒 Files selected for processing (4)
  • docs-site/src/content/docs/guides/claude-code.md
  • src/claude/inbound.ts
  • structure/clients/claude-desktop.md
  • tests/claude-integration/claude-inbound.test.ts

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


📝 Walkthrough

Walkthrough

The change bounds Claude bundled-skill directory parsing to 4,096 UTF-16 code units. Longer or unterminated directory lines remain unchanged. Tests and documentation cover the new boundary and preserved behavior.

Changes

Claude skill-text elision

Layer / File(s) Summary
Bounded directory-path probe
src/claude/inbound.ts
maybeElideSkillText limits path scanning to 4,096 UTF-16 code units and returns the original text when no newline appears within that bound.
Boundary validation and documentation
tests/claude-integration/claude-inbound.test.ts, docs-site/src/content/docs/guides/claude-code.md, structure/clients/claude-desktop.md
Tests cover 4,096- and 4,097-character paths, oversized paths, separator variants, and newline-free input. Documentation records the bound and unchanged elision policies.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: lidge-jun

Merge Risk: ⚪ Minimal · up to 305d5

No merge-blocking risk was identified in the bounded Claude skill-text parsing change.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: bounding bundled-skill marker path inspection in Claude handling.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (2 skipped: 2 …
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.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

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 bug Something isn't working label Sep 22, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 44 / 80

이 PR은 Claude Code가 보낸 “스킬 문서 묶음” 텍스트를 줄일 때, 첫 줄에 있는 경로를 끝없이 읽지 않도록 막아 줍니다. 라우팅된(Anthropic이 아닌) 모델로 갈 때 Base directory for this skill: …로 시작하는 긴 텍스트에서, 경로 줄이 막힌 스킬 이름(기본 claude-api)과 같으면 짧은 안내문으로 바꿉니다. 예전에는 그 줄의 첫 줄바꿈까지 indexOf("\n")로 통째로 찾았습니다. 줄이 아주 길거나 줄바꿈이 없으면 한 번에 너무 많이 훑을 수 있습니다. 지금은 경로 부분만 UTF-16 코드 단위 4,096까지 보고, 줄바꿈을 알기 위해 한 글자를 더 봅니다. 그 안에서 줄이 끝나지 않고 이미 한도를 넘으면 원문을 그대로 둡니다. 경로 끝의 /·\도 벗긴 뒤 마지막 폴더 이름을 비교해서, 테스트에 나온 것처럼 …/claude-api/ 형태도 막힙니다. 문서(가이드·structure)와 4,096/4,097·줄바꿈 없는 큰 경로 테스트가 같이 옵니다. 파일은 4개(+48/−4)이고 base는 dev입니다. #5310의 Claude 경로 조각만 가져 온 것이고, Kiro는 #5553, Fernet은 따로라서 이 PR만으로 #5310 전체를 대체하진 않습니다. Draft이며 작성자 로컬 포커스 테스트 52개는 통과했다고 적혀 있습니다.

라인 - src/claude/inbound.ts maybeElideSkillText — 경로 첫 줄이 4,096을 넘거나, 그 창 안에 줄바꿈이 없으면 검사하지 않고 원문 통과입니다. 막힌 스킬 문서가 일부러/우연히 긴 경로 줄이면 라우팅 모델에 큰 묶음이 그대로 갈 수 있습니다. 작성자도 문서에 “자르거나 전부 제거를 보장하지 않는다”고 적어 두었습니다.

라인 - SKILL_TEXT_PATH_MAX_CHARS 이름 — 상수 이름은 CHARS인데 가이드·structure는 “UTF-16 code units”라고 합니다. JS length/slice와는 맞지만, 나중에 읽는 사람이 “글자 수”로 오해할 수 있습니다.

라인 - 끝 슬래시 제거 후 lastIndexOf("/") — 예전 split+pop과 흔한 POSIX/Windows/UNC는 같고, 끝 구분자 있는 경로를 더 잘 처리합니다. 동작 변경 폭은 작아 보입니다.

라인 - #5310 — 같은 “skill-path line slice”가 아직 열린 큰 PR에 남아 있습니다. 이 PR이 머지되면 #5310의 Claude 부분은 중복이 됩니다. Fernet·나머지는 이 PR 범위 밖입니다.

라인 - Draft + 보안 체크 미체크 + 포커스 테스트만 — 호스트 풀 CI·exact-head는 작성자도 주장하지 않았습니다. types/config 분할·프리뷰 배포와는 무관합니다.

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

4,096 넘는 경로 줄을 “통과”로 둘지, 창 안의 마지막 구간만으로라도 basename을 짐작해 막을지. #5310은 이 PR·#5553·Fernet 조각이 다 올라온 뒤 닫거나 Claude diff만 빼서 좁힐지. Draft를 Ready로 올리기 전에 호스트 CI(관련 샤드) 초록을 머지 조건으로 둘지.

너의 추천

방향이 맞고, 한도·의도적 통과·테스트 경계가 한줄로 읽힙니다. Ready로 바꾼 뒤 관련 CI만 확인하면 머지해도 됩니다. 긴 경로 통과는 문서화한 DoS 방어 선택이니 기본은 유지하고, 우회가 걱정되면 후속으로 “창 끝 근처 basename”을 따로 논의하세요. 머지 후에는 #5310에서 Claude 경로 조각을 빼거나, 나머지가 다 쪼개졌으면 #5310 닫기를 권합니다. 지금 닫을 types/config 중복 PR은 없습니다.

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

@luvs01
luvs01 marked this pull request as ready for review September 23, 2026 00:04
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants