feat: Integrate expression validation into the definition issue framework#795
Merged
kingston merged 4 commits intoMar 6, 2026
Conversation
🦋 Changeset detectedLatest commit: 8cd9635 The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR integrates expression validation into the definition-issue pipeline by adding a collector for expression parser warnings, introducing an ExpressionValidationContext, and refactoring authorizer expression validators and related web UI code to consume pluginStore + definition. Expression parser warnings are surfaced as definition issues with warning severity. Changes
Sequence Diagram(s)sequenceDiagram
participant Collector as collectDefinitionIssues
participant ExprCollector as collectExpressionIssues
participant Schema as Schema (zod)
participant Parser as RefExpressionParser
participant Store as PluginSpecStore
participant Issues as DefinitionIssues
Collector->>ExprCollector: extract expressions from Schema + data
ExprCollector->>Parser: parse(expression.value)
Parser-->>ExprCollector: parseResult / throws
ExprCollector->>Parser: getWarnings(parseResult, ExpressionValidationContext{definition, pluginStore})
Parser-->>ExprCollector: [warnings]
ExprCollector->>Issues: map warnings -> DefinitionIssue(path, message, severity: warning)
ExprCollector-->>Collector: [DefinitionIssue...]
Collector->>Issues: append expression issues to final issues array
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.unit.test.ts (1)
23-29: Make the mock depend on the passeddefinition.The real
authConfigSpec.use()contract isgetAuthConfig(definition), but this stub ignoresdefinitionentirely. As written, these tests stay green even if the newpluginStore + definitionplumbing is broken, so the main refactor is not actually exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.unit.test.ts` around lines 23 - 29, createMockPluginStore returns a PluginSpecStore whose stubbed authConfigSpec.use() ignores the incoming definition; update createMockPluginStore so authConfigSpec.use() returns an object whose getAuthConfig takes the passed definition and uses it (e.g., selects roles based on definition or returns a structure derived from it) so tests exercise the real contract getAuthConfig(definition). Locate createMockPluginStore and the stub for authConfigSpec.use()/getAuthConfig and change getAuthConfig to accept the definition parameter and respond deterministically based on that parameter instead of closing over the outer roles only.packages/project-builder-lib/src/parser/collect-definition-issues.unit.test.ts (1)
257-287: Add one end-to-end case with the real authorizer parser.This proves the new wiring with a stubbed warning parser, but it still doesn't pin the user-facing path this PR is advertising. A single case using the real authorizer parser would catch drift if malformed expressions stop surfacing through
collectDefinitionIssues()as warnings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/project-builder-lib/src/parser/collect-definition-issues.unit.test.ts` around lines 257 - 287, Add an end-to-end test that uses the real authorizer expression parser instead of the stubbed WarningParser to ensure expression warnings surface through collectDefinitionIssues; update the test block (same describe 'collectDefinitionIssues') to create a schema via definitionSchema where the condition uses ctx.withExpression(realAuthorizerParser) (or the actual exported authorizer parser used in production), instantiate the parser context with createDefinitionSchemaParserContext({ plugins: pluginStore }), pass data that triggers a known authorizer warning, call collectDefinitionIssues(schema, data, pluginStore), and assert the returned issues include the expected warning with path ['condition'] and severity 'warning'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/project-builder-lib/src/parser/collect-definition-issues.ts`:
- Around line 96-98: collectExpressionIssues currently lets any parser.parse()
or parser.getWarnings() exception bubble up and abort collectDefinitionIssues;
update the expression collection to be fail-soft so a single throwing parser
produces a warning rather than throwing. Specifically, modify
collectExpressionIssues (or wrap the call in collectDefinitionIssues around
collectExpressionIssues) so that each parser invocation (parser.parse /
parser.getWarnings) is run in a try/catch, and on exception push a normalized
warning issue into the returned array (including parser name/expression context
and the error message) instead of rethrowing; ensure collectDefinitionIssues
still spreads the returned issues (issues.push(...expressionIssues)) unchanged.
In
`@packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.ts`:
- Around line 43-50: getRoleNames currently assumes
authConfig.getAuthConfig(definition as ProjectDefinition) returns an object with
a roles array and crashes when it is undefined; update getRoleNames to
defensively handle missing/undefined return values from authConfig.getAuthConfig
by checking the result before using roles (e.g., ensure you read the authConfig
result into a local variable, validate it or treat it as nullable, then build
the Set from roles?.map(role => role.name) ?? []), and also validate or guard
the unsafe cast of definition before passing it to authConfig.getAuthConfig so
plugin implementations that expect a parsed ProjectDefinition won't cause
runtime failures (refer to getRoleNames, pluginStore.use,
authConfig.getAuthConfig, ProjectDefinition, and the local roles variable).
---
Nitpick comments:
In
`@packages/project-builder-lib/src/parser/collect-definition-issues.unit.test.ts`:
- Around line 257-287: Add an end-to-end test that uses the real authorizer
expression parser instead of the stubbed WarningParser to ensure expression
warnings surface through collectDefinitionIssues; update the test block (same
describe 'collectDefinitionIssues') to create a schema via definitionSchema
where the condition uses ctx.withExpression(realAuthorizerParser) (or the actual
exported authorizer parser used in production), instantiate the parser context
with createDefinitionSchemaParserContext({ plugins: pluginStore }), pass data
that triggers a known authorizer warning, call collectDefinitionIssues(schema,
data, pluginStore), and assert the returned issues include the expected warning
with path ['condition'] and severity 'warning'.
In
`@packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.unit.test.ts`:
- Around line 23-29: createMockPluginStore returns a PluginSpecStore whose
stubbed authConfigSpec.use() ignores the incoming definition; update
createMockPluginStore so authConfigSpec.use() returns an object whose
getAuthConfig takes the passed definition and uses it (e.g., selects roles based
on definition or returns a structure derived from it) so tests exercise the real
contract getAuthConfig(definition). Locate createMockPluginStore and the stub
for authConfigSpec.use()/getAuthConfig and change getAuthConfig to accept the
definition parameter and respond deterministically based on that parameter
instead of closing over the outer roles only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67de19ae-182e-4d2c-823a-954c03ef31ba
📒 Files selected for processing (13)
.changeset/expression-validation-issues.mdpackages/project-builder-lib/src/parser/collect-definition-issues.tspackages/project-builder-lib/src/parser/collect-definition-issues.unit.test.tspackages/project-builder-lib/src/parser/collect-expression-issues.tspackages/project-builder-lib/src/parser/collect-expression-issues.unit.test.tspackages/project-builder-lib/src/parser/index.tspackages/project-builder-lib/src/references/expression-types.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-parser.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.tspackages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.unit.test.tspackages/project-builder-lib/src/testing/expression-warning-parser.test-helper.tspackages/project-builder-web/src/routes/data/models/edit.$key/-components/authorizer/authorizer-expression-linter.tspackages/project-builder-web/src/routes/data/models/edit.$key/-components/authorizer/model-authorizer-role-form.tsx
Comment on lines
+96
to
+98
| // Collect expression validation issues | ||
| const expressionIssues = collectExpressionIssues(schema, data, pluginStore); | ||
| issues.push(...expressionIssues); |
There was a problem hiding this comment.
Make expression collection fail-soft before calling it here.
collectExpressionIssues() currently invokes each parser’s parse() and getWarnings() directly. Wiring it into collectDefinitionIssues() means a single throwing expression parser now aborts the entire definition-issue pass instead of surfacing a warning for that expression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/project-builder-lib/src/parser/collect-definition-issues.ts` around
lines 96 - 98, collectExpressionIssues currently lets any parser.parse() or
parser.getWarnings() exception bubble up and abort collectDefinitionIssues;
update the expression collection to be fail-soft so a single throwing parser
produces a warning rather than throwing. Specifically, modify
collectExpressionIssues (or wrap the call in collectDefinitionIssues around
collectExpressionIssues) so that each parser invocation (parser.parse /
parser.getWarnings) is run in a try/catch, and on exception push a normalized
warning issue into the returned array (including parser name/expression context
and the error message) instead of rethrowing; ensure collectDefinitionIssues
still spreads the returned issues (issues.push(...expressionIssues)) unchanged.
Comment on lines
+43
to
+50
| function getRoleNames( | ||
| pluginStore: PluginSpecStore, | ||
| definition: unknown, | ||
| ): Set<string> { | ||
| const authConfig = pluginStore.use(authConfigSpec); | ||
|
|
||
| const roles = authConfig.getAuthConfig( | ||
| definition as ProjectDefinition, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== collectExpressionIssues.ts (first 150 lines) ==="
head -150 packages/project-builder-lib/src/parser/collect-expression-issues.ts
echo ""
echo "=== authorizer-expression-validator.ts (full file) ==="
cat packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.ts
echo ""
echo "=== All getAuthConfig implementations ==="
rg -n -B2 -A8 '\bgetAuthConfig\b' packages/project-builder-lib/src --type tsRepository: halfdomelabs/baseplate
Length of output: 11500
🏁 Script executed:
#!/bin/bash
# Check if there are concrete getAuthConfig implementations in plugin files
find packages -name "*.ts" -type f | xargs rg -l "getAuthConfig.*=" | head -20Repository: halfdomelabs/baseplate
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Look for auth config plugin implementations
rg -n "getAuthConfig\s*[:=]" packages --type ts -B5 -A10 | head -80Repository: halfdomelabs/baseplate
Length of output: 4223
Fix crash in getRoleNames() when getAuthConfig() returns undefined or missing roles.
The unsafe cast definition as ProjectDefinition is confirmed. More critically, getRoleNames() crashes with a TypeError if authConfig.getAuthConfig() returns undefined/null or an object without a roles property: the expression roles?.map((role) => role.name) returns undefined, and new Set(undefined) throws "undefined is not iterable."
Add a fallback to safely handle missing roles:
return new Set(roles?.map((role) => role.name) ?? []);Additionally, validate the cast or add a runtime check before passing the unparsed definition to getAuthConfig(), as plugin implementations may assume a fully parsed ProjectDefinition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/project-builder-lib/src/schema/models/authorizer/authorizer-expression-validator.ts`
around lines 43 - 50, getRoleNames currently assumes
authConfig.getAuthConfig(definition as ProjectDefinition) returns an object with
a roles array and crashes when it is undefined; update getRoleNames to
defensively handle missing/undefined return values from authConfig.getAuthConfig
by checking the result before using roles (e.g., ensure you read the authConfig
result into a local variable, validate it or treat it as nullable, then build
the Set from roles?.map(role => role.name) ?? []), and also validate or guard
the unsafe cast of definition before passing it to authConfig.getAuthConfig so
plugin implementations that expect a parsed ProjectDefinition won't cause
runtime failures (refer to getRoleNames, pluginStore.use,
authConfig.getAuthConfig, ProjectDefinition, and the local roles variable).
…idation-into-main-issue-framework
kingston
deleted the
kingston/eng-1066-incorporate-expression-validation-into-main-issue-framework
branch
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit