Skip to content

R1 P0: server session 设计收口与吸收清单验真 - #1894

Open
Kizunad wants to merge 15 commits into
mainfrom
refactor/plan-refactor-server-session-v1
Open

R1 P0: server session 设计收口与吸收清单验真#1894
Kizunad wants to merge 15 commits into
mainfrom
refactor/plan-refactor-server-session-v1

Conversation

@Kizunad

@Kizunad Kizunad commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • docs/plans-skeleton/plan-refactor-master-v1.md 的 R1 轨道、文件所有权与覆盖矩阵,将 plan-refactor-server-session-v1 从 skeleton 促升为 active
  • 冻结 InteractionSessionSessionRegistry、durability、busy claim、五态生命周期与 completion delivery 原子性合同
  • 第一性验真 13 项既有吸收对象,并核对新增 session/dimension 候选的 R1/R2/R4/R6/R7/R10 唯一 owner
  • 收口两项 pre-P0 决议:显式 Checkpointed/Volatile 经 R3 单一出口;非自愿中断全退、主动取消沿用域内规则

Test plan

  • git diff --check
  • 检查 P0 合同 marker、13 项吸收结论及覆盖审计差分
  • 基于最新 origin/main 完成 rebase(主线无新增差分)
  • docs-only 变更;未运行 server lifecycle 脚本或 Cargo

Plan

  • 总纲:docs/plans-skeleton/plan-refactor-master-v1.md
  • Active:docs/plan-refactor-server-session-v1.md
  • 本 PR 仅完成 R1 P0 / Wave 0,不落实现代码,不越界修改 R3/R4/R6/R10 独占文件

主导模型:cc-sonnet-high(Claude Code 客户端占位模型;AxonHub 实际上游未在请求中可靠暴露)

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Kizunad, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 43 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 96e0615c-d1d4-4519-a52e-9fc11c7d2700

📥 Commits

Reviewing files that changed from the base of the PR and between dfe84d9 and e8461f2.

📒 Files selected for processing (9)
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plan-refactor-inventory-core-v1.md
  • docs/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
📝 Walkthrough

Summary by CodeRabbit

  • 文档
    • 完善交互会话的暂停、恢复、关闭、重连及持久化生命周期规划。
    • 明确 Craft Screen 的打开、暂停、取消和恢复行为,以及工作台目标校验规则。
    • 补充 Craft 请求校验、消息契约、投递可靠性和端到端验收场景。
    • 统一客户端状态、服务端会话及跨模块改造边界,更新实施阶段与依赖关系。
    • 移除已废弃的服务端会话重构计划文档。
    • 新增 Craft 生命周期数据契约及终端交付工作流程规划。

Walkthrough

本次变更更新会话生命周期、持久化交付、Craft 协议、客户端状态和跨轨实施计划。新增 InteractionSession、Craft schema、SessionDeliveryWorker 及对应的门禁、恢复和验收约束。

Changes

会话重构计划

Layer / File(s) Summary
服务端会话合同与生命周期
docs/plan-refactor-server-session-v1.md
定义 InteractionSession、registry、durability、暂停、终止、恢复、delivery outbox 和生命周期不变量。
持久化切片与终端交付
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md, docs/plan-refactor-inventory-core-v1.md
增加 suspension lease、TsyPresence coupled snapshot、reservation、outbox 和 SessionDeliveryWorker 的 claim、receipt、重试及 dead-letter 规则。
Craft 合同、协议与门禁
docs/plans-skeleton/plan-agent-craft-schema-v1.md, docs/plans-skeleton/plan-refactor-wire-s2c-v1.md, docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
新增四类 Craft 合同、CraftSessionStateV2、三类 lifecycle intent、116 个 C2S 变体及 owner、phase、busy 和 generation 校验。
Craft 客户端生命周期
docs/plan-refactor-client-store-lifecycle-v1.md, docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
规定 CraftStore 为唯一 session-state owner,并定义 Craft Screen 的 open、pause、cancel 和 resume 行为。
阶段放行与跨轨治理
docs/plans-skeleton/plan-refactor-master-v1.md, docs/plan-refactor-server-session-v1.md
调整 server/client 修改边界、Agent schema 所有权、Wave 依赖、artifact producer/consumer 闭环、宿主迁移顺序和 P1–P4 验收范围。

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • Kizunad/Bong#1892:提供 C2S gate matrix 的前置工作,本次计划扩展 Craft 变体。
  • Kizunad/Bong#1893:建立 R10 inventory-core 计划,本次增加 terminal-delivery worker。
  • Kizunad/Bong#1302:提供 SessionScopedStore 与 registry 生命周期框架,本次扩展至 CraftStore

Poem

兔子跳过 Craft 门,
Store 守住状态根。
Pause、Resume 带身份,
Outbox 记录每次交付。
会话按计划恢复运行。

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed 标题准确概括了 R1 P0 的 server session 设计收口和吸收清单验真,符合主要变更。
Description check ✅ Passed 描述明确说明了文档变更、R1 P0 目标、合同冻结、验真范围和测试计划,与变更内容相关。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/plan-refactor-server-session-v1
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch refactor/plan-refactor-server-session-v1

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: 25e3e11be7102a1ea226ecf8eac7245d2a5eb1bf
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Bound suspended sessions before retaining facility claims indefinitely

docs/plan-refactor-server-session-v1.md:136 · performance

A player can start a checkpointed alchemy or forge session, disconnect, and never reconnect. The new contract requires disconnect to keep the stable owner/facility claim while the session remains Suspended (lines 80, 92, and 136), and the only restore-conflict behavior is to keep it suspended (line 112). No phase, deliverable, or P1-P4 acceptance item defines an abandonment timeout, administrative teardown, checkpoint pruning, or claim reclamation. The referenced R3 interface only provides guarded load, autosave, shutdown flush, and tick rebasing, so it does not supply a retention policy. Consequently every abandoned unique-player session remains in the registry/checkpoint store indefinitely; more importantly, each abandoned furnace/station claim can remain unavailable forever. Account churn therefore causes monotonically growing durable/live session state and permanently consumes a growing set of finite world facilities.

Root cause: the frozen lifecycle treats every disconnected checkpointed session as indefinitely resumable and requires its facility claim to be retained, but it defines no bounded suspension lifetime or reclamation path for owners who never return. because later implementation phases are required to preserve this contract, the omission creates an unbounded resource-retention path rather than merely leaving an optional cleanup optimization unspecified.

[major] Checkpointed disconnect/shutdown both preserves and refunds escrow

docs/plan-refactor-server-session-v1.md:143 · schema-contracts

The frozen lifecycle contract says every Checkpointed session enters Suspended after disconnect/shutdown and retains its checkpointed state (docs/plan-refactor-server-session-v1.md:80, :92, :105). The durability matrix likewise requires craft to save progress, batch, and escrow on disconnect/shutdown (:118). However, the refund contract states that Disconnect and Shutdown are non-voluntary interruptions for which all unconsumed escrow is fully refunded (:143, repeated at :157). A craft session with remaining escrow that disconnects or shuts down therefore reaches both rules: one requires preserving the escrow in a resumable suspended session, while the other requires returning it to the player. Neither the phase/transition enums nor surrounding text establishes precedence or excludes checkpointed sessions from the refund rule. Implementing both can duplicate resources on restore; choosing either violates the other documented contract. This contradiction is introduced by the expanded P0 plan and is not present in the deleted skeleton, which left interruption policy as an open question.

Root cause: the plan resolved the interruption-refund policy globally by terminationcause without distinguishing teardown from checkpoint/suspend transitions. it consequently treats disconnect/shutdown as refunding terminal interruptions even when sessiondurability::checkpointed requires those same events to preserve escrow for recovery.

[major] Contract pins omit the termination-cause refund matrix

docs/plan-refactor-server-session-v1.md:206 · testing

The plan freezes an observable economic contract in §2.5: VoluntaryCancel may apply the craft 70% refund, while Disconnect, DimensionChange, Shutdown, and InvalidRestore must return all unconsumed escrow. It also introduces TerminationCause as a new enum whose variants drive that behavior. However, the P1 test deliverables at line 206 pin phase transitions, rebinding, lifecycle ordering, busy conflicts, and delivery commit gating, but do not pin every termination-cause variant or its refund result. The later P4 scenario names likewise do not state refund assertions for all causes. An implementation that routes Shutdown or InvalidRestore through the voluntary-cancel branch and refunds only 70% would therefore satisfy every named pin while causing reproducible resource loss; no existing test can protect a contract that is newly defined by this plan.

Root cause: the test plan does not include a positive/negative matrix over the newly frozen terminationcause enum and its cause-specific refund semantics, despite the plan making those variants economically distinct and requiring adapters to branch on them.

[major] TsyPresence restore is delegated to an R3 deliverable that does not exist

docs/plan-refactor-server-session-v1.md:218 · wiring

The plan requires TsyPresence to be checkpointed by R3 (docs/plan-refactor-server-session-v1.md:114-127) and makes guarded restore parity a P3 deliverable (:214-218), while R1 may only consume R3 persistence hooks and cannot edit persistence code (:190-198). But the referenced R3 plan neither lists TsyPresence in its persistence inputs, phases, absorbed work, nor tests; its omitted-runtime-state P4 list does not include it (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:21-33), and its bot scenarios likewise contain no TSY presence restore case (:41-46). Therefore neither track owns a concrete production persistence registration/loading deliverable for the state that R1's P3 acceptance requires, so implementing both plans as written can still leave the existing position/dimension-to-presence restore break disconnected.

Root cause: r1 delegates the actual tsypresence checkpoint and restore producer to r3, but r3's plan does not register or schedule that state, leaving the cross-track persistence connection without an implementation owner.

[major] P1 pause/resume contract has no schedulable cross-stack owner

docs/plan-refactor-server-session-v1.md:198 · schema-contracts

P1 claims craft screen close/pause, explicit cancel, and reopen/resume as its deliverable (docs/plan-refactor-server-session-v1.md:204-205), while the frozen contract acknowledges that the current wire exposes only CraftCancel and therefore requires new cross-stack open/pause/resume intents (:141). Yet the ownership contract forbids R1 from changing the proto/bridge, client UI/store, or client_request_handler.rs, assigning those pieces to R6, R7/R2, and R4 (:194-196), and the only implementation-wave dependency stated for R1 is R3 P1 (:198). The master schedule also places R4 in Wave 2 after R6 P1 while R1 starts in Wave 1, so R4 cannot be assumed available for R1 P1. Following the documented schedule can therefore complete the permitted R1 server adapter while leaving no production producer/decoder/handler for pause and resume, making the claimed P1 user workflow unreachable. Contract pins inside R1 cannot repair the absent cross-stack wiring.

Root cause: the plan assigns a cross-stack wire feature to p1 but neither includes the owning r6/r4/r7 deliverables in that phase nor declares them as prerequisites. its file-ownership and wave rules prevent r1 from implementing the required producer, schema, handler, and client consumer itself.

[major] R1 phases are scheduled before their required cross-track wiring

docs/plan-refactor-server-session-v1.md:197 · strict-maintainability

The plan allows the R1 implementation wave after only R3 P1 and says P1 may implement the craft adapter (line 197), while P1's claimed behavior requires screen-close pause, explicit cancel, and reopen resume (lines 203-205). The same document assigns the required proto/bridge changes to R6, request handler/gate changes to R4, and client Screen changes to R7/R2 (lines 193-195). None is made a prerequisite here. The master schedules R1 in Wave 1 but R4 in Wave 2 (docs/plans-skeleton/plan-refactor-master-v1.md:52-55), and the R4 and R6 plans contain no craft pause/resume deliverable. Likewise P2 requires InventoryTxn::deliver, but R10 is only a parallel Wave 2 track and its plan says R10 defines that API (docs/plans-skeleton/plan-refactor-inventory-core-v1.md:31-35). Following the stated schedule leaves the production C2S/client wiring and delivery API unavailable; tests could exercise registry transitions directly while the real pause/resume or safe-delivery path remains unreachable.

Root cause: the promoted plan consumes mandatory cross-track contracts but records only ownership notes, not dependency gates or matching deliverables in the owning plans, so its implementation phases can be declared complete before their production producers and consumers exist.

[major] P1 craft pause/resume has no scheduled wire or handler path

docs/plan-refactor-server-session-v1.md:205 · wiring

P1 explicitly promises that closing the craft screen pauses, an explicit cancel remains available, and reopening resumes (docs/plan-refactor-server-session-v1.md:202-206). The plan also admits the current wire exposes only CraftCancel, so this behavior requires new open/pause/resume intents (:139-144). However, R1 is forbidden from changing the proto/S2C bridge and client_request_handler.rs; those are assigned to R6 and R4 (:190-198). No P1 dependency or named R6/R4 deliverable ensures those producers, wire variants, dispatch handlers, and client handlers land before the craft adapter: the only implementation-wave dependency is R3 P1 (:198), while the master schedules R4 later in Wave 2 (docs/plans-skeleton/plan-refactor-master-v1.md:51-56). Thus P1 can satisfy its owned-file checklist while the claimed production pause/resume path remains impossible to invoke.

Root cause: the plan assigns a cross-stack p1 acceptance behavior to r1 while assigning every required wire producer and dispatcher to other tracks, without adding a blocking dependency or concrete cross-track phase deliverable for that connection.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: 59f5da797f5a22d8be7c418da2a6756a9775a9f6
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Terminal delivery loses the existing crash-atomic checkpoint

docs/plan-refactor-server-session-v1.md:121 · concurrency-atomicity

The new terminal sequence at line 121 is deliver -> clear checkpoint -> release claims -> Terminal, while ownership is deliberately split between R10 inventory code and R3 persistence (docs/plan-refactor-server-session-v1.md:211-215). The R10 plan only requires an "idempotency key" and failure-not-commit behavior (docs/plans-skeleton/plan-refactor-inventory-core-v1.md:21); it never requires the idempotency receipt to be durable or atomically committed with the inventory/drop mutation and R3 session-checkpoint deletion. A reachable reproduction is: an expired craft/alchemy/forge session successfully inserts its refund/output into inventory (or creates the spill), then the process exits before the checkpoint is cleared. On restart R3 restores the still-AwaitingDelivery checkpoint and retries line 121, so an in-memory idempotency key/registry lock cannot prevent duplicate delivery. Conversely, clearing the checkpoint first would lose the items on a delivery failure. The live SessionKey CAS at line 120 only serializes concurrent actors and does not close this crash window. This is a regression in the proposed migration because current craft explicitly commits inventory, session, and durable drops in one SQLite transaction (server/src/player/state.rs:820-869), and the finished-plan contract documents that terminal publication/removal occurs only after that unified checkpoint (docs/finished_plans/plan-bughunt-craft-refund-full-inventory-loss-v1.md:68-74). The listed tests cover delivery failure/retry and ordinary restart, but not a restart between delivery commit and checkpoint deletion (docs/plan-refactor-server-session-v1.md:124,253-254).

Root cause: the plan splits terminal delivery and session persistence across r10 and r3 without assigning a durable atomic commit or recovery protocol. a live registry lock plus an unspecified idempotency key cannot provide exactly-once behavior across process failure; the contract must require a durable receipt/outbox (or a single transaction where storage permits) atomically coupled to inventory/spill mutation and session terminalization, and test the crash boundary.

[major] Persist the TSY entry inventory snapshot across relog

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:24 · strict-maintainability

The changed P1 contract fixes the persisted TsyPresence fields to family_id, entered_at_tick, return_to, and schema/version, omitting entry_inventory_snapshot. The production component includes that field (server/src/world/tsy.rs:47-57), entry captures it from the player's inventory (server/src/world/tsy_portal.rs:108-114), and TSY death handling uses it as the authoritative distinction between carried items and TSY-acquired items (server/src/inventory/tsy_death_drop.rs:54-62,84-102). The absorbed bug plan also explicitly requires it for the persistence-based remedy (docs/plans-skeleton/plan-bughunt-world-transport-tsy-relog-presence-v1.md:47-49). Thus a player who enters TSY with items, disconnects/restarts, restores under this planned schema, and then dies cannot recover the original snapshot; those carried items are classified as TSY-acquired and dropped at 100% instead of receiving the carried-item 50%/weapon-protection treatment. The new relog tests only pin the same incomplete field list, so they would accept this loss.

Root cause: the plan duplicates the tsypresence persistence schema but leaves out the component's gameplay-critical entry_inventory_snapshot, so the proposed authoritative restore contract cannot reconstruct the state required by the death-drop consumer.

[major] Require authorization for forced session termination

docs/plan-refactor-server-session-v1.md:121 · security

The new plan requires an “管理员/维护命令” to terminate any suspended session early, and specifies only CAS/registry locking for races. It never requires authentication/authorization of the command executor, defines no trusted operator identity source, and includes no ordinary-player rejection test in the P1/P4 acceptance list. This is not supplied implicitly by the existing command framework: commands registered with add_command are client-invokable, while the existing privileged season command performs an explicit SeasonCommandPermissions::is_allowed(username) check in server/src/cmd/dev/season.rs:153-165. A regular connected player able to invoke the planned maintenance command could therefore select another player's SessionKey, force SuspensionExpired settlement, release their facility claim, and prevent normal resume, while an implementation would still satisfy every acceptance item written here.

Root cause: the plan introduces a destructive cross-player administrative entry point but treats concurrency/idempotency as its only trust boundary; it omits a mandatory operator permission check, authoritative principal, target authorization rules, and negative permission coverage.

[major] Bound failed reclamation retries and retained expired sessions

docs/plan-refactor-server-session-v1.md:122 · performance

A checkpointed session reaches this path by disconnecting, remaining abandoned until the 1,728,000-tick TTL expires, and then having InventoryTxn::deliver repeatedly fail or remain unavailable. The new contract requires the session to remain in AwaitingDelivery with its claim and requires delivery-failure retries (lines 122-125), but defines no retry cadence/backoff, maximum age, operator dead-letter path, or other bound. Each newly expired session can therefore add another permanently retained registry/checkpoint entry and facility claim, while periodic reclamation continues to encounter failed work. The idempotency requirement prevents duplicate refunds but does not bound retained state or retry work. This behavior is introduced by the new session plan and is not constrained by the R10 plan, which only specifies idempotency and failure-not-committed semantics.

Root cause: the reclamation contract makes successful inventory delivery the only way to release an expired session, while requiring failed deliveries to remain retryable indefinitely without specifying any resource or retry bound. a sustained delivery failure therefore causes unbounded accumulation of session/checkpoint state and permanent facility claims, with potentially unbounded repeated scan work.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md (1)

26-26: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

在 skeleton 末尾补齐 §10 实施工作流

该计划包含 P0-P5 多阶段交付,但文件末尾没有依赖顺序、每 PR 单 plan、中文 atomic commit、Model: trailer、重新 review 和自动归档规则。

请追加 §10,并包含“单次 consume-plan 全自动到 merge”章节。

As per coding guidelines,多 PR plan 必须在末尾包含 §10 实施工作流。Based on learnings,skeleton plan 的实施和归档还应保持每个 PR 只修改一个 plan。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md` at line 26, 在计划文档末尾新增“§10
实施工作流”,补充 P0-P5 的依赖执行顺序、每个 PR 仅修改一个 plan、使用中文 atomic commit 并包含 Model:
trailer、重新 review,以及自动归档规则;同时加入“单次 consume-plan 全自动到 merge”章节,确保实施与归档遵循每个 PR
只修改一个 plan。

Sources: Coding guidelines, Learnings

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/plan-refactor-server-session-v1.md`:
- Around line 171-185: 在“## 3.1 决议”之前恢复“## 3 开放问题”章节,逐项列出与各项决议对应的 P0
问题并标记为已解决;保留现有“决议 1”和“决议 2”的结论、实施方案、边界与拒绝理由,以及“文件:行号 + plan
章节”双锚点,并确保章节结构符合开放问题先于对应 §N.1 决议的约定。
- Line 266: 在计划文件末尾新增“§10 实施工作流”,覆盖 P0-P4 的依赖与执行顺序、每个 PR 单独维护 plan、中文 atomic
commit 及 Model: trailer 要求、重新 review 流程和自动归档规则;同时加入“单次 consume-plan 全自动到
merge”章节,明确从执行计划到合并的完整自动化流程。
- Around line 255-266: 为 P4 中的每个场景补充可核验的实现锚点,逐项关联具体模块路径、类型或函数、schema、Redis
key、跨仓库契约 symbol 及对应 bot 测试脚本;至少覆盖 SessionRegistry、SessionDeliveryOutbox、R3/R10
事务 symbol 和现有回归脚本。保持现有验收语义不变,确保每项都能直接定位实现、契约与测试。
- Around line 1-3: 统一补齐四份计划的 plan header 接入面规范:在
docs/plan-refactor-server-session-v1.md
第1-3行新增完整“接入面”章节,明确进料、出料、共享类型/event/schema、跨仓库契约及 worldview.md 锚点;在
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
第17行、docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第17行和
docs/plans-skeleton/plan-refactor-client-ui-base-v1.md 第19行补充 worldview.md
章节及准确行号。

In `@docs/plans-skeleton/plan-refactor-c2s-gate-v1.md`:
- Line 25: 在文档末尾追加“§10 实施工作流”,补充 P0-P4 的依赖顺序、每个 PR 仅修改一个 plan、使用中文 atomic
commit、包含 Model: trailer、重新 review 以及自动归档规则;同时新增“单次 consume-plan 全自动到
merge”章节,明确从消费计划到合并的完整自动化流程。

In `@docs/plans-skeleton/plan-refactor-client-ui-base-v1.md`:
- Line 28: 在计划文档末尾补充“§10 实施工作流”,新增“单次 consume-plan 全自动到 merge”章节;明确 P0-P5
的依赖执行顺序、每个 PR 仅修改一个 plan、使用中文 atomic commit 并包含 Model: trailer、重新
review,以及合并后的自动归档规则。
- Around line 19-25: 更新 CraftScreen 的关闭处理,不要在 removed() 中直接将所有 Screen removal
映射为 CraftPause;引入明确的关闭原因或抑制标记,仅用户主动关闭时发送 CraftPause,显式取消路径抑制
Pause,断线或服务端替换路径不发送任何 UI intent并交由 R2/R1 生命周期处理。为用户关闭、取消、断线/服务端替换三条路径分别补充客户端测试。

In `@docs/plans-skeleton/plan-refactor-inventory-core-v1.md`:
- Around line 15-23: 更新 InventoryTxn 的 deliver 幂等契约及 durable
DeliveryCommitReceipt:持久化规范化 payload 摘要与目标身份,并以 delivery_id 强制唯一;delivery_id
已存在且摘要或目标不匹配时返回冲突并完全不执行 mutation,匹配时仅返回原始持久化 receipt,不重复处理。

In `@docs/plans-skeleton/plan-refactor-master-v1.md`:
- Around line 54-55: 为三个计划文件新增严格命名的“§10 实施工作流”,并在各自末尾按依赖顺序序列化
PR:docs/plans-skeleton/plan-refactor-master-v1.md(54-55)覆盖
R3、R1、R10、R6、R4、R7、R2;docs/plans-skeleton/plan-refactor-persistence-slices-v1.md(23-27)覆盖
Slice 框架、guard、flush、TsyPresence parity、outbox 和 bot
验收;docs/plans-skeleton/plan-refactor-inventory-core-v1.md(36-45)覆盖
InventoryTxn、生产调用点迁移、outbox 消费和 DeadLetter 验收。将现有 flash-review
流程顺延至后续章节,并确保每个计划在一个 §10 内完成多 PR 编排。

In `@docs/plans-skeleton/plan-refactor-persistence-slices-v1.md`:
- Around line 47-49: Update the TsyPresence recovery flow and its acceptance
criteria so a missing or invalid presence slice also prevents restoring TSY
position and dimension in the same recovery transaction, or moves them to a
separately trusted safe location and dimension. Preserve the existing
refusal-to-attach and request rejection behavior, and never derive the fallback
destination from corrupted return_to data.

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Line 17: 在跨仓库契约说明中为 Craft 协议切换补充最小协议版本门、feature gate 与原子发布要求,明确未满足版本时拒绝新
intent 和 session-state payload。增加并说明旧 client/new server 及 new client/旧 server
两种组合的拒绝测试,确保 rollout 期间不会写入或解析不兼容的 enum/payload;保留现有一次性切换和 CraftCancel 约束。

