PR4: activate CI, OIDC publish, User Guide#8
Merged
Conversation
CI (.github/workflows/run-tests.yml) - Re-enable pull_request / push / schedule / workflow_dispatch triggers. - Lint job runs ruff check, mypy, sphinx -W, pymarkdown, pip-audit on the project's declared deps. Python pinned to 3.10 to match target-version = "py310". - Test job re-enabled with matrix 3.10 / 3.11 / 3.12 / 3.13 and the existing codecov upload (3.13 only, non-fork). Publish workflows (publish_to_pypi.yml, publish_to_test_pypi.yml) - Switch to PyPI Trusted Publishers / OIDC: add permissions.id-token: write, drop password / user secrets, bind each job to its environment (pypi / testpypi). A pre-merge manual step on the PyPI side is required (documented in CONTRIBUTING.md). Coverage (pyproject.toml) - Ratchet [tool.coverage.report].fail_under from 0 to 59 (floor of the 59.24% measured against the full pytest run on the post-PR-3 codebase). User Guide (docs/user_guide.rst + docs/_static/user_guide/*) - New top-level RST page wired into docs/index.rst toctree with all 12 required sections: overview, installation, quick start, full CLI reference (one table per --help group), input-format cascade, per-instrument tints (Cassini, Voyager, Galileo, HST, NH MVIC), output formats, enhancement / geometry pipelines, --versions form, programmatic usage, troubleshooting. - 23 example thumbnails generated under docs/_static/user_guide/ by tests/fixture_recipes/generate_user_guide_thumbnails.py, modelled on PR 2's generate_snapshots.py so the gallery stays reproducible. - docs/conf.py enables html_static_path so the gallery is copied into the Sphinx build. CLI / doc drift gate (tests/test_cli.py) - New test_user_guide_documents_every_cli_flag asserts every long flag in tests/fixtures/.baseline-flags.txt appears in the User Guide. A new picmaker flag with no docs now fails CI. CONTRIBUTING.md - "Releasing to PyPI" section with the five-step OIDC trusted-publisher setup (PyPI + TestPyPI + GitHub Environments + tag-push release). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRaises Python to 3.11, migrates PyPI/TestPyPI publishing to OIDC trusted publishers, expands CI matrix and linting, centralizes option validation via PicmakerOptions, refactors I/O (ReadResult and PDS3 handling) and pipeline filter rename, rewrites TIFF16 read/write, hardens color/PIL utilities, expands Sphinx docs, and adds broad tests. ChangesUnified project-wide update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user_guide.rst`:
- Around line 279-283: Replace British spelling "colour/coloured" with American
"color/colored" in the user-facing docs for the pad options and any related
descriptions: update the block documenting ``--pad-color``, the ``pad_color``
field, the default ``'black'``, and the phrase "Padding fill colour (any X11
name from :class:`picmaker.colornames.ColorNames` or ``#RRGGBB``)" to use
"color" instead of "colour"; apply the same change at the other mentioned
occurrences (around lines 324-327, 397-408, 632-633) so all user-visible text
consistently uses American spelling.
- Around line 475-499: Replace inline code literals with Sphinx cross-reference
roles throughout the paragraph: change occurrences like pickle.load to
:func:`pickle.load`, numpy.load to :func:`numpy.load`, vicar.VicarImage to
:class:`vicar.VicarImage`, detect_vicar to :func:`detect_vicar`, detect_fits to
:func:`detect_fits`, astropy.io.fits.open to :func:`astropy.io.fits.open`,
PIL.Image.open to :func:`PIL.Image.open`, and rms-pdsparser to
:mod:`rms-pdsparser`; also convert attribute-like tokens such as ^IMAGE,
--pointer, and --alt-pointer to appropriate roles (e.g., :attr:`^IMAGE` or use
:option: if you have that role) and ensure returned tuple names (inst_host,
inst_id, filter_name) referenced in prose use :data: or :attr: roles as
appropriate so every API symbol in the narrative uses the correct Sphinx role.
In `@tests/fixture_recipes/generate_user_guide_thumbnails.py`:
- Around line 87-98: Update the docstring so the mention of the helper function
uses a Sphinx cross-reference: replace the plain `images_to_pics` text with a
function role such as `:func:\`images_to_pics\`` (or
`:func:\`module.images_to_pics\`` if fully qualified) in the docstring of the
thumbnail-rendering function so the reference is rendered and validated by
Sphinx.
- Around line 132-141: Add a descriptive docstring to the main() function that
briefly explains its purpose (generates thumbnails for fixtures in ALL_GALLERIES
into OUT_DIR), what it returns (exit code int), and any notable side effects
(creates OUT_DIR, prints progress and summary, calls _generate_one for each
gallery). Place the docstring as the first statement inside the main()
definition above the implementation so tooling and linters pick it up; reference
the function name main(), constants OUT_DIR and ALL_GALLERIES, and helper
_generate_one in the prose as appropriate.
- Around line 32-34: Rename the module-level constants HERE, FIXTURES, and
OUT_DIR to use the ALL_CAPS_WITH_UNDERSCORES convention for internal constants
(prefixing with an underscore as suggested): change HERE → _HERE, FIXTURES →
_FIXTURES, and OUT_DIR → _OUT_DIR; update all references to these symbols
throughout the file (e.g., usages like fixture = FIXTURES / fixture_name →
fixture = _FIXTURES / fixture_name, _OUT_DIR.mkdir(...), and the formatted
message referencing OUT_DIR → _OUT_DIR) so identifiers remain consistent.
- Line 22: Remove the unnecessary module-level import "from __future__ import
annotations" in tests/fixture_recipes/generate_user_guide_thumbnails.py — simply
delete that import line so annotations use native Python 3.10+ syntax; no other
changes required.
In `@tests/test_cli.py`:
- Line 69: The regex used to extract flags in the test (the expression assigned
to guide_flags via re.findall in tests/test_cli.py) is too restrictive because
r'--[a-z_-]+' excludes digits; update that pattern to allow digits (use
r'--[a-z0-9_-]+') so numeric flags like --16 would be matched in future
baselines—locate the guide_flags = set(re.findall(...)) line and replace the
pattern string accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f06d03d-5c6f-40d6-8664-9672c4ead6fa
⛔ Files ignored due to path filters (23)
docs/_static/user_guide/cassini_iss_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_colormap.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_default.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_gamma2.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_histogram.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_pct5_95.jpgis excluded by!**/*.jpgdocs/_static/user_guide/enhance_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/galileo_ssi_tint_a.jpgis excluded by!**/*.jpgdocs/_static/user_guide/galileo_ssi_tint_b.jpgis excluded by!**/*.jpgdocs/_static/user_guide/geom_default.jpgis excluded by!**/*.jpgdocs/_static/user_guide/geom_frame_max_50.jpgis excluded by!**/*.jpgdocs/_static/user_guide/geom_frame_pad.jpgis excluded by!**/*.jpgdocs/_static/user_guide/geom_rot90.jpgis excluded by!**/*.jpgdocs/_static/user_guide/geom_scale200.jpgis excluded by!**/*.jpgdocs/_static/user_guide/hst_acs_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/hst_wfc3_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/hst_wfpc2_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/nh_mvic_tint.jpgis excluded by!**/*.jpgdocs/_static/user_guide/output_jpg.jpgis excluded by!**/*.jpgdocs/_static/user_guide/output_png.pngis excluded by!**/*.pngdocs/_static/user_guide/output_tiff.tiffis excluded by!**/*.tiffdocs/_static/user_guide/output_tiff16.tiffis excluded by!**/*.tiffdocs/_static/user_guide/voyager_iss_tint.jpgis excluded by!**/*.jpg
📒 Files selected for processing (10)
.github/workflows/publish_to_pypi.yml.github/workflows/publish_to_test_pypi.yml.github/workflows/run-tests.ymlCONTRIBUTING.mddocs/conf.pydocs/index.rstdocs/user_guide.rstpyproject.tomltests/fixture_recipes/generate_user_guide_thumbnails.pytests/test_cli.py
Coverage gains by module - cli.py: 5% -> 91% (test_cli_unit.py — in-process tests for _build_parser, _separate_files_and_dirs, _normalize_and_validate, main()). - io.py: 50% -> 87% (test_io_branches.py + test_pds3_reader.py + the read_pds_labeled_image_array port to the current pdsparser API). - pipeline.py: 50% -> 87% (test_pipeline_branches.py — mutex checks, movie mode, --proceed, HST mosaic, --versions PDS3 label branch). - pil_utils.py: 66% -> 98% (test_misc_branches.py — 16-bit RGB path, PIL list handling, parent-dir creation). - enhance.py: 69% -> 90% (trim_zeros, footprint, below/above/invalid paints, named two-stop colormap, uniform-input limit fallbacks). - geometry.py: 70% -> 90% (wrap_ratio wide/tall, frame_max cap, rot90/180/270/fliptb, crop_array 2-D + uniform input). - instruments: per-module 67-83% -> 96-100% (parametrised Cassini filter chain, Voyager/Galileo/NH detect-fail predicates, HST POL0L/POL0S/FQUV/FQCH4 pinning paths). - tiff16.py: 86% -> 88% (RGB write, 3-D grayscale, big-endian byteorder, --up, transpose=ROTATE_90). - __init__.py: 71% -> 100% (pragma: no cover on the version-metadata fallback that only fires when the package isn't installed). Code change: src/picmaker/io.py::read_pds_labeled_image_array - Port to the current pdsparser API: the function previously referenced `pdsparser.PdsOffsetPointer` and `node.name` / `node.value`, which no longer exist (the function raised AttributeError on every PDS3 file in the test suite). The rewrite uses the dict-style `label[key]` API with the companion `<pname>_offset` / `<pname>_unit` keys that pdsparser now emits for tuple pointers. Pointer-value forms covered: attached int, detached str, tuple-with-filename. Unit is RECORDS by default; `<BYTES>` is recognised. Coverage gate - pyproject.toml: ratchet [tool.coverage.report].fail_under from 59 to 90. Total project coverage is now 90.31% across 466 tests (+204 over the previous baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop the `branches: [main]` filter from both `pull_request` and `push` triggers so the workflow fires on any branch. The schedule and workflow_dispatch entries are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
astropy 7.x requires Python >= 3.11, which collides with the
`requires-python = ">=3.10"` declaration and trips the resolver on
Python 3.10 ("Could not find a version that satisfies the requirement
astropy>=7.2.0"). Picmaker only uses `astropy.io.fits.open()` and the
standard HDU access surface, but the pin already captured 7.2.0 from
the dev venv and lowering it would be a step backwards on security
fixes.
pyproject.toml
- `requires-python = ">=3.11"` (was >=3.10).
- Drop `Programming Language :: Python :: 3.10` classifier.
- `[tool.ruff].target-version = "py311"` (was py310).
.github/workflows/run-tests.yml
- Lint job pinned to Python 3.11 to match target-version.
- Test matrix: drop 3.10; add macOS and Windows runners so the
cross-platform behaviour the README/classifiers promise is now
actually exercised. Matrix is 3.11 / 3.12 / 3.13 x
ubuntu / macos / windows (= 9 jobs).
CONTRIBUTING.md / docs/user_guide.rst
- Python 3.10+ -> Python 3.11+ in the compat copy.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/picmaker/__init__.py (1)
17-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow the version fallback exception scope.
Catching
Exceptionhere can hide legitimate import/runtime failures and incorrectly force__version__to0.0.0+unknown.As per coding guidelines, "Catch exceptions at the smallest granularity possible. Do NOT wrap large blocks in a single try/except."Proposed fix
try: from importlib.metadata import version as _version + from importlib.metadata import PackageNotFoundError __version__ = _version('rms-picmaker') -except Exception: # pragma: no cover — only fires when the package isn't installed +except PackageNotFoundError: # pragma: no cover — package metadata unavailable try: from picmaker._version import __version__ except ImportError: __version__ = '0.0.0+unknown'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/__init__.py` around lines 17 - 21, The current broad "except Exception" in src/picmaker/__init__.py should be narrowed so real runtime errors aren't swallowed; change the outer "except Exception" that surrounds the import of picmaker._version to only catch ImportError (or ModuleNotFoundError) and keep the existing fallback assignment to __version__ = '0.0.0+unknown' inside the ImportError handler; reference the import of "picmaker._version" and the variable "__version__" when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/picmaker/io.py`:
- Around line 309-315: The code constructs imagefile from untrusted label/node
values (in the node string and list/tuple branches) which permits path
traversal; update the logic in the node handling (where imagefile is set using
filename_str, node, and node[0] and offset_value is derived) to resolve and
normalize the joined path to an absolute path (use os.path.abspath +
os.path.normpath) and then verify it is inside the expected base directory (the
directory of filename_str) using os.path.commonpath (or equivalent) before
accepting it; if the resolved path escapes the base directory, treat it as
invalid (raise or skip) and do the same validation for both the str and
list/tuple branches where imagefile is computed.
In `@tests/test_pipeline_branches.py`:
- Line 125: Move the "import logging" statement out of the test functions and
place it with the module-level imports at the top of the file; then remove the
inline "import logging" lines from the test functions that currently include
them so the module uses the single top-level logging import instead of
per-function imports. Ensure any logging usage in those tests continues to
reference the same logging symbol.
- Line 6: Remove the unnecessary future import statement: delete the line "from
__future__ import annotations" from the test module since the codebase targets
Python 3.10+ and uses native generics (e.g., dict[str, Any], Path), so the
import is redundant per guidelines.
---
Outside diff comments:
In `@src/picmaker/__init__.py`:
- Around line 17-21: The current broad "except Exception" in
src/picmaker/__init__.py should be narrowed so real runtime errors aren't
swallowed; change the outer "except Exception" that surrounds the import of
picmaker._version to only catch ImportError (or ModuleNotFoundError) and keep
the existing fallback assignment to __version__ = '0.0.0+unknown' inside the
ImportError handler; reference the import of "picmaker._version" and the
variable "__version__" when making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d7819f3-3e28-4c0a-b593-be7207196f3c
📒 Files selected for processing (10)
.github/workflows/run-tests.ymlpyproject.tomlsrc/picmaker/__init__.pysrc/picmaker/io.pytests/test_cli_unit.pytests/test_io_branches.pytests/test_io_extra.pytests/test_misc_branches.pytests/test_pds3_reader.pytests/test_pipeline_branches.py
`pdsparser.Pds3Label` implements `__getitem__`, `__contains__`, `keys()`, `items()`, etc. but not `__iter__`. Python's old-style iteration protocol then falls back to `obj[0]`, `obj[1]`, …, which raises `KeyError: 0` at the first probe — that's why `for key in label:` crashes on a freshly constructed label. The class exposes `label.dict` directly (a plain `dict` assigned in `Pds3Label.__init__`), so iterating *it* is both clean and removes the `noqa: SIM118` we were carrying for the `label.keys()` workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Pds3Label` proxies every dict operation through to its underlying `.dict` attribute, which is a plain `dict`. Pull it out once into a local `label_dict` and use it directly — `label_dict[k]`, `label_dict.get(k, default)`, `pname in label_dict` — instead of going through the Pds3Label proxy for each access. No behaviour change, just clearer that the function is operating on a plain dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CONTRIBUTING.md is contributor-facing — release-engineering setup is maintainer-internal and doesn't belong there. Split the OIDC trusted- publisher steps into a new repo-root RELEASING.md (the conventional location for Python project release docs). Also fix the URLs while moving: a brand-new project that has never been uploaded has no per-project settings page on PyPI, so the previous instructions linked to a 404. The pending-publisher form at `pypi.org/manage/account/publishing/` (and the TestPyPI equivalent) is the right entry point for a first release; PyPI promotes the pending publisher to a regular one on the first successful upload. - CONTRIBUTING.md: drop the "Releasing to PyPI" section. - RELEASING.md: new file with the one-time-setup steps (using the pending-publisher URL) plus the cut-a-release checklist. - publish_to_pypi.yml / publish_to_test_pypi.yml: comment references CONTRIBUTING.md -> RELEASING.md. - scripts/run-all-checks.sh / run-tests.yml: include RELEASING.md in the pymarkdown scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the still-valid findings from the latest review and skip the rest with rationale. Applied - docs/user_guide.rst: replace British "colour"/"coloured" with American "color"/"colored" (9 occurrences) to match the rest of the user-facing docs. - tests/fixture_recipes/generate_user_guide_thumbnails.py: cross- reference :func:`picmaker.pipeline.images_to_pics` from the _generate_one docstring; add a docstring to main() covering purpose, side effects, and return value; drop the now-redundant `from __future__ import annotations` (requires-python is 3.11+ so every annotation syntax in this file is native). - tests/test_cli.py: widen the user-guide flag-extraction regex to `r'--[a-z0-9_-]+'` so numeric flags such as `--16` are matched when the baseline is regenerated to include them. Skipped - Sphinx role conversion for `vicar.VicarImage`, `astropy.io.fits.open`, `PIL.Image.open`, `detect_vicar`, `detect_fits`, `rms-pdsparser`, `^IMAGE`, `--pointer`, tuple field names. The project's `intersphinx_mapping` covers only python / numpy / matplotlib, so adding `:func:` / `:class:` roles for unresolvable symbols would emit reference-target-not-found warnings that the `-W` Sphinx build promotes to fatal errors. - Renaming HERE / FIXTURES / OUT_DIR to _HERE / _FIXTURES / _OUT_DIR. The file is a standalone script (not imported as a module) and the module-level ALL_GALLERIES already uses the all-caps non-underscored convention; splitting the convention adds noise without semantic benefit. The _generate_one helper is underscored because it is genuinely module-private. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`Test rms-picmaker (py3.11)` becomes `Test rms-picmaker (ubuntu-latest, 3.11)` so the matrix dimension is visible at a glance in the checks list (relevant now that the matrix runs on ubuntu/macos/windows). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`test_falls_through_to_unrecognized_format_error` hardcoded the POSIX path `/nonexistent/...`, which on Windows is not absolute. astropy's FITS reader inside `read_one_image_array`'s cascade rejects the path with "Local file URL is not absolute" before the cascade ends, so the test fails with the wrong exception type. Construct the non-existent path via `tmp_path / 'does_not_exist.IMG'` so the absolute form is OS-native — same idiom already used in `tests/test_io_branches.py::test_unrecognized_missing_file_raises`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drives `CODEBASE_CRITIQUE.md` and `TEST_SUITE_CRITIQUE.md` to "fixed" or "issue filed" on every item that didn't require a major-version bump. Twelve follow-up issues (#9-#20) carry the deferred work. ## Production fixes - pil_utils._one_pil_to_array now honours rescale=True for 'L' mode PIL images, returning a float in [0, 1] instead of the raw uint8 array. Closes #10. - tiff16:281: missing `raise` on the version-byte mismatch (the function silently accepted non-TIFF files with a valid 'II'/'MM' byte order but wrong version field). Regression test added. - tiff16 WriteTiff16 `palette != None` → `palette is not None` (numpy ambiguous-truth bug). Removed the two strict-xfail palette tests that pinned the bug. - ReadTiff16 + WriteTiff16 file-handle leaks: both functions now wrap the open() in a `with` block. Surfaced by adding filterwarnings = ["error"] to pytest (ResourceWarning was the smoking gun). - colornames.ColorNames.lookup uses ast.literal_eval (not `eval`), raises KeyError on partial regex matches, and no longer shadows the builtin `tuple` local. Two new tests pin the boundary. - io.read_one_image_array chains the cascade-end OSError from ExceptionGroup('No reader matched', cascade_errors) so per-reader failure causes survive for diagnosis. - pipeline.process_images: assert→raise ValueError for the movie-mode proceed-consistency check (assert is stripped under python -O). - pipeline.images_to_pics: traceback.print_tb → logger.exception (deterministic ordering under pytest -n auto). - pipeline.find_common_path: switched to os.path.commonpath so the Windows path-separator bug is gone. - cli.main: print(this_dir)/print(dirpath) → logger.info (matches the pipeline verbose path). - __init__.py: except Exception → except PackageNotFoundError on the importlib.metadata fallback. - io.get_outfile: suffix annotation widened to `str | None` to match the documented None-accepting behaviour. - _filters.filter_image docstring corrected to reflect the actual KeyError-on-unknown-name semantics. ## API changes - New picmaker.options.PicmakerOptions dataclass owns all post-normalization mutex / value-validity checks via validate(); cli._normalize_and_validate and pipeline.images_to_pics both delegate to it. The legacy kwargs surface on images_to_pics is unchanged. - New picmaker.io.ReadResult NamedTuple (array3d, default_is_up, filter_info) returned by the reader cascade. Positional unpacking still works; .array3d-style attribute access now also works. - New picmaker.io.FilterInfo type alias for the (inst_host, inst_id, filter_name) | None union. - pipeline.images_to_pics `filter=` kwarg renamed to `filter_name=`; the CLI --filter flag is unchanged (dest='filter_name'). The PDS3-label local `filter_name` was renamed to `pds_filter_name` so it no longer shadows the parameter. ## Ruff cleanups - Removed per-file ignores for colornames.py (was E701, E711, N801, W291, W293, RUF012, SIM102, A001) and most of tiff16.py (was E701, N802, N806, E501, E711, W291, W293, SIM115; only N802 remains for legacy WriteTiff16 / ReadTiff16 casing). - Removed A001/A002/A003 ignores for cli.py / pipeline.py / options.py once the `filter` → `filter_name` rename landed. - All remaining per-file ignores have an inline reason. ## Tooling - pytest filterwarnings = ["error"] (with one targeted Pillow `Image.getdata` ignore tracked in #9). - bandit and vulture uncommented in [project.optional-dependencies].dev and wired into CI; both clean (bandit suppresses B301/B403 for the documented pickle-fallback path). - publish_to_pypi.yml / publish_to_test_pypi.yml now build on Python 3.11 (lowest supported) instead of 3.12. - run-tests.yml push: trigger scoped to `branches: [main]` so feature branches don't pay for the 9-cell matrix on every dev push. - docs/module.rst gains a "Options" section for PicmakerOptions. ## Test-suite hygiene - Split tests/test_misc_branches.py (842 lines) and tests/test_io_branches.py (283 lines) into nine focused files (test_pil_utils_branches.py, test_enhance_branches.py, test_geometry_branches.py, test_filters_branches.py, test_instruments_branches.py, test_tiff16_branches.py, test_package_init.py, test_io_cascade.py, test_pds3_reader_branches.py). - Removed `from __future__ import annotations` from 24 files. - Migrated `from picmaker.picmaker import X` to `from picmaker import X` in 17 files (test_api_compat.py stays — it's the BC verifier). - Tightened existence-only asserts: test_io.py / test_pipeline.py / test_io_extra.py now check exact shapes instead of `.ndim == 3` or `width > 0`. - Added regression tests for the tiff16 version-byte fix and the colornames partial-match / injection-payload rejection. ## Final check status - pytest: 471 passed - ruff: clean - mypy strict: clean (79 source files) - bandit: clean - vulture: clean - sphinx -W: clean - All snapshot tests still byte-identical. ## Filed follow-up issues (with labels) - #9 Cleanup / Useful / Medium — Pillow getdata migration + WriteTiff16 leak follow-up - #10 Bug / Important / Easy — _one_pil_to_array rescale (FIXED HERE) - #11 Big Project / Important / Medium — Prune picmaker.picmaker BC shim exports - #12 Big Project / Important / Hard — Split images_to_pics + complete RORO migration - #13 Cleanup / Minor / Easy — Derive VICAR/FITS_INSTRUMENTS from one list - #14 Cleanup / Minor / Medium — Rename picmaker.io to avoid stdlib shadow - #15 Cleanup / Minor / Easy — Make instrument FILTER_DICT private - #16 Cleanup / Useful / Easy — Stop mutating argparse.Namespace - #17 Cleanup / Useful / Medium — Standardize on pathlib.Path - #18 Enhancement / Minor / Medium — Vectorize fill_zebra_stripes - #19 Enhancement / Useful / Easy — Expand CI lint matrix to 3.11/3.12/3.13 - #20 Enhancement / Useful / Medium — Magic-byte dispatch for read_one_image_array Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…inks
## New: docs/developer_guide.rst
A maintainer-facing complement to the User Guide. Eight sections:
1. Repository overview — purpose, layout, who reads what.
2. Module-by-module description — one paragraph per leaf module
under `src/picmaker/`, every public symbol linked via Sphinx
cross-reference.
3. Pipeline flowchart — Mermaid diagram from CLI entry through to
`write_pil`.
4. Major functions in prose — covers the CLI entry, option
validation, reader cascade, path planning, per-image pipeline,
enhancement / geometry / color / PIL bridges. Cross-refs all the
way through.
5. Running the test suite.
6. Adding a new instrument — full step-by-step:
- The four-function protocol (`detect_vicar`, `detect_fits`,
`matches`, `tint_for`).
- Per-step instructions: create module, register in
`instruments/__init__.py`, fixture recipe, cross-instrument
test, per-helper unit tests, snapshot append, user-guide
update, check-suite run.
- When-to-break-protocol notes covering Cassini's tint chain
helper and HST's wavelength-inference branches.
7. Releasing to PyPI — moved verbatim from RELEASING.md (one-time
trusted-publisher setup + cut-a-release steps).
8. Where to look next — pointers to user guide, module reference,
GitHub issues, critique audits, contributor workflow.
## docs/module.rst
Source-code links added at the top of each section, pointing at the
relevant file under github.com/SETI/rms-picmaker. New sections for
the previously-undocumented package surface:
- `picmaker` (top-level package)
- `picmaker.picmaker` (BC shim)
- `picmaker._rgb` (wavelength → RGB tables)
- `picmaker.instruments.*` (each per-mission module)
- `picmaker.tiff16`
- `picmaker.colornames`
CLI automodule gains `:private-members: _build_parser,
_separate_files_and_dirs, _normalize_and_validate` so the private
helpers exercised by `tests/test_cli_unit.py` appear in the rendered
API.
## docs/conf.py
- Intersphinx mappings for PIL, astropy, sphinx, and pytest so
external `:func:` / `:class:` / `:mod:` references resolve under
`sphinx-build -n`.
- `nitpick_ignore` for sibling-package types (`vicar.VicarImage`,
`vicar.VicarError`, `tabulation.Tabulation`), module-level dunders
(`__all__`, `__version__`), and a small set of internal data /
helper names that autodoc cannot pick up cleanly (legacy
`colornames.ColorNames`, the per-instrument `FILTER_DICT` /
`FILTER_NAMES` constants without docstrings, the `_rgb` splines,
the private `cli._normalize_and_validate`).
- `autodoc_default_options['show-inheritance'] = True`.
## RELEASING.md
Replaced with a five-line pointer to the new
`docs/developer_guide.rst` §7. CI workflow comments updated to point
at the new location. `pymarkdown scan` continues to lint the file.
## Source-side cross-reference fixes
To satisfy `sphinx-build -n -b html`, qualified bare names in
docstrings:
- `instruments/{galileo,nh,voyager,hst}.py`: `:data:`FILTER_DICT``
etc. → `:data:`picmaker.instruments.galileo.FILTER_DICT`` etc.
- `instruments/hst.py`: `:data:`RFUNC`` etc. →
`:data:`picmaker._rgb.RFUNC``.
- `_filters.py`: `:data:`FILTER_DICT`` →
`:data:`picmaker._filters.FILTER_DICT``.
Other doc-rule alignment:
- `tiff16.WriteTiff16` and `tiff16.ReadTiff16` docstrings converted
from the legacy `Inputs:` / `Return:` shape to Google style
(`Parameters:` / `Returns:`). Fixes the long-standing docutils
"Unexpected indentation" warning.
- American spelling pass: `colour` → `color`, `coloured` →
`colored`, `grey` (in prose) → `gray`, `honour(s)` → `honor(s)`,
`normalise(d)` → `normalize(d)`, `behaviour` → `behavior`,
`organis(ed)` → `organiz(ed)`. The X11 color-name table in
`colornames.py` keeps the legacy `grey` aliases (they are literal
X11 names, not English prose).
- `instruments/__init__.py`: `VICAR_INSTRUMENTS` / `FITS_INSTRUMENTS`
/ `ALL_INSTRUMENTS` gain `list[ModuleType]` annotations and
`#:`-style attribute docstrings so autodoc picks them up.
- `__init__.py` module docstring uses fully-qualified targets
(`picmaker.pipeline.images_to_pics`) so the references resolve
outside the `picmaker` namespace.
## Final check status
- pytest: 471 passed.
- ruff: clean.
- mypy strict: clean.
- bandit: clean.
- vulture: clean.
- `sphinx-build -W -b html`: clean.
- `sphinx-build -n -b html`: clean.
- pymarkdown: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/test_color.py (1)
57-59: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueStale "eval" reference in comment.
The production code uses :func:
ast.literal_eval, not :func:eval. Update the comment to avoid misleading future readers. If you adopt the tuple-normalization suggested incolornames.py, this test (and its comment) will need a corresponding update to assert a tuple instead.📝 Proposed wording fix (no behavior change)
- # Bracket form `[100, 100, 100]` parses but returns a list (eval result), - # not a tuple — documenting the actual current behavior. + # Bracket form `[100, 100, 100]` parses via ast.literal_eval and + # returns a list, not a tuple — documenting current behavior. assert ColorNames.lookup('[100, 100, 100]') == [100, 100, 100]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_color.py` around lines 57 - 59, The comment in tests referencing "eval" is stale; update the comment near the assertion calling ColorNames.lookup('[100, 100, 100]') to say the production code uses ast.literal_eval (not eval), and if you implement the tuple-normalization change in colornames.py (the tuple normalization behavior in ColorNames.lookup), also update the test assertion to expect a tuple instead of a list and adjust the comment to describe that tuple-normalization behavior.src/picmaker/colornames.py (1)
782-836:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a docstring to
lookupand tighten the return type.Two issues on the public API surface of :meth:
ColorNames.lookup:
- The method has no docstring. Per coding guidelines it must document parameters, return value, raised exceptions (
TypeError,KeyError,ValueError) and the accepted input forms — enough to write a black-box test from the docstring alone.- The annotated return type is
Any. Concretely, the dict path returnstuple[int, int, int], the parenthesized RGB form returnstuple[int, int, int](via :func:ast.literal_eval), but the bracket form[r, g, b]returnslist[int]— an inconsistency thattests/test_color.py::TestRgbExpression::test_rgb_brackets_expressionhad to document around. Normalizing to a tuple removes a real ergonomic wart for callers and lets the annotation become precise.♻️ Proposed fix
`@staticmethod` - def lookup(name: str) -> Any: + def lookup(name: str) -> tuple[int, int, int]: + """Resolve a color specifier to an ``(r, g, b)`` tuple. + + Accepts an X11 color name (case- and whitespace-insensitive, with + optional surrounding quotes) or a literal ``(r, g, b)`` / + ``[r, g, b]`` expression with components in ``[0, 255]``. + + Parameters: + name: Color specifier to resolve. + + Returns: + A 3-tuple of integer RGB components. + + Raises: + TypeError: If ``name`` is not a string. + KeyError: If ``name`` is not a recognized color name and is not + a well-formed RGB literal that fully consumes the input. + ValueError: If any parsed RGB component exceeds 255. + """ @@ - for i in rgb: - if i > 255: - raise ValueError("Color value out of range: " + name) - - return rgb + for i in rgb: + if i > 255: + raise ValueError("Color value out of range: " + name) + + return tuple(rgb)The corresponding test
test_rgb_brackets_expressionshould then assert== (100, 100, 100).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/colornames.py` around lines 782 - 836, Add a docstring to ColorNames.lookup describing the parameter (name: str), the return value (tuple[int,int,int]), and the raised exceptions (TypeError for non-str, KeyError for unknown names, ValueError for out-of-range components) plus the accepted input forms (named colors, lower/normalized variants, RGB tuple or bracketed list). Change the annotated return type from Any to tuple[int, int, int] and normalize any parsed RGB list to a 3-tuple before returning (e.g. convert the ast.literal_eval result or any list from RGB parsing into tuple(r,g,b)). Ensure the function still uses ColorNames.COLOR_NAME_DICT, ColorNames.COLOR_NAME_LOWER_DICT and ColorNames.RGB_PATTERN as currently to locate the code to modify.CODEBASE_CRITIQUE.md (1)
1-212: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor documentation quality issues in internal audit document.
This internal codebase critique has several minor formatting and spelling issues flagged by static analysis:
- Mixed British/American spelling: "modernization" (line 141) vs "modernisation" (line 178)
- GitHub should be capitalized in several places (lines 4, 168, 170, 171, 173)
- Several code blocks missing blank lines before/after (markdownlint MD031)
Since this is an internal audit document rather than user-facing documentation, these are low-priority, but consistency would improve readability.
As per coding guidelines: "Always use American spelling instead of British spelling (e.g., color instead of colour) in documentation."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CODEBASE_CRITIQUE.md` around lines 1 - 212, The audit document CODEBASE_CRITIQUE.md contains inconsistent British/American spelling ("modernisation" vs "modernization"), inconsistent capitalization of "GitHub", and missing blank lines around fenced/code blocks flagged by markdownlint MD031; edit CODEBASE_CRITIQUE.md to standardize to American spelling (e.g., change "modernisation" → "modernization"), capitalise all occurrences of "GitHub", and ensure there is a blank line before and after each code block (or fenced block) to satisfy MD031.src/picmaker/io.py (1)
178-243:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a context manager for FITS files to avoid resource leaks on exceptions.
hdulist.close()only runs on the success path. The code can raiseIndexError(line 181, 189, 206),OSError(line 230), or other exceptions before reaching the explicitclose()call, leaving the file handle open and causing resource leaks.Proposed fix
- hdulist = pyfits.open(filename_str) - _fitsobj = hdulist[0] # IndexError if not FITS + with pyfits.open(filename_str) as hdulist: + _fitsobj = hdulist[0] # IndexError if not FITS + ... - hdulist.close() - return ReadResult(array3d, True, filter_info) + return ReadResult(array3d, True, filter_info)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 178 - 243, The FITS file handle opened via pyfits.open(filename_str) (hdulist) can leak if exceptions occur before hdulist.close(); change the code to open the FITS file with a context manager (e.g., with pyfits.open(filename_str) as hdulist:) or wrap the existing logic in try/finally to ensure hdulist.close() always runs; apply this around the block that uses hdulist (the detection loop using instruments.FITS_INSTRUMENTS, the HST branches for 'ACS/WFC' and 'WFPC2', the obj handling and the final ReadResult return) so that exceptions like IndexError or OSError won’t leave the file open.
♻️ Duplicate comments (2)
src/picmaker/io.py (1)
349-355:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDetached PDS pointer filenames still allow directory traversal.
At Line 350 and Line 354, untrusted pointer paths are joined directly against the label directory without containment checks. This can escape via
../....As per coding guidelines, "For file paths, resolve to absolute paths and verify they remain within the expected directory (prevent path traversal)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 349 - 355, The code that sets imagefile in the node-handling branch (the string branch and the list/tuple branch treating node[0]) joins untrusted pointer values directly to the label directory using filename_str, allowing path traversal; fix by resolving the candidate path to an absolute/resolved path (use os.path.abspath or pathlib.Path(filename).resolve()) and compare it against the base directory resolved path (os.path.dirname(filename_str) resolved) using os.path.commonpath or Path.commonpath to ensure the resulting imagefile is contained within the base directory; if the containment check fails, reject the pointer (raise an error or skip) rather than using the escaped path, and preserve existing offset_value logic for valid paths (references: filename_str, pname, label_dict, imagefile, offset_value, node and node[0]).tests/fixture_recipes/generate_user_guide_thumbnails.py (1)
30-32: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse underscore prefix for internal module-level constants.
HERE,FIXTURES, andOUT_DIRare internal to this script and should follow the private naming convention. As per coding guidelines, names not part of the public API should be prefixed with a single underscore.♻️ Proposed fix
-HERE = Path(__file__).parent -FIXTURES = HERE.parent / 'fixtures' -OUT_DIR = HERE.parent.parent / 'docs' / '_static' / 'user_guide' +_HERE = Path(__file__).parent +_FIXTURES = _HERE.parent / 'fixtures' +_OUT_DIR = _HERE.parent.parent / 'docs' / '_static' / 'user_guide'Then update all references throughout the file:
- Line 98:
fixture = _FIXTURES / fixture_name- Line 142:
_OUT_DIR.mkdir(parents=True, exist_ok=True)- Line 145:
path = _generate_one(slug, fixture_name, kwargs, ext, _OUT_DIR)- Line 149:
print(f'...to {_OUT_DIR}')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixture_recipes/generate_user_guide_thumbnails.py` around lines 30 - 32, Rename the module-level constants HERE, FIXTURES, and OUT_DIR to use a single leading underscore (_HERE, _FIXTURES, _OUT_DIR) to mark them as internal, then update every use in this file accordingly (e.g., replace fixture = FIXTURES / fixture_name with fixture = _FIXTURES / fixture_name, call _OUT_DIR.mkdir(...), pass _OUT_DIR into _generate_one(...) and update print messages to reference _OUT_DIR); ensure imports/other symbols remain unchanged and run tests to confirm no remaining references to the old names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user_guide.rst`:
- Around line 457-459: The table documents the CLI option ``--filter`` as having
dest ``filter`` but the actual argparse binding uses ``filter_name``; update the
docs entry for the ``--filter``/``-f NAME`` option to show dest as
``filter_name`` (replace the current ``filter`` value), ensuring the table row
for the ``--filter`` option matches the actual binding name used in the codebase
(``filter_name``).
In `@src/picmaker/colornames.py`:
- Line 15: Add a descriptive class docstring to ColorNames that explains the
class exposes the standard X11 color-name table and provides the lookup entry
point (lookup), and describe supported input forms: color name (case-insensitive
/ lowercase normalized) and RGB containers like (r, g, b) or [r, g, b]; place
this docstring immediately under the class ColorNames declaration and keep it
concise but informative per project docstring guidelines.
In `@src/picmaker/instruments/galileo.py`:
- Around line 34-35: The docstring in src/picmaker/instruments/galileo.py
contains lines that exceed the 90-character wrap target (notably the long lines
around the block where FILTER index and the "GLL/SSI" prefix are described and
the similar lines at the later occurrence); split those long strings into
multiple shorter lines (<=90 chars) while preserving docstring indentation and
wording, updating both places (the docstring segment near the
FILTER/FILTER_NAMES description and the repeated lines at the later location) so
they remain valid Python docstrings and keep existing punctuation and backticks
intact.
In `@src/picmaker/instruments/hst.py`:
- Around line 87-88: The docstring incorrectly uses :data: for callable symbols;
change the Sphinx roles for RFUNC, GFUNC, and BFUNC to use :func: instead of
:data: (find the docstring referencing picmaker._rgb.RFUNC / picmaker._rgb.GFUNC
/ picmaker._rgb.BFUNC in src/picmaker/instruments/hst.py and replace each
:data:`picmaker._rgb.RFUNC` / :data:`picmaker._rgb.GFUNC` /
:data:`picmaker._rgb.BFUNC` with :func:`picmaker._rgb.RFUNC` /
:func:`picmaker._rgb.GFUNC` / :func:`picmaker._rgb.BFUNC` so they are proper
callable cross-references).
In `@src/picmaker/instruments/nh.py`:
- Around line 87-88: The docstring's "Raises:" entry for KeyError in
src/picmaker/instruments/nh.py exceeds the 90-character wrap limit; edit the
docstring (the Raises: KeyError line that mentions FILTER_DICT and inst_id/MVIC
path) to wrap the sentence to 90 characters or less (split into two properly
indented lines while preserving Sphinx/reST markup and references like
picmaker.instruments.nh.FILTER_DICT, filter_name, and inst_id).
In `@src/picmaker/instruments/voyager.py`:
- Around line 86-87: The docstring in src/picmaker/instruments/voyager.py has a
too-long Raises entry for KeyError; edit the docstring (near the
FILTER_DICT/inst_id explanation) to reflow the KeyError description to a maximum
line length of 90 characters, preserving wording and meaning but breaking the
sentence across lines to comply with the project's docstring wrapping rules
(ensure the “Raises:” section and the KeyError bullet remain correctly indented
and formatted).
In `@src/picmaker/io.py`:
- Around line 135-143: The current reader silently calls pickle.load on
filename_str and appends any exception to cascade_errors; remove this unsafe
default by gating pickle deserialization behind an explicit opt-in flag (e.g.,
add and check an allow_pickle boolean parameter to the public reader function)
or remove the pickle branch entirely; when gated, require callers to pass
allow_pickle=True and include a documented security warning before invoking
pickle.load, and keep the rest of the flow (constructing ReadResult and
appending to cascade_errors) unchanged so callers that do opt-in still get the
same return semantics.
In `@src/picmaker/options.py`:
- Around line 87-113: The validate method contains a redundant None check for
filter_name: since filter_name is declared as str with default 'NONE', remove
the unnecessary "is not None" branch and directly check the value when twobytes
is true; i.e., in validate(), under the twobytes block use filter_name.lower()
!= 'none' (and keep the existing extension check and error message), referencing
the validate method, the twobytes field, and filter_name to locate and simplify
the conditional.
In `@src/picmaker/pipeline.py`:
- Around line 100-110: When movie mode is enabled the code indexes
option_dicts[0] without ensuring the list is non-empty; add a guard before the
any(...) check to raise a clear ValueError if option_dicts is empty (e.g.,
"movie mode requires at least one option_dict") and only perform the
proceed-equality check afterwards—update the branch that currently references
option_dicts[0] so it first validates option_dicts is truthy, then runs the
existing any(...) comparison and raises the existing ValueError for mismatched
'proceed' values.
- Around line 58-72: The root-only check after calling os.path.commonpath
(variable result) fails to treat Windows drive-only roots like 'C:\\' as "no
useful prefix"; update the condition to detect drive roots explicitly by using
os.path.splitdrive(result) and ensuring there's a meaningful path component
after the root separator — e.g., splitdrive_root, rest =
os.path.splitdrive(result); if not rest or rest == os.sep or
(rest.startswith(os.sep) and rest.count(os.sep) == 1): return '' — or
alternatively check that there is at least one non-root separator character
after the anchor before returning result.
In `@src/picmaker/tiff16.py`:
- Around line 106-114: The code currently treats any byteorder other than
"little" as big-endian; instead validate byteorder (case-insensitive) only for
"native", "little", or "big": if "native" set byteorder = sys.byteorder, then if
byteorder == "little" set o = "<" and flag = b"I", elif byteorder == "big" set o
= ">" and flag = b"M", otherwise raise a ValueError describing the invalid
byteorder input; update the logic around the byteorder variable and the o/flag
assignments (refer to byteorder, sys.byteorder, o, flag) to fail fast on unknown
values.
In `@TEST_SUITE_CRITIQUE.md`:
- Around line 73-88: The document uses bare code names in narrative prose
instead of Sphinx cross-reference roles; update every plain-symbol mention
(e.g., images_to_pics, ColorNames.lookup, picmaker.io.read_one_image_array and
other function/class/module/constant mentions) to the appropriate Sphinx role
(for example :func:`images_to_pics`, :meth:`ColorNames.lookup`,
:mod:`picmaker.io` or :func:`picmaker.io.read_one_image_array`, or
:class:/:attr:/:data: as appropriate) throughout the markdown sections flagged
(including the other noted blocks around the document) so that each class,
method, function, module, attribute, or data constant in narrative prose uses
the correct :class:, :meth:, :func:, :mod:, :attr:, or :data: role.
- Around line 282-283: The audit paragraph in TEST_SUITE_CRITIQUE.md is
inconsistent: remove the stale "RESOLVED" claim and any text that suggests
`palette != None` was fixed, and replace it with a single clear statement that
the underlying bug remains because tiff16.py still uses the problematic
comparison `has_palette = (palette != None)` (which can raise on a non-None
numpy palette), therefore the strict-xfail markers in test_tiff16.py should
still be considered valid; also mention the remaining strict-xfail in
test_pil_utils.py::test_rescale_grayscale_returns_float_in_unit_range as a
separate outstanding issue.
- Around line 197-201: Multiple fenced code blocks (e.g., the pytest snippet
calling ColorNames.lookup('not_a_real_color')) are missing surrounding blank
lines and some fences lack a language tag; update each listed block by ensuring
there is a blank line before and after the ``` fence (fix MD031) and add an
appropriate language identifier (e.g., ```python for Python blocks, ```bash for
shell commands) to the opening fence (fix MD040) for the ranges mentioned
(include the pytest example and the blocks at the other noted ranges).
- Line 19: The document incorrectly states the coverage gate as "fail_under =
90"; update every occurrence in TEST_SUITE_CRITIQUE.md that mentions 'fail_under
= 90' (including the later sections referenced) to match the actual project
setting 'fail_under = 59' as defined in pyproject.toml [tool.coverage.report],
and ensure any nearby text (e.g., references to CI/pytest addopts or coverage
checks) is adjusted for consistency with the real threshold.
In `@tests/test_enhance_branches.py`:
- Around line 73-76: The test uses a loose inequality for the upper limit
(assert hi <= 200 + 0.5) which can mask regressions; update the assertion to
check the precise expected value for hi returned by get_limits for this fixture
(e.g., replace the inequality with assert hi == <expected_value>) and ensure you
reference the same call to get_limits(...) and the hi variable in
tests/test_enhance_branches.py to keep the branch test strict.
In `@tests/test_filters_branches.py`:
- Around line 15-19: The test test_filter_image_blur currently only checks size;
update it to assert the blur actually changed pixel values by comparing the
input image pixels (im) with the output image pixels (out) produced by
filter_image(im, 'blur') — e.g., verify that at least one pixel value differs
(or compute a full pixel-wise difference and assert non-zero) so a no-op
implementation will fail; keep the test using the same 8x8 Image.new('L', ...)
and reference filter_image and the test name when making the change.
In `@tests/test_pil_utils.py`:
- Around line 38-39: Replace the weak range checks on the variable back with a
precise equality assertion: compute or provide the exact expected normalized
numpy array for the test case and assert back equals that expected array using
numpy's array comparison utilities (e.g., np.testing.assert_allclose or
np.testing.assert_array_equal) instead of the two assertions on dtype and max;
update the assertions in tests/test_pil_utils.py referencing back to compare the
full array contents and dtype to the expected normalized values.
In `@tests/test_pipeline_branches.py`:
- Around line 328-329: The test currently uses a too-broad exception check
around the process_images(...) call; change pytest.raises((OSError, Exception))
to the single expected exception type (e.g., pytest.raises(OSError)) and assert
the error message content (either via the raises(..., match="expected text")
parameter or by capturing excinfo and asserting str(excinfo.value) contains the
expected substring) for the call to process_images([str(bogus)], str(tmp_path /
'out'), True, [od]) so the test only passes for the intended failure mode.
In `@tests/test_tiff16_branches.py`:
- Line 42: The test currently allows two shapes which weakens regression checks;
update test_tiff16_branches to assert a single canonical shape by normalizing
the result first (e.g., apply np.squeeze or reshape to remove a singleton
channel) and then assert array.shape == (8, 8). Concretely: ensure the code
operating on the test variable array removes any trailing singleton dimension
(use array = np.squeeze(array) or equivalent) and replace the "assert
array.shape == (8, 8) or array.shape == (8, 8, 1)" with a single exact assertion
"assert array.shape == (8, 8)".
- Around line 28-34: The tests currently only assert file creation; update them
to validate TIFF semantics by reading the written file with ReadTiff16 and
asserting the output shape and pixel values for each branch: for
test_tiff16_three_d_grayscale validate the read array equals the original
reduced grayscale data and shape (h,w,1); for the ROTATE_90 branch call
WriteTiff16 with the rotation option then ReadTiff16 and assert the data matches
a 90-degree rotated version of the input; for the up=True branch write with
up=True, read back with ReadTiff16 and assert both the expected shape and
scaled/upsampled pixel values; ensure assertions use the same unique helpers
WriteTiff16 and ReadTiff16 to check content rather than just file existence.
---
Outside diff comments:
In `@CODEBASE_CRITIQUE.md`:
- Around line 1-212: The audit document CODEBASE_CRITIQUE.md contains
inconsistent British/American spelling ("modernisation" vs "modernization"),
inconsistent capitalization of "GitHub", and missing blank lines around
fenced/code blocks flagged by markdownlint MD031; edit CODEBASE_CRITIQUE.md to
standardize to American spelling (e.g., change "modernisation" →
"modernization"), capitalise all occurrences of "GitHub", and ensure there is a
blank line before and after each code block (or fenced block) to satisfy MD031.
In `@src/picmaker/colornames.py`:
- Around line 782-836: Add a docstring to ColorNames.lookup describing the
parameter (name: str), the return value (tuple[int,int,int]), and the raised
exceptions (TypeError for non-str, KeyError for unknown names, ValueError for
out-of-range components) plus the accepted input forms (named colors,
lower/normalized variants, RGB tuple or bracketed list). Change the annotated
return type from Any to tuple[int, int, int] and normalize any parsed RGB list
to a 3-tuple before returning (e.g. convert the ast.literal_eval result or any
list from RGB parsing into tuple(r,g,b)). Ensure the function still uses
ColorNames.COLOR_NAME_DICT, ColorNames.COLOR_NAME_LOWER_DICT and
ColorNames.RGB_PATTERN as currently to locate the code to modify.
In `@src/picmaker/io.py`:
- Around line 178-243: The FITS file handle opened via pyfits.open(filename_str)
(hdulist) can leak if exceptions occur before hdulist.close(); change the code
to open the FITS file with a context manager (e.g., with
pyfits.open(filename_str) as hdulist:) or wrap the existing logic in try/finally
to ensure hdulist.close() always runs; apply this around the block that uses
hdulist (the detection loop using instruments.FITS_INSTRUMENTS, the HST branches
for 'ACS/WFC' and 'WFPC2', the obj handling and the final ReadResult return) so
that exceptions like IndexError or OSError won’t leave the file open.
In `@tests/test_color.py`:
- Around line 57-59: The comment in tests referencing "eval" is stale; update
the comment near the assertion calling ColorNames.lookup('[100, 100, 100]') to
say the production code uses ast.literal_eval (not eval), and if you implement
the tuple-normalization change in colornames.py (the tuple normalization
behavior in ColorNames.lookup), also update the test assertion to expect a tuple
instead of a list and adjust the comment to describe that tuple-normalization
behavior.
---
Duplicate comments:
In `@src/picmaker/io.py`:
- Around line 349-355: The code that sets imagefile in the node-handling branch
(the string branch and the list/tuple branch treating node[0]) joins untrusted
pointer values directly to the label directory using filename_str, allowing path
traversal; fix by resolving the candidate path to an absolute/resolved path (use
os.path.abspath or pathlib.Path(filename).resolve()) and compare it against the
base directory resolved path (os.path.dirname(filename_str) resolved) using
os.path.commonpath or Path.commonpath to ensure the resulting imagefile is
contained within the base directory; if the containment check fails, reject the
pointer (raise an error or skip) rather than using the escaped path, and
preserve existing offset_value logic for valid paths (references: filename_str,
pname, label_dict, imagefile, offset_value, node and node[0]).
In `@tests/fixture_recipes/generate_user_guide_thumbnails.py`:
- Around line 30-32: Rename the module-level constants HERE, FIXTURES, and
OUT_DIR to use a single leading underscore (_HERE, _FIXTURES, _OUT_DIR) to mark
them as internal, then update every use in this file accordingly (e.g., replace
fixture = FIXTURES / fixture_name with fixture = _FIXTURES / fixture_name, call
_OUT_DIR.mkdir(...), pass _OUT_DIR into _generate_one(...) and update print
messages to reference _OUT_DIR); ensure imports/other symbols remain unchanged
and run tests to confirm no remaining references to the old names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 68b14d1c-1a10-4403-b69e-b9a295df492f
📒 Files selected for processing (68)
.cursor/rules/python_best_practices.mdc.github/workflows/publish_to_pypi.yml.github/workflows/publish_to_test_pypi.yml.github/workflows/run-tests.ymlCODEBASE_CRITIQUE.mdCONTRIBUTING.mdRELEASING.mdTEST_SUITE_CRITIQUE.mddocs/conf.pydocs/developer_guide.rstdocs/index.rstdocs/module.rstdocs/user_guide.rstpyproject.tomlscripts/run-all-checks.shsrc/picmaker/__init__.pysrc/picmaker/_filters.pysrc/picmaker/cli.pysrc/picmaker/colornames.pysrc/picmaker/instruments/__init__.pysrc/picmaker/instruments/cassini.pysrc/picmaker/instruments/galileo.pysrc/picmaker/instruments/hst.pysrc/picmaker/instruments/nh.pysrc/picmaker/instruments/voyager.pysrc/picmaker/io.pysrc/picmaker/options.pysrc/picmaker/picmaker.pysrc/picmaker/pil_utils.pysrc/picmaker/pipeline.pysrc/picmaker/tiff16.pytests/fixture_recipes/generate_user_guide_thumbnails.pytests/test_alt_strip_alias.pytests/test_apply_gamma.pytests/test_cli.pytests/test_cli_unit.pytests/test_color.pytests/test_enhance.pytests/test_enhance_branches.pytests/test_filters_branches.pytests/test_frame_max.pytests/test_geometry.pytests/test_geometry_branches.pytests/test_geometry_extra.pytests/test_hst_filter_tuple_normalization.pytests/test_instruments_branches.pytests/test_io.pytests/test_io_cascade.pytests/test_io_extra.pytests/test_mutable_defaults.pytests/test_overlap_vs_overlaps.pytests/test_package_init.pytests/test_paths.pytests/test_pds3_reader.pytests/test_pds3_reader_branches.pytests/test_pickle_iolost_propagates_to_final_error.pytests/test_pil_utils.pytests/test_pil_utils_branches.pytests/test_pipeline.pytests/test_pipeline_branches.pytests/test_snapshots.pytests/test_tiff16.pytests/test_tiff16_branches.pytests/test_tinted_colormap.pytests/test_unknown_filter_warning.pytests/test_versions_override.pytests/test_warning_elevation.pytests/test_zebra.py
💤 Files with no reviewable changes (3)
- tests/test_alt_strip_alias.py
- tests/test_overlap_vs_overlaps.py
- tests/test_versions_override.py
Comment on lines
+87
to
+88
| :data:`picmaker._rgb.RFUNC` / :data:`picmaker._rgb.GFUNC` / | ||
| :data:`picmaker._rgb.BFUNC` splines. Each detector family has its |
Contributor
There was a problem hiding this comment.
Use :func: roles for callable symbols in this docstring.
RFUNC, GFUNC, and BFUNC are functions, so these references should use :func: instead of :data:.
Suggested docstring fix
- :data:`picmaker._rgb.RFUNC` / :data:`picmaker._rgb.GFUNC` /
- :data:`picmaker._rgb.BFUNC` splines. Each detector family has its
+ :func:`picmaker._rgb.RFUNC` / :func:`picmaker._rgb.GFUNC` /
+ :func:`picmaker._rgb.BFUNC` splines. Each detector family has itsAs per coding guidelines, “EVERY mention of a class, method, function, module, attribute, or data constant in narrative prose MUST use appropriate Sphinx cross-reference roles.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/picmaker/instruments/hst.py` around lines 87 - 88, The docstring
incorrectly uses :data: for callable symbols; change the Sphinx roles for RFUNC,
GFUNC, and BFUNC to use :func: instead of :data: (find the docstring referencing
picmaker._rgb.RFUNC / picmaker._rgb.GFUNC / picmaker._rgb.BFUNC in
src/picmaker/instruments/hst.py and replace each :data:`picmaker._rgb.RFUNC` /
:data:`picmaker._rgb.GFUNC` / :data:`picmaker._rgb.BFUNC` with
:func:`picmaker._rgb.RFUNC` / :func:`picmaker._rgb.GFUNC` /
:func:`picmaker._rgb.BFUNC` so they are proper callable cross-references).
## New layout The monolithic `docs/developer_guide.rst` is now an index page with a nested toctree. The major sections live as separate files under `docs/dev/`: - `docs/dev/repository_overview.rst` — purpose + repository tree. - `docs/dev/module_layout.rst` — module-by-module description with per-symbol Sphinx cross-references. - `docs/dev/pipeline.rst` — Mermaid flowchart + prose walk-through of CLI → reader cascade → enhancement → geometry → write. - `docs/dev/running_tests.rst` — pytest invocation, snapshot regeneration. - `docs/dev/adding_an_instrument.rst` — the four-function protocol and the seven-step recipe. - `docs/dev/releasing.rst` — moved from RELEASING.md verbatim (one- time trusted-publisher setup + cut-a-release flow). `docs/developer_guide.rst` is now a slim index page with the toctree plus the "Where to look next" section (pointers to the API reference, user guide, GitHub issues, and the two critique audits). ## RELEASING.md removed The file is deleted. The CI step list in `.github/workflows/run-tests.yml` and the optional-scan list in `scripts/run-all-checks.sh` were updated to stop referencing it. The two publish workflow comments now point at `docs/dev/releasing.rst` instead. ## Critique against `.cursor/rules/documentation.mdc` - American spelling: clean. - Sphinx cross-references for every API symbol in narrative: present in the new files (audited; the bare-name section titles like `:mod:`picmaker.cli`` are kept because they remain useful links in the rendered TOC). - `sphinx-build -W -b html`: clean. - `sphinx-build -n -b html`: zero warnings. - `pymarkdown scan` over `docs/ .cursor/ README.md CONTRIBUTING.md`: clean (the deleted `RELEASING.md` was dropped from the scan list). - Per-page TOC entries appear correctly in the rendered `developer_guide.html` (`toctree-l2` entries for each `dev/*.rst`). ## Full check status - pytest: 471 passed. - ruff: clean. - mypy strict: clean. - bandit: clean. - vulture: clean. - sphinx -W: clean. - sphinx -n: zero warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Verified each comment against current code; applied the still-valid
fixes. Skipped CODEBASE_CRITIQUE.md / TEST_SUITE_CRITIQUE.md items
and anything related to pickle or path traversal per reviewer
guidance. Skipped one further item (hst.py :data: → :func: for the
Tabulation splines) because Tabulation instances are module-level
data even though they are callable; :data: is the correct role.
## Production source
- `docs/user_guide.rst:458`: `--filter` table row now shows
`dest = filter_name` to match the actual argparse binding.
- `src/picmaker/colornames.py`: added a class docstring for
`ColorNames` and a full Google-style docstring for
`ColorNames.lookup` (parameters / returns / raises / accepted
input forms). Narrowed the return annotation from `Any` to
`tuple[int, int, int]` and normalized list-style `[r, g, b]`
parses to a tuple before returning so the two bracketing styles
give identical output. Removed the now-unused `typing.Any` import.
- `src/picmaker/instruments/galileo.py`, `.../nh.py`, `.../voyager.py`:
wrapped the long lines in the `FILTER_NAMES` description and the
`Raises: KeyError` bullets so every line fits in 90 columns.
- `src/picmaker/options.py`: dropped the redundant
`self.filter_name is not None` test in `validate()`; the field is
typed `str` with default `'NONE'`, so a direct case-folded compare
is enough.
- `src/picmaker/pipeline.py::process_images`: added an empty-list
guard in movie mode (`raise ValueError('movie mode requires at
least one option_dict')`) before indexing `option_dicts[0]`.
- `src/picmaker/pipeline.py::find_common_path`: replaced the
length-based root check with `os.path.splitdrive` so Windows
drive-only roots (`'C:\\'`) are recognized as "no useful prefix"
alongside POSIX `/`.
- `src/picmaker/tiff16.py::WriteTiff16`: validate `byteorder` against
`{'native', 'little', 'big'}` (case-insensitive) and raise
`ValueError` otherwise — previously any non-`'little'` value
silently produced a big-endian file.
- `src/picmaker/io.py::read_one_image_array`: open the FITS file via
`with pyfits.open(...) as hdulist:` so the handle is always
closed, even when downstream HST mosaic / `obj` branches raise.
Removed the now-redundant trailing `hdulist.close()`.
## Tests
- `tests/test_enhance_branches.py::test_get_limits_with_footprint_filter`:
replaced `hi <= 200 + 0.5` with the precise `lo == 0 and hi == 0`
(the 3×3 median of a single-outlier array is uniformly 0).
- `tests/test_filters_branches.py::test_filter_image_blur`: use a
non-uniform input (one bright pixel against black) and assert
blur actually changes pixel values, so a no-op `blur` would fail.
- `tests/test_pil_utils.py::test_rescale_grayscale_returns_float_in_unit_range`:
replaced the dtype/max checks with an exact
`np.testing.assert_array_equal` against the precisely-computed
expected normalized array (first pixel 0.0, the rest 1.0 due to
the input being deliberately outside the `[0, 1]` rescale
contract).
- `tests/test_pipeline_branches.py::test_process_images_movie_failure_raises`:
tightened `pytest.raises((OSError, Exception))` to
`pytest.raises(OSError, match='Unrecognized image file format')`.
- `tests/test_tiff16_branches.py`: every test now reads the file
back with `ReadTiff16` and asserts pixel-wise round-trip equality
via `np.testing.assert_array_equal`. Replaced the
`(8, 8) or (8, 8, 1)` shape OR with a single canonical shape via
`np.squeeze`. `transpose=ROTATE_90` asserts against
`np.rot90(arr, 1)`; `up=True` asserts against `np.flipud(arr)`.
- `tests/test_color.py::test_rgb_brackets_expression`: updated to
expect a tuple (matching the new `ColorNames.lookup` normalization)
and replaced the stale "eval result" comment with one that
references `ast.literal_eval`.
## Fixture-recipe internal-name convention
- `tests/fixture_recipes/generate_user_guide_thumbnails.py`:
`HERE` / `FIXTURES` / `OUT_DIR` renamed to `_HERE` / `_FIXTURES`
/ `_OUT_DIR` and every consumer updated. The script is invoked
only from the command line and from `regenerate_all.py`, so the
underscore prefix makes their module-private status explicit.
## Validation
- pytest: 471 passed.
- ruff: clean.
- mypy strict: clean.
- bandit / vulture: clean.
- `sphinx-build -W` and `sphinx-build -n`: clean (zero warnings).
- pymarkdown: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`find_common_path` delegates to `os.path.commonpath`, which returns
the platform-native separator. On Windows
`os.path.commonpath(['/foo/bar/x', '/foo/bar/y'])` returns
`'\\foo\\bar'`, not `'/foo/bar'`, so the two `TestFindCommonPath`
assertions that hard-coded forward slashes were failing on the
windows-latest CI cell.
Wrap the expected values in `os.path.normpath(...)` so they match
the platform separator the function actually returns. The behaviour
is unchanged on POSIX (`os.path.normpath('/foo/bar') == '/foo/bar'`).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/picmaker/io.py (2)
347-358:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical security issue: Path traversal in PDS pointer resolution remains unresolved.
Lines 348 and 352 join untrusted pointer paths from PDS labels without validation. A malicious label could use
^IMAGE = "../../../sensitive_file"to read arbitrary files. The past review requested validation usingos.path.abspathandos.path.commonpathto ensure paths remain within the expected directory.As per coding guidelines, "For file paths, resolve to absolute paths and verify they remain within the expected directory (prevent path traversal)."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 347 - 358, The pointer-resolution branches (the isinstance(node, str) and isinstance(node, (list, tuple)) paths that set imagefile using filename_str and node values) join untrusted PDS label values directly, allowing path traversal; fix by resolving the candidate image path to an absolute path with os.path.abspath, compute the expected base directory from filename_str (e.g., os.path.dirname(filename_str) and its abspath), then verify with os.path.commonpath that the resolved image path is inside that base directory; if the check fails, raise a ValueError/TypeError and do not use the path; keep existing offset_value logic and continue to use label_dict/pname for offset lookup but validate all pointer-derived paths before assigning imagefile.
136-142:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical security issue: pickle.load on untrusted input remains unresolved.
pickle.load(f)at line 137 can execute arbitrary code embedded in malicious pickle files. This function is a public API that accepts user-provided filenames without any opt-in flag or security warning. The past review flagged this as critical and requested either removal or explicit opt-in gating.As per coding guidelines, "NEVER trust external input (function arguments from callers, file contents, environment variables, data from remote URLs). Validate inputs at the public API boundary of the library."
Based on learnings, "NEVER guess at bug causes. Use logic, stack traces, and targeted logging. If stuck in a fix loop, revert and re-approach from first principles. When necessary, ask for help."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 136 - 142, The use of pickle.load on untrusted files in the public API must be removed or explicitly gated: replace the blind pickle.load call inside the function that opens filename_str and returns ReadResult with a safe alternative or an explicit opt-in check (e.g., add a new boolean parameter allow_pickle defaulting to False) so that by default loading raises a clear security error; if allow_pickle is True, require callers to explicitly opt in and log a security warning before using pickle (or use a documented, sandboxed custom Unpickler). Update the code paths that call this loader to pass the flag where appropriate and ensure ReadResult creation remains unchanged (return ReadResult(array3d, False, None)) only after safe loading succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/module_layout.rst`:
- Around line 20-24: Replace the inline literal API symbols in the prose with
Sphinx cross-reference roles: wrap class names like VicarImage and VicarError
with :class:, functions like ReadTiff16 and WriteTiff16 with :func:, and
data/constants like ColorNames, GALILEO_SSI_DICT, GALILEO_SSI_NAMES,
NH_MVIC_DICT, and VOYAGER_ISS_DICT with :data: (adjust role if a symbol is a
different type), e.g. use :class:`VicarImage`, :func:`ReadTiff16`,
:data:`GALILEO_SSI_DICT` so the references are validated and linkable and
conform to the coding guideline requiring roles for API mentions.
In `@docs/dev/pipeline.rst`:
- Around line 130-135: Replace bare mentions of functions in narrative prose
with Sphinx cross-reference roles: change occurrences of "fits.open" to an
appropriate function role (e.g. :func:`astropy.io.fits.open`), and wrap
"tint_for" and "write_pil" with function roles (e.g. :func:`tint_for`,
:func:`write_pil`) so they become linkable and validated by Sphinx; apply the
same conversion to the other affected paragraph range (the section around lines
273-290) to ensure every prose mention of these callables uses the proper
cross-reference role.
In `@docs/user_guide.rst`:
- Line 400: Replace British spelling "Colour" with American "Color" in the three
table description phrases that read "Colour used for pixels below the lower
limit." (and similar occurrences) in docs/user_guide.rst so all instances use
"Color"; update each matching phrase exactly (e.g., change "Colour used for
pixels below the lower limit." to "Color used for pixels below the lower
limit.") to maintain consistency with the coding guidelines.
In `@tests/fixture_recipes/generate_user_guide_thumbnails.py`:
- Around line 78-84: The helper _generate_one currently accepts five positional
args; change its signature to make parameters after the first three keyword-only
(e.g. def _generate_one(slug: str, fixture_name: str, kwargs: dict[str, Any], *,
ext: str, out_dir: Path) -> Path | None) and apply the same change to the other
occurrence referenced at line 145; then update all call sites to pass ext and
out_dir by name instead of position. Ensure type hints remain and no behavioral
changes are introduced.
In `@tests/test_enhance_branches.py`:
- Line 66: Replace the single relational assertion "assert lo == hi - 1" with
two precise assertions that check the exact expected values for the test;
specifically assert that hi equals 100 and lo equals 99 (since the test
describes "all 100s except one masked"), updating the assertions in
tests/test_enhance_branches.py to use these exact constants instead of a
difference check.
- Around line 86-87: The assertions use computed expressions rather than
explicit expected values; replace the two asserts that compare lo and hi against
arr[4:-4, 4:-4].min() - 0.5 and arr[4:-4, 4:-4].max() + 0.5 with hard-coded
numeric expected values (e.g., assert lo == <expected_min_float> and assert hi
== <expected_max_float>) so the test checks precise values for lo and hi using
the actual numbers derived from the arr[4:-4,4:-4] slice.
- Around line 122-126: The test test_apply_colormap_named_two_stop currently
only checks shape; update it to assert actual RGB values produced by
apply_colormap(arr, (0.0, 7.0), colormap='red-blue'). Compute or hardcode the
expected 3-channel RGB result for the input arr (including endpoint colors for
0.0 and 7.0 and interpolated values for intermediate indices), then assert
out.dtype is correct, out.shape == (2,4,3), and that out equals (or is
approximately equal to) the expected RGB array element-wise; reference
apply_colormap and the test function name to locate and replace the shape-only
assertion with these value assertions.
In `@tests/test_pipeline_branches.py`:
- Line 123: Move the inline "import logging" statements out of the two test
functions and add a single "import logging" to the module-level standard-library
import block at the top of the file (alphabetize it within the stdlib group and
keep the blank-line separation between stdlib/third-party/local groups); then
remove the two inline "import logging" lines from the test functions so they use
the module-level logger import instead.
---
Duplicate comments:
In `@src/picmaker/io.py`:
- Around line 347-358: The pointer-resolution branches (the isinstance(node,
str) and isinstance(node, (list, tuple)) paths that set imagefile using
filename_str and node values) join untrusted PDS label values directly, allowing
path traversal; fix by resolving the candidate image path to an absolute path
with os.path.abspath, compute the expected base directory from filename_str
(e.g., os.path.dirname(filename_str) and its abspath), then verify with
os.path.commonpath that the resolved image path is inside that base directory;
if the check fails, raise a ValueError/TypeError and do not use the path; keep
existing offset_value logic and continue to use label_dict/pname for offset
lookup but validate all pointer-derived paths before assigning imagefile.
- Around line 136-142: The use of pickle.load on untrusted files in the public
API must be removed or explicitly gated: replace the blind pickle.load call
inside the function that opens filename_str and returns ReadResult with a safe
alternative or an explicit opt-in check (e.g., add a new boolean parameter
allow_pickle defaulting to False) so that by default loading raises a clear
security error; if allow_pickle is True, require callers to explicitly opt in
and log a security warning before using pickle (or use a documented, sandboxed
custom Unpickler). Update the code paths that call this loader to pass the flag
where appropriate and ensure ReadResult creation remains unchanged (return
ReadResult(array3d, False, None)) only after safe loading succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3820e490-3657-4625-8e51-ae9f78b533bc
📒 Files selected for processing (26)
.github/workflows/publish_to_pypi.yml.github/workflows/publish_to_test_pypi.yml.github/workflows/run-tests.ymldocs/dev/adding_an_instrument.rstdocs/dev/module_layout.rstdocs/dev/pipeline.rstdocs/dev/releasing.rstdocs/dev/repository_overview.rstdocs/dev/running_tests.rstdocs/developer_guide.rstdocs/user_guide.rstsrc/picmaker/colornames.pysrc/picmaker/instruments/galileo.pysrc/picmaker/instruments/nh.pysrc/picmaker/instruments/voyager.pysrc/picmaker/io.pysrc/picmaker/options.pysrc/picmaker/pipeline.pysrc/picmaker/tiff16.pytests/fixture_recipes/generate_user_guide_thumbnails.pytests/test_color.pytests/test_enhance_branches.pytests/test_filters_branches.pytests/test_pil_utils.pytests/test_pipeline_branches.pytests/test_tiff16_branches.py
Verified each comment against the current code. Every item the
reviewer flagged was still present.
Notable verification correction:
``test_get_limits_trim_zeros_with_mask`` — the reviewer suggested
`hi == 100, lo == 99`. Tracing `get_limits` shows the actual values
are `lo == 100, hi == 101` (after the all-zero exterior is trimmed,
the inner 6x6 is uniform 100 except for one masked cell, so
`array_min == array_max == 100` and the equal-values integer
branch returns `(value, value + 1)`). The exact-equality assertions
in the patch use the actual values.
## Documentation
- `docs/dev/module_layout.rst`: inline-literal API symbols in the
BC-shim paragraph (`ColorNames`, `ReadTiff16`, `WriteTiff16`,
`VicarError`, `VicarImage`, `GALILEO_SSI_DICT`,
`GALILEO_SSI_NAMES`, `NH_MVIC_DICT`, `VOYAGER_ISS_DICT`) replaced
with Sphinx `:class:` / `:func:` / `:data:` roles.
- `docs/dev/pipeline.rst`: the bare `fits.open` reference in the
reader-cascade paragraph now uses `:func:`astropy.io.fits.open``.
The bare `tint_for` and `write_pil` mentions in the "Color, filter,
and PIL bridges" paragraph were promoted to cross-references
(`:func:`picmaker.instruments.cassini.tint_for`` as the
canonical concrete example, and a `~picmaker.pil_utils.write_pil`
back-reference).
- `docs/user_guide.rst`: three "Colour used for..." table rows under
the enhancement options changed to American "Color used for...".
## Tests
- `tests/test_enhance_branches.py`:
* `test_get_limits_trim_zeros_with_mask`: the relational
`lo == hi - 1` is replaced with explicit `lo == 100, hi == 101`.
* `test_get_limits_with_trim`: the computed
`lo == arr[4:-4, 4:-4].min() - 0.5` style assertions are replaced
with literal `lo == 67.5, hi == 187.5` (the inner 8x8 block of
`arange(256).reshape(16, 16)` runs from 68..187).
* `test_apply_colormap_named_two_stop`: shape-only assertion is
replaced with `np.testing.assert_allclose` against the
analytically-computed expected RGB volume (red channel
`1 - arr/7`, blue channel `arr/7`, green channel zero) plus
explicit dtype and shape checks.
- `tests/test_pipeline_branches.py`: the two inline `import logging`
statements are removed; the module-level stdlib import block now
has a single `import logging` placed alphabetically with the
other stdlib imports.
## Fixture recipe
- `tests/fixture_recipes/generate_user_guide_thumbnails.py`: the
`_generate_one` helper's `ext` and `out_dir` parameters are now
keyword-only (`def _generate_one(slug, fixture_name, kwargs, *,
ext, out_dir)`); the single call site in `main()` passes them by
name.
## Validation
- pytest: 471 passed.
- ruff: clean.
- mypy strict: clean.
- bandit / vulture: clean.
- `sphinx-build -W` and `sphinx-build -n`: clean (zero warnings).
- pymarkdown: clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tighten typing surface in picmaker.io, expose the public API in the docs, and correct a stale comment that mis-described mypy's coverage of tests/. - src/picmaker/io.py: introduce ObjectSelector type alias and replace Any in read_image_array / read_one_image_array / read_pds_labeled_image_array signatures with concrete unions (str/Sequence for filename, str|PathLike|None for labelfile). Narrow read_pil's return to Image.Image | list[Image.Image] and read_array's to NDArray[Any] (dtype genuinely varies); bridge the still-Any array_to_pil / pil_to_array helpers with typing.cast. Replace the broad TypeError handler in read_pds_labeled_image_array with an early isinstance check that preserves the original "Invalid index type" message. - tests/test_io_cascade.py, tests/test_io_extra.py: assert isinstance(img, Image.Image) where the tests treat read_pil's result as a single PIL image, narrowing the new union. - docs/module.rst: add a "Public API" section above the leaf-module sections that lists every name in picmaker.__all__ via autofunction / autoclass, organized by topic. Bare data exports without their own docstrings (FILTER_DICT, RGB_BY_NM, RFUNC/GFUNC/BFUNC) are pointed at by prose to avoid autodata inheriting the type's stub docstring (e.g. "dict() -> new empty dictionary..."). - scripts/run-all-checks.sh: rewrite the comment above the mypy invocation, which incorrectly claimed tests/ is not type-checked. Verified by the (src + tests) file count in the mypy success line. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/picmaker/io.py (2)
137-152:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftPickle deserialization remains ungated.
The past review comment requesting removal or opt-in gating for
pickle.load()was not addressed. This function is part of the public API and accepts filenames from users without validation. Per coding guidelines, never trust external input at the public API boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 137 - 152, This code performs an unguarded pickle.load on filename_str in the public API; add an explicit opt-in gate (e.g., a boolean parameter like allow_pickle=False) to the public read function that contains this block, and only execute the pickle.load branch when allow_pickle is True; if allow_pickle is False, skip the pickle branch (append a clear exception to cascade_errors or raise a ValueError indicating pickle deserialization is disabled) so callers must opt-in to use pickle; keep the existing try/except around pickle.load and return ReadResult(array3d, False, None) only when allow_pickle is enabled and loading succeeds.
353-367:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winValidate detached PDS pointer paths to prevent traversal.
The pointer resolution joins untrusted label values directly without validation. A malicious label with
node = "../../../etc/passwd"could read arbitrary files. Per coding guidelines, "For file paths, resolve to absolute paths and verify they remain within the expected directory (prevent path traversal)."Past review comment marked this as addressed, but the validation logic is not present in the code.
🛡️ Proposed fix
+ base_dir = os.path.abspath(os.path.dirname(filename_str)) + if isinstance(node, int): imagefile = filename_str offset_value = node elif isinstance(node, str): - imagefile = os.path.join(os.path.split(filename_str)[0], node) + candidate = os.path.abspath(os.path.join(base_dir, node)) + if not candidate.startswith(base_dir + os.sep) and candidate != base_dir: + raise ValueError(f'Pointer path escapes label directory: {node!r}') + imagefile = candidate offset_value = label_dict.get(pname + '_offset', 1) or 1 elif isinstance(node, (list, tuple)): if isinstance(node[0], str): - imagefile = os.path.join(os.path.split(filename_str)[0], node[0]) + candidate = os.path.abspath(os.path.join(base_dir, node[0])) + if not candidate.startswith(base_dir + os.sep) and candidate != base_dir: + raise ValueError(f'Pointer path escapes label directory: {node[0]!r}') + imagefile = candidate offset_value = int(node[1]) if len(node) >= 2 else 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/picmaker/io.py` around lines 353 - 367, The pointer resolution currently joins untrusted label values (node) into imagefile using filename_str without validating path traversal; update the logic in the branch handling str and list/tuple string-first entries so after computing the candidate path (from os.path.join(os.path.split(filename_str)[0], node or node[0])) you resolve it to an absolute path and verify it remains inside the expected base directory (the directory portion of filename_str) — e.g., use os.path.abspath or pathlib.Path.resolve and compare against the base dir (using Path.relative_to or a safe startswith check with a trailing separator); if the check fails, raise a ValueError/TypeError rather than using the path. Ensure the same validation is applied to all branches that set imagefile (str branch and list/tuple branch where node[0] is str) and keep existing behavior for numeric offsets and error messages referencing filename_str and pname.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_enhance_branches.py`:
- Around line 123-126: The tests in tests/test_enhance_branches.py only assert a
single channel for the sentinel colors (assert out[0,0,2] and assert out[0,3,0])
so other channels could be wrong; update these to assert the full RGB triplets
at positions out[0,0] and out[0,3] equal the expected vectors (blue [0,0,1] and
red [1,0,0]) using a vector comparison (e.g., pytest.approx on the list/tuple or
numpy.testing.assert_allclose) so all three channels are verified for the
below/above sentinel cases.
In `@tests/test_io_cascade.py`:
- Line 214: Remove the redundant inline imports "from PIL import Image as
PILImage" and their aliases; replace any usages of PILImage in the test with the
already-imported module-level Image symbol (i.e., change PILImage.open(...) /
PILImage.new(...) calls to Image.open(...) / Image.new(...)), and delete both
inline import statements so the file uses the single top-level Image import.
---
Duplicate comments:
In `@src/picmaker/io.py`:
- Around line 137-152: This code performs an unguarded pickle.load on
filename_str in the public API; add an explicit opt-in gate (e.g., a boolean
parameter like allow_pickle=False) to the public read function that contains
this block, and only execute the pickle.load branch when allow_pickle is True;
if allow_pickle is False, skip the pickle branch (append a clear exception to
cascade_errors or raise a ValueError indicating pickle deserialization is
disabled) so callers must opt-in to use pickle; keep the existing try/except
around pickle.load and return ReadResult(array3d, False, None) only when
allow_pickle is enabled and loading succeeds.
- Around line 353-367: The pointer resolution currently joins untrusted label
values (node) into imagefile using filename_str without validating path
traversal; update the logic in the branch handling str and list/tuple
string-first entries so after computing the candidate path (from
os.path.join(os.path.split(filename_str)[0], node or node[0])) you resolve it to
an absolute path and verify it remains inside the expected base directory (the
directory portion of filename_str) — e.g., use os.path.abspath or
pathlib.Path.resolve and compare against the base dir (using Path.relative_to or
a safe startswith check with a trailing separator); if the check fails, raise a
ValueError/TypeError rather than using the path. Ensure the same validation is
applied to all branches that set imagefile (str branch and list/tuple branch
where node[0] is str) and keep existing behavior for numeric offsets and error
messages referencing filename_str and pname.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a8158e0-32b0-4954-b78d-b495b7687f62
📒 Files selected for processing (12)
docs/dev/module_layout.rstdocs/dev/pipeline.rstdocs/module.rstdocs/user_guide.rstscripts/run-all-checks.shsrc/picmaker/io.pytests/fixture_recipes/generate_user_guide_thumbnails.pytests/test_enhance_branches.pytests/test_io_cascade.pytests/test_io_extra.pytests/test_paths.pytests/test_pipeline_branches.py
- Remove CODEBASE_CRITIQUE.md, CODEBASE_REVIEW.md, and
TEST_SUITE_CRITIQUE.md from git tracking (the files stay in the
working tree for local reference) and add them, the local
modernization-plan drafts, and CLAUDE.md to .gitignore so they don't
re-enter the index. Drop the two doc cross-references that used to
point at them (docs/dev/repository_overview.rst tree listing and
docs/developer_guide.rst "where to look next" section).
- User Guide (docs/user_guide.rst) rewritten as an end-user document.
Drops the "without reading the source code" framing, fixes the Node
name to "PDS Ring-Moon Systems Node", adds the pipx install
alternative, removes the dev venv recipe and the dependency list
(those belong in the dev guide), converts the eleven wide CLI
reference tables to bullet lists and drops the dest column, strips
every code-level reference (module paths, function names, exception
types, return-value shapes, package-internal names), and uses bare
re-exported names like `images_to_pics` instead of full
picmaker.pipeline / picmaker.io paths. The 23 thumbnail image
references are gone -- the underlying _static/user_guide/*.jpg files
were 1x1-pixel placeholders that rendered as black squares.
- Developer Guide refreshed:
* docs/dev/repository_overview.rst: fixed Node name, added a
"Setting up a development environment" section with clone / venv
/ `pip install -e ".[dev]"` / `bash scripts/run-all-checks.sh`,
and neutralized the picmaker.py tree-comment to "Alternate import
path".
* docs/dev/module_layout.rst: dropped the "prose below" phrasing and
the picmaker.picmaker "BC shim" / "pre-PR-3 1.x API" section in
favor of a single line on the top-level package.
* docs/dev/pipeline.rst: removed the "prose walk-through" /
"surrounding prose" / "Major functions in prose" phrasings and
documented that the Mermaid diagram renders client-side as
natively zoomable inline SVG (mermaid_output_format = 'raw' in
conf.py); added :align: center.
- Cleanup of the orphan user-guide thumbnails:
docs/_static/user_guide/* (23 files) and
tests/fixture_recipes/generate_user_guide_thumbnails.py removed.
The recipe is no longer referenced from CI or regenerate_all.py.
- Test fixes from a fresh round of inline review comments:
* tests/test_enhance_branches.py: widen the below_color / above_color
sentinel assertions to compare the full RGB triplet via
np.testing.assert_allclose, not just one channel.
* tests/test_io_cascade.py: remove the two redundant inline
`from PIL import Image as PILImage` shadows; use the top-level
`Image` import for the `.new(...)` calls.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous commit added local-only files (CLAUDE.md, the critique docs, MODERNIZATION_PLAN drafts) to .gitignore. That was unrequested; back it out. The files remain untracked but will now show up in `git status` like any other untracked path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop five of the six per-file ignores in pyproject.toml by fixing the underlying violations. Behaviour is preserved; the public-API symbols (rotate_array_rgb, the io.py reader cascade, the cli.py validator) keep the same call shape, return types, and error messages, and the full 472-test suite plus mypy --strict pass against both src/ and tests/. - RUF005 (io.py, pipeline.py): 8 sites of `(N,) + shape` rewritten as `(N, *shape)` -- both forms produce byte-identical numpy shape tuples. - N806 (pipeline.py, geometry.py): the four camelCase locals `arrayRGB` / `arraysRGB` / `panelsRGB` / `quadsRGB` renamed to snake_case. They were function-local in `images_to_pics` and on `rotate_array_rgb`, never exported, and every caller passes them positionally. `rotate_array_rgb`'s param `arrayRGB` is renamed array_rgb and the `# noqa: N803 -- preserved kwarg name` shim alongside it is deleted (the package is pre-1.0 `0.1.dev20` so no external kwarg-name contract exists). - SIM102 (cli.py, geometry.py): 4 nested-if blocks collapsed into single `if X and Y` guards. The cli.py case (the hst/band/bands mutex chain) is preserved by the existing `pytest.raises(..., match='hst and band'|'band and bands')` tests in test_cli_unit.py. The lone remaining ignore is `tiff16.py = ["N802"]`. The misleading "modernisation pass" wording is replaced with the real reason: WriteTiff16 / ReadTiff16 are public-API entries pinned by tests/test_api_compat.py and are still PascalCase in the documented call signature. Tracked for a future deprecation cycle in #22. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Replace mtime comparison with content comparison in test_versions_override to avoid flakiness on coarse-mtime filesystems. - Add test_happy_path_no_warnings to guard against spurious WARNING-level log records on the default images_to_pics path. - Add a Snapshot freshness CI step (ubuntu-latest + py3.13) that re-runs the generator and asserts git diff is clean, catching silent drift in tests/fixtures/expected/ and tests/snapshots_index.py. - Emit the snapshots_index.py with type annotations so regeneration is byte-identical to the committed file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The user-guide thumbnail purge in 43239de removed every file under docs/_static/, but conf.py still pointed html_static_path at the directory. Sphinx then emits "html_static_path entry '_static' does not exist", which the CI build promotes to an error via -W. Nothing in the docs sources references _static/... any more, so the path is dropped entirely rather than recreating an empty directory. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rfrenchseti
added a commit
that referenced
this pull request
May 12, 2026
…s/CI (#23) * PR1: foundation — packaging, metadata, latent bug fixes (#5) Implements PR 1 of the four-PR modernization plan. Files created: - src/picmaker/__init__.py: module docstring; __version__ resolved via importlib.metadata("rms-picmaker") with fallback to picmaker._version, then to "0.0.0+unknown". Placeholder __all__ (re-exports come in PR 3). - src/picmaker/py.typed: empty marker file already declared in [tool.setuptools.package-data] but missing on disk. - tests/fixtures/.baseline-help.txt + .baseline-flags.txt: snapshots of the current optparse help and flag set, captured for PR 3's argparse migration to diff against. Files edited: - README.md: replaced three TODO blocks with concrete CLI and library usage examples, plus a pickle-trust warning (pickle dispatch is preserved per scope guard). - pyproject.toml: - description and keywords get real values; pyroma now scores 10/10. - Runtime dep floors set to currently installed versions: astropy>=7.2.0, numpy>=2.4.4, pillow>=12.2.0, scipy>=1.17.1, rms-pdsparser>=2.0.0, rms-tabulation>=2.0.0, rms-vicar>=1.2.1. Rationale: pip-audit reports no advisories against any of these versions; pillow 12.2 is well above the CVE-2023-50447 cutoff (Pillow<10.2). Only pip itself shows advisories, which are not in scope. - Uncommented mypy>=1.0 and added pip-audit>=2.7 to dev extras. - Added [[tool.mypy.overrides]] with ignore_missing_imports=true for astropy.*, PIL.*, scipy.*, vicar, pdsparser, tabulation so the first mypy run in PR 3 doesn't drown in stub-missing errors. - Dropped [tool.pytest.ini_options].pythonpath=["src"] (no longer needed once the package is installed editable). - Changed addopts --cov=src -> --cov=picmaker to match [tool.coverage.run].source. - Added "*/picmaker.py" to coverage omit (anticipating PR 3 where picmaker.py collapses to a BC re-export module whose from-X-import-Y lines would otherwise inflate coverage). - Set [tool.coverage.report].fail_under=0 temporarily; PR 4 ratchets to floor(measured). - src/picmaker/picmaker.py: - Line 3373: return IOError(...) -> raise IOError(...). The original code constructed but did not raise the exception, so the function silently fell through to array_to_pil() with palette data lost. - process_images proceed scope bug: in the --movie branch, "if proceed:" referenced a nonexistent local. Changed to "if option_dicts[0]['proceed']:" with an assertion that all dicts in the list agree on proceed (main() forces this invariant by writing the same value into every dict). Verification: - pip install -e ".[dev]" succeeds. - python -c "import picmaker; print(picmaker.__version__)" prints 0.1.dev7. - pyroma . -> 10/10. - picmaker --help renders without error; flag set identical to the captured baseline. - scripts/run-all-checks.sh (pyroma + pymarkdown, the currently-enabled defaults) passes. - pip-audit --skip-editable reports no advisories on the project's runtime dependencies. - All previously-importable names from picmaker.picmaker remain importable; py.typed is now installed alongside the package. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2: tests + tiff16.py cleanup (#6) * PR2 commit 1: add conftest.py + tests/fixtures/ Adds the shared test scaffolding for the PR 2 safety net described in MODERNIZATION_PLAN.md: - tests/conftest.py with fixtures_dir / expected_dir / tiny_array. - tests/fixtures/*.recipe.py recipes (regenerable via tests/fixtures/regenerate_all.py) plus their generated binary outputs: - VICAR: cassini_iss, voyager_iss, galileo_ssi_a, galileo_ssi_b - FITS: hst_wfc3, hst_acs (5 HDUs), hst_wfpc2 (5 HDUs), nh_mvic - PDS3: pds3_sample.IMG (512-byte records) - PIL: small_grayscale.png, small_rgb.png, small_tiff16.tiff - Malformed: malformed_pickle.bin, malformed_numpy.bin, corrupt_vicar.vic, corrupt_fits.fits - tests/fixtures/two_versions.txt for the --versions round-trip test. Detection paths verified by running read_one_image_array against each fixture inside the dev venv (rms-vicar, astropy, pdsparser). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 2: add unit tests for paths/geometry/pil/enhance/color/zebra 86 passing tests, 3 xfailed (xfails gate PR 3's mutable-default fix). Coverage: - test_paths.py: find_common_path + get_outfile (replace policies, strip, suffix, extension swap, directory tree creation). - test_geometry.py: circle_mask (diameters 1/3/5/8/9), slice_array (band average, slice axes, valid mask, NaN propagation), crop_array (interior, all-zero, axis-selective). - test_geometry_extra.py: wrap_image (horizontal/vertical sections, named gap color), pad_image (interior pad, no-pad short-circuit, named color), get_size (size/scale/frame/frame_max paths), resize_image (gray + RGB), rotate_array_rgb (all 5 named rotations + unknown raises). - test_pil_utils.py: array_to_pil↔pil_to_array round-trip for grayscale and RGB; documents the rescale-flag dead-code bug at picmaker.py:3029. - test_enhance.py: get_limits (integer half-pixel extension, explicit limits, percentile bands); _percentile_lookup (below 0, above 100, interpolation midpoint, exact match); apply_colormap (grayscale no-colormap, 3-entry grayscale LUT, 3-entry color LUT). - test_color.py: ColorNames.lookup positive cases (red/GhostWhite/ whitespace-tolerant), negative cases (unknown/empty/hex/non-str), RGB parenthetical and bracket expressions. - test_apply_gamma.py: gamma=1 identity (2D + 3D RGB), gamma=2.0 and 0.5 exact boundary values, NaN/Inf safety. - test_mutable_defaults.py: xfail(strict=True) gates for the mutable defaults `strip=[]`, `pointer=['IMAGE']` in images_to_pics/get_outfile — PR 3's fix will flip these green. - test_zebra.py: fill_zebra_stripes left edge, right edge, in-place modification, no-op on fully-nonzero arrays. Documents two pre-existing bugs in tests rather than fixing them (PR 3 scope): pil_to_array L-mode rescale flag is dead-code; ColorNames hex codes are unsupported. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 3: add integration tests for io/pipeline/cli-flag semantics 90+ new tests covering: - test_io.py: read_one_image_array end-to-end on every instrument fixture (cassini, voyager, galileo_ssi_a/b, hst_wfc3, hst_acs, hst_wfpc2, nh_mvic); malformed cascade reaches IOError("Unrecognized image file format ..."). - test_tinted_colormap.py: cassini substring chain (CL1+GRN/CL1+RED/CL1+UV/ BL+GRN combo/unknown), voyager dict lookup, galileo dict lookup, NH dict lookup, HST F606W special case, F555W wavelength inference, unknown HST filter returns None. - test_hst_filter_tuple_normalization.py: CL1/CL2/N/A/CLEAR* normalization paths at picmaker.py:2314-2327. - test_unknown_filter_warning.py: captures current print()-to-stdout behavior + xfail gate for PR 3's logger.warning() conversion. - test_pipeline.py: images_to_pics with default options, gamma=2.0, percentile stretch, tint, rot90, --movie (via process_images), 16-bit TIFF extension, --hst (ACS/WFC + WFPC2 branches). - test_frame_max.py: --frame=512,512 --frame_max=50 on 16x16 input caps output dims at <=256. - test_io_extra.py: read_array on PNG/RGB/16-bit TIFF, read_pil on PNG/TIFF, write_pil quality round-trip, PDS3 reader marked skip pending PR 3 (pdsparser API mismatch documented inline). - test_warning_elevation.py: corrupt_fits.fits triggers astropy warning → elevated to error → cascade continues to PIL → final IOError. - test_versions_override.py: --replace=none overrides versions-line --replace=all; two_versions.txt produces two distinct output files. - test_alt_strip_alias.py: every kebab/snake spelling parses identically (--alt-strip / --alt_strip, --gap-size / --gapsize, --gap-color / --gapcolor, --alt-pointer / --alt_pointer, --trim-zeros / --trimzeros). - test_overlap_vs_overlaps.py: --overlap=0.1 ≡ --overlaps 0.1 0.1 (byte-identical outputs). - test_pickle_iolost_propagates_to_final_error.py: documents current pre-PR3 pickle-IOError propagation + xfail gate for the PR 3 fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 4: add subprocess-based test_cli Five smoke tests cover the user-facing CLI surface: - `picmaker --help` exits 0 and lists representative flags. - `picmaker --help`'s flag set matches tests/fixtures/.baseline-flags.txt byte-for-byte (gates PR 3's argparse migration: same flags must remain). - `picmaker` with no args exits 0 (no work to do). - `picmaker /nonexistent.IMG` exits non-zero and surfaces the underlying FileNotFoundError via sys.excepthook (verifying picmaker.py:787-789). - `picmaker --versions tests/fixtures/two_versions.txt cassini_iss.vic` produces two output files (_v1.jpg + _v2.tif). test_api_compat.py is deferred to PR 3 commit 5 per MODERNIZATION_PLAN.md (the BC re-export module only exists then). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 5: drop tiff16 driver block, add tests/test_tiff16.py Atomically replaces the legacy `__main__`-only test scaffolding at tiff16.py:474-680 (the `from vicar import *`, `from optparse import OptionParser`, dead `ROTATE_DICT`, and the four `*_test()` driver functions) with a proper pytest module. tiff16.py now ends at line 473 with `raise IOError("Not a recognized TIFF16 file.")` — the last legitimate production line. tests/test_tiff16.py covers: - 16-bit grayscale round-trip (WriteTiff16 → ReadTiff16 → array_equal). - 16-bit RGB round-trip. - 16-bit palette round-trip with translate=True (write-as-RGB) and translate=False (native palette TIFF) — both currently xfail(strict) because tiff16.py:96 uses `palette != None` instead of `is not None`, which raises ValueError under modern numpy. PR 3 will fix this. - `up=True` flip round-trip. - ReadTiff16 raises IOError on a non-TIFF file. The deletion and the new test file land in a single commit so there is no window where the driver block exists but is unreachable from tests; PR 3's agent can rely on tiff16.py being trim-shaped from this commit forward. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 6: snapshot the unvectorized pipeline output (63 fixtures) Adds the baseline that PR 3's vectorization commit will diff against. Without this, "PR 3 is byte-identical to the legacy pipeline" is unverifiable. Files added: - tests/generate_snapshots.py — regenerator script. Iterates over the 7 instrument fixtures × 9 representative option combos, calls images_to_pics with the combo's kwargs, logs each as ok/FAIL, and emits tests/snapshots_index.py with the surviving (fixture, slug, ext) tuples. Failures don't halt the script — they're filtered out of the emitted index. - tests/fixtures/expected/ — 63 generated snapshots: - 7 fixtures × 9 combos = 63 entries. - Combos: default, gamma2, pct5_95, colormap_red_blue, tint, rot90, frame_128_pad, frame_max_50, twobytes_tiff. - tests/snapshots_index.py — auto-generated index (committed so test collection doesn't require running the generator first). - tests/test_snapshots.py — parametrized test asserting produced_bytes == fixture_bytes for every entry in SNAPSHOTS. All 63 tests pass against HEAD. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 commit 7: flip ENABLE_PYTEST=true in run-all-checks.sh PR 2 now has a real test suite (235 collected, 232 pass, 2 xfail, 1 skip documented). Enable pytest by default in the orchestrator so a bare `scripts/run-all-checks.sh` exercises it without needing the env flag. CLAUDE.md's note about ENABLE_PYTEST being off by default is now stale — PR 3/4 will rewrite CLAUDE.md alongside the larger restructure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR2 cleanup: ruff --fix tests/ + manual lint cleanups Final cleanup pass on the test suite so `ruff check tests/` is clean: - Auto-fixes from `ruff --fix` (I001 import sort, F401 unused pytest import, RUF100 redundant noqa). - Split chained `assert a and b` into separate asserts (PT018) in test_geometry, test_geometry_extra, test_overlap_vs_overlaps, test_tinted_colormap. - Drop unused tuple unpacking targets (RUF059) in test_geometry, test_geometry_extra, test_io_extra. - Move `import pytest` to the top of test_tiff16.py (E402). - Replace U+00D7 (×) with `x` in test_versions_override docstring (RUF002). All 248 tests still pass; ruff (default rules from pyproject.toml) is clean against tests/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * New Pds3Label init * Label fix * Split fixture-generator scripts out of tests/fixtures/ `tests/fixtures/` now contains only data (the binary/text files tests consume); the generator scripts live in `tests/fixture_recipes/`. - tests/fixtures/ — *.vic, *.fits, *.IMG, *.bin, *.png, *.tiff, two_versions.txt, .baseline-*.txt, expected/. - tests/fixture_recipes/ — 16 *.recipe.py files, regenerate_all.py, generate_snapshots.py. Each recipe's `OUT` is rewritten from Path(__file__).parent / '<name>.<ext>' to Path(__file__).parent.parent / 'fixtures' / '<name>.<ext>' so the output still lands in tests/fixtures/ regardless of where the recipe lives. generate_snapshots.py now: - reads inputs from tests/fixtures/<name> (via FIXTURES const), - writes outputs to tests/fixtures/expected/ (via EXPECTED const), - emits tests/snapshots_index.py (kept next to test_snapshots.py so the `from snapshots_index import SNAPSHOTS` import resolves without sys.path gymnastics). test_snapshots.py docstring and error-message references updated to point at the new path. Verified: regenerate_all.py + generate_snapshots.py both produce byte-identical outputs from the new location; 248 tests pass / 1 skipped / 7 xfailed; pyroma + pymarkdown green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop "*/picmaker.py" from coverage omit so PR 2's baseline is real PR 1 added the omit entry forward-looking: after PR 3 commit 5 collapses picmaker.py into a BC re-export module, its import-time-executed `from X import Y` lines would otherwise count as covered for free (via `test_api_compat`'s import side-effect), inflating coverage to ~100% without exercising any logic. See §A16. The cost in PR 2, however, is that picmaker.py *is* the codebase — the omit hides the entire safety-net's coverage signal and makes the "PR 2 covers the legacy pipeline" claim unfalsifiable. With the omit gone: Name Stmts Miss Cover src/picmaker/__init__.py 9 5 44% src/picmaker/colornames.py 36 1 94% src/picmaker/picmaker.py 1402 642 52% src/picmaker/tiff16.py 187 21 86% TOTAL 1634 669 56% That's a meaningful baseline against which PR 3 commit 9's vectorization can be judged. PR 3 commit 5 (when picmaker.py becomes a re-export shim) must re-add the entry; the plan now lists that as an explicit step under commit 5 and the §A16 description has been updated to reflect the temporary absence. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix: apply CodeRabbit auto-fixes Fixed 21 file(s) based on 40 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * Tighten 8 PR2 tests from external code review Triage of a stylistic-review pass dropped on the PR. Most findings contradict project conventions (no docstrings on tests per CLAUDE.md; Sphinx :func: roles in non-autodoc files; line-number refs are deliberate PR3-navigation aids); see PR thread for the rejection list. Eight findings were genuine correctness/strength improvements: - conftest.py: drop "(populated by PR 2 commit 6)" mod-history note in the expected_dir docstring. - test_geometry.py: test_diameter_8 asserts mask.shape == (8, 8), not just shape[0] == 8. - test_geometry_extra.py: test_named_pad_color asserts the exact blue tuple (0, 0, 255) instead of "not red"; test_named_rotations_run parametrizes (name, expected_shape) and asserts equality, so ROT90/ROT270 vs FLIP/ROT180 dimensions are individually checked. - test_frame_max.py: tighten the loose `<= 256` check to the exact produced size (8, 8), with a comment explaining the frame_max%-of- input semantics that legacy get_size implements. - test_hst_filter_tuple_normalization.py: replace the tautological `result is not None or result is None` self-canceling assert with a concrete equality assertion against the wavelength-inference output ([(0,0,0),(255,60,60),(255,255,255)] at 555 nm). - test_alt_strip_alias.py: assert byte-equality of the two output files, not just name equality — verified locally that every alias pair produces byte-identical jpeg output. - test_snapshots.py + generate_snapshots.py: remove the duplicated slug→kwargs branch in the test. generate_snapshots.py now emits a KWARGS_BY_SLUG dict next to SNAPSHOTS in tests/snapshots_index.py, and the test imports both — single source of truth. - test_io_extra.py: fix the unpack in the (currently-skipped) read_pds_labeled_image_array test from a 2-tuple to the actual 3-tuple (arr, default_is_up, filter_info), so the test runs cleanly when PR3 unskips it. All 248 tests still pass (1 skipped, 7 xfailed); ruff clean; default orchestrator green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Refresh PDS3 skip reason after Pds3Label constructor patch Commits db0ea1e + 8f11fce switched read_pds_labeled_image_array's constructor call from the removed PdsLabel.from_file() back to PdsLabel() — but the body still iterates `for node in label` expecting `node.name` and references `pdsparser.PdsOffsetPointer`, which now raises AttributeError instead of the original symptom. Update the test's skip reason so the inline rationale matches the current failure mode; the test still skips for the same overall "PR 3 will rewrite the reader" reason. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * PR3: decompose picmaker into per-concern modules + argparse + cleanup (#7) * PR3: decompose picmaker into per-concern modules + argparse + cleanup Splits the ~3500-line `src/picmaker/picmaker.py` god module into purpose-specific leaf modules and an `instruments/` subpackage, swaps the CLI from optparse to argparse, and applies the hygiene fixes the modernization plan catalogues. The legacy import path `from picmaker.picmaker import X` still resolves to the same function object as `from picmaker import X` (identity-equal — gated by `tests/test_api_compat.py`). Modules created - cli.py — argparse rewrite of the optparse `main()`. Every flag spelling preserved, including the underscore aliases (`--alt_strip`, `--alt_pointer`, `--gapsize`, `--gapcolor`, `--trimzeros`, `--frame_max`). `--versions FILE` semantics, the 13-check mutex / value-validity chain, and `sys.excepthook`-based error reporting all match the legacy module byte for byte (verified against `tests/fixtures/.baseline-flags.txt`). - pipeline.py — `find_common_path`, `process_images`, `images_to_pics`. - io.py — `read_image_array`, `read_one_image_array`, `read_pds_labeled_image_array`, `read_pil`, `read_array`, `get_outfile`. The pickle branch's `except IOError as e: raise e` is gone; the VICAR / FITS magic-byte branches now also tolerate `OSError` so a missing-file path falls through to the final `OSError("Unrecognized image file format: ...")` instead of leaking the per-stage error message. - enhance.py — `fill_zebra_stripes`, `get_limits`, `_percentile_lookup`, `apply_colormap`, `apply_gamma`. Algorithms preserved verbatim so PR 2's byte-identical snapshots stay green. - geometry.py — `circle_mask`, `slice_array`, `crop_array`, `rotate_array_rgb`, `get_size`, `resize_image`, `wrap_image`, `pad_image`, and the private `_get_size_for_*`/`_resize_one_image` helpers. - color.py — `tinted_colormap` only; delegates to `instruments.lookup` and re-exports the `RGB_BY_NM`/`RFUNC`/`GFUNC`/`BFUNC` constants from `_rgb.py`. - pil_utils.py — `array_to_pil`, `pil_to_array`, `write_pil`, and the `_one_pil_to_array` helper. - _filters.py — `FILTER_DICT` + `filter_image`. - _rgb.py — leaf module holding the wavelength→RGB tables; placed outside `color.py` so `instruments/hst.py` can import without a cycle (`color → instruments → hst → _rgb (leaf)`). - instruments/{cassini,voyager,galileo,hst,nh}.py — each implements the 4-method protocol (`detect_vicar`, `detect_fits`, `matches`, `tint_for`) so the dispatch tables in `io.read_one_image_array` and `color.tinted_colormap` are data, not control flow. Backward compatibility - `src/picmaker/picmaker.py` collapses to a re-export shim with an explicit `__all__` covering 53 legacy names (every public function, the documented private helpers, the sibling-package re-exports `ColorNames` / `WriteTiff16` / `ReadTiff16` / `VicarImage` / `VicarError`, and the four instrument dicts `VOYAGER_ISS_DICT`, `NH_MVIC_DICT`, `GALILEO_SSI_DICT`, `GALILEO_SSI_NAMES`). - `src/picmaker/__init__.py` mirrors the public surface so `from picmaker import images_to_pics` works. - `tests/test_api_compat.py` asserts identity-equality between the two paths. Hygiene fixes (per MODERNIZATION_PLAN §"Bug fixes and autofixes") - Shebangs dropped from `tiff16.py` and `colornames.py`. - Mutable defaults flipped: `strip=None` and `pointer=None` with the list normalized inside `images_to_pics` / `get_outfile`. - `from struct import *` in `tiff16.py` → `from struct import pack, unpack`. - Pillow deprecation: `Image.FLIP_*` / `Image.ROTATE_*` → `Image.Transpose.*` (25 sites in `tiff16.py`). - Lose-traceback fix: `except IOError as e: raise e` deleted from the pickle branch of `read_one_image_array`. Behavior change documented in `test_pickle_iolost_propagates_to_final_error.py`. - `warnings.warn` call in `get_outfile` now passes `UserWarning` explicitly. - `print('******UNKNOWN FILTER:', ...)` in the HST tint dispatcher is now `logger.warning(...)`. CLI migration notes - Argparse uses lowercase `usage:` (optparse used capital `Usage:`). Updated `tests/test_cli.py::test_help_exits_zero_and_lists_flags` to accept either form. - Argparse does NOT accept the mixed `--option=N M` form for `nargs=2` options (it consumes only the `N` and leaves `M` as positional). Adjusted `tests/test_overlap_vs_overlaps.py` to use the space-separated form. - Flag set matches `tests/fixtures/.baseline-flags.txt` exactly. Documentation, packaging, and tooling - `pyproject.toml`: - `[project.scripts]` flipped from `picmaker.picmaker:main` to `picmaker.cli:main`. - `[tool.coverage.run].omit` adds `*/picmaker.py` so the BC re-export module's `from X import Y` lines don't inflate coverage. - `[tool.ruff.lint.per-file-ignores]` adds `A002` for the preserved `filter` kwarg, `N806` for the preserved camelCase locals in `pipeline.py`/`geometry.py`, and broad legacy-style ignores for `tiff16.py` and `colornames.py`. - `[[tool.mypy.overrides]]` adds permissive blocks for `picmaker.tiff16`, `picmaker.colornames`, and pytest test modules. - `scripts/run-all-checks.sh` defaults to `ENABLE_RUFF_CHECK=true` and `ENABLE_MYPY=true`. The mypy step runs only against `src/` because the bare `tests/test_*.py` filename pattern doesn't match mypy's module-pattern syntax without a `tests/__init__.py`. - `docs/module.rst` switches from a single `automodule:: picmaker` to per-leaf-module autodocs. Verification - `scripts/run-all-checks.sh -s` passes: ruff check, mypy, pytest (261 passed, 1 skipped, 2 xfailed), pyroma, pymarkdown. - `picmaker --help | grep -oE -- '--[a-z_-]+' | sort -u` matches `tests/fixtures/.baseline-flags.txt`. - `picmaker --versions tests/fixtures/two_versions.txt --directory /tmp/out tests/fixtures/cassini_iss.vic` produces both `cassini_iss_v1.jpg` and `cassini_iss_v2.tif`. Sphinx `-W` still fails on 4 pre-existing infrastructure warnings (`code_of_conduct.md` include path, `index.rst` title underline, two `toctree.not_included` entries); those are addressed in PR 4 along with `ENABLE_SPHINX=true`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Annotate legacy tiff16 + colornames; mypy now strict-clean on tests too The PR3 baseline hid the WriteTiff16 / ReadTiff16 / ColorNames.lookup untyped-call errors behind per-module mypy ignores. Replace those with real annotations so `mypy src tests` is strict-clean without `disable_error_code` for `no-untyped-call`. Source changes - src/picmaker/tiff16.py: type WriteTiff16, ReadTiff16, and my_assert. - src/picmaker/colornames.py: type ColorNames.lookup. - Remove the per-callsite `# type: ignore[no-untyped-call]` shims from io.py / pil_utils.py / enhance.py / geometry.py. - pyproject.toml: drop `no-untyped-call` from the picmaker.tiff16 / picmaker.colornames mypy override. - src/picmaker/_rgb.py: type RGB_BY_NM as NDArray[np.float64] and the three Tabulation splines as Tabulation instead of Any. - src/picmaker/cli.py: drop modification-history references from `_separate_files_and_dirs`, `_normalize_and_validate`, and `main()` docstrings; expand `_normalize_and_validate`'s docstring with proper Args: / Returns: sections. Test changes - tests/test_api_compat.py: add `-> None` to every test function. - tests/test_mutable_defaults.py: type the `_default` helper. - tests/test_enhance.py: annotate the `tiny_array` fixture parameters. - tests/test_io.py, tests/test_geometry_extra.py: replace bare `tuple` with `tuple[str, str, str]` / `tuple[int, ...]`. - tests/test_cli.py, tests/test_alt_strip_alias.py, tests/test_overlap_vs_overlaps.py, tests/test_versions_override.py: parametrize CompletedProcess with `[str]`. - tests/test_unknown_filter_warning.py: parametrize `pytest.CaptureFixture` with `[str]`. - tests/test_io_extra.py: split the tuple-unpack so the typed-None return doesn't trip mypy. - tests/test_color.py: add a localized `# type: ignore[arg-type]` for the intentional non-string lookup in `test_non_string_raises_typeerror`. - tests/snapshots_index.py + tests/fixture_recipes/generate_snapshots.py: type SNAPSHOTS / KWARGS_BY_SLUG / COMBOS / `_generate_one`. Build script - scripts/run-all-checks.sh: mypy step now reads `src tests`. Verification - `scripts/run-all-checks.sh -s` with ruff, mypy, pytest, pyroma, pymarkdown all green. - 261 tests pass (1 skipped, 2 xfailed); CLI baseline unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Docstrings: Args: -> Parameters: per project rules; add test docstrings Section 6 of `.cursor/rules/python_best_practices.mdc` explicitly mandates Google-style docstrings using ``Parameters:`` (not the ``Args:`` variant that Google's original guide allows). Bring every PR-3-touched docstring in line: Source modules (15 sites): - src/picmaker/{cli,color,enhance,geometry,io,pil_utils,pipeline}.py - src/picmaker/_filters.py - src/picmaker/instruments/__init__.py Tests: - tests/test_cli.py: docstrings for `_run`, every test function. - tests/test_overlap_vs_overlaps.py: expand `test_overlap_and_overlaps_byte_identical` to a black-box-testable docstring explaining the wrap-path invocation. - tests/test_unknown_filter_warning.py: docstrings for both test functions describing the logger expectations and the negative capsys assertion. - tests/test_api_compat.py: docstrings for every identity-equality assertion test, including the package-level smoke test. No behavior changes; ruff/mypy/pytest still green (261/1/2). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix Sphinx warnings + drop tiff16/colornames mypy override Sphinx - Restore CODE_OF_CONDUCT.md at the repo root (accidentally removed in commit 9f55adf). docs/code_of_conduct.md's `:include:` directive needed it. - docs/index.rst: extend the title underline to match "Welcome to the Documentation for rms-picmaker!" (46 chars, not 42) so docutils stops emitting WARNING: Title underline too short. - docs/index.rst: add `contributing` and `code_of_conduct` to the master toctree so those documents are no longer orphaned (toc.not_included warnings). - scripts/run-all-checks.sh: flip ENABLE_SPHINX from `false` to `true` so the documentation build is part of the default check set and any future warning is surfaced as a hard failure. Update the ENABLE_MYPY docstring entry to match the actual default (true). Mypy - Drop the per-module override for `picmaker.tiff16` and `picmaker.colornames`. The remaining errors that surfaced are real: - src/picmaker/colornames.py: the lookup() method reused the local variable name `test` for two incompatible types (`str` and `re.Match | None`). Renamed the regex result to `match` so each variable has a single, narrowable type. Also corrected `if test == None:` to `if match is None:`. - src/picmaker/geometry.py: - `Image.NEAREST` / `Image.LANCZOS` → `Image.Resampling.NEAREST` / `Image.Resampling.LANCZOS` (the non-deprecated PIL spelling). - `list.append(_resize_one_image(...))` was a real bug — calling the unbound `list` class method instead of `result.append(...)`. The line-level `# type: ignore[call-arg]` is gone; the call now appends to the local `result` list. Verification - `scripts/run-all-checks.sh -s` is fully green: ruff, mypy, pytest (261 / 1 skipped / 2 xfailed), pyroma, sphinx, pymarkdown. - Only one `# type: ignore` remains in the project, in tests/test_color.py for a deliberate non-string lookup that exercises the TypeError branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Remove "legacy" / "backwards compatibility" wording from documentation Project rule .cursor/rules/python_best_practices.mdc §4 forbids modification-history comments and §6 forbids backwards-compatibility references in docstrings. Bring every docstring / inline comment in the picmaker package and rendered docs in line with that. Source modules - src/picmaker/_rgb.py: docstring describes what the tables are for and why this module imports nothing from the rest of the package (cycle-prevention) rather than citing a pre-PR-3 line range. - src/picmaker/__init__.py: docstring describes the public surface and points at the two top-level entry points. - src/picmaker/picmaker.py: docstring states what the module is (an alternate import path that re-exports the public API) without the words "legacy" or "backward compatibility". - src/picmaker/cli.py: docstring describes the CLI entry point and the three error-handling layers; the obsolete file-level `# ruff: noqa: A002` is gone (pyproject already has the per-file ignore). - src/picmaker/pipeline.py: module + images_to_pics docstrings drop the BC framing; the `results` declaration comment now explains WHY `results` lives outside the per-filename loop (reuse path). - src/picmaker/color.py: module docstring drops the "callers that imported them from this module before PR 3" line; the filter normalization comment now describes the rule itself. - src/picmaker/pil_utils.py: 8-bit grayscale comment now describes why the rescale flag is ignored, not what the legacy code did. - src/picmaker/instruments/{cassini,voyager,galileo,hst,nh}.py: drop "Source: picmaker.py (pre-PR-3) lines N-M" headers; expand every public function's docstring with Parameters: and Returns: sections describing the actual behavior (and Raises: where it raises). HST's `tint_for` now documents the per-detector wavelength-scaling rules in a single bullet list. Documentation - docs/module.rst: drop the "legacy ... backward-compatibility re-export shim" sentence; the package overview now just lists what's documented below. No behavior changes; ruff, mypy, pytest, pyroma, sphinx (-W), pymarkdown all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * PR4: activate CI, OIDC publish, User Guide (#8) * PR4: activate CI, switch publish to OIDC, add User Guide CI (.github/workflows/run-tests.yml) - Re-enable pull_request / push / schedule / workflow_dispatch triggers. - Lint job runs ruff check, mypy, sphinx -W, pymarkdown, pip-audit on the project's declared deps. Python pinned to 3.10 to match target-version = "py310". - Test job re-enabled with matrix 3.10 / 3.11 / 3.12 / 3.13 and the existing codecov upload (3.13 only, non-fork). Publish workflows (publish_to_pypi.yml, publish_to_test_pypi.yml) - Switch to PyPI Trusted Publishers / OIDC: add permissions.id-token: write, drop password / user secrets, bind each job to its environment (pypi / testpypi). A pre-merge manual step on the PyPI side is required (documented in CONTRIBUTING.md). Coverage (pyproject.toml) - Ratchet [tool.coverage.report].fail_under from 0 to 59 (floor of the 59.24% measured against the full pytest run on the post-PR-3 codebase). User Guide (docs/user_guide.rst + docs/_static/user_guide/*) - New top-level RST page wired into docs/index.rst toctree with all 12 required sections: overview, installation, quick start, full CLI reference (one table per --help group), input-format cascade, per-instrument tints (Cassini, Voyager, Galileo, HST, NH MVIC), output formats, enhancement / geometry pipelines, --versions form, programmatic usage, troubleshooting. - 23 example thumbnails generated under docs/_static/user_guide/ by tests/fixture_recipes/generate_user_guide_thumbnails.py, modelled on PR 2's generate_snapshots.py so the gallery stays reproducible. - docs/conf.py enables html_static_path so the gallery is copied into the Sphinx build. CLI / doc drift gate (tests/test_cli.py) - New test_user_guide_documents_every_cli_flag asserts every long flag in tests/fixtures/.baseline-flags.txt appears in the User Guide. A new picmaker flag with no docs now fails CI. CONTRIBUTING.md - "Releasing to PyPI" section with the five-step OIDC trusted-publisher setup (PyPI + TestPyPI + GitHub Environments + tag-push release). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Raise test coverage from 59% to 90% and ratchet fail_under Coverage gains by module - cli.py: 5% -> 91% (test_cli_unit.py — in-process tests for _build_parser, _separate_files_and_dirs, _normalize_and_validate, main()). - io.py: 50% -> 87% (test_io_branches.py + test_pds3_reader.py + the read_pds_labeled_image_array port to the current pdsparser API). - pipeline.py: 50% -> 87% (test_pipeline_branches.py — mutex checks, movie mode, --proceed, HST mosaic, --versions PDS3 label branch). - pil_utils.py: 66% -> 98% (test_misc_branches.py — 16-bit RGB path, PIL list handling, parent-dir creation). - enhance.py: 69% -> 90% (trim_zeros, footprint, below/above/invalid paints, named two-stop colormap, uniform-input limit fallbacks). - geometry.py: 70% -> 90% (wrap_ratio wide/tall, frame_max cap, rot90/180/270/fliptb, crop_array 2-D + uniform input). - instruments: per-module 67-83% -> 96-100% (parametrised Cassini filter chain, Voyager/Galileo/NH detect-fail predicates, HST POL0L/POL0S/FQUV/FQCH4 pinning paths). - tiff16.py: 86% -> 88% (RGB write, 3-D grayscale, big-endian byteorder, --up, transpose=ROTATE_90). - __init__.py: 71% -> 100% (pragma: no cover on the version-metadata fallback that only fires when the package isn't installed). Code change: src/picmaker/io.py::read_pds_labeled_image_array - Port to the current pdsparser API: the function previously referenced `pdsparser.PdsOffsetPointer` and `node.name` / `node.value`, which no longer exist (the function raised AttributeError on every PDS3 file in the test suite). The rewrite uses the dict-style `label[key]` API with the companion `<pname>_offset` / `<pname>_unit` keys that pdsparser now emits for tuple pointers. Pointer-value forms covered: attached int, detached str, tuple-with-filename. Unit is RECORDS by default; `<BYTES>` is recognised. Coverage gate - pyproject.toml: ratchet [tool.coverage.report].fail_under from 59 to 90. Total project coverage is now 90.31% across 466 tests (+204 over the previous baseline). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * CI: run tests on every branch, not just main Drop the `branches: [main]` filter from both `pull_request` and `push` triggers so the workflow fires on any branch. The schedule and workflow_dispatch entries are unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Drop Python 3.10; expand CI matrix to macOS and Windows astropy 7.x requires Python >= 3.11, which collides with the `requires-python = ">=3.10"` declaration and trips the resolver on Python 3.10 ("Could not find a version that satisfies the requirement astropy>=7.2.0"). Picmaker only uses `astropy.io.fits.open()` and the standard HDU access surface, but the pin already captured 7.2.0 from the dev venv and lowering it would be a step backwards on security fixes. pyproject.toml - `requires-python = ">=3.11"` (was >=3.10). - Drop `Programming Language :: Python :: 3.10` classifier. - `[tool.ruff].target-version = "py311"` (was py310). .github/workflows/run-tests.yml - Lint job pinned to Python 3.11 to match target-version. - Test matrix: drop 3.10; add macOS and Windows runners so the cross-platform behaviour the README/classifiers promise is now actually exercised. Matrix is 3.11 / 3.12 / 3.13 x ubuntu / macos / windows (= 9 jobs). CONTRIBUTING.md / docs/user_guide.rst - Python 3.10+ -> Python 3.11+ in the compat copy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Iterate label.dict instead of label.keys() in PDS3 reader `pdsparser.Pds3Label` implements `__getitem__`, `__contains__`, `keys()`, `items()`, etc. but not `__iter__`. Python's old-style iteration protocol then falls back to `obj[0]`, `obj[1]`, …, which raises `KeyError: 0` at the first probe — that's why `for key in label:` crashes on a freshly constructed label. The class exposes `label.dict` directly (a plain `dict` assigned in `Pds3Label.__init__`), so iterating *it* is both clean and removes the `noqa: SIM118` we were carrying for the `label.keys()` workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Use label.dict directly throughout the PDS3 reader `Pds3Label` proxies every dict operation through to its underlying `.dict` attribute, which is a plain `dict`. Pull it out once into a local `label_dict` and use it directly — `label_dict[k]`, `label_dict.get(k, default)`, `pname in label_dict` — instead of going through the Pds3Label proxy for each access. No behaviour change, just clearer that the function is operating on a plain dict. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Move PyPI release docs out of CONTRIBUTING.md into RELEASING.md CONTRIBUTING.md is contributor-facing — release-engineering setup is maintainer-internal and doesn't belong there. Split the OIDC trusted- publisher steps into a new repo-root RELEASING.md (the conventional location for Python project release docs). Also fix the URLs while moving: a brand-new project that has never been uploaded has no per-project settings page on PyPI, so the previous instructions linked to a 404. The pending-publisher form at `pypi.org/manage/account/publishing/` (and the TestPyPI equivalent) is the right entry point for a first release; PyPI promotes the pending publisher to a regular one on the first successful upload. - CONTRIBUTING.md: drop the "Releasing to PyPI" section. - RELEASING.md: new file with the one-time-setup steps (using the pending-publisher URL) plus the cut-a-release checklist. - publish_to_pypi.yml / publish_to_test_pypi.yml: comment references CONTRIBUTING.md -> RELEASING.md. - scripts/run-all-checks.sh / run-tests.yml: include RELEASING.md in the pymarkdown scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Review pass: spelling, Sphinx role, docstrings, regex Apply the still-valid findings from the latest review and skip the rest with rationale. Applied - docs/user_guide.rst: replace British "colour"/"coloured" with American "color"/"colored" (9 occurrences) to match the rest of the user-facing docs. - tests/fixture_recipes/generate_user_guide_thumbnails.py: cross- reference :func:`picmaker.pipeline.images_to_pics` from the _generate_one docstring; add a docstring to main() covering purpose, side effects, and return value; drop the now-redundant `from __future__ import annotations` (requires-python is 3.11+ so every annotation syntax in this file is native). - tests/test_cli.py: widen the user-guide flag-extraction regex to `r'--[a-z0-9_-]+'` so numeric flags such as `--16` are matched when the baseline is regenerated to include them. Skipped - Sphinx role conversion for `vicar.VicarImage`, `astropy.io.fits.open`, `PIL.Image.open`, `detect_vicar`, `detect_fits`, `rms-pdsparser`, `^IMAGE`, `--pointer`, tuple field names. The project's `intersphinx_mapping` covers only python / numpy / matplotlib, so adding `:func:` / `:class:` roles for unresolvable symbols would emit reference-target-not-found warnings that the `-W` Sphinx build promotes to fatal errors. - Renaming HERE / FIXTURES / OUT_DIR to _HERE / _FIXTURES / _OUT_DIR. The file is a standalone script (not imported as a module) and the module-level ALL_GALLERIES already uses the all-caps non-underscored convention; splitting the convention adds noise without semantic benefit. The _generate_one helper is underscored because it is genuinely module-private. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * CI: include OS in the test job name; drop the py prefix `Test rms-picmaker (py3.11)` becomes `Test rms-picmaker (ubuntu-latest, 3.11)` so the matrix dimension is visible at a glance in the checks list (relevant now that the matrix runs on ubuntu/macos/windows). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix Windows test: use tmp_path for nonexistent-file probe `test_falls_through_to_unrecognized_format_error` hardcoded the POSIX path `/nonexistent/...`, which on Windows is not absolute. astropy's FITS reader inside `read_one_image_array`'s cascade rejects the path with "Local file URL is not absolute" before the cascade ends, so the test fails with the wrong exception type. Construct the non-existent path via `tmp_path / 'does_not_exist.IMG'` so the absolute form is OS-native — same idiom already used in `tests/test_io_branches.py::test_unrecognized_missing_file_raises`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Modernization pass from codebase + test critiques (closes #10) Drives `CODEBASE_CRITIQUE.md` and `TEST_SUITE_CRITIQUE.md` to "fixed" or "issue filed" on every item that didn't require a major-version bump. Twelve follow-up issues (#9-#20) carry the deferred work. ## Production fixes - pil_utils._one_pil_to_array now honours rescale=True for 'L' mode PIL images, returning a float in [0, 1] instead of the raw uint8 array. Closes #10. - tiff16:281: missing `raise` on the version-byte mismatch (the function silently accepted non-TIFF files with a valid 'II'/'MM' byte order but wrong version field). Regression test added. - tiff16 WriteTiff16 `palette != None` → `palette is not None` (numpy ambiguous-truth bug). Removed the two strict-xfail palette tests that pinned the bug. - ReadTiff16 + WriteTiff16 file-handle leaks: both functions now wrap the open() in a `with` block. Surfaced by adding filterwarnings = ["error"] to pytest (ResourceWarning was the smoking gun). - colornames.ColorNames.lookup uses ast.literal_eval (not `eval`), raises KeyError on partial regex matches, and no longer shadows the builtin `tuple` local. Two new tests pin the boundary. - io.read_one_image_array chains the cascade-end OSError from ExceptionGroup('No reader matched', cascade_errors) so per-reader failure causes survive for diagnosis. - pipeline.process_images: assert→raise ValueError for the movie-mode proceed-consistency check (assert is stripped under python -O). - pipeline.images_to_pics: traceback.print_tb → logger.exception (deterministic ordering under pytest -n auto). - pipeline.find_common_path: switched to os.path.commonpath so the Windows path-separator bug is gone. - cli.main: print(this_dir)/print(dirpath) → logger.info (matches the pipeline verbose path). - __init__.py: except Exception → except PackageNotFoundError on the importlib.metadata fallback. - io.get_outfile: suffix annotation widened to `str | None` to match the documented None-accepting behaviour. - _filters.filter_image docstring corrected to reflect the actual KeyError-on-unknown-name semantics. ## API changes - New picmaker.options.PicmakerOptions dataclass owns all post-normalization mutex / value-validity checks via validate(); cli._normalize_and_validate and pipeline.images_to_pics both delegate to it. The legacy kwargs surface on images_to_pics is unchanged. - New picmaker.io.ReadResult NamedTuple (array3d, default_is_up, filter_info) returned by the reader cascade. Positional unpacking still works; .array3d-style attribute access now also works. - New picmaker.io.FilterInfo type alias for the (inst_host, inst_id, filter_name) | None union. - pipeline.images_to_pics `filter=` kwarg renamed to `filter_name=`; the CLI --filter flag is unchanged (dest='filter_name'). The PDS3-label local `filter_name` was renamed to `pds_filter_name` so it no longer shadows the parameter. ## Ruff cleanups - Removed per-file ignores for colornames.py (was E701, E711, N801, W291, W293, RUF012, SIM102, A001) and most of tiff16.py (was E701, N802, N806, E501, E711, W291, W293, SIM115; only N802 remains for legacy WriteTiff16 / ReadTiff16 casing). - Removed A001/A002/A003 ignores for cli.py / pipeline.py / options.py once the `filter` → `filter_name` rename landed. - All remaining per-file ignores have an inline reason. ## Tooling - pytest filterwarnings = ["error"] (with one targeted Pillow `Image.getdata` ignore tracked in #9). - bandit and vulture uncommented in [project.optional-dependencies].dev and wired into CI; both clean (bandit suppresses B301/B403 for the documented pickle-fallback path). - publish_to_pypi.yml / publish_to_test_pypi.yml now build on Python 3.11 (lowest supported) instead of 3.12. - run-tests.yml push: trigger scoped to `branches: [main]` so feature branches don't pay for the 9-cell matrix on every dev push. - docs/module.rst gains a "Options" section for PicmakerOptions. ## Test-suite hygiene - Split tests/test_misc_branches.py (842 lines) and tests/test_io_branches.py (283 lines) into nine focused files (test_pil_utils_branches.py, test_enhance_branches.py, test_geometry_branches.py, test_filters_branches.py, test_instruments_branches.py, test_tiff16_branches.py, test_package_init.py, test_io_cascade.py, test_pds3_reader_branches.py). - Removed `from __future__ import annotations` from 24 files. - Migrated `from picmaker.picmaker import X` to `from picmaker import X` in 17 files (test_api_compat.py stays — it's the BC verifier). - Tightened existence-only asserts: test_io.py / test_pipeline.py / test_io_extra.py now check exact shapes instead of `.ndim == 3` or `width > 0`. - Added regression tests for the tiff16 version-byte fix and the colornames partial-match / injection-payload rejection. ## Final check status - pytest: 471 passed - ruff: clean - mypy strict: clean (79 source files) - bandit: clean - vulture: clean - sphinx -W: clean - All snapshot tests still byte-identical. ## Filed follow-up issues (with labels) - #9 Cleanup / Useful / Medium — Pillow getdata migration + WriteTiff16 leak follow-up - #10 Bug / Important / Easy — _one_pil_to_array rescale (FIXED HERE) - #11 Big Project / Important / Medium — Prune picmaker.picmaker BC shim exports - #12 Big Project / Important / Hard — Split images_to_pics + complete RORO migration - #13 Cleanup / Minor / Easy — Derive VICAR/FITS_INSTRUMENTS from one list - #14 Cleanup / Minor / Medium — Rename picmaker.io to avoid stdlib shadow - #15 Cleanup / Minor / Easy — Make instrument FILTER_DICT private - #16 Cleanup / Useful / Easy — Stop mutating argparse.Namespace - #17 Cleanup / Useful / Medium — Standardize on pathlib.Path - #18 Enhancement / Minor / Medium — Vectorize fill_zebra_stripes - #19 Enhancement / Useful / Easy — Expand CI lint matrix to 3.11/3.12/3.13 - #20 Enhancement / Useful / Medium — Magic-byte dispatch for read_one_image_array Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add Developer Guide; move RELEASING content into it; add API source links ## New: docs/developer_guide.rst A maintainer-facing complement to the User Guide. Eight sections: 1. Repository overview — purpose, layout, who reads what. 2. Module-by-module description — one paragraph per leaf module under `src/picmaker/`, every public symbol linked via Sphinx cross-reference. 3. Pipeline flowchart — Mermaid diagram from CLI entry through to `write_pil`. 4. Major functions in prose — covers the CLI entry, option validation, reader cascade, path planning, per-image pipeline, enhancement / geometry / color / PIL bridges. Cross-refs all the way through. 5. Running the test suite. 6. Adding a new instrument — full step-by-step: - The four-function protocol (`detect_vicar`, `detect_fits`, `matches`, `tint_for`). - Per-step instructions: create module, register in `instruments/__init__.py`, fixture recipe, cross-instrument test, per-helper unit tests, snapshot append, user-guide update, check-suite run. - When-to-break-protocol notes covering Cassini's tint chain helper and HST's wavelength-inference branches. 7. Releasing to PyPI — moved verbatim from RELEASING.md (one-time trusted-publisher setup + cut-a-release steps). 8. Where to look next — pointers to user guide, module reference, GitHub issues, critique audits, contributor workflow. ## docs/module.rst Source-code links added at the top of each section, pointing at the relevant file under github.com/SETI/rms-picmaker. New sections for the previously-undocumented package surface: - `picmaker` (top-level package) - `picmaker.picmaker` (BC shim) - `picmaker._rgb` (wavelength → RGB tables) - `picmaker.instruments.*` (each per-mission module) - `picmaker.tiff16` - `picmaker.colornames` CLI automodule gains `:private-members: _build_parser, _separate_files_and_dirs, _normalize_and_validate` so the private helpers exercised by `tests/test_cli_unit.py` appear in the rendered API. ## docs/conf.py - Intersphinx mappings for PIL, astropy, sphinx, and pytest so external `:func:` / `:class:` / `:mod:` references resolve under `sphinx-build -n`. - `nitpick_ignore` for sibling-package types (`vicar.VicarImage`, `vicar.VicarError`, `tabulation.Tabulation`), module-level dunders (`__all__`, `__version__`), and a small set of internal data / helper names that autodoc cannot pick up cleanly (legacy `colornames.ColorNames`, the per-instrument `FILTER_DICT` / `FILTER_NAMES` constants without docstrings, the `_rgb` splines, the private `cli._normalize_and_validate`). - `autodoc_default_options['show-inheritance'] = True`. ## RELEASING.md Replaced with a five-line pointer to the new `docs/developer_guide.rst` §7. CI workflow comments updated to point at the new location. `pymarkdown scan` continues to lint the file. ## Source-side cross-reference fixes To satisfy `sphinx-build -n -b html`, qualified bare names in docstrings: - `instruments/{galileo,nh,voyager,hst}.py`: `:data:`FILTER_DICT`` etc. → `:data:`picmaker.instruments.galileo.FILTER_DICT`` etc. - `instruments/hst.py`: `:data:`RFUNC`` etc. → `:data:`picmaker._rgb.RFUNC``. - `_filters.py`: `:data:`FILTER_DICT`` → `:data:`picmaker._filters.FILTER_DICT``. Other doc-rule alignment: - `tiff16.WriteTiff16` and `tiff16.ReadTiff16` docstrings converted from the legacy `Inputs:` / `Return:` shape to Google style (`Parameters:` / `Returns:`). Fixes the long-standing docutils "Unexpected indentation" warning. - American spelling pass: `colour` → `color`, `coloured` → `colored`, `grey` (in prose) → `gray`, `honour(s)` → `honor(s)`, `normalise(d)` → `normalize(d)`, `behaviour` → `behavior`, `organis(ed)` → `organiz(ed)`. The X11 color-name table in `colornames.py` keeps the legacy `grey` aliases (they are literal X11 names, not English prose). - `instruments/__init__.py`: `VICAR_INSTRUMENTS` / `FITS_INSTRUMENTS` / `ALL_INSTRUMENTS` gain `list[ModuleType]` annotations and `#:`-style attribute docstrings so autodoc picks them up. - `__init__.py` module docstring uses fully-qualified targets (`picmaker.pipeline.images_to_pics`) so the references resolve outside the `picmaker` namespace. ## Final check status - pytest: 471 passed. - ruff: clean. - mypy strict: clean. - bandit: clean. - vulture: clean. - `sphinx-build -W -b html`: clean. - `sphinx-build -n -b html`: clean. - pymarkdown: clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Split Developer Guide into per-section files; drop RELEASING.md ## New layout The monolithic `docs/developer_guide.rst` is now an index page with a nested toctree. The major sections live as separate files under `docs/dev/`: - `docs/dev/repository_overview.rst` — purpose + repository tree. - `docs/dev/module_layout.rst` — module-by-module description with per-symbol Sphinx cross-references. - `docs/dev/pipeline.rst` — Mermaid flowchart + prose walk-through of CLI → reader cascade → enhancement → geometry → write. - `docs/dev/running_tests.rst` — pytest invocation, snapshot regeneration. - `docs/dev/adding_an_instrument.rst` — the four-function protocol and the seven-step recipe. - `docs/dev/releasing.rst` — moved from RELEASING.md verbatim (one- time trusted-publisher setup + cut-a-release flow). `docs/developer_guide.rst` is now a slim index page with the toctree plus the "Where to look next" section (pointers to the API reference, user guide, GitHub issues, and the two critique audits). ## RELEASING.md removed The file is deleted. The CI step list in `.github/workflows/run-tests.yml` and the optional-scan list in `scripts/run-all-checks.sh` were updated to stop referencing it. The two publish workflow comments now point at `docs/dev/releasing.rst` instead. ## Critique against `.cursor/rules/documentation.mdc` - American spelling: clean. - Sphinx cross-references for every API symbol in narrative: present in the new files (audited; the bare-name section titles like `:mod:`picmaker.cli`` are kept because they remain useful links in the rendered TOC). - `sphinx-build -W -b html`: clean. - `sphinx-build -n -b html`: zero warnings. - `pymarkdown scan` over `docs/ .cursor/ README.md CONTRIBUTING.md`: clean (the deleted `RELEASING.md` was dropped from the scan list). - Per-page TOC entries appear correctly in the rendered `developer_guide.html` (`toctree-l2` entries for each `dev/*.rst`). ## Full check status - pytest: 471 passed. - ruff: clean. - mypy strict: clean. - bandit: clean. - vulture: clean. - sphinx -W: clean. - sphinx -n: zero warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address reviewer findings Verified each comment against current code; applied the still-valid fixes. Skipped CODEBASE_CRITIQUE.md / TEST_SUITE_CRITIQUE.md items and anything related to pickle or path traversal per reviewer guidance. Skipped one further item (hst.py :data: → :func: for the Tabulation splines) because Tabulation instances are module-level data even though they are callable; :data: is the correct role. ## Production source - `docs/user_guide.rst:458`: `--filter` table row now shows `dest = filter_name` to match the actual argparse binding. - `src/picmaker/colornames.py`: added a class docstring for `ColorNames` and a full Google-style docstring for `ColorNames.lookup` (parameters / returns / raises / accepted input forms). Narrowed the return annotation from `Any` to `tuple[int, int, int]` and normalized list-style `[r, g, b]` parses to a tuple before returning so the two bracketing styles give identical output. Removed the now-unused `typing.Any` import. - `src/picmaker/instruments/galileo.py`, `.../nh.py`, `.../voyager.py`: wrapped the long lines in the `FILTER_NAMES` description and the `Raises: KeyError` bullets so every line fits in 90 columns. - `src/picmaker/options.py`: dropped the redundant `self.filter_name is not None` test in `validate()`; the field is typed `str` with default `'NONE'`, so a direct case-folded compare is enough. - `src/picmaker/pipeline.py::process_images`: added an empty-list guard in movie mode (`raise ValueError('movie mode requires at least one option_dict')`) before indexing `option_dicts[0]`. - `src/picmaker/pipeline.py::find_common_path`: replaced the length-based root check with `os.path.splitdrive` so Windows drive-only roots (`'C:\\'`) are recognized as "no useful prefix" alongside POSIX `/`. - `src/picmaker/tiff16.py::WriteTiff16`: validate `byteorder` against `{'native', 'little', 'big'}` (case-insensitive) and raise `ValueError` otherwise — previously any non-`'little'` value silently produced a big-endian file. - `src/picmaker/io.py::read_one_image_array`: open the FITS file via `with pyfits.open(...) as hdulist:` so the handle is always closed, even when downstream HST mosaic / `obj` branches raise. Removed the now-redundant trailing `hdulist.close()`. ## Tests - `tests/test_enhance_branches.py::test_get_limits_with_footprint_filter`: replaced `hi <= 200 + 0.5` with the precise `lo == 0 and hi == 0` (the 3×3 median of a single-outlier array is uniformly 0). - `tests/test_filters_branches.py::test_filter_image_blur`: use a non-uniform input (one bright pixel against black) and assert blur actually changes pixel values, so a no-op `blur` would fail. - `tests/test_pil_utils.py::test_rescale_grayscale_returns_float_in_unit_range`: replaced the dtype/max checks with an exact `np.testing.assert_array_equal` against the precisely-computed expected normalized array (first pixel 0.0, the rest 1.0 due to the input being deliberately outside the `[0, 1]` rescale contract). - `tests/test_pipeline_branches.py::test_process_images_movie_failure_raises`: tightened `pytest.raises((OSError, Exception))` to `pytest.raises(OSError, match='Unrecognized image file format')`. - `tests/test_tiff16_branches.py`: every test now reads the file back with `ReadTiff16` and asserts pixel-wise round-trip equality via `np.testing.assert_array_equal`. Replaced the `(8, 8) or (8, 8, 1)` shape OR with a single canonical shape via `np.squeeze`. `transpose=ROTATE_90` asserts against `np.rot90(arr, 1)`; `up=True` asserts against `np.flipud(arr)`. - `tests/test_color.py::test_rgb_brackets_expression`: updated to expect a tuple (matching the new `ColorNames.lookup` normalization) and replaced the stale "eval result" comment with one that references `ast.literal_eval`. ## Fixture-recipe internal-name convention - `tests/fixture_recipes/generate_user_guide_thumbnails.py`: `HERE` / `FIXTURES` / `OUT_DIR` renamed to `_HERE` / `_FIXTURES` / `_OUT_DIR` and every consumer updated. The script is invoked only from the command line and from `regenerate_all.py`, so the underscore prefix makes their module-private status explicit. ## Validation - pytest: 471 passed. - ruff: clean. - mypy strict: clean. - bandit / vulture: clean. - `sphinx-build -W` and `sphinx-build -n`: clean (zero warnings). - pymarkdown: clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Fix test_paths Windows separator expectations `find_common_path` delegates to `os.path.commonpath`, which returns the platform-native separator. On Windows `os.path.commonpath(['/foo/bar/x', '/foo/bar/y'])` returns `'\\foo\\bar'`, not `'/foo/bar'`, so the two `TestFindCommonPath` assertions that hard-coded forward slashes were failing on the windows-latest CI cell. Wrap the expected values in `os.path.normpath(...)` so they match the platform separator the function actually returns. The behaviour is unchanged on POSIX (`os.path.normpath('/foo/bar') == '/foo/bar'`). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address reviewer findings (round 2) Verified each comment against the current code. Every item the reviewer flagged was still present. Notable verification correction: ``test_get_limits_trim_zeros_with_mask`` — the reviewer suggested `hi == 100, lo == 99`. Tracing `get_limits` shows the actual values are `lo == 100, hi == 101` (after the all-zero exterior is trimmed, the inner 6x6 is uniform 100 except for one masked cell, so `array_min == array_max == 100` and the equal-values integer branch returns `(value, value + 1)`). The exact-equality assertions in the patch use the actual values. ## Documentation - `docs/dev/module_layout.rst`: inline-literal API symbols in the BC-shim paragraph (`ColorNames`, `ReadTiff16`, `WriteTiff16`, `VicarError`, `VicarImage`, `GALILEO_SSI_DICT`, `GALILEO_SSI_NAMES`, `NH_MVIC_DICT`, `VOYAGER_ISS_DICT`) replaced with Sphinx `:class:` / `:func:` / `:data:` roles. - `docs/dev/pipeline.rst`: the bare `fits.open` reference in the reader-cascade paragraph now uses `:func:`astropy.io.fits.open``. The bare `tint_for` and `write_pil` mentions in the "Color, filter, and PIL bridges" paragraph were promoted to cross-references (`:func:`picmaker.instruments.cassini.tint_for`` as the canonical concrete example, and a `~picmaker.pil_utils.write_pil` back-reference). - `docs/user_guide.rst`: three "Colour used for..." table rows under the enhancement options changed to American "Color used for...". ## Tests - `tests/test_enhance_branches.py`: * `test_get_limits_trim_zeros_with_mask`: the relational `lo == hi - 1` is replaced with explicit `lo == 100, hi == 101`. * `test_get_limits_with_trim`: the computed `lo == arr[4:-4, 4:-4].min() - 0.5` style assertions are replaced with literal `lo == 67.5, hi == 187.5` (the inner 8x8 block of `arange(256).reshape(16, 16)` runs from 68..187). * `test_apply_colormap_named_two_stop`: shape-only assertion is replaced with `np.testing.assert_allclose` against the analytically-computed expected RGB volume (red channel `1 - arr/7`, blue channel `arr/7`, green channel zero) plus explicit dtype and shape checks. - `tests/test_pipeline_branches.py`: the two inline `import logging` statements are removed; the module-level stdlib import block now has a single `import logging` placed alphabetically with the other stdlib imports. ## Fixture recipe - `tests/fixture_recipes/generate_user_guide_thumbnails.py`: the `_generate_one` helper's `ext` and `out_dir` parameters are now keyword-only (`def _generate_one(slug, fixture_name, kwargs, *, ext, out_dir)`); the single call site in `main()` passes them by name. ## Validation - pytest: 471 passed. - ruff: clean. - mypy strict: clean. - bandit / vulture: clean. - `sphinx-build -W` and `sphinx-build -n`: clean (zero warnings). - pymarkdown: clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address reviewer findings (round 3) Tighten typing surface in picmaker.io, expose the public API in the docs, and correct a stale comment that mis-described mypy's coverage of tests/. - src/picmaker/io.py: introduce ObjectSelector type alias and replace Any in read_image_array / read_one_image_array / read_pds_labeled_image_array signatures with concrete unions (str/Sequence for filename, str|PathLike|None for labelfile). Narrow read_pil's return to Image.Image | list[Image.Image] and read_array's to NDArray[Any] (dtype genuinely varies); bridge the still-Any array_to_pil / pil_to_array helpers with typing.cast. Replace the broad TypeError handler in read_pds_labeled_image_array with an early isinstance check that preserves the original "Invalid index type" message. - tests/test_io_cascade.py, tests/test_io_extra.py: assert isinstance(img, Image.Image) where the tests treat read_pil's result as a single PIL image, narrowing the new union. - docs/module.rst: add a "Public API" section above the leaf-module sections that lis…
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch is the second half of the rms-picmaker modernization. It
sits on top of
picmaker_rewrite(which split the 3,493-line god moduleinto per-concern leaves) and finishes the job: turns on every quality
gate that was scaffolded but left off, makes the test suite enforce
≥90% coverage, switches PyPI publishing to OIDC, fills out the User
Guide and Developer Guide, and applies a modernization pass over the
remaining legacy hot spots in the package.
96 files changed; +6,409 / −816.
Closes #10 (grayscale
rescale=Truewas ignored bypil_to_arrayfor'L'mode PIL images).Source modernization
picmaker.options.PicmakerOptions— centralizes the ~45CLI/library knobs and their mutex / value-validity invariants in one
dataclass. Both
cli._normalize_and_validateandpipeline.images_to_picsbuild aPicmakerOptionsfrom their kwargsand call
.validate(), so the CLI and the library agree on everyrule.
mypy --strictis clean onboth
src/andtests/.io.pyin particular replaces the legacyAnysurface with concrete unions:str | Sequence[str | os.PathLike[str]]for multi-file inputs, anObjectSelectoralias(
int | str | list[…] | tuple[…] | None) for PDS / FITS objectselection,
NDArray[Any]returns where the dtype genuinely varies,and a
ReadResultNamedTuple replacing the bare 3-tuple. Thereader cascade chains every per-format failure into an
ExceptionGroupon the finalOSErrorso callers can inspect whateach branch rejected.
io.py/enhance.py/geometry.py/pipeline.py/cli.py/pil_utils.py/colornames.py/tiff16.pyand theinstruments/subpackage now follow thecoding-standards rules in
.cursor/rules/python_best_practices.mdc(line length 100, Google-style docstrings,
pathliboveros.path, no mutable defaults, raise-from on chained exceptions,__all__+py.typed).handling in the PIL bridge, fixed the Windows-separator bug in
find_common_path, hardened the PDS3 reader to iteratelabel.dictdirectly, repaired error-cause chaining in the readercascade, and tightened color-name parsing.
target-version = "py311"; classifiers and CI matrix updated.Test suite
pyproject.tomlratchetsfail_underfrom 0 → 90. The full suite runs in ~10s underpytest-xdist.(
test_cli_unit.py,test_io_cascade.py,test_io_extra.py,test_pipeline.py,test_pipeline_branches.py,test_enhance_branches.py,test_geometry_branches.py,test_pil_utils_branches.py,test_filters_branches.py,test_tiff16_branches.py,test_instruments_branches.py,test_pds3_reader.py,test_pds3_reader_branches.py,test_package_init.py) plus targeted regression tests for thebug fixes listed above.
test_user_guide_documents_every_cli_flagdiffs
tests/fixtures/.baseline-flags.txtagainstdocs/user_guide.rst; adding a CLI flag without documenting itfails CI.
filterwarnings = ["error", …]inpyproject.tomlsurfaces unexpected runtime warnings as testfailures, with a single targeted
ignore::entry for Pillow 12'sImage.Image.getdatadeprecation (tracked in Adopt pytest filterwarnings = ['error']; track Pillow get_flattened_data migration #9).CI and release
.github/workflows/run-tests.ymlturned back on forpull_request/push/ weeklyschedule/workflow_dispatch.ruff check,mypy src tests,bandit,vulture,sphinx-build -W,pymarkdown scan, andpip-audit .(the dottedform scans the project's declared deps through pyproject
resolution, not the runner's own pip).
ubuntu-latest,macos-latest,windows-latest× Python3.11,3.12,3.13.generate_snapshots.py+git diff --exit-code) runs onubuntu-latest+3.13to catch fixturedrift.
ubuntu-latest+3.13only, skipped onforks.
.github/workflows/publish_to_pypi.ymlandpublish_to_test_pypi.ymladdpermissions: id-token: write,drop the
password: ${{ secrets.…_API_TOKEN }}lines, and bind eachjob to its GitHub Environment (
pypi/testpypi).invalid-publisheruntil the trusted publisher is configured onthe PyPI side.
CONTRIBUTING.md/ the Developer Guide releasingpage have the PyPI + TestPyPI + GitHub Environments setup steps.
Documentation
docs/user_guide.rst) — twelve sections coveringoverview, installation (
pipandpipx), quick start, command-linereference (one bullet list per
--helpgroup), supported inputformats, supported instruments and filters (Cassini ISS, Voyager
ISS, Galileo SSI, HST WFC3/ACS/WFPC2/NICMOS, New Horizons MVIC),
output formats, enhancement and geometry pipelines, the
--versions FILEform, programmatic usage, and a troubleshootingreference. Written for end users; intentionally free of code-level
references (no module paths, function names, exception types, or
return-value shapes).
docs/developer_guide.rst+docs/dev/) —six per-concern pages: repository overview (with the development
environment setup recipe — clone, venv at
./venv,pip install -e ".[dev]",bash scripts/run-all-checks.sh), module-by-moduledescription, pipeline walk-through with a client-side Mermaid SVG
flowchart, test-suite guide, adding-an-instrument tutorial, and
release process.
docs/module.rst) — top-level Public APIsection (
autofunction/autoclassfor every name inpicmaker.__all__, organized by topic) above the leaf-modulesections, which remain the authoritative per-symbol source.
docs/conf.py— Mermaid configured withmermaid_output_format = 'raw'so the pipeline diagram is renderedclient-side as inline SVG (natively zoomable, no
mmdcbinaryrequired for CI or ReadTheDocs).
Test plan
bash scripts/run-all-checks.sh -sclean (ruff, mypy, pytest,pyroma, bandit, vulture, sphinx -W, pymarkdown).
pytestreports 472 passed.coverage report≥ 90% (gate enforced bytool.coverage.report.fail_under = 90).sphinx-build -W -b html docs docs/_buildsucceeds with theUser Guide, Developer Guide, and Module reference all wired in.
pip-audit .clean against project deps.3.12 / 3.13.
and TestPyPI per the Developer Guide's releasing page before
tagging the first OIDC-published release.
publish_to_test_pypi.ymlandconfirm OIDC upload succeeds without
TEST_PYPI_API_TOKEN.🤖 Generated with Claude Code