[#838] return-interactive-prompt - #853
Open
nrslib wants to merge 1 commit into
Open
Conversation
📝 Walkthroughウォークスルータスク完了後にインタラクティブモードで 変更内容インタラクティブ継続プロンプト機能
シーケンス図sequenceDiagram
actor User
participant runInteractiveLoop
participant selectInteractiveMode
participant selectAndExecuteTask
participant shouldPromptForInteractiveContinue
participant promptContinueAfterTaskResult
rect rgba(100, 149, 237, 0.5)
Note over runInteractiveLoop: インタラクティブループ開始
User->>runInteractiveLoop: takt 起動（通常インタラクティブ実行）
loop ループ継続中
runInteractiveLoop->>selectInteractiveMode: モード選択
selectInteractiveMode-->>runInteractiveLoop: selectedMode
runInteractiveLoop->>selectAndExecuteTask: タスク実行（exitOnFailure: false）
selectAndExecuteTask-->>runInteractiveLoop: SelectAndExecuteTaskResult
runInteractiveLoop->>shouldPromptForInteractiveContinue: 継続プロンプト表示可否判定
shouldPromptForInteractiveContinue-->>runInteractiveLoop: true / false
alt 表示する場合
runInteractiveLoop->>promptContinueAfterTaskResult: Continue? [Y/n] を表示
promptContinueAfterTaskResult-->>User: Task completed/failed\nContinue? [Y/n]
User-->>promptContinueAfterTaskResult: y または n
promptContinueAfterTaskResult-->>runInteractiveLoop: true / false
end
alt false（継続しない）
runInteractiveLoop->>runInteractiveLoop: process.exit(1) または正常終了
end
end
end
推定コードレビュー工数🎯 4 (Complex) | ⏱️ ~60 minutes 関連する可能性のあるPR
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli-reference.ja.md`:
- Around line 99-107: The Japanese CLI reference documentation
(docs/cli-reference.ja.md) shows an English continuation prompt "Continue?
[Y/n]" in the execution example output, but the actual implementation uses
getLabel to return the Japanese text "続行しますか？ [Y/n]" to Japanese users. Replace
the English prompt text with the Japanese equivalent "続行しますか？ [Y/n]" in the
execution example to align the documentation with the actual localized output
that users will see.
In `@src/__tests__/cli-routing-issue-resolve.test.ts`:
- Around line 923-936: The test case uses exitSpy.mockRestore() at the end
without a guaranteed cleanup mechanism, which means if the test fails before
reaching that line, the spy won't be restored and will leak side effects to
subsequent tests. Wrap the test body in a try/finally block to ensure
mockRestore() is always called regardless of test success or failure, or
alternatively move the spy creation and cleanup to shared beforeEach/afterEach
hooks. Apply this same pattern consistently to all affected test cases including
those at lines 938-951 and 1046-1059.
In `@src/__tests__/cli-routing-pr-resolve.test.ts`:
- Around line 595-616: The test modifies the pipelineMode property at the
beginning but only restores it at the end without error handling. If the test
execution fails before reaching the restoration code, pipelineMode will remain
modified and break subsequent tests. Wrap the test execution logic and the
pipelineMode restoration in a try-finally block to ensure the original
pipelineMode value is always restored to the originalPipelineMode variable
regardless of whether the test passes or fails during the executeDefaultAction
call and assertions.
In `@src/__tests__/interactiveContinuePrompt.test.ts`:
- Around line 119-127: The test for promptContinueAfterTaskResult with false
input only verifies that mockInfo and mockReadMultilineInput were called, but
does not verify the order in which they were called. To ensure the user-facing
contract is met (failure message displays before the continue prompt), add an
assertion to verify that mockInfo is called before mockReadMultilineInput. Use
Jest's toHaveBeenCalledBefore matcher or check the call order by examining the
call indices of both mocks to confirm the failure status message appears before
the continue prompt question.
In `@src/__tests__/selectAndExecute-autoPr.test.ts`:
- Around line 200-215: The process.exit spy restoration in the test around the
selectAndExecuteTask function call is not guaranteed to execute if assertions
fail, potentially causing state to leak to subsequent tests. Move the
processExitSpy.mockRestore() call into a finally block that wraps the test logic
(the selectAndExecuteTask call and all expect assertions) to ensure cleanup
happens regardless of test success or failure.
In `@src/app/cli/interactiveLoop.ts`:
- Around line 269-271: When the `--continue` option is specified and the
provider is not configured, the error thrown in the providerType check will
propagate unhandled and crash the process with a stack trace. Evaluate whether
this error should be caught at a higher level in the call stack and handled
gracefully with a user-friendly error message and appropriate exit code before
the process terminates, rather than allowing the raw error to propagate. If
intentional as a hard fail, add a comment documenting this is by design;
otherwise, add proper error handling around the provider validation check to
intercept this case and exit cleanly with a clear message.
In `@src/features/tasks/execute/selectAndExecute.ts`:
- Around line 47-53: The condition in the return statement at line 47
incorrectly requires both result.exceeded and result.exceededInfo to be truthy,
which causes cases where exceeded is true but exceededInfo is missing to be
misclassified as failed status. Change the condition to check only
result.exceeded for determining the exceeded status, then conditionally include
exceededInfo in the return object only when it exists, similar to how the reason
property is conditionally added using the spread operator.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ce121b0d-d94e-4ab2-9226-c24daa0b2084
📒 Files selected for processing (18)
docs/cli-reference.ja.mddocs/cli-reference.mdsrc/__tests__/ask-user-question-handler.test.tssrc/__tests__/cli-routing-issue-resolve.test.tssrc/__tests__/cli-routing-pr-resolve.test.tssrc/__tests__/i18n.test.tssrc/__tests__/interactiveContinuePrompt.test.tssrc/__tests__/selectAndExecute-autoPr.test.tssrc/__tests__/selectAndExecute-skipTaskList.test.tssrc/app/cli/interactiveLoop.tssrc/app/cli/routing.tssrc/features/interactive/continuePrompt.tssrc/features/interactive/index.tssrc/features/tasks/execute/selectAndExecute.tssrc/features/tasks/execute/types.tssrc/features/tasks/index.tssrc/shared/i18n/labels_en.yamlsrc/shared/i18n/labels_ja.yaml
Comment on lines
+99
to
107
| Task completed | ||
| Continue? [Y/n] y | ||
|
|
||
| Select workflow: | ||
| > default (current) | ||
| Development/ | ||
| Research/ | ||
| Cancel | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
TAKT プロジェクトの i18n ラベル定義（特に interactive.taskResult.continuePrompt の日本語版）で実際に使用されるプロンプトテキストは何か
💡 Result:
TAKT プロジェクトの i18n ラベル定義において、interactive.taskResult.continuePrompt（実際には labels_ja.yaml 内で interactive.continuePrompt として定義）に対応する日本語のプロンプトテキストは以下の通りです。 続けてタスク内容を入力してください。 このテキストは、TAKT プロジェクトのリポジトリ内にある src/shared/i18n/labels_ja.yaml ファイルにて定義されています [1]。この設定値は、インタラクティブモードでの会話ループにおいて、ユーザーがタスクの要約後に会話を続けることを選択した際に表示されるメッセージとして使用されます [2][3]。 トップ結果: [1][2][3]
Citations:
- 1: https://github.com/nrslib/takt/blob/c226cca9/src/shared/i18n/labels_ja.yaml
- 2: https://github.com/nrslib/takt/blob/c226cca9/src/features/interactive/conversationLoop.ts
- 3: https://github.com/nrslib/takt/blob/c226cca9/src/features/interactive/interactive-summary.ts
🏁 Script executed:
# Find and read the Japanese i18n labels file to get the actual prompt text
fd -t f "labels_ja.yaml" | head -5Repository: nrslib/takt
Length of output: 86
🏁 Script executed:
# Once we find the file, let's check the interactive.continuePrompt or interactive.taskResult.continuePrompt label
rg -A 2 -B 2 "interactive.*continuePrompt|interactive\.taskResult" --type yamlRepository: nrslib/takt
Length of output: 37
🏁 Script executed:
# Also check the current documentation to confirm the English text
sed -n '99,107p' docs/cli-reference.ja.mdRepository: nrslib/takt
Length of output: 176
🏁 Script executed:
# Read the labels_ja.yaml file to find the actual Japanese prompt text
cat src/shared/i18n/labels_ja.yaml | head -100Repository: nrslib/takt
Length of output: 3736
🏁 Script executed:
# Search for keywords related to continue or prompt in the file
rg "continue|Continue|継続|プロンプト" src/shared/i18n/labels_ja.yaml -iRepository: nrslib/takt
Length of output: 423
🏁 Script executed:
# Verify which label key is actually used in the implementation for task result continue prompt
rg -B 5 -A 5 "taskResult.*continuePrompt|interactive.*continuePrompt" src/features --type tsRepository: nrslib/takt
Length of output: 1036
🏁 Script executed:
# Check the specific implementation of the continue prompt display in interactive mode
rg -B 3 -A 3 "getLabel.*continue|continuePrompt" src/features/interactive --type tsRepository: nrslib/takt
Length of output: 2580
日本語版実行例に表示される継続プロンプトを日本語に統一してください。
実装では getLabel('interactive.taskResult.continuePrompt', lang) で日本語テキスト「続行しますか？」を取得しており、実際の日本語ユーザーには「続行しますか？ [Y/n]」と表示されます。しかし、docs/cli-reference.ja.md（行99-107）の実行例には英語テキスト「Continue? [Y/n]」が記載されており、実装と見本が乖離しています。
ドキュメント例を以下のように修正してください：
タスク完了
続行しますか？ [Y/n] y
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli-reference.ja.md` around lines 99 - 107, The Japanese CLI reference
documentation (docs/cli-reference.ja.md) shows an English continuation prompt
"Continue? [Y/n]" in the execution example output, but the actual implementation
uses getLabel to return the Japanese text "続行しますか？ [Y/n]" to Japanese users.
Replace the English prompt text with the Japanese equivalent "続行しますか？ [Y/n]" in
the execution example to align the documentation with the actual localized
output that users will see.
Comment on lines
+923
to
+936
| it('should exit normally after a successful run when the user declines to continue', async () => { | ||
| const exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => { | ||
| throw new Error('process.exit called'); | ||
| }); | ||
| mockShouldPromptForInteractiveContinue.mockReturnValue(true); | ||
| mockPromptContinueAfterTaskResult.mockResolvedValue(false); | ||
|
|
||
| await executeDefaultAction(); | ||
|
|
||
| expect(mockPromptContinueAfterTaskResult).toHaveBeenCalledWith(true, 'en'); | ||
| expect(exitSpy).not.toHaveBeenCalled(); | ||
| expect(mockInteractiveMode).toHaveBeenCalledTimes(1); | ||
| exitSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
process.exit モックの復元を全ケースで finally に統一してください。
これらのテストは mockRestore() が末尾直書きのため、途中失敗時に復元されず後続ケースへ副作用が波及します。try/finally（または共通 afterEach）で確実に復元してください。
Also applies to: 938-951, 1046-1059
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/cli-routing-issue-resolve.test.ts` around lines 923 - 936, The
test case uses exitSpy.mockRestore() at the end without a guaranteed cleanup
mechanism, which means if the test fails before reaching that line, the spy
won't be restored and will leak side effects to subsequent tests. Wrap the test
body in a try/finally block to ensure mockRestore() is always called regardless
of test success or failure, or alternatively move the spy creation and cleanup
to shared beforeEach/afterEach hooks. Apply this same pattern consistently to
all affected test cases including those at lines 938-951 and 1046-1059.
Comment on lines
+595
to
+616
| it('should not call interactive continue prompt helpers in pipeline mode', async () => { | ||
| const programModule = await import('../app/cli/program.js'); | ||
| const originalPipelineMode = programModule.pipelineMode; | ||
| Object.defineProperty(programModule, 'pipelineMode', { value: true, writable: true }); | ||
|
|
||
| mockOpts.workflow = 'default'; | ||
| mockShouldPromptForInteractiveContinue.mockReturnValue(true); | ||
| mockExecutePipeline.mockResolvedValue(0); | ||
|
|
||
| await executeDefaultAction(); | ||
|
|
||
| expect(mockExecutePipeline).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| workflow: 'default', | ||
| }), | ||
| ); | ||
| expect(mockSelectInteractiveMode).not.toHaveBeenCalled(); | ||
| expect(mockShouldPromptForInteractiveContinue).not.toHaveBeenCalled(); | ||
| expect(mockPromptContinueAfterTaskResult).not.toHaveBeenCalled(); | ||
|
|
||
| Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true }); | ||
| }); |
There was a problem hiding this comment.
pipelineMode の一時上書きは finally で必ず復元してください。
現状はテスト途中で失敗すると pipelineMode が復元されず、後続テストの分岐条件を壊す可能性があります。
✅ 提案差分
it('should not call interactive continue prompt helpers in pipeline mode', async () => {
const programModule = await import('../app/cli/program.js');
const originalPipelineMode = programModule.pipelineMode;
Object.defineProperty(programModule, 'pipelineMode', { value: true, writable: true });
- mockOpts.workflow = 'default';
- mockShouldPromptForInteractiveContinue.mockReturnValue(true);
- mockExecutePipeline.mockResolvedValue(0);
-
- await executeDefaultAction();
-
- expect(mockExecutePipeline).toHaveBeenCalledWith(
- expect.objectContaining({
- workflow: 'default',
- }),
- );
- expect(mockSelectInteractiveMode).not.toHaveBeenCalled();
- expect(mockShouldPromptForInteractiveContinue).not.toHaveBeenCalled();
- expect(mockPromptContinueAfterTaskResult).not.toHaveBeenCalled();
-
- Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true });
+ try {
+ mockOpts.workflow = 'default';
+ mockShouldPromptForInteractiveContinue.mockReturnValue(true);
+ mockExecutePipeline.mockResolvedValue(0);
+
+ await executeDefaultAction();
+
+ expect(mockExecutePipeline).toHaveBeenCalledWith(
+ expect.objectContaining({
+ workflow: 'default',
+ }),
+ );
+ expect(mockSelectInteractiveMode).not.toHaveBeenCalled();
+ expect(mockShouldPromptForInteractiveContinue).not.toHaveBeenCalled();
+ expect(mockPromptContinueAfterTaskResult).not.toHaveBeenCalled();
+ } finally {
+ Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true });
+ }
});📝 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('should not call interactive continue prompt helpers in pipeline mode', async () => { | |
| const programModule = await import('../app/cli/program.js'); | |
| const originalPipelineMode = programModule.pipelineMode; | |
| Object.defineProperty(programModule, 'pipelineMode', { value: true, writable: true }); | |
| mockOpts.workflow = 'default'; | |
| mockShouldPromptForInteractiveContinue.mockReturnValue(true); | |
| mockExecutePipeline.mockResolvedValue(0); | |
| await executeDefaultAction(); | |
| expect(mockExecutePipeline).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| workflow: 'default', | |
| }), | |
| ); | |
| expect(mockSelectInteractiveMode).not.toHaveBeenCalled(); | |
| expect(mockShouldPromptForInteractiveContinue).not.toHaveBeenCalled(); | |
| expect(mockPromptContinueAfterTaskResult).not.toHaveBeenCalled(); | |
| Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true }); | |
| }); | |
| it('should not call interactive continue prompt helpers in pipeline mode', async () => { | |
| const programModule = await import('../app/cli/program.js'); | |
| const originalPipelineMode = programModule.pipelineMode; | |
| Object.defineProperty(programModule, 'pipelineMode', { value: true, writable: true }); | |
| try { | |
| mockOpts.workflow = 'default'; | |
| mockShouldPromptForInteractiveContinue.mockReturnValue(true); | |
| mockExecutePipeline.mockResolvedValue(0); | |
| await executeDefaultAction(); | |
| expect(mockExecutePipeline).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| workflow: 'default', | |
| }), | |
| ); | |
| expect(mockSelectInteractiveMode).not.toHaveBeenCalled(); | |
| expect(mockShouldPromptForInteractiveContinue).not.toHaveBeenCalled(); | |
| expect(mockPromptContinueAfterTaskResult).not.toHaveBeenCalled(); | |
| } finally { | |
| Object.defineProperty(programModule, 'pipelineMode', { value: originalPipelineMode, writable: true }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/cli-routing-pr-resolve.test.ts` around lines 595 - 616, The
test modifies the pipelineMode property at the beginning but only restores it at
the end without error handling. If the test execution fails before reaching the
restoration code, pipelineMode will remain modified and break subsequent tests.
Wrap the test execution logic and the pipelineMode restoration in a try-finally
block to ensure the original pipelineMode value is always restored to the
originalPipelineMode variable regardless of whether the test passes or fails
during the executeDefaultAction call and assertions.
Comment on lines
+119
to
+127
| it('prints failed status and returns false for no input', async () => { | ||
| mockReadMultilineInput.mockResolvedValue('n'); | ||
|
|
||
| const result = await promptContinueAfterTaskResult(false, 'en'); | ||
|
|
||
| expect(result).toBe(false); | ||
| expect(mockInfo).toHaveBeenCalledWith('Task failed'); | ||
| expect(mockReadMultilineInput).toHaveBeenCalledWith('Continue? [Y/n]'); | ||
| }); |
There was a problem hiding this comment.
失敗時の表示順（失敗メッセージ→Continue質問）を呼び出し順で固定してください。
現在は両方が呼ばれたことしか検証しておらず、順序が逆でも通ってしまいます。ユーザー向け契約を担保するため、呼び出し順のアサーションを追加してください。
✅ 提案差分
it('prints failed status and returns false for no input', async () => {
mockReadMultilineInput.mockResolvedValue('n');
const result = await promptContinueAfterTaskResult(false, 'en');
expect(result).toBe(false);
expect(mockInfo).toHaveBeenCalledWith('Task failed');
expect(mockReadMultilineInput).toHaveBeenCalledWith('Continue? [Y/n]');
+ expect(mockInfo.mock.invocationCallOrder[0]).toBeLessThan(
+ mockReadMultilineInput.mock.invocationCallOrder[0],
+ );
});As per coding guidelines, "When judging failures in user-facing contract or prompt ordering (failure info visible before continue question), require explicit test proof—not only logs/snapshots."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/interactiveContinuePrompt.test.ts` around lines 119 - 127, The
test for promptContinueAfterTaskResult with false input only verifies that
mockInfo and mockReadMultilineInput were called, but does not verify the order
in which they were called. To ensure the user-facing contract is met (failure
message displays before the continue prompt), add an assertion to verify that
mockInfo is called before mockReadMultilineInput. Use Jest's
toHaveBeenCalledBefore matcher or check the call order by examining the call
indices of both mocks to confirm the failure status message appears before the
continue prompt question.
Source: Coding guidelines
Comment on lines
+200
to
+215
| const processExitSpy = vi.spyOn(process, 'exit').mockImplementation((() => { | ||
| throw new Error('process exit'); | ||
| }) as (code?: string | number | null | undefined) => never); | ||
|
|
||
| const result = await selectAndExecuteTask('/project', 'test task', { | ||
| workflow: 'default', | ||
| exitOnFailure: false, | ||
| }); | ||
|
|
||
| expect(result).toEqual({ success: false, status: 'failed', reason: 'Task failed' }); | ||
| expect(processExitSpy).not.toHaveBeenCalled(); | ||
| expect(mockAddTask).toHaveBeenCalledWith('test task', { workflow: 'default' }); | ||
| expect(mockFailTask).toHaveBeenCalledTimes(1); | ||
| expect(mockCompleteTask).not.toHaveBeenCalled(); | ||
| processExitSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
process.exit スパイの復元を finally で保証してください。
このテストは失敗時に processExitSpy.mockRestore() まで到達せず、後続テストへ状態が漏れる可能性があります。
✅ 提案差分
- const result = await selectAndExecuteTask('/project', 'test task', {
- workflow: 'default',
- exitOnFailure: false,
- });
-
- expect(result).toEqual({ success: false, status: 'failed', reason: 'Task failed' });
- expect(processExitSpy).not.toHaveBeenCalled();
- expect(mockAddTask).toHaveBeenCalledWith('test task', { workflow: 'default' });
- expect(mockFailTask).toHaveBeenCalledTimes(1);
- expect(mockCompleteTask).not.toHaveBeenCalled();
- processExitSpy.mockRestore();
+ try {
+ const result = await selectAndExecuteTask('/project', 'test task', {
+ workflow: 'default',
+ exitOnFailure: false,
+ });
+
+ expect(result).toEqual({ success: false, status: 'failed', reason: 'Task failed' });
+ expect(processExitSpy).not.toHaveBeenCalled();
+ expect(mockAddTask).toHaveBeenCalledWith('test task', { workflow: 'default' });
+ expect(mockFailTask).toHaveBeenCalledTimes(1);
+ expect(mockCompleteTask).not.toHaveBeenCalled();
+ } finally {
+ processExitSpy.mockRestore();
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/__tests__/selectAndExecute-autoPr.test.ts` around lines 200 - 215, The
process.exit spy restoration in the test around the selectAndExecuteTask
function call is not guaranteed to execute if assertions fail, potentially
causing state to leak to subsequent tests. Move the processExitSpy.mockRestore()
call into a finally block that wraps the test logic (the selectAndExecuteTask
call and all expect assertions) to ensure cleanup happens regardless of test
success or failure.
Comment on lines
+269
to
+271
| if (!providerType) { | ||
| throw new Error('Provider is not configured.'); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
--continue オプション使用時のプロバイダー未設定エラーがハンドリングされずに伝播する可能性
--continue が指定されているがプロバイダーが設定されていない場合、ここで投げられたエラーは上位でキャッチされず、スタックトレース付きでプロセスがクラッシュします。設定エラーとしてフェイルファストは妥当ですが、ユーザー向けにはもう少しフレンドリーなエラーメッセージと終了コードが望ましい可能性があります。
現状の挙動が意図通りであれば、このままで問題ありません。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/cli/interactiveLoop.ts` around lines 269 - 271, When the `--continue`
option is specified and the provider is not configured, the error thrown in the
providerType check will propagate unhandled and crash the process with a stack
trace. Evaluate whether this error should be caught at a higher level in the
call stack and handled gracefully with a user-friendly error message and
appropriate exit code before the process terminates, rather than allowing the
raw error to propagate. If intentional as a hard fail, add a comment documenting
this is by design; otherwise, add proper error handling around the provider
validation check to intercept this case and exit cleanly with a clear message.
Comment on lines
+47
to
+53
| if (result.exceeded && result.exceededInfo) { | ||
| return { | ||
| success: false, | ||
| status: 'exceeded', | ||
| ...(result.reason ? { reason: result.reason } : {}), | ||
| exceededInfo: result.exceededInfo, | ||
| }; |
There was a problem hiding this comment.
exceeded 判定が exceededInfo 依存になっており状態を取りこぼします
Line 47 の条件が result.exceeded && result.exceededInfo になっているため、exceeded: true でも exceededInfo が欠けた入力は status: 'failed' に誤分類されます。status は exceeded を優先判定し、exceededInfo は存在時のみ添付してください。
修正例
- if (result.exceeded && result.exceededInfo) {
+ if (result.exceeded) {
return {
success: false,
status: 'exceeded',
...(result.reason ? { reason: result.reason } : {}),
- exceededInfo: result.exceededInfo,
+ ...(result.exceededInfo ? { exceededInfo: result.exceededInfo } : {}),
};
}As per coding guidelines, 「契約/UI/CLI behavior changes は edge case を含めて一致させる」「REJECT条件（契約不整合）」に該当しないよう、状態境界の判定を契約どおり維持する必要があります。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/tasks/execute/selectAndExecute.ts` around lines 47 - 53, The
condition in the return statement at line 47 incorrectly requires both
result.exceeded and result.exceededInfo to be truthy, which causes cases where
exceeded is true but exceededInfo is missing to be misclassified as failed
status. Change the condition to check only result.exceeded for determining the
exceeded status, then conditionally include exceededInfo in the return object
only when it exists, similar to how the reason property is conditionally added
using the spread operator.
Source: Coding guidelines
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
1 participant
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
Problem
When TAKT is used interactively, a workflow/task run exits back to the shell after completion or failure. This forces the user to run
taktagain for the next operation, even during an interactive session where they are repeatedly adding, running, inspecting, or retrying tasks.This is inconvenient for manual review/fix workflows where the user wants to keep working in TAKT after each run.
Goal
For normal interactive TAKT usage, return to the interactive prompt after a workflow/task finishes.
The user should be asked whether to continue:
or:
If the user chooses yes, TAKT should return to the initial interactive prompt/menu. If the user chooses no, TAKT exits as it does today.
Scope
Apply this only to interactive usage, such as invoking
taktwithout a non-interactive command path and entering the existing interactive flow.Do not change behavior for scripted or pipeline execution.
Non-goals
takt interactivesubcommand for this issue.Expected behavior
In interactive mode:
Modes that must not prompt
Do not ask
Continue?in non-interactive or automation-oriented paths, including:--pipelinetakt add --pr ...Failure handling
The continue prompt should be shown for terminal workflow results such as:
The prompt should not hide the failure. The previous run result must remain visible before the continue question.
Ctrl-C / EOF
Implementation notes
Prefer wrapping the existing interactive flow in a loop rather than introducing a new command mode.
The loop should be guarded by an explicit interactive-mode check so that non-interactive code paths cannot accidentally prompt.
Avoid calling
process.exit()deep inside the interactive flow if that prevents returning to the top prompt. If needed, refactor those exits into returned status values only for the interactive path.Acceptance criteria
--pipelineand direct scripted commands do not prompt.Execution Report
Workflow
takt-defaultcompleted successfully.Closes #838
Summary by CodeRabbit
リリースノート
New Features
Documentation