---

Outside diff comments:
In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Line 26: 在计划文档末尾新增“§10 实施工作流”,补充 P0-P5 的依赖执行顺序、每个 PR 仅修改一个 plan、使用中文 atomic
commit 并包含 Model: trailer、重新 review,以及自动归档规则;同时加入“单次 consume-plan 全自动到
merge”章节,确保实施与归档遵循每个 PR 只修改一个 plan。
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 00ad1f54-e9bd-4bf9-a618-3aa436e70ff7

📥 Commits

Reviewing files that changed from the base of the PR and between 3dfa7a0 and 5e96338.

📒 Files selected for processing (9)
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
💤 Files with no reviewable changes (1)
  • docs/plans-skeleton/plan-refactor-server-session-v1.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
docs/**/*.md

📄 CodeRabbit inference engine (docs/CLAUDE.md)

docs/**/*.md: 新 plan 头部必须写明接入面:进料、出料、复用的共享类型/event/schema、server/agent/client 跨仓库契约,以及对应的 worldview.md 锚点。
涉及真元、灵气、衰减、逸散、半衰、距离损耗、排斥或吸力的 plan,必须调用 qi_physics;新物理常数和公式必须先扩展 qi_physics,不得在功能 plan 中重复实现。
所有真元/灵气流动必须遵守守恒律并通过 qi_physics::ledger::QiTransfer;释放使用 qi_release_to_zone,吸收使用 qi_excretion,不得凭空生成或销毁真元。
涉及玩家可感知行为的 plan,必须在对应机制阶段中内联可直接实现的粒子、音效、HUD、环境、动画和 narration 规格;不得将视听内容笼统推迟到独立阶段。纯 server 逻辑 plan 例外。
每份 plan 必须列出开放问题;实施前必须追加 §N.1 决议,逐项给出结论、实施方案、边界条件,并以文件:行号和 plan 章节双锚点落地。
scope 大于或等于 4 个 PR 的 plan,必须在末尾包含 §10 实施工作流,并按依赖顺序在一个 plan 内序列化多个 PR,不得拆成多个 plan。
涉及 NBT 建筑、worldgen layout 或复杂视觉资产的 TODO,必须完成三轮提交:(round 1/3)(round 2/3)(round 3/3);终轮提交必须包含拼写准确的 <PROMISE> 担保块。
§10 最末必须包含“单次 consume-plan 全自动到 merge”章节,明确用户提交 /consume-plan 后即可等待最终归档至 docs/finished_plans/

一个 PR 只允许修改一个 plan;/consume-plan 只能追加 Finish Evidence 或执行允许的 plan 归档移动,不得自动修改其他 docs 文件、CLAUDE.md 或 worldview 文档。

Files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plan-refactor-server-session-v1.md
docs/plans-skeleton/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

新建 plan 前必须先阅读 docs/CLAUDE.md;骨架 plan 必须包含主题和 P0/P1 等阶段总览。

