fix(runtime): preserve compacted user anchor at canonical commit - #690
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
结论:请求修改(Request Changes)
我发现一个高优先级回归风险(P1)需要修复:
- canonical
Replace场景下,保留 pre-boundary user 的行为会把已被替换/失效的早期用户输入持久化并带入下一轮对话,改变语义。 - 该 PR 在
sanitize_compacted_canonical_continuation_messages_with_turn_semantics(..., preserve_head=true)中无条件保留 boundary 前最后一个 user,即使 boundary 之后有新的有效用户消息。 - 在 tiered compaction 后的
CanonicalRewriteProof场景中,下一轮conversation会拿到“旧目标+新目标”,旧目标不应再影响后续行为。
我在本地构造了多轮复现:
- 先用 userA 压入旧目标并触发 compact(
review everything), - 同一轮引入 userB 新目标(
do not review; translate instead), - 保存/恢复续写。
结果下一轮仍残留 userA 的旧目标。
建议
请仅在“保留 boundary 之前 user anchor”确实必要时再保留(即 boundary 后没有新 user anchor 时):
- 参考旧提交 689 的兜底:当 boundary 后存在可见用户消息时,放弃保留旧 head;
- 或用显式元数据标记 canonical rewrite 的必要 user anchor。
|
该意见有效,已在 实现没有无条件保留 pre-boundary user:completed 与 resumable current-turn projection 现在共用同一个边界起点判定。boundary 后存在有效可见 user 时,从 boundary 开始投影,旧 head 被丢弃;tail 没有新 user 时才保留 protected head,避免单用户工具长任务失去唯一锚点。 新增两层回归覆盖:直接投影单测,以及实际 |
XuPeng-SH
left a comment
There was a problem hiding this comment.
结论:Request Changes。
上一轮指出的“明确新目标不应保留旧目标”在当前 head 中已有条件化处理;但这个条件仍然会破坏 Astra 自己定义的 typed objective semantics。
P1 — 不能用任意 tail user 代表目标替换
compacted_canonical_turn_start(crates/astra-turn-core/src/prompt_facing.rs:569-578)只要 compaction boundary 后存在一个可见 user,就从 boundary 开始投影并删除 protected head。可是 ObjectiveRelation 明确区分:
Replace:替换目标;Refine:给当前目标增加要求;Correct:修正当前理解/方法;Continue/Acknowledge:目标不变。
例如 user A: review the repo -> boundary -> user B (Refine): also check unhappy paths。当前实现令 tail_has_user = true,最终 completed/resumable canonical commit 都只剩 B,A 被永久删除;B 中的 “also” 随即失去所指目标。恢复投影本身也把 Refine/Correct 作为在现有 objective 上追加,而不是清空 objective,所以这里与既有语义契约不一致。
新增的 replacement 测试使用了普通 user JSON,没有附 _astra_user_turn_semantics: Replace,实际上是通过英文内容暗示“替换”,绕过了本仓库“不要从文本猜 lifecycle state”的 typed contract。
建议按语义选择 canonical 起点:只让具有明确替换权限的 tail user(至少 ObjectiveRelation::Replace)淘汰旧 head,并从真正的 replacement anchor 开始,而不是用 any(user) 布尔值;Refine/Correct/Continue/Acknowledge 必须保留原目标。对缺失/Unknown semantics 需要采取不丢数据的保守策略,或明确兼容契约。请补真实 TieredCompaction 的 Replace、Refine、Continue(completed + resumable)回归测试。
其余检查:当前 head 的 GitHub CI 全绿,git diff --check 通过;PR 仅落后 main 两个无源代码重叠的提交,不是本 finding 的原因。
|
这条 P1 有效,已修复于 核对了
验证:
两个 PR 描述已同步更新,请重新审查。 |
XuPeng-SH
left a comment
There was a problem hiding this comment.
已重新审查当前 head。之前针对任意 tail user 都触发替换、以及 Refine/Correct 被误删的两个问题,已经通过类型化 ObjectiveRelation::Replace 修复;resumable projection 保留完整 tool groups,completed projection 只去除执行 scratch,边界清楚。新增测试覆盖 Replace、Refine、Correct、Continue、Acknowledge、Unknown 和缺省语义。当前实现未发现阻断问题,批准;合并前按仓库规则更新到最新 main 即可。
Merge Queue Status
This pull request spent 15 minutes 41 seconds in the queue, including 15 minutes 13 seconds running CI. Required conditions to merge
|
What type of PR is this?
Which issue(s) this PR fixes
N/A — main-branch port of the production MOI failure fixed for
moi-devin #689.What this PR does / why we need it
Tiered compaction physically removes the compacted middle of the runtime message vector while deliberately preserving the first user message before its
_compact_boundarymarker. Both canonical projections onmainincorrectly treated that marker as another truncation point. For a single-user, tool-heavy turn with no later user message, terminal commit discarded the only user anchor and then dropped all remaining messages as orphaned, ending withcanonical turn produced no committable messages.This change makes the completed and resumable current-turn commit projections honor producer-owned
UserTurnSemantics:ObjectiveRelation::Replaceauthorizes dropping earlier retained messages; projection starts at the last such replacement message, not at the boundary marker;Refine,Correct,Continue,Acknowledge,Unknown, and absent semantics do not authorize dropping the existing objective;Session/CLI continuation recovery keeps its pre-existing latest-boundary contract through a separate entrypoint backed by the shared sanitizer. No lifecycle decision is inferred from message text.
The real regression matrix runs
CompactionEngine + TieredCompaction, authorizes the rewrite withCanonicalRewriteProof, exercises a canonicalReplacecommit, serializes/restores the result, and compares typed objective context before and after. It covers all six objective relations plus absent metadata, for both completed and resumable projections. The original single-user, tool-heavy production regression remains covered.Architecture and complexity delta
astra-turn-corecurrent-turn commit projections, using existingastra-turn-types::ObjectiveRelationauthority.Production wiring and verification
N/Awith reason: N/A; this is an in-memory message projection correction and does not touch persistence schema or queries.Verification:
cargo fmt --all -- --checkcargo test -p astra-runtime --lib single_user_tool_turn_remains_committable_after_real_tiered_compaction -- --nocapturecargo test -p astra-runtime --lib typed_objective_relations_survive_real_tiered_compaction -- --nocapturecargo test -p astra-turn-core --lib compacted_current_turn -- --nocapturecargo nextest run --manifest-path Cargo.toml -p astra-cli --profile ci --lib -E 'test(/preloaded_turn_messages_drop_stale_pre_compaction_goal_and_trace|preloaded_turn_messages_with_corrupt_semantics_still_drop_runtime_scaffolding|sanitize_compaction_boundary_drops_pre_boundary_stale_goal|sanitize_corrupt_turn_semantics_still_enforces_the_continuation_boundary|csl_projection_uses_the_committed_active_conversation/)'— 5 passed on the preceding revision; this review fix leaves the main Session/CLI path unchanged.cargo test -p astra-turn-core --lib --quiet— 3729 passedcargo check -p astra-turn-corecargo clippy -p astra-turn-core -p astra-runtime --lib -- -D warningsmake lintrun — blocked by an existingorigin/mainerror in untouchedcrates/astra-cli/src/cli/turn/turn_entry.rs: unused importacquire_interactive_turn_admission