-
Notifications
You must be signed in to change notification settings - Fork 295
Replace inlined Go builtin engine definitions with embedded shared agentic workflow files #20500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6097248
8a098cc
4a6f53a
2eee3ff
aa1af02
5ae6466
ef76fa7
63714b4
e5e57b5
889894c
48a4ecc
b3ae08b
2ea2345
f3dfe5a
81cc602
c536da1
f5e3621
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,16 @@ func isRepositoryImport(importPath string) bool { | |
| func ResolveIncludePath(filePath, baseDir string, cache *ImportCache) (string, error) { | ||
| remoteLog.Printf("Resolving include path: file_path=%s, base_dir=%s", filePath, baseDir) | ||
|
|
||
| // Handle builtin paths - these are embedded files that bypass filesystem resolution. | ||
| // No security check is needed since the content is compiled into the binary. | ||
| if strings.HasPrefix(filePath, BuiltinPathPrefix) { | ||
| if !BuiltinVirtualFileExists(filePath) { | ||
| return "", fmt.Errorf("builtin file not found: %s", filePath) | ||
| } | ||
| remoteLog.Printf("Resolved builtin path: %s", filePath) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice early-return pattern for builtin paths. Bypassing the filesystem resolution for embedded files makes sense, and the |
||
| return filePath, nil | ||
| } | ||
|
|
||
| // Check if this is a workflowspec (contains owner/repo/path format) | ||
| // Format: owner/repo/path@ref or owner/repo/path@ref#section | ||
| if isWorkflowSpec(filePath) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,68 @@ | ||
| package parser | ||
|
|
||
| import "os" | ||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "strings" | ||
| "sync" | ||
| ) | ||
|
|
||
| // builtinVirtualFiles holds embedded built-in files registered at startup. | ||
| // Keys use the "@builtin:" path prefix (e.g. "@builtin:engines/copilot.md"). | ||
| // The map is populated once and then read-only; concurrent reads are safe. | ||
| var ( | ||
| builtinVirtualFiles map[string][]byte | ||
| builtinVirtualFilesMu sync.RWMutex | ||
| ) | ||
|
|
||
| // RegisterBuiltinVirtualFile registers an embedded file under a canonical builtin path. | ||
| // Paths must start with BuiltinPathPrefix ("@builtin:"); it panics if they do not. | ||
| // If the same path is registered twice with identical content the call is a no-op. | ||
| // Registering the same path with different content panics to surface configuration errors early. | ||
| // This function is safe for concurrent use. | ||
| func RegisterBuiltinVirtualFile(path string, content []byte) { | ||
| if !strings.HasPrefix(path, BuiltinPathPrefix) { | ||
| panic(fmt.Sprintf("RegisterBuiltinVirtualFile: path %q does not start with %q", path, BuiltinPathPrefix)) | ||
| } | ||
| builtinVirtualFilesMu.Lock() | ||
| defer builtinVirtualFilesMu.Unlock() | ||
| if builtinVirtualFiles == nil { | ||
| builtinVirtualFiles = make(map[string][]byte) | ||
| } | ||
| if existing, ok := builtinVirtualFiles[path]; ok { | ||
| if string(existing) != string(content) { | ||
| panic(fmt.Sprintf("RegisterBuiltinVirtualFile: path %q already registered with different content", path)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Panicking on duplicate registration with different content is a good safety net during startup, but consider adding a note about the registration order. If two |
||
| } | ||
| return // idempotent: same content, no-op | ||
| } | ||
| builtinVirtualFiles[path] = content | ||
| } | ||
pelikhan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // BuiltinVirtualFileExists returns true if the given path is registered as a builtin virtual file. | ||
| func BuiltinVirtualFileExists(path string) bool { | ||
| builtinVirtualFilesMu.RLock() | ||
| defer builtinVirtualFilesMu.RUnlock() | ||
| _, ok := builtinVirtualFiles[path] | ||
| return ok | ||
| } | ||
|
|
||
| // BuiltinPathPrefix is the path prefix used for embedded builtin files. | ||
| // Paths with this prefix bypass filesystem resolution and security checks. | ||
| const BuiltinPathPrefix = "@builtin:" | ||
|
|
||
| // readFileFunc is the function used to read file contents throughout the parser. | ||
| // In wasm builds, this is overridden to read from a virtual filesystem | ||
| // populated by the browser via SetVirtualFiles. | ||
| var readFileFunc = os.ReadFile | ||
| // In native builds, builtin virtual files are checked first, then os.ReadFile. | ||
| var readFileFunc = func(path string) ([]byte, error) { | ||
| builtinVirtualFilesMu.RLock() | ||
| content, ok := builtinVirtualFiles[path] | ||
| builtinVirtualFilesMu.RUnlock() | ||
| if ok { | ||
| return content, nil | ||
| } | ||
| return os.ReadFile(path) | ||
| } | ||
|
|
||
| // ReadFile reads a file using the parser's file reading function, which | ||
| // checks the virtual filesystem first in wasm builds. Use this instead of | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -98,6 +98,27 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean | |
| c.IncrementWarningCount() | ||
| } | ||
| engineSetting = c.engineOverride | ||
| // Update engineConfig.ID so that downstream code (e.g. generateCreateAwInfo) uses | ||
| // the override engine ID, not the one parsed from the frontmatter. | ||
| if engineConfig != nil { | ||
| engineConfig.ID = c.engineOverride | ||
| } | ||
| } | ||
|
|
||
| // When the engine is specified in short/string form ("engine: copilot") and no CLI | ||
| // override is active, inject the corresponding builtin shared-workflow .md as an | ||
| // import. This makes "engine: copilot" syntactic sugar for importing the builtin | ||
| // copilot.md, which carries the full engine definition. The engine field is removed | ||
| // from the frontmatter so the definition comes entirely from the import. | ||
| if c.engineOverride == "" && isStringFormEngine(result.Frontmatter) && engineSetting != "" { | ||
| builtinPath := builtinEnginePath(engineSetting) | ||
| if parser.BuiltinVirtualFileExists(builtinPath) { | ||
| orchestratorEngineLog.Printf("Injecting builtin engine import: %s", builtinPath) | ||
| addImportToFrontmatter(result.Frontmatter, builtinPath) | ||
| delete(result.Frontmatter, "engine") | ||
| engineSetting = "" | ||
| engineConfig = nil | ||
| } | ||
| } | ||
|
|
||
| // Process imports from frontmatter first (before @include directives) | ||
|
|
@@ -197,6 +218,19 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean | |
| return nil, fmt.Errorf("failed to extract engine config from included file: %w", err) | ||
| } | ||
| engineConfig = extractedConfig | ||
|
|
||
| // If the imported engine is an inline definition (engine.runtime sub-object), | ||
| // validate and register it in the catalog. This mirrors the handling for inline | ||
| // definitions declared directly in the main workflow (above). | ||
| if engineConfig != nil && engineConfig.IsInlineDefinition { | ||
| if err := c.validateEngineInlineDefinition(engineConfig); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := c.validateEngineAuthDefinition(engineConfig); err != nil { | ||
| return nil, err | ||
| } | ||
| c.registerInlineEngineDefinition(engineConfig) | ||
| } | ||
| } | ||
|
|
||
| // Apply the default AI engine setting if not specified | ||
|
|
@@ -299,3 +333,43 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean | |
| configSteps: configSteps, | ||
| }, nil | ||
| } | ||
|
|
||
| // isStringFormEngine reports whether the "engine" field in the given frontmatter is a | ||
| // plain string (e.g. "engine: copilot"), as opposed to an object with an "id" or | ||
| // "runtime" sub-key. | ||
| func isStringFormEngine(frontmatter map[string]any) bool { | ||
| engine, exists := frontmatter["engine"] | ||
| if !exists { | ||
| return false | ||
| } | ||
| _, isString := engine.(string) | ||
| return isString | ||
| } | ||
|
|
||
| // addImportToFrontmatter appends importPath to the "imports" slice in frontmatter. | ||
| // It handles the case where "imports" may be absent, a []any, a []string, or a | ||
| // single string (which is converted to a two-element slice preserving the original value). | ||
| // Any other unexpected type is left unchanged and importPath is not injected. | ||
| func addImportToFrontmatter(frontmatter map[string]any, importPath string) { | ||
| existing, hasImports := frontmatter["imports"] | ||
| if !hasImports { | ||
| frontmatter["imports"] = []any{importPath} | ||
| return | ||
| } | ||
| switch v := existing.(type) { | ||
| case []any: | ||
| frontmatter["imports"] = append(v, importPath) | ||
| case []string: | ||
| newSlice := make([]any, len(v)+1) | ||
| for i, s := range v { | ||
| newSlice[i] = s | ||
| } | ||
| newSlice[len(v)] = importPath | ||
| frontmatter["imports"] = newSlice | ||
| case string: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| // Single string import — preserve it and append the new one. | ||
| frontmatter["imports"] = []any{v, importPath} | ||
| // For any other unexpected type, leave the field untouched so the | ||
| // downstream parser can still report its own error for the invalid value. | ||
| } | ||
pelikhan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good guard — combining the
inputscheck with theBuiltinPathPrefixcheck in a single condition keeps the logic concise. The comment on lines 80-81 makes the intent clear for future readers.