Files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
**/*.{rs,ts,tsx,java,json,md}

📄 CodeRabbit inference engine (CLAUDE.md)

唯一真货币是骨币;矿物是交易筹码,灵石是燃料/衰变物,金银不是货币。

Files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plan-refactor-server-session-v1.md
**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: 禁止 git stash push 后不执行对应 git stash pop;不得留下孤儿 WIP stash。
每个逻辑单元使用中文 atomic commit;agent 产生的每个 commit 必须包含真实模型 ID 的 Model: trailer。
未经明确确认不得执行 force push、hard reset、amend、交互式 rebase、批量删除/移动文件或依赖版本/生产配置修改;严禁 --no-verify--no-gpg-sign 及关闭签名。
PR review 只能通过独立评论 /review 触发;不得等待 Codex,review 修改后必须重新等待 re-review。

Files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plan-refactor-server-session-v1.md
docs/plan-*.md

📄 CodeRabbit inference engine (CLAUDE.md)

docs/plan-*.md: Active plan 每个阶段必须写出可核验的模块路径、类型/函数、测试、schema、Redis key 或跨仓库契约 symbol。
plan 归档前必须所有阶段标记为 ✅ YYYY-MM-DD 并补充严格标题为 ## Finish Evidence 的证据章节。

Files:

  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plan-refactor-server-session-v1.md
🧠 Learnings (3)
📚 Learning: 2026-07-17T00:31:10.779Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:1-1
Timestamp: 2026-07-17T00:31:10.779Z
Learning: 在审查该仓库 `docs/plans-skeleton/` 下的“docs-only skeleton plan”创建类 PR 时:先核对 `docs/CLAUDE.md` 中“Plan 消费规范”,并逐份查看本计划文档里的“§10 实施工作流”,确认后续实施阶段是否会遵循“每个 PR 只修改一个 plan”,且实施/归档时不会出现跨 plan 的修改。该规则不适用于用户显式指定、共享调研基线且计划之间存在互相交叉引用的 skeleton plan 同批创建 PR;对这类情况应按实际交叉引用关系放宽,确保仍能按独立或约定的序列化方式推进。

Applied to files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
📚 Learning: 2026-07-17T00:31:13.643Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:81-85
Timestamp: 2026-07-17T00:31:13.643Z
Learning: 审核 docs/plans-skeleton/*.md 下的 skeleton 草案 PR 时:不得要求作者在“§N 开放问题(P0 决策门前需收口)”核查完成之前提前填写对应的“§N.1 决议”。仅当计划进入 active 且 P0 实施前,已由 Explore agent 并行核查代码现状后,才允许追加“§N.1 决议”,且该决议需包含结论、实施方案、边界条件,并使用“文件:行号 + plan 章节”的双锚点格式。

Applied to files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
📚 Learning: 2026-07-22T02:11:59.191Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1247
File: docs/plans-skeleton/plan-bughunt-animal-air-spawn-gravity-v1.md:0-0
Timestamp: 2026-07-22T02:11:59.191Z
Learning: 在 `docs/plans-skeleton/` 目录下的计划状态行中,若起草日期同时涉及 UTC 与本地日期(例如时区换算后可能跨到不同日期),请在该行中同时标注 UTC 起草日与本地起草日。这样可以避免基于 UTC 的时间基准在 GitHub/CodeRabbit 审查时将本地日期误判为“未来日期”。

Applied to files:

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
🪛 LanguageTool
docs/plan-refactor-server-session-v1.md

[grammar] ~96-~96: Ensure spelling is correct
Context: ...connected_clients及 R3 player save 前接收 disconnect。 2.Checkpointed`:停止 tick → 写 checkpoint(包含 escrow,但不执行 refu...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~242-~242: Ensure spelling is correct
Context: ... volatile adapter;断线/跨维/关服不再 tick 或结算离线 actor。 - qi refund/release 测试从 SPIRIT_QI_TOTAL 与 ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 markdownlint-cli2 (0.23.1)
docs/plan-refactor-server-session-v1.md

[warning] 132-132: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above

(MD022, blanks-around-headings)

🔇 Additional comments (2)
docs/plan-refactor-client-store-lifecycle-v1.md (1)

27-27: LGTM!

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md (1)

16-19: 🗄️ Data Integrity & Integration

无需修改。 恢复失败的 release_dormant_qi_to_zone 会先设置 NPC/zone shadow balance,再调用 qi_release_to_zone 构造 QiTransfer(ReleaseToZone) 并写入 ledger。

Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plan-refactor-server-session-v1.md Outdated
9. `session_craft_pause_resume_wire`:真实 client producer → R6 proto/bridge → R4 production handler/gate → R1 registry → S2C hydrate → client reopen consumer 全链路。
10. `session_tsy_presence_relog`:R3 guarded presence Slice 完整恢复 `family_id`、`entered_at_tick`、`entry_inventory_snapshot`、`return_to`、schema/version 并与 position/dimension 同事务对拍;校验失败不 attach、不开放 TSY 请求,恢复后 death-drop 仍正确区分原带物与 TSY 所得。

另回归现有 `production_craft_disconnect_resume.py`、`production_craft_cancel_full_inventory_refund.py`、`production_handcraft_stone_knife.py`。对应 implementation PR 合入后,按总纲 §7 为吸收项补 Finish Evidence 并做每轨一次 docs-only 批量归档。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

在计划末尾补齐 §10 实施工作流

该计划包含 P0-P4 多阶段交付,但文件在此结束,没有依赖顺序、每 PR 单 plan、中文 atomic commit、Model: trailer、重新 review 和自动归档规则。

请在最后追加 §10,并包含“单次 consume-plan 全自动到 merge”章节。

As per coding guidelines,多 PR plan 必须在末尾包含 §10 实施工作流

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 266, 在计划文件末尾新增“§10 实施工作流”,覆盖
P0-P4 的依赖与执行顺序、每个 PR 单独维护 plan、中文 atomic commit 及 Model: trailer 要求、重新 review
流程和自动归档规则;同时加入“单次 consume-plan 全自动到 merge”章节,明确从执行计划到合并的完整自动化流程。

Source: Coding guidelines

- ⬜ P1 门禁中间件落地:gate 层上线,先给"已知漏洞簇"与 R1 craft `CraftOpen`/`CraftPause`/`CraftResume` 挂 spec;新增 intent 必须在同一提交进入 production decode/dispatch 和全量 gate matrix,旧内联校验保留并行断言一个版本期。
- ⬜ P2 巨石拆分批次 A:巨型 match 拆为按域 handler 注册表(combat/production/world/social/npc 五组),行为不变,bot 场景锁住。
- ⬜ P3 巨石拆分批次 B + 全量挂 spec + 删旧:113 变体全部声明门禁(含显式 `no_gate` 声明,杜绝静默无门禁);删除各域内联距离常量与重复维度判断。
- ⬜ P4 bot 验收 + 吸收 plan 批量归档。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

在 skeleton 末尾补齐 §10 实施工作流

该计划包含 P0-P4 多阶段交付,但文件末尾没有依赖顺序、每 PR 单 plan、中文 atomic commit、Model: trailer、重新 review 和自动归档规则。

请追加 §10,并包含“单次 consume-plan 全自动到 merge”章节。

As per coding guidelines,多 PR plan 必须在末尾包含 §10 实施工作流。Based on learnings,skeleton plan 的实施和归档还应保持每个 PR 只修改一个 plan。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-c2s-gate-v1.md` at line 25, 在文档末尾追加“§10
实施工作流”,补充 P0-P4 的依赖顺序、每个 PR 仅修改一个 plan、使用中文 atomic commit、包含 Model: trailer、重新
review 以及自动归档规则;同时新增“单次 consume-plan 全自动到 merge”章节,明确从消费计划到合并的完整自动化流程。

Sources: Coding guidelines, Learnings

Comment thread docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
Comment thread docs/plans-skeleton/plan-refactor-inventory-core-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-master-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-persistence-slices-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-wire-s2c-v1.md Outdated
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: 5e96338c1428372f11173853c508260faa8d42f9
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] R3 P1 gate can activate sessions before atomic outbox handoff exists

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:25 · wiring

The changed plan assigns the SessionDeliveryOutbox table and an “atomic handoff API” to P1 at line 23, but defers the actual guarantee that checkpoint terminalization/deletion and outbox insertion are committed in one all-or-nothing transaction to P3 at line 25. That staging conflicts with the production dependency chain: docs/plans-skeleton/plan-refactor-master-v1.md:55 allows the R1 craft host migration after R3 P1, and the promoted R1 plan at docs/plan-refactor-server-session-v1.md:224 likewise treats R3 P1 as providing the durable outbox/terminal-checkpoint transaction. Therefore, after R3 P1 but before R3 P3, the documented gates permit craft production wiring even though the transaction that owns the delivery payload is not yet a required deliverable. A crash between checkpoint terminalization and outbox insertion on that reachable completion path can leave neither a resumable checkpoint nor a delivery obligation (lost output/refund), while the existing tests do not prevent this because crash-boundary injection is also assigned to P3.

Root cause: the plan splits one indivisible production dependency across phases: downstream plans gate session activation on r3 p1, while the durable checkpoint-to-outbox ownership transfer and its crash pins are scheduled for r3 p3. the atomic transaction must be an explicit p1 acceptance requirement, or every downstream production gate must wait for p3.

[major] Reserve outbox byte capacity, not only a row slot

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:15 · concurrency-atomicity

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:15 requires each checkpointed session to reserve only a terminal-obligation "slot", while independently imposing both row and byte quotas on SessionDeliveryOutbox, and then relies on that reservation to make terminal handoff safe. A reachable counterexample is a session admitted while one row slot is available whose escrow/output grows before termination, or whose eventual serialized payload is simply larger than the remaining byte budget. The row slot remains reserved, but the atomic outbox insert can still fail the byte quota. Because the same contract requires checkpoint terminalization and outbox insertion to succeed together, the session cannot terminalize and its facility/target claim remains retained. Neither this plan nor the promoted R1 contract defines a maximum payload size, reserves byte capacity, or permits a lossless overflow store, so the row reservation does not prevent this state. The new atomicity scenario at line 48 only injects crashes around a handoff that can already be inserted; it does not exercise byte-quota exhaustion after admission.

Root cause: the admission reservation and the resource constrained at terminal handoff use different units: one row slot is reserved, but terminalization also requires unreserved byte capacity for a variable-size payload. consequently, admission does not guarantee that the later atomic handoff can commit, despite the plan relying on that guarantee to release gameplay claims.

[major] Assign a production client event to CraftOpen

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19 · correctness

The changed contract adds CraftOpen alongside CraftPause and CraftResume, but the client production mapping only defines close -> CraftPause, explicit cancel -> CraftCancel, and reopen-after-hydration -> CraftResume. It never assigns any client action to emit CraftOpen; the stated client test likewise covers only those three mappings. The server-session plan requires the real client producer through R6/R4/R1 and forbids mocks from substituting for missing production wiring, so an initial open cannot reach the new open handler under the documented implementation plan.

Root cause: the cross-stack plan introduces a new c2s craftopen intent but omits its production producer and corresponding client contract test, leaving one member of the newly frozen wire contract disconnected while the phase is described as completing craft production wiring.

[major] Add regression coverage for the PendingInsightOffer deadline lifecycle

docs/plan-refactor-server-session-v1.md:144 · testing

The new durability matrix defines PendingInsightOffer as Volatile + deadline and requires disconnect/timeout cleanup (line 144), but the required P1 pins at lines 236-237 cover session phases, termination causes, delivery, and suspension only, while the complete P4 acceptance list at lines 255-264 contains no insight-offer timeout scenario. An implementation that removes the offer on disconnect but never processes its deadline would satisfy every listed acceptance test while leaving the newly identified same-connection stranded-offer defect reachable.

Root cause: the plan adds a timed lifecycle contract for pendinginsightoffer without adding positive, timeout-boundary/off-by-one, response-versus-timeout race, and teardown regression pins for that state. the plan therefore does not provide the saturated acceptance evidence required for this changed state-machine behavior.

[major] Outbox handoff terminalizes the session before the receipt-gated terminal transition

docs/plan-refactor-server-session-v1.md:88 · strict-maintainability

The fallback path says R3 writes the outbox and "terminalize session" in the checkpoint transaction, then R10 consumes it, but the same invariant says CommitTerminal is allowed only after the delivery receipt is durable (docs/plan-refactor-server-session-v1.md:88). The next invariant requires the session to be removed from the live registry immediately after that outbox transaction (:89), and the expiration flow repeats terminalization/removal before R10 delivery (:126). A delivery that is delayed or reaches DeadLetter therefore leaves no live, non-terminal session on which the receipt-gated CommitTerminal transition can occur. Conversely, treating the outbox handoff as CommitTerminal violates the stated receipt gate. The phase list provides only one Terminal state (:72), so these cannot be interpreted as two named terminalization stages.

Root cause: the plan assigns two incompatible meanings and times to session terminalization: durable outbox handoff terminalizes and removes the session before delivery, while committerminal is specified as a later transition gated by r10's receipt. this duplicates the canonical terminal-state decision across r1/r3 and r10 without representing the intermediate durable-handoff state, making the required lifecycle impossible to implement consistently.

[major] Gate completeness contract still stops at 113 after adding three intents

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:21 · schema-contracts

This changed P1 contract adds three new ClientRequestV1 variants (CraftOpen, CraftPause, and CraftResume) and requires them to enter the production decoder and gate matrix, while the same plan still defines P0/P3 and gate_matrix_sweep as covering exactly 113 variants (lines 20, 23, and 43). The repository's current 113-variant baseline already includes the existing craft requests (CraftStart and CraftCancel in server/src/schema/client_request.rs), so adding these three variants makes the new total 116. A parameterized sweep that stops after 113 entries can remain green while three requests lack a GateSpec/no_gate declaration or decode coverage; the changed sentence's claim of a “full” gate matrix does not prevent that because the acceptance count remains stale, and the same stale 113 assertion is repeated in the master and wire plans.

Root cause: the plan extends the c2s union by three variants without updating the fixed cardinality used as the completeness contract and test oracle from 113 to 116 (or replacing it with a count derived from the schema).

[major] Pin every TsyPresence guarded-restore rejection branch

docs/plan-refactor-server-session-v1.md:264 · testing

The restore contract requires independent facility/target existence, owner, dimension, and version validation (lines 115-117), and TsyPresence specifically requires identity/version/dimension validation before attaching or opening TSY requests (line 249). The only planned e2e assertion says generically that "validation failure" must not attach (line 264). A test using only a version-mismatch fixture would pass even if owner identity or dimension checks were omitted, allowing a stale or wrong-player presence to attach in production.

Root cause: the tsy restore acceptance test collapses several security- and state-relevant rejection branches into one unspecified failure case instead of pinning each guarded-load predicate independently, so concrete incorrect implementations of the unexercised branches remain accepted by the planned suite.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: c0de0a601594c7311b4298e0673ff566bb7ae747
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Craft reopen pin lacks the no-session negative case

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:43 · testing

The new client contract says reopening must use server-hydrated session state to decide whether to send CraftResume (line 19), while the promised producer pins reduce this to the positive mapping reopen hydrate -> CraftResume (lines 25 and 43). A client implementation that sends CraftResume on every reopen, including an idle/no-session hydration, can still pass all four listed intent mapping tests as long as the positive paused-session case emits Resume. The R4 server gate may reject that packet, but it does not verify the changed client producer contract and can expose a user-visible rejection on an ordinary idle reopen. No negative idle, terminal, missing, or stale hydration case is listed. This test gap is introduced by the diff's new state-dependent reopen behavior.

Root cause: the planned client tests identify which intent is emitted on the happy paths but do not pin the authoritative hydration precondition under which craftresume must not be emitted.

[major] Do not document the three craft intents as current 116-variant production coverage

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:7 · strict-maintainability

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:7 now states that the current ClientRequestV1 match covers 116 variants, and lines 3/17/21 similarly treat 116 as the current full enum. The repository's actual Rust enum still exposes only the existing CraftStart and CraftCancel craft variants at server/src/schema/client_request.rs:658-672; the protobuf bridge likewise only defines those two messages at proto/bong/envelope.proto:1303-1312, and repository searches find no CraftOpen, CraftPause, or CraftResume symbols. Those three variants are described elsewhere in this diff as future R6 deliverables, so the current count remains 113 until that implementation lands. This makes the plan's current-state evidence, 116-variant gate sweep, and completion accounting factually wrong and can cause reviewers/implementers to treat three disconnected intents as already present.

Root cause: the documentation updates the baseline enum count and current production coverage without updating the producer, protobuf schema, rust conversion, and handler symbols that would make the three new intents reachable. planned future additions are being represented as if they already exist in the current contract.

[major] Offline clients can spoof an allow-listed maintenance operator

docs/plan-refactor-server-session-v1.md:78 · security

The new contract makes SessionMaintenancePermissions trust an allow-list keyed only by the executor entity's Username, and §2.2.1 uses that check to authorize session-maintenance terminate <SessionKey> plus dead-letter inspect/retry/resolve. Bong explicitly runs with ConnectionMode::Offline (docs/roadmap.md:131, docs/local-test-env.md:167), where the server does not authenticate Minecraft identities, so a connecting client can select an allow-listed name such as the repository's existing default Admin/admin pattern (server/src/cmd/dev/season.rs:24-31). That client then has an authoritative Username component and passes the proposed check; target/owner mismatch checks do not help because operators are intentionally allowed to act across owners. The observable result is unauthorized terminalization of another player's suspended session, claim release, and access to delivery-maintenance operations. This is introduced by the new plan's frozen authorization contract and is not mitigated by its tests, which only distinguish allowed usernames from ordinary/unknown executors rather than authenticated from spoofed identities.

Root cause: the plan treats an unauthenticated, client-selected offline-mode username as an operator identity. authorization must instead derive from an identity or capability the client cannot choose (or the maintenance surface must be unavailable in offline mode), with the executor entity used only to bind that authenticated principal.

[major] Dead-letter tests allow resolve to discard undelivered payloads

docs/plans-skeleton/plan-refactor-inventory-core-v1.md:42 · testing

The changed contract says an authorized resolve must not delete an undelivered payload (line 23), but inv_delivery_dead_letter only checks retry timing, transition to DeadLetter, ordinary-player authorization, and claim release. An implementation in which an authorized operator's resolve command simply deletes the dead-letter row and its payload would pass every listed R10 bot scenario, including the crash-atomicity test, while permanently losing the retained delivery obligation. The server-session plan's SuspensionPolicy pins likewise mention only manual retry/resolve permissions, not payload preservation, and the R3 outbox test stops at handoff rather than exercising resolve. This omission is introduced with the new resolve contract in this diff.

Root cause: the plan adds a data-conservation requirement for dead-letter resolution but provides no acceptance assertion for the payload's durable disposition after an authorized resolve.

[major] Add the three craft intents to the TypeBox source-of-truth deliverable

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 · schema-contracts

The changed P1 deliverable says the three new C2S intents will be added to proto/schema/proto_convert and line 16 characterizes agent TypeScript as only passively regenerated. However, Bong's TypeBox IPC source is agent/packages/schema/src/client-request.ts, whose ClientRequestV1 union (around line 1136) currently contains neither the existing craft requests nor any CraftOpen/CraftPause/CraftResume variants. The plan also does not require rebuilding and committing the generated JSON Schema/distribution consumed through @bong/schema. Following this phase literally can therefore produce protobuf/Rust support and make the new client/server path pass its proto samples while the canonical TypeBox schema rejects or cannot describe the same three wire variants; neither the later 116-variant proto sample sweep nor R4's Rust decoder/gate work exercises that omitted artifact boundary.

Root cause: the plan treats protobuf/rust conversion as the defining c2s contract and relegates the agent schema to passive regeneration, instead of explicitly changing the typebox source of truth and propagating it through generated json schema, the packaged distribution, rust mirrors/converters, and consumers in one contract change.

[major] Serialize delivery, dead-letter, and operator outbox transitions

docs/plans-skeleton/plan-refactor-inventory-core-v1.md:23 · concurrency-atomicity

The changed P2 contract adds automatic retry, threshold-based transition to DeadLetter, and operator retry/resolve, but defines no lease, compare-and-swap, or transactional state precondition between those actors. At the exact attempts/age boundary, an automatic consumer can be processing a Pending row while the scanner marks it DeadLetter; an operator can likewise retry or resolve the same row concurrently. The listed tests cover crash boundaries and sequential threshold/authorization behavior, but no worker-vs-dead-letter or worker-vs-operator race. A conforming implementation could therefore commit inventory plus a receipt while another writer leaves the outbox in DeadLetter, retaining quota and reporting an undelivered obligation after delivery; alternatively an unconditional acknowledgement could overwrite an operator transition. The stable receipt prevents duplicate inventory mutation, but it does not make the outbox state/quota transition itself atomic.

Root cause: the plan assigns several concurrent writers to deliveryoutboxstate without freezing a single atomic transition protocol (for example, a claimed/in-flight state plus cas on expected state/generation) or requiring race tests. session expiration/reconnect races are explicitly serialized elsewhere, but equivalent serialization is absent for delivery retry, dead-lettering, and operator maintenance.

[major] Suspension lease rebasing has no restart regression pin

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:16 · testing

The new contract at line 16 requires tick_rebase to preserve a suspended session's remaining TTL across restarts and explicitly forbids turning restarts into an unlimited renewal. However, the only tick_rebase acceptance scenario (line 46) exercises cooldown/regeneration timers, while session_suspension_reclamation in docs/plan-refactor-server-session-v1.md covers TTL boundaries and lifecycle races without a restart/rebase case. An implementation that resets suspended_at_tick to the current tick on every guarded load would pass all named scenarios, yet a player could keep a facility claim indefinitely by repeatedly restarting before expiry.

Root cause: the plan adds a restart-sensitive suspension-lease invariant but does not add a regression test that persists a partially elapsed lease, restarts, rebases ticks, and verifies expiry after only the original remaining duration (including repeated restarts and boundary ticks).

[major] Gate sweep does not test the new craft authorization states

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:44 · testing

The diff adds CraftOpen, CraftPause, and CraftResume and explicitly requires owner, phase, and busy gates to reject forged resumes and duplicate packets (lines 17 and 22). However, the only named gate-matrix acceptance test still exercises just legal, out-of-range, and cross-dimension inputs. An implementation that registers all three variants and applies distance/dimension checks but accepts CraftResume from a non-owner, in a non-paused phase, or while a conflicting busy claim exists would satisfy the described gate_matrix_sweep and production registration checks. No other listed test names those negative request states; the session plan's generic transition/busy unit pins do not exercise the R4 production decoder and gate path. This gap is introduced by adding the three stateful intents without extending the production gate test dimensions that cover them.

Root cause: the acceptance matrix was updated only in variant count, not in input dimensions, so it does not verify the newly claimed owner/phase/busy authorization contract for craft lifecycle requests.

[major] TSY shutdown test does not prove atomic location and presence persistence

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:48 · testing

Line 26 newly requires the TsyPresence, position, and dimension shutdown persistence to have a fixed same-transaction ordering. The added tsy_presence_shutdown_order scenario at line 48 only says the session registry is stopped before flushing and that position/dimension/presence are compared in the same restore transaction; it does not inject a shutdown crash between those writes or assert that the three writes committed atomically. An implementation that commits presence first and position/dimension in a later transaction would satisfy the listed test on a clean shutdown, but a crash between commits would restore a TSY presence against stale location state.

Root cause: the acceptance scenario verifies restore-time consistency after an orderly flush, but omits the crash boundary needed to protect the newly claimed atomic shutdown-write contract.

[major] No track owns the client producer APIs for the three new craft intents

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19 · wiring

The changed client plan requires CraftScreen to send CraftOpen, CraftPause, and CraftResume, while its ownership boundary says R7 does not own client/network/ and only consumes R6 contracts (docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:36-39). The master likewise assigns client network code to R6 (docs/plans-skeleton/plan-refactor-master-v1.md:62). However, R6 P1 only assigns the new intents' proto/schema/proto_convert and S2C sample pins (docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22), not the client JSON encoder/sender APIs. Those APIs are required on the real production path: today ClientRequestProtocol only exposes encodeCraftStart and encodeCraftCancel (client/src/main/java/com/bong/client/network/ClientRequestProtocol.java:1382-1406), and ClientRequestSender only exposes the corresponding send methods (client/src/main/java/com/bong/client/network/ClientRequestSender.java:557-569). Following the stated ownership and phase deliverables therefore leaves R7 with no production method through which its Screen can emit any of the three new intents; the proposed close/reopen chain cannot be completed even after all listed gates are checked.

Root cause: the cross-track contract allocates the ui call sites to r7 and client network files to r6, but no r6 deliverable owns the required client-side c2s encoding and sender methods. add those production producer apis and their contract pins to r6's frozen interface before r7 consumes them.

@Kizunad
Kizunad force-pushed the refactor/plan-refactor-server-session-v1 branch from c0de0a6 to 9821f72 Compare August 3, 2026 07:45
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: 9821f728d21dbf1b8ce164f63c744116ac7554bd
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Frozen outbox schema omits required CAS and disposition data

docs/plan-refactor-server-session-v1.md:76 · schema-contracts

The frozen SessionDeliveryOutbox shape lists delivery_id, session_key, owner_key, payload_digest, payload, cause, state, attempts, next_retry_tick, created_at_tick but omits the generation that line 77 and the R3/R10 contracts require to be durable and used in every (state, generation) CAS. The adjacent frozen enum also declares ResolvedDisposition as a unit variant, while line 128 later requires the persisted state to carry { delivery_id, operator_principal, reason, payload }. P1 explicitly requires implementations to expose the frozen symbols, so an implementation conforming to these exact declarations cannot persist the CAS token or the payload-retaining resolve disposition, and the later prose/tests do not repair the type-level contract.

Root cause: the plan added generation-based concurrency and payload-retaining resolution semantics without updating the authoritative frozen outbox and enum shapes to contain the data those semantics require.

[major] Make terminal-obligation quota reservation atomic across concurrent admissions

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:16 · concurrency-atomicity

The changed contract says concurrent checkpointed-session admission/restore must first reserve one outbox row and SESSION_DELIVERY_MAX_PAYLOAD_BYTES, and must reject work when row/byte quota is full. However, it defines no transactional/CAS operation that atomically checks quota and records that reservation. P1's persisted outbox fields cover only delivery rows (delivery_id, payload, state, generation, lease), and the new session_delivery_outbox_atomicity acceptance case tests crash points and a single admitted session at the payload boundary, but not two admissions/restores racing for the last row or bytes. Thus an implementation that independently reads remaining quota and then records reservations would satisfy every listed pin while two concurrent callers both succeed, overcommit the configured bound, and later leave one terminal handoff unable to persist after that session has already accepted escrow/output.

Root cause: the new quota-reservation contract specifies capacity accounting as a precondition but does not make reservation acquisition/release an atomic persisted state transition or test the concurrent admission/restore race. the outbox generation cas described elsewhere only serializes transitions of an already-created delivery row, so it does not prevent quota check-then-reserve overbooking.

[major] C2S reconnaissance claims future craft variants are already wired

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:6 · wiring

The changed baseline statement says the current handle_client_request_payloads match covers 116 variants (docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:6), but the same document says the current baseline is 113 and only expands to 116 after R6 adds the three craft intents (docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:2,20,43). The master completion contract likewise requires 116 only after those intents are wired (docs/plans-skeleton/plan-refactor-master-v1.md:100). This makes the plan's production enum/dispatch inventory internally inconsistent: an implementation or gate sweep using the “current 116” evidence can treat three not-yet-produced requests as existing and omit verifying their later producer-to-decoder path.

Root cause: the reconnaissance evidence was updated from 113 to 116 without preserving the conditional nature of the future r6 variants. the same plan now simultaneously treats the three craft intents as present in the production handler and as future additions, so the end-to-end request inventory has no single authoritative baseline.

[major] Craft lifecycle plan leaves the required S2C hydration payload unwired

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · wiring

The changed contract says R6 will freeze a corresponding S2C craft session-state payload, while the real consumer contract requires server-hydrated phase plus session key and generation so the client can distinguish a matching paused session from idle, terminal, delivery-pending, stale, or mismatched state (docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19). However, the P1 deliverable at line 22 enumerates only the CraftOpen/CraftPause/CraftResume C2S TypeBox-to-schema/dist-to-proto/Rust chain and client send APIs; it neither specifies the required S2C fields nor assigns its TypeBox/schema, protobuf mirror/converter, server producer, client bridge/router consumer, samples, or contract pin. An implementation can therefore satisfy the newly written R6 P1 checklist with the three C2S intents while leaving the promised S2C hydration contract unchanged or incomplete, at which point R7 cannot safely decide whether to emit CraftResume. Existing dependency wording does not prevent this because it gates R1/R4 on the C2S intent pins only.

Root cause: the plan introduces an end-to-end craft lifecycle that depends on a new s2c session-state contract, but only wires and pins the c2s half of that contract. the s2c producer-to-schema-to-bridge-to-client-consumer chain and its required identity/version fields are left undefined, so the phase deliverable does not enforce the cross-layer connection required by the client behavior.

[major] Successful session completion has no representable outbox cause

docs/plan-refactor-server-session-v1.md:73 · correctness

The frozen contract defines TerminationCause with only VoluntaryCancel, Disconnect, DimensionChange, Shutdown, InvalidRestore, and SuspensionExpired, while the same changed section requires every normally completed session to enter AwaitingDelivery and defines SessionDeliveryOutbox with a mandatory cause field (lines 73, 76, and 88). A successful craft completion therefore reaches the required outbox handoff but has no representable cause in the frozen enum. Nothing surrounding this contract supplies a separate completion cause type or makes the field optional, and P1 is instructed not to change the frozen semantics.

Root cause: the frozen termination/cause model omits a successful completion variant even though normal completion is a required producer of the durable delivery outbox whose record includes a cause.

[major] Rebase durable outbox deadlines across restarts

docs/plan-refactor-server-session-v1.md:76 · concurrency-atomicity

The new durable outbox persists next_retry_tick and created_at_tick as runtime ticks and defines retry/dead-letter decisions from them, but gives these deadlines no restart conversion. The only explicit tick_rebase rule is for suspension leases (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:16,46); the outbox P1 contract merely reloads the raw timing fields (:24), and the new crash test checks delivery identity/count rather than post-restart retry timing (:49). A reachable example is an outbox row created after 100 days of server uptime: after restart resets the runtime clock near zero, its next_retry_tick remains near the old 100-day tick, so the first retry can be postponed by roughly 100 days rather than the specified 1,200 ticks. The same stale comparison can postpone the "7 days" DeadLetter bound. Repeated restarts can therefore keep failed obligations outside their promised bounded retry/reclamation schedule.

Root cause: the plan makes retry and age deadlines durable as absolute process-local ticks without defining an epoch, elapsed-duration representation, or mandatory tick_rebase behavior for outbox timing fields. persisting those values across a clock reset creates stale ordering/deadline reads even though the surrounding contract claims cross-restart retry and dead-letter guarantees.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: infrastructure_failure
Reviewed head: bd5f0eaefa02e2965e1a4562c27bd76c803cbb68
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

The review could not complete safely. No approval or code finding was inferred from the failed stages.

  • adjudicate:0e3e766d254dd6f29ba4875305c8e123d797024b1c475f521231f3f934c63233infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:22063c051ce265f479ba4ca97a75561b785515a6c0edf77ca08fe2d17b92e20a:1infra_error: could not collect 5 valid votes after 15 attempts
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:2ef9c4cce299b2f6b5408a9ef0d796cd82c69522dc944daa942c8f13b56133df:1infra_error: could not collect 5 valid votes after 15 attempts
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"system","subtype":"api_retry","attempt":1,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":2,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":3,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":4,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":5,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":6,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":7,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":8,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":9,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"system","subtype":"api_retry","attempt":1,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":2,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":3,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":4,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":5,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":6,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":7,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":8,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":9,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"system","subtype":"api_retry","attempt":1,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":2,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":3,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":4,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":5,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":6,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":7,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":8,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":9,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"system","subtype":"api_retry","attempt":1,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":2,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":3,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":4,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":5,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":6,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":7,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":8,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":9,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"system","subtype":"api_retry","attempt":1,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":2,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":3,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":4,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":5,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":6,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":7,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":8,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"system","subtype":"api_retry","attempt":9,"maxRetries":10,"errorStatus":503,"error":"server_error"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:349464154247a5be83d9a2eca15a2afa0c978760ce3c310f5551c65406114d50:1infra_error: could not collect 5 valid votes after 15 attempts
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:4dc86d9c31628345c602f81c906b5a2c232cf9615107e9b9986503036401e746:1infra_error: could not collect 5 valid votes after 15 attempts
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:86244d3d264da7245583e65dbcb678f1c029a052cfc90eec6f21796eb4c9c262:1infra_error: could not collect 5 valid votes after 15 attempts
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:0:attempt-1infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:1:attempt-2infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:2:attempt-3infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:3:attempt-4infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:4:attempt-5infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:0:attempt-6infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:1:attempt-7infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:2:attempt-8infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:3:attempt-9infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:4:attempt-10infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:0:attempt-11infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:1:attempt-12infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:2:attempt-13infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:3:attempt-14infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1:4:attempt-15infra_error: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}
  • validate:a0ea251757057a6c9b404600874c25589092ea4bed1de4a249b0cc32fc51d83b:1infra_error: could not collect 5 valid votes after 15 attempts

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: bd5f0eaefa02e2965e1a4562c27bd76c803cbb68
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Freeze session identity fields for the new craft lifecycle intents

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · schema-contracts

The changed contract declares CraftOpen, CraftPause, and CraftResume and enumerates every artifact that must mirror them, but never defines the payload fields of any of the three intents. This is not supplied elsewhere in the changed plans: docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:44 requires stale or mismatched session identity to be rejected, while docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19 requires Resume to be based on the hydrated session_key and generation. A conforming-looking implementation can therefore encode CraftResume as an empty message, as the existing CraftCancel is encoded, and pass the requested schema/protobuf round-trip and producer tests. On the reachable sequence old session generation N terminates, the same key is reused for paused generation N+1, and a delayed Resume for N arrives, the server receives no claimed key/generation and cannot distinguish that stale request from a Resume for N+1; it can resume the wrong session despite the stated gate contract. Existing validation cannot prevent this because the PR assigns the TypeBox source of truth here but does not specify identity fields or their requiredness for the C2S messages.

Root cause: the plan freezes intent names and the s2c identity model but omits the c2s wire shapes needed to bind pause/resume requests to a specific session generation. consequently the typebox, protobuf, rust, and client implementations have no authoritative contract for carrying and validating session_key/generation.

[major] Add the omitted AwaitingDelivery no-Resume contract pin

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 · testing

CraftSessionStateV2 adds AwaitingDelivery to the observable phase enum at line 17, but the required Resume-gating pins at line 22 enumerate Idle, Running, Suspended, DeliveryPending, and Terminal and omit AwaitingDelivery. The corresponding client producer plan also omits AwaitingDelivery from its negative Resume cases (docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19,25), while wire_contract_sweep only promises phase serialization/identity combinations and a stale-generation check, not the phase-to-Resume decision. Therefore an implementation that correctly round-trips AwaitingDelivery but treats it as resumable and emits CraftResume would satisfy every named test while reopening a session that is waiting for terminal handoff. No surrounding listed test closes this gap; the omission is introduced with the new V2 phase contract in this diff.

Root cause: the negative producer contract was copied as a selective phase list instead of requiring a resume decision pin for every craftsessionstatev2 phase, leaving the newly declared awaitingdelivery variant unprotected.

[major] R6 P1 cannot prove the later R7 Resume producer

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 · strict-maintainability

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 makes R6 P1's contract pins prove that a matching Paused hydration causes "R7" to emit exactly one Resume and that the other hydration states emit none. But the changed master sequencing at docs/plans-skeleton/plan-refactor-master-v1.md:55 places R6 craft intents before the separately gated R7 P2 Craft Screen producer, and the active R1 plan likewise sequences R6 P1 -> R4 P1 -> R7 P2 (docs/plan-refactor-server-session-v1.md:226). Therefore R7's real production producer does not exist when R6 P1 is supposed to satisfy this acceptance item. An R6-only test could pass only by injecting or duplicating the future R7 behavior, which the production-wiring policy explicitly disallows, while waiting for R7 makes the declared dependency order cyclic.

Root cause: the r6 wire plan assigns an r7-owned screen behavior and its production-level acceptance proof to the earlier r6 p1 phase, instead of limiting r6 to the frozen schema/bridge/store contract and leaving exact resume emission pins with the later r7 producer.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: e0459c19ccaf48bed8782405f099ba2d0317d217
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Identity-less Idle clients cannot send the required CraftOpen request

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · correctness

The changed contract requires every CraftOpen to carry a server-hydrated session_key and generation, while the same line defines Idle as having no session identity. The associated R7 contract says the first screen entry must emit CraftOpen, and forbids the client from omitting, defaulting, or generating those fields. A player whose authoritative hydration is Idle therefore cannot construct the first-open request: omitting the fields fails the required schema/gate, while inventing them violates the identity contract. No existing validation resolves this because the proposed negative pins explicitly reject missing or mismatched identity. This contradiction is introduced by the new craft wire contract and makes its initial-open production path unreachable.

Root cause: the plan applies an existing-session identity precondition uniformly to craftopen even though craftopen is also designated as the producer that initiates the ui lifecycle from the identity-less idle state. the contract needs a distinct identity-free initial-open form or must define a server-issued identity before the client sends craftopen.

[major] Craft identity pins omit default and uint64 boundary cases

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 · testing

The changed contract forbids omitted or default craft identities and defines generation as a monotonic uint64 (line 17), but the P1 pins at this line only require field presence/roundtrip plus missing, stale, and mismatched identities. The later sweep similarly covers required/forbidden field combinations and stale generations, not semantic defaults or numeric boundaries. Therefore an implementation that accepts session_key = "" or generation = 0, or whose monotonic generation wraps from u64::MAX to zero, can satisfy every named test while violating the newly frozen wire contract. The client UI plan also lists missing/stale/mismatched hydration cases but no empty-key, zero-generation, or maximum-generation case, so surrounding track tests do not close this gap.

Root cause: the test deliverables equate required-field presence and ordinary roundtrips with validation of the identity contract, omitting explicit empty/default and uint64 maximum/off-by-one pins for the new generation field.

[major] TsyPresence autosave can commit separately from position and dimension

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:24 · concurrency-atomicity

The changed P1 deliverable persists TsyPresence through its own guarded load/autosave/flush hooks, while the only all-or-nothing requirement with player position and dimension is deferred to P3 and explicitly limited to shutdown flush (line 26). A reachable ordinary runtime save or process crash can therefore commit the presence slice before the independently saved position/dimension, or vice versa. On restart, each row can be individually valid yet represent different snapshots, so guarded field/version validation does not prevent attaching TsyPresence to an overworld position or restoring a TSY position without presence. The new tests at lines 47-48 cover guarded field corruption and crashes between shutdown writes, but do not exercise the routine autosave/crash path that P1 introduces.

Root cause: the plan treats tsypresence as an independently autosaved slice and defines a shared transaction with position/dimension only for shutdown. the three logically coupled values need one atomic snapshot/version boundary for every persistence path, including autosave and disconnect, not only shutdown flush.

[major] Resume producer tests omit Running and Suspended hydrations

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:25 · testing

The changed R7 P2 contract says only a matching server-hydrated Paused session may produce exactly one CraftResume, but its required negative producer tests cover only idle, terminal, AwaitingDelivery, DeliveryPending, and stale/mismatched hydration. They omit both Running and Suspended. A concrete implementation such as if (hasIdentity && !isTerminalOrDelivery) sendResume() would pass every listed R7 producer pin while incorrectly sending CraftResume for hydrated Running and Suspended sessions. The R6 plan cannot close this gap: it explicitly leaves the production Resume producer to R7, so R6 schema/bridge/store tests do not exercise this caller behavior.

Root cause: the production-producer test matrix does not enumerate all non-paused phase variants even though phase is an enum and the observable contract permits resume only from paused; the omitted running and suspended transitions therefore remain unprotected.

[major] Initial CraftOpen requires an identity that Idle hydration cannot supply

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19 · security

On the initial craft-screen entry while the authoritative server hydration is Idle/no-session, this changed contract requires the client to send CraftOpen with a hydrated session_key and generation and forbids omission, defaults, or client generation. However, docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 explicitly specifies that Idle has no session identity while also making both fields required on CraftOpen. The R7 negative cases only prohibit CraftResume for idle/no-session; they do not provide any server-issued open capability or alternate identity for CraftOpen. Therefore the newly specified first-open production path cannot construct a valid request. An implementation must either reject every first open (making craft entry unreachable) or weaken the identity boundary by accepting invented/default identity values, which contradicts the stated anti-forgery contract. This is introduced by the newly added CraftOpen lifecycle wiring and is not resolved by the surrounding R4 owner/phase checks because those checks consume, rather than mint, the missing identity.

Root cause: the plan uses an existing-session identity as a required authorization token for both creating/opening a session and operating an already hydrated session, but the authoritative idle state intentionally supplies no such identity. the protocol needs a distinct server-issued open target/capability or must define craftopen as identity-free with server-side ownership/target validation, while retaining session identity for pause/resume.

[major] Durable delivery acceptance omits the production loot path

docs/plans-skeleton/plan-refactor-inventory-core-v1.md:23 · testing

P2 now requires every give/craft/alchemy/forge/loot production delivery call site to migrate to durable deliver(delivery_id, items), but the acceptance matrix at lines 40-42 exercises full-inventory delivery only for craft, alchemy, forge, and give, then tests crash/replay behavior generically. No listed scenario proves that a production loot pickup/drop-delivery caller actually uses the durable API under full inventory or replay. The repository's existing rat drop round-trip test (server/src/fauna/drop.rs:1239) picks loot into an empty inventory, and the available full-inventory spiritwood scenario tests harvest fallback rather than a migrated loot delivery receipt. An implementation that migrates the other four named callers but leaves loot on its legacy non-receipted path would therefore satisfy all specified R10 acceptance scenarios while violating the changed all-call-sites contract and remaining vulnerable to loss or duplicate mutation at a retry/crash boundary.

Root cause: the plan expands the durable delivery contract to the production loot path without adding a producer-level full-inventory and stable-delivery-id replay test for that fifth call-site category; api-level atomicity tests cannot detect a caller that never adopts the api.

[major] Resolved payloads escape the delivery quota and grow without bound

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:15 · performance

The new terminal-delivery contract releases SessionDeliveryQuota when an obligation reaches ResolvedDisposition, while requiring that disposition to retain the complete payload. The corresponding R1 contract makes the reachable path explicit: a delivery that fails 10 times or ages 7 days becomes DeadLetter, and an operator may resolve it by moving the full payload into a persistent ResolvedDisposition; only then is the outbox row/quota released (docs/plan-refactor-server-session-v1.md:126-128). Repeating this normal recovery path creates one durable full-payload disposition per resolution, but neither this plan nor R1/R10 gives those dispositions a quota, retention period, compaction rule, or deletion transition. SESSION_DELIVERY_MAX_PAYLOAD_BYTES bounds each record only, not their count, so long-running servers can accumulate unbounded SQLite data even though the newly added outbox quota reports capacity as free.

Root cause: the quota lifecycle accounts only for active outbox obligations. resolving a dead letter transfers its payload into a second durable store and decrements the quota without placing that retained store under any bounded-capacity or retention policy, moving rather than bounding persistent resource growth.

[major] Resolved payloads escape the quota and can grow storage without bound

docs/plan-refactor-server-session-v1.md:128 · performance

A reachable cycle is: admit a checkpointed session (reserving at most SESSION_DELIVERY_MAX_PAYLOAD_BYTES), terminalize it into the outbox, let delivery reach DeadLetter, then have an authorized operator resolve it. This line requires the complete payload to remain in a durable ResolvedDisposition, but also releases the original row/quota after that disposition is committed. The released quota permits another maximum-size session to be admitted, while the previous payload remains persisted. Repeating normal dead-letter resolution therefore accumulates durable payloads without any row/byte cap, retention period, compaction, or deletion rule. The quota described here and in plan-refactor-persistence-slices-v1.md does not bound this storage because resolved dispositions are explicitly outside the released obligation.

Root cause: the plan makes resolved payload retention permanent for audit/recovery purposes while accounting those bytes as freed capacity. because retained resolveddisposition data has no separate quota or lifecycle policy, each resolve operation can add up to the maximum payload size to unmetered persistent storage, creating an unbounded database-growth path.

[major] R6's new TypeBox ownership contradicts the master agent boundary

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:16 · strict-maintainability

The changed line assigns R6 ownership of edits to the TypeBox source files under agent/packages/schema/src/ and requires those edits in the same delivery. The master plan still states that the refactor touches only server/ and client/, that agent/ is not touched, and that agent-side work is limited to passive regeneration (docs/plans-skeleton/plan-refactor-master-v1.md:8-9); the unchanged introduction of this plan repeats that agent TS is only passively regenerated. Editing the TypeBox source is an authoritative source change, not passive regeneration. A conforming implementation therefore cannot satisfy both plans' file/scope contracts, and ownership/review of the schema source is left contradictory.

Root cause: the new cross-stack contract adds authoritative agent schema-source work to r6 without updating the master scope and ownership boundary that explicitly excludes agent source changes.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: ddfcd9bc61c85eab38f2622753d0bad5c7ff3dec
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Workbench close producer is omitted from the pause/resume wiring

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:25 · wiring

The absorbed defect is explicitly reachable through both CraftScreen and WorkbenchScreen: client/src/main/java/com/bong/client/craft/CraftScreen.java:116-123 and client/src/main/java/com/bong/client/craft/WorkbenchScreen.java:122-129 both send CraftCancel from removed() whenever a session is active. The source plan being absorbed also names both production paths and requires workbench_screen_escape_does_not_destroy_session (docs/plans-skeleton/plan-craft-close-pause-loss-v1.md:21-25,61-71). However, the changed R7 P2 deliverable at this line assigns open/pause/cancel/resume production wiring only to CraftScreen, and its acceptance list likewise names only four generic intent pins; WorkbenchScreen is not assigned a producer migration or regression pin anywhere in the changed R7 text. The R1 plan then gates and claims the absorbed close/pause behavior on that R7 P2 deliverable (docs/plan-refactor-server-session-v1.md:193,236-238). Following the plan as written therefore leaves the normal workbench Esc/forced-screen-transition path emitting CraftCancel, so the server applies the existing 70% cancellation refund and the player still loses 30% of unconsumed materials. Neither the shared CraftStore nor the server gate prevents this, because the wrong intent is produced before either can distinguish a close from an explicit cancel.

Root cause: the cross-track handoff narrows a two-producer lifecycle defect to craftscreen only. it does not migrate or test workbenchscreen.removed(), even though that independent production producer implements the same erroneous close-to-cancel mapping and is explicitly part of the absorbed plan.

[major] Resolved delivery payloads bypass every storage quota

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:54 · correctness

The changed contract allows an authorized operator to resolve each failed delivery by moving its complete payload into a durable ResolvedDisposition, then deleting the original obligation and releasing its quota (lines 15 and 23). The plan itself states here that these retained payloads have no frozen retention, compaction, or independent quota and defers all bounding policy. A reachable sequence is: admit sessions up to the bounded outbox quota, let deliveries dead-letter, resolve them into retained dispositions, regain the quota, and repeat. Every cycle permanently adds payload bytes while all admission checks continue to pass, so the SQLite database can grow without bound. The row/byte reservation, dead-letter quota, and CAS rules only bound live outbox obligations; none accounts for payload after resolution. This gap is introduced by this diff's new durable resolution and quota-release contract.

Root cause: resolveddisposition is defined as a durable full-payload sink that releases the only row/byte quota, while its own retention and capacity policy is explicitly postponed. moving data out of the metered outbox therefore bypasses the storage bound.

[major] R6's required agent schema wiring is excluded by the master plan

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · wiring

The changed R6 contract assigns agent/packages/schema/src/client-request.ts, agent/packages/schema/src/craft.ts, generated JSON Schema, and the committed @bong/schema dist to R6, and P1 repeats that these TypeBox sources/artifacts must be delivered. However, the master plan's governing scope says the refactor changes only server/ and client/, that agent/ is not touched (docs/plans-skeleton/plan-refactor-master-v1.md:9), and that agent work remains an independent track (:82). Following the master scope therefore omits the TypeBox source-of-truth and dist updates required for the three new C2S variants and CraftSessionStateV2; following R6 instead violates the master ownership/scope contract. The dependency chain cannot be implemented unambiguously as written, and the agent schema/dist can remain on the old 113-variant/V1 contract while server and client switch to 116/V2.

Root cause: the diff adds agent typebox source and distribution artifacts to r6's required production wiring without updating the master plan's explicit exclusion and independent-agent ownership boundary. because typebox is the ipc source of truth, this is not an optional documentation reorganization: one of the authoritative plans directs implementers to skip artifacts that the other requires for the cross-stack contract.

[major] Authenticated players satisfy the frozen maintenance authorization rule

docs/plan-refactor-server-session-v1.md:78 · security

A normal online-mode player has a server-authenticated principal bound to their current executor entity, so the stated predicate accepts the request even though that principal has no maintenance role. They can submit session-maintenance terminate <victim SessionKey>; the target key is explicitly part of the command at line 125, and the lifecycle then terminalizes the victim session and releases its facility/target claims. The later requirement that ordinary players and cross-owner targets be rejected does not prevent this because the frozen authorization contract defines no privileged role, capability class, allow-list, or owner restriction with which to distinguish them. This ambiguity is introduced by the new SessionMaintenancePermissions contract and is security-relevant because an implementation that follows its positive rule can satisfy authentication/binding while permitting destructive administration of another player's session.

Root cause: the plan conflates authentication and executor binding with authorization: authenticated principal/capability is accepted without requiring a server-issued maintenance privilege or restricting the operation to the session owner. the contract must require a specific unforgeable maintenance capability/role (and define its issuance/validation), with console as the only alternative, so ordinary authenticated principals fail closed.

[major] Freeze a lossless generation representation across the wire bridge

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · schema-contracts

The changed contract requires generation: uint64 to round-trip through TypeBox/JSON Schema, protobuf, JsonFormat, the Java bridge, handler, and CraftStore, while line 52 explicitly defers the u64::MAX and overflow pins. This is not merely an untested edge: protobuf JSON serializes uint64 as a decimal string, whereas TypeBox's ordinary integer representation is a JavaScript number and cannot distinguish consecutive values above Number.MAX_SAFE_INTEGER. The current bridge also converts numeric strings with Long.parseLong (client/src/main/java/com/bong/client/network/ProtoServerDataBridge.java:1501-1523), so values above signed i64::MAX remain strings while smaller values become JSON numbers. A long-running or restored session whose generation crosses either boundary therefore changes representation or loses identity, and stale-generation rejection can accept/reject the wrong state. No surrounding contract prevents this because the plan freezes the full uint64 domain but postpones the only tests that expose the boundary.

Root cause: the plan assigns one logical uint64 field incompatible numeric representations across protobuf json, javascript/typebox, and java without freezing a canonical boundary representation or a safe-range restriction.

[major] Generation overflow can pass the deferred V2 contract tests

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:52 · testing

The changed plan defines CraftSessionStateV2.generation as a monotonic uint64 used to reject stale state (lines 17 and 22), but line 52 explicitly defers tests for 0, u64::MAX, overflow refusal, and off-by-one behavior. An implementation that increments u64::MAX by wrapping to 0 and then lets CraftStore accept that value as a newer generation would satisfy the planned basic roundtrip and ordinary stale-generation tests while rolling the client store back at the boundary. Neither schema validation nor protobuf roundtripping prevents this because both legitimately represent the boundary values; only the omitted producer/store boundary contract pins expose the failure. This gap is introduced by the new V2 generation contract and its explicit test deferral.

Root cause: the plan introduces a monotonic cross-stack uint64 generation contract but postpones the boundary and overflow tests needed to enforce monotonicity at the producer-to-store transition, leaving a concrete wrapping/rollback implementation able to pass the stated p1 acceptance suite.

[major] Resolved dispositions bypass the only payload-storage quota

docs/plan-refactor-server-session-v1.md:128 · correctness

ResolvedDisposition { ..., payload } is required to retain the complete payload, but the same transition releases the original obligation quota. The coordinated persistence plan explicitly says retention, compaction, and a separate disposition quota are not defined (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:55). A reachable sequence of failed deliveries followed by authorized resolve operations therefore moves each payload into durable disposition storage, frees admission capacity, and permits more sessions to repeat the cycle indefinitely. None of the TTL, 10-attempt, or seven-day limits removes resolved dispositions, so database growth is unbounded even while all stated quotas pass.

Root cause: the terminal-obligation quota accounts only for active outbox obligations and is released when a full payload is moved to resolveddisposition, while the plan defines no bounded retention, compaction, or quota for those retained payloads.

[major] Define the winner for same-tick insight response and timeout

docs/plan-refactor-server-session-v1.md:240 · concurrency-atomicity

The new P1 acceptance text requires a test for a PendingInsightOffer response racing its timeout in the same tick, but it never defines the winner or linearization rule. A concrete boundary execution is: an offer reaches its deadline, the client's valid InsightChosen event is already queued for that tick, and the timeout system is also eligible. If timeout runs first it removes the pending offer and apply_insight_chosen ignores the response; if response runs first it applies the choice and mutates cultivation/quota before timeout observes removal. Current production data does not resolve this ordering: PendingInsightOffer contains only trigger_id and choices (server/src/cultivation/insight_flow.rs:33-36), while InsightChosen contains only entity, trigger ID, and choice index (server/src/cultivation/insight.rs:382-386), with no response timestamp, offer generation, or committed terminal state. Thus scheduler/system ordering can produce two different player-visible outcomes at the same deadline, and a test merely "covering" the race can accept either behavior because the plan supplies no expected result.

Root cause: the plan adds a concurrent deadline transition without freezing a deterministic response-versus-timeout precedence or a single atomic/cas transition keyed by stable offer identity. boundary and off-by-one coverage do not substitute for the missing same-tick state-transition contract, so the implementation can remain schedule-dependent while still satisfying the stated acceptance checklist.

[major] P2 Resume tests omit Running and Suspended hydration

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:49 · testing

The changed P2 contract says CraftResume must be emitted only for a matching server-hydrated Paused session, but its required negative producer tests enumerate Idle, Terminal, AwaitingDelivery, DeliveryPending, and stale/mismatched identity while line 49 explicitly defers Running and Suspended. Both are members of the frozen CraftSessionStateV2 phase enum, and the server-session reconnect contract permits hydration as Paused or Running. A producer implemented as “resume for any identified non-terminal phase” would therefore incorrectly emit CraftResume for Running or Suspended hydration while passing every P2 test listed here. The master plan places R7 P2 itself in Wave 2, so deferring these cases to the same Wave 2 window does not identify a later concrete test gate that closes the gap before this deliverable is accepted.

Root cause: the plan claims a phase-exclusive resume producer contract but omits two valid non-paused enum states from its negative acceptance matrix, leaving the changed observable contract unprotected against an over-broad phase predicate.

1 similar comment
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: ddfcd9bc61c85eab38f2622753d0bad5c7ff3dec
Policy: project-review-policy.v2
Policy SHA-256: 3d5a5de3227540025123a516e2856b2c183ac6bf029b6863404d5d30d3f00c70

Validated findings

[major] Workbench close producer is omitted from the pause/resume wiring

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:25 · wiring

The absorbed defect is explicitly reachable through both CraftScreen and WorkbenchScreen: client/src/main/java/com/bong/client/craft/CraftScreen.java:116-123 and client/src/main/java/com/bong/client/craft/WorkbenchScreen.java:122-129 both send CraftCancel from removed() whenever a session is active. The source plan being absorbed also names both production paths and requires workbench_screen_escape_does_not_destroy_session (docs/plans-skeleton/plan-craft-close-pause-loss-v1.md:21-25,61-71). However, the changed R7 P2 deliverable at this line assigns open/pause/cancel/resume production wiring only to CraftScreen, and its acceptance list likewise names only four generic intent pins; WorkbenchScreen is not assigned a producer migration or regression pin anywhere in the changed R7 text. The R1 plan then gates and claims the absorbed close/pause behavior on that R7 P2 deliverable (docs/plan-refactor-server-session-v1.md:193,236-238). Following the plan as written therefore leaves the normal workbench Esc/forced-screen-transition path emitting CraftCancel, so the server applies the existing 70% cancellation refund and the player still loses 30% of unconsumed materials. Neither the shared CraftStore nor the server gate prevents this, because the wrong intent is produced before either can distinguish a close from an explicit cancel.

Root cause: the cross-track handoff narrows a two-producer lifecycle defect to craftscreen only. it does not migrate or test workbenchscreen.removed(), even though that independent production producer implements the same erroneous close-to-cancel mapping and is explicitly part of the absorbed plan.

[major] Resolved delivery payloads bypass every storage quota

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:54 · correctness

The changed contract allows an authorized operator to resolve each failed delivery by moving its complete payload into a durable ResolvedDisposition, then deleting the original obligation and releasing its quota (lines 15 and 23). The plan itself states here that these retained payloads have no frozen retention, compaction, or independent quota and defers all bounding policy. A reachable sequence is: admit sessions up to the bounded outbox quota, let deliveries dead-letter, resolve them into retained dispositions, regain the quota, and repeat. Every cycle permanently adds payload bytes while all admission checks continue to pass, so the SQLite database can grow without bound. The row/byte reservation, dead-letter quota, and CAS rules only bound live outbox obligations; none accounts for payload after resolution. This gap is introduced by this diff's new durable resolution and quota-release contract.

Root cause: resolveddisposition is defined as a durable full-payload sink that releases the only row/byte quota, while its own retention and capacity policy is explicitly postponed. moving data out of the metered outbox therefore bypasses the storage bound.

[major] R6's required agent schema wiring is excluded by the master plan

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · wiring

The changed R6 contract assigns agent/packages/schema/src/client-request.ts, agent/packages/schema/src/craft.ts, generated JSON Schema, and the committed @bong/schema dist to R6, and P1 repeats that these TypeBox sources/artifacts must be delivered. However, the master plan's governing scope says the refactor changes only server/ and client/, that agent/ is not touched (docs/plans-skeleton/plan-refactor-master-v1.md:9), and that agent work remains an independent track (:82). Following the master scope therefore omits the TypeBox source-of-truth and dist updates required for the three new C2S variants and CraftSessionStateV2; following R6 instead violates the master ownership/scope contract. The dependency chain cannot be implemented unambiguously as written, and the agent schema/dist can remain on the old 113-variant/V1 contract while server and client switch to 116/V2.

Root cause: the diff adds agent typebox source and distribution artifacts to r6's required production wiring without updating the master plan's explicit exclusion and independent-agent ownership boundary. because typebox is the ipc source of truth, this is not an optional documentation reorganization: one of the authoritative plans directs implementers to skip artifacts that the other requires for the cross-stack contract.

[major] Authenticated players satisfy the frozen maintenance authorization rule

docs/plan-refactor-server-session-v1.md:78 · security

A normal online-mode player has a server-authenticated principal bound to their current executor entity, so the stated predicate accepts the request even though that principal has no maintenance role. They can submit session-maintenance terminate <victim SessionKey>; the target key is explicitly part of the command at line 125, and the lifecycle then terminalizes the victim session and releases its facility/target claims. The later requirement that ordinary players and cross-owner targets be rejected does not prevent this because the frozen authorization contract defines no privileged role, capability class, allow-list, or owner restriction with which to distinguish them. This ambiguity is introduced by the new SessionMaintenancePermissions contract and is security-relevant because an implementation that follows its positive rule can satisfy authentication/binding while permitting destructive administration of another player's session.

Root cause: the plan conflates authentication and executor binding with authorization: authenticated principal/capability is accepted without requiring a server-issued maintenance privilege or restricting the operation to the session owner. the contract must require a specific unforgeable maintenance capability/role (and define its issuance/validation), with console as the only alternative, so ordinary authenticated principals fail closed.

[major] Freeze a lossless generation representation across the wire bridge

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 · schema-contracts

The changed contract requires generation: uint64 to round-trip through TypeBox/JSON Schema, protobuf, JsonFormat, the Java bridge, handler, and CraftStore, while line 52 explicitly defers the u64::MAX and overflow pins. This is not merely an untested edge: protobuf JSON serializes uint64 as a decimal string, whereas TypeBox's ordinary integer representation is a JavaScript number and cannot distinguish consecutive values above Number.MAX_SAFE_INTEGER. The current bridge also converts numeric strings with Long.parseLong (client/src/main/java/com/bong/client/network/ProtoServerDataBridge.java:1501-1523), so values above signed i64::MAX remain strings while smaller values become JSON numbers. A long-running or restored session whose generation crosses either boundary therefore changes representation or loses identity, and stale-generation rejection can accept/reject the wrong state. No surrounding contract prevents this because the plan freezes the full uint64 domain but postpones the only tests that expose the boundary.

Root cause: the plan assigns one logical uint64 field incompatible numeric representations across protobuf json, javascript/typebox, and java without freezing a canonical boundary representation or a safe-range restriction.

[major] Generation overflow can pass the deferred V2 contract tests

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:52 · testing

The changed plan defines CraftSessionStateV2.generation as a monotonic uint64 used to reject stale state (lines 17 and 22), but line 52 explicitly defers tests for 0, u64::MAX, overflow refusal, and off-by-one behavior. An implementation that increments u64::MAX by wrapping to 0 and then lets CraftStore accept that value as a newer generation would satisfy the planned basic roundtrip and ordinary stale-generation tests while rolling the client store back at the boundary. Neither schema validation nor protobuf roundtripping prevents this because both legitimately represent the boundary values; only the omitted producer/store boundary contract pins expose the failure. This gap is introduced by the new V2 generation contract and its explicit test deferral.

Root cause: the plan introduces a monotonic cross-stack uint64 generation contract but postpones the boundary and overflow tests needed to enforce monotonicity at the producer-to-store transition, leaving a concrete wrapping/rollback implementation able to pass the stated p1 acceptance suite.

[major] Resolved dispositions bypass the only payload-storage quota

docs/plan-refactor-server-session-v1.md:128 · correctness

ResolvedDisposition { ..., payload } is required to retain the complete payload, but the same transition releases the original obligation quota. The coordinated persistence plan explicitly says retention, compaction, and a separate disposition quota are not defined (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:55). A reachable sequence of failed deliveries followed by authorized resolve operations therefore moves each payload into durable disposition storage, frees admission capacity, and permits more sessions to repeat the cycle indefinitely. None of the TTL, 10-attempt, or seven-day limits removes resolved dispositions, so database growth is unbounded even while all stated quotas pass.

Root cause: the terminal-obligation quota accounts only for active outbox obligations and is released when a full payload is moved to resolveddisposition, while the plan defines no bounded retention, compaction, or quota for those retained payloads.

[major] Define the winner for same-tick insight response and timeout

docs/plan-refactor-server-session-v1.md:240 · concurrency-atomicity

The new P1 acceptance text requires a test for a PendingInsightOffer response racing its timeout in the same tick, but it never defines the winner or linearization rule. A concrete boundary execution is: an offer reaches its deadline, the client's valid InsightChosen event is already queued for that tick, and the timeout system is also eligible. If timeout runs first it removes the pending offer and apply_insight_chosen ignores the response; if response runs first it applies the choice and mutates cultivation/quota before timeout observes removal. Current production data does not resolve this ordering: PendingInsightOffer contains only trigger_id and choices (server/src/cultivation/insight_flow.rs:33-36), while InsightChosen contains only entity, trigger ID, and choice index (server/src/cultivation/insight.rs:382-386), with no response timestamp, offer generation, or committed terminal state. Thus scheduler/system ordering can produce two different player-visible outcomes at the same deadline, and a test merely "covering" the race can accept either behavior because the plan supplies no expected result.

Root cause: the plan adds a concurrent deadline transition without freezing a deterministic response-versus-timeout precedence or a single atomic/cas transition keyed by stable offer identity. boundary and off-by-one coverage do not substitute for the missing same-tick state-transition contract, so the implementation can remain schedule-dependent while still satisfying the stated acceptance checklist.

[major] P2 Resume tests omit Running and Suspended hydration

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:49 · testing

The changed P2 contract says CraftResume must be emitted only for a matching server-hydrated Paused session, but its required negative producer tests enumerate Idle, Terminal, AwaitingDelivery, DeliveryPending, and stale/mismatched identity while line 49 explicitly defers Running and Suspended. Both are members of the frozen CraftSessionStateV2 phase enum, and the server-session reconnect contract permits hydration as Paused or Running. A producer implemented as “resume for any identified non-terminal phase” would therefore incorrectly emit CraftResume for Running or Suspended hydration while passing every P2 test listed here. The master plan places R7 P2 itself in Wave 2, so deferring these cases to the same Wave 2 window does not identify a later concrete test gate that closes the gap before this deliverable is accepted.

Root cause: the plan claims a phase-exclusive resume producer contract but omits two valid non-paused enum states from its negative acceptance matrix, leaving the changed observable contract unprotected against an over-broad phase predicate.

@Kizunad
Kizunad force-pushed the refactor/plan-refactor-server-session-v1 branch from ddfcd9b to 8fcc4d7 Compare August 3, 2026 15:58
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: infrastructure_failure

The read-only review job did not produce a verifiable artifact. No code verdict was inferred.

server_kizuna and others added 8 commits August 4, 2026 02:05
冻结持久与易失会话的生命周期、忙态和交付原子性边界,并以当前源码验真吸收清单,防止后续迁移跨轨重复实现。

Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
冻结有限 suspension 回收和 termination cause 经济矩阵,并为 craft wire、delivery 与 TSY presence 持久化建立真实生产依赖,避免各轨提前宣称闭环。

Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
明确 durable outbox 与 runtime claim 的提交后幂等关系,补全 TSY 快照、管理员授权、dead-letter 退避和 obligation quota,避免崩溃重放与 retained state 无界增长。

Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
将 atomic handoff 前移至 R3 P1,补齐交付中间态、byte quota、CraftOpen producer、PendingInsightOffer 回归、TSY 拒绝分支与 116 变体计数。

Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
Model: claude-sonnet-5

Co-Authored-By: Claude <noreply@anthropic.com>
Model: claude-sonnet-5

Co-Authored-By: Claude <noreply@anthropic.com>
明确 Idle 初开由服务端分配会话身份,并冻结 TsyPresence 耦合快照;其余深度验收留待 Wave 2 接缝收口。

Model: claude-sonnet-5

Co-Authored-By: Claude <noreply@anthropic.com>
@Kizunad
Kizunad force-pushed the refactor/plan-refactor-server-session-v1 branch from 8fcc4d7 to aa20664 Compare August 3, 2026 18:06
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: infrastructure_failure
Reviewed head: aa20664378bfca5be6dc776bb8a7bbbe684135bd
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

The review could not complete safely. No approval or code finding was inferred from the failed stages.

  • find:strict-maintainabilityinfra_error: 2 occurrence(s) in 2/2 failed batch(es); samples: batch-0: after 3 attempts: claude exited 1 | batch-1: after 3 attempts: claude exited 1
    • diagnostic: {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""} {"events":[{"type":"system","subtype":"init","model":"cc-review","apiKeySource":"ANTHROPIC_API_KEY","claudeCodeVersion":"2.1.220"},{"type":"result","subtype":"success","isError":true,"apiErrorStatus":403,"terminalReason":"api_error"}],"stderr":""}

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: aa20664378bfca5be6dc776bb8a7bbbe684135bd
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] P4 termination matrix omits the required Completed transition

docs/plan-refactor-server-session-v1.md:264 · concurrency-atomicity

The contract pins explicitly require coverage for normal Completed handoff and assert that it delivers only the output without refunding inputs (docs/plan-refactor-server-session-v1.md:237), and the retention checklist also calls out Completed as a required no-refund/cause-roundtrip case (docs/plan-refactor-server-session-v1.md:131). However, the P4 session_termination_cause_matrix acceptance scenario enumerates only VoluntaryCancel, Disconnect, DimensionChange, Shutdown, InvalidRestore, and SuspensionExpired, omitting Completed (docs/plan-refactor-server-session-v1.md:264). An implementation can therefore pass the named P4 termination-cause scenario without testing the normal terminal handoff's exactly-once delivery versus input-refund exclusion.

Root cause: the required acceptance inventory and the declared terminationcause contract are out of sync: the normal completed transition is required by the lifecycle and delivery contract but is absent from the dedicated p4 state-transition test scenario. this leaves a concrete incorrect implementation, such as refunding escrow or duplicating output during normal completion, unprotected by the stated acceptance test.

[major] R6 craft schema ownership contradicts the master agent scope

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:16 · strict-maintainability

The updated wire plan assigns R6 ownership of agent/packages/schema/src/client-request.ts, generated JSON Schema and committed @bong/schema dist, and says R6 must deliver those agent artifacts (plan-refactor-wire-s2c-v1.md:16, 21). The master plan still explicitly freezes agent/ out of the refactor scope (plan-refactor-master-v1.md:8) and says the tracks do not modify it, while its ownership matrix describes R6 as owning server emit/proto-convert and client network areas only (plan-refactor-master-v1.md:60-62). These instructions cannot both be followed: implementing the newly required craft C2S schema/dist path either violates the master scope or leaves the declared R6 contract incomplete.

Root cause: the wire plan expands r6 into the agent schema source and distribution artifacts without reconciling the master plan's explicit agent exclusion and ownership matrix.

[major] TerminationCause cannot represent authorized administrator closure

docs/plan-refactor-server-session-v1.md:73 · strict-maintainability

An authorized operator can reach session-maintenance terminate <SessionKey> for a still-valid, unexpired suspended session via the production maintenance path specified at line 125. However, the frozen enum at line 73 contains only Completed, VoluntaryCancel, Disconnect, DimensionChange, Shutdown, InvalidRestore, and SuspensionExpired; there is no cause for an administrative termination. Line 84 consequently groups administrator closure with InvalidRestore/SuspensionExpired, even though neither condition occurred, while lines 130 and 162-169 require the cause to drive audit and refund semantics and line 238 declares exhaustive pins only for those seven variants. Thus an implementation following this plan must either reject a permitted maintenance action or persist a factually false cause and exercise the wrong cause-specific contract. The authorization checks do not prevent this path, and the exhaustive seven-cause tests would not cover it because the omission is in the newly frozen contract itself.

Root cause: the plan adds administrator-forced session termination as a first-class reachable lifecycle event but freezes terminationcause and its exhaustive test/refund matrix without a corresponding administrative cause.

补齐 Completed 的 P4 终止矩阵覆盖,新增授权管理员结案原因及审计结算语义,并将 R6 的 agent schema/dist 改为冻结依赖而非所有权。

Model: cc-haiku-max
Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: bbdfcc8c2d5e7d794fd7e7c1b40e99cd554bbc99
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Craft schema artifacts have no consistent production owner

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:21 · wiring

At docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:16, the changed contract says R6 “不拥有或修改” the TypeBox sources, generated JSON Schema, or committed @bong/schema dist and requires those artifacts to be supplied first by an independent owner. However, changed P1 at line 21 assigns R6 the complete TypeBox source→generated JSON Schema/dist delivery for all three C2S intents and the S2C V2 payload. The master plan still declares agent/ out of scope (docs/plans-skeleton/plan-refactor-master-v1.md:8) and names no independent track/phase that will produce these prerequisite schema artifacts. Following the ownership boundary leaves R6 P1 blocked with no producer; following the P1 deliverable makes R6 violate its explicit file ownership and master scope. Consequently the planned craft wire chain cannot be scheduled or accepted unambiguously, and downstream R4/R1/R7 gates that require R6 P1 cannot become reachable under the stated plan.

Root cause: the plan simultaneously assigns schema-source generation to r6 and excludes those artifacts from r6 ownership, without registering any other production owner or prerequisite phase for them.

[major] P1 defers the crash evidence needed to verify coupled snapshot atomicity

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:23 · concurrency-atomicity

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:23 makes the P1 acceptance gate require crash-injection pins for the coupled snapshot boundaries, including autosave/disconnect/shutdown behavior. However, :25 moves the crash regression to P3, and :55 explicitly says the fault-injection harness and cross-connection timing are deferred to P3. Thus a P1 implementation can be accepted without the test mechanism that would expose a crash between the presence, position, and dimension writes, despite the plan requiring those writes to be atomic.

Root cause: the coupled snapshot's atomicity requirement is declared in p1, but its executable concurrency/crash evidence is assigned to p3 and separately described as deferred. this leaves the phase gate unable to distinguish a genuinely single-transaction implementation from three independently committed writes, so a partial snapshot can be introduced while still satisfying the stated p1 acceptance boundary.

[major] Craft release gate has no owner for the TypeBox source-of-truth artifacts

docs/plan-refactor-server-session-v1.md:228 · schema-contracts

The new stage gate says R6 P1 delivers CraftOpen/CraftPause/CraftResume "proto/schema/bridge" artifacts and makes every downstream production phase depend on that delivery. However, the canonical R6 contract at docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17 explicitly says R6 does not own or modify agent/packages/schema/src/client-request.ts, agent/packages/schema/src/craft.ts, generated JSON Schema, or the committed @bong/schema distribution; those artifacts must come from an independent owner. The master plan also excludes agent/ from the refactor at docs/plans-skeleton/plan-refactor-master-v1.md:9. The current TypeBox sources still expose CraftSessionStateV1 (agent/packages/schema/src/craft.ts:123) and a ClientRequestV1 union without the three lifecycle intents (agent/packages/schema/src/client-request.ts:1135). No independent schema owner or prerequisite appears in this new release matrix, so following the documented gates can leave the IPC source of truth and distribution at 113 variants while protobuf/Rust/client consumers advance to 116.

Root cause: the plan assigns the typebox/schema prerequisite to r6 even though the authoritative ownership contract forbids r6 from producing it, and it omits the independent owner that must update and publish the schema source-of-truth artifacts before r6 consumes them.

[major] CraftOpen has no frozen target bridge for workbench sessions

docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19 · wiring

The changed contract says CraftOpen is sent when either Craft Screen enters authoritative Idle/no-session and that R4 validates the target, but it freezes no target/station locator or prior-open capability in that request. The two real entry paths require different authority: CraftScreenBootstrap directly creates new CraftScreen() (client/src/main/java/com/bong/client/craft/CraftScreenBootstrap.java:29-33), while a workbench first sends WorkbenchOpen { entity_id } (server/src/schema/client_request.rs:532-538), whose server handler only emits a response (server/src/craft/workbench.rs:115-159); the client response handler currently discards the envelope fields and constructs new WorkbenchScreen() (client/src/main/java/com/bong/client/craft/WorkbenchScreenBootstrap.java:26-35). CraftStore is simultaneously declared the sole session-state owner, and Idle intentionally has no session identity. Therefore, on the documented initial CraftOpen, R4 has no frozen wire value or retained context with which to distinguish handcraft from a particular workbench and enforce the promised target/facility gate. An implementation following this plan must either reject valid workbench opens, create the wrong kind of session, or accept without the claimed target authorization. The new cross-track contract must define and carry a target/station identity (or explicitly persist and bind a server-issued open capability) through R6, R4, R7, and R1.

Root cause: the newly frozen craftopen producer/consumer chain specifies that target gating occurs but omits the target-bearing wire field or another authoritative context bridge needed to perform that gate across the existing workbench s2c-to-screen boundary.

server_kizuna and others added 2 commits August 4, 2026 15:46
由 Agent 轨原子拥有 TypeBox source 及生成物,R6 仅消费并落 wire;同时把耦合快照强杀证据前移至 R3 P1,并冻结 workbench_key 的跨端校验契约。

Model: cc-haiku-max
Co-Authored-By: Claude <noreply@anthropic.com>
吸收 R10 促升及 inventory 接缝更新,同时保留 craft schema 所有权、workbench_key 目标桥与 R3 P1 强杀验收边界。

Model: cc-haiku-max
Co-Authored-By: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 12

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md (1)

12-17: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

在 skeleton 计划的接入面补充 worldview.md 锚点。

两个接入面已经列出进料、出料、共享类型和跨仓库契约,但没有对应的 worldview.md §X L<line> 锚点。请在同一接入面章节中补充准确、可核验的世界观引用。不要使用无法支持计划范围的泛化锚点。

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md#L12-L17: 补充 wire、作用域广播和跨仓库契约的世界观依据。
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md#L12-L17: 补充 C2S 门禁、距离、维度和所有权规则的世界观依据。

As per coding guidelines,新的 plan 接入面必须同时包含进料、出料、共享类型/event/schema、跨仓库契约和对应的 worldview.md 锚点。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md` around lines 12 - 17, 在
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md 第12-17行的“接入面”中补充准确、可核验的
worldview.md 锚点,覆盖 wire、作用域广播及跨仓库契约依据;在
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第12-17行的对应“接入面”中补充覆盖 C2S
门禁、距离、维度和所有权规则的 worldview.md 锚点。两处均需保留进料、出料、共享类型/event/schema
与跨仓库契约内容,并仅引用能支持计划范围的具体章节和行号。

Source: Coding guidelines

♻️ Duplicate comments (5)
docs/plan-refactor-server-session-v1.md (4)

1-3: 📐 Maintainability & Code Quality | 🟠 Major

两个 plan 的 header 接入面仍不完整。

  • docs/plan-refactor-server-session-v1.md#L1-L3:新增完整 ## 接入面,声明进料、出料、共享类型/event/schema、server/agent/client 跨仓库契约和准确的 worldview.md §X L<line>
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L13-L19:补充准确的 worldview.md 章节和行号,并明确“零 wire 改动”是 server/agent/client 契约。

As per coding guidelines,plan header 必须声明完整接入面和 worldview.md 锚点。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` around lines 1 - 3, Update the
header of docs/plan-refactor-server-session-v1.md at lines 1-3 by adding a
complete “接入面” section covering inputs, outputs, shared types/events/schemas,
server/agent/client cross-repository contracts, and an accurate worldview.md §X
line anchor. Also update
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md at lines 13-19 with
the accurate worldview.md section and line reference, explicitly defining “zero
wire changes” as the server/agent/client contract.

Source: Coding guidelines


174-188: 📐 Maintainability & Code Quality | 🟠 Major

恢复 ## 3 开放问题

当前文件从 §2.5 直接进入 ## 3.1 决议(原开放问题 §N.1)。计划没有列出这些决议所对应的 P0 问题。请在第 174 行前恢复 ## 3 开放问题,逐项标记解决状态,并保留结论、实施方案、边界条件以及文件:行号和 plan 章节双锚点。

As per coding guidelines,每份 plan 必须先列出开放问题,再追加对应的 §N.1 决议

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` around lines 174 - 188, 在“## 3.1
决议(原开放问题 §N.1)”之前恢复“## 3 开放问题”章节,列出与“决议 1”和“决议 2”对应的 P0
开放问题并明确标记已解决状态;保留现有各决议的结论、实施方案、边界与拒绝理由,以及文件:行号和 plan 章节双锚点内容不变。

Source: Coding guidelines


271-271: 📐 Maintainability & Code Quality | 🟠 Major

统一补齐两个 plan 的 §10 实施工作流

  • docs/plan-refactor-server-session-v1.md#L271-L271:追加 P0-P4 的依赖顺序、中文 atomic commit、Model: <精确模型 id> trailer、Git/stash 约束、重新 review、Finish Evidence 和归档流程。
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L65-L66:追加 P0-P5 的同类执行顺序和阶段门禁。

两个 plan 的末尾都必须包含“单次 consume-plan 全自动到 merge”,明确 /consume-plan 后最终归档到 docs/finished_plans/

As per coding guidelines,scope >= 4 个 PR 的 plan 必须在末尾包含 §10 实施工作流 和自动归档章节。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 271, 为
docs/plan-refactor-server-session-v1.md 的末尾补齐 §10 实施工作流,包含 P0-P4 依赖顺序、中文 atomic
commit、精确 Model trailer、Git/stash 约束、重新 review、Finish Evidence、每轨 docs-only
批量归档及自动归档流程;为 docs/plans-skeleton/plan-refactor-persistence-slices-v1.md 的末尾补齐同类
§10,使用 P0-P5 阶段门禁。两个文件都必须明确单次 /consume-plan 全自动执行至 merge,并最终归档到
docs/finished_plans/。

Source: Coding guidelines


259-270: 📐 Maintainability & Code Quality | 🟠 Major

为每个 P4 场景补充可执行实现锚点。

当前 P4 主要列出场景名称和断言。每个场景仍缺少具体模块路径、类型/函数、schema 或 Redis key、跨仓库契约 symbol 及 bot 脚本路径。请逐项补充实现锚点。至少覆盖 server/src/session/registry.rs::SessionRegistrySessionDeliveryOutbox、R3/R10 transaction symbol 和实际 scripts/bot/... 路径。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` around lines 259 - 270, 为 P4
清单中的每个场景补充可执行实现锚点,不要只保留场景名称和断言;至少明确相关模块路径、类型/函数、schema 或 Redis key、跨仓库契约 symbol
及实际 bot 脚本路径。重点覆盖 server/src/session/registry.rs 中的
SessionRegistry、SessionDeliveryOutbox、R3/R10 transaction symbol,并为每项补充对应
scripts/bot/... 路径。
docs/plans-skeleton/plan-refactor-master-v1.md (1)

54-58: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

补齐严格命名的 §10 实施工作流

当前 §10flash-review issue 消化流程,不是按依赖顺序序列化实施 PR 的工作流。

请在计划末尾新增严格命名的 §10 实施工作流,覆盖 Wave 0、Wave 1、Wave 2、R1-R10、V 和 registry-datafication 的 PR 顺序、依赖、原子边界、中文 commit、Model: trailer、验收证据,以及末尾的“单次 consume-plan 全自动到 merge”章节。将现有 flash-review 内容顺延到后续编号。

As per coding guidelines, scope 大于或等于 4 个 PR 的 plan 必须在末尾包含 §10 实施工作流,并按依赖顺序序列化多个 PR。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` around lines 54 - 58,
在文档末尾新增严格命名的“§10 实施工作流”,按依赖顺序覆盖 Wave 0、Wave 1、Wave 2、R1-R10、V 和
registry-datafication 的 PR 排序、依赖关系、原子边界、中文 commit、Model: trailer、验收证据,以及“单次
consume-plan 全自动到 merge”章节;将现有 flash-review issue 消化流程顺延至后续编号,并保持计划原有依赖与阶段约束。

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/plan-refactor-client-store-lifecycle-v1.md`:
- Around line 27-30: 统一三个计划文档中的 CraftStore 生命周期契约:在
docs/plan-refactor-client-store-lifecycle-v1.md:27-30 明确将同一个 CraftStore 同时写入
P0/P1 及 FQCN manifest,并规定 R6 P1 先完成创建与登记,R2 P1 后方可放行 R7 P2 使用;在
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:21-22 调整 R6 P1 交付表述,避免产生第二个状态
owner;在 docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:39 对齐 Craft
Screen 接线依赖,确保其只消费该已登记的单一 CraftStore。

In `@docs/plan-refactor-server-session-v1.md`:
- Line 133: 在标题“2.3 durability 决议矩阵”前补充一个空行,使其与前方列表内容分隔并符合 Markdown 标题格式要求。
- Around line 127-130: Freeze terminal-obligation quota until each
ResolvedDisposition payload reaches a bounded retention or compaction boundary;
update docs/plan-refactor-server-session-v1.md lines 127-130 to define this
release condition and update
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md lines 55-56 to
specify retention/compaction, a separate disposition quota, or equivalent
storage budget with atomic cleanup boundaries. Ensure quota cannot be repeatedly
reused while durable resolved payloads remain indefinitely retained.
- Line 146: 在 TsyPresence 的 coupled restore 事务中定义校验失败时的安全回退:阻止 TSY
position/dimension 恢复,或将其移动到独立可信的安全位置与维度,且不得使用损坏的 return_to 推导目标;同步更新
docs/plan-refactor-server-session-v1.md:146 和
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:45-49,将该行为写入
tsy_presence_relog_parity 与 coupled-snapshot crash acceptance。

In `@docs/plans-skeleton/plan-refactor-client-ui-base-v1.md`:
- Around line 19-25: 在 Craft Screen 的移除与共享 close callback 流程中引入明确的 close reason
或 suppression flag,区分用户关闭、显式取消、断线和 server 驱动替换;仅用户关闭发送 hydrated identity/version
的 CraftPause,显式取消发送 CraftCancel,断线或 server 替换不得发送任何带旧 identity 的 UI
intent。补充客户端单测覆盖这四种 removal 路径。
- Line 49: 统一该段对 CraftSessionStateV2 phase 的引用:先依据共享 wire/schema 契约确认 Paused 与
Suspended 是否等价且可用于 Resume;若不等价,将 Running/Suspended 调整为实际定义的 Paused
phase,确保负例覆盖真实的 Paused Resume 拒绝路径,避免使用未定义 phase。

In `@docs/plans-skeleton/plan-refactor-master-v1.md`:
- Around line 61-64: 将 CraftOpen 的 facility gate 与 facility claim 合并为由单一 owner
执行的原子检查并占用操作,使用同一事务、锁或 compare-and-claim 确保并发请求不会同时通过检查;R4 负责调用该原子流程,R1
仅接受仍有效的检查结果并在过期或占用冲突时拒绝请求。
- Around line 61-64: 在 §4 或 §6.11 为 Agent schema 交付增加唯一且可验证的版本锚点,至少明确 schema
version、commit/tag、artifact digest 或 contract sample。更新 R6、R4、R7、R1 的 craft gate
与消费说明,统一引用该锚点,并要求 TypeBox source、generated schema 与 dist 均与其匹配,避免各轨消费不同版本。
- Around line 61-64: 为 CraftPause/CraftResume 的服务端处理增加独立的调用者身份校验:从认证连接获取 caller
identity,并在状态转移前验证其为 session owner 或具备明确授权关系。不要复用或依赖 CraftOpen 的 owner
gate;将该校验纳入 R4/R10 各自的 gate 决议,同时保留现有 hydrated session identity/version 校验。
- Line 9: 补全 plan-refactor-master-v1.md 的 skeleton→active 促升证据与决议链条,在计划头部明确
server/agent/client 契约、复用的 shared type/event/schema 及 worldview.md 锚点。按
docs/CLAUDE.md §10 模板在文末新增本 plan 的实施工作流,并在对应 §10.N 明确保留单次 /consume-plan 全自动执行至
merge 并归档到 docs/finished_plans/ 的声明。
- Line 103: 将计划完成定义中的“116 个 C2S 变体全部有声明”改为“当前为 113 个,待 R6 三个 craft intent 合入后扩至
116”。不要直接宣称 116 已同步;仅在补充三个新增变体、生成来源、schema/source trace,以及 scenarios 与
handler/gate 的全量声明、矩阵和样本证据后,才可保留 116 的表述。

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Line 17: 在 docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17-17 补充 S2C
payload 与 CraftSessionStateV2 的最低协议/schema 版本门、feature gate、原子发布顺序及 old
client/new server 的 typed reject 测试;若依赖现有原子发布契约,明确引用并增加验证。在
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:17-17 使用同一版本门接入 C2S production
decode/dispatch 与 gate matrix,并覆盖 new client/old server 的拒绝测试,确保混合版本不会进入业务处理。

---

Outside diff comments:
In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Around line 12-17: 在 docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
第12-17行的“接入面”中补充准确、可核验的 worldview.md 锚点,覆盖 wire、作用域广播及跨仓库契约依据;在
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第12-17行的对应“接入面”中补充覆盖 C2S
门禁、距离、维度和所有权规则的 worldview.md 锚点。两处均需保留进料、出料、共享类型/event/schema
与跨仓库契约内容,并仅引用能支持计划范围的具体章节和行号。

---

Duplicate comments:
In `@docs/plan-refactor-server-session-v1.md`:
- Around line 1-3: Update the header of docs/plan-refactor-server-session-v1.md
at lines 1-3 by adding a complete “接入面” section covering inputs, outputs, shared
types/events/schemas, server/agent/client cross-repository contracts, and an
accurate worldview.md §X line anchor. Also update
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md at lines 13-19 with
the accurate worldview.md section and line reference, explicitly defining “zero
wire changes” as the server/agent/client contract.
- Around line 174-188: 在“## 3.1 决议(原开放问题 §N.1)”之前恢复“## 3 开放问题”章节,列出与“决议 1”和“决议
2”对应的 P0 开放问题并明确标记已解决状态;保留现有各决议的结论、实施方案、边界与拒绝理由,以及文件:行号和 plan 章节双锚点内容不变。
- Line 271: 为 docs/plan-refactor-server-session-v1.md 的末尾补齐 §10 实施工作流,包含 P0-P4
依赖顺序、中文 atomic commit、精确 Model trailer、Git/stash 约束、重新 review、Finish Evidence、每轨
docs-only 批量归档及自动归档流程;为
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md 的末尾补齐同类 §10,使用 P0-P5
阶段门禁。两个文件都必须明确单次 /consume-plan 全自动执行至 merge,并最终归档到 docs/finished_plans/。
- Around line 259-270: 为 P4
清单中的每个场景补充可执行实现锚点,不要只保留场景名称和断言;至少明确相关模块路径、类型/函数、schema 或 Redis key、跨仓库契约 symbol
及实际 bot 脚本路径。重点覆盖 server/src/session/registry.rs 中的
SessionRegistry、SessionDeliveryOutbox、R3/R10 transaction symbol,并为每项补充对应
scripts/bot/... 路径。

In `@docs/plans-skeleton/plan-refactor-master-v1.md`:
- Around line 54-58: 在文档末尾新增严格命名的“§10 实施工作流”,按依赖顺序覆盖 Wave 0、Wave 1、Wave
2、R1-R10、V 和 registry-datafication 的 PR 排序、依赖关系、原子边界、中文 commit、Model:
trailer、验收证据,以及“单次 consume-plan 全自动到 merge”章节;将现有 flash-review issue
消化流程顺延至后续编号,并保持计划原有依赖与阶段约束。
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: c5657253-954d-4694-ac76-5f261109eccd

📥 Commits

Reviewing files that changed from the base of the PR and between 5e96338 and 2a00d04.

📒 Files selected for processing (8)
  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
💤 Files with no reviewable changes (1)
  • docs/plans-skeleton/plan-refactor-server-session-v1.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md

📄 CodeRabbit inference engine (docs/CLAUDE.md)

docs/**/*.md: 新 plan 头部必须写明接入面:进料、出料、复用的共享类型/event/schema、server/agent/client 跨仓库契约,以及对应的 worldview.md 锚点。
涉及真元、灵气、衰减、逸散、半衰、距离损耗、排斥或吸力的 plan,必须调用 qi_physics;新物理常数和公式必须先扩展 qi_physics,不得在功能 plan 中重复实现。
所有真元/灵气流动必须遵守守恒律并通过 qi_physics::ledger::QiTransfer;释放使用 qi_release_to_zone,吸收使用 qi_excretion,不得凭空生成或销毁真元。
涉及玩家可感知行为的 plan,必须在对应机制阶段中内联可直接实现的粒子、音效、HUD、环境、动画和 narration 规格;不得将视听内容笼统推迟到独立阶段。纯 server 逻辑 plan 例外。
每份 plan 必须列出开放问题;实施前必须追加 §N.1 决议,逐项给出结论、实施方案、边界条件,并以文件:行号和 plan 章节双锚点落地。
scope 大于或等于 4 个 PR 的 plan,必须在末尾包含 §10 实施工作流,并按依赖顺序在一个 plan 内序列化多个 PR,不得拆成多个 plan。
涉及 NBT 建筑、worldgen layout 或复杂视觉资产的 TODO,必须完成三轮提交:(round 1/3)(round 2/3)(round 3/3);终轮提交必须包含拼写准确的 <PROMISE> 担保块。
§10 最末必须包含“单次 consume-plan 全自动到 merge”章节,明确用户提交 /consume-plan 后即可等待最终归档至 docs/finished_plans/

