Address PR #1854 review: blank-span guard, prepared type, docs link/scope, comments

- Skip blank/whitespace exact_string in add_annotations_from_exact_strings
  (prevents wasted geocoder call and an infinite occurrence loop on find(""))
- Tighten prepared tuple type to dict[str, str] | None
- Fix stale docs link (walkthrough/step-9-corpus-actions.md)
- Note migration system_instructions is a point-in-time snapshot
- Scope end-to-end cleanup delete to the test corpus (was DB-wide OC_* wipe)
- Comment the unsaved-then-save pattern, the migration identity check, and the
  population tie-break assumption in test_city_without_hints_resolves_to_france

Author: JSv4

CHANGELOG.md

@@ -46,6 +46,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
     degraded state is visible; added tests pinning the migration→settings
     `system_instructions` wiring and proving geocoding is resolved once per item
     and reused for every occurrence (not re-resolved per match).
+  - **Blank-span guard (#1822 review)**: `add_annotations_from_exact_strings`
+    now skips items whose `exact_string` is blank/whitespace-only. Besides
+    wasting a geocoder call, `doc_text.find("")` returns the search start so the
+    occurrence loop never advanced — a blank span would have spun forever.
+    Mirrors the blank-`raw_text` guard in `_create_geographic_annotation`.
 
 - **Chunked (resumable) uploads for large files** — work around the 100 MB
   per-request body ceiling that upstream proxies (Cloudflare) impose on the

docs/agents/location_tagger.md

@@ -7,7 +7,7 @@ resolved coordinates and administrative codes in its `data` payload, so the
 places show up as pins on the **Discover** and **Corpus Home** maps.
 
 It is wired to run through the ordinary
-[corpus-actions](../walkthrough/key-concepts/corpus-actions.md) framework — the
+[corpus-actions](../walkthrough/step-9-corpus-actions.md) framework — the
 same execution path used by the built-in Document Assistant and Corpus
 Assistant — so you configure it once on a corpus and it runs automatically.
 
@@ -42,6 +42,13 @@ them, so existing callers of the tool keep working unchanged.
    docker compose -f production.yml --profile migrate up migrate
    ```
 
+   > **Note — the migration is a one-time snapshot.** It copies
+   > `DEFAULT_LOCATION_TAGGER_INSTRUCTIONS` from settings into the agent's
+   > `system_instructions` column at creation time. If you later improve the
+   > prompt in `config/settings/base.py`, **existing databases keep the old
+   > prompt** — update the `Location Tagger` agent record via the Django admin
+   > (or a follow-up data migration) to pick up the revision.
+
 2. **Add a corpus action.** On the corpus you want tagged, create a
    `CorpusAction` that points at the **Location Tagger** agent and choose a
    trigger:

docs/test_scripts/location_tagger_end_to_end.md

@@ -89,10 +89,16 @@ from opencontractserver.annotations.models import Annotation
 from opencontractserver.constants.annotations import (
     OC_CITY_LABEL, OC_COUNTRY_LABEL, OC_STATE_LABEL,
 )
-Annotation.objects.filter(
-    annotation_label__text__in=[OC_COUNTRY_LABEL, OC_STATE_LABEL, OC_CITY_LABEL]
-).delete()
 from opencontractserver.corpuses.models import CorpusAction
-CorpusAction.objects.filter(name='Auto location tagger').delete()
+# Scope the cleanup to the corpus this test targeted. A global
+# annotation_label__text__in delete would wipe OC_* annotations across EVERY
+# corpus in the database — never run that against a populated staging/prod DB.
+action = CorpusAction.objects.filter(name='Auto location tagger').first()
+if action is not None:
+    Annotation.objects.filter(
+        corpus=action.corpus,
+        annotation_label__text__in=[OC_COUNTRY_LABEL, OC_STATE_LABEL, OC_CITY_LABEL],
+    ).delete()
+    action.delete()
 "
 ```

opencontractserver/agents/migrations/0015_create_location_tagger_agent.py

@@ -63,6 +63,10 @@ def create_location_tagger_agent(apps, schema_editor):
     instructions = getattr(
         settings, "DEFAULT_LOCATION_TAGGER_INSTRUCTIONS", _FALLBACK_INSTRUCTIONS
     )
+    # Identity check (``is``, not ``==``) is intentional: ``getattr`` returns the
+    # exact ``_FALLBACK_INSTRUCTIONS`` object only when the setting is missing, so
+    # this distinguishes "setting absent" from "setting happens to equal the
+    # fallback text".
     if instructions is _FALLBACK_INSTRUCTIONS:
         # The rich production prompt lives in settings; if it is absent (e.g. a
         # stripped-down CI image) we still create a working agent, but with the

opencontractserver/llms/tools/core_tools/annotations.py

@@ -225,11 +225,18 @@ def add_annotations_from_exact_strings(
         # ``item`` is an AnnotationItem TypedDict (always a dict at runtime);
         # the inner isinstance still guards against the LLM sending a non-dict
         # ``hints`` value.
+        exact_str = str(item["exact_string"])
+        # Skip blank/whitespace-only spans. Besides wasting a geocoder call for
+        # OC_* labels, ``doc_text.find("")`` returns the search start so the
+        # occurrence loop below would never advance (infinite loop). Mirrors the
+        # blank-``raw_text`` guard in ``_create_geographic_annotation``.
+        if not exact_str.strip():
+            continue
         raw_hints = item.get("hints")
         parsed_items.append(
             (
                 str(item["label_text"]),
-                str(item["exact_string"]),
+                exact_str,
                 raw_hints if isinstance(raw_hints, dict) else None,
             )
         )
@@ -349,7 +356,7 @@ def _create_annotation(pos: int, end_idx: int, label_obj):
     # the payload for every occurrence inside the loop. Only the OC_*
     # geographic labels geocode; every other label keeps ``data`` NULL
     # (backward compatible with existing callers such as structured extraction).
-    prepared: list[tuple[str, str, dict | None]] = []
+    prepared: list[tuple[str, str, dict[str, str] | None]] = []
     for label_text, exact_str, hints in parsed_items:
         geocode_label_type = LABEL_TEXT_TO_GEOCODE_LABEL_TYPE.get(label_text)
         annotation_data = None
@@ -379,6 +386,9 @@ def _create_annotation(pos: int, end_idx: int, label_obj):
 
                 end_idx = pos + len(exact_str)
 
+                # ``_create_annotation`` returns an *unsaved* instance; set the
+                # pre-resolved geocode ``data`` (when present) before the single
+                # ``save()`` so each occurrence is one DB write, not two.
                 annot_obj = _create_annotation(pos, end_idx, label_obj)
                 if annotation_data is not None:
                     annot_obj.data = annotation_data

opencontractserver/tests/test_location_tagger_agent.py

@@ -161,7 +161,11 @@ def test_city_hints_disambiguate_paris_to_texas(self):
         self.assertLess(ann.data["lng"], 0)
 
     def test_city_without_hints_resolves_to_france(self):
-        # Population tie-break: unhinted "Paris" is the French one.
+        # Assumption: an unhinted "Paris" resolves to the highest-population
+        # match (Paris, FR over Paris, TX). This ties to the geocoder's current
+        # tie-break dataset; if that ranking changes, relax this to assert only
+        # ``ann.data["geocoded"] is True`` and leave the country to the
+        # geocoding service's own tests.
         ids = self._annotate([{"label_text": OC_CITY_LABEL, "exact_string": "Paris"}])
         ann = Annotation.objects.get(pk=ids[0])
         self.assertTrue(ann.data["geocoded"])