Skip to content

fix(runtime): accept sequence observations and share bound execution - #5475

Merged
Astro-Han merged 7 commits into
apache:mainfrom
testikun:codex/cu-observation-lease
Sep 27, 2026
Merged

Astro-Han merged 7 commits into
apache:mainfrom
testikun:codex/cu-observation-lease

Conversation

@testikun

@testikun testikun commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #5474. After element_sequence returned a fresh observation, its next bound action was rejected as reobserve_required: the frame was registered but the session was not activated. Accept all six fresh-observation paths with a pre-capture lease check and synchronous frame/session activation. This also declines captures invalidated while in flight.

A separate second commit shares the existing bound-action execution protocol between a single semantic action and each sequence step: recheck the action lease after presentation, preserve partial delivery as outcome_unknown, and apply the same frame retirement/confirmation rules. Sequence still owns target lookup, progress, capture policy and response formatting; ordinary type/key is unchanged. Host/fake-backend timing checks do not establish real OS input delivery. Refs #4909; independent of open #5458.

Verification

  • Current head 95e45c7a4 merges main bc0786ee6 and addresses the review nit by checking the observe record locally at capture acceptance. Record creation still precedes observation leasing.
  • Repository test build, all workspace typechecks, lint, format and ASF header checks pass. All 381 Computer Use runtime and backend tests passed for the review fix. After syncing main, the full Runtime suite passes: 3,525 passed, 14 skipped, 0 failed.
  • The earlier hosted run at c31c7fbe5 failed the unchanged Code Mode 200 ms budget test under load; that file passes alone and the full Runtime suite passes on the synced head. GitHub denied a rerun because this account lacks repository admin rights. Fresh hosted CI passed on this head, including the full standard workspaces, Runtime Host and installed CLI validation. These controlled-backend checks do not establish real OS input delivery; no new real Desktop/model/OS sequence acceptance is claimed.
  • Prior ablations removed the presentation-time lease recheck and observation lease validation: they respectively dispatched a stopped second step and accepted invalidated captures. Both safeguards are retained.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex (OpenAI) authored the scoped runtime changes, regression tests, and this description; the contributor reviews and owns submission. Both commits carry Generated-by trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 18, 2026
@testikun

Copy link
Copy Markdown
Contributor Author

Status for maintainers: this PR is mergeable and has no review threads. The hosted Runtime Host step failed only in the unchanged runtime-resource-process PTY integrity test after its 60s close wait; the affected Computer Use build and focused tests passed. I tried gh run rerun 35312795350 --failed, but GitHub requires repository admin rights for this account. Please rerun that failed check and review the PR; no unrelated CI-only commit has been added.

Generated-by: OpenAI Codex
Comment thread packages/runtime/src/computer-use-tools.ts
@testikun

Copy link
Copy Markdown
Contributor Author

Synced current apache/main into head 74fdb22 on September 20, 2026. The merge was conflict-free and the effective PR diff remains limited to the two Computer Use runtime files. GitHub now reports MERGEABLE; fresh CI is running. Local full build was blocked by an unrelated current-main storage typing failure in read_usage, outside this PR diff. Please review the updated head.

@me2seeks

Copy link
Copy Markdown
Contributor

Review:推荐合并(Approve with nits)

我按对抗式复核逐调用点比对了 packages/runtime/src/computer-use-tools.ts 的改动,未发现高危正确性回归,重构在各调用点行为等价。这是一个质量较高的修复,推荐合并。

复核确认的点

  • blocked_url / user_stopped 区分没有丢:base 里 if (activated.status !== 'active') 那段其实是死代码——freshObservationSucceeded() 只在 !canObserve() 时不迁移,而前一行 validateObservationLease(...).ok 已要求 canObserve(),所以 activated.status 恒为 'active'。新代码用 state.beforeAction() 兜底,blockReason() 仍能把 blocked_url / user_stopped 正确映射出来。
  • observingRecord 提前计算是必要的:新回合 sessionObservation 会 reobserveRequired() 推进 generation;base 里租约在 bump 之前取、且 registerObservation 不校验租约,属于「碰巧能跑」。重排后配合 acceptObservation 的租约校验才是自洽的。
  • finish() 不会双调也不会泄漏:runWithPresentation 有 finished 幂等标志,executeBoundAction 每条分支恰好调一次。
  • 测试覆盖充分:新增 9 个回归用例(closing observation 可被下一步使用、in-flight 失效捕获被拒、部分投递保留为 outcome_unknown、异常释放 presentation 等),PR 说明里还做了 ablation 验证。

两个低危 nit(不阻塞合并)

  1. observingRecord! 非空断言偏脆弱:computer-use-tools.ts:2313 用 observingRecord!,其赋值在 :1667,中间隔了约 150 行控制流。当前确实安全(该行在 if (input.action === 'observe') 块内,且 sessionObservation 不会返回 undefined),但断言依赖远距离不变量。建议改成局部 const record = observingRecord! 加注释,或把 record 作为参数传下去。

  2. 缺少真实 OS 输入投递验证:PR 自述「host/fake-backend 时序检查不能证明真实 OS 输入投递」,且「未跑真实 Desktop/model/OS CU 复现」。这是已知证据缺口而非缺陷,合入前值得在真实环境跑一次序列操作。

关于 CI

本地 npm test 被中断、远端 Runtime Host 步骤在未改动的 runtime-resource-process PTY 用例上超时——这些都不在本次改动范围内,最新 head 的 build / typecheck / 受影响 workspace 测试已通过。建议由有权限的维护者 rerun 一次 Runtime Host 步骤后合并。

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

推荐合并。逐调用点复核未发现高危正确性回归,重构行为等价;仅两个低危 nit(observingRecord! 远距离非空断言、缺真实 OS 输入验证),不阻塞。详见上方 review 评论。

Astro-Han added a commit to Astro-Han/maka-agent that referenced this pull request Sep 21, 2026
…artup

Brings in named project creation and task moves (apache#5475) plus the eval
comparison doc (apache#3158). addProject keeps the bootstrap invoke gate while
gaining the name parameter.

Generated-by: Devin
Replace the distant non-null assertion with a local guard while preserving record creation before leasing.

Generated-by: OpenAI Codex
Preserve the Computer Use observation and bound-action fixes on the same baseline as the other open PR repairs.

Generated-by: OpenAI Codex

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P0-P3 findings on the current head.

This change makes accepted observations validate their lease before publishing a fresh frame, routes sequence and ordinary semantic actions through the same bound-execution lifecycle, and retires the previous Turn observation before leasing a new one. I independently exercised the production tool path: a sequence-closing observation can be reused immediately on this head, while the exact base returns reobserve_required; a capture invalidated by userStopped is rejected here, while the exact base incorrectly publishes it as fresh.

Clean install/build, the focused Computer Use tool suite (93/93), full Runtime suite (3511 passed / 13 skipped), full Computer Use suite (117/117), typecheck, lint, format, ASF headers, diff check, and the hosted test check passed. A clean synthetic merge against current main also passed build, Computer Use, typecheck, lint, format, ASF, and diff checks. Its sole Runtime failure reproduced identically on exact current main and is unrelated to this PR.

Residual scope: I did not exercise native macOS/Windows/Desktop input. A backend that ignores AbortSignal can continue work after cancellation, but that behavior is unchanged from the exact base.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at this head by three independent AI reviewers (different model families). No P0–P2 findings; one P3 note inline.

The root cause is right: the sequence's closing capture registered the new frame but never made the session active again, so the next action was refused, and several observation paths registered a frame without checking that the session lease was still valid. All fresh-observation paths now go through acceptObservation, which validates the lease before touching the record, and reports the lease's block reason (user_stopped, blocked_url, …) when it fails. element_sequence now takes a fresh observation and action lease per step, which is what lets a multi-step sequence continue after the generation bump of each transition.

executeBoundAction was compared line by line with the standalone action block it replaces and is equivalent (validation, presentation, partial delivery, retire vs. consume, refusal after dispatch). It shares code only, not state, so unrelated sessions are not coupled. Focused tool and session-state tests pass (102/102 in one reviewer's run) and CI is green.

Smaller notes, not blocking:

  • When every step succeeds but the closing capture is refused, the sequence headline still says failed and sets error.
  • Two of the new tests would also pass on the old code; they are useful guards but do not prove the fix.

Not verified: real macOS/Windows input, or a full Runtime run.


Automated review notice: This review was posted by an automated review agent operated by Astro-Han. It combines independent reviews from several AI models. It is not an independent human review and does not replace one.

}
result = preservePartialDelivery(presentation.result);
applyTypedOutcomeState(state, result.outcome);
if (result.outcome.ok) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 — If the session is invalidated while the action is in flight (for example the user presses Stop), an action that was actually delivered (outcome.ok) is returned as blocked, so a sequence step is now reported as failed. The standalone action path already behaved this way, so this makes sequences consistent with it rather than introducing a new rule; the model also has to re-observe before it can act again. Still, reporting a delivered step as failed could invite a retry of a non-idempotent action. Worth either carrying the delivered outcome alongside the block reason, or at least a test that pins the intended behaviour.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved at @Astro-Han's explicit request: a small, focused fix with no blocking findings in the automated review of this head and green CI.

@Astro-Han
Astro-Han merged commit a0ce9aa into apache:main Sep 27, 2026
1 check passed
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
…action

After element_sequence returned a fresh observation, the next bound action
was refused as reobserve_required: the closing capture registered the frame
but never activated the session. Other fresh-observation paths registered
frames without checking the session lease, so a capture invalidated in flight
could still reach the model. All of them now go through one acceptObservation
step that validates the lease before registering the frame and activating the
session, and reports the lease's block reason when it fails. Each sequence
step takes a fresh observation and action lease, and shares the bound-action
execution rules with the single semantic action. Two hunks were rebased by
hand onto our `Computer.` tool naming.

Lead: apache#5475 (a0ce9aa).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Shouly pushed a commit to Shouly/maka that referenced this pull request Sep 27, 2026
Watermark 1e31d0a. Done: apache#5663, apache#5475, apache#5293, apache#5590 (Edit and Read; no
FormatJson here), apache#5356, apache#5426. Consider: apache#5470, apache#5096. Skipped: upstream
renderer and packages/ui, one refactor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(runtime): sequence closing observation cannot be used for the next action

4 participants