Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/facd6500-049a-408b-99d6-ba96fb3ea1f1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/facd6500-049a-408b-99d6-ba96fb3ea1f1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor expression detection helpers in pkg/workflow
Refactor expression helper semantics into a single workflow source and remove duplicate CLI version state
Apr 19, 2026
Collaborator
|
@copilot more tests |
…d CLI version unification Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Commit pushed:
|
Contributor
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (145 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft captures two distinct design decisions implicit in this PR:
What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: Note 🔒 Integrity filter blocked 1 itemThe following item were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates GitHub Actions expression detection into shared helpers within pkg/workflow and removes duplicate CLI-local version state by delegating CLI version getters/setters to the workflow package.
Changes:
- Centralized expression checks (
hasExpressionMarker,containsExpression,isExpression) and updated call sites to use consistent semantics. - Removed several file-local expression helper implementations in favor of the shared helpers.
- Unified version state by making
pkg/clidelegate version operations topkg/workflow, with added tests validating delegation.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/expression_patterns.go | Adds shared expression helper functions alongside existing regex patterns. |
| pkg/workflow/expression_patterns_test.go | Adds focused tests covering the new expression helper semantics. |
| pkg/workflow/tools_parser.go | Updates expression detection for GitHub tool parsing and templated timeout parsing. |
| pkg/workflow/templatables.go | Replaces local expression checks with shared isExpression and removes now-redundant helper. |
| pkg/workflow/shell.go | Switches shell argument escaping to use shared containsExpression. |
| pkg/workflow/safe_outputs_validation.go | Uses shared containsExpression for target validation and removes local helper. |
| pkg/workflow/safe_outputs_max_validation.go | Uses shared isExpression to skip compile-time validation for expression max values. |
| pkg/workflow/frontmatter_extraction_metadata.go | Uses shared isExpression when extracting timeout fields from frontmatter. |
| pkg/workflow/dispatch_repository_validation.go | Uses shared hasExpressionMarker and removes local expression marker helper. |
| pkg/workflow/expression_safety_validation.go | Renames list membership helper to avoid collision (containsExpressionInList). |
| pkg/workflow/safe_outputs_target_validation_test.go | Updates test naming and calls to reflect shared containsExpression. |
| pkg/cli/version.go | Removes CLI-local version state; delegates to workflow version functions. |
| pkg/cli/version_test.go | Adds tests to ensure CLI version delegation behavior stays correct. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 1
Comment on lines
+73
to
+82
| // containsExpression reports whether s contains a complete non-empty GitHub Actions expression. | ||
| // A complete expression has a "${{" marker that appears before a closing "}}" marker | ||
| // with at least one character between them. | ||
| func containsExpression(s string) bool { | ||
| _, afterOpen, found := strings.Cut(s, "${{") | ||
| if !found { | ||
| return false | ||
| } | ||
| closeIdx := strings.Index(afterOpen, "}}") | ||
| return closeIdx > 0 |
There was a problem hiding this comment.
containsExpression can return true for strings that are not actually a complete ${{ ... }} expression because it just looks for the first ${{ and then the first }} anywhere later in the string. For example, it will treat ${{ '}}' (no closing marker) or a string with an unmatched opening marker followed by a later unrelated }} as containing a complete expression. Since this helper is now used for validation/quoting decisions, consider implementing it using the existing ExpressionPattern (or a small scanner that finds a properly paired ${{ … }}) so it only returns true when a real expression token is present.
Suggested change
| // containsExpression reports whether s contains a complete non-empty GitHub Actions expression. | |
| // A complete expression has a "${{" marker that appears before a closing "}}" marker | |
| // with at least one character between them. | |
| func containsExpression(s string) bool { | |
| _, afterOpen, found := strings.Cut(s, "${{") | |
| if !found { | |
| return false | |
| } | |
| closeIdx := strings.Index(afterOpen, "}}") | |
| return closeIdx > 0 | |
| // findExpressionEnd returns the index of the closing "}}" for the expression that starts at | |
| // openIdx, or -1 if no properly closed expression exists. Closing markers inside quoted | |
| // string literals are ignored. | |
| func findExpressionEnd(s string, openIdx int) int { | |
| start := openIdx + len("${{") | |
| if start >= len(s) { | |
| return -1 | |
| } | |
| var quote byte | |
| for i := start; i < len(s)-1; i++ { | |
| ch := s[i] | |
| if quote != 0 { | |
| if ch == '\\' && i+1 < len(s) { | |
| i++ | |
| continue | |
| } | |
| if ch == quote { | |
| quote = 0 | |
| } | |
| continue | |
| } | |
| switch ch { | |
| case '\'', '"', '`': | |
| quote = ch | |
| case '}': | |
| if s[i+1] == '}' && i > start { | |
| return i | |
| } | |
| } | |
| } | |
| return -1 | |
| } | |
| // containsExpression reports whether s contains a complete non-empty GitHub Actions expression. | |
| // A complete expression has a "${{" marker that appears before a matching closing "}}" | |
| // marker, with at least one character between them. | |
| func containsExpression(s string) bool { | |
| for searchFrom := 0; searchFrom < len(s); { | |
| openRel := strings.Index(s[searchFrom:], "${{") | |
| if openRel == -1 { | |
| return false | |
| } | |
| openIdx := searchFrom + openRel | |
| if findExpressionEnd(s, openIdx) != -1 { | |
| return true | |
| } | |
| searchFrom = openIdx + len("${{") | |
| } | |
| return false |
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 75/100
Test Classification DetailsView all 4 tests
Test Inflation Detected
Analysis Notes
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §24632327321
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0031cc01-b58a-4390-8312-a5de6e666c0e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/0031cc01-b58a-4390-8312-a5de6e666c0e Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Added more focused tests in
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.
Expression detection logic in
pkg/workflowhad drifted into multiple near-duplicate helpers with different semantics, and CLI/workflow versioning maintained two mutable copies that required manual synchronization. This PR consolidates expression checks behind shared helpers and makes workflow version the only version state.Expression helper consolidation (
pkg/workflow)expression_patterns.go:hasExpressionMarker(permissive${{marker check)containsExpression(complete, non-empty${{ ... }}containment)isExpression(entire string is an expression)dispatch_repository_validation.goshell.gosafe_outputs_validation.gotemplatables.gotools_parser.gosafe_outputs_max_validation.gofrontmatter_extraction_metadata.goexpression_safety_validation.goby renaming list membership helper tocontainsExpressionInList.Version state unification (
pkg/cli)versionvariable andinitsynchronization path inpkg/cli/version.go.cli.GetVersion()now delegates directly toworkflow.GetVersion().cli.SetVersionInfo(v)now delegates directly toworkflow.SetVersion(v).Tests aligned with new semantics
pkg/workflow/expression_patterns_test.go.pkg/workflow/safe_outputs_target_validation_test.go.pkg/cli/version_test.goto verify CLI↔workflow version delegation behavior.pkg/workflow/shell_test.gofor embedded expressions, incomplete marker input (${{ ...), and empty expression body (${{}}) handling in shell escaping.pkg/workflow/dispatch_repository_test.gofor permissive marker-based bypass behavior in repository/allowed repository validation, with explicit runtime-behavior documentation for malformed expressions.