Skip to content

Approval binding (#52) follow-ups: pinning test, docs wording, loud invalidateFor, release TOCTOU #97

Closed

Description

@hsliuustc0106

Non-blocking findings from the independent review of #94 (rounds 1–2, tips fe8eef2 → f87ed85). None blocks the merge; collected here so they are not lost.

  1. Pinning test for the invalidation narrowing (should-fix first). Round 1→2 narrowed Chief-replacement invalidation to sender-side holds: a hold addressed to a replaced Chief stays pending and decidable. Correct per docs/subsystems/team.md (decision authority is the sender Bot's owner; target reach is re-judged by reach() at release), but no test pins it — the existing test holds with the demoted Chief as sender. Add: hold from another Bot to the Chief, replace the Chief, assert the hold is still pending and the owner can still decide it.

  2. Docs wording (trivial). team.md (approval-binding section), plugin-runtime.md invariant 10 and the ds-bot-core.md row list "demoted Chief" among release()/staleReason causes; the code realizes it only via the eager setMain wrapper (hooks.js), not inside staleReason. Observable contract holds; reword to "invalidated eagerly on setMain".

  3. invalidateFor(botIds, reason, field) fails silent (hooks.js ~327). An unexpected field value no-ops the whole sweep instead of throwing. Two call sites today; make an unknown value throw so a future call site cannot silently skip invalidation.

  4. Pre-existing, in-flight release TOCTOU (outside the [Team] Preserve request provenance and owner-bound approvals #52 diff, note for [Team] Recover task handoffs without duplicate work #55/[Team] Bound agent loops and resource consumption #56). Between release()'s revalidation and dispatch(), a concurrent setMain/deleteBot invalidation skips the record (status already approved) and the send can go out on the old verdict. Bounded in-process window; ownerPolicy (reach()) re-runs at release, which covers the per-Bot-grant half. Natural home: the recovery/dedup work ([Team] Recover task handoffs without duplicate work #55) or budget/cancellation ([Team] Bound agent loops and resource consumption #56), whichever touches release first.

Related: #52, #94, #55, #56.

Activity

  1. Birfy commented on Oct 16, 2026

    @Birfy
    Collaborator

    Fixed on main (566f1e6) by #135. All four items:

    1. Pinning test: replacing a Chief leaves holds addressed to it pending and decidable in core/test/approval-binding.test.js.
    2. Docs wording: team.md, plugin-runtime.md invariant 10 and ds-bot-core.md now say a demoted Chief's sender-side holds are invalidated eagerly on setMain, not inside staleReason.
    3. invalidateFor: an unknown field now throws (hooks.js). It has no dedicated test.
    4. Release TOCTOU: invalidateFor also covers approved-but-not-yet-dispatched records, and release() re-checks staleReason right before dispatch(). Test: an in-flight approve is invalidated if the Chief is replaced before dispatch.

    Closing as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions