-
Notifications
You must be signed in to change notification settings - Fork 0
test: line comment integration testing #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ Strategic line-specific PR commenting for GitHub CLI (optimized for AI) | |
|
|
||
| `gh-comment` is the first GitHub CLI extension designed for comprehensive PR comment management. It provides a unified system for both general PR discussion and line-specific code review comments, filling a genuine gap in the GitHub CLI ecosystem. Features smart suggestion expansion, complete comment visibility, and universal reply capabilities. Built specifically for AI assistants and automated workflows. | ||
|
|
||
| **NEW**: Now includes enhanced line-by-line commenting with improved validation! | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing comment without validation
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validation fix confirmed working
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This enhancement makes review body optional - great improvement!
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test validation works with fixed diff parsing |
||
|
|
||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test batch comment from integration testing - EDITED
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Production integration test - line-specific review comment
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Production test: Batch review comment #1 |
||
| ## Features | ||
|
|
||
| - 🤖 **AI-optimized design**: Specifically built for usage with AI assistants and automated workflows | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,10 +17,13 @@ const ( | |||||||||||||
| MaxBranchLength = 255 // Git branch name max length | ||||||||||||||
| DefaultPageSize = 30 | ||||||||||||||
|
|
||||||||||||||
| // New constants for line comment validation | ||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice constant definition |
||||||||||||||
| MaxLineNumber = 50000 // Maximum line number for commenting | ||||||||||||||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Excellent addition of MaxLineNumber constant for validation
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added MaxLineNumber constant successfully
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Production test: Batch review comment #2 |
||||||||||||||
|
|
||||||||||||||
| // Security validation constants | ||||||||||||||
| MaxCommentThreadDepth = 10 // Maximum nested comment thread depth | ||||||||||||||
| MaxRepositoryDepth = 5 // Maximum repository path depth | ||||||||||||||
| MaxValidationErrors = 50 // Maximum validation errors to report | ||||||||||||||
| MaxCommentThreadDepth = 10 // Maximum nested comment thread depth | ||||||||||||||
| MaxRepositoryDepth = 5 // Maximum repository path depth | ||||||||||||||
| MaxValidationErrors = 50 // Maximum validation errors to report | ||||||||||||||
|
|
||||||||||||||
| // Display constants | ||||||||||||||
| MaxDisplayBodyLength = 200 // Max length for comment body display | ||||||||||||||
|
|
@@ -35,10 +38,10 @@ const ( | |||||||||||||
| // Security validation patterns | ||||||||||||||
| var ( | ||||||||||||||
| // HTML/Script tag detection patterns | ||||||||||||||
| htmlTagPattern = regexp.MustCompile(`(?i)<\s*\/?\s*(script|iframe|object|embed|form|input|meta|link)\b[^>]*>`) | ||||||||||||||
| scriptPattern = regexp.MustCompile(`(?i)javascript\s*:|<\s*script\b`) | ||||||||||||||
| htmlTagPattern = regexp.MustCompile(`(?i)<\s*\/?\s*(script|iframe|object|embed|form|input|meta|link)\b[^>]*>`) | ||||||||||||||
| scriptPattern = regexp.MustCompile(`(?i)javascript\s*:|<\s*script\b`) | ||||||||||||||
| dangerousAttrPattern = regexp.MustCompile(`(?i)\b(on\w+|javascript|vbscript|data|mocha|livescript)\s*=`) | ||||||||||||||
|
|
||||||||||||||
| // Repository validation patterns | ||||||||||||||
| validRepoPattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$`) | ||||||||||||||
| validOwnerPattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) | ||||||||||||||
|
|
@@ -173,7 +176,7 @@ func parsePositiveInt(s, fieldName string) (int, error) { | |||||||||||||
| // validateCommentBody validates comment body length and content for security | ||||||||||||||
| func validateCommentBody(body string) error { | ||||||||||||||
| if len(body) > MaxCommentLength { | ||||||||||||||
| return formatValidationError("comment body", fmt.Sprintf("%d chars", len(body)), | ||||||||||||||
| return formatValidationError("comment body", fmt.Sprintf("%d chars", len(body)), | ||||||||||||||
| fmt.Sprintf("must be %d characters or less", MaxCommentLength)) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -189,19 +192,19 @@ func validateCommentBody(body string) error { | |||||||||||||
| func validateCommentSecurity(body string) error { | ||||||||||||||
| // Check for dangerous HTML tags | ||||||||||||||
| if htmlTagPattern.MatchString(body) { | ||||||||||||||
| return formatSecurityValidationError("comment body", "dangerous HTML tags detected", | ||||||||||||||
| return formatSecurityValidationError("comment body", "dangerous HTML tags detected", | ||||||||||||||
| "HTML tags like <script>, <iframe>, <object>, <embed>, <form>, <input>, <meta>, <link> are not allowed") | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+195
to
197
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
making human line comment on non review comment |
||||||||||||||
|
|
||||||||||||||
| // Check for JavaScript and event handlers | ||||||||||||||
| if scriptPattern.MatchString(body) { | ||||||||||||||
| return formatSecurityValidationError("comment body", "JavaScript content detected", | ||||||||||||||
| return formatSecurityValidationError("comment body", "JavaScript content detected", | ||||||||||||||
| "JavaScript code, inline event handlers, and script tags are not allowed") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Check for dangerous attributes | ||||||||||||||
| if dangerousAttrPattern.MatchString(body) { | ||||||||||||||
| return formatSecurityValidationError("comment body", "dangerous attributes detected", | ||||||||||||||
| return formatSecurityValidationError("comment body", "dangerous attributes detected", | ||||||||||||||
| "Event handlers and script attributes like onclick, onload, href='javascript:' are not allowed") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -263,24 +266,24 @@ func validateRepositoryAccess(repo string) error { | |||||||||||||
|
|
||||||||||||||
| // Check for potentially dangerous repository patterns | ||||||||||||||
| if strings.Contains(owner, "..") || strings.Contains(repoName, "..") { | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with path traversal", | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with path traversal", | ||||||||||||||
| "Repository names cannot contain '..' sequences for security reasons") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Validate against pattern for additional security | ||||||||||||||
| if !validRepoPattern.MatchString(repo) { | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with invalid characters", | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with invalid characters", | ||||||||||||||
| "Repository must match pattern 'owner/repo' with only alphanumeric, dot, underscore, and hyphen characters") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| // Check for reserved or potentially dangerous names | ||||||||||||||
| dangerousNames := []string{".", "..", "CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"} | ||||||||||||||
| ownerUpper := strings.ToUpper(owner) | ||||||||||||||
| repoUpper := strings.ToUpper(repoName) | ||||||||||||||
|
|
||||||||||||||
| for _, dangerous := range dangerousNames { | ||||||||||||||
| if ownerUpper == dangerous || repoUpper == dangerous { | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with reserved name", | ||||||||||||||
| return formatAccessValidationError("repository", "access repository with reserved name", | ||||||||||||||
| "Repository owner or name cannot use reserved system names") | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
@@ -293,9 +296,9 @@ func validateCommentThreadDepth(depth int) error { | |||||||||||||
| if depth < 0 { | ||||||||||||||
| return formatValidationError("thread depth", fmt.Sprintf("%d", depth), "must be non-negative") | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| if depth > MaxCommentThreadDepth { | ||||||||||||||
| return formatAccessValidationError("comment thread", fmt.Sprintf("nest deeper than %d levels", MaxCommentThreadDepth), | ||||||||||||||
| return formatAccessValidationError("comment thread", fmt.Sprintf("nest deeper than %d levels", MaxCommentThreadDepth), | ||||||||||||||
| fmt.Sprintf("Deep comment nesting can cause performance issues and poor user experience. Maximum allowed depth is %d levels", MaxCommentThreadDepth)) | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -316,7 +319,7 @@ func validateMultipleFields(validations []func() ValidationResult) []ValidationR | |||||||||||||
| for _, validate := range validations { | ||||||||||||||
| result := validate() | ||||||||||||||
| results = append(results, result) | ||||||||||||||
|
|
||||||||||||||
| // Stop early if we hit the max validation errors | ||||||||||||||
| if len(results) >= MaxValidationErrors { | ||||||||||||||
| break | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great documentation structure