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
There was a problem hiding this comment.
[Claude Code 🤖] This review checked whether the compiled grant expressions keep their identity from one request to the next on the admission, explain and actingUser paths, whether moving from runBxlTransform to a held program changes error phases or context slots, whether the new tests could pass without the cache, and the resolution in the integration base where policy was added to TransformProgramContext and program.run.
No blocking issues. The admission and actingUser paths reach the cached CompiledRealmPolicy's own grant objects. Explain holds one compile for each explain, so draft explains prepare once for each draft and release the programs with it. The base merge carries policy through, and bxl-mirror-check.ts checks the key sets in both directions, so a dropped slot would fail the typecheck.
- Non-blocking: the failing-expression half of the new unit test never checks that the first settlement failed (inline on
anonymous-access-test.ts).
|
|
||
| let failing = compiled('blocklist', 'realmConfig("blockedIps")'); | ||
| let beforeFailing = grantExpressionPreparations(); | ||
| await settleTraffic( |
There was a problem hiding this comment.
[Claude Code 🤖] Please assert on this first settlement. The result is discarded, so the assertion below, "an expression that throws on its values is still prepared once", never checks that the expression threw. If realmConfig("blockedIps") against {} ever produced a value instead of throwing, this half would pass while testing nothing about failed runs keeping their program. That is the case the comment on preparedExpressions and the description call out.
let unset = await settleTraffic(
{ blocklist: failing },
{ realmConfig: {} },
PLATFORM,
);
assert.true(blocklistCloses(unset), 'the unset blocklist fails');Asserting unset.blocklist.failed names the evaluate phase would also pin that the failure happens at run time rather than at preparation. Non-blocking: a test gap in this PR.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in 0645e61. The first settlement is now kept and asserted to have failed with BxlTransformError (evaluate), so the 'still prepared once' check covers a program that is held after it throws at run time.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…re-each-grants-bxl-expressions-once-not-on-every
…re-each-grants-bxl-expressions-once-not-on-every
lukemelia
left a comment
There was a problem hiding this comment.
What's the compilation performance hit? Are we OK incurring it on every instance of the realm-server tier?
Important
Stacked on #6622 and #6645 through the integration branch
cs-13772-base-6622-and-6645(mainwith both merged). Don't merge this while it is based there. Once both parents have merged, it gets retargeted tomain.In service of CS-13772, which makes an
OperationGrant's BXL expressions cheap to evaluate on every request. A grant'sactingUser,blocklist,rateLimitRequestsandrateLimitWindowSecondsran through the one-shotrunBxlTransform. So every evaluation parsed and profile-checked a source that can't change while the compiled policy is held. The blocklist and the rate limit are settled at admission for every request from a caller who isn't signed in, so this cost landed on the hottest anonymous path.evaluateGrantExpressionnow holds oneprepareBxlTransformprogram per compiled grant expression, in aWeakMapkeyed on theCompiledGrantExpression. This is the same arrangement the gate uses forwherepredicates. A program lives exactly as long as the compiled policy that holds it, and a recompiled policy prepares its own. An expression that fails to prepare is not held, and fails again on its next evaluation. One that throws on the values it reads, such as arealmConfig()key that is absent, is held: preparing it succeeded, and the next request's values may well be there.The counter,
grantExpressionPreparations(), counts for the whole process rather than per realm core. Admission settles traffic from the realm's anonymous admission environment, which has no operation core to count against. Tests read the counter as a delta.The integration base resolves the one conflict between the parents. #6622 passes
policy()to thewhereevaluation, and #6645 moves that evaluation onto the prepared program. SoTransformProgramContextgainspolicy, and the gate passes it toprogram.run.🤖 Generated with Claude Code