Skip to content

feat(approvals): read buyer delivery acceptance off the existing approval contract (#539) - #540

Open
mobmob719 wants to merge 4 commits into
bytefolk:mainfrom
mobmob719:feat/delivery-acceptance
Open

mobmob719 wants to merge 4 commits into
bytefolk:mainfrom
mobmob719:feat/delivery-acceptance

Conversation

@mobmob719

Copy link
Copy Markdown
Contributor

Closes #539

Summary

Buyer acceptance at delivery time, taken as C2: no vocabulary change. Acceptance rides on the existing tool action kind, and the commercial meaning rides in action.target behind a delivery: prefix. Settlement is read off the approval.granted audit chain that already exists — hash 前溯, seq continuity, and the bound policy.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:

Touch point Why it is not touched
store.ts:86 action.kind allowlist kind stays tool
store.ts:83 schemaVersion v1/v2 stays workbench-approval.v2, no v3
store.ts:112/114 context.requestedCapability / impact both are keyed off the existing tool mapping
context.ts:19-24 capabilityContext switch no new case; nothing can fall through to undefined
policy.ts:31 rule.actionKinds tool is already admissible
risk-overlay.ts:8 tool already maps to medium — no new risk band
前端 apr.kind.* + CJK gate no new kind, so no new label

The action kind list is declared a verbatim mirror of the engine #187 vocabulary (shared/turns.ts:62), with final authority in packages/engine/src/contracts.ts, so extending it would have meant going through the engine first. C2 exists to avoid that.

Changes

File Change
apps/server/src/approvals/delivery.ts New. delivery: target convention, isDeliveryAcceptance, verifyApprovalAuditChain, deliveryAcceptance. Pure functions; no state, no I/O.
apps/server/test/approval-delivery.test.ts New. 14 cases.

What this deliberately does not do

  1. No new action.kind, no schema bump. tool is the only entry ruleRiskFromKind maps to medium, which is exactly the band a delivery acceptance should land on.
  2. No "don't re-run the turn" work item. fix(release): release.yml is missing three Phase 4 in-workflow checks #187 Option 2 (suspend pending an external verdict, then resume) is parked — it breaks "exactly one trusted terminal state" and widens the security surface. The executeTurn at service.ts:320-359 is Option 1 terminal-and-resume: the contract, not a shortcut. This PR does not add a second settlement path.
  3. No buyer identity. decidedBy stays "operator" (service.ts:330-331 — the engine protocol marker; the audit retains the verified actor). Whether a buyer enters policy.eligibleApprovers or arrives through delegation is not decided here — it needs Peter and Bindy.
  4. No new event type. No task/goal completion triggering a grant-shaped audit entry (that was option B, and it conflicts with "don't invent new events").
  5. No UI / i18n change.

How your two hard points land

  • An acceptance cannot be minted without a turn. isDeliveryAcceptance requires source.turnId and source.runId as well as the tool kind and the delivery target, so an acceptance that is not bound to the turn that asked for it is not recognised. This mirrors store.ts:85 rather than relying on it, so the invariant holds even when the caller holds an unpersisted record.
  • Buyer identity is not modelled. The module never reads or writes an identity field. It reports decidedBy from 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 on medium, accepted / declined / pending (a partial grant under a multi-approver policy does not settle), and fail-closed on a tampered chain, a missing policy.digest, a record that disagrees with its audit, and an approval that expired before deciding.
  • Full server suite: 662 tests, with the same 15 pre-existing Windows-only failures (symlink cases plus reg.exe / sc.exe sandbox blocks) as the 648-test baseline measured with this change removed.

Open questions

  • The 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.
  • Buyer identity — policy.eligibleApprovers or delegation, pending Peter and Bindy. Nothing here blocks that decision; it is simply not taken.
  • Wiring. The module has no caller yet, on purpose: the platform side that supplies the target is not implemented, and calling it before the identity question is settled would bake in operator as the acceptor.

@waterbro-8 waterbro-8 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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、暂时不接调用方)按你说的保持不动。这条改完我再过。

@mobmob719
mobmob719 force-pushed the feat/delivery-acceptance branch from ec32e55 to f9c5bf5 Compare September 30, 2026 05:08

@waterbro-8 waterbro-8 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

复核 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 同样红,属基线问题,另开单跟。

批准。

@mobmob719

Copy link
Copy Markdown
Contributor Author

CI note: the two Node 24 legs failed on security:check, not on any test

Everything else on this head is green: test:renderer (71 files), the desktop-main suite (383 tests), the server suite, Unpacked staging smoke (Windows + macOS), Unsigned installers (Windows + macOS), Layout parity, Dependency review and OpenSSF Scorecard. The only failure is the last step of npm run check — npm audit --audit-level=high.

It is advisory drift, not this change. Two high advisories were published after this PR's earlier green run:

  • GHSA-qmv3-fv6v-rmhq (Electron: sandboxed preload code cache can be poisoned by a compromised renderer) — published 2026-09-29T18:02Z
  • GHSA-q2hr-2g5m-vwhr (brace-expansion: quadratic-time expansion causing CPU DoS) — published 2026-09-29T23:45Z

This PR touches no dependency manifest: git show --stat lists zero changes to package.json / package-lock.json. Re-running the previous head (ec32e55), which was green earlier today, would fail exactly the same way now.

I have left dependencies alone on purpose — fixing them belongs in its own change:

  • brace-expansion is fixable with npm audit fix, but that rewrites package-lock.json
  • Electron cannot be fixed in range: the audit asks for 43.7.6, outside the declared dependency range, so it is a version-policy decision rather than a patch

Happy to open a separate lockfile fix once someone decides how far Electron should move.

@PeterGuy326

Copy link
Copy Markdown
Contributor

Status update at current head (branch synced with latest main):

  • I ran "Update branch" for you, so feat/delivery-acceptance now includes latest main (including the v0.4.0 release train and the test-worker-chain security fix). Fresh CI on the updated head: Dependency review, OpenSSF Scorecard, Node 24 (ubuntu + macos), Layout parity, Unpacked staging smoke (Windows + macOS), Unsigned installers (Windows + macOS) — all green. The earlier security:check audit failure you diagnosed is gone on the updated head.

  • Remaining blocker is structural, not about your code: this PR's head branch lives on your fork, and CodeQL is a required check that is always SKIPPED for fork PRs, so branch protection can never be satisfied from a fork branch. To get this merged, please recut the branch into the bytefolk/roleweave repository (same branch name is fine) and repoint or reopen the PR from it — the now-updated fork branch can simply be pushed as-is to a same-repo branch, no rebase needed. Once CI (including CodeQL) is green on the same-repo branch, this can merge.

This branch has not been deployed

No deployments
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.

feat(approvals): read buyer delivery acceptance off the existing approval contract (C2)

3 participants