refactor: split expression_validation.go into three focused files#21505
Merged
refactor: split expression_validation.go into three focused files#21505
Conversation
7 tasks
Split the 715-line expression_validation.go (2.4× the 300-line hard limit) into three focused files with clear responsibilities: - expression_safety_validation.go (299 lines): safety orchestration, allowlist checks, dangerous-property detection - expression_syntax_validation.go (230 lines): syntax-level helpers (balanced braces/quotes, expression content validation) - runtime_import_validation.go (154 lines): runtime-import path extraction and file validation No function logic was changed. Pure code reorganization refactor. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] [plan] Split expression_validation.go into three focused files
refactor: split expression_validation.go into three focused files
Mar 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors workflow expression validation by splitting the previous monolithic implementation into focused modules for expression safety (allowlist), expression syntax checks, and runtime-import file validation.
Changes:
- Split expression validation into
expression_safety_validation.goandexpression_syntax_validation.go. - Introduce
runtime_import_validation.goto validate expressions inside runtime-imported markdown files and enforce.github-scoped paths. - Remove the old combined
expression_validation.go.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| pkg/workflow/runtime_import_validation.go | Adds extraction + compile-time validation of runtime-import file paths and expression safety within imported files. |
| pkg/workflow/expression_safety_validation.go | Contains allowlist-based expression safety validation logic previously in the removed file. |
| pkg/workflow/expression_syntax_validation.go | Contains syntax-level validation helpers (balanced braces/quotes/parentheses, parsing checks). |
| pkg/workflow/expression_validation.go | Deleted in favor of the new split files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+44
to
+61
| for _, match := range matches { | ||
| if len(match) > 1 { | ||
| pathWithRange := strings.TrimSpace(match[1]) | ||
|
|
||
| // Remove line range if present (e.g., "file.md:10-20" -> "file.md") | ||
| filepath := pathWithRange | ||
| if colonIdx := strings.Index(pathWithRange, ":"); colonIdx > 0 { | ||
| // Check if what follows colon looks like a line range (digits-digits) | ||
| afterColon := pathWithRange[colonIdx+1:] | ||
| if regexp.MustCompile(`^\d+-\d+$`).MatchString(afterColon) { | ||
| filepath = pathWithRange[:colonIdx] | ||
| } | ||
| } | ||
|
|
||
| // Skip URLs - they don't need file validation | ||
| if strings.HasPrefix(filepath, "http://") || strings.HasPrefix(filepath, "https://") { | ||
| continue | ||
| } |
Comment on lines
+49
to
+66
| filepath := pathWithRange | ||
| if colonIdx := strings.Index(pathWithRange, ":"); colonIdx > 0 { | ||
| // Check if what follows colon looks like a line range (digits-digits) | ||
| afterColon := pathWithRange[colonIdx+1:] | ||
| if regexp.MustCompile(`^\d+-\d+$`).MatchString(afterColon) { | ||
| filepath = pathWithRange[:colonIdx] | ||
| } | ||
| } | ||
|
|
||
| // Skip URLs - they don't need file validation | ||
| if strings.HasPrefix(filepath, "http://") || strings.HasPrefix(filepath, "https://") { | ||
| continue | ||
| } | ||
|
|
||
| // Add to list if not already seen | ||
| if !seen[filepath] { | ||
| paths = append(paths, filepath) | ||
| seen[filepath] = true |
Comment on lines
+50
to
+55
| if colonIdx := strings.Index(pathWithRange, ":"); colonIdx > 0 { | ||
| // Check if what follows colon looks like a line range (digits-digits) | ||
| afterColon := pathWithRange[colonIdx+1:] | ||
| if regexp.MustCompile(`^\d+-\d+$`).MatchString(afterColon) { | ||
| filepath = pathWithRange[:colonIdx] | ||
| } |
Comment on lines
+121
to
+122
| // Skip validation for optional imports ({{#runtime-import? ...}}) | ||
| // We can't determine if it's optional here, but missing files will be caught at runtime |
Comment on lines
+32
to
+35
| // validateBalancedBraces checks that all ${{ }} braces are balanced and properly closed | ||
| func validateBalancedBraces(group string) error { | ||
| expressionValidationLog.Print("Checking balanced braces in expression") | ||
| openCount := 0 |
Comment on lines
+213
to
+217
| // Check for OR expressions with literals (e.g., "inputs.repository || 'default'") | ||
| if !allowed { | ||
| orPattern := regexp.MustCompile(`^(.+?)\s*\|\|\s*(.+)$`) | ||
| orMatch := orPattern.FindStringSubmatch(expression) | ||
| if len(orMatch) > 2 { |
Contributor
|
@copilot review comments |
- runtime_import_validation.go: promote regex vars to package-level (runtimeImportMacroRe, lineRangeRe), skip empty import targets, rename 'filepath' local var to 'importPath' (avoid package shadowing), fix stale comment about missing file handling - expression_safety_validation.go: promote orPattern to package-level orExpressionPattern, move containsLogicalOperators to syntax file, restore accidentally-removed containsExpression helper - expression_syntax_validation.go: add containsLogicalOperators (its only call site, moved from safety file) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Addressed all 6 review comments in f268a17:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
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.
pkg/workflow/expression_validation.gowas 715 lines — 2.4× the 300-line hard limit — mixing unrelated concerns in a single file.Split
expression_safety_validation.govalidateExpressionSafety,validateSingleExpressionexpression_syntax_validation.govalidateExpressionSyntax, shared literal regexesruntime_import_validation.go{{#runtime-import}}path extraction and expression validation of imported filesPure code reorganization — no logic changes.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/repos/actions/github-script/git/ref/tags/v8/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha 5Tms/lnm6SkSogsgGOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE 5849564/b391/imp/tmp/go-build1716240525/b084/vet.cfg -c che/go-build/12/GOSUMDB GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc-buildtags(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha prettier --check 64/bin/go **/*.ts **/*.json --ignore-path go env GOPATH sh 64/bin/go tierignore(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env ath ../../../.pr**/*.json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet(http block)If you need me to access, download, or install something from one of these locations, you can either:
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.