State the rule the guards enforce, site-level grants only, instead of
describing the asymmetry abstractly. The allow-list case is now covered
alongside negative permissions, which the previous wording omitted even
though the code treats them identically.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

ScopesCover checked the requested scope for org and user grants but not
the allowed scopes, whose User and ByOrgID permissions were discarded
unread. A scope granting workspace:* at site level while negating
workspace:delete for the user would have covered a request for
workspace:delete, because the negative that carves the action back out
lives in the half coverage never examined.

No catalog scope populates those fields today, so nothing was
miscompared in practice. The gap mattered because these guards exist to
keep the comparison fail-closed, and this one failed open.

Both sides now run the same checkCoverable helper, which refuses a scope
carrying org or user grants, a negative permission, or a resource allow
list. The helper names the side, so an error reports which half of the
comparison was undecidable. The doc comment claimed an unmodeled grant
on the allowed side is dropped; nothing is dropped now, so it is gone.

ScopesCover builds every Scope it reads from ExpandScope, which cannot
produce these shapes, so the guards are unreachable through the public
API. scopes_internal_test.go drives synthetic Scope values through
checkCoverable instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

permissionCovered skipped negative permissions, but checkCoverable now
refuses a scope carrying one on either side, so the branch was dead. It
was never defense in depth. Had a negative reached it, skipping the
anti-grant would leave any wildcard beside it free to match, and a scope
granting workspace:* while negating workspace:delete would report
workspace:delete as covered. The skip widened the ceiling while looking
like it narrowed it.

The precondition moves to the doc comment, which names checkCoverable as
what enforces it and says why subsumption cannot answer the question an
anti-grant poses.

No behavior change: the branch was unreachable. permissionCovered goes
from 88.9% to 100% statement coverage.

Five review findings on the coverage tests, all in scopes_test.go.

CanonicalScopeName had both alias arms at zero coverage. Its only caller
in the tests loops over ExternalScopeNames, which yields canonical names
only, so the canonicalizing call returned its input unchanged on every
iteration and read as coverage without being any. Swapping the arms, so
that `all` persisted application_connect and the reverse, kept the suite
green. TestCanonicalScopeName now pins the mapping and the loop appends
the aliases, taking the function from 50% to 100%.

The appended aliases raise branch coverage and assert a requestable name
is comparable once canonicalized, but they cannot detect a swapped
mapping, since both aliases resolve to scopes that cover themselves. The
comment says so rather than implying the loop guards more than it does.

CompositeDoesNotCoverNonMember and
CompositeDoesNotCoverWiderActionOnCoveredResource both asked for an
ungranted action on a resource coder:workspaces.access does grant, so
they tested one branch twice and left "resource not granted at all"
untested. They are now split along that line, with names that describe
which failure each one is.

The three wantErr rows shared a bare require.Error, so any error passed
any row and a bug failing every input on the requested side would have
left the allowed-side row green. wantErrContains replaces the bool and
names the side. Rewording the allowed-side message as the requested-side
one now fails three rows that previously all passed.

Alias rejection was tested for one alias on one side. Both aliases are
now tested on both sides. The allowed-side rows are the ones that earn
their place: they are what would catch someone canonicalizing inside the
allowed loop and widening the contract without a caller asking.

ScopesCover expanded and compared in a single pass, so the invariant
guards only ever ran on scopes ExpandScope had produced. Every such scope
satisfies them, which left the guards unverified in the position that
matters: the existing test called checkCoverable directly and could not
tell whether ScopesCover consulted it on both sides, or at all.

Split the comparison into scopesCoverExpanded, which takes already
expanded scopes paired with the names they came from. Tests drive
synthetic Scope values through it, so dropping the guard from either side
now fails, as does an allowed scope that grants every workspace action
except delete answering a request for delete.

Expanding every allowed scope before any guard runs reorders two error
paths against each other: a requested scope that fails a guard alongside
an unknown allowed name now reports the expansion failure rather than the
guard failure. Both return (false, error), and no ScopeName reaches that
combination today.

…ontract

The knowledge of which spellings are backward-compatibility aliases lived
in two switches, one in IsExternalScope and one in CanonicalScopeName,
kept in step by discipline. Drift between them is asymmetric: a name the
first accepts and the second does not rewrite is declared public and then
fails to expand on every request naming it. Both now read one table, so
they agree by construction, and an internal test walks that table
asserting each alias is public, resolves to a public name, and resolves
to one ExpandScope accepts. A third alias is covered the day it is added.

