Skip to content

Replace lint-monster path literals with repository constants in CLI/workflow codepaths #41611

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.

Sign up for GitHub

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

Merged
pelikhan merged 2 commits into from
Jun 26, 2026
Merged

Replace lint-monster path literals with repository constants in CLI/workflow codepaths #41611

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ce0607
Initial plan
Copilot Jun 26, 2026
1ef58e9
Replace hard-coded workflow paths with constants
Copilot Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions pkg/cli/add_package_manifest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (

"github.com/goccy/go-yaml"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/semverutil"
Expand All @@ -30,7 +31,7 @@ var getRepositoryPackageDefaultBranch = resolveRepositoryPackageDefaultBranch
var getRepositoryPackageLatestRelease = resolveRepositoryPackageLatestRelease
var addPackageManifestLog = logger.New("cli:add_package_manifest")

var packageSourceDirectories = []string{"workflows", ".github/workflows"}
var packageSourceDirectories = []string{"workflows", constants.WorkflowsDir}

const repositoryPackageManifestFileName = "aw.yml"
const repositoryPackageManifestVersion = "1"
Expand Down Expand Up @@ -496,27 +497,27 @@ func isSupportedManifestIncludePath(p string) bool {

func isSupportedSkillDirectoryPrefix(cleaned string) bool {
return strings.HasPrefix(cleaned, packageSkillsDirectory+"/") ||
strings.HasPrefix(cleaned, ".github/"+packageSkillsDirectory+"/")
strings.HasPrefix(cleaned, constants.GithubDir+packageSkillsDirectory+"/")

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/improve-codebase-architecture] The expressions constants.GithubDir + packageSkillsDirectory and constants.GithubDir + packageAgentsDirectory are now repeated 3–4 times each. Consider hoisting them into package-level vars to reduce silent coupling.

💡 Suggested refactor

Add two vars near the existing constants:

// Derived from constants.GithubDir; centralises the composition rule.
var githubSkillsDir = constants.GithubDir + packageSkillsDirectory // ".github/skills"
var githubAgentsDir = constants.GithubDir + packageAgentsDirectory // ".github/agents"

Then use githubSkillsDir / githubAgentsDir throughout isSupportedSkillDirectoryPrefix, skillDirectoryRoot, scanPackageSkillDirs, etc.

If GithubDir ever changes, there would be one correction point instead of eight.

}

func skillDirectoryRoot(cleaned string) string {
switch {
case strings.HasPrefix(cleaned, ".github/"+packageSkillsDirectory+"/"):
return ".github/" + packageSkillsDirectory
case strings.HasPrefix(cleaned, constants.GithubDir+packageSkillsDirectory+"/"):
return constants.GithubDir + packageSkillsDirectory
default:
return packageSkillsDirectory
}
}

func isSupportedAgentDirectoryPrefix(cleaned string) bool {
return strings.HasPrefix(cleaned, packageAgentsDirectory+"/") ||
strings.HasPrefix(cleaned, ".github/"+packageAgentsDirectory+"/")
strings.HasPrefix(cleaned, constants.GithubDir+packageAgentsDirectory+"/")
}

func agentDirectoryRoot(cleaned string) string {
switch {
case strings.HasPrefix(cleaned, ".github/"+packageAgentsDirectory+"/"):
return ".github/" + packageAgentsDirectory
case strings.HasPrefix(cleaned, constants.GithubDir+packageAgentsDirectory+"/"):
return constants.GithubDir + packageAgentsDirectory
default:
return packageAgentsDirectory
}
Expand Down Expand Up @@ -620,7 +621,7 @@ func resolvePackageAgentFiles(owner, repo, packagePath, ref, host string, explic
}

var agentFiles []string
for _, root := range []string{packageAgentsDirectory, ".github/" + packageAgentsDirectory} {
for _, root := range []string{packageAgentsDirectory, constants.GithubDir + packageAgentsDirectory} {
agentsDir := joinRepositoryPackagePath(packagePath, root)
files, err := listPackageDirFilesForHost(owner, repo, ref, agentsDir, host)
if err != nil {
Expand All @@ -642,7 +643,7 @@ func resolvePackageAgentFiles(owner, repo, packagePath, ref, host string, explic
// of skill subdirectories (those that contain a SKILL.md file).
func scanPackageSkillDirs(owner, repo, packagePath, ref, host string) ([]string, error) {
var skillDirs []string
for _, root := range []string{packageSkillsDirectory, ".github/" + packageSkillsDirectory} {
for _, root := range []string{packageSkillsDirectory, constants.GithubDir + packageSkillsDirectory} {
skillsDir := joinRepositoryPackagePath(packagePath, root)
subdirs, err := listPackageDirSubdirsForHost(owner, repo, ref, skillsDir, host)
if err != nil {
Expand Down Expand Up @@ -755,14 +756,14 @@ func isSupportedPackageInstallablePath(p string) bool {
if strings.HasSuffix(lowerCleaned, ".md") {
return strings.HasPrefix(cleaned, "workflows/") ||
strings.HasPrefix(cleaned, "agentic-workflows/") ||
strings.HasPrefix(cleaned, ".github/workflows/")
strings.HasPrefix(cleaned, constants.WorkflowsDirSlash)

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/zoom-out] Good — constants.WorkflowsDirSlash is now used correctly here. Two sibling prefixes on lines 757–758 ("workflows/" and "agentic-workflows/") are still raw string literals. The cleanup is consistent for .github/workflows/ but not for the alternate path roots.

💡 Options

Either:

  • Define const WorkflowsAltDir = "workflows/" and const AgenticWorkflowsDir = "agentic-workflows/" in pkg/constants so all three prefix checks are driven by constants, or
  • Add a // TODO: add constants for "workflows/" and "agentic-workflows/" alt paths comment to track the remaining cleanup.

The current mix of constants and literals at the same call site makes it easy to miss if one prefix needs updating.

}
if isActionWorkflowPath(cleaned) {
if !strings.HasPrefix(cleaned, ".github/workflows/") {
if !strings.HasPrefix(cleaned, constants.WorkflowsDirSlash) {
return false
}
// Reject nested subdirectories: only direct children of .github/workflows/ are allowed.
remaining := strings.TrimPrefix(cleaned, ".github/workflows/")
remaining := strings.TrimPrefix(cleaned, constants.WorkflowsDirSlash)
return !strings.Contains(remaining, "/")
}
return false
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/audit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1154,7 +1154,7 @@ func fetchWorkflowRunMetadata(ctx context.Context, runID int64, owner, repo, hos
// When the GitHub API returns the workflow file path as the run's name (e.g. for runs
// that were cancelled or failed before any jobs started), resolve the actual workflow
// display name so that audit output is consistent with 'gh aw logs'.
if strings.HasPrefix(run.WorkflowName, ".github/") {
if strings.HasPrefix(run.WorkflowName, constants.GithubDir) {
if displayName := resolveWorkflowDisplayName(ctx, run.WorkflowPath, owner, repo, hostname); displayName != "" {
auditLog.Printf("Resolved workflow display name: %q -> %q", run.WorkflowName, displayName)
run.WorkflowName = displayName
Expand Down
3 changes: 2 additions & 1 deletion pkg/cli/audit_comparison.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sort"
"strings"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/workflow"
)
Expand Down Expand Up @@ -513,7 +514,7 @@ func findPreviousSuccessfulWorkflowRuns(ctx context.Context, current WorkflowRun
}

for index := range runs {
if strings.HasPrefix(runs[index].WorkflowName, ".github/") {
if strings.HasPrefix(runs[index].WorkflowName, constants.GithubDir) {
if displayName := resolveWorkflowDisplayName(ctx, runs[index].WorkflowPath, owner, repo, hostname); displayName != "" {
runs[index].WorkflowName = displayName
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/cli/experiments_command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func loadRemoteExperimentConfigs(repoOverride, experimentName string) map[string
}

for _, candidate := range candidates {
apiPath := ".github/workflows/" + candidate + ".md"
apiPath := constants.WorkflowsDirSlash + candidate + ".md"
args := []string{"api",
"repos/{owner}/{repo}/contents/" + url.PathEscape(apiPath),
"--jq", ".content",
Expand Down
3 changes: 2 additions & 1 deletion pkg/cli/fetch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"time"

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/parser"
"github.com/github/gh-aw/pkg/stringutil"
Expand Down Expand Up @@ -122,7 +123,7 @@ func fetchRemoteWorkflow(ctx context.Context, spec *WorkflowSpec, verbose bool)
// Try with common workflow directory prefixes if the direct path fails.
// This handles short workflow names without path separators (e.g. "my-workflow.md").
if !strings.HasPrefix(spec.WorkflowPath, "workflows/") && !strings.Contains(spec.WorkflowPath, "/") {
for _, prefix := range []string{"workflows/", ".github/workflows/"} {
for _, prefix := range []string{"workflows/", constants.WorkflowsDirSlash} {
altPath := prefix + spec.WorkflowPath
if !strings.HasSuffix(altPath, ".md") {
altPath += ".md"
Expand Down
3 changes: 2 additions & 1 deletion pkg/cli/firewall_policy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"slices"
"strings"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/fileutil"
"github.com/github/gh-aw/pkg/logger"
)
Expand Down Expand Up @@ -464,7 +465,7 @@ func detectFirewallAuditArtifacts(runDir string) (manifestPath, auditJSONLPath s
if !checkDir(filepath.Join(agentDir, "sandbox", "firewall", "audit"), agentBase+"/sandbox/firewall/audit") {
// Old artifact structure (/tmp/gh-aw/ prefix preserved inside the artifact):
// <agentDir>/tmp/gh-aw/sandbox/firewall/audit/
checkDir(filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit"), agentBase+"/tmp/gh-aw/sandbox/firewall/audit")
checkDir(filepath.Join(agentDir, "tmp", "gh-aw", "sandbox", "firewall", "audit"), agentBase+constants.AWFAuditDir)

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantic mismatch: AWFAuditDir (absolute path) used as a relative label suffix — the label is correct only because the constant starts with /, which is incidental and fragile.

💡 Details

AWFAuditDir = "/tmp/gh-aw/sandbox/firewall/audit" is documented as an absolute runtime filesystem path. Here it serves as a string suffix in a display label:

agentBase + constants.AWFAuditDir   // → "<base>/tmp/gh-aw/sandbox/firewall/audit"

This produces the right output only because the constant starts with /, acting as a path separator. If AWFAuditDir is ever reorganized or changed to not start with /, this label silently computes a wrong string with no compile-time warning.

The old literal "/tmp/gh-aw/sandbox/firewall/audit" was unambiguous as an artifact-structure label. Consider keeping the literal in this context (artifact label ≠ live filesystem path), or introducing a dedicated constant for the artifact-internal path suffix.

}
if manifestPath != "" && auditJSONLPath != "" {
return manifestPath, auditJSONLPath, nil
Expand Down
4 changes: 2 additions & 2 deletions pkg/workflow/opencode_engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func (e *OpenCodeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile s
modelConfigured := workflowData.EngineConfig != nil && workflowData.EngineConfig.Model != ""

openCodeArgs = append(openCodeArgs, "--print-logs", "--log-level", "DEBUG")
promptArg := "\"$(cat /tmp/gh-aw/aw-prompts/prompt.txt)\""
promptArg := fmt.Sprintf("\"$(cat %s)\"", constants.AwPromptsFile)

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt.Sprintf is unnecessary for a constant-only substitution and sets a fragile shell-command pattern — simple concatenation avoids both the allocation and the implicit %s-in-shell-command footgun.

💡 Details

constants.AwPromptsFile is a compile-time constant, so fmt.Sprintf("\"\"", constants.AwPromptsFile) is equivalent to a string literal. The %s format verb here is problematic as a pattern: "" is a shell command template, and %s in shell context is a shell-injection point if the argument is ever made mutable.

The original literal was unambiguous — it was obviously a static string. The new form looks like it was designed to accept a variable path, which it wasn't.

Prefer:

promptArg := `""`

Or, if the escape-heavy form is needed for readability:

promptArg := "\"\""`

This makes the static nature explicit and removes the fmt import dependency for this callsite.


commandName := "opencode"
if workflowData.EngineConfig != nil && workflowData.EngineConfig.Command != "" {
Expand Down Expand Up @@ -173,7 +173,7 @@ func (e *OpenCodeEngine) GetExecutionSteps(workflowData *WorkflowData, logFile s
}

env := map[string]string{
"GH_AW_PROMPT": "/tmp/gh-aw/aw-prompts/prompt.txt",
"GH_AW_PROMPT": constants.AwPromptsFile,

@github-actions github-actions Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] AWFAuditDir has a spec-pin assertion in pkg/constants/spec_test.go that would catch a silent value change. AwPromptsFile, GithubDir, and WorkflowsDirSlash — all now load-bearing for path resolution in 7 files — lack equivalent assertions.

💡 Suggested additions to spec_test.go 
assert.Equal(t, ".github/", constants.GithubDir,
    "GithubDir must carry trailing slash — callers depend on direct concatenation")
assert.Equal(t, ".github/workflows/", constants.WorkflowsDirSlash,
    "WorkflowsDirSlash must end with '/' for HasPrefix path checks")
assert.Equal(t, "/tmp/gh-aw/aw-prompts/prompt.txt", constants.AwPromptsFile,
    "AwPromptsFile is wired into GH_AW_PROMPT and the opencode CLI arg")

This mirrors the existing AWFAuditDir test and ensures any accidental constant change surfaces immediately rather than failing silently in runtime path logic.

"GITHUB_WORKSPACE": "${{ github.workspace }}",
"RUNNER_TEMP": "${{ runner.temp }}",
"NO_PROXY": "localhost,127.0.0.1",
Expand Down
Loading