Re-review of Task 6's fix round 1 (plan amendment d6a2d054) found three
items in that diff plus one adjacent pre-existing issue that C1 made
reachable:
- Important (perf): permissionsFor() rebuilt the deny Set on every
entry in the granted set (O(grants x denies) allocations on a
per-request path). Hoisted to build the Set once. Measured
4000x4000: 665.92ms before, 3.90ms after.
- Important (contract accuracy): permissionsFor() only half-agrees
with decide() — a narrow deny under a broad grant (e.g. role editor's
"post:*" plus a deny on "post:delete") can't be represented in a flat
Set, so the set still contains "post:*" while decide() correctly
refuses "post:delete". Documented as NOT authoritative on the
AuthzResolver interface, and pinned with a regression test asserting
the divergence is deliberate.
- Minor: subject.id === "" was audited as subjectId: "" instead of
omitted, so consoleAuditSink printed a blank subject= rather than
subject=anonymous. Reused the same non-empty-string guard as the
decide() path.
- Important (adjacent, advanced.ts): owner() compared subject[key] to
resource[key] with Object.is without checking either side was
present, so two absent ids (Object.is(undefined, undefined) ===
true) satisfied ownership. Unreachable before this task, but C1 now
runs bound policies for anonymous/empty subjects, putting this on a
live path. Fixed to deny whenever either side is undefined or null.
Every fix's regression test was verified by reverting the fix and
confirming the test fails against the pre-fix code before restoring.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coordinator review of Task 6's resolution engine (plan amendment
86b3dc1e) found two critical and four important defects, all inherited
from the brief's original engine snippet:
- C1: anonymous callers on a public permission returned allow before
running bound policies, so the least-trusted caller got the weakest
evaluation. Policies now run for anonymous subjects too.
- C2: the policy verdict check was a truthiness test (`!verdict.allowed`),
so a policy returning `{allowed: "yes"}` granted access. Now requires
`verdict?.allowed === true` exactly, and no longer spreads the raw
verdict into the decision (which leaked arbitrary policy fields).
- I1: a binding naming a policy the catalog doesn't have was silently
`continue`d, granting whatever the policy was meant to guard. Now
denies with "Policy unavailable".
- I3: denies were checked by exact string equality, so a wildcard deny
(e.g. "post:*") was accepted and silently did nothing. Denies now go
through the same depth-aware wildcard matching as grants, via the new
exported `deniedBy()`.
- I2: `permissionsFor` now subtracts denied entries so it agrees with
`decide()` — needed for Task 7's UI gating to compose correctly.
- I4: non-string/empty `subject.id` (0, "", 123, {}) no longer silently
falls back to anonymous; it denies with "Invalid subject". `subject:
null` (no subject at all) remains genuinely anonymous.
Added six regression tests, each verified by reverting its fix and
confirming the test fails against the old code before restoring.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ClintchizandClaude Opus 5