ScopesCover stated "names must be canonical" in prose only, which is
wrong for exactly the two inputs IsExternalScope accepts and ExpandScope
does not. The parameters are now canonicalAllowed and canonicalRequested,
so the requirement shows up in editor hints at every call site rather
than only in a doc comment the caller may not have opened.

Naming the parameters was chosen over canonicalizing inside ScopesCover.
The single downstream caller already canonicalizes both sides in bulk
before comparing, so absorbing the step would remove nothing from it
while dissolving the distinction between a public spelling and a stored
one at the layer that should hold it.

…roken

The site-only, wildcard-allow-list, no-negatives invariant was described
on ScopesCover and enforced by its guards, but ExpandScope, which is what
produces those values, had no doc comment at all. Someone adding a scope
reads ExpandScope and its neighbors; nothing there warned that populating
User or adding a negative makes the scope uncomparable. State it there,
along with the canonicalization requirement, and name the consequence
rather than just the rule.

Also note on ScopesCover that a wildcard request needs a wildcard grant.
Enumerating today's concrete actions genuinely is narrower than
`workspace:*`, so the rejection is intended. The
OneActionDoesNotCoverResourceWildcard row already pins the behavior; the
note stops the next reader of an authorize endpoint from taking it for a
bug and closing the gap.

Comments only. Checked that the documented invariant actually holds for
all three builtin scopes and all seven composites.

TestScopesCoverAllowedNegativeDoesNotWiden drove the same scope shape as
the NegativeUserPermission row of TestScopesCoverGuards, but asserted only
that some error came back. The row asserts the message, the side it names,
and that the comparison reports no coverage, and it runs the shape on both
sides rather than one. The weaker copy could pass on a regression that
returned the wrong error or stopped naming the side. Fold the scenario it
documented into the row's comment and drop the copy.

Rename the shared permission fixtures after the value they hold. The site
prefix read as "belongs in Role.Site", while two of the three are placed in
Role.User to build the shapes the guards refuse, and the No suffix gave no
hint that it means Negate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The allowed-side wrap printed the scope name and then wrapped an error that
prints it again, so the two sides of one comparison read differently:

  expand allowed scope "foo": no scope named "foo"
  expand requested scope: no scope named "foo"

Drop the redundant verb and let the inner error carry the name on both
sides.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The docstring said the list includes the `all` and `application_connect`
special scopes. It appends ScopeAll and ScopeApplicationConnect, which are
the `coder:` spellings, so the bare aliases are absent. Two callers already
compensate by appending them by hand, one of them with a comment stating
the mismatch. Describe what the function returns and name the helper that
bridges the gap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The invariant that expansion populates Site only was stated in full on
ExpandScope, checkCoverable and ScopesCover, and the "everything except
delete" example appeared on checkCoverable and again on permissionCovered
twenty lines below. State it once on ScopesCover, which is the function
whose behaviour depends on it, and cross-reference from the other two. Drop
framing that ranked implementation choices nobody proposed, and cut the two
test comments down to the facts the assertions do not already carry.

Kept in full: what each guard in checkCoverable defends, since no other
comment says it, and the wildcard rule on ScopesCover.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

The docstring listed the aliases and the low-level scopes, omitting the
curated composites the function also accepts. A caller consulting it to
decide whether coder:workspaces.access is public read no from the doc
and yes from the code.

ExternalScopeNames promises it offers each scope under one canonical
spelling, and no test held it to that. TestScopesCoverEveryExternalScope
appended the two aliases, but canonicalized them back into names the
list already carries, so it re-ran assertions the list iteration had
made and left the promise itself unpinned.

Assert on the alias table instead: the list omits the alias and offers
its canonical target. Every offered name is already proven coverable, so
the aliases inherit coverage, and a third alias inherits both invariants
the day it is added rather than needing a third hardcoded pair here.

The authorize endpoint parsed the scope parameter and discarded it, so an
app's configured allowlist never restricted anything and a client asking
for more than it should get was never told no. Phase 1 added the columns
that carry a negotiated scope from a code to the token it becomes, but
nothing wrote one, so every code was stamped unrestricted.

Negotiate the scope at authorization time and persist the result:

