fix(config): persist resolved locations and align deployment guidance

[IceCodeNew]

Summary

• persist exact forward and reverse geocoding results in the state cache, then backfill missing name, latitude, and longitude fields in locations.json
• validate provider-resolved names and coordinates before backfilling and preserve the location file when invalid values are returned
• preserve existing and unknown location fields, while leaving reduced-precision matches for user confirmation
• serialize location configuration reads and in-place updates with bounded shared and exclusive file locks
• treat location-file backfill as best effort: report write or lock failures without interrupting briefings and alerts
• persist resolved fields before sending reduced-precision confirmation alerts, so delivery failures cannot skip exact-location backfills
• rewrite the English and Japanese READMEs from the revised Chinese source while preserving idiomatic localized wording
• make Docker the recommended deployment method and align writable location configuration, state mounts, upgrades, and optional RSS instructions across all three languages
• initialize the deployed container name once per shell, validate it in each operational block, use a non-expiring forecast-date placeholder, separate diagnostic enable and disable steps, and synchronize timezone parsing across deployment examples

Behavior

Exact geocoding results are written to state/geocoding.json before the program attempts to update locations.json. If the writable bind mount is unavailable, locked, or encounters another I/O failure, the program logs a warning and continues the current run; later runs reuse the cached result instead of querying the geocoding provider again.

After all locations resolve, exact results are backfilled before any reduced-precision confirmation alert is sent. A delivery failure therefore cannot prevent resolved fields for other locations from being persisted.

Resolved names must be non-empty strings, and coordinates must be non-boolean numbers that are finite and within valid latitude and longitude ranges before they can be written back. Invalid provider results leave locations.json unchanged and are handled by the same best-effort backfill boundary.\n\nLatitude and longitude are applied as an atomic pair against the current locked file contents. If another writer leaves only one coordinate present between configuration loading and backfill, the existing coordinate is preserved and the resolved pair is not mixed into it.

The writable locations.json bind mount is intentional. Updates happen on the existing inode for compatibility with a single-file Docker bind mount. Readers take a shared lock and backfills hold an exclusive lock through read, merge, write, truncate, flush, and fsync; lock acquisition is limited to five seconds.

Users who do not enable RSS do not need to create or mount an RSS source file. The documented docker exec commands are noninteractive one-shot operations and therefore do not allocate stdin or a TTY with -it.

Native Windows is outside the supported runtime scope. Docker and direct POSIX execution are supported, so the location-file locking implementation uses the Python fcntl standard-library module.

Validation

• prek run --all-files
• uv run --with pytest --with pytest-cov -- pytest --cov --cov-branch --cov-report=xml (885 passed)
• total coverage: 99.85% lines, 99.55% branches
• focused location configuration tests cover cross-process readers and writers, lock retries and timeouts, lock-specific diagnostics, permission failures, non-permission I/O failures, and malformed resolved names and coordinates from JSON cache data
• focused CLI coverage verifies location backfill happens before reduced-precision confirmation alerts
• sh -n on the deployment shell blocks in all three READMEs

Summary by CodeRabbit

• Improvements
  • Geocoding/provider results can now be written back into locations.json to fill only missing fields without overwriting existing values; precision-reduced matches require user confirmation.
  • Monitoring/notification now triggers only within the configured time window.
  • Concurrency-safe handling of locations.json prevents conflicts when multiple processes run.
  • Runtime diagnostics guidance updated, with safe enable/disable for rendered-text output.
• Configuration
  • DeepSeek API base URL is now optional in env.example.
• Documentation
  • README updates cover clearer deployment steps (including state/bind mounts, pull/remove/start), location language/provider rules, Telegram onboarding differences, and RSS setup (not mounted by default).
• Tests
  • Expanded coverage for location backfill behavior and failure/unavailability handling.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR adds locked location-field backfilling during CLI execution, validates persistence and error handling, documents matching and caching rules, and revises multilingual Docker, configuration, publishing, troubleshooting, diagnostics, and environment-variable guidance.

Changes

Location resolution backfill

Layer / File(s)	Summary
Locked location persistence weather_briefing/config.py, docs/design.md, docs/requirements.md	Locations are validated, read, and updated with shared or exclusive locks; precise resolution fills only missing fields, while reduced-precision matches are not automatically persisted.
CLI integration and validation weather_briefing/cli.py, tests/test_config.py, tests/test_cli.py, docs/notes.md	The CLI invokes backfilling after resolution, moves settings and lock-sensitive work off the event loop, logs failures or successful updates, and tests concurrency, preservation, reduced precision, and persistence errors.

Documentation and deployment guidance

Layer / File(s)	Summary
Capabilities, location, and Docker setup README.md, README_ja.md, README_zh-Hans.md	Multilingual guidance updates capabilities, prerequisites, location resolution, provider configuration, bind mounts, permissions, image startup, and container replacement.
Publishing and operations README.md, README_ja.md, README_zh-Hans.md, env.example	Telegram, RSS, task execution, scheduling, logging, diagnostics, container naming, and optional DeepSeek configuration instructions are revised.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant LocationResolver
  participant backfill_location_fields
  participant locations_json
  CLI->>LocationResolver: Resolve configured locations
  LocationResolver-->>CLI: Return resolved locations
  CLI->>backfill_location_fields: Pass configured and resolved locations
  backfill_location_fields->>locations_json: Persist missing precise fields under lock
  locations_json-->>CLI: Return persistence result

Loading

Possibly related PRs

• IceCodeNew/weather-briefing#12: Both update CLI tests around precision-reduced location matching and alert behavior.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly reflects the two main changes: persisting resolved locations and updating deployment guidance.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/improve-readme-deployment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Docs: align multilingual Docker deployment and optional RSS guidance

📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Make Docker the recommended deployment path and standardize host/state mount steps.
• Align English/Japanese docs with the Chinese source, keeping idiomatic localization.
• Clarify Telegram private-chat vs group setup, monitoring windows, and optional RSS mounting.

Diagram

graph TD
  U([User]) --> H["Host data dir"] --> C["Docker container"] --> T["Telegram delivery"]
  H --> E[".env"] --> C
  H --> L["locations.json"] --> C
  H --> S[("state/")] --> C
  H --> R["rss-sources.json (opt)"] --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Publish a docker-compose.yml example

