feat(goal): let the user arm a Goal from the composer - #3199
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used all 3 included reviews currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughProblem solvedThis PR lets users arm a Goal from the composer + menu through the new The operation:
Source of truthThe PR extends the existing The PR moves shared budget limits into The PR replaces separate Session mode setters with atomic Smallest coherent solutionThe Host operation requires coordinated changes across the protocol, coordinator, IPC, preload, runtime, and UI layers. The Goal dialog and composer entry point are required for the stated user flow. The unified Session mode removes three independent mode states and callbacks. It reduces invalid combinations and keeps persisted mode updates atomic. No code or test can be safely deleted based on the supplied diff. Boundary validation, compatibility handling, and regression coverage protect distinct behavior. Validation and risksThe PR adds coverage for:
The objectives state that typechecks, formatting, and lint coverage were updated. Final required-check status is unverified. Complexity delta
The feature increases public-surface, validation, and UI complexity. The increase is justified by the requested capability. Shared limits and unified mode state reduce related maintenance complexity. Review-relevant risks
The person performing the merge reviews the final diff. A maintainer makes the final determination. WalkthroughChangesGoal arming and unified composer modes
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to User-initiated Goal arming is not merge-ready because the desktop bridge can pass a composite session key while using a Host-local identifier, causing arming to target an unknown Session. The related test and inventory issues should also be corrected or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant GoalDialog
participant PreloadBridge
participant GoalArmIPC
participant DesktopRuntimeHostClient
participant HostGoalCoordinator
GoalDialog->>PreloadBridge: call window.maka.goal.arm
PreloadBridge->>GoalArmIPC: invoke goal:arm
GoalArmIPC->>DesktopRuntimeHostClient: call armGoal
DesktopRuntimeHostClient->>HostGoalCoordinator: request goal.arm
HostGoalCoordinator-->>GoalArmIPC: return projected Goal
GoalArmIPC-->>GoalDialog: return GoalState
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoArm Goals from the composer and unify session mode selection
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/app-shell.tsx (1)
1154-1187: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSession identity is read from render state, not from the ref the writes use.
setSessionModereadsactiveSessionMode, which is derived at render time fromactiveId. The chained writes (setPlanMode,setSwarmMode,setGraphMode) each re-readactiveIdRef.current. Between the firstawaitand the next branch the active Session can change, socurrentcan describe a Session that is no longer the write target. The result is a transition computed for one Session and applied to another.The existing pending gates limit the window, and this matches the pattern used by the permission control, so this is
follow-uprather than a blocker. If you want it closed now, capture the session id and its mode once at entry and pass both down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2723400f-0d43-4f1b-bd13-48fdddd5fc9e
📒 Files selected for processing (23)
apps/desktop/src/main/__tests__/runtime-host-client-operations.test.tsapps/desktop/src/main/__tests__/runtime-host-session-domains-ipc-main.test.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-session-domains-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/goal-dialog.tsxapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/renderer/styles/astryx-mount.cssapps/desktop/src/renderer/styles/composer.cssapps/desktop/stories/app-shell.stories.tsxpackages/core/src/goal.tspackages/runtime-host/src/__tests__/goal-coordinator.test.tspackages/runtime-host/src/__tests__/goal-protocol.test.tspackages/runtime-host/src/protocol/goal.tspackages/runtime-host/src/protocol/operations.tspackages/runtime-host/src/server/goal-coordinator.tspackages/runtime/src/goal-state.tspackages/runtime/src/goal-tools.tspackages/ui/src/components.tsxpackages/ui/src/composer.tsxpackages/ui/src/conversation-copy.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
Code Review by Qodo
1.
|
e412263 to
974a407
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/desktop/src/renderer/app-shell.tsx (1)
2188-2190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo spellings of "is this Session in swarm".
These status branches read
activeSessionForView?.orchestrationModedirectly.activeSessionModealready answers the same question through the sharedsessionModeOfauthority, andsetOrchestrationSessionMode(line 1105) uses it. Reading it here too keeps one derivation.♻️ Optional consolidation
- const active = activeIdRef.current - ? (activeSessionForView?.orchestrationMode ?? 'default') === 'swarm' - : newChatSessionMode === 'swarm'; + const active = activeSessionMode === 'swarm';Also applies to: 2234-2236
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fb46c9d-3383-409b-8d00-caf4cbbf8891
📒 Files selected for processing (18)
apps/desktop/src/main/__tests__/runtime-host-session-domains-ipc-main.test.tsapps/desktop/src/main/__tests__/session-mode-ipc-main.test.tsapps/desktop/src/main/__tests__/session-mode.test.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-session-catalog-ipc-main.tsapps/desktop/src/main/runtime-host-session-domains-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/app-shell.tsxapps/desktop/src/renderer/goal-dialog.tsxapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/shared/session-mode.tsdocs/astryx-surface-file-inventory.mddocs/astryx-surface-file-inventory.pathspackages/runtime/src/__tests__/goal-state.test.tspackages/runtime/src/goal-state.tspackages/ui/src/__tests__/composer-plus-menu.test.tsxpackages/ui/src/composer.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src/main/runtime-host-client.ts
- apps/desktop/src/main/tests/runtime-host-session-domains-ipc-main.test.ts
- packages/runtime/src/goal-state.ts
- apps/desktop/src/renderer/goal-dialog.tsx
- apps/desktop/src/main/runtime-host-session-domains-ipc-main.ts
- packages/ui/src/composer.tsx
- apps/desktop/src/renderer/locales/shell-copy.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
974a407 to
6e5ea80
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/app-shell.tsx (2)
2003-2003: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
SESSION_MODE_FIELDSfor the new-chat field derivation.Lines 2003, 2056, and 2057-2060 re-derive the mode-to-field mapping that
SESSION_MODE_FIELDSalready owns.applySessionModereads that table for the Session path, so the same mapping now has two authorities. If a fifth mode is added to the table, these ternaries still compile and map it toagent/default.♻️ Proposed reuse of the existing table
- newSessionCollaborationMode: newChatSessionMode === 'plan' ? 'plan' : 'agent', + newSessionCollaborationMode: SESSION_MODE_FIELDS[newChatSessionMode].collaborationMode,- newChatCollaborationMode: newChatSessionMode === 'plan' ? 'plan' : 'agent', - newChatOrchestrationMode: - newChatSessionMode === 'graph' || newChatSessionMode === 'swarm' - ? newChatSessionMode - : 'default', + newChatCollaborationMode: SESSION_MODE_FIELDS[newChatSessionMode].collaborationMode, + newChatOrchestrationMode: SESSION_MODE_FIELDS[newChatSessionMode].orchestrationMode,As per path instructions: "Choose remedies in this order: delete an unnecessary path, consolidate duplicated authority, reuse the closest existing seam".
Also applies to: 2056-2060
Source: Path instructions
2185-2194: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReport
/swarmand/graphstatus fromactiveSessionMode.Both status branches read
activeSessionForView?.orchestrationModedirectly. The matchingset_modebranches act onactiveSessionMode, which gives Plan precedence. For a record that carriescollaborationMode: 'plan'together withorchestrationMode: 'swarm', the status reports swarm enabled while/swarm offtreats the Session as not in swarm. ReadingactiveSessionModein both branches removes the second authority.♻️ Proposed alignment
- const active = activeIdRef.current - ? (activeSessionForView?.orchestrationMode ?? 'default') === 'swarm' - : newChatSessionMode === 'swarm'; + const active = activeSessionMode === 'swarm';- const active = activeIdRef.current - ? (activeSessionForView?.orchestrationMode ?? 'default') === 'graph' - : newChatSessionMode === 'graph'; + const active = activeSessionMode === 'graph';As per path instructions: "consolidate duplicated authority".
Also applies to: 2231-2240
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 59eb421b-1bee-404f-b5bc-2a20f71db0d0
📒 Files selected for processing (7)
apps/desktop/src/main/__tests__/runtime-host-client-operations.test.tsapps/desktop/src/main/runtime-host-client.tsapps/desktop/src/main/runtime-host-session-catalog-ipc-main.tsapps/desktop/src/renderer/app-shell.tsxpackages/runtime-host/src/__tests__/goal-coordinator.test.tspackages/runtime-host/src/server/goal-coordinator.tspackages/ui/src/conversation-copy.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/main/tests/runtime-host-client-operations.test.ts
- apps/desktop/src/main/runtime-host-client.ts
- packages/ui/src/conversation-copy.ts
- apps/desktop/src/main/runtime-host-session-catalog-ipc-main.ts
- packages/runtime-host/src/tests/goal-coordinator.test.ts
- packages/runtime-host/src/server/goal-coordinator.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.
6e5ea80 to
3e6bbc3
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
English
Requesting changes. I found five non-duplicate correctness and compatibility issues:
- an armed-but-not-started Goal begins autonomously after Host restart;
- the first Goal-bound Turn is excluded from a manually selected token budget;
- invalid pending budget text can silently arm a Goal with different or unlimited bounds;
- the new wire operation does not advance the compatibility epoch;
- a response-lost arm can commit durably while the UI reports failure.
The Host admission, scoped Session binding, conflict/archive handling, and durable flush paths otherwise look coherent. The simplify audit also found a second Desktop-side runtime schema for goal.arm; consolidating it into the canonical Host operation decoder is a worthwhile non-blocking follow-up, but I am not opening a separate inline thread because it substantially overlaps the existing single-source and exact-IPC-shape discussions.
One additional upgrade reminder: credentials issued to remote owners before this change do not contain the new goal.arm grant. The current Remote Host user population is effectively negligible and reissuing those credentials is inexpensive, so the practical impact can be ignored for this PR; it should not block the change.
All checks on the current head are green, but the affected restart, first-Turn budget, mixed-version, invalid pending-input, and dispatched-interruption paths are not covered by those tests.
简体中文
请求修改。本次发现 5 个没有与现有评论重复的 correctness 和 compatibility 问题:
- 尚未开始的手动 Goal 会在 Host 重启后自行执行;
- 首个受 Goal 约束的 Turn 不计入手动选择的 token budget;
- 无效的待输入预算可能被静默替换为其他限制或无限制;
- 新增 wire operation 后没有提升 compatibility epoch;
- arm 响应丢失时,Goal 可能已经持久化,但 UI 会错误报告失败。
除此之外,Host admission、scoped Session 绑定、conflict/archive 处理和 durable flush 路径整体一致。simplify-audit 还发现 Desktop 为 goal.arm 建立了第二套 runtime schema;将其收敛到 Host operation 的 canonical decoder 是有价值的非阻塞 follow-up,但它与现有 single-source 和 exact IPC shape 讨论高度相邻,因此不再单独创建 inline thread。
另有一项升级提醒:本次变更之前签发给 remote owner 的 credential 不包含新增的 goal.arm grant。目前 Remote Host 用户量几乎可以忽略,重新签发 credential 的成本也很低,因此该项的实际影响可以忽略,不作为本 PR 的阻塞理由。
当前 head 的所有 checks 均为绿色,但 restart、首个 Turn 预算、混合版本、无效待输入以及 dispatched interruption 这些路径没有被现有测试覆盖。
66def06 to
66f5d39
Compare
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: 929de8b0-f211-4644-8b7c-4f76ac2fa215
📒 Files selected for processing (20)
apps/desktop/e2e/goal-dialog-budget.spec.tsapps/desktop/src/main/__tests__/session-mode-ipc-main.test.tsapps/desktop/src/main/runtime-host-session-catalog-ipc-main.tsapps/desktop/src/main/runtime-host-session-domains-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/goal-dialog.tsxapps/desktop/src/renderer/locales/shell-copy.tsapps/desktop/src/renderer/use-session-goal.tsapps/desktop/src/shared/goal-arm.tsdocs/astryx-surface-file-inventory.mdpackages/core/src/goal.tspackages/runtime-host/src/__tests__/goal-coordinator.test.tspackages/runtime-host/src/protocol/index.tspackages/runtime-host/src/server/goal-coordinator.tspackages/runtime/src/__tests__/goal-state.test.tspackages/runtime/src/__tests__/goal-tools.test.tspackages/runtime/src/goal-state.tspackages/runtime/src/goal-tools.tspackages/ui/src/__tests__/composer-plus-menu.test.tsx
Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.
|
Thanks for the careful pass — the restart and budget findings were both real, and chasing them turned up more than the diff showed. All five are answered in their own threads: 1, 3 and 4 are fixed; for 2 I took the finding but not the remedy, since the baseline parameter turned out to be inert on both paths, so it is removed and the semantics stated; 5 is deferred to #3234, where it belongs as a contract shared by every control operation. Two of your suggested implementations I did not take, with reasons in the threads. Would appreciate another look when you have time. AI disclosure: this round's code, tests and thread replies were produced with Claude Code. I reviewed the final diff and own the result; each fix was verified by first making its test fail for its own reason. 感谢细致 review——重启和预算这两条都成立,而且顺着查下去发现的比 diff 上能看到的更多。 五条都已在各自 thread 中回复:1、3、4 已修;第 2 条我接受问题但未采用该修法——那个 baseline 参数在两条路径上都从未生效,因此删除并明确语义;第 5 条推迟到 #3234,它本就属于所有 control 操作共享的契约。另有两处你建议的实现我没有采纳,理由写在对应 thread 里。 方便时麻烦再看一轮。 AI 披露:本轮代码、测试与 thread 回复由 Claude Code 产出。我审阅了最终 diff 并对结果负责;每处修复都先让其测试因它自己的原因失败来验证。 |
M4n5ter
left a comment
There was a problem hiding this comment.
English
Approved on head 66f5d39 for the Goal-specific delta stacked after c759107 (#3198).
I reverified all five previous findings. The restart behavior, invalid budget input, and compatibility epoch are fixed on their real paths. Removing the inert creation-time token baseline correctly exposes the existing Goal budget semantics. The dispatched-interruption ambiguity remains real but is narrow, recoverable through the canonical Goal projection, and tracked across control operations in #3234; it is a non-blocking follow-up here.
No new P0–P2 issue was found. The automated composite-Session-key concern is a false positive: preload resolves the Desktop key to its Host scope and Host-local Session id before invoking goal:arm, then projects the returned identifiers back to Desktop keys. All checks on this head are green.
Scope caveat: this PR is explicitly stacked on #3198. This approval assumes #3198 lands first with its outstanding mode-pair authority fix, followed by the documented rebase; it does not supersede that review.
简体中文
批准 head 66f5d39 中叠加在 c759107(#3198)之后的 Goal 专属增量。
我重新验证了上一轮的五个问题。重启行为、无效预算输入和 compatibility epoch 均已在真实路径上修复。删除创建时无效的 token baseline,正确显式化了既有 Goal budget 语义。Dispatch 后中断造成的结果不确定仍然真实存在,但触发窗口较窄,可以通过 canonical Goal projection 恢复确认,并已在 #3234 中按所有 control operation 的共用问题跟踪;这里降为非阻塞 follow-up。
未发现新的 P0–P2。自动审查提出的 composite Session key 问题是误报:preload 会先把 Desktop key 解析为对应 Host scope 和 Host-local Session id,再调用 goal:arm,最后才把返回标识重新投影为 Desktop key。当前 head 的所有 checks 均为绿色。
范围说明:本 PR 明确叠加在 #3198 之上。本 approval 假设 #3198 会先带着仍待修复的 mode-pair authority 问题合并,随后按说明 rebase;它不会覆盖 #3198 的 review 结论。
d3c680f to
77a5c1f
Compare
1defc12 to
360b23e
Compare
|
Heads-up on a cross-PR collision — not a review comment on your change.
The trap is that this does not conflict. All three branches write the same text to that line, so git's three-way merge takes it silently; only the adjacent comment block conflicts, and keeping both comments is the natural resolution. Each PR's own Please re-check against (Posted with Claude Code (Opus 5) assistance; the epoch values were read from each branch head.) |
A Goal could only be armed by the model, from inside a Turn, with the GoalSet tool. There was no Host operation for it, no bridge method, and no control anywhere in the product — the user could stop a Goal but never start one. Add `goal.arm` as a real Host operation, next to `goal.query` and `goal.control`, and carry it out to the + menu: - `HostGoalCoordinator#arm` creates the Goal through the same GoalManager the tool uses, under the same Session admission gate, with the same durable record. It schedules nothing: a Goal armed outside a Turn takes hold on the next one, which `beginObservedTurn` binds to the live control lease. A Session that already has an unfinished Goal gets `operation_conflict`. - The two budget ceilings and the token floor move to `@maka/core/goal`, so the protocol codec and the GoalSet tool schema validate against one number instead of two copies of it. - Remote owners are granted `goal.arm` at the same tier as `goal.control`: withholding it would withhold nothing, since a remote owner sends Turns and the model arms its own Goal inside one. The only thing a refusal removes is the explicit path the user can see and stop. - The IPC handler takes the Session from the scoped channel, never from the renderer's frame, and normalizes the budgets without clamping them — an out-of-range value is refused once, by the Host. - The + menu gains one "设定 Goal…" row in its action group, at the same 28px rhythm as the rest, and app-shell opens a dialog that collects the condition and the two budgets. The row explains itself when a Goal is already running or a Turn is in flight. Generated-by: Claude Code
The GoalSet tool trimmed the condition in its own schema, so `goal.arm` persisted surrounding whitespace that a model-created Goal never could. Two callers each normalizing in their own way is the defect, so the trim moves to `GoalManager.create` — the one place every caller reaches and the only place the condition is stored. Generated-by: Claude Code
Three places on the arm path restated what the protocol already owns: `armGoal` declared a structural copy of the operation input, the dialog carried the token-budget floor as a literal beside the constant this change introduces for it, and the IPC validator rebuilt the frame from recognized fields, silently dropping anything else. The validator now refuses any key the frame does not carry. That is only safe if both sides read one declaration of what it carries, so the shape lives in `shared/goal-arm.ts`, which preload types its parameter with and the validator derives its key check from. The Session is not in it: it travels as the scoped IPC argument, so preload passes it separately rather than restating it in the payload, and a renderer-side Session id is now a compile error instead of a rejected frame at runtime. Generated-by: Claude Code
`useSessionGoal` kept the old Session's running Goal until the new fetch resolved, so switching Sessions briefly showed the + menu's Goal entry as already armed for a Session that may have no Goal at all. The state now resets with the Session rather than with the response. Generated-by: Claude Code
`goal.arm` is a new wire operation, so a client that has it and a Host that does not are no longer safely interoperable. Without a new epoch the pair completes the handshake, the client offers Set Goal, and the first arm reaches an older decoder as an unknown operation — a protocol error that tears the connection down instead of refusing the pair up front and routing the user to the Host upgrade path. The epoch is the boundary the repository already uses for this: apache#3145 advanced it for a new operation and apache#3165 advanced it merely for removing catalog filters. Handshake fixtures name deliberately stale epochs to exercise rejection, so they stay as they are. Generated-by: Claude Code
…r recovery `goal.arm` persists an active Goal whose contract is that arming alone starts nothing: the Goal takes hold when `beginObservedTurn` binds the user's next Turn. The durable record it writes has no current execution, which is also what a Goal sitting between continuations looks like, so a Host restart handed it to `recoverActiveGoal`, which built a continuation intent and scheduled it. A user who armed a Goal and quit the app came back to it spending tokens on its own. `status` cannot separate those two, but `iterations` already can: only a Turn that settled while bound to the Goal raises it, so a Goal at zero has never been carried by one. Recovery reads that instead, under the name `hasCarriedTurn`, and leaves an uncarried Goal exactly where the restart found it. A current execution is its own proof of carrying, so that branch is unchanged. This also covers a Goal that `GoalSet` created inside a Turn the Host never lived to settle. That Turn is gone after the restart, so waiting for the user's next one is the same answer, and it is the safe direction: the Goal stays armed and visible instead of starting unattended. Generated-by: Claude Code
`GoalManager.create` took a `tokensAtStart`, and both callers computed one: `GoalSet` read the token count from its deps and `goal.arm` read the Host's continuation cache. Neither ever reached a decision. `create` wrote the value into `tokensAtStart` and `tokensNow` together, so the spend it implies is zero whatever it holds, and the first settled Turn overwrote both from `tokensBaselinePending` before any budget was compared. The parameter has been inert since it was introduced. Removing it settles what the token budget measures, which a review of the new arm path asked for: the budget starts at the first Turn carrying the Goal and covers everything the Goal drives after it. That Turn is one the user is present for — `GoalSet` runs inside it, and an armed Goal takes hold on the one the user sends next — while the budget exists to bound what runs unattended. Naming an earlier baseline would need a token count read at the moment a Turn binds the Goal, and there is no such read point today; arming cannot substitute its own, because an unrelated Turn may be in flight and its spend would be charged to a Goal that never caused it. `#tokenCache` stays. It is what a settling Turn reports, not a baseline. Generated-by: Claude Code
The two budget fields were Astryx `NumberInput`s carrying the Host's bounds. That component only reports a value its own parse accepts and keeps anything else to itself until blur, when it silently reverts — and the dialog's submit guard read the condition alone. So a user who typed a 500-token budget armed a Goal with `null`, which is not a smaller ceiling but no ceiling at all, and one who typed 250 turns armed the Host's default instead. Narrowing a run's budget was the one gesture that could widen it. The fields now hold the user's own text and the dialog reads it, so what it arms is what the form shows and an entry the Host would refuse blocks Start with the reason beside the field. `readGoalBudget` repeats the operation's bounds to say that locally rather than as a rejected frame; the numbers still come from the shared Goal limits, and the copy takes them as arguments so no sentence carries its own copy of a limit. Verified end to end: the new e2e spec fails on the previous dialog with Start still enabled under an invalid budget, and reads the armed Goal back from the Host rather than watching the bridge call. Generated-by: Claude Code
A Goal is born into one of two situations. The model sets one from inside a Turn, so that Turn is already bound to it and settles into the Goal's first continuation -- the loop is running from the moment the Goal exists. `goal.arm` creates one from outside every Turn and deliberately starts nothing: it waits for a Turn to take hold of it. Recovery has to tell those apart, and until now it inferred the answer from `iterations === 0`. That reads as "no Turn has carried it", which is true of both: an armed Goal, and a model-set Goal whose birth Turn has yet to settle. So the inference was right for the case it was written for and wrong for the other one -- and `resumeFromControl` never consulted it at all, which is how a resumed Goal could be given a continuation the restart afterwards refused to put back. One question, two sites, two different answers. The distinction is now on the Goal. `armedAt` is stamped once by `goal.arm`, never cleared, and says how the Goal began rather than where it is now; `isDrivingGoal` combines it with `iterations` to answer the only question either site actually asks -- is this Goal in a loop that should be put back. An armed Goal is not, until a Turn settles while bound to it. The rule itself moves to the one place both callers pass through. `restoreDrive` is what recovery and resume now share, so the coordinator no longer keeps a copy of the predicate, a third restoration door would inherit it, and `hasCarriedTurn` -- a name for the proxy, not for the question -- is gone. Generated-by: Claude Code
Session admission is a queue, and the composition begins to drain without waiting for it to empty. `beginDrain` clears the Goal manager immediately, so an operation that was admitted before the drain and is still queued behind another one runs after that: it wakes up holding a lease on a composition that is already shutting down. Every other Goal operation survives that honestly. They read a manager that no longer holds anything and answer `not_found`, which is what a drained Host has to say. `goal.arm` is the one operation that answers by creating something -- it repopulated the map its owner had just emptied, flushed the new Goal to durable storage, and reported success -- so the Goal outlived the composition that was told to stop. It now rechecks the drain inside the admitted callback, before any read or mutation, and returns `host_draining` -- already one of the operation's declared errors, and the same recheck-after-the-wait that `message-coordinator` does at its own admitted entry points. `GoalManager.dispose()` also becomes a disposal rather than a `clear()` that leaves the object usable. Creating a Goal on a disposed manager is not a smaller version of creating one, it is a mistake about lifecycle, so it throws instead of quietly succeeding. The recheck above means no caller reaches it; the throw is there so the next entry point that forgets fails loudly rather than persisting a Goal nobody can stop. Generated-by: Claude Code
…tests apache#3249 removed `backend` from `CreateSessionInput`, so the four Session fixtures this branch adds no longer compile against `main`. The field carried no choice for a live build, and the store stamps every new header itself, so the fixtures need nothing in its place. Generated-by: Claude Code
2902ecc to
fa0dc52
Compare
|
Rebased onto current Two adaptations: #3332 took epoch 28, so Verified on the rebased tree: @hqhq1025 both findings are answered in the threads above — re-review appreciated. @M4n5ter @jackwener @likun666661 a second pair of eyes is very welcome too, whenever you have time. (Rebase and local verification run with Claude Code (Opus 5).) |
hqhq1025
left a comment
There was a problem hiding this comment.
[P1] The existing Resume-intent finding remains unresolved on this head.
At packages/runtime/src/goal-continuation.ts:421, resumeFromControl() returns an active Goal but restoreDrive() refuses to schedule it when armedAt exists and iterations === 0. This is a normal product path: arm a Goal, pause it before the first carried Turn, then press Resume. The UI promises that resuming continues autonomous iteration immediately and renders the Goal as running, but no continuation runs, and a restart preserves the inert state. The new regression at packages/runtime-host/src/__tests__/goal-coordinator.test.ts:743 asserts this mismatch.
Please persist explicit current drive/resume intent, or an equivalently simple phase, and use it for both Resume and recovery. The regression should cover pause -> Resume while admission is busy -> restart before currentExecution is recorded.
I did not open a duplicate inline thread because this is the existing unresolved hqhq1025 finding. I reviewed the complete revision; the required check and 73 focused local tests pass, but this P1 keeps this exact head from merge readiness.
Codex-assisted review performed under the maintainer-approved review workflow.
Resuming an armed Goal did nothing. `restoreDrive` refused any Goal with `armedAt` and `iterations: 0`, and the control that sends the resume promises the opposite — "Resuming continues autonomous iteration immediately". The Goal came back `active`, the chip showed it running, and nothing ran until the user typed again. `iterations > 0` was standing in for "a Turn has taken hold of this Goal", and it is not that fact. Iterations only rise when a carried Turn settles into an evaluation, so the whole of a Goal's first Turn reads as untouched — and a pause taken during that Turn discards the settlement, leaving a Goal that was genuinely driving permanently indistinguishable from one nothing ever carried. So record the fact instead of inferring it. `armedAt` now means the Goal is waiting to be driven rather than how it was born, and the two events that end that wait clear it: a carried Turn settling into a continuation, and the user resuming. `isDrivingGoal` reads that one field. Both writes ride commits the Goal already makes, so no revision moves that did not move before, and absence still means driving — which is what every Goal persisted before arming existed is. The regression covers the case that has nothing else to fall back on: arm, pause, resume into a busy admission, then restart before any execution is recorded. Only the Goal itself can say the user asked for continuation. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current revision, including the fix for the prior resume/restart finding. The durable armedAt transition now distinguishes an armed Goal that is still waiting from one whose first settlement or explicit resume made it autonomous, and the recovery regression covers busy admission across restart. The repository build and 96 focused Goal tests passed, and the required CI check is green.
Codex-assisted review performed under the maintainer-approved review workflow.
Summary
A Goal could only be armed by the model, from inside a Turn, with the GoalSet tool. There was no Host operation for it, no bridge method, and no control anywhere in the product — the user could stop a Goal but never start one.
This adds
goal.armas a real Host operation, next togoal.queryandgoal.control, and carries it out to the + menu:HostGoalCoordinator#armcreates the Goal through the sameGoalManagerthe tool uses, under the same Session admission gate, with the same durable record. It schedules nothing: a Goal armed outside a Turn takes hold on the next one, whichbeginObservedTurnbinds to the live control lease. A Session that already has an unfinished Goal getsoperation_conflict.@maka/core/goal, so the protocol codec and the GoalSet tool schema validate against one number instead of two copies of it.goal.arm, at the same tier asgoal.control. Withholding it would withhold nothing: a remote owner sends Turns, and the model arms its own Goal inside one. The only thing a refusal removes is the explicit path the user can see and stop.What it looks like
Shot live in Storybook, same viewport, each surface cropped to its own bounds and composited at 1:1 pixel scale. (The mode rows below the divider are #3198's doing, not this PR's.)
Left: 设定 Goal… joins 添加文件或目录 and 选择技能 in the action group — the same 28px row, no new component kind in the panel. Middle: arming refuses a second Goal, so the row says why instead of opening a dialog that would fail on submit; it is the same mechanism the Skills row already uses for an empty catalog, and the only row that grows past 28px is the one carrying an explanation. Right: the two budgets that stop a Goal are visible while it is being armed, the condition counter is the shared
GOAL_CONDITION_TEXT_LIMIT, and 开始 stays disabled until there is a condition.Verification
packages/runtime-host:goal-protocol.test.ts(+3 tests — exact frame, budget bounds, remote-owner grant) andgoal-coordinator.test.ts(+1 — arm, conflict, not-found, archived) pass.apps/desktop:runtime-host-client-operations.test.ts(+1 — one request, no retry on conflict) andruntime-host-session-domains-ipc-main.test.ts(+1 — scoped Session wins over the frame's, budget normalization) pass.packages/runtimegoal-tools.test.ts/goal-state.test.tsandpackages/coregoal-authority.test.tspass unchanged, confirming the constant move is behaviour-preserving.@maka/core,@maka/runtime,@maka/runtime-host,@maka/ui, and all fourapps/desktoptsconfig projects.npm run format:checkandbiome lintclean.GoalDialogOpenandGoalAlreadySetadded. Measured live — the seven + rows stay 28px, and the disabled Goal row grows to 48px only because it carries its explanation, the same way the Skills row already does with an empty catalog.AI use
Tool(s) and scope: Claude Code (Opus) wrote the diff, the tests, and the commit message. The decision to add a real Host operation rather than route the user through the model is the human contributor's. The commit carries a
Generated-by: Claude Codetrailer.Checklist
Does this PR entail a change in behavior?
Review focus
Runtime Host is the sole execution authority, and this adds an operation to it. Two things deserve the attention:
#armschedules nothing. A Goal armed while a Turn is in flight does not drive that Turn — it registered before the Goal existed, andactivateGoaldeclines exactly that case (goal_not_observed). The UI disables the entry during a Turn so this is never a silent no-op, but the Host accepts it either way; the Goal simply starts from the next Turn.REMOTE_OWNER_OPERATION_GRANTSis fail-closed by design, and this PR opens one entry in it. The reasoning is in the test that asserts it; if you disagree, the grant is the one line to revert.