From f0738d169affe8aaf0397489cea53c84fb4cee77 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 15:47:48 +0000 Subject: [PATCH 01/11] progress: issue review for #112 --- .agent/work-plans/issue-112/progress.md | 58 +++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 .agent/work-plans/issue-112/progress.md diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md new file mode 100644 index 0000000..90e286f --- /dev/null +++ b/.agent/work-plans/issue-112/progress.md @@ -0,0 +1,58 @@ +--- +issue: 112 +--- + +# Issue #112 — GGGS tile glob matches `_time`/`_source` companion files + +## Issue Review +**Status**: complete +**When**: 2026-06-21 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #112 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +Companion tiles (`_time.tif`, `_source.tif`) produced by the bathy and MBES backscatter stores (unh_marine_autonomy#178 / #194) land in the same directory as the value tile (`.tif`). The GGGS tile-layer and store-source glob `*.tif`/`*.tiff` with no companion-suffix filter, so CAMP treats the companions as renderable value tiles — producing garbage renders. + +**Current scan sites (verified against HEAD, post–camp#104):** +- `src/camp2/raster/gggs_tile_layer.cpp:122` — `loadDirectory()` entryList glob +- `src/camp2/raster/gggs_tile_layer.cpp:170` — `rescan()` entryList glob +- `src/camp2/raster/gggs_store_source.cpp:23–24` — `dirHasTifs()` entryList glob + +The two stale sites from the original issue body (`gggs_store_layer.cpp:21` / `:27`) no longer exist — that file was retired by camp#104. Only the three sites above remain. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| A change includes its consequences | Action needed | The fix requires a companion test (companion files in a store dir; assert only base tiles listed/rendered). Extensive GGGS test coverage already exists (`test_gggs_rescan.cpp`, `test_gggs_flat_layer_spawn.cpp`, `test_gggs_render.cpp`, etc.) — the new test follows the same pattern. | +| Test what breaks | Action needed | No test currently exercises `_time`/`_source` companions; one must be added. | +| Only what's needed | OK | Targeted 3-site fix + shared helper; no scope creep. | +| Improve incrementally | OK | Narrow fix, single PR, no cross-cutting redesign. | +| Capture decisions, not just implementations | Watch | The positive-pattern (`\d+_\d+_\d+\.tif`) vs. denylist (`_time`/`_source` suffix exclusion) choice should be noted inline so future companion additions know which approach was taken and why. | +| Workspace vs. project separation | OK | All changes are in the `camp` project repo; no workspace infra touched. | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0001 (Adopt ADRs) | No | Design choice (positive pattern vs. denylist) is implementor's call; a code comment suffices — no new ADR needed. | +| ADR-0002 (Worktree isolation) | Yes | Worktree already in place for issue-112. | +| ADR-0005 (Stores browser / flat display layers) | Yes (context) | This fix is consistent with ADR-0005: `dirHasTifs()` and `buildNode()` in `GggsStoreSource` are the successors to `GggsStoreLayer`'s subtree scan; the companion filter applies here too. | +| ADR-0008 (ROS 2 conventions) | Yes | C++ change in a ROS 2 package — follow standard ROS 2/ament_cmake C++ conventions. | +| ADR-0013 (progress.md vocabulary) | Yes | This entry. | + +### Consequences + +- A shared `isValueTile(QString)` helper (or positive-pattern lambda) must be placed in a single location and called at all three scan sites — the issue explicitly flags this so future companion bands don't require multi-site edits. +- One new test file (or additions to `test_gggs_flat_layer_spawn.cpp` / `test_gggs_rescan.cpp`) must exercise the companion-filter path. +- No interface changes, no `.msg`/`.srv` changes, no doc updates needed beyond inline comments. + +### Actions +- [ ] Implement `isValueTile()` helper (or positive-pattern filter) at a single definition site. +- [ ] Apply the filter at all three current scan sites: `loadDirectory()`, `rescan()`, `dirHasTifs()`. +- [ ] Add a test: construct a store dir with base + `_time` + `_source` companions; assert only the base tile is listed/loaded/rendered. +- [ ] Document the positive-vs.-denylist choice in an inline comment so future companion bands know which approach to extend. From 1251ab753b4e3fd93544a6ec61a3d42868a377b9 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 15:57:14 +0000 Subject: [PATCH 02/11] =?UTF-8?q?plan:=20work=20plan=20for=20#112=20?= =?UTF-8?q?=E2=80=94=20companion=20tile=20filter?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add isValueTile() helper at a single definition site (gggs_tile_util.h) and apply at all three current scan sites (loadDirectory, rescan, dirHasTifs). Add a test with base + _time + _source companions asserting only the base tile is enumerated. --- .agent/work-plans/issue-112/plan.md | 112 ++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 .agent/work-plans/issue-112/plan.md diff --git a/.agent/work-plans/issue-112/plan.md b/.agent/work-plans/issue-112/plan.md new file mode 100644 index 0000000..dcd36d2 --- /dev/null +++ b/.agent/work-plans/issue-112/plan.md @@ -0,0 +1,112 @@ +# Plan: GGGS tile glob matches `_time`/`_source` companion files + +## Issue + +https://github.com/rolker/camp/issues/112 + +## Context + +The bathy and MBES backscatter stores produce three files per tile in the same +directory: `__.tif` (value), `___time.tif` +(Int64 ns timestamp), and `___source.tif` (uint16 source index). +CAMP's `*.tif`/`*.tiff` globs in `loadDirectory()`, `rescan()`, and `dirHasTifs()` +match all three. CAMP tries to render the `_time` and `_source` companions as +depth/intensity tiles, producing garbage renders. + +**Current scan sites (verified against HEAD, post-camp#104):** + +- `src/camp2/raster/gggs_tile_layer.cpp:122` — `loadDirectory()` `entryList` glob +- `src/camp2/raster/gggs_tile_layer.cpp:170` — `rescan()` `entryList` glob +- `src/camp2/raster/gggs_store_source.cpp:23` — `dirHasTifs()` `entryList` glob + +## Approach + +1. **Add `isValueTile(const QString&)` free function** (anonymous namespace) in + `gggs_tile_layer.cpp` — returns `true` iff the basename matches the positive + pattern `\d+_\d+_\d+\.tiff?` (digits-underscore-digits-underscore-digits, then + `.tif` or `.tiff`). Positive-match is preferred over a denylist because the + companion-suffix set may grow (ADR-0006-D7 mentions future sidescan time + tiles); the pattern is the canonical definition shared by the stores' own tile + naming convention. Add a one-line comment explaining the pattern so future + companion additions know where to look. + +2. **Apply `isValueTile()` in `loadDirectory()`** — filter the entryList result + before constructing `GggsTile` objects (lines 122–147). + +3. **Apply `isValueTile()` in `rescan()`** — filter the entryList result before + the `known.contains()` check (lines 170–185). + +4. **Move `isValueTile()` to a shared location visible to both compilation units** + OR duplicate the lambda in `gggs_store_source.cpp`. Because the function is + tiny (one QRegularExpression match) and both files are in the same directory, + add it to `gggs_tile_layer.h` as an inline free function in the anonymous + implementation detail namespace — OR declare it in a new `gggs_tile_util.h` + if reviewers prefer. The simplest option is a static helper in a new + `gggs_tile_util.h` (one-line inline) included by both `.cpp` files. + +5. **Apply `isValueTile()` in `dirHasTifs()`** in `gggs_store_source.cpp` — filter + after the `entryList` call so the helper sees only value tiles when deciding + whether a directory holds a tile-set. + +6. **Add a test** — extend `test_catalog_source.cpp` with a case that places + `0_0_0.tif`, `0_0_0_time.tif`, and `0_0_0_source.tif` in a store directory and + asserts `discover()` returns a leaf (the companions don't trip the tile-free + pruning path) and the leaf key points to the directory. Add a parallel case to + `test_gggs_rescan.cpp` (or a new `test_gggs_companion_filter.cpp`) that creates + a tile directory with base + companion files, constructs a `GggsTileLayer`, and + asserts `valid() == true` with exactly one tile (not three). + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp2/raster/gggs_tile_util.h` | New header: `isValueTile(const QString&)` inline function | +| `src/camp2/raster/gggs_tile_layer.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `loadDirectory()` and `rescan()` | +| `src/camp2/raster/gggs_store_source.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `dirHasTifs()` | +| `test/test_gggs_companion_filter.cpp` | New test: store dir with base + `_time` + `_source` companions; assert only base tile is enumerated by `GggsTileLayer` and by `GggsStoreSource::discover()` | +| `CMakeLists.txt` | Register `test_gggs_companion_filter` with `ament_add_gtest` (same pattern as `test_catalog_source` / `test_gggs_rescan`) | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| A change includes its consequences | New test file + CMakeLists registration included in this plan. | +| Test what breaks | Test explicitly covers the bug path: companions in directory → only base tile enumerated. | +| Only what's needed | Narrow 3-site fix + shared helper + one test; no scope beyond issue description. | +| Capture decisions, not just implementations | Positive-pattern vs. denylist choice documented via inline comment in `gggs_tile_util.h`. | +| Improve incrementally | Single PR, no architectural change. | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0001 (Adopt ADRs) | No | Pattern choice (positive vs. denylist) is a code-comment decision, not ADR-worthy. | +| ADR-0002 (Worktree isolation) | Yes | Worktree `issue-camp-112` already in place; all changes land here. | +| ADR-0005 (Layered enforcement) | Context | `dirHasTifs()` in `gggs_store_source.cpp` is the post-#104 successor to `GggsStoreLayer`'s subtree scan; the filter applies identically. | +| ADR-0008 (ROS 2 conventions) | Yes | C++ changes in a ROS 2 ament_cmake package; no convention violations. | +| ADR-0013 (progress.md vocabulary) | Yes | `progress.md` has an `## Issue Review` entry; `## Plan Authored` will be appended. | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `dirHasTifs()` in `gggs_store_source.cpp` | `test_catalog_source.cpp` (companion-in-dir case) | Yes — new test file covers both `GggsTileLayer` and `GggsStoreSource` | +| `loadDirectory()` / `rescan()` glob filter | `test_gggs_rescan.cpp` or new test | Yes — `test_gggs_companion_filter.cpp` covers this | +| Add `gggs_tile_util.h` | `CMakeLists.txt` (install headers if needed) | No install target needed — internal header, not exported | + +## Open Questions + +- **Header placement**: `gggs_tile_util.h` (new shared header) vs. a static lambda + duplicated in each `.cpp` (avoids an extra file). The shared header is cleaner for + future companion types; the duplicate lambda is simpler if no other callers are + expected. Recommendation: shared header, given the issue's explicit callout that + the excluded-suffix list must live in one place. +- **New test file vs. extending existing tests**: `test_gggs_companion_filter.cpp` + (dedicated) vs. adding cases to `test_gggs_rescan.cpp` and `test_catalog_source.cpp`. + Recommendation: add to existing tests where the fixture already covers the relevant + class, rather than creating a new file — unless the test logic is orthogonal enough + to warrant its own setup. + +## Estimated Scope + +Single PR. From 3939067fcc20244f2d18f69e9017ce7e0536d465 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 15:57:27 +0000 Subject: [PATCH 03/11] progress: plan authored for #112 --- .agent/work-plans/issue-112/progress.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md index 90e286f..693455b 100644 --- a/.agent/work-plans/issue-112/progress.md +++ b/.agent/work-plans/issue-112/progress.md @@ -56,3 +56,16 @@ The two stale sites from the original issue body (`gggs_store_layer.cpp:21` / `: - [ ] Apply the filter at all three current scan sites: `loadDirectory()`, `rescan()`, `dirHasTifs()`. - [ ] Add a test: construct a store dir with base + `_time` + `_source` companions; assert only the base tile is listed/loaded/rendered. - [ ] Document the positive-vs.-denylist choice in an inline comment so future companion bands know which approach to extend. + +## Plan Authored +**Status**: complete +**When**: 2026-06-21 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-112/plan.md` at `1251ab7` +**Branch**: feature/issue-112 at `1251ab7` +**Phases**: single + +### Open questions +- [ ] Header placement: new `gggs_tile_util.h` (shared, one definition site) vs. static lambda duplicated in each `.cpp` — recommendation is shared header per the issue's "one place" requirement. +- [ ] New test file (`test_gggs_companion_filter.cpp`) vs. adding cases to existing `test_gggs_rescan.cpp` and `test_catalog_source.cpp` — recommendation is add to existing tests where fixture already covers the relevant class. From f68cb37aeef5fc666fc5e48eee2f542aa842af4c Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:04:57 +0000 Subject: [PATCH 04/11] progress: plan review for #112 --- .agent/work-plans/issue-112/progress.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md index 693455b..0b2bad5 100644 --- a/.agent/work-plans/issue-112/progress.md +++ b/.agent/work-plans/issue-112/progress.md @@ -69,3 +69,27 @@ The two stale sites from the original issue body (`gggs_store_layer.cpp:21` / `: ### Open questions - [ ] Header placement: new `gggs_tile_util.h` (shared, one definition site) vs. static lambda duplicated in each `.cpp` — recommendation is shared header per the issue's "one place" requirement. - [ ] New test file (`test_gggs_companion_filter.cpp`) vs. adding cases to existing `test_gggs_rescan.cpp` and `test_catalog_source.cpp` — recommendation is add to existing tests where fixture already covers the relevant class. + +## Plan Review +**Status**: complete +**When**: 2026-06-21 16:04 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-112/plan.md` at `1251ab7` +**PR**: PR-less (`--issue` mode; `gh` unauthenticated — issue substance cross-checked against the `## Issue Review` entry above) +**Verdict**: approve-with-suggestions + +Independent review (fresh-context Opus dispatch; the `## Plan Authored` entry is a +separate Sonnet dispatch under the shared `Claude Code Agent` name — not author +self-review). Core approach validated against HEAD: three scan sites confirmed +(`gggs_tile_layer.cpp:122`, `:170`; `gggs_store_source.cpp:23`); positive pattern +`\d+_\d+_\d+\.tiff?` matches every legitimate value tile (all tiles/tests use +`__.tif`) and excludes `_time`/`_source` companions without +dropping real tiles. Consequences map and test obligation are complete. All +findings below are suggestions; none block implementation. + +### Findings +- [ ] (suggestion) ADR-0005 mislabeled "Layered enforcement" (workspace title); camp ADR-0005 is "Catalog browser + flat selectable display layers" — fix title, substance is correct — `plan.md:85` +- [ ] (suggestion) Spurious "ADR-0006-D7" citation — camp has no ADR-0006 (only 0001–0005); drop or correct, the "companion set may grow" rationale stands alone — `plan.md:28` +- [ ] (suggestion) Specify anchored/full regex match (`anchoredPattern`/`^…$`) for `isValueTile()` so partial matches can't admit malformed names — `plan.md:27` +- [ ] (suggestion) Resolve test-structure inconsistency: "Files to Change" adds new `test_gggs_companion_filter.cpp` while Approach/Open-Questions recommend extending existing `test_catalog_source.cpp`/`test_gggs_rescan.cpp` (which already have reusable fixtures) — pick one; extending is recommended — `plan.md:56,66,104` From 6f4070dc5cb19a548cca2276cb109439699c10cf Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:18:38 +0000 Subject: [PATCH 05/11] fix(gggs): filter _time/_source companion tiles from value-tile scans (#112) The bathy/MBES stores write three files per tile in one directory: __.tif (value), _time.tif (Int64), and _source.tif (uint16). The GggsTileLayer/GggsStoreSource *.tif globs matched all three, so companions were scanned as renderable value tiles -> garbage renders. Add a shared inline isValueTile() in src/camp2/raster/gggs_tile_util.h matching the anchored value-tile pattern ^\d+_\d+_\d+\.tiff?$ against the filename, and apply it at the three scan sites: loadDirectory() and rescan() in gggs_tile_layer.cpp, and dirHasTifs() in gggs_store_source.cpp. A positive pattern excludes any future companion suffix for free. Sync plan.md to as-built: resolve the two open questions (shared header; extend existing tests), fix the ADR-0005 title, drop the spurious ADR-0006 citation. Co-Authored-By: Claude Opus 4.8 --- .agent/work-plans/issue-112/plan.md | 86 +++++++++++++------------- src/camp2/raster/gggs_store_source.cpp | 16 +++-- src/camp2/raster/gggs_tile_layer.cpp | 9 +++ src/camp2/raster/gggs_tile_util.h | 31 ++++++++++ 4 files changed, 96 insertions(+), 46 deletions(-) create mode 100644 src/camp2/raster/gggs_tile_util.h diff --git a/.agent/work-plans/issue-112/plan.md b/.agent/work-plans/issue-112/plan.md index dcd36d2..34581da 100644 --- a/.agent/work-plans/issue-112/plan.md +++ b/.agent/work-plans/issue-112/plan.md @@ -21,14 +21,18 @@ depth/intensity tiles, producing garbage renders. ## Approach -1. **Add `isValueTile(const QString&)` free function** (anonymous namespace) in - `gggs_tile_layer.cpp` — returns `true` iff the basename matches the positive - pattern `\d+_\d+_\d+\.tiff?` (digits-underscore-digits-underscore-digits, then - `.tif` or `.tiff`). Positive-match is preferred over a denylist because the - companion-suffix set may grow (ADR-0006-D7 mentions future sidescan time - tiles); the pattern is the canonical definition shared by the stores' own tile - naming convention. Add a one-line comment explaining the pattern so future - companion additions know where to look. +1. **Add `isValueTile(const QString&)` inline free function** in a new shared + header `src/camp2/raster/gggs_tile_util.h` (namespace `camp::raster`) — + returns `true` iff the **filename** (what `QDir::entryList` returns, never a + full path) matches the **anchored, full-match** positive pattern + `^\d+_\d+_\d+\.tiff?$` via `QRegularExpression::anchoredPattern(...)` with a + function-local `static const QRegularExpression` (compiled once). Exactly three + underscore-separated digit groups then `.tif`/`.tiff`. Positive-match is + preferred over a denylist because the companion-suffix set may grow; the + pattern is the canonical value-tile name shared with the stores' own tile- + naming convention, and a 4th `_time`/`_source` component (or any future + companion suffix) is excluded for free by not matching. A one-line comment in + the header explains this so future companion additions know where to look. 2. **Apply `isValueTile()` in `loadDirectory()`** — filter the entryList result before constructing `GggsTile` objects (lines 122–147). @@ -36,25 +40,25 @@ depth/intensity tiles, producing garbage renders. 3. **Apply `isValueTile()` in `rescan()`** — filter the entryList result before the `known.contains()` check (lines 170–185). -4. **Move `isValueTile()` to a shared location visible to both compilation units** - OR duplicate the lambda in `gggs_store_source.cpp`. Because the function is - tiny (one QRegularExpression match) and both files are in the same directory, - add it to `gggs_tile_layer.h` as an inline free function in the anonymous - implementation detail namespace — OR declare it in a new `gggs_tile_util.h` - if reviewers prefer. The simplest option is a static helper in a new - `gggs_tile_util.h` (one-line inline) included by both `.cpp` files. +4. **Share `isValueTile()` via the new `gggs_tile_util.h`** (resolved: shared + header, not a duplicated lambda) — included by both `gggs_tile_layer.cpp` and + `gggs_store_source.cpp` so the value-tile definition lives in exactly one + place, per the issue's "one place" requirement. 5. **Apply `isValueTile()` in `dirHasTifs()`** in `gggs_store_source.cpp` — filter after the `entryList` call so the helper sees only value tiles when deciding whether a directory holds a tile-set. -6. **Add a test** — extend `test_catalog_source.cpp` with a case that places - `0_0_0.tif`, `0_0_0_time.tif`, and `0_0_0_source.tif` in a store directory and - asserts `discover()` returns a leaf (the companions don't trip the tile-free - pruning path) and the leaf key points to the directory. Add a parallel case to - `test_gggs_rescan.cpp` (or a new `test_gggs_companion_filter.cpp`) that creates - a tile directory with base + companion files, constructs a `GggsTileLayer`, and - asserts `valid() == true` with exactly one tile (not three). +6. **Extend the existing tests** (resolved: extend, no new test file). In + `test_catalog_source.cpp` add a case that places `0_0_0.tif`, `0_0_0_time.tif`, + and `0_0_0_source.tif` in a store directory and asserts `discover()` returns a + leaf keyed to the directory, plus a negative case that a directory holding ONLY + companions is not a tile-set (pruned). In `test_gggs_rescan.cpp` add a case + that constructs a `GggsTileLayer` over a directory with base + companions + (companions written at a disjoint eastward extent) and asserts `valid()==true` + with `sceneBounds()` equal to a base-only layer's bounds (companions excluded — + only one tile loaded), plus a case that companions landing after load make + `rescan()` no-op (`false`, extent untouched). ## Files to Change @@ -63,16 +67,16 @@ depth/intensity tiles, producing garbage renders. | `src/camp2/raster/gggs_tile_util.h` | New header: `isValueTile(const QString&)` inline function | | `src/camp2/raster/gggs_tile_layer.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `loadDirectory()` and `rescan()` | | `src/camp2/raster/gggs_store_source.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `dirHasTifs()` | -| `test/test_gggs_companion_filter.cpp` | New test: store dir with base + `_time` + `_source` companions; assert only base tile is enumerated by `GggsTileLayer` and by `GggsStoreSource::discover()` | -| `CMakeLists.txt` | Register `test_gggs_companion_filter` with `ament_add_gtest` (same pattern as `test_catalog_source` / `test_gggs_rescan`) | +| `test/test_catalog_source.cpp` | Add: tile-set dir with base + `_time` + `_source` companions discovers as a leaf; companions-only dir is not a tile-set | +| `test/test_gggs_rescan.cpp` | Add: `GggsTileLayer` over base + companions loads only the base tile; companions landing after load make `rescan()` no-op | ## Principles Self-Check | Principle | Consideration | |---|---| -| A change includes its consequences | New test file + CMakeLists registration included in this plan. | -| Test what breaks | Test explicitly covers the bug path: companions in directory → only base tile enumerated. | -| Only what's needed | Narrow 3-site fix + shared helper + one test; no scope beyond issue description. | +| A change includes its consequences | Companion-filter cases added to the existing `test_catalog_source.cpp` and `test_gggs_rescan.cpp` targets (no new target/CMakeLists change). | +| Test what breaks | Tests explicitly cover the bug path: companions in directory → only base tile enumerated. | +| Only what's needed | Narrow 3-site fix + shared helper + extended tests; no scope beyond issue description. | | Capture decisions, not just implementations | Positive-pattern vs. denylist choice documented via inline comment in `gggs_tile_util.h`. | | Improve incrementally | Single PR, no architectural change. | @@ -82,7 +86,7 @@ depth/intensity tiles, producing garbage renders. |---|---|---| | ADR-0001 (Adopt ADRs) | No | Pattern choice (positive vs. denylist) is a code-comment decision, not ADR-worthy. | | ADR-0002 (Worktree isolation) | Yes | Worktree `issue-camp-112` already in place; all changes land here. | -| ADR-0005 (Layered enforcement) | Context | `dirHasTifs()` in `gggs_store_source.cpp` is the post-#104 successor to `GggsStoreLayer`'s subtree scan; the filter applies identically. | +| ADR-0005 (Catalog browser + flat selectable display layers) | Context | `dirHasTifs()` in `gggs_store_source.cpp` is the post-#104 successor to `GggsStoreLayer`'s subtree scan; the filter applies identically. | | ADR-0008 (ROS 2 conventions) | Yes | C++ changes in a ROS 2 ament_cmake package; no convention violations. | | ADR-0013 (progress.md vocabulary) | Yes | `progress.md` has an `## Issue Review` entry; `## Plan Authored` will be appended. | @@ -90,22 +94,20 @@ depth/intensity tiles, producing garbage renders. | If we change... | Also update... | Included in plan? | |---|---|---| -| `dirHasTifs()` in `gggs_store_source.cpp` | `test_catalog_source.cpp` (companion-in-dir case) | Yes — new test file covers both `GggsTileLayer` and `GggsStoreSource` | -| `loadDirectory()` / `rescan()` glob filter | `test_gggs_rescan.cpp` or new test | Yes — `test_gggs_companion_filter.cpp` covers this | +| `dirHasTifs()` in `gggs_store_source.cpp` | `test_catalog_source.cpp` (companion-in-dir case) | Yes — companion + companions-only cases added | +| `loadDirectory()` / `rescan()` glob filter | `test_gggs_rescan.cpp` | Yes — load + rescan companion cases added | | Add `gggs_tile_util.h` | `CMakeLists.txt` (install headers if needed) | No install target needed — internal header, not exported | -## Open Questions - -- **Header placement**: `gggs_tile_util.h` (new shared header) vs. a static lambda - duplicated in each `.cpp` (avoids an extra file). The shared header is cleaner for - future companion types; the duplicate lambda is simpler if no other callers are - expected. Recommendation: shared header, given the issue's explicit callout that - the excluded-suffix list must live in one place. -- **New test file vs. extending existing tests**: `test_gggs_companion_filter.cpp` - (dedicated) vs. adding cases to `test_gggs_rescan.cpp` and `test_catalog_source.cpp`. - Recommendation: add to existing tests where the fixture already covers the relevant - class, rather than creating a new file — unless the test logic is orthogonal enough - to warrant its own setup. +## Open Questions (resolved) + +- **Header placement** — RESOLVED: shared header `src/camp2/raster/gggs_tile_util.h` + with an inline `isValueTile()`, included by both `gggs_tile_layer.cpp` and + `gggs_store_source.cpp`. The excluded-suffix logic lives in exactly one place, + per the issue's explicit callout. +- **New test file vs. extending existing tests** — RESOLVED: extend the existing + `test_catalog_source.cpp` and `test_gggs_rescan.cpp` targets (their `touchTif`/ + `writeTile` fixtures already cover the relevant classes); no new test file or + CMakeLists change. ## Estimated Scope diff --git a/src/camp2/raster/gggs_store_source.cpp b/src/camp2/raster/gggs_store_source.cpp index b7c92e4..c3d3a5f 100644 --- a/src/camp2/raster/gggs_store_source.cpp +++ b/src/camp2/raster/gggs_store_source.cpp @@ -1,6 +1,7 @@ #include "gggs_store_source.h" #include "gggs_tile_layer.h" +#include "gggs_tile_util.h" #include "../catalog/catalog_item.h" #include "../map/layer_list.h" @@ -16,12 +17,19 @@ namespace raster namespace { -// A directory holds a tile-set if it contains GeoTIFF tiles directly. Salvaged -// from the retired GggsStoreLayer's anonymous-namespace helper. +// A directory holds a tile-set if it contains a base value GeoTIFF tile +// directly. Salvaged from the retired GggsStoreLayer's anonymous-namespace +// helper. [camp#112] The `*.tif` glob also matches `_time`/`_source` companion +// tiles, so a directory of companions alone is NOT a tile-set — filter to base +// value tiles (isValueTile) before deciding. bool dirHasTifs(const QString& path) { - return !QDir(path).entryList(QStringList() << "*.tif" << "*.tiff", - QDir::Files).isEmpty(); + const QStringList tifs = QDir(path).entryList( + QStringList() << "*.tif" << "*.tiff", QDir::Files); + for(const QString& name : tifs) + if(isValueTile(name)) + return true; + return false; } // Build a CatalogItem subtree for `path`: a leaf if it holds tiles directly, a diff --git a/src/camp2/raster/gggs_tile_layer.cpp b/src/camp2/raster/gggs_tile_layer.cpp index 41bcf78..9a2050d 100644 --- a/src/camp2/raster/gggs_tile_layer.cpp +++ b/src/camp2/raster/gggs_tile_layer.cpp @@ -1,6 +1,7 @@ #include "gggs_tile_layer.h" #include "gggs_tile.h" +#include "gggs_tile_util.h" #include "../map_view/web_mercator.h" #include @@ -124,6 +125,10 @@ void GggsTileLayer::loadDirectory(const QString& directory) bool first_extent = true; // first geometrically-valid tile (scene_bounds_) for(const QString& name : files) { + // [camp#112] Skip companion tiles (`_time`/`_source`): the `*.tif` glob also + // matches them, but only the base value tile is renderable. + if(!isValueTile(name)) + continue; // [camp#102] Extent/metadata only — the GggsTile ctor no longer reads pixels. // boundingRect()/sceneBounds() are valid immediately (fit-to-extent works at // load time); the band reads (and therefore the data range) are deferred to @@ -172,6 +177,10 @@ bool GggsTileLayer::rescan() std::vector> new_tiles; for(const QString& name : files) { + // [camp#112] Skip companion tiles (`_time`/`_source`) before the known check + // so a rescan never adds them as renderable tiles. + if(!isValueTile(name)) + continue; const QString path = dir.filePath(name); if(known.contains(path)) continue; diff --git a/src/camp2/raster/gggs_tile_util.h b/src/camp2/raster/gggs_tile_util.h new file mode 100644 index 0000000..0dd55a2 --- /dev/null +++ b/src/camp2/raster/gggs_tile_util.h @@ -0,0 +1,31 @@ +#ifndef RASTER_GGGS_TILE_UTIL_H +#define RASTER_GGGS_TILE_UTIL_H + +#include +#include + +namespace camp +{ +namespace raster +{ + +/// [camp#112] True iff @p filename is a base value tile `__.tif` +/// (or `.tiff`). The positive, anchored full-match pattern is the canonical +/// value-tile name shared with the stores' own tile-naming convention: exactly +/// three underscore-separated digit groups then `.tif`/`.tiff`. Companion tiles +/// (`___time.tif` Int64, `___source.tif` +/// uint16) carry a 4th `_time`/`_source` component and so fail the match — and +/// any future companion suffix is excluded for free by not matching, without a +/// denylist to maintain. Match @p filename only (what QDir::entryList returns), +/// never a full path. +inline bool isValueTile(const QString& filename) +{ + static const QRegularExpression re( + QRegularExpression::anchoredPattern(QStringLiteral("\\d+_\\d+_\\d+\\.tiff?"))); + return re.match(filename).hasMatch(); +} + +} // namespace raster +} // namespace camp + +#endif From 4e46fb1913425b37e69e78ecaf9d42f599f6c8ce Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:18:43 +0000 Subject: [PATCH 06/11] test(gggs): cover _time/_source companion-tile filtering (#112) Extend the existing targets (no new file/CMakeLists change): - test_catalog_source.cpp: a tile-set dir with base + _time + _source companions still discovers as a leaf; a dir of only companions is not a tile-set. - test_gggs_rescan.cpp: a GggsTileLayer over base + companions (written at a disjoint eastward extent) loads only the base tile (sceneBounds equals a base-only layer's); companions landing after load make rescan() no-op. Co-Authored-By: Claude Opus 4.8 --- test/test_catalog_source.cpp | 37 ++++++++++++++++++++++++ test/test_gggs_rescan.cpp | 56 ++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) diff --git a/test/test_catalog_source.cpp b/test/test_catalog_source.cpp index 1c07ce6..cb4cd33 100644 --- a/test/test_catalog_source.cpp +++ b/test/test_catalog_source.cpp @@ -106,6 +106,43 @@ TEST(CatalogSourceTest, RootThatIsATileSetIsALeaf) EXPECT_EQ(tree->key(), store.path()); } +// [camp#112] A tile-set directory that also holds `_time`/`_source` companion +// tiles still discovers as a leaf: the companions don't break discovery, and a +// dir whose only `*.tif` entries are companions is NOT a tile-set. +TEST(CatalogSourceTest, CompanionTilesDoNotBreakDiscovery) +{ + QTemporaryDir store; + ASSERT_TRUE(store.isValid()); + const QString root = store.path(); + + // A real tile-set: base value tile plus its Int64 `_time` and uint16 `_source` + // companions, all in the same directory (the bathy/MBES store layout). + const QString tileset = root + "/bathymetry/tileset"; + touchTif(tileset, "0_0_0.tif"); + touchTif(tileset, "0_0_0_time.tif"); + touchTif(tileset, "0_0_0_source.tif"); + + // A directory holding ONLY companions (no base value tile) is not a tile-set. + const QString companions_only = root + "/orphan_companions"; + touchTif(companions_only, "0_0_0_time.tif"); + touchTif(companions_only, "0_0_0_source.tif"); + + GggsStoreSource source; + std::unique_ptr tree = source.discover(root); + ASSERT_NE(tree, nullptr); + + // Only the real tile-set's modality survives; the companions-only branch prunes. + EXPECT_EQ(childNamed(tree.get(), "orphan_companions"), nullptr) + << "a dir of only companion tiles must not count as a tile-set"; + + const CatalogItem* bathy = childNamed(tree.get(), "bathymetry"); + ASSERT_NE(bathy, nullptr); + const CatalogItem* leaf = childNamed(bathy, "tileset"); + ASSERT_NE(leaf, nullptr); + EXPECT_TRUE(leaf->isLeaf()); + EXPECT_EQ(leaf->key(), tileset); +} + // A root with no tiles anywhere (and a non-existent path) yields no catalog. TEST(CatalogSourceTest, EmptyOrMissingRootYieldsNothing) { diff --git a/test/test_gggs_rescan.cpp b/test/test_gggs_rescan.cpp index 7c583a7..64b41b2 100644 --- a/test/test_gggs_rescan.cpp +++ b/test/test_gggs_rescan.cpp @@ -109,6 +109,62 @@ TEST(GggsRescanTest, PicksUpNewlyLandedTile) EXPECT_FALSE(layer->rescan()); // now nothing new } +// [camp#112] loadDirectory() filters companion tiles (`_time`/`_source`): a tile +// directory holding the base value tile plus its companions enumerates EXACTLY +// the base tile. The companions are written at a disjoint (eastward) extent, so +// if they were (wrongly) loaded as tiles the layer's sceneBounds would widen — +// the layer instead matches a base-only layer's extent exactly. +TEST(GggsRescanTest, CompanionTilesAreNotLoaded) +{ + QTemporaryDir base_only; + ASSERT_TRUE(base_only.isValid()); + ASSERT_FALSE(writeTile(base_only, "13_0_0.tif", -71.40, 43.00).isEmpty()); + + camp::map::Map base_map; + auto* base_layer = + new camp::raster::GggsTileLayer(base_map.topLevelLayers(), base_only.path()); + ASSERT_TRUE(base_layer->valid()); + const QRectF base_bounds = base_layer->sceneBounds(); + + QTemporaryDir with_companions; + ASSERT_TRUE(with_companions.isValid()); + ASSERT_FALSE(writeTile(with_companions, "13_0_0.tif", -71.40, 43.00).isEmpty()); + // Companions at a disjoint eastward extent: a real GeoTIFF, excluded by NAME. + ASSERT_FALSE(writeTile(with_companions, "13_0_0_time.tif", -71.39, 43.00).isEmpty()); + ASSERT_FALSE(writeTile(with_companions, "13_0_0_source.tif", -71.38, 43.00).isEmpty()); + + camp::map::Map map; + auto* layer = + new camp::raster::GggsTileLayer(map.topLevelLayers(), with_companions.path()); + ASSERT_TRUE(layer->valid()); + // Only the base tile counted: same extent as the base-only layer (companions' + // eastward extents did not widen it). + EXPECT_EQ(layer->sceneBounds(), base_bounds) + << "companion tiles must not be loaded as renderable tiles"; +} + +// [camp#112] rescan() filters companion tiles too: companions landing after the +// initial load are not treated as new renderable tiles, so rescan() no-ops +// (returns false) and leaves the extent untouched. +TEST(GggsRescanTest, RescanIgnoresCompanionTiles) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_FALSE(writeTile(dir, "13_0_0.tif", -71.40, 43.00).isEmpty()); + + camp::map::Map map; + auto* layer = new camp::raster::GggsTileLayer(map.topLevelLayers(), dir.path()); + ASSERT_TRUE(layer->valid()); + const QRectF bounds_before = layer->sceneBounds(); + + // Companions for the existing tile land afterward (disjoint eastward extents). + ASSERT_FALSE(writeTile(dir, "13_0_0_time.tif", -71.39, 43.00).isEmpty()); + ASSERT_FALSE(writeTile(dir, "13_0_0_source.tif", -71.38, 43.00).isEmpty()); + + EXPECT_FALSE(layer->rescan()); // companions are not new tiles + EXPECT_EQ(layer->sceneBounds(), bounds_before); // extent untouched +} + int main(int argc, char** argv) { qputenv("QT_QPA_PLATFORM", "offscreen"); From 37981cb175ed0d5261a0f27d3bda0730388f97cf Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:19:16 +0000 Subject: [PATCH 07/11] progress: implementation complete for #112 Co-Authored-By: Claude Opus 4.8 --- .agent/work-plans/issue-112/progress.md | 65 +++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md index 0b2bad5..26d8add 100644 --- a/.agent/work-plans/issue-112/progress.md +++ b/.agent/work-plans/issue-112/progress.md @@ -93,3 +93,68 @@ findings below are suggestions; none block implementation. - [ ] (suggestion) Spurious "ADR-0006-D7" citation — camp has no ADR-0006 (only 0001–0005); drop or correct, the "companion set may grow" rationale stands alone — `plan.md:28` - [ ] (suggestion) Specify anchored/full regex match (`anchoredPattern`/`^…$`) for `isValueTile()` so partial matches can't admit malformed names — `plan.md:27` - [ ] (suggestion) Resolve test-structure inconsistency: "Files to Change" adds new `test_gggs_companion_filter.cpp` while Approach/Open-Questions recommend extending existing `test_catalog_source.cpp`/`test_gggs_rescan.cpp` (which already have reusable fixtures) — pick one; extending is recommended — `plan.md:56,66,104` + +## Implementation +**Status**: complete +**When**: 2026-06-21 17:30 +00:00 +**By**: Claude Opus + +**Branch**: feature/issue-112 at `4e46fb1` +**Commits**: +- `6f4070d` fix(gggs): filter `_time`/`_source` companion tiles from value-tile scans — helper + 3 sites + plan sync +- `4e46fb1` test(gggs): cover companion-tile filtering — extend existing targets + +### What was built + +**Shared helper** — new `src/camp2/raster/gggs_tile_util.h` with an inline +`camp::raster::isValueTile(const QString& filename)`. It matches the filename +(never a full path) against an anchored, full-match positive pattern +`^\d+_\d+_\d+\.tiff?$` via `QRegularExpression::anchoredPattern(...)` with a +function-local `static const QRegularExpression` (compiled once). Exactly three +underscore-separated digit groups then `.tif`/`.tiff` → the base value tile; +companions (a 4th `_time`/`_source` component) and malformed names fail to +match. A header comment records the positive-pattern decision and that future +companion suffixes are excluded for free. + +**Three patched scan sites** (each now skips non-value tiles), both `.cpp`s +include `gggs_tile_util.h`: +- `gggs_tile_layer.cpp` `loadDirectory()` — `if(!isValueTile(name)) continue;` + before constructing the `GggsTile`. +- `gggs_tile_layer.cpp` `rescan()` — same skip before the `known.contains()` + check, so a rescan never adds a companion as a new tile. +- `gggs_store_source.cpp` `dirHasTifs()` — iterate the `*.tif` entryList and + return true only if at least one entry `isValueTile()`, so a directory of + companions alone is not counted as a tile-set. + +**Tests** (extended existing targets — no new file, no CMakeLists change): +- `test_catalog_source.cpp` → `CompanionTilesDoNotBreakDiscovery`: a tile-set + dir with `0_0_0.tif` + `0_0_0_time.tif` + `0_0_0_source.tif` discovers as a + leaf keyed to the dir; a sibling dir holding ONLY companions is pruned (not a + tile-set). +- `test_gggs_rescan.cpp` → `CompanionTilesAreNotLoaded`: a `GggsTileLayer` over + base + companions (companions written at a disjoint *eastward* extent, so they + would widen `sceneBounds()` if loaded) has `valid()==true` and `sceneBounds()` + equal to a base-only layer's — exactly one tile loaded. +- `test_gggs_rescan.cpp` → `RescanIgnoresCompanionTiles`: companions landing + after the initial load make `rescan()` return `false` with the extent + untouched. +- No tile-count accessor was added; the existing public `sceneBounds()`/`valid()` + surface (with disjoint-extent companions) distinguishes one-tile from + three-tile loads, per the plan. + +### Doc fixes (Plan Review findings, applied to `plan.md`) +- ADR-0005 retitled "Catalog browser + flat selectable display layers". +- Spurious "ADR-0006-D7" citation removed (camp has no ADR-0006). +- `isValueTile()` spec updated to anchored/full-match + filename-only. +- Both Open Questions resolved (shared header; extend existing tests); Files-to- + Change / Consequences tables updated to drop the new-test-file + CMakeLists rows. + +### Build / test +Camp deps were absent (empty `core_ws/install`): built `marine_ais_msgs`, +`marine_interfaces`, `marine_autonomy` via `colcon build --packages-up-to` in +`core_ws` first (warnings only). Then: +- Build: `./ui_ws/build.sh camp` → 1 package finished (pre-existing warnings only). +- Test: `./ui_ws/test.sh camp` → **99 tests, 0 errors, 0 failures, 2 skipped**. +The three new cases were confirmed run and passing (direct binary runs). + +Not pushed. From 172aff5d174026a1519be6a735213b7e88eca3b2 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:29:59 +0000 Subject: [PATCH 08/11] progress: local review for #112 --- .agent/work-plans/issue-112/progress.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md index 26d8add..1f35045 100644 --- a/.agent/work-plans/issue-112/progress.md +++ b/.agent/work-plans/issue-112/progress.md @@ -158,3 +158,25 @@ Camp deps were absent (empty `core_ws/install`): built `marine_ais_msgs`, The three new cases were confirmed run and passing (direct binary runs). Not pushed. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-21 16:29 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-112 at `37981cb` +**Mode**: pre-push +**Depth**: Standard (reason: 5 reviewable code files / ~149 lines; raster-render path) +**Must-fix**: 0 | **Suggestions**: 2 +**Round**: 1 | **Ship**: recommended — no must-fix; only optional case-sensitivity/test-coverage suggestions + +### Findings +- [ ] (suggestion) Case mismatch: `*.tif`/`*.tiff` glob may admit `.TIF`/`.TIFF` but regex `\.tiff?` is case-sensitive → uppercase-ext value tile silently dropped; theoretical (producer emits lowercase), optional `CaseInsensitiveOption` — `src/camp2/raster/gggs_tile_util.h:24` +- [ ] (suggestion) No test for a 4-group name (`13_0_0_0.tif`); correctly rejected by the anchored regex but unexercised — `src/camp2/raster/gggs_tile_util.h:24` + +### Notes +- Two fresh-context Claude adversarial passes (Lens A logic / Lens B systemic) both verdict the fix correct: regex accepts all legitimate `__.tif` value tiles and rejects all companions; the three patched sites (`loadDirectory`, `rescan`, `dirHasTifs`) are the complete `*.tif` scan set (no missed site); `GggsTile` is constructed only at the two filtered layer sites; the async worker/ctor need no guard (companions never enter `tiles_`). The case-sensitivity suggestion was cross-pass confirmed. +- Static analysis: no actionable findings. `ament_cpplint` copyright/header-guard/C-cast hits dropped — camp does not enforce ament_cpplint (no CI/pre-commit/CMake hook), cast hits are on untouched lines, and the new header matches every sibling's guard style and no-copyright convention. +- Plan adherence: exact — files and approach match `plan.md` as written; no drift/scope creep. +- Doc-citation nit (not code): earlier lifecycle entries cite ADR-0008/ADR-0013, which are workspace ADRs; camp's own ADR set is 0001–0005. From c5b907a1b9c28b9787c35809889e5e391c67c6e3 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:51:57 +0000 Subject: [PATCH 09/11] fix(gggs): match value-tile extension case-insensitively (#112) --- src/camp2/raster/gggs_tile_util.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/camp2/raster/gggs_tile_util.h b/src/camp2/raster/gggs_tile_util.h index 0dd55a2..f8687e0 100644 --- a/src/camp2/raster/gggs_tile_util.h +++ b/src/camp2/raster/gggs_tile_util.h @@ -18,10 +18,17 @@ namespace raster /// any future companion suffix is excluded for free by not matching, without a /// denylist to maintain. Match @p filename only (what QDir::entryList returns), /// never a full path. +/// +/// [camp#112] Case-insensitive: the scan-site globs (`*.tif`/`*.tiff`) can admit +/// an upper/mixed-case extension (`.TIF`/`.TIFF`), so the pattern matches case- +/// insensitively too — otherwise such a value tile would pass the glob but be +/// silently dropped here. (Producers emit lowercase today; this keeps the filter +/// from diverging from the globs if that ever changes.) inline bool isValueTile(const QString& filename) { static const QRegularExpression re( - QRegularExpression::anchoredPattern(QStringLiteral("\\d+_\\d+_\\d+\\.tiff?"))); + QRegularExpression::anchoredPattern(QStringLiteral("\\d+_\\d+_\\d+\\.tiff?")), + QRegularExpression::CaseInsensitiveOption); return re.match(filename).hasMatch(); } From fce9fd8c81cf591e203cb2c0033182c5519e0767 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:52:04 +0000 Subject: [PATCH 10/11] =?UTF-8?q?test(gggs):=20pin=20isValueTile=20name=20?= =?UTF-8?q?grammar=20=E2=80=94=20reject=204-group=20names,=20accept=20uppe?= =?UTF-8?q?r-case=20ext=20(#112)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/test_gggs_tile.cpp | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/test_gggs_tile.cpp b/test/test_gggs_tile.cpp index 5942ad0..4378098 100644 --- a/test/test_gggs_tile.cpp +++ b/test/test_gggs_tile.cpp @@ -22,9 +22,11 @@ #include #include "raster/gggs_tile.h" +#include "raster/gggs_tile_util.h" #include "map_view/web_mercator.h" using camp::raster::GggsTile; +using camp::raster::isValueTile; namespace { @@ -256,6 +258,40 @@ TEST(GggsTileTest, GeoToMapMatchesWebMercator) } } +// [camp#112] isValueTile() is the single companion-filter predicate applied at +// every `*.tif` scan site. Pin its name grammar directly (the layer/discovery +// tests cover it behaviorally; this exercises the boundary cases cheaply): +// - exactly three underscore-separated digit groups + .tif/.tiff are accepted; +// - a 4-group name (a `_time`/`_source` companion, or an extra component like +// `13_0_0_0.tif`) is rejected by the anchored full match; +// - the extension match is case-insensitive (mirrors the `*.tif` glob, which +// can admit `.TIF`/`.TIFF`). +TEST(GggsTileUtilTest, IsValueTileNameGrammar) +{ + // Base value tiles: three digit groups, .tif or .tiff. + EXPECT_TRUE(isValueTile("0_0_0.tif")); + EXPECT_TRUE(isValueTile("13_10_20.tif")); + EXPECT_TRUE(isValueTile("13_10_20.tiff")); + + // Companions carry a 4th component -> rejected. + EXPECT_FALSE(isValueTile("13_10_20_time.tif")); + EXPECT_FALSE(isValueTile("13_10_20_source.tif")); + + // A 4-group all-digit name is also rejected by the anchored 3-group match. + EXPECT_FALSE(isValueTile("13_0_0_0.tif")); + + // Malformed / non-value names are rejected (anchored full match). + EXPECT_FALSE(isValueTile("13_0.tif")); // too few groups + EXPECT_FALSE(isValueTile("a_0_0.tif")); // non-digit group + EXPECT_FALSE(isValueTile("13_0_0.png")); // wrong extension + EXPECT_FALSE(isValueTile("x13_0_0.tif")); // leading junk (anchored) + EXPECT_FALSE(isValueTile("13_0_0.tif.bak")); // trailing junk (anchored) + + // Extension match is case-insensitive (the glob admits upper/mixed case). + EXPECT_TRUE(isValueTile("13_0_0.TIF")); + EXPECT_TRUE(isValueTile("13_0_0.Tiff")); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); From a332ca110e7abc9d87e3cc5772e5fee3b3dd5dc1 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 21 Jun 2026 16:52:35 +0000 Subject: [PATCH 11/11] progress: addressed review findings for #112 --- .agent/work-plans/issue-112/progress.md | 29 +++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md index 1f35045..cd7a388 100644 --- a/.agent/work-plans/issue-112/progress.md +++ b/.agent/work-plans/issue-112/progress.md @@ -172,11 +172,36 @@ Not pushed. **Round**: 1 | **Ship**: recommended — no must-fix; only optional case-sensitivity/test-coverage suggestions ### Findings -- [ ] (suggestion) Case mismatch: `*.tif`/`*.tiff` glob may admit `.TIF`/`.TIFF` but regex `\.tiff?` is case-sensitive → uppercase-ext value tile silently dropped; theoretical (producer emits lowercase), optional `CaseInsensitiveOption` — `src/camp2/raster/gggs_tile_util.h:24` -- [ ] (suggestion) No test for a 4-group name (`13_0_0_0.tif`); correctly rejected by the anchored regex but unexercised — `src/camp2/raster/gggs_tile_util.h:24` +- [x] (suggestion) Case mismatch: `*.tif`/`*.tiff` glob may admit `.TIF`/`.TIFF` but regex `\.tiff?` is case-sensitive → uppercase-ext value tile silently dropped; theoretical (producer emits lowercase), optional `CaseInsensitiveOption` — `src/camp2/raster/gggs_tile_util.h:24` +- [x] (suggestion) No test for a 4-group name (`13_0_0_0.tif`); correctly rejected by the anchored regex but unexercised — `src/camp2/raster/gggs_tile_util.h:24` ### Notes - Two fresh-context Claude adversarial passes (Lens A logic / Lens B systemic) both verdict the fix correct: regex accepts all legitimate `__.tif` value tiles and rejects all companions; the three patched sites (`loadDirectory`, `rescan`, `dirHasTifs`) are the complete `*.tif` scan set (no missed site); `GggsTile` is constructed only at the two filtered layer sites; the async worker/ctor need no guard (companions never enter `tiles_`). The case-sensitivity suggestion was cross-pass confirmed. - Static analysis: no actionable findings. `ament_cpplint` copyright/header-guard/C-cast hits dropped — camp does not enforce ament_cpplint (no CI/pre-commit/CMake hook), cast hits are on untouched lines, and the new header matches every sibling's guard style and no-copyright convention. - Plan adherence: exact — files and approach match `plan.md` as written; no drift/scope creep. - Doc-citation nit (not code): earlier lifecycle entries cite ADR-0008/ADR-0013, which are workspace ADRs; camp's own ADR set is 0001–0005. + +## Implementation +**Status**: complete +**When**: 2026-06-21 16:52 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Branch**: feature/issue-112 at `fce9fd8` +**Addressed**: `## Local Review (Pre-Push)` (When 2026-06-21 16:29 +00:00, branch `feature/issue-112` at `37981cb`) — both unchecked suggestions +**Commits**: `c5b907a`, `fce9fd8` + +### Actions +- [x] (suggestion) Make `isValueTile()` extension match case-insensitive so an upper/mixed-case-extension value tile that passes the `*.tif`/`*.tiff` glob isn't silently dropped — added `QRegularExpression::CaseInsensitiveOption` + a comment recording the glob-alignment rationale — `src/camp2/raster/gggs_tile_util.h:21` +- [x] (suggestion) Add a test exercising a 4-group name (`13_0_0_0.tif`) rejection — new `GggsTileUtilTest.IsValueTileNameGrammar` direct unit test in `test_gggs_tile.cpp` pins the predicate: accepts 3-group `.tif`/`.tiff` (incl. upper-case ext), rejects `_time`/`_source` companions, 4-group names, too-few-groups, non-digit groups, wrong/extra extension, and leading/trailing junk — `test/test_gggs_tile.cpp:259` + +### Build / test +- `test_gggs_tile.cpp` already builds with `${CMAKE_CURRENT_SOURCE_DIR}/src/camp2` on its include path and links `Qt5::Core`, so the header-only `isValueTile()` test needed **no CMakeLists change**. +- Built core_ws deps first (`marine_ais_msgs`, `marine_interfaces`, `marine_autonomy` via `colcon build --packages-up-to`), then `./ui_ws/build.sh camp` → 1 package finished (pre-existing warnings only). +- `./ui_ws/test.sh camp` → **100 tests, 0 errors, 0 failures, 2 skipped** (was 99; the new case is the +1). The new case was confirmed run+passing via a direct `--gtest_filter='GggsTileUtilTest.*'` run. + +Not pushed. + +### Next step +Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand off to a fresh-context sub-agent: + + .agent/scripts/dispatch_subagent.sh --mode in-process --issue 112 --skill review-code