Skip to content

[#778] fix-source-review-issues#779

Open
nrslib wants to merge 1 commit into
mainfrom
takt/778/fix-source-review-issues
Open

[#778] fix-source-review-issues#779
nrslib wants to merge 1 commit into
mainfrom
takt/778/fix-source-review-issues

Conversation

@nrslib
Copy link
Copy Markdown
Owner

@nrslib nrslib commented Jun 1, 2026

Summary

確認範囲

  • /home/opa/work/takt
  • 静的レビューのみ。テストや実行確認はしていません。
  • 主に clone/exec と repertoire import 周辺を確認しました。

Findings

  1. High: worktree の .git relative gitdir: を process cwd 基準で解決しています。

    • 対象: src/infra/task/clone-exec.ts:42
    • Git の gitdir: 相対パスは .git ファイルのあるディレクトリ基準です。現在は path.resolve(worktreePath, '..', '..') と cwd 依存の解決になり、別ディレクトリから実行すると main repo を誤認して --reference clone が壊れます。path.dirname(gitPath) 基準で解決してください。
  2. Medium: root 配下判定が / 固定で Windows path を壊します。

    • 対象: src/features/repertoire/takt-repertoire-config.ts:194
    • realPath.startsWith(realRoot + '/') は Windows の \ separator で valid child path を拒否します。既存の path boundary helper か path.sep/path.relative ベースにしてください。
  3. Medium: repertoire add の temp path が時刻ベースで予測可能です。

    • 対象: src/commands/repertoire/add.ts:71
    • shared temp dir に takt-import-${Date.now()} を作るため、衝突や symlink race の余地があります。fs.mkdtempSync(path.join(tmpdir(), 'takt-import-')) を使ってください。

Execution Report

Workflow takt-default completed successfully.

Closes #778

Summary by CodeRabbit

リリースノート

  • テスト

    • ワークツリー参照解決の動作検証テストを追加
    • レポジトリ追加処理の一時ファイル取り扱いを検証するテストを追加
    • Windows パス境界の検証ロジックテストを追加
  • バグ修正

    • 一時ファイルのクリーンアップ処理を改善
    • パス検証ロジックを強化し、より正確な検証を実装
    • ワークツリー形式のリポジトリ参照解決を修正

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 1, 2026

Review 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: Pro Plus

Run ID: 36bd3271-6d82-4fdd-987e-ac186665ea25

📥 Commits

Reviewing files that changed from the base of the PR and between 7cd9018 and 563dd4a.

📒 Files selected for processing (6)
  • src/__tests__/clone.test.ts
  • src/__tests__/repertoire/add.test.ts
  • src/__tests__/repertoire/takt-repertoire-config-windows-boundary.test.ts
  • src/commands/repertoire/add.ts
  • src/features/repertoire/takt-repertoire-config.ts
  • src/infra/task/clone-exec.ts

📝 Walkthrough

Walkthrough

このPRは、Git worktreeのgitdir相対パス解決、repertoire addの一時ディレクトリ生成、およびWindows対応のパス境界検証の3つの修正を実装します。worktreeの.gitファイル内のgitdir相対参照をプロセスcwd基準ではなく.gitファイルのディレクトリから解決し、一時ファイルのタイムスタンプベース命名をmkdtempSyncに置き換え、パス包含判定をプラットフォーム依存の手書き文字列比較からisPathInsideユーティリティに変更します。

Changes

Worktree gitdir解決とclone参照パス修正

Layer / File(s) Summary
Clone gitdir path resolution logic
src/infra/task/clone-exec.ts
resolveMainRepo.git ファイルのディレクトリを基準にgitdir相対パスを解決し、commonGitDir を経由してメインリポジトリディレクトリを導出するよう修正されました。
Worktree reference resolution tests
src/__tests__/clone.test.ts
fs.statSync のモック追加、import の複数行化、新しい describe('worktree reference resolution') ブロックにより、createSharedClonecreateSharedCloneAbortable--reference パラメータが解決済みのメインリポジトリパスを正しく使用することを検証します。

セキュアな一時ディレクトリ処理

Layer / File(s) Summary
Temporary directory implementation
src/commands/repertoire/add.ts
mkdtempSync を追加して takt-import- プレフィックス付きのセキュアな一時ディレクトリを生成し、アーカイブ・展開・includeファイルを その配下に配置、finally ブロックで tmpBase をまとめて再帰削除するよう変更されました。
Temporary directory test coverage
src/__tests__/repertoire/add.test.ts
repertoireAddCommand の mkdtemp ベースのライフサイクル、extract サブディレクトリ作成、アーティファクト生成、単一回の rmSync 呼び出しによるクリーンアップを検証する新規テストが追加されました。

パス境界検証のWindows対応化

Layer / File(s) Summary
Path containment validation refactor
src/features/repertoire/takt-repertoire-config.ts
validateRealpathInsideRoot の判定ロジックが、ハードコードされた startsWith(realRoot + '/') パターンから isPathInside(realRoot, realPath) ユーティリティ関数への委譲に変更されました。
Windows path boundary tests
src/__tests__/repertoire/takt-repertoire-config-windows-boundary.test.ts
node:path を win32 モードでモック化し、Windows パス区切り文字 \ 環境において、ルート配下のパスが許可され、接頭辞が同じでも兄弟パスが拒否されることを検証する新規テストが追加されました。

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title '[#778] fix-source-review-issues' directly references the linked issue and describes the main purpose of the PR (fixing review findings).
Linked Issues check ✅ Passed All three coding requirements from issue #778 are fully addressed: worktree gitdir resolution corrected in clone-exec.ts, Windows path boundary check fixed in takt-repertoire-config.ts, and secure mkdtempSync used in add.ts.
Out of Scope Changes check ✅ Passed All changes are directly related to the three findings in issue #778; test additions verify the fixes and are appropriately scoped.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch takt/778/fix-source-review-issues

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 and usage tips.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ソースレビュー指摘 (2026-05-30)

1 participant