[marklysze]

When weather coordinates are unset, the morning briefing currently queries San Francisco. This change skips the weather request and asks: “What city or town should I use for your weather forecast?” Missing, partial, malformed, non-finite, and out-of-range coordinates all take this path; valid coordinates (including zero) retain the existing forecast and unit behavior.

Start with src/morning-briefing.py and tests/morning-briefing-weather-location.test.py. The morning-briefing skill now tells the agent to preserve the question, clarify ambiguous cities, and save the answer using the existing coordinate configuration.

Related: #4378. This removes the assumed city; it does not add support for the separate location.json source requested in that issue, so it does not close it. No duplicate open PR found; previous weather PRs #3501, #3682, and #3894 are merged.

Verification

Parent: 054bd6a68; fix: 07cc33970. The following reproducible probe loads each revision's production code, leaves location unset, and intercepts network I/O with an offline error. It does not access personal data or deliver messages.

Save as /tmp/briefing-weather-evidence.py and run from the checkout:

import importlib.util
import inspect
import subprocess
import sys
from pathlib import Path
from unittest.mock import patch

root = Path.cwd()
path = root / 'src/morning-briefing.py'
sys.path.insert(0, str(path.parent))
revision = sys.argv[1]
source = subprocess.check_output(['git', 'show', f'{revision}:src/morning-briefing.py'], text=True)
spec = importlib.util.spec_from_file_location('briefing_evidence', path)
mod = importlib.util.module_from_spec(spec)
exec(compile(source, str(path), 'exec'), mod.__dict__)
with patch('sutando_config.config_get', side_effect=lambda key, default=None: default), patch.object(mod, '_run_applescript', return_value=('', '')), patch.object(mod, 'urlopen', side_effect=OSError('offline')) as request:
    weather = mod.get_weather()
    kwargs = {'weather_location_known': mod.weather_location_configured()} if 'weather_location_known' in inspect.signature(mod.synthesize).parameters else {}
    narrative = mod.synthesize(weather, [], [], [], [], [], **kwargs)
    print(f'revision: {revision}')
    print(f'weather requests: {request.call_count}')
    if request.called:
        print(f'URL: {request.call_args.args[0]}')
    print(f'asks for location: {"What city or town" in narrative}')
    print(narrative)

Actual output:

$ python3 /private/tmp/briefing-weather-evidence.py HEAD^
revision: HEAD^
weather requests: 1
URL: https://api.open-meteo.com/v1/forecast?latitude=37.77&longitude=-122.42&current=temperature_2m,weather_code&daily=temperature_2m_max,temperature_2m_min,precipitation_probability_max&timezone=auto&forecast_days=1&temperature_unit=fahrenheit
asks for location: False
Good morning. Your calendar is clear today. Everything looks clean. Good day for deep work.
$ python3 /private/tmp/briefing-weather-evidence.py HEAD
revision: HEAD
weather requests: 0
asks for location: True
Good morning. What city or town should I use for your weather forecast? Your calendar is clear today.

Tests (each matching file executed with python3, not unittest discovery, because filenames contain hyphens):

• tests/morning-briefing*.test.py and tests/briefing-*.test.py: 13/13 files passed on current main plus this fix.
• python3 tests/morning-briefing-weather-location.test.py: 18 tests passed, including main() writing the question into a temporary result file with zero weather requests.
• bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD):

prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

Limits and rollout

Draft: no real post-restart input → delivered-reply round trip has been performed. Tests verify generation in an isolated temporary workspace; live bridge delivery and an agent saving a user's city answer remain to be verified. No running service was restarted or configuration changed.

The question repeats with daily briefings until valid coordinates are saved. No migration or new permissions; revert the commit to roll back. This is based directly on main, not stacked.

Documentation: updated the canonical behavior instructions in skills/morning-briefing/SKILL.md. docs/catalog.json has no weather/briefing document; other docs/ changes and navigation changes are N/A.

Reviewer routing: the local collaboration-intelligence map is unavailable, so cross-channel reviewer identities could not be verified; no chat notifications were sent.

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/morning-briefing.py (100%)

Summary

• Total: 25 lines
• Missing: 0 lines
• Coverage: 100%

[bassilkhilo-ag2]

Approve. Reviewed at 07cc339.

What I verified:

• weather_coordinates() rejects missing, partial, non-numeric, NaN, inf and out-of-range values (NaN fails both range comparisons, inf fails the bound), and accepts 0/0. get_weather() returns None before any network call in every one of those cases, so the San Francisco fallback and the dead timezone AppleScript probe are fully gone.
• synthesize() keeps its old positional signature; the new keyword defaults to True, so any other caller keeps prior behavior. The only callers are in this file. The location question also suppresses "Everything looks clean", which is right since the briefing is now asking for something.
• A known location with a network failure stays silent rather than asking, covered by test_network_failure_does_not_ask_for_known_location.
• The SKILL.md instruction to save WEATHER_LAT/WEATHER_LON in the env stanza matches config_get, which reads env.<key> from the config file before os.environ.
• Ran all 13 tests/morning-briefing*.test.py and tests/briefing-*.test.py files at the head: all pass. scripts/review-checks.sh --diff passes. CI is green.

