Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 37 additions & 21 deletions .claude/commands/rockout.md
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,23 @@ review is the audit trail.

## Step 10 -- Follow Up on Review Findings

Treat the review output as expert input, not a verdict. The reviewer is
another LLM running a checklist -- it catches real issues, but it also
flags false positives, misreads context, and occasionally invents
problems. Your job is to weigh each finding against the actual code, not
to mechanically apply every suggestion.
Treat the review output as expert input. The reviewer is another LLM
running a checklist -- it catches real issues but occasionally misreads
context or invents problems. Your default disposition is **fix it**.
Deferral and dismissal are exceptions that require justification, not
the easy path.

**Default to fixing.** If a finding describes a real problem and the
fix is a reasonable size (typically anything that can be done in the
current session without expanding the PR's scope by more than ~50% or
pulling in unrelated subsystems), fix it now in this PR. Do not defer
work just because it is slightly more effort than the original change.
Suggestions and Nits in particular should be applied unless you have a
concrete reason not to -- "the PR already works" is not a reason.

Address every Blocker first, then work through Suggestions and Nits in
that order.
that order. Treat Suggestions and Nits as work to be done, not
optional polish.

1. For each finding:
- Read the referenced file at the cited line and understand the
Expand All @@ -209,19 +218,24 @@ that order.
misread the code, the cited line does not exist, or the
"issue" is actually intended behavior, mark it **dismissed**
and record the reason -- do not fix phantom bugs.
- For Blockers: take them seriously, but they are not automatic.
Confirm the issue is real before fixing. Real blockers must be
either fixed or explicitly deferred with a written justification --
do not silently skip them. A blocker you genuinely believe is
wrong can be dismissed, but the dismissal note must explain why
the reviewer was mistaken.
- For Suggestions: consider each one seriously. Apply when the
change clearly improves correctness, clarity, or performance.
Dismiss when the suggestion conflicts with project conventions,
would regress something else, or the cost outweighs the value.
- For Nits: apply when trivially cheap and clearly an improvement.
Dismiss when stylistic preference, churn for churn's sake, or
not aligned with how the rest of the codebase is written.
- For Blockers: fix unless you can demonstrate the reviewer was
wrong. Deferral is not an option for Blockers -- either fix or
dismiss with a clear written explanation of the reviewer error.
- For Suggestions: **fix by default.** Apply the change unless it
conflicts with project conventions, would regress something else,
or the work would substantially exceed the original PR's scope.
A suggestion that takes a few edits and a test run is "reasonable
size" -- do it. Do not dismiss with vague rationales like "out of
scope" or "can be a follow-up" when the change fits in this PR.
- For Nits: **fix by default.** Apply the change unless it is purely
stylistic preference that conflicts with surrounding code. Nits
are cheap; the cost of leaving them is reviewer fatigue on the
next pass. Do not dismiss a nit just because it is a nit.
- Deferral to a follow-up issue is only appropriate when the fix
genuinely cannot fit in this PR -- e.g. it requires a separate
design decision, touches an unrelated subsystem, or would more
than roughly double the diff. When deferring, file a follow-up
issue with `gh issue create` and link it in the summary.
- In all cases, record the reason for dismiss / defer so the
summary captures the reasoning, not just the verdict.
2. Group related fixes into focused commits referencing the issue number
Expand All @@ -234,8 +248,10 @@ that order.
(`gh pr review <PR_NUMBER> --comment --body ...`). Stop iterating once
only dismissed-with-reason items remain.
5. Summarize the disposition of each original finding (fixed / deferred /
dismissed, with the reason for dismissals) in the final rockout summary
so the trail is visible.
dismissed, with the reason for dismissals or deferrals) in the final
rockout summary so the trail is visible. If the fixed count is low
relative to the total findings, the summary should explain why --
the expectation is that most findings get fixed in-PR.

