chore: Extract hard-coded file paths to constants (120 instances)#38774
chore: Extract hard-coded file paths to constants (120 instances)#38774Copilot wants to merge 4 commits into
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes hard-coded path string literals into named constants (primarily under pkg/constants/ and pkg/workflow/), addressing findings from the hardcodedfilepath linter and improving consistency across workflow compilation, parsing, and CLI logic.
Changes:
- Added
pkg/constants/path_constants.gowith shared.github/and/tmp/gh-aw/...path constants and updatedGetWorkflowDir()to returnDotGithubWorkflowsDir. - Introduced runner-side path constants in
pkg/workflow/setup_action_paths.go(shell vs GitHub Actions expression forms) and updated workflow rendering callsites to use them. - Updated many callsites across
pkg/workflow/,pkg/parser/,pkg/cli/, and a compiled.lock.ymlto replace inline path literals with constants (plus a self-referential fix in thehardcodedfilepathlinter).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/threat_detection.go | Replaces threat-detection log path literal with a shared constant. |
| pkg/workflow/step_order_validation.go | Uses shared constants for /tmp/gh-aw/ and runner temp path prefix checks. |
| pkg/workflow/setup_action_paths.go | Adds runner-side path constants (shell vs GHA expression forms). |
| pkg/workflow/safe_outputs_steps.go | Uses constants.TmpGhAwDir for artifact download path. |
| pkg/workflow/safe_outputs_config_generation.go | Uses shared repo-memory base dir constant when generating config. |
| pkg/workflow/safe_jobs.go | Switches safe-jobs download/output paths to workflow-level constants. |
| pkg/workflow/runtime_import_validation.go | Uses .github/ prefix constant when normalizing import paths. |
| pkg/workflow/repo_memory.go | Uses shared repo-memory base dir constant when building steps and artifact paths. |
| pkg/workflow/pi_engine.go | Replaces prompt/models.json-related /tmp/gh-aw/... literals with constants. |
| pkg/workflow/opencode_mcp.go | Uses shared MCP servers config path constant. |
| pkg/workflow/opencode_engine.go | Uses shared prompt path constant in command/env wiring. |
| pkg/workflow/mcp_renderer_builtin.go | Uses runner-side gh-aw binary path constant for release-mode entrypoint. |
| pkg/workflow/mcp_environment.go | Uses shared MCP config dir constant for CODEX_HOME. |
| pkg/workflow/mcp_config_playwright_renderer.go | Uses MCP logs directory constants for output-dir and mounts. |
| pkg/workflow/gemini_mcp.go | Uses runner-side MCP servers config path constant (shell form). |
| pkg/workflow/gemini_logs.go | Uses shared agent stdio log path constant. |
| pkg/workflow/gemini_engine.go | Uses shared prompt path and error glob constants. |
| pkg/workflow/frontmatter_extraction_metadata.go | Uses .github/ prefix constant when constructing local source URLs. |
| pkg/workflow/engine_output.go | Uses shared /tmp/gh-aw/ base dir constant for workspace-vs-tmp filtering. |
| pkg/workflow/crush_mcp.go | Uses shared MCP servers config path constant. |
| pkg/workflow/crush_engine.go | Uses shared prompt path constant in command/env wiring. |
| pkg/workflow/copilot_logs.go | Uses shared sandbox logs dir constant (with explicit trailing slash at return site). |
| pkg/workflow/copilot_engine.go | Uses .github/ prefix constant for manifest path prefixes. |
| pkg/workflow/copilot_engine_execution.go | Uses shared prompt/tmp constants and introduces an AWF copilot binary path const. |
| pkg/workflow/compiler_yaml.go | Uses .github/ prefix constant in path normalization logic. |
| pkg/workflow/compiler_yaml_main_job.go | Replaces multiple /tmp/gh-aw/... artifact path literals with constants/globs. |
| pkg/workflow/compiler_safe_outputs_job.go | Uses constants.TmpGhAwDir for artifact download paths. |
| pkg/workflow/compiler_main_job.go | Uses MCP safe-outputs logs dir constant for GH_AW_MCP_LOG_DIR. |
| pkg/workflow/compiler_difc_proxy.go | Uses proxy/TLS constants for env vars and proxy log collection base path. |
| pkg/workflow/codex_mcp.go | Uses runner-side MCP servers config path constant (shell form). |
| pkg/workflow/codex_engine.go | Uses shared prompt/MCP config constants for env wiring. |
| pkg/workflow/claude_mcp.go | Uses runner-side MCP servers config path constant (shell form). |
| pkg/workflow/claude_engine.go | Uses shared prompt and runner-side MCP config path constant (GHA expr form). |
| pkg/workflow/awf_helpers.go | Uses runner temp root and staging dir constants in AWF argument/mount generation. |
| pkg/workflow/antigravity_mcp.go | Uses runner-side MCP servers config path constant (shell form). |
| pkg/workflow/antigravity_logs.go | Uses shared agent stdio log path constant. |
| pkg/workflow/antigravity_engine.go | Uses shared prompt path and error glob constants. |
| pkg/workflow/agentic_engine.go | Uses shared default agent stdio log path constant. |
| pkg/parser/remote_fetch.go | Uses .github/workflows and .github/agents constants for path classification/security checks. |
| pkg/parser/mcp.go | Uses MCP logs dir constant for Playwright docker volume wiring. |
| pkg/parser/include_expander.go | Uses .github/ prefix constant when normalizing include/import paths. |
| pkg/parser/import_field_extractor.go | Uses .github/ prefix constant when computing import relative paths. |
| pkg/linters/hardcodedfilepath/hardcodedfilepath.go | Fixes self-flagging by extracting pathPrefixes entries to package-level consts. |
| pkg/constants/path_constants.go | Introduces shared .github/ and /tmp/gh-aw/... path constants. |
| pkg/constants/constants.go | Updates GetWorkflowDir() default to use DotGithubWorkflowsDir. |
| pkg/cli/trial_repository.go | Uses shared workflows directory constant when compiling trial workflows. |
| pkg/cli/shell_completion.go | Extracts OS-specific completion paths to file-level constants. |
| pkg/cli/mcp_tools_privileged.go | Uses shared agentic-workflows MCP logs dir constant for output paths. |
| pkg/cli/logs_run_processor.go | Uses shared workflows directory constant when inferring workflow paths. |
| pkg/cli/includes.go | Uses .github/ prefix constant when resolving shared includes. |
| pkg/cli/git.go | Uses shared workflows dir constants for staging and gitattributes entries. |
| pkg/cli/firewall_policy.go | Uses constants.AWFAuditDir to avoid hard-coded audit label suffix. |
| pkg/cli/fetch.go | Uses workflows dir constant in remote fetch fallback prefixes. |
| pkg/cli/experiments_command.go | Uses workflows dir constant when building remote API paths. |
| pkg/cli/audit.go | Uses .github/ prefix constant when resolving workflow display names. |
| pkg/cli/audit_comparison.go | Uses .github/ prefix constant when resolving workflow display names. |
| pkg/cli/add_package_manifest.go | Uses .github/ and workflows dir constants for package path validation/resolution. |
| .github/workflows/smoke-copilot-aoai-entra.lock.yml | Replaces hard-coded /home/runner/... with $HOME-based paths and moves related env to exports within the step script. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 58/58 changed files
- Comments generated: 2
| return []string{ | ||
| "/tmp/gh-aw/proxy-logs/", | ||
| constants.ProxyLogsDir + "/", | ||
| "!/tmp/gh-aw/proxy-logs/proxy-tls/", |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. No test files were added or modified in PR #38774. This PR only extracts hard-coded file paths to constants across production Go source files — no test files (_test.go, .test.cjs, .test.js) were touched. Test Quality Sentinel analysis skipped. |
✅ Design Decision Gate — ADR VerifiedADR reviewed: ADR-38742: Enforce File-Path Constant Usage via a Custom Linter — this PR faithfully implements the recorded decision. No divergence found. 🏗️
📋 Verification SummaryADR-38742 decided to enforce file-path constants mechanically via the custom
No code contradicts the stated decision and no architectural scope creep was observed. 🔗 Recommended: link the ADR in the PR bodyThe governing ADR is on the branch but is not referenced in the PR description. Consider adding a line so the link is durable:
ADR-38742 is also currently
|
|
@copilot run pr-finisher skill |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out and /improve-codebase-architecture — no blocking issues; leaving observations and concrete improvement suggestions.
📋 Key Themes & Highlights
Themes
- Sub-path constants should compose from parent constants — Go constant expressions make this free; skipping it means a future rename of
TmpGhAwDirorMCPLogsDirsilently leaves 7+ sub-constants stale. - Inconsistent trailing-slash policy —
DotGithubDircarries a trailing slash butDotGithubWorkflowsDirandDotGithubAgentsDirdo not, creating a per-constant memory burden for callers. pathPrefixDotGithubin the linter duplicatesconstants.DotGithubDir— two independent definitions of the same string will drift.- Undocumented behavioral change in the lock file —
/home/runner→$HOMEandXDG_CONFIG_HOME/GH_AW_MCP_CONFIGmoving from the static env block to inline exports is a portability improvement, but is not mentioned in the PR description. - Call-site trailing-slash tax —
constants.X + "/"repeated across 7+ call sites; the constants package could own this pattern.
Positive Highlights
- ✅ The self-referential linter fix (
pathPrefixRunnerTempShelletc. as package-level consts) is clever and correct. - ✅ The new constants file is well-structured with clear doc comments and logical grouping by category.
- ✅ 58 files mechanically transformed with no logic changes — the blast radius is wide but the transformation is clean.
- ✅ The
/home/runner→$HOMEportability improvement in the lock file is a real correctness win for non-standard runners. - ✅
setup_action_paths.gocleanly separates runner-side (shell + GHA-expr forms) from container-side constants.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 517.8 AIC · ⌖ 13.9 AIC · ⊞ 27.8K
| const MCPLogsDir = "/tmp/gh-aw/mcp-logs" | ||
|
|
||
| // MCPLogsSafeOutputsDir is the directory for safe-outputs MCP server logs. | ||
| const MCPLogsSafeOutputsDir = "/tmp/gh-aw/mcp-logs/safeoutputs" |
There was a problem hiding this comment.
[/improve-codebase-architecture] MCPLogsSafeOutputsDir and other sub-path constants duplicate the /tmp/gh-aw prefix as a literal — if TmpGhAwDir or MCPLogsDir is ever renamed, these sub-constants silently become stale.
💡 Go constant expressions allow safe composition
Go string constants can be concatenated at compile time:
const MCPLogsSafeOutputsDir = MCPLogsDir + "/safeoutputs"
const MCPLogsPlaywrightDir = MCPLogsDir + "/playwright"
const MCPConfigLogsDir = MCPConfigDir + "/logs"
const MCPConfigServersFilePath = MCPConfigDir + "/mcp-servers.json"
const SandboxAgentLogsDir = TmpGhAwDir + "/sandbox/agent/logs"
const ProxyLogsTLSCACertPath = ProxyLogsDir + "/proxy-tls/ca.crt"
const ThreatDetectionLogPath = TmpGhAwDir + "/threat-detection/detection.log"This is valid Go — the compiler evaluates these as constant expressions, so there is no runtime cost and any rename propagates automatically to all derivatives.
|
|
||
| // DotGithubDir is the repository-relative path to the .github directory (with trailing slash). | ||
| // Use for prefix comparisons and directory operations. | ||
| const DotGithubDir = ".github/" |
There was a problem hiding this comment.
[/zoom-out] DotGithubDir includes a trailing slash (".github/") while DotGithubWorkflowsDir and DotGithubAgentsDir do not. This asymmetry forces callers to know which constants need an appended "/" and which already include it, making subtle double-slash bugs easy to introduce.
💡 Pick one convention and apply it consistently
Option A — remove the trailing slash from DotGithubDir (matches the other two):
const DotGithubDir = ".github"Callers that do strings.HasPrefix(p, constants.DotGithubDir) would use constants.DotGithubDir + "/" — the same pattern already used for DotGithubWorkflowsDir.
Option B — add trailing slash to all three:
const DotGithubDir = ".github/"
const DotGithubWorkflowsDir = ".github/workflows/"
const DotGithubAgentsDir = ".github/agents/"This removes the append-"/" burden from all call sites.
Either is fine; the key is applying one policy uniformly so readers do not need to check each constant before using it.
| const ( | ||
| pathPrefixRunnerTempShell = "${RUNNER_TEMP}/" | ||
| pathPrefixRunnerTempGHAExpr = "${{ runner.temp }}/" | ||
| pathPrefixDotGithub = ".github/" |
There was a problem hiding this comment.
[/zoom-out] pathPrefixDotGithub = ".github/" is a second independent definition of the same string already exported as constants.DotGithubDir. If the .github directory prefix is ever changed in the constants package, the linter will silently continue checking the old prefix while production code uses the updated value.
💡 Options to remove the duplication
Option A — import pkg/constants and reference it directly (no circular dependency; the linter package imports are analysis-only):
const pathPrefixDotGithub = constants.DotGithubDirOption B — if keeping the linter self-contained is a hard requirement, add a test that asserts:
func TestPathPrefixMatchesConstants(t *testing.T) {
assert.Equal(t, constants.DotGithubDir, pathPrefixDotGithub)
}This fails the build if the two definitions drift.
| trap 'rm -f "$HOME/.copilot/settings.json"' EXIT | ||
| mkdir -p "$HOME/.copilot" | ||
| printf '%s' '{"builtInAgents":{"rubberDuck":false}}' > "$HOME/.copilot/settings.json" | ||
| export XDG_CONFIG_HOME="$HOME" |
There was a problem hiding this comment.
[/zoom-out] This diff contains a behavioral change beyond constant extraction: XDG_CONFIG_HOME is moved from a static env: block value (/home/runner) to an inline export XDG_CONFIG_HOME="$HOME", and GH_AW_MCP_CONFIG is moved similarly. On enterprise or container runners where $HOME != /home/runner, this changes where the Copilot CLI resolves its config directory.
The change looks correct and makes the compiled workflow more portable, but it is not mentioned in the PR description. A one-line note in the description (e.g. "also makes Copilot config paths $HOME-relative for portability") would help future git blame readers distinguish intentional behavior changes from mechanical refactors.
|
|
||
| // Collect MCP logs. | ||
| paths = append(paths, "/tmp/gh-aw/mcp-logs/") | ||
| paths = append(paths, constants.MCPLogsDir+"/") |
There was a problem hiding this comment.
[/improve-codebase-architecture] constants.MCPLogsDir + "/" appears seven times in collectArtifactPaths alone. The "no trailing slash" policy on directory constants creates a recurring call-site tax — every caller must remember to append "/", and a missed append silently breaks artifact upload by pointing at a file instead of a directory.
💡 Two options to remove the burden
Option A — directory-form companion constants (valid Go const expressions):
const MCPLogsDirSlash = MCPLogsDir + "/"
const AgentDirSlash = AgentDir + "/"
const TmpGhAwDirSlash = TmpGhAwDir + "/"Or simply rename the existing constants to always include the slash for directory-only uses.
Option B — a small helper:
// Dir returns path with a guaranteed trailing slash.
func Dir(path string) string { return strings.TrimRight(path, "/") + "/" }This makes the intent self-documenting at the call site: constants.Dir(constants.MCPLogsDir).
| } else { | ||
| // Original args for non-sandbox mode | ||
| copilotArgs = []string{"--add-dir", "/tmp/", "--add-dir", "/tmp/gh-aw/", "--add-dir", "/tmp/gh-aw/agent/", "--log-level", "all", "--log-dir", logsFolder} | ||
| copilotArgs = []string{"--add-dir", "/tmp/", "--add-dir", constants.TmpGhAwDir + "/", "--add-dir", constants.AgentDir + "/", "--log-level", "all", "--log-dir", logsFolder} |
There was a problem hiding this comment.
[/zoom-out] "/tmp/" remains as the only hardcoded path literal on this line alongside two freshly-extracted constants.* references. It is exempt from the linter (too short), but the asymmetry is noticeable. A brief inline comment explaining why /tmp/ is intentionally left as a literal (e.g. // /tmp/ is the host root, not gh-aw-specific) would make this clearer for future readers and distinguish it from an oversight.
…tant Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 5b3b251:
|
120+ inline path string literals were scattered across
pkg/workflow/,pkg/parser/, andpkg/cli/, flagged by the customhardcodedfilepathlinter. This PR extracts them all to named constants, eliminating the findings.New constants
pkg/constants/path_constants.go(new file) — 26 constants grouped by category:.github/paths:DotGithubDir,DotGithubWorkflowsDir,DotGithubAgentsDir/tmp/gh-aw/paths:TmpGhAwDir,AgentStdioLogPath,AgentPromptFilePath,MCPLogsDir,MCPConfigDir,MCPConfigServersFilePath,RepoMemoryDir,ThreatDetectionLogPath,AwPatchGlob,AwBundleGlob, etc.pkg/workflow/setup_action_paths.go— 5 new runner-side constants (shell/GHA-expr variants for MCP config path, safe-jobs dir, upload-artifacts staging dir).Self-referential linter fix
pkg/linters/hardcodedfilepath/hardcodedfilepath.go— thepathPrefixesvar was flagging itself. Extracted the three affected prefix strings as unexported package-levelconsts (which the linter skips).Callsite updates
~50 files updated to use constants instead of literals. Representative before/after:
Platform-specific paths in
pkg/cli/shell_completion.go(/opt/homebrew,/usr/local,/etc/bash_completion.d) are defined as file-level constants since they have no natural home in the shared constants package.