Skip to content

refactor: generate_title から app_name 生成を削除し session ID ベースに統一#84

Open
mats16 wants to merge 4 commits into
mainfrom
feat/dynamic-app-name
Open

refactor: generate_title から app_name 生成を削除し session ID ベースに統一#84
mats16 wants to merge 4 commits into
mainfrom
feat/dynamic-app-name

Conversation

@mats16
Copy link
Copy Markdown
Owner

@mats16 mats16 commented Mar 5, 2026

Summary

generate_title API から app_name(slugify したタイトル)を返す機能を削除し、アプリ名を常に app-{sessionSuffix} 形式(session ID から自動生成)に戻す。

Changes

  • GenerateTitleResponse から app_name フィールドを削除
  • TitleService.generateTitle() の戻り値を { title, appName }string に簡素化
  • slugifyTitle() 関数と関連定数を削除
  • フロントエンドの generateTitle()string | null に変更し、appName の参照を除去
  • session.service.ts の既存フォールバック app-${sessionId.getSuffix()} が常に使用される
  • ルートテストから slugifyTitle テストと app_name アサーションを削除

Test Plan

  • npm run type-check passes
  • npm run test passes (500/500)
  • npm run build succeeds

Checklist

  • npm run type-check passes
  • npm run lint passes
  • npm run build succeeds
  • Tested locally with npm run dev

mats16 added 2 commits March 5, 2026 14:43
…ame handling

- Updated the title generation logic to return both the generated title and a slugified app name.
- Introduced a new `slugifyTitle` function to convert titles into a Databricks Apps-compatible format, ensuring they meet specific criteria (lowercase, hyphens, max length).
- Modified the API response structure to include the app name alongside the title.
- Updated related tests to validate the new functionality and ensure correct behavior across the application.

These changes improve the user experience by providing a more consistent and user-friendly app name format derived from the session title.
- Refactored the title generation logic to return only the generated title, removing the app name from the response.
- Eliminated the `slugifyTitle` function and related tests, streamlining the title handling process.
- Updated API responses and service methods to reflect the changes, ensuring consistency across the application.
- Adjusted tests to validate the new title generation behavior without app name handling.

These changes enhance the clarity of the title generation process and reduce unnecessary complexity in the codebase.
@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

コードレビュー

PR の目的(generate_title からの app_name 生成削除、MCP ツールから CLI コマンドへの移行)は理解できますが、いくつか気になる点があります。


セキュリティ:サンドボックス無効化

最も懸念される変更です。createSession と sendMessageToSession の両方でサンドボックスが無効化されています(sandbox.enabled: true から false へ)。

これにより AI エージェントが制限なしに任意のコマンドを実行できる状態になります。

  • なぜ無効化が必要なのか、PR の説明に理由がありません
  • コメントアウトされたコードが残っているのは一時的な状態を示唆していますが、このまま本番にマージすると深刻なセキュリティリスクになります
  • 無効化の理由と、いつ再有効化するのかを説明してください

unknown フォールバックの問題

apps/api/src/utils/system-prompt.helper.ts で appsOutcome.name のフォールバックとして unknown を使用しています。

appsOutcome.name が未設定の場合、AI エージェントには databricks apps create unknown のような指示が渡り、リテラルに unknown という名前のアプリを作成しようとします。

提案

  • app-{suffix} のようなより意味のある値をフォールバックにする
  • または appsOutcome.name が未設定の場合は Apps instruction 自体を生成しないようにする

コードの可読性

title.ts のリネーム:変数名を title から result に変更していますが、result は汎用的すぎます。generatedTitle などの方が意図が明確です。

title.service.ts の簡素化:2行を1行にまとめていますが、機能的には同等で、元の2行の方が読みやすかったです。


良い点

  • バグ修正:session_context.outcomes から outcomes への変更は正しい修正です。名前が設定済みの outcomes を systemPrompt 構築に使用するようになりました
  • MCP から CLI への移行:テストも合わせて更新されており、一貫性があります
  • 型の簡素化:generateTitle() の戻り値をオブジェクトから string に変更したのはシンプルで良いです
  • テストが 500/500 通過しているのも確認済みとのこと

まとめ

項目 評価
セキュリティ(サンドボックス無効化) 要対応
unknown フォールバック 要検討
コメントアウトされたコード 要整理
型簡素化・バグ修正 良い
テスト更新 良い

サンドボックスの無効化について明確な説明と対応方針を示していただければ、他の点は軽微です。DRAFT のまま保留されているのも、ローカルテスト未完了を示していますね。準備ができたらまた確認します。

- Updated the sandbox configuration in the createSession and sendMessageToSession functions to enable sandboxing.
- Added options for auto-allowing bash commands and specifying allowed network domains, enhancing the flexibility and security of session operations.
- These changes improve the functionality of session handling by allowing more robust execution environments for user interactions.
@mats16 mats16 marked this pull request as ready for review March 5, 2026 15:57
@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

PR レビュー

全体的には、title 生成と app_name 生成の責務を明確に分離する良いリファクタリングです。CLI ベースの指示へ移行するアーキテクチャ判断も理解できます。いくつか気になる点を挙げます。


要確認:mcp__apps__ がデッドコードになる可能性

apps/web/src/components/main/MainArea.tsx にて mcp__apps__ 系が disallowed_tools に残っています。createDbAppsMcpServer を削除した結果、apps プレフィックスの MCP サーバーはもう存在しないはずです。この disallow 指定は実質的にデッドコードになっていないか確認が必要です。system prompt 側では mcp__dbapps__ 系という別プレフィックスが使われていたので、意図的に別物として扱っていたなら問題ないですが、将来の混乱を招く可能性があります。


sendMessageToSession での unknown フォールバック

