Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review covered the anonymous eligibility check, readsActor detection, the anonymous summary, how anonymousAdmission() fails, the new tests, and the pairing declarations. It didn't check the claim that the opt-in admits nothing beyond the compile path.
Nothing blocks while the opt-in admits nothing. Two decisions should be settled before the gate reads this: whether an author's specialization of a base operation is eligible, and how a where that reads actor() is treated for an anonymous caller.
- Decide whether a declared
update,createand so on, under its own name, is eligible. See the thread on the eligibility check inpolicy.ts. - Keep grants whose
wherereads the caller out ofanonymous.operations, warn the author, and correct the "never holds" wording. See theanonymousSummarythread. - Record
actingUserKeyonly for writes. - Log the fault
anonymousAdmission()swallows. - Add the missing negative-space tests. See the compile test thread.
Validate catalog test subset is red with "waiting on … to merge" for the paired catalog pull request. That's expected until it merges; re-run it afterwards.
| if (grant?.anonymous === true) { | ||
| if ( | ||
| granted.base !== operation || | ||
| !ANONYMOUS_ELIGIBLE_OPERATIONS.includes(granted.base) | ||
| ) { |
There was a problem hiding this comment.
[Claude Code 🤖] Are an author's own declarations under a base operation's name meant to be eligible? granted.base !== operation lets them through. A type can write @operation static update = { base: 'update', … } (and the same for create, read, delete and the rest; the only reserved names are readSource and query). grantedOperation returns that declaration with base: 'update', so the check passes. But a declaration like that is the "contract its author wrote for signed-in callers" that the issue message refuses, and its clauses can read actor() (the declaration's lowered OperationDefinition.readsActor already says so).
- If they shouldn't be eligible, refuse when
ownOperation(definition, operation)is set. If a specialization that doesn't read the caller is fine, refuse only when itsreadsActoris set. Add a fixture type that redeclaresupdateto the compile test either way. - If they should be eligible, the issue message and the
ANONYMOUS_ELIGIBLE_OPERATIONScomment ("anything else declared under its own name") should say so.
This doesn't block while the opt-in admits nothing, but it needs a decision before the gate reads it.
There was a problem hiding this comment.
[Claude Code 🤖] Kept eligible, and the wording now says so (f87ad86). The rule is the one the decision states: the operation's name is checked against the base operations. A type's own declaration under a base operation's name, its update for example, is that base operation. The issue code's comment and ANONYMOUS_ELIGIBLE_OPERATIONS now say a custom operation or a named query is refused, and a base-named declaration is eligible. A declaration whose clauses read actor() refuses at invocation for a caller with no actor ("No actor in scope"), so it fails closed rather than widening anything.
| let operations = new Set<string>(); | ||
| for (let rule of rules) { | ||
| for (let grant of rule.grants) { | ||
| if (grant.anonymous && (grant.operation !== 'query' || grant.filter)) { |
There was a problem hiding this comment.
[Claude Code 🤖] A grant whose where reads actor() still puts its operation in anonymous.operations, even though the gate will skip that predicate for an anonymous caller. Take a policy whose only anonymous read grant is .public == true or .owner == actor(). It advertises read as open, but it can never admit an anonymous caller.
The comment on CompiledPolicyPredicate.readsActor (and the catalog field doc) says such a predicate "never holds" for an anonymous caller, but that isn't true. or short-circuits, so with no actor in the context, runBxlTransform answers true for { public: true } and throws only for { public: false }. Skipping the predicate is a rule the realm chooses, not a consequence, and the author whose public cards stay closed gets no signal.
Suggest:
- Leave
grant.where?.readsActorgrants out here, as the filterless query grant already is. - Record a
warning-severity issue (inKEEPS_ITS_PART, since the grant stays live for signed-in callers) on an anonymous grant whosewherereads the caller. - Reword the
readsActorcomment as the rule: "is not evaluated for one, so it never admits one".
Non-blocking, but cheaper to settle before the gate consumes the summary.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86. A grant whose where reads actor() is left out of anonymous.operations, and lowering records a new anonymous-grant-reads-actor warning on its where. It's in KEEPS_ITS_PART, so the grant stays live for signed-in callers. The readsActor comment now states the rule: the realm doesn't evaluate such a predicate for a caller who isn't signed in, so it never admits one. The test gives that grant a rule of its own and checks that its operation drops out of the summary.
| ); | ||
| continue; | ||
| } | ||
| anonymous = actingUserKey ? { actingUserKey } : {}; |
There was a problem hiding this comment.
[Claude Code 🤖] An actingUser on an anonymous read grant is quietly kept as actingUserKey, but the CompiledOperationGrant.anonymous comment says only a write grant names one. If the gate later reads actingUserKey without checking isWrite again, an anonymous read starts running as that user and sees what they can see. Suggest anonymous = isWrite(granted.base) && actingUserKey ? { actingUserKey } : {}, or a warning that the key is ignored on a read. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86. Only a write records actingUserKey. An actingUser on an anonymous read is ignored, and the test pins it on a readSource grant.
| try { | ||
| compiled = await this.get(); | ||
| } catch { | ||
| return NO_OPERATIONS; | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] Refusing on a fault is right, but this bare catch drops the fault without a trace. A broken policy read then shows up only as anonymous callers being turned away, and nothing anyone can find says why. Please log it through this module's realm:policy logger, with no card content. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86. anonymousAdmission() still answers with nothing on a fault, and now logs the fault's message on realm:policy. No card content goes in the log.
| assert.notOk(policy?.uncompilable, 'the policy as a whole compiled'); | ||
| }); | ||
|
|
||
| test('a grant opens a base operation to callers who are not signed in, and a write names who it is made as', async function (assert) { |
There was a problem hiding this comment.
[Claude Code 🤖] Three branches this change adds have no test that would fail if they broke:
- A named query with
anonymous. The issue message names it, but the fixture has no saved search, so only the custom-transform half ofanonymous-not-base-operationis pinned. - A query grant left with no filter (
policy-not-filterable) stays out ofanonymous.operations. The only anonymous query here has a filter, so deleting|| grant.filterfromanonymousSummarykeeps this test green. - An uncompilable policy gives an empty
getAnonymousAdmission(). This is the fail-closed path the doc comment promises.
Also, the actor-reading read grant sits beside an unconditional anonymous read, so the summary assertion can't tell how a caller-reading grant is treated. Whatever is decided in the anonymousSummary thread, give that grant a rule of its own here. Non-blocking.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86. The module's Classroom declares a named query, listOpen, which is refused with anonymous-not-base-operation. A query grant that compiles no filter (realmConfig(...)) gives no anonymous summary and an empty admission. A policy card that isn't a RealmPolicy is uncompilable and gives an empty admission. The grant that reads the caller is the only grant of its own rule.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 973b4cb95e
ℹ️ 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".
| let actingUserKey = | ||
| typeof grant.actingUser === 'string' && grant.actingUser.trim() | ||
| ? grant.actingUser.trim() | ||
| : undefined; |
There was a problem hiding this comment.
When actingUser names a setting whose key has leading or trailing whitespace, this stores the trimmed name even though realm configuration deliberately preserves keys exactly (" writer " and "writer" are distinct). The anonymous write will therefore resolve the wrong setting, potentially running as the user under the trimmed key instead of the one the policy named; use trimming only to validate non-blank input and retain the original string as actingUserKey.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86. The key is kept exactly as written, so the key the grant view shows is the key the realm looks up. A blank one names nothing, and a write naming nothing is anonymous-write-without-acting-user.
| if (grant.anonymous && (grant.operation !== 'query' || grant.filter)) { | ||
| operations.add(grant.operation); |
There was a problem hiding this comment.
When an operation's only anonymous grant has a predicate containing actor(), the compiler correctly marks that predicate readsActor, but this summary still advertises the operation through getAnonymousAdmission(). Since an unsigned caller has no actor and the new contract says such a predicate never holds for one, the returned set incorrectly claims that the policy opens the operation; skip grants whose where?.readsActor is set when building this summary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in f87ad86, the same change as the summary thread above: grants whose where reads actor() no longer count toward anonymous.operations, and they carry an anonymous-grant-reads-actor warning.
A grant whose `where` reads `actor()` opens nothing to anonymous callers and is warned about; only a write records an acting-user key, used exactly as written; a fault reading the policy for an anonymous caller is logged. Tests cover a named query, a query grant with no filter, an uncompilable policy, and the grant view; the subset pin follows the catalog PR head. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 1h 22m 53s ⏱️ + 1h 15m 32s Results for commit d737d7b. ± Comparison against earlier commit c215d1e. Realm Server Test Results 1 files ±0 314 suites +1 2h 2m 9s ⏱️ + 4m 1s Results for commit d737d7b. ± Comparison against earlier commit c215d1e. |
…rged Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
richardhjtan
left a comment
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] No blocking findings. Merge order: boxel-catalog#802 → re-run the subset check → this.
|
[Claude Code 🤖] Thanks for the review. That's the merge order, with one gate ahead of it: the host test "realm config policy standing: a policy created from the card chooser…" flakes when this catalog definition is in place. Its fix, #6546, merges first. Then main comes into this branch, boxel-catalog#802 merges, the subset check is re-run here, and this merges. |
|
[Claude Code 🤖] Thanks. With two approvals and all host shards green after the race fix, this waits only on the merge order: #6538, then boxel-catalog#802, then this. |
# Conflicts: # packages/catalog/test-subset.json
3 participants
Important
Merge order (each must merge before the next):
Each "not opened yet" line gets its PR URL as soon as that PR opens.
Merges after: cardstack/boxel-catalog#802
In service of letting a realm's policy admit callers who aren't signed in: this compiles a grant's opt-in. Nothing admits an anonymous caller on it yet; the realm starts doing so in a later change in the stack.
The catalog's
OperationGrantgainsanonymousandactingUser(paired catalog PR above), and lowering carries them onto the compiled policy:CompiledOperationGrant.anonymousis set on a grant that opts in, withactingUserKeyfor a write: the key in the governed realm'srealm.jsonsettings whose value is the user the write is made as. The policy names a setting, never a user, so the governed realm decides whose identity its writes carry.CompiledRealmPolicy.anonymous.operationslists the operations any live grant opens to anonymous callers, and is absent when none does.Realm#getAnonymousAdmission()reads it through the policy cache, which is all the realm will need to answer an anonymous request it has no grant for, without loading a target or evaluating a predicate. A policy that won't compile opens nothing, so such a caller gets the answer every unauthenticated request gets rather than a 500 that would reveal the realm names a policy.CompiledPolicyPredicate.readsActormarks awherethat callsactor(). The realm doesn't evaluate such a predicate for a caller with no actor, so it never admits one; the gate skips it rather than evaluate it into a fault. A grant like that opens nothing to anonymous callers in the summary, and lowering warns about it (anonymous-grant-reads-actor, kept live for signed-in callers).Two new policy issues make a grant inactive:
anonymous-not-base-operation: the opt-in sits on anything but a grantable base operation invoked under its own name (a type's own declaration under a base name counts as that base operation) (read,readSource,query,create,update,delete,transform,appendContainsMany,appendLine). A custom operation or a named query is a contract its author wrote for signed-in callers.anonymous-write-without-acting-user: an anonymous write names no acting-user key. The key is used exactly as written, and only a write records one.A query grant with no search filter admits no search, so it opens nothing to anonymous callers either.
The manifest pins the catalog PR's head. The compile tests pass locally against a stack serving that catalog branch, and CI runs the rest of the subset's consumers.
🤖 Generated with Claude Code