• ➕ Reduces copy/paste errors in long docker run commands
• ➕ Makes optional mounts (RSS) and upgrades clearer via service definition changes
• ➕ Easier to keep identical across languages
• ➖ Adds an additional artifact to maintain and document
• ➖ Some users prefer a single docker run command over Compose

2. Single-source multilingual docs (generate localized READMEs)

• ➕ Prevents drift between English/Japanese/Chinese deployment instructions
• ➕ Enables consistent snippet reuse (mounts, upgrade steps, RSS enablement)
• ➖ Requires tooling and a contributor workflow for localization updates
• ➖ May constrain language-specific/idiomatic phrasing without careful templates

Recommendation: The PR’s approach (manually aligning the three READMEs) is appropriate for a docs-only change and quickly fixes deployment drift. If the Docker instructions keep evolving, consider adding a docker-compose.yml example and/or extracting shared deployment snippets into a single-source workflow to reduce future multilingual divergence.

Files changed (4) +105 / -108

Documentation (3) +104 / -107

README.mdStandardize Docker deployment, optional RSS, and Telegram setup (EN) +46/-47

Standardize Docker deployment, optional RSS, and Telegram setup (EN)

• Rewords the overview and capability bullets for clarity and source-link emphasis. Updates the Docker deployment section to recommend Docker, standardize host directory variables, clarify upgrade behavior, and make RSS mounting explicitly optional. Clarifies Telegram private-chat vs group prerequisites and refines default monitoring window wording.

README.md

README_ja.mdAlign Japanese deployment and configuration guidance with current workflow +31/-32

Align Japanese deployment and configuration guidance with current workflow

• Updates the Japanese README to match the current Docker image workflow (container naming/paths, pull + replace-on-run upgrade flow). Clarifies that locations.json must be valid JSON, RSS is optional and not mounted by default, and adds explicit guidance for Telegram private chat vs group delivery.

README_ja.md

README_zh-Hans.mdRefresh Chinese deployment guidance and clarify optional RSS/Telegram behavior +27/-28

Refresh Chinese deployment guidance and clarify optional RSS/Telegram behavior

• Makes Docker the recommended deployment method and aligns the directory/mount/upgrade instructions with the current image workflow. Clarifies Telegram private-chat vs group requirements and describes RSS as optional with an explicit opt-in mount step.

README_zh-Hans.md

Other (1) +1 / -1

env.exampleComment DeepSeek API base example setting +1/-1

Comment DeepSeek API base example setting

• Changes DEEPSEEK_API_BASE from an empty assignment to a commented example line to signal it is optional and avoid implying it must be set.