apps/api/src/utils/system-prompt.helper.ts で appsOutcome.name ?? 'unknown' というフォールバックが使われています。createSession 側では outcomes に必ず name を設定するロジックがあるため、DB から読んだ outcomes には通常 name が設定されています。ただし理論上 name が未設定のまま unknown になる経路が残ります。Claude へのプロンプトで unknown というアプリ名になると不適切です。app-unknown のような形式や明示的なエラー処理の方が望ましいかもしれません。


apps/api/src/routes/title.ts の変数名

generateTitle の戻り値を result にリネームしていますが、title の方が意図が明確でした。return reply.send({ title: result }) と書くより、const title = await ... として return reply.send({ title }) の方がシンプルです。


title.service.ts の可読性

return (rawTitle ? cleanTitle(rawTitle) : FALLBACK_TITLE) || FALLBACK_TITLE; は機能的には元の2行と等価ですが、cleanTitle が空文字を返す可能性があるため二重チェックが必要という意図が、元の2行の方が伝わりやすかったです。


良い点

  • slugifyTitle() を削除してシンプルにした判断は適切です。title 生成と app 名生成は別の関心事なので、この分離は筋が良い。
  • DATABRICKS_APP_NAME を環境変数として渡すことで、CLI コマンドで環境変数を直接参照できるようになり、プロンプトの実用性が上がっています。
  • テストが新しい動作に合わせて適切に更新されており、mcp__dbapps__ 参照の削除も確認されています。
  • outcomes の name 設定ロジック(createSession 内)が outcome.name || fallback になっており、既存の name を尊重する形になっているのは適切です。

補足:3コミット目のサンドボックス変更について

コミットメッセージに Enable sandbox features for session creation and messaging とありますが、今回の diff 上でその変更内容が明示的に確認できませんでした。サンドボックスの有効化はセキュリティ上重要な変更なので、意図した変更が正しく含まれているか確認をお勧めします。

- Updated the title generation logic to directly return the generated title variable, improving code clarity.
- Refactored the title service to ensure consistent fallback handling for title generation, enhancing robustness.
- These changes streamline the title generation process and improve maintainability of the codebase.
@claude
Copy link
Copy Markdown

claude Bot commented Mar 5, 2026

コードレビュー

全体的に、MCP サーバー依存を排除して CLI ベースの操作に統一するというリファクタリングの方向性は明確で、コードが簡潔になっています。いくつか気になった点を挙げます。


✅ 良い点

buildSystemPromptConfig(outcomes) への修正
session.service.ts の以下の変更は、実質的なバグ修正です:

- const systemPromptConfig = buildSystemPromptConfig(session_context.outcomes);
+ const systemPromptConfig = buildSystemPromptConfig(outcomes);

session_context.outcomesname がセットされる前の古い状態を参照していた可能性があり、outcomes(name セット済み)を使うことで DATABRICKS_APP_NAME が正しくシステムプロンプトに反映されます。重要な修正です。

フォールバックの保持

- name: `app-${sessionId.getSuffix()}`,
+ name: outcome.name || `app-${sessionId.getSuffix()}`,

既存の name を優先しつつフォールバックも維持しており、後方互換性が保たれています。


⚠️ 懸念点

1. 'unknown' フォールバックの扱い (system-prompt.helper.ts)

append = createDatabricksAppsInstruction(workspacePath, appsOutcome.name ?? 'unknown');

appsOutcome.nameundefined の場合、Claude に渡されるプロンプトに DATABRICKS_APP_NAME = unknown が含まれてしまいます。createSession では必ず name がセットされますが、既存セッションのデータ(DBに name が保存されていないケース)では undefined になる可能性があります。'unknown' という文字列より、appsOutcome.name が必須の状態を保証するか、または undefined の場合は Apps instruction 自体を生成しないほうが安全かもしれません。

2. createSession での型キャストの一貫性

createSession では as DatabricksAppsOutcome | undefined という型キャストが使われています:

DATABRICKS_APP_NAME: (
  outcomes.find(o => o.type === 'databricks_apps') as DatabricksAppsOutcome | undefined
)?.name,

一方 sendMessageToSession では型ガード関数が使われています:

const appName = sessionContext.outcomes?.find(
  (o): o is DatabricksAppsOutcome => o.type === 'databricks_apps'
)?.name;

型ガードを使う方がより型安全です。createSession 側も型ガード方式に統一することを検討してください。

3. MainArea.tsx の変更が実質的に空

- outcomes.push({ type: 'databricks_apps' });
+ outcomes.push({
+   type: 'databricks_apps',
+ });

フォーマット変更のみで機能的な差がありません。PR の説明にある「appName の参照を除去」については、generateTitle() の戻り値型が変わった({ title, appName }string)ことへの対応だと思いますが、その変更自体はすでに titleResult へのリネームで対応済みです。このフォーマット変更は不要な差分になっています。

4. list_deployments 機能の喪失

MCP サーバー削除に伴い mcp__dbapps__list_deployments ツールが使えなくなり、CLI 参照テーブルからも対応するコマンドが消えています。databricks apps list-deployments $DATABRICKS_APP_NAME 相当の CLI コマンドをリファレンスに追加することで、デプロイ履歴確認の手段を残せます。


💡 提案

title.service.ts の変更は単純なリネームで問題ありませんが、return title || FALLBACK_TITLEreturn title にして cleanTitle() の戻り値が falsy になる可能性はないか確認してみてください(空文字列が返りうる実装であれば現状維持が正しい)。


全体的にリファクタリングの意図は明確で、テストも適切に更新されています。上記の型キャストの一貫性と 'unknown' フォールバックの扱いを改善できると、より堅牢なコードになります。

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.

1 participant