Replace lint-monster path literals with repository constants in CLI/workflow codepaths#41611
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. PR #41611 only modifies production Go files (pkg/cli/.go, pkg/workflow/.go) to replace lint-monster path literals with repository constants. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #41611 does not have the 'implementation' label and has only 23 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This pull request removes hard-coded repository/runtime path literals in several CLI and workflow engine codepaths, replacing them with shared values from pkg/constants to keep path semantics consistent (including trailing-slash behavior) and reduce drift across the codebase.
Changes:
- Swapped
.github/and.github/workflows/string literals forconstants.GithubDir,constants.WorkflowsDir, andconstants.WorkflowsDirSlashin CLI prefix checks and remote-path construction. - Replaced AWF/runtime
/tmp/gh-aw/...literals withconstants.AWFAuditDirandconstants.AwPromptsFile. - Updated package manifest path validation and root/prefix handling to use the shared constants.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/add_package_manifest.go | Replaces .github/... and workflows path literals with constants.GithubDir / constants.WorkflowsDirSlash in include/path validation and scanning. |
| pkg/cli/audit.go | Uses constants.GithubDir for workflow-name prefix detection when resolving display names. |
| pkg/cli/audit_comparison.go | Uses constants.GithubDir for workflow-name prefix detection in baseline run selection. |
| pkg/cli/experiments_command.go | Builds remote workflow content API paths using constants.WorkflowsDirSlash. |
| pkg/cli/fetch.go | Uses constants.WorkflowsDirSlash for fallback remote workflow path prefixes. |
| pkg/cli/firewall_policy.go | Uses constants.AWFAuditDir for the legacy artifact label/path suffix. |
| pkg/workflow/opencode_engine.go | Uses constants.AwPromptsFile for the prompt file path in both command construction and env wiring. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Low
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture, /zoom-out, and /tdd — approving with minor suggestions.
📋 Key Themes & Highlights
Key Themes
- Pattern repetition:
constants.GithubDir + packageSkillsDirectory/Agentsis constructed 8 times — package-level vars would DRY this up. - Partial cleanup:
"workflows/"and"agentic-workflows/"are still hardcoded literals alongside the now-constantWorkflowsDirSlashinisSupportedPackageInstallablePath. - Spec-test gap:
AWFAuditDirhas a spec-pin test;GithubDir,WorkflowsDirSlash, andAwPromptsFiledo not — worth adding given they're now relied on across 7 files.
Positive Highlights
- ✅ Correct use of
WorkflowsDirvsWorkflowsDirSlashin every context — trailing-slash semantics are fully preserved. - ✅ Scope is appropriately tight: only the flagged literals, no collateral changes.
- ✅ All substitutions are semantically equivalent; no behavioral drift.
- ✅
AwPromptsFileadoption inopencode_engine.gois clean — both the CLI arg and theGH_AW_PROMPTenv var are updated together.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.7 AIC · ⌖ 11.1 AIC · ⊞ 6.5K
| func isSupportedSkillDirectoryPrefix(cleaned string) bool { | ||
| return strings.HasPrefix(cleaned, packageSkillsDirectory+"/") || | ||
| strings.HasPrefix(cleaned, ".github/"+packageSkillsDirectory+"/") | ||
| strings.HasPrefix(cleaned, constants.GithubDir+packageSkillsDirectory+"/") |
There was a problem hiding this comment.
[/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.
| return strings.HasPrefix(cleaned, "workflows/") || | ||
| strings.HasPrefix(cleaned, "agentic-workflows/") || | ||
| strings.HasPrefix(cleaned, ".github/workflows/") | ||
| strings.HasPrefix(cleaned, constants.WorkflowsDirSlash) |
There was a problem hiding this comment.
[/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/"andconst AgenticWorkflowsDir = "agentic-workflows/"inpkg/constantsso all three prefix checks are driven by constants, or - Add a
// TODO: add constants for "workflows/" and "agentic-workflows/" alt pathscomment 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.
|
|
||
| env := map[string]string{ | ||
| "GH_AW_PROMPT": "/tmp/gh-aw/aw-prompts/prompt.txt", | ||
| "GH_AW_PROMPT": constants.AwPromptsFile, |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
All constant substitutions are semantically correct — values match their replaced literals exactly, imports are present where needed, and no functional behavior changes. Two non-blocking observations logged as inline comments.
Findings summary
Medium: AWFAuditDir semantic mismatch (firewall_policy.go:468)
AWFAuditDir is an absolute-path constant (/tmp/gh-aw/sandbox/firewall/audit). The label agentBase+constants.AWFAuditDir produces the right string only because the constant begins with /. The constant documents a runtime filesystem path, not an artifact-internal label fragment — these are different concepts that happen to share the same string value. If the constant ever changes to not start with /, the label silently computes wrong.
Low: fmt.Sprintf for constant-only shell command construction (opencode_engine.go:122)
fmt.Sprintf("\"\"", constants.AwPromptsFile) is unnecessary — AwPromptsFile is a compile-time constant. More importantly, %s inside $(cat ...) is a shell-injection pattern: if this is later extended to use a mutable path, the risk becomes real. Simple concatenation makes the static nature explicit.
Sub-agent note
The grumpy-coder first-pass agent did not complete within the timeout; second-pass analysis was performed independently.
🔎 Code quality review by PR Code Quality Reviewer · 143.7 AIC · ⌖ 7.04 AIC · ⊞ 5.2K
| // 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) |
There was a problem hiding this comment.
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.
|
|
||
| openCodeArgs = append(openCodeArgs, "--print-logs", "--log-level", "DEBUG") | ||
| promptArg := "\"$(cat /tmp/gh-aw/aw-prompts/prompt.txt)\"" | ||
| promptArg := fmt.Sprintf("\"$(cat %s)\"", constants.AwPromptsFile) |
There was a problem hiding this comment.
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.
|
Hey One thing that would strengthen this before merge:
If you'd like a hand, you can assign this prompt to your coding agent:
|
This change addresses the lint-monster path-constants cleanup by removing hard-coded repository/runtime paths in the flagged files and switching to existing
pkg/constantsvalues. Scope is limited to direct literal-to-constant substitutions while preserving path semantics (including trailing-slash behavior).Workflow/GitHub path constant adoption
pkg/cli/add_package_manifest.go.github/...and.github/workflows...fragments withconstants.GithubDir,constants.WorkflowsDir, andconstants.WorkflowsDirSlash.pkg/cli/audit.go,pkg/cli/audit_comparison.go.github/workflow-name prefix checks withconstants.GithubDir.pkg/cli/experiments_command.go,pkg/cli/fetch.go.github/workflows/construction/fallback prefixes withconstants.WorkflowsDirSlash.AWF/runtime tmp-path constant adoption
pkg/cli/firewall_policy.go/tmp/gh-aw/sandbox/firewall/auditlabel/path literal usage withconstants.AWFAuditDir.pkg/workflow/opencode_engine.go/tmp/gh-aw/aw-prompts/prompt.txtin both command and env wiring withconstants.AwPromptsFile.Representative change