Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPRはCIにStrykerミューテーションテストを追加し、Vitest/Stryker設定・スクリプト・.gitignoreを導入。大量のユニットテスト追加・拡張と限定的な本番修正（volume名のtrim、sourceIPのURLエンコード、timeout型注釈、isValidEmail強化）を含みます。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant PR as "GitHub PR"
participant Actions as "GitHub Actions"
participant Checkout as "actions/checkout"
participant Repo as "Repository"
participant Stryker as "Stryker"
participant Vitest as "Vitest"
participant Summary as "Workflow summary / ci-check"
PR->>Actions: pull_request イベント
Actions->>Checkout: checkout (fetch-depth: 0)
Actions->>Repo: 変更された TS ファイルを収集 (src/services, src/lib)
alt has targets
Actions->>Stryker: Stryker 実行 (mutate targets)
Stryker->>Vitest: vitest (vitest.config.stryker.ts) を呼び出し
Vitest-->>Stryker: テスト結果を返す
Stryker-->>Summary: レポートを出力 (break=85)
Summary->>Actions: mutation-test 成否を報告
else no targets
Actions->>Summary: mutation-test をスキップ
end
Summary->>Actions: ci-check が mutation-test 結果を考慮
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 分 Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
Stryker Mutator（Vitest runner）を導入してミューテーションテストをCIに組み込みつつ、ネットワークフィルタ（proxy）連携やパス安全性など周辺のテスタビリティ/堅牢性改善を同時に進めるPRです。
Changes:
- Stryker/Vitest向けのミューテーションテスト設定を追加し、GitHub ActionsでPR差分ファイルに対して実行（品質ゲート導入）
- network-filter-proxy のManagement API向け
ProxyClientと、ルール同期ヘルパー（proxy-sync）を追加し、Dockerセッション作成/終了時の同期・クリーンアップを実装 path-safety追加やremote-repo-serviceのパス/プロセス実行まわりを強化、関連テストを大幅拡充
Reviewed changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | Stryker実行用のVitest設定を追加 |
| stryker.config.json | Strykerのmutate対象/レポータ/閾値などを定義 |
| src/services/remote-repo-service.ts | clone/pullの安全性向上（パス検証、timeout、オプション解釈防止等） |
| src/services/proxy-client.ts | proxy Management API用HTTPクライアントを新規追加 |
| src/services/environment-adapter.ts | CreateSessionOptionsにfilterEnabledを追加 |
| src/services/adapters/docker-adapter.ts | filter有効時のNetworkMode/proxy env設定、proxyルール同期/クリーンアップ追加 |
| src/services/adapters/tests/host-adapter.test.ts | HostAdapterテスト拡充（shell/claude両モードの挙動） |
| src/services/adapters/tests/docker-adapter.test.ts | DockerAdapterのnetwork filtering関連テスト追加 |
| src/services/adapters/tests/base-adapter.test.ts | BasePTYAdapterの追加テスト（session抽出、spawnPTYのredactなど） |
| src/services/adapter-factory.ts | 環境IDからキャッシュ済みDockerAdapter取得APIを追加 |
| src/services/tests/scrollback-buffer.test.ts | ScrollbackBufferの境界/マルチバイト/compact等テスト拡充 |
| src/services/tests/run-script-manager.test.ts | stop/kill・spawn error・exit cleanup等のテスト拡充 |
| src/services/tests/pty-manager.test.ts | PTYManagerの検証範囲を拡大（env/allowed dirs等） |
| src/services/tests/port-checker.test.ts | config異常系/優先順位などのテスト拡充 |
| src/services/tests/github-pat-service.test.ts | insert/update payloadや暗号化失敗の詳細テスト追加 |
| src/services/tests/git-operations.test.ts | GitOperationErrorのテストを新規追加 |
| src/services/tests/gh-cli.test.ts | gh-cliのユニットテストを大幅拡充（execFileSyncモック等） |
| src/services/tests/environment-adapter.test.ts | isEnvironmentAdapterの追加ケース（欠損/型違い等） |
| src/services/tests/encryption-service.test.ts | Error classや鍵長/形式エラーのテスト拡充 |
| src/services/tests/docker-client.test.ts | DockerClient APIとログ/例外系のテスト拡充 |
| src/services/tests/developer-settings-service.test.ts | SettingsNotFoundError等のテスト追加 |
| src/services/tests/config-service.test.ts | singleton/ログ/merge/save等のテスト拡充 |
| src/services/tests/adapter-factory.test.ts | ログ検証、getDockerAdapterForEnvironment等のテスト追加 |
| src/lib/websocket/tests/session-ws.test.ts | Session WS handlerのイベント転送/接続処理テストを大幅拡充 |
| src/lib/volume-naming.ts | generateSlugのReDoS対策リファクタ |
| src/lib/validation.ts | isValidEmailを追加 |
| src/lib/proxy-sync.ts | DB→proxy形式への変換と、必要時の再同期ヘルパーを追加 |
| src/lib/path-safety.ts | sanitize/within-base/safe-componentのユーティリティを新規追加 |
| src/lib/tests/validation.test.ts | isValidEmail等のテスト追加 |
| src/lib/tests/session.test.ts | session moduleの挙動テストを新規追加 |
| src/lib/tests/session-name-generator.test.ts | 乱数/リトライ境界のテスト追加 |
| src/lib/tests/schema-check.test.ts | スキーマ整合性/表示の追加テスト |
| src/lib/tests/run-scripts.test.ts | run-scriptsのparse/serializeテストを新規追加 |
| src/lib/tests/proxy-sync.test.ts | proxy-syncの同期条件/例外系テストを新規追加 |
| src/lib/tests/path-safety.test.ts | path-safetyのテストを新規追加 |
| src/lib/tests/notification-service.test.ts | 設定/保存失敗/OS通知などのテスト拡充 |
| src/lib/tests/errors.test.ts | handleApiError/createErrorResponseのログ・分岐テスト追加 |
| src/lib/tests/env-validation.test.ts | env validationのエラーメッセージ/which分岐などのテスト拡充 |
| src/lib/tests/docker-volume-utils.test.ts | docker-volume-utilsのテストを新規追加 |
| src/lib/tests/docker-config-validator.test.ts | port/volume validatorの境界・メッセージ検証追加 |
| src/lib/tests/db-module.test.ts | dbモジュールのモジュールレベル挙動テスト追加 |
| src/lib/tests/action-detector.test.ts | stripAnsi/detectActionRequest/cooldown境界のテスト追加 |
| src/app/api/environments/tests/route.test.ts | 環境作成時のネットワークフィルタテンプレ適用のテスト追加 |
| src/app/api/environments/[id]/network-rules/tests/route.test.ts | ルールCRUD後のproxy同期呼び出しテスト追加 |
| src/app/api/environments/[id]/network-filter/tests/route.test.ts | filter設定/テンプレ適用後のproxy同期呼び出しテスト追加 |
| src/app/api/docker/image-build/tests/route.test.ts | バリデーションエラーメッセージ変更に追従 |
| package.json | Stryker依存追加、test:mutationスクリプト追加 |
| .gitignore | Stryker出力（reports/.stryker-tmp）を追加除外 |
| .github/workflows/test.yml | mutation-testジョブ追加、ci-checkに統合 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/test.yml
Outdated
Comment on lines
+262
to
+268
| MUTATE_ARGS="" | ||
| for f in "${FILES[@]}"; do | ||
| MUTATE_ARGS="$MUTATE_ARGS \"$f\"" | ||
| done | ||
|
|
||
| pnpm exec stryker run \ | ||
| --mutate $MUTATE_ARGS \ |
Comment on lines
+144
to
+146
| async setRules(sourceIP: string, entries: ProxyRuleEntry[]): Promise<ProxyRuleSet> { | ||
| const url = `${this.baseUrl}/api/v1/rules/${sourceIP}`; | ||
| const body = JSON.stringify({ entries }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/__tests__/pty-manager.test.ts (1)
31-37:⚠️ Potential issue | 🟠 Majorユニットテストで実ファイルシステムを直接操作しています
Line 31–37 と Line 73–77 で実 FS を使っており、テストの再現性と実行安定性が環境依存になりやすいです。
fsをモック化し、必要な戻り値/例外だけを制御する形に寄せてください。As per coding guidelines, "Mock child_process, fs operations, and ws library in unit tests. Use EventEmitter for testing WebSocket behavior."
Also applies to: 73-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/pty-manager.test.ts` around lines 31 - 37, The tests currently call real filesystem APIs in beforeAll/afterAll (mkdtempSync, rmSync, tmpdir, join) and similarly at the second occurrence (lines ~73-77); replace those direct FS calls by mocking the fs module (e.g., jest.mock('fs') or jest.spyOn) and stub mkdtempSync and rmSync to return controlled values and no-op behavior, or provide a temporary in-memory path string; ensure teardown uses the mocked rmSync behavior; update the tests in pty-manager.test.ts to use the mocked implementations for mkdtempSync and rmSync (and any tmpdir/join interactions) so the tests do not touch the real filesystem and remain deterministic.
🧹 Nitpick comments (18)
src/services/__tests__/developer-settings-service.test.ts (1)
275-284: 非効率なエラーアサーション: 同じメソッドを2回呼び出しているエラーの型とメッセージを検証するために
deleteProjectSettingsを2回呼び出しています。エラーを一度キャッチして複数のアサーションを行う方が効率的で保守性が高いです。♻️ エラーを一度キャッチして検証する方法
it('存在しないプロジェクト設定を削除しようとした場合、SettingsNotFoundErrorをスローする', async () => { mockDbSelectGet.mockReturnValue(undefined); - await expect( - service.deleteProjectSettings('non-existent-project') - ).rejects.toThrow(SettingsNotFoundError); - await expect( - service.deleteProjectSettings('non-existent-project') - ).rejects.toThrow(/Settings not found: project non-existent-project/); + const error = await service.deleteProjectSettings('non-existent-project') + .catch((e) => e); + + expect(error).toBeInstanceOf(SettingsNotFoundError); + expect(error.message).toBe('Settings not found: project non-existent-project'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/developer-settings-service.test.ts` around lines 275 - 284, The test calls service.deleteProjectSettings twice to assert both the error type and message; instead, call it once, capture the rejected error (e.g., using await expect(...).rejects.toThrow or try/catch) and then make two assertions against that single error object: verify it is an instance of SettingsNotFoundError and that its message matches /Settings not found: project non-existent-project/; update the test around mockDbSelectGet and service.deleteProjectSettings to perform one invocation and assert both the class and message on the same caught error.src/services/adapters/__tests__/base-adapter.test.ts (1)
341-352:process.envの直接変更はテスト分離を損なうリスクがありますアサーションが失敗した場合、
process.env = originalEnvが実行されず、後続のテストに影響を与える可能性があります。try/finallyまたはvi.stubEnvを使用することを推奨します。♻️ 提案する修正
it('should exclude CLAUDECODE from environment', () => { const originalEnv = process.env; - process.env = { ...originalEnv, CLAUDECODE: 'should-be-excluded' }; - - adapter.testSpawnPTY('claude', [], {}); - - const spawnCall = vi.mocked(pty.spawn).mock.calls[0]; - const envArg = spawnCall[2].env; - expect(envArg).not.toHaveProperty('CLAUDECODE'); - - process.env = originalEnv; + try { + process.env = { ...originalEnv, CLAUDECODE: 'should-be-excluded' }; + + adapter.testSpawnPTY('claude', [], {}); + + const spawnCall = vi.mocked(pty.spawn).mock.calls[0]; + const envArg = spawnCall[2].env; + expect(envArg).not.toHaveProperty('CLAUDECODE'); + } finally { + process.env = originalEnv; + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/base-adapter.test.ts` around lines 341 - 352, The test currently mutates process.env directly before calling adapter.testSpawnPTY('claude', ...) which can leak state if the assertion fails; replace the manual assignment with vi.stubEnv({ CLAUDECODE: 'should-be-excluded' }) to automatically restore environment after the test, or wrap the mutation in a try/finally that sets process.env = originalEnv in finally to guarantee restoration; update the assertion code that reads pty.spawn mock (vi.mocked(pty.spawn).mock.calls[0]) unchanged.src/lib/__tests__/docker-config-validator.test.ts (1)
161-164:errors[0]前提の検証は順序変更に弱いので、条件一致ベースに寄せると安定します。将来バリデーション順が変わるとテストが不要に壊れるため、
some/findでメッセージ要件を直接確認する形のほうが保守しやすいです。差分例
- expect(result.errors[0]).toContain('整数'); - // マッピング番号が正しい - expect(result.errors[0]).toContain('マッピング1'); + expect( + result.errors.some((e) => e.includes('整数') && e.includes('マッピング1')) + ).toBe(true); - expect(result.errors[0]).toContain('マッピング1'); + expect(result.errors.some((e) => e.includes('マッピング1'))).toBe(true); - expect(result.errors[0]).toContain('マウント1'); + expect(result.errors.some((e) => e.includes('マウント1'))).toBe(true);Also applies to: 204-205, 326-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/docker-config-validator.test.ts` around lines 161 - 164, The tests currently assert on a specific position (expect(result.errors[0])...) which is brittle; update these assertions to check presence using condition matching (e.g. use result.errors.some or result.errors.find) instead of indexing so the order doesn't matter — replace the two fragile expects that reference result.errors[0] (checking for '整数' and 'マッピング1') with existence checks that verify any entry contains those substrings, and make the same change for the other occurrences noted (the similar assertions around the other two failure cases).src/app/api/environments/__tests__/route.test.ts (1)
598-658: テストカバレッジが充実しています。DOCKER環境作成時のフィルタリング自動適用テストは、呼び出し順序の検証（
invocationCallOrder）も含めて適切に実装されています。軽微な改善提案: Line 643-648の
expect.arrayContainingは1つのルールのみ検証しています。defaultTemplatesには3つのルール（Anthropic API: 1, npm: 2）が定義されているため、より厳密な検証として全ルールの存在確認を追加することも検討できます。,
♻️ より厳密なルール検証の例（オプション）
expect(mockApplyTemplates).toHaveBeenCalledWith( 'env-docker-filter', - expect.arrayContaining([ - expect.objectContaining({ target: 'api.anthropic.com', port: 443 }), - ]) + expect.arrayContaining([ + expect.objectContaining({ target: 'api.anthropic.com', port: 443 }), + expect.objectContaining({ target: '*.npmjs.org', port: 443 }), + expect.objectContaining({ target: '*.npmjs.com', port: 443 }), + ]) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/environments/__tests__/route.test.ts` around lines 598 - 658, Test currently asserts only one rule via expect.arrayContaining for mockApplyTemplates even though defaultTemplates contains three rules; update the assertion in the 'DOCKER環境作成時にフィルタリングが有効化されテンプレートが適用される' test to verify all expected rules are passed to applyTemplates by replacing the single expect.objectContaining check with a more complete expectation that includes the three rule objects (e.g., assert array contains objects for 'api.anthropic.com' port 443 and '*.npmjs.org'/'*.npmjs.com' port 443) when calling mockApplyTemplates; keep the existing invocation order and call count assertions for mockGetDefaultTemplates, mockCreateConfigVolumes, mockApplyTemplates, and mockUpdateFilterConfig.src/lib/websocket/__tests__/session-ws.test.ts (1)
100-168: 未使用変数hについて確認してください。変数
hはコンストラクタの副作用（リスナー登録）のためだけに作成されており、直接使用されていません。意図的なパターンですが、リンターの警告を避けるためにvoid h;を追加するか、アンダースコアプレフィックス（_h）を使用することを検討してください。♻️ リンター警告を回避する修正案
it('should forward output events as run_script_log', () => { // Re-create handler to attach listeners const cm = createMockConnectionManager(); - const h = new SessionWebSocketHandler(cm); + const _h = new SessionWebSocketHandler(cm); mockRunScriptManager.emit('output', {または:
it('should forward output events as run_script_log', () => { // Re-create handler to attach listeners const cm = createMockConnectionManager(); - const h = new SessionWebSocketHandler(cm); + new SessionWebSocketHandler(cm); mockRunScriptManager.emit('output', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/session-ws.test.ts` around lines 100 - 168, The test creates a SessionWebSocketHandler instance as const h = new SessionWebSocketHandler(cm) solely for its side effects but leaves h unused, which triggers a linter warning; fix by either marking the variable as intentionally unused (e.g., add a void h; after instantiation) or rename it with an underscore prefix (e.g., const _h = new SessionWebSocketHandler(cm)) in each test that currently declares h, so the side-effect construction remains but the linter stop flag is addressed.src/lib/websocket/__tests__/terminal-ws.test.ts (1)
103-107:flush()の実装に関する懸念点。
setImmediateを2回呼び出す理由についてコメントがありますが、この数が十分かどうかは環境依存の可能性があります。Lines 308, 334, 420 ではfor (let i = 0; i < 5; i++) await flush();と5回ループしており、一貫性がありません。♻️ flush 回数を定数化する提案
async function flush(): Promise<void> { // Drain microtasks and macrotasks await new Promise(resolve => setImmediate(resolve)); await new Promise(resolve => setImmediate(resolve)); } + +async function flushAll(): Promise<void> { + // Multiple flushes for complex async operations + for (let i = 0; i < 5; i++) await flush(); +}その後、テスト内で
flushAll()を統一的に使用することで、一貫性が向上します。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 103 - 107, The current flush() awaits setImmediate twice which is arbitrary and tests inconsistently call flush multiple times; introduce a single shared constant (e.g., FLUSH_IMMEDIATE_COUNT) and refactor flush() to loop awaiting setImmediate that many times (or add a helper flushAll() that does this), then replace the ad-hoc loops like "for (let i = 0; i < 5; i++) await flush();" with a single call to the unified helper so tests and the flush implementation (symbols: flush(), flushAll(), FLUSH_IMMEDIATE_COUNT) use the same, clearly-configured number of micro/macrotask drains.src/lib/volume-naming.ts (1)
37-44: 先頭/末尾ハイフン除去が最悪 O(n^2) になります。
slice()をループで1文字ずつ繰り返すため、長い入力で不要な文字列コピーが増えます。開始/終了インデックスを進めて最後に1回だけsliceすると、同じ安全性のまま線形にできます。差分案
- // 先頭・末尾のハイフンを除去（個別に処理してReDoSを回避） - let result = deduped; - while (result.startsWith('-')) { - result = result.slice(1); - } - while (result.endsWith('-')) { - result = result.slice(0, -1); - } - return result; + // 先頭・末尾のハイフンを除去（線形走査 + 1回のslice） + let start = 0; + let end = deduped.length; + while (start < end && deduped[start] === '-') start++; + while (end > start && deduped[end - 1] === '-') end--; + return deduped.slice(start, end);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/volume-naming.ts` around lines 37 - 44, The current loop removes leading/trailing '-' from variable deduped by repeatedly slicing into result, which causes O(n^2) copies; instead compute start and end indices once by advancing a startIndex while deduped[startIndex] === '-' and decrementing an endIndex while deduped[endIndex - 1] === '-' (preserving the same per-character checks used now to avoid regex), then set result = deduped.slice(startIndex, endIndex) in one operation. Update the block that currently uses result.startsWith / result.endsWith and result = result.slice(...) to use startIndex/endIndex and a single final slice to maintain behavior and performance.src/lib/__tests__/notification-service.test.ts (1)
254-277: モック復元はfinallyで保護したほうが安全です。Line 275 の手動復元は、途中アサーション失敗時に実行されず、後続テストへ状態が漏れる可能性があります。
try/finallyかvi.spyOn(...).mockImplementation(...)で自動復元に寄せるのを推奨します。差分案
it('localStorageへの保存が失敗してもエラーを投げない', () => { const originalSetItem = mockLocalStorage.setItem; const consoleSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - // setItemが例外をスローするように一時的に変更 - (mockLocalStorage as any).setItem = () => { throw new Error('QuotaExceeded'); }; - - expect(() => { - saveSettings({ - onTaskComplete: true, - onPermissionRequest: true, - onError: true, - onActionRequired: true, - }); - }).not.toThrow(); - - expect(consoleSpy).toHaveBeenCalledWith( - expect.stringContaining('Failed to save'), - expect.any(Error) - ); - - // 元に戻す - (mockLocalStorage as any).setItem = originalSetItem; - consoleSpy.mockRestore(); + try { + // setItemが例外をスローするように一時的に変更 + (mockLocalStorage as any).setItem = () => { throw new Error('QuotaExceeded'); }; + + expect(() => { + saveSettings({ + onTaskComplete: true, + onPermissionRequest: true, + onError: true, + onActionRequired: true, + }); + }).not.toThrow(); + + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('Failed to save'), + expect.any(Error) + ); + } finally { + (mockLocalStorage as any).setItem = originalSetItem; + consoleSpy.mockRestore(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/notification-service.test.ts` around lines 254 - 277, The test manually restores mockLocalStorage.setItem and consoleSpy outside of a finally block which can leak state on assertion failures; wrap the temporary override of (mockLocalStorage as any).setItem and the console spy teardown in a try/finally (or use vi.spyOn(...).mockImplementation and rely on mockRestore in finally) so that after calling saveSettings(...) (the test invoking saveSettings) you always restore originalSetItem and call consoleSpy.mockRestore() in the finally block to guarantee cleanup even if assertions fail.src/lib/__tests__/env-validation.test.ts (1)
59-78: テスト構造の改善提案: 関数を2回呼び出す冗長なパターン現在のテストは
toThrow()で例外発生を確認した後、同じ関数を再度呼び出して try/catch でメッセージを検証しています。より簡潔なパターンとして、エラーを1回キャプチャして全てのアサーションを行う方法があります。♻️ 提案: エラーを1回キャプチャするパターン
it('DATABASE_URLが未設定の場合、変数名とセットアップ手順を含むエラーメッセージをスローする', () => { delete process.env.DATABASE_URL; - expect(() => { - validateRequiredEnvVars(); - }).toThrow(/DATABASE_URL/); - - try { - validateRequiredEnvVars(); - } catch (e: unknown) { - const msg = (e as Error).message; - // エラーメッセージに変数名が含まれる - expect(msg).toContain('DATABASE_URL'); - // セットアップ手順が含まれる - expect(msg).toContain('Please follow these steps'); - expect(msg).toContain('cp .env.example .env'); - expect(msg).toContain('Database connection URL'); - expect(msg).toContain('README.md'); - } + let error: Error | null = null; + try { + validateRequiredEnvVars(); + } catch (e: unknown) { + error = e as Error; + } + expect(error).not.toBeNull(); + const msg = error!.message; + // エラーメッセージに変数名が含まれる + expect(msg).toContain('DATABASE_URL'); + // セットアップ手順が含まれる + expect(msg).toContain('Please follow these steps'); + expect(msg).toContain('cp .env.example .env'); + expect(msg).toContain('Database connection URL'); + expect(msg).toContain('README.md'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/env-validation.test.ts` around lines 59 - 78, The test calls validateRequiredEnvVars() twice; change it to call it only once and capture the thrown error for all assertions: remove the first expect(() => validateRequiredEnvVars()).toThrow(/DATABASE_URL/) call, invoke validateRequiredEnvVars() inside a single try/catch (or use expect(() => validateRequiredEnvVars()).toThrow() combined with capturing the error via a thrownError variable), assign the caught error to a variable (as Error), then perform all message assertions (contains 'DATABASE_URL', 'Please follow these steps', 'cp .env.example .env', 'Database connection URL', 'README.md') against that single captured error; reference validateRequiredEnvVars in your change.src/lib/__tests__/run-scripts.test.ts (1)
18-25:RunScript要素の形状不正ケースも追加したいです。現在は「配列であること」中心なので、
[{ name: 1 }]のような要素不正が混入した場合の期待挙動をテストで固定しておくと、将来の回帰検知が強くなります。テスト追加例
it('should return empty array for JSON number', () => { expect(parseRunScripts(JSON.stringify(42))).toEqual([]); }); + + it('should return empty array for invalid RunScript item shape', () => { + const invalid = [{ name: 123 }, { command: 'npm test' }]; + // 期待仕様に合わせて [] などへ固定 + expect(parseRunScripts(JSON.stringify(invalid))).toEqual([]); + });Also applies to: 43-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/run-scripts.test.ts` around lines 18 - 25, Add a unit test that exercises element-shape validation for parseRunScripts: supply JSON.stringify([{ name: 1 }]) (and similarly for [{ command: 123 }]) and assert the function returns the expected safe result (e.g., an empty array or only-valid entries) so the behavior is pinned; locate tests around parseRunScripts and RunScript in this test file and add two cases (invalid name type and invalid command type) mirroring the existing "should parse valid JSON array" test and the other test at lines 43-46.src/app/api/environments/[id]/network-rules/__tests__/route.test.ts (1)
166-180: 同期処理の呼び出し回数も固定しておくと回帰に強くなります。現状は「呼ばれたこと」のみなので、
toHaveBeenCalledTimes(1)を追加して多重同期の回帰も検知できるようにするのがおすすめです。追記例
await POST(request, { params: Promise.resolve({ id: 'env-uuid' }) }); expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledWith('env-uuid'); + expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledTimes(1);await PUT(request, { params: Promise.resolve({ id: 'env-uuid', ruleId: 'rule-uuid' }), }); expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledWith('env-uuid'); + expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledTimes(1);await DELETE(request, { params: Promise.resolve({ id: 'env-uuid', ruleId: 'rule-uuid' }), }); expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledWith('env-uuid'); + expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledTimes(1);Also applies to: 231-248, 285-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/api/environments/`[id]/network-rules/__tests__/route.test.ts around lines 166 - 180, The tests currently assert only that mockSyncProxyRulesIfNeeded was called; update each relevant test (the one calling POST/PUT/DELETE and asserting mockSyncProxyRulesIfNeeded with toHaveBeenCalledWith) to also assert the exact call count by adding expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledTimes(1) immediately after the toHaveBeenCalledWith assertion so you detect accidental duplicate invocations; apply this change for all test cases that reference mockSyncProxyRulesIfNeeded (e.g., the tests invoking POST via POST(request, { params: ... }), and the other tests that assert mockSyncProxyRulesIfNeeded) to make the assertions stricter.src/lib/__tests__/session.test.ts (1)
44-98:warnSpyのクリーンアップを早期に行うとテスト信頼性が向上します。各テストで
warnSpy.mockRestore()がモジュールインポート直後に呼ばれていますが、アサーション前にrestoreすると、テスト失敗時に他のテストへの影響が残る可能性があります。afterEachでのクリーンアップまたはtry-finallyパターンを検討してください。ただし、現状でも
vi.resetModules()により各テストが独立しているため、致命的な問題ではありません。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/session.test.ts` around lines 44 - 98, The tests create a console.warn spy (warnSpy) and restore it immediately after importing the session module, which can leave teardown inconsistent on failures; change each test to ensure warnSpy is always restored by either moving the restore into a shared afterEach cleanup (declare warnSpy in the test scope) or wrap the import/assertion block in a try/finally that calls warnSpy.mockRestore(), referencing the existing warnSpy, the dynamic import('../session') and assertions against mod.sessionOptions.cookieOptions so the spy is reliably cleaned up even if an assertion throws.src/services/__tests__/config-service.test.ts (1)
236-248: 未使用変数origReadFileがあります。236行目で
origReadFileを宣言していますが、使用されていません。モックのリストア時に必要ない場合は削除してください。🧹 修正案
- const origReadFile = fs.readFile; vi.spyOn(fs, 'readFile').mockRejectedValueOnce(Object.assign(new Error('Permission denied'), { code: 'EACCES' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 236 - 248, Remove the unused variable origReadFile declared on line where const origReadFile = fs.readFile; since the test restores the spy via vi.mocked(fs.readFile).mockRestore() there is no need to keep the original reference; simply delete the origReadFile declaration so the test only mocks and restores fs.readFile (related symbols: origReadFile, fs.readFile, vi.spyOn, vi.mocked(...).mockRestore, configService.load, configService.getConfig, mockLogger.error).src/services/__tests__/docker-pty-adapter.test.ts (1)
154-176:cwdパラメータの検証にexpect.objectContaining()を使用することを推奨します。コーディングガイドラインに基づき、
createOptionsの検証でより明示的なcwdの検証を行うことを検討してください。- expect(createOptions.WorkingDir).toBe('/workspace'); + expect(createOptions).toEqual(expect.objectContaining({ + WorkingDir: '/workspace', + }));Based on learnings: "Validate cwd parameter in spawnSync/execSync tests using expect.objectContaining() to ensure child process working directory is explicitly verified"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-pty-adapter.test.ts` around lines 154 - 176, The test should explicitly validate the container's working directory using expect.objectContaining to assert the cwd/WorkingDir instead of a bare equality; update the assertion around mockDockerClient.createContainer (referencing createSession -> mockDockerClient.createContainer and the local createOptions) to use expect(createOptions).toEqual(expect.objectContaining({ WorkingDir: '/workspace' })) or assert mockDockerClient.createContainer was called with expect.objectContaining({ WorkingDir: '/workspace' })) so the cwd is explicitly verified.src/lib/proxy-sync.ts (1)
20-30: 同じルール取得をコンテナ数ぶん繰り返しています。
syncProxyRulesIfNeeded()からsyncRulesForContainer()を呼ぶたびに同じenvironmentIdのルールを再読込しているので、アクティブコンテナ数に比例して DB クエリと変換処理が増えます。1 回の再同期では同一スナップショットを全コンテナへ配りたい処理なので、entries の生成はループ前に 1 回だけに切り出す方が安定です。Also applies to: 72-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/proxy-sync.ts` around lines 20 - 30, 現在の実装は syncProxyRulesIfNeeded() から syncRulesForContainer() を呼ぶたびに networkFilterService.getRules(environmentId) を再実行し entries を毎回生成しているため、getRules と enabledRules → ProxyRuleEntry の変換をループ前に一度だけ実行するよう修正してください: syncProxyRulesIfNeeded() の先頭で networkFilterService.getRules(environmentId) を1回呼び出し、enabledRules をフィルタして entries（ProxyRuleEntry の配列）を作り、それを各コンテナへ配るか syncRulesForContainer() に引数で渡すように変更し（同様の重複箇所はコメントにある72-75行の処理もまとめてください）。src/services/remote-repo-service.ts (2)
417-528: fetch/pullのタイムアウト処理。
fetchTimeoutとpullTimeoutの両方を管理し、finishラッパーで両方のタイムアウトをクリアしています。ただし、pullTimeoutはfetchProcess.on('close')内で初期化されるため、clearTimeout(pullTimeout)がfinishの最初の呼び出し時に未定義の可能性があります。現在の実装では
clearTimeout(undefined)はエラーを発生させませんが、明示的な初期化を検討してください。♻️ 明示的な初期化の提案
return new Promise((resolve) => { let settled = false; + let pullTimeout: ReturnType<typeof setTimeout> | undefined; const finish = (result: PullResult) => { if (settled) return; settled = true; clearTimeout(fetchTimeout); - clearTimeout(pullTimeout); + if (pullTimeout) clearTimeout(pullTimeout); resolve(result); }; - let pullTimeout: ReturnType<typeof setTimeout>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/remote-repo-service.ts` around lines 417 - 528, The issue is that pullTimeout may be undefined when finish() calls clearTimeout(pullTimeout); to fix, explicitly initialize pullTimeout (e.g., let pullTimeout: ReturnType<typeof setTimeout> | undefined = undefined) and guard the clearTimeout call in finish (only call clearTimeout(pullTimeout) if pullTimeout !== undefined) so calling finish from the fetch failure path is safe; update the variable declaration near the existing let pullTimeout and the finish function to include this defensive check (references: pullTimeout, fetchTimeout, finish, fetchProcess.on('close')).
541-543:getBranchesとgetDefaultBranchでの絶対パス検証。相対パスが渡された場合に早期リターンするガードが追加されていますが、
getBranchesは空配列を返し、getDefaultBranchは'main'を返すという非対称な動作があります。エラーをスローするか、両方とも同じパターン（例：空配列 / デフォルト値）に統一することを検討してください。現状でも機能上の問題はありませんが、一貫性の観点から改善の余地があります。
Also applies to: 632-634
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/remote-repo-service.ts` around lines 541 - 543, Both getBranches and getDefaultBranch currently handle non-absolute repoPath differently (one returns [] and the other returns 'main'); make them consistent by choosing a single behavior—preferably throwing a clear Error for invalid repoPath. Update the repoPath guard in both getBranches and getDefaultBranch to throw new Error(`getBranches: repoPath must be absolute: ${repoPath}`) / new Error(`getDefaultBranch: repoPath must be absolute: ${repoPath}`) (or a shared helper validateAbsoluteRepoPath that throws), replacing the early returns so both functions behave identically and include the repoPath in the error message for debugging.src/services/adapters/docker-adapter.ts (1)
948-960: createSession失敗時のクリーンアップでawaitを使用。セッション作成失敗時のクリーンアップでは
await proxyClient.deleteRules(containerIP)を使用していますが、onExitハンドラではvoidプレフィックスで fire-and-forget しています。一貫性を持たせるか、ここでもbest-effortとしてvoidを使用することを検討してください。ただし、セッション作成の catch ブロック内なので、呼び出し元には既にエラーがスローされるため、実質的な影響は軽微です。
♻️ 一貫性のための修正案
if (containerIP) { - try { - const proxyClient = new ProxyClient(); - await proxyClient.deleteRules(containerIP); - logger.info('DockerAdapter: Proxy rules cleaned up on createSession failure', { sessionId, containerIP }); - } catch (cleanupError) { - logger.warn('DockerAdapter: Failed to cleanup proxy rules on createSession failure', { - sessionId, - containerIP, - error: cleanupError instanceof Error ? cleanupError.message : 'Unknown error', - }); - } + void new ProxyClient().deleteRules(containerIP) + .then(() => { + logger.info('DockerAdapter: Proxy rules cleaned up on createSession failure', { sessionId, containerIP }); + }) + .catch((cleanupError: unknown) => { + logger.warn('DockerAdapter: Failed to cleanup proxy rules on createSession failure', { + sessionId, + containerIP, + error: cleanupError instanceof Error ? cleanupError.message : 'Unknown error', + }); + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/docker-adapter.ts` around lines 948 - 960, In the createSession failure catch block, make proxy cleanup fire-and-forget like the onExit handler by replacing the awaited call to ProxyClient.deleteRules(containerIP) with a void call and attach a .catch to log failures; specifically, change the try/await pattern around new ProxyClient() / proxyClient.deleteRules(containerIP) to invoke void proxyClient.deleteRules(containerIP).catch(cleanupError => logger.warn(...)) (keep the same log message and context: sessionId and containerIP) so you avoid blocking error handling while also preventing unhandled promise rejections.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 261-269: The current loop builds MUTATE_ARGS by splitting and
quoting each file which yields multiple args with literal quotes; instead pass
the existing comma-separated string directly to Stryker: stop the IFS/read loop
and feeding MUTATE_ARGS, and call the pnpm exec stryker run invocation with
--mutate "${{ steps.changed-files.outputs.mutate_targets }}" (or use the single
variable name in that position) so --mutate receives one comma-delimited
argument; update references to MUTATE_ARGS accordingly and remove the
quoting/concatenation logic.
In `@src/lib/__tests__/docker-volume-utils.test.ts`:
- Around line 46-50: The test currently asserts that getConfigVolumeNames('
spaced ').claudeVolume includes the raw whitespace, which contradicts the
Docker volume-name sanitization logic used when building volume names; update
the test to assert the sanitized behavior instead: call getConfigVolumeNames('
spaced ') and check that the returned claudeVolume contains no spaces (e.g.,
use expect(...).not.toContain(' ')) and assert the sanitized exact value that
matches the volume-naming sanitization logic (the output from
getConfigVolumeNames for that input), referencing the getConfigVolumeNames call
and the volume-naming sanitization logic used when generating volume names.
In `@src/lib/__tests__/schema-check.test.ts`:
- Around line 213-223: The boundary-value email test currently builds a
254-character address using domainBase (185 chars) which creates a single domain
label >63 and makes the test invalid; change the test in schema-check.test.ts
(the test named '罫線と修復方法を含む') to construct a valid 254-char email by splitting
the domain into multiple labels each <=63 characters (e.g., build local part +
'.'-joined labels whose lengths sum to the required remainder) and use that
constructed string in the assertion instead of the existing domainBase; ensure
any helper logic is local to the test and that the final email length is exactly
254 and all labels are <=63 so the boundary assertion is meaningful.
In `@src/lib/validation.ts`:
- Around line 126-127: The local-part validation currently only uses /^[^\s@]+$/
on the variable local and therefore allows leading/trailing dots and consecutive
dots; update the validation in the function that declares local and domain (the
code around the existing local/domain checks and the /^[^\s@]+$/ test) to also
reject any local that starts with '.' or ends with '.' or includes '..' (e.g.,
add guard checks like if (local.startsWith('.') || local.endsWith('.') ||
local.includes('..')) return false) before the existing regex test so addresses
like ".abc@..." or "a..b@..." are rejected.
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 405-429: The test uses a fallback when closeHandler is null which
masks whether the close handler was actually registered; update the test to
always exercise the cleanup path by ensuring the WS 'close' handler runs: after
localWss.emit('connection', ws, createMockReq('session-1')) and awaiting
flush(), assert that a close handler was registered (i.e. closeHandler is not
null) and then call it (or call ws.close()/emit 'close' on the ws) to trigger
cleanup; finally assert mockCM.removeConnection was called with
'session-1-terminal' and ws. Reference setupTerminalWebSocket, createMockWs,
localWss.emit('connection', ...), closeHandler, and mockCM.removeConnection to
locate and modify the test.
- Around line 290-318: The test currently allows a false positive by returning
success when messageHandler is not registered; change it so the test fails if
the message handler was never captured: after emitting the connection
(localWss.emit(...)) assert that messageHandler is truthy (or use an explicit
expect(messageHandler).toBeDefined()), then invoke the captured handler with the
input payload and assert mockPtyManager.write was called with
'session-1-terminal' and 'test'; alternatively, if the async flow is truly
flaky, mark the test with it.skip and add a TODO to investigate. Ensure you
update references to messageHandler, localWss.emit, setupTerminalWebSocket, and
mockPtyManager.write/mockPtyManager.createPTY accordingly so there is no
fallback branch that masks failures.
In `@src/services/__tests__/docker-pty-stream.test.ts`:
- Around line 287-293: Replace the fixed 10ms setTimeout-based waits in this
test file with vitest's vi.waitFor to avoid flaky timing; specifically, where
you currently do await new Promise(resolve => setTimeout(resolve, 10)) after
emitting events such as mockStream.emit('end'), change to await vi.waitFor(() =>
{ expect(mockExec.inspect).toHaveBeenCalled();
expect(exitSpy).toHaveBeenCalledWith({ exitCode: 0, signal: 0 }); }); and apply
the same pattern to the other 16 occurrences in the file so each test waits for
the expected assertions (e.g., checks involving mockExec.inspect, exitSpy, and
related stream emissions) rather than a fixed sleep.
In `@src/services/__tests__/gh-cli.test.ts`:
- Around line 166-170: The test assertion for getPRStatus uses mockExecFileSync
and currently matches the options with expect.any(Object); update that
expectation to explicitly verify the working directory by replacing
expect.any(Object) with expect.objectContaining({ cwd, encoding }) (or the
test's local cwd variable and encoding) so the call to mockExecFileSync('gh',
['pr','view','789','--json','state,merged'], ...) asserts the cwd is set; keep
the same mockExecFileSync and getPRStatus references when making the change.
In `@src/services/__tests__/pty-manager.test.ts`:
- Around line 95-97: The test attaches persistent listeners to the ptyManager
singleton using ptyManager.on('data', ...) (and similarly for 'error'), which
can accumulate across tests; change those to ptyManager.once('data', ...) /
ptyManager.once('error', ...) when you only need a single emission, or ensure
you remove the listeners in an afterEach by calling
ptyManager.off/removeListener for the same callback (references: ptyManager,
testSessionId, emittedData, the 'data' and 'error' event handlers) so tests no
longer leak event handlers between runs.
- Around line 61-66: The test for the concurrent-create guard is inconsistent:
the it() title says it should throw but the assertion uses not.toThrow(), and
another test block (around the second occurrence covering the same behavior) has
no assertions; update the tests to match the intended behavior by either
renaming the test to reflect non-throwing behavior or change the expectation to
expect(() => ptyManager.createPTY(testSessionId, testWorkingDir)).toThrow(), and
add an explicit assertion in the second test block to validate the creating-flag
behavior (e.g., verify that when createPTY is called concurrently it either
throws or returns a specific error/flag), referencing ptyManager.createPTY and
testSessionId to locate and modify the two failing test cases so their names and
assertions align with the documented spec.
- Around line 39-41: The tests mutate process.env without restoring it, risking
leakage between tests; update the test setup to snapshot and restore env or use
Vitest helpers: in the beforeEach/afterEach surrounding the tests in
pty-manager.test.ts (affecting tests around lines with vi.clearAllMocks and the
blocks at 286-359) either call vi.stubEnv(...) for per-test env values and
vi.unstubAllEnvs() in afterEach, or capture a copy of process.env in beforeEach
and restore it in afterEach so that functions like the pty manager tests do not
leave modified environment variables for subsequent tests.
In `@src/services/__tests__/remote-repo-service.test.ts`:
- Around line 33-35: The test suite in remote-repo-service.test.ts overwrites
process.env.ALLOW_LOCAL_REPO_URL in beforeAll but unconditionally deletes it in
afterAll; modify the tests to capture the original value (e.g., const
originalAllowLocal = process.env.ALLOW_LOCAL_REPO_URL) before setting it, and in
afterAll restore it by setting process.env.ALLOW_LOCAL_REPO_URL =
originalAllowLocal if defined or deleting the key if it was undefined; apply the
same pattern to the other test block(s) that set ALLOW_LOCAL_REPO_URL so
original environment state is preserved.
In `@src/services/adapters/__tests__/docker-adapter.test.ts`:
- Around line 288-295: The test mutates process.env.PROXY_NETWORK_NAME without
restoring its original value; update the test around the buildContainerOptions
invocation to save the original = process.env.PROXY_NETWORK_NAME before setting
it, then in the finally block restore it (set it back if original was defined,
or delete it if original was undefined) so that buildContainerOptions /
createOptions.HostConfig.NetworkMode checks do not break other tests.
In `@src/services/proxy-client.ts`:
- Around line 163-178: The code currently throws ProxyValidationError for all
4xx responses but its JSDoc says it represents only 422 validation errors;
change the behavior so ProxyValidationError is only thrown for status === 422
(preserving the existing JSON details parsing), and introduce a separate error
class (e.g., ProxyClientError or ProxyHttpError) for other 4xx responses that
accepts the HTTP status and message; update all throw sites that currently use
ProxyValidationError for non-422 4xx to use the new error class, and update the
JSDoc on ProxyValidationError to reflect it is strictly for 422 validation
failures.
In `@stryker.config.json`:
- Around line 16-20: The CI and local Stryker thresholds are inconsistent:
update the "break" threshold under "thresholds" in stryker.config.json from 0 to
85 so it matches the workflow CLI flag (--thresholds.break 85); locate the
"thresholds" object and change the "break" value to 85 (alternatively, remove
the CLI --thresholds.break flag in the workflow, but prefer updating the "break"
property to keep config authoritative).
---
Outside diff comments:
In `@src/services/__tests__/pty-manager.test.ts`:
- Around line 31-37: The tests currently call real filesystem APIs in
beforeAll/afterAll (mkdtempSync, rmSync, tmpdir, join) and similarly at the
second occurrence (lines ~73-77); replace those direct FS calls by mocking the
fs module (e.g., jest.mock('fs') or jest.spyOn) and stub mkdtempSync and rmSync
to return controlled values and no-op behavior, or provide a temporary in-memory
path string; ensure teardown uses the mocked rmSync behavior; update the tests
in pty-manager.test.ts to use the mocked implementations for mkdtempSync and
rmSync (and any tmpdir/join interactions) so the tests do not touch the real
filesystem and remain deterministic.
---
Nitpick comments:
In `@src/app/api/environments/__tests__/route.test.ts`:
- Around line 598-658: Test currently asserts only one rule via
expect.arrayContaining for mockApplyTemplates even though defaultTemplates
contains three rules; update the assertion in the
'DOCKER環境作成時にフィルタリングが有効化されテンプレートが適用される' test to verify all expected rules are
passed to applyTemplates by replacing the single expect.objectContaining check
with a more complete expectation that includes the three rule objects (e.g.,
assert array contains objects for 'api.anthropic.com' port 443 and
'*.npmjs.org'/'*.npmjs.com' port 443) when calling mockApplyTemplates; keep the
existing invocation order and call count assertions for mockGetDefaultTemplates,
mockCreateConfigVolumes, mockApplyTemplates, and mockUpdateFilterConfig.
In `@src/app/api/environments/`[id]/network-rules/__tests__/route.test.ts:
- Around line 166-180: The tests currently assert only that
mockSyncProxyRulesIfNeeded was called; update each relevant test (the one
calling POST/PUT/DELETE and asserting mockSyncProxyRulesIfNeeded with
toHaveBeenCalledWith) to also assert the exact call count by adding
expect(mockSyncProxyRulesIfNeeded).toHaveBeenCalledTimes(1) immediately after
the toHaveBeenCalledWith assertion so you detect accidental duplicate
invocations; apply this change for all test cases that reference
mockSyncProxyRulesIfNeeded (e.g., the tests invoking POST via POST(request, {
params: ... }), and the other tests that assert mockSyncProxyRulesIfNeeded) to
make the assertions stricter.
In `@src/lib/__tests__/docker-config-validator.test.ts`:
- Around line 161-164: The tests currently assert on a specific position
(expect(result.errors[0])...) which is brittle; update these assertions to check
presence using condition matching (e.g. use result.errors.some or
result.errors.find) instead of indexing so the order doesn't matter — replace
the two fragile expects that reference result.errors[0] (checking for '整数' and
'マッピング1') with existence checks that verify any entry contains those substrings,
and make the same change for the other occurrences noted (the similar assertions
around the other two failure cases).
In `@src/lib/__tests__/env-validation.test.ts`:
- Around line 59-78: The test calls validateRequiredEnvVars() twice; change it
to call it only once and capture the thrown error for all assertions: remove the
first expect(() => validateRequiredEnvVars()).toThrow(/DATABASE_URL/) call,
invoke validateRequiredEnvVars() inside a single try/catch (or use expect(() =>
validateRequiredEnvVars()).toThrow() combined with capturing the error via a
thrownError variable), assign the caught error to a variable (as Error), then
perform all message assertions (contains 'DATABASE_URL', 'Please follow these
steps', 'cp .env.example .env', 'Database connection URL', 'README.md') against
that single captured error; reference validateRequiredEnvVars in your change.
In `@src/lib/__tests__/notification-service.test.ts`:
- Around line 254-277: The test manually restores mockLocalStorage.setItem and
consoleSpy outside of a finally block which can leak state on assertion
failures; wrap the temporary override of (mockLocalStorage as any).setItem and
the console spy teardown in a try/finally (or use
vi.spyOn(...).mockImplementation and rely on mockRestore in finally) so that
after calling saveSettings(...) (the test invoking saveSettings) you always
restore originalSetItem and call consoleSpy.mockRestore() in the finally block
to guarantee cleanup even if assertions fail.
In `@src/lib/__tests__/run-scripts.test.ts`:
- Around line 18-25: Add a unit test that exercises element-shape validation for
parseRunScripts: supply JSON.stringify([{ name: 1 }]) (and similarly for [{
command: 123 }]) and assert the function returns the expected safe result (e.g.,
an empty array or only-valid entries) so the behavior is pinned; locate tests
around parseRunScripts and RunScript in this test file and add two cases
(invalid name type and invalid command type) mirroring the existing "should
parse valid JSON array" test and the other test at lines 43-46.
In `@src/lib/__tests__/session.test.ts`:
- Around line 44-98: The tests create a console.warn spy (warnSpy) and restore
it immediately after importing the session module, which can leave teardown
inconsistent on failures; change each test to ensure warnSpy is always restored
by either moving the restore into a shared afterEach cleanup (declare warnSpy in
the test scope) or wrap the import/assertion block in a try/finally that calls
warnSpy.mockRestore(), referencing the existing warnSpy, the dynamic
import('../session') and assertions against mod.sessionOptions.cookieOptions so
the spy is reliably cleaned up even if an assertion throws.
In `@src/lib/proxy-sync.ts`:
- Around line 20-30: 現在の実装は syncProxyRulesIfNeeded() から syncRulesForContainer()
を呼ぶたびに networkFilterService.getRules(environmentId) を再実行し entries
を毎回生成しているため、getRules と enabledRules → ProxyRuleEntry
の変換をループ前に一度だけ実行するよう修正してください: syncProxyRulesIfNeeded() の先頭で
networkFilterService.getRules(environmentId) を1回呼び出し、enabledRules をフィルタして
entries（ProxyRuleEntry の配列）を作り、それを各コンテナへ配るか syncRulesForContainer()
に引数で渡すように変更し（同様の重複箇所はコメントにある72-75行の処理もまとめてください）。
In `@src/lib/volume-naming.ts`:
- Around line 37-44: The current loop removes leading/trailing '-' from variable
deduped by repeatedly slicing into result, which causes O(n^2) copies; instead
compute start and end indices once by advancing a startIndex while
deduped[startIndex] === '-' and decrementing an endIndex while deduped[endIndex
- 1] === '-' (preserving the same per-character checks used now to avoid regex),
then set result = deduped.slice(startIndex, endIndex) in one operation. Update
the block that currently uses result.startsWith / result.endsWith and result =
result.slice(...) to use startIndex/endIndex and a single final slice to
maintain behavior and performance.
In `@src/lib/websocket/__tests__/session-ws.test.ts`:
- Around line 100-168: The test creates a SessionWebSocketHandler instance as
const h = new SessionWebSocketHandler(cm) solely for its side effects but leaves
h unused, which triggers a linter warning; fix by either marking the variable as
intentionally unused (e.g., add a void h; after instantiation) or rename it with
an underscore prefix (e.g., const _h = new SessionWebSocketHandler(cm)) in each
test that currently declares h, so the side-effect construction remains but the
linter stop flag is addressed.
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 103-107: The current flush() awaits setImmediate twice which is
arbitrary and tests inconsistently call flush multiple times; introduce a single
shared constant (e.g., FLUSH_IMMEDIATE_COUNT) and refactor flush() to loop
awaiting setImmediate that many times (or add a helper flushAll() that does
this), then replace the ad-hoc loops like "for (let i = 0; i < 5; i++) await
flush();" with a single call to the unified helper so tests and the flush
implementation (symbols: flush(), flushAll(), FLUSH_IMMEDIATE_COUNT) use the
same, clearly-configured number of micro/macrotask drains.
In `@src/services/__tests__/config-service.test.ts`:
- Around line 236-248: Remove the unused variable origReadFile declared on line
where const origReadFile = fs.readFile; since the test restores the spy via
vi.mocked(fs.readFile).mockRestore() there is no need to keep the original
reference; simply delete the origReadFile declaration so the test only mocks and
restores fs.readFile (related symbols: origReadFile, fs.readFile, vi.spyOn,
vi.mocked(...).mockRestore, configService.load, configService.getConfig,
mockLogger.error).
In `@src/services/__tests__/developer-settings-service.test.ts`:
- Around line 275-284: The test calls service.deleteProjectSettings twice to
assert both the error type and message; instead, call it once, capture the
rejected error (e.g., using await expect(...).rejects.toThrow or try/catch) and
then make two assertions against that single error object: verify it is an
instance of SettingsNotFoundError and that its message matches /Settings not
found: project non-existent-project/; update the test around mockDbSelectGet and
service.deleteProjectSettings to perform one invocation and assert both the
class and message on the same caught error.
In `@src/services/__tests__/docker-pty-adapter.test.ts`:
- Around line 154-176: The test should explicitly validate the container's
working directory using expect.objectContaining to assert the cwd/WorkingDir
instead of a bare equality; update the assertion around
mockDockerClient.createContainer (referencing createSession ->
mockDockerClient.createContainer and the local createOptions) to use
expect(createOptions).toEqual(expect.objectContaining({ WorkingDir: '/workspace'
})) or assert mockDockerClient.createContainer was called with
expect.objectContaining({ WorkingDir: '/workspace' })) so the cwd is explicitly
verified.
In `@src/services/adapters/__tests__/base-adapter.test.ts`:
- Around line 341-352: The test currently mutates process.env directly before
calling adapter.testSpawnPTY('claude', ...) which can leak state if the
assertion fails; replace the manual assignment with vi.stubEnv({ CLAUDECODE:
'should-be-excluded' }) to automatically restore environment after the test, or
wrap the mutation in a try/finally that sets process.env = originalEnv in
finally to guarantee restoration; update the assertion code that reads pty.spawn
mock (vi.mocked(pty.spawn).mock.calls[0]) unchanged.
In `@src/services/adapters/docker-adapter.ts`:
- Around line 948-960: In the createSession failure catch block, make proxy
cleanup fire-and-forget like the onExit handler by replacing the awaited call to
ProxyClient.deleteRules(containerIP) with a void call and attach a .catch to log
failures; specifically, change the try/await pattern around new ProxyClient() /
proxyClient.deleteRules(containerIP) to invoke void
proxyClient.deleteRules(containerIP).catch(cleanupError => logger.warn(...))
(keep the same log message and context: sessionId and containerIP) so you avoid
blocking error handling while also preventing unhandled promise rejections.
In `@src/services/remote-repo-service.ts`:
- Around line 417-528: The issue is that pullTimeout may be undefined when
finish() calls clearTimeout(pullTimeout); to fix, explicitly initialize
pullTimeout (e.g., let pullTimeout: ReturnType<typeof setTimeout> | undefined =
undefined) and guard the clearTimeout call in finish (only call
clearTimeout(pullTimeout) if pullTimeout !== undefined) so calling finish from
the fetch failure path is safe; update the variable declaration near the
existing let pullTimeout and the finish function to include this defensive check
(references: pullTimeout, fetchTimeout, finish, fetchProcess.on('close')).
- Around line 541-543: Both getBranches and getDefaultBranch currently handle
non-absolute repoPath differently (one returns [] and the other returns 'main');
make them consistent by choosing a single behavior—preferably throwing a clear
Error for invalid repoPath. Update the repoPath guard in both getBranches and
getDefaultBranch to throw new Error(`getBranches: repoPath must be absolute:
${repoPath}`) / new Error(`getDefaultBranch: repoPath must be absolute:
${repoPath}`) (or a shared helper validateAbsoluteRepoPath that throws),
replacing the early returns so both functions behave identically and include the
repoPath in the error message for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f1f79e3-b0e8-4ee7-9bc9-3cb4a569dba2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
.github/workflows/test.yml.gitignorepackage.jsonsrc/app/api/docker/image-build/__tests__/route.test.tssrc/app/api/environments/[id]/network-filter/__tests__/route.test.tssrc/app/api/environments/[id]/network-rules/__tests__/route.test.tssrc/app/api/environments/__tests__/route.test.tssrc/lib/__tests__/action-detector.test.tssrc/lib/__tests__/db-module.test.tssrc/lib/__tests__/docker-config-validator.test.tssrc/lib/__tests__/docker-volume-utils.test.tssrc/lib/__tests__/env-validation.test.tssrc/lib/__tests__/errors.test.tssrc/lib/__tests__/notification-service.test.tssrc/lib/__tests__/path-safety.test.tssrc/lib/__tests__/proxy-sync.test.tssrc/lib/__tests__/run-scripts.test.tssrc/lib/__tests__/schema-check.test.tssrc/lib/__tests__/session-name-generator.test.tssrc/lib/__tests__/session.test.tssrc/lib/__tests__/validation.test.tssrc/lib/path-safety.tssrc/lib/proxy-sync.tssrc/lib/validation.tssrc/lib/volume-naming.tssrc/lib/websocket/__tests__/session-ws.test.tssrc/lib/websocket/__tests__/terminal-ws.test.tssrc/services/__tests__/adapter-factory.test.tssrc/services/__tests__/config-service.test.tssrc/services/__tests__/developer-settings-service.test.tssrc/services/__tests__/docker-client.test.tssrc/services/__tests__/docker-git-service.test.tssrc/services/__tests__/docker-pty-adapter.test.tssrc/services/__tests__/docker-pty-stream.test.tssrc/services/__tests__/encryption-service.test.tssrc/services/__tests__/environment-adapter.test.tssrc/services/__tests__/gh-cli.test.tssrc/services/__tests__/git-operations.test.tssrc/services/__tests__/github-pat-service.test.tssrc/services/__tests__/port-checker.test.tssrc/services/__tests__/proxy-client.test.tssrc/services/__tests__/pty-manager.test.tssrc/services/__tests__/remote-repo-service.test.tssrc/services/__tests__/run-script-manager.test.tssrc/services/__tests__/scrollback-buffer.test.tssrc/services/adapter-factory.tssrc/services/adapters/__tests__/base-adapter.test.tssrc/services/adapters/__tests__/docker-adapter.test.tssrc/services/adapters/__tests__/host-adapter.test.tssrc/services/adapters/docker-adapter.tssrc/services/environment-adapter.tssrc/services/proxy-client.tssrc/services/remote-repo-service.tsstryker.config.jsonvitest.config.stryker.ts
Comment on lines
+213
to
+223
| const lines = message.split('\n'); | ||
| expect(lines.length).toBeGreaterThan(5); | ||
| }); | ||
|
|
||
| it('罫線と修復方法を含む', () => { | ||
| const result = { | ||
| valid: false, | ||
| missingColumns: [ | ||
| { table: 'Project', column: 'test_col', expectedType: 'text' }, | ||
| ], | ||
| checkedTables: ['Project'], |
There was a problem hiding this comment.
境界値テストが不完全です。
このテストは254文字のメールアドレスの境界値テストを意図していますが、domainBaseが185文字となりラベル長制限（63文字）を超えるため、実際のアサーションが欠落しています。コメントでも「Need to split into valid labels」と記載されていますが、修正されていません。
🛠️ 修正案: 有効な254文字のメールアドレスを構築
it('ちょうど254文字のアドレスは受け入れる（境界値）', () => {
- // local@domain.com: local = 'a'.repeat(X), domain needs to fill to 254 total
- // 254 = local.length + 1(@) + domain.length
- const local = 'a'.repeat(64);
- const domainBase = 'b'.repeat(254 - 64 - 1 - 4); // -4 for '.com'
- const domain = domainBase + '.com';
- const email = `${local}@${domain}`;
- expect(email.length).toBe(254);
- // Domain labels max 63 chars - domainBase is 185 chars, over 63
- // Need to split into valid labels
+ // 254文字のメールを有効なラベル構成で構築
+ // local(64) + @(1) + domain(189) = 254
+ const local = 'a'.repeat(64);
+ // domain: 63文字ラベル x 2 + 61文字ラベル + .com = 63+1+63+1+61+4 = 193 (調整が必要)
+ const label63 = 'b'.repeat(63);
+ const label59 = 'c'.repeat(59);
+ const domain = `${label63}.${label63}.${label59}.com`;
+ const email = `${local}@${domain}`;
+ expect(email.length).toBe(254);
+ expect(isValidEmail(email)).toBe(true);
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/__tests__/schema-check.test.ts` around lines 213 - 223, The
boundary-value email test currently builds a 254-character address using
domainBase (185 chars) which creates a single domain label >63 and makes the
test invalid; change the test in schema-check.test.ts (the test named
'罫線と修復方法を含む') to construct a valid 254-char email by splitting the domain into
multiple labels each <=63 characters (e.g., build local part + '.'-joined labels
whose lengths sum to the required remainder) and use that constructed string in
the assertion instead of the existing domainBase; ensure any helper logic is
local to the test and that the final email length is exactly 254 and all labels
are <=63 so the boundary assertion is meaningful.
Comment on lines
+163
to
+178
| if (response.status === 422) { | ||
| let details: Array<{ field: string; message: string }> | undefined; | ||
| try { | ||
| const errorBody = await response.json() as { details?: Array<{ field: string; message: string }> }; | ||
| details = errorBody.details; | ||
| } catch { | ||
| // JSON解析失敗は無視 | ||
| } | ||
| throw new ProxyValidationError('ルールのバリデーションに失敗しました', details); | ||
| } | ||
|
|
||
| if (response.status >= 400 && response.status < 500) { | ||
| throw new ProxyValidationError( | ||
| `proxyがクライアントエラーを返しました: HTTP ${response.status}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What HTTP status codes are typically considered validation errors vs client errors?
💡 Result:
Validation errors (commonly used in APIs) are a subset of 4xx client errors:
- 400 Bad Request: use when the server “cannot or will not process” the request due to a client error such as malformed request syntax or invalid request framing (often: invalid JSON, missing/invalid syntax-level inputs). [1]
- 422 Unprocessable Content: use when the server understands the content type and the request is syntactically correct, but it can’t process it due to semantic/validation failures (e.g., field fails constraints, semantically invalid instructions). [1][2] (Often described as “validation errors” in API tooling/docs.) [3]
Client errors (broader category) = all 4xx codes, including (not limited to):
- 401 Unauthorized / 403 Forbidden: authentication/authorization problems. [1]
- 404 Not Found: resource doesn’t exist (or not disclosed). [1]
- 405 Method Not Allowed: HTTP method not supported for the resource. [1]
- 409 Conflict: request conflicts with current server state (often business-rule/state conflicts, e.g., version conflicts, uniqueness conflicts). [1]
- 415 Unsupported Media Type: unsupported
Content-Type/encoding. [1] - (Also commonly: 429 Too Many Requests for rate limiting—still a 4xx client error.)
Sources
[1] RFC 9110 (HTTP Semantics) — definitions for 400/401/403/404/409/415/422: (datatracker.ietf.org)
[2] MDN — 422 Unprocessable Content (semantic/validation-type meaning): (developer.mozilla.org)
[3] Postman — 422 commonly indicates syntactically-correct requests that fail processing/validation: (blog.postman.com)
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc9110-
- 2: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/422?utm_source=openai
- 3: https://blog.postman.com/http-422-error/?utm_source=openai
🏁 Script executed:
cat -n src/services/proxy-client.ts | sed -n '163,178p'Repository: windschord/claude-work
Length of output: 765
🏁 Script executed:
rg -n "class ProxyValidationError|class Proxy.*Error" src/services/proxy-client.ts -A 5Repository: windschord/claude-work
Length of output: 450
🏁 Script executed:
rg -n "ProxyValidationError|ProxyConnectionError" src/ --type ts --type tsx -B 2 -A 2Repository: windschord/claude-work
Length of output: 93
🏁 Script executed:
rg -n "ProxyValidationError|ProxyConnectionError" src/ -B 2 -A 2Repository: windschord/claude-work
Length of output: 25782
クラス名と実装の不一致: ProxyValidationErrorは422以外の4xxエラーにも使用されている
ProxyValidationErrorのJSDocでは「プロキシへのリクエストのバリデーションエラー（422レスポンス）」と記載されていますが、実装では400～499のすべての4xxエラーに対してこのエラーをスローしています（lines 174-177, 212-215, 245-248）。
これにより、セマンティック的に異なるエラーが同じクラスでラップされます：
- 422: リクエストが構文的に正しいが検証に失敗（実際のバリデーションエラー）
- 404: リソースが見つからない
- 401/403: 認証・認可エラー
- 400: リクエスト形式の誤り
- etc.
呼び出し側でエラータイプを区別する必要がある場合、クラス名またはステータスコードベースのエラー分類の見直しを検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/proxy-client.ts` around lines 163 - 178, The code currently
throws ProxyValidationError for all 4xx responses but its JSDoc says it
represents only 422 validation errors; change the behavior so
ProxyValidationError is only thrown for status === 422 (preserving the existing
JSON details parsing), and introduce a separate error class (e.g.,
ProxyClientError or ProxyHttpError) for other 4xx responses that accepts the
HTTP status and message; update all throw sites that currently use
ProxyValidationError for non-422 4xx to use the new error class, and update the
JSDoc on ProxyValidationError to reflect it is strictly for 422 validation
failures.
There was a problem hiding this comment.
Pull request overview
Stryker（Vitest runner）によるミューテーションテストを導入し、CI に変更ファイル対象のミューテーションテスト品質ゲートを追加するとともに、関連するサービス/ライブラリ群のテスト拡充と一部セキュリティ・テスタビリティ改善（proxy 同期、パス安全化、入力検証強化など）を行うPRです。
Changes:
- Stryker + Vitest runner の設定追加と
test:mutationスクリプト/レポート除外の整備 - GitHub Actions に PR 時の変更ファイルのみを対象にした mutation-test ジョブ（閾値 85%）を追加
src/services/src/libの多数モジュールでテスト追加・改善、および network-filter proxy 同期/パス安全化等の軽微なリファクタ
Reviewed changes
Copilot reviewed 50 out of 53 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | ミューテーションテスト用 Vitest 設定（除外/環境等）を追加 |
| stryker.config.json | Stryker の mutate 対象/レポーター/閾値などを定義 |
| src/services/remote-repo-service.ts | clone/pull の安全性・タイムアウト・パス検証を強化 |
| src/services/proxy-client.ts | network-filter-proxy 管理 API 用の HTTP クライアントを追加 |
| src/services/environment-adapter.ts | セッション作成オプションに filterEnabled を追加 |
| src/services/adapters/docker-adapter.ts | フィルタ有効時のネットワーク設定・proxy ルール同期/クリーンアップを追加 |
| src/services/adapters/tests/docker-adapter.test.ts | DockerAdapter のフィルタリング動作（NetworkMode/env/同期/掃除）をテスト追加 |
| src/services/adapters/tests/base-adapter.test.ts | BasePTYAdapter のログ/サニタイズ等のテスト強化 |
| src/services/adapter-factory.ts | environmentId からキャッシュ済み DockerAdapter を取得する API を追加 |
| src/services/tests/scrollback-buffer.test.ts | ScrollbackBuffer の境界/UTF-8/コンパクション等のテスト追加 |
| src/services/tests/run-script-manager.test.ts | stop(SIGTERM/SIGKILL)/spawn error/exit cleanup 等のテスト追加 |
| src/services/tests/remote-repo-service.test.ts | ローカル URL 制御や edge case を含むテスト拡充 |
| src/services/tests/pty-manager.test.ts | PTYManager の挙動/例外/許可ディレクトリ等のテスト拡充 |
| src/services/tests/port-checker.test.ts | config 不正時などのフォールバック/単一ポート判定のテスト追加 |
| src/services/tests/github-pat-service.test.ts | insert/update 引数や暗号化/復号化エラーの検証を追加 |
| src/services/tests/git-operations.test.ts | GitOperationError のテストを新規追加 |
| src/services/tests/gh-cli.test.ts | gh コマンド呼び出しをモックしてユースケース別テストを拡充 |
| src/services/tests/environment-adapter.test.ts | type guard の網羅性向上 |
| src/services/tests/encryption-service.test.ts | エラークラス/鍵長/形式などのテスト追加 |
| src/services/tests/docker-git-service.test.ts | Docker Git 操作（clone/worktree/diff 等）のテスト拡充 |
| src/services/tests/docker-client.test.ts | DockerClient の各メソッドとエラーハンドリングのテスト拡充 |
| src/services/tests/developer-settings-service.test.ts | SettingsNotFoundError 等のテスト追加 |
| src/services/tests/config-service.test.ts | ConfigService のデフォルト/ログ/保存/シングルトン等のテスト拡充 |
| src/services/tests/adapter-factory.test.ts | AdapterFactory のログ/異常系/新規 API のテスト追加 |
| src/lib/websocket/tests/session-ws.test.ts | SessionWebSocketHandler のイベント中継/接続処理を詳細にテスト |
| src/lib/volume-naming.ts | slug 生成の処理を分割して安全性を向上（ReDoS 対策） |
| src/lib/validation.ts | メールアドレス検証 isValidEmail を追加 |
| src/lib/proxy-sync.ts | DB ルール→proxy 形式変換と再同期ヘルパーを追加 |
| src/lib/path-safety.ts | パス正規化/ベース配下判定/安全なコンポーネント検証を追加 |
| src/lib/tests/validation.test.ts | validation の境界/ミューテーション耐性を含むテスト追加 |
| src/lib/tests/session.test.ts | session 設定（開発/本番/secure 等）のテストを新規追加 |
| src/lib/tests/session-name-generator.test.ts | 乱数・試行回数境界などのテスト追加 |
| src/lib/tests/schema-check.test.ts | schema integrity の追加ケース/フォーマット検証を追加 |
| src/lib/tests/run-scripts.test.ts | run-scripts の parse/serialize のテストを新規追加 |
| src/lib/tests/proxy-sync.test.ts | proxy-sync の分岐/エラー時ログ等のテストを新規追加 |
| src/lib/tests/path-safety.test.ts | path-safety のユニットテストを新規追加 |
| src/lib/tests/notification-service.test.ts | 設定フォールバック/保存失敗/通知タイトル等のテスト追加 |
| src/lib/tests/errors.test.ts | エラー応答/ログ/stack 表示条件等のテスト追加 |
| src/lib/tests/env-validation.test.ts | エラーメッセージ内容/which 解決分岐等のテスト追加 |
| src/lib/tests/docker-volume-utils.test.ts | docker-volume-utils のテストを新規追加 |
| src/lib/tests/docker-config-validator.test.ts | validator のメッセージ/境界/警告等のテスト追加 |
| src/lib/tests/db-module.test.ts | db モジュールの export/基本動作を新規テスト |
| src/lib/tests/action-detector.test.ts | stripAnsi/detectActionRequest/cooldown のテスト追加 |
| package.json | test:mutation スクリプトと Stryker 依存関係を追加 |
| .gitignore | Stryker レポート/一時ディレクトリを除外 |
| .github/workflows/test.yml | mutation-test ジョブを追加し、最終ゲートに含める |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/test.yml
Outdated
Comment on lines
+262
to
+268
| MUTATE_ARGS="" | ||
| for f in "${FILES[@]}"; do | ||
| MUTATE_ARGS="$MUTATE_ARGS \"$f\"" | ||
| done | ||
|
|
||
| pnpm exec stryker run \ | ||
| --mutate $MUTATE_ARGS \ |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/lib/__tests__/db-module.test.ts (2)
41-45:DATABASE_URLの判定条件を少し強化できます。
not.toBe('')だと空白文字のみの値を許容します。trim()後の truthy 判定のほうが意図を正確に表せます。🔧 提案diff
- expect(process.env.DATABASE_URL).toBeDefined(); - expect(process.env.DATABASE_URL).not.toBe(''); + expect(process.env.DATABASE_URL).toBeDefined(); + expect(process.env.DATABASE_URL?.trim()).toBeTruthy();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/db-module.test.ts` around lines 41 - 45, The test "should have DATABASE_URL set in test environment" currently allows whitespace-only values; update the assertion to validate process.env.DATABASE_URL after trimming (use process.env.DATABASE_URL?.trim()) so the test requires a non-empty, non-whitespace value — keep the existing toBeDefined() check and replace the not.toBe('') assertion with a trimmed truthy check (or equivalent regex like /\S/) referencing the same test block and process.env.DATABASE_URL.
8-10: モジュール初期化の検証が import キャッシュに依存しています。Line 9 の静的 import により、Line 28/34 の dynamic import は同一モジュールキャッシュを参照します。これだと「モジュール読み込み時」の検証（初期化パスや環境変数検証）を実質的に再評価できません。
vi.resetModules()を使った分離を推奨します。♻️ 提案diff
-import { describe, it, expect } from 'vitest'; +import { beforeEach, describe, it, expect, vi } from 'vitest'; -// モジュールが既にロード済みなので、直接importしてテストする -import { db, schema } from '../db'; +beforeEach(() => { + vi.resetModules(); +}); it('should have db as default export', async () => { const mod = await import('../db'); - expect(mod.default).toBe(db); + expect(mod.default).toBe(mod.db); });Also applies to: 27-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/db-module.test.ts` around lines 8 - 10, The test currently uses a static import of db and schema which primes the module cache and prevents later dynamic imports from re-evaluating module initialization; remove or stop using the top-level import of { db, schema } and instead call vi.resetModules() before each scenario and use dynamic import('../db') to load a fresh module instance so the initialization paths and env-var checks are re-run; ensure the tests reference the newly imported module's exports (db, schema) from the dynamic import rather than the stale static import.src/lib/websocket/__tests__/session-ws.test.ts (1)
356-405: handleMessageテストのタイミングに関する軽微な懸念
ws.emit('message', message)は同期的に実行されますが、handleMessageは非同期関数です。イベントハンドラ内でhandleMessageを呼び出す際にawaitされていない場合、handleMessage内にawaitがあると、アサーションが完了前に実行される可能性があります。現在の実装では
ws.sendの呼び出し前に非同期処理がないようなので問題ありませんが、将来の変更で壊れる可能性があります。より堅牢にするにはawait vi.waitFor()の使用を検討できます。🔧 より堅牢なテストパターンの例
it('should respond with error for any message type', async () => { const ws = createMockWs(); vi.mocked(db.query.sessions.findFirst).mockResolvedValue({ status: 'running', } as any); await handler.handleConnection(ws, 'session-1'); // Send a message const message = Buffer.from(JSON.stringify({ type: 'input', data: 'test' })); ws.emit('message', message); + // handleMessageが非同期の場合に備えて待機 + await vi.waitFor(() => { + expect(ws.send).toHaveBeenCalledWith( + expect.stringContaining('Use /ws/claude/:sessionId') + ); + }); - // Should respond with error directing to Claude WS - expect(ws.send).toHaveBeenCalledWith( - expect.stringContaining('Use /ws/claude/:sessionId') - ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/session-ws.test.ts` around lines 356 - 405, The tests call ws.emit('message', ...) which triggers the asynchronous handler.handleMessage via handler.handleConnection; to avoid race conditions when handleMessage contains await, change assertions to wait for the expected side effects using vi.waitFor: e.g., after ws.emit(...) wrap the expect(...) calls inside await vi.waitFor(() => { expect(ws.send)... }) or await vi.waitFor(() => { expect(mockLifecycleManager.updateActivity).toHaveBeenCalledWith('session-1') }); this keeps tests robust if handleMessage becomes asynchronous in the future while still using the same symbols handler.handleConnection, ws.emit, ws.send, and mockLifecycleManager.updateActivity.src/services/__tests__/config-service.test.ts (2)
236-236: 未使用変数_origReadFileを削除してください。この変数は宣言されていますが、コード内で使用されていません。元の関数を保持する必要がなければ削除するのが適切です。
♻️ 修正案
// readFileをモックして非ENOENTエラーを発生させる - const _origReadFile = fs.readFile; vi.spyOn(fs, 'readFile').mockRejectedValueOnce(Object.assign(new Error('Permission denied'), { code: 'EACCES' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` at line 236, テスト内で宣言されている未使用の変数 `_origReadFile` を削除してください: 該当箇所は `const _origReadFile = fs.readFile;` の宣言行で、元の `fs.readFile` を保持する目的がないためこの代入を取り除き、関連する参照がないことを確認して不要な変数定義を削除してください。
317-322: JSONフォーマット検証のアサーションが弱いです。
content.includes(' ')は2スペースインデントの検証としては不十分です。2スペースは文字列値内にも偶然含まれる可能性があります。♻️ より堅牢なアサーション案
const content = await fs.readFile(testConfigPath, 'utf-8'); // 2スペースインデントであること - expect(content).toContain(' '); + // 改行+2スペースのパターンでインデントを検証 + expect(content).toMatch(/\n "/); const parsed = JSON.parse(content); expect(parsed.git_clone_timeout_minutes).toBe(7);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 317 - 322, The JSON indentation assertion is weak — replace the fragile expect(content).toContain(' ') with a robust check: read the file into content (as already done with testConfigPath), parse it (parsed) and then either re-stringify parsed with JSON.stringify(parsed, null, 2) and assert equality with content (or trimmed variants), or assert via a regex that property lines (e.g., the "git_clone_timeout_minutes" line) start with exactly two spaces; update the test around variables content and parsed accordingly to ensure you verify actual 2-space indentation rather than accidental spaces inside string values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/__tests__/validation.test.ts`:
- Around line 225-236: The test "ちょうど255文字のアドレスは拒否する（境界値）" currently only
asserts >254 instead of exactly 255; update the test that builds longEmail so
its length equals 255 (construct local and domain parts precisely, e.g. use
string repeats to sum to 255 including the '@' and dots), replace the
expect(longEmail.length).toBeGreaterThan(254) with
expect(longEmail.length).toBe(255), and keep the assertion
expect(isValidEmail(longEmail)).toBe(false) to verify the validator rejects a
255-character address (refer to isValidEmail and the test case name to locate
changes).
- Around line 213-223: The test currently only asserts the constructed email
length but doesn't call isValidEmail; update the "ちょうど254文字のアドレスは受け入れる（境界値）"
test to build a valid domain made of labels <=63 chars (split domainBase into
chunks of max 63 and join with '.' to form domain), ensure email =
`${local}@${domain}` is length 254, then add an assertion like
expect(isValidEmail(email)).toBe(true) to verify the validation; refer to the
test variables (local, domainBase, domain) and the isValidEmail function when
making the change.
In `@src/services/__tests__/run-script-manager.test.ts`:
- Around line 276-279: The lint failure comes from using single-expression arrow
callbacks with forEach (implicit return) such as errorHandlers.forEach((handler)
=> handler(new Error('spawn ENOENT'))); update each forEach callback
(occurrences around the eventHandlers/errorHandlers usages) to use a block arrow
with no return, e.g. forEach((handler) => { handler(...); }); — apply this
change for all similar calls (the ones that iterate handlers via .forEach in the
test file, including the places that create/dispatch error handlers) so the
callbacks do not implicitly return a value and satisfy the
useIterableCallbackReturn rule.
In `@src/services/adapters/__tests__/host-adapter.test.ts`:
- Around line 100-108: The test is emitting ptyManager data for the wrong
session id; change the emit call to use the session created by
hostAdapter.createSession so it actually verifies non-shell sessions aren't
forwarded. Specifically, keep the call to hostAdapter.createSession('claude-1',
'/path') and replace mockPtyManagerObj.emit('data', 'unknown-session', 'output
data') with mockPtyManagerObj.emit('data', 'claude-1', 'output data') so the
dataHandler on the 'data' event is asserted not to be called for that non-shell
session.
- Around line 392-401: Update the test "should handle async destroySession
Promise rejection" to add real assertions: call
hostAdapter.createSession('session-1','/path/to/work') then mockPtyInstance.kill
to throw, then await or use expect(...).resolves on the second createSession
call (hostAdapter.createSession('session-1','/path/to/work2')) to assert it does
not reject, and additionally assert observable state changed — e.g. verify
mockPtyInstance.kill was called and that hostAdapter.getSession('session-1') (or
equivalent accessor) reflects the new work path or new session instance; use
hostAdapter.createSession, mockPtyInstance.kill and hostAdapter.getSession (or
the adapter's session map) to locate and assert the expected behavior.
- Around line 6-18: The shared EventEmitter on mockPtyManagerObj (created via
vi.hoisted) is leaking listeners across tests causing false positives for
listenerCount; modify tests to reset listeners before each test by calling
mockPtyManagerObj.removeAllListeners() (or recreate the emitter) in a
beforeEach/afterEach block so that assertions on
mockPtyManagerObj.listenerCount(...) are strict and deterministic; ensure this
change is applied where mockPtyManagerObj is used (including the block around
lines that define the hoisted value and the tests referencing listenerCount).
---
Nitpick comments:
In `@src/lib/__tests__/db-module.test.ts`:
- Around line 41-45: The test "should have DATABASE_URL set in test environment"
currently allows whitespace-only values; update the assertion to validate
process.env.DATABASE_URL after trimming (use process.env.DATABASE_URL?.trim())
so the test requires a non-empty, non-whitespace value — keep the existing
toBeDefined() check and replace the not.toBe('') assertion with a trimmed truthy
check (or equivalent regex like /\S/) referencing the same test block and
process.env.DATABASE_URL.
- Around line 8-10: The test currently uses a static import of db and schema
which primes the module cache and prevents later dynamic imports from
re-evaluating module initialization; remove or stop using the top-level import
of { db, schema } and instead call vi.resetModules() before each scenario and
use dynamic import('../db') to load a fresh module instance so the
initialization paths and env-var checks are re-run; ensure the tests reference
the newly imported module's exports (db, schema) from the dynamic import rather
than the stale static import.
In `@src/lib/websocket/__tests__/session-ws.test.ts`:
- Around line 356-405: The tests call ws.emit('message', ...) which triggers the
asynchronous handler.handleMessage via handler.handleConnection; to avoid race
conditions when handleMessage contains await, change assertions to wait for the
expected side effects using vi.waitFor: e.g., after ws.emit(...) wrap the
expect(...) calls inside await vi.waitFor(() => { expect(ws.send)... }) or await
vi.waitFor(() => {
expect(mockLifecycleManager.updateActivity).toHaveBeenCalledWith('session-1')
}); this keeps tests robust if handleMessage becomes asynchronous in the future
while still using the same symbols handler.handleConnection, ws.emit, ws.send,
and mockLifecycleManager.updateActivity.
In `@src/services/__tests__/config-service.test.ts`:
- Line 236: テスト内で宣言されている未使用の変数 `_origReadFile` を削除してください: 該当箇所は `const
_origReadFile = fs.readFile;` の宣言行で、元の `fs.readFile`
を保持する目的がないためこの代入を取り除き、関連する参照がないことを確認して不要な変数定義を削除してください。
- Around line 317-322: The JSON indentation assertion is weak — replace the
fragile expect(content).toContain(' ') with a robust check: read the file into
content (as already done with testConfigPath), parse it (parsed) and then either
re-stringify parsed with JSON.stringify(parsed, null, 2) and assert equality
with content (or trimmed variants), or assert via a regex that property lines
(e.g., the "git_clone_timeout_minutes" line) start with exactly two spaces;
update the test around variables content and parsed accordingly to ensure you
verify actual 2-space indentation rather than accidental spaces inside string
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8b09638-3d4c-4df0-b76c-a70ca1df4a6d
📒 Files selected for processing (7)
data/test.db-shmsrc/lib/__tests__/db-module.test.tssrc/lib/__tests__/validation.test.tssrc/lib/websocket/__tests__/session-ws.test.tssrc/services/__tests__/config-service.test.tssrc/services/__tests__/run-script-manager.test.tssrc/services/adapters/__tests__/host-adapter.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/lib/websocket/__tests__/terminal-ws.test.ts (1)
329-334:⚠️ Potential issue | 🟡 Minor
messageHandler未登録時のフォールバックで偽陽性になりますLine 329-334 はハンドラー登録失敗でも
createPTY呼び出しだけでテストが通るため、resize 転送の本来の検証を満たしません。ハンドラー未登録は即 fail にしてください。💡 修正案
- if (messageHandler) { - messageHandler(Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); - expect(mockPtyManager.resize).toHaveBeenCalledWith('session-1-terminal', 100, 50); - } else { - expect(mockPtyManager.createPTY).toHaveBeenCalled(); - } + expect(messageHandler).not.toBeNull(); + messageHandler!(Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); + expect(mockPtyManager.resize).toHaveBeenCalledWith('session-1-terminal', 100, 50);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 329 - 334, The test currently allows a false positive when messageHandler is undefined; change the assertion so that if messageHandler is not set the test explicitly fails instead of only checking mockPtyManager.createPTY. Locate the test block referencing messageHandler and replace the else branch to call fail() (or throw) with a descriptive message, ensuring the resize behavior is only validated when messageHandler exists and that missing handler causes the test to fail; keep references to messageHandler, mockPtyManager.resize and mockPtyManager.createPTY so the correct assertions remain in the resize branch.src/services/proxy-client.ts (1)
137-145:⚠️ Potential issue | 🟡 Minor公開メソッドの
@throws契約が実装とズレています。
setRules()は 422 以外の 4xx でもProxyValidationErrorを投げますし、deleteRules()/deleteAllRules()も 4xx で同じ例外を返します。いまのJSDocだと呼び出し側がProxyConnectionErrorしか想定できず、4xx 分岐を見落としやすいです。コメントを実装に合わせるか、422 とその他 4xx を別エラーに分けて契約を明確にしたいです。📝 JSDoc を実装に合わせるならこんな形です
/** * 指定した送信元IPのルールセットを丸ごと置換する（リトライあり） * * `@param` sourceIP - コンテナの送信元IPアドレス * `@param` entries - 許可するホスト一覧 * `@returns` ProxyRuleSet - * `@throws` ProxyValidationError ルール形式が不正な場合（422） + * `@throws` ProxyValidationError 4xx クライアントエラーの場合 * `@throws` ProxyConnectionError proxyに接続できない場合 */ ... /** * 指定した送信元IPのルールを削除する（リトライあり） * * `@param` sourceIP - コンテナの送信元IPアドレス + * `@throws` ProxyValidationError 4xx クライアントエラーの場合 * `@throws` ProxyConnectionError proxyに接続できない場合 */ ... /** * proxyの全ルールを削除する（リトライあり） * + * `@throws` ProxyValidationError 4xx クライアントエラーの場合 * `@throws` ProxyConnectionError proxyに接続できない場合 */Also applies to: 194-199, 228-232
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/proxy-client.ts` around lines 137 - 145, Update the JSDoc for the public methods setRules, deleteRules, and deleteAllRules to match implementation: either (A) document that any 4xx HTTP response (not just 422) will throw ProxyValidationError and reserve ProxyConnectionError for network/5xx cases, or (B) change the implementation to throw a distinct error for non-422 4xx and keep the JSDoc as-is; pick one approach and make the JSDoc and thrown exception types consistent (refer to the symbols setRules, deleteRules, deleteAllRules, ProxyValidationError, ProxyConnectionError to locate where to edit).
🧹 Nitpick comments (4)
src/lib/__tests__/docker-volume-utils.test.ts (1)
46-51: 期待値の明示的な検証を追加することを検討してください。空白が含まれないことの検証は正しいですが、トリム後の正確な出力値も同時に検証することで、テストがより堅牢になります。
♻️ 提案する改善
it('environmentId由来の空白を含むVolume名を生成しない', () => { const result = getConfigVolumeNames(' spaced '); // Docker Volume名に空白が含まれないことを検証 expect(result.claudeVolume).not.toContain(' '); expect(result.configClaudeVolume).not.toContain(' '); + // トリム後の正確な値も検証 + expect(result.claudeVolume).toBe('claude-config-claude-spaced'); + expect(result.configClaudeVolume).toBe('claude-config-configclaude-spaced'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/docker-volume-utils.test.ts` around lines 46 - 51, The test currently only asserts that getConfigVolumeNames(' spaced ') returns volume names without spaces; update the test to also assert the exact trimmed outputs by computing the expected base (e.g., trimmedEnv = 'spaced') and checking result.claudeVolume and result.configClaudeVolume equal the exact expected strings (use the same naming scheme/formatting logic as getConfigVolumeNames) so the test verifies both absence of spaces and the precise returned values..github/workflows/test.yml (2)
238-254: スペースや特殊文字を含むファイルパスのエッジケースに注意
git diff --name-onlyの出力をtr '\n' ','で処理していますが、ファイルパスにスペースや特殊文字が含まれる場合、Stryker の--mutate引数が正しく解析されない可能性があります。このリポジトリのファイル命名規則でそのようなパスが存在しないことが前提であれば問題ありませんが、防御的に考慮する価値はあります。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 238 - 254, The current pipeline uses git diff --name-only and tr '\n' ',' which breaks when paths contain spaces/special chars; switch to NUL-separated handling by using git diff -z --name-only and update the filters to their NUL-capable variants (use grep -z -v '__tests__' and grep -z -v '\.test\.ts$' / '\.spec\.ts$' or use printf + awk -v RS='\0'), then build MUTATE_TARGETS from CHANGED_FILES with tr '\0' ',' (and trim the trailing comma) so CHANGED_FILES and MUTATE_TARGETS correctly preserve file names with spaces/special characters.
268-275: 実際のミューテーションスコアの表示を検討現在のサマリーは入力パラメータ（対象ファイルと閾値）のみを出力しています。Stryker の実行結果（実際のスコアや killed/survived mutants 数）を追加すると、PR レビュー時により有用な情報を提供できます。
Stryker は JSON レポーターで結果を出力でき、それを解析してサマリーに追加することが可能です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 268 - 275, Update the "Report mutation score to summary" step to read the Stryker JSON report (e.g., the mutation report file produced by the mutation test job) and append actual results (overall score, killed/survived mutant counts and a link to the full report) to $GITHUB_STEP_SUMMARY instead of only echoing inputs; specifically, in the step named "Report mutation score to summary" use a small shell snippet to check for the Stryker report file (the mutation JSON from the mutation test run), parse fields like "metrics" or "mutationScore" and "killed"/"survived" from that JSON, and then echo those values along with the existing Target files and Threshold to $GITHUB_STEP_SUMMARY, handling the case the report is missing by printing a clear message.src/services/__tests__/docker-pty-stream.test.ts (1)
54-59:DockerPTYStream生成の重複はヘルパー化すると保守しやすくなります同一初期化コードが多いため、共通化するとテスト追加時の変更コストを下げられます。
♻️ 例: 共通ファクトリの導入
+const createContainerPty = () => + new DockerPTYStream({ + cols: 80, + rows: 24, + isContainer: true, + container: mockContainer as any, + }); + +const createExecPty = () => + new DockerPTYStream({ + cols: 80, + rows: 24, + isContainer: false, + exec: mockExec as any, + }); - stream = new DockerPTYStream({ - cols: 80, - rows: 24, - isContainer: true, - container: mockContainer as any, - }); + stream = createContainerPty();Also applies to: 71-76, 85-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-pty-stream.test.ts` around lines 54 - 59, There are repeated DockerPTYStream initializations; create a shared factory/helper function (e.g., makeDockerPTYStream or createTestStream) that constructs and returns new DockerPTYStream instances using the common options (cols: 80, rows: 24, isContainer: true, container: mockContainer) and replace duplicate constructions (the stream variable assignments and other occurrences around the DockerPTYStream instantiations) with calls to that helper to DRY the tests and simplify future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 247-250: The test "should handle PTY creation error" sets
mockPtyManager.createPTY using mockImplementation which persists across tests
and isn't undone by vi.clearAllMocks(); change that to mockImplementationOnce
(or otherwise reset the mock after the test, e.g.,
mockPtyManager.createPTY.mockReset()) so the failure behavior is only applied
for this test; update the call site where mockPtyManager.createPTY is currently
set to throw to use mockImplementationOnce(() => { throw new Error('PTY
failed'); }) (or add a cleanup reset) to avoid leaking the failing
implementation into subsequent tests.
In `@src/services/adapters/__tests__/docker-adapter.test.ts`:
- Around line 431-453: The test only asserts that
destroySession('session-cleanup-fail') does not throw but doesn't verify the
container was actually stopped; update the test to assert that the container
stop was called when deleteRules() (mockProxyDeleteRules) fails: keep
mockDockerClient.getContainer returning an object with a mocked stop method,
store that stop mock (e.g., stopMock) and after awaiting
adapter.destroySession('session-cleanup-fail') add an explicit expectation that
stopMock was called (or that getContainer was called and its returned object's
stop method was invoked) so the behavior "コンテナ停止は継続する" is enforced.
- Around line 360-366:
テストがヘルスチェック失敗で「起動前に中断した」ことを保証していないので、mockProxyHealthCheckを拒否させた上で
adapter.createSession('session-1','/workspace')
が例外になるだけでなく、コンテナ生成/起動の呼び出しが行われていないことを検証してください：mockProxyHealthCheck.mockRejectedValue(...)
のまま await expect(...).rejects.toThrow() を行い、その後に setupContainerMock がセットするモック（例:
mockCreateContainer または Docker の createContainer 呼び出しをラップしたモック）とコンテナの start
メソッドモック（例: mockContainerStart）が一度も呼ばれていないことを expect(...).not.toHaveBeenCalled()
で確認するように修正してください（参照するシンボル：mockProxyHealthCheck, adapter.createSession,
setupContainerMock, mockCreateContainer, mockContainerStart）。
---
Duplicate comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 329-334: The test currently allows a false positive when
messageHandler is undefined; change the assertion so that if messageHandler is
not set the test explicitly fails instead of only checking
mockPtyManager.createPTY. Locate the test block referencing messageHandler and
replace the else branch to call fail() (or throw) with a descriptive message,
ensuring the resize behavior is only validated when messageHandler exists and
that missing handler causes the test to fail; keep references to messageHandler,
mockPtyManager.resize and mockPtyManager.createPTY so the correct assertions
remain in the resize branch.
In `@src/services/proxy-client.ts`:
- Around line 137-145: Update the JSDoc for the public methods setRules,
deleteRules, and deleteAllRules to match implementation: either (A) document
that any 4xx HTTP response (not just 422) will throw ProxyValidationError and
reserve ProxyConnectionError for network/5xx cases, or (B) change the
implementation to throw a distinct error for non-422 4xx and keep the JSDoc
as-is; pick one approach and make the JSDoc and thrown exception types
consistent (refer to the symbols setRules, deleteRules, deleteAllRules,
ProxyValidationError, ProxyConnectionError to locate where to edit).
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 238-254: The current pipeline uses git diff --name-only and tr
'\n' ',' which breaks when paths contain spaces/special chars; switch to
NUL-separated handling by using git diff -z --name-only and update the filters
to their NUL-capable variants (use grep -z -v '__tests__' and grep -z -v
'\.test\.ts$' / '\.spec\.ts$' or use printf + awk -v RS='\0'), then build
MUTATE_TARGETS from CHANGED_FILES with tr '\0' ',' (and trim the trailing comma)
so CHANGED_FILES and MUTATE_TARGETS correctly preserve file names with
spaces/special characters.
- Around line 268-275: Update the "Report mutation score to summary" step to
read the Stryker JSON report (e.g., the mutation report file produced by the
mutation test job) and append actual results (overall score, killed/survived
mutant counts and a link to the full report) to $GITHUB_STEP_SUMMARY instead of
only echoing inputs; specifically, in the step named "Report mutation score to
summary" use a small shell snippet to check for the Stryker report file (the
mutation JSON from the mutation test run), parse fields like "metrics" or
"mutationScore" and "killed"/"survived" from that JSON, and then echo those
values along with the existing Target files and Threshold to
$GITHUB_STEP_SUMMARY, handling the case the report is missing by printing a
clear message.
In `@src/lib/__tests__/docker-volume-utils.test.ts`:
- Around line 46-51: The test currently only asserts that getConfigVolumeNames('
spaced ') returns volume names without spaces; update the test to also assert
the exact trimmed outputs by computing the expected base (e.g., trimmedEnv =
'spaced') and checking result.claudeVolume and result.configClaudeVolume equal
the exact expected strings (use the same naming scheme/formatting logic as
getConfigVolumeNames) so the test verifies both absence of spaces and the
precise returned values.
In `@src/services/__tests__/docker-pty-stream.test.ts`:
- Around line 54-59: There are repeated DockerPTYStream initializations; create
a shared factory/helper function (e.g., makeDockerPTYStream or createTestStream)
that constructs and returns new DockerPTYStream instances using the common
options (cols: 80, rows: 24, isContainer: true, container: mockContainer) and
replace duplicate constructions (the stream variable assignments and other
occurrences around the DockerPTYStream instantiations) with calls to that helper
to DRY the tests and simplify future changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2cfd1388-d754-4842-b549-ad8c40e9b442
📒 Files selected for processing (13)
.github/workflows/test.ymlsrc/lib/__tests__/docker-volume-utils.test.tssrc/lib/__tests__/validation.test.tssrc/lib/docker-volume-utils.tssrc/lib/validation.tssrc/lib/websocket/__tests__/terminal-ws.test.tssrc/services/__tests__/docker-pty-stream.test.tssrc/services/__tests__/gh-cli.test.tssrc/services/__tests__/pty-manager.test.tssrc/services/__tests__/remote-repo-service.test.tssrc/services/adapters/__tests__/docker-adapter.test.tssrc/services/proxy-client.tsstryker.config.json
🚧 Files skipped from review as they are similar to previous changes (4)
- stryker.config.json
- src/services/tests/pty-manager.test.ts
- src/lib/validation.ts
- src/lib/tests/validation.test.ts
Comment on lines
+360
to
+366
| it('proxyヘルスチェック失敗時はセッション作成がエラーになる（フェイルセーフ）', async () => { | ||
| mockIsFilterEnabled.mockResolvedValue(true); | ||
| mockProxyHealthCheck.mockRejectedValue(new Error('proxy unreachable')); | ||
| setupContainerMock('172.20.0.5'); | ||
|
|
||
| await expect(adapter.createSession('session-1', '/workspace')).rejects.toThrow(); | ||
| }); |
There was a problem hiding this comment.
このテストだけでは「フェイルセーフ」を固定できていません。
rejects.toThrow() だけだと、ヘルスチェック失敗ではなく「コンテナ作成後に別要因で落ちる」退行でも通ってしまいます。ここは createContainer / start が未実行であることまで見て、起動前に中断したことを固定したいです。
✅ 例えばこうすると意図が明確になります
it('proxyヘルスチェック失敗時はセッション作成がエラーになる（フェイルセーフ）', async () => {
mockIsFilterEnabled.mockResolvedValue(true);
mockProxyHealthCheck.mockRejectedValue(new Error('proxy unreachable'));
- setupContainerMock('172.20.0.5');
+ const mockContainer = setupContainerMock('172.20.0.5');
await expect(adapter.createSession('session-1', '/workspace')).rejects.toThrow();
+ expect(mockDockerClient.createContainer).not.toHaveBeenCalled();
+ expect(mockContainer.start).not.toHaveBeenCalled();
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/adapters/__tests__/docker-adapter.test.ts` around lines 360 -
366, テストがヘルスチェック失敗で「起動前に中断した」ことを保証していないので、mockProxyHealthCheckを拒否させた上で
adapter.createSession('session-1','/workspace')
が例外になるだけでなく、コンテナ生成/起動の呼び出しが行われていないことを検証してください：mockProxyHealthCheck.mockRejectedValue(...)
のまま await expect(...).rejects.toThrow() を行い、その後に setupContainerMock がセットするモック（例:
mockCreateContainer または Docker の createContainer 呼び出しをラップしたモック）とコンテナの start
メソッドモック（例: mockContainerStart）が一度も呼ばれていないことを expect(...).not.toHaveBeenCalled()
で確認するように修正してください（参照するシンボル：mockProxyHealthCheck, adapter.createSession,
setupContainerMock, mockCreateContainer, mockContainerStart）。
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/websocket/__tests__/terminal-ws.test.ts (1)
329-334:⚠️ Potential issue | 🟡 Minor
messageHandler未登録時にテストが通ってしまう分岐を削除してください。Line 329 の
if (messageHandler) ... else ...は、ハンドラー登録失敗を見逃して false positive になります。ここは常に登録を必須化して検証した方が安全です。修正例
- if (messageHandler) { - messageHandler(Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); - expect(mockPtyManager.resize).toHaveBeenCalledWith('session-1-terminal', 100, 50); - } else { - expect(mockPtyManager.createPTY).toHaveBeenCalled(); - } + expect(messageHandler).not.toBeNull(); + messageHandler!(Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); + expect(mockPtyManager.resize).toHaveBeenCalledWith('session-1-terminal', 100, 50);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 329 - 334, Remove the conditional branch that allows the test to pass when messageHandler is not registered; instead require and assert registration, then invoke it and verify resize. Specifically, in the test referencing messageHandler and mockPtyManager (the block that currently does "if (messageHandler) { ... } else { ... }"), delete the else branch, add an assertion like expect(messageHandler).toBeDefined() (or equivalent) before invoking messageHandler(Buffer.from(...)), and then assert mockPtyManager.resize was called with 'session-1-terminal', 100, 50; do not fall back to checking createPTY.
🧹 Nitpick comments (2)
src/services/adapters/__tests__/docker-adapter.test.ts (1)
360-368: フェイルセーフ検証をもう一段強化できます現状でも cleanup は検証できていますが、
healthCheck失敗時にsyncRulesForContainerが呼ばれないことを明示すると、意図しない順序変更の回帰を防ぎやすくなります。💡 追記例
it('proxyヘルスチェック失敗時はセッション作成がエラーになる（フェイルセーフ）', async () => { mockIsFilterEnabled.mockResolvedValue(true); mockProxyHealthCheck.mockRejectedValue(new Error('proxy unreachable')); const mockContainer = setupContainerMock('172.20.0.5'); await expect(adapter.createSession('session-1', '/workspace')).rejects.toThrow(); // コンテナはhealthCheckの前に作成されるが、失敗時にクリーンアップされる expect(mockContainer.remove).toHaveBeenCalledWith({ force: true }); + expect(mockSyncRulesForContainer).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/docker-adapter.test.ts` around lines 360 - 368, The test currently verifies cleanup on proxy healthCheck failure but doesn't assert that syncRulesForContainer is skipped; update the failing-health-check test (the case using mockProxyHealthCheck rejecting and adapter.createSession) to also assert that syncRulesForContainer (the mocked function used in the adapter) was not called, e.g. add an expectation like expect(mockSyncRulesForContainer).not.toHaveBeenCalled() after the rejects assertion and before/after checking mockContainer.remove to prevent regressions that might call syncRulesForContainer when health checks fail.src/lib/websocket/__tests__/terminal-ws.test.ts (1)
290-292: 重要経路の検証がit.skipのまま残っています。Line 290 と Line 399 の skip は、
input転送とcloseクリーンアップの回帰を検出できません。非同期安定化（ws.emit('message'/'close')ベースへ寄せる等）で通常テスト化を推奨します。必要なら、skip を外せる形にテストを書き換える差分をこちらで作成します。対応しますか？
Also applies to: 399-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 290 - 292, Un-skip the two tests ('should forward input to ptyManager (via connection handler)' and the close/cleanup test) but make their async setup deterministic: ensure the connection handler's addConnection has completed before emitting events by either (a) calling ws.emit('connection', socket) / ws.emit('message' / 'close') directly instead of relying on external async callbacks, or (b) awaiting a promise that resolves when addConnection finishes (e.g. wrap the connection registration callback and await it), then emit ws.emit('message', ...) to test input forwarding to ptyManager and ws.emit('close') to test cleanup; reference addConnection, ptyManager, and ws.emit('message'/'close') when updating the tests so they wait for handler registration rather than racing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 329-334: Remove the conditional branch that allows the test to
pass when messageHandler is not registered; instead require and assert
registration, then invoke it and verify resize. Specifically, in the test
referencing messageHandler and mockPtyManager (the block that currently does "if
(messageHandler) { ... } else { ... }"), delete the else branch, add an
assertion like expect(messageHandler).toBeDefined() (or equivalent) before
invoking messageHandler(Buffer.from(...)), and then assert mockPtyManager.resize
was called with 'session-1-terminal', 100, 50; do not fall back to checking
createPTY.
---
Nitpick comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 290-292: Un-skip the two tests ('should forward input to
ptyManager (via connection handler)' and the close/cleanup test) but make their
async setup deterministic: ensure the connection handler's addConnection has
completed before emitting events by either (a) calling ws.emit('connection',
socket) / ws.emit('message' / 'close') directly instead of relying on external
async callbacks, or (b) awaiting a promise that resolves when addConnection
finishes (e.g. wrap the connection registration callback and await it), then
emit ws.emit('message', ...) to test input forwarding to ptyManager and
ws.emit('close') to test cleanup; reference addConnection, ptyManager, and
ws.emit('message'/'close') when updating the tests so they wait for handler
registration rather than racing.
In `@src/services/adapters/__tests__/docker-adapter.test.ts`:
- Around line 360-368: The test currently verifies cleanup on proxy healthCheck
failure but doesn't assert that syncRulesForContainer is skipped; update the
failing-health-check test (the case using mockProxyHealthCheck rejecting and
adapter.createSession) to also assert that syncRulesForContainer (the mocked
function used in the adapter) was not called, e.g. add an expectation like
expect(mockSyncRulesForContainer).not.toHaveBeenCalled() after the rejects
assertion and before/after checking mockContainer.remove to prevent regressions
that might call syncRulesForContainer when health checks fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82447621-2357-4b11-b722-7e79d4095305
📒 Files selected for processing (2)
src/lib/websocket/__tests__/terminal-ws.test.tssrc/services/adapters/__tests__/docker-adapter.test.ts
There was a problem hiding this comment.
Pull request overview
Stryker（vitest-runner）によるミューテーションテスト基盤を追加し、CIでの品質ゲート（85%）を導入するとともに、テスト容易性/安全性のための周辺リファクタ（proxy同期・パス安全性・Git操作タイムアウト等）とユニットテスト拡充を行うPRです。
Changes:
- Stryker設定（
stryker.config.json,vitest.config.stryker.ts）とtest:mutationスクリプト、変更ファイルのみを対象にしたGitHub Actionsジョブを追加 - network-filter-proxy向け
ProxyClientとルール同期ヘルパーを追加し、DockerAdapter にフィルタリング時のネットワーク/プロキシ設定・ルール同期・クリーンアップを実装 path-safety/validation等の小規模リファクタと、大量のユニットテスト追加・改善
Reviewed changes
Copilot reviewed 51 out of 54 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
vitest.config.stryker.ts |
Stryker実行用のVitest設定を追加 |
stryker.config.json |
mutate対象/しきい値/レポータ等のStryker設定を追加 |
src/services/remote-repo-service.ts |
clone/pullの安全性向上（パス検証、タイムアウト、--付与等） |
src/services/proxy-client.ts |
proxy管理API用HTTPクライアントを新規追加 |
src/services/environment-adapter.ts |
CreateSessionOptions に filterEnabled を追加 |
src/services/adapters/docker-adapter.ts |
フィルタリング時のNetworkMode/Proxy env設定、ルール同期、終了時クリーンアップ等を追加 |
src/services/adapters/__tests__/host-adapter.test.ts |
HostAdapterのイベント転送/各メソッド挙動のテスト拡充 |
src/services/adapters/__tests__/docker-adapter.test.ts |
DockerAdapterのフィルタリング関連挙動のテスト追加 |
src/services/adapters/__tests__/base-adapter.test.ts |
BasePTYAdapterの引数サニタイズ等のテスト拡充 |
src/services/adapter-factory.ts |
環境IDからキャッシュ済みDockerAdapterを取得するAPIを追加 |
src/services/__tests__/scrollback-buffer.test.ts |
バッファ境界/UTF-8境界/コンパクション等のテスト追加 |
src/services/__tests__/run-script-manager.test.ts |
stop時のSIGTERM/SIGKILL・spawn error等のテスト追加 |
src/services/__tests__/pty-manager.test.ts |
PTYManagerのガード/環境変数/エラー処理等のテスト拡充 |
src/services/__tests__/port-checker.test.ts |
config不正/判定優先順位等のテスト追加 |
src/services/__tests__/github-pat-service.test.ts |
insert/update内容や暗号化失敗時のメッセージ等のテスト追加 |
src/services/__tests__/git-operations.test.ts |
GitOperationError のユニットテスト新規追加 |
src/services/__tests__/gh-cli.test.ts |
gh CLIラッパのモック/エラー分岐含むテスト拡充 |
src/services/__tests__/environment-adapter.test.ts |
type guardの境界条件テスト追加 |
src/services/__tests__/encryption-service.test.ts |
エラークラス/不正キー/復号エラー等のテスト追加 |
src/services/__tests__/docker-git-service.test.ts |
clone/worktree/volume等の挙動・エラー系テスト拡充 |
src/services/__tests__/docker-client.test.ts |
DockerClientの各API/エラー処理/ログ等のテスト拡充 |
src/services/__tests__/developer-settings-service.test.ts |
SettingsNotFoundError等のテスト追加 |
src/services/__tests__/config-service.test.ts |
singleton初期化/ログ/エラー系等のテスト拡充 |
src/services/__tests__/adapter-factory.test.ts |
追加API含むAdapterFactoryのテスト/ログ検証を拡充 |
src/lib/websocket/__tests__/session-ws.test.ts |
WSハンドラのイベント転送/DB失敗時等のテスト拡充 |
src/lib/volume-naming.ts |
slug生成処理の変更（末尾/先頭処理の分割） |
src/lib/validation.ts |
isValidEmail を追加 |
src/lib/proxy-sync.ts |
proxyルール同期ヘルパー（dynamic import含む）を新規追加 |
src/lib/path-safety.ts |
パス正規化/ベース配下判定/コンポーネント検証を新規追加 |
src/lib/docker-volume-utils.ts |
Volume名生成時に environmentId.trim() を使用 |
src/lib/__tests__/validation.test.ts |
PAT/URL/email等の境界・ミューテーション対策テスト追加 |
src/lib/__tests__/session.test.ts |
sessionオプションの環境依存挙動テスト追加 |
src/lib/__tests__/session-name-generator.test.ts |
乱数選択/リトライ境界のテスト追加 |
src/lib/__tests__/schema-check.test.ts |
schema整合性チェックの追加テスト |
src/lib/__tests__/run-scripts.test.ts |
run-scripts parse/serializeのテスト新規追加 |
src/lib/__tests__/proxy-sync.test.ts |
proxy-syncのユニットテスト新規追加 |
src/lib/__tests__/path-safety.test.ts |
path-safetyのユニットテスト新規追加 |
src/lib/__tests__/notification-service.test.ts |
localStorage/AudioContext等の分岐テスト追加 |
src/lib/__tests__/errors.test.ts |
API error handlingのログ/分岐テスト追加 |
src/lib/__tests__/env-validation.test.ts |
環境変数検証のメッセージ/分岐テスト追加 |
src/lib/__tests__/docker-volume-utils.test.ts |
docker-volume-utilsのユニットテスト新規追加 |
src/lib/__tests__/docker-config-validator.test.ts |
port/volume validatorの境界・メッセージテスト追加 |
src/lib/__tests__/db-module.test.ts |
dbモジュールのロード時挙動をテスト |
src/lib/__tests__/action-detector.test.ts |
ANSI除去/アクション検出/クールダウン境界のテスト追加 |
package.json |
test:mutation スクリプトとStryker依存を追加 |
.gitignore |
Stryker生成物（reports/.stryker-tmp）を追加 |
.github/workflows/test.yml |
PR時に変更ファイルのみを対象にしたmutation-testジョブを追加 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
417
to
+425
| return new Promise((resolve) => { | ||
| let settled = false; | ||
| const finish = (result: PullResult) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| clearTimeout(fetchTimeout); | ||
| clearTimeout(pullTimeout); | ||
| resolve(result); | ||
| }; |
Comment on lines
72
to
76
| }); | ||
|
|
||
| afterEach(() => { | ||
| hostAdapter.removeAllListeners(); | ||
| }); |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/services/adapters/__tests__/host-adapter.test.ts (2)
101-110:⚠️ Potential issue | 🟡 Minorテスト対象のセッションIDが不正です
このテストは「非シェルセッションではデータをフォワードしない」ことを検証する目的ですが、
unknown-sessionをemitしているため、作成済みのclaude-1セッションに対する挙動を検証できていません。🔧 修正案
it('should not forward ptyManager data events for non-shell sessions', () => { hostAdapter.createSession('claude-1', '/path'); const dataHandler = vi.fn(); hostAdapter.on('data', dataHandler); - // Emit data from ptyManager for a different session - mockPtyManagerObj.emit('data', 'unknown-session', 'output data'); + // Emit data for existing non-shell session (Claude mode) + mockPtyManagerObj.emit('data', 'claude-1', 'output data'); expect(dataHandler).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/host-adapter.test.ts` around lines 101 - 110, The test emits ptyManager data for the wrong session id so it never exercises the created session; change the emitted session id to the one created by the test (use the same 'claude-1' from the hostAdapter.createSession call) so mockPtyManagerObj.emit('data', ...) targets the created non-shell session and the assertion that dataHandler was not called is meaningful; update the emit invocation in the test accordingly (referencing hostAdapter.createSession and mockPtyManagerObj.emit).
393-402:⚠️ Potential issue | 🟡 Minor失敗系テストにアサーションが不足しています
このテストは「destroySession が失敗してもエラーがスローされない」ことを検証する意図ですが、実質的なアサーションがありません。明示的に検証してください。
🔧 修正案
it('should handle async destroySession Promise rejection', async () => { // Create a session hostAdapter.createSession('session-1', '/path/to/work'); // Make cleanupPTY (kill) throw (mockPtyInstance.kill as any).mockImplementation(() => { throw new Error('kill failed'); }); // Re-create should try to destroy first, and handle the error - hostAdapter.createSession('session-1', '/path/to/work2'); - // Should not throw + expect(() => hostAdapter.createSession('session-1', '/path/to/work2')).not.toThrow(); + expect(hostAdapter.hasSession('session-1')).toBe(true); + expect(hostAdapter.getWorkingDir('session-1')).toBe('/path/to/work2'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/host-adapter.test.ts` around lines 393 - 402, The test lacks assertions verifying that re-creating a session does not throw when destroySession (async) rejects; update the test to explicitly assert that calling hostAdapter.createSession('session-1', '/path/to/work2') does not throw (use expect(() => hostAdapter.createSession(...)).not.toThrow() if createSession is synchronous, or await expect(hostAdapter.createSession(...)).resolves.toBeUndefined() if it returns a Promise) and also assert that mockPtyInstance.kill (or the spy used to simulate failure) was invoked so you confirm destroySession was attempted and its rejection was handled.
🧹 Nitpick comments (3)
src/services/adapters/__tests__/host-adapter.test.ts (1)
80-87: リスナー数のアサーションをより厳密にすることを推奨
afterEachでリスナーをクリアしているため、各テスト実行時のリスナー数は正確に1であるはずです。toBeGreaterThan(0)ではなくtoBe(1)を使用することで、意図しないリスナー登録を検出できます。♻️ 修正案
it('should register data event handler on ptyManager', () => { // ptyManager emitter should have 'data' listener registered - expect(mockPtyManagerObj.listenerCount('data')).toBeGreaterThan(0); + expect(mockPtyManagerObj.listenerCount('data')).toBe(1); }); it('should register exit event handler on ptyManager', () => { - expect(mockPtyManagerObj.listenerCount('exit')).toBeGreaterThan(0); + expect(mockPtyManagerObj.listenerCount('exit')).toBe(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/host-adapter.test.ts` around lines 80 - 87, The tests currently assert listenerCount('data') and listenerCount('exit') with toBeGreaterThan(0); tighten these to exact counts by asserting toBe(1) so unintended extra listeners are caught. Update the two assertions in the test cases that reference mockPtyManagerObj.listenerCount('data') and mockPtyManagerObj.listenerCount('exit') to expect 1, relying on the afterEach listener cleanup to guarantee a single registered handler.src/services/remote-repo-service.ts (2)
576-576: 一貫性のため、safePathを使用すべきです。
getDefaultBranchに元のrepoPathを渡していますが、他の操作ではsafePathを使用しています。getDefaultBranch内で再度sanitizePathが呼ばれるため機能的には問題ありませんが、コードの一貫性のためsafePathを渡すことを推奨します。♻️ 提案する修正
// デフォルトブランチを取得 - const defaultBranch = await this.getDefaultBranch(repoPath); + const defaultBranch = await this.getDefaultBranch(safePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/remote-repo-service.ts` at line 576, The call to getDefaultBranch currently passes the original repoPath; for consistency with other operations use the already-sanitized safePath variable instead. Update the invocation from getDefaultBranch(repoPath) to getDefaultBranch(safePath) (noting getDefaultBranch still calls sanitizePath internally) so code consistently uses safePath across remote-repo-service operations.
337-353: パス生成ロジックは正しく動作しますが、軽微な最適化の余地があります。
sanitizePath(baseDir)は既に呼び出し元で実行されているケースがありますが、防御的プログラミングとして許容範囲です。ループ内でのsanitizePath呼び出しは毎回実行されますが、実用上問題になることはないでしょう。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/remote-repo-service.ts` around lines 337 - 353, In getUniqueClonePath, avoid repeatedly calling sanitizePath inside the loop: keep sanitizePath(safeBase) once (safeBase already computed), build candidate paths with join(safeBase, repoName) and join(safeBase, `${repoName}-${counter}`) and only call sanitizePath once when returning or when you first create a candidate to check existsSync; update symbols: getUniqueClonePath, safeBase, clonePath, repoName, counter, sanitizePath, join, existsSync so the loop uses unsanitized joins for existence checks and performs a single sanitizePath on the final clonePath before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/services/adapters/__tests__/host-adapter.test.ts`:
- Around line 101-110: The test emits ptyManager data for the wrong session id
so it never exercises the created session; change the emitted session id to the
one created by the test (use the same 'claude-1' from the
hostAdapter.createSession call) so mockPtyManagerObj.emit('data', ...) targets
the created non-shell session and the assertion that dataHandler was not called
is meaningful; update the emit invocation in the test accordingly (referencing
hostAdapter.createSession and mockPtyManagerObj.emit).
- Around line 393-402: The test lacks assertions verifying that re-creating a
session does not throw when destroySession (async) rejects; update the test to
explicitly assert that calling hostAdapter.createSession('session-1',
'/path/to/work2') does not throw (use expect(() =>
hostAdapter.createSession(...)).not.toThrow() if createSession is synchronous,
or await expect(hostAdapter.createSession(...)).resolves.toBeUndefined() if it
returns a Promise) and also assert that mockPtyInstance.kill (or the spy used to
simulate failure) was invoked so you confirm destroySession was attempted and
its rejection was handled.
---
Nitpick comments:
In `@src/services/adapters/__tests__/host-adapter.test.ts`:
- Around line 80-87: The tests currently assert listenerCount('data') and
listenerCount('exit') with toBeGreaterThan(0); tighten these to exact counts by
asserting toBe(1) so unintended extra listeners are caught. Update the two
assertions in the test cases that reference
mockPtyManagerObj.listenerCount('data') and
mockPtyManagerObj.listenerCount('exit') to expect 1, relying on the afterEach
listener cleanup to guarantee a single registered handler.
In `@src/services/remote-repo-service.ts`:
- Line 576: The call to getDefaultBranch currently passes the original repoPath;
for consistency with other operations use the already-sanitized safePath
variable instead. Update the invocation from getDefaultBranch(repoPath) to
getDefaultBranch(safePath) (noting getDefaultBranch still calls sanitizePath
internally) so code consistently uses safePath across remote-repo-service
operations.
- Around line 337-353: In getUniqueClonePath, avoid repeatedly calling
sanitizePath inside the loop: keep sanitizePath(safeBase) once (safeBase already
computed), build candidate paths with join(safeBase, repoName) and
join(safeBase, `${repoName}-${counter}`) and only call sanitizePath once when
returning or when you first create a candidate to check existsSync; update
symbols: getUniqueClonePath, safeBase, clonePath, repoName, counter,
sanitizePath, join, existsSync so the loop uses unsanitized joins for existence
checks and performs a single sanitizePath on the final clonePath before
returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1700096a-5393-480b-a147-ba2e7a31b3bc
📒 Files selected for processing (2)
src/services/adapters/__tests__/host-adapter.test.tssrc/services/remote-repo-service.ts
windschord
force-pushed
the
test/mutation-testing-clean
branch
from
b456746 to
7346255
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Pull request overview
This PR introduces mutation testing (Stryker + Vitest runner) and strengthens unit tests across src/services/ and src/lib/ to improve mutation score and overall test robustness, while making a few small production-code tweaks for testability/safety.
Changes:
- Add Stryker configuration and a dedicated Vitest config for mutation runs; add
test:mutationscript and ignore Stryker artifacts. - Add a GitHub Actions
mutation-testjob that targets only PR-changed files and enforces an 85% mutation score gate. - Expand/strengthen unit tests across many services/libs; apply a few small correctness/safety fixes in production code (e.g., URL encoding, email validation edge cases, trimming IDs).
Reviewed changes
Copilot reviewed 49 out of 52 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | Adds a Stryker-specific Vitest configuration (timeouts, excludes, concurrency). |
| stryker.config.json | Defines mutation targets, reporters, and thresholds (break at 85%). |
| src/services/remote-repo-service.ts | Makes pullTimeout optional before clearing to avoid unsafe timeout handling. |
| src/services/proxy-client.ts | Encodes sourceIP in URL path segments to safely support non-trivial identifiers. |
| src/services/adapters/tests/host-adapter.test.ts | Expands HostAdapter unit tests and improves mocking strategy with hoisted mocks. |
| src/services/adapters/tests/docker-adapter.test.ts | Adds assertions around env restoration and cleanup behavior. |
| src/services/adapters/tests/base-adapter.test.ts | Adds tests for session ID extraction and argument redaction/logging paths. |
| src/services/tests/scrollback-buffer.test.ts | Adds extensive boundary/UTF-8/compaction/env-var coverage for scrollback buffer behavior. |
| src/services/tests/run-script-manager.test.ts | Adds coverage for stop/timeout/SIGKILL behavior, spawn error handling, and cleanup. |
| src/services/tests/remote-repo-service.test.ts | Adds many edge-case tests and restores env var state more safely. |
| src/services/tests/pty-manager.test.ts | Adds more validation/env filtering/concurrency-guard tests and cleans up env/listeners. |
| src/services/tests/proxy-client.test.ts | Adds retry/error parsing/error-class behavior tests for proxy client. |
| src/services/tests/port-checker.test.ts | Adds fallback/precedence tests for malformed env config and OS vs ClaudeWork checks. |
| src/services/tests/github-pat-service.test.ts | Adds insert/update argument assertions and richer error-path tests. |
| src/services/tests/git-operations.test.ts | Adds unit tests for GitOperationError construction and properties. |
| src/services/tests/gh-cli.test.ts | Adds child_process mocking and tests for gh availability/PR creation/status parsing. |
| src/services/tests/environment-adapter.test.ts | Adds negative/edge-case coverage for isEnvironmentAdapter. |
| src/services/tests/encryption-service.test.ts | Adds error-class tests and more detailed invalid-key/invalid-format coverage. |
| src/services/tests/docker-pty-adapter.test.ts | Greatly expands coverage for DockerPTYAdapter session lifecycle and error parsing. |
| src/services/tests/docker-git-service.test.ts | Adds broad behavior/error/logging coverage for Docker git operations. |
| src/services/tests/docker-client.test.ts | Adds extensive tests for DockerClient methods and error/log handling. |
| src/services/tests/developer-settings-service.test.ts | Adds tests for SettingsNotFoundError messages and delete error messaging. |
| src/services/tests/config-service.test.ts | Adds singleton/init/logging/save-merge/error-path tests for ConfigService. |
| src/services/tests/adapter-factory.test.ts | Adds logging assertions, cache accessor tests, and invalid-config handling tests. |
| src/lib/websocket/tests/session-ws.test.ts | Adds unit tests for websocket event forwarding and connection/message handling. |
| src/lib/validation.ts | Tightens email local-part validation (dot rules). |
| src/lib/docker-volume-utils.ts | Trims environmentId to avoid generating volume names with leading/trailing whitespace. |
| src/lib/tests/validation.test.ts | Adds boundary/mutation-oriented tests for validation utilities including email edge cases. |
| src/lib/tests/session.test.ts | Adds tests for session option behavior across NODE_ENV/SESSION_SECRET combinations. |
| src/lib/tests/session-name-generator.test.ts | Adds mutation-oriented tests around randomness and retry boundaries. |
| src/lib/tests/schema-check.test.ts | Adds schema-integrity and message-format tests using drizzle schema introspection. |
| src/lib/tests/run-scripts.test.ts | Adds tests for parsing/serializing run scripts with invalid inputs. |
| src/lib/tests/proxy-sync.test.ts | Adds coverage for undefined port handling, logging, and null container IPs. |
| src/lib/tests/path-safety.test.ts | Adds unit tests for path sanitization, base containment, and safe component checks. |
| src/lib/tests/notification-service.test.ts | Adds tests for defaults, invalid persisted settings, save failures, and sound cleanup. |
| src/lib/tests/errors.test.ts | Mocks logger and expands coverage for ApiError handling/logging/stack inclusion logic. |
| src/lib/tests/env-validation.test.ts | Expands tests for error-message content and CLAUDE_CODE_PATH resolution edge cases. |
| src/lib/tests/docker-volume-utils.test.ts | Adds coverage for environmentId validation and whitespace trimming in volume naming. |
| src/lib/tests/docker-config-validator.test.ts | Adds boundary/indexing/warnings/duplicate detection tests for port/volume validation. |
| src/lib/tests/db-module.test.ts | Adds module-level tests for db exports and basic query execution. |
| src/lib/tests/action-detector.test.ts | Adds mutation-oriented coverage for ANSI stripping and action request detection patterns. |
| src/app/api/environments/tests/route.test.ts | Removes network filter auto-apply tests and associated service mocking from env route tests. |
| src/app/api/environments/[id]/network-rules/tests/route.test.ts | Removes proxy sync mocking expectations from network-rules route tests. |
| src/app/api/environments/[id]/network-filter/tests/route.test.ts | Removes proxy sync mocking expectations from network-filter route tests. |
| src/app/api/docker/image-build/tests/route.test.ts | Updates expected validation error messages for docker image build API. |
| package.json | Adds Stryker dependencies and test:mutation script; removes @types/tar-fs. |
| .gitignore | Ignores Stryker temp output and mutation report directory. |
| .github/workflows/test.yml | Adds PR-only mutation testing job and wires it into the final CI check. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/test.yml
Outdated
Comment on lines
+238
to
+242
| CHANGED_FILES=$(git diff --name-only --diff-filter=ACMR origin/${{ github.base_ref }}...HEAD -- \ | ||
| 'src/services/**/*.ts' 'src/lib/**/*.ts' | \ | ||
| grep -v '__tests__' | \ | ||
| grep -v '\.test\.ts$' | \ | ||
| grep -v '\.spec\.ts$' || true) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 236-254: The git diff failure is being silenced by the "|| true"
which causes CHANGED_FILES to be empty and wrongly set has_targets=false; remove
the "|| true" and instead check the git diff command exit status (the git diff
invocation that sets CHANGED_FILES) and if it fails, emit an error message and
exit non-zero so the workflow fails; keep the rest of the logic that transforms
CHANGED_FILES into MUTATE_TARGETS unchanged but ensure you reference
CHANGED_FILES, the git diff command, and MUTATE_TARGETS when adding the failure
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03e10bff-9278-41f6-ace5-d965960f6f36
📒 Files selected for processing (3)
.github/workflows/test.yml.gitignoredata/test.db-shm
There was a problem hiding this comment.
Pull request overview
Stryker（vitest-runner）によるミューテーションテスト環境を導入し、CI で PR の変更ファイルに対してスコア閾値（85%）をゲートとして適用できるようにするPRです。あわせて src/services/ と src/lib/ を中心にテストの新規追加・拡充と、テスタビリティ向上のための軽微なコード修正が含まれます。
Changes:
- Stryker 設定（
stryker.config.json/vitest.config.stryker.ts）とtest:mutationスクリプトを追加 - GitHub Actions に PR 時のみ変更ファイル対象の mutation-test ジョブを追加し、
break: 85を品質ゲートとして適用 src/services/src/libの各種ユニットテストを追加・拡充し、いくつかの実装もテスタビリティ改善
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | Stryker 用の Vitest 設定を追加（実行環境・除外・DB等） |
| stryker.config.json | Stryker core/vitest-runner の設定、mutate 対象、閾値を定義 |
| src/services/remote-repo-service.ts | pull タイムアウトの後始末を安全にする小修正 |
| src/services/proxy-client.ts | ルールAPIの path パラメータを encodeURIComponent し、4xx/422 の説明を明確化 |
| src/services/adapters/tests/host-adapter.test.ts | HostAdapter のイベント転送/引数/分岐などのテスト拡充・モック改善 |
| src/services/adapters/tests/docker-adapter.test.ts | env復元や失敗時クリーンアップ等の期待値を追加 |
| src/services/adapters/tests/base-adapter.test.ts | ロガーモックの整理と、セッションID抽出/引数マスキング等のテスト追加 |
| src/services/tests/scrollback-buffer.test.ts | UTF-8境界、chunk compaction、env設定などの境界/分岐テストを拡充 |
| src/services/tests/run-script-manager.test.ts | stop の SIGTERM/SIGKILL・spawn error・exit cleanup 等のテストを拡充 |
| src/services/tests/remote-repo-service.test.ts | env復元の強化と、URL/clone/pull/branch等のエッジケーステスト追加 |
| src/services/tests/pty-manager.test.ts | env/リスナーのクリーンアップ強化と、作成/例外/許可ディレクトリ等のテスト追加 |
| src/services/tests/proxy-client.test.ts | 接続失敗/HTTPエラー/JSONパース失敗/リトライ等のテスト追加 |
| src/services/tests/port-checker.test.ts | 環境config異常時フォールバックや判定優先度などのテスト追加 |
| src/services/tests/github-pat-service.test.ts | DB insert/update 引数検証、暗号化/復号例外メッセージ等のテスト追加 |
| src/services/tests/git-operations.test.ts | GitOperationError のプロパティ/派生ケースのテスト新規追加 |
| src/services/tests/gh-cli.test.ts | child_process モック化し、gh 利用可否/PR作成/状態取得などをテスト拡充 |
| src/services/tests/environment-adapter.test.ts | isEnvironmentAdapter の欠落メソッド/型不正/非オブジェクト等のテスト追加 |
| src/services/tests/encryption-service.test.ts | Error class と鍵長/形式エラー等のテスト追加 |
| src/services/tests/docker-pty-adapter.test.ts | DockerPTYAdapter の挙動・エラー解析・env/安全設定などを大幅にテスト拡充 |
| src/services/tests/docker-git-service.test.ts | clone/worktree/volume/diff parsing 等のテスト追加とロガー検証 |
| src/services/tests/docker-client.test.ts | logger モック追加、API群の成功/失敗/進捗コールバック等のテスト拡充 |
| src/services/tests/developer-settings-service.test.ts | SettingsNotFoundError のメッセージ/名前等のテスト追加 |
| src/services/tests/config-service.test.ts | シングルトン/ログ/保存失敗/マージ等のテスト追加、loggerモック整理 |
| src/services/tests/adapter-factory.test.ts | ログ出力や invalid config JSON 等のケースを追加テスト |
| src/lib/websocket/tests/session-ws.test.ts | SessionWebSocketHandler の接続/DB取得/イベント転送等をユニットテスト化 |
| src/lib/validation.ts | email の local-part に関する追加バリデーション（先頭/末尾ドット、連続ドット） |
| src/lib/docker-volume-utils.ts | environmentId を trim して volume 名に反映 |
| src/lib/tests/validation.test.ts | validation の境界/アンカー等の mutation 対応テスト追加（※重複ブロックあり） |
| src/lib/tests/session.test.ts | sessionOptions と production 制約等のテスト新規追加 |
| src/lib/tests/session-name-generator.test.ts | Math.random と maxAttempts 境界のテスト追加 |
| src/lib/tests/schema-check.test.ts | schema 整合性の追加検証とフォーマット出力のテスト拡充 |
| src/lib/tests/run-scripts.test.ts | run-scripts の parse/serialize をテスト新規追加 |
| src/lib/tests/proxy-sync.test.ts | port省略やログ、containerIPs null 等の同期挙動をテスト追加 |
| src/lib/tests/path-safety.test.ts | sanitize/isWithinBase/isSafePathComponent のテスト新規追加 |
| src/lib/tests/notification-service.test.ts | localStorage 異常・AudioContext cleanup・通知タイトル等のテスト追加 |
| src/lib/tests/errors.test.ts | ApiError/unknown error のログとレスポンス生成のテスト追加 |
| src/lib/tests/env-validation.test.ts | DATABASE_URL/which解決/絶対パス等のエラーメッセージ検証を強化 |
| src/lib/tests/docker-volume-utils.test.ts | getConfigVolumeNames のテスト新規追加 |
| src/lib/tests/docker-config-validator.test.ts | portMappings/volumeMounts の index 表示や境界・warnings 等をテスト拡充 |
| src/lib/tests/db-module.test.ts | db モジュールのエクスポート/基本クエリ等のテスト新規追加 |
| src/lib/tests/action-detector.test.ts | ANSI除去・action検知・クールダウン境界のテスト追加 |
| src/app/api/environments/tests/route.test.ts | ネットワークフィルタ自動適用のテスト群を削除（スコープ調整） |
| src/app/api/environments/[id]/network-rules/tests/route.test.ts | proxy-sync 呼び出しを前提にしたテストを削除（スコープ調整） |
| src/app/api/environments/[id]/network-filter/tests/route.test.ts | proxy-sync 呼び出しを前提にしたテストを削除（スコープ調整） |
| package.json | Stryker 依存追加と test:mutation スクリプト追加 |
| .gitignore | Stryker の成果物（reports/mutation, .stryker-tmp）を除外 |
| .github/workflows/test.yml | mutation-test ジョブ追加、ci-check に組み込み（skipped許容） |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/lib/tests/validation.test.ts:347
isValidEmailのテストが同一ファイル内で2回describe('isValidEmail', ...)として定義されており、内容も重複しています。重複ブロックがあるとテスト実行時間と保守コストが増えるので、どちらか一方に統合（不足分のみ追記）してください。
describe('isValidEmail', () => {
it('有効なメールアドレスを受け入れる', () => {
expect(isValidEmail('user@example.com')).toBe(true);
expect(isValidEmail('user@sub.example.com')).toBe(true);
expect(isValidEmail('user+tag@example.com')).toBe(true);
});
it('ドメイン部分に連続ドットを含むアドレスを拒否する', () => {
expect(isValidEmail('user@foo..bar')).toBe(false);
});
it('ドメインがドットで始まるアドレスを拒否する', () => {
expect(isValidEmail('user@.foo')).toBe(false);
expect(isValidEmail('user@.foo.com')).toBe(false);
});
it('ドメインにTLDが無いアドレスを拒否する', () => {
expect(isValidEmail('user@localhost')).toBe(false);
});
it('@が無いアドレスを拒否する', () => {
expect(isValidEmail('userexample.com')).toBe(false);
});
it('空文字列を拒否する', () => {
expect(isValidEmail('')).toBe(false);
});
it('254文字を超えるアドレスを拒否する', () => {
const longLocal = 'a'.repeat(64);
const longDomain = 'b'.repeat(250) + '.com';
expect(isValidEmail(`${longLocal}@${longDomain}`)).toBe(false);
});
it('ドメインラベルがハイフンで始まる場合を拒否する', () => {
expect(isValidEmail('user@-example.com')).toBe(false);
});
it('ドメインラベルがハイフンで終わる場合を拒否する', () => {
expect(isValidEmail('user@example-.com')).toBe(false);
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
238-242:⚠️ Potential issue | 🟠 Major
git diff失敗が無音化され、mutation 品質ゲートを迂回できます。Line 242 の
|| trueにより、git diffが失敗してもCHANGED_FILESが空扱いになり、has_targets=falseで mutation-test がスキップされます。失敗時はジョブを明示的に fail させるべきです。修正案（失敗時に即時エラー、フィルタ処理は維持）
- CHANGED_FILES=$(git diff --name-only --diff-filter=ACMR ${{ github.event.pull_request.base.sha }}...${{ github.sha }} -- \ - 'src/services/**/*.ts' 'src/lib/**/*.ts' | \ - grep -v '__tests__' | \ - grep -v '\.test\.ts$' | \ - grep -v '\.spec\.ts$' || true) + RAW_CHANGED_FILES=$(git diff --name-only --diff-filter=ACMR ${{ github.event.pull_request.base.sha }}...${{ github.sha }} -- \ + 'src/services/**/*.ts' 'src/lib/**/*.ts') || { + echo "::error::git diff failed while collecting mutation targets" + exit 1 + } + CHANGED_FILES=$(printf '%s\n' "$RAW_CHANGED_FILES" | \ + grep -Ev '(__tests__|\.test\.ts$|\.spec\.ts$)' || true)#!/bin/bash set -euo pipefail echo "[1] 対象スニペット確認" sed -n '236,254p' .github/workflows/test.yml echo "[2] 無効なbase refで再現（現状ロジック）" set +e CHANGED_FILES=$(git diff --name-only --diff-filter=ACMR __invalid_base__...HEAD -- \ 'src/services/**/*.ts' 'src/lib/**/*.ts' | \ grep -v '__tests__' | \ grep -v '\.test\.ts$' | \ grep -v '\.spec\.ts$' || true) status=$? set -e printf "assignment_exit=%s\n" "$status" printf "changed_files=<%s>\n" "$CHANGED_FILES" echo "期待結果: assignment_exit=0 かつ changed_files が空でも、stderr には git diff failure が出る（=失敗が握りつぶされる）"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test.yml around lines 238 - 242, The current CHANGED_FILES assignment silences git diff failures via the trailing "|| true", allowing mutation tests to be skipped; remove "|| true" and ensure failures propagate by enabling pipefail or explicitly checking the git diff exit status: update the pipeline that constructs CHANGED_FILES (the line containing CHANGED_FILES=$(git diff ... | grep -v ... | grep -v ... | grep -v ... || true)) to drop "|| true" and either set "set -o pipefail" / "set -euo pipefail" in the step or capture the git diff exit code and fail the job if non-zero so git diff errors cause the workflow to fail rather than producing an empty CHANGED_FILES.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/test.yml:
- Around line 238-242: The current CHANGED_FILES assignment silences git diff
failures via the trailing "|| true", allowing mutation tests to be skipped;
remove "|| true" and ensure failures propagate by enabling pipefail or
explicitly checking the git diff exit status: update the pipeline that
constructs CHANGED_FILES (the line containing CHANGED_FILES=$(git diff ... |
grep -v ... | grep -v ... | grep -v ... || true)) to drop "|| true" and either
set "set -o pipefail" / "set -euo pipefail" in the step or capture the git diff
exit code and fail the job if non-zero so git diff errors cause the workflow to
fail rather than producing an empty CHANGED_FILES.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e283093-a01a-42e3-9421-f5b8ef8aea2f
📒 Files selected for processing (1)
.github/workflows/test.yml
Owner
Author
|
@coderabbitai review |
- terminal-ws.test.ts: mockImplementationをmockImplementationOnceに変更し後続テストへの漏れを防止 - docker-adapter.test.ts: proxyヘルスチェック失敗時にコンテナがクリーンアップされることを検証 - docker-adapter.test.ts: deleteRules失敗時にコンテナstopが実際に呼ばれたことを検証 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- remote-repo-service.ts: pullTimeoutの型をundefined許容に変更し、clearTimeout前にガード追加 - host-adapter.test.ts: afterEachでmockPtyManagerObjのリスナーをクリアしてリーク防止 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- validation.test.ts: 255文字境界テストを正確に255文字で構築 - run-script-manager.test.ts: forEachコールバックの暗黙returnをブロック形式に修正 - host-adapter.test.ts: listenerCount検証をtoBe(1)に厳密化 - host-adapter.test.ts: non-shellテストで正しいセッションID(claude-1)を使用 - host-adapter.test.ts: destroySessionテストに明示的アサーション追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
rebase時のコンフリクト解決で誤って削除された依存を復元 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
origin/$base_refが未フェッチの場合に失敗する問題を修正 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|| trueを除去し、git diff失敗時はexit 1でジョブを停止する。 grepのフィルタリングのみ || true を許可（対象ファイルなしは正常）。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
windschord
force-pushed
the
test/mutation-testing-clean
branch
from
b8685a1 to
ca300a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/__tests__/validation.test.ts (1)
306-347:⚠️ Potential issue | 🟡 Minor
isValidEmailの describe ブロックが重複しています。Lines 179-303 で既に同名の
describe('isValidEmail', ...)ブロックが存在し、この Lines 306-347 のブロックは重複したテストケースを含んでいます（例: "有効なメールアドレスを受け入れる"、"ドメイン部分に連続ドットを含むアドレスを拒否する" など）。この重複ブロックを削除してください。
🗑️ 削除対象
- describe('isValidEmail', () => { - it('有効なメールアドレスを受け入れる', () => { - expect(isValidEmail('user@example.com')).toBe(true); - expect(isValidEmail('user@sub.example.com')).toBe(true); - expect(isValidEmail('user+tag@example.com')).toBe(true); - }); - - it('ドメイン部分に連続ドットを含むアドレスを拒否する', () => { - expect(isValidEmail('user@foo..bar')).toBe(false); - }); - - it('ドメインがドットで始まるアドレスを拒否する', () => { - expect(isValidEmail('user@.foo')).toBe(false); - expect(isValidEmail('user@.foo.com')).toBe(false); - }); - - it('ドメインにTLDが無いアドレスを拒否する', () => { - expect(isValidEmail('user@localhost')).toBe(false); - }); - - it('@が無いアドレスを拒否する', () => { - expect(isValidEmail('userexample.com')).toBe(false); - }); - - it('空文字列を拒否する', () => { - expect(isValidEmail('')).toBe(false); - }); - - it('254文字を超えるアドレスを拒否する', () => { - const longLocal = 'a'.repeat(64); - const longDomain = 'b'.repeat(250) + '.com'; - expect(isValidEmail(`${longLocal}@${longDomain}`)).toBe(false); - }); - - it('ドメインラベルがハイフンで始まる場合を拒否する', () => { - expect(isValidEmail('user@-example.com')).toBe(false); - }); - - it('ドメインラベルがハイフンで終わる場合を拒否する', () => { - expect(isValidEmail('user@example-.com')).toBe(false); - }); - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/validation.test.ts` around lines 306 - 347, There is a duplicate test suite for describe('isValidEmail') — remove the redundant describe('isValidEmail', ...) block (the second one containing tests like "有効なメールアドレスを受け入れる", "ドメイン部分に連続ドットを含むアドレスを拒否する", etc.) so only a single describe('isValidEmail') remains; confirm you delete the entire duplicate block including its it(...) cases and keep the original suite with the intended cases.
🧹 Nitpick comments (25)
src/services/__tests__/encryption-service.test.ts (1)
116-134: 複数回の非同期関数呼び出しを1回にまとめることを検討
service.encrypt('test')が複数回呼び出されており、非効率です。エラーを一度キャプチャして複数のアサーションを行うことで、テストの実行速度と可読性が向上します。♻️ エラーを1回キャプチャするリファクタ案
it('ENCRYPTION_KEYが無効な長さの場合、EncryptionErrorをスローする', async () => { // 16バイトのキー(AES-128)を設定 → 32バイト必須なのでエラー process.env.ENCRYPTION_KEY = Buffer.from('a'.repeat(16)).toString('base64'); const { EncryptionService, EncryptionError } = await import('../encryption-service'); const service = new EncryptionService(); - await expect(service.encrypt('test')).rejects.toThrow(EncryptionError); - await expect(service.encrypt('test')).rejects.toThrow(/Invalid ENCRYPTION_KEY length/); - await expect(service.encrypt('test')).rejects.toThrow(/expected 32 bytes/); + const error = await service.encrypt('test').catch((e) => e); + expect(error).toBeInstanceOf(EncryptionError); + expect(error.message).toMatch(/Invalid ENCRYPTION_KEY length/); + expect(error.message).toMatch(/expected 32 bytes/); }); it('ENCRYPTION_KEYが短すぎる場合、正確なバイト数をエラーメッセージに含む', async () => { process.env.ENCRYPTION_KEY = Buffer.from('short').toString('base64'); const { EncryptionService } = await import('../encryption-service'); const service = new EncryptionService(); - await expect(service.encrypt('test')).rejects.toThrow(/got 5 bytes/); - await expect(service.encrypt('test')).rejects.toThrow(/openssl rand -base64 32/); + const error = await service.encrypt('test').catch((e) => e); + expect(error.message).toMatch(/got 5 bytes/); + expect(error.message).toMatch(/openssl rand -base64 32/); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/encryption-service.test.ts` around lines 116 - 134, The test calls service.encrypt('test') multiple times; instead, call it once and capture the thrown error (e.g., with try/catch or awaiting a single promise) and then run all assertions against that captured error. Update the two specs to instantiate EncryptionService and perform a single await of service.encrypt('test') to capture the error object, then assert its type (EncryptionError) and messages (/Invalid ENCRYPTION_KEY length/, /expected 32 bytes/, /got 5 bytes/, /openssl rand -base64 32/) against that single captured error to avoid redundant async calls.src/services/__tests__/config-service.test.ts (4)
290-291: 未使用の変数_origReadFileを削除してください。この変数は宣言されていますが使用されていません。コメントから判断すると、モックのリストアに使う意図があったようですが、Line 301 で
vi.mocked(fs.readFile).mockRestore()を使用しているため不要です。🧹 修正案
await fs.writeFile(testConfigPath, JSON.stringify({ git_clone_timeout_minutes: 10 }), 'utf-8'); // readFileをモックして非ENOENTエラーを発生させる - const _origReadFile = fs.readFile; vi.spyOn(fs, 'readFile').mockRejectedValueOnce(Object.assign(new Error('Permission denied'), { code: 'EACCES' }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 290 - 291, Remove the unused local variable _origReadFile from the test; it was declared but never referenced. Locate the lines where _origReadFile is assigned and where fs.readFile is spied on (vi.spyOn(fs, 'readFile').mockRejectedValueOnce(...)) and simply delete the _origReadFile declaration, keeping the vi.spyOn call and the later restore via vi.mocked(fs.readFile).mockRestore().
329-341: シングルトンのテストにおけるモジュール状態の共有に注意してください。
getConfigService()はモジュールレベルのconfigServiceInstance変数を使用するため、他のテストが先に実行された場合、すでにインスタンスが作成されている可能性があります。テスト間の独立性を確保するため、シングルトンのリセット機構（例:resetConfigService()関数のエクスポート）を検討するか、vi.resetModules()と動的インポートを使用してください。現状のテストは「2回呼び出しで同一インスタンスが返る」ことを確認しているので機能しますが、コメントで示唆されている動的インポートは実装されていません。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 329 - 341, The test relies on module-level singleton state from getConfigService; to ensure isolation add a way to reset the singleton or use module reset + dynamic import in the test. Either export and call a resetConfigService() (or clearConfigServiceInstance) from the config module and invoke it in beforeEach/afterEach, or call vi.resetModules() then dynamically import the module (e.g. const { getConfigService, ConfigService } = await import('...')) inside each test to ensure a fresh module instance; update the tests to use one of these approaches so getConfigService() does not reuse state across tests.
121-122:.then()/.catch()チェーンを async/await に置き換えてください。コーディングガイドラインに従い、Promise 処理には async/await を使用してください。
♻️ 修正案
- const exists = await fs.access(nonExistentPath).then(() => true).catch(() => false); + let exists = false; + try { + await fs.access(nonExistentPath); + exists = true; + } catch { + exists = false; + } expect(exists).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 121 - 122, テスト内の Promise チェーンを async/await に置き換えてください: fs.access 呼び出しと exists 変数を直接 then()/catch() で処理するのではなく、await を使い try/catch で await fs.access(nonExistentPath) を実行し、成功時は exists を true、例外時は false に設定するように修正してください（参照シンボル: exists, fs.access in src/services/__tests__/config-service.test.ts）。
343-348:initializeConfigServiceのテストでload()が呼ばれたことを検証してください。実装によると
initializeConfigService()は内部でservice.load()を呼び出しますが、このテストはインスタンスの型のみを検証しています。load()が実行されたことを確認するため、ログ出力のアサーションを追加することを推奨します。♻️ 修正案
describe('initializeConfigService', () => { it('ConfigServiceを初期化して返す', async () => { + mockLogger.info.mockClear(); const service = await initializeConfigService(); expect(service).toBeInstanceOf(ConfigService); + // load()が呼ばれたことを確認（デフォルト設定ファイルが存在しない場合のログ） + expect(mockLogger.info).toHaveBeenCalledWith( + 'Configuration file not found, using defaults', + expect.any(Object) + ); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/config-service.test.ts` around lines 343 - 348, テストがインスタンス型だけを検証しており initializeConfigService が内部で呼ぶ ConfigService#load の実行を検証していないため、テストに load 呼び出しのアサーションを追加してください: テスト内で ConfigService.prototype.load をスパイ/モック（jest.spyOn など）してから initializeConfigService() を実行し、スパイが呼ばれたことを expect(...).toHaveBeenCalled() で検証し、最後にスパイを復元してください（対象シンボル: initializeConfigService, ConfigService, load）。src/lib/__tests__/notification-service.test.ts (1)
141-149: 既存テスト（130-139行目）と重複していますこのテストは
beforeEachで既にmockLocalStorage.clear()が呼ばれている状態でgetSettings()を呼び出しており、130-139行目の「設定がない場合デフォルト値を返す」テストと同じコードパスをテストしています。ミューテーションテストのスコア向上のために追加された可能性がありますが、メンテナンス性の観点から既存テストに統合するか、このテストを削除することを検討してください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/notification-service.test.ts` around lines 141 - 149, The test duplicates an existing case that calls mockLocalStorage.clear() then getSettings(); remove this redundant test or merge its assertions into the existing "設定がない場合デフォルト値を返す" test to avoid duplicate coverage; locate the duplicated test (the it(...) block that uses mockLocalStorage.clear() and asserts defaults) and either delete that it block or consolidate its expectations into the earlier test that already runs after beforeEach, ensuring getSettings(), mockLocalStorage, and the default-expect assertions (onTaskComplete, onPermissionRequest, onError, onActionRequired) remain covered once.src/services/__tests__/gh-cli.test.ts (2)
153-159:getPRStatusの merged ケースでも subprocess 呼び出し条件を固定してください戻り値の検証だけだと、
cwd/encodingや引数構築の崩れをこのテストでは拾えません。ここでもexpect.objectContainingで options を明示してください。差分案
it('should return merged status', () => { mockExecFileSync.mockReturnValue(JSON.stringify({ state: 'merged', merged: true })); const result = getPRStatus(456, '/repo'); expect(result.state).toBe('merged'); expect(result.merged).toBe(true); + expect(mockExecFileSync).toHaveBeenCalledWith( + 'gh', + ['pr', 'view', '456', '--json', 'state,merged'], + expect.objectContaining({ + cwd: '/repo', + encoding: 'utf-8', + }) + ); });Based on learnings "Applies to /tests//*.{js,ts,tsx} : Use
expect.objectContaining()with explicit checks for important subprocess options (cwd,env) rather than relying on it to ignore unmatched keys".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/gh-cli.test.ts` around lines 153 - 159, Update the merged-case test for getPRStatus to assert the subprocess invocation options as well as the return values: after calling getPRStatus(456, '/repo') add an expectation that mockExecFileSync was called with the expected CLI args and an options object matching expect.objectContaining({ cwd: '/repo', encoding: 'utf8', env: expect.any(Object) }) (or at least cwd and encoding/env as in other tests) so the test verifies argument construction and working directory rather than only the parsed return value.
81-92:createPRの body未指定ケースでもcwd/encodingを明示検証してくださいこのケースは
--bodyの有無しか見ていないため、subprocess options の回帰を検知できません。toHaveBeenCalledWith(..., expect.objectContaining({ cwd, encoding }))を併記するのが安全です。差分案
it('should not include body when not provided', () => { mockExecFileSync.mockReturnValue('https://github.com/owner/repo/pull/456\n'); createPR({ title: 'Test PR', branchName: 'feature-branch', cwd: '/path/to/repo', }); const args = mockExecFileSync.mock.calls[0][1]; expect(args).not.toContain('--body'); + expect(mockExecFileSync).toHaveBeenCalledWith( + 'gh', + ['pr', 'create', '--title', 'Test PR', '--head', 'feature-branch'], + expect.objectContaining({ + cwd: '/path/to/repo', + encoding: 'utf-8', + }) + ); });As per coding guidelines "Ensure
cwdis asserted in tests forspawnSync/execSynccalls; assertenvexplicitly when it is passed to the subprocess".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/gh-cli.test.ts` around lines 81 - 92, Test `createPR` currently only asserts absence of '--body' and misses validating subprocess options; update the test to also assert that `mockExecFileSync` was called with options containing the expected cwd and encoding (and `env` if your implementation passes it) using expect.objectContaining so regressions in the exec/spawn options are caught—keep the existing check on the command args (mockExecFileSync.mock.calls[0][1] not toContain '--body') and add a toHaveBeenCalledWith check against `mockExecFileSync` referencing `createPR` invocation to assert { cwd: '/path/to/repo', encoding: 'utf8' } (and env if applicable).src/services/__tests__/docker-git-service.test.ts (1)
769-776: ログ検証の一貫性を改善できます。このテストは
mockLogger.infoが呼ばれたことだけを確認していますが、他のロギングテスト（lines 778-804）ではメッセージとパラメータを明示的に検証しています。一貫性のため、具体的なログ内容を検証することを推奨します。♻️ より具体的なアサーションの例
it('cloneRepository成功時にログを記録する', async () => { await dockerGitService.cloneRepository({ url: 'https://github.com/user/repo.git', projectId: 'log-test', }); - expect(mockLogger.info).toHaveBeenCalled(); + expect(mockLogger.info).toHaveBeenCalledWith( + expect.stringContaining('clone'), + expect.objectContaining({ projectId: 'log-test' }) + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-git-service.test.ts` around lines 769 - 776, The test for cloneRepository should assert the specific log message and parameters instead of only checking that mockLogger.info was called; update the test in docker-git-service.test.ts that invokes dockerGitService.cloneRepository to expect mockLogger.info toHaveBeenCalledWith the exact expected message and context (e.g., include the URL 'https://github.com/user/repo.git' and projectId 'log-test' or the structured meta object the service uses). Locate the assertion around mockLogger.info for the 'cloneRepository成功時にログを記録する' case and replace the generic toHaveBeenCalled() with a precise toHaveBeenCalledWith(...) matching the log string and/or meta object emitted by cloneRepository.src/lib/websocket/__tests__/session-ws.test.ts (2)
100-122: 未使用変数_hのパターンについてコメント追加を検討してください。
_hはイベントリスナー登録のために作成されていますが、その意図がコードから明確ではありません。コメントを追加することで、このパターンの理由が明確になります。💡 コメント追加の提案
it('should forward output events as run_script_log', () => { - // Re-create handler to attach listeners + // ハンドラーを作成してイベントリスナーを登録（変数自体は使用しない） const cm = createMockConnectionManager(); const _h = new SessionWebSocketHandler(cm);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/session-ws.test.ts` around lines 100 - 122, The test creates an intentionally unused handler instance (_h) solely to register event listeners via the SessionWebSocketHandler constructor, but that intent isn't documented; add a short inline comment above the `_h = new SessionWebSocketHandler(cm)` line explaining that `_h` is created only to attach listeners (keeps the underscore to satisfy unused-variable linting) so readers understand why the instance is not otherwise used; reference the symbols SessionWebSocketHandler, _h, createMockConnectionManager and mockRunScriptManager in the comment for clarity.
356-404: 非同期ハンドラーのテストで競合状態の可能性があります。
handleMessageは非同期メソッド（Promise<void>を返す）ですが、テストではws.emit('message', message)の後、非同期処理の完了を待たずに即座にアサーションを実行しています。EventEmitterのemitは同期的に実行されますが、非同期リスナー内にawaitがある場合、アサーションがws.sendより先に実行される可能性があります。現在の実装では問題ないかもしれませんが、将来の変更でフレーキーテストになるリスクがあります。
♻️ 非同期完了を待つ修正案
// Send a message const message = Buffer.from(JSON.stringify({ type: 'input', data: 'test' })); ws.emit('message', message); + + // 非同期ハンドラーの完了を待つ + await vi.waitFor(() => { + expect(ws.send).toHaveBeenCalledWith( + expect.stringContaining('Use /ws/claude/:sessionId') + ); + }); - - // Should respond with error directing to Claude WS - expect(ws.send).toHaveBeenCalledWith( - expect.stringContaining('Use /ws/claude/:sessionId') - );または、よりシンプルに:
ws.emit('message', message); + await new Promise(resolve => setImmediate(resolve)); // Should respond with error directing to Claude WS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/session-ws.test.ts` around lines 356 - 404, テストが ws.emit('message', ...) で非同期のメッセージハンドラ完了を待たずにアサーションを行しているため競合が起きる可能性があります。各テスト（該当するケースは handler.handleConnection と ws.emit を使っているテスト群）で ws.emit の直後に非同期処理を待つ一行を追加して、ハンドラ内の await が完了するのを保証してください（例: await new Promise(resolve => setImmediate(resolve)) またはテストユーティリティの waitFor を使う）。これにより ws.send の呼び出し検証や mockLifecycleManager.updateActivity の呼び出し検証が確実にハンドラ完了後に行われます。src/services/adapters/__tests__/host-adapter.test.ts (1)
154-174:cwdオプションの明示的なアサーションを検討してくださいこれらのテストは
expect.any(Object)を使用してオプションを検証していますが、cwdのような重要なサブプロセスオプションが正しく渡されているか確認していません。テストの堅牢性を高めるため、最低限cwdが設定されていることを検証することを推奨します。♻️ 修正案
it('should pass --print flag with initial prompt', () => { hostAdapter.createSession('session-1', '/path/to/work', 'Hello Claude'); expect(pty.spawn).toHaveBeenCalledWith( 'claude', ['--print', 'Hello Claude'], - expect.any(Object) + expect.objectContaining({ + cwd: '/path/to/work', + }) ); }); it('should pass --resume flag with resumeSessionId', () => { hostAdapter.createSession('session-1', '/path/to/work', undefined, { resumeSessionId: 'prev-session-id', }); expect(pty.spawn).toHaveBeenCalledWith( 'claude', ['--resume', 'prev-session-id'], - expect.any(Object) + expect.objectContaining({ + cwd: '/path/to/work', + }) ); });Based on learnings: "Ensure
cwdis asserted in tests forspawnSync/execSynccalls" および "Useexpect.objectContaining()with explicit checks for important subprocess options"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/adapters/__tests__/host-adapter.test.ts` around lines 154 - 174, Update the two tests that call hostAdapter.createSession to assert the subprocess options include the working directory: replace the current expect.any(Object) matcher for pty.spawn with expect.objectContaining({ cwd: '/path/to/work' }) (or a variable for the passed path) so the tests explicitly verify that cwd is passed; reference the test calls to hostAdapter.createSession and the pty.spawn expectation to locate where to change the matcher.src/lib/__tests__/schema-check.test.ts (2)
143-143: インラインオブジェクト型ではなくinterfaceを使ってください。Line 143 の
(col: { name: string; dataType: string })は、型の再利用性と可読性を落とします。テスト内ローカルでもinterface定義に寄せるのがよいです。♻️ 修正案
+ interface ColumnMeta { + name: string; + dataType: string; + } + for (const table of tables) { const tableName = getTableName(table); const columns = getTableColumns(table); - const colDefs = Object.values(columns).map((col: { name: string; dataType: string }) => { + const colDefs = Object.values(columns).map((col: ColumnMeta) => { let sqlType = 'TEXT'; if (col.dataType === 'number') sqlType = 'INTEGER'; return `${col.name} ${sqlType}`; });As per coding guidelines "Prefer
interfacefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/schema-check.test.ts` at line 143, The inline object type used in the map callback for colDefs (currently written as (col: { name: string; dataType: string })) should be replaced with a local interface to improve reuse and readability; declare an interface (e.g., ColumnShape or ColumnDef) near the top of the test file and update the mapping callback signature to use that interface for the columns variable and the (col) parameter, ensuring any other local references to the same shape (columns, colDefs) use the new interface.
127-155:fullDb.close()はfinallyで保証してください。現状は Line 154 でしか close されないため、途中でアサーション失敗するとハンドルが残ります。
try/finallyで後始末を固定化してください。♻️ 修正案
it('全カラムが揃っている場合はvalid=trueを返す', () => { - // 完全なスキーマに合わせたDBを作成 - const fullDb = new Database(':memory:'); - const tables = [ - schema.projects, - schema.sessions, - schema.executionEnvironments, - schema.messages, - schema.prompts, - schema.runScripts, - schema.githubPats, - ]; - - for (const table of tables) { - const tableName = getTableName(table); - const columns = getTableColumns(table); - const colDefs = Object.values(columns).map((col: { name: string; dataType: string }) => { - let sqlType = 'TEXT'; - if (col.dataType === 'number') sqlType = 'INTEGER'; - return `${col.name} ${sqlType}`; - }); - fullDb.exec(`CREATE TABLE ${tableName} (${colDefs.join(', ')})`); - } - - const result = validateSchemaIntegrity(fullDb); - expect(result.valid).toBe(true); - expect(result.missingColumns).toHaveLength(0); - fullDb.close(); + const fullDb = new Database(':memory:'); + try { + const tables = [ + schema.projects, + schema.sessions, + schema.executionEnvironments, + schema.messages, + schema.prompts, + schema.runScripts, + schema.githubPats, + ]; + + for (const table of tables) { + const tableName = getTableName(table); + const columns = getTableColumns(table); + const colDefs = Object.values(columns).map((col: { name: string; dataType: string }) => { + let sqlType = 'TEXT'; + if (col.dataType === 'number') sqlType = 'INTEGER'; + return `${col.name} ${sqlType}`; + }); + fullDb.exec(`CREATE TABLE ${tableName} (${colDefs.join(', ')})`); + } + + const result = validateSchemaIntegrity(fullDb); + expect(result.valid).toBe(true); + expect(result.missingColumns).toHaveLength(0); + } finally { + fullDb.close(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/schema-check.test.ts` around lines 127 - 155, The test creates an in-memory Database instance (fullDb) but only calls fullDb.close() at the end, so if an assertion fails the DB handle leaks; wrap the test body that uses fullDb in a try/finally where the finally always calls fullDb.close() to guarantee cleanup. Locate the test case (the it block that constructs fullDb and calls validateSchemaIntegrity) and change its structure so fullDb is closed in a finally block (keeping validateSchemaIntegrity and assertions inside the try), ensuring no other behavior changes.src/services/__tests__/docker-client.test.ts (1)
170-224: ストリーム生成の重複はヘルパー化すると読みやすくなります
runテスト内でWritableの生成が繰り返されているので、ヘルパー化すると保守性が上がります。♻️ リファクタ例
describe('run', () => { + const createWritableStream = async () => { + const { Writable } = await import('stream'); + return new Writable({ write(_chunk, _encoding, cb) { cb(); } }); + }; + it('should return StatusCode from array result (Dockerode format)', async () => { - const { Writable } = await import('stream'); - const stream = new Writable({ write(_chunk, _encoding, cb) { cb(); } }); + const stream = await createWritableStream(); // ... }); it('should return StatusCode from plain object result', async () => { - const { Writable } = await import('stream'); - const stream = new Writable({ write(_chunk, _encoding, cb) { cb(); } }); + const stream = await createWritableStream(); // ... }); });Also applies to: 232-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-client.test.ts` around lines 170 - 224, Consolidate the repeated Writable stream creation in the docker client tests by adding a small helper (e.g., makeWritableStream or getWritable) inside the test suite scope and replace each repeated block that does "const { Writable } = await import('stream'); const stream = new Writable({ write(...){ cb(); } });" with a single call to that helper; update all tests that call dockerClient.run and mockDockerInstance.run to use the helper so the tests remain the same but duplication is removed.src/lib/websocket/__tests__/terminal-ws.test.ts (1)
290-292: スキップされたテストの追跡を検討してください。非同期コールバックの問題でスキップされているテストが複数あります。TODOコメントは適切ですが、これらのテストが確実に修正されるよう、Issue として追跡することを検討してください。
Issue を作成して追跡しましょうか？
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 290 - 292, The PR contains skipped tests (e.g., the test named 'should forward input to ptyManager (via connection handler)' in terminal-ws.test.ts) due to an asynchronous callback registration issue; create a tracked Issue that lists all skipped tests, their filenames and test names, the root cause ("WS connection callback addConnection completes asynchronously in test env"), reproduction steps, expected behavior, and a proposed fix (ensure handler registration awaits or mock the async callback), then assign an owner and add labels like "tests", "flaky", and "needs-investigation" so the skipped tests are prioritized and not forgotten.src/services/__tests__/environment-adapter.test.ts (1)
139-144: 重複するprimitive検証は統合してもよいです。Line 139-143 は既存の primitive ケース（Line 50-54）と重複しているため、
Symbolの追加確認だけに絞るか、1つのテーブル駆動テストに寄せると保守しやすくなります。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/environment-adapter.test.ts` around lines 139 - 144, The test duplicates existing primitive checks for isEnvironmentAdapter; consolidate by removing the redundant checks and either (a) keep this spec focused only on the Symbol case (e.g. rename the test to indicate Symbol) or (b) merge all primitive cases into a single table-driven test that covers Number, String, Boolean and Symbol; update the test description (the it(...) string) accordingly and ensure the single source uses isEnvironmentAdapter in each row so maintenance is easier.src/services/__tests__/pty-manager.test.ts (1)
282-288:pty.spawnオプション検証はobjectContainingに寄せてください。
mock.calls[0][2]の直接参照より、toHaveBeenCalledWith(..., expect.objectContaining({ cwd }))の形にすると、重要オプションの意図が明確でガイドラインにも一致します。差分例
- const options = vi.mocked(pty.spawn).mock.calls[0][2]; - expect(options).toHaveProperty('cwd', path.resolve(testWorkingDir)); + expect(vi.mocked(pty.spawn)).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ + cwd: path.resolve(testWorkingDir), + }), + );As per coding guidelines: Use
expect.objectContaining()with explicit checks for important subprocess options (cwd,env) rather than relying on it to ignore unmatched keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/pty-manager.test.ts` around lines 282 - 288, Replace the direct inspection of pty.spawn mock call args with an assertion that pty.spawn was called with expect.objectContaining for the options object: call ptyManager.createPTY(testSessionId, testWorkingDir) then assert vi.mocked(pty.spawn) was called with the expected args and use expect.objectContaining({ cwd: path.resolve(testWorkingDir) }) (and include env if relevant) so the test checks the important option(s) without depending on exact other keys; reference ptyManager.createPTY, vi.mocked(pty.spawn) and testWorkingDir to locate the change.src/lib/__tests__/docker-volume-utils.test.ts (1)
46-50: このケースは完全一致まで見たほうがミューテーションに強いです。
not.toContain(' ')だけだと、ID を別文字へ置換したり一部だけ落とす実装でも通ります。ここはclaude-config-claude-spacedとclaude-config-configclaude-spacedの exact value まで固定したほうが、今回の trim/sanitize 変更をきちんと守れます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/__tests__/docker-volume-utils.test.ts` around lines 46 - 50, テストで空白が含まれないことだけを確認している getConfigVolumeNames の検証を、より厳密な完全一致に置き換えてください：getConfigVolumeNames(' spaced ') の戻り値のプロパティ claudeVolume と configClaudeVolume がそれぞれ期待される正確な文字列（例: "claude-config-claude-spaced" と "claude-config-configclaude-spaced"）と完全に一致するよう expect(...).toBe(...) でチェックするよう修正し、trim/sanitize の仕様を正確に検証できるようにしてください。src/services/__tests__/docker-pty-adapter.test.ts (2)
386-393:claudeCodeOptionsの検証が実質ベースコマンドしか見ていません。
claudeが含まれることだけだと、オプション変換が丸ごと消えても通ります。少なくともclaude-3-opus自体、または変換後フラグがcreateOptions.Cmdに入ることまで見たほうが、この追加ケースの意味が出ます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-pty-adapter.test.ts` around lines 386 - 393, The test for claudeCodeOptions only asserts that 'claude' appears in createOptions.Cmd which doesn't verify option conversion; update the test in docker-pty-adapter.test.ts that calls adapter.createSession('session-1', '/workspace', ..., { claudeCodeOptions: { model: 'claude-3-opus' } }) to assert that mockDockerClient.createContainer's first call (createOptions = mockDockerClient.createContainer.mock.calls[0][0]) has createOptions.Cmd containing either the exact model string 'claude-3-opus' or the expected translated flag(s) produced by the code path that converts claudeCodeOptions (e.g., the specific CLI flag your adapter emits for model), so the test fails if option conversion is dropped.
569-591: この 2 ケースも判定結果まで固定したいです。
port is already allocatedとMounts deniedはerrorHandlerが呼ばれたことしか見ていないので、専用の分類が壊れて汎用エラーに落ちても通ります。先の 3 ケースと同じく、errorTypeか識別用メッセージまで確認したほうが分岐の回帰を防げます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/docker-pty-adapter.test.ts` around lines 569 - 591, The two tests only assert that errorHandler was called, which allows regressions that collapse specific classifications into generic errors; update each test (the ones using adapter.createSession and mockPTYStreamInstance.emit) to assert the emitted error payload contains the specific classification or identifying message (e.g., check the event arg passed to errorHandler includes a specific errorType like 'PORT_IN_USE' for "port is already allocated" and a 'VOLUME_MOUNT_ERROR' or the mount message for "Mounts denied"). Locate the tests that register errorHandler and change the expectation from toHaveBeenCalled() to toHaveBeenCalledWith(expect.objectContaining({ errorType: '...', message: expect.stringContaining('port is already allocated' / 'Mounts denied') })) so the tests fail if specific classification regresses.src/services/__tests__/adapter-factory.test.ts (1)
465-479:null configケースも既定値まで固定したいです。ここはインスタンスが作れたことしか見ていないので、
config: nullでimageName/imageTagの既定値が崩れても検出できません。直前の{}ケースと同じく、DockerAdapterへ渡る既定の設定値まで確認したほうがこの分岐の回帰を防げます。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/adapter-factory.test.ts` around lines 465 - 479, The test only asserts the adapter is defined for config: null but should also verify that DockerAdapter receives the default imageName/imageTag; update the test that calls AdapterFactory.getAdapter(environment) to either assert the adapter is an instance of DockerAdapter and then check its config/image fields (e.g., adapter.config.imageName and adapter.config.imageTag) against the known default constants on DockerAdapter (or import DEFAULT_* constants used by DockerAdapter), or mock/spyon on DockerAdapter constructor to capture the passed config and assert imageName/imageTag equal the defaults, ensuring the null-config branch preserves default values.src/services/__tests__/scrollback-buffer.test.ts (3)
69-78: このケースは>=分岐を実証できていません。いまの期待値だと、条件を
>に変えても append→trim で同じ結果になり、>=の境界変異を取り逃します。分岐自体を固定したいならchunks.lengthを観測するテスト用フックを足すか、ケース名を結果ベースに寄せた方がよいです。
As per coding guidelines,Ensure code comments describing best practices or anti-patterns are aligned with the actual implementation; if implementation contradicts a comment, fix the code or update the comment during code review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/scrollback-buffer.test.ts` around lines 69 - 78, The test doesn't actually exercise the >= branch because the observable result is identical if the condition were >; update the test to assert the internal chunk behavior instead of only the final string. Add a test-only accessor on ScrollbackBuffer (e.g., getChunkCount or getChunksLength) or expose a test hook so the case can assert that after smallBuffer.append('s1','cdefg') the chunks length for stream 's1' is 1 (verifying the replacement path), then keep the existing getBuffer/getByteSize assertions; alternatively, rename the test to be result-focused if you prefer not to add a hook.
222-275: コンパクションの発生自体はまだ観測できていません。50/101/105 チャンクのケースはいずれも内容と
byteSizeしか見ていないので、> 100→>= 100の閾値変更やコンパクション無効化でも通る可能性が残ります。ミューテーションスコアを狙うなら、ちょうど 100/101 でchunks.lengthを見られる補助を入れて、非圧縮と圧縮後を区別できるようにしたいです。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/scrollback-buffer.test.ts` around lines 222 - 275, The tests only check content and byteSize, so add assertions that distinguish pre- and post-compaction by inspecting the internal chunk count: introduce or use a method like ScrollbackBuffer.getChunksCount(sessionId) (or expose the internal chunks map/array for test access) and in the exact-100 test assert chunksCount === 100 (no compaction) and in the 101/105 tests assert chunksCount is reduced after append (e.g., < 100 or === 1 depending on compaction strategy); update the 50/101/105 test cases to include these chunks-count assertions to ensure compaction actually triggers only after the threshold.
121-158:chunks.length > 1/ 空チャンク分岐の説明と、実際に検証できている内容がずれています。121-158 は公開 API だけでは「最後の 1 チャンクが maxSize 超過」の状態を作れておらず、直前ケースと同じ overflow→trim を再確認しているだけです。160-163 も空チャンクではなく未作成セッションの再テストになっています。内部ガードを狙うなら観測ポイントを追加するか、ケース名を実際の期待値に合わせて絞った方がよいです。
As per coding guidelines,Ensure code comments describing best practices or anti-patterns are aligned with the actual implementation; if implementation contradicts a comment, fix the code or update the comment during code review.Also applies to: 160-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/scrollback-buffer.test.ts` around lines 121 - 158, The test descriptions claim they're verifying the internal guard that prevents deleting the last chunk when chunks.length === 1, but the test cases using ScrollbackBuffer.append, getBuffer and getByteSize actually reproduce the same overflow→trim behavior rather than producing a final single-chunk state that exceeds maxSize; either change the test assertions/comments to match the observed behavior or create a new observable scenario that truly leaves a single oversized chunk (or expose a test-only accessor) and assert that chunks.length remains 1 and that getBuffer/getByteSize reflect the oversized final chunk; update the test names/comments around the ScrollbackBuffer append/getBuffer/getByteSize cases to accurately describe what's being validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/__tests__/env-validation.test.ts`:
- Around line 172-180: The test should also verify that detectClaudePath()
invokes which with the actual CLAUDE_CODE_PATH value instead of a hardcoded
default: keep the existing expect(() => detectClaudePath()).toThrow(...)
assertion, and add an assertion that mockExecSync was called with the command
string constructed from process.env.CLAUDE_CODE_PATH (e.g.
expect(mockExecSync).toHaveBeenCalledWith('which claude-custom') or matching the
dynamic `"which ${process.env.CLAUDE_CODE_PATH}"`), optionally asserting call
count to ensure only the expected which invocation occurred; reference
detectClaudePath, mockExecSync and CLAUDE_CODE_PATH to locate where to add this
assertion.
In `@src/lib/__tests__/schema-check.test.ts`:
- Around line 232-234: The test is asserting the obsolete fixed command 'npx
drizzle-kit push' (via the expect on the variable message) which diverges from
current operations; update the assertions in
src/lib/__tests__/schema-check.test.ts so the message either includes 'npm run
db:push' instead of 'npx drizzle-kit push' or accepts both forms (e.g., change
the single expect against message to allow /(?:npx drizzle-kit push|npm run
db:push)/ or check message.includes for either string), keeping the existing
checks for '修復方法' and 'npx claude-work' intact.
In `@src/lib/__tests__/session-name-generator.test.ts`:
- Around line 45-46: The comment in session-name-generator.test.ts uses the
wrong array length literal "51" which doesn't match the actual 50-element data;
update the comment to use 50 and adjust the example calculations accordingly
(e.g., 0.99 * 50 = 49.5 -> floor = 49 and 0.99 / 50 = 0.0198... -> floor = 0) so
the comment correctly reflects the implementation for the session name
generation test.
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 329-335: The resize test still contains a fallback branch that
lets the test pass by asserting mockPtyManager.createPTY when messageHandler is
missing; update the test in terminal-ws.test.ts to either mark this test as
skipped (like the input transfer test) or remove the else branch and make
handler registration required by asserting messageHandler is defined (e.g.,
expect(messageHandler).toBeDefined() / throw if undefined) before calling
messageHandler(Buffer.from(...)) and then assert mockPtyManager.resize was
called with 'session-1-terminal', 100, 50; target the messageHandler variable
and mockPtyManager.resize/createPTY assertions to locate and change the code.
In `@src/services/__tests__/docker-client.test.ts`:
- Around line 357-371: The tests for inspectImage and inspectContainer currently
assert that the methods throw but do not verify that no logging occurred; update
the two test cases in docker-client.test.ts (the "inspectImage error handling"
and "inspectContainer error handling" it blocks) to also assert that
mockLogger.error was not called by adding
expect(mockLogger.error).not.toHaveBeenCalled() after the await
expect(...).rejects.toThrow(...) assertions so the tests will fail if logging is
added.
In `@src/services/__tests__/docker-git-service.test.ts`:
- Around line 518-528: テスト「SSHキー認証時にBindsが追加される」は現在
dockerGitService.cloneRepository を呼び出して result.success
のみを検証していますが、mockDockerClient.run の呼び出し引数に Binds が含まれているかを検証していません。修正方法：テスト内で
mockDockerClient.run が呼ばれたことを期待し、呼び出し時の引数オブジェクト（mockDockerClient.run.mock.calls
または vi.mocked(...).mock.calls）を取得して、その中に期待する Binds 配列またはキーのマウント（例：SSH キーのパス ->
コンテナ内の .ssh パス）が含まれていることをアサートするように dockerGitService.cloneRepository
呼び出し後に追加してください（関数名参照：dockerGitService.cloneRepository、mockDockerClient.run）。
In `@src/services/__tests__/proxy-client.test.ts`:
- Around line 219-232: The test currently uses a try/catch which can miss
regressions where setRules succeeds; replace it with an assertion that the
promise rejects and validates the error details. Change the try/catch block in
the test for client.setRules(...) to use await
expect(client.setRules('172.20.0.3', [{ host: 'invalid'
}])).rejects.toBeInstanceOf(ProxyValidationError) and also assert the payload
using .rejects.toMatchObject({ details: errorBody.details }) (or
.rejects.toHaveProperty('details', errorBody.details)); keep the mocked
fetch/Response as-is and reference the setRules function and
ProxyValidationError class in the updated assertions.
In `@src/services/__tests__/pty-manager.test.ts`:
- Around line 404-415: The test assumes a non-Windows environment but lacks a
runtime guard, causing failures on win32; update the test in pty-manager.test.ts
to check the platform (use os.platform() or process.platform) and skip or return
early when platform === 'win32' before setting process.env.SHELL and calling
ptyManager.createPTY; keep the existing assertions for pty.spawn and restore
process.env.SHELL in the non-win32 path only so the test only runs where the
code path in ptyManager.createPTY (which picks 'powershell.exe' on win32)
matches the expectation.
In `@src/services/__tests__/run-script-manager.test.ts`:
- Around line 198-225: The test "should send SIGKILL when timeout fires and
process still exists" sets a spy on global.setTimeout but restores mocks only at
the end, which can leak if assertions fail; wrap the spy setup and the test
logic that uses timeoutCallback/runScriptManager.runScript/runScriptManager.stop
inside a try/finally and call vi.restoreAllMocks() (or explicitly restore the
setTimeout spy) in the finally block so the global timer is always restored even
on test failure.
In `@src/services/adapters/__tests__/base-adapter.test.ts`:
- Around line 341-351: The test mutates global process.env and doesn't guarantee
restoration on assertion failures; wrap the mutation and assertions in a
try/finally so originalEnv is always restored. Specifically, in the test case
around adapter.testSpawnPTY('claude', [], {}), save const originalEnv =
process.env, set process.env = {...originalEnv, CLAUDECODE:
'should-be-excluded'} inside the try block, run adapter.testSpawnPTY and the
spawn/expect assertions, and perform process.env = originalEnv inside the
finally block to ensure cleanup even on failure.
In `@src/services/proxy-client.ts`:
- Line 147: Add a regression test in proxy-client.test.ts that ensures the URL
built in src/services/proxy-client.ts is percent-encoded for values with colons:
call the ProxyClient method that constructs the URL (the code path that creates
const url = `${this.baseUrl}/api/v1/rules/${encodeURIComponent(sourceIP)}`)
using an IPv6 address like "2001:db8::1", mock/fake fetch, and assert that fetch
was invoked with the encoded URL (i.e. encodeURIComponent("2001:db8::1") or
contains "%3A") so the test fails if encodeURIComponent is removed or omitted.
- Around line 45-47: The JSDoc is inconsistent: the class ProxyValidationError
now represents 4xx client errors but setRules() still documents only 422 and
deleteRules()/deleteAllRules() omit it; update the JSDoc for setRules(),
deleteRules(), and deleteAllRules() so their `@throws` tags match the class-level
contract by documenting that they can throw ProxyValidationError for 4xx client
errors (for setRules you may note 422 as a common case), and ensure the thrown
description text is consistent across these methods and the ProxyValidationError
class comment.
---
Outside diff comments:
In `@src/lib/__tests__/validation.test.ts`:
- Around line 306-347: There is a duplicate test suite for
describe('isValidEmail') — remove the redundant describe('isValidEmail', ...)
block (the second one containing tests like "有効なメールアドレスを受け入れる",
"ドメイン部分に連続ドットを含むアドレスを拒否する", etc.) so only a single describe('isValidEmail')
remains; confirm you delete the entire duplicate block including its it(...)
cases and keep the original suite with the intended cases.
---
Nitpick comments:
In `@src/lib/__tests__/docker-volume-utils.test.ts`:
- Around line 46-50: テストで空白が含まれないことだけを確認している getConfigVolumeNames
の検証を、より厳密な完全一致に置き換えてください：getConfigVolumeNames(' spaced ') の戻り値のプロパティ
claudeVolume と configClaudeVolume がそれぞれ期待される正確な文字列（例:
"claude-config-claude-spaced" と "claude-config-configclaude-spaced"）と完全に一致するよう
expect(...).toBe(...) でチェックするよう修正し、trim/sanitize の仕様を正確に検証できるようにしてください。
In `@src/lib/__tests__/notification-service.test.ts`:
- Around line 141-149: The test duplicates an existing case that calls
mockLocalStorage.clear() then getSettings(); remove this redundant test or merge
its assertions into the existing "設定がない場合デフォルト値を返す" test to avoid duplicate
coverage; locate the duplicated test (the it(...) block that uses
mockLocalStorage.clear() and asserts defaults) and either delete that it block
or consolidate its expectations into the earlier test that already runs after
beforeEach, ensuring getSettings(), mockLocalStorage, and the default-expect
assertions (onTaskComplete, onPermissionRequest, onError, onActionRequired)
remain covered once.
In `@src/lib/__tests__/schema-check.test.ts`:
- Line 143: The inline object type used in the map callback for colDefs
(currently written as (col: { name: string; dataType: string })) should be
replaced with a local interface to improve reuse and readability; declare an
interface (e.g., ColumnShape or ColumnDef) near the top of the test file and
update the mapping callback signature to use that interface for the columns
variable and the (col) parameter, ensuring any other local references to the
same shape (columns, colDefs) use the new interface.
- Around line 127-155: The test creates an in-memory Database instance (fullDb)
but only calls fullDb.close() at the end, so if an assertion fails the DB handle
leaks; wrap the test body that uses fullDb in a try/finally where the finally
always calls fullDb.close() to guarantee cleanup. Locate the test case (the it
block that constructs fullDb and calls validateSchemaIntegrity) and change its
structure so fullDb is closed in a finally block (keeping
validateSchemaIntegrity and assertions inside the try), ensuring no other
behavior changes.
In `@src/lib/websocket/__tests__/session-ws.test.ts`:
- Around line 100-122: The test creates an intentionally unused handler instance
(_h) solely to register event listeners via the SessionWebSocketHandler
constructor, but that intent isn't documented; add a short inline comment above
the `_h = new SessionWebSocketHandler(cm)` line explaining that `_h` is created
only to attach listeners (keeps the underscore to satisfy unused-variable
linting) so readers understand why the instance is not otherwise used; reference
the symbols SessionWebSocketHandler, _h, createMockConnectionManager and
mockRunScriptManager in the comment for clarity.
- Around line 356-404: テストが ws.emit('message', ...)
で非同期のメッセージハンドラ完了を待たずにアサーションを行しているため競合が起きる可能性があります。各テスト（該当するケースは
handler.handleConnection と ws.emit を使っているテスト群）で ws.emit
の直後に非同期処理を待つ一行を追加して、ハンドラ内の await が完了するのを保証してください（例: await new Promise(resolve =>
setImmediate(resolve)) またはテストユーティリティの waitFor を使う）。これにより ws.send の呼び出し検証や
mockLifecycleManager.updateActivity の呼び出し検証が確実にハンドラ完了後に行われます。
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 290-292: The PR contains skipped tests (e.g., the test named
'should forward input to ptyManager (via connection handler)' in
terminal-ws.test.ts) due to an asynchronous callback registration issue; create
a tracked Issue that lists all skipped tests, their filenames and test names,
the root cause ("WS connection callback addConnection completes asynchronously
in test env"), reproduction steps, expected behavior, and a proposed fix (ensure
handler registration awaits or mock the async callback), then assign an owner
and add labels like "tests", "flaky", and "needs-investigation" so the skipped
tests are prioritized and not forgotten.
In `@src/services/__tests__/adapter-factory.test.ts`:
- Around line 465-479: The test only asserts the adapter is defined for config:
null but should also verify that DockerAdapter receives the default
imageName/imageTag; update the test that calls
AdapterFactory.getAdapter(environment) to either assert the adapter is an
instance of DockerAdapter and then check its config/image fields (e.g.,
adapter.config.imageName and adapter.config.imageTag) against the known default
constants on DockerAdapter (or import DEFAULT_* constants used by
DockerAdapter), or mock/spyon on DockerAdapter constructor to capture the passed
config and assert imageName/imageTag equal the defaults, ensuring the
null-config branch preserves default values.
In `@src/services/__tests__/config-service.test.ts`:
- Around line 290-291: Remove the unused local variable _origReadFile from the
test; it was declared but never referenced. Locate the lines where _origReadFile
is assigned and where fs.readFile is spied on (vi.spyOn(fs,
'readFile').mockRejectedValueOnce(...)) and simply delete the _origReadFile
declaration, keeping the vi.spyOn call and the later restore via
vi.mocked(fs.readFile).mockRestore().
- Around line 329-341: The test relies on module-level singleton state from
getConfigService; to ensure isolation add a way to reset the singleton or use
module reset + dynamic import in the test. Either export and call a
resetConfigService() (or clearConfigServiceInstance) from the config module and
invoke it in beforeEach/afterEach, or call vi.resetModules() then dynamically
import the module (e.g. const { getConfigService, ConfigService } = await
import('...')) inside each test to ensure a fresh module instance; update the
tests to use one of these approaches so getConfigService() does not reuse state
across tests.
- Around line 121-122: テスト内の Promise チェーンを async/await に置き換えてください: fs.access
呼び出しと exists 変数を直接 then()/catch() で処理するのではなく、await を使い try/catch で await
fs.access(nonExistentPath) を実行し、成功時は exists を true、例外時は false
に設定するように修正してください（参照シンボル: exists, fs.access in
src/services/__tests__/config-service.test.ts）。
- Around line 343-348: テストがインスタンス型だけを検証しており initializeConfigService が内部で呼ぶ
ConfigService#load の実行を検証していないため、テストに load 呼び出しのアサーションを追加してください: テスト内で
ConfigService.prototype.load をスパイ/モック（jest.spyOn など）してから
initializeConfigService() を実行し、スパイが呼ばれたことを expect(...).toHaveBeenCalled()
で検証し、最後にスパイを復元してください（対象シンボル: initializeConfigService, ConfigService, load）。
In `@src/services/__tests__/docker-client.test.ts`:
- Around line 170-224: Consolidate the repeated Writable stream creation in the
docker client tests by adding a small helper (e.g., makeWritableStream or
getWritable) inside the test suite scope and replace each repeated block that
does "const { Writable } = await import('stream'); const stream = new Writable({
write(...){ cb(); } });" with a single call to that helper; update all tests
that call dockerClient.run and mockDockerInstance.run to use the helper so the
tests remain the same but duplication is removed.
In `@src/services/__tests__/docker-git-service.test.ts`:
- Around line 769-776: The test for cloneRepository should assert the specific
log message and parameters instead of only checking that mockLogger.info was
called; update the test in docker-git-service.test.ts that invokes
dockerGitService.cloneRepository to expect mockLogger.info toHaveBeenCalledWith
the exact expected message and context (e.g., include the URL
'https://github.com/user/repo.git' and projectId 'log-test' or the structured
meta object the service uses). Locate the assertion around mockLogger.info for
the 'cloneRepository成功時にログを記録する' case and replace the generic toHaveBeenCalled()
with a precise toHaveBeenCalledWith(...) matching the log string and/or meta
object emitted by cloneRepository.
In `@src/services/__tests__/docker-pty-adapter.test.ts`:
- Around line 386-393: The test for claudeCodeOptions only asserts that 'claude'
appears in createOptions.Cmd which doesn't verify option conversion; update the
test in docker-pty-adapter.test.ts that calls adapter.createSession('session-1',
'/workspace', ..., { claudeCodeOptions: { model: 'claude-3-opus' } }) to assert
that mockDockerClient.createContainer's first call (createOptions =
mockDockerClient.createContainer.mock.calls[0][0]) has createOptions.Cmd
containing either the exact model string 'claude-3-opus' or the expected
translated flag(s) produced by the code path that converts claudeCodeOptions
(e.g., the specific CLI flag your adapter emits for model), so the test fails if
option conversion is dropped.
- Around line 569-591: The two tests only assert that errorHandler was called,
which allows regressions that collapse specific classifications into generic
errors; update each test (the ones using adapter.createSession and
mockPTYStreamInstance.emit) to assert the emitted error payload contains the
specific classification or identifying message (e.g., check the event arg passed
to errorHandler includes a specific errorType like 'PORT_IN_USE' for "port is
already allocated" and a 'VOLUME_MOUNT_ERROR' or the mount message for "Mounts
denied"). Locate the tests that register errorHandler and change the expectation
from toHaveBeenCalled() to toHaveBeenCalledWith(expect.objectContaining({
errorType: '...', message: expect.stringContaining('port is already allocated' /
'Mounts denied') })) so the tests fail if specific classification regresses.
In `@src/services/__tests__/encryption-service.test.ts`:
- Around line 116-134: The test calls service.encrypt('test') multiple times;
instead, call it once and capture the thrown error (e.g., with try/catch or
awaiting a single promise) and then run all assertions against that captured
error. Update the two specs to instantiate EncryptionService and perform a
single await of service.encrypt('test') to capture the error object, then assert
its type (EncryptionError) and messages (/Invalid ENCRYPTION_KEY length/,
/expected 32 bytes/, /got 5 bytes/, /openssl rand -base64 32/) against that
single captured error to avoid redundant async calls.
In `@src/services/__tests__/environment-adapter.test.ts`:
- Around line 139-144: The test duplicates existing primitive checks for
isEnvironmentAdapter; consolidate by removing the redundant checks and either
(a) keep this spec focused only on the Symbol case (e.g. rename the test to
indicate Symbol) or (b) merge all primitive cases into a single table-driven
test that covers Number, String, Boolean and Symbol; update the test description
(the it(...) string) accordingly and ensure the single source uses
isEnvironmentAdapter in each row so maintenance is easier.
In `@src/services/__tests__/gh-cli.test.ts`:
- Around line 153-159: Update the merged-case test for getPRStatus to assert the
subprocess invocation options as well as the return values: after calling
getPRStatus(456, '/repo') add an expectation that mockExecFileSync was called
with the expected CLI args and an options object matching
expect.objectContaining({ cwd: '/repo', encoding: 'utf8', env:
expect.any(Object) }) (or at least cwd and encoding/env as in other tests) so
the test verifies argument construction and working directory rather than only
the parsed return value.
- Around line 81-92: Test `createPR` currently only asserts absence of '--body'
and misses validating subprocess options; update the test to also assert that
`mockExecFileSync` was called with options containing the expected cwd and
encoding (and `env` if your implementation passes it) using
expect.objectContaining so regressions in the exec/spawn options are caught—keep
the existing check on the command args (mockExecFileSync.mock.calls[0][1] not
toContain '--body') and add a toHaveBeenCalledWith check against
`mockExecFileSync` referencing `createPR` invocation to assert { cwd:
'/path/to/repo', encoding: 'utf8' } (and env if applicable).
In `@src/services/__tests__/pty-manager.test.ts`:
- Around line 282-288: Replace the direct inspection of pty.spawn mock call args
with an assertion that pty.spawn was called with expect.objectContaining for the
options object: call ptyManager.createPTY(testSessionId, testWorkingDir) then
assert vi.mocked(pty.spawn) was called with the expected args and use
expect.objectContaining({ cwd: path.resolve(testWorkingDir) }) (and include env
if relevant) so the test checks the important option(s) without depending on
exact other keys; reference ptyManager.createPTY, vi.mocked(pty.spawn) and
testWorkingDir to locate the change.
In `@src/services/__tests__/scrollback-buffer.test.ts`:
- Around line 69-78: The test doesn't actually exercise the >= branch because
the observable result is identical if the condition were >; update the test to
assert the internal chunk behavior instead of only the final string. Add a
test-only accessor on ScrollbackBuffer (e.g., getChunkCount or getChunksLength)
or expose a test hook so the case can assert that after
smallBuffer.append('s1','cdefg') the chunks length for stream 's1' is 1
(verifying the replacement path), then keep the existing getBuffer/getByteSize
assertions; alternatively, rename the test to be result-focused if you prefer
not to add a hook.
- Around line 222-275: The tests only check content and byteSize, so add
assertions that distinguish pre- and post-compaction by inspecting the internal
chunk count: introduce or use a method like
ScrollbackBuffer.getChunksCount(sessionId) (or expose the internal chunks
map/array for test access) and in the exact-100 test assert chunksCount === 100
(no compaction) and in the 101/105 tests assert chunksCount is reduced after
append (e.g., < 100 or === 1 depending on compaction strategy); update the
50/101/105 test cases to include these chunks-count assertions to ensure
compaction actually triggers only after the threshold.
- Around line 121-158: The test descriptions claim they're verifying the
internal guard that prevents deleting the last chunk when chunks.length === 1,
but the test cases using ScrollbackBuffer.append, getBuffer and getByteSize
actually reproduce the same overflow→trim behavior rather than producing a final
single-chunk state that exceeds maxSize; either change the test
assertions/comments to match the observed behavior or create a new observable
scenario that truly leaves a single oversized chunk (or expose a test-only
accessor) and assert that chunks.length remains 1 and that getBuffer/getByteSize
reflect the oversized final chunk; update the test names/comments around the
ScrollbackBuffer append/getBuffer/getByteSize cases to accurately describe
what's being validated.
In `@src/services/adapters/__tests__/host-adapter.test.ts`:
- Around line 154-174: Update the two tests that call hostAdapter.createSession
to assert the subprocess options include the working directory: replace the
current expect.any(Object) matcher for pty.spawn with expect.objectContaining({
cwd: '/path/to/work' }) (or a variable for the passed path) so the tests
explicitly verify that cwd is passed; reference the test calls to
hostAdapter.createSession and the pty.spawn expectation to locate where to
change the matcher.
Comment on lines
+172
to
+180
| it('CLAUDE_CODE_PATHがコマンド名でwhichが空文字を返す場合、which解決をスキップしてエラー', () => { | ||
| process.env.CLAUDE_CODE_PATH = 'claude-custom'; | ||
| mockExistsSync.mockReturnValue(false); | ||
| mockExecSync.mockReturnValue(''); | ||
|
|
||
| expect(() => { | ||
| detectClaudePath(); | ||
| }).toThrow('CLAUDE_CODE_PATH is set but the path does not exist: claude-custom'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "env-validation" --type fRepository: windschord/claude-work
Length of output: 132
🏁 Script executed:
cat -n src/lib/__tests__/env-validation.test.ts | head -200Repository: windschord/claude-work
Length of output: 6905
🏁 Script executed:
rg "detectClaudePath" --type ts -A 5 -B 2 src/lib/Repository: windschord/claude-work
Length of output: 8242
🏁 Script executed:
cat -n src/lib/env-validation.tsRepository: windschord/claude-work
Length of output: 3829
テストで which claude-custom コマンドが実際に渡されていることを検証してください。
現在のテストは最終的な例外メッセージのみを確認しているため、実装が CLAUDE_CODE_PATH を無視して固定の which claude を呼び出していてもこのテストは通ります。成功ケース（145行目）では mockExecSync の呼び出しを検証しているのに対し、このテストで同様の検証が欠けています。mockExecSync に渡される引数まで確認することで、環境変数の取り違えを防ぐことができます。
推奨される修正
it('CLAUDE_CODE_PATHがコマンド名でwhichが空文字を返す場合、which解決をスキップしてエラー', () => {
process.env.CLAUDE_CODE_PATH = 'claude-custom';
mockExistsSync.mockReturnValue(false);
mockExecSync.mockReturnValue('');
expect(() => {
detectClaudePath();
}).toThrow('CLAUDE_CODE_PATH is set but the path does not exist: claude-custom');
+ expect(mockExecSync).toHaveBeenCalledWith(
+ 'which claude-custom',
+ expect.objectContaining({
+ encoding: 'utf-8',
+ timeout: 5000,
+ })
+ );
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('CLAUDE_CODE_PATHがコマンド名でwhichが空文字を返す場合、which解決をスキップしてエラー', () => { | |
| process.env.CLAUDE_CODE_PATH = 'claude-custom'; | |
| mockExistsSync.mockReturnValue(false); | |
| mockExecSync.mockReturnValue(''); | |
| expect(() => { | |
| detectClaudePath(); | |
| }).toThrow('CLAUDE_CODE_PATH is set but the path does not exist: claude-custom'); | |
| }); | |
| it('CLAUDE_CODE_PATHがコマンド名でwhichが空文字を返す場合、which解決をスキップしてエラー', () => { | |
| process.env.CLAUDE_CODE_PATH = 'claude-custom'; | |
| mockExistsSync.mockReturnValue(false); | |
| mockExecSync.mockReturnValue(''); | |
| expect(() => { | |
| detectClaudePath(); | |
| }).toThrow('CLAUDE_CODE_PATH is set but the path does not exist: claude-custom'); | |
| expect(mockExecSync).toHaveBeenCalledWith( | |
| 'which claude-custom', | |
| expect.objectContaining({ | |
| encoding: 'utf-8', | |
| timeout: 5000, | |
| }) | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/__tests__/env-validation.test.ts` around lines 172 - 180, The test
should also verify that detectClaudePath() invokes which with the actual
CLAUDE_CODE_PATH value instead of a hardcoded default: keep the existing
expect(() => detectClaudePath()).toThrow(...) assertion, and add an assertion
that mockExecSync was called with the command string constructed from
process.env.CLAUDE_CODE_PATH (e.g.
expect(mockExecSync).toHaveBeenCalledWith('which claude-custom') or matching the
dynamic `"which ${process.env.CLAUDE_CODE_PATH}"`), optionally asserting call
count to ensure only the expected which invocation occurred; reference
detectClaudePath, mockExecSync and CLAUDE_CODE_PATH to locate where to add this
assertion.
Comment on lines
+232
to
+234
| expect(message).toContain('修復方法'); | ||
| expect(message).toContain('npx drizzle-kit push'); | ||
| expect(message).toContain('npx claude-work'); |
There was a problem hiding this comment.
修復コマンドの期待値が現在の運用手順とズレる可能性があります。
Line 233 で npx drizzle-kit push を固定しているため、運用標準の npm run db:push にメッセージを揃えたときにテストが不要に壊れます。期待値を運用手順に合わせるか、移行期間中は両方許容する形にしてください。
Based on learnings: Database migrations require updating src/db/schema.ts, then running npm run db:push to apply changes to SQLite; use npm run db:generate only for generating optional migration files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/__tests__/schema-check.test.ts` around lines 232 - 234, The test is
asserting the obsolete fixed command 'npx drizzle-kit push' (via the expect on
the variable message) which diverges from current operations; update the
assertions in src/lib/__tests__/schema-check.test.ts so the message either
includes 'npm run db:push' instead of 'npx drizzle-kit push' or accepts both
forms (e.g., change the single expect against message to allow /(?:npx
drizzle-kit push|npm run db:push)/ or check message.includes for either string),
keeping the existing checks for '修復方法' and 'npx claude-work' intact.
Comment on lines
+45
to
+46
| // 0.99 * 51 = 50.49 -> floor = 50 -> リストの最後の方の要素 | ||
| // 0.99 / 51 = 0.019... -> floor = 0 -> 常に最初の要素 ('swift') |
There was a problem hiding this comment.
コメント内の配列長が実装と不一致です。
Line 45-46 の 51 は実データ（50要素）と一致していません。テスト意図の誤読を防ぐため、コメントを 50 ベースに修正してください。
修正例
- // 0.99 * 51 = 50.49 -> floor = 50 -> リストの最後の方の要素
- // 0.99 / 51 = 0.019... -> floor = 0 -> 常に最初の要素 ('swift')
+ // 0.99 * 50 = 49.5 -> floor = 49 -> リストの最後の方の要素
+ // 0.99 / 50 = 0.0198 -> floor = 0 -> 常に最初の要素 ('swift')As per coding guidelines "src/**/*.{js,ts,tsx}: Ensure code comments describing best practices or anti-patterns are aligned with the actual implementation; if implementation contradicts a comment, fix the code or update the comment during code review".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // 0.99 * 51 = 50.49 -> floor = 50 -> リストの最後の方の要素 | |
| // 0.99 / 51 = 0.019... -> floor = 0 -> 常に最初の要素 ('swift') | |
| // 0.99 * 50 = 49.5 -> floor = 49 -> リストの最後の方の要素 | |
| // 0.99 / 50 = 0.0198 -> floor = 0 -> 常に最初の要素 ('swift') |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib/__tests__/session-name-generator.test.ts` around lines 45 - 46, The
comment in session-name-generator.test.ts uses the wrong array length literal
"51" which doesn't match the actual 50-element data; update the comment to use
50 and adjust the example calculations accordingly (e.g., 0.99 * 50 = 49.5 ->
floor = 49 and 0.99 / 50 = 0.0198... -> floor = 0) so the comment correctly
reflects the implementation for the session name generation test.
Comment on lines
+357
to
+371
| describe('inspectImage error handling', () => { | ||
| it('should throw on error without logging', async () => { | ||
| const mockImage = { inspect: vi.fn().mockRejectedValue(new Error('not found')) }; | ||
| mockDockerInstance.getImage.mockReturnValue(mockImage); | ||
| await expect(dockerClient.inspectImage('nonexistent')).rejects.toThrow('not found'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('inspectContainer error handling', () => { | ||
| it('should throw on error without logging', async () => { | ||
| const mockContainer = { inspect: vi.fn().mockRejectedValue(new Error('not found')) }; | ||
| mockDockerInstance.getContainer.mockReturnValue(mockContainer); | ||
| await expect(dockerClient.inspectContainer('nonexistent')).rejects.toThrow('not found'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
without logging を名乗るテストで未ログ出力の検証が不足しています
テスト名は「ログしない」挙動を期待していますが、mockLogger.error の未呼び出しを検証していないため、将来ログが追加されてもテストが通ってしまいます。not.toHaveBeenCalled() を追加してください。
As per coding guidelines src/**/*.{js,ts,tsx}: Ensure code comments describing best practices or anti-patterns are aligned with the actual implementation; if implementation contradicts a comment, fix the code or update the comment during code review.
✅ 修正例
describe('inspectImage error handling', () => {
it('should throw on error without logging', async () => {
const mockImage = { inspect: vi.fn().mockRejectedValue(new Error('not found')) };
mockDockerInstance.getImage.mockReturnValue(mockImage);
await expect(dockerClient.inspectImage('nonexistent')).rejects.toThrow('not found');
+ expect(mockLogger.error).not.toHaveBeenCalled();
});
});
describe('inspectContainer error handling', () => {
it('should throw on error without logging', async () => {
const mockContainer = { inspect: vi.fn().mockRejectedValue(new Error('not found')) };
mockDockerInstance.getContainer.mockReturnValue(mockContainer);
await expect(dockerClient.inspectContainer('nonexistent')).rejects.toThrow('not found');
+ expect(mockLogger.error).not.toHaveBeenCalled();
});
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/docker-client.test.ts` around lines 357 - 371, The
tests for inspectImage and inspectContainer currently assert that the methods
throw but do not verify that no logging occurred; update the two test cases in
docker-client.test.ts (the "inspectImage error handling" and "inspectContainer
error handling" it blocks) to also assert that mockLogger.error was not called
by adding expect(mockLogger.error).not.toHaveBeenCalled() after the await
expect(...).rejects.toThrow(...) assertions so the tests will fail if logging is
added.
Comment on lines
+404
to
415
| it('should use SHELL env var when available on non-win32', async () => { | ||
| const pty = await import('node-pty'); | ||
| const originalShell = process.env.SHELL; | ||
| process.env.SHELL = '/usr/bin/zsh'; | ||
|
|
||
| ptyManager.createPTY(testSessionId, testWorkingDir); | ||
|
|
||
| const shellArg = vi.mocked(pty.spawn).mock.calls[0][0]; | ||
| expect(shellArg).toBe('/usr/bin/zsh'); | ||
|
|
||
| process.env.SHELL = originalShell; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the test file structure and read the specific lines
wc -l src/services/__tests__/pty-manager.test.tsRepository: windschord/claude-work
Length of output: 112
🏁 Script executed:
# Read the test file around lines 404-415
sed -n '395,425p' src/services/__tests__/pty-manager.test.tsRepository: windschord/claude-work
Length of output: 814
🏁 Script executed:
# Check for other tests and their platform guard patterns in the file
rg -n "process.platform" src/services/__tests__/pty-manager.test.tsRepository: windschord/claude-work
Length of output: 48
🏁 Script executed:
# Check the actual import and test setup
sed -n '1,50p' src/services/__tests__/pty-manager.test.tsRepository: windschord/claude-work
Length of output: 1256
🏁 Script executed:
# Find the pty-manager implementation
fd -e ts -e js "pty-manager" src/services --type f | grep -v testRepository: windschord/claude-work
Length of output: 93
🏁 Script executed:
# Read the createPTY implementation
sed -n '1,100p' src/services/pty-manager.tsRepository: windschord/claude-work
Length of output: 2355
🏁 Script executed:
# Search for where SHELL is used and platform handling
rg -n "SHELL|process.platform" src/services/pty-manager.ts -A 3 -B 3Repository: windschord/claude-work
Length of output: 623
non-win32 前提テストに実行時ガードがありません。
テスト名は non-win32 前提ですが、実装（pty-manager.ts:120-126）で os.platform() === 'win32' により Windows では 'powershell.exe' を使用するのに対し、このテストは process.env.SHELL の値を期待しています。Windows実行時にはテストが失敗するため、プラットフォームガードを追加してください。
差分例
it('should use SHELL env var when available on non-win32', async () => {
+ if (process.platform === 'win32') return;
const pty = await import('node-pty');
const originalShell = process.env.SHELL;
process.env.SHELL = '/usr/bin/zsh';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/pty-manager.test.ts` around lines 404 - 415, The test
assumes a non-Windows environment but lacks a runtime guard, causing failures on
win32; update the test in pty-manager.test.ts to check the platform (use
os.platform() or process.platform) and skip or return early when platform ===
'win32' before setting process.env.SHELL and calling ptyManager.createPTY; keep
the existing assertions for pty.spawn and restore process.env.SHELL in the
non-win32 path only so the test only runs where the code path in
ptyManager.createPTY (which picks 'powershell.exe' on win32) matches the
expectation.
Comment on lines
+198
to
+225
| it('should send SIGKILL when timeout fires and process still exists', async () => { | ||
| // setTimeoutのコールバックを即座に実行するようにモック | ||
| let timeoutCallback: (() => void) | null = null; | ||
| const originalSetTimeout = globalThis.setTimeout; | ||
| vi.spyOn(global, 'setTimeout').mockImplementation((fn: any, delay: any) => { | ||
| if (delay === 5000) { | ||
| timeoutCallback = fn; | ||
| return 999 as any; | ||
| } | ||
| return originalSetTimeout(fn, delay); | ||
| }); | ||
|
|
||
| const options: RunOptions = { | ||
| sessionId: 'test-session', | ||
| workingDirectory: '/path/to/worktree', | ||
| command: 'npm test', | ||
| }; | ||
|
|
||
| const runId = await runScriptManager.runScript(options); | ||
| await runScriptManager.stop(runId); | ||
|
|
||
| // タイムアウトコールバックを手動実行 | ||
| expect(timeoutCallback).not.toBeNull(); | ||
| timeoutCallback!(); | ||
|
|
||
| expect(mockChildProcess.kill).toHaveBeenCalledWith('SIGKILL'); | ||
|
|
||
| vi.restoreAllMocks(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the test file
find . -name "run-script-manager.test.ts" -type fRepository: windschord/claude-work
Length of output: 117
🏁 Script executed:
# Once found, examine the test file content around lines 198-225 and 228-244
cat -n src/services/__tests__/run-script-manager.test.ts | sed -n '190,250p'Repository: windschord/claude-work
Length of output: 2449
🏁 Script executed:
# Also check if there are other setTimeout/clearTimeout spies in the file
rg "spyOn.*setTimeout|spyOn.*clearTimeout" src/services/__tests__/run-script-manager.test.ts -A 5 -B 2Repository: windschord/claude-work
Length of output: 1035
🏁 Script executed:
# Check the entire test file structure to understand spy usage patterns
wc -l src/services/__tests__/run-script-manager.test.tsRepository: windschord/claude-work
Length of output: 119
🏁 Script executed:
# Check if there are subprocess spawning calls in this test file
rg "spawnSync|execSync|spawn|exec" src/services/__tests__/run-script-manager.test.ts -B 2 -A 3Repository: windschord/claude-work
Length of output: 2495
🏁 Script executed:
# Check the implementation file to understand what the tests are testing
cat -n src/services/run-script-manager.ts | head -100Repository: windschord/claude-work
Length of output: 2917
🏁 Script executed:
# Check if spawn calls are being asserted with cwd/env checks
rg "expect.*spawn.*cwd|expect.*spawn.*env|expect.*mockSpawn" src/services/__tests__/run-script-manager.test.ts -B 2 -A 2Repository: windschord/claude-work
Length of output: 542
🏁 Script executed:
# Get more context around the stop() tests to understand the full test structure
cat -n src/services/__tests__/run-script-manager.test.ts | sed -n '165,245p'Repository: windschord/claude-work
Length of output: 3259
タイマーのスパイは finally ブロックで必ず復元してください
線202と線229のスパイ設定は、途中でアサーションが失敗すると復元処理に到達せず、後続テストへ影響します。try/finally で復元を保証してください。
修正案
it('should send SIGKILL when timeout fires and process still exists', async () => {
// setTimeoutのコールバックを即座に実行するようにモック
let timeoutCallback: (() => void) | null = null;
const originalSetTimeout = globalThis.setTimeout;
- vi.spyOn(global, 'setTimeout').mockImplementation((fn: any, delay: any) => {
+ const setTimeoutSpy = vi.spyOn(global, 'setTimeout').mockImplementation((fn: any, delay: any) => {
if (delay === 5000) {
timeoutCallback = fn;
return 999 as any;
}
return originalSetTimeout(fn, delay);
});
- const options: RunOptions = {
- sessionId: 'test-session',
- workingDirectory: '/path/to/worktree',
- command: 'npm test',
- };
-
- const runId = await runScriptManager.runScript(options);
- await runScriptManager.stop(runId);
-
- // タイムアウトコールバックを手動実行
- expect(timeoutCallback).not.toBeNull();
- timeoutCallback!();
-
- expect(mockChildProcess.kill).toHaveBeenCalledWith('SIGKILL');
-
- vi.restoreAllMocks();
+ try {
+ const options: RunOptions = {
+ sessionId: 'test-session',
+ workingDirectory: '/path/to/worktree',
+ command: 'npm test',
+ };
+
+ const runId = await runScriptManager.runScript(options);
+ await runScriptManager.stop(runId);
+
+ // タイムアウトコールバックを手動実行
+ expect(timeoutCallback).not.toBeNull();
+ timeoutCallback!();
+
+ expect(mockChildProcess.kill).toHaveBeenCalledWith('SIGKILL');
+ } finally {
+ setTimeoutSpy.mockRestore();
+ }
});
it('should clear kill timeout when process exits during stop', async () => {
const clearTimeoutSpy = vi.spyOn(global, 'clearTimeout');
- const options: RunOptions = {
- sessionId: 'test-session',
- workingDirectory: '/path/to/worktree',
- command: 'npm test',
- };
-
- const runId = await runScriptManager.runScript(options);
- await runScriptManager.stop(runId);
-
- // プロセスが終了するとclearTimeoutが呼ばれる
- mockChildProcess.emit('exit', 0, null);
-
- expect(clearTimeoutSpy).toHaveBeenCalled();
- clearTimeoutSpy.mockRestore();
+ try {
+ const options: RunOptions = {
+ sessionId: 'test-session',
+ workingDirectory: '/path/to/worktree',
+ command: 'npm test',
+ };
+
+ const runId = await runScriptManager.runScript(options);
+ await runScriptManager.stop(runId);
+
+ // プロセスが終了するとclearTimeoutが呼ばれる
+ mockChildProcess.emit('exit', 0, null);
+
+ expect(clearTimeoutSpy).toHaveBeenCalled();
+ } finally {
+ clearTimeoutSpy.mockRestore();
+ }
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/__tests__/run-script-manager.test.ts` around lines 198 - 225,
The test "should send SIGKILL when timeout fires and process still exists" sets
a spy on global.setTimeout but restores mocks only at the end, which can leak if
assertions fail; wrap the spy setup and the test logic that uses
timeoutCallback/runScriptManager.runScript/runScriptManager.stop inside a
try/finally and call vi.restoreAllMocks() (or explicitly restore the setTimeout
spy) in the finally block so the global timer is always restored even on test
failure.
Comment on lines
+341
to
+351
| it('should exclude CLAUDECODE from environment', () => { | ||
| const originalEnv = process.env; | ||
| process.env = { ...originalEnv, CLAUDECODE: 'should-be-excluded' }; | ||
|
|
||
| adapter.testSpawnPTY('claude', [], {}); | ||
|
|
||
| const spawnCall = vi.mocked(pty.spawn).mock.calls[0]; | ||
| const envArg = spawnCall[2].env; | ||
| expect(envArg).not.toHaveProperty('CLAUDECODE'); | ||
|
|
||
| process.env = originalEnv; |
There was a problem hiding this comment.
process.env の復元を finally で保証したほうが安全です。
このテストはグローバル状態を書き換えているので、途中のアサーション失敗で復元処理に到達しないと後続テストへ漏れます。ここも try/finally で元の process.env を戻しておくと安定します。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/adapters/__tests__/base-adapter.test.ts` around lines 341 - 351,
The test mutates global process.env and doesn't guarantee restoration on
assertion failures; wrap the mutation and assertions in a try/finally so
originalEnv is always restored. Specifically, in the test case around
adapter.testSpawnPTY('claude', [], {}), save const originalEnv = process.env,
set process.env = {...originalEnv, CLAUDECODE: 'should-be-excluded'} inside the
try block, run adapter.testSpawnPTY and the spawn/expect assertions, and perform
process.env = originalEnv inside the finally block to ensure cleanup even on
failure.
| */ | ||
| async setRules(sourceIP: string, entries: ProxyRuleEntry[]): Promise<ProxyRuleSet> { | ||
| const url = `${this.baseUrl}/api/v1/rules/${sourceIP}`; | ||
| const url = `${this.baseUrl}/api/v1/rules/${encodeURIComponent(sourceIP)}`; |
There was a problem hiding this comment.
この URL エンコード変更を区別できる回帰テストも欲しいです。
encodeURIComponent() の追加自体は必要ですが、現状の proxy-client.test.ts は IPv4 しか使っていないので、実装からエンコードが消えても通ります。2001:db8::1 のように : を含む値で、fetch に渡す URL がエンコード済みであることまで固定しておくと、この修正をちゃんと守れます。
Also applies to: 201-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/proxy-client.ts` at line 147, Add a regression test in
proxy-client.test.ts that ensures the URL built in src/services/proxy-client.ts
is percent-encoded for values with colons: call the ProxyClient method that
constructs the URL (the code path that creates const url =
`${this.baseUrl}/api/v1/rules/${encodeURIComponent(sourceIP)}`) using an IPv6
address like "2001:db8::1", mock/fake fetch, and assert that fetch was invoked
with the encoded URL (i.e. encodeURIComponent("2001:db8::1") or contains "%3A")
so the test fails if encodeURIComponent is removed or omitted.
レビュー指摘（PR #229）に対応: resize転送テストのif/elseフォールバックパターンを 除去し、messageHandlerが必ず登録されることをassertする形に修正。 input転送テストと同じパターンに統一。 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces mutation testing (Stryker + Vitest runner) and expands/improves unit test coverage across src/services and src/lib, along with a CI job that runs mutation tests only against PR-changed files with an 85% quality gate.
Changes:
- Add Stryker configuration + a dedicated Vitest config for mutation runs, and a
test:mutationscript. - Add a GitHub Actions
mutation-testjob that targets changedsrc/services/**/*.ts/src/lib/**/*.tsfiles on PRs. - Add/extend many unit tests and a few small code adjustments (e.g., URL encoding, timeout handling, email validation, volume name trimming) to improve mutation score.
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | Adds a Vitest configuration intended for Stryker runs. |
| stryker.config.json | Adds Stryker mutation testing configuration (mutate targets, thresholds, reporters). |
| src/services/remote-repo-service.ts | Minor refactor to avoid clearing an uninitialized timeout. |
| src/services/proxy-client.ts | Encodes sourceIP in rule URLs; clarifies 4xx error semantics in docs. |
| src/services/adapters/tests/host-adapter.test.ts | Reworks mocks and significantly expands HostAdapter tests. |
| src/services/adapters/tests/docker-adapter.test.ts | Improves env var restoration and adds cleanup/assertion cases. |
| src/services/adapters/tests/base-adapter.test.ts | Adds tests for session id extraction and arg/env sanitization/logging. |
| src/services/tests/scrollback-buffer.test.ts | Expands edge-case coverage for trimming/UTF-8 boundaries/compaction/env var sizing. |
| src/services/tests/run-script-manager.test.ts | Adds stop/kill-timeout behavior tests and spawn/exit cleanup tests. |
| src/services/tests/remote-repo-service.test.ts | Adds many edge-case tests and improves env var cleanup. |
| src/services/tests/pty-manager.test.ts | Expands PTYManager behavior tests (env filtering, guards, spawn options). |
| src/services/tests/proxy-client.test.ts | Adds extensive error-handling/retry/message tests for ProxyClient. |
| src/services/tests/port-checker.test.ts | Adds parsing fallback tests and more checkSinglePort matrix cases. |
| src/services/tests/github-pat-service.test.ts | Adds DB insert/update assertion tests and encryption/decryption error-message cases. |
| src/services/tests/git-operations.test.ts | Adds unit tests for GitOperationError. |
| src/services/tests/gh-cli.test.ts | Adds exec mocking and expands tests for gh CLI wrapper functions. |
| src/services/tests/environment-adapter.test.ts | Adds more negative/edge tests for the type guard. |
| src/services/tests/encryption-service.test.ts | Adds error class tests and more validation/error-message coverage. |
| src/services/tests/docker-pty-adapter.test.ts | Adds logger mocking, env restoration, and broadens DockerPTYAdapter behavior tests. |
| src/services/tests/docker-git-service.test.ts | Adds extensive success/error/logging/parse coverage for DockerGitService. |
| src/services/tests/docker-client.test.ts | Adds logger mocking and expands DockerClient API + error handling tests. |
| src/services/tests/developer-settings-service.test.ts | Adds SettingsNotFoundError tests and message assertions. |
| src/services/tests/config-service.test.ts | Expands ConfigService tests for defaults, logging, save/load behavior, singleton helpers. |
| src/services/tests/adapter-factory.test.ts | Adds logging assertions, invalid-config handling, and cache access tests. |
| src/lib/websocket/tests/session-ws.test.ts | Adds comprehensive tests for SessionWebSocketHandler event forwarding and connection handling. |
| src/lib/validation.ts | Tightens email local-part validation (dot rules). |
| src/lib/docker-volume-utils.ts | Trims environmentId before deriving volume names. |
| src/lib/tests/validation.test.ts | Adds many validation tests (notably email/PAT/repo URL cases). |
| src/lib/tests/session.test.ts | Adds tests for session options behavior across NODE_ENV/SESSION_SECRET scenarios. |
| src/lib/tests/session-name-generator.test.ts | Adds deterministic tests to catch operator/boundary mutations. |
| src/lib/tests/schema-check.test.ts | Adds broader schema integrity tests and message formatting assertions. |
| src/lib/tests/run-scripts.test.ts | Adds unit tests for parse/serialize helpers. |
| src/lib/tests/proxy-sync.test.ts | Adds edge cases and logging assertions for proxy rule syncing. |
| src/lib/tests/path-safety.test.ts | Adds tests for sanitization and base-path containment/safe component checks. |
| src/lib/tests/notification-service.test.ts | Adds default/fallback cases, localStorage error handling, and more OS notification assertions. |
| src/lib/tests/errors.test.ts | Adds logger mocking and expands API error handling/logging assertions. |
| src/lib/tests/env-validation.test.ts | Expands error message/edge cases for required env vars and Claude path detection. |
| src/lib/tests/docker-volume-utils.test.ts | Adds unit tests for volume-name helper behavior. |
| src/lib/tests/docker-config-validator.test.ts | Expands validation tests for ports/volumes and error/warning messages. |
| src/lib/tests/db-module.test.ts | Adds module-level export/smoke tests for the db module. |
| src/lib/tests/action-detector.test.ts | Adds more ANSI stripping/action detection/cooldown boundary tests. |
| src/app/api/environments/tests/route.test.ts | Removes network-filter auto-apply related tests/mocks from this suite. |
| src/app/api/environments/[id]/network-rules/tests/route.test.ts | Removes proxy-sync invocation tests/mocks from this suite. |
| src/app/api/environments/[id]/network-filter/tests/route.test.ts | Removes proxy-sync invocation tests/mocks from this suite. |
| package.json | Adds Stryker dev dependencies and a test:mutation script. |
| .gitignore | Ignores Stryker output directories (reports/mutation/, .stryker-tmp/). |
| .github/workflows/test.yml | Adds mutation-test job and wires it into CI gating (skipped allowed outside PRs). |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/lib/tests/validation.test.ts:347
isValidEmailのテストが同一ファイル内で2回describe('isValidEmail', ...)として重複しており、同じケースが二重に実行されています。片方に統合して重複ブロック（後半の describe）を削除し、テストの実行時間とメンテ負荷を下げてください。
describe('isValidEmail', () => {
it('有効なメールアドレスを受け入れる', () => {
expect(isValidEmail('user@example.com')).toBe(true);
expect(isValidEmail('user@sub.example.com')).toBe(true);
expect(isValidEmail('user+tag@example.com')).toBe(true);
});
it('ドメイン部分に連続ドットを含むアドレスを拒否する', () => {
expect(isValidEmail('user@foo..bar')).toBe(false);
});
it('ドメインがドットで始まるアドレスを拒否する', () => {
expect(isValidEmail('user@.foo')).toBe(false);
expect(isValidEmail('user@.foo.com')).toBe(false);
});
it('ドメインにTLDが無いアドレスを拒否する', () => {
expect(isValidEmail('user@localhost')).toBe(false);
});
it('@が無いアドレスを拒否する', () => {
expect(isValidEmail('userexample.com')).toBe(false);
});
it('空文字列を拒否する', () => {
expect(isValidEmail('')).toBe(false);
});
it('254文字を超えるアドレスを拒否する', () => {
const longLocal = 'a'.repeat(64);
const longDomain = 'b'.repeat(250) + '.com';
expect(isValidEmail(`${longLocal}@${longDomain}`)).toBe(false);
});
it('ドメインラベルがハイフンで始まる場合を拒否する', () => {
expect(isValidEmail('user@-example.com')).toBe(false);
});
it('ドメインラベルがハイフンで終わる場合を拒否する', () => {
expect(isValidEmail('user@example-.com')).toBe(false);
});
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 290-292: Two important tests were left disabled with it.skip
because the asynchronous registration of the WS connection handler (which calls
addConnection) can finish after the test proceeds; re-enable those tests (remove
it.skip) and add a small helper that waits for the handler registration using
EventEmitter.listenerCount before continuing. Specifically, implement and use a
waitForHandler helper in the terminal-ws.test.ts tests to poll or await
socketServer.listenerCount('connection') (or the emitter that registers
addConnection) becoming >= the expected count, then proceed to send input to
ptyManager and assert cleanup on close; reference the addConnection registration
point and the ptyManager interaction so the test only continues after
listenerCount confirms the handler is registered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b90a332f-408a-47de-a197-6d68124cd20c
📒 Files selected for processing (2)
data/test.db-shmsrc/lib/websocket/__tests__/terminal-ws.test.ts
- proxy-client.ts: setRules/deleteRules/deleteAllRulesの@throwsタグを ProxyValidationError（4xx全般）に統一 - proxy-client.test.ts: IPv6アドレス（2001:db8::1）のURLエンコード検証テストを setRulesとdeleteRulesに追加 - terminal-ws.test.ts: waitForListenerヘルパーを導入し、input転送と close時クリーンアップのit.skipを解除 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/__tests__/proxy-client.test.ts (1)
248-253:⚠️ Potential issue | 🟡 Minorこのテストは例外が投げられない場合でも成功してしまいます。
try/catchブロックを使用していますが、setRules()が例外をスローせずに成功した場合、テストはアサーションなしで通過します。await expect(...).rejectsパターンを使用することで、確実に例外がスローされることを検証できます。🛡️ 修正案
- try { - await client.setRules('172.20.0.3', [{ host: 'invalid' }]); - } catch (err) { - expect(err).toBeInstanceOf(ProxyValidationError); - expect((err as ProxyValidationError).details).toEqual(errorBody.details); - } + await expect(client.setRules('172.20.0.3', [{ host: 'invalid' }])) + .rejects.toSatisfy((err: Error) => { + return err instanceof ProxyValidationError && + JSON.stringify((err as ProxyValidationError).details) === JSON.stringify(errorBody.details); + });または、より簡潔に:
- try { - await client.setRules('172.20.0.3', [{ host: 'invalid' }]); - } catch (err) { - expect(err).toBeInstanceOf(ProxyValidationError); - expect((err as ProxyValidationError).details).toEqual(errorBody.details); - } + const error = await client.setRules('172.20.0.3', [{ host: 'invalid' }]).catch((e) => e); + expect(error).toBeInstanceOf(ProxyValidationError); + expect(error.details).toEqual(errorBody.details);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/proxy-client.test.ts` around lines 248 - 253, The test uses a try/catch which lets it pass if setRules('172.20.0.3', [{ host: 'invalid' }]) does not throw; change it to use Jest's promise rejection matcher: await expect(client.setRules('172.20.0.3', [{ host: 'invalid' }])).rejects.toBeInstanceOf(ProxyValidationError) and then assert the error payload (e.g. await expect(...).rejects.toMatchObject({ details: errorBody.details }) or chain a second awaits to verify (err as ProxyValidationError).details equals errorBody.details) so the test fails when no exception is thrown and still verifies the details.
🧹 Nitpick comments (3)
src/services/__tests__/proxy-client.test.ts (2)
427-438: テストを1回の呼び出しに統合できます。同じエラー条件に対して
healthCheck()を2回呼び出していますが、エラーをキャプチャして複数のアサーションを行う方がシンプルです。♻️ 改善案
it('不正なJSONレスポンスでProxyConnectionErrorをスローする', async () => { vi.mocked(fetch).mockResolvedValueOnce( new Response('not json{{', { status: 200, headers: { 'Content-Type': 'text/plain' } }) ); - await expect(client.healthCheck()).rejects.toThrow(ProxyConnectionError); - vi.mocked(fetch).mockResolvedValueOnce( - new Response('not json{{', { status: 200, headers: { 'Content-Type': 'text/plain' } }) - ); - await expect(client.healthCheck()).rejects.toThrow(/パースできません/); + await expect(client.healthCheck()).rejects.toSatisfy((err: Error) => { + return err instanceof ProxyConnectionError && /パースできません/.test(err.message); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/proxy-client.test.ts` around lines 427 - 438, 統合されたテストにするため、同じモック応答を2回呼ぶ代わりに一度だけ vi.mocked(fetch).mockResolvedValueOnce(new Response('not json{{', { status: 200, headers: { 'Content-Type': 'text/plain' } })) をセットし、client.healthCheck() を try/catch で1回呼び出して catch したエラーを変数に保持し、そのエラーが ProxyConnectionError のインスタンスであることとエラーメッセージが /パースできません/ にマッチすることを 2つのアサーションで検証するように修正してください（参照: client.healthCheck(), ProxyConnectionError, vi.mocked(fetch).mockResolvedValueOnce）。
401-411: テスト効率の改善案: 2回のメソッド呼び出しを1回に統合できます。現在の実装では
deleteAllRules()を2回呼び出していますが、各呼び出しでリトライロジック（3回試行）が実行されます。1回の呼び出しで両方の条件を検証する方が効率的です。♻️ 改善案
it('500エラー時はProxyValidationErrorではなくProxyConnectionErrorをスローする', async () => { const originalSetTimeout = globalThis.setTimeout; vi.stubGlobal('setTimeout', (fn: () => void) => originalSetTimeout(fn, 0)); try { vi.mocked(fetch).mockResolvedValue(new Response('Error', { status: 500 })); - await expect(client.deleteAllRules()).rejects.toThrow(ProxyConnectionError); - await expect(client.deleteAllRules()).rejects.not.toThrow(ProxyValidationError); + const error = await client.deleteAllRules().catch((e) => e); + expect(error).toBeInstanceOf(ProxyConnectionError); + expect(error).not.toBeInstanceOf(ProxyValidationError); } finally { vi.unstubAllGlobals(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/proxy-client.test.ts` around lines 401 - 411, Replace the two separate awaits of client.deleteAllRules() with a single invocation and assert both type checks on the caught error: call client.deleteAllRules() once (keeping the existing vi.mocked(fetch).mockResolvedValue and vi.stubGlobal handling), await the promise and catch the thrown error (e.g. try/catch or .catch), then assert that the error is an instance of ProxyConnectionError and not an instance of ProxyValidationError using the same assertions but against the single caught error; this avoids triggering the retry logic twice while still verifying both conditions for deleteAllRules().src/lib/websocket/__tests__/terminal-ws.test.ts (1)
311-330: resize テストもwaitForListener()+ws.emit()に揃えると保守しやすいです。このケースだけ
ws.onを差し替えて handler を直接叩いており、input / close テストと検証経路が分かれています。await waitForListener(ws, 'message')の後にws.emit('message', ...)を使う形に揃えると、同期化方法も実行経路も一貫します。♻️ 例
const ws = createMockWs(); - let messageHandler: Function | null = null; - const originalOn = ws.on.bind(ws); - ws.on = vi.fn((event: string, handler: Function) => { - if (event === 'message') messageHandler = handler; - return originalOn(event, handler); - }); - localWss.emit('connection', ws, createMockReq('session-1')); - for (let i = 0; i < 5; i++) await flush(); - - // ハンドラーが必ず登録されることを確認 - expect(messageHandler).not.toBeNull(); - messageHandler!(Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); + await waitForListener(ws, 'message'); + ws.emit('message', Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); expect(mockPtyManager.resize).toHaveBeenCalledWith('session-1-terminal', 100, 50);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/websocket/__tests__/terminal-ws.test.ts` around lines 311 - 330, The test should be refactored to use the existing waitForListener pattern instead of overriding ws.on and calling messageHandler directly: remove the custom ws.on override and the messageHandler variable, call await waitForListener(ws, 'message') after localWss.emit('connection', ws, createMockReq('session-1')) and flush, then trigger the handler via ws.emit('message', Buffer.from(JSON.stringify({ type: 'resize', data: { cols: 100, rows: 50 } }))); keep the assertion that mockPtyManager.resize was called with ('session-1-terminal', 100, 50) so setupTerminalWebSocket and mockPtyManager.resize remain the targets for locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 281-295: The setupConnection helper currently returns the mock ws
before the terminal 'message' handler is guaranteed to be registered; update
setupConnection (the async function in terminal-ws.test.ts) to await
waitForListener(ws, 'message') after the two flush() calls and before returning
ws so tests that emit 'message' always exercise the real handler path; keep the
existing session/environment mocking (mockFindFirst and
environmentService.findById) as-is and only add the await waitForListener(ws,
'message') step to ensure handlers are attached.
---
Duplicate comments:
In `@src/services/__tests__/proxy-client.test.ts`:
- Around line 248-253: The test uses a try/catch which lets it pass if
setRules('172.20.0.3', [{ host: 'invalid' }]) does not throw; change it to use
Jest's promise rejection matcher: await expect(client.setRules('172.20.0.3', [{
host: 'invalid' }])).rejects.toBeInstanceOf(ProxyValidationError) and then
assert the error payload (e.g. await expect(...).rejects.toMatchObject({
details: errorBody.details }) or chain a second awaits to verify (err as
ProxyValidationError).details equals errorBody.details) so the test fails when
no exception is thrown and still verifies the details.
---
Nitpick comments:
In `@src/lib/websocket/__tests__/terminal-ws.test.ts`:
- Around line 311-330: The test should be refactored to use the existing
waitForListener pattern instead of overriding ws.on and calling messageHandler
directly: remove the custom ws.on override and the messageHandler variable, call
await waitForListener(ws, 'message') after localWss.emit('connection', ws,
createMockReq('session-1')) and flush, then trigger the handler via
ws.emit('message', Buffer.from(JSON.stringify({ type: 'resize', data: { cols:
100, rows: 50 } }))); keep the assertion that mockPtyManager.resize was called
with ('session-1-terminal', 100, 50) so setupTerminalWebSocket and
mockPtyManager.resize remain the targets for locating the change.
In `@src/services/__tests__/proxy-client.test.ts`:
- Around line 427-438: 統合されたテストにするため、同じモック応答を2回呼ぶ代わりに一度だけ
vi.mocked(fetch).mockResolvedValueOnce(new Response('not json{{', { status: 200,
headers: { 'Content-Type': 'text/plain' } })) をセットし、client.healthCheck() を
try/catch で1回呼び出して catch したエラーを変数に保持し、そのエラーが ProxyConnectionError
のインスタンスであることとエラーメッセージが /パースできません/ にマッチすることを 2つのアサーションで検証するように修正してください（参照:
client.healthCheck(), ProxyConnectionError,
vi.mocked(fetch).mockResolvedValueOnce）。
- Around line 401-411: Replace the two separate awaits of
client.deleteAllRules() with a single invocation and assert both type checks on
the caught error: call client.deleteAllRules() once (keeping the existing
vi.mocked(fetch).mockResolvedValue and vi.stubGlobal handling), await the
promise and catch the thrown error (e.g. try/catch or .catch), then assert that
the error is an instance of ProxyConnectionError and not an instance of
ProxyValidationError using the same assertions but against the single caught
error; this avoids triggering the retry logic twice while still verifying both
conditions for deleteAllRules().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0675dcf1-1915-4ba4-8fbe-982e36cd8129
📒 Files selected for processing (3)
src/lib/websocket/__tests__/terminal-ws.test.tssrc/services/__tests__/proxy-client.test.tssrc/services/proxy-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/proxy-client.ts
- proxy-client.test.ts: try/catchパターンを.catch(e=>e)+expectパターンに変更 - proxy-client.test.ts: healthCheck/deleteAllRulesの2回呼び出しを1回に統合 - terminal-ws.test.ts: resizeテストをwaitForListenerパターンに統一 - terminal-ws.test.ts: setupConnectionヘルパーにwaitForListener追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/services/__tests__/proxy-client.test.ts (1)
248-250:⚠️ Potential issue | 🟡 Minor
.catch()チェーンをawait expect(...).rejectsに統一してください。Line 248 / Line 403 / Line 431 の
.catch((e) => e)はガイドライン違反で、失敗ケース検証の一貫性も落ちます。promiseを変数化してrejectsで検証してください。🔧 修正例
- const error = await client.setRules('172.20.0.3', [{ host: 'invalid' }]).catch((e) => e); - expect(error).toBeInstanceOf(ProxyValidationError); - expect(error.details).toEqual(errorBody.details); + const promise = client.setRules('172.20.0.3', [{ host: 'invalid' }]); + await expect(promise).rejects.toBeInstanceOf(ProxyValidationError); + await expect(promise).rejects.toHaveProperty('details', errorBody.details); - const error = await client.deleteAllRules().catch((e) => e); - expect(error).toBeInstanceOf(ProxyConnectionError); - expect(error).not.toBeInstanceOf(ProxyValidationError); + const promise = client.deleteAllRules(); + await expect(promise).rejects.toBeInstanceOf(ProxyConnectionError); + await expect(promise).rejects.not.toBeInstanceOf(ProxyValidationError); - const error = await client.healthCheck().catch((e) => e); - expect(error).toBeInstanceOf(ProxyConnectionError); - expect(error.message).toMatch(/パースできません/); + const promise = client.healthCheck(); + await expect(promise).rejects.toBeInstanceOf(ProxyConnectionError); + await expect(promise).rejects.toThrow(/パースできません/);#!/bin/bash # `.catch(` チェーンが残っていないことを確認 rg -nP '\.catch\s*\(' src/services/__tests__/proxy-client.test.tsAs per coding guidelines
src/**/*.{ts,tsx}: Always use async/await for promise handling in TypeScript/JavaScript code instead of.then()/.catch()chains.Also applies to: 403-405, 431-433
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/__tests__/proxy-client.test.ts` around lines 248 - 250, Replace the `.catch((e) => e)` pattern by turning the call into a promise and using Jest's `rejects` assertions: e.g., assign const promise = client.setRules('172.20.0.3', [{ host: 'invalid' }]) then await expect(promise).rejects.toBeInstanceOf(ProxyValidationError) and await expect(promise).rejects.toMatchObject({ details: errorBody.details }); apply the same change for the other occurrences that use `.catch((e) => e)` (the calls to client.* at the other noted locations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/__tests__/proxy-client.test.ts`:
- Around line 339-345: The test for client.deleteRules currently asserts that it
throws on a 5xx but doesn't verify retries; add an assertion that the mocked
fetch was called 3 times (use vi.mocked(fetch) and
expect(...).toHaveBeenCalledTimes(3)) after the await expect(...).rejects check
so the test validates the retry behavior of deleteRules.
---
Duplicate comments:
In `@src/services/__tests__/proxy-client.test.ts`:
- Around line 248-250: Replace the `.catch((e) => e)` pattern by turning the
call into a promise and using Jest's `rejects` assertions: e.g., assign const
promise = client.setRules('172.20.0.3', [{ host: 'invalid' }]) then await
expect(promise).rejects.toBeInstanceOf(ProxyValidationError) and await
expect(promise).rejects.toMatchObject({ details: errorBody.details }); apply the
same change for the other occurrences that use `.catch((e) => e)` (the calls to
client.* at the other noted locations).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1277020-22eb-48a4-bb3b-3454cb33b5c9
📒 Files selected for processing (2)
src/lib/websocket/__tests__/terminal-ws.test.tssrc/services/__tests__/proxy-client.test.ts
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Stryker Mutator（vitest-runner）を導入し、CI上でPR差分に対してミューテーションテスト（品質ゲート 85%）を走らせることで、テストの網羅性/堅牢性を継続的に担保するためのPRです。あわせて、src/services / src/lib を中心に多数のユニットテスト拡充と、テスタビリティ向上の軽微な実装修正が含まれます。
Changes:
- Stryker（mutation testing）用の設定ファイル追加と、
npm run test:mutationの追加 - GitHub Actions に PR の変更ファイルのみを対象にした mutation-test ジョブを追加（break=85）
src/services/src/libのテスト拡充、および軽微な実装改善（URLエンコード、メール検証強化、タイムアウト後始末改善、など）
Reviewed changes
Copilot reviewed 48 out of 51 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.stryker.ts | Stryker向け Vitest 設定を新規追加 |
| stryker.config.json | Stryker設定（mutate対象/threshold等）を新規追加 |
| src/services/remote-repo-service.ts | pull処理のタイムアウト後始末を安全化 |
| src/services/proxy-client.ts | rules API のURLパスに encodeURIComponent 適用、4xxエラー説明を更新 |
| src/services/adapters/tests/host-adapter.test.ts | HostAdapter のイベント転送/引数等のテスト拡充 |
| src/services/adapters/tests/docker-adapter.test.ts | env復元や失敗時クリーンアップ等のテスト改善 |
| src/services/adapters/tests/base-adapter.test.ts | sessionId抽出・spawn redaction 等のテスト拡充 |
| src/services/tests/scrollback-buffer.test.ts | バッファ境界/UTF-8境界/圧縮などのテスト拡充 |
| src/services/tests/run-script-manager.test.ts | stop/killタイムアウトやspawn失敗等のテスト拡充 |
| src/services/tests/remote-repo-service.test.ts | env復元の安全化、エッジケース/エラー系テスト拡充 |
| src/services/tests/pty-manager.test.ts | env/リスナーの後始末や作成/例外系のテスト拡充 |
| src/services/tests/proxy-client.test.ts | 接続失敗/HTTPエラー/IPv6などのテスト拡充 |
| src/services/tests/port-checker.test.ts | 不正config/判定優先順位等のテスト拡充 |
| src/services/tests/github-pat-service.test.ts | insert/update引数検証、エラー文言等のテスト拡充 |
| src/services/tests/git-operations.test.ts | GitOperationError のテスト新規追加 |
| src/services/tests/gh-cli.test.ts | child_processモック導入、gh-cli機能テスト拡充 |
| src/services/tests/environment-adapter.test.ts | isEnvironmentAdapter の否定ケース等テスト拡充 |
| src/services/tests/encryption-service.test.ts | Error class / invalid key 等のテスト拡充 |
| src/services/tests/docker-pty-adapter.test.ts | DockerPTYAdapter の挙動・例外系のテスト拡充 |
| src/services/tests/docker-git-service.test.ts | DockerGitService の機能/例外/ログ等のテスト拡充 |
| src/services/tests/docker-client.test.ts | DockerClient のAPI追加分/例外/ログのテスト拡充 |
| src/services/tests/developer-settings-service.test.ts | SettingsNotFoundError 等のテスト拡充 |
| src/services/tests/config-service.test.ts | logging/シングルトン/merge保存等のテスト拡充 |
| src/services/tests/adapter-factory.test.ts | ログ検証、invalid JSON等のテスト拡充 |
| src/lib/websocket/tests/session-ws.test.ts | Session WS のイベント転送/DB失敗時等のテスト拡充 |
| src/lib/validation.ts | メールローカル部のドット境界を追加で拒否 |
| src/lib/docker-volume-utils.ts | environmentId の trim() を反映してVolume名を生成 |
| src/lib/tests/validation.test.ts | validation系（PAT/email等）のテスト拡充 |
| src/lib/tests/session.test.ts | session設定のテスト新規追加 |
| src/lib/tests/session-name-generator.test.ts | random/attempt境界のテスト拡充 |
| src/lib/tests/schema-check.test.ts | schema整合性/メッセージ整形のテスト拡充 |
| src/lib/tests/run-scripts.test.ts | run-scripts parse/serialize のテスト新規追加 |
| src/lib/tests/proxy-sync.test.ts | port省略/ログ/NULLケース等のテスト拡充 |
| src/lib/tests/path-safety.test.ts | path-safety のテスト新規追加 |
| src/lib/tests/notification-service.test.ts | 設定読み込み/保存失敗/音通知等のテスト拡充 |
| src/lib/tests/errors.test.ts | handleApiError のログ/分岐等テスト拡充 |
| src/lib/tests/env-validation.test.ts | env不足時のメッセージ/which挙動等のテスト拡充 |
| src/lib/tests/docker-volume-utils.test.ts | docker-volume-utils のテスト新規追加 |
| src/lib/tests/docker-config-validator.test.ts | port/volume validation の境界・文言テスト拡充 |
| src/lib/tests/db-module.test.ts | dbモジュール export/基本動作のテスト新規追加 |
| src/lib/tests/action-detector.test.ts | ANSI除去/検出/クールダウン境界のテスト拡充 |
| src/app/api/environments/tests/route.test.ts | network-filter自動適用関連のテスト群を削除 |
| src/app/api/environments/[id]/network-rules/tests/route.test.ts | proxy-sync 呼び出し検証テスト等を削除 |
| src/app/api/environments/[id]/network-filter/tests/route.test.ts | proxy-sync 呼び出し検証テスト等を削除 |
| package.json | test:mutation とStryker依存を追加 |
| .gitignore | Stryker生成物（reports/.stryker-tmp）を追加 |
| .github/workflows/test.yml | PR向け mutation-test ジョブを追加、ci-checkへ統合 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
src/lib/tests/validation.test.ts:307
isValidEmailのテストが同一ファイル内で2つのdescribe('isValidEmail', ...)に分かれており、同じケースが重複実行されています（メンテコスト増・実行時間増）。片方に統合して、追加した新ケース（ローカル部のドット境界など）を既存ブロックへ移す形に整理してください。
});
describe('isValidEmail', () => {
it('有効なメールアドレスを受け入れる', () => {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+7
to
+11
| test: { | ||
| environment: 'jsdom', | ||
| globals: true, | ||
| globalSetup: './vitest.global-setup.ts', | ||
| setupFiles: ['./src/test/setup.ts'], |
…s リトライ回数検証を追加 PR #229 レビュー指摘対応: - 3箇所の .catch((e) => e) を await expect(promise).rejects.toSatisfy に変更 - deleteRules 5xxテストに fetch 呼び出し回数(3回)の検証を追加 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
変更内容
ミューテーションテスト環境
@stryker-mutator/core+@stryker-mutator/vitest-runnerを導入stryker.config.jsonとvitest.config.stryker.tsを新規作成npm run test:mutationスクリプトを追加CI/CD
.github/workflows/test.ymlにmutation-testジョブを追加テスト改善 (26ファイル 85%達成)
ソースコード修正 (テスタビリティ向上)
Test plan
npx vitest run src/lib src/services- 1767 passed, 30 skippednpm run build)🤖 Generated with Claude Code
Summary by CodeRabbit
新機能
改善
テスト
その他