引用世界观内容时统一使用 worldview.md §X L<line> 格式;不得自动修改 docs/worldview.md 或主动回写 docs/library/

Files:

  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-server-session-v1.md
**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: commit message 必须使用中文,每个逻辑单元一个 atomic commit;agent 生成的 commit 必须带真实模型 Model: <精确模型 id> trailer。
禁止使用 --no-verify--no-gpg-sign、关闭签名配置、未经确认的 force push、hard reset、amend 或交互式 rebase。
禁止留下 auto-stash 产生的孤儿 WIP stash;自动 stash 流程完成后必须恢复自己的 stash。

Files:

  • docs/plan-refactor-client-store-lifecycle-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-server-session-v1.md
docs/plans-skeleton/**/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

新 plan 必须先读取 docs/CLAUDE.md;骨架、Active 和 Finished plan 必须遵循三态流转及规定的阶段状态、Finish Evidence 结构。

Files:

  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
🧠 Learnings (3)
📚 Learning: 2026-07-17T00:31:10.779Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:1-1
Timestamp: 2026-07-17T00:31:10.779Z
Learning: 在审查该仓库 `docs/plans-skeleton/` 下的“docs-only skeleton plan”创建类 PR 时:先核对 `docs/CLAUDE.md` 中“Plan 消费规范”,并逐份查看本计划文档里的“§10 实施工作流”,确认后续实施阶段是否会遵循“每个 PR 只修改一个 plan”,且实施/归档时不会出现跨 plan 的修改。该规则不适用于用户显式指定、共享调研基线且计划之间存在互相交叉引用的 skeleton plan 同批创建 PR;对这类情况应按实际交叉引用关系放宽,确保仍能按独立或约定的序列化方式推进。