env.example

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@README.md`:
- Around line 117-124: After creating rss-sources.json, add the required chgrp
and chmod commands so the mounted file is readable inside the container. Apply
the same post-creation permission instructions in README.md lines 117-124,
README_ja.md lines 117-125, and README_zh-Hans.md lines 117-125.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 87ff0c5d-64db-408a-95cf-86d0357257ff

📥 Commits

Reviewing files that changed from the base of the PR and between 2ad7397 and 2831de3.

📒 Files selected for processing (4)

• README.md
• README_ja.md
• README_zh-Hans.md
• env.example

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 41 rules

1. Partial coordinate backfill ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

backfill_location_fields() writes latitude and longitude independently when merging into
locations.json, so if the current file has only one coordinate present it can persist a mixed
coordinate pair (one from the file, one from the resolved result). This violates the codebase’s
paired-coordinate contract and can lead to incorrect persisted locations.

Code

weather_briefing/config.py[R356-385]

+        if location.latitude is None and location.longitude is None:
+            if not _valid_coordinate(resolved_location.latitude, -90, 90):
+                raise ConfigurationError(f"Resolved latitude for location {location.id} is invalid")
+            if not _valid_coordinate(resolved_location.longitude, -180, 180):
+                raise ConfigurationError(f"Resolved longitude for location {location.id} is invalid")
+            fields["latitude"] = resolved_location.latitude
+            fields["longitude"] = resolved_location.longitude
+        if fields:
+            updates[location.id] = fields
+    if not updates:
+        return False
+
+    try:
+        with path.open("r+", encoding="utf-8") as locations_file:
+            _lock_location_file(
+                path,
+                locations_file.fileno(),
+                fcntl.LOCK_EX,
+                "save resolved location fields",
+            )
+            items = _json_array(path, locations_file.read())
+            changed = False
+            for item in items:
+                location_id = item.get("id")
+                if not isinstance(location_id, str) or location_id not in updates:
+                    continue
+                for field, value in updates[location_id].items():
+                    if item.get(field) is None:
+                        item[field] = value
+                        changed = True

Relevance

⭐⭐⭐ High

Repo contract enforces coordinate pairs; PR #29 states single coordinate invalid and config
validation is strict (PR #52).

PR-#29
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The loader enforces that coordinates must be provided together, but the backfill merge loop writes
each coordinate separately when the field is None, allowing a mixed pair to be persisted if the
file currently has only one coordinate present.

weather_briefing/config.py[276-305]
weather_briefing/config.py[339-399]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`backfill_location_fields()` computes coordinate updates as a pair, but applies them per-field (`if item.get(field) is None`), which can write only one coordinate when the current file already contains the other. This can persist a mixed coordinate pair, even though `_locations()` enforces that coordinates must be provided together.

### Issue Context
This can occur if `locations.json` changes between the initial `Settings.from_env()` load (which produces `configured`) and the later backfill step (e.g., manual edits, non-cooperating writers, or stale snapshots). Even with the exclusive lock during backfill, the merge logic should enforce the “pair” invariant against the *current* file contents.

### Fix Focus Areas
- weather_briefing/config.py[276-305]
- weather_briefing/config.py[339-399]

### Suggested remediation
- When applying updates, treat coordinates as an atomic pair:
 - Only write both `latitude` and `longitude` if **both** are currently `None` in the file item.
 - If exactly one is present, skip coordinate backfill for that location (or raise a `ConfigurationError`, depending on desired behavior).
- Keep name backfill as-is (field-by-field is fine for a single field).
- Add/adjust a unit test to cover the scenario where the on-disk JSON has one coordinate set and the other `null`, ensuring backfill does not write a partial pair.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. docs/design.md duplicates requirements ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The updated docs/design.md restates functional behaviors (e.g., when to backfill locations.json,
best-effort persistence, and reduced-precision handling) that are already specified in
docs/requirements.md, instead of only referencing them. This violates the rule that
docs/design.md should be limited to the current technical contract and should not duplicate
requirements text.

Code

docs/design.md[R60-62]

+精确解析完成后，程序会把地点配置中缺失的名称或经纬度写回 `locations.json`，但不会覆盖已有字段或写入内部地区元数据。写入前按配置加载时的规则验证名称和坐标。文件使用原挂载点原地更新，以兼容单文件 Docker bind mount。读取地点配置时持有共享锁，读改写事务在同一个文件描述符上持有独占锁，避免并发运行读到部分内容或覆盖彼此的更新；获取锁最多等待 5 秒。CLI 在工作线程中执行这些带锁操作，避免锁等待阻塞异步事件循环。回写失败时，CLI 记录警告并继续生成简报。较低精度匹配不会自动写回。
+
+定位结果在回写地点配置前缓存在 `GEOCODING_CACHE_PATH`，默认是 `state/geocoding.json`。缓存保存坐标、行政区和时区，不保存当前简报语言。每次读取缓存后，都以地点文件中的 `language` 覆盖运行时值。因此即使地点配置回写失败，后续运行也可以复用定位结果。

Relevance

⭐⭐⭐ High

Team previously asked to remove duplicated doc text to avoid drift (partially accepted in PR #62);
docs ownership reinforced in PR #81.

PR-#62
PR-#81

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

docs/design.md now explicitly restates the backfill/continuation/reduced-precision behaviors,
while docs/requirements.md already specifies the same behaviors as requirements. The compliance
rule for docs/design.md requires referencing requirements rather than duplicating them.

Rule 2141667: Keep docs/design.md limited to the current technical contract
docs/design.md[58-62]
docs/requirements.md[45-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`docs/design.md` contains requirement-level behavior statements that substantially duplicate `docs/requirements.md`, which the compliance checklist disallows.

## Issue Context
The new design text repeats behaviors like backfilling missing location fields, best-effort continuation on failure, and not persisting reduced-precision matches—these are already present as requirements.

## Fix Focus Areas
- docs/design.md[58-62]
- docs/requirements.md[45-53]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Uncaught TypeError on backfill ✓ Resolved 🐞 Bug ☼ Reliability

Description

backfill_location_fields() calls math.isfinite() on resolved latitude/longitude without first
ensuring they are numeric, so malformed geocoding cache data can raise TypeError and abort the run.
This bypasses the intended best-effort behavior because _save_resolved_location_fields() only
catches ConfigurationError.

Code

weather_briefing/config.py[R350-356]

+        if location.latitude is None and location.longitude is None:
+            if not math.isfinite(resolved_location.latitude) or not -90 <= resolved_location.latitude <= 90:
+                raise ConfigurationError(f"Resolved latitude for location {location.id} is invalid")
+            if not math.isfinite(resolved_location.longitude) or not -180 <= resolved_location.longitude <= 180:
+                raise ConfigurationError(f"Resolved longitude for location {location.id} is invalid")
+            fields["latitude"] = resolved_location.latitude
+            fields["longitude"] = resolved_location.longitude

Relevance

⭐⭐⭐ High

Repo consistently accepts hardening JSON/numeric boundaries to avoid crashes (e.g.,
non-finite/non-numeric rejection in PR #60; config validation in PR #52).

PR-#60
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The backfill code invokes math.isfinite() on resolved coordinates without a numeric type guard,
while CachedLocationResolver constructs ResolvedLocation directly from arbitrary JSON dicts (no
runtime type enforcement). Since the CLI’s best-effort wrapper only catches ConfigurationError, a
TypeError from math.isfinite() would escape and terminate the run.

weather_briefing/config.py[333-356]
weather_briefing/cli.py[262-271]
weather_briefing/geocoding.py[433-451]
weather_briefing/models.py[56-77]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`backfill_location_fields()` uses `math.isfinite(resolved_location.latitude/longitude)` without guarding for non-numeric types. If the geocoding cache JSON is corrupted/hand-edited (e.g., latitude is a string), `ResolvedLocation(**cached)` will still succeed (dataclasses don’t enforce runtime types), and `math.isfinite()` will raise `TypeError`. This exception is *not* caught by `_save_resolved_location_fields()` (which only catches `ConfigurationError`), so the run can terminate even though location-file backfill is supposed to be best-effort.

## Issue Context
Cached geocoding records are loaded from JSON and unpacked into `ResolvedLocation` without type validation. Backfill runs before alerts and before location processing, so a crash here can prevent a whole briefing run.

## Fix Focus Areas
- weather_briefing/config.py[333-393]
- weather_briefing/cli.py[262-271]
- weather_briefing/geocoding.py[433-451]

## Suggested remediation
- In `backfill_location_fields()`, validate/cast coordinates safely before calling `math.isfinite()`:
 - e.g., `lat = float(resolved_location.latitude)` / `lon = float(resolved_location.longitude)` in a `try` block catching `(TypeError, ValueError)` and raising `ConfigurationError("Resolved latitude/longitude ... is invalid")`.
 - This ensures malformed cached/provider values are handled via `ConfigurationError`, which the CLI already treats as best-effort.
- Optionally (stronger): also validate cached record types at cache read time in `CachedLocationResolver` (reject cached dicts whose `latitude/longitude` are not castable to floats), so malformed cache data can’t reach other parts of the system either.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (8)

4. Unvalidated name persisted ✓ Resolved 🐞 Bug ☼ Reliability

Description

backfill_location_fields() can write ResolvedLocation.name into locations.json without validating it
is non-empty, so a malformed/corrupted cached ResolvedLocation (loaded directly from JSON) can
permanently poison locations.json and make the next Settings.from_env() fail. This breaks future
runs until the operator manually repairs the configuration file.

Code

weather_briefing/config.py[R342-345]

+        fields: dict[str, str | float] = {}
+        if location.name is None:
+            fields["name"] = resolved_location.name
+        if location.latitude is None and location.longitude is None:

Relevance

⭐⭐⭐ High

Repo repeatedly adds strict non-empty string validation to prevent poisoned state/config (e.g.,
warning IDs) in PR33/52.

PR-#33
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The backfill path writes the resolved name directly, while the configuration loader requires any
present name to be a non-empty string; ResolvedLocation itself does not validate name content and
cached JSON records are rehydrated directly into ResolvedLocation, so a bad cached value can be
persisted and later rejected on load.

weather_briefing/config.py[330-352]
weather_briefing/config.py[195-207]
weather_briefing/models.py[56-76]
weather_briefing/geocoding.py[433-451]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`backfill_location_fields()` writes `resolved_location.name` into `locations.json` whenever the configured location has no name, but it does not validate that the resolved name is a non-empty/whitespace-free string. Because `ResolvedLocation` does not validate `name`, and cached records are rehydrated from JSON, a malformed cache entry (or other upstream corruption) can cause an empty name to be persisted into `locations.json`, which then fails validation on the next config load.

### Issue Context
- The config loader treats `name` as an optional field, but if present it must be a **non-empty string**.
- Backfill should therefore enforce the same invariant before persisting `name`.

### Fix Focus Areas
- weather_briefing/config.py[330-354]
- weather_briefing/config.py[195-207]
- weather_briefing/geocoding.py[433-451]
- weather_briefing/models.py[56-76]

### Suggested fix
- Before setting `fields["name"]`, validate `resolved_location.name` similarly to `_required_string_field` semantics (e.g., `isinstance(name, str)` and `name.strip()` is truthy).
- If invalid, raise a `ConfigurationError` like `Resolved name for location {id} is invalid` (so the CLI logs a warning and continues without writing), or skip the `name` update for that location.
- Add a unit test that passes `ResolvedLocation(name="")` (or whitespace) into `backfill_location_fields()` and asserts it raises `ConfigurationError` and leaves the file unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Writable locations mount ✗ Dismissed 🐞 Bug ⛨ Security

Description

The Docker examples bind-mount locations.json without readonly, giving the container write
access to the host’s configuration file even though the configuration loader only reads JSON from
it. This unnecessarily increases the blast radius (config tampering/drift) if the container is
compromised or misbehaves.

Code

README.md[R69-72]

  --mount \
-  "type=bind,src=${ROOT_DIR}/locations.json,dst=/home/nonroot/app/locations.json,readonly" \
+  "type=bind,src=${ROOT_DIR}/locations.json,dst=${CONTAINER_ROOT_DIR}/locations.json" \
  --mount \
-  "type=bind,src=${ROOT_DIR}/state,dst=/home/nonroot/app/state" \
-  "${IMAGE}" daemon
+  "type=bind,src=${ROOT_DIR}/state,dst=${CONTAINER_ROOT_DIR}/state" \

Relevance

⭐⭐⭐ High

Earlier README Docker examples mounted locations.json with readonly; repo accepts similar
config-hardening doc fixes (PRs #40/#90).

PR-#40
PR-#90

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The READMEs show locations.json being mounted without readonly, while the config loader reads
JSON content from the path using read_text, indicating write access is not required for config
loading.

README.md[64-74]
README_ja.md[63-74]
README_zh-Hans.md[63-74]
weather_briefing/config.py[119-128]
weather_briefing/config.py[235-239]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Docker deployment examples mount `locations.json` as a writable bind mount. This grants the container the ability to modify host configuration, which is unnecessary for normal operation and increases the blast radius of container compromise or application bugs.

## Issue Context
In `weather_briefing/config.py`, JSON configuration is loaded via `Path.read_text(...)` and parsed; no write is required to read configuration.

## Fix
Update the `docker run` examples in all three READMEs to add `,readonly` back to the `locations.json` bind mount (consistent with the RSS mount already being read-only).

## Fix Focus Areas
- README.md[64-74]
- README_ja.md[63-74]
- README_zh-Hans.md[63-74]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Lock tests not cross-process ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new location-file lock contention tests use threads within a single Python process to simulate
competing writers/readers, but the documented locking contract is for coordinating separate
processes; flock semantics across multiple opens in the same process are platform-dependent and
can make these tests flaky or non-representative on macOS. This risks CI instability and may fail to
validate the actual multi-process safety the feature relies on.

Code

tests/test_config.py[R629-636]

+    with ThreadPoolExecutor(max_workers=1) as executor:
+        with location_file.open("r+", encoding="utf-8") as locked_file:
+            fcntl.flock(locked_file.fileno(), fcntl.LOCK_EX)
+            future = executor.submit(backfill_after_ready)
+            assert ready.wait(timeout=10)
+            with pytest.raises(TimeoutError):
+                future.result(timeout=1)
+

Relevance

⭐⭐ Medium

No prior reviews about thread-vs-process flock tests; team does accept “prevent test flakiness”
fixes generally (PR #11).

PR-#11
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The tests explicitly create lock contention via threads, while the design docs state the locking
exists to coordinate concurrent runs/processes and CI includes macOS, where same-process locking
behavior can differ and undermine these assertions.

tests/test_config.py[618-682]
docs/design.md[58-62]
docs/notes.md[101-106]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The lock-contention tests use `ThreadPoolExecutor` to run the contending operation in another thread while the main thread holds an exclusive `flock`. This does not guarantee real contention on all POSIX platforms because both threads are in the same process, and the locking design/contract is explicitly about coordinating *different processes*.

### Issue Context
The docs describe `fcntl` locks as coordinating different processes and note CI covers macOS; tests should therefore validate cross-process contention deterministically.

### Fix Focus Areas
- tests/test_config.py[618-682]
- docs/notes.md[101-106]

### Proposed fix
- Replace the thread-based lock holder (or contender) with a true separate process (e.g., `multiprocessing.Process` or a small `subprocess` Python snippet) that opens the file and holds `LOCK_EX` until signaled.
- Keep the existing `future.result(timeout=...)` pattern if desired, but ensure the blocking is caused by a lock held by *another process*, not another thread.
- Apply the same adjustment to both:
 - `test_backfill_location_fields_locks_read_modify_write_transaction`
 - `test_locations_waits_for_locked_writer_before_reading`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Unvalidated backfill coordinates ✓ Resolved 🐞 Bug ☼ Reliability

Description

backfill_location_fields() writes resolved latitude/longitude into locations.json without
validating finiteness/range, so a corrupted cache/provider response can persist invalid coordinates
and make later Settings.from_env() fail when _locations() re-validates the file. This can
convert a one-off bad resolution into a persistent configuration break until the user manually edits
the file.

Code

weather_briefing/config.py[R344-346]

+        if location.latitude is None and location.longitude is None:
+            fields["latitude"] = resolved_location.latitude
+            fields["longitude"] = resolved_location.longitude

Relevance

⭐⭐ Medium

Team favors strict numeric/config validation (PRs #60, #46, #52), but no prior finding about
validating backfilled coordinates.

PR-#60
PR-#46
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The backfill path persists resolved coordinates without checks, while the config loader enforces
strict bounds; geocoding providers/cached records create float coordinates without performing those
bounds checks, so a malformed resolved value can be written and then rejected on the next load.

weather_briefing/config.py[329-347]
weather_briefing/config.py[286-300]
weather_briefing/geocoding.py[156-178]
weather_briefing/geocoding.py[500-516]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`backfill_location_fields()` persists `ResolvedLocation.latitude/longitude` directly into `locations.json` when the configured location has no coordinates. The location loader `_locations()` enforces latitude/longitude bounds, but the backfill path does not validate the resolved values before writing.

If the resolved values are non-finite (NaN/Inf) or out of bounds (e.g., due to cache corruption or an unexpected provider response), the backfill can write invalid coordinates to `locations.json`. Subsequent runs will then fail during configuration loading, turning a transient resolution issue into a persistent startup failure.

## Issue Context
- Backfill writes coordinates without validation.
- `_locations()` later validates coordinate ranges strictly.
- Geocoding providers parse floats from responses, and cache reads reconstruct `ResolvedLocation` objects without applying range checks.

## Fix Focus Areas
- weather_briefing/config.py[329-383]
- weather_briefing/config.py[286-301]
- weather_briefing/geocoding.py[113-179]

## Suggested fix
1. In `backfill_location_fields()`, before adding `latitude/longitude` into `fields`, validate that:
  - both values are finite (`math.isfinite`)
  - latitude is within [-90, 90]
  - longitude is within [-180, 180]
2. If validation fails:
  - do not write those values to `locations.json`
  - either (a) skip just that location’s coordinate update and continue updating other locations, or (b) raise a `ConfigurationError` that includes the location id and the invalid values (CLI will already treat this as best-effort and log a warning).
3. Add a small unit test for out-of-range or non-finite resolved coordinates to ensure backfill never persists them.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. Blocking sleep in async ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new location-file lock retry loop uses time.sleep(), but it is invoked from async run()/daemon
paths (during Settings.from_env() location loading and during location backfill), so lock contention
can freeze the entire asyncio event loop for up to the lock timeout. This can delay scheduled jobs
and stall in-flight async I/O while the process waits to acquire the advisory lock.

Code

weather_briefing/config.py[R143-154]

+def _lock_location_file(path: Path, file_descriptor: int, operation: int, timeout_action: str) -> None:
+    deadline = time.monotonic() + _LOCATION_FILE_LOCK_TIMEOUT_SECONDS
+    while True:
+        try:
+            fcntl.flock(file_descriptor, operation | fcntl.LOCK_NB)
+            return
+        except BlockingIOError as exc:
+            remaining = deadline - time.monotonic()
+            if remaining <= 0:
+                raise ConfigurationError(f"{path} is locked; cannot {timeout_action}") from exc
+            time.sleep(min(_LOCATION_FILE_LOCK_RETRY_SECONDS, remaining))
+

Relevance

⭐⭐ Medium

No historical evidence on time.sleep blocking asyncio; repo often accepts reliability refactors but
none matching this pattern.

PR-#86
PR-#84
PR-#88

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

_lock_location_file() performs lock retries with time.sleep(), and both the locked read path
(_locked_location_json_file() used by _locations()) and the exclusive write path
(backfill_location_fields()) call into it. cli.run() is an async coroutine that calls
Settings.from_env() and then synchronously calls _save_resolved_location_fields(), so under lock
contention the blocking sleep runs on the asyncio event loop thread; daemon() schedules run()
via AsyncIOScheduler, so the daemon can be stalled during retries.

weather_briefing/config.py[143-154]
weather_briefing/config.py[156-165]
weather_briefing/cli.py[276-330]
weather_briefing/cli.py[715-742]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_lock_location_file()` retries `fcntl.flock(..., LOCK_NB)` using `time.sleep()`. Because both location reads (`_locations()` via `_locked_location_json_file()`) and location backfills (`backfill_location_fields()`) are called from the async `run()` coroutine (and therefore from the daemon’s asyncio event loop), this blocking sleep can stall the whole loop during lock contention.

