Skip to content

fix(command-code): preserve canonical path case in project confinement - #6323

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:fix/command-code-path-case-upstream-20260930
Oct 1, 2026
Merged

lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:fix/command-code-path-case-upstream-20260930

Conversation

@luvs01

@luvs01 luvs01 commented Sep 30, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Preserve case in canonical project-context paths instead of lowercasing Windows paths. Require exact component-boundary containment and revalidate the opened file's canonical identity before publishing context.

  • Carry my original change, inspected source head 517a1a4ba59b1a68ff55610695f95c66badf1c83, onto upstream dev.

  • Add drive-root and UNC boundary regressions and ensure the Windows post-open case-change test actually exercises the changed canonical path.

  • Preserve the existing no-follow/inode checks, shared deadline and best-effort Windows boundary. No user filesystem permissions or installed runtime are changed.

  • Integrate upstream dev at 64294638a69e25ca0c7a4e2102e2349973161f71 into head 067801270bcc14d9407d9d4ed34a820bac61247c in response to the October 1 freshness review. Runtime and regression-test files are unchanged from the reviewed head; one documentation line-wrap adjustment preserves the structure budget.

Verification

Historical verification for 06780127 (2026-10-01)

  • Head: 067801270bcc14d9407d9d4ed34a820bac61247c; integrated dev: 64294638a69e25ca0c7a4e2102e2349973161f71; tree: 32cfba59f3d99a4e493086900adfa54472a8b4b7. The merge retains reviewed parent c2f69f26e372a15f40a4715363d3bddda5cee40c without rewriting history.
  • Windows / repository Bun 1.4.0: command-code project-context regressions 42 passed; file-size ratchet 9/9 passed on an unchanged retry. The initial combined run had 50 passes and one repository-scan timeout; no timeout, assertion or baseline was relaxed. The Windows-only post-open case-change test executed.
  • bun node_modules/typescript/bin/tsc --noEmit, bun scripts/structure-ssot.ts, bun scripts/privacy-scan.ts, bun scripts/file-size-ratchet.ts and dev-relative git diff --check passed. A documentation-only reflow corrected the integration's initial 601/600-line structure result without raising the cap.
  • Resource exception: the full local suite and test:changed were not run on this head. Removable-drive I/O delay required an isolated fixed-disk worktree; the focused Windows regressions cover the authored behavior, with broader integration coverage left to CI. Synthetic/local fixtures do not establish protection against every Windows filesystem race.
  • Cross-platform CI 36823104902 succeeded for this head, including all four test shards and the aggregate gate. React Doctor, PR hygiene and target checks passed. The full Windows/macOS matrix was skipped, not counted as passing.
  • CI used its normal PR merge-ref checkout, dd4c3b414c0e53264a76cdf06aaa2bbf28b0a8a3; that checkout tree equals this head's tree above. Historical CI for c2f69f26 is not being used as new-head evidence.
  • Remaining condition: independent re-review of this refreshed head and applicable maintainer security review remain outstanding. Earlier source-review results and readiness checkmarks are not approval of the new head or permission to merge.

Historical verification for c2f69f26 (retained below; not new-head validation or approval)

  • Base: cda8ff4e4eb5c48d7af1f5b108e6551fbdc1e5d3; validated head: c2f69f26e372a15f40a4715363d3bddda5cee40c; tree: 63af6081a3a41de6afddc0889bfea293210790e5. The remote commit/tree match the locally verified objects.
  • Latest inspected upstream dev: 3008bae77043701fe0f6200c9117ab7e1f9da2ee, four commits ahead of the base. That comparison does not change these three affected files.
  • On Windows, bun test tests/providers/command-code-project-context.test.ts: 42 passed, 0 failed. Against the base runtime the same regression set produced 40 passed, 2 expected failures. The Windows-only post-open test executed; the added counter prevents a silent no-op fixture.
  • bun run typecheck, bun run structure:check, bun run privacy:scan and the file-size ratchet passed. The local working tree was clean at the validated commit.
  • Documented resource exception: this focused three-file carry reuses the transfer workspace and verified dependencies. The focused suite covers production loading, canonical containment and post-open identity checks; repeating the full repository suite for each independent carry is disproportionate to the local validation budget. The full local suite and test:changed were not run on this head. Broader integration/platform coverage remains for exact-head hosted CI.
  • Synthetic/local fixtures only. These tests do not establish protection against every Windows filesystem race. No timeout, test baseline or security setting was relaxed.
  • Exact-head hosted CI has completed successfully. Local regression evidence and independent source review support review readiness; automated review will follow the repository's normal Ready event.

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.

Privacy validation and independent source review passed. Canonicalization precedes containment; post-open device/inode and exact canonical identity checks are retained. Windows remains best-effort, not a claim of eliminating all races. Human maintainer security review/sponsorship, where applicable, remains required before merge.

Review readiness

The existing readiness record below is retained. Revision-specific local and CI evidence is recorded above; ready for review does not mean approved, and the refreshed head still needs independent re-review and applicable maintainer security review.

  • Required local validation passed with its scope documented.
  • Branch meets the documented recent-dev criterion.
  • No unresolved Codex or CodeRabbit findings are currently present; new automated findings will be addressed when returned.
  • Ready for review.

Source attribution: the original work and this PR are authored by luvs01. Source commits: 017e3997b6ff79bd268283632d5b37fda62144be and 517a1a4ba59b1a68ff55610695f95c66badf1c83.

Readiness checkpoint

Historical checkpoint for c2f69f26, retained verbatim below. These earlier results and review status do not certify the current head.

  • Exact head c2f69f26e372a15f40a4715363d3bddda5cee40c: Cross-platform CI passed, including all four test shards and the aggregate. Optional skipped jobs are not represented as passing. PR hygiene, target checks and React Doctor passed; the source-attribution warning was corrected by naming the proper fork repository, without inventing coauthor credit.
  • Inspected dev at 592c5cfc043cd5b69e8aea0f12b9a0644cc50612 is five commits ahead of the recorded base with no overlap in these three files.
  • Automated review has not yet completed and is not counted as evidence. No manual bot invocation is requested; follow the repository's Ready-event review flow. This checkpoint is review readiness, not maintainer approval, merge or deployment.

Summary by CodeRabbit

  • Bug Fixes
    • Project file checks now use case-sensitive canonical paths on all platforms, preventing paths with different letter casing from being treated as the same location.
    • Directory containment checks respect path-component boundaries, so similarly named neighboring directories are not treated as contained.
    • Filesystem roots, drive paths, and network-share paths are checked against the correct boundaries.

@coderabbitai

coderabbitai Bot commented Sep 30, 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: 9cd1c434-a837-4ab5-8022-13fdee803cf4

📥 Commits

Reviewing files that changed from the base of the PR and between c2f69f2 and 0678012.

📒 Files selected for processing (1)
  • structure/providers-and-adapters.md

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


📝 Walkthrough

Walkthrough

Canonical path containment now uses case-sensitive equality or a separator-terminated prefix. Post-open confinement also requires an exact path match. Tests cover filesystem roots, Windows drive and UNC paths, component boundaries, and changed path casing.

Changes

Canonical path containment and validation

Layer / File(s) Summary
Canonical containment and validation
src/adapters/command-code-project-context.ts, tests/providers/command-code-project-context.test.ts, structure/providers-and-adapters.md
Containment checks use an exact path match or a separator-terminated prefix. Post-open confinement requires an exact path match. Tests cover root paths, Windows casing, drive and UNC roots, and changed filename casing. Documentation describes the canonical path checks and retains the existing macOS/Linux no-follow open behavior.

Priority: ⚪ Not assessed

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

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 06780

The change tightens path confinement for Command Code project context and adds regression tests, and no merge-blocking issue was found. In the worst case a file is omitted from context when its canonical path changes after it is opened.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to 06780

The change strengthens project confinement by requiring exact canonical path identity. Checks before and after reading remain in place, and no material security risk was found to be introduced or worsened. Windows filesystem race protection remains best-effort.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant exposure is filesystem content selected as project context and subsequently included in a Command Code request. The assessed change affects that selection boundary rather than granting new filesystem authority.

Security Findings and Attack Paths

  • observed — The suspected publication of bytes without post-read confinement validation is contradicted by the caller: it checks before reading, checks again afterward, and returns bytes only after both checks succeed. This supports the supplied assessment retaining no finding for that path.

