Skip to content

Conversation

@hayasaki-shunsuke
Copy link
Contributor

@hayasaki-shunsuke hayasaki-shunsuke commented Jan 19, 2026

Summary

  • SyncAll() 実行時に、Dify から取得したアプリ一覧と app_map.json を比較し、新規アプリを自動追加する機能を実装
  • これにより difync init を再実行せずに、通常の sync で新規アプリが取り込まれるようになる
  • ファイル名重複回避ロジックを共通化(generateUniqueFilename() ヘルパー関数)

背景・課題

現在の difync(sync 動作)は app_map.json に登録済みのアプリのみを同期する。Dify に新しいアプリが追加されても app_map.json
には追加されないため、新規アプリを取り込むには difync init の再実行が必要だった。

しかし difync init を複数回実行すると、既存ファイルとの重複回避でサフィックス(_1, _2)が付き、ファイルが増殖する問題があった。

期待する動作

$ difync
Starting sync...

Sync Summary:
Total apps: 155
Downloads: 2
Added: 2 ← 新規追加
No action (in sync): 151
Errors: 0

Test plan

  • 既存のテストがすべてパスすることを確認
  • TestSyncAllWithNewApps テストを追加して新機能を検証
  • ビルドが成功することを確認

🤖 Generated with Claude Code

kentaro and others added 4 commits January 13, 2026 17:59
fix: Support new Dify API cookie-based authentication
Previously, GetAppList only fetched the first page of results (default 20 apps).
When has_more was true, subsequent pages were not fetched, causing apps beyond
the first page to be incorrectly identified as "deleted remotely" and triggering
unwanted deletion of local DSL files.

This fix:
- Loops through all pages when has_more is true
- Adds page parameter to API requests
- Accumulates results from all pages before returning
- Adds comprehensive tests for pagination scenarios

Fixes the issue where all DSL files were deleted when syncing with
Dify instances containing more than 20 apps.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fix: GetAppList APIのページネーション未対応によるDSLファイル全削除問題を修正
SyncAll() 実行時に、Dify から取得したアプリ一覧と app_map.json を比較し、
新規アプリを自動的に検出して追加するようにした。

これにより、Dify に新しいアプリが追加された場合でも difync init を
再実行する必要がなくなり、通常の sync で新規アプリが取り込まれる。

変更内容:
- SyncStats に Added フィールドを追加
- generateUniqueFilename() ヘルパー関数を追加(重複回避ロジック共通化)
- SyncAll() に新規アプリ自動追加ロジックを実装
- TestSyncAllWithNewApps テストを追加

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@yoshikouki yoshikouki left a comment

Choose a reason for hiding this comment

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

レビュー結果

新規アプリの自動追加機能は非常に有用です。generateUniqueFilename()の共通化も良いリファクタリングです。ただし、エラーハンドリングに重大な問題があるため修正が必要です。


🔴 重大な問題

1. DSL取得失敗時のapp_map.json不整合

問題箇所: internal/syncer/syncer.go:380-420

現在の実装では、DSL取得/書き込みに失敗してもnewAppsへの追加が既に済んでいるため、app_map.jsonにファイルが存在しないアプリが記録されます。

// ❌ 現在の実装
newApp := AppMapping{
    Filename: filename,
    AppID:    appID,
}
newApps = append(newApps, newApp)  // ダウンロード前に追加

// Download DSL for new app
if !s.config.DryRun {
    dsl, err := s.client.GetDSL(appID)
    if err != nil {
        stats.Errors++
        continue  // ここでスキップされるが、newAppsには既に追加済み
    }
}

影響:

  • 次回のsync時にos.Statエラーが発生し続ける
  • ユーザーは手動でapp_map.jsonを編集する必要がある

修正案:

// ✅ 修正後
newApp := AppMapping{
    Filename: filename,
    AppID:    appID,
}

// Download DSL for new app
localPath := filepath.Join(s.config.DSLDirectory, filename)
if !s.config.DryRun {
    dsl, err := s.client.GetDSL(appID)
    if err != nil {
        fmt.Printf("Warning: Failed to download DSL for new app %s: %v\n", appInfo.Name, err)
        stats.Errors++
        continue  // 成功前にスキップ
    }

    if err := os.WriteFile(localPath, dsl, 0644); err != nil {
        fmt.Printf("Warning: Failed to write DSL file for new app %s: %v\n", appInfo.Name, err)
        stats.Errors++
        continue  // 成功前にスキップ
    }

    if s.config.Verbose {
        fmt.Printf("Downloaded DSL for new app %s to %s\n", appInfo.Name, localPath)
    }
}

// ✅ 成功した場合のみ追加
newApps = append(newApps, newApp)
stats.Added++
stats.Total++

🟡 軽微な改善提案

2. Total apps統計の意味が不明確

stats.Totalは削除分を差し引かず、新規追加分のみ加算されるため、削除と追加が同時に発生するケースでリモートの総数と乖離します。

該当箇所: internal/syncer/syncer.go:418, cmd/difync/main.go:124

提案:

  • 表示文言を「Processing apps」などに変更する
  • または、削除分を差し引いて実際のリモート総数と一致させる

3. テストカバレッジの追加

TestSyncAllWithNewAppsは正常系のみです。以下のエラーケースも追加すると安心です:

  • 新規アプリのDSL取得が失敗するケース
  • ファイル書き込みが失敗するケース
  • 複数の新規アプリで一部が失敗するケース