### Issue Context
- `run()` is async and runs under `AsyncIOScheduler` in `daemon()`.
- `Settings.from_env()` triggers `_locations()` which now takes a shared flock.
- After resolving locations, `run()` calls `_save_resolved_location_fields()` which calls `backfill_location_fields()` (exclusive flock) synchronously.

### Fix Focus Areas
- weather_briefing/config.py[143-154]
- weather_briefing/cli.py[262-330]
- weather_briefing/cli.py[715-742]

### Suggested implementation direction
- Offload the blocking file-lock/read-modify-write work to a worker thread from `run()` (and any other async entrypoints) using `await asyncio.to_thread(...)` (or `loop.run_in_executor`).
 - Example: `settings = await asyncio.to_thread(Settings.from_env)` inside `run()`.
 - Example: `await asyncio.to_thread(backfill_location_fields, settings.locations_path, settings.locations, locations)` (or wrap `_save_resolved_location_fields` similarly).
- Keep the underlying locking logic synchronous (fcntl/time.sleep) but ensure it never runs on the event loop thread.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Locations read without lock ✓ Resolved 🐞 Bug ☼ Reliability

Description

backfill_location_fields() rewrites locations.json in-place under an advisory exclusive flock, but
_locations() loads the same file via Path.read_text() without acquiring any lock. A concurrent run
can read truncated/partial JSON and fail Settings.from_env() with a ConfigurationError.

