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 5 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"的假设。我没有验证这个假设是否成立;如果成立,现在这样写没有问题。
Summary
Fixes #3362
Verification
npx biome checkon changed files passed after rebasing ontomain.npm --workspace @maka/desktop run typecheck, the affected Runtime Host protocol tests, Desktop IPC tests, and the renderer timing regression test passed.A fresh full workspace build in the isolated rebase worktree could not complete because the environment blocks the lockfile's remote
@xterm/xtermtarball. The PR has no failing CI checks at present.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?