refactor: implement semantic function clustering improvements across pkg/#19926
Merged
refactor: implement semantic function clustering improvements across pkg/#19926
Conversation
8 tasks
- Extract newValidationLogger() helper to validation_helpers.go and update all ~30 *_validation.go files to use it - Split codemod_permissions.go into codemod_permissions_read.go and codemod_permissions_write.go following one-codemod-per-file pattern - Move repoutil.SanitizeForFilename to stringutil package with tests - Consolidate error_helpers.go + error_aggregation.go + shared_workflow_error.go into workflow_errors.go - Establish Format* vs Render* naming convention in console package via doc.go - Document intentional String()/IsValid() method duplication in constants package - Consolidate 14 safe_outputs_* files into 4 cohesive modules: safe_outputs_config.go, safe_outputs_validation.go, safe_outputs_generation.go, safe_outputs_jobs.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor semantic function clustering analysis results
refactor: implement semantic function clustering improvements across pkg/
Mar 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors pkg/ to reduce duplication and consolidate related workflow functionality (validation logging, safe-outputs modules, and workflow error types), while also moving/organizing a few cross-package utilities and docs.
Changes:
- Add a shared
newValidationLogger(domain)helper and update many*_validation.gofiles to use it. - Consolidate the workflow safe-outputs subsystem and workflow error types into fewer, larger modules.
- Move
SanitizeForFilenametopkg/stringutil, split a combined codemod into separate files, and add console naming-convention documentation.
Reviewed changes
Copilot reviewed 57 out of 57 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_errors.go | Consolidates workflow error types + aggregation into one file |
| pkg/workflow/validation_helpers.go | Adds newValidationLogger(domain) helper |
| pkg/workflow/tools_validation.go | Switches to newValidationLogger("tools") |
| pkg/workflow/template_validation.go | Switches to newValidationLogger("template") |
| pkg/workflow/template_injection_validation.go | Switches to newValidationLogger("template_injection") |
| pkg/workflow/strict_mode_validation.go | Switches to newValidationLogger("strict_mode") |
| pkg/workflow/step_order_validation.go | Switches to newValidationLogger("step_order") |
| pkg/workflow/shared_workflow_error.go | Removes file (moved into workflow_errors.go) |
| pkg/workflow/secrets_validation.go | Switches to newValidationLogger("secrets") |
| pkg/workflow/schema_validation.go | Switches to newValidationLogger("schema") |
| pkg/workflow/sandbox_validation.go | Switches to newValidationLogger("sandbox") |
| pkg/workflow/safe_outputs_validation.go | Adds/merges safe-outputs target validation into consolidated validation file |
| pkg/workflow/safe_outputs_tools_generation.go | Removes file (tools generation moved into consolidated modules) |
| pkg/workflow/safe_outputs_target_validation.go | Removes file (merged into safe_outputs_validation.go) |
| pkg/workflow/safe_outputs_steps.go | Removes file (merged into consolidated safe-outputs modules) |
| pkg/workflow/safe_outputs_permissions.go | Removes file (merged into consolidated safe-outputs modules) |
| pkg/workflow/safe_outputs_jobs.go | Expands file to include permissions/env/step helpers previously split across multiple files |
| pkg/workflow/safe_outputs_env.go | Removes file (merged into consolidated safe-outputs modules) |
| pkg/workflow/safe_outputs_config_messages.go | Removes file (messages logic moved into consolidated config module) |
| pkg/workflow/safe_outputs_config_helpers.go | Removes file (helpers moved into consolidated modules) |
| pkg/workflow/safe_outputs_config_generation_helpers.go | Removes file (generation helpers moved into consolidated modules) |
| pkg/workflow/safe_outputs_config_generation.go | Removes file (generation moved into consolidated modules) |
| pkg/workflow/safe_outputs_config.go | Expands file to include GitHub App + messages config logic (consolidation) |
| pkg/workflow/safe_outputs_app.go | Removes file (GitHub App logic moved into consolidated config module) |
| pkg/workflow/safe_outputs.go | Removes file (previously documentation-only) |
| pkg/workflow/runtime_validation.go | Switches to newValidationLogger("runtime") |
| pkg/workflow/runs_on_validation.go | Switches to newValidationLogger("runs_on") |
| pkg/workflow/repository_features_validation.go | Switches to newValidationLogger("repository_features") |
| pkg/workflow/pip_validation.go | Switches to newValidationLogger("pip") |
| pkg/workflow/permissions_validation.go | Switches to newValidationLogger("permissions") |
| pkg/workflow/npm_validation.go | Switches to newValidationLogger("npm") |
| pkg/workflow/network_firewall_validation.go | Switches to newValidationLogger("network_firewall") |
| pkg/workflow/mcp_config_validation.go | Switches to newValidationLogger("mcp_config") |
| pkg/workflow/labels_validation.go | Switches to newValidationLogger("labels") |
| pkg/workflow/imported_steps_validation.go | Switches to newValidationLogger("imported_steps") |
| pkg/workflow/firewall_validation.go | Switches to newValidationLogger("firewall") |
| pkg/workflow/features_validation.go | Switches to newValidationLogger("features") |
| pkg/workflow/expression_validation.go | Switches to newValidationLogger("expression") |
| pkg/workflow/error_helpers.go | Removes file (merged into workflow_errors.go) |
| pkg/workflow/error_aggregation.go | Removes file (merged into workflow_errors.go) |
| pkg/workflow/engine_validation.go | Switches to newValidationLogger("engine") |
| pkg/workflow/docker_validation.go | Switches to newValidationLogger("docker") |
| pkg/workflow/dispatch_workflow_validation.go | Switches to newValidationLogger("dispatch_workflow") |
| pkg/workflow/dangerous_permissions_validation.go | Switches to newValidationLogger("dangerous_permissions") |
| pkg/workflow/concurrency_validation.go | Switches to newValidationLogger("concurrency") |
| pkg/workflow/compiler_filters_validation.go | Switches to newValidationLogger("filter") |
| pkg/workflow/agent_validation.go | Switches to newValidationLogger("agent") |
| pkg/stringutil/sanitize_test.go | Moves SanitizeForFilename tests into stringutil |
| pkg/stringutil/sanitize.go | Adds stringutil.SanitizeForFilename |
| pkg/repoutil/repoutil_test.go | Removes SanitizeForFilename tests from repoutil |
| pkg/repoutil/repoutil.go | Removes SanitizeForFilename from repoutil |
| pkg/constants/constants.go | Documents intentional String()/IsValid() duplication across string-typed constants |
| pkg/console/doc.go | Adds package doc describing Format* vs Render* naming convention |
| pkg/cli/trial_command.go | Switches call sites to stringutil.SanitizeForFilename |
| pkg/cli/codemod_permissions_write.go | Removes read-codemod code after splitting per-codemod-per-file |
| pkg/cli/codemod_permissions_read.go | New file containing the read shorthand codemod |
💡 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
+766
to
+770
| // convertPermissionsToAppTokenFields converts job Permissions to permission-* action inputs | ||
| // This follows GitHub's recommendation for explicit permission control | ||
| // Note: This only includes permissions that are valid for GitHub App tokens. | ||
| // Some GitHub Actions permissions (like 'discussions', 'models') don't have | ||
| // corresponding GitHub App permissions and are skipped. |
There was a problem hiding this comment.
The doc comment says permissions like discussions are skipped because they don't have GitHub App equivalents, but the implementation later in this function adds permission-discussions. Please reconcile the comment with the actual behavior to avoid misleading maintainers.
| // Check for permissions line with shorthand | ||
| if strings.HasPrefix(trimmedLine, "permissions:") { | ||
| // Handle shorthand on same line: "permissions: read" or "permissions: write" | ||
| if strings.Contains(trimmedLine, ": read") && !strings.Contains(trimmedLine, "read-all") && !strings.Contains(trimmedLine, ": read\n") { |
There was a problem hiding this comment.
trimmedLine is produced by strings.TrimSpace(line), so it will never contain a newline. The !strings.Contains(trimmedLine, ": read\n") check is therefore redundant/unreachable and can be removed to simplify the condition.
Suggested change
| if strings.Contains(trimmedLine, ": read") && !strings.Contains(trimmedLine, "read-all") && !strings.Contains(trimmedLine, ": read\n") { | |
| if strings.Contains(trimmedLine, ": read") && !strings.Contains(trimmedLine, "read-all") { |
Comment on lines
+579
to
+583
| name: "multiple slashes", | ||
| slug: "owner/repo/extra", | ||
| expected: "owner-repo-extra", | ||
| }, | ||
| { |
There was a problem hiding this comment.
This test table includes the same input/expected pair twice (owner/repo/extra → owner-repo-extra) under two different case names. Consider removing one of these duplicate cases to keep the test list minimal and easier to maintain.
Suggested change
| name: "multiple slashes", | |
| slug: "owner/repo/extra", | |
| expected: "owner-repo-extra", | |
| }, | |
| { |
Comment on lines
+737
to
+739
| // Extract repo name from github.repository (which is "owner/repo") | ||
| // Using GitHub Actions expression: split(github.repository, '/')[1] | ||
| steps = append(steps, " repositories: ${{ github.event.repository.name }}\n") |
There was a problem hiding this comment.
The comment says the repo name is extracted via split(github.repository, '/')[1], but the code actually uses ${{ github.event.repository.name }}. Please update the comment to match the implementation (or switch the implementation to match the documented expression) to avoid confusion when maintaining the YAML generation logic.
|
I need help no proble m
في السبت، ٧ مارس ٢٠٢٦، ٠٥:٢٥ Peli de Halleux ***@***.***> كتب:
… Merged #19926 <#19926> into main.
—
Reply to this email directly, view it on GitHub
<#19926 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/B2DFN3WL5AYOOTRFQRZV5KL4POXFPAVCNFSM6AAAAACWKE77J2VHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMRTGM2TEMRWHAZTOMY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
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.
Automated semantic analysis identified 7 refactoring opportunities across
pkg/workflow/,pkg/cli/,pkg/console/,pkg/constants/,pkg/repoutil/, andpkg/stringutil/. This PR addresses all of them.Changes
pkg/workflow/— validation logger boilerplate eliminatedAdded
newValidationLogger(domain string)tovalidation_helpers.go. Every*_validation.gofile previously duplicatedlogger.New("workflow:<domain>_validation")— now each uses the helper:pkg/workflow/— safe_outputs file sprawl: 14 → 4 filesConsolidated the fragmented
safe_outputs_*subsystem into cohesive modules following the data-flow pattern:safe_outputs_config.gosafe_outputs_config.go+safe_outputs_app.go+safe_outputs_config_messages.go+safe_outputs.gosafe_outputs_validation.gosafe_outputs_domains_validation.go+safe_outputs_target_validation.gosafe_outputs_generation.gosafe_outputs_config_generation.go+safe_outputs_config_generation_helpers.go+safe_outputs_config_helpers.go+safe_outputs_tools_generation.gosafe_outputs_jobs.gosafe_outputs_jobs.go+safe_outputs_permissions.go+safe_outputs_steps.go+safe_outputs_env.gopkg/workflow/— error types: 3 → 1 fileMerged
error_helpers.go+error_aggregation.go+shared_workflow_error.gointoworkflow_errors.gowith a unified doc comment covering all error types.pkg/cli/— codemod_permissions.go splitcodemod_permissions.goviolated the one-codemod-per-file rule followed by all other 50+ codemod files. Split intocodemod_permissions_read.goandcodemod_permissions_write.go.pkg/stringutil/— SanitizeForFilename moved from repoutilrepoutil.SanitizeForFilename(convertsowner/repo→owner-repo) is a pure string utility with no repo logic. Moved tostringutil.SanitizeForFilenamealongside the otherSanitize*functions. Updated the 3 call sites intrial_command.goand relocated tests.pkg/console/— Format* vs Render* convention documentedCreated
doc.goestablishing the naming convention:Format*= pure string transformations for single items/messages;Render*= structured/multi-element layout output.pkg/constants/— intentional String()/IsValid() duplication annotatedAdded a comment block explaining why
Version,JobName,StepID,CommandPrefix, andDocURLeach define identicalString()/IsValid()bodies — Go's type system requires it, and code-generation adds more complexity than the duplication itself.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/graphql/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw GO111MODULE 64/bin/go git rev-�� --git-dir go /usr/bin/git -json GO111MODULE 64/bin/go git(http block)/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw 1537139/b433/wor-p x_amd64/link git rev-�� --show-toplevel x_amd64/link /usr/bin/git --show-toplevel node x_amd64/vet git(http block)/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw --jq /usr/bin/git git -C /tmp/gh-aw-test-runs/20260307-041324-39469/test-2739779282 rev-parse /usr/bin/git @{u} git /usr/bin/git git(http block)https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq /usr/bin/git -json GO111MODULE 64/bin/go git rev-�� --show-toplevel(http block)/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha xterm-color git /usr/bin/gh --show-toplevel go /usr/bin/git gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq /usr/bin/git --show-toplevel go /usr/bin/git git(http block)/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha --show-toplevel l /usr/bin/git --show-toplevel 1147781/b001/clirev-parse /usr/bin/git git rev-�� --show-toplevel git /usr/bin/git --show-toplevel git /usr/bin/git git(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v3/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha dRRm/hXuSY5ACtRY9la0wdRRm l(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha --show-toplevel infocmp 1147781/b346/vet.cfg xterm-color x_amd64/compile /usr/bin/git git rev-�� --show-toplevel git /opt/hostedtoolcache/go/1.25.0/x64/pkg/tool/linux_amd64/vet --show-toplevel go /usr/bin/git /opt/hostedtoolcache/go/1.25.0/x64/pkg/tool/linux_amd64/vet(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha --show-toplevel git /opt/hostedtoolcache/node/24.14.0/x64/bin/node --show-toplevel git /usr/bin/git node js/f�� 64/pkg/tool/linu--show-toplevel git /opt/hostedtoolcache/node/24.14.0/x64/bin/node --show-toplevel 64/pkg/tool/linurev-parse /usr/bin/git node(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v5/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha --show-toplevel node /usr/bin/find prettier --check 64/bin/go find -O3 /var/lib/php/sessions/ -ignore_readdir_race /usr/bin/git -mindepth 1 -name git(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha --show-toplevel go /usr/bin/git -json GO111MODULE ache/go/1.25.0/x--show-toplevel git rev-�� --show-toplevel go /usr/bin/git -json GO111MODULE is-results git(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v6/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq .object.sha /tmp/gh-aw-test-runs/20260307-040905-28297/test--nxv config 1537139/b439/vet.cfg remote.origin.urgit GO111MODULE 64/bin/go git init�� GOMODCACHE go clusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle -json GO111MODULE 64/bin/go node(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq .object.sha -test.paniconexit0 -test.v=true /usr/bin/git -test.timeout=10git -test.run=^Test -test.short=true--show-toplevel git init�� GOMODCACHE go /usr/bin/git -json GO111MODULE 64/bin/go git(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq .object.sha --show-toplevel go /usr/bin/git -json GO111MODULE ache/go/1.25.0/x--show-toplevel git rev-�� --show-toplevel go /usr/bin/git -json GO111MODULE 64/pkg/tool/linu--show-toplevel git(http block)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 ck '**/*.cjs' '*GOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE ache/go/1.25.0/xGO111MODULE env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE sh(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE node(http block)https://api.github.com/repos/actions/setup-go/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha /tmp/gh-aw-test-runs/20260307-040905-28297/test-2000129479/.github/workflows(http block)/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha /tmp/gh-aw-test-runs/20260307-041110-32792/test-3526710684/.github/workflows config /usr/bin/git remote.origin.urgit go /usr/bin/git git rev-�� --show-toplevel git ache/node/24.14.0/x64/bin/node --show-toplevel go /usr/bin/git ache/node/24.14.0/x64/bin/node(http block)/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha .cfg git /usr/bin/git --show-toplevel git /usr/bin/git git rev-�� --show-toplevel git /usr/bin/git --show-toplevel ache/go/1.25.0/xrev-parse /usr/bin/git git(http block)https://api.github.com/repos/actions/setup-node/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha --show-toplevel go /usr/bin/git ck 'scripts/**/*git GO111MODULE 64/bin/go git init�� GOMODCACHE go /usr/bin/git ays.md GO111MODULE 64/bin/go git(http block)/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha --show-toplevel git /usr/bin/git --show-toplevel go /usr/bin/git git rev-�� --show-toplevel git 0/x64/bin/node ted-objects.md go /usr/bin/git 0/x64/bin/node(http block)/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha /usr/bin/git git /usr/bin/git --show-toplevel git /usr/bin/git git rev-�� --show-toplevel git /usr/bin/git --show-toplevel ache/go/1.25.0/xrev-parse(http block)https://api.github.com/repos/actions/upload-artifact/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v4 --jq .object.sha -json GO111MODULE 1537139/b413/repoutil.test GOINSECURE GOMOD GOMODCACHE 1537139/b413/repoutil.test e=/t�� t0 GO111MODULE(http block)/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v4 --jq .object.sha --show-toplevel git /usr/bin/git add origin /usr/bin/git git rev-�� --show-toplevel git /usr/bin/git --show-toplevel go /usr/bin/infocmp--show-toplevel git(http block)/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v4 --jq .object.sha --show-toplevel git ache/node/24.14.0/x64/bin/node --show-toplevel node /usr/bin/git git _out�� --show-toplevel git /usr/bin/git --show-toplevel git /opt/hostedtoolc--show-toplevel git(http block)https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts/usr/bin/gh gh run download 1 --dir test-logs/run-1 GO111MODULE 002e2e4ac57478dd1e9e84ba08852377bcaa8cd93b04e6c1-d GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 0/x64/bin/node GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run download 1 --dir test-logs/run-1 node /usr/bin/git /home/REDACTED/worgit(http block)/usr/bin/gh gh run download 1 --dir test-logs/run-1 git /usr/bin/git --show-toplevel git /usr/bin/git git rev-�� --show-toplevel git ache/node/24.14.0/x64/bin/node --show-toplevel git /usr/bin/git git(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifactsupdate all ~30 *_validati-o 64/bin/go GOSUMDB GOWORK 64/bin/go node` (http block)
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.