Address General Code Review Findings

[mwaddell]

General Code Review Findings

This review covers the application code, command-line entry points, cache,
configuration, tests, and CI configuration. It excludes issues already tracked
in API_FAILURE_HANDLING_ISSUES.md and TEST_COVERAGE_GAPS.md.

High Priority

Writable caches with an incompatible schema are accepted and fail later

Relevant code: src/cache.py:87-94, src/cache.py:143-174, src/cache.py:178-203.

Cache._open_writer() runs CREATE TABLE IF NOT EXISTS, which succeeds when an
existing database already has a cache table. Unlike _open_reader(), it does
not check that the table has the columns required by lookups and writes. The
first lookup or insert then raises a raw sqlite3.OperationalError, potentially
after work has started.

For example, --cache legacy.sqlite can point to a database containing
cache(key, value). Startup succeeds, but the first lookup fails because
query and response are absent; a cache miss instead fails on the missing
original_query or fetched_at columns.

Recommended remediation:

• Validate a writable cache's existing schema before processing, using the same
  checks as read-only caches plus all columns required for inserts.
• Alternatively, implement an explicit, tested schema migration.
• Translate an invalid writable cache into the existing user-facing cache error.

Missing test:

• A writable SQLite database with a foreign or legacy-shaped cache table
  fails at initialization with a descriptive error.

Unopenable writable cache paths escape the CLI's error handling

Relevant code: src/cache.py:87, src/geocoder.py:404-407.

The SQLite connection is created before _open_writer() enters its try block.
Consequently, a missing parent directory, denied permissions, or another
connection-creation failure raises sqlite3.OperationalError, not the
ValueError that main() converts into error: ... output.

For example, --cache missing-directory/geocoder.sqlite terminates with a
traceback instead of explaining that the cache cannot be opened.

Recommended remediation:

• Catch sqlite3.Error around sqlite3.connect() and re-raise a contextual
  ValueError that names the cache path.
• Keep the existing main() translation so all expected cache setup failures
  use the same CLI error format.

Missing test:

• main() given a writable cache under a nonexistent directory exits with
  a readable SystemExit message rather than a traceback.

Medium Priority

Invalid JSON in an otherwise valid cache row aborts the run without context

Relevant code: src/cache.py:171-174.

lookup() calls json.loads(response) without handling decode failures. A
truncated, manually modified, or externally generated cache row therefore
raises an uncontextualized JSONDecodeError. The user cannot identify the
cache file, provider, or query that is corrupt, nor can the application apply a
deliberate fallback policy.

Recommended remediation:

• Catch JSON decode errors and raise a cache-corruption error including the
  cache source, provider, and normalized query key.
• Decide and document whether a corrupt writable-cache entry should be ignored
  and fetched again, or should fail the run. Never silently use malformed data.

Missing test:

• An invalid response JSON value in a valid cache table produces the
  selected behavior and a contextual diagnostic.

Cache construction can leak an opened writer and create a cache on failed startup

Relevant code: src/cache.py:62-65.

The writable cache is opened before every read-only cache. If opening or
validating a later read_paths entry raises, the already-open writer is never
closed. If it is a new path, an empty cache file is also left behind even though
the command never starts processing.

For example, --cache new.sqlite --cacheRead invalid.sqlite creates
new.sqlite and retains its open connection before reporting that
invalid.sqlite is unusable. In a long-lived embedding process this can leak
file descriptors; at the CLI it leaves misleading state for a failed run.

Recommended remediation:

• Open and validate all reader connections before creating the writer, or close
  every successfully opened connection when constructor initialization fails.
• Avoid creating a new writable cache until all configured cache inputs have
  passed validation.

Missing test:

• An invalid read-only cache combined with a new writable cache leaves no
  writable cache file behind and closes any opened connection.

.env loading persists credentials across calls to main() in one process

Relevant code: src/geocoder.py:69-89, src/geocoder.py:380-401.

load_dotenv() writes values into process-global os.environ and main() does
not restore them. Calling main() more than once, including from a test runner,
GUI, or another Python application, therefore makes later calls inherit keys
from the first working directory's .env file.

For example, a first call from directory A loads
GOOGLE_GEOCODING_API_KEY; a second call from directory B without a key can
unexpectedly use A's credential rather than fail its required-key check.

Recommended remediation:

• Parse .env into a local mapping and pass it to key resolution, preserving
  the precedence of real environment variables and --apiKey.
• If global environment mutation is intentionally retained, isolate it to the
  executable entry point and document that main() is not re-entrant.

Missing test:

• Two main() calls from different working directories cannot cause the
  second call to inherit an API key loaded from the first directory's .env.

Formula cells may silently become stale or blank input values

Relevant code: src/geocoder.py:268, README.md:35-56, README.md:134-143.

Workbooks are loaded with data_only=True, which returns Excel's most recently
cached formula result rather than the formula. openpyxl does not calculate
formulas. A workbook generated without Excel recalculation can consequently
yield None for an address component, skip an otherwise valid row, or geocode
an obsolete cached value. The generated SOURCE_* columns also retain only the
cached value, not the original formula, despite the README describing source
fields as retained.

Recommended remediation:

• Explicitly document that input address fields must contain current literal
  values and formulas are unsupported, or reject formulas in required columns
  with a clear error.
• If formulas are a supported input, require a calculation engine or a
  pre-calculated workbook as part of the ingestion contract.

Missing test:

• A workbook with an uncached formula in a required address field is either
  rejected descriptively or processed according to documented supported
  behavior.

Low Priority

Duplicate results share a mutable raw-response object

Relevant code: src/api.py:285-293, src/api.py:46-63.

Deduplicated input queries are backed by one responses[key] dictionary. The
base class parses that same object independently for each duplicate, and the
providers store it directly in GeocodeResult.raw. As a result, callers that
modify one result's raw response also modify every duplicate result's raw
response.

For example, an embedding application that redacts or annotates
results[0].raw before rendering it alters the debug data associated with other
rows for the same query.

Recommended remediation:

• Define raw as immutable application data, or deep-copy it when materializing
  each GeocodeResult.
• Document the ownership contract so provider implementations do not
  accidentally reintroduce shared mutable responses.

Missing test:

• Mutating one duplicate query's returned raw value does not alter the
  other result's raw value.

Pytest configuration selects a nonexistent test directory

Relevant code: pytest.ini:1-3, pyproject.toml:36-38,
.github/workflows/build-and-test.yml:54-57.

pytest.ini takes precedence over the equivalent configuration in
pyproject.toml and sets testpaths = src/test, but tests reside in test/.
A plain pytest invocation warns and falls back to recursive collection, while
CI masks the problem by explicitly invoking pytest test.

This makes local and CI collection behavior differ. It can also make warnings-
as-errors environments fail or cause unrelated tests below the repository root
to be collected.

Recommended remediation:

• Change pytest.ini to testpaths = test, or remove it and retain the
  pyproject.toml configuration as the sole source of truth.
• Run plain pytest in CI so the supported developer command is verified.

Missing test:

• No dedicated test is needed after consolidation; CI should execute plain
  pytest without collection warnings.

[mwaddell]

Note that some of these findings were based on the to-be-merged #15