0.22.0: four ACL fixes and features (#104, #105, #96, #106) by drernie · Pull Request #107 · quiltdata/quiltx

drernie · 2026-09-02T09:09:19Z

Closes #104, closes #105, closes #96, closes #106.

Four independent ACL changes, one release. Each is described at length in
CHANGELOG.md; this is the short version.

#104 (bug) — `users:` keys resolve only by username

Keys were matched against user.name, and user.name is not one thing: the
registry derives it from email[:64] for SSO self-registrations, while an
admin-created account must satisfy ^[a-z][a-z0-9_]*$. So one captured ACL
mixes both shapes with nothing marking which is which, and writing someone's
email when their account is under a handle produced a nonfatal notice and
silently skipped the grant.

Keys now resolve by exact username first, then by a unique case-insensitive
email match. Precedence decides, so a key that names an account still means
that account even when a different account holds it as an email — that clash
is a warning naming both, because making it fatal would reject this tool's own
--yaml capture, which keys every entry by user.name. Two things remain
fatal: a key that names no account and is the email of two, and two keys
resolving to one account. Everything downstream (the SDK call, downgrade
analysis, verbose output) now uses the resolved server username.

#105 (feature) — `config.default_policy`, a floor rather than a fallback

config.default_role fires only when a user's SSO claims matched no mapping,
so a specific grant cost a user the general one — one real config carried
twelve hand-maintained extra_roles entries expressing a single intent. A
default policy composes into the roles a user already matched, so it grants the
same permissions without adding a role to anyone's set. Requires
config.synthesize: false; unmanaged roles are out of reach and that
combination warns.

Because the floor becomes a dependency of every managed role, a default policy
that fails to create now names itself as the cause in a
!! DEFAULT POLICY MISSING block instead of leaving one unknown policy line
per role to explain it.

#96 (feature + fix) — per-bucket `config.no_preflight`, and loud failures

A cross-account bucket already prepared owner-side cannot be preflighted by the
catalog admin applying the ACL, so the add failed and the bucket was never
registered (sierra-general on open.quiltdata.com needed two attempts and the
global flag). The new top-level buckets: block keeps that fact with the
bucket; --no-preflight remains a global override.

Separately: a bucket that does not register is now a named
!! BUCKET REGISTRATION FAILED block, and the command re-reads the catalog
afterwards and exits non-zero for any bucket it still does not hold — so an add
that returns cleanly without registering is caught too.

#106 (feature) — `--create-and-email-users`

users: only ever reaches accounts that exist, so an ACL file could not onboard
anyone. Creation is driven from sso.email (unambiguous identifiers, and the
role is implied by nesting so it cannot drift from what the SSO mapping grants)
The admin API requires a username, so quiltx cannot use the registry's own
email[:64] derivation and instead folds the whole address into a handle the
grammar accepts: alice@example.com -> alice_example_com. The domain is folded
in rather than dropped, because a local-part handle maps alice@example.com and
alice@contractor.example onto one name and only one account can hold it. The
handle is an administrative label, not the identity: first SSO login reconciles
against the pre-created account by email. Folding is not injective, so the handle
is checked against server usernames and against the rest of the roster, and any
clash refuses those addresses instead of picking a winner.

An address whose local part an existing account already uses is also refused.
quiltx never calls set_email, so an address nobody holds may be a person whose
address changed, and creating there would split one person across two logins.
Role availability is read from the server on a real run, never from the plan, so a
role whose create failed cannot turn into a creation the registry will reject.

There is deliberately no config key. The registry mails a welcome and
password-reset link as part of creating an account, with no suppress flag, so a
config default would make the first apply the irreversible one. The flag is
named for that side effect, prints every address before asking, and refuses more
than --max-created-users (default 10).

Testing

651 passed, 1 skipped (was 548 on main); ./poe lint-check clean. New tests
cover each feature plus the cross-feature seams: the --yaml capture/replay
round trip under a username/email collision, the failed-default-policy cascade,
per-bucket vs global preflight mode in one apply, and the two-prompt ordering
where declining the apply prompt creates nobody.

A design-level review of the combined diff caught seven issues that are fixed
here rather than filed, including the capture/replay regression above and a
silent last-write-wins when two users: keys addressed one account.

Not addressed

The tool analyses access reductions only, so a default policy that broadens
every managed role shows as ~ role X at default verbosity (--verbose
lists the composed policies). That asymmetry predates this branch. There is
also no per-role opt-out from a default policy; both are worth their own issues
if wanted.

Greptile Summary

The PR releases QuiltX 0.22.0 with four coordinated ACL enhancements and associated documentation and tests.

Resolves configured users by username or unique email while detecting ambiguous or duplicate assignments.
Adds default policies that compose a permission floor into every managed role.
Adds durable per-bucket preflight bypass configuration and stronger registration-failure reporting.
Adds explicit, capped onboarding of missing sso.email users after ACL reconciliation.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code defect identified.

The new ACL behaviors preserve the reconciliation sequence, explicitly handle ambiguous identities and partial failures, verify bucket registration against refreshed state, and gate irreversible user creation behind an explicit capped CLI operation.

Important Files Changed

Filename	Overview
quiltx/acl.py	Extends ACL parsing, desired-state construction, user resolution and onboarding, default-policy composition, per-bucket registration behavior, and apply diagnostics without an accepted correctness issue.
quiltx/tools/catalog/acl.py	Integrates the new CLI flags, confirmation and creation flow, preflight notices, post-apply checks, and warning-based exit behavior.
tests/test_acl.py	Adds broad coverage for the four ACL changes, including collisions, failure cascades, prompt ordering, registration verification, and cross-feature behavior.
uv.lock	Updates only the local QuiltX package version; flagged third-party dependency versions predate this PR.
README.md	Documents the new default-policy, per-bucket registration, user-resolution, and explicit onboarding contracts.
pyproject.toml	Bumps the package version from 0.21.0 to 0.22.0.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Parse ACL YAML] --> B[Build desired policies, roles, SSO, users, and buckets]
  B --> C[Fetch current catalog state]
  C --> D[Compute ACL diff and downgrade warnings]
  D --> E{Dry run?}
  E -- Yes --> F[Print ACL and user-creation plans]
  E -- No --> G{ACL changes present?}
  G -- Yes --> H[Register buckets]
  H --> I[Reconcile policies and roles]
  I --> J[Update users, default role, and SSO]
  J --> K[Re-read state and recover policy drift]
  G -- No --> K
  K --> L{Create-and-email flag enabled?}
  L -- Yes --> M[Plan, confirm, cap, and create roster users]
  L -- No --> N[Report final status]
  M --> N

