fix(salary): validate category shape and add --validate preflight (#156)

validate_data() accepted category values that are not {count?, index?}
objects. They slipped through to format_entry(), which then raised
AttributeError on a normal table lookup (or serialized a malformed shape
under --json). It also accepted duplicate company names silently.

- collect_validation_issues() now also flags a non-object category value
  (and non-numeric count / non number-or-string index) as a hard error,
  and duplicate company names as a warning.
- validate_data() keeps its eager-fail behavior (same messages), so
  existing tests and load_data() are unchanged.
- --validate runs the checks standalone and prints an actionable report
  (exit 1 on errors, 0 on warnings-only/clean), letting users pre-flight
  their BYO salary_data.json.

Reproduced on master: validate_data({'companies':[{'company':'Acme',
'categories':{'eng':'not_a_dict'}}]}) returns without error, but
format_entry then raises AttributeError.

Co-authored-by: Tunic Assistant <assistant@tunic.local>

salary_lookup.py

@@ -51,39 +51,94 @@ def fail_data_error(message):
     sys.exit(1)
 
 
-def validate_data(data):
+def collect_validation_issues(data):
-    """Validate the salary data shape before lookups use it."""
+    """Return (errors, warnings) for the salary data shape.
+
+    errors   -> hard problems that make lookups crash or emit wrong output
+                (these cause validate_data() to exit(1)).
+    warnings -> usability concerns that still work (e.g. duplicate company
+                names); --validate reports them but exits 0.
+    """
+    errors = []
+    warnings = []
+
     if not isinstance(data, dict):
-        fail_data_error("top-level JSON value must be an object")
+        errors.append("top-level JSON value must be an object")
+        return errors, warnings
 
     metadata = data.get("metadata", {})
     if metadata is not None and not isinstance(metadata, dict):
-        fail_data_error("'metadata' must be an object when provided")
+        errors.append("'metadata' must be an object when provided")
 
     companies = data.get("companies")
     if not isinstance(companies, list):
-        fail_data_error("'companies' must be a list")
+        errors.append("'companies' must be a list")
+        return errors, warnings
 
+    seen_companies = {}
     for index, entry in enumerate(companies, start=1):
         if not isinstance(entry, dict):
-            fail_data_error(f"companies[{index}] must be an object")
+            errors.append(f"companies[{index}] must be an object")
+            continue
 
         company = entry.get("company")
         if not isinstance(company, str) or not company.strip():
-            fail_data_error(f"companies[{index}].company must be a non-empty string")
+            errors.append(f"companies[{index}].company must be a non-empty string")
+        else:
+            key = company.lower()
+            if key in seen_companies:
+                warnings.append(
+                    f"Duplicate company name '{company}' "
+                    f"(companies[{seen_companies[key]}] and companies[{index}])"
+                )
+            else:
+                seen_companies[key] = index
 
         city = entry.get("city")
         if city is not None and not isinstance(city, str):
-            fail_data_error(f"companies[{index}].city must be a string when provided")
+            errors.append(f"companies[{index}].city must be a string when provided")
 
         categories = entry.get("categories", {})
         if categories is not None and not isinstance(categories, dict):
-            fail_data_error(f"companies[{index}].categories must be an object when provided")
+            errors.append(f"companies[{index}].categories must be an object when provided")
+        elif categories:
+            for cat_label, cat_data in categories.items():
+                if not isinstance(cat_data, dict):
+                    errors.append(
+                        f"companies[{index}].categories.{cat_label} must be an object "
+                        f"with 'count' and/or 'index' (got {type(cat_data).__name__})"
+                    )
+                    continue
+                count = cat_data.get("count")
+                if count is not None and not isinstance(count, (int, float)):
+                    errors.append(
+                        f"companies[{index}].categories.{cat_label}.count must be a "
+                        f"number (got {type(count).__name__})"
+                    )
+                index_val = cat_data.get("index")
+                if index_val is not None and not isinstance(index_val, (int, float, str)):
+                    errors.append(
+                        f"companies[{index}].categories.{cat_label}.index must be a "
+                        f"number or string (got {type(index_val).__name__})"
+                    )
 
+    return errors, warnings
+
+
+def validate_data(data):
+    """Validate the salary data shape before lookups use it.
+
+    Preserves historical behavior: exits(1) on the first hard error with the
+    same user-facing message, and returns data unchanged when valid.
+    """
+    errors, _ = collect_validation_issues(data)
+    if errors:
+        fail_data_error(errors[0])
     return data
 
 
-def load_data():
+def read_raw_data():
+    """Load and JSON-parse salary_data.json; exit with a helpful message if missing/invalid."""
     if not DATA_FILE.exists():
         print("Error: salary_data.json not found.", file=sys.stderr)
         print("", file=sys.stderr)
@@ -98,7 +153,12 @@ def load_data():
             data = json.load(f)
     except json.JSONDecodeError as exc:
         fail_data_error(f"invalid JSON at line {exc.lineno}, column {exc.colno}: {exc.msg}")
-    return validate_data(data)
+    return data
+
+
+def load_data():
+    """Load, parse, and validate salary_data.json for lookups."""
+    return validate_data(read_raw_data())
 
 
 def normalize(s):
@@ -292,14 +352,45 @@ def format_entry(entry, metadata):
     return "\n".join(lines)
 
 
+def print_validation_report(errors, warnings):
+    """Print an actionable validation report. Returns the process exit code."""
+    if not errors and not warnings:
+        print("OK - no issues found.")
+        return 0
+    print(f"Found {len(errors) + len(warnings)} issue(s):")
+    if errors:
+        print("  Errors:")
+        for i, msg in enumerate(errors, start=1):
+            print(f"    [{i}] {msg}")
+    if warnings:
+        print("  Warnings:")
+        for i, msg in enumerate(warnings, start=1):
+            print(f"    [{i}] {msg}")
+    if errors:
+        print("")
+        print("Fix the errors above, then re-run. See tools/README_SALARY_TOOL.md "
+              "for the expected format.")
+        return 1
+    return 0
+
+
 def main():
     parser = argparse.ArgumentParser(description="Salary Benchmark Lookup")
     parser.add_argument("company", nargs="?", help="Company name to search for")
     parser.add_argument("--city", help="Filter by city name")
     parser.add_argument("--json", action="store_true", help="Output as JSON")
     parser.add_argument("--list-all", action="store_true", help="List all companies")
+    parser.add_argument("--validate", action="store_true",
+                        help="Validate salary_data.json and print a report, then exit")
     args = parser.parse_args()
 
+    if args.validate:
+        data = read_raw_data()
+        errors, warnings = collect_validation_issues(data)
+        print(f"Validating {DATA_FILE.name} ...")
+        print("")
+        sys.exit(print_validation_report(errors, warnings))
+
     data = load_data()
     metadata = data.get("metadata", {})
     companies = data.get("companies", [])

tests/test_salary_lookup.py

@@ -3,8 +3,9 @@
 import io
 import tempfile
 import unittest
-from contextlib import redirect_stderr
+from contextlib import redirect_stderr, redirect_stdout
 from pathlib import Path
+from unittest import mock
 
 import salary_lookup
 from salary_lookup import (
@@ -15,6 +16,7 @@ from salary_lookup import (
     match_score,
     search_company,
     validate_data,
+    collect_validation_issues,
 )
 
 
@@ -241,6 +243,88 @@ class ValidateDataTests(unittest.TestCase):
         )
 
 
+class ValidateDataShapeTests(ValidateDataTests):
+    """Category-shape and duplicate-name checks (reuses assert_invalid_data)."""
+
+    def test_malformed_category_value_rejected(self):
+        data = {"companies": [{"company": "Acme", "categories": {"eng": "not_a_dict"}}]}
+        self.assert_invalid_data(data, "must be an object with 'count' and/or 'index'")
+
+    def test_non_numeric_count_rejected(self):
+        data = {
+            "companies": [
+                {"company": "Acme", "categories": {"eng": {"count": "many"}}}
+            ]
+        }
+        self.assert_invalid_data(data, "count must be a number")
+
+    def test_duplicate_company_name_is_warning(self):
+        data = {
+            "companies": [
+                {"company": "Acme"},
+                {"company": "Other Corp"},
+                {"company": "Acme"},
+            ]
+        }
+        errors, warnings = collect_validation_issues(data)
+        self.assertEqual(errors, [])
+        self.assertEqual(len(warnings), 1)
+        self.assertIn("Duplicate company name 'Acme'", warnings[0])
+
+    def test_valid_categories_have_no_issues(self):
+        data = {
+            "companies": [
+                {"company": "Acme", "categories": {"eng": {"count": 5, "index": 108.5}}}
+            ]
+        }
+        errors, warnings = collect_validation_issues(data)
+        self.assertEqual(errors, [])
+        self.assertEqual(warnings, [])
+
+
+class ValidateFlagTests(unittest.TestCase):
+    """End-to-end checks for the --validate pre-flight flow."""
+
+    def _run_validate(self, payload):
+        with tempfile.TemporaryDirectory() as tmpdir:
+            data_file = Path(tmpdir) / "salary_data.json"
+            data_file.write_text(payload, encoding="utf-8")
+            original_data_file = salary_lookup.DATA_FILE
+            salary_lookup.DATA_FILE = data_file
+            argv_patch = mock.patch("sys.argv", ["salary_lookup.py", "--validate"])
+            argv_patch.start()
+            try:
+                stdout = io.StringIO()
+                with self.assertRaises(SystemExit) as raised:
+                    with redirect_stdout(stdout):
+                        salary_lookup.main()
+                return raised.exception.code, stdout.getvalue()
+            finally:
+                argv_patch.stop()
+                salary_lookup.DATA_FILE = original_data_file
+
+    def test_validate_flag_exits_1_on_errors(self):
+        code, out = self._run_validate(
+            '{"companies": [{"company": "Acme", "categories": {"eng": "not_a_dict"}}]}'
+        )
+        self.assertEqual(code, 1)
+        self.assertIn("must be an object with 'count' and/or 'index'", out)
+
+    def test_validate_flag_exits_0_on_clean(self):
+        code, out = self._run_validate(
+            '{"companies": [{"company": "Acme", "categories": {"eng": {"count": 5}}}]}'
+        )
+        self.assertEqual(code, 0)
+        self.assertIn("OK", out)
+
+    def test_validate_flag_exits_0_on_duplicates_only(self):
+        code, out = self._run_validate(
+            '{"companies": [{"company": "Acme"}, {"company": "Acme"}]}'
+        )
+        self.assertEqual(code, 0)
+        self.assertIn("Duplicate company name", out)
+
+
 class UtilityTests(unittest.TestCase):
     def test_normalize_strips_suffix_and_noise(self):
         self.assertEqual(normalize("Novo Nordisk A/S"), "novonordisk")

tools/README_SALARY_TOOL.md

@@ -113,6 +113,7 @@ python3 salary_lookup.py "Novo Nordisk"
 python3 salary_lookup.py "Ørsted" --city "Fredericia"
 python3 salary_lookup.py "COWI" --json
 python3 salary_lookup.py --list-all
+python3 salary_lookup.py --validate      # pre-flight check your salary_data.json
 ```
 
 ## Important notes
@@ -120,3 +121,4 @@ python3 salary_lookup.py --list-all
 - The data file (`salary_data.json`) is **excluded from git** (see `.gitignore`). Your salary data may be proprietary or confidential.
 - If the data file is missing, `salary_lookup.py` exits with a helpful error message and the `/apply` workflow skips the salary benchmark step.
 - The fuzzy matcher handles Danish company name variations: legal suffixes, Nordic characters, anglicized spellings, and partial matches.
+- `--validate` checks your data file for malformed category values and duplicate company names and prints a report, without performing a lookup.