Conversation
This comment has been minimized.
This comment has been minimized.
|
Hey A couple of things to address before this is ready for review:
If you'd like a hand addressing the remaining work, here's a ready-to-use prompt:
|
…zeHeredocDelimiters to strings.go Agent-Logs-Url: https://github.com/github/gh-aw/sessions/3f255865-2b14-4042-95a3-b72d8d7b1fd5 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refactors workflow internals by co-locating heredoc normalization utilities with other string helpers and consolidating scattered any → []string conversion helpers into a single helper module.
Changes:
- Moved
heredocDelimiterREandnormalizeHeredocDelimitersfromcompiler.gointostrings.go. - Consolidated multiple
any → []stringhelpers intovalidation_helpers.goand removed their old definitions. - Removed now-unused imports after function moves.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/validation_helpers.go | Adds consolidated any → []string helpers and documents the variants. |
| pkg/workflow/strings.go | Co-locates heredoc delimiter regex + normalization with other heredoc/string utilities. |
| pkg/workflow/role_checks.go | Removes the local extractStringSliceField helper (now provided centrally). |
| pkg/workflow/compiler_filters_validation.go | Removes local toStringSlice helper and unused import. |
| pkg/workflow/compiler.go | Removes heredoc normalization utilities and unused regexp import. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| // | ||
| // - parseStringSliceAny() - Canonical coercion of []string/[]any to []string; skips non-strings | ||
| // - toStringSlice() - Strict variant: returns error on non-string elements; also accepts bare string | ||
| // - extractStringSliceField() - Accepts string/[]string/[]any; skips empty strings; wraps bare string |
There was a problem hiding this comment.
The file-level doc says extractStringSliceField() “skips empty strings”, but the implementation only skips empty strings for string and []any inputs; for []string it returns the slice as-is (so empty-string elements are preserved). Either update the docs to match the actual behavior, or filter empty strings from the []string case as well so the documented behavior is correct.
| // - extractStringSliceField() - Accepts string/[]string/[]any; skips empty strings; wraps bare string | |
| // - extractStringSliceField() - Accepts string/[]string/[]any; wraps bare string; skips empty strings only when coercing from string or []any |
| validationHelpersLog.Printf("Extracted single %s: %s", fieldName, v) | ||
| return []string{v} |
There was a problem hiding this comment.
extractStringSliceField now logs via validationHelpersLog, but previously (when this helper lived in role_checks.go) it logged via roleLog. That changes log categorization/output, which is observable behavior and conflicts with the PR description’s “No behavioral changes”. Consider taking a logger as a parameter (or otherwise preserving the previous logger) if you want to keep log output stable, or update the PR description to acknowledge the logging change.
Four near-duplicate
any→[]stringconverters were scattered across unrelated files, andnormalizeHeredocDelimiterslived incompiler.gowhile all other heredoc utilities lived instrings.go.Changes
strings.go— MoveheredocDelimiterRE+normalizeHeredocDelimitershere fromcompiler.go, alongsideGenerateHeredocDelimiterFromSeed/ValidateHeredocContent/ValidateHeredocDelimitervalidation_helpers.go— Consolidateany→[]stringconverters:toStringSlicefromcompiler_filters_validation.go(strict: errors on non-string elements, also accepts a barestring)extractStringSliceFieldfromrole_checks.go(lenient: skips non-strings and empty strings, wraps barestring)parseStringSliceAnywas already here (lenient: skips non-strings, preserves empty strings)compiler_filters_validation.go— RemovetoStringSliceand now-unusederrorsimportcompiler.go— RemoveheredocDelimiterRE,normalizeHeredocDelimiters, and now-unusedregexpimportrole_checks.go— RemoveextractStringSliceFieldNo behavioral changes. All call sites remain unchanged since these are package-internal functions.
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 bin/bash git rev-�� --show-toplevel go /usr/bin/git -json GO111MODULE /opt/hostedtoolc--show-toplevel git(http block)https://api.github.com/orgs/test-owner/actions/secrets/usr/bin/gh gh api /orgs/test-owner/actions/secrets --jq .secrets[].name aEPi/kReFZFZIhM0GOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE 7588832/b397/impGO111MODULE -c che/go-build/12/GOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc-test.v=true(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 --show-toplevel x_amd64/compile /usr/bin/git -json GO111MODULE 64/bin/go /usr/bin/git remo�� -v go /usr/bin/git ty-test.md GO111MODULE 64/bin/go 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 /tmp/TestGuardPolicyMinIntegrityOnlymin-integrity_with_repos=public_1960448525/001 remote clusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle hyphen3124705747git hyphen3124705747rev-parse 64/bin/go git -C /home/REDACTED/work/gh-aw/gh-aw/.github/workflows rev-parse /tmp/go-build2571715227/b435/stringutil.test "prettier" --chegit sh 64/bin/go /tmp/go-build2571715227/b435/stringutil.test(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 07/001/test-complex-frontmatter-with-tools.md GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/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 go /usr/bin/git -json GO111MODULE 64/pkg/tool/linu--show-toplevel git rev-�� it/ref/tags/v4 64/pkg/tool/linux_amd64/vet /usr/bin/git 07/001/test-simpgit GO111MODULE ache/go/1.25.8/x--show-toplevel 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 /opt/hostedtoolc--show-toplevel git rev-�� --show-toplevel /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet otOrdering1484408848/001/go/1.25.0/x64/bin/go -bool -buildtags ache/go/1.25.8/x--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 --show-toplevel go /usr/bin/git -json GO111MODULE ache/go/1.25.8/x64/bin/go git rev-�� --show-toplevel go /usr/bin/git -json GO111MODULE bash git(http block)https://api.github.com/repos/actions/github-script/git/ref/tags/v9/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq .object.sha GOSUMDB GOWORK 64/bin/go GOINSECURE(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq .object.sha "prettier" --cheGOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc-trimpath -o /tmp/go-build411-p -trimpath 64/bin/go -p github.com/githu--git-dir=/tmp/TestParseDefaultBranchFromLsRemoteWithRealGitbranch_with_hyphen3124705747/001 -lang=go1.25 go(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq .object.sha k/gh-aw/gh-aw/pkGOINSECURE k/gh-aw/gh-aw/pkGOMOD 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcGOPROXY -o /tmp/go-build411GOSUMDB -trimpath 64/bin/go -p main -lang=go1.25 go(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 GOMODCACHE go /usr/bin/git -json GO111MODULE x_amd64/vet git rev-�� --show-toplevel x_amd64/vet /usr/bin/git -json GO111MODULE 64/bin/go 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 add remote2 /usr/bin/git 958255/001 GO111MODULE x_amd64/vet git conf�� user.email test@example.com /usr/bin/git -json GO111MODULE 64/bin/go git(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 sistency_GoAndJavaScript2188178007/001/test-simple-frontmatter.md -buildtags /usr/bin/gh -errorsas -ifaceassert -nilfunc gh api r-test834263649/existing.md --jq /usr/bin/git Htin/ORwXMWFyuJ5git GO111MODULE 64/bin/go git(http block)https://api.github.com/repos/actions/upload-artifact/git/ref/tags/v7/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v7 --jq .object.sha 01/test1.md GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go env 1757-30416/test-3560970757 GO111MODULE res.lock.yml GOINSECURE GOMOD GOMODCACHE 1715227/b418/logger.test(http block)/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v7 --jq .object.sha 2417407117 stmain.go 1715227/b393/agentdrain.test GOINSECURE GOMOD GOMODCACHE 1715227/b393/agentdrain.test e=/t�� t0 GO111MODULE(http block)/usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v7 --jq .object.sha 1757-30416/test-369925659 GO111MODULE ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile env 1715227/b417/_pkg_.a GO111MODULE 1/x64/bin/node GOINSECURE b/gh-aw/pkg/loggrev-parse GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v0.1.2/usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v0.1.2 --jq .object.sha --get remote.origin.url /usr/bin/git -json GO111MODULE nch,headSha,disp--show-toplevel git rev-�� --show-toplevel x_amd64/vet 1715227/b451/vet.cfg -json GO111MODULE 64/bin/go git(http block)https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.0.0/usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.0.0 --jq .object.sha --show-toplevel GOPROXY ache/node/24.14.1/x64/bin/node GOSUMDB GOWORK 64/bin/go git t-27�� bility_SameInputSameOutput3797585594/001/stabili-errorsas go ache/node/24.14.1/x64/bin/node 263080876/001' 263080876/001' 64/bin/go git(http block)https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.2.3/usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.2.3 --jq .object.sha --show-toplevel GOPROXY ache/node/24.14.1/x64/bin/node GOSUMDB GOWORK 64/bin/go git t-67�� bility_SameInputSameOutput3797585594/001/stability-test.md go /usr/bin/git u3Ug/Rz1hPeZdcb9git GO111MODULE 64/bin/go git(http block)https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/1/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 1271685257 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run download 1 --dir test-logs/run-1 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 3560970757 GO111MODULE ger.test GOINSECURE GOMOD GOMODCACHE ger.test(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12345/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env ithub/workflows o 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile(http block)/usr/bin/gh gh run download 12345 --dir test-logs/run-12345 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12346/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env ithub/workflows GO111MODULE x_amd64/link GOINSECURE GOMOD GOMODCACHE x_amd64/link(http block)/usr/bin/gh gh run download 12346 --dir test-logs/run-12346 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE S8eKncR/bXjFK1lrFoiYicFsYe-O env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/2/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 1271685257 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run download 2 --dir test-logs/run-2 GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE ylQP4Z8/vCNYLdc7D8RXanEmFBss env til.go til_test.go ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/3/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env 1271685257 GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet(http block)/usr/bin/gh gh run download 3 --dir test-logs/run-3 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile(http block)https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/4/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run download 4 --dir test-logs/run-4 GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env e-analyzer.md GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts/usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/5/artifacts --jq .artifacts[].name GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run download 5 --dir test-logs/run-5 GO111MODULE n-dir/bash GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/actions/workflows/usr/bin/gh gh workflow list --json name,state,path -c=4 -nolocalimports -importcfg /tmp/go-build2571715227/b411/importcfg -pack /home/REDACTED/work/gh-aw/gh-aw/pkg/fileutil/fileutil.go /home/REDACTED/work/gh-aw/gh-aw/pkg/fileutil/tar.go -c che/go-build/52/GOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc-trimpath(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100 scripts/**/*.js 64/bin/go go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6 GOMOD GOMODCACHE go env -json GO111MODULE ntdrain.test GOINSECURE GOMOD GOMODCACHE ntdrain.test(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v0.47.4/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v0.47.4 --jq .object.sha --show-toplevel x_amd64/vet /usr/bin/git -json .cfg ache/go/1.25.8/x--show-toplevel git rev-�� 848/001/go/1.25.0/x64"; export PATH="$(find "/tmp/TestGetNpmBinPathSetup_GorootOrdering148440884git go /usr/bin/git efaultBranchFromgit efaultBranchFromrev-list ache/go/1.25.8/x--count git(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq .object.sha edOutput3486139453/001 GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.2.3/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.2.3 --jq .object.sha -mod=readonly -e 64/bin/go npx prettier --wgit(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v2.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq .object.sha log.showsignatur-errorsas log 64/bin/go -d(http block)/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq .object.sha /tmp/go-build411-p -trimpath 64/bin/go -p main -lang=go1.25 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v3.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v3.0.0 --jq .object.sha(http block)https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999/usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq .object.sha edOutput3486139453/001 GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/nonexistent/repo/actions/runs/12345/usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/cgo env -json GO111MODULE ache/go/1.25.8/x64/bin/go GOINSECURE GOMOD GOMODCACHE go(http block)https://api.github.com/repos/owner/repo/actions/workflows/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo 64/bin/go GOINSECURE GOMOD GOMODCACHE 7588832/b403/impGO111MODULE -c che/go-build/a1/GOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcGOPROXY(http block)/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo -nolocalimports -importcfg /tmp/go-build2571715227/b415/importcfg -pack /tmp/go-build2571715227/b415/_testmain.go -c che/go-build/b7/GOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolctest@example.com(http block)https://api.github.com/repos/owner/repo/contents/file.md/tmp/go-build2571715227/b397/cli.test /tmp/go-build2571715227/b397/cli.test -test.testlogfile=/tmp/go-build2571715227/b397/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE sh(http block)https://api.github.com/repos/test-owner/test-repo/actions/secrets/usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name Htin/ORwXMWFyuJ5GOINSECURE GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE 7588832/b388/impGO111MODULE -c che/go-build/8e/GOINSECURE GOPROXY 64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc-test.v=true(http block)If you need me to access, download, or install something from one of these locations, you can either: