feat: auto-inject workspace skills during provisioning#375
Closed
ptone wants to merge 6 commits into
Closed
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces auto-injection of workspace skills from the /workspace/skills/ directory into the agent, supporting conditional injection based on the environment (e.g., git workspace or hub enabled) via YAML frontmatter in SKILL.md files. It also removes the deprecated appendExtraInstructions logic and the default agents.md template. The reviewer identified critical issues in parseSkillFrontmatter, including a potential CRLF bypass on Windows, prompt pollution from appending raw frontmatter in fallback mode, and silent YAML parsing failures. Suggestions were provided to normalize line endings, strip frontmatter before appending, use directory-derived skill names for better logging, and update the corresponding unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1194
to
+1208
| // parseSkillFrontmatter extracts YAML frontmatter from a SKILL.md file. | ||
| // Returns zero-value skillFrontmatter if no frontmatter is found. | ||
| func parseSkillFrontmatter(data []byte) skillFrontmatter { | ||
| content := string(data) | ||
| if !strings.HasPrefix(content, "---\n") { | ||
| return skillFrontmatter{} | ||
| } | ||
| end := strings.Index(content[4:], "\n---") | ||
| if end < 0 { | ||
| return skillFrontmatter{} | ||
| } | ||
| var fm skillFrontmatter | ||
| _ = yaml.Unmarshal([]byte(content[4:4+end]), &fm) | ||
| return fm | ||
| } |
There was a problem hiding this comment.
CRLF Bypass & Frontmatter Pollution in Fallback Mode
There are three issues with the current implementation of parseSkillFrontmatter:
- CRLF Bypass (High Severity): If
SKILL.mduses CRLF line endings (common on Windows or via git'sautocrlf),strings.HasPrefix(content, "---\n")will fail because the file starts with---\r\n. This causes parsing to fail silently, returning an emptyfm.InjectWhen, which then defaults to unconditionally injecting the skill. This bypasses the conditional injection guard. - Instruction Pollution (Medium Severity): In fallback mode (when
skillsDir == ""), the entireSKILL.mdfile—including the raw YAML frontmatter—is appended to the agent instructions. This pollutes the prompt with system-level metadata that can confuse the LLM. - Silent YAML Failures (Medium Severity): If there is a syntax error in the frontmatter,
yaml.Unmarshalfails silently, defaulting to unconditional injection. We should log this error to aid debugging.
We can resolve all three issues by normalizing CRLF to LF, parsing the frontmatter, and returning the remaining markdown body with the frontmatter stripped.
func parseSkillFrontmatter(data []byte) (skillFrontmatter, string) {
content := string(data)
normalized := strings.ReplaceAll(content, "\r\n", "\n")
if !strings.HasPrefix(normalized, "---\n") {
return skillFrontmatter{}, content
}
end := strings.Index(normalized[4:], "\n---")
if end < 0 {
return skillFrontmatter{}, content
}
var fm skillFrontmatter
if err := yaml.Unmarshal([]byte(normalized[4:4+end]), &fm); err != nil {
util.Debugf("provision: failed to parse frontmatter in SKILL.md: %v", err)
}
bodyOffset := 4 + end + 4
if bodyOffset < len(normalized) && normalized[bodyOffset] == '\n' {
bodyOffset++
}
return fm, normalized[bodyOffset:]
}
Comment on lines
+1217
to
+1231
| // shouldInjectSkill checks whether a skill should be injected based on its | ||
| // inject_when frontmatter condition and the current provisioning context. | ||
| func shouldInjectSkill(fm skillFrontmatter, injCtx workspaceSkillsInjectionContext) bool { | ||
| switch fm.InjectWhen { | ||
| case "": | ||
| return true | ||
| case "git_workspace": | ||
| return injCtx.IsGit | ||
| case "hub_enabled": | ||
| return injCtx.HubEnabled | ||
| default: | ||
| util.Debugf("provision: unknown inject_when=%q for skill %q, skipping", fm.InjectWhen, fm.Name) | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Use Directory-Derived Skill Name in Debug Log
If the frontmatter does not explicitly define a name field, fm.Name will be empty, making the debug log on line 1228 unhelpful (e.g., unknown inject_when="..." for skill ""). Passing the directory-derived skillName to shouldInjectSkill ensures the log is always useful.
func shouldInjectSkill(skillName string, fm skillFrontmatter, injCtx workspaceSkillsInjectionContext) bool {
switch fm.InjectWhen {
case "":
return true
case "git_workspace":
return injCtx.IsGit
case "hub_enabled":
return injCtx.HubEnabled
default:
util.Debugf("provision: unknown inject_when=%q for skill %q, skipping", fm.InjectWhen, skillName)
return false
}
}
Comment on lines
+1266
to
1313
| for _, entry := range entries { | ||
| if !entry.IsDir() || strings.HasPrefix(entry.Name(), ".") { | ||
| continue | ||
| } | ||
| skillName := entry.Name() | ||
| skillSrc := filepath.Join(wsSkillsDir, skillName) | ||
|
|
||
| // Parse SKILL.md frontmatter for injection conditions | ||
| skillMD := filepath.Join(skillSrc, "SKILL.md") | ||
| var fm skillFrontmatter | ||
| if data, err := os.ReadFile(skillMD); err == nil { | ||
| fm = parseSkillFrontmatter(data) | ||
| } | ||
|
|
||
| if !shouldInjectSkill(fm, injCtx) { | ||
| util.Debugf("provision: skipping workspace skill %q (inject_when=%q not satisfied)", skillName, fm.InjectWhen) | ||
| continue | ||
| } | ||
|
|
||
| if skillsDir != "" { | ||
| // Harness supports skills — copy the skill directory | ||
| skillDest := filepath.Join(agentHome, skillsDir, skillName) | ||
|
|
||
| // Template skills take precedence: skip if already present | ||
| if _, err := os.Stat(skillDest); err == nil { | ||
| util.Debugf("provision: workspace skill %q skipped (template skill takes precedence)", skillName) | ||
| continue | ||
| } | ||
|
|
||
| if err := os.MkdirAll(skillDest, 0755); err != nil { | ||
| return content, fmt.Errorf("failed to create workspace skill dir %s: %w", skillName, err) | ||
| } | ||
| if err := util.CopyDir(skillSrc, skillDest); err != nil { | ||
| return content, fmt.Errorf("failed to copy workspace skill %s: %w", skillName, err) | ||
| } | ||
| util.Debugf("provision: injected workspace skill %q into %s", skillName, skillDest) | ||
| } else { | ||
| // Harness lacks skill support — composite SKILL.md content into agent instructions | ||
| data, err := os.ReadFile(skillMD) | ||
| if err != nil { | ||
| util.Debugf("provision: workspace skill %q has no SKILL.md, skipping fallback injection", skillName) | ||
| continue | ||
| } | ||
| util.Debugf("provision: compositing workspace skill %q SKILL.md (%d bytes) into agent instructions", skillName, len(data)) | ||
| content = append(content, '\n') | ||
| content = append(content, extra...) | ||
| content = append(content, data...) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update injectWorkspaceSkills to Use Stripped Body and New Signatures
Update the loop to use the new parseSkillFrontmatter and shouldInjectSkill signatures, and append only the stripped markdown body in fallback mode.
for _, entry := range entries {
if !entry.IsDir() || strings.HasPrefix(entry.Name(), ".") {
continue
}
skillName := entry.Name()
skillSrc := filepath.Join(wsSkillsDir, skillName)
// Parse SKILL.md frontmatter for injection conditions
skillMD := filepath.Join(skillSrc, "SKILL.md")
var fm skillFrontmatter
var skillBody string
if data, err := os.ReadFile(skillMD); err == nil {
fm, skillBody = parseSkillFrontmatter(data)
}
if !shouldInjectSkill(skillName, fm, injCtx) {
util.Debugf("provision: skipping workspace skill %q (inject_when=%q not satisfied)", skillName, fm.InjectWhen)
continue
}
if skillsDir != "" {
// Harness supports skills — copy the skill directory
skillDest := filepath.Join(agentHome, skillsDir, skillName)
// Template skills take precedence: skip if already present
if _, err := os.Stat(skillDest); err == nil {
util.Debugf("provision: workspace skill %q skipped (template skill takes precedence)", skillName)
continue
}
if err := os.MkdirAll(skillDest, 0755); err != nil {
return content, fmt.Errorf("failed to create workspace skill dir %s: %w", skillName, err)
}
if err := util.CopyDir(skillSrc, skillDest); err != nil {
return content, fmt.Errorf("failed to copy workspace skill %s: %w", skillName, err)
}
util.Debugf("provision: injected workspace skill %q into %s", skillName, skillDest)
} else {
// Harness lacks skill support — composite SKILL.md content into agent instructions
if skillBody == "" {
util.Debugf("provision: workspace skill %q has no SKILL.md content, skipping fallback injection", skillName)
continue
}
util.Debugf("provision: compositing workspace skill %q SKILL.md (%d bytes) into agent instructions", skillName, len(skillBody))
content = append(content, '\n')
content = append(content, []byte(skillBody)...)
}
}
Comment on lines
+2051
to
+2092
| func TestParseSkillFrontmatter(t *testing.T) { | ||
| t.Run("valid frontmatter", func(t *testing.T) { | ||
| data := []byte("---\nname: test-skill\ndescription: A test skill\ninject_when: git_workspace\n---\n\n# Content here\n") | ||
| fm := parseSkillFrontmatter(data) | ||
| if fm.Name != "test-skill" { | ||
| t.Errorf("Name=%q, want %q", fm.Name, "test-skill") | ||
| } | ||
| if fm.Description != "A test skill" { | ||
| t.Errorf("Description=%q, want %q", fm.Description, "A test skill") | ||
| } | ||
| if fm.InjectWhen != "git_workspace" { | ||
| t.Errorf("InjectWhen=%q, want %q", fm.InjectWhen, "git_workspace") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("no frontmatter", func(t *testing.T) { | ||
| data := []byte("# Just a markdown file\nNo frontmatter here\n") | ||
| fm := parseSkillFrontmatter(data) | ||
| if fm.Name != "" || fm.InjectWhen != "" { | ||
| t.Errorf("expected zero-value frontmatter, got %+v", fm) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("no inject_when field", func(t *testing.T) { | ||
| data := []byte("---\nname: unconditional\ndescription: Always inject\n---\n\n# Content\n") | ||
| fm := parseSkillFrontmatter(data) | ||
| if fm.Name != "unconditional" { | ||
| t.Errorf("Name=%q, want %q", fm.Name, "unconditional") | ||
| } | ||
| if fm.InjectWhen != "" { | ||
| t.Errorf("InjectWhen=%q, want empty", fm.InjectWhen) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("unclosed frontmatter", func(t *testing.T) { | ||
| data := []byte("---\nname: broken\n# No closing delimiter\n") | ||
| fm := parseSkillFrontmatter(data) | ||
| if fm.Name != "" { | ||
| t.Errorf("expected zero-value for unclosed frontmatter, got %+v", fm) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Update TestParseSkillFrontmatter for New Signature and CRLF Handling
Update the unit tests to match the new (skillFrontmatter, string) signature, assert that the stripped body is returned correctly, and add a test case for CRLF line endings.
func TestParseSkillFrontmatter(t *testing.T) {
t.Run("valid frontmatter", func(t *testing.T) {
data := []byte("---\nname: test-skill\ndescription: A test skill\ninject_when: git_workspace\n---\n\n# Content here\n")
fm, body := parseSkillFrontmatter(data)
if fm.Name != "test-skill" {
t.Errorf("Name=%q, want %q", fm.Name, "test-skill")
}
if fm.Description != "A test skill" {
t.Errorf("Description=%q, want %q", fm.Description, "A test skill")
}
if fm.InjectWhen != "git_workspace" {
t.Errorf("InjectWhen=%q, want %q", fm.InjectWhen, "git_workspace")
}
if !strings.Contains(body, "# Content here") {
t.Errorf("expected body to contain content, got %q", body)
}
})
t.Run("no frontmatter", func(t *testing.T) {
data := []byte("# Just a markdown file\nNo frontmatter here\n")
fm, body := parseSkillFrontmatter(data)
if fm.Name != "" || fm.InjectWhen != "" {
t.Errorf("expected zero-value frontmatter, got %+v", fm)
}
if string(data) != body {
t.Errorf("expected body to be original content, got %q", body)
}
})
t.Run("no inject_when field", func(t *testing.T) {
data := []byte("---\nname: unconditional\ndescription: Always inject\n---\n\n# Content\n")
fm, body := parseSkillFrontmatter(data)
if fm.Name != "unconditional" {
t.Errorf("Name=%q, want %q", fm.Name, "unconditional")
}
if fm.InjectWhen != "" {
t.Errorf("InjectWhen=%q, want empty", fm.InjectWhen)
}
if !strings.Contains(body, "# Content") {
t.Errorf("expected body to contain content, got %q", body)
}
})
t.Run("unclosed frontmatter", func(t *testing.T) {
data := []byte("---\nname: broken\n# No closing delimiter\n")
fm, body := parseSkillFrontmatter(data)
if fm.Name != "" {
t.Errorf("expected zero-value for unclosed frontmatter, got %+v", fm)
}
if string(data) != body {
t.Errorf("expected body to be original content, got %q", body)
}
})
t.Run("CRLF handling", func(t *testing.T) {
data := []byte("---\r\nname: crlf-skill\r\ninject_when: git_workspace\r\n---\r\n\r\n# CRLF Content\r
")
fm, body := parseSkillFrontmatter(data)
if fm.Name != "crlf-skill" {
t.Errorf("Name=%q, want %q", fm.Name, "crlf-skill")
}
if fm.InjectWhen != "git_workspace" {
t.Errorf("InjectWhen=%q, want %q", fm.InjectWhen, "git_workspace")
}
if !strings.Contains(body, "# CRLF Content") {
t.Errorf("expected body to contain content, got %q", body)
}
})
}
Comment on lines
+2094
to
+2118
| func TestShouldInjectSkill(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| injectWhen string | ||
| ctx workspaceSkillsInjectionContext | ||
| want bool | ||
| }{ | ||
| {"unconditional always injects", "", workspaceSkillsInjectionContext{}, true}, | ||
| {"git_workspace with git", "git_workspace", workspaceSkillsInjectionContext{IsGit: true}, true}, | ||
| {"git_workspace without git", "git_workspace", workspaceSkillsInjectionContext{IsGit: false}, false}, | ||
| {"hub_enabled with hub", "hub_enabled", workspaceSkillsInjectionContext{HubEnabled: true}, true}, | ||
| {"hub_enabled without hub", "hub_enabled", workspaceSkillsInjectionContext{HubEnabled: false}, false}, | ||
| {"unknown condition skips", "unknown_condition", workspaceSkillsInjectionContext{IsGit: true, HubEnabled: true}, false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| fm := skillFrontmatter{Name: "test", InjectWhen: tt.injectWhen} | ||
| got := shouldInjectSkill(fm, tt.ctx) | ||
| if got != tt.want { | ||
| t.Errorf("shouldInjectSkill(inject_when=%q)=%v, want %v", tt.injectWhen, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Update TestShouldInjectSkill for New Signature
Update the unit tests to pass the skill name to shouldInjectSkill.
func TestShouldInjectSkill(t *testing.T) {
tests := []struct {
name string
injectWhen string
ctx workspaceSkillsInjectionContext
want bool
}{
{"unconditional always injects", "", workspaceSkillsInjectionContext{}, true},
{"git_workspace with git", "git_workspace", workspaceSkillsInjectionContext{IsGit: true}, true},
{"git_workspace without git", "git_workspace", workspaceSkillsInjectionContext{IsGit: false}, false},
{"hub_enabled with hub", "hub_enabled", workspaceSkillsInjectionContext{HubEnabled: true}, true},
{"hub_enabled without hub", "hub_enabled", workspaceSkillsInjectionContext{HubEnabled: false}, false},
{"unknown condition skips", "unknown_condition", workspaceSkillsInjectionContext{IsGit: true, HubEnabled: true}, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fm := skillFrontmatter{Name: "test", InjectWhen: tt.injectWhen}
got := shouldInjectSkill("test", fm, tt.ctx)
if got != tt.want {
t.Errorf("shouldInjectSkill(inject_when=%q)=%v, want %v", tt.injectWhen, got, tt.want)
}
})
}
}
…uild context (GoogleCloudPlatform#576) * Refocus image builds on base and harness catalog * image-build: add gemini-cli harness build step * image-build: copy resources/ and harnesses/ dirs into build context --------- Co-authored-by: Scion Agent (hr-arch-codex-2) <agent@scion.dev>
Add injectWorkspaceSkills() to discover and inject all skills from the workspace-level skills/ directory into every agent at provision time. For harnesses with native skill support (SkillsDir != ""), skill directories are copied into the agent's harness-specific skills location. For harnesses without skill support, SKILL.md content is composited into the agent instructions as a fallback. Template skills take precedence — if a template already installed a skill with the same name, the workspace skill is skipped. Includes conditional injection via SKILL.md frontmatter: skills can declare inject_when (git_workspace, hub_enabled) to control when they are injected based on provisioning context. Implements Sub-Plan C phases C1 and C3 of the contrib refactor.
Remove the appendExtraInstructions() function and its two call sites, replacing them with the workspace skill auto-injection mechanism. The status boilerplate in resources/templates/default/agents.md is cleared — this content is now delivered by the agent-status-signals workspace skill via auto-injection. Existing tests for appendExtraInstructions are removed. Replacement coverage is added in Phase C4. Implements Sub-Plan C phase C2 of the contrib refactor.
Covers: - parseSkillFrontmatter: valid frontmatter, missing frontmatter, unclosed frontmatter, omitted inject_when - shouldInjectSkill: unconditional, git_workspace, hub_enabled, and unknown conditions - injectWorkspaceSkills with harness skill support: unconditional injection, conditional git_workspace/hub_enabled, template precedence, graceful missing dir, hidden dir/file skipping - injectWorkspaceSkills fallback composition: SKILL.md content appended to instructions, conditional respected, missing SKILL.md skipped Implements Sub-Plan C phase C4 of the contrib refactor.
M1: Log YAML parse errors in parseSkillFrontmatter instead of
silently discarding them.
M2: Add debug log for symlinked skill directories that are skipped
by os.ReadDir's IsDir() check.
M3: Isolate test subtests with per-subtest t.TempDir() via a shared
setupWorkspaceSkillsTest helper, eliminating state leaks between
subtests.
Also adds test for malformed YAML frontmatter (test gap identified
in review).
Review feedback (Gemini code review): - CRLF normalization: add strings.ReplaceAll for \r\n before parsing SKILL.md frontmatter - Double file read: read SKILL.md once and reuse bytes for both frontmatter parsing and fallback content injection - Empty agent_instructions: inject workspace skills fallback content even when no base agent_instructions are defined CI fixes: - golangci-lint errcheck: check return values of os.MkdirAll and os.WriteFile in all new test helpers and subtests - TestInitMachine_RestoresDeletedFiles: switch test target from agents.md (now intentionally empty) to scion-agent.yaml
ptone
force-pushed
the
contrib-refactor/auto-inject-skills
branch
from
July 2, 2026 16:15
2bef22f to
c3d6134
Compare
Owner
Author
|
Work merged upstream via GoogleCloudPlatform/scion PR GoogleCloudPlatform#573. Closing fork staging PR. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Sub-Plan C of the contrib refactor — modifies scion's provisioning code to auto-inject all skills from
/workspace/skills/into every agent during provisioning, replacing theappendExtraInstructions()text-composition mechanism.injectWorkspaceSkills()with conditional injection via SKILL.md frontmatter (inject_when: git_workspace | hub_enabled). For harnesses with native skill support, copies skill directories. For harnesses without, composites SKILL.md content into agent instructions.appendExtraInstructions()and its call sites. Clears status boilerplate fromresources/templates/default/agents.md(now delivered byagent-status-signalsskill).Key design decisions:
projectDir(strips.scionsuffix)inject_whenvalues cause the skill to be skipped (fail-safe)Test plan
./pkg/agent/test suite passes