working on local implementation

[bmaitner]

No description provided.

[claude]

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.

Once extra usage is available, comment @claude review on this pull request, or push a new commit, to trigger a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate issues remain in historical conversion, successor traversal, validation, and build/cache behavior.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 4 · 2

Open (8)

• Convert cshapes output to sf before processing · New
• Continue traversal to include curated successor entities · New
• Validate tolerance_years as a finite non-negative scalar · New
• Invalidate history when rebuilding CShapes · New
• Verify all component files before marking builds complete · New
• Increase timeout for the CShapes download · New
• Document conditional CShapes download behavior · New
• Remove hardcoded paths from the regeneration script · New

What changed in this PR

Adds historical political-division resolution to GNRS_local() using cached CShapes data, curated history, date-aware matching, lineage, citations, and tests.

Changes:

• Adds history modes, successor handling, and date tolerance.
• Adds CShapes/history assembly, caching, and build orchestration.
• Adds curated historical data and updates tests and documentation.

File	Change / final review notes
tests/​testthat/​test-local-resolve.R	Updates local resolver expectations.
tests/​testthat/​test-local-history.R	Adds historical resolution coverage.
tests/​testthat/​test-local-gadm.R	Supplies CShapes for default history-mode tests.
tests/​testthat/​helper-local.R	Adds CShapes test setup.
R/​local_reference.R	Applies historical fuzzy-matching flags.
R/​local_history.R	Reads and builds CShapes data. moderate (2 votes): Increase and restore the timeout for the fallback download. critical (2 votes): Convert cshapes::cshp() output to sf before the build workflow.
R/​local_history_resolve.R	Resolves historical entities and successors. critical (1 vote): Continue traversal after current nodes so YUCS can reach XK. moderate (1 vote): Preserve NA for unmatched current-only results. moderate (1 vote): Re-summarize or copy dependent fields after successor replacement.
R/​local_history_assemble.R	Assembles history tables and lineage.
R/​local_cache.R	Registers history/CShapes components. moderate (3 votes): Require all component files before reporting a build as complete.
R/​local_build.R	Integrates history/CShapes builds. moderate (2 votes): Invalidate or rebuild history when CShapes is replaced. nit (1 vote): Update source documentation and the offline vignette. moderate (1 vote): Avoid implicitly building GNRS for an independent CShapes build.
R/​GNRS_local.R	Adds historical API options and output fields. nit (3 votes): Document conditional CShapes downloads. moderate (3 votes): Validate tolerance_years as a finite, non-negative scalar. moderate (1 vote): Prevent the new default from breaking callers without history data. moderate (1 vote): Append new arguments after quiet for positional compatibility. nit (1 vote): Regenerate man/GNRS_local.Rd.
R/​GNRS_local_citations.R	Adds history and CShapes citations.
inst/​extdata/​SOURCES.md	Documents source licensing.
inst/​extdata/​history_geonames_names.csv	Adds curated historical GeoNames names.
inst/​extdata/​history_current_entities.csv	Adds current historical entities.
inst/​extdata/​history_curated_periods.csv	Adds curated entity periods.
inst/​extdata/​history_curated_names.csv	Adds curated historical names.
inst/​extdata/​history_curated_lineage.csv	Adds curated successor lineage.
inst/​extdata/​history_curated_entities.csv	Adds curated historical entities.
inst/​extdata/​history_codes.csv	Adds historical codes.
DESCRIPTION	Adds optional dependencies.
data-raw/​history_crosswalk.R	Generates curated history inputs. nit (3 votes): Replace developer-specific absolute paths with configurable paths or inputs.
.Rbuildignore	Excludes data-generation scripts.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved findings affect validation, cache rebuilding, downloads, historical matching, and test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 4 · 3

Open (9)

• Continue traversal to include curated successor entities
• Convert cshapes output to sf before processing
• Increase timeout for the CShapes download
• Verify all component files before marking builds complete
• Invalidate history when rebuilding CShapes
• Validate tolerance_years as a finite non-negative scalar
• Document history and CShapes build sources · New
• Remove hardcoded paths from the regeneration script
• Document conditional CShapes download behavior

Files not reviewed (1)

• man/GNRS_local.Rd: Generated file

Previously missed (1)

In code that hasn't changed since last review

Harden fallback CShapes download and rename handling

R/​local_history.R:68

This is the only download path that does not raise R's timeout or verify the partial file. A default 60-second timeout can abort the ~60 MB fallback download, and an unsuccessful file.rename() is ignored, leaving the cache without a valid CShapes file even though the build continues. Match the existing GeoNames/GADM download handling by extending the timeout, checking file.exists(partial), and checking the rename result.

[Copilot]

Copilot review overview

🟡 Changes recommended

The critical positional-argument break and multiple unresolved history/cache correctness issues require fixes.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Appending new GNRS_local parameters breaks positional callers · New
• Document history and CShapes build sources

Resolved since last review (8)

• Continue traversal to include curated successor entities
• Convert cshapes output to sf before processing
• Increase timeout for the CShapes download
• Verify all component files before marking builds complete
• Invalidate history when rebuilding CShapes
• Validate tolerance_years as a finite non-negative scalar
• Remove hardcoded paths from the regeneration script
• Document conditional CShapes download behavior

Files not reviewed (2)

• man/GNRS_local.Rd: Generated file
• man/GNRS_local_build.Rd: Generated file

Previously missed (4)

In code that hasn't changed since last review

History cache remains stale after dependency rebuilds

R/​local_build.R:182

An existing history cache is rebuilt here only when history or cshapes is requested. However, gnrs_build_history() also prunes names against the assembled current GNRS/GeoNames/GADM name table (R/local_history_resolve.R:75-98), so rebuilding any of those dependencies with overwrite = TRUE leaves history stale. A later default historical resolution can then use old collision decisions; track whether a reference/name dependency was actually rebuilt and invalidate/rebuild history as well.

History aliases bypass alternate-name filtering

R/​local_history_resolve.R:184

This marks every history name as original, but GNRS_local() removes GeoNames alternate names before calling gnrs_backbone_with_history(). The appended Curation/CShapes/GeoNames/ISO history names therefore re-enter both exact and fuzzy alternate-name scopes even when alternate_names = FALSE, contradicting the documented standard-names-and-codes contract (and the existing filtering in R/GNRS_local.R:260-274). Pass the option through and keep history aliases in a separate scope, or filter them here rather than treating them as GeoNames originals.

Default history resolution performs an unnecessary second cascade

R/​local_history_resolve.R:287

This unconditional second resolution runs the entire cascade against the current-only backbone even though r_cur is only needed when history == "at_date" to replace out-of-date historical matches. Because history = "all" is the default, ordinary calls pay roughly twice the matching cost; compute r_cur only for at_date (or reuse r_all otherwise).

Overall scores remain stale after historical successor updates

R/​local_history_resolve.R:334

The successor state/county values and their individual scores are copied into r_all, but overall_score was computed by gnrs_summarize() before those fields were filled. A row that fully resolves a state or county under a historical country is therefore returned with a stale country-only score (for example, 1/3) despite the updated full-match status. Re-summarize the affected rows after the copy or recompute overall_score.

[Copilot]

Copilot review overview

🟡 Changes recommended

Nine unresolved moderate findings affect cache invalidation, historical matching, successor outputs, performance, and test-cache completeness.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• History cache not invalidated when name sources are removed · New
• ISO aliases use descriptive names instead of historical codes · New
• Document history and CShapes build sources

Resolved since last review (1)

• Appending new GNRS_local parameters breaks positional callers

Files not reviewed (2)

• man/GNRS_local.Rd: Generated file
• man/GNRS_local_build.Rd: Generated file

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical and moderate findings remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1 · 1

Open (4)

• Default build omits required history dependency · New
• Successor merge leaves stale summary fields · New
• History cache not invalidated when name sources are removed
• Document history and CShapes build sources

Resolved since last review (1)

• ISO aliases use descriptive names instead of historical codes

Files not reviewed (3)

• man/GNRS_local.Rd: Generated file
• man/GNRS_local_build.Rd: Generated file
• man/GNRS_local_remove.Rd: Generated file

Previously missed (1)

In code that hasn't changed since last review

Point rebuild unnecessarily triggers history rebuild

R/​local_build.R:162

dependency_rebuilt is set for every rebuilt source, including points, but the history assembly depends on CShapes and the assembled name table—not the coordinates cache. Consequently, GNRS_local_build("points", overwrite = TRUE) needlessly reruns the full history/geometry-lineage build, adding a potentially expensive geospatial rebuild with no changed inputs. Track only gnrs, geonames, and gadm as history name-table dependencies (or use a separate dependency flag).

[claude]

⚠️ Code review skipped — your organization has no extra usage available to pay for this review.

If your organization's extra usage balance is empty, an organization admin can add extra usage credits at claude.ai/admin-settings/usage. If its monthly spend limit was reached, an admin can raise it on the same page. If neither applies, contact Anthropic support.

Once extra usage is available, comment @claude review on this pull request, or push a new commit, to trigger a review.

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved cache, build, date-aware matching, historical filtering, and test-coverage issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 3 · 4

Open (11)

• Key altdiv cache by directory and file state · New
• Pass record dates to alternative division matching · New
• Successor merge leaves stale summary fields
• Default build omits required history dependency
• Create custom cache directory before altdiv build · New
• Avoid unnecessary GNRS prerequisite for standalone sources · New
• History cache not invalidated when name sources are removed
• Document altdiv columns in public return values · New
• Correct optional component count in documentation · New
• Replace absolute paths in altdiv generation script · New
• Document history and CShapes build sources

Files not reviewed (3)

• man/GNRS_local.Rd: Generated file
• man/GNRS_local_build.Rd: Generated file
• man/GNRS_local_remove.Rd: Generated file

[Copilot]

Copilot review overview

🟡 Changes recommended

Unresolved moderate issues remain in resolver date/name handling and build dependency invalidation.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 3 · 1

Open (5)

• Pass record dates to alternative division matching
• Return canonical alternative division names · New
• Create custom cache directory before altdiv build
• History cache not invalidated when name sources are removed
• Document history and CShapes build sources

Resolved since last review (7)

• Key altdiv cache by directory and file state
• Successor merge leaves stale summary fields
• Default build omits required history dependency
• Avoid unnecessary GNRS prerequisite for standalone sources
• Replace absolute paths in altdiv generation script
• Correct optional component count in documentation
• Document altdiv columns in public return values

Files not reviewed (3)

• man/GNRS_local.Rd: Generated file
• man/GNRS_local_build.Rd: Generated file
• man/GNRS_local_remove.Rd: Generated file

Previously missed (1)

In code that hasn't changed since last review

Limit dependency_rebuilt to history source components

R/​local_build.R:169

dependency_rebuilt is set for every source, including points, but history is derived only from the GNRS/GADM/GeoNames name table and CShapes. As a result, GNRS_local_build("points") unnecessarily reruns the expensive history assembly whenever a history cache exists. Set this flag only for the components that feed gnrs_assemble_names() (the removal path already uses that narrower dependency set).