Non-blocking:

• config_get now runs outside the try in get_weather() (inside weather_coordinates()), and only TypeError/ValueError are caught there. Previously an OSError from reading the config would have been swallowed and returned None; now it would propagate out of main(). load_config likely handles its own I/O errors, so this is low risk, but worth a glance.
• The PR is still a draft and notes the live round trip (owner answers with a city, agent saves coordinates, next briefing uses them) is not yet exercised; that is the remaining check before marking ready.

[liususan091219]

Approve. Reviewed at 07cc339. Ready to merge. The live round trip described below is still owed, but it is not a blocker.

What I checked

• Diff against its merge-base (the branch is current with main). The only call path is main() → get_weather() / weather_location_configured() → synthesize(). synthesize() now takes the new flag as keyword-only with default True, so the existing callers in the briefing tests keep their behavior.
• weather_coordinates() returns None when either value is missing, non-numeric, nan, inf, or outside the lat/lon range. The subtests cover each of these cases, plus 0,0 as valid.
• SKILL.md says to save the coordinates in the env stanza. That matches sutando_config.config_get, which reads env.<key> from the config file before os.environ.
• Ran locally: morning-briefing-weather-location 18/18, morning-briefing-calendar-unavailable 21/21, briefing-all-clear-verified 27/27.
• Control: I put the parent's src/morning-briefing.py back and kept the new tests. They fail: weather-location has 8 failures and 3 errors, and calendar-unavailable has 1 failure. So the tests exercise the behavior, not the source text.
• scripts/review-checks.sh over the diff: PASS. CI is all green (diff coverage 100%), and license/cla is success.
• Sibling sweep: git grep for 37.77, 122.42 and open-meteo outside the tests finds no other default-city weather source.
• About the earlier note that config_get moved outside the try: load_config raises RuntimeError on malformed config, and the old except (URLError, KeyError, ValueError, OSError) did not catch that either. So the move introduces no new failure.

Worst-case disruption (REVIEW.md #5): most installs have no WEATHER_LAT/WEATHER_LON set. Every briefing on those installs will now ask for a city and drop the "Everything looks clean" line, until someone saves coordinates. That is the intended trade, and a revert undoes it.

Not verified: the live round trip. That is a briefing asking the question, the owner replying, the coordinates being saved, and the next briefing showing weather. The PR body says this has not been run, and it remains owed.

Non-blocking nit: test_the_two_renderings_are_distinguishable and test_config_file_only_does_not_print_the_default_location_line still describe the old "default location" annotation.

— Sutando (susans-MacBook-Pro)

[qingyun-wu]

Approving at 07cc339. CI is green at this head (newest run per check name; only the Windows runtime job is skipped, which is expected for this diff).

What I checked:

• No hardcoded fallback is left. The SF coordinates and the timezone AppleScript probe are gone from get_weather(). No other code path in src/ or skills/ falls back to a fixed city. weather_coordinates() returns None for missing, partial, unparsable, NaN/inf and out-of-range values, so all of them take the ask path, and (0, 0) is still valid.
• Store and read-back. The answer is saved, per SKILL.md, as WEATHER_LAT/WEATHER_LON in the env stanza of sutando.config.local.json. config_get reads that stanza before the environment, so the next run picks it up and stops asking.
• Tests. The ask path is covered by synthesize(..., weather_location_known=False) and by main() writing the question with zero network calls. The known path is covered by the unit tests with coordinates set, the zero-coordinate case and the config-file-only diagnostic. A network failure with a known location does not ask.
• Comments and privacy. New comments and docstrings are one line each. Neither the log nor the spoken text includes coordinates. The test coordinates are generic public city values.

Non-blocking notes:

1. It asks once per briefing, not once overall. Nothing records that the question was already asked, so an owner who never replies hears it every morning. There is also no way to say "no weather, please". If that matters, a later change could store an opt-out (for example WEATHER_LAT=none) or a last-asked stamp.
2. Saving the answer depends on the agent. It relies on the SKILL.md instruction reaching whoever handles the owner's reply. A reply arriving as an ordinary task may not have the skill loaded. The PR body already lists the live round trip as unverified, and it is worth checking once after merge.
3. Some test names are stale. test_config_file_only_does_not_print_the_default_location_line and test_configured_is_not_annotated describe the old default-location behavior. Optional rename.