Repository navigation
Enforce single-occupancy slot pinning with human-approved moves - #16
josephschorr wants to merge 13 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
3ac96c9 to
97e6c39
Compare
Review: slot pinning (verified against the branch head)Each finding below was checked against the code, not just flagged. Fixes are in progress locally. Should fix before merge
Needs a decision
Minor
🤖 Generated with Claude Code |
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Fixes for the review findings (1db0c42)Pushed fixes for the findings in the review above.
Not changed: scope-first ordering in TestsNew regression tests:
🤖 Generated with Claude Code |
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1db0c42 to
a712433
Compare
AuthzSlot gains occupancy (single|multi, default single) and rebind (approval|never, default approval). The installer completes both before SSA apply, the same way membership is completed, so a byte-identical re-install stays a no-op. Class admission rejects a single-occupancy slot declaring more than one default. AgentSession status gains a display-only slotPins mirror type; SpiceDB remains the only enforcement source of truth.
The schema composer adds a non-expiring slot_pin relation to every slot resource definition. GrantSlots — the one funnel every slot-grant write passes through — gates single-occupancy bindings: the first bind writes the pin under a MUST_NOT_MATCH precondition, grant writes carry a MUST_MATCH guard so a bind can never race an approved move, a different instance is refused with a message routed by the slot's rebind policy, and a single call naming two instances of one single-occupancy type binds nothing for that type while other types still land. Grants keep their mandatory expiry; the pin does not expire. Tuple validation refuses tool-authored writes to slot_pin and slot_grant relations, and a guard test over pkg/, internal/ and cmd/ asserts no production write bypasses the gate. Pinned grant writes and moves advance the read-your-writes freshness floor.
…d amendment Every producer that reaches GrantSlots — defaults, extracted and observed promotion, thread seeding, trigger binding, tool approval and the plan gate — copies the slot's occupancy and rebind onto its bindings. A plan amendment that names a different instance for a filled single-occupancy slot records the displaced instance on the approval card at build time; the approved move executes atomically (the pin repoints under a MUST_MATCH guard and the displaced instance's actual grants are revoked in the same request), and the card shows the move as a move: current instance, proposed instance, and the revocation. A move whose recorded starting point no longer matches the live pin fails the approval loudly rather than moving something the approver never saw.
The runner mirrors pins onto AgentSession.status.slotPins — derived from SpiceDB reads after a successful bind, never from candidate lists, so the mirror cannot claim a pin that was never written — and oap session show renders them. Pin refusals from the promotion paths are recorded per resource type (newest wins, cleared when that type later binds or moves) and annotate the next denied tool call for that type, so the model is told the pinned instance and the route out rather than reading a bare denial as a system fault. A pin failure after a plan-gate approval tells the approver whether the slot had drifted or was already committed elsewhere, with accurate claims about what was granted.
channelsd pre-stamps the session finalizer at its create site and sweeps stale slot tuples itself before trigger and thread-seed binding, so the operator's admission sweep can never race the mint and silently delete legitimate authority; a sweep failure fails the session start rather than proceeding past a possible leak. Mint-time pin refusals record a SlotBindRefused session notice with the refusal text, with the suggested next step routed by the slot's rebind policy. A tool approval that lands on a pinned slot in a class with no plan gate is rejected naming the pinned instance and the new-session route.
Fork and restart copy grant and pin tuples verbatim — a lifecycle clone is not a new arrival and never re-enters the gate — while session teardown deletes pins alongside grants (pins never expire, so GC is the only thing that removes them). A new session's first reconcile sweeps stale slot tuples left under its name by a dead predecessor, except for fork children, whose tuples are the parent's legitimate copy and whose generated names make name reuse moot. The status webhook waives slotPins for the runner: it is a display-only mirror no authorization reads.
A scenario suite drives the full arc on the in-process harness: the first bind pins, a second instance is refused and never written, an approved amendment moves the pin and revokes the displaced instance's grants, rebind: never admits no move, and two concurrent first binds resolve to exactly one winner through the precondition rather than process ordering. A scenario bundle proves the move card on a real published approval; the existing re-point bundle keeps its re-ask coverage, and two fixtures that bind a constant workspace instance alongside the chosen repository under one resource type now declare that slot multi-occupancy, which is what they are.
The security model's slot section now describes what the code enforces: the commitment is a non-expiring pinned relationship written under an atomic precondition; a filled single-occupancy slot cannot be silently reopened — a different instance needs a human-approved plan amendment that moves the pin and revokes the old instance's access, and rebind: never admits no move. The stated limits are documented plainly: pins are per resource type, pin identity is derived-ID string equality, moving a pin requires the plan gate, and a slot binding a constant instance alongside the chosen one needs multi occupancy or split types.
Addresses the verified findings from the PR #16 review: - rebind: never is enforced on the approval path. The plan gate no longer stamps a move for a never slot (new PlanGateDeps.SlotRebind), and moveApprovedPins refuses one as a backstop. - First-approval and whole-plan cards show the move. The plan's own slots now carry MovedFrom, the whole-plan renderer prints the move and the revocation, and CardLine.MovedFrom becomes a separate revocation line on the wire. - The AgentSession admission sweep runs before the finalizer is added, so a failed sweep is retried on requeue instead of skipped forever. - moveApprovedPins refuses deterministic conflicts (a never slot, or a second instance of the moving type in the same approval) before any MovePin. A grant failure after a committed move returns ErrSlotMoveCommitted, and a MUST_MATCH failure returns ErrSlotMoveDrifted, so the approver notice says what actually changed. - GrantSlots collects EnsurePin/WriteGrantsPinned errors instead of returning early, so later types and the multi-occupancy write land. - The slot occupancy lookup logs the error it used to drop. - The occupancy: single default is documented as an upgrade break (CRD UPGRADE NOTE, security model), with CRDs, install.yaml and CRD docs regenerated. mage test:unit passes. mage test:integration and mage test:e2e were not run: Docker was unavailable, so every SpiceDB-backed test failed to start. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
moveApprovedPins' ListGrantsFor error path returned a bare error even after an earlier type's move had already committed (pin repointed, prior grants revoked), so BindApproved reported it as an ordinary failure. The approver was then told nothing was granted while access had in fact been removed — the exact case ErrSlotMoveCommitted exists to flag, and which the sibling MovePin error path already wraps. Wrap it the same way once a move landed.
A plan-scoped approval is resolved one BindApproved call per covered phase, and stampMovedFrom stamps the same pin move onto every covered phase that names the moved-to instance. A single-occupancy pin moves at most once per approval: the first phase repointed repoA->repoB, and each later phase then re-attempted the identical move with a now-stale PriorID=repoA whose MUST_MATCH drift-failed — silently dropping that phase's grant while still recording it approved, and making a re-approval drift-fail every phase. dedupeCoveredMoves keeps the move on only the first covered record per type; the rest bind same-instance against the pin the first move already set. Run before the delegable lookup, since a SlotRef's identity includes MovedFrom and clearing it afterward would leave a later phase's request ref no longer matching the delegable key looked up for it.
a712433 to
48f371e
Compare
…rror A plain pinned refusal on the approval path — the slot is committed to a different instance and no approved move applies — was published to the approver under categories.InternalError, whose meaning is "we broke, and retrying is the remedy." It is neither: re-approving the same card cannot change it, as the notice copy itself says. Publish it under the refusal-flavored SlotBindRefused instead (same ToneDegraded weight; the category is what a surface reads to tell a fault from an answer). The fault and committed-move cases keep InternalError.
Review of the fix commits (afbc1d8, b5f447d, 48f371e, 05928fe)Every finding below was checked against Security summary: none of these lets a session reach an instance nobody approved. The pin still fails closed, and every path ends with less access, not more. The weak spot is the human's consent and the information they get:
Weakens the approval guarantee1. A merged whole-plan line hides the revocation ( 2. Store errors are folded into pin refusals (
Effect: a small information leak (internal error detail and endpoints reach the model and chat users) and a misleading answer to the approver. 3. "Move committed" can name the wrong failure ( 4. A 5. Refusal advice that can't work (
"To target the committed instance" also points at the wrong instance; it should be the new target. channelsd already changes its wording for Fail-closed availability bugs (access lost, never gained)6. A failed later move drops an earlier move's new grant ( 7. Pre-move refusals block unrelated types ( 8. Later phases depend on phase 1's move ( 9. Suggestion: a simpler fix for item 8 ( Suggested orderFix items 1 and 2 first. Most of 3, 6, 7 and 8 go away if 🤖 Generated with Claude Code |
Fixes the nine findings from the review of the fix commits: - Whole-plan card: pre-scan for a move per derived-id so the merged (git/gh) line still shows the revocation when the moving type loses the dedup, instead of rendering it as a plain first-fill. - GrantSlots store failures carry a distinct ErrSlotStore fault sentinel: a transient blip joined with a real pin refusal now classifies as a retryable fault, and its raw store text no longer leaks to the model or the chat notice. - Fold the approved pin move into GrantSlots' per-type loop and delete moveApprovedPins, so one type's move failure can no longer mislabel, strand, or block a sibling type's grant. - MovePin is idempotent at the target (pin already names toID -> no-op, not drift); dedupeCoveredMoves removed, so multi-phase approvals are order-independent and every covered phase keeps its MovedFrom audit. - A rebind:never slot pinned elsewhere renders as a blocked line (new PinnedImmovable field) rather than a grantable-looking first-fill. - Pinned-refusal notice routes by rebind policy (new session for never, amendment for movable) and names the instance the approval wanted, not the committed one. allRebindNever lifted to shared authz.AllRebindNever. All three suites green: mage test:unit, test:integration, test:e2e.
Fixes for the second-round findings (b1910e0)All nine addressed. The consent-accuracy and leak issues are fixed at the surface; 3/6/7/8/9 are gone by construction — the approved move now runs inside
|
This branch was successfully deployed
2 participants
What this changes
The security model has long claimed that a slot, once filled, "cannot be reopened" and that the slot write "checks that the slot is still empty as a precondition." Neither was enforced: slot grants were plain TOUCH writes, and a session bound to resource A could silently bind resource B of the same type through any bind path. This PR makes the claim true by construction, with one honest amendment: a slot cannot be silently reopened — a different instance needs a human's approval.
How it works
Declaration.
AuthzSlotgains two fields:occupancy: single | multi(defaultsingle) andrebind: approval | never(defaultapproval). The installer completes both before apply so re-installs stay SSA no-ops, and class admission rejects a single-occupancy slot declaring multiple defaults.Enforcement. The schema composes a non-expiring
slot_pinrelation onto every slot resource type. The first bind of a single-occupancy slot writes the pin under an atomicMUST_NOT_MATCHprecondition atauthz.GrantSlots— the single funnel every production slot-grant write already passes through — and grant writes for pinned slots carry aMUST_MATCHguard so a bind can never race a move. A different instance is refused with a message that names the pinned instance and the route out, routed by the slot'srebindpolicy. Grants keep their mandatory expiry and revocability; the pin persists for the session's life, so expiry never silently reopens the slot. Tool-authored writes toslot_pin/slot_grant_*relations are refused at tuple validation, and a guard test asserts no production write bypasses the gate.The approved move. A plan amendment naming a different instance records the displaced instance on the approval card at build time. Approval executes the move atomically: the pin repoints under a
MUST_MATCHguard and the displaced instance's actual grants are revoked in the same request. The card shows the move as a move — current instance, proposed instance, and the revocation — and a move whose starting point no longer matches the live pin fails the approval loudly instead of moving something the approver never saw.Lifecycle. Fork/restart copies grant and pin tuples verbatim (a clone is not a new arrival); teardown deletes pins alongside grants; a new session's first reconcile sweeps stale tuples left under a reused name, with channelsd sweeping before its own mint-time binds so the two can never race.
Visibility. Pins mirror onto
AgentSession.status.slotPins(display-only; SpiceDB stays the only enforcement input) and render inoap session show. Pin refusals annotate the model's next denial for that type with the pinned instance and the route out, and mint-time refusals record a session notice.Documented limits
occupancy: multi(or split types).Migration
Existing classes pick up the defaults on their next apply. Multi-instance classes must declare
occupancy: multi. Pre-existing sessions have no pins; the first bind after upgrade writes one.Verification
All three suites green at the tip (
mage test:unit,mage test:integration,mage test:e2e, including all 82 scenario bundles). New coverage includes a real-SpiceDB end-to-end arc (pin → refusal → approved move → revocation), a concurrent first-bind race resolved by the precondition rather than process ordering, a red-first regression test for the fork/sweep interaction, and a scenario bundle asserting the move card on a real published approval.