Code

weather_briefing/config.py[R340-361]

+    try:
+        with path.open("r+", encoding="utf-8") as locations_file:
+            _lock_location_file(path, locations_file.fileno())
+            items = _json_array(path, locations_file.read())
+            changed = False
+            for item in items:
+                location_id = item.get("id")
+                if not isinstance(location_id, str) or location_id not in updates:
+                    continue
+                for field, value in updates[location_id].items():
+                    if item.get(field) is None:
+                        item[field] = value
+                        changed = True
+            if not changed:
+                return False
+
+            payload = json.dumps(items, ensure_ascii=False, indent=2) + "\n"
+            locations_file.seek(0)
+            locations_file.write(payload)
+            locations_file.truncate()
+            locations_file.flush()
+            os.fsync(locations_file.fileno())

Relevance

⭐⭐ Medium

No historical evidence on flocking config-file reads; team does accept config reliability hardening
in prior PRs.

PR-#46
PR-#52

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The writer path truncates and rewrites locations.json under an exclusive flock, but the reader path
uses read_text() without flock, so it can observe intermediate invalid JSON while the file is being
rewritten.

weather_briefing/config.py[133-140]
weather_briefing/config.py[260-263]
weather_briefing/config.py[340-366]
weather_briefing/config.py[448-481]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`backfill_location_fields()` updates `locations.json` by truncating and rewriting the file contents while holding an exclusive `flock`. However, the configuration reader path (`Settings.from_env()` -> `_locations()` -> `_json_file()` -> `Path.read_text()`) does not take a shared lock. Because `flock` is advisory, readers that do not also lock can observe an empty/partial JSON document during the rewrite window and fail with `ConfigurationError`.

## Issue Context
This PR intentionally rewrites `locations.json` in-place (not via rename) to support single-file Docker bind mounts; that makes consistent locking on both the read and write paths especially important.

## Fix Focus Areas
- weather_briefing/config.py[133-155]
- weather_briefing/config.py[260-314]
- weather_briefing/config.py[317-366]

## What to change
1. Introduce a shared-lock read path for `locations.json` (e.g., open the file and acquire `fcntl.LOCK_SH` with the same timeout policy), then parse via `_json_array()`.
2. Use that shared-lock reader in `_locations()` (and any other read path that can overlap with `backfill_location_fields()` writes).
3. Add a regression test that simulates a concurrent writer holding the exclusive lock and verifies `_locations()` either (a) waits up to the timeout and succeeds after release, or (b) times out with a clear ConfigurationError (consistent with your chosen policy).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Unbounded flock stalls runs ✓ Resolved 🐞 Bug ☼ Reliability

Description

backfill_location_fields() acquires a blocking exclusive flock() on locations.json with no timeout,
and cli.run() calls it before processing any locations, so a long-held lock can indefinitely delay
an entire briefing run. This can prevent scheduled briefings/alerts from being processed until the
competing process releases the lock.

Code

weather_briefing/config.py[R324-327]

+    try:
+        with path.open("r+", encoding="utf-8") as locations_file:
+            fcntl.flock(locations_file.fileno(), fcntl.LOCK_EX)
+            items = _json_array(path, locations_file.read())

Relevance

⭐⭐ Medium

No prior reviews on flock timeouts; team accepts other anti-hang bounds (RSS retry bounding PR43,
context size cap PR58).

PR-#43
PR-#58

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation takes a blocking exclusive lock and the CLI calls it on the hot path; the test
suite explicitly demonstrates that the backfill call blocks while another process holds the lock.

weather_briefing/config.py[324-327]
weather_briefing/cli.py[305-317]
tests/test_config.py[583-610]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`backfill_location_fields()` uses `fcntl.flock(..., LOCK_EX)` in blocking mode with no timeout. Because `cli.run()` invokes this synchronously before iterating locations, any other live process that holds the lock for an extended period will stall the entire run.

### Issue Context
The lock is important to serialize the read-modify-write transaction on a single bind-mounted file, so the fix should preserve mutual exclusion while preventing unbounded waits.

### Fix Focus Areas
- weather_briefing/config.py[324-350]
- weather_briefing/cli.py[312-317]

### Implementation notes
- Switch to non-blocking acquisition (`LOCK_EX | LOCK_NB`) and implement a bounded retry loop (e.g., small sleep + max elapsed time).
- Decide a clear failure policy after timeout:
 - either raise `ConfigurationError` with a specific message like “locations.json is locked; cannot persist resolved fields”,
 - or log a warning and skip backfill (continuing the run) if persistence is best-effort.
- Add/adjust tests to validate the bounded behavior (no indefinite blocking).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. POSIX-only fcntl import 🐞 Bug ☼ Reliability

Description

weather_briefing.config now unconditionally imports fcntl, which is unavailable on native
Windows Python, causing an import-time crash and making the package unusable there. The test suite
also imports fcntl, preventing test collection on Windows.

Code

weather_briefing/config.py[5]

+import fcntl

Relevance

⭐⭐ Medium

No prior Windows/fcntl guard precedent; repo focus appears container/Linux-centric, so unsure
they’ll change.

PR-#8
PR-#83

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The module-level import fcntl makes import fail on platforms without fcntl, and the same
dependency is used inside backfill_location_fields; tests also import fcntl at module import
time.

weather_briefing/config.py[1-24]
weather_briefing/config.py[301-348]
tests/test_config.py[1-13]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`weather_briefing/config.py` imports `fcntl` at module import time and uses `fcntl.flock()` for locking. On native Windows, `fcntl` does not exist, so importing `weather_briefing.config` fails immediately.

## Issue Context
The new `backfill_location_fields()` feature needs an exclusive lock to avoid concurrent writers, but the implementation should not make the entire package un-importable on non-POSIX platforms.

## Fix Focus Areas
- weather_briefing/config.py[1-6]
- weather_briefing/config.py[301-348]
- tests/test_config.py[1-5]

## Suggested change
- Introduce a small locking abstraction, e.g. `_lock_exclusive(file: IO[str]) -> context manager`.
 - On POSIX: use `fcntl.flock(..., LOCK_EX)`.
 - On Windows: either implement an equivalent via `msvcrt.locking` or (if Windows is intentionally unsupported) raise a clear, explicit error at runtime (not import time) and skip/guard the Windows-incompatible tests.
