From 49b6c09f730ab1f1f7cf023db09597ce8fd73517 Mon Sep 17 00:00:00 2001 From: Brendan Collins Date: Tue, 19 May 2026 07:04:04 -0700 Subject: [PATCH] updated the rockout cmd --- .claude/commands/rockout.md | 58 ++++++++++++++++++++------------ .claude/sweep-metadata-state.csv | 2 +- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/.claude/commands/rockout.md b/.claude/commands/rockout.md index c309b7e89..05fdc3c97 100644 --- a/.claude/commands/rockout.md +++ b/.claude/commands/rockout.md @@ -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 @@ -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 @@ -234,8 +248,10 @@ that order. (`gh pr review --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 diff --git a/.claude/sweep-metadata-state.csv b/.claude/sweep-metadata-state.csv index 7ad7a401c..3160c2fdb 100644 --- a/.claude/sweep-metadata-state.csv +++ b/.claude/sweep-metadata-state.csv @@ -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.