Applied to files:

  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
📚 Learning: 2026-07-17T00:31:13.643Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:81-85
Timestamp: 2026-07-17T00:31:13.643Z
Learning: 审核 docs/plans-skeleton/*.md 下的 skeleton 草案 PR 时:不得要求作者在“§N 开放问题(P0 决策门前需收口)”核查完成之前提前填写对应的“§N.1 决议”。仅当计划进入 active 且 P0 实施前,已由 Explore agent 并行核查代码现状后,才允许追加“§N.1 决议”,且该决议需包含结论、实施方案、边界条件,并使用“文件:行号 + plan 章节”的双锚点格式。

Applied to files:

  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
📚 Learning: 2026-07-22T02:11:59.191Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1247
File: docs/plans-skeleton/plan-bughunt-animal-air-spawn-gravity-v1.md:0-0
Timestamp: 2026-07-22T02:11:59.191Z
Learning: 在 `docs/plans-skeleton/` 目录下的计划状态行中,若起草日期同时涉及 UTC 与本地日期(例如时区换算后可能跨到不同日期),请在该行中同时标注 UTC 起草日与本地起草日。这样可以避免基于 UTC 的时间基准在 GitHub/CodeRabbit 审查时将本地日期误判为“未来日期”。

Applied to files:

  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
🪛 LanguageTool
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md

[grammar] ~45-~45: Ensure spelling is correct
Context: ...sfer_resync:bot 跨位面→断言 env/season/zone 大气全量重发。 3. wire_contract_sweep`:对 144 S2C 变体的 sample 对拍在 CI 常绿(配 proto-br...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/plan-refactor-server-session-v1.md

[grammar] ~96-~96: Ensure spelling is correct
Context: ...connected_clients及 R3 player save 前接收 disconnect。 2.Checkpointed`:停止 tick → 写 checkpoint(包含 escrow,但不执行 refu...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)


[grammar] ~246-~246: Ensure spelling is correct
Context: ... volatile adapter;断线/跨维/关服不再 tick 或结算离线 actor。 - qi refund/release 测试从 SPIRIT_QI_TOTAL 与 ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🪛 markdownlint-cli2 (0.23.1)
docs/plan-refactor-server-session-v1.md

[warning] 133-133: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above

(MD022, blanks-around-headings)

🔇 Additional comments (7)
docs/plan-refactor-server-session-v1.md (3)

72-75: 🗄️ Data Integrity & Integration

统一 SessionPhase 的状态集合。

SessionPhase 当前包含六个 variant:RunningPausedSuspendedAwaitingDeliveryDeliveryPendingTerminal。PR objective 声明冻结五态生命周期,但本文件将 DeliveryPending 也定义为 phase。请在 P0 冻结前明确它是第六个 durable handoff state,还是应移到独立的 delivery state,并同步更新 transition、consumer contract 和 acceptance pins。


5-71: LGTM!

Also applies to: 76-126, 131-132, 134-145, 147-173, 189-258


127-130: 🔒 Security & Privacy

Sensitive Data Exposure (CWE-359)

补充完整 delivery payload 的访问控制和 retention 合同。

  • docs/plan-refactor-server-session-v1.md#L127-L130:为 ResolvedDisposition 的 payload 定义 operator read ACL、tenant isolation、访问审计和 erasure deadline。
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L55-L56:不要把完整 payload 的保留边界完全延期到后续阶段;至少冻结一个可验证的最小保护合同。
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md (1)

21-44: LGTM!

Also applies to: 49-54, 58-64

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md (1)

28-34: LGTM!

Also applies to: 38-40, 46-47, 53-54

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md (1)

3-7: LGTM!

Also applies to: 36-36, 40-43

docs/plans-skeleton/plan-refactor-master-v1.md (1)

54-58: 🗄️ Data Integrity & Integration

无需修改当前总纲的 R10 deliver 表述。

plan-refactor-server-session-v1.md 已明确 R10 P1 deliver 由库存核心冻结 contract,R10 P2 把 craft/alchemy/forge production delivery 迁入执行点;R3 只负责 durable outbox handoff 与 receipt 预留,不定义 InventoryTxn::deliver,不会形成 R1↔R10 循环。

Comment thread docs/plan-refactor-client-store-lifecycle-v1.md
Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plan-refactor-server-session-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-client-ui-base-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-client-ui-base-v1.md
Comment thread docs/plans-skeleton/plan-refactor-master-v1.md
Comment thread docs/plans-skeleton/plan-refactor-master-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-master-v1.md Outdated
Comment thread docs/plans-skeleton/plan-refactor-wire-s2c-v1.md Outdated
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: infrastructure_failure
Reviewed head: 2a00d04c30099898425c87969d56a20d012329c4
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

The review could not complete safely. No approval or code finding was inferred from the failed stages.

  • summary:server-session-plan-part-3schema_error: structured output failed schema validation

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: 2a00d04c30099898425c87969d56a20d012329c4
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Register the mandatory Agent craft-schema producer in the plan graph

docs/plans-skeleton/plan-refactor-master-v1.md:63 · wiring

The changed ownership rule says an independent Agent-track craft-schema batch "must" land before R6 P1 and must deliver the TypeBox source, generated schema/dist, and variant count. However, the actual Agent-track inventory in §6.11 (line 84) contains only the pre-existing named active/skeleton plans and does not contain a craft-schema plan or phase, while the wave schedule (lines 53-55) also schedules no Agent craft-schema work. Consequently the documented production chain has no actionable owner that can produce R6's prerequisite: R6 is required to consume frozen artifacts it is forbidden to edit, so R6 P1 and every downstream CraftOpen/Pause/Resume path remain blocked. The surrounding R6/R4/R7/R1 assignments cannot fill this gap because this same line explicitly reserves all source/generated/dist files to the absent Agent delivery.

Root cause: the plan introduces a mandatory cross-track producer as an informal "independent batch" but does not register that batch as a concrete agent plan/phase in the master coverage and wave schedule, leaving the first producer in the craft lifecycle wiring chain unscheduled.

[major] Payload boundary test rejects the contract's valid maximum

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:49 · schema-contracts

The plan first defines the payload contract as payload may grow up to, but not beyond, a reservation of SESSION_DELIVERY_MAX_PAYLOAD_BYTES (line 16). The new acceptance scenario then says to use a payload that "达到 SESSION_DELIVERY_MAX_PAYLOAD_BYTES 的边界" and assert that it is rejected as over-limit. Thus the exact maximum, which the contract permits, is described as the rejection case; there is no distinct MAX + 1 input. An implementation that rejects a payload of exactly the reserved size would satisfy this acceptance wording while violating the stated <= reservation contract.

Root cause: the payload-size acceptance case conflates the inclusive maximum with the first invalid value, leaving the serialization/storage boundary internally contradictory instead of pinning max as accepted and max + 1 as rejected.

[major] Final acceptance gate omits the three newly required bot scenarios

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:64 · correctness

The changed acceptance section says the bot e2e gate covers only restart_player_slices, restart_world_runtime, load_failure_guard, and tick_rebase. However, this same changed plan defines tsy_presence_relog_parity, tsy_presence_snapshot_atomicity, and session_delivery_outbox_atomicity as bot acceptance scenarios at lines 46-48, and P1 at line 23 requires the coupled-snapshot and outbox crash/concurrency evidence. An implementation can therefore satisfy the document's final stated acceptance boundary while never running the three tests that prove the newly added contracts.

Root cause: the final acceptance declaration was not updated when the plan added three mandatory bot scenarios, leaving two conflicting definitions of completion in the same document.

[major] Losing a concurrent busy-claim race permanently leaks delivery quota

docs/plan-refactor-server-session-v1.md:128 · concurrency-atomicity

The new contract requires every checkpointed session to reserve durable terminal-obligation capacity before obtaining its gameplay claim ("在取得 claim 前预留"), while SessionRegistry::try_acquire(BusyClaim) is the separate, authoritative conflict point. With two concurrent admissions for different SessionKeys targeting the same furnace/station, both quota reservations can commit, but only one try_acquire can succeed. The losing request never becomes a session and can never produce a delivery receipt or ResolvedDisposition; nevertheless, this same paragraph says quota is released only after those terminal outcomes. Its reservation therefore remains permanently charged. Repeating losing admissions can exhaust row/byte quota and make unrelated checkpointed admission/restore fail closed. The listed concurrency tests cover competition for the last quota slot and later release after terminal delivery, but not rollback when reservation succeeds and busy-claim acquisition loses, so the stated acceptance suite does not prevent this state.

Root cause: admission is split across a durable quota reservation and an in-memory busy-claim acquisition without a shared linearization/rollback contract. the plan mandates reservation first but defines reservation release only for terminal delivery outcomes, omitting cancellation of an unconsumed reservation when claim acquisition or subsequent admission validation fails.

[major] Terminal delivery outbox has no planned production consumer

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:24 · wiring

The changed R3 P1 deliverable creates SessionDeliveryOutbox, SessionDeliveryQuota, and SessionDeliveryReservation and tests only persistence-side reservation/handoff/crash atomicity (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:24, with the named scenario at line 49). The promoted R1 plan says R10 must asynchronously consume that outbox, atomically commit inventory/spill plus DeliveryCommitReceipt, and advance the durable state (docs/plan-refactor-server-session-v1.md:87, :126, :224, :227). However, the canonical R10 plan's complete phase list contains only inventory txn/capacity, writer migration, pickup/merge, and e2e (docs/plan-refactor-inventory-core-v1.md:4-10); its InventoryTxn API has no outbox claim/ack or receipt input (:24-33), its ownership section merely labels R3 as owning “SQL/outbox” (:87-97), and its required pins/e2e exercise direct deliver calls rather than a SessionDeliveryOutbox worker (:99-124). Thus all stated R3 and R10 phase deliverables can be implemented while no production consumer ever claims a terminal session obligation. A completed, expired, or invalid-restored checkpointed session then remains DeliveryPending indefinitely and its output/refund is never delivered. The R3 atomicity tests cannot catch that missing consumer because they stop at durable handoff, while R10 has no corresponding outbox-to-receipt acceptance item.

Root cause: the diff adds the durable terminal-delivery producer and storage contract to r3, but the cross-track plan set never assigns the matching production outbox worker, cas/lease processing, receipt commit, and acknowledgement deliverables to an implementable r10 phase. references to a future “r10 durable-delivery” window do not create that consumer in r10's canonical scope.

[major] Restore retries cannot reuse the durable terminal reservation

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:15 · concurrency-atomicity

A normally admitted checkpointed session first creates SessionDeliveryReservation { session_key, ... }, and that reservation remains durable until terminal handoff. After a process restart, restoring that same nonterminal session reaches the newly specified admission/restore path, which unconditionally performs the quota conditional update and then inserts a reservation whose session_key must be unique. The persisted row already has that key, so the insert fails and the stated all-or-nothing transaction rolls back; the guarded restore cannot reacquire the session even though its capacity is already reserved. Treating the duplicate as a fresh success instead would overcount quota unless the API explicitly distinguishes reuse from allocation. The acceptance cases on line 48 cover concurrent competition for free capacity but not idempotent restore with an existing reservation, so they do not resolve this path. This reservation protocol is newly added by the diff and breaks the plan's checkpointed restart/reconnect contract.

Root cause: reserve_terminal_obligation(session_key) uses a create-only unique insert for both first admission and restart restore, but restore is a retry over an already persisted reservation. the contract lacks an atomic idempotent branch that validates and reuses the existing reservation without incrementing quota.

[major] Craft migration is allowed before workbench identity can survive restart

docs/plans-skeleton/plan-refactor-master-v1.md:55 · strict-maintainability

The new Wave 2 gate allows the craft host migration after R3 P1 plus the wire/UI/delivery prerequisites, but it does not wait for R3's placeable-entity-restart-loss work, which remains assigned to R3 P4 (docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:27,33). Today a workbench is identified only by a process-local Bevy entity (server/src/craft/workbench.rs:38-43,141-143), while the placeable persistence plan explicitly requires a new stable placed_id, forbids persisting Bevy Entity, and recreates a new runtime entity during hydration (docs/plans-skeleton/plan-bughunt-placeable-entity-restart-loss-v1.md:63-71,80-86). Thus the permitted sequence R3 P1 -> R1 craft migration leaves no stable facility identity or restored workbench after shutdown. A checkpointed workbench craft session cannot rebind its retained facility claim on restart and will fail restore/refund instead of satisfying the newly stated restart-recovery contract. The surrounding gate lists every other craft prerequisite but neither the R3 P4 placed-entity prerequisite nor a stable placed-ID bridge.

Root cause: the dependency graph treats a runtime entity::to_bits() workbench locator as sufficient for a checkpointed facility claim, but checkpoint recovery requires the separately planned stable placed-entity identity and hydration work. that dependency and the mapping from stable placed_id to the new runtime entity are absent from the craft migration gate.

server_kizuna and others added 2 commits August 4, 2026 18:23
以唯一 reservation 状态矩阵消除额度泄漏和重复计量,并为 craft schema、稳定工作台身份及 terminal outbox 建立可执行的生产消费与验收顺序。

Model: claude-sonnet-5
Co-Authored-By: Claude <noreply@anthropic.com>
保留生命周期与生产消费表为唯一权威,将重复规则收敛为规范性引用,使结构整改保持可执行且控制文档体量。

Model: claude-sonnet-5
Co-Authored-By: Claude <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (7)
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md (3)

13-19: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

补齐 plan header 的 worldview.md 锚点。

## 接入面 已列出进料、出料、共享类型和跨仓库契约,但没有 worldview.md §X L<line> 锚点。请补充准确的章节和行号,不要自动修改 docs/worldview.md

As per coding guidelines,新 plan 头部必须写明完整接入面和对应的 worldview.md 锚点。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-persistence-slices-v1.md` around lines 13 -
19, 在计划头部的“接入面”部分补充准确的 docs/worldview.md 章节与行号锚点,覆盖该计划涉及的接入契约;不要修改
docs/worldview.md 本身,并保留现有进料、出料、共享类型和跨仓库契约内容。

Source: Coding guidelines


57-58: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

在释放 obligation quota 前冻结有界 retention。

ResolvedDisposition 必须保留完整 payload,但当前计划允许在释放 quota 后无限保留 durable disposition。这样 admission quota 可以重复复用,而 disposition payload 持续占用数据库容量。

  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L57-L58:定义 bounded retention/compaction、独立 disposition quota 或等价 storage budget。
  • docs/plan-refactor-server-session-v1.md#L127-L130:把 quota release 绑定到已完成的 retention/compaction 边界。
  • docs/plan-refactor-inventory-core-v1.md#L57-L57:让 worker、scanner 和 operator resolve 遵守同一释放条件。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-persistence-slices-v1.md` around lines 57 -
58, 在 docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:57-58 中更新
ResolvedDisposition 设计:在释放 obligation quota 前明确有界 retention/compaction、独立
disposition quota 或等价 storage budget。同步修改
docs/plan-refactor-server-session-v1.md:127-130,使 quota release 绑定已完成的
retention/compaction 边界;修改 docs/plan-refactor-inventory-core-v1.md:57,使
worker、scanner 与 operator resolve 统一遵守该释放条件。

16-16: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

移除接入面中的旧 reservation API。

Line 16 仍要求调用 reserve_terminal_obligation(session_key),并描述 create-only 的 +Q 行为。Line 25 又明确废止该语义,改用 reserve_new_terminal_obligationreuse_terminal_obligationcancel_unconsumed_reservation

请直接更新接入面文本。否则实现者可能在 restore/retry 时重复增加 quota,或遗漏 busy loser 的 -Q

Also applies to: 25-25

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-persistence-slices-v1.md` at line 16,
更新接入面中描述 terminal obligation reservation 的文字,移除旧的
reserve_terminal_obligation(session_key) 及其仅执行 +Q 的 create-only 语义。改为明确使用
reserve_new_terminal_obligation、reuse_terminal_obligation 和
cancel_unconsumed_reservation,并保留 restore/retry 不重复增加 quota、busy loser 正确执行 -Q
的行为。
docs/plan-refactor-server-session-v1.md (2)

77-77: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

统一 outbox 状态集合。

DeliveryOutboxState 在 Line 77 只定义了 PendingInFlightDeadLetterCommittedResolvedDisposition。但 Line 145-148 又使用 CommittedReleasedDispositionReleased 作为持久状态。

请选择一个唯一的持久状态模型。否则 worker、恢复和 quota replay 无法确定应读写哪个状态。若 Released 是派生状态,请改为独立的 release 标记;若它是持久状态,请加入 enum 并补齐转移规则。

Also applies to: 145-148

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 77, 统一 DeliveryOutboxState
的持久状态模型:在 DeliveryOutboxState 及 Line 145-148 的 worker、恢复和 quota replay 语义中,明确
CommittedReleased 与 DispositionReleased 是派生状态还是持久 enum 成员;若为持久状态,将其加入 enum
并补齐所有状态转移及 (state,generation) CAS 规则,若为派生状态则改用独立 release 标记,并确保所有读写路径采用同一模型。

191-191: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

改用 R10 的唯一 deliver 合同。

Line 191 冻结了 deliver(delivery_id, items) -> Delivered | Spilled(fallback)。但 docs/plan-refactor-inventory-core-v1.md Line 28-38 的 canonical contract 是 InventoryTxn::deliver(DeliveryRequest, Option<&mut SpillContext>) -> Result<InventoryDeliveryReceipt, InventoryTxnError>

请删除第二套 API 表述,并明确 delivery_id 如何进入 DeliveryRequest、如何生成 receipt,以及如何处理 spill。否则 R1 与 R10 会实现不兼容的生产接口。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 191, 更新退款交付表述,统一采用 R10 的唯一
`InventoryTxn::deliver(DeliveryRequest, Option<&mut SpillContext>) ->
Result<InventoryDeliveryReceipt, InventoryTxnError>` 合同;删除 `deliver(delivery_id,
items) -> Delivered | Spilled(fallback)` 这套并行 API,并明确将 `delivery_id` 写入
`DeliveryRequest`、由成功调用生成 `InventoryDeliveryReceipt`,以及通过 `SpillContext` 处理
spill。
docs/plans-skeleton/plan-agent-craft-schema-v1.md (1)

40-47: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

补充“开放问题”章节。

当前计划直接进入验收和 Finish Evidence,没有列出未决问题。请先列出仍需在 P0 前收口的边界、依赖和验收决策。不要在 skeleton 阶段提前填写 §N.1 决议;进入 active 且完成 P0 前核查后,再补充结论、实施方案、边界条件和双锚点。

Based on learnings,当前阶段只需补充开放问题,不要求现在填写 §N.1 决议

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-agent-craft-schema-v1.md` around lines 40 - 47,
在“验收与边界”之前补充“开放问题”章节,列出进入 P0 前仍需收口的边界、依赖和验收决策。仅记录待决事项,不提前填写“§N.1 决议”;待计划进入
active 并完成 P0 核查后,再补充结论、实施方案、边界条件和双锚点。

Sources: Coding guidelines, Learnings

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md (1)

53-54: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

在两个多 PR plan 末尾补齐 §10 实施工作流

当前两个 plan 都在多阶段交付后直接结束,没有定义依赖顺序、提交约束、重新 review 和自动归档流程。

  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md#L53-L54: 追加 P0-P5 的序列化实施流程,以及每个 PR 只修改一个 plan、中文 atomic commit、Model: <精确模型 id> trailer、危险 Git 操作禁止项和自动归档规则。
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md#L40-L44: 追加 P0-P4 的相同工作流,并明确 gate matrix 验收后再进入归档。
  • 两个 plan 都必须在 §10 最末包含“单次 consume-plan 全自动到 merge”。

As per coding guidelines,多 PR plan 必须包含 §10 实施工作流

Based on learnings,实施和归档默认保持每个 PR 只修改一个 plan。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md` around lines 53 - 54, 在
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md 第53-54行末尾追加 §10 实施工作流,定义 P0-P5
的依赖顺序、每个 PR 只修改一个 plan、中文 atomic commit、精确的 “Model: <模型 id>” trailer、禁止危险 Git
操作、重新 review 与自动归档,并以“单次 consume-plan 全自动到 merge”收尾;在
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第40-44行末尾追加对应的 §10,覆盖 P0-P4,并明确
gate matrix 验收后才能归档及同样的提交约束和自动流程。

Sources: Coding guidelines, Learnings

♻️ Duplicate comments (3)
docs/plan-refactor-server-session-v1.md (2)

291-291: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

补齐三份计划末尾的 §10 实施工作流

这些计划均包含多个 PR 阶段,但末尾没有依赖顺序、中文 atomic commit、Model: <精确模型 id> trailer、重新 review 和自动归档流程。Deferred-to-implementation-wave 不能替代实施工作流。

  • docs/plan-refactor-server-session-v1.md#L291-L291:在文件末尾新增 §10 实施工作流
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L160-L171:将 deferred decisions 与实施顺序分开,并补齐 workflow。
  • docs/plan-refactor-inventory-core-v1.md#L160-L171:补齐 P1-P4、P2a/P2b/P2c 的依赖序列和归档规则。

§10 最末必须包含“单次 consume-plan 全自动到 merge”,明确用户提交 /consume-plan 后可等待最终归档至 docs/finished_plans/

As per coding guidelines,scope 大于或等于 4 个 PR 的 plan 必须在末尾包含 §10 实施工作流

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 291, 在
docs/plan-refactor-server-session-v1.md 第291行末尾新增 §10 实施工作流;在
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md 第160-171行分离 deferred
decisions 与实施顺序并补齐该工作流;在 docs/plan-refactor-inventory-core-v1.md 第160-171行补充
P1-P4、P2a/P2b/P2c 的依赖序列和归档规则。三份 §10 均需包含依赖顺序、中文 atomic commit、精确的 Model: <模型 id>
trailer、重新 review、自动归档,以及“单次 consume-plan 全自动到 merge”,明确提交 /consume-plan
后可持续至最终归档到 docs/finished_plans/。

Source: Coding guidelines


165-165: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

在 coupled restore 中定义 TsyPresence 失败回退。

当前验收只要求缺失或非法 presence 时不 attach 并拒绝 TSY 请求。它没有冻结 position/dimension 是否仍会恢复。这样可能形成“玩家位于 TSY,但没有有效 presence”的断裂状态。

  • docs/plan-refactor-server-session-v1.md#L165-L165:presence 校验失败时,禁止恢复 TSY position/dimension,或移动到独立可信的安全位置和维度。
  • docs/plan-refactor-server-session-v1.md#L288-L288:为四类 guarded-load 失败增加上述回退断言。
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md#L49-L50:在同一恢复事务中验证回退。不得从损坏的 return_to 推导目标。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plan-refactor-server-session-v1.md` at line 165, 问题:coupled restore 在
TsyPresence 校验失败时未定义位置与维度的安全回退,可能恢复出位于 TSY 但无有效 presence 的状态。更新
docs/plan-refactor-server-session-v1.md:165 的 TsyPresence/guarded restore
规则,明确失败时禁止恢复 TSY position/dimension,或仅移动到独立可信的安全位置和维度;在
docs/plan-refactor-server-session-v1.md:288 的四类 guarded-load 失败验收中加入该回退断言;在
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:49-50
明确回退必须在同一恢复事务中完成验证,且不得从损坏的 return_to 推导目标。
docs/plans-skeleton/plan-refactor-master-v1.md (1)

9-9: 📐 Maintainability & Code Quality | 🟠 Major

在计划头部补齐接入面与跨仓契约。

当前头部没有列出进料、出料、复用的 shared type/event/schema、server/agent/client 跨仓契约,也没有 worldview.md §X L<line> 锚点。请在 §0 前增加“接入面与冻结合同”小节。

As per coding guidelines,docs/**/*.md 新 plan 头部必须写明进料、出料、复用的共享类型/event/schema、跨仓契约和 worldview.md 锚点。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` at line 9, 在计划正文的 §0
之前新增“接入面与冻结合同”小节,明确列出计划的进料、出料、复用的 shared type/event/schema、server/agent/client
跨仓契约,并补充对应的 worldview.md §X L<line> 锚点;保留现有 R1-R10 修改范围及 §6.11-6.12 独立交付约束不变。

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/plan-refactor-inventory-core-v1.md`:
- Line 55: 在 docs/plan-refactor-inventory-core-v1.md 第55行定义唯一的 terminal-delivery
transaction coordinator API,并明确 R10 worker 如何加入同一 R3 durable transaction,涵盖
rollback、ack、quota release 责任。同步更新 docs/plan-refactor-server-session-v1.md
第240-244行统一 R3/R10 ownership 与上述协调边界;更新
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md 第24行,使 P1 acceptance
引用同一 transaction boundary,避免双重 durable commit。

In `@docs/plans-skeleton/plan-agent-craft-schema-v1.md`:
- Around line 14-17: 补齐三个 plan 的接入面锚点:在
docs/plans-skeleton/plan-agent-craft-schema-v1.md 第14-17行的“接入面与冻结合同”中加入
server/agent/client 跨仓库契约、复用的共享 schema/type/event,以及对应的 worldview.md §X L<line>
锚点;在 docs/plans-skeleton/plan-refactor-wire-s2c-v1.md 第12-17行和
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第12-17行分别加入对应的 worldview.md §X
L<line> 锚点,并确保新增 plan 的接入面字段完整。

In `@docs/plans-skeleton/plan-refactor-master-v1.md`:
- Around line 53-58: 在计划文档中新增严格命名的“§10 实施工作流”章节,按依赖顺序编排并序列化所有实施 PR,覆盖 scope 不少于
4 个 PR 的执行流程;将现有 flash-review issue 消化流程顺延至后续章节,并在“§10 实施工作流”末尾明确单次
`/consume-plan` 可自动执行至合并并归档到 `docs/finished_plans/`。
- Around line 64-65: 明确要求 Craft admission 校验与 BusyClaim 获取由单一 owner 通过同一事务、锁或
compare-and-claim 原子完成,覆盖 R4 的实体存活、WorkbenchBlock、同维、距离、owner、busy、facility 检查及
R1 的 try_acquire/BusyClaim 映射;任一检查结果过期或发生占用冲突时必须拒绝请求,不能依赖 race tests 替代原子性约束。
- Line 64: 为 CraftPause 和 CraftResume 定义并实现独立的 R4/R10 授权 gate,在执行状态转移前校验认证连接的
caller identity 属于目标 session 的 owner 或具备明确授权关系;不要依赖 CraftOpen 的 owner
gate,也不要仅信任请求中的 session identity/version。更新协议与处理流程以传递并验证所需 caller/session
归属信息,并确认两个 handler 在授权失败时拒绝请求且不会修改会话状态。
- Line 64: Update the CraftPause/CraftResume contract wording in the plan so
“hydrated session identity/version” uses the exact required fields “session_key
+ generation” defined by plan-agent-craft-schema-v1.md §4; ensure it continues
to state that target is not included and aligns the R6, R4, R7, and R1 payload
terminology.
- Line 75: 更新计划中所有使用“116”的 C2S 变体数量记录,明确区分当前 registry 基线 113 与合入三个新增 craft
intent 后的目标 116,并列出这三个变体及 generated registry、SHA 和 sample 证据。同步为“144
S2C”补充同类生成证据,覆盖 `craft proto/Rust mirror/converter/samples` 条目及其他对应位置,避免在
A-CS/R6 尚未合入时将目标值误报为当前值。

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Line 17: Define and apply one unified protocol/schema version gate for S2C,
CraftSessionStateV2, and the new Craft intents: document the minimum versions
and feature gate, specify atomic publication order (or cite the deployment
contract and add verification), and require typed rejection before
decode/dispatch for incompatible peers. In
docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:17-17, cover S2C/intent
producers plus tests rejecting old clients; in
docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:17-17, apply the same gate to
production C2S decode/dispatch and test both new-client/old-server rejection
paths.

---

Outside diff comments:
In `@docs/plan-refactor-server-session-v1.md`:
- Line 77: 统一 DeliveryOutboxState 的持久状态模型:在 DeliveryOutboxState 及 Line 145-148 的
worker、恢复和 quota replay 语义中,明确 CommittedReleased 与 DispositionReleased 是派生状态还是持久
enum 成员;若为持久状态,将其加入 enum 并补齐所有状态转移及 (state,generation) CAS 规则,若为派生状态则改用独立
release 标记,并确保所有读写路径采用同一模型。
- Line 191: 更新退款交付表述,统一采用 R10 的唯一 `InventoryTxn::deliver(DeliveryRequest,
Option<&mut SpillContext>) -> Result<InventoryDeliveryReceipt,
InventoryTxnError>` 合同;删除 `deliver(delivery_id, items) -> Delivered |
Spilled(fallback)` 这套并行 API,并明确将 `delivery_id` 写入 `DeliveryRequest`、由成功调用生成
`InventoryDeliveryReceipt`,以及通过 `SpillContext` 处理 spill。

In `@docs/plans-skeleton/plan-agent-craft-schema-v1.md`:
- Around line 40-47: 在“验收与边界”之前补充“开放问题”章节,列出进入 P0
前仍需收口的边界、依赖和验收决策。仅记录待决事项,不提前填写“§N.1 决议”;待计划进入 active 并完成 P0
核查后,再补充结论、实施方案、边界条件和双锚点。

In `@docs/plans-skeleton/plan-refactor-persistence-slices-v1.md`:
- Around line 13-19: 在计划头部的“接入面”部分补充准确的 docs/worldview.md
章节与行号锚点,覆盖该计划涉及的接入契约;不要修改 docs/worldview.md 本身,并保留现有进料、出料、共享类型和跨仓库契约内容。
- Around line 57-58: 在
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:57-58 中更新
ResolvedDisposition 设计:在释放 obligation quota 前明确有界 retention/compaction、独立
disposition quota 或等价 storage budget。同步修改
docs/plan-refactor-server-session-v1.md:127-130,使 quota release 绑定已完成的
retention/compaction 边界;修改 docs/plan-refactor-inventory-core-v1.md:57,使
worker、scanner 与 operator resolve 统一遵守该释放条件。
- Line 16: 更新接入面中描述 terminal obligation reservation 的文字,移除旧的
reserve_terminal_obligation(session_key) 及其仅执行 +Q 的 create-only 语义。改为明确使用
reserve_new_terminal_obligation、reuse_terminal_obligation 和
cancel_unconsumed_reservation,并保留 restore/retry 不重复增加 quota、busy loser 正确执行 -Q
的行为。

In `@docs/plans-skeleton/plan-refactor-wire-s2c-v1.md`:
- Around line 53-54: 在 docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
第53-54行末尾追加 §10 实施工作流,定义 P0-P5 的依赖顺序、每个 PR 只修改一个 plan、中文 atomic commit、精确的
“Model: <模型 id>” trailer、禁止危险 Git 操作、重新 review 与自动归档,并以“单次 consume-plan 全自动到
merge”收尾;在 docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第40-44行末尾追加对应的
§10,覆盖 P0-P4,并明确 gate matrix 验收后才能归档及同样的提交约束和自动流程。

---

Duplicate comments:
In `@docs/plan-refactor-server-session-v1.md`:
- Line 291: 在 docs/plan-refactor-server-session-v1.md 第291行末尾新增 §10 实施工作流;在
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md 第160-171行分离 deferred
decisions 与实施顺序并补齐该工作流;在 docs/plan-refactor-inventory-core-v1.md 第160-171行补充
P1-P4、P2a/P2b/P2c 的依赖序列和归档规则。三份 §10 均需包含依赖顺序、中文 atomic commit、精确的 Model: <模型 id>
trailer、重新 review、自动归档,以及“单次 consume-plan 全自动到 merge”,明确提交 /consume-plan
后可持续至最终归档到 docs/finished_plans/。
- Line 165: 问题:coupled restore 在 TsyPresence 校验失败时未定义位置与维度的安全回退,可能恢复出位于 TSY 但无有效
presence 的状态。更新 docs/plan-refactor-server-session-v1.md:165 的
TsyPresence/guarded restore 规则,明确失败时禁止恢复 TSY
position/dimension,或仅移动到独立可信的安全位置和维度;在
docs/plan-refactor-server-session-v1.md:288 的四类 guarded-load 失败验收中加入该回退断言;在
docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:49-50
明确回退必须在同一恢复事务中完成验证,且不得从损坏的 return_to 推导目标。

In `@docs/plans-skeleton/plan-refactor-master-v1.md`:
- Line 9: 在计划正文的 §0 之前新增“接入面与冻结合同”小节,明确列出计划的进料、出料、复用的 shared
type/event/schema、server/agent/client 跨仓契约,并补充对应的 worldview.md §X L<line>
锚点;保留现有 R1-R10 修改范围及 §6.11-6.12 独立交付约束不变。
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 39fc7b5e-25b4-4997-9916-3941bdd44ffa

📥 Commits

Reviewing files that changed from the base of the PR and between 2a00d04 and dfe84d9.

📒 Files selected for processing (7)
  • docs/plan-refactor-inventory-core-v1.md
  • docs/plan-refactor-server-session-v1.md
  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.md

📄 CodeRabbit inference engine (docs/CLAUDE.md)

docs/**/*.md: 新 plan 头部必须写明接入面:进料、出料、复用的共享类型/event/schema、server/agent/client 跨仓库契约,以及对应的 worldview.md 锚点。
涉及真元、灵气、衰减、逸散、半衰、距离损耗、排斥或吸力的 plan,必须调用 qi_physics;新物理常数和公式必须先扩展 qi_physics,不得在功能 plan 中重复实现。
所有真元/灵气流动必须遵守守恒律并通过 qi_physics::ledger::QiTransfer;释放使用 qi_release_to_zone,吸收使用 qi_excretion,不得凭空生成或销毁真元。
涉及玩家可感知行为的 plan,必须在对应机制阶段中内联可直接实现的粒子、音效、HUD、环境、动画和 narration 规格;不得将视听内容笼统推迟到独立阶段。纯 server 逻辑 plan 例外。
每份 plan 必须列出开放问题;实施前必须追加 §N.1 决议,逐项给出结论、实施方案、边界条件,并以文件:行号和 plan 章节双锚点落地。
scope 大于或等于 4 个 PR 的 plan,必须在末尾包含 §10 实施工作流,并按依赖顺序在一个 plan 内序列化多个 PR,不得拆成多个 plan。
涉及 NBT 建筑、worldgen layout 或复杂视觉资产的 TODO,必须完成三轮提交:(round 1/3)(round 2/3)(round 3/3);终轮提交必须包含拼写准确的 <PROMISE> 担保块。
§10 最末必须包含“单次 consume-plan 全自动到 merge”章节,明确用户提交 /consume-plan 后即可等待最终归档至 docs/finished_plans/

引用世界观内容时统一使用 worldview.md §X L<line> 格式;不得自动修改 docs/worldview.md 或主动回写 docs/library/

Files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plan-refactor-server-session-v1.md
docs/plans-skeleton/**/*.md

📄 CodeRabbit inference engine (CLAUDE.md)

新 plan 必须先读取 docs/CLAUDE.md;骨架、Active 和 Finished plan 必须遵循三态流转及规定的阶段状态、Finish Evidence 结构。

Files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
**/*

📄 CodeRabbit inference engine (CLAUDE.md)

**/*: commit message 必须使用中文,每个逻辑单元一个 atomic commit;agent 生成的 commit 必须带真实模型 Model: <精确模型 id> trailer。
禁止使用 --no-verify--no-gpg-sign、关闭签名配置、未经确认的 force push、hard reset、amend 或交互式 rebase。
禁止留下 auto-stash 产生的孤儿 WIP stash;自动 stash 流程完成后必须恢复自己的 stash。

Files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plan-refactor-inventory-core-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
  • docs/plan-refactor-server-session-v1.md
🧠 Learnings (3)
📚 Learning: 2026-07-17T00:31:10.779Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:1-1
Timestamp: 2026-07-17T00:31:10.779Z
Learning: 在审查该仓库 `docs/plans-skeleton/` 下的“docs-only skeleton plan”创建类 PR 时:先核对 `docs/CLAUDE.md` 中“Plan 消费规范”,并逐份查看本计划文档里的“§10 实施工作流”,确认后续实施阶段是否会遵循“每个 PR 只修改一个 plan”,且实施/归档时不会出现跨 plan 的修改。该规则不适用于用户显式指定、共享调研基线且计划之间存在互相交叉引用的 skeleton plan 同批创建 PR;对这类情况应按实际交叉引用关系放宽,确保仍能按独立或约定的序列化方式推进。

Applied to files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
📚 Learning: 2026-07-17T00:31:13.643Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1218
File: docs/plans-skeleton/plan-skill-av-relink-v1.md:81-85
Timestamp: 2026-07-17T00:31:13.643Z
Learning: 审核 docs/plans-skeleton/*.md 下的 skeleton 草案 PR 时:不得要求作者在“§N 开放问题(P0 决策门前需收口)”核查完成之前提前填写对应的“§N.1 决议”。仅当计划进入 active 且 P0 实施前,已由 Explore agent 并行核查代码现状后,才允许追加“§N.1 决议”,且该决议需包含结论、实施方案、边界条件,并使用“文件:行号 + plan 章节”的双锚点格式。

Applied to files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
📚 Learning: 2026-07-22T02:11:59.191Z
Learnt from: Kizunad
Repo: Kizunad/Bong PR: 1247
File: docs/plans-skeleton/plan-bughunt-animal-air-spawn-gravity-v1.md:0-0
Timestamp: 2026-07-22T02:11:59.191Z
Learning: 在 `docs/plans-skeleton/` 目录下的计划状态行中,若起草日期同时涉及 UTC 与本地日期(例如时区换算后可能跨到不同日期),请在该行中同时标注 UTC 起草日与本地起草日。这样可以避免基于 UTC 的时间基准在 GitHub/CodeRabbit 审查时将本地日期误判为“未来日期”。

Applied to files:

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md
  • docs/plans-skeleton/plan-refactor-persistence-slices-v1.md
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
  • docs/plans-skeleton/plan-refactor-master-v1.md
🪛 LanguageTool
docs/plans-skeleton/plan-agent-craft-schema-v1.md

[grammar] ~38-~38: Ensure spelling is correct
Context: ...gest、最终 C2S/S2C count 与 exports。R6 只接受该 SHA并做 TypeBox↔proto/Rust/client roundtrip;不一致...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

docs/plan-refactor-inventory-core-v1.md

[style] ~130-~130: Consider using the more formal “until”.
Context: ...ot-only baseline 不通过。 10. forge 深链保留;另锁 /give hoe_iron → 新 snapshot → 真实非零 instance → held/equip → lingtian_start_till,禁止 instance_id=0 或任意 server-data 冒充成...

(TILL)


[grammar] ~133-~133: Ensure spelling is correct
Context: ...ation failure 保留旧行可重试。 ## 8. Named bot acceptance(P4) 以下名称即 scripts/bot/scenarios/<name>.py ...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (10)
docs/plan-refactor-server-session-v1.md (1)

40-76: LGTM!

Also applies to: 81-118, 134-144, 152-164, 169-190, 252-262

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md (3)

1-3: 📐 Maintainability & Code Quality

核对 skeleton plan 的三态元数据。

此文件位于 docs/plans-skeleton/。可见头部只有阶段列表,没有显示 skeleton、active、finished 状态和 Finish Evidence。请先读取 docs/CLAUDE.md,确认这些字段是否位于未展示段落;若缺失,请补齐元数据,但不要在 P0 验真前提前填写 §N.1 决议

As per coding guidelines,docs/plans-skeleton/**/*.md 必须遵循三态流转和 Finish Evidence 结构。Based on learnings,应先核对 docs/CLAUDE.md 的 Plan 消费规范。

Sources: Coding guidelines, Learnings


19-19: 🗄️ Data Integrity & Integration

核对真元释放出口。

Line 19 指定 release_dormant_qi_to_zone。编码规则要求释放使用 qi_release_to_zone,并通过 qi_physics::ledger::QiTransfer。如果前者是严格委托给 canonical API 的 wrapper,请在计划中明确该关系并增加守恒测试;否则请改用 canonical API。

As per coding guidelines,所有真元流动必须通过 QiTransfer,释放必须使用 qi_release_to_zone

Source: Coding guidelines


67-67: 🎯 Functional Correctness

无需修正 Cargo 验收命令的过滤语义。

--package bong-server 只选择包;没有 test target filter 时,Cargo 会运行该包下的测试 target,persistence 在此处是传给测试用例的 name filter,而不是选择模块的 Cargo flag。

			> Likely an incorrect or invalid review comment.
docs/plan-refactor-inventory-core-v1.md (1)

117-158: LGTM!

docs/plans-skeleton/plan-refactor-master-v1.md (2)

61-63: LGTM!

Also applies to: 66-66


68-74: LGTM!

Also applies to: 76-79, 87-101

docs/plans-skeleton/plan-agent-craft-schema-v1.md (1)

1-12: LGTM!

Also applies to: 18-38

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md (1)

21-25: LGTM!

Also applies to: 38-40, 46-47

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md (1)

3-7: LGTM!

Also applies to: 18-24, 36-36

Comment thread docs/plan-refactor-inventory-core-v1.md Outdated
Comment on lines +14 to +17
## 接入面与冻结合同

- **进料**:总纲 §4 canonical shape、R1 phase/identity/generation、`agent/packages/schema/src/{craft.ts,client-request.ts,server-data.ts,schema-registry.ts}`。
- **出料/owner**:本 plan 唯一修改 `agent/packages/schema/**`,原子交付 `CraftOpen`、`CraftPause`、`CraftResume`、`CraftSessionStateV2` 的 source/generated/dist;R6 只消费记录 SHA,不回改 Agent artifacts。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

统一补齐三个 plan 的接入面锚点。

三个 plan 都缺少 worldview.md §X L<line> 锚点。该缺口会阻断后续实现对共享世界观规则和跨轨合同的追溯。

  • docs/plans-skeleton/plan-agent-craft-schema-v1.md#L14-L17: 增加 server/agent/client 跨仓库契约、复用的共享 schema/type/event,以及 worldview.md §X L<line> 锚点。
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md#L12-L17: 增加对应的 worldview.md §X L<line> 锚点。
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md#L12-L17: 增加对应的 worldview.md §X L<line> 锚点。

As per coding guidelines,新增 plan 的接入面必须包含这些字段。

📍 Affects 3 files
  • docs/plans-skeleton/plan-agent-craft-schema-v1.md#L14-L17 (this comment)
  • docs/plans-skeleton/plan-refactor-wire-s2c-v1.md#L12-L17
  • docs/plans-skeleton/plan-refactor-c2s-gate-v1.md#L12-L17
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-agent-craft-schema-v1.md` around lines 14 - 17, 补齐三个
plan 的接入面锚点:在 docs/plans-skeleton/plan-agent-craft-schema-v1.md
第14-17行的“接入面与冻结合同”中加入 server/agent/client 跨仓库契约、复用的共享 schema/type/event,以及对应的
worldview.md §X L<line> 锚点;在 docs/plans-skeleton/plan-refactor-wire-s2c-v1.md
第12-17行和 docs/plans-skeleton/plan-refactor-c2s-gate-v1.md 第12-17行分别加入对应的
worldview.md §X L<line> 锚点,并确保新增 plan 的接入面字段完整。

Source: Coding guidelines

Comment on lines 53 to 58
- **Wave 0(立即并行)**:V(bot 骨干 + build token 最先)、R3、R5、R2、registry-datafication;同时全部轨道的 P0(设计收口 + 吸收清单验真)都可开工。§6.11 登记的 Agent `A-CS`(`plan-agent-craft-schema-v1`)也在本波启动并独立提交,必须先于 R6 P1 合入。
- **Wave 1**:R6(R2 + Agent `A-CS` 合入后)、R7 基础设施(R2 合入后)、R1 framework-only(仅 `InteractionSession`/registry/lifecycle 骨架,R3 P1 合入后;不得宣称 craft pause/resume 或 delivery 生产闭环)。
- **Wave 2**:R4(#1287 + R6 P1 后)、R9(R5/R6/R2 P1 后)、R10(R3 P1 后);R1 宿主迁移按显式 gate 分批放行:craft 需 Agent `A-CS` + R3 P1 reservation/outbox + R3 P4 placeable-entity stable `placed_id` 持久化/hydration/runtime 映射 + R6 craft intents + R4 craft handler/gate + R7 P2 Craft Screen + R2 P1 已登记的 `CraftStore` + R10 P1 `deliver`/receipt contract + R10 P2c production outbox worker,alchemy/forge 同样需 R3 P1 与 R10 P2c,`TsyPresence` 需 R3 P1 auxiliary Slice 与 R3 P4 restore parity
- 近完成独立 plan(§6.9)在 Wave 0 窗口内优先收尾清场。
- R5 P1(字段收私有的全仓编译大爆破)挑在飞 PR 队列清空的窗口单独合入。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major

补充严格命名的 §10 实施工作流

本计划包含多轨道和多 PR 依赖,但当前 §10 是 flash-review issue 消化流程,不是按依赖顺序编排实施 PR 的工作流。请新增严格命名的 §10 实施工作流,将 flash-review 流程顺延,并在该节末尾写明单次 /consume-plan 可自动执行至 merge 并归档到 docs/finished_plans/

As per coding guidelines,scope 大于或等于 4 个 PR 的 plan 必须包含 §10 实施工作流,并在同一 plan 内序列化多个 PR。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` around lines 53 - 58,
在计划文档中新增严格命名的“§10 实施工作流”章节,按依赖顺序编排并序列化所有实施 PR,覆盖 scope 不少于 4 个 PR 的执行流程;将现有
flash-review issue 消化流程顺延至后续章节,并在“§10 实施工作流”末尾明确单次 `/consume-plan` 可自动执行至合并并归档到
`docs/finished_plans/`。

Source: Coding guidelines

- `persistence/**`+autosave=R3;`session/`+7 域 session.rs=R1;`client_request_handler.rs`+`gate/`=R4;`*_emit.rs` 公共层+`proto_convert.rs`=R6;`qi_physics/**`+qi 字段直写行=R5;`inventory/**`=R10;cast/AV emit+skill 注册=R9。
- client:Store 生命周期+`clearClientStateOnDisconnect` 区段=R2;channel 注册区段+桥+router=R6(与 R2 同文件不同区段,merge 前互 fetch);Screen/hud/keybind/InspectScreen=R7;combat cast store=R9。
- `agent/packages/schema/src/**`、generated JSON Schema 与提交的 `@bong/schema` dist=§6.11 Agent 轨;craft schema prerequisite 的 production owner 是 Agent 轨的独立 craft-schema 交付批次,必须在 R6 P1 前提交并验收 TypeBox source、generated schema/dist 与变体计数。理由:TypeBox 是 agent-side source of truth,生成物必须与 source 同 owner 原子提交;让 R6 同时改 source 和 wire 会违反本总纲的 agent 排除边界并形成双 owner。R6 独占 proto/Rust mirror/converter、client wire encode/send 与 bridge/router plumbing,只消费该冻结版本并负责一次性 wire 反映。R6、R4、R7、R1 的 craft gate 均以本处 ownership 决议为准。
- CraftOpen target bridge(跨轨 canonical contract):`CraftOpen` 必须携带 required `target` 判别联合:`Handcraft` 或 `Workbench { workbench_key }`,不得省略。`workbench_key` 是现有成功 S2C `WorkbenchOpen.entity_id` 的 ECS `Entity::to_bits()` locator:逻辑/Rust 类型 `u64`、protobuf `uint64`、JSON/TypeBox 为无符号十进制字符串;它只在当前进程的首次请求中定位 runtime entity,不是授权能力,也绝不写进 checkpoint。普通手搓发送 `Handcraft`;`WorkbenchScreen` 必须从 response 保留该 key 并在初次 `CraftOpen` 原样回传。R4 将 key 解析为 entity 后重验实体存活且携带 `WorkbenchBlock`、玩家同维且在既有距离内,并执行 owner/busy/facility gate。missing、malformed、stale、despawned、跨维或越距 key 均拒绝;R7 不得从 UI 猜测或改写 key。`CraftPause`/`CraftResume` 只携带 hydrated session identity/version,不重复 target。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major

