Switch embedded weather skill to keyless wttr.in endpoint

[zaidoskate]

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]

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.

[initializ-mk]

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.

[initializ-mk]

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]

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

[initializ-mk]

Update review: runnable scripts added — this closes the gap 🎉

This fully finishes the fix. The two scripts now exist and register as first-class tools, so weather_current / weather_forecast are actually runnable — the "prose-only, nothing wired" half of #349 is genuinely resolved now. Really nice execution on the guidance.

Wiring ✅ — scripts committed 100755, jq added to bins, and NewSkillTool invokes them as bash <script> <json> with the JSON as $1, which is exactly how the runtime resolves weather_current → weather-current.sh.

Security ✅ — the jq -rn --arg s "$location" '$s|@uri' URL-encoding is the right injection defense, and every JSON output (including the error paths) is built with jq rather than hand-interpolated — so there's no JSON-injection surface and the error messages don't echo the raw location. That's actually cleaner than a naive error string would be.

Correctness ✅ — days is validated (^[0-9]+$, rejects floats/negatives), clamped to 1–3, and honored via .weather[0:$days]; HTTP status is checked; failures return shaped {error: ...} JSON.

Optional polish (all Low — none blocking)

1. Network-failure UX: with set -e, a curl DNS/connection failure exits before the shaped-error branch (HTTP 4xx/5xx are handled fine, since curl -s returns 0 for those). Capturing curl's exit would let transport errors also emit {error: ...} — see inline.
2. Numeric types: wttr.in's j1 returns temp_C/humidity/etc. as strings, so the output is "18" not 18. Add | tonumber if consumers expect numbers — see inline.
3. User-Agent: fine as-is (curl's default UA already gets JSON from format=j1); an explicit -H "User-Agent: curl" is belt-and-suspenders, not required.

Strong finish from a first-time contributor — took the direction and ran with it well. From my side this is ready; the nits are polish you can take or leave.

Maintainer note (@initializ-mk): CI hasn't run — external-fork PRs need a maintainer to approve the workflow run. Worth approving so the forge-skills suites exercise the new scripts before merge.

[initializ-mk]

Optional (Low): under set -e, if curl fails at the transport layer (DNS/connection), this assignment exits the script before the http_code != 200 branch — so a network failure produces no output rather than a shaped {error: ...}. HTTP 4xx/5xx are handled correctly (curl -s returns 0 for those). If you want transport errors to also emit JSON:

response=$(curl -s -w "\n%{http_code}" "https://wttr.in/${encoded_location}?format=j1") \
  || { jq -n '{error: "network request failed"}'; exit 0; }

Same line in weather-forecast.sh. Non-blocking.

[initializ-mk]

Optional (Low, cosmetic): wttr.in's j1 returns temp_C, humidity, windspeedKmph as JSON strings ("18"), so this emits string values. If the tool's consumers expect numbers, wrap with | tonumber (e.g. temperature_c: (.current_condition[0].temp_C | tonumber)). Take it or leave it depending on how the output is consumed.

[initializ-mk]

🔒 Security sign-off

Did a dedicated adversarial pass over both commits, treating location / days as attacker-influenced (they're LLM-chosen tool args downstream of user input). No security issues found — the scripts are hardened above the bar. Recording the analysis for the trail:

Command injection — clean.

• location never reaches shell eval: parsed out via jq -r, re-encoded via jq -rn --arg s "$location" '$s|@uri', placed in a quoted URL. No eval, no unquoted command-position expansion.
• days flows into bash arithmetic (( days < 1 )) — a real injection sink — but is regex-gated ([[ "$days" =~ ^[0-9]+$ ]]) before the (( )), so arithmetic only ever sees pure digits. Ordering is correct in both scripts (reversing those two lines would make it exploitable — good that it isn't).

SSRF / egress — clean.

• @uri percent-encodes / ? # @ :, so location can't change the host, traverse the path, or inject query params — the request stays on wttr.in.
• No -L (won't follow redirects to internal hosts); no -k (TLS cert validation on).
• Subprocess is egress-gated: egress_domains: [wttr.in] matches the actual call; off-allowlist is blocked by the proxy. Double-layered.

Response handling — clean. wttr.in body is parsed by jq (safe parser — a hostile/compromised response can't inject shell); malformed → || jq -n '{error:...}' fallback. Output is bounded shaped JSON.

Secrets / files — clean. Keyless (env.required: []), reads no env, prints no secrets, nothing on disk. 100755, plain shebang, invoked as bash <script> <json> (argv-separated, JSON as $1 — never shell-interpreted). set -euo pipefail.

Non-issues worth knowing (not vulnerabilities): no curl --max-time (bounded by the skill executor's process timeout); input="$1" under set -u fails closed if called with no arg; a user-influenced city name egresses to wttr.in (inherent to a weather skill, declared via egress_domains, non-sensitive).

Bottom line: nothing exploitable. The @uri encoding and the regex-before-arithmetic ordering are exactly the two things a careless version would get wrong, and both are right. ✅