_{Reviews (1): Last reviewed commit: "0.22.0: four ACL fixes and features (#10..." | Re-trigger Greptile}

Context used (3)

Knowledge Base — Catalog access control and API keys
Knowledge Base — Stack access-control configuration
Knowledge Base — Detect and recover silent ACL policy drift

Resolve `users:` keys by username or email (#104). Keys matched `user.name` only, which is email-shaped for SSO self-registrations and handle-shaped for admin-created accounts, so writing someone's email when their account uses a handle silently skipped the grant. Precedence now decides, a clash with a different account's email is a warning rather than a rejection of the tool's own `--yaml` capture, and only genuinely undecidable keys are fatal. Add `config.default_policy: true`, the floor every managed role stands on (#105). `config.default_role` is a fallback that fires only when no SSO mapping matched, so a specific grant used to cost a user the general one. Composing at the policy layer grants the same permissions without adding a role to anyone's set. Add per-bucket `config.no_preflight` under a new `buckets:` block (#96), so a cross-account bucket already prepared owner-side registers without the global all-or-nothing flag, and make a bucket that does not register a loud, named, non-zero-exit failure instead of one warning among many. Add `--create-and-email-users` (#106), which creates accounts for `sso.email` roster addresses that have none. CLI-only, because the registry mails a welcome and password-reset link as part of creating an account with no suppress flag; the addresses are printed before the prompt and a run over `--max-created-users` is refused.

drernie

Code review — 0.22.0 ACL changes

Reviewed the single commit (df1a46f, ~1000 lines of new logic in quiltx/acl.py and quiltx/tools/catalog/acl.py): the buckets: block with per-bucket config.no_preflight, config.default_policy, email-keyed users: resolution, --create-and-email-users, and the new failure-reporting blocks. Working tree clean; ./poe test passes (618 passed, 1 skipped).

Three findings, inline below — the first two make a documented configuration unusable:

--create-and-email-users cannot create anyone (quiltx/acl.py) — the derived username is the email address, which the registry rejects.
config.default_policy + any config.unmanaged role makes every successful apply exit 1 (quiltx/acl.py) — an informational note lands in diff.warnings, and _run exits 1 on any non-empty warnings.
_unmakeable_accounts double-counts held addresses (quiltx/tools/catalog/acl.py) — minor reporting bug.

Areas I checked and found sound: per-bucket graphql_only_buckets selection and the narrowed control_account_id fetch in apply_acl; _resolve_configured_user precedence, collision, and the two fatal cases; default_policy composition after the ladder (no KeyError path — role_updates always holds every non-unmanaged role name, and source_policies/_synthesized_role_name stay untouched); default_policy_titles threading into the KeyError handlers; resolved_users plumbing through analyze_user_downgrades/_projected_user_access and the applied_user_names back-compat branch; the unregistered post-apply bucket check; and the new parse validation for buckets:, config.default_policy/synthesize, and entry-key hints.

🤖 Generated with Claude Code

Review finding 1: `--create-and-email-users` could never create anyone. The derived username was the email address, and the registry validates an explicitly supplied username against `^[a-z][a-z0-9_]*$`, deriving `email[:64]` itself only when the name is omitted — which quiltx cannot do, since `UserInput.name` is required. Every address therefore failed at the registry. The grammar is now a documented constant, `plan_user_creations` refuses an address it cannot name, and no derivation is invented: mapping an address to a handle depends on whether an SSO login reconciles a pre-created account by email, which is registry behaviour this repo cannot verify, and guessing it wrong would send irreversible mail and orphan the roles. Review finding 2: `config.default_policy` plus any `config.unmanaged` role made every apply that had work to do exit 1. The note saying default policies do not reach unmanaged roles is informational, but it landed in `diff.warnings`, and a non-empty warnings list means failure. `_DesiredAclState` gains a notices channel and the note goes there, printing as NONFATAL. Review finding 3: `_unmakeable_accounts` counted an ambiguous already-held address as uncreatable, reporting one address twice in contradictory ways. `UserCreationPlan` now separates notices from warnings. Fix #110: a missing default policy no longer deletes anything. Because the floor composes into every managed role, its absence made every role create fail — and the run then continued into the delete phases, removing the roles and policies the file drops while provably unable to create the ones it adds. `apply_acl` now stops at a gate before the role loops, keeping the policy changes that landed and deleting nothing. Fix #112: CLI-level coverage that an unresolvable `users:` key exits 1 with no mutation attempted. The behaviour was already correct; the tests pin it against a fixture that would otherwise mutate.

drernie · 2026-09-02T23:03:06Z

Second commit: `92ba139`

Addresses all three review threads plus two follow-up issues. 627 passed, 1 skipped; ./poe lint-check clean; CI green on all six jobs.

Review findings

All three reproduced before fixing, so none of these are speculative.

Exit code punished a documented config. The CLI exits 1 on any warning, and the "default policies do not reach unmanaged roles" note was a warning. A config of exactly the shape stack-acl.example.yaml documents — a default policy beside a config.unmanaged: true role — therefore reported failure on every --yes run that had work to do, with that note as the only warning and every API call successful. Split into _DesiredAclState.notices -> diff.notices, printed as NONFATAL:, outside the exit-code decision. _resolve_policy_admin_vote stays a warning: that one is a genuine conflict between two lines of the config, not a note about something quiltx declines to touch.

Roster notices inflated the same exit code. UserCreationPlan mixed notices into .warnings. Split; only .warnings reaches the exit code.

_unmakeable_accounts double-counted. An address that two accounts already answer for was counted as onboarded and again as uncreatable.

#110 — a failed default policy blocked every managed role

Fail-fast gate between the policy phase and the role phase. The issue described the symptom (every role fails); tracing the phase order found the destructive part: the role loops continue rather than abort, so the run reached roles_to_delete and policies_to_delete and deleted the roles and policies the file drops while provably unable to create the ones it adds. The gate returns before both delete phases. It only fires when the diff has a role create or update, since a delete-only apply composes the floor onto nothing and must not have its deletes suppressed. The default_policy_skips branches in the two KeyError handlers are now unreachable and were removed rather than left as a weaker second explanation.

Rejected: excluding the failed floor so the remaining roles land. That grants less than the file asks for while making the roles look reconciled — the exact failure a floor exists to prevent.

Verified non-vacuous by mutation: deleting the gate's return warnings fails test_apply_acl_deletes_nothing_when_a_default_policy_is_missing at tests/test_acl.py:3574.

#112 — no CLI-level abort test

Covered the CLI path that turns a refusal into an exit code, which the library-level tests did not reach.

Still open for a maintainer call: `--create-and-email-users` (#106)

The flag ships inert. quilt3.admin.users.create requires name, the registry validates admin-supplied usernames against ^[a-z][a-z0-9_]*$, and it derives email[:64] itself only when name is omitted — which the client does not allow. Every email address therefore derives a username the registry rejects, so plan_user_creations refuses each one up front, emits warnings, and never contacts the registry. No mail is sent.

I did not invent a handle derivation, because the correct one depends on registry behaviour this repo cannot verify: whether a later SSO login reconciles against a pre-created account by email, or opens a second account under its own derived name. Guessing wrong sends irreversible mail and leaves roles on an orphaned account.

Three ways forward, in preference order:

Answer the SSO-reconciliation question, then implement the handle mapping.
Land a quilt3 change making UserInput.name optional so the registry derives it.
Pull acl: --create-and-email-users — create missing users from sso.email rosters #106 from 0.22.0 and re-land it once (1) or (2) is settled.

#104, #105 and #96 are independent of this and unaffected by whichever option is chosen.

SSO login reconciles a pre-created account by email, which is the fact that makes pre-creation safe: the handle quiltx supplies is an administrative label, not the identity. derive_username folds the whole address (alice@example.com -> alice_example_com), keeping the domain so alice@example.com and alice@contractor.example do not contend for one name. The fold always satisfies USERNAME_PATTERN, pinned by a parametrized invariant test rather than a branch that cannot fire. Folding is not injective, so the handle is checked against server usernames and against the rest of the roster in a second pass; either clash refuses those addresses instead of appending a suffix that would depend on the order the file was written in. Existence is not identity. quiltx never calls users.set_email, so an address no account holds may be a person who already has one under their old address, and neither older guard catches it: the duplicate notice needs two accounts sharing one email, and the handle check compares folded addresses. Observed on open.quiltdata.com, where robbyqbutler@pm.me would have created and mailed a second account for an existing robbyqbutler / robbyqbutler@protonmail.com. An address whose local part an existing account already uses is now a warning naming both records, with nothing created. Also corrects the roster docstring: resolution is username-first, then email, matching _resolve_configured_user.

drernie · 2026-09-04T18:26:22Z

Third commit: `0d290c1` — `--create-and-email-users` now creates accounts

Option 1 from the comment above. SSO login reconciles a pre-created account by email, which is the fact that makes pre-creation safe: the handle quiltx supplies is an administrative label (admin UI, --yaml captures), not the identity. That answered the question that was blocking a derivation.

649 passed, 1 skipped; lint-check clean; all six CI jobs green.

The mapping

derive_username folds every character outside [a-z0-9_] to _, prefixes u_ when the result does not start with a letter, and truncates to 64. alice@example.com becomes alice_example_com.

The domain is folded in rather than dropped. A local-part handle maps alice@example.com and alice@contractor.example to one name, and only one account can hold it, so that collision costs one of two people their account — and a catalog with an outside collaborator is exactly where this flag gets used.

The fold always satisfies USERNAME_PATTERN, so plan_user_creations no longer pre-validates the name it just built. That invariant is pinned by a parametrized test over empty, blank, leading-digit, leading-underscore, punctuation-only, non-ASCII and over-length inputs, rather than by a branch that cannot fire. Disabling the u_ prefix fails 9 of those cases.

Folding is not injective (. and + both become _; 64 characters is a hard cap), so the handle is checked against server usernames and against the rest of the roster. The roster check is a second pass over a claimants map, so which address gets refused does not depend on the order the file was written in. No disambiguating suffix: it would make someone's username depend on what else happened to be in the file.

The bug this turned up

Making the flag actually create accounts exposed a duplicate-account hazard, reported against the live catalog.

quiltx never calls quilt3.admin.users.set_email — confirmed, the symbol appears nowhere in the tree. So an address the server does not hold is either a new person or somebody whose address changed, and a changed address is usually why an operator edits a roster. Neither existing guard catches the second case:

the duplicate-account notice fires only when two accounts share one email; here they hold different ones
the handle check compares folded addresses, so robbyqbutler@protonmail.com, robbyqbutler@pm.me and an account named robbyqbutler are three distinct strings

Reproduced before fixing, against the real library:

existing : ()
creations: [('robbyqbutler@pm.me', 'robbyqbutler_pm_me', 'Analysts')]
warnings : ()
notices  : ()

A real run would have created and mailed a second account silently, leaving the person's roles on the new account and their login on the old one. Now:

Warning: Roster address 'robbyqbutler@pm.me' has no account, but its local part
'robbyqbutler' is already used by 'robbyqbutler' (email robbyqbutler@protonmail.com);
not created. ... Set the existing account's email to this address if it is the same
person, or create the account by hand if it is not.

_accounts_sharing_a_local_part matches the roster address's local part against each account's username and the local part of its email, so an email-shaped user.name is covered too. It runs before handle assignment: identity precedes naming, and an address refused there never competes for a handle it was not going to get.

This treats alice@example.com and alice@partner.example as possibly one person while derive_username treats them as two. Not a contradiction — one refuses to silently merge two people, the other refuses to silently split one, and both resolve to reporting rather than guessing. Rejected: calling set_email when a single account "looks like" the same person. That heuristic, guessed wrong, moves a real account's login identity.

Both new gates verified non-vacuous by mutation: disabling either fails exactly the tests written for it.

Doc correction

The roster docstring claimed resolution was "email first, then username". _resolve_configured_user checks users_by_name first. Corrected, along with the passages in AGENTS.md, README.md and stack-acl.example.yaml that described the flag as creating nobody.

No open questions left on #106.

Copilot

🟡 Changes recommended

Post-apply onboarding can attempt irreversible user creation against a managed role whose creation failed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Releases QuiltX 0.22.0 with expanded ACL reconciliation and onboarding capabilities.

Changes:

Adds email-based user resolution and explicit roster onboarding.
Adds default policies and safer failure handling.
Adds per-bucket preflight control and registration verification.

File summaries

File	Description
`quiltx/acl.py`	Implements ACL behavior and validation.
`quiltx/tools/catalog/acl.py`	Integrates new CLI workflows.
`tests/test_acl.py`	Adds comprehensive ACL tests.
`README.md`	Documents user, policy, and bucket features.
`stack-acl.example.yaml`	Expands example configuration guidance.
`CHANGELOG.md`	Records the 0.22.0 release.
`AGENTS.md`	Adds developer implementation notes.
`pyproject.toml`	Bumps the package version.
`uv.lock`	Synchronizes the locked package version.

Review details

Files reviewed: 7/9 changed files
Comments generated: 5
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

plan_user_creations mixed the desired role set into "available", which is right for --dry-run (nothing is applied yet, so a fresh catalog would otherwise flag every address) and wrong after apply_acl: a managed role whose create failed is still in state.role_updates, so the real run could call users.create naming a role the registry does not have. Whether the registry mails the welcome before rejecting is not something quiltx controls, so the attempt must not be made. include_planned_roles now selects between the two, defaulting to the strict server-only reading. Forgetting the flag costs a spurious dry-run warning; the reverse costs an irreversible creation attempt, so the safe reading is the default and _print_user_creation_dry_run opts in explicitly. Docs: the #105 changelog entry called the unmanaged-role note a warning, which contradicts the notice routing it actually got. README and stack-acl.example.yaml claimed quiltx never creates users, which --create-and-email-users contradicts; both are now scoped to the users: block, with deletion still absolute. The role-availability rule is restated in all three. Also dropped the unverifiable claim that the registry mails before rejecting a bad role.

drernie · 2026-09-05T04:25:39Z

Reviewed 0d290c1 against a live case that hit this exact failure mode on open.quiltdata.com, and the guard does what it claims. Recording the verification here, plus two residuals.

The rename guard holds

The case: an account existed as robbyqbutler / robbyqbutler@protonmail.com; the roster was updated to robbyqbutler@pm.me. Running plan_user_creations against the live catalog, with that account's email shimmed back to its pre-change value:

--- PRE-FIX  (@protonmail.com) ---
  creations naming robby: NONE
  ! WARN: Roster address 'robbyqbutler@pm.me' has no account, but its local part
    'robbyqbutler' is already used by 'robbyqbutler' (email robbyqbutler@protonmail.com);
    not created. ...

--- POST-FIX (@pm.me, live) ---
  existing naming robby : ['robbyqbutler@pm.me']

At 92ba139 the same input produced creations: [('robbyqbutler@pm.me', 'robbyqbutler_pm_me', 'communional')] with no warning and no notice — a second account and an unrecallable welcome mail, silently. So the commit closes a real gap, not a theoretical one.

Across the full live roster it is 24 creations, 15 existing, 1 blocked. The one blocked is ernest@quilt.bio, matched against both drernie1901 (email ernest@drernie.com) and ernest@quiltdata.io — a genuine ambiguity that a human should settle, correctly refused and correctly reported with both candidates named.

One note on the reasoning, because I got this wrong first: I had assumed local-part matching would contradict derive_username's deliberate domain-folding. The docstring answers it directly and is right — one refuses to merge two people into one handle, the other refuses to split one person across two accounts, and both resolve to reporting rather than guessing. Worth keeping that paragraph; it is the non-obvious part.

Residual 1 — the `users:` path has the same root cause and no guard

Filed as #119. Briefly: compute_diff (acl.py:798-806) resolves a users: key username-then-email and, on a miss, appends a notices line and continues. A key whose account changed address misses both, so it is skipped — while the roster path now warns and exits non-zero for the identical condition.

_accounts_sharing_a_local_part is a free function over (address, users) with no dependency on the creation plan, so consulting it here is cheap. Suggested shape in the issue: warn when it finds candidates, keep the notice when it does not, so only the rename case becomes fatal.

This matters because a users: entry is what pins the active role under union_roles: true. When the pin silently stops applying the account keeps whatever it last had, which is precisely the drift the pin exists to prevent. The live config still carries two pins, so the mechanism is not vestigial.

Residual 2 — the recommended remedy is not reachable from quiltx

The warning says "Set the existing account's email to this address if it is the same person," and README.md:464 / CHANGELOG.md:105 both describe the refusal, but no quiltx command performs that. The only route is quilt3.admin.users.set_email by hand, which is what unblocked the case above:

from quilt3 import admin
admin.users.set_email('robbyqbutler', 'robbyqbutler@pm.me')

Since the refusal is exit-code-fatal, every operator who trips it has to leave the tool to clear it. A snippet in the README next to the refusal, or a --set-user-email, would close the loop. Not a blocker for this PR.

Not blocking

Both residuals are follow-ups; nothing here argues against merging. The creation path is the one that sends mail, and it is now the guarded one.

drernie · 2026-09-05T04:26:20Z

Correction to my comment above: Residual 2 is already tracked as #117, which I had missed — it covers the set_email gap more completely than my note did, including two verified rename shapes the local-part heuristic does not catch (local part changed, and a full name change where the two records share nothing).

So the follow-up list against this PR is:

acl: declare a changed sso.email address so a rename can be reconciled #117 — explicit rename declaration; the durable fix, and it subsumes my Residual 2.
acl: users: keys give no rename hint when an account's email changes #119 — the users: path has no rename guard at all; small, and closes an asymmetry between the two paths rather than completing either.

Residual 1 stands as written. Everything else in my review above is unchanged: the guard in 0d290c1 does what it claims on the live case, and neither follow-up argues against merging.

The roster path refuses an address whose local part an existing account already uses; the users: path had the same root cause and none of the reporting. A key whose account moved misses both the username and the email index, so compute_diff dropped the entry with a nonfatal notice while the roster path exited non-zero for the identical condition. _accounts_sharing_a_local_part is a free function over (address, users), so the signal was already available here. Candidates now produce a warning naming them; no candidates keeps the notice, so a captured config carrying an entry for a deleted account still does not fail a clean run. The message offers rekeying as well as set_email, since a users: key can be rewritten and a roster address cannot. That alone was not enough. _run's early return for an unchanged config skipped the exit-code decision, so the warning printed and the run still exited 0 -- and that is precisely the #119 case: when a pin is the only thing naming someone, there is nothing else to apply. Verified by probe before and after. The early return is now `return 1 if diff.warnings else 0`, which also removes a broader inconsistency where one warning failed a run with work to do and passed one without. Notices remain outside the exit code everywhere. Docs: README gains the set_email snippet the refusal recommends but no quiltx command performs, pointing at #117 for the durable fix.

drernie · 2026-09-05T04:33:41Z

Thanks for running it against the live catalog — the ernest@quilt.bio case is a better demonstration than anything in the test suite, since it is a genuine two-candidate ambiguity rather than a constructed one.

Both residuals addressed in 8151fe7. 654 passed, 1 skipped; lint clean; CI green.

Residual 1 (#119) — fixed, and it needed a second change to actually work

Implemented your suggested shape: compute_diff consults _accounts_sharing_a_local_part on an unresolved users: key, warns naming the candidates via _quote_user_identities when it finds any, and keeps today's notice when it does not. So only the rename case is fatal, and a captured config carrying an entry for a since-deleted account still does not fail a run with nothing to fix.

One deviation from the issue text, in the message rather than the mechanism. The roster message says "set the existing account's email", because a roster is keyed by email and cannot be rekeyed. A users: key can be, so the message offers both routes — rekey the entry to the account's current username or email, or change the account's email. Worth the divergence: rekeying is usually the cheaper fix and it is the one available without leaving the tool.

The interesting part: routing to warnings did not change the exit code. _run returns early on not diff.has_changes(), before the exit code is computed. has_changes() counts users_to_update and friends, not warnings — and an unresolved key produces no update, so the config looks reconciled. Probed it directly:

has_changes : False
warnings    : 1
   W: Configured user 'robbyqbutler@pm.me' does not exist on the server, but its
      local part 'robbyqbutler' is already used by ...
>>> exit code: 0

Which is exactly the case your issue singles out — when the pin is the only thing naming the person, there is nothing else to apply, so that is the common path rather than a corner. The fix would have been inert without this.

The early return is now return 1 if diff.warnings else 0. That is slightly broader than #119 asked for, so calling it out explicitly: it also removes a pre-existing inconsistency where the same warning failed a run that had work to do and passed one that did not. Notices stay outside the exit code, on this path as on every other — the invariant is now uniform rather than dependent on whether the run happened to have work.

Both halves pinned, and both verified non-vacuous by mutation:

test_user_block_warns_when_a_key_looks_like_a_changed_address
test_acl_tool_reconciled_config_exits_one_on_a_warning
test_acl_tool_reconciled_config_exits_zero_on_a_notice — the other side, so a future edit cannot quietly make notices fatal

Residual 2 — README snippet added

You are right that the refusal recommended something the tool could not do, which is worse for a fatal error than for a warning. Added next to the refusal:

from quilt3 import admin

admin.users.set_email("robbyqbutler", "robbyqbutler@pm.me")

with a note that the first argument is the username the warning prints, that quiltx catalog acl with no config file lists usernames and emails, and a pointer to #117 for the declarative fix. Not a --set-user-email flag: #117 subsumes it, and adding a second mutating user command to this PR would widen it past the four issues it is meant to close.

On the docstring

Glad that paragraph earned its place. I very nearly cut it as over-explaining, and kept it precisely because I had talked myself into the same contradiction you did and wanted the resolution written down rather than rediscovered.

Closing #119.

This was referenced Sep 2, 2026

acl: reconciliation reports access reductions but never expansions #108

Open

acl: no per-role opt-out from config.default_policy #109

Open

drernie commented Sep 2, 2026

View reviewed changes

Comment thread quiltx/acl.py Outdated

Comment thread quiltx/acl.py

Comment thread quiltx/tools/catalog/acl.py

drernie mentioned this pull request Sep 5, 2026

acl: declare a changed sso.email address so a rename can be reconciled #117

Open

drernie requested a balanced review from Copilot September 5, 2026 02:41

Copilot started reviewing on behalf of drernie September 5, 2026 02:42 View session

Copilot AI reviewed Sep 5, 2026

View reviewed changes

Comment thread quiltx/acl.py Outdated

Comment thread CHANGELOG.md Outdated

Comment thread README.md Outdated

Comment thread quiltx/acl.py

Comment thread stack-acl.example.yaml Outdated

drernie mentioned this pull request Sep 5, 2026

acl: users: keys give no rename hint when an account's email changes #119

Closed

drernie merged commit 79b036f into main Sep 5, 2026
6 checks passed

drernie deleted the 260901-acl-issues branch September 5, 2026 04:39

Navigation Menu

0.22.0: four ACL fixes and features (#104, #105, #96, #106) - #107

0.22.0: four ACL fixes and features (#104, #105, #96, #106)#107
drernie merged 5 commits into
mainfrom
260901-acl-issues

drernie commented Sep 2, 2026 •

edited

Loading

Uh oh!

drernie left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drernie commented Sep 2, 2026

Uh oh!

drernie commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drernie commented Sep 5, 2026

Uh oh!

drernie commented Sep 5, 2026

Uh oh!

drernie commented Sep 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

drernie commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

#104 (bug) — users: keys resolve only by username

#105 (feature) — config.default_policy, a floor rather than a fallback

#96 (feature + fix) — per-bucket config.no_preflight, and loud failures

#106 (feature) — --create-and-email-users

Testing

Not addressed

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Context used (3)

Uh oh!

drernie left a comment

Choose a reason for hiding this comment

Code review — 0.22.0 ACL changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drernie commented Sep 2, 2026

Second commit: 92ba139

Review findings

#110 — a failed default policy blocked every managed role

#112 — no CLI-level abort test

Still open for a maintainer call: --create-and-email-users (#106)

Uh oh!

drernie commented Sep 4, 2026

Third commit: 0d290c1 — --create-and-email-users now creates accounts

The mapping

The bug this turned up

Doc correction

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

drernie commented Sep 5, 2026

The rename guard holds

Residual 1 — the users: path has the same root cause and no guard

Residual 2 — the recommended remedy is not reachable from quiltx

Not blocking

Uh oh!

drernie commented Sep 5, 2026

Uh oh!

drernie commented Sep 5, 2026

Residual 1 (#119) — fixed, and it needed a second change to actually work

Residual 2 — README snippet added

On the docstring

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

drernie commented Sep 2, 2026 •

edited

Loading

#104 (bug) — `users:` keys resolve only by username

#105 (feature) — `config.default_policy`, a floor rather than a fallback

#96 (feature + fix) — per-bucket `config.no_preflight`, and loud failures

#106 (feature) — `--create-and-email-users`

Second commit: `92ba139`

Still open for a maintainer call: `--create-and-email-users` (#106)

Third commit: `0d290c1` — `--create-and-email-users` now creates accounts

Residual 1 — the `users:` path has the same root cause and no guard