Closes #10.
What was wrong
access.Store had no version check, and every editor read the row, changed a field
and wrote the whole document back. SaveSettings in particular loaded the ACL and
rewrote it when the visibility changed. A repository admin’s stale save could
therefore restore a role an instance admin had just revoked.
Separately, approvals were counted by file alone and ACL.CanReport accepted any
name in the runners list, so a demoted maintainer’s approval and a retired
account’s CI report kept satisfying the gates.
What this does
Compare-and-swap on the rules. repo_acl gains a revision column, added on
open for existing instances. Get attaches the revision it read; Set updates
with WHERE revision=? and returns access.ErrConflict when it lost a race, and a
from-scratch rule set inserts only if no row exists. No stale document can replace
a live one, even from a caller that never learned about revisions.
Field-scoped updates. Store.Update re-reads, applies a function, and retries
the swap. The paths that own one field now use it:
SaveSettingswrites only the visibility; entries, runners and the approval policy are untouched, which is what closes the settings/role-removal race.AddRepositoryAccesswrites one entry.SaveRepositoryAccessapplies the submitted fields to the current row and preserves entries/runners for accounts the form did not render.
Approvals bound to current standing. FindApprovedMatch takes an eligibility
predicate, and landing.Gate supplies ACL.EligibleApprover: active account,
currently maintain or admin. The request page and list count with the same
rule and show an approval that is current but no longer counts as “no longer
counts”, so the page cannot promise a merge the gate refuses.
Runners bound to current standing. ACL.CanReport refuses a retired account
even while its name is in the list: the ACL row outlives the account, so
membership alone would let a revoked account keep vouching for builds and leasing
jobs. The roster predicate is attached by the store in both the server and the hook
process, beside the instance-role lookup that was already wired there. An explicit
runner still does not need a repository role, which is the documented design.
Scope notes
- The SSH admin and CLI
aclcommands still replace the whole document, but now through the same compare-and-swap, so a concurrent edit producesErrConflictrather than a silent overwrite. Converting each of those subcommands to a field-scoped update is a follow-up, not a correctness gap. - Two instance admins saving the access form at once get last-writer-wins per field. The issue’s race (repository admin vs instance admin) is closed, and a hidden revision field on the form is the next step if we want that case reported instead of merged.
Verification
gofmt,go vet ./...,go test -race ./...clean../vole workflow checkand./vole workflow run verifypass.
Tests, each failing before its fix
TestARepoAdminSaveCannotRestoreARevokedRole: read, revoke, then write the stale rule set — refused, and the role stays revoked.TestConcurrentUpdatesAllLand: eight concurrent one-entry updates all survive.TestUpdateKeepsAConcurrentChange,TestRetiredAccountsStopCounting,TestEligibleApproverNeedsMaintainOrAdmin.TestLandRefusesAnApprovalFromADemotedMaintainer,TestLandRefusesAnApprovalFromARetiredAccount.TestSettingsSaveLeavesAccessRulesAlone: a settings save leaves entries, runners, approval policy and protection as they were.TestARetiredRunnerMayNotReportAResult(hook).
Docs: accounts-and-access.md describes the compare-and-swap and that retirement
reaches the runners list; change-requests.md describes when an approval stops
counting.