Location Tagging Agent Follow-up

[JSv4]

Code Review — Location Tagger default agent + geocoding (#1822)

Overview

This PR implements a well-structured feature: a built-in Location Tagger agent that auto-creates geocoded OC_COUNTRY/OC_STATE/OC_CITY annotations via the existing corpus-actions framework. The core design — extracting build_geocoded_annotation_data as a shared service function — is the right call and follows the DRY principle described in CLAUDE.md.

---

Strengths

• Clean DRY factoring: build_geocoded_annotation_data and LABEL_TEXT_TO_GEOCODE_LABEL_TYPE in geographic_service.py are a genuine single source of truth; both the GraphQL mutation and the agent tool now emit the identical Annotation.data shape.
• Performance: Geocoding is resolved before the DB transaction and once per item (not per occurrence). The comment explaining the rationale (avoids holding row locks across disk I/O) is exactly the kind of non-obvious WHY that earns a comment.
• Blank-span guard: The if not exact_str.strip(): continue fix for the latent infinite loop on doc_text.find("") is a good catch. The parallel guard in _create_geographic_annotation keeps the two surfaces consistent.
• Backward compatibility: Non-geographic labels are untouched; existing callers get annotation_data=None and no change in behaviour.
• Test coverage: The test suite is thorough — registration, idempotency, country/state/city geocoding, hint disambiguation, miss sentinel (geocoded=False), non-geographic backward-compat, and the resolve-once-per-item optimisation.

---

Issues

Medium — test assertion only checks the last loop iteration

File: opencontractserver/tests/test_location_tagger_agent.py, lines 1000–1006

The ann.annotation_type assertion is outside the for-loop and only tests the last element (queryset iteration order is non-deterministic without order_by). Move it inside the loop, or add self.assertTrue(all(a.annotation_type == SPAN_LABEL for a in anns)) after the loop.

for ann in anns:
    self.assertIsNotNone(ann.data)
    self.assertTrue(ann.data["geocoded"])
    self.assertEqual(ann.data["admin_codes"]["iso_alpha2"], "FR")
# Resolver invoked exactly once for the single item, not per occurrence.
self.assertEqual(spy.call_count, 1)
self.assertEqual(ann.annotation_type, SPAN_LABEL)   # <-- only the last `ann` from the loop

---

Minor — hints dict values are not validated as strings

File: opencontractserver/llms/tools/core_tools/annotations.py, around line 670

The isinstance(raw_hints, dict) guard catches a non-dict hints value, but LLM output can still produce integer or None values inside the dict. The type annotation says dict[str, str] but that is unenforced at runtime. Since country_hint and state_hint are passed straight through to resolve_place, a non-string value there could produce a confusing error at the geocoder level rather than at the tool boundary. Consider coercing hint values with str() before passing to resolve_place.

---

Minor — build_geocoded_annotation_data docstring style

File: opencontractserver/annotations/services/geographic_service.py, ~line 558

The country_hint / state_hint: combined argument entry is not valid Google or Sphinx style — each parameter needs its own Args: line for tooling (autodoc, IDEs) to parse correctly.

---

Minor — migration skip on no-superuser has no automated recovery path

File: opencontractserver/agents/migrations/0015_create_location_tagger_agent.py, lines 436–449

The skip + warning when no superuser exists is better than the previous silent skip, but in containerised CI workflows (ephemeral DB, migrations run before fixtures) this agent will never be created. Worth documenting in docs/agents/location_tagger.md alongside the existing prompt-snapshot note — e.g. "If the agent is absent after migration (no superuser existed at migration time), create it via the Django admin or re-run the data migration after seeding a superuser."

---

Note — test_city_without_hints_resolves_to_france ties to geocoder dataset ordering

The inline comment already flags this. Seconding the suggestion to relax the assertion to assertTrue(ann.data["geocoded"]) only, leaving the population-ranking tie-break to the geocoding service's own test suite. Asserting iso_alpha2 == "FR" couples this unit test to geocoder internals that could legitimately change.

---

Nits

• LABEL_TEXT_TO_GEOCODE_LABEL_TYPE value type — typed as dict[str, str] but values are exclusively "country"/"state"/"city". dict[str, Literal["country", "state", "city"]] would make the contract self-documenting and enable exhaustiveness checking at call sites.
• 0015 migration — permission_required_tools=[]: Fine for an automated pipeline agent, but a one-line comment explaining the intent (as 0002 has for its agents) would help future readers understand this is deliberate, not an omission.

---

Summary

The feature is well-implemented and production-ready. The main actionable item before merge is the loop assertion bug in test_geocoding_resolved_once_and_reused_for_each_occurrence — the ann.annotation_type check is outside the loop and only validates one of the two created annotations. The remaining points are minor style/hardening concerns.

Originally posted by @claude[bot] in #1854 (comment)