feat(cli): /goal pause|resume|clear control from the TUI - #3026
Conversation
850847b to
36a9b7f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review. 📝 WalkthroughProblem solvedAdds direct TUI controls for autonomous goals:
The commands avoid model turns. The TUI reports confirmations, automatic goal attachment, external pauses, invalid transitions, missing goals, and mutation rejection during active turns. Source of truthThe PR extends the existing Complexity deltaThe PR adds:
The PR adds no new goal authority, durable state store, configuration, or protocol family. It adds control states, transition branches, conflict retries, user-visible notices, and test-maintenance burden. These additions are necessary for direct controls, concurrency handling, session recovery, and clear feedback. The solution is the smallest coherent path identified in the current diff. Some retry and test patterns could be shared or simplified later. Those findings are non-blocking follow-up items. Overall maintenance complexity increases, but the increase is bounded and justified by the feature. ValidationTests cover goal controls, busy-turn rejection, invalid transitions, missing goals, revision conflicts and retries, goal removal, snapshot updates, pause and attachment notices, notice suppression, session restoration, status formatting, elapsed time, sanitization, truncation, and terminal states. The current repository state provides no direct evidence for final required-check status. Required checks remain unverified. Review-relevant risksThe PR changes user-visible TUI behavior. Material changes in this area require independent human review under repository policy. The PR adds the optional The PR changes runtime-host goal-control and optimistic-concurrency behavior. Material changes in this area require independent human review under repository policy. No protected-area effect was identified for security, licensing, releases, or governance in the current diff. The person performing the merge must review the final diff. A maintainer makes the final determination. WalkthroughThe CLI adds direct ChangesAutonomous goal control
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🔵 Low · up to The terminal-output sanitization change may still allow certain escape sequences to alter rendering in the TUI, so the PR is mergeable with explicit owner awareness and follow-up to close that bounded terminal-safety risk. Sequence Diagram(s)sequenceDiagram
participant Operator
participant pi-tui-runner
participant MakaSessionDriver
participant RuntimeHost
Operator->>pi-tui-runner: /goal pause, resume, or clear
pi-tui-runner->>MakaSessionDriver: controlGoal(action)
MakaSessionDriver->>RuntimeHost: goal.control(expectedRevision)
RuntimeHost-->>MakaSessionDriver: updated projection or error
MakaSessionDriver-->>pi-tui-runner: projection or null
pi-tui-runner-->>Operator: confirmation or error notice
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoAdd autonomous goal visibility and controls to the TUI
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/cli/src/__tests__/pi-goal.test.ts (1)
128-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider dropping this test; line 89 already covers it.
assert.deepEqual(detailed.slice(2), ['Tokens: 45k / 100k', …])at line 89 already proves thatgoalSummaryLinesroutes token counts throughformatTokenCount. This test re-asserts the formatter itself, which belongs topi-transcript-format.ts.♻️ Proposed removal
- test('token formatting is the shared status-line formatter', () => { - assert.equal(formatTokenCount(45_200), '45k'); - }); -Then drop the now-unused import at line 5.
As per path instructions: "Flag tests that duplicate existing coverage, assert implementation details, or do not protect observable behavior."
Source: Path instructions
packages/cli/src/pi-tui-runner.ts (1)
1071-1079: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the command-token parse instead of hardcoding
/goal.Line 1075 re-implements the token extraction that
handleSlashCommandalready performs at line 2842, and it matches the literal name only. If/goalever gains an alias inSLASH_COMMAND_CATALOG, the alias steers into the model mid-turn instead of answering locally.♻️ Optional consolidation
Extract the token parse next to
handleSlashCommand:const slashCommandToken = (prompt: string): string => prompt.trim().split(/\s+/, 1)[0] ?? '';Then use it in both places, and match against the resolved command instead of the literal:
- if (prompt.trim().split(/\s+/, 1)[0] === '/goal') { + if (resolveSlashCommand(slashCommandToken(prompt))?.name === 'goal') { editor.addToHistory(prompt); handleSlashCommand(prompt, 0); return; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ef1278d-7a12-4741-a207-7fcf7cba9041
📒 Files selected for processing (12)
packages/cli/src/__tests__/pi-goal.test.tspackages/cli/src/__tests__/pi-transcript.test.tspackages/cli/src/__tests__/pi-tui-runner.test.tspackages/cli/src/__tests__/runtime-host-session-driver.test.tspackages/cli/src/pi-goal.tspackages/cli/src/pi-transcript-format.tspackages/cli/src/pi-transcript.tspackages/cli/src/pi-tui-runner.tspackages/cli/src/runtime-host-session-channel.tspackages/cli/src/runtime-host-session-driver.tspackages/cli/src/session-driver.tspackages/core/src/slash-command-catalog.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 1 remains after this review.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
b810dbd to
bd02b63
Compare
|
Thanks for the rework — the design is genuinely minimal: goal state stays in runtime-host's Conclusion: PASS — no P0/P1/P2. P3 (optional): the last retry attempt still drops the host's reason, contradicting the PR's own fix claim ("a conflict at an unchanged revision rethrows the host's message") — AI-assisted review disclosure: this review was produced with AI assistance (pi review subagent on 中文摘要（AI 辅助审查）结论：PASS（无 P0/P1/P2）。设计真最小：goal 状态仍由 runtime-host 的 GoalManager 持有（单一权威），CLI 只做投影读取/订阅/控制转发，经既有 goal.control 协议 + expectedRevision 乐观并发（与 desktop clearGoal 同构）；"re-query 后 revision 未变 ⇒ 状态拒绝而非竞态"的判定成立（host 每次接受转换必 bump revision，已逐一核对客户端预校验与 host 规则：pause 需 active|waiting、resume 需 paused、clear 拒绝 terminal）；无新协议/无轮询/无并行 authority。测试扎实（driver revision 序列/冲突重试/goal 消失/busy 门禁/notice 抑制/预校验）。CI 18/18 绿。P3（可选）：末次重试仍丢 host 原因与 PR 自述修复矛盾——attempt===MAX-1 时 break 跳过 re-query 落到通用"revision conflict after 3 attempts"；两次竞态后遇状态拒绝（另一客户端先 pause 再 clear、本端 pause 到达 cleared）显示误导消息。一行修复（throw conflict 或 re-query 移到 break 前）+ 补"两次竞态后状态拒绝"用例。另：controlGoal 跨重试复用捕获 sessionId 无会话代际护栏（其他异步路径有 #assertCurrentSession；TUI busy 串行锁使实际窗口近乎理论，每 RPC 比对 generation 成本低）；resume 到已 paused 的 goal 无 notice（attach notice 只覆盖 active/waiting，订阅仅 previous?.goalId 相同时宣布暂停转换）——用户只能靠状态栏黄色段；mid-turn 路由匹配字面 /goal 而非 catalog 解析的命令名（将来加 alias 会 steering 进模型）；pi-goal.test.ts:128-130 重复 :89 的表单测试属于 pi-transcript-format 可删；goal 控制重试循环已有三份（desktop clearGoal、#3027 的 controlGoalWithRetry（#3027 仍 OPEN 未合并）、本 PR controlGoal）——PR 已注明 mirroring 可接受但值得日后抽共享控制器；formatGoalElapsed(24h) 输出 1d 0h（整日可省 0h）。 |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The Goal control design itself is coherent: Runtime Host remains authoritative, mutations carry goalId + expectedRevision, concurrent conflicts are retried, and the TUI projections/notices cover pause, resume, clear, attach, and auto-pause. Previously reported behavioral issues appear fixed.
The current head is nevertheless not reviewable as a merge result: it conflicts with latest main in the exact TUI/transcript files where main introduced locale and primary-guidance behavior. The simplest path is a clean rebase that preserves both contracts, followed by focused CLI/typecheck/CI validation; the existing green checks only validate the old base.
Review performed with two Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I reproduced the conflicts against current main.
中文评论
Goal control 设计本身闭环：Runtime Host 保持权威，mutation 携带 goalId + expectedRevision，并发冲突会重试，TUI projection/notice 覆盖 pause、resume、clear、attach 和 auto-pause。此前公开的行为问题已修复。
但当前 head 不能作为最终 merge result 审查：它与最新 main 在同一组 TUI/transcript 文件中冲突，而 main 已在这些文件引入 locale 与 primary-guidance 行为。最简单的路径是 clean rebase，同时保留两边契约，再运行 focused CLI/typecheck/CI；现有绿色检查只验证旧 base。
本次审查使用了两位 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具；我已对当前 main 复现冲突。
bd02b63 to
4c20d9f
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The control flow is otherwise well-shaped: it reuses the Host goal authority, applies optimistic concurrency with bounded retries, and keeps TUI command handling local. One display-boundary issue should be addressed before treating this as merge-ready: goal conditions and evaluator reasons are durable user/model-controlled text and are now copied into terminal notices without control-sequence sanitization. The natural owner is the shared goal-to-terminal formatter, so one sanitization step can cover summaries, attach notices, and pause notices without parallel fixes.
AI-assisted review disclosure: Codex verified the final diff, goal-control retry semantics, Session switching, notice rendering, terminal formatting, focused tests, and live CI. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs. No local tests were run.
中文复核
控制流程整体合理：复用 Host goal 权威，以有界重试实现 optimistic concurrency，并保持 TUI 命令本地处理。合并前应修复一处展示边界：goal condition 与 evaluator reason 都是持久化的用户/模型可控文本，现在会未经控制序列清理直接进入终端 notice。最自然的 owner 是共享的 goal-to-terminal formatter，一处清理即可覆盖 summary、attach notice 与 pause notice。
本次为 AI 辅助审查：Codex 核验最终 diff、goal-control 重试、Session 切换、notice rendering、terminal formatter、聚焦测试与实时 CI；另使用两次独立 reviewer 及一次 OpenCode Go DeepSeek V4 Flash（high）对抗审查。未运行本地测试。
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The current head resolves the substantive issues from the earlier pass: goal text is sanitized consistently, the final host conflict reason is preserved, and resume/pause lifecycle behavior now has focused regression coverage. The command remains owned by the Runtime Host path rather than introducing a second goal authority.
No remaining P0-P2 findings on this head. The existing bot notes about future aliases and a theoretical cross-session race are reasonable non-blocking follow-ups, but neither warrants expanding this PR.
AI-assisted review disclosure: Codex reviewed exact head 29d3773, inspected the affected goal-control paths and current CI, and found all checks green with no unresolved review threads.
中文说明
当前 head 已修复上一轮的实质问题：goal 文本清理一致、最后一次 Host conflict 原因会保留，resume/pause 生命周期也有针对性回归测试。没有剩余 P0-P2。现有 bot 提到的 alias 扩展和极端跨 session 竞态可作为非阻塞后续，不需要扩大本 PR。
|
One little thing before merging, A screenshot would be great~ |
|
Added a screenshot to the PR body showing /goal pause, resume, and clear together with the Host-projected status-line changes. |
The only way to steer an autonomous goal from the TUI was asking the model to call GoalPause/GoalResume/GoalClear — a burned model turn that can hit the goal-control decline rules. And the pause semantics were invisible: Ctrl+C on a goal continuation turn auto-pauses the durable goal, and attaching to a session with a live goal auto-continues it, both without a word. - /goal pause|resume|clear dispatch through the driver's new controlGoal (host goal.control with expectedRevision optimistic retry, mirroring the desktop client's clearGoal). Writes take the runControl gate, and typed mid-turn they refuse with a clear message instead of steering into the model or being swallowed. - A goal that transitions into paused while attached (typically the abort auto-pause) prints a one-line notice naming the reason and the /goal resume | /goal clear controls; a /goal pause we initiated prints its own confirmation instead of a duplicate notice. - Attaching to a session whose goal is active/waiting announces the running loop — a token-burning loop never resumes silently. - Invalid transitions are pre-validated against the live projection (cannot pause a cleared goal, cannot resume an active one) so they get a plain message instead of the host's operation error. Ref apache#3023 Generated-by: Maka
…e resumed live goals - /goal pause cleared selfInitiatedPauseGoalId only when the push handler observed the transition; a response-before-push ordering left the flag set and suppressed a later host-initiated pause notice. The success path now clears the flag and syncs the transition cache to the authoritative response, so the trailing push no longer duplicates the notice. - The attach-time goal notice ran before the driver adopted a resumed session, so resuming into a session with a live durable goal never announced the auto-continuing loop. switchSession now syncs the goal transition cache and emits the notice after the transcript replacement that would erase an adoption-time notice.
29d3773 to
32dbabf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Replacing an accidentally bodyless approval with the complete exact-head review and disclosure.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved on exact rebased head 32dbabf8c2c030a4ca267d6d049d5df444c46ab2.
The stack now cleanly contains only the #3026 Goal-control intent on top of merged #3025. The rebase preserves primaryGuidance.commands.goal as the single localization authority; the obsolete duplicate localization patch dropped cleanly.
Focused validation passed locally: CLI tests 310/310, CLI typecheck, Biome on every changed file, and git diff --check. No remaining P0-P2 findings.
AI-assisted review disclosure: Codex performed the controlled rebase and verified the exact delta and focused checks. Astro-Han authorized the rebase, approval, and merge.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 65a914af-480e-42d7-9014-06928461c467
📒 Files selected for processing (8)
packages/cli/src/__tests__/pi-goal.test.tspackages/cli/src/__tests__/pi-tui-runner.test.tspackages/cli/src/__tests__/runtime-host-session-driver.test.tspackages/cli/src/pi-goal.tspackages/cli/src/pi-tui-runner.tspackages/cli/src/runtime-host-session-driver.tspackages/cli/src/session-driver.tspackages/cli/src/tui-ansi.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cli/src/tests/pi-goal.test.ts
- packages/cli/src/session-driver.ts
- packages/cli/src/tests/runtime-host-session-driver.test.ts
- packages/cli/src/tests/pi-tui-runner.test.ts
- packages/cli/src/pi-tui-runner.ts
- packages/cli/src/pi-goal.ts
Included review availability: Your plan provides up to 3 included reviews per hour; 2 remain after this review.
Generated-by: Maka
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved on exact head 19b7bf3bceb4e6b509585155302beef3e2567eb2.
The follow-up delta closes the remaining terminal-sanitization gap for generic ESC sequences such as character-set selection without adding another display path. The shared stripAnsi owner remains the right seam, and the focused regression test covers the concrete sequence.
Focused validation passed locally: CLI tests 311/311, CLI typecheck, Biome on the changed files, and git diff --check. There are no unresolved review threads and no remaining P0-P2 findings.
AI-assisted review disclosure: Codex reviewed the exact new delta and reran the focused workspace validation. Astro-Han authorized the approval and merge.
中文说明
已批准精确 head 19b7bf3。这次补丁补齐了 generic ESC sequence（例如 character-set selection）的终端清理边界，并继续由共享 stripAnsi 统一负责，没有新增平行展示路径。回归测试覆盖了具体序列。
本地聚焦验证全部通过：CLI 测试 311/311、CLI typecheck、变更文件的 Biome 检查和 git diff --check。当前无未解决审查线程，也无剩余 P0-P2。
2 participants
Summary
The only way to steer an autonomous goal from the TUI was asking the model to call
GoalPause/GoalResume/GoalClear— a burned model turn that can hit the goal-control decline rules ingoal-tools.ts. And the pause semantics were invisible: Ctrl+C on a goal continuation turn auto-pauses the durable goal, and attaching to a session with a live goal auto-continues it, both without a word./goal pause|resume|cleardispatch through the driver's newcontrolGoal(hostgoal.controlwithexpectedRevisionoptimistic retry, mirroring the desktop client'sclearGoal). Writes take therunControlgate; typed mid-turn they refuse with a clear message instead of steering into the model or being silently swallowed.pausedwhile attached (typically the abort auto-pause after Ctrl+C) prints a one-line notice naming the reason and the/goal resume|/goal clearcontrols. A/goal pausewe initiated prints its own confirmation instead of a duplicate notice.Closes #3023. Stacked on #3025 (goal visibility) — merge that first; this branch will be rebased onto main.
Review process
Two independent read-only review passes (first-principles + Occam's razor). Findings addressed:
operation_conflictalongside revision races; the retry loop burned 3 round-trips on a provably-futile status refusal, then threw a misleading "revision conflict after 3 attempts" that discarded the host's reason. Now: a conflict at an unchanged revision rethrows the host's message, and pause pre-validation matches the host rules exactly (isLiveGoalStatuswrongly includedpaused;clearhad no check).inlineGoalTexthelper.Judged acceptable (documented, not fixed): the sub-frame staleness window between a successful control RPC and the pushed projection (self-heals on the next frame;
expectedRevisionprevents double-apply); a foreign controller pausing inside our own pause window has its notice suppressed (our confirmation still covers the state).Test plan
packages/cli: 278 tests pass, including new coverage:expectedRevision, conflict retry sequence[1,2,3], goal-disappeared-mid-flight → null, status conflict rethrows host reason without futile retries/goal pauserefuses with a message and never steers; pause/resume/clear confirmations; self-initiated pause suppression vs host-pushed pause notice; pre-validation for all three actions; no-goal handlingpi-goal: pause/attach notice text, whitespace collapse, 120-char capbiome checkclean;tsc --noEmitclean for@maka/cli.🤖 Generated by Maka
Visual evidence
The TUI handles pause, resume, and clear locally while the status line follows the Host-owned goal projection.
Generated-by: Maka