From 137528b764aab6571c31f795ad010b58880f4fee Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Dec 2025 22:22:44 -0800 Subject: [PATCH 01/10] Replace custom JSON handler with StreamableHTTPHandler in mcp-gateway (#7585) --- .github/workflows/issue-classifier.lock.yml | 2 +- .github/workflows/release.lock.yml | 6 +- .../workflows/stale-repo-identifier.lock.yml | 2 +- .github/workflows/super-linter.lock.yml | 2 +- pkg/awmg/gateway.go | 305 +++++++++--------- pkg/awmg/gateway_streamable_http_test.go | 219 ++++++++++--- 6 files changed, 330 insertions(+), 206 deletions(-) diff --git a/.github/workflows/issue-classifier.lock.yml b/.github/workflows/issue-classifier.lock.yml index 86c01e6d76f..a22d440eb5c 100644 --- a/.github/workflows/issue-classifier.lock.yml +++ b/.github/workflows/issue-classifier.lock.yml @@ -2211,7 +2211,7 @@ jobs: path: /tmp/gh-aw/aw_info.json if-no-files-found: warn - name: Run AI Inference - uses: actions/ai-inference@334892bb203895caaed82ec52d23c1ed9385151e # v1 + uses: actions/ai-inference@334892bb203895caaed82ec52d23c1ed9385151e # v2.0.4 env: GH_AW_MCP_CONFIG: /tmp/gh-aw/mcp-config/mcp-servers.json GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt diff --git a/.github/workflows/release.lock.yml b/.github/workflows/release.lock.yml index f5a0cc548d6..6f307c8c770 100644 --- a/.github/workflows/release.lock.yml +++ b/.github/workflows/release.lock.yml @@ -6031,13 +6031,13 @@ jobs: - name: Download Go modules run: go mod download - name: Generate SBOM (SPDX format) - uses: anchore/sbom-action@43a17d6e7add2b5535efe4dcae9952337c479a93 # v0.20.10 + uses: anchore/sbom-action@43a17d6e7add2b5535efe4dcae9952337c479a93 # v0.20.11 with: artifact-name: sbom.spdx.json format: spdx-json output-file: sbom.spdx.json - name: Generate SBOM (CycloneDX format) - uses: anchore/sbom-action@43a17d6e7add2b5535efe4dcae9952337c479a93 # v0.20.10 + uses: anchore/sbom-action@43a17d6e7add2b5535efe4dcae9952337c479a93 # v0.20.11 with: artifact-name: sbom.cdx.json format: cyclonedx-json @@ -6244,7 +6244,7 @@ jobs: fetch-depth: 0 persist-credentials: false - name: Release with gh-extension-precompile - uses: cli/gh-extension-precompile@9e2237c30f869ad3bcaed6a4be2cd43564dd421b # v2 + uses: cli/gh-extension-precompile@9e2237c30f869ad3bcaed6a4be2cd43564dd421b # v2.1.0 with: build_script_override: scripts/build-release.sh go_version_file: go.mod diff --git a/.github/workflows/stale-repo-identifier.lock.yml b/.github/workflows/stale-repo-identifier.lock.yml index a2dc0fc2462..2917200924f 100644 --- a/.github/workflows/stale-repo-identifier.lock.yml +++ b/.github/workflows/stale-repo-identifier.lock.yml @@ -176,7 +176,7 @@ jobs: ORGANIZATION: ${{ env.ORGANIZATION }} id: stale-repos name: Run stale_repos tool - uses: github/stale-repos@a21e55567b83cf3c3f3f9085d3038dc6cee02598 # v3 + uses: github/stale-repos@a21e55567b83cf3c3f3f9085d3038dc6cee02598 # v3.0.2 - env: INACTIVE_REPOS: ${{ steps.stale-repos.outputs.inactiveRepos }} name: Save stale repos output diff --git a/.github/workflows/super-linter.lock.yml b/.github/workflows/super-linter.lock.yml index 27afaf48be3..2324c85f388 100644 --- a/.github/workflows/super-linter.lock.yml +++ b/.github/workflows/super-linter.lock.yml @@ -6161,7 +6161,7 @@ jobs: persist-credentials: false - name: Super-linter id: super-linter - uses: super-linter/super-linter@47984f49b4e87383eed97890fe2dca6063bbd9c3 # v8.2.1 + uses: super-linter/super-linter@47984f49b4e87383eed97890fe2dca6063bbd9c3 # v8.3.1 env: CREATE_LOG_FILE: "true" DEFAULT_BRANCH: main diff --git a/pkg/awmg/gateway.go b/pkg/awmg/gateway.go index 567ccfca4f5..40fcdd1246f 100644 --- a/pkg/awmg/gateway.go +++ b/pkg/awmg/gateway.go @@ -58,6 +58,7 @@ type GatewaySettings struct { type MCPGatewayServer struct { config *MCPGatewayConfig sessions map[string]*mcp.ClientSession + servers map[string]*mcp.Server // Proxy servers for each session mu sync.RWMutex logDir string } @@ -148,6 +149,7 @@ func runMCPGateway(configFiles []string, port int, logDir string) error { gateway := &MCPGatewayServer{ config: config, sessions: make(map[string]*mcp.ClientSession), + servers: make(map[string]*mcp.Server), logDir: logDir, } @@ -489,6 +491,12 @@ func (g *MCPGatewayServer) initializeSessions() error { g.sessions[serverName] = session g.mu.Unlock() + // Create a proxy MCP server that forwards calls to this session + proxyServer := g.createProxyServer(serverName, session) + g.mu.Lock() + g.servers[serverName] = proxyServer + g.mu.Unlock() + successCount++ gatewayLog.Printf("Successfully initialized session for %s (%d/%d)", serverName, successCount, len(g.config.MCPServers)) fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Successfully initialized session for %s (%d/%d)", serverName, successCount, len(g.config.MCPServers)))) @@ -604,6 +612,100 @@ func (g *MCPGatewayServer) createMCPSession(serverName string, config MCPServerC return nil, fmt.Errorf("invalid server configuration: must specify command, url, or container") } +// createProxyServer creates a proxy MCP server that forwards all calls to the backend session +func (g *MCPGatewayServer) createProxyServer(serverName string, session *mcp.ClientSession) *mcp.Server { + gatewayLog.Printf("Creating proxy MCP server for %s", serverName) + + // Create a server that will proxy requests to the backend session + server := mcp.NewServer(&mcp.Implementation{ + Name: fmt.Sprintf("gateway-proxy-%s", serverName), + Version: GetVersion(), + }, &mcp.ServerOptions{ + Capabilities: &mcp.ServerCapabilities{ + Tools: &mcp.ToolCapabilities{ + ListChanged: false, + }, + Resources: &mcp.ResourceCapabilities{ + Subscribe: false, + ListChanged: false, + }, + Prompts: &mcp.PromptCapabilities{ + ListChanged: false, + }, + }, + Logger: logger.NewSlogLoggerWithHandler(gatewayLog), + }) + + // Query backend for its tools and register them on the proxy server + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + // List tools from backend + toolsResult, err := session.ListTools(ctx, &mcp.ListToolsParams{}) + if err != nil { + gatewayLog.Printf("Warning: Failed to list tools from backend %s: %v", serverName, err) + } else { + // Register each tool on the proxy server + for _, tool := range toolsResult.Tools { + toolCopy := tool // Capture for closure + gatewayLog.Printf("Registering tool %s from backend %s", tool.Name, serverName) + + server.AddTool(toolCopy, func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + gatewayLog.Printf("Proxy %s: Calling tool %s on backend", serverName, req.Params.Name) + return session.CallTool(ctx, &mcp.CallToolParams{ + Name: req.Params.Name, + Arguments: req.Params.Arguments, + }) + }) + } + gatewayLog.Printf("Registered %d tools from backend %s", len(toolsResult.Tools), serverName) + } + + // List resources from backend + resourcesResult, err := session.ListResources(ctx, &mcp.ListResourcesParams{}) + if err != nil { + gatewayLog.Printf("Warning: Failed to list resources from backend %s: %v", serverName, err) + } else { + // Register each resource on the proxy server + for _, resource := range resourcesResult.Resources { + resourceCopy := resource // Capture for closure + gatewayLog.Printf("Registering resource %s from backend %s", resource.URI, serverName) + + server.AddResource(resourceCopy, func(ctx context.Context, req *mcp.ReadResourceRequest) (*mcp.ReadResourceResult, error) { + gatewayLog.Printf("Proxy %s: Reading resource %s from backend", serverName, req.Params.URI) + return session.ReadResource(ctx, &mcp.ReadResourceParams{ + URI: req.Params.URI, + }) + }) + } + gatewayLog.Printf("Registered %d resources from backend %s", len(resourcesResult.Resources), serverName) + } + + // List prompts from backend + promptsResult, err := session.ListPrompts(ctx, &mcp.ListPromptsParams{}) + if err != nil { + gatewayLog.Printf("Warning: Failed to list prompts from backend %s: %v", serverName, err) + } else { + // Register each prompt on the proxy server + for _, prompt := range promptsResult.Prompts { + promptCopy := prompt // Capture for closure + gatewayLog.Printf("Registering prompt %s from backend %s", prompt.Name, serverName) + + server.AddPrompt(promptCopy, func(ctx context.Context, req *mcp.GetPromptRequest) (*mcp.GetPromptResult, error) { + gatewayLog.Printf("Proxy %s: Getting prompt %s from backend", serverName, req.Params.Name) + return session.GetPrompt(ctx, &mcp.GetPromptParams{ + Name: req.Params.Name, + Arguments: req.Params.Arguments, + }) + }) + } + gatewayLog.Printf("Registered %d prompts from backend %s", len(promptsResult.Prompts), serverName) + } + + gatewayLog.Printf("Proxy MCP server created for %s", serverName) + return server +} + // startHTTPServer starts the HTTP server for the gateway func (g *MCPGatewayServer) startHTTPServer() error { port := g.config.Gateway.Port @@ -617,41 +719,62 @@ func (g *MCPGatewayServer) startHTTPServer() error { fmt.Fprintf(w, "OK") }) - // MCP endpoint for each server + // List servers endpoint + mux.HandleFunc("/servers", func(w http.ResponseWriter, r *http.Request) { + g.handleListServers(w, r) + }) + + // Create StreamableHTTPHandler for each MCP server for serverName := range g.config.MCPServers { serverNameCopy := serverName // Capture for closure path := fmt.Sprintf("/mcp/%s", serverName) - gatewayLog.Printf("Registering endpoint: %s", path) - - mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { - g.handleMCPRequest(w, r, serverNameCopy) + gatewayLog.Printf("Registering StreamableHTTPHandler endpoint: %s", path) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf("Registering StreamableHTTPHandler endpoint: %s", path))) + + // Create streamable HTTP handler for this server + handler := mcp.NewStreamableHTTPHandler(func(req *http.Request) *mcp.Server { + // Get the proxy server for this backend + g.mu.RLock() + defer g.mu.RUnlock() + server, exists := g.servers[serverNameCopy] + if !exists { + gatewayLog.Printf("Server not found in handler: %s", serverNameCopy) + return nil + } + gatewayLog.Printf("Returning proxy server for: %s", serverNameCopy) + return server + }, &mcp.StreamableHTTPOptions{ + SessionTimeout: 2 * time.Hour, // Close idle sessions after 2 hours + Logger: logger.NewSlogLoggerWithHandler(gatewayLog), }) - } - // List servers endpoint - mux.HandleFunc("/servers", func(w http.ResponseWriter, r *http.Request) { - g.handleListServers(w, r) - }) + // Add authentication middleware if API key is configured + if g.config.Gateway.APIKey != "" { + wrappedHandler := g.withAuth(handler, serverNameCopy) + mux.Handle(path, wrappedHandler) + } else { + mux.Handle(path, handler) + } + } - server := &http.Server{ - Addr: fmt.Sprintf(":%d", port), - Handler: mux, - ReadTimeout: 30 * time.Second, - WriteTimeout: 30 * time.Second, + httpServer := &http.Server{ + Addr: fmt.Sprintf(":%d", port), + Handler: mux, + ReadHeaderTimeout: 30 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, } fmt.Fprintf(os.Stderr, "%s\n", console.FormatSuccessMessage(fmt.Sprintf("MCP gateway listening on http://localhost:%d", port))) - gatewayLog.Printf("HTTP server ready on port %d", port) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Using StreamableHTTPHandler for MCP protocol")) + gatewayLog.Printf("HTTP server ready on port %d with StreamableHTTPHandler", port) - return server.ListenAndServe() + return httpServer.ListenAndServe() } -// handleMCPRequest handles an MCP protocol request for a specific server -func (g *MCPGatewayServer) handleMCPRequest(w http.ResponseWriter, r *http.Request, serverName string) { - gatewayLog.Printf("Handling MCP request for server: %s", serverName) - - // Check API key if configured - if g.config.Gateway.APIKey != "" { +// withAuth wraps an HTTP handler with authentication if API key is configured +func (g *MCPGatewayServer) withAuth(handler http.Handler, serverName string) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { authHeader := r.Header.Get("Authorization") expectedAuth := fmt.Sprintf("Bearer %s", g.config.Gateway.APIKey) if authHeader != expectedAuth { @@ -659,142 +782,8 @@ func (g *MCPGatewayServer) handleMCPRequest(w http.ResponseWriter, r *http.Reque http.Error(w, "Unauthorized", http.StatusUnauthorized) return } - } - - // Get the session - g.mu.RLock() - session, exists := g.sessions[serverName] - g.mu.RUnlock() - - if !exists { - gatewayLog.Printf("Server not found: %s", serverName) - http.Error(w, fmt.Sprintf("Server not found: %s", serverName), http.StatusNotFound) - return - } - - // Parse request body - var reqBody map[string]any - if err := json.NewDecoder(r.Body).Decode(&reqBody); err != nil { - gatewayLog.Printf("Failed to decode request: %v", err) - http.Error(w, "Invalid request body", http.StatusBadRequest) - return - } - - method, _ := reqBody["method"].(string) - gatewayLog.Printf("MCP method: %s for server: %s", method, serverName) - - // Handle different MCP methods - var response any - var err error - - switch method { - case "initialize": - response, err = g.handleInitialize(session) - case "tools/list": - response, err = g.handleListTools(session) - case "tools/call": - response, err = g.handleCallTool(session, reqBody) - case "resources/list": - response, err = g.handleListResources(session) - case "prompts/list": - response, err = g.handleListPrompts(session) - default: - gatewayLog.Printf("Unsupported method: %s", method) - http.Error(w, fmt.Sprintf("Unsupported method: %s", method), http.StatusBadRequest) - return - } - - if err != nil { - gatewayLog.Printf("Error handling %s: %v", method, err) - http.Error(w, fmt.Sprintf("Error: %v", err), http.StatusInternalServerError) - return - } - - // Send response - w.Header().Set("Content-Type", "application/json") - if err := json.NewEncoder(w).Encode(response); err != nil { - gatewayLog.Printf("Failed to encode JSON response: %v", err) - } -} - -// handleInitialize handles the initialize method -func (g *MCPGatewayServer) handleInitialize(session *mcp.ClientSession) (any, error) { - // Return server capabilities - return map[string]any{ - "protocolVersion": "2024-11-05", - "capabilities": map[string]any{ - "tools": map[string]any{}, - "resources": map[string]any{}, - "prompts": map[string]any{}, - }, - "serverInfo": map[string]any{ - "name": "mcp-gateway", - "version": GetVersion(), - }, - }, nil -} - -// handleListTools handles the tools/list method -func (g *MCPGatewayServer) handleListTools(session *mcp.ClientSession) (any, error) { - ctx := context.Background() - result, err := session.ListTools(ctx, &mcp.ListToolsParams{}) - if err != nil { - return nil, fmt.Errorf("failed to list tools: %w", err) - } - - return map[string]any{ - "tools": result.Tools, - }, nil -} - -// handleCallTool handles the tools/call method -func (g *MCPGatewayServer) handleCallTool(session *mcp.ClientSession, reqBody map[string]any) (any, error) { - params, ok := reqBody["params"].(map[string]any) - if !ok { - return nil, fmt.Errorf("invalid params") - } - - name, _ := params["name"].(string) - arguments := params["arguments"] - - ctx := context.Background() - result, err := session.CallTool(ctx, &mcp.CallToolParams{ - Name: name, - Arguments: arguments, + handler.ServeHTTP(w, r) }) - if err != nil { - return nil, fmt.Errorf("failed to call tool: %w", err) - } - - return map[string]any{ - "content": result.Content, - }, nil -} - -// handleListResources handles the resources/list method -func (g *MCPGatewayServer) handleListResources(session *mcp.ClientSession) (any, error) { - ctx := context.Background() - result, err := session.ListResources(ctx, &mcp.ListResourcesParams{}) - if err != nil { - return nil, fmt.Errorf("failed to list resources: %w", err) - } - - return map[string]any{ - "resources": result.Resources, - }, nil -} - -// handleListPrompts handles the prompts/list method -func (g *MCPGatewayServer) handleListPrompts(session *mcp.ClientSession) (any, error) { - ctx := context.Background() - result, err := session.ListPrompts(ctx, &mcp.ListPromptsParams{}) - if err != nil { - return nil, fmt.Errorf("failed to list prompts: %w", err) - } - - return map[string]any{ - "prompts": result.Prompts, - }, nil } // handleListServers handles the /servers endpoint diff --git a/pkg/awmg/gateway_streamable_http_test.go b/pkg/awmg/gateway_streamable_http_test.go index bbec08c96cf..8e55f135735 100644 --- a/pkg/awmg/gateway_streamable_http_test.go +++ b/pkg/awmg/gateway_streamable_http_test.go @@ -104,62 +104,48 @@ func TestStreamableHTTPTransport_GatewayConnection(t *testing.T) { } t.Logf("✓ Gateway has %d server(s): %v", len(servers), servers) - // Test 2: Test the MCP endpoint directly using HTTP POST + // Test 2: Connect to the MCP endpoint using StreamableClientTransport mcpURL := "http://localhost:8091/mcp/gh-aw" - t.Logf("Testing MCP endpoint: %s", mcpURL) + t.Logf("Testing MCP endpoint with StreamableClientTransport: %s", mcpURL) - // Send initialize request - initReq := map[string]any{ - "method": "initialize", - "params": map[string]any{}, + // Create streamable client transport + transport := &mcp.StreamableClientTransport{ + Endpoint: mcpURL, } - initReqJSON, _ := json.Marshal(initReq) - resp, err := http.Post(mcpURL, "application/json", strings.NewReader(string(initReqJSON))) - if err != nil { - t.Fatalf("Failed to send initialize request: %v", err) - } - defer resp.Body.Close() + // Create MCP client + client := mcp.NewClient(&mcp.Implementation{ + Name: "test-client", + Version: "1.0.0", + }, nil) - if resp.StatusCode != http.StatusOK { - t.Fatalf("Initialize request failed: status=%d", resp.StatusCode) - } + // Connect to the gateway + connectCtx, connectCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer connectCancel() - var initResponse map[string]any - if err := json.NewDecoder(resp.Body).Decode(&initResponse); err != nil { - t.Fatalf("Failed to decode initialize response: %v", err) + session, err := client.Connect(connectCtx, transport, nil) + if err != nil { + cancel() + t.Fatalf("Failed to connect via StreamableClientTransport: %v", err) } - t.Logf("✓ Initialize response: %v", initResponse) + defer session.Close() - // Test 3: Send tools/list request - listToolsReq := map[string]any{ - "method": "tools/list", - "params": map[string]any{}, - } - listToolsReqJSON, _ := json.Marshal(listToolsReq) + t.Log("✓ Successfully connected via StreamableClientTransport") - toolsResp, err := http.Post(mcpURL, "application/json", strings.NewReader(string(listToolsReqJSON))) - if err != nil { - t.Fatalf("Failed to send tools/list request: %v", err) - } - defer toolsResp.Body.Close() + // Test listing tools + toolsCtx, toolsCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer toolsCancel() - if toolsResp.StatusCode != http.StatusOK { - t.Fatalf("tools/list request failed: status=%d", toolsResp.StatusCode) + toolsResult, err := session.ListTools(toolsCtx, &mcp.ListToolsParams{}) + if err != nil { + t.Fatalf("Failed to list tools: %v", err) } - var toolsResponse map[string]any - if err := json.NewDecoder(toolsResp.Body).Decode(&toolsResponse); err != nil { - t.Fatalf("Failed to decode tools/list response: %v", err) + if len(toolsResult.Tools) == 0 { + t.Error("Expected at least one tool from backend") } - t.Logf("✓ Tools/list response received with %d tools", len(toolsResponse)) - // Verify the response contains tools array - if tools, ok := toolsResponse["tools"].([]any); ok { - t.Logf("✓ Found %d tools in response", len(tools)) - } else { - t.Logf("Note: Tools response format: %v", toolsResponse) - } + t.Logf("✓ Found %d tools from backend via gateway", len(toolsResult.Tools)) t.Log("✓ All streamable HTTP transport tests completed successfully") @@ -438,3 +424,152 @@ This workflow tests the streamable HTTP transport via mcp inspect. t.Log("✓ MCP inspect test for streamable HTTP completed successfully") } + +// TestStreamableHTTPTransport_GatewayWithSDKClient tests that the gateway properly +// exposes backend servers via StreamableHTTPHandler and that we can connect to them +// using the go-sdk StreamableClientTransport. +func TestStreamableHTTPTransport_GatewayWithSDKClient(t *testing.T) { + // Get absolute path to binary + binaryPath, err := filepath.Abs(filepath.Join("..", "..", "gh-aw")) + if err != nil { + t.Fatalf("Failed to get absolute path: %v", err) + } + + if _, err := os.Stat(binaryPath); os.IsNotExist(err) { + t.Skipf("Skipping test: gh-aw binary not found at %s. Run 'make build' first.", binaryPath) + } + + // Create temporary directory for config + tmpDir := t.TempDir() + configFile := filepath.Join(tmpDir, "gateway-config.json") + + // Create gateway config with the gh-aw MCP server + config := MCPGatewayConfig{ + MCPServers: map[string]MCPServerConfig{ + "gh-aw": { + Command: binaryPath, + Args: []string{"mcp-server"}, + }, + }, + Gateway: GatewaySettings{ + Port: 8092, // Use a different port to avoid conflicts + }, + } + + configJSON, err := json.Marshal(config) + if err != nil { + t.Fatalf("Failed to marshal config: %v", err) + } + + if err := os.WriteFile(configFile, configJSON, 0644); err != nil { + t.Fatalf("Failed to write config file: %v", err) + } + + // Start the gateway in background + _, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + gatewayErrChan := make(chan error, 1) + go func() { + gatewayErrChan <- runMCPGateway([]string{configFile}, 8092, tmpDir) + }() + + // Wait for gateway to start + t.Log("Waiting for MCP gateway to start...") + time.Sleep(3 * time.Second) + + // Verify gateway health + healthResp, err := http.Get("http://localhost:8092/health") + if err != nil { + cancel() + t.Fatalf("Failed to connect to gateway health endpoint: %v", err) + } + healthResp.Body.Close() + + if healthResp.StatusCode != http.StatusOK { + cancel() + t.Fatalf("Gateway health check failed: status=%d", healthResp.StatusCode) + } + t.Log("✓ Gateway health check passed") + + // Now test connecting to the gateway using StreamableClientTransport + gatewayURL := "http://localhost:8092/mcp/gh-aw" + t.Logf("Connecting to gateway via StreamableClientTransport: %s", gatewayURL) + + // Create streamable client transport + transport := &mcp.StreamableClientTransport{ + Endpoint: gatewayURL, + } + + // Create MCP client + client := mcp.NewClient(&mcp.Implementation{ + Name: "test-client", + Version: "1.0.0", + }, nil) + + // Connect to the gateway + connectCtx, connectCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer connectCancel() + + session, err := client.Connect(connectCtx, transport, nil) + if err != nil { + cancel() + t.Fatalf("Failed to connect to gateway via StreamableClientTransport: %v", err) + } + defer session.Close() + + t.Log("✓ Successfully connected to gateway via StreamableClientTransport") + + // Test listing tools + toolsCtx, toolsCancel := context.WithTimeout(context.Background(), 10*time.Second) + defer toolsCancel() + + toolsResult, err := session.ListTools(toolsCtx, &mcp.ListToolsParams{}) + if err != nil { + t.Fatalf("Failed to list tools: %v", err) + } + + if len(toolsResult.Tools) == 0 { + t.Error("Expected at least one tool from gh-aw MCP server") + } + + t.Logf("✓ Successfully listed %d tools from backend via gateway", len(toolsResult.Tools)) + for i, tool := range toolsResult.Tools { + if i < 3 { // Log first 3 tools + t.Logf(" - %s: %s", tool.Name, tool.Description) + } + } + + // Test calling a tool (status tool should be available) + callCtx, callCancel := context.WithTimeout(context.Background(), 30*time.Second) + defer callCancel() + + // Create a simple test by calling the status tool + callResult, err := session.CallTool(callCtx, &mcp.CallToolParams{ + Name: "status", + Arguments: map[string]any{}, + }) + if err != nil { + t.Logf("Note: Failed to call status tool (may not be in test environment): %v", err) + } else { + t.Logf("✓ Successfully called status tool via gateway") + if len(callResult.Content) > 0 { + t.Logf(" Tool returned %d content items", len(callResult.Content)) + } + } + + t.Log("✓ All StreamableHTTPHandler gateway tests completed successfully") + + // Clean up + cancel() + + // Wait for gateway to stop + select { + case err := <-gatewayErrChan: + if err != nil && err != http.ErrServerClosed && !strings.Contains(err.Error(), "context canceled") { + t.Logf("Gateway stopped with error: %v", err) + } + case <-time.After(3 * time.Second): + t.Log("Gateway shutdown timed out") + } +} From 7a3e1e790a75052edbe1e5455ec855588ee20948 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 24 Dec 2025 22:23:44 -0800 Subject: [PATCH 02/10] Add oneOf schema constraints for mutually exclusive field pairs (#7583) --- .github/aw/schemas/agentic-workflow.json | 158 ++++++++- pkg/parser/schema_oneof_test.go | 332 +++++++++++++++++++ pkg/parser/schemas/main_workflow_schema.json | 158 ++++++++- 3 files changed, 638 insertions(+), 10 deletions(-) create mode 100644 pkg/parser/schema_oneof_test.go diff --git a/.github/aw/schemas/agentic-workflow.json b/.github/aw/schemas/agentic-workflow.json index f8f6e5c73ac..510479189b5 100644 --- a/.github/aw/schemas/agentic-workflow.json +++ b/.github/aw/schemas/agentic-workflow.json @@ -273,7 +273,47 @@ "type": "string" } } - } + }, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "pull_request": { "description": "Pull request event trigger that runs the workflow when pull requests are created, updated, or closed", @@ -379,10 +419,50 @@ ] } }, - "additionalProperties": false + "additionalProperties": false, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "issues": { - "description": "Issues event trigger that runs the workflow when repository issues are created, updated, or managed", + "description": "Issues event trigger that runs when repository issues are created, updated, or managed", "type": "object", "additionalProperties": false, "properties": { @@ -577,7 +657,25 @@ "type": "string" } } - } + }, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ] }, "release": { "description": "Release event trigger", @@ -886,7 +984,47 @@ ] } }, - "additionalProperties": false + "additionalProperties": false, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "pull_request_review": { "description": "Pull request review event trigger that runs when a pull request review is submitted, edited, or dismissed", @@ -4982,6 +5120,16 @@ } }, "required": ["pull_request_review_comment"] + }, + { + "properties": { + "label": { + "not": { + "type": "null" + } + } + }, + "required": ["label"] } ] } diff --git a/pkg/parser/schema_oneof_test.go b/pkg/parser/schema_oneof_test.go new file mode 100644 index 00000000000..8fcf36c45bd --- /dev/null +++ b/pkg/parser/schema_oneof_test.go @@ -0,0 +1,332 @@ +package parser + +import ( + "strings" + "testing" +) + +// TestValidateOneOfConstraints tests the oneOf constraints added to the schema +// to prevent mutually exclusive fields from being specified together. +func TestValidateOneOfConstraints(t *testing.T) { + tests := []struct { + name string + frontmatter map[string]any + wantErr bool + errContains string + }{ + // branches vs branches-ignore in push event + { + name: "invalid: both branches and branches-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "branches": []string{"main"}, + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only branches in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "branches": []string{"main"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: only branches-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: neither branches nor branches-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "tags": []string{"v*"}, + }, + }, + }, + wantErr: false, + }, + + // paths vs paths-ignore in push event + { + name: "invalid: both paths and paths-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "paths": []string{"src/**"}, + "paths-ignore": []string{"docs/**"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only paths in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "paths": []string{"src/**"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: only paths-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "paths-ignore": []string{"docs/**"}, + }, + }, + }, + wantErr: false, + }, + + // branches vs branches-ignore in pull_request event + { + name: "invalid: both branches and branches-ignore in pull_request", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "branches": []string{"main"}, + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only branches in pull_request", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "branches": []string{"main"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: only branches-ignore in pull_request", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: false, + }, + + // paths vs paths-ignore in pull_request event + { + name: "invalid: both paths and paths-ignore in pull_request", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "paths": []string{"src/**"}, + "paths-ignore": []string{"docs/**"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only paths in pull_request", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "paths": []string{"src/**"}, + }, + }, + }, + wantErr: false, + }, + + // branches vs branches-ignore in pull_request_target event + { + name: "invalid: both branches and branches-ignore in pull_request_target", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request_target": map[string]any{ + "branches": []string{"main"}, + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only branches in pull_request_target", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request_target": map[string]any{ + "branches": []string{"main"}, + }, + }, + }, + wantErr: false, + }, + + // paths vs paths-ignore in pull_request_target event + { + name: "invalid: both paths and paths-ignore in pull_request_target", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request_target": map[string]any{ + "paths": []string{"src/**"}, + "paths-ignore": []string{"docs/**"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + + // branches vs branches-ignore in workflow_run event + { + name: "invalid: both branches and branches-ignore in workflow_run", + frontmatter: map[string]any{ + "on": map[string]any{ + "workflow_run": map[string]any{ + "workflows": []string{"CI"}, + "branches": []string{"main"}, + "branches-ignore": []string{"dev"}, + }, + }, + }, + wantErr: true, + errContains: "oneOf", + }, + { + name: "valid: only branches in workflow_run", + frontmatter: map[string]any{ + "on": map[string]any{ + "workflow_run": map[string]any{ + "workflows": []string{"CI"}, + "branches": []string{"main"}, + }, + }, + }, + wantErr: false, + }, + + // slash_command vs label events + { + name: "invalid: slash_command with label event", + frontmatter: map[string]any{ + "on": map[string]any{ + "slash_command": "mybot", + "label": map[string]any{ + "types": []string{"created"}, + }, + }, + }, + wantErr: true, + errContains: "not", + }, + { + name: "valid: slash_command without label event", + frontmatter: map[string]any{ + "on": map[string]any{ + "slash_command": "mybot", + }, + }, + wantErr: false, + }, + { + name: "valid: label event without slash_command", + frontmatter: map[string]any{ + "on": map[string]any{ + "label": map[string]any{ + "types": []string{"created"}, + }, + }, + }, + wantErr: false, + }, + + // command vs label events (deprecated command field) + { + name: "invalid: command with label event", + frontmatter: map[string]any{ + "on": map[string]any{ + "command": "mybot", + "label": map[string]any{ + "types": []string{"created"}, + }, + }, + }, + wantErr: true, + errContains: "not", + }, + + // Valid combinations of branches and paths + { + name: "valid: branches and paths in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "branches": []string{"main"}, + "paths": []string{"src/**"}, + }, + }, + }, + wantErr: false, + }, + { + name: "valid: branches-ignore and paths-ignore in push", + frontmatter: map[string]any{ + "on": map[string]any{ + "push": map[string]any{ + "branches-ignore": []string{"dev"}, + "paths-ignore": []string{"docs/**"}, + }, + }, + }, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateMainWorkflowFrontmatterWithSchema(tt.frontmatter) + + if tt.wantErr && err == nil { + t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() expected error, got nil") + return + } + + if !tt.wantErr && err != nil { + t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v", err) + return + } + + if err != nil && tt.errContains != "" { + if !strings.Contains(err.Error(), tt.errContains) { + t.Errorf("ValidateMainWorkflowFrontmatterWithSchema() error = %v, expected to contain %q", err.Error(), tt.errContains) + } + } + }) + } +} diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index f8f6e5c73ac..510479189b5 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -273,7 +273,47 @@ "type": "string" } } - } + }, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "pull_request": { "description": "Pull request event trigger that runs the workflow when pull requests are created, updated, or closed", @@ -379,10 +419,50 @@ ] } }, - "additionalProperties": false + "additionalProperties": false, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "issues": { - "description": "Issues event trigger that runs the workflow when repository issues are created, updated, or managed", + "description": "Issues event trigger that runs when repository issues are created, updated, or managed", "type": "object", "additionalProperties": false, "properties": { @@ -577,7 +657,25 @@ "type": "string" } } - } + }, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ] }, "release": { "description": "Release event trigger", @@ -886,7 +984,47 @@ ] } }, - "additionalProperties": false + "additionalProperties": false, + "oneOf": [ + { + "required": ["branches"], + "not": { "required": ["branches-ignore"] } + }, + { + "required": ["branches-ignore"], + "not": { "required": ["branches"] } + }, + { + "not": { + "anyOf": [ + { "required": ["branches"] }, + { "required": ["branches-ignore"] } + ] + } + } + ], + "allOf": [ + { + "oneOf": [ + { + "required": ["paths"], + "not": { "required": ["paths-ignore"] } + }, + { + "required": ["paths-ignore"], + "not": { "required": ["paths"] } + }, + { + "not": { + "anyOf": [ + { "required": ["paths"] }, + { "required": ["paths-ignore"] } + ] + } + } + ] + } + ] }, "pull_request_review": { "description": "Pull request review event trigger that runs when a pull request review is submitted, edited, or dismissed", @@ -4982,6 +5120,16 @@ } }, "required": ["pull_request_review_comment"] + }, + { + "properties": { + "label": { + "not": { + "type": "null" + } + } + }, + "required": ["label"] } ] } From de8b23cb179386e1e1bfb2fcb35f3b6575b2cdfb Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:37:02 -0800 Subject: [PATCH 03/10] Add debug logging to CLI utility and configuration files (#7594) --- pkg/cli/ci.go | 10 +++++++++- pkg/cli/compile_config.go | 13 +++++++++++++ pkg/cli/github.go | 7 +++++++ pkg/cli/logs_command.go | 14 ++++++++++++++ pkg/cli/logs_display.go | 8 ++++++++ 5 files changed, 51 insertions(+), 1 deletion(-) diff --git a/pkg/cli/ci.go b/pkg/cli/ci.go index 56a748ccad2..dcb940c258e 100644 --- a/pkg/cli/ci.go +++ b/pkg/cli/ci.go @@ -1,6 +1,12 @@ package cli -import "os" +import ( + "os" + + "github.com/githubnext/gh-aw/pkg/logger" +) + +var ciLog = logger.New("cli:ci") // IsRunningInCI checks if we're running in a CI environment func IsRunningInCI() bool { @@ -13,8 +19,10 @@ func IsRunningInCI() bool { for _, v := range ciVars { if os.Getenv(v) != "" { + ciLog.Printf("CI environment detected via %s", v) return true } } + ciLog.Print("No CI environment detected") return false } diff --git a/pkg/cli/compile_config.go b/pkg/cli/compile_config.go index 94e7c7de2e5..7f83d705907 100644 --- a/pkg/cli/compile_config.go +++ b/pkg/cli/compile_config.go @@ -2,8 +2,12 @@ package cli import ( "regexp" + + "github.com/githubnext/gh-aw/pkg/logger" ) +var compileConfigLog = logger.New("cli:compile_config") + // CompileConfig holds configuration options for compiling workflows type CompileConfig struct { MarkdownFiles []string // Files to compile (empty for all files) @@ -105,18 +109,25 @@ func sanitizeErrorMessage(message string) string { return message } + compileConfigLog.Printf("Sanitizing error message: length=%d", len(message)) + // Redact uppercase snake_case patterns (e.g., MY_SECRET_KEY, API_TOKEN) sanitized := secretNamePattern.ReplaceAllStringFunc(message, func(match string) string { // Don't redact common workflow keywords if commonWorkflowKeywords[match] { return match } + compileConfigLog.Printf("Redacted snake_case secret pattern: %s", match) return "[REDACTED]" }) // Redact PascalCase patterns ending with security suffixes (e.g., GitHubToken, ApiKey) sanitized = pascalCaseSecretPattern.ReplaceAllString(sanitized, "[REDACTED]") + if sanitized != message { + compileConfigLog.Print("Error message sanitization applied redactions") + } + return sanitized } @@ -129,6 +140,8 @@ func sanitizeValidationResults(results []ValidationResult) []ValidationResult { return nil } + compileConfigLog.Printf("Sanitizing validation results: workflow_count=%d", len(results)) + sanitized := make([]ValidationResult, len(results)) for i, result := range results { sanitized[i] = ValidationResult{ diff --git a/pkg/cli/github.go b/pkg/cli/github.go index 8b2bac1a194..2199d70e39d 100644 --- a/pkg/cli/github.go +++ b/pkg/cli/github.go @@ -3,8 +3,12 @@ package cli import ( "os" "strings" + + "github.com/githubnext/gh-aw/pkg/logger" ) +var githubLog = logger.New("cli:github") + // getGitHubHost returns the GitHub host URL from environment variables. // It checks GITHUB_SERVER_URL first (GitHub Actions standard), // then falls back to GH_HOST (gh CLI standard), @@ -16,6 +20,9 @@ func getGitHubHost() string { } if host == "" { host = "https://github.com" + githubLog.Print("Using default GitHub host: https://github.com") + } else { + githubLog.Printf("Resolved GitHub host: %s", host) } // Remove trailing slash for consistency diff --git a/pkg/cli/logs_command.go b/pkg/cli/logs_command.go index 8cbe706190c..50b22a7b2c0 100644 --- a/pkg/cli/logs_command.go +++ b/pkg/cli/logs_command.go @@ -16,10 +16,13 @@ import ( "github.com/githubnext/gh-aw/pkg/console" "github.com/githubnext/gh-aw/pkg/constants" + "github.com/githubnext/gh-aw/pkg/logger" "github.com/githubnext/gh-aw/pkg/workflow" "github.com/spf13/cobra" ) +var logsCommandLog = logger.New("cli:logs_command") + // NewLogsCommand creates the logs command func NewLogsCommand() *cobra.Command { logsCmd := &cobra.Command{ @@ -102,8 +105,12 @@ Examples: ` + constants.CLIExtensionPrefix + ` logs --parse --json # Generate both Markdown and JSON ` + constants.CLIExtensionPrefix + ` logs weekly-research --repo owner/repo # Download logs from specific repository`, RunE: func(cmd *cobra.Command, args []string) error { + logsCommandLog.Printf("Starting logs command: args=%d", len(args)) + var workflowName string if len(args) > 0 && args[0] != "" { + logsCommandLog.Printf("Resolving workflow name from argument: %s", args[0]) + // Convert workflow ID to GitHub Actions workflow name // First try to resolve as a workflow ID resolvedName, err := workflow.ResolveWorkflowName(args[0]) @@ -161,22 +168,27 @@ Examples: // Resolve relative dates to absolute dates for GitHub CLI now := time.Now() if startDate != "" { + logsCommandLog.Printf("Resolving start date: %s", startDate) resolvedStartDate, err := workflow.ResolveRelativeDate(startDate, now) if err != nil { return fmt.Errorf("invalid start-date format '%s': %v", startDate, err) } startDate = resolvedStartDate + logsCommandLog.Printf("Resolved start date to: %s", startDate) } if endDate != "" { + logsCommandLog.Printf("Resolving end date: %s", endDate) resolvedEndDate, err := workflow.ResolveRelativeDate(endDate, now) if err != nil { return fmt.Errorf("invalid end-date format '%s': %v", endDate, err) } endDate = resolvedEndDate + logsCommandLog.Printf("Resolved end date to: %s", endDate) } // Validate engine parameter using the engine registry if engine != "" { + logsCommandLog.Printf("Validating engine parameter: %s", engine) registry := workflow.GetGlobalEngineRegistry() if !registry.IsValidEngine(engine) { supportedEngines := registry.GetSupportedEngines() @@ -184,6 +196,8 @@ Examples: } } + logsCommandLog.Printf("Executing logs download: workflow=%s, count=%d, engine=%s", workflowName, count, engine) + return DownloadWorkflowLogs(workflowName, count, startDate, endDate, outputDir, engine, ref, beforeRunID, afterRunID, repoOverride, verbose, toolGraph, noStaged, firewallOnly, noFirewall, parse, jsonOutput, timeout, campaignOnly, summaryFile) }, } diff --git a/pkg/cli/logs_display.go b/pkg/cli/logs_display.go index b591739321c..7935e24ee9d 100644 --- a/pkg/cli/logs_display.go +++ b/pkg/cli/logs_display.go @@ -15,15 +15,21 @@ import ( "time" "github.com/githubnext/gh-aw/pkg/console" + "github.com/githubnext/gh-aw/pkg/logger" "github.com/githubnext/gh-aw/pkg/timeutil" ) +var logsDisplayLog = logger.New("cli:logs_display") + // displayLogsOverview displays a summary table of workflow runs and metrics func displayLogsOverview(processedRuns []ProcessedRun, verbose bool) { if len(processedRuns) == 0 { + logsDisplayLog.Print("No processed runs to display") return } + logsDisplayLog.Printf("Displaying logs overview: runs=%d, verbose=%v", len(processedRuns), verbose) + // Prepare table data headers := []string{"Run ID", "Workflow", "Status", "Duration", "Tokens", "Cost ($)", "Turns", "Errors", "Warnings", "Missing", "Noops", "Created", "Logs Path"} var rows [][]string @@ -176,5 +182,7 @@ func displayLogsOverview(processedRuns []ProcessedRun, verbose bool) { TotalRow: totalRow, } + logsDisplayLog.Printf("Rendering table: total_tokens=%d, total_cost=%.3f, total_duration=%s", totalTokens, totalCost, totalDuration) + fmt.Print(console.RenderTable(tableConfig)) } From 960d768441b8a993cb52802bac0aba46c786eb3c Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:39:41 -0800 Subject: [PATCH 04/10] Make actions/setup/ the source of truth for both shell and JavaScript files (#7582) --- .gitattributes | 3 +- AGENTS.md | 52 +++++++++ Makefile | 21 +++- actions/setup/js/add_copilot_reviewer.cjs | 8 +- actions/setup/js/mcp_http_transport.cjs | 5 +- .../setup/js/safe_inputs_mcp_server_http.cjs | 4 +- pkg/cli/actions_build_command.go | 95 +++++----------- specs/actions.md | 107 ++++++++++++++++-- 8 files changed, 203 insertions(+), 92 deletions(-) diff --git a/.gitattributes b/.gitattributes index 33e102d50b4..c80e1a7bf05 100644 --- a/.gitattributes +++ b/.gitattributes @@ -4,7 +4,8 @@ .github/aw/github-agentic-workflows.md linguist-generated=true merge=ours pkg/cli/workflows/*.lock.yml linguist-generated=true merge=ours pkg/workflow/js/*.js linguist-generated=true +pkg/workflow/js/*.cjs linguist-generated=true +pkg/workflow/sh/*.sh linguist-generated=true actions/*/index.js linguist-generated=true -actions/setup/js/*.cjs linguist-generated=true .github/workflows/*.campaign.g.md linguist-generated=true merge=ours diff --git a/AGENTS.md b/AGENTS.md index d4eac2f5d9a..c76d0be84e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -112,6 +112,58 @@ make build # ~1.5s ./gh-aw --help ``` +## Build System + +### Shell Script Sync + +**ALWAYS sync shell scripts before building:** + +Shell scripts in `actions/setup/sh/` are the **source of truth** and are automatically synced to `pkg/workflow/sh/` during the build process. + +```bash +make sync-shell-scripts # Copies actions/setup/sh/*.sh → pkg/workflow/sh/ +make build # Automatically runs sync-shell-scripts +``` + +**When modifying shell scripts:** +1. Edit files in `actions/setup/sh/` (source of truth) +2. Run `make build` (automatically syncs to pkg/workflow/sh/) +3. The synced files in `pkg/workflow/sh/` are embedded in the binary via `//go:embed` +4. **Never** edit files in `pkg/workflow/sh/` directly - they are generated + +**Key points:** +- `actions/setup/sh/*.sh` = Source of truth (manually edited) +- `pkg/workflow/sh/*.sh` = Generated (copied during build, marked as linguist-generated) +- The build process: `actions/setup/sh/` → `pkg/workflow/sh/` → embedded in binary + +### JavaScript File Sync + +**JavaScript files follow the SAME pattern as shell scripts:** + +JavaScript files in `actions/setup/js/` are the **source of truth** and are automatically synced to `pkg/workflow/js/` during the build process. + +```bash +make sync-js-scripts # Copies actions/setup/js/*.cjs → pkg/workflow/js/ +make build # Automatically runs sync-js-scripts +``` + +**When modifying JavaScript files:** +1. Edit files in `actions/setup/js/` (source of truth) +2. Run `make build` (automatically syncs to pkg/workflow/js/) +3. The synced files in `pkg/workflow/js/` are embedded in the binary via `//go:embed` +4. **Never** edit production files in `pkg/workflow/js/` directly - they are generated +5. Test files (*.test.cjs) remain only in `pkg/workflow/js/` and are not synced + +**Key points:** +- `actions/setup/js/*.cjs` = Source of truth (manually edited, production files only) +- `pkg/workflow/js/*.cjs` = Generated (copied during build, marked as linguist-generated) +- `pkg/workflow/js/*.test.cjs` = Test files (remain in pkg/workflow/js/, not synced) +- The build process: `actions/setup/js/` → `pkg/workflow/js/` → embedded in binary + +**Summary of patterns:** +- Shell scripts: `actions/setup/sh/` (source) → `pkg/workflow/sh/` (generated) +- JavaScript: `actions/setup/js/` (source) → `pkg/workflow/js/` (generated) + ## Development Workflow ### Build & Test Commands diff --git a/Makefile b/Makefile index fec9603d820..d16b4328ebf 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ all: build build-awmg # Build the binary, run make deps before this .PHONY: build -build: sync-templates sync-action-pins +build: sync-templates sync-action-pins sync-shell-scripts sync-js-scripts go build $(LDFLAGS) -o $(BINARY_NAME) ./cmd/gh-aw # Build the awmg (MCP gateway) binary @@ -433,6 +433,23 @@ sync-templates: @cp .github/agents/debug-agentic-workflow.agent.md pkg/cli/templates/ @echo "✓ Templates synced successfully" +# Sync shell scripts from actions/setup/sh to pkg/workflow/sh +.PHONY: sync-shell-scripts +sync-shell-scripts: + @echo "Syncing shell scripts from actions/setup/sh to pkg/workflow/sh..." + @mkdir -p pkg/workflow/sh + @cp actions/setup/sh/*.sh pkg/workflow/sh/ + @echo "✓ Shell scripts synced successfully" + +# Sync JavaScript files from actions/setup/js to pkg/workflow/js +.PHONY: sync-js-scripts +sync-js-scripts: + @echo "Syncing JavaScript files from actions/setup/js to pkg/workflow/js..." + @mkdir -p pkg/workflow/js + @cp actions/setup/js/*.cjs pkg/workflow/js/ + @cp actions/setup/js/*.json pkg/workflow/js/ 2>/dev/null || true + @echo "✓ JavaScript files synced successfully" + # Sync action pins from .github/aw to pkg/workflow/data .PHONY: sync-action-pins sync-action-pins: @@ -567,6 +584,8 @@ help: @echo " install - Install binary locally" @echo " sync-templates - Sync templates from .github to pkg/cli/templates (runs automatically during build)" @echo " sync-action-pins - Sync actions-lock.json from .github/aw to pkg/workflow/data (runs automatically during build)" + @echo " sync-shell-scripts - Sync shell scripts from actions/setup/sh to pkg/workflow/sh (runs automatically during build)" + @echo " sync-js-scripts - Sync JavaScript files from actions/setup/js to pkg/workflow/js (runs automatically during build)" @echo " update - Update GitHub Actions and workflows, sync action pins, and rebuild binary" @echo " fix - Apply automatic codemod-style fixes to workflow files (depends on build)" @echo " recompile - Recompile all workflow files (runs init, depends on build)" diff --git a/actions/setup/js/add_copilot_reviewer.cjs b/actions/setup/js/add_copilot_reviewer.cjs index 4da23077c20..51c5a43a634 100644 --- a/actions/setup/js/add_copilot_reviewer.cjs +++ b/actions/setup/js/add_copilot_reviewer.cjs @@ -17,14 +17,14 @@ const COPILOT_REVIEWER_BOT = "copilot-pull-request-reviewer[bot]"; async function main() { // Validate required environment variables - const prNumberStr = process.env.PR_NUMBER; + const prNumberStr = process.env.PR_NUMBER?.trim(); - if (!prNumberStr || prNumberStr.trim() === "") { + if (!prNumberStr) { core.setFailed("PR_NUMBER environment variable is required but not set"); return; } - const prNumber = parseInt(prNumberStr.trim(), 10); + const prNumber = parseInt(prNumberStr, 10); if (isNaN(prNumber) || prNumber <= 0) { core.setFailed(`Invalid PR_NUMBER: ${prNumberStr}. Must be a positive integer.`); return; @@ -52,7 +52,7 @@ Successfully added Copilot as a reviewer to PR #${prNumber}. ) .write(); } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); + const errorMessage = error?.message ?? String(error); core.error(`Failed to add Copilot as reviewer: ${errorMessage}`); core.setFailed(`Failed to add Copilot as reviewer to PR #${prNumber}: ${errorMessage}`); } diff --git a/actions/setup/js/mcp_http_transport.cjs b/actions/setup/js/mcp_http_transport.cjs index 25ceb145fe6..b37a581e4da 100644 --- a/actions/setup/js/mcp_http_transport.cjs +++ b/actions/setup/js/mcp_http_transport.cjs @@ -272,17 +272,14 @@ class MCPHTTPTransport { res.writeHead(200, headers); res.end(JSON.stringify(response)); } catch (error) { - // Log the full error with stack trace on the server for debugging - this.logger.debugError("Error in handleRequest: ", error); if (!res.headersSent) { res.writeHead(500, { "Content-Type": "application/json" }); - // Send a generic error message to the client to avoid exposing stack traces res.end( JSON.stringify({ jsonrpc: "2.0", error: { code: -32603, - message: "Internal server error", + message: error instanceof Error ? error.message : String(error), }, id: null, }) diff --git a/actions/setup/js/safe_inputs_mcp_server_http.cjs b/actions/setup/js/safe_inputs_mcp_server_http.cjs index 2c555d39de7..2ddffc10371 100644 --- a/actions/setup/js/safe_inputs_mcp_server_http.cjs +++ b/actions/setup/js/safe_inputs_mcp_server_http.cjs @@ -220,17 +220,15 @@ async function startHttpServer(configPath, options = {}) { // Let the transport handle the request await transport.handleRequest(req, res, body); } catch (error) { - // Log the full error with stack trace on the server for debugging logger.debugError("Error handling request: ", error); if (!res.headersSent) { res.writeHead(500, { "Content-Type": "application/json" }); - // Send a generic error message to the client to avoid exposing stack traces res.end( JSON.stringify({ jsonrpc: "2.0", error: { code: -32603, - message: "Internal server error", + message: error instanceof Error ? error.message : String(error), }, id: null, }) diff --git a/pkg/cli/actions_build_command.go b/pkg/cli/actions_build_command.go index f8f19969b15..279d5c3a8dc 100644 --- a/pkg/cli/actions_build_command.go +++ b/pkg/cli/actions_build_command.go @@ -129,26 +129,8 @@ func ActionsCleanCommand() error { } } - // Clean js/ and sh/ directories for setup action - if actionName == "setup" { - jsDir := filepath.Join(actionsDir, actionName, "js") - if _, err := os.Stat(jsDir); err == nil { - if err := os.RemoveAll(jsDir); err != nil { - return fmt.Errorf("failed to remove %s: %w", jsDir, err) - } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Removed %s/js/", actionName))) - cleanedCount++ - } - - shDir := filepath.Join(actionsDir, actionName, "sh") - if _, err := os.Stat(shDir); err == nil { - if err := os.RemoveAll(shDir); err != nil { - return fmt.Errorf("failed to remove %s: %w", shDir, err) - } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Removed %s/sh/", actionName))) - cleanedCount++ - } - } + // For setup action, both js/ and sh/ directories are source of truth (NOT generated) + // Do not clean them } fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("✨ Cleanup complete (%d files removed)", cleanedCount))) @@ -342,65 +324,44 @@ func buildSetupSafeOutputsAction(actionsDir, actionName string) error { return nil } -// buildSetupAction builds the setup action by copying JavaScript files to js/ directory -// and shell scripts to sh/ directory +// buildSetupAction builds the setup action by checking that source files exist. +// Note: Both JavaScript and shell scripts are source of truth in actions/setup/js/ and actions/setup/sh/ +// They get synced to pkg/workflow/js/ and pkg/workflow/sh/ during the build process via Makefile targets. func buildSetupAction(actionsDir, actionName string) error { actionPath := filepath.Join(actionsDir, actionName) jsDir := filepath.Join(actionPath, "js") shDir := filepath.Join(actionPath, "sh") - // Get dependencies for this action - dependencies := getActionDependencies(actionName) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Found %d JavaScript dependencies", len(dependencies)))) - - // Get all JavaScript sources - sources := workflow.GetJavaScriptSources() - - // Create js directory if it doesn't exist - if err := os.MkdirAll(jsDir, 0755); err != nil { - return fmt.Errorf("failed to create js directory: %w", err) - } - - // Copy each dependency file to the js directory - copiedCount := 0 - for _, dep := range dependencies { - if content, ok := sources[dep]; ok { - destPath := filepath.Join(jsDir, dep) - if err := os.WriteFile(destPath, []byte(content), 0644); err != nil { - return fmt.Errorf("failed to write %s: %w", dep, err) + // JavaScript files in actions/setup/js/ are the source of truth + if _, err := os.Stat(jsDir); err == nil { + // Count JavaScript files + entries, err := os.ReadDir(jsDir) + if err == nil { + jsCount := 0 + for _, entry := range entries { + if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".cjs") { + jsCount++ + } } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" - %s", dep))) - copiedCount++ - } else { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf(" ⚠ Warning: Could not find %s", dep))) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ JavaScript files in js/ (source of truth): %d", jsCount))) } } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Copied %d files to js/", copiedCount))) - - // Get bundled shell scripts - shellScripts := workflow.GetBundledShellScripts() - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Found %d shell scripts", len(shellScripts)))) - - // Create sh directory if it doesn't exist - if err := os.MkdirAll(shDir, 0755); err != nil { - return fmt.Errorf("failed to create sh directory: %w", err) - } - - // Copy each shell script to the sh directory - shCopiedCount := 0 - for filename, content := range shellScripts { - destPath := filepath.Join(shDir, filename) - // Shell scripts should be executable (0755) - if err := os.WriteFile(destPath, []byte(content), 0755); err != nil { - return fmt.Errorf("failed to write %s: %w", filename, err) + // Shell scripts in actions/setup/sh/ are the source of truth + if _, err := os.Stat(shDir); err == nil { + // Count shell scripts + entries, err := os.ReadDir(shDir) + if err == nil { + shCount := 0 + for _, entry := range entries { + if !entry.IsDir() && strings.HasSuffix(entry.Name(), ".sh") { + shCount++ + } + } + fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Shell scripts in sh/ (source of truth): %d", shCount))) } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" - %s", filename))) - shCopiedCount++ } - fmt.Fprintln(os.Stderr, console.FormatInfoMessage(fmt.Sprintf(" ✓ Copied %d shell scripts to sh/", shCopiedCount))) - return nil } diff --git a/specs/actions.md b/specs/actions.md index edf9b19f76b..4a67b54e2be 100644 --- a/specs/actions.md +++ b/specs/actions.md @@ -141,6 +141,14 @@ Create a custom actions system that: gh-aw/ ├── actions/ # Custom GitHub Actions │ ├── README.md # Actions documentation +│ ├── setup/ # Setup action with shell scripts +│ │ ├── action.yml # Action metadata +│ │ ├── setup.sh # Main setup script +│ │ ├── js/ # JavaScript files (generated) +│ │ │ └── *.cjs # Copied from pkg/workflow/js/ +│ │ ├── sh/ # Shell scripts (SOURCE OF TRUTH) +│ │ │ └── *.sh # Manually edited shell scripts +│ │ └── README.md # Action-specific docs │ ├── setup-safe-inputs/ # Safe inputs MCP server setup │ │ ├── action.yml # Action metadata │ │ ├── index.js # Bundled output (committed) @@ -158,17 +166,36 @@ gh-aw/ │ │ └── actions_build_command.go # Build system implementation │ └── workflow/ │ ├── js.go # JavaScript sources map -│ └── js/ # Embedded JavaScript files -│ ├── *.cjs # CommonJS modules -│ └── *.json # JSON configuration files +│ ├── sh.go # Shell script sources map +│ ├── js/ # Embedded JavaScript files +│ │ ├── *.cjs # CommonJS modules +│ │ └── *.json # JSON configuration files +│ └── sh/ # Embedded shell scripts (GENERATED) +│ └── *.sh # Copied from actions/setup/sh/ ├── cmd/gh-aw/ │ └── main.go # CLI entry point with commands -├── Makefile # Build targets -├── .gitattributes # Mark generated files +├── Makefile # Build targets (includes sync-shell-scripts) +├── .gitattributes # Mark generated files (pkg/workflow/sh/*.sh) └── .github/workflows/ └── ci.yml # CI pipeline with actions-build job ``` +**Shell Script Sync Flow:** +1. **Source of Truth**: `actions/setup/sh/*.sh` (manually edited) +2. **Sync Step**: `make sync-shell-scripts` copies to `pkg/workflow/sh/` +3. **Build Step**: `make build` embeds `pkg/workflow/sh/*.sh` via `//go:embed` +4. **Actions Build**: `make actions-build` does NOT copy shell scripts (they already exist in actions/setup/sh/) + +**JavaScript File Sync Flow:** +1. **Source of Truth**: `actions/setup/js/*.cjs` (manually edited, production files only) +2. **Sync Step**: `make sync-js-scripts` copies to `pkg/workflow/js/` +3. **Build Step**: `make build` embeds `pkg/workflow/js/*.cjs` via `//go:embed` +4. **Test Files**: `pkg/workflow/js/*.test.cjs` remain only in pkg/workflow/js/ (not synced) + +Both follow the same pattern now: +- Shell: `actions/setup/sh/` (source) → `pkg/workflow/sh/` (generated) +- JavaScript: `actions/setup/js/` (source) → `pkg/workflow/js/` (generated) + ### Action Structure Each action follows this template: @@ -225,16 +252,50 @@ for (const [filename, content] of Object.entries(FILES)) { The build system is implemented entirely in Go and follows these steps: -1. **Discovery**: Scans `actions/` directory for action subdirectories -2. **Validation**: Validates each `action.yml` file structure -3. **Dependency Resolution**: Maps action name to required JavaScript files (manual mapping in Go) -4. **File Reading**: Retrieves file contents from `workflow.GetJavaScriptSources()` -5. **Bundling**: Creates JSON object with all dependencies -6. **Code Generation**: Replaces `FILES` placeholder in source with bundled content -7. **Output**: Writes bundled `index.js` to action directory +1. **Shell Script Sync** (NEW): Copies shell scripts from `actions/setup/sh/` to `pkg/workflow/sh/` +2. **Discovery**: Scans `actions/` directory for action subdirectories +3. **Validation**: Validates each `action.yml` file structure +4. **Dependency Resolution**: Maps action name to required JavaScript files (manual mapping in Go) +5. **File Reading**: Retrieves file contents from `workflow.GetJavaScriptSources()` +6. **Bundling**: Creates JSON object with all dependencies +7. **Code Generation**: Replaces `FILES` placeholder in source with bundled content +8. **Output**: Writes bundled `index.js` to action directory **Key Point**: The build system is pure Go code in `pkg/cli/actions_build_command.go`. There are no JavaScript build scripts - everything uses the workflow compiler's existing infrastructure. +### Shell Script Sync Process + +**Important**: Shell scripts follow a different pattern than JavaScript files: + +**Shell Scripts** (source in actions/setup/sh/): +``` +actions/setup/sh/*.sh → pkg/workflow/sh/*.sh → Embedded in binary + (SOURCE OF TRUTH) (GENERATED) (go:embed) +``` + +**JavaScript Files** (source in actions/setup/js/): +``` +actions/setup/js/*.cjs → pkg/workflow/js/*.cjs → Embedded in binary + (SOURCE OF TRUTH) (GENERATED) (go:embed) +``` + +Note: Test files (`*.test.cjs`) remain only in `pkg/workflow/js/` and are not synced from actions/setup/js/. + +The sync happens via Makefile targets, which are automatically run as part of `make build`: + +```bash +make sync-shell-scripts # Explicit shell sync +make sync-js-scripts # Explicit JavaScript sync +make build # Includes both syncs +``` + +**Why this pattern?** +- Both shell scripts and JavaScript are part of the setup action itself and live in `actions/setup/` +- They need to be embedded in the Go binary for compilation +- Syncing before build ensures the embedded files match the action's source files +- This creates a single source of truth in the actions directory +- Test files remain in pkg/workflow/js/ for development + ### Build Commands Use Makefile targets for building actions: @@ -533,6 +594,21 @@ The CI runs when: #### Common Tasks +**Modify a shell script**: +1. Edit the file in `actions/setup/sh/` (source of truth) +2. Run `make build` (syncs to pkg/workflow/sh/ and rebuilds binary) +3. Run `make actions-build` (builds actions including setup) +4. Test in a workflow to verify behavior +5. Commit both `actions/setup/sh/*.sh` and generated `pkg/workflow/sh/*.sh` + +**Add a new shell script**: +1. Create the file in `actions/setup/sh/` +2. Add `//go:embed` directive in `pkg/workflow/sh.go` +3. Add to `GetBundledShellScripts()` function in `sh.go` +4. Run `make build` to sync and embed +5. Update setup action's `setup.sh` to use the new script +6. Run `make actions-build` to build the setup action + **Add a new action**: 1. Create directory structure in `actions/` 2. Write `action.yml`, `src/index.js`, `README.md` @@ -557,6 +633,13 @@ The CI runs when: - `pkg/cli/actions_build_command.go` - **Pure Go build system** (no JavaScript) - `internal/tools/actions-build/main.go` - Internal CLI tool dispatcher (development only) - `pkg/workflow/js.go` - JavaScript source map and embedded files +- `pkg/workflow/sh.go` - Shell script source map and embedded files +- `actions/setup/sh/` - **Source of truth for shell scripts** (manually edited) +- `pkg/workflow/sh/` - Generated shell scripts (synced from actions/setup/sh/) +- `Makefile` - Primary interface for building actions (`make actions-build`, `make sync-shell-scripts`) +- `.github/workflows/ci.yml` - CI validation +- `actions/README.md` - Actions documentation +- `.gitattributes` - Marks generated files (pkg/workflow/sh/*.sh, actions/setup/js/*.cjs) - `Makefile` - Primary interface for building actions (`make actions-build`) - `.github/workflows/ci.yml` - CI validation - `actions/README.md` - Actions documentation From 5c6cfece827dfd3acc43e5a377dc38fb2ad39cdb Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:48:04 -0800 Subject: [PATCH 05/10] Consolidate MCPServerConfig into single definition in parser package (#7590) --- pkg/awmg/gateway.go | 20 ++++--------- pkg/awmg/gateway_inspect_integration_test.go | 4 ++- pkg/awmg/gateway_integration_test.go | 4 ++- pkg/awmg/gateway_rewrite_test.go | 6 ++-- pkg/awmg/gateway_streamable_http_test.go | 6 ++-- pkg/awmg/gateway_test.go | 30 +++++++++++--------- 6 files changed, 36 insertions(+), 34 deletions(-) diff --git a/pkg/awmg/gateway.go b/pkg/awmg/gateway.go index 40fcdd1246f..4dd6e536b9e 100644 --- a/pkg/awmg/gateway.go +++ b/pkg/awmg/gateway.go @@ -14,6 +14,7 @@ import ( "github.com/githubnext/gh-aw/pkg/console" "github.com/githubnext/gh-aw/pkg/logger" + "github.com/githubnext/gh-aw/pkg/parser" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/cobra" ) @@ -35,17 +36,8 @@ func GetVersion() string { // MCPGatewayConfig represents the configuration for the MCP gateway. type MCPGatewayConfig struct { - MCPServers map[string]MCPServerConfig `json:"mcpServers"` - Gateway GatewaySettings `json:"gateway,omitempty"` -} - -// MCPServerConfig represents configuration for a single MCP server. -type MCPServerConfig struct { - Command string `json:"command,omitempty"` - Args []string `json:"args,omitempty"` - Env map[string]string `json:"env,omitempty"` - URL string `json:"url,omitempty"` - Container string `json:"container,omitempty"` + MCPServers map[string]parser.MCPServerConfig `json:"mcpServers"` + Gateway GatewaySettings `json:"gateway,omitempty"` } // GatewaySettings represents gateway-specific settings. @@ -312,7 +304,7 @@ func parseGatewayConfig(data []byte) (*MCPGatewayConfig, error) { // Filter out internal workflow MCP servers (safeinputs and safeoutputs) // These are used internally by the workflow and should not be proxied by the gateway - filteredServers := make(map[string]MCPServerConfig) + filteredServers := make(map[string]parser.MCPServerConfig) for name, serverConfig := range config.MCPServers { if name == "safeinputs" || name == "safeoutputs" { gatewayLog.Printf("Filtering out internal workflow server: %s", name) @@ -329,7 +321,7 @@ func parseGatewayConfig(data []byte) (*MCPGatewayConfig, error) { // mergeConfigs merges two gateway configurations, with the second overriding the first func mergeConfigs(base, override *MCPGatewayConfig) *MCPGatewayConfig { result := &MCPGatewayConfig{ - MCPServers: make(map[string]MCPServerConfig), + MCPServers: make(map[string]parser.MCPServerConfig), Gateway: base.Gateway, } @@ -508,7 +500,7 @@ func (g *MCPGatewayServer) initializeSessions() error { } // createMCPSession creates an MCP session for a single server configuration -func (g *MCPGatewayServer) createMCPSession(serverName string, config MCPServerConfig) (*mcp.ClientSession, error) { +func (g *MCPGatewayServer) createMCPSession(serverName string, config parser.MCPServerConfig) (*mcp.ClientSession, error) { // Create log file for this server (flat directory structure) logFile := filepath.Join(g.logDir, fmt.Sprintf("%s.log", serverName)) gatewayLog.Printf("Creating log file for %s: %s", serverName, logFile) diff --git a/pkg/awmg/gateway_inspect_integration_test.go b/pkg/awmg/gateway_inspect_integration_test.go index 6e8bc1914bd..90968f01491 100644 --- a/pkg/awmg/gateway_inspect_integration_test.go +++ b/pkg/awmg/gateway_inspect_integration_test.go @@ -13,6 +13,8 @@ import ( "strings" "testing" "time" + + "github.com/githubnext/gh-aw/pkg/parser" ) // TestMCPGateway_InspectWithPlaywright tests the MCP gateway by: @@ -66,7 +68,7 @@ This workflow tests the MCP gateway configuration and tool list. // Create MCP gateway configuration with gh-aw MCP server configFile := filepath.Join(tmpDir, "gateway-config.json") config := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "gh-aw": { Command: binaryPath, Args: []string{"mcp-server"}, diff --git a/pkg/awmg/gateway_integration_test.go b/pkg/awmg/gateway_integration_test.go index e47b0282fd6..180621928ac 100644 --- a/pkg/awmg/gateway_integration_test.go +++ b/pkg/awmg/gateway_integration_test.go @@ -10,6 +10,8 @@ import ( "path/filepath" "testing" "time" + + "github.com/githubnext/gh-aw/pkg/parser" ) func TestMCPGateway_BasicStartup(t *testing.T) { @@ -24,7 +26,7 @@ func TestMCPGateway_BasicStartup(t *testing.T) { configFile := filepath.Join(tmpDir, "gateway-config.json") config := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "gh-aw": { Command: binaryPath, Args: []string{"mcp-server"}, diff --git a/pkg/awmg/gateway_rewrite_test.go b/pkg/awmg/gateway_rewrite_test.go index d402f05b8ee..ff71424ae17 100644 --- a/pkg/awmg/gateway_rewrite_test.go +++ b/pkg/awmg/gateway_rewrite_test.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/githubnext/gh-aw/pkg/parser" ) // TestRewriteMCPConfigForGateway_PreservesNonProxiedServers tests that @@ -42,7 +44,7 @@ func TestRewriteMCPConfigForGateway_PreservesNonProxiedServers(t *testing.T) { // Gateway config only includes external server (github), not internal servers gatewayConfig := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "github": { Command: "docker", Args: []string{"run", "-i", "--rm", "ghcr.io/github-mcp-server"}, @@ -184,7 +186,7 @@ func TestRewriteMCPConfigForGateway_NoGatewaySection(t *testing.T) { } gatewayConfig := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "github": { Command: "gh", Args: []string{"aw", "mcp-server"}, diff --git a/pkg/awmg/gateway_streamable_http_test.go b/pkg/awmg/gateway_streamable_http_test.go index 8e55f135735..3e13f184309 100644 --- a/pkg/awmg/gateway_streamable_http_test.go +++ b/pkg/awmg/gateway_streamable_http_test.go @@ -15,6 +15,8 @@ import ( "testing" "time" + "github.com/githubnext/gh-aw/pkg/parser" + "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -38,7 +40,7 @@ func TestStreamableHTTPTransport_GatewayConnection(t *testing.T) { // Create gateway config with the gh-aw MCP server config := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "gh-aw": { Command: binaryPath, Args: []string{"mcp-server"}, @@ -336,7 +338,7 @@ func TestStreamableHTTPTransport_URLConfigured(t *testing.T) { } // Create a session with URL configuration - serverConfig := MCPServerConfig{ + serverConfig := parser.MCPServerConfig{ URL: mockServer.URL, } diff --git a/pkg/awmg/gateway_test.go b/pkg/awmg/gateway_test.go index 8bad37e79e3..699e7fa91bf 100644 --- a/pkg/awmg/gateway_test.go +++ b/pkg/awmg/gateway_test.go @@ -5,6 +5,8 @@ import ( "os" "path/filepath" "testing" + + "github.com/githubnext/gh-aw/pkg/parser" ) func TestReadGatewayConfig_FromFile(t *testing.T) { @@ -13,7 +15,7 @@ func TestReadGatewayConfig_FromFile(t *testing.T) { configFile := filepath.Join(tmpDir, "gateway-config.json") config := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "test-server": { Command: "test-command", Args: []string{"arg1", "arg2"}, @@ -83,7 +85,7 @@ func TestReadGatewayConfig_InvalidJSON(t *testing.T) { func TestMCPGatewayConfig_EmptyServers(t *testing.T) { config := &MCPGatewayConfig{ - MCPServers: make(map[string]MCPServerConfig), + MCPServers: make(map[string]parser.MCPServerConfig), Gateway: GatewaySettings{ Port: 8080, }, @@ -95,7 +97,7 @@ func TestMCPGatewayConfig_EmptyServers(t *testing.T) { } func TestMCPServerConfig_CommandType(t *testing.T) { - config := MCPServerConfig{ + config := parser.MCPServerConfig{ Command: "gh", Args: []string{"aw", "mcp-server"}, Env: map[string]string{ @@ -117,7 +119,7 @@ func TestMCPServerConfig_CommandType(t *testing.T) { } func TestMCPServerConfig_URLType(t *testing.T) { - config := MCPServerConfig{ + config := parser.MCPServerConfig{ URL: "http://localhost:3000", } @@ -131,7 +133,7 @@ func TestMCPServerConfig_URLType(t *testing.T) { } func TestMCPServerConfig_ContainerType(t *testing.T) { - config := MCPServerConfig{ + config := parser.MCPServerConfig{ Container: "mcp-server:latest", Args: []string{"--verbose"}, Env: map[string]string{ @@ -188,7 +190,7 @@ func TestReadGatewayConfig_EmptyServers(t *testing.T) { configFile := filepath.Join(tmpDir, "empty-servers.json") config := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{}, + MCPServers: map[string]parser.MCPServerConfig{}, Gateway: GatewaySettings{ Port: 8080, }, @@ -237,7 +239,7 @@ func TestReadGatewayConfig_MultipleFiles(t *testing.T) { tmpDir := t.TempDir() baseConfig := filepath.Join(tmpDir, "base-config.json") baseConfigData := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "server1": { Command: "command1", Args: []string{"arg1"}, @@ -263,7 +265,7 @@ func TestReadGatewayConfig_MultipleFiles(t *testing.T) { // Create override config file overrideConfig := filepath.Join(tmpDir, "override-config.json") overrideConfigData := MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "server2": { Command: "override-command2", Args: []string{"override-arg2"}, @@ -336,7 +338,7 @@ func TestReadGatewayConfig_MultipleFiles(t *testing.T) { func TestMergeConfigs(t *testing.T) { base := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "server1": { Command: "cmd1", }, @@ -351,7 +353,7 @@ func TestMergeConfigs(t *testing.T) { } override := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "server2": { Command: "override-cmd2", }, @@ -396,7 +398,7 @@ func TestMergeConfigs(t *testing.T) { func TestMergeConfigs_EmptyOverride(t *testing.T) { base := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "server1": { Command: "cmd1", }, @@ -407,7 +409,7 @@ func TestMergeConfigs_EmptyOverride(t *testing.T) { } override := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{}, + MCPServers: map[string]parser.MCPServerConfig{}, Gateway: GatewaySettings{}, } @@ -533,7 +535,7 @@ func TestRewriteMCPConfigForGateway(t *testing.T) { // Create a gateway config (after filtering) gatewayConfig := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "github": { Command: "gh", Args: []string{"aw", "mcp-server"}, @@ -634,7 +636,7 @@ func TestRewriteMCPConfigForGateway_WithAPIKey(t *testing.T) { // Create a gateway config with API key gatewayConfig := &MCPGatewayConfig{ - MCPServers: map[string]MCPServerConfig{ + MCPServers: map[string]parser.MCPServerConfig{ "github": { Command: "gh", Args: []string{"aw", "mcp-server"}, From 3642f045667cb9fbb50128013a494484d810f635 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:48:20 -0800 Subject: [PATCH 06/10] Add minItems constraints to array fields requiring values (#7597) --- pkg/parser/schema_test.go | 39 ++++++++++++++++ pkg/parser/schemas/main_workflow_schema.json | 48 +++++++------------- 2 files changed, 55 insertions(+), 32 deletions(-) diff --git a/pkg/parser/schema_test.go b/pkg/parser/schema_test.go index c2257ae482f..0ce13d1ebf8 100644 --- a/pkg/parser/schema_test.go +++ b/pkg/parser/schema_test.go @@ -621,6 +621,45 @@ func TestValidateMainWorkflowFrontmatterWithSchema(t *testing.T) { wantErr: true, errContains: "minItems: got 0, want 1", }, + { + name: "invalid empty toolsets array", + frontmatter: map[string]any{ + "on": "push", + "tools": map[string]any{ + "github": map[string]any{ + "toolsets": []string{}, + }, + }, + }, + wantErr: true, + errContains: "minItems", + }, + { + name: "invalid empty issue names array", + frontmatter: map[string]any{ + "on": map[string]any{ + "issues": map[string]any{ + "types": []string{"labeled"}, + "names": []string{}, + }, + }, + }, + wantErr: true, + errContains: "minItems", + }, + { + name: "invalid empty pull_request names array", + frontmatter: map[string]any{ + "on": map[string]any{ + "pull_request": map[string]any{ + "types": []string{"labeled"}, + "names": []string{}, + }, + }, + }, + wantErr: true, + errContains: "minItems", + }, { name: "valid schedule with multiple cron entries", frontmatter: map[string]any{ diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 510479189b5..fd9e83996e0 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -285,10 +285,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -305,10 +302,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -414,7 +408,8 @@ "items": { "type": "string", "description": "Label name" - } + }, + "minItems": 1 } ] } @@ -431,10 +426,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -451,10 +443,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -486,7 +475,8 @@ "items": { "type": "string", "description": "Label name" - } + }, + "minItems": 1 } ] }, @@ -669,10 +659,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ] @@ -996,10 +983,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -1016,10 +1000,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -1899,7 +1880,8 @@ "items": { "type": "string", "description": "Domain name or ecosystem identifier (supports wildcards like '*.example.com' and ecosystem names like 'python', 'node')" - } + }, + "$comment": "Empty array is valid and means deny all network access. Omit the field entirely or use network: defaults to use default network permissions." }, "firewall": { "description": "AWF (Agent Workflow Firewall) configuration for network egress control. Only supported for Copilot engine.", @@ -2406,7 +2388,9 @@ "stargazers", "users" ] - } + }, + "minItems": 1, + "$comment": "At least one toolset is required when toolsets array is specified. Use null or omit the field to use all toolsets." } }, "additionalProperties": false, From 5a344f78c14dd834674ba88a2f2aaf4279d1799b Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:48:57 -0800 Subject: [PATCH 07/10] Migrate MCP relationship prose to JSON Schema constraints with $comment documentation (#7598) --- pkg/parser/schemas/main_workflow_schema.json | 48 ++++++++++++++++++-- pkg/workflow/sandbox_test.go | 1 + 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index fd9e83996e0..ecc885b6a4a 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -1847,6 +1847,7 @@ } }, "network": { + "$comment": "Strict mode requirements: When strict=true, the 'network' field must be present (not null/undefined) and cannot contain wildcard '*' in allowed domains. This is validated in Go code (pkg/workflow/strict_mode_validation.go) via validateStrictNetwork().", "description": "Network access control for AI engines using ecosystem identifiers and domain allowlists. Controls web fetch and search capabilities.", "examples": [ "defaults", @@ -2106,12 +2107,14 @@ "properties": { "command": { "type": "string", - "description": "Custom command to execute the MCP gateway (mutually exclusive with 'container')" + "$comment": "Mutually exclusive with 'container' - only one execution mode can be specified.", + "description": "Custom command to execute the MCP gateway" }, "container": { "type": "string", "pattern": "^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*$", - "description": "Container image for the MCP gateway executable (mutually exclusive with 'command')" + "$comment": "Mutually exclusive with 'command' - only one execution mode can be specified.", + "description": "Container image for the MCP gateway executable" }, "version": { "type": ["string", "number"], @@ -2130,7 +2133,8 @@ "items": { "type": "string" }, - "description": "Arguments to add after the container image (container entrypoint arguments, only valid with 'container')" + "$comment": "Requires 'container' to be specified - entrypoint arguments only apply to container execution.", + "description": "Arguments to add after the container image (container entrypoint arguments)" }, "env": { "type": "object", @@ -2154,7 +2158,35 @@ "description": "API key for authenticating with the MCP gateway (supports ${{ secrets.* }} syntax)" } }, - "additionalProperties": false + "additionalProperties": false, + "anyOf": [ + { + "required": ["command"] + }, + { + "required": ["container"] + } + ], + "not": { + "allOf": [ + { + "required": ["command"] + }, + { + "required": ["container"] + } + ] + }, + "allOf": [ + { + "if": { + "required": ["entrypointArgs"] + }, + "then": { + "required": ["container"] + } + } + ] } }, "additionalProperties": false @@ -3139,6 +3171,7 @@ }, "safe-outputs": { "type": "object", + "$comment": "Strict mode dependency: When strict=true AND permissions contains write values (contents:write, issues:write, or pull-requests:write), safe-outputs must be configured. This relationship is validated in Go code (pkg/workflow/strict_mode_validation.go) via validateStrictPermissions() because it requires complex logic to check if ANY permission property equals 'write', which cannot be expressed concisely in JSON Schema.", "description": "Safe output processing configuration that automatically creates GitHub issues, comments, and pull requests from AI workflow output without requiring write permissions in the main job", "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, close-discussion, close-issue, close-pull-request, create-agent-task, create-code-scanning-alert, create-discussion, create-issue, create-pull-request, create-pull-request-review-comment, hide-comment, link-sub-issue, missing-tool, noop, push-to-pull-request-branch, threat-detection, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-asset. See documentation for complete details.", "properties": { @@ -4845,6 +4878,7 @@ "strict": { "type": "boolean", "default": true, + "$comment": "Strict mode enforces several security constraints that are validated in Go code (pkg/workflow/strict_mode_validation.go) rather than JSON Schema: (1) Write Permissions + Safe Outputs: When strict=true AND permissions contains write values (contents:write, issues:write, pull-requests:write), safe-outputs must be configured. This relationship is too complex for JSON Schema as it requires checking if ANY permission property has a 'write' value. (2) Network Requirements: When strict=true, the 'network' field must be present and cannot contain wildcard '*'. (3) MCP Container Network: Custom MCP servers with containers require explicit network configuration. (4) Action Pinning: Actions must be pinned to commit SHAs. These are enforced during compilation via validateStrictMode().", "description": "Enable strict mode validation for enhanced security and compliance. Strict mode enforces: (1) Write Permissions - refuses contents:write, issues:write, pull-requests:write; requires safe-outputs instead, (2) Network Configuration - requires explicit network configuration with no wildcard '*' in allowed domains, (3) Action Pinning - enforces actions pinned to commit SHAs instead of tags/branches, (4) MCP Network - requires network configuration for custom MCP servers with containers, (5) Deprecated Fields - refuses deprecated frontmatter fields. Can be enabled per-workflow via 'strict: true' in frontmatter, or disabled via 'strict: false'. CLI flag takes precedence over frontmatter (gh aw compile --strict enforces strict mode). Defaults to true. See: https://githubnext.github.io/gh-aw/reference/frontmatter/#strict-mode-strict", "examples": [true, false] }, @@ -5292,12 +5326,14 @@ "command": { "type": "string", "minLength": 1, + "$comment": "Mutually exclusive with 'container' - only one execution mode can be specified. Validated by 'not.allOf' constraint below.", "description": "Command for stdio MCP connections" }, "container": { "type": "string", "pattern": "^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*$", - "description": "Container image for stdio MCP connections (alternative to command)" + "$comment": "Mutually exclusive with 'command' - only one execution mode can be specified. Validated by 'not.allOf' constraint below.", + "description": "Container image for stdio MCP connections" }, "version": { "type": ["string", "number"], @@ -5330,6 +5366,7 @@ }, "network": { "type": "object", + "$comment": "Requires 'container' to be specified - network configuration only applies to container-based MCP servers. Validated by 'if/then' constraint in 'allOf' below.", "properties": { "allowed": { "type": "array", @@ -5362,6 +5399,7 @@ } }, "additionalProperties": false, + "$comment": "Validation constraints: (1) Mutual exclusion: 'command' and 'container' cannot both be specified. (2) Requirement: Either 'command' or 'container' must be provided (via 'anyOf'). (3) Dependency: 'network' requires 'container' (validated in 'allOf'). (4) Type constraint: When 'type' is 'stdio' or 'local', either 'command' or 'container' is required.", "anyOf": [ { "required": ["type"] diff --git a/pkg/workflow/sandbox_test.go b/pkg/workflow/sandbox_test.go index 9ae41678bd0..79bdcd4f8ee 100644 --- a/pkg/workflow/sandbox_test.go +++ b/pkg/workflow/sandbox_test.go @@ -458,6 +458,7 @@ engine: copilot sandbox: agent: awf mcp: + container: "ghcr.io/githubnext/mcp-gateway" port: 9090 api-key: "${{ secrets.MCP_API_KEY }}" features: From 9e549015fa7f0cf917c496802c1d07750b10b535 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 02:53:00 -0800 Subject: [PATCH 08/10] Optimize CI integration test matrix by splitting bottleneck groups (#7603) --- .github/workflows/ci.yml | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a0dd1710a2..db37467fea1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -99,10 +99,19 @@ jobs: - name: "CLI Security Tools" # Group security tool compilation tests packages: "./pkg/cli" pattern: "TestCompileWithZizmor|TestCompileWithPoutine|TestCompileWithPoutineAndZizmor" + - name: "CLI Add & List Commands" + packages: "./pkg/cli" + pattern: "^TestAdd|^TestList" + - name: "CLI Update Command" + packages: "./pkg/cli" + pattern: "^TestUpdate" + - name: "CLI Audit & Inspect" + packages: "./pkg/cli" + pattern: "^TestAudit|^TestInspect" - name: "CLI Completion & Other" # Remaining catch-all (reduced from original) packages: "./pkg/cli" pattern: "" # Catch-all for tests not matched by other CLI patterns - skip_pattern: "^TestCompile[^W]|TestPoutine|TestMCPInspectPlaywright|TestMCPGateway|TestMCPAdd|TestMCPInspectGitHub|TestMCPServer|TestMCPConfig|TestLogs|TestFirewall|TestNoStopTime|TestLocalWorkflow|TestProgressFlagSignature|TestConnectHTTPMCPServer|TestCompileWorkflows_EmptyMarkdown|TestCompileWithZizmor|TestCompileWithPoutine|TestCompileWithPoutineAndZizmor" + skip_pattern: "^TestCompile[^W]|TestPoutine|TestMCPInspectPlaywright|TestMCPGateway|TestMCPAdd|TestMCPInspectGitHub|TestMCPServer|TestMCPConfig|TestLogs|TestFirewall|TestNoStopTime|TestLocalWorkflow|TestProgressFlagSignature|TestConnectHTTPMCPServer|TestCompileWorkflows_EmptyMarkdown|TestCompileWithZizmor|TestCompileWithPoutine|TestCompileWithPoutineAndZizmor|^TestAdd|^TestList|^TestUpdate|^TestAudit|^TestInspect" - name: "Workflow Compiler" packages: "./pkg/workflow" pattern: "TestCompile|TestWorkflow|TestGenerate|TestParse" @@ -121,9 +130,15 @@ jobs: - name: "Workflow Rendering & Bundling" packages: "./pkg/workflow" pattern: "Render|Bundle|Script|WritePromptText" - - name: "Workflow Cache & Actions" + - name: "Workflow Cache" + packages: "./pkg/workflow" + pattern: "^TestCache|TestCacheDependencies|TestCacheKey|TestValidateCache" + - name: "Workflow Actions Pin Validation" packages: "./pkg/workflow" - pattern: "Cache|Action|Container" + pattern: "^TestActionPinSHAsMatchVersionTags" + - name: "Workflow Actions & Containers" + packages: "./pkg/workflow" + pattern: "^TestAction[^P]|Container" - name: "Workflow Dependabot & Security" packages: "./pkg/workflow" pattern: "Dependabot|Security|PII" @@ -150,10 +165,16 @@ jobs: - name: "Workflow Misc Part 1" # Split large catch-all into two balanced groups packages: "./pkg/workflow" pattern: "TestAgent|TestCopilot|TestCustom|TestEngine|TestModel|TestNetwork|TestOpenAI|TestProvider" + - name: "Workflow String & Sanitization" + packages: "./pkg/workflow" + pattern: "String|Sanitize|Normalize|Trim|Clean|Format" + - name: "Workflow Runtime & Setup" + packages: "./pkg/workflow" + pattern: "Runtime|Setup|Install|Download|Version|Binary" - name: "Workflow Misc Part 2" # Remaining workflow tests packages: "./pkg/workflow" pattern: "" - skip_pattern: "TestCompile|TestWorkflow|TestGenerate|TestParse|TestMCP|TestTool|TestSkill|TestPlaywright|TestFirewall|TestValidat|TestLock|TestError|TestWarning|SafeOutputs|CreatePullRequest|OutputLabel|HasSafeOutputs|GitHub|Git|PushToPullRequest|BuildFromAllowed|Render|Bundle|Script|WritePromptText|Cache|Action|Container|Dependabot|Security|PII|TestPermissions|TestPackageExtractor|TestCollectPackagesFromWorkflow|TestExpression|TestValidateExpressionSafety|TestCheckNetworkSupport|TestValidateStrictMCPNetwork|TestJobManager|TestWorkflowStep|TestScriptRegistry|TestAgent|TestCopilot|TestCustom|TestEngine|TestModel|TestNetwork|TestOpenAI|TestProvider" + skip_pattern: "TestCompile|TestWorkflow|TestGenerate|TestParse|TestMCP|TestTool|TestSkill|TestPlaywright|TestFirewall|TestValidat|TestLock|TestError|TestWarning|SafeOutputs|CreatePullRequest|OutputLabel|HasSafeOutputs|GitHub|Git|PushToPullRequest|BuildFromAllowed|Render|Bundle|Script|WritePromptText|^TestCache|TestCacheDependencies|TestCacheKey|TestValidateCache|^TestActionPinSHAsMatchVersionTags|^TestAction[^P]|Container|Dependabot|Security|PII|TestPermissions|TestPackageExtractor|TestCollectPackagesFromWorkflow|TestExpression|TestValidateExpressionSafety|TestCheckNetworkSupport|TestValidateStrictMCPNetwork|TestJobManager|TestWorkflowStep|TestScriptRegistry|TestAgent|TestCopilot|TestCustom|TestEngine|TestModel|TestNetwork|TestOpenAI|TestProvider|String|Sanitize|Normalize|Trim|Clean|Format|Runtime|Setup|Install|Download|Version|Binary" concurrency: group: ci-${{ github.ref }}-integration-${{ matrix.test-group.name }} cancel-in-progress: true From 71967e9d9b3d30ab8809fee0d288dd2909c56c83 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 10:54:00 +0000 Subject: [PATCH 09/10] Initial plan From 21ff090416c369665ca0e92bedf9f937862051e7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 25 Dec 2025 11:04:21 +0000 Subject: [PATCH 10/10] Recompile workflows after merging main Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/aw/schemas/agentic-workflow.json | 96 ++++++++++++------- .../agent-performance-analyzer.lock.yml | 2 +- .github/workflows/ai-moderator.lock.yml | 2 +- .github/workflows/archie.lock.yml | 2 +- .github/workflows/artifacts-summary.lock.yml | 2 +- .github/workflows/audit-workflows.lock.yml | 2 +- .github/workflows/blog-auditor.lock.yml | 2 +- .github/workflows/brave.lock.yml | 2 +- .../breaking-change-checker.lock.yml | 2 +- .github/workflows/campaign-generator.lock.yml | 2 +- .github/workflows/campaign-manager.lock.yml | 2 +- .github/workflows/changeset.lock.yml | 2 +- .github/workflows/ci-coach.lock.yml | 2 +- .github/workflows/ci-doctor.lock.yml | 2 +- .../cli-consistency-checker.lock.yml | 2 +- .../workflows/cli-version-checker.lock.yml | 2 +- .github/workflows/cloclo.lock.yml | 2 +- .../workflows/close-old-discussions.lock.yml | 2 +- .../commit-changes-analyzer.lock.yml | 2 +- .../workflows/copilot-agent-analysis.lock.yml | 2 +- .../copilot-pr-merged-report.lock.yml | 2 +- .../copilot-pr-nlp-analysis.lock.yml | 2 +- .../copilot-pr-prompt-analysis.lock.yml | 2 +- .../copilot-session-insights.lock.yml | 2 +- .github/workflows/craft.lock.yml | 2 +- .../daily-assign-issue-to-user.lock.yml | 2 +- .github/workflows/daily-choice-test.lock.yml | 2 +- .github/workflows/daily-code-metrics.lock.yml | 2 +- .../daily-copilot-token-report.lock.yml | 2 +- .github/workflows/daily-doc-updater.lock.yml | 2 +- .github/workflows/daily-fact.lock.yml | 2 +- .github/workflows/daily-file-diet.lock.yml | 2 +- .../workflows/daily-firewall-report.lock.yml | 2 +- .../workflows/daily-issues-report.lock.yml | 2 +- .../daily-malicious-code-scan.lock.yml | 2 +- .../daily-multi-device-docs-tester.lock.yml | 2 +- .github/workflows/daily-news.lock.yml | 2 +- .../daily-performance-summary.lock.yml | 2 +- .../workflows/daily-repo-chronicle.lock.yml | 2 +- .github/workflows/daily-team-status.lock.yml | 2 +- .../workflows/daily-workflow-updater.lock.yml | 2 +- .github/workflows/deep-report.lock.yml | 2 +- .../workflows/dependabot-go-checker.lock.yml | 2 +- .github/workflows/dev-hawk.lock.yml | 2 +- .github/workflows/dev.lock.yml | 2 +- .../developer-docs-consolidator.lock.yml | 2 +- .github/workflows/dictation-prompt.lock.yml | 2 +- .github/workflows/docs-noob-tester.lock.yml | 2 +- .../duplicate-code-detector.lock.yml | 2 +- .../example-workflow-analyzer.lock.yml | 2 +- .../github-mcp-structural-analysis.lock.yml | 2 +- .../github-mcp-tools-report.lock.yml | 2 +- .../workflows/glossary-maintainer.lock.yml | 2 +- .github/workflows/go-fan.lock.yml | 2 +- ...ze-reduction-project64.campaign.g.lock.yml | 2 +- .github/workflows/go-logger.lock.yml | 2 +- .../workflows/go-pattern-detector.lock.yml | 2 +- .github/workflows/grumpy-reviewer.lock.yml | 2 +- .github/workflows/hourly-ci-cleaner.lock.yml | 2 +- .../workflows/human-ai-collaboration.lock.yml | 2 +- .github/workflows/incident-response.lock.yml | 2 +- .../workflows/instructions-janitor.lock.yml | 2 +- .github/workflows/intelligence.lock.yml | 2 +- .github/workflows/issue-arborist.lock.yml | 2 +- .github/workflows/issue-classifier.lock.yml | 4 +- .github/workflows/issue-monster.lock.yml | 2 +- .github/workflows/issue-triage-agent.lock.yml | 2 +- .github/workflows/jsweep.lock.yml | 2 +- .../workflows/layout-spec-maintainer.lock.yml | 2 +- .github/workflows/lockfile-stats.lock.yml | 2 +- .github/workflows/mcp-inspector.lock.yml | 2 +- .github/workflows/mergefest.lock.yml | 2 +- .../workflows/notion-issue-summary.lock.yml | 2 +- .github/workflows/org-health-report.lock.yml | 2 +- .github/workflows/org-wide-rollout.lock.yml | 2 +- .github/workflows/pdf-summary.lock.yml | 2 +- .github/workflows/plan.lock.yml | 2 +- ...ayground-org-project-update-issue.lock.yml | 2 +- .../playground-snapshots-refresh.lock.yml | 2 +- .github/workflows/poem-bot.lock.yml | 2 +- .github/workflows/portfolio-analyst.lock.yml | 2 +- .../workflows/pr-nitpick-reviewer.lock.yml | 2 +- .../prompt-clustering-analysis.lock.yml | 2 +- .github/workflows/python-data-charts.lock.yml | 2 +- .github/workflows/q.lock.yml | 2 +- .github/workflows/release.lock.yml | 8 +- .github/workflows/repo-tree-map.lock.yml | 2 +- .../repository-quality-improver.lock.yml | 2 +- .github/workflows/research.lock.yml | 2 +- .github/workflows/safe-output-health.lock.yml | 2 +- .../schema-consistency-checker.lock.yml | 2 +- .github/workflows/scout.lock.yml | 2 +- .../workflows/security-compliance.lock.yml | 2 +- .github/workflows/security-fix-pr.lock.yml | 2 +- .../semantic-function-refactor.lock.yml | 2 +- .../workflows/slide-deck-maintainer.lock.yml | 2 +- .github/workflows/smoke-claude.lock.yml | 2 +- .../workflows/smoke-codex-firewall.lock.yml | 2 +- .github/workflows/smoke-codex.lock.yml | 2 +- .../smoke-copilot-no-firewall.lock.yml | 2 +- .../smoke-copilot-playwright.lock.yml | 2 +- .../smoke-copilot-safe-inputs.lock.yml | 2 +- .github/workflows/smoke-detector.lock.yml | 2 +- .github/workflows/smoke-srt.lock.yml | 2 +- .github/workflows/spec-kit-execute.lock.yml | 2 +- .github/workflows/spec-kit-executor.lock.yml | 2 +- .github/workflows/speckit-dispatcher.lock.yml | 2 +- .../workflows/stale-repo-identifier.lock.yml | 4 +- .../workflows/static-analysis-report.lock.yml | 2 +- .github/workflows/sub-issue-closer.lock.yml | 2 +- .github/workflows/super-linter.lock.yml | 4 +- .../workflows/technical-doc-writer.lock.yml | 2 +- .github/workflows/tidy.lock.yml | 2 +- .github/workflows/typist.lock.yml | 2 +- .github/workflows/unbloat-docs.lock.yml | 2 +- .github/workflows/video-analyzer.lock.yml | 2 +- .../workflows/weekly-issue-summary.lock.yml | 2 +- .github/workflows/workflow-generator.lock.yml | 2 +- .../workflow-health-manager.lock.yml | 2 +- pkg/workflow/js/sanitize_content_core.cjs | 5 +- 120 files changed, 187 insertions(+), 162 deletions(-) diff --git a/.github/aw/schemas/agentic-workflow.json b/.github/aw/schemas/agentic-workflow.json index 510479189b5..ecc885b6a4a 100644 --- a/.github/aw/schemas/agentic-workflow.json +++ b/.github/aw/schemas/agentic-workflow.json @@ -285,10 +285,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -305,10 +302,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -414,7 +408,8 @@ "items": { "type": "string", "description": "Label name" - } + }, + "minItems": 1 } ] } @@ -431,10 +426,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -451,10 +443,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -486,7 +475,8 @@ "items": { "type": "string", "description": "Label name" - } + }, + "minItems": 1 } ] }, @@ -669,10 +659,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ] @@ -996,10 +983,7 @@ }, { "not": { - "anyOf": [ - { "required": ["branches"] }, - { "required": ["branches-ignore"] } - ] + "anyOf": [{ "required": ["branches"] }, { "required": ["branches-ignore"] }] } } ], @@ -1016,10 +1000,7 @@ }, { "not": { - "anyOf": [ - { "required": ["paths"] }, - { "required": ["paths-ignore"] } - ] + "anyOf": [{ "required": ["paths"] }, { "required": ["paths-ignore"] }] } } ] @@ -1866,6 +1847,7 @@ } }, "network": { + "$comment": "Strict mode requirements: When strict=true, the 'network' field must be present (not null/undefined) and cannot contain wildcard '*' in allowed domains. This is validated in Go code (pkg/workflow/strict_mode_validation.go) via validateStrictNetwork().", "description": "Network access control for AI engines using ecosystem identifiers and domain allowlists. Controls web fetch and search capabilities.", "examples": [ "defaults", @@ -1899,7 +1881,8 @@ "items": { "type": "string", "description": "Domain name or ecosystem identifier (supports wildcards like '*.example.com' and ecosystem names like 'python', 'node')" - } + }, + "$comment": "Empty array is valid and means deny all network access. Omit the field entirely or use network: defaults to use default network permissions." }, "firewall": { "description": "AWF (Agent Workflow Firewall) configuration for network egress control. Only supported for Copilot engine.", @@ -2124,12 +2107,14 @@ "properties": { "command": { "type": "string", - "description": "Custom command to execute the MCP gateway (mutually exclusive with 'container')" + "$comment": "Mutually exclusive with 'container' - only one execution mode can be specified.", + "description": "Custom command to execute the MCP gateway" }, "container": { "type": "string", "pattern": "^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*$", - "description": "Container image for the MCP gateway executable (mutually exclusive with 'command')" + "$comment": "Mutually exclusive with 'command' - only one execution mode can be specified.", + "description": "Container image for the MCP gateway executable" }, "version": { "type": ["string", "number"], @@ -2148,7 +2133,8 @@ "items": { "type": "string" }, - "description": "Arguments to add after the container image (container entrypoint arguments, only valid with 'container')" + "$comment": "Requires 'container' to be specified - entrypoint arguments only apply to container execution.", + "description": "Arguments to add after the container image (container entrypoint arguments)" }, "env": { "type": "object", @@ -2172,7 +2158,35 @@ "description": "API key for authenticating with the MCP gateway (supports ${{ secrets.* }} syntax)" } }, - "additionalProperties": false + "additionalProperties": false, + "anyOf": [ + { + "required": ["command"] + }, + { + "required": ["container"] + } + ], + "not": { + "allOf": [ + { + "required": ["command"] + }, + { + "required": ["container"] + } + ] + }, + "allOf": [ + { + "if": { + "required": ["entrypointArgs"] + }, + "then": { + "required": ["container"] + } + } + ] } }, "additionalProperties": false @@ -2406,7 +2420,9 @@ "stargazers", "users" ] - } + }, + "minItems": 1, + "$comment": "At least one toolset is required when toolsets array is specified. Use null or omit the field to use all toolsets." } }, "additionalProperties": false, @@ -3155,6 +3171,7 @@ }, "safe-outputs": { "type": "object", + "$comment": "Strict mode dependency: When strict=true AND permissions contains write values (contents:write, issues:write, or pull-requests:write), safe-outputs must be configured. This relationship is validated in Go code (pkg/workflow/strict_mode_validation.go) via validateStrictPermissions() because it requires complex logic to check if ANY permission property equals 'write', which cannot be expressed concisely in JSON Schema.", "description": "Safe output processing configuration that automatically creates GitHub issues, comments, and pull requests from AI workflow output without requiring write permissions in the main job", "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, close-discussion, close-issue, close-pull-request, create-agent-task, create-code-scanning-alert, create-discussion, create-issue, create-pull-request, create-pull-request-review-comment, hide-comment, link-sub-issue, missing-tool, noop, push-to-pull-request-branch, threat-detection, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-asset. See documentation for complete details.", "properties": { @@ -4861,6 +4878,7 @@ "strict": { "type": "boolean", "default": true, + "$comment": "Strict mode enforces several security constraints that are validated in Go code (pkg/workflow/strict_mode_validation.go) rather than JSON Schema: (1) Write Permissions + Safe Outputs: When strict=true AND permissions contains write values (contents:write, issues:write, pull-requests:write), safe-outputs must be configured. This relationship is too complex for JSON Schema as it requires checking if ANY permission property has a 'write' value. (2) Network Requirements: When strict=true, the 'network' field must be present and cannot contain wildcard '*'. (3) MCP Container Network: Custom MCP servers with containers require explicit network configuration. (4) Action Pinning: Actions must be pinned to commit SHAs. These are enforced during compilation via validateStrictMode().", "description": "Enable strict mode validation for enhanced security and compliance. Strict mode enforces: (1) Write Permissions - refuses contents:write, issues:write, pull-requests:write; requires safe-outputs instead, (2) Network Configuration - requires explicit network configuration with no wildcard '*' in allowed domains, (3) Action Pinning - enforces actions pinned to commit SHAs instead of tags/branches, (4) MCP Network - requires network configuration for custom MCP servers with containers, (5) Deprecated Fields - refuses deprecated frontmatter fields. Can be enabled per-workflow via 'strict: true' in frontmatter, or disabled via 'strict: false'. CLI flag takes precedence over frontmatter (gh aw compile --strict enforces strict mode). Defaults to true. See: https://githubnext.github.io/gh-aw/reference/frontmatter/#strict-mode-strict", "examples": [true, false] }, @@ -5308,12 +5326,14 @@ "command": { "type": "string", "minLength": 1, + "$comment": "Mutually exclusive with 'container' - only one execution mode can be specified. Validated by 'not.allOf' constraint below.", "description": "Command for stdio MCP connections" }, "container": { "type": "string", "pattern": "^[a-zA-Z0-9][a-zA-Z0-9/:_.-]*$", - "description": "Container image for stdio MCP connections (alternative to command)" + "$comment": "Mutually exclusive with 'command' - only one execution mode can be specified. Validated by 'not.allOf' constraint below.", + "description": "Container image for stdio MCP connections" }, "version": { "type": ["string", "number"], @@ -5346,6 +5366,7 @@ }, "network": { "type": "object", + "$comment": "Requires 'container' to be specified - network configuration only applies to container-based MCP servers. Validated by 'if/then' constraint in 'allOf' below.", "properties": { "allowed": { "type": "array", @@ -5378,6 +5399,7 @@ } }, "additionalProperties": false, + "$comment": "Validation constraints: (1) Mutual exclusion: 'command' and 'container' cannot both be specified. (2) Requirement: Either 'command' or 'container' must be provided (via 'anyOf'). (3) Dependency: 'network' requires 'container' (validated in 'allOf'). (4) Type constraint: When 'type' is 'stdio' or 'local', either 'command' or 'container' is required.", "anyOf": [ { "required": ["type"] diff --git a/.github/workflows/agent-performance-analyzer.lock.yml b/.github/workflows/agent-performance-analyzer.lock.yml index 0621e9f0b27..0766e54f989 100644 --- a/.github/workflows/agent-performance-analyzer.lock.yml +++ b/.github/workflows/agent-performance-analyzer.lock.yml @@ -3204,7 +3204,7 @@ jobs: let previous; do { previous = s; - s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(//g, "").replace(//g, ""); + s = s.replace(/ and malformed + // Use a single regex that matches both patterns to avoid introducing comment markers + // when removing one pattern reveals another // Apply repeatedly to handle nested/overlapping patterns that could reintroduce comment markers let previous; do { previous = s; - s = s.replace(//g, "").replace(//g, ""); + // Single regex matches: OR + s = s.replace(/