**Do not skip this step.** Even if Step 9 returned no Blockers,
Suggestions, or Nits, the review of type `COMMENTED` from step 9.5 must
Expand Down
2 changes: 1 addition & 1 deletion .claude/sweep-metadata-state.csv
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
module,last_inspected,issue,severity_max,categories_found,notes
geotiff,2026-05-15,1909,HIGH,4;5,"Re-audit 2026-05-15 (agent-a55b69cec1ef2a092 worktree, branch deep-sweep-metadata-geotiff-2026-05-15). 4-backend (numpy/cupy/dask+numpy/dask+cupy) parity reverified after the #1813 modular refactor: full reads, windowed reads, multi-band, band=N selection, no-georef integer pixel coords, crs/crs_wkt/transform/nodata/x_resolution/y_resolution/resolution_unit/image_description/gdal_metadata all agree across backends. DataArray .name and dims agree (y, x for 2D; y, x, band for 3D). NEW HIGH finding #1909: GDS chunked GPU path (_read_geotiff_gpu_chunked_gds) declared the dask graph dtype as float64 when source had an in-range integer nodata sentinel, matching the CPU dask path's #1597 contract, but the per-chunk _chunk_task did not cast its returned cupy array to declared_dtype -- chunks with no sentinel hit returned the raw uint16/int16 source dtype, producing a silent declared/actual dtype mismatch. Fix mirrors the #1597 + #1624 CPU dask pattern: compute declared_dtype before defining _chunk_task, cast inside the task only when arr.dtype != declared_dtype to skip the no-op astype(copy=True). 6 regression tests added in test_chunked_gpu_declared_dtype_1909.py covering declared vs computed parity, CPU/GPU dask declared-dtype agreement, eager paths preserve source dtype, no-nodata round-trip, explicit dtype= kwarg, and sentinel-hit float64 promotion. Pre-existing test failures in test_predictor2_big_endian_gpu_1517.py and test_size_param_validation_gpu_vrt_1776.py exist on main (read_to_array AttributeError after #1813 refactor, tile_size=4 rejected by stricter _validate_tile_size_arg) and are unrelated to this audit."
geotiff,2026-05-18,1909,HIGH,4;5,"Re-audit 2026-05-15 (agent-a55b69cec1ef2a092 worktree, branch deep-sweep-metadata-geotiff-2026-05-15). 4-backend (numpy/cupy/dask+numpy/dask+cupy) parity reverified after the #1813 modular refactor: full reads, windowed reads, multi-band, band=N selection, no-georef integer pixel coords, crs/crs_wkt/transform/nodata/x_resolution/y_resolution/resolution_unit/image_description/gdal_metadata all agree across backends. DataArray .name and dims agree (y, x for 2D; y, x, band for 3D). NEW HIGH finding #1909: GDS chunked GPU path (_read_geotiff_gpu_chunked_gds) declared the dask graph dtype as float64 when source had an in-range integer nodata sentinel, matching the CPU dask path's #1597 contract, but the per-chunk _chunk_task did not cast its returned cupy array to declared_dtype -- chunks with no sentinel hit returned the raw uint16/int16 source dtype, producing a silent declared/actual dtype mismatch. Fix mirrors the #1597 + #1624 CPU dask pattern: compute declared_dtype before defining _chunk_task, cast inside the task only when arr.dtype != declared_dtype to skip the no-op astype(copy=True). 6 regression tests added in test_chunked_gpu_declared_dtype_1909.py covering declared vs computed parity, CPU/GPU dask declared-dtype agreement, eager paths preserve source dtype, no-nodata round-trip, explicit dtype= kwarg, and sentinel-hit float64 promotion. Pre-existing test failures in test_predictor2_big_endian_gpu_1517.py and test_size_param_validation_gpu_vrt_1776.py exist on main (read_to_array AttributeError after #1813 refactor, tile_size=4 rejected by stricter _validate_tile_size_arg) and are unrelated to this audit. | Re-audited 2026-05-18 (agent-a59a61958f181c31a worktree, branch deep-sweep-metadata-geotiff-2026-05-18). 4-backend (numpy / cupy / dask+numpy / dask+cupy) metadata parity reverified end-to-end: open_geotiff over a tiled uint16 fixture with crs + transform + GDAL_NODATA sentinel emits identical attrs across all 4 backends (crs=32633, crs_wkt, transform 6-tuple, nodata=5, masked_nodata=True, _xrspatial_geotiff_contract=2, extra_tags, image_description, resolution_unit, x_resolution, y_resolution). Multi-band 3D (y, x, band) with band coord, no-georef int64 pixel coords, windowed reads with transform origin shift, and mask_nodata=False keeping integer dtype all agree across the 4 backends. Write round-trip via to_geotiff (numpy, cupy, dask streaming) re-emits crs / transform / nodata / masked_nodata / contract version with byte-stable transform. Band-first (band, y, x) input correctly remaps to (y, x, band) on disk. _populate_attrs_from_geo_info, _set_nodata_attrs, and _extract_rich_tags centralise attrs emission across all read paths (_init_, _backends/dask, _backends/gpu, _backends/vrt) and write paths (_writers/eager, _writers/gpu, _writers/vrt). _ATTRS_CONTRACT_VERSION=2 is stamped on every path including the chunked GPU GDS and chunked VRT inline-attrs branches. No new CRITICAL/HIGH/MEDIUM/LOW findings."
rasterize,2026-05-17,2018,HIGH,1;2;4,"rasterize() drops like.attrs, rebuilds like.coords via linspace (not bit-identical), and never emits _FillValue/nodatavals even when fill is non-NaN. Cat 1 HIGH: chained pipelines like slope(rasterize(gdf, like=elevation)) silently lose crs/res/transform. Cat 2 MEDIUM: linspace round-trip from re-derived bounds breaks xr.align with like. Cat 4 MEDIUM: rasterize(..., fill=-9999, dtype=int32) emits no _FillValue. All 4 backends share the same final return so the fix is one place. Fixed in deep-sweep-metadata-rasterize-2026-05-17-01 (worktree agent-ab7a9aee97c1e4cdf): _extract_grid_from_like now returns coords/attrs; rasterize() reuses like.coords directly when grid matches, copies like.attrs, and emits _FillValue + nodatavals when fill is not NaN. 9 new tests in TestMetadataPropagation cover attrs propagation, bit-identical coord reuse, fill-value emission, isolation from template attrs, and parity across numpy/cupy/dask+numpy/dask+cupy backends. Full test suite (193 passing) clean."
reproject,2026-05-10,1572;1573,HIGH,1;3;4,geoid_height_raster dropped input attrs and used dims[-2:] for 3D inputs (#1572). reproject/merge ignored nodatavals (rasterio convention) when rioxarray absent (#1573). Fixed in same branch.
Loading