Repository navigation
Conversation
…d blocklist, each anonymous grant's acting user, and each grant's own issues Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…an-anonymous' into cs-13609-policy-explain-what-a-realm-does-with-a-caller-who-isnt
…eats callers who aren't signed in Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…an-anonymous' into cs-13609-policy-explain-what-a-realm-does-with-a-caller-who-isnt
…an-anonymous' into cs-13609-policy-explain-what-a-realm-does-with-a-caller-who-isnt # Conflicts: # packages/catalog/test-subset.json
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f57f3fb8f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (grant?.anonymous) { | ||
| let key = grant.anonymous.actingUserKey; |
There was a problem hiding this comment.
Report opt-in metadata for actor-reading operations
When an anonymous: true grant targets an operation whose declaration reads actor(), the compiler keeps the grant for signed-in callers and records anonymous-grant-reads-actor, but it does not set CompiledOperationGrant.anonymous. Consequently, this condition omits the new anonymous field even though ExplainedGrant documents it as present whenever the grant opts in. The explain panel therefore cannot identify the grant as an attempted anonymous grant or show its configured acting-user key except by interpreting the issue code; preserve the authored opt-in metadata for this retained-grant case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Leaving this as is. When the opened operation reads actor(), compile keeps the grant for signed-in callers only, deliberately without anonymous. With it, the realm's admission summary would open the operation and every visitor would get the gate's 401. The authored opt-in isn't lost to an author: the grant carries the anonymous-grant-reads-actor warning in ExplainedGrant.issues (asserted in realm-policy-anonymous-explain-test.ts), and the panel shows it as a warning on that grant. Reporting anonymous there would show it as open to visitors, which it isn't.
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Independent review of this PR
I reviewed this PR's own diff only, statically. I did not run any tests. The paired catalog PR was out of scope.
What I checked:
- Coverage of answer paths.
detaileris applied on every path: the card answer, the earlyactor-requiredreturn (it goes throughanswered, so a refused anonymous explain still carries the realm's limit), the search lane including itsaclearly return, each listing entry, and drafts. For a draft,governing.corekeeps the realm'sactingUser/anonymousAccess, becausedraftGovernedspreadsaccessand replaces onlycompiledPolicy. So the draft's own compile supplies both the grants and the per-grant issues. - Which core. Both detail and access are read from
governing.core, which is the target's realm (targetCore, or its draft-wrapped copy), not the policy card's realm. So the limit, blocklist and acting user are the governed realm's. That matches the rule that per-realm controls live in the governed realm'srealm.json. - Disclosure. An explain is answered only to an asker the target realm's ACL lets read. Those readers can already read
realm.json, includingconfig,anonymousRateLimitandanonymousBlocklist. The comment onRealmInfo.anonymousAccesssays so. The one new fact is whether a configured user can write (no-write), and an explain with that user asactoralready discloses theiracl. I found no new exposure. - Signed-in explains. These now also carry
anonymouson opted-in grants andissues. That costs one extracompiledPolicy()read and one acting-user resolution per distinct key per explain, which looks acceptable. Existing realm-server explain assertions I sampled use field-level orrules-onlydeepEquals on grants without compile warnings, so I don't expect them to break. CI is the real check. - Back-compat. All new fields are optional. The only other consumer of the types is the
base/operations.tsre-export. There is no JSON schema or exhaustive key check overExplainedGrantorPolicyExplanationin this repo. - Comments. New comments and test names are evergreen: no ticket IDs and no journey wording.
Inline findings, all minor or nit: the decision disagrees with invalidBlocklistEntries, the compile snapshot can skew, acting users are resolved separately from the gate, and the path-prefix guard has no test.
| anonymous: { | ||
| limit: { ...access.limit }, | ||
| limitFrom: access.limitFrom, | ||
| invalidBlocklistEntries: [...access.invalidBlocklistEntries], |
There was a problem hiding this comment.
[Claude Code 🤖] minor. The decision can contradict invalidBlocklistEntries. Scenario: realm.json has anonymousBlocklist: ['not an address'] and the policy opens read to callers who aren't signed in. An explain with actor: '' answers decision: 'allowed', reason: 'granted' and also lists the invalid entry. Every real request from such a caller is refused (blockReason: 'blocklist-invalid'), because the blocklist is enforced in anonymous admission, not in the gate the explain runs. The new type comment states that "the realm admits no caller who isn't signed in at all", so the answer contradicts itself, and the panel's headline decision is wrong for the person debugging. Suggested fix: when actor.kind !== 'user' and invalidBlocklistEntries.length > 0, answer denied with a dedicated reason and refusal, mirroring the 401/403 the request gets. Or, at minimum, document on PolicyExplanation.decision that it doesn't account for the blocklist, and assert the decision in the blocklist test so the chosen behavior is pinned.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 49d83d5b1d. For a caller who isn't signed in, an invalid blocklist entry turns the answer into decision denied, reason blocklist-invalid, refusal 401 actor-required (no admittedBy), matching what admission does to the request. It applies only where the answer turned on the policy. A realm whose ACL answers the request (acl), or whose policy opens nothing to such callers (actor-required), refuses or serves before reading the blocklist, so those answers stand. The blocklist test asserts the decision, reason, refusal and missing admittedBy, and that a signed-in reader is unaffected. The panel's text for the reason is in boxel-catalog#809.
| core: OperationCore, | ||
| actor: ScopeCaller, | ||
| ): Promise<(explanation: PolicyExplanation) => Promise<PolicyExplanation>> { | ||
| let compiled = await core.policy?.compiledPolicy(); |
There was a problem hiding this comment.
[Claude Code 🤖] minor. The compile snapshot can skew. detailer reads compiledPolicy() once, before any gate runs. explain() and the gate read it again for each card, and a listing runs one gate per card in sequence. The saved policy cache recompiles when the card or a type it names is indexed. If that happens mid-explain, rules[].grants[].path comes from the new compile while anonymous/issues are looked up by path in the old one. A reordered or inserted grant then silently gets another grant's acting user or warnings. That is likeliest on a long listing explain while the author is editing the policy. Suggested fix: pin one snapshot for the whole explain, the way draftGoverned does. Read it once, wrap governing.core with policy: { ...access, compiledPolicy: async () => snapshot }, and pass that core to both detailer and explain/explainSearch.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 49d83d5b1d. pinnedForExplain reads the compiled policy once per explain and wraps the governing core, as draftGoverned does. The gate, every listing entry and the detailer all decide from that one compile.
| let resolve = (key: string) => { | ||
| let resolution = resolutions.get(key); | ||
| if (!resolution) { | ||
| resolution = core.policy?.actingUser?.(key) ?? NO_ACTING_USER(); |
There was a problem hiding this comment.
[Claude Code 🤖] nit. The acting user is resolved separately from the gate's decision. Each explain() builds a scope through newOperationScope, which gets its own ActingUsers, so the gate resolves the key once per card and this map resolves it again. The two reads can disagree: if the user's write permission changes between them, the grant can be reported as actingUser: X while the decision says the gate refused it (no-write), or the reverse. The extra reads also add up across a listing. Suggested fix: create one ActingUsers per explain and pass it both to newOperationScope (ScopeInvocation.actingUsers) and to the detailer. Then every card's gate and the reported detail share one resolution per key.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 49d83d5b1d. The same wrapped core memoizes actingUser per key for the whole explain. Each card's gate scope resolves through it, and so does the detailer, so the reported acting user is the one the gate saw, with one lookup per key per explain.
| grantsOf(await explain(BOARD_POLICY, READER, NOTICE, 'update')).some( | ||
| ({ issues }) => issues, | ||
| ), | ||
| 'a grant with nothing recorded against it carries no issues', |
There was a problem hiding this comment.
[Claude Code 🤖] nit. No test covers the rules[1] vs rules[10] guard in detail. These assertions would still pass if the filter were a bare path.startsWith(explained.path), because no listed grant's path is a string prefix of another's (rules[0].grants[5] vs rules[0].grants[0..3]). The comment in detail calls out this case, but nothing pins it. Suggested fix: add an 11th grant (grants[10]) carrying a warning and assert it isn't attached to grants[1]. Alternatively, do the same with a rule at index 10.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 49d83d5b1d. The fixture puts a warned grant at rules[0].grants[10]. The update explanation lists rules[0].grants[1], and the test asserts that no listed grant carries issues, which a bare startsWith would fail.
…isitor on an invalid blocklist, and test the path-prefix guard Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ±0 1 suites ±0 1h 33m 18s ⏱️ - 17m 48s Results for commit 6b5f97b. ± Comparison against earlier commit 548ef91. Realm Server Test Results 1 files 319 suites 1h 50m 8s ⏱️ Results for commit 6b5f97b. |
…o isn't signed in Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…an-anonymous' into cs-13609-policy-explain-what-a-realm-does-with-a-caller-who-isnt
Important
Merge order (each must merge before the next):
Compile a policy grant's opt-in to callers who aren't signed in #6534: compile a grant's anonymous opt-in (merged)Policy: admit anonymous reads through a grant that opts in, rate-limited per realm and address #6568(CS-13536): anonymous reads (merged)4a. Policy: let a grant on a declared operation admit a caller who isn't signed in #6579 (CS-13605): anonymous named operations, stacked on Policy: admit anonymous writes as a user the realm's config names #6577. boxel-skills#174's §12 describes it
4b. This PR (CS-13609): explaining a visitor's refusal, stacked on Policy: let a grant on a declared operation admit a caller who isn't signed in #6579. boxel-skills#174's §10/§12 describe it
Add the judgment calls to the realm-policy-authoring skill boxel-skills#175(CS-13108): merged, and merged into boxel-skills#174. This section is §12, Refusals §13, its section §14 and the checklist §15.5a.
Say that no policy grant reaches a path the realm ignores boxel-skills#178(CS-13336): merged, and merged into boxel-skills#1747a. Say that a policy's rules are not secret from the writers of other realms boxel-skills#181 (CS-13398): a realm-policy-authoring §9 bullet, stacked on z-index handling for modals opening modals #179, merges right after it
Every PR in this order carries this callout. Whoever opens one of the PRs above tells the owners of the others, and each owner updates its own callout.
Catalog: this PR pins cardstack/boxel-catalog#809, the explain panel's visitor section, stacked on boxel-catalog#808. The order there is boxel-catalog#808, then #6579, then boxel-catalog#809, then this PR.
Merges after: cardstack/boxel-catalog#809
In service of CS-13521, which lets a realm's policy admit a caller who isn't signed in. An explain asked about such a caller (an empty actor) decided allowed or denied without saying why in the ways that are particular to them. A write grant whose acting-user key didn't resolve just answered denied. Nothing said what limit or blocklist the realm applies. And the policy's own warnings on a grant showed only for a draft. This PR makes explain say all three. The fields are optional and add no new reasons.
PolicyExplanation.anonymous, present when the actor is empty, including on an answer refusedactor-required. It has the target realm'slimit, whether that is its own setting or the platform default (limitFrom), and anyinvalidBlocklistEntries, which close the realm to every such caller. It's read through a new optionalOperationPolicyAccess.anonymousAccess(), which the realm wires togetAnonymousAccess(). So a target in another realm answers with its own settings, and a draft keeps them.ExplainedGrant.anonymous, on every listed grant that opts in, whoever the explain is about. A read gives{}. A write gives itsactingUserKeyand either theactingUserit resolves to or theactingUserFailure(key-missing,not-a-matrix-id,no-write). The key is resolved by the same resolver the gate uses at write time, once per key per explain.ExplainedGrant.issues: the compile issues recorded at that grant's path, from the same compiled policy (saved or draft) its rules came from. Paths match exactly or at a.boundary, sincerules[1]is a prefix ofrules[10]. A grant an inactive issue dropped isn't listed, so its issue stays only in the policy's own issues.denied, reasonblocklist-invalid, refusal 401actor-required. A realm whose ACL answers the request (acl), or whose policy opens nothing to such callers (actor-required), refuses or serves before reading the blocklist, so those answers stand.detailerinexplain.ts) over the card answer, the search answer and each listing entry. It reads the same compiled policy and acting users the gate decided with:pinnedForExplainholds one compile and one answer per acting-user key for the whole explain, so a listing's cards are all judged against one policy, and the acting user reported is the one the gate saw.The panel side is boxel-catalog#809, which this PR pins: a "Limit" row, a blocklist warning, each anonymous grant's acting user or why its key names no one, and each grant's issues.
The panel, asked about someone who isn't signed in. The realm's blocklist has an invalid entry, and three grants open
updateto such callers. The first resolves to a user who may write, the second names a setting the realm doesn't have, and the third's condition readsactor().Before, the answer was a bare "denied" with nothing about the visitor:
After, the panel gives the reason, the realm's limit, the blocklist warning, who each grant's writes are made as or why its setting names no one, and the warning on the grant that reads the caller:
Tests:
realm-policy-anonymous-explain-test.ts:{};rules[0].grants[10]thatrules[0].grants[1]'s path is a prefix of.Integration | realm policy: the panel's reason, limit row, blocklist warning, acting-user lines and a grant's warning. The host test helper'srealmConfigCardJSONgainsconfigandanonymousRateLimit.🤖 Generated with Claude Code