Skip to content

Switch embedded weather skill to keyless wttr.in endpoint#354

Open
zaidoskate wants to merge 2 commits into
initializ:mainfrom
zaidoskate:feature/weather-skill-keyless-fix
Open

Switch embedded weather skill to keyless wttr.in endpoint#354
zaidoskate wants to merge 2 commits into
initializ:mainfrom
zaidoskate:feature/weather-skill-keyless-fix

Conversation

@zaidoskate

@zaidoskate zaidoskate commented Jul 21, 2026

Copy link
Copy Markdown

Type of Change

  • Bug fix

Description

The embedded weather skill was dead on arrival: it only allowed egress to api.openweathermap.org and api.weatherapi.com, both of which require an API key, while the skill declared no env.required key. 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 runnable curl commands to both tools.

General Checklist

  • Tests pass for affected modules (go test ./...)
  • Code is formatted (gofmt -w)
  • Linter passes (golangci-lint run)
  • go vet reports no issues
  • No new egress domains added without justification

Skill Contribution Checklist

  • forge skills validate passes with no errors
  • forge skills audit reports no policy violations
  • egress_domains lists every domain the skill contacts
  • No secrets or credentials are hardcoded
  • SKILL.md includes ## Tool: sections with input/output tables
  • Skill tested locally with expected input/output

Related Issues

Closes #349

Notes

  • forge skills validate expects a project-level forge.yaml/skills/ layout rather than the embedded skills path, so I validated via forge skills audit --embedded and --dir instead — both report weather at low risk (3/100) with no violations.
  • Corrected the forecast range from "1-7" to "1-3" days in SKILL.md, since wttr.in's free JSON endpoint caps at 3 days (today + 2).
  • Side note: while testing with forge skills audit, I noticed egress domains aren't reported correctly for weather (and pre-existing, for github too, which I didn't touch) — seems unrelated to this fix. Happy to open a separate issue if useful.

@initializ-mk initializ-mk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

  1. Add the two script files under forge-skills/local/embedded/weather/scripts/ (committed executable, chmod 755):
    • weather-current.sh
    • weather-forecast.sh
      The runtime invokes each with the tool's JSON input as an argument (SkillCommandExecutor runs the script and passes the input), so each script should read $1 as JSON.
  2. Add jq to bins (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=j1 dump would push the entire blob into the model context.
  3. Inside each script: read the JSON arg, pull location (and days for the forecast) with jq, 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 days input entirely (?format=j1 returns 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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

@zaidoskate

zaidoskate commented Jul 22, 2026

Copy link
Copy Markdown
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Weather embedded skill points at key-gated APIs (never works) — switch to a keyless service

2 participants