feat: allow instruction_file to accept a list of files#3306
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| if len(f) == 1 { | ||
| return f[0], nil | ||
| } | ||
| return []string(f), nil |
There was a problem hiding this comment.
[MEDIUM] MarshalYAML returns empty slice for zero-length InstructionFiles, bypassing omitempty
When instruction_file: [] is parsed from YAML, UnmarshalYAML correctly falls through the scalar path (can't decode a sequence into string) and calls list(&many), which succeeds with many = []string{} — giving a non-nil empty InstructionFiles{}. This non-nil empty value later reaches MarshalYAML:
func (f InstructionFiles) MarshalYAML() (any, error) {
if len(f) == 1 {
return f[0], nil
}
return []string(f), nil // returns []string{} for len==0 — not nil
}In go-yaml v3, omitempty on the struct field checks whether the value equals the zero value for its type before deciding to call MarshalYAML. For a named slice type, nil is zero but a non-nil empty slice is not — so go-yaml v3 will call MarshalYAML, receive []string{}, and serialize it as instruction_file: [] in the output YAML instead of omitting the field entirely.
resolveInstructionFiles skips agents with len(agent.InstructionFile) == 0 but does not normalize an empty slice to nil, so the empty InstructionFiles{} persists through OCI packaging and share push workflows.
Fix: add an early-return guard in MarshalYAML:
func (f InstructionFiles) MarshalYAML() (any, error) {
if len(f) == 0 {
return nil, nil
}
if len(f) == 1 {
return f[0], nil
}
return []string(f), nil
}|
|
||
| parts := make([]string, 0, len(paths)) | ||
| for _, path := range paths { | ||
| if !filepath.IsLocal(path) { |
There was a problem hiding this comment.
[LOW] Empty string entry in instruction_file list causes confusing error; inconsistent with scalar handling
The scalar form instruction_file: "" is silently treated as absent (the unmarshal path explicitly sets *f = nil when one == ""). But an empty string inside a list — e.g. instruction_file: ["", "file.md"] — is not filtered during unmarshal, and later trips the path-safety check:
if !filepath.IsLocal(path) { // filepath.IsLocal("") == false in Go ≥1.20
return "", fmt.Errorf("instruction_file %q must be a local relative path …", path)
}This produces the error: instruction_file "" must be a local relative path inside the config directory — which correctly rejects the input but gives no indication that the problem is an empty entry rather than a traversal attempt.
The inconsistency means a user who migrates from the scalar "" (silently ignored) to the list form [""] will get an unexpected error instead of the same no-op. Consider filtering out empty strings in unmarshal (same as the scalar branch does) and/or improving the error message to distinguish an empty path from a path-traversal attempt.
|
Thanks for the review! Addressed both points in 1e0b633:
Added tests: |
|
👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added |
When several paths are listed their contents are concatenated in order, separated by a blank line, so a shared preamble can be reused across agents.
Eliminates three manual `_ = root.Close()` call sites and prevents a potential handle leak if an early return is added in the future. Assisted-By: Claude
Zero-length InstructionFiles now marshals to nil/null so the omitempty tag suppresses the field entirely. Empty strings in the list form are stripped on decode, matching scalar behaviour and avoiding a confusing path-safety error. Assisted-By: Claude
1e0b633 to
2b5ab5e
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The implementation looks correct and well-structured. The custom YAML/JSON marshalers handle both scalar and list forms properly, the helper correctly applies path-safety rules (absolute path rejection, ... traversal, os.OpenRoot confinement) to every entry, the deferred root.Close avoids handle leaks, and the OCI push probe covers both string and list forms. Concatenation with a blank-line separator and single-element scalar round-tripping are correct.
Agents often share a common preamble — coding conventions, tone guidelines, a project glossary — but each also needs its own role-specific instructions. Previously
instruction_fileaccepted only a single path, so teams had to maintain one monolithic file per agent or work around the limitation with custom tooling.This change extends
instruction_fileso it accepts either a single string (unchanged behaviour) or a list of strings. When multiple paths are given, their contents are concatenated in order separated by a blank line, letting you compose a shared preamble with per-agent specifics cleanly. A single-element list serialises back to scalar form, so existing configs and round-tripped YAML are unaffected. The new form is reflected inagent-schema.json(oneOfstring or array), the docs, and a new example underexamples/instruction_file.yamlwith an accompanyingexamples/instructions/shared-preamble.md.All existing path-safety rules apply to every entry in the list: absolute paths and
..traversal are rejected, and reads are confined withos.OpenRootso symlinks cannot escape the config directory. List-form configs are local-file-only; OCI and URL sources are not supported (consistent with the existing single-file restriction). TheconfigUsesInstructionFileOCI-push probe handles both string and list forms so pushed artifacts remain self-contained. The implementation lives in a newInstructionFilestype with custom YAML/JSON marshalers inpkg/config/latest/types.go; the resolution logic inpkg/config/config.gowas refactored into areadInstructionFileshelper that uses a deferredroot.Closeto avoid leaking theos.Roothandle. New tests cover list concatenation, missing files, per-entry traversal rejection, empty-entry rejection, and single-element scalar marshalling.