- At minimum, move the `fcntl` import inside the POSIX-only code path so `weather_briefing.config` can be imported everywhere.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Lock errors misdiagnosed ✓ Resolved 🐞 Bug ◔ Observability

Description

_locked_location_json_file() maps any OSError during open/lock/read to "must contain readable JSON",
so non-content failures (e.g., fcntl.flock() failing due to filesystem lock support) can be
misreported as JSON corruption. This slows debugging because operators may focus on JSON formatting
instead of addressing locking/filesystem issues.

Code

weather_briefing/config.py[R160-166]

+    try:
+        with path.open(encoding="utf-8") as locations_file:
+            _lock_location_file(path, locations_file.fileno(), fcntl.LOCK_SH, "read location configuration")
+            content = locations_file.read()
+    except OSError as exc:
+        raise ConfigurationError(f"{path} must contain readable JSON") from exc
+    return _json_array(path, content)

Relevance

⭐⭐ Medium

Team values actionable diagnostics (PR84), but config tests currently expect generic “readable JSON”
message for OS errors.

PR-#84
PR-#46

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new locked read path wraps all OS-level failures as a JSON readability issue; the existing test
asserts this generic message for open failures, demonstrating the behavior and its diagnostic
ambiguity.

weather_briefing/config.py[157-166]
weather_briefing/config.py[144-155]
tests/test_config.py[712-723]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`_locked_location_json_file()` wraps all `OSError` exceptions from `path.open(...)`, `_lock_location_file(...)`, and `locations_file.read()` into `ConfigurationError(f"{path} must contain readable JSON")`. This conflates lock/IO failures with actual JSON parse failures, and can mislead operators when the real cause is a lock-related `OSError` (non-`BlockingIOError`) or a read I/O error.

### Issue Context
Normal lock contention is already handled via `_lock_location_file()` raising a specific "is locked; cannot ..." error after timeout. The remaining gap is other `OSError` paths which are currently attributed to JSON readability.

### Fix Focus Areas
- weather_briefing/config.py[144-166]
- tests/test_config.py[712-723]

### Suggested fix
- Split exception handling in `_locked_location_json_file()`:
 - Catch `OSError` from `open()` / `read()` and report as read errors.
 - Catch `OSError` from `fcntl.flock()` (or from `_lock_location_file` if you decide to let it propagate) and report as lock acquisition errors.
 - Keep JSON parsing errors (`json.JSONDecodeError`) mapped to the existing "must contain readable JSON" message.
- Optionally add/adjust a test that simulates `fcntl.flock` raising a generic `OSError` and asserts the resulting message references locking rather than JSON readability.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Misleading backfill error ✓ Resolved 🐞 Bug ◔ Observability

Description

backfill_location_fields maps any OSError (lock failures, fsync failures, disk-full, etc.) to a
“must be writable” configuration error, which can misdirect operators during debugging. Only
permission-related failures actually imply non-writability.

Code

weather_briefing/config.py[R324-348]

+    try:
+        with path.open("r+", encoding="utf-8") as locations_file:
+            fcntl.flock(locations_file.fileno(), fcntl.LOCK_EX)
+            items = _json_array(path, locations_file.read())
+            changed = False
+            for item in items:
+                location_id = item.get("id")
+                if not isinstance(location_id, str) or location_id not in updates:
+                    continue
+                for field, value in updates[location_id].items():
+                    if item.get(field) is None:
+                        item[field] = value
+                        changed = True
+            if not changed:
+                return False
+
+            payload = json.dumps(items, ensure_ascii=False, indent=2) + "\n"
+            locations_file.seek(0)
+            locations_file.write(payload)
+            locations_file.truncate()
+            locations_file.flush()
+            os.fsync(locations_file.fileno())
+    except OSError as exc:
+        raise ConfigurationError(f"{path} must be writable to save resolved location fields") from exc
+    return True

Relevance

⭐⭐ Medium

Some history of improving error classification, but no direct precedent for narrowing OSError
mapping in config.

PR-#84
PR-#88

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The try block includes locking and fsync, but the exception handler treats all OSError as a
write-permission problem, losing important diagnostic context.

weather_briefing/config.py[324-348]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`backfill_location_fields()` wraps open+lock+read+write+fsync in a single `try` and converts all `OSError` to `"{path} must be writable..."`. This message is inaccurate for non-permission failures (e.g., unsupported `flock`, I/O errors, ENOSPC).

## Issue Context
When this fails in production, the current error will push operators toward permission debugging even if the actual cause is unrelated.

## Fix Focus Areas
- weather_briefing/config.py[324-348]

## Suggested change
- Catch `PermissionError` (or `OSError` with `errno.EACCES`/`EPERM`) and keep the current “must be writable” message.
- For other `OSError`, raise `ConfigurationError(f"Failed to persist resolved location fields to {path}: {exc}")` (or similar), preserving the true failure reason in the message/logs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

14. Hard-coded exec container name ✓ Resolved 🐞 Bug ≡ Correctness

Description

The READMEs define CONTAINER_NAME for docker run --name, but the one-off/troubleshooting
commands still use docker exec weather-briefing. If CONTAINER_NAME is changed (or the container
is deployed under a different name), these docker exec commands will fail by targeting a
non-existent container.

Code

↗ README.md

Relevance

⭐⭐ Medium

No direct historical evidence about parameterizing docker exec with CONTAINER_NAME; similar README
consistency fixes often accepted.

PR-#40
PR-#90

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Each README defines CONTAINER_NAME for container creation, but later uses a literal container name
in docker exec, which becomes incorrect if the container name is changed from the documented
variable.

README.md[40-46]
README.md[134-142]
README_ja.md[40-46]
README_ja.md[135-142]
README_zh-Hans.md[40-46]
README_zh-Hans.md[132-141]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The docs introduce `CONTAINER_NAME` for the `docker run` command, but later `docker exec` examples hard-code `weather-briefing`. This makes the troubleshooting instructions inconsistent and they fail when the container name differs from the hard-coded value.

## Issue Context
This inconsistency exists in English, Japanese, and Simplified Chinese READMEs.

