roles: collapse vault roles into a single instance-role tier#154
Draft
dangtony98 wants to merge 4 commits intomainfrom
Draft
roles: collapse vault roles into a single instance-role tier#154dangtony98 wants to merge 4 commits intomainfrom
dangtony98 wants to merge 4 commits intomainfrom
Conversation
|
💬 Discussion in Slack: #pr-review-agent-vault-154-roles-collapse-vault-roles-into-a-single-instance-role Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Replaces the two-axis permission model (instance role + per-vault role) with a single instance-level role: owner, admin, or agent. Owners auto-access every vault; admins and agents are scoped via vault_grants (now a pure scope table). The agent role is programmatic-only and proxy-only — agents can use the proxy and raise proposals but cannot reveal credentials or approve/reject proposals. - Migration 045 maps existing data with highest-wins semantics: any admin/member grant keeps an admin instance role; proxy-only grants downgrade to the agent role; owner grants are dropped. - Middleware collapses to requireVaultAccess + requireVaultAdmin via a shared resolveVaultActor helper that always returns the actor. - Admin invites enforce two safety rails: cannot grant a role above the inviter's own, and cannot pre-assign vaults outside the inviter's scope. Agents cannot create invites. - Proposal notifications now include all instance owners alongside scoped admins. - Skill docs, public docs, CLI flags, and the SPA invite/scope flows are aligned with the new model.
- migration 045: fold the proxy-only admin downgrade into the
agents_new INSERT...SELECT so the new value 'agent' is written
against the relaxed CHECK rather than the existing
CHECK(role IN ('owner','admin')) on the live agents table. The
prior standalone UPDATE bricked any deploy that had at least one
proxy-only admin agent — exactly the cohort this PR targets.
- RegisterFirstUser: drop the legacy vault_grants INSERT that still
referenced the removed `role` column. After 045, every fresh
install would 500 on POST /v1/init. Owners auto-access every vault
under the unified instance-role model, so no per-vault grant is
needed; defaultVaultID is now removed from the signature, the
Store interface, mockStore, and both callers.
- agent listings: include owner agents in /v1/agents and
/v1/vaults/{name}/agents for non-owner viewers. Owner agents have
no vault_grants rows, so the previous filter (and the inner-join
in store.ListAgents) silently dropped them, hiding which owner
agents may be hitting a scoped admin's vault through the proxy.
Pending owner-role invites are also surfaced.
Tests: added regression coverage for first-user onboarding after
migration 045 and for owner-agent visibility to a scoped admin.
A deeper review surfaced two correctness gaps beyond the originally flagged review comments: 1. sessions.agent_id lost ON DELETE CASCADE in migration 045's table rebuild. Migration 035 originally set it, and the cascade survived through 044 (only ALTER TABLE was used). The 045 rebuild silently dropped it. In practice agents are revoked rather than deleted, but this is a regression in defensive behavior — restored to match the prior schema. 2. UpdateUserRole and UpdateAgentRole did not maintain the migration's invariant that owners have no vault_grants rows. Promoting an admin with explicit scope to 'owner' left orphan grant rows; demotion was already correct (operator must explicitly re-grant). Wrapped both updates in a transaction that DELETEs the actor's grants when the new role is 'owner'. Test added: TestUpdateRolePromotionToOwnerClearsGrants exercises both the user and agent paths. go test ./... and go vet ./... both clean.
Under the unified instance-role model, owners auto-access every vault
and non-owners only see vaults they're explicitly scoped to — there is
no longer any reason for a "Join" affordance. But the SPA was still
splitting vaults into "My Vaults" / "Other Vaults" and showing a Join
button on every entry whose `membership` field was `"implicit"`.
The backend listed *every* vault as `"implicit"` for owners, so the
UI rendered every vault — including ones the owner just created —
as unclickable with a Join button. The Join endpoint itself was
already a no-op shim that only owners could call, making the button
doubly useless.
Rip out the whole concept:
- handleVaultList: drop the `membership` field and collapse the two
branches; both owners and non-owners get a flat list.
- Delete handleVaultJoin and its `POST /v1/vaults/{name}/join` route.
- Delete `agent-vault owner vault join` and its docs accordion.
- VaultsListTab: drop the membership field, the My/Other split, the
Join button, and the Link/div fallback — every card is just a Link.
- Tests: drop the three join tests and the membership assertions in
the list tests.
dangtony98
force-pushed
the
roles/unified-instance-tier
branch
from
a416085 to
72eb5c2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the two-axis permission model (instance role + per-vault role) with a single instance-level role:
owner,admin, oragent. Owners auto-access every vault; admins and agents are scoped viavault_grants, which becomes a pure scope table (no role column). The newagentrole is programmatic-only and proxy-only — agents can use the proxy and raise proposals but cannot reveal credentials or approve/reject proposals.The plan that drove this change:
/Users/tuandang/.claude/plans/at-the-moment-there-deep-honey.md.Highlights
045_unified_instance_roles.sqlmaps existing data with highest-wins semantics: any admin/member grant keeps the admin instance role, proxy-only grants downgrade toagent, owner grants are dropped (owners now auto-access every vault).requireVaultAccess+requireVaultAdminvia a sharedresolveVaultActorhelper that always returns the resolved actor — proposal handlers no longer re-resolve.TestUserInviteAdmin*/TestAgentInviteAdmin*.vault_rolefield is gone everywhere.actorVaultScope/scopeContains/canManageInvite/vaultGrantsToJSON/inviteVaultsToJSONto deduplicate four scope-build patterns, three invite-auth blocks, and six vault-list flatten loops; replaced raw\"owner\"compares withUser.IsOwner()/Agent.IsOwner()predicates on the hot paths.Public API changes
POST /v1/vaults/{name}/users/{email}/role,POST /v1/vaults/{name}/agents/{name}/role— per-vault role no longer exists.vault_rolefrom the per-vault entries; agent-inviteroleacceptsowner | admin | agent.GET /v1/vaults/{name}/contextnow returnsrole(the actor's instance role) instead ofvault_role.POST /v1/vaults/{name}/joinbecomes a no-op compatibility shim for owners.Test plan
make test(16 packages green locally)make build(frontend + Go binary green locally)vault_grants; run the binary; confirm migration applies, owners auto-access vaults, agents with only proxy grants downgrade to instanceagent.Settings/Users/Agents/Credentialstabs render correctly under all three roles.