Repository navigation
Conversation
39f06d5 to
ec32e55
Compare
waterbro-8
left a comment
There was a problem hiding this comment.
delivery.ts 和测试都读了,你两个硬点的落法我认可:不碰词表、验收挂 tool、必须绑定 turnId+runId、decidedBy 只读审计链里已记录的 actor,模块本身无状态无 I/O。delivery: 前缀这次归 roleweave 我也认,平台方要收编以后再说。
但有一个会在接线时直接炸的问题:
deliveryAcceptance 先 audit.filter(e => e.approvalId === record.id),再把过滤后的子集交给 verifyApprovalAuditChain,而它断言 event.seq === index + 1、且首个事件 previousHash === undefined。store.ts:212 的 seq 是全工作区递增的(seq: existing.length + 1),audit(workspace, id) 也是按全局校验完再过滤。所以只要这条审批不是 audit.jsonl 里的第一条,子集里的 seq 就不是 1..n、previousHash 也接不上,结果一律 indeterminate / audit_chain_broken。测试没暴露,是因为 chain() 每条审批都从 1 开始造。
建议二选一:
- 对未过滤的全量数组做链校验,通过后再按 approvalId 取子集;或
- 子集内只校验 seq 严格递增 + previousHash 首尾相接,不要要求首个元素的 previousHash 为 undefined。
另外补一条"第二条审批"的用例把回归钉住,否则这个坑会等到第一个调用方接上去才被发现。
其余(买家身份等 Peter / Bindy、暂时不接调用方)按你说的保持不动。这条改完我再过。
ec32e55 to
f9c5bf5
Compare
waterbro-8
left a comment
There was a problem hiding this comment.
复核 f9c5bf5:改法成立。
- delivery.ts:126-129 先对传入的全量日志 verifyApprovalAuditChain,再按 record.id 过滤;seq / previousHash 的全工作区语义写进函数注释(对应 store.ts:212 的 seq = existing.length + 1),调用方传子集会 fail closed,方向安全。
- 三条新用例覆盖到点上:非首条审批(自己从 seq 3 起、previousHash 跨审批相接仍 accepted)、跨审批不串味(邻居 granted / 自己 denied → declined)、篡改他人事件 fail closed(自家排第一,只有全量校验能看到);原「忽略其他审批事件」已从拼两条各自从 1 开始的链重写为单条工作区日志。共 16 条,CI 885 passed / 0 fail。
- Node 24 ubuntu/macos 的红在 security:check(npm audit --audit-level=high:brace-expansion、electron、@vitest/mocker),与本次改动无关;同一时段 feat/settings-design-storage-notifications 同样红,属基线问题,另开单跟。
批准。
CI note: the two Node 24 legs failed on
|
|
Status update at current head (branch synced with latest main):
|
Closes #539
Summary
Buyer acceptance at delivery time, taken as C2: no vocabulary change. Acceptance rides on the existing
toolaction kind, and the commercial meaning rides inaction.targetbehind adelivery:prefix. Settlement is read off theapproval.grantedaudit chain that already exists — hash 前溯,seqcontinuity, and the boundpolicy.digest.Why the change surface is this small
The six roleweave-side touch points a new kind would have forced are all untouched, because C2 adds no kind:
store.ts:86action.kindallowlistkindstaystoolstore.ts:83schemaVersionv1/v2workbench-approval.v2, no v3store.ts:112/114context.requestedCapability/impacttoolmappingcontext.ts:19-24capabilityContextswitchundefinedpolicy.ts:31rule.actionKindstoolis already admissiblerisk-overlay.ts:8toolalready maps tomedium— no new risk bandapr.kind.*+ CJK gateThe action kind list is declared a verbatim mirror of the engine #187 vocabulary (
shared/turns.ts:62), with final authority inpackages/engine/src/contracts.ts, so extending it would have meant going through the engine first. C2 exists to avoid that.Changes
apps/server/src/approvals/delivery.tsdelivery:target convention,isDeliveryAcceptance,verifyApprovalAuditChain,deliveryAcceptance. Pure functions; no state, no I/O.apps/server/test/approval-delivery.test.tsWhat this deliberately does not do
action.kind, no schema bump.toolis the only entryruleRiskFromKindmaps tomedium, which is exactly the band a delivery acceptance should land on.executeTurnatservice.ts:320-359is Option 1 terminal-and-resume: the contract, not a shortcut. This PR does not add a second settlement path.decidedBystays"operator"(service.ts:330-331— the engine protocol marker; the audit retains the verified actor). Whether a buyer enterspolicy.eligibleApproversor arrives through delegation is not decided here — it needs Peter and Bindy.task/goalcompletion triggering a grant-shaped audit entry (that was option B, and it conflicts with "don't invent new events").How your two hard points land
isDeliveryAcceptancerequiressource.turnIdandsource.runIdas well as thetoolkind and the delivery target, so an acceptance that is not bound to the turn that asked for it is not recognised. This mirrorsstore.ts:85rather than relying on it, so the invariant holds even when the caller holds an unpersisted record.decidedByfrom the audit actor only, so the result is whatever the chain already recorded.Validation
npm run build(typecheck across projects) — pass.apps/server/test/approval-delivery.test.ts— 14/14: prefix convention and its bounds,tool-only recognition (write/exec/network with the same target are refused), turn binding required, risk lands onmedium, accepted / declined / pending (a partial grant under a multi-approver policy does not settle), and fail-closed on a tampered chain, a missingpolicy.digest, a record that disagrees with its audit, and an approval that expired before deciding.reg.exe/sc.exesandbox blocks) as the 648-test baseline measured with this change removed.Open questions
delivery:prefix is defined on the roleweave side in this PR. If you would rather platform own the convention outright, say so and this collapses to a pass-through: the module stops recognising anything and only the docs entry stays.policy.eligibleApproversor delegation, pending Peter and Bindy. Nothing here blocks that decision; it is simply not taken.operatoras the acceptor.