Code Review for Obsidian LiveSync

Overview

このスキルは、Obsidian LiveSync プロジェクトのコードレビューを体系的に実施するためのガイドラインを提供します。

Instructions

1. DDD アーキテクチャ観点のチェック

レイヤー構造の確認:

domain/          ← 依存なし（最内層）
application/     ← domain のみ依存可
infrastructure/  ← domain のみ依存可
interfaces/      ← application, domain に依存可

チェックポイント:

レビュー質問:

• 新しい依存関係はレイヤールールに従っているか？
• ドメインモデルは infrastructure の詳細を知らないか？
• トレイトは domain に、実装は infrastructure にあるか？

2. セキュリティチェックリスト

認証情報の取り扱い:

// NG: 認証情報のハードコード
let password = "secret123";

// OK: 環境変数から取得
let password = env::var("COUCHDB_PASSWORD").expect("...");

チェックポイント:

• 認証情報がコードにハードコードされていないか？
• ログに機密情報（パスワード、トークン）が出力されていないか？
• 入力値の検証が行われているか？
• SQL/NoSQL インジェクション対策はあるか？

CouchDB 固有のセキュリティ:

// NG: CouchDB はデフォルトで完全オープン
// セキュリティオブジェクトなしでデータベース作成

// OK: セキュリティオブジェクトを設定
async fn create_secure_database(&self, db_name: &str) -> Result<(), DomainError> {
    // 1. データベース作成
    self.ensure_database(db_name).await?;

    // 2. セキュリティオブジェクト設定
    let security = json!({
        "admins": { "roles": ["_admin"] },
        "members": { "roles": ["user"] }
    });
    let url = format!("{}{}/_security", self.base_url, db_name);
    self.client.put(&url)
        .basic_auth(&self.username, Some(&self.password))
        .json(&security)
        .send().await?;
    Ok(())
}

CouchDB セキュリティチェックリスト:

• データベースにセキュリティオブジェクトが設定されているか？
• _users データベースへの直接アクセスを制限しているか？
• レプリケーション設定に認証情報が含まれているか？
• 添付ファイルのサイズ制限があるか？
• _design ドキュメントへの書き込み権限は適切か？

入力検証:

// NG: 未検証の入力をそのまま使用
let doc_id = request.params.get("id");
db.get_document(doc_id).await?;

// OK: 入力を検証
let doc_id = request.params.get("id")
    .filter(|id| id.chars().all(|c| c.is_alphanumeric() || c == '-'))
    .ok_or(DomainError::InvalidMessage("Invalid document ID".into()))?;

3. パフォーマンス考慮事項

非同期処理:

// NG: ブロッキング処理を async 内で実行
async fn process() {
    std::thread::sleep(Duration::from_secs(1)); // ブロック！
}

// OK: tokio の非同期スリープを使用
async fn process() {
    tokio::time::sleep(Duration::from_secs(1)).await;
}

タイムアウト設定:

// NG: タイムアウトなしのリクエスト
let response = client.get(&url).send().await?;

// OK: タイムアウト付き
let client = Client::builder()
    .timeout(Duration::from_secs(30))
    .build()?;

チェックポイント:

• 長時間リクエスト（longpoll）に適切なタイムアウトがあるか？
• 接続プールが適切に設定されているか？
• 不要な clone() がないか？
• ループ内で毎回アロケーションしていないか？

4. Rust 固有のベストプラクティス

所有権とライフタイム:

// NG: 不要なクローン
fn process(data: &String) {
    let owned = data.clone(); // 必要？
}

// OK: 参照を使用
fn process(data: &str) {
    // 参照のまま使用
}

エラーハンドリング:

// NG: unwrap() の使用（パニックの可能性）
let value = result.unwrap();

// OK: ? 演算子または適切なエラー処理
let value = result?;
// または
let value = result.map_err(|e| DomainError::CouchDbError(e.to_string()))?;

Option/Result の処理:

// NG: 複数の unwrap()
let a = option.unwrap();
let b = a.field.unwrap();

// OK: チェーン処理
let b = option
    .and_then(|a| a.field)
    .ok_or(DomainError::InvalidMessage("Missing field".into()))?;

チェックポイント:

• unwrap() / expect() は本当に必要か？
• &String ではなく &str を使用しているか？
• Vec<T> ではなく &[T] を受け取れないか？
• デッドロックの可能性はないか？

5. テストの品質

チェックポイント:

• ユニットテストがあるか？
• エッジケースがテストされているか？
• モックが適切に使用されているか？
• 非同期テストに #[tokio::test] が使用されているか？

Examples

レビューコメント例

アーキテクチャ違反:

# 指摘
infrastructure 層から application 層への依存が見られます。
DDD のレイヤールールに反しています。

# 提案
`LiveSyncService` の呼び出しを interfaces 層に移動してください。

セキュリティ:

# 指摘
ログにパスワードが出力される可能性があります。

# 提案
debug!("credentials: {:?}", creds) → debug!("credentials present: {}", creds.is_some())

パフォーマンス:

# 指摘
ループ内で毎回 String をアロケートしています。

# 提案
ループ外で String::with_capacity() を使用するか、
イテレータチェーンに置き換えてください。

Reference

レビュー対象ファイル

• livesync-proxy/src/domain/ - ドメイン層
• livesync-proxy/src/application/ - アプリケーション層
• livesync-proxy/src/infrastructure/ - インフラ層
• livesync-proxy/src/interfaces/ - インターフェース層

品質チェックコマンド

cargo fmt --all -- --check  # フォーマット
cargo clippy -- -D warnings # リンター（警告をエラー扱い）
cargo test --verbose        # テスト