Switch embedded weather skill to keyless wttr.in endpoint#354
Switch embedded weather skill to keyless wttr.in endpoint#354zaidoskate wants to merge 2 commits into
Conversation
initializ-mk
left a comment
There was a problem hiding this comment.
Really nice diagnosis on this one — you correctly identified both halves of the bug (egress allowed only key-gated hosts and the tool sections had nothing runnable), and picking wttr.in because it takes a city name directly and maps 1:1 to the location input is exactly the right call. The 1-7 → 1-3 forecast correction is spot on (wttr.in's free j1 endpoint really does cap at 3 days), and you kept the blast radius in sync nicely — the egress-wizard hint and the scanner test fixture both updated. That attention to the surrounding call sites is the part contributors usually miss.
There's one thing left to make the skill actually run, and then this is a complete fix. Right now the ```bash blocks inside SKILL.md are documentation — the skill runtime never executes them. A ## Tool: entry only becomes a callable tool if there's a matching script file on disk; here's the exact resolution logic in `registerSkillTools` (forge-cli/runtime/runner.go):
if scriptPath == "" {
continue // No script file, skip
}
st = tools.NewSkillTool(entry.Name, entry.Description, entry.InputSpec, scriptPath, skillExec)It looks for skills/<skill>/scripts/<tool-name>.sh, where <tool-name> is the tool name with underscores turned into hyphens. So weather_current needs scripts/weather-current.sh and weather_forecast needs scripts/weather-forecast.sh. Since the weather/ dir currently ships only SKILL.md (no scripts/), both tools hit that continue and register nothing — the skill loads but exposes no runnable tool.
What finishes the fix
- Add the two script files under
forge-skills/local/embedded/weather/scripts/(committed executable,chmod 755):weather-current.shweather-forecast.sh
The runtime invokes each with the tool's JSON input as an argument (SkillCommandExecutorruns the script and passes the input), so each script should read$1as JSON.
- Add
jqtobins(bins: [curl, jq]) — the scripts need it both to parse the JSON input and to shape wttr.in's (very large) j1 response down to the handful of fields your SKILL.md documents. Right now a raw?format=j1dump would push the entire blob into the model context. - Inside each script: read the JSON arg, pull
location(anddaysfor the forecast) withjq, URL-encode the location before putting it in the path (jq -rn --arg s "$LOCATION" '$s|@uri'is the clean way — it also closes the injection gap that a bare${location}in a URL would open), check the HTTP status, and emit the shaped JSON your output tables promise.
A couple of smaller things that fall out naturally once the scripts exist:
- The forecast tool currently ignores its
daysinput entirely (?format=j1returns a fixed window) — the script should slice.weather[0:$days]so the documented input actually does something. - Worth a
set -euo pipefail+ input validation (empty/invalid JSON → a shaped{"error": ...}) so a bad call fails cleanly.
And yes please — do open that separate issue for the forge skills audit egress-domain reporting gap you spotted on weather and github. That's a real, independent bug and a genuinely useful catch; keeping it out of this PR is the right instinct.
This is close. Land the two scripts + jq and the skill goes from correctly-configured to actually-working. Happy to re-review as soon as you push them.
| **Output:** Current temperature, conditions, humidity, and wind speed | ||
|
|
||
| ```bash | ||
| curl -s "https://wttr.in/${location}?format=j1" |
There was a problem hiding this comment.
This ```bash block is documentation — the runtime won't execute it. For weather_current to be a callable tool, add `scripts/weather-current.sh` (tool name with `_`→`-`) next to this SKILL.md. The script receives the tool's JSON input as its first argument, so it should parse `$1` with `jq`, pull `.location`, URL-encode it (`jq -rn --arg s "$LOCATION" '$s|@uri'`), curl wttr.in, check the HTTP status, and emit the shaped fields you list just below (`temp_C`, `weatherDesc`, `humidity`, `windspeedKmph`) rather than the full j1 blob. You'll also want `jq` added to `bins` above.
| **Output:** Daily forecast with high/low temperatures and conditions | ||
|
|
||
| ```bash | ||
| curl -s "https://wttr.in/${location}?format=j1" |
There was a problem hiding this comment.
Same here — this needs a scripts/weather-forecast.sh to become runnable. One extra thing for the forecast: the tool declares a days input but this curl ignores it (?format=j1 returns a fixed window). Have the script read .days (clamp to 1–3, wttr.in's free cap) and slice .weather[0:$days] so the documented input actually shapes the output. Passing days through jq --argjson keeps it out of the shell.
|
Pushed the two scripts (weather-current.sh, weather-forecast.sh) under scripts/, added jq to bins, marked both executable, and tested locally with Git Bash. Forecast now slices to the requested days (clamped 1-3). Will open a separate issue for the audit egress-reporting gap shortly. Here is the new issue: https://github.com/initializ/forge/issues/357 |
Type of Change
Description
The embedded weather skill was dead on arrival: it only allowed egress to
api.openweathermap.organdapi.weatherapi.com, both of which require an API key, while the skill declared noenv.requiredkey. On top of that, the tool sections were prose-only with no runnable command — nothing was actually wired to make a request.This switches the skill to
wttr.in, which needs no API key for reasonable free traffic, and adds runnablecurlcommands to both tools.General Checklist
go test ./...)gofmt -w)golangci-lint run)go vetreports no issuesSkill Contribution Checklist
forge skills validatepasses with no errorsforge skills auditreports no policy violationsegress_domainslists every domain the skill contacts## Tool:sections with input/output tablesRelated Issues
Closes #349
Notes
forge skills validateexpects a project-levelforge.yaml/skills/layout rather than the embedded skills path, so I validated viaforge skills audit --embeddedand--dirinstead — both reportweatherat low risk (3/100) with no violations.SKILL.md, since wttr.in's free JSON endpoint caps at 3 days (today + 2).forge skills audit, I noticed egress domains aren't reported correctly forweather(and pre-existing, forgithubtoo, which I didn't touch) — seems unrelated to this fix. Happy to open a separate issue if useful.