Trust Boundaries and Controls

  • observed — Canonical containment separates authorized project paths from outside paths. Opened-file validation additionally requires regular-file status, matching device/inode identity, continued containment, and exact canonical path identity.

Resilience and Maintainability Implications

  • observed — The documented platform boundary remains: macOS/Linux retain nonblocking, no-follow open protections and inode checks, while Windows confinement remains best-effort. Exact-case checks strengthen this existing boundary without establishing comprehensive race resistance.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. (1 skipped: 1… 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 clearly and concisely describes the main change: preserving canonical path case during Command Code project confinement checks. It matches the implementation and stated objectives.
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • 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 github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_coauthor_credit — This pull request says it reimplements, supersedes, carries, or rebases another author's pull request, but no Co-authored-by trailer names that author. Prose in a commit body is not read by anything; the trailer is what GitHub counts. Add it to the description or a commit, or obtain attribution-approved. Paths: #673.

@github-actions github-actions Bot added the bug Something isn't working label Sep 30, 2026
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed.

Hygiene

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 30, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 1 / 80

Command Code가 프로젝트 폴더 안에서만 AGENTS.md, taste, SKILL.md를 읽게 가두는 검사를 고친다. 예전에는 윈도우에서 경로를 전부 소문자로 바꾼 다음 비교했다. 윈도우에는 대소문자를 구분하는 폴더가 있을 수 있어서, project와 옆의 PROJECT를 같은 곳으로 볼 수 있었다. 지금은 정규화한 경로를 글자 그대로 보고, 앞부분이 그 프로젝트 폴더인지 검사한다. 파일을 연 뒤에도 경로가 한 글자라도 바뀌면 내용을 내보내지 않는다. 테스트에 드라이브 루트, UNC 공유, 대소문자만 다른 옆 폴더를 넣었다. 건드린 파일은 세 개고 기준 브랜치는 dev다.

라인 - src/adapters/command-code-project-context.ts openedFileIsConfined: 연 뒤 realpath가 열기 전 경로와 완전히 같아야 한다. 대소문자만 바꾼 공격은 막힌다. 보통 윈도우(대소문자 무시 디스크)에서 Node가 같은 파일을 두 번 realpath 할 때 철자만 다르게 주면, 안전한 파일도 읽기를 포기한다. 실패하면 내용을 비우니 보안은 닫히는 쪽이다. 대신 윈도우에서 프로젝트 맥락이 가끔 빠질 수 있다.
라인 - src/adapters/command-code-project-context.ts isContainedCanonicalPath: startsWith는 \와 /가 섞이거나, \\?\ 긴 경로 접두사가 한쪽에만 있으면 같은 폴더라도 바깥으로 본다. 양쪽 다 realpath를 거친 뒤에는 보통 같다. 단위 테스트는 구분 문자를 직접 넣어서, 리눅스에서도 윈도우 문자열만 검사한다.
라인 - tests/providers/command-code-project-context.test.ts rejects a file whose post-open canonical path changes case: 윈도우가 아니면 바로 return한다. 리눅스 CI만 보면 이 분기가 실제로 돌았는지 모른다. 작성자는 로컬 윈도우에서 돌렸다고 적었다.
라인 - PR 설명/커밋: luvs01/opencodex#673을 옮긴다고 했는데, 지금 보이는 커밋 본문에는 Co-authored-by가 없다. 초반 hygiene 봇이 그걸 지적했고, 나중 게이트는 통과로 찍혔다.

메인테이너의 판단이 필요한 지점
이 코드는 파일 가두기라서 보안 리뷰 대상이다. PR 체크리스트의 보안 항목과 리뷰 준비 항목은 아직 비어 있고 Draft다. 보통 윈도우에서 대소문자만 다른 realpath를 거절하는 것이 맞는지, 아니면 가두기만 엄격히 하고 철자는 디스크 규칙에 맞출지가 갈린다. 전체 테스트 묶음은 이 커밋에서 안 돌렸다고 본문에 적혀 있다. 같은 주제의 다른 열린 PR은 보이지 않는다.

너의 추천
보안 쪽에서 의도한 대로면 머지 방향은 맞다. 윈도우에서 같은 파일을 두 번 realpath 했을 때 철자가 달라지는 실측이 있는지만 확인하고, 기여 표시 트레일러를 규칙에 맞게 넣은 뒤 Draft를 풀면 된다. types.ts/config.ts 쪼개기와는 무관하다. 무효가 된 중복 PR을 닫을 대상은 지금 목록에 없다.

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

@luvs01
luvs01 marked this pull request as ready for review September 30, 2026 12:10

@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.

Exact-head correctness re-review of c2f69f2 found no new P0-P2 in the three-file canonical-path change. The pre/post-open containment and inode checks remain intact. My isolated Bun 1.4.0 Linux run returned 42 pass, 0 fail (125 assertions) under CPUQuota=75%, MemoryMax=1536M and fresh temporary homes. The Windows-only post-open branch did not execute on this Linux host; your Windows red/green evidence is separate, not relabeled as my validation. No security scan was run.

HOLD on approval/readiness freshness: current dev 6429463 contains 26 commits missing from this head, beyond the documented 10-commit contributor threshold. This branch still carries package version 2.74.0 while v2.75.0 has since been published. Run 36710996850 passed on this exact head, but that older run does not prove the branch against the now-advanced release/tag state. Rebase onto current dev, preserve the updated structure documentation, and re-attest/rerun required checks on the resulting SHA. I am not requesting a standalone version bump or any gate relaxation.

@luvs01

luvs01 commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

Maintenance verification for 992a49bc38d8a5a8255647fe956a4da3fc94d495:

Merged latest upstream dev (0328373fb88fe0d019b29ae278e153d7fed4bcc7) as requested. The PR-specific command-code source and tests are unchanged by this last dev merge. Earlier focused command-code verification passed 42 cases; after this merge, all 4 snapshot cases and typecheck, structure, privacy and file-size gates passed.

Cross-platform CI succeeded for this exact HEAD; its checkout tree matches the PR HEAD tree. Skipped jobs remain skipped. Local validation used focused tests; the full local suite and test:changed were not run.

This addresses the latest-dev and new-SHA CI request. Independent maintainer approval and the maintainer merge decision remain pending.

@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.

Re-reviewed current head 992a49b. The source and focused command-code test files are byte-identical to the previously reviewed/tested c2f69f2 tree; the latest docs retain the case-preserving confinement contract while reflowing the paragraph. Exact-head Cross-platform CI 36832779453 is successful and the freshness/version condition I raised is cleared. I did not rerun the same passing 42 Linux cases, and that earlier run still does not exercise the Windows-only post-open branch. Your real Windows evidence is separate. No new P0-P2 correctness finding; the applicable filesystem-confinement security review remains outstanding as recorded in the description. This comment is not a security scan, sponsorship, approval or merge.

@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration record (MAINTAINERS.md, dev-only)

  • Decision: integrate fix(command-code): preserve canonical path case in project confinement #6323 into dev for the next release without a second maintainer approval, as the project owner authorized on 2026-10-01.
  • Exact head: 992a49bc38d8a5a8255647fe956a4da3fc94d495. It merged dev 0328373fb8; git merge-tree --write-tree against the current dev is clean (6 commits behind, no conflicts).
  • Hosted CI at this head: Cross-platform CI run 36832779453 succeeded. These jobs ran and passed: test 1/4 to test 4/4, gates, structure gate, storage policy, api usage, docker smoke, keyring ubuntu, keyring windows, npm-global ubuntu-latest, npm-global windows-latest, and ci. React Doctor and the hygiene and enforce-target checks also passed. The Windows-only post-open case-change test does not run in the PR lane. The author's real Windows run in the PR description is the evidence for it.
  • Local suites: not run, by explicit owner instruction.
  • Security review (filesystem confinement): an independent read-only reviewer returned VERDICT: PASS — HEAD 992a49b. Both sides of the check come from realpath. Appending the separator rejects prefix siblings. A \\?\ prefix or a case mismatch can only fail closed. The exact post-open comparison is stricter than the old lowercase one. The no-follow, inode, and deadline checks are unchanged.
  • Findings: Ingwannu re-reviewed the head and found no P0–P2 issue. His freshness hold was cleared by the dev merge. The maintainer review notes that a Windows realpath spelling change could omit project context; that failure mode is closed and accepted.
  • Attribution: authored by @luvs01, squash-merged as is.

@lidge-jun
lidge-jun merged commit faf946e into lidge-jun:dev Oct 1, 2026
35 checks passed
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.

3 participants