## Fix
Either:
1) Update `docker exec` examples to use the configured name (e.g., `docker exec "${CONTAINER_NAME}" ...`) and ensure `CONTAINER_NAME` is defined in that section as well, or
2) Remove `CONTAINER_NAME` variableization and keep the container name consistently documented as `weather-briefing`.

## Fix Focus Areas
- README.md[40-46]
- README.md[134-160]
- README_ja.md[40-46]
- README_ja.md[132-160]
- README_zh-Hans.md[40-46]
- README_zh-Hans.md[132-159]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (2)

15. Requirements specify persistent cache ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The updated requirements include implementation-level directives (e.g., persisting geocoding results
in a cache and writing back to configuration) instead of focusing on user needs and externally
observable behavior. This makes docs/requirements.md less portable as a true requirements document
and risks conflating design choices with product requirements.

Code

docs/requirements.md[R47-53]

+- 只有地点名称时，程序应找到坐标，并把精确匹配的坐标补充到地点配置中供以后运行复用。
+- 只有经纬度坐标时，程序应找到便于阅读的地点名称和行政区信息，并把缺失的地点名称补充到地点配置中。
+- 定位结果应保存在持久化缓存中；地点配置回写失败时，当前简报仍应继续运行，后续运行也不应重复查询相同地点。
- 地点名称和经纬度坐标都有时，不应再请求定位服务。
- 名称过于具体而无法识别时，可以逐步降低精度。
- 首次使用较低精度匹配时，要把匹配结果发给用户确认。
+- 较低精度匹配在用户确认前不能自动写入地点配置。

Relevance

⭐ Low

Team keeps implementation specifics in requirements; prior “remove detailed behavior from
requirements” only partially accepted (PR62).

PR-#62
PR-#81

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2225178 requires requirements to avoid implementation details and focus on user
needs/observable behavior. The changed requirements lines prescribe internal persistence/backfill
mechanisms rather than stating outcomes (e.g., avoiding repeat geocoding calls across runs).

Rule 2225178: Requirements document must avoid implementation details and focus on user needs and observable behavior
docs/requirements.md[47-53]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The requirements text describes internal implementation mechanisms (persistent cache + config backfill) rather than stating the required observable outcomes.

## Issue Context
Per compliance, `docs/requirements.md` should express user needs and externally observable behavior, avoiding internal storage/implementation prescriptions.

## Fix Focus Areas
- docs/requirements.md[47-53]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

16. Image tied to container name ✓ Resolved 🐞 Bug ≡ Correctness

Description

The README Docker example derives WEATHER_BRIEFING_IMAGE from CONTAINER_NAME, so changing the
container name also changes the image repository and can make docker pull/docker run fail. This
breaks the documented deployment path for common “rename container” customizations.

Code

README.md[R41-60]

[Comment truncated to fit github's 65,536-char limit.]

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@weather_briefing/config.py`:
- Around line 315-335: Serialize the full read-modify-write transaction in the
relevant location-update function: acquire the file lock before calling
_json_file(path), reload the current items while holding it, and retain the lock
through write, truncate, flush, and fsync. Ensure concurrent daemon and manual
runs cannot overwrite each other with stale snapshots or interleave writes.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 306ad96b-6baa-4ad7-8188-0af93f06cc68

📥 Commits

Reviewing files that changed from the base of the PR and between 2831de3 and b864d20.

📒 Files selected for processing (9)

• README.md
• README_ja.md
• README_zh-Hans.md
• docs/design.md
• docs/requirements.md
• tests/test_cli.py
• tests/test_config.py
• weather_briefing/cli.py
• weather_briefing/config.py

🚧 Files skipped from review as they are similar to previous changes (2)

• README_ja.md
• README.md

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit b486050

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit cf96c00

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 35c0af1

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 8385107

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit fc302db

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

README.md (1)

34-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Clarify the native Windows support boundary.

The statement that the project can run “in other ways” does not communicate the documented scope that native Windows is unsupported. Narrow this wording or add an explicit note so users do not interpret native Windows as a supported runtime.

Suggested wording

-Docker is the recommended deployment method. The examples below use a fixed-version image from Docker Hub. You can also run the project in other ways, as long as it can run persistently and preserve the configuration and state described above.
+Docker is the recommended deployment method. The examples below use a fixed-version image from Docker Hub. Other supported deployment methods must run persistently and preserve the configuration and state described above. Native Windows is not a supported runtime.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` around lines 34 - 36, Update the “Using the published image”
section to explicitly state that native Windows is unsupported, while retaining
Docker and other supported persistent deployment options. Narrow the “other
ways” wording so it cannot imply native Windows is an available runtime.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/design.md`:
- Line 60: Update the documentation describing backfill_location_fields() so it
states that latitude and longitude are written only when both existing
coordinate values are null, rather than independently filling missing fields.
Keep the documented name backfill behavior and preservation of other fields
unchanged.

---

Outside diff comments:
In `@README.md`:
- Around line 34-36: Update the “Using the published image” section to
explicitly state that native Windows is unsupported, while retaining Docker and
other supported persistent deployment options. Narrow the “other ways” wording
so it cannot imply native Windows is an available runtime.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fa7eeafa-1149-41fc-875a-052e9336876c

📥 Commits

Reviewing files that changed from the base of the PR and between b35980a and fc302db.

📒 Files selected for processing (10)

• README.md
• README_ja.md
• README_zh-Hans.md
• docs/design.md
• docs/notes.md
• docs/requirements.md
• tests/test_cli.py
• tests/test_config.py
• weather_briefing/cli.py
• weather_briefing/config.py

🚧 Files skipped from review as they are similar to previous changes (7)

• weather_briefing/cli.py
• docs/requirements.md
• docs/notes.md
• weather_briefing/config.py
• tests/test_cli.py
• README_zh-Hans.md
• README_ja.md

[IceCodeNew]

/agentic_review

[IceCodeNew]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 87fcd10

[IceCodeNew]

Qodo item 11 (POSIX-only fcntl import) is intentionally rejected. Native Windows is outside this project's supported runtime contract; supported targets are Linux containers and direct POSIX execution. The three READMEs and docs/notes.md explicitly document this boundary. Deferring the import would not provide supported cross-platform locking semantics and would misleadingly imply native Windows portability.

[IceCodeNew]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7833b1d

[IceCodeNew]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.