Skip to content

fix(runtime): preserve compacted user anchor at canonical commit - #690

Merged
mergify[bot] merged 6 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-canonical-compaction-boundary-main
Sep 4, 2026
Merged

fix(runtime): preserve compacted user anchor at canonical commit#690
mergify[bot] merged 6 commits into
matrixorigin:mainfrom
gouhongshen:codex/fix-canonical-compaction-boundary-main

Conversation

@gouhongshen

@gouhongshen gouhongshen commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

What type of PR is this?

  • feat (new feature)
  • fix (bug fix)
  • docs (documentation)
  • style (formatting, no code change)
  • refactor (code change that neither fixes a bug nor adds a feature)
  • perf (performance improvement)
  • test (adding or updating tests)
  • chore (maintenance, tooling)
  • build / ci (build or CI changes)

Which issue(s) this PR fixes

N/A — main-branch port of the production MOI failure fixed for moi-dev in #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_boundary marker. Both canonical projections on main incorrectly 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 with canonical turn produced no committable messages.

This change makes the completed and resumable current-turn commit projections honor producer-owned UserTurnSemantics:

  • only a visible tail user explicitly marked ObjectiveRelation::Replace authorizes 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;
  • runtime-owned or empty messages cannot replace the objective, and malformed semantics remain errors.

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 with CanonicalRewriteProof, exercises a canonical Replace commit, 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

  • Canonical owner changed or extended: astra-turn-core current-turn commit projections, using existing astra-turn-types::ObjectiveRelation authority.
  • Existing implementations/callers searched: tiered compaction layers and engine, runtime lifecycle terminal commit, canonical delta construction, recovery projection, and prompt-facing projection tests.
  • Superseded code, states, tables, shims, and self-only tests removed: replaced the any-tail-user truncation predicate and untyped replacement fixture with typed authorization and regression matrices.
  • Net code/state/table delta: three existing Rust files; one explicit cross-crate projection entrypoint; no schema, state, configuration, fallback, or compatibility layer.
  • If parallel implementations remain, the external boundary and retirement condition: N/A. Display/provider projection and canonical projections intentionally have different contracts and retain their existing owners.

Production wiring and verification

  • Public product entrypoint exercised: Astra server run lifecycle canonical commit used by MOI.
  • Unhappy paths exercised: single-user anchor loss, non-replacing follow-ups, multiple typed replacements, runtime-owned/empty replacement messages, and corrupt semantics. Real compaction/commit/restore checks preserve typed objective context.
  • Database schema/query/transaction/migration verified against a real database, or N/A with reason: N/A; this is an in-memory message projection correction and does not touch persistence schema or queries.

Verification:

  • cargo fmt --all -- --check
  • cargo test -p astra-runtime --lib single_user_tool_turn_remains_committable_after_real_tiered_compaction -- --nocapture
  • cargo test -p astra-runtime --lib typed_objective_relations_survive_real_tiered_compaction -- --nocapture
  • cargo test -p astra-turn-core --lib compacted_current_turn -- --nocapture
  • cargo 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 passed
  • Negative control: temporarily restoring the prior any-tail-user decision makes the new objective-preservation and last-replacement tests fail; restoring the typed decision makes the full suite pass.
  • cargo check -p astra-turn-core
  • cargo clippy -p astra-turn-core -p astra-runtime --lib -- -D warnings
  • Earlier full-workspace make lint run — blocked by an existing origin/main error in untouched crates/astra-cli/src/cli/turn/turn_entry.rs: unused import acquire_interactive_turn_admission

@gouhongshen
gouhongshen marked this pull request as ready for review September 3, 2026 13:38
@mergify
mergify Bot requested a review from XuPeng-SH September 3, 2026 13:38

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

