Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 114 additions & 2 deletions pkg/parser/remote_fetch.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package parser

import (
"encoding/base64"
"encoding/json"
"fmt"
"os"
"os/exec"
Expand Down Expand Up @@ -197,7 +199,9 @@ func resolveRefToSHA(owner, repo, ref string) (string, error) {
if err != nil {
outputStr := string(output)
if strings.Contains(outputStr, "GH_TOKEN") || strings.Contains(outputStr, "authentication") || strings.Contains(outputStr, "not logged into") {
return "", fmt.Errorf("failed to resolve ref to SHA: GitHub authentication required. Please run 'gh auth login' or set GH_TOKEN/GITHUB_TOKEN environment variable: %w", err)
// Try fallback without authentication
remoteLog.Printf("gh CLI authentication failed, trying unauthenticated REST API fallback")
return resolveRefToSHAUnauthenticated(owner, repo, ref)
Comment on lines 199 to +204

Copilot AI Nov 16, 2025

Copy link

Choose a reason for hiding this comment

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

The fallback detection logic only checks for specific strings in error output. This approach may be fragile and could miss authentication failures with different error messages.

Issue: The string matching uses || (OR) logic, which means any single match triggers the fallback, but the condition checks three different patterns:

  • "GH_TOKEN" - could appear in various contexts
  • "authentication" - generic word that might appear in non-auth errors
  • "not logged into" - most specific pattern

Potential problem: If GitHub changes its error messages or if there's a different error that happens to contain one of these strings, the fallback could be triggered incorrectly.

Recommendation: Consider making the detection more specific by:

  1. Checking the error type or exit code first
  2. Using more specific error message patterns
  3. Adding logging to track when fallback is used vs. when authentication truly failed

See below for a potential fix:

		if isAuthFailure(outputStr) {
			// Try fallback without authentication
			remoteLog.Printf("gh CLI authentication failed (exit code: %v, output: %q), trying unauthenticated REST API fallback", err, strings.TrimSpace(outputStr))

Copilot uses AI. Check for mistakes.
}
return "", fmt.Errorf("failed to resolve ref %s to SHA for %s/%s: %s: %w", ref, owner, repo, strings.TrimSpace(outputStr), err)
}
Expand Down Expand Up @@ -236,7 +240,9 @@ func downloadFileFromGitHub(owner, repo, path, ref string) ([]byte, error) {
// Check if this is an authentication error
stderrStr := stderr.String()
if strings.Contains(stderrStr, "GH_TOKEN") || strings.Contains(stderrStr, "authentication") || strings.Contains(stderrStr, "not logged into") {
return nil, fmt.Errorf("failed to fetch file content: GitHub authentication required. Please run 'gh auth login' or set GH_TOKEN/GITHUB_TOKEN environment variable: %w", err)
// Try fallback without authentication
remoteLog.Printf("gh CLI authentication failed, trying unauthenticated REST API fallback")
return downloadFileFromGitHubUnauthenticated(owner, repo, path, ref)
}
return nil, fmt.Errorf("failed to fetch file content from %s/%s/%s@%s: %s: %w", owner, repo, path, ref, strings.TrimSpace(stderrStr), err)
}
Expand All @@ -256,3 +262,109 @@ func downloadFileFromGitHub(owner, repo, path, ref string) ([]byte, error) {

return content, nil
}

// resolveRefToSHAUnauthenticated resolves a git ref to SHA using unauthenticated REST API
// This is a fallback for when gh CLI authentication is not available
func resolveRefToSHAUnauthenticated(owner, repo, ref string) (string, error) {
remoteLog.Printf("Attempting to resolve ref %s to SHA for %s/%s using unauthenticated API", ref, owner, repo)

// Use curl to make unauthenticated request
// -f flag makes curl fail on HTTP errors (404, 500, etc.)
url := fmt.Sprintf("https://api.github.com/repos/%s/%s/commits/%s", owner, repo, ref)
cmd := exec.Command("curl", "-s", "-f", "-H", "Accept: application/vnd.github.v3+json", url)

output, err := cmd.CombinedOutput()
if err != nil {
return "", fmt.Errorf("failed to resolve ref using unauthenticated API: %w", err)
}

// Parse JSON response
var response struct {
SHA string `json:"sha"`
Message string `json:"message"`
}

if err := json.Unmarshal(output, &response); err != nil {
return "", fmt.Errorf("failed to parse JSON response: %w", err)
}

// Check for error message in response
if response.Message != "" {
if strings.Contains(response.Message, "Not Found") {
return "", fmt.Errorf("ref %s not found in %s/%s", ref, owner, repo)
}
if strings.Contains(response.Message, "rate limit") {
return "", fmt.Errorf("GitHub API rate limit exceeded")
}
return "", fmt.Errorf("GitHub API error: %s", response.Message)
}

// Validate it's a valid SHA (40 hex characters)
if len(response.SHA) != 40 || !isHexString(response.SHA) {
return "", fmt.Errorf("invalid SHA format returned: %s", response.SHA)
}

remoteLog.Printf("Successfully resolved ref %s to SHA %s using unauthenticated API", ref, response.SHA)
return response.SHA, nil
}

// downloadFileFromGitHubUnauthenticated downloads a file using unauthenticated REST API
// This is a fallback for when gh CLI authentication is not available
func downloadFileFromGitHubUnauthenticated(owner, repo, path, ref string) ([]byte, error) {
remoteLog.Printf("Attempting to download %s/%s/%s@%s using unauthenticated API", owner, repo, path, ref)

// Use curl to make unauthenticated request
// -f flag makes curl fail on HTTP errors (404, 500, etc.)
url := fmt.Sprintf("https://api.github.com/repos/%s/%s/contents/%s?ref=%s", owner, repo, path, ref)
cmd := exec.Command("curl", "-s", "-f", "-H", "Accept: application/vnd.github.v3+json", url)

output, err := cmd.CombinedOutput()
if err != nil {
return nil, fmt.Errorf("failed to fetch file using unauthenticated API: %w", err)
}

// Parse JSON response
var response struct {
Content string `json:"content"`
Encoding string `json:"encoding"`
Message string `json:"message"`
}

if err := json.Unmarshal(output, &response); err != nil {
return nil, fmt.Errorf("failed to parse JSON response: %w", err)
}

// Check for error message in response
if response.Message != "" {
if strings.Contains(response.Message, "Not Found") {
return nil, fmt.Errorf("file %s not found in %s/%s@%s", path, owner, repo, ref)
}
if strings.Contains(response.Message, "rate limit") {
return nil, fmt.Errorf("GitHub API rate limit exceeded")
}
return nil, fmt.Errorf("GitHub API error: %s", response.Message)
}

// Verify encoding
if response.Encoding != "base64" {
return nil, fmt.Errorf("unexpected encoding: %s (expected base64)", response.Encoding)
}

// Remove newlines and whitespace from base64 content
contentBase64 := strings.ReplaceAll(response.Content, "\n", "")
contentBase64 = strings.ReplaceAll(contentBase64, " ", "")
contentBase64 = strings.TrimSpace(contentBase64)

if contentBase64 == "" {
return nil, fmt.Errorf("empty content returned from GitHub API")
}

// Decode base64 content using Go's standard library (more portable than external base64 command)
content, err := base64.StdEncoding.DecodeString(contentBase64)
if err != nil {
return nil, fmt.Errorf("failed to decode base64 content: %w", err)
}

remoteLog.Printf("Successfully downloaded %s/%s/%s@%s using unauthenticated API (%d bytes)", owner, repo, path, ref, len(content))
return content, nil
}
71 changes: 71 additions & 0 deletions pkg/parser/remote_fetch_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package parser

import (
"encoding/json"
"testing"
)

func TestJSONParsing(t *testing.T) {
// Test SHA resolution JSON parsing
t.Run("parse SHA from commit response", func(t *testing.T) {
response := `{"sha":"1e366aa4518cf83d25defd84e454b9a41e87cf7c","node_id":"C_kwDOKr1234","commit":{"message":"test"}}`

var parsed struct {
SHA string `json:"sha"`
Message string `json:"message"`
}

if err := json.Unmarshal([]byte(response), &parsed); err != nil {
t.Fatalf("Failed to parse JSON: %v", err)
}

if parsed.SHA != "1e366aa4518cf83d25defd84e454b9a41e87cf7c" {
t.Errorf("Expected SHA 1e366aa4518cf83d25defd84e454b9a41e87cf7c, got %s", parsed.SHA)
}
})

// Test file content JSON parsing
t.Run("parse content from file response", func(t *testing.T) {
response := `{"content":"IyBUZXN0IGNvbnRlbnQ=\n","encoding":"base64","name":"test.md"}`

var parsed struct {
Content string `json:"content"`
Encoding string `json:"encoding"`
Message string `json:"message"`
}

if err := json.Unmarshal([]byte(response), &parsed); err != nil {
t.Fatalf("Failed to parse JSON: %v", err)
}

if parsed.Encoding != "base64" {
t.Errorf("Expected encoding base64, got %s", parsed.Encoding)
}

if parsed.Content == "" {
t.Error("Expected non-empty content")
}
})

// Test error response parsing
t.Run("parse error response", func(t *testing.T) {
response := `{"message":"Not Found","documentation_url":"https://docs.github.com/rest"}`

var parsed struct {
SHA string `json:"sha"`
Message string `json:"message"`
}

if err := json.Unmarshal([]byte(response), &parsed); err != nil {
t.Fatalf("Failed to parse JSON: %v", err)
}

if parsed.Message != "Not Found" {
t.Errorf("Expected message 'Not Found', got %s", parsed.Message)
}

if parsed.SHA != "" {
t.Errorf("Expected empty SHA for error response, got %s", parsed.SHA)
}
})
}
4 changes: 2 additions & 2 deletions pkg/workflow/action_pins.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func getActionPins() []ActionPin {

// GetActionPin returns the pinned action reference for a given action repository
// It uses the golden/default version defined in actionPins
// If no pin is found, it returns an empty string
// If no pin is found, it returns an empty string (e.g., for custom action repos)
// The returned reference includes a comment with the version tag (e.g., "repo@sha # v1")
func GetActionPin(actionRepo string) string {
actionPins := getActionPins()
Expand All @@ -77,7 +77,7 @@ func GetActionPin(actionRepo string) string {
return actionRepo + "@" + pin.SHA + " # " + pin.Version
}
}
// If no pin exists, return empty string to signal that this action is not pinned
// If no pin exists, return empty string - caller should handle this case
return ""
}

Expand Down
42 changes: 36 additions & 6 deletions pkg/workflow/compiler_jobs.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,16 @@ import (
// These functions are responsible for constructing the various jobs that make up
// a compiled agentic workflow, including activation, main, safe outputs, and custom jobs.

// getRequiredActionPin gets an action pin and returns an error if it's not found
// This ensures that required system actions always have pins defined
func getRequiredActionPin(actionRepo string) (string, error) {
pin := GetActionPin(actionRepo)
if pin == "" {
return "", fmt.Errorf("action pin not found for %s - this action must be added to .github/aw/actions-lock.json", actionRepo)
}
return pin, nil
}

Comment on lines +18 to +27

Copilot AI Nov 16, 2025

Copy link

Choose a reason for hiding this comment

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

The new getRequiredActionPin function only validates system actions (actions/checkout and actions/github-script), but this pattern is inconsistent with many other places in the codebase where GetActionPin is called directly without validation.

Issue: Many other critical actions lack validation:

  • actions/cache (cache.go, lines 242, 349)
  • actions/upload-artifact (compiler_yaml.go, artifacts.go, etc.)
  • actions/download-artifact (artifacts.go, line 38)
  • actions/checkout (compiler_yaml.go, line 174)
  • actions/github-script (compiler_yaml.go, multiple locations)

Why this matters: If action pins are missing, these would produce empty uses: fields, causing compilation errors.

Recommendation: Either:

  1. Apply getRequiredActionPin consistently to all system actions throughout the codebase, OR
  2. Remove this validation since GetActionPin is designed to return empty string for missing pins (which is the expected behavior for custom action repos per runtime_setup.go:357-367)

Given the codebase pattern where GetActionPin is designed to gracefully return empty string for custom actions, the current selective validation appears to be an over-engineered solution.

Suggested change
// getRequiredActionPin gets an action pin and returns an error if it's not found
// This ensures that required system actions always have pins defined
func getRequiredActionPin(actionRepo string) (string, error) {
pin := GetActionPin(actionRepo)
if pin == "" {
return "", fmt.Errorf("action pin not found for %s - this action must be added to .github/aw/actions-lock.json", actionRepo)
}
return pin, nil
}

Copilot uses AI. Check for mistakes.
func (c *Compiler) isActivationJobNeeded() bool {
// Activation job is always needed to perform the timestamp check
// It also handles:
Expand Down Expand Up @@ -405,9 +415,14 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec
// Extract workflow name for the stop-time check
workflowName := data.Name

githubScriptPin, err := getRequiredActionPin("actions/github-script")
if err != nil {
return nil, err
}

steps = append(steps, " - name: Check stop-time limit\n")
steps = append(steps, fmt.Sprintf(" id: %s\n", constants.CheckStopTimeStepID))
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", githubScriptPin))
steps = append(steps, " env:\n")
steps = append(steps, fmt.Sprintf(" GH_AW_STOP_TIME: %s\n", data.StopTime))
steps = append(steps, fmt.Sprintf(" GH_AW_WORKFLOW_NAME: %q\n", workflowName))
Expand All @@ -421,9 +436,14 @@ func (c *Compiler) buildPreActivationJob(data *WorkflowData, needsPermissionChec

// Add command position check if this is a command workflow
if data.Command != "" {
githubScriptPin, err := getRequiredActionPin("actions/github-script")
if err != nil {
return nil, err
}

steps = append(steps, " - name: Check command position\n")
steps = append(steps, fmt.Sprintf(" id: %s\n", constants.CheckCommandPositionStepID))
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", githubScriptPin))
steps = append(steps, " env:\n")
steps = append(steps, fmt.Sprintf(" GH_AW_COMMAND: %s\n", data.Command))
steps = append(steps, " with:\n")
Expand Down Expand Up @@ -518,10 +538,20 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate
// Team member check is now handled by the separate check_membership job
// No inline role checks needed in the task job anymore

checkoutPin, err := getRequiredActionPin("actions/checkout")
if err != nil {
return nil, err
}

githubScriptPin, err := getRequiredActionPin("actions/github-script")
if err != nil {
return nil, err
}

// Add shallow checkout for timestamp check
// Only checkout .github/workflows directory for minimal performance impact
steps = append(steps, " - name: Checkout workflows\n")
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/checkout")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", checkoutPin))
steps = append(steps, " with:\n")
steps = append(steps, " sparse-checkout: |\n")
steps = append(steps, " .github/workflows\n")
Expand All @@ -531,7 +561,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate

// Add timestamp check for lock file vs source file
steps = append(steps, " - name: Check workflow file timestamps\n")
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", githubScriptPin))
steps = append(steps, " env:\n")
steps = append(steps, fmt.Sprintf(" GH_AW_WORKFLOW_FILE: \"%s\"\n", lockFilename))
steps = append(steps, " with:\n")
Expand All @@ -545,7 +575,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate
if data.NeedsTextOutput {
steps = append(steps, " - name: Compute current body text\n")
steps = append(steps, " id: compute-text\n")
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", githubScriptPin))
steps = append(steps, " with:\n")
steps = append(steps, " script: |\n")

Expand All @@ -563,7 +593,7 @@ func (c *Compiler) buildActivationJob(data *WorkflowData, preActivationJobCreate
steps = append(steps, fmt.Sprintf(" - name: Add %s reaction to the triggering item\n", data.AIReaction))
steps = append(steps, " id: react\n")
steps = append(steps, fmt.Sprintf(" if: %s\n", reactionCondition.Render()))
steps = append(steps, fmt.Sprintf(" uses: %s\n", GetActionPin("actions/github-script")))
steps = append(steps, fmt.Sprintf(" uses: %s\n", githubScriptPin))

// Add environment variables
steps = append(steps, " env:\n")
Expand Down
Loading
Loading