Skip to content
56 changes: 33 additions & 23 deletions pkg/workflow/action_resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,29 +113,12 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (
resolverLog.Printf("Cache miss for %s@%s, checking embedded action pins", repo, version)

// Check embedded action pins for a semver-compatible version before making
// a network call. The embedded pins are the source-of-truth for known versions
// and are always available without network access. This avoids a ~1s gh-api
// subprocess for any action that is already covered by the embedded pin set.
requested := semverutil.EnsureVPrefix(version)
requestedVer := semverutil.ParseVersion(requested)
requestedIsPrecise := requestedVer != nil && requestedVer.IsPreciseVersion()

for _, pin := range actionpins.GetActionPinsByRepo(repo) {
pinVersion := semverutil.EnsureVPrefix(pin.Version)
if requestedIsPrecise {
if pinVersion != requested {
continue
}
} else if !semverutil.IsCompatible(pinVersion, requested) {
continue
}

resolverLog.Printf("Embedded pin hit for %s@%s → %s (%s)", repo, version, pin.SHA, pin.Version)
// Note: we intentionally do NOT call r.cache.Set() here. The embedded pins
// are always available in memory so there is nothing to persist, and writing
// to the on-disk cache would create root-owned files when compiling inside
// Docker containers (e.g. the Alpine CI test), preventing cleanup by the host.
return pin.SHA, nil
// a network call. Note: we intentionally do NOT call r.cache.Set() for
// embedded pins — they are always available in memory, and writing to the
// on-disk cache would create root-owned files inside Docker containers
// (e.g. the Alpine CI test), preventing cleanup by the host.
if sha, found := lookupEmbeddedActionPin(repo, version); found {
return sha, nil
}

resolverLog.Printf("No embedded pin for %s@%s, querying GitHub API", repo, version)
Expand All @@ -160,6 +143,33 @@ func (r *ActionResolver) ResolveSHA(ctx context.Context, repo, version string) (
return sha, nil
}

// lookupEmbeddedActionPin returns the pinned SHA for repo@version from the
// embedded action pin set. It returns ("", false) if no matching pin is found.
// The embedded pins are the source-of-truth for known versions and are always
// available without network access, avoiding a ~1s gh-api subprocess for any
// action covered by the embedded pin set.
func lookupEmbeddedActionPin(repo, version string) (string, bool) {
requested := semverutil.EnsureVPrefix(version)
requestedVer := semverutil.ParseVersion(requested)
requestedIsPrecise := requestedVer != nil && requestedVer.IsPreciseVersion()

for _, pin := range actionpins.GetActionPinsByRepo(repo) {
pinVersion := semverutil.EnsureVPrefix(pin.Version)
if requestedIsPrecise {
if pinVersion != requested {
continue
}
} else if !semverutil.IsCompatible(pinVersion, requested) {
continue
}

resolverLog.Printf("Embedded pin hit for %s@%s → %s (%s)", repo, version, pin.SHA, pin.Version)
return pin.SHA, true
}

return "", false

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] lookupEmbeddedActionPin contains non-trivial semver matching logic (precise vs. range version comparison) but has no direct test — existing ResolveSHA tests only cover cache-hit and network-error paths; the embedded-pin branch is untested.

💡 Suggested test

lookupEmbeddedActionPin is package-private and can be unit-tested directly:

func TestLookupEmbeddedActionPin(t *testing.T) {
    t.Run("SHA returned for semver range match", func(t *testing.T) {
        sha, found := lookupEmbeddedActionPin("actions/checkout", "v4")
        require.True(t, found)
        require.NotEmpty(t, sha)
    })
    t.Run("false for unknown repo", func(t *testing.T) {
        _, found := lookupEmbeddedActionPin("nonexistent/action", "v1")
        require.False(t, found)
    })
}

This locks down the semver-matching logic independently of the cache layer.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit b299c85. Added TestLookupEmbeddedActionPin to action_resolver_test.go covering four paths: precise-version match, semver-range match, unknown-repo not-found, and known-repo incompatible-version not-found.

}

// ParseTagRefTSV parses the tab-separated output from the GitHub API
// `[.object.sha, .object.type] | @tsv` jq expression.
// It returns the object SHA and type, or an error if the output is malformed.
Expand Down
66 changes: 34 additions & 32 deletions pkg/workflow/antigravity_tools.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,38 +56,7 @@ func computeAntigravityToolsCore(tools map[string]any) []string {

// Map bash neutral tool to run_shell_command
if bashConfig, hasBash := tools["bash"]; hasBash {
bashCommands, ok := bashConfig.([]any)
if !ok || len(bashCommands) == 0 {
// bash with no specific commands - allow all shell commands
antigravityToolsLog.Print("bash (no specific commands) → run_shell_command")
toolsCore = append(toolsCore, "run_shell_command")
} else {
// Check for wildcard (* or :*)
hasWildcard := false
for _, cmd := range bashCommands {
if cmdStr, ok := cmd.(string); ok && (cmdStr == "*" || cmdStr == ":*") {
hasWildcard = true
break
}
}
if hasWildcard {
antigravityToolsLog.Print("bash wildcard → run_shell_command")
toolsCore = append(toolsCore, "run_shell_command")
} else {
// Add an entry for each specific command: run_shell_command(cmd)
for _, cmd := range bashCommands {
if cmdStr, ok := cmd.(string); ok {
// Normalize trailing " *" wildcard (e.g. "jq *" → "jq") so that
// all engines emit the canonical prefix form (run_shell_command(jq))
// regardless of whether the command was written with or without the wildcard.
normalized, _ := normalizeBashCommand(cmdStr)
entry := fmt.Sprintf("run_shell_command(%s)", normalized)
antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
toolsCore = append(toolsCore, entry)
}
}
}
}
toolsCore = appendBashTools(toolsCore, bashConfig)
}

// Map edit neutral tool to write_file and replace (Antigravity's file write tools)
Expand All @@ -108,6 +77,39 @@ func computeAntigravityToolsCore(tools map[string]any) []string {
return toolsCore
}

// appendBashTools maps the bash neutral tool configuration to run_shell_command
// entries and appends them to toolsCore.
func appendBashTools(toolsCore []string, bashConfig any) []string {
bashCommands, ok := bashConfig.([]any)
if !ok || len(bashCommands) == 0 {
// bash with no specific commands - allow all shell commands
antigravityToolsLog.Print("bash (no specific commands) → run_shell_command")
return append(toolsCore, "run_shell_command")
}

// Check for wildcard (* or :*)
for _, cmd := range bashCommands {
if cmdStr, ok := cmd.(string); ok && (cmdStr == "*" || cmdStr == ":*") {
antigravityToolsLog.Print("bash wildcard → run_shell_command")
return append(toolsCore, "run_shell_command")
}
}

// Add an entry for each specific command: run_shell_command(cmd)
for _, cmd := range bashCommands {
if cmdStr, ok := cmd.(string); ok {
// Normalize trailing " *" wildcard (e.g. "jq *" → "jq") so that
// all engines emit the canonical prefix form (run_shell_command(jq))
// regardless of whether the command was written with or without the wildcard.
normalized, _ := normalizeBashCommand(cmdStr)
entry := fmt.Sprintf("run_shell_command(%s)", normalized)
antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
toolsCore = append(toolsCore, entry)
}
}
return toolsCore
}

// generateAntigravitySettingsStep creates a GitHub Actions step that writes the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] appendBashTools now returns early for the wildcard path, which is an improvement — but the two-pass loop over bashCommands (scan for wildcard, then iterate again for specific commands) is worth noting as a minor navigability issue.

💡 Consider single-pass alternative

The current implementation iterates bashCommands twice: once to detect any wildcard, and once to produce per-command entries. This means if any wildcard is mid-slice, commands before it are silently ignored during the second pass (which is fine, since the function returns early on wildcard). But a single-pass form makes the intent explicit and eliminates the implicit dependency between the two loops:

for _, cmd := range bashCommands {
    cmdStr, ok := cmd.(string)
    if !ok {
        continue
    }
    if cmdStr == "*" || cmdStr == ":*" {
        antigravityToolsLog.Print("bash wildcard → run_shell_command")
        return append(toolsCore, "run_shell_command")
    }
    normalized, _ := normalizeBashCommand(cmdStr)
    entry := fmt.Sprintf("run_shell_command(%s)", normalized)
    antigravityToolsLog.Printf("bash %q → %s", cmdStr, entry)
    toolsCore = append(toolsCore, entry)
}
return toolsCore

This is a nit — the current version is correct and the tests cover both paths.

@copilot please address this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in commit b299c85. Refactored to a true single-pass loop using a separate specific accumulator — wildcard (* or :*) anywhere in the list discards accumulated entries and returns only run_shell_command. The two-pass structure is gone.

// Antigravity CLI project settings file (.antigravity/settings.json) before execution.
//
Expand Down
51 changes: 34 additions & 17 deletions pkg/workflow/build_input_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,34 +46,20 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string
case "boolean":
inputType = "boolean"
case "choice":
inputType = "string"
if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 {
prop := map[string]any{
"type": inputType,
"description": inputDescription,
"enum": options,
}
if defaultVal, ok := inputDefMap["default"]; ok {
prop["default"] = defaultVal
}
if prop, isChoice := buildChoiceInputProperty(inputDefMap, inputDescription); isChoice {
properties[inputName] = prop
if inputRequired {
required = append(required, inputName)
}
continue
}
inputType = "string"
case "environment":
inputType = "string"
}
}

prop := map[string]any{
"type": inputType,
"description": inputDescription,
}
if defaultVal, ok := inputDefMap["default"]; ok {
prop["default"] = defaultVal
}
prop := buildInputProperty(inputType, inputDescription, inputDefMap)
buildInputSchemaLog.Printf("Input %q: type=%s, required=%v", inputName, inputType, inputRequired)
properties[inputName] = prop

Expand All @@ -85,3 +71,34 @@ func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string
buildInputSchemaLog.Printf("Built input schema: %d properties, %d required", len(properties), len(required))
return properties, required
}

// buildChoiceInputProperty builds a JSON Schema property for a "choice" input type.
// It returns (property, true) when options are available, or (nil, false) otherwise.
func buildChoiceInputProperty(inputDefMap map[string]any, inputDescription string) (map[string]any, bool) {
options, ok := inputDefMap["options"].([]any)
if !ok || len(options) == 0 {
return nil, false
}
prop := map[string]any{
"type": "string",
"description": inputDescription,
"enum": options,
}
if defaultVal, ok := inputDefMap["default"]; ok {
prop["default"] = defaultVal
}
return prop, true
}

// buildInputProperty builds a JSON Schema property map for a scalar input (string,
// number, or boolean). Use buildChoiceInputProperty for "choice" inputs instead.
func buildInputProperty(inputType, inputDescription string, inputDefMap map[string]any) map[string]any {
prop := map[string]any{
"type": inputType,
"description": inputDescription,
}
if defaultVal, ok := inputDefMap["default"]; ok {
prop["default"] = defaultVal
}
return prop
}
71 changes: 34 additions & 37 deletions pkg/workflow/codex_engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -584,43 +584,7 @@ func (e *CodexEngine) getShellEnvironmentPolicyVars(tools map[string]any, mcpToo

// Check each MCP tool for required environment variables
for _, toolName := range mcpTools {
switch toolName {
case "github":
// GitHub MCP server needs GITHUB_PERSONAL_ACCESS_TOKEN
envVars["GITHUB_PERSONAL_ACCESS_TOKEN"] = struct {
}{}
case "agentic-workflows":
// Agentic workflows MCP server needs GITHUB_TOKEN
envVars["GITHUB_TOKEN"] = struct {
}{}
case "safe-outputs":
// Safe outputs MCP server needs several environment variables
envVars["GH_AW_SAFE_OUTPUTS"] = struct {
}{}
envVars["GH_AW_ASSETS_BRANCH"] = struct {
}{}
envVars["GH_AW_ASSETS_MAX_SIZE_KB"] = struct {
}{}
envVars["GH_AW_ASSETS_ALLOWED_EXTS"] = struct {
}{}
envVars["GITHUB_REPOSITORY"] = struct {
}{}
envVars["GITHUB_SERVER_URL"] = struct {
}{}
default:
// For custom MCP tools, check if they have env configuration
if toolValue, ok := tools[toolName]; ok {
if toolConfig, ok := toolValue.(map[string]any); ok {
// Extract environment variable names from env configuration
if env, hasEnv := toolConfig["env"].(map[string]any); hasEnv {
for envKey := range env {
envVars[envKey] = struct {
}{}
}
}
}
}
}
addMCPToolEnvVars(toolName, tools, envVars)
}

sortedEnvVars := sliceutil.SortedKeys(envVars)
Expand All @@ -634,6 +598,39 @@ func (e *CodexEngine) getShellEnvironmentPolicyVars(tools map[string]any, mcpToo
return includeOnlyPatterns
}

// addMCPToolEnvVars adds the environment variables required by the named MCP tool
// to the envVars set. For custom tools, it reads the "env" configuration map.
func addMCPToolEnvVars(toolName string, tools map[string]any, envVars map[string]struct{}) {
switch toolName {
case "github":
// GitHub MCP server needs GITHUB_PERSONAL_ACCESS_TOKEN
envVars["GITHUB_PERSONAL_ACCESS_TOKEN"] = struct{}{}
case "agentic-workflows":
// Agentic workflows MCP server needs GITHUB_TOKEN
envVars["GITHUB_TOKEN"] = struct{}{}
case "safe-outputs":
// Safe outputs MCP server needs several environment variables
envVars["GH_AW_SAFE_OUTPUTS"] = struct{}{}
envVars["GH_AW_ASSETS_BRANCH"] = struct{}{}
envVars["GH_AW_ASSETS_MAX_SIZE_KB"] = struct{}{}
envVars["GH_AW_ASSETS_ALLOWED_EXTS"] = struct{}{}
envVars["GITHUB_REPOSITORY"] = struct{}{}
envVars["GITHUB_SERVER_URL"] = struct{}{}
default:
// For custom MCP tools, check if they have env configuration
if toolValue, ok := tools[toolName]; ok {
if toolConfig, ok := toolValue.(map[string]any); ok {
// Extract environment variable names from env configuration
if env, hasEnv := toolConfig["env"].(map[string]any); hasEnv {
for envKey := range env {
envVars[envKey] = struct{}{}
}
}
}
}
}
}

// renderShellEnvironmentPolicy generates the [shell_environment_policy] section for config.toml
// This controls which environment variables are passed through to MCP servers for security
func (e *CodexEngine) renderShellEnvironmentPolicy(yaml *strings.Builder, tools map[string]any, mcpTools []string) {
Comment thread
pelikhan marked this conversation as resolved.
Expand Down
Loading