Fix Glossary Maintainer workflow: Move documentation skill to .github folder and add compiler validation #14637
Conversation
|
@copilot do not use runtime import for the skill.md path outside of the .github folder. The go compiler should detect and enforce this rule: all runtime-imports must be under .github folder Error: Failed to process runtime import for ../../skills/documentation/SKILL.md: Security: Path ../../skills/documentation/SKILL.md must be within .github folder (resolves to: ../../skills/documentation/SKILL.md) |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in commit f6e76d1. Moved the documentation skill from |
|
@copilot update go compiler to detect invalid paths in runtime-imports |
…older Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added Go compiler validation in commits 4b6ec3a and a7a222a. The compiler now enforces the security constraint at two levels:
Both checks use |
There was a problem hiding this comment.
Pull request overview
Fixes agentic workflow compilation/runtime-import security violations by relocating the documentation skill into the .github tree and adding earlier validation so invalid runtime-import paths are caught before workflows run.
Changes:
- Move/copy
skills/documentation/SKILL.mdinto.github/skills/documentation/SKILL.mdand update workflow imports accordingly. - Regenerate the compiled
.lock.ymlworkflows so runtime-imports reference.github/skills/.... - Add Go-side security validation for runtime-import / include path resolution to prevent escaping the
.githubdirectory.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/expression_validation.go | Adds compile-time runtime-import path containment checks under .github. |
| pkg/parser/remote_fetch.go | Adds include-path resolution containment checks intended to prevent escaping .github. |
| .github/workflows/technical-doc-writer.md | Updates imports to reference ../skills/documentation/SKILL.md (within .github). |
| .github/workflows/technical-doc-writer.lock.yml | Regenerated compiled workflow to runtime-import .github/skills/documentation/SKILL.md. |
| .github/workflows/glossary-maintainer.md | Updates imports to reference ../skills/documentation/SKILL.md (within .github). |
| .github/workflows/glossary-maintainer.lock.yml | Regenerated compiled workflow to runtime-import .github/skills/documentation/SKILL.md. |
| .github/skills/documentation/SKILL.md | Adds a .github-scoped copy of the documentation skill content for runtime-import compatibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remoteLog.Printf("Security: Path escapes .github folder: %s (resolves to: %s)", filePath, relativePath) | ||
| return "", fmt.Errorf("Security: Path %s must be within .github folder (resolves to: %s)", filePath, relativePath) |
There was a problem hiding this comment.
If filepath.Rel returns an error, relativePath will be empty but the log/error message still prints it as the resolved location. Include the err and/or the normalized absolute paths in the message so the failure is actionable (and so cross-volume cases on Windows are understandable).
| remoteLog.Printf("Security: Path escapes .github folder: %s (resolves to: %s)", filePath, relativePath) | |
| return "", fmt.Errorf("Security: Path %s must be within .github folder (resolves to: %s)", filePath, relativePath) | |
| remoteLog.Printf( | |
| "Security: Path escapes .github folder: include=%s, base_dir=%s, github_folder=%s, full_path=%s, rel_path=%s, rel_error=%v", | |
| filePath, baseDir, normalizedGithubFolder, normalizedFullPath, relativePath, err, | |
| ) | |
| return "", fmt.Errorf( | |
| "security: path %q must be within .github folder (github_folder=%q, full_path=%q, rel_path=%q, rel_error=%v)", | |
| filePath, normalizedGithubFolder, normalizedFullPath, relativePath, err, | |
| ) |
| // Security check: ensure the resolved path is within the .github folder | ||
| // baseDir should be .github or a subdirectory within it | ||
| githubFolder := baseDir | ||
| // Find the .github folder by traversing up from baseDir | ||
| for !strings.HasSuffix(githubFolder, ".github") && githubFolder != "." && githubFolder != "/" { | ||
| githubFolder = filepath.Dir(githubFolder) | ||
| if githubFolder == "." || githubFolder == "/" { | ||
| // If we can't find .github folder, use baseDir | ||
| githubFolder = baseDir | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // Normalize paths for comparison | ||
| normalizedGithubFolder := filepath.Clean(githubFolder) | ||
| normalizedFullPath := filepath.Clean(fullPath) | ||
|
|
||
| // Check if fullPath is within githubFolder | ||
| relativePath, err := filepath.Rel(normalizedGithubFolder, normalizedFullPath) | ||
| if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { | ||
| remoteLog.Printf("Security: Path escapes .github folder: %s (resolves to: %s)", filePath, relativePath) | ||
| return "", fmt.Errorf("Security: Path %s must be within .github folder (resolves to: %s)", filePath, relativePath) | ||
| } |
There was a problem hiding this comment.
This new path-escape security behavior in ResolveIncludePath isn’t covered by unit tests. Add test cases (e.g., baseDir under .github/workflows with an import like ../../skills/... that should now error) and a regression test that the .github-search logic terminates on Windows-style roots.
| normalizedGithubFolder := filepath.Clean(githubFolder) | ||
| normalizedAbsolutePath := filepath.Clean(absolutePath) | ||
| relativePath, err := filepath.Rel(normalizedGithubFolder, normalizedAbsolutePath) | ||
| if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { |
There was a problem hiding this comment.
When the security check fails because filepath.Rel errors, the validation error currently reports an empty relativePath. Include the underlying err and/or the fully resolved normalizedAbsolutePath in the message so users can see what path was rejected and why.
| if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { | |
| if err != nil { | |
| validationErrors = append(validationErrors, fmt.Sprintf("%s: Security: Path must be within .github folder (failed to resolve relative path for %q from %q: %v)", filePath, normalizedAbsolutePath, normalizedGithubFolder, err)) | |
| continue | |
| } | |
| if relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { |
| // Security check: ensure the resolved path is within the .github folder | ||
| // Use filepath.Rel to check if the path escapes the .github folder | ||
| normalizedGithubFolder := filepath.Clean(githubFolder) | ||
| normalizedAbsolutePath := filepath.Clean(absolutePath) | ||
| relativePath, err := filepath.Rel(normalizedGithubFolder, normalizedAbsolutePath) | ||
| if err != nil || relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) || filepath.IsAbs(relativePath) { | ||
| validationErrors = append(validationErrors, fmt.Sprintf("%s: Security: Path must be within .github folder (resolves to: %s)", filePath, relativePath)) | ||
| continue | ||
| } |
There was a problem hiding this comment.
There are existing tests for validateRuntimeImportFiles, but none assert that paths escaping .github (e.g., ../outside.md or absolute paths) are rejected by this new compile-time security check. Add unit tests that exercise the traversal/escape cases so this doesn’t regress.
| name: documentation | ||
| description: Documentation Brief description for SEO and navigation | ||
| --- | ||
|
|
There was a problem hiding this comment.
This file appears to be a duplicate of skills/documentation/SKILL.md. Keeping two independent copies is likely to drift over time; consider making one location canonical and generating/copying the other as part of a build step, or add a clear note here about the source of truth and update process.
| > Note: The canonical source for this skill description is `skills/documentation/SKILL.md`. | |
| > Update that file first; this copy is kept in sync as part of the project’s build/release process. |
| for !strings.HasSuffix(githubFolder, ".github") && githubFolder != "." && githubFolder != "/" { | ||
| githubFolder = filepath.Dir(githubFolder) | ||
| if githubFolder == "." || githubFolder == "/" { | ||
| // If we can't find .github folder, use baseDir | ||
| githubFolder = baseDir | ||
| break | ||
| } |
There was a problem hiding this comment.
The loop that walks up to find the .github directory can become infinite on Windows when baseDir is a volume root (e.g., C:\) or otherwise reaches a point where filepath.Dir(githubFolder) returns the same path. Consider breaking when the parent equals the current directory and/or using a more robust stop condition (e.g., detect volume root), and prefer filepath.Base(githubFolder) == ".github" over strings.HasSuffix to avoid matching directories like foo.github.
| for !strings.HasSuffix(githubFolder, ".github") && githubFolder != "." && githubFolder != "/" { | |
| githubFolder = filepath.Dir(githubFolder) | |
| if githubFolder == "." || githubFolder == "/" { | |
| // If we can't find .github folder, use baseDir | |
| githubFolder = baseDir | |
| break | |
| } | |
| for { | |
| if filepath.Base(githubFolder) == ".github" { | |
| break | |
| } | |
| parent := filepath.Dir(githubFolder) | |
| if parent == githubFolder || parent == "." { | |
| // If we can't find a .github folder in the ancestors, use baseDir | |
| githubFolder = baseDir | |
| break | |
| } | |
| githubFolder = parent |
Run #21821181676 failed at the "Interpolate variables and render templates" step due to a security violation: the workflow was attempting to import a skill file from outside the
.githubfolder.Investigation
../../skills/documentation/SKILL.mdimport path violates the runtime-import security constraint.githubfolder per security validation inruntime_import.cjs(lines 656-661)Security: Path ../../skills/documentation/SKILL.md must be within .github folderRoot Cause
Security constraint violation - the workflow was importing
../../skills/documentation/SKILL.mdwhich resolves outside the.githubfolder, triggering the runtime-import security check.Resolution
Changes Made:
skills/documentation/SKILL.mdto.github/skills/documentation/SKILL.mdglossary-maintainer.mdfrom../../skills/documentation/SKILL.mdto../skills/documentation/SKILL.mdtechnical-doc-writer.mdfrom../../skills/documentation/SKILL.mdto../skills/documentation/SKILL.mdpkg/workflow/expression_validation.govalidates all{{#runtime-import}}macrospkg/parser/remote_fetch.goprevents path resolution outside.githubfolderfilepath.Rel()to detect path traversal attempts and provide clear security error messagesThe runtime-import security validation is now enforced at three levels: compile-time validation, import resolution, and runtime execution. This ensures early detection of security violations before workflows are deployed, in addition to the existing runtime protection.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.