feat(cli): add TUI-based interactive terminal interface#14
Merged
Conversation
Major refactoring of CLI entrypoint with pi-tui integration: - Replace readline-based input with TUI Editor component - Add pi-tui-stream-renderer for better stream rendering - Implement new /new command for session management - Add Spinner output toggle for TUI mode - Improve command handling and autocomplete - Add think selector UI component New features: - TUI-based chat interface with markdown support - Better session management with visual feedback - Improved command autocomplete with suggestions - Cleaner stream rendering with reasoning steps Breaking changes: - CLI now requires TTY for full functionality
Improve readability by showing path and range, using concise omitted-line messages, and keeping long lines truncated instead of wrapped.
- Add prompts: prefix support for skill commands (e.g., /prompts:rams) - Add abortSignal support to agent stream for interruption - Improve CLI autocomplete with alias-aware suggestions - Enhance model selector UI with current indicator and truncation - Add stream interruption handling with user feedback - Improve Ctrl+C behavior: first press clears input, second exits
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughGemini 공급자 제거, readline 기반 CLI를 PiTUI 기반 UI로 대체, 도구 폴백을 Boolean → 모드 기반으로 전환, 명령 별칭/prompts: 접두사 도입, 스트림 렌더러를 PiTUI용으로 재구성하고 관련 테스트 추가/교체. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as PiTUI CLI
participant Resolver as CommandResolver
participant AgentMgr as AgentManager
participant Agent as Agent
User->>CLI: 입력(명령 또는 메시지)
CLI->>Resolver: resolveRegisteredCommandName(input)
Resolver-->>CLI: canonicalName
alt 내장 명령
CLI->>AgentMgr: execute command
AgentMgr-->>CLI: CommandResult
CLI->>User: 결과 렌더링
else 스킬/스트림
CLI->>AgentMgr: stream(messages, options)
AgentMgr->>Agent: create (toolFallbackMode)
Agent->>Agent: streamText / tool loop
Agent-->>AgentMgr: AgentStreamResult
loop 반복(shouldContinueManualToolLoop)
AgentMgr->>CLI: 이벤트(텍스트/도구/추론)
CLI->>CLI: PiTUI로 렌더링
end
AgentMgr-->>CLI: 최종 결과
CLI->>User: 출력 표시
end
sequenceDiagram
participant User
participant CLI
participant ToolFallbackCmd as ToolFallbackCommand
participant AgentMgr as AgentManager
participant Middleware as Middleware
User->>CLI: /tool-fallback [mode]
CLI->>ToolFallbackCmd: execute(args)
ToolFallbackCmd->>ToolFallbackCmd: parseToolFallbackMode(arg)
alt valid
ToolFallbackCmd->>AgentMgr: setToolFallbackMode(mode)
AgentMgr->>Middleware: rebuild middlewares with mode
Middleware-->>AgentMgr: middleware applied
ToolFallbackCmd-->>CLI: 성공 메시지
else invalid
ToolFallbackCmd-->>CLI: 오류 메시지
end
CLI->>User: 상태 출력
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Summary of ChangesHello @minpeter, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the command-line interface by integrating a new Text User Interface (TUI). This change modernizes the user interaction, making the CLI more intuitive and responsive. It also introduces more flexible tool fallback options and refines how models are selected, providing a more robust and user-friendly environment for interacting with the agent. The underlying message handling and command processing have been updated to support these new interactive capabilities. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da8b3f2647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring of the CLI's user interface and agent interaction, primarily by integrating the @mariozechner/pi-tui library for a richer terminal experience. Key changes include removing support for Gemini models, refactoring the agent's streaming logic to use streamText directly, and enhancing command handling with aliases and a more robust ToolFallbackMode system. The CLI now uses pi-tui for rendering output, input, and interactive selectors for models, thinking, and tool fallback modes, replacing custom readline and ANSI escape code logic. The clear command is updated to trigger a 'new-session' action, and skill commands can now be addressed with a 'prompts:' prefix. The message-history now unconditionally trims trailing newlines from assistant messages. Review comments highlight potential performance issues with safeStringify on large tool results, suggest a more efficient Int32Array for the LCS algorithm in diff rendering, and recommend a more conservative safety cap for the MANUAL_TOOL_LOOP_MAX_STEPS to prevent excessive token consumption.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/tools/execute/noninteractive-wrapper.ts (1)
276-284:⚠️ Potential issue | 🟡 Minor
wasModified가 항상true→description: null분기 및getFullWrappedCommand조기 반환이 데드 코드
env는 항상getBaseEnv()의 결과(CI,NONINTERACTIVE,NO_TTY,NO_COLOR등 최소 4개 키 포함)를 포함하므로Object.keys(env).length > 0은 항상true입니다. 결과적으로:
- Line 284:
description: wasModified ? tool.description : null—null을 반환하는 분기는 절대 도달 불가- Line 292: 폴백 분기에서도
wrapped: true를 하드코딩하므로wrapCommandNonInteractive는wrapped: false를 반환하는 경우가 없음- Line 310 (
getFullWrappedCommand의if (!result.wrapped) return command): 데드 코드
wasModified조건식의 의도(명령 문자열 변경 여부와 환경 변수 주입 여부를 분리)와 실제 동작이 일치하지 않습니다. 항상 wrapping이 적용된다면wrapped필드를 제거하거나, 환경 변수 주입이 없는 경우를 정확히 반영하도록 조건을 수정해야 합니다.🛠️ 데드 코드 제거 및 명확한 의도 표현을 위한 제안
- const wasModified = - wrappedCommand !== trimmedCommand || Object.keys(env).length > 0; - return { command: wrappedCommand, env, - wrapped: wasModified, + wrapped: true, tool: tool.name, - description: wasModified ? tool.description : null, + description: tool.description, };그리고
getFullWrappedCommand의 데드 코드 제거:export function getFullWrappedCommand(command: string): string { const result = wrapCommandNonInteractive(command); - if (!result.wrapped) { - return command; - } - const envPrefix = buildEnvPrefix(result.env); return `${envPrefix}${result.command}`; }또는
wrapped: false를 실제로 반환할 수 있는 경로가 필요하다면WrapperResult.wrapped필드의 의미를 재정의하고,getBaseEnv()를 호출하지 않는 경로를 별도로 두는 방향으로 설계를 재검토해야 합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/execute/noninteractive-wrapper.ts` around lines 276 - 284, The current wasModified uses Object.keys(env).length>0 which is always true because getBaseEnv() injects keys; change the modification check to only consider command string changes and whether new env keys beyond the base env were added: compute baseEnv = getBaseEnv() (or its keys) and set envAdded = Object.keys(env).some(k => !Object.prototype.hasOwnProperty.call(baseEnv,k)) (or compare env vs baseEnv), then set wasModified = wrappedCommand !== trimmedCommand || envAdded; update wrapCommandNonInteractive's fallback so it can return wrapped: false when neither the command nor extra env changed (remove the hardcoded wrapped: true), and update getFullWrappedCommand/WrapperResult.wrapped usage accordingly (either remove the dead early return or rely on the corrected wrapped flag) so the description and early-return branches become reachable and consistent.src/interaction/spinner.ts (1)
34-45:⚠️ Potential issue | 🟠 Major
stop()의 early return이 interval을 누수시킵니다.
spinnerOutputEnabled가드를clearInterval호출 이전에 배치했기 때문에,start()가 출력 활성 상태에서 호출되어this.interval이 설정된 뒤 출력이 비활성화되면(setSpinnerOutputEnabled(false)), 이후stop()호출 시 line 35에서 조기 반환하여clearInterval이 절대 실행되지 않습니다. 결과적으로 interval이 무기한 실행되며process.stdout.write를 계속 호출합니다.수정 방향: interval 정리는
spinnerOutputEnabled상태와 무관하게 항상 수행하고, stdout 기록만 플래그로 제어하세요.🐛 수정 제안
stop(): void { - if (!spinnerOutputEnabled) { - return; - } - if (this.interval) { clearInterval(this.interval); this.interval = null; } + + if (!spinnerOutputEnabled) { + return; + } + process.stdout.write("\r\x1B[K"); process.stdout.write("\x1B[?25h"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/spinner.ts` around lines 34 - 45, The early return in stop() leaks the timer: move the spinnerOutputEnabled guard so that clearInterval(this.interval) and this.interval = null always run regardless of spinnerOutputEnabled; only the process.stdout.write calls should be conditional. Update the stop() method (referencing stop(), start(), this.interval and spinnerOutputEnabled / setSpinnerOutputEnabled) to always clear and null out this.interval, then if spinnerOutputEnabled perform the "\r\x1B[K" and "\x1B[?25h" writes.src/entrypoints/headless.ts (1)
465-480:⚠️ Potential issue | 🟠 MajorTodo 연속 실행 외부 루프에 안전 상한이 없습니다
이전 코드의
CONTINUATION_MAX_LOOPS제한이 제거되면서 외부while (true)루프는 반복 횟수 상한이 없습니다. 내부processAgentResponse는MANUAL_TOOL_LOOP_MAX_STEPS = 200으로 보호되지만, 모델이 매 호출마다 미완료 Todo를 계속 생성한다면(예: 모델 버그, 잘못된 도구 출력) 프로세스가 무한 루프에 빠집니다. Headless 모드는 벤치마크 파이프라인에서 사용되므로 실질적인 위험입니다.🛡️ 안전 상한 추가 제안
+ const MAX_TODO_REMINDER_ITERATIONS = 20; + let todoReminderCount = 0; + while (true) { const incompleteTodos = await getIncompleteTodos(); if (incompleteTodos.length === 0) { break; } + todoReminderCount += 1; + if (todoReminderCount > MAX_TODO_REMINDER_ITERATIONS) { + emitEvent({ + timestamp: new Date().toISOString(), + type: "error", + sessionId, + error: `Todo continuation safety cap reached (${MAX_TODO_REMINDER_ITERATIONS} reminders). Incomplete todos: ${incompleteTodos.map((t) => t.id).join(", ")}`, + }); + break; + } + const reminder = buildTodoContinuationUserMessage(incompleteTodos);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/headless.ts` around lines 465 - 480, The outer while(true) loop that fetches getIncompleteTodos() lacks a safety cap and can loop forever; add a bounded iteration limit (reintroduce a CONTINUATION_MAX_LOOPS constant or similar) and a counter in headless.ts around the loop that breaks and logs/emits an error event when the limit is reached. Ensure the counter is checked each iteration before calling buildTodoContinuationUserMessage/processAgentResponse and include a clear emitEvent/processLogger entry referencing sessionId when aborting so callers can diagnose the forced stop; update any tests or callers that assumed unlimited looping.src/context/message-history.ts (1)
44-50:⚠️ Potential issue | 🟡 Minor
trimTrailingNewlines에서providerOptions필드가 유실될 수 있음Line 47에서
{ type: "text" as const, text: trimmedText }로만 새 객체를 만들고 있어, 원본textPart에 존재할 수 있는 선택적 필드providerOptions가 조용히 사라집니다. 같은 기능을 구현한 테스트 파일(line 48)과trim-leading-newlines.ts미들웨어(line 22)는 모두{ ...textPart, text: trimmedText }방식으로 스프레드를 사용해 모든 필드를 보존하는 반면, 프로덕션 코드는 그렇지 않습니다. 스프레드 방식으로 정렬하는 것이 좋습니다.♻️ 수정 제안
- const newContent = content.map((part, idx) => { - if (idx === lastTextIndex) { - return { type: "text" as const, text: trimmedText }; - } - return part; - }); + const newContent = [...content]; + newContent[lastTextIndex] = { ...textPart, text: trimmedText };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/message-history.ts` around lines 44 - 50, In trimTrailingNewlines' content.map (where part, idx, lastTextIndex and trimmedText are used) you currently return a new object that only sets { type: "text", text: trimmedText } which drops optional fields like providerOptions; change the return to spread the original part and overwrite text (e.g. return { ...part, text: trimmedText } or { ...part, type: "text" as const, text: trimmedText } ) so all existing fields (providerOptions, etc.) are preserved.
🧹 Nitpick comments (16)
src/tools/explore/safety-utils.ts (1)
88-91:FileCheckResult내보내기 제거 고려
checkFileReadable이 비공개 함수이므로 이를 반환하는 공개 함수가 없습니다.FileCheckResult인터페이스는 현재 외부에서 사용되지 않으므로 불필요한 공개 API 표면을 줄이기 위해 export를 제거하는 것을 권장합니다.♻️ 제안된 수정
-export interface FileCheckResult { +interface FileCheckResult { allowed: boolean; reason?: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore/safety-utils.ts` around lines 88 - 91, FileCheckResult is exported but only used by the private helper checkFileReadable and not exposed by any public API; remove the unnecessary export by making the interface non-exported (change "export interface FileCheckResult" to "interface FileCheckResult") and adjust any internal references in checkFileReadable or nearby functions to the now-local FileCheckResult type to reduce public surface area.src/tools/execute/noninteractive-wrapper.ts (1)
200-203:hasFlag: 루프 내부에서 매번 새로운 RegExp 인스턴스 생성 + 향후 메타문자 포함 값 추가 시 버그 위험
hasFlag는args.filter()콜백 내부에서 반복 호출되므로 루프마다 새로운RegExp인스턴스를 생성합니다(코딩 가이드라인: "Use top-level regex literals instead of creating them in loops").현재
suffixArgs값(["-y"],["--non-interactive"],["--noconfirm"],["-auto-approve"],["--no-document"])은 모두 정규식 메타문자를 포함하지 않아 안전하지만, 향후.,+,*등의 문자가 포함된 플래그가 추가될 경우 패턴이 의도와 다르게 동작할 수 있습니다.CLI 플래그는 단순 정확한 문자열 매칭이므로, 정규식을 사용하지 않고 공백으로 분리하는 방식이 더 간단하고 안전합니다.
♻️ 공백 분리 방식으로 변경하는 제안
function hasFlag(command: string, flag: string): boolean { - const flagPattern = new RegExp(`(^|\\s)${flag}(\\s|$)`); - return flagPattern.test(command); + return command.split(/\s+/).includes(flag); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/execute/noninteractive-wrapper.ts` around lines 200 - 203, The hasFlag function currently builds a new RegExp on every call (called repeatedly from args.filter) and risks misbehaving if flags later include regex meta-characters; change hasFlag to avoid regex: split the command string on whitespace into tokens (e.g., command.split(/\s+/)) and check for exact equality with the flag (use tokens.includes(flag)), so hasFlag(command, flag) performs a simple token-based membership test and removes the dynamic RegExp creation.src/tool-fallback-mode.ts (1)
29-31:"disable"리터럴 대신DEFAULT_TOOL_FALLBACK_MODE를 사용하고, 죽은 코드 조건을 제거하세요.
value === "disable"는 line 21의isToolFallbackMode(value)검사에서 이미 처리되므로 이 분기에 도달하지 않습니다. 또한 반환값을 리터럴"disable"로 직접 하드코딩하면DEFAULT_TOOL_FALLBACK_MODE가 변경될 경우 이 분기만 조용히 불일치가 생깁니다.♻️ 리팩토링 제안
- if (value === "off" || value === "disable" || value === "false") { - return "disable"; + if (value === "off" || value === "false") { + return DEFAULT_TOOL_FALLBACK_MODE; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tool-fallback-mode.ts` around lines 29 - 31, The branch returns the hardcoded literal "disable" and redundantly checks value === "disable" even though isToolFallbackMode(value) already handles that case; replace the literal with DEFAULT_TOOL_FALLBACK_MODE and remove the unreachable value === "disable" condition so the block becomes a single branch that maps off/false to DEFAULT_TOOL_FALLBACK_MODE, keeping isToolFallbackMode(value) handling earlier; update any references to the function or constant names (isToolFallbackMode, DEFAULT_TOOL_FALLBACK_MODE, value) as needed.src/agent.ts (2)
27-35:AgentStreamOptions가 export 되지 않았습니다.
AgentStreamResult는 export 되었지만AgentStreamOptions는 export 되지 않았습니다.agentManager.stream()이 public API이므로, 외부 소비자가 이 타입을 참조해야 할 수 있습니다. 현재는 선택적 필드만 있어 인라인 객체로 전달 가능하지만, 타입 명시가 필요한 경우 문제가 될 수 있습니다.♻️ export 추가 제안
-interface AgentStreamOptions { +export interface AgentStreamOptions { abortSignal?: AbortSignal; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` around lines 27 - 35, AgentStreamOptions is not exported while AgentStreamResult is and agentManager.stream() is part of the public API, so export the AgentStreamOptions interface to allow external consumers to reference the type; locate the AgentStreamOptions declaration in src/agent.ts and add an export modifier (export interface AgentStreamOptions) so the type is part of the module's public surface while leaving AgentStreamResult as-is.
141-154:createAgent의 반환 타입이 명시되지 않았습니다.
createAgent가{ stream: (...) => ... }형태의 객체를 반환하지만 반환 타입이 추론에 의존합니다. 이전에는 클래스 인스턴스를 반환했을 것으로 보이는데, 명시적 인터페이스를 정의하면 API 계약이 더 명확해집니다.As per coding guidelines: "Use explicit types for function parameters and return values when they enhance clarity"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` around lines 141 - 154, The createAgent function currently returns an object literal relying on type inference; add an explicit return type (e.g., define an Agent interface with a stream method signature) and annotate createAgent to return that interface so the API is clear and stable. Update or add an explicit interface that references the stream method signature using AgentStreamOptions and ModelMessage (and any required types like abortSignal) and change the createAgent declaration to return that interface; ensure the stream implementation still calls streamText with wrappedModel, system, tools, messages, maxOutputTokens, providerOptions, stopWhen, and abortSignal to match the declared types.src/interaction/pi-tui-stream-renderer.ts (3)
65-72:BACKTICK_FENCE_PATTERN이matchAll과 함께 안전하게 동작하는지 확인하세요.모듈 레벨의 global 정규식(
/g플래그)은 statefullastIndex를 가지고 있어,exec/test반복 호출 시 예기치 않은 동작을 일으킬 수 있습니다. 다만String.prototype.matchAll은 내부적으로 regex를 복제하므로 여기서는 안전합니다. 향후 이 정규식을exec나test로 사용하게 될 경우를 대비해 의도를 명확히 하는 것을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 65 - 72, The BACKTICK_FENCE_PATTERN use inside renderCodeBlock with String.prototype.matchAll is currently safe but the pattern being a module-level global regex can be stateful if later used with exec/test; to fix, make the intent explicit by either defining BACKTICK_FENCE_PATTERN without the /g flag (use a non-global RegExp) or always use a fresh RegExp instance (e.g., new RegExp(BACKTICK_FENCE_PATTERN.source, BACKTICK_FENCE_PATTERN.flags)) before calling .exec/.test, and document/rename the symbol (BACKTICK_FENCE_PATTERN) or add a comment near renderCodeBlock to indicate matchAll-safe usage to prevent future misuse.
297-311: ANSI 상수가cli.ts와 중복 정의되어 있습니다.
ANSI_RESET,ANSI_DIM,ANSI_ITALIC,ANSI_GRAY,ANSI_BG_GRAY등이 이 파일과src/entrypoints/cli.ts모두에 정의되어 있습니다. 공통 모듈(예:src/interaction/colors.ts또는 새 ANSI 유틸)로 추출하면 DRY 원칙을 준수하고 유지보수가 용이해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 297 - 311, This file duplicates ANSI constants and helper functions; extract the shared symbols (ANSI_RESET, ANSI_DIM, ANSI_ITALIC, ANSI_GRAY, ANSI_BG_GRAY, LEADING_NEWLINES, TRAILING_NEWLINES) and the helpers applyReadPreviewBackground and styleThinkingText into a new shared colors/ANSI utility module, export them, and replace the local definitions with imports in this file and in the CLI file so both use the single canonical implementation.
91-98:as Record<string, unknown>타입 단언 대신 런타임 타입 가드를 고려하세요.
extractReadFilePath(Line 96)과tryExtractEditPayload(Line 534) 모두input as Record<string, unknown>패턴을 사용합니다. 코딩 가이드라인에서 타입 단언보다 타입 내로잉(narrowing)을 선호합니다.typeof input === "object" && input !== null체크 후에는as없이 프로퍼티 접근이 가능합니다.♻️ 예시 (extractReadFilePath)
const extractReadFilePath = (input: unknown): string | null => { if (typeof input !== "object" || input === null) { return null; } - const record = input as Record<string, unknown>; - return typeof record.path === "string" ? record.path : null; + if ("path" in input && typeof (input as { path: unknown }).path === "string") { + return (input as { path: string }).path; + } + return null; };As per coding guidelines: "Leverage TypeScript's type narrowing instead of type assertions"
Also applies to: 522-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 91 - 98, The functions extractReadFilePath and tryExtractEditPayload use type assertions like "input as Record<string, unknown>"; replace these with runtime type-narrowing and property checks (e.g., confirm input is an object and not null, use the "in" operator or typeof checks for each required property such as "path" in extractReadFilePath and the specific edit payload fields in tryExtractEditPayload) and access properties without "as" assertions so TypeScript infers narrowed types safely at runtime; update both functions to perform explicit existence and type checks for each property before returning values.src/entrypoints/cli.ts (2)
1458-1465:isNativeCommand하드코딩 리스트가 확장에 취약합니다.새로운 "네이티브" 명령어가 추가될 때 이 리스트를 수동으로 업데이트해야 합니다.
Command인터페이스에isNative또는showLoader플래그를 추가하는 것이 더 확장 가능한 접근입니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 1458 - 1465, Replace the hardcoded isNativeCommand name-check with a property on the command definition: add an optional boolean (e.g., isNative or showLoader) to the Command interface and set it when commands are registered; then, where resolvedCommandName is used, look up the resolved Command object (the same registry/map used to resolve commands) and use command.isNative (or command.showLoader === false) to decide whether to call ui.showLoader("Running command...") instead of the current string-list check; update command registration sites to set the flag for existing native commands like "clear", "think", and "tool-fallback".
711-896:showThinkSelector와showToolFallbackSelector에 상당한 코드 중복이 있습니다.두 함수 모두 동일한 패턴을 따릅니다: 컨테이너 생성 → SelectList 설정 → 입력 리스너 등록 → Promise 반환(cleanup, finish, cancel). 공통
showSelector헬퍼를 추출하면 코드 중복을 줄일 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 711 - 896, Extract a shared helper showSelector that encapsulates the duplicated pattern used by showThinkSelector and showToolFallbackSelector: create a selector container, mount a SelectList, register the same input listener/cleanup/finish logic (using statusContainer, tui.requestRender, setActiveModalCancel, isCtrlCInput, handleCtrlCPress, shouldClearPendingExitConfirmation, clearPendingExitConfirmation), and return a Promise resolving to the selected value or null; implement showSelector to accept parameters (title text, select items array, initialIndex, and an optional transform function to map SelectList item.value to the desired return type), then refactor showThinkSelector to call showSelector with its two items (and simple identity/boolean mapping) and refactor showToolFallbackSelector to call showSelector with its TOOL_FALLBACK_MODES-built items, currentModeIndex, and parseToolFallbackMode as the transform; keep SelectList creation/selection UI details (editorTheme.selectList, buildCurrentIndicatorLabel usage) in the callers while centralizing the lifecycle and input handling in showSelector.src/interaction/pi-tui-stream-renderer.test.ts (1)
97-103:as never캐스트 사용에 대한 설명을 고려해주세요.
reasoning-start/reasoning-end등의 파트 타입이TextStreamPart<ToolSet>유니온에 직접 포함되지 않아as never로 캐스팅한 것으로 보입니다. 간단한 주석으로 이 의도를 명시하면 향후 유지보수에 도움이 됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.test.ts` around lines 97 - 103, Add a short inline comment explaining the use of the as never cast for the { type: "reasoning-start", id: "reason_1" } and { type: "reasoning-end", id: "reason_1" } values: note that these part types are intentionally not part of the TextStreamPart<ToolSet> union in this test and are being cast to never to satisfy the compiler for test construction; reference the specific literals ("reasoning-start"/"reasoning-end") and the TextStreamPart<ToolSet> union in the comment so future maintainers understand the intent.src/context/skills-integration.test.ts (1)
95-104:toPromptsCommandName/parsePromptsCommandName테스트를 전용 파일로 분리 고려lines 95~104의 두 테스트는
skill-command-prefix.ts에 대한 순수 유닛 테스트이며, 스킬 시스템과 통합 의존성이 없습니다. 현재 위치(Skills Integration Tests)에 두어도 작동하지만, 별도의skill-command-prefix.test.ts파일로 분리하면 테스트 파일 간 관심사가 더 명확해집니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/skills-integration.test.ts` around lines 95 - 104, These two pure-unit tests for toPromptsCommandName and parsePromptsCommandName should be moved out of the integration test file into a dedicated unit test file: remove the two tests from src/context/skills-integration.test.ts and create a new skill-command-prefix.test.ts containing the same test cases to isolate unit tests for the functions toPromptsCommandName and parsePromptsCommandName; keep assertions identical so behavior is preserved and run them with the existing test runner.src/commands/aliases-and-tool-fallback.test.ts (1)
68-72:isSkillCommandResult타입 가드 활용 고려
"isSkill" in result && result.isSkill패턴이 두 테스트에서 반복되고 있습니다../index에서 이미isSkillCommandResult타입 가드를 export하고 있으므로 이를 활용하면 중복을 줄이고 타입 내로잉이 더 명확해집니다.♻️ 제안 리팩터링
-import { executeCommand, registerCommand } from "./index"; +import { executeCommand, isSkillCommandResult, registerCommand } from "./index";- expect(result && "isSkill" in result && result.isSkill).toBe(true); - - if (result && "isSkill" in result && result.isSkill) { - expect(result.skillId).toBe("prompts:example"); - } + expect(isSkillCommandResult(result)).toBe(true); + + if (isSkillCommandResult(result)) { + expect(result.skillId).toBe("prompts:example"); + }Also applies to: 80-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/aliases-and-tool-fallback.test.ts` around lines 68 - 72, Replace the repeated checks '"isSkill" in result && result.isSkill' with the exported type guard isSkillCommandResult(result) from ./index to reduce duplication and improve type narrowing; update both occurrences that assert result and then check isSkill (the blocks around the expect(result && "isSkill" in result && result.isSkill) and the subsequent if) to use if (isSkillCommandResult(result)) { expect(result.skillId).toBe("prompts:example"); } (and similarly for the second block) so the tests use the single type guard for clearer type-coverage and fewer repetitive conditions.src/context/skills.ts (1)
459-463: LGTM!
parsePromptsCommandName이 null을 반환하는 경우(prefix 없거나"prompts:"뒤가 빈 경우) 올바르게 원래skillId로 폴백합니다.선택적으로 아래처럼 한 줄로 단순화할 수 있습니다.
♻️ 선택적 단순화 제안
- const promptsSkillId = parsePromptsCommandName(skillId); - const skill = promptsSkillId - ? allSkills.find((s) => s.id === promptsSkillId) - : allSkills.find((s) => s.id === skillId); + const targetId = parsePromptsCommandName(skillId) ?? skillId; + const skill = allSkills.find((s) => s.id === targetId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/skills.ts` around lines 459 - 463, The current logic sets promptsSkillId = parsePromptsCommandName(skillId) and then chooses skill via a ternary checking promptsSkillId; simplify by collapsing into a single find that compares s.id to either the parsed ID or the original skillId: call parsePromptsCommandName(skillId) once, use its non-null result when present, otherwise fall back to skillId when searching allSkills for the matching id (adjust variable names skill, promptsSkillId accordingly).src/entrypoints/headless.ts (1)
127-145: 인덱스 기반for루프 대신while루프로 리팩터링을 권장합니다코딩 가이드라인에 따라 인덱스 기반
for루프보다for...of를 선호해야 하나, 인수 소비(i +=) 특성상 직접for...of는 어렵습니다. 대신 배열을 큐로 사용하는while루프로 동일한 의도를 더 명시적으로 표현할 수 있습니다.♻️ 리팩터링 제안
- for (let i = 0; i < args.length; i++) { - const arg = args[i]; - - if (arg === "-p" || arg === "--prompt") { - prompt = args[i + 1] || ""; - i++; - } else if (arg === "-m" || arg === "--model") { - model = args[i + 1] || undefined; - i++; - } else if (arg === "--think") { - thinking = true; - } else { - const toolFallbackOption = parseToolFallbackCliOption(args, i); - if (toolFallbackOption) { - toolFallbackMode = toolFallbackOption.mode; - i += toolFallbackOption.consumedArgs; - } - } - } + const remaining = [...args]; + while (remaining.length > 0) { + const arg = remaining.shift()!; + if (arg === "-p" || arg === "--prompt") { + prompt = remaining.shift() ?? ""; + } else if (arg === "-m" || arg === "--model") { + model = remaining.shift() ?? undefined; + } else if (arg === "--think") { + thinking = true; + } else if (arg === "--tool-fallback-mode") { + const next = remaining[0]; + if (next && !next.startsWith("--")) { + remaining.shift(); + const parsedMode = parseToolFallbackMode(next); + toolFallbackMode = parsedMode ?? DEFAULT_TOOL_FALLBACK_MODE; + } + } else if (arg === "--tool-fallback") { + const next = remaining[0]; + if (next && !next.startsWith("--")) { + remaining.shift(); + const parsedMode = parseToolFallbackMode(next); + toolFallbackMode = parsedMode ?? LEGACY_ENABLED_TOOL_FALLBACK_MODE; + } else { + toolFallbackMode = LEGACY_ENABLED_TOOL_FALLBACK_MODE; + } + } + }As per coding guidelines: "Prefer
for...ofloops over.forEach()and indexedforloops".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/headless.ts` around lines 127 - 145, The indexed for loop over args in headless.ts makes arg consumption via i++/i+= toolFallbackOption.consumedArgs awkward; refactor to a while loop that treats args as a queue (shift/pop) or an index-based while so consumption is explicit: iterate while args.length > 0 (or while idx < args.length), shift the current token into arg and, when you need the next token for prompt or model, shift it immediately; when parseToolFallbackCliOption returns a toolFallbackOption use its consumedArgs to shift that many additional tokens and set toolFallbackMode, and preserve existing behavior for setting prompt, model, thinking, and toolFallbackMode (symbols: args, arg, prompt, model, thinking, toolFallbackMode, parseToolFallbackCliOption, toolFallbackOption).src/context/message-history.test.ts (1)
144-161: 배열 콘텐츠 경로에 대한MessageHistory테스트 누락새로 추가된
MessageHistorydescribe 블록은 문자열 콘텐츠 트리밍만 검증합니다. 기존의trimTrailingNewlines단위 테스트(라인 80–141)는 이 파일 내 로컬 복사본을 테스트하는 것이라 프로덕션 구현을 직접 커버하지 않습니다. 특히 배열 콘텐츠(TextPart + ToolCallPart 혼합 등)의 복잡한 경로는MessageHistory를 통해서도 검증되어야 합니다.✅ 추가 테스트 케이스 제안
it("trims trailing newlines in array content when storing assistant messages", () => { const history = new MessageHistory(); history.addModelMessages([ { role: "assistant", content: [ { type: "text", text: "Thinking...\n\n" }, { type: "tool-call", toolCallId: "call_1", toolName: "shell_command", input: { command: "ls" }, }, ], }, ]); const messages = history.toModelMessages(); const content = messages[0].content as Array<{ type: string; text?: string }>; expect(content[0].text).toBe("Thinking..."); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/context/message-history.test.ts` around lines 144 - 161, The new MessageHistory tests only cover string trimming but miss trimming when message.content is an array; add a unit test in the MessageHistory describe block that uses MessageHistory, calls addModelMessages with an assistant message whose content is an array (include a text part with trailing newlines and a tool-call part), then call toModelMessages and assert that the first content array element's text has had trailing newlines removed (use MessageHistory, addModelMessages, toModelMessages and check content[0].text === "Thinking...").
🤖 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/commands/index.ts`:
- Around line 19-54: registerCommand currently misses the case where the new
command's canonical name is already registered as an alias for another command;
add a check near the top of registerCommand (after computing normalizedName)
that queries commandAliases.get(normalizedName) (or
commandAliases.has(normalizedName)) and, if present and mapping !==
normalizedName, throw an Error like "Command name 'normalizedName' conflicts
with existing alias for 'existingTarget'". This prevents creating a canonical
command that is unreachable; refer to registerCommand, commands, and
commandAliases to locate where to insert the check.
In `@src/commands/model.ts`:
- Around line 79-112: applyModelSelection fails to clear a previous modelType
when switching to Anthropic-style models that lack a type field; after you call
agentManager.setModelId(selectedModel.id) ensure you explicitly reset modelType
when selectedModel.type is falsy by calling agentManager.setModelType with a
clear value (e.g., undefined/null/empty) instead of only setting it when
selectedModel.type exists, so that agentManager.getModelType() no longer returns
the prior value after provider/model changes.
In `@src/entrypoints/cli.ts`:
- Around line 83-84: ANSI_CYAN and ANSI_BRIGHT_CYAN are both set to "\x1b[96m"
(bright cyan); either change ANSI_CYAN to the standard cyan code "\x1b[36m" if
you intended a normal cyan, or remove/alias one of the constants to avoid
duplication; update the declarations for ANSI_CYAN / ANSI_BRIGHT_CYAN
accordingly so their values reflect distinct intended colors or have a single
source of truth.
In `@src/entrypoints/headless.ts`:
- Around line 88-106: The --tool-fallback-mode branch currently always consumes
the next token even if it's a flag or an invalid mode; update the logic in
headless.ts so that --tool-fallback-mode only consumes the following arg when
that arg exists, does not start with "--", and parseToolFallbackMode(candidate)
returns a non-null value; if any of those checks fail, return consumedArgs: 0
and mode: DEFAULT_TOOL_FALLBACK_MODE instead of consuming the token (use the
same pattern as the --tool-fallback branch and reference parseToolFallbackMode,
DEFAULT_TOOL_FALLBACK_MODE, and LEGACY_ENABLED_TOOL_FALLBACK_MODE to locate the
code).
- Around line 387-391: Replace the unsafe type assertion on the "finish-step"
handler by using the explicit discriminated Extract pattern: instead of "part as
{ finishReason: string }", cast to Extract<StreamPart, { type: \"finish-step\"
}> and assign its finishReason to lastFinishReason; follow the same pattern used
in pi-tui-stream-renderer.ts so the handler references the discriminated
StreamPart type and updates lastFinishReason accordingly.
In `@src/tools/execute/shell-execute.ts`:
- Around line 8-11: The exported function executeCommand currently returns the
internal interface CommandResult which is not exported; export the CommandResult
interface so it becomes part of the public API (make CommandResult exported) and
update any imports/usages if necessary; specifically locate the CommandResult
interface declaration and change it to an exported interface so the exported
function executeCommand's return type is publicly visible for declaration
generation.
---
Outside diff comments:
In `@src/context/message-history.ts`:
- Around line 44-50: In trimTrailingNewlines' content.map (where part, idx,
lastTextIndex and trimmedText are used) you currently return a new object that
only sets { type: "text", text: trimmedText } which drops optional fields like
providerOptions; change the return to spread the original part and overwrite
text (e.g. return { ...part, text: trimmedText } or { ...part, type: "text" as
const, text: trimmedText } ) so all existing fields (providerOptions, etc.) are
preserved.
In `@src/entrypoints/headless.ts`:
- Around line 465-480: The outer while(true) loop that fetches
getIncompleteTodos() lacks a safety cap and can loop forever; add a bounded
iteration limit (reintroduce a CONTINUATION_MAX_LOOPS constant or similar) and a
counter in headless.ts around the loop that breaks and logs/emits an error event
when the limit is reached. Ensure the counter is checked each iteration before
calling buildTodoContinuationUserMessage/processAgentResponse and include a
clear emitEvent/processLogger entry referencing sessionId when aborting so
callers can diagnose the forced stop; update any tests or callers that assumed
unlimited looping.
In `@src/interaction/spinner.ts`:
- Around line 34-45: The early return in stop() leaks the timer: move the
spinnerOutputEnabled guard so that clearInterval(this.interval) and
this.interval = null always run regardless of spinnerOutputEnabled; only the
process.stdout.write calls should be conditional. Update the stop() method
(referencing stop(), start(), this.interval and spinnerOutputEnabled /
setSpinnerOutputEnabled) to always clear and null out this.interval, then if
spinnerOutputEnabled perform the "\r\x1B[K" and "\x1B[?25h" writes.
In `@src/tools/execute/noninteractive-wrapper.ts`:
- Around line 276-284: The current wasModified uses Object.keys(env).length>0
which is always true because getBaseEnv() injects keys; change the modification
check to only consider command string changes and whether new env keys beyond
the base env were added: compute baseEnv = getBaseEnv() (or its keys) and set
envAdded = Object.keys(env).some(k =>
!Object.prototype.hasOwnProperty.call(baseEnv,k)) (or compare env vs baseEnv),
then set wasModified = wrappedCommand !== trimmedCommand || envAdded; update
wrapCommandNonInteractive's fallback so it can return wrapped: false when
neither the command nor extra env changed (remove the hardcoded wrapped: true),
and update getFullWrappedCommand/WrapperResult.wrapped usage accordingly (either
remove the dead early return or rely on the corrected wrapped flag) so the
description and early-return branches become reachable and consistent.
---
Nitpick comments:
In `@src/agent.ts`:
- Around line 27-35: AgentStreamOptions is not exported while AgentStreamResult
is and agentManager.stream() is part of the public API, so export the
AgentStreamOptions interface to allow external consumers to reference the type;
locate the AgentStreamOptions declaration in src/agent.ts and add an export
modifier (export interface AgentStreamOptions) so the type is part of the
module's public surface while leaving AgentStreamResult as-is.
- Around line 141-154: The createAgent function currently returns an object
literal relying on type inference; add an explicit return type (e.g., define an
Agent interface with a stream method signature) and annotate createAgent to
return that interface so the API is clear and stable. Update or add an explicit
interface that references the stream method signature using AgentStreamOptions
and ModelMessage (and any required types like abortSignal) and change the
createAgent declaration to return that interface; ensure the stream
implementation still calls streamText with wrappedModel, system, tools,
messages, maxOutputTokens, providerOptions, stopWhen, and abortSignal to match
the declared types.
In `@src/commands/aliases-and-tool-fallback.test.ts`:
- Around line 68-72: Replace the repeated checks '"isSkill" in result &&
result.isSkill' with the exported type guard isSkillCommandResult(result) from
./index to reduce duplication and improve type narrowing; update both
occurrences that assert result and then check isSkill (the blocks around the
expect(result && "isSkill" in result && result.isSkill) and the subsequent if)
to use if (isSkillCommandResult(result)) {
expect(result.skillId).toBe("prompts:example"); } (and similarly for the second
block) so the tests use the single type guard for clearer type-coverage and
fewer repetitive conditions.
In `@src/context/message-history.test.ts`:
- Around line 144-161: The new MessageHistory tests only cover string trimming
but miss trimming when message.content is an array; add a unit test in the
MessageHistory describe block that uses MessageHistory, calls addModelMessages
with an assistant message whose content is an array (include a text part with
trailing newlines and a tool-call part), then call toModelMessages and assert
that the first content array element's text has had trailing newlines removed
(use MessageHistory, addModelMessages, toModelMessages and check content[0].text
=== "Thinking...").
In `@src/context/skills-integration.test.ts`:
- Around line 95-104: These two pure-unit tests for toPromptsCommandName and
parsePromptsCommandName should be moved out of the integration test file into a
dedicated unit test file: remove the two tests from
src/context/skills-integration.test.ts and create a new
skill-command-prefix.test.ts containing the same test cases to isolate unit
tests for the functions toPromptsCommandName and parsePromptsCommandName; keep
assertions identical so behavior is preserved and run them with the existing
test runner.
In `@src/context/skills.ts`:
- Around line 459-463: The current logic sets promptsSkillId =
parsePromptsCommandName(skillId) and then chooses skill via a ternary checking
promptsSkillId; simplify by collapsing into a single find that compares s.id to
either the parsed ID or the original skillId: call
parsePromptsCommandName(skillId) once, use its non-null result when present,
otherwise fall back to skillId when searching allSkills for the matching id
(adjust variable names skill, promptsSkillId accordingly).
In `@src/entrypoints/cli.ts`:
- Around line 1458-1465: Replace the hardcoded isNativeCommand name-check with a
property on the command definition: add an optional boolean (e.g., isNative or
showLoader) to the Command interface and set it when commands are registered;
then, where resolvedCommandName is used, look up the resolved Command object
(the same registry/map used to resolve commands) and use command.isNative (or
command.showLoader === false) to decide whether to call ui.showLoader("Running
command...") instead of the current string-list check; update command
registration sites to set the flag for existing native commands like "clear",
"think", and "tool-fallback".
- Around line 711-896: Extract a shared helper showSelector that encapsulates
the duplicated pattern used by showThinkSelector and showToolFallbackSelector:
create a selector container, mount a SelectList, register the same input
listener/cleanup/finish logic (using statusContainer, tui.requestRender,
setActiveModalCancel, isCtrlCInput, handleCtrlCPress,
shouldClearPendingExitConfirmation, clearPendingExitConfirmation), and return a
Promise resolving to the selected value or null; implement showSelector to
accept parameters (title text, select items array, initialIndex, and an optional
transform function to map SelectList item.value to the desired return type),
then refactor showThinkSelector to call showSelector with its two items (and
simple identity/boolean mapping) and refactor showToolFallbackSelector to call
showSelector with its TOOL_FALLBACK_MODES-built items, currentModeIndex, and
parseToolFallbackMode as the transform; keep SelectList creation/selection UI
details (editorTheme.selectList, buildCurrentIndicatorLabel usage) in the
callers while centralizing the lifecycle and input handling in showSelector.
In `@src/entrypoints/headless.ts`:
- Around line 127-145: The indexed for loop over args in headless.ts makes arg
consumption via i++/i+= toolFallbackOption.consumedArgs awkward; refactor to a
while loop that treats args as a queue (shift/pop) or an index-based while so
consumption is explicit: iterate while args.length > 0 (or while idx <
args.length), shift the current token into arg and, when you need the next token
for prompt or model, shift it immediately; when parseToolFallbackCliOption
returns a toolFallbackOption use its consumedArgs to shift that many additional
tokens and set toolFallbackMode, and preserve existing behavior for setting
prompt, model, thinking, and toolFallbackMode (symbols: args, arg, prompt,
model, thinking, toolFallbackMode, parseToolFallbackCliOption,
toolFallbackOption).
In `@src/interaction/pi-tui-stream-renderer.test.ts`:
- Around line 97-103: Add a short inline comment explaining the use of the as
never cast for the { type: "reasoning-start", id: "reason_1" } and { type:
"reasoning-end", id: "reason_1" } values: note that these part types are
intentionally not part of the TextStreamPart<ToolSet> union in this test and are
being cast to never to satisfy the compiler for test construction; reference the
specific literals ("reasoning-start"/"reasoning-end") and the
TextStreamPart<ToolSet> union in the comment so future maintainers understand
the intent.
In `@src/interaction/pi-tui-stream-renderer.ts`:
- Around line 65-72: The BACKTICK_FENCE_PATTERN use inside renderCodeBlock with
String.prototype.matchAll is currently safe but the pattern being a module-level
global regex can be stateful if later used with exec/test; to fix, make the
intent explicit by either defining BACKTICK_FENCE_PATTERN without the /g flag
(use a non-global RegExp) or always use a fresh RegExp instance (e.g., new
RegExp(BACKTICK_FENCE_PATTERN.source, BACKTICK_FENCE_PATTERN.flags)) before
calling .exec/.test, and document/rename the symbol (BACKTICK_FENCE_PATTERN) or
add a comment near renderCodeBlock to indicate matchAll-safe usage to prevent
future misuse.
- Around line 297-311: This file duplicates ANSI constants and helper functions;
extract the shared symbols (ANSI_RESET, ANSI_DIM, ANSI_ITALIC, ANSI_GRAY,
ANSI_BG_GRAY, LEADING_NEWLINES, TRAILING_NEWLINES) and the helpers
applyReadPreviewBackground and styleThinkingText into a new shared colors/ANSI
utility module, export them, and replace the local definitions with imports in
this file and in the CLI file so both use the single canonical implementation.
- Around line 91-98: The functions extractReadFilePath and tryExtractEditPayload
use type assertions like "input as Record<string, unknown>"; replace these with
runtime type-narrowing and property checks (e.g., confirm input is an object and
not null, use the "in" operator or typeof checks for each required property such
as "path" in extractReadFilePath and the specific edit payload fields in
tryExtractEditPayload) and access properties without "as" assertions so
TypeScript infers narrowed types safely at runtime; update both functions to
perform explicit existence and type checks for each property before returning
values.
In `@src/tool-fallback-mode.ts`:
- Around line 29-31: The branch returns the hardcoded literal "disable" and
redundantly checks value === "disable" even though isToolFallbackMode(value)
already handles that case; replace the literal with DEFAULT_TOOL_FALLBACK_MODE
and remove the unreachable value === "disable" condition so the block becomes a
single branch that maps off/false to DEFAULT_TOOL_FALLBACK_MODE, keeping
isToolFallbackMode(value) handling earlier; update any references to the
function or constant names (isToolFallbackMode, DEFAULT_TOOL_FALLBACK_MODE,
value) as needed.
In `@src/tools/execute/noninteractive-wrapper.ts`:
- Around line 200-203: The hasFlag function currently builds a new RegExp on
every call (called repeatedly from args.filter) and risks misbehaving if flags
later include regex meta-characters; change hasFlag to avoid regex: split the
command string on whitespace into tokens (e.g., command.split(/\s+/)) and check
for exact equality with the flag (use tokens.includes(flag)), so
hasFlag(command, flag) performs a simple token-based membership test and removes
the dynamic RegExp creation.
In `@src/tools/explore/safety-utils.ts`:
- Around line 88-91: FileCheckResult is exported but only used by the private
helper checkFileReadable and not exposed by any public API; remove the
unnecessary export by making the interface non-exported (change "export
interface FileCheckResult" to "interface FileCheckResult") and adjust any
internal references in checkFileReadable or nearby functions to the now-local
FileCheckResult type to reduce public surface area.
Apply upstream pi-mono pattern: simple timestamp check (500ms window) with direct process.exit() instead of fragile promise-based exit. - Replace boolean/counter state machine with timestamp-only Ctrl+C handling - Add exitWithCleanup() for guaranteed tmux cleanup on all exit paths - Add idempotent process signal handlers (SIGTERM, SIGHUP, SIGQUIT) - Add stale tmux session scavenging via owner PID tracking - Update Anthropic model IDs to latest versions
Address PR review feedback: - Add inline comment explaining stepCountIs as replacement for deprecated maxSteps - Add Architecture section to README explaining the streamText + stopWhen pattern
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (12)
src/tools/explore/glob.ts (1)
82-84: 불필요한 블록 바디 제거 가능단일 표현식 반환에 블록 바디(
{ return f.path; })를 사용하고 있습니다. 간결한 화살표 함수 표현식으로 단순화하세요.♻️ 제안된 리팩터
- const body = displayFiles - .map((f) => { - return f.path; - }) - .join("\n"); + const body = displayFiles + .map((f) => f.path) + .join("\n");As per coding guidelines: "Use arrow functions for callbacks and short functions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/explore/glob.ts` around lines 82 - 84, The map callback currently uses a block body with an explicit return (.map((f) => { return f.path; })) — replace the block-bodied arrow function with a concise arrow expression to return the property directly (e.g., use a single-expression arrow for the map callback that returns f.path) so the code follows the "arrow functions for callbacks and short functions" guideline and removes the unnecessary block/return.src/tools/execute/shared-tmux-session.test.ts (1)
8-10: 테스트 상수가 프로덕션 코드의 상수를 중복 정의하고 있습니다.
TMUX_OWNER_PID_KEY("CEA_OWNER_PID")와TMUX_SESSION_PREFIX("cea-")가 프로덕션 코드(shared-tmux-session.ts)의OWNER_PID_ENV_KEY,SESSION_PREFIX와 동일한 값을 하드코딩하고 있습니다. 프로덕션 값이 변경되면 테스트가 잘못된 값으로 계속 통과할 수 있습니다.프로덕션 모듈에서 이 상수들을 export하고 테스트에서 import하는 것이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/execute/shared-tmux-session.test.ts` around lines 8 - 10, Replace the duplicated test constants TMUX_OWNER_PID_KEY and TMUX_SESSION_PREFIX with imports of the production exports OWNER_PID_ENV_KEY and SESSION_PREFIX from shared-tmux-session.ts; update the test file to import OWNER_PID_ENV_KEY and SESSION_PREFIX (instead of hardcoding "CEA_OWNER_PID" and "cea-") and use those symbols throughout the tests so changes in production values propagate to tests.src/agent.ts (2)
146-146:options.instructions || SYSTEM_PROMPT에서||대신??사용을 권장합니다.빈 문자열(
"")이 전달되면||는 이를 falsy로 처리하여SYSTEM_PROMPT로 폴백합니다. 의도적일 수 있지만, 코딩 가이드라인의 nullish coalescing(??) 사용 권장 사항에 따라??가 더 명시적입니다. As per coding guidelines, "Use optional chaining (?.) and nullish coalescing (??) for safer property access."♻️ 수정 제안
- system: options.instructions || SYSTEM_PROMPT, + system: options.instructions ?? SYSTEM_PROMPT,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` at line 146, Replace the use of logical OR with nullish coalescing for the system prompt assignment: change the expression that sets system (currently using options.instructions || SYSTEM_PROMPT) to use options.instructions ?? SYSTEM_PROMPT so that an intentionally empty string is preserved while only null/undefined fall back to SYSTEM_PROMPT; locate the assignment where the system field is set (the object using options.instructions and SYSTEM_PROMPT in src/agent.ts) and update it accordingly.
85-156:createAgent가 매 호출마다 새 agent를 생성하며, 반환 타입이 명시되지 않았습니다.
AgentManager.stream이 호출될 때마다createAgent를 재호출하여wrapLanguageModel및buildMiddlewares를 반복 실행합니다. 현재 규모에서는 무시할 수 있지만, 설정이 변경되지 않았을 때 캐싱을 고려할 수 있습니다. 또한 코딩 가이드라인에 따라 함수의 반환 타입을 명시하면 API 명확성이 향상됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` around lines 85 - 156, createAgent currently recreates agents on every call (causing repeated wrapLanguageModel/buildMiddlewares work) and lacks an explicit return type; change createAgent to declare and export a concrete Agent return type (e.g., Agent with a stream method) and add simple memoization keyed by (modelId, provider, thinkingEnabled, options.toolFallbackMode, options.instructions) so repeated calls return a cached wrappedModel and middleware instead of rebuilding them each time; ensure cache invalidates when any of those inputs change and update references like AgentManager.stream to accept the typed Agent.src/entrypoints/cli.ts (4)
815-849: 도구 폴백 모드 목록이 하드코딩되어TOOL_FALLBACK_MODES와 동기화 이탈 위험이 있습니다.
SelectList의 항목이"disable","morphxml","hermes","qwen3coder"4개로 하드코딩되어 있으며,TOOL_FALLBACK_MODES배열과 순서/항목이 정확히 일치해야TOOL_FALLBACK_MODES.indexOf(currentMode)(라인 850)가 올바른 인덱스를 반환합니다. 모드가 추가/변경되면 이 부분이 깨질 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 815 - 849, The SelectList items are hardcoded which can drift from TOOL_FALLBACK_MODES; change the construction to derive items from TOOL_FALLBACK_MODES (use TOOL_FALLBACK_MODES.map(...)) so values and labels come from that source, calling buildCurrentIndicatorLabel(value, currentMode===value) for each entry and looking up descriptions from a single source (e.g., TOOL_FALLBACK_MODE_DESCRIPTIONS or a switch/fallback to the value) and pass editorTheme.selectList; also replace the fixed index (4) with TOOL_FALLBACK_MODES.indexOf(currentMode) so selection remains correct when modes change.
929-930:providerLabel매핑이 2개 프로바이더만 처리하며 fallback이 없습니다.
ProviderType이 현재"friendli" | "anthropic"이지만, 향후 프로바이더 추가 시 이 하드코딩된 삼항 연산자는 잘못된 레이블을 표시합니다. 기본값(fallback)을 추가하거나 매핑 객체 사용을 고려하세요.♻️ 개선 제안
- const providerLabel = - model.provider === "anthropic" ? "Anthropic" : "FriendliAI"; + const providerLabels: Record<string, string> = { + anthropic: "Anthropic", + friendli: "FriendliAI", + }; + const providerLabel = providerLabels[model.provider] ?? model.provider;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 929 - 930, The providerLabel calculation currently uses a two-way ternary (providerLabel) that only handles "anthropic" vs FriendliAI and has no fallback; update providerLabel to derive its value from a mapping object keyed by model.provider (or a switch) and include a sensible default label (e.g., model.provider or "Unknown Provider") to cover future ProviderType values; reference the model.provider field and ProviderType union when implementing the mapping to ensure new providers map to correct human-readable labels.
571-1132:createCliUi함수가 560줄 이상으로 인지 복잡도가 높습니다.UI 생성, 입력 처리, 모달 표시(model/think/tool-fallback selector), 이벤트 리스너 관리가 모두 하나의 팩토리 함수에 포함되어 있습니다. 각 selector를 별도 함수나 모듈로 분리하면 가독성과 테스트 용이성이 개선됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 571 - 1132, createCliUi is too large and does UI construction, input handling, modal selectors and lifecycle management in one function; split responsibilities by extracting showModelSelector, showThinkSelector and showToolFallbackSelector (each returning Promise and managing their own Container, SelectList/Input and tui input listener cleanup) into their own module/functions, move input listener setup/teardown (tui.addInputListener usage and removeInputListener), signal handlers (onSigInt) and dispose logic into a small lifecycle manager, and keep createCliUi to assemble components and wire the exposed methods (updateHeader, waitForInput, requestExit, showLoader, and the selector call sites) so each extracted function (showModelSelector, showThinkSelector, showToolFallbackSelector, dispose, removeInputListener) is independently testable and the factory only wires them together.
1326-1341: 중단 시buildTodoContinuationUserMessage가 history에 추가되지만 UI에 표시되지 않습니다.
processAgentResponse가"completed"를 반환한 후 미완료 TODO가 있으면reminder가messageHistory.addUserMessage로 추가되지만(라인 1339),addUserMessage(ui.chatContainer, ...)를 통해 UI에 렌더링되지 않습니다. 사용자에게는 보이지 않는 자동 continuation이므로 의도된 것일 수 있지만, 디버깅 시 혼란을 줄 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entrypoints/cli.ts` around lines 1326 - 1341, The code adds the follow-up reminder to the message history via messageHistory.addUserMessage(reminder) but never renders it to the CLI UI; update handleAgentResponse so after building the reminder with buildTodoContinuationUserMessage and calling messageHistory.addUserMessage(reminder) you also render the same message to the UI (e.g., call the UI rendering helper used elsewhere such as addUserMessage(ui.chatContainer, reminder) or the equivalent method on the CliUi instance) so the reminder is both persisted and visible to the user; reference messageHistory.addUserMessage, buildTodoContinuationUserMessage, and ui.chatContainer when making the change.src/interaction/pi-tui-stream-renderer.ts (3)
613-615:render()내부의 인라인 정규식을 모듈 레벨 상수로 이동해야 합니다
/\t/g정규식이render(width)호출마다 새로 생성됩니다.TruncatedReadBody.render는 UI 업데이트마다 반복 호출될 수 있으므로, 다른 패턴 상수들과 함께 모듈 최상위에 정의해야 합니다.♻️ 리팩터링 제안
모듈 상단 상수 섹션에 추가:
+const TAB_PATTERN = /\t/g;
render메서드 내부:- const normalizedText = this.text.replace(/\t/g, " "); + const normalizedText = this.text.replace(TAB_PATTERN, " ");As per coding guidelines, "Use top-level regex literals instead of creating them in loops".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 613 - 615, The inline regex /\t/g created on each render call should be hoisted to a module-level constant to avoid reallocating it on every UI update; define a top-level constant (e.g., TAB_RE = /\t/g) near the other pattern constants and replace uses in TruncatedReadBody.render (the line creating normalizedText) to use that constant instead of the inline literal.
103-119:extractGlobPattern과extractGrepPattern은 동일한 구현입니다두 함수 모두
input객체에서pattern필드를 추출하는 동일한 로직을 가집니다. 또한extractReadFilePath도 필드명만 다른 동일한 패턴입니다. 세 함수를 하나의 제네릭 헬퍼로 통합하면 중복을 제거할 수 있습니다.♻️ 리팩터링 제안
-const extractReadFilePath = (input: unknown): string | null => { - if (typeof input !== "object" || input === null) { - return null; - } - const record = input as Record<string, unknown>; - return typeof record.path === "string" ? record.path : null; -}; - -const extractGlobPattern = (input: unknown): string | null => { - if (typeof input !== "object" || input === null) { - return null; - } - const record = input as Record<string, unknown>; - return typeof record.pattern === "string" ? record.pattern : null; -}; - -const extractGrepPattern = (input: unknown): string | null => { - if (typeof input !== "object" || input === null) { - return null; - } - const record = input as Record<string, unknown>; - return typeof record.pattern === "string" ? record.pattern : null; -}; +const extractStringField = (input: unknown, field: string): string | null => { + if (typeof input !== "object" || input === null) { + return null; + } + const record = input as Record<string, unknown>; + return typeof record[field] === "string" ? (record[field] as string) : null; +}; + +const extractReadFilePath = (input: unknown): string | null => + extractStringField(input, "path"); +const extractGlobPattern = (input: unknown): string | null => + extractStringField(input, "pattern"); +const extractGrepPattern = (input: unknown): string | null => + extractStringField(input, "pattern");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 103 - 119, The three functions extractGlobPattern, extractGrepPattern, and extractReadFilePath duplicate the same logic; replace them with a single generic helper that extracts a string field from an unknown input (e.g., function extractStringField(input: unknown, field: string): string | null) and update extractGlobPattern, extractGrepPattern, and extractReadFilePath to delegate to that helper (or remove the wrappers and call the helper directly) so all three use the same implementation and eliminate duplication.
963-1072:tryRenderReadFileMode/tryRenderGlobMode/tryRenderGrepMode메서드 구조가 중복됩니다세 메서드 모두 동일한 패턴을 따릅니다: 도구명/에러/거부 여부 확인 → 입력 파싱 → 출력이 있으면 렌더링 / 없으면 대기 상태 표시. 설정 객체를 통해 단일 메서드로 통합하면 향후 새 도구 추가 시 변경 지점이 줄어듭니다.
또한
refresh()에서 세tryRender*Mode()메서드가 각각 독립적으로resolveBestInput()을 호출하므로 매 새로고침마다 3회 호출됩니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.ts` around lines 963 - 1072, All three methods (tryRenderReadFileMode, tryRenderGlobMode, tryRenderGrepMode) share the same control flow and should be consolidated into a single generic renderer to remove duplication and avoid calling resolveBestInput() multiple times: create a helper like tryRenderMode(toolConfig, bestInput) where toolConfig includes the toolName to check, extractInputFn (e.g., extractReadFilePath/extractGlobPattern/extractGrepPattern), renderOutputFn (renderReadFileOutput/renderGlobOutput/renderGrepOutput), renderPendingFn (renderReadFilePendingOutput/renderGlobPendingOutput/renderGrepPendingOutput), and label (“Read”/“Glob”/“Grep”); move the common checks (this.toolName/toolName, this.error, this.outputDenied), reuse this.setReadMode, this.readHeader, this.readBody, and safeStringify, and update refresh() to call resolveBestInput() once and pass bestInput into the unified tryRenderMode for each tool config.src/interaction/pi-tui-stream-renderer.test.ts (1)
387-416:as never타입 단언 대신as unknown as TestStreamPart를 사용하는 것이 좋습니다
as never캐스팅은 TypeScript의 모든 타입 검사를 우회하여, 실제 타입 오류가 있어도 컴파일러가 감지하지 못합니다.as unknown as TestStreamPart를 사용하면 의도적인 타입 변환임을 명시하면서도 변수 사용 지점에서 타입 힌트를 유지합니다.♻️ 리팩터링 제안
- { type: "tool-input-start", - toolCallId: "call_3", - toolName: "write_file", - } as never, - { - type: "tool-input-delta", - toolCallId: "call_3", - inputTextDelta: '{"path":"src/big.ts","content":"chunk"}', - } as never, + { + type: "tool-input-start", + toolCallId: "call_3", + toolName: "write_file", + } as unknown as TestStreamPart, + { + type: "tool-input-delta", + toolCallId: "call_3", + inputTextDelta: '{"path":"src/big.ts","content":"chunk"}', + } as unknown as TestStreamPart,다른
as never사용 위치(Lines 97-99, 115-117, 127-129 등)에도 동일하게 적용하세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interaction/pi-tui-stream-renderer.test.ts` around lines 387 - 416, Replace the unsafe "as never" casts in the test helpers with a proper two-step cast to preserve type hints: change occurrences like the cast on the objects passed to renderParts (e.g., the literal objects with type fields used in the "supports toolCallId and inputTextDelta aliases" test) from "as never" to "as unknown as TestStreamPart"; update other similar sites (the other tests using "as never" at the noted locations) so the compiler still accepts the test data while keeping TestStreamPart type information for renderParts and related helpers.
🤖 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/entrypoints/cli.ts`:
- Around line 1160-1191: The parseToolFallbackCliOption implementation is
duplicated and missing validation for the argument after "--tool-fallback-mode";
extract parseToolFallbackCliOption into a shared utility (e.g., a new cli-args
module) and update its logic so when arg === "--tool-fallback-mode" you first
check the next argument exists and does not start with "--" (use nextArg =
args[index+1]) and if missing or startsWith("--") return consumedArgs: 0 with
mode: DEFAULT_TOOL_FALLBACK_MODE; otherwise parse nextArg via
parseToolFallbackMode and return consumedArgs: 1 with the parsed mode (fallback
to DEFAULT_TOOL_FALLBACK_MODE on parse failure); keep the existing
"--tool-fallback" legacy branch (using LEGACY_ENABLED_TOOL_FALLBACK_MODE) and
replace usages in both locations to import the new shared
parseToolFallbackCliOption.
- Around line 1575-1583: The cleanupTmuxSession function currently always calls
cleanupSession; wrap the cleanupSession() call in the same env guard used in
headless.ts so it only runs when env.TMUX_CLEANUP_SESSION is truthy: inside
cleanupTmuxSession (which uses tmuxCleanupExecuted) add if
(env.TMUX_CLEANUP_SESSION) { cleanupSession(); } preserving the
tmuxCleanupExecuted flag behavior; if this change is intentional instead, add an
explanatory comment referencing env.TMUX_CLEANUP_SESSION and why cli.ts differs
from headless.ts.
In `@src/interaction/pi-tui-stream-renderer.ts`:
- Around line 249-257: The fallback that extracts a filename from a full path in
resolveReadPath is unreachable because pathValue.trim() is falsy when that
branch runs; move the filename-extraction step (the
pathValue.split(PATH_SEPARATOR_PATTERN).at(-1)?.trim() logic) to run immediately
after checking pathValue (i.e., before falling back to parsed.blockTitle and
"(unknown)") or remove it if unnecessary. Specifically, update resolveReadPath
so it first reads pathValue from parsed.metadata, then if non-empty use the
PATH_SEPARATOR_PATTERN split to derive the basename, otherwise fall back to
parsed.blockTitle or "(unknown)"; ensure references remain to resolveReadPath,
PATH_SEPARATOR_PATTERN, parsed.metadata, and parsed.blockTitle.
In `@src/tools/execute/shared-tmux-session.test.ts`:
- Around line 12-26: The catch in findUnusedPid currently treats any error from
process.kill(pid, 0) as "unused"; change it to only treat ESRCH as unused and
treat EPERM as "in use" (or rethrow/continue). Specifically, in function
findUnusedPid(), inspect the caught error's code (NodeJS.ErrnoException.code)
and return candidate only if code === 'ESRCH'; for other errors (e.g., 'EPERM')
do not return that PID and continue searching (or rethrow unexpected errors).
Apply the same fix to isProcessAlive() so that EPERM is considered the process
being alive rather than false.
In `@src/tools/execute/shared-tmux-session.ts`:
- Around line 163-174: isProcessAlive currently treats all errors from
process.kill(pid, 0) as the process being dead; change it to treat an EPERM
error as alive: in the catch block inspect the thrown error (cast to
NodeJS.ErrnoException or similar) and if error.code === 'EPERM' return true,
otherwise return false; update the isProcessAlive method accordingly so callers
like cleanupStaleOwnedSessions() won't remove sessions owned by other users.
In `@src/tools/explore/glob.test.ts`:
- Around line 60-71: The tests "does not include mtime for each file" and "does
not include numbered list" only use negative assertions which can pass on empty
or errored output; update each test to also assert positively that the expected
file(s) are present in the result from executeGlob({ pattern: "*.ts", path:
tempDir }) — e.g., after calling executeGlob within those it blocks, add an
expect that the known test file name or tempDir path appears (using
expect(result).toContain(...)) to ensure the output is non-empty and the
negative checks are meaningful.
---
Duplicate comments:
In `@src/entrypoints/cli.ts`:
- Around line 83-84: Duplicate ANSI constants: ANSI_CYAN and ANSI_BRIGHT_CYAN
are both set to the bright-cyan value; change ANSI_CYAN to the regular cyan ANSI
code (ANSI 36) so code paths that use ANSI_CYAN (e.g., createMarkdownTheme's
link) render the intended non-bright cyan; update the constant ANSI_CYAN
definition and run a quick smoke test of createMarkdownTheme to confirm links
and other uses show the expected color.
In `@src/entrypoints/headless.ts`:
- Around line 431-435: The "finish-step" case uses an unsafe type assertion
(part as { finishReason: string }); instead, narrow the discriminated union by
casting part to Extract<StreamPart, { type: "finish-step" }> (or by using a
local typed variable) and read its finishReason into lastFinishReason; update
the case labeled "finish-step" to use that Extract<StreamPart, { type:
"finish-step" }> type for part so the compiler guarantees finishReason exists
when assigning lastFinishReason.
- Around line 132-137: The --tool-fallback-mode branch currently unconditionally
consumes the next arg (using parseToolFallbackMode(args[index + 1] ?? "")) which
lets a subsequent flag like "--think" be swallowed; update the handling in the
headless args parser so before calling parseToolFallbackMode you verify the
candidate (args[index + 1]) does not start with "--" (similar to the
--tool-fallback branch) and only then consume it, otherwise return consumedArgs:
0 and use DEFAULT_TOOL_FALLBACK_MODE; reference parseToolFallbackMode,
DEFAULT_TOOL_FALLBACK_MODE, the --tool-fallback-mode flag handling, and the
args/index variables when making the change.
---
Nitpick comments:
In `@src/agent.ts`:
- Line 146: Replace the use of logical OR with nullish coalescing for the system
prompt assignment: change the expression that sets system (currently using
options.instructions || SYSTEM_PROMPT) to use options.instructions ??
SYSTEM_PROMPT so that an intentionally empty string is preserved while only
null/undefined fall back to SYSTEM_PROMPT; locate the assignment where the
system field is set (the object using options.instructions and SYSTEM_PROMPT in
src/agent.ts) and update it accordingly.
- Around line 85-156: createAgent currently recreates agents on every call
(causing repeated wrapLanguageModel/buildMiddlewares work) and lacks an explicit
return type; change createAgent to declare and export a concrete Agent return
type (e.g., Agent with a stream method) and add simple memoization keyed by
(modelId, provider, thinkingEnabled, options.toolFallbackMode,
options.instructions) so repeated calls return a cached wrappedModel and
middleware instead of rebuilding them each time; ensure cache invalidates when
any of those inputs change and update references like AgentManager.stream to
accept the typed Agent.
In `@src/entrypoints/cli.ts`:
- Around line 815-849: The SelectList items are hardcoded which can drift from
TOOL_FALLBACK_MODES; change the construction to derive items from
TOOL_FALLBACK_MODES (use TOOL_FALLBACK_MODES.map(...)) so values and labels come
from that source, calling buildCurrentIndicatorLabel(value, currentMode===value)
for each entry and looking up descriptions from a single source (e.g.,
TOOL_FALLBACK_MODE_DESCRIPTIONS or a switch/fallback to the value) and pass
editorTheme.selectList; also replace the fixed index (4) with
TOOL_FALLBACK_MODES.indexOf(currentMode) so selection remains correct when modes
change.
- Around line 929-930: The providerLabel calculation currently uses a two-way
ternary (providerLabel) that only handles "anthropic" vs FriendliAI and has no
fallback; update providerLabel to derive its value from a mapping object keyed
by model.provider (or a switch) and include a sensible default label (e.g.,
model.provider or "Unknown Provider") to cover future ProviderType values;
reference the model.provider field and ProviderType union when implementing the
mapping to ensure new providers map to correct human-readable labels.
- Around line 571-1132: createCliUi is too large and does UI construction, input
handling, modal selectors and lifecycle management in one function; split
responsibilities by extracting showModelSelector, showThinkSelector and
showToolFallbackSelector (each returning Promise and managing their own
Container, SelectList/Input and tui input listener cleanup) into their own
module/functions, move input listener setup/teardown (tui.addInputListener usage
and removeInputListener), signal handlers (onSigInt) and dispose logic into a
small lifecycle manager, and keep createCliUi to assemble components and wire
the exposed methods (updateHeader, waitForInput, requestExit, showLoader, and
the selector call sites) so each extracted function (showModelSelector,
showThinkSelector, showToolFallbackSelector, dispose, removeInputListener) is
independently testable and the factory only wires them together.
- Around line 1326-1341: The code adds the follow-up reminder to the message
history via messageHistory.addUserMessage(reminder) but never renders it to the
CLI UI; update handleAgentResponse so after building the reminder with
buildTodoContinuationUserMessage and calling
messageHistory.addUserMessage(reminder) you also render the same message to the
UI (e.g., call the UI rendering helper used elsewhere such as
addUserMessage(ui.chatContainer, reminder) or the equivalent method on the CliUi
instance) so the reminder is both persisted and visible to the user; reference
messageHistory.addUserMessage, buildTodoContinuationUserMessage, and
ui.chatContainer when making the change.
In `@src/interaction/pi-tui-stream-renderer.test.ts`:
- Around line 387-416: Replace the unsafe "as never" casts in the test helpers
with a proper two-step cast to preserve type hints: change occurrences like the
cast on the objects passed to renderParts (e.g., the literal objects with type
fields used in the "supports toolCallId and inputTextDelta aliases" test) from
"as never" to "as unknown as TestStreamPart"; update other similar sites (the
other tests using "as never" at the noted locations) so the compiler still
accepts the test data while keeping TestStreamPart type information for
renderParts and related helpers.
In `@src/interaction/pi-tui-stream-renderer.ts`:
- Around line 613-615: The inline regex /\t/g created on each render call should
be hoisted to a module-level constant to avoid reallocating it on every UI
update; define a top-level constant (e.g., TAB_RE = /\t/g) near the other
pattern constants and replace uses in TruncatedReadBody.render (the line
creating normalizedText) to use that constant instead of the inline literal.
- Around line 103-119: The three functions extractGlobPattern,
extractGrepPattern, and extractReadFilePath duplicate the same logic; replace
them with a single generic helper that extracts a string field from an unknown
input (e.g., function extractStringField(input: unknown, field: string): string
| null) and update extractGlobPattern, extractGrepPattern, and
extractReadFilePath to delegate to that helper (or remove the wrappers and call
the helper directly) so all three use the same implementation and eliminate
duplication.
- Around line 963-1072: All three methods (tryRenderReadFileMode,
tryRenderGlobMode, tryRenderGrepMode) share the same control flow and should be
consolidated into a single generic renderer to remove duplication and avoid
calling resolveBestInput() multiple times: create a helper like
tryRenderMode(toolConfig, bestInput) where toolConfig includes the toolName to
check, extractInputFn (e.g.,
extractReadFilePath/extractGlobPattern/extractGrepPattern), renderOutputFn
(renderReadFileOutput/renderGlobOutput/renderGrepOutput), renderPendingFn
(renderReadFilePendingOutput/renderGlobPendingOutput/renderGrepPendingOutput),
and label (“Read”/“Glob”/“Grep”); move the common checks
(this.toolName/toolName, this.error, this.outputDenied), reuse this.setReadMode,
this.readHeader, this.readBody, and safeStringify, and update refresh() to call
resolveBestInput() once and pass bestInput into the unified tryRenderMode for
each tool config.
In `@src/tools/execute/shared-tmux-session.test.ts`:
- Around line 8-10: Replace the duplicated test constants TMUX_OWNER_PID_KEY and
TMUX_SESSION_PREFIX with imports of the production exports OWNER_PID_ENV_KEY and
SESSION_PREFIX from shared-tmux-session.ts; update the test file to import
OWNER_PID_ENV_KEY and SESSION_PREFIX (instead of hardcoding "CEA_OWNER_PID" and
"cea-") and use those symbols throughout the tests so changes in production
values propagate to tests.
In `@src/tools/explore/glob.ts`:
- Around line 82-84: The map callback currently uses a block body with an
explicit return (.map((f) => { return f.path; })) — replace the block-bodied
arrow function with a concise arrow expression to return the property directly
(e.g., use a single-expression arrow for the map callback that returns f.path)
so the code follows the "arrow functions for callbacks and short functions"
guideline and removes the unnecessary block/return.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/agent.ts (2)
27-29:AgentStreamOptions내보내기 누락
AgentManager.stream()의options파라미터 타입으로 사용되고 있지만export가 없습니다.agentManager는 외부로 내보내지므로(268번째 줄),abortSignal을 명시적으로 타이핑하는 호출 코드에서 이 타입을 임포트할 수 없습니다.♻️ 수정 제안
-interface AgentStreamOptions { +export interface AgentStreamOptions { abortSignal?: AbortSignal; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` around lines 27 - 29, The interface AgentStreamOptions is declared but not exported, so external callers cannot import the type used by AgentManager.stream(); export the interface by changing its declaration to an exported interface (export interface AgentStreamOptions { abortSignal?: AbortSignal }) so call sites that reference AgentManager.stream() or the exported agentManager can import and use the AbortSignal-typed options.
144-156:||대신??사용 필요
options.instructions || SYSTEM_PROMPT(146번째 줄)에서||를 사용하면 빈 문자열""도 거짓 값으로 처리되어SYSTEM_PROMPT로 대체됩니다. 가이드라인에 따라??를 사용해야 합니다.♻️ 수정 제안
- system: options.instructions || SYSTEM_PROMPT, + system: options.instructions ?? SYSTEM_PROMPT,As per coding guidelines, "Use optional chaining (
?.) and nullish coalescing (??) for safer property access".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent.ts` around lines 144 - 156, The call to streamText uses the || operator for the system prompt which treats empty string as falsy; update the argument to use nullish coalescing (and optional chaining if options may be undefined) so the system value becomes system: options?.instructions ?? SYSTEM_PROMPT; locate this change around the streamText invocation (reference symbols: streamText, options.instructions, SYSTEM_PROMPT) and replace the || usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/agent.ts`:
- Around line 27-29: The interface AgentStreamOptions is declared but not
exported, so external callers cannot import the type used by
AgentManager.stream(); export the interface by changing its declaration to an
exported interface (export interface AgentStreamOptions { abortSignal?:
AbortSignal }) so call sites that reference AgentManager.stream() or the
exported agentManager can import and use the AbortSignal-typed options.
- Around line 144-156: The call to streamText uses the || operator for the
system prompt which treats empty string as falsy; update the argument to use
nullish coalescing (and optional chaining if options may be undefined) so the
system value becomes system: options?.instructions ?? SYSTEM_PROMPT; locate this
change around the streamText invocation (reference symbols: streamText,
options.instructions, SYSTEM_PROMPT) and replace the || usage accordingly.
- Fix spinner interval leak: always clear interval in stop() regardless of spinnerOutputEnabled flag to prevent timer leaks - Fix trimTrailingNewlines: use spread operator to preserve providerOptions and other optional fields on TextPart - Add safety cap (20 iterations) to todo continuation loop in headless mode to prevent infinite loops when model keeps generating incomplete todos - Remove dead code in noninteractive-wrapper: wasModified is always true since getBaseEnv() always returns env keys; simplify to wrapped: true - Fix isProcessAlive EPERM handling: treat EPERM as process alive since it means the process exists but we lack permission to signal it - Fix findUnusedPid in tests: only treat ESRCH as unused PID - Fix --tool-fallback-mode arg swallowing: validate next arg does not start with -- before consuming it (both cli.ts and headless.ts) - Add TMUX_CLEANUP_SESSION env guard to cli.ts cleanupTmuxSession to match headless.ts behavior - Fix unreachable code in resolveReadPath: remove dead PATH_SEPARATOR_PATTERN fallback branch that could never execute - Use Extract type narrowing for finish-step in headless.ts instead of unsafe type assertion - Use nullish coalescing (??) for system prompt in agent.ts - Export AgentStreamOptions interface for external consumers - Remove unnecessary export from FileCheckResult interface - Replace regex in hasFlag with string split for simpler, safer matching - Fix tool-fallback-mode: remove unreachable disable check, use constant - Add positive assertions to glob tests alongside negative ones - Fix ANSI_CYAN to use regular cyan instead of bright cyan - Hoist tab regex to module-level TAB_PATTERN constant - Extract extractStringField helper to deduplicate field extraction logic
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
This PR introduces a new TUI (Text User Interface) based CLI experience using the
@mariozechner/pi-tuilibrary, replacing the previous stream-renderer implementation with a more interactive and modern terminal interface.Changes
src/interaction/pi-tui-stream-renderer.ts) for richer terminal outputstdin-buffer.tsand replaced with TUI input handlingTesting
pi-tui-stream-renderer.test.ts,tool-loop-control.test.tsaliases-and-tool-fallback.test.tsskills-integration.test.tsBreaking Changes
@mariozechner/pi-tuias a dependencystream-renderer.ts- usepi-tui-stream-renderer.tsinsteadstdin-buffer.ts- replaced with TUI-based input handlingSummary by CodeRabbit
새로운 기능
개선사항
테스트