- Requested names must be in the external scope catalog, and the app's
  stored allowlist is filtered through that same catalog. Filtering only
  ever narrows what can be granted.
- The allowlist bounds authority, not spelling. A request is granted when
  every permission it grants is also granted by the allowlist, whether or
  not the allowlist names it, so an app allowed coder:workspaces.access
  can approve a client asking only for workspace:ssh.
- Omitting scope grants the filtered allowlist, per RFC 6749 section 3.3.
- Both handlers negotiate, so a request that cannot succeed fails before
  the consent page renders rather than after the user clicks Allow. Each
  reports the failure the way it already reports its own errors: a static
  error page on the GET side, an OAuth2 error body on the POST side.
- Two paths produce an empty result and are deliberately distinct. No
  allowlist and no request keeps the previous unrestricted grant, written
  as an explicit sentinel because the column is NOT NULL with a non-empty
  CHECK. An allowlist that filters to nothing is rejected, since falling
  back would grant strictly more than the allowlist ever permitted.

Dynamic client registration performs no catalog validation, so apps
registered with scopes such as openid or admin hold allowlists this
server cannot grant from. They now fail authorization in both directions.
Grandfathering unknown names would seed the enforcement path with values
it cannot evaluate, trading a visible negotiation-time error for a silent
enforcement-time hole. The failure names the registered scopes and the
remedy.

Issued tokens are still unrestricted: the exchange copies the negotiated
scope onto the token record, but the API key it mints carries no scope.
This changes which authorization requests succeed, not what a token can
do.

… sentinels

The black-box tests assert on the description that reaches the client, and
they did so through hand-copied fragments of the sentinel messages. A
reworded sentinel would leave every case asserting on text no branch
produces, and each case would still pass through whichever branch happened
to match next.

The sentinels live in package oauth2provider and the tests live in
oauth2provider_test, so they are bound through exported values declared in
the package's internal test file, which compiles into the same binary.

…turning it

rbac.ScopesCover reports an error when it cannot expand one of the names it
was handed. That is a deployment-side condition: the app's stored allowlist
holds something RBAC will not resolve, and no client can fix it by asking
differently. Folding it into errScopeNotAllowed both told the client it had
asked for too much, which is not what happened, and rendered RBAC internals
into error_description.

The failure now goes to the log with the app that provoked it, and the
client receives a sentinel of its own. negotiateScope takes the whole app
rather than its scope alone so the log line can name it.

… is grantable

The rejection named the filter's input, rejoined from fields. For a
whitespace-only allowlist that input is empty, so the app owner was shown
"" as the value they had to change: the one configuration where the message
is the only clue anything is set at all.

It now names the stored value verbatim.

…asons

Two reasons said things the code does not do.

"scope is not in this app's allowed scope list" described membership, but
the check is permission coverage: a scope the allowlist never names is
granted when a listed composite already confers it. A client reading the
old text would go looking for its scope in a list it was never matched
against.

"re-register the app with supported scopes" prescribed the one remedy a DCR
client has. An admin-created app is edited, not re-registered, and a DCR
client can update itself in place through RFC 7592.

The new text carries an apostrophe on the path the GET handler renders
through an HTML template, so the helper that reads those responses now
unescapes before matching.

The swagger annotation said a requested scope must be within the app's
configured allowlist, which is wrong twice over. The allowlist is checked by
permission coverage, not name membership, and it is not the only gate: every
requested name must also be in this deployment's scope catalog, including
for an app that has no allowlist at all. The omitted-scope default was
likewise stated only for apps that have one.

Two code comments went stale the same way. The branch table called the
omitted-scope default the whole allowlist when it is the catalog-filtered
one, and the comment over the persisted scope said the token minted from the
code will carry it, which is the next phase's work, not this one's.

… subtest

NoAllowlistStaysUnrestricted and NullAndEmptyAllowlistBehaveIdentically sent
the same request against the same NULL-allowlist app and asserted the same
persisted value. The second already covers the first, so the guarantee moves
into its comment rather than staying as a subtest that only restates it.

The comments on negotiateScope, its sentinels, and the scope tests
restated what the code and the case names already say. Keep the
non-obvious parts: the branch table, the alias-versus-enum reason for
canonicalization, the NULL/'' unification, the CHECK on the scope
column, and the xerrors wrap ordering the doubled-text assertion
guards. Drop RFC citations from test cases and shorten the swagger
scope description.

Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>