[Bindy-lbb]

Reopens the #403 delivery against main after #410 merged only into the already-squashed feat/402 branch.

Implements the engine-facing batch decision path and the outstanding review work:

• explicitly permits only homogeneous medium-risk tool batches. Coverage pins same-request replay, no batch classification, differing source runs, write, exec, network, expired, version-conflicted, run-scope, and batches still needing more than one vote; a final vote may batch-settle a multi-party member that already has prior grants;
• keeps per-member durable audit/idempotency identities and sends one pendingApprovals recovery envelope;
• reverses every audit decision that made it into the append-only SHA-256 ledger if either a later audit append or a record write fails. Recovery tests inject the second audit append failure, the second record write failure, and a rollback write failure; the last returns typed approval_storage_failed and publishes approvals.changed.

Validation: npm run build; node --test --test-timeout=120000 apps/server/dist/test/approvals-center.test.js (30 passed, 1 intentional engine-integration skip).

npm run test:renderer remains blocked before the approval tests by the branch baseline's unresolved @fullstack-ai-infra/ui import; server and compilation validation above passed.

[Bindy-lbb]

Added the persistence-failure regression: a synthetic second-member write fault now verifies every record rolls back to pending/not_started and no recovery envelope dispatches. Targeted approval-center suite: 26 passed, 1 intentional engine integration skip.

[waterbro-8]

代码方向对，#410 那轮真正的合入阻断已经补上了：base 是当前 main（ac916af，ahead 1 / behind 0）、write/exec/network/过期/version/threshold>1/run-scope 有独立用例、部分 put 失败会回滚 before-image 且不发 pendingApprovals。客户端仍然不能投 pendingApprovals。但现在还不能批。

Blocker：#403 AC 测试被减掉了，不是「已有覆盖 + 补边界」

#410 里已经有、而且上次当成现有覆盖的两项，这次重做没带过来：

1. 同源 tool 批量成功之后的 replay：同一 requestId 必须 200，且不能再发一次 pendingApprovals。Issue 原文就是 Idempotency/retry cannot duplicate a verdict。
2. 无 batch 策略 / 不同来源 → 全员 rejected、零 envelope。现在服务端 sourceSignature 在 CI 里根本跑不到。请拆成两条，第二条必须是 策略已分类的 tool + 两个 source。

rejectBatch 只断言全员 rejected、没有信封，不能替代上面两条。

非产品验收。CI 11/11 是绿的，CODEOWNERS BLOCKED 是评审门，不是失败。我不会合。

[PeterGuy326]