结论:请求修改(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 会拿到“旧目标+新目标”,旧目标不应再影响后续行为。

我在本地构造了多轮复现:

  1. 先用 userA 压入旧目标并触发 compact(review everything),
  2. 同一轮引入 userB 新目标(do not review; translate instead),
  3. 保存/恢复续写。
    结果下一轮仍残留 userA 的旧目标。

建议

请仅在“保留 boundary 之前 user anchor”确实必要时再保留(即 boundary 后没有新 user anchor 时):

  • 参考旧提交 689 的兜底:当 boundary 后存在可见用户消息时,放弃保留旧 head;
  • 或用显式元数据标记 canonical rewrite 的必要 user anchor。

@gouhongshen

Copy link
Copy Markdown
Collaborator Author

该意见有效,已在 d06be068 修复,并同步到 moi-dev PR #689(条件逻辑已在 dd7f13a3,真实回归测试补于 d983185d)。

实现没有无条件保留 pre-boundary user:completed 与 resumable current-turn projection 现在共用同一个边界起点判定。boundary 后存在有效可见 user 时,从 boundary 开始投影,旧 head 被丢弃;tail 没有新 user 时才保留 protected head,避免单用户工具长任务失去唯一锚点。

新增两层回归覆盖:直接投影单测,以及实际 CompactionEngine + TieredCompaction + canonical_commit_delta 的多用户 case;同时保留原有单用户真实 compaction case。两种形态及 completed/resumable 路径均已通过。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

结论:Request Changes。

上一轮指出的“明确新目标不应保留旧目标”在当前 head 中已有条件化处理;但这个条件仍然会破坏 Astra 自己定义的 typed objective semantics。

P1 — 不能用任意 tail user 代表目标替换

compacted_canonical_turn_startcrates/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 的原因。

@gouhongshen

Copy link
Copy Markdown
Collaborator Author

这条 P1 有效,已修复于 4c626927,并同步到 #689cc789120

核对了 ObjectiveRelation 定义及 resume_hydration::objective_context_from_entries:只有 Replace 会清空原目标,Refine/Correct 是追加,Continue/Acknowledge 不改变目标。因此 canonical 起点现在只选择 boundary 后最后一条有效、可见、显式 Replace user,并从该消息本身开始;其他 relation、Unknown 和缺失 metadata 不授权删除旧目标。runtime-owned/空消息不能充当 replacement,损坏的 metadata 仍严格报错。

main 的 completed/resumable 共用上述 helper;#689 同步相同判定,并把当前任务提交与既有 Session/CLI 续写入口分开,后者维持原合同。

验证:

  • 相同文本配不同 typed metadata,覆盖 6 种 relation 和缺失 metadata,另测多个 replacement、runtime-owned/空 replacement、损坏 metadata。
  • 真实 TieredCompaction → CanonicalRewriteProof → Replace commit → 序列化恢复,比较恢复前后 typed objective;main 同时覆盖 completed/resumable。
  • 将判定临时恢复成上一版 any-tail-user 后,两条新回归测试失败;恢复修复后全绿。
  • main turn-core 3729 passed、受影响库 clippy 通过;moi-dev turn-core 3634 passed、workspace/all-targets check 和 5 条 Session/CLI nextest 均通过。

两个 PR 描述已同步更新,请重新审查。

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

已重新审查当前 head。之前针对任意 tail user 都触发替换、以及 Refine/Correct 被误删的两个问题,已经通过类型化 ObjectiveRelation::Replace 修复;resumable projection 保留完整 tool groups,completed projection 只去除执行 scratch,边界清楚。新增测试覆盖 Replace、Refine、Correct、Continue、Acknowledge、Unknown 和缺省语义。当前实现未发现阻断问题,批准;合并前按仓库规则更新到最新 main 即可。

@mergify mergify Bot added the queued label Sep 4, 2026
@mergify

mergify Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

  • Entered queue2026-09-04 04:30 UTC · Rule: main · triggered by rule Automatic queue on approval for main
  • Checks passed · in-place
  • Merged2026-09-04 04:46 UTC · at 5814616207ca133d7a7fa84bf4f5a2c017c3da0f · squash

This pull request spent 15 minutes 41 seconds in the queue, including 15 minutes 13 seconds running CI.

Required conditions to merge
  • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
  • github-review-approved [🛡 GitHub branch protection]
  • any of [🛡 GitHub branch protection]:
    • check-success = Check PR title
    • check-neutral = Check PR title
    • check-skipped = Check PR title
  • any of [🛡 GitHub branch protection]:
    • check-success = check
    • check-neutral = check
    • check-skipped = check
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: core crates + bridge hooks
    • check-neutral = Test: core crates + bridge hooks
    • check-skipped = Test: core crates + bridge hooks
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-runtime
    • check-neutral = Test: astra-runtime
    • check-skipped = Test: astra-runtime
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-cli (edge-fs-tools)
    • check-neutral = Test: astra-cli (edge-fs-tools)
    • check-skipped = Test: astra-cli (edge-fs-tools)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-cli (edge-git-gix)
    • check-neutral = Test: astra-cli (edge-git-gix)
    • check-skipped = Test: astra-cli (edge-git-gix)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-cli (edge-rest)
    • check-neutral = Test: astra-cli (edge-rest)
    • check-skipped = Test: astra-cli (edge-rest)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-cli (edge-shell)
    • check-neutral = Test: astra-cli (edge-shell)
    • check-skipped = Test: astra-cli (edge-shell)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: astra-cli (non-edge)
    • check-neutral = Test: astra-cli (non-edge)
    • check-skipped = Test: astra-cli (non-edge)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: online (core)
    • check-neutral = Test: online (core)
    • check-skipped = Test: online (core)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: online (integration)
    • check-neutral = Test: online (integration)
    • check-skipped = Test: online (integration)
  • any of [🛡 GitHub branch protection]:
    • check-success = Test: turn-core + services + plan
    • check-neutral = Test: turn-core + services + plan
    • check-skipped = Test: turn-core + services + plan
  • any of [🛡 GitHub branch protection]:
    • check-skipped = @astra/sdk (typecheck, test+coverage, build)
    • check-neutral = @astra/sdk (typecheck, test+coverage, build)
    • check-success = @astra/sdk (typecheck, test+coverage, build)
  • any of [🛡 GitHub branch protection]:
    • check-skipped = web (typecheck, test, build)
    • check-neutral = web (typecheck, test, build)
    • check-success = web (typecheck, test, build)

@mergify
mergify Bot merged commit 5814616 into matrixorigin:main Sep 4, 2026
19 of 20 checks passed
@mergify mergify Bot removed the queued label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants