fix(scripts): bump PirateWeatherADV timeouts to v3, correct 10s→30s docs

[Yeraze]

Summary

Closes #3941.

• The bundled examples/auto-responder-scripts/PirateWeatherADV.py sized its per-request Nominatim/Pirate Weather urlopen timeouts (3s/4s/3s) against a stale "MeshMonitor enforces... a 10-second script timeout" comment. I verified against src/server/meshtasticManager.ts that both the Auto-Responder path and the Timed Event path actually kill scripts after 30 seconds (execFileAsync(..., { timeout: 30000 })), not 10.
• A slow Nominatim/Pirate Weather response could exceed the old, too-tight per-call socket timeouts well before the real 30s kill signal, producing the The read operation timed out error reported in User Script Submission update PirateWeatherADV v3 #3941 (mostly seen via Timed Events).
• Adopts the reporter's community-submitted v3 fix: bumps the three urlopen timeouts to 5s/8s/5s as named constants (TIMEOUT_GEOCODE/TIMEOUT_API/TIMEOUT_REVERSE) and corrects the misleading comment. I diffed the submitted v3 attachment against the currently-bundled script to confirm this was the full extent of the intended change (just timeouts + comment) before applying it verbatim.
• Corrects the same stale "10-second" claim everywhere else it was documented (verified as wrong in every case, since the real value is 30s): docs/features/automation.md (5 spots), docs/developers/auto-responder-scripting.md (2 spots), docs/user-scripts.md, examples/auto-responder-scripts/README.md.
• Adds a CHANGELOG entry under [Unreleased].

Test plan

• python3 -m py_compile examples/auto-responder-scripts/PirateWeatherADV.py — syntax OK
• Diffed the fixed script byte-for-byte against the reporter's submitted v3 attachment — matches exactly
• Grepped repo-wide for remaining "10-second"/"10 second" script-timeout claims — none left (other unrelated 10-second references, e.g. TCP connect timeout and heap-reboot disconnect duration, are untouched as they describe different mechanisms)
• No automated tests cover this example script or docs content; CI (lint/build/vitest) will validate no regressions elsewhere

🤖 Generated with Claude Code

https://claude.ai/code/session_01Shsuu7NEH9Z7w1va1YcpZj

---

Generated by Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(scripts): bump PirateWeatherADV timeouts to v3, correct 10s→30s docs

• Read changed files
• Verify timeout claims against source code
• Review documentation changes
• Provide feedback

---

Summary

This is a straightforward, well-scoped fix. The PR's central claim — that the actual script kill timeout is 30 seconds, not 10 — is verified correct in src/server/meshtasticManager.ts:

const SCRIPT_AUTO_RESPONDER_TIMEOUT_MS = 30_000;
// ...
const { stdout, stderr } = await execFileAsync(interpreter, [resolvedPath, ...scriptArgsList], {
  timeout: SCRIPT_AUTO_RESPONDER_TIMEOUT_MS,
  ...
});

The same 30s timeout also applies to the Timed Event (geofence/timer) code paths (timeout: 30000 in both).

---

Code Review

examples/auto-responder-scripts/PirateWeatherADV.py

The timeout constants and comment are correct and clear:

# The Auto-Responder hard kills scripts after 30 seconds (execFileAsync timeout).
# We set individual HTTP timeouts conservatively so the total worst-case network
# time stays well under 20s, leaving a safe buffer before the 30s kill signal:
TIMEOUT_GEOCODE = 5   # forward geocode (Nominatim)
TIMEOUT_API     = 8   # Pirate Weather API fetch
TIMEOUT_REVERSE = 5   # reverse geocode (Nominatim)

Total worst-case: 5+8+5 = 18 seconds against a 30-second hard kill. This is a reasonable safety margin.

One minor observation: in GPS weather mode, both get_weather() and reverse_geocode_city() are called sequentially, so the actual worst-case is TIMEOUT_GEOCODE(5) + TIMEOUT_API(8) + TIMEOUT_REVERSE(5) = 18s for forecast mode, or TIMEOUT_API(8) + TIMEOUT_REVERSE(5) = 13s for GPS weather. The comment's "sum of all three" correctly captures the worst case (forecast + GPS mode with reverse geocode). No issue here.

Documentation

All 5 occurrences in docs/features/automation.md, 2 in docs/developers/auto-responder-scripting.md, 1 in docs/user-scripts.md, and 1 in examples/auto-responder-scripts/README.md are corrected consistently.

CHANGELOG.md

Entry is accurate and well-placed under [Unreleased].

---

Issues Found

None. This is a correct, focused fix with no functional changes to production code.

---

Verdict

✅ Approved for merge. The fix is accurate, the timeout arithmetic is sound (5+8+5=18s worst case vs 30s kill budget), and the grep confirms no stale "10-second" documentation references remain anywhere in the repo that relate to script execution.

[Yeraze]

Folded in the follow-up fixes from @Nullvoid3771's second v3 attachment (validated in @nodezero's review above) — commit 6349606e:

1. _fmt_label/_fmt_part — 1–3 letter alpha tokens are now uppercased as abbreviations regardless of input case (on → ON), not just preserved when already uppercase.
2. resolve_input — guard so a location that normalises to empty (e.g. just commas) falls through to the GPS-coordinate fallback instead of calling geocode_location('').
3. main() exception handler — redacts PIRATE_WEATHER_API_KEY from any exception text before broadcasting the error on the mesh (key-leak guard).

Also applied the module-docstring improvements from the attachment plus the two function-docstring corrections NodeZero flagged (_fmt_label and resolve_input) — the attachment itself hadn't updated those. Script compiles and the built-in TEST_MODE run exercises the new empty→GPS fallback path correctly. Timeout constants (5/8/5) from the original v3 fix are retained.

-- folded in on behalf of the reviewers; leaving the merge to @Yeraze.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(scripts): bump PirateWeatherADV timeouts to v3, correct 10s→30s docs

• Read changed files
• Review code quality and security
• Review documentation changes
• Provide feedback

---

Summary

This PR does two things: (1) bumps PirateWeatherADV.py socket timeouts to 5s/8s/5s and corrects the wrong "10-second" comment, and (2) adds three behavioral improvements from the reporter's v3 attachment. The timeout claim is verified correct — meshtasticManager.ts uses SCRIPT_AUTO_RESPONDER_TIMEOUT_MS = 30_000 and both Timed Event code paths also use timeout: 30000.

---

Code Quality

_fmt_label / _fmt_part (lines 271–302)

The new abbreviation logic works correctly but has a subtle inconsistency in the inner loop at lines 295–298:

if w.isupper() and len(w) <= 3:
    out.append(w)           # preserve already-uppercase abbreviation
else:
    out.append(w[0].upper() + w[1:])

In the multi-word path (e.g. "new york, on" where "on" is multi-word), the single-token guard at line 289 catches single short tokens and uppercases them. For the multi-word path, the inner loop at line 295 only preserves short tokens that are already uppercase — it won't uppercase "on" if it ends up in a multi-word part. This is an edge case (short abbreviation embedded in a multi-word phrase like "on something") and practically harmless, but worth noting.

resolve_input — empty-location guard (lines 404–408)

if not location_normalised:
    location = ''
else:
    coords = geocode_location(location_normalised)

Correct — this prevents calling geocode_location('') which would cause a confusing API error. The fall-through to GPS mode is the right behavior.

API key redaction (lines 514, 721–723)

Both call sites correctly redact PIRATE_WEATHER_API_KEY before the message is broadcast. The guard if PIRATE_WEATHER_API_KEY: before calling .replace() avoids a spurious empty-string replace. This is the most security-relevant change in the PR and is implemented correctly.

One minor stylistic note: the get_weather exception handler at line 514 does the replacement inline, while main() at line 721 does it in a separate variable. Both are fine — they're in different exception scopes — but they could be unified via a helper for DRYness. Not a blocking concern for an example script.

---

Security

API key redaction is the key security improvement here. The guard correctly prevents the key from appearing in mesh broadcasts if an unexpected exception includes the URL (which embeds the key). No concerns.

---

Performance

Timeout arithmetic: TIMEOUT_GEOCODE(5) + TIMEOUT_API(8) + TIMEOUT_REVERSE(5) = 18s worst-case vs. a 30s hard kill. The 12s buffer is reasonable. No performance concerns.

---

Test Coverage

As noted in the PR, there are no automated tests for example scripts or docs. The manual test plan is appropriate for the scope of this change (py_compile + byte-for-byte diff against the submitted attachment + repo-wide grep for stale claims).

---

Documentation

All 10-second → 30-second corrections in docs/features/automation.md (5 spots), docs/developers/auto-responder-scripting.md (2 spots), docs/user-scripts.md, and examples/auto-responder-scripts/README.md are consistent and accurate.

---

Issues Found

None blocking. One minor edge case in _fmt_part (abbreviation not uppercased when embedded in a multi-word part) and a stylistic note about the two slightly different API key redaction call sites — neither warrants changes in an example script.

---

Verdict

✅ Looks good. The timeout values are correct, the arithmetic is sound, the security fix (API key redaction) is implemented correctly in both exception handlers, and the documentation corrections are accurate and complete.

View job