feat(validation): allow dots in change names as valid separators#842
Open
gqcn wants to merge 1 commit intoFission-AI:mainfrom
Open
feat(validation): allow dots in change names as valid separators#842gqcn wants to merge 1 commit intoFission-AI:mainfrom
gqcn wants to merge 1 commit intoFission-AI:mainfrom
Conversation
Dots are now permitted alongside hyphens in change names (e.g., `v1.2`, `add-auth.v2`). Path traversal is still prevented: consecutive dots, leading dots, and trailing dots are all rejected by the regex.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Contributor
📝 WalkthroughWalkthroughUpdates validation logic in change name validation to accept dots alongside hyphens as separators, expanding the kebab-case pattern from hyphen-only to hyphen-or-dot separators. Adds explicit validation to reject consecutive dots with corresponding error messaging. No public API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/utils/change-utils.ts (1)
48-91: Validation logic looks correct and security is preserved.The updated regex
/^[a-z][a-z0-9]*([-.][a-z0-9]+)*$/properly prevents path traversal:
- Names must start with a lowercase letter (no leading dots)
- Separators must be followed by at least one alphanumeric character (no trailing dots, no
..)- The explicit consecutive-dot check at lines 77-79 provides a clear error message
Consider adding explicit checks for leading/trailing dots for clearer error messages.
Currently, names like
.hiddenorname.fail the regex silently and fall through to the generic fallback at line 87. Adding explicit checks (similar to lines 68-72 for hyphens) would provide more actionable feedback.💡 Optional: Add explicit dot boundary checks for clearer errors
if (/--/.test(name)) { return { valid: false, error: 'Change name cannot contain consecutive hyphens' }; } if (/\.\./.test(name)) { return { valid: false, error: 'Change name cannot contain consecutive dots' }; } + if (name.startsWith('.')) { + return { valid: false, error: 'Change name cannot start with a dot' }; + } + if (name.endsWith('.')) { + return { valid: false, error: 'Change name cannot end with a dot' }; + } if (/[^a-z0-9.\-]/.test(name)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/change-utils.ts` around lines 48 - 91, The regex in validateChangeName and kebabCasePattern already blocks leading/trailing dots but falls back to the generic error; add explicit checks for names starting or ending with a dot (e.g., name.startsWith('.') and name.endsWith('.')) alongside the existing hyphen checks so you return clear errors like 'Change name cannot start with a dot' or 'Change name cannot end with a dot' before the final generic message; update the branching inside validateChangeName (the block after !kebabCasePattern.test(name)) to include these two dot-boundary tests.test/utils/change-utils.test.ts (1)
117-123: Consider adding edge case tests for dot boundary conditions.The consecutive dots test is good. For more comprehensive coverage, consider adding tests for:
- Leading dot (
.hidden) - should be rejected- Trailing dot (
name.) - should be rejected- Mixed consecutive separators (
add.-auth,add-.auth) - should be rejected- Multiple consecutive dots (
a...b) - should be rejectedThese would help ensure the regex handles all boundary conditions correctly.
💡 Optional: Additional edge case tests
describe('invalid names - consecutive dots rejected', () => { it('should reject name with double dots', () => { const result = validateChangeName('add..auth'); expect(result.valid).toBe(false); expect(result.error).toContain('consecutive dots'); }); + + it('should reject name with leading dot', () => { + const result = validateChangeName('.hidden'); + expect(result.valid).toBe(false); + }); + + it('should reject name with trailing dot', () => { + const result = validateChangeName('feature.'); + expect(result.valid).toBe(false); + }); + + it('should reject name with mixed consecutive separators', () => { + const result = validateChangeName('add.-auth'); + expect(result.valid).toBe(false); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/utils/change-utils.test.ts` around lines 117 - 123, Add additional unit cases to the existing "invalid names - consecutive dots rejected" suite to cover dot boundary edge cases by calling validateChangeName with leading dot ('.hidden'), trailing dot ('name.'), mixed consecutive separators ('add.-auth' and 'add-.auth'), and multiple consecutive dots ('a...b') and asserting result.valid is false and result.error contains an appropriate message (e.g., 'invalid name' or specific 'consecutive'/'boundary' text); update the describe block around validateChangeName to include these new it() tests so the regex handling for validateChangeName is verified against leading/trailing dots, mixed separator sequences, and multiple consecutive dots.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/utils/change-utils.ts`:
- Around line 48-91: The regex in validateChangeName and kebabCasePattern
already blocks leading/trailing dots but falls back to the generic error; add
explicit checks for names starting or ending with a dot (e.g.,
name.startsWith('.') and name.endsWith('.')) alongside the existing hyphen
checks so you return clear errors like 'Change name cannot start with a dot' or
'Change name cannot end with a dot' before the final generic message; update the
branching inside validateChangeName (the block after
!kebabCasePattern.test(name)) to include these two dot-boundary tests.
In `@test/utils/change-utils.test.ts`:
- Around line 117-123: Add additional unit cases to the existing "invalid names
- consecutive dots rejected" suite to cover dot boundary edge cases by calling
validateChangeName with leading dot ('.hidden'), trailing dot ('name.'), mixed
consecutive separators ('add.-auth' and 'add-.auth'), and multiple consecutive
dots ('a...b') and asserting result.valid is false and result.error contains an
appropriate message (e.g., 'invalid name' or specific 'consecutive'/'boundary'
text); update the describe block around validateChangeName to include these new
it() tests so the regex handling for validateChangeName is verified against
leading/trailing dots, mixed separator sequences, and multiple consecutive dots.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb31f239-df47-41ba-a8f3-2b63c9d297a8
📒 Files selected for processing (2)
src/utils/change-utils.tstest/utils/change-utils.test.ts
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
.) as valid separators in change names alongside hyphens, enabling names likev1.2.0,add-auth.v2,feature.sub-task..) to give clear feedbackMotivation
Dotted naming conventions are common in versioning and hierarchical naming (e.g.,
v1.2.0,api.auth). The current strict kebab-case validationrejects dots entirely, which limits expressiveness without a strong reason — path traversal can be prevented without banning dots altogether.
Changes
src/utils/change-utils.ts/^[a-z][a-z0-9]*(-[a-z0-9]+)*$/to/^[a-z][a-z0-9]*([-.][a-z0-9]+)*$/..) check with specific error messagetest/utils/change-utils.test.tsv1.2,add-auth.v2,feature.sub-taskadd..auth(consecutive dots)Security
Path traversal is still fully prevented by the regex design:
Test plan
Summary by CodeRabbit
New Features
Improvements