IDOR (CWE-639): Authorization Bypass Through User-Controlled Key (IDOR)

为暂停和恢复请求增加独立的调用者授权。

当前合同只描述 session identity/version,没有绑定认证连接中的 caller identity。若服务端接受请求中的 session identity,已认证客户端只要获得其他会话的 identity/version,就可能暂停或恢复其他玩家的会话。请在状态转移前校验 caller 是 session owner 或具备明确授权关系。不要依赖 CraftOpen 的 owner gate;请为 CraftPauseCraftResume 定义独立的 R4/R10 gate。当前未提供 handler 实现,请验证该校验确实存在。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` at line 64, 为 CraftPause 和
CraftResume 定义并实现独立的 R4/R10 授权 gate,在执行状态转移前校验认证连接的 caller identity 属于目标 session
的 owner 或具备明确授权关系;不要依赖 CraftOpen 的 owner gate,也不要仅信任请求中的 session
identity/version。更新协议与处理流程以传递并验证所需 caller/session 归属信息,并确认两个 handler
在授权失败时拒绝请求且不会修改会话状态。

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== locate files =="
git ls-files | rg '(^|/)plan-refactor-master-v1\.md$|(^|/)plan-agent-craft-schema-v1\.md$|(^|/)worldview\.md$' || true

echo "== outline context =="
for f in docs/plans-skeleton/plan-refactor-master-v1.md docs/plan-agent-craft-schema-v1.md; do
  if [ -f "$f" ]; then
    echo "--- $f line count"
    wc -l "$f"
    echo "--- $f relevant lines"
    nl -ba "$f" | sed -n '1,120p'
  fi
done

echo "== searches =="
rg -n "CraftPause|CraftResume|session_key|generation|version|target|hydrated" docs/plans-skeleton/plan-refactor-master-v1.md docs/plan-agent-craft-schema-v1.md docs || true

Repository: Kizunad/Bong

Length of output: 526


🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== files =="
printf '%s\n' docs/plans-skeleton/plan-agent-craft-schema-v1.md docs/plans-skeleton/plan-refactor-master-v1.md docs/worldview.md

echo "== plan-refactor-master-v1 relevant lines =="
awk 'NR>=50 && NR<=75 {printf "%d:\t%s\n", NR, $0}' docs/plans-skeleton/plan-refactor-master-v1.md

echo "== plan-agent-craft-schema-v1 relevant lines =="
awk 'NR>=1 && NR<=40 {printf "%d:\t%s\n", NR, $0}' docs/plans-skeleton/plan-agent-craft-schema-v1.md

echo "== target schema mentions =="
rg -n "CraftPause|CraftResume|session_key|generation|version|target|hydrated|CraftOpen" docs/plans-skeleton/plan-agent-craft-schema-v1.md docs/plans-skeleton/plan-refactor-master-v1.md

Repository: Kizunad/Bong

Length of output: 9512


使用 CraftPause/CraftResume 的精确字段名。

docs/plans-skeleton/plan-agent-craft-schema-v1.md §4 将 pause/resume 合同定义为 required session_key + generation,且不得携带 target;总纲 line 64 的 hydrated session identity/version 会引入未定义的 version 别名。将 version 对齐为 generation,或同步两处字段名,避免 R6、R4、R7、R1 实现不一致的 payload。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` at line 64, Update the
CraftPause/CraftResume contract wording in the plan so “hydrated session
identity/version” uses the exact required fields “session_key + generation”
defined by plan-agent-craft-schema-v1.md §4; ensure it continues to state that
target is not included and aligns the R6, R4, R7, and R1 payload terminology.

Comment on lines +64 to +65
- CraftOpen target bridge(跨轨 canonical contract):`CraftOpen` 必须携带 required `target` 判别联合:`Handcraft` 或 `Workbench { workbench_key }`,不得省略。`workbench_key` 是现有成功 S2C `WorkbenchOpen.entity_id` 的 ECS `Entity::to_bits()` locator:逻辑/Rust 类型 `u64`、protobuf `uint64`、JSON/TypeBox 为无符号十进制字符串;它只在当前进程的首次请求中定位 runtime entity,不是授权能力,也绝不写进 checkpoint。普通手搓发送 `Handcraft`;`WorkbenchScreen` 必须从 response 保留该 key 并在初次 `CraftOpen` 原样回传。R4 将 key 解析为 entity 后重验实体存活且携带 `WorkbenchBlock`、玩家同维且在既有距离内,并执行 owner/busy/facility gate。missing、malformed、stale、despawned、跨维或越距 key 均拒绝;R7 不得从 UI 猜测或改写 key。`CraftPause`/`CraftResume` 只携带 hydrated session identity/version,不重复 target。
- **Stable workbench addendum**:R4 还须从 R3 P4 registry 把 runtime entity 映射为唯一 `placed_id` 后才交 R1 持久化 claim;restore 以 `placed_id` 绑定本次 hydrate 的新 Entity。missing/duplicate/unhydrated mapping 按 `InvalidRestore` fail closed。

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major

把 Craft admission 校验与 BusyClaim 合并为原子操作。

Line 64 描述 R4 的实体、owner、busy、facility 检查。Lines 80-86 又把该 admission 映射到 R1 的 try_acquireBusyClaim。当前合同没有要求检查和占用由同一事务、锁或 compare-and-claim 完成;race tests 也不能替代原子性约束。请明确单一 owner 执行原子检查并占用,并在结果过期或占用冲突时拒绝请求。

Also applies to: 80-86

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` around lines 64 - 65, 明确要求
Craft admission 校验与 BusyClaim 获取由单一 owner 通过同一事务、锁或 compare-and-claim 原子完成,覆盖 R4
的实体存活、WorkbenchBlock、同维、距离、owner、busy、facility 检查及 R1 的 try_acquire/BusyClaim
映射;任一检查结果过期或发生占用冲突时必须拒绝请求,不能依赖 race tests 替代原子性约束。

| Artifact | Producer | Production consumer | Gate / evidence |
|---|---|---|---|
| craft TypeBox source + generated schema + committed dist | Agent `A-CS` P1-P2 | R6 P1 mirror/converter;agent runtime | A-CS SHA;正反 sample、freshness、variant count |
| craft proto/Rust mirror/converter/samples | R6 P1 | R4 P1 decode;R1 P1 emit | `A-CS`;buf/sample、116 C2S/144 S2C |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major

将 C2S 变体数量绑定到 registry-derived 基线和目标。

Lines 75、121、140 使用 116,但没有区分当前基线和 A-CS/R6 合入后的目标。上游合同在 docs/plan-agent-craft-schema-v1.md Lines 14-20 明确当前 registry 基线为 113,新增三个 craft intent 后目标为 116。请记录当前值、目标值、三个新增变体及 generated registry/SHA/sample 证据;144 S2C 也应使用同类生成证据。否则未完成 A-CS/R6 合入时可能被错误报告为 116。

Also applies to: 121-121, 140-140

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans-skeleton/plan-refactor-master-v1.md` at line 75, 更新计划中所有使用“116”的
C2S 变体数量记录,明确区分当前 registry 基线 113 与合入三个新增 craft intent 后的目标 116,并列出这三个变体及
generated registry、SHA 和 sample 证据。同步为“144 S2C”补充同类生成证据,覆盖 `craft proto/Rust
mirror/converter/samples` 条目及其他对应位置,避免在 A-CS/R6 尚未合入时将目标值误报为当前值。

Comment thread docs/plans-skeleton/plan-refactor-wire-s2c-v1.md Outdated
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: dfe84d974befbd9319096d9de21f37b6f24c0f42
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] A-CS cannot reach its claimed 116-variant schema baseline within its declared scope

docs/plans-skeleton/plan-agent-craft-schema-v1.md:19 · schema-contracts

The new plan states that adding the three craft lifecycle requests takes the TypeBox ClientRequestV1 baseline from 113 to 116, and P1/P3 require union membership plus a final count of 116. However, the current source union at agent/packages/schema/src/client-request.ts:1135-1223 does not even include already-live craft_start, craft_cancel, or workbench_open (and docs/plans-skeleton/plan-bughunt-client-request-schema-drift.md:7-42 documents additional production requests missing from this union). A-CS simultaneously limits itself to the four new craft contracts (plan-agent-craft-schema-v1.md:16,25,41), while the master schedules A-CS before R6 and R6 P4 owns the later 116-variant sweep. Therefore implementing only A-CS's owned contracts cannot produce the promised 116-member TypeBox union or pass its own P3 count gate; reaching that count requires unrelated drift fixes for which this plan declares neither ownership nor a prerequisite.

Root cause: the plan uses the rust/production request count (113) as though it were the current typebox union baseline, despite the repository's known schema drift, and omits an ordering/ownership dependency that would first bring the typebox union to parity.

[major] Terminal delivery failure has contradictory session ownership semantics

docs/plan-refactor-server-session-v1.md:265 · strict-maintainability

A terminal delivery reaches this conflict after R3 has committed the outbox and removed the live gameplay session, then the P2c worker encounters a retryable inventory/spill failure (for example, persistence unavailable). The new R1 contract says that after handoff the live session/claim is removed and delivery failure must not reattach it (docs/plan-refactor-server-session-v1.md:265; also §2.1 invariant 7), while R10 still states categorically that teardown occurs only after stored/spilled success and that failure retains the session (docs/plan-refactor-inventory-core-v1.md:112). Neither statement scopes itself to a different delivery path, and the PR explicitly makes P2c the sole terminal-delivery path. Implementing R1 leaves no session to retain; implementing R10 reopens or retains gameplay state in violation of R1, so the two owning tracks cannot satisfy both acceptance contracts.

Root cause: the pr introduced the durable asynchronous outbox lifecycle without removing or narrowing r10's earlier synchronous delivery/teardown rule, leaving duplicated canonical failure semantics that now prescribe opposite session states for the same retryable worker failure.

[major] Worker commit can acknowledge one outbox payload while delivering another

docs/plan-refactor-inventory-core-v1.md:48 · wiring

The fixed worker API accepts both claimed, which contains the authoritative outbox payload, and a separately supplied DeliveryRequest. The commit contract only says to call InventoryTxn::deliver with the stable delivery_id; it never requires the request to be decoded from, or cryptographically/equivalently match, claimed.payload/payload_digest. Thus a stale or mismatched request can deliver item set B while the same transaction records the receipt, commits, deletes the obligation, and releases quota for outbox payload A. Neither the malformed-payload pin at line 56 nor the crash/replay pins assert this producer-to-consumer payload binding, so those tests would accept such an implementation.

Root cause: the proposed outbox-consumer boundary duplicates the delivery payload across an authoritative claimed record and an unconstrained caller-provided request without defining validation or derivation between them.

[major] Successful delivery receipts have no bounded retention

docs/plan-refactor-server-session-v1.md:77 · performance

The new lifecycle requires a durable DeliveryCommitReceipt for cross-restart replay (line 77), and the terminal matrix keeps CommittedReleased available for receipt replay after quota has returned to zero (line 147). A normal, reachable path such as repeatedly completing craft sessions therefore creates one more durable receipt/terminal record per delivery; because released records no longer consume SessionDeliveryQuota, admission continues indefinitely, while this plan defines no receipt retention, compaction, deletion, or separate storage bound. The related R3 plan explicitly defers a bound only for ResolvedDisposition payloads, so it does not bound successful receipts. On a long-running server, the receipt table and its indexes grow linearly with all historical deliveries, increasing storage and query/backup/startup costs without limit.

Root cause: the exactly-once design makes successful delivery receipts permanently durable but specifies lifecycle and quota release only for active obligations; it omits any bounded retention or compaction contract for terminal deliverycommitreceipt records.

[major] Terminal handoff and teardown use conflicting linearization points

docs/plan-refactor-inventory-core-v1.md:55 · concurrency-atomicity

The new P2c contract makes terminal delivery asynchronous: line 55 says the worker claims an already-terminal outbox row and atomically commits inventory/spill, receipt, obligation deletion, and quota release. The new R1 plan likewise states that outbox/checkpoint commit is followed immediately by claim release and live-session removal, and that delivery failure must not reattach either (docs/plan-refactor-server-session-v1.md:123-126, 265). However, the unchanged R10 ownership rule at line 112 still says R1 tears down only after stored/spilled succeeds and preserves the session on delivery failure. A reachable full-inventory/persistence failure or a delivery that enters retry/DeadLetter therefore has two incompatible prescribed transitions: following line 112 retains the gameplay session/claim while an outbox obligation is pending, blocking the facility and allowing a state the new R1 contract explicitly forbids; following the new handoff rule contradicts R10's stated teardown gate. The new worker and host-migration gates do not resolve this because both statements remain normative in the same changed plan family.

Root cause: the plans define two different linearization points for teardown: the new durable outbox handoff commit versus the later asynchronous inventory delivery success. the legacy synchronous-delivery rule was not narrowed or replaced when p2c was added, leaving claim/session state non-atomic with the authoritative terminal handoff.

[major] R6 P1 requires a craft producer before its R1 state source exists

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:21 · schema-contracts

The changed P1 deliverable requires R6 to ship the craft_emit production producer in this phase. However, the master contract says that producer reads R1 state (docs/plans-skeleton/plan-refactor-master-v1.md:76), while Wave 1 permits only an R1 framework skeleton and explicitly forbids claiming craft pause/resume production closure (docs/plans-skeleton/plan-refactor-master-v1.md:53). The actual R1 craft adapter is gated on R6 P1 plus R4, R7, R3, and R10 deliverables and is enabled only after all of them merge (docs/plan-refactor-server-session-v1.md:247, docs/plan-refactor-server-session-v1.md:257). Thus the planned R6 P1 producer has no production state source when it is required to be delivered; tests can only inject/mock that missing connection, which the plan family explicitly disallows (docs/plans-skeleton/plan-refactor-master-v1.md:69, docs/plan-refactor-server-session-v1.md:250).

Root cause: the phase assigns production ownership and completion of the v2 s2c emitter to r6 before the r1 adapter that supplies its authoritative state is reachable, while simultaneously making r6 p1 a prerequisite of that adapter. the plan needs to separate r6's schema/converter/emit api and contract pins from activation of the real craft_emit producer, then place production wiring in a phase gated on the r1 state source (or otherwise remove the circular gate).

[major] R6 P1 requires downstream R4/R7 consumers before they are allowed to exist

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:21 · correctness

The changed P1 deliverable requires both a real response→screen→request workbench-key path and R4 rejection pins for stale/despawned/cross-dimension/out-of-range keys. Those consumers cannot exist at this gate: the same document says the production Resume producer remains in R7 P2 (lines 16 and 52) and that R4 handler/gate lands only after R6 (line 39). The master plan likewise orders R4 after R6 P1 (docs/plans-skeleton/plan-refactor-master-v1.md:54) and assigns the client producer to R7 P2 (:75, :77, :88). Following the stated dependency order therefore leaves R6 P1 unable to satisfy its own acceptance criteria; implementing the criteria inside R6 would instead violate the documented ownership boundary and prematurely duplicate downstream production wiring.

Root cause: the p1 phase mixes r6-owned wire/api artifacts with acceptance evidence owned by downstream r4 and r7 phases, despite those phases being explicitly gated on completion of r6 p1.

[major] A-CS omits two contracts required by the craft lifecycle chain

docs/plans-skeleton/plan-agent-craft-schema-v1.md:17 · wiring

The new A-CS plan makes itself the only owner of agent schema changes but limits its output to CraftOpen, CraftPause, CraftResume, and CraftSessionStateV2. The production chain defined by the same diff also requires the existing S2C WorkbenchOpen.entity_id as the producer of workbench_key and retains CraftCancel as the only explicit-cancel intent (docs/plans-skeleton/plan-refactor-client-ui-base-v1.md:19,25). Neither contract currently exists anywhere under agent/packages/schema (the current ClientRequestV1 union in agent/packages/schema/src/client-request.ts:1135-1223 has no craft request, and ServerDataV1 in agent/packages/schema/src/server-data.ts:2024-2135 has no WorkbenchOpen), while the real server wire already has both (proto/bong/envelope.proto:226-230,257 and server/src/schema/client_request.rs:671-673). Because line 42 forbids A-CS from changing proto/Rust/Java and the master plan forbids R6 from changing agent artifacts, no later phase owns adding these required producer/consumer contracts to the TypeBox source, generated schemas, or dist. Thus the planned response-to-screen-to-CraftOpen path and explicit cancellation path cannot satisfy the stated TypeBox source-of-truth/union/freshness gates even after every listed phase is completed.

Root cause: the schema production batch scopes ownership to only four newly named symbols, but the end-to-end lifecycle it is meant to enable also depends on two existing wire contracts that are absent from the authoritative typebox schema. the cross-track ownership rules then leave those omissions with no production owner.

[major] R6 P1 requires R4 rejection pins before R4 can start

docs/plans-skeleton/plan-refactor-wire-s2c-v1.md:22 · wiring

Changed P1 line 22 makes “R4 pins” for stale/despawned/cross-dimension/out-of-range workbench keys part of the R6 P1 deliverable. However, line 40 says the R4 handler/gate lands only afterward, line 39 forbids R6 from touching client_request_handler.rs, and the master plan orders R4 after R6 P1 (docs/plans-skeleton/plan-refactor-master-v1.md:55). Those rejection paths require R4's production runtime lookup and GateSpec consumer, so they cannot be exercised during R6 P1. The phase therefore either deadlocks waiting for its declared downstream consumer or can be marked complete while its own stated acceptance pins are absent.

Root cause: the plan assigns downstream r4 production-gate tests to the upstream r6 p1 phase even though ownership and wave ordering explicitly prevent r4 from existing until r6 p1 has completed. the r6 phase should limit its evidence to wire/conversion boundaries and make the runtime-key rejection pins an r4 deliverable/gate after r6.

[major] Maintenance tests omit the authenticated capability binding boundary

docs/plan-refactor-server-session-v1.md:260 · testing

The new permission contract at line 78 allows maintenance only for server-console origin or an authenticated principal/capability bound to the current executor entity. The required P1 tests at line 260 cover a generic authorized administrator and several unauthenticated/identity negatives, but never pin the two allowed origins separately or the critical case where an otherwise valid authenticated capability is bound to a different executor. An implementation that accepts every valid capability without checking its executor binding (or that supports console only and rejects the authenticated-capability branch) would satisfy the listed tests while violating the changed authorization contract. No surrounding plan assigns this missing permission-branch test to another track; the master plan likewise only says authenticated allow generically.

Root cause: the acceptance matrix does not enumerate all branches of the newly defined maintenance authorization boundary, especially capability-to-current-executor binding and separate positive coverage for console versus authenticated capability origins.

[major] Terminal delivery is specified as both outbox-only and an optional direct transaction

docs/plan-refactor-server-session-v1.md:190 · correctness

For any terminal refund or output delivery (for example a volatile-session disconnect or a checkpointed completion), this line permits either a "crash-atomic transaction" or the durable outbox/receipt path. That directly conflicts with the same plan's invariant at line 88, which requires every checkpointed completion to follow reservation -> outbox -> R10 P2c and forbids a second direct-delivery protocol, and with docs/plan-refactor-inventory-core-v1.md:44, which makes SessionDeliveryWorker the sole production consumer and forbids domain-level direct deliver calls. The new worker acceptance requirements at inventory-plan lines 57 and 137 also require the real outbox path. An implementation choosing the expressly allowed direct-transaction branch would therefore bypass the mandated Pending/InFlight retry, quota, and worker lifecycle while still appearing compliant with this plan; conversely, enforcing the sole-worker rule makes this sentence impossible to implement as written.

Root cause: the new plan retains an alternative synchronous/direct terminal-delivery contract while simultaneously declaring the durable outbox worker to be the only production delivery protocol. the authoritative path was not made singular consistently across the document.

[major] Failed admission cancellation can leak terminal-delivery quota until restart

docs/plans-skeleton/plan-refactor-persistence-slices-v1.md:25 · concurrency-atomicity

The new protocol requires admission to durably create a reservation before the runtime busy claim, and line 25 assigns cancel_unconsumed_reservation to busy losers and post-claim validation failures. The authoritative lifecycle matrix in docs/plan-refactor-server-session-v1.md:141-142 makes this a multi-step sequence: a busy loser must cancel its committed reservation, while a post-claim validation failure first releases the runtime claim and then performs the reservation CAS cancellation. If that cancellation returns a transient SQLite/persistence error after the reservation commit (or after the runtime claim has been released), the durable reservation and used_rows/used_bytes remain charged although no session, checkpoint, outbox, or claim owns them. The only reconciliation described by the matrix is startup handling of a reservation with no checkpoint/live claim; there is no live orphan-reservation scanner, retry obligation, or rule to retain/reacquire the claim until cancellation commits. Repeating this reachable failure can exhaust terminal-delivery quota and reject every later checkpointed-session admission until a restart, despite there being no delivery obligations. The added acceptance at line 51 covers successful cancellation, duplicate cancellation, and CAS losers, but not cancellation persistence failure or live reconciliation, so it would accept this partial state.

Root cause: the plan splits admission rollback across an in-memory busy claim and a durable quota/reservation transaction without defining an atomic handoff or mandatory live retry/reconciliation path when cancel_unconsumed_reservation fails. consequently, a failed compensating write leaks durable quota after the corresponding runtime ownership has already disappeared.

Model: claude-sonnet-5
Co-Authored-By: Claude <noreply@anthropic.com>
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Central review

Decision: request_changes
Reviewed head: b34d992ed04b78980cdec348995306f2ed90ca1e
Policy: project-review-policy.v2
Policy SHA-256: ba594fb26bae4f60ebc26a470e1a7b55bfa5bf3ecb1313dd89c22721e772bcbf

Validated findings

[major] Keep the C2S P3 phase as a separate deliverable

docs/plans-skeleton/plan-refactor-c2s-gate-v1.md:22 · strict-maintainability

The P2 bullet ends with 不得以 mock 或旧 schema 接线。- ⬜ P3 巨石拆分批次 B..., so the P3 phase marker is embedded in the P2 paragraph rather than being a separate list item. As rendered, the phase table has no independently trackable P3 deliverable, even though the later acceptance and completion criteria depend on the full registry-derived type set being declared and old checks being removed.

Root cause: a missing newline/list delimiter merged two phase entries during the plan rewrite. this makes the stated implementation phase boundary and its completion unit ambiguous, so a consumer can mark p2 complete without a distinct p3 work item or acceptance boundary.

[major] Remove conflicting ownership of delivery-retention limits

docs/plan-refactor-server-session-v1.md:151 · strict-maintainability

The plan fixes SESSION_DELIVERY_RESULT_REPLAY_TTL_TICKS, SESSION_DELIVERY_TOMBSTONE_TTL_TICKS, and both history limits as contract constants in §3.1 (line 108), and the open-questions section says those retention values are already frozen and R3 may only choose storage scheduling (line 203). However, the obligation invariant says the concrete horizon, tombstone limit, and alert thresholds are to be frozen by the R3 implementation P0. These statements give R3 conflicting authority over the same cross-track retention contract.

Root cause: the rewrite simultaneously introduced fixed canonical retention constants and retained wording that delegates the concrete retention policy to the durable-storage implementation. r3/r1 can therefore implement different replay/compaction boundaries while both claiming to satisfy the plan, breaking the shared o-21..o-24 contract and its acceptance evidence.

[major] Pin every CraftSessionStateV2 phase and identity rule

docs/plans-skeleton/plan-agent-craft-schema-v1.md:40 · testing

The A-06 contract explicitly expands CraftSessionStateV2 over five gameplay phases (Running | Paused | Suspended | HandoffPreparing | Ended) and requires phase-specific identity/generation rules at line 23, but the acceptance section only requires generic positive/negative samples at line 40. A test suite that checks one valid state and one invalid state, while omitting (for example) HandoffPreparing or accepting Ended as resumable, would satisfy the stated acceptance without detecting that concrete wrong implementation. The master testing policy requires every enum variant and every state transition to be pinned, and the later R1/R4 plans rely on these phase boundaries for Resume rejection.

Root cause: the a-cs acceptance criteria do not enumerate the observable phase/state-identity matrix required by the newly claimed statev2 contract, so coverage can pass while a phase is missing or assigned the wrong resumability/identity rules.

Model: cc-sonnet-high
Co-Authored-By: Claude <noreply@anthropic.com>
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.

1 participant