fix(desktop): show armed goals waiting for their first Turn - #3368
fix(desktop): show armed goals waiting for their first Turn#3368CxHsin wants to merge 7 commits into
Conversation
a7196a3 to
c4e694d
Compare
|
Independent review of Before the findings, the state chain this was checked against, since the whole change is about telling two of these apart:
[P2] The CLI/TUI ignores the new armed projection entirely, so a Goal that may never start anything is announced as already runningif (currentGoal !== null &&
(currentGoal.status === 'active' || currentGoal.status === 'waiting')) {
state.entries.push({ kind: 'notice', level: 'info', text: goalAttachedNoticeText(currentGoal) });
}—
This misreport does not self-correct, and that is what makes it a P2 rather than a cosmetic gap: waiting indefinitely is the armed state's correct behavior. There is no eventual Turn that repairs the display. The CLI tests touched by this PR only add [P3][design] Desktop substitutes "some Turn is running in this Session" for "this Turn is bound to this Goal"
Repro (reachable whenever two clients share a Session, and [P3][design]
|
|
Fourth finding — [P3], posted here rather than inline because the file it lands in ( Only the StatusDot learned about the armed phase; the two buttons beside it still describe the opposite state. While a Goal is armed with no Turn yet, the dot correctly reads "takes hold on the next Turn", but the adjacent tooltips still say:
In this phase there is no continuation and nothing is burning. The user is told simultaneously that nothing has started and that stopping it will halt token spend — and the two claims sit inches apart. This is the same shape as the P2 on the CLI side, at a smaller scale: the phase distinction reached the state and one indicator, but not the other consumers of that state. Here it is one file instead of a whole client. Fix either way: give the armed phase its own action copy (clear a Goal that has only been set; pause so it does not take hold yet), or have the copy accept the phase rather than only Same blind line and same head ( |
|
Hi — this PR conflicts with current I tested a rebase onto current
These are real source conflicts, so they need your judgement rather than a mechanical rebase — please rebase onto current Thanks for the contribution — happy to help if any conflict is unclear. AI-assisted maintenance note, not a review. It does not count as the required human review under |
c4e694d to
3537323
Compare
ARE404
left a comment
There was a problem hiding this comment.
NO-GO at head 3537323722beda01f95d67f2bd209d075439a11a.
Gate: test ❌ on the exact head — Check ASF source headers fails (new use-session-goal.test.ts lacks the license header; [P1], reproduced locally). This aborts the job before Build/tests. Epoch guard ✓ (against the branch's older base). audit not shown (no pkg.json/lockfile change → correctly not triggered).
Findings: 1 × [P1] (missing header, self-caused — inline @ use-session-goal.test.ts:1), 1 × [P2] (protocol epoch collision post-rebase — inline @ protocol/index.ts:94), 1 × [P3] (cross-surface predicate divergence — inline @ pi-goal.ts:186).
On the two review focuses you called out:
- Red attribution: this is the PR's OWN fault (missing header), NOT the stale-branch
goal-services-adapter.test.ts/SessionChangedEvent.typeTS2353. I explicitly checked — thetestjob here fails at the source-header step (before that TS build surface), the goal-services TS2353 is not the trigger on this head. The header gap is introduced by this PR's added test file. - State-machine enter/exit symmetry for the new armed-waiting state: SOUND — I traced every exit path of
isGoalArmedAwaitingFirstTurn(armedAt!==undefined && boundTurnId===undefined):- Enter:
goal.armsetsarmedAt(goal-state.ts:248), no boundTurn yet → chip shows armed-waiting. - Exit via running first Turn:
startObservedTurn/beginbind the turn →observedGoalTurnIdreturns it →boundTurnIdset → chip flips to running-first-turn. - Exit via settlement/pause/clear/resume/terminal:
settleTurncontinuation branch and status transitions all cleararmedAt(goal-state.ts 331/334/395/413/429/447), so the chip cannot stay stuck showing armed-waiting. Crash recovery correctly treats an un-settled armed Goal as still-waiting (no dead state). - No asymmetric enter-without-exit path found; both durable (armedAt) and transient (boundTurnId) markers clear on the correct triggers.
- Enter:
- Cross-process contract sync: consistent.
armedAt/boundTurnIdare added toGoalProjection(host), plumbed throughtoDesktopGoal(main) →preload.goal.get→bridge-contract.d.ts→ rendereruse-session-goal.ts+app-shell.tsx. All handshake/decode sides updated; epoch comment acknowledges it must be a closed wire change. Old-frontend-on-new-contract is handled by the epoch handshake (see [P2]: must sit above main's 44).
No APPROVE (read scope); flagging to orchestrator for a MEMBER approve once the [P1] header + rebase/[P2] epoch-45 are addressed and checks go green. Not merging.
| @@ -0,0 +1,12 @@ | |||
| import assert from 'node:assert/strict'; | |||
There was a problem hiding this comment.
[P1] This new file is missing the Apache ASF license header. The test job's Check ASF source headers step fails on this exact head — I reproduced it locally: node scripts/asf-license-headers.mjs check → "1 file(s) are missing the ASF license header: .../use-session-goal.test.ts". The job aborts here, before Build/typecheck/tests, so this is the current blocking red. Fix with npm run write:asf-headers (adds the license block) and re-push. This is distinct from the earlier goal-services-adapter.test.ts SessionChangedEvent.type TS2353 you may have seen elsewhere — this PR itself introduces this header gap.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 42 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 44 as const; |
There was a problem hiding this comment.
[P2] After this branch is rebased onto current main (it's already DIRTY/conflicting), the protocol epoch guard will flag this. Current origin/main already has RUNTIME_HOST_COMPATIBILITY_EPOCH = 44 (same value as this branch), and the guard rejects "same-number" epoch bumps that would merge without a git conflict — the exact projection-schema change here (goal.ts adds armedAt + boundTurnId to GoalProjection) is a closed wire change, so it must land with an epoch the current base has NOT seen. Please rebase and set the epoch to 45+ (and keep the 43/44 changelog entries, adding 45). Otherwise the test job will fail on the epoch guard post-rebase even though it's green against today's older base. Both handshake sides + decode are otherwise consistent.
There was a problem hiding this comment.
One refinement on the number, from the orchestration side — the epoch analysis above is right, but please do not write down a specific value.
45 is a single slot and there are already several PRs queued for it (#3316, #3376, #3573, #3635, and now this one). Whichever lands first takes it, and every other branch that hard-coded 45 goes red again on the same guard, for the same reason, one merge later.
The durable instruction is a rule rather than a number: rebase onto current main, read RUNTIME_HOST_COMPATIBILITY_EPOCH there, and use that value + 1 — then add the corresponding changelog entry for whatever number that turns out to be. That stays correct no matter who merges ahead of you.
Everything else in the comment stands, including keeping the existing changelog entries and the point that this is a closed wire change.
| return lines; | ||
| } | ||
|
|
||
| function isArmedGoal(goal: Pick<GoalProjection, 'status' | 'armedAt'>): boolean { |
There was a problem hiding this comment.
[P3] Minor cross-surface predicate divergence on the same "armed" concept: this CLI predicate (status==='active' && armedAt!==null) and the shared @maka/ui chip (session-context-layer.tsx: status==='active' && armedAt!==undefined) do NOT consider boundTurnId, while the desktop uses isGoalArmedAwaitingFirstTurn (armedAt!==undefined && boundTurnId===undefined). So in the window where an armed Goal's first Turn is already bound and running (armedAt still set, not yet settled), desktop renders it as the first-turn "running" state, but the CLI/@maka/ui would still say "set / takes hold on the next Turn". It self-corrects when the Turn settles and clears armedAt, so it's cosmetic drift today (desktop is the only consumer feeding armedAt to the ui chip), but the three predicates drifting is exactly the kind of thing #3393-style convergence was meant to avoid. Consider routing CLI/ui through the same boundTurnId-aware predicate, or documenting why they intentionally differ. Non-blocking.
3537323 to
c3f4cc3
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
#3368 c3f4cc3 — review (bind exact head)
Gate: CI run 32680590190 completed/failure (test job failure at affected standard workspace tests). Lint/typecheck/build success, guard success. No GO while red — author should triage whether failure is PR-introduced.
Verdict: COMMENT only (no GO due to red gate)
Q1 armed-without-Turn persistence: core/goal.ts and runtime-host goal-projection carry armedAt+boundTurnId through projection/IPC (protocol epoch 45, desktop-goal-state). Armed is legitimate persistent state until first Turn binds (isGoalArmedAwaitingFirstTurn). Not a transient artifact — duration is user-visible until next Turn.
Q2 overlap: session-context-layer.tsx phase derived as paused > armed > waiting > running with isGoalArmedAwaitingFirstTurn check before waiting, and explicit armed phase suppresses elapsed. No mutual hit — LiveGoal type now via DesktopGoalState includes armedAt fields.
Q3 condition sanitization: condition text flows via goal-state → projection → desktop-goal-state → UI. Prior P2 was agent-created Goal condition bypassing redaction. This PR does not change condition redaction path, only exposes armed phase. No new unsanitized display path observed (sampling 33 files, conversation-copy/session-context-layer/pi-goal). Full sanitization confirmation requires tracing core goal sanitizer — mark as sampled.
Epoch 45 strictly greater than base (44) — correct direction; if contend, rebase rule is strictly greater.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up to the review above, replacing one sentence in it.
That review asked the author to triage whether the red test job was introduced by this PR. That was under-specified — the run log answers it, and the answer is unambiguous.
Run 32680590190, step Run affected standard workspace tests, single failure:
an armed goal waits for its first Turn without looking like it is running
AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value
That is this PR's own test for this PR's headline behaviour, and it is the only failing test in the job. So this is neither an inherited breakage from the base nor unrelated flake: the change does not yet do what its own test asserts it does. Everything else in the job — lint, typecheck, build, and the protocol epoch guard — is green.
Nothing else in the review changes. The epoch direction is right: the head is strictly greater than the base, which is what scripts/protocol-epoch-check.mjs requires. If the number is contended by the time you rebase, the rule to apply is "read the value on current main and take one greater", not a fixed number.
Coverage note, stated plainly because the review above should have led with it: the redaction finding in Q3 was reached by sampling 33 files, not by tracing the core goal sanitizer end to end. Treat it as "nothing found in the sampled paths", not as a clean bill.
68f5f45 to
7625591
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
The core change holds up. Keeping armedAt until the first bound Turn ends, instead of clearing it the moment the goal is driven, is the right model for what the UI needs to show, and all four exit paths — settle, pause, clear, terminal — set armedAt: undefined with a test asserting each one. I checked the counterfactual: reverting goal-state.ts to the merge-base makes "leaving the armed phase clears its marker" fail, so the tests genuinely hold the new behaviour rather than passing incidentally. The projection decode stays a closed key set, the epoch gate is what keeps an old peer from seeing the two new keys, and the UI phase ordering matches the state machine.
Three things, none of them about the goal logic itself.
[P3] The secret redaction on goal conditions is not mentioned in the description
packages/cli/src/pi-goal.ts:128 now redacts inlineGoalText, and packages/ui/src/session-context-layer.tsx:121,376-384 plus apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx:112,119 redact the condition in the chip, the tooltip, the lineage and the reconciliation dialog. That is a user-visible behaviour change covering CLI notices and UI tooltip and aria text, and the description only claims the armed-state display and the epoch.
The change itself is in the safe direction and has tests holding it, so I am not asking you to remove it. But a reviewer checking the diff against the stated scope will skip past it, which is the failure mode worth avoiding. Please either say so in the description or split it out.
Rebase and epoch, before anything else can run
This branch is conflicting and has no check runs at all, so nothing here has been verified by CI yet. It declares RUNTIME_HOST_COMPATIBILITY_EPOCH = 45 while main is at 48, and the "30 → 32" wording in the description no longer matches the code.
Please do not simply take 49: #3299 and #3651 both already declare it, so it is contested and whoever merges first will take it. Re-derive the number at rebase time rather than trusting any figure written here:
git fetch origin main
git show origin/main:packages/runtime-host/src/protocol/index.ts | grep COMPATIBILITY_EPOCH
gh pr list --repo apache/maka --limit 200 --json number --jq '.[].number' | while read n; do
gh api repos/apache/maka/pulls/$n/files --paginate \
--jq '.[]|select(.filename=="packages/runtime-host/src/protocol/index.ts")|.patch' 2>/dev/null \
| grep -oP '^\+export const RUNTIME_HOST_COMPATIBILITY_EPOCH = \K\d+' | sed "s/^/#$n /"
done
The ^\+ matters — it counts only what a branch adds, rather than what it inherited from main at its last rebase. Match the assertion in protocol.test.ts to whatever you land on, one per generation asserting > previous. The Verification section will need rewriting after the rebase too.
Two observations, neither a finding
An armed goal persisted under the old semantics will show as "set, not started" after the upgrade until the next settle clears the marker, because boundTurnId is null for it. That is transient, display-only and self-healing, so I mention it only in case you would rather migrate it.
observedGoalTurnId takes the first registration whose control lease matches within the lane, which assumes one in-flight Turn per Session at a time. I did not verify that assumption holds; if it does, this is fine as written.
简体中文
核心改动是站得住的。把 armedAt 保留到第一个绑定 Turn 结束、而不是目标一被驱动就清掉,正是界面需要展示的那个模型;settle、pause、clear、terminal 四条退出路径都设置了 armedAt: undefined,并且每条都有对应断言。我做了反向验证:把 goal-state.ts 退回 merge-base 版本后,「离开 armed 阶段会清掉标记」这条测试真的会红,说明这些测试确实锁住了新行为,而不是碰巧通过。投影解码保持为闭合键集,epoch 门禁挡住旧 peer 看到两个新键,UI 的相态顺序与状态机一致。
三点,都不是目标逻辑本身的问题。
[P3] 对 goal condition 的密钥脱敏没有写进正文
packages/cli/src/pi-goal.ts:128 现在会对 inlineGoalText 脱敏,packages/ui/src/session-context-layer.tsx:121,376-384 以及 apps/desktop/src/renderer/features/goals/ui/goal-dialog.tsx:112,119 也对 chip、tooltip、lineage 和 reconciliation 对话框里的 condition 做了脱敏。这是一项用户可见的行为变更,覆盖 CLI 提示与 UI 的 tooltip、aria 文案,而 PR 正文只声明了 armed 状态展示与 epoch。
改动本身方向是安全的,也有测试锁住,所以我不要求你去掉它。但一个照着"声明范围"核对 diff 的审查者会直接略过这块——这才是需要避免的失败模式。请在正文中说明,或者把它拆成独立 PR。
rebase 与 epoch,在其它一切之前
这个分支处于冲突状态,而且一条 check 记录都没有,也就是说这里的任何东西都还没有被 CI 验证过。它声明 RUNTIME_HOST_COMPATIBILITY_EPOCH = 45,而 main 已经是 48;正文里"30 → 32"的说法也已经和代码对不上。
请不要直接取 49:#3299 与 #3651 都已经声明了它,属于争用状态,先合并的那个会拿走。请在 rebase 时重新推导,而不要相信这里写下的任何一个数字:
git fetch origin main
git show origin/main:packages/runtime-host/src/protocol/index.ts | grep COMPATIBILITY_EPOCH
gh pr list --repo apache/maka --limit 200 --json number --jq '.[].number' | while read n; do
gh api repos/apache/maka/pulls/$n/files --paginate \
--jq '.[]|select(.filename=="packages/runtime-host/src/protocol/index.ts")|.patch' 2>/dev/null \
| grep -oP '^\+export const RUNTIME_HOST_COMPATIBILITY_EPOCH = \K\d+' | sed "s/^/#$n /"
done
其中的 ^\+ 很关键——它只统计分支自己新增的那一行,而不是它上次 rebase 时从 main 继承来的值。选定之后请同步 protocol.test.ts 里的断言,惯例是每个世代一条、断言 > 上一代。rebase 之后 Verification 段也需要整体重写。
两条观察,都不算 finding
在旧语义下持久化的 armed 目标,升级后会因为 boundTurnId 为空而显示成"已设置、未开始",直到下一次 settle 清掉标记为止。这是瞬态的、纯展示的、会自愈,所以只是提一句,以防你更愿意为它做一次迁移。
observedGoalTurnId 取的是 lane 内第一个 control lease 匹配的注册,这隐含了"同一 Session 同时只有一个在途 Turn"的假设。我没有验证这个假设是否成立;如果成立,现在这样写没有问题。
Generated-by: Codex
Generated-by: Codex
c40633b to
b967991
Compare
Reuse the existing goal-arm shared contract for the Desktop-only goal state so the renderer architecture ledger does not grow a new legacy closure edge. Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Re-review at 0e9ece9a. The core model is settled and I am not reopening it: keeping armedAt durable and pairing it with a transient boundTurnId is the right answer, because clearing armedAt at beginObservedTurn instead would make a Host that restarts mid Turn treat the Goal as driving and let recoverActiveGoal start a loop for it. packages/core/src/goal.ts:64-74 states that trade-off, all four exit paths clear the marker with an assertion each, and the projection key set stays closed. Moving DesktopGoalState into the existing apps/desktop/src/shared/goal-arm.ts rather than a new file was the right call.
Prior round, one by one:
- ASF header on
use-session-goal.test.ts: resolved, the file left the PR andtestis green. - Epoch rule (rebase onto main, read the value, take one greater): applied on 2026-09-03 against a base at 105, but stale again. See P1.
- Cross-surface predicate divergence: resolved. CLI,
@maka/uiand desktop all callisGoalArmedAwaitingFirstTurnnow. - Redaction not in the description: resolved, both the PR body and the CHANGELOG entry state it.
Still true from last time, still not findings: an armed Goal persisted under the old semantics reads as "set" until the next settle, and observedGoalTurnId returns the first lease match in the lane, which is fine because the UI only reads presence, not identity.
One cross-cutting note on verification: the green test run (33760740417) is bound to this head but to the 2026-09-03 base, not to current main. It does not predict the post-rebase result, and the epoch guard runs on the merge result.
Findings: 1 x P1 (epoch, inline), 1 x P2 (TUI attach notice, inline), 1 x P3 (dead UI prop, inline). git merge-tree against current main reports exactly one conflicting file, the epoch file, so the rebase itself is small.
| // Increment when the same protocol version no longer guarantees safe Client-Host | ||
| // interoperability. Mismatches are rejected before domain commands are admitted. | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 105 as const; | ||
| export const RUNTIME_HOST_COMPATIBILITY_EPOCH = 107 as const; |
There was a problem hiding this comment.
P1: 107 is now taken. origin/main declares RUNTIME_HOST_COMPATIBILITY_EPOCH = 109, and its 107: comment already belongs to the token_usage anchor generation. evaluateEpochCheck in scripts/protocol-epoch-check.mjs compares the merge result against the base parent and fails with "went backward" whenever head < base, so this lands red the moment it is rebased or the merge result is computed. This file is also the only conflict git merge-tree origin/main 0e9ece9a reports, so the rebase is otherwise clean.
Smallest fix: rebase, read the value on current main and take one greater (110 today), and place the new comment line above the 109 entry rather than at 107. Then update two stale copies of the number: the CHANGELOG.md entry that says "moves to 107", and the comment in packages/runtime-host/src/__tests__/protocol.test.ts explaining that 106 is claimed so this generation is "deliberately 107". The > 106 assertion itself can stay.
| return `Autonomous goal is running (${goal.iterations}/${goal.maxIterations}): ${short} — /goal shows details, /goal pause pauses it.`; | ||
| } | ||
|
|
||
| export function shouldAnnounceGoalAttachment( |
There was a problem hiding this comment.
P2: this predicate is applied at two of the three TUI attach sites, and it removes the notice from the case that most needs it.
pi-tui-runner.ts:601 and pi-tui-runner.ts:1697 now gate on shouldAnnounceGoalAttachment, so an armed Goal produces no notice at all. pi-tui-runner.ts:1749, the switchSession adoption path, still uses the old status === 'active' || status === 'waiting' test and does announce it. Same fact, two behaviours, and the armed wording you added at line 157 is only reachable through the one site you did not change.
The suppressed sites are the ones whose comment says attaching must never resume a token burning loop silently. An armed Goal takes hold on the user's very next Turn, so suppressing the notice deletes exactly that warning. The status line still shows goal set 3/50, which is why this is P2 and not P1.
Smallest fix: drop shouldAnnounceGoalAttachment, announce unconditionally at all three sites, and let goalAttachedNoticeText pick the wording, which it already does. That is one concept fewer, all three sites agree, and the armed text stops being nearly unreachable.
| /** Present while a user-armed Goal waits for its first Turn. */ | ||
| armedAt?: number; | ||
| /** Present while the first Turn carrying an armed Goal is running. */ | ||
| boundTurnId?: string | null; |
There was a problem hiding this comment.
P3: nothing in production sets this prop. git grep boundTurnId across packages/ui and apps/desktop/src/renderer hits only this declaration and one line in session-context-layer-goal.test.tsx.
The reason is that use-goal-controller.ts:148 already calls isGoalArmedAwaitingFirstTurn and forwards armedAt only when that is true, so by the time the chip re-derives the same predicate at line 126, boundTurnId is always absent and the branch is dead. One fact, two derivation sites, and armedAt quietly means "upstream already decided this is armed" rather than what its own doc comment says.
Smallest fix, either direction: forward armedAt and boundTurnId unconditionally from the controller and let the chip be the single deriver, or drop this prop and the second isGoalArmedAwaitingFirstTurn call and pass the boolean the controller already computed.
Summary
Fixes #3362
Compatibility
apache/main.107, avoiding collisions with compatibility changes already queued in adjacent work.Verification
Passed:
build:test.git diff --check.Known environment or baseline issues:
npm run build:testand Desktop typecheck are still blocked by existing UI type errors involvingsettledText,autoScroll, andtrailingAction; those files are outside this PR's changes.EBUSYfile locks).AI use
Tool(s) and scope: Codex implemented the protocol, Desktop, UI, and test changes. The commit retains the required
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?