Skip to content

feat(runtime-host): bind ScheduledTasks to Connection identity - #4321

Open
testikun wants to merge 6 commits into
apache:mainfrom
testikun:codex/issue-3927-scheduled-task-identity
Open

feat(runtime-host): bind ScheduledTasks to Connection identity#4321
testikun wants to merge 6 commits into
apache:mainfrom
testikun:codex/issue-3927-scheduled-task-identity

Conversation

@testikun

@testikun testikun commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #3927

Scheduled agent_run effects were identified only by slug. Because slugs can be reused after a Connection is deleted, an existing task could resolve to a different account and execute under the wrong provider credentials.

This PR freezes the executable identity to llmConnectionId + llmConnectionSlug + model for new tasks, resolves by the immutable Connection ID, and validates the stored slug and model before creating a Session or AgentRun. Legacy slug-only tasks remain readable and editable for non-effect fields, but fail closed at execution time until explicitly recreated with a Connection identity. Recovery admission also uses a stable execution fingerprint containing the full identity, and the Runtime Host protocol compatibility epoch is set to 89; it follows #4308’s epoch 88 change and must merge after it.

The behavior change is intentionally fail-closed: deleting Connection A and creating Connection B with the same slug can no longer cause A's scheduled task to run as B.

中文摘要

修复 ScheduledTask 仅按 slug 识别 agent_run 的问题。现在新任务固定保存 Connection ID、slug 和 model,执行时按 ID 解析并校验 slug/model;旧的 slug-only 任务仍可读取和修改非 effect 字段,但执行时会安全失败,不会误用同 slug 的新账号。恢复 admission 也使用包含完整身份的稳定指纹,协议 epoch 在本 PR 中为 89;它专用于 ScheduledTask Connection 身份契约,并应在 #4308 的 epoch 88 sandbox continuation 变更之后合并。

Verification

  • Core, Storage, Runtime, and Runtime Host builds pass on the repository-supported Node.js version (>=22.19).
  • Core ScheduledTask tests: 733 passed.
  • Storage tests: 1,037 passed, 8 skipped, 0 failed.
  • Runtime Host ScheduledTask production tests: 13 passed.
  • ScheduledTask protocol tests: 5 passed.
  • Coordinator recovery tests: 3 passed on the supported Node.js version.
  • Runtime Host integration coverage verifies that deleting Connection A and recreating its slug as Connection B leaves the legacy task fail-closed without creating a Session or AgentRun.
  • Biome checks and git diff --check pass.
  • The local system Node.js is v20.20.2; tests that require node:sqlite must run with the repository-supported Node.js version and are covered by CI.
  • The full UI build still reports unrelated pre-existing main-branch errors (settledText, autoScroll, trailingAction); this PR adds no new UI type errors.

AI use

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

Tool(s) and scope: OpenAI Codex designed and implemented the bounded Connection-identity change, added regression coverage, and ran the listed verification. The human contributor remains responsible for review and submission.

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 Aug 31, 2026
@testikun

Copy link
Copy Markdown
Contributor Author

Technical approach

Problem

A scheduled agent_run was historically identified by slug alone. A Connection slug is user-visible and reusable: after Connection A is deleted, a new Connection B can legitimately receive the same slug. Resolving an old task by slug alone can therefore execute it with the wrong account, credentials, or provider configuration.

The fix must preserve old records for safe inspection and migration, while ensuring that execution never silently changes identity.

Implementation

  1. Freeze the executable identity for new tasks

    • New or replaced agent_run effects persist llmConnectionId, llmConnectionSlug, and model together.
    • A new agent_run without a Connection ID is rejected instead of creating an ambiguous task.
  2. Resolve by immutable ID, then validate the snapshot

    • Runtime Host first loads the Connection by llmConnectionId.
    • It requires the Connection to be enabled and checks that its current slug and model policy still match the task snapshot.
    • There is no fallback to another Connection with the same slug. Any missing, disabled, renamed, or incompatible Connection fails closed before creating a Session or AgentRun.
  3. Keep legacy data readable, but fail closed at execution

    • Existing slug-only tasks can still be decoded and displayed.
    • Editing a legacy task's title, intent, schedule, or other non-effect fields does not resubmit the old ambiguous effect.
    • A legacy slug-only agent_run cannot execute until it is explicitly recreated with a Connection identity. This avoids an unsafe implicit migration.
  4. Make recovery and admission identity-aware

    • Scheduled-task root admission receives a stable execution fingerprint containing Connection ID, slug, and model.
    • Recovery matching therefore cannot attach a pending execution to a different Connection that happens to reuse the slug.
    • The Connection is checked again immediately before Session/AgentRun creation to close the delete/recreate race between resolution and admission.
  5. Update protocol compatibility and regression coverage

    • Runtime Host protocol compatibility epoch is bumped from 78 to 79.
    • Coverage includes successful new-task execution, legacy decode/fail-closed behavior, corrupted identity data, slug reuse after deletion, recovery fingerprint matching, and protocol round-tripping.

Safety and compatibility boundary

This is deliberately fail-closed. A task that cannot prove the original Connection identity is not allowed to run, and no provider request is sent. Existing legacy rows are not rewritten automatically; users must recreate them with an explicit Connection selection. The only behavior change is preventing an ambiguous task from running under a different account.

中文摘要

问题根因是旧 ScheduledTask 只保存 Connection slug。slug 可以在删除账号后被新账号复用,导致旧任务可能误用新账号的凭据执行。

本方案让新 agent_run 固定保存 Connection ID + slug + model,执行时先按不可变 ID 查找,再校验启用状态、slug 和 model;不再按 slug 回退。历史 slug-only 任务仍可读取,非 effect 字段仍可编辑,但执行时安全失败,必须显式重新创建后才会运行。恢复 admission 使用包含完整身份的指纹,并在真正创建 Session/AgentRun 前再次校验,以覆盖删除/复用期间的竞态。协议 epoch 从 78 升到 79,并补充新任务成功、旧数据 fail-closed、损坏数据、slug 复用和恢复匹配等回归测试。

@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.

Blocking compatibility issue: this PR declares epoch 84 for the ScheduledTask Connection-identity wire contract, while PR #4308 independently declares the same global epoch 84 for the sandbox-continuation wire contract. RUNTIME_HOST_COMPATIBILITY_EPOCH is a single Host/Client interoperability boundary, not a per-feature version. Both branches are based on the old 9249bf3 base and are currently conflicting with main (which is at epoch 83). Please rebase and either compose both closed-shape changes under one epoch-84 ledger entry if they are intended to ship together, or land one at 84 and bump the other to 85 after the first. The stale 78→79 explanation should be updated as part of the same repair. Until this is resolved, the meaning of epoch 84 depends on merge order and clients cannot be given a deterministic compatibility contract.

Comment thread packages/runtime-host/src/protocol/index.ts Outdated
@testikun
testikun force-pushed the codex/issue-3927-scheduled-task-identity branch from 0d82021 to 19065e0 Compare September 1, 2026 06:14
testikun added 3 commits September 1, 2026 15:04
Keep ScheduledTask Connection identity at epoch 88 after the sandbox continuity contract at epoch 87.\n\nGenerated-by: OpenAI Codex
# Conflicts:
#	packages/runtime-host/src/protocol/index.ts

@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.

Thanks — reviewed 435d1838 for substance. @me2seeks holds the open block on the epoch, so I've left that to their thread apart from one factual note at the end.

The problem is real and the fix is the right shape. Identifying an agent_run effect by slug alone means a deleted Connection's slug can be re-taken and a stored task then executes under a different account's credentials — a credential-confusion bug, not just a resolution bug. Freezing llmConnectionId + llmConnectionSlug + model, resolving by the immutable ID, and validating the stored slug and model before a Session or AgentRun exists is the correct authority: the ID is the identity and the slug becomes a consistency check rather than a lookup key.

Fail-closed for legacy rows is the right default here. Silently resolving a slug-only task to some Connection is exactly the behaviour being removed, and refusing to execute is recoverable by recreating the task, while running under the wrong credentials is not.

I checked the one thing that looked like it might contradict the description. normalizeExecution rejects a missing llmConnectionId outright, while the type marks it optional for legacy rows, so I expected editing a legacy task's title to fail. It doesn't: normalizeUpdateScheduledTaskInput only normalizes effect when the patch carries it (scheduled-task.ts:215), so non-effect edits never reach that check. The claim in the description holds.

Also confirmed the execution fingerprint covers the full identity rather than the slug, so recovery admission can't re-admit a task under a replacement Connection.

No findings from me on the implementation.

Evidence boundary: I read the effect contract, the normalizer paths, the coordinator's resolution and fingerprint, and the agent_run origin change; I did not run the suites and did not review the 476 lines of new tests in detail.

Factual note, not a verdict — that stays with @me2seeks: the description currently tells three different epoch stories (the English summary says 78 → 79, the Chinese summary says 88 and asks for a specific merge order, the code declares 89), while main is at 87. Worth collapsing to one when you next rebase. On the merge-order point specifically, I'd gently suggest this PR shouldn't need to reference another one at all — taking main + 1 and documenting only its own contract makes it mergeable on its own schedule, whichever lands first.


AI-assisted review: drafted with Maka; I verified the legacy-edit path, the normalizer boundaries, and the fingerprint contents against the branch source myself.

@testikun

testikun commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Updated to current main in commit c2e3fe8. This PR now owns epoch 89 and explicitly follows #4308's epoch 88 change; the old epoch-collision review issue is resolved and the PR description is corrected. CI has been retriggered on the new head. Please re-review.

@testikun

testikun commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Also fixed the Windows-only release qualification failure in commit 91ec3f0: the test now compares against path.join('/qualification-scope', 'state-root') instead of a POSIX-literal path. The new CI run is active.

@testikun

testikun commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

The epoch conflict review is now resolved on the current branch: main epoch 87 → this PR epoch 89, with #4308 owning the preceding epoch 88 contract. The PR body and Chinese summary have been updated to describe the required merge order. Latest CLI package validation and Windows recovery checks are passing; the remaining CI run is still queued. Please re-review the current head after the epoch fix.

@testikun

testikun commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

@me2seeks @Astro-Han — the epoch collision is resolved on current head 91ec3f0b3 (87 → #4308/88 → this PR/89), and CLI package validation plus Windows recovery are green. Could you please re-review the updated head?

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.

feat(runtime-host): bind ScheduledTasks to immutable Connection identity

3 participants