Review (roleweave #418, reopens #403 against main)

Re-review of head 7a792fc. Base verified: origin/main is an ancestor of this branch, single commit, and the #410/feat/402 lineage is gone — this is a real main delivery.

What I reproduced locally (clean worktree, @roleweave/* linked to this branch's sources): all 9 #403 cases pass; approvals-center.test.js 27 tests / 26 pass / 1 skip (the skip is the APPROVAL_ENGINE_MODULE integration gate). CI green on Node 24 macOS + Ubuntu. Your body says 25 passed — that's an under-count, not an over-claim.

Claims that hold, checked against code rather than the description:

• Same-source is enforced on both ends: sourceSignature() over the (kind, positionId, conversationId, turnId, runId, engine) tuple server-side, and cross-run checkboxes greyed out in ApprovalQueue.
• tool-only is structural, not merely a default: risk === "medium" plus the policy schema pinning batch.actionKinds to length === 1 && [0] === "tool", in both validRule and the store's valid(). write/exec/network cannot be configured in.
• pendingApprovals really is internal-only: POST /turns validates against a strict allowedKeys whitelist that omits the field (apps/server/src/routes/turns.ts:102), so a public client cannot forge a plural verdict. I went looking specifically for this.
• Rollback test is the real thing — it fails the second granted write and asserts the first member is restored to pending / not_started / no decision, plus zero engine calls.
• Concurrency: decideBatch holds the serial() mutex from synchronize through activeKeys.add, so the execution.turnId-set-but-not-yet-active window cannot be observed by a racing synchronize. This is easy to get wrong and you got it right.
• synchronize() re-reads via store.list() on every call, so the mid-loop throw conflict("Approval policy did not settle the batch member") does not leak partial in-memory state.

Requested before merge

1. Rollback does not cover the audit chain — apps/server/src/approvals/service.ts:445-472.

decideBatch appends audit for all members first, then enters the record save loop; the catch restores records only. So when the second member's write fails, on disk you are left with two type: "decision", decision: "granted" audit events (chained into the SHA-256 previousHash ledger) while both records are still pending. audit() (service.ts:257) reads the audit file directly and never reconciles against record state, so GET /approvals/:id/audit exports grant evidence for approvals that were never granted — on a feature whose whole framing is a tamper-evident compliance trail.

The rollback test asserts record state and engine silence, but never touches audit, so nothing pins this.

Recommend one of:

• emit a compensating event per member carrying the same batchId when the record loop fails, so the chain self-describes the rollback; or
• state it in Known limitations and assert current audit contents in the rollback test.

Related: the (approvalId, requestId) dedupe in appendAudit means re-submitting the same members under a new batch requestId re-appends a second granted per member. Deterministic batchMemberRequestId protects retries of the same batch id, which is the case you documented — this is the case after a rollback.

2. Rollback failure is swallowed — service.ts:465-471.

.catch(() => undefined) on the restore loop means a failed rollback is invisible; the caller sees only the original storage error while disk may sit half-granted. Given "全员回滚" is a headline guarantee here, at minimum publish approvals.changed and surface that the rollback itself failed.

3. threshold > 1 is only rejected in the fresh case — description and test overstate the gate.

The check is a.progress.pending !== 1, and policyProgress returns pending = threshold - granted (apps/server/src/approvals/policy.ts:85). So a threshold: 2 record with one prior grant has pending === 1 and is batchable by the second approver. #403 rejects batches that need more than one approval principal uses a fresh threshold: 2 (pending === 2), which is why it passes.

I think "this batch vote must settle each member" is the right invariant — but then the PR body's "threshold>1 ... independently covered" and docs/approvals-p0.md's "升级中的多人门槛不允许加入批量" are inaccurate. Either fix the wording, or tighten to threshold === 1 and add a case that pins whichever semantics you intend.

Non-blocking

• Queue projection is looser than server acceptance. view() derives batch from status/kind/risk/policy.batch only; acceptance additionally requires no run in scope.allowed, maxItems >= n, progress.pending === 1, actor rights, same source, and a matching source turn. So the UI draws a checkbox on items the server will always reject — a tool whose engine offered run scope is the reachable case. Consider extracting one eligibility predicate used by both.
  • Minor, same area: selectBatchItem computes Math.min(item.batchMaxItems, selected.batchMaxItems) against a single representative rather than all selected. Unreachable today because same-source implies same position implies uniform maxItems, but it is a trap if the source constraint ever loosens.
• Unadvertised trust-boundary change bundled in. approvalDecision's IPC whitelist gains scope, including "run", and the desktop test that asserted scope: "run" is rejected was rewritten to assert scope: "workspace". Two consequences worth a sentence in the body: (a) this implies that on main every desktop approve/deny was 400, because decide() unconditionally sends scope and the old whitelist lacked the key — a real bug, fixed as a side effect; (b) run-scope granting is now reachable from the renderer. Server-side validRunScopeBinding still fails closed on a forged binding, so this is not a vulnerability, but please add a case asserting run-scope round-trips and that an invalid runBinding is still refused server-side.
• Risk vocabulary. ApprovalRiskLevel is "medium" | "high" — there is no low. The gate is medium, but the rejection copy ("not eligible for a low-risk batch"), the recovery turn input ("已批准的低风险动作"), the test names, and the docs/approvals-p0.md heading all say low-risk. The doc body is accurate ("服务端还要求请求上下文风险为 medium"), so a reader can conclude a safer low class exists and is batchable, when it does not exist and would be rejected. Worth aligning.
• Persistence failure returns a bare 500 (the test asserts 500), while the client copy apr.submitFailed says "结果尚未确认，请刷新后重试" — but the server rolled back cleanly. A typed code (e.g. approval_batch_persist_failed) would make the retry semantics match what actually happened.
• Cross-repo ordering not stated. The recovery turn emits pendingApprovals, which only digital-employee #337's engine schema accepts. If #418 lands first, every batch settles the records and then fails at beforeRun, leaving indeterminate — please note the dependency and any engine-version floor in the body.

Not product 验收, and no merge judgment from this review. Findings 1-3 are the ones I would like answered.

[waterbro-8]

head ab45c05 相对 7a792fc 补上了胡奕舟 1–3：append-only decision_reverted、restore 失败不再 .catch(() => undefined)、threshold 用 progress.pending !== 1 收最后一票。回滚用例也核了链。这些点过了。还是不能批。

Blocker：#403 AC 测试仍缺，代码有、CI 跑不到

1. 成功批量之后的 replay：同一 requestId 必须 200，且不能再发一次 pendingApprovals。decideBatch 开头有 prior 分支，但 approvals-center.test.ts 没有成功路径的 replay 断言。Issue 原文就是 Idempotency/retry cannot duplicate a verdict。
2. 已分类 tool + 两个 source：全员 rejected、零 envelope。sourceSignature 仍然只有实现、没有用例。请拆成两条，第二条必须是策略已分类的 tool + 两个 source。rejectBatch 的 write/exec/network/过期/version/threshold/run-scope 不能替代这两条。

非产品验收。CI 这轮还在跑，CODEOWNERS BLOCKED 是评审门。我不会合。

[waterbro-8]

head 45c77b3（rebase 到 main 610bea12 / #419）补上了上一轮两条 blocker，CI 全绿。

• 同源 restricted-tool 成功路径：同一 requestId replay 为 200，且 pendingApprovals 仍只有一次。
• 已分类 tool + repo-owner / community-operator 两个 source：全员 rejected、零 envelope。sourceSignature 这条 CI 能跑到。
• 无 batch 策略的 tool 也有独立拒绝用例。胡奕舟 1–3 的 decision_reverted / restore 失败可见 / 最后一票仍在。

代码 APPROVE，不是产品验收。CODEOWNERS BLOCKED 是评审门。我不会合。

[PeterGuy326]

Re-review — changes requested on 45c77b3

The previously reported record-write rollback path is fixed: the second-member put failure restores every before-image, appends per-member decision_reverted events with the same batchId, surfaces typed approval_storage_failed, and a retry leaves one unreverted grant. The successful same-requestId replay, unclassified tool, and classified different-source cases are now independently covered. I also verified the current head and green CI, and locally ran npm run build plus the approval-center suite (28 pass / 1 intentional skip).

Blocker: partial audit-append failure still leaves a false unreverted grant

decideBatch() appends each member decision sequentially at apps/server/src/approvals/service.ts:459-462, but the compensating decision_reverted logic is only inside the later record-save catch at lines 463-500. If audit append 1 succeeds and audit append 2 fails, control exits through the outer catch before any record is written and before any reversal is appended. The records remain pending, but /approvals/:id/audit exposes an unreverted decision: granted for member 1.

I reproduced this against the exact head by injecting a failure on the second appendAudit(... type: "decision"): the API returned 500 and records stayed pending, but the first member had one unreverted grant; the assertion expecting zero failed (1 !== 0).

Please make the pre-record audit phase compensating too: track successfully appended member decisions and append deterministic decision_reverted events for them if any later audit append fails. If that compensation itself fails, return the typed unconfirmed rollback/storage failure and publish refresh events, matching the record-restore failure contract. Add a regression that fails the second audit append and verifies no unreverted decision remains.

Test gap

The code now surfaces record restore failure and publishes approvals.changed, but the focused suite does not inject a restore failure to pin that branch. Please add the regression claimed in the handoff: force a restore put to fail, assert typed approval_storage_failed with the unconfirmed-rollback message, and assert refresh events are emitted.

Not product acceptance. I am not merging.

[PeterGuy326]

Re-review — APPROVE on 559f2fc

复核了上一轮 blocker 及当前完整分支：

• 第二次 decision audit append 失败时，现在只对已成功写入 ledger 的成员追加同 batchId、绑定原 member requestId 的 decision_reverted；记录保持 pending，且不会派发 recovery envelope。
• record restore 自身失败不再被吞掉：返回 typed approval_storage_failed，并发布 approvals.changed 让客户端重新读取磁盘真相。
• 新增两条故障注入回归覆盖上述路径；同 requestId replay、无 batch 策略、已分类但不同 source run 等既有边界仍在。

本地独立 worktree 验证：npm run build 通过；approvals-center.test.js 30 pass / 1 个预期 engine-integration skip。GitHub 失败的 macOS job 我已按原 head 重跑，现 Node 24 macOS/Ubuntu、smoke、installers、layout、CodeQL 全绿。

技术评审通过；不是产品验收，也不代替合并授权。