✅ 良い点

  1. ファイル名生成ロジックの共通化(generateUniqueFilename())
  2. Addedカウントで新規追加数が可視化される
  3. 詳細なログ出力(Verboseモード)
  4. 新機能に対するテストの追加

質問

  • 新規DSL取得に失敗した場合でもアプリマップに残す意図はありますか?
    • 意図がない場合は、上記の修正案のように失敗時はマップ更新対象から外す実装が必要です

Move newApps append to after DSL download and file write succeed,
preventing app_map.json inconsistency when errors occur.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@hayasaki-shunsuke
Copy link
Contributor Author

hayasaki-shunsuke commented Jan 20, 2026

確認ありがとうございます、修正しました!

🔴 重大な問題について

ご指摘の通り、DSL取得/書き込み失敗時にapp_map.jsonに不整合が生じる問題がありました。修正しました。

修正内容:

  • newApps = append(newApps, newApp) をDSL取得・ファイル書き込み成功後に移動
  • これにより、失敗したアプリはapp_map.jsonに追加されなくなります

f8807a4

🟡 軽微な改善提案について

Total apps統計の意味が不明確

こちらは別PRで対応を検討します。

テストカバレッジの追加(エラーケース)

以下のエラーケーステストを追加しました:

  • TestSyncAllWithNewAppsDSLFetchFails: 新規アプリのDSL取得が失敗するケース
  • TestSyncAllWithNewAppsPartialFailure: 複数の新規アプリで一部が失敗するケース

d59e0a7

✅ 質問への回答

新規DSL取得に失敗した場合でもアプリマップに残す意図はありますか?

意図はありません。今回の修正で、失敗時はマップ更新対象から外すようにしました。

- TestSyncAllWithNewAppsDSLFetchFails: DSL fetch failure case
- TestSyncAllWithNewAppsPartialFailure: Multiple new apps with partial failure

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
hayasaki-shunsuke and others added 2 commits January 21, 2026 10:53
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Copy link

@yoshikouki yoshikouki left a comment

Choose a reason for hiding this comment

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

レビュー結果

sync時に新規アプリを自動追加する機能、よく実装されています!

✅ 良い点

  • 堅牢なエラーハンドリング: DSL取得失敗時に app_map.json の整合性を保護
  • テストが充実: 正常系・異常系・部分的失敗のテストを網羅
  • リファクタリング: generateUniqueFilename() の共通化でコード重複を削減
  • PR説明が明確: 背景・課題・期待動作が分かりやすい

📝 軽微な指摘

インラインコメントで4点指摘しました。いずれも軽微なもので、マージ後のフォローアップで対応可能です。

  1. ページネーションの無限ループリスク (Medium)
  2. mapの走査順序が非決定的 (Low)
  3. DryRunモードでの新規アプリ検出 (Low)
  4. Debug出力の制御 (Nit)

LGTM 👍

page := 1

fmt.Printf("Debug - Using app list URL: %s\n", url)
for {

Choose a reason for hiding this comment

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

[Medium] ページネーションの無限ループリスク

APIが常に has_more: true を返すバグがある場合、無限ループになる可能性があります。

// 提案: 最大ページ数の制限を追加
const maxPages = 1000
for page := 1; page <= maxPages; page++ {
    // ...
    if !hasMore || page >= maxPages {
        break
    }
}

現時点では軽微なリスクですが、将来的に対応を検討することを推奨します。

}

newApps := []AppMapping{}
for appID, appInfo := range remoteApps {

Choose a reason for hiding this comment

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

[Low] mapの走査順序が非決定的

Goのmapは走査順序が保証されないため、新規アプリの追加順序が実行ごとに変わる可能性があります。app_map.json の差分が毎回変わったり、テストの安定性に影響する可能性があります。

// 提案: ソートしてから処理
var newAppIDs []string
for appID := range remoteApps {
    if !existingAppIDs[appID] {
        newAppIDs = append(newAppIDs, appID)
    }
}
sort.Strings(newAppIDs)
for _, appID := range newAppIDs {
    appInfo := remoteApps[appID]
    // ...
}


// Download DSL for new app
localPath := filepath.Join(s.config.DSLDirectory, filename)
if !s.config.DryRun {

Choose a reason for hiding this comment

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

[Low] DryRunモードで新規アプリの検出が表示されない

現在の実装では、DryRunモードの場合、新規アプリの検出・追加処理がスキップされます。ユーザーが「何が追加されるか」を事前確認したい場合に不便です。

// 提案: DryRunでもログ出力と統計カウントは行う
if s.config.DryRun {
    fmt.Printf("Dry run: Would download DSL for new app %s to %s\n", appInfo.Name, localPath)
    newApps = append(newApps, newApp)
    stats.Added++
    stats.Total++
    continue
}

// generateUniqueFilename generates a unique filename for an app, avoiding duplicates
func (s *DefaultSyncer) generateUniqueFilename(appName string, usedFilenames map[string]bool) string {
safeName := s.sanitizeFilename(appName)
fmt.Printf("Debug - sanitizeFilename(%q) = %q\n", appName, safeName)

Choose a reason for hiding this comment

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

[Nit] Debug出力が常に出力される

Verbose フラグに関係なく常にDebug出力されます。本番環境でログが冗長になる可能性があります。

// 提案: Verboseフラグで制御
if s.config.Verbose {
    fmt.Printf("Debug - sanitizeFilename(%q) = %q\n", appName, safeName)
}

または、別途 Debug フラグを追加する方法もあります。

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.

4 participants