fix: correct error locations when type/schema lines have trailing comments#628
Open
SoulPancake wants to merge 11 commits into
Open
fix: correct error locations when type/schema lines have trailing comments#628SoulPancake wants to merge 11 commits into
SoulPancake wants to merge 11 commits into
Conversation
…ments The DSL validator resolved error line numbers with anchored regexes (`^type NAME$`, `^schema V$`) that failed to match lines carrying a trailing comment (e.g. `type page # module: ...`). A miss returned -1, which was then propagated as a negative `skipIndex` into the relation lookup, producing nonsensical negative lines (-1, -2) and, for the invalid-type path, a crash (`rawLine.split` on `undefined`). Make the type/schema lookups comment-tolerant and guard against negative `skipIndex` propagation in both the JS validator and its module transformer copy, and mirror the fix in the Java validator. Add 7 shared regression cases (simple + the full reported model) covering missing-definition, reserved type/relation names, invalid type, no entrypoint, and invalid schema with trailing comments.
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughLine-number lookup helpers in Java and JavaScript are updated to clamp/normalize negative skip indexes and return safe failure values, and to match type/schema lines followed by trailing comments. Error builders add bounds/negative-index guards. New YAML test cases and a module test filter validate these behaviors. ChangesLine number and validation error fixes
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
SoulPancake
commented
Jul 6, 2026
SoulPancake
left a comment
SoulPancake
left a comment
Member
Author
There was a problem hiding this comment.
TODO: After Go semantic validation is generally done, we can run these fixtures and see if it needs the same fix too
SoulPancake
changed the title
Line-number lookups return -1 when they cannot locate a line. The JS and Java error builders dereferenced lines[lineIndex] without checking for a miss, which could crash (e.g. custom word resolvers calling methods on undefined) or attach negative line locations to errors. Treat a negative lineIndex like a missing one and emit the error without line/column information instead.
Member
Author
|
Tested it in the VSCode extension 
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes inaccurate (and sometimes negative) error line/column reporting in the DSL semantic validator when type/schema lines include trailing # ... comments, and mirrors the fix across the JS validator, the module-transformer copy, and the Java validator. It also adds shared regression cases to ensure error locations remain correct for these commented lines.
Changes:
- Make JS type/schema line lookup comment-tolerant and prevent negative
skipIndexpropagation from producing bogus locations or crashes. - Mirror the negative-index / bounds guards in the Java validator’s line lookup and error property building.
- Add 7 shared semantic-validation regression cases covering trailing-comment scenarios across multiple error types.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/data/dsl-semantic-validation-cases.yaml | Adds regression cases ensuring correct error locations when trailing comments exist on type/schema lines. |
| pkg/js/validator/validate-dsl.ts | Makes type/relation/schema line lookups comment-tolerant and guards against negative skipIndex. |
| pkg/js/util/line-numbers.ts | Mirrors JS line-lookup fixes in the shared utility used by the module transformer. |
| pkg/js/util/exceptions.ts | Guards against negative line indices when computing error properties/columns. |
| pkg/js/tests/modules/module-to-model.test.ts | Skips semantic DSL validation cases in module tests when schema is not 1.1 (since rewrite only handles that). |
| pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java | Prevents crashes by ensuring lineIndex is within bounds before reading lines[lineIndex]. |
| pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java | Prevents negative skipIndex and makes type line lookup tolerate trailing comments. |
Comments suppressed due to low confidence (1)
pkg/js/util/exceptions.ts:444
constructValidationErroruses a RegExp built frommetadata.symbol(and\bword boundaries) to locate the symbol column. Symbols in this DSL can contain non-word characters (e.g..or-), which makes\bunreliable and also risks regex metacharacters changing the match. Using a literalindexOfavoids both issues and keeps error columns stable for extended identifiers.
if (lines?.length && lineIndex != undefined && lineIndex >= 0) {
const rawLine = lines[lineIndex];
const re = new RegExp("\\b" + metadata.symbol + "\\b");
let wordIdx = rawLine?.search(re);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java (1)
99-100: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
raiseAssignableRelationMustHaveTypesbypasses the bounds check and can crash on negativelineIndex.Line 100 directly accesses
lines[lineIndex]without the bounds guard added tobuildErrorPropertieson line 28. If a upstream lookup returns-1(the exact scenario this PR hardens against), this will throwArrayIndexOutOfBoundsException.🔒 Proposed fix
public void raiseAssignableRelationMustHaveTypes(int lineIndex, String symbol) { - var rawLine = lines[lineIndex]; + var rawLine = (lines != null && lineIndex >= 0 && lineIndex < lines.length) ? lines[lineIndex] : ""; var actualValue = rawLine.contains("[") ? rawLine.substring(rawLine.indexOf('['), rawLine.lastIndexOf(']') + 1) : "self";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java` around lines 99 - 100, The `raiseAssignableRelationMustHaveTypes` method still reads `lines[lineIndex]` directly, so it can throw on a negative or out-of-range index. Update this method in `ValidationErrorsBuilder` to use the same bounds-safe handling as `buildErrorProperties`, checking `lineIndex` before accessing `lines` and falling back to an empty/placeholder line when the index is invalid.
🧹 Nitpick comments (2)
pkg/js/util/line-numbers.ts (1)
24-24: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider escaping
typeNamein the regex pattern.
typeNameis interpolated directly into the regex string. If it contains regex metacharacters (e.g.,.,*,+), the pattern will match incorrectly. This is the same concern flagged in the JavaDsl.javacounterpart. Since the pattern was modified in this PR, it's a good opportunity to escape it.🛡️ Proposed fix
- .findIndex((line: string) => line.trim().match(`^${extension ? "extend " : ""}type ${typeName}\\s*(#.*)?$`)); + .findIndex((line: string) => line.trim().match(`^${extension ? "extend " : ""}type ${typeName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\s*(#.*)?$`));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/js/util/line-numbers.ts` at line 24, The regex in the line lookup logic currently interpolates typeName directly, so special characters can change the match behavior. Update the findIndex pattern in the line-number utility to escape typeName before building the regex, keeping the existing extension prefix and optional comment handling intact. Use the line-number helper function where this pattern is built to apply the escaping consistently.pkg/js/validator/validate-dsl.ts (1)
307-317: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider escaping
typeNamein the regex to prevent false matches.
typeNameis interpolated directly into the regex string. Type names can contain.(per naming rule^[^:#@\*\s]{1,254}$), which acts as a regex wildcard. For example,type a.bwould also matchtype aXb. This is pre-existing, but since the regex is being modified here, it's a good opportunity to fix it.♻️ Proposed fix using a literal string match instead of regex
- const index = lines.slice(skipIndex).findIndex((line: string) => line.trim().match(`^type ${typeName}\\s*(#.*)?$`)); + const index = lines.slice(skipIndex).findIndex((line: string) => { + const trimmed = line.trim(); + return trimmed.startsWith(`type ${typeName}`) && ( + trimmed.length === `type ${typeName}`.length || + /^\s*(#.*)?$/.test(trimmed.slice(`type ${typeName}`.length)) + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/js/validator/validate-dsl.ts` around lines 307 - 317, The regex in getTypeLineNumber is interpolating typeName directly, so names containing regex metacharacters like . can match the wrong type line. Update the matching logic in getTypeLineNumber to treat typeName as a literal when comparing against the line contents, using a safely escaped pattern or a non-regex literal string check. Keep the existing skipIndex and trailing-comment behavior intact while fixing the matching for all valid type names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/java/src/main/java/dev/openfga/language/validation/Dsl.java`:
- Around line 54-55: Escape the interpolated type name in Dsl.getTypeLineNumber
before building the regex, since the current line-level match in findLine treats
typeName as raw regex input. Update the pattern to use a literal-safe form such
as Pattern.quote(typeName) so names passed from ModelValidator
reserved/invalid-name paths cannot cause incorrect matches or regex backtracking
issues. Keep the optional trailing comment handling in the same match logic.
---
Outside diff comments:
In
`@pkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.java`:
- Around line 99-100: The `raiseAssignableRelationMustHaveTypes` method still
reads `lines[lineIndex]` directly, so it can throw on a negative or out-of-range
index. Update this method in `ValidationErrorsBuilder` to use the same
bounds-safe handling as `buildErrorProperties`, checking `lineIndex` before
accessing `lines` and falling back to an empty/placeholder line when the index
is invalid.
---
Nitpick comments:
In `@pkg/js/util/line-numbers.ts`:
- Line 24: The regex in the line lookup logic currently interpolates typeName
directly, so special characters can change the match behavior. Update the
findIndex pattern in the line-number utility to escape typeName before building
the regex, keeping the existing extension prefix and optional comment handling
intact. Use the line-number helper function where this pattern is built to apply
the escaping consistently.
In `@pkg/js/validator/validate-dsl.ts`:
- Around line 307-317: The regex in getTypeLineNumber is interpolating typeName
directly, so names containing regex metacharacters like . can match the wrong
type line. Update the matching logic in getTypeLineNumber to treat typeName as a
literal when comparing against the line contents, using a safely escaped pattern
or a non-regex literal string check. Keep the existing skipIndex and
trailing-comment behavior intact while fixing the matching for all valid type
names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb2ac57d-cd79-41af-9320-dc10fff02abe
📒 Files selected for processing (7)
pkg/java/src/main/java/dev/openfga/language/validation/Dsl.javapkg/java/src/main/java/dev/openfga/language/validation/ValidationErrorsBuilder.javapkg/js/tests/modules/module-to-model.test.tspkg/js/util/exceptions.tspkg/js/util/line-numbers.tspkg/js/validator/validate-dsl.tstests/data/dsl-semantic-validation-cases.yaml
Type, relation and schema names were interpolated into regexes unescaped. Extended identifiers may contain regex metacharacters (e.g. '.'), which could match the wrong line and report incorrect error locations. Use literal prefix matching in JS and Pattern.quote in Java.
…schema lines
Line lookups used bare prefix matching, so a name that is a prefix of
another (e.g. relation 'writer' vs 'writers', condition 'less' vs
'less_than') could attach errors to the wrong line when the longer name
was defined first. Require the structural suffix that must follow each
name: ':' for relations, '(' for conditions (per grammar,
conditionName is always followed by LPAREN), and end-of-line or a
comment for schema versions.
…on names
Add shared fixtures where a relation ('writer' vs 'writers') and a
condition ('less' vs 'less_than') are prefixes of earlier definitions,
asserting errors point at the correct lines. Both cases fail against
the previous prefix-matching lookups.
…nMustHaveTypes The method read lines[lineIndex] without the bounds check used by buildErrorProperties, so a failed line lookup (-1) would throw ArrayIndexOutOfBoundsException. Mirror the JS implementation, which falls back to an empty value when the line cannot be resolved.
rhamzeh
reviewed
Jul 9, 2026
| const typePrefix = `type ${typeName}`; | ||
| const index = lines.slice(skipIndex).findIndex((line: string) => { | ||
| const trimmed = line.trim(); | ||
| return trimmed.startsWith(typePrefix) && /^\s*(#.*)?$/.test(trimmed.slice(typePrefix.length)); |
Member
There was a problem hiding this comment.
Suggested change
| return trimmed.startsWith(typePrefix) && /^\s*(#.*)?$/.test(trimmed.slice(typePrefix.length)); | |
| return trimmed.startsWith(typePrefix) && /^\s+(#.*)?$/.test(trimmed.slice(typePrefix.length)); |
require a space before the comment, otherwise you'll trigger on e.g. team#member
Member
Author
There was a problem hiding this comment.
Thanks!
I pushed a fix. \s+ would actually regress the common no-comment case (type user → empty remainder → no match), and the team#member example can't reach this matcher since it only tests type declration lines. But \s*(#.*)? did treat a glued # as a comment. Changed both the type and schema matchers to /^(\s+#.*)?$/, which requires a space before an inline comment while still allowing the empty case.
…schema lines A glued `#` (e.g. `type team#member`) was matched as a trailing comment by `\s*(#.*)?`. Require a preceding space via `(\s+#.*)?` so only real inline comments are treated as such, while still allowing the no-comment case.
The type/schema matchers exist in two places; mirror the `(\s+#.*)?` fix in util/line-numbers.ts (used by the modules transformer) so both paths treat a glued `#` as part of the token rather than a comment.
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
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.
The DSL validator resolved error line numbers with anchored regexes (
^type NAME$,^schema V$) that failed to match lines carrying a trailing comment (e.g.type page # module: ...). A miss returned -1, which was then propagated as a negativeskipIndexinto the relation lookup, producing nonsensical negative lines (-1, -2) and, for the invalid-type path, a crash (rawLine.splitonundefined).Make the type/schema lookups comment-tolerant and guard against negative
skipIndexpropagation in both the JS validator and its module transformer copy, and mirror the fix in the Java validator. Add 7 shared regression cases (simple + the full reported model) covering missing-definition, reserved type/relation names, invalid type, no entrypoint, and invalid schema with trailing comments.Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit