Replace hard-coded activation symlink path literals with named/shared path constants - #44177
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (actual: 34 additions across 2 files). |
There was a problem hiding this comment.
Pull request overview
This PR removes hard-coded .github/... activation symlink candidate path literals from the activation job compiler logic by replacing them with named, shared-constant-derived path variables, and extends unit tests to cover additional symlinked directories.
Changes:
- Introduced named activation symlink candidate path variables derived from
constants.GithubDir. - Updated
resolveSymlinkExtraPathsto build its candidate list from those variables instead of inline string literals. - Extended
TestResolveSymlinkExtraPathswith new cases covering.github/skillsand.github/promptssymlink resolution.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_activation_job.go | Replaces inline .github/... candidate literals with named activation symlink path variables derived from shared constants. |
| pkg/workflow/compiler_activation_job_test.go | Adds targeted tests to ensure .github/skills and .github/prompts symlinks are resolved and appended to sparse-checkout extra paths. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
🧪 Test Quality Sentinel Report
📊 Metrics (2 tests)
Verdict✅ Passed. 0% implementation tests (threshold: 30%). No violations. Strengths:
Notes:
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![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 /codebase-design — requesting changes on two maintainability issues before merge.
📋 Key Themes & Highlights
Issues
-
Duplicate source of truth (
compiler_activation_job.goline 19):constants.AgentsDiralready encodes.github/agents/; the newactivationSymlinkAgentDirrecomputes the same value independently. Consider usingstrings.TrimSuffix(constants.AgentsDir, "/")and adding sibling constants (SkillsDir,PromptsDir) topkg/constantsfor the other two paths. -
varinstead ofconst(compiler_activation_job.golines 18–23): path values are immutable;varallows mutation and misleads readers. If the three sub-dirs are derived from proper constants, they can be expressed asconst(nostrings.TrimSuffixneeded).
Positive Highlights
- ✅ Clean, narrow scope — only the relevant candidate-path literals were touched
- ✅ New test cases for
skillsandpromptsfollow the exact same structure as the existingagentstest, making them easy to read and maintain - ✅
t.Skipfguard for environments that don't support symlinks is a nice defensive touch
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 42.4 AIC · ⌖ 5.21 AIC · ⊞ 6.6K
Comment /matt to run again
| var compilerActivationJobLog = logger.New("workflow:compiler_activation_job") | ||
|
|
||
| var ( | ||
| activationSymlinkBaseDir = strings.TrimSuffix(constants.GithubDir, "/") |
There was a problem hiding this comment.
[/codebase-design] activationSymlinkAgentDir duplicates the already-existing constants.AgentsDir — this creates two sources of truth for .github/agents.
pkg/constants/constants.go already has const AgentsDir = ".github/agents/". Deriving activationSymlinkAgentDir independently means a future rename of AgentsDir won't automatically propagate here.
💡 Suggested fix
Replace the agent-dir line with:
activationSymlinkAgentDir = strings.TrimSuffix(constants.AgentsDir, "/")(Consider also adding SkillsDir and PromptsDir constants to pkg/constants/ and using them in the same way, so all three paths share one source of truth.)
@copilot please address this.
|
|
||
| var compilerActivationJobLog = logger.New("workflow:compiler_activation_job") | ||
|
|
||
| var ( |
There was a problem hiding this comment.
[/codebase-design] These new vars should be const (computed at init from other constants), not var — var allows mutation and signals the values might change.
Since constants.GithubDir is a const and all the derived paths are pure string concatenations, all four could be defined as constants (or, if the strings.TrimSuffix call prevents that, at minimum the three sub-path vars could be const once the base is established). Using var for immutable configuration values reduces readability and opens the door to accidental modification in tests.
💡 Suggested fix
If constants.AgentsDir, SkillsDir, and PromptsDir are added to pkg/constants, the entire block reduces to three const references and the var block disappears entirely.
@copilot please address this.
There was a problem hiding this comment.
The refactoring is clean and well-scoped. One suggestion: since constants.GithubDir already ends with /, the four new var declarations can be simplified to true compile-time const values (no strings.TrimSuffix needed). See the inline comment for the suggested change. No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 49.9 AIC · ⌖ 5.83 AIC · ⊞ 4.8K
| var compilerActivationJobLog = logger.New("workflow:compiler_activation_job") | ||
|
|
||
| var ( | ||
| activationSymlinkBaseDir = strings.TrimSuffix(constants.GithubDir, "/") |
There was a problem hiding this comment.
var mutability: consider const instead
The four new package-level variables are declared as var, making them mutable and subject to init-order dependencies. Since constants.GithubDir = ".github/" already ends with a slash, strings.TrimSuffix is unnecessary — the subdirectory paths can be expressed as true compile-time constants:
const (
activationSymlinkAgentDir = constants.GithubDir + "agents"
activationSymlinkSkillDir = constants.GithubDir + "skills"
activationSymlinkPromptDir = constants.GithubDir + "prompts"
)This removes the strings.TrimSuffix call, the activationSymlinkBaseDir intermediate, and makes the values immutable. The resulting strings (".github/agents", etc.) are identical to the current runtime values.
@copilot please address this.
There was a problem hiding this comment.
Two issues block merge
1. Runtime work on a compile-time constant (line 19): strings.TrimSuffix(constants.GithubDir, "/") is called at package-init time on a value already known at compile time. Go string constants support concatenation; the entire block should be const, removing the strings call and the intermediate activationSymlinkBaseDir variable.
2. Divergent source of truth for the agents path (line 20): constants.AgentsDir = ".github/agents/" already exists, but activationSymlinkAgentDir re-derives the same path from scratch (".github/agents", no trailing slash). If AgentsDir is ever renamed or restructured, the activation path will silently stay behind. Skills and prompts have no exported constant yet — this is the right moment to add them to constants.go and derive from there, completing the symmetry already established by AgentsDir.
🔎 Code quality review by PR Code Quality Reviewer · 179.8 AIC · ⌖ 7.56 AIC · ⊞ 5.4K
Comment /review to run again
| var compilerActivationJobLog = logger.New("workflow:compiler_activation_job") | ||
|
|
||
| var ( | ||
| activationSymlinkBaseDir = strings.TrimSuffix(constants.GithubDir, "/") |
There was a problem hiding this comment.
var doing runtime work on a compile-time constant: strings.TrimSuffix(constants.GithubDir, "/") is a runtime call on a fully-known compile-time constant, and the whole block uses var (mutable) when const would work.
💡 Suggested fix
Go string constants support concatenation at compile time:
const (
activationSymlinkAgentDir = constants.GithubDir + "agents" // .github/agents
activationSymlinkSkillDir = constants.GithubDir + "skills" // .github/skills
activationSymlinkPromptDir = constants.GithubDir + "prompts" // .github/prompts
)This removes the strings.TrimSuffix call, eliminates the intermediate activationSymlinkBaseDir variable (which has no other use), makes all three values immutable, and still derives from constants.GithubDir. GithubDir is ".github/", so GithubDir+"agents" → ".github/agents" — identical output.
|
|
||
| var ( | ||
| activationSymlinkBaseDir = strings.TrimSuffix(constants.GithubDir, "/") | ||
| activationSymlinkAgentDir = activationSymlinkBaseDir + "/agents" |
There was a problem hiding this comment.
Partial reuse of constants.AgentsDir: constants.AgentsDir = ".github/agents/" already exists in constants.go, but activationSymlinkAgentDir is re-derived as ".github/agents" (no trailing slash). These two paths describe the same directory; now they can diverge silently if AgentsDir is updated.
💡 Detail
constants.AgentsDir has a trailing slash (".github/agents/") while activationSymlinkAgentDir has none (".github/agents"). The different forms exist for different callers (prefix matching vs. path joining), but since there is already an established pattern of exporting path constants from constants.go, activationSymlinkSkillDir and activationSymlinkPromptDir should also be exported from constants.go as SkillsDir/PromptsDir to complete the symmetry — not re-derived inline.
At minimum, activationSymlinkAgentDir should be derived from strings.TrimSuffix(constants.AgentsDir, "/") rather than computed from scratch so there is a single source of truth for the agents path.
|
@copilot run pr-finisher skill |
…attern Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in the latest commit: converted the |
|
🎉 This pull request is included in a new release. Release: |
LintMonster flagged hard-coded
.github/...activation paths inpkg/workflow/compiler_activation_job.go. This change removes those inline literals and makes activation symlink candidate paths constant-driven to align with existing path-constant patterns.Scope: activation symlink candidate paths only
resolveSymlinkExtraPathswith named variables:.github/agents.github/skills.github/promptsconstants.GithubDir) to avoid embedding well-known repo paths directly in activation logic.Implementation details
compiler_activation_job.go:activationSymlinkBaseDiractivationSymlinkAgentDiractivationSymlinkSkillDiractivationSymlinkPromptDirFocused coverage updates
TestResolveSymlinkExtraPathswith explicit cases for:.github/skillssymlink target resolution.github/promptssymlink target resolution