Skip to content

docs(desktop): propose unified retry-from-turn semantics (#5132) - #5275

Closed
Nyvo-io wants to merge 2 commits into
apache:mainfrom
Nyvo-io:codex/issue-5132
Closed

Nyvo-io wants to merge 2 commits into
apache:mainfrom
Nyvo-io:codex/issue-5132

Conversation

@Nyvo-io

@Nyvo-io Nyvo-io commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • document a single Host-owned retry-from-turn contract for Regenerate and Edit-and-resend
  • preserve Branch as an inclusive through-turn continuation
  • define lineage compatibility, provider-input exclusion, idempotency, lifecycle, and recovery invariants
  • record the measurements and staged implementation required before runtime changes

Review scope

This is a design-only draft PR. It does not change runtime behavior. Please review the proposed contract and lineage rules before implementation begins.

Verification

  • git diff --check
  • reviewed against the Maka architecture documentation quality gate
  • verified implementation anchors against the current revision coordinator, Desktop revision actions, and Regenerate adapters

Generated-by: OpenAI Codex

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 14, 2026
@Nyvo-io
Nyvo-io force-pushed the codex/issue-5132 branch 2 times, most recently from 89b8f3c to 6cb5049 Compare September 14, 2026 01:33

@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 for bringing this as a design-only draft. I checked #5132 and the current revision coordinator, admission protocol, Desktop edit flow and lineage consumers. The underlying problem is real: Regenerate currently appends in the same Session, while Edit copies the prefix before T. I support converging those two behaviors on the existing Host revision-copy path, while keeping Branch inclusive and independent.

P2 — Specify the durable binding between copy identity and the first replacement admission before calling recovery unified. Reachability: normal reconnect/crash or concurrent-client recovery in the proposed flow. The existing copy fingerprint contains copy kind, source Session, target Session and source Turn; it contains neither replacement content nor the first Message/Turn identity. A committed copy can therefore prove that the prefix exists without proving which replacement was admitted. The proposal currently asks that copy identity/fingerprint to resolve an unknown overall outcome, but does not state how replay distinguishes “copy only” from “replacement already admitted,” or rejects different replacement content under the same attempt. Simply composing copy and send can reuse one target yet still admit a second replacement after a lost acknowledgement.

Please define that binding using the existing message/admission authority: stable first-message identity, canonical-content conflict handling, and recovery behavior before copy commit, after copy commit but before admission, and after admission but before acknowledgement/visibility publication. The lifecycle diagram should map to the existing conversationCopy/revisionState/admission facts rather than suggest another persisted retry state machine. No separate retry ledger is needed unless an existing contract is demonstrably insufficient.

The other P2 is inline: the proposed new regeneratedFromTurnId duplicates existing cross-Session revision provenance while its current UI consumer expects the source Turn inside the same Session.

One wording clarification would help the same contract: the exclusion invariant should allow the explicitly retained/edited canonical user input of T, while excluding T's old execution output and later derived context. Otherwise “no ... provider input derived from T” also excludes the original input required by Regenerate. “Source remains immutable” should mean retry does not rewrite it, without forbidding subsequent independent use of that source Session.

I would keep this as a proposed contract until these boundaries are explicit. Please capture a representative long-history copy baseline before implementation relies on the physical-copy choice; the measurement need not introduce a competing storage design. The material product decision should also follow the existing public dev-list process in CONTRIBUTING before runtime implementation.

Reviewed head: 6cb5049be6ffdac8ad5226c60e597a2941dbb3ac (+113/-0, one design document). CI passes. This was a source-backed design review; I did not run runtime tests or performance measurements, and this comment does not approve an implementation. Codex-assisted review.

Comment thread docs/architecture/desktop-retry-from-turn-design.md Outdated
@Nyvo-io

Nyvo-io commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Updated the design at 4efe77528 and kept the PR as a draft.

The revision now:

  • uses revision provenance for new cross-Session retries and leaves regeneratedFromTurnId as legacy same-Session data;
  • defines the copy fingerprint, derived first-message/Turn identities, canonical-content conflict handling, and the one-replacement guard separately from ordinary later sends;
  • maps lifecycle states to conversationCopy, revisionState, and root-turn admission facts;
  • states the required recover() retention change for a committed copy with a durable retry plan but no admission;
  • preserves the selected canonical user input while excluding superseded output and derived provider context; and
  • makes the long-history baseline, public dev-list decision, and remaining storage/UI choices explicit prerequisites.

I also corrected the admission description to match the current durable identity: admitRootTurn is keyed by (sessionId, turnId), with source-message identity enforced through its proof record. npm run check:asf-headers, git diff --check, and CI test pass.

@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 for the revision. I rechecked 4efe775 against the earlier draft and the existing copy/admission owners. The lineage P2 is resolved: new retries use revision provenance and leave regeneratedFromTurnId as legacy same-Session data. The distinction between copy completion and first-message admission is also much clearer, as are the provider-input exclusion and source immutability statements.

P2 — Define who can complete or abandon the retained pre-admission draft. Reachability: ordinary client exit/Host restart after copy commit but before the first replacement admission. The proposed persisted plan (lines 181–187) contains provenance, a derived message identity and a canonical-content digest, but not the edited canonical content or a durable reference to it. That digest can validate a later resubmission; it cannot reconstruct a user's replacement text/quotes. At the same time, the proposal retains this target across recovery and hides it from ordinary catalogs. If the client draft is gone, the design currently leaves a retained copy with no specified route to complete or abandon it.

Please distinguish two contracts explicitly: resolving/retrying the same request when the client can resend its payload, versus autonomously continuing after the client disappears. If only the former is required, state the durable draft owner, rediscovery and abandonment behavior and avoid promising autonomous recovery. If the latter is required, identify where the full bound canonical input is durably available before copy commit, preferably through an existing message/draft authority. Do not create a separate revision-plan table or repeat family provenance merely to implement a content hash. Also scope any missing-source abandonment rule to unadmitted targets; an already admitted target must recover from its own facts.

My previous request for idempotency was not a requirement to retain every unadmitted copy forever. Please choose the smallest complete recovery contract before expanding the lifecycle. A short failure-boundary table covering lost copy acknowledgement, client exit before admission, lost admission acknowledgement, and changed-content replay would be enough; another framework is unnecessary.

I support the revision-copy direction. The lineage question is closed, but the new retention contract above still needs clarification before implementation. The existing public-decision and baseline prerequisites remain as stated in the draft. CI passes on this head; this was a design/source review, with no runtime tests or new performance measurements. Codex-assisted review.

@Nyvo-io

Nyvo-io commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Updated the design to address the pre-admission recovery P2.

The contract now keeps the complete canonical payload with the initiating client and does not promise autonomous continuation after client exit. It defines the handling for lost copy acknowledgement, client exit before admission, lost admission acknowledgement, and changed-content replay. The Host stores only the copy binding, uses existing orphan cleanup for unadmitted targets, and recovers admitted targets from their own admission facts. No separate retry-plan table or expanded retention is introduced.

Verified with git diff --check and npm run check:asf-headers.

@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 for the third revision; I rechecked fbf606d against main. The round-2 P2 is resolved: the client owns the payload, no autonomous continuation is promised, the binding lives in conversationCopy, and recover() already behaves the way the table relies on (a committed revision target with no admission is discarded, session-revision-coordinator.ts:183-191), so no retention change is needed. Dropping the plan table and the recover() rework made the contract smaller, which is the right direction.

Before the next round I'd like to change where this lands, and that's on me for not saying it earlier. #5132 asks for one decision on the model, recorded on the issue before any implementation. What we have instead is a 287-line document headed for docs/architecture/, and three review rounds have mostly been me checking its description of current code against the source. Two of this round's remaining points are again the document describing main inaccurately: it says copied-but-unadmitted targets are excluded from the ordinary catalog, but the Host catalog only filters conversationCopy.state = 'preparing' (sqlite-session-catalog-query.ts:41) and the hiding is the client's visibleSessionRevisionMembers with an activeId exception; and it promises to report an abandoned target as abandoned rather than silently recreate it, while discardStableConversationCopy deletes the metadata and claim rows with no tombstone, so a replay resolves as absent and creates a new copy. A planned-behaviour document merged into main has no source to stay honest against, and these are exactly the drifts it would carry.

So the proposal: close this PR without merging, and post the decision itself on #5132 in the shape the issue asked for. Roughly a screen of text is enough:

  • the model: Regenerate becomes the unchanged-input case of the revision-copy path, Branch stays inclusive and independent;
  • the binding: derived first-message/first-Turn identity and canonical-content digest stored on conversationCopy (extending its shape, guard and transition check; SubagentSessionSpawn is the same pattern already in core/session.ts), admission keyed as today;
  • the four recovery boundaries as you have them, with the abandoned/absent choice made explicitly (either they are indistinguishable and both mean a new attempt, or a tombstone is part of the plan);
  • the two prerequisites: the dev-list decision per CONTRIBUTING and the long-history copy baseline.

Once that's agreed on the issue, the design note belongs with the implementation PR, where every sentence in it can be checked against the code it describes. Everything you've worked out here carries over; it just shouldn't be a standalone document that main has to keep true on its own.

AI assistance: I used Claude Code to verify the catalog filter and discard paths against main; conclusions are mine.

中文版

感谢第三版;我对着 main 重新核过 fbf606d。第二轮的 P2 已解决:客户端持有 payload,不承诺自动续跑,绑定放在 conversationCopy,而 recover() 现状正好就是表格依赖的行为(已提交但无 admission 的 revision 目标会被丢弃,session-revision-coordinator.ts:183-191),不需要改保留规则。去掉 plan table 和 recover() 改造让契约更小了,方向是对的。

下一轮之前我想改一下这件事落地的位置,之前没早说是我的问题。#5132 要的是在 issue 上记录一个关于模型的决策,然后再实现。现在的形态是一份 287 行、准备合进 docs/architecture/ 的文档,三轮评审大部分时间是我在把它对现有代码的描述回源码核对。这轮剩下的两点又是文档对 main 的描述不准:它说已复制未 admit 的目标不在普通目录里,但 Host 目录只过滤 conversationCopy.state = 'preparing'sqlite-session-catalog-query.ts:41),隐藏是客户端 visibleSessionRevisionMembers 做的,还带 activeId 例外;它承诺已放弃的目标要如实报告而不是静默重建,但 discardStableConversationCopy 直接删掉 metadata 和 claim 行、不留 tombstone,重放时解析为 absent 就会新建一份。一份描述计划行为的文档合进 main 后没有源码可以对照,这些漂移它会原样带进去。

所以建议:这个 PR 不合并、关闭,把决策本身按 issue 要求的形式发到 #5132。大约一屏文字就够:

  • 模型:Regenerate 成为 revision-copy 路径的"输入未改"情形,Branch 保持 inclusive 且独立;
  • 绑定:派生的首条消息/首个 Turn 身份和规范内容摘要存在 conversationCopy 上(扩展它的 shape、guard 和 transition 检查;SubagentSessionSpawncore/session.ts 里已经是同样的模式),admission 键保持现状;
  • 四条恢复边界照你现在的写法,并明确 abandoned/absent 的取舍(要么两者不可区分、都算新尝试,要么 tombstone 列入计划);
  • 两条前置:CONTRIBUTING 要求的 dev-list 决策,和长历史 copy 基线。

在 issue 上达成一致后,设计说明随实现 PR 一起提,那时每一句都能对着它描述的代码核对。你在这里做出的东西都能带过去,只是不该是一份需要 main 自己维持其正确性的独立文档。

AI 辅助:我用 Claude Code 对着 main 核实了目录过滤和丢弃路径;结论由我负责。

@Nyvo-io

Nyvo-io commented Sep 15, 2026

Copy link
Copy Markdown
Contributor Author

Closing this design-only PR without merging, per the maintainer guidance. The agreed retry model, binding, recovery boundaries, and prerequisites are now recorded on Issue #5132: #5132. Any implementation will be proposed in a follow-up PR after the prerequisites are satisfied.

@Nyvo-io Nyvo-io closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants