Fix api.weather.gov/points response handling in WeatherTools

[stvansolano]

Calls to #GetForecast fail for a basic query. Periods is not present so using /Forecast attribute instead

Forecast docs: https://www.weather.gov/documentation/services-web-api

How do I get the forecast?
Forecasts are created at each [NWS Weather Forecast Office (WFO)](https://www.weather.gov/srh/nwsoffices) on their own grid definition, at a resolution of about 2.5km x 2.5km. The API endpoint for the 12h forecast periods at a specific grid location is formatted as:

https://api.weather.gov/gridpoints/{office}/{gridX},{gridY}/forecast

For example: https://api.weather.gov/gridpoints/TOP/31,80/forecast

Motivation and Context

Fixes sample code for WeatherTools

How Has This Been Tested?

Breaking Changes

N/A

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update

Checklist

• I have read the MCP Documentation
• My code follows the repository's style guidelines
• New and existing tests pass locally
• I have added appropriate error handling
• I have added or updated documentation as needed

Additional context

N/A

[stvansolano]

+ @stephentoub @aaronpowell found this.

[PederHP]

Looks good to me.
nit: I prefer string.Join to Aggregate

[aaronpowell]

The intent of the sample was to be parity with https://github.com/modelcontextprotocol/quickstart-resources/blob/main/weather-server-python/weather.py#L57-L90 and https://github.com/modelcontextprotocol/quickstart-resources/blob/main/weather-server-typescript/src/index.ts#L141-L203 but the change here deviates from it, so I don't think it's needed.

[stephentoub]

@aaronpowell, how does this relate to the claim in the opening post that Calls to #GetForecast fail for a basic query. ?

I'm still not clear... is the sample broken as it currently is?

[aaronpowell]

@stephentoub I think we have a different bug, I've just tried it out on main and it appears that the HttpClient is being read an input parameter to the tool call. This is what I see in the tools/list call:

    {
      "name": "GetForecast",
      "description": "Get weather forecast for a location.",
      "inputSchema": {
        "type": "object",
        "properties": {
          "client": {
            "type": "object",
            "properties": {
              "defaultRequestHeaders": {
                "type": "array",
                "items": {
                  "type": "object",
                  "properties": {
                    "key": {
                      "type": [
                        "string",
                        "null"
                      ]
                    },
                    "value": {
                      "type": [
                        "array",
                        "null"
                      ],
                      "items": {
                        "type": "string"
                      }
                    }
                  },
                  "additionalProperties": false,
                  "required": [
                    "key",
                    "value"
                  ]
                }
              },
              "defaultRequestVersion": {
                "$comment": "Represents a version string.",
                "type": "string"
              },
              "defaultVersionPolicy": {
                "type": "string",
                "enum": [
                  "RequestVersionOrLower",
                  "RequestVersionOrHigher",
                  "RequestVersionExact"
                ]
              },
              "baseAddress": {
                "type": [
                  "string",
                  "null"
                ]
              },
              "timeout": {
                "$comment": "Represents a System.TimeSpan value.",
                "type": "string"
              },
              "maxResponseContentBufferSize": {
                "type": "integer"
              }
            },
            "additionalProperties": false,
            "required": [
              "defaultRequestHeaders",
              "defaultRequestVersion",
              "defaultVersionPolicy",
              "baseAddress",
              "timeout",
              "maxResponseContentBufferSize"
            ]
          },
          "latitude": {
            "description": "Latitude of the location.",
            "type": "number"
          },
          "longitude": {
            "description": "Longitude of the location.",
            "type": "number"
          }
        },
        "title": "GetForecast",
        "description": "Get weather forecast for a location.",
        "required": [
          "client",
          "latitude",
          "longitude"
        ]
      }
    }

[stephentoub]

I think we have a different bug, I've just tried it out on main and it appears that the HttpClient is being read an input parameter to the tool call.

That's because you didn't add HttpClient to DI, so McpServerTool can't satisfy that parameter from the IServiceProvider.

[stephentoub]

Sorry, I take that back, you did add it here:

csharp-sdk/samples/QuickstartWeatherServer/Program.cs

Line 16 in 8f1a123

return client;

[stephentoub]

It's a bug in an argument in WithTools. I'll fix it.

[stephentoub]

#152

[stvansolano]

Hey @aaronpowell sorry, still not clear to me. The code initially failed for me since the call can't get the attribute and then it fails.

Plus, Weather API point to use /forecast they provide in docs. Did I miss a recent change or something else added recently to sort it out?

[aaronpowell]

As noted, there's a bug that causes the pipeline to think that HttpClient is a parameter not something injected via DI, so the problem becomes that it doesn't set the endpoint for HttpClient properly or it sometimes does but it doesn't set a User Agent, which then gets rejected by weather.gov.

Once #152 merges in, it should fix that problem.

[stephentoub]

Thanks. In that case, @stvansolano, can this be closed?

[stvansolano]

Fixed conflicts. @aaronpowell @stephentoub should we merge this one on the API calls / fix sample or is already covered per other discussions/fixes/PRs? Sorry, got confused again 😆

modelcontextprotocol/docs#230
modelcontextprotocol/docs#228

[stvansolano]

@halter73 went through some weird PR build issues for the past days. Now they're gone. Can we merge this?

[DGuhr]

Jftr, as #230 is mentioned, which I wrote: There's still a problem with internationalization, as the double values latitude/longitude are transformed e.g. in germany (where I live) as comma-seperated, not dot-separated strings. This lets the api call fail, effectively rendering the example not working for all folks in those areas. It's already fixed in modelcontextprotocol/docs#228 , and I'm not sure you have it on the radar, so before the other one gets closed and this one makes it, I wanted to call it out and hope you'll add the (trivial) fix - see my comment on modelcontextprotocol/docs#230

[stvansolano]

Can we track it as improvement or separate PR maybe @DGuhr? @stephentoub thoughts?

[halter73]

I definitely agree perfect shouldn't be the enemy of good, and I see the current WeatherTools.GetForecast implementation gives a "the given key was not present in the dictionary" error when I pass it 47.6,-122.3 (Seattle) for the lat,lon pair, so we should definitely fix it.

However, I don't want to merge it with the ?? $"/{properties.GetProperty("gridId")}... logic since that will never work considering it doesn't start with "/gridpoints". We should just fail whenever the forecast URL isn't supplied by the /points/lat,lon response. The likelihood of us being able to guess the gridpoints URL from response.properties if api.weather.gov removes response.properties.forecast part is basically none.