[#721] add-claude-terminal-provider#727
Open
nrslib wants to merge 3 commits into
Open
Conversation
Owner
Author
|
/review |
3 tasks
Owner
Author
|
/review |
📝 WalkthroughWalkthroughThis PR implements a new ChangesClaude Terminal Provider Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Owner
Author
|
コード品質観点で 1 点、merge 前に直した方がよいと思います。
そのため Ctrl+C / abort 後も、裏で transcript polling のタイマーが最大 15 分残り続ける可能性があります。現在のテストは 修正案:
CI とローカル |
Owner
Author
Summaryタスク指示書:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/core/workflow/engine/OptionsBuilder.ts`:
- Around line 246-247: The options object creation in OptionsBuilder is
currently assigning a maxTurns property even when unsupported (via
resolveSupportedMaxTurns returning undefined); change to conditional object
spreading so maxTurns is only present when defined: call
resolveSupportedMaxTurns(step, overrides.maxTurns, runtime) into a local
variable (e.g., const maxTurns = this.resolveSupportedMaxTurns(...)) and then
include ...(maxTurns !== undefined ? { maxTurns } : {}) when building the
options in the methods that construct the options (where maxTurns is currently
added, referenced by OptionsBuilder and resolveSupportedMaxTurns) so unsupported
providers receive no maxTurns key at all.
In `@src/core/workflow/status-judgment-phase.ts`:
- Around line 119-121: Move the call to resolveStepProviderInfo(step, ctx)
inside the existing try block so any exception it throws is caught and triggers
the existing error handling path (including the onPhaseComplete(..., 'error',
...) call). Locate the current resolveStepProviderInfo invocation (near
stepProvider variable) and hoist its declaration if needed, then assign
stepProvider within the try before any code that relies on it; ensure subsequent
references use that variable so the catch block executes for resolve failures.
In `@src/infra/claude-terminal/response-normalizer.ts`:
- Around line 95-96: The rate-limit field builder always hardcodes source:
'stream_marker' which mislabels non-marker detections; update the call to
buildRateLimitedResponseFields('claude-terminal', ..., input.assistantText) to
pass the actual detector source determined from the matched detector (e.g., use
the matched detector's type or a boolean like isMarker to choose between
'stream_marker' and 'error_text'); find other similar invocations around the
matched detector handling (the block that currently accepts both marker-based
and error-text-based detections at the later lines) and apply the same dynamic
source selection so all rate-limit diagnostics use the correct source value.
In `@src/infra/claude-terminal/transcript-reader.ts`:
- Around line 193-206: parseClaudeTerminalTranscript currently hard-fails when
parseTranscriptLine throws on a partially-written last JSONL line; change
behavior to tolerate transient incomplete lines by skipping (or deferring) the
final line parse if JSON parsing fails for the last line instead of throwing.
Specifically, in parseClaudeTerminalTranscript (and the same logic area around
the block using transcriptSinceBaseline -> .split -> .reduce), catch JSON/parse
errors from parseTranscriptLine for the last raw line (index === last) and
simply return the accumulated parsed state without appending that entry (so
polling code like findSession/waitForAssistantResponse can retry), while still
propagating non-transient errors for earlier lines; keep use of
appendTranscriptEntry and lineNumberOffset unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70e7b6f6-5d85-48bf-8009-b56d796415ab
📒 Files selected for processing (53)
docs/configuration.ja.mddocs/configuration.mdsrc/__tests__/agent-usecases.test.tssrc/__tests__/claude-mcp-config.test.tssrc/__tests__/claude-terminal-client-abort-cleanup.test.tssrc/__tests__/claude-terminal-client.test.tssrc/__tests__/claude-terminal-command.test.tssrc/__tests__/claude-terminal-provider-contract.test.tssrc/__tests__/claude-terminal-provider-options.test.tssrc/__tests__/claude-terminal-provider-wiring.test.tssrc/__tests__/claude-terminal-response-normalizer.test.tssrc/__tests__/claude-terminal-tmux-backend.test.tssrc/__tests__/claude-terminal-transcript-reader.test.tssrc/__tests__/engine-provider-options.test.tssrc/__tests__/initialization.test.tssrc/__tests__/it-provider-config-block.test.tssrc/__tests__/options-builder.test.tssrc/__tests__/provider-capabilities.test.tssrc/__tests__/provider-options-contract.test.tssrc/__tests__/status-judgment-phase-structured-caller.test.tssrc/__tests__/status-judgment-phase.test.tssrc/__tests__/structured-caller-prompt-based.test.tssrc/__tests__/workflowExecution-claude-terminal.test.tssrc/agents/decompose-task-usecase.tssrc/agents/judge-status-usecase.tssrc/agents/provider-call-options.tssrc/agents/structured-caller/prompt-based-structured-caller.tssrc/app/cli/program.tssrc/core/models/provider-profiles.tssrc/core/models/schema-base.tssrc/core/models/workflow-provider-options.tssrc/core/models/workflow-types.tssrc/core/workflow/engine/OptionsBuilder.tssrc/core/workflow/permission-profile-resolution.tssrc/core/workflow/phase-runner.tssrc/core/workflow/report-phase-runner.tssrc/core/workflow/status-judgment-phase.tssrc/infra/claude-headless/client.tssrc/infra/claude-terminal/client.tssrc/infra/claude-terminal/command.tssrc/infra/claude-terminal/response-normalizer.tssrc/infra/claude-terminal/tmux-backend.tssrc/infra/claude-terminal/transcript-reader.tssrc/infra/claude-terminal/types.tssrc/infra/claude/mcp-config.tssrc/infra/config/configNormalizers.tssrc/infra/config/global/initialization.tssrc/infra/config/providerOptions.tssrc/infra/config/providerOptionsContract.tssrc/infra/providers/claude-terminal.tssrc/infra/providers/index.tssrc/infra/providers/provider-capabilities.tssrc/shared/types/provider.ts
Comment on lines
+246
to
247
| maxTurns: this.resolveSupportedMaxTurns(step, overrides.maxTurns, runtime), | ||
| }; |
There was a problem hiding this comment.
Truly omit unsupported maxTurns instead of assigning undefined.
Line 246 and Line 260 still emit a maxTurns property even when unsupported. Prefer conditional object spreading so unsupported providers receive no maxTurns key at all.
Suggested patch
buildResumeOptions(
@@
): RunAgentOptions {
+ const supportedMaxTurns = this.resolveSupportedMaxTurns(step, overrides.maxTurns, runtime);
return {
...this.buildBaseOptions(step, undefined, runtime),
// Report/status phases are read-only regardless of step settings.
permissionMode: 'readonly',
sessionId,
allowedTools: [],
- maxTurns: this.resolveSupportedMaxTurns(step, overrides.maxTurns, runtime),
+ ...(supportedMaxTurns !== undefined ? { maxTurns: supportedMaxTurns } : {}),
};
}
@@
): RunAgentOptions {
+ const supportedMaxTurns = this.resolveSupportedMaxTurns(step, overrides.maxTurns, runtime);
return {
...this.buildBaseOptions(step, undefined, runtime),
permissionMode: 'readonly',
allowedTools: overrides.allowedTools,
- maxTurns: this.resolveSupportedMaxTurns(step, overrides.maxTurns, runtime),
+ ...(supportedMaxTurns !== undefined ? { maxTurns: supportedMaxTurns } : {}),
};
}Also applies to: 260-261
🤖 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/core/workflow/engine/OptionsBuilder.ts` around lines 246 - 247, The
options object creation in OptionsBuilder is currently assigning a maxTurns
property even when unsupported (via resolveSupportedMaxTurns returning
undefined); change to conditional object spreading so maxTurns is only present
when defined: call resolveSupportedMaxTurns(step, overrides.maxTurns, runtime)
into a local variable (e.g., const maxTurns =
this.resolveSupportedMaxTurns(...)) and then include ...(maxTurns !== undefined
? { maxTurns } : {}) when building the options in the methods that construct the
options (where maxTurns is currently added, referenced by OptionsBuilder and
resolveSupportedMaxTurns) so unsupported providers receive no maxTurns key at
all.
Comment on lines
+119
to
121
| const stepProvider = resolveStepProviderInfo(step, ctx); | ||
|
|
||
| try { |
There was a problem hiding this comment.
Resolve step provider inside the try block to preserve error callbacks.
resolveStepProviderInfo() can throw on Line 119 before try starts, which skips onPhaseComplete(..., 'error', ...) for that failure path.
Suggested patch
- const stepProvider = resolveStepProviderInfo(step, ctx);
-
try {
+ const stepProvider = resolveStepProviderInfo(step, ctx);
const result = await ctx.structuredCaller.judgeStatus(structuredInstruction, tagInstruction, step.rules, {
cwd: ctx.cwd,
stepName: step.name,🤖 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/core/workflow/status-judgment-phase.ts` around lines 119 - 121, Move the
call to resolveStepProviderInfo(step, ctx) inside the existing try block so any
exception it throws is caught and triggers the existing error handling path
(including the onPhaseComplete(..., 'error', ...) call). Locate the current
resolveStepProviderInfo invocation (near stepProvider variable) and hoist its
declaration if needed, then assign stepProvider within the try before any code
that relies on it; ensure subsequent references use that variable so the catch
block executes for resolve failures.
Comment on lines
+95
to
+96
| ...buildRateLimitedResponseFields('claude-terminal', 'stream_marker', input.assistantText), | ||
| }; |
There was a problem hiding this comment.
Use the matched rate-limit detector to set source correctly.
Line 114 accepts both marker-based and error-text-based rate-limit detections, but Line 95 always emits source: 'stream_marker'. That misclassifies non-marker cases and degrades rate-limit diagnostics.
Also applies to: 114-116
🤖 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/infra/claude-terminal/response-normalizer.ts` around lines 95 - 96, The
rate-limit field builder always hardcodes source: 'stream_marker' which
mislabels non-marker detections; update the call to
buildRateLimitedResponseFields('claude-terminal', ..., input.assistantText) to
pass the actual detector source determined from the matched detector (e.g., use
the matched detector's type or a boolean like isMarker to choose between
'stream_marker' and 'error_text'); find other similar invocations around the
matched detector handling (the block that currently accepts both marker-based
and error-text-based detections at the later lines) and apply the same dynamic
source selection so all rate-limit diagnostics use the correct source value.
Comment on lines
+193
to
+206
| export function parseClaudeTerminalTranscript( | ||
| transcript: string, | ||
| baseline?: ClaudeTranscriptBaseline, | ||
| ): ClaudeTerminalTranscript { | ||
| const lineNumberOffset = baseline?.lineNumberOffset ?? 0; | ||
| return transcriptSinceBaseline(transcript, baseline) | ||
| .split(/\r?\n/) | ||
| .reduce<ClaudeTerminalTranscript>((parsed, rawLine, index) => { | ||
| const line = rawLine.trim(); | ||
| if (line.length === 0) { | ||
| return parsed; | ||
| } | ||
| return appendTranscriptEntry(parsed, parseTranscriptLine(line, index + 1 + lineNumberOffset)); | ||
| }, { sessionId: '', assistantText: '', events: [] }); |
There was a problem hiding this comment.
Handle in-progress JSONL writes without hard-failing polling.
The polling path currently fails if the transcript’s last line is temporarily incomplete while Claude is writing. That turns a transient state into a hard error instead of retrying, which can prematurely abort findSession/waitForAssistantResponse.
Suggested fix
-export function parseClaudeTerminalTranscript(
- transcript: string,
- baseline?: ClaudeTranscriptBaseline,
-): ClaudeTerminalTranscript {
+export function parseClaudeTerminalTranscript(
+ transcript: string,
+ baseline?: ClaudeTranscriptBaseline,
+ options?: { allowIncompleteTrailingLine?: boolean },
+): ClaudeTerminalTranscript {
const lineNumberOffset = baseline?.lineNumberOffset ?? 0;
- return transcriptSinceBaseline(transcript, baseline)
- .split(/\r?\n/)
+ const sliced = transcriptSinceBaseline(transcript, baseline);
+ const lines = sliced.split(/\r?\n/);
+ return lines
.reduce<ClaudeTerminalTranscript>((parsed, rawLine, index) => {
const line = rawLine.trim();
if (line.length === 0) {
return parsed;
}
- return appendTranscriptEntry(parsed, parseTranscriptLine(line, index + 1 + lineNumberOffset));
+ try {
+ return appendTranscriptEntry(parsed, parseTranscriptLine(line, index + 1 + lineNumberOffset));
+ } catch (error) {
+ const isTrailingLine = index === lines.length - 1;
+ if (
+ options?.allowIncompleteTrailingLine
+ && isTrailingLine
+ && !sliced.endsWith('\n')
+ && error instanceof Error
+ && /Malformed Claude terminal transcript JSON/.test(error.message)
+ ) {
+ return parsed;
+ }
+ throw error;
+ }
}, { sessionId: '', assistantText: '', events: [] });
}- const parsed = parseClaudeTerminalTranscript(transcript);
+ const parsed = parseClaudeTerminalTranscript(transcript, undefined, {
+ allowIncompleteTrailingLine: true,
+ });- const parsed = parseClaudeTerminalTranscript(transcript, options.baseline);
+ const parsed = parseClaudeTerminalTranscript(transcript, options.baseline, {
+ allowIncompleteTrailingLine: true,
+ });Also applies to: 326-367
🤖 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/infra/claude-terminal/transcript-reader.ts` around lines 193 - 206,
parseClaudeTerminalTranscript currently hard-fails when parseTranscriptLine
throws on a partially-written last JSONL line; change behavior to tolerate
transient incomplete lines by skipping (or deferring) the final line parse if
JSON parsing fails for the last line instead of throwing. Specifically, in
parseClaudeTerminalTranscript (and the same logic area around the block using
transcriptSinceBaseline -> .split -> .reduce), catch JSON/parse errors from
parseTranscriptLine for the last raw line (index === last) and simply return the
accumulated parsed state without appending that entry (so polling code like
findSession/waitForAssistantResponse can retry), while still propagating
non-transient errors for earlier lines; keep use of appendTranscriptEntry and
lineNumberOffset unchanged.
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
概要
Claude Code を headless 実行ではなく、通常の interactive terminal session として起動する
provider: claude-terminalを追加したい。現在の
provider: claudeは headless CLI 実行を前提にしている。一方で、TAKT の workflow step から通常の Claude Code session を扱える provider があると、既存の Claude Code 環境、ログイン状態、worktree 上のファイル操作と自然に統合しやすい。この provider は既存
provider: claudeを置き換えるものではなく、interactive terminal 実行を選びたい場合の新しい experimental provider として追加する。目的
provider: claude-terminalを新規 provider として追加するProviderinterface と同じ形で扱えるようにするProviderCallOptionsで渡される主要機能をclaude-terminalでも扱えるようにするAgentResponseに正規化するtmuxbackend のみ対応する必要な互換性
claude-terminalは新しい provider だが、TAKT workflow から見た provider としては既存 provider と同じ契約を満たす必要がある。次の項目は対応対象とする。ただし interactive terminal 実行の制約により headless provider と同じ粒度で扱えない可能性がある。実装できない項目は黙って無視せず、明確な unsupported error、provider error、または
usageMissing: trueのような明示的な欠落情報として扱う。cwdabortSignalsessionIdmodelallowedToolsmcpServersmaxTurnspermissionModeproviderOptionsonStreamonPermissionRequestonAskUserQuestionbypassPermissionsoutputSchemaまた、戻り値では次の情報を可能な限り既存 provider と同じ意味で埋める。取得できない値は推測で埋めず、欠落理由を明示する。
contentstatussessionIderrorerrorKindrateLimitInfofailureCategorystructuredOutputproviderUsage特に workflow 実行上重要な
abortSignal、permissionMode、allowedTools、mcpServers、onStream、outputSchemaは初期実装の対応対象に含める。完全に同等の観測性を保証できない場合でも、その制約を明示して失敗または欠落として表現する。非目的
provider: claudeを置き換えないprovider: claudeの内部実装をclaude-terminalに寄せない利用例
providerとmodelは既存仕様通り step と同じ階層に置く。provider_options.claudeは Claude 共通の option として扱う。provider_options.claude_terminalは terminal 実行固有の option として扱う。設計案
新しい provider type を追加する。
provider 実装は既存の
Providerinterface に乗せる。supportsStructuredOutputはoutputSchemaを受け取ったときにstructuredOutputを返せることを意味する。terminal 実行で native structured output が使えない場合でも、provider 境界で schema 指示、応答抽出、検証、失敗時エラー化まで責務を持つ。内部では terminal 操作と Claude Code の応答検出を分離する。
想定する責務分離は次の通り。
provider_options
terminal 固有 option を追加する。
YAML では snake_case で指定する。
実行フロー
ProviderCallOptionsをclaude-terminalの実行設定へ変換するoptions.cwdで terminal session を開始するmodel、permissionMode、allowedTools、mcpServers、bypassPermissionsを Claude Code 起動設定へ反映するoutputSchemaがある場合は structured output 用の指示と検証を有効にするonStream/onPermissionRequest/onAskUserQuestionを既存 provider と同じ意味で呼び出すAgentResponseに変換するkeep_sessionが false の場合は terminal session を停止するエラー処理
tmuxが存在しない場合は明確なエラーで fail fast するProviderCallOptionsを反映できない場合は、黙って無視せず明確な unsupported error または provider error とするabortSignalが発火した場合は terminal session を停止するoutputSchema指定時に構造化出力を抽出・検証できない場合は provider error とするprovider_options.claude_terminalは黙って無視しないテスト観点
ProviderRegistryでclaude-terminalを解決できるprovider_options.claude_terminalを normalize / denormalize できるProviderCallOptionsが terminal 実行設定へ正しく変換されるonStreamが既存 provider と同じ event 意味で呼ばれるoutputSchema指定時はstructuredOutputを返すか、抽出・検証できない理由を明示した provider error にするAgentResponseへの変換を検証する受け入れ条件
provider: claude-terminalを指定できるcwdで起動されるmodel、allowedTools、mcpServers、permissionMode、bypassPermissionsが反映されるabortSignalにより terminal session が停止されるonStream、onPermissionRequest、onAskUserQuestionは対応対象とし、terminal 実行の制約で同等に扱えない場合は明示的に unsupported とするoutputSchema指定時はstructuredOutputを返すか、抽出・検証できない理由を明示した provider error にするAgentResponseを返すprovider_options.claude_terminalが YAML から正規化され provider に渡るExecution Report
Workflow
takt-defaultcompleted successfully.Closes #721
Summary by CodeRabbit
New Features
claude-terminal) supporting interactive